Skip to content

Commit 6a5e679

Browse files
authored
fix(drift): require real drift resolution, not fixture relaxation, before PR/merge (#310)
## The hole `scripts/fix-drift.ts`'s abort guard accepted any auto-fix run that changed a file under `src/` (`builderFiles.length > 0 || testFiles.length > 0`) and exited 0. The drift tests are three-way triangulations (SDK vs real API vs mock), and the **SDK leg is literally the repo fixture** `src/__tests__/drift/sdk-shapes.ts`. So the LLM could "resolve" drift by **relaxing that fixture**: deleting the field makes the SDK leg null → the `critical` branch is unreachable → the collector re-reports clean (exit 0) with the production mock **untouched** → WS-1's hardened merge gate sees green CI and auto-merges. Re-running the collector alone can't catch this because its own SDK leg reads the relaxed fixture. ## The fix — a 3-signal predicate New `scripts/drift-success-predicate.ts` (pure `evaluateDriftResolved()` + thin CLI). `resolved: true` requires ALL of: 1. **Authoritative** — post-fix collector re-run clean (`exit 0` AND `criticalCount 0`). Distinct exits 2/5/1 → `STILL_DIRTY` / `QUARANTINE_AFTER_FIX` / `COLLECTOR_INFRA`. 2. **Production change present** — ≥1 file under `src/**` excluding `src/__tests__/`. A relaxation NEVER changes one. Empty → `NO_PRODUCTION_CHANGE`; only comparison-leg files → `COMPARISON_LEG_ONLY` (the headline cheat). 3. **No standalone comparison-leg relaxation** — a gameable-leg edit (`sdk-shapes.ts` / `providers.ts` / `ws-providers.ts` / harness `helpers.ts`) must be accompanied by a production change; and `schema.ts` (its `ALLOWLISTED_PATHS`) or any `*.drift.ts` assertion edit is **always** blocked as `SUPPRESSION_SUSPECTED`, even alongside a production change. Legit fixture targets (`model-registry.ts`, `model-family.ts`, `voice-models.ts`) are sanctioned only via the report's `builderFile`/`typesFile` (canary routing). An off-target production change → `PRODUCTION_CHANGE_OFF_TARGET` (WARNING, still blocks). Distinct exit codes (0, 10–16, 2) route each failure to a named needs-human Slack reason. ### Wiring - `fix-drift.ts` `createPr()`: the predicate replaces the weak guard and runs **before any `git add`/commit** (blocked verdict → no PR → no merge). Legacy exit-4 guard retained as a fallback when the workflow doesn't pass `--post-fix-report`/`--post-fix-exit`. - `.github/workflows/fix-drift.yml`: fresh authoritative re-collect to `drift-report.post-fix.json`, uploaded as an artifact; new "Assert drift truly resolved" step; new reasons wired into the Slack `case` block. - Fixed a latent bug in the assert step: `PIPESTATUS[0]` after a `VAR="$(pipeline)"` assignment reports the assignment's status (always 0), not the predicate's — so the step would never have blocked a cheat. Now the predicate runs to `predicate.log`, its real exit is captured directly, and the reason is grepped from the log. ## Red-green proof Unit suite (`src/__tests__/drift-success-predicate.test.ts`, 28 tests, no LLM): **RED** — stubbing `evaluateDriftResolved` to always accept (== the old `testFiles>0` guard) fails all 10 cheat-detection cases, including the headline: ``` ❯ src/__tests__/drift-success-predicate.test.ts (28 tests | 10 failed) × HEADLINE: fixture-relaxation-only (sdk-shapes.ts) → COMPARISON_LEG_ONLY, and the OLD guard would have ACCEPTED it × schema/allowlist edit + real builder change → SUPPRESSION_SUSPECTED × *.drift.ts assertion loosened only → SUPPRESSION_SUSPECTED × no changes at all → NO_PRODUCTION_CHANGE × production change but collector still dirty (exit 2) → STILL_DIRTY ... (10 total) ``` **GREEN** — real implementation restored: 28/28 pass. **CLI transcript** (the exact cheat vs a real fix): ``` ############## RED: fixture-relaxation-only (sdk-shapes.ts) ############## DRIFT NOT RESOLVED [comparison-leg-only]: Fix changed ONLY comparison-leg files (src/__tests__/drift/sdk-shapes.ts) with no production mock-builder change — this relaxes the drift detector instead of fixing the mock. The exact cheat this gate blocks. Offending files: src/__tests__/drift/sdk-shapes.ts reason=comparison-leg-only RED exit code: 11 ############## GREEN: real production fix (src/helpers.ts) ############## Drift genuinely resolved: post-fix collector clean, real production change (src/helpers.ts), no comparison-leg relaxation. reason=resolved GREEN exit code: 0 ``` Full suite (`pnpm test`): 4533 passed / 44 skipped. `prettier --check`, `eslint`, `tsc --noEmit`, `pnpm build`, `actionlint` (incl. shellcheck) all green. ## Out of scope — residual WS-2b (flagged) **Hybrid gaming**: a fix that relaxes `sdk-shapes.ts` AND makes a token edit to the *named* production builder passes both signal 1 (collector clean — its own SDK leg reads the relaxed fixture) and signal 2 (production change present). Fully closing this needs an **SDK leg sourced independently of the repo fixture** (e.g. generated from the vendored SDK's `.d.ts` at collect time). That is tracked as WS-2b and is out of this PR's scope; `SUPPRESSION_SUSPECTED` on schema/allowlist plus the OFF_TARGET warning are the partial mitigations here. --- ## Update (CR fix round 1, 75c42ab) — WS-2b CLOSED for auto-merge The "residual WS-2b" hybrid above is now **closed for the auto-merge path**. The predicate no longer ignores a gameable-leg edit when a production file also changed: **any** edit to a gameable leg (`sdk-shapes.ts`, the real-API/harness files incl. `voice-models.ts`, `providers.ts`/`ws-providers.ts`, `schema.ts`, `*.drift.ts`) now blocks **regardless** of how many production files also changed. So relaxing `sdk-shapes.ts` + a trivial on-target production edit → needs-human (`SUPPRESSION_SUSPECTED`), never auto-merge. The independently-sourced SDK leg remains a good future hardening for the *detection* side, but is no longer required to keep the hybrid cheat out of auto-merge — a leg edit can never reach a PR/merge unattended. Also in this round: dual-classified `voice-models.ts` blocks (precedence); empty sanctioned-target set fails closed; `--changed-file` lists are cross-checked against git (fail-closed on mismatch); the no-post-fix-args legacy PR path is removed (fail-closed); exit 5/1 get distinct reasons ahead of `criticalCount`; `readReport` rejects a missing timestamp. Real-predicate red→green locks added (WS-2b headline flips resolved→SUPPRESSION_SUSPECTED) plus a revert guard.
2 parents 0dbb6ae + 71856a3 commit 6a5e679

10 files changed

Lines changed: 3232 additions & 1148 deletions

.github/workflows/fix-drift.yml

Lines changed: 196 additions & 170 deletions
Large diffs are not rendered by default.

.gitignore

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,5 +16,8 @@ docs/plans/
1616
docs/specs/
1717

1818
# fix-drift.ts workflow artifacts (regenerated each run; uploaded via fix-drift.yml)
19+
# The workflow writes these to $RUNNER_TEMP (outside the checkout) so the
20+
# drift-success predicate's `git status` scan never sees them; this glob is
21+
# belt-and-suspenders for local runs and covers the post-fix / pinned variants.
1922
claude-code-output.log
20-
drift-report.json
23+
drift-report*.json

scripts/ci-merge-gate.sh

Lines changed: 0 additions & 327 deletions
This file was deleted.

scripts/drift-success-predicate.ts

Lines changed: 882 additions & 0 deletions
Large diffs are not rendered by default.

scripts/fix-drift.ts

Lines changed: 250 additions & 45 deletions
Large diffs are not rendered by default.

src/__tests__/ci-merge-gate.test.ts

Lines changed: 0 additions & 592 deletions
This file was deleted.

src/__tests__/drift-success-predicate.test.ts

Lines changed: 1400 additions & 0 deletions
Large diffs are not rendered by default.
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
/**
2+
* FIX #F5 (round-4) — createPr's STRAGGLER fail-closed guard, in isolation.
3+
*
4+
* On a RESOLVED verdict every changed file MUST fall into a gated commit group
5+
* (production builder, report-named fixture target, or skills/). A "straggler"
6+
* (a changed file in none of those groups) means the predicate verdict and the
7+
* staging partition have DIVERGED — staging would silently drop it and ship an
8+
* incomplete fix behind a green verdict. createPr must exit UNSANCTIONED_CHANGE
9+
* and stage NOTHING.
10+
*
11+
* By construction with the CURRENT predicate + gatedCommitFiles there is no file
12+
* that both PASSES the predicate and becomes a straggler (the allowlist blocks
13+
* everything gatedCommitFiles would not classify). This guard is therefore
14+
* defense-in-depth against a FUTURE predicate change. To lock it load-bearingly
15+
* we mock `evaluateDriftResolved` to force a RESOLVED verdict while git reports a
16+
* straggler in the working tree, then assert createPr fail-closes before any
17+
* `git add`. This is a pure-function/mock test — it never runs the autofix
18+
* subprocess.
19+
*/
20+
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
21+
22+
import type { DriftReport } from "../../scripts/drift-types.js";
23+
24+
// Mock git so gitChangedFiles()/exec() are controllable, and force the predicate
25+
// to RESOLVED so the straggler guard (which runs AFTER the verdict) is reached.
26+
vi.mock("node:fs", async () => {
27+
const actual = await vi.importActual<typeof import("node:fs")>("node:fs");
28+
return { ...actual, readFileSync: vi.fn(actual.readFileSync), writeFileSync: vi.fn() };
29+
});
30+
31+
vi.mock("node:child_process", async () => {
32+
const actual = await vi.importActual<typeof import("node:child_process")>("node:child_process");
33+
return { ...actual, execFileSync: vi.fn(), execSync: vi.fn() };
34+
});
35+
36+
vi.mock("../../scripts/drift-success-predicate.js", async () => {
37+
const actual = await vi.importActual<typeof import("../../scripts/drift-success-predicate.js")>(
38+
"../../scripts/drift-success-predicate.js",
39+
);
40+
return {
41+
...actual,
42+
// Force RESOLVED regardless of inputs so the guard downstream is exercised.
43+
evaluateDriftResolved: vi.fn(() => ({
44+
resolved: true,
45+
reason: actual.PredicateReason.RESOLVED,
46+
detail: "forced resolved for the straggler-guard test",
47+
offendingFiles: [],
48+
})),
49+
// gitChangedFiles is used by createPr — return a production file PLUS a
50+
// straggler (a root file gatedCommitFiles cannot classify).
51+
gitChangedFiles: vi.fn(() => ["src/helpers.ts", "weird-root-file.txt"]),
52+
};
53+
});
54+
55+
import { execSync, execFileSync } from "node:child_process";
56+
57+
import { createPr } from "../../scripts/fix-drift.js";
58+
59+
const mockedExecSync = vi.mocked(execSync);
60+
const mockedExecFileSync = vi.mocked(execFileSync);
61+
62+
describe("createPr straggler guard fail-closed (fix #F5, isolated)", () => {
63+
let logSpy: ReturnType<typeof vi.spyOn>;
64+
let errSpy: ReturnType<typeof vi.spyOn>;
65+
let exitSpy: ReturnType<typeof vi.spyOn>;
66+
67+
beforeEach(() => {
68+
vi.clearAllMocks();
69+
logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
70+
errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
71+
exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => {
72+
throw new Error(`__exit__${code}`);
73+
}) as never);
74+
// Default git calls return empty (branch lookups etc.).
75+
mockedExecSync.mockReturnValue("fix/drift-2026-07-16\n" as unknown as string);
76+
});
77+
78+
afterEach(() => {
79+
logSpy.mockRestore();
80+
errSpy.mockRestore();
81+
exitSpy.mockRestore();
82+
});
83+
84+
const rep: DriftReport = {
85+
timestamp: "2026-07-16T00:00:00.000Z",
86+
entries: [
87+
{
88+
provider: "OpenAI",
89+
scenario: "chat completion",
90+
builderFile: "src/helpers.ts",
91+
builderFunctions: ["buildChatCompletion"],
92+
typesFile: null,
93+
sdkShapesFile: "src/__tests__/drift/sdk-shapes.ts",
94+
diffs: [
95+
{
96+
path: "x",
97+
severity: "critical",
98+
issue: "missing",
99+
expected: "string",
100+
real: "string",
101+
mock: "<missing>",
102+
},
103+
],
104+
},
105+
],
106+
};
107+
108+
it("exits UNSANCTIONED_CHANGE (17) and NEVER stages the straggler even on a RESOLVED verdict", () => {
109+
expect(() => createPr(rep, { report: { timestamp: "t", entries: [] }, exitCode: 0 })).toThrow(
110+
/__exit__17/,
111+
);
112+
113+
const staged = mockedExecFileSync.mock.calls.some(
114+
(c) =>
115+
c[0] === "git" && Array.isArray(c[1]) && (c[1] as string[]).includes("weird-root-file.txt"),
116+
);
117+
expect(staged).toBe(false);
118+
119+
const lines = logSpy.mock.calls.map((c) => String(c[0]));
120+
expect(lines).toContain("reason=unsanctioned-change");
121+
});
122+
});
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
/**
2+
* Static (text-level) assertions on .github/workflows/fix-drift.yml.
3+
*
4+
* These pin the LOAD-BEARING wiring that the drift-success predicate/guard now
5+
* REQUIRE (CR round-3):
6+
*
7+
* F1 — the workflow must (a) re-collect drift AUTHORITATIVELY after the autofix
8+
* to a distinct post-fix path, capturing its exit code, and (b) pass BOTH
9+
* --post-fix-report and --post-fix-exit into the `--create-pr` invocation.
10+
* Without these, the mandatory-post-fix guard fails closed and NO PR is
11+
* ever opened (the gate would be inert).
12+
*
13+
* F-A — the PRE-fix report the allowlist's sanctioned-target set is derived
14+
* from must be PINNED outside the LLM-writable repo checkout BEFORE the
15+
* autofix runs, and both the Assert and Create-PR steps must read
16+
* --report from that pinned copy — NEVER the in-repo drift-report.json
17+
* the autofix LLM could overwrite.
18+
*
19+
* No YAML dependency is added; the repo ships none. These are deliberately
20+
* text-shape assertions on the committed workflow — an actionlint run in CI
21+
* covers structural validity separately.
22+
*/
23+
import { readFileSync } from "node:fs";
24+
import { resolve } from "node:path";
25+
26+
import { describe, it, expect } from "vitest";
27+
28+
const WORKFLOW_PATH = resolve(__dirname, "../../.github/workflows/fix-drift.yml");
29+
const wf = readFileSync(WORKFLOW_PATH, "utf-8");
30+
31+
/** Collapse runs of whitespace so multi-line YAML `run:` blocks match linearly. */
32+
const wfFlat = wf.replace(/\s+/g, " ");
33+
34+
describe("fix-drift.yml — F1: post-fix re-collect + args wired into --create-pr", () => {
35+
it("has an authoritative post-fix re-collect step writing a DISTINCT report path OUTSIDE the repo (FIX #F3)", () => {
36+
expect(wf).toContain("Re-collect drift (authoritative)");
37+
// FIX #F3 — the re-collect writes to $RUNNER_TEMP (via the POST_FIX_REPORT
38+
// env), NOT the repo cwd, so it is never scored by the predicate's git scan.
39+
expect(wf).toContain('npx tsx scripts/drift-report-collector.ts --out "$POST_FIX_REPORT"');
40+
expect(wf).toContain("POST_FIX_REPORT: ${{ runner.temp }}/drift-report.post-fix.json");
41+
});
42+
43+
it("captures the post-fix collector exit code as a step output", () => {
44+
expect(wfFlat).toContain('POST_FIX_EXIT=$? set -e echo "post_fix_exit=$POST_FIX_EXIT"');
45+
});
46+
47+
it("passes BOTH --post-fix-report and --post-fix-exit into `fix-drift.ts --create-pr`", () => {
48+
expect(wfFlat).toContain("npx tsx scripts/fix-drift.ts --create-pr");
49+
expect(wfFlat).toMatch(
50+
/fix-drift\.ts --create-pr[^]*?--post-fix-report "\$\{POST_FIX_REPORT\}"[^]*?--post-fix-exit "\$\{POST_FIX_EXIT\}"/,
51+
);
52+
});
53+
54+
it("the Assert step runs the predicate with post-fix args (the happy-path gate)", () => {
55+
expect(wf).toContain("Assert drift truly resolved");
56+
expect(wfFlat).toMatch(
57+
/drift-success-predicate\.ts[^]*?--post-fix-report "\$\{POST_FIX_REPORT\}"[^]*?--post-fix-exit "\$\{POST_FIX_EXIT\}"/,
58+
);
59+
});
60+
});
61+
62+
describe("fix-drift.yml — F-A: PRE-fix report pinned outside the LLM-writable checkout", () => {
63+
it("has a pin step that copies the pre-fix report into runner.temp before autofix", () => {
64+
expect(wf).toContain("Pin pre-fix drift report (integrity)");
65+
// FIX #F3 — the pre-fix report is itself collected into $RUNNER_TEMP
66+
// (PRE_FIX_REPORT), so the pin copies from that out-of-repo path, never the
67+
// repo cwd. Both source and destination are outside the LLM-writable checkout.
68+
expect(wf).toContain('cp "$PRE_FIX_REPORT" "$PINNED_REPORT"');
69+
expect(wf).toContain("PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json");
70+
});
71+
72+
it("the pin step runs BEFORE the Auto-fix step (so the LLM cannot pre-tamper the pin)", () => {
73+
const pinIdx = wf.indexOf("Pin pre-fix drift report");
74+
const autofixIdx = wf.indexOf("name: Auto-fix drift");
75+
expect(pinIdx).toBeGreaterThan(-1);
76+
expect(autofixIdx).toBeGreaterThan(-1);
77+
expect(pinIdx).toBeLessThan(autofixIdx);
78+
});
79+
80+
it("the Assert step reads --report from the PINNED copy, not the in-repo file", () => {
81+
// The YAML line-continuation `\` survives whitespace-flattening, so match
82+
// tolerantly across it.
83+
expect(wfFlat).toMatch(/drift-success-predicate\.ts \\? *--report "\$\{PINNED_REPORT\}"/);
84+
});
85+
86+
it("the Create PR step reads --report from the PINNED copy, not the in-repo file", () => {
87+
expect(wfFlat).toMatch(
88+
/scripts\/fix-drift\.ts --create-pr \\? *--report "\$\{PINNED_REPORT\}"/,
89+
);
90+
});
91+
92+
it("neither the Assert nor Create-PR predicate invocation reads --report drift-report.json (the LLM-writable file)", () => {
93+
// The in-repo drift-report.json is still uploaded as an artifact + copied by
94+
// the pin step, but must NEVER be the --report source for the gate.
95+
expect(wfFlat).not.toMatch(/drift-success-predicate\.ts \\? *--report drift-report\.json/);
96+
expect(wfFlat).not.toMatch(/fix-drift\.ts --create-pr \\? *--report drift-report\.json/);
97+
});
98+
});
99+
100+
// ---------------------------------------------------------------------------
101+
// FIX (round-4, user-approved) — HUMAN-APPROVAL BACKSTOP. The drift path opens a
102+
// PR but must NEVER auto-merge: the predicate is a strong AUTO-FILTER, not a
103+
// provable merge gate (the re-collect is not independent of the fix — WS-2b), so
104+
// a human reviews CI + the diff + the verdict and merges. These lock that the
105+
// unattended in-workflow merge is GONE and the Slack copy no longer claims
106+
// "merged to main".
107+
// ---------------------------------------------------------------------------
108+
describe("fix-drift.yml — human-approval backstop: no unattended auto-merge", () => {
109+
it("has NO auto-merge step and never runs `gh pr merge`", () => {
110+
expect(wf).not.toContain("Auto-merge PR");
111+
expect(wf).not.toMatch(/gh pr merge/);
112+
});
113+
114+
it("documents WHY the drift path is human-gated (predicate is a filter, not a merge gate)", () => {
115+
expect(wf).toContain("NO AUTO-MERGE");
116+
// The rationale wraps across comment lines (a `#` marker survives flattening),
117+
// so match tolerantly across the wrap.
118+
expect(wfFlat).toMatch(/AUTO-FILTER, NOT a provable merge (# )?gate/i);
119+
});
120+
121+
it("the success Slack message says the PR needs human review + merge, NOT merged to main", () => {
122+
expect(wf).not.toContain("Drift auto-fix merged to main");
123+
expect(wf).toContain("Drift-fix PR opened — needs human review + merge");
124+
});
125+
126+
it("the fix-failure Slack step no longer references the removed merge step outputs", () => {
127+
expect(wf).not.toContain("steps.merge.outputs");
128+
expect(wf).not.toContain("MERGE_REASON");
129+
});
130+
});

0 commit comments

Comments
 (0)