From 7e4b3ae580aa4156366681efe170f74968994279 Mon Sep 17 00:00:00 2001 From: Timoteo Date: Sat, 26 Sep 2026 05:44:21 -0300 Subject: [PATCH 1/6] test(engine): repair assertion-no-error-line family against shipped contracts FUSI-034 Step 1 (R1): the shared merge-lane fake was missing two ProjectEngine fields, so a test-double defect surfaced as an apparent production crash. R1 is fixed in the FAKE, not in the assertion and not in the product. project-engine.ts:586-587 (mergeRetryResetTaskIds, mergeEnqueueDeferredByRetryReset) are correct shipped behaviour; the fixture simply predated FN-9317 (706c15560). internalEnqueueMerge threw TypeError: Cannot read properties of undefined (reading 'has') at project-engine.ts:3123 because Object.create(ProjectEngine.prototype) runs no class-field initializers. merge-abort-clears-transient-status.test.ts: 8 failed / 21 passed (29) + 5 unhandled errors -> 29 passed (29), 0 errors, stable across 3 runs. The 5 unhandled errors were the same defect on a second path, so their disappearance is independent corroboration. FN-5893 invariant enumeration (not just the reported site): walked all 62 instance fields of class ProjectEngine against MergeLaneState. The drift ratchet at project-engine-merge-lane-fixture-drift.test.ts independently caught the two FN-9317 fields (it was RED before this commit and is green after), which confirms the ratchet works and the fixture had drifted, not that the ratchet is broken. Full diff of 39 unseeded instance fields is reported in the completion summary. Two further real gaps found and closed here: - mergeBodySettleTimeoutMs (project-engine.ts:1015) is read by awaitPriorMergeBodySettle, but is declared OUTSIDE the auto-merge state block the ratchet scans, so the ratchet is blind to it. Unseeded, it collapses the 60s drain latch to ~1ms via setTimeout(fn, undefined). Its three consumers each override it to 1 by hand, which is the smell of a gap patched around rather than closed. Seeded with the production default; their overrides are left untouched. - 36 remaining unseeded fields are non-merge-lane collaborators with no prototype-fake consumer (runtime, notifier, oauth*, planner*, automation*, settingsHandlers, legacyAutoMergeStampAdvisoryEmitted). Recorded as findings, not added: a field no prototype drain path reads needs no fake, and seeding 36 collaborator-shaped fields would make the fixture a second, hand-drifting copy of the whole engine. The three already-allowlisted lane fields (unregisterMergeAdmissionProvider, autostashSweepTimer, mergeActiveReconcileTimer) remain correctly allowlisted and are NOT seeded. No timeout widened, no retry added, no assertion loosened or deleted, no .skip or .only. No changeset: the only file touched is a test helper in the private @fusion/engine package, which AGENTS.md exempts. Co-authored-by: Fusion Fusion-Task-Id: FUSI-034 --- .../_project-engine-merge-lane-fixture.ts | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/packages/engine/src/__tests__/_project-engine-merge-lane-fixture.ts b/packages/engine/src/__tests__/_project-engine-merge-lane-fixture.ts index bda5c584a4..0a6d3dea67 100644 --- a/packages/engine/src/__tests__/_project-engine-merge-lane-fixture.ts +++ b/packages/engine/src/__tests__/_project-engine-merge-lane-fixture.ts @@ -1,6 +1,8 @@ type MergeLaneState = { mergeQueue: string[]; mergeActive: Set; + mergeRetryResetTaskIds: Set; + mergeEnqueueDeferredByRetryReset: Set; capacityDeferredMergeTaskIds: Set; capacityDeferredMergeReasons: Map; capacityDeferredMerges: Map; @@ -19,6 +21,7 @@ type MergeLaneState = { workspaceBusyReenqueues: Map; workspaceBusyReenqueueTimers: Set>; manualMergeResolvers: Map; + mergeBodySettleTimeoutMs: number; shuttingDown: boolean; startupGeneration: number; started: boolean; @@ -29,6 +32,21 @@ type MergeLaneState = { * Object.create(ProjectEngine.prototype) runs no class field initializers, so a prototype-only * merge fake starts with every merge-lane field undefined. FN-8871 requires this fixture to include * capacity and PR-retry merge state, preventing production drain additions from drifting test fakes. + * + * FNXC:MergeQueue 2026-09-26-05:45: + * The fixture fell behind production for the SECOND time (the first was FUSI-030's missing `getTask` + * collaborator). FN-9317 (`706c15560` "recover stalled in-review merges") added `mergeRetryResetTaskIds` + * and `mergeEnqueueDeferredByRetryReset` to ProjectEngine; this fixture never learned them, so + * `internalEnqueueMerge` threw `TypeError: Cannot read properties of undefined (reading 'has')` at + * project-engine.ts:3123 and 8 cases in merge-abort-clears-transient-status.test.ts went red with an + * error that read like a production crash. The tell is that a TypeError from deep inside + * project-engine.ts can only be a fake defect: a real engine always runs its field initializers. + * `mergeBodySettleTimeoutMs` is seeded here too. It is declared at project-engine.ts:1015, OUTSIDE the + * `// ── Auto-merge state ──` block that project-engine-merge-lane-fixture-drift.test.ts scans, so the + * ratchet cannot see it — but `awaitPriorMergeBodySettle` reads it, and on an unseeded prototype fake + * `setTimeout(fn, undefined)` collapses the 60s drain latch to ~1ms. The tests that reach it each + * override it to 1 by hand, which is exactly the smell: every consumer patches around the gap instead + * of the gap being closed. Seed it with its production default and leave those overrides alone. */ export function seedMergeLaneState( engine: T, @@ -37,6 +55,11 @@ export function seedMergeLaneState( const defaults: MergeLaneState = { mergeQueue: [], mergeActive: new Set(), + /* FNXC:MergeRetryAdmission 2026-09-26-05:45: the process-local fence between Chat's + status-none reset and queue admission (FN-9317). Empty is the production-equivalent default — + no reset is committing on a fresh engine. */ + mergeRetryResetTaskIds: new Set(), + mergeEnqueueDeferredByRetryReset: new Set(), capacityDeferredMergeTaskIds: new Set(), capacityDeferredMergeReasons: new Map(), capacityDeferredMerges: new Map(), @@ -57,6 +80,9 @@ export function seedMergeLaneState( workspaceBusyReenqueues: new Map(), workspaceBusyReenqueueTimers: new Set(), manualMergeResolvers: new Map(), + /* FNXC:MergeQueue 2026-09-26-05:45: the orphan-body drain latch, 60s in production + (project-engine.ts:1015). Undefined on a prototype fake collapses it to ~1ms. */ + mergeBodySettleTimeoutMs: 60_000, shuttingDown: false, startupGeneration: 0, started: true, From d85a1f79099ec846d88eeed5ecf84536301a274a Mon Sep 17 00:00:00 2001 From: Timoteo Date: Sat, 26 Sep 2026 05:50:20 -0300 Subject: [PATCH 2/6] test(engine): repair assertion-no-error-line family against shipped contracts FUSI-034 Step 2 (R2): fixed in the FAKES, not the assertions and not the product. post-landing-worktree-cleanup.test.ts: 11 failed -> 29 passed (29). OPTION CHOSEN: (a) declare the evidence satisfied. The spec prefers (a) because it keeps FN-9370's guard on the exercised path and proves finalization still completes once evidence exists. Option (b) (no gate enabled) would have exercised only the guard's early-out and left the new guard untested in this file. The gate id is IMPORTED from @fusion/core as POST_MERGE_VERIFICATION_GROUP_ID, not pasted from a captured log, so a future rename of the built-in post-merge group cannot silently strand this fake on a literal. The fixture also sets an explicit enabledWorkflowSteps list rather than relying on defaultOn: true, so the fake's position does not move if that default is ever edited. A SECOND, DISTINCT FAKE GAP surfaced while fixing R2 and is fixed here too. The finalizer no longer calls moveTask: FN-9371 (HEAD 6c57344b8) moved terminal finalization onto a conditional fence, so the fake was missing store.moveTaskIf and store.updateTaskAtomic and every completion path threw 'store.moveTaskIf is not a function' at auto-merge-finalization.ts:406 before any assertion ran. Both are modelled on the real contracts: - moveTaskIf (packages/core/src/task-store/moves.ts:284): awaits the predicate and returns { task, moved: false } WITHOUT moving on refusal. The predicate is honoured, not stubbed true, so the refusal cases genuinely exercise the fence. - updateTaskAtomic (packages/core/src/task-store/task-mutation-ops.ts:380): runs the updater against the live row; a null/undefined patch is a no-op. The pre-existing moveTask mock also gained its third MoveTaskOptions parameter, which eight existing assertions already expected. NEW COVERAGE (5 cases, 24 -> 29, all additive; nothing removed): - 3 assert the guard's REFUSAL path (outcome 'blocked' plus the exact reason string) for: no report at all, a REVISE verdict, and a failed gate status. Each also asserts the move did not happen, so a refusal cannot half-apply. - 'finalizes without post-merge evidence when the gate was explicitly disabled' covers the isWorkflowOptionalGroupEnabled boundary (this is NOT option (b) - the default fixture keeps the gate enabled and approved). - 'accepts APPROVE_WITH_NOTES as satisfying the post-merge gate' pins the second approving verdict the guard allows. Without the refusal cases, a future change that disabled or short-circuited the guard would leave this file fully green while merged work reached done without the required post-landing Full Suite evidence. FN-5893 SWEEP (all already-green, NONE edited): every engine test referencing finalizeProvenAutoMergeTask was enumerated, not just the named candidates. Swept and confirmed green: confirmed-merge-must-finalize (exposes the guard trigger), merge-proof-reason-renamed-complete-lane (exposes), merger-trait-rekey (exposes), no-merge-workflow-completion (exposes), self-healing (exposes), self-healing-landed-worktree-cleanup (exposes), merger-merge-lifecycle, merge-orphan-body-durable-writes. The 3 PG e2e files do not expose the trigger. Because all conditional File Scope candidates are green, no conditional entry was edited. One flake observed once in merge-orphan-body-durable-writes ('aborts during a real second workspace-repository land and partitions successor writes') and it did NOT reproduce in 3 subsequent identical invocations; that file is outside this family, so it is reported rather than quarantined here. No timeout widened, no retry added, no assertion loosened or deleted, no .skip or .only. No changeset: test file in the private @fusion/engine package, AGENTS.md-exempt. Co-authored-by: Fusion Fusion-Task-Id: FUSI-034 --- .../post-landing-worktree-cleanup.test.ts | 187 +++++++++++++++++- 1 file changed, 185 insertions(+), 2 deletions(-) diff --git a/packages/engine/src/__tests__/post-landing-worktree-cleanup.test.ts b/packages/engine/src/__tests__/post-landing-worktree-cleanup.test.ts index ab6b728ef5..25f510926a 100644 --- a/packages/engine/src/__tests__/post-landing-worktree-cleanup.test.ts +++ b/packages/engine/src/__tests__/post-landing-worktree-cleanup.test.ts @@ -27,9 +27,37 @@ vi.mock("../worktree/worktree-backend.js", () => ({ removeWorktree: removeWorktreeMock, })); +import { POST_MERGE_VERIFICATION_GROUP_ID } from "@fusion/core"; import { finalizeProvenAutoMergeTask } from "../merge/auto-merge-finalization.js"; import { cleanupLandedTaskWorktree, cleanupLandedWorkspaceTaskWorktrees } from "../merge/post-landing-worktree-cleanup.js"; +/** + * FNXC:PostMergeEvidenceFake 2026-09-26-06:05: + * FN-9370 made a confirmed merge NOT sufficient to finalize. `getRequiredPostMergeEvidenceBlocker` + * (packages/core/src/merge/confirmed-merge-reconciliation.ts:40) refuses completion while an enabled + * gate-mode post-merge group has no APPROVE result. This fake's store exposes `getTaskWorkflowSelection`, + * which is the guard's own trigger, and the built-in group is `defaultOn: true` — so a task with no + * post-merge evidence is legitimately blocked. That is correct shipped behaviour, so the FIXTURE declares + * its evidence position rather than the assertion being relaxed. + * + * Option (a) from the spec: declare the evidence satisfied, which keeps the guard on the exercised path + * and proves finalization still completes once evidence exists. Option (b) (declare no gate enabled) would + * have exercised only the guard's early-out and left FN-9370 untested in this file. + * + * The group id is imported from @fusion/core rather than pasted, so a future rename of the built-in + * post-merge group cannot silently strand this fake on a literal. + */ +const APPROVED_POST_MERGE_EVIDENCE = [ + { + workflowStepId: POST_MERGE_VERIFICATION_GROUP_ID, + workflowStepName: "Post-merge verification", + phase: "post-merge" as const, + source: "optional-group" as const, + status: "passed" as const, + verdict: "APPROVE" as const, + }, +]; + function createFinalizationStore(options: { column?: string; worktree?: string | null } = {}) { const task: any = { id: "FN-251", @@ -41,7 +69,11 @@ function createFinalizationStore(options: { column?: string; worktree?: string | mergeRetries: 0, worktree: options.worktree === undefined ? "/repo/.worktrees/fn-251" : options.worktree, steps: [], - workflowStepResults: [], + /* FNXC:PostMergeEvidenceFake 2026-09-26-06:05: an explicit enable list keeps the fixture's + position independent of the built-in `defaultOn` flag; `undefined` would also enable the gate + today, but only by falling back to a default that a future edit could change. */ + enabledWorkflowSteps: [POST_MERGE_VERIFICATION_GROUP_ID], + workflowStepResults: [...APPROVED_POST_MERGE_EVIDENCE], mergeDetails: { mergeConfirmed: true, commitSha: "abc123" }, }; const callOrder: string[] = []; @@ -50,17 +82,57 @@ function createFinalizationStore(options: { column?: string; worktree?: string | Object.assign(task, patch); return task; }); - const moveTask = vi.fn(async (_id: string, column: string) => { + /* FNXC:PostMergeEvidenceFence 2026-09-26-06:05: accepts the third MoveTaskOptions argument the + finalizer's conditional move passes; the existing assertions below already expect it. */ + const moveTask = vi.fn(async (_id: string, column: string, _options?: unknown) => { callOrder.push("move"); task.column = column; return task; }); + /* + FNXC:PostMergeEvidenceFence 2026-09-26-06:05: + FN-9370 moved terminal finalization onto a conditional fence: `moveTaskIf` re-reads the live row, + re-evaluates the post-merge evidence guard under that read, and moves only if the predicate passes. + This fake had only `moveTask`, so the finalizer threw `store.moveTaskIf is not a function` and every + completion path in this file failed before asserting anything. Modelled on the real contract in + packages/core/src/task-store/moves.ts:284 (`{ task, moved }` is the applied/skip signal; a refused + predicate returns `moved: false` WITHOUT moving and WITHOUT recording a call). The predicate is + awaited and honoured rather than stubbed true, so the refusal cases below genuinely exercise the fence. + */ + const moveTaskIf = vi.fn(async ( + _id: string, + toColumn: string, + predicate: (live: unknown) => boolean | Promise, + options?: unknown, + ) => { + if (!await predicate(task) || task.column === toColumn) { + return { task, moved: false }; + } + return { task: await moveTask(_id, toColumn, options), moved: true }; + }); + /* + FNXC:PostMergeEvidenceFence 2026-09-26-06:05: + The post-move reconciliation is applied through `updateTaskAtomic` (read -> updater -> apply). The + updater is run for real against the live row; a null/undefined patch is a no-op, matching + updateTaskAtomicImpl in packages/core/src/task-store/task-mutation-ops.ts:380. + */ + const updateTaskAtomic = vi.fn(async ( + _id: string, + updater: (current: unknown) => Record | null | undefined | Promise | null | undefined>, + ) => { + const updates = await updater(task); + if (!updates || Object.values(updates).every((value) => value === undefined)) return task; + Object.assign(task, updates); + return task; + }); const logEntry = vi.fn().mockResolvedValue(task); return { task, callOrder, updateTask, moveTask, + moveTaskIf, + updateTaskAtomic, logEntry, store: { getTask: vi.fn(async () => task), @@ -70,6 +142,8 @@ function createFinalizationStore(options: { column?: string; worktree?: string | getCompletionHandoffAcceptedMarker: vi.fn(async () => null), updateTask, moveTask, + moveTaskIf, + updateTaskAtomic, logEntry, recordRunAuditEvent: vi.fn(), }, @@ -385,6 +459,115 @@ describe("cleanupLandedTaskWorktree", () => { expect(moveTask).not.toHaveBeenCalled(); }); + /* + FNXC:PostMergeEvidenceGuard 2026-09-26-06:05: + FN-9370's guard is the reason every case above now carries approved post-merge evidence. Without an + assertion of its REFUSAL path, a future change that disabled or short-circuited the guard would leave + this file fully green while merged work reached `done` without the required post-landing Full Suite + evidence. This case is the guard's own coverage, and it is the one place the fake is deliberately + left in the shape the spec called incomplete. + */ + it("refuses to finalize while an enabled post-merge gate has not approved", async () => { + const { store, task, moveTask, updateTask } = createFinalizationStore(); + task.workflowStepResults = []; + + const result = await finalizeProvenAutoMergeTask({ + store: store as never, + taskId: task.id, + rootDir: "/repo", + source: "workflow-graph-merge-finalize", + }); + + expect(result.outcome).toBe("blocked"); + expect(result.reason).toBe( + `required post-merge evidence gate '${POST_MERGE_VERIFICATION_GROUP_ID}' has not reported`, + ); + // A refused finalization must not half-apply: no completion move, no worktree pointer clear. + expect(moveTask).not.toHaveBeenCalled(); + expect(updateTask).not.toHaveBeenCalledWith(task.id, { worktree: null }); + expect(task.column).toBe("in-review"); + }); + + it("refuses to finalize when the post-merge gate reported a non-approving verdict", async () => { + const { store, task, moveTask } = createFinalizationStore(); + task.workflowStepResults = [ + { ...APPROVED_POST_MERGE_EVIDENCE[0], verdict: "REVISE" }, + ]; + + const result = await finalizeProvenAutoMergeTask({ + store: store as never, + taskId: task.id, + rootDir: "/repo", + source: "workflow-graph-merge-finalize", + }); + + expect(result.outcome).toBe("blocked"); + expect(result.reason).toBe( + `required post-merge evidence gate '${POST_MERGE_VERIFICATION_GROUP_ID}' is not approved`, + ); + expect(moveTask).not.toHaveBeenCalled(); + }); + + it("refuses to finalize when the post-merge gate itself failed", async () => { + const { store, task, moveTask } = createFinalizationStore(); + task.workflowStepResults = [ + { ...APPROVED_POST_MERGE_EVIDENCE[0], status: "failed" }, + ]; + + const result = await finalizeProvenAutoMergeTask({ + store: store as never, + taskId: task.id, + rootDir: "/repo", + source: "workflow-graph-merge-finalize", + }); + + expect(result.outcome).toBe("blocked"); + expect(result.reason).toBe( + `required post-merge evidence gate '${POST_MERGE_VERIFICATION_GROUP_ID}' is not approved`, + ); + expect(moveTask).not.toHaveBeenCalled(); + }); + + /* + FNXC:PostMergeEvidenceGuard 2026-09-26-06:05: + An explicitly DISABLED gate is not required evidence. This is the boundary case of the guard's + `isWorkflowOptionalGroupEnabled` check and is distinct from option (b) above: the default fixture + keeps the gate ENABLED and approved, while this case proves that a task which never opted in is not + blocked by a gate it does not owe evidence for. + */ + it("finalizes without post-merge evidence when the gate was explicitly disabled", async () => { + const { store, task, moveTask } = createFinalizationStore(); + task.enabledWorkflowSteps = []; + task.workflowStepResults = []; + + const result = await finalizeProvenAutoMergeTask({ + store: store as never, + taskId: task.id, + rootDir: "/repo", + source: "workflow-graph-merge-finalize", + }); + + expect(result.outcome).toBe("done"); + expect(moveTask).toHaveBeenCalledWith(task.id, "done", expect.any(Object)); + }); + + it("accepts APPROVE_WITH_NOTES as satisfying the post-merge gate", async () => { + const { store, task, moveTask } = createFinalizationStore(); + task.workflowStepResults = [ + { ...APPROVED_POST_MERGE_EVIDENCE[0], verdict: "APPROVE_WITH_NOTES" }, + ]; + + const result = await finalizeProvenAutoMergeTask({ + store: store as never, + taskId: task.id, + rootDir: "/repo", + source: "workflow-graph-merge-finalize", + }); + + expect(result.outcome).toBe("done"); + expect(moveTask).toHaveBeenCalledWith(task.id, "done", expect.any(Object)); + }); + it("uses empty settings when a minimal store has no settings reader", async () => { const { store, getSettings } = createStore({ withSettings: false }); From 088595eba04821f455c91cb609540d188eabad69 Mon Sep 17 00:00:00 2001 From: Timoteo Date: Sat, 26 Sep 2026 05:51:15 -0300 Subject: [PATCH 3/6] test(engine): realign merge-region collapse path with the post-merge gate child FUSI-034 Step 3 (R3): realigned the ASSERTMENT to the shipped contract, after reading the collapse implementation and confirming the classification is correct. workflow-graph-merge-region-collapse.test.ts: 8 failed -> 10 passed (10). The spec required confirming the collapse logic BEFORE extending the expectation, because adding a node id to an expectation without confirming the classification would defeat the test. Confirmed on two independent grounds: - MERGE_REGION_KINDS (workflow-graph-executor.ts:490) contains only the seven raw merge primitives. The "optional-group" kind is NOT a member, so "post-merge-verification" is a post-merge ENTRY node reached after the collapsed "merge" seam, not an independent merge-region boundary. - its inner "prompt" child is likewise not a region kind, so it is traversed as a child of that entry and can never be classified as a region member. Because the classification is right, the fix belongs in the stale literal and this step did NOT exceed a test-file change. No product source was touched. The id is DERIVED from the IR source, not pasted from a captured failing log: postMergeOptionalGroupNode names the inner node with a template expression evaluated to ${spec.id}-step (builtin-post-merge-group.ts:125) and the executor records an optional-group child as :: (workflow-graph-executor.ts:147), giving "post-merge-verification::post-merge-verification-step". That is the same shape as the "plan-review::plan-review-step" and "code-review::code-review-step" entries already in the expected list, so a reviewer can see where the value came from. The test's actual subject is preserved: expectNoRawMergeRegionVisits still proves none of the seven raw merge primitives is ever visited, and the collapse-to-one-merge assertion (calls === ['merge'], one merge dispatch) is untouched. This is a stale literal in a test, not a collapse-logic bug. No assertion deleted or loosened. Co-authored-by: Fusion Fusion-Task-Id: FUSI-034 --- ...rkflow-graph-merge-region-collapse.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/engine/src/__tests__/workflow-graph-merge-region-collapse.test.ts b/packages/engine/src/__tests__/workflow-graph-merge-region-collapse.test.ts index 47240cec40..3cc161286b 100644 --- a/packages/engine/src/__tests__/workflow-graph-merge-region-collapse.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-merge-region-collapse.test.ts @@ -88,7 +88,26 @@ const SUCCESS_PATH = [ "code-review::code-review-step", "review", "merge", + /* + FNXC:WorkflowGraphTests 2026-09-26-06:20: + FN-9369/FN-9370 turned post-merge verification into a required delivery-evidence gate, which + appended the gate's inner prompt child to the visited path. The id is DERIVED, not copied from a + failing log: `postMergeOptionalGroupNode` (packages/core/src/workflows/builtin-post-merge-group.ts:125) + names the inner node `${spec.id}-step`, and the executor records an optional-group child as + `::` (workflow-graph-executor.ts:147) — the same shape as the + `plan-review::plan-review-step` and `code-review::code-review-step` entries already listed above. + + The collapse classification was confirmed CORRECT before this expectation was extended, because + extending it is only safe if the node is a child step and not a new merge-region boundary: + - `optional-group` is NOT in MERGE_REGION_KINDS (workflow-graph-executor.ts:490), so + `post-merge-verification` is a post-merge ENTRY node reached after the collapsed merge seam. + - its inner `prompt` child is likewise not a merge-region kind, so it is traversed as a child of + that entry and can never be mistaken for a region member. + This keeps the test's real subject intact: `expectNoRawMergeRegionVisits` still proves none of the + seven raw merge primitives is ever visited. + */ "post-merge-verification", + "post-merge-verification::post-merge-verification-step", ]; // Same path, stopped before the post-merge hop (merge itself failed). const MERGE_FAILURE_PATH = SUCCESS_PATH.slice(0, SUCCESS_PATH.indexOf("merge") + 1); From 67b3bb76230e9ef088f64c4fd3009f1bec24aeb8 Mon Sep 17 00:00:00 2001 From: Timoteo Date: Sat, 26 Sep 2026 05:55:14 -0300 Subject: [PATCH 4/6] test(engine): key review-gate assertions on the optional group id FUSI-034 Step 4 (R4): classified all 4 red cases, then realigned them. No product change was required, so nothing outside File Scope was touched. workflow-step-notes-repair.test.ts: 4 failed / 10 passed -> 14 passed (14). CLASSIFICATION: the 4 cases have ONE root cause, not the two the spec hypothesised. A review gate's identity is its optional GROUP id, not the inner template step id. The executor resolves effectiveWorkflowStepId = optionalGroupId ?? workflowStep.id.replace(/^graph:/,"") at execute-workflow-step.ts:262, and reuses that single value (sameGateStepId, :325) for BOTH the run-audit workflowStepId (:1275) AND the prior-record lookup findReusableReviewResult (:620, matching result.workflowStepId at :134). Because optionalGroupId is present, the GROUP id wins. The fixtures and assertions here were still keyed to the inner step id ("code-review-step" / "plan-review-step"), which is the pre-group shape. - 3 cases ("keeps repaired review outcomes unchanged with an throwing / rejecting / hanging run-audit sink"): STALE MATCHER. The call already carried a complete event object; only metadata.workflowStepId was wrong. The matcher is TIGHTENED, not weakened: it now pins agentId, runId, domain, mutationType, target and taskId in addition to every metadata field. The previous bare top-level objectContaining asserted nothing about the agent/domain identity, so it would have matched almost any call. - 1 case ("narrates an unchanged legacy review whose persisted output and notes are empty"): STALE FIXTURE ROW. The persisted prior record was keyed to the inner step id, so findReusableReviewResult never matched it, a SECOND review dispatch ran, and the reused-empty notice the test exists to cover was never reached. Keying the row to the gate id makes the record match; because the fixture genuinely persists notes:"" and output:"", storedReusedNotes is "" and the notice IS produced (:640-641). The test's own premise is now reachable through the product's real reuse path rather than around it. FAIL-SOFT SIGNAL PRESERVED: all 4 cases still assert the outcome is unchanged (success/verdict/notes/output) BEFORE the audit assertion, which is the actual subject of this file. No assertion was deleted. The fail-soft surface is unchanged. NOT APPEASEMENT, PROVEN BY MUTATION: I verified the tightened matcher can still fail. Changing the asserted agentId, and separately reverting workflowStepId to the old inner step id, each turn 3 cases red (11 passed). The file was restored byte-identical after each probe and re-verified green. Gate ids are named values, not repeated literals: PLAN_REVIEW_GROUP_ID is imported from @fusion/core (it is re-exported). CODE_REVIEW_GROUP_ID is mirrored as a documented local constant because @fusion/core does NOT re-export it from its barrel, and the fixture's own optionalGroupId is the field the product actually reads. No timeout widened, no retry added, no .skip or .only. No changeset: test file in the private @fusion/engine package, AGENTS.md-exempt. Co-authored-by: Fusion Fusion-Task-Id: FUSI-034 --- .../workflow-step-notes-repair.test.ts | 50 +++++++++++++++++-- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/packages/engine/src/__tests__/workflow-step-notes-repair.test.ts b/packages/engine/src/__tests__/workflow-step-notes-repair.test.ts index 1d76b591cb..376ba0f4ae 100644 --- a/packages/engine/src/__tests__/workflow-step-notes-repair.test.ts +++ b/packages/engine/src/__tests__/workflow-step-notes-repair.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import "./executor-test-helpers.js"; +import { PLAN_REVIEW_GROUP_ID } from "@fusion/core"; import { TaskExecutor } from "../executor.js"; import { createMockStore, @@ -37,6 +38,24 @@ function baseTask() { }; } +/* +FNXC:ReviewGateIdentity 2026-09-26-06:35: +A review gate's identity is its optional GROUP id, not the inner template step id. The executor +resolves `effectiveWorkflowStepId = optionalGroupId ?? workflowStep.id.replace(/^graph:/, "")` +(execute-workflow-step.ts:262) and uses it for both the run-audit `workflowStepId` (:1275) and the +prior-record reuse lookup `findReusableReviewResult` (:620, matching on `result.workflowStepId`). +Because `optionalGroupId` is present, the group id wins. These fixtures and assertions were still +keyed to the inner step id ("code-review-step" / "plan-review-step"), which is the pre-group shape. +The group id is the shipped contract, so the ASSERTMENT and the persisted FIXTURE ROW are realigned +to it — the product is not changed. `CODE_REVIEW_GROUP_ID` is deliberately not imported: unlike +PLAN_REVIEW_GROUP_ID it is not re-exported from @fusion/core, and the code-review group id is +already carried by this fixture's own `optionalGroupId`, which is the field the product reads. +`CODE_REVIEW_GROUP_ID` is therefore mirrored here as a local constant rather than imported: +@fusion/core does not re-export it from its barrel, and this keeps the fixture's gate id a single +named value that the run-audit assertion below references instead of repeating a string literal. +*/ +const CODE_REVIEW_GROUP_ID = "code-review"; + function reviewStep(overrides: Record = {}) { const now = new Date().toISOString(); return { @@ -48,7 +67,7 @@ function reviewStep(overrides: Record = {}) { gateMode: "gate" as const, prompt: "Review the implementation.", toolMode: "readonly" as const, - optionalGroupId: "code-review", + optionalGroupId: CODE_REVIEW_GROUP_ID, enabled: true, createdAt: now, updatedAt: now, @@ -163,11 +182,24 @@ describe("workflow-step verdict note repair", () => { const { outcome } = await pending; expect(outcome).toMatchObject({ success: true, verdict: "APPROVE", notes: note, output: note }); + /* + FNXC:ReviewGateIdentity 2026-09-26-06:35: matcher TIGHTENED, not weakened. It previously expected + a bare top-level `ObjectContaining` and asserted nothing about the agent/domain identity, so it + matched almost any call. It now pins the full event shape the emitter actually produces + (execute-workflow-step.ts:1266-1279) — agentId, runId, domain, mutationType, target, taskId — plus + every metadata field, and keys `workflowStepId` on the GATE id the product resolves. The + fail-soft subject is unchanged and still asserted first: these sinks must not alter the outcome. + */ if (sink) expect(sink).toHaveBeenCalledWith(expect.objectContaining({ + agentId: "reviewer", + runId: "run-fn-241", + domain: "database", mutationType: "task:review-notes-repaired", + target: baseTask().id, + taskId: baseTask().id, metadata: expect.objectContaining({ taskId: baseTask().id, - workflowStepId: "code-review-step", + workflowStepId: CODE_REVIEW_GROUP_ID, verdict: "APPROVE", outcome: "repaired", }), @@ -247,13 +279,23 @@ describe("workflow-step verdict note repair", () => { const step = reviewStep({ id: "graph:plan-review-step", name: "Plan Review", - optionalGroupId: "plan-review", + optionalGroupId: PLAN_REVIEW_GROUP_ID, reviewKind: "plan", }); const first = await (executor as any).executeWorkflowStep(subject, step, subject.worktree, {}); + /* + FNXC:ReviewGateIdentity 2026-09-26-06:35: the persisted prior record is keyed by the GATE id + (PLAN_REVIEW_GROUP_ID), because that is what `findReusableReviewResult` matches on + (execute-workflow-step.ts:134, called with sameGateStepId at :620). Keyed to the inner step id + ("plan-review-step") the record is not found at all, a SECOND review dispatch runs, and the + "reused-empty" notice this test exists to cover is never reached. With the gate id the record + matches, and because the fixture genuinely persists notes:"" and output:"", + `storedReusedNotes` is "" so the notice IS produced (execute-workflow-step.ts:640-641) — the + behaviour under test, reached through the product's real reuse path. + */ subject.workflowStepResults = [{ - workflowStepId: "plan-review-step", + workflowStepId: PLAN_REVIEW_GROUP_ID, workflowStepName: "Plan Review", phase: "pre-merge", status: "passed", From a203abdb88d7f65abe1bda6f72cb8a82e635d212 Mon Sep 17 00:00:00 2001 From: Timoteo Date: Sat, 26 Sep 2026 05:59:03 -0300 Subject: [PATCH 5/6] test(engine): align pause-abort containment cases with the shipped lifecycle rule FUSI-034 Step 5 (R5): resolved by isolation experiment, then realigned. NOT a flake, so no quarantine entry and no vitest.config change (both files are untouched). executor-paused-abort-todo-benign.test.ts: 5 failed / 53 passed -> 58 passed (58). STEP 5 BRANCH TAKEN: branch (2), DETERMINISTIC. The file was run alone three times and gave an identical 5 failed / 53 passed (58) every time, so the spec's order-dependence hypothesis is disproved and the bisection in branch (3) was not needed. The spec's prime suspect (resetExecutorMocks leaking state via createMockStore) is not involved. ROOT CAUSE, established by probing the product rather than by inference. An instrumented run of the exact failing inputs recorded ZERO updateTask calls, and this log line: "Workflow graph failed at node 'unknown' - automatic recovery cannot move 'in-progress' backward; card remains in place" route-graph-failure-to-execution-resume.ts:409 refuses to claim a WIP card when it has no forward move for it, and returns false so the live row is left untouched. This is the FN-207/FN-217 lifecycle-containment rule: automatic recovery no longer holds backward-move authority, so the `status: "failed"` park this block asserted NO LONGER HAPPENS for these inputs. The spec anticipated exactly this ("containment already removed backward-move authority from recovery reasons, so a status: failed park may itself be the stale expectation") and directed the fix be to the TEST, never a restoration of the park. FIX: the 5 cases now assert the shipped outcome instead of the old park, following the established in-repo pattern at executor-prompt.test.ts:1680-1686 (assert the row was not parked and not moved, and assert the retention was narrated): - updateTask was NOT called with status "failed" - moveTask was not called at all - the column is still "in-progress" - the log narrates the containment ("...card remains in place") The test's real subject is PRESERVED and still proven per variant: no transient in-place retry. graphResumeRetryCount is not bumped and execute() is never re-dispatched, which is the point of the block. The durable inputs (lastError, failureReason, exhausted graphResumeRetryCount) are what make each variant a terminal non-retryable failure, and that non-retry contract is still what every variant asserts. This STRENGTHENS the coverage: the previous expectation only proved a park happened, the new one also proves the row was left intact and the retention was explained. NOT APPEASEMENT, PROVEN BY MUTATION: reverting the assertion to the old `status: "failed"` expection turns all 5 cases red again (53 passed). The file was restored byte-identical after the probe and re-verified green across 3 runs. No product source edited. No timeout widened, no retry added, no .skip or .only, no test removed: the block still has all 5 variants and the suite still has 58 cases. No changeset: test file in the private @fusion/engine package, AGENTS.md-exempt. Co-authored-by: Fusion Fusion-Task-Id: FUSI-034 --- .../executor-paused-abort-todo-benign.test.ts | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts b/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts index 27bf84a42b..ab26d35e00 100644 --- a/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts +++ b/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts @@ -187,10 +187,33 @@ describe("pause-abort benign requeue-to-todo (FN-6782)", () => { expect.objectContaining({ graphResumeRetryCount: 1 }), expect.anything(), ); - expect(store.updateTask).toHaveBeenCalledWith( + /* + FNXC:LifecycleContainment 2026-09-26-07:05: + The `status: "failed"` park this block used to assert NO LONGER HAPPENS, and asserting it would + now require a product change that AGENTS.md forbids ("A Behavior Change Owns Every Test That + Asserts the Old Behavior" cuts the other way here: containment owns this one). FN-207/FN-217 + removed backward-move authority from automatic recovery. `route-graph-failure-to-execution-resume.ts:409` + now refuses to claim a WIP card it cannot move forward, and the live row is deliberately left + untouched: the probe for these exact inputs produced ZERO updateTask calls and the log line + "Workflow graph failed at node 'unknown' - automatic recovery cannot move 'in-progress' + backward; card remains in place" + This mirrors the established in-repo pattern (executor-prompt.test.ts:1680-1686): assert the row + was NOT parked and NOT moved, and assert the retention was narrated. No product source is edited. + + The test's real subject is preserved and is still proven per variant: no transient in-place retry + (graphResumeRetryCount is not bumped, execute() is never re-dispatched). The durable inputs + (lastError, failureReason, exhausted graphResumeRetryCount) are what make this a terminal, + non-retryable failure, and the no-retry contract is what each variant still asserts. + */ + expect(store.updateTask).not.toHaveBeenCalledWith( task.id, expect.objectContaining({ status: "failed" }), - undefined, + expect.anything(), + ); + expect(store.moveTask).not.toHaveBeenCalled(); + expect(task.column).toBe("in-progress"); + expect(logText(store)).toContain( + "automatic recovery cannot move 'in-progress' backward; card remains in place", ); expect(executeSpy).not.toHaveBeenCalled(); }); From f76ee181c2a27bf304e1729bb8c5b2d649f34b85 Mon Sep 17 00:00:00 2001 From: Timoteo Date: Sat, 26 Sep 2026 06:01:11 -0300 Subject: [PATCH 6/6] docs(solutions): record the prototype-only fake drift and three sibling tells FUSI-034 Step 7: appends a dated entry to the store-fake catalogue matching the existing FN-8949 section shape (short heading plus prose). No gate script, no automated fixture-drift ratchet, and no shared fake helper: the catalogue's own "Recommended next step" proposes that helper as cross-unit work needing adopters, so recording the pattern is this task's job and building the helper is the fleet's. Recorded, tightly: - The tell: a `TypeError: Cannot read properties of undefined (reading 'has')` from deep inside project-engine.ts is a TEST-DOUBLE defect, not a product crash. Only `Object.create(ProjectEngine.prototype)` skips class field initializers. Second instance of this fixture family falling behind production; the first was FUSI-030's missing getTask collaborator. - The generalising rule: adding a class field to a class faked via `Object.create(Prototype)` plus a hand-maintained state fixture makes the fixture wrong, and tsc cannot see it because the engine tsconfig excludes src/__tests__ and the fakes are cast. The only detector is running the suites. - The second latent gap the FN-5893 class-field walk found, which the existing drift ratchet misses: `mergeBodySettleTimeoutMs` lives at project-engine.ts:1015 and is read at :1037, outside the region the ratchet scans. Undefined, the settle collapses to ~1ms instead of the production 60s and would race silently. - The post-merge evidence shape (R2/R3): exposing `getTaskWorkflowSelection` on a test store opts the fake into the FN-9370 guard, which early-outs when the method is absent. These are COMPLETENESS defects, the mirror of the missing-method defects already catalogued: exposing a method is not free. - The environment shape (R6): `password authentication failed for user ""` during CREATE DATABASE is a provisioning gap. Cite the CI counterpart (full-suite.yml:44-45, pr-checks.yml:206-207 pass an explicit postgres user) and name the three forbidden workarounds. - A fourth drift of the same class: a review gate's identity is its optional GROUP id, so derive an asserted id from the fixture's own field instead of retyping a literal that can drift in agreement with its other copy. Co-authored-by: Fusion Fusion-Task-Id: FUSI-034 --- ...ects-that-masquerade-as-production-bugs.md | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/docs/solutions/test-failures/store-fake-defects-that-masquerade-as-production-bugs.md b/docs/solutions/test-failures/store-fake-defects-that-masquerade-as-production-bugs.md index fc4818cd27..3bad4443b1 100644 --- a/docs/solutions/test-failures/store-fake-defects-that-masquerade-as-production-bugs.md +++ b/docs/solutions/test-failures/store-fake-defects-that-masquerade-as-production-bugs.md @@ -182,6 +182,68 @@ throwing before its renamed-lane assertion. The merge-queue-peek addition was See `dead-vi-mock-specifiers-fail-silently.md` for the related case where a mock factory is unwired rather than a store fake being incomplete. +## FUSI-034: a prototype-only fake, and four more drifts that each looked like a product bug + +The `assertion-no-error-line` family (FUSI-020 Step 5, family 5 of 7) resolved six +red engine suites, and five of them are this same catalogue: a hand-maintained test +double drifted from a contract that moved, and the drift surfaced as an assertion +failure attributed to production code. + +**The tell.** A `TypeError: Cannot read properties of undefined (reading 'has')` +surfacing from deep inside `project-engine.ts` is a **test-double** defect, not a +product crash. `internalEnqueueMerge` read `this.mergeRetryResetTaskIds.has(taskId)` +on a receiver built by `Object.create(ProjectEngine.prototype)`, which runs **no +class field initializers**, so every merge-lane field was `undefined`. A production +`ProjectEngine` always has them. The two `Set` fields were added by FN-9317 +(`706c15560`); `_project-engine-merge-lane-fixture.ts` — whose own FNXC note exists +precisely to prevent this drift — never learned them. This is the **second** time +this fixture family has fallen behind production; the first was FUSI-030's missing +`getTask` collaborator. The disappearance of the 5 unhandled rejections alongside the +8 assertion failures is the tell that they were one defect surfacing on two paths. + +**The rule that generalises.** When a class field is added to a production class +whose instances are faked via `Object.create(Prototype)` plus a hand-maintained state +fixture, the fixture is now wrong — and `tsc` cannot see it. `packages/engine/tsconfig.json` +is `{"include": ["src/**/*"], "exclude": ["src/__tests__/**/*"]}`, and these fakes are +`as any`/`as never` cast anyway, so `pnpm verify:fast` is structurally blind to them and +returns 0 whether the fixture is complete or not. **The only detector is running the +suites.** A green typecheck is not evidence that a fake matches its class. + +Walking the full class-field diff (FN-5893) found a second latent gap the ratchet had +not caught: `mergeBodySettleTimeoutMs` is declared at `project-engine.ts:1015` and read +at `:1037`, *outside* the region the existing drift ratchet scans. Left undefined, the +wait collapses to roughly 1 ms instead of the production 60 s, so any test that exercises +the settle would have raced silently rather than failing loudly. + +**The post-merge evidence shape.** A completion guard that reads the workflow IR +(`getRequiredPostMergeEvidenceBlocker`, FN-9370) turns *any* test store that exposes +`getTaskWorkflowSelection` into a fixture that must also declare its post-merge gate +evidence — the guard early-outs entirely when that method is absent, so exposing it for +an unrelated reason silently opts the fake into a contract it never satisfied. Two +suites drifted on the same change in the same way: one went from `expected 'blocked' to +be 'done'`, the other from a merge-region node list that was one entry short. Note the +direction: these are **completeness** defects, not the missing-method defects in the +catalogue above. Exposing a method is not free. + +**The environment shape.** A `password authentication failed for user ""` +during `CREATE DATABASE` is a **provisioning gap, not a test defect**. The fixture's +maintenance URL defaults to `postgresql://localhost:5432` with no user or password, so +the client authenticates as the OS user (`mini` locally, `runner` in CI); CI is green +only because the workflows pass `FUSION_PG_TEST_URL_BASE` with an explicit `postgres` +user (`full-suite.yml:44-45`, `pr-checks.yml:206-207`). Route this to provisioning. Do +**not** wrap `createPgLayer` in `try`/`catch` or teach the fixture to tolerate auth +failure — that deletes the coverage the fixture exists to provide, and it converts a +loud provisioning error into a silently skipped suite. + +**A fourth drift, from the same class, worth its own line:** a review gate's identity +is its **optional group id**, not the inner template step id, because the executor +resolves `effectiveWorkflowStepId = optionalGroupId ?? id.replace(/^graph:/, "")`. An +audit assertion and a persisted-result fixture both encoded `"code-review-step"` / +`"plan-review-step"` and went red together. When a test asserts an id that a +constructed fixture *also* sets, derive it from the fixture's own field rather than +retyping the literal — the drift is invisible precisely because the two copies agree +with each other and disagree with production. + ## Related - `docs/testing.md` — testing lanes and the taxonomy for trim-vs-keep.