test(drift): drift guard and collector hardening (split out of #343) - #349
Draft
jpr5 wants to merge 11 commits into
Draft
test(drift): drift guard and collector hardening (split out of #343)#349jpr5 wants to merge 11 commits into
jpr5 wants to merge 11 commits into
Conversation
The collector's alert prose told the reader to "add to knownModels in ws-realtime.drift.ts". That symbol no longer exists: it was renamed to `knownVoiceModelFamilies` and moved out of ws-realtime.drift.ts into src/__tests__/drift/voice-models.ts (extracted so the live canary and its unit test share one detection path). Anyone following the Slack alert went looking for a symbol and a location that are both gone. Only the human-readable `issue` prose changes. The `path: "knownModels"` values are left alone deliberately — drift-delta keys entries by provider+id+path, so renaming the path would break delta continuity against already-recorded history (and drift-delta.test.ts / drift-collector.test.ts assert on it).
…turally 8d2320b renamed the collector's alert PROSE but stopped there, so the same two retired symbols were still being shipped to whoever reads the alert: - "Update the gaModels list in ws-realtime.drift.ts" — neither the symbol nor that location exists; it is `gaRealtimeModels` in src/__tests__/drift/voice-models.ts. - `path: "knownModels"` / `expected: "(not in knownModels set)"` / `path: "knownModels[truncated]"` — the symbol is `knownVoiceModelFamilies`. 8d2320b left the `path` values alone out of concern for delta continuity. That concern only applies to a diff with no `id`: drift-delta keys by `provider + (diff.id ?? diff.path)`, and the unknown-model diffs all set `id` to the model id, so renaming their `path` cannot move a key. Two diffs do key on path — the no-GA diff and the truncation diff — and for those the rename shifts the key once. The only effect is that a drift already recorded in the cached same-UTC-day BASE report is re-classified as new-in-head for that one day. Both are `critical` and block when genuinely new anyway, and a missing GA realtime family is a hard alarm a human acts on immediately — cheaper than permanently shipping alert text that names a symbol nobody can find. The durable part is the new guard. These strings rotted twice because nothing checked them, so drift-remediation-strings.test.ts now asserts, over the collector and the drift-proposals notes, that every "<symbol> in <file>" pair resolves to a file that really DECLARES that symbol, and that no retired name (knownModels, gaModels) is referenced at all. It carries its own vacuity guards: the cases are generated from the extraction, so it also asserts each scanned surface still yields a pair and that none of them contains a Prettier-bolded `__tests__` — the reshaping that would otherwise drop a citation from the guard silently instead of failing it. drift-collector.test.ts asserts on the collector's `path` values, so its two assertions move with the rename.
…d input The guard added in 6614bb2 required EVERY `.md` under `drift-proposals/` to yield a "<symbol> in <file>" pair. That premise is wrong: those notes are written mechanically by drift-sync, and a freshly generated one cites no symbol at all. So the guard shipped three false reds: - the next needs-human PR goes red with "prose was removed" about a note the tool just wrote correctly: AssertionError: No "<symbol> in <file>" pair was extracted from drift-proposals/openai-gpt-nova-new-family.md. Either its remediation prose was removed, or the citation was reshaped … - the >=5-pair floor was met only by note-borne pairs (the collector alone contributes 3), so DELETING a resolved note — which fb9d9c5 already does — fails it: AssertionError: expected 3 to be greater than or equal to 5 - once the last note is deleted the directory itself goes away (git tracks no empty directory) and the top-level readdirSync throws a COLLECTION error that takes all 18 tests down with it: Error: ENOENT: no such file or directory, scandir '…/drift-proposals' All three have one cause, so they get one fix: only the hand-written collector prose is REQUIRED to carry a citation, and that is what the vacuity floor is pinned to. The notes are scanned opportunistically — whatever citation one does carry must resolve, and none may name a retired symbol — but a note is never required to carry one, and an absent or empty directory is not an error. The retired-symbol scan, which previously hard-coded the collector while the docstring claimed the gap was closed generally, now covers every scanned surface. Planting `knownModels` in a note used to leave the suite green at 16/16; it now fails on that note by name. Added the verification form that was missing: a known-NEGATIVE control. The guard is run against a note built by the REAL renderProposalNote, against an empty notes directory, and against an absent one, and is required to accept all three. Every assertion here was watched to fail — forcing requiresPair() true, giving the note template a citation, dropping the try/catch, and removing one citation from the collector prose each produce the specific red they are there to catch.
drift-delta keys a failure by `provider + (diff.id ?? diff.path)`. Two realtime
canary diffs shipped with no `id` — the no-GA diff and the truncation diff — so
their delta key WAS their `path`, which is also the human-facing "Path:" line of
the alert. Renaming the prose therefore moves the key: drift already recorded in
the cached same-UTC-day BASE report is re-classified as new-in-head and BLOCKS
the PR, and the base key is reported as spuriously "fixed". Observed directly:
computeDelta(base=[path:"gaModels"], head=[path:"gaRealtimeModels"])
advisory: []
block: ["gaRealtimeModels"]
fixed: ["gaModels"]
Freezing the display string is the wrong way out — a "Path:" naming a symbol that
exists nowhere is exactly what drift-remediation-strings.test.ts exists to
prevent, and it rotted twice already. Reverting it also cannot be done: the guard
now scans the collector for retired names, so putting the old string back turns
that guard red.
So the two diffs get an explicit SEMANTIC id that never appears in prose. The key
shifts once — the same one-day cost already accepted in 6614bb2 — and after that
the display text is free to change without moving any key again. A vacuity guard
asserts EVERY canary diff carries an id, so none can silently re-acquire a
path-derived key.
RED (before): three failures, e.g.
FAIL … > the collector sets the stable id on the no-GA diff
AssertionError: expected undefined to be 'openai-realtime:no-ga-family'
The MECHANISM characterization was watched to fail by stubbing indexReport's
`diff.id ?? diff.path` to a constant.
…exist
The knownModels -> knownVoiceModelFamilies rename missed several of its own
downstream citations, and some comments cite capture artifacts that were always
throwaway and are not in the repo:
- drift-collector.test.ts named `knownModels` "in ws-realtime.drift.ts" in four
places; the symbol is `knownVoiceModelFamilies` in
src/__tests__/drift/voice-models.ts;
- three comments cited `tmp/canary-fixtures.json`,
`tmp/captured-vitest-shapes.json` and "RED/GREEN logs under tmp/". There is no
tmp/ directory and never was one in the repo — the provenance statement is
kept, the dead paths are dropped;
- drift-delta.ts's docstring used `"knownModels"` as its example of a generic
path bucket, and two test fixtures used it as a `path` value, which reads as a
real collector output.
Comment-only, plus fixture string values the delta layer does not interpret.
The retired names deliberately REMAIN in three places: the guard that lists them
as retired, and the two delta-key tests that model the historical rename in order
to demonstrate the key-move mechanism. That is why the retired-symbol scan stays
scoped to remediation-prose surfaces rather than the whole tree — widening it
would red-flag the tests that exist to document the rename.
…ance
The delta gate annotates a blocking key as `${k.provider} ${k.id}`, and
indexReport builds `id` as `diff.id ?? diff.path` — so a diff with no `id`
promotes its human-facing display string to the key. That is how `gaModels` and
`knownModels[truncated]`, symbols retired from this repo, became printable delta
keys: the annotation named a symbol the reader could not find anywhere.
98b9cf1 fixed the two id-less diffs by giving them explicit semantic ids. Nothing
prevented the next one from re-acquiring a path-derived key, so this makes the
property structural instead of a matter of remembering. Every key a
collector-CONSTRUCTED diff can contribute must have one of three provenances:
a namespaced semantic id from an explicit namespace allowlist, the value the live
API returned (the diff's own `real`), or an identifier this repo actually
declares. A bare display string has none of the three and fails.
Positive check, not a blacklist of known-bad names: the accepted cases are
enumerated, so an unforeseen display string fails by default rather than needing
to be listed. The generated per-key cases are backed by a vacuity guard (6 lanes,
>=8 diffs, both semantic ids and the WS key present) plus known-positive controls
for all three provenances, so neither an empty enumeration nor an
always-true classifier can look green.
Also covers the WS handshake lane, which had no collector test at all.
isBaseReportReusable checks that a cached base report is non-empty, known-good and same-UTC-day. It deliberately does NOT check that the base was produced by the same collector code — the base comes from origin/main. So while a PR that changes how a diff is keyed is unmerged, base and head key the same drift differently: the base key reports as `fixed`, the head key as `block`, and a non-empty block exits 1 — a hard required-check failure on a PR that introduced no drift at all. It only fires if real drift appears while such a PR is open, and it self-clears once the change is on main and the next base is collected by the new code. That was only written down in the PR description, where nobody debugging the failing check will look, so it goes at the guard whose missing check causes it, with a pointer from the keying site in indexReport.
The provenance guard exists so the drift gate can never print a symbol that does not exist at a human. Its `declared` arm scanned every `.ts` under `src/` and `scripts/` for any `const|function|class|…` declaration of the name, which made it a near-wildcard: all 20 generic one-word strings tried — `path`, `models`, `entries`, `key`, `id`, `usage`, `body`, `type`, … — are each some local `const` somewhere in the tree, so a bare display path satisfied the guard. Narrowed structurally rather than by blacklisting words: the key must name a symbol EXPORTED (`^export`, top-level) by a drift classification module (`src/__tests__/drift/*.ts`) — the files that hold the seed set or rule the annotation is pointing at. 0 of the 20 generics now pass; `gaRealtimeModels` and `knownVoiceModelFamilies` (the keys that ship) still do, and the retired `gaModels`/`knownModels` still do not. Locked in by a new negative control over all 20 words, watched RED against the old implementation before the narrowing.
…cannot steal ownership `extractProviderName` accepted a registry label found ANYWHERE in the text and ranked candidates by length alone, so a qualifier later in a title outranked the surface the title was about: "Gemini Live Transcription session" resolved to `Transcription` (13 chars) over `Gemini Live` (11), routing a Gemini Live drift at src/transcription.ts with its builder functions. It failed OPEN — a confident wrong owner, no error — and this PR's subject (transcription model families) is exactly the input that trips it. Both call sites lead with the provider label (`formatDriftReport`'s "<Provider> (<scenario>)" context, and drift describe titles), so the match is now anchored at the start; longest-first still resolves nested labels at the same anchor (Google Gemini over Gemini, Bedrock ConverseStream over Bedrock Converse). Text that does not lead with a known label is unattributable and returns null, which routes the block to the quarantine lane for human review instead of inventing an owner — fail-closed rather than fail-open. RED on the reported input plus a not-anchored control; full suite green after, including the marker-seam cases that require this fallback to stay null.
…use defect it hid The fixture claimed to be "a report shaped like what main() writes for a clean run" but invented a non-empty `entries[]` holding an entry with `diffs: []`. main() writes neither shape: entries come only from FAILED assertions, and a block parsing to zero diffs goes to the unparseable/quarantine lane, so no entry is ever written with empty diffs. A clean run writes `entries: []` — confirmed by running collectDriftEntries over an all-passing result. With the accurate fixture the guard rejects it, which is the finding the invented one concealed: `isBaseReportReusable` demands non-empty `entries[]` while the only conclusions it accepts are "clean"/"success", and "clean" is precisely the run with no entries — so a healthy main can never supply a reusable base and every PR pays for a fresh live base. The guard conflates "empty" (main is clean, the most useful base of all) with "malformed" (truncated cached JSON). Pinned as a named known defect rather than papered over, so correcting the guard turns this test RED and forces a deliberate update. The remaining fixtures in the describe now share one honest `driftingEntry()` helper; the staleness test needed a base that carries drift, since a clean one is rejected before the day is even considered.
…logical count The vacuity guard asserted that `requiredPairs.map(…)` had length `requiredPairs.length` — true of every array, so it could never fail. That left a raw-count floor of 3 as the only real check, and the required surface cites `knownVoiceModelFamilies in …voice-models.ts` twice, so 3 raw pairs are only 2 distinct citations. A raw count cannot tell "a citation was reshaped away" from "a citation is mentioned once more" — the exact silent drop this guard exists to catch. Proven: rewriting the `gaRealtimeModels` citation into a second `knownVoiceModelFamilies` one leaves the raw count at 3, so both old assertions passed while a citation the alert depends on vanished. The new form names each citation and fails on that mutation, quoting what was extracted; the vacuity floor now counts DISTINCT citations, which duplication cannot satisfy.
commit: |
jpr5
added a commit
that referenced
this pull request
Aug 3, 2026
…349) This is NOT a coverage reduction to accept on its own terms — it is a mechanism that does not work being taken out of the way of one that does. Removed from logic-pin.test.ts: the behavioural anchor on `assertNoUnclassifiedFamilies`, its partnering known-negative control, and the per-leg source pins on each live canary's skip GATE and CALL SITE (`freezes liveLeg.{openai,anthropic,gemini}` plus their structural anchors, including the "must pass the fetched `models` collection" assertion). WHY, and why not a fifth pin instead. The live text canary is a CHAIN — fetcher -> gate -> call site -> formatter -> collector. Pinning the TEXT of one link only relocates the silencing edit one frame outward, and that has now happened four times against this same anchor: 1. the skip gate (`describe.skipIf(...)` -> `describe.skip`) 2. the call site (`assertNoUnclassifiedFamilies(models, ...)` -> `models.slice(0, 0)`) 3. the report FORMAT — replacing the `formatDriftReport(...)` payload with an opaque string. Verified locally: logic-pin stayed 37/37 green, because the surviving `.toThrow(/gpt-live/)` was satisfied by vitest's OWN diff text (`opaque drift message: expected [ 'gpt-live' ] to deeply equal []`) rather than by anything production emitted. The consequence is not cosmetic: the collector routes on `/API DRIFT DETECTED:/`, so real drift with an unformatted payload lands in exit-5 quarantine instead of the exit-2 auto-fix lane — seen, misrouted, never fixed. 4. the FETCHER — `providers.ts`'s `listOpenAIModels` `return json.data.map(m => m.id)` -> `.slice(0, 0)`. Full suite green, eslint and prettier clean, canary silenced end to end. A fifth defeat is already known: an example-shaped callee anchor is beaten by `.filter(f => f === "gpt-live")`, which keeps the one asserted family and drops every other. Each fix closed the named surface and left the next one open, so the DESIGN is what is wrong, not the coverage. The replacement, built in #349, is a fetch-stubbed end-to-end harness: stub the fetch layer to return a listing containing an unclassified family and require the chain to emit a collector-routable `API DRIFT DETECTED:` report. That observes the OUTPUT of the whole chain rather than the text of one link, so silencing at the fetcher, gate, call site or formatter all fail it. The probe built to demonstrate defeat 4 is the seed of that harness. Kept intact — this PR's actual subject and its supporting structure: the four behavioural classification tests in text-drift.test.ts, the `text-drift.ts` extraction itself (those tests depend on it, and it is what keeps the live canaries out of the offline suite), the lazy `FROZEN` thunk, the frozen-surface inventory correction, the narrowed anchor claim, and the "still propagates" fix. All remaining pins verify unchanged — the three removed live-leg hashes are the only `pin:` lines touched, and nothing was re-pinned. `assertNoUnclassifiedFamilies` stays exported (the three live legs of models.drift.ts call it) with its doc corrected: it now records the wrapper as knowingly unguarded pending #349, instead of claiming an anchor that no longer exists.
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.
Split out of #343. Pre-strip sha of that branch:
146cb9d2b81017097480b0616d36b576411aa11c.#343 had accumulated 19 commits mixing two unrelated concerns. It has been stripped in place to the 8 commits that carry the actual model-family classification decision; this PR carries the other 11 — drift guard and collector/tooling hardening. Both branches are cut from the same merge-base (
9ce3107), so this is not stacked on #343.What is here
Guard/tooling hardening only. This branch touches no classification data and no pin table:
scripts/drift-delta.tsscripts/drift-report-collector.tssrc/__tests__/drift-collector.test.tssrc/__tests__/drift-delta.test.tssrc/__tests__/drift-remediation-strings.test.tssrc/__tests__/drift-scripts.test.tsHighlights:
declareddelta-key provenance actually binds. It previously scanned every.tsundersrc/andscripts/for any declaration of the name, which made it a near-wildcard — all 20 generic one-word strings tried (path,models,key,id, …) are some localconstsomewhere, so a bare display path satisfied it. Now the key must name a symbol exported by a drift classification module."Gemini Live Transcription session"resolved toTranscription(13 chars) overGemini Live(11) and routed a Gemini Live drift atsrc/transcription.ts— a confident wrong owner that failed open. Now anchored at the start, longest-first, and unattributable text fails closed to human review.Expected to be RED until #343 lands
Two live-API canaries fail on this branch, and both already fail on
main— they are inherited, not introduced here:src/__tests__/drift/models.drift.ts›live /models contains no unclassified family—['gpt-live-transcribe', 'gpt-transcribe']src/__tests__/drift/ws-realtime.drift.ts›canary: GA realtime models available—UNKNOWN_REALTIME_MODELS=gpt-transcribe,gpt-live-transcribeOpenAI shipped those two transcription families on 2026-07-28 and
maindoes not classify them yet. #343 is the PR that classifies them, so these go green once it merges. Nothing in this branch can fix them — the classification data deliberately lives entirely in #343 so the decision exists in exactly one place.Local gate on this branch:
pnpm build,pnpm lint,prettier --check .all pass.pnpm test— 4810 passed, 2 failed (one is the canary above, one is the knowntiming-replaywall-clock flake, which passes isolated).pnpm test:drift— 134 passed, 2 failed (the two canaries above). No pin failures.Verification
Each of the three guards added here was mutation-tested on this branch — the guard was reverted to its pre-fix behaviour, the target test was confirmed RED, then the mutation was reverted:
declaredlogic →"path"accepted,expected 'declared' to be null(RED)expected 'Transcription' to be 'Gemini Live'(RED), plus the fail-closed caseDRAFT — do not merge. Ordering with #343 is intentional.