From ce5e7afe57b7e3a985f0f445792abef4f7a9044d Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Sat, 5 Sep 2026 02:11:42 -0700 Subject: [PATCH] fix(native-chat): stop an unrelated turn end settling an outside-turn child MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `settleTurn` swept the `outside-turn` group on every turn end, so a child Claude announced while no turn was live — a frame trailing the previous turn's result, or one that arrives before the first turn starts — was marked `unverifiable` by the next, unrelated turn ending. That state is terminal and latches, so the `task_updated: completed` that followed was discarded: loss of contact was recorded as the child's outcome on evidence that was never about that child. A turn end now sweeps exactly the group its key names. `outside-turn` belongs to no turn, so only an end with no key of its own reaches it, and what no turn end reaches `settleSession` does — reliably, since teardown without an `ended` event also routes through it. The cost is a child outside every turn showing `working` a little longer; the alternative prints a wrong outcome that nothing can revise. Also pins that a subagent announced after a task the filter rejected still rosters: the announcement path was never what the child-traffic gate closes. --- ...ured-journal-translation-subagents.test.ts | 62 ++++++++++++++----- .../claude/claude-subagent-roster.test.ts | 41 +++++++++++- src/main/claude/claude-subagent-roster.ts | 16 ++--- 3 files changed, 93 insertions(+), 26 deletions(-) diff --git a/src/main/claude/claude-structured-journal-translation-subagents.test.ts b/src/main/claude/claude-structured-journal-translation-subagents.test.ts index 1c548542229..3b280020c31 100644 --- a/src/main/claude/claude-structured-journal-translation-subagents.test.ts +++ b/src/main/claude/claude-structured-journal-translation-subagents.test.ts @@ -27,12 +27,7 @@ function harness() { const translator = createClaudeJournalTranslator({ sink, fallbackIdPrefix: 'test' }) const groupRows = () => items.filter((item) => orcaClientMessageId(item.identity) === GROUP_ITEM_ID) - /** The last roster row written for one turn, so a test can read a turn that is - * no longer the live one. */ - const rosterOf = (turnUuid: string): NativeChatSubagentEntry[] => { - const body = items.findLast( - (item) => orcaClientMessageId(item.identity) === `claude-subagents:claude-session:${turnUuid}` - )?.body + const agentsOf = (body: AgentJournalItemBody | undefined): NativeChatSubagentEntry[] => { if (!body || body.kind !== 'message') { return [] } @@ -41,21 +36,21 @@ function harness() { ) return block ? block.agents : [] } - const roster = (): NativeChatSubagentEntry[] => { - const body = groupRows().at(-1)?.body - if (!body || body.kind !== 'message') { - return [] - } - const block = body.blocks.find( - (candidate): candidate is NativeChatSubagentGroupBlock => candidate.type === 'subagent-group' + /** The last roster row written for one group, so a test can read a group that + * is no longer the live one. */ + const rosterIn = (groupId: string): NativeChatSubagentEntry[] => + agentsOf( + items.findLast((item) => orcaClientMessageId(item.identity) === `claude-subagents:${groupId}`) + ?.body ) - return block ? block.agents : [] - } + const rosterOf = (turnUuid: string): NativeChatSubagentEntry[] => + rosterIn(`claude-session:${turnUuid}`) + const roster = (): NativeChatSubagentEntry[] => agentsOf(groupRows().at(-1)?.body) const fallbackRows = (): AgentJournalItemBody[] => items .filter((item) => (orcaClientMessageId(item.identity) ?? '').startsWith('provider-frame:')) .map((item) => item.body) - return { translator, groupRows, roster, rosterOf, fallbackRows } + return { translator, groupRows, roster, rosterIn, rosterOf, fallbackRows } } function userTurn(uuid: string) { @@ -226,4 +221,39 @@ describe('claude journal translation — subagents', () => { expect(rosterOf('user-1')).toEqual([expect.objectContaining({ state: 'unverifiable' })]) expect(rosterOf('user-2')).toEqual([expect.objectContaining({ state: 'working' })]) }) + + it('does not let an unrelated turn end settle a child announced outside a turn', () => { + const { translator, rosterIn } = harness() + // No turn is live yet, so this child has no turn key to belong to. + translator.handle( + systemFrame('task_started', { + task_id: 'task-early', + task_type: 'local_agent', + description: 'Before the turn' + }) + ) + translator.handle(userTurn('user-1')) + translator.handle(resultFrame()) + expect(rosterIn('outside-turn')).toEqual([expect.objectContaining({ state: 'working' })]) + // The outcome still lands, which a latched `unverifiable` would have lost. + translator.handle( + systemFrame('task_updated', { task_id: 'task-early', patch: { status: 'completed' } }) + ) + expect(rosterIn('outside-turn')).toEqual([expect.objectContaining({ state: 'completed' })]) + }) + + it('settles a child left outside every turn when the session ends', () => { + const { translator, rosterIn } = harness() + translator.handle( + systemFrame('task_started', { + task_id: 'task-early', + task_type: 'local_agent', + description: 'Before the turn' + }) + ) + translator.handle(userTurn('user-1')) + translator.handle(resultFrame()) + translator.handle({ type: 'ended', sessionId: 'orca-session', reason: 'closed' }) + expect(rosterIn('outside-turn')).toEqual([expect.objectContaining({ state: 'unverifiable' })]) + }) }) diff --git a/src/main/claude/claude-subagent-roster.test.ts b/src/main/claude/claude-subagent-roster.test.ts index b101218b596..ee48acbf5e0 100644 --- a/src/main/claude/claude-subagent-roster.test.ts +++ b/src/main/claude/claude-subagent-roster.test.ts @@ -296,6 +296,21 @@ describe('ClaudeSubagentRoster', () => { expect(items).toHaveLength(0) }) + it('still rosters a subagent announced after a task the filter rejected', () => { + const { roster, roles } = harness() + roster.observeSystemFrame( + system('task_started', { task_id: 'task-bash', task_type: 'local_bash' }) + ) + // The gate closes the child-traffic fallback, never the announcement path. + roster.observeSystemFrame( + started({ task_id: 'task-1', tool_use_id: 'toolu_1', description: 'Explore' }) + ) + roster.observeChildActivity('toolu_1') + expect(roles()).toEqual([ + expect.objectContaining({ id: 'task-1', label: 'Explore', state: 'working' }) + ]) + }) + it('leaves a grandchild parented inside the sidechain out of the roster', () => { const { roster, roles } = harness() roster.observeSystemFrame( @@ -362,14 +377,36 @@ describe('ClaudeSubagentRoster', () => { }) describe('ClaudeSubagentRoster — the turn that is ending', () => { - it('sweeps children rostered before any turn key existed', () => { + it('leaves a child announced outside any turn alone when an unrelated turn ends', () => { const { roster, rolesIn, setGroupKey } = harness(null) roster.observeSystemFrame(started({ task_id: 'task-early', description: 'Early' })) setGroupKey(TURN_1) roster.observeSystemFrame(started({ task_id: 'task-turn', description: 'In turn' })) roster.settleTurn(TURN_1) - expect(rolesIn('outside-turn')).toEqual([expect.objectContaining({ state: 'unverifiable' })]) + expect(rolesIn('outside-turn')).toEqual([expect.objectContaining({ state: 'working' })]) expect(rolesIn(TURN_1)).toEqual([expect.objectContaining({ state: 'unverifiable' })]) + // `unverifiable` latches, so sweeping it above would have swallowed this. + roster.observeSystemFrame( + system('task_updated', { task_id: 'task-early', patch: { status: 'completed' } }) + ) + expect(rolesIn('outside-turn')).toEqual([expect.objectContaining({ state: 'completed' })]) + }) + + it('sweeps the outside-turn group when a turn with no key of its own ends', () => { + const { roster, rolesIn } = harness(null) + roster.observeSystemFrame(started({ task_id: 'task-early', description: 'Early' })) + roster.settleTurn(null) + expect(rolesIn('outside-turn')).toEqual([expect.objectContaining({ state: 'unverifiable' })]) + }) + + it('still settles an outside-turn child once the session itself ends', () => { + const { roster, rolesIn, setGroupKey } = harness(null) + roster.observeSystemFrame(started({ task_id: 'task-early', description: 'Early' })) + setGroupKey(TURN_1) + roster.observeSystemFrame(started({ task_id: 'task-turn', description: 'In turn' })) + roster.settleTurn(TURN_1) + roster.settleSession() + expect(rolesIn('outside-turn')).toEqual([expect.objectContaining({ state: 'unverifiable' })]) }) it('sweeps the turn that ended, not whichever turn is live now', () => { diff --git a/src/main/claude/claude-subagent-roster.ts b/src/main/claude/claude-subagent-roster.ts index aaa987a7929..bba85d6bc2b 100644 --- a/src/main/claude/claude-subagent-roster.ts +++ b/src/main/claude/claude-subagent-roster.ts @@ -46,7 +46,9 @@ type RosterGroup = { /** Insertion order is the display order; the map holds the state. */ entries: Map /** High-water mark of claims per label, so a repeat gets an ordinal suffix. - * It never decreases: re-issuing an ordinal would print two identical rows. */ + * It never decreases: re-issuing an ordinal would print two identical rows. + * Growing it past the entry cap takes a stream that re-announces a rostered + * agent as a shell task, which churns the row far harder than the map. */ labelCounts: Map /** Last body written, so an idempotent replay writes no new revision. */ lastSerialized: string | null @@ -166,13 +168,11 @@ export class ClaudeSubagentRoster { * explicitly told to outlive the turn and is left alone. */ settleTurn(groupKey: string | null): void { - const ending = groupKey ?? OUTSIDE_TURN - this.sweep(this.groups.get(ending), false) - if (ending !== OUTSIDE_TURN) { - // Children Claude reported before any turn key existed sit here, and no - // turn will ever name this group, so every turn end sweeps it too. - this.sweep(this.groups.get(OUTSIDE_TURN), false) - } + // Only the group this key names. `OUTSIDE_TURN` belongs to no turn, so an + // unrelated turn ending is no evidence about a child announced outside it — + // and `unverifiable` latches, so sweeping it there would swallow the + // `completed` that still arrives. `settleSession` reaches what no turn does. + this.sweep(this.groups.get(groupKey ?? OUTSIDE_TURN), false) } /** The provider is gone. Nothing more will arrive for any child, backgrounded