fix(executor): re-land the no-wip-lane fix — #2757 merged a snapshot that predated it (#2760)

## 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.
2026-07-30 07:44:35 -07:00
parent b0b9d1b373
commit c53d3aec38
2 changed files with 108 additions and 12 deletions
--- 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,
    });
  });
 });
--- 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<boolean> {
    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<boolean> {
    /*
    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<boolean> {
    /*
    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<boolean> {
    /*
    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<boolean> {
    /*
    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<boolean> {
    /*
    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<boolean> {
    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<boolean> {
    /*
     * 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;