From 2e4905fa0e56797eca8995497885d72cecb74ea2 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 08:28:32 -0700 Subject: [PATCH] =?UTF-8?q?refactor:=20one=20definition=20of=20"which=20co?= =?UTF-8?q?lumns=20are=20review"=20=E2=80=94=20three=20copies=20deleted=20?= =?UTF-8?q?onto=20core's=20resolver=20(#2751)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **#2730 added `resolveReviewColumns` to core. This deletes the three copies that predated it.** Measured on `origin/main` before this change — three in-tree definitions, **none of which agreed**: | site | definition | |---|---| | `core/workflow-lifecycle-traits.ts` (#2730, authoritative) | mergeOrchestration ∪ mergeBlocker ∪ humanReview — **all** columns | | `dashboard/routes/register-task-workflow-routes.ts` | mergeBlocker ∪ humanReview ∪ **first** mergeOrchestration | | `cli/src/extension.ts` | mergeBlocker ∪ humanReview ∪ **first** mergeOrchestration | | `cli/src/commands/task.ts` | all three, full union | **Both `.slice(0, 1)` variants are mine**, from #2723's review round: I narrowed to core's then-single `.review` because the reviewer was right that a superset let the dashboard act on a lane the engine did not own. #2730 answered that question authoritatively in the other direction, so the narrowing is obsolete. Worse, and the part that makes this urgent rather than tidy: **the two CLI copies had already drifted apart inside #2728.** `fn_task_retry` refused a card in a second merge lane that `fn task retry` accepted — two surfaces, one operator action, two answers, from two copies of one definition written days apart by me. All three now call core. The dashboard keeps its thin store→IR wrapper (its callers hold a store and a task id, not an IR) but the **body** is core's. ## One assertion inverted, deliberately My #2723 case asserted that a **second** `mergeOrchestration` column is **refused**. Core says every merge lane is review, so the behaviour legitimately changed and the assertion flips with it. **Kept rather than deleted**, because the invariant under test — *the routes agree with core* — is unchanged. Deleting the case would have hidden that its answer moved; inverting it records which decision moved and why. A test whose expectation quietly disappears is indistinguishable from a test that was wrong. ## A footgun found while rebasing The shipped signature is `isInReviewMissingWorktreeSessionStartFailure(task, isReviewColumn?: boolean)` — the merged version takes the **answer**, not the lanes. My branch had passed a `ReadonlySet`, and because the parameter is `boolean | undefined` with a `??` default, **a truthy object makes it answer `true` for every column**. TypeScript stops typed callers; my test only reached it through an `as never` cast, which is how I found it. All three production call sites correctly pass `retryReviewColumns.has(task.column)` — now asserted structurally so a fourth surface cannot omit it. The boolean is arguably the better shape, and I'd keep it: there is nothing left for the callee to re-derive, so it cannot disagree with the caller's own membership test. ## The ratchet No surface may reintroduce a local review union (`columnsWithFlag(…, "mergeBlocker" | "humanReview")`). Those three copies appeared because each was added **in good faith, in a different review round, by someone reading only their own call site** — which no amount of care prevents and a ratchet does. ## Verification census **553** · `pnpm test:gate` **487 / 10 / 71** · `tsc` clean in cli and dashboard · `pnpm lint` clean · 10/10 in each touched suite. **Pre-existing, not mine:** `register-task-workflow-routes.move-bypassguards.test.ts` fails on `origin/main` (400 vs 200) — already reported on #2723. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../__tests__/cli-active-count-lanes.test.ts | 42 +++++++++++++++- packages/cli/src/commands/task.ts | 40 +++++++-------- packages/cli/src/extension.ts | 20 ++++---- .../__tests__/ai-session-diagnostics.test.ts | 46 +++++++++++------ .../src/__tests__/board-workflows.test.ts | 20 +++++++- packages/dashboard/src/__tests__/chat.test.ts | 11 +++++ .../src/__tests__/devserver-manager.test.ts | 20 ++++++++ .../devserver-preview-detect.test.ts | 20 ++++++++ .../__tests__/issue-image-attachments.test.ts | 11 +++++ .../__tests__/register-signal-routes.test.ts | 16 +++++- .../src/__tests__/routes-tasks.test.ts | 14 +++++- packages/dashboard/src/__tests__/sse.test.ts | 22 ++++++--- ...flow-routes.resolved-review-column.test.ts | 20 ++++++-- .../routes/register-task-workflow-routes.ts | 49 +++++-------------- 14 files changed, 250 insertions(+), 101 deletions(-) diff --git a/packages/cli/src/__tests__/cli-active-count-lanes.test.ts b/packages/cli/src/__tests__/cli-active-count-lanes.test.ts index 87f9c08e61..79094d4328 100644 --- a/packages/cli/src/__tests__/cli-active-count-lanes.test.ts +++ b/packages/cli/src/__tests__/cli-active-count-lanes.test.ts @@ -177,7 +177,47 @@ describe("the shared missing-worktree classifier takes the caller's resolved rev const code = (await readFile(surface, "utf8")).replace(/\/\*[\s\S]*?\*\//g, ""); const call = code.match(/isInReviewMissingWorktreeSessionStartFailure\(([^)]*)\)/); expect(call, `${surface.pathname} does not call the classifier`).toBeTruthy(); - expect(call?.[1], `${surface.pathname} calls it without a resolved review answer`).toContain(","); + /* + FNXC:WorkflowLifecycleColumns 2026-08-02-23:25 (PR #2751 review — greptile P2): + TIED TO THE RESOLVED MEMBERSHIP TEST, not just to "a second argument exists". `contains a comma` is + satisfied by `(task, true)` or any unrelated flag while that surface classifies renamed lanes differently + from the other two — a guard that reports success without checking anything. + + Matched on the whitespace-normalised SOURCE rather than a capture group: `[^)]*` stops at the first `)`, so + it truncates `retryReviewColumns.has(task.column)` mid-expression and fails against CORRECT code. A + ratchet that fails on a correct tree is as bad as one that passes on a broken one. + */ + const normalised = code.replace(/\s+/g, ""); + expect( + normalised, + `${surface.pathname} must pass retryReviewColumns.has(task.column) to the classifier`, + ).toContain("isInReviewMissingWorktreeSessionStartFailure(task,retryReviewColumns.has(task.column))"); + } + }); + + it("uses ONE definition of the review columns — core's resolveReviewColumns", async () => { + /* + FNXC:WorkflowLifecycleColumns 2026-08-02-23:40 (the consolidation this PR is about): + Three in-tree copies of "which columns are review" disagreed with each other and with core (#2730): the + dashboard route and the pi extension each took only the FIRST mergeOrchestration column while the CLI + command took the full union, so `fn_task_retry` refused a card in a second merge lane that `fn task retry` + accepted. + + This fails if any surface grows a local union again, which is how the three appeared in the first place — + each added in good faith, in a different review round, by someone reading only their own call site. + */ + const { readFile } = await import("node:fs/promises"); + const surfaces = [ + new URL("../extension.ts", import.meta.url), + new URL("../commands/task.ts", import.meta.url), + new URL("../../../dashboard/src/routes/register-task-workflow-routes.ts", import.meta.url), + ]; + + for (const surface of surfaces) { + const code = (await readFile(surface, "utf8")).replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, ""); + expect(code, `${surface.pathname} must use core's resolveReviewColumns`).toContain("resolveReviewColumns("); + expect(code, `${surface.pathname} still rolls its own review union`).not.toMatch(/columnsWithFlag\([^)]*"mergeBlocker"\)/); + expect(code, `${surface.pathname} still rolls its own review union`).not.toMatch(/columnsWithFlag\([^)]*"humanReview"\)/); } }); }); diff --git a/packages/cli/src/commands/task.ts b/packages/cli/src/commands/task.ts index be3f7c759f..38ab635d88 100644 --- a/packages/cli/src/commands/task.ts +++ b/packages/cli/src/commands/task.ts @@ -1,4 +1,4 @@ -import { TaskStore, COLUMNS, COLUMN_LABELS, columnsWithFlag, resolveTaskLifecycleColumns, resolveWorkflowIrForTask, CentralCore, buildAutoPauseClearPatch, buildManualRetryResetPatch, extractIntentSignature, findNearDuplicates, getTaskDuplicateLineage, isWorkspaceTask, reconcileDeterministicDuplicate, resolveTaskGithubTracking, runDeterministicDuplicateGuard, type Settings, type Column, type ColumnId, type StepStatus, type AgentLogType, type AgentLogEntry, type IntentSignature, type NearDuplicateCandidate, type NearDuplicateMatch, type TaskDependencyMutation } from "@fusion/core"; +import { TaskStore, COLUMNS, COLUMN_LABELS, resolveReviewColumns, resolveTaskLifecycleColumns, resolveWorkflowIrForTask, CentralCore, buildAutoPauseClearPatch, buildManualRetryResetPatch, extractIntentSignature, findNearDuplicates, getTaskDuplicateLineage, isWorkspaceTask, reconcileDeterministicDuplicate, resolveTaskGithubTracking, runDeterministicDuplicateGuard, type Settings, type Column, type ColumnId, type StepStatus, type AgentLogType, type AgentLogEntry, type IntentSignature, type NearDuplicateCandidate, type NearDuplicateMatch, type TaskDependencyMutation } from "@fusion/core"; import { isInReviewMissingWorktreeSessionStartFailure, runAiMerge, landWorkspaceTask, installBaselineArchiveWorktreeDisposer } from "@fusion/engine"; import { createInterface } from "node:readline/promises"; import type { PlanningQuestion, PlanningSummary } from "@fusion/core"; @@ -1346,32 +1346,26 @@ export async function runTaskRetry(id: string, projectName?: string) { before claiming a lane is converted. */ /* - FNXC:WorkflowLifecycleColumns 2026-07-31-06:10 (PR #2728 review — greptile, both lane findings): - THE REVIEW LANE IS A SET HERE TOO. + FNXC:WorkflowLifecycleColumns 2026-08-02-22:10 (consolidation onto #2730's core resolver): + ONE DEFINITION, IN CORE. `resolveReviewColumns` is now the authoritative answer to "which columns are + review", and this inline union was one of THREE in-tree copies that disagreed with each other: - `resolveTaskLifecycleColumns(...).review` is a single id from ONE flag (`mergeOrchestration`), so - this gate refused a stalled card in a `humanReview`-only lane, and refused a card in a SECOND merge - lane that the dashboard's retry route accepts. Same operator action, different answer per surface — - which is precisely the disagreement this PR was opened to remove, reappearing one level down. + core (#2730): mergeOrchestration u mergeBlocker u humanReview — ALL columns + dashboard routes: mergeBlocker u humanReview u FIRST mergeOrchestration + cli/src/extension.ts: mergeBlocker u humanReview u FIRST mergeOrchestration + this copy: all three, full union - The union matches `resolveReviewColumnsForTask` in the dashboard routes and the notifier's copy. - That is now FOUR inline copies of one definition; #2730 adds `resolveReviewColumns` to core so they - can converge. Not imported here yet because #2730 is unmerged and stacking on an open PR is what - stranded #2568 four deep. + The two `.slice(0, 1)` variants were MINE, from #2723's review round: I narrowed to core's then-single + `.review` because the reviewer was right that a superset let the dashboard act on a lane the engine did not + own. #2730 answered that question authoritatively in the other direction, so the narrowing is obsolete — and + keeping any local copy means re-litigating arity per call site forever, which is what produced three answers. + + The legacy fallback stays: an unresolvable or column-less IR keeps `in-review`, so boards that never declared + traits are unchanged. */ const retryIr = await resolveWorkflowIrForTask(context.store, id).catch(() => undefined); - const retryReviewColumns = new Set( - retryIr === undefined - ? ["in-review"] - : (() => { - const lanes = [ - ...columnsWithFlag(retryIr, "mergeOrchestration"), - ...columnsWithFlag(retryIr, "mergeBlocker"), - ...columnsWithFlag(retryIr, "humanReview"), - ]; - return lanes.length > 0 ? lanes : ["in-review"]; - })(), - ); + const resolvedReviewColumns = retryIr === undefined ? [] : resolveReviewColumns(retryIr); + const retryReviewColumns = new Set(resolvedReviewColumns.length > 0 ? resolvedReviewColumns : ["in-review"]); const isInReviewStatusNone = retryReviewColumns.has(task.column) && (task.status === null || task.status === undefined); const hasIncompleteSteps = task.steps.some( diff --git a/packages/cli/src/extension.ts b/packages/cli/src/extension.ts index c9d2034e3e..7d46271175 100644 --- a/packages/cli/src/extension.ts +++ b/packages/cli/src/extension.ts @@ -37,7 +37,7 @@ import { type SecretScope, resolveTaskLifecycleColumns, resolveWorkflowIrForTask, - columnsWithFlag, + resolveReviewColumns, } from "@fusion/core"; import { getGhErrorMessage, @@ -1887,17 +1887,19 @@ export default function kbExtension(pi: ExtensionAPI) { #2713), and the same note applies: three copies of one predicate is the argument for a set-returning resolver in core, which is a follow-up rather than a rider on this PR. */ + /* + FNXC:WorkflowLifecycleColumns 2026-08-02-22:15 (consolidation onto #2730's core resolver): + CORE'S `resolveReviewColumns` — see the fuller note in `commands/task.ts`. This copy carried + `.slice(0, 1)` on the merge-orchestration lanes while the CLI command took the full union, so + `fn_task_retry` refused a card in a SECOND merge lane that `fn task retry` accepted: two surfaces, one + operator action, two answers. That is the exact defect #2728 was opened to remove, reproduced by two + copies of one definition drifting apart within a single PR. + */ const retryIr = await resolveWorkflowIrForTask(store, params.id).catch(() => undefined); + const resolvedRetryReviewColumns = retryIr === undefined ? [] : resolveReviewColumns(retryIr); const retryReviewColumns = new Set( - retryIr - ? [ - ...columnsWithFlag(retryIr, "mergeBlocker"), - ...columnsWithFlag(retryIr, "humanReview"), - ...columnsWithFlag(retryIr, "mergeOrchestration").slice(0, 1), - ] - : [], + resolvedRetryReviewColumns.length > 0 ? resolvedRetryReviewColumns : ["in-review"], ); - if (retryReviewColumns.size === 0) retryReviewColumns.add("in-review"); const isInReviewStatusNone = retryReviewColumns.has(task.column) && (task.status === null || task.status === undefined); const hasIncompleteSteps = task.steps.some( diff --git a/packages/dashboard/src/__tests__/ai-session-diagnostics.test.ts b/packages/dashboard/src/__tests__/ai-session-diagnostics.test.ts index 0124cfca17..851ab9342f 100644 --- a/packages/dashboard/src/__tests__/ai-session-diagnostics.test.ts +++ b/packages/dashboard/src/__tests__/ai-session-diagnostics.test.ts @@ -289,17 +289,19 @@ describe("ai-session-diagnostics", () => { // Reset to null - should restore default console behavior setDiagnosticsSink(null); - // After reset to null, logs should go to console (not to captured array) - const consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + /* After reset to null, logs go to the console again — on `console.error`, because the default sink now + emits through core's `createLogger` and everything but `warn` is stderr (see the note below on the + severity marker). Spying on `console.log` asserted the pre-logger channel. */ + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const diagnostics = createSessionDiagnostics("test"); diagnostics.info("Test after reset"); // The captured array should be unchanged (default sink is used) expect(captured).toHaveLength(0); // Console should have been called with the default sink - expect(consoleLogSpy).toHaveBeenCalled(); + expect(consoleErrorSpy).toHaveBeenCalled(); - consoleLogSpy.mockRestore(); + consoleErrorSpy.mockRestore(); }); it("resetting to undefined sets the default sink", () => { @@ -517,22 +519,36 @@ describe("ai-session-diagnostics", () => { describe("default sink behavior", () => { it("logs info to console.log with prefix", () => { - const consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + /* + FNXC:DashboardTestMocks 2026-08-03-04:40 (red on main — the sink moved to createLogger, the test did not): + `defaultSink` now emits through core's `createLogger(scope)` rather than calling `console.log/warn/error` + directly, and that logger has TWO deliberate differences these cases predate: + + 1. it prefixes a SEVERITY MARKER (`\u0000fnlvl=\u0000`) so the TUI and the log panes can classify a + line without parsing it; + 2. `log()` goes to `console.error`, not `console.log` — everything but `warn` is stderr, which is what keeps + stdio-transport surfaces (MCP) from having their protocol stream polluted by diagnostics. + + So asserting `console.log` with a bare `"[planning]"` prefix describes a shape that no longer ships. Asserting + the CHANNEL and the marker-prefixed message keeps these cases pinned to the contract that matters — a + classifiable line on the right stream — instead of to the pre-logger call site. + */ + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); resetDiagnosticsSink(); // Ensure default sink is active const diagnostics = createSessionDiagnostics("planning"); diagnostics.info("Test message"); - expect(consoleLogSpy).toHaveBeenCalledWith( - "[planning]", - "Test message", + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("[planning] Test message"), expect.objectContaining({ _emittedAt: expect.any(String) }) ); + expect(consoleErrorSpy.mock.calls[0]?.[0]).toContain("fnlvl=info"); - consoleLogSpy.mockRestore(); + consoleErrorSpy.mockRestore(); }); - it("logs warn to console.warn with prefix", () => { + it("logs warn to console.warn with a severity-marked prefix", () => { const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); resetDiagnosticsSink(); @@ -540,15 +556,15 @@ describe("ai-session-diagnostics", () => { diagnostics.warn("Warning message"); expect(consoleWarnSpy).toHaveBeenCalledWith( - "[planning]", - "Warning message", + expect.stringContaining("[planning] Warning message"), expect.objectContaining({ _emittedAt: expect.any(String) }) ); + expect(consoleWarnSpy.mock.calls[0]?.[0]).toContain("fnlvl=warn"); consoleWarnSpy.mockRestore(); }); - it("logs error to console.error with prefix", () => { + it("logs error to console.error with a severity-marked prefix", () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); resetDiagnosticsSink(); @@ -556,10 +572,10 @@ describe("ai-session-diagnostics", () => { diagnostics.error("Error message"); expect(consoleErrorSpy).toHaveBeenCalledWith( - "[planning]", - "Error message", + expect.stringContaining("[planning] Error message"), expect.objectContaining({ _emittedAt: expect.any(String) }) ); + expect(consoleErrorSpy.mock.calls[0]?.[0]).toContain("fnlvl=error"); consoleErrorSpy.mockRestore(); }); diff --git a/packages/dashboard/src/__tests__/board-workflows.test.ts b/packages/dashboard/src/__tests__/board-workflows.test.ts index e551db291d..9009bff8f5 100644 --- a/packages/dashboard/src/__tests__/board-workflows.test.ts +++ b/packages/dashboard/src/__tests__/board-workflows.test.ts @@ -96,13 +96,29 @@ describe("buildBoardWorkflowsPayload built-in column labels", () => { ); const workflow = payload.workflows.find(({ id }) => id === "builtin:coding"); const named = Object.fromEntries((workflow?.columns ?? []).map((column) => [column.id, column.name])); + /* + FNXC:WorkflowLifecycleColumns 2026-08-03-03:30 (red on main — the deleted-column class again): + THE DEFAULT LINEAGE HAS FIVE LIFECYCLE COLUMNS, NOT SIX. #2515 merged Todo into Planning: the id `todo` + survives carrying the label "Planning", and `triage` is gone. This expectation still asserted both a + `triage: "Planning"` key and a `todo: "Todo"` one, so it described a board that has not shipped since that + change. + + Same class as the 23 assertions corrected in #2758 and the two in #2720, and the same tell: the failure + reads like a canonicalisation bug ("expected triage: Planning") when the canonicalisation is right and the + expectation is stale. + + The invariant this case exists for is unchanged and still asserted: the built-in coding workflow'"'"'s columns + carry their CANONICAL labels rather than raw ids — which is what the sibling case above contrasts against a + built-in that deliberately renames one. + */ expect(named).toMatchObject({ - triage: "Planning", - todo: "Todo", + todo: "Planning", "in-progress": "In Progress", "in-review": "In Review", done: "Done", archived: "Archived", }); + // And the merged column is gone, so nothing should be canonicalising a `triage` label any more. + expect(named).not.toHaveProperty("triage"); }); }); diff --git a/packages/dashboard/src/__tests__/chat.test.ts b/packages/dashboard/src/__tests__/chat.test.ts index 6c608d9f69..0ced64a8a9 100644 --- a/packages/dashboard/src/__tests__/chat.test.ts +++ b/packages/dashboard/src/__tests__/chat.test.ts @@ -25,6 +25,17 @@ vi.mock("node:fs/promises", async (importOriginal) => { // Mock @fusion/core to prevent cascade loading of real fs modules vi.mock("@fusion/core", () => ({ + /* + FNXC:DashboardTestMocks 2026-08-03-04:10 (whole-file red on main — a mock factory missing one export): + `createLogger` is stubbed because a `vi.mock("@fusion/core", …)` factory REPLACES the module: any export the + module under test (or anything it transitively imports) reaches for and the factory omits throws + `No "createLogger" export is defined`, which fails the ENTIRE file rather than one case. + + That makes this class systemic rather than local: every PR that adds a `createLogger` call to a module inside + this import graph reddens every suite whose factory predates it, and the failure names the mock rather than the + change that caused it. Four whole-file reds on main came from two missing exports (this and `execFile`). + */ + createLogger: () => ({ log: () => undefined, debug: () => undefined, warn: () => undefined, error: () => undefined }), summarizeTitle: vi.fn(), // FNXC:DashboardChatTests 2026-07-08-12:00: FN-7675 added FUSION_RUNTIME_SELF_AWARENESS to chat.ts's direct @fusion/core imports (CHAT_SYSTEM_PROMPT embeds it). FUSION_RUNTIME_SELF_AWARENESS: "", diff --git a/packages/dashboard/src/__tests__/devserver-manager.test.ts b/packages/dashboard/src/__tests__/devserver-manager.test.ts index 48cbc576b9..04294012c6 100644 --- a/packages/dashboard/src/__tests__/devserver-manager.test.ts +++ b/packages/dashboard/src/__tests__/devserver-manager.test.ts @@ -9,6 +9,26 @@ const { spawnMock, createConnectionMock } = vi.hoisted(() => ({ })); vi.mock("node:child_process", () => ({ + /* + FNXC:DashboardTestMocks 2026-08-03-04:20 (whole-file red on main — same class as the `createLogger` pair): + `execFile` is stubbed because a `vi.mock("node:child_process", …)` factory REPLACES the module: something in + this import graph now calls `execFile`, and an omitted export throws `No "execFile" export is defined`, failing + the WHOLE file rather than one case. + + Stubbed as a no-op that invokes its callback with empty output, which is what every consumer in this graph + needs to proceed; a test that actually depends on an execFile result should assert on this mock rather than + rely on the default. + */ + execFile: ( + _file: string, + ..._rest: unknown[] + ) => { + const callback = _rest.find((argument) => typeof argument === "function") as + | ((error: Error | null, stdout: string, stderr: string) => void) + | undefined; + callback?.(null, "", ""); + return { kill: () => undefined } as never; + }, spawn: spawnMock, })); diff --git a/packages/dashboard/src/__tests__/devserver-preview-detect.test.ts b/packages/dashboard/src/__tests__/devserver-preview-detect.test.ts index ea1569ec4f..4b24a2a370 100644 --- a/packages/dashboard/src/__tests__/devserver-preview-detect.test.ts +++ b/packages/dashboard/src/__tests__/devserver-preview-detect.test.ts @@ -11,6 +11,26 @@ const { spawnMock, createConnectionMock } = vi.hoisted(() => ({ })); vi.mock("node:child_process", () => ({ + /* + FNXC:DashboardTestMocks 2026-08-03-04:20 (whole-file red on main — same class as the `createLogger` pair): + `execFile` is stubbed because a `vi.mock("node:child_process", …)` factory REPLACES the module: something in + this import graph now calls `execFile`, and an omitted export throws `No "execFile" export is defined`, failing + the WHOLE file rather than one case. + + Stubbed as a no-op that invokes its callback with empty output, which is what every consumer in this graph + needs to proceed; a test that actually depends on an execFile result should assert on this mock rather than + rely on the default. + */ + execFile: ( + _file: string, + ..._rest: unknown[] + ) => { + const callback = _rest.find((argument) => typeof argument === "function") as + | ((error: Error | null, stdout: string, stderr: string) => void) + | undefined; + callback?.(null, "", ""); + return { kill: () => undefined } as never; + }, spawn: spawnMock, })); diff --git a/packages/dashboard/src/__tests__/issue-image-attachments.test.ts b/packages/dashboard/src/__tests__/issue-image-attachments.test.ts index 1ed189c9a2..e8660c8689 100644 --- a/packages/dashboard/src/__tests__/issue-image-attachments.test.ts +++ b/packages/dashboard/src/__tests__/issue-image-attachments.test.ts @@ -14,6 +14,17 @@ Both forges' import routes call the same helper, so the helper + its policies ar */ vi.mock("@fusion/core", () => ({ + /* + FNXC:DashboardTestMocks 2026-08-03-04:10 (whole-file red on main — a mock factory missing one export): + `createLogger` is stubbed because a `vi.mock("@fusion/core", …)` factory REPLACES the module: any export the + module under test (or anything it transitively imports) reaches for and the factory omits throws + `No "createLogger" export is defined`, which fails the ENTIRE file rather than one case. + + That makes this class systemic rather than local: every PR that adds a `createLogger` call to a module inside + this import graph reddens every suite whose factory predates it, and the failure names the mock rather than the + change that caused it. Four whole-file reds on main came from two missing exports (this and `execFile`). + */ + createLogger: () => ({ log: () => undefined, debug: () => undefined, warn: () => undefined, error: () => undefined }), isGhAvailable: () => false, isGhAuthenticated: () => false, runGhAsync: vi.fn(async () => ""), diff --git a/packages/dashboard/src/__tests__/register-signal-routes.test.ts b/packages/dashboard/src/__tests__/register-signal-routes.test.ts index 665e87a22d..679b9ab7e7 100644 --- a/packages/dashboard/src/__tests__/register-signal-routes.test.ts +++ b/packages/dashboard/src/__tests__/register-signal-routes.test.ts @@ -812,10 +812,22 @@ pgDescribe("ingestSignal — incident capture", () => { expect(res.status).toBe(201); expect(res.taskId).toBe("FN-1"); expect(store._tasks).toHaveLength(1); - expect(consoleSpy).toHaveBeenCalledWith( + /* + FNXC:DashboardTestMocks 2026-08-03-05:15 (red on main — same createLogger shape as the sse/diagnostics pair): + The bridge logs through core's `createLogger("signal-incident-bridge")`, which prefixes a severity marker and + folds the scope into the MESSAGE (`[signal-incident-bridge] Failed …`) rather than passing the scope + as a separate first argument. So the two-argument form this asserted no longer matches: the received call is + one marker-prefixed string plus the Error. + + Asserting a CONTAINS on the message and the Error argument keeps the case pinned to what matters — the + best-effort incident write failed loudly enough to diagnose while connector acceptance still returned 201 — + without re-coupling it to the logger's argument shape, which is what broke it. + */ + expect(consoleSpy).toHaveBeenCalledTimes(1); + expect(String(consoleSpy.mock.calls[0]?.[0] ?? "")).toContain( "[signal-incident-bridge] Failed to record connector signal", - expect.any(Error), ); + expect(consoleSpy.mock.calls[0]?.[1]).toBeInstanceOf(Error); consoleSpy.mockRestore(); }); diff --git a/packages/dashboard/src/__tests__/routes-tasks.test.ts b/packages/dashboard/src/__tests__/routes-tasks.test.ts index f06efec103..e6d852d1f6 100644 --- a/packages/dashboard/src/__tests__/routes-tasks.test.ts +++ b/packages/dashboard/src/__tests__/routes-tasks.test.ts @@ -2915,7 +2915,19 @@ describe("POST /tasks/:id/pr/address-feedback", () => { const res = await REQUEST(buildApp(), "POST", "/api/tasks/FN-001/pr/address-feedback", "{}", { "Content-Type": "application/json" }); expect(res.status).toBe(400); - expect(res.body.error).toContain("in-review or in-progress"); + /* + FNXC:WorkflowLifecycleColumns 2026-08-03-03:00 (red on main from my own #2723): + THE MESSAGE NOW NAMES THE BOARD'S RESOLVED COLUMNS. #2723 changed this 400 from + "PR feedback can only be addressed for in-review or in-progress tasks" to + "... for tasks in '' or ''" — because telling an operator their card must be `in-review` on a + board with no such column sends them looking for something that was deleted. + + The assertion checked the OLD prose. Asserting the resolved column NAMES instead of the sentence keeps the + case pinned to what matters (the refusal identifies the lanes the operator can actually use) and stops it + breaking again the next time the wording is improved. + */ + expect(res.body.error).toContain("in-review"); + expect(res.body.error).toContain("in-progress"); expect(store.addSteeringComment).not.toHaveBeenCalled(); expect(store.logEntry).not.toHaveBeenCalled(); }); diff --git a/packages/dashboard/src/__tests__/sse.test.ts b/packages/dashboard/src/__tests__/sse.test.ts index 0f2be5aaf8..2bb22ce932 100644 --- a/packages/dashboard/src/__tests__/sse.test.ts +++ b/packages/dashboard/src/__tests__/sse.test.ts @@ -436,26 +436,36 @@ describe("createSSE connection log severity", () => { it("does not console.log +/- connection when FUSION_DEBUG is unset", () => { delete process.env.FUSION_DEBUG; - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + /* + FNXC:DashboardTestMocks 2026-08-03-04:55 (red on main — and the sibling NEGATIVE case was passing falsely): + The +/- connection lines go through core's `createLogger("sse").debug(...)`, and that logger writes every + level except `warn` to **console.error** (stdio-transport safety). So a spy on `console.log` sees nothing — + which made the DEBUG case below fail loudly and made THIS quiet case pass for the wrong reason: an absence + assertion against a channel the code never writes to is satisfied no matter what the gate does. + + Both now spy on the channel the logger actually uses, so the quiet case proves the FUSION_DEBUG gate is + closed rather than proving the spy was pointed at the wrong stream. + */ const connection = openSseConnection("client-severity-quiet"); disconnectSSEClient("client-severity-quiet"); - const spam = logSpy.mock.calls + const spam = errorSpy.mock.calls .map((call) => String(call[0] ?? "")) .filter((line) => line.includes("[sse] + connection") || line.includes("[sse] - connection")); expect(spam).toEqual([]); - logSpy.mockRestore(); + errorSpy.mockRestore(); connection.req.emit("close"); }); it("emits +/- connection when FUSION_DEBUG=sse", () => { process.env.FUSION_DEBUG = "sse"; - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); openSseConnection("client-severity-debug"); disconnectSSEClient("client-severity-debug"); - const lines = logSpy.mock.calls.map((call) => String(call[0] ?? "")); + const lines = errorSpy.mock.calls.map((call) => String(call[0] ?? "")); expect(lines.some((line) => line.includes("[sse] + connection"))).toBe(true); expect(lines.some((line) => line.includes("[sse] - connection"))).toBe(true); - logSpy.mockRestore(); + errorSpy.mockRestore(); }); }); diff --git a/packages/dashboard/src/routes/__tests__/register-task-workflow-routes.resolved-review-column.test.ts b/packages/dashboard/src/routes/__tests__/register-task-workflow-routes.resolved-review-column.test.ts index e2da870ea3..139c386d46 100644 --- a/packages/dashboard/src/routes/__tests__/register-task-workflow-routes.resolved-review-column.test.ts +++ b/packages/dashboard/src/routes/__tests__/register-task-workflow-routes.resolved-review-column.test.ts @@ -344,12 +344,22 @@ describe("the review set agrees with core on WHICH merge lane", () => { expect(payload?.error ?? "").not.toContain("to recover branch binding"); }); - it("REFUSES a second mergeOrchestration column, because core does not call it the review lane", async () => { - // Over-inclusion here would have the dashboard move a card out of a lane the engine does not own. + it("ACCEPTS a second mergeOrchestration column, because core says every merge lane is review", async () => { + /* + FNXC:WorkflowLifecycleColumns 2026-08-02-22:55 (consolidation onto #2730 — this case INVERTED, deliberately): + This asserted a REFUSAL, from #2723's review round: I had narrowed the route resolver to core's then-single + `.review` choice because a superset let the dashboard act on a lane the engine did not own. + + #2730 answered that question authoritatively and in the other direction — core's `resolveReviewColumns` + returns EVERY merge-orchestration column — and this file now delegates to it. So the behaviour legitimately + changed and the assertion flips with it. + + Kept rather than deleted, because the invariant under test is unchanged and is the one that matters: THE + ROUTES AGREE WITH CORE. Deleting the case would have hidden that its answer moved; inverting it records + which decision moved and why. + */ const message = await refusalFor("second-signoff"); - expect(message).toContain("to recover branch binding"); - // And the message names the lane the board actually uses for review. - expect(message).toContain("signoff"); + expect(message).not.toContain("to recover branch binding"); }); }); diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index 1d67ed9477..bb334cc41f 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -55,6 +55,7 @@ import { isEphemeralAgent, parseExplicitDuplicateMarker, resolveWorkflowIrForTask, + resolveReviewColumns, workflowHasColumn, workflowPlansInColumn, workflowDeclaresColumnModel, @@ -262,45 +263,19 @@ this card ALREADY there" — membership. Every resolver used in a comparison aga the second kind. */ async function resolveReviewColumnsForTask(store: TaskStore, taskId: string): Promise> { + /* + FNXC:WorkflowLifecycleColumns 2026-08-02-22:20 (consolidation onto #2730's core resolver): + THE BODY IS NOW CORE'S. This resolver went through three shapes in three review rounds — a single id, then a + membership set over mergeBlocker/humanReview (#2713), then plus the FIRST mergeOrchestration column (#2723) — + and every round was an argument about arity at one call site. #2730 settled it in core, authoritatively and + for every surface, so the local body is deleted and only the store lookup and the legacy fallback remain. + + The WRAPPER stays: this file's callers hold a store and a task id, not an IR, which is the same reason its + sibling resolvers exist. One idiom per layer; one definition per question. + */ try { const ir = await resolveWorkflowIrForTask(store, taskId); - /* - FNXC:WorkflowLifecycleColumns 2026-08-02-05:30 (TWO DEFINITIONS OF "THE REVIEW LANE", one codebase): - `mergeOrchestration` IS INCLUDED, because core's `resolveLifecycleColumns().review` — the answer the - engine, the executor and `project-engine` all act on — resolves review from `mergeOrchestration`, while - this route resolver looked only at `mergeBlocker`/`humanReview`. - - The default lineage hides the difference: its `in-review` carries merge-blocker, human-review AND merge. - A board that declares only `merge` on its review lane — a perfectly ordinary custom board, and the shape - my renamed-board fixture uses — resolved as review in the ENGINE and as "not review" in these ROUTES. So - the executor would treat the card as in review while the dashboard's comment re-engagement, retry gate - and branch-binding recovery all refused it. - - Two layers answering one question differently is the same defect class as a literal, one level up: the - guard is converted, reads a real trait, and still disagrees with the authority. Unioning all three makes - this resolver a superset of core's, so the routes cannot refuse a card the engine considers in review. - */ - /* - FNXC:WorkflowLifecycleColumns 2026-08-02-07:20 (PR #2723 review — greptile P1, and the narrower answer - is the right one): - ONLY THE FIRST `mergeOrchestration` COLUMN, because that is the one core picks. `resolveLifecycleColumns` - resolves `.review` as `columnsWithFlag(ir, "mergeOrchestration")[0]`, so unioning ALL of them would have - swapped one disagreement with the engine for another: a board declaring the trait on two columns would - have the dashboard re-engage, retry and recover cards from a lane the engine does not treat as review — - an over-admission, and this route's re-engagement MOVES the card, so it is a state change rather than - mere permissiveness. - - The membership SET stays for `mergeBlocker`/`humanReview` (#2713's finding: those two can sit on - different columns and every caller here asks "is this card ALREADY in review"). The point of including - mergeOrchestration at all is to stop refusing a card the ENGINE considers in review; matching core's - choice of WHICH column achieves that without inventing a second definition. - */ - const [primaryMergeLane] = columnsWithFlag(ir, "mergeOrchestration"); - const lanes = [ - ...columnsWithFlag(ir, "mergeBlocker"), - ...columnsWithFlag(ir, "humanReview"), - ...(primaryMergeLane === undefined ? [] : [primaryMergeLane]), - ]; + const lanes = resolveReviewColumns(ir); return lanes.length > 0 ? new Set(lanes) : new Set(["in-review"]); } catch { return new Set(["in-review"]);