Skip to content

test(drift): contain the DATA_FROZEN collection blast radius, and make the mirror guard catch the deprecation .sort() - #351

Merged
jpr5 merged 3 commits into
mainfrom
fix/drift-pin-blast-radius
Aug 4, 2026
Merged

test(drift): contain the DATA_FROZEN collection blast radius, and make the mirror guard catch the deprecation .sort()#351
jpr5 merged 3 commits into
mainfrom
fix/drift-pin-blast-radius

Conversation

@jpr5

@jpr5 jpr5 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Two post-merge review findings on the drift guards, fixed forward off fresh main. Both arrived after PR #343 had already merged (d835753), so this is a follow-up rather than an amendment to anything. Tests only — no product code, no version-bearing file, no scripts/, no .github/.

M1 — DATA_FROZEN.members was evaluated at module scope

FROZEN.source was made a thunk for one reason: so that one renamed or reshaped surface fails its OWN case instead of throwing during collection and taking the whole file with it. DATA_FROZEN.members, twelve lines below, kept the module-scope evaluation — the identical defect, in the twin of the surface we fixed. Vitest resolves a renamed export to undefined rather than failing the import, so [...undefined] at module scope throws at collection time.

RED (pre-fix) — rename gaRealtimeModels in voice-models.ts, run logic-pin.test.ts:

TypeError: gaRealtimeModels is not iterable
 ❯ src/__tests__/drift/logic-pin.test.ts:433:18
    members: [...gaRealtimeModels].sort(),

 Test Files  1 failed (1)
      Tests  no tests

One rename zeroed the file: 12 source pins, 7 behavioural anchors, and all 9 data pins. Baseline for that file is 29 passing.

GREEN (post-fix) — same rename, blast radius contained:

 FAIL  ... > freezes detectVoiceModelDrift
 FAIL  ... > freezes gaRealtimeModels membership
      Tests  2 failed | 30 passed (32)

Both failures are surfaces the rename genuinely moved (detectVoiceModelDrift's body references the renamed const). The rename was then edited back out: 32/32 pass.

Checked for a third module-scope surface in that file: there are exactly three module-scope bindings — srcCache (an empty Map), FROZEN (all thunks), DATA_FROZEN (now all thunks). No third defect.

While in the file, pinned FORWARD_LOOKING_FAMILIES per provider (including the two empty sets). A family listed there is dropped from the sync mirror's deprecation candidates entirely — no removal proposal, no needs-human note — so adding an id was a one-line way to stop reporting a genuinely retired family, and no guard watched it. Mutation-checked: seeding the openai set reddens that pin and nothing else.

M2 — the mirror-equivalence guard did not catch the deprecation .sort()

There are two .sort()s in the drift-sync mirror, one per mirrored function, and only one was covered. Every deprecation fixture yielded at most ONE missing family (one candidate is trivially in order), and the sole two-candidate case asserted with order-insensitive toContain.

RED (pre-fix) — drop detectDeprecatedFamiliesForSync's .sort():

src/__tests__/drift-sync-mirror-equivalence.test.ts   6 passed (6)
src/__tests__/drift-sync-core.test.ts                34 passed (34)

Fully green with the divergence in place.

This corrects the record on commit c89cf6a ("make the mirror-equivalence guard actually catch all three named divergences"), which landed on main in #343. That claim was true of two of the three, not three: the fail-closed floor and the NON_MODEL_TOKENS check were genuinely covered, and the .sort() fixture it added covers unclassifiedFamiliesForSync's sort — a different .sort() in a different function. The deprecation copy's sort was uncovered. A commit message on main cannot be rewritten, so the correction is recorded here and in the file's docstring, which now states there are two sorts and that each needs its own order-distinguishing fixture.

GREEN (post-fix) — the new fixture removes two families whose registry insertion order is not alphabetical, so sorted and unsorted missing differ, and asserts the whole result with order-sensitive toEqual:

AssertionError: expected { status: 'checked', …(1) } to deeply equal { status: 'checked', …(1) }
- Expected
+ Received
-       "family": "gpt-4.1",
+       "family": "gpt-4o",
...
-       "family": "gpt-4o",
+       "family": "gpt-4.1",

Restored: 8/8 pass. It also pins the order absolutely, so dropping the sort from both copies at once (which keeps them equivalent) still reddens — verified. The pair is derived from the registry, not hard-coded, and firstUnsortedPair throws if the registry ever becomes fully sorted rather than letting the fixture go quietly vacuous.

Re-verified the other two named divergences really are caught, post-fix:

mutation to the mirror result
drop NON_MODEL_TOKENS.has(...) line 2 failed | 6 passed
weaken the fail-closed floor to if (false) 1 failed | 7 passed
drop unclassifiedFamiliesForSync's .sort() 1 failed | 5 passed
drop detectDeprecatedFamiliesForSync's .sort() 1 failed | 7 passed (was 6 passed)

Smaller items taken in the same pass

  • mirror: the || NON_MODEL_TOKENS.has(id) half is unexercised. The comment claimed the fixture protects the token check; it protects the line as a whole. Reducing the mirror's line to has(family) alone keeps the file green, because the sole token normalizes to itself. Narrowed the comment to what is true rather than widening the code.
  • mirror: the "real isFamilyStillReferenced" title was false — that fixture's missing set is empty, so it never reaches the reference scan. Title corrected, and the case now pins { status: "checked", candidates: [] } so the equivalence cannot be satisfied by both copies going equally wrong. The new two-missing-families fixture is what exercises the real scan (it passes no isReferenced).
  • mirror: gemini was never exercised — the two exclude-by-rule lanes (PREVIEW_FAMILY, GEMMA_FAMILY) had no coverage on either copy. Added.
  • mirror: fixture identity on failure — the seven-fixture loop now names the listing in its assertion message; also dropped the re-declared local Provider in favour of the mirror's own exported type.
  • text-drift.test.ts: a toEqual([]) that a neutered detector also satisfies. It fed in the bare registry keys (which are excludeFamilies.openai by definition, so that half could never contribute) and passed under unclassifiedFamilies → return []. Now dated-only, plus a negative control of the same shape that must report. Verified: it reddens under that mutation, where before it stayed green.
  • text-drift.test.ts: the generated id did not exist. The loop derives ids from the registry keys, which are seeded through normalizeModelFamily, so text-embedding-ada-002 is stored as text-embedding-ada and the generated id was text-embedding-ada-2026-07-28. Added the real bare and dated forms, which need the -002 build tag and the date to normalize in composition.

Nothing deferred.

Gates

  • pnpm build — clean
  • pnpm test4764 passed / 4765, 0 skipped. The one failure is the known timing-replay.test.ts wall-clock flake (replaySpeed 2.0), which passes 5/5 isolated and also failed on unmodified origin/main in this worktree. Six new tests (3 + 2 + 1), consistent with the 4759 baseline.
  • pnpm lint — clean · prettier --check . — clean
  • Nothing re-pinned: git diff origin/main..HEAD | grep -E '^[-+].*pin:' shows only the three NEW FORWARD_LOOKING_FAMILIES pins and the DATA_FROZEN type signature. No existing pin line was removed or changed.
  • pnpm test:drift includes only *.drift.ts — untouched, so no live canary is in play and no provider key was used.

jpr5 added 3 commits August 4, 2026 08:47
…AMILIES

`FROZEN.source` was made a thunk precisely so that one renamed or reshaped
surface fails its OWN case instead of throwing during collection and taking the
file down. `DATA_FROZEN.members` twelve lines below kept the module-scope
evaluation: vitest resolves a renamed export to `undefined` rather than failing
the import, so a single renamed registry export made `[...undefined]` throw at
collection and reported the WHOLE file as zero tests — all twelve source pins,
every behavioural anchor, and all nine data pins. Verified: renaming
`gaRealtimeModels` took the file from 29 passing to `Tests no tests`; with
`members` lazy the same rename fails only its own pin (plus the
`detectVoiceModelDrift` source pin, whose body genuinely references it) and the
other 30 still run and pass.

Also pins `FORWARD_LOOKING_FAMILIES` (deprecation-detector.ts) per provider,
including the two empty sets. A family listed there is dropped from the sync
mirror's deprecation candidates entirely — no removal proposal, no needs-human
note — so adding an id was a one-line way to stop reporting a genuinely retired
family, and no guard watched it. Mutation-checked: seeding the openai set reddens
its pin and nothing else.
…orrect the coverage claim

There are TWO `.sort()`s in the drift-sync mirror, one per mirrored function, and
the guard only covered one of them. Every deprecation fixture produced at most
ONE missing family, and the sole two-candidate case asserted with
order-INSENSITIVE `toContain`, so dropping
`detectDeprecatedFamiliesForSync`'s `.sort()` left this file 6/6 green and
`drift-sync-core.test.ts` 34/34 green. The docstring's claim to catch "all three
named divergences" was therefore true of the unclassified-families sort only.

Adds a fixture that removes two families whose registry INSERTION order is not
alphabetical, so sorted and unsorted `missing` differ, and asserts the whole
result object with order-sensitive `toEqual` plus an absolute order pin (so
dropping the sort from BOTH copies at once, which keeps them equivalent, still
reddens). The pair is derived from the registry rather than hard-coded, and
`firstUnsortedPair` throws if the registry ever becomes fully sorted rather than
letting the fixture go quietly vacuous.

Re-verified the other two named divergences DO red: dropping the
`NON_MODEL_TOKENS` check and weakening the fail-closed floor each fail. Narrowed
the token-fixture comment to what it actually covers — the line as a whole, not
its `|| NON_MODEL_TOKENS.has(id)` half, which is unreachable while the sole token
normalizes to itself.

Also: exercise `gemini` (the two exclude-by-rule lanes had no coverage on either
copy), name the fixture in the loop's assertion message, drop the re-declared
local `Provider` in favour of the mirror's own exported type, and make the
"healthy listing" case honest — its `missing` set is empty, so it never reaches
`isFamilyStillReferenced`; the new two-missing-families fixture is what exercises
the real reference scan.
…and cover the real ada-002 id

Two problems in the enumerate-every-exclude-family case. It fed in the BARE
registry keys alongside the dated ones, and those are `excludeFamilies.openai`
by definition, so that half of the payload could never contribute a result. And
`toEqual([])` is precisely what a neutered `unclassifiedFamilies` (`return []`)
produces, so the assertion passed under the mutation it looks like it guards.
Now only the dated ids go in, plus a negative control of the same shape that MUST
report. Verified: with `unclassifiedFamilies` neutered to `return []` this test
reddens, where before it stayed green.

Also adds the id form OpenAI actually lists. The loop derives ids from the
registry keys, which are seeded through `normalizeModelFamily`, so
`text-embedding-ada-002` is stored as `text-embedding-ada` and the id the loop
generated for it was `text-embedding-ada-2026-07-28` — a string that does not
exist. The real bare and dated forms, which need the `-002` build tag and the
date to normalize in composition, were exercised nowhere.
@pkg-pr-new

pkg-pr-new Bot commented Aug 4, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@copilotkit/aimock@351

commit: 4f845ae

@jpr5
jpr5 marked this pull request as ready for review August 4, 2026 17:23
@jpr5
jpr5 merged commit 96503ca into main Aug 4, 2026
28 checks passed
@jpr5
jpr5 deleted the fix/drift-pin-blast-radius branch August 4, 2026 17:24
jpr5 added a commit that referenced this pull request Aug 4, 2026
…unreadable file list is not "note absent"

Two fail-silent defects in the CANDIDATE SELECTION around the PR-body marker
repair (the repair itself is unchanged).

H-F3. `^fix/drift-` is not a bot marker. Humans use `fix/drift-<slug>` for drift
work — #348 `fix/drift-alert-signal`, #351, #352, which is exactly the hand-off
the needs-human PR body asks a human to perform. So the note-path anchor selected
HUMAN PRs: the step appended machine markers to prose it does not own, and folded
that human PR into the bot's dedup set, where it suppressed a genuine needs-human
PR indefinitely. Candidacy now requires the bot branch shape both branch builders
emit — prefix plus date, run id and a trailing 16-hex changeset key — which is the
signal the ok-applied dedup already relied on, generalised from "this run's key"
to "a changeset key" so a bot PR carrying a DIFFERENT key (precisely what the
note-path anchor exists for) still qualifies. A guard pins the pattern against
BOTH branch-construction sites, so changing either shape reds instead of silently
selecting nothing.

H-F1. The note-in-files question was answered by TWO independently written jq
expressions that had to agree, the second ending `jq -e … >/dev/null 2>&1` — so a
jq fault, a malformed payload, or gh's `first: 100` file-list truncation ALL read
as "note not present", which is the answer that opens a duplicate PR. One jq
definition now answers it once and carries its verdict forward with the candidate
(`keyAnchored`, `wantMarkers`), so nothing downstream re-derives it; an unreadable
or truncated file list on a bot candidate is reported and FAILS CLOSED instead of
being read as absence.

The note-path anchor previously had no guard anywhere: it could be killed three
ways with the whole suite green. It now has executable ones — the step's own jq
programs and its whole NOTES_JSON…REASSERTED section are run against fixture
payloads and what they select is measured, so forcing the index check false,
rewiring the notes argjson to `[]`, dropping the anchor, or reverting the bot
pattern each red.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant