fix(threads): typed ThreadNotFound error + skip Sentry for stale-thread RPCs (OPENHUMAN-TAURI-4H, -60)#1570
fix(threads): typed ThreadNotFound error + skip Sentry for stale-thread RPCs (OPENHUMAN-TAURI-4H, -60)#1570oxoxDev wants to merge 9 commits into
Conversation
📝 WalkthroughWalkthroughStructured "thread not found" errors are added end-to-end: Rust introduces ThreadsError and structured envelopes, rpc_handler propagates error.data, TypeScript CoreRpcError/classifyRpcError recognize the payload, Redux clears stale threads and refreshes lists, UI suppresses send error, and tests cover the flow. ChangesThread-not-found structured error handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/store/threadSlice.ts (1)
142-155: ⚡ Quick winDeduplicate stale-thread recovery flow across thunks.
The stale-thread branch is repeated in three places (clear local state → refresh threads → dev log → reject standardized message). Extracting this into one helper will reduce drift risk when recovery behavior changes.
Also applies to: 192-205, 221-234
🤖 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/store/threadSlice.ts` around lines 142 - 155, Extract the repeated stale-thread recovery sequence into a single helper (e.g., handleStaleThreadRecovery) and call it from each thunk instead of duplicating logic: the helper should accept (dispatch, payload.threadId, rejectWithValue) and perform the same steps currently repeated — call isThreadNotFoundCoreRpcError flow’s clearStaleThread via dispatch(clearStaleThread(threadId)), await dispatch(loadThreads()).unwrap() inside a try/catch that logs the refreshError when IS_DEV (same debug message text), and finally return rejectWithValue(THREAD_NOT_FOUND_MESSAGE); replace the three duplicated blocks (the one around addMessageLocal and the blocks at the other locations) with calls to this helper to centralize behavior and avoid drift.
🤖 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/core/jsonrpc.rs`:
- Around line 63-73: The jsonrpc handler currently contains a domain-specific
branch using thread_not_found_rpc_error_data(&method, &message) and logging a
thread-specific "thread-not-found" message; remove this domain-specific
classification from rpc_handler/jsonrpc.rs and instead surface a structured
ThreadNotFound error through the controller/error boundary (e.g., return a
domain error variant or attach a ThreadNotFound payload from the
controller/handler code so jsonrpc.rs only sees a generic error enum). Update
the controller/registered handler that currently recognizes openhuman.threads_*
to produce the ThreadNotFound structured error (so
thread_not_found_rpc_error_data is no longer used in jsonrpc.rs), keep the
generic is_session_expired_error(&message) check in jsonrpc.rs, and ensure
jsonrpc.rs treats the propagated ThreadNotFound as a general error
(logging/metrics) without domain-specific fields so transport layer stays
generic.
---
Nitpick comments:
In `@app/src/store/threadSlice.ts`:
- Around line 142-155: Extract the repeated stale-thread recovery sequence into
a single helper (e.g., handleStaleThreadRecovery) and call it from each thunk
instead of duplicating logic: the helper should accept (dispatch,
payload.threadId, rejectWithValue) and perform the same steps currently repeated
— call isThreadNotFoundCoreRpcError flow’s clearStaleThread via
dispatch(clearStaleThread(threadId)), await dispatch(loadThreads()).unwrap()
inside a try/catch that logs the refreshError when IS_DEV (same debug message
text), and finally return rejectWithValue(THREAD_NOT_FOUND_MESSAGE); replace the
three duplicated blocks (the one around addMessageLocal and the blocks at the
other locations) with calls to this helper to centralize behavior and avoid
drift.
🪄 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: 56935af7-5556-4fb1-b09e-74c0735985d0
📒 Files selected for processing (14)
Cargo.tomlapp/src/pages/Conversations.tsxapp/src/services/__tests__/coreRpcClient.test.tsapp/src/services/coreRpcClient.tsapp/src/store/__tests__/threadSlice.test.tsapp/src/store/threadSlice.tssrc/core/jsonrpc.rssrc/core/jsonrpc_tests.rssrc/openhuman/memory/conversations/store.rssrc/openhuman/threads/error.rssrc/openhuman/threads/mod.rssrc/openhuman/threads/ops.rssrc/openhuman/threads/ops_tests.rstests/json_rpc_e2e.rs
…oundary Introduces `StructuredRpcError` + sentinel-encoded transport so domain controllers can surface typed RPC errors (with `data`, `expected_user_state`) without forcing every handler to change signature and without the JSON-RPC transport branching on RPC method names. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves the `kind=ThreadNotFound` / `thread_id` payload and the "skip Sentry" flag out of the JSON-RPC transport layer and into the threads error boundary. `From<ThreadsError> for String` now encodes the structured envelope at the controller handler boundary; jsonrpc.rs decodes it generically without knowing about the threads domain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`rpc_handler` no longer inspects the RPC method name to recover ThreadNotFound semantics or to decide whether to suppress Sentry. It now decodes any `StructuredRpcError` envelope emitted by a domain controller and honors the generic `expected_user_state` flag uniformly across domains, matching the workspace rule that transport layers must stay domain-agnostic (CLAUDE.md / AGENTS.md). Replaces the threads-scoped helper test with a generic dispatch test that asserts the structured envelope surfaces on the wire without leaking the encoded sentinel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The transport layer no longer stamps the originating RPC method into `error.data` — that field leaked the transport concern into the domain envelope. Frontend keys on `data.kind` + `data.thread_id` (see coreRpcClient.isThreadNotFoundRpcData), so the assertion was checking a field nothing actually consumes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/threads/error.rs`:
- Around line 26-31: from_thread_scoped_store_error currently treats any parsed
"not found" id as the requested thread and returns Self::not_found(thread_id);
change it to first call parse_thread_not_found_message(&err), and only return
Self::not_found(thread_id) when the parser yields Some(parsed_id) and parsed_id
equals the provided thread_id (compare as strings/str slices). If the parsed id
is None or does not match thread_id, return Self::Message(err) to avoid clearing
the wrong thread. Ensure you reference parse_thread_not_found_message,
from_thread_scoped_store_error, Self::not_found and Self::Message when making
the change.
🪄 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: 4a7e48c8-316b-4089-9a0c-23cea9200730
📒 Files selected for processing (7)
src/core/jsonrpc.rssrc/core/jsonrpc_tests.rssrc/openhuman/threads/error.rssrc/openhuman/threads/mod.rssrc/rpc/mod.rssrc/rpc/structured_error.rstests/json_rpc_e2e.rs
✅ Files skipped from review due to trivial changes (1)
- src/rpc/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/threads/mod.rs
| pub fn from_thread_scoped_store_error(thread_id: &str, err: String) -> Self { | ||
| if parse_thread_not_found_message(&err).is_some() { | ||
| Self::not_found(thread_id) | ||
| } else { | ||
| Self::Message(err) | ||
| } |
There was a problem hiding this comment.
Only classify NotFound when the ids agree.
from_thread_scoped_store_error ignores the id parsed from the store error and always re-emits the requested thread_id. If the store ever reports a different missing id, the frontend will clear the wrong stale thread and the underlying mismatch disappears. Fall back to Message(err) unless the parsed id matches.
Suggested guard
pub fn from_thread_scoped_store_error(thread_id: &str, err: String) -> Self {
- if parse_thread_not_found_message(&err).is_some() {
- Self::not_found(thread_id)
+ if let Some(found_thread_id) = parse_thread_not_found_message(&err) {
+ if found_thread_id == thread_id {
+ Self::not_found(thread_id)
+ } else {
+ Self::Message(err)
+ }
} else {
Self::Message(err)
}
}📝 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.
| pub fn from_thread_scoped_store_error(thread_id: &str, err: String) -> Self { | |
| if parse_thread_not_found_message(&err).is_some() { | |
| Self::not_found(thread_id) | |
| } else { | |
| Self::Message(err) | |
| } | |
| pub fn from_thread_scoped_store_error(thread_id: &str, err: String) -> Self { | |
| if let Some(found_thread_id) = parse_thread_not_found_message(&err) { | |
| if found_thread_id == thread_id { | |
| Self::not_found(thread_id) | |
| } else { | |
| Self::Message(err) | |
| } | |
| } else { | |
| Self::Message(err) | |
| } | |
| } |
🤖 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/threads/error.rs` around lines 26 - 31,
from_thread_scoped_store_error currently treats any parsed "not found" id as the
requested thread and returns Self::not_found(thread_id); change it to first call
parse_thread_not_found_message(&err), and only return Self::not_found(thread_id)
when the parser yields Some(parsed_id) and parsed_id equals the provided
thread_id (compare as strings/str slices). If the parsed id is None or does not
match thread_id, return Self::Message(err) to avoid clearing the wrong thread.
Ensure you reference parse_thread_not_found_message,
from_thread_scoped_store_error, Self::not_found and Self::Message when making
the change.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — fix(threads): typed ThreadNotFound error + skip Sentry for stale-thread RPCs
Walkthrough
Clean end-to-end implementation of typed stale-thread error handling. A generic StructuredRpcError envelope lets domain errors carry machine-readable data through the existing Result<_, String> channel without changing every controller signature. The transport layer decodes it domain-agnostically — no method-name branching. Frontend gets a thread_not_found error kind, clears stale state, and suppresses the toast. Test coverage is solid across all layers including a Sentry TestTransport smoke test.
Changes
| File | Summary |
|---|---|
Cargo.toml |
Adds "test" feature to existing sentry 0.47.0 |
src/rpc/structured_error.rs |
New — generic structured-error envelope with encode/decode |
src/openhuman/threads/error.rs |
New — ThreadsError enum with NotFound variant + parser |
src/openhuman/threads/ops.rs |
message_append and thread_generate_title return ThreadsError |
src/openhuman/memory/conversations/store.rs |
Normalises "does not exist" → "not found" |
src/core/jsonrpc.rs |
Transport decodes StructuredRpcError, skips Sentry for expected_user_state |
app/src/services/coreRpcClient.ts |
thread_not_found kind + isThreadNotFoundCoreRpcError helper |
app/src/store/threadSlice.ts |
clearStaleThread reducer + stale-thread recovery in 3 thunks |
app/src/pages/Conversations.tsx |
Suppresses send-error toast for stale threads |
| Tests (6 files) | Unit, JSON-RPC E2E, Sentry transport smoke, frontend slice + RPC client |
Verified / looks good
StructuredRpcError::decodeis pure and panic-free — gracefulNoneon corrupt envelopessentry "test"feature is correctly scoped — not a new runtime dependencyclearStaleThreadreducer correctly handleswelcomeThreadIdisThreadNotFoundCoreRpcErrorwith missingerrorThreadIdreturnstrue— correct conservative behavior- Generic test IDs (
"t-1","thread-123") — no real names/emails - No
import.meta.env, nowindow.__TAURI__, no dynamic imports in production code - New Rust files placed in proper subdirectories, not at
src/openhuman/root
See inline comments for actionable findings.
| display_message | ||
| ); | ||
| } else if !is_session_expired_error(&display_message) { | ||
| crate::core::observability::report_error( |
There was a problem hiding this comment.
[major] Potential merge-conflict regression: report_error vs report_error_or_expected
The current main branch uses report_error_or_expected at this call site (introduced to suppress Sentry noise for LocalAiDisabled and ApiKeyMissing conditions). This PR replaces it with bare report_error, which would re-introduce Sentry noise for those expected errors once merged.
This likely happened because the PR branched before report_error_or_expected landed on main. When rebasing, make sure this line becomes:
crate::core::observability::report_error_or_expected(
display_message.as_str(),
"rpc",
"invoke_method",
&[("method", method.as_str()), ("elapsed_ms", &ms.to_string())],
);Both suppression mechanisms (structured expected_user_state flag for new domains + expected_error_kind() pattern matching for existing plain-string errors) need to coexist.
| } | ||
| return { threadId: payload.threadId, message: persisted }; | ||
| } catch (error) { | ||
| if (isThreadNotFoundCoreRpcError(error, payload.threadId)) { |
There was a problem hiding this comment.
[minor] Stale-thread recovery block is copy-pasted three times (here, addInferenceResponse, and generateThreadTitleIfNeeded).
Consider extracting to a shared helper:
async function handleStaleThread(
threadId: string,
dispatch: AppDispatch,
thunkName: string
): Promise<void> {
dispatch(clearStaleThread(threadId));
try {
await dispatch(loadThreads()).unwrap();
} catch (refreshError) {
if (IS_DEV) {
console.debug(`[threadSlice] ${thunkName} stale-thread refresh failed`, {
threadId,
error: refreshError,
});
}
}
}Functionally correct as-is — all three copies are tested — but DRY would reduce the chance of future divergence when the recovery logic evolves.
| await dispatch(addMessageLocal({ threadId: sendingThreadId, message: userMessage })).unwrap(); | ||
| } catch (error) { | ||
| const msg = error instanceof Error ? error.message : String(error); | ||
| if (msg === THREAD_NOT_FOUND_MESSAGE) { |
There was a problem hiding this comment.
[minor] String equality check on THREAD_NOT_FOUND_MESSAGE works today but is fragile.
dispatch(thunk).unwrap() on a rejectWithValue throws the payload directly (a plain string), so String(error) matches. But if addMessageLocal ever wraps the rejection in an Error object (e.g. in a future middleware), this equality check silently stops matching.
A comment here would clarify the contract:
// error is the rejectWithValue payload (plain string), not an Error object —
// THREAD_NOT_FOUND_MESSAGE is a string sentinel from threadSlice.
if (msg === THREAD_NOT_FOUND_MESSAGE) {Alternatively, checking isThreadNotFoundCoreRpcError before unwrap() would be more robust, but that changes the control flow.
| /// `Result<_, String>` error channel. | ||
| pub fn encode(&self) -> String { | ||
| // serde_json::to_string on a struct of String/Option<Value>/bool | ||
| // cannot fail in practice, so unwrap is acceptable. |
There was a problem hiding this comment.
[nitpick] .unwrap_or_else(|_| String::from("{}")) silently produces an invalid envelope if serialization fails. Since serde_json::to_string on String + Option<Value> + bool cannot fail in practice, consider expect("StructuredRpcError serialization cannot fail") — any future struct change that breaks serialization would then fail visibly in CI rather than producing a silent sentinel-prefixed {}.
| "createdAt": "2026-01-01T00:00:00Z" | ||
| } | ||
| }), | ||
| ) |
There was a problem hiding this comment.
[nitpick] The comment below says the transport no longer stamps method into the structured error data — but the test only asserts kind and thread_id are present without asserting method is absent. Adding:
assert!(
append_err["data"]["method"].is_null(),
"method must not appear in structured error data"
);would make the contract explicit and catch a future accidental re-introduction.
2 participants
Summary
ThreadsError::NotFoundpath for stale conversation thread references.thread <id> not found.kind: "ThreadNotFound"and skips Sentry reporting for this expected user state.Problem
openhuman.threads_message_appendandopenhuman.threads_generate_titleto run against threads that no longer exist.report_error, creating Sentry noise underdomain=rpc operation=invoke_method.does not existandnot found), and the frontend had no structured discriminator to branch on safely.Solution
ThreadsError::NotFound { thread_id }and canonical parsing for both current and legacy missing-thread text.threads_message_appendandthreads_generate_titleto the typed error.ThreadNotFoundonly foropenhuman.threads_*methods, return structured error data, and skipreport_error.TestTransportsmoke test provingThreadNotFounddoes not emit an event while unrelated RPC errors still do.thread_not_found; clear stale thread refs, reload threads, avoid retrying the failed RPC, and suppress the send-error toast for the benign stale-thread case.Submission Checklist
pnpm test:coverage/pnpm test:rustnot run); CIdiff-coveris the authoritative check.## Related— N/A: Sentry-noise cleanup is not represented by a matrix feature ID.Closes #NNNin the## Relatedsection — N/A: source task tracks Sentry issue IDs, not a GitHub issue number.Impact
datafor newer clients.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/thread-not-found-noisebe9647f596d185a416edf69749232f7a5aa82f63Validation Run
pnpm --filter openhuman-app format:check(also passed in pre-push hook)pnpm typecheckcargo test --lib thread_not_found_rpc_error -- --nocapturecargo test --lib openhuman::threadscargo test --test json_rpc_e2epnpm debug unit src/services/__tests__/coreRpcClient.test.ts src/store/__tests__/threadSlice.test.tspnpm --filter openhuman-app test:unitcargo fmt --check --allcargo check --manifest-path Cargo.tomlcargo test --lib openhuman::threadscargo test --test json_rpc_e2ecargo fmt --manifest-path app/src-tauri/Cargo.toml --all --checkpnpm --filter openhuman-app rust:checkValidation Blocked
command:cargo clippy --workspace -- -D warningserror:fails on pre-existing workspace warnings/errors outside this patch, including unused imports insrc/openhuman/agent/harness/builtin_definitions.rs,src/openhuman/channels/providers/telegram/channel.rs,src/openhuman/composio/providers/profile.rs,clippy::empty_line_after_doc_commentsinsrc/openhuman/tools/impl/agent/onboarding_status.rs, and many existingderivable_impls/manual_pattern_char_comparison/too_many_argumentsfindings.impact:workspace clippy remains blocked independently of this branch; changed behavior is covered by the focused Rust/JSON-RPC/frontend tests above.Behavior Changes
Parity Contract
report_error; session-expired suppression remains unchanged.openhuman.threads_*and rejects unrelatednot foundstrings such as message-level misses.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests