From c395c53bcebfd40c8fe807fb61d4233856d420e0 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Fri, 17 Jul 2026 11:15:55 -0700 Subject: [PATCH 1/6] fix(drift): require real drift resolution, not fixture relaxation, before PR/merge The auto-fix abort guard in fix-drift.ts accepted any run that changed a file under src/ (builderFiles>0 || testFiles>0) and exited 0. Because the drift tests are three-way triangulations whose SDK leg is the repo fixture src/__tests__/drift/sdk-shapes.ts, the LLM could "resolve" drift by RELAXING that fixture: the critical branch goes unreachable, the collector re-reports clean, and WS-1's merge gate auto-merges with the production mock untouched. Add scripts/drift-success-predicate.ts (a pure evaluateDriftResolved fn plus a thin CLI) that requires THREE independent signals for resolved:true: 1. post-fix collector clean (exit 0 AND criticalCount 0), 2. >=1 PRODUCTION file changed (src/** excluding src/__tests__/), 3. no standalone comparison-leg relaxation; allowlist / *.drift.ts assertion edits ALWAYS block (SUPPRESSION_SUSPECTED). Legit fixture targets (model-registry.ts, model-family.ts, voice-models.ts) are sanctioned via the report's builderFile/typesFile. Distinct exit codes (10-16, 2) route each failure to a named needs-human Slack reason. Replace the weak guard in fix-drift.ts createPr() with the predicate (gated before any git add/commit) and wire the workflow: a fresh authoritative re-collect to drift-report.post-fix.json, an "Assert drift truly resolved" step, and the new reasons into the Slack case block. --- .github/workflows/fix-drift.yml | 96 +++- scripts/drift-success-predicate.ts | 530 ++++++++++++++++++ scripts/fix-drift.ts | 125 ++++- src/__tests__/drift-success-predicate.test.ts | 472 ++++++++++++++++ 4 files changed, 1199 insertions(+), 24 deletions(-) create mode 100644 scripts/drift-success-predicate.ts create mode 100644 src/__tests__/drift-success-predicate.test.ts diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index 298c519b..48e1d058 100644 --- a/.github/workflows/fix-drift.yml +++ b/.github/workflows/fix-drift.yml @@ -150,6 +150,10 @@ jobs: if: steps.autofix.outcome == 'success' && steps.check.outputs.skip != 'true' run: pnpm test + # `pnpm test:drift` is a FAST fail here, but it is NO LONGER the + # authoritative signal — it is the same suite the fixer was told to make + # pass, so a fixture-relaxation cheat sails through it. The authoritative + # signal is the fresh re-collect + drift-success predicate below. - name: Verify drift resolved if: steps.autofix.outcome == 'success' && steps.check.outputs.skip != 'true' env: @@ -158,6 +162,67 @@ jobs: GOOGLE_API_KEY: ${{ secrets.GOOGLE_API_KEY }} run: pnpm test:drift + # Step 4c: Re-collect drift AUTHORITATIVELY against the LIVE SDK, writing + # to a DISTINCT path so the pre-fix report is preserved for classification. + # This is a fresh collector invocation (same capture pattern as Step 1) — + # its exit code is the authoritative "is the drift gone?" signal that the + # predicate scores. A cheat that relaxed the SDK-shape fixture still + # reports clean HERE (its own SDK leg reads the relaxed fixture), which is + # exactly why the predicate ALSO requires a real production change. + - name: Re-collect drift (authoritative) + id: recollect + if: steps.autofix.outcome == 'success' && steps.check.outputs.skip != 'true' + env: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + GOOGLE_API_KEY: ${{ secrets.GOOGLE_API_KEY }} + run: | + set +e + npx tsx scripts/drift-report-collector.ts --out drift-report.post-fix.json + POST_FIX_EXIT=$? + set -e + echo "post_fix_exit=$POST_FIX_EXIT" >> "$GITHUB_OUTPUT" + echo "Post-fix collector exited with code $POST_FIX_EXIT" + + - name: Upload post-fix drift report + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: drift-report-post-fix + path: drift-report.post-fix.json + if-no-files-found: warn + retention-days: 30 + + # Step 4d: Assert the drift was TRULY resolved (not a fixture relaxation). + # The predicate scores three independent signals — post-fix collector + # clean, a real production mock-builder change, and no comparison-leg + # relaxation — and exits non-zero (with a distinct reason) on a cheat. + # A non-zero exit blocks the pipeline: no PR is opened, so nothing merges. + - name: Assert drift truly resolved + id: assert + if: steps.autofix.outcome == 'success' && steps.check.outputs.skip != 'true' + run: | + # Run the predicate to a log file so we capture its REAL exit code + # directly (a `VAR="$(pipeline)"` assignment would report the + # assignment's own status via PIPESTATUS, never the predicate's). + set +e + npx tsx scripts/drift-success-predicate.ts \ + --report drift-report.json \ + --post-fix-report drift-report.post-fix.json \ + --post-fix-exit "${POST_FIX_EXIT}" 2>&1 | tee predicate.log + PRED_EXIT=${PIPESTATUS[0]} + set -e + REASON="$(grep '^reason=' predicate.log | tail -n1)" + REASON="${REASON#reason=}" + echo "reason=${REASON}" >> "$GITHUB_OUTPUT" + if [ "$PRED_EXIT" -ne 0 ]; then + echo "::error::Drift-success predicate refused (exit ${PRED_EXIT}, reason=${REASON}) — not a real drift fix, blocking PR" + exit "$PRED_EXIT" + fi + echo "Drift-success predicate: drift truly resolved" + env: + POST_FIX_EXIT: ${{ steps.recollect.outputs.post_fix_exit }} + # Inject git credentials only when needed for push (persist-credentials: false above) - name: Configure git for push if: steps.autofix.outcome == 'success' && success() && steps.check.outputs.skip != 'true' @@ -175,9 +240,17 @@ jobs: if: steps.autofix.outcome == 'success' && success() && steps.check.outputs.skip != 'true' env: GH_TOKEN: ${{ steps.app-token.outputs.token }} + POST_FIX_EXIT: ${{ steps.recollect.outputs.post_fix_exit }} run: | set -euo pipefail - npx tsx scripts/fix-drift.ts --create-pr + # Defense-in-depth: the workflow already asserted the predicate in the + # "Assert drift truly resolved" step, but fix-drift.ts runs it AGAIN + # (via --post-fix-report/--post-fix-exit) as an in-script gate BEFORE + # any git add/commit, so a cheat opens no PR even if the step guard is + # ever bypassed. + npx tsx scripts/fix-drift.ts --create-pr \ + --post-fix-report drift-report.post-fix.json \ + --post-fix-exit "${POST_FIX_EXIT}" HEAD_SHA="$(git rev-parse HEAD)" echo "Pushed head SHA: $HEAD_SHA" # Find the open PR whose headRefOid matches the exact SHA we pushed. @@ -465,11 +538,15 @@ jobs: RUN_ID: ${{ github.run_id }} MERGE_REASON: ${{ steps.merge.outputs.reason }} PR_REASON: ${{ steps.pr.outputs.reason }} + ASSERT_REASON: ${{ steps.assert.outputs.reason }} run: | set -euo pipefail - # The merge-step reason takes precedence; fall back to the PR-step - # reason so a SHA-match exhaustion in Create PR is not reported blank. - REASON="${MERGE_REASON:-${PR_REASON:-}}" + # The merge-step reason takes precedence, then the PR-step reason, then + # the drift-success predicate's reason (the "Assert drift truly + # resolved" step) so a blocked cheat is named rather than reported + # blank. The predicate reasons below (WS-6 needs-human tie-in) name the + # attempted-cheat / not-actually-fixed cause explicitly. + REASON="${MERGE_REASON:-${PR_REASON:-${ASSERT_REASON:-}}}" case "$REASON" in timeout) DETAIL=" (CI checks timed out before completing)";; not-green) DETAIL=" (merge gate refused — PR not truly green)";; @@ -480,6 +557,17 @@ jobs: merge-command-failed) DETAIL=" (the merge command failed — head moved after the gate scored it, or the merge was rejected)";; no-checks) DETAIL=" (no CI checks ever registered)";; no-pr-match) DETAIL=" (no open PR matched the pushed head SHA)";; + # Drift-success predicate reasons — NEEDS HUMAN. The LOUD cheat + # reasons (comparison-leg-only / suppression-suspected) mean the + # auto-fix tried to game the drift detector rather than fix the mock. + comparison-leg-only) DETAIL=" (🚨 NEEDS HUMAN: auto-fix changed ONLY comparison-leg fixtures — a fixture-relaxation cheat, NOT a real mock fix. See run logs for the offending files)";; + suppression-suspected) DETAIL=" (🚨 NEEDS HUMAN: auto-fix edited the drift allowlist or a *.drift.ts assertion — silencing the detector, never a valid fix. See run logs)";; + no-production-change) DETAIL=" (NEEDS HUMAN: auto-fix changed zero production mock-builder files — nothing shippable)";; + still-dirty) DETAIL=" (NEEDS HUMAN: post-fix collector still reports critical drift — the fix did not resolve it)";; + quarantine-after-fix) DETAIL=" (NEEDS HUMAN: post-fix collector returned quarantine — unparseable/untrusted output after the fix)";; + collector-infra) DETAIL=" (NEEDS HUMAN: post-fix collector infra failure — cannot trust a clean signal)";; + production-change-off-target) DETAIL=" (NEEDS HUMAN: production change did not touch any file the drift report named as a fix target — may be a legit shared-helper fix)";; + config-error) DETAIL=" (drift-success predicate CONFIG error — missing/unreadable report or bad args; the gate itself needs attention)";; *) DETAIL="";; esac if [ -z "${SLACK_WEBHOOK:-}" ]; then diff --git a/scripts/drift-success-predicate.ts b/scripts/drift-success-predicate.ts new file mode 100644 index 00000000..341a0ca9 --- /dev/null +++ b/scripts/drift-success-predicate.ts @@ -0,0 +1,530 @@ +/// + +/** + * Drift-Success Predicate (WS-2) + * + * A pure predicate — plus a thin CLI wrapper — that decides whether an + * auto-fix run ACTUALLY resolved API drift, versus merely GAMING the drift + * detector by relaxing one of the comparison legs (the SDK-shape fixture, the + * triangulation schema/allowlist, the real-API harness, or a `*.drift.ts` + * assertion). + * + * The hole this closes: the drift tests are three-way triangulations + * (SDK vs real API vs mock). The SDK leg is literally the repo fixture + * `src/__tests__/drift/sdk-shapes.ts`. Deleting a field from that fixture makes + * the SDK leg null for that path, so the "critical" branch cannot fire and the + * collector reports clean (exit 0) WITHOUT any change to the mock builder. + * Re-running the collector alone therefore cannot detect the cheat — its own + * SDK leg reads the relaxed fixture. The old guard in fix-drift.ts + * (`builderFiles.length === 0 && testFiles.length === 0`) ACCEPTS such a run + * because `testFiles` is non-empty. + * + * The predicate requires THREE independent signals for `resolved:true`: + * 1. AUTHORITATIVE — the post-fix collector re-run is clean (exit 0 AND + * criticalCount 0). + * 2. PRODUCTION CHANGE — at least one PRODUCTION mock-builder file changed + * (`src/**` excluding `src/__tests__/`). A relaxation NEVER changes one. + * 3. NO STANDALONE COMPARISON-LEG RELAXATION — a change that touches a + * gameable comparison leg must be ACCOMPANIED by a production change, and + * edits to the triangulation allowlist / `*.drift.ts` assertions are + * ALWAYS blocked (silencing the detector is never a valid drift fix). + * + * Additionally, the production change should intersect the report's SANCTIONED + * target set (`union(entry.builderFile, entry.typesFile≠null)`); an off-target + * production change is a WARNING that still blocks (a shared helper MAY be the + * real fix, so it is distinct from an outright cheat). + * + * CLI exit codes (mirrors drift-report-collector.ts's distinct-code discipline): + * 0 — RESOLVED + * 10 — NO_PRODUCTION_CHANGE + * 11 — COMPARISON_LEG_ONLY (the headline cheat) + * 12 — SUPPRESSION_SUSPECTED (allowlist / *.drift.ts assertion edited) + * 13 — STILL_DIRTY (post-fix collector exit 2) + * 14 — QUARANTINE_AFTER_FIX (post-fix collector exit 5) + * 15 — COLLECTOR_INFRA (post-fix collector exit 1) + * 16 — PRODUCTION_CHANGE_OFF_TARGET (WARNING, still blocks) + * 2 — CONFIG_ERROR (missing/unreadable report, bad args) + * + * Usage: + * npx tsx scripts/drift-success-predicate.ts \ + * --report drift-report.json \ + * --post-fix-report drift-report.post-fix.json \ + * --post-fix-exit \ + * [--changed-file src/helpers.ts ...] + * + * When no --changed-file args are supplied the CLI derives the changed set from + * `git status --porcelain` (mirrors fix-drift.ts:getChangedFiles()). + */ + +import { execSync } from "node:child_process"; +import { existsSync, readFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +import type { DriftReport } from "./drift-types.js"; + +// --------------------------------------------------------------------------- +// Public types +// --------------------------------------------------------------------------- + +export enum PredicateReason { + RESOLVED = "resolved", + NO_PRODUCTION_CHANGE = "no-production-change", + COMPARISON_LEG_ONLY = "comparison-leg-only", + SUPPRESSION_SUSPECTED = "suppression-suspected", + STILL_DIRTY = "still-dirty", + QUARANTINE_AFTER_FIX = "quarantine-after-fix", + COLLECTOR_INFRA = "collector-infra", + PRODUCTION_CHANGE_OFF_TARGET = "production-change-off-target", + CONFIG_ERROR = "config-error", +} + +/** Stable exit code for each reason (see module header). */ +export const REASON_EXIT_CODE: Record = { + [PredicateReason.RESOLVED]: 0, + [PredicateReason.NO_PRODUCTION_CHANGE]: 10, + [PredicateReason.COMPARISON_LEG_ONLY]: 11, + [PredicateReason.SUPPRESSION_SUSPECTED]: 12, + [PredicateReason.STILL_DIRTY]: 13, + [PredicateReason.QUARANTINE_AFTER_FIX]: 14, + [PredicateReason.COLLECTOR_INFRA]: 15, + [PredicateReason.PRODUCTION_CHANGE_OFF_TARGET]: 16, + [PredicateReason.CONFIG_ERROR]: 2, +}; + +export interface PredicateInputs { + /** Changed-file paths from getChangedFiles() (git porcelain). */ + changedFiles: string[]; + /** The ORIGINAL pre-fix drift report (source of sanctioned fix targets). */ + report: DriftReport; + /** Exit code of the re-run collector (0 clean / 2 dirty / 5 quarantine / 1 infra). */ + postFixCollectorExit: number; + /** criticalCount parsed from the re-run report (belt-and-suspenders vs exit 0). */ + postFixCriticalCount: number; +} + +export interface PredicateResult { + resolved: boolean; + reason: PredicateReason; + /** Human-readable one-liner for Slack / PR body. */ + detail: string; + /** The subset of changedFiles that triggered a block (for LOUD alerts). */ + offendingFiles: string[]; +} + +// --------------------------------------------------------------------------- +// File classification (see spec §2) +// --------------------------------------------------------------------------- + +/** + * The triangulation SCHEMA file. Editing it (esp. its ALLOWLISTED_PATHS set) + * silences diffs globally — a human-reviewed artifact, never a valid fix. + */ +const SCHEMA_FILE = "src/__tests__/drift/schema.ts"; + +/** + * The SDK-shape fixture — the SDK leg of the three-way compare and the primary + * cheat surface (relaxing it makes the critical branch unreachable). + */ +const SDK_SHAPES_FILE = "src/__tests__/drift/sdk-shapes.ts"; + +/** + * The real-API call harness files. Weakening these could elicit a smaller real + * shape (shrinking the real leg), making a diff disappear without a mock change. + */ +const HARNESS_FILES: ReadonlySet = new Set([ + "src/__tests__/drift/providers.ts", + "src/__tests__/drift/ws-providers.ts", + "src/__tests__/drift/helpers.ts", + "src/__tests__/drift/voice-models.ts", +]); + +/** + * LEGITIMATE-FIXTURE-THAT-IS-THE-FIX-TARGET: drift fixtures under + * `src/__tests__/drift/` that ARE the correct fix target for certain drifts + * (the known-models canary routes fixes to these model-list files). These are + * NOT gameable comparison legs — adding a newly-shipped model id here is a + * legit fix, not a relaxation. They are allowed as accompanying changes and are + * never counted as a standalone comparison-leg cheat. + */ +const LEGIT_FIXTURE_TARGETS: ReadonlySet = new Set([ + "src/__tests__/drift/model-registry.ts", + "src/__tests__/drift/model-family.ts", + "src/__tests__/drift/voice-models.ts", +]); + +/** + * True when `file` is a PRODUCTION mock-builder source file: under `src/` but + * NOT under `src/__tests__/`. This matches fix-drift.ts's existing + * `builderFiles` predicate exactly. + */ +export function isProductionFile(file: string): boolean { + return file.startsWith("src/") && !file.startsWith("src/__tests__/"); +} + +/** + * True when `file` is a `*.drift.ts` test file whose assertions could be + * loosened to make a diff disappear (e.g. `expect(...).toEqual([])`). + */ +export function isDriftTestFile(file: string): boolean { + return file.startsWith("src/__tests__/drift/") && file.endsWith(".drift.ts"); +} + +/** + * SUPPRESSION surface: editing these ALWAYS blocks, even alongside a production + * change, because silencing the detector (allowlist growth, loosened assertions) + * is never a valid drift fix. The schema file carries ALLOWLISTED_PATHS; the + * `*.drift.ts` files carry the assertions. + */ +export function isSuppressionSurface(file: string): boolean { + return file === SCHEMA_FILE || isDriftTestFile(file); +} + +/** + * GAMEABLE-COMPARISON-LEG: a comparison leg (SDK fixture, schema, real-API + * harness, or drift-test assertion) whose edit can erase a diff WITHOUT + * changing mock output. Explicitly EXCLUDES LEGIT_FIXTURE_TARGETS + * (canary model-list fixtures) — those are legit fix targets, not cheats. + * Note `voice-models.ts` is both a harness file AND a legit canary target; it + * is treated as a legit target (excluded here) so a canary fix is not blocked. + */ +export function isComparisonLeg(file: string): boolean { + if (LEGIT_FIXTURE_TARGETS.has(file)) return false; + if (file === SDK_SHAPES_FILE) return true; + if (file === SCHEMA_FILE) return true; + if (HARNESS_FILES.has(file)) return true; + if (isDriftTestFile(file)) return true; + return false; +} + +/** + * Derive the SANCTIONED fix-target set from the pre-fix report: + * `union(entry.builderFile, entry.typesFile≠null)`. These are the files the + * collector itself named as the correct place to fix each drift. + */ +export function sanctionedTargets(report: DriftReport): Set { + const targets = new Set(); + for (const entry of report.entries) { + if (entry.builderFile) targets.add(entry.builderFile); + if (entry.typesFile) targets.add(entry.typesFile); + } + return targets; +} + +// --------------------------------------------------------------------------- +// The predicate (see spec §3) +// --------------------------------------------------------------------------- + +export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { + const { changedFiles, report, postFixCollectorExit, postFixCriticalCount } = i; + + // ---- Signal 1: AUTHORITATIVE — collector clean on re-run. ------------- + // Checked FIRST: a dirty/quarantine/infra collector makes any file-set moot. + if (postFixCollectorExit === 2 || postFixCriticalCount > 0) { + return { + resolved: false, + reason: PredicateReason.STILL_DIRTY, + detail: + "Post-fix drift collector still reports critical drift " + + `(exit ${postFixCollectorExit}, criticalCount ${postFixCriticalCount}). The fix did not resolve the drift.`, + offendingFiles: [], + }; + } + if (postFixCollectorExit === 5) { + return { + resolved: false, + reason: PredicateReason.QUARANTINE_AFTER_FIX, + detail: + "Post-fix drift collector returned quarantine (exit 5) — unparseable/untrusted output after the fix. Needs human review.", + offendingFiles: [], + }; + } + if (postFixCollectorExit === 1) { + return { + resolved: false, + reason: PredicateReason.COLLECTOR_INFRA, + detail: + "Post-fix drift collector returned infra failure (exit 1) — AG-UI skipped or the collector crashed. Cannot trust a clean signal.", + offendingFiles: [], + }; + } + if (postFixCollectorExit !== 0) { + // Any other non-zero exit is an unrecognized collector state — fail closed. + return { + resolved: false, + reason: PredicateReason.COLLECTOR_INFRA, + detail: `Post-fix drift collector returned an unexpected exit code (${postFixCollectorExit}). Failing closed.`, + offendingFiles: [], + }; + } + + // ---- Classify the changed-file set. ----------------------------------- + const productionFiles = changedFiles.filter(isProductionFile); + const comparisonLegFiles = changedFiles.filter(isComparisonLeg); + const suppressionFiles = changedFiles.filter(isSuppressionSurface); + + // ---- Signal 3 (suppression): ALWAYS block. ---------------------------- + // Edits to the allowlist/schema or a *.drift.ts assertion silence the + // detector and are never a valid fix — block even with a production change. + if (suppressionFiles.length > 0) { + return { + resolved: false, + reason: PredicateReason.SUPPRESSION_SUSPECTED, + detail: + "Fix edited the triangulation schema/allowlist or a *.drift.ts assertion " + + `(${suppressionFiles.join(", ")}) — silencing the drift detector is never a valid fix. Needs human review.`, + offendingFiles: suppressionFiles, + }; + } + + // ---- Signal 2: PRODUCTION change present. ------------------------------ + if (productionFiles.length === 0) { + // No production change at all. Distinguish the pure comparison-leg cheat + // (the headline case) from a truly empty / non-source change. + if (comparisonLegFiles.length > 0) { + return { + resolved: false, + reason: PredicateReason.COMPARISON_LEG_ONLY, + detail: + "Fix changed ONLY comparison-leg files " + + `(${comparisonLegFiles.join(", ")}) with no production mock-builder change — ` + + "this relaxes the drift detector instead of fixing the mock. The exact cheat this gate blocks.", + offendingFiles: comparisonLegFiles, + }; + } + return { + resolved: false, + reason: PredicateReason.NO_PRODUCTION_CHANGE, + detail: + "Fix changed zero PRODUCTION mock-builder files — a clean collector is meaningless without a real mock change. Nothing shippable.", + offendingFiles: [], + }; + } + + // ---- Signal 3 (off-target WARNING): production change must intersect --- + // the report's sanctioned target set. A shared helper MAY legitimately be + // the real fix, so this WARNS and blocks but is distinct from a cheat. + const targets = sanctionedTargets(report); + const onTarget = productionFiles.some((f) => targets.has(f)); + if (targets.size > 0 && !onTarget) { + return { + resolved: false, + reason: PredicateReason.PRODUCTION_CHANGE_OFF_TARGET, + detail: + "Production change did not touch any file the drift report named as a fix target " + + `(changed: ${productionFiles.join(", ")}; sanctioned: ${[...targets].join(", ")}). ` + + "May be a legitimate shared-helper fix — needs human review.", + offendingFiles: productionFiles, + }; + } + + // ---- All signals satisfied. ------------------------------------------- + return { + resolved: true, + reason: PredicateReason.RESOLVED, + detail: + "Drift genuinely resolved: post-fix collector clean, " + + `real production change (${productionFiles.join(", ")}), no comparison-leg relaxation.`, + offendingFiles: [], + }; +} + +// --------------------------------------------------------------------------- +// CLI wrapper +// --------------------------------------------------------------------------- + +/** Thrown for malformed CLI args / unreadable inputs — maps to exit 2. */ +export class PredicateConfigError extends Error {} + +interface CliArgs { + reportPath: string; + postFixReportPath: string; + postFixExit: number; + /** Explicit changed files, or null to derive from git. */ + changedFiles: string[] | null; +} + +/** Parse argv (without node/script) into CliArgs. Throws PredicateConfigError. */ +export function parseCliArgs(argv: string[]): CliArgs { + let reportPath: string | null = null; + let postFixReportPath: string | null = null; + let postFixExit: number | null = null; + const changedFiles: string[] = []; + let sawChangedFlag = false; + + for (let idx = 0; idx < argv.length; idx++) { + const arg = argv[idx]; + const next = argv[idx + 1]; + switch (arg) { + case "--report": + if (!next) throw new PredicateConfigError("--report requires a path argument"); + reportPath = next; + idx++; + break; + case "--post-fix-report": + if (!next) throw new PredicateConfigError("--post-fix-report requires a path argument"); + postFixReportPath = next; + idx++; + break; + case "--post-fix-exit": { + if (next === undefined) + throw new PredicateConfigError("--post-fix-exit requires a numeric argument"); + const parsed = Number(next); + if (!Number.isInteger(parsed)) { + throw new PredicateConfigError(`--post-fix-exit must be an integer, got "${next}"`); + } + postFixExit = parsed; + idx++; + break; + } + case "--changed-file": + if (!next) throw new PredicateConfigError("--changed-file requires a path argument"); + sawChangedFlag = true; + changedFiles.push(next); + idx++; + break; + default: + throw new PredicateConfigError(`Unknown argument: ${arg}`); + } + } + + if (!reportPath) throw new PredicateConfigError("--report is required"); + if (!postFixReportPath) throw new PredicateConfigError("--post-fix-report is required"); + if (postFixExit === null) throw new PredicateConfigError("--post-fix-exit is required"); + + return { + reportPath, + postFixReportPath, + postFixExit, + changedFiles: sawChangedFlag ? changedFiles : null, + }; +} + +/** Minimal drift-report read + shape validation. Throws PredicateConfigError. */ +export function readReport(path: string): DriftReport { + if (!existsSync(path)) { + throw new PredicateConfigError(`Drift report not found at ${path}`); + } + let parsed: unknown; + try { + parsed = JSON.parse(readFileSync(path, "utf-8")); + } catch (err: unknown) { + throw new PredicateConfigError( + `Drift report at ${path} is not valid JSON: ${err instanceof Error ? err.message : String(err)}`, + ); + } + if ( + !parsed || + typeof parsed !== "object" || + !Array.isArray((parsed as Record).entries) + ) { + throw new PredicateConfigError( + `Drift report at ${path} has invalid structure: expected { entries: [...] }`, + ); + } + return parsed as DriftReport; +} + +/** + * Count critical diffs in a (post-fix) report. Belt-and-suspenders against a + * collector exit code that disagrees with the report contents. + */ +export function countCriticalDiffs(report: DriftReport): number { + return report.entries.reduce( + (sum, e) => sum + e.diffs.filter((d) => d.severity === "critical").length, + 0, + ); +} + +/** + * Parse a `git status --porcelain` line into a file path. Handles quoted paths + * and rename notation. Kept in sync with fix-drift.ts:parsePorcelainLine. + */ +export function parsePorcelainLine(line: string): string { + let path = line.slice(3).trim(); + const arrowIdx = path.indexOf(" -> "); + if (arrowIdx !== -1) path = path.slice(arrowIdx + 4); + if (path.startsWith('"') && path.endsWith('"')) path = path.slice(1, -1); + return path; +} + +/** Changed files from `git status --porcelain`. */ +export function gitChangedFiles(): string[] { + const out = execSync("git status --porcelain", { encoding: "utf-8" }).trimEnd(); + return out.split("\n").filter(Boolean).map(parsePorcelainLine); +} + +/** + * Run the predicate from CLI args. Returns the process exit code and prints + * `detail` (and offending files for LOUD reasons) to stdout/stderr. + */ +export function runCli(argv: string[]): number { + let args: CliArgs; + try { + args = parseCliArgs(argv); + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`CONFIG_ERROR: ${msg}`); + console.log(`reason=${PredicateReason.CONFIG_ERROR}`); + return REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]; + } + + let report: DriftReport; + let postFixReport: DriftReport; + try { + report = readReport(args.reportPath); + postFixReport = readReport(args.postFixReportPath); + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`CONFIG_ERROR: ${msg}`); + console.log(`reason=${PredicateReason.CONFIG_ERROR}`); + return REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]; + } + + let changedFiles: string[]; + try { + changedFiles = args.changedFiles ?? gitChangedFiles(); + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`CONFIG_ERROR: could not read changed files from git: ${msg}`); + console.log(`reason=${PredicateReason.CONFIG_ERROR}`); + return REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]; + } + + const verdict = evaluateDriftResolved({ + changedFiles, + report, + postFixCollectorExit: args.postFixExit, + postFixCriticalCount: countCriticalDiffs(postFixReport), + }); + + if (verdict.resolved) { + console.log(verdict.detail); + } else { + console.error(`DRIFT NOT RESOLVED [${verdict.reason}]: ${verdict.detail}`); + if (verdict.offendingFiles.length > 0) { + console.error(`Offending files: ${verdict.offendingFiles.join(", ")}`); + } + } + // Emit a machine-readable reason line for the workflow to capture. + console.log(`reason=${verdict.reason}`); + return REASON_EXIT_CODE[verdict.reason]; +} + +// --------------------------------------------------------------------------- +// Entry-point guard (mirrors drift-report-collector.ts:isDirectRun) +// --------------------------------------------------------------------------- + +function isDirectRun(): boolean { + const entry = process.argv[1]; + if (!entry) return false; + try { + return fileURLToPath(import.meta.url) === resolve(entry); + } catch { + return false; + } +} + +if (isDirectRun()) { + process.exit(runCli(process.argv.slice(2))); +} diff --git a/scripts/fix-drift.ts b/scripts/fix-drift.ts index 8321146c..b16ebc47 100644 --- a/scripts/fix-drift.ts +++ b/scripts/fix-drift.ts @@ -19,6 +19,21 @@ * 3 — unhandled error (e.g. bad arguments, missing report, git/gh command failure) * 124 — Claude Code timed out (default mode) * In default mode, the exit code is passed through from Claude Code. + * + * In --create-pr mode, the drift-success predicate (drift-success-predicate.ts) + * gates PR creation BEFORE any git add/commit. When it rejects the fix (e.g. a + * fixture-relaxation cheat rather than a real mock change), createPr exits with + * the predicate's reason code instead of opening a PR: + * 10 — NO_PRODUCTION_CHANGE (zero production mock-builder files changed) + * 11 — COMPARISON_LEG_ONLY (only comparison-leg files changed — the cheat) + * 12 — SUPPRESSION_SUSPECTED (allowlist / *.drift.ts assertion edited) + * 13 — STILL_DIRTY (post-fix collector still reports critical drift) + * 14 — QUARANTINE_AFTER_FIX (post-fix collector returned quarantine) + * 15 — COLLECTOR_INFRA (post-fix collector infra failure) + * 16 — PRODUCTION_CHANGE_OFF_TARGET (production change not in report's target set) + * The legacy exit 4 (no source files changed) is subsumed by 10/11. The + * predicate runs ONLY when the workflow supplies --post-fix-report and + * --post-fix-exit; without them createPr falls back to the legacy exit-4 guard. */ import { spawn, execSync, execFileSync } from "node:child_process"; @@ -26,6 +41,12 @@ import { readFileSync, writeFileSync, existsSync, unlinkSync } from "node:fs"; import { resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import { + evaluateDriftResolved, + readReport as readPostFixReport, + countCriticalDiffs, + REASON_EXIT_CODE, +} from "./drift-success-predicate.js"; import type { DriftReport, DriftSeverity } from "./drift-types.js"; // --------------------------------------------------------------------------- @@ -604,9 +625,68 @@ export function getChangedFiles(): string[] { return exec("git status --porcelain").split("\n").filter(Boolean).map(parsePorcelainLine); } -function createPr(report: DriftReport): void { +/** + * Optional post-fix collector result, supplied by the workflow so createPr does + * not re-shell the (2-3 min) collector. When present, the drift-success + * predicate is the authoritative PR-open gate; when absent, createPr falls back + * to the legacy "no source files changed" guard (exit 4). + */ +export interface PostFixCollectorResult { + /** Parsed post-fix drift report (from --post-fix-report). */ + report: DriftReport; + /** Exit code of the re-run collector (from --post-fix-exit). */ + exitCode: number; +} + +function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { const stamp = todayStamp(); + // Detect uncommitted changes (staged + unstaged) BEFORE any git write ops so + // the drift-success predicate can gate PR-open ahead of branch/commit/push. + const changedFiles = getChangedFiles(); + + const builderFiles = changedFiles.filter( + (f) => f.startsWith("src/") && !f.startsWith("src/__tests__/"), + ); + const testFiles = changedFiles.filter((f) => f.startsWith("src/__tests__/")); + const skillFiles = changedFiles.filter((f) => f.startsWith("skills/")); + + // PR-OPEN GATE (WS-2). When the workflow supplies the post-fix collector + // result, the drift-success predicate is the authoritative decision: it + // rejects fixture-relaxation cheats (a diff that changed ONLY comparison-leg + // files, or silenced the detector) and drifts that were not actually + // resolved. This runs BEFORE any git add/commit — a blocked verdict opens no + // PR (and therefore never reaches auto-merge). Without --post-fix-* args, + // fall back to the legacy "no source files changed" guard (exit 4). + if (postFix) { + const verdict = evaluateDriftResolved({ + changedFiles, + report, + postFixCollectorExit: postFix.exitCode, + postFixCriticalCount: countCriticalDiffs(postFix.report), + }); + if (!verdict.resolved) { + console.error(`ERROR: Drift NOT resolved [${verdict.reason}]: ${verdict.detail}`); + if (verdict.offendingFiles.length > 0) { + console.error(`Offending files: ${verdict.offendingFiles.join(", ")}`); + } + console.error("Aborting PR creation — this fix will be routed to human review."); + console.log(`reason=${verdict.reason}`); + process.exit(REASON_EXIT_CODE[verdict.reason]); + } + console.log(`Drift-success predicate: RESOLVED — ${verdict.detail}`); + } else { + // Legacy guard: abort if no source files changed (a version-bump-only PR + // would be misleading). Superseded by the predicate above when present. + if (builderFiles.length === 0 && testFiles.length === 0) { + console.error( + "ERROR: No source files changed. Claude Code may not have made any fixes, " + + "or all changes were reverted during verification. Aborting PR creation.", + ); + process.exit(4); // no source files changed (distinct from exit 2 = critical drift) + } + } + // Determine branch name let currentBranch: string; try { @@ -625,24 +705,6 @@ function createPr(report: DriftReport): void { console.log(`Created branch ${branchName}`); } - // Stage and commit in groups — detect uncommitted changes (staged + unstaged) - const changedFiles = getChangedFiles(); - - const builderFiles = changedFiles.filter( - (f) => f.startsWith("src/") && !f.startsWith("src/__tests__/"), - ); - const testFiles = changedFiles.filter((f) => f.startsWith("src/__tests__/")); - const skillFiles = changedFiles.filter((f) => f.startsWith("skills/")); - - // Abort if no source files were changed — a version-bump-only PR would be misleading - if (builderFiles.length === 0 && testFiles.length === 0) { - console.error( - "ERROR: No source files changed. Claude Code may not have made any fixes, " + - "or all changes were reverted during verification. Aborting PR creation.", - ); - process.exit(4); // no source files changed (distinct from exit 2 = critical drift) - } - if (builderFiles.length > 0) { execFileSafe("git", ["add", ...builderFiles]); execFileSafe("git", ["commit", "-m", "fix: auto-remediate API drift in builder functions"]); @@ -826,7 +888,30 @@ async function main(): Promise { console.log(`Loaded drift report: ${report.entries.length} entries from ${report.timestamp}`); if (mode === "pr") { - createPr(report); + // Parse the optional post-fix collector result. When BOTH flags are + // supplied, the drift-success predicate gates PR creation. If only one is + // supplied the args are malformed — fail rather than silently skip the gate. + const postFixReportIdx = args.indexOf("--post-fix-report"); + const postFixExitIdx = args.indexOf("--post-fix-exit"); + const hasPostFixReport = postFixReportIdx !== -1 && args[postFixReportIdx + 1] !== undefined; + const hasPostFixExit = postFixExitIdx !== -1 && args[postFixExitIdx + 1] !== undefined; + + let postFix: PostFixCollectorResult | undefined; + if (hasPostFixReport || hasPostFixExit) { + if (!hasPostFixReport || !hasPostFixExit) { + throw new Error( + "--post-fix-report and --post-fix-exit must be supplied together (the drift-success predicate needs both)", + ); + } + const postFixExit = Number(args[postFixExitIdx + 1]); + if (!Number.isInteger(postFixExit)) { + throw new Error(`--post-fix-exit must be an integer, got "${args[postFixExitIdx + 1]}"`); + } + const postFixReport = readPostFixReport(resolve(args[postFixReportIdx + 1])); + postFix = { report: postFixReport, exitCode: postFixExit }; + } + + createPr(report, postFix); } else { const prompt = buildPrompt(report); console.log("Invoking Claude Code CLI..."); diff --git a/src/__tests__/drift-success-predicate.test.ts b/src/__tests__/drift-success-predicate.test.ts new file mode 100644 index 00000000..5ac85d11 --- /dev/null +++ b/src/__tests__/drift-success-predicate.test.ts @@ -0,0 +1,472 @@ +/** + * Tests for the WS-2 drift-success predicate. + * + * These exercise the REAL exported pure function `evaluateDriftResolved` and + * the CLI helpers from scripts/drift-success-predicate.ts. The predicate is a + * pure function over a small `DriftReport` fixture + a synthetic changed-file + * array — no live API, no LLM, no aimock needed. + * + * The headline case is the fixture-relaxation cheat: a run that changes ONLY + * `src/__tests__/drift/sdk-shapes.ts` (relaxing the SDK leg) must be REJECTED + * (COMPARISON_LEG_ONLY), whereas the OLD fix-drift.ts guard (`builderFiles>0 || + * testFiles>0`) would have ACCEPTED it — demonstrated by contrast below. + */ + +import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { describe, it, expect, afterEach } from "vitest"; + +import type { DriftEntry, DriftReport, ParsedDiff } from "../../scripts/drift-types.js"; +import { + evaluateDriftResolved, + PredicateReason, + REASON_EXIT_CODE, + isProductionFile, + isComparisonLeg, + isSuppressionSurface, + sanctionedTargets, + countCriticalDiffs, + parseCliArgs, + PredicateConfigError, + readReport, + runCli, +} from "../../scripts/drift-success-predicate.js"; + +// --------------------------------------------------------------------------- +// Fixture builders +// --------------------------------------------------------------------------- + +function diff(overrides: Partial = {}): ParsedDiff { + return { + path: "choices[0].message.content", + severity: "critical", + issue: "field present in SDK+real but missing from mock", + expected: "string", + real: "string", + mock: "", + ...overrides, + }; +} + +function entry(overrides: Partial = {}): DriftEntry { + return { + provider: "OpenAI", + scenario: "chat completion", + builderFile: "src/helpers.ts", + builderFunctions: ["buildChatCompletion"], + typesFile: "src/types.ts", + sdkShapesFile: "src/__tests__/drift/sdk-shapes.ts", + diffs: [diff()], + ...overrides, + }; +} + +function report(entries: DriftEntry[] = [entry()]): DriftReport { + return { timestamp: "2026-07-16T00:00:00.000Z", entries }; +} + +/** The OLD, gameable guard from fix-drift.ts:638 — for contrast assertions. */ +function oldGuardWouldAccept(changedFiles: string[]): boolean { + const builderFiles = changedFiles.filter( + (f) => f.startsWith("src/") && !f.startsWith("src/__tests__/"), + ); + const testFiles = changedFiles.filter((f) => f.startsWith("src/__tests__/")); + // OLD guard aborts ONLY when BOTH are empty; otherwise it proceeds. + return !(builderFiles.length === 0 && testFiles.length === 0); +} + +// --------------------------------------------------------------------------- +// RED cases — resolved:false +// --------------------------------------------------------------------------- + +describe("evaluateDriftResolved — RED (cheat/failure) cases", () => { + it("HEADLINE: fixture-relaxation-only (sdk-shapes.ts) → COMPARISON_LEG_ONLY, and the OLD guard would have ACCEPTED it", () => { + const changedFiles = ["src/__tests__/drift/sdk-shapes.ts"]; + const verdict = evaluateDriftResolved({ + changedFiles, + report: report(), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.COMPARISON_LEG_ONLY); + expect(verdict.offendingFiles).toContain("src/__tests__/drift/sdk-shapes.ts"); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(11); + + // Contrast: the OLD guard would have proceeded (testFiles non-empty). + expect(oldGuardWouldAccept(changedFiles)).toBe(true); + }); + + it("schema/allowlist edit + real builder change → SUPPRESSION_SUSPECTED (blocks even with a prod change)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/__tests__/drift/schema.ts", "src/helpers.ts"], + report: report(), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.SUPPRESSION_SUSPECTED); + expect(verdict.offendingFiles).toContain("src/__tests__/drift/schema.ts"); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(12); + }); + + it("*.drift.ts assertion loosened only → SUPPRESSION_SUSPECTED", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/__tests__/drift/openai-chat.drift.ts"], + report: report(), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.SUPPRESSION_SUSPECTED); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(12); + }); + + it("no changes at all → NO_PRODUCTION_CHANGE", () => { + const verdict = evaluateDriftResolved({ + changedFiles: [], + report: report(), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.NO_PRODUCTION_CHANGE); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(10); + }); + + it("production change but collector still dirty (exit 2) → STILL_DIRTY", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: report(), + postFixCollectorExit: 2, + postFixCriticalCount: 1, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.STILL_DIRTY); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(13); + }); + + it("production change + collector exit 0 but criticalCount>0 → STILL_DIRTY (belt-and-suspenders)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: report(), + postFixCollectorExit: 0, + postFixCriticalCount: 3, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.STILL_DIRTY); + }); + + it("post-fix quarantine (exit 5) → QUARANTINE_AFTER_FIX", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: report(), + postFixCollectorExit: 5, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.QUARANTINE_AFTER_FIX); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(14); + }); + + it("post-fix collector infra (exit 1) → COLLECTOR_INFRA", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: report(), + postFixCollectorExit: 1, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.COLLECTOR_INFRA); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(15); + }); + + it("production change off-target → PRODUCTION_CHANGE_OFF_TARGET", () => { + // report names src/helpers.ts; the change is an unrelated production file. + const verdict = evaluateDriftResolved({ + changedFiles: ["src/gemini.ts"], + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.PRODUCTION_CHANGE_OFF_TARGET); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(16); + }); +}); + +// --------------------------------------------------------------------------- +// GREEN cases — resolved:true +// --------------------------------------------------------------------------- + +describe("evaluateDriftResolved — GREEN (real fix) cases", () => { + it("real src/helpers.ts fix + clean collector → RESOLVED", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "src/types.ts"], + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(true); + expect(verdict.reason).toBe(PredicateReason.RESOLVED); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(0); + }); + + it("legit canary: model-registry.ts + ws-realtime.ts (report sanctions ws-realtime.ts) → RESOLVED", () => { + // The known-models canary routes its fix to the production ws-realtime.ts + // (builderFile), while the model list lives in the model-registry fixture. + const canary = report([ + entry({ + provider: "OpenAI Realtime", + scenario: "known-models canary", + builderFile: "src/ws-realtime.ts", + builderFunctions: ["buildRealtimeSession"], + typesFile: null, + diffs: [ + diff({ path: "knownModels", issue: "Unknown realtime model detected", mock: "" }), + ], + }), + ]); + const verdict = evaluateDriftResolved({ + changedFiles: ["src/__tests__/drift/model-registry.ts", "src/ws-realtime.ts"], + report: canary, + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(true); + expect(verdict.reason).toBe(PredicateReason.RESOLVED); + }); + + it("AG-UI: report names src/agui-types.ts; change to that file → RESOLVED", () => { + const agui = report([ + entry({ + provider: "AG-UI", + scenario: "missing event types", + builderFile: "src/agui-types.ts", + builderFunctions: ["AGUIEventType"], + typesFile: "src/agui-types.ts", + sdkShapesFile: "src/__tests__/drift/agui-schema.drift.ts", + diffs: [diff({ path: "AGUIEventType", issue: "missing event type" })], + }), + ]); + const verdict = evaluateDriftResolved({ + changedFiles: ["src/agui-types.ts"], + report: agui, + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(true); + expect(verdict.reason).toBe(PredicateReason.RESOLVED); + }); + + it("production change + accompanying legit canary fixture (not a comparison leg) → RESOLVED", () => { + // model-family.ts is a legit fixture target, not a gameable comparison leg, + // so accompanying a real production change it does not trip the cheat guard. + const verdict = evaluateDriftResolved({ + changedFiles: ["src/ws-realtime.ts", "src/__tests__/drift/model-family.ts"], + report: report([entry({ builderFile: "src/ws-realtime.ts", typesFile: null })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(true); + expect(verdict.reason).toBe(PredicateReason.RESOLVED); + }); +}); + +// --------------------------------------------------------------------------- +// File-classification unit coverage +// --------------------------------------------------------------------------- + +describe("file classification", () => { + it("isProductionFile matches src/** except src/__tests__/**", () => { + expect(isProductionFile("src/helpers.ts")).toBe(true); + expect(isProductionFile("src/agui-types.ts")).toBe(true); + expect(isProductionFile("src/__tests__/drift/sdk-shapes.ts")).toBe(false); + expect(isProductionFile("scripts/fix-drift.ts")).toBe(false); + }); + + it("isComparisonLeg flags SDK/schema/harness/*.drift.ts but NOT legit fixture targets", () => { + expect(isComparisonLeg("src/__tests__/drift/sdk-shapes.ts")).toBe(true); + expect(isComparisonLeg("src/__tests__/drift/schema.ts")).toBe(true); + expect(isComparisonLeg("src/__tests__/drift/providers.ts")).toBe(true); + expect(isComparisonLeg("src/__tests__/drift/ws-providers.ts")).toBe(true); + expect(isComparisonLeg("src/__tests__/drift/helpers.ts")).toBe(true); + expect(isComparisonLeg("src/__tests__/drift/openai-chat.drift.ts")).toBe(true); + // Legit fixture targets are NOT comparison legs. + expect(isComparisonLeg("src/__tests__/drift/model-registry.ts")).toBe(false); + expect(isComparisonLeg("src/__tests__/drift/model-family.ts")).toBe(false); + expect(isComparisonLeg("src/__tests__/drift/voice-models.ts")).toBe(false); + // Production files are not comparison legs. + expect(isComparisonLeg("src/helpers.ts")).toBe(false); + }); + + it("isSuppressionSurface flags schema + *.drift.ts only", () => { + expect(isSuppressionSurface("src/__tests__/drift/schema.ts")).toBe(true); + expect(isSuppressionSurface("src/__tests__/drift/openai-chat.drift.ts")).toBe(true); + expect(isSuppressionSurface("src/__tests__/drift/sdk-shapes.ts")).toBe(false); + expect(isSuppressionSurface("src/helpers.ts")).toBe(false); + }); + + it("sanctionedTargets unions builderFile + non-null typesFile", () => { + const t = sanctionedTargets( + report([ + entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" }), + entry({ builderFile: "src/gemini.ts", typesFile: null }), + ]), + ); + expect(t.has("src/helpers.ts")).toBe(true); + expect(t.has("src/types.ts")).toBe(true); + expect(t.has("src/gemini.ts")).toBe(true); + expect(t.has("src/__tests__/drift/sdk-shapes.ts")).toBe(false); + }); + + it("countCriticalDiffs counts only critical severities", () => { + const r = report([ + entry({ + diffs: [ + diff({ severity: "critical" }), + diff({ severity: "warning" }), + diff({ severity: "critical" }), + ], + }), + ]); + expect(countCriticalDiffs(r)).toBe(2); + }); +}); + +// --------------------------------------------------------------------------- +// CLI arg parsing + config errors (exit 2) +// --------------------------------------------------------------------------- + +describe("parseCliArgs", () => { + it("parses a full valid arg set", () => { + const args = parseCliArgs([ + "--report", + "a.json", + "--post-fix-report", + "b.json", + "--post-fix-exit", + "0", + "--changed-file", + "src/helpers.ts", + "--changed-file", + "src/types.ts", + ]); + expect(args.reportPath).toBe("a.json"); + expect(args.postFixReportPath).toBe("b.json"); + expect(args.postFixExit).toBe(0); + expect(args.changedFiles).toEqual(["src/helpers.ts", "src/types.ts"]); + }); + + it("null changedFiles when no --changed-file flag (derive from git later)", () => { + const args = parseCliArgs([ + "--report", + "a.json", + "--post-fix-report", + "b.json", + "--post-fix-exit", + "2", + ]); + expect(args.changedFiles).toBeNull(); + }); + + it("throws on missing --report", () => { + expect(() => parseCliArgs(["--post-fix-report", "b.json", "--post-fix-exit", "0"])).toThrow( + PredicateConfigError, + ); + }); + + it("throws on non-integer --post-fix-exit", () => { + expect(() => + parseCliArgs(["--report", "a.json", "--post-fix-report", "b.json", "--post-fix-exit", "abc"]), + ).toThrow(PredicateConfigError); + }); + + it("throws on unknown argument", () => { + expect(() => parseCliArgs(["--nope"])).toThrow(PredicateConfigError); + }); +}); + +describe("readReport", () => { + it("throws PredicateConfigError on a missing file", () => { + expect(() => readReport("/no/such/drift-report.json")).toThrow(PredicateConfigError); + }); +}); + +// --------------------------------------------------------------------------- +// runCli end-to-end (in-process): exit codes over real temp report files +// --------------------------------------------------------------------------- + +describe("runCli", () => { + let dir: string | null = null; + + afterEach(() => { + if (dir) rmSync(dir, { recursive: true, force: true }); + dir = null; + }); + + function writeReports(pre: DriftReport, post: DriftReport): { pre: string; post: string } { + dir = mkdtempSync(join(tmpdir(), "ws2-predicate-")); + const preP = join(dir, "drift-report.json"); + const postP = join(dir, "drift-report.post-fix.json"); + writeFileSync(preP, JSON.stringify(pre), "utf-8"); + writeFileSync(postP, JSON.stringify(post), "utf-8"); + return { pre: preP, post: postP }; + } + + it("exits 11 for the comparison-leg-only cheat", () => { + const paths = writeReports(report(), report([])); + const code = runCli([ + "--report", + paths.pre, + "--post-fix-report", + paths.post, + "--post-fix-exit", + "0", + "--changed-file", + "src/__tests__/drift/sdk-shapes.ts", + ]); + expect(code).toBe(11); + }); + + it("exits 0 for a real production fix", () => { + const paths = writeReports( + report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + report([]), + ); + const code = runCli([ + "--report", + paths.pre, + "--post-fix-report", + paths.post, + "--post-fix-exit", + "0", + "--changed-file", + "src/helpers.ts", + ]); + expect(code).toBe(0); + }); + + it("exits 2 (CONFIG_ERROR) on a missing post-fix report", () => { + const paths = writeReports(report(), report()); + const code = runCli([ + "--report", + paths.pre, + "--post-fix-report", + join(dir!, "does-not-exist.json"), + "--post-fix-exit", + "0", + "--changed-file", + "src/helpers.ts", + ]); + expect(code).toBe(2); + }); + + it("exits 2 (CONFIG_ERROR) on malformed args", () => { + const code = runCli(["--bogus"]); + expect(code).toBe(2); + }); +}); From 36fec1bc3cc6476659fcf2c9a9827a43d557b719 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Fri, 17 Jul 2026 11:38:52 -0700 Subject: [PATCH 2/6] fix(drift): block any gameable-leg edit in the success predicate (WS-2b) The predicate ignored comparison/SDK/harness-leg edits once any production file also changed, so relaxing sdk-shapes.ts plus one trivial on-target production edit returned resolved:true and auto-merged (the WS-2b hybrid cheat). Now any gameable-leg edit always blocks regardless of production files; schema/*.drift.ts always map to SUPPRESSION_SUSPECTED, other legs to SUPPRESSION_SUSPECTED when paired with a production change and COMPARISON_LEG_ONLY when standalone. Also: dual-classified voice-models.ts blocks (precedence), empty sanctioned-target set fails closed, --changed-file lists are cross-checked against git, the no-post-fix-args legacy PR path is removed (fail-closed), exit 5/1 get distinct reasons over criticalCount, and readReport rejects a missing timestamp. --- scripts/drift-success-predicate.ts | 279 +++++++++++---- scripts/fix-drift.ts | 108 +++--- src/__tests__/drift-success-predicate.test.ts | 337 ++++++++++++++++-- src/__tests__/fix-drift.test.ts | 32 ++ 4 files changed, 626 insertions(+), 130 deletions(-) diff --git a/scripts/drift-success-predicate.ts b/scripts/drift-success-predicate.ts index 341a0ca9..aed497e2 100644 --- a/scripts/drift-success-predicate.ts +++ b/scripts/drift-success-predicate.ts @@ -24,26 +24,42 @@ * criticalCount 0). * 2. PRODUCTION CHANGE — at least one PRODUCTION mock-builder file changed * (`src/**` excluding `src/__tests__/`). A relaxation NEVER changes one. - * 3. NO STANDALONE COMPARISON-LEG RELAXATION — a change that touches a - * gameable comparison leg must be ACCOMPANIED by a production change, and - * edits to the triangulation allowlist / `*.drift.ts` assertions are - * ALWAYS blocked (silencing the detector is never a valid drift fix). + * 3. NO GAMEABLE-LEG EDIT AT ALL — the changed set touches NO gameable leg. + * HARDENED (fix #1): a gameable-leg edit ALWAYS blocks, INDEPENDENT of how + * many production files also changed. A legitimate auto-remediation updates + * the mock BUILDER to match the SDK; it never edits a comparison/SDK/harness + * leg (those change only on a deliberate human vendored-SDK bump). This + * closes the WS-2b hybrid cheat (relax a leg + one trivial on-target + * production edit) that the old "only check legs when productionFiles===0" + * logic passed straight to auto-merge. Schema/allowlist and `*.drift.ts` + * assertion edits always map to SUPPRESSION_SUSPECTED (actively silencing + * the detector); other gameable legs (sdk-shapes, harness incl the + * dual-classified voice-models.ts) map to SUPPRESSION_SUSPECTED when paired + * with a production change and COMPARISON_LEG_ONLY when standalone. * * Additionally, the production change should intersect the report's SANCTIONED * target set (`union(entry.builderFile, entry.typesFile≠null)`); an off-target * production change is a WARNING that still blocks (a shared helper MAY be the - * real fix, so it is distinct from an outright cheat). + * real fix, so it is distinct from an outright cheat). An EMPTY sanctioned set is + * fail-closed (fix #3): with no named target we cannot verify the change landed + * where the drift lives, so it routes to human rather than rubber-stamps. * * CLI exit codes (mirrors drift-report-collector.ts's distinct-code discipline): * 0 — RESOLVED * 10 — NO_PRODUCTION_CHANGE - * 11 — COMPARISON_LEG_ONLY (the headline cheat) - * 12 — SUPPRESSION_SUSPECTED (allowlist / *.drift.ts assertion edited) - * 13 — STILL_DIRTY (post-fix collector exit 2) - * 14 — QUARANTINE_AFTER_FIX (post-fix collector exit 5) - * 15 — COLLECTOR_INFRA (post-fix collector exit 1) - * 16 — PRODUCTION_CHANGE_OFF_TARGET (WARNING, still blocks) - * 2 — CONFIG_ERROR (missing/unreadable report, bad args) + * 11 — COMPARISON_LEG_ONLY (leg edit with NO production change) + * 12 — SUPPRESSION_SUSPECTED (schema/*.drift.ts edit, OR any gameable + * leg edited ALONGSIDE a production change) + * 13 — STILL_DIRTY (post-fix collector exit 2, or exit 0 with + * criticalCount>0) + * 14 — QUARANTINE_AFTER_FIX (post-fix collector exit 5 — checked BEFORE + * criticalCount, so exit 5 wins) + * 15 — COLLECTOR_INFRA (post-fix collector exit 1 — likewise wins + * over criticalCount) + * 16 — PRODUCTION_CHANGE_OFF_TARGET (off-target OR zero sanctioned targets; + * WARNING, still blocks) + * 2 — CONFIG_ERROR (missing/unreadable report, bad args, or a + * --changed-file list that disagrees w/ git) * * Usage: * npx tsx scripts/drift-success-predicate.ts \ @@ -53,7 +69,10 @@ * [--changed-file src/helpers.ts ...] * * When no --changed-file args are supplied the CLI derives the changed set from - * `git status --porcelain` (mirrors fix-drift.ts:getChangedFiles()). + * `git status --porcelain` (mirrors fix-drift.ts:getChangedFiles()). When a + * --changed-file list IS supplied it is cross-checked against git and rejected + * (CONFIG_ERROR) on any mismatch (fix #4 — a leg-omitting list must not blind + * the predicate). */ import { execSync } from "node:child_process"; @@ -99,7 +118,18 @@ export interface PredicateInputs { report: DriftReport; /** Exit code of the re-run collector (0 clean / 2 dirty / 5 quarantine / 1 infra). */ postFixCollectorExit: number; - /** criticalCount parsed from the re-run report (belt-and-suspenders vs exit 0). */ + /** + * criticalCount parsed from the re-run report (belt-and-suspenders vs exit 0). + * + * FIX #7 — INDEPENDENCE CAVEAT: this signal (and the collector exit code) is + * derived from the SAME fixtures the fixer was told to make pass, so it is + * NOT independent of a fixture-relaxation cheat — a run that relaxed the SDK + * leg reports clean here too. It is therefore only trustworthy BECAUSE fix #1 + * now blocks ANY gameable-leg edit (see isGameableLeg / the SUPPRESSION_SUSPECTED + * + COMPARISON_LEG_ONLY branches) regardless of production files. That + * always-block rule is load-bearing: it is what makes a clean post-fix signal + * mean "the mock was really fixed" rather than "the leg was relaxed". + */ postFixCriticalCount: number; } @@ -145,12 +175,15 @@ const HARNESS_FILES: ReadonlySet = new Set([ * (the known-models canary routes fixes to these model-list files). These are * NOT gameable comparison legs — adding a newly-shipped model id here is a * legit fix, not a relaxation. They are allowed as accompanying changes and are - * never counted as a standalone comparison-leg cheat. + * never counted as a comparison-leg cheat. + * + * FIX #2 — `voice-models.ts` is deliberately NOT listed here: it is also a + * real-API HARNESS file, and block-classification wins over legit-accept + * (fail-closed precedence). It is classified as a gameable leg in isGameableLeg. */ const LEGIT_FIXTURE_TARGETS: ReadonlySet = new Set([ "src/__tests__/drift/model-registry.ts", "src/__tests__/drift/model-family.ts", - "src/__tests__/drift/voice-models.ts", ]); /** @@ -171,30 +204,59 @@ export function isDriftTestFile(file: string): boolean { } /** - * SUPPRESSION surface: editing these ALWAYS blocks, even alongside a production - * change, because silencing the detector (allowlist growth, loosened assertions) - * is never a valid drift fix. The schema file carries ALLOWLISTED_PATHS; the - * `*.drift.ts` files carry the assertions. + * GAMEABLE-LEG (the block set). Editing ANY of these can erase a drift diff + * WITHOUT changing the mock output — either by relaxing a comparison leg (the + * SDK-shape fixture, the schema/allowlist, the real-API harness) or by loosening + * a `*.drift.ts` assertion. Every one of these is a HUMAN-REVIEWED artifact: a + * legitimate auto-remediation updates the mock BUILDER to match the SDK, and the + * leg only changes on a deliberate human vendored-SDK bump. So any leg edit — + * ALONE OR ACCOMPANIED BY A PRODUCTION CHANGE — is fail-closed to needs-human + * (SUPPRESSION_SUSPECTED). This closes the WS-2b hybrid cheat (relax a leg + + * one trivial on-target production edit), which the old "only check legs when + * productionFiles===0" logic let through to auto-merge. + * + * FIX #2 — CLASSIFICATION PRECEDENCE: a file that is BOTH a harness leg AND a + * legit fixture target (e.g. `voice-models.ts`) is treated as gameable (block). + * Block-classification wins over legit-accept; fail-closed. Only PURE legit + * fixture targets (model-registry.ts / model-family.ts), which are NOT harness + * legs, are excluded — a canary model-list fix routes to those plus its + * production builder and is not blocked. + */ +export function isGameableLeg(file: string): boolean { + // Block-set membership is checked FIRST (fix #2 precedence): a file that is + // BOTH a harness leg AND a legit fixture target (voice-models.ts) matches + // HARNESS_FILES here and blocks, before the legit-target set is ever consulted. + if (file === SDK_SHAPES_FILE) return true; + if (file === SCHEMA_FILE) return true; + if (HARNESS_FILES.has(file)) return true; + if (isDriftTestFile(file)) return true; + // PURE legit fixture targets (not also a harness leg) are explicitly + // non-gameable — a canary model-list fix routes here plus its production + // builder and must not be blocked. + if (LEGIT_FIXTURE_TARGETS.has(file)) return false; + // Everything else (production files, unrelated paths) is non-gameable. + return false; +} + +/** + * SUPPRESSION surface (the NARROW always-SUPPRESSION_SUSPECTED subset): the + * triangulation schema/allowlist and `*.drift.ts` assertion files. Editing these + * ACTIVELY SILENCES the detector (allowlist growth, loosened assertion) and is + * never a valid fix — so it ALWAYS maps to SUPPRESSION_SUSPECTED, standalone or + * alongside a production change. (The broader gameable-leg set below — sdk-shapes, + * harness — also always blocks per fix #1, but maps to COMPARISON_LEG_ONLY when + * standalone and SUPPRESSION_SUSPECTED only when paired with a production change.) */ export function isSuppressionSurface(file: string): boolean { return file === SCHEMA_FILE || isDriftTestFile(file); } /** - * GAMEABLE-COMPARISON-LEG: a comparison leg (SDK fixture, schema, real-API - * harness, or drift-test assertion) whose edit can erase a diff WITHOUT - * changing mock output. Explicitly EXCLUDES LEGIT_FIXTURE_TARGETS - * (canary model-list fixtures) — those are legit fix targets, not cheats. - * Note `voice-models.ts` is both a harness file AND a legit canary target; it - * is treated as a legit target (excluded here) so a canary fix is not blocked. + * GAMEABLE-COMPARISON-LEG (retained for API compatibility / classification + * unit coverage). Same full set as isGameableLeg — every leg edit blocks. */ export function isComparisonLeg(file: string): boolean { - if (LEGIT_FIXTURE_TARGETS.has(file)) return false; - if (file === SDK_SHAPES_FILE) return true; - if (file === SCHEMA_FILE) return true; - if (HARNESS_FILES.has(file)) return true; - if (isDriftTestFile(file)) return true; - return false; + return isGameableLeg(file); } /** @@ -220,16 +282,14 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { // ---- Signal 1: AUTHORITATIVE — collector clean on re-run. ------------- // Checked FIRST: a dirty/quarantine/infra collector makes any file-set moot. - if (postFixCollectorExit === 2 || postFixCriticalCount > 0) { - return { - resolved: false, - reason: PredicateReason.STILL_DIRTY, - detail: - "Post-fix drift collector still reports critical drift " + - `(exit ${postFixCollectorExit}, criticalCount ${postFixCriticalCount}). The fix did not resolve the drift.`, - offendingFiles: [], - }; - } + // + // FIX #6 — the collector-STATE classification (exit 2/5/1) is checked BEFORE + // the belt-and-suspenders criticalCount>0 branch. A quarantine (exit 5) or an + // infra failure (exit 1) that ALSO happens to carry a parseable criticalCount>0 + // is a quarantine/infra event, NOT a plain STILL_DIRTY — it gets its own + // distinct reason so the Slack alert names the real cause. STILL_DIRTY is + // reserved for exit 2, or a clean-looking exit 0 that nonetheless reports + // criticalCount>0 (report/exit disagreement). if (postFixCollectorExit === 5) { return { resolved: false, @@ -248,6 +308,16 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { offendingFiles: [], }; } + if (postFixCollectorExit === 2 || postFixCriticalCount > 0) { + return { + resolved: false, + reason: PredicateReason.STILL_DIRTY, + detail: + "Post-fix drift collector still reports critical drift " + + `(exit ${postFixCollectorExit}, criticalCount ${postFixCriticalCount}). The fix did not resolve the drift.`, + offendingFiles: [], + }; + } if (postFixCollectorExit !== 0) { // Any other non-zero exit is an unrecognized collector state — fail closed. return { @@ -260,12 +330,13 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { // ---- Classify the changed-file set. ----------------------------------- const productionFiles = changedFiles.filter(isProductionFile); - const comparisonLegFiles = changedFiles.filter(isComparisonLeg); + const gameableLegFiles = changedFiles.filter(isGameableLeg); const suppressionFiles = changedFiles.filter(isSuppressionSurface); - // ---- Signal 3 (suppression): ALWAYS block. ---------------------------- - // Edits to the allowlist/schema or a *.drift.ts assertion silence the - // detector and are never a valid fix — block even with a production change. + // ---- Signal 3a (suppression surface): ALWAYS block, standalone or paired. - + // Editing the schema/allowlist or a *.drift.ts assertion actively SILENCES the + // detector — never a valid fix, even alongside a production change. Distinct + // reason so the workflow's Slack alert names "silenced the detector". if (suppressionFiles.length > 0) { return { resolved: false, @@ -277,21 +348,46 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { }; } - // ---- Signal 2: PRODUCTION change present. ------------------------------ - if (productionFiles.length === 0) { - // No production change at all. Distinguish the pure comparison-leg cheat - // (the headline case) from a truly empty / non-source change. - if (comparisonLegFiles.length > 0) { + // ---- Signal 3b (gameable leg): ALWAYS block, independent of production. - + // FIX #1 (HEADLINE) + FIX #2 — a legitimate auto-remediation updates the mock + // BUILDER to match the SDK; it NEVER edits a comparison/SDK/harness leg (those + // change only on a deliberate human vendored-SDK bump). So the presence of ANY + // gameable-leg file blocks REGARDLESS of how many production files also + // changed. This closes the WS-2b hybrid cheat (relax sdk-shapes.ts + one + // trivial on-target production edit) that the old "only check legs when + // productionFiles===0" logic passed straight to auto-merge. voice-models.ts is + // dual-classified (harness + legit target) and blocks here (fix #2 precedence). + // + // Reason split (both distinct, both NEEDS-HUMAN in the workflow): + // • leg edit WITH a production change → SUPPRESSION_SUSPECTED — the WS-2b + // hybrid, the dangerous auto-merge vector. + // • leg edit with NO production change → COMPARISON_LEG_ONLY — a pure + // relaxation, no mock fix even attempted. + if (gameableLegFiles.length > 0) { + if (productionFiles.length > 0) { return { resolved: false, - reason: PredicateReason.COMPARISON_LEG_ONLY, + reason: PredicateReason.SUPPRESSION_SUSPECTED, detail: - "Fix changed ONLY comparison-leg files " + - `(${comparisonLegFiles.join(", ")}) with no production mock-builder change — ` + - "this relaxes the drift detector instead of fixing the mock. The exact cheat this gate blocks.", - offendingFiles: comparisonLegFiles, + "Fix edited a gameable comparison/SDK/harness leg " + + `(${gameableLegFiles.join(", ")}) ALONGSIDE a production change — relaxing a leg is never a valid ` + + "drift fix (a real fix updates the mock builder, not the leg). The WS-2b hybrid cheat. Needs human review.", + offendingFiles: gameableLegFiles, }; } + return { + resolved: false, + reason: PredicateReason.COMPARISON_LEG_ONLY, + detail: + "Fix changed ONLY comparison/SDK/harness-leg files " + + `(${gameableLegFiles.join(", ")}) with no production mock-builder change — ` + + "this relaxes the drift detector instead of fixing the mock. The exact cheat this gate blocks.", + offendingFiles: gameableLegFiles, + }; + } + + // ---- Signal 2: PRODUCTION change present. ------------------------------ + if (productionFiles.length === 0) { return { resolved: false, reason: PredicateReason.NO_PRODUCTION_CHANGE, @@ -301,12 +397,28 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { }; } - // ---- Signal 3 (off-target WARNING): production change must intersect --- - // the report's sanctioned target set. A shared helper MAY legitimately be - // the real fix, so this WARNS and blocks but is distinct from a cheat. + // ---- Signal 3 (off-target): production change must intersect the report's + // sanctioned target set. A shared helper MAY legitimately be the real fix, so + // this WARNS and blocks but is distinct from a cheat. + // + // FIX #3 — an EMPTY sanctioned-target set is fail-closed, not a free pass. The + // old `targets.size > 0` guard disabled the off-target check entirely when the + // report named no targets, accepting ANY production change. An empty target set + // means the report could not name where to fix the drift — route to human + // rather than rubber-stamp an unverifiable production change. const targets = sanctionedTargets(report); + if (targets.size === 0) { + return { + resolved: false, + reason: PredicateReason.PRODUCTION_CHANGE_OFF_TARGET, + detail: + "Drift report named ZERO sanctioned fix targets (no builderFile/typesFile) — cannot verify the " + + `production change (${productionFiles.join(", ")}) landed where the drift lives. Fail-closed, needs human review.`, + offendingFiles: productionFiles, + }; + } const onTarget = productionFiles.some((f) => targets.has(f)); - if (targets.size > 0 && !onTarget) { + if (!onTarget) { return { resolved: false, reason: PredicateReason.PRODUCTION_CHANGE_OFF_TARGET, @@ -422,6 +534,19 @@ export function readReport(path: string): DriftReport { `Drift report at ${path} has invalid structure: expected { entries: [...] }`, ); } + // FIX #6 — align this (previously loose) validator with the stricter + // fix-drift.ts:readDriftReport: a report missing/carrying a non-string + // `timestamp` is a corrupt/truncated collector run and must fail-closed rather + // than be silently trusted as a clean "drift is gone" signal. NOTE: an EMPTY + // `entries: []` is intentionally ACCEPTED — that is exactly what the collector + // emits when no drift remains (the legitimate clean signal); the trust anchor + // for "clean" is the collector EXIT CODE plus fix #1's always-block-on-leg-edit + // rule, not a non-empty entries array. + if (typeof (parsed as Record).timestamp !== "string") { + throw new PredicateConfigError( + `Drift report at ${path} is missing a string "timestamp" — corrupt/truncated report, failing closed`, + ); + } return parsed as DriftReport; } @@ -454,6 +579,33 @@ export function gitChangedFiles(): string[] { return out.split("\n").filter(Boolean).map(parsePorcelainLine); } +/** + * FIX #4 — AUTHORITATIVE changed-file set. The git working tree is the single + * source of truth for what actually changed. An explicit `--changed-file` list + * is only a hint, and a supplied list that OMITS a relaxed leg would BLIND the + * predicate (the exact WS-2b vector, from the other side). So when a list is + * supplied we cross-check it against git as a set and fail-closed + * (PredicateConfigError → exit 2) on ANY disagreement — a missing file (leg + * hidden) OR an extra file (phantom) both mean the caller's view diverges from + * ground truth and the verdict cannot be trusted. When no list is supplied we + * use the git set directly. + */ +export function crossCheckChangedFiles(explicit: string[] | null, git: string[]): string[] { + if (explicit === null) return git; + const gitSet = new Set(git); + const explicitSet = new Set(explicit); + const missing = git.filter((f) => !explicitSet.has(f)); // git has it, list omits it + const extra = explicit.filter((f) => !gitSet.has(f)); // list has it, git does not + if (missing.length > 0 || extra.length > 0) { + throw new PredicateConfigError( + "--changed-file list disagrees with the git working tree (fail-closed): " + + `${missing.length > 0 ? `omitted by the list: ${missing.join(", ")}; ` : ""}` + + `${extra.length > 0 ? `not present in git: ${extra.join(", ")}` : ""}`.trim(), + ); + } + return explicit; +} + /** * Run the predicate from CLI args. Returns the process exit code and prints * `detail` (and offending files for LOUD reasons) to stdout/stderr. @@ -481,12 +633,15 @@ export function runCli(argv: string[]): number { return REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]; } + // FIX #4 — always derive the AUTHORITATIVE changed set from git, and + // cross-check any supplied --changed-file list against it (fail-closed on + // mismatch) so a leg-omitting list cannot blind the predicate. let changedFiles: string[]; try { - changedFiles = args.changedFiles ?? gitChangedFiles(); + changedFiles = crossCheckChangedFiles(args.changedFiles, gitChangedFiles()); } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); - console.error(`CONFIG_ERROR: could not read changed files from git: ${msg}`); + console.error(`CONFIG_ERROR: ${msg}`); console.log(`reason=${PredicateReason.CONFIG_ERROR}`); return REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]; } diff --git a/scripts/fix-drift.ts b/scripts/fix-drift.ts index b16ebc47..3502c27a 100644 --- a/scripts/fix-drift.ts +++ b/scripts/fix-drift.ts @@ -46,6 +46,7 @@ import { readReport as readPostFixReport, countCriticalDiffs, REASON_EXIT_CODE, + PredicateReason, } from "./drift-success-predicate.js"; import type { DriftReport, DriftSeverity } from "./drift-types.js"; @@ -658,34 +659,41 @@ function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { // resolved. This runs BEFORE any git add/commit — a blocked verdict opens no // PR (and therefore never reaches auto-merge). Without --post-fix-* args, // fall back to the legacy "no source files changed" guard (exit 4). - if (postFix) { - const verdict = evaluateDriftResolved({ - changedFiles, - report, - postFixCollectorExit: postFix.exitCode, - postFixCriticalCount: countCriticalDiffs(postFix.report), - }); - if (!verdict.resolved) { - console.error(`ERROR: Drift NOT resolved [${verdict.reason}]: ${verdict.detail}`); - if (verdict.offendingFiles.length > 0) { - console.error(`Offending files: ${verdict.offendingFiles.join(", ")}`); - } - console.error("Aborting PR creation — this fix will be routed to human review."); - console.log(`reason=${verdict.reason}`); - process.exit(REASON_EXIT_CODE[verdict.reason]); - } - console.log(`Drift-success predicate: RESOLVED — ${verdict.detail}`); - } else { - // Legacy guard: abort if no source files changed (a version-bump-only PR - // would be misleading). Superseded by the predicate above when present. - if (builderFiles.length === 0 && testFiles.length === 0) { - console.error( - "ERROR: No source files changed. Claude Code may not have made any fixes, " + - "or all changes were reverted during verification. Aborting PR creation.", - ); - process.exit(4); // no source files changed (distinct from exit 2 = critical drift) + // FIX #5 — the drift-success predicate is MANDATORY. The old legacy fallback + // (accept a PR when `builderFiles.length || testFiles.length`) re-opened the + // original fixture-only cheat: a run that changed only comparison-leg test + // files satisfied `testFiles.length > 0` and sailed through to a PR. There is + // no safe "no post-fix result" path — without the authoritative post-fix + // collector signal we cannot tell a real fix from a relaxation, so we + // fail-closed to human review (COLLECTOR_INFRA) rather than open a PR. The + // real workflow (fix-drift.yml "Create PR" step) always supplies + // --post-fix-report/--post-fix-exit, so this only fires on a misconfigured + // invocation — which must NOT auto-merge. + if (!postFix) { + console.error( + "ERROR: PR-open gate requires the post-fix collector result " + + "(--post-fix-report + --post-fix-exit). Refusing to open a PR without the " + + "authoritative drift-success signal — routing to human review.", + ); + console.log(`reason=${PredicateReason.COLLECTOR_INFRA}`); + process.exit(REASON_EXIT_CODE[PredicateReason.COLLECTOR_INFRA]); + } + const verdict = evaluateDriftResolved({ + changedFiles, + report, + postFixCollectorExit: postFix.exitCode, + postFixCriticalCount: countCriticalDiffs(postFix.report), + }); + if (!verdict.resolved) { + console.error(`ERROR: Drift NOT resolved [${verdict.reason}]: ${verdict.detail}`); + if (verdict.offendingFiles.length > 0) { + console.error(`Offending files: ${verdict.offendingFiles.join(", ")}`); } + console.error("Aborting PR creation — this fix will be routed to human review."); + console.log(`reason=${verdict.reason}`); + process.exit(REASON_EXIT_CODE[verdict.reason]); } + console.log(`Drift-success predicate: RESOLVED — ${verdict.detail}`); // Determine branch name let currentBranch: string; @@ -856,6 +864,20 @@ export function parseMode(args: string[]): "pr" | "issue" | "default" { return "default"; } +/** + * FIX #5 — true only when BOTH post-fix collector flags are present with values. + * PR mode requires both (the drift-success predicate is the authoritative + * PR-open gate); there is no legacy no-post-fix fallback that could re-open the + * fixture-only cheat. + */ +export function hasPostFixArgs(args: string[]): boolean { + const reportIdx = args.indexOf("--post-fix-report"); + const exitIdx = args.indexOf("--post-fix-exit"); + const hasReport = reportIdx !== -1 && args[reportIdx + 1] !== undefined; + const hasExit = exitIdx !== -1 && args[exitIdx + 1] !== undefined; + return hasReport && hasExit; +} + async function main(): Promise { const args = process.argv.slice(2); const mode = parseMode(args); @@ -888,28 +910,26 @@ async function main(): Promise { console.log(`Loaded drift report: ${report.entries.length} entries from ${report.timestamp}`); if (mode === "pr") { - // Parse the optional post-fix collector result. When BOTH flags are - // supplied, the drift-success predicate gates PR creation. If only one is - // supplied the args are malformed — fail rather than silently skip the gate. const postFixReportIdx = args.indexOf("--post-fix-report"); const postFixExitIdx = args.indexOf("--post-fix-exit"); - const hasPostFixReport = postFixReportIdx !== -1 && args[postFixReportIdx + 1] !== undefined; - const hasPostFixExit = postFixExitIdx !== -1 && args[postFixExitIdx + 1] !== undefined; - let postFix: PostFixCollectorResult | undefined; - if (hasPostFixReport || hasPostFixExit) { - if (!hasPostFixReport || !hasPostFixExit) { - throw new Error( - "--post-fix-report and --post-fix-exit must be supplied together (the drift-success predicate needs both)", - ); - } - const postFixExit = Number(args[postFixExitIdx + 1]); - if (!Number.isInteger(postFixExit)) { - throw new Error(`--post-fix-exit must be an integer, got "${args[postFixExitIdx + 1]}"`); - } - const postFixReport = readPostFixReport(resolve(args[postFixReportIdx + 1])); - postFix = { report: postFixReport, exitCode: postFixExit }; + // FIX #5 — the post-fix collector result is REQUIRED in PR mode. The old + // path allowed --create-pr with NO post-fix args, which fell back to the + // gameable legacy guard (a test-file-only change satisfied it and opened a + // PR). Both flags must be present; a missing/partial pair throws rather than + // silently skipping the authoritative drift-success gate. + if (!hasPostFixArgs(args)) { + throw new Error( + "--create-pr requires BOTH --post-fix-report and --post-fix-exit (the drift-success " + + "predicate is the authoritative PR-open gate; there is no legacy no-post-fix path)", + ); + } + const postFixExit = Number(args[postFixExitIdx + 1]); + if (!Number.isInteger(postFixExit)) { + throw new Error(`--post-fix-exit must be an integer, got "${args[postFixExitIdx + 1]}"`); } + const postFixReport = readPostFixReport(resolve(args[postFixReportIdx + 1])); + const postFix: PostFixCollectorResult = { report: postFixReport, exitCode: postFixExit }; createPr(report, postFix); } else { diff --git a/src/__tests__/drift-success-predicate.test.ts b/src/__tests__/drift-success-predicate.test.ts index 5ac85d11..a1863723 100644 --- a/src/__tests__/drift-success-predicate.test.ts +++ b/src/__tests__/drift-success-predicate.test.ts @@ -26,9 +26,12 @@ import { isProductionFile, isComparisonLeg, isSuppressionSurface, + isGameableLeg, sanctionedTargets, countCriticalDiffs, parseCliArgs, + parsePorcelainLine, + crossCheckChangedFiles, PredicateConfigError, readReport, runCli, @@ -196,6 +199,135 @@ describe("evaluateDriftResolved — RED (cheat/failure) cases", () => { expect(verdict.reason).toBe(PredicateReason.PRODUCTION_CHANGE_OFF_TARGET); expect(REASON_EXIT_CODE[verdict.reason]).toBe(16); }); + + // ------------------------------------------------------------------------- + // WS-2b HYBRID CHEAT — a gameable-leg edit ACCOMPANIED by a trivial, on-target + // production edit. Pre-fix the predicate ignored the leg once ANY production + // file changed → RESOLVED (the exact WS-2b auto-merge cheat). Post-fix a leg + // edit ALWAYS blocks (SUPPRESSION_SUSPECTED), regardless of production files. + // ------------------------------------------------------------------------- + it("HEADLINE WS-2b: sdk-shapes.ts relaxation + on-target production edit → SUPPRESSION_SUSPECTED (block), NOT resolved", () => { + const changedFiles = ["src/__tests__/drift/sdk-shapes.ts", "src/helpers.ts"]; + const verdict = evaluateDriftResolved({ + changedFiles, + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.SUPPRESSION_SUSPECTED); + expect(verdict.offendingFiles).toContain("src/__tests__/drift/sdk-shapes.ts"); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(12); + // Contrast: the OLD guard would have proceeded (both builder + test present). + expect(oldGuardWouldAccept(changedFiles)).toBe(true); + }); + + it("harness leg (providers.ts) relaxation + on-target production edit → SUPPRESSION_SUSPECTED (block)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/__tests__/drift/providers.ts", "src/helpers.ts"], + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.SUPPRESSION_SUSPECTED); + expect(verdict.offendingFiles).toContain("src/__tests__/drift/providers.ts"); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(12); + }); + + it("harness-only leg edit (ws-providers.ts), no production change → COMPARISON_LEG_ONLY (block, pure relaxation)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/__tests__/drift/ws-providers.ts"], + report: report(), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + // Leg edit with NO production change → COMPARISON_LEG_ONLY (a pure + // relaxation; no mock fix even attempted). Still a hard block (exit 11). + expect(verdict.reason).toBe(PredicateReason.COMPARISON_LEG_ONLY); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(11); + }); + + // ------------------------------------------------------------------------- + // FIX #2 — dual-classification precedence: voice-models.ts is BOTH a harness + // leg AND a legit fixture target. Block-classification MUST win (fail-closed). + // ------------------------------------------------------------------------- + it("voice-models.ts (dual-classified harness+target) + on-target production edit → SUPPRESSION_SUSPECTED (block wins)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/__tests__/drift/voice-models.ts", "src/ws-realtime.ts"], + report: report([entry({ builderFile: "src/ws-realtime.ts", typesFile: null })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.SUPPRESSION_SUSPECTED); + expect(verdict.offendingFiles).toContain("src/__tests__/drift/voice-models.ts"); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(12); + }); + + // ------------------------------------------------------------------------- + // FIX #3 — empty sanctioned-target set must fail closed (needs-human), not + // silently accept any production change by disabling the off-target guard. + // ------------------------------------------------------------------------- + it("empty sanctionedTargets (report entries have no usable target) → PRODUCTION_CHANGE_OFF_TARGET (fail-closed)", () => { + // Fabricate a report whose entries yield ZERO sanctioned targets: builderFile + // "" and typesFile null. (evaluateDriftResolved does not re-validate the + // report shape — it only reads builderFile/typesFile via sanctionedTargets.) + const emptyTargetReport: DriftReport = { + timestamp: "2026-07-16T00:00:00.000Z", + entries: [ + { + provider: "OpenAI", + scenario: "chat completion", + builderFile: "", + builderFunctions: ["buildChatCompletion"], + typesFile: null, + sdkShapesFile: "src/__tests__/drift/sdk-shapes.ts", + diffs: [diff()], + }, + ], + }; + expect(sanctionedTargets(emptyTargetReport).size).toBe(0); + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: emptyTargetReport, + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.PRODUCTION_CHANGE_OFF_TARGET); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(16); + }); + + // ------------------------------------------------------------------------- + // FIX #6 — exit 5/1 WITH parseable criticalCount>0 gets its OWN reason + // (quarantine/infra), NOT STILL_DIRTY. The collector-state classification + // wins over the belt-and-suspenders criticalCount check. + // ------------------------------------------------------------------------- + it("post-fix quarantine (exit 5) with criticalCount>0 → QUARANTINE_AFTER_FIX (not STILL_DIRTY)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: report(), + postFixCollectorExit: 5, + postFixCriticalCount: 4, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.QUARANTINE_AFTER_FIX); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(14); + }); + + it("post-fix infra (exit 1) with criticalCount>0 → COLLECTOR_INFRA (not STILL_DIRTY)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: report(), + postFixCollectorExit: 1, + postFixCriticalCount: 4, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.COLLECTOR_INFRA); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(15); + }); }); // --------------------------------------------------------------------------- @@ -288,28 +420,50 @@ describe("file classification", () => { expect(isProductionFile("scripts/fix-drift.ts")).toBe(false); }); - it("isComparisonLeg flags SDK/schema/harness/*.drift.ts but NOT legit fixture targets", () => { + it("isComparisonLeg flags SDK/schema/harness/*.drift.ts (incl dual-classified voice-models) but NOT pure legit targets", () => { expect(isComparisonLeg("src/__tests__/drift/sdk-shapes.ts")).toBe(true); expect(isComparisonLeg("src/__tests__/drift/schema.ts")).toBe(true); expect(isComparisonLeg("src/__tests__/drift/providers.ts")).toBe(true); expect(isComparisonLeg("src/__tests__/drift/ws-providers.ts")).toBe(true); expect(isComparisonLeg("src/__tests__/drift/helpers.ts")).toBe(true); expect(isComparisonLeg("src/__tests__/drift/openai-chat.drift.ts")).toBe(true); - // Legit fixture targets are NOT comparison legs. + // Dual-classified voice-models.ts blocks (fix #2: harness membership wins). + expect(isComparisonLeg("src/__tests__/drift/voice-models.ts")).toBe(true); + // PURE legit fixture targets (not also a harness leg) are NOT comparison legs. expect(isComparisonLeg("src/__tests__/drift/model-registry.ts")).toBe(false); expect(isComparisonLeg("src/__tests__/drift/model-family.ts")).toBe(false); - expect(isComparisonLeg("src/__tests__/drift/voice-models.ts")).toBe(false); // Production files are not comparison legs. expect(isComparisonLeg("src/helpers.ts")).toBe(false); }); - it("isSuppressionSurface flags schema + *.drift.ts only", () => { + it("isSuppressionSurface flags the NARROW always-suppress set (schema + *.drift.ts) only", () => { + // Suppression surface = the actively-silencing subset: schema/allowlist and + // *.drift.ts assertions. These always map to SUPPRESSION_SUSPECTED. The + // broader legs (sdk-shapes / harness) are gameable but are NOT suppression + // surfaces (they map to COMPARISON_LEG_ONLY when standalone). expect(isSuppressionSurface("src/__tests__/drift/schema.ts")).toBe(true); expect(isSuppressionSurface("src/__tests__/drift/openai-chat.drift.ts")).toBe(true); expect(isSuppressionSurface("src/__tests__/drift/sdk-shapes.ts")).toBe(false); + expect(isSuppressionSurface("src/__tests__/drift/providers.ts")).toBe(false); + expect(isSuppressionSurface("src/__tests__/drift/voice-models.ts")).toBe(false); expect(isSuppressionSurface("src/helpers.ts")).toBe(false); }); + it("isGameableLeg flags every leg (incl dual-classified voice-models) but NOT model-registry/model-family/production", () => { + expect(isGameableLeg("src/__tests__/drift/sdk-shapes.ts")).toBe(true); + expect(isGameableLeg("src/__tests__/drift/schema.ts")).toBe(true); + expect(isGameableLeg("src/__tests__/drift/providers.ts")).toBe(true); + expect(isGameableLeg("src/__tests__/drift/ws-providers.ts")).toBe(true); + expect(isGameableLeg("src/__tests__/drift/helpers.ts")).toBe(true); + expect(isGameableLeg("src/__tests__/drift/openai-chat.drift.ts")).toBe(true); + // Dual-classified: harness membership wins over legit-target (fix #2). + expect(isGameableLeg("src/__tests__/drift/voice-models.ts")).toBe(true); + // Pure legit fixture targets are NOT gameable legs. + expect(isGameableLeg("src/__tests__/drift/model-registry.ts")).toBe(false); + expect(isGameableLeg("src/__tests__/drift/model-family.ts")).toBe(false); + expect(isGameableLeg("src/helpers.ts")).toBe(false); + }); + it("sanctionedTargets unions builderFile + non-null typesFile", () => { const t = sanctionedTargets( report([ @@ -391,9 +545,68 @@ describe("parseCliArgs", () => { }); describe("readReport", () => { + let dir: string | null = null; + afterEach(() => { + if (dir) rmSync(dir, { recursive: true, force: true }); + dir = null; + }); + it("throws PredicateConfigError on a missing file", () => { expect(() => readReport("/no/such/drift-report.json")).toThrow(PredicateConfigError); }); + + // FIX #6 — align the strict/loose validators. readReport must fail-closed on a + // structurally-untrustworthy report the same way fix-drift.ts:readDriftReport + // does: a missing/non-string `timestamp` is a corrupt/truncated collector run + // and must be REJECTED, never silently trusted as a clean signal. + it("rejects a report missing the timestamp field (fail-closed, aligns with readDriftReport)", () => { + dir = mkdtempSync(join(tmpdir(), "ws2-readreport-")); + const p = join(dir, "no-ts.json"); + writeFileSync(p, JSON.stringify({ entries: [] }), "utf-8"); + expect(() => readReport(p)).toThrow(PredicateConfigError); + }); + + it("rejects a non-object / non-{entries:[]} structure", () => { + dir = mkdtempSync(join(tmpdir(), "ws2-readreport-")); + const p = join(dir, "bad.json"); + writeFileSync(p, JSON.stringify({ timestamp: "t", entries: "nope" }), "utf-8"); + expect(() => readReport(p)).toThrow(PredicateConfigError); + }); + + // A legitimately-clean post-fix report IS { entries: [] } — the collector + // emits exactly that when no drift remains. It must be ACCEPTED (the trust + // anchor for "clean" is the collector EXIT CODE, corroborated by fix #1's + // always-block-on-leg-edit rule, not the entries array being non-empty). + it("accepts a well-formed EMPTY report (the genuine clean-collector signal)", () => { + dir = mkdtempSync(join(tmpdir(), "ws2-readreport-")); + const p = join(dir, "clean.json"); + writeFileSync(p, JSON.stringify({ timestamp: "t", entries: [] }), "utf-8"); + expect(() => readReport(p)).not.toThrow(); + }); + + it("accepts a well-formed non-empty report", () => { + dir = mkdtempSync(join(tmpdir(), "ws2-readreport-")); + const p = join(dir, "ok.json"); + writeFileSync(p, JSON.stringify(report()), "utf-8"); + expect(() => readReport(p)).not.toThrow(); + }); +}); + +describe("parsePorcelainLine", () => { + it("strips the 2-char status + space prefix", () => { + expect(parsePorcelainLine(" M src/helpers.ts")).toBe("src/helpers.ts"); + expect(parsePorcelainLine("?? src/new.ts")).toBe("src/new.ts"); + expect(parsePorcelainLine("A src/added.ts")).toBe("src/added.ts"); + }); + + it("takes the NEW path from rename notation (old -> new)", () => { + expect(parsePorcelainLine("R src/old.ts -> src/new.ts")).toBe("src/new.ts"); + }); + + it("unquotes paths with special characters", () => { + expect(parsePorcelainLine('?? "src/weird name.ts"')).toBe("src/weird name.ts"); + expect(parsePorcelainLine('R "src/old x.ts" -> "src/new x.ts"')).toBe("src/new x.ts"); + }); }); // --------------------------------------------------------------------------- @@ -417,7 +630,13 @@ describe("runCli", () => { return { pre: preP, post: postP }; } - it("exits 11 for the comparison-leg-only cheat", () => { + // NOTE (fix #4): runCli now ALWAYS cross-checks any --changed-file list + // against the real git working tree, so a synthetic list that does not match + // the checkout is rejected (exit 2) BEFORE the predicate runs. The exit-code- + // per-reason mapping for the cheat / real-fix cases is covered directly by the + // evaluateDriftResolved + REASON_EXIT_CODE tests above; here we lock the + // cross-check itself (the fix #4 blinding guard) at the runCli boundary. + it("exits 2 (CONFIG_ERROR) when --changed-file disagrees with git (fix #4 blinding guard)", () => { const paths = writeReports(report(), report([])); const code = runCli([ "--report", @@ -429,25 +648,7 @@ describe("runCli", () => { "--changed-file", "src/__tests__/drift/sdk-shapes.ts", ]); - expect(code).toBe(11); - }); - - it("exits 0 for a real production fix", () => { - const paths = writeReports( - report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), - report([]), - ); - const code = runCli([ - "--report", - paths.pre, - "--post-fix-report", - paths.post, - "--post-fix-exit", - "0", - "--changed-file", - "src/helpers.ts", - ]); - expect(code).toBe(0); + expect(code).toBe(2); }); it("exits 2 (CONFIG_ERROR) on a missing post-fix report", () => { @@ -470,3 +671,91 @@ describe("runCli", () => { expect(code).toBe(2); }); }); + +// --------------------------------------------------------------------------- +// FIX #4 — authoritative changed-files: crossCheckChangedFiles +// --------------------------------------------------------------------------- + +describe("crossCheckChangedFiles", () => { + it("returns the git set when no explicit list is supplied", () => { + const git = ["src/helpers.ts", "src/types.ts"]; + expect(crossCheckChangedFiles(null, git)).toEqual(git); + }); + + it("accepts an explicit list that matches the git set (order-independent)", () => { + const git = ["src/helpers.ts", "src/types.ts"]; + expect(crossCheckChangedFiles(["src/types.ts", "src/helpers.ts"], git)).toEqual( + expect.arrayContaining(git), + ); + }); + + it("throws when the explicit list OMITS a file git reports (leg-blinding attack)", () => { + const git = ["src/helpers.ts", "src/__tests__/drift/sdk-shapes.ts"]; + // Attacker passes only the benign production file, hiding the relaxed leg. + expect(() => crossCheckChangedFiles(["src/helpers.ts"], git)).toThrow(PredicateConfigError); + }); + + it("throws when the explicit list ADDS a file git does not report", () => { + const git = ["src/helpers.ts"]; + expect(() => crossCheckChangedFiles(["src/helpers.ts", "src/phantom.ts"], git)).toThrow( + PredicateConfigError, + ); + }); +}); + +// --------------------------------------------------------------------------- +// REVERT GUARD + RED-COUNT LOCK +// --------------------------------------------------------------------------- + +describe("revert guard — old always-accept predicate would FAIL these locks", () => { + // The pre-hardening predicate ignored gameable legs once ANY production file + // changed (and had a legacy always-accept fallback). This models that old + // behavior; asserting it DISAGREES with the real predicate on the WS-2b cheat + // proves the hardening is load-bearing. If someone reverts evaluateDriftResolved + // to the old logic, the HEADLINE WS-2b test above flips and the suite breaks. + function oldPredicateWouldAccept(changedFiles: string[]): boolean { + const productionFiles = changedFiles.filter(isProductionFile); + // Old logic: leg files only checked when productionFiles === 0. + return productionFiles.length > 0; + } + + it("WS-2b cheat: old predicate ACCEPTS, real predicate BLOCKS (SUPPRESSION_SUSPECTED)", () => { + const cheat = ["src/__tests__/drift/sdk-shapes.ts", "src/helpers.ts"]; + // Old behavior would have accepted (a production file is present). + expect(oldPredicateWouldAccept(cheat)).toBe(true); + // Real predicate blocks. + const verdict = evaluateDriftResolved({ + changedFiles: cheat, + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.SUPPRESSION_SUSPECTED); + }); +}); + +describe("RED-count lock", () => { + // A structural lock: exactly this many distinct block reasons must reject. + // NOTE: the current RED-case count in the top describe block is 9 (NOT 10) — + // HEADLINE, schema+builder, *.drift.ts, no-change, still-dirty(exit2), + // criticalCount>0, quarantine, infra, off-target. The WS-2b / dual-class / + // empty-targets / exit5+critical / exit1+critical additions raise the total. + const BLOCK_REASONS = [ + PredicateReason.NO_PRODUCTION_CHANGE, + PredicateReason.COMPARISON_LEG_ONLY, + PredicateReason.SUPPRESSION_SUSPECTED, + PredicateReason.STILL_DIRTY, + PredicateReason.QUARANTINE_AFTER_FIX, + PredicateReason.COLLECTOR_INFRA, + PredicateReason.PRODUCTION_CHANGE_OFF_TARGET, + PredicateReason.CONFIG_ERROR, + ]; + + it("every block reason has a distinct non-zero exit code and RESOLVED is 0", () => { + expect(REASON_EXIT_CODE[PredicateReason.RESOLVED]).toBe(0); + const codes = BLOCK_REASONS.map((r) => REASON_EXIT_CODE[r]); + for (const c of codes) expect(c).not.toBe(0); + expect(new Set(codes).size).toBe(codes.length); // all distinct + }); +}); diff --git a/src/__tests__/fix-drift.test.ts b/src/__tests__/fix-drift.test.ts index 09fe8b86..7d122a25 100644 --- a/src/__tests__/fix-drift.test.ts +++ b/src/__tests__/fix-drift.test.ts @@ -39,6 +39,7 @@ import { readFileIfExists, execFileSafe, parseMode, + hasPostFixArgs, getChangedFiles, affectedSkillSections, BUILDER_TO_SKILL_SECTION, @@ -778,6 +779,37 @@ describe("parseMode", () => { }); }); +// --------------------------------------------------------------------------- +// FIX #5 — hasPostFixArgs: PR mode REQUIRES both post-fix flags. The old legacy +// no-post-fix fallback re-opened the fixture-only cheat (a test-file-only change +// satisfied it and opened a PR), so main() throws unless BOTH are present. A +// live subprocess red-green confirmed the OLD code proceeded to `gh pr create` +// with no post-fix args while the NEW code fails-closed BEFORE any git op. +// --------------------------------------------------------------------------- +describe("hasPostFixArgs (fix #5 legacy-fallback closure)", () => { + it("false when NO post-fix flags (the legacy cheat path — must be rejected)", () => { + expect(hasPostFixArgs(["--create-pr", "--report", "drift-report.json"])).toBe(false); + }); + + it("false when only --post-fix-report is present", () => { + expect(hasPostFixArgs(["--create-pr", "--post-fix-report", "post.json"])).toBe(false); + }); + + it("false when only --post-fix-exit is present", () => { + expect(hasPostFixArgs(["--create-pr", "--post-fix-exit", "0"])).toBe(false); + }); + + it("false when a flag is present but its value is missing (trailing flag)", () => { + expect(hasPostFixArgs(["--post-fix-report", "post.json", "--post-fix-exit"])).toBe(false); + }); + + it("true only when BOTH flags carry values", () => { + expect( + hasPostFixArgs(["--create-pr", "--post-fix-report", "post.json", "--post-fix-exit", "0"]), + ).toBe(true); + }); +}); + // --------------------------------------------------------------------------- // getChangedFiles // --------------------------------------------------------------------------- From 07a833cc58b5bdfb1fc5c6c8ec9447a274f7034e Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Fri, 17 Jul 2026 11:57:02 -0700 Subject: [PATCH 3/6] fix(drift): invert success-predicate denylist to an allowlist (default-deny) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-2 adversarial CR showed the "block known gameable legs" denylist still leaks: any editable collector input NOT on the static list (package.json/lockfile pinning a vendored SDK, tsconfig, an imported sub-fixture, an unknown path, a drift-dir *.test.ts) could accompany a token on-target production edit and reach resolved:true. Invert to an allowlist: a fix is RESOLVED only when EVERY changed file is (a) production source (src/** not under src/__tests__/) or (b) a fixture explicitly named for a drift entry in the report (builderFile/typesFile). Anything else blocks with a new UNSANCTIONED_CHANGE reason (exit 17). This closes CR F1/F2/F3 and the drift-dir .test.ts gap by default-deny rather than a stale denylist. Also: - Canonicalize every changed path before classification (strip ./, collapse //, resolve ./.. , fail closed on repo-root escape) so a spelling variant cannot sneak a leg past the exact-string matchers; read git with -c core.quotePath=false (slot-1/slot-2 path findings). - F6: reject empty/whitespace --post-fix-exit (Number("")===0 must not masquerade as a clean exit 0) — fail closed. - F8: catch a malformed post-fix report that would crash countCriticalDiffs and map it to a named config-error, in both runCli and the fix-drift.ts createPr in-script gate, instead of a bare stacktrace with an empty reason= line. - F4: bind the scored reports to the in-workflow re-collect output (a clarifying comment — neither report is repo-committed). - Wire unsanctioned-change into the workflow Slack reason routing. - Signal 2 now accepts a report-named fixture target as a valid fix (canary model-list case), not just production source. - Retain: crossCheckChangedFiles, empty-sanctioned fail-closed, legacy no-post-fix-args fail-closed, voice-models->block, exit-code lock. Red-green: reverting the predicate to the denylist flips 18 tests (allowlist/canonicalization/F6/F8) from green to red; the inverted predicate passes all 67. Full suite 4621 pass, tsc clean. --- .github/workflows/fix-drift.yml | 1 + scripts/drift-success-predicate.ts | 229 ++++++++++--- scripts/fix-drift.ts | 23 +- src/__tests__/drift-success-predicate.test.ts | 313 +++++++++++++++++- 4 files changed, 509 insertions(+), 57 deletions(-) diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index 48e1d058..26328da6 100644 --- a/.github/workflows/fix-drift.yml +++ b/.github/workflows/fix-drift.yml @@ -562,6 +562,7 @@ jobs: # auto-fix tried to game the drift detector rather than fix the mock. comparison-leg-only) DETAIL=" (🚨 NEEDS HUMAN: auto-fix changed ONLY comparison-leg fixtures — a fixture-relaxation cheat, NOT a real mock fix. See run logs for the offending files)";; suppression-suspected) DETAIL=" (🚨 NEEDS HUMAN: auto-fix edited the drift allowlist or a *.drift.ts assertion — silencing the detector, never a valid fix. See run logs)";; + unsanctioned-change) DETAIL=" (🚨 NEEDS HUMAN: auto-fix changed a file NOT on the sanctioned allowlist — deps/config/manifests, unrelated tests, or an unnamed fixture that could game the collector. See run logs for the offending files)";; no-production-change) DETAIL=" (NEEDS HUMAN: auto-fix changed zero production mock-builder files — nothing shippable)";; still-dirty) DETAIL=" (NEEDS HUMAN: post-fix collector still reports critical drift — the fix did not resolve it)";; quarantine-after-fix) DETAIL=" (NEEDS HUMAN: post-fix collector returned quarantine — unparseable/untrusted output after the fix)";; diff --git a/scripts/drift-success-predicate.ts b/scripts/drift-success-predicate.ts index aed497e2..739e9490 100644 --- a/scripts/drift-success-predicate.ts +++ b/scripts/drift-success-predicate.ts @@ -9,6 +9,23 @@ * triangulation schema/allowlist, the real-API harness, or a `*.drift.ts` * assertion). * + * ALLOWLIST MODEL (round-2 CR — replaces the earlier denylist). A denylist of + * "known gameable legs" leaks: any editable collector input NOT on the list + * (package.json/lockfile pinning a vendored SDK, a tsconfig, an imported + * sub-fixture, an unknown path, a drift-dir `*.test.ts`) could accompany a token + * on-target production edit and reach `resolved:true`. So the predicate INVERTS + * to an allowlist: a fix is RESOLVED only when EVERY changed file is on the + * allowlist, which is (a) PRODUCTION SOURCE — `src/**` that is NOT under + * `src/__tests__/` and is not a config/manifest — PLUS (b) a fixture file + * EXPLICITLY NAMED for a drift entry in the report (`entry.builderFile` / + * `entry.typesFile`, e.g. a canary model-registry.ts the collector sanctioned). + * ANYTHING else blocks: any other `src/__tests__/**` file (legs, `*.drift.ts`, + * `*.test.ts`, providers/ws-providers/schema/sdk-shapes), package.json / + * lockfiles / manifests / config, and any unrecognized path. All paths are + * CANONICALIZED (strip `./`, collapse `//`, resolve `.`/`..`, reject repo-root + * escapes) before classification so a spelling variant cannot sneak a leg past + * the matcher. + * * The hole this closes: the drift tests are three-way triangulations * (SDK vs real API vs mock). The SDK leg is literally the repo fixture * `src/__tests__/drift/sdk-shapes.ts`. Deleting a field from that fixture makes @@ -58,8 +75,16 @@ * over criticalCount) * 16 — PRODUCTION_CHANGE_OFF_TARGET (off-target OR zero sanctioned targets; * WARNING, still blocks) - * 2 — CONFIG_ERROR (missing/unreadable report, bad args, or a - * --changed-file list that disagrees w/ git) + * 17 — UNSANCTIONED_CHANGE (a changed file is NOT on the allowlist — + * package.json/lockfile/config/unknown path/ + * non-drift or drift `*.test.ts`; a real fix + * touches only production source + report- + * named fixture targets) + * 2 — CONFIG_ERROR (missing/unreadable report, bad args, a + * --changed-file list that disagrees w/ git, + * a path escaping the repo root, or a + * malformed post-fix report that cannot be + * scored) * * Usage: * npx tsx scripts/drift-success-predicate.ts \ @@ -91,6 +116,7 @@ export enum PredicateReason { NO_PRODUCTION_CHANGE = "no-production-change", COMPARISON_LEG_ONLY = "comparison-leg-only", SUPPRESSION_SUSPECTED = "suppression-suspected", + UNSANCTIONED_CHANGE = "unsanctioned-change", STILL_DIRTY = "still-dirty", QUARANTINE_AFTER_FIX = "quarantine-after-fix", COLLECTOR_INFRA = "collector-infra", @@ -104,6 +130,7 @@ export const REASON_EXIT_CODE: Record = { [PredicateReason.NO_PRODUCTION_CHANGE]: 10, [PredicateReason.COMPARISON_LEG_ONLY]: 11, [PredicateReason.SUPPRESSION_SUSPECTED]: 12, + [PredicateReason.UNSANCTIONED_CHANGE]: 17, [PredicateReason.STILL_DIRTY]: 13, [PredicateReason.QUARANTINE_AFTER_FIX]: 14, [PredicateReason.COLLECTOR_INFRA]: 15, @@ -124,11 +151,24 @@ export interface PredicateInputs { * FIX #7 — INDEPENDENCE CAVEAT: this signal (and the collector exit code) is * derived from the SAME fixtures the fixer was told to make pass, so it is * NOT independent of a fixture-relaxation cheat — a run that relaxed the SDK - * leg reports clean here too. It is therefore only trustworthy BECAUSE fix #1 - * now blocks ANY gameable-leg edit (see isGameableLeg / the SUPPRESSION_SUSPECTED - * + COMPARISON_LEG_ONLY branches) regardless of production files. That - * always-block rule is load-bearing: it is what makes a clean post-fix signal - * mean "the mock was really fixed" rather than "the leg was relaxed". + * leg reports clean here too. It is therefore only trustworthy BECAUSE the + * ALLOWLIST gate now requires EVERY changed file to be production source or a + * report-named fixture target (see isAllowlisted / the UNSANCTIONED_CHANGE + + * SUPPRESSION_SUSPECTED + COMPARISON_LEG_ONLY branches). Any leg/fixture/config + * edit that could relax the collector's own inputs is NOT allowlisted and + * blocks, regardless of production files. That default-deny rule is + * load-bearing: it is what makes a clean post-fix signal mean "the mock was + * really fixed" rather than "an input the collector reads was relaxed". + * + * FIX #4 (assess) — the criticalCount and the post-fix exit code are both + * scored from the AUTHORITATIVE in-workflow re-collect output + * (drift-report.post-fix.json, written by the "Re-collect drift" step), NOT a + * repo-committed file the fixer could forge: neither the pre-fix report nor the + * post-fix report is committed to the repo (see .github/workflows/fix-drift.yml + * — both are produced by collector invocations in-workflow). The re-collect + * runs AFTER autofix and OVERWRITES anything written to that path during the + * fix, so the predicate scores a freshly-generated report, not attacker + * content. */ postFixCriticalCount: number; } @@ -142,10 +182,44 @@ export interface PredicateResult { offendingFiles: string[]; } +/** Thrown for malformed CLI args / unreadable inputs / bad paths — maps to exit 2. */ +export class PredicateConfigError extends Error {} + // --------------------------------------------------------------------------- // File classification (see spec §2) // --------------------------------------------------------------------------- +/** + * Canonicalize a git-reported path to a stable repo-relative POSIX form BEFORE + * classification, so an equivalent-but-non-identical spelling of a leg cannot + * sneak past the exact-string matchers (round-2 CR F1 / slot-1 / slot-2): + * - strip a leading `./` + * - collapse doubled slashes (`//` → `/`) + * - resolve `.` segments and interior `..` segments + * - FAIL CLOSED (PredicateConfigError → exit 2) on any path that escapes the + * repo root (a leading `..` after resolution) or is absolute — such a path + * was never a legitimate in-repo change and must not be silently reclassified. + */ +export function canonicalizePath(file: string): string { + if (file.startsWith("/")) { + throw new PredicateConfigError(`Refusing to classify an absolute path: ${file}`); + } + const rawSegments = file.split("/"); + const out: string[] = []; + for (const seg of rawSegments) { + if (seg === "" || seg === ".") continue; // drop empty (//, leading ./) and `.` + if (seg === "..") { + if (out.length === 0) { + throw new PredicateConfigError(`Path escapes the repo root (fail-closed): ${file}`); + } + out.pop(); + continue; + } + out.push(seg); + } + return out.join("/"); +} + /** * The triangulation SCHEMA file. Editing it (esp. its ALLOWLISTED_PATHS set) * silences diffs globally — a human-reviewed artifact, never a valid fix. @@ -267,18 +341,44 @@ export function isComparisonLeg(file: string): boolean { export function sanctionedTargets(report: DriftReport): Set { const targets = new Set(); for (const entry of report.entries) { - if (entry.builderFile) targets.add(entry.builderFile); - if (entry.typesFile) targets.add(entry.typesFile); + if (entry.builderFile) targets.add(canonicalizePath(entry.builderFile)); + if (entry.typesFile) targets.add(canonicalizePath(entry.typesFile)); } return targets; } +/** + * ALLOWLIST membership (round-2 CR F1/F2/F3 — the inversion). A changed file is + * SANCTIONED for an auto-fix run when it is either: + * (a) PRODUCTION SOURCE — `src/**` that is NOT under `src/__tests__/` (a mock + * builder / type file, the legitimate fix target); OR + * (b) a fixture file EXPLICITLY NAMED for a drift entry in the report + * (`entry.builderFile` / `entry.typesFile`, e.g. a canary model-registry.ts + * the collector itself sanctioned as the fix target for this run). + * + * EVERYTHING else is NOT allowlisted and blocks: any other `src/__tests__/**` + * file (comparison legs, `*.drift.ts`, `*.test.ts`, providers/schema/sdk-shapes), + * `package.json` / lockfiles / manifests / config, and any unrecognized path. + * `file` MUST already be canonicalized; `sanctioned` is the canonicalized + * sanctioned-target set (see sanctionedTargets). + */ +export function isAllowlisted(file: string, sanctioned: ReadonlySet): boolean { + if (isProductionFile(file)) return true; + if (sanctioned.has(file)) return true; + return false; +} + // --------------------------------------------------------------------------- // The predicate (see spec §3) // --------------------------------------------------------------------------- export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { - const { changedFiles, report, postFixCollectorExit, postFixCriticalCount } = i; + const { report, postFixCollectorExit, postFixCriticalCount } = i; + // Canonicalize every changed path BEFORE classification so a spelling variant + // (`./src/...`, `src//...`, `.`/`..` segments) of a leg cannot slip past the + // exact-string matchers (round-2 CR F1). An absolute path or a repo-root escape + // throws PredicateConfigError from canonicalizePath → CONFIG_ERROR at the CLI. + const changedFiles = i.changedFiles.map(canonicalizePath); // ---- Signal 1: AUTHORITATIVE — collector clean on re-run. ------------- // Checked FIRST: a dirty/quarantine/infra collector makes any file-set moot. @@ -329,6 +429,7 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { } // ---- Classify the changed-file set. ----------------------------------- + const targets = sanctionedTargets(report); const productionFiles = changedFiles.filter(isProductionFile); const gameableLegFiles = changedFiles.filter(isGameableLeg); const suppressionFiles = changedFiles.filter(isSuppressionSurface); @@ -386,44 +487,66 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { }; } - // ---- Signal 2: PRODUCTION change present. ------------------------------ - if (productionFiles.length === 0) { + // ---- ALLOWLIST GATE (round-2 CR F1/F2/F3 — the inversion): EVERY changed file + // must be on the allowlist. The suppression + gameable-leg branches above give + // the KNOWN gameable legs their own LOUD, specific reasons; this gate catches + // EVERYTHING ELSE that is not sanctioned — package.json / lockfiles / manifests + // / config, a drift-dir `*.test.ts`, a non-drift `__tests__` file, an imported + // sub-fixture the report did not name, or any unrecognized path. A denylist of + // "known legs" leaks these; an allowlist does not (default-deny). This is what + // closes the in-diff vectors (F3) and the drift-dir `.test.ts` gap. + const unsanctionedFiles = changedFiles.filter((f) => !isAllowlisted(f, targets)); + if (unsanctionedFiles.length > 0) { + return { + resolved: false, + reason: PredicateReason.UNSANCTIONED_CHANGE, + detail: + "Fix changed files that are NOT on the sanctioned allowlist " + + `(${unsanctionedFiles.join(", ")}) — a real drift fix touches only production mock-builder ` + + "source and the fixture targets the report named. Any other change (deps/config/manifests, " + + "unrelated tests, unnamed fixtures) could game the collector. Fail-closed, needs human review.", + offendingFiles: unsanctionedFiles, + }; + } + + // ---- Signal 2: a REAL fix change is present. -------------------------- + // A legitimate fix touches either production mock-builder source or a fixture + // the report explicitly named (the canary model-list case). An empty diff (or a + // diff of only files that are neither) means nothing shippable was attempted. + const onTargetFiles = changedFiles.filter((f) => targets.has(f)); + if (productionFiles.length === 0 && onTargetFiles.length === 0) { return { resolved: false, reason: PredicateReason.NO_PRODUCTION_CHANGE, detail: - "Fix changed zero PRODUCTION mock-builder files — a clean collector is meaningless without a real mock change. Nothing shippable.", + "Fix changed zero production mock-builder files and no report-named fixture target — a clean collector is meaningless without a real fix. Nothing shippable.", offendingFiles: [], }; } - // ---- Signal 3 (off-target): production change must intersect the report's - // sanctioned target set. A shared helper MAY legitimately be the real fix, so - // this WARNS and blocks but is distinct from a cheat. + // ---- Signal 3 (on-target): the change must land on a file the report named as + // a fix target. A production change to an UNNAMED file (a shared helper) MAY be + // a legitimate fix, so it WARNS and blocks (distinct from a cheat). // - // FIX #3 — an EMPTY sanctioned-target set is fail-closed, not a free pass. The - // old `targets.size > 0` guard disabled the off-target check entirely when the - // report named no targets, accepting ANY production change. An empty target set - // means the report could not name where to fix the drift — route to human - // rather than rubber-stamp an unverifiable production change. - const targets = sanctionedTargets(report); + // FIX #3 — an EMPTY sanctioned-target set is fail-closed, not a free pass. An + // empty target set means the report could not name where to fix the drift — + // route to human rather than rubber-stamp an unverifiable change. if (targets.size === 0) { return { resolved: false, reason: PredicateReason.PRODUCTION_CHANGE_OFF_TARGET, detail: "Drift report named ZERO sanctioned fix targets (no builderFile/typesFile) — cannot verify the " + - `production change (${productionFiles.join(", ")}) landed where the drift lives. Fail-closed, needs human review.`, + `change (${productionFiles.join(", ")}) landed where the drift lives. Fail-closed, needs human review.`, offendingFiles: productionFiles, }; } - const onTarget = productionFiles.some((f) => targets.has(f)); - if (!onTarget) { + if (onTargetFiles.length === 0) { return { resolved: false, reason: PredicateReason.PRODUCTION_CHANGE_OFF_TARGET, detail: - "Production change did not touch any file the drift report named as a fix target " + + "Change did not touch any file the drift report named as a fix target " + `(changed: ${productionFiles.join(", ")}; sanctioned: ${[...targets].join(", ")}). ` + "May be a legitimate shared-helper fix — needs human review.", offendingFiles: productionFiles, @@ -436,7 +559,8 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { reason: PredicateReason.RESOLVED, detail: "Drift genuinely resolved: post-fix collector clean, " + - `real production change (${productionFiles.join(", ")}), no comparison-leg relaxation.`, + `the change landed on a report-named fix target (${onTargetFiles.join(", ")}), and every ` + + "changed file is on the sanctioned allowlist (production source + report-named fixture targets only).", offendingFiles: [], }; } @@ -445,9 +569,6 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { // CLI wrapper // --------------------------------------------------------------------------- -/** Thrown for malformed CLI args / unreadable inputs — maps to exit 2. */ -export class PredicateConfigError extends Error {} - interface CliArgs { reportPath: string; postFixReportPath: string; @@ -481,6 +602,15 @@ export function parseCliArgs(argv: string[]): CliArgs { case "--post-fix-exit": { if (next === undefined) throw new PredicateConfigError("--post-fix-exit requires a numeric argument"); + // FIX #6 — fail CLOSED on an empty/whitespace value. `Number("")` and + // `Number(" ")` are both 0, which Number.isInteger accepts, so a missing + // recollect output (`--post-fix-exit ""`) would masquerade as a clean + // exit 0 and be trusted as authoritative-clean. Reject it explicitly. + if (next.trim() === "") { + throw new PredicateConfigError( + "--post-fix-exit is empty/whitespace — a missing collector exit code must fail closed, not be treated as clean exit 0", + ); + } const parsed = Number(next); if (!Number.isInteger(parsed)) { throw new PredicateConfigError(`--post-fix-exit must be an integer, got "${next}"`); @@ -573,9 +703,16 @@ export function parsePorcelainLine(line: string): string { return path; } -/** Changed files from `git status --porcelain`. */ +/** + * Changed files from `git status --porcelain`. `-c core.quotePath=false` keeps + * non-ASCII paths verbatim (UTF-8) rather than C-quoted/octal-escaped, so a leg + * path with a special character is not mangled into a non-matching spelling + * before classification (round-2 CR slot-1/slot-2 path finding). + */ export function gitChangedFiles(): string[] { - const out = execSync("git status --porcelain", { encoding: "utf-8" }).trimEnd(); + const out = execSync("git -c core.quotePath=false status --porcelain", { + encoding: "utf-8", + }).trimEnd(); return out.split("\n").filter(Boolean).map(parsePorcelainLine); } @@ -646,12 +783,28 @@ export function runCli(argv: string[]): number { return REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]; } - const verdict = evaluateDriftResolved({ - changedFiles, - report, - postFixCollectorExit: args.postFixExit, - postFixCriticalCount: countCriticalDiffs(postFixReport), - }); + // FIX #8 — a post-fix report that structurally passes readReport (has a + // string timestamp + an entries array) can still be malformed at the entry + // level (e.g. an entry missing its `diffs` array), which would make + // countCriticalDiffs throw a bare TypeError. evaluateDriftResolved can also + // throw PredicateConfigError from canonicalizePath (a repo-root-escaping or + // absolute changed-file path). Catch both here and map to a NAMED CONFIG_ERROR + // so the human gets a named cause instead of an uncaught stacktrace with an + // empty reason= line. + let verdict: PredicateResult; + try { + verdict = evaluateDriftResolved({ + changedFiles, + report, + postFixCollectorExit: args.postFixExit, + postFixCriticalCount: countCriticalDiffs(postFixReport), + }); + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`CONFIG_ERROR: unable to score the drift reports: ${msg}`); + console.log(`reason=${PredicateReason.CONFIG_ERROR}`); + return REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]; + } if (verdict.resolved) { console.log(verdict.detail); diff --git a/scripts/fix-drift.ts b/scripts/fix-drift.ts index 3502c27a..3762113a 100644 --- a/scripts/fix-drift.ts +++ b/scripts/fix-drift.ts @@ -678,12 +678,23 @@ function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { console.log(`reason=${PredicateReason.COLLECTOR_INFRA}`); process.exit(REASON_EXIT_CODE[PredicateReason.COLLECTOR_INFRA]); } - const verdict = evaluateDriftResolved({ - changedFiles, - report, - postFixCollectorExit: postFix.exitCode, - postFixCriticalCount: countCriticalDiffs(postFix.report), - }); + let verdict; + try { + verdict = evaluateDriftResolved({ + changedFiles, + report, + postFixCollectorExit: postFix.exitCode, + postFixCriticalCount: countCriticalDiffs(postFix.report), + }); + } catch (err: unknown) { + // A malformed report or a repo-escaping changed-file path throws from the + // predicate — fail-closed to a NAMED config-error rather than an uncaught + // stacktrace, so the workflow's Slack alert names the cause (mirrors runCli). + const msg = err instanceof Error ? err.message : String(err); + console.error(`ERROR: unable to score the drift reports: ${msg}`); + console.log(`reason=${PredicateReason.CONFIG_ERROR}`); + process.exit(REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]); + } if (!verdict.resolved) { console.error(`ERROR: Drift NOT resolved [${verdict.reason}]: ${verdict.detail}`); if (verdict.offendingFiles.length > 0) { diff --git a/src/__tests__/drift-success-predicate.test.ts b/src/__tests__/drift-success-predicate.test.ts index a1863723..84b1d111 100644 --- a/src/__tests__/drift-success-predicate.test.ts +++ b/src/__tests__/drift-success-predicate.test.ts @@ -27,6 +27,8 @@ import { isComparisonLeg, isSuppressionSurface, isGameableLeg, + canonicalizePath, + isAllowlisted, sanctionedTargets, countCriticalDiffs, parseCliArgs, @@ -347,16 +349,18 @@ describe("evaluateDriftResolved — GREEN (real fix) cases", () => { expect(REASON_EXIT_CODE[verdict.reason]).toBe(0); }); - it("legit canary: model-registry.ts + ws-realtime.ts (report sanctions ws-realtime.ts) → RESOLVED", () => { + it("legit canary: model-registry.ts + ws-realtime.ts (report sanctions BOTH) → RESOLVED", () => { // The known-models canary routes its fix to the production ws-realtime.ts - // (builderFile), while the model list lives in the model-registry fixture. + // (builderFile) AND the model list fixture (typesFile). Under the allowlist a + // fixture is accepted ONLY when the report names it as a target — so the + // report here sanctions model-registry.ts via typesFile. const canary = report([ entry({ provider: "OpenAI Realtime", scenario: "known-models canary", builderFile: "src/ws-realtime.ts", builderFunctions: ["buildRealtimeSession"], - typesFile: null, + typesFile: "src/__tests__/drift/model-registry.ts", diffs: [ diff({ path: "knownModels", issue: "Unknown realtime model detected", mock: "" }), ], @@ -394,12 +398,18 @@ describe("evaluateDriftResolved — GREEN (real fix) cases", () => { expect(verdict.reason).toBe(PredicateReason.RESOLVED); }); - it("production change + accompanying legit canary fixture (not a comparison leg) → RESOLVED", () => { - // model-family.ts is a legit fixture target, not a gameable comparison leg, - // so accompanying a real production change it does not trip the cheat guard. + it("production change + accompanying report-NAMED canary fixture → RESOLVED", () => { + // model-family.ts is accepted as an accompanying change ONLY because the + // report names it (typesFile) — under the allowlist a fixture is not a free + // pass by static membership; it must be sanctioned by the report for this run. const verdict = evaluateDriftResolved({ changedFiles: ["src/ws-realtime.ts", "src/__tests__/drift/model-family.ts"], - report: report([entry({ builderFile: "src/ws-realtime.ts", typesFile: null })]), + report: report([ + entry({ + builderFile: "src/ws-realtime.ts", + typesFile: "src/__tests__/drift/model-family.ts", + }), + ]), postFixCollectorExit: 0, postFixCriticalCount: 0, }); @@ -408,6 +418,238 @@ describe("evaluateDriftResolved — GREEN (real fix) cases", () => { }); }); +// --------------------------------------------------------------------------- +// ALLOWLIST INVERSION (round-2 CR F1/F2/F3) — a fix is RESOLVED only when EVERY +// changed file is on the allowlist. Anything not recognized as production source +// or a report-sanctioned fixture target BLOCKS. This closes path-spelling +// sneak-ins, stale-denylist gaps, and in-diff vectors (package.json / lockfiles +// / sub-fixtures / unknown paths). +// --------------------------------------------------------------------------- + +describe("allowlist inversion — non-allowlisted changed files ALWAYS block", () => { + // A report that sanctions src/helpers.ts as the fix target, so the production + // edit itself is legitimately allowlisted; the SECOND file is the attack. + const sanctioned = () => + report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]); + + const cleanSignal = { postFixCollectorExit: 0, postFixCriticalCount: 0 }; + + it("package.json changed alongside a real production fix → UNSANCTIONED_CHANGE (block)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "package.json"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + expect(verdict.offendingFiles).toContain("package.json"); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(17); + }); + + it("pnpm-lock.yaml changed alongside a real production fix → UNSANCTIONED_CHANGE (block)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "pnpm-lock.yaml"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + expect(verdict.offendingFiles).toContain("pnpm-lock.yaml"); + }); + + it("tsconfig.json changed alongside a real production fix → UNSANCTIONED_CHANGE (block)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "tsconfig.json"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + }); + + it("an unknown/unrecognized path alongside a real production fix → UNSANCTIONED_CHANGE (block)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "scripts/fix-drift.ts"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + expect(verdict.offendingFiles).toContain("scripts/fix-drift.ts"); + }); + + it("a drift-dir *.test.ts (NOT a *.drift.ts) alongside a real production fix → UNSANCTIONED_CHANGE (closes the drift-dir .test.ts gap)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "src/__tests__/drift/model-registry.test.ts"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(false); + // A drift-dir unit test is not a gameable comparison leg (isGameableLeg is + // false for it) and is not allowlisted → UNSANCTIONED_CHANGE. + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + expect(verdict.offendingFiles).toContain("src/__tests__/drift/model-registry.test.ts"); + }); + + it("a non-drift __tests__ file alongside a real production fix → UNSANCTIONED_CHANGE (block)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "src/__tests__/server.test.ts"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + }); + + it("model-registry.ts fixture NOT named by the report + prod fix → UNSANCTIONED_CHANGE (fixtures allowlisted ONLY when report-named)", () => { + // model-registry.ts is a legit fixture target but the report does NOT name it + // (builderFile/typesFile are src/helpers.ts / src/types.ts). It is therefore + // NOT on the allowlist for THIS run and must block. + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "src/__tests__/drift/model-registry.ts"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + expect(verdict.offendingFiles).toContain("src/__tests__/drift/model-registry.ts"); + }); + + it("a report-NAMED fixture target (builderFile) + clean collector → RESOLVED", () => { + // The collector sanctions the fixture as the fix target (canary routing). + const canary = report([ + entry({ + provider: "OpenAI Realtime", + scenario: "known-models canary", + builderFile: "src/__tests__/drift/model-registry.ts", + builderFunctions: ["knownModels"], + typesFile: null, + diffs: [diff({ path: "knownModels", issue: "new model shipped" })], + }), + ]); + const verdict = evaluateDriftResolved({ + changedFiles: ["src/__tests__/drift/model-registry.ts"], + report: canary, + ...cleanSignal, + }); + expect(verdict.resolved).toBe(true); + expect(verdict.reason).toBe(PredicateReason.RESOLVED); + }); + + it("production-source-only fix (no fixtures at all) + clean collector → RESOLVED", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(true); + expect(verdict.reason).toBe(PredicateReason.RESOLVED); + }); +}); + +// --------------------------------------------------------------------------- +// PATH CANONICALIZATION (round-2 CR slot-1/slot-2 F1) — a leg edit presented +// under an equivalent-but-non-identical spelling must still be recognized and +// blocked; classification runs on the canonical form. +// --------------------------------------------------------------------------- + +describe("path canonicalization defeats spelling-variant leg sneak-ins", () => { + it("./src/... leading-dot spelling of the SDK leg still blocks", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["./src/__tests__/drift/sdk-shapes.ts"], + report: report(), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.COMPARISON_LEG_ONLY); + }); + + it("doubled-slash spelling of the SDK leg + prod edit still blocks (SUPPRESSION_SUSPECTED)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src//__tests__/drift/sdk-shapes.ts", "src/helpers.ts"], + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.SUPPRESSION_SUSPECTED); + }); + + it("trailing-dot-segment spelling of a production target canonicalizes and RESOLVES", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["./src/helpers.ts"], + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(true); + expect(verdict.reason).toBe(PredicateReason.RESOLVED); + }); + + it("canonicalizePath normalizes ./ , // and . segments", () => { + expect(canonicalizePath("./src/helpers.ts")).toBe("src/helpers.ts"); + expect(canonicalizePath("src//__tests__/drift/sdk-shapes.ts")).toBe( + "src/__tests__/drift/sdk-shapes.ts", + ); + expect(canonicalizePath("src/./helpers.ts")).toBe("src/helpers.ts"); + expect(canonicalizePath("src/helpers.ts")).toBe("src/helpers.ts"); + }); + + it("canonicalizePath rejects a path escaping the repo root (fail-closed)", () => { + expect(() => canonicalizePath("../ag-ui/events.ts")).toThrow(PredicateConfigError); + expect(() => canonicalizePath("src/../../etc/passwd")).toThrow(PredicateConfigError); + }); +}); + +// --------------------------------------------------------------------------- +// F6 — empty / non-integer --post-fix-exit must FAIL CLOSED (Number("")===0 +// must NOT be treated as a clean exit 0). +// --------------------------------------------------------------------------- + +describe("F6 — --post-fix-exit fails closed on empty/whitespace", () => { + it("throws on an empty --post-fix-exit (Number('')===0 must not slip through)", () => { + expect(() => + parseCliArgs(["--report", "a.json", "--post-fix-report", "b.json", "--post-fix-exit", ""]), + ).toThrow(PredicateConfigError); + }); + + it("throws on a whitespace-only --post-fix-exit", () => { + expect(() => + parseCliArgs(["--report", "a.json", "--post-fix-report", "b.json", "--post-fix-exit", " "]), + ).toThrow(PredicateConfigError); + }); +}); + +// --------------------------------------------------------------------------- +// F8 — a malformed post-fix report that crashes countCriticalDiffs must be +// caught and mapped to a NAMED config-error reason, not a bare stacktrace. +// --------------------------------------------------------------------------- + +describe("F8 — malformed post-fix report → named config-error (not a bare crash)", () => { + let dir: string | null = null; + afterEach(() => { + if (dir) rmSync(dir, { recursive: true, force: true }); + dir = null; + }); + + it("runCli exits 2 (CONFIG_ERROR) when the post-fix report has entries with no diffs array", () => { + dir = mkdtempSync(join(tmpdir(), "ws2-f8-")); + const preP = join(dir, "drift-report.json"); + const postP = join(dir, "drift-report.post-fix.json"); + writeFileSync(preP, JSON.stringify(report()), "utf-8"); + // Structurally passes readReport (timestamp + entries array) but each entry + // is missing `diffs`, so countCriticalDiffs would throw. + writeFileSync( + postP, + JSON.stringify({ timestamp: "t", entries: [{ provider: "OpenAI" }] }), + "utf-8", + ); + const code = runCli(["--report", preP, "--post-fix-report", postP, "--post-fix-exit", "0"]); + expect(code).toBe(REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]); + }); +}); + // --------------------------------------------------------------------------- // File-classification unit coverage // --------------------------------------------------------------------------- @@ -464,6 +706,24 @@ describe("file classification", () => { expect(isGameableLeg("src/helpers.ts")).toBe(false); }); + it("isAllowlisted: production source is allowed; config/manifests/tests are not", () => { + const targets = new Set(["src/helpers.ts", "src/__tests__/drift/model-registry.ts"]); + // Production source (non-test) is always allowlisted. + expect(isAllowlisted("src/helpers.ts", targets)).toBe(true); + expect(isAllowlisted("src/agui-types.ts", targets)).toBe(true); + // A report-named fixture target is allowlisted. + expect(isAllowlisted("src/__tests__/drift/model-registry.ts", targets)).toBe(true); + // A fixture NOT named by the report is not allowlisted. + expect(isAllowlisted("src/__tests__/drift/model-family.ts", targets)).toBe(false); + // Config/manifests/lockfiles/unknown paths are not allowlisted. + expect(isAllowlisted("package.json", targets)).toBe(false); + expect(isAllowlisted("pnpm-lock.yaml", targets)).toBe(false); + expect(isAllowlisted("tsconfig.json", targets)).toBe(false); + expect(isAllowlisted("scripts/fix-drift.ts", targets)).toBe(false); + // A non-production src config-ish manifest is not allowlisted. + expect(isAllowlisted("src/__tests__/server.test.ts", targets)).toBe(false); + }); + it("sanctionedTargets unions builderFile + non-null typesFile", () => { const t = sanctionedTargets( report([ @@ -733,18 +993,45 @@ describe("revert guard — old always-accept predicate would FAIL these locks", expect(verdict.resolved).toBe(false); expect(verdict.reason).toBe(PredicateReason.SUPPRESSION_SUSPECTED); }); + + // ALLOWLIST-INVERSION revert lock: the OLD denylist only blocked KNOWN gameable + // legs, so an in-diff vector NOT on the denylist (package.json, a lockfile, an + // unknown path) paired with an on-target production edit sailed through to + // RESOLVED. The allowlist inverts that: anything not explicitly allowed blocks. + // Reverting evaluateDriftResolved to a denylist model flips these to RESOLVED + // and breaks the suite. + function denylistWouldAccept(changedFiles: string[]): boolean { + // Old denylist model: block ONLY if a changed file is a known gameable leg; + // otherwise (production + package.json/lockfile/unknown) accept. + return !changedFiles.some(isGameableLeg) && changedFiles.some(isProductionFile); + } + + it("in-diff vector (package.json + prod): old DENYLIST accepts, allowlist BLOCKS (UNSANCTIONED_CHANGE)", () => { + const vector = ["src/helpers.ts", "package.json"]; + // The old denylist would have accepted (no known leg in the set). + expect(denylistWouldAccept(vector)).toBe(true); + const verdict = evaluateDriftResolved({ + changedFiles: vector, + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + }); }); -describe("RED-count lock", () => { - // A structural lock: exactly this many distinct block reasons must reject. - // NOTE: the current RED-case count in the top describe block is 9 (NOT 10) — - // HEADLINE, schema+builder, *.drift.ts, no-change, still-dirty(exit2), - // criticalCount>0, quarantine, infra, off-target. The WS-2b / dual-class / - // empty-targets / exit5+critical / exit1+critical additions raise the total. +describe("exit-code distinctness lock", () => { + // Structural lock on the reason→exit-code contract: every block reason maps to + // a distinct NON-ZERO exit code and RESOLVED alone is 0. This does NOT count + // RED test cases (it asserts the code table, not the number of scenarios) — it + // guarantees the workflow can route each cause to its own Slack DETAIL without + // two reasons colliding on one exit code. const BLOCK_REASONS = [ PredicateReason.NO_PRODUCTION_CHANGE, PredicateReason.COMPARISON_LEG_ONLY, PredicateReason.SUPPRESSION_SUSPECTED, + PredicateReason.UNSANCTIONED_CHANGE, PredicateReason.STILL_DIRTY, PredicateReason.QUARANTINE_AFTER_FIX, PredicateReason.COLLECTOR_INFRA, From b5135552d645680d2f280444b9909876bbfddcbc Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Fri, 17 Jul 2026 12:14:55 -0700 Subject: [PATCH 4/6] fix(drift): pin pre-fix report, gate createPr staging, align readReport validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR round-3 report-integrity + workflow-wiring hardening for the drift-success allowlist predicate. The allowlist LOGIC was already sound; this closes the last layer — the report the sanctioned set is derived from, the files createPr actually commits, and the entry-level validation of that report. F-A — pin the PRE-fix report so the autofix LLM cannot forge the allowlist. The sanctioned-target set is derived from drift-report.json, which the autofix Claude Code subprocess has Write/Edit on (its cwd is the repo checkout). A new "Pin pre-fix drift report" workflow step copies it to $RUNNER_TEMP (outside the LLM-writable scope) BEFORE the autofix runs; the Assert and Create PR steps now pass --report from that pinned copy, never the in-repo file. sanctionedTargets is a pure function of the passed report, so a forged on-disk file cannot widen the allowlist. F-C / F2 — createPr now derives its changed-file set from the predicate's own gitChangedFiles() (git -c core.quotePath=false) + canonicalizePath, so staging matches the verdict byte-for-byte. A new pure gatedCommitFiles() partitions the set into the ONLY groups permitted to stage (production source + report-named fixture targets); the "remaining"/straggler catch-all git add is removed, so no unclassified file can land in the PR after the verdict. The version bump + CHANGELOG remain an explicit, exact-path-staged part of the set. F3 — the predicate's readReport now validates per-entry builderFile/typesFile/ diffs, mirroring fix-drift.ts:readDriftReport, so a malformed report fails closed with a DISTINCT named CONFIG_ERROR instead of a bare TypeError caught as an unnamed error. F1 — locked the already-wired post-fix re-collect + args and corrected the now-stale comments (no legacy no-post-fix fallback remains) in fix-drift.ts. Tests: red-green proven by stashing the source and confirming the new F-A/F-C/ F3 locks fail on baseline, then pass after the fix. Added a workflow-shape test (fix-drift-workflow.test.ts) asserting the pin step ordering and that both gate invocations read --report from the pinned copy; end-to-end canonicalization (../ and absolute) and unrecognized-collector-exit-code locks. Full suite, format, lint, tsc, actionlint, build all green. --- .github/workflows/fix-drift.yml | 42 ++++- scripts/drift-success-predicate.ts | 30 +++ scripts/fix-drift.ts | 110 ++++++++--- src/__tests__/drift-success-predicate.test.ts | 173 ++++++++++++++++++ src/__tests__/fix-drift-workflow.test.ts | 94 ++++++++++ src/__tests__/fix-drift.test.ts | 91 +++++++++ 6 files changed, 511 insertions(+), 29 deletions(-) create mode 100644 src/__tests__/fix-drift-workflow.test.ts diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index 26328da6..cfae5b0b 100644 --- a/.github/workflows/fix-drift.yml +++ b/.github/workflows/fix-drift.yml @@ -121,6 +121,33 @@ jobs: echo "No critical drift detected (exit code: $DETECT_EXIT_CODE) — skipping fix" fi + # Step 2b: PIN the PRE-fix report OUTSIDE the repo before the autofix LLM + # runs (INTEGRITY GUARANTEE — CR round-3 F-A). + # + # The drift-success predicate derives its ALLOWLIST's sanctioned-target set + # (union of every entry's builderFile/typesFile) from the PRE-fix report. The + # autofix step below grants the Claude Code subprocess unrestricted Write/Edit + # scoped to the repo checkout (its cwd), so a repo-committed `drift-report.json` + # is LLM-writable: a malicious/confused fix could overwrite it to name an + # arbitrary fixture as a builderFile and thereby expand the allowlist. + # + # Pinning the report to $RUNNER_TEMP — which is OUTSIDE the repo checkout and + # therefore outside the autofix tool's writable scope — makes the sanctioned + # set immutable across the autofix. Both the "Assert" and "Create PR" steps + # read `--report` from this pinned copy, NEVER the in-repo file the LLM can + # touch. The POST-fix report is separately re-generated after autofix (Step + # 4c) and so needs no pin. Runs AFTER "Check for critical diffs" (so it is + # gated by the same skip guard) and BEFORE "Auto-fix drift" (so the pin is + # captured before the LLM can touch the on-disk report). + - name: Pin pre-fix drift report (integrity) + if: steps.check.outputs.skip != 'true' + env: + PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json + run: | + set -euo pipefail + cp drift-report.json "$PINNED_REPORT" + echo "Pinned pre-fix drift report to $PINNED_REPORT (outside the LLM-writable repo checkout)" + # Step 3: Invoke Claude Code to fix - name: Auto-fix drift id: autofix @@ -205,9 +232,14 @@ jobs: # Run the predicate to a log file so we capture its REAL exit code # directly (a `VAR="$(pipeline)"` assignment would report the # assignment's own status via PIPESTATUS, never the predicate's). + # + # --report reads the PINNED pre-fix report (Step 1b), NOT the in-repo + # drift-report.json the autofix LLM could have overwritten — the + # allowlist's sanctioned-target set is derived from it, so it must be + # the untamperable copy (CR round-3 F-A). set +e npx tsx scripts/drift-success-predicate.ts \ - --report drift-report.json \ + --report "${PINNED_REPORT}" \ --post-fix-report drift-report.post-fix.json \ --post-fix-exit "${POST_FIX_EXIT}" 2>&1 | tee predicate.log PRED_EXIT=${PIPESTATUS[0]} @@ -222,6 +254,7 @@ jobs: echo "Drift-success predicate: drift truly resolved" env: POST_FIX_EXIT: ${{ steps.recollect.outputs.post_fix_exit }} + PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json # Inject git credentials only when needed for push (persist-credentials: false above) - name: Configure git for push @@ -241,6 +274,7 @@ jobs: env: GH_TOKEN: ${{ steps.app-token.outputs.token }} POST_FIX_EXIT: ${{ steps.recollect.outputs.post_fix_exit }} + PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json run: | set -euo pipefail # Defense-in-depth: the workflow already asserted the predicate in the @@ -248,7 +282,13 @@ jobs: # (via --post-fix-report/--post-fix-exit) as an in-script gate BEFORE # any git add/commit, so a cheat opens no PR even if the step guard is # ever bypassed. + # + # --report reads the PINNED pre-fix report (Step 1b), NOT the in-repo + # drift-report.json — same integrity guarantee as the Assert step: the + # in-script predicate's sanctioned-target set must come from the copy + # the autofix LLM could not overwrite (CR round-3 F-A). npx tsx scripts/fix-drift.ts --create-pr \ + --report "${PINNED_REPORT}" \ --post-fix-report drift-report.post-fix.json \ --post-fix-exit "${POST_FIX_EXIT}" HEAD_SHA="$(git rev-parse HEAD)" diff --git a/scripts/drift-success-predicate.ts b/scripts/drift-success-predicate.ts index 739e9490..bcf1a73d 100644 --- a/scripts/drift-success-predicate.ts +++ b/scripts/drift-success-predicate.ts @@ -677,6 +677,36 @@ export function readReport(path: string): DriftReport { `Drift report at ${path} is missing a string "timestamp" — corrupt/truncated report, failing closed`, ); } + + // F3 — ENTRY-LEVEL validation, aligned with fix-drift.ts:readDriftReport. The + // predicate reads `entry.builderFile` / `entry.typesFile` (sanctionedTargets) + // and `entry.diffs` (countCriticalDiffs); a structurally-valid report whose + // entries are malformed at those fields would otherwise throw a bare TypeError + // deep inside classification, caught only by the outer runCli try/catch and + // surfaced as an UNNAMED config-error. Validate here so every malformed shape + // fails-closed with a DISTINCT, named PredicateConfigError → CONFIG_ERROR. + const entries = (parsed as { entries: unknown[] }).entries; + for (let i = 0; i < entries.length; i++) { + const entry = entries[i] as Record | null | undefined; + if (!entry || typeof entry !== "object") { + throw new PredicateConfigError(`Drift report at ${path} entry[${i}] is not an object`); + } + if (typeof entry.builderFile !== "string" || entry.builderFile === "") { + throw new PredicateConfigError( + `Drift report at ${path} entry[${i}] has a missing/empty "builderFile" — cannot derive the sanctioned target set, failing closed`, + ); + } + if (entry.typesFile !== null && typeof entry.typesFile !== "string") { + throw new PredicateConfigError( + `Drift report at ${path} entry[${i}] "typesFile" must be a string or null, failing closed`, + ); + } + if (!Array.isArray(entry.diffs)) { + throw new PredicateConfigError( + `Drift report at ${path} entry[${i}] is missing a "diffs" array — cannot score criticalCount, failing closed`, + ); + } + } return parsed as DriftReport; } diff --git a/scripts/fix-drift.ts b/scripts/fix-drift.ts index 3762113a..35a037f1 100644 --- a/scripts/fix-drift.ts +++ b/scripts/fix-drift.ts @@ -29,11 +29,17 @@ * 12 — SUPPRESSION_SUSPECTED (allowlist / *.drift.ts assertion edited) * 13 — STILL_DIRTY (post-fix collector still reports critical drift) * 14 — QUARANTINE_AFTER_FIX (post-fix collector returned quarantine) - * 15 — COLLECTOR_INFRA (post-fix collector infra failure) + * 15 — COLLECTOR_INFRA (post-fix collector infra failure, OR the + * MANDATORY post-fix args were not supplied) * 16 — PRODUCTION_CHANGE_OFF_TARGET (production change not in report's target set) + * 17 — UNSANCTIONED_CHANGE (a changed file is not on the allowlist) * The legacy exit 4 (no source files changed) is subsumed by 10/11. The - * predicate runs ONLY when the workflow supplies --post-fix-report and - * --post-fix-exit; without them createPr falls back to the legacy exit-4 guard. + * drift-success predicate is MANDATORY in --create-pr mode: BOTH + * --post-fix-report and --post-fix-exit are required (there is no legacy + * no-post-fix fallback — a missing pair fails closed to COLLECTOR_INFRA rather + * than opening a PR). The real workflow also supplies --report pointing at the + * PINNED pre-fix report (see .github/workflows/fix-drift.yml) so the + * sanctioned-target set cannot be forged by the autofix LLM. */ import { spawn, execSync, execFileSync } from "node:child_process"; @@ -45,6 +51,10 @@ import { evaluateDriftResolved, readReport as readPostFixReport, countCriticalDiffs, + canonicalizePath, + gitChangedFiles, + isProductionFile, + sanctionedTargets, REASON_EXIT_CODE, PredicateReason, } from "./drift-success-predicate.js"; @@ -639,36 +649,72 @@ export interface PostFixCollectorResult { exitCode: number; } +/** + * The GATED commit groups for a RESOLVED verdict (CR round-3 F-C / F2). Given + * the canonicalized changed-file set and the report's sanctioned-target set, + * partition into the ONLY groups createPr is permitted to stage: + * - `builderFiles` — production mock-builder source (always allowlisted). + * - `testFiles` — ONLY report-named fixture targets under src/__tests__/ + * (any other test file would have BLOCKED at the gate, so + * it must never be staged). + * - `skillFiles` — skills/ files (NOT allowlisted by the predicate; a + * RESOLVED verdict implies this is empty — kept for + * defensive completeness). + * `stragglers` is every canonicalized changed file that is NOT in one of those + * gated groups. On a RESOLVED verdict it MUST be empty (the predicate allowlist + * already rejected any unclassified file); createPr never `git add`s it. The + * version bump + CHANGELOG are added separately by exact path and are not part + * of this changed-file partition. + */ +export function gatedCommitFiles( + changedFiles: string[], + sanctioned: ReadonlySet, +): { + builderFiles: string[]; + testFiles: string[]; + skillFiles: string[]; + stragglers: string[]; +} { + const builderFiles = changedFiles.filter(isProductionFile); + const testFiles = changedFiles.filter((f) => f.startsWith("src/__tests__/") && sanctioned.has(f)); + const skillFiles = changedFiles.filter((f) => f.startsWith("skills/")); + const gated = new Set([...builderFiles, ...testFiles, ...skillFiles]); + const stragglers = changedFiles.filter((f) => !gated.has(f)); + return { builderFiles, testFiles, skillFiles, stragglers }; +} + function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { const stamp = todayStamp(); // Detect uncommitted changes (staged + unstaged) BEFORE any git write ops so // the drift-success predicate can gate PR-open ahead of branch/commit/push. - const changedFiles = getChangedFiles(); - - const builderFiles = changedFiles.filter( - (f) => f.startsWith("src/") && !f.startsWith("src/__tests__/"), - ); - const testFiles = changedFiles.filter((f) => f.startsWith("src/__tests__/")); - const skillFiles = changedFiles.filter((f) => f.startsWith("skills/")); + // + // Use the predicate's OWN `gitChangedFiles()` (which runs + // `git -c core.quotePath=false status --porcelain`) and canonicalize every + // path with the predicate's `canonicalizePath` (CR round-3 F-C). This keeps + // classification/staging BYTE-FOR-BYTE identical to what the predicate scored: + // both callers now read the same git invocation and the same canonical spelling, + // so a non-ASCII/C-quoted path or a `./`-prefixed spelling cannot be classified + // one way by the verdict and staged another way here. + const changedFiles = gitChangedFiles().map(canonicalizePath); // PR-OPEN GATE (WS-2). When the workflow supplies the post-fix collector // result, the drift-success predicate is the authoritative decision: it // rejects fixture-relaxation cheats (a diff that changed ONLY comparison-leg // files, or silenced the detector) and drifts that were not actually // resolved. This runs BEFORE any git add/commit — a blocked verdict opens no - // PR (and therefore never reaches auto-merge). Without --post-fix-* args, - // fall back to the legacy "no source files changed" guard (exit 4). - // FIX #5 — the drift-success predicate is MANDATORY. The old legacy fallback - // (accept a PR when `builderFiles.length || testFiles.length`) re-opened the - // original fixture-only cheat: a run that changed only comparison-leg test - // files satisfied `testFiles.length > 0` and sailed through to a PR. There is - // no safe "no post-fix result" path — without the authoritative post-fix + // PR (and therefore never reaches auto-merge). + // FIX #5 — the drift-success predicate is MANDATORY; there is NO legacy + // no-post-fix fallback. The old fallback (accept a PR when + // `builderFiles.length || testFiles.length`) re-opened the original + // fixture-only cheat: a run that changed only comparison-leg test files + // satisfied `testFiles.length > 0` and sailed through to a PR. There is no + // safe "no post-fix result" path — without the authoritative post-fix // collector signal we cannot tell a real fix from a relaxation, so we // fail-closed to human review (COLLECTOR_INFRA) rather than open a PR. The - // real workflow (fix-drift.yml "Create PR" step) always supplies - // --post-fix-report/--post-fix-exit, so this only fires on a misconfigured - // invocation — which must NOT auto-merge. + // real workflow (fix-drift.yml "Create PR" step) ALWAYS supplies --report + // (the PINNED pre-fix report), --post-fix-report, and --post-fix-exit, so this + // only fires on a misconfigured invocation — which must NOT auto-merge. if (!postFix) { console.error( "ERROR: PR-open gate requires the post-fix collector result " + @@ -724,6 +770,17 @@ function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { console.log(`Created branch ${branchName}`); } + // Stage ONLY the GATED set (CR round-3 F-C / F2). The predicate already + // verified that EVERY changed file is on the sanctioned allowlist — production + // mock-builder source OR a fixture the report explicitly named. We stage + // exactly that allowlisted set here (grouped by commit purpose) and NEVER a + // catch-all `git add` of "whatever is still dirty": a straggler-add re-widened + // the PR past the verdict (any unclassified file the predicate did not judge — + // an unnamed fixture, a config file — would have BLOCKED at the gate, so + // sweeping it in AFTER the pass silently defeats the allowlist). + const sanctioned = sanctionedTargets(report); + const { builderFiles, testFiles, skillFiles } = gatedCommitFiles(changedFiles, sanctioned); + if (builderFiles.length > 0) { execFileSafe("git", ["add", ...builderFiles]); execFileSafe("git", ["commit", "-m", "fix: auto-remediate API drift in builder functions"]); @@ -743,6 +800,10 @@ function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { ]); } + // The version bump + CHANGELOG are an EXPLICIT, gated part of the fix set — a + // release always accompanies an auto-remediation — not an unclassified + // straggler. They are workflow-authored (never LLM-authored) and staged by an + // exact path list, so they do not re-open the allowlist. try { const newVersion = patchBumpVersion(); console.log(`Bumped version to ${newVersion}`); @@ -750,7 +811,7 @@ function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { addChangelogEntry(report, newVersion); console.log("Added CHANGELOG.md entry"); - // Always commit version bump + changelog + // Commit version bump + changelog by EXACT path (not a catch-all). execFileSafe("git", ["add", "package.json", "CHANGELOG.md"]); execFileSafe("git", ["commit", "-m", `chore: bump version to ${newVersion}`, "--allow-empty"]); } catch (err) { @@ -758,13 +819,6 @@ function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { // Continue with PR creation without version bump } - // Catch any remaining files - const remaining = getChangedFiles(); - if (remaining.length > 0) { - execFileSafe("git", ["add", ...remaining]); - execFileSafe("git", ["commit", "-m", "fix: remaining drift remediation changes"]); - } - execFileSafe("git", ["push", "-u", "origin", branchName]); console.log(`Pushed branch ${branchName}`); diff --git a/src/__tests__/drift-success-predicate.test.ts b/src/__tests__/drift-success-predicate.test.ts index 84b1d111..d327fd89 100644 --- a/src/__tests__/drift-success-predicate.test.ts +++ b/src/__tests__/drift-success-predicate.test.ts @@ -1046,3 +1046,176 @@ describe("exit-code distinctness lock", () => { expect(new Set(codes).size).toBe(codes.length); // all distinct }); }); + +// --------------------------------------------------------------------------- +// CR round-3 slot-3 LOW gaps — unrecognized collector exit code + `..`/absolute +// canonicalization variants driven end-to-end through the predicate/runCli. +// --------------------------------------------------------------------------- + +describe("unrecognized collector exit code fails closed → COLLECTOR_INFRA", () => { + // The `postFixCollectorExit !== 0` catch-all (COLLECTOR_INFRA / exit 15) had + // no locking test. Tests only fed 0/1/2/5. A future refactor could let an + // unknown exit fall through to a clean accept — this pins it closed. + for (const exit of [3, 7, 99]) { + it(`exit ${exit} (unrecognized) with a production change → COLLECTOR_INFRA / exit 15`, () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: exit, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.COLLECTOR_INFRA); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(15); + }); + } +}); + +describe("`..`-segment and absolute path variants block/fail-closed THROUGH the predicate", () => { + it("a `..`-containing spelling of the SDK leg canonicalizes and BLOCKS (COMPARISON_LEG_ONLY)", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/__tests__/drift/foo/../sdk-shapes.ts"], + report: report(), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.COMPARISON_LEG_ONLY); + }); + + it("an absolute changed-file path throws PredicateConfigError from evaluateDriftResolved", () => { + expect(() => + evaluateDriftResolved({ + changedFiles: ["/etc/passwd"], + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }), + ).toThrow(PredicateConfigError); + }); + + it("a `..`-escaping changed-file path throws PredicateConfigError from evaluateDriftResolved", () => { + expect(() => + evaluateDriftResolved({ + changedFiles: ["src/../../etc/passwd"], + report: report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]), + postFixCollectorExit: 0, + postFixCriticalCount: 0, + }), + ).toThrow(PredicateConfigError); + }); +}); + +describe("runCli maps a repo-escaping/absolute changed-file to CONFIG_ERROR (exit 2)", () => { + let dir: string | null = null; + afterEach(() => { + if (dir) rmSync(dir, { recursive: true, force: true }); + dir = null; + }); + + // Drive an absolute/`..`-escaping path all the way through runCli. A supplied + // --changed-file cross-checks against git first, so an absolute path that git + // never reports would be rejected by the cross-check (still exit 2). To pin the + // fix #8 canonicalize-throw path specifically, we assert the CONFIG_ERROR exit. + it("runCli exits 2 (CONFIG_ERROR) when a --changed-file is an absolute path", () => { + dir = mkdtempSync(join(tmpdir(), "ws2-canon-")); + const preP = join(dir, "drift-report.json"); + const postP = join(dir, "drift-report.post-fix.json"); + writeFileSync(preP, JSON.stringify(report()), "utf-8"); + writeFileSync(postP, JSON.stringify(report([])), "utf-8"); + const code = runCli([ + "--report", + preP, + "--post-fix-report", + postP, + "--post-fix-exit", + "0", + "--changed-file", + "/etc/passwd", + ]); + expect(code).toBe(REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]); + }); +}); + +// --------------------------------------------------------------------------- +// CR round-3 F3 — readReport ENTRY-LEVEL validation aligned with +// fix-drift.ts:readDriftReport. A structurally-valid report whose entries are +// malformed at the fields the predicate reads must fail-closed with a DISTINCT, +// NAMED PredicateConfigError (not a bare TypeError caught as an unnamed error). +// --------------------------------------------------------------------------- + +describe("readReport entry-level validation (F3)", () => { + let dir: string | null = null; + afterEach(() => { + if (dir) rmSync(dir, { recursive: true, force: true }); + dir = null; + }); + + function writeAndRead(obj: unknown): void { + dir = mkdtempSync(join(tmpdir(), "ws2-f3-")); + const p = join(dir, "r.json"); + writeFileSync(p, JSON.stringify(obj), "utf-8"); + readReport(p); + } + + it("throws a named PredicateConfigError when an entry is missing its diffs array", () => { + expect(() => + writeAndRead({ + timestamp: "t", + entries: [{ provider: "OpenAI", builderFile: "src/helpers.ts", typesFile: null }], + }), + ).toThrow(PredicateConfigError); + }); + + it('throws a named PredicateConfigError when "diffs" is present via the message', () => { + expect(() => + writeAndRead({ + timestamp: "t", + entries: [{ provider: "OpenAI", builderFile: "src/helpers.ts", typesFile: null }], + }), + ).toThrow(/diffs/); + }); + + it("throws when an entry has a non-string builderFile (cannot derive sanctioned set)", () => { + expect(() => + writeAndRead({ + timestamp: "t", + entries: [{ provider: "OpenAI", builderFile: 123, typesFile: null, diffs: [] }], + }), + ).toThrow(PredicateConfigError); + }); + + it("throws when an entry has an empty builderFile", () => { + expect(() => + writeAndRead({ + timestamp: "t", + entries: [{ provider: "OpenAI", builderFile: "", typesFile: null, diffs: [] }], + }), + ).toThrow(/builderFile/); + }); + + it("throws when an entry has a numeric typesFile (must be string or null)", () => { + expect(() => + writeAndRead({ + timestamp: "t", + entries: [{ provider: "OpenAI", builderFile: "src/helpers.ts", typesFile: 42, diffs: [] }], + }), + ).toThrow(/typesFile/); + }); + + it("throws when an entry is not an object", () => { + expect(() => writeAndRead({ timestamp: "t", entries: [null] })).toThrow(PredicateConfigError); + }); + + it("still ACCEPTS a well-formed report (empty entries + valid entries both OK)", () => { + expect(() => writeAndRead({ timestamp: "t", entries: [] })).not.toThrow(); + expect(() => + writeAndRead({ + timestamp: "t", + entries: [ + { provider: "OpenAI", builderFile: "src/helpers.ts", typesFile: null, diffs: [] }, + ], + }), + ).not.toThrow(); + }); +}); diff --git a/src/__tests__/fix-drift-workflow.test.ts b/src/__tests__/fix-drift-workflow.test.ts new file mode 100644 index 00000000..49bb5e58 --- /dev/null +++ b/src/__tests__/fix-drift-workflow.test.ts @@ -0,0 +1,94 @@ +/** + * Static (text-level) assertions on .github/workflows/fix-drift.yml. + * + * These pin the LOAD-BEARING wiring that the drift-success predicate/guard now + * REQUIRE (CR round-3): + * + * F1 — the workflow must (a) re-collect drift AUTHORITATIVELY after the autofix + * to a distinct post-fix path, capturing its exit code, and (b) pass BOTH + * --post-fix-report and --post-fix-exit into the `--create-pr` invocation. + * Without these, the mandatory-post-fix guard fails closed and NO PR is + * ever opened (the gate would be inert). + * + * F-A — the PRE-fix report the allowlist's sanctioned-target set is derived + * from must be PINNED outside the LLM-writable repo checkout BEFORE the + * autofix runs, and both the Assert and Create-PR steps must read + * --report from that pinned copy — NEVER the in-repo drift-report.json + * the autofix LLM could overwrite. + * + * No YAML dependency is added; the repo ships none. These are deliberately + * text-shape assertions on the committed workflow — an actionlint run in CI + * covers structural validity separately. + */ +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; + +import { describe, it, expect } from "vitest"; + +const WORKFLOW_PATH = resolve(__dirname, "../../.github/workflows/fix-drift.yml"); +const wf = readFileSync(WORKFLOW_PATH, "utf-8"); + +/** Collapse runs of whitespace so multi-line YAML `run:` blocks match linearly. */ +const wfFlat = wf.replace(/\s+/g, " "); + +describe("fix-drift.yml — F1: post-fix re-collect + args wired into --create-pr", () => { + it("has an authoritative post-fix re-collect step writing a DISTINCT report path", () => { + expect(wf).toContain("Re-collect drift (authoritative)"); + expect(wf).toContain( + "npx tsx scripts/drift-report-collector.ts --out drift-report.post-fix.json", + ); + }); + + it("captures the post-fix collector exit code as a step output", () => { + expect(wfFlat).toContain('POST_FIX_EXIT=$? set -e echo "post_fix_exit=$POST_FIX_EXIT"'); + }); + + it("passes BOTH --post-fix-report and --post-fix-exit into `fix-drift.ts --create-pr`", () => { + expect(wfFlat).toContain("npx tsx scripts/fix-drift.ts --create-pr"); + expect(wfFlat).toMatch( + /fix-drift\.ts --create-pr[^]*?--post-fix-report drift-report\.post-fix\.json[^]*?--post-fix-exit "\$\{POST_FIX_EXIT\}"/, + ); + }); + + it("the Assert step runs the predicate with post-fix args (the happy-path gate)", () => { + expect(wf).toContain("Assert drift truly resolved"); + expect(wfFlat).toMatch( + /drift-success-predicate\.ts[^]*?--post-fix-report drift-report\.post-fix\.json[^]*?--post-fix-exit "\$\{POST_FIX_EXIT\}"/, + ); + }); +}); + +describe("fix-drift.yml — F-A: PRE-fix report pinned outside the LLM-writable checkout", () => { + it("has a pin step that copies the pre-fix report into runner.temp before autofix", () => { + expect(wf).toContain("Pin pre-fix drift report (integrity)"); + expect(wf).toContain('cp drift-report.json "$PINNED_REPORT"'); + expect(wf).toContain("PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json"); + }); + + it("the pin step runs BEFORE the Auto-fix step (so the LLM cannot pre-tamper the pin)", () => { + const pinIdx = wf.indexOf("Pin pre-fix drift report"); + const autofixIdx = wf.indexOf("name: Auto-fix drift"); + expect(pinIdx).toBeGreaterThan(-1); + expect(autofixIdx).toBeGreaterThan(-1); + expect(pinIdx).toBeLessThan(autofixIdx); + }); + + it("the Assert step reads --report from the PINNED copy, not the in-repo file", () => { + // The YAML line-continuation `\` survives whitespace-flattening, so match + // tolerantly across it. + expect(wfFlat).toMatch(/drift-success-predicate\.ts \\? *--report "\$\{PINNED_REPORT\}"/); + }); + + it("the Create PR step reads --report from the PINNED copy, not the in-repo file", () => { + expect(wfFlat).toMatch( + /scripts\/fix-drift\.ts --create-pr \\? *--report "\$\{PINNED_REPORT\}"/, + ); + }); + + it("neither the Assert nor Create-PR predicate invocation reads --report drift-report.json (the LLM-writable file)", () => { + // The in-repo drift-report.json is still uploaded as an artifact + copied by + // the pin step, but must NEVER be the --report source for the gate. + expect(wfFlat).not.toMatch(/drift-success-predicate\.ts \\? *--report drift-report\.json/); + expect(wfFlat).not.toMatch(/fix-drift\.ts --create-pr \\? *--report drift-report\.json/); + }); +}); diff --git a/src/__tests__/fix-drift.test.ts b/src/__tests__/fix-drift.test.ts index 7d122a25..0bac377a 100644 --- a/src/__tests__/fix-drift.test.ts +++ b/src/__tests__/fix-drift.test.ts @@ -41,12 +41,14 @@ import { parseMode, hasPostFixArgs, getChangedFiles, + gatedCommitFiles, affectedSkillSections, BUILDER_TO_SKILL_SECTION, truncateBody, GH_BODY_MAX, GH_BODY_SAFE_MAX, } from "../../scripts/fix-drift.js"; +import { sanctionedTargets } from "../../scripts/drift-success-predicate.js"; import { readFileSync, writeFileSync, existsSync } from "node:fs"; import { execFileSync, execSync } from "node:child_process"; @@ -846,6 +848,95 @@ describe("getChangedFiles", () => { // affectedSkillSections // --------------------------------------------------------------------------- +// --------------------------------------------------------------------------- +// CR round-3 F-C / F2 — gatedCommitFiles: createPr stages ONLY the allowlisted +// set (production source + report-named fixture targets), NEVER a straggler +// catch-all. A file the predicate would have blocked (config/manifest/unnamed +// fixture) must land in `stragglers`, which createPr never `git add`s. +// --------------------------------------------------------------------------- + +describe("gatedCommitFiles (F-C: no straggler catch-all)", () => { + const sanctioned = new Set([ + "src/helpers.ts", + "src/__tests__/drift/model-registry.ts", + "src/types.ts", + ]); + + it("groups production source into builderFiles", () => { + const g = gatedCommitFiles(["src/helpers.ts", "src/types.ts"], sanctioned); + expect(g.builderFiles).toEqual(["src/helpers.ts", "src/types.ts"]); + expect(g.stragglers).toEqual([]); + }); + + it("stages a report-named fixture target as a testFile", () => { + const g = gatedCommitFiles(["src/__tests__/drift/model-registry.ts"], sanctioned); + expect(g.testFiles).toEqual(["src/__tests__/drift/model-registry.ts"]); + expect(g.stragglers).toEqual([]); + }); + + it("EXCLUDES an UNSANCTIONED src/__tests__ file — it lands in stragglers, never testFiles", () => { + const g = gatedCommitFiles(["src/__tests__/drift/sdk-shapes.ts"], sanctioned); + expect(g.testFiles).toEqual([]); + expect(g.stragglers).toEqual(["src/__tests__/drift/sdk-shapes.ts"]); + }); + + it("puts package.json / lockfiles / config in stragglers (never staged)", () => { + const g = gatedCommitFiles( + ["src/helpers.ts", "package.json", "pnpm-lock.yaml", "tsconfig.json"], + sanctioned, + ); + expect(g.builderFiles).toEqual(["src/helpers.ts"]); + expect(g.stragglers).toEqual(["package.json", "pnpm-lock.yaml", "tsconfig.json"]); + }); + + it("stragglers is empty for a clean allowlisted set (the RESOLVED-verdict invariant)", () => { + const g = gatedCommitFiles( + ["src/helpers.ts", "src/__tests__/drift/model-registry.ts"], + sanctioned, + ); + expect(g.stragglers).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// CR round-3 F-A — the sanctioned/allowlist set is derived SOLELY from the +// (pinned) report object passed to createPr, NOT from any on-disk file. The +// workflow pins the pre-fix report OUTSIDE the LLM-writable repo checkout and +// passes THAT copy via --report; here we prove that a DIFFERENT on-disk report +// cannot expand the allowlist, because the set is a pure function of the passed +// report — the file the LLM could overwrite has no bearing on the sanctioned set. +// --------------------------------------------------------------------------- + +describe("F-A: sanctioned set comes from the passed (pinned) report, not on-disk", () => { + it("sanctionedTargets is a pure function of the report — a forged on-disk file cannot widen it", () => { + // The PINNED report names only the production builder as a target. + const pinned = makeReport({ + entries: [makeEntry({ builderFile: "src/helpers.ts", typesFile: null })], + }); + // A FORGED report (what an autofix LLM might write to drift-report.json in the + // repo) tries to sanction the SDK-shape fixture as a target. + const forged = makeReport({ + entries: [makeEntry({ builderFile: "src/__tests__/drift/sdk-shapes.ts", typesFile: null })], + }); + + const pinnedSet = sanctionedTargets(pinned); + const forgedSet = sanctionedTargets(forged); + + // The sanctioned set is derived purely from the object passed in. + expect(pinnedSet.has("src/helpers.ts")).toBe(true); + expect(pinnedSet.has("src/__tests__/drift/sdk-shapes.ts")).toBe(false); + // The forged set (only relevant if createPr were fed the LLM-writable file) + // would sanction the fixture — which is EXACTLY why the workflow must pin the + // pre-fix report and pass THAT copy, never the in-repo drift-report.json. + expect(forgedSet.has("src/__tests__/drift/sdk-shapes.ts")).toBe(true); + // The two sets are disjoint on the fixture: pinning the report is what keeps + // the fixture OUT of the allowlist. + expect(pinnedSet.has("src/__tests__/drift/sdk-shapes.ts")).not.toBe( + forgedSet.has("src/__tests__/drift/sdk-shapes.ts"), + ); + }); +}); + describe("affectedSkillSections", () => { it("returns empty array when no builder files are present", () => { expect(affectedSkillSections(["src/__tests__/foo.test.ts", "package.json"])).toEqual([]); From fef8476d1cb34803168e421fc982ec1108166405 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Fri, 17 Jul 2026 12:42:03 -0700 Subject: [PATCH 5/6] fix(drift): human-approval backstop for drift PRs + close predicate holes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drift-success predicate becomes a strong AUTO-FILTER, not a provable merge gate: the in-workflow re-collect is not independent of the fix (its SDK/expected leg reads the fixtures the autofix touched — WS-2b), so a clean post-fix signal can filter out cheats but cannot prove the mock was genuinely fixed. Drift-fix PRs therefore require ONE HUMAN APPROVAL to merge — no unattended auto-merge on the drift path. - Remove the in-workflow auto-merge step and every `gh pr merge`; the drift path now opens a PR (verdict recorded in the PR body) and STOPS. A human reviews CI + the diff + the verdict and merges (branch ruleset 1-approval is the gate). Slack success copy says "PR opened — needs human review + merge", never "merged to main"; failure Slack drops the removed merge-step reasons. - F3 (happy-path break): the collector outputs (pre-fix, re-collect, predicate log) were written into the repo cwd, so they showed as untracked and the predicate scored them UNSANCTIONED — always fail-closed. Write all three to $RUNNER_TEMP (outside the checkout) and broaden .gitignore to drift-report*.json as belt-and-suspenders. - F2 (canary-only bypass): a fixture-target-only change (no production change) reached resolved:true despite the ">=1 production change" invariant. Require >=1 production mock-builder change for RESOLVED; fixture-only diffs route to NO_PRODUCTION_CHANGE (needs-human). - F7 (fail-open): fix-drift.ts PR path accepted Number("")===0 as a clean exit 0. Add parsePostFixExit, mirroring the predicate CLI's non-empty integer guard; empty/invalid post-fix exit fails closed. - F5: createPr asserts the stragglers set is empty on a RESOLVED verdict and fail-closes (UNSANCTIONED_CHANGE) rather than silently dropping an unclassified file from the diff. - Test-tightness: lock the runCli CONFIG_ERROR path and the machine- readable reason= stdout line the workflow greps for Slack routing. - Document accepted residuals (F6 TOCTOU, WS-2b independent SDK leg) as mitigated by the human-approval backstop. --- .github/workflows/fix-drift.yml | 257 ++++++------------ .gitignore | 5 +- scripts/drift-success-predicate.ts | 26 +- scripts/fix-drift.ts | 78 +++++- src/__tests__/drift-success-predicate.test.ts | 187 ++++++++++++- src/__tests__/fix-drift-straggler.test.ts | 122 +++++++++ src/__tests__/fix-drift-workflow.test.ts | 50 +++- src/__tests__/fix-drift.test.ts | 126 ++++++++- 8 files changed, 644 insertions(+), 207 deletions(-) create mode 100644 src/__tests__/fix-drift-straggler.test.ts diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index cfae5b0b..4a955573 100644 --- a/.github/workflows/fix-drift.yml +++ b/.github/workflows/fix-drift.yml @@ -62,16 +62,25 @@ jobs: git config user.email "drift-bot@copilotkit.ai" git checkout -B "fix/drift-$(date +%Y-%m-%d)-${RUN_ID}" - # Step 1: Detect drift and produce report + # Step 1: Detect drift and produce report. + # + # FIX #F3 (round-4) — write the report OUTSIDE the repo checkout, to + # $RUNNER_TEMP. A collector output left in the repo cwd shows up as an + # untracked file in `git status --porcelain`, which the drift-success + # predicate scores as an UNSANCTIONED_CHANGE (fail-closed) and would break + # the happy path. $RUNNER_TEMP is outside the checkout, so neither the + # predicate's changed-file scan nor createPr's staging ever sees it. (The + # .gitignore also covers `drift-report*.json` as belt-and-suspenders.) - name: Collect drift report id: detect env: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} GOOGLE_API_KEY: ${{ secrets.GOOGLE_API_KEY }} + PRE_FIX_REPORT: ${{ runner.temp }}/drift-report.json run: | set +e - npx tsx scripts/drift-report-collector.ts + npx tsx scripts/drift-report-collector.ts --out "$PRE_FIX_REPORT" EXIT_CODE=$? set -e echo "exit_code=$EXIT_CODE" >> "$GITHUB_OUTPUT" @@ -89,13 +98,13 @@ jobs: exit $EXIT_CODE fi - # Always upload the report as an artifact + # Always upload the report as an artifact (from $RUNNER_TEMP — see Step 1). - name: Upload drift report if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: drift-report - path: drift-report.json + path: ${{ runner.temp }}/drift-report.json if-no-files-found: warn retention-days: 30 @@ -142,10 +151,11 @@ jobs: - name: Pin pre-fix drift report (integrity) if: steps.check.outputs.skip != 'true' env: + PRE_FIX_REPORT: ${{ runner.temp }}/drift-report.json PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json run: | set -euo pipefail - cp drift-report.json "$PINNED_REPORT" + cp "$PRE_FIX_REPORT" "$PINNED_REPORT" echo "Pinned pre-fix drift report to $PINNED_REPORT (outside the LLM-writable repo checkout)" # Step 3: Invoke Claude Code to fix @@ -196,6 +206,13 @@ jobs: # predicate scores. A cheat that relaxed the SDK-shape fixture still # reports clean HERE (its own SDK leg reads the relaxed fixture), which is # exactly why the predicate ALSO requires a real production change. + # + # FIX #F3 (round-4) — write the post-fix report to $RUNNER_TEMP, OUTSIDE + # the repo checkout, for the same reason as Step 1: a post-fix report left + # in the repo cwd is an untracked file that the predicate's changed-file + # scan would score as UNSANCTIONED_CHANGE, breaking the happy path. Writing + # it outside the checkout (and out of the autofix LLM's writable scope) + # keeps it out of `git status --porcelain` entirely. - name: Re-collect drift (authoritative) id: recollect if: steps.autofix.outcome == 'success' && steps.check.outputs.skip != 'true' @@ -203,9 +220,10 @@ jobs: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} GOOGLE_API_KEY: ${{ secrets.GOOGLE_API_KEY }} + POST_FIX_REPORT: ${{ runner.temp }}/drift-report.post-fix.json run: | set +e - npx tsx scripts/drift-report-collector.ts --out drift-report.post-fix.json + npx tsx scripts/drift-report-collector.ts --out "$POST_FIX_REPORT" POST_FIX_EXIT=$? set -e echo "post_fix_exit=$POST_FIX_EXIT" >> "$GITHUB_OUTPUT" @@ -216,7 +234,7 @@ jobs: uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: drift-report-post-fix - path: drift-report.post-fix.json + path: ${{ runner.temp }}/drift-report.post-fix.json if-no-files-found: warn retention-days: 30 @@ -237,14 +255,19 @@ jobs: # drift-report.json the autofix LLM could have overwritten — the # allowlist's sanctioned-target set is derived from it, so it must be # the untamperable copy (CR round-3 F-A). + # Write the predicate log OUTSIDE the repo checkout ($RUNNER_TEMP): + # the predicate scans `git status --porcelain` in this same working + # directory, so a predicate.log written into cwd would appear as an + # untracked file and be scored as UNSANCTIONED_CHANGE, breaking the + # happy path (FIX #F3). set +e npx tsx scripts/drift-success-predicate.ts \ --report "${PINNED_REPORT}" \ - --post-fix-report drift-report.post-fix.json \ - --post-fix-exit "${POST_FIX_EXIT}" 2>&1 | tee predicate.log + --post-fix-report "${POST_FIX_REPORT}" \ + --post-fix-exit "${POST_FIX_EXIT}" 2>&1 | tee "${PREDICATE_LOG}" PRED_EXIT=${PIPESTATUS[0]} set -e - REASON="$(grep '^reason=' predicate.log | tail -n1)" + REASON="$(grep '^reason=' "${PREDICATE_LOG}" | tail -n1)" REASON="${REASON#reason=}" echo "reason=${REASON}" >> "$GITHUB_OUTPUT" if [ "$PRED_EXIT" -ne 0 ]; then @@ -254,7 +277,9 @@ jobs: echo "Drift-success predicate: drift truly resolved" env: POST_FIX_EXIT: ${{ steps.recollect.outputs.post_fix_exit }} + POST_FIX_REPORT: ${{ runner.temp }}/drift-report.post-fix.json PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json + PREDICATE_LOG: ${{ runner.temp }}/predicate.log # Inject git credentials only when needed for push (persist-credentials: false above) - name: Configure git for push @@ -274,6 +299,7 @@ jobs: env: GH_TOKEN: ${{ steps.app-token.outputs.token }} POST_FIX_EXIT: ${{ steps.recollect.outputs.post_fix_exit }} + POST_FIX_REPORT: ${{ runner.temp }}/drift-report.post-fix.json PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json run: | set -euo pipefail @@ -281,7 +307,9 @@ jobs: # "Assert drift truly resolved" step, but fix-drift.ts runs it AGAIN # (via --post-fix-report/--post-fix-exit) as an in-script gate BEFORE # any git add/commit, so a cheat opens no PR even if the step guard is - # ever bypassed. + # ever bypassed. This opens the PR and STOPS — a human reviews and + # merges (see the "NO AUTO-MERGE" comment below); fix-drift.ts records + # the predicate verdict in the PR body for that reviewer. # # --report reads the PINNED pre-fix report (Step 1b), NOT the in-repo # drift-report.json — same integrity guarantee as the Assert step: the @@ -289,7 +317,7 @@ jobs: # the autofix LLM could not overwrite (CR round-3 F-A). npx tsx scripts/fix-drift.ts --create-pr \ --report "${PINNED_REPORT}" \ - --post-fix-report drift-report.post-fix.json \ + --post-fix-report "${POST_FIX_REPORT}" \ --post-fix-exit "${POST_FIX_EXIT}" HEAD_SHA="$(git rev-parse HEAD)" echo "Pushed head SHA: $HEAD_SHA" @@ -319,149 +347,30 @@ jobs: echo "head_sha=$HEAD_SHA" } >> "$GITHUB_OUTPUT" - # Step 5.5: Auto-merge the drift fix PR behind a REAL green gate. + # NO AUTO-MERGE FOR THE DRIFT PATH (WS-2, round-4 — user-approved design). # - # A ruleset bypass actor bypasses the entire ruleset, so the green gate - # must be enforced HERE, not in branch protection. The old gate counted - # `gh pr checks | wc -l` rows (a "no checks" message is a row) and trusted - # `--watch --fail-fast`, which exits 0 on empty/pending/skipped/neutral — - # a false-green that could merge an unverified PR. We now: - # 1. poll on machine-readable JSON length for check REGISTRATION, - # 2. `--watch` (with a hard timeout) to let checks run to completion, - # 3. re-query JSON and hand it to scripts/ci-merge-gate.sh, which exits - # 0 ONLY when pass>=1, nothing pending/fail/cancel, and every - # required context is present and passing. - - name: Auto-merge PR - id: merge - if: success() && steps.pr.outputs.url != '' - env: - GH_TOKEN: ${{ steps.app-token.outputs.token }} - PR_URL: ${{ steps.pr.outputs.url }} - HEAD_SHA: ${{ steps.pr.outputs.head_sha }} - run: | - set -euo pipefail - - # Phase 1: poll on JSON length until at least one check is registered - # (guards against a registration race letting an empty set through). - registered=0 - for i in $(seq 1 10); do - # Distinguish a `gh` API error from a genuine zero-check response: - # capture stdout and the exit status separately. `|| echo 0` would - # collapse a transient API failure into a spurious "no checks" and - # could let the poll exhaust on an error rather than a real empty set. - if count="$(gh pr checks "$PR_URL" --json name --jq 'length' 2>/dev/null)"; then - if [ "${count:-0}" -gt 0 ]; then - echo "CI checks registered ($count checks), proceeding to watch" - registered=1 - break - fi - echo "No checks registered yet (attempt $i/10), waiting 30s..." - else - echo "::warning::gh pr checks query failed (attempt $i/10) — retrying, not treating as zero checks" - fi - sleep 30 - done - if [ "$registered" -eq 0 ]; then - echo "::error::No CI checks ever registered — not merging" - echo "reason=no-checks" >> "$GITHUB_OUTPUT" - exit 1 - fi - - # Phase 2: let checks run to completion under a hard timeout so we - # never block the job indefinitely. `--watch` exit status is advisory - # only — the authoritative decision is the gate in phase 3. - if timeout 1200 gh pr checks "$PR_URL" --watch --interval 30; then - echo "watch completed" - else - watch_rc=$? - if [ "$watch_rc" -eq 124 ]; then - echo "::error::CI checks timed out after 20m — not merging" - echo "reason=timeout" >> "$GITHUB_OUTPUT" - exit 1 - fi - echo "watch exited non-zero ($watch_rc) — deferring to gate assertion" - fi - - # Phase 3: authoritative green-gate assertion on machine-readable JSON. - # - # `gh pr checks --json` exits NON-ZERO whenever any check is failing or - # pending — even in --json mode — so under `set -euo pipefail` the job - # would abort HERE and never reach the gate script (reason would never - # be set to not-green). Append `|| true` so the JSON capture is - # advisory and the gate script is the sole authority on the decision. - # The JSON the gate scores MUST be tied to the exact SHA we pushed. - # If HEAD_SHA is empty the anti-TOCTOU guarantee is void — HARD-FAIL - # here rather than re-querying the live head (which would re-open the - # TOCTOU window the pin exists to close). Distinct reason so the Slack - # alert names the real cause. - if [ -z "${HEAD_SHA:-}" ]; then - echo "::error::Merge gate: HEAD_SHA is empty — cannot pin the gate snapshot to the pushed commit, refusing to merge" - echo "reason=head-sha-empty" >> "$GITHUB_OUTPUT" - exit 1 - fi - - # Score the checks for the pinned SHA. gh does not filter --json by - # SHA, so we verify the live head still equals HEAD_SHA before scoring; - # if it moved, the snapshot would be stale — bail with a distinct - # reason rather than score checks that belong to a different commit. - LIVE_SHA="$(gh pr view "$PR_URL" --json headRefOid --jq '.headRefOid' 2>/dev/null || true)" - if [ -z "$LIVE_SHA" ]; then - echo "::error::Merge gate: could not read the PR's live head SHA to confirm the gate snapshot — not merging" - echo "reason=head-requery-error" >> "$GITHUB_OUTPUT" - exit 1 - fi - if [ "$LIVE_SHA" != "$HEAD_SHA" ]; then - echo "::error::Merge gate: head moved ($HEAD_SHA -> $LIVE_SHA) before scoring — snapshot is stale, not merging" - echo "reason=head-moved" >> "$GITHUB_OUTPUT" - exit 1 - fi - - gh pr checks "$PR_URL" --json name,state,bucket > /tmp/pr-checks.json || true - if [ ! -s /tmp/pr-checks.json ]; then - echo "::error::Merge gate: could not read checks JSON — not merging" - echo "reason=not-green" >> "$GITHUB_OUTPUT" - exit 1 - fi - - # Run the gate. Exit 2 is a distinct CONFIG/parse error (malformed - # input, empty/contradictory required set, jq failure) — it is NOT the - # same as "PR is not green" (exit 1) and must be labeled distinctly in - # Slack so a human triages a broken gate, not a red PR. - set +e - ./scripts/ci-merge-gate.sh /tmp/pr-checks.json - gate_rc=$? - set -e - if [ "$gate_rc" -eq 2 ]; then - echo "::error::Merge gate: config/parse error (exit 2) — the gate could not score the checks, not merging" - echo "reason=gate-config-error" >> "$GITHUB_OUTPUT" - exit 1 - fi - if [ "$gate_rc" -ne 0 ]; then - echo "::error::Merge gate refused: PR is not truly green — not merging" - echo "reason=not-green" >> "$GITHUB_OUTPUT" - exit 1 - fi - - # Re-verify the head SHA at merge time matches the SHA the gate - # snapshot was taken against, closing the TOCTOU window where a new - # push lands between the gate check and the merge. `--match-head-commit` - # makes `gh pr merge` refuse if the head moved. HEAD_SHA is guaranteed - # non-empty (asserted above), so there is no live-head fallback. - # - # If the merge command itself fails (head moved and --match-head-commit - # refused, a merge conflict, a transient API error, etc.) set a - # SPECIFIC reason so the Slack alert names "merge command failed" - # rather than falling back to a generic/blank detail. - set +e - gh pr merge "$PR_URL" --merge --match-head-commit "$HEAD_SHA" - merge_rc=$? - set -e - if [ "$merge_rc" -ne 0 ]; then - echo "::error::Merge gate: 'gh pr merge --match-head-commit' failed (rc=$merge_rc) — head may have moved after the gate scored it, or the merge was rejected" - echo "reason=merge-command-failed" >> "$GITHUB_OUTPUT" - exit 1 - fi - echo "merged=true" >> "$GITHUB_OUTPUT" + # The drift-success predicate is a strong AUTO-FILTER, NOT a provable merge + # gate. Its authoritative "is the drift gone?" signal comes from the in- + # workflow RE-COLLECT (Step 4c), which is NOT independent of the fix: the + # collector's SDK/expected leg reads the very fixtures the autofix touched, + # so a run that relaxed an input leg reports clean here too. The allowlist + + # always-block-on-leg-edit rules make that clean signal trustworthy enough + # to FILTER OUT cheats and never-fixed drift, but they cannot PROVE the mock + # was genuinely fixed (that requires an independent SDK leg — WS-2b, out of + # scope). Given that fundamental residual, the drift path opens a PR and + # STOPS: a HUMAN reviews CI + the committed diff + the predicate verdict + # (recorded in the PR body) and merges. The merge gate is the human plus the + # branch ruleset's one-approval requirement — NOT an unattended in-workflow + # merge command. The predicate having already filtered the PR down to + # genuine-looking fixes means the human review is a confirmation step, not a + # from-scratch triage. + # + # Accepted residuals under this backstop (documented, not fixed): + # • F6 — working-tree-vs-committed TOCTOU: the predicate scores the working + # tree; the committed diff is a gated subset. Mitigated by human review + # of the COMMITTED diff and CI running on the pushed SHA. + # • WS-2b — no independent SDK leg: the re-collect is not independent of the + # fix. Mitigated by the human confirming the diff is a real mock change. # Alert: the Step-1 drift collector crashed (exit code other than # 0/2/5). This is a DISTINCT cause from an auto-fix step failure — the @@ -540,15 +449,15 @@ jobs: -H "Content-Type: application/json" \ -d "$PAYLOAD" - # Slack notification on success. Distinguishes "merged to main" (the merge - # command returned 0) from "PR open, not yet merged" so the message never - # overstates what happened. + # Slack notification on success. The drift path NEVER auto-merges (see the + # "NO AUTO-MERGE" comment above): a passing predicate opens a PR that a + # human must review and merge. The message says exactly that — it never + # claims "merged to main". - name: Notify Slack on fix success if: success() && steps.pr.outputs.url != '' env: SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} PR_URL: ${{ steps.pr.outputs.url }} - MERGED: ${{ steps.merge.outputs.merged }} REPO: ${{ github.repository }} RUN_ID: ${{ github.run_id }} run: | @@ -557,45 +466,33 @@ jobs: echo "::error::SLACK_WEBHOOK not set — cannot send fix-success alert" exit 0 fi - if [ "${MERGED:-}" = "true" ]; then - MSG="✅ *Drift auto-fix merged to main*\nPR: ${PR_URL}\nRun: https://github.com/${REPO}/actions/runs/${RUN_ID}" - else - MSG="✅ *Drift auto-fix PR open* (not yet merged — gate did not complete a merge)\nPR: ${PR_URL}\nRun: https://github.com/${REPO}/actions/runs/${RUN_ID}" - fi + MSG="✅ *Drift-fix PR opened — needs human review + merge*\nThe drift-success predicate passed (verdict in the PR body); a human must review CI + the diff and merge.\nPR: ${PR_URL}\nRun: https://github.com/${REPO}/actions/runs/${RUN_ID}" PAYLOAD="$(jq -n --arg text "$MSG" '{text: $text}')" curl -fsS --show-error -X POST "$SLACK_WEBHOOK" \ -H "Content-Type: application/json" \ -d "$PAYLOAD" # Slack notification on failure. A missing SLACK_WEBHOOK is a VISIBLE - # ::error:: annotation (not a silent exit 0). Distinguishes the merge-gate - # timeout / not-green reasons. + # ::error:: annotation (not a silent exit 0). The drift path has no merge + # step, so the failure reasons are the PR-step reason and the drift-success + # predicate's reason (the "Assert drift truly resolved" step). - name: Notify Slack on fix failure if: failure() && steps.check.outputs.skip != 'true' && steps.autofix.outcome == 'success' env: SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} REPO: ${{ github.repository }} RUN_ID: ${{ github.run_id }} - MERGE_REASON: ${{ steps.merge.outputs.reason }} PR_REASON: ${{ steps.pr.outputs.reason }} ASSERT_REASON: ${{ steps.assert.outputs.reason }} run: | set -euo pipefail - # The merge-step reason takes precedence, then the PR-step reason, then - # the drift-success predicate's reason (the "Assert drift truly - # resolved" step) so a blocked cheat is named rather than reported - # blank. The predicate reasons below (WS-6 needs-human tie-in) name the - # attempted-cheat / not-actually-fixed cause explicitly. - REASON="${MERGE_REASON:-${PR_REASON:-${ASSERT_REASON:-}}}" + # The PR-step reason takes precedence, then the drift-success + # predicate's reason (the "Assert drift truly resolved" step) so a + # blocked cheat is named rather than reported blank. The predicate + # reasons below (WS-6 needs-human tie-in) name the attempted-cheat / + # not-actually-fixed cause explicitly. + REASON="${PR_REASON:-${ASSERT_REASON:-}}" case "$REASON" in - timeout) DETAIL=" (CI checks timed out before completing)";; - not-green) DETAIL=" (merge gate refused — PR not truly green)";; - gate-config-error) DETAIL=" (merge gate CONFIG/parse error — the gate could not score checks; not a red PR, the gate itself needs attention)";; - head-sha-empty) DETAIL=" (merge gate could not pin the snapshot — pushed HEAD SHA was empty)";; - head-requery-error) DETAIL=" (merge gate could not read the PR live head SHA to confirm the snapshot)";; - head-moved) DETAIL=" (PR head moved before the gate scored it — stale snapshot, refused)";; - merge-command-failed) DETAIL=" (the merge command failed — head moved after the gate scored it, or the merge was rejected)";; - no-checks) DETAIL=" (no CI checks ever registered)";; no-pr-match) DETAIL=" (no open PR matched the pushed head SHA)";; # Drift-success predicate reasons — NEEDS HUMAN. The LOUD cheat # reasons (comparison-leg-only / suppression-suspected) mean the diff --git a/.gitignore b/.gitignore index a590bb57..0e8cbce0 100644 --- a/.gitignore +++ b/.gitignore @@ -16,5 +16,8 @@ docs/plans/ docs/specs/ # fix-drift.ts workflow artifacts (regenerated each run; uploaded via fix-drift.yml) +# The workflow writes these to $RUNNER_TEMP (outside the checkout) so the +# drift-success predicate's `git status` scan never sees them; this glob is +# belt-and-suspenders for local runs and covers the post-fix / pinned variants. claude-code-output.log -drift-report.json +drift-report*.json diff --git a/scripts/drift-success-predicate.ts b/scripts/drift-success-predicate.ts index bcf1a73d..d6ec958c 100644 --- a/scripts/drift-success-predicate.ts +++ b/scripts/drift-success-predicate.ts @@ -509,17 +509,31 @@ export function evaluateDriftResolved(i: PredicateInputs): PredicateResult { }; } - // ---- Signal 2: a REAL fix change is present. -------------------------- - // A legitimate fix touches either production mock-builder source or a fixture - // the report explicitly named (the canary model-list case). An empty diff (or a - // diff of only files that are neither) means nothing shippable was attempted. + // ---- Signal 2: at least one PRODUCTION mock-builder change is present. -- + // The module invariant (Signal 2 in the header) is that a genuine drift fix + // ALWAYS changes at least one production mock-builder file (`src/**` excluding + // `src/__tests__/`) — that is the only place the mock output is produced. A run + // that changed ZERO production files cannot be a real fix, even if it edited a + // report-named fixture target (the canary model-registry.ts case): a + // fixture-target-only change (e.g. adding a model id to the model-list fixture + // with NO production/builder change) is NOT independently verifiable — the + // re-collect's clean signal is derived from the same fixture the change touched, + // so a clean post-fix report there means only "the fixture agrees with itself", + // not "the mock was fixed". Fix #F2 (round-4): require >=1 production change for + // RESOLVED, unconditionally. A fixture-target-only diff is routed to + // needs-human (NO_PRODUCTION_CHANGE) rather than auto-resolved — matching the + // docstring invariant, which the earlier `onTargetFiles`-satisfies-it logic + // violated (it let a canary fixture-only edit reach resolved:true). const onTargetFiles = changedFiles.filter((f) => targets.has(f)); - if (productionFiles.length === 0 && onTargetFiles.length === 0) { + if (productionFiles.length === 0) { return { resolved: false, reason: PredicateReason.NO_PRODUCTION_CHANGE, detail: - "Fix changed zero production mock-builder files and no report-named fixture target — a clean collector is meaningless without a real fix. Nothing shippable.", + "Fix changed zero production mock-builder files — a real drift fix always updates the " + + "production mock builder (src/** outside src/__tests__/). A fixture-target-only change " + + "(e.g. a model-list fixture edit with no builder change) is not independently verifiable " + + "(the re-collect reads the same fixture) and is routed to human review, not auto-resolved.", offendingFiles: [], }; } diff --git a/scripts/fix-drift.ts b/scripts/fix-drift.ts index 35a037f1..c712ba92 100644 --- a/scripts/fix-drift.ts +++ b/scripts/fix-drift.ts @@ -557,7 +557,11 @@ export function addChangelogEntry(report: DriftReport, version: string): void { } } -export function buildPrBody(report: DriftReport, changedFiles?: string[]): string { +export function buildPrBody( + report: DriftReport, + changedFiles?: string[], + verdictDetail?: string, +): string { const providers: string[] = []; const diffs: string[] = []; @@ -575,6 +579,19 @@ export function buildPrBody(report: DriftReport, changedFiles?: string[]): strin "", "Auto-generated drift remediation.", "", + // Human-approval backstop (WS-2): this PR was auto-FILTERED by the + // drift-success predicate but is NOT auto-merged. A human must review CI + + // this diff + the verdict below and merge. The predicate is a strong filter, + // not a provable merge gate (the re-collect is not independent of the fix — + // WS-2b), so the merge decision stays with a human. + "> **Needs human review + merge.** This drift-fix PR was opened by the", + "> automated pipeline after the drift-success predicate passed. It is NOT", + "> auto-merged — review CI, the diff, and the verdict below, then merge.", + "", + "### Drift-success predicate verdict", + "", + verdictDetail ? `RESOLVED — ${verdictDetail}` : "RESOLVED.", + "", "### Providers affected", ...providers, "", @@ -683,7 +700,7 @@ export function gatedCommitFiles( return { builderFiles, testFiles, skillFiles, stragglers }; } -function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { +export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { const stamp = todayStamp(); // Detect uncommitted changes (staged + unstaged) BEFORE any git write ops so @@ -779,7 +796,28 @@ function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { // an unnamed fixture, a config file — would have BLOCKED at the gate, so // sweeping it in AFTER the pass silently defeats the allowlist). const sanctioned = sanctionedTargets(report); - const { builderFiles, testFiles, skillFiles } = gatedCommitFiles(changedFiles, sanctioned); + const { builderFiles, testFiles, skillFiles, stragglers } = gatedCommitFiles( + changedFiles, + sanctioned, + ); + + // FIX #F5 (round-4) — on a RESOLVED verdict `stragglers` MUST be empty: the + // predicate allowlist already rejected any file that is neither production + // source nor a report-named fixture target, so every changed file must fall + // into one of the gated groups above. If a straggler survives here, the + // verdict and the staging partition have DIVERGED (e.g. a future predicate + // change admitted a file gatedCommitFiles does not classify) — silently + // dropping it would ship an incomplete fix behind a green verdict. Fail closed + // to human review rather than open a PR whose diff differs from what was scored. + if (stragglers.length > 0) { + console.error( + `ERROR: RESOLVED verdict but ${stragglers.length} changed file(s) are not in any gated ` + + `commit group (${stragglers.join(", ")}) — the verdict and the staging partition have ` + + "diverged. Refusing to open a PR that would silently drop these from the diff.", + ); + console.log(`reason=${PredicateReason.UNSANCTIONED_CHANGE}`); + process.exit(REASON_EXIT_CODE[PredicateReason.UNSANCTIONED_CHANGE]); + } if (builderFiles.length > 0) { execFileSafe("git", ["add", ...builderFiles]); @@ -822,7 +860,7 @@ function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { execFileSafe("git", ["push", "-u", "origin", branchName]); console.log(`Pushed branch ${branchName}`); - const prBody = buildPrBody(report, changedFiles); + const prBody = buildPrBody(report, changedFiles, verdict.detail); const prTitle = `fix: auto-remediate API drift (${stamp})`; const prBodyFile = `/tmp/aimock-drift-${process.pid}-pr-body.md`; @@ -943,6 +981,29 @@ export function hasPostFixArgs(args: string[]): boolean { return hasReport && hasExit; } +/** + * FIX #F7 (round-4) — parse the `--post-fix-exit` value, failing CLOSED on an + * empty/whitespace or non-integer value. `Number("")` and `Number(" ")` are + * both 0, which Number.isInteger accepts, so a missing recollect output + * (`--post-fix-exit ""`, e.g. a skipped step) would masquerade as a clean + * collector exit 0 and open a PR on an unverified fix. This mirrors the + * predicate CLI's guard (drift-success-predicate.ts:parseCliArgs). Throws on any + * empty/whitespace/non-integer input; the caller must NOT treat that as clean. + */ +export function parsePostFixExit(raw: string): number { + if (raw.trim() === "") { + throw new Error( + "--post-fix-exit is empty/whitespace — a missing collector exit code must fail closed, " + + "not be treated as clean exit 0", + ); + } + const parsed = Number(raw); + if (!Number.isInteger(parsed)) { + throw new Error(`--post-fix-exit must be an integer, got "${raw}"`); + } + return parsed; +} + async function main(): Promise { const args = process.argv.slice(2); const mode = parseMode(args); @@ -989,10 +1050,11 @@ async function main(): Promise { "predicate is the authoritative PR-open gate; there is no legacy no-post-fix path)", ); } - const postFixExit = Number(args[postFixExitIdx + 1]); - if (!Number.isInteger(postFixExit)) { - throw new Error(`--post-fix-exit must be an integer, got "${args[postFixExitIdx + 1]}"`); - } + // FIX #F7 (round-4) — fail CLOSED on an empty/whitespace/non-integer + // --post-fix-exit (see parsePostFixExit): a missing recollect output must + // not masquerade as a clean collector exit 0 and open a PR on an unverified + // fix. Mirrors the predicate CLI's guard. + const postFixExit = parsePostFixExit(args[postFixExitIdx + 1]); const postFixReport = readPostFixReport(resolve(args[postFixReportIdx + 1])); const postFix: PostFixCollectorResult = { report: postFixReport, exitCode: postFixExit }; diff --git a/src/__tests__/drift-success-predicate.test.ts b/src/__tests__/drift-success-predicate.test.ts index d327fd89..3ad04a74 100644 --- a/src/__tests__/drift-success-predicate.test.ts +++ b/src/__tests__/drift-success-predicate.test.ts @@ -12,11 +12,12 @@ * testFiles>0`) would have ACCEPTED it — demonstrated by contrast below. */ -import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, writeFileSync, rmSync, mkdirSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect, afterEach, vi } from "vitest"; import type { DriftEntry, DriftReport, ParsedDiff } from "../../scripts/drift-types.js"; import { @@ -515,8 +516,13 @@ describe("allowlist inversion — non-allowlisted changed files ALWAYS block", ( expect(verdict.offendingFiles).toContain("src/__tests__/drift/model-registry.ts"); }); - it("a report-NAMED fixture target (builderFile) + clean collector → RESOLVED", () => { - // The collector sanctions the fixture as the fix target (canary routing). + it("FIX #F2 (round-4): a report-NAMED fixture target ALONE (no production change) → NO_PRODUCTION_CHANGE (routed to human, NOT auto-resolved)", () => { + // A canary that names ONLY the fixture as its target and whose diff touches + // ONLY that fixture (model-registry.ts) — with NO production/builder change. + // The module invariant (Signal 2) requires >=1 production mock-builder change + // for RESOLVED: a fixture-target-only change is not independently verifiable + // (the re-collect reads the same fixture), so it must route to human review, + // never auto-resolve. This is the canary-only bypass F2 closes. const canary = report([ entry({ provider: "OpenAI Realtime", @@ -532,6 +538,31 @@ describe("allowlist inversion — non-allowlisted changed files ALWAYS block", ( report: canary, ...cleanSignal, }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.NO_PRODUCTION_CHANGE); + expect(REASON_EXIT_CODE[verdict.reason]).toBe(10); + }); + + it("FIX #F2 (round-4): a report-NAMED fixture target ACCOMPANIED BY a production change → RESOLVED (the legit canary shape)", () => { + // The legit canary shape: the report names BOTH a production builder and the + // fixture, and BOTH change. The production change satisfies the >=1 + // production-change invariant, so this auto-resolves (unlike the + // fixture-only case above). + const canary = report([ + entry({ + provider: "OpenAI Realtime", + scenario: "known-models canary", + builderFile: "src/ws-realtime.ts", + builderFunctions: ["buildRealtimeSession"], + typesFile: "src/__tests__/drift/model-registry.ts", + diffs: [diff({ path: "knownModels", issue: "new model shipped" })], + }), + ]); + const verdict = evaluateDriftResolved({ + changedFiles: ["src/ws-realtime.ts", "src/__tests__/drift/model-registry.ts"], + report: canary, + ...cleanSignal, + }); expect(verdict.resolved).toBe(true); expect(verdict.reason).toBe(PredicateReason.RESOLVED); }); @@ -1137,6 +1168,154 @@ describe("runCli maps a repo-escaping/absolute changed-file to CONFIG_ERROR (exi }); }); +// --------------------------------------------------------------------------- +// FIX #F3 (round-4) — a collector-output artifact left in the repo working tree +// (drift-report.post-fix.json / drift-report.json / claude-code-output.log) +// appears as an untracked file in `git status --porcelain` and is scored by the +// predicate as UNSANCTIONED_CHANGE, breaking the happy path. The workflow fix +// writes those artifacts to $RUNNER_TEMP (outside the checkout) so they never +// enter the changed-file set. These pure-function locks prove the predicate's +// behaviour on BOTH sides of that move. +// --------------------------------------------------------------------------- +describe("FIX #F3 — collector-output artifacts must not be in the changed-file set", () => { + const sanctioned = () => + report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]); + const cleanSignal = { postFixCollectorExit: 0, postFixCriticalCount: 0 }; + + it("RED (the bug): a post-fix report artifact in the changed set → UNSANCTIONED_CHANGE (happy path broken)", () => { + // This is exactly what happened when the re-collect wrote into the repo cwd: + // the untracked drift-report.post-fix.json joined the changed set and, being + // neither production source nor a report-named target, fail-closed the run. + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts", "drift-report.post-fix.json"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(false); + expect(verdict.reason).toBe(PredicateReason.UNSANCTIONED_CHANGE); + expect(verdict.offendingFiles).toContain("drift-report.post-fix.json"); + }); + + it("GREEN (the fix): the SAME production fix WITHOUT the artifact (now in $RUNNER_TEMP) → RESOLVED", () => { + const verdict = evaluateDriftResolved({ + changedFiles: ["src/helpers.ts"], + report: sanctioned(), + ...cleanSignal, + }); + expect(verdict.resolved).toBe(true); + expect(verdict.reason).toBe(PredicateReason.RESOLVED); + }); +}); + +// --------------------------------------------------------------------------- +// TEST-TIGHTNESS (round-4 slot-3) — lock the runCli behaviour the workflow +// depends on: (F2) the machine-readable `reason=` stdout line the +// "Assert" step greps for Slack routing, and (F1) the CONFIG_ERROR path so +// deleting the parse-error catch fails a test. These drive the REAL runCli in an +// isolated temp git repo so gitChangedFiles() returns a controlled set. +// --------------------------------------------------------------------------- +describe("runCli machine-readable reason= line + CONFIG_ERROR lock (slot-3 F1/F2)", () => { + let repo: string | null = null; + let logSpy: ReturnType | null = null; + let errSpy: ReturnType | null = null; + const origCwd = process.cwd(); + + afterEach(() => { + process.chdir(origCwd); + logSpy?.mockRestore(); + errSpy?.mockRestore(); + logSpy = null; + errSpy = null; + if (repo) rmSync(repo, { recursive: true, force: true }); + repo = null; + }); + + function initRepo(): string { + const dir = mkdtempSync(join(tmpdir(), "ws2-runcli-")); + execFileSync("git", ["init", "-q"], { cwd: dir }); + execFileSync("git", ["config", "user.email", "t@t"], { cwd: dir }); + execFileSync("git", ["config", "user.name", "t"], { cwd: dir }); + return dir; + } + + function captureConsole(): void { + logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + } + + function stdoutLines(): string[] { + return (logSpy?.mock.calls ?? []).map((c) => String(c[0])); + } + + it("prints `reason=resolved` on a genuine RESOLVED verdict (the line the workflow greps)", () => { + repo = initRepo(); + // A real production change in the working tree → gitChangedFiles() reports it. + // `git add -N` (intent-to-add) makes porcelain list the INDIVIDUAL file + // (`A src/helpers.ts`) rather than collapsing an all-untracked dir to `?? src/`. + mkdirSync(join(repo, "src"), { recursive: true }); + writeFileSync(join(repo, "src", "helpers.ts"), "export const x = 1;\n", "utf-8"); + execFileSync("git", ["add", "-N", "src/helpers.ts"], { cwd: repo }); + // Report files live OUTSIDE the repo (mirrors the workflow's $RUNNER_TEMP, + // FIX #F3) so they are NOT untracked entries in `git status --porcelain` and + // do not pollute the changed-file set the predicate scores. + const outDir = mkdtempSync(join(tmpdir(), "ws2-runcli-out-")); + const rep = report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]); + const preP = join(outDir, "pre.json"); + const postP = join(outDir, "post.json"); + writeFileSync(preP, JSON.stringify(rep), "utf-8"); + writeFileSync(postP, JSON.stringify(report([])), "utf-8"); + process.chdir(repo); + captureConsole(); + try { + const code = runCli(["--report", preP, "--post-fix-report", postP, "--post-fix-exit", "0"]); + expect(code).toBe(0); + expect(stdoutLines()).toContain(`reason=${PredicateReason.RESOLVED}`); + } finally { + rmSync(outDir, { recursive: true, force: true }); + } + }); + + it("prints `reason=unsanctioned-change` (non-zero) on a blocked verdict — the Slack routing key", () => { + repo = initRepo(); + mkdirSync(join(repo, "src"), { recursive: true }); + writeFileSync(join(repo, "src", "helpers.ts"), "export const x = 1;\n", "utf-8"); + // package.json IN the repo is the unsanctioned change; reports live OUTSIDE. + writeFileSync(join(repo, "package.json"), '{"name":"x"}\n', "utf-8"); + execFileSync("git", ["add", "-N", "src/helpers.ts", "package.json"], { cwd: repo }); + const outDir = mkdtempSync(join(tmpdir(), "ws2-runcli-out-")); + const rep = report([entry({ builderFile: "src/helpers.ts", typesFile: "src/types.ts" })]); + const preP = join(outDir, "pre.json"); + const postP = join(outDir, "post.json"); + writeFileSync(preP, JSON.stringify(rep), "utf-8"); + writeFileSync(postP, JSON.stringify(report([])), "utf-8"); + process.chdir(repo); + captureConsole(); + try { + const code = runCli(["--report", preP, "--post-fix-report", postP, "--post-fix-exit", "0"]); + expect(code).toBe(REASON_EXIT_CODE[PredicateReason.UNSANCTIONED_CHANGE]); + expect(stdoutLines()).toContain(`reason=${PredicateReason.UNSANCTIONED_CHANGE}`); + } finally { + rmSync(outDir, { recursive: true, force: true }); + } + }); + + it("prints `reason=config-error` and exits 2 when the report is unreadable (CONFIG_ERROR lock)", () => { + repo = initRepo(); + process.chdir(repo); + captureConsole(); + const code = runCli([ + "--report", + join(repo, "does-not-exist.json"), + "--post-fix-report", + join(repo, "also-missing.json"), + "--post-fix-exit", + "0", + ]); + expect(code).toBe(REASON_EXIT_CODE[PredicateReason.CONFIG_ERROR]); + expect(stdoutLines()).toContain(`reason=${PredicateReason.CONFIG_ERROR}`); + }); +}); + // --------------------------------------------------------------------------- // CR round-3 F3 — readReport ENTRY-LEVEL validation aligned with // fix-drift.ts:readDriftReport. A structurally-valid report whose entries are diff --git a/src/__tests__/fix-drift-straggler.test.ts b/src/__tests__/fix-drift-straggler.test.ts new file mode 100644 index 00000000..4bd78a49 --- /dev/null +++ b/src/__tests__/fix-drift-straggler.test.ts @@ -0,0 +1,122 @@ +/** + * FIX #F5 (round-4) — createPr's STRAGGLER fail-closed guard, in isolation. + * + * On a RESOLVED verdict every changed file MUST fall into a gated commit group + * (production builder, report-named fixture target, or skills/). A "straggler" + * (a changed file in none of those groups) means the predicate verdict and the + * staging partition have DIVERGED — staging would silently drop it and ship an + * incomplete fix behind a green verdict. createPr must exit UNSANCTIONED_CHANGE + * and stage NOTHING. + * + * By construction with the CURRENT predicate + gatedCommitFiles there is no file + * that both PASSES the predicate and becomes a straggler (the allowlist blocks + * everything gatedCommitFiles would not classify). This guard is therefore + * defense-in-depth against a FUTURE predicate change. To lock it load-bearingly + * we mock `evaluateDriftResolved` to force a RESOLVED verdict while git reports a + * straggler in the working tree, then assert createPr fail-closes before any + * `git add`. This is a pure-function/mock test — it never runs the autofix + * subprocess. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +import type { DriftReport } from "../../scripts/drift-types.js"; + +// Mock git so gitChangedFiles()/exec() are controllable, and force the predicate +// to RESOLVED so the straggler guard (which runs AFTER the verdict) is reached. +vi.mock("node:fs", async () => { + const actual = await vi.importActual("node:fs"); + return { ...actual, readFileSync: vi.fn(actual.readFileSync), writeFileSync: vi.fn() }; +}); + +vi.mock("node:child_process", async () => { + const actual = await vi.importActual("node:child_process"); + return { ...actual, execFileSync: vi.fn(), execSync: vi.fn() }; +}); + +vi.mock("../../scripts/drift-success-predicate.js", async () => { + const actual = await vi.importActual( + "../../scripts/drift-success-predicate.js", + ); + return { + ...actual, + // Force RESOLVED regardless of inputs so the guard downstream is exercised. + evaluateDriftResolved: vi.fn(() => ({ + resolved: true, + reason: actual.PredicateReason.RESOLVED, + detail: "forced resolved for the straggler-guard test", + offendingFiles: [], + })), + // gitChangedFiles is used by createPr — return a production file PLUS a + // straggler (a root file gatedCommitFiles cannot classify). + gitChangedFiles: vi.fn(() => ["src/helpers.ts", "weird-root-file.txt"]), + }; +}); + +import { execSync, execFileSync } from "node:child_process"; + +import { createPr } from "../../scripts/fix-drift.js"; + +const mockedExecSync = vi.mocked(execSync); +const mockedExecFileSync = vi.mocked(execFileSync); + +describe("createPr straggler guard fail-closed (fix #F5, isolated)", () => { + let logSpy: ReturnType; + let errSpy: ReturnType; + let exitSpy: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`__exit__${code}`); + }) as never); + // Default git calls return empty (branch lookups etc.). + mockedExecSync.mockReturnValue("fix/drift-2026-07-16\n" as unknown as string); + }); + + afterEach(() => { + logSpy.mockRestore(); + errSpy.mockRestore(); + exitSpy.mockRestore(); + }); + + const rep: DriftReport = { + timestamp: "2026-07-16T00:00:00.000Z", + entries: [ + { + provider: "OpenAI", + scenario: "chat completion", + builderFile: "src/helpers.ts", + builderFunctions: ["buildChatCompletion"], + typesFile: null, + sdkShapesFile: "src/__tests__/drift/sdk-shapes.ts", + diffs: [ + { + path: "x", + severity: "critical", + issue: "missing", + expected: "string", + real: "string", + mock: "", + }, + ], + }, + ], + }; + + it("exits UNSANCTIONED_CHANGE (17) and NEVER stages the straggler even on a RESOLVED verdict", () => { + expect(() => createPr(rep, { report: { timestamp: "t", entries: [] }, exitCode: 0 })).toThrow( + /__exit__17/, + ); + + const staged = mockedExecFileSync.mock.calls.some( + (c) => + c[0] === "git" && Array.isArray(c[1]) && (c[1] as string[]).includes("weird-root-file.txt"), + ); + expect(staged).toBe(false); + + const lines = logSpy.mock.calls.map((c) => String(c[0])); + expect(lines).toContain("reason=unsanctioned-change"); + }); +}); diff --git a/src/__tests__/fix-drift-workflow.test.ts b/src/__tests__/fix-drift-workflow.test.ts index 49bb5e58..98af1c79 100644 --- a/src/__tests__/fix-drift-workflow.test.ts +++ b/src/__tests__/fix-drift-workflow.test.ts @@ -32,11 +32,12 @@ const wf = readFileSync(WORKFLOW_PATH, "utf-8"); const wfFlat = wf.replace(/\s+/g, " "); describe("fix-drift.yml — F1: post-fix re-collect + args wired into --create-pr", () => { - it("has an authoritative post-fix re-collect step writing a DISTINCT report path", () => { + it("has an authoritative post-fix re-collect step writing a DISTINCT report path OUTSIDE the repo (FIX #F3)", () => { expect(wf).toContain("Re-collect drift (authoritative)"); - expect(wf).toContain( - "npx tsx scripts/drift-report-collector.ts --out drift-report.post-fix.json", - ); + // FIX #F3 — the re-collect writes to $RUNNER_TEMP (via the POST_FIX_REPORT + // env), NOT the repo cwd, so it is never scored by the predicate's git scan. + expect(wf).toContain('npx tsx scripts/drift-report-collector.ts --out "$POST_FIX_REPORT"'); + expect(wf).toContain("POST_FIX_REPORT: ${{ runner.temp }}/drift-report.post-fix.json"); }); it("captures the post-fix collector exit code as a step output", () => { @@ -46,14 +47,14 @@ describe("fix-drift.yml — F1: post-fix re-collect + args wired into --create-p it("passes BOTH --post-fix-report and --post-fix-exit into `fix-drift.ts --create-pr`", () => { expect(wfFlat).toContain("npx tsx scripts/fix-drift.ts --create-pr"); expect(wfFlat).toMatch( - /fix-drift\.ts --create-pr[^]*?--post-fix-report drift-report\.post-fix\.json[^]*?--post-fix-exit "\$\{POST_FIX_EXIT\}"/, + /fix-drift\.ts --create-pr[^]*?--post-fix-report "\$\{POST_FIX_REPORT\}"[^]*?--post-fix-exit "\$\{POST_FIX_EXIT\}"/, ); }); it("the Assert step runs the predicate with post-fix args (the happy-path gate)", () => { expect(wf).toContain("Assert drift truly resolved"); expect(wfFlat).toMatch( - /drift-success-predicate\.ts[^]*?--post-fix-report drift-report\.post-fix\.json[^]*?--post-fix-exit "\$\{POST_FIX_EXIT\}"/, + /drift-success-predicate\.ts[^]*?--post-fix-report "\$\{POST_FIX_REPORT\}"[^]*?--post-fix-exit "\$\{POST_FIX_EXIT\}"/, ); }); }); @@ -61,7 +62,10 @@ describe("fix-drift.yml — F1: post-fix re-collect + args wired into --create-p describe("fix-drift.yml — F-A: PRE-fix report pinned outside the LLM-writable checkout", () => { it("has a pin step that copies the pre-fix report into runner.temp before autofix", () => { expect(wf).toContain("Pin pre-fix drift report (integrity)"); - expect(wf).toContain('cp drift-report.json "$PINNED_REPORT"'); + // FIX #F3 — the pre-fix report is itself collected into $RUNNER_TEMP + // (PRE_FIX_REPORT), so the pin copies from that out-of-repo path, never the + // repo cwd. Both source and destination are outside the LLM-writable checkout. + expect(wf).toContain('cp "$PRE_FIX_REPORT" "$PINNED_REPORT"'); expect(wf).toContain("PINNED_REPORT: ${{ runner.temp }}/drift-report.pinned.json"); }); @@ -92,3 +96,35 @@ describe("fix-drift.yml — F-A: PRE-fix report pinned outside the LLM-writable expect(wfFlat).not.toMatch(/fix-drift\.ts --create-pr \\? *--report drift-report\.json/); }); }); + +// --------------------------------------------------------------------------- +// FIX (round-4, user-approved) — HUMAN-APPROVAL BACKSTOP. The drift path opens a +// PR but must NEVER auto-merge: the predicate is a strong AUTO-FILTER, not a +// provable merge gate (the re-collect is not independent of the fix — WS-2b), so +// a human reviews CI + the diff + the verdict and merges. These lock that the +// unattended in-workflow merge is GONE and the Slack copy no longer claims +// "merged to main". +// --------------------------------------------------------------------------- +describe("fix-drift.yml — human-approval backstop: no unattended auto-merge", () => { + it("has NO auto-merge step and never runs `gh pr merge`", () => { + expect(wf).not.toContain("Auto-merge PR"); + expect(wf).not.toMatch(/gh pr merge/); + }); + + it("documents WHY the drift path is human-gated (predicate is a filter, not a merge gate)", () => { + expect(wf).toContain("NO AUTO-MERGE"); + // The rationale wraps across comment lines (a `#` marker survives flattening), + // so match tolerantly across the wrap. + expect(wfFlat).toMatch(/AUTO-FILTER, NOT a provable merge (# )?gate/i); + }); + + it("the success Slack message says the PR needs human review + merge, NOT merged to main", () => { + expect(wf).not.toContain("Drift auto-fix merged to main"); + expect(wf).toContain("Drift-fix PR opened — needs human review + merge"); + }); + + it("the fix-failure Slack step no longer references the removed merge step outputs", () => { + expect(wf).not.toContain("steps.merge.outputs"); + expect(wf).not.toContain("MERGE_REASON"); + }); +}); diff --git a/src/__tests__/fix-drift.test.ts b/src/__tests__/fix-drift.test.ts index 0bac377a..83dc10b3 100644 --- a/src/__tests__/fix-drift.test.ts +++ b/src/__tests__/fix-drift.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { resolve } from "node:path"; import type { @@ -40,8 +40,10 @@ import { execFileSafe, parseMode, hasPostFixArgs, + parsePostFixExit, getChangedFiles, gatedCommitFiles, + createPr, affectedSkillSections, BUILDER_TO_SKILL_SECTION, truncateBody, @@ -812,6 +814,33 @@ describe("hasPostFixArgs (fix #5 legacy-fallback closure)", () => { }); }); +// --------------------------------------------------------------------------- +// FIX #F7 (round-4) — parsePostFixExit: the PR path must fail CLOSED on an +// empty/whitespace --post-fix-exit rather than accept Number("")===0 as a clean +// collector exit 0. Mirrors the predicate CLI's guard so a missing recollect +// output never masquerades as clean and opens a PR on an unverified fix. +// --------------------------------------------------------------------------- +describe("parsePostFixExit (fix #F7 empty-exit fail-closed)", () => { + it("throws on an empty string (Number('')===0 must NOT slip through as clean)", () => { + expect(() => parsePostFixExit("")).toThrow(/empty\/whitespace/); + }); + + it("throws on a whitespace-only value", () => { + expect(() => parsePostFixExit(" ")).toThrow(/empty\/whitespace/); + }); + + it("throws on a non-integer value", () => { + expect(() => parsePostFixExit("abc")).toThrow(/integer/); + expect(() => parsePostFixExit("1.5")).toThrow(/integer/); + }); + + it("returns the integer for a valid value", () => { + expect(parsePostFixExit("0")).toBe(0); + expect(parsePostFixExit("2")).toBe(2); + expect(parsePostFixExit("-1")).toBe(-1); + }); +}); + // --------------------------------------------------------------------------- // getChangedFiles // --------------------------------------------------------------------------- @@ -898,6 +927,101 @@ describe("gatedCommitFiles (F-C: no straggler catch-all)", () => { }); }); +// --------------------------------------------------------------------------- +// FIX #F5 (round-4) — createPr MUST fail-closed if any changed file falls +// outside every gated commit group (a straggler): staging it as part of an +// allowlisted group would silently drop or mis-stage it. Because the predicate +// allowlist already blocks any non-production, non-report-named file BEFORE +// createPr stages, an unclassified working-tree file causes createPr to exit +// with UNSANCTIONED_CHANGE and stage NOTHING — the straggler is never `git add`ed. +// +// Driven against the REAL createPr with git mocked (no autofix subprocess). +// --------------------------------------------------------------------------- +describe("createPr straggler / unsanctioned fail-closed (fix #F5)", () => { + const mockedExecSync = vi.mocked(execSync); + let logSpy: ReturnType | null = null; + let errSpy: ReturnType | null = null; + let exitSpy: ReturnType | null = null; + + beforeEach(() => { + vi.clearAllMocks(); + logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`__exit__${code}`); + }) as never); + void errSpy; + void exitSpy; + }); + + afterEach(() => { + logSpy?.mockRestore(); + errSpy?.mockRestore(); + exitSpy?.mockRestore(); + }); + + function stdoutLines(): string[] { + return (logSpy?.mock.calls ?? []).map((c) => String(c[0])); + } + + const rep: DriftReport = { + timestamp: "2026-07-16T00:00:00.000Z", + entries: [ + { + provider: "OpenAI", + scenario: "chat completion", + builderFile: "src/helpers.ts", + builderFunctions: ["buildChatCompletion"], + typesFile: null, + sdkShapesFile: "src/__tests__/drift/sdk-shapes.ts", + diffs: [ + { + path: "x", + severity: "critical", + issue: "missing", + expected: "string", + real: "string", + mock: "", + }, + ], + }, + ], + }; + + it("exits UNSANCTIONED_CHANGE (17) and stages NOTHING when an unclassified file is in the tree", () => { + // A production fix (helpers.ts, allowlisted) PLUS an unclassified root file. + // createPr fail-closes (the root file is neither allowlisted nor a gated + // group) BEFORE any staging — the straggler is never git-added. + mockedExecSync.mockImplementation((cmd: unknown) => { + if (typeof cmd === "string" && cmd.includes("status --porcelain")) { + return "M src/helpers.ts\n?? weird-root-file.txt\n" as unknown as string; + } + return "" as unknown as string; + }); + + expect(() => createPr(rep, { report: { timestamp: "t", entries: [] }, exitCode: 0 })).toThrow( + /__exit__17/, + ); + + const addedStraggler = mockedExecFileSync.mock.calls.some( + (c) => + c[0] === "git" && Array.isArray(c[1]) && (c[1] as string[]).includes("weird-root-file.txt"), + ); + expect(addedStraggler).toBe(false); + expect(stdoutLines()).toContain("reason=unsanctioned-change"); + }); + + it("gatedCommitFiles never leaves a production or report-named file as a straggler (the invariant createPr asserts)", () => { + // The straggler guard in createPr is defense-in-depth: prove the partition + // it relies on classifies every allowlisted file into a gated group so + // stragglers is empty on any RESOLVED-shaped set. + const s = sanctionedTargets(rep); + const g = gatedCommitFiles(["src/helpers.ts", "src/foo.ts"], s); + expect(g.stragglers).toEqual([]); + expect(g.builderFiles).toEqual(["src/helpers.ts", "src/foo.ts"]); + }); +}); + // --------------------------------------------------------------------------- // CR round-3 F-A — the sanctioned/allowlist set is derived SOLELY from the // (pinned) report object passed to createPr, NOT from any on-disk file. The From 71856a3ffefd12209342a9587a847d476ce1ab89 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Fri, 17 Jul 2026 12:53:00 -0700 Subject: [PATCH 6/6] chore(drift): align autofix prompt with predicate allowlist; drop dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The buildPrompt() skill-update instruction told the LLM to edit skills/write-fixtures/SKILL.md, but the drift-success predicate allowlist only sanctions src/** production code and report-named fixtures. Any SKILL.md edit therefore tripped UNSANCTIONED_CHANGE and rejected the whole run, blocking legitimate fixes. Remove the instruction so the autofix only touches production mock code (the allowlist stays tight — skills/ is deliberately not added). With the prompt no longer touching skills and the allowlist never sanctioning skills/**, the skillFiles staging group in gatedCommitFiles/createPr is unreachable dead code; remove it along with the now-unused SKILL_FILE constant and the stale prompt test. Also remove scripts/ci-merge-gate.sh and its test: the workflow no longer auto-merges, so the green-gate script has zero references. --- scripts/ci-merge-gate.sh | 327 --------------- scripts/fix-drift.ts | 33 +- src/__tests__/ci-merge-gate.test.ts | 592 ---------------------------- src/__tests__/fix-drift.test.ts | 12 - 4 files changed, 3 insertions(+), 961 deletions(-) delete mode 100755 scripts/ci-merge-gate.sh delete mode 100644 src/__tests__/ci-merge-gate.test.ts diff --git a/scripts/ci-merge-gate.sh b/scripts/ci-merge-gate.sh deleted file mode 100755 index 870d9ac3..00000000 --- a/scripts/ci-merge-gate.sh +++ /dev/null @@ -1,327 +0,0 @@ -#!/usr/bin/env bash -# -# ci-merge-gate.sh — the auto-merge green-gate decision, factored out of the -# fix-drift workflow so it can be unit-tested in isolation. -# -# Reads the machine-readable check-state JSON produced by -# `gh pr checks --json name,state,bucket` (an array of objects) from a -# file argument or stdin, and exits 0 ONLY when the PR is truly green: -# -# 1. at least one check is in the "pass" bucket (>=1), AND -# 2. EVERY present check lands in a recognized bucket and is either in the -# "pass" bucket or on the explicit, documented IGNORE_CONTEXTS allow-list -# — an unknown bucket/state, or a non-ignored skipped/neutral/pending/ -# fail/cancel check, fails the gate (we never silently accept), AND -# 3. every REQUIRED context is present AND concluded in the "pass" bucket. -# -# Policy on neutral/skipped: the "skipping" bucket does NOT count toward -# pass>=1, a required context in "skipping" is treated as NOT satisfied, and a -# NON-required skipped/neutral check FAILS the gate unless its exact name is on -# IGNORE_CONTEXTS. This closes the "newly-added gating check resolves skipped/ -# neutral and is silently ignored → false-green" hole: to accept a skipped -# check you must name it explicitly. -# -# `gh pr checks --json` buckets (from cli/cli): pass | fail | pending | -# skipping | cancel. `state` is the raw check/status conclusion. We key the -# decision off `bucket` and fall back to `state` when bucket is absent so the -# gate is robust to either shape. Any check whose bucket/state does not map to -# one of the five recognized buckets is treated as NOT-pass (fails the gate); -# the recognized buckets must sum to the total check count or we abort. -# -# Required contexts default to the set a drift-fix PR must pass on this repo -# (Static Quality + Unit Tests matrix + Drift Tests PR legs + Zizmor). Override -# via REQUIRED_CONTEXTS (newline- or comma-separated) when the expected set -# changes. An empty/whitespace-only REQUIRED_CONTEXTS is a configuration error -# (exit 2), never a silent no-op that lets a PR through with zero requirements. -# -# Non-required checks that are legitimately skipped may be allow-listed via -# IGNORE_CONTEXTS (newline- or comma-separated, same parsing as -# REQUIRED_CONTEXTS). Anything not on that list and not passing fails the gate. -# -# Usage: -# ci-merge-gate.sh [checks.json] # or pipe JSON on stdin -# REQUIRED_CONTEXTS="a,b,c" ci-merge-gate.sh checks.json -# IGNORE_CONTEXTS="notify,drift" ci-merge-gate.sh checks.json -# -# Exit codes: -# 0 true-green — safe to merge -# 1 not green — do NOT merge (reason printed to stderr) -# 2 usage / malformed-input / configuration error. This is a FAIL-CLOSED -# assertion, not a downstream side effect: a non-array/non-object input, a -# non-string .bucket/.state, an empty/contradictory required set, or ANY -# failure of the SINGLE guarded verdict computation (jq parse/runtime -# error, empty jq output, or a verdict object lacking a boolean .green) -# exits 2 here rather than reading an emptied jq result as green. The whole -# pass/fail decision is computed by ONE jq program and validated ONCE -# before the shell reads any field; the gate never merges on a verdict it -# could not trust. - -set -euo pipefail - -DEFAULT_REQUIRED_CONTEXTS='prettier -eslint -exports -commitlint -test (20) -test (22) -test (24) -agui-schema-drift -drift-live-pr -zizmor' - -# Non-required checks that legitimately do not run on a drift-fix PR and must -# NOT block the merge when they resolve skipped/neutral. Keep this list tight — -# every name here is an explicit, reviewed decision to tolerate a non-passing -# state for that context. -DEFAULT_IGNORE_CONTEXTS='notify -drift' - -REQUIRED_CONTEXTS="${REQUIRED_CONTEXTS:-$DEFAULT_REQUIRED_CONTEXTS}" -IGNORE_CONTEXTS="${IGNORE_CONTEXTS:-$DEFAULT_IGNORE_CONTEXTS}" - -# Read the check JSON from the file arg or stdin. -if [ "$#" -ge 1 ] && [ "$1" != "-" ]; then - if [ ! -f "$1" ]; then - echo "::error::ci-merge-gate: input file not found: $1" >&2 - exit 2 - fi - CHECKS_JSON="$(cat "$1")" -else - CHECKS_JSON="$(cat)" -fi - -if [ -z "${CHECKS_JSON//[[:space:]]/}" ]; then - echo "::error::ci-merge-gate: empty check JSON — treating as NOT green" >&2 - exit 1 -fi - -# Validate it parses as a JSON array whose every element is an object. An -# array of non-objects (e.g. [1,2,3] or ["a"]) would pass a bare type=="array" -# guard and then crash jq on `.bucket` indexing (undocumented exit 5); the -# script's contract is 0/1/2 only, so malformed shapes must exit 2 here. -if ! echo "$CHECKS_JSON" | jq -e 'type == "array"' >/dev/null 2>&1; then - echo "::error::ci-merge-gate: check JSON is not a JSON array" >&2 - exit 2 -fi -if ! echo "$CHECKS_JSON" | jq -e 'all(.[]; type == "object")' >/dev/null 2>&1; then - echo "::error::ci-merge-gate: check JSON array contains a non-object element — malformed input" >&2 - exit 2 -fi - -# Field-type guard: a valid JSON object can still carry a non-string `.bucket` -# or `.state` (a number/object/array) — that passes the object guard above but -# is malformed check data from `gh` and, without this guard, would throw -# "explode input must be a string" inside `ascii_downcase` (undocumented jq -# exit 5). We validate here and FAIL CLOSED with the documented config-error -# exit 2. Absent/null fields are fine (handled downstream as derive-from-state / -# unknown); only a PRESENT non-string value is rejected. -if ! echo "$CHECKS_JSON" | jq -e \ - 'all(.[]; (.bucket | type | . == "string" or . == "null") and (.state | type | . == "string" or . == "null"))' \ - >/dev/null 2>&1; then - echo "::error::ci-merge-gate: a check has a non-string .bucket or .state — malformed check data, treating as config error" >&2 - exit 2 -fi - -# Normalize a comma-/newline-separated list into a JSON array so jq can reason -# over it. Trims surrounding whitespace, drops blank entries. Uses `|| true` so -# an all-blank input (grep matches nothing → exit 1) does not abort under -# `set -e`; the caller inspects the resulting array length. -normalize_list() { - printf '%s' "$1" \ - | tr ',' '\n' \ - | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ - | { grep -v '^$' || true; } \ - | jq -R . \ - | jq -s . -} - -REQUIRED_JSON="$(normalize_list "$REQUIRED_CONTEXTS")" -IGNORE_JSON="$(normalize_list "$IGNORE_CONTEXTS")" - -# An empty required set is a configuration error, not a silent green: a gate -# with zero requirements would merge a PR that ran no gating checks at all. -if [ "$(echo "$REQUIRED_JSON" | jq 'length')" -eq 0 ]; then - echo "::error::ci-merge-gate: REQUIRED_CONTEXTS is empty or whitespace-only — refusing to run a gate with no required checks (config error)" >&2 - exit 2 -fi - -# Contradictory config: a name that is BOTH required and ignored is -# self-contradictory (a required context can never be "safe to skip"). Rather -# than silently resolve the ambiguity one way, fail closed as a config error. -CONFLICTING_CONTEXTS="$( - jq -rn --argjson req "$REQUIRED_JSON" --argjson ign "$IGNORE_JSON" \ - '$req - ($req - $ign) | .[]' -)" -if [ -n "$CONFLICTING_CONTEXTS" ]; then - echo "::error::ci-merge-gate: context(s) appear in BOTH REQUIRED_CONTEXTS and IGNORE_CONTEXTS — contradictory config, refusing to run:" >&2 - while IFS= read -r line; do - [ -n "$line" ] && echo "::error:: - $line" >&2 - done <<<"$CONFLICTING_CONTEXTS" - exit 2 -fi - -# Effective bucket for a check: prefer `.bucket`; else derive from `.state`. -# gh buckets: pass | fail | pending | skipping | cancel. Anything we cannot map -# to one of those five becomes the sentinel "unknown" so it is counted, never -# silently dropped. -# -# `.bucket`/`.state` are coerced to strings BEFORE `ascii_downcase`: a valid -# JSON object can carry a non-string `.bucket` (e.g. a number or object) that -# passes the array-of-objects guard above but throws "explode input must be a -# string" inside `ascii_downcase` (undocumented jq exit 5). A non-string field -# is not a recognized bucket/state, so `coerce_str` maps it to "" which then -# resolves to the "unknown" sentinel — counted, never dropped, never a crash. -# shellcheck disable=SC2016 # jq program; $-vars are jq's, must NOT be shell-expanded -JQ_BUCKET=' - def known: ["pass","fail","pending","skipping","cancel"]; - def coerce_str: if type == "string" then . else "" end; - def eff_bucket: - ( if ((.bucket | coerce_str)) != "" then (.bucket | coerce_str | ascii_downcase) - else - (.state | coerce_str | ascii_downcase) as $s - | if ($s == "success") then "pass" - elif ($s | test("^(neutral|skipped)$")) then "skipping" - elif ($s | test("^(failure|error|action_required|startup_failure|timed_out)$")) then "fail" - elif ($s | test("^(cancelled|canceled|stale)$")) then "cancel" - elif ($s == "") then "unknown" - else "pending" - end - end ) as $b - | if (known | index($b)) != null then $b else "unknown" end; -' - -# --------------------------------------------------------------------------- -# SINGLE GUARDED VERDICT (the structural fix that kills the recurring -# "bare-jq-assignment defaults to empty → green-signal" class). -# -# INVARIANT: there is EXACTLY ONE jq computation over the check data, and its -# output is validated ONCE before the shell reads any field from it. The old -# gate scored the checks through ~9 separate `VAR="$(echo "$CHECKS_JSON" | jq -# ...)"` command-substitutions. `set -e` does NOT guard a command-substitution -# RHS, so a jq crash in any of them produced an EMPTY string — and for several -# (notably UNACCEPTED_CHECKS and MISSING_REQUIRED, whose emptiness means "no -# unaccepted checks" / "no missing required") that empty read as the GREEN -# signal. Consolidating into one call + one assertion means ANY jq/parse -# failure is caught HERE as exit 2 (fail-closed), and it is structurally -# impossible for an unguarded empty jq result to be interpreted as green. -# -# The jq program computes the ENTIRE decision — the green/not-green boolean AND -# the human reason AND every scalar — inside jq, so the shell only consumes the -# already-validated object. It emits ONE JSON object: -# { green: bool, reason: string, total, pass, pending, fail, cancel, skip, -# unknown, unaccepted: [string], missing_required: [string] } -# The shell then asserts jq exited 0 AND produced non-empty valid JSON with a -# boolean `.green`; otherwise `::error::` + exit 2. There is NO bare -# `VAR=$(jq)` whose emptiness can be read as green anywhere below. -# --------------------------------------------------------------------------- - -# shellcheck disable=SC2016 # jq program; $-vars are jq's, must NOT be shell-expanded -JQ_VERDICT="$JQ_BUCKET"' - ('"$REQUIRED_JSON"') as $required - | ('"$IGNORE_JSON"') as $ignored - | . as $checks - # Per-check effective bucket, computed once. - | [ $checks[] | { name: .name, b: (. | eff_bucket) } ] as $scored - | ($scored | length) as $total - | ([ $scored[] | select(.b == "pass") ] | length) as $pass - | ([ $scored[] | select(.b == "pending") ] | length) as $pending - | ([ $scored[] | select(.b == "fail") ] | length) as $fail - | ([ $scored[] | select(.b == "cancel") ] | length) as $cancel - | ([ $scored[] | select(.b == "skipping") ] | length) as $skip - | ([ $scored[] | select(.b == "unknown") ] | length) as $unknown - | ($pass + $pending + $fail + $cancel + $skip) as $recognized_sum - # Non-passing checks that are neither required nor on the ignore allow-list. - # Capture the element into $c first: inside `$required | index(...)` the pipe - # rebinds `.` to $required (the array), so `.name` there would index the array - # (jq error) — reference the captured $c.name instead. - | [ $scored[] - | . as $c - | select($c.b != "pass") - | select( ($required | index($c.name)) == null ) - | select( ($ignored | index($c.name)) == null ) - | "\($c.name) [\($c.b)]" - ] as $unaccepted - # Required contexts that are NOT present-and-passing. Only STRING names of - # pass-bucket checks can satisfy a requirement: a pass check with a - # null/absent/non-string name counts toward pass>=1 but must NOT resolve a - # named requirement. - | ( [ $scored[] | select(.b == "pass") | .name | select(type == "string") ] ) as $passing - | ( $required | map(. as $name | select( ($passing | index($name)) == null )) ) as $missing_required - # ALL triggered reasons, in the same order the old sequence of `if` blocks - # emitted them (a single check can trip more than one, e.g. a required - # context in the cancel bucket trips BOTH "cancelled/stale" AND - # "required context missing"). We collect every applicable reason rather than - # short-circuiting on the first so the human-facing diagnostics — and the - # tests that assert on specific reasons — see exactly the same messages as - # before. An empty reasons array == green. - | ( [ (if ($recognized_sum != $total) then "bucket sum mismatch — recognized=\($recognized_sum) total=\($total) (unknown-bucket check(s) present) — NOT green" else empty end), - (if ($unknown > 0) then "\($unknown) check(s) in an unrecognized bucket/state — NOT green" else empty end), - (if ($pass < 1) then "no checks in '"'"'pass'"'"' bucket (pass=\($pass)) — NOT green" else empty end), - (if ($pending > 0) then "\($pending) check(s) still pending/queued/in_progress — NOT green" else empty end), - (if ($fail > 0) then "\($fail) check(s) failed/errored — NOT green" else empty end), - (if ($cancel > 0) then "\($cancel) check(s) cancelled/stale — NOT green" else empty end), - (if (($unaccepted | length) > 0) then "non-passing check(s) not required and not on IGNORE_CONTEXTS allow-list — NOT green" else empty end), - (if (($missing_required | length) > 0) then "required context(s) missing or not passing" else empty end) - ] ) as $reasons - | { - green: (($reasons | length) == 0), - reasons: $reasons, - total: $total, - pass: $pass, - pending: $pending, - fail: $fail, - cancel: $cancel, - skip: $skip, - unknown: $unknown, - unaccepted: $unaccepted, - missing_required: $missing_required - } -' - -# THE one guarded jq computation. Capture and IMMEDIATELY assert: jq exited 0 -# AND emitted a non-empty object with a boolean `.green`. Any failure (parse -# error, runtime crash, empty output, missing/non-boolean `.green`) is a hard -# config error — exit 2, NEVER green. This assertion is what makes it -# impossible for an emptied jq result to be read as a pass. -set +e -VERDICT="$(echo "$CHECKS_JSON" | jq -c "$JQ_VERDICT")" -verdict_rc=$? -set -e -if [ "$verdict_rc" -ne 0 ] || [ -z "${VERDICT//[[:space:]]/}" ] \ - || ! printf '%s' "$VERDICT" | jq -e 'type == "object" and (.green | type == "boolean")' >/dev/null 2>&1; then - echo "::error::ci-merge-gate: jq failed to compute a valid verdict over the input (parse/runtime error, empty output, or missing boolean .green) — cannot score checks, treating as config error" >&2 - exit 2 -fi - -# Extract scalars from the ALREADY-VALIDATED verdict object. These reads are -# safe: the object was asserted to be valid JSON with a boolean .green above, -# so a jq extraction here cannot silently produce a green-reading empty. -GREEN="$(printf '%s' "$VERDICT" | jq -r '.green')" -PASS_COUNT="$(printf '%s' "$VERDICT" | jq -r '.pass')" -SKIP_COUNT="$(printf '%s' "$VERDICT" | jq -r '.skip')" - -if [ "$GREEN" != "true" ]; then - # Emit every triggered reason, and — right after the unaccepted-checks reason - # and the missing-required reason — the per-line `- ` detail so the - # human-facing diagnostics match the previous multi-if output exactly. - while IFS= read -r reason; do - [ -z "$reason" ] && continue - echo "::error::ci-merge-gate: $reason" >&2 - case "$reason" in - "non-passing check(s) not required and not on IGNORE_CONTEXTS allow-list"*) - printf '%s' "$VERDICT" | jq -r '.unaccepted[]?' | while IFS= read -r line; do - [ -n "$line" ] && echo "::error:: - $line" >&2 - done - ;; - "required context(s) missing or not passing"*) - printf '%s' "$VERDICT" | jq -r '.missing_required[]?' | while IFS= read -r line; do - [ -n "$line" ] && echo "::error:: - $line" >&2 - done - ;; - esac - done < <(printf '%s' "$VERDICT" | jq -r '.reasons[]?') - exit 1 -fi - -echo "ci-merge-gate: GREEN — pass=$PASS_COUNT, pending=0, fail=0, cancel=0, skipping=$SKIP_COUNT (all allow-listed), all required contexts present and passing" -exit 0 diff --git a/scripts/fix-drift.ts b/scripts/fix-drift.ts index c712ba92..b6047d48 100644 --- a/scripts/fix-drift.ts +++ b/scripts/fix-drift.ts @@ -72,8 +72,6 @@ const KILL_GRACE_MS = 10_000; const VALID_SEVERITIES: ReadonlySet = new Set(["critical", "warning", "info"]); -const SKILL_FILE = "skills/write-fixtures/SKILL.md"; - /** * Map builder source files to the corresponding section names in the * write-fixtures skill documentation. Used to flag which skill sections @@ -333,14 +331,6 @@ export function buildPrompt(report: DriftReport): string { lines.push(""); } - lines.push("## Skill file update"); - lines.push(""); - lines.push("If any builder's output format changed (new fields, renamed fields, changed event"); - lines.push("types), update the write-fixtures skill documentation to match:"); - lines.push(` File: ${SKILL_FILE}`); - lines.push("Only update the Response Types and API Endpoints sections that correspond to the"); - lines.push("changed builders. Do not rewrite unrelated sections."); - lines.push(""); // Add AG-UI specific guidance if any AG-UI entries exist const hasAgUiDrift = report.entries.some((e) => e.provider === "AG-UI"); if (hasAgUiDrift) { @@ -674,9 +664,6 @@ export interface PostFixCollectorResult { * - `testFiles` — ONLY report-named fixture targets under src/__tests__/ * (any other test file would have BLOCKED at the gate, so * it must never be staged). - * - `skillFiles` — skills/ files (NOT allowlisted by the predicate; a - * RESOLVED verdict implies this is empty — kept for - * defensive completeness). * `stragglers` is every canonicalized changed file that is NOT in one of those * gated groups. On a RESOLVED verdict it MUST be empty (the predicate allowlist * already rejected any unclassified file); createPr never `git add`s it. The @@ -689,15 +676,13 @@ export function gatedCommitFiles( ): { builderFiles: string[]; testFiles: string[]; - skillFiles: string[]; stragglers: string[]; } { const builderFiles = changedFiles.filter(isProductionFile); const testFiles = changedFiles.filter((f) => f.startsWith("src/__tests__/") && sanctioned.has(f)); - const skillFiles = changedFiles.filter((f) => f.startsWith("skills/")); - const gated = new Set([...builderFiles, ...testFiles, ...skillFiles]); + const gated = new Set([...builderFiles, ...testFiles]); const stragglers = changedFiles.filter((f) => !gated.has(f)); - return { builderFiles, testFiles, skillFiles, stragglers }; + return { builderFiles, testFiles, stragglers }; } export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): void { @@ -796,10 +781,7 @@ export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): // an unnamed fixture, a config file — would have BLOCKED at the gate, so // sweeping it in AFTER the pass silently defeats the allowlist). const sanctioned = sanctionedTargets(report); - const { builderFiles, testFiles, skillFiles, stragglers } = gatedCommitFiles( - changedFiles, - sanctioned, - ); + const { builderFiles, testFiles, stragglers } = gatedCommitFiles(changedFiles, sanctioned); // FIX #F5 (round-4) — on a RESOLVED verdict `stragglers` MUST be empty: the // predicate allowlist already rejected any file that is neither production @@ -829,15 +811,6 @@ export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): execFileSafe("git", ["commit", "-m", "test: update SDK shapes for drift remediation"]); } - if (skillFiles.length > 0) { - execFileSafe("git", ["add", ...skillFiles]); - execFileSafe("git", [ - "commit", - "-m", - "docs: update write-fixtures skill for builder format changes", - ]); - } - // The version bump + CHANGELOG are an EXPLICIT, gated part of the fix set — a // release always accompanies an auto-remediation — not an unclassified // straggler. They are workflow-authored (never LLM-authored) and staged by an diff --git a/src/__tests__/ci-merge-gate.test.ts b/src/__tests__/ci-merge-gate.test.ts deleted file mode 100644 index 0e9a7d01..00000000 --- a/src/__tests__/ci-merge-gate.test.ts +++ /dev/null @@ -1,592 +0,0 @@ -import { describe, it, expect } from "vitest"; -import { spawnSync } from "node:child_process"; -import { readFileSync, writeFileSync, mkdtempSync, rmSync } from "node:fs"; -import { resolve, join } from "node:path"; -import { tmpdir } from "node:os"; - -// --------------------------------------------------------------------------- -// Tests for scripts/ci-merge-gate.sh — the auto-merge green-gate decision. -// -// EVERY case here INVOKES THE REAL scripts/ci-merge-gate.sh with fixture JSON -// and asserts its exit code / stderr. Nothing is asserted against a JS replica -// of the gate — so if the gate script were deleted or reverted to the old -// row-count logic, this suite FAILS (see the "reverting the gate" guard test). -// -// Exit-code contract of the gate: 0 = true-green (merge), 1 = not green -// (do not merge), 2 = usage / malformed-input / config error. -// --------------------------------------------------------------------------- - -const GATE = resolve(__dirname, "../../scripts/ci-merge-gate.sh"); - -type Check = { name: string; state: string; bucket?: string }; - -/** Run the real gate with a check array (or raw string) on stdin. */ -function runGate( - input: Check[] | string, - env: Record = {}, -): { code: number; stdout: string; stderr: string } { - const payload = typeof input === "string" ? input : JSON.stringify(input); - const r = spawnSync("bash", [GATE], { - input: payload, - encoding: "utf8", - env: { ...process.env, ...env }, - }); - return { code: r.status ?? -1, stdout: r.stdout ?? "", stderr: r.stderr ?? "" }; -} - -/** Run the real gate passing the JSON via a FILE ARGUMENT (not stdin). */ -function runGateWithFile( - input: Check[] | string, - env: Record = {}, -): { code: number; stdout: string; stderr: string } { - const payload = typeof input === "string" ? input : JSON.stringify(input); - const dir = mkdtempSync(join(tmpdir(), "gate-file-")); - try { - const file = join(dir, "checks.json"); - writeFileSync(file, payload); - const r = spawnSync("bash", [GATE, file], { - encoding: "utf8", - env: { ...process.env, ...env }, - }); - return { code: r.status ?? -1, stdout: r.stdout ?? "", stderr: r.stderr ?? "" }; - } finally { - rmSync(dir, { recursive: true, force: true }); - } -} - -/** Run a COPY of the gate whose source has been mutated, from a temp dir. */ -function runMutatedGate( - mutate: (src: string) => string, - input: Check[] | string, - env: Record = {}, -): { code: number; stdout: string; stderr: string } { - const payload = typeof input === "string" ? input : JSON.stringify(input); - const dir = mkdtempSync(join(tmpdir(), "gate-mut-")); - try { - const mutated = mutate(readFileSync(GATE, "utf8")); - const script = join(dir, "ci-merge-gate.sh"); - writeFileSync(script, mutated); - const r = spawnSync("bash", [script], { - input: payload, - encoding: "utf8", - env: { ...process.env, ...env }, - }); - return { code: r.status ?? -1, stdout: r.stdout ?? "", stderr: r.stderr ?? "" }; - } finally { - rmSync(dir, { recursive: true, force: true }); - } -} - -// The canonical required-context set a drift-fix PR must pass on this repo. -// This mirrors the branch's real gating checks. The regression test below -// asserts the gate script's DEFAULT_REQUIRED_CONTEXTS matches this list, so if -// the two drift (a gating check is added/removed in one place only) CI fails -// loudly here instead of silently merging an unverified PR in prod. -const CANONICAL_REQUIRED = [ - "prettier", - "eslint", - "exports", - "commitlint", - "test (20)", - "test (22)", - "test (24)", - "agui-schema-drift", - "drift-live-pr", - "zizmor", -]; - -// All-required-green shape (mirrors PR #305's passing checks). notify/drift are -// non-required extras that legitimately skip; they are on the gate's default -// IGNORE_CONTEXTS allow-list so they must not block. -const ALL_GREEN: Check[] = [ - ...CANONICAL_REQUIRED.map((name) => ({ name, state: "SUCCESS", bucket: "pass" })), - { name: "notify", state: "SKIPPED", bucket: "skipping" }, - { name: "drift", state: "SKIPPED", bucket: "skipping" }, -]; - -describe("ci-merge-gate.sh — refuses every false-green shape (real gate invoked)", () => { - it("empty array [] — REFUSES (exit 1)", () => { - const r = runGate([]); - expect(r.code).toBe(1); - // Tight reason: an empty array specifically has zero pass-bucket checks. - expect(r.stderr).toMatch(/no checks in 'pass' bucket/); - }); - - it("historical no-rows / empty-string input — REFUSES (exit 1)", () => { - // The old gate counted a "no checks reported" stdout LINE as a row and - // sailed through. The real gate treats empty/whitespace input as NOT green. - const r = runGate(" \n "); - expect(r.code).toBe(1); - // Tight reason: whitespace-only input is the empty-JSON path specifically. - expect(r.stderr).toMatch(/empty check JSON/); - }); - - it("pending-only — REFUSES (exit 1)", () => { - const r = runGate([{ name: "test (20)", state: "IN_PROGRESS", bucket: "pending" }]); - expect(r.code).toBe(1); - // Specific pending reason — not any stray occurrence of "pending". - expect(r.stderr).toMatch(/check\(s\) still pending\/queued\/in_progress — NOT green/); - }); - - it("skipped/neutral-only — REFUSES (exit 1), skips never count as pass", () => { - const r = runGate([ - { name: "prettier", state: "SKIPPED", bucket: "skipping" }, - { name: "eslint", state: "NEUTRAL", bucket: "skipping" }, - ]); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/no checks in 'pass' bucket/); - }); - - it("one unrelated pass, a required context missing — REFUSES (exit 1)", () => { - const r = runGate([{ name: "Continuous Releases", state: "SUCCESS", bucket: "pass" }]); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/required context\(s\) missing/); - }); - - it("a required context in the fail bucket — REFUSES (exit 1)", () => { - const checks = ALL_GREEN.map((c) => - c.name === "eslint" ? { ...c, state: "FAILURE", bucket: "fail" } : c, - ); - const r = runGate(checks); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/failed\/errored/); - }); - - it("a required context in the pending bucket — REFUSES (exit 1)", () => { - const checks = ALL_GREEN.map((c) => - c.name === "test (24)" ? { ...c, state: "IN_PROGRESS", bucket: "pending" } : c, - ); - const r = runGate(checks); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/check\(s\) still pending\/queued\/in_progress — NOT green/); - // A required context that is pending is ALSO missing-required (not passing). - expect(r.stderr).toMatch(/required context\(s\) missing or not passing/); - expect(r.stderr).toMatch(/- test \(24\)$/m); - }); - - it("a check in the cancel/stale bucket — REFUSES (exit 1)", () => { - const checks = [...ALL_GREEN, { name: "extra", state: "STALE", bucket: "cancel" }]; - const r = runGate(checks); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/cancelled\/stale/); - }); - - it("action_required (derived from state, no bucket field) — REFUSES (exit 1)", () => { - const r = runGate([{ name: "only-check", state: "ACTION_REQUIRED" }], { - REQUIRED_CONTEXTS: "only-check", - }); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/failed\/errored/); - }); - - // Exercise each derived-state leg INDIVIDUALLY (bucket field absent, so - // eff_bucket derives the bucket from raw .state). Each state maps to a - // specific effective bucket with a specific refusal reason; a single lumped - // assertion could pass on the wrong mapping, so we pin each leg to its exact - // reason. The single required "only-check" is present-and-passing, so the - // ONLY reason to refuse is the extra check's derived bucket. - const DERIVED_STATE_LEGS: Array<{ state: string; reason: RegExp }> = [ - { state: "ERROR", reason: /check\(s\) failed\/errored — NOT green/ }, - { state: "STARTUP_FAILURE", reason: /check\(s\) failed\/errored — NOT green/ }, - { state: "TIMED_OUT", reason: /check\(s\) failed\/errored — NOT green/ }, - { state: "CANCELED", reason: /check\(s\) cancelled\/stale — NOT green/ }, - { state: "NEUTRAL", reason: /not required and not on IGNORE_CONTEXTS/ }, - ]; - for (const { state, reason } of DERIVED_STATE_LEGS) { - it(`derived-state leg '${state}' (no bucket field) maps to its bucket and REFUSES (exit 1)`, () => { - const r = runGate( - [ - { name: "only-check", state: "SUCCESS", bucket: "pass" }, - { name: "extra", state }, - ], - { REQUIRED_CONTEXTS: "only-check" }, - ); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(reason); - }); - } - - it("a non-required skipped check NOT on the ignore-list — REFUSES (exit 1)", () => { - // Guards finding #1: a newly-added gating check that resolves skipped must - // NOT be silently ignored just because it is not (yet) in REQUIRED_CONTEXTS. - const r = runGate([ - { name: "only-check", state: "SUCCESS", bucket: "pass" }, - { name: "new-gating-check", state: "SKIPPED", bucket: "skipping" }, - ]); - // only-check is not in the default REQUIRED set → required missing too, - // but the salient assertion is the unaccepted-check refusal. - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/not required and not on IGNORE_CONTEXTS/); - }); - - it("an unknown-bucket check — REFUSES (exit 1), never silently dropped", () => { - // Guards finding #2: a check whose bucket/state spelling is unrecognized - // must be treated as NOT-pass and fail the gate. - const r = runGate( - [ - { name: "only-check", state: "SUCCESS", bucket: "pass" }, - { name: "weird", state: "WHAT", bucket: "mystery" }, - ], - { REQUIRED_CONTEXTS: "only-check" }, - ); - expect(r.code).toBe(1); - // An unknown-bucket check trips BOTH the unknown-count reason AND the sum - // mismatch (it does not land in any recognized bucket). Assert both specific - // reasons rather than an either-or match. - expect(r.stderr).toMatch(/check\(s\) in an unrecognized bucket\/state — NOT green/); - expect(r.stderr).toMatch(/bucket sum mismatch — recognized=\d+ total=\d+/); - }); - - it("malformed JSON (array of non-objects [1,2,3]) — exit 2, SPECIFICALLY the object-guard branch", () => { - // Guards finding #4: this used to slip past the type=="array" guard and - // crash jq with an undocumented exit 5. Assert the SPECIFIC object-guard - // message so this cannot pass on the field-type guard's "non-string .bucket - // or .state" message instead — the two guards catch different malformations - // and a test that matched either would not lock the object-guard branch. - const r = runGate("[1,2,3]"); - expect(r.code).toBe(2); - expect(r.stderr).toMatch(/check JSON array contains a non-object element/); - // NOT the field-type guard: [1,2,3] fails the object guard first. - expect(r.stderr).not.toMatch(/non-string \.bucket or \.state/); - }); - - it("malformed JSON (not an array) — exit 2 per contract", () => { - const r = runGate('{"name":"x"}'); - expect(r.code).toBe(2); - expect(r.stderr).toMatch(/not a JSON array/); - }); - - it("empty/whitespace REQUIRED_CONTEXTS — exit 2 config error, never a no-op green", () => { - // Guards finding #7: a gate with zero requirements must NOT merge. - const r = runGate([{ name: "x", state: "SUCCESS", bucket: "pass" }], { - REQUIRED_CONTEXTS: " ", - }); - expect(r.code).toBe(2); - expect(r.stderr).toMatch(/REQUIRED_CONTEXTS is empty/); - }); - - it("a REQUIRED context resolved to skipping — REFUSES (exit 1), skip never satisfies a requirement", () => { - const checks = ALL_GREEN.map((c) => - c.name === "zizmor" ? { ...c, state: "SKIPPED", bucket: "skipping" } : c, - ); - const r = runGate(checks); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/required context\(s\) missing or not passing/); - // The salient reason is the missing-required refusal, NOT a generic skip. - expect(r.stderr).toMatch(/- zizmor$/m); - }); - - it("a REQUIRED context in the cancel bucket — REFUSES (exit 1)", () => { - const checks = ALL_GREEN.map((c) => - c.name === "commitlint" ? { ...c, state: "CANCELLED", bucket: "cancel" } : c, - ); - const r = runGate(checks); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/cancelled\/stale/); - expect(r.stderr).toMatch(/required context\(s\) missing or not passing/); - }); - - it("duplicate same-name REQUIRED where one leg FAILS — REFUSES (exit 1)", () => { - // A required context with a passing leg AND a failing leg is NOT green: the - // failing leg must sink the gate even though the name is technically present - // in a pass bucket. - const checks = [...ALL_GREEN, { name: "eslint", state: "FAILURE", bucket: "fail" }]; - const r = runGate(checks); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/failed\/errored/); - }); - - it("empty-string state AND bucket → unknown (exit 1), never silently pass", () => { - // A blank state+bucket must resolve to the "unknown" sentinel and fail — - // never be dropped or defaulted to pass. - const r = runGate([{ name: "only-check", state: "", bucket: "" }], { - REQUIRED_CONTEXTS: "only-check", - }); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/check\(s\) in an unrecognized bucket\/state — NOT green/); - expect(r.stderr).toMatch(/bucket sum mismatch — recognized=\d+ total=\d+/); - }); - - it("non-string .bucket (object) with state SUCCESS — exit 2 config error (closes a false-GREEN, not a jq crash)", () => { - // CORRECTED RATIONALE: the coerce_str helper already maps a non-string - // .bucket to "" (no ascii_downcase crash / exit 5). The REAL hole this - // field-type guard closes is a FALSE-GREEN via the state fallback: a check - // with a non-string .bucket but state="SUCCESS" would, absent the guard, - // fall through coerce_str ("") into the state branch and resolve to the - // "pass" bucket — a malformed check silently scored as passing. The guard - // rejects it as the documented config error (exit 2) instead. The - // ISOLATED-VECTOR test below proves that flip directly. - const r = runGate('[{"name":"x","state":"SUCCESS","bucket":{"weird":true}}]', { - REQUIRED_CONTEXTS: "x", - }); - expect(r.code).toBe(2); - expect(r.stderr).toMatch(/non-string \.bucket or \.state/); - }); - - it("ISOLATED VECTOR: non-string .bucket→state-fallback false-green (guard removed → GREEN; present → refused)", () => { - // Prove the guard's VALUE, not just its presence: the same malformed input - // (non-string .bucket, state SUCCESS) flips from a false-GREEN to a refusal - // when the field-type guard is present. Mutating the guard out demonstrates - // the exact false-green the guard closes. - const payload = '[{"name":"x","state":"SUCCESS","bucket":{"weird":true}}]'; - // Guard REMOVED: the non-string bucket coerces to "" → state fallback → - // "pass" bucket → the gate would MERGE this malformed check (false-green). - const withoutGuard = runMutatedGate( - (src) => - src.replace( - /# Field-type guard:[\s\S]*?exit 2\nfi\n/, - "# field-type guard removed for this test\n", - ), - payload, - { REQUIRED_CONTEXTS: "x" }, - ); - expect(withoutGuard.code).toBe(0); - expect(withoutGuard.stdout).toMatch(/GREEN/); - // Guard PRESENT (real gate): the malformed check is refused as exit 2. - const withGuard = runGate(payload, { REQUIRED_CONTEXTS: "x" }); - expect(withGuard.code).toBe(2); - expect(withGuard.stderr).toMatch(/non-string \.bucket or \.state/); - }); - - it("non-string .state (number) — exit 2 config error (malformed check data, not a jq crash)", () => { - // Same field-type guard on .state: a numeric state is malformed check data - // from gh and must fail closed as the documented config error, not be - // coerced and scored. - const r = runGate('[{"name":"x","state":123}]', { REQUIRED_CONTEXTS: "x" }); - expect(r.code).toBe(2); - expect(r.stderr).toMatch(/non-string \.bucket or \.state/); - }); - - it("contradictory config: a name in BOTH required and ignore — exit 2 config error", () => { - // Guards finding #5: a required context can never be "safe to skip". The - // gate fails closed rather than silently resolving the contradiction. - const r = runGate([{ name: "x", state: "SUCCESS", bucket: "pass" }], { - REQUIRED_CONTEXTS: "x,dup", - IGNORE_CONTEXTS: "dup", - }); - expect(r.code).toBe(2); - expect(r.stderr).toMatch(/BOTH REQUIRED_CONTEXTS and IGNORE_CONTEXTS/); - }); - - it("null .name in the pass bucket does NOT satisfy a required context — REFUSES (exit 1)", () => { - // Guards finding #5: a nameless pass check counts toward pass>=1 but must - // not be able to resolve a named required context. - const r = runGate('[{"name":null,"state":"SUCCESS","bucket":"pass"}]', { - REQUIRED_CONTEXTS: "realreq", - }); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/required context\(s\) missing or not passing/); - expect(r.stderr).toMatch(/- realreq$/m); - }); -}); - -describe("ci-merge-gate.sh — accepts genuine true-green (real gate invoked)", () => { - it("all-required-green (extras skipped + allow-listed) — ACCEPTS (exit 0)", () => { - const r = runGate(ALL_GREEN); - expect(r.code).toBe(0); - expect(r.stdout).toMatch(/GREEN/); - }); - - it("duplicate/mixed same-name required context (one pass) — ACCEPTS (exit 0)", () => { - // A required context appearing twice (e.g. re-run) where at least one leg - // passes and none fail/pend is satisfied. - const checks = [ - ...ALL_GREEN, - { name: "eslint", state: "SUCCESS", bucket: "pass" }, // duplicate pass - ]; - const r = runGate(checks); - expect(r.code).toBe(0); - }); - - it("duplicate same-name required with one leg still pending — REFUSES (exit 1)", () => { - const checks = [ - ...ALL_GREEN, - { name: "eslint", state: "IN_PROGRESS", bucket: "pending" }, // duplicate pending - ]; - const r = runGate(checks); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/check\(s\) still pending\/queued\/in_progress — NOT green/); - }); - - it("honors REQUIRED_CONTEXTS override (comma-separated)", () => { - const r = runGate([{ name: "only-check", state: "SUCCESS", bucket: "pass" }], { - REQUIRED_CONTEXTS: "only-check", - }); - expect(r.code).toBe(0); - }); - - it("honors IGNORE_CONTEXTS override for a non-required skipped check", () => { - const r = runGate( - [ - { name: "only-check", state: "SUCCESS", bucket: "pass" }, - { name: "optional-skip", state: "SKIPPED", bucket: "skipping" }, - ], - { REQUIRED_CONTEXTS: "only-check", IGNORE_CONTEXTS: "optional-skip" }, - ); - expect(r.code).toBe(0); - }); - - it("derives bucket from raw state when the bucket field is absent", () => { - const checks = ALL_GREEN.filter((c) => c.bucket === "pass").map(({ name, state }) => ({ - name, - state, - })); - const r = runGate(checks as Check[]); - expect(r.code).toBe(0); - }); -}); - -describe("ci-merge-gate.sh — file-argument input path (real gate invoked)", () => { - it("reads JSON from a FILE ARG and accepts true-green (exit 0)", () => { - const r = runGateWithFile(ALL_GREEN); - expect(r.code).toBe(0); - expect(r.stdout).toMatch(/GREEN/); - }); - - it("reads JSON from a FILE ARG and refuses a false-green (exit 1)", () => { - const r = runGateWithFile([{ name: "test (20)", state: "IN_PROGRESS", bucket: "pending" }]); - expect(r.code).toBe(1); - expect(r.stderr).toMatch(/check\(s\) still pending\/queued\/in_progress — NOT green/); - }); - - it("file-not-found → exit 2 config error", () => { - const r = spawnSync("bash", [GATE, "/nonexistent/path/does-not-exist.json"], { - encoding: "utf8", - env: { ...process.env }, - }); - expect(r.status).toBe(2); - expect(r.stderr).toMatch(/input file not found/); - }); -}); - -describe("ci-merge-gate.sh — drift + revert guards", () => { - it("gate script's DEFAULT_REQUIRED_CONTEXTS matches the canonical required set", () => { - // Finding #1(b): catch REQUIRED_CONTEXTS drifting from the repo's real - // required checks in CI, not in prod. If a gating check is added/removed in - // only one place, this fails loudly. - const src = readFileSync(GATE, "utf8"); - const m = src.match(/DEFAULT_REQUIRED_CONTEXTS='([^']*)'/); - expect(m, "DEFAULT_REQUIRED_CONTEXTS assignment not found in gate script").toBeTruthy(); - const scriptList = m![1] - .split("\n") - .map((s) => s.trim()) - .filter(Boolean); - expect(scriptList).toEqual(CANONICAL_REQUIRED); - }); - - it("gate script's DEFAULT_IGNORE_CONTEXTS is exactly the tight reviewed set (widening fails here)", () => { - // Drift-guard on the DANGEROUS direction: widening IGNORE_CONTEXTS silently - // tolerates more non-passing checks. Pin it to the exact reviewed set so any - // addition to the allow-list must be a conscious, test-updating change. - const src = readFileSync(GATE, "utf8"); - const m = src.match(/DEFAULT_IGNORE_CONTEXTS='([^']*)'/); - expect(m, "DEFAULT_IGNORE_CONTEXTS assignment not found in gate script").toBeTruthy(); - const scriptList = m![1] - .split("\n") - .map((s) => s.trim()) - .filter(Boolean); - expect(scriptList).toEqual(["notify", "drift"]); - }); - - it("CONSOLIDATED-VERDICT GUARD: a runtime jq failure in the verdict program exits 2, never masks to green", () => { - // Structural fix (single guarded verdict): the entire decision is computed - // by ONE jq program whose output is validated ONCE before the shell reads - // any field. Mutate a shared helper (coerce_str) so eff_bucket — and thus - // the whole verdict program — throws at runtime, and assert the gate fails - // CLOSED with the documented config-error exit 2 (the verdict validity - // assertion), NOT a scored 0/1 decision, not a false-green, not exit 5. - const r = runMutatedGate( - (src) => src.replace(/def coerce_str:[^\n]*\n/, "def coerce_str: (undefined_fn);\n"), - [{ name: "x", state: "SUCCESS", bucket: "pass" }], - { REQUIRED_CONTEXTS: "x" }, - ); - expect(r.code).toBe(2); - // The SPECIFIC consolidated-verdict assertion message — not a generic match. - expect(r.stderr).toMatch(/jq failed to compute a valid verdict over the input/); - }); - - it("CONSOLIDATED-VERDICT GUARD: an EMPTY verdict (jq emits nothing) exits 2, never reads as green", () => { - // This is the crux of the recurring class: `set -e` does NOT guard a - // command-substitution RHS, so a crashed jq yields an EMPTY VERDICT. The - // old scattered `UNACCEPTED_CHECKS`/`MISSING_REQUIRED` assignments read that - // empty as "no unaccepted / no missing" = GREEN. Force the single verdict - // capture to emit nothing and confirm the validity assertion rejects it as - // exit 2 — an empty verdict can NEVER be interpreted as a pass. - const r = runMutatedGate( - (src) => - src.replace( - /VERDICT="\$\(echo "\$CHECKS_JSON" \| jq -c "\$JQ_VERDICT"\)"/, - 'VERDICT="$(printf %s "")"', - ), - [{ name: "x", state: "SUCCESS", bucket: "pass" }], - { REQUIRED_CONTEXTS: "x" }, - ); - expect(r.code).toBe(2); - expect(r.stderr).toMatch(/jq failed to compute a valid verdict over the input/); - }); - - it("CONSOLIDATED-VERDICT GUARD: a verdict object with a non-boolean .green exits 2, never green", () => { - // The validity assertion requires .green to be a BOOLEAN. A verdict whose - // .green somehow becomes a non-boolean (e.g. a string) must fail closed as - // exit 2 rather than let `[ "$GREEN" != "true" ]` misfire. Mutate the - // verdict program to emit green as a string and confirm exit 2. - const r = runMutatedGate( - (src) => src.replace(/green: \(\(\$reasons \| length\) == 0\),/, 'green: "yes",'), - [{ name: "x", state: "SUCCESS", bucket: "pass" }], - { REQUIRED_CONTEXTS: "x" }, - ); - expect(r.code).toBe(2); - expect(r.stderr).toMatch(/jq failed to compute a valid verdict over the input/); - }); - - it("STRUCTURAL INVARIANT: no bare `VAR=$(jq ...)` whose emptiness could read as green", () => { - // The recurring bug class is a scattered `VAR="$(echo "$CHECKS_JSON" | jq - // ...)"` whose crash-emptied output is later interpreted as a green signal. - // Lock the invariant structurally: the ONLY jq invocation that consumes - // $CHECKS_JSON to score checks is the single guarded $JQ_VERDICT capture. - // No `$CHECKS_JSON | jq` assignment may exist outside that one line. - const src = readFileSync(GATE, "utf8"); - const checksJsonJqAssignments = src - .split("\n") - .filter((l) => !/^\s*#/.test(l)) // ignore comment lines - .filter((l) => /=\s*"?\$\(\s*echo "\$CHECKS_JSON" \| jq/.test(l)); - // Exactly one: the guarded VERDICT capture. - expect(checksJsonJqAssignments.length).toBe(1); - expect(checksJsonJqAssignments[0]).toMatch( - /VERDICT="\$\(echo "\$CHECKS_JSON" \| jq -c "\$JQ_VERDICT"\)"/, - ); - }); - - it("REVERT GUARD: the old row-count logic would NOT pass this suite", () => { - // Prove the suite is anchored to the real gate, not a replica: model the - // OLD gate (merge iff row-count>0 AND nothing in fail/cancel) and assert it - // gives the WRONG answer on false-green shapes the real gate refuses. If - // someone reverted ci-merge-gate.sh to the old logic, the false-green cases - // above would flip to exit 0 and this expectation documents why they fail. - const oldGateWouldMerge = (checks: Check[]): boolean => { - const rowCount = checks.length === 0 ? 1 : checks.length; - if (rowCount === 0) return false; - const watchFails = checks.some((c) => c.bucket === "fail" || c.bucket === "cancel"); - return !watchFails; - }; - // Empty, pending-only, skipped-only, unrelated-pass, skipped-required: old - // logic MERGES (bug) on all of these. - expect(oldGateWouldMerge([])).toBe(true); - expect(oldGateWouldMerge([{ name: "t", state: "IN_PROGRESS", bucket: "pending" }])).toBe(true); - expect(oldGateWouldMerge([{ name: "t", state: "SKIPPED", bucket: "skipping" }])).toBe(true); - // An UNRELATED pass with a required missing: old row-count logic sees >0 rows - // and no fail/cancel → MERGES (bug); the real gate refuses (required missing). - const unrelatedPass = [{ name: "Continuous Releases", state: "SUCCESS", bucket: "pass" }]; - expect(oldGateWouldMerge(unrelatedPass)).toBe(true); - // A skipped-ONLY (no pass at all) set: old logic MERGES (bug); real gate - // refuses (no pass bucket). - const skippedOnly = [{ name: "t", state: "SKIPPED", bucket: "skipping" }]; - expect(oldGateWouldMerge(skippedOnly)).toBe(true); - // The REAL gate refuses ALL of those, so a revert to old logic flips these - // exit codes from 1 → 0 and breaks CI on the broadened cases too. - expect(runGate([]).code).toBe(1); - expect(runGate([{ name: "t", state: "IN_PROGRESS", bucket: "pending" }]).code).toBe(1); - expect(runGate(unrelatedPass).code).toBe(1); - expect(runGate(skippedOnly).code).toBe(1); - }); -}); diff --git a/src/__tests__/fix-drift.test.ts b/src/__tests__/fix-drift.test.ts index 83dc10b3..297d770a 100644 --- a/src/__tests__/fix-drift.test.ts +++ b/src/__tests__/fix-drift.test.ts @@ -1151,15 +1151,3 @@ describe("buildPrBody — skill sections", () => { expect(body).toContain("- Responses API"); }); }); - -// --------------------------------------------------------------------------- -// buildPrompt — skill file reference -// --------------------------------------------------------------------------- - -describe("buildPrompt — skill file", () => { - it("includes skill file update instructions", () => { - const prompt = buildPrompt(makeReport()); - expect(prompt).toContain("## Skill file update"); - expect(prompt).toContain("skills/write-fixtures/SKILL.md"); - }); -});