diff --git a/.changeset/pr-review-changes-requested-renamed-lane.md b/.changeset/pr-review-changes-requested-renamed-lane.md new file mode 100644 index 0000000000..f65e07d13a --- /dev/null +++ b/.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. diff --git a/packages/engine/src/__tests__/pr-comment-handler.test.ts b/packages/engine/src/__tests__/pr-comment-handler.test.ts index 53a31361da..f93c2948b7 100644 --- a/packages/engine/src/__tests__/pr-comment-handler.test.ts +++ b/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).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).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).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", () => { diff --git a/packages/engine/src/pr-comment-handler.ts b/packages/engine/src/pr-comment-handler.ts index 8bcee0f062..b76cb83b80 100644 --- a/packages/engine/src/pr-comment-handler.ts +++ b/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 { 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(["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`, diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 7737bc9948..803eb6fd22 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/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 },