From a0d8f33b29234f65468b90b9ff0128656c84fc3b Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 7 Jul 2026 00:51:55 +0800 Subject: [PATCH] Address review: structure-preserving delta scrub, live header-uniformity guard, RFC style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review findings on the pinned-header change: 1. scrubRequestHeaders flattened a request/header-delta's whole system/tools payload to one token, so two meaningfully different deltas compared equal. Now the structural facts survive — keepStart/keepEnd line positions, added/removed/changed tool NAMES — and only the bulk (inserted prompt lines, schema bodies) is tokenized. 2. The one-pin design rested on an unasserted premise (all sessions compose the same header). Every non-pinning scenario now asserts, live, that each request/header its run produces equals the pinned fixture's header (both sides normalized against their own volatile values), so a session-dependent header fails loud until it gets its own pin. Verified the guard bites: perturbing the pinned fixture's prompt fails a non-pinned scenario with the intended message. 3. RFC de-slopped per docs/AGENTS.md: no PR reference, no SHOULD spec-speak; Decision/Verification/Consequences updated for 1 and 2. --- ...-request-header-content-in-one-scenario.md | 12 ++-- examples/acp-agent/tests/acp.snapshot.ts | 58 +++++++++++++++++-- .../tests/snapshot-normalize.spec.ts | 36 ++++++++++-- .../acp-agent/tests/snapshot-normalize.ts | 43 ++++++++++---- 4 files changed, 120 insertions(+), 29 deletions(-) diff --git a/docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md b/docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md index e742a4d0db..c833425145 100644 --- a/docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md +++ b/docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md @@ -4,17 +4,17 @@ Status: implemented ## Problem -Every model-driving ACP snapshot fixture (`session.jsonl`) embedded the full composed system prompt and the complete tool-schema list in its `request/header` event — roughly 8 KB on one line, per fixture. That content is identical across the suite (byte-identical tool list everywhere, including subagent children; identical prompt modulo each recording's temp cwd), so any PR touching a tool description or a system-prompt line had to update every fixture: re-record everything against the live API (churning model responses and stdout goldens along the way) or hand-edit ~35 giant header lines. The dynamic-workflows PR (#170) is the canonical example — adding one tool and one prompt paragraph rewrote every snapshot fixture in the repo, burying the behavioral diff a reviewer should be reading. +Every model-driving ACP snapshot fixture (`session.jsonl`) embedded the full composed system prompt and the complete tool-schema list in its `request/header` event — roughly 8 KB on one line, per fixture. That content is identical across the suite (byte-identical tool list everywhere, including subagent children; identical prompt modulo each recording's temp cwd), so any change touching a tool description or a system-prompt line had to update every fixture: re-record everything against the live API (churning model responses and stdout goldens along the way) or hand-edit ~35 giant header lines. Introducing the dynamic-workflows feature — one new tool plus one prompt paragraph — rewrote every snapshot fixture in the repo, burying the behavioral diff a reviewer should be reading. ## Decision -Exactly one scenario — `text-turn`, flagged `pinsHeader` in `acp.snapshot.ts` — commits and compares the full request-header content. Every other fixture stores and compares that content as stable tokens: a `request/header` event's `header.system` becomes `"{{system}}"` and `header.tools` becomes `"{{tools}}"`, and a `request/header-delta`'s `system`/`tools` payloads likewise (a delta embeds prompt/schema fragments, so leaving it raw would reopen the churn). The scrub is a pure normalizer, `scrubRequestHeaders` in `snapshot-normalize.ts`, composed in front of `normalizeSessionLog` on BOTH sides of a non-pinning scenario's log compare and applied to the harvested logs record mode writes, so a re-record cannot smuggle the content back. Absent fields stay absent — WHETHER a header carried a prompt or tools is behavior and stays visible — and `config`/`reason` stay verbatim: a model swap SHOULD churn every fixture (it invalidates the recorded responses), while a prompt or schema edit does not (replay derives model behavior exclusively from `assistant/chunk` events and never reads header content — see `dsh-llm-replay`). +Exactly one scenario — `text-turn`, flagged `pinsHeader` in `acp.snapshot.ts` — commits and compares the full request-header content. Every other fixture stores and compares that content as stable tokens via the pure normalizer `scrubRequestHeaders` in `snapshot-normalize.ts`: a `request/header` event's `header.system` becomes `"{{system}}"` and `header.tools` becomes `"{{tools}}"`; a `request/header-delta` keeps its structural facts — the system delta's `keepStart`/`keepEnd` line positions, the tools delta's added/removed/changed tool names — and tokenizes only the bulk (inserted prompt lines, schema bodies), so two different deltas still compare different. The scrub is composed in front of `normalizeSessionLog` on BOTH sides of a non-pinning scenario's log compare and applied to the harvested logs record mode writes, so a re-record cannot smuggle the content back. Absent fields stay absent — WHETHER a header carried a prompt or tools is behavior and stays visible — and `config`/`reason` stay verbatim: a model swap churns every fixture by design (it invalidates the recorded responses), while a prompt or schema edit churns none of them (replay derives model behavior exclusively from `assistant/chunk` events and never reads header content — see `dsh-llm-replay`). A system-prompt or tool-schema change therefore lands as exactly one committed-fixture diff — the pinned `text-turn` header line — updated by hand or by re-recording that one scenario (`pnpm run test:snapshot:record` with `-t text-turn`). -Guards in the fixture meta-tests make the split self-enforcing: every non-pinning `session*.jsonl` must be a fixed point of `scrubRequestHeaders` (unscrubbed content crept in — apply the scrub), the pinning scenario's fixture must NOT be one (the pin lost its content), and exactly one scenario must pin. +Guards make the split self-enforcing. On disk (fixture meta-tests): every non-pinning `session*.jsonl` must be a fixed point of `scrubRequestHeaders` (unscrubbed content crept in — apply the scrub), the pinning scenario's fixture must NOT be one (the pin lost its content), and exactly one scenario must pin. Live (every non-pinning scenario run): each `request/header` the run produces — parent, spawn child, fork child, initial or resume — must equal the pinned fixture's header after both sides normalize their own volatile values, so the single-pin premise is asserted rather than assumed. -Today one pin covers the whole suite because every fixture — parent, spawn child, fork child — records the identical tool list and the identical prompt modulo cwd. If header composition ever becomes session-dependent (a restricted subagent toolset, say), the flag extends to one pinning scenario per distinct header shape. +One pin covers the whole suite because every session — parent, spawn child, fork child — composes the identical tool list and the identical prompt modulo cwd, and the uniformity guard fails the suite the moment that stops holding. If header composition ever becomes session-dependent by design (a restricted subagent toolset, say), the divergent shape gets its own pinning scenario. ## Alternatives considered @@ -25,8 +25,8 @@ Today one pin covers the whole suite because every fixture — parent, spawn chi ## Verification -All 37 snapshot scenarios replay green with the scrubbed fixtures (the committed fixtures were rewritten once through `scrubRequestHeaders` itself; `text-turn` untouched). The fixed-point, pin-retains-content, and exactly-one-pin guards run inside the suite, and `scrubRequestHeaders` has unit coverage for both header event types, absent-field preservation, config/reason retention, byte-for-byte pass-through of other lines, and idempotence. +All 37 snapshot scenarios replay green with the scrubbed fixtures (the committed fixtures were rewritten once through `scrubRequestHeaders` itself; `text-turn` untouched). The fixed-point, pin-retains-content, exactly-one-pin, and live header-uniformity guards run inside the suite, and `scrubRequestHeaders` has unit coverage for both header event types, delta structure preservation (line positions, tool names), absent-field preservation, config/reason retention, byte-for-byte pass-through of other lines, and idempotence. ## Consequences -A tool-description or system-prompt change churns one committed fixture line instead of every fixture in the suite, so snapshot diffs read as behavior again, and ~270 KB of duplicated header bytes leave the repo. The cost: fixtures no longer show per-scenario header content, so a non-uniform header would be invisible outside the pinned scenario — accepted while composition is provably suite-uniform (the recording is made by one `cordis.yml` for all scenarios), and revisited with additional pins if that ever changes. +A tool-description or system-prompt change churns one committed fixture line instead of every fixture in the suite, so snapshot diffs read as behavior again, and ~270 KB of duplicated header bytes leave the repo. The cost: non-pinning fixtures no longer display header content, so reading one shows tokens where the prompt and schemas were — the pinned `text-turn` fixture is the place to look, and the live uniformity guard guarantees it speaks for every session in the suite. A header change surfaces as a suite-wide test failure whose fix is the one pinned line, rather than as ~35 fixture rewrites. diff --git a/examples/acp-agent/tests/acp.snapshot.ts b/examples/acp-agent/tests/acp.snapshot.ts index 8ec0cef3c3..977615f841 100644 --- a/examples/acp-agent/tests/acp.snapshot.ts +++ b/examples/acp-agent/tests/acp.snapshot.ts @@ -20,7 +20,9 @@ import { type NormalizeContext, normalizeSessionLog, normalizeStdout, scrubReque * `request/header` events) is pinned by exactly ONE scenario — the one with * `pinsHeader` — and scrubbed to `{{system}}`/`{{tools}}` tokens in every * other fixture and compare, so a prompt or tool-schema edit churns one - * committed line instead of every fixture (see the pinned-header RFC, + * committed line instead of every fixture. A per-run uniformity guard keeps + * the single pin sound: every live header must equal the pinned one (see the + * pinned-header RFC, * docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md). * * `pnpm run test:snapshot:record` (DSH_SNAPSHOT=record + -u) re-records the @@ -69,11 +71,13 @@ interface Scenario { * scenario pins it; every other scenario stores and compares that content as * `{{system}}`/`{{tools}}` tokens ({@link scrubRequestHeaders}), so a system * prompt or tool-schema change shows up as ONE committed-fixture diff, not - * one per scenario. Today every fixture (parent, spawn child, fork child) - * carries the identical prompt-modulo-cwd and identical tools, so one pin - * covers the whole suite; if header composition ever becomes - * session-dependent (say, a restricted subagent toolset), pin one scenario - * per distinct header shape. Defaults to false. + * one per scenario. One pin suffices because header composition is + * suite-uniform (parent, spawn child, and fork child all compose the same + * prompt-modulo-cwd and the same tools) — and that premise is ASSERTED, not + * assumed: every non-pinning run's live headers must equal the pinned + * fixture's (normalized), so a session-dependent header (say, a restricted + * subagent toolset) fails loud until it gets its own pinning scenario. + * Defaults to false. */ pinsHeader?: boolean } @@ -143,6 +147,10 @@ const SCENARIOS: Scenario[] = [ { name: 'hook-codex-stop-continue', hasModelTurn: true, recorded: true }, ] +/** The single header-pinning scenario. Guarded here (and by a meta-test) so the pin cannot silently vanish. */ +const pinningScenario = SCENARIOS.find(s => s.pinsHeader === true) +if (pinningScenario === undefined) throw new Error('acp.snapshot: no scenario pins the request-header content') + /** The sibling child-fixture paths for a scenario (`session.1.jsonl` …). */ function childFixturePaths(dir: string, childSessions: number): string[] { return Array.from({ length: childSessions }, (_, i) => join(dir, `session.${i + 1}.jsonl`)) @@ -170,6 +178,22 @@ function fixtureContext(fixture: string): NormalizeContext { } } +/** + * The `data.header` payload of every `request/header` event in a session + * JSONL, in log order, with the log's volatile values scrubbed first + * ({@link normalizeSessionLog}) so headers harvested from different runs — + * each embedding its own temp cwd in the composed prompt — compare on equal + * footing. + */ +function normalizedHeaders(rawLog: string, ctx: NormalizeContext): unknown[] { + return normalizeSessionLog(rawLog, ctx) + .split('\n') + .filter(line => line.trim().length > 0) + .map(line => JSON.parse(line) as { type?: unknown; data?: { header?: unknown } }) + .filter(record => record.type === 'request/header') + .map(record => record.data?.header) +} + for (const scenario of SCENARIOS) { describe(`snapshot: ${scenario.name}`, () => { // In RECORD mode, only re-run the `recorded` (live-API) scenarios; the @@ -247,6 +271,28 @@ for (const scenario of SCENARIOS) { .toEqual(normalizeSessionLog(fixture, fixtureContext(fixture))) } } + + // Header-uniformity guard: the single pin is sound only while every + // session in the suite composes the SAME header. Assert it live — every + // request/header the run produced (parent, spawn child, fork child, + // initial or resume) must equal the pinned fixture's header after each + // side is normalized against its own volatile values. If this fails, + // either the header changed (update the pin: re-record or hand-edit the + // pinning scenario's fixture) or composition became session-dependent + // by design (give the divergent shape its own pinning scenario). + if (scenario.pinsHeader !== true) { + const pinnedFixture = await readFile(join(SNAPSHOTS_DIR, pinningScenario.name, 'session.jsonl'), 'utf8') + const pinned = normalizedHeaders(pinnedFixture, fixtureContext(pinnedFixture)) + expect(pinned.length, `the pinning fixture (${pinningScenario.name}) must carry exactly one request/header`) + .toBe(1) + for (const log of result.sessionLogs) { + const headers = normalizedHeaders(log.content, ctx) + for (const [k, header] of headers.entries()) { + expect(header, `session ${log.id}: request/header #${k + 1} diverged from the pinned (${pinningScenario.name}) header`) + .toEqual(pinned[0]) + } + } + } }) }) } diff --git a/examples/acp-agent/tests/snapshot-normalize.spec.ts b/examples/acp-agent/tests/snapshot-normalize.spec.ts index 5c97c7fe5d..e3e7eb2616 100644 --- a/examples/acp-agent/tests/snapshot-normalize.spec.ts +++ b/examples/acp-agent/tests/snapshot-normalize.spec.ts @@ -135,24 +135,50 @@ describe('scrubRequestHeaders', () => { expect(out).not.toContain('{{tools}}') }) - it('scrubs a request/header-delta system/tools payload', () => { + it('scrubs a header-delta system payload but keeps its line positions', () => { const delta = JSON.stringify({ type: 'request/header-delta', seq: 8, time: 9, - data: { system: { keepStart: 1, keepEnd: 0, insert: ['leaked prompt line'] }, config: { model: 'm2' } }, + data: { system: { keepStart: 1, keepEnd: 4, insert: ['leaked prompt line'] }, config: { model: 'm2' } }, }) const out = scrubRequestHeaders(`${headerLine}\n${delta}\n`) - expect(out).toContain('"system":"{{system}}"') + expect(out).toContain('"insert":"{{system}}"') + expect(out).toContain('"keepStart":1') + expect(out).toContain('"keepEnd":4') expect(out).toContain('"config":{"model":"m2"}') expect(out).not.toContain('leaked prompt line') expect(out).not.toContain('{{tools}}') // no tools delta → none invented }) + it('scrubs a header-delta tools payload but keeps the added/removed/changed names', () => { + const delta = JSON.stringify({ + type: 'request/header-delta', seq: 8, time: 9, + data: { + tools: { + added: [{ name: 'grep', description: 'Search files.', parameters: { type: 'object' } }], + removed: ['bash_kill'], + changed: [{ name: 'read', description: 'Read v2.', parameters: { type: 'object' } }], + }, + }, + }) + const out = scrubRequestHeaders(`${headerLine}\n${delta}\n`) + // WHICH tools changed is behavior and survives; their bulk does not. + expect(out).toContain('"added":[{"name":"grep","description":"{{tools}}","parameters":"{{tools}}"}]') + expect(out).toContain('"removed":["bash_kill"]') + expect(out).toContain('"changed":[{"name":"read","description":"{{tools}}","parameters":"{{tools}}"}]') + expect(out).not.toContain('Search files') + expect(out).not.toContain('Read v2') + }) + it('passes every other line through byte-for-byte and is idempotent', () => { const other = JSON.stringify({ type: 'assistant/chunk', seq: 4, time: 9, data: { turn: 1, step: 1, chunk: { type: 'text-delta', index: 0, text: 'hi' } } }) - const raw = `${headerLine}\n${headerEvent({ config: { model: 'm' }, system: 's', tools: [] })}\n${other}\n` + const delta = JSON.stringify({ + type: 'request/header-delta', seq: 8, time: 9, + data: { system: { keepStart: 0, keepEnd: 0, insert: ['x'] }, tools: { added: [{ name: 't', description: 'd', parameters: {} }], removed: [], changed: [] } }, + }) + const raw = `${headerLine}\n${headerEvent({ config: { model: 'm' }, system: 's', tools: [] })}\n${delta}\n${other}\n` const once = scrubRequestHeaders(raw) expect(once.split('\n')[0]).toBe(headerLine) - expect(once.split('\n')[2]).toBe(other) + expect(once.split('\n')[3]).toBe(other) expect(scrubRequestHeaders(once)).toBe(once) }) }) diff --git a/examples/acp-agent/tests/snapshot-normalize.ts b/examples/acp-agent/tests/snapshot-normalize.ts index 1ca78d0287..163bf2bc93 100644 --- a/examples/acp-agent/tests/snapshot-normalize.ts +++ b/examples/acp-agent/tests/snapshot-normalize.ts @@ -122,16 +122,19 @@ export function normalizeSessionLog(rawLog: string, ctx: NormalizeContext): stri } /** - * Replace request-header CONTENT in a session JSONL with stable tokens: a - * `request/header` event's `data.header.system` → `{{system}}` and - * `data.header.tools` → `{{tools}}`, and a `request/header-delta` event's - * `data.system`/`data.tools` payloads likewise (a delta embeds prompt/schema - * fragments, so leaving it raw would reopen the churn this scrub closes). + * Replace request-header CONTENT in a session JSONL with stable tokens, + * keeping its structure: a `request/header` event's `data.header.system` → + * `{{system}}` and `data.header.tools` → `{{tools}}`; a + * `request/header-delta` event keeps every structural fact — the system + * delta's `keepStart`/`keepEnd` line positions, the tools delta's + * added/removed/changed tool NAMES — and tokenizes only the bulk (inserted + * prompt lines → `{{system}}`; each added/changed schema's fields other than + * `name` → `{{tools}}`), so two different deltas still compare different. * Absent fields stay absent — WHETHER a header carried a system prompt or * tools is behavior and stays visible; `config` and `reason` are small and - * stable, so they stay verbatim (a model swap SHOULD churn every fixture — it - * invalidates the recorded responses; a prompt/schema edit does NOT — replay - * never reads this content, see dsh-llm-replay). + * stable, so they stay verbatim (a model swap churns every fixture by design + * — it invalidates the recorded responses; a prompt/schema edit churns none — + * replay never reads this content, see dsh-llm-replay). * * Only lines with something to scrub are re-serialized; every other line * passes through byte-for-byte, so the transform is idempotent and applying @@ -154,12 +157,28 @@ export function scrubRequestHeaders(rawLog: string): string { return JSON.stringify(record) } if (record.type === 'request/header-delta') { - if (!('system' in data) && !('tools' in data)) return line - if ('system' in data) data.system = SYSTEM - if ('tools' in data) data.tools = TOOLS - return JSON.stringify(record) + let touched = false + const system = data.system as Record | null | undefined + if (system !== null && typeof system === 'object' && 'insert' in system) { + system.insert = SYSTEM + touched = true + } + const tools = data.tools as Record | null | undefined + if (tools !== null && typeof tools === 'object') { + if (Array.isArray(tools.added)) { tools.added = tools.added.map(scrubToolSchema); touched = true } + if (Array.isArray(tools.changed)) { tools.changed = tools.changed.map(scrubToolSchema); touched = true } + } + return touched ? JSON.stringify(record) : line } return line }) return out.join('\n') } + +/** Tokenize one tool schema's bulk (description, parameters, anything else), keeping its identifying `name`. */ +function scrubToolSchema(tool: unknown): unknown { + if (tool === null || typeof tool !== 'object' || Array.isArray(tool)) return tool + const out: Record = {} + for (const [k, v] of Object.entries(tool)) out[k] = k === 'name' ? v : TOOLS + return out +}