From 5791dfeeb73a128b35284e94873e8e8e17851bd0 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 05:25:28 -0700 Subject: [PATCH] =?UTF-8?q?fix(dashboard):=20the=20routes=20and=20the=20en?= =?UTF-8?q?gine=20disagreed=20about=20what=20"review"=20is=20=E2=80=94=20a?= =?UTF-8?q?=20converted=20guard=20that=20still=20contradicts=20core=20(#27?= =?UTF-8?q?23)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Built **on top of #2713**, which converted this file to trait-resolved membership sets while my #2701 was open on the same file. Their design is better than the single-id resolver I had — the arity argument in their comment (a single id answers "where should this card GO", a SET answers "is this card ALREADY there") is correct, and I have closed #2701 rather than contest it. Two things survive that #2713 did not cover. ## 1. The routes and the engine disagreed about what "review" IS Core's `resolveLifecycleColumns().review` — the answer the **executor**, the **merger** and **project-engine** all act on — resolves review from `mergeOrchestration`. This file's resolver looked only at `mergeBlocker` / `humanReview`. The default lineage hides it, because `in-review` carries all three: ``` in-review[merge-blocker, human-review, stall-detection, merge] ``` A board that declares only `merge` on its review lane resolved as **review in the engine** and **not review in the routes**. The executor treats the card as in review; comment re-engagement, the retry gate and branch-binding recovery all refuse it. **That is the same defect class as a literal, one level up:** the guard is converted, it reads a real trait, and it 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. How I found it is worth stating: my renamed-board fixture declares only `merge` on its review lane, and the ported suite failed against main. I nearly "fixed" the fixture to add `mergeBlocker` — which would have papered over a real cross-layer inconsistency to make my own test pass. ## 2. The 400s name the board's own columns #2713 converted both gates but left the messages saying `in-review` / `in-progress`. **Being told your card must be in a column that does not exist is worse than a wrong guard** — a wrong guard is a bug report; a wrong column name sends the operator looking for something that was deleted. ## Tests 7 cases ported from #2701 and adapted to the membership-set shape. **3 red on revert** of the `mergeOrchestration` union. ## A pre-existing failure I am reporting, not folding in `register-task-workflow-routes.move-bypassguards.test.ts` **fails on `origin/main`** — 400 where it expects 200. Verified by stashing my change and re-running, so it is not mine. Someone owns that regression; it should not ride in on a lane-consistency PR. ## Verification `pnpm test:gate` **10 / 71** · the ten other `register-task-workflow-routes` suites pass · 7/7 new · `tsc -p packages/dashboard` clean · `pnpm lint` clean · census `--strict` exit 0, no count change (this PR converts nothing new — it makes an already-converted resolver agree with core). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- ...flow-routes.resolved-review-column.test.ts | 355 ++++++++++++++++++ .../routes/register-task-workflow-routes.ts | 52 ++- 2 files changed, 404 insertions(+), 3 deletions(-) create mode 100644 packages/dashboard/src/routes/__tests__/register-task-workflow-routes.resolved-review-column.test.ts 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 new file mode 100644 index 0000000000..e2da870ea3 --- /dev/null +++ b/packages/dashboard/src/routes/__tests__/register-task-workflow-routes.resolved-review-column.test.ts @@ -0,0 +1,355 @@ +// @vitest-environment node +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-15:00 (fleet: register-task-workflow-routes.ts review lane): + +THE INVARIANT: the task routes decide "is this card in review?" from the task's OWN workflow. + +Ten route guards spelled it as `in-review`. Two of them are operator-visible in a way that is worse +than an inert guard, because the wrong answer is REPORTED as a fact about the operator's own board: + + - the user-comment re-engagement returns `suppressedReason: "not-in-review"` for a review card on a + renamed board, so a comment on a card plainly sitting in review is dropped and the API says the + card is not in review; + - the branch-binding recovery and PR-feedback routes reject with 400 messages naming `in-review` — + a column the operator's board does not have. Both messages now name the resolved columns. + +WHICH ROUTE THIS DRIVES, and why the assertions are on the MESSAGE rather than the status: the +branch-binding route's success path continues into the self-healing manager, which a partial store mock +cannot provide, so a passing card also ends in a 400 ("Self-healing manager unavailable"). Asserting +`status !== 400` would therefore be asserting nothing. The discriminator is WHICH 400 comes back — the +column refusal or the manager one — which is exactly the guard under test. My first version drove +`/address-pr-feedback`, a path that does not exist (it is `/pr/address-feedback`), and all five cases +hung for 15s each on the unmatched route: a test that never reached the code it named. + +THE ROUTE IS THE SURFACE, not the resolver. A unit test on `resolveReviewColumnForTask` would pass with +every call site still comparing against the literal, which is precisely the gap this file exists to +close. + +REVERT PROOF, measured below. The default-board cases pass either way by design — `builtin:coding`'s +review column IS `in-review` — and they are here to prove the conversion did not change the shipped +board's behaviour, not as evidence for it. +*/ +import { describe, it, expect, vi } from "vitest"; +import type { TaskStore } from "@fusion/core"; +import express from "express"; +import { createApiRoutes } from "../../routes.js"; +import { request as REQUEST } from "../../test-request.js"; + +/** Default lifecycle SHAPE, renamed ids — `signoff` carries the merge-orchestration traits. */ +const RENAMED_IR = { + version: "v2", + id: "wf-renamed", + name: "Renamed Flow", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }] }, + { id: "staging", name: "Staging", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "signoff", name: "Sign-off", traits: [{ trait: "merge" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }], + edges: [], +}; + +function buildStore(options: { taskColumn: string; workflowId?: string }) { + const task = { + id: "FN-001", + column: options.taskColumn, + dependencies: [], + steps: [], + currentStep: 0, + prInfo: { number: 7, url: "https://example.invalid/pr/7", state: "open" }, + }; + const recoverBranchBinding = vi.fn(async () => ({ recovered: true, branch: "fusion/FN-001" })); + + const store = { + getRootDir: vi.fn(() => process.cwd()), + getProjectScopedPluginMcpServers: vi.fn(async () => []), + getTask: vi.fn(async () => task), + getSettings: vi.fn(async () => ({})), + listTasks: vi.fn(async () => [task]), + getTaskWorkflowSelection: vi.fn(() => (options.workflowId ? { workflowId: options.workflowId } : undefined)), + getTaskWorkflowSelectionAsync: vi.fn(async () => (options.workflowId ? { workflowId: options.workflowId } : undefined)), + getWorkflowDefinition: vi.fn(async (id: string) => + id === "wf-renamed" ? { id, name: "Renamed Flow", kind: "workflow", ir: RENAMED_IR } : null, + ), + updateTask: vi.fn(async () => task), + logEntry: vi.fn(async () => undefined), + recordRunAuditEvent: vi.fn(async () => undefined), + recoverBranchBinding, + } as unknown as TaskStore; + + return { store, recoverBranchBinding }; +} + +async function post(store: TaskStore, path: string, body: unknown = {}) { + const app = express(); + app.use(express.json()); + app.use("/api", createApiRoutes(store)); + return REQUEST(app, "POST", `/api/tasks/FN-001${path}`, JSON.stringify(body), { + "content-type": "application/json", + }); +} + +describe("the task routes resolve the board's own review column", () => { + const COLUMN_REFUSAL = "to recover branch binding"; + + async function refusal(store: TaskStore): Promise { + const res = await post(store, "/recover-branch-binding"); + const payload = res.body as { error?: string } | undefined; + return payload?.error ?? JSON.stringify(res.body ?? {}); + } + + it("does not refuse a RENAMED board's review card as being in the wrong column", async () => { + // Pre-fix: `signoff` !== "in-review", so the route rejected with a message naming a column this + // board does not have. The card was in review the whole time. + const { store } = buildStore({ taskColumn: "signoff", workflowId: "wf-renamed" }); + + expect(await refusal(store)).not.toContain(COLUMN_REFUSAL); + }); + + it("still refuses a card outside the review lane, naming the board's OWN column", async () => { + // The paired negative, plus the message fix: an operator must be told `signoff`, not `in-review`. + const { store } = buildStore({ taskColumn: "building", workflowId: "wf-renamed" }); + + const message = await refusal(store); + expect(message).toContain(COLUMN_REFUSAL); + expect(message).toContain("signoff"); + expect(message).not.toContain("in-review"); + }); + + it("behaves identically on the DEFAULT board, where the literal was already correct", async () => { + // No workflow selection: the resolver falls back to `in-review`. Passes either way by design. + const { store } = buildStore({ taskColumn: "in-review" }); + + expect(await refusal(store)).not.toContain(COLUMN_REFUSAL); + }); + + it("still refuses an intake-lane card on the DEFAULT board", async () => { + const { store } = buildStore({ taskColumn: "todo" }); + + expect(await refusal(store)).toContain(COLUMN_REFUSAL); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-08-01-21:40 (PR #2701 review — greptile P1): + +THE INVARIANT: an open PR blocks the in-review user-comment re-engagement on ANY board. + +THE THIRD FORM OF THIS PROGRAM'S DEFECT, and the most interesting one. The guard was deliberately left +on the legacy `COLUMNS.indexOf` enum, with a comment justifying it: "this whole function is gated on the +literal `task.column !== "in-review"` above and re-engages to the literal `"in-progress"`, so both +endpoints are legacy ids by construction". That was TRUE when written. + +Then U5 converted the re-engage target to `resolveWipColumnForTask`, and this PR converted the entry gate +to the resolved review lane. Both halves of the premise are now false, so on a renamed board the enum +scores -1 for both endpoints — and `isBackwardMoveBlockedByOpenPr` treats a negative index as "cannot +tell -> allow". A user comment on a review card with an OPEN PR resumed execution behind that PR. + +Nobody edited the guard or the comment. The comment's correctness argument depended on literals +elsewhere, and someone else converted them. When you convert a lane, grep for comments justifying a +nearby legacy path by "the surrounding literals" — they are load-bearing. + +REVERT PROOF, measured: restore `COLUMNS.indexOf` and the renamed-board case fails — the task is moved +to the wip lane despite the open PR. +*/ +describe("the re-engage open-PR guard survives a renamed board", () => { + function buildReengageStore(column: string, workflowId: string | undefined, prState: string | null) { + const task = { + id: "FN-002", column, dependencies: [], steps: [], currentStep: 0, sessionFile: null, + }; + const moveTask = vi.fn(async (_id: string, to: string) => ({ ...task, column: to })); + const logEntry = vi.fn(async () => undefined); + + const store = { + getRootDir: vi.fn(() => process.cwd()), + getProjectScopedPluginMcpServers: vi.fn(async () => []), + getTask: vi.fn(async () => task), + getSettings: vi.fn(async () => ({})), + getTaskWorkflowSelection: vi.fn(() => (workflowId ? { workflowId } : undefined)), + getTaskWorkflowSelectionAsync: vi.fn(async () => (workflowId ? { workflowId } : undefined)), + getWorkflowDefinition: vi.fn(async (id: string) => + id === "wf-renamed" ? { id, name: "Renamed Flow", kind: "workflow", ir: RENAMED_IR } : null, + ), + getActivePrEntityBySource: vi.fn(async () => + prState ? { id: "PR-1", state: prState, sourceType: "task", sourceId: "FN-002" } : null, + ), + /* The route calls `addTaskComment` and re-engages the TASK IT RETURNS, so the mock must return the + task row — returning a bare comment made the whole block vacuous: the route threw before reaching + the guard, `moveTask` was never called, and both negative cases "passed" for the wrong reason. + Caught it because the paired POSITIVE case failed; without that case this suite would have looked + green and proven nothing. */ + addTaskComment: vi.fn(async () => ({ ...task, comments: [{ id: "c1", text: "please fix", author: "user" }] })), + updateTask: vi.fn(async () => task), + updateStep: vi.fn(async () => undefined), + logEntry, + moveTask, + recordRunAuditEvent: vi.fn(async () => undefined), + } as unknown as TaskStore; + + return { store, moveTask, logEntry }; + } + + async function comment(store: TaskStore) { + const app = express(); + app.use(express.json()); + app.use("/api", createApiRoutes(store)); + return REQUEST(app, "POST", "/api/tasks/FN-002/comments", JSON.stringify({ text: "please fix", author: "user" }), { + "content-type": "application/json", + }); + } + + it("does NOT re-engage a renamed board's review card while a PR is open", async () => { + // Pre-fix: both endpoints scored -1 on the legacy enum, a negative index means "allow", and the card + // was moved into the wip lane behind an open PR. + const { store, moveTask } = buildReengageStore("signoff", "wf-renamed", "open"); + + await comment(store); + + expect(moveTask).not.toHaveBeenCalled(); + }); + + it("DOES re-engage a renamed board's review card once no PR is active", async () => { + // The paired positive: the guard must not block re-engagement outright. + const { store, moveTask } = buildReengageStore("signoff", "wf-renamed", null); + + await comment(store); + + expect(moveTask).toHaveBeenCalledTimes(1); + expect(moveTask.mock.calls[0]?.[1]).toBe("building"); + }); + + it("still blocks on the DEFAULT board with an open PR", async () => { + // The case the legacy enum already handled; it must keep working. + const { store, moveTask } = buildReengageStore("in-review", undefined, "open"); + + await comment(store); + + expect(moveTask).not.toHaveBeenCalled(); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-08-02-07:35 (PR #2723 review — greptile P1): + +THE ROUTES MUST AGREE WITH CORE ON *WHICH* MERGE LANE, not just on the trait. + +My first version unioned every `mergeOrchestration` column into the review set. That fixed the +under-inclusion (a board declaring only `merge` was in review for the engine and not for the routes) and +introduced over-inclusion: `resolveLifecycleColumns().review` is `columnsWithFlag(ir, "mergeOrchestration")[0]`, +so on a board declaring the trait TWICE the dashboard would have re-engaged, retried and recovered cards +from a lane the engine does not treat as review. + +That direction is worse than it sounds. Re-engagement MOVES the card out of review into the wip lane — a +state change the engine would never make for that column — so an over-inclusive read here is not +permissiveness, it is the dashboard acting on a lane it does not own. + +The `mergeBlocker`/`humanReview` membership set stays (#2713's finding: those can sit on different columns). +Only the mergeOrchestration axis is narrowed to core's choice. +*/ +describe("the review set agrees with core on WHICH merge lane", () => { + const TWO_MERGE_LANES = { + version: "v2", + id: "wf-two-merge", + name: "Two Merge Lanes", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "signoff", name: "Sign-off", traits: [{ trait: "merge" }] }, + { id: "second-signoff", name: "Second Sign-off", traits: [{ trait: "merge" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }], + edges: [], + } as unknown as WorkflowIr; + + function storeFor(column: string) { + const task = { id: "FN-003", column, dependencies: [], steps: [], currentStep: 0 }; + return { + getRootDir: vi.fn(() => process.cwd()), + getProjectScopedPluginMcpServers: vi.fn(async () => []), + getTask: vi.fn(async () => task), + getSettings: vi.fn(async () => ({})), + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "wf-two-merge" })), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "wf-two-merge" })), + getWorkflowDefinition: vi.fn(async () => ({ id: "wf-two-merge", name: "Two", kind: "workflow", ir: TWO_MERGE_LANES })), + logEntry: vi.fn(async () => undefined), + updateTask: vi.fn(async () => task), + } as unknown as TaskStore; + } + + async function refusalFor(column: string): Promise { + const app = express(); + app.use(express.json()); + app.use("/api", createApiRoutes(storeFor(column))); + const res = await REQUEST(app, "POST", "/api/tasks/FN-003/recover-branch-binding", "{}", { + "content-type": "application/json", + }); + const payload = res.body as { error?: string } | undefined; + return payload?.error ?? JSON.stringify(res.body ?? {}); + } + + it("accepts the FIRST mergeOrchestration column — the one core resolves", async () => { + expect(await refusalFor("signoff")).not.toContain("to recover branch binding"); + }); + + it("STILL admits a second merge lane when it ALSO carries humanReview — traits only ever ADD", async () => { + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-04:05 (PR #2723 review — greptile; I judge the finding NOT a + defect, and pin the behaviour so the question is settled rather than re-litigated). + + The finding asks to exclude a secondary `mergeOrchestration` column that ALSO carries `humanReview`, + because core resolves only the first merge lane. That would make the predicate NON-MONOTONIC: adding + the merge trait to a human-review lane would REMOVE it from the review set, so a card plainly sitting + in a human-review column would stop counting as in review because its column gained an unrelated + capability. + + The two traits answer different questions. `humanReview` says "a person reviews cards here" — plural + by nature, which is why #2713 made it a set. `mergeOrchestration` says "the merge gate lives here", + singular by core's construction. A column may be both, and being both must not be worse than being + either. + + The real divergence is upstream: core's `.review` is a single id derived from ONE flag, so every + consumer re-derives its own answer. Flagged there rather than papered over per file. + */ + const task = { id: "FN-004", column: "second-signoff", dependencies: [], steps: [], currentStep: 0 }; + const dualIr = { + ...(TWO_MERGE_LANES as unknown as { columns: Array> }), + columns: (TWO_MERGE_LANES as unknown as { columns: Array> }).columns.map((c) => + c.id === "second-signoff" ? { ...c, traits: [{ trait: "merge" }, { trait: "human-review" }] } : c, + ), + } as unknown as WorkflowIr; + + const store = { + getRootDir: vi.fn(() => process.cwd()), + getProjectScopedPluginMcpServers: vi.fn(async () => []), + getTask: vi.fn(async () => task), + getSettings: vi.fn(async () => ({})), + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "wf-two-merge" })), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "wf-two-merge" })), + getWorkflowDefinition: vi.fn(async () => ({ id: "wf-two-merge", name: "Two", kind: "workflow", ir: dualIr })), + logEntry: vi.fn(async () => undefined), + updateTask: vi.fn(async () => task), + } as unknown as TaskStore; + + const app = express(); + app.use(express.json()); + app.use("/api", createApiRoutes(store)); + const res = await REQUEST(app, "POST", "/api/tasks/FN-004/recover-branch-binding", "{}", { + "content-type": "application/json", + }); + const payload = res.body as { error?: string } | undefined; + + 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. + 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"); + }); +}); diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index d9cb4f7f6f..76d88512b7 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -264,7 +264,43 @@ the second kind. async function resolveReviewColumnsForTask(store: TaskStore, taskId: string): Promise> { try { const ir = await resolveWorkflowIrForTask(store, taskId); - const lanes = [...columnsWithFlag(ir, "mergeBlocker"), ...columnsWithFlag(ir, "humanReview")]; + /* + 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]), + ]; return lanes.length > 0 ? new Set(lanes) : new Set(["in-review"]); } catch { return new Set(["in-review"]); @@ -3873,7 +3909,15 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork } const prStatusReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); if (!prStatusReviewColumns.has(task.column)) { - throw badRequest("Task must be in 'in-review' column to recover branch binding"); + /* + FNXC:WorkflowLifecycleColumns 2026-08-02-05:10 (the operator-facing half of #2713's conversion): + THE MESSAGE NAMES THE BOARD'S OWN COLUMNS. The gate resolves review by trait, but the 400 still + said `in-review` — a column the operator's board may not have. Being told your card must be in a + column that does not exist is worse than a wrong guard: a wrong guard is a bug report, a wrong + column name sends the operator looking for something that was deleted. + */ + const expected = [...prStatusReviewColumns].map((column) => `'${column}'`).join(" or "); + throw badRequest(`Task must be in ${expected} to recover branch binding`); } const selfHealingManager = _resolveSelfHealingManager(scopedStore); @@ -5918,7 +5962,9 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork const prFeedbackReviewColumns = await resolveReviewColumnsForTask(scopedStore, task.id); const prFeedbackWipColumn = await resolveWipColumnForTask(scopedStore, task.id); if (!prFeedbackReviewColumns.has(task.column) && task.column !== prFeedbackWipColumn) { - throw badRequest("PR feedback can only be addressed for in-review or in-progress tasks"); + /* FNXC:WorkflowLifecycleColumns 2026-08-02-05:12: same fix — the operator reads their own columns. */ + const allowed = [...prFeedbackReviewColumns, prFeedbackWipColumn].map((column) => `'${column}'`).join(" or "); + throw badRequest(`PR feedback can only be addressed for tasks in ${allowed}`); } /*