Repository navigation
fix(native-chat): structured chat failures reach the diagnostic trace instead of vanishing - #24079
Closed
brennanb2025 wants to merge 8 commits into
Closed
brennanb2025 wants to merge 8 commits into
brennanb2025 wants to merge 8 commits into
Conversation
…e diagnostic trace Structured chat session code reported background failures (exit recovery, lease renewal, journal writes, late settlement, restart recovery) through an optional onError/onEventSinkError sink chain that no installer supplied, and through console.warn/error calls that reach nothing in a packaged build. Both dropped every failure, which is why the "native Codex chat cannot resume after update" settlement failure stayed invisible. - Add reportAgentSessionFailure (src/main/observability/agent-session-failure.ts): a failed `agent-session.failure` span with a closed step vocabulary and the session id, written through the ambient tracer to the redacted, rotating main.trace.ndjson, plus a console line for dev runs. Never throws. Steps whose errors can quote the recovery capsule record the error's kind only. - Remove the reporting-only sink hops (runtime onError, host onEventSinkError, onBarrierError, onDeliveryError, onNoteFailed, and the onError deps of the lease renewer, idle sweep, delivery loop, queued drain, lifecycle delivery and dead-generation settlement). Each catch site reports its own step. - Keep behavioral callbacks required: the event sink's onFailure forces the session's provider down; dead-generation settlement returns its error so the host stop can throw it as the cause. - Migrate the console failure sites in native-chat agent-session code and runtime structured-agent-session files, attaching the error where it was dropped. - Ratchet test fails on console.warn/error or an optional on*Error? sink in that scope. No user-visible behavior change.
9 tasks done
…view - Rename the reporter to observability/agent-session-error-trace (traceAgentSessionError) so it no longer shares a name with the user-facing failure vocabulary in shared/agent-session-failure. - Derive the step union from one step -> 'full' | 'kind' table, so a new step must choose what its record may carry; test every 'kind' step through a real trace file. - Keep error.code in full records; the report's own step and session win over detail keys. - Record a stop-time dead-generation settlement failure once, where it is swallowed. - Report the remaining console-only structured session failures (Claude resume point, exit cursor, dispatch, history window, option restore; chat create) and two swallowed durable writes; widen the ratchet to those files, optional on*Failure/on*Failed sinks and bracket console access. - Pass the now-required onFailure in the e2e unit test that still omitted it.
…ed-session-failure-reports
… again
- Revert the round-1 move of the dead-generation settlement report: the settlement records its own
failure, as the cancel, close, tab-close, /clear and quit paths swallow or strip the cause; drop
the session-close report that only existed to recover it. Pin one record per host-stop failure.
- Error-trace tests: drive the kind-only cases from a pinned step list, and quote the sentinel
where V8's JSON.parse message actually repeats it. The operation-settlement privacy check reads
each console argument's text instead of JSON.stringify, which prints an Error as {}.
- Pin the queued-send-failure-hold and dispatch-doubt-record reports.
- Claude dispatch, attachment-read and option-restore records carry the Orca session id.
…ery tick After one store EACCES at provider exit, lease renewal failed with ownership_unknown every 10 s for as long as the app ran, and each failure wrote a trace span. The renewer now records a session's lease-renewal or lease-probe failure once until that session renews or the failure changes, keyed on the error's code where it has one, and forgets sessions that stop being renewable.
…ed-session-failure-reports
The trace tests narrow parsed trace lines with a guard instead of casting, and the lease-renewer repeat tests run on a real record store with spies instead of a cast stub. The two pre-existing runtime-state doubles this branch re-touched carry line-specific SAFETY rationales.
…be and per-session rules The failure key read an error's name, code and message; a hostile getter would have rejected the renewal tick, so it falls back to a constant. Tests pin one lease-probe record per session when the batch probe keeps failing, and that a renewal clears only its own session's recorded failure.
Contributor
Author
Review status: ready (head 4a2e408)The problem. When something failed in the background of a structured native chat (recording that the agent process exited, renewing the session lease, writing the journal, saving a Claude resume point), Orca recorded nothing a person could find. Failures went either to an optional callback that the only production caller never supplied, or to What changes for the user. Nothing visible in the chat. Each such failure now leaves one Fixed during review:
Deferred (nits):
Verified:
Not verified:
Found in QA, not caused by this PR: after a short store failure at provider exit, the next message fails with "Claude couldn't restart", even after permissions are restored. Main behaves identically. The trace this PR adds now shows the likely cause: the lease stays in |
brennanb2025
marked this pull request as ready for review
September 30, 2026 10:13
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .catch(() => { | ||
| console.warn('[structured-agent-session] restart offer completion failed') | ||
| }) | ||
| .catch((error: unknown) => traceAgentSessionError({ step: 'restart-offer-complete', error })) |
There was a problem hiding this comment.
Knowledge Base Used: Agent workspaces and sessions
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — reviewed the full 126-file diff at 4a2e408, focused on the new reporter, the callback removals, and the lease-renewer dedupe.
- New failure reporter —
traceAgentSessionErrorinsrc/main/observability/agent-session-error-trace.tswrites one failedagent-session.failurespan plus aconsole.warn, never throws, and gates per step between'full'and'kind'. - Step table —
AGENT_SESSION_ERROR_STEPS(70 entries, 15'kind') is closed, so a new catch site cannot compile without choosing what it may record. - Callback removal — optional
on*Error?/on*Failure?/on*Failed?sinks are gone from host/journal/lease/idle/delivery/queued/runtime; every catch reports its own step. Behavioral callbacks (onFailureforce-close,onOpened) are retained and now required. - Dead-generation settlement — returns
{ ok: true } | { ok: false; error }; the host-lifetime caller rethrows withcause: settled.error. - Lease-renewal dedupe — a stuck lease reports once per session until it renews, leaves the renewable set, or the failure key changes; the key uses the error
code, not its per-attempt temp path. - Ratchet + privacy tests — a repo scan fails on new
console.warn/console.erroror optional error sinks in the structured-session trees; a table-driven test proves every'kind'step drops a prompt-quotingSyntaxError.
Two claims were checked against the source and hold: no 'full' step's error can carry prompt/draft/journal-row/transcript text (the one content-quoting JSON.parse, the recovery capsule, sits behind the 'kind' steps), and no removed callback carried a non-reporting side effect (the provider force-close on sink failure is preserved).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (126)
💤 Files with no reviewable changes (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change improves structured-chat diagnostics without an established regression in chat behavior or recovery. It appears mergeable after normal checks; platform-specific live validation remains limited. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change improves failure diagnosis, but some newly reported errors also reach process output without redaction. Protected diagnostic files and the opt-in feature limit exposure. Who can access or collect process output remains uncertain. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 54.05% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 50 files. (72 skipped: 72 over the file limit.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
Superseded by #24312, which landed the same fix with a required |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
When something fails in the background of a structured native chat (the Codex/Claude chat UI), Orca records nothing a person can find. Examples: recording that the agent process exited, renewing the session lease, writing the conversation journal, settling a late message, saving the point where a Claude chat can resume, saving restart-recovery data. The chat may show that something went wrong (#23116, #22999, #23679), but the error itself, its stack and its cause are lost.
This is what kept the Sep 10 P0 ("native Codex chat cannot resume after update") invisible: the write that records a provider's exit kept failing, and no log said so. That P0 itself is fixed on main (#22820, #20509). This PR makes the next failure like it diagnosable: each one now lands in the local diagnostic trace with the step that failed and the chat's session id.
Scope: structured native chat only (behind
experimentalStructuredNativeChat, off by default), main process. That covers the Codex/Claude structured session host and journal, the Claude structured-session adapter, and the structured-session create RPC. Not affected: terminal agents, the renderer, the chat UI (nothing visible changes), other subsystems' logging.Supersedes #20542, which wired the one missing callback but kept the optional-callback shape that caused the loss.
ELI5
The chat's background machinery had two ways to shout "this failed": a phone line nobody ever plugged in, and shouting into a room that doesn't exist in the shipped app. Now every failure writes a note in the app's diagnostic trace instead, and a test stops anyone adding a new silent shout.
What Changed
Before: a background failure in a structured chat went to an optional callback that the only production caller never supplied, or to
console.warn, which a packaged Electron main process has nowhere to print. Either way it was gone. After: it becomes oneagent-session.failurerecord inlogs/main.trace.ndjson(the rotated, size-capped, redacted trace the diagnostic bundle already collects), withstep,sessionIdand the error. The user sees no difference in the chat.traceAgentSessionError({ step, sessionId, error, detail })insrc/main/observability/agent-session-error-trace.tswrites a failed span through the existing ambient tracer plus a console line (the only record where no trace sink is installed, e.g. the headless server or diagnostics disabled). It never throws.'full'(message, stack, cause chain, error code) or'kind'(only the error's class and code, e.g.SqliteError SQLITE_FULL); the step type is derived from it, so a new step can't compile without that choice. The 15'kind'steps are the restart-recovery and operation-settlement steps that existing privacy tests already keep out of logs: reading the recovery capsule (which holds the user's latest prompt) can throw aJSON.parseerror that quotes its input.onError, hostonEventSinkError,onBarrierError,onDeliveryError,onNoteFailedand the reporting-onlyonErrordeps of the lease renewer, idle sweep, delivery loop, queued drain and lifecycle delivery are removed; each catch site reports with its own step and session id.onFailurestill forces the failing session's provider down, exactly as before. Dead-generation settlement returns{ ok } | { ok: false, error }and the host stop still throws that error as the cause.console.warn/console.erroror an optionalon*Error?/on*Failure?/on*Failed?callback reappears in structured-session production code (agent-session wire/journal, model catalog, runtimestructured-agent-session-*, Claudeclaude-structured-*, RPCstructured-agent-session-*). Two behavioral optional callbacks are allowlisted with a comment.Why
The bug was structural: a failure's destination was optional, so forgetting to wire it was silent, and the fallback (the console) does not exist in a shipped app. Wiring the missing callback (#20542) fixes today's gap but leaves nine optional hops that the next refactor can break the same way. Here reporting doesn't depend on any caller: the trace sink is installed once at startup, the same way the tracer is already used across the main process (one span helper per domain in
observability/), and every catch site reports with its own context. It removes plumbing rather than adding it, and the ratchet keeps the class from coming back.Differences from how comparable session runtimes do this:
Privacy notes.
'full'steps can record local file paths from filesystem errors (OS username, project folder), which the redactor keeps by existing policy.provider-start,operation-agent-startandclaude-dispatch-writecan carry the provider's stderr tail (up to 8 KB for Claude) or Claude's error-result text; Orca already stores that text locally and shows it behind "Details", and the diagnostic bundle is previewed before any upload. No'full'step was found to carry prompt, message, draft, journal-row or transcript text.Linked Issue
N/A (maintainer). Follow-up to the Sep 10 P0 fixed by #22820 and #20509; supersedes #20542.
Visual Proof
There is no UI change; the proof is that the chat looks identical while the trace now records the failure. Live QA on a remote Mac (M4Air), rig Orca with an isolated profile and home, a structured Claude chat. Scenario: send one message, make the session store folder read-only,
kill -TERMthe Claude process, restore permissions.Chat after the failure, this PR vs main (identical apart from the clock):
Trace after the fix (538b8cc), all with the chat's sessionId; main writes none of these:
lease-renewalclaude-exit-cursor-persistprovider-exit-lease-releaselease-renewalagent_session_ownership_unknownNothing further in the next 203 s (before the dedupe, 33cd520 wrote this
lease-renewalline every 10 s: 42 lines in 6.5 min).Sending another message after permissions are restored fails the same way on this PR and on main ("Claude couldn't restart"); that is an existing problem, not changed here:
Testing
Typecheck (
config/tsconfig.node.json, through the machine typecheck gate): clean on every pushed head.Changed-lines quality gate (
check-changed-code-quality.mjs) and oxlint on all changed files: clean.Explicit vitest files per round (reporter, ratchet, agent-session wire/journal, runtime structured-*,
orca-runtime.test.ts, Claude structured tests that changed,tests/e2e/codex-child-approval-activity-row.unit.test.ts): 2394 passed / 2 skipped at the round-2 head; renewer and reporter suites re-run after each later fix.Privacy: a table-driven test runs every
'kind'step through a real trace file with an error that quotes a prompt-like sentinel and proves the sentinel never lands; the kind list is pinned as a literal list; each was checked to fail by flipping a step to'full'.Dedupe: tests for repeat, relapse after recovery, changed failure, leave-and-return, a batch probe that always fails, and per-session clearing; each fails when its rule is removed.
Live Electron QA on macOS (remote M4Air), as above. Not run live: Windows, Linux, SSH hosts (the trace lands in the execution host's own profile; nothing here is platform-specific), and the journal-write steps (the scenario exercised the record store, not the journal).
Not run: the real Claude CLI tests (
claude-structured-real-cli*.test.ts), which write into the real~/.claude.CI on 4a2e408: PR Checks and Mobile Checks green, including static analysis, typecheck, all 5 unit-test shards, relay integration, cross-version wire compatibility and packaging (macOS, Windows).
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Reviewed in rounds by independent reviewers (privacy, behavior preservation, performance/completeness, architecture), a readiness checklist, and live QA. Fixed during review: a CI unit test still calling the event-sink factory without its now-required callback; the privacy choice made mandatory per step (it was an opt-in list); the module renamed away from
src/shared/agent-session-failure.ts(the user-facing failure vocabulary); the Claude structured-session and create-RPC console sites brought into scope and under the ratchet; two silently swallowed writes; a duplicate-report fix that turned out to leave Stop/close/quit with no record (reverted); weak privacy tests; the lease-renewal flood found in live QA; type assertions that kept CI's unit job from running. Deferred as nits: the idle-sweep double record, span naming consistency, ratchet evasions via destructuring (const { warn } = console), an optional sessionId on the Claude dispatch input, "unknown" reconciliation outcomes recorded as failures.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Security: no new network sinks; trace content passes the existing redactor, and the privacy table above keeps prompt-quoting errors out. Cross-platform: no platform-specific code. Remote SSH: structured sessions run on the execution host, which writes its own trace; nothing assumes local. Mobile: two RPC methods changed only their log line; the refusal they return is identical. Backwards compatibility: no persisted data, schema or wire change. Performance: nothing added on success paths; one record per failure, with the one timer-driven repeat deduped.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)