From 698bded47603e37b85e7dc5a75e3081ec4009f8a Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 09:40:49 -0700 Subject: [PATCH] =?UTF-8?q?fix(tests):=204=20engine=20reds=20on=20main=20?= =?UTF-8?q?=E2=80=94=20each=20was=20green=20for=20a=20reason=20the=20fleet?= =?UTF-8?q?=20removed=20(#2778)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Context A full `@fusion/engine` run on `origin/main` (`9b61d795c9`) reports **39 failures / 10835 passed**. 32 are the notifier harness, fixed in #2776. This PR takes 4 of the remaining 7. All three files share one shape: **each case was passing off something the lifecycle conversions have since correctly taken away.** In every one, the product is fine and a good change landed as a red test. --- ### 1. `executor-graph-failure-lanes-resolved.ts` — an equality that fails on its own fix The guard forbids resolving a lifecycle *guard* through the synchronous `resolvePlannerLanes` (a no-op under the shipped PostgreSQL backend, so the census counts the site as converted while it behaves like the literal). It asserted `expect(callSites).toBe(3)`. #2764 converted the promotion-path site to `resolvePlannerLanesForTaskAsync` — exactly the direction this guard wants. Count went **3 → 2** and the assertion failed. The guard's own comment states the invariant as *"Any FOURTH is a new sync resolution"* — one-directional. Coded as equality, it fails on removal, which is the change it exists to encourage. Now `toBeLessThanOrEqual(2)`. **Mutation:** adding a third sync call site → `expected 3 to be less than or equal to 2`. Still load-bearing. ### 2. `restart.integration.test.ts` — a fixture matching a fallback constant `recoverCompletedTask` re-homes intake → hold → wip only when the origin is the board's **intake** lane; otherwise it hands straight to review. The failure showed the 1st move as `in-review` with no re-home. Nothing regressed. The fixture put the card in `triage` and resolved lanes through the sync resolver, so it fell through to `LEGACY_PLANNER_LANES` — where `intake` is literally `"triage"`. **It was matching a hardcoded fallback, not a declared lane.** #2764 made the site await the real resolver; the mock selects `builtin:coding`, and **U11 merged intake and hold onto one Planning column (`todo`)**, so `triage` is not a lane on that board and the two-hop correctly collapses. The invariant the test is named for — completed work in a distinct intake lane is re-homed along a legal path, not moved intake → review, which role adjacency rejects — is still real. So the fixture now **declares** a board with intake separate from hold, the only shape where the two-hop is reachable. **Mutation:** removing the re-home hop from the product → fails with the expected `todo` first-move. Load-bearing. ### 3. `executor-abort-provenance.test.ts` — a call one argument short Both provenance cases returned `false` for a clean completed in-review row. This reads as an FN-6796 regression stranding rows that are already handed off for review. It is not. #2703 added a 7th `reviewLane` parameter so the lane is resolved by the caller. **The call goes through `as any`, so the missing argument was not a type error** — it arrived `undefined`, `live.column !== reviewLane` held for every row, and the classifier answered false for everything. Passed explicitly rather than defaulted inside the classifier: a default would restore the literal the parameter exists to remove. Added a **differential** — a card resting in a *renamed* review lane classifies the same, a mismatched one does not — so the parameter cannot be re-literalized while still looking converted. **Mutation:** `live.column !== "in-review"` → the differential fails. The other cases pass, which is precisely why it was worth adding. --- ## Evidence | file | result | |---|---| | `executor-graph-failure-lanes-resolved` | **24 passed** | | `restart.integration` | **48 passed** | | `executor-abort-provenance` | **16 passed** | Gate **732 green** · `pnpm lint` clean · engine `tsc --noEmit` **0 errors**. Test-only — no product file is touched by this PR (the mutations above were run and reverted; `git diff` confirms clean). ## Deliberately NOT fixed here 3 cases in `executor-prompt.test.ts` ("global pause behavior") remain red on main: **a user-paused todo task now reaches `createFnAgent`**. That is a safety invariant rather than a stale fixture, and neither `executeCore` nor the graph executor holds a pause gate — the refusal #2371 documented is not where its note implies. It gets its own change; editing the fixture to match current behaviour would hide it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) --- .../executor-abort-provenance.test.ts | 41 +++++++++++++++++++ ...cutor-graph-failure-lanes-resolved.test.ts | 17 ++++++-- .../src/__tests__/restart.integration.test.ts | 39 ++++++++++++++++++ 3 files changed, 93 insertions(+), 4 deletions(-) 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"),