From d1a98ff41bb6fd22ef8ca3665aa646f221baf56d Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 28 May 2026 03:15:25 +0530 Subject: [PATCH 1/2] fix(observability): classify RPC filesystem path validation errors as expected (Sentry TAURI-RUST-4QH) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `ExpectedErrorKind::FilesystemUserPathInvalid` variant and `is_filesystem_user_path_invalid_message` predicate anchored on the literal phrase `"path is not a directory:"`. Routes the dispatcher's `report_error_or_expected` call (via `src/core/jsonrpc.rs:135-141`) through the demote path so RPC-level user-input validation failures emit a `tracing::warn!` breadcrumb instead of a Sentry error event. Drops Sentry TAURI-RUST-4QH — `vault_create` called with a `root_path` that doesn't resolve to an existing directory. Emission site: `src/openhuman/vault/ops.rs:37`. Observed ~2 events/day on `openhuman@0.56.0`, `domain=rpc method=openhuman.vault_create operation=invoke_method`. Also preempts the symmetric `"hosted path is not a directory:"` shape from `openhuman::http_host::path_utils:23` once it surfaces. Anchor design: trailing colon discriminates user input (path follows the colon, as both wire shapes emit) from invariant violations. The `skills::ops_install:475` SAFETY GUARD (`"{path} is not a directory — refusing to remove"`, em-dash, no colon) stays actionable — it catches an `rm -rf` invariant violation, which IS a code bug. Pinned by the rejection test. Bonus: the user-supplied path embeds OS username via the home-directory prefix (`/Users//...`), so demoting these out of Sentry is a small privacy win on top of the noise reduction. 3 new tests: vault wire shape (verbatim TAURI-RUST-4QH body + the dispatcher's `rpc.invoke_method failed:` re-wrap), http_host wire shape, and a 4-case rejection contract covering the safety guard, a narrative log line, the inverse `Is a directory (os error 21)` EISDIR rendering, and an adjacent vault validation error (`"root_path must be absolute:"`) that's actionable. `cargo test --lib core::observability::tests` → 91 passed. Sentry-Issue: TAURI-RUST-4QH --- src/core/observability.rs | 163 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/src/core/observability.rs b/src/core/observability.rs index 515f2a18b..374416675 100644 --- a/src/core/observability.rs +++ b/src/core/observability.rs @@ -160,6 +160,27 @@ pub enum ExpectedErrorKind { /// state snapshots) — every one of them emits the same canonical errno /// rendering. DiskFull, + /// A user-supplied filesystem path failed an RPC-level validation + /// check — e.g. `openhuman.vault_create` was called with a + /// `root_path` that doesn't exist or points at a file rather than a + /// directory. The UI already shows the typed error to the user, and + /// Sentry has no remediation path (we can't `mkdir -p` a folder the + /// user hasn't actually picked yet). User-supplied paths can also + /// embed PII fragments (the home-directory segment leaks the OS + /// username), so demoting these out of the Sentry event stream is a + /// small privacy win on top of the noise reduction. + /// + /// Drops Sentry TAURI-RUST-4QH (`root_path is not a directory: + /// /Users//Documents/`, observed on + /// `openhuman@0.56.0`) and preempts the symmetric + /// `hosted path is not a directory:` shape from + /// `openhuman::http_host::path_utils` once it starts surfacing. + /// See [`is_filesystem_user_path_invalid_message`] for the polarity + /// contract — the safety-guard variant in `skills::ops_install` + /// (`{path} is not a directory — refusing to remove`) is + /// deliberately not matched because that's an `rm -rf` invariant + /// violation, not user input. + FilesystemUserPathInvalid, } pub fn expected_error_kind(message: &str) -> Option { @@ -241,6 +262,14 @@ pub fn expected_error_kind(message: &str) -> Option { if is_disk_full_message(&lower) { return Some(ExpectedErrorKind::DiskFull); } + // RPC-level filesystem path validation must run last so an unrelated + // earlier matcher (e.g. a future provider error whose body happens to + // contain "path is not a directory:") doesn't steal the message from + // its rightful bucket. See the variant doc-comment for scope and the + // [`is_filesystem_user_path_invalid_message`] polarity contract. + if is_filesystem_user_path_invalid_message(&lower) { + return Some(ExpectedErrorKind::FilesystemUserPathInvalid); + } None } @@ -795,6 +824,42 @@ fn is_prompt_injection_blocked_message(lower: &str) -> bool { || lower.contains("prompt blocked by security policy") } +/// Detect an RPC-level filesystem path validation failure from user input. +/// +/// Anchored on the literal phrase `"path is not a directory:"` (with the +/// trailing colon followed by the user-supplied path). This matches the +/// two known wire shapes — both emitted at the RPC entry boundary when a +/// user typed/picked a path that doesn't resolve to an existing directory: +/// +/// - `"root_path is not a directory: "` — +/// [`crate::openhuman::vault::ops::vault_create`] when the chosen vault +/// folder doesn't exist or points at a file (Sentry TAURI-RUST-4QH). +/// - `"hosted path is not a directory: "` — +/// [`crate::openhuman::http_host::path_utils`] when an HTTP host config +/// references a missing directory. Not yet observed in Sentry but +/// shares the same user-input failure mode; preempts a future ID. +/// +/// Both are deterministic Err returns at the validation gate of an RPC +/// handler, BEFORE any side-effect happens. The UI already surfaces the +/// typed error and Sentry has no remediation path. +/// +/// **Polarity contract** — the trailing colon discriminates user input +/// from invariant violations: +/// +/// - `skills::ops_install` emits `"{path} is not a directory — refusing +/// to remove"` (em-dash separator, no colon after `"directory"`). That +/// is an `rm -rf` safety guard catching an UNEXPECTED state, not user +/// input — it must STAY actionable. +/// - A narrative log line like `"this path is not a directory check +/// passed"` lacks the colon and won't classify. +/// +/// All matches are substring-based against the lower-cased message so +/// the classifier survives caller wrapping (`rpc.invoke_method`, +/// anyhow context chains, …). +fn is_filesystem_user_path_invalid_message(lower: &str) -> bool { + lower.contains("path is not a directory:") +} + /// Capture an error to Sentry with structured tags. /// /// `domain` and `operation` are required and become tags `domain:<…>` and @@ -1042,6 +1107,25 @@ fn report_expected_message(kind: ExpectedErrorKind, message: &str, domain: &str, "[observability] {domain}.{operation} skipped expected memory-store circuit-breaker-open error" ); } + ExpectedErrorKind::FilesystemUserPathInvalid => { + // User-input validation failure surfaced at the RPC + // boundary — e.g. `openhuman.vault_create` called with a + // `root_path` that doesn't exist. The typed error is + // already shown to the user; Sentry has no remediation + // path. Demote to `warn!` (same tier as `BackendUserError`) + // so the local trace still pins which RPC + which method + // tripped the gate, but no Sentry event fires. The + // `error = %message` field intentionally retains the + // user-supplied path so the local log can correlate with a + // user bug report — Sentry doesn't see it. + tracing::warn!( + domain = domain, + operation = operation, + kind = "filesystem_user_path_invalid", + error = %message, + "[observability] {domain}.{operation} skipped expected filesystem path validation error: {message}" + ); + } } } @@ -1707,6 +1791,85 @@ mod tests { ); } + // ── FilesystemUserPathInvalid (TAURI-RUST-4QH) ───────────────────────── + + #[test] + fn classifies_vault_create_root_path_not_a_directory_as_filesystem_user_path_invalid() { + // TAURI-RUST-4QH: verbatim wire shape from + // `openhuman::vault::ops::vault_create` line 37 when the + // user-picked vault folder doesn't resolve to an existing + // directory. Bubbles up as the RPC dispatcher's + // `display_message` and reaches `report_error_or_expected` — + // must classify so no Sentry event fires. + assert_eq!( + expected_error_kind( + "root_path is not a directory: /Users/zadam/Documents/SndBrainOpenHuman" + ), + Some(ExpectedErrorKind::FilesystemUserPathInvalid) + ); + + // The same body wrapped by the JSON-RPC dispatcher's `display_message` + // prefix (`rpc.invoke_method` re-emit shape from `src/core/jsonrpc.rs`). + // Must still classify so the dispatch-site re-report doesn't escape + // the matcher even if a future caller layers more context. + assert_eq!( + expected_error_kind( + "rpc.invoke_method failed: root_path is not a directory: /Users/alice/openhuman-data" + ), + Some(ExpectedErrorKind::FilesystemUserPathInvalid) + ); + } + + #[test] + fn classifies_http_host_hosted_path_not_a_directory_as_filesystem_user_path_invalid() { + // Preempt the symmetric shape from + // `openhuman::http_host::path_utils:23` — + // `"hosted path is not a directory: "`. Not yet observed + // in Sentry but shares the same RPC validation polarity as + // vault_create's `root_path` check. Anchoring on + // `"path is not a directory:"` (with trailing colon) covers + // both without two separate matchers. + assert_eq!( + expected_error_kind("hosted path is not a directory: /var/www/static-site"), + Some(ExpectedErrorKind::FilesystemUserPathInvalid) + ); + } + + #[test] + fn does_not_classify_unrelated_path_messages_as_filesystem_user_path_invalid() { + // Polarity contract — the anchor requires a trailing colon + // after `"is not a directory"`, which discriminates user input + // (path follows the colon) from other shapes: + // + // 1. The `skills::ops_install:475` SAFETY GUARD — + // `" is not a directory — refusing to remove"` — must + // stay actionable. It catches an `rm -rf` invariant violation + // (the target should have been a directory but wasn't), + // which is a code bug, not user input. + // 2. A narrative log line that happens to mention the phrase + // without the user-path colon suffix is not a validation + // failure and must not be silenced. + // 3. The dot-prefix variant from POSIX `EISDIR`/`ENOTDIR` + // renderings (`"Is a directory (os error 21)"`) is the + // inverse condition — different code path entirely. + for raw in [ + // Safety guard — must NOT classify. + "/tmp/openhuman-cache is not a directory — refusing to remove", + // Narrative log line — must NOT classify. + "checked that path is not a directory before mkdir", + // Inverse condition (os error 21: EISDIR) — must NOT classify. + "open /etc/passwd failed: Is a directory (os error 21)", + // Bare path with no `directory` mention — must NOT classify. + "root_path must be absolute: ./relative/path", + ] { + assert_eq!( + expected_error_kind(raw), + None, + "polarity contract: must NOT classify as FilesystemUserPathInvalid: {raw}" + ); + } + } + #[test] fn classifies_memory_store_breaker_open() { // TAURI-RUST-52X (~455 events on self-hosted Sentry): the chunk-store From cb193131f5e818ffbe56df0495efd4fd6c992dd7 Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 28 May 2026 03:37:39 +0530 Subject: [PATCH 2/2] fix(observability): redact user filesystem path from FilesystemUserPathInvalid breadcrumb (review of #2795) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original commit on this branch included `error = %message` plus `{message}` interpolation in the demoted `tracing::warn!` body. The intent was "warn-level local log for bug-report correlation, Sentry doesn't see it." That intent is wrong: - `src/core/logging.rs:366` maps `Level::WARN` to `EventFilter::Breadcrumb` in the `sentry_tracing_layer`. - Every `tracing::warn!` therefore becomes a Sentry breadcrumb that attaches to any subsequent event captured by the same hub. - The user-supplied path embeds the local filesystem layout (`/Users//Documents//…`) — PII that should never reach Sentry, even when the immediate classifier decision is "skip the event." Mirror the existing `LoopbackUnavailable` arm: drop the `error` structured field and the `{message}` interpolation, keep only `domain` / `operation` / `kind`. Comment block updated to explain the redaction reasoning and point at the `core::logging` layer mapping that makes it load-bearing. Full-path diagnostics for local debugging remain available via `RUST_LOG=openhuman_core::core::observability=debug` since `Level::DEBUG` / `TRACE` are mapped to `EventFilter::Ignore`. Classifier behaviour is unchanged — the three `is_filesystem_user_path_invalid_message` tests already on this branch continue to pass (3/3), and the broader observability suite stays green (91 tests). ## Test plan - [x] `cargo test filesystem_user_path_invalid` — 3 tests pass - [x] `cargo test core::observability` — 91 tests pass, 0 regressions - [x] `cargo check --bin openhuman-core` — passes - [x] `cargo fmt --check` — clean --- src/core/observability.rs | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/core/observability.rs b/src/core/observability.rs index 374416675..044f24457 100644 --- a/src/core/observability.rs +++ b/src/core/observability.rs @@ -1112,18 +1112,28 @@ fn report_expected_message(kind: ExpectedErrorKind, message: &str, domain: &str, // boundary — e.g. `openhuman.vault_create` called with a // `root_path` that doesn't exist. The typed error is // already shown to the user; Sentry has no remediation - // path. Demote to `warn!` (same tier as `BackendUserError`) - // so the local trace still pins which RPC + which method - // tripped the gate, but no Sentry event fires. The - // `error = %message` field intentionally retains the - // user-supplied path so the local log can correlate with a - // user bug report — Sentry doesn't see it. + // path. Demote to `warn!` so the local trace still pins + // which RPC + which method tripped the gate. + // + // **Do not include the raw `message` here.** The message + // body embeds the user's local filesystem layout (username, + // project name, document directory, …) and + // `sentry_tracing_layer` in `core::logging` maps + // `Level::WARN` to `EventFilter::Breadcrumb` — so any + // formatted body would be attached as a breadcrumb to + // every subsequent Sentry event from this hub, leaking + // user paths into unrelated reports. Log only `domain` / + // `operation` / `kind` (no PII), matching the + // `LoopbackUnavailable` arm above ("metadata over raw text + // for noise demotions", per the #1719 review feedback). + // Full-path diagnostics for local debugging stay available + // via `RUST_LOG=…=debug` since `Level::DEBUG` / `TRACE` + // are mapped to `EventFilter::Ignore`. tracing::warn!( domain = domain, operation = operation, kind = "filesystem_user_path_invalid", - error = %message, - "[observability] {domain}.{operation} skipped expected filesystem path validation error: {message}" + "[observability] {domain}.{operation} skipped expected filesystem path validation error" ); } }