Coder/deepseek-harness
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Address review: structure-preserving delta scrub, live header-uniformity guard, RFC style

Browse Source 
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.
This commit is contained in:
Tianyi Cui
2026-07-07 00:51:55 +08:00
parent 1fd9e2a9ab
commit a0d8f33b29
4 changed files with 120 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
examples/acp-agent/tests/acp.snapshot.ts
View File

@@ -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])
}
}
}
})
})
}

36
examples/acp-agent/tests/snapshot-normalize.spec.ts
View File

@@ -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)
})
})

43
examples/acp-agent/tests/snapshot-normalize.ts
View File

@@ -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<string, unknown> | null | undefined
if (system !== null && typeof system === 'object' && 'insert' in system) {
system.insert = SYSTEM
touched = true
}
const tools = data.tools as Record<string, unknown> | 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<string, unknown> = {}
for (const [k, v] of Object.entries(tool)) out[k] = k === 'name' ? v : TOOLS
return out
}