engine: a GitHub "changes requested" review was silently dropped on a renamed board (1 → 0) (#2807)

A human reviewer's feedback was being thrown away.

`PrCommentHandler.handleChangesRequested` gated on `task.column !==
"in-review"` and returned early. On any board whose review lane is
renamed, a GitHub **"changes requested"** review produced **no steering
comment** and the card **never went back to work** — the feedback
vanished behind a log line nobody reads. No error, no audit row.

## Census

| file | main | here |
| --- | ---: | ---: |
| `packages/engine/src/pr-comment-handler.ts` | 1 | **0** |

## Two literals, only one countable — again

```ts
if (task.column !== "in-review") { … return; }   // counted
…
await this.store.moveTask(taskId, "in-progress"); // INVISIBLE to the census
```

The census scores comparisons. The requeue **destination** is a call
argument, so nothing in the backlog pointed at it — the same pairing as
the branch-worktree auto-requeue in #2797, and the same trap: converting
the gate alone would make the handler *admit* the review and then
attempt a move into a lane the board may not declare, which `moveTask`
rejects. A half-conversion here turns a silent drop into a thrown
rejection. They convert together or not at all.

That is now the second confirmed instance of this shape. The pattern to
look for is a **counted guard whose body performs a hardcoded
`moveTask`** — the guard is the visible half and the move is the
dangerous one.

## Revert results (measured, each run independently)

| conversion | reverted → |
| --- | --- |
| review-lane gate | RENAMED case fails — `updateTask`/`moveTask` never
called; the review is dropped |
| requeue destination | RENAMED case fails — `moveTask` called with
`"in-progress"` instead of the board's wip lane |

The legacy case passes both ways, which is why both vocabularies run. A
non-vacuous companion (renamed board, card sitting in the hold lane)
keeps a gate that admits everything from passing.

## Verification

- `pnpm test:gate` — 161 + 487 + 13 + 71, green
- `pr-comment-handler.test.ts` — 34 passed
- `npx tsc -p packages/engine/tsconfig.json --noEmit` — clean
- `pnpm lint` — clean
- `node scripts/lifecycle-column-census.mjs --strict` — exit 0

(Running the census explicitly, not just `pnpm lint`: CI's Lint job runs
both, and a clean local `pnpm lint` is **not** evidence the Lint check
passes — that cost a round-trip on #2797.)

.changeset/pr-review-changes-requested-renamed-lane.md

@@ -0,0 +1,7 @@
+---
+"@runfusion/fusion": patch
+---
+
+summary: A GitHub "changes requested" review is no longer dropped on boards with renamed columns.
+category: fix
+dev: `PrCommentHandler.handleChangesRequested` gated on `task.column !== "in-review"` and requeued to a hardcoded `"in-progress"`. Both now resolve from the task's workflow — the review lane via the `mergeOrchestration` role, the requeue target via the first `countsTowardWip` column — each falling back to the legacy id.

packages/engine/src/__tests__/pr-comment-handler.test.ts

@@ -1,5 +1,6 @@
 import { describe, it, expect, vi, beforeEach } from "vitest";
 import { PrCommentHandler } from "../pr-comment-handler.js";
+import { RENAMED_VOCAB, lifecycleIr } from "./_workflow-vocabulary-fixture.js";
 import type { TaskStore, Task } from "@fusion/core";
 
 const mockStore = {
@@ -237,6 +238,111 @@ describe("PrCommentHandler", () => {
       );
       expect(mockStore.moveTask).toHaveBeenCalledWith("FN-001", "in-progress");
     });
+
+    /*
+    FNXC:WorkflowResolvedColumns 2026-07-30-18:30 (engine):
+    DIFFERENTIAL over the column vocabulary. The case above asserts the LEGACY ids, which is what both
+    literals on this path compared against — it passed before this conversion and would pass for a broken
+    one, because the fake supplies no workflow and the resolver degrades to the built-in board.
+
+    The failure is silent and human-facing: on a renamed review lane the gate returned early, so a
+    reviewer's "changes requested" produced NO steering comment and the card never went back to work.
+    The feedback simply vanished behind a log line.
+
+    Both literals are covered, deliberately. The destination (`moveTask(taskId, "in-progress")`) is a
+    call argument the census cannot see; converting only the gate would admit the review and then attempt
+    a move into a lane the board may not declare.
+
+    REVERT CHECK, measured (each independently):
+      - gate restored to `task.column !== "in-review"` -> fails; updateTask/moveTask never called.
+      - destination restored to the literal            -> fails; moveTask called with "in-progress"
+        instead of the renamed wip lane.
+    */
+    it("handles a changes-requested review on a RENAMED board and requeues to its OWN wip lane", async () => {
+      const ir = lifecycleIr(RENAMED_VOCAB, "pr-comment-lifecycle", { mergeOrchestration: true });
+      (mockStore.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(
+        { id: "FN-001", column: RENAMED_VOCAB.review, review: undefined } as Task,
+      );
+      Object.assign(mockStore, {
+        getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "pr-comment-lifecycle", stepIds: [] })),
+        getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "pr-comment-lifecycle", stepIds: [] })),
+        getWorkflowDefinition: vi.fn(async (id: string) => (id === "pr-comment-lifecycle" ? { ir } : undefined)),
+      });
+
+      await handler.handleChangesRequested("FN-001", mockPrInfo, "reviewer", "Please add tests");
+
+      // The reviewer's feedback is recorded rather than dropped...
+      expect(mockStore.updateTask).toHaveBeenCalledWith(
+        "FN-001",
+        expect.objectContaining({
+          reviewState: expect.objectContaining({
+            items: expect.arrayContaining([
+              expect.objectContaining({ source: "github-pr", body: "Please add tests" }),
+            ]),
+          }),
+        }),
+      );
+      // ...and the card returns to THIS board's wip lane, not the legacy id.
+      expect(mockStore.moveTask).toHaveBeenCalledWith("FN-001", RENAMED_VOCAB.wip);
+      expect(mockStore.moveTask).not.toHaveBeenCalledWith("FN-001", "in-progress");
+    });
+
+    /*
+    FNXC:WorkflowResolvedColumns 2026-07-30-18:55 (#2807 review — greptile P1):
+    PINS THE NARROWING. The first version admitted any column carrying mergeOrchestration OR
+    mergeBlocker OR humanReview. On a workflow that puts those on SEPARATE columns that is a widening,
+    not a conversion: a card parked in a human-approval lane gets bounced into wip by a PR review that
+    has nothing to do with it.
+
+    This board declares a human-review/merge-blocker column that is NOT the merge-orchestration lane —
+    the exact shape the union got wrong, and one the default board cannot express.
+
+    REVERT CHECK, measured: restoring the three-flag union makes this fail — the handler admits the
+    review and moves the card out of a lane it was never responsible for.
+    */
+    it("does not admit a PR review from a human-approval lane that is not the merge stage", async () => {
+      const base = lifecycleIr(RENAMED_VOCAB, "pr-comment-lifecycle", { mergeOrchestration: true });
+      const ir = {
+        ...base,
+        columns: [
+          ...base.columns,
+          { id: "approval", name: "Approval", traits: [{ trait: "human-review" as const }, { trait: "merge-blocker" as const }] },
+        ],
+      };
+      (mockStore.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(
+        { id: "FN-001", column: "approval", review: undefined } as Task,
+      );
+      Object.assign(mockStore, {
+        getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "pr-comment-lifecycle", stepIds: [] })),
+        getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "pr-comment-lifecycle", stepIds: [] })),
+        getWorkflowDefinition: vi.fn(async (id: string) => (id === "pr-comment-lifecycle" ? { ir } : undefined)),
+      });
+
+      await handler.handleChangesRequested("FN-001", mockPrInfo, "reviewer", "Please add tests");
+
+      expect(mockStore.moveTask).not.toHaveBeenCalled();
+      expect(mockStore.updateTask).not.toHaveBeenCalled();
+    });
+
+    it("still ignores a review on a RENAMED board when the card is not in a review lane", async () => {
+      /*
+      Non-vacuous companion: without it, a gate admitting every column would satisfy the case above.
+      Same renamed board, same review — only the card's lane changes.
+      */
+      const ir = lifecycleIr(RENAMED_VOCAB, "pr-comment-lifecycle", { mergeOrchestration: true });
+      (mockStore.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(
+        { id: "FN-001", column: RENAMED_VOCAB.hold, review: undefined } as Task,
+      );
+      Object.assign(mockStore, {
+        getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "pr-comment-lifecycle", stepIds: [] })),
+        getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "pr-comment-lifecycle", stepIds: [] })),
+        getWorkflowDefinition: vi.fn(async (id: string) => (id === "pr-comment-lifecycle" ? { ir } : undefined)),
+      });
+
+      await handler.handleChangesRequested("FN-001", mockPrInfo, "reviewer", "Please add tests");
+
+      expect(mockStore.moveTask).not.toHaveBeenCalled();
+    });
   });
 
   describe("createFollowUpTask", () => {

packages/engine/src/pr-comment-handler.ts

@@ -2,6 +2,7 @@ import type { TaskStore } from "@fusion/core";
 import type { PrInfo } from "@fusion/core";
 import { prMonitorLog } from "./logger.js";
 import { resolveTerminalColumnsFor } from "./executor.js";
+import { resolveWorkflowIrForTask, columnsWithFlag } from "@fusion/core";
 
 /*
 FNXC:PullRequestReview 2026-07-26-00:00:
@@ -180,8 +181,52 @@ export class PrCommentHandler {
   ): Promise<void> {
     try {
       const task = await this.store.getTask(taskId);
-      if (task.column !== "in-review") {
-        prMonitorLog.log(`Task ${taskId} not in-review (${task.column}), skipping changes-requested handling`);
+      /*
+      FNXC:WorkflowResolvedColumns 2026-07-30-18:20 (engine):
+      TWO lifecycle literals on this path, and only ONE of them is countable.
+
+      The GATE (`!== "in-review"`) silently dropped a GitHub "changes requested" review on any board
+      whose review lane is renamed: no steering comment was recorded and the card never went back to
+      work, so a human reviewer's feedback vanished with a log line nobody reads. That is the counted one.
+
+      The DESTINATION below (`moveTask(taskId, "in-progress")`) is a call argument, so the census cannot
+      see it — the same pairing as the branch-worktree auto-requeue (#2797). Converting the gate alone
+      would let the handler admit the review and then attempt a move into a lane the board may not
+      declare, which `moveTask` rejects. They convert together or not at all.
+
+      Both fall back to the legacy ids: `resolveWorkflowIrForTask` degrades to the BUILT-IN IR rather
+      than throwing, so a board whose workflow cannot be read behaves exactly as before.
+      */
+      /*
+      FNXC:WorkflowResolvedColumns 2026-07-30-18:55 (#2807 review — greptile P1 "broad review-lane
+      admission"):
+      MERGE-ORCHESTRATION ONLY, not the three-flag review union. The first version admitted any column
+      carrying `mergeOrchestration` OR `mergeBlocker` OR `humanReview`; on a workflow that puts those on
+      SEPARATE columns that is a widening, not a conversion — a card parked in a plan-review or
+      human-approval lane would be bounced into wip by a PR review that has nothing to do with it.
+
+      The literal this replaced admitted exactly ONE lane, and the faithful resolution is the one role
+      that means "this is the PR/merge stage": `mergeOrchestration`. That is also what
+      `resolveLifecycleColumns` keys its `review` role on, so this handler and the core resolver agree.
+
+      Known consequence, and it is the pre-existing behaviour rather than a regression: a custom workflow
+      whose review lane carries ONLY `human-review` resolves nothing here and falls back to the legacy
+      `in-review`. Widening to cover it is the gap recorded in
+      `notification-renamed-lifecycle-columns.test.ts`, and it belongs in the shared resolver rather than
+      being invented per-handler.
+      */
+      const reviewLanes = new Set<string>(["in-review"]);
+      let wipTarget = "in-progress";
+      try {
+        const ir = await resolveWorkflowIrForTask(this.store, taskId);
+        if (ir) {
+          for (const id of columnsWithFlag(ir, "mergeOrchestration")) reviewLanes.add(id);
+          const wipLanes = columnsWithFlag(ir, "countsTowardWip");
+          if (wipLanes.length > 0) wipTarget = wipLanes[0];
+        }
+      } catch { /* degraded: legacy ids */ }
+      if (!reviewLanes.has(task.column)) {
+        prMonitorLog.log(`Task ${taskId} not in a review lane (${task.column}), skipping changes-requested handling`);
         return;
       }
 
@@ -208,7 +253,7 @@ export class PrCommentHandler {
         },
         "queued",
       );
-      await this.store.moveTask(taskId, "in-progress");
+      await this.store.moveTask(taskId, wipTarget);
       await this.store.logEntry(
         taskId,
         `PR #${prInfo.number}: changes requested by @${reviewerLogin} — moved back to in-progress`,

scripts/lib/lifecycle-column-census-baseline.json

@@ -84,7 +84,6 @@
     "packages/engine/src/backlog-pressure-reporter.ts": 1,
     "packages/engine/src/ephemeral-worker-manager.ts": 1,
     "packages/engine/src/merger.ts": 1,
-    "packages/engine/src/pr-comment-handler.ts": 1,
     "packages/engine/src/runtimes/in-process-runtime.ts": 1,
     "packages/engine/src/triage.ts": 1
   },