audit wave 2: three entry modes, three kinds of untruth - #11
Merged
Conversation
Audit Wave 2 — M5a, M5b, H4, M8, M9, M10, plus the M5 minor items. Six independent findings that share one shape: a surface that reports success while doing nothing, or while doing something other than what it says. Full reasoning in DECISIONS §44. M5a — the MCP entry mode was a guaranteed no-op. `optimize_context` exposed no `targetReductionRatio`, so with no budget the planner returned `pass_through` with an empty `stageIds`: zero stages, the input back unchanged, `reductionRatio: 0`, and no error. A client calling the tool exactly as documented received a clean success for work that never happened. The parameter is now in the schema, range-checked and rejected rather than clamped (§29's argument: a value silently coerced into range is a run the caller believes they configured and did not). The response now carries `budgetApplied`, `planMode`, `stagesExecuted` and a `notice` when no budget was in effect — invariant 10 applied to a budget rather than a validator, because `reductionRatio: 0` alone cannot distinguish "nothing was compressible" from "nothing ran", and only one of those is the caller's to fix. Measured through the stdio server on src/core/planner/index.ts: 0 stages / 0.0% without a budget, 4 stages / 69.1% at 0.3, the latter matching the CLI's `tokenEstimateSaved: 586` on the same file. M5b — a marker the product has never produced. `rehydrate_context` matched `<ELIDED: ref=... >`; `cleanup:session-dedup` emits `[TokenDamper Elided: ref=... bytes=... kind=...]`. No overlap, so session rehydration matched nothing on every input and returned the text unchanged. It had never worked. The fix is not the pattern: both sides were internally consistent and each restated the format the other owned. `renderSessionElisionMarker` and `SESSION_ELISION_MARKER_PATTERN` now live together in core/elision/marker.ts and both sides derive from them. The new test takes its marker from running the stage, never from a literal — a test restating either format would have passed while the pair was broken. H4 — three knobs parsed, validated, then read by nothing. `--max-output-tokens` and `--max-latency-ms` (with their TOKENDAMPER_* variables) reached no consumer anywhere; `--risk-tolerance` and the MCP `riskTolerance` property reached exactly one, cli/bench-table-renderer.ts:97, which prints it in a column. Removed from the surface, not from the model: `OptimizationBudget` keeps the fields because ARCHITECTURE.md pins that model as frozen, and a field awaiting an implementation is not the same defect as a dial that reports success. Each field now carries a doc comment naming its consumer or stating it has none. Removal is a hard `Unknown argument`, not a silent no-op — that would be the same defect wearing a new hat. `--target-reduction-ratio` deliberately stays despite the planner reading it only as `> 0`: it is the only budget flag every doc uses, and making it a real proportional target is a planner change. `--max-debt` also stays; unlike the other three it is wired to DebtTracker and merely arithmetically inert. M8 — test seams in the request path. `TOKENDAMPER_MOCK_UPSTREAM=true` made the proxy answer with the caller's own optimized prompt and a 200 as though a model had produced it; `NODE_ENV === 'test'` waived the missing-credentials 401, and that variable is set by a great many CI systems for unrelated reasons. Both env reads are removed rather than demoted to fallbacks — keeping TOKENDAMPER_MOCK_UPSTREAM would have preserved precisely the hazard the finding names — and replaced by `mockUpstream` and `allowMissingUpstreamCredentials` on ProxyHandlerOptions, GatewayConfig and ExecOptions. Ten tests in test/unit/gateway.test.ts failed the moment the NODE_ENV branch went: they had been passing because vitest sets that variable, and none of them mentioned it. M9 — request headers returned as response headers. Both optimize paths spread `cleanHeaders` into the response, and it strips only `host` and `content-length` — so `authorization`, `x-api-key` and cookies came back out on the response, reproducible under mock upstream as `x-api-key: sk-test`. Latent, because the normal path overwrites them with the upstream response's headers, but latent meant one environment variable away (M8), and a response header is a value that gets logged, cached and proxied onward. Now constructed explicitly: the fix is to stop deriving one from the other, not to lengthen a strip-list. M10 — `bench` threw for every installed user. The bundled datasets resolved against `process.cwd()` alone and `test/` was not in package.json's `files`; the CLI additionally defaulted to the literal path `test/fixtures/bench`. Every existing bench test runs with the repo as its working directory, which is exactly why none of them saw it. `resolveBundledFixture` tries the working directory first, then the package root — found by walking up to the nearest package.json rather than by a fixed `..` offset, because this module runs from src/bench/fixtures/ under vitest and dist/src/bench/fixtures/ compiled, so a constant is right for exactly one route. Verified by running the built CLI from a directory with no test/ tree: 10 fixtures, exit 0. Also fixes `EISDIR` when `loadBenchmarkFixtures` is handed a directory. M5 minor — `traceStore` was a module-level Map shared by every server in the process (two servers could evict each other's traces, and a request id minted by one was retrievable through the other), now per-server and injectable; `get_session_metrics` and `resources/read` called `getOrCreateSession`, so asking about a session created it — an unknown id answered with a plausible all-zero record and, under `maxSessions`, could evict a live one — both now use the new read-only `GatewaySessionStore.getSession`; and `initialize` returned MCP_PROTOCOL_VERSION unconditionally, telling a client its unsupported request had been agreed to, now negotiated against a list holding the single revision actually implemented. Measurement: 594 of 594 corpus rows are identical to the pre-Wave-2 engine across outputSha, byteIdentical, tokenBefore, tokenAfter, reduction, fallbackUsed, driftScore, debtScore, planMode, stageCount, contentType, astChecked, astUnchecked, driftMeasured and unwitnessedItems — both engines run against the same frozen corpus, varying only dist/. Nothing here touches a stage's output, and that is measured rather than assumed. The bucket table moves anyway, which is the trap that method exists to catch: typescript file reads 23.26% over 60 files where dd540fe read 25.35% over 59, with `reduced` unchanged at 33. The denominator grew by src/bench/fixtures/bundled-path.ts, added by M10, which falls back and contributes zero. The prose bucket 28 -> 29 was already outstanding: docs/audit-remediation-status.md landed in 7a1b5a7, after the dd540fe baseline was recorded. collect.js refused on both mismatches before measuring anything; recipe.json records each step with its cause. Two false greens hit while doing that A/B, both recorded in the status doc's traps section because they are the same class of failure as the findings above: a build that failed left the previous dist/ in place, so an engine was compared against itself and reported a perfect match; and a per-file diff keyed on a field the harness does not emit collapsed 594 rows onto one undefined key and reported "compared 2 rows, differing: 0". Suite: 557 passing (was 525), typecheck and lint clean. Every new test was run against the unfixed code first — M5b's two round-trip cases, all five M10 cases and M5a's reduction case fail there. C4 is now the only unstarted audit item. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit Wave 2 — M5a, M5b, H4, M8, M9, M10, plus the M5 minor items. Six independent findings
that share one shape: a surface that reports success while doing nothing, or while doing
something other than what it says. Full reasoning in DECISIONS §44.
C4 is the only unstarted audit item after this.
What was wrong
optimize_contexthad no budget parameter, so the MCP entry mode was a guaranteed 0% no-op that reported successtargetReductionRatioin the schema; response statesbudgetApplied/planMode/stagesExecutedrehydrate_contextmatched<ELIDED: ref=… >; the stage emits[TokenDamper Elided: …]. Session rehydration had never workedcore/elision/marker.ts, used by both sidesUnknown argumentTOKENDAMPER_MOCK_UPSTREAMandNODE_ENV === 'test'read from the environment inside the request pathauthorization/x-api-keyreturned as response headersbenchthrew for every installed userpackage.jsonwalktraceStore; reads that create sessions;initializethat does not negotiategetSession; real negotiationThree notes for the reviewer
H4 removes the surface, not the model.
OptimizationBudgetkeepsmaxOutputTokens,maxLatencyMsandriskTolerance, becauseARCHITECTURE.mdpins that model as frozen andmax_audit.md:737asks for removal "from the surface". A field awaiting an implementation is adifferent thing from a dial that reports success. Each field now carries a doc comment naming its
consumer or stating it has none.
Two knobs were deliberately kept:
--target-reduction-ratio, because it is the only budgetflag every doc and example uses and making it a real proportional target is a planner change (it
stays a named open decision); and
--max-debt, which unlike the other three is wired toDebtTrackerand is merely arithmetically inert.M8 produced its own evidence. Removing the
NODE_ENVbranch failed ten existing tests intest/unit/gateway.test.ts— they had been passing because vitest sets that variable, and noneof them mentioned it. The env reads were removed rather than demoted to fallbacks; keeping
TOKENDAMPER_MOCK_UPSTREAMwould have preserved exactly the hazard the finding names.M5b's fix is the shared format, not the regex. Both sides were internally consistent and had
never agreed, because each restated the format the other owned. The new test builds its marker by
running the stage — a test restating either format would have passed while the pair was broken.
Verification
M10 cases, and M5a's reduction case fail there.
initializenegotiation,optimize_contextwith andwithout a budget (0 stages / 0.0% vs 4 stages / 69.1%, the latter matching the CLI's
tokenEstimateSaved: 586on the same file), out-of-range ratio rejected, unknown sessionreported;
benchrun from a directory with notest/tree (10 fixtures, exit 0); the threeremoved flags erroring.
Corpus measurement
594 of 594 rows identical to the pre-Wave-2 engine across
outputSha,byteIdentical,tokenBefore,tokenAfter,reduction,fallbackUsed,driftScore,debtScore,planMode,stageCount,contentType,astChecked,astUnchecked,driftMeasured,unwitnessedItems—both engines against the same frozen corpus, varying only
dist/. Nothing here touches astage's output, and that is measured rather than assumed.
The bucket table moves anyway, and that is the denominator, not a regression. TypeScript file
reads 23.26% over 60 files where
dd540feread 25.35% over 59, withreducedunchanged at 33 —the added file is
src/bench/fixtures/bundled-path.ts(from M10), which falls back andcontributes zero. The prose bucket 28 → 29 was already outstanding:
docs/audit-remediation-status.mdlanded in7a1b5a7, after the baseline was recorded.collect.jsrefused on both mismatches before measuring anything, andrecipe.jsonrecords eachstep with its cause.
Two false greens were hit doing that A/B, both now recorded in the status doc's traps section
because they are the same class of failure as the findings above: a build that failed left the
previous
dist/in place, so an engine was compared against itself and reported a perfect match;and a per-file diff keyed on a field the harness does not emit collapsed 594 rows onto one
undefined key and reported
compared 2 rows, differing: 0.Docs
DECISIONS.md§44 ·CHANGELOG.md·docs/audit-remediation-status.md(now records Wave 2 asdone, the new baseline, what was deliberately not done, and the residue) ·
CLAUDE.md·ROADMAP.mdquestion C answered ·README.mdenv-var table ·tools/corpus-harness/recipe.json.🤖 Generated with Claude Code