From 740fea38c2d141e07b3b21020257e65c78e803a2 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 02:51:14 -0700 Subject: [PATCH] =?UTF-8?q?fleet:=20restart-recovery-coordinator.ts=204=20?= =?UTF-8?q?=E2=86=92=201=20(dead=20fallbacks=20deleted,=20not=20converted)?= =?UTF-8?q?=20(#3059)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claiming `packages/engine/src/restart-recovery-coordinator.ts`. ## Census before/after | File | Before | After | |---|---:|---:| | `packages/engine/src/restart-recovery-coordinator.ts` | 4 | **1** | ## These were deletions, not conversions All three sites were fail-soft fallbacks behind an **optional** `reviewColumns` parameter: ```ts return (reviewColumns ? reviewColumns.has(task.column) : task.column === "in-review") ``` Production never took that branch — `self-healing.ts:13646-13649` supplies the resolved set at every call site. So the correct change is to make the parameter required and delete the literal, not to swap it for a role lookup. ## The trap this hit, which would have shipped a crash **Making the parameter required produced ZERO tsc errors.** That looked like proof the fallback was unreachable. It is not: the engine `tsconfig` covers `src` and not `__tests__`, so the type-checker cannot see the callers that actually relied on the default. Running the tests surfaced them immediately as `TypeError: Cannot read properties of undefined (reading 'has')`. This is the same class as finding 2 in `docs/solutions/best-practices/proving-a-code-path-actually-runs.md` — a negative result from a checker that cannot see the thing it is being asked about. Anyone converting a `src`-only-typechecked package should assume tsc is blind to test call sites. The blast radius was also one site larger than grep suggested: the `isRecoverableMissingWorktreeReviewFailure` **combiner** threads the set to all three inner predicates. Its own comment already names why — *"a caller cannot convert the outer question and leave one of the three inner ones on the legacy id — the half-conversion shape this program keeps finding."* Tests now pass the set production always passes, preserving exactly what each case asserted. ## Remaining 1, flagged not guessed `L149` uses a different shape (`isReviewColumn ?? task.column === "in-review"`) whose callers I did not establish. Absence from grep is not proof of no caller, so it stays counted. ## Verification - census: 4 → 1 - `restart-recovery-coordinator` + `self-healing` — **424 tests green** - `tsc --noEmit` clean; `pnpm lint` clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) --- .../restart-recovery-coordinator.test.ts | 41 ++++++++++++------- .../src/restart-recovery-coordinator.ts | 14 +++---- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/packages/engine/src/__tests__/restart-recovery-coordinator.test.ts b/packages/engine/src/__tests__/restart-recovery-coordinator.test.ts index c3094ca6b8..bbc3664341 100644 --- a/packages/engine/src/__tests__/restart-recovery-coordinator.test.ts +++ b/packages/engine/src/__tests__/restart-recovery-coordinator.test.ts @@ -37,6 +37,19 @@ literal, because each one individually looks right. `reviewColumns` is optional and defaults to the legacy id, so the three existing call sites are unchanged until each passes its own resolved set. */ +/* +FNXC:WorkflowResolvedColumns 2026-07-31-11:20 (fleet: restart-recovery roles): +`reviewColumns` is REQUIRED now. It was optional with a `task.column === "in-review"` fallback that +production never took — `self-healing.ts` supplies the resolved set at every call site — so the +literal survived only because these tests omitted the argument. Passing the set preserves exactly +what each case asserts while removing the last thing keeping the fallback alive. + +Worth recording: making the parameter required produced ZERO tsc errors, because the engine +tsconfig covers `src` and not `__tests__`. A clean typecheck was not evidence here; only running +the tests found these call sites. +*/ +const REVIEW_LANES: ReadonlySet = new Set(["in-review"]); + describe("isInReviewMissingWorktreeSessionStartFailure", () => { /* FNXC:MissingWorktreeRetry 2026-07-30-10:05 (PR #2728, aligned to #2736's signature): @@ -107,13 +120,13 @@ describe("RestartRecoveryCoordinator", () => { steps: [{ id: "s1", title: "step", status: "done" }] as any, }); - expect(isRecoverableMissingWorktreeReviewFailure({ ...baseTask, error: "Refusing to start coding agent in missing worktree: /tmp/wt" })).toBe(true); - expect(isRecoverableMissingWorktreeReviewFailure({ ...baseTask, error: "Refusing to start coding agent in incomplete worktree: /tmp/wt" })).toBe(true); - expect(isRecoverableMissingWorktreeReviewFailure({ ...baseTask, error: "Refusing to start coding agent in unregistered git worktree: /tmp/wt" })).toBe(true); + expect(isRecoverableMissingWorktreeReviewFailure({ ...baseTask, error: "Refusing to start coding agent in missing worktree: /tmp/wt" }, REVIEW_LANES)).toBe(true); + expect(isRecoverableMissingWorktreeReviewFailure({ ...baseTask, error: "Refusing to start coding agent in incomplete worktree: /tmp/wt" }, REVIEW_LANES)).toBe(true); + expect(isRecoverableMissingWorktreeReviewFailure({ ...baseTask, error: "Refusing to start coding agent in unregistered git worktree: /tmp/wt" }, REVIEW_LANES)).toBe(true); - expect(isRecoverableMissingWorktreeReviewFailureWithProgress({ ...baseTask, paused: true, error: "Refusing to start coding agent in missing worktree: /tmp/wt" })).toBe(false); - expect(isRecoverableMissingWorktreeReviewFailureWithProgress({ ...baseTask, error: "other" })).toBe(false); - expect(isRecoverableMissingWorktreeReviewFailureWithProgress({ ...baseTask, steps: [{ id: "s2", title: "y", status: "pending" }] as any, error: "Refusing to start coding agent in missing worktree: /tmp/wt" })).toBe(false); + expect(isRecoverableMissingWorktreeReviewFailureWithProgress({ ...baseTask, paused: true, error: "Refusing to start coding agent in missing worktree: /tmp/wt" }, REVIEW_LANES)).toBe(false); + expect(isRecoverableMissingWorktreeReviewFailureWithProgress({ ...baseTask, error: "other" }, REVIEW_LANES)).toBe(false); + expect(isRecoverableMissingWorktreeReviewFailureWithProgress({ ...baseTask, steps: [{ id: "s2", title: "y", status: "pending" }] as any, error: "Refusing to start coding agent in missing worktree: /tmp/wt" }, REVIEW_LANES)).toBe(false); const errors = [ "Refusing to start coding agent in missing worktree: /tmp/wt", @@ -123,9 +136,9 @@ describe("RestartRecoveryCoordinator", () => { for (const error of errors) { const withProgressTask = { ...baseTask, error }; const noProgressTask = { ...baseTask, steps: [{ id: "s2", title: "y", status: "pending" }] as any, error }; - expect(isRecoverableMissingWorktreeReviewFailureWithProgress(withProgressTask)).toBe(true); - expect(isRecoverableMissingWorktreeReviewFailureNoProgress(noProgressTask)).toBe(true); - expect(isRecoverableMissingWorktreeReviewFailure(noProgressTask)).toBe(true); + expect(isRecoverableMissingWorktreeReviewFailureWithProgress(withProgressTask, REVIEW_LANES)).toBe(true); + expect(isRecoverableMissingWorktreeReviewFailureNoProgress(noProgressTask, REVIEW_LANES)).toBe(true); + expect(isRecoverableMissingWorktreeReviewFailure(noProgressTask, REVIEW_LANES)).toBe(true); } }); @@ -139,13 +152,13 @@ describe("RestartRecoveryCoordinator", () => { for (const status of ["merging", "merging-pr", "merging-fix"] as const) { const task = { ...baseTask, status }; - expect(isMergeActiveMissingWorktreeSessionStartFailure(task)).toBe(true); - expect(isRecoverableMissingWorktreeReviewFailure(task)).toBe(true); + expect(isMergeActiveMissingWorktreeSessionStartFailure(task, REVIEW_LANES)).toBe(true); + expect(isRecoverableMissingWorktreeReviewFailure(task, REVIEW_LANES)).toBe(true); } - expect(isMergeActiveMissingWorktreeSessionStartFailure({ ...baseTask, status: "failed" })).toBe(false); - expect(isMergeActiveMissingWorktreeSessionStartFailure({ ...baseTask, status: null as any })).toBe(false); - expect(isMergeActiveMissingWorktreeSessionStartFailure({ ...baseTask, status: "merging", error: "ordinary merge failure" })).toBe(false); + expect(isMergeActiveMissingWorktreeSessionStartFailure({ ...baseTask, status: "failed" }, REVIEW_LANES)).toBe(false); + expect(isMergeActiveMissingWorktreeSessionStartFailure({ ...baseTask, status: null as any }, REVIEW_LANES)).toBe(false); + expect(isMergeActiveMissingWorktreeSessionStartFailure({ ...baseTask, status: "merging", error: "ordinary merge failure" }, REVIEW_LANES)).toBe(false); }); it("requeues interrupted failed tasks with no progress, then resumes remaining orphans", async () => { diff --git a/packages/engine/src/restart-recovery-coordinator.ts b/packages/engine/src/restart-recovery-coordinator.ts index 4150d56d88..d80d211529 100644 --- a/packages/engine/src/restart-recovery-coordinator.ts +++ b/packages/engine/src/restart-recovery-coordinator.ts @@ -81,9 +81,9 @@ Optional, defaulting to the legacy id, so no existing caller or test changes beh */ export function isRecoverableMissingWorktreeReviewFailureWithProgress( task: Task, - reviewColumns?: ReadonlySet, + reviewColumns: ReadonlySet, ): boolean { - return (reviewColumns ? reviewColumns.has(task.column) : task.column === "in-review") + return reviewColumns.has(task.column) && !task.paused && task.status === "failed" && isMissingWorktreeSessionStartFailure(task.error) @@ -92,9 +92,9 @@ export function isRecoverableMissingWorktreeReviewFailureWithProgress( export function isRecoverableMissingWorktreeReviewFailureNoProgress( task: Task, - reviewColumns?: ReadonlySet, + reviewColumns: ReadonlySet, ): boolean { - return (reviewColumns ? reviewColumns.has(task.column) : task.column === "in-review") + return reviewColumns.has(task.column) && !task.paused && task.status === "failed" && isMissingWorktreeSessionStartFailure(task.error) @@ -106,9 +106,9 @@ const MERGE_ACTIVE_MISSING_WORKTREE_STATUS_SET = new Set(MERGE_ACTIVE_MI export function isMergeActiveMissingWorktreeSessionStartFailure( task: Task, - reviewColumns?: ReadonlySet, + reviewColumns: ReadonlySet, ): boolean { - return (reviewColumns ? reviewColumns.has(task.column) : task.column === "in-review") + return reviewColumns.has(task.column) && !task.paused && typeof task.status === "string" && MERGE_ACTIVE_MISSING_WORKTREE_STATUS_SET.has(task.status) @@ -152,7 +152,7 @@ export function isInReviewMissingWorktreeSessionStartFailure( export function isRecoverableMissingWorktreeReviewFailure( task: Task, - reviewColumns?: ReadonlySet, + reviewColumns: ReadonlySet, ): boolean { /* The combiner threads the set to all three, so a caller cannot convert the outer question and leave one of the three inner ones on the legacy id — the half-conversion shape this program keeps finding. */