From c53d3aec381d9b93a0426cd266cda34e4c8bded0 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 07:44:35 -0700 Subject: [PATCH] =?UTF-8?q?fix(executor):=20re-land=20the=20no-wip-lane=20?= =?UTF-8?q?fix=20=E2=80=94=20#2757=20merged=20a=20snapshot=20that=20predat?= =?UTF-8?q?ed=20it=20(#2760)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why this exists #2757 merged as `9a2a033b9e`, but **the third of its three fixes is not on main**: ``` $ git show origin/main:packages/engine/src/executor.ts | grep -c wipDeclared 0 ``` The merge captured my branch *before* commit `68381db72f`, so the no-wip-lane fix was dropped while the other two landed. `executor-execution-policy-renamed-columns` → *"a workflow with no wip column terminalizes visibly instead of claiming the card advanced"* is still red on main, still returning `status: null, error: null`. This is that commit, cherry-picked cleanly onto current main. No new work — the review discussion is in #2757. ## What it fixes (recap) `resolveResumeLanes` defaulted `wip: lifecycle?.wip ?? "in-progress"`, collapsing two different states: 1. **the IR failed to resolve** — defaulting is right; the `catch` arm wants exactly this 2. **the IR resolved and declares NO wip column** — defaulting *invents a lane the workflow does not have* `routeGraphFailureToExecutionResume` then admitted a card resting in that workflow's **hold** lane with incomplete steps, rehomed it, returned `true` — and the terminalize branch never ran. Resuming into a workflow with no implementation lane *is* "claiming the card advanced" when nothing did. The fix adds `wipDeclared` (declared, as opposed to defaulted) and declines the resume when it is false — the same fail-closed rule the sibling branch already applies with `wipColumn !== undefined` before calling a card "already advanced". That path failed closed; this one failed open. IR-unavailable deliberately keeps today's behaviour (`catch` reports `wipDeclared: true`), so an infrastructure error does not start refusing legitimate resumes. ## Verification on current main | check | result | |---|---| | the three affected files | **23 passed** | | `pnpm test:gate` | **726** | | engine `tsc --noEmit`, `pnpm lint` | clean | | remove the guard | 1 failed — the fail-closed case goes red again | | always decline | 1 failed — a legitimate resume breaks | Full-suite blast radius was measured on #2757 before it merged: 834 files / 10,845 tests, 5 failures, both files pre-existing (`executor-prompt`'s pause-guard 3 and `executor-abort-provenance`'s 2, byte-identical to baseline). Zero new failures. ## Note Worth checking whether other PRs merged in that window lost their final commits the same way — I only noticed because I re-verified main after the merge rather than assuming a merged PR contains what the branch held. --- .../executor-resume-lanes-resolved.test.ts | 60 +++++++++++++++++++ packages/engine/src/executor.ts | 60 +++++++++++++++---- 2 files changed, 108 insertions(+), 12 deletions(-) diff --git a/packages/engine/src/__tests__/executor-resume-lanes-resolved.test.ts b/packages/engine/src/__tests__/executor-resume-lanes-resolved.test.ts index 1b18473f38..bf28779f4c 100644 --- a/packages/engine/src/__tests__/executor-resume-lanes-resolved.test.ts +++ b/packages/engine/src/__tests__/executor-resume-lanes-resolved.test.ts @@ -101,10 +101,54 @@ describe("resume lanes come from the task's own workflow", () => { // renamed board answered "not a safe resume state" and the re-entry never fired. const h = harness(RENAMED_IR); + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-14:35: + `wipDeclared` reports whether the resolved IR actually DECLARES an implementation lane, which the + `?? "in-progress"` default destroys. The resume router needs it: a workflow with no wip column has + nowhere to resume TO, so claiming the card there swallowed a graph failure silently. + */ await expect(h.lanes("FN-1")).resolves.toEqual({ hold: "queued", wip: "building", review: "checking", + wipDeclared: true, + }); + }); + + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-15:30 (PR #2760 review — greptile P1): + The regression this nearly shipped. A v1 workflow upgraded in place synthesizes its columns with + `traits: []`, so `resolveLifecycleColumns` returns `{}` and NO lane is declared. A naive + `wipDeclared = lifecycle?.wip !== undefined` reads that as "this workflow has no implementation + lane" and the resume router declines — terminalizing every legacy custom workflow's graph-failure + recovery instead of resuming it. + + Measured, not assumed: parsing a v1 IR yields + `[{id:"triage",traits:[]},{id:"todo",traits:[]},{id:"in-progress",traits:[]},...]`. + + So `wipDeclared` is TRUE when the IR expresses no lifecycle intent at all, and false only when it + declares lanes and omits wip — the case in executor-execution-policy-renamed-columns. + */ + it("treats an untraited (v1-upgraded) board as HAVING an implementation lane", async () => { + const untraited = { + version: "v2", + id: "WF-legacy", + nodes: [], + edges: [], + columns: [ + { id: "triage", name: "triage", traits: [] }, + { id: "todo", name: "todo", traits: [] }, + { id: "in-progress", name: "in-progress", traits: [] }, + { id: "in-review", name: "in-review", traits: [] }, + ], + } as unknown as WorkflowIr; + const h = harness(untraited); + + await expect(h.lanes("FN-1")).resolves.toEqual({ + hold: "todo", + wip: "in-progress", + review: "in-review", + wipDeclared: true, }); }); @@ -113,10 +157,20 @@ describe("resume lanes come from the task's own workflow", () => { // and the default lineage behaves exactly as before. const h = harness(undefined); + /* + `wipDeclared` is TRUE here, and the distinction is worth stating: "no workflow resolves" does not + mean "no columns" — `resolveWorkflowIrForTask` falls back to the DEFAULT coding lineage, which + declares `in-progress`. So the legacy trio is a real declaration on that lineage, not an invented + lane, and a resume there is legitimate. + + The case `wipDeclared` exists to catch is different: an IR that resolves and declares NO wip column + at all (see the no-wip workflow in executor-execution-policy-renamed-columns). + */ await expect(h.lanes("FN-1")).resolves.toEqual({ hold: "todo", wip: "in-progress", review: "in-review", + wipDeclared: true, }); }); @@ -127,10 +181,16 @@ describe("resume lanes come from the task's own workflow", () => { throw new Error("workflow store unavailable"); }; + /* + IR UNAVAILABLE is deliberately different from "resolved, declares no wip": we cannot know, so this + keeps the legacy board's assumption and today's routing behaviour rather than failing closed on an + infrastructure error. + */ await expect(h.lanes("FN-1")).resolves.toEqual({ hold: "todo", wip: "in-progress", review: "in-review", + wipDeclared: true, }); }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index c919055682..8cb01fe142 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -10032,7 +10032,7 @@ export class TaskExecutor { live: TaskDetail, result: WorkflowGraphTaskRunResult, /** Shared per-recovery lane snapshot — see `resolveResumeLanes`. */ - resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, ): Promise { if (live.deletedAt) return false; if (live.paused || live.userPaused === true) return false; @@ -10247,7 +10247,7 @@ export class TaskExecutor { 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 } }, + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-06-19-00:05: @@ -10358,7 +10358,7 @@ export class TaskExecutor { 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 } }, + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-07-09-14:54: @@ -10394,7 +10394,7 @@ export class TaskExecutor { 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 } }, + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-06-28-21:05: @@ -10469,7 +10469,7 @@ export class TaskExecutor { 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 } }, + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-06-29-01:18: @@ -10583,7 +10583,7 @@ export class TaskExecutor { abortProvenance: PausedAbortProvenance | undefined, pausedAborted: boolean, userCanceled: boolean, - resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, ): Promise { /* FNXC:WorkflowLifecycle 2026-06-28-18:32: @@ -10660,8 +10660,8 @@ export class TaskExecutor { */ private async resolveResumeLanes( taskId: string, - memo?: { lanes?: { hold: string; wip: string; review: string } }, - ): Promise<{ hold: string; wip: string; review: string }> { + memo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, + ): Promise<{ hold: string; wip: string; review: string; wipDeclared: boolean }> { /* FNXC:WorkflowLifecycleColumns 2026-07-31-01:00 (PR #2640 review, greptile P2): ONE RESOLUTION PER RECOVERY, and the reason is correctness as much as I/O. Eligibility and @@ -10679,11 +10679,34 @@ export class TaskExecutor { hold: lifecycle?.hold ?? "todo", wip: lifecycle?.wip ?? "in-progress", review: lifecycle?.review ?? "in-review", + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-15:30 (PR #2760 review — greptile P1): + Whether the resolved IR actually DECLARES an implementation lane, which the `?? "in-progress"` + default above destroys. Callers that must not act without a real implementation lane read this + instead of comparing against the default. + + THREE states, not two, and conflating the last two is a regression: + a. wip declared -> true + b. lifecycle lanes declared, wip NOT -> FALSE; the workflow genuinely has no implementation + lane, so there is nowhere to resume TO + c. NO lifecycle lane declared at all -> true; this is a v1 workflow upgraded in place. Its + synthesized columns carry `traits: []`, so + `resolveLifecycleColumns` returns `{}` — measured, not + assumed — and treating that as "no wip lane" would + terminalize every legacy custom workflow's + graph-failure recovery instead of resuming it. + + The discriminator is whether the IR expresses lifecycle intent AT ALL. An untraited legacy board + expresses none, so the legacy trio is the honest answer and today's behaviour is preserved. + */ + wipDeclared: lifecycle?.wip !== undefined + || (lifecycle?.hold === undefined && lifecycle?.review === undefined), }; if (memo) memo.lanes = lanes; return lanes; } catch { - const lanes = { hold: "todo", wip: "in-progress", review: "in-review" }; + // IR unavailable: we cannot know, so keep the legacy board's assumption and today's behaviour. + const lanes = { hold: "todo", wip: "in-progress", review: "in-review", wipDeclared: true }; if (memo) memo.lanes = lanes; return lanes; } @@ -10693,7 +10716,7 @@ export class TaskExecutor { live: TaskDetail, result: WorkflowGraphTaskRunResult, abortProvenance: PausedAbortProvenance | undefined, - resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, ): Promise { const nodeId = result.interruptedNodeId ?? result.visitedNodeIds[result.visitedNodeIds.length - 1] ?? "unknown"; const priorRetries = live.graphResumeRetryCount ?? 0; @@ -10892,7 +10915,7 @@ export class TaskExecutor { 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 resumeLanesMemo: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } } = {}; const failureLanes = await this.resolveResumeLanes(live.id, resumeLanesMemo); /* FNXC:Lifecycle 2026-07-16-21:22: @@ -11826,7 +11849,7 @@ export class TaskExecutor { failedNode: string, failureValue: string | undefined, /** Shared per-recovery lane snapshot — see `resolveResumeLanes`. */ - resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string } }, + resumeLanesMemo?: { lanes?: { hold: string; wip: string; review: string; wipDeclared: boolean } }, ): Promise { /* * FNXC:WorkflowLifecycle 2026-06-29-11:08: @@ -11866,6 +11889,19 @@ export class TaskExecutor { defect, opposite direction, and the silent one. */ const resumeRouterLanes = await this.resolveResumeLanes(live.id, resumeLanesMemo); + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-14:20: + A workflow that declares NO implementation lane has nowhere to resume TO, so this router must not + claim the card — the graph failure has to reach the terminalize branch and be visible. + + Without this, a card resting in such a workflow's HOLD lane with incomplete steps matched the + second arm above (`incompleteSteps && live.column === lanes.hold`), the router rehomed it and + returned true, and the failure was swallowed: `status` and `error` both stayed null. The operator + saw a card that had silently stopped. That is the exact shape the sibling branch below already + guards with `wipColumn !== undefined` before claiming a card "already advanced"; this is the same + fail-closed rule on the opposite path, which was failing OPEN. + */ + if (!resumeRouterLanes.wipDeclared) return false; if (live.column !== resumeRouterLanes.review && !(incompleteSteps && live.column === resumeRouterLanes.hold) && !(prematureMergeWithIncompleteSteps && live.column === resumeRouterLanes.wip)) return false;