mirror of
https://github.com/stablyai/orca.git
synced 2026-10-09 00:02:39 +00:00
fix(native-chat): loop-3 fixes to the Codex subagent worklog
Five defects loop 2's own fixes introduced. Twin claiming was order-blind: the count-based claim silenced whichever roster block came first, so a lone twin belonging to a LATER group erased an earlier group's roster and printed the later sentence twice. Exact-text claims are now settled for every group before any leftover twin is claimed by position; the positional fallback stays for a newer build's frozen twin, which can never equal a recomputed sentence. `boundSubagentField` sliced UTF-16 units and could leave a lone high surrogate in a durable row, and the clip removed exactly the tail that told two children apart — `id` is the renderer's React key and `claimLabel` writes its repeat ordinal at the end. It now backs off a split pair and reserves the child index inside the bound, so both readers' re-clip cannot cut the disambiguator off again. `MAX_SUBAGENT_FIELD_CHARS`'s doc claimed a `groupId` bound the producer never applies; the doc now says so and why. The worker-transcript metadata cap is a separate literal again: it governs message ids, turn ids, tool-call names and image urls, so a roster-motivated change must not move it.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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++) {
|
||||
|
||||
@@ -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}`
|
||||
}
|
||||
|
||||
@@ -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<string, ProviderFrameClassification> =
|
||||
// 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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<number, string> {
|
||||
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<number, string>()
|
||||
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)
|
||||
|
||||
Reference in New Issue
Block a user