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. 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, 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(); }); 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 }); 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); 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",