From 515d04339b56a137070c6411ed5fac40e08b95ca Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 7 Jul 2026 01:26:25 +0800 Subject: [PATCH] Address review round 2: preserve delta insert arity; reject unpinned header-deltas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Residuals from the Codex re-review: 1. A system delta's insert was flattened to one token, so deltas differing only in inserted-line count compared equal. Now one {{system}} token per inserted line — position AND extent survive, content does not. 2. The live uniformity guard folded only request/header snapshots, so a mid-run header CHANGE (request/header-delta) could diverge from the pin invisibly. Non-pinning runs now assert zero header-delta events: a scenario that legitimately changes its header mid-run exists to show that change, so it must pin (fail-loud until it does). --- ...-request-header-content-in-one-scenario.md | 6 ++-- examples/acp-agent/tests/acp.snapshot.ts | 31 ++++++++++++++----- .../tests/snapshot-normalize.spec.ts | 7 +++-- .../acp-agent/tests/snapshot-normalize.ts | 13 ++++---- 4 files changed, 37 insertions(+), 20 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 c833425145..4e4b6a85d7 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 @@ -8,11 +8,11 @@ Every model-driving ACP snapshot fixture (`session.jsonl`) embedded the full com ## 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 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`). +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 with one `{{system}}` token per inserted line, the tools delta's added/removed/changed tool names — and tokenizes only the bulk (prompt text, 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 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. +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, and no `request/header-delta` may appear at all (a mid-run header change diverges from the pin by construction, and its content would be invisible under the scrub), so the single-pin premise is asserted rather than assumed. 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. @@ -25,7 +25,7 @@ One pin covers the whole suite because every session — parent, spawn child, fo ## 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, 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. +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, live header-uniformity, and no-unpinned-delta guards run inside the suite, and `scrubRequestHeaders` has unit coverage for both header event types, delta structure preservation (line positions, insert arity, tool names), absent-field preservation, config/reason retention, byte-for-byte pass-through of other lines, and idempotence. ## Consequences diff --git a/examples/acp-agent/tests/acp.snapshot.ts b/examples/acp-agent/tests/acp.snapshot.ts index 977615f841..bde4f4dbb9 100644 --- a/examples/acp-agent/tests/acp.snapshot.ts +++ b/examples/acp-agent/tests/acp.snapshot.ts @@ -21,7 +21,8 @@ import { type NormalizeContext, normalizeSessionLog, normalizeStdout, scrubReque * `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. A per-run uniformity guard keeps - * the single pin sound: every live header must equal the pinned one (see the + * the single pin sound: every live header must equal the pinned one, and no + * header-delta may appear outside the pinning scenario (see the * pinned-header RFC, * docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md). * @@ -194,6 +195,14 @@ function normalizedHeaders(rawLog: string, ctx: NormalizeContext): unknown[] { .map(record => record.data?.header) } +/** Count the `request/header-delta` events in a session JSONL. */ +function headerDeltaCount(rawLog: string): number { + return rawLog.split('\n') + .filter(line => line.trim().length > 0) + .filter(line => (JSON.parse(line) as { type?: unknown }).type === 'request/header-delta') + .length +} + for (const scenario of SCENARIOS) { describe(`snapshot: ${scenario.name}`, () => { // In RECORD mode, only re-run the `recorded` (live-API) scenarios; the @@ -273,19 +282,25 @@ for (const scenario of SCENARIOS) { } // 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). + // session in the suite composes the SAME header and keeps it for the + // whole run. Assert both halves live. (1) 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. (2) No request/header-delta may + // appear at all — a mid-run header change diverges from the pin by + // construction, and its content would be invisible under the scrub. If + // either 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) { + expect(headerDeltaCount(log.content), `session ${log.id}: a request/header-delta in a non-pinning scenario`) + .toBe(0) 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`) diff --git a/examples/acp-agent/tests/snapshot-normalize.spec.ts b/examples/acp-agent/tests/snapshot-normalize.spec.ts index e3e7eb2616..fa225bd659 100644 --- a/examples/acp-agent/tests/snapshot-normalize.spec.ts +++ b/examples/acp-agent/tests/snapshot-normalize.spec.ts @@ -135,13 +135,14 @@ describe('scrubRequestHeaders', () => { expect(out).not.toContain('{{tools}}') }) - it('scrubs a header-delta system payload but keeps its line positions', () => { + it('scrubs a header-delta system payload but keeps its line positions and arity', () => { const delta = JSON.stringify({ type: 'request/header-delta', seq: 8, time: 9, - data: { system: { keepStart: 1, keepEnd: 4, insert: ['leaked prompt line'] }, config: { model: 'm2' } }, + data: { system: { keepStart: 1, keepEnd: 4, insert: ['leaked prompt line', 'second line'] }, config: { model: 'm2' } }, }) const out = scrubRequestHeaders(`${headerLine}\n${delta}\n`) - expect(out).toContain('"insert":"{{system}}"') + // One token PER inserted line: the edit's position AND extent survive. + expect(out).toContain('"insert":["{{system}}","{{system}}"]') expect(out).toContain('"keepStart":1') expect(out).toContain('"keepEnd":4') expect(out).toContain('"config":{"model":"m2"}') diff --git a/examples/acp-agent/tests/snapshot-normalize.ts b/examples/acp-agent/tests/snapshot-normalize.ts index 163bf2bc93..28c10f102d 100644 --- a/examples/acp-agent/tests/snapshot-normalize.ts +++ b/examples/acp-agent/tests/snapshot-normalize.ts @@ -126,10 +126,11 @@ export function normalizeSessionLog(rawLog: string, ctx: NormalizeContext): stri * 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. + * delta's `keepStart`/`keepEnd` line positions and inserted-line COUNT (one + * `{{system}}` token per inserted line), the tools delta's + * added/removed/changed tool NAMES — and tokenizes only the bulk (prompt + * text; 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 churns every fixture by design @@ -159,8 +160,8 @@ export function scrubRequestHeaders(rawLog: string): string { if (record.type === 'request/header-delta') { let touched = false const system = data.system as Record | null | undefined - if (system !== null && typeof system === 'object' && 'insert' in system) { - system.insert = SYSTEM + if (system !== null && typeof system === 'object' && Array.isArray(system.insert)) { + system.insert = system.insert.map(() => SYSTEM) touched = true } const tools = data.tools as Record | null | undefined