refactor: one definition of "which columns are review" — three copies deleted onto core's resolver (#2751)

**#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) <noreply@anthropic.com>
2026-07-30 08:28:32 -07:00
parent ea008b4064
commit 2e4905fa0e
14 changed files with 250 additions and 101 deletions
--- 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"\)/);
    }
  });
 });
--- 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(
--- 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<string>(
-        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(
--- 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=<level>\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();
    });
--- 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");
  });
 });
--- 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: "",
--- 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,
 }));

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

--- 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 () => ""),
--- 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 (`<marker>[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();
  });

--- 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 '<review>' or '<wip>'" — 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();
  });
--- 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();
  });
 });

--- 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");
  });
 });
--- 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<Set<string>> {
+  /*
+  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"]);