fix(drift): key the delta by scenario — a new critical drift was landing in advisory - #353
Merged
Conversation
…ing in advisory The delta key was `provider::(diff.id ?? diff.path)`. `parseDriftBlock` sets `id: path`, and the SAME wire path is asserted by several scenarios of one provider — `openaiChatCompletionShape` (non-streaming text) and the tool-call shape both carry `usage.prompt_tokens`. So a drift already present on main in scenario A and a genuinely NEW critical drift on the same field in scenario B collapsed to ONE key: both-present → `advisory` → the required check PASSED on drift the diff introduced. Fail-silent, no error, nothing in `block`. Key on provider + scenario + id, JSON-encoded so no component can contain the separator and re-create the collapse. A residual collision (two assertions in the same scenario on the same path) was resolved last-wins, making the annotated `class` depend on report order: [Critical, Advisory] annotated `advisory`, the reverse annotated `critical`. The most severe class now wins, deterministically. Cached base reports stay valid: the gate imports this module from the PR checkout and indexes BOTH reports with it, so the key shape changes symmetrically, and `scenario` is a required DriftEntry field the collector always populates.
commit: |
jpr5
marked this pull request as ready for review
August 4, 2026 18:41
jpr5
added a commit
that referenced
this pull request
Aug 4, 2026
Brings in #343, #349, #351, #352, #353 and #342 so the branch reflects what 1.38.0 will actually ship. CHANGELOG.md was the only conflict. #342's encrypted-reasoning bullet arrived under [Unreleased], but #342 changed src/responses.ts and src/ws-responses.ts with no version bump, so that code ships inside 1.38.0. The bullet is folded verbatim into [1.38.0] ### Added (ahead of the OpenRouter bullets, preserving main's relative order) and [Unreleased] is left empty, so the published release notes describe every source change in the tarball. No version value changed.
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
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.
The bug: fail-SILENT — a new critical drift was classified
advisoryand the gate PASSEDscripts/drift-delta.tsbuilt the delta key as`${provider}::${id}`withconst id = diff.id ?? diff.path.parseDriftBlocksetsid: path, so the key is effectively provider + wire path — andscenario(the other half of aDriftEntry's identity) was absent from it.The same wire path is asserted by many scenarios of one provider:
openaiChatCompletionShape(non-streaming text) and the tool-call shape both carryusage.prompt_tokens(src/__tests__/drift/sdk-shapes.ts:35,81; scenarios"non-streaming text"/"non-streaming tool call",src/__tests__/drift/openai-chat.drift.ts:162,266).So a drift already present on main in scenario A absorbed a genuinely new critical drift on the same field in scenario B: one key, base-present, routed to
advisory,blockempty,process.exit(0). No error, no annotation, required check green.RED (real
computeDelta,origin/maincode)base =
[OpenAI Chat / non-streaming text / usage.prompt_tokens]head = base +
[OpenAI Chat / non-streaming tool call / usage.prompt_tokens, class critical]GREEN (same inputs, after the fix)
Bound
It only bit when the base already carried that exact provider + path. A brand-new field, a new model id, or a provider with no base drift always keyed new and blocked correctly. That is why it never showed up as a false green in practice — it needed a pre-existing drift on main to hide behind.
Second defect: order-dependent last-wins collapse
A residual collision (two assertions in the same scenario reporting the same path) was resolved last-wins, so the
classprinted on a blocking key depended on report order. Real pre/post output, both orders, same provider+scenario+id:Routing never depended on
class(the #292 invariant is untouched — routing is still key-presence only), but the annotation a human reads on a hard failure did. The most severe class now wins:critical > quarantine > advisory > none > (absent).CLASS_RANKis typedRecord<DriftClass, number>, so adding aDriftClassmember is a compile error until it is ranked.Known-negative control — no false blocks
An unchanged base/head pair still blocks nothing; scenario scoping must not turn pre-existing drift into a block. Both scenarios survive as distinct advisory keys:
Covered by a named test (
KNOWN-NEGATIVE control: an unchanged base/head pair still blocks nothing), plus afixed-direction test so a scenario disappearing from head is reported rather than silently absorbed.Cached base reports stay valid
The earlier key-shape incident in this area (
gaModels→gaRealtimeModels) invalidated the cached base because the key lived in the collector's persisteddiff.id. This change is different in kind: the delta gate importsdrift-delta.tsfrom the PR checkout and indexes both reports with it (.github/workflows/test-drift.yml, "Delta gate" step), so akeyOfchange applies symmetrically to base and head — no transitional block/fixed churn.The requirement is that every key component exists in the persisted JSON.
scenariois a requiredDriftEntryfield, populated on every collector path since the pipeline's first commit (extractScenariofalls back to the whole context string when there are no parens), so scenario-scoped keys are computable from any base reportisBaseReportReusableaccepts — and that guard already rejects anything not same-UTC-day. The transition note onisBaseReportReusablenow records this distinction.One follow-up for whoever owns
.github/**(deliberately untouched here): the gate's annotation formats a key as${k.provider} ${k.id}, so two blocked keys differing only in scenario print identically.DeltaKey.scenariois now available to include — additive, no behaviour change.Gates
pnpm build— cleanpnpm test— 169 files, 4821 tests, 0 skipped, 0 failed (notiming-replayflake this run)pnpm lint— clean;prettier --check .— cleantsc --noEmiton both tsconfigs — clean; the delta-related test files typecheck standalone (the newly requiredscenariofield surfaced one staleDeltaKeyliteral in the fix(drift): flag new voice/audio model families beyond the "realtime" substring #292 simulation stub, fixed)pnpm test:driftnot run: this is a pure, side-effect-free reduction of two JSON reports with no live surface, and the offline suite drives the realcomputeDeltadirectly. Running it would spend live API calls for no additional signal.package.json,CHANGELOG.md,.claude-plugin/*,charts/*,packages/aimock-pytest/*, or.github/**file touched.git diff origin/main..HEAD | grep -E '^[-+].*pin:'is empty.