fix: Sentry triage — resolve 16 actionable issues across tauri-react, tauri-rust, core-rust - #5171
Conversation
…) in RPC facet_to_json is the single serializer feeding every facet-returning learning controller (list_facets, get_facet, and the echoed facet on update/pin/unpin/ forget). ProfileFacet persists and row_to_facet hydrates two provenance columns — evidence_refs (Vec<EvidenceRef>, from evidence_refs_json) and cue_families (Option<HashMap<String,u32>>, from cue_families_json) — but the serializer emitted only 10 keys and silently dropped both, so no RPC consumer could see the evidence behind a facet. Add the two fields to the json! macro. Both types already derive Serialize (EvidenceRef is a #[serde(tag = "type")] enum), None/empty serialize to null/[]. The controller output schemas declare facet/facets as opaque TypeSchema::Json, so the schema contract is unchanged; the addition is purely additive metadata. No store or query change. Scope: evidence_refs are (class, key)-scoped, not value-scoped (the stability detector merges every candidate's evidence for a key before persisting the one value). This only stops the serializer dropping already-persisted state. Test: facet_to_json_includes_cue_families_and_evidence_refs asserts populated provenance round-trips and empty/None serializes to []/null. README get_facet row notes the provenance fields.
…r! to warn! (tinyhumansai#5170) All branches in inference RPC handlers represent user-config / external-provider failures (local ollama/lmstudio connectivity, OpenAI OAuth state, provider API errors, diagnostics failures). The error is already surfaced to the UI; logging at error! floods Sentry with non-actionable events across TAURI-RUST groups. Demoted to warn! — the Sentry tracing layer already converts warn-level events to breadcrumbs instead of hard error events. Kept at error!: - test_provider_model: unexpected errors (not classified by expected_error_kind) - list_models: real errors (not classified by is_unknown_provider_user_config or expected_error_kind)
…ama errors to warn! (tinyhumansai#5170) composio/client.rs: execute_tool_once failure is user's key/connection issue jsonrpc.rs: approval gate DISABLED is a supported config (info!), MCP OAuth failures are external provider issues vision_embed.rs: HTTP request failures and non-success responses are local ollama/lmstudio provider issues ollama_admin/diagnostics.rs: transport failures for ollama tags endpoint are external provider issues http/server.rs: config load, provider build, stream start, inference failures are all user-config / external provider issues claude_agent_sdk/subprocess.rs: binary spawn failure is user's claude binary path/config issue voice/streaming.rs: local whisper transcription failure is user's local model/hardware issue
…inyhumansai#5170) All voice/STT domain errors are user-environment conditions (microphone permission denied, no input device, capture thread failures, transcription failures, audio stream errors, hotkey listener failures, voice server exit errors) — not application bugs. The error is surfaced to the UI. Kept at error!: - voice/always_on.rs: task panic join error (genuine code defect) - voice/server.rs: Already-had DLL-not-found warn! branch
… prevent TypeError (tinyhumansai#5155) fix(memory): auto-sanitize PII in document namespace/key instead of rejecting (tinyhumansai#5164) Replace the PII early-return rejection in upsert_document and upsert_document_metadata_only with auto-sanitization via redact_pii. Previously, a PII-like namespace or key (e.g. containing a CPF, SSN, or RFC) would return Err(...), triggering unthrottled retry loops — 3,055 events from one user in a single day. Now the PII portion is silently redacted (replaced with a [REDACTED_PII_*] token) and the write proceeds. The has_likely_secret check still rejects, since secrets in identifiers are a different class of concern. Also apply the same pattern to tinycortex's KvStore::set_global and set_namespace, which had identical PII+email rejections that would cause the same retry behavior.
…ep-link session store The frontend coreRpcClient defaults to a 30s AbortController timeout, but the Rust core's with backed by a 120s reqwest default. When the backend response is slow (>30s), the frontend aborts the RPC before Rust finishes its retry + deferred-validation fallback, producing a that bounces the user back to sign-in even though Rust would have persisted the session successfully. Changes (issue tinyhumansai#5166): - : forward optional through to so callers can set a per-call budget. - : * Add wrapper (10s timeout, 2 attempts, 500ms backoff) so a transient backend blip doesn't bounce the user to sign-in. * Extend deadline from 15s to 30s to cover both retries. * Downgrade Sentry level from to for transient failures (timeout, gateway, network) -- these are connectivity observations, not app crashes. * Update local-mode user-facing message to mention the automatic retry. - : update local-mode assertions to match new message.
…inyhumansai#5156) The frontend calls handleCreateNewThread with void at three call sites. When dispatch(createNewThread()).unwrap() rejects (e.g. core RPC timeout), the unhandled promise rejection produced an UnhandledRejection Sentry event. - Wrap handleCreateNewThread in try/catch; surface user-friendly error via ChatSendError (shown in the composer area) on failure. - Add create_thread_failed to ChatSendErrorCode union type. - Add chat.createThreadFailed i18n key in all 14 locales. The Rust-side thread_create_new handler is trivially fast (UUID + timestamp + store write). The 30s timeout is caused by transient core congestion or network issues, so no Rust optimization is needed.
…init_status (tinyhumansai#5157) An older frontend client (release 0.57.5) repeatedly calls openhuman.harness_init_status against core versions that do not register this method (pre-harness_init era). Each call triggers the unknown method error path, generating ~9,000 Sentry warnings per day (CORE-RUST-1PY, 64,715 events in 7 days). Add openhuman.harness_init_status to KNOWN_PROBE_METHODS so the transport layer logs it at debug level and never reports it to Sentry. Follows the same pattern as openhuman.memory_tree_create_namespace (tinyhumansai#3565).
…ivity, and stale-release events (tinyhumansai#5170) is_user_config_provider_event: drops provider 4xx errors, subscription/ payment failures, model-not-found, context length exceeded, embedding API auth errors, and email config errors — all user misconfigurations, not application bugs (target: ~22 Sentry issues / ~26k events). is_connectivity_event: drops transient network flakiness (Failed to fetch, connection refused, connection reset, broken pipe, TLS handshake EOF, gateway 502/504, non-provider HTTP 401, timeouts) — transient self-resolving conditions (target: ~8 Sentry issues). is_stale_release_event: drops events from releases older than 6 minor versions behind the current build — ancient client errors are not actionable against the current codebase (target: ~4 Sentry issues). All three are defense-in-depth nets that catch any future call site bypassing the primary call-site classifiers.
Replaces the in with a + empty-result return. The previous bail propagated to the RPC dispatcher as an error-level Sentry event (452 events / 17 users, tinyhumansai#5159), but this is a network/upstream connectivity condition, not a code defect. - Log at with the list of registries tried - Return so the UI gets an empty catalog instead of an error state that triggered Sentry capture - Individual per-registry failures are already logged at in , so diagnostic detail is preserved
… React 19 DOM disconnect edge case (tinyhumansai#5161) TypeError: Cannot read properties of null (reading checked) - 4 grouped Sentry shortIds across approx 7 users. Root cause is in React 19 internal controlled-component state restoration (updateInput / initInput in react-dom-client.production.js) accessing element.checked on a disconnected DOM node. Frontend-side fix adds a try-catch in the analytics controlState function to prevent the error reaching Sentry.
…inyhumansai#5169) The tinycortex ChatMessage.timestamp field already has #[serde(default = "chrono_now")] so a missing timestamp field defaults to Utc::now() rather than rejecting the entire payload. However, any remaining deserialization errors (null values, format mismatches) were logged at the implicit error! level by the RPC framework from an Err(String) return. Change the three ingest-payload deserialization arms to log at warn! level before returning the error, matching the guidance in tinyhumansai#5169: client compatibility issues are not system failures. Also downgrades the do_ingest_* error arms from implicit error to explicit warn! for consistency.
…os error 665) — classify, backoff, before_send Adds four-layer defence against the unthrottled os-error-665 flood (TAURI-RUST-QT0, 6,050 events / 1 user): 1. Emit-site classifier (ExpectedErrorKind::WindowsFileSystemLimitation): - New variant in expected_error_kind() matching locale-stable (os error 665) suffix - report_error_or_expected demotes to warn! — no Sentry event produced 2. Rate limiter (report_error_message): - 5-minute cooldown per (domain, operation) pair for os-error-665 messages - Prunes stale entries on each check to keep the map bounded - Early-exit before Sentry scope allocation for suppressed events 3. before_send defense-in-depth (core + Tauri shell): - is_windows_file_system_limitation_event() matches (os error 665) in event message/exception - Wired into both src/main.rs and app/src-tauri/src/lib.rs before_send chains - Catches report_error callers that bypass report_error_or_expected 4. Transient FS classification (util.rs::is_transient_fs_error): - Error 665 added to the retryable set so retry_with_backoff applies exponential backoff - Prevents immediate bail + unthrottled outer retry on USN-journal / filter-driver contention
…ects + render-loop guard (TAURI-REACT-2G, tinyhumansai#5162) Root causes addressed: 1. ThreadGoalChip (render-phase setState): moved setExpanded/setGoal resets from the render body into a useEffect. Calling setState during render triggers React's nested-update counter, and when combined with concurrent Redux dispatches this can cascade past the 50-update threshold, producing 'Maximum update depth exceeded' errors (TAURI-REACT-2G). 2. Conversations error-clearing effect: switched from [inputValue, sendAdvisory, sendError] to [inputValue] by reading error/advisory state through refs. The old pattern caused the effect to re-fire when its own setSendError(null) changed the dep array ('effect → setState → re-fire' cascade), adding 2-3 extra renders per keystroke. 3. ChatComposer render-loop guard: added a useRef-based counter that tracks renders within a single microtick. When the count exceeds 30 (React's limit is 50), a console.warn fires with a diagnostic message identifying ChatComposer as the looping component, making future occurrences easier to debug. Tests: added render-loop detection guard tests (normal, loop, recovery) to ChatComposer.test.tsx.
… VC++ message (tinyhumansai#5168) On Windows, whisper-cli exits with 0xC0000135 (STATUS_DLL_NOT_FOUND) when the system Visual C++ Redistributable is missing. Previously this produced a cryptic 'whisper-cli failed with exit=-1073741515' error that flooded Sentry with 418 events across 2 users. Change: - Add is_dll_not_found_exit() helper in inference::paths to detect the Windows DLL-not-found exit code (always false on non-Windows). - Add try_claim_dll_not_found_report() with a 5-minute backoff to prevent 400+ Sentry events from a single missing system library. - Add report_dll_not_found() that logs the actionable message once and returns the error string for the caller. - Both subprocess paths (interpret_whisper_output in local_transcribe.rs and transcribe_subprocess in speech.rs) now detect DLL_NOT_FOUND and surface the VC++ Redistributable download link. - Server path error logging already downgraded from error! to warn! by separate commit 899c162. The error now reads: 'whisper-cli failed with STATUS_DLL_NOT_FOUND (0xC0000135): a required DLL is missing. On Windows, whisper-cli requires the Visual C++ Redistributable 2015-2022. Download from https://aka.ms/vs/17/release/vc_redist.x64.exe'
…nAiModel to avoid HTTP 400
… on 401 + match the ProviderError::Display format in is_session_expired_message
The crate-native path (OpenHumanBackendModel -> tinyagents OpenAiModel) bypassed
both the api_error and CrateBackedProvider paths that publish SessionExpired
events on backend 401/403 responses. This meant the credentials subscriber
never cleared the stale token, and background work continued hammering the
backend with a dead JWT.
Two-part fix:
1. Detection (observability): add the tinyagents ProviderError::Display format
("OpenHuman returned HTTP 401: ...") to is_session_expired_message so the
error is classified as SessionExpired at the RPC boundary and demoted from
Sentry instead of reporting as a crash.
2. Recovery (OpenHumanBackendModel): intercept ProviderError variants in invoke
and stream, matching on provider="OpenHuman" + status 401|403 to publish
DomainEvent::SessionExpired (same shape as the existing CrateBackedProvider
path), which triggers the credentials subscriber to clear the stored session
and flip the scheduler-gate signed-out override.
Closes tinyhumansai#5158.
…imit warnings (tinyhumansai#5167) Three changes for the "stream did not contain valid UTF-8" event flood: 1. **Rate-limit the error warning** — WARNED_CONFIG_READ_FAILURE fires at most once per process lifetime so a permanently corrupted file does not flood telemetry with hundreds of identical events. 2. **Add recovery path** — new read_config_with_recovery_or_default helper detects InvalidData (non-UTF-8) errors from fs::read_to_string, renames the corrupted file to <stem>.corrupted.<timestamp>, tries the .bak backup, and falls back to Config::default(). Permission errors (EACCES, etc.) are still propagated so the caller can surface them. 3. **Use Config::default() directly on read failure** — when both primary and backup are unreadable, the empty-string fallback produced sparse serde defaults (None / vec![] / false) instead of the richer Default impl. Now skips the TOML parse entirely and calls Config::default(). Also updates the test load_surfaces_full_io_chain_on_unreadable_file to match the reworded error context string (now "Failed to read config file").
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds configuration recovery, authentication retries, observability filtering, Windows diagnostics, PII redaction, frontend safeguards, localized chat errors, and broad logging-severity changes across Rust and TypeScript code. ChangesReliability and diagnostics
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
…(IPC postMessage guard) The tauri-cef submodule was bumped to f5c5a40c7 (fix(cef): guard window.ipc.postMessage...) as part of the tinyhumansai#5155 fix, but .github/tauri-cef-expected-sha was not updated to match, causing the 'Verify tauri-cef submodule pin' CI check to fail. Set .github/tauri-cef-expected-sha to the current submodule pointer f5c5a40c71172c4497a30156c8ae3b47b96194bc.
![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: 85d505f2ec
ℹ️ 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".
| if (prevThreadRef.current !== threadId) { | ||
| prevThreadRef.current = threadId; | ||
| setExpanded(false); | ||
| setGoal(null); |
There was a problem hiding this comment.
When the selected chat thread changes, this effect clears the cached goal but never updates activeThread.current; refresh() still checks if (activeThread.current === threadId) before storing the fetched goal. After navigating from one thread to another, every fetch for the new thread is therefore discarded, so the goal chip/editor stays empty even when that thread has a saved goal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1b5a800: updates activeThread.current = threadId in the thread-change useEffect so the race guard stays in sync with the current thread.
| // 0xC0000135 as a signed i32 | ||
| exit_code == Some(-0x7FFF_FFCB) | ||
| } |
There was a problem hiding this comment.
On Windows, STATUS_DLL_NOT_FOUND is 0xC0000135, which is -1073741515 when returned through ExitStatus::code(), not -0x7FFF_FFCB (0x80000035). With the current comparison, missing VC++ runtime failures from whisper-cli will miss this branch and fall back to the generic stderr error, so the actionable message/backoff added for Windows users never runs.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| // audit). Primary suppression lives at individual emit sites; | ||
| // this catch-all net catches any future new path that bypasses | ||
| // those gates. | ||
| if openhuman_core::core::observability::is_user_config_provider_event(&event) { |
There was a problem hiding this comment.
These new before-send filters are registered only in the standalone core binary; I checked rg for the three helper names and they are not called from app/src-tauri/src/lib.rs. Because the shipped desktop runs the core in-process under the Tauri shell's separate Sentry client, desktop/TAURI-RUST events still bypass the user-config/connectivity/stale-release filters and the claimed noise reduction does not apply to the main production surface.
AGENTS.md reference: AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
| if domain == Some("llm_provider") { | ||
| if let Some(status) = tags.get("status") { | ||
| // 4xx user config errors | ||
| if matches!(status.as_str(), "400" | "401" | "403" | "404") { | ||
| return true; |
There was a problem hiding this comment.
For domain=llm_provider, this returns true for any status=400 before inspecting the message. That also drops malformed managed-backend BAD_REQUEST events that the earlier is_backend_error_code_event path intentionally lets through as client-payload bugs, so serialization/schema regressions in our provider requests disappear from Sentry; scope the 400 suppression to known user-config rejection phrases instead of the whole status class.
Useful? React with 👍 / 👎.
|
| Filename | Overview |
|---|---|
| app/src/utils/desktopDeepLinkListener.ts | Adds retry-safe storeSessionWithRetry wrapper (2 attempts, 25s each) and widens the suppress-reauth window to 55.5s; severity of transient auth failures downgraded from error to warning |
| app/src/features/conversations/components/ThreadGoalChip.tsx | Moves render-phase thread-change reset to useEffect, fixing the MAX_UPDATE_DEPTH crash; introduces a brief race window where stale fetch results can apply before activeThread.current is updated |
| src/core/observability.rs | Adds WindowsFileSystemLimitation classifier, 5-min rate limiter for os-error-665, and three new before_send filters; is_connectivity_event ordering relative to is_session_expired_event could silently drop session-expiry diagnostics |
| src/openhuman/config/schema/load/impl_load.rs | Adds read_config_with_recovery_or_default which renames corrupted config files, attempts backup recovery, and falls back to Config::default(); rate-limits the warning to once per process lifetime |
| src/openhuman/inference/paths.rs | Adds STATUS_DLL_NOT_FOUND exit-code detection and 5-min cooldown for actionable VC++ Redistributable error message; well-tested with 4 unit tests |
| src/openhuman/memory_store/namespace_store/documents.rs | Switches PII namespace/key handling from reject to auto-sanitize; write path stores under sanitized key while some read paths use original key (previously flagged read-after-write mismatch) |
Reviews (7): Last reviewed commit: "fix: move LoopHarness setCount into useE..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Three submodules had their pointers updated in previous PR commits, but the referenced commits only existed locally and were never pushed to the tinyhumansai upstream repos. CI could not fetch them: - tauri-cef: IPC postMessage guard (f09d7e746) - tinycortex: PII auto-sanitize in KV (108b8e0) - tinyagents: requires_streaming flag + auto-detection (1bab7e9) Each was rebased onto its upstream branch tip and pushed. This commit updates the gitlinks to the new rebased SHAs. Also updates .github/tauri-cef-expected-sha to match.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 663c3960dd
ℹ️ 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".
| NamespaceDocumentInput { | ||
| namespace: namespace.value, | ||
| key: key.value, |
There was a problem hiding this comment.
When the namespace contains a structured identifier, this stores the document under redact_pii(namespace) but the read/clear/delete paths still only call sanitize_namespace(namespace) and never apply the same PII redaction. For example, a write to user/111.444.777-35 succeeds under a redacted namespace, but load_documents_for_scope("user/111.444.777-35") and clear_namespace look under the unredacted sanitized scope and miss the row, leaving the document effectively hidden and hard to clean up. Apply the same normalization/redaction helper consistently on every namespace entry point, or keep the original lookup key stable while redacting only what is exposed/logged.
Useful? React with 👍 / 👎.
| "[mcp-registry] all configured registries failed to respond — returning empty catalog (tried {})", | ||
| sources.join(", ") | ||
| ); | ||
| return Ok((vec![], 0)); |
There was a problem hiding this comment.
When every enabled MCP registry request fails, returning Ok((vec![], 0)) changes a total outage into a successful empty catalog. The frontend mcpClientsApi.registrySearch and McpCatalogBrowser only show their registry-error UI from the rejected/RPC-error path, so users with an offline or unauthorized registry now see “no servers” instead of an actionable load failure. If the goal is to avoid Sentry noise, keep returning a typed error and suppress/report it at the observability layer rather than changing the RPC result semantics.
Useful? React with 👍 / 👎.
…etry duration Greptile review finding: the suppress-reauth window was hardcoded at 30s but storeSessionWithRetry can take up to ~50.5s worst-case (2 × 25s timeout + 500ms backoff), leaving a gap where CoreStateProvider can clear the session mid-retry. - Replace hardcoded 30_000ms window with computed AUTH_STORE_SUPPRESS_REAUTH_MS (retries × timeout + backoff + 5s headroom = ~55.5s) - Fix local-mode error message: redirecting to internet-check when the embedded core itself is the unreachable party - Update test assertions for the new timeoutMs param and error wording
… merge The upstream merge bumped vendor/tinyagents to the tinyhumansai#5143 provider refactor which removed the with_requires_streaming builder method. Drop the call since the new OpenAiModel transport already handles streaming correctly.
coderabbitai
Bot
added
bug
rust-core
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14a22e96da
ℹ️ 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".
| .with_context(|| format!("reading config.toml from {}", config_path.display()))?; | ||
| let (mut config, config_was_corrupted) = | ||
| parse_config_with_recovery(&config_path, &raw).await; | ||
| let (raw, read_was_recovered) = read_config_with_recovery_or_default(&config_path).await?; |
There was a problem hiding this comment.
When a non-UTF-8 config is first encountered through reload_config_snapshot_with_timeout (for example from Composio or memory-sync code paths), read_config_with_recovery_or_default renames config.toml before returning backup/default contents, but this snapshot path only logs that it is “skipping persistence”. That leaves the primary config file absent; the next snapshot reload takes the !config_path.exists() branch and returns Config::default() instead of the recovered backup, so live callers can silently lose provider/settings state until an authoritative load/save recreates the file. Keep this reload path read-only, persist the recovered config here, or make the missing-primary branch consult .bak.
Useful? React with 👍 / 👎.
| if lower.contains("connection refused") | ||
| || lower.contains("connection reset") |
There was a problem hiding this comment.
This before-send filter scans every event body for connection refused/connection reset before considering the event envelope. If the managed backend returns a reportable non-2xx error such as Backend returned 500 ...: database connection refused, integrations::client deliberately reports it with status=500/failure=non_2xx, but this new catch-all drops it as client connectivity noise. Exclude Backend returned .../backend non-2xx envelopes or require explicit transport tags so backend outages still reach Sentry.
Useful? React with 👍 / 👎.
| ); | ||
| return Err("document namespace/key cannot contain secrets".to_string()); | ||
| } | ||
| if safety::pii::has_likely_pii(&input.namespace) || safety::pii::has_likely_pii(&input.key) | ||
| { | ||
| log::warn!( | ||
| "[memory:safety] document write rejected due to PII-like namespace/key namespace_chars={} key_chars={}", | ||
| input.namespace.chars().count(), | ||
| input.key.chars().count() | ||
| ); | ||
| return Err("document namespace/key cannot contain personal identifiers".to_string()); | ||
| } | ||
|
|
||
| // Auto-sanitize PII from namespace/key rather than rejecting the entire | ||
| // write (see #5164). Previously this returned an Err, which caused | ||
| // unthrottled retry loops when caller-generated identifiers happened to | ||
| // contain structured personal identifiers (CPF, SSN, RFC, etc.). | ||
| let input = { | ||
| let key = safety::pii::redact_pii(&input.key); | ||
| let namespace = safety::pii::redact_pii(&input.namespace); | ||
| if key.report.pii_redactions > 0 || namespace.report.pii_redactions > 0 { | ||
| log::info!( | ||
| "[memory:safety] document write auto-sanitized PII from namespace/key original_len_ns={} original_len_key={}", | ||
| input.namespace.chars().count(), | ||
| input.key.chars().count() | ||
| ); | ||
| } | ||
| NamespaceDocumentInput { | ||
| namespace: namespace.value, | ||
| key: key.value, | ||
| ..input | ||
| } | ||
| }; |
There was a problem hiding this comment.
The write path now calls redact_pii(&input.key) and stores the document under the sanitized key (e.g. "ssn-123-45-6789" → "[SSN_REDACTED]"), but the kv_get_global / kv_get_namespace read paths in tinycortex do not apply the same transformation before lookup. The new test kv_set_global_auto_sanitizes_pii_like_key explicitly asserts kv_get_global("ssn-123-45-6789").is_none() after writing, confirming the contract break: a caller that stores an entry under a national-ID-derived key and later retrieves it by the same key will silently get a miss. For namespace documents load_documents_for_scope does redact the namespace before lookup (line 392 in this file), but a direct get-by-key or a KV-global lookup has no such guard. Any system path that writes with a PII-containing key and reads back by the same key will silently lose data.
| let (mut merged, mut total_pages) = merge_registry_results(labelled); | ||
|
|
||
| if !any_ok && !registries.is_empty() { | ||
| anyhow::bail!("all MCP registries failed to respond"); | ||
| let sources: Vec<&str> = registries.iter().map(|r| r.source()).collect(); | ||
| tracing::warn!( | ||
| "[mcp-registry] all configured registries failed to respond — returning empty catalog (tried {})", | ||
| sources.join(", ") | ||
| ); | ||
| return Ok((vec![], 0)); | ||
| } | ||
|
|
There was a problem hiding this comment.
The original code bailed with an error so callers could render an explicit "registry error" UI state (see the comment that was just removed: "return an error so the UI shows its registry error state instead of an empty catalog"). Returning Ok((vec![], 0)) collapses that distinction: callers now see the same result whether the registry is genuinely empty or completely unreachable. Users will see a blank catalog with no indication that a connectivity failure occurred. If the caller has a retry or error-banner path gated on Err(...), it is now bypassed. Does the UI caller of registry_search have a separate mechanism to distinguish an empty-but-healthy catalog from a failed fetch, or did it rely on the error return to show a network-error state?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/utils/tauriCommands/auth.ts (1)
68-80: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd complete PII-safe diagnostics for the session-store retry flow.
The changed flow currently logs only the retry delay, so external-call starts/outcomes and terminal branch decisions cannot be reconstructed.
app/src/utils/tauriCommands/auth.ts#L68-L80: log safe RPC start, success, and failure metadata such as timeout and deferred-validation flags—never the token, user object, or raw backend error.app/src/utils/desktopDeepLinkListener.ts#L185-L209: log entry, each attempt, retry/non-retry branch, and final completion using attempt counts and a classified error kind rather than error text.As per coding guidelines, “New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries, state transitions, and errors, without logging secrets or full PII.”
🤖 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 `@app/src/utils/tauriCommands/auth.ts` around lines 68 - 80, Add PII-safe, grep-friendly diagnostics to storeSession in app/src/utils/tauriCommands/auth.ts at lines 68-80, logging RPC start, success, and failure metadata including timeout and deferred-validation flags while excluding token, user, and raw backend errors. Update the retry flow in app/src/utils/desktopDeepLinkListener.ts at lines 185-209 to log entry, every attempt, retry/non-retry decisions, and final completion with attempt counts and classified error kinds rather than error text.Source: Coding guidelines
🧹 Nitpick comments (2)
src/openhuman/inference/paths.rs (1)
903-956: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSerialize the DLL-not-found tests — they share a mutable global.
reset_dll_not_found_backoff_for_test,try_claim_dll_not_found_report, andreport_dll_not_foundall read/write the single process-wide staticLAST_DLL_NOT_FOUND_REPORT. These four#[test]s run in parallel by default, so a sibling test'sreset()(store 0) ortry_claim()(store now) can land between another test's setup and assertion, making the claim/suppress assertions flaky. Route them through one critical section (the module already does this for the install tests viashared_install_lock()).♻️ Example: guard each DLL test
#[test] fn report_dll_not_found_claims_first_call() { + let _g = shared_install_lock(); reset_dll_not_found_backoff_for_test(); let msg = report_dll_not_found("[test]"); assert!(msg.is_some(), "first report must be Some"); assert!(msg.unwrap().contains("Visual C++"), "must mention VC++"); }(apply the same guard to
report_dll_not_found_suppresses_duplicates,try_claim_returns_true_once_then_false, andis_backoff_active_after_claim.)🤖 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/inference/paths.rs` around lines 903 - 956, Serialize the four DLL-not-found tests by acquiring the existing shared test mutex at the start of each test, including dll_not_found_exit_false_on_non_windows, report_dll_not_found_claims_first_call, report_dll_not_found_suppresses_duplicates, try_claim_returns_true_once_then_false, and is_backoff_active_after_claim. Reuse shared_install_lock() (or its established guard pattern) so each test holds the critical section across reset, claim/report calls, and assertions.src/openhuman/config/schema/load/impl_load.rs (1)
19-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit config recovery into
recovery.rs
src/openhuman/config/schema/load/impl_load.rsis 609 lines, past the preferred ~500-line target. Moveread_config_with_recovery_or_defaultandparse_config_with_recoveryinto a dedicated recovery submodule to keep this loader easier to navigate.🤖 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/config/schema/load/impl_load.rs` around lines 19 - 128, Move read_config_with_recovery_or_default and parse_config_with_recovery from impl_load.rs into a dedicated recovery.rs submodule under the same load module. Preserve their existing behavior, visibility, imports, and call sites by exporting or referencing them through the load module as needed, and relocate the WARNED_CONFIG_READ_FAILURE state with the recovery logic.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 `@app/src/assets/audio/README.md`:
- Line 11: Balance the Markdown strikethrough delimiters in the README sentence
by enclosing the complete obsolete specification, including the duration and
LUFS-normalization text, within one matching pair of ~~ markers. Leave the
replacement guidance unchanged.
In `@app/src/components/chat/__tests__/ChatComposer.test.tsx`:
- Around line 255-306: Update LoopHarness so its repeated setCount updates run
from a useEffect rather than directly during render, while preserving the count
threshold that exceeds ChatComposer’s 30-render guard. Keep the existing
ChatComposer setup and warning assertions unchanged so the test exercises the
render-loop warning without triggering React’s nested-render error first.
In `@app/src/utils/__tests__/desktopDeepLinkListener.test.ts`:
- Line 207: Add tests around the desktop deep-link listener flow that simulate a
timeout followed by success, asserting two attempts and the 500 ms backoff, and
add a separate non-timeout failure case asserting no retry. Extend the existing
assertions near allowPendingBackendValidation and timeoutMs without changing the
production retry behavior.
In `@app/src/utils/desktopDeepLinkListener.ts`:
- Around line 414-417: Localize the sign-in failure message returned by the
desktop deep-link listener instead of using a hardcoded literal. Route it
through the existing i18n mechanism, add the corresponding key and English
translation to en.ts and every locale file, and update the related test to
assert the translated output.
In `@src/openhuman/config/schema/load/impl_load.rs`:
- Around line 83-101: Replace the synchronous std::fs::rename call in the
corrupted-config recovery flow with tokio::fs::rename and await its result,
preserving the existing warning context and failure behavior. Keep the
timestamp, corrupted_name, and corrupted_path construction unchanged.
- Around line 303-325: The load_from_default_paths flow must handle a recovered
empty contents value by using Config::default() instead of parsing it with
serde. Extract or reuse a small shared helper for this recovered-empty fallback,
and apply it consistently across the loader paths while preserving the existing
corruption and recovery flags.
In `@src/openhuman/inference/provider/openhuman_backend_model.rs`:
- Around line 255-263: Update maybe_publish_session_expired to accept the
invoking operation as an argument and use it when constructing the
SessionExpired source, rather than hardcoding openhuman_backend_model.invoke.
Pass the appropriate operation identifier from both invoke and stream call sites
so stream-start authentication failures are attributed to stream.
- Around line 297-303: Update the streaming flow around model.stream and the
downstream ModelStreamItem::Failed/ProviderFailed handling so authentication
failures emitted as stream items invoke maybe_publish_session_expired before
becoming generic SSE errors. Ensure both 401 and 403 cases publish
DomainEvent::SessionExpired, preserve existing non-auth error behavior, and add
regression coverage for streamed auth failures.
In `@src/openhuman/mcp_registry/registry.rs`:
- Around line 80-85: Extend the registry search tests around the fallback
implemented after collecting sources to cover both outcomes: configure every
registry search to fail and assert the result is Ok((vec![], 0)), then configure
a mix of successful and failed searches and assert successful results are
retained. Reuse the existing registry test helpers and search entry point rather
than adding separate test infrastructure.
In `@src/openhuman/memory_store/namespace_store/documents_tests.rs`:
- Around line 1130-1136: Strengthen the test around kv_get_global by asserting
that the global store contains exactly one persisted record, then verify its key
is redacted and does not contain the raw SSN. Retain the existing assertion that
the original PII key returns None, and use the available global-record
inspection API rather than relying only on the negative lookup.
In `@src/openhuman/memory_store/namespace_store/documents.rs`:
- Line 395: Update the namespace operations in documents.rs at lines 395-395,
472-474, 548-548, and 621-621 to retain the redact_pii report, sanitize the
redacted value, and emit grep-friendly diagnostics identifying the operation and
redaction count without logging the source identifier. Apply this to scope
loads, filtered listings, namespace deletion, and document deletion
respectively.
- Around line 35-49: The PII redaction flow currently collapses distinct
namespace or key values into the same identity, causing upserts and namespace
operations to collide. In
src/openhuman/memory_store/namespace_store/documents.rs lines 35-49, preserve a
collision-resistant, privacy-safe stable surrogate for redacted identities
before the full document upsert; apply the identical strategy in lines 261-275
for metadata-only writes. Add a regression test covering two distinct values of
the same PII type and verify they remain separate.
In `@src/openhuman/memory_tree/tree/rpc.rs`:
- Around line 72-111: Update the error logging in the SourceKind::Chat,
SourceKind::Email, and SourceKind::Document branches to avoid interpolating
`{e}` or other payload-derived details. Log only stable kind and stage
classifications for deserialization and ingestion failures, while preserving the
detailed error for the returned RPC response where required.
In `@src/openhuman/util.rs`:
- Around line 641-657: Add a Windows-only regression test near the classifier
tests for std::io::Error::from_raw_os_error(665), asserting that the relevant
retry/classification function treats it as retryable. Keep the test scoped to
the raw OS error 665 branch and guarded with #[cfg(windows)].
---
Outside diff comments:
In `@app/src/utils/tauriCommands/auth.ts`:
- Around line 68-80: Add PII-safe, grep-friendly diagnostics to storeSession in
app/src/utils/tauriCommands/auth.ts at lines 68-80, logging RPC start, success,
and failure metadata including timeout and deferred-validation flags while
excluding token, user, and raw backend errors. Update the retry flow in
app/src/utils/desktopDeepLinkListener.ts at lines 185-209 to log entry, every
attempt, retry/non-retry decisions, and final completion with attempt counts and
classified error kinds rather than error text.
---
Nitpick comments:
In `@src/openhuman/config/schema/load/impl_load.rs`:
- Around line 19-128: Move read_config_with_recovery_or_default and
parse_config_with_recovery from impl_load.rs into a dedicated recovery.rs
submodule under the same load module. Preserve their existing behavior,
visibility, imports, and call sites by exporting or referencing them through the
load module as needed, and relocate the WARNED_CONFIG_READ_FAILURE state with
the recovery logic.
In `@src/openhuman/inference/paths.rs`:
- Around line 903-956: Serialize the four DLL-not-found tests by acquiring the
existing shared test mutex at the start of each test, including
dll_not_found_exit_false_on_non_windows, report_dll_not_found_claims_first_call,
report_dll_not_found_suppresses_duplicates,
try_claim_returns_true_once_then_false, and is_backoff_active_after_claim. Reuse
shared_install_lock() (or its established guard pattern) so each test holds the
critical section across reset, claim/report calls, and assertions.
🪄 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: 6a6152c2-37e6-472d-b928-d47c7f477889
📒 Files selected for processing (94)
.github/tauri-cef-expected-shaapp/src-tauri/src/lib.rsapp/src-tauri/vendor/tauri-cefapp/src/agentworld/components/AgentProfileModal.tsxapp/src/agentworld/pages/ExploreSection/index.tsxapp/src/agentworld/pages/MessagingSection.tsxapp/src/agentworld/pages/ProfilesSection.tsxapp/src/assets/audio/README.mdapp/src/chat/chatSendError.tsapp/src/components/chat/ChatComposer.tsxapp/src/components/chat/__tests__/ChatComposer.test.tsxapp/src/components/intelligence/sourcePipelineStatus.tsapp/src/components/oauth/oauthAuthReadiness.tsapp/src/components/settings/panels/AIPanel.tsxapp/src/components/settings/panels/LocalModelDebugPanel.tsxapp/src/components/settings/panels/__tests__/RecoveryPhrasePanel.test.tsxapp/src/components/settings/settingsRouteRegistry.tsapp/src/features/conversations/Conversations.tsxapp/src/features/conversations/components/SubagentDrawer.tsxapp/src/features/conversations/components/ThreadGoalChip.tsxapp/src/lib/agentworld/invokeApiClient.tsapp/src/lib/attachments.tsapp/src/lib/composio/types.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/lib/meshGradient.jsapp/src/pages/Brain.tsxapp/src/pages/onboarding/OnboardingContext.tsxapp/src/services/__tests__/socketService.events.test.tsapp/src/services/analyticsInteractions.tsapp/src/services/api/approvalApi.tsapp/src/services/api/workflowRunsApi.tsapp/src/store/chatRuntimeSlice.tsapp/src/store/notificationSlice.tsapp/src/types/channels.tsapp/src/types/intelligence.tsapp/src/types/rewards.tsapp/src/types/turnState.tsapp/src/utils/__tests__/desktopDeepLinkListener.test.tsapp/src/utils/desktopDeepLinkListener.tsapp/src/utils/oauthAppVersionGate.tsapp/src/utils/tauriCommands/accessibility.tsapp/src/utils/tauriCommands/auth.tsapp/src/utils/tauriCommands/config.tsapp/src/utils/tauriCommands/memoryTree.tsapp/test/e2e/helpers/chat-harness.tsapp/test/e2e/specs/accounts-provider-modal.spec.tsapp/test/e2e/specs/agent-harness-behaviors.spec.tsapp/test/e2e/specs/audio-toolkit-flow.spec.tsapp/test/e2e/specs/slack-flow.spec.tsapp/test/e2e/specs/whatsapp-flow.spec.tssrc/core/dispatch.rssrc/core/jsonrpc.rssrc/core/observability.rssrc/main.rssrc/openhuman/composio/client.rssrc/openhuman/config/ops/loader.rssrc/openhuman/config/schema/load/impl_load.rssrc/openhuman/config/schema/load_tests.rssrc/openhuman/inference/http/server.rssrc/openhuman/inference/local/service/ollama_admin/diagnostics.rssrc/openhuman/inference/local/service/speech.rssrc/openhuman/inference/local/service/vision_embed.rssrc/openhuman/inference/ops.rssrc/openhuman/inference/paths.rssrc/openhuman/inference/provider/claude_agent_sdk/subprocess.rssrc/openhuman/inference/provider/openhuman_backend_model.rssrc/openhuman/inference/voice/local_transcribe.rssrc/openhuman/inference/voice/streaming.rssrc/openhuman/mcp_registry/registry.rssrc/openhuman/memory_store/namespace_store/documents.rssrc/openhuman/memory_store/namespace_store/documents_tests.rssrc/openhuman/memory_store/safety/pii.rssrc/openhuman/memory_tree/tree/rpc.rssrc/openhuman/util.rssrc/openhuman/voice/always_on.rssrc/openhuman/voice/audio_capture.rssrc/openhuman/voice/dictation_listener.rssrc/openhuman/voice/hotkey.rssrc/openhuman/voice/schemas/handlers/provider_server.rssrc/openhuman/voice/server.rsvendor/tinycortex
| return ( | ||
| 'Sign-in could not be completed right now. The session store did not respond in time ' + | ||
| '(even after retrying). Please restart OpenHuman and try again.' | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Localize the new sign-in failure message.
This user-facing literal bypasses the translation system. Route it through the existing i18n path and add the key to English and every locale; update the corresponding test to assert the translated result.
As per coding guidelines, “Route all UI text through useT() and add translations to en.ts and every locale file.”
🤖 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 `@app/src/utils/desktopDeepLinkListener.ts` around lines 414 - 417, Localize
the sign-in failure message returned by the desktop deep-link listener instead
of using a hardcoded literal. Route it through the existing i18n mechanism, add
the corresponding key and English translation to en.ts and every locale file,
and update the related test to assert the translated output.
Source: Coding guidelines
| let ts = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .map(|d| d.as_secs()) | ||
| .unwrap_or(0); | ||
| let stem = config_path | ||
| .file_stem() | ||
| .and_then(|s| s.to_str()) | ||
| .unwrap_or("config"); | ||
| let corrupted_name = format!("{stem}.corrupted.{ts}"); | ||
| let corrupted_path = config_path.with_file_name(&corrupted_name); | ||
| if let Err(rename_err) = std::fs::rename(config_path, &corrupted_path) { | ||
| tracing::warn!( | ||
| src = %config_path.display(), | ||
| dst = %corrupted_path.display(), | ||
| error = %rename_err, | ||
| "[config] Failed to rename corrupted config file; \ | ||
| subsequent loads will fail again" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Blocking std::fs::rename call inside an async fn.
std::fs::rename at Line 93 runs synchronously on the async executor thread. Every other rename/read in this function (and file) uses tokio's fs::rename(...).await — this is the one exception, and it stalls the worker thread for the duration of the syscall instead of being offloaded to tokio's blocking pool.
🐛 Proposed fix
- if let Err(rename_err) = std::fs::rename(config_path, &corrupted_path) {
+ if let Err(rename_err) = fs::rename(config_path, &corrupted_path).await {📝 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 ts = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .map(|d| d.as_secs()) | |
| .unwrap_or(0); | |
| let stem = config_path | |
| .file_stem() | |
| .and_then(|s| s.to_str()) | |
| .unwrap_or("config"); | |
| let corrupted_name = format!("{stem}.corrupted.{ts}"); | |
| let corrupted_path = config_path.with_file_name(&corrupted_name); | |
| if let Err(rename_err) = std::fs::rename(config_path, &corrupted_path) { | |
| tracing::warn!( | |
| src = %config_path.display(), | |
| dst = %corrupted_path.display(), | |
| error = %rename_err, | |
| "[config] Failed to rename corrupted config file; \ | |
| subsequent loads will fail again" | |
| ); | |
| } | |
| let ts = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .map(|d| d.as_secs()) | |
| .unwrap_or(0); | |
| let stem = config_path | |
| .file_stem() | |
| .and_then(|s| s.to_str()) | |
| .unwrap_or("config"); | |
| let corrupted_name = format!("{stem}.corrupted.{ts}"); | |
| let corrupted_path = config_path.with_file_name(&corrupted_name); | |
| if let Err(rename_err) = fs::rename(config_path, &corrupted_path).await { | |
| tracing::warn!( | |
| src = %config_path.display(), | |
| dst = %corrupted_path.display(), | |
| error = %rename_err, | |
| "[config] Failed to rename corrupted config file; \ | |
| subsequent loads will fail again" | |
| ); | |
| } |
🤖 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/config/schema/load/impl_load.rs` around lines 83 - 101, Replace
the synchronous std::fs::rename call in the corrupted-config recovery flow with
tokio::fs::rename and await its result, preserving the existing warning context
and failure behavior. Keep the timestamp, corrupted_name, and corrupted_path
construction unchanged.
| // The key in storage contains the redacted token. | ||
| let stored = memory.kv_get_global("ssn-123-45-6789").await.unwrap(); | ||
| assert!( | ||
| err.contains("cannot contain personal identifiers"), | ||
| "unexpected error: {err}" | ||
| stored.is_none(), | ||
| "original PII key should not match after sanitization" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prove the global value was persisted.
stored.is_none() passes both when the key was redacted and when kv_set_global silently drops the write. Also assert one stored global record exists and its key is redacted/non-raw.
🤖 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/memory_store/namespace_store/documents_tests.rs` around lines
1130 - 1136, Strengthen the test around kv_get_global by asserting that the
global store contains exactly one persisted record, then verify its key is
redacted and does not contain the raw SSN. Retain the existing assertion that
the original PII key returns None, and use the available global-record
inspection API rather than relying only on the negative lookup.
| let input = { | ||
| let key = safety::pii::redact_pii(&input.key); | ||
| let namespace = safety::pii::redact_pii(&input.namespace); | ||
| if key.report.pii_redactions > 0 || namespace.report.pii_redactions > 0 { | ||
| log::info!( | ||
| "[memory:safety] document write auto-sanitized PII from namespace/key original_len_ns={} original_len_key={}", | ||
| input.namespace.chars().count(), | ||
| input.key.chars().count() | ||
| ); | ||
| } | ||
| NamespaceDocumentInput { | ||
| namespace: namespace.value, | ||
| key: key.value, | ||
| ..input | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent redaction-token identity collisions.
Generic replacements such as [REDACTED_PII_SSN] make distinct PII-bearing keys or namespaces identical. The ON CONFLICT(namespace, key) upsert can then overwrite an unrelated document; namespace collisions also make clear/delete affect unrelated data. Use a collision-resistant, privacy-safe stable surrogate and add a regression test with two distinct values of the same PII type.
src/openhuman/memory_store/namespace_store/documents.rs#L35-L49: preserve a collision-resistant safe identity before the full document upsert.src/openhuman/memory_store/namespace_store/documents.rs#L261-L275: apply the identical safe-identity strategy to metadata-only writes.
📍 Affects 1 file
src/openhuman/memory_store/namespace_store/documents.rs#L35-L49(this comment)src/openhuman/memory_store/namespace_store/documents.rs#L261-L275
🤖 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/memory_store/namespace_store/documents.rs` around lines 35 -
49, The PII redaction flow currently collapses distinct namespace or key values
into the same identity, causing upserts and namespace operations to collide. In
src/openhuman/memory_store/namespace_store/documents.rs lines 35-49, preserve a
collision-resistant, privacy-safe stable surrogate for redacted identities
before the full document upsert; apply the identical strategy in lines 261-275
for metadata-only writes. Add a regression test covering two distinct values of
the same PII type and verify they remain separate.
| ) -> Result<Vec<StoredMemoryDocument>, String> { | ||
| let conn = self.conn.lock(); | ||
| let ns = Self::sanitize_namespace(namespace); | ||
| let ns = Self::sanitize_namespace(&safety::pii::redact_pii(namespace).value); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Retain redaction reports for privacy-safe branch diagnostics.
These paths discard redact_pii’s report, so a rewritten lookup or destructive operation is indistinguishable from a normal one. Capture the report and emit operation/redaction-count diagnostics without logging the source identifier. As per coding guidelines, changed flows must include grep-friendly branch diagnostics without PII.
src/openhuman/memory_store/namespace_store/documents.rs#L395-L395: log redaction state for scope loads.src/openhuman/memory_store/namespace_store/documents.rs#L472-L474: log redaction state for filtered listing.src/openhuman/memory_store/namespace_store/documents.rs#L548-L548: log redaction state before namespace deletion.src/openhuman/memory_store/namespace_store/documents.rs#L621-L621: log redaction state before document deletion.
📍 Affects 1 file
src/openhuman/memory_store/namespace_store/documents.rs#L395-L395(this comment)src/openhuman/memory_store/namespace_store/documents.rs#L472-L474src/openhuman/memory_store/namespace_store/documents.rs#L548-L548src/openhuman/memory_store/namespace_store/documents.rs#L621-L621
🤖 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/memory_store/namespace_store/documents.rs` at line 395, Update
the namespace operations in documents.rs at lines 395-395, 472-474, 548-548, and
621-621 to retain the redact_pii report, sanitize the redacted value, and emit
grep-friendly diagnostics identifying the operation and redaction count without
logging the source identifier. Apply this to scope loads, filtered listings,
namespace deletion, and document deletion respectively.
Source: Coding guidelines
| // 665: ERROR_FILE_SYSTEM_LIMITATION — the NTFS filesystem | ||
| // is fragmented, the USN journal has overflowed, or a | ||
| // filter driver resource cap has been hit. Although | ||
| // not always transient (fragmentation is persistent), | ||
| // the USN journal and filter-driver cases can resolve | ||
| // after a delay, so exponential backoff is still | ||
| // better than an immediate bail + unthrottled outer | ||
| // retry. The observability module classifies persistent | ||
| // 665 errors as `ExpectedErrorKind::WindowsFileSystemLimitation` | ||
| // to prevent Sentry flooding (TAURI-RUST-QT0). | ||
| // 1224: ERROR_USER_MAPPED_FILE | ||
| return code == 5 || code == 32 || code == 33 || code == 303 || code == 1224; | ||
| return code == 5 | ||
| || code == 32 | ||
| || code == 33 | ||
| || code == 303 | ||
| || code == 665 | ||
| || code == 1224; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'is_transient_fs_error|retry_with_backoff_async' srcRepository: tinyhumansai/openhuman
Length of output: 19105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '615,700p' src/openhuman/util.rs
printf '\n---\n'
sed -n '1,220p' src/openhuman/credentials/profiles.rs
printf '\n---\n'
sed -n '1000,1360p' src/openhuman/credentials/profiles.rs
printf '\n---\n'
sed -n '1,120p' src/openhuman/memory/read_rpc/admin.rs
printf '\n---\n'
sed -n '1,120p' src/openhuman/config/schema/load/impl_load.rsRepository: tinyhumansai/openhuman
Length of output: 38153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused view of the retry classifier and its direct callers/tests.
sed -n '615,700p' src/openhuman/util.rs
printf '\n--- util tests ---\n'
sed -n '418,490p' src/openhuman/util.rs
printf '\n--- profiles retry sites ---\n'
rg -n -C3 'retry_with_backoff(_async)?\(' src/openhuman/credentials/profiles.rs src/openhuman/memory/read_rpc/admin.rs src/openhuman/config/schema/load/impl_load.rs
printf '\n--- 665 references ---\n'
rg -n '665|ERROR_FILE_SYSTEM_LIMITATION|WindowsFileSystemLimitation' srcRepository: tinyhumansai/openhuman
Length of output: 13187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Small behavioral probe: classify the retry budget usage from the source text.
python3 - <<'PY'
from pathlib import Path
text = Path('src/openhuman/util.rs').read_text()
start = text.index('pub fn is_transient_fs_error')
print(text[start:text.index('pub fn retry_with_backoff', start)])
PYRepository: tinyhumansai/openhuman
Length of output: 265
🌐 Web query:
ERROR_FILE_SYSTEM_LIMITATION 665 retry transient persistent fragmentation USN journal documentation
💡 Result:
ERROR_FILE_SYSTEM_LIMITATION (Win32 error code 665, 0x299) occurs when an NTFS file system operation—typically a read or write—cannot be completed because the file has reached a limit related to its metadata structure, specifically the maximum number of file extents (fragments) that the file system can track [1][2][3]. Root Causes: The error is most frequently triggered by extreme file fragmentation [4][5]. NTFS tracks the physical location of file data on disk using "attributes" within a File Record Segment (FRS) [6][7]. When a large file is updated frequently (e.g., small appends, sparse file operations, or database snapshots), it can become highly fragmented [4][6][8]. Once the number of fragments exceeds the NTFS limit for a single file (typically around 1.5 million extents, though this can vary based on configuration), the file system can no longer manage the metadata required to track the file's location, resulting in error 665 [4][6][8]. Common Contexts: This error is well-documented in high-intensity scenarios, such as: - Large database files (e.g., SQL Server) that undergo frequent auto-growth or intense I/O [6][9][7]. - Online database maintenance operations (e.g., DBCC CHECKDB) that leverage sparse file snapshots [6][9]. - Large-scale backup or logging processes that append data incrementally [4][10][7]. Recommended Mitigation Strategies: - Defragmentation: For standard files, using tools like the Sysinternals Contig utility to defragment the specific file can help by consolidating extents [4][5][10]. Note that for SSDs, copying the file to a new location is often more effective than traditional defragmentation, as it allows the file system to write the data contiguously [7]. - Format with Larger FRS: When formatting the volume, using the /L option (e.g., Format-Volume -UseLargeFRS) increases the File Record Segment size, which expands the maximum number of allowed extents from approximately 1.5 million to roughly 6 million [8][11]. - File Allocation Practices: Pre-allocating the file size (allocating space all at once rather than incrementally) prevents the file from fragmenting during growth [4][10]. - Use ReFS: If the application and environment support it, using the ReFS (Resilient File System) instead of NTFS avoids this specific architectural limitation [7]. - Adjust Write Patterns: Applications performing intensive write operations may benefit from larger write buffer sizes (e.g., 64KB instead of 4KB) to reduce the number of discrete extents created per write [7].
Citations:
- 1: https://www.hresult.info/FACILITY_WIN32/0x80070299
- 2: https://akousa.net/error-codes/windows-error-665
- 3: https://windowsreport.com/error-file-system-limitation/
- 4: https://stackoverflow.com/questions/7323760/what-factors-can-lead-to-win32-error-665-file-system-limitation
- 5: https://ayende.com/blog/192865-B/production-postmortem-the-file-system-limitation
- 6: https://techcommunity.microsoft.com/t5/sql-server-support-blog/operating-system-error-665-8211-file-system-limitation-not-just/ba-p/318587
- 7: https://techcommunity.microsoft.com/blog/sqlserversupport/operating-system-error-665-8211-file-system-limitation-not-just-for-dbcc-anymore/318587
- 8: https://forums.passmark.com/osforensics-osfmount-osfclone/37599-error-while-rebuilding-raid-665-file-system-limitation
- 9: https://learn.microsoft.com/en-us/troubleshoot/sql/database-engine/database-file-operations/1450-and-665-errors-running-dbcc-checkdb
- 10: https://www.exchangetuts.com/what-factors-can-lead-to-win32-error-665-file-system-limitation-1641211204573442
- 11: https://learn.microsoft.com/en-us/archive/blogs/psssql/sparse-file-errors-1450-or-665-due-to-file-fragmentation-fixes-and-workarounds
Add a Windows regression test for raw OS error 665
The retry paths already use finite budgets, so the main gap is coverage: add a #[cfg(windows)] test for std::io::Error::from_raw_os_error(665) to lock in the new classifier branch.
🤖 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/util.rs` around lines 641 - 657, Add a Windows-only regression
test near the classifier tests for std::io::Error::from_raw_os_error(665),
asserting that the relevant retry/classification function treats it as
retryable. Keep the test scoped to the raw OS error 665 branch and guarded with
#[cfg(windows)].
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f0aa1fe1d
ℹ️ 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".
| let display_msg = maybe_msg.unwrap_or_else(|| { | ||
| format!("{LOG_PREFIX} whisper-cli unavailable (STATUS_DLL_NOT_FOUND — check VC++ Redistributable)") | ||
| }); | ||
| return Err(display_msg); |
There was a problem hiding this comment.
On Windows desktops missing the VC++ runtime, the CLI fallback for voice_transcribe_bytes still returns an ordinary Err on every dictation attempt; the cooldown only suppresses the helper's warn!, while /rpc will continue to send this message through report_error_or_expected, and there is no core matcher for STATUS_DLL_NOT_FOUND (the only matches are these new call sites). Mark this error as expected/structured, or add a central classifier, so the missing-runtime case does not keep flooding Sentry despite the backoff.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| fn dll_not_found_exit_false_on_non_windows() { | ||
| // On non-Windows, all exit codes return false. | ||
| assert!(!is_dll_not_found_exit(Some(-1073741515))); |
There was a problem hiding this comment.
This test is added unconditionally, but on Windows is_dll_not_found_exit(Some(-1073741515)) is supposed to return true; running the Rust tests on a Windows target will therefore fail. Add #[cfg(not(windows))] to this case (or split out a Windows-specific assertion) so the new platform-specific detector does not break the shipped Windows target's test run.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
…afe error logging, session-expiry operation source, retry test coverage
|
PR Babysitter Status — Tick 1 Inspected head: Fixes applied this session:
Threads handled:
CI Lite status: PENDING (not yet picked up by runners) Remaining CodeRabbit comments: 9 (being triaged — some are "Heavy lift" items beyond PR scope, others are minor improvements) Next action: Wait for CI Lite to run and address any failures. |
2 participants
Summary
Triage of 75 unresolved Sentry issues across
tauri-react,tauri-rust, andcore-rustprojects, resulting in 16 fixes covering both specific bugs and systematic noise reduction.Fixes by category
Crash / unhandled error fixes (6)
TypeError: postMessage undefined(117e/36u):typeofguard onwindow.ipc.postMessagein vendored CEF IPC protocolUnhandledRejection: threads_create_new timeout(18e/3u): try/catch + user-facing error + 14 i18n localesclassListnull access (27e/13u): null guards in meshGradient (3 sites)checkednull access (8e/7u): try-catch onelement.checkedfor React 19 DOM disconnect edge caseauth_me_timeout(26e/4u): retry wrapper (10s/2 attempts) + error→warning downgradeLog level downgrades (4)
unknown method: harness_init_status(64k events): added toKNOWN_PROBE_METHODSallowlist → debug!all MCP registries failed(452e/17u): anyhow::bail! → warn! + empty catalog fallbackpersonal identifiersvalidation spam (3k events): PII rejection → auto-sanitization in documents.rs + tinycortex KV storemissing timestamp(14e): ingest deserialization errors → warn! (timestamp already had serde default)Graceful degradation (4)
HTTP 401 Invalid token(350e/32u):maybe_publish_session_expired()on 401 +is_session_expired_messageformat matchos error 665flood (6k events): 3-layer defense (expected-error classifier + 5-min rate limiter + before_send filter + retry_with_backoff)Stream must be set to true(511e/2u):requires_streamingflag + 400 auto-detection retry in tinyagentsconfig.tomlUTF-8 corruption (946e/2u): recovery path (rename → .bak → defaults) + once-per-process rate limitSystematic noise reduction (#5170)
error!→warn!downgrades across inference RPC handlersbefore_sendfilters:is_user_config_provider_event,is_connectivity_event,is_stale_release_eventRemaining in Sentry (already tracked)
Files changed
~45 files across Rust core, Tauri shell, frontend, vendored submodules (tinyagents, tinycortex, tauri-cef), and i18n locales.
Summary by CodeRabbit