fix(drift): process-group kill, fail-closed version bump, early-infra alerting#312
Merged
Conversation
… alerting 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).
commit: |
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.
jpr5
marked this pull request as ready for review
July 18, 2026 00:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardens three drift-remediation tail workstreams (WS-4/6/8) as one coherent PR, each with a red-green regression lock (WS-10). All against
origin/main@a21e786.Fixes
WS-4 — real process-group subprocess control (
scripts/fix-drift.ts)invokeClaudeCodespawnednpxwith no process group and gated the SIGKILL escalation onif (!child.killed). Node setschild.killed = truethe instant SIGTERM is delivered (not when the process exits), so!child.killedis ~always false and SIGKILL never fires; and signalling thenpxwrapper never reached the@anthropic-ai/claude-codegrandchild — a wedged fixer survived SIGTERM and burned the full 30-min job budget.Fix: spawn with
detached: true(own process group), and on timeout SIGTERM then — after a grace — SIGKILL the whole group viaprocess.kill(-pid, …), gated on a real has-exited flag flipped by thecloseevent. ExtractedkillProcessGroup(ESRCH/EPERM tolerant) +scheduleEscalatingKillso the escalation is unit-testable against a real wedged child. Normal completion still reaps cleanly (theclosehandler cancels the grace timer and setsexitedfirst).WS-8 — version-bump failure must fail closed (
scripts/fix-drift.ts)createPrcaught a version-bump/CHANGELOG failure,console.warn("…skipping"), and shipped an unversioned PR that never publishes a release (silent value loss). Fix: a bump failure is now a hard error — exit with a newVERSION_BUMP_FAILEDreason (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 (
.github/workflows/fix-drift.yml)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. Fix: added an end-of-job
if: failure()catch-all that fires only when none of the specific alerts did (the early-infra window), labelling it an infra/setup failure distinct from a drift-fix failure. Tightened the autofix-step alert to requirecheckto have run (skip == 'false') so it no longer misfires on that window (no double-alert). 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.Red-green (WS-10 locks)
WS-4 — real wedged subprocess. A node child that traps SIGTERM and sleeps:
OLD: child.killed=true -> SIGKILL fired? false ; process still alive? truevsNEW: exited=true ; process still alive? false.fix-drift.ts→killProcessGroup/scheduleEscalatingKillarenot a function(4 fail).!child.killed-guard contrast test proves the old dead-code path.WS-8 — createPr fail-closed. Git mocked so the version-bump commit throws:
__exit__18, emitsreason=version-bump-failed, no push / nogh pr, noconsole.warn.WS-6 — workflow static assertions.
version-bump-failedcase).failure()-unconditional on step outcomes + anti-double-alert guard; autofix alert requiresskip == 'false'; version-bump-failed reason wired.Gates (all green locally)
pnpm test(4614 passed / 44 skipped) ·format:check·lint·tsc --noEmit·build·actionlint(+ shellcheck) onfix-drift.yml·commitlint --from origin/main --to HEAD→ 0 problems.Deferred
parsePorcelainLineC-unescape — not a trivial 1-3 line change (mirrored across two files + would need a shared helper), so skipped per the re-audit.🤖 Generated with Claude Code