Repository navigation
refactor(native-chat): the rest of structured chat reports failures through the host's logger - #24504
Open
brennanb2025 wants to merge 5 commits into
Open
brennanb2025 wants to merge 5 commits into
brennanb2025 wants to merge 5 commits into
Conversation
…hrough the host's logger The structured chat host's required logger (#24312) did not reach the chat journal, the Claude and Codex adapters, or the runtime and orchestration callers outside the host, so their failures still went to console.warn/console.error, which a packaged desktop build never shows. These now report through the same logger: - The journal database takes the logger at open and every chat's journal reports through it: a failed rollback, an import that reads back different, a retired import file left behind, a skipped queued-message repair, skipped row bookkeeping, and an unconfirmed submission. - The Claude and Codex adapters take it as a required dependency: exit cursor and turn-end resume point writes, unreadable attachments, failed message writes, an unprovable history window, an option restore that was never answered, and a child CLI launcher that did not resolve. - The runtime holds the logger it hands the host, so a create refused before the host exists, chat-tab retirement, mail redrive and worker discard report to it too. A failed Stop child end reports from the turn context instead of through an optional callback. - Restart-offer bookkeeping failures name the affected session ids as well as the operation id, still with no error object, since those errors can quote the recovery payload. A ratchet test fails on console.warn/console.error or an optional on*Error/on*Failed/on*Failure callback in structured chat production code; the behavioral optional hooks are listed by file.
…ed-chat-logger-followups # Conflicts: # src/main/runtime/structured-agent-session-held-signal.test.ts
…through the logger, and widen the ratchet to match - Claude: a descendant that survives a close and a failed at-rest `/` command scan now go to the chat's logger with its session id. The stream-json connection and the at-rest catalog take the logger as a required dependency; only structured Claude code opens or builds either. - Runtime: a failed structured host install before a worker stop or release, and a forced worktree removal that leaves structured sessions unclosed, report through the runtime's logger. The removal keeps its console line when no runtime is passed. The runtime wraps an injected logger so a throwing one cannot fail the step it reports. - The structured mail redrive's catch moves into structured-session-mail-target.ts, so the ratchet covers it without scanning the PTY record file's own mailbox warnings. - The WSL transcript gate's deadline warning also writes a failed span to the trace file through the existing tracer; the console line stays because scanner workers install no trace sink. - Ratchet scope: drop the trailing dash that skipped rpc/methods/structured-agent-session.ts, cover the runtime's structured-* files, the structured worker stop/release files and the two Claude files, and fail if a listed file stops being scanned. - The held-signal test is back to main's version (#24494 fixed it the same way). - The option-restore test asserts the logger entry instead of a console spy that no longer fired.
…belonged to The journal-import-retire entry carried only the legacy directory; its one caller holds the chat's session id, so the entry now carries it like the other per-chat journal sites.
…he dispatch, not the worker handle A structured worker's handle is a bearer credential, and the entry now reaches the trace file and the diagnostic bundle. Log the dispatch id instead, as the release path already does.
Contributor
Author
Review status: ready for review (head ef3f035)The problem. In structured native chat, some background failures were reported only with What changes for you. Nothing in the chat UI. These failures now also go to Fixed during review (each with tests):
Deferred (P3, not fixing here):
Scope note. The Claude/Codex adapter sites go slightly beyond the list #24312 named. They are the same failure class, and its session-environment item already needs the logger in both adapters. I kept them; the PR can't be split usefully because the ratchet test must land with every conversion. Verified:
Not verified:
|
brennanb2025
marked this pull request as ready for review
October 2, 2026 04:33
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (128)
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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+30
to
+37
| const SCOPE_FILES = new Set([ | ||
| // Opened and built only by structured Claude chats. | ||
| 'main/claude/claude-stream-json-connection.ts', | ||
| 'main/claude/claude-at-rest-commands.ts', | ||
| // Install the structured host before a structured worker's stop or release. | ||
| 'main/runtime/rpc/methods/orchestration/worker/worker-stop.ts', | ||
| 'main/runtime/rpc/methods/orchestration/worker/worker-release-completion.ts' | ||
| ]) |
There was a problem hiding this comment.
worktree-teardown.ts, where this PR adds structured-chat logging, and the orca-runtime-* files that wire the logger and mail redrive. A future console-only failure in those paths would pass this test and could go unrecorded in a packaged app. Include those paths, with narrow allowances for unrelated console warnings.
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 critical issues — one scope gap in the new ratchet guard, noted below.
Reviewed changes
- Required logger replaces the last console failures in structured chat. The runtime now mints one
StructuredAgentSessionLoggerand threads it through the host install, the Claude/Codex adapters, the journal database, launch resolution, and the child-identity env, so rollback, import verify/retire, queued-message repair, row bookkeeping, restart reconciliation, exit cursor, resume point, attachment/dispatch writes, history window, option-restore timeout, close-descendant, at-rest scans, and the child CLI launcher report with the chat'ssessionIdinstead of vanishing into a console a packaged app does not have. - One optional reporting callback removed.
onStopChildError?is gone;performCancelreportsstop-childthroughctx.loggerdirectly. - Restart-offer failures now name the chats. The three ledger warn sites add
sessionIds(completed / failed / all-settled) while still omitting any error object and prompt. - Worker host-install failures log the dispatch, not the handle.
worker-stop/worker-releasenow reportdispatchId, with a test pinning that the worker's bearer handle never reaches the record. - Publication minting moved into
createClaudeSessionPublication. Supporting change to stay under the file-length limit; verified behavior-preserving (exactly-onceacquisitionGeneration/linkId/observedAt, same order,continuesChainuntouched). - WSL transcript timeout now emits a trace span in addition to its console line, through the existing tracer rather than the chat logger since the gate is shared with non-chat scanners.
- New ratchet test fails on
console.warn/console.error(three spellings) and reporting-onlyon*Error?/on*Failed?/on*Failure?callbacks across the structured-chat scope, with the logger sink and four behavioral hooks allowlisted.
ℹ️ The ratchet's scope list skips two structured-chat files that its own regex would flag
structured-background-logger-ratchet.test.ts claims no structured-chat production code reports a failure only to the console or through a reporting-only optional on*Failure? callback. But SCOPE_PREFIXES/SCOPE_FILES never feed src/main/claude/claude-background-task-rows.ts or src/main/claude/claude-background-task-row-writer.ts to the scanner, and both are structured Claude-session production code (used by claude-structured-journal-translation.ts) that declare an optional onPersistenceFailure? callback. The guard's own OPTIONAL_FAILURE_CALLBACK regex matches onPersistenceFailure, so widening the scope without allowlisting it would turn the guard red — the current green state depends on the omission. No failure is dropped today (the callback is always wired by createClaudeJournalFailureHandler), so this is a guard-completeness gap rather than a runtime bug, but it is the exact class the ratchet exists to make hard to reintroduce.
Technical details
# Ratchet scope gap: background-task row persistence failures
## Affected sites
- `src/main/native-chat/agent-session-wire/structured-chat-logger-ratchet.test.ts` — `SCOPE_PREFIXES` has no `main/claude/claude-background-task-` entry and `SCOPE_FILES` does not list either file, so `inScope` returns false for both.
- `src/main/claude/claude-background-task-rows.ts:56` — `onPersistenceFailure?: (error: Error) => void` on `ClaudeBackgroundTaskRowsDeps`.
- `src/main/claude/claude-background-task-row-writer.ts:24,48,55,75` — constructor param and the three `?.()` call sites.
- `src/main/claude/claude-structured-journal-translation.ts:15,111,120` — imports `ClaudeBackgroundTaskRows` and forwards the callback, which is how these files are reached from structured chat.
## Required outcome
The guard's stated guarantee should either hold for this code or be explicitly out of scope. If these files are in scope, add them (to `SCOPE_FILES` or a `main/claude/claude-background-task-` prefix) and add `onPersistenceFailure` to `BEHAVIORAL_OPTIONAL_HOOKS` for the two paths, since the callback changes behavior (ends the session) rather than merely reporting. If they are deliberately out of scope, say so in the comment so the next reader does not read the guard as covering all structured Claude journaling.
## Suggested approach (optional)
Check first whether every `onPersistenceFailure` call site has a logger-equivalent already; if a future omission would silently drop the failure, prefer widening the scope over documenting the gap.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
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
Some failures in structured native chat (the Codex/Claude chat UI) still leave no record anyone can find. When a background step fails, for example saving Claude's resume point when the agent exits, rolling back a chat-journal transaction, importing or retiring a legacy journal, repairing queued messages, or retiring a worker's chat tab, the error goes only to
console.warn/console.error. A packaged Electron main process has no console, so in a shipped app those errors vanish. That is what kept the Sep 10 "native Codex chat cannot resume after update" P0 invisible.#24312 fixed this for the chat host and its collaborators by giving them a required
loggerthat writes to the local diagnostics trace (<userData>/logs/main.trace.ndjson, which the share-diagnostics bundle collects). Its own description lists the rest as a Temporary follow-up: "give the journal database the host's logger, and pass it to the orchestration and session-environment callers", with the WSL transcript gate listed alongside. This PR is that follow-up. It also supersedes #24079, an earlier attempt at the same fix with a different mechanism; #24079 was closed in favour of #24312's design.Scope: structured native chat only (behind
experimentalStructuredNativeChat, off by default), in the main process and the headlessorcadhost, plus the WSL transcript gate #24312 listed. Terminal agents and the renderer are not touched.What is different for the user: nothing visible in the chat. These failures now also land in the diagnostics trace, with the chat's session id, instead of disappearing.
What changes
JournalHostDatabase.open/openWith,openJournalDatabaseandrunJournalTransactiontake it. Rollback, legacy import verify/retire, queued-message repair, row bookkeeping and restart reconciliation's "unknown" outcome report through it, with the chat's session id, instead of to the console.onStopChildError?(added in fix(native-chat): a Codex Stop that Codex refuses or never answers ends the Codex process #24334) went the way refactor(native-chat): structured chat failures always reach the diagnostics log #24312 removed the others: cancel reportsstop-childthrough the logger.sessionIds: the completed, the failed, or every settled session respectively. They are still one record per failure, and still have no error object and no prompt, since refactor(native-chat): structured chat failures always reach the diagnostics log #24312's privacy rule for recovery payloads still holds.wslTranscriptFs.timeout, with the task's priority, timeout and key). The gate is shared with the session-history scanners, which are not chat, so it writes through the existing tracer directly rather than the chat logger.console.warn/console.error(any spelling) or an optionalon*Error?/on*Failed?/on*Failure?callback comes back in structured-chat production code. It coversnative-chat/agent-session-{wire,journal}andnative-chat/structured-agent-session-*,claude/claude-structured-*,codex/codex-structured-*, the runtime'sstructured-*and orchestration-structured files, their RPC methods, and four files whose names don't say so: the Claude stream connection and slash-command catalog (used only by structured chats), and worker stop and worker release (which install the structured host first). It exempts the logger's own sink and the four callbacks that carry behavior rather than reporting:onAcquisitionFailed,onBackgroundTaskJournalFailure, the event sink'sonFailedandonEventSinkFailure.Why this design
A failure's destination should not be optional. With an optional callback, forgetting to wire it is silent, and the console fallback does not exist in a shipped app. #24312 established the required-logger mechanism for the host; this PR finishes moving the remaining structured-chat code onto it, rather than adding a second mechanism. The ratchet makes the class hard to reintroduce: a new console-only failure, or a new optional error callback, fails a test rather than shipping silently.
Where this differs from the common pattern, beyond what #24312 already records:
Testing
CI on
ef3f0358b28is green: typecheck, static analysis, all five unit-test shards, cross-version wire compatibility, and mac and Windows packaging.Locally: the node typecheck through the machine-wide gate after each fix, and explicit vitest runs over every touched test, its importers,
orca-runtime.test.tsand thetests/e2eunit files that build these dependencies (latest broad run: 137 files, 2551 passed, 2 skipped). The real-CLI Claude tests were excluded; they write into the real~/.claude.The ratchet fails against main's
journal-row-writer.tsandturns-cancel.ts, and against aconsole.warnplanted in each of the 19 files the widened scope added. New tests assert the session id or dispatch id on the import-retire, option-restore, stream-close, slash-command-scan, worker-stop, forced-removal and restart-offer entries; the import-retire and worker-stop assertions were confirmed to fail with the field removed.Live QA on Windows (a separate machine, isolated profiles, dev builds of this PR and of
origin/main9e34bd0): a seeded Claude chat with a leftover old-format journal file that cannot be deleted (a deny-delete permission on its folder). On restart, this PR wrote one failedagentSession.journal-import-retireentry tomain.trace.ndjsonwithsessionId, the folder andEPERM;origin/mainwrote nothing to the trace, only a console line. The restored chat looks the same on both builds:ef3f0358b28)origin/mainNot verified live: failures that need a running, logged-in agent (exit cursor, resume point, dispatch write, Stop). Unit tests cover them. A remote
orcadhost's trace was checked by unit tests only.I manually tested these changes locally
Automated tests added/updated, or explained why not below
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.