Repository navigation
feat(chat_with_data): NL dataset querying + anomaly detection - #4148
Sathvik-1007 wants to merge 2 commits into
Conversation
- Cargo.toml: sqlparser, whatlang, nnnoiseless, strsim, lettre async - util.rs: uuid_v4(), now_epoch() shared helpers - about_app: UserContent privacy variant, 6 capability catalog entries - docs: capability plan + full API reference
9-file domain: types, engine (in-memory execution), sql_gen (NL->SQL + sqlparser AST safety), db_connector (SQLite read-only), anomaly (z-score detection), webhooks (async notifications), rpc, schemas. E2E: register_dataset -> query -> list -> generate_insight -> scan_anomalies.
📝 WalkthroughWalkthroughThe PR adds the ChangesChat-with-data capability
Sequence Diagram(s)sequenceDiagram
participant Engine as chat_with_data::engine
participant SQLGen as chat_with_data::sql_gen
participant DB as chat_with_data::db_connector
Engine->>SQLGen: generate_sql_for_question(table_name, columns, question)
SQLGen-->>Engine: GeneratedSql
Engine->>SQLGen: is_safe_query(sql)
alt SQLite dataset
Engine->>DB: execute_sqlite_query(db_path, sql)
DB-->>Engine: rows
else In-memory dataset
Engine->>Engine: execute_in_memory(dataset_id, sql)
end
Engine-->>Engine: QueryResult
sequenceDiagram
participant Engine as chat_with_data::engine
participant Anomaly as chat_with_data::anomaly
participant Webhooks as chat_with_data::webhooks
Engine->>Anomaly: detect_combined(values, z_threshold, iqr_k)
Anomaly-->>Engine: AnomalyReport
Engine->>Webhooks: notify_insight(insight, AnomalyDetected)
Webhooks-->>Webhooks: POST matching webhook URLs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7c6caecdd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .await; | ||
| let draft_r = assert_no_jsonrpc_error(&draft, "operator_inbox_generate_draft"); |
There was a problem hiding this comment.
The newly added fragment starts after the previous test has already closed, so this .await and the following draft assertions are at module scope. Any Rust test/CI run that includes tests/json_rpc_e2e.rs will fail to parse this integration test target before reaching the new chat-with-data test.
Useful? React with 👍 / 👎.
| how_to: "Start a voice session via RPC: openhuman.voice_assistant_start_session.", | ||
| status: CapabilityStatus::Beta, |
There was a problem hiding this comment.
This new Beta catalog entry, and the similar guided_flows/live_captions/voice_actions/operator_inbox entries below it, points users at RPC methods that are not registered anywhere in the controller registry. In the current tree, invoking openhuman.voice_assistant_start_session and the other advertised methods returns unknown-method errors, so the about_app catalog now exposes broken user-facing how-to steps; these should be marked coming soon or backed by registered controllers before being listed as Beta.
Useful? React with 👍 / 👎.
| columns: Vec<String>, | ||
| row_count: u64, | ||
| ) -> Result<DatasetMeta, String> { | ||
| let id = format!("ds-{}", name.to_lowercase().replace(' ', "_")); |
There was a problem hiding this comment.
When callers register a dataset with source: "sqlite", this always creates an id like ds-sales, but query_dataset only attempts real SQLite execution when the id starts with sqlite:. As a result, every SQLite dataset registered through the new RPC path skips db_connector and falls back to an empty in-memory store, so database-backed queries never execute against the SQLite file.
Useful? React with 👍 / 👎.
| arg_list.args.iter().find_map(|a| match a { | ||
| FunctionArg::Unnamed(FunctionArgExpr::Expr( | ||
| Expr::Identifier(ident), | ||
| )) => Some(ident.value.to_lowercase()), |
There was a problem hiding this comment.
For in-memory aggregates, the SQL identifier is lowercased before looking it up in rows that ingest_rows stored with the caller's original keys. Any dataset with mixed-case headers such as Amount will generate AVG("Amount") but then look for amount, returning NULL (no matching column data) despite valid ingested values.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
src/openhuman/chat_with_data/rpc.rs-11-16 (1)
11-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unknown
sourcevalues instead of coercing them to CSV.A typo currently registers the dataset as
DataSource::Csvand still returns success. Since the schema advertises a closed set (csv|json|sqlite|api), invalid values should fail fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/rpc.rs` around lines 11 - 16, The `get("source")` handling in `rpc.rs` is too permissive because unknown values currently fall through to `DataSource::Csv`, which hides typos and returns success. Update the source parsing logic in the `source` match to treat only the advertised closed set (`csv`, `json`, `sqlite`, `api`) as valid and return an error for anything else, so the RPC fails fast instead of coercing invalid input. Keep the fix localized to the `source` mapping used during dataset registration.src/openhuman/chat_with_data/engine.rs-132-134 (1)
132-134: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFix the execution caveat for SQLite results.
Successful SQLite execution currently reports
"Executed against in-memory dataset", which misleads RPC/API consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/engine.rs` around lines 132 - 134, The execution caveat in the chat_with_data result path is using the in-memory dataset message even when SQLite execution succeeds. Update the caveat selection logic in the engine.rs result assembly around execution_result and generated.is_valid so SQLite-backed successful runs emit a SQLite-specific caveat instead of "Executed against in-memory dataset", and keep the in-memory message only for the in-memory execution branch.src/openhuman/chat_with_data/db_connector.rs-39-42 (1)
39-42: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid logging full local database paths.
db_pathcan expose usernames or sensitive filesystem layout in logs. Log a redacted basename/hash instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/db_connector.rs` around lines 39 - 42, The debug log in the sqlite query path is exposing the full db_path, which can leak sensitive local filesystem details. Update the logging in the sqlite query execution flow to redact the path before emitting it, using a basename or stable hash instead of the raw value. Keep the change localized to the db connector’s query logging near the debug! call so the SQL length context remains while the path is sanitized.src/openhuman/chat_with_data/webhooks.rs-191-199 (1)
191-199: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSerialize
severityas a number, not a debug string.Webhook consumers will receive
"0.7"instead of0.7, which breaks numeric comparisons and schema expectations.Proposed fix
- "severity": format!("{:?}", insight.severity), + "severity": insight.severity,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/webhooks.rs` around lines 191 - 199, The webhook payload in the `payload` निर्माण inside `webhooks.rs` is serializing `insight.severity` with debug formatting, which turns the numeric value into a string. Update the `payload` construction to emit `severity` as a numeric JSON value instead of a formatted debug string, using the existing `insight.severity` field directly so consumers receive a number and not text.docs/boost-vc-capability-plan.md-56-65 (1)
56-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify whether this file is aspirational or current-state documentation.
These non-goals say v1 has no speaker diarization, no real email transport, no real SQL execution, and no persistence, but
docs/VC_CAPABILITIES.mdin the same PR describes those exact capabilities as implemented. One of the two docs is stale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/boost-vc-capability-plan.md` around lines 56 - 65, Clarify the intent of docs/boost-vc-capability-plan.md by updating the Explicit Non-Goals section to state whether it is aspirational v1 planning or current-state behavior, and make it consistent with docs/VC_CAPABILITIES.md. Use the section header and bullets under Explicit Non-Goals to either reframe them as future targets or remove/adjust any items that contradict implemented capabilities such as diarization, email transport, SQL execution, and persistence.docs/VC_CAPABILITIES.md-286-287 (1)
286-287: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the hard-coded
src/core/all.rsline references.This section points to lines 252–265, but in the supplied branch the new
chat_with_datawiring is at Lines 308-310 and 473-474. Referencingbuild_registered_controllers/build_declared_controller_schemasinstead will stay accurate as the file shifts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/VC_CAPABILITIES.md` around lines 286 - 287, Update the documentation reference that still hard-codes `src/core/all.rs` line numbers, since those positions have shifted with the new `chat_with_data` wiring. Replace the line-specific mention with symbol-based references to `build_registered_controllers` and `build_declared_controller_schemas` in `src/core/all.rs` so the capability docs stay accurate as the file changes.
🧹 Nitpick comments (7)
tests/json_rpc_e2e.rs (1)
13428-13518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the lifecycle assertions and close the loop with deletion.
Right now this only proves each RPC returned
ok: true. It will not catch regressions wherelist_datasetsomits the registered dataset,generate_insightreferences the wrong dataset, orscan_anomaliesreturns an unexpected payload shape. Please assert the propagateddataset_idshows up downstream, and finish the lifecycle withdelete_datasetso that path is covered too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/json_rpc_e2e.rs` around lines 13428 - 13518, The chat-with-data lifecycle test only checks ok:true responses and misses propagation/cleanup validation. In the test flow around post_json_rpc, assert the registered dataset_id is present in the downstream list_datasets result, verify generate_insight and scan_anomalies return payloads tied to that same dataset_id and expected shape, and add a delete_dataset call at the end so the full register-query-list-insight-scan-delete lifecycle is covered.src/openhuman/chat_with_data/schemas.rs (1)
304-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDelegate schema handlers to
ops.rs, notrpc.rs.This wiring keeps the transport layer as the domain boundary instead of the canonical ops layer the repo expects for new controller migrations. As per coding guidelines, "
src/openhuman/*/{mod,schemas}.rs: Controller migration requires ... define schemas with handlers inschemas.rsdelegating toops.rs" and "src/openhuman/*/: Canonical Rust module shape ...ops.rs(business logic)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/schemas.rs` around lines 304 - 329, The schema handler wiring is pointing to the wrong layer: the handler functions in schemas.rs are delegating into rpc.rs instead of the canonical ops.rs boundary. Update the h_register, h_query, h_insight, h_list_ds, h_list_ins, h_get, h_ingest, h_scan, and h_delete adapters to call the corresponding ops.rs functions so schemas.rs only routes transport requests to business logic through ops.rs.Source: Coding guidelines
src/openhuman/chat_with_data/rpc.rs (1)
95-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon't swallow LLM/config/provider failures without debug logs.
Both helpers convert config load, provider creation, and model-call failures into
Nonevia.ok()?, so the fallback path becomes opaque in production. As per coding guidelines, "Add debug logging to entry/exit, branches, external calls, retries/timeouts, state transitions, and errors usinglog/tracingatdebug/tracelevel in Rust."Also applies to: 154-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/rpc.rs` around lines 95 - 125, The `chat_with_data` flow is swallowing failures from `load_config_with_timeout`, `create_chat_provider`, and `provider.chat_with_system` by converting them to `None` with `.ok()?`, which makes production fallback behavior opaque. Update the relevant helper(s) in `rpc.rs` to emit `debug`/`trace` logs around each external call and failure branch, using the existing `chat_with_data` context and the `provider/chat_with_system` path, so config-load, provider-creation, and LLM-call errors are visible before returning `None`. Apply the same logging pattern to the other affected helper mentioned in the review so both entry/exit and error paths are observable without changing the return type.Source: Coding guidelines
src/openhuman/chat_with_data/db_connector.rs (1)
88-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd debug logs around schema and table-list DB calls.
get_sqlite_schemaandlist_sqlite_tablesperform external SQLite calls without entry/exit/error branch logging. As per coding guidelines, Rust code undersrc/openhuman/**/*.rsshould add debug logging for entry/exit, external calls, state transitions, and errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/db_connector.rs` around lines 88 - 136, Add debug logging to the SQLite helper functions so their database interactions are traceable. In get_sqlite_schema and list_sqlite_tables, log entry before opening the connection, log before and after each external SQLite call (Connection::open_with_flags, prepare, query_map/collection), and log error branches with the existing LOG_PREFIX context. Use the function names get_sqlite_schema and list_sqlite_tables to place the logs so the entry/exit/state transitions are covered without changing behavior.Source: Coding guidelines
src/openhuman/chat_with_data/sql_gen.rs (1)
462-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove tests out to keep the module under the size limit.
This file is 579 lines; moving the test module to
sql_gen_tests.rsbehind#[cfg(test)] mod sql_gen_tests;keeps production logic focused and satisfies the repository limit. As per coding guidelines, “Rust modules must be ≤ ~500 lines in size.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/sql_gen.rs` around lines 462 - 579, The test module at the bottom of sql_gen.rs is pushing the module over the size limit. Move the entire #[cfg(test)] mod tests block into a separate sql_gen_tests.rs module and keep sql_gen.rs focused on production code. Update the module wiring by replacing the inline tests with a #[cfg(test)] mod sql_gen_tests; declaration, preserving all existing test functions that reference validate_sql, generate_sql_for_question, is_safe_query, and extract_time_filter.Source: Coding guidelines
src/openhuman/chat_with_data/engine.rs (1)
418-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove tests out to keep the module under the size limit.
This file is 572 lines; moving tests to
engine_tests.rsbehind#[cfg(test)] mod engine_tests;brings the module back under the repository limit. As per coding guidelines, “Rust modules must be ≤ ~500 lines in size.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/engine.rs` around lines 418 - 572, The tests block in the `tests` module is pushing `engine.rs` over the module size limit. Move these test cases into a separate `engine_tests.rs` test module and expose them with `#[cfg(test)] mod engine_tests;` from the `engine` module so the main module stays under the ~500 line guideline. Keep the existing test functions and references like `get_dataset`, `register_dataset`, `query_dataset`, `generate_insight`, `delete_dataset`, and `scan_all_datasets_for_anomalies` in the new test file.Source: Coding guidelines
docs/boost-vc-capability-plan.md (1)
69-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign this architecture section with the repo’s canonical module layout.
This documents
engine.rs/rpc.rsand points readers toabout_app/catalog.rs, but the repo standard forsrc/openhuman/*/is export-onlymod.rsplusops.rs/schemas.rs/tools.rs/bus.rs, and the capability list in this PR is insrc/openhuman/about_app/catalog_data.rs. Leaving the old structure here will send follow-up work to the wrong files.As per coding guidelines,
src/openhuman/*/: "Canonical Rust module shape:mod.rs(export-focused only),types.rs(Serde types),store.rs(persistence),ops.rs(business logic),schemas.rs(RPC schemas),tools.rs(agent tools),bus.rs(event subscribers)"; andsrc/openhuman/*/mod.rs: "Rust modules must have export-focusedmod.rswith no business logic; business logic goes inops.rs."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/boost-vc-capability-plan.md` around lines 69 - 77, Update the architecture section to match the repo’s canonical `src/openhuman/*/` layout: replace the outdated `engine.rs`/`rpc.rs`/`about_app/catalog.rs` references with the export-only `mod.rs` pattern and the expected split into `types.rs`, `store.rs`, `ops.rs`, `schemas.rs`, `tools.rs`, and `bus.rs`. Use the capability list source from `src/openhuman/about_app/catalog_data.rs` when describing where the catalog data lives, and make sure the text reflects that `mod.rs` is export-focused only while business logic belongs in `ops.rs`.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/about_app/catalog_data.rs`:
- Around line 1452-1538: The Beta capability catalog currently advertises
unimplemented features, so either remove the `Capability` entries for
`voice_assistant.session`, `guided_flows.recommendation`,
`live_captions.transcript`, `voice_actions.intent`, and `operator_inbox.triage`
from `catalog_data.rs` or fully implement and register their Rust modules first.
Make sure each advertised capability has a matching source directory, RPC
handler, and `src/core/all.rs` registration before it remains in the catalog;
`chat_with_data.query` can stay as-is since it is already wired up.
In `@src/openhuman/chat_with_data/anomaly.rs`:
- Around line 54-63: Non-finite inputs are being fed into the anomaly stats
pipeline, which can poison mean/stddev/percentile calculations and suppress
detection. Update the data filtering in the anomaly flow around detect_zscore
and the other affected report/stat helpers (the sections referenced at the later
ranges) to reject or skip NaN/inf values before any aggregation, and ensure the
returned anomaly/report stats are computed only from finite samples.
In `@src/openhuman/chat_with_data/db_connector.rs`:
- Around line 33-37: The public SQLite connector validation in
execute_sqlite_query is too shallow because it only checks for a SELECT prefix,
allowing unsafe queries like UNIONs or subqueries to slip past. Reuse the
existing SQL safety validation used by is_safe_query in db_connector.rs before
executing the query, and keep the current read-only check as a defense-in-depth
layer. Make the fix in the execute_sqlite_query path so direct callers are
subject to the same syntax and safety rules as other entry points.
- Around line 52-81: The SQLite query result cap is only applied after execution
via the query_map iterator, so expensive statements can still run unbounded
inside SQLite. Update the query path in db_connector.rs to enforce a hard limit
at execution time by adding a SQL LIMIT clause before prepare/query, and
consider using conn.get_interrupt_handle() with a timeout to abort long-running
queries. Keep the existing row decoding logic and the 1000-row cap, but move the
protection to the SQL/connection level in the same query execution flow.
In `@src/openhuman/chat_with_data/engine.rs`:
- Around line 262-274: The anomaly insight generation in generate_insight is
using synthetic sample_values with an injected spike at index 42, which can
produce false alerts. Replace this test data logic with values derived from the
actual dataset rows in ds so detect_combined analyzes real data only, and remove
the hardcoded outlier injection while keeping the existing
anomaly::detect_combined call intact.
- Around line 63-85: The Sqlite path currently uses ds.id for SQL generation,
which includes the full encoded dataset string instead of the real table name,
and the execution failure is only logged then ignored. Update
chat_with_data::engine to extract and pass the actual SQLite table name
separately to sql_gen::generate_sql_for_question, and in the
execute_sqlite_query branch surface the error back to the caller instead of
falling through to the generated-SQL answer when execution fails.
- Around line 332-337: The anomaly scan in the `ROW_STORE` lookup is picking a
metric with `values().next()`, which is nondeterministic because `HashMap`
iteration order is unstable. Update the logic in the `engine.rs` data scan path
to avoid choosing an arbitrary first value; instead use a deterministic
configured metric column or iterate all numeric columns explicitly in the
relevant `ROW_STORE`/scan code.
- Around line 58-80: The dataset registry lock is held for too long in
`chat_with_data` while SQL generation and SQLite execution run, which can block
other dataset operations. In `engine.rs`, clone the needed `DatasetMeta` (or the
specific fields used by `generate_sql_for_question` and the execution branch)
from `DATASETS.lock()` and then drop the lock before calling
`sql_gen::generate_sql_for_question` and `db_connector::execute_sqlite_query`;
keep the lock only around the `get(dataset_id)` access.
- Around line 196-207: The in-memory aggregation path in engine.rs is
normalizing parsed identifiers to lowercase before looking them up, but row keys
are stored with their original casing, so matching fails for columns like
Amount. Update the column resolution logic in the aggregation code near the
Expr::Identifier handling to preserve the parsed column name casing when
performing r.get lookups, or otherwise resolve against the original identifier
value consistently. Keep the fix localized to the aggregation lookup that builds
values from rows so identifiers and stored keys use the same casing.
In `@src/openhuman/chat_with_data/mod.rs`:
- Around line 8-15: The chat_with_data domain is using a custom root-module
layout instead of the canonical openhuman/* structure. Update the module
organization around mod.rs, engine, db_connector, rpc, and webhooks so the
domain follows the required mod.rs + types.rs + store.rs + ops.rs + schemas.rs +
tools.rs + bus.rs pattern, with mod.rs kept export-focused only. Move
controller/handler definitions into schemas.rs and delegate their logic into
ops.rs, and adjust or rename the existing engine/db_connector/rpc/webhooks
modules to fit the canonical roles or exports expected by the openhuman module
layout.
In `@src/openhuman/chat_with_data/rpc.rs`:
- Around line 61-76: The LLM fallback branch in chat_with_data/rpc.rs is
returning the generated SQL text instead of running it, so successful matches
can short-circuit without actual results. Update the SqlGenMethod::Fallback flow
to pass the validated llm_sql into the same read-only SQLite execution path used
elsewhere, then build the response from the query output rather than formatting
the SQL string; keep the existing validation/safety checks in place before
execution.
- Around line 204-220: The row parser in ingest_rows is dropping any non-numeric
cell by only keeping as_f64 values, which breaks the expected {col: value}
contract and can mark partially empty rows as ingested. Update the parsing in
rpc.rs around rows_val/engine::ingest_rows so it either preserves arbitrary JSON
values for each column or explicitly rejects payloads containing non-numeric
fields, and make the schema in schemas.rs match that decision. Ensure the
ingest_rows flow does not silently succeed on rows that lose data.
In `@src/openhuman/chat_with_data/schemas.rs`:
- Around line 111-124: The ControllerSchema outputs are missing fields that the
matching RPC handlers in rpc.rs actually return, so the published contract is
incomplete. Update each affected schema definition in schemas.rs to match the
payloads from the corresponding functions in rpc.rs: register_dataset, query,
generate_insight, get_dataset, scan_anomalies, and the other referenced
handlers. Make sure the outputs in the relevant ControllerSchema entries include
every returned field with the correct names and types so the registry published
from core/all.rs stays in sync.
In `@src/openhuman/chat_with_data/sql_gen.rs`:
- Around line 82-89: Escape all SQL identifiers before interpolating them into
query strings: the current string formatting in sql_gen.rs directly wraps
table/column names in quotes, which can still break on embedded quote
characters. Introduce a shared identifier-quoting helper and use it in the
SQL-building paths around the SELECT construction and the other affected
formatting sites, so table_name and column names are always safely escaped
before being inserted into SQL.
- Around line 281-289: The time-window conversion in sql_gen::parse_time_filter
can overflow when multiplying the parsed count by the suffix multiplier. Update
the logic around the `num_str.parse::<u32>()` branch to use `checked_mul` on `n`
with the computed multiplier instead of პირდაპირ `n * multiplier`, and return
`None` (or otherwise reject the filter) when the multiplication would overflow.
Keep the fix localized to the time-filter parsing path so very large inputs like
“last 4294967295 months” are safely rejected.
- Around line 431-459: `is_safe_query` currently relies on brittle string
matching and can miss non-SELECT statements that bypass the keyword list.
Replace the logic in `sql_gen::is_safe_query` with `sqlparser` parsing via
`Parser::parse_sql`, require exactly one `Statement`, accept only
`Statement::Query`, and ensure the query body is `SetExpr::Select` while
rejecting `SetExpr::SetOperation` and any non-empty `cte_tables`. If strict
select-only behavior is needed, also walk the `FROM` clause to reject derived
tables/subqueries so the validator enforces the contract structurally instead of
by keywords.
In `@src/openhuman/chat_with_data/webhooks.rs`:
- Around line 208-225: `notify_insight` currently uses `tokio::spawn`, which can
panic when called without an active runtime. Update the spawn path to use a
captured `tokio::runtime::Handle` and call `handle.spawn(...)` instead, or store
the handle in the shared state used by `webhooks.rs`. Make sure the async block
that does `resolve_and_check`, the POST request, and the logging remains
unchanged, but the task is scheduled through the runtime handle rather than the
thread-local `tokio::spawn`.
- Around line 104-124: The current pre-flight DNS check in resolve_and_check is
vulnerable to DNS rebinding because reqwest::Client::send will resolve the host
again later. Move the IP filtering into a custom reqwest::dns::Resolve
implementation that rejects private, loopback, and link-local addresses at
connection time, and wire it into the ClientBuilder in the webhook client setup.
Also ensure the reqwest client is built with no_proxy() and strict redirect
limits so the protection applies during actual requests.
---
Minor comments:
In `@docs/boost-vc-capability-plan.md`:
- Around line 56-65: Clarify the intent of docs/boost-vc-capability-plan.md by
updating the Explicit Non-Goals section to state whether it is aspirational v1
planning or current-state behavior, and make it consistent with
docs/VC_CAPABILITIES.md. Use the section header and bullets under Explicit
Non-Goals to either reframe them as future targets or remove/adjust any items
that contradict implemented capabilities such as diarization, email transport,
SQL execution, and persistence.
In `@docs/VC_CAPABILITIES.md`:
- Around line 286-287: Update the documentation reference that still hard-codes
`src/core/all.rs` line numbers, since those positions have shifted with the new
`chat_with_data` wiring. Replace the line-specific mention with symbol-based
references to `build_registered_controllers` and
`build_declared_controller_schemas` in `src/core/all.rs` so the capability docs
stay accurate as the file changes.
In `@src/openhuman/chat_with_data/db_connector.rs`:
- Around line 39-42: The debug log in the sqlite query path is exposing the full
db_path, which can leak sensitive local filesystem details. Update the logging
in the sqlite query execution flow to redact the path before emitting it, using
a basename or stable hash instead of the raw value. Keep the change localized to
the db connector’s query logging near the debug! call so the SQL length context
remains while the path is sanitized.
In `@src/openhuman/chat_with_data/engine.rs`:
- Around line 132-134: The execution caveat in the chat_with_data result path is
using the in-memory dataset message even when SQLite execution succeeds. Update
the caveat selection logic in the engine.rs result assembly around
execution_result and generated.is_valid so SQLite-backed successful runs emit a
SQLite-specific caveat instead of "Executed against in-memory dataset", and keep
the in-memory message only for the in-memory execution branch.
In `@src/openhuman/chat_with_data/rpc.rs`:
- Around line 11-16: The `get("source")` handling in `rpc.rs` is too permissive
because unknown values currently fall through to `DataSource::Csv`, which hides
typos and returns success. Update the source parsing logic in the `source` match
to treat only the advertised closed set (`csv`, `json`, `sqlite`, `api`) as
valid and return an error for anything else, so the RPC fails fast instead of
coercing invalid input. Keep the fix localized to the `source` mapping used
during dataset registration.
In `@src/openhuman/chat_with_data/webhooks.rs`:
- Around line 191-199: The webhook payload in the `payload` निर्माण inside
`webhooks.rs` is serializing `insight.severity` with debug formatting, which
turns the numeric value into a string. Update the `payload` construction to emit
`severity` as a numeric JSON value instead of a formatted debug string, using
the existing `insight.severity` field directly so consumers receive a number and
not text.
---
Nitpick comments:
In `@docs/boost-vc-capability-plan.md`:
- Around line 69-77: Update the architecture section to match the repo’s
canonical `src/openhuman/*/` layout: replace the outdated
`engine.rs`/`rpc.rs`/`about_app/catalog.rs` references with the export-only
`mod.rs` pattern and the expected split into `types.rs`, `store.rs`, `ops.rs`,
`schemas.rs`, `tools.rs`, and `bus.rs`. Use the capability list source from
`src/openhuman/about_app/catalog_data.rs` when describing where the catalog data
lives, and make sure the text reflects that `mod.rs` is export-focused only
while business logic belongs in `ops.rs`.
In `@src/openhuman/chat_with_data/db_connector.rs`:
- Around line 88-136: Add debug logging to the SQLite helper functions so their
database interactions are traceable. In get_sqlite_schema and
list_sqlite_tables, log entry before opening the connection, log before and
after each external SQLite call (Connection::open_with_flags, prepare,
query_map/collection), and log error branches with the existing LOG_PREFIX
context. Use the function names get_sqlite_schema and list_sqlite_tables to
place the logs so the entry/exit/state transitions are covered without changing
behavior.
In `@src/openhuman/chat_with_data/engine.rs`:
- Around line 418-572: The tests block in the `tests` module is pushing
`engine.rs` over the module size limit. Move these test cases into a separate
`engine_tests.rs` test module and expose them with `#[cfg(test)] mod
engine_tests;` from the `engine` module so the main module stays under the ~500
line guideline. Keep the existing test functions and references like
`get_dataset`, `register_dataset`, `query_dataset`, `generate_insight`,
`delete_dataset`, and `scan_all_datasets_for_anomalies` in the new test file.
In `@src/openhuman/chat_with_data/rpc.rs`:
- Around line 95-125: The `chat_with_data` flow is swallowing failures from
`load_config_with_timeout`, `create_chat_provider`, and
`provider.chat_with_system` by converting them to `None` with `.ok()?`, which
makes production fallback behavior opaque. Update the relevant helper(s) in
`rpc.rs` to emit `debug`/`trace` logs around each external call and failure
branch, using the existing `chat_with_data` context and the
`provider/chat_with_system` path, so config-load, provider-creation, and
LLM-call errors are visible before returning `None`. Apply the same logging
pattern to the other affected helper mentioned in the review so both entry/exit
and error paths are observable without changing the return type.
In `@src/openhuman/chat_with_data/schemas.rs`:
- Around line 304-329: The schema handler wiring is pointing to the wrong layer:
the handler functions in schemas.rs are delegating into rpc.rs instead of the
canonical ops.rs boundary. Update the h_register, h_query, h_insight, h_list_ds,
h_list_ins, h_get, h_ingest, h_scan, and h_delete adapters to call the
corresponding ops.rs functions so schemas.rs only routes transport requests to
business logic through ops.rs.
In `@src/openhuman/chat_with_data/sql_gen.rs`:
- Around line 462-579: The test module at the bottom of sql_gen.rs is pushing
the module over the size limit. Move the entire #[cfg(test)] mod tests block
into a separate sql_gen_tests.rs module and keep sql_gen.rs focused on
production code. Update the module wiring by replacing the inline tests with a
#[cfg(test)] mod sql_gen_tests; declaration, preserving all existing test
functions that reference validate_sql, generate_sql_for_question, is_safe_query,
and extract_time_filter.
In `@tests/json_rpc_e2e.rs`:
- Around line 13428-13518: The chat-with-data lifecycle test only checks ok:true
responses and misses propagation/cleanup validation. In the test flow around
post_json_rpc, assert the registered dataset_id is present in the downstream
list_datasets result, verify generate_insight and scan_anomalies return payloads
tied to that same dataset_id and expected shape, and add a delete_dataset call
at the end so the full register-query-list-insight-scan-delete lifecycle is
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e9f1081-9907-49c6-ab50-cff07c42d9c9
📒 Files selected for processing (18)
Cargo.tomldocs/VC_CAPABILITIES.mddocs/boost-vc-capability-plan.mdsrc/core/all.rssrc/openhuman/about_app/catalog_data.rssrc/openhuman/about_app/types.rssrc/openhuman/chat_with_data/anomaly.rssrc/openhuman/chat_with_data/db_connector.rssrc/openhuman/chat_with_data/engine.rssrc/openhuman/chat_with_data/mod.rssrc/openhuman/chat_with_data/rpc.rssrc/openhuman/chat_with_data/schemas.rssrc/openhuman/chat_with_data/sql_gen.rssrc/openhuman/chat_with_data/types.rssrc/openhuman/chat_with_data/webhooks.rssrc/openhuman/mod.rssrc/openhuman/util.rstests/json_rpc_e2e.rs
| // ── Boost VC capability foundations ───────────────────────────────────── | ||
| Capability { | ||
| id: "voice_assistant.session", | ||
| name: "Voice Assistant Sessions", | ||
| domain: "voice_assistant", | ||
| category: CapabilityCategory::Automation, | ||
| description: "Core/RPC voice session foundation with open STT/TTS adapters, transcript \ | ||
| handoff, and session status controls.", | ||
| how_to: "Start a voice session via RPC: openhuman.voice_assistant_start_session.", | ||
| status: CapabilityStatus::Beta, | ||
| privacy: Some(CapabilityPrivacy { | ||
| leaves_device: true, | ||
| data_kind: PrivacyDataKind::UserContent, | ||
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | ||
| }), | ||
| }, | ||
| Capability { | ||
| id: "guided_flows.recommendation", | ||
| name: "Guided Recommendation Flows", | ||
| domain: "guided_flows", | ||
| category: CapabilityCategory::Automation, | ||
| description: "Core/RPC quiz and recommendation flow engine with structured answers, \ | ||
| reusable flow definitions, and recommendation output.", | ||
| how_to: "Start a flow via RPC: openhuman.guided_flows_start_flow with flow_id.", | ||
| status: CapabilityStatus::Beta, | ||
| privacy: None, | ||
| }, | ||
| Capability { | ||
| id: "live_captions.transcript", | ||
| name: "Live Caption Transcripts", | ||
| domain: "live_captions", | ||
| category: CapabilityCategory::Automation, | ||
| description: "Core/RPC transcript pipeline for caption segments, saved transcripts, \ | ||
| summaries, and meeting-note handoff.", | ||
| how_to: "Start via RPC: openhuman.live_captions_start_transcript with source.", | ||
| status: CapabilityStatus::Beta, | ||
| privacy: Some(CapabilityPrivacy { | ||
| leaves_device: true, | ||
| data_kind: PrivacyDataKind::UserContent, | ||
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | ||
| }), | ||
| }, | ||
| Capability { | ||
| id: "voice_actions.intent", | ||
| name: "Voice Action Recognition", | ||
| domain: "voice_actions", | ||
| category: CapabilityCategory::Automation, | ||
| description: "Core/RPC voice-command recognizer that maps utterances to controller-backed \ | ||
| or skill-backed actions with visible status metadata for app callers.", | ||
| how_to: "Recognize via RPC: openhuman.voice_actions_recognize with utterance.", | ||
| status: CapabilityStatus::Beta, | ||
| privacy: Some(CapabilityPrivacy { | ||
| leaves_device: true, | ||
| data_kind: PrivacyDataKind::UserContent, | ||
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | ||
| }), | ||
| }, | ||
| Capability { | ||
| id: "operator_inbox.triage", | ||
| name: "Operator Inbox Triage", | ||
| domain: "operator_inbox", | ||
| category: CapabilityCategory::Automation, | ||
| description: "Core/RPC operator inbox foundation with IMAP fetching, SMTP reply sending, \ | ||
| message triage, contextual draft replies, and follow-up scheduling.", | ||
| how_to: "Triage via RPC: openhuman.operator_inbox_triage_message with sender/subject/body; fetch via openhuman.operator_inbox_fetch_inbox.", | ||
| status: CapabilityStatus::Beta, | ||
| privacy: Some(CapabilityPrivacy { | ||
| leaves_device: true, | ||
| data_kind: PrivacyDataKind::UserContent, | ||
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | ||
| }), | ||
| }, | ||
| Capability { | ||
| id: "chat_with_data.query", | ||
| name: "Chat-with-Data Analytics", | ||
| domain: "chat_with_data", | ||
| category: CapabilityCategory::Automation, | ||
| description: "Core/RPC natural-language analytics over registered datasets with \ | ||
| SQL validation, anomaly detection, trend analysis, and summaries.", | ||
| how_to: "Query via RPC: openhuman.chat_with_data_query with dataset_id and question.", | ||
| status: CapabilityStatus::Beta, | ||
| privacy: Some(CapabilityPrivacy { | ||
| leaves_device: true, | ||
| data_kind: PrivacyDataKind::UserContent, | ||
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | ||
| }), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
domains='voice_assistant|guided_flows|live_captions|voice_actions|operator_inbox|chat_with_data'
echo "== Domain directories =="
fd -td 'voice_assistant|guided_flows|live_captions|voice_actions|operator_inbox|chat_with_data' src/openhuman
echo
echo "== Controller/schema registration symbols =="
rg -nP "all_(${domains})_(registered_controllers|controller_schemas)" src/openhuman src/core/all.rs
echo
echo "== Namespace/RPC references =="
rg -nP "\"(${domains})\"\\s*=>|openhuman\\.(${domains})_" src/core/all.rs src/openhuman testsRepository: tinyhumansai/openhuman
Length of output: 2061
Remove unimplemented capability entries or implement the required Rust modules.
The catalog exposes five Beta capabilities (voice_assistant, guided_flows, live_captions, voice_actions, operator_inbox) that lack corresponding Rust implementation directories and RPC controller registration in src/core/all.rs.
Only chat_with_data is fully wired. The others have no source directories (e.g., src/openhuman/voice_assistant/) and their RPC endpoints (e.g., openhuman.voice_assistant_start_session) will fail in the runtime.
This violates the required feature workflow: "Specify → prove in Rust → prove over RPC → surface in UI".
Options:
- Remove these entries from
src/openhuman/about_app/catalog_data.rsuntil the feature is fully implemented and registered. - Or, implement the missing modules and wire them in
src/core/all.rsbefore surfacing them as Beta features.
- Capability {
- id: "voice_assistant.session", ...
- },
- Capability {
- id: "guided_flows.recommendation", ...
- },
- ... (all 5 unimplemented domains)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ── Boost VC capability foundations ───────────────────────────────────── | |
| Capability { | |
| id: "voice_assistant.session", | |
| name: "Voice Assistant Sessions", | |
| domain: "voice_assistant", | |
| category: CapabilityCategory::Automation, | |
| description: "Core/RPC voice session foundation with open STT/TTS adapters, transcript \ | |
| handoff, and session status controls.", | |
| how_to: "Start a voice session via RPC: openhuman.voice_assistant_start_session.", | |
| status: CapabilityStatus::Beta, | |
| privacy: Some(CapabilityPrivacy { | |
| leaves_device: true, | |
| data_kind: PrivacyDataKind::UserContent, | |
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | |
| }), | |
| }, | |
| Capability { | |
| id: "guided_flows.recommendation", | |
| name: "Guided Recommendation Flows", | |
| domain: "guided_flows", | |
| category: CapabilityCategory::Automation, | |
| description: "Core/RPC quiz and recommendation flow engine with structured answers, \ | |
| reusable flow definitions, and recommendation output.", | |
| how_to: "Start a flow via RPC: openhuman.guided_flows_start_flow with flow_id.", | |
| status: CapabilityStatus::Beta, | |
| privacy: None, | |
| }, | |
| Capability { | |
| id: "live_captions.transcript", | |
| name: "Live Caption Transcripts", | |
| domain: "live_captions", | |
| category: CapabilityCategory::Automation, | |
| description: "Core/RPC transcript pipeline for caption segments, saved transcripts, \ | |
| summaries, and meeting-note handoff.", | |
| how_to: "Start via RPC: openhuman.live_captions_start_transcript with source.", | |
| status: CapabilityStatus::Beta, | |
| privacy: Some(CapabilityPrivacy { | |
| leaves_device: true, | |
| data_kind: PrivacyDataKind::UserContent, | |
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | |
| }), | |
| }, | |
| Capability { | |
| id: "voice_actions.intent", | |
| name: "Voice Action Recognition", | |
| domain: "voice_actions", | |
| category: CapabilityCategory::Automation, | |
| description: "Core/RPC voice-command recognizer that maps utterances to controller-backed \ | |
| or skill-backed actions with visible status metadata for app callers.", | |
| how_to: "Recognize via RPC: openhuman.voice_actions_recognize with utterance.", | |
| status: CapabilityStatus::Beta, | |
| privacy: Some(CapabilityPrivacy { | |
| leaves_device: true, | |
| data_kind: PrivacyDataKind::UserContent, | |
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | |
| }), | |
| }, | |
| Capability { | |
| id: "operator_inbox.triage", | |
| name: "Operator Inbox Triage", | |
| domain: "operator_inbox", | |
| category: CapabilityCategory::Automation, | |
| description: "Core/RPC operator inbox foundation with IMAP fetching, SMTP reply sending, \ | |
| message triage, contextual draft replies, and follow-up scheduling.", | |
| how_to: "Triage via RPC: openhuman.operator_inbox_triage_message with sender/subject/body; fetch via openhuman.operator_inbox_fetch_inbox.", | |
| status: CapabilityStatus::Beta, | |
| privacy: Some(CapabilityPrivacy { | |
| leaves_device: true, | |
| data_kind: PrivacyDataKind::UserContent, | |
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | |
| }), | |
| }, | |
| Capability { | |
| id: "chat_with_data.query", | |
| name: "Chat-with-Data Analytics", | |
| domain: "chat_with_data", | |
| category: CapabilityCategory::Automation, | |
| description: "Core/RPC natural-language analytics over registered datasets with \ | |
| SQL validation, anomaly detection, trend analysis, and summaries.", | |
| how_to: "Query via RPC: openhuman.chat_with_data_query with dataset_id and question.", | |
| status: CapabilityStatus::Beta, | |
| privacy: Some(CapabilityPrivacy { | |
| leaves_device: true, | |
| data_kind: PrivacyDataKind::UserContent, | |
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | |
| }), | |
| }, | |
| // ── Boost VC capability foundations ───────────────────────────────────── | |
| Capability { | |
| id: "chat_with_data.query", | |
| name: "Chat-with-Data Analytics", | |
| domain: "chat_with_data", | |
| category: CapabilityCategory::Automation, | |
| description: "Core/RPC natural-language analytics over registered datasets with \ | |
| SQL validation, anomaly detection, trend analysis, and summaries.", | |
| how_to: "Query via RPC: openhuman.chat_with_data_query with dataset_id and question.", | |
| status: CapabilityStatus::Beta, | |
| privacy: Some(CapabilityPrivacy { | |
| leaves_device: true, | |
| data_kind: PrivacyDataKind::UserContent, | |
| destinations: &["OpenHuman backend", "TinyHumans Neocortex"], | |
| }), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/about_app/catalog_data.rs` around lines 1452 - 1538, The Beta
capability catalog currently advertises unimplemented features, so either remove
the `Capability` entries for `voice_assistant.session`,
`guided_flows.recommendation`, `live_captions.transcript`,
`voice_actions.intent`, and `operator_inbox.triage` from `catalog_data.rs` or
fully implement and register their Rust modules first. Make sure each advertised
capability has a matching source directory, RPC handler, and `src/core/all.rs`
registration before it remains in the catalog; `chat_with_data.query` can stay
as-is since it is already wired up.
Source: Coding guidelines
| pub fn detect_zscore(data: &[f64], threshold: f64) -> Vec<Anomaly> { | ||
| if !threshold.is_finite() || threshold <= 0.0 { | ||
| return vec![]; | ||
| } | ||
| if data.len() < 3 { | ||
| return vec![]; | ||
| } | ||
|
|
||
| let mean = mean(data); | ||
| let std = std_dev(data, mean); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter or reject non-finite values before computing stats.
A single NaN/inf value makes mean/stddev/percentiles unreliable and can suppress anomaly detection while surfacing invalid report stats over RPC.
Proposed fix
pub fn detect_zscore(data: &[f64], threshold: f64) -> Vec<Anomaly> {
+ let data: Vec<f64> = data.iter().copied().filter(|v| v.is_finite()).collect();
if !threshold.is_finite() || threshold <= 0.0 {
return vec![];
}
- if data.len() < 3 {
+ if data.len() < 3 {
return vec![];
}Also applies to: 107-112, 187-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/chat_with_data/anomaly.rs` around lines 54 - 63, Non-finite
inputs are being fed into the anomaly stats pipeline, which can poison
mean/stddev/percentile calculations and suppress detection. Update the data
filtering in the anomaly flow around detect_zscore and the other affected
report/stat helpers (the sections referenced at the later ranges) to reject or
skip NaN/inf values before any aggregation, and ensure the returned
anomaly/report stats are computed only from finite samples.
| // Validate read-only (defense in depth — sqlparser already validates). | ||
| let lower = sql.trim().to_lowercase(); | ||
| if !lower.starts_with("select") { | ||
| return Err("only SELECT queries allowed on database connector".into()); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reuse the SQL safety validator in the public connector.
execute_sqlite_query accepts any string starting with SELECT, so direct callers can bypass is_safe_query restrictions for UNION and subqueries. Validate syntax and safety here too.
Proposed fix
let lower = sql.trim().to_lowercase();
if !lower.starts_with("select") {
return Err("only SELECT queries allowed on database connector".into());
}
+ super::sql_gen::validate_sql(sql)?;
+ super::sql_gen::is_safe_query(sql)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validate read-only (defense in depth — sqlparser already validates). | |
| let lower = sql.trim().to_lowercase(); | |
| if !lower.starts_with("select") { | |
| return Err("only SELECT queries allowed on database connector".into()); | |
| } | |
| // Validate read-only (defense in depth — sqlparser already validates). | |
| let lower = sql.trim().to_lowercase(); | |
| if !lower.starts_with("select") { | |
| return Err("only SELECT queries allowed on database connector".into()); | |
| } | |
| super::sql_gen::validate_sql(sql)?; | |
| super::sql_gen::is_safe_query(sql)?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/chat_with_data/db_connector.rs` around lines 33 - 37, The
public SQLite connector validation in execute_sqlite_query is too shallow
because it only checks for a SELECT prefix, allowing unsafe queries like UNIONs
or subqueries to slip past. Reuse the existing SQL safety validation used by
is_safe_query in db_connector.rs before executing the query, and keep the
current read-only check as a defense-in-depth layer. Make the fix in the
execute_sqlite_query path so direct callers are subject to the same syntax and
safety rules as other entry points.
| let mut stmt = conn | ||
| .prepare(sql) | ||
| .map_err(|e| format!("{LOG_PREFIX} prepare: {e}"))?; | ||
| let col_count = stmt.column_count(); | ||
| let col_names: Vec<String> = (0..col_count) | ||
| .map(|i| stmt.column_name(i).unwrap_or("?").to_string()) | ||
| .collect(); | ||
|
|
||
| let rows = stmt | ||
| .query_map([], |row| { | ||
| let mut map = HashMap::new(); | ||
| for (i, name) in col_names.iter().enumerate() { | ||
| let val: String = row | ||
| .get::<_, rusqlite::types::Value>(i) | ||
| .map(|v| match v { | ||
| rusqlite::types::Value::Null => "NULL".into(), | ||
| rusqlite::types::Value::Integer(n) => n.to_string(), | ||
| rusqlite::types::Value::Real(f) => f.to_string(), | ||
| rusqlite::types::Value::Text(s) => s, | ||
| rusqlite::types::Value::Blob(_) => "<blob>".into(), | ||
| }) | ||
| .unwrap_or_else(|_| "?".into()); | ||
| map.insert(name.clone(), val); | ||
| } | ||
| Ok(map) | ||
| }) | ||
| .map_err(|e| format!("{LOG_PREFIX} query: {e}"))? | ||
| .take(1000) // Limit rows returned. | ||
| .collect::<Result<Vec<_>, _>>() | ||
| .map_err(|e| format!("{LOG_PREFIX} row decode: {e}"))?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In rusqlite 0.37.0, what is the recommended way to interrupt long-running SQLite queries using progress handlers or timeouts?
💡 Result:
In rusqlite 0.37.0, there are two primary mechanisms for interrupting long-running SQLite queries, both of which are designed to handle concurrency and long-running operations [1][2]. 1. Using InterruptHandle (Recommended for Cross-Thread Cancellation) The recommended way to stop a query from a different thread is to use the InterruptHandle [1][2]. - You obtain this handle by calling connection.get_interrupt_handle() on the database connection [2]. - The InterruptHandle can be shared across threads [3]. - When you need to abort the operation, call the .interrupt() method on the handle [3]. - This triggers the underlying sqlite3_interrupt behavior, which forces the running SQL statement to terminate as soon as possible, typically returning an SQLITE_INTERRUPT error [3][4]. 2. Using Progress Handlers (Recommended for In-Process Checks) If you need to interrupt a query based on its internal progress or a custom condition, you can use a progress handler [2][5]. - This functionality requires the hooks crate feature to be enabled [2][6]. - You register the handler using connection.progress_handler(num_ops, handler), where num_ops defines the number of SQLite virtual machine instructions executed between handler invocations [2]. - The handler is a closure that returns a bool [2]. If the closure returns true, the query is interrupted [2]. Neither method provides a built-in "timeout" mechanism directly; to implement a timeout, you would typically spawn a separate thread that waits for the desired duration and then calls .interrupt() on the InterruptHandle [3][4]. Top results: [1][3][4][2]
Citations:
- 1: https://docs.rs/rusqlite/0.37.0/rusqlite/
- 2: https://docs.rs/rusqlite/latest/rusqlite/struct.Connection.html
- 3: https://docs.rs/rusqlite/latest/rusqlite/struct.InterruptHandle.html
- 4: https://sqlite.org/c3ref/interrupt.html
- 5: https://docs.rs/rusqlite/latest/rusqlite/functions/struct.ConnectionRef.html
- 6: https://docs.rs/rusqlite/latest/rusqlite/vtab/struct.ConnectionRef.html
Enforce query execution limits at the SQLite level
The current .take(1000) only limits rows after SQLite finishes execution. For potentially expensive queries (joins, full scans), add a SQL LIMIT clause directly to the query string and/or use conn.get_interrupt_handle() with a timeout to abort long-running operations.
// Append LIMIT if not present in original SQL
let sql = if !sql.to_uppercase().contains("LIMIT") {
format!("{sql} LIMIT 1000")
} else {
sql
};
let interrupt_handle = conn.get_interrupt_handle();
let timeout_handle = tokio::spawn({
let handle = interrupt_handle.clone();
async move {
tokio::time::sleep(Duration::from_secs(30)).await;
handle.interrupt();
}
});
// Run query; SQLite stops early
let rows = stmt.query_map(...)...;Relying solely on .take(1000) allows SQLite to process unbounded data before yielding, risking timeouts and instability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/chat_with_data/db_connector.rs` around lines 52 - 81, The
SQLite query result cap is only applied after execution via the query_map
iterator, so expensive statements can still run unbounded inside SQLite. Update
the query path in db_connector.rs to enforce a hard limit at execution time by
adding a SQL LIMIT clause before prepare/query, and consider using
conn.get_interrupt_handle() with a timeout to abort long-running queries. Keep
the existing row decoding logic and the 1000-row cap, but move the protection to
the SQL/connection level in the same query execution flow.
| let store = DATASETS.lock().map_err(|e| format!("lock poisoned: {e}"))?; | ||
| let ds = store | ||
| .get(dataset_id) | ||
| .ok_or_else(|| format!("dataset not found: {dataset_id}"))?; | ||
|
|
||
| // Generate real SQL using sqlparser-validated generation. | ||
| let generated = super::sql_gen::generate_sql_for_question(&ds.id, &ds.columns, question); | ||
|
|
||
| // Validate safety (no DROP/DELETE/etc). | ||
| if let Err(e) = super::sql_gen::is_safe_query(&generated.sql) { | ||
| return Err(format!("unsafe query rejected: {e}")); | ||
| } | ||
|
|
||
| // Execute against in-memory data if available, or SQLite if source is Sqlite. | ||
| let execution_result = if ds.source == DataSource::Sqlite { | ||
| // Try real SQLite execution via db_connector. | ||
| // Dataset ID encodes the path for Sqlite sources (convention: "sqlite:/path/to/db:table"). | ||
| let db_path = ds | ||
| .id | ||
| .strip_prefix("sqlite:") | ||
| .and_then(|s| s.split(':').next()); | ||
| if let Some(path) = db_path { | ||
| match super::db_connector::execute_sqlite_query(path, &generated.sql) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Drop the dataset lock before query execution.
store remains locked while SQL generation and SQLite execution run, so a slow DB query can block dataset registration/deletion/listing. Clone DatasetMeta inside the lock, then release it before I/O.
Proposed fix
- let store = DATASETS.lock().map_err(|e| format!("lock poisoned: {e}"))?;
- let ds = store
+ let ds = DATASETS
+ .lock()
+ .map_err(|e| format!("lock poisoned: {e}"))?
.get(dataset_id)
- .ok_or_else(|| format!("dataset not found: {dataset_id}"))?;
+ .cloned()
+ .ok_or_else(|| format!("dataset not found: {dataset_id}"))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let store = DATASETS.lock().map_err(|e| format!("lock poisoned: {e}"))?; | |
| let ds = store | |
| .get(dataset_id) | |
| .ok_or_else(|| format!("dataset not found: {dataset_id}"))?; | |
| // Generate real SQL using sqlparser-validated generation. | |
| let generated = super::sql_gen::generate_sql_for_question(&ds.id, &ds.columns, question); | |
| // Validate safety (no DROP/DELETE/etc). | |
| if let Err(e) = super::sql_gen::is_safe_query(&generated.sql) { | |
| return Err(format!("unsafe query rejected: {e}")); | |
| } | |
| // Execute against in-memory data if available, or SQLite if source is Sqlite. | |
| let execution_result = if ds.source == DataSource::Sqlite { | |
| // Try real SQLite execution via db_connector. | |
| // Dataset ID encodes the path for Sqlite sources (convention: "sqlite:/path/to/db:table"). | |
| let db_path = ds | |
| .id | |
| .strip_prefix("sqlite:") | |
| .and_then(|s| s.split(':').next()); | |
| if let Some(path) = db_path { | |
| match super::db_connector::execute_sqlite_query(path, &generated.sql) { | |
| let ds = DATASETS | |
| .lock() | |
| .map_err(|e| format!("lock poisoned: {e}"))? | |
| .get(dataset_id) | |
| .cloned() | |
| .ok_or_else(|| format!("dataset not found: {dataset_id}"))?; | |
| // Generate real SQL using sqlparser-validated generation. | |
| let generated = super::sql_gen::generate_sql_for_question(&ds.id, &ds.columns, question); | |
| // Validate safety (no DROP/DELETE/etc). | |
| if let Err(e) = super::sql_gen::is_safe_query(&generated.sql) { | |
| return Err(format!("unsafe query rejected: {e}")); | |
| } | |
| // Execute against in-memory data if available, or SQLite if source is Sqlite. | |
| let execution_result = if ds.source == DataSource::Sqlite { | |
| // Try real SQLite execution via db_connector. | |
| // Dataset ID encodes the path for Sqlite sources (convention: "sqlite:/path/to/db:table"). | |
| let db_path = ds | |
| .id | |
| .strip_prefix("sqlite:") | |
| .and_then(|s| s.split(':').next()); | |
| if let Some(path) = db_path { | |
| match super::db_connector::execute_sqlite_query(path, &generated.sql) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/chat_with_data/engine.rs` around lines 58 - 80, The dataset
registry lock is held for too long in `chat_with_data` while SQL generation and
SQLite execution run, which can block other dataset operations. In `engine.rs`,
clone the needed `DatasetMeta` (or the specific fields used by
`generate_sql_for_question` and the execution branch) from `DATASETS.lock()` and
then drop the lock before calling `sql_gen::generate_sql_for_question` and
`db_connector::execute_sqlite_query`; keep the lock only around the
`get(dataset_id)` access.
| columns | ||
| .iter() | ||
| .map(|c| format!("\"{}\"", c)) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| }; | ||
| ( | ||
| format!("SELECT {cols} FROM \"{}\" LIMIT 100", table_name), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape SQL identifiers before interpolation.
Column/table names containing " can break out of the quoted identifier and produce invalid or attacker-shaped SQL. Centralize identifier quoting and use it everywhere SQL is formatted.
Proposed fix
+fn quote_ident(ident: &str) -> String {
+ format!("\"{}\"", ident.replace('"', "\"\""))
+}
+
...
- .map(|c| format!("\"{}\"", c))
+ .map(|c| quote_ident(c))
...
- format!("SELECT {cols} FROM \"{}\" LIMIT 100", table_name),
+ format!("SELECT {cols} FROM {} LIMIT 100", quote_ident(table_name)),
...
- "SELECT {}(\"{}\") AS result FROM \"{}\"",
- agg_fn, target_col, table
+ "SELECT {}({}) AS result FROM {}",
+ agg_fn, quote_ident(&target_col), quote_ident(table)
...
- "SELECT {cols} FROM \"{table}\" WHERE \"{date_col}\" >= datetime('now', '-{days} days') ORDER BY \"{date_col}\" DESC LIMIT 100"
+ "SELECT {cols} FROM {} WHERE {} >= datetime('now', '-{days} days') ORDER BY {} DESC LIMIT 100",
+ quote_ident(table),
+ quote_ident(&date_col),
+ quote_ident(&date_col)Also applies to: 154-159, 194-195, 237-238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/chat_with_data/sql_gen.rs` around lines 82 - 89, Escape all SQL
identifiers before interpolating them into query strings: the current string
formatting in sql_gen.rs directly wraps table/column names in quotes, which can
still break on embedded quote characters. Introduce a shared identifier-quoting
helper and use it in the SQL-building paths around the SELECT construction and
the other affected formatting sites, so table_name and column names are always
safely escaped before being inserted into SQL.
| if let Ok(n) = num_str.parse::<u32>() { | ||
| let multiplier = if suffix.contains("week") { | ||
| 7 | ||
| } else if suffix.contains("month") { | ||
| 30 | ||
| } else { | ||
| 1 | ||
| }; | ||
| return Some(n * multiplier); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid overflow in time-filter multiplication.
A question like “last 4294967295 months” can overflow n * multiplier, causing debug panics or wrapped release values. Use checked_mul and reject overly large windows.
Proposed fix
- return Some(n * multiplier);
+ return n.checked_mul(multiplier);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Ok(n) = num_str.parse::<u32>() { | |
| let multiplier = if suffix.contains("week") { | |
| 7 | |
| } else if suffix.contains("month") { | |
| 30 | |
| } else { | |
| 1 | |
| }; | |
| return Some(n * multiplier); | |
| if let Ok(n) = num_str.parse::<u32>() { | |
| let multiplier = if suffix.contains("week") { | |
| 7 | |
| } else if suffix.contains("month") { | |
| 30 | |
| } else { | |
| 1 | |
| }; | |
| return n.checked_mul(multiplier); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/chat_with_data/sql_gen.rs` around lines 281 - 289, The
time-window conversion in sql_gen::parse_time_filter can overflow when
multiplying the parsed count by the suffix multiplier. Update the logic around
the `num_str.parse::<u32>()` branch to use `checked_mul` on `n` with the
computed multiplier instead of პირდაპირ `n * multiplier`, and return `None` (or
otherwise reject the filter) when the multiplication would overflow. Keep the
fix localized to the time-filter parsing path so very large inputs like “last
4294967295 months” are safely rejected.
| pub fn is_safe_query(sql: &str) -> Result<(), String> { | ||
| let upper = sql.to_uppercase(); | ||
|
|
||
| // Reject multiple statements (semicolons). | ||
| if sql.contains(';') { | ||
| return Err("Query contains multiple statements (semicolons not allowed)".into()); | ||
| } | ||
|
|
||
| let dangerous = [ | ||
| "DROP", "DELETE", "TRUNCATE", "ALTER", "INSERT", "UPDATE", "CREATE", "EXEC", "EXECUTE", | ||
| ]; | ||
| for keyword in &dangerous { | ||
| // Check it's a standalone keyword, not part of a column name. | ||
| if upper.split_whitespace().any(|w| w == *keyword) { | ||
| return Err(format!("Query contains dangerous operation: {keyword}")); | ||
| } | ||
| } | ||
|
|
||
| // Reject UNION-based injection attempts. | ||
| if upper.split_whitespace().any(|w| w == "UNION") { | ||
| return Err("Query contains UNION (not allowed for safety)".into()); | ||
| } | ||
|
|
||
| // Reject subqueries (parenthesized SELECT). | ||
| if upper.contains("(SELECT") { | ||
| return Err("Query contains subquery (not allowed for safety)".into()); | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🌐 Web query:
For sqlparser 0.62.0, which AST variants should be accepted to enforce a single plain SELECT query and reject PRAGMA, ATTACH, EXPLAIN, UNION, and subqueries?
💡 Result:
To enforce a single plain SELECT query using sqlparser 0.62.0 while rejecting other statement types (PRAGMA, ATTACH, EXPLAIN, etc.), you must inspect the variant of the Statement enum returned by the parser [1]. The sqlparser library does not provide a built-in "restrictive" mode to automatically reject specific SQL statement types at the parser level [2][3]. Instead, you must parse the input into an AST and programmatically validate that the resulting Statement matches only the Query variant containing a plain SELECT [2][1]. 1. Parse the SQL input into a Statement: Use Parser::parse_sql or Parser::parse_statement to obtain a Statement enum [1]. 2. Validate the AST: Check that the returned object is specifically the Statement::Query variant [2]. 3. Enforce "Plain SELECT" Constraints: Within the Query struct (the body of the Statement::Query), you must ensure: - The body is a SetExpr::Select [4]. - The Select object does not contain features you wish to reject (e.g., subqueries in the FROM clause, which are represented by TableFactor::Derived or similar structures within the TableWithJoins entries) [4]. - You can also verify that fields like CTEs (ctes field in Query) are empty to enforce a "plain" query [2]. To reject statements like PRAGMA, ATTACH, or EXPLAIN, you simply assert that the statement is a Statement::Query. If the parser returns any other variant (e.g., Statement::Pragma, Statement::Attach, Statement::Explain), you reject the input [1]. To reject UNION, verify that the Query body is a SetExpr::Select rather than a SetExpr::SetOperation (which handles UNION, EXCEPT, INTERSECT) [4]. In summary, the target structure to accept is: Statement::Query { ctes: [], body: SetExpr::Select(Box),... } Any other variant of Statement or non-Select body should be treated as unauthorized [4][1]. Citations: 1: https://docs.rs/sqlparser/latest/sqlparser/parser/struct.Parser.html 2: https://github.com/sqlparser-rs/sqlparser-rs 3: https://github.com/apache/datafusion-sqlparser-rs 4: https://docs.rs/sqlparser/latest/sqlparser/ast/struct.Select.html Refactor is_safe_query to use sqlparser AST validation instead of string matching. The current string-based checks are insufficient to enforce a strict "SELECT-only" contract. As confirmed by sqlparser 0.62.0 semantics, relying on keyword detection allows bypassing rules for statements like PRAGMA, ATTACH, EXPLAIN, or DETACH that do not contain the forbidden keywords but violate the contract. Replace the current logic with a parser-based validation that: Parses the input to a Statement using Parser::parse_sql. Verifies the statement is exactly Statement::Query. Ensures the query body is SetExpr::Select (rejecting SetExpr::SetOperation which covers UNION). Ensures cte_tables is empty and no derived tables (subqueries) exist in the FROM clause if strict select-only is required. This approach natively rejects Statement::Pragma, Statement::Attach, Statement::Explain, and other non-query variants without brittle string heuristics. Recommended AST shape check let mut statements = Parser::parse_sql(&dialect, sql)?; if statements.len() != 1 { return Err("Only a single statement is allowed".into()); } match statements.pop().unwrap() { Statement::Query(query) => { // Reject UNION/INTERSECT/EXCEPT if !matches!(query.body, SetExpr::Select(_)) || !query.cte_tables.is_empty() { return Err("Only plain SELECT queries are allowed".into()); } // Optional: Recursively check for subqueries in FROM clause if strictness is needed Ok(()) } _ => Err("Only SELECT queries are allowed".into()), } 🤖 Prompt for AI Agents Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/chat_with_data/sql_gen.rs` around lines 431 - 459, `is_safe_query` currently relies on brittle string matching and can miss non-SELECT statements that bypass the keyword list. Replace the logic in `sql_gen::is_safe_query` with `sqlparser` parsing via `Parser::parse_sql`, require exactly one `Statement`, accept only `Statement::Query`, and ensure the query body is `SetExpr::Select` while rejecting `SetExpr::SetOperation` and any non-empty `cte_tables`. If strict select-only behavior is needed, also walk the `FROM` clause to reject derived tables/subqueries so the validator enforces the contract structurally instead of by keywords.
| async fn resolve_and_check(url: &str) -> Result<(), String> { | ||
| let parsed = reqwest::Url::parse(url).map_err(|e| format!("invalid URL: {e}"))?; | ||
| let host = unbracket_host( | ||
| parsed | ||
| .host_str() | ||
| .ok_or_else(|| "URL has no host".to_string())?, | ||
| ) | ||
| .to_string(); | ||
| // Literal IPs are already covered by validate_webhook_url, but lookup_host | ||
| // handles them uniformly and we re-check regardless. | ||
| let port = parsed.port_or_known_default().unwrap_or(443); | ||
| let mut resolved = false; | ||
| for addr in tokio::net::lookup_host((host.as_str(), port)) | ||
| .await | ||
| .map_err(|e| format!("dns resolution failed: {e}"))? | ||
| { | ||
| resolved = true; | ||
| if is_blocked_ip(addr.ip()) { | ||
| return Err("resolved address is private/loopback/link-local".into()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🌐 Web query:
In reqwest 0.12, does Client::send perform DNS resolution independently of a prior tokio::net::lookup_host call, and what supported patterns pin or filter resolved IPs for SSRF-safe outbound webhooks?
💡 Result:
In reqwest 0.12, the Client::send method performs DNS resolution independently of any prior calls made elsewhere in your application, such as tokio::net::lookup_host [1]. When a request is executed, the client uses its internal connector (which incorporates the configured DNS resolver) to resolve the hostname at the moment the connection is established [2][1]. If you rely on a pre-flight DNS check (e.g., calling lookup_host to validate an IP before passing a URL to reqwest), you create a Time-of-Check to Time-of-Use (TOCTOU) vulnerability [1]. An attacker could provide a domain that resolves to a public, allowed IP during your pre-flight check but switches to a restricted, internal IP (like 169.254.169.254) by the time the reqwest client actually resolves it [3][1]. To implement SSRF-safe outbound webhooks, you should not rely on external pre-flight checks. Instead, implement the following patterns: 1. Custom DNS Resolver (Recommended): Implement the reqwest::dns::Resolve trait and inject it into the client using the dns_resolver method on ClientBuilder [4][5]. This allows you to inspect and filter resolved IP addresses synchronously at the exact point of connection [1][4]. By enforcing your allowlist or blocklist (e.g., denying private ranges like 127.0.0.0/8, 10.0.0.0/8, or 169.254.0.0/16) within this resolver, you close the TOCTOU window entirely [6][1]. 2. Disable Proxies: Always call.no_proxy on your ClientBuilder [1]. Reqwest's default behavior may respect system proxy environment variables (e.g., HTTP_PROXY), which can be exploited to bypass your local DNS or network restrictions [7][1]. 3. Manage Redirects: Be aware that the default RedirectPolicy (limited to 10 hops) may follow redirects to restricted IPs [3]. You may need to set a stricter policy (e.g., RedirectPolicy::none) or implement custom redirect handling that re-verifies the target URL/IP at each hop [3]. Several community patterns exist, such as the http-acl-reqwest crate, which uses a middleware and custom DNS resolver approach to enforce access control lists on outbound requests [8][9]. Building your own resolver, as seen in patterns like SsrfScreeningResolver, is considered the standard practice for high-security environments [1].
Citations:
- 1: andrico21/rmcp-server-kit@10c0bf5
- 2: https://docs.rs/reqwest/0.12.23/reqwest/struct.Client.html
- 3: Default RedirectPolicy is unsafe in server-side setting seanmonstar/reqwest#680
- 4: https://docs.rs/reqwest/latest/reqwest/dns/trait.Resolve.html
- 5: Custom DNS resolver seanmonstar/reqwest#1125
- 6: https://auditbuffet.com/patterns/ab-002397
- 7: https://docs.rs/reqwest/%3E=0.12,%20%3C0.13/
- 8: https://docs.rs/crate/http-acl-reqwest/latest
- 9: https://crates.io/crates/http-acl-reqwest
Close the DNS rebinding TOCTOU by injecting a custom DNS resolver into the reqwest client.
reqwest::Client::send performs its own DNS resolution independent of prior tokio::net::lookup_host calls. The current pre-flight check in resolve_and_check creates a Time-of-Check to Time-of-Use (TOCTOU) vulnerability: an attacker-controlled hostname can resolve to a public IP during validation and a private/internal IP during the actual request.
Remediation steps:
- Implement a custom
reqwest::dns::Resolvethat filters out private, loopback, and link-local ranges at connection time. - Inject this resolver into the
reqwest::ClientBuilderviadns_resolver. - Ensure the client disables system proxies (
no_proxy()) and restricts redirects.
This fix is required for:
src/openhuman/chat_with_data/webhooks.rs(lines 104–124 and 215–219)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/chat_with_data/webhooks.rs` around lines 104 - 124, The current
pre-flight DNS check in resolve_and_check is vulnerable to DNS rebinding because
reqwest::Client::send will resolve the host again later. Move the IP filtering
into a custom reqwest::dns::Resolve implementation that rejects private,
loopback, and link-local addresses at connection time, and wire it into the
ClientBuilder in the webhook client setup. Also ensure the reqwest client is
built with no_proxy() and strict redirect limits so the protection applies
during actual requests.
| tokio::spawn(async move { | ||
| // Re-resolve and re-validate at send time (DNS rebinding guard). | ||
| if let Err(e) = resolve_and_check(&url).await { | ||
| warn!("{LOG_PREFIX} host={host} blocked: {e}"); | ||
| return; | ||
| } | ||
| debug!("{LOG_PREFIX} firing to host={host}"); | ||
| match reqwest::Client::new() | ||
| .post(&url) | ||
| .json(&payload) | ||
| .timeout(std::time::Duration::from_secs(10)) | ||
| .send() | ||
| .await | ||
| { | ||
| Ok(resp) => debug!("{LOG_PREFIX} host={host} responded {}", resp.status()), | ||
| Err(e) => warn!("{LOG_PREFIX} host={host} failed: {e}"), | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🌐 Web query:
For tokio 1.x, what happens when tokio::spawn is called outside an active runtime, and how should libraries safely spawn optional background tasks?
💡 Result:
In Tokio 1.x, calling tokio::spawn from outside an active runtime context will result in a panic [1][2][3]. This function requires that a Tokio runtime is currently active on the thread, which is typically ensured by running code inside a task spawned by the runtime or by entering a runtime context using methods like Runtime::enter or Handle::enter [4][5][6]. For libraries that need to spawn optional background tasks safely, the recommended approach is to accept a tokio::runtime::Handle as an argument or capture it from the environment rather than relying on the global context [6][7]. Here is how to safely manage background tasks in a library: 1. Accept a Handle: Instead of calling tokio::spawn directly, allow users to pass a tokio::runtime::Handle to your library types or functions. This allows your library to spawn tasks onto the user's existing runtime explicitly [6]. 2. Use Handle::spawn: Use the handle.spawn method instead of the global tokio::spawn. This method is tied to the specific runtime instance represented by the handle and will not panic if called outside the context, provided the handle is valid [6]. 3. Capture the Handle: If your library creates its own tasks and needs to spawn others, you can capture the handle at the moment the library is initialized (e.g., using tokio::runtime::Handle::current) and store it for later use [6][7]. By using Handle::spawn, your library avoids the dependency on a thread-local runtime context, making it more robust and easier to integrate into various application architectures, including those where tasks might be spawned from non-Tokio threads [6][7].
Citations:
- 1: https://docs.rs/tokio/1.46.1/tokio/task/fn.spawn.html
- 2: https://docs.rs/tokio/1.0.1/tokio/fn.spawn.html
- 3: https://docs.rs/tokio/1.32.0/tokio/task/fn.spawn.html
- 4: https://docs.rs/tokio/1.22.0/tokio/runtime/struct.Runtime.html
- 5: https://docs.rs/tokio/latest/tokio/task/fn.spawn.html
- 6: https://docs.rs/tokio/latest/tokio/runtime/struct.Handle.html
- 7: https://stackoverflow.com/questions/61292425/how-to-run-an-asynchronous-task-from-a-non-main-thread-in-tokio
Replace tokio::spawn with Handle::spawn to prevent panics outside a runtime
The current implementation of tokio::spawn inside notify_insight will panic if called from a synchronous context that lacks an active Tokio runtime (e.g., synchronous schedulers, initialization code, or certain test setups). This causes a hard crash rather than a graceful degradation.
To fix this safely:
- Capture
tokio::runtime::Handle::current()at the entry of the async boundary or store aHandlein the shared state. - Use
handle.spawn(...)instead oftokio::spawn(...).Handle::spawnuses the stored runtime instance and avoids the thread-local runtime dependency that triggers the panic.
If notify_insight must remain synchronous and the caller might not be in a runtime, capture the Handle once during initialization or return an error if no handle is available. Do not rely on tokio::spawn in library code that may be invoked from non-async contexts.
Original code at src/openhuman/chat_with_data/webhooks.rs:208-225
tokio::spawn(async move {
// Re-resolve and re-validate at send time (DNS rebinding guard).
if let Err(e) = resolve_and_check(&url).await {
warn!("{LOG_PREFIX} host={host} blocked: {e}");
return;
}
debug!("{LOG_PREFIX} firing to host={host}");
match reqwest::Client::new()
.post(&url)
.json(&payload)
.timeout(std::time::Duration::from_secs(10))
.send()
.await
{
Ok(resp) => debug!("{LOG_PREFIX} host={host} responded {}", resp.status()),
Err(e) => warn!("{LOG_PREFIX} host={host} failed: {e}"),
}
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/chat_with_data/webhooks.rs` around lines 208 - 225,
`notify_insight` currently uses `tokio::spawn`, which can panic when called
without an active runtime. Update the spawn path to use a captured
`tokio::runtime::Handle` and call `handle.spawn(...)` instead, or store the
handle in the shared state used by `webhooks.rs`. Make sure the async block that
does `resolve_and_check`, the POST request, and the logging remains unchanged,
but the task is scheduled through the runtime handle rather than the
thread-local `tokio::spawn`.
|
Closing — these need to be restacked properly with CI passing. Will resubmit once #4142 (shared wiring) lands and each domain PR compiles cleanly against it. |
Self-contained domain module — split from #2261 (PR 7/7). Depends on #4142.
9 files: types, engine, sql_gen, db_connector, anomaly, webhooks, rpc, schemas, mod.
E2E: register → query → list → insight → scan_anomalies.
This completes the split of #2261 into 7 focused PRs.
Summary by CodeRabbit
New Features
Bug Fixes