From a4b633770099e313c522c991777b0aaecc0dc09d Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Fri, 17 Jul 2026 17:27:09 -0700 Subject: [PATCH 1/2] fix(drift): process-group kill, fail-closed version bump, early-infra alerting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Harden three drift-remediation tail workstreams as one PR (WS-4/6/8), each with a red-green regression lock (WS-10). WS-4 — real process-group subprocess control. invokeClaudeCode spawned npx with no process group and gated SIGKILL escalation on `if (!child.killed)`, which is ~always false right after SIGTERM delivery, so SIGKILL never fired and killing the npx wrapper never reached the claude-code grandchild — a wedged fixer burned the full 30-min budget. Now spawn with `detached: true` and, on timeout, SIGTERM then (after a grace) SIGKILL the whole GROUP via `process.kill(-pid, …)`, gated on a REAL has-exited flag from the `close` event. Extracted killProcessGroup + scheduleEscalatingKill (ESRCH/EPERM tolerant) so the escalation is unit-testable against a real wedged child. WS-8 — version-bump failure must fail closed. createPr caught a version bump/CHANGELOG failure, warned "skipping", and shipped an unversioned PR that never publishes a release (silent value loss). Now a bump failure is a hard error: exit with a new VERSION_BUMP_FAILED reason (exit 18) routed to the workflow's human-review alert; no push, no PR. patchBumpVersion's own throw is unchanged — the caller now propagates it. WS-6 — early-infra catch-all alert. The four specific Slack alerts are all gated on step outputs that only exist after the collector ran, so an early failure (checkout / mint-app-token / pnpm install / ag-ui clone / git config) died red with zero Slack signal. Added an end-of-job `if: failure()` catch-all that fires only when none of the specific alerts did (early-infra window), distinguishing infra/setup from drift-fix failure. Tightened the autofix-step alert to require `check` to have run so it no longer misfires on that window. The Create-PR step now captures the script's exit code + `reason=` so a fail-closed exit (e.g. version-bump-failed) is named in the failure alert. WS-10 — regression locks: a real SIGTERM-trapping subprocess killed within grace (old logic leaves it alive, fixed logic kills it and the group); a createPr fail-closed test (git mocked so the bump commit throws → exit 18, no PR); workflow static assertions for the catch-all's presence + unconditional `if: failure()` gating + the version-bump-failed reason wiring. Deferred: standalone actionlint-as-CI; the LOW WS-3 parsePorcelainLine C-unescape (not a trivial 1-3 line change across the two mirrored copies). --- .github/workflows/fix-drift.yml | 67 ++++++++- scripts/drift-success-predicate.ts | 15 ++ scripts/fix-drift.ts | 105 ++++++++++++-- src/__tests__/fix-drift-kill.test.ts | 171 +++++++++++++++++++++++ src/__tests__/fix-drift-workflow.test.ts | 74 ++++++++++ src/__tests__/fix-drift.test.ts | 112 +++++++++++++++ 6 files changed, 532 insertions(+), 12 deletions(-) create mode 100644 src/__tests__/fix-drift-kill.test.ts diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index 4a955573..1d4160ab 100644 --- a/.github/workflows/fix-drift.yml +++ b/.github/workflows/fix-drift.yml @@ -301,6 +301,7 @@ jobs: 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 + CREATE_PR_LOG: ${{ runner.temp }}/create-pr.log run: | set -euo pipefail # Defense-in-depth: the workflow already asserted the predicate in the @@ -315,10 +316,24 @@ jobs: # 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). + # Capture the script's own exit code + emitted `reason=` line so a + # fail-closed exit (e.g. version-bump-failed) is NAMED in the failure + # alert rather than reported blank. Write the log OUTSIDE the repo + # checkout ($RUNNER_TEMP) so it is never scored by any git scan. + set +e npx tsx scripts/fix-drift.ts --create-pr \ --report "${PINNED_REPORT}" \ --post-fix-report "${POST_FIX_REPORT}" \ - --post-fix-exit "${POST_FIX_EXIT}" + --post-fix-exit "${POST_FIX_EXIT}" 2>&1 | tee "${CREATE_PR_LOG}" + PR_EXIT=${PIPESTATUS[0]} + set -e + if [ "$PR_EXIT" -ne 0 ]; then + PR_REASON="$(grep '^reason=' "${CREATE_PR_LOG}" | tail -n1)" + PR_REASON="${PR_REASON#reason=}" + echo "::error::Create-PR step failed (exit ${PR_EXIT}, reason=${PR_REASON:-unknown})" + echo "reason=${PR_REASON}" >> "$GITHUB_OUTPUT" + exit "$PR_EXIT" + fi HEAD_SHA="$(git rev-parse HEAD)" echo "Pushed head SHA: $HEAD_SHA" # Find the open PR whose headRefOid matches the exact SHA we pushed. @@ -401,9 +416,14 @@ jobs: # `continue-on-error: true`, on failure the job would otherwise sail on # green with no signal. Detect outcome != 'success' (and not the skip # case), alert distinctly, then fail the job. + # NOTE: also require the `check` step to have RUN (skip output is one of + # 'true'/'false', never empty). Without that guard this step would ALSO + # fire on an EARLY-infra failure (checkout/token/pnpm/clone/git-config — + # all before `detect`/`check`), mislabelling it "auto-fix STEP failed" + # when the fixer never ran. The end-of-job catch-all owns that window. - name: Alert on autofix step failure if: >- - always() && steps.check.outputs.skip != 'true' && + always() && steps.check.outputs.skip == 'false' && steps.autofix.outcome != 'success' && steps.detect.outputs.collector_crashed != 'true' env: @@ -505,6 +525,7 @@ jobs: 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)";; + version-bump-failed) DETAIL=" (NEEDS HUMAN: the version bump / CHANGELOG step failed — refused to open an UNVERSIONED PR that would merge a fix which never publishes a release. See run logs)";; config-error) DETAIL=" (drift-success predicate CONFIG error — missing/unreadable report or bad args; the gate itself needs attention)";; *) DETAIL="";; esac @@ -517,3 +538,45 @@ jobs: curl -fsS --show-error -X POST "$SLACK_WEBHOOK" \ -H "Content-Type: application/json" \ -d "$PAYLOAD" + + # End-of-job CATCH-ALL for ANY job failure the specific alerts above did + # NOT cover — chiefly the EARLY-infra window (checkout / mint-app-token / + # pnpm install / clone ag-ui / git config) that runs BEFORE `detect`/ + # `check` set their outputs. Those early steps leave collector_crashed, + # quarantine, and check.outputs.skip all EMPTY, so none of the four + # specific alerts fire and the job would otherwise die red with ZERO Slack + # signal. This step is UNCONDITIONAL on the earlier step outputs (only + # `failure()` + the anti-double-alert guard below), so it fires whenever + # nothing else did. + # + # Anti-double-alert: the four specific alerts fire iff collector_crashed + # or quarantine is set, OR the fixer actually ran (check.outputs.skip is + # 'true'/'false' — never empty — once `check` executed). So this catch-all + # only fires when NONE of those hold: collector_crashed != 'true' AND + # quarantine != 'true' AND check.outputs.skip is empty (check never ran). + # That is exactly the early-infra window, and it never overlaps with the + # specific alerts. + - name: Alert on early-infra failure (catch-all) + if: >- + failure() && steps.detect.outputs.collector_crashed != 'true' && + steps.check.outputs.quarantine != 'true' && + steps.check.outputs.skip == '' + env: + SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} + REPO: ${{ github.repository }} + RUN_ID: ${{ github.run_id }} + run: | + set -euo pipefail + # This is an INFRA/SETUP failure — the fixer never ran (checkout / + # token mint / pnpm install / ag-ui clone / git config failed), so it + # is distinct from a drift-fix failure. A missing webhook must NOT + # swallow it: emit a VISIBLE ::error:: and fail the step. + if [ -z "${SLACK_WEBHOOK:-}" ]; then + echo "::error::Drift-fix job failed during INFRA/SETUP (before the collector ran) AND SLACK_WEBHOOK is not set — no alert could be sent. See run https://github.com/${REPO}/actions/runs/${RUN_ID}" + exit 1 + fi + MSG="🧱❌ *Drift-fix job — INFRA/SETUP failure* — the job failed during setup (checkout / token mint / pnpm install / ag-ui clone / git config) BEFORE the drift collector ran; this is not a drift-fix failure. Manual intervention needed.\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" diff --git a/scripts/drift-success-predicate.ts b/scripts/drift-success-predicate.ts index d6ec958c..5999a646 100644 --- a/scripts/drift-success-predicate.ts +++ b/scripts/drift-success-predicate.ts @@ -80,6 +80,11 @@ * non-drift or drift `*.test.ts`; a real fix * touches only production source + report- * named fixture targets) + * 18 — VERSION_BUMP_FAILED (NOT scored by the predicate — surfaced by + * fix-drift.ts's createPr when the mandatory + * version bump / CHANGELOG step fails, so an + * unversioned fix that never publishes is + * never opened as a PR; fail-closed to human) * 2 — CONFIG_ERROR (missing/unreadable report, bad args, a * --changed-file list that disagrees w/ git, * a path escaping the repo root, or a @@ -122,6 +127,15 @@ export enum PredicateReason { COLLECTOR_INFRA = "collector-infra", PRODUCTION_CHANGE_OFF_TARGET = "production-change-off-target", CONFIG_ERROR = "config-error", + /** + * The version bump / CHANGELOG step failed while opening a drift-fix PR. A + * release ALWAYS accompanies an auto-remediation; without it the PR would + * merge a fix that never publishes (silent value loss). This is a hard, + * fail-closed reason surfaced by fix-drift.ts's createPr — never produced by + * the predicate's own scoring — routed to human review like any other + * needs-human reason. + */ + VERSION_BUMP_FAILED = "version-bump-failed", } /** Stable exit code for each reason (see module header). */ @@ -136,6 +150,7 @@ export const REASON_EXIT_CODE: Record = { [PredicateReason.COLLECTOR_INFRA]: 15, [PredicateReason.PRODUCTION_CHANGE_OFF_TARGET]: 16, [PredicateReason.CONFIG_ERROR]: 2, + [PredicateReason.VERSION_BUMP_FAILED]: 18, }; export interface PredicateInputs { diff --git a/scripts/fix-drift.ts b/scripts/fix-drift.ts index b6047d48..7f66c3a1 100644 --- a/scripts/fix-drift.ts +++ b/scripts/fix-drift.ts @@ -364,6 +364,66 @@ export function buildPrompt(report: DriftReport): string { // Claude Code invocation (default mode) // --------------------------------------------------------------------------- +/** + * Kill an entire process GROUP by its leader pid. + * + * `spawn(..., { detached: true })` makes the child a group leader whose group + * id equals its pid, so `process.kill(-pid, signal)` signals the child AND all + * of its descendants (e.g. the `npx` wrapper's `@anthropic-ai/claude-code` + * grandchild). Signalling the child pid alone (`child.kill()`) reaches only the + * `npx` wrapper and leaves a wedged grandchild alive to burn the job budget. + * + * Swallows ESRCH (group already gone) and EPERM (nothing left we own to + * signal) — both mean "there is nothing more to kill", which is success for a + * best-effort teardown. Returns true if a signal was delivered, false if the + * group was already gone / unkillable. + */ +export function killProcessGroup(pid: number, signal: NodeJS.Signals): boolean { + try { + // Negative pid targets the whole process group led by `pid`. + process.kill(-pid, signal); + return true; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code === "ESRCH" || code === "EPERM") { + // ESRCH: the group has already fully exited. EPERM: nothing we still own + // to signal in that group. Either way, nothing left to kill. + return false; + } + throw err; + } +} + +/** + * Escalating timeout kill for a detached subprocess: deliver SIGTERM to the + * whole GROUP, then after a grace period escalate to SIGKILL on the GROUP — + * but ONLY if the process has NOT already exited. + * + * The has-exited signal is the caller-supplied `hasExited()` predicate, which + * MUST be backed by the real `close` event (not `child.killed`). Node sets + * `child.killed = true` the instant a signal is DELIVERED, long before the + * process actually exits, so a `!child.killed` guard makes the SIGKILL + * escalation dead code — the original WS-4 defect. Gating on a real exit flag + * is what makes the escalation actually fire against a process that ignores + * SIGTERM. + * + * Returns the grace timer so the caller can cancel it from its `close` handler + * (a clean early exit must not leave a pending SIGKILL escalation queued). + */ +export function scheduleEscalatingKill( + pid: number, + hasExited: () => boolean, + graceMs: number = KILL_GRACE_MS, +): NodeJS.Timeout { + killProcessGroup(pid, "SIGTERM"); + return setTimeout(() => { + if (!hasExited()) { + console.error("Process group did not exit after SIGTERM. Sending SIGKILL to the group..."); + killProcessGroup(pid, "SIGKILL"); + } + }, graceMs); +} + function invokeClaudeCode(prompt: string): Promise { return new Promise((done, reject) => { const args = [ @@ -392,26 +452,35 @@ function invokeClaudeCode(prompt: string): Promise { "50", ]; + // `detached: true` puts the child in its OWN process group (gpid === pid), + // so a timeout can signal the WHOLE group — the `npx` wrapper AND its + // `@anthropic-ai/claude-code` grandchild — via `process.kill(-pid, …)`. + // Without it, killing the child pid reaches only the wrapper and a wedged + // grandchild survives to burn the 30-min job budget. const child = spawn("npx", args, { stdio: ["inherit", "pipe", "pipe"], + detached: true, }); const logChunks: Buffer[] = []; let killGraceTimer: NodeJS.Timeout | undefined; let timedOut = false; + // REAL has-exited flag, flipped by the `close` handler. The SIGKILL + // escalation is gated on THIS, never `child.killed` (which is true the + // instant SIGTERM is delivered, making the escalation dead code). + let exited = false; const killTimer = setTimeout(() => { timedOut = true; console.error( - `Claude Code timed out after ${CLAUDE_TIMEOUT_MS / 60000} minutes. Sending SIGTERM...`, + `Claude Code timed out after ${CLAUDE_TIMEOUT_MS / 60000} minutes. ` + + "Sending SIGTERM to the process group...", ); - child.kill("SIGTERM"); - killGraceTimer = setTimeout(() => { - if (!child.killed) { - console.error("Process did not exit after SIGTERM. Sending SIGKILL..."); - child.kill("SIGKILL"); - } - }, KILL_GRACE_MS); + // child.pid can be undefined if the spawn failed; the `error` handler + // covers that path, so only escalate when we have a real group leader. + if (typeof child.pid === "number") { + killGraceTimer = scheduleEscalatingKill(child.pid, () => exited); + } }, CLAUDE_TIMEOUT_MS); child.on("error", (err) => { @@ -439,6 +508,9 @@ function invokeClaudeCode(prompt: string): Promise { }); child.on("close", (code, signal) => { + // Mark real exit BEFORE clearing timers so any in-flight grace timer that + // fires in the same tick sees `exited === true` and skips the SIGKILL. + exited = true; clearTimeout(killTimer); if (killGraceTimer) clearTimeout(killGraceTimer); const logContent = Buffer.concat(logChunks).toString("utf-8"); @@ -815,6 +887,14 @@ export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): // 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. + // + // WS-8 — this step is MANDATORY, so a failure here is a HARD, fail-closed + // error: opening an UNVERSIONED PR would ship a "fix" that a human might merge + // but which never publishes a release, silently delivering no value. We exit + // with a distinct VERSION_BUMP_FAILED reason (routed to the workflow's + // human-review alert) rather than warn-and-continue. No push has happened yet, + // and the builder/test commits above are local-only until the push below — + // which we never reach — so no partial/unversioned PR is ever opened. try { const newVersion = patchBumpVersion(); console.log(`Bumped version to ${newVersion}`); @@ -826,8 +906,13 @@ export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): execFileSafe("git", ["add", "package.json", "CHANGELOG.md"]); execFileSafe("git", ["commit", "-m", `chore: bump version to ${newVersion}`, "--allow-empty"]); } catch (err) { - console.warn("Version bump failed, skipping:", err); - // Continue with PR creation without version bump + console.error( + "ERROR: version bump / CHANGELOG step failed — refusing to open an UNVERSIONED " + + "drift-fix PR that would merge a fix which never publishes a release:", + err instanceof Error ? err.message : err, + ); + console.log(`reason=${PredicateReason.VERSION_BUMP_FAILED}`); + process.exit(REASON_EXIT_CODE[PredicateReason.VERSION_BUMP_FAILED]); } execFileSafe("git", ["push", "-u", "origin", branchName]); diff --git a/src/__tests__/fix-drift-kill.test.ts b/src/__tests__/fix-drift-kill.test.ts new file mode 100644 index 00000000..dfc21eaa --- /dev/null +++ b/src/__tests__/fix-drift-kill.test.ts @@ -0,0 +1,171 @@ +/** + * WS-4 regression locks — real process-group subprocess control. + * + * The original `invokeClaudeCode` had two live defects: + * 1. `spawn("npx", …)` had NO `detached: true`, so signalling the child pid + * reached only the `npx` wrapper, never the `@anthropic-ai/claude-code` + * grandchild — a wedged fixer survived and burned the 30-min job budget. + * 2. The SIGKILL escalation was gated on `if (!child.killed)`, but Node sets + * `child.killed = true` the instant SIGTERM is DELIVERED (not when the + * process exits), so `!child.killed` was ~always false and SIGKILL NEVER + * fired against a process that ignored SIGTERM. + * + * These tests exercise a REAL controlled subprocess that traps SIGTERM and + * sleeps, proving the OLD logic leaves it alive and the NEW logic + * (`killProcessGroup` + `scheduleEscalatingKill`, gated on a real has-exited + * flag) kills it and its whole group within the grace window. + */ +import { spawn } from "node:child_process"; + +import { describe, it, expect } from "vitest"; + +/** Real subprocess spin-up + grace windows need more than the default budget. */ +const SUBPROC_TIMEOUT = 15000; + +import { killProcessGroup, scheduleEscalatingKill } from "../../scripts/fix-drift.js"; + +/** A child that TRAPS SIGTERM and keeps sleeping — models a wedged fixer. */ +const WEDGED_CHILD = ` +process.on("SIGTERM", () => { /* ignore — wedged, refuse to die on SIGTERM */ }); +setTimeout(() => process.exit(0), 60000); +`; + +/** A child that exits cleanly on SIGTERM — models a well-behaved fixer. */ +const OBEDIENT_CHILD = ` +process.on("SIGTERM", () => process.exit(0)); +setTimeout(() => process.exit(0), 60000); +`; + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function sleep(ms: number): Promise { + return new Promise((r) => setTimeout(r, ms)); +} + +describe("killProcessGroup", () => { + it( + "delivers a signal to the whole GROUP of a detached child (kills a SIGTERM-trapping grandchild-style process)", + { timeout: SUBPROC_TIMEOUT }, + async () => { + const child = spawn("node", ["-e", WEDGED_CHILD], { stdio: "ignore", detached: true }); + const pid = child.pid!; + await sleep(300); + expect(isAlive(pid)).toBe(true); + + // SIGTERM to the group is IGNORED by the wedged child (it traps it). + expect(killProcessGroup(pid, "SIGTERM")).toBe(true); + await sleep(200); + expect(isAlive(pid)).toBe(true); // still alive — it trapped SIGTERM + + // SIGKILL to the GROUP cannot be trapped — it dies. + expect(killProcessGroup(pid, "SIGKILL")).toBe(true); + await sleep(400); + expect(isAlive(pid)).toBe(false); + }, + ); + + it("tolerates ESRCH (group already gone) and returns false, never throwing", () => { + // A pid that is essentially certain not to exist as a group leader. + const missing = 2 ** 30; + expect(() => killProcessGroup(missing, "SIGTERM")).not.toThrow(); + expect(killProcessGroup(missing, "SIGTERM")).toBe(false); + }); + + it("re-throws unexpected errors (e.g. EINVAL from a bad signal)", () => { + const child = spawn("node", ["-e", OBEDIENT_CHILD], { stdio: "ignore", detached: true }); + const pid = child.pid!; + try { + // An invalid signal name yields a non-ESRCH/EPERM error, which must + // propagate rather than be swallowed as "nothing to kill". + expect(() => killProcessGroup(pid, "SIGNOTAREALSIGNAL" as NodeJS.Signals)).toThrow(); + } finally { + try { + process.kill(-pid, "SIGKILL"); + } catch { + /* best effort */ + } + } + }); +}); + +describe("scheduleEscalatingKill — SIGKILL escalation gated on a REAL exit flag", () => { + it( + "SIGKILLs a wedged (SIGTERM-trapping) subprocess group within the grace window (GREEN)", + { timeout: SUBPROC_TIMEOUT }, + async () => { + const child = spawn("node", ["-e", WEDGED_CHILD], { stdio: "ignore", detached: true }); + const pid = child.pid!; + let exited = false; + child.on("close", () => { + exited = true; + }); + await sleep(300); + expect(isAlive(pid)).toBe(true); + + // Short grace so the test is fast. hasExited() is backed by the real + // `close` event, NOT child.killed. + const timer = scheduleEscalatingKill(pid, () => exited, 150); + // Before the grace elapses, SIGTERM has been delivered but the wedged child + // is still alive (it trapped SIGTERM). + await sleep(80); + expect(isAlive(pid)).toBe(true); + // After the grace, the escalation SIGKILLs the group. + await sleep(400); + expect(isAlive(pid)).toBe(false); + clearTimeout(timer); + }, + ); + + it( + "does NOT SIGKILL when the process has already exited (clean completion reaps without a stray kill)", + { timeout: SUBPROC_TIMEOUT }, + async () => { + // Model a child that exited cleanly right after SIGTERM: hasExited() true. + const child = spawn("node", ["-e", OBEDIENT_CHILD], { stdio: "ignore", detached: true }); + const pid = child.pid!; + const timer = scheduleEscalatingKill(pid, () => true, 100); + await sleep(250); + // The obedient child exited on SIGTERM; the escalation must have SKIPPED + // SIGKILL (hasExited() === true). + await sleep(200); + expect(isAlive(pid)).toBe(false); + clearTimeout(timer); + }, + ); + + it( + "the OLD `!child.killed` guard would NOT have fired SIGKILL — regression contrast (RED demonstration)", + { timeout: SUBPROC_TIMEOUT }, + async () => { + // This test documents the ORIGINAL defect against a real subprocess: the + // old code gated SIGKILL on `!child.killed`, which is false immediately + // after SIGTERM delivery, so a SIGTERM-trapping child SURVIVES. + const child = spawn("node", ["-e", WEDGED_CHILD], { stdio: "ignore" }); + const pid = child.pid!; + await sleep(300); + child.kill("SIGTERM"); // old: signal the child pid directly + await sleep(150); + // Node flips child.killed true on delivery — the old guard is dead code. + expect(child.killed).toBe(true); + const oldGuardWouldFireSigkill = !child.killed; + expect(oldGuardWouldFireSigkill).toBe(false); + // Consequently the wedged process is STILL ALIVE under the old logic. + await sleep(200); + expect(isAlive(pid)).toBe(true); + + // Clean up: this child was spawned NON-detached (to mirror the old code + // exactly), so it is not its own group leader — kill it by pid directly. + // SIGKILL cannot be trapped, so the wedged child dies. + process.kill(pid, "SIGKILL"); + await sleep(400); + expect(isAlive(pid)).toBe(false); + }, + ); +}); diff --git a/src/__tests__/fix-drift-workflow.test.ts b/src/__tests__/fix-drift-workflow.test.ts index 98af1c79..bd3504e8 100644 --- a/src/__tests__/fix-drift-workflow.test.ts +++ b/src/__tests__/fix-drift-workflow.test.ts @@ -128,3 +128,77 @@ describe("fix-drift.yml — human-approval backstop: no unattended auto-merge", expect(wf).not.toContain("MERGE_REASON"); }); }); + +// --------------------------------------------------------------------------- +// WS-6 — end-of-job CATCH-ALL alert for the EARLY-infra window. The four +// specific alerts (collector-crash / autofix-step-fail / quarantine / +// fix-failure) are all gated on step outputs that only exist AFTER the +// collector ran. An EARLY failure (checkout / mint-app-token / pnpm install / +// clone ag-ui / git config) leaves those outputs empty, so without a catch-all +// the job dies red with ZERO Slack signal. These lock the catch-all's presence, +// its UNCONDITIONAL `if: failure()` gating, the anti-double-alert guard, and the +// infra-vs-drift-fix distinction. +// --------------------------------------------------------------------------- +describe("fix-drift.yml — WS-6: early-infra catch-all failure alert", () => { + it("has an end-of-job catch-all alert step", () => { + expect(wf).toContain("Alert on early-infra failure (catch-all)"); + }); + + it("the catch-all is gated on failure() and is UNCONDITIONAL on the earlier step OUTCOMES", () => { + // Isolate the catch-all step's `if:` expression. + const idx = wf.indexOf("Alert on early-infra failure (catch-all)"); + expect(idx).toBeGreaterThan(-1); + const stepBlock = wf.slice(idx, idx + 600); + const ifMatch = stepBlock.match(/if:\s*>-([\s\S]*?)\n\s{8}env:/); + expect(ifMatch).not.toBeNull(); + const ifExpr = (ifMatch?.[1] ?? "").replace(/\s+/g, " ").trim(); + + // MUST fire on any job failure. + expect(ifExpr).toContain("failure()"); + // MUST NOT gate on the autofix step OUTCOME (that would re-open the + // early-infra silence: autofix.outcome is empty pre-detect). + expect(ifExpr).not.toContain("steps.autofix.outcome"); + // Anti-double-alert guard: only fires when NONE of the specific alerts did + // (collector_crashed unset, quarantine unset, and check never ran so its + // skip output is empty). + expect(ifExpr).toContain("steps.detect.outputs.collector_crashed != 'true'"); + expect(ifExpr).toContain("steps.check.outputs.quarantine != 'true'"); + expect(ifExpr).toContain("steps.check.outputs.skip == ''"); + }); + + it("distinguishes an INFRA/SETUP failure from a drift-fix failure in its message", () => { + const idx = wf.indexOf("Alert on early-infra failure (catch-all)"); + const stepBlock = wf.slice(idx, idx + 1400); + expect(stepBlock).toMatch(/INFRA\/SETUP failure/); + // Missing-webhook must be a VISIBLE ::error:: + step failure, never silent. + expect(stepBlock).toContain("SLACK_WEBHOOK is not set"); + expect(stepBlock).toMatch(/::error::/); + }); + + it("the autofix-step-failure alert requires `check` to have RUN (skip == 'false'), so it never misfires on the early-infra window", () => { + // The autofix-failure alert must NOT fire before the collector ran — that + // window belongs to the catch-all. Gating on skip == 'false' (never empty + // once `check` executed) ensures the two are mutually exclusive. + const idx = wf.indexOf("Alert on autofix step failure"); + expect(idx).toBeGreaterThan(-1); + const stepBlock = wf.slice(idx, idx + 400); + expect(stepBlock).toContain("steps.check.outputs.skip == 'false'"); + }); +}); + +// --------------------------------------------------------------------------- +// WS-8 — the version-bump fail-closed reason must be NAMED in the failure +// alert, and the Create-PR step must surface the script's `reason=` on a +// non-zero exit so a fail-closed exit is not reported blank. +// --------------------------------------------------------------------------- +describe("fix-drift.yml — WS-8: version-bump-failed reason wiring", () => { + it("the fix-failure Slack alert names the version-bump-failed reason", () => { + expect(wf).toContain("version-bump-failed)"); + expect(wf).toMatch(/version-bump-failed\)\s+DETAIL=.*UNVERSIONED PR/); + }); + + it("the Create-PR step captures the script exit code + reason and surfaces it as a step output on failure", () => { + expect(wfFlat).toContain("PR_EXIT=${PIPESTATUS[0]}"); + expect(wfFlat).toContain("reason=${PR_REASON}"); + }); +}); diff --git a/src/__tests__/fix-drift.test.ts b/src/__tests__/fix-drift.test.ts index 297d770a..af5f8841 100644 --- a/src/__tests__/fix-drift.test.ts +++ b/src/__tests__/fix-drift.test.ts @@ -1061,6 +1061,118 @@ describe("F-A: sanctioned set comes from the passed (pinned) report, not on-disk }); }); +// --------------------------------------------------------------------------- +// WS-8 — version-bump failure must FAIL CLOSED, not warn-and-continue. +// +// The original createPr wrapped patchBumpVersion()+changelog+version-commit in +// a try/catch that did `console.warn("Version bump failed, skipping")` and +// CONTINUED to push + open the PR — shipping an UNVERSIONED "fix" that a human +// might merge but which never publishes a release (silent value loss). The fix +// makes a bump failure a HARD, fail-closed error: exit VERSION_BUMP_FAILED (18) +// with a named reason and NO push / NO PR. +// +// Driven against the REAL createPr with git mocked so the version-bump commit +// throws (no autofix subprocess). +// --------------------------------------------------------------------------- +describe("createPr version-bump fail-closed (WS-8)", () => { + const mockedExecSync = vi.mocked(execSync); + let logSpy: ReturnType | null = null; + let errSpy: ReturnType | null = null; + let warnSpy: ReturnType | null = null; + let exitSpy: ReturnType | null = null; + + beforeEach(() => { + vi.clearAllMocks(); + logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + warnSpy = vi.spyOn(console, "warn").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(); + warnSpy?.mockRestore(); + exitSpy?.mockRestore(); + }); + + function stdoutLines(): string[] { + return (logSpy?.mock.calls ?? []).map((c) => String(c[0])); + } + + // A clean RESOLVED-shaped report: ONE production builder changed (helpers.ts, + // sanctioned), no stragglers — so createPr proceeds PAST the straggler guard + // into the version-bump step. + 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: "", + }, + ], + }, + ], + }; + + function onlyHelpersChanged(cmd: unknown): string { + if (typeof cmd === "string" && cmd.includes("status --porcelain")) { + return "M src/helpers.ts\n" as unknown as string; + } + return "" as unknown as string; + } + + it("exits VERSION_BUMP_FAILED (18) with a named reason and opens NO PR when the version-bump commit throws", () => { + mockedExecSync.mockImplementation(onlyHelpersChanged as never); + // Make the version-bump git commit throw (mock git so the bump step fails). + mockedExecFileSync.mockImplementation((file: unknown, args: unknown) => { + if ( + file === "git" && + Array.isArray(args) && + (args as string[]).includes("commit") && + (args as string[]).some((a) => typeof a === "string" && a.includes("bump version")) + ) { + throw new Error("simulated git failure during version-bump commit"); + } + return Buffer.from("") as unknown as void; + }); + + expect(() => createPr(rep, { report: { timestamp: "t", entries: [] }, exitCode: 0 })).toThrow( + /__exit__18/, + ); + + // Named, fail-closed reason emitted for the workflow's failure alert. + expect(stdoutLines()).toContain("reason=version-bump-failed"); + + // FAIL CLOSED: it must NOT warn-and-continue, and must NEVER push or open a PR. + expect(warnSpy?.mock.calls ?? []).toEqual([]); + const pushed = mockedExecFileSync.mock.calls.some( + (c) => c[0] === "git" && Array.isArray(c[1]) && (c[1] as string[]).includes("push"), + ); + const openedPr = mockedExecFileSync.mock.calls.some( + (c) => c[0] === "gh" && Array.isArray(c[1]) && (c[1] as string[]).includes("pr"), + ); + expect(pushed).toBe(false); + expect(openedPr).toBe(false); + }); +}); + describe("affectedSkillSections", () => { it("returns empty array when no builder files are present", () => { expect(affectedSkillSections(["src/__tests__/foo.test.ts", "package.json"])).toEqual([]); From 30799392464daca7f48e308918cb12e240d1bbf0 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Fri, 17 Jul 2026 17:42:15 -0700 Subject: [PATCH 2/2] fix(drift): close tail-batch CR residuals on subprocess/alerting Address CR-confirmed residuals from the tail batch on top of the WS-4/6/8 core fixes. Edges, observability, and test-quality only. - invokeClaudeCode: a synchronous throw while wiring the stdout/stderr handlers (e.g. null streams from a spawn edge case) armed the 30-min killTimer at line 473 but never cleared it, stranding the timer and risking a late group-kill on a reused PID. Guard the handler setup in a try/catch that clears killTimer before rejecting (slot2-F1). - killProcessGroup: stop treating EPERM as a benign "nothing to kill". EPERM can mean the group is alive but unkillable by us (re-credentialed or re-parented child) and still burning the budget. Log a distinct warning and attempt a single-PID fallback; ESRCH stays benign (slot2-F5). - fix-drift.yml: quarantine (collector exit 5) now fails the job with exit 1 after the Slack ping, so a human watching CI status (not just Slack) sees the needs-human condition. The fix-failure alert and the catch-all are both disjoint from quarantine (skip == 'true'), so no double-alert (slot2-F3). - Fail-closed parse/git paths now emit a NAMED reason instead of a blank one: a new POST_FIX_PARSE_ERROR (19) wraps the parsePostFixExit / readPostFixReport throw in PR mode, and GIT_PUSH_FAILED (20) wraps the createPr git checkout/add/commit/push sequence. Both are wired into the workflow's reason -> message case block (slot2-F7/F12). - Tighten the WS-4 kill locks: the "does NOT SIGKILL when already exited" test now uses a SIGTERM-trapping child forced hasExited()==true so it genuinely proves the escalation is SKIPPED (the old obedient-child version passed for the wrong reason); add a no-late-timer lock and an executor-throw lock (fix-drift-invoke.test.ts); replace the illustrative old-guard contrast with real revert-guards (slot3-F1/F2). Red-green verified per fix by stashing the source fixes and running the new tests against the pre-fix code. Full suite (4666), format, lint, tsc, actionlint, and build all pass. --- .github/workflows/fix-drift.yml | 9 ++ scripts/drift-success-predicate.ts | 19 +++ scripts/fix-drift.ts | 148 ++++++++++++++++++----- src/__tests__/fix-drift-invoke.test.ts | 77 ++++++++++++ src/__tests__/fix-drift-kill.test.ts | 116 ++++++++++++++---- src/__tests__/fix-drift-workflow.test.ts | 48 ++++++++ src/__tests__/fix-drift.test.ts | 26 ++++ 7 files changed, 390 insertions(+), 53 deletions(-) create mode 100644 src/__tests__/fix-drift-invoke.test.ts diff --git a/.github/workflows/fix-drift.yml b/.github/workflows/fix-drift.yml index 1d4160ab..6c80672b 100644 --- a/.github/workflows/fix-drift.yml +++ b/.github/workflows/fix-drift.yml @@ -468,6 +468,13 @@ jobs: curl -fsS --show-error -X POST "$SLACK_WEBHOOK" \ -H "Content-Type: application/json" \ -d "$PAYLOAD" + # Quarantine is a needs-human outcome — FAIL the job so a human + # watching CI status (not just Slack) sees it (matching the + # collector-crash / autofix-failure alerts which also exit 1). The + # other alert steps are mutually exclusive with quarantine (fix-failure + # requires skip != 'true'; catch-all requires skip == ''; both false on + # quarantine where skip == 'true'), so this does not double-alert. + exit 1 # 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 @@ -526,6 +533,8 @@ jobs: 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)";; version-bump-failed) DETAIL=" (NEEDS HUMAN: the version bump / CHANGELOG step failed — refused to open an UNVERSIONED PR that would merge a fix which never publishes a release. See run logs)";; + post-fix-parse-error) DETAIL=" (NEEDS HUMAN: the post-fix collector result could not be parsed/read — empty/non-integer --post-fix-exit (e.g. a skipped recollect) or an unreadable post-fix report. Failed closed: no PR opened. See run logs)";; + git-push-failed) DETAIL=" (NEEDS HUMAN: a git checkout/add/commit/push failed while opening the PR — failed closed, no PR opened. See run logs)";; config-error) DETAIL=" (drift-success predicate CONFIG error — missing/unreadable report or bad args; the gate itself needs attention)";; *) DETAIL="";; esac diff --git a/scripts/drift-success-predicate.ts b/scripts/drift-success-predicate.ts index 5999a646..02a30672 100644 --- a/scripts/drift-success-predicate.ts +++ b/scripts/drift-success-predicate.ts @@ -136,6 +136,23 @@ export enum PredicateReason { * needs-human reason. */ VERSION_BUMP_FAILED = "version-bump-failed", + /** + * The `--post-fix-*` arguments were present but could not be parsed/read + * while opening a drift-fix PR (e.g. an empty/non-integer `--post-fix-exit` + * from a skipped recollect, or an unreadable post-fix report). fix-drift.ts + * already fails CLOSED on this (no PR), but the throw historically reached the + * top-level catch with a BLANK `reason=`; this names the cause so the Slack + * alert is not blank. Surfaced by fix-drift.ts's main(), never the predicate. + */ + POST_FIX_PARSE_ERROR = "post-fix-parse-error", + /** + * A git operation (checkout / add / commit / push) failed while opening a + * drift-fix PR. This fails CLOSED (no PR is opened — the push never completed, + * so no partial/unversioned PR ships) but historically alerted with a BLANK + * `reason=`; this names the cause. Surfaced by fix-drift.ts's createPr, never + * the predicate. + */ + GIT_PUSH_FAILED = "git-push-failed", } /** Stable exit code for each reason (see module header). */ @@ -151,6 +168,8 @@ export const REASON_EXIT_CODE: Record = { [PredicateReason.PRODUCTION_CHANGE_OFF_TARGET]: 16, [PredicateReason.CONFIG_ERROR]: 2, [PredicateReason.VERSION_BUMP_FAILED]: 18, + [PredicateReason.POST_FIX_PARSE_ERROR]: 19, + [PredicateReason.GIT_PUSH_FAILED]: 20, }; export interface PredicateInputs { diff --git a/scripts/fix-drift.ts b/scripts/fix-drift.ts index 7f66c3a1..40af57bd 100644 --- a/scripts/fix-drift.ts +++ b/scripts/fix-drift.ts @@ -33,6 +33,9 @@ * 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) + * 18 — VERSION_BUMP_FAILED (version bump / CHANGELOG step failed) + * 19 — POST_FIX_PARSE_ERROR (unparseable --post-fix-exit / -report) + * 20 — GIT_PUSH_FAILED (git checkout/add/commit/push failed) * The legacy exit 4 (no source files changed) is subsumed by 10/11. The * drift-success predicate is MANDATORY in --create-pr mode: BOTH * --post-fix-report and --post-fix-exit are required (there is no legacy @@ -373,10 +376,19 @@ export function buildPrompt(report: DriftReport): string { * grandchild). Signalling the child pid alone (`child.kill()`) reaches only the * `npx` wrapper and leaves a wedged grandchild alive to burn the job budget. * - * Swallows ESRCH (group already gone) and EPERM (nothing left we own to - * signal) — both mean "there is nothing more to kill", which is success for a - * best-effort teardown. Returns true if a signal was delivered, false if the - * group was already gone / unkillable. + * ESRCH (group already gone) is a benign "nothing left to kill" and is swallowed + * to `false`. EPERM is DIFFERENT and must NOT be treated as benign: `kill(2)` + * returns EPERM when the target process(es) EXIST but the caller lacks + * permission to signal them — e.g. a grandchild that changed credentials (a + * setuid postinstall) or was re-parented under a remapped container user. Such a + * process can be STILL ALIVE and STILL BURNING the job budget — exactly the + * WS-4 leak this guards against. So on EPERM we log a VISIBLE warning (never + * silently claim success) and attempt a single-PID fallback (`kill(pid)` rather + * than the whole group) in case only the group-leader escaped our permission. + * The job's `timeout-minutes: 30` ceiling remains the ultimate backstop. + * + * Returns true if a group signal was delivered, false if the group was already + * gone (ESRCH) or is present-but-unkillable (EPERM after the fallback attempt). */ export function killProcessGroup(pid: number, signal: NodeJS.Signals): boolean { try { @@ -385,11 +397,34 @@ export function killProcessGroup(pid: number, signal: NodeJS.Signals): boolean { return true; } catch (err) { const code = (err as NodeJS.ErrnoException).code; - if (code === "ESRCH" || code === "EPERM") { - // ESRCH: the group has already fully exited. EPERM: nothing we still own - // to signal in that group. Either way, nothing left to kill. + if (code === "ESRCH") { + // The group has already fully exited — nothing left to kill. return false; } + if (code === "EPERM") { + // NOT a benign "nothing to kill": the group (or part of it) exists but is + // unkillable by us. Surface it loudly and try a single-PID fallback in + // case only the leader escaped our permission. + console.error( + `WARNING: EPERM signalling process group ${pid} with ${signal} — the group may ` + + "still be ALIVE and unkillable by us (re-credentialed / re-parented child). " + + "Attempting single-PID fallback; the 30-min job ceiling is the final backstop.", + ); + try { + process.kill(pid, signal); + return true; + } catch (fallbackErr) { + const fallbackCode = (fallbackErr as NodeJS.ErrnoException).code; + if (fallbackCode === "ESRCH") { + return false; + } + console.error( + `WARNING: single-PID fallback kill of ${pid} with ${signal} also failed ` + + `(${fallbackCode ?? "unknown"}) — process may leak until the job timeout.`, + ); + return false; + } + } throw err; } } @@ -424,7 +459,7 @@ export function scheduleEscalatingKill( }, graceMs); } -function invokeClaudeCode(prompt: string): Promise { +export function invokeClaudeCode(prompt: string): Promise { return new Promise((done, reject) => { const args = [ "@anthropic-ai/claude-code", @@ -497,15 +532,32 @@ function invokeClaudeCode(prompt: string): Promise { reject(err); }); - child.stdout.on("data", (chunk: Buffer) => { - process.stdout.write(chunk); - logChunks.push(chunk); - }); - - child.stderr.on("data", (chunk: Buffer) => { - process.stderr.write(chunk); - logChunks.push(chunk); - }); + // Wire the stream + close handlers inside a guard so a SYNCHRONOUS throw + // here (e.g. `child.stdout` is null on a spawn edge case, making + // `child.stdout.on(...)` a TypeError) cannot strand the already-armed + // `killTimer`. Without this, such a throw rejects the Promise via the + // executor's synchronous-throw semantics BUT the 30-min timer stays live and + // would later group-kill a possibly-reused PID. Clear the timer, then reject. + try { + if (!child.stdout || !child.stderr) { + throw new Error( + "Claude Code child has no stdout/stderr pipe (spawn produced null streams)", + ); + } + child.stdout.on("data", (chunk: Buffer) => { + process.stdout.write(chunk); + logChunks.push(chunk); + }); + + child.stderr.on("data", (chunk: Buffer) => { + process.stderr.write(chunk); + logChunks.push(chunk); + }); + } catch (setupErr) { + clearTimeout(killTimer); + reject(setupErr instanceof Error ? setupErr : new Error(String(setupErr))); + return; + } child.on("close", (code, signal) => { // Mark real exit BEFORE clearing timers so any in-flight grace timer that @@ -826,12 +878,18 @@ export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): } console.log(`Drift-success predicate: RESOLVED — ${verdict.detail}`); - // Determine branch name + // Determine branch name. A git failure here fails CLOSED with a NAMED reason + // (git-push-failed) rather than a blank-reason exit 3 (see the catch at the + // end of the git-op sequence below). let currentBranch: string; try { currentBranch = exec("git rev-parse --abbrev-ref HEAD"); } catch (err: unknown) { - throw new Error(`Cannot determine current branch for PR creation: ${(err as Error).message}`); + console.error( + `ERROR: cannot determine current branch for PR creation: ${(err as Error).message}`, + ); + console.log(`reason=${PredicateReason.GIT_PUSH_FAILED}`); + process.exit(REASON_EXIT_CODE[PredicateReason.GIT_PUSH_FAILED]); } const branchName = @@ -839,8 +897,26 @@ export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): ? `fix/drift-${stamp}` : currentBranch; + // A git checkout/stage/commit/push failure anywhere below fails CLOSED (no PR + // — the push never completes, so no partial/unversioned PR ships) but the raw + // throw would reach the top-level catch with a BLANK `reason=`. `gitOp` names + // the cause (git-push-failed) so the operator alert is not blank. The + // version-bump block keeps its OWN distinct VERSION_BUMP_FAILED reason. + const gitOp = (label: string, fn: () => void): void => { + try { + fn(); + } catch (err) { + console.error( + `ERROR: git operation failed (${label}) while opening the drift-fix PR — no PR opened:`, + err instanceof Error ? err.message : err, + ); + console.log(`reason=${PredicateReason.GIT_PUSH_FAILED}`); + process.exit(REASON_EXIT_CODE[PredicateReason.GIT_PUSH_FAILED]); + } + }; + if (branchName !== currentBranch) { - execFileSafe("git", ["checkout", "-b", branchName]); + gitOp("checkout -b", () => execFileSafe("git", ["checkout", "-b", branchName])); console.log(`Created branch ${branchName}`); } @@ -874,13 +950,17 @@ export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): } if (builderFiles.length > 0) { - execFileSafe("git", ["add", ...builderFiles]); - execFileSafe("git", ["commit", "-m", "fix: auto-remediate API drift in builder functions"]); + gitOp("add builders", () => execFileSafe("git", ["add", ...builderFiles])); + gitOp("commit builders", () => + execFileSafe("git", ["commit", "-m", "fix: auto-remediate API drift in builder functions"]), + ); } if (testFiles.length > 0) { - execFileSafe("git", ["add", ...testFiles]); - execFileSafe("git", ["commit", "-m", "test: update SDK shapes for drift remediation"]); + gitOp("add tests", () => execFileSafe("git", ["add", ...testFiles])); + gitOp("commit tests", () => + execFileSafe("git", ["commit", "-m", "test: update SDK shapes for drift remediation"]), + ); } // The version bump + CHANGELOG are an EXPLICIT, gated part of the fix set — a @@ -915,7 +995,7 @@ export function createPr(report: DriftReport, postFix?: PostFixCollectorResult): process.exit(REASON_EXIT_CODE[PredicateReason.VERSION_BUMP_FAILED]); } - execFileSafe("git", ["push", "-u", "origin", branchName]); + gitOp("push", () => execFileSafe("git", ["push", "-u", "origin", branchName])); console.log(`Pushed branch ${branchName}`); const prBody = buildPrBody(report, changedFiles, verdict.detail); @@ -1112,8 +1192,22 @@ async function main(): Promise { // --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])); + // + // These parse/read failures fail closed (no PR) — but the raw throw would + // reach the top-level catch with a BLANK `reason=`, so the operator alert is + // uninformative. Emit a NAMED reason (post-fix-parse-error) before + // rethrowing so the workflow's `grep '^reason='` names the cause. + let postFixExit: number; + let postFixReport: DriftReport; + try { + postFixExit = parsePostFixExit(args[postFixExitIdx + 1]); + postFixReport = readPostFixReport(resolve(args[postFixReportIdx + 1])); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + console.error(`ERROR: could not parse/read the post-fix collector result: ${msg}`); + console.log(`reason=${PredicateReason.POST_FIX_PARSE_ERROR}`); + process.exit(REASON_EXIT_CODE[PredicateReason.POST_FIX_PARSE_ERROR]); + } const postFix: PostFixCollectorResult = { report: postFixReport, exitCode: postFixExit }; createPr(report, postFix); diff --git a/src/__tests__/fix-drift-invoke.test.ts b/src/__tests__/fix-drift-invoke.test.ts new file mode 100644 index 00000000..e000c2ce --- /dev/null +++ b/src/__tests__/fix-drift-invoke.test.ts @@ -0,0 +1,77 @@ +/** + * WS-4 residual lock (slot2-F1) — a SYNCHRONOUS throw during the Promise + * executor setup of `invokeClaudeCode` must NOT strand the already-armed 30-min + * `killTimer`. + * + * The executor arms `killTimer` (a 30-minute setTimeout that would later + * group-kill `child.pid`) BEFORE it wires the stdout/stderr handlers. If + * `child.stdout` is null (a spawn edge case), `child.stdout.on(...)` throws a + * TypeError synchronously inside the executor. Without the fix, that throw + * rejects the Promise but leaves `killTimer` LIVE — a leaked 30-min timer that + * could later SIGKILL a reused PID. The fix clears the timer before rejecting. + * + * We mock `spawn` to return a fake child with NULL streams, drive + * `invokeClaudeCode`, and assert (a) the Promise REJECTS (not hangs) and (b) no + * timer is left pending (fake timers report zero pending after the reject). + */ +import { EventEmitter } from "node:events"; + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("node:child_process", async () => { + const actual = await vi.importActual("node:child_process"); + return { ...actual, spawn: vi.fn() }; +}); + +import { spawn } from "node:child_process"; + +import { invokeClaudeCode } from "../../scripts/fix-drift.js"; + +const mockedSpawn = vi.mocked(spawn); + +/** A fake detached child whose stdout/stderr are NULL (the edge case). */ +function makeNullStreamChild(pid = 4242): EventEmitter & { + pid: number; + stdout: null; + stderr: null; +} { + const child = new EventEmitter() as EventEmitter & { + pid: number; + stdout: null; + stderr: null; + }; + child.pid = pid; + child.stdout = null; + child.stderr = null; + return child; +} + +describe("invokeClaudeCode — executor-setup throw must not strand killTimer (WS-4/slot2-F1)", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.useFakeTimers(); + vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + vi.clearAllTimers(); + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it("REJECTS when the child has null streams (no stdout/stderr to attach handlers to)", async () => { + mockedSpawn.mockReturnValue(makeNullStreamChild() as never); + await expect(invokeClaudeCode("prompt")).rejects.toThrow(/no stdout\/stderr pipe/i); + }); + + it("clears the 30-min killTimer on the setup-throw path — NO timer is left pending", async () => { + mockedSpawn.mockReturnValue(makeNullStreamChild() as never); + + // The executor arms killTimer, then throws attaching the null-stream + // handler. The fix must clearTimeout(killTimer) before rejecting, so after + // the reject settles there must be ZERO pending fake timers. Without the + // fix, the 30-min killTimer would still be pending here. + await expect(invokeClaudeCode("prompt")).rejects.toThrow(); + expect(vi.getTimerCount()).toBe(0); + }); +}); diff --git a/src/__tests__/fix-drift-kill.test.ts b/src/__tests__/fix-drift-kill.test.ts index dfc21eaa..0f0e62d8 100644 --- a/src/__tests__/fix-drift-kill.test.ts +++ b/src/__tests__/fix-drift-kill.test.ts @@ -17,7 +17,7 @@ */ import { spawn } from "node:child_process"; -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi } from "vitest"; /** Real subprocess spin-up + grace windows need more than the default budget. */ const SUBPROC_TIMEOUT = 15000; @@ -78,6 +78,37 @@ describe("killProcessGroup", () => { expect(killProcessGroup(missing, "SIGTERM")).toBe(false); }); + it("does NOT silently treat EPERM as success — logs a visible warning and attempts a single-PID fallback (slot2-F5)", () => { + // EPERM means the group EXISTS but is unkillable by us (re-credentialed / + // re-parented child) — it may still be ALIVE burning the budget. It must + // NOT be swallowed as a benign "nothing to kill" like ESRCH. Assert we (a) + // log a distinct WARNING and (b) attempt the single-PID fallback. + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + let call = 0; + const killSpy = vi.spyOn(process, "kill").mockImplementation(((pidArg: number) => { + call += 1; + if (call === 1) { + // First call: group signal (negative pid) → EPERM. + expect(pidArg).toBeLessThan(0); + const e = new Error("EPERM") as NodeJS.ErrnoException; + e.code = "EPERM"; + throw e; + } + // Second call: the single-PID fallback (positive pid) succeeds. + expect(pidArg).toBeGreaterThan(0); + return true; + }) as never); + try { + expect(killProcessGroup(12345, "SIGKILL")).toBe(true); // fallback delivered + expect(call).toBe(2); // group attempt + single-PID fallback + const warned = errSpy.mock.calls.some((c) => String(c[0]).includes("EPERM")); + expect(warned).toBe(true); // distinct, visible EPERM warning — not silent + } finally { + killSpy.mockRestore(); + errSpy.mockRestore(); + } + }); + it("re-throws unexpected errors (e.g. EINVAL from a bad signal)", () => { const child = spawn("node", ["-e", OBEDIENT_CHILD], { stdio: "ignore", detached: true }); const pid = child.pid!; @@ -124,47 +155,80 @@ describe("scheduleEscalatingKill — SIGKILL escalation gated on a REAL exit fla ); it( - "does NOT SIGKILL when the process has already exited (clean completion reaps without a stray kill)", + "does NOT SIGKILL when hasExited() is true — the gate genuinely SKIPS escalation (a SIGTERM-trapping child that WOULD have died to a stray SIGKILL stays alive)", { timeout: SUBPROC_TIMEOUT }, async () => { - // Model a child that exited cleanly right after SIGTERM: hasExited() true. - const child = spawn("node", ["-e", OBEDIENT_CHILD], { stdio: "ignore", detached: true }); + // The previous version of this test used an OBEDIENT child and asserted + // it was dead — but an obedient child dies from the unconditional SIGTERM + // that scheduleEscalatingKill sends FIRST, regardless of whether the + // SIGKILL escalation is skipped, so it passed for the WRONG reason (it + // could not distinguish "escalation skipped" from "escalation fired"). + // + // Use a WEDGED child that TRAPS SIGTERM instead: the first SIGTERM leaves + // it alive, so the ONLY thing that could kill it within the window is the + // SIGKILL escalation. With hasExited() forced true, that escalation MUST + // be skipped — so the child must remain ALIVE after the grace elapses. If + // the has-exited gate regressed to always-escalate (the WS-4 defect), the + // group SIGKILL would fire and the child would be DEAD — turning this RED. + const child = spawn("node", ["-e", WEDGED_CHILD], { stdio: "ignore", detached: true }); const pid = child.pid!; + await sleep(300); + expect(isAlive(pid)).toBe(true); + const timer = scheduleEscalatingKill(pid, () => true, 100); - await sleep(250); - // The obedient child exited on SIGTERM; the escalation must have SKIPPED - // SIGKILL (hasExited() === true). + // Wait well past the grace window: the escalation callback has run (and, + // gated on hasExited()===true, SKIPPED the SIGKILL). + await sleep(400); + // Wedged child trapped the SIGTERM and no SIGKILL was sent → STILL ALIVE. + expect(isAlive(pid)).toBe(true); + + // The grace timer must have already fired-and-skipped (not still pending): + // clearing it now is a no-op, and no LATE SIGKILL can arrive after this. + clearTimeout(timer); await sleep(200); + expect(isAlive(pid)).toBe(true); // no late kill + + // Clean up the still-alive wedged group. + try { + process.kill(-pid, "SIGKILL"); + } catch { + /* best effort */ + } + await sleep(300); expect(isAlive(pid)).toBe(false); - clearTimeout(timer); }, ); it( - "the OLD `!child.killed` guard would NOT have fired SIGKILL — regression contrast (RED demonstration)", + "the returned grace timer, once cleared before it fires, delivers NO late SIGKILL to a still-running group", { timeout: SUBPROC_TIMEOUT }, async () => { - // This test documents the ORIGINAL defect against a real subprocess: the - // old code gated SIGKILL on `!child.killed`, which is false immediately - // after SIGTERM delivery, so a SIGTERM-trapping child SURVIVES. - const child = spawn("node", ["-e", WEDGED_CHILD], { stdio: "ignore" }); + // Locks the caller-side lifecycle used by invokeClaudeCode's `close` + // handler: on a clean early exit the caller clears the returned timer, and + // a pending SIGKILL escalation must NOT fire late against a (possibly + // reused) PID. Here the child is still running and hasExited() would be + // false, so ONLY a fired escalation could kill it — we clear the timer + // BEFORE the grace elapses and assert the child survives. + const child = spawn("node", ["-e", WEDGED_CHILD], { stdio: "ignore", detached: true }); const pid = child.pid!; await sleep(300); - child.kill("SIGTERM"); // old: signal the child pid directly - await sleep(150); - // Node flips child.killed true on delivery — the old guard is dead code. - expect(child.killed).toBe(true); - const oldGuardWouldFireSigkill = !child.killed; - expect(oldGuardWouldFireSigkill).toBe(false); - // Consequently the wedged process is STILL ALIVE under the old logic. - await sleep(200); expect(isAlive(pid)).toBe(true); - // Clean up: this child was spawned NON-detached (to mirror the old code - // exactly), so it is not its own group leader — kill it by pid directly. - // SIGKILL cannot be trapped, so the wedged child dies. - process.kill(pid, "SIGKILL"); - await sleep(400); + // Long grace so we can cancel before it fires. hasExited stays false. + const timer = scheduleEscalatingKill(pid, () => false, 5000); + // The initial SIGTERM is trapped; child alive. Cancel before the grace. + await sleep(100); + clearTimeout(timer); + // Well past what the grace would have been — no SIGKILL arrives. + await sleep(300); + expect(isAlive(pid)).toBe(true); + + try { + process.kill(-pid, "SIGKILL"); + } catch { + /* best effort */ + } + await sleep(300); expect(isAlive(pid)).toBe(false); }, ); diff --git a/src/__tests__/fix-drift-workflow.test.ts b/src/__tests__/fix-drift-workflow.test.ts index bd3504e8..39cf930d 100644 --- a/src/__tests__/fix-drift-workflow.test.ts +++ b/src/__tests__/fix-drift-workflow.test.ts @@ -202,3 +202,51 @@ describe("fix-drift.yml — WS-8: version-bump-failed reason wiring", () => { expect(wfFlat).toContain("reason=${PR_REASON}"); }); }); + +// --------------------------------------------------------------------------- +// slot2-F3 — QUARANTINE must FAIL THE JOB (non-green), not just Slack-ping. +// A human watching CI status (not Slack) must see quarantine as a failure, like +// the collector-crash / autofix-failure alerts. Because quarantine sets +// check.outputs.skip == 'true', the fix-failure alert (needs skip != 'true') +// and the catch-all (needs skip == '') are both disjoint from it, so making the +// quarantine step exit 1 does NOT double-alert. +// --------------------------------------------------------------------------- +describe("fix-drift.yml — slot2-F3: quarantine fails the job (non-green)", () => { + it("the quarantine alert step exits non-zero on the happy (webhook-sent) path too", () => { + const idx = wf.indexOf("Alert on drift quarantine"); + expect(idx).toBeGreaterThan(-1); + // The step body runs until the next `- name:` step. + const nextStep = wf.indexOf("\n - name:", idx + 1); + const stepBlock = wf.slice(idx, nextStep === -1 ? undefined : nextStep); + // The curl (happy path) must be FOLLOWED by an `exit 1` — the step is not + // allowed to end green after sending the Slack ping. + const curlIdx = stepBlock.lastIndexOf("curl -fsS"); + expect(curlIdx).toBeGreaterThan(-1); + expect(stepBlock.slice(curlIdx)).toMatch(/\n\s*exit 1\b/); + }); + + it("does not overlap the fix-failure alert or the catch-all (both disjoint from quarantine's skip=='true')", () => { + // fix-failure requires skip != 'true'; catch-all requires skip == ''; + // quarantine sets skip == 'true' — so neither fires alongside it. + expect(wf).toContain("steps.check.outputs.skip != 'true'"); // fix-failure guard + const catchAllIdx = wf.indexOf("Alert on early-infra failure (catch-all)"); + expect(wf.slice(catchAllIdx, catchAllIdx + 600)).toContain("steps.check.outputs.skip == ''"); + }); +}); + +// --------------------------------------------------------------------------- +// slot2-F7/F12 — the fail-closed parse/git paths must be NAMED in the failure +// alert (post-fix-parse-error / git-push-failed), not blank. The code emits the +// reason; the workflow's case block must translate it to a human DETAIL. +// --------------------------------------------------------------------------- +describe("fix-drift.yml — slot2-F7/F12: fail-closed reasons are named in the alert", () => { + it("the fix-failure alert names the post-fix-parse-error reason", () => { + expect(wf).toContain("post-fix-parse-error)"); + expect(wfFlat).toMatch(/post-fix-parse-error\) DETAIL=.*Failed closed/); + }); + + it("the fix-failure alert names the git-push-failed reason", () => { + expect(wf).toContain("git-push-failed)"); + expect(wfFlat).toMatch(/git-push-failed\) DETAIL=.*no PR opened/); + }); +}); diff --git a/src/__tests__/fix-drift.test.ts b/src/__tests__/fix-drift.test.ts index af5f8841..24bee46c 100644 --- a/src/__tests__/fix-drift.test.ts +++ b/src/__tests__/fix-drift.test.ts @@ -1171,6 +1171,32 @@ describe("createPr version-bump fail-closed (WS-8)", () => { expect(pushed).toBe(false); expect(openedPr).toBe(false); }); + + it("exits GIT_PUSH_FAILED (20) with a named reason and opens NO PR when `git push` throws", () => { + // slot2-F12 lock: a git push failure after the local commits must fail + // CLOSED (no PR) AND emit a NAMED reason — historically it reached the + // top-level catch as a blank-reason exit 3. + mockedExecSync.mockImplementation(onlyHelpersChanged as never); + mockedExecFileSync.mockImplementation((file: unknown, args: unknown) => { + if (file === "git" && Array.isArray(args) && (args as string[]).includes("push")) { + throw new Error("simulated git push failure (auth/network)"); + } + return Buffer.from("") as unknown as void; + }); + + expect(() => createPr(rep, { report: { timestamp: "t", entries: [] }, exitCode: 0 })).toThrow( + /__exit__20/, + ); + + // Named, fail-closed reason (not blank) for the workflow's failure alert. + expect(stdoutLines()).toContain("reason=git-push-failed"); + + // No PR opened — push failed before `gh pr create`. + const openedPr = mockedExecFileSync.mock.calls.some( + (c) => c[0] === "gh" && Array.isArray(c[1]) && (c[1] as string[]).includes("pr"), + ); + expect(openedPr).toBe(false); + }); }); describe("affectedSkillSections", () => {