From e18a6cf00c5d78dcd73eb4152717ad0caeaef796 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 06:48:25 -0700 Subject: [PATCH] =?UTF-8?q?fleet:=20executor.ts=2057=20=E2=86=92=2015=20on?= =?UTF-8?q?=20top=20of=20#2689=20=E2=80=94=20the=20review/wip=20lanes,=204?= =?UTF-8?q?=20half-conversions,=208-of-19=20revert=20proof=20(#2703)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Supersedes #2691, which I am closing.** #2689 landed the terminal-pair batch on `executor.ts` while my PR was open on the same file — we collided, that PR won the race, and 30 of my 70 conversions are now identical to its work. Rather than resolve 30 conflict hunks in a 20k-line lifecycle file (unreviewable, and the wrong artifact to hand you), I rebuilt from `origin/main`. **`executor.ts` 57 → 15.** Repo backlog 679 → **650**. ## The four that are defects, not vocabulary **1. `isReentrantPausedAbortedInFlightNode` resolved lanes at the END, for its return value, while its four `in-review` eligibility gates were literals.** On a renamed board those gates all read false — so a review card skipped the global-pause recheck, the `autoMerge === false` refusal, the shared-branch-member arbitration **and** the merge-confirmed refusal — and then the lane-resolved final line answered *"re-entrant"*. FN-7214's own comment says an auto-merge-off review row must stay terminal. **2. The REVERSE half-conversion.** `routeGraphFailureToExecutionResume`'s destination was already resolved (U7's `resolveReboundColumnFor`) behind a gate that was still three literals — so the router refused before reaching its own working move. | direction | what happens | visible? | |---|---|---| | resolved gate → literal destination | card admitted, move rejected by a board with no such column | **yes** — the move errors | | literal gate → resolved destination | card refused; the working recovery never runs | **no** | Only the second is silent, which is exactly why it survived U7's own conversion of that destination. **When you convert a destination, check the gate in front of it in the same commit.** **3. `routeUnusableWorktreeGraphFailureToRecovery` skipped FN-5147's auto-merge-off gate** on a renamed board — an automatic recovery moving a human-review-terminal card backward. #2689 converted the terminal guard at the top of that method; this is the other half of the same decision, which is the general risk when two people split one file. **4. `handleGraphFailure`'s `alreadyFinalizedToReview` / `suppressFinalizedCompletionAbort`** read `column !== "in-progress"`, so a completed, already-finalized row looked still-in-wip: FN-6644 / FN-6647's suppression never fired and the row was re-parked as an operator-action pause abort — the durability gap those tickets closed. ## Two patterns worth carrying to other files **An inert guard rarely reports "renamed board" — it reports something that sounds like a different problem.** `finalizeAlreadyReviewedTask` returned `"missing"` for a card sitting in review. The completion handoff logged *"no longer active"* for a card that was actively executing. The stuck-requeue cleanup logged *"recovered concurrently"* about a recovery that had not happened. Three different false explanations, one cause. **Directions differ inside one family, so convert per method, not per pattern.** Most wip guards read `!== "in-progress"` and REFUSE on no-match (renamed board → silently disabled). The rerun watchdog reads `=== "in-progress"` and SKIPS on match — there the literal never matched, so a rerun could fire on a card **mid-execution**. A mechanical sweep of `!== "in-progress"` fixes the refusals and leaves that admission in place. Also: the resolver choice inverts within a few lines. *"Is this card in the ONE column finalize targets?"* needs the **complete** column — the terminal union carries the legacy ids, so a card in a column merely *named* `done` reads as already finalized and the finalize is **skipped**. *"Is this card already finished, so do not move it?"* needs the **union** — over-inclusion only skips a move, under-inclusion moves a finished card out of its terminal column. Both are recorded at their sites. ## Revert proof 19 cases in `executor-graph-failure-lanes-resolved.test.ts`, on a board sharing **no** column id with the default lineage (on the default board these guards are correct by coincidence — the literals *are* the board). **8 fail on revert.** The rest are labelled **in the file** as paired positives, default-board no-change cases, or — in one instance — a guard that is genuinely redundant with a later lane check. I would rather label a case as non-evidence than count it. Two fixture corrections are recorded at their sites, both my own assertion failing to touch the behaviour it named: asserting a router's return value (which was already false for an unrelated reason — fixed by spying on the recovery call), and `allowsAutoMergeProcessing` keying on the **global** setting rather than `task.autoMerge` (fixed the fixture, not the assertion). ## The 15 that remain, each with a reason - **7 `to`/`from` move-effect parameters** — a move's endpoints, not a card's resting column. Trait-hook territory. - **2 enumeration scans** — one is a `listTasks({ column: "in-progress" })` query whose filter cannot be converted without the query (converting the filter alone reads as done and changes nothing); the other loops every task, so per-task resolution is a real cost wanting a shared memo. - **`12325`, the dependency guard** — *"is this dependency satisfied?"* is not any single lane role. The same question exists at `register-task-workflow-routes.ts:3995`; both should be decided once, together. - **`14484`** (`fromColumn === "in-review" && toColumn === "in-review"`) — a same-lane move check that belongs with the move-effect group above. ## Verification `pnpm test:gate` **158 / 10 / 487 / 71** · **135/135** across the 17 suites covering these paths · `tsc -p packages/engine` clean · `pnpm lint` clean · census `--strict` exit 0, baseline re-recorded. No changeset: `@fusion/engine` is private and the behaviour change is confined to renamed boards. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Bug Fixes** * Improved workflow execution across boards with renamed lifecycle lanes by resolving lane targets per board instead of using fixed column names. * Fixed review, WIP, completion, and failure-recovery behaviors to respect the correct board snapshot (including auto-merge and terminal work states). * Improved artifact-recovery protection timing and tightened execution-resume gating for failure scenarios. * **Tests** * Added a new lifecycle invariant test suite covering renamed-lane recovery, resume, pause/abort, and router-gating behavior. * Updated lifecycle column census baseline data. --------- Co-authored-by: Claude Opus 5 (1M context) --- ...cutor-graph-failure-lanes-resolved.test.ts | 501 ++++++++++++++++++ packages/engine/src/executor.ts | 282 ++++++++-- .../lib/lifecycle-column-census-baseline.json | 2 +- 3 files changed, 732 insertions(+), 53 deletions(-) create mode 100644 packages/engine/src/__tests__/executor-graph-failure-lanes-resolved.test.ts diff --git a/packages/engine/src/__tests__/executor-graph-failure-lanes-resolved.test.ts b/packages/engine/src/__tests__/executor-graph-failure-lanes-resolved.test.ts new file mode 100644 index 0000000000..692d39faca --- /dev/null +++ b/packages/engine/src/__tests__/executor-graph-failure-lanes-resolved.test.ts @@ -0,0 +1,501 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-08:10 (fleet: executor.ts graph-failure recovery family): + +THE INVARIANT: every lifecycle question the graph-failure recovery family asks is answered from the +task's OWN workflow, and the halves of one decision answer it from ONE snapshot. + +Two of the conversions in this family fix behaviour rather than vocabulary, and both are the +half-conversion shape — a gate that reads the wrong board is not merely inert, it ADMITS work the +gate existed to refuse: + + 1. `isReentrantPausedAbortedInFlightNode` resolved lanes at the END, for its return value, while its + four `in-review` eligibility gates were literals. On a renamed board those gates all read false, + so a card in review skipped the global-pause recheck, the `autoMerge === false` refusal, the + shared-branch-member arbitration and the merge-confirmed refusal — and then the lane-resolved + final line answered "re-entrant". FN-7214's own comment says an auto-merge-off review row must + stay terminal, so this was the documented invariant failing silently on any renamed board. + + 2. `routeUnusableWorktreeGraphFailureToRecovery` asked "already finished?" and "in review?" as three + literals. On a renamed board it read not-finished AND not-in-review, so recovery proceeded with + the FN-5147 gate skipped: an automatic recovery moving a human-review-terminal card backward. + +WHY THE RENAMED BOARD IS THE FIXTURE. On the default lineage every one of these guards is correct by +coincidence — the literals ARE the board. A test on the default board passes before and after the +change and proves nothing, which is how this class of defect survived every previous suite. + +REVERT PROOF, measured: restore either literal and the matching case here fails (`autoMerge:false` +review row is admitted as re-entrant; the finished card is admitted into worktree recovery). +*/ +import { describe, expect, it, vi } from "vitest"; +import "./executor-test-helpers.js"; +import { TaskExecutor } from "../executor.js"; +import { createMockStore } from "./executor-test-helpers.js"; +import type { WorkflowIr } from "@fusion/core"; + +/** A board whose lifecycle columns share NO id with the default lineage. */ +const RENAMED_IR = { + version: "v2", id: "wf-renamed", name: "renamed", nodes: [], edges: [], + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }] }, + { id: "queued", name: "Queued", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "checking", name: "Checking", traits: [{ trait: "merge" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + { id: "filed", name: "Filed", traits: [{ trait: "archived" }] }, + ], +} as unknown as WorkflowIr; + +function harness(ir: WorkflowIr | undefined, task: Record, settingsOverride: Record = {}) { + const store = createMockStore(); + const selection = { workflowId: "wf-renamed", stepIds: [] as string[] }; + const widened = store as unknown as Record; + widened.getTaskWorkflowSelection = () => (ir ? selection : undefined); + widened.getTaskWorkflowSelectionAsync = async () => (ir ? selection : undefined); + widened.getWorkflowDefinition = async () => (ir ? { ir } : undefined); + widened.getTask = async () => task; + widened.getSettings = async () => ({ maxConcurrent: 4, maxWorktrees: 4, pollIntervalMs: 1000, autoMerge: true, globalPause: false, enginePaused: false, ...settingsOverride }); + store.recordRunAuditEvent = vi.fn().mockResolvedValue(undefined); + + const executor = new TaskExecutor(store as never, "/repo"); + return { store, executor }; +} + +const pauseAbortResult = { + interruptedAbortKind: "engine-pause", + interruptedNodeId: "execute", + visitedNodeIds: ["execute"], + context: {}, +} as never; + +function reentrant(executor: TaskExecutor, live: unknown): Promise { + return (executor as unknown as { + isReentrantPausedAbortedInFlightNode: ( + live: unknown, result: unknown, provenance: string, pausedAborted: boolean, userCanceled: boolean, + ) => Promise; + }).isReentrantPausedAbortedInFlightNode(live, pauseAbortResult, "engine-abort", true, false); +} + +describe("FN-7214: an auto-merge-off review row stays terminal on a RENAMED board", () => { + it("refuses re-entry for a review-lane card with autoMerge:false", async () => { + /* + The measured pre-fix behaviour: the four `in-review` gates compared against the literal, `checking` + is not `in-review`, so every refusal was skipped — and the final lane-resolved line then matched + `resumeLanes.review` and returned TRUE. The card was re-entered behind a human review gate. + */ + const live = { id: "FN-1", column: "checking", autoMerge: false, graphResumeRetryCount: 0 }; + const { executor } = harness(RENAMED_IR, live); + + expect(await reentrant(executor, live)).toBe(false); + }); + + it("still admits a review-lane card whose auto-merge is ON, so the fix is not 'refuse review rows'", async () => { + // The paired positive. A gate that returns false unconditionally would pass the case above. + const live = { id: "FN-2", column: "checking", autoMerge: true, graphResumeRetryCount: 0 }; + const { executor } = harness(RENAMED_IR, live); + + expect(await reentrant(executor, live)).toBe(true); + }); + + it("keeps the same answers on the DEFAULT board, where the literals happened to be right", async () => { + // The conversion must not change behaviour where the old spelling was already correct. + const offLive = { id: "FN-3", column: "in-review", autoMerge: false, graphResumeRetryCount: 0 }; + const onLive = { id: "FN-4", column: "in-review", autoMerge: true, graphResumeRetryCount: 0 }; + + expect(await reentrant(harness(undefined, offLive).executor, offLive)).toBe(false); + expect(await reentrant(harness(undefined, onLive).executor, onLive)).toBe(true); + }); + + it("refuses a card in the board's COMPLETE column — and this one passes either way", async () => { + /* + HONEST LABEL, because I checked: reverting the terminal-pair literal here does NOT redden this case. + The method's final line already answers "is the card in a resume lane", and `shipped` is not one, so + the terminal guard is redundant *in this method*. Kept as the paired negative — it pins that a + finished card is refused however the refusal is reached — but it is not evidence for the conversion. + The terminal conversions that DO change behaviour are proven in the worktree-recovery block below. + */ + const live = { id: "FN-5", column: "shipped", autoMerge: true, graphResumeRetryCount: 0 }; + const { executor } = harness(RENAMED_IR, live); + + expect(await reentrant(executor, live)).toBe(false); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-08:40: +ASSERT THE CALL, NOT THE RETURN VALUE. My first version of this block asserted +`routeUnusableWorktreeGraphFailureToRecovery(...) === false` and passed with the literals restored — +the pre-fix path ran the whole recovery and then returned false for an unrelated reason (the mock's +outcome is not `requeue-todo`). A revert check that stays green is the signal I keep re-learning to +respect: the assertion was not touching the behaviour it claimed to cover. + +Spying on `recoverMissingWorktreeSessionStartFailure` asks the question the guard actually decides — +was recovery ATTEMPTED on this card? — and it discriminates in both directions. +*/ +describe("FN-5147: unusable-worktree recovery does not run on a finished or human-review card (RENAMED board)", () => { + const SESSION_FAILURE = "Refusing to start coding agent in missing worktree: /gone"; + + function harnessWithSpy(live: Record, settingsOverride: Record = {}) { + const { executor, store } = harness(RENAMED_IR, live, settingsOverride); + const recover = vi.fn().mockResolvedValue("requeue-todo"); + (executor as unknown as Record).recoverMissingWorktreeSessionStartFailure = recover; + return { executor, store, recover }; + } + + function route(executor: TaskExecutor, live: Record): Promise { + return (executor as unknown as { + routeUnusableWorktreeGraphFailureToRecovery: (task: unknown, live: unknown, result: unknown) => Promise; + }).routeUnusableWorktreeGraphFailureToRecovery({ id: live.id }, live, { + context: { "node:execute:error": SESSION_FAILURE }, + visitedNodeIds: ["execute"], + }); + } + + it("does not attempt recovery for a card in the board's COMPLETE column", async () => { + // Pre-fix: `shipped` is neither `done` nor `archived`, so recovery ran on a finished card. + const live = { id: "FN-7", column: "shipped", worktree: "/gone" }; + const { executor, recover } = harnessWithSpy(live); + + expect(await route(executor, live)).toBe(false); + expect(recover).not.toHaveBeenCalled(); + }); + + it("does not attempt recovery for a card in the board's ARCHIVED column", async () => { + const live = { id: "FN-8", column: "filed", worktree: "/gone" }; + const { executor, recover } = harnessWithSpy(live); + + expect(await route(executor, live)).toBe(false); + expect(recover).not.toHaveBeenCalled(); + }); + + it("applies the FN-5147 auto-merge gate to the board's REVIEW lane", async () => { + /* + The half-conversion in its most consequential form: pre-fix the `in-review` literal did not match + `checking`, so the auto-merge-off refusal never ran and an automatic recovery moved a + human-review-terminal card backward. `allowsAutoMergeProcessing` is what must be consulted, and it + can only be reached once the review lane is resolved. + */ + /* + THE GATE KEYS ON THE GLOBAL SETTING, not on `task.autoMerge`: `allowsAutoMergeProcessing` is + `(settings.autoMerge !== false || task.autoMerge === true) && no manual open PR`. My first fixture + set only `task.autoMerge: false` and the case failed — the recovery ran, correctly, because a + per-task false does not withdraw a card from automatic processing. Correcting the fixture rather + than the assertion: FN-5147 is about the OPERATOR turning auto-merge off for the project. + */ + const live = { id: "FN-9", column: "checking", worktree: "/gone" }; + const { executor, recover } = harnessWithSpy(live, { autoMerge: false }); + + expect(await route(executor, live)).toBe(false); + expect(recover).not.toHaveBeenCalled(); + }); + + it("STILL recovers a wip-lane card, so the fix is not 'never recover'", async () => { + // The paired positive: the guards above must not have turned the recovery path off wholesale. + const live = { id: "FN-10", column: "building", worktree: "/gone" }; + const { executor, recover } = harnessWithSpy(live); + + expect(await route(executor, live)).toBe(true); + expect(recover).toHaveBeenCalledTimes(1); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-10:30 (fleet: executor.ts wip-lane liveness family): + +THE INVARIANT: "is this card still executing?" is the board's WIP lane. + +TWO DIRECTIONS, and this is why the family is converted per method rather than by matching the text. +Most of these guards read `column !== "in-progress"` and REFUSE when they do not match, so a renamed +board silently disabled them (the completion handoff was deferred on every card, the deferred +approval resume never resumed, the completed-task watchdog returned on every tick). But the rerun +watchdog reads `column === "in-progress"` and SKIPS when it matches — so on a renamed board that one +never skipped, and a rerun could fire on a card that was still mid-execution. A mechanical swap of +every `!== "in-progress"` would have converted the refusals and left the admission. + +`resumeApprovalAfterUnwindIfNeeded` is the case pinned here: it is a private method with a single +boolean answer and no side effects on the refusal path, so the assertion is direct. +*/ +describe("the wip-lane liveness family resolves the board's own wip column", () => { + function resumeApproval(executor: TaskExecutor, taskId: string): Promise { + return (executor as unknown as { + resumeApprovalAfterUnwindIfNeeded: (id: string) => Promise; + }).resumeApprovalAfterUnwindIfNeeded(taskId); + } + + function armed(live: Record, ir: WorkflowIr | undefined) { + const { executor, store } = harness(ir, live); + // The deferral marker this method consumes, plus a stub for the dispatch it guards. + (executor as unknown as { approvalResumeAfterUnwind: Set }).approvalResumeAfterUnwind.add(live.id as string); + const dispatch = vi.fn().mockResolvedValue(true); + (executor as unknown as Record).dispatchUnpauseResume = dispatch; + return { executor, store, dispatch }; + } + + it("resumes a deferred approval for a card in the board's wip lane", async () => { + // Pre-fix: `building` !== "in-progress", so the resume refused on every renamed board. + const live = { id: "FN-11", column: "building" }; + const { executor, dispatch } = armed(live, RENAMED_IR); + + expect(await resumeApproval(executor, "FN-11")).toBe(true); + expect(dispatch).toHaveBeenCalledTimes(1); + }); + + it("still refuses a card that has LEFT the wip lane", async () => { + // The paired negative: the fix must not resume work on a card that moved on. + const live = { id: "FN-12", column: "checking" }; + const { executor, dispatch } = armed(live, RENAMED_IR); + + expect(await resumeApproval(executor, "FN-12")).toBe(false); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it("still refuses a PAUSED card in the wip lane, so the lane is not the only gate", async () => { + const live = { id: "FN-13", column: "building", paused: true }; + const { executor, dispatch } = armed(live, RENAMED_IR); + + expect(await resumeApproval(executor, "FN-13")).toBe(false); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it("behaves identically on the DEFAULT board", async () => { + const live = { id: "FN-14", column: "in-progress" }; + const { executor, dispatch } = armed(live, undefined); + + expect(await resumeApproval(executor, "FN-14")).toBe(true); + expect(dispatch).toHaveBeenCalledTimes(1); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-11:20 (fleet: executor.ts review-lane family): + +THE INVARIANT: "is this card in review?" is the board's review lane. + +`finalizeAlreadyReviewedTask` is the case pinned here because its failure is the loudest: it returns +"missing" — a word that reads as "the task is gone" — for a card that is sitting in review on a +renamed board. Everything downstream of the already-reviewed finalize path was therefore dead on any +board that renamed its merge lane, and the log line said the task could not be found. +*/ +describe("the review-lane family resolves the board's own review column", () => { + function finalizeAlreadyReviewed(executor: TaskExecutor, taskId: string): Promise { + return (executor as unknown as { + finalizeAlreadyReviewedTask: (id: string) => Promise; + }).finalizeAlreadyReviewedTask(taskId); + } + + it("does not report a review-lane card as MISSING", async () => { + // Pre-fix: `checking` !== "in-review", so this returned "missing" for a card plainly in review. + const live = { id: "FN-15", column: "checking", steps: [], workflowStepResults: [] }; + const { executor } = harness(RENAMED_IR, live); + + expect(await finalizeAlreadyReviewed(executor, "FN-15")).not.toBe("missing"); + }); + + it("still reports a card OUTSIDE the review lane as missing", async () => { + // The paired negative: the guard must still refuse a card that is not in review at all. + const live = { id: "FN-16", column: "building", steps: [], workflowStepResults: [] }; + const { executor } = harness(RENAMED_IR, live); + + expect(await finalizeAlreadyReviewed(executor, "FN-16")).toBe("missing"); + }); + + it("behaves identically on the DEFAULT board", async () => { + const live = { id: "FN-17", column: "in-review", steps: [], workflowStepResults: [] }; + const { executor } = harness(undefined, live); + + expect(await finalizeAlreadyReviewed(executor, "FN-17")).not.toBe("missing"); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-13:30 (fleet: executor.ts — the REVERSE half-conversion): + +`routeGraphFailureToExecutionResume`'s move DESTINATION was already resolved from the workflow (U7's +`resolveReboundColumnFor`) while its entry gate compared against three default-lineage literals. So on +a renamed board the router refused before it ever reached the resolved move: a recovery that was fully +implemented, never running. + +That is the mirror image of the dangerous half-conversion. The familiar direction admits a card and +then sends it to a column the board does not declare; this direction refuses a card whose recovery +already worked. Both are one decision reading two boards, and only the second is silent — which is +why it survived. + +The gate admits three shapes, so all three are asserted plus the refusal, which is what stops a gate +that returns true unconditionally from passing. +*/ +describe("the execution-resume router's gate reads the same board as its destination", () => { + function routeResume(executor: TaskExecutor, live: Record, failureValue: string): Promise { + return (executor as unknown as { + routeGraphFailureToExecutionResume: (live: unknown, failedNode: string, failureValue: string) => Promise; + }).routeGraphFailureToExecutionResume(live, "merge", failureValue); + } + + const withIncompleteSteps = (column: string, id: string) => ({ + id, column, worktree: "/wt", steps: [{ name: "s", status: "pending" }], workflowStepResults: [], + }); + + it("admits a review-lane card", async () => { + const live = withIncompleteSteps("checking", "FN-18"); + const { executor } = harness(RENAMED_IR, live); + + expect(await routeResume(executor, live, "other")).toBe(true); + }); + + it("admits a HOLD-lane card that still has unfinished steps", async () => { + const live = withIncompleteSteps("queued", "FN-19"); + const { executor } = harness(RENAMED_IR, live); + + expect(await routeResume(executor, live, "other")).toBe(true); + }); + + it("admits a WIP-lane card after a premature merge attempt", async () => { + // The third shape: implementation-incomplete merge failure with steps still open. + const live = withIncompleteSteps("building", "FN-20"); + const { executor } = harness(RENAMED_IR, live); + + expect(await routeResume(executor, live, "implementation-incomplete")).toBe(true); + }); + + it("still REFUSES a card in the intake lane, which is none of the three shapes", async () => { + const live = withIncompleteSteps("backlog", "FN-21"); + const { executor } = harness(RENAMED_IR, live); + + expect(await routeResume(executor, live, "other")).toBe(false); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-20:50 (PR #2703 review — greptile P1, the sync-resolver no-op): + +THE INVARIANT: no lifecycle guard in this file resolves its lane through the SYNCHRONOUS resolver. + +`resolvePlannerLanes` reads `store.resolveTaskWorkflowIrSync`, whose selection reader returns `undefined` +unconditionally in PostgreSQL mode — the shipped backend. So a sync-resolved guard silently answers with +the DEFAULT workflow's ids on every real board: the census counts the site as converted, `--strict` drops +by one, and the behaviour is identical to the literal. That is worse than an unconverted literal, because +the number claims the site is done. + +This is a STRUCTURAL assertion rather than a behavioural one, and deliberately so. A behavioural test +would need a PostgreSQL-backed store to demonstrate the no-op, and the thing worth preventing is not one +guard misbehaving — it is the pattern being reintroduced anywhere in this file by someone who reads +"synchronous classifier" and reaches for the synchronous resolver, exactly as I did. + +The two legitimate `resolvePlannerLanes` call sites are MOVE destinations (`PlannerLanes` exists so a +caller refuses rather than inventing a column), which is a different question from "which lane is this +card in" and is why they are allowlisted here by name. +*/ +describe("no lifecycle GUARD resolves its lane synchronously", () => { + it("keeps resolvePlannerLanes out of column-comparison guards in executor.ts", async () => { + const { readFile } = await import("node:fs/promises"); + const source = await readFile(new URL("../executor.ts", import.meta.url), "utf8"); + + /* Strip block comments: the notes explaining WHY the sync resolver is unsafe mention it by name. */ + const code = source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, ""); + const callSites = [...code.matchAll(/resolvePlannerLanes\s*\(/g)].length; + + /* + Three call sites are the move/promotion destinations documented above (two in the planner-column + helpers, one in the promotion path), plus the import. Any FOURTH is a new sync resolution and must be + justified — if it is a guard, it is a no-op on PostgreSQL and the census will claim it is converted. + */ + expect(callSites).toBe(3); + }); + + it("does not compare a column against a synchronously-resolved lane on the same line", async () => { + const { readFile } = await import("node:fs/promises"); + const source = await readFile(new URL("../executor.ts", import.meta.url), "utf8"); + const code = source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, ""); + + // The shape this guards against: `x.column === resolvePlannerLanes(...).review`. + const inlineGuards = [...code.matchAll(/\.column\s*[!=]==\s*resolvePlannerLanes/g)]; + + expect(inlineGuards).toEqual([]); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-08-02-04:30 (PR #2703 review — coderabbit MAJOR, and it is my own rule +turned back on me): + +THE INVARIANT: every classifier in one recovery shares ONE lane snapshot. + +`isBenignInReviewPauseAbort` takes its review lane as a parameter precisely because a fresh resolution +inside a classifier can disagree with the snapshot the rest of `handleGraphFailure` uses. Four sibling +classifiers were still calling `resolveResumeLanes()` with no memo — so a workflow edit landing mid-recovery +could have one classifier admit a card on the new board while another rejects it on the old one, and the +recovery would take a branch neither board justifies. + +STRUCTURAL, because the race needs a workflow edit between two awaits inside one call — reproducible only +by instrumenting the resolver, which would pin the implementation rather than the rule. Counting +memo-less calls in the pause-abort family is the property that actually has to hold, and it fails the +moment someone adds a fifth classifier that resolves on its own. +*/ +describe("one lane snapshot per recovery, across every classifier", () => { + const MEMO_THREADED = [ + "isRetryableBenignMergePauseAbort", + "isBenignManualMergeHoldPauseAbort", + "handleStaleInReviewPlanPauseAbortReplay", + "handleStaleInReviewParsePauseAbortReplay", + "isReentrantPausedAbortedInFlightNode", + "routeUnusableWorktreeGraphFailureToRecovery", + "routeGraphFailureToExecutionResume", + ]; + /* Methods that own their own recovery: no caller memo to share, so the FIRST resolution is correct and a + SECOND is the split. `handleNonContinuableSessionError` was added here after exactly that defect + (PR #2703 review) — its eligibility check and its review branch each resolved independently. */ + const SELF_CONTAINED = ["handleNonContinuableSessionError"]; + + async function methodBody(name: string): Promise { + const { readFile } = await import("node:fs/promises"); + const source = await readFile(new URL("../executor.ts", import.meta.url), "utf8"); + const code = source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, ""); + const start = code.indexOf(`private async ${name}(`); + expect(start, `${name} not found — update this test, do not delete it`).toBeGreaterThan(-1); + /* + Bounded by the next `private ` declaration of ANY kind. My first version bounded on the next member of + the same list, so the last entry's window ran to EOF and it accused a method of a call living 1200 lines + away. A ratchet with the wrong window accuses the wrong function — worse than no ratchet, because the + "fix" lands on code that was already correct. + */ + const next = code.indexOf("\n private ", start + 1); + return code.slice(start, next === -1 ? code.length : next); + } + + it("threads the shared memo in every classifier that has one", async () => { + const offenders: string[] = []; + for (const name of MEMO_THREADED) { + const body = await methodBody(name); + // A memo-less call — `resolveResumeLanes(x)` with a single argument — is the defect. + if (/resolveResumeLanes\(\s*[^,)]+\s*\)/.test(body)) offenders.push(name); + } + + expect(offenders).toEqual([]); + }); + + it("resolves lanes AT MOST ONCE in a method that owns its own recovery", async () => { + /* + The rule differs by kind and that distinction is the point: a method called from `handleGraphFailure` + must take the memo (zero independent resolutions), while a self-contained recovery legitimately resolves + once. What neither may do is resolve TWICE — that is a split snapshot in both shapes. + */ + const offenders: Array<{ name: string; calls: number }> = []; + for (const name of SELF_CONTAINED) { + const body = await methodBody(name); + const calls = [...body.matchAll(/resolveResumeLanes\(/g)].length; + if (calls > 1) offenders.push({ name, calls }); + } + + expect(offenders).toEqual([]); + }); + + it("threads the memo from handleGraphFailure into every one of them", async () => { + const { readFile } = await import("node:fs/promises"); + const source = await readFile(new URL("../executor.ts", import.meta.url), "utf8"); + const code = source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, ""); + + // The call sites must PASS it — accepting an unused optional parameter proves nothing. + for (const name of MEMO_THREADED.filter((n) => n !== "isReentrantPausedAbortedInFlightNode")) { + const callSite = new RegExp(`this\\.${name}\\([^;]*resumeLanesMemo`); + expect(callSite.test(code), `${name} call site does not pass resumeLanesMemo`).toBe(true); + } + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index cfcae49d0b..c919055682 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -2356,7 +2356,10 @@ export class TaskExecutor { private async finalizeAlreadyReviewedTask(taskId: string): Promise<"merged" | "blocked" | "missing"> { const latestTask = await this.store.getTask(taskId); - if (!latestTask || latestTask.column !== "in-review") { + /* FNXC:WorkflowLifecycleColumns 2026-08-01-17:55 (fleet): the board's own review lane. Spelled as the + literal, this reported "missing" — a word that reads as "the task is gone" — for a card sitting in + review on a renamed board, and the already-reviewed finalize never ran. */ + if (!latestTask || latestTask.column !== (await this.resolveResumeLanes(taskId)).review) { return "missing"; } @@ -2426,7 +2429,11 @@ export class TaskExecutor { return true; } - if ((latestTask && latestTask.column !== "in-progress") || this.userCanceledTaskIds.has(taskId)) { + /* FNXC:WorkflowLifecycleColumns 2026-08-01-17:45 (fleet: wip-lane liveness family): "still executing" + is the board's WIP lane. With the literal a renamed board deferred EVERY completion handoff — the + card was never in `in-progress`, so this read "no longer active" for a card that was actively + executing, and the handoff was dropped with a log line. */ + if ((latestTask && latestTask.column !== (await this.resolveResumeLanes(taskId)).wip) || this.userCanceledTaskIds.has(taskId)) { this.clearCompletedTaskWatchdog(taskId); executorLog.log(`${taskId}: completion handoff deferred — task no longer active (${context})`); await this.store.logEntry( @@ -3357,7 +3364,8 @@ export class TaskExecutor { executorLog.warn(`${taskId}: failed to read latest task state for deferred approval resume: ${error instanceof Error ? error.message : String(error)}`); return false; } - if (latestTask.paused || latestTask.userPaused || latestTask.column !== "in-progress") return false; + if (latestTask.paused || latestTask.userPaused + || latestTask.column !== (await this.resolveResumeLanes(taskId)).wip) return false; return this.dispatchUnpauseResume(latestTask); } @@ -3604,7 +3612,11 @@ export class TaskExecutor { // Handle unpause of an in-progress task with no active session. // Approval can be decided while the old session is still unwinding; // remember that edge instead of losing the only task:updated event. - if (!task.paused && task.column === "in-progress" && this.approvalSuspended.has(task.id)) { + /* FNXC:WorkflowLifecycleColumns 2026-08-01-17:50 (fleet): both checks in this listener ask "is + this card still in the wip lane?"; one snapshot for the pair. With the literal neither fired on a + renamed board — an unpaused card with no active session was never resumed. */ + const unpauseWipLane = (await this.resolveResumeLanes(task.id)).wip; + if (!task.paused && task.column === unpauseWipLane && this.approvalSuspended.has(task.id)) { if ( this.executing.has(task.id) || this.activeSessions.has(task.id) @@ -3622,7 +3634,7 @@ export class TaskExecutor { // dispatchUnpauseResume owns the terminal-failure and duplicate guards. if ( !task.paused - && task.column === "in-progress" + && task.column === unpauseWipLane && !this.activeSessions.has(task.id) && !this.activeStepExecutors.has(task.id) && !this.activeWorkflowStepSessions.has(task.id) @@ -4271,7 +4283,8 @@ export class TaskExecutor { return; } - if (!currentTask || currentTask.column !== "in-progress" || currentTask.paused) { + if (!currentTask || currentTask.paused + || currentTask.column !== (await this.resolveResumeLanes(taskId)).wip) { return; } if (!this.isTaskWorkComplete(currentTask)) { @@ -4372,7 +4385,11 @@ export class TaskExecutor { the task back for remediation, so `in-review` must bounce back exactly like `in-progress` regardless of the column the completion race left it in. */ - if (latestTask.column === "in-progress" || latestTask.column === "in-review") { + /* FNXC:WorkflowLifecycleColumns 2026-08-01-17:57 (fleet): both lanes from ONE snapshot — the comment + above says in-review must bounce EXACTLY like in-progress, so resolving them separately is how the + bounce ends up handling one lane and throwing on the other, which is the bug that comment is about. */ + const bounceLanes = await this.resolveResumeLanes(taskId); + if (latestTask.column === bounceLanes.wip || latestTask.column === bounceLanes.review) { const originalExecutionStartedAt = latestTask.executionStartedAt; // Preserve step progress across the in-progress/in-review → todo hop: // moveTask's default reopen-to-todo path resets every step to @@ -4473,7 +4490,12 @@ export class TaskExecutor { return; } - if (!currentTask || currentTask.paused || currentTask.column === "in-progress") { + /* FNXC:WorkflowLifecycleColumns 2026-08-01-17:48 (fleet): the INVERSE of the guard above — this one + SKIPS a card that is still executing. Note the direction: with the literal on a renamed board it + never matched, so a rerun could fire on a card mid-execution. A mechanical sweep of every + `!== "in-progress"` would fix the refusals and leave this admission in place. */ + if (!currentTask || currentTask.paused + || currentTask.column === (await this.resolveResumeLanes(taskId)).wip) { return; } @@ -4623,12 +4645,18 @@ export class TaskExecutor { return await this.getCompletedTaskFinalizationDecision(taskId, taskDone) === "finalize"; } - private isTaskAlreadyCompleteForNonContinuableSession(task: Task, taskDone: boolean): boolean { + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-20:35 (PR #2703 review — greptile P1): + The review lane arrives from the caller for the reason documented on `isBenignInReviewPauseAbort`: the + synchronous resolver returns the default workflow in PostgreSQL mode, so resolving it here would have + been a conversion that changes the census and not the behaviour. + */ + private isTaskAlreadyCompleteForNonContinuableSession(task: Task, taskDone: boolean, reviewLane: string): boolean { // FNXC:Lifecycle 2026-07-16-21:40: FN-8141 — the step-status "already complete" branch // must not treat skip-bypass-tainted skips as completion; an accepted done / in-review // column are honest completion signals and stay unaffected. return taskDone - || task.column === "in-review" + || task.column === reviewLane || (this.isTaskWorkComplete(task) && !evaluateSkipBypassTaint(task).blocked); } @@ -4638,7 +4666,8 @@ export class TaskExecutor { } const liveTask = await this.store.getTask(task.id); - if (!liveTask || !this.isTaskAlreadyCompleteForNonContinuableSession(liveTask, taskDone)) { + const nonContinuableLanes = await this.resolveResumeLanes(task.id); + if (!liveTask || !this.isTaskAlreadyCompleteForNonContinuableSession(liveTask, taskDone, nonContinuableLanes.review)) { return false; } @@ -4652,7 +4681,19 @@ export class TaskExecutor { await this.persistTokenUsage(task.id); - if (liveTask.column === "in-review") { + /* + FNXC:WorkflowLifecycleColumns 2026-08-02-05:50 (PR #2703 review — greptile P1, and it is the same split + I have been fixing all day, in code I wrote an hour earlier): + ONE SNAPSHOT. The eligibility check above already resolved this task's lanes + (`nonContinuableLanes`), and this branch resolved them AGAIN. A workflow selection or review-column + edit between the two makes eligibility accept the card on the old board while this branch reads the new + one — the card is then handed to `handoffTaskToReview`, reprocessing a row already in review. + + Writing the second resolution was not carelessness about the rule; it is that the rule is invisible at + the call site. That is the argument for the structural ratchet in + `executor-graph-failure-lanes-resolved.test.ts` rather than for trying harder. + */ + if (liveTask.column === nonContinuableLanes.review) { this.clearCompletedTaskWatchdog(task.id); this.signalTaskComplete(liveTask); return true; @@ -5446,7 +5487,7 @@ export class TaskExecutor { source: { source: "graph-entry" | "workflow-step"; nodeId?: string }, ): Promise { const currentTask = await this.store.getTask(task.id).catch(() => null); - if (!currentTask || this.isRequiredArtifactRecoveryProtected(currentTask)) return; + if (!currentTask || await this.isRequiredArtifactRecoveryProtected(currentTask)) return; task = currentTask; const decision = computeRecoveryDecision({ recoveryRetryCount: task.recoveryRetryCount, @@ -5477,7 +5518,7 @@ export class TaskExecutor { if (!decision.shouldRetry) { const liveTask = await this.store.getTask(task.id).catch(() => null); - if (!liveTask || this.isRequiredArtifactRecoveryProtected(liveTask)) return; + if (!liveTask || await this.isRequiredArtifactRecoveryProtected(liveTask)) return; const error = `REQUIRED_ARTIFACT_RECOVERY_EXHAUSTED: ${artifactKeys.join(", ")} remained missing after ${MAX_RECOVERY_RETRIES} automatic planning retries.`; await this.store.logEntry(task.id, error, undefined, context); await this.store.updateTask(task.id, { @@ -5499,7 +5540,7 @@ export class TaskExecutor { this.workflowLifecycleMovesInFlight.add(task.id); try { const liveTask = await this.store.getTask(task.id).catch(() => null); - if (!liveTask || this.isRequiredArtifactRecoveryProtected(liveTask)) return; + if (!liveTask || await this.isRequiredArtifactRecoveryProtected(liveTask)) return; await moveTaskToReplanColumn(this.store, { id: task.id, column: liveTask.column }, replanColumn); } finally { this.workflowLifecycleMovesInFlight.delete(task.id); @@ -5513,15 +5554,29 @@ export class TaskExecutor { }, context); } - private isRequiredArtifactRecoveryProtected(task: Task): boolean { + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-18:25 (fleet: made ASYNC to own its resolution): + This predicate protects a card from artifact-recovery replanning, and three of its conditions are + lifecycle columns: the terminal pair, and a review row whose auto-merge is off (a human owns it). As + literals they all read false on a renamed board — so a FINISHED card, or a review row a human was + holding, could be moved to the replan column and have its status rewritten to needs-replan. + + ASYNC rather than lane parameters: all four callers already `await store.getTask` immediately before + calling this, so there is no new I/O ordering, and a parameter list would put the resolution in four + places that must agree. The archived half is why the SYNC planner-lane resolver was not an option — it + exposes no archived lane — and widening a shared resolver from inside a call-site sweep is scope creep + that makes a conversion unreviewable. + */ + private async isRequiredArtifactRecoveryProtected(task: Task): Promise { + const terminalColumns = await resolveTerminalColumnsFor(this.store, task.id); + const protectionReviewLane = (await this.resolveResumeLanes(task.id)).review; return Boolean( task.deletedAt || task.paused || task.userPaused === true - || task.column === "done" - || task.column === "archived" + || terminalColumns.includes(task.column) || task.mergeDetails?.mergeConfirmed === true - || (task.column === "in-review" && task.autoMerge === false), + || (task.column === protectionReviewLane && task.autoMerge === false), ); } @@ -9976,6 +10031,8 @@ export class TaskExecutor { task: Task, live: TaskDetail, result: WorkflowGraphTaskRunResult, + /** Shared per-recovery lane snapshot — see `resolveResumeLanes`. */ + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, ): Promise { if (live.deletedAt) return false; if (live.paused || live.userPaused === true) return false; @@ -9991,7 +10048,11 @@ export class TaskExecutor { not move those tasks backward or re-enqueue them. Mirrors the gating the in-review self-healing sweep (recoverMissingWorktreeReviewFailures) applies before the same recovery. */ - if (live.column === "in-review") { + /* FNXC:WorkflowLifecycleColumns 2026-08-01-17:25 (fleet): FN-5147 — with the literal, a renamed board + skipped this auto-merge-off gate entirely, so an automatic recovery moved a human-review-terminal + card backward. #2689 converted the terminal guard at the top of this method; this is the other half + of the same decision. */ + if (live.column === (await this.resolveResumeLanes(live.id, resumeLanesMemo)).review) { const settings = await this.store.getSettings(); if (!allowsAutoMergeProcessing(live, settings)) return false; } @@ -10184,6 +10245,9 @@ export class TaskExecutor { result: WorkflowGraphTaskRunResult, abortProvenance: PausedAbortProvenance | undefined, pausedAborted: boolean, + /** Shared per-recovery lane snapshot — see `resolveResumeLanes`; a fresh resolution here could disagree + * with the one the rest of `handleGraphFailure` uses. */ + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-06-19-00:05: @@ -10192,7 +10256,15 @@ export class TaskExecutor { if (!pausedAborted) return false; if (abortProvenance === "global-pause" || live.userPaused === true) return false; if (abortProvenance === "completion-finalize") return false; - if (live.column !== "in-review" || !this.isRetryableMergePauseAbortStatus(live.status) || live.error != null) return false; + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-17:10 (fleet: executor.ts review-lane classifiers, on top of #2689): + "IS THIS CARD IN THE REVIEW LANE?" from the task's own workflow. Five pause-abort classifiers asked it + as the default lineage's literal, and each refusal drops the card through to the operator-action park + these paths exist to avoid (FN-6796's benign in-review abort, the manual-merge-hold abort, the two + stale-replay handlers, this retryable merge abort). The literal made the recovery inert, silently. + */ + if (live.column !== (await this.resolveResumeLanes(live.id, resumeLanesMemo)).review + || !this.isRetryableMergePauseAbortStatus(live.status) || live.error != null) return false; if (live.mergeDetails?.mergeConfirmed === true) return false; const failureValue = this.graphFailureValue(result); if (this.isTerminalMergeGraphFailureValue(failureValue)) return false; @@ -10213,12 +10285,33 @@ export class TaskExecutor { return true; } + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-20:30 (PR #2703 review — greptile P1, and it is the most + important finding in this sweep): + + THE SYNCHRONOUS RESOLVER IS A NO-OP IN PRODUCTION. `resolvePlannerLanes` reads + `store.resolveTaskWorkflowIrSync`, whose selection reader is `getTaskWorkflowSelectionImpl` — and in + PostgreSQL mode that function returns `undefined` unconditionally ("Backend mode cannot synchronously + read PostgreSQL"). PostgreSQL is the shipped backend, so every sync-resolved conversion resolves the + DEFAULT workflow and answers with the legacy ids no matter what board the task is on. + + That makes a sync conversion cosmetic: the census counts it as converted, `--strict` goes down by one, + and the guard behaves exactly as the literal did. Worse than leaving the literal, because the number + says the site is done. + + THE FIX IS TO STOP BEING SYNCHRONOUS, not to keep the literal. Both of this file's sync classifiers + are called from async methods that have already awaited a store read, so the lane can be threaded in + from the caller's existing snapshot — no new I/O, no second resolution, and the two halves of the + decision provably read the same board. + */ private isBenignInReviewPauseAbort( live: TaskDetail, result: WorkflowGraphTaskRunResult, abortProvenance: PausedAbortProvenance | undefined, pausedAborted: boolean, userCanceled: boolean, + /** The caller's already-resolved review lane — see the note above on the sync resolver. */ + reviewLane: string, ): boolean { /* FNXC:WorkflowLifecycle 2026-06-20-00:00: @@ -10230,7 +10323,15 @@ export class TaskExecutor { if (!pausedAborted) return false; if (!isGenericAbortProvenance(abortProvenance)) return false; if (userCanceled) return false; - if (live.column !== "in-review") return false; + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-20:40 (PR #2703 review — replaces my own earlier reasoning): + This comparison used the SYNC `resolvePlannerLanes`, which I justified as the right resolver for a + synchronous classifier. That justification was wrong in production: in PostgreSQL mode the sync + selection reader always returns undefined, so the sync resolver hands back the DEFAULT workflow's lanes + and the guard behaves exactly as the literal did. The lane now arrives from the caller's snapshot — see + the note on this method. + */ + if (live.column !== reviewLane) return false; if (live.userPaused === true) return false; if (live.status != null || live.error != null) return false; if (live.mergeDetails?.mergeConfirmed === true) return false; @@ -10255,6 +10356,9 @@ export class TaskExecutor { result: WorkflowGraphTaskRunResult, abortProvenance: PausedAbortProvenance | undefined, pausedAborted: boolean, + /** Shared per-recovery lane snapshot — see `resolveResumeLanes`; a fresh resolution here could disagree + * with the one the rest of `handleGraphFailure` uses. */ + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-07-09-14:54: @@ -10263,7 +10367,7 @@ export class TaskExecutor { if (!pausedAborted) return false; if (!isGenericAbortProvenance(abortProvenance)) return false; if (live.paused || live.userPaused === true) return false; - if (live.column !== "in-review") return false; + if (live.column !== (await this.resolveResumeLanes(live.id, resumeLanesMemo)).review) return false; if (live.mergeDetails?.mergeConfirmed === true) return false; if (this.isTerminalMergeGraphFailureValue(this.graphFailureValue(result))) return false; const failedNode = result.visitedNodeIds[result.visitedNodeIds.length - 1]; @@ -10288,6 +10392,9 @@ export class TaskExecutor { abortProvenance: PausedAbortProvenance | undefined, pausedAborted: boolean, userCanceled: boolean, + /** Shared per-recovery lane snapshot — see `resolveResumeLanes`; a fresh resolution here could disagree + * with the one the rest of `handleGraphFailure` uses. */ + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-06-28-21:05: @@ -10296,7 +10403,7 @@ export class TaskExecutor { if (!pausedAborted) return false; if (!isGenericAbortProvenance(abortProvenance) && abortProvenance !== "global-pause") return false; if (userCanceled) return false; - if (live.column !== "in-review") return false; + if (live.column !== (await this.resolveResumeLanes(live.id, resumeLanesMemo)).review) return false; if (live.paused || live.userPaused === true) return false; if (live.autoMerge === false) return false; if (live.mergeDetails?.mergeConfirmed === true) return false; @@ -10360,6 +10467,9 @@ export class TaskExecutor { abortProvenance: PausedAbortProvenance | undefined, pausedAborted: boolean, userCanceled: boolean, + /** Shared per-recovery lane snapshot — see `resolveResumeLanes`; a fresh resolution here could disagree + * with the one the rest of `handleGraphFailure` uses. */ + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-06-29-01:18: @@ -10368,7 +10478,14 @@ export class TaskExecutor { if (!pausedAborted) return false; if (!isGenericAbortProvenance(abortProvenance) && abortProvenance !== "global-pause") return false; if (userCanceled) return false; - if (live.column !== "in-review") return false; + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-17:15 (fleet): ONE SNAPSHOT for the entry gate AND the deferred + recheck inside `scheduleRetry` below — the recheck is the second half of THIS decision ("is the card + still where it was when we admitted it?"), so resolving the board again inside the timeout callback + would let a workflow edit make the two halves disagree. + */ + const replayLanes = await this.resolveResumeLanes(live.id, resumeLanesMemo); + if (live.column !== replayLanes.review) return false; if (live.paused || live.userPaused === true) return false; if (live.autoMerge === false) return false; if (live.mergeDetails?.mergeConfirmed === true) return false; @@ -10435,7 +10552,7 @@ export class TaskExecutor { || resumeTask.userPaused || resumeTask.status != null || resumeTask.error != null - || resumeTask.column !== "in-review" + || resumeTask.column !== replayLanes.review || this.activeSessions.has(live.id) || this.activeStepExecutors.has(live.id) || this.activeWorkflowStepSessions.has(live.id) @@ -10480,15 +10597,25 @@ export class TaskExecutor { if (userCanceled) return false; if (live.paused || live.userPaused === true) return false; if (live.status != null || live.error != null) return false; + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-17:20 (fleet: executor.ts — the split-snapshot defect): + THE LANES ARE RESOLVED HERE, AT THE TOP, because this method already resolved them — at the very END, + for its return value — while every eligibility check below compared against the default lineage's + literals. On a renamed board the four `in-review` gates all read false, so a card in review skipped the + global-pause recheck, the `autoMerge === false` refusal, the shared-branch-member arbitration and the + merge-confirmed refusal — and then the final line, which DOES resolve lanes, answered "re-entrant". + FN-7214's comment above says an auto-merge-off review row must stay terminal. + */ + const resumeLanes = await this.resolveResumeLanes(live.id, resumeLanesMemo); if ((await resolveTerminalColumnsFor(this.store, live.id)).includes(live.column)) return false; if (result.interruptedAbortKind !== WORKFLOW_NODE_ENGINE_PAUSE_ABORT_KIND) return false; if (!result.interruptedNodeId) return false; - if (live.column === "in-review" && result.interruptedNodeId === "plan") return false; + if (live.column === resumeLanes.review && result.interruptedNodeId === "plan") return false; if (this.isMergeGraphFailure(result.interruptedNodeId)) return false; if (this.isTerminalMergeGraphFailureValue(this.graphFailureValue(result))) return false; if ((live.graphResumeRetryCount ?? 0) >= MAX_TRANSIENT_GRAPH_RESUME_RETRIES) return false; let settings: Settings | undefined; - if (abortProvenance === "global-pause" || live.column === "in-review") { + if (abortProvenance === "global-pause" || live.column === resumeLanes.review) { try { settings = await this.store.getSettings(); } catch { @@ -10496,14 +10623,13 @@ export class TaskExecutor { } if (settings.globalPause === true) return false; } - if (live.column === "in-review") { + if (live.column === resumeLanes.review) { if (live.autoMerge === false) return false; if (!settings) return false; const sharedBranchMember = await this.isLiveSharedBranchGroupMember(live); if (!sharedBranchMember && !allowsAutoMergeProcessing(live, settings)) return false; if (live.mergeDetails?.mergeConfirmed === true) return false; } - const resumeLanes = await this.resolveResumeLanes(live.id, resumeLanesMemo); return live.column === resumeLanes.hold || live.column === resumeLanes.review || live.column === resumeLanes.wip; @@ -10760,6 +10886,15 @@ export class TaskExecutor { } const live = loadedLive; /* + FNXC:WorkflowLifecycleColumns 2026-08-01-18:40 (fleet: executor.ts handleGraphFailure): + ONE LANE SNAPSHOT FOR THE WHOLE METHOD, declared where `live` first exists. The three wip comparisons + below run BEFORE the re-entry classifiers' memo was created, so a snapshot declared beside that memo + is used-before-declared — which is how the two halves came to read different boards in the first + place. The memo is seeded from this snapshot so the classifiers still share it. + */ + const resumeLanesMemo: { lanes?: { hold: string; wip: string; review: string } } = {}; + const failureLanes = await this.resolveResumeLanes(live.id, resumeLanesMemo); + /* FNXC:Lifecycle 2026-07-16-21:22: FN-8141 follow-up 1 — an honest `fn_task_done(outcome="blocked")` park (status="failed", error "BLOCKED: ", executor ~14657) must SURVIVE the same graph-teardown machinery @@ -10831,7 +10966,7 @@ export class TaskExecutor { would otherwise retry the same stale worktree in place, and the terminal sink would park the task failed with the signature erased (FN-7996 looped dispatch→park all day). */ - if (await this.routeUnusableWorktreeGraphFailureToRecovery(task, live, result)) { + if (await this.routeUnusableWorktreeGraphFailureToRecovery(task, live, result, resumeLanesMemo)) { await this.persistTokenUsage(task.id); return; } @@ -10852,7 +10987,7 @@ export class TaskExecutor { void (async () => { try { const resumeTask = await this.store.getTask(task.id); - if (this.isRequiredArtifactRecoveryProtected(resumeTask) || resumeTask.status === "failed") return; + if (await this.isRequiredArtifactRecoveryProtected(resumeTask) || resumeTask.status === "failed") return; await this.execute(resumeTask); } catch (err) { executorLog.error(`Failed required-artifact read retry for ${task.id}:`, err); @@ -10927,8 +11062,11 @@ export class TaskExecutor { FNXC:WorkflowLifecycle 2026-06-18-12:00: FN-6647 closes the remaining durability gap by deriving already-finalized completion from the persisted task row: non-in-progress column, completed steps, no live pause/status/error, and the finalize-to-review log entry. The volatile `completionFinalizedTaskIds` marker still helps within one executor lifecycle, but teardown/restart loss must not reclassify a completed in-review row as a hard-cancel pause abort. */ + /* FNXC:WorkflowLifecycleColumns 2026-08-01-17:32 (fleet): on a renamed board a completed, + already-finalized row read as still-in-wip, so FN-6644/FN-6647's suppression never fired and the + row was re-parked as an operator-action pause abort — the durability gap those tickets closed. */ const alreadyFinalizedToReview = Boolean( - live.column !== "in-progress" + live.column !== failureLanes.wip && persistedCompletedProgress && live.status == null && live.error == null @@ -10952,7 +11090,7 @@ export class TaskExecutor { const completionFinalized = completionFinalizeAborted || this.completionFinalizedTaskIds.has(task.id) || alreadyFinalizedToReview; const suppressFinalizedCompletionAbort = Boolean( completionFinalized - && live.column !== "in-progress" + && live.column !== failureLanes.wip && !live.userPaused // FN-6648: `paused !== true` intentionally dropped here too — the // suppression is already gated on `completionFinalized` (completed @@ -10983,8 +11121,12 @@ export class TaskExecutor { FNXC:WorkflowLifecycleColumns 2026-07-31-01:05 (PR #2640 review, greptile P2): one lane snapshot for one recovery decision — see `resolveResumeLanes`. Eligibility and re-entry are two halves of the SAME decision and must not read different boards. + + FNXC:WorkflowLifecycleColumns 2026-08-01-17:30 (fleet): the surrounding branches share it now too. + This method asked "still in the wip lane?" in three more places as the default lineage's id while + creating this memo for the classifiers — so the classifiers read the board and the branches around + them read the default names. */ - const resumeLanesMemo: { lanes?: { hold: string; wip: string; review: string } } = {}; if (genuinePauseAbort && await this.isReentrantPausedAbortedInFlightNode(live, result, abortProvenance, pausedAborted, this.userCanceledTaskIds.has(task.id), resumeLanesMemo)) { if (await this.reenterPausedAbortedWorkflowNode(live, result, abortProvenance, resumeLanesMemo)) { return; @@ -11008,12 +11150,12 @@ export class TaskExecutor { return; } } - if (genuinePauseAbort && await this.isRetryableBenignMergePauseAbort(live, result, abortProvenance, pausedAborted)) { + if (genuinePauseAbort && await this.isRetryableBenignMergePauseAbort(live, result, abortProvenance, pausedAborted, resumeLanesMemo)) { if (await this.routeGraphMergeFailureToRetry(live, result, abortProvenance)) { return; } } - if (genuinePauseAbort && await this.isBenignManualMergeHoldPauseAbort(live, result, abortProvenance, pausedAborted)) { + if (genuinePauseAbort && await this.isBenignManualMergeHoldPauseAbort(live, result, abortProvenance, pausedAborted, resumeLanesMemo)) { /* FNXC:WorkflowLifecycle 2026-07-09-14:56: FN-7749 / Runfusion#1979: auto-merge-off manual merge hold is terminal-until-human-merged, not an executor failure. Preserve the `in-review` row for Merge & Close, do not invoke merge retry, and clear only stale pause-abort status/error so FN-5147's no-backward-move/no-reenqueue contract stays intact. @@ -11030,7 +11172,7 @@ export class TaskExecutor { await this.persistTokenUsage(task.id); return; } - if (genuinePauseAbort && this.isBenignInReviewPauseAbort(live, result, abortProvenance, pausedAborted, this.userCanceledTaskIds.has(task.id))) { + if (genuinePauseAbort && this.isBenignInReviewPauseAbort(live, result, abortProvenance, pausedAborted, this.userCanceledTaskIds.has(task.id), failureLanes.review)) { this.clearPausedAborted(task.id); this.activeWorktrees.delete(task.id); const inReviewBenign = "Workflow graph run ended during engine pause/resume while already in-review — benign, in-review state preserved"; @@ -11039,10 +11181,10 @@ export class TaskExecutor { await this.persistTokenUsage(task.id); return; } - if (genuinePauseAbort && await this.handleStaleInReviewParsePauseAbortReplay(live, result, abortProvenance, pausedAborted, this.userCanceledTaskIds.has(task.id))) { + if (genuinePauseAbort && await this.handleStaleInReviewParsePauseAbortReplay(live, result, abortProvenance, pausedAborted, this.userCanceledTaskIds.has(task.id), resumeLanesMemo)) { return; } - if (genuinePauseAbort && await this.handleStaleInReviewPlanPauseAbortReplay(live, result, abortProvenance, pausedAborted, this.userCanceledTaskIds.has(task.id))) { + if (genuinePauseAbort && await this.handleStaleInReviewPlanPauseAbortReplay(live, result, abortProvenance, pausedAborted, this.userCanceledTaskIds.has(task.id), resumeLanesMemo)) { return; } if (genuinePauseAbort) { @@ -11078,7 +11220,7 @@ export class TaskExecutor { // the human-readable provenance label is ever revised. const isEngineInternalAbort = pausedAborted && !live.paused && !live.userPaused && abortProvenance !== "global-pause"; - if (live.column !== "in-progress") { + if (live.column !== failureLanes.wip) { // FN-6782: a pause/resume abort that has left the task back in `todo` // is benign — the work is simply re-queued for a fresh dispatch, not // stranded. Parking it `status: "failed"` (operator action required) @@ -11438,7 +11580,7 @@ export class TaskExecutor { if (await this.routeRetryableRemediationGraphFailureToPreMergeFix(live, failedNode, failureValue)) { return; } - if (await this.routeGraphFailureToExecutionResume(live, failedNode ?? "unknown", failureValue)) { + if (await this.routeGraphFailureToExecutionResume(live, failedNode ?? "unknown", failureValue, resumeLanesMemo)) { return; } /* @@ -11683,6 +11825,8 @@ export class TaskExecutor { live: TaskDetail, failedNode: string, failureValue: string | undefined, + /** Shared per-recovery lane snapshot — see `resolveResumeLanes`. */ + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, ): Promise { /* * FNXC:WorkflowLifecycle 2026-06-29-11:08: @@ -11712,7 +11856,19 @@ export class TaskExecutor { const implementationIncompleteMergeFailure = this.isMergeGraphFailure(failedNode) && failureValue === "implementation-incomplete"; if (implementationIncompleteMergeFailure && !incompleteSteps) return false; const prematureMergeWithIncompleteSteps = implementationIncompleteMergeFailure && incompleteSteps; - if (live.column !== "in-review" && !(incompleteSteps && live.column === "todo") && !(prematureMergeWithIncompleteSteps && live.column === "in-progress")) return false; + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-17:40 (fleet: executor.ts — the REVERSE half-conversion): + THE DESTINATION WAS ALREADY RESOLVED HERE AND THE GATE WAS NOT. `resolveReboundColumnFor` below picks + the board's rebound column (U7), but this gate compared against three default-lineage literals — so on + a renamed board the router refused before ever reaching the resolved move. That is the mirror image of + the dangerous half-conversion: instead of admitting a card and sending it nowhere, it refuses a card + whose recovery was fully implemented, and nothing is logged as wrong. Same one-decision-two-boards + defect, opposite direction, and the silent one. + */ + const resumeRouterLanes = await this.resolveResumeLanes(live.id, resumeLanesMemo); + if (live.column !== resumeRouterLanes.review + && !(incompleteSteps && live.column === resumeRouterLanes.hold) + && !(prematureMergeWithIncompleteSteps && live.column === resumeRouterLanes.wip)) return false; const message = incompleteSteps ? `Workflow graph failed at node '${failedNode}'${failureValue ? ` (${failureValue})` : ""} with incomplete steps — moved back to todo for execution resume` @@ -12272,7 +12428,15 @@ export class TaskExecutor { // executor can still recover by falling through to the fresh-worktree // path below, but we emit a loud audit record so these states stop being // silent. - if (task.column === "in-progress" && task.mergeDetails?.mergeConfirmed === true) { + /* + FNXC:WorkflowLifecycleColumns 2026-08-01-18:10 (fleet: execute() preflight): THREE DRIFT CHECKS, ONE + SNAPSHOT — merge-confirmed while still executing, stale mergeDetails, and in-wip with no worktree. None + fired on a renamed board, so every recovery they perform silently stopped happening. The third one's own + message says it "usually indicates a partial updateTask/moveTask sequence failed" — a diagnostic that + could never print on a renamed board. + */ + const preflightWipLane = (await this.resolveResumeLanes(task.id)).wip; + if (task.column === preflightWipLane && task.mergeDetails?.mergeConfirmed === true) { if (await this.finalizeMergeConfirmedWorkflowGraphTask(task.id, "execute-preflight")) { this.executing.delete(task.id); executingTaskLock.release(task.id); @@ -12281,7 +12445,7 @@ export class TaskExecutor { } } - if (task.column === "in-progress" && task.mergeDetails) { + if (task.column === preflightWipLane && task.mergeDetails) { executorLog.warn(`${task.id}: stale mergeDetails found while executing in-progress task — resetting merge state before continuing`); task = await this.cleanupMergeStateForReverification( task, @@ -12289,7 +12453,7 @@ export class TaskExecutor { ); } - if (task.column === "in-progress" && !task.worktree) { + if (task.column === preflightWipLane && !task.worktree) { executorLog.error( `${task.id}: drift detected — task is in-progress with no worktree. ` + `Recovering by creating a fresh worktree. This usually indicates a partial ` + @@ -13231,8 +13395,13 @@ export class TaskExecutor { // was unwinding; continuing the cleanup would clobber a valid // recovery (see the analogous block in the outer finally for the // full reasoning). + /* FNXC:WorkflowLifecycleColumns 2026-08-01-18:05 (fleet: stuck-requeue family): "has a + concurrent recovery already moved this card on?" — the pre-completion lanes are the board's + wip and hold. With literals a renamed board always answered "moved on", the cleanup never + ran, and the log line blamed a concurrent recovery that had not happened. */ const latestTask = await this.store.getTask(task.id); - if (latestTask.column !== "in-progress" && latestTask.column !== "todo") { + const requeueLanes = await this.resolveResumeLanes(task.id); + if (latestTask.column !== requeueLanes.wip && latestTask.column !== requeueLanes.hold) { executorLog.log( `${task.id} stuck-requeue skipped — task is now in '${latestTask.column}' (recovered concurrently)`, ); @@ -14038,7 +14207,9 @@ export class TaskExecutor { } const hasExplicitWorktreeBinding = typeof liveTask.worktree === "string" || liveTask.worktree === null; const hasExplicitBranchBinding = typeof liveTask.branch === "string" || liveTask.branch === null; - const worktreeContractIntact = liveTask.column === "in-progress" + /* FNXC:WorkflowLifecycleColumns 2026-08-01-18:15 (fleet): the contract holds while the card is + in ITS board's wip lane; the literal made every renamed-board retry look reclaimed. */ + const worktreeContractIntact = liveTask.column === (await this.resolveResumeLanes(task.id)).wip && !liveTask.paused && (!hasExplicitWorktreeBinding || liveTask.worktree === worktreePath) && (!hasExplicitBranchBinding || (typeof liveTask.branch === "string" && liveTask.branch.length > 0)); @@ -14505,7 +14676,10 @@ export class TaskExecutor { this.clearPausedAborted(task.id); const latestTask = await this.store.getTask(task.id); if ( - latestTask?.column === "todo" && + /* FNXC:WorkflowLifecycleColumns 2026-08-01-18:18 (fleet): the HOLD lane — this recognises a card the + abort already parked with its progress preserved, and skipping the cleanup is what keeps that + progress. On a renamed board the cleanup ran anyway and discarded it. */ + latestTask?.column === (await this.resolveResumeLanes(task.id)).hold && latestTask.paused === true && ((latestTask.currentStep ?? 0) > 0 || latestTask.steps?.some((step) => step.status === "done" || step.status === "in-progress")) ) { @@ -15192,7 +15366,8 @@ export class TaskExecutor { let cleanupLockHeld = true; try { const latestTask = await this.store.getTask(task.id); - if (latestTask.column === "in-progress" || latestTask.column === "todo") { + const continuationLanes = await this.resolveResumeLanes(task.id); + if (latestTask.column === continuationLanes.wip || latestTask.column === continuationLanes.hold) { await this.store.updateTask(task.id, { sessionFile: null, status: null, @@ -15244,7 +15419,8 @@ export class TaskExecutor { // all step progress reset, undoing valid completion. Skip the // entire cleanup if the column has moved on past in-progress/todo. const latestTask = await this.store.getTask(task.id); - if (latestTask.column !== "in-progress" && latestTask.column !== "todo") { + const outerRequeueLanes = await this.resolveResumeLanes(task.id); + if (latestTask.column !== outerRequeueLanes.wip && latestTask.column !== outerRequeueLanes.hold) { executorLog.log( `${task.id} stuck-requeue skipped — task is now in '${latestTask.column}' (recovered concurrently)`, ); @@ -20823,7 +20999,9 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB `${taskId} force-requeue could not read latest task state: ${err instanceof Error ? err.message : String(err)}`, ); } - if (latestColumn && latestColumn !== "in-progress") { + /* FNXC:WorkflowLifecycleColumns 2026-08-01-17:52 (fleet): the board's wip lane; with the literal a + renamed board skipped every force-requeue as "recovered concurrently". */ + if (latestColumn && latestColumn !== (await this.resolveResumeLanes(taskId)).wip) { executorLog.log( `${taskId} force-requeue skipped — task is now in '${latestColumn}' (recovered concurrently)`, ); diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 3b01762e67..59de493a12 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -2,7 +2,7 @@ "generatedFrom": "node scripts/lifecycle-column-census.mjs --strict --update-baseline", "byFile": { "packages/engine/src/self-healing.ts": 110, - "packages/engine/src/executor.ts": 57, + "packages/engine/src/executor.ts": 15, "packages/core/src/store.ts": 12, "packages/engine/src/scheduler.ts": 12, "packages/core/src/task-store/async-comments-attachments.ts": 9,