diff --git a/CHANGELOG.md b/CHANGELOG.md index 3647693..fe433c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,39 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). largest remaining gain on Go is in that gate, not in the scanner. ### Fixed +- **One `content: null` message no longer zeroes the whole request's Gateway saving (audit OX-H4, + DECISIONS §65).** Egress splices replacements into the caller's raw bytes rather than + re-serializing (invariant 9), and located each message by searching for `JSON.stringify(text)` — + where `text` came from `flattenMessageContent`, which sends every **non-string** content through + `JSON.stringify`. For `content: null` that produced the four-character string `null`, so the + search string was `"null"` *with quotes*, absent where the body holds a bare `null`. + `spliceIntoRawBody` returns `undefined` on the **first** miss, so one unmatchable message + discarded the replacements for every other message in the payload. + + `content: null` is the standard OpenAI shape for an assistant turn that calls a tool, so + essentially every agentic OpenAI conversation carried one. Measured on a three-times-repeated + block the Gateway does save on: with such a message present, **8,685 bytes sent and 8,685 + forwarded** — the entire saving gone. Array (multimodal) content failed identically at + **8,530 / 8,530**, which is why this is a structural span scan rather than the `null` + special-case the audit offered as an alternative: that would have fixed one shape and left the + other. Both share a cause — `JSON.stringify` of a *parsed* value is not the caller's bytes, and + a pretty-printed body defeats the search even for plain strings. + + `scanContentSpans` now walks the raw body and returns each spliceable slot's `[start, end)` span, + and `spliceBySpans` overwrites those ranges directly. A span is where the value *is*, so it is + correct for every content shape, and repeated blocks need no forward cursor to disambiguate — the + cursor requirement survives by becoming unnecessary rather than by being dropped. + + **The old value search is kept as a fallback**, so a payload the scanner declines behaves exactly + as before and this change can only add savings. **Declining remains the failure direction:** the + scanner refuses a non-object root, absent or non-array `messages`, a message with no `content` + key, a truncated body, or a missing expected `system`, and the splice refuses when spans do not + ascend across the entries it replaces. + + Invariant 8 is untouched — the Gateway still plans only `cleanup:session-dedup`, and a sole + cross-turn copy is still refused. What this recovers is the *within-payload* saving on payloads + that happen to carry a non-string content. + - **`debtScore` reported 35.00 on every file that reduced, and now measures something (audit OX-M7, DECISIONS §64).** `computeDebtBreakdown` added `metadata.originalBytes` to `elidedBytes` for any item flagged `elided`. But `originalBytes` is the item's **entire** pre-transform length diff --git a/DECISIONS.md b/DECISIONS.md index 946e759..872f1f8 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -4730,3 +4730,89 @@ rather than the bug. - **`--max-debt` still gates nothing on the CLI.** Debt is now a real number, but the default threshold of 75 remains unreachable without a ledger, so no CLI run can trip it. Whether the threshold or the weights should change is a separate question and was not touched. + +--- + +## 65. One `content: null` Turn Zeroed the Whole Request, So Egress Anchors on Positions Now + +**Audit OX-H4**, the highest-value finding in `oxaudit.md` and the first Lane B item taken. + +### The mechanism + +Egress splices replacements into the caller's raw bytes rather than re-serializing the payload +(invariant 9, §54). It located each message by searching the raw body for `JSON.stringify(text)`, +where `text` came from `flattenMessageContent` — which sends every **non-string** content through +`JSON.stringify`. For `content: null` that yields the four-character string `null`, so the search +string became `"null"` *with quotes*, which does not occur where the body holds a bare `null`. + +`spliceIntoRawBody` returns `undefined` on the **first** miss, and `forwardableBody` maps that back +to the untouched `rawBody`. So the failure was all-or-nothing: one unmatchable message discarded +the replacements for every other message in the payload. + +`content: null` is the standard OpenAI shape for an assistant turn that calls a tool. Essentially +every agentic OpenAI conversation carries one. + +### Measured, on a payload the Gateway does save on + +A three-times-repeated block with a turn 1 to seed the session store: + +| payload | bytes sent | bytes forwarded | +|---|---|---| +| all-string content (control) | 8,685 | **less than sent** — the saving lands | +| one `content: null` tool-call turn | 8,685 | **8,685** — entire saving gone | +| one array (multimodal) content part | 8,530 | **8,530** — entire saving gone | + +**The array row is why this is a span scan and not the `null` special-case the audit proposed as +an alternative.** That would have fixed one shape and left the other, and the two share a cause: +`JSON.stringify` of a *parsed* value is not the caller's bytes. It is not even reliably so for +strings — a pretty-printed body defeats the search for the same reason. + +### The fix + +`scanContentSpans` walks the raw body structurally and returns the `[start, end)` span of each +spliceable slot, in the order entries are built (`system` first for Anthropic, then messages). +`spliceBySpans` overwrites those ranges directly. + +A span is *where the value is*, so it is correct for every content shape, and repeated blocks — the +case `session-dedup` exists for — need no forward cursor to disambiguate. The cursor requirement +the audit said "must survive any rewrite" survives by becoming unnecessary, not by being dropped. + +**Kept: the old value search, as a fallback.** `forwardableBody` tries spans first and falls back to +`spliceIntoRawBody`. A payload the scanner declines behaves exactly as it did before, so this change +can only add savings. + +**Kept: declining as the failure direction.** The scanner returns `undefined` on anything it does +not fully understand — a non-object root, absent or non-array `messages`, a message with no +`content` key, a truncated body, an expected `system` that is missing. `spliceBySpans` additionally +refuses when spans do not ascend across the entries it is replacing, which is the case where a +backwards splice would corrupt. + +### Why the tests are shaped the way they are + +This is the code that decides which bytes of a caller's request get overwritten. A wrong span does +not lose a saving, it corrupts a field being sent to a provider — the one direction invariant 3 +forbids. So `test/unit/gateway-content-span-scan.test.ts` is mostly about refusal, every span it +accepts is checked by slicing the input and parsing the result, and the adversarial cases are the +ones a naive scan gets wrong: `"content"` appearing inside a string value, a `meta: { content: … }` +decoy preceding the real key, escaped quotes and backslashes, braces and brackets inside strings, +and a pretty-printed body. + +`test/integration/gateway-null-content-splice.test.ts` adds the property that matters more than the +saving: the forwarded body still parses, message count and roles are unchanged, and the tool-call +turn comes back exactly as sent, `null` included. That assertion would have passed before the fix +too — declining is safe — and it is here to stay true afterwards, which is the harder half. + +### What this does not establish + +- **Nothing about the corpus.** The harness measures CLI routes; the Gateway is not in it. The + instrument for this change is the Gateway integration suite, and the numbers above come from it. +- **Nothing about cross-turn saving.** Invariant 8 is untouched: the Gateway still plans only + `cleanup:session-dedup`, and a sole cross-turn copy is still refused (§41). What was recovered is + the *within-payload* saving, on payloads that happen to contain a non-string content. +- **The `system`-after-`messages` ordering is still a partial decline.** Entries list `system` + first, so such a payload yields non-ascending spans; the splice proceeds when `system` itself has + no replacement and declines when it does. Ordering entries by span position would close that and + was not attempted. +- **`flattenMessageContent` is unchanged.** Structured content is still tagged `'structured'` and + `core/elision` still refuses to elide it. This change lets other messages be elided *despite* one, + not that one be elided. diff --git a/docs/audit-remediation-status.md b/docs/audit-remediation-status.md index bd009de..65a8259 100644 --- a/docs/audit-remediation-status.md +++ b/docs/audit-remediation-status.md @@ -54,7 +54,7 @@ and the reasoning. | Lane | Scope | State | |---|---|---| | **A** | `src/cli/**`, `src/config/**`, `src/core/engine/`, `planner/`, `topology/`, `src/bench/runner.ts`, `src/gateway/exec.ts`, repo hygiene | partially closed — see below | -| **B** | `src/gateway/{proxy,server,session-store,types}.ts`, `stages/cleanup/session-dedup.ts`, `stages/compression/token-hashing.ts`, `adapters/mcp/server.ts`, `bench/fixtures/loader.ts` | **entirely open** (H2, H4, M1, M5, M8, M9, L6, L7, L9, L10, L13) | +| **B** | `src/gateway/{proxy,server,session-store,types}.ts`, `stages/cleanup/session-dedup.ts`, `stages/compression/token-hashing.ts`, `adapters/mcp/server.ts`, `bench/fixtures/loader.ts` | **H4 closed**; open: H2, M1, M5, M8, M9, L6, L7, L9, L10, L13 | **Closed (Lane A):** H1, H3, **H5**, M2, M3, M4, **M6**, **M7**, M10, M11, M12, M14, M16. @@ -74,6 +74,16 @@ reachable only through the exported `optimize` API, not through any of the three points, so its corpus arm differs on **0 of 578 rows** — which is a fact about the instrument, not evidence of correctness. +**H4 is closed — DECISIONS §65, the first Lane B item taken.** Egress located each message by +searching the raw body for `JSON.stringify(text)`, so a `content: null` tool-call turn — the +standard OpenAI shape — produced the search string `"null"` *with quotes*, missed, and because +`spliceIntoRawBody` declines on the **first** miss, discarded the replacements for every other +message. Measured: **8,685 bytes sent, 8,685 forwarded**, the entire saving gone. Array content +failed identically at 8,530/8,530, which is why the fix is a structural span scan rather than the +`null` special-case the audit suggested. The old value search is kept as a fallback, so a declined +payload behaves exactly as before. Invariant 8 untouched: still only `cleanup:session-dedup`, still +no cross-turn saving. + **Open (Lane A):** **M15 is awaiting a decision, not implementation** — whether plain `bench` should shell out to `python` by default. M13 and seven lows (L1, L8, L12, L17–L19, plus L4 recorded below) are unstarted; L2, L3, L5 and L11 were taken from the float pool in §63. diff --git a/src/gateway/proxy.ts b/src/gateway/proxy.ts index fe71460..a6024bd 100644 --- a/src/gateway/proxy.ts +++ b/src/gateway/proxy.ts @@ -644,6 +644,212 @@ function replacementFor(item: ContextItem | undefined, originalText: string): st * That direction is deliberate and follows invariant 3. Losing a saving costs tokens; corrupting * a request field costs correctness, and only one of them is recoverable by the caller. */ +/** A half-open `[start, end)` range of `rawBody`, holding one JSON value exactly as sent. */ +export interface RawSpan { + readonly start: number; + readonly end: number; +} + +const isWs = (c: string): boolean => c === ' ' || c === '\t' || c === '\n' || c === '\r'; + +function skipWs(source: string, index: number): number { + let i = index; + while (i < source.length && isWs(source[i] as string)) i += 1; + return i; +} + +/** Index just past the closing quote of the JSON string starting at `index`, or -1. */ +function scanString(source: string, index: number): number { + let i = index + 1; + while (i < source.length) { + const c = source[i]; + if (c === '\\') { + i += 2; + continue; + } + if (c === '"') return i + 1; + i += 1; + } + return -1; +} + +/** Index just past the JSON value starting at `index` (leading whitespace skipped), or -1. */ +function scanValue(source: string, index: number): number { + const start = skipWs(source, index); + const first = source[start]; + if (first === undefined) return -1; + if (first === '"') return scanString(source, start); + + if (first === '{' || first === '[') { + // Only the outer bracket type is counted. Inner brackets of the *other* type are balanced + // within, so they cannot affect this depth, and strings are stepped over whole so a bracket + // inside one is never seen. + const close = first === '{' ? '}' : ']'; + let depth = 0; + let i = start; + while (i < source.length) { + const c = source[i] as string; + if (c === '"') { + const after = scanString(source, i); + if (after === -1) return -1; + i = after; + continue; + } + if (c === first) depth += 1; + else if (c === close) { + depth -= 1; + if (depth === 0) return i + 1; + } + i += 1; + } + return -1; + } + + // A primitive: number, `true`, `false`, `null`. Ends at the first structural character. + let i = start; + while (i < source.length) { + const c = source[i] as string; + if (c === ',' || c === '}' || c === ']' || isWs(c)) break; + i += 1; + } + return i === start ? -1 : i; +} + +/** The span of `key`'s value inside the JSON object beginning at `objectStart`, or undefined. */ +function findMemberValue(source: string, objectStart: number, key: string): RawSpan | undefined { + let i = skipWs(source, objectStart); + if (source[i] !== '{') return undefined; + i += 1; + + for (;;) { + i = skipWs(source, i); + if (source[i] === '}') return undefined; + if (source[i] !== '"') return undefined; + + const keyEnd = scanString(source, i); + if (keyEnd === -1) return undefined; + const name = source.slice(i + 1, keyEnd - 1); + + i = skipWs(source, keyEnd); + if (source[i] !== ':') return undefined; + + const valueStart = skipWs(source, i + 1); + const valueEnd = scanValue(source, valueStart); + if (valueEnd === -1) return undefined; + + if (name === key) return { start: valueStart, end: valueEnd }; + + i = skipWs(source, valueEnd); + if (source[i] === ',') { + i += 1; + continue; + } + if (source[i] === '}') return undefined; + return undefined; + } +} + +/** + * Where each spliceable slot's value actually sits in the caller's bytes — audit OX-H4. + * + * The splice used to locate every message by searching for `JSON.stringify(text)`, where `text` + * came from `flattenMessageContent` — which sends every **non-string** content through + * `JSON.stringify`. For `content: null` that produces the four-character string `null`, so the + * search string is `"null"` *with quotes*, which does not occur where the body holds a bare + * `null`. `spliceIntoRawBody` returns `undefined` on the first miss, so **one unmatchable message + * discarded the replacements for every other message in the payload.** + * + * `content: null` is the standard OpenAI shape for an assistant turn that calls a tool, so + * essentially every agentic OpenAI conversation carries one. Measured on a payload with a + * three-times-repeated block that the Gateway does save on: with one such message present, bytes + * forwarded equalled bytes received exactly — the entire saving, gone. Array content (multimodal + * parts) failed the same way, which is why this is a span scan rather than the `null` special-case + * the audit suggested: that would have fixed one shape and left the other. + * + * Returning spans instead of search strings removes the question. A span is where the value *is*, + * so it is correct for every content shape, and duplicate blocks need no cursor to disambiguate. + * + * Declines — returns `undefined` — on anything it does not fully understand, and the caller then + * falls back to the value search. Losing a saving costs tokens; corrupting a request field costs + * correctness, and only one of those is recoverable by the caller (invariant 3). + */ +export function scanContentSpans( + rawBody: string, + options: { readonly includeSystem: boolean }, +): ReadonlyArray | undefined { + const rootStart = skipWs(rawBody, 0); + if (rawBody[rootStart] !== '{') return undefined; + + const spans: RawSpan[] = []; + + if (options.includeSystem) { + const system = findMemberValue(rawBody, rootStart, 'system'); + if (!system) return undefined; + spans.push(system); + } + + const messages = findMemberValue(rawBody, rootStart, 'messages'); + if (!messages || rawBody[messages.start] !== '[') return undefined; + + let i = skipWs(rawBody, messages.start + 1); + if (rawBody[i] === ']') return spans; + + for (;;) { + const elementStart = skipWs(rawBody, i); + const elementEnd = scanValue(rawBody, elementStart); + if (elementEnd === -1) return undefined; + + // A message that is not an object, or carries no `content` key, has no span to splice. The + // entry list still holds a slot for it, so alignment would break — decline instead. + const content = findMemberValue(rawBody, elementStart, 'content'); + if (!content) return undefined; + spans.push(content); + + i = skipWs(rawBody, elementEnd); + if (rawBody[i] === ',') { + i += 1; + continue; + } + if (rawBody[i] === ']') return spans; + return undefined; + } +} + +/** + * Replaces each entry's value in place, using the span where that value actually sits. + * + * No cursor and no searching: a span is a position, so repeated blocks — the case + * `session-dedup` exists for — need nothing to disambiguate them. + */ +function spliceBySpans( + rawBody: string, + entries: ReadonlyArray<{ readonly from: string; readonly to?: string }>, + spans: ReadonlyArray, +): string | undefined { + // Alignment is the whole safety argument: span *k* must be the value that entry *k* describes. + // Both are built in payload order, but a mismatch would splice a replacement over an unrelated + // field, so it is checked rather than assumed. + if (spans.length !== entries.length) return undefined; + + let out = ''; + let cursor = 0; + + for (let k = 0; k < entries.length; k += 1) { + const entry = entries[k] as { readonly from: string; readonly to?: string }; + const span = spans[k] as RawSpan; + if (entry.to === undefined) continue; + + // Spans are ascending by construction; a violation means the scan and the entry list + // disagree about order, and splicing on that would corrupt the payload. + if (span.start < cursor) return undefined; + + out += rawBody.slice(cursor, span.start) + JSON.stringify(entry.to); + cursor = span.end; + } + + return out + rawBody.slice(cursor); +} + function spliceIntoRawBody( rawBody: string, entries: ReadonlyArray<{ readonly from: string; readonly to?: string }>, @@ -705,12 +911,20 @@ function wireTokenMetrics( function forwardableBody( rawBody: string, entries: ReadonlyArray<{ readonly from: string; readonly to?: string }>, + options: { readonly includeSystem: boolean }, ): string { if (!entries.some((entry) => entry.to !== undefined)) { return rawBody; } - const spliced = spliceIntoRawBody(rawBody, entries); + // Spans first, value search second (audit OX-H4). The span scan is correct for every content + // shape; the value search only works when the content is a string whose canonical encoding + // happens to be the caller's own bytes. Keeping the search as a fallback means this change can + // only add savings — a payload the scan declines behaves exactly as it did before. + const spans = scanContentSpans(rawBody, options); + const spliced = + (spans ? spliceBySpans(rawBody, entries, spans) : undefined) ?? spliceIntoRawBody(rawBody, entries); + if (spliced === undefined || Buffer.byteLength(spliced, 'utf8') >= Buffer.byteLength(rawBody, 'utf8')) { return rawBody; } @@ -811,7 +1025,7 @@ function processOpenAiRequest( }); // Spliced into the caller's bytes rather than re-serialized around them (audit M7). - const finalBody = forwardableBody(rawBody, entries); + const finalBody = forwardableBody(rawBody, entries, { includeSystem: false }); options.sessionStore.recordTurn( session.sessionId, @@ -951,7 +1165,7 @@ function processAnthropicRequest( }); // Spliced into the caller's bytes rather than re-serialized around them (audit M7). - const finalBody = forwardableBody(rawBody, entries); + const finalBody = forwardableBody(rawBody, entries, { includeSystem: Boolean(parsedPayload.system) }); options.sessionStore.recordTurn( session.sessionId, diff --git a/test/integration/gateway-null-content-splice.test.ts b/test/integration/gateway-null-content-splice.test.ts new file mode 100644 index 0000000..d55cd14 --- /dev/null +++ b/test/integration/gateway-null-content-splice.test.ts @@ -0,0 +1,199 @@ +import { request as httpRequest } from 'node:http'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { GatewayServer } from '../../src/gateway/server'; + +/** + * One `content: null` message must not zero the whole request's saving — audit OX-H4. + * + * Egress splices replacements into the caller's raw bytes rather than re-serializing the payload + * (invariant 9, DECISIONS §54). It locates each message by searching the raw body for + * `JSON.stringify(text)`, where `text` comes from `flattenMessageContent` — which sends every + * non-string through `JSON.stringify`. For `content: null` that yields the four-character string + * `null`, and the search string becomes `"null"` **with quotes**, which does not occur where the + * body holds a bare `null`. + * + * `spliceIntoRawBody` returns `undefined` on the *first* miss and `forwardableBody` maps that back + * to the untouched `rawBody`, so the failure is **all-or-nothing**: one unmatchable message + * discards the replacements for every other message in the payload. + * + * `content: null` is not an edge case. It is the standard OpenAI shape for an assistant turn that + * calls a tool, so essentially every agentic OpenAI conversation carries one. The direction is + * safe — bytes are forwarded unchanged, and `wireTokenMetrics` measures what actually left, so the + * reported numbers stay honest — but the product's headline transform silently stops happening on + * its most common payload shape. + */ +describe('gateway splice with non-string message content', () => { + let server: GatewayServer; + let port: number; + + const BLOCK = Array.from( + { length: 30 }, + (_, i) => `export function helper${i}(input) {\n const scaled = input * ${i};\n return scaled + ${i};\n}`, + ).join('\n\n'); + + const post = (sessionId: string, payload: unknown) => + new Promise<{ sent: number; forwarded: number }>((resolve, reject) => { + const body = Buffer.from(JSON.stringify(payload), 'utf8'); + const req = httpRequest( + { + host: '127.0.0.1', + port, + path: '/v1/chat/completions', + method: 'POST', + headers: { + 'content-type': 'application/json', + authorization: 'Bearer sk-test', + 'x-session-id': sessionId, + 'content-length': body.length, + }, + }, + (res) => { + const chunks: Buffer[] = []; + res.on('data', (c: Buffer) => chunks.push(c)); + res.on('end', () => resolve({ sent: body.length, forwarded: Buffer.concat(chunks).length })); + }, + ); + req.on('error', reject); + req.end(body); + }); + + beforeAll(async () => { + // The mock upstream echoes the outgoing body, which is what makes "bytes actually forwarded" + // observable without a provider. + server = new GatewayServer({ port: 0, mockUpstream: true }); + await server.start(); + const bound = server.port; + expect(bound).toBeTypeOf('number'); + port = bound as number; + }); + + afterAll(async () => { + await server.stop(); + }); + + /** Same request, but returning the echoed body so its structure can be inspected. */ + const postBody = (sessionId: string, payload: unknown) => + new Promise((resolve, reject) => { + const body = Buffer.from(JSON.stringify(payload), 'utf8'); + const req = httpRequest( + { + host: '127.0.0.1', + port, + path: '/v1/chat/completions', + method: 'POST', + headers: { + 'content-type': 'application/json', + authorization: 'Bearer sk-test', + 'x-session-id': sessionId, + 'content-length': body.length, + }, + }, + (res) => { + const chunks: Buffer[] = []; + res.on('data', (c: Buffer) => chunks.push(c)); + res.on('end', () => resolve(Buffer.concat(chunks).toString('utf8'))); + }, + ); + req.on('error', reject); + req.end(body); + }); + + /** The within-payload repetition the Gateway does save on, with a turn 1 to seed the store. */ + const repeatedPayload = (extra: ReadonlyArray) => ({ + model: 'gpt-x', + messages: [ + { role: 'user', content: BLOCK }, + { role: 'assistant', content: 'ok' }, + ...extra, + { role: 'user', content: BLOCK }, + { role: 'assistant', content: 'still ok' }, + { role: 'user', content: BLOCK }, + ], + }); + + it('saves bytes on a repeated block when every message content is a string — the control', async () => { + // Without this the assertion below would pass on a payload that never had a saving to lose, + // which is the shape of a green test that measured nothing. + const session = `sess-openai-control-${Date.now()}`; + await post(session, { model: 'gpt-x', messages: [{ role: 'user', content: BLOCK }] }); + + const turn2 = await post(session, repeatedPayload([])); + + expect(turn2.forwarded).toBeLessThan(turn2.sent); + }); + + it('still saves those bytes when an assistant tool-call turn carries content: null', async () => { + const session = `sess-openai-null-${Date.now()}`; + await post(session, { model: 'gpt-x', messages: [{ role: 'user', content: BLOCK }] }); + + // The standard OpenAI assistant tool-call turn. Nothing about it is elidable, and nothing + // about it should prevent the *other* messages from being elided. + const turn2 = await post( + session, + repeatedPayload([ + { + role: 'assistant', + content: null, + tool_calls: [ + { id: 'call_1', type: 'function', function: { name: 'read_file', arguments: '{"path":"a.ts"}' } }, + ], + }, + { role: 'tool', tool_call_id: 'call_1', content: 'file contents here' }, + ]), + ); + + expect(turn2.forwarded).toBeLessThan(turn2.sent); + }); + + it('forwards a body that still parses, with every untouched field intact', async () => { + // The property that matters more than the saving. A splice writes over the caller's bytes, so + // the failure mode this guards is not "saved less" but "sent something else" — the direction + // invariant 3 forbids. Asserted on the payload shape that previously declined entirely, so it + // is now exercising the span path rather than the old value search. + const session = `sess-openai-shape-${Date.now()}`; + await post(session, { model: 'gpt-x', messages: [{ role: 'user', content: BLOCK }] }); + + const payload = repeatedPayload([ + { + role: 'assistant', + content: null, + tool_calls: [ + { id: 'call_1', type: 'function', function: { name: 'read_file', arguments: '{"path":"a.ts"}' } }, + ], + }, + ]); + const echoed = await postBody(session, payload); + const forwarded = JSON.parse(echoed) as { + model: string; + messages: ReadonlyArray>; + }; + const sent = payload.messages as ReadonlyArray>; + + expect(forwarded.model).toBe('gpt-x'); + expect(forwarded.messages.length).toBe(sent.length); + + // The tool-call turn is not elidable and must come back exactly as sent, `null` included. + const toolTurn = forwarded.messages[2] as Record; + expect(toolTurn.role).toBe('assistant'); + expect(toolTurn.content).toBeNull(); + expect(toolTurn.tool_calls).toEqual(sent[2]?.tool_calls); + + // Roles are structural, never touched by elision. + expect(forwarded.messages.map((m) => m.role)).toEqual(sent.map((m) => m.role)); + }); + + it('still saves those bytes when a message carries array content', async () => { + // The same defect through the other common non-string shape: OpenAI multimodal content parts. + // `JSON.stringify` of the parsed array is not guaranteed to be the caller's own bytes, so the + // search can miss for formatting reasons alone. + const session = `sess-openai-array-${Date.now()}`; + await post(session, { model: 'gpt-x', messages: [{ role: 'user', content: BLOCK }] }); + + const turn2 = await post( + session, + repeatedPayload([{ role: 'user', content: [{ type: 'text', text: 'and this part' }] }]), + ); + + expect(turn2.forwarded).toBeLessThan(turn2.sent); + }); +}); diff --git a/test/unit/gateway-content-span-scan.test.ts b/test/unit/gateway-content-span-scan.test.ts new file mode 100644 index 0000000..68cb69c --- /dev/null +++ b/test/unit/gateway-content-span-scan.test.ts @@ -0,0 +1,160 @@ +import { describe, expect, it } from 'vitest'; +import { scanContentSpans } from '../../src/gateway/proxy'; + +/** + * The span scanner that replaced value-searching on egress — audit OX-H4. + * + * This is the riskiest kind of code in the project: it decides which bytes of a caller's request + * get overwritten. A wrong span does not lose a saving, it corrupts a field being sent to a + * provider, which is the one direction invariant 3 forbids. So the cases below are mostly about + * what it must *refuse*, and every span it does return is checked by slicing the input with it and + * parsing the result. + */ +describe('scanContentSpans', () => { + const spansOf = (body: string, includeSystem = false) => scanContentSpans(body, { includeSystem }); + + /** Every returned span must delimit exactly one parseable JSON value. */ + const valuesAt = (body: string, includeSystem = false): unknown[] => { + const spans = spansOf(body, includeSystem); + expect(spans).toBeDefined(); + return (spans as ReadonlyArray<{ start: number; end: number }>).map((s) => + JSON.parse(body.slice(s.start, s.end)), + ); + }; + + describe('the shapes that broke the value search', () => { + it('finds a null content value', () => { + const body = JSON.stringify({ model: 'm', messages: [{ role: 'assistant', content: null }] }); + expect(valuesAt(body)).toEqual([null]); + }); + + it('finds array content', () => { + const body = JSON.stringify({ + model: 'm', + messages: [{ role: 'user', content: [{ type: 'text', text: 'hi' }] }], + }); + expect(valuesAt(body)).toEqual([[{ type: 'text', text: 'hi' }]]); + }); + + it('finds every content in a mixed payload, in payload order', () => { + const body = JSON.stringify({ + model: 'm', + messages: [ + { role: 'user', content: 'first' }, + { role: 'assistant', content: null, tool_calls: [{ id: 'c1' }] }, + { role: 'user', content: [{ type: 'text', text: 'third' }] }, + { role: 'assistant', content: 'fourth' }, + ], + }); + expect(valuesAt(body)).toEqual(['first', null, [{ type: 'text', text: 'third' }], 'fourth']); + }); + }); + + describe('things that must not fool it', () => { + it('ignores the word content inside a string value', () => { + const body = JSON.stringify({ + model: 'm', + messages: [{ role: 'user', content: 'the key "content" appears here', extra: 'x' }], + }); + expect(valuesAt(body)).toEqual(['the key "content" appears here']); + }); + + it('steps over escaped quotes and backslashes', () => { + const tricky = 'he said \\"hi\\" and \\\\ then left'; + const body = JSON.stringify({ model: 'm', messages: [{ role: 'user', content: tricky }] }); + expect(valuesAt(body)).toEqual([tricky]); + }); + + it('ignores braces and brackets inside strings', () => { + const body = JSON.stringify({ + model: 'm', + messages: [{ role: 'user', content: '{"not":"json"} and ] [ }' }], + }); + expect(valuesAt(body)).toEqual(['{"not":"json"} and ] [ }']); + }); + + it('handles deeply nested structured content', () => { + const nested = { a: [{ b: { c: [1, 2, { d: 'e' }] } }] }; + const body = JSON.stringify({ model: 'm', messages: [{ role: 'user', content: nested }] }); + expect(valuesAt(body)).toEqual([nested]); + }); + + it('handles a pretty-printed body, where canonical encoding differs from the bytes', () => { + // The formatting case: `JSON.stringify` of the parsed value is compact, so a value search + // could never have matched these bytes even when the content is a plain string. + const body = JSON.stringify( + { model: 'm', messages: [{ role: 'user', content: 'hello' }, { role: 'assistant', content: null }] }, + null, + 2, + ); + expect(valuesAt(body)).toEqual(['hello', null]); + }); + + it('is not confused by a content key nested inside another message field', () => { + const body = JSON.stringify({ + model: 'm', + messages: [{ role: 'tool', meta: { content: 'decoy' }, content: 'real' }], + }); + // `meta` precedes `content`, and its own inner `content` must not be taken for the message's. + expect(valuesAt(body)).toEqual(['real']); + }); + }); + + describe('what it refuses', () => { + it('declines a message with no content key, rather than misaligning', () => { + // The entry list still holds a slot for such a message, so returning fewer spans than + // entries would splice a replacement over the wrong value. + const body = JSON.stringify({ model: 'm', messages: [{ role: 'user', text: 'no content key' }] }); + expect(spansOf(body)).toBeUndefined(); + }); + + it('declines when messages is absent', () => { + expect(spansOf(JSON.stringify({ model: 'm' }))).toBeUndefined(); + }); + + it('declines when messages is not an array', () => { + expect(spansOf(JSON.stringify({ model: 'm', messages: 'nope' }))).toBeUndefined(); + }); + + it('declines a truncated body', () => { + expect(spansOf('{"model":"m","messages":[{"role":"user","content":"unclo')).toBeUndefined(); + }); + + it('declines a non-object root', () => { + expect(spansOf('[1,2,3]')).toBeUndefined(); + }); + + it('declines when system was expected but is absent', () => { + const body = JSON.stringify({ model: 'm', messages: [{ role: 'user', content: 'x' }] }); + expect(spansOf(body, true)).toBeUndefined(); + }); + + it('returns an empty list for an empty messages array', () => { + expect(spansOf(JSON.stringify({ model: 'm', messages: [] }))).toEqual([]); + }); + }); + + describe('the Anthropic system slot', () => { + it('puts system first, matching the order entries are built in', () => { + const body = JSON.stringify({ + model: 'm', + system: 'be terse', + messages: [{ role: 'user', content: 'hi' }], + }); + expect(valuesAt(body, true)).toEqual(['be terse', 'hi']); + }); + + it('finds system even when it is written after messages', () => { + // Entries always list system first, so the scan returns it first too, regardless of where + // the caller put it — which makes the span list non-ascending for this payload. + // + // `spliceBySpans` only enforces ascent across entries it actually replaces, so this is not + // simply a decline: if only messages are being replaced, the out-of-order `system` span is + // never spliced and the saving still lands. It declines exactly when `system` itself has a + // replacement and a later span would then splice backwards, which is the case that would + // corrupt. + const body = '{"messages":[{"role":"user","content":"hi"}],"system":"be terse"}'; + expect(valuesAt(body, true)).toEqual(['be terse', 'hi']); + }); + }); +});