diff --git a/src/main/claude/claude-subagent-id-aliases.test.ts b/src/main/claude/claude-subagent-id-aliases.test.ts index 4b2bb407493..bffb549d173 100644 --- a/src/main/claude/claude-subagent-id-aliases.test.ts +++ b/src/main/claude/claude-subagent-id-aliases.test.ts @@ -38,6 +38,17 @@ describe('ClaudeSubagentIds', () => { expect(ids.isExcluded('task-1')).toBe(true) }) + it('does not retain oversized aliases or exclusions', () => { + const ids = new ClaudeSubagentIds() + const oversized = 'x'.repeat(513) + ids.alias(oversized, 'task-1') + ids.alias('tool-1', oversized) + ids.exclude(oversized) + expect(ids.canonical(oversized)).toBe(oversized) + expect(ids.canonical('tool-1')).toBe('tool-1') + expect(ids.isExcluded(oversized)).toBe(false) + }) + it('forgets everything on clear', () => { const ids = new ClaudeSubagentIds() ids.alias('toolu_1', 'task-1') diff --git a/src/main/claude/claude-subagent-id-aliases.ts b/src/main/claude/claude-subagent-id-aliases.ts index eb34d40c0e4..d06bc00bf8d 100644 --- a/src/main/claude/claude-subagent-id-aliases.ts +++ b/src/main/claude/claude-subagent-id-aliases.ts @@ -9,6 +9,8 @@ // said "this is a backgrounded shell, not an agent" has to be remembered or a // later frame re-admits it. +import { isBoundedClaudeTaskId } from './claude-background-task-tracker' + /** Both maps are event-accumulated and nothing prunes them, so both are bounded. */ const MAX_TOOL_USE_ALIASES = 512 const MAX_EXCLUDED_IDS = 512 @@ -23,6 +25,9 @@ export class ClaudeSubagentIds { } alias(toolUseId: string, taskId: string): void { + if (!isBoundedClaudeTaskId(toolUseId) || !isBoundedClaudeTaskId(taskId)) { + return + } this.canonicalByToolUse.set(toolUseId, taskId) while (this.canonicalByToolUse.size > MAX_TOOL_USE_ALIASES) { const oldest = this.canonicalByToolUse.keys().next() @@ -34,6 +39,9 @@ export class ClaudeSubagentIds { } exclude(id: string): void { + if (!isBoundedClaudeTaskId(id)) { + return + } this.excluded.add(id) while (this.excluded.size > MAX_EXCLUDED_IDS) { const oldest = this.excluded.values().next() diff --git a/src/main/claude/claude-subagent-roster-state.ts b/src/main/claude/claude-subagent-roster-state.ts new file mode 100644 index 00000000000..2fa8971d856 --- /dev/null +++ b/src/main/claude/claude-subagent-roster-state.ts @@ -0,0 +1,75 @@ +import type { AgentJournalItemIdentity } from '../../shared/agent-session-journal-types' +import type { NativeChatSubagentEntry } from '../../shared/native-chat-types' +import type { ClaudeSubagentTaskFrame } from './claude-subagent-task-frames' + +const MAX_INVOCATIONS_PER_SUBAGENT = 16 + +export type TrackedEntry = { + entry: NativeChatSubagentEntry + /** The only signal separating a child that dies with its turn from one told to + * outlive it. A turn-end sweep must leave a backgrounded child alone. */ + backgrounded: boolean + toolUseId: string | null + invocationIds: Set | null + /** Label before its ordinal suffix, so a later announcement can tell a + * provisional row from one that already carries the provider's own name. */ + labelBase: string +} + +export type RosterGroup = { + groupId: string + identity: AgentJournalItemIdentity + /** Insertion order is the display order; the map holds the state. */ + entries: Map + /** Lifetime admissions bound retained labels even when entries are removed. */ + admittedEntries: number + /** Labels remain reserved after removal or provisional-name replacement. */ + claimedLabels: Set + /** Last body written, so an idempotent replay writes no new revision. */ + lastSerialized: string | null +} + +// Invocation history stays with the entry, independent of the evicting alias cache. +export function applyClaudeSubagentInvocation( + tracked: TrackedEntry, + frame: ClaudeSubagentTaskFrame, + now: () => number +): boolean { + if (tracked.invocationIds === null) { + return false + } + const newInvocation = + frame.announcement && frame.toolUseId !== null && !tracked.invocationIds.has(frame.toolUseId) + if (newInvocation && frame.toolUseId) { + if (tracked.invocationIds.size >= MAX_INVOCATIONS_PER_SUBAGENT) { + tracked.invocationIds = null + tracked.entry = { ...tracked.entry, state: 'unverifiable', settledAt: now() } + return true + } + tracked.invocationIds.add(frame.toolUseId) + if (tracked.toolUseId !== null && tracked.toolUseId !== frame.toolUseId) { + tracked.backgrounded = frame.backgrounded ?? false + tracked.entry = { ...tracked.entry, state: frame.state ?? 'working', settledAt: undefined } + } + tracked.toolUseId = frame.toolUseId + } else if (tracked.toolUseId && frame.toolUseId && tracked.toolUseId !== frame.toolUseId) { + return false + } + if (tracked.toolUseId === null) { + tracked.toolUseId = frame.toolUseId + } + return true +} + +/** Two children can share a description; the ordinal keeps their rows apart + * without inventing a name the provider never sent. The probe is over the + * labels actually rendered, not a per-base counter: a generated `Audit 2` + * must not collide with a provider that names its own child `Audit 2`. */ +export function claimClaudeSubagentLabel(group: RosterGroup, base: string): string { + let candidate = base + for (let ordinal = 2; group.claimedLabels.has(candidate); ordinal++) { + candidate = `${base} ${ordinal}` + } + group.claimedLabels.add(candidate) + return candidate +} diff --git a/src/main/claude/claude-subagent-roster.test.ts b/src/main/claude/claude-subagent-roster.test.ts index af107481c37..da1f16d0a0e 100644 --- a/src/main/claude/claude-subagent-roster.test.ts +++ b/src/main/claude/claude-subagent-roster.test.ts @@ -483,3 +483,120 @@ describe('ClaudeSubagentRoster — through the real sink queue', () => { expect(published).toBeGreaterThan(0) }) }) + +describe('ClaudeSubagentRoster — authoritative outcomes and retained budgets', () => { + it('accepts a notification after the foreground turn lost contact', () => { + const { roster, roles } = harness() + roster.observeSystemFrame(started({ task_id: 'task-1' })) + roster.settleTurn(TURN_1) + roster.observeSystemFrame( + system('task_notification', { task_id: 'task-1', status: 'completed' }) + ) + expect(roles()).toEqual([expect.objectContaining({ state: 'completed' })]) + }) + + it('settles a background child from its notification without a task_updated', () => { + const { roster, roles } = harness() + roster.observeSystemFrame(started({ task_id: 'task-1', is_backgrounded: true })) + roster.settleTurn(TURN_1) + roster.observeSystemFrame(system('task_notification', { task_id: 'task-1', status: 'failed' })) + expect(roles()).toEqual([expect.objectContaining({ state: 'failed' })]) + }) + + it('bounds lifetime admissions when reclassification repeatedly removes entries', () => { + const { roster, items } = harness() + for (let i = 0; i < 100; i++) { + roster.observeSystemFrame(started({ task_id: `task-${i}`, description: `Agent ${i}` })) + roster.observeSystemFrame( + system('task_started', { task_id: `task-${i}`, task_type: 'local_bash' }) + ) + } + expect(items).toHaveLength(64) + }) +}) + +describe('ClaudeSubagentRoster — resumed invocation', () => { + it('reopens one canonical child on a new announcement without replaying old results', () => { + const { roster, roles } = harness() + roster.observeSystemFrame(started({ task_id: 'task-1', tool_use_id: 'first' })) + roster.observeToolResult('first', false) + roster.observeSystemFrame( + started({ task_id: 'task-1', tool_use_id: 'resumed', is_backgrounded: true }) + ) + expect(roles()).toEqual([expect.objectContaining({ id: 'task-1', state: 'working' })]) + expect(roles()[0].settledAt).toBeUndefined() + roster.observeSystemFrame( + system('task_notification', { task_id: 'task-1', tool_use_id: 'first', status: 'completed' }) + ) + roster.observeSystemFrame(started({ task_id: 'task-1', tool_use_id: 'first' })) + expect(roles()[0].state).toBe('working') + roster.observeSystemFrame( + system('task_notification', { + task_id: 'task-1', + tool_use_id: 'resumed', + status: 'completed' + }) + ) + roster.observeSystemFrame( + started({ task_id: 'task-1', tool_use_id: 'resumed', is_backgrounded: true }) + ) + expect(roles()[0].state).toBe('completed') + }) +}) + +describe('ClaudeSubagentRoster — invocation fences', () => { + it('ignores a previous invocation tool result even without a background flag', () => { + const { roster, roles } = harness() + roster.observeSystemFrame(started({ task_id: 'task-1', tool_use_id: 'first' })) + roster.observeToolResult('first', false) + roster.observeSystemFrame(started({ task_id: 'task-1', tool_use_id: 'next' })) + roster.observeToolResult('first', true) + expect(roles()[0].state).toBe('working') + roster.observeToolResult('next', false) + expect(roles()[0].state).toBe('completed') + }) + + it('does not treat an evicted alias as a new invocation', () => { + const { roster, rolesIn, setGroupKey } = harness() + roster.observeSystemFrame(started({ task_id: 'task-1', tool_use_id: 'first' })) + roster.observeToolResult('first', false) + setGroupKey('churn') + for (let i = 0; i < 513; i++) { + roster.observeSystemFrame( + system('task_updated', { task_id: `other-${i}`, tool_use_id: `tool-${i}` }) + ) + } + roster.observeSystemFrame(started({ task_id: 'task-1', tool_use_id: 'first' })) + expect(rolesIn(TURN_1)[0].state).toBe('completed') + }) + + it('bounds invocation history and refuses to reopen beyond the retained budget', () => { + const { roster, roles } = harness() + for (let i = 0; i < 20; i++) { + roster.observeSystemFrame(started({ task_id: 'task-1', tool_use_id: `tool-${i}` })) + if (i >= 16) { + expect(roles()[0].state).toBe('unverifiable') + } + roster.observeToolResult(`tool-${i}`, false) + } + roster.observeSystemFrame(started({ task_id: 'task-1', tool_use_id: 'tool-0' })) + expect(roles()[0].state).toBe('unverifiable') + }) +}) + +it('merges an explicit foreground patch without clearing on absent metadata', () => { + const { roster, roles } = harness() + roster.observeSystemFrame( + started({ task_id: 'task-1', tool_use_id: 'tool', is_backgrounded: true }) + ) + roster.observeSystemFrame( + system('task_updated', { task_id: 'task-1', patch: { description: 'Audit' } }) + ) + roster.observeToolResult('tool', false) + expect(roles()[0].state).toBe('working') + roster.observeSystemFrame( + system('task_updated', { task_id: 'task-1', patch: { is_backgrounded: false } }) + ) + roster.observeToolResult('tool', false) + expect(roles()[0].state).toBe('completed') +}) diff --git a/src/main/claude/claude-subagent-roster.ts b/src/main/claude/claude-subagent-roster.ts index ca06b1c7a0e..34083f2ee8c 100644 --- a/src/main/claude/claude-subagent-roster.ts +++ b/src/main/claude/claude-subagent-roster.ts @@ -8,18 +8,25 @@ // // Claude re-announces a resumed task under a NEW `tool_use_id`, so `task_id` is // the key and tool ids are aliases; keying on the tool id would duplicate the -// child on every resume. Every transition is idempotent and a terminal state -// latches, because progress, updates and the parent's tool result can each -// report the same outcome. +// child on every resume. Outcomes latch within an invocation; a new spawn +// alias can reopen it, and authoritative evidence can correct lost contact. -import type { AgentJournalItemIdentity } from '../../shared/agent-session-journal-types' -import { isTerminalSubagentState } from '../../shared/native-chat-subagent-summary' +import { + canReplaceSubagentState, + isTerminalSubagentState +} from '../../shared/native-chat-subagent-summary' import type { NativeChatSubagentEntry } from '../../shared/native-chat-types' import type { StructuredAgentSessionEventSink } from '../native-chat/agent-session-wire/structured-agent-session-event-sink' import { isBoundedClaudeTaskId } from './claude-background-task-tracker' import { claudeSubagentGroupBody, claudeSubagentGroupIdentity } from './claude-subagent-group-row' import { ClaudeSubagentIds } from './claude-subagent-id-aliases' import { readClaudeSubagentTaskFrame } from './claude-subagent-task-frames' +import { + applyClaudeSubagentInvocation, + claimClaudeSubagentLabel, + type RosterGroup, + type TrackedEntry +} from './claude-subagent-roster-state' /** Spawn-group rows kept live per session, and children per row. Both bound an * event-accumulated map that no provider snapshot ever prunes. */ @@ -31,30 +38,6 @@ const OUTSIDE_TURN = 'outside-turn' const UNLABELLED_AGENT = 'subagent' -type TrackedEntry = { - entry: NativeChatSubagentEntry - /** The only signal separating a child that dies with its turn from one told to - * outlive it. A turn-end sweep must leave a backgrounded child alone. */ - backgrounded: boolean - /** Label before its ordinal suffix, so a later announcement can tell a - * provisional row from one that already carries the provider's own name. */ - labelBase: string -} - -type RosterGroup = { - groupId: string - identity: AgentJournalItemIdentity - /** Insertion order is the display order; the map holds the state. */ - entries: Map - /** Every RENDERED label this group has handed out. Nothing releases one: - * re-issuing a label 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 set. */ - claimedLabels: Set - /** Last body written, so an idempotent replay writes no new revision. */ - lastSerialized: string | null -} - export type ClaudeSubagentRosterDeps = { sink: StructuredAgentSessionEventSink /** The turn that owns children spawned right now; null outside any turn. */ @@ -107,10 +90,20 @@ export class ClaudeSubagentRoster { (frame.toolUseId ? this.adopt(frame.toolUseId, frame.taskId) : null) if (!located) { if (frame.announcesSubagent) { - this.create(frame.taskId, frame.label, frame.state ?? 'working', frame.backgrounded) + this.create( + frame.taskId, + frame.label, + frame.state ?? 'working', + frame.backgrounded ?? false, + frame.toolUseId + ) } return true } + const tracked = located.group.entries.get(frame.taskId) + if (tracked && !applyClaudeSubagentInvocation(tracked, frame, this.now)) { + return true + } this.revise(located.group, frame.taskId, { label: frame.label, state: frame.state, @@ -146,7 +139,7 @@ export class ClaudeSubagentRoster { // enter under a looser rule. return } - this.create(canonical, null, 'working', false) + this.create(canonical, null, 'working', false, parentToolUseId) } /** @@ -158,7 +151,12 @@ export class ClaudeSubagentRoster { observeToolResult(toolUseId: string, failed: boolean): void { const canonical = this.ids.canonical(toolUseId) const located = this.locate(canonical) - if (!located || located.tracked.backgrounded) { + if ( + !located || + located.tracked.invocationIds === null || + located.tracked.backgrounded || + (located.tracked.toolUseId !== null && located.tracked.toolUseId !== toolUseId) + ) { return } this.revise(located.group, canonical, { @@ -176,9 +174,8 @@ export class ClaudeSubagentRoster { */ settleTurn(groupKey: string | null): void { // 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. + // unrelated turn ending is no evidence about a child announced outside it. + // `settleSession` reaches what no turn does. this.sweep(this.groups.get(groupKey ?? OUTSIDE_TURN), false) } @@ -228,20 +225,24 @@ export class ClaudeSubagentRoster { id: string, label: string | null, state: NativeChatSubagentEntry['state'], - backgrounded: boolean + backgrounded: boolean, + toolUseId: string | null ): void { const group = this.groupFor() - if (group.entries.size >= MAX_SUBAGENTS_PER_GROUP) { + if (group.admittedEntries >= MAX_SUBAGENTS_PER_GROUP) { return } + group.admittedEntries += 1 const now = this.now() const labelBase = label ?? UNLABELLED_AGENT group.entries.set(id, { backgrounded, + toolUseId, + invocationIds: new Set(toolUseId ? [toolUseId] : []), labelBase, entry: { id, - label: this.claimLabel(group, labelBase), + label: claimClaudeSubagentLabel(group, labelBase), state, startedAt: now, ...(isTerminalSubagentState(state) ? { settledAt: now } : {}) @@ -257,7 +258,7 @@ export class ClaudeSubagentRoster { change: { label: string | null state: NativeChatSubagentEntry['state'] | null - backgrounded: boolean + backgrounded: boolean | null } ): void { const tracked = group.entries.get(id) @@ -266,7 +267,7 @@ export class ClaudeSubagentRoster { } const next: TrackedEntry = { ...tracked, - backgrounded: tracked.backgrounded || change.backgrounded, + backgrounded: change.backgrounded ?? tracked.backgrounded, entry: { ...tracked.entry } } // A provisional row built from child traffic takes the real name the first @@ -277,11 +278,10 @@ export class ClaudeSubagentRoster { change.label !== UNLABELLED_AGENT ) { next.labelBase = change.label - next.entry.label = this.claimLabel(group, change.label) + next.entry.label = claimClaudeSubagentLabel(group, change.label) } - // Terminal latches: a duplicate or out-of-order frame must not resurrect a - // settled child, and re-applying a live state is a no-op. - if (change.state && !isTerminalSubagentState(tracked.entry.state)) { + // Proven outcomes latch; lost contact can still receive a later verdict. + if (change.state && canReplaceSubagentState(tracked.entry.state, change.state)) { next.entry.state = change.state if (isTerminalSubagentState(change.state)) { next.entry.settledAt = this.now() @@ -338,6 +338,7 @@ export class ClaudeSubagentRoster { groupId, identity: claudeSubagentGroupIdentity(groupId), entries: new Map(), + admittedEntries: 0, claimedLabels: new Set(), lastSerialized: null } @@ -359,19 +360,6 @@ export class ClaudeSubagentRoster { return group } - /** Two children can share a description; the ordinal keeps their rows apart - * without inventing a name the provider never sent. The probe is over the - * labels actually rendered, not a per-base counter: a generated `Audit 2` - * must not collide with a provider that names its own child `Audit 2`. */ - private claimLabel(group: RosterGroup, base: string): string { - let candidate = base - for (let ordinal = 2; group.claimedLabels.has(candidate); ordinal++) { - candidate = `${base} ${ordinal}` - } - group.claimedLabels.add(candidate) - return candidate - } - private write(group: RosterGroup): void { const agents = [...group.entries.values()].map((tracked) => tracked.entry) const options = { coalescingKey: `claude-subagents:${group.groupId}` } diff --git a/src/main/claude/claude-subagent-task-frames.test.ts b/src/main/claude/claude-subagent-task-frames.test.ts index e3d7f2f67cf..230ef45e19c 100644 --- a/src/main/claude/claude-subagent-task-frames.test.ts +++ b/src/main/claude/claude-subagent-task-frames.test.ts @@ -143,8 +143,8 @@ describe('readClaudeSubagentTaskFrame', () => { } }) - it('treats progress and notification as no lifecycle verdict', () => { - for (const subtype of ['task_progress', 'task_notification']) { + it('treats progress as no lifecycle verdict', () => { + for (const subtype of ['task_progress']) { expect( readClaudeSubagentTaskFrame( system(subtype, { task_id: 'task-1', status: 'completed', patch: { status: 'failed' } }) @@ -154,6 +154,20 @@ describe('readClaudeSubagentTaskFrame', () => { }) }) + it('reads the notification verdict from its top-level status', () => { + for (const state of ['completed', 'failed', 'stopped']) { + expect( + readClaudeSubagentTaskFrame( + system('task_notification', { + task_id: 'task-1', + status: state, + patch: { status: 'running' } + }) + ) + ).toMatchObject({ state }) + } + }) + it('reads the backgrounded flag from the frame or its patch', () => { expect( readClaudeSubagentTaskFrame( @@ -171,7 +185,7 @@ describe('readClaudeSubagentTaskFrame', () => { ).toMatchObject({ backgrounded: true }) expect( readClaudeSubagentTaskFrame(system('task_updated', { task_id: 'task-1', patch: {} })) - ).toMatchObject({ backgrounded: false }) + ).toMatchObject({ backgrounded: null }) }) it('collapses a multi-line description into one bounded label', () => { diff --git a/src/main/claude/claude-subagent-task-frames.ts b/src/main/claude/claude-subagent-task-frames.ts index 434b02a7405..e5f361fa8c4 100644 --- a/src/main/claude/claude-subagent-task-frames.ts +++ b/src/main/claude/claude-subagent-task-frames.ts @@ -10,7 +10,8 @@ import type { NativeChatSubagentState } from '../../shared/native-chat-types' import { classifyClaudeBackgroundTaskKind, claudeTaskDescription, - claudeTaskId + claudeTaskId, + isBoundedClaudeTaskId } from './claude-background-task-tracker' import { claudeRecord, claudeText } from './claude-structured-item-translation' @@ -43,7 +44,7 @@ export type ClaudeSubagentTaskFrame = { label: string | null /** null when the frame reported no lifecycle status. */ state: NativeChatSubagentState | null - backgrounded: boolean + backgrounded: boolean | null /** Any `task_started`, subagent or not. Proof this CLI declares its tasks. */ announcement: boolean /** `task_started` for a task the roster should show. Only an announcement @@ -89,25 +90,32 @@ export function readClaudeSubagentTaskFrame( return null } const patch = claudeRecord(message.patch) + const toolUseId = claudeText(message.tool_use_id) ?? claudeText(patch?.tool_use_id) const announcement = subtype === 'task_started' // Housekeeping Claude runs for itself; the user never asked for it. const suppressed = message.ambient === true || message.skip_transcript === true const subagent = announcement && !suppressed && isClaudeSubagentTask(message) return { taskId, - toolUseId: claudeText(message.tool_use_id) ?? claudeText(patch?.tool_use_id), + toolUseId: toolUseId && isBoundedClaudeTaskId(toolUseId) ? toolUseId : null, label: claudeTaskDescription(message.description) ?? claudeTaskDescription(patch?.description) ?? // Bounded like a description: the roster stores whatever this returns. (announcement ? (claudeTaskDescription(message.subagent_type) ?? null) : null), - // A notification or progress ping is not a lifecycle verdict: latching one - // terminal would settle a child that is still running. + // Notifications carry terminal evidence; progress carries usage only. state: - announcement || subtype === 'task_updated' - ? taskState(patch?.status ?? message.status) - : null, - backgrounded: message.is_backgrounded === true || patch?.is_backgrounded === true, + subtype === 'task_notification' + ? taskState(message.status) + : announcement || subtype === 'task_updated' + ? taskState(patch?.status ?? message.status) + : null, + backgrounded: + typeof patch?.is_backgrounded === 'boolean' + ? patch.is_backgrounded + : typeof message.is_backgrounded === 'boolean' + ? message.is_backgrounded + : null, announcement, announcesSubagent: subagent, excluded: announcement && !subagent