fix(tests): new CLI red from #2775 — the test pinned a decision its own PR superseded (#2801)

## New red on main #2775 landed and put one failure on `main`, in a test that PR itself added: ``` pr-create-review-lane-resolved.test.ts > refuses WITHOUT naming a phantom lane when the workflow declares no review lane AssertionError: expected undefined to be defined ``` ## Two review rounds pushed `pr.ts` in opposite directions; the test is from the losing one | round | decision | |---|---| | **1** (greptile P2) | a resolved workflow with no review-trait column is an **answer** — do not invent `'in-review'`, say *"no review lane"*. **This test was written against that.** | | **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 shipped** (`pr.ts:206-207`) and is right: 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. Both rounds are extensively documented in `pr.ts` — the code is deliberate and I have not touched it. The consequence is simply that **there is no "no review lane" message in the shipped code at all**, so `errors.find((e) => e.includes("no review lane"))` returned `undefined`. The test could never have passed against what merged. ## The fix Re-pointed at the contract that actually shipped: the filtered board takes the legacy `'in-review'` fallback, and the refusal must **not** name the renamed lanes (`signoff`, `waiting-on-a-human`) that this board no longer declares — which preserves the anti-phantom-lane intent the test was named for. ## Flagged, not guessed The round-1 behaviour is **not recoverable** without a way to distinguish *"v2 board that declares no review lane"* from *"v1 board whose traits were synthesised empty"*. The IR does not currently carry that signal, so emitting a distinct message would re-break every pre-v2 project — the exact regression round 2 caught. Recorded in the test rather than invented. ## Evidence Mutations, both caught: | mutation | result | |---|---| | fallback names lanes the board lacks | **1 failed** | | the review-lane gate removed entirely | **2 failed** | Full CLI package **1684 passed / 106 skipped (126 files)** — was 1 failed. Gate **732 green** · lint clean. Test-only; `pr.ts` restored clean after the mutations. ## How this was found Pre-flighting the open batch PRs against current `main` rather than their branch heads, after batch-engine's previous landing put 32 failures on main that were only caught post-merge. #2785 and #2783 both came back clean (commented on each); re-running `main` itself after the newest landings surfaced this one. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-30 11:25:46 -07:00
parent bb8be93c52
commit 29186a96da
2 changed files with 98 additions and 7 deletions
--- 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 () => {
--- 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<typeof resolveReviewColumns>[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)) {
      /*