diff --git a/src/cli/handlers/orchestration/worker-output.test.ts b/src/cli/handlers/orchestration/worker-output.test.ts index 02c1dcbd6d2..895e9909d05 100644 --- a/src/cli/handlers/orchestration/worker-output.test.ts +++ b/src/cli/handlers/orchestration/worker-output.test.ts @@ -396,6 +396,52 @@ describe('formatWorkerRead', () => { expect(output).toContain(`[subagents] ${subagentGroupFallbackText(other)}`) }) + // Which group a lone twin belongs to is decided by its TEXT, not its position. + // Claiming positionally silenced whichever group came first, so a twin + // belonging to a LATER group erased the earlier group's roster and printed the + // later one's sentence twice — the same silent drop, one permutation over. + it('claims a lone twin for the group it names, not the first group in the message', () => { + const other: readonly NativeChatSubagentEntry[] = [ + { id: 'child-3', label: 'plan', state: 'completed' } + ] + const second = subagentGroupFallbackText(other) + const output = formatWorkerRead( + transcriptRead( + [ + { type: 'text', text: second }, + { type: 'subagent-group', groupId: 'thread:turn-1', agents: [...ROSTER] }, + { type: 'subagent-group', groupId: 'thread:turn-2', agents: [...other] } + ], + 'system' + ) + ) + + expect(occurrences(output, second)).toBe(1) + expect(output).toContain(`[subagents] ${subagentGroupFallbackText(ROSTER)}`) + }) + + // The same claim, with the twin written after both blocks: nothing about the + // ORDER of a twin and its group is guaranteed by the block schema. + it('claims a trailing twin for the group it names', () => { + const other: readonly NativeChatSubagentEntry[] = [ + { id: 'child-3', label: 'plan', state: 'completed' } + ] + const second = subagentGroupFallbackText(other) + const output = formatWorkerRead( + transcriptRead( + [ + { type: 'subagent-group', groupId: 'thread:turn-1', agents: [...ROSTER] }, + { type: 'subagent-group', groupId: 'thread:turn-2', agents: [...other] }, + { type: 'text', text: second } + ], + 'system' + ) + ) + + expect(occurrences(output, second)).toBe(1) + expect(output).toContain(`[subagents] ${subagentGroupFallbackText(ROSTER)}`) + }) + // A group with no twin beside it is a shape the block schema admits and no // producer writes. Dropping it would lose the roster entirely, so the block // itself carries the sentence when nothing else does. diff --git a/src/main/codex/codex-subagent-roster.test.ts b/src/main/codex/codex-subagent-roster.test.ts index d267773c674..9b48080dc5f 100644 --- a/src/main/codex/codex-subagent-roster.test.ts +++ b/src/main/codex/codex-subagent-roster.test.ts @@ -485,14 +485,59 @@ describe('CodexSubagentRoster', () => { ) const entry = agents()[0] - expect(entry?.label.length).toBe(MAX_SUBAGENT_FIELD_CHARS) - expect(entry?.label.endsWith('…')).toBe(true) - expect(entry?.id.length).toBe(MAX_SUBAGENT_FIELD_CHARS) - expect(entry?.id.endsWith('…')).toBe(true) + expect(entry?.label.length).toBeLessThanOrEqual(MAX_SUBAGENT_FIELD_CHARS) + expect(entry?.label).toMatch(/…~0$/) + expect(entry?.id.length).toBeLessThanOrEqual(MAX_SUBAGENT_FIELD_CHARS) + expect(entry?.id).toMatch(/…~0$/) expect(JSON.stringify(latest()?.body)).not.toContain('output truncated') expect(isAdmissibleAgentJournalItemBody(latest()?.body)).toBe(true) }) + // The clip cuts UTF-16 code units, so a boundary landing inside a surrogate + // pair left a LONE high surrogate in a durable row — malformed, and replaced + // with U+FFFD through any non-JSON UTF-8 hop. + it('never clips a provider string mid surrogate pair', () => { + const { roster, agents } = createHarness() + const astral = '😀'.repeat(400) + + deliver( + roster, + activity({ kind: 'started', agentThreadId: astral, agentPath: `/root/${astral}` }) + ) + + const entry = agents()[0] + expect(entry?.id.length).toBeLessThanOrEqual(MAX_SUBAGENT_FIELD_CHARS) + expect(Buffer.from(entry?.id ?? '', 'utf8').toString('utf8')).toBe(entry?.id) + expect(Buffer.from(entry?.label ?? '', 'utf8').toString('utf8')).toBe(entry?.label) + }) + + // The clip removes exactly the tail that told two children apart: `id` is the + // renderer's React key, and `claimLabel` writes its repeat ordinal at the end. + // Two clipped children collapsing to one key drew two rows under one identity. + it('keeps clipped ids and labels distinct between children', () => { + const { roster, agents } = createHarness() + const prefix = 'p'.repeat(MAX_SUBAGENT_FIELD_CHARS) + const sharedPath = `/root/${'q'.repeat(640)}` + + deliver( + roster, + activity({ kind: 'started', agentThreadId: `${prefix}AAAA`, agentPath: sharedPath }) + ) + deliver( + roster, + activity({ kind: 'started', agentThreadId: `${prefix}BBBB`, agentPath: sharedPath }) + ) + + const entries = agents() + expect(entries).toHaveLength(2) + expect(new Set(entries.map((agent) => agent.id)).size).toBe(2) + expect(new Set(entries.map((agent) => agent.label)).size).toBe(2) + for (const agent of entries) { + expect(agent.id.length).toBeLessThanOrEqual(MAX_SUBAGENT_FIELD_CHARS) + expect(agent.label.length).toBeLessThanOrEqual(MAX_SUBAGENT_FIELD_CHARS) + } + }) + it('caps the children one spawn group admits', () => { const { roster, agents, appended } = createHarness() for (let index = 0; index < MAX_CODEX_SUBAGENTS_PER_GROUP; index++) { diff --git a/src/main/codex/codex-subagent-roster.ts b/src/main/codex/codex-subagent-roster.ts index 1c5360f81e9..932cb5eb551 100644 --- a/src/main/codex/codex-subagent-roster.ts +++ b/src/main/codex/codex-subagent-roster.ts @@ -315,10 +315,10 @@ export function codexSubagentGroupBody( groupId: string, agents: readonly NativeChatSubagentEntry[] ): AgentJournalItemBody { - const bounded = agents.map((agent) => ({ + const bounded = agents.map((agent, index) => ({ ...agent, - id: boundSubagentField(agent.id), - label: boundSubagentField(agent.label) + id: boundSubagentField(agent.id, index), + label: boundSubagentField(agent.label, index) })) return { kind: 'message', @@ -333,9 +333,22 @@ export function codexSubagentGroupBody( /** `id` and `label` are provider strings, so they take the bound both readers of * this row already clip them to. A plain length check, not the tool-output * bound: that one digests the whole value before it checks the length, and this - * runs twice per child on every streamed token-usage frame. */ -function boundSubagentField(value: string): string { - return value.length <= MAX_SUBAGENT_FIELD_CHARS - ? value - : `${value.slice(0, MAX_SUBAGENT_FIELD_CHARS - 1)}…` + * runs twice per child on every streamed token-usage frame. + * + * A clip is not identity-preserving, so a clipped value carries the child's + * index: two ids sharing a long prefix collapse to one React key, and + * `claimLabel` writes its ordinal at the very tail the clip removes. The index + * is reserved out of the bound, not appended to it, because both readers + * re-clip to the same cap and would cut a suffix that overflowed it. */ +function boundSubagentField(value: string, index: number): string { + if (value.length <= MAX_SUBAGENT_FIELD_CHARS) { + return value + } + const suffix = `…~${index}` + const keep = MAX_SUBAGENT_FIELD_CHARS - suffix.length + // Slicing UTF-16 units can split a surrogate pair; a lone surrogate is + // malformed in a durable row and lossy through any non-JSON UTF-8 hop. + const last = value.charCodeAt(keep - 1) + const end = last >= 0xd800 && last <= 0xdbff ? keep - 1 : keep + return `${value.slice(0, end)}${suffix}` } diff --git a/src/main/native-chat/agent-session-wire/provider-frame-disposition.ts b/src/main/native-chat/agent-session-wire/provider-frame-disposition.ts index 7737db6358e..35223a1971f 100644 --- a/src/main/native-chat/agent-session-wire/provider-frame-disposition.ts +++ b/src/main/native-chat/agent-session-wire/provider-frame-disposition.ts @@ -1,4 +1,5 @@ import type { CodexAppServerNotificationMethod } from '../../codex/codex-app-server-notification-schema' +import { CODEX_SUBAGENT_ITEM_TYPE } from '../../codex/codex-subagent-activity' import type { ClaudeStreamJsonFrameKind } from './claude-stream-json-frame-schema' export type ProviderFrameClassification = @@ -208,7 +209,7 @@ const CODEX_ITEM_CLASSIFICATIONS: Record = // guarantees a session reports subagent work as `subAgentActivity` at all; one // that only ever emits the collab tool call gets no roster row, and suppressing // that too would leave its fan-out showing nothing. - subAgentActivity: 'status-chrome', + [CODEX_SUBAGENT_ITEM_TYPE]: 'status-chrome', // `{id, durationMs}` and nothing else — Codex's own transcript renders it as // nothing at all. Every other item type this build does not model carries text // a user would want (review output, an image path, hook prompt text), so those diff --git a/src/main/runtime/orchestration/worker-transcript-payload.ts b/src/main/runtime/orchestration/worker-transcript-payload.ts index 271c0bfa565..f9d83e20c62 100644 --- a/src/main/runtime/orchestration/worker-transcript-payload.ts +++ b/src/main/runtime/orchestration/worker-transcript-payload.ts @@ -1,8 +1,5 @@ import { createHash } from 'node:crypto' -import { - MAX_SUBAGENT_FIELD_CHARS, - normalizeSubagentState -} from '../../../shared/native-chat-subagent-summary' +import { normalizeSubagentState } from '../../../shared/native-chat-subagent-summary' import type { NativeChatBlock, NativeChatMessage, @@ -19,9 +16,10 @@ const MAX_WORKER_TRANSCRIPT_INPUT_NODES = 100 // here. The bound stays because the journal schema declares no maximum and a // remote host may run a build with a larger one. const MAX_WORKER_TRANSCRIPT_SUBAGENTS = 64 -// Roster ids and labels arrive already bounded to this; every other piece of -// transcript metadata takes the same one. -const MAX_WORKER_TRANSCRIPT_METADATA_CHARS = MAX_SUBAGENT_FIELD_CHARS +// Message ids, turn ids, tool-call names and image urls, not only roster fields. +// Equal to `MAX_SUBAGENT_FIELD_CHARS` today, kept a separate literal so a +// roster-motivated change to that cap cannot silently move this one. +const MAX_WORKER_TRANSCRIPT_METADATA_CHARS = 512 const MAX_WORKER_TRANSCRIPT_RESPONSE_BYTES = 512 * 1024 const TRUNCATION_MARKER = '\n… (truncated)' const DISPATCH_CAPABILITY_PATTERN = /\bdcap_[A-Za-z0-9_-]{20,}\b/g diff --git a/src/shared/native-chat-subagent-summary.ts b/src/shared/native-chat-subagent-summary.ts index 8742dfe4a4f..c81d28e5c20 100644 --- a/src/shared/native-chat-subagent-summary.ts +++ b/src/shared/native-chat-subagent-summary.ts @@ -43,10 +43,15 @@ export function normalizeSubagentState(state: string): NativeChatSubagentState { return TERMINAL_SUBAGENT_STATES.has(state) ? (state as NativeChatSubagentState) : 'unverifiable' } -/** Bound on the provider strings a roster row carries — `id`, `label`, - * `groupId`. One constant because the producer writes a durable row and both +/** Bound on the per-child provider strings a roster row carries — `id` and + * `label`. One constant because the producer writes a durable row and both * readers clip it again: a larger producer bound is bytes every consumer throws - * away, replayed on every reconnect. */ + * away, replayed on every reconnect. + * + * `groupId` is deliberately NOT bounded by the producer: the row's durable + * identity is `codex-subagents:${groupId}` and cannot be clipped without + * changing which row a replay finds, so bounding only the block field would + * save nothing and make the two disagree. Both readers still clip it. */ export const MAX_SUBAGENT_FIELD_CHARS = 512 export function isTerminalSubagentState(state: string): boolean { diff --git a/src/shared/worker-transcript-text.ts b/src/shared/worker-transcript-text.ts index 618c19254a6..ce57d09d3b4 100644 --- a/src/shared/worker-transcript-text.ts +++ b/src/shared/worker-transcript-text.ts @@ -15,15 +15,12 @@ import type { NativeChatMessage } from './native-chat-types' export function formatWorkerTranscriptMessage(message: NativeChatMessage): string { // Every roster block is written beside a plain-text twin carrying the same // sentence, for clients that cannot draw the block. Text surfaces are those - // clients, so they print the twin and drop the block — the mirror of the - // renderer, which draws the block and drops the twin. Either way the sentence - // prints once. - // Counted, not a boolean: a message carrying two roster blocks and one twin - // suppressed BOTH groups and printed one sentence, losing a roster silently. - let unclaimedTwins = message.blocks.filter( - (block) => block.type === 'text' && isSubagentGroupFallbackText(block.text) - ).length - const blocks = message.blocks.map((block) => { + // clients, so they print the twin and drop the block. The renderer reaches the + // same single print from the other side but not by the same rule: it drops + // every fallback-shaped text block as soon as any group is present and draws + // each group, so it never has to decide which twin belongs to which group. + const standIns = claimSubagentGroupTwins(message.blocks) + const blocks = message.blocks.map((block, index) => { if (block.type === 'text') { return block.text } @@ -37,17 +34,7 @@ export function formatWorkerTranscriptMessage(message: NativeChatMessage): strin return block.url ? `[image] ${block.url}` : `[image omitted]` } if (block.type === 'subagent-group') { - // Stand in for the block only when no twin is left to print it: the wire - // admits a roster that arrived without one, and dropping that - // unconditionally would lose the sentence altogether. Counted, not a - // byte compare against a recomputed sentence — a roster from a newer - // build holds a state this build reads as `unverifiable`, so recomputing - // yields a different sentence and both would print. - if (unclaimedTwins > 0) { - unclaimedTwins -= 1 - return null - } - return `[subagents] ${subagentGroupFallbackText(block.agents)}` + return standIns.get(index) ?? null } // The journal deliberately admits block types this build does not know, and // a newer remote host can send one over the wire. Degrade to a marker rather @@ -57,6 +44,46 @@ export function formatWorkerTranscriptMessage(message: NativeChatMessage): strin return `[${message.role}] ${blocks.filter((line) => line !== null).join('\n')}`.trimEnd() } +/** For each roster block, the sentence it must print itself — absent when a twin + * beside it already prints one. + * + * Exact-text claims are settled for EVERY group before any leftover twin is + * claimed by position: claiming in block order let an earlier group consume a + * later group's twin, silencing the earlier roster while the later one printed + * twice. The positional fallback stays because a roster written by a newer build + * holds a state this build reads as `unverifiable`, so its frozen twin can never + * equal the sentence recomputed here and a text match alone would print it + * twice. A group left with no twin prints its own: the wire admits a roster that + * arrived without one, and dropping that would lose the sentence altogether. */ +function claimSubagentGroupTwins(blocks: NativeChatMessage['blocks']): Map { + const twins: string[] = [] + const groups: { index: number; sentence: string }[] = [] + blocks.forEach((block, index) => { + if (block.type === 'text' && isSubagentGroupFallbackText(block.text)) { + twins.push(block.text) + } else if (block.type === 'subagent-group') { + groups.push({ index, sentence: subagentGroupFallbackText(block.agents) }) + } + }) + const standIns = new Map() + const unclaimed = groups.filter((group) => { + const exact = twins.indexOf(group.sentence) + if (exact === -1) { + return true + } + twins.splice(exact, 1) + return false + }) + for (const group of unclaimed) { + if (twins.length > 0) { + twins.pop() + continue + } + standIns.set(group.index, `[subagents] ${group.sentence}`) + } + return standIns +} + function safeJson(value: unknown): string { try { return JSON.stringify(value)