Repository navigation
fix(native-chat): give a failed Claude background task a typed row instead of an opcode - #20519
Conversation
…stead of an opcode A failed backgrounded command printed red rows whose visible text was the wire opcode, and printed one failure twice. All five task lifecycle kinds are catalogued status-chrome, but the payload sniffer in classifyProviderFrame runs first and promotes any frame reporting a failure to the generic unknown-frame fallback, whose sentence lookup has no key for the field Claude puts its own sentence in. Two frames for one task therefore produced two rows, both of them the method name. Suppressing those frames is not the fix: when the last background task settles the tracker flushes it and the strip unmounts, local_bash is excluded from the subagent roster, and the status feed publishes only live tasks, so for a lone backgrounded command the transcript row is the only report of the failure that exists anywhere. So the catalogue now binds: kinds a dedicated typed translator owns are named as covered, and the generic fallback refuses to emit for them in either direction. A new row owner keeps one durable row per task id, opened by the announcement, revised in place by the lifecycle frames and closed by the notification, carrying the provider's summary, error, output path, usage and a run state. The row is written on the same dual carrier the subagent roster uses: a frozen text twin plus a typed block, so a client without the block type reads the sentence rather than nothing. hasProviderError keeps its authority everywhere else, unchanged.
|
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:
📝 WalkthroughWalkthroughThe change adds validated background-task blocks, shared state and fallback-text rules, and twin-text handling. Claude frames now create durable task rows with lifecycle updates, settlement, deduplication, generation tracking, and typed-fallback suppression. Transcript payloads bound task fields and remove unsupported output files. Native-chat derivation and rendering display task rows with summaries, errors, status, tokens, duration, and output files. Journal liveness handling settles persisted live tasks. Tests cover translation, state rules, transcript formatting, attribution folding, fallback behavior, and rendering. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Some Claude task lifecycle sequences can hide a restarted task or display incorrect or duplicate task information. Resolve the durable-row restart defect before merging; the remaining bounded presentation issues should also be tracked. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed failure context, implementation scope, visual proof, and validation results. It does not include the required Linked Issue, template section headings, checklist confirmations, or explicit cross-platform and compatibility notes. Resolution Add the required Linked Issue in the form
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 883c30b4-dc18-4388-8011-697b01db5df8
📒 Files selected for processing (21)
src/main/claude/claude-background-task-frames.tssrc/main/claude/claude-background-task-rows.test.tssrc/main/claude/claude-background-task-rows.tssrc/main/claude/claude-structured-journal-translation-background-tasks.test.tssrc/main/claude/claude-structured-journal-translation.tssrc/main/native-chat/agent-session-wire/provider-frame-disposition.test.tssrc/main/native-chat/agent-session-wire/provider-frame-disposition.tssrc/main/native-chat/agent-session-wire/unhandled-provider-frame.tssrc/main/runtime/orchestration/worker-transcript-payload.tssrc/renderer/src/components/native-chat/NativeChatBackgroundTaskRun.test.tsxsrc/renderer/src/components/native-chat/NativeChatBackgroundTaskRun.tsxsrc/renderer/src/components/native-chat/NativeChatBackgroundTasksStatus.tsxsrc/renderer/src/components/native-chat/NativeChatMessageRow.tsxsrc/renderer/src/components/native-chat/NativeChatToolRun.tsxsrc/renderer/src/components/native-chat/native-chat-row-content.tssrc/shared/agent-session-journal-schemas.tssrc/shared/native-chat-background-task-row.test.tssrc/shared/native-chat-background-task-row.tssrc/shared/native-chat-types.tssrc/shared/worker-transcript-background-task-text.test.tssrc/shared/worker-transcript-text.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| state, | ||
| summary: taskText(message.summary), | ||
| error: taskText(message.error), | ||
| outputFile: taskText(message.output_file), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve whitespace in output_file.
taskText() collapses every whitespace sequence. A valid path such as /tmp/task output.txt becomes /tmp/task output.txt, so the displayed output path no longer identifies the provider file.
Use a length-bounded path parser that does not normalize internal whitespace.
| label: clipMetadata(block.label, state), | ||
| ...(block.summary ? { summary: clipText(block.summary, state) } : {}), | ||
| ...(block.error ? { error: clipText(block.error, state) } : {}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Regenerate the background-task twin after bounding the block.
When summary and error are absent, backgroundTaskFallbackText includes label. boundBlock clips that label to 512 characters, but it clips the adjacent text twin to the 1,200-character text limit. An oversized label can therefore produce different sentences. claimBackgroundTaskTwins fails to pair them, so both the bounded row and its twin render.
Regenerate the adjacent text twin from the bounded background-task block. Add a long-label regression test. This is a narrow duplicate-rendering issue; the task row remains visible.
| for (const [index, block] of blocks.entries()) { | ||
| const count = block.type === 'text' ? (wanted.get(block.text) ?? 0) : 0 | ||
| if (block.type === 'text' && count > 0) { | ||
| wanted.set(block.text, count - 1) | ||
| twinTextIndexes.add(index) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pair each background-task row with its adjacent twin.
claudeBackgroundTaskBody writes the fallback text immediately before its task row. claimBackgroundTaskTwins can claim an earlier matching text block instead. With [real text S, twin text S, row S], the transcript formatter prints both text blocks, and the native-chat renderer retains the adjacent twin beside the row. This produces localized duplicate text.
Match each row only with its exact adjacent producer twin. Add a regression case with [real text S, twin text S, row S].
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Three concerns worth resolving before merge: one likely spurious-row bug (inline), plus a crash-recovery gap and a tool-fold asymmetry.
Reviewed changes
- Coverage contract —
provider-frame-disposition.tsaddsCLAUDE_TYPED_TRANSLATOR_KINDSandhasTypedProviderFrameTranslator;unhandled-provider-frame.tschecks it before classification so a covered kind never degrades to its opcode.hasProviderErroris unchanged. - Typed durable row — new
claude-background-task-rows.tskeys a row bytask_id(tool id as alias), opens ontask_started, revises ontask_updated/task_progress/background_tasks_changed, closes ontask_notification, latches outcomes, and opens a row for a failure whose announcement was missed. Wired intoclaude-structured-journal-translation.tsand settled ondispose. - Carrier and rendering — new
NativeChatBackgroundTaskBlock(frozen text twin + typed block) with journal-schema arm,NativeChatBackgroundTaskRun, row-content extraction, worker transcript text/payload handling, and shared state/kind normalization.
⚠️ A host that dies leaves a background-task row claiming working forever
dispose() settles live rows, but a crashed host never calls it. The next journal open reconciles stale working subagent rosters and nothing else, so a background-task row left mid-run keeps its last revision indefinitely — a transcript that lies about a task that is long gone.
Technical details
# Stale background-task row is never reconciled on reopen
## Affected sites
- `src/main/native-chat/agent-session-journal/journal-subagent-liveness.ts:42` — `staleSubagentRosterRevisions` only walks `subagent-group` blocks (`hasWorkingChild`).
- `src/main/native-chat/agent-session-journal/journal-store-open.ts:142` — the open path calls only `staleSubagentRosterRevisions`.
- `src/main/claude/claude-background-task-rows.ts:137` — `settleSession` exists, but is only reachable from `dispose()` (in-process graceful teardown).
## Required outcome
- A `background-task` block left in an in-flight state (`working`/`monitoring`/`waiting`) by a host that is no longer writing must be revised to `unverifiable` when the journal is reopened, exactly as a stale roster is — including the text twin it sits beside, so the two do not disagree.
- `hasWorkingChild`/`settleBlocks` should learn the background-task case (there is one block per row, so pairing the twin is straightforward), or the reconcile should be generalized to both row kinds.
## Open question for the human
Is this intentionally deferred to a follow-up, given it only reproduces after a host crash/force-quit rather than a normal close?⚠️ A background-task row can split the tool run it lands in
foldToolMessages exempts a subagent roster from ending the assistant tool run because the row is activity chrome. A background-task row is the same shape (role system, text twin + typed block) but is not exempted, so one arriving mid-turn resets the fold and can leave the turn's tool activity split across two runs.
Technical details
# background-task rows are not treated as activity chrome during folding
## Affected sites
- `src/shared/native-chat-tool-fold.ts:42` — `isSubagentRosterMessage` returns `message.blocks.some(isSubagentGroupBlock)`; a background-task message is not matched, so line 117 clears `mutableAssistantIndex`.
- `src/main/claude/claude-background-task-rows.ts:76` — the row is written as `role: 'system'` with `[{text twin}, {...block}]`, structurally identical to `claude-subagent-group-row.ts`.
## Required outcome
- A message carrying a `background-task` block should not end the assistant run any more than a roster message does, so the tool activity that follows folds into the same run. Generalizing the predicate (e.g. an "activity chrome" check covering both block types) is preferable to a second special case.
- Worth a fold-level test mirroring `native-chat-tool-fold.test.ts` "does not end the assistant run the following tool messages fold into" but with a `background-task` row.
## Open question for the human
If the two row kinds are meant to differ here, what should a background-task row do to the run boundary?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
A background task's row is a system message landing mid-turn between the assistant's tool calls, exactly where the spawn-group roster row lands. Without the same exemption it ended the run the following tool messages fold into, so a tool result arriving after one stopped folding into its own assistant turn. Also syncs the catalog with the row's one new translate key.
Update — merged
|
| Lever broken | Result |
|---|---|
Empty CLAUDE_TYPED_TRANSLATOR_KINDS |
3 fail — the reported opcode rows return |
summary in MESSAGE_KEYS instead |
2 fail — one row's text fixed, duplicate + opcode remain |
Catalogue outranks hasProviderError |
10 fail — incl. all four pinned promotions |
| Drop the fold exemption | 1 fail — tool result stops folding |
Note on the catalog sync: no vitest test catches a missing en.json key —
English resolves from the translate() call-site default, so the key's absence
changes no rendering. It is added for catalog completeness, matching how every
sibling key in that block is maintained (all of them live in en.json only; no
other locale carries any).
Gates re-run on the merged tree: pnpm tc clean, oxlint clean across src/,
check:code-quality:changed 0 findings across 23 files, and 10,283 tests pass
across the affected surface.
Electron QA (Grok) — native-chat typed background-task rowLaunched this merged branch with Identity (
|
There was a problem hiding this comment.
✅ No new issues found in the new commits.
Reviewed changes
- Exempted
background-taskrows from ending the assistant tool run —foldToolMessagesnow treats a message carrying abackground-taskblock the same as a subagent roster message (activity chrome), so a tool result arriving after one still folds into its own assistant turn;native-chat-tool-attribution-allocation.test.tsadds a regression test that folds a result across the row. - Synced the
backgroundTasks.outputFilecatalog entry —en.jsongains the keyNativeChatBackgroundTaskRunalready calls, so a row with an output path renders it instead of the fallback literal.
The prior review's tool-fold asymmetry is resolved. The other two concerns it raised (the observeStart foreground-task question and the crash-recovery reconciliation gap) remain open and are unchanged by these commits, so the existing thread stands.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Review status: ready to land.
|
The malformed-task-frame test asserted the generic fallback resolves Claude's `summary` field itself, which was true only while that key sat in the shared key list. Eligibility is what this layer decides; the sentence the row leads with is Claude's, supplied through the display-text seam and proven in the translation test.
|
Final follow-up: pushed |
|
P2 accepted and fixed in 32a79b1 — Two corrections to b8293eb, both re-introduced traps the plan named explicitly:
Keeping the opt-in coverage flag: gating suppression on whether the row owner actually claimed the frame is better than my per-kind version, because it closes the edge I had documented and accepted (a task frame naming no task wrote nothing). Noting one latent gap: the All six ablations re-proved at head 32c0b15: empty coverage contract (2 fail), summary-in-MESSAGE_KEYS instead of the typed row (2 fail), catalogue outranking hasProviderError (11 fail), tool-fold exemption dropped (1), settleSession on ended dropped (1), coverage made unconditional (1). Gates green: pnpm tc, oxlint across src/, changed-code quality 0 findings across 33 files, 1169 tests pass on the affected surface. claude-structured-real-cli.test.ts fails identically on the base commit — pre-existing, re-baselined. |
|
the model provider had a temporary upstream failure. The provider refused or dropped the request mid-run — nothing in your repo or your credentials is at fault. Re-trigger Pullfrog; if it keeps happening, check the provider's status page.
|
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: b17111ed-56bb-4efb-9a3e-52466f6d1c64
📒 Files selected for processing (18)
src/main/claude/claude-background-task-frames.tssrc/main/claude/claude-background-task-row-journal.tssrc/main/claude/claude-background-task-row-lifecycle.tssrc/main/claude/claude-background-task-rows.test.tssrc/main/claude/claude-background-task-rows.tssrc/main/claude/claude-structured-journal-translation-background-tasks.test.tssrc/main/claude/claude-structured-journal-translation.tssrc/main/claude/claude-structured-prompt-journal.tssrc/main/claude/claude-structured-provider-fallback.tssrc/main/native-chat/agent-session-journal/journal-subagent-liveness.test.tssrc/main/native-chat/agent-session-journal/journal-subagent-liveness.tssrc/main/native-chat/agent-session-wire/provider-frame-disposition.test.tssrc/main/native-chat/agent-session-wire/structured-rewind-journal-body.test.tssrc/main/native-chat/agent-session-wire/structured-rewind-journal-body.tssrc/main/native-chat/agent-session-wire/unhandled-provider-frame.tssrc/main/runtime/orchestration/worker-transcript-activity-block-bounds.tssrc/main/runtime/orchestration/worker-transcript-payload.test.tssrc/main/runtime/orchestration/worker-transcript-payload.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
| message: Record<string, unknown> | ||
| ): ClaudeBackgroundTaskChange { | ||
| return { | ||
| state: terminalClaudeTaskRunState(message.status) ?? 'done', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not classify an explicit non-terminal notification status as done.
terminalClaudeTaskRunState returns null for any status outside completed, failed, killed, and stopped. claudeBackgroundTaskNotificationChange converts that null to 'done'. For an untracked task_notification without error, observeNotification then returns true. The typed observer claims the frame, so the generic fallback does not display its summary.
Preserve the null state and make observeNotification return false when change.state === null, before rememberTerminalId and row handling. Do not apply ?? 'done' to an explicit non-terminal status.
| !handleMessage(event.message, event.startsTurn === true, event.observedAt ?? Date.now()) | ||
| ) { | ||
| providerFallback.append(kind, event.message) | ||
| providerFallback.append(kind, event.message, taskFrameSentence(event.message), { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit task display text to task lifecycle frames.
taskFrameSentence() does not validate the frame subtype. An unmodelled Claude system failure with both summary and error can therefore replace the generic fallback error sentence with its summary. Pass this override only for known background-task frame kinds, so other frames retain normal readableProviderFrameText() selection.
| (block.type === 'background-task' && | ||
| 'taskId' in block && | ||
| 'kind' in block && | ||
| 'label' in block && | ||
| 'state' in block) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C5 '\bisAdmissibleAgentJournalItemBody\b' src/shared src/main
rg -n -C5 "'background-task'|\"background-task\"" src/shared/agent-session-journal-schemas.ts src/shared/native-chat-types.tsRepository: stablyai/orca
Length of output: 27291
🏁 Script executed:
sed -n '1,85p' src/main/native-chat/agent-session-wire/structured-rewind-journal-body.tsRepository: stablyai/orca
Length of output: 2692
Validate the background-task block before retaining it.
The guard checks property presence only. A block with taskId: 1 reaches the final validation unchanged. isAdmissibleAgentJournalItemBody rejects it because the schema requires a non-empty string, and recovery then throws agent_session_rewind:invalid-retained-body instead of using the block's evidence-text fallback. Require the complete background-task shape before preserving the block.
|
CI diagnosis: the code and test jobs passed; only The static-analysis failure is three |
Admission now matches the reference on all three gates. Type is the whole gate and MONITORS ARE NOT ADMITTED: a monitor runs for the life of the session and has no outcome a row could report, so it never reaches the timeline. On first admission only, the task's tool_use_id must name a tool call this session forwarded at the TOP level — a Task spawned inside a subagent's sidechain names an id that never reached the transcript, and a top-level row for it would claim an invocation the user never saw. And a task that already exists and has not finished is not re-opened: a duplicate announcement is a redelivery, not a second run. Rows are now keyed per RUN. A provider may reuse a task id for a distinct later invocation, and a row keyed by the id alone overwrote the first run's transcript history instead of leaving it standing. Generation 1 keeps the bare key, so every row already written is unaffected. The spawning tool call is carried on the row as parentToolUseId. Orca's journal has no structural parent link for an item — AgentJournalItemIdentity has four arms and none carries one — so the relationship is data on the item rather than nesting. A terminal frame that names NO tool still opens a row. That is a named deviation, recorded at its call site, and the measurement behind it is in the PR.
Alignment report — four deviationsHead 1. ADMISSION — closedThree gates now hold, in
Both required regression tests exist at both levels — unit ( Ablations: re-admit monitors → 2 fail. Drop the forwarded gate → 2 fail. Allow re-opening a live task → 1 fail. One note: my first version of the re-open test passed both before and after. It only discriminates once the redelivery carries metadata the first announcement lacked (a label the row did not have), because otherwise the row serializes identically and the write dedups. Rewritten; it now fails under ablation. 2. RESTART IDENTITY — closed
Ablation: collapse back to one key → the reused-id test fails. 3. PARENT LINKAGE — closed as data, NOT as nestingBlocking file:line: Smallest faithful equivalent, implemented: Named residual deviation: the row still renders as a standalone system message rather than nested inside the spawning tool call's run. Closing that is a renderer change ( Ablation: drop the field → 2 fail. 4. UNKNOWN TERMINAL FRAMES — MEASURED, deviation-with-causeThe capture cannot answer the question, and that is a finding, not a gap in effort. PLAN.md's ground truth is pulled from the user's What the capture DOES prove is the shape. The third real frame is: It carries no SDK evidence, read and quoted verbatim from
So the resume path argues against orphan notifications, but does not exclude them, and it is not the only candidate path — our own admission gates now create one (a refused Decision taken: keep the permissive path, narrowly gated and named. A terminal frame naming a tool we never forwarded is now REFUSED (that is what makes the nested-child regression test pass). Only a terminal frame naming NO tool still synthesizes a row. The deviation is recorded at its call site in Ablation: drop the synthesis → 2 fail, including the real-capture test. Over to you for Brennan: aligning here means a real, captured failure reports nothing anywhere. I have not made that call. Constraints
One thing I did NOT touch, flagged not fixed
|
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: 018902f9-a7aa-4917-a60b-7f775a03286d
📒 Files selected for processing (9)
src/main/claude/claude-background-task-row-journal.tssrc/main/claude/claude-background-task-row-lifecycle.tssrc/main/claude/claude-background-task-rows.test.tssrc/main/claude/claude-background-task-rows.tssrc/main/claude/claude-forwarded-tool-registry.tssrc/main/claude/claude-structured-journal-translation-background-tasks.test.tssrc/main/claude/claude-structured-journal-translation.tssrc/shared/agent-session-journal-schemas.tssrc/shared/native-chat-types.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| return true | ||
| } | ||
| if (shouldRestartClaudeBackgroundTaskRow(existing, message)) { | ||
| this.openRow(id, message, existing.generation + 1) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Allow terminal-only rows to restart under a new tool ID.
When a failed task_notification creates a terminal-only row, newClaudeBackgroundTaskTerminalRow omits startedAt. A later backgrounded task_started with a different forwarded tool_use_id therefore fails shouldRestartClaudeBackgroundTaskRow. observeStart calls revise and writes the same journal identity instead of opening a new generation. The prior failure is not overwritten for a start frame without a status, but the new run remains hidden.
The reachable frame handling accepts both task subtypes, and no ordering rule prohibits this sequence. Treat a different tool ID on any settled row as a new run. Add a test for terminal-only failure followed by a forwarded start with the same task ID.
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes — the five commits since the last Pullfrog review hardened the row owner, added crash recovery, and gated admission.
- Gated admission to forwarded top-level tasks — a
ClaudeForwardedToolRegistryrecords each top-level tool use, and the row owner refuses a task whose spawning tool only exists in a subagent sidechain. - Restricted materialized kinds and excluded foreground tasks —
isClaudeBackgroundTranscriptTaskadmits onlycommand/workflowwithis_backgrounded !== false, keeping monitors, agents and ambient housekeeping off the timeline. - Scoped rows per run — rows carry a
generation, and the journal identity and coalescing key gain a#Nsuffix so a reused task id opens a new durable row instead of overwriting the finished one. - Reconciled stale rows on journal reopen —
journal-subagent-liveness.tsnow settles livebackground-taskblocks tounverifiableand moves their text twin alongside stale subagent rosters, closing the crash-recovery gap raised in the prior review. - Settled live rows on provider end — the translator calls
backgroundTasks.settleSession()onended, and the new block is bounded in worker transcripts (droppingoutputFile). - Scoped malformed-frame fallback text —
taskFrameSentencefeeds Claude'ssummary/errorto the generic fallback only for frames the row owner declined, so a covered kind never degrades to its opcode. - Pinned disposition-layer eligibility —
provider-frame-disposition.test.tsasserts the covered-kind suppression and the Claude-only boundary.
The prior review's observeStart foreground-task thread is addressed and has been retired; the tool-fold asymmetry was resolved earlier.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…tom status
The background-tasks payload types every entry as exactly
{task_id, task_type, description, ambient?}. It has no per-entry status, so the
state this owner derived from one was always undefined and the reopen branch it
guarded was unreachable on every real payload — proven by deriving the state
from an SDK-shaped entry and getting null.
Membership is the only liveness the payload carries: it is the whole live set
after a change, so presence means live and absence means merely "no longer
listed", never an outcome. Presence does not revive a settled row either — the
level's ordering against the start/stop edges is unspecified and it carries no
evidence of a new run, so the task's own frames stay the only thing that opens
or settles one. Only the identity fields it really sends are read, and ambient
housekeeping entries are excluded as the payload asks.
The two helpers that served the dead branch are removed, along with the test
that exercised it through a synthetic status the CLI cannot send.
Both changes made. Head
|
There was a problem hiding this comment.
✅ No new issues found in the new commits.
Reviewed changes — the single commit since the prior Pullfrog review (7acb0bc0a1) removes a dead aggregate-roster revive path and pins the latch rule.
- Stopped reading per-entry status from
background_tasks_changed—observeAggregateRosterno longer derives a run state fromtask.status, so a settled row is never revived by mere membership in the live set; onlytask_started/task_updated/task_notificationmove a row's state. - Deleted the now-unreachable revive helpers —
reopenClaudeBackgroundTaskRowandcanReopenClaudeBackgroundTaskRowFromAggregateare gone;shouldRestartClaudeBackgroundTaskRow(a new tool id on a settled row) remains the only re-open path. - Excluded ambient entries from roster revisions — an aggregate entry marked
ambient: trueno longer overwrites an existing row's label or kind. - Re-shaped the aggregate tests to the real payload — the synthetic per-entry
statusfixture is replaced with the{task_id, task_type, description, ambient?}shape, covering identity update, the latch rule, and ambient exclusion.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…sis path The forwarded-parent gate is conditional on the field being PRESENT. An announcement naming a tool this session never forwarded is a nested child and is refused; one naming no tool at all is admitted, because absence of the field is not evidence of an unforwarded parent. The previous rule required the field and so refused every tool-less task. Terminal frames now match on task_id alone. The forwarded-parent question is settled once, at admission, and is never re-asked on a notification or a patch. A frame for a task that was never admitted yields no row, and a patch is folded into the row it names rather than opening one. That removes the synthesized-row path entirely, and with it the named deviation it carried: the captured tool-less failure lands on a row that already exists, because its own tool-less announcement is admitted. The dead builders go with it. Left deliberately stricter than the reference, and flagged rather than changed: a terminal frame still records its task id as terminal even for a task never admitted, so a late announcement cannot open a row for work already reported finished. Two existing tests pin that.
Deviation 4 REMOVED. Fully aligned, no exception. Head
|
| Case | Result |
|---|---|
A — its own tool-less task_started arrives |
admitted; row exists; notification lands the sentence |
| B — announcement carries a forwarded tool | same |
| C — no announcement ever admitted | orphan, now dropped, matching the reference |
So the premise is dead: the captured frame is reported through its own row, and needs no synthesis. I cannot produce an orphan from a real capture — the capture records journal ROWS and task_started never becomes one, so it cannot show absence — and with tool-less announcements now admitted there is no constructed path either. Per your instruction, no named deviation. The section is struck from the PR body.
Ablations (all at head)
- Restore the
&&misreading →lands the captured failure on its row when the announcement carried no tool idfails. - Drop the forwarded refusal entirely → both nested-child regression tests fail.
- Re-add synthesis for an untracked terminal frame → 4 fail, including
drops a terminal frame for a task that was never admitted.
My first attempt at that third ablation did not discriminate — I had disabled the branch rather than restoring the behaviour, and revise on a missing row is already a no-op. Rebuilt it to actually re-add the row creation.
One thing I nearly changed silently — flagging instead
Removing synthesis first led me to also stop recording a terminal task_id for a task never admitted, which is what the reference does (it keeps no trace). Two existing tests pin the opposite — does not resurrect a task whose terminal edge arrived before its start and its task_updated twin — so a late announcement cannot open a row for work already reported finished.
I restored the stricter Orca behaviour and left it in place, because it is a third point you did not ask me to mirror and it guards a real out-of-order hazard. It is a deliberate deviation, now commented as one at the call site. Your ruling if you want it mirrored too.
Gates
pnpm tc clean, oxlint clean, changed-code quality 0 findings across 34 files, 9,145 tests pass. claude-structured-real-cli.test.ts and structured-agent-session-lease-renewer.test.ts both pass in isolation and fail only under full-suite load — the first re-baselined as pre-existing, the second untouched by this PR. Lockfile diff vs origin/main empty. en.json untouched.
There was a problem hiding this comment.
Important
Withdrawing the terminal-evidence synthesis makes an orphan failed task silently invisible, and the commit's stated justification rests on a synthetic task_started that contradicts the repo's own captured-frame evidence. See the two inline comments.
Reviewed changes — the single commit since the prior Pullfrog review (398971ea6e) reverses the terminal-synthesis path.
- Removed the terminal-only synthesis path —
observeNotification/observePatchno longer mint a row from terminal evidence for a task with no row;newClaudeBackgroundTaskTerminalRow,admitsTerminalOnlyandopenTerminalRoware deleted. A terminal frame for an untracked task now returnstrueand yields nothing. - Admitted tool-less announcements —
admitsFirstRunreturnstoolUseId === undefined || isForwardedParentTool(toolUseId), so atask_startednaming notool_use_idnow opens a row. - Re-shaped the orphan tests — the captured-failure test seeds a hand-written tool-less
task_started, and a new test pins that a terminal frame for a never-admitted task produces no row.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| private admitsFirstRun(message: Record<string, unknown>): boolean { | ||
| const toolUseId = claudeBackgroundTaskToolUseId(message) | ||
| // Conditional on the field being PRESENT. An announcement that names a tool | ||
| // this session never forwarded is a nested child and is refused; one that | ||
| // names no tool at all is admitted, because there is nothing to contradict | ||
| // — absence of the field is not evidence of an unforwarded parent. | ||
| return toolUseId === undefined || this.deps.isForwardedParentTool(toolUseId) |
There was a problem hiding this comment.
This branch admits an announcement that names no tool_use_id, but the repo's own task-frame reader documents the opposite — claude-subagent-task-frames.ts:4-6 says local_agent/local_workflow/local_bash frames ALL carry a tool_use_id — and every real-capture task_started fixture (claude-background-task-tracker.test.ts:191-204, claude-structured-journal-translation-background-tasks.test.ts:83-112) carries one. If the branch is unreachable on real payloads, then the only frame it was added to allow — a terminal failure with no admitted row — is silently dropped by observeNotification, which returns true so coveredByTypedTranslator suppresses the generic fallback. That is the exact failure the PR set out to make visible, now reported nowhere.
Technical details
# Tool-less admission is unproven; orphan terminal failures are now invisible
## Affected sites
- `src/main/claude/claude-background-task-rows.ts:178` — `toolUseId === undefined || ...` admits an announcement free of any forwarded-tool check.
- `src/main/claude/claude-background-task-rows.ts:187-201` — `observeNotification` returns `true` for a task with no row after `rememberTerminalId`.
- `src/main/claude/claude-structured-journal-translation.ts:284-285` — `backgroundTaskCovered` true + covered kind makes `unhandledProviderFrameJournalItem` return null.
- `src/main/claude/claude-subagent-task-frames.ts:4-6` — documents that all task frames carry `tool_use_id`.
## Required outcome
- Establish, from a captured transcript rather than a hand-written fixture, whether a backgrounded `local_bash` `task_started` can arrive without a `tool_use_id`. If it cannot, drop the tool-less branch and state that admission is tool-gated only.
- Decide what a terminal frame for a task with no row should do. Today it is silently invisible (coverage claimed, no row). Either leave coverage declined so the generic fallback surfaces the provider sentence, or restore a terminal-evidence row; both were available before this commit.
## Open questions for the human
- Is the "reference" that drops map-less terminal frames able to report the failure through some other channel? Orca's strip unmounts, the roster excludes shell commands, and the status feed publishes only live tasks.| it('lands the captured failure on its row when the announcement carried no tool id', () => { | ||
| // An announcement naming NO tool is admitted — absence of the field is not | ||
| // evidence of an unforwarded parent — so by the time this REAL captured | ||
| // frame arrives its row already exists and simply takes the sentence. | ||
| const { rows, latestTwin } = harness() | ||
| rows.observe({ | ||
| type: 'system', | ||
| subtype: 'task_started', | ||
| task_id: 'bo2vuy8qb', | ||
| task_type: 'local_bash', | ||
| description: 'verify', | ||
| is_backgrounded: true | ||
| }) |
There was a problem hiding this comment.
The comment calls this a "REAL captured frame", but only the task_notification was captured — the task_started above it is hand-written, and the withdrawn text in the PR body says the bo2vuy8qb capture contained no task_started at all. As written, the test proves the tool-less branch fires; it does not show the reported failure lands on a row, since real task_started frames are documented and observed to carry tool_use_id. Either seed the test with the real capture (start carrying its forwarded tool) and keep a separate falsifying case, or add the raw transcript that shows a genuinely tool-less announcement before dropping the orphan path.
Both finished. Head
|
There was a problem hiding this comment.
ℹ️ No new issues in these commits. This stays non-approving because the two earlier threads on tool-less admission / orphan-failure invisibility are untouched by them — nothing new to fix.
Reviewed changes — the single commit since the prior Pullfrog review (11ab98cb21) makes a background-task row's durable identity resolve against the journal when the sink binds, instead of trusting the in-process generation.
- Resolved row identity against the durable journal —
writeClaudeBackgroundTaskRownow preferssink.tryAppendResolvedItem, reserving a size bound and deferringresolveClaudeBackgroundTaskIdentityto bind time; the resolver scans persistedbackground-taskblocks for the task id, reuses the generation whoseparentToolUseIdmatches the row's tool id, and otherwise opensmaxGeneration + 1. - Exposed item bodies to
visitItems—AgentSessionJournal.visitItemsnow passes the reduced body as a third argument so the resolver reads the block without allocating a snapshot. - Added a deferred-identity append primitive —
StructuredAgentSessionEventSink.tryAppendResolvedItemqueues an ordinary append whose identity resolves after journal bind (async, with a resolved-size check); the lifecycle resolver type is renamed toStructuredAgentSessionIdentityResolver, with the old name kept as an alias for the Codex caller. - Pinned the rebind behavior — one test drives the real deferred sink through a queued restart (
basethenbase#2); the other re-announces a reused id from a fresh translator and asserts the prior settled run is revised in place rather than overwritten. Both go red if the resolution is removed.
ℹ️ Nitpicks
- The doc comment on
NativeChatBackgroundTaskBlock.parentToolUseId(src/shared/native-chat-types.ts:161) still says "consumers co-locate the row with that tool call", but its only reader now uses it as the run-identity key for generation matching; co-location is done byfoldToolMessagestreating the row as activity chrome. Worth rewording so it describes the reader it actually has.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this commit. This stays non-approving because the two earlier threads on tool-less admission / orphan-failure invisibility are untouched by it — nothing new to fix.
Reviewed changes — the single commit since the prior Pullfrog review (1e0843011b) changes the coalescing key so queued writes for distinct provider runs no longer overwrite each other.
- Discriminated queued writes by provider run —
writeClaudeBackgroundTaskRownow coalesces onJSON.stringify(['claude-background-task', id, row.toolUseId ?? null])instead of the translator-local journal identity. Two translators sharing one unbound deferred sink used to compute the same key for distinct runs (both local generation 1), so the queue replaced run A's pending write with run B's and dropped run A; distinct tool ids now keep both side by side, while same-run revisions still coalesce. - Dropped the always-orca ternary — the old
identity.provider === 'orca' ? identity.clientMessageId : \task:${id}`fallback was dead (the identity is always orca);identity` still feeds the non-deferred fallback path. - Pinned the overlap with a test — a new case queues a
toolu-firstand atoolu-secondannouncement for one reused id before the sink binds and assertsbaseandbase#2both persist; it fails on the previous key.
No inline concerns.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status — final head
|
…conflict-resolve # Conflicts: # src/renderer/src/components/native-chat/NativeChatToolRun.tsx
There was a problem hiding this comment.
ℹ️ No new issues found in this merge — this stays non-approving only because the two earlier threads on tool-less admission / orphan-failure invisibility are untouched by it.
Reviewed changes — the single commit since the prior Pullfrog review (29e3bcbf51) merges origin/main (60d793956a) into the branch.
- Adopted main's provider-frame rename —
isDeltaShapedProviderFrameKind→isDeltaProviderFrameKindinprovider-frame-disposition.tsand its test; no dangling reference to the old name remains anywhere in the repo. - Merged main's awaiting-input row into the tool run —
NativeChatToolRun.tsxnow carries main'snativeChatAskRunBlocks/headerBlocks/NativeChatAwaitingInputRowwork alongside the branch'sbackgroundTasks/standaloneRowsadditions, which are preserved verbatim. - Took main's journal-schema type rename —
CanonicalJournalShapesAreAdmissible→CanonicalJournalTypesAreAdmissible; the branch's'background-task'block arm is intact. - Kept the branch's catalog additions —
backgroundTasks.outputFilesurvives main's newen.jsonkeys.
Restricted to the PR's 40 paths, only these merge adoptions differ from the previously reviewed head; no PR-authored behavior changed. Verified by running provider-frame-disposition.test.ts (25), claude-background-task-rows.test.ts (40), claude-structured-journal-translation-background-tasks.test.ts (11), NativeChatToolRun.test.tsx (41), and NativeChatBackgroundTaskRun.test.tsx (6) — all pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
The bounded collections behind a background-task row were read out of the class with `Reflect.get` to prove they stay capped, which the anti-slop gate rejects. Move them into `ClaudeBackgroundTaskLedgers`, which owns the caps beside the eviction helpers and reports a typed readonly size view the tests assert against. Also replace a `Reflect.get` in the mobile recording proxy with typed property access.
There was a problem hiding this comment.
✅ No new issues found in these commits. This stays non-approving only because the two earlier threads on tool-less admission / orphan-failure invisibility are untouched by them — nothing new to fix.
Reviewed changes — the single commit since the prior Pullfrog review (7a9dd44b4c) consolidates the bounded background-task ledgers under one owner.
- Gave the bounded ledgers one owner —
ClaudeBackgroundTaskLedgersinclaude-background-task-memory.tsnow holdsgenerations,foreign,fallbackTaskIds,terminalTaskIdsandterminalToolUseIdswith their caps moved beside the eviction helpers;ClaudeBackgroundTaskRowskeeps a singleledgersfield, anddispose()clears them throughledgers.clear(). - Replaced the
Reflect.gettest probes with a typed view — the three bound tests now assertrows.ledgerSizes.{generations,foreign,fallbackTaskIds}through a new readonlyClaudeBackgroundTaskLedgerSizesgetter instead of reaching into private collections, which the anti-slop gate rejects. - Narrowed the memory helpers to module scope —
ClaudeBackgroundTaskGenerationLedger,rememberBoundedClaudeTaskSet/MapandrememberClaudeBackgroundTaskTerminalare no longer exported; no import outside the module referenced them. - Made the mobile recording proxy use typed property access —
partialNativeModulewidens its target toRecord<string | symbol, unknown>and readstarget[key]in place ofReflect.get, preserving the throw-on-unsubstituted-member behavior.
Verified by running pnpm test src/main/claude/claude-background-task-rows.test.ts → 40/40 pass, and oxlint on all four changed files → 0 findings.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ask-frame-rows Two conflicts, both resolved keeping each side's intent: - mobile/src/test-support/rpc-recording/native-mounting-substitutes.ts: took main's version wholesale. The branch's Reflect.get -> target[key] edit was only there to clear the anti-slop gate, and main fixed the same violation independently before evolving the file further. - src/main/claude/claude-structured-journal-translation.ts: main moved turn identity and the reopen latch into ClaudeOpenTurn so a client's Stop names the same turn the journal row carries; this branch added the typed background-task row wiring. Both survive. The extracted message journaler now takes the ClaudeOpenTurn itself rather than three closures over the old translator-local state, so the single-owner property main established holds across the extraction.
There was a problem hiding this comment.
ℹ️ No new issues in this merge. This stays non-approving only because the two earlier threads on tool-less admission / orphan-failure invisibility are untouched by it — nothing new to fix.
Reviewed changes — the single commit since the prior Pullfrog review (9f5c738937) merges origin/main (291b4ddd6f) into the branch.
- Adopted main's
ClaudeOpenTurn— the branch's own turn plumbing (currentTurn,openTurn,ensureTurnOpen,reopenSuppressed,publishLifecycle) is replaced by main's single-owner class:turn.ensureOpen,turn.open,turn.allowReopen,turn.suppressReopen(),turn.suppressReopenOnFailure(...)andturn.settle(...). The message-journaling context'sensureTurnOpen/openTurn/liftSuppressiontrio collapses into oneturn: ClaudeOpenTurn; behavior is preserved and no reference to the old names remains insrc/. - Preserved every branch addition — the
forwardedToolsregistry,backgroundTasksobserve / settleSession / dispose,taskFrameSentencefallback wiring andcoveredByTypedTranslatoroption all survive the conflict resolution unchanged. - Superseded the mobile recording-proxy edit — main moved
partialNativeModuleintonative-module-traps.ts, where it already widens to symbol keys and readstarget[key]behind aSAFETY:cast, so the branch's edit tonative-mounting-substitutes.tsis no longer part of the PR diff.
Verified by running pnpm exec vitest run --config config/vitest.config.ts on claude-background-task-rows.test.ts (40), claude-background-task-turn-resumption.test.ts (2), claude-structured-journal-translation-background-tasks.test.ts (11), provider-frame-disposition.test.ts (25) and claude-turn-ownership.test.ts (14) — 92/92 pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
What this looks like to a userBefore/after for a failed Claude background task in native chat. Before — current
|
…orphan-fix # Conflicts: # src/main/claude/claude-structured-journal-translation.ts
…er admitted A terminal `task_notification` for a task no announcement ever admitted rendered nothing at all. The typed row owner declined the row because its map held no entry for the id, and reported the frame as handled — which is exactly what tells the generic provider-frame fallback to stay quiet. Both surfaces declined the same frame, so a real failed background task was dropped on the floor. A terminal frame is self-sufficient: it states an outcome, and it carries the summary, status, error, output path and usage that outcome needs. It now opens its own row from those fields, with the summary as the label and `unknown` as the kind when the frame names no task type. The row map enriches a terminal frame; it never gates one. Every terminal status writes one, not failures alone, so there is one rule here rather than a third behaviour for failures. The deliberate hand-offs still win, because they are recorded rather than implied: ambient, subagent and foreground tasks are claimed in the foreign-owner ledger the notification path already checks first. A Task spawned inside a subagent's sidechain now records its refusal there too, under `sidechain`, instead of leaving no trace and reading as a task nothing ever decided about. A capacity-refused task whose outcome the generic fallback already printed records `fallback` the same way, so a redelivery neither prints twice nor mints the row capacity refused. The anti-resurrection guard stays and stays scoped to announcements: a late `task_started` cannot reopen work already reported finished. The restart rule is now stated once instead of twice — a different parent alias is the provider's restart signal only when BOTH runs name their parent, which is what the terminal ledger already required of an evicted row and what the live row now requires too.
There was a problem hiding this comment.
✅ No new issues found in the new commits. This stays non-approving only because the two earlier Pullfrog threads on tool-less admission remain open; the description note below is informational.
Reviewed changes — the commits since the prior Pullfrog review (571cbdfcd7), consisting of a merge of origin/main (e42f7c00bd) and two PR-authored commits.
- Restored a row for an orphan terminal notification —
observeNotificationnow callsopenTerminalRow, minting a durable row from the notification's ownsummary/status/error/output_file/usagewhen no announcement ever admitted the task, so a failure the transcript never saw start is shown instead of being silently dropped. The previouse39a25adc1behavior (returntrue, no row) is reversed. - Recorded the deliberate hand-offs in the foreign ledger —
ForeignOwnergains'sidechain' | 'fallback'; anadmitsFirstRunrefusal recordssidechain, and a capacity-refused task whose outcome the generic fallback already printed recordsfallback, so redeliveries stay suppressed instead of looking like a task nothing ever decided about. - Aligned the restart rule —
shouldRestartClaudeBackgroundTaskRownow requires both the settled row and the new announcement to name a parent tool, matching the evicted-row rule interminalToolUseIds. A settled row whosetoolUseIdis undefined no longer restarts on a new-tool announcement. - Split the test fixtures and added a terminal-frame suite — fixtures moved to
claude-background-task-row-test-support.ts; newclaude-background-task-terminal-frames.test.tscovers the orphan failure, the frame's own error/output/usage, non-failure terminal outcomes, the hand-offs, and both anti-resurrection cases; a translator-level test pins one typed row and no fallback row. - Merged
origin/main— adopts main'srequestedAtthreading throughjournalClaudeMessage; the covered-kindproviderFallback.appendcall site is unchanged.
Verified pnpm exec vitest run on the five related suites → 73/73 pass.
ℹ️ The PR description still says an orphan terminal frame is dropped
The "Withdrawn — the unknown-terminal-frame deviation" section states that a task never admitted at all "leaves an orphan, and that is now dropped — matching the reference" and concludes "No synthesis path remains." 11d534a10f reverses exactly that: openTerminalRow now mints a row from the terminal frame. The earlier "What changed" paragraph ("A task_notification ... still opens a row") is accurate again, so the description currently contradicts itself about the behavior this PR is named for. Rewriting the withdrawn section to match the shipped code would keep it usable as review context.
Technical details
# Stale PR description after restoring the terminal-evidence row
## Affected sites
- PR body `## Withdrawn — the unknown-terminal-frame deviation` — claims the orphan terminal frame is dropped and no synthesis path remains.
- `src/main/claude/claude-background-task-rows.ts:251-275` — `observeNotification` → `openTerminalRow` opens the row instead.
## Required outcome
- Remove or rewrite the withdrawn section so the description matches the shipped behavior: an orphan terminal frame opens its own row, and the anti-resurrection guard fences only the announcement path.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
An orphan row took its header label from the notification's summary, which also renders as the row's sentence, so the same string appeared in both slots of the same collapsed row. The label now stays empty and the header falls back to the task kind, leaving the sentence to carry the provider's words.
There was a problem hiding this comment.
✅ No new issues found in the new commit.
Reviewed changes — the single commit since the prior Pullfrog review (f93c2cb526) stops an orphan task row from drawing its provider sentence twice.
- Stopped duplicating the provider sentence on an orphan task row —
newClaudeBackgroundTaskRowFromNotificationno longer overlays the notification'ssummaryontolabel. The summary already lands insummarythroughclaudeBackgroundTaskNotificationChange, and the renderer drawslabelin the header andsummaryas the sentence beneath it, so an orphan failure printed the same provider sentence twice. The label now keeps whatevernewClaudeBackgroundTaskRowderived (description/taskName/ empty), so the header falls back to the kind label while the sentence carries the provider's own words once. - Re-shaped the terminal-frame test —
reports a failure for a task it never saw admitteddropslabelfrom the row match and assertslabel === ''with the twin still equal to the summary; the constructor's doc comment now reads "summary as the sentence".
Verified pnpm exec vitest run on claude-background-task-rows, claude-background-task-terminal-frames, claude-background-task-turn-resumption, claude-structured-journal-translation-background-tasks, provider-frame-disposition, native-chat-background-task-row and worker-transcript-background-task-text → 94/94 pass. This stays non-approving only because the two earlier Pullfrog threads (tool-less admission / orphan-failure visibility, and the "REAL captured frame" test premise) are untouched by this commit and remain open — nothing new to fix.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
One reachable identity-resolution gap in the new journal-pressure work can overwrite a prior run's durable transcript row.
Reviewed changes — the four commits since the prior Pullfrog review at b598e36604 (three PR-authored plus a merge of origin/main).
- Made task rows survive journal pressure —
ClaudeBackgroundTaskRowWriterretries a row whose append was refused (bounded at 512 pending), promotes pending writes to the lifecycle reservation at teardown, and routes exhaustion / a latched sink failure to the session viaonPersistenceFailure;writeClaudeBackgroundTaskRownow returns its admission. - Coupled sink pressure to the provider stream —
ClaudeStreamJsonConnectiongainspauseReading/resumeReading(a barrier the reader awaits before and after eachiterator.next()), the deferred sink's newtryAppendResolvedItemAndPublishqueues an append and its publication as one admitted operation, andbindClaudeJournalReadingControlretries pending task rows before the SDK read resumes. - Kept capacity-refused outcomes on typed rows — new
ClaudeOverflowTerminalRowswrites a settled row (bounded at 512) for a task whose 64 live slots were full, so its terminal patch/notification renders as a typed row instead of the opcode fallback;ForeignOwner's'fallback'becomes'terminal'. - Extracted the row/roster/start logic into domain modules —
claude-background-task-start.ts,claude-background-task-roster.ts,claude-background-task-row-writer.ts,claude-overflow-terminal-rows.ts;shouldRestartClaudeBackgroundTaskRownow keys onblock.parentToolUseId(only affects orphan rows whose parent was enriched). - Made a terminal notification the authoritative verdict —
finalizeClaudeBackgroundTaskRowrevises then hard-sets the state/settled time and enriches a missingparentToolUseId; a write only opens an output turn when the row was live before the change (so an overflow notification does not reopen a settled turn). - Recognized an ambient monitor before any announcement — a top-level forwarded
Monitortool result records its task id asforeign 'ambient', so a monitor with notask_startedno longer mints an orphan terminal row. - Merged
origin/main— adopts main'sClaudeOpenTurn,requestedAtthreading, and the session-exit lifecycle extraction.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| // A prior parentless row cannot be proved distinct from the later aliased outcome. | ||
| return claudeBackgroundTaskIdentity( | ||
| id, | ||
| matchingGeneration ?? parentlessGeneration ?? (maxGeneration === 0 ? 1 : maxGeneration + 1) |
There was a problem hiding this comment.
The parentlessGeneration fallback reuses the parentless row's generation for any later aliased resolve, but the comment above it scopes the trade-off to "the later aliased outcome". A fresh translator after a stream rebind that admits a genuinely new task_started for a task whose only durable row is a settled parentless one resolves to that same generation and upserts over the earlier run's journal body instead of opening #2 — the overwrite the #N generation exists to prevent, and the in-memory restart guard cannot catch it because it is translator-local.
Technical details
# A new announcement can reuse a parentless row's generation
## Affected sites
- `src/main/claude/claude-background-task-row-journal.ts:74-81` — `parentlessGeneration` is consulted for every aliased resolve, with no distinction between a terminal outcome and a new announcement.
- `src/main/claude/claude-background-task-rows.ts:161-165` — `openRow` (a fresh `task_started`) calls `write` with the fresh in-memory generation, which the deferred resolve then overrides with the persisted parentless generation.
- `src/main/claude/claude-background-task-row-journal.ts:162-170` — the deferred branch routes identity through `identities.resolve`, so the persisted journal decides the generation.
- `src/main/native-chat/agent-session-journal/journal-store.ts` — `appendItem` upserts by identity, so the earlier body is replaced rather than versioned apart.
## Reachable sequence
1. 64 live rows; `task_started` for task `X` falls back (`rememberFallback`), then a terminal `task_updated` with no `tool_use_id` persists a parentless row at `claude-background-task:X` (generation 1).
2. A stream rebind builds a fresh translator, `ClaudeBackgroundTaskRows`, ledgers and resolver; the durable journal (and that parentless row) survives.
3. A new `task_started` for `X` carrying forwarded `tool_use_id: B` is admitted (`observeStart` sees no in-memory row, so the restart guard is skipped), `openRow` takes the fresh ledger's generation 1, and the deferred resolve finds the persisted parentless row and returns generation 1.
4. `appendItem` overwrites `claude-background-task:X` with run B's body — no `#2`, and run A's durable row is gone.
## Required outcome
- A new run's announcement must not reuse a parentless generation. The reconcile the fallback exists for (a terminal notification enriching the provisional parentless patch row) must keep working.
## Suggested approach (optional)
- Tell the resolver whether the write is an announcement or an outcome (e.g. thread a flag through `writeClaudeBackgroundTaskRow` from `openRow` vs `observeNotification`), and consult `parentlessGeneration` only for the outcome path.
## Open question for the human
- Is the collapse meant to cover the announcement path too? The doc on `claudeBackgroundTaskIdentity` and the PR description both state a reused task id must not overwrite the first run's transcript history, which this path does.
|
Review status for pushed head Functional correctness: a real Claude background command exited 1 in structured native chat. One journal row progressed through start, failed status, and final notification; the rendered failure sentence is readable, with no internal task opcode row. The full-window screenshot and exact scenario are in the PR description. Architecture and performance: typed task ownership, atomic journal append/publication, bounded retry obligations and sink backpressure, prompt child-tree closure on provider exit, and cached row identity address the delivery/lifecycle and scale failures without adding polling or unbounded scans. Review checked compatibility with older text-only clients and the supported host boundaries. Precedent and deliberate difference: the task lifecycle uses the established typed ownership/delivery shape, but a terminal frame is independently reportable if its start was missed. An ambiguous ID-only late start cannot resurrect a finished task without fresh parent-tool identity. This is intentional for the observed producer ordering, not a claim that every lifecycle mechanism is identical elsewhere. Validation: local typecheck, 0 changed-code-quality findings across 59 files, 333 focused/adjacent tests, real-Claude Electron proof, both packaging jobs, all eight CI test shards, static analysis, cross-version wire compatibility, and final verify passed. Historical rows already persisted with the old opcode text are not migrated. The separately requested Grok-operated visual pass could not run because that account hit its weekly credit limit; the live-Claude CDP pass supplies the final-head visual proof instead. No direct SSH/mobile UI run was performed because this change is in host-side Claude ingestion, not a mobile or SSH UI path. |
…stead of an opcode (stablyai#20519) * fix(native-chat): give a failed Claude background task a typed row instead of an opcode A failed backgrounded command printed red rows whose visible text was the wire opcode, and printed one failure twice. All five task lifecycle kinds are catalogued status-chrome, but the payload sniffer in classifyProviderFrame runs first and promotes any frame reporting a failure to the generic unknown-frame fallback, whose sentence lookup has no key for the field Claude puts its own sentence in. Two frames for one task therefore produced two rows, both of them the method name. Suppressing those frames is not the fix: when the last background task settles the tracker flushes it and the strip unmounts, local_bash is excluded from the subagent roster, and the status feed publishes only live tasks, so for a lone backgrounded command the transcript row is the only report of the failure that exists anywhere. So the catalogue now binds: kinds a dedicated typed translator owns are named as covered, and the generic fallback refuses to emit for them in either direction. A new row owner keeps one durable row per task id, opened by the announcement, revised in place by the lifecycle frames and closed by the notification, carrying the provider's summary, error, output path, usage and a run state. The row is written on the same dual carrier the subagent roster uses: a frozen text twin plus a typed block, so a client without the block type reads the sentence rather than nothing. hasProviderError keeps its authority everywhere else, unchanged. * fix(native-chat): keep tool attribution across a background-task row A background task's row is a system message landing mid-turn between the assistant's tool calls, exactly where the spawn-group roster row lands. Without the same exemption it ended the run the following tool messages fold into, so a tool result arriving after one stopped folding into its own assistant turn. Also syncs the catalog with the row's one new translate key. * fix(native-chat): harden background task rows * fix(native-chat): settle background rows on provider end * fix(native-chat): scope malformed task fallback text * test(native-chat): assert only eligibility at the disposition layer The malformed-task-frame test asserted the generic fallback resolves Claude's `summary` field itself, which was true only while that key sat in the shared key list. Eligibility is what this layer decides; the sentence the row leads with is Claude's, supplied through the display-text seam and proven in the translation test. * fix(native-chat): gate background-task admission and scope rows per run Admission now matches the reference on all three gates. Type is the whole gate and MONITORS ARE NOT ADMITTED: a monitor runs for the life of the session and has no outcome a row could report, so it never reaches the timeline. On first admission only, the task's tool_use_id must name a tool call this session forwarded at the TOP level — a Task spawned inside a subagent's sidechain names an id that never reached the transcript, and a top-level row for it would claim an invocation the user never saw. And a task that already exists and has not finished is not re-opened: a duplicate announcement is a redelivery, not a second run. Rows are now keyed per RUN. A provider may reuse a task id for a distinct later invocation, and a row keyed by the id alone overwrote the first run's transcript history instead of leaving it standing. Generation 1 keeps the bare key, so every row already written is unaffected. The spawning tool call is carried on the row as parentToolUseId. Orca's journal has no structural parent link for an item — AgentJournalItemIdentity has four arms and none carries one — so the relationship is data on the item rather than nesting. A terminal frame that names NO tool still opens a row. That is a named deviation, recorded at its call site, and the measurement behind it is in the PR. * fix(native-chat): read the aggregate roster by membership, not a phantom status The background-tasks payload types every entry as exactly {task_id, task_type, description, ambient?}. It has no per-entry status, so the state this owner derived from one was always undefined and the reopen branch it guarded was unreachable on every real payload — proven by deriving the state from an SDK-shaped entry and getting null. Membership is the only liveness the payload carries: it is the whole live set after a change, so presence means live and absence means merely "no longer listed", never an outcome. Presence does not revive a settled row either — the level's ordering against the start/stop edges is unspecified and it carries no evidence of a new run, so the task's own frames stay the only thing that opens or settles one. Only the identity fields it really sends are read, and ambient housekeeping entries are excluded as the payload asks. The two helpers that served the dead branch are removed, along with the test that exercised it through a synthetic status the CLI cannot send. * fix(native-chat): mirror reference task admission and drop the synthesis path The forwarded-parent gate is conditional on the field being PRESENT. An announcement naming a tool this session never forwarded is a nested child and is refused; one naming no tool at all is admitted, because absence of the field is not evidence of an unforwarded parent. The previous rule required the field and so refused every tool-less task. Terminal frames now match on task_id alone. The forwarded-parent question is settled once, at admission, and is never re-asked on a notification or a patch. A frame for a task that was never admitted yields no row, and a patch is folded into the row it names rather than opening one. That removes the synthesized-row path entirely, and with it the named deviation it carried: the captured tool-less failure lands on a row that already exists, because its own tool-less announcement is admitted. The dead builders go with it. Left deliberately stricter than the reference, and flagged rather than changed: a terminal frame still records its task id as terminal even for a task never admitted, so a late announcement cannot open a row for work already reported finished. Two existing tests pin that. * fix(native-chat): preserve background task ownership across restarts * chore: restore pnpm-lock.yaml to origin/main A local pnpm run rewrote the lockfile and the merge commit swept it in. The branch changes no dependencies, so it must carry no lockfile delta at all. * fix(claude): harden background task lifecycle * fix(claude): bound task generation history * fix(claude): preserve task identity after history eviction * fix(claude): resolve background task identity after rebind * fix(claude): isolate queued task runs * refactor(claude): give the background-task ledgers one bounded owner The bounded collections behind a background-task row were read out of the class with `Reflect.get` to prove they stay capped, which the anti-slop gate rejects. Move them into `ClaudeBackgroundTaskLedgers`, which owns the caps beside the eviction helpers and reports a typed readonly size view the tests assert against. Also replace a `Reflect.get` in the mobile recording proxy with typed property access. * fix(native-chat): report a background task failure the transcript never admitted A terminal `task_notification` for a task no announcement ever admitted rendered nothing at all. The typed row owner declined the row because its map held no entry for the id, and reported the frame as handled — which is exactly what tells the generic provider-frame fallback to stay quiet. Both surfaces declined the same frame, so a real failed background task was dropped on the floor. A terminal frame is self-sufficient: it states an outcome, and it carries the summary, status, error, output path and usage that outcome needs. It now opens its own row from those fields, with the summary as the label and `unknown` as the kind when the frame names no task type. The row map enriches a terminal frame; it never gates one. Every terminal status writes one, not failures alone, so there is one rule here rather than a third behaviour for failures. The deliberate hand-offs still win, because they are recorded rather than implied: ambient, subagent and foreground tasks are claimed in the foreign-owner ledger the notification path already checks first. A Task spawned inside a subagent's sidechain now records its refusal there too, under `sidechain`, instead of leaving no trace and reading as a task nothing ever decided about. A capacity-refused task whose outcome the generic fallback already printed records `fallback` the same way, so a redelivery neither prints twice nor mints the row capacity refused. The anti-resurrection guard stays and stays scoped to announcements: a late `task_started` cannot reopen work already reported finished. The restart rule is now stated once instead of twice — a different parent alias is the provider's restart signal only when BOTH runs name their parent, which is what the terminal ledger already required of an evicted row and what the live row now requires too. * test(native-chat): pin that an orphan task row reopens the provider's turn * fix(native-chat): stop an orphan task row drawing its sentence twice An orphan row took its header label from the notification's summary, which also renders as the row's sentence, so the same string appeared in both slots of the same collapsed row. The label now stays empty and the header falls back to the task kind, leaving the sentence to carry the provider's words. * fix(native-chat): keep Claude task outcomes owned through capacity and redelivery * fix(native-chat): keep settled overflow notifications from reopening a turn * fix(native-chat): retain Claude task rows through journal pressure
Failure and fix
A failed Claude background command previously produced opaque
message:system:task_updated/message:system:task_notificationrows. The generic error fallback sawstatus: failedbefore the typed-frame classification and did not read the provider sentence fromsummary; a status update and terminal notice could also appear as duplicate errors.The Claude session now owns one typed journal row per background task ID. Start, progress, failure status, and final notification revise that row instead of leaking internal opcodes. The final notification carries its own truthful outcome even if the start was missed. Backgrounded command/workflow tasks use this owner; subagents and ambient tasks retain their existing owners. Older/text-only clients receive a readable frozen text twin alongside the optional structured block; no new wire opcode or required field is introduced.
Journal writes and publication are admitted together. At bounded sink pressure the provider reader pauses, retains refused terminal rows as bounded retry obligations, retries them before reading resumes, and enters generation-fenced recovery on permanent failure. Provider exit starts child-tree closure promptly while retained delivery ownership drains buffered frames. Task identity resolution is cached per journal object/epoch with a bounded LRU, removing repeated whole-journal scans on revisions.
A settled command/workflow task opens a new generation only when a fresh parent-tool identity proves a new invocation. An ambiguous late ID-only start stays terminal: the producer can remove a task from the live roster before delivering its outcome, and reopening that already-finished task would recreate the original class of defect. Historical opcode rows already persisted are not rewritten; this corrects new emission.
Real-Claude Electron proof
On pushed merged head
d8c7281924638fe513eea26222a9ff832f164190, a fresh native-chat Claude run launched background commandbsjej7w8z, which exited 1. The journal shows a single row revised from start to failed status to final notification (revisions 1–3). The rendered row saysBackground command "Run native-chat regression check probe" failed with exit code 1, and nomessage:system:task_notificationrow appears. This is live provider output, not replay or mock data.Validation
Local exact-head checks:
pnpm tc; changed-code quality 0 findings across 59 files; 252 focused tests in 20 files plus 81 adjacent lifecycle tests in 4 files;git diff --check; clean worktree. Independent final review and closing readiness found no proven in-scope issue. CI passed on this exact pushed SHA: all eight test shards, both packaging jobs, typecheck, static analysis, cross-version wire compatibility, and the final verify gate.