diff --git a/packages/cli/src/__tests__/pr-create-review-lane-resolved.test.ts b/packages/cli/src/__tests__/pr-create-review-lane-resolved.test.ts index 52785c8aff..bf245695d9 100644 --- a/packages/cli/src/__tests__/pr-create-review-lane-resolved.test.ts +++ b/packages/cli/src/__tests__/pr-create-review-lane-resolved.test.ts @@ -160,12 +160,40 @@ describe("fn pr create resolves the board's own review lane", () => { expect(refusal).not.toContain("in-review"); }); - it("refuses WITHOUT naming a phantom lane when the workflow declares no review lane", async () => { + it("takes the legacy fallback when the resolved workflow declares no review lane", async () => { /* - #2775 review (greptile P2). My first pass fell back to `'in-review'` for BOTH the unresolvable - workflow and the resolved-but-empty case, so the refusal named a column this board does not have - — the very defect this change exists to fix, reintroduced one branch over. A resolved workflow - with no review-trait column is an ANSWER; only an unreadable workflow is a missing one. + FNXC:WorkflowLifecycleColumns 2026-07-31-03:10: + THIS TEST ASSERTED A DECISION THAT WAS SUPERSEDED BEFORE #2775 LANDED, and it went red on main + the moment it did. + + Two review rounds on #2775 pushed `pr.ts` in opposite directions and the SECOND one won: + + round 1 (greptile P2) — a resolved workflow with no review-trait column is an ANSWER; do not + invent `'in-review'`, say "no review lane". That is what this test + was written against. + round 2 (greptile) — refusing on an empty set rejects EVERY v1 workflow, because + `synthesizeDefaultColumns` upgrades a v1 graph by emitting every + column with `traits: []`. So a v1 board whose `in-review` column + plainly exists resolves to an empty review set. + + Round 2 is decisive and is what shipped: an empty set is indistinguishable from a v1 upgrade, so + it means UNEXPRESSED rather than absent and takes the same legacy fallback as an unreadable + workflow. `pr.ts:206-207` implements exactly that. The round-1 assertion could not pass against + it — there is no "no review lane" message in the shipped code at all, so `errors.find(...)` + returned undefined. + + round 3 (greptile, #2801) — round 2 over-corrected. Falling back for EVERY empty set means a + native v2 board that expresses traits and declares no review lane is + told to move its card to `'in-review'` — a column it does not have. + An impossible instruction is the defect this conversion set out to + remove, reappearing inside its own fallback. + + Round 2's note claimed the distinction was "NOT recoverable ... which the IR does not currently + carry". That was wrong, and this test asserted the wrong contract because of it: the IR DOES carry + it. A v1 upgrade emits every column with `traits: []`, so NO column expresses any trait; a native + v2 board that declares traits elsewhere expresses some. `workflowExpressesAnyTrait` in `pr.ts` + separates them, and the three states now get three answers — legacy fallback for unresolvable and + for v1, an explicit refusal naming no lane for a v2 board with no review column. */ const noReviewIr = { ...RENAMED_IR, @@ -176,9 +204,32 @@ describe("fn pr create resolves the board's own review lane", () => { await expect(runPrCreate("FN-001", { ai: false })).rejects.toThrow("process.exit:1"); expect(store.updatePrInfo).not.toHaveBeenCalled(); - const refusal = errors.find((e) => e.includes("no review lane")); + /* + A v2 board that expresses traits and has no review lane is told exactly that — not sent to a + column it does not declare. Asserting the ABSENCE of `'in-review'` is the point of the case. + */ + const refusal = errors.find((e) => e.includes("no review column")); expect(refusal).toBeDefined(); - expect(refusal).not.toContain("in-review"); + expect(refusal).not.toContain("'in-review'"); + expect(refusal).not.toContain("signoff"); + }); + + it("keeps the legacy fallback for a V1-UPGRADED board, whose columns express no traits at all", async () => { + /* + The other side of round 3, and the reason the predicate is trait-EXPRESSION rather than + review-set-emptiness: both boards resolve an empty review set, and only one of them should refuse. + `synthesizeDefaultColumns` emits every default column with `traits: []`, so `in-review` plainly + exists and holds this board's cards. + */ + const v1Ir = { + ...RENAMED_IR, + columns: ["todo", "in-progress", "in-review", "done", "archived"].map((id) => ({ id, name: id, traits: [] })), + }; + const store = mockBoard("in-review", v1Ir); + + await runPrCreate("FN-001", { ai: false }); + + expect(store.updatePrInfo).toHaveBeenCalledTimes(1); }); it("keeps the legacy literal when the workflow cannot be resolved", async () => { diff --git a/packages/cli/src/commands/pr.ts b/packages/cli/src/commands/pr.ts index 2a49453d07..d7331863dc 100644 --- a/packages/cli/src/commands/pr.ts +++ b/packages/cli/src/commands/pr.ts @@ -121,6 +121,21 @@ export interface PrCreateOptions { reviewers?: string[]; } +/* +FNXC:WorkflowResolvedColumns 2026-07-30-14:15 (#2801 review): +Does this workflow express ANY lifecycle trait? Separates a v1 upgrade (every column emitted with +`traits: []` by `synthesizeDefaultColumns`, so EVERY role resolves empty) from a native v2 board that +declares traits and simply has no review lane. The empty review set alone cannot tell them apart. + +Local rather than imported: the shared `declaresAnyLifecycleTrait` lands in `@fusion/core` on the +batch-core branch and is not on `main` yet, and this fix should not wait on that merge. Collapse this +into the core helper once it is available — the two are deliberately the same predicate. +*/ +function workflowExpressesAnyTrait(ir: Parameters[0]): boolean { + const columns = (ir as unknown as { columns?: Array<{ traits?: unknown[] }> }).columns ?? []; + return columns.some((column) => Array.isArray(column.traits) && column.traits.length > 0); +} + export async function runPrCreate(id: string, options: PrCreateOptions = {}, projectName?: string) { let context: ProjectContext | undefined; try { @@ -204,7 +219,32 @@ export async function runPrCreate(id: string, options: PrCreateOptions = {}, pro The general "an empty resolved set is an answer" rule still holds elsewhere; it fails here only because the v1 upgrade path manufactures empty traits for columns that do exist. */ + /* + FNXC:WorkflowResolvedColumns 2026-07-30-14:10 (#2801 review — greptile): + THREE STATES, NOT TWO. This collapsed the last two and named a lane that cannot exist. + + unresolvable -> legacy id. We know nothing; keep today's behaviour. + resolved, NO traits -> legacy id. A v1 upgrade: `synthesizeDefaultColumns` emits every + default column with `traits: []`, so `in-review` plainly exists and + holds the cards. Refusing here would break every pre-v2 project. + resolved, traits, none -> REFUSE, and say so. A native v2 board that expresses traits and + carry review declares no review lane genuinely has nowhere to open a PR from. + Naming `'in-review'` sends the operator to a column their board does + not have — an impossible instruction, which is the defect this + conversion set out to remove, reappearing in its own fallback. + + `declaresAnyLifecycleTrait` is what separates the middle case from the last; the empty set alone + cannot, which is why the first pass got it wrong in the safe direction and this one in the loud one. + */ const resolvedReviewColumns = prIr === undefined ? [] : resolveReviewColumns(prIr); + const traitsExpressed = prIr !== undefined && workflowExpressesAnyTrait(prIr); + if (traitsExpressed && resolvedReviewColumns.length === 0) { + console.error( + `Error: Task ${id}'s workflow declares no review column, so a PR cannot be created from it (current: ${task.column})`, + ); + await closeProjectStore(context); + process.exit(1); + } const reviewColumns = new Set(resolvedReviewColumns.length > 0 ? resolvedReviewColumns : ["in-review"]); if (!reviewColumns.has(task.column)) { /*