fix(dashboard): the routes and the engine disagreed about what "review" is — a converted guard that still contradicts core (#2723)
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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
@@ -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<string> {
|
||||
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<string> {
|
||||
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<Record<string, unknown>> }),
|
||||
columns: (TWO_MERGE_LANES as unknown as { columns: Array<Record<string, unknown>> }).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");
|
||||
});
|
||||
});
|
||||
@@ -264,7 +264,43 @@ the second kind.
|
||||
async function resolveReviewColumnsForTask(store: TaskStore, taskId: string): Promise<Set<string>> {
|
||||
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}`);
|
||||
}
|
||||
|
||||
/*
|
||||
|
||||
Reference in New Issue
Block a user