diff --git a/packages/engine/src/__tests__/executor-abort-provenance.test.ts b/packages/engine/src/__tests__/executor-abort-provenance.test.ts index 27e14a2ecf..c06a15743f 100644 --- a/packages/engine/src/__tests__/executor-abort-provenance.test.ts +++ b/packages/engine/src/__tests__/executor-abort-provenance.test.ts @@ -256,10 +256,51 @@ describe("pause-abort provenance truthfulness (KB-PROV)", () => { provenance, true, false, + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-21:45: + THE REVIEW LANE, which this call was silently omitting. + + #2703 added a seventh parameter so the lane is resolved by the caller instead of through the + sync resolver (a no-op under the shipped backend). The call goes through `as any`, so the + missing argument was not a type error — it arrived `undefined`, `live.column !== reviewLane` + was true for every row, and the classifier returned false for BOTH provenances. That reads as + "FN-6796 regressed and clean in-review rows are being stranded again" when the product is + fine and the call is short one argument. + + Passed explicitly rather than defaulted inside the classifier: a default would restore the + literal this parameter exists to remove. + */ + "in-review", ); expect(benign).toBe(true); }, ); + + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-21:45: + The lane must be USED, not merely accepted. Without this, passing the argument could be reverted to + a hardcoded "in-review" inside the classifier and every assertion above would still pass — the + parameter would look converted while behaving like the literal, which is the exact failure the + census counts as a win. A card resting in a RENAMED review lane is the differential. + */ + it("honours the caller's review lane: a renamed lane classifies the same, a mismatched one does not", () => { + const { executor } = makeExecutor(); + const result = { disposition: "failed", outcome: "failure", visitedNodeIds: ["plan", "execute"], context: {} }; + const classify = (column: string, reviewLane: string) => + (executor as any).isBenignInReviewPauseAbort( + makeTask({ column, steps: [{ name: "Implement", status: "done" }] }), + result, + "engine-abort", + true, + false, + reviewLane, + ); + + // Resting in the board's declared review lane, whatever it is called. + expect(classify("validating", "validating")).toBe(true); + // Resting somewhere else: not a benign in-review pause-abort. + expect(classify("validating", "in-review")).toBe(false); + }); }); }); 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 index 692d39faca..2c05c76221 100644 --- a/packages/engine/src/__tests__/executor-graph-failure-lanes-resolved.test.ts +++ b/packages/engine/src/__tests__/executor-graph-failure-lanes-resolved.test.ts @@ -393,11 +393,20 @@ describe("no lifecycle GUARD resolves its lane synchronously", () => { 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. + A CEILING, not an equality. + + The invariant this guard exists for is one-directional: no NEW synchronous resolution may appear. + Removing one is always safe — it is the fix this test is trying to encourage — so an exact count + fails on exactly the change it wants. That is what happened: #2764 converted the promotion-path + site to `resolvePlannerLanesForTaskAsync`, the count went 3 -> 2, and a correct improvement + landed as a red test on main with nothing wrong in the product. + + What remains are the two planner-column move destinations documented above (`PlannerLanes` exists + so a caller refuses rather than inventing a column), which is a different question from "which + lane is this card in". A THIRD 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); + expect(callSites).toBeLessThanOrEqual(2); }); it("does not compare a column against a synchronously-resolved lane on the same line", async () => { diff --git a/packages/engine/src/__tests__/restart.integration.test.ts b/packages/engine/src/__tests__/restart.integration.test.ts index b06be2373b..2e6a79572e 100644 --- a/packages/engine/src/__tests__/restart.integration.test.ts +++ b/packages/engine/src/__tests__/restart.integration.test.ts @@ -1101,8 +1101,47 @@ describe("In-progress task resume after restart", () => { })); }); + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-21:30: + THIS FIXTURE NOW DECLARES THE BOARD IT DEPENDS ON, instead of inheriting a lane by accident. + + It went red on main after #2764 with the FIRST move landing as `in-review` and no re-home at all. + Nothing regressed in the product — the test had been passing for the wrong reason. The card sits + in `triage`, and recovery only re-homes when the origin is the board's INTAKE lane. It used to + resolve lanes through the SYNC `resolvePlannerLanes`, whose selection reader is a no-op under the + shipped backend, so it fell through to LEGACY_PLANNER_LANES — where `intake` is literally + "triage". The fixture was therefore matching a hardcoded fallback constant, not a declared lane. + + #2764 correctly made the site await the real resolver. The mock selects `builtin:coding`, and + U11 MERGED intake and hold onto one Planning column (`todo`), so post-U11 `triage` is not a lane + on that board at all and the two-hop correctly collapses. + + The invariant this test is NAMED for is still real and still worth pinning: on a board with a + DISTINCT intake lane, completed work stranded there is re-homed along a legal path rather than + moved intake -> review, which role adjacency rejects. So the workflow is declared explicitly here + with intake separate from hold. That is the only shape under which the two-hop is reachable, and + saying so in the fixture means the next vocabulary change fails loudly instead of silently + selecting a different code path. + */ it("recoverCompletedTask() legally re-homes a completed triage zombie before review handoff", async () => { + /* Distinct intake ("triage") and hold ("todo") — pre-U11 / custom-lineage shape. */ + const distinctIntakeIr = { + version: "v2", + id: "custom:distinct-intake", + nodes: [], + edges: [], + columns: [ + { id: "triage", label: "Triage", traits: [{ trait: "intake" }] }, + { id: "todo", label: "Planning", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "in-progress", label: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "in-review", label: "Review", traits: [{ trait: "humanReview" }, { trait: "mergeBlocker" }] }, + { id: "done", label: "Done", traits: [{ trait: "complete" }] }, + ], + }; const store = createMockStore(); + store.getTaskWorkflowSelectionAsync = vi.fn().mockResolvedValue({ workflowId: "custom:distinct-intake", stepIds: [] }); + store.getTaskWorkflowSelection = vi.fn().mockReturnValue({ workflowId: "custom:distinct-intake", stepIds: [] }); + store.getWorkflowDefinition = vi.fn().mockResolvedValue({ ir: distinctIntakeIr }); const task = makeTask("FN-TRIAGE-ZOMBIE", "triage", { worktree: "/tmp/wt/FN-TRIAGE-ZOMBIE", steps: makeSteps("done"),