Repository navigation
fix(native-chat): report structured agent-session failures instead of discarding them - #20542
brennanb2025 wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change adds and re-exports a shared runtime dependency type. Runtime failures use Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to If a configured diagnostic reporter fails, the underlying structured-session error may be absent from fallback logs. Resolve or explicitly accept this diagnostic gap before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is otherwise complete and covers the change, motivation, testing, compatibility, scope, and checklist items. However, the required Linked Issue section states that no tracking issue exists.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one refactor nit inline, one scope note below.
Reviewed changes
- Structured-session failures now reach the trace sink:
ensureStructuredAgentSessionHostinorca-runtime-get-worktree-ps.tspasses anonErrorthat records a failedagent-session.errorspan with the failure scope as an attribute. - Console fallbacks for omitted reporters: every
deps.onErrorcall site instructured-agent-session-runtime.tsnow falls back toconsole.errorwhen no reporter is installed, and wraps the reporter call so a throwing reporter cannot poison later callbacks — this also closes a latent unhandled rejection on the late-settlement path. - Deps type extracted:
StructuredAgentSessionRuntimeDepsmoved tostructured-agent-session-runtime-deps.ts(re-exported, fields preserved verbatim) to keep the runtime under its line cap. - Tests: a wiring test invokes the installer's real
onErrorand reads the actualmain.trace.ndjsonthroughcreateLocalFileSink, and a runtime-errors test covers omitted/configured/throwing reporters across the exit, late-settlement, and journal paths. Both would fail if the wiring or fallbacks regressed.
ℹ️ Failures stay silent on hosts where the tracer sink is inactive
Because the production installer now always provides onError, the runtime's new console.error fallback can never fire on the production path — the if (deps.onError) branch always wins. Where observability is off (orcad, which the PR discloses does not call initObservability; also ORCA_DIAGNOSTICS_DISABLED / CI), startSpan returns its no-op span, so the failure is discarded with neither a trace nor a console line. This is not a regression, but the "no silent discard" guarantee still does not hold on the headless host, and the fallback added here only protects non-production callers (tests).
Technical details
# Headless path has no failure visibility
## Affected sites
- `src/main/runtime/orca-runtime-get-worktree-ps.ts:153-157` — `onError` always defined; writes only to the active tracer sink.
- `src/main/observability/tracer.ts:183-185` — `startSpan` returns `noopSpan` when `activeSink` is null.
- `src/main/orcad/orcad-entry.ts:205` — constructs the same `OrcaRuntimeService`, so it reaches this installer without a sink.
## Required outcome
- Confirm the disclosed follow-up (wire `initObservability` into `orcad`) is tracked.
- Optionally decide whether the production `onError` should also log as a last resort when no sink is installed, so headless failures are not fully invisible.
## Open questions for the human
- Is the `orcad` observability gap tracked by an issue, or should one be filed from this PR?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 1adba5ed-f5cf-48ef-9aa6-d0caf1416f18
📒 Files selected for processing (2)
src/main/runtime/structured-agent-session-runtime-errors.test.tssrc/main/runtime/structured-agent-session-runtime.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| } catch (reportingError) { | ||
| console.error(`[structured-agent-session] ${label} error reporting failed`, reportingError) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the original failure when onError throws.
This catch path logs only reportingError. If onError throws before it records the runtime error, the original exit, settlement, reaper, or journal failure is still discarded.
Log the original error with its session context. Then log the reporter failure. Update the throwing-reporter test accordingly.
Proposed fix
} catch (reportingError) {
+ console.error(
+ `[structured-agent-session] ${label} failed${sessionId ? `:${sessionId}` : ''}`,
+ error
+ )
console.error(`[structured-agent-session] ${label} error reporting failed`, reportingError)
}📝 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.
| } catch (reportingError) { | |
| console.error(`[structured-agent-session] ${label} error reporting failed`, reportingError) | |
| } catch (reportingError) { | |
| console.error( | |
| `[structured-agent-session] ${label} failed${sessionId ? `:${sessionId}` : ''}`, | |
| error | |
| ) | |
| console.error(`[structured-agent-session] ${label} error reporting failed`, reportingError) |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Consolidated the five error-reporting blocks:
structured-agent-session-runtime.ts:168-187now routes the orphan reaper, both exit callbacks, late settlement, and journal failures through a singlereportRuntimeError(deps, scope, label, error, sessionId?)helper, replacing the five near-identical inlineonError-or-console.errorblocks. The extracted helper preserves the exact console strings and try/catch behavior at every site, and resolves the prior review's duplication nit. - Split late-settlement coverage by provider:
structured-agent-session-runtime-errors.test.tsnow exercises codex and claude late settlement as separate cases (previously one claude-only case), alongside the exit and journal paths. The codex provider identity shape matchesAgentJournalItemIdentity. - Merged latest
origin/main: the two merge commits carry no authored delta; the deps type extraction and thestartSpan('agent-session.error', …).fail(…)wiring are unchanged from the prior review.
Ran the two changed tests locally at this head: 21 passed.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Review status for head 5fdde00: no proven in-scope defect remains. The conflict with main was resolved without rewriting history; the shared late-settlement callback still covers both Codex and Claude, and the PR remains a focused host-side error-reporting change. GitHub reports CLEAN/MERGEABLE, and all required checks are green after a single surface-lifetime test timed out on the first CI attempt and passed on rerun. Functional correctness: the production installer now supplies the previously missing error reporter, and exit-recovery, late-settlement, and journal failures reach the bounded, redacted local trace sink; a failing reporter cannot poison the callback. Architecture: error delivery stays with the execution host, with no new session-state copy, retry latch, or steady-state polling. The optional reporter and desktop process-global sink do not cover the headless daemon, which is an explicit separate host-sink/lifecycle gap. Validation: independent review and start/end readiness passes found zero proven P0/P1/P2 findings; 50 focused tests, typecheck, and targeted lint passed locally. Full CI, including Windows packaging and the retried test shard, is green. Hidden exact-head Electron QA verified app identity, the rendered Privacy diagnostics surface, and an active private NDJSON sink; its uncropped final-head screenshot is in Visual Proof. No real structured-session failure was induced in that app run, so the screenshot is related-surface evidence rather than proof of an agent-session.error span. The existing real-file wiring test supplies that callback-to-trace evidence. This PR improves diagnosis; it does not establish or fix the cause of the originally reported readable-but-unwritable chat. Live SSH/mobile and daemon error-path validation were not performed for this no-wire/no-mobile delta. I would treat the desktop observability scope as reviewed, with the headless tracing gap tracked separately rather than claiming universal failure coverage. |
…ctured-onerror-wiring # Conflicts: # src/main/runtime/structured-agent-session-runtime.ts
|
Merge conflict with the current Architecture status: the failure-reporting path is owned by the execution host and routes runtime faults into the host diagnostic sink without adding reader-side state, retry latches, polling, or steady-state work. It is not full lifecycle alignment: the reporter remains optional and backed by a desktop process-global sink, I kept the conflict resolution focused on observability rather than expanding this PR into daemon logging or settlement-lifecycle ownership. The only runtime cost is a diagnostic write on an error path. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since pullfrog's prior review (5fdde00 → b1bb2d0): a merge of origin/main plus two authored adaptations.
- Late-settlement parameter retyped:
onDispatchSettledLateinstructured-agent-session-runtime.ts:214-216now derives its parameter fromParameters<StructuredAgentSessionHost['settleLateDispatch']>[0], dropping the duplicated inline shape and the now-unusedAgentJournalItemIdentityimport. The body only readssessionId, so behavior is unchanged. - Wiring test installs an app environment:
orca-runtime-structured-status-sink-wiring.test.ts:38-48installs a testAppEnvironmentinbeforeAll, needed because the merged runtime resolves its state directory throughgetAppEnvironment().getPath('userData'); without it the real installer would throw on a missing environment.
Ran both changed test files at this head: 21 tests pass. The prior review's duplication thread is resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since pullfrog's prior review (9320989 → 05e315d2): a merge of origin/main that had to reconcile main's new Agent Permissions plumbing with this PR's extracted deps type.
- Permission deps preserved in the extracted type:
structured-agent-session-runtime-deps.ts:1,34-36gainsresolveClaudePermissionMode?: () => Promise<PermissionMode> | PermissionModeandresolveCodexPermissionArgs?: () => string[]plus thePermissionModeimport — main's two new fields, carried into the file this PR created rather than dropped during the conflict. - Runtime forwards them conditionally:
structured-agent-session-runtime.ts:232-234,272-274passes each to the codex/claude adapters only when defined, matching the surrounding optional-field style. - Installer keeps both concerns:
orca-runtime-get-worktree-ps.ts:148-152still installs theagent-session.errortraceonError, while:165-168now re-reads the Agent Permissions setting per acquisition after main removed the configured-CLI-args path.
Verified locally at this head: pnpm run tc:node clean, and the two changed test files pass (21 tests). The prior review's duplication thread is resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Superseded by #24079. This PR wired the one missing |
1 participant
ELI5
When something goes wrong inside a native chat session — cleaning up after an agent process dies, for example — the code reports the error by calling an error handler that the app is supposed to provide.
Nobody ever provided one. The call was written as "call the handler if there is one," and there never was one, so every single error reported this way was thrown into nothing and disappeared.
This PR provides the handler, and points it at the diagnostic log file Orca already writes.
What Changed
main.trace.ndjson).Why
A user reported a chat that became permanently unusable after an update. When we went looking for the cause, there was nothing to find — zero trace across nine log rotations in two separate profiles. The failure that caused it reported itself correctly, into a handler that didn't exist.
That meant we couldn't diagnose that incident, and wouldn't have been able to diagnose the next one either.
One detail worth knowing: the file that sets up these sessions has type-checking disabled, so the compiler couldn't see the missing handler. Making the handler required wouldn't have helped for the same reason — a missing optional field is legal, and type-checking is off there regardless. A runtime assertion would have caught it, but at the cost of updating roughly eleven test setups. The fallback plus real wiring removes the silent-discard problem more cheaply.
Linked Issue
No tracking issue — found while investigating a reported P0.
Visual Proof
N/A — this changes nothing a user sees. It changes whether we can see anything after a failure.
Testing
The test asserts the real artifact rather than a mock: it triggers the error handler, then reads the actual
main.trace.ndjsonfile after its normal flush and checks the scope, the message, and the stack trace are all there.pnpm tc,check:code-quality:changed(0 new findings across 5 files),pnpm format— all cleanKnown gap, disclosed: the desktop app sets up this log file at startup; the background daemon (
orcad) does not. So failures on the daemon side still won't be traced until that's wired up too. Out of scope here, worth a follow-up.Review
Agent skill upstream boundary
Notes
Cross-platform: no platform-specific code. SSH/remote: this is local error reporting and publishes nothing over the wire. Mobile: unaffected. Backwards compatible: the handler is still an optional field, so no existing caller breaks. Performance: one log entry per error, on a path that previously did nothing at all.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)