feat(drift): ironclad drift-detection hardening (Phases 1-4)#297
Merged
Conversation
… and quarantine[]
Extract the OpenAI voice-model dated-snapshot/build-tag family-strip loop into a side-effect-free normalizeModelFamily(id, provider) shared core. The provider argument is a forward hook; the strip is applied identically for all three providers today, so normalizeModelFamily(id, "openai") is byte-identical to normalizeVoiceModelFamily(id).
…imitive normalizeVoiceModelFamily(id) is now a thin wrapper over normalizeModelFamily(id, "openai"); the duplicated dated-snapshot/build-tag strip loop moves to the shared model-family primitive. Behavior-preserving: knownVoiceModelFamilies is byte-identical and all voice-model unit tests plus the ws-realtime drift selection stay green. Adds a characterization test asserting the wrapper is byte-identical to the shared primitive.
…ad of throwing (exit 5)
…ode/collector (A1.4)
…opagate immediately
Add EXIT_QUARANTINE=5 constant and quarantine?: boolean field on RetryResult.
retryUntilStable now recognises exit 5 as a distinct terminal outcome: it does
not retry (unlike exit 2) and marks quarantine:true on the result. The crash
branch remains for all other non-{0,2,5} codes. main() already propagates the
exit code via process.exit(result.exitCode) so exit 5 reaches the caller.
Test: inject collector→5; RED (pre-change) quarantine field was undefined;
GREEN asserts exitCode===5, quarantine===true, calls===1.
…workflow
Add elif branch for EXIT_CODE==5 in the collector step. Exit 5 means the
collector quarantined unparseable output — it is not a script crash, so the
workflow should continue rather than abort. Exit 1 (and all other non-{0,2,5}
codes) remain fatal. The exit-2-only auto-fix gate (step 2 check) is unchanged.
Red/green shell-harness: pre-change EXIT_CODE=5 routed to abort (fell into
the non-0 catch-all); post-change routes to continue.
Add side-effect-free model-registry.ts exporting per-provider include/ exclude family Sets (seeded through normalizeModelFamily so membership is normalization-consistent and idempotent) plus a provider-agnostic NON_MODEL_TOKENS allowlist carrying gemini-interactions. include = text-generation families aimock mocks (derived from conformance tests/README/fixtures); exclude = retired/preview/non-text/voice families. The union is what the drift check (B4.2) subtracts against normalized live /models; leftover families are the drift signal. Unit test proves registry shape: include membership via normalized id, NON_MODEL_TOKENS carries gemini-interactions, no family on both include and exclude, and seeds are idempotent under normalization.
… workflow
Add elif branch for EXIT_CODE==5 in the collector step of the drift job. Exit 5
emits a distinct quarantine-labelled ::error:: annotation ("quarantined
unparseable output — manual triage required") and re-exits 5 via the existing
exit "$EXIT_CODE" path (O1). The generic crash message is reserved for all
other non-{0,2,5} codes. The exit_code==2 fail step is untouched.
Red/green shell-harness: pre-change EXIT_CODE=5 emitted the generic "crashed"
message; post-change emits the quarantine label and still exits 5.
Delete the prose-scrape path (scrapeModels, sourceFiles, GEMINI_MODEL_PATTERN, filterStableGeminiModels, AIMOCK_GEMINI_PROVIDER_MODES) that caused incident 5. Each provider block now fetches live /models, normalizes every id to its family via normalizeModelFamily, subtracts includeFamilies ∪ excludeFamilies and NON_MODEL_TOKENS, and surfaces the unclassified remainder as a formatDriftReport-wrapped failing assertion so the collector routes it (exit-2 auto-fix / exit-5 quarantine). Adds a no-key regression suite exercising the real pipeline: incident-2 dated snapshots of known families produce zero delta; a prose provider-mode token can never enter; a genuinely new family still fires.
…eModelFamily
Anthropic dates its model ids with an undelimited -YYYYMMDD suffix
(e.g. claude-3-5-sonnet-20241022) rather than the dashed -YYYY-MM-DD
form the shared core strips. Add an anthropic-only /-\d{8}$/ strip via
the existing provider hook, applied inside the same idempotent reduction
loop, so dated Claude ids collapse onto their family. openai/gemini keep
only the shared core so a non-date 8-digit tail is never over-stripped.
Re-add the Anthropic dated-id case B4.2 removed to the incident-2 injected-payload regression: claude-3-5-sonnet-20241022, claude-3-7-sonnet-20250219, claude-3-5-haiku-20241022 now normalize onto their included families and assert ZERO unclassified delta, guarding the incident-2 class for Anthropic's contiguous -YYYYMMDD snapshots.
…ape regression suite (B4.3) Extend model-registry.test.ts with the §6.1c cross-check (Part 2): a static table of model ids referenced by aimock's DEFAULT_MODELS (server.ts), conformance tests, and fixture-blocks tests, organised by provider. Every entry must resolve via normalizeModelFamily to a family in includeFamilies (aimock mocks it) or excludeFamilies (retired/non-text/preview). The gemini-interactions provider-mode token must appear on NON_MODEL_TOKENS. A stray builder id or dropped allowlist entry fails the unit test before it can cause a live crash in the drift job. Delete models-scrape.drift.ts: its two tests exercised scrapeModels, filterStableGeminiModels, and sourceFiles which were deleted in B4.2 (they tested the now-gone scrape path). Both tests were already failing with TypeError. grep -rn "models-scrape" src/ confirms no remaining importers.
Introduce `InfraError extends Error` with a first-class readonly `status: number` field. Update `assertOk` to throw `InfraError(msg, status)` and `withInfraErrorTag` to propagate the numeric status on re-wrap, so downstream classifiers (Slack alerts C5.2, preflight C5.3a) can key off `status === 401 || 403 → stale-key` vs `429/5xx → infra-transient` without any prose-message parsing. Network errors that never reach assertOk have no HTTP status; they remain plain Errors with status 0 on the InfraError wrapper.
…tReport
Upgrade summarizeDriftReport to:
- Compute a headline class (real-drift / stale-key / infra-transient /
quarantine / test-infra-false-positive) from InfraError status, report
entries severity, and quarantine[] presence. Priority: stale-key >
infra-transient > real-drift > quarantine > test-infra-false-positive.
Critical wins over quarantine (mirrors computeExitCode contract).
- Emit per-item detail: provider / id-or-path / builderFile / issue text.
- Render a distinct quarantine section for report.quarantine[] entries,
with testName + rawLocation (file:line) + truncated message.
- Accept optional SummarizeOptions { infraErrorStatus?, exitCode? } for
classification context; zero-opt callers and empty reports preserve the
historical empty-string return.
- Export HeadlineClass and SummarizeOptions types for downstream use.
The drift_summary GITHUB_OUTPUT key is unchanged — callers write the full
return value verbatim; the workflow's DRIFT_SUMMARY env-var receives the
augmented mrkdwn string. Verified: test-drift.yml reads only
steps.summary.outputs.drift_summary (line 42) via writeGithubOutput, which
is invoked unchanged in main() with the summary as the sole value.
Golden-file snapshot tests cover all five classes (real-drift, stale-key,
infra-transient, quarantine, test-infra-false-positive), per-item id
preference over path, quarantine distinct-line format, and mixed
entries+quarantine. 10 new tests; all 4386 tests pass, tsc clean, lint clean.
Add a preflight step at the start of the `drift` job: a cheap GET /models per provider (OpenAI/Anthropic/Gemini) before the 15-minute suite. A dead or revoked key returns 401/403, which the step classifies as `stale-key` and fails LOUD with a "rotate <PROVIDER>_API_KEY" message — never a silent green skip — so a dead key surfaces up front instead of as a late generic streaming failure. Classification keys off the numeric InfraError.status (401/403), not the prose message. Transient statuses (429/5xx/network) are not stale keys: the preflight warns and continues, since the retry-wrapped collector handles those. The preflight program is written to the checkout root before running so its relative provider import resolves against the repo (ESM relative specifiers key off the module location, not cwd); cleaned up on exit.
…fy quarantine class Promote the drift collector's exit_code from a step output to a drift-job JOB-level output (alongside summary/runs) so the separate notify job can read needs.drift.outputs.exit_code. In the notify job, classify exit_code==5 (collector quarantined unparseable output) as a distinct quarantine alert BEFORE the generic DRIFT_RESULT==failure -> HTTP_DRIFT=true fallback. Without this, an exit-5 quarantine (drift job concludes failure) was misreported as real HTTP API drift (providers changed formats), sending readers chasing a phantom format change instead of doing the manual triage a quarantine needs. Exit 2 (real critical drift) and normal failures still route to the HTTP-drift/real-drift path unchanged. M5 backstop per ironclad spec 6.4.
Set `id` on every ParsedDiff produced by the collector so downstream delta logic (D6.1) can key findings by provider+id: - parseDriftBlock: `id = path` (stable per-item key from the offending field path; different paths → different delta keys) - canary CLASS 3 (unknown-models): `id = <model-id>` (the value already in `diff.real`), so N distinct unknown model ids → N distinct delta keys rather than collapsing under a shared `path:"knownModels"` bucket - canary CLASS 2 / noGA unknownIds spread: same `id = <model-id>` treatment Additive only — no existing field removed; exit-code/quarantine logic (computeExitCode, collectDriftEntries quarantine path) is untouched. RED: 3 unknown model ids → all diffs had `id === undefined` → 1 collapsed key GREEN: 3 unknown model ids → 3 distinct `id` values → 3 distinct delta keys
Adds scripts/drift-delta.ts: a pure, side-effect-free delta layer that reduces a base (main) report and head (PR) report to block/advisory/fixed keyed by provider+per-item-id. Routing is by key presence ONLY — DriftClass is annotation and never enters the block/advisory decision: - new-in-head (absent from base) -> block (diff-attributable) - present in both -> advisory (environmental/world drift) - base-only -> fixed (informational) isBaseReportReusable (O-2) guards cached-base reuse: non-empty entries + known-good conclusion + same-UTC-day. Includes the M-1 golden regression (#292): a real-drift/critical finding new-in-head MUST block regardless of class (the old class-routed rule would have greenlit it).
Add a new drift-live-pr job to test-drift.yml that runs the live drift collector on both base (main) and head (the PR ref), producing two report artifacts. Base reuses the most recent same-UTC-day well-formed main scheduled report via isBaseReportReusable (D6.1), else runs a fresh live base. Triggered on pull_request (NOT pull_request_target, O3) so fork PRs run without secrets; path-filtered to drift scripts, drift tests, and drift workflows. This is the skeleton only — the delta comparison / block-vs- advisory step plugs in at the D6.3b SEAM marker.
…e-present Plug the D6.3b delta step into the drift-live-pr job at the SEAM. The step loads drift-report-base.json + drift-report-head.json and runs computeDelta (scripts/drift-delta.ts) via a quoted-heredoc tsx invocation, matching the preflight step's proven pattern. Routing is by KEY PRESENCE only (#292 invariant), never by DriftClass: - block[] (new-in-head) → ::error:: annotation + exit 1 (required check fails) - advisory[] (base-present) → ::notice:: neutral annotation + summary, exit 0 - fixed[] → informational summary line only Also documents the fork→maintainer-dry-run fallback in-job: fork PRs run without secrets, so a maintainer runs the delta dry-run from a trusted checkout and records the block/advisory conclusion.
…ndered binary) Replace the \x00 byte separator in keyOf() with '::' — a printable separator that cannot appear in a provider name or model id. The NUL caused git to classify scripts/drift-delta.ts as binary (Bin diff), making the file unreadable in any diff or review. Behavior-preserving: both base and head key through the same keyOf(), so equality/presence comparisons are unchanged.
commit: |
The drift-live-pr required check hard-failed on every drift-code PR: the
head-leg step ran drift-report-collector.ts directly and let the collector's
exit 2 ("critical drift found") propagate as a step failure, so the delta
gate — the step that actually decides block-vs-advisory by key presence — was
skipped and the check died for the wrong reason. A dead configured key also
went silently green because the PR leg had no key-freshness preflight.
Fix, scoped entirely to the workflow:
- Add a PR-scoped per-provider key-triage preflight. No key configured for a
provider -> SKIP that provider's live leg with a neutral ::notice:: advisory
(not a failure; the drift suite's own describe.skipIf already drops the
tests). Key present but 401/403 -> loud ::error title=stale-key:: and block
(a configured-but-dead key stays a real failure, per the InfraError.status
contract). Transient (429/5xx/network) warns and continues. Records
live_legs_ran for downstream steps.
- Base and head legs: capture the collector exit code and treat exit 2
(drift-present) as non-fatal — the report file is the signal, the delta gate
interprets it. Exit 5 (quarantine) and any other non-zero still fail the leg.
- When no provider key had a usable credential (fork PR / all keys absent),
write empty well-formed base/head reports and neutral-pass the delta gate
with a 'live drift leg skipped — no keys' advisory (maintainer-dry-run
fallback applies).
Delta blocking behavior is unchanged when keys exist: a new-in-head critical
still fails the required check.
jpr5
force-pushed
the
drift-ironclad-impl
branch
2 times, most recently
from
99b0922 to
8c7fd88
Compare
jpr5
added a commit
that referenced
this pull request
Jul 15, 2026
…s non-fatal (#298) ## The bug The `drift-live-pr` job's **head leg** let the drift collector's exit code 2 (critical drift found) propagate as a **step failure**. But exit-2 is the *expected* "drift present" signal that must feed the delta gate — it has to be **non-fatal** for the leg so the delta gate can decide block-vs-advisory by key presence. GitHub Actions runs `run:` blocks under `bash -e`. The head-leg step's `set -uo pipefail` line does **not** clear that `-e`, and there was no `set +e` guard around the collector call. So the instant the collector returned 2, `-e` aborted the step at the `npx` line — before `HEAD_EXIT=$?` was even captured — and the delta gate never ran. On PR #297 the head leg logged `Exiting with code 2 (critical diffs found)` then `##[error]Process completed with exit code 2`. The prior fix (#297) only hardened the *base* leg (which already wrapped the collector in `set +e`/`set -e`) and the no-keys degradation path; the keys-present head-leg live run still propagated exit-2. Makes drift-live-pr functional on drift-code PRs (was merged non-functional in #297). ## The fix (scoped only to `.github/workflows/test-drift.yml`) Both the base-leg and head-leg keys-present (`LIVE_LEGS_RAN=true`) live-run steps now capture the collector's exit code with `set +e`/`set -e` and treat it as **data**: - **exit 0** → clean, continue. - **exit 2** → drift present → **non-fatal**, continue (the `drift-report-{base,head}.json` report feeds the delta gate). - **exit 5** (quarantine) → genuine fault → **fail the leg** (explicit classification). - **any other non-{0,2}** → genuine collector fault → **fail the leg**. Report filenames/paths are unchanged (`drift-report-base.json` / `drift-report-head.json`), so the delta gate contract is untouched. The head leg was the actual #297 bug (it lacked the `set +e` guard); the base leg already tolerated exit-2, and gains only the explicit exit-5 classification for symmetry. ### Diff hunk ```diff @@ head leg ("Head drift report — run live on PR ref") @@ + # ... capture exit as DATA (see comment) ... + set +e npx tsx scripts/drift-report-collector.ts --out drift-report-head.json HEAD_EXIT=$? + set -e + if [ "$HEAD_EXIT" -eq 5 ]; then + echo "::error::Head collector quarantined unparseable output (exit 5) — manual triage required" + exit "$HEAD_EXIT" + fi if [ "$HEAD_EXIT" -ne 0 ] && [ "$HEAD_EXIT" -ne 2 ]; then echo "::error::Head collector faulted (exit $HEAD_EXIT) — not a drift signal" exit "$HEAD_EXIT" fi echo "head: collector exit $HEAD_EXIT (0 clean / 2 drift-present — both non-fatal here)" ``` (The base leg gains the same explicit `exit 5` classification block ahead of its existing `-ne 0 && -ne 2` fault check.) ## Red-green proof (keys-present live-run path) The harness programmatically extracts the **real** base-/head-leg `run:` bash from the YAML (python YAML parse — not hand-reconstructed), then drives it under GitHub's exact default shell (`bash --noprofile --norc -e -o pipefail`) with a fake `npx` collector on PATH that writes the expected `drift-report-{base,head}.json` and returns a configurable exit code. `LIVE_LEGS_RAN=true` forces the keys-present branch. ### RED — current `main` YAML, head leg, fake collector exit 2 (reproduces #297) ``` === RED: HEAD leg, current main YAML, fake collector exit 2 === ===== STEP STDOUT ===== fake-collector: writing report, exiting 2 (critical diffs found) ===== STEP STDERR ===== ===== STEP EXIT CODE: 2 ===== REPORT PRESENT: drift-report-head.json (103 bytes) HARNESS RC (RED): 2 ``` The step FAILS with exit 2 (the `head: collector exit ...` line never prints — `-e` killed the step at the `npx` line). This is the #297 failure. ### GREEN — fixed YAML, head leg ``` === HEAD leg, fake collector exit 2 === fake-collector: writing report, exiting 2 (critical diffs found) head: collector exit 2 (0 clean / 2 drift-present — both non-fatal here) ===== STEP EXIT CODE: 0 ===== REPORT PRESENT: drift-report-head.json (103 bytes) HARNESS RC (head exit 2): 0 === HEAD leg, fake collector exit 5 === fake-collector: writing report, exiting 5 (critical diffs found) ::error::Head collector quarantined unparseable output (exit 5) — manual triage required ===== STEP EXIT CODE: 5 ===== HARNESS RC (head exit 5): 5 === HEAD leg, fake collector exit 0 === fake-collector: writing report, exiting 0 (critical diffs found) head: collector exit 0 (0 clean / 2 drift-present — both non-fatal here) ===== STEP EXIT CODE: 0 ===== HARNESS RC (head exit 0): 0 ``` - exit 2 → step **succeeds** (rc captured, non-fatal), report present, control reaches the post-collector line (delta gate runs). - exit 5 → step **still fails** the leg (quarantine). - exit 0 → clean. ### GREEN — fixed YAML, base leg (same three) ``` === BASE leg, fake collector exit 2 === base: no reusable same-UTC-day main report — running fresh live base fake-collector(base): writing report, exiting 2 ===== STEP EXIT CODE: 0 ===== REPORT PRESENT: drift-report-base.json (103 bytes) === BASE leg, fake collector exit 5 === fake-collector(base): writing report, exiting 5 ::error::Base collector quarantined unparseable output (exit 5) — manual triage required ===== STEP EXIT CODE: 5 ===== === BASE leg, fake collector exit 0 === fake-collector(base): writing report, exiting 0 ===== STEP EXIT CODE: 0 ===== ``` ## Quality - `actionlint` clean on my changed steps. (The one reported SC2086 is at the pre-existing notify-job "Check previous run status" step — not part of this change.) - `pnpm run format:check` — clean. - `pnpm run lint` — clean. - `git status` — only `.github/workflows/test-drift.yml` modified. Harness is a scratch tool under `/tmp`, not committed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
Implements the reviewed aimock Drift-Detection Ironclad hardening spec: https://app.notion.com/p/39d3aa38185281c582b1d819c2842518
Built via 4 gated waves (17 micro-tasks), each red-green against the real failure surface, per-wave reviewed to zero, plus a final full-branch integration CR. 21 files, ~+2238/-288. 224 drift tests green.
What changed, by wave
computeExitCode(crit>0→2, quarantine>0→5, agUiSkipped→1, else 0);collectDriftEntriesnow quarantines unmapped/unparseable failures (capturing file:line) instead of throwing — the exit-1 crash that hid incident fix: latency chunkSize replaySpeed logLevel in startFromConfig #294's root cause is gone. Report schema gainsDriftClass/per-itemid/class/quarantine[]. SharednormalizeModelFamily(id, provider)primitive (voice-models migrated onto it, behavior-identical).drift-retry+ both workflows thread exit-5 (quarantine → run fails, no auto-fix abort). Model identity no longer scrapes prose/source (the incident root cause): enumerate live/models→ normalize → subtract curated include/exclude → unclassified delta is the signal, with an LLM-triage seam.models-scrape.drift.tsretired.InfraErrorcarries a structured HTTPstatus; Slack summary emits a closedclassenum (real-drift / false-positive / infra-transient / stale-key / quarantine) with per-item id + file:line; key-freshness preflight fails loud on 401/403 (never a green skip); notify routes exit-5 → quarantine before the HTTP-drift fallback.drift-live-prjob (pull_request, path-filtered, no secrets on forks) runs base+head and blocks only on failures the PR introduces (new-in-head), regardless of class — a drift-code PR is judged on "did your change break the detector," not "is the world drifting today." Base-present failures are advisory. Fork PRs use a maintainer dry-run.Bugs the review process caught (not shipped)
-YYYYMMDDnormalize gap — dated Claude snapshots wouldn't collapse to their family → would false-positive (incident-2 class, untested provider). Fixed via the provider hook.real-driftwould've been advisory → would have re-greenlit the exact fix(drift): flag new voice/audio model families beyond the "realtime" substring #292 incident. Fixed to gate purely on new-in-head.drift-delta.tsrendered the module binary/unreviewable in diffs — caught only by the final full-branch CR.Follow-up before merge
/modelsprovider blocks were skipped in the impl env (no keys). A keyed CI run is the remaining real-surface validation.DRAFT — awaiting review before merge to main.