From 642a4fa264eb4c61544235e04522567bc36ba27b Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 00:13:58 -0700 Subject: [PATCH] =?UTF-8?q?consolidate/u12=20=E2=80=94=20U12=20consolidati?= =?UTF-8?q?on:=204=20live=20defects,=20the=20AST=20ratchet=20fail-closed,?= =?UTF-8?q?=20and=20the=20moves.ts=20flag=20scoped=20(#2647)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One branch, one PR, per the consolidation directive. Contents file-by-file below. **Supersedes #2625** (its overlapping conversions landed via U11's #2624/#2626/#2636; only the parts nobody else did are folded here). **#2630 and #2639 stay open** — both green with zero threads, per rule 3. ## Four live defects, each measured **1. Every planning card renders an actions menu.** `TaskContextMenu.tsx` still had `shouldShowActionsMenu: task.column !== "triage"` on main *after* the rest of that file was converted. Since #2515 removed the id, the condition is TRUE for every card, so the suppression stopped applying anywhere — including on cards whose menu is empty, the orphaned click target the Surface Enumeration rule exists to catch. Found **twice independently**: by reading the guard, and again by the invariance test below, which failed on main with `shouldShowActionsMenu` true on one lineage and false on another. That is the argument for an invariance property over per-site conversion — the file had already been converted "2 → 1" and the survivor was the live one. **2. Worktree upcoming-work list empty on renamed boards.** `groupByWorktree` filtered `t.column === "todo"`. On the default board the id and the role coincide so every existing test passed; renamed, it matched nothing and a whole panel read as idle. **3. Hold-lane FIFO ordering lost on renamed boards.** `sortTasksForDisplayColumn` gated priority-then-FIFO on `column === "todo"`, degrading to the generic id-ordered sort elsewhere. Cards simply appear in the wrong order, silently. **4. The AST ratchet still failed open** — fourth time in that file, third found by review. `receiverName` understood only one-level property access and bare identifiers, so `task["column"]`, `metadataColumn(entry, "to")`, ternaries, `(task!.column)` and backtick literals were dropped. **Measured on main: `in-progress` 196 → 197, `in-review` 211 → 213** — three real guards nobody counted, including `metadataColumn(entry, "to") === "in-review"` in `reliability-metrics.ts`. Now walks wrappers, resolves calls to the callee name, and emits a `` **sentinel** for anything unnameable: counted *and* trips the classification guard, so a human judges it instead of it vanishing. ## Per-file guard counts | file | before | after | |---|---:|---:| | `app/components/TaskContextMenu.tsx` | 1 | **0** | | `app/utils/worktreeGrouping.ts` | 1 | **0** | | `app/components/taskSorting.ts` | 1 | **0** | The other dashboard files I had converted reached 0 via U11's PRs; where our work overlapped I took theirs during the rebase, including two places where theirs was **stronger** than mine — they deleted Column's unreachable quick-create arm outright (with fixtures migrated) where I had converted it, and they verified the same `isPreExecutionHoldColumn` degraded-set asymmetry I did, independently. ## Flip precondition: the moves.ts flag is scoped, not flipped `move-target-declared-census.test.ts` answers precondition 2 with measurement. 41 engine `moveTask` calls have literal targets — `todo` 27, `in-progress` 7, `done` 6, `archived` 1 — and **all four are declared by the default lineage**, so the default board is not the exposure. `triage` appears only in a comment noting `replan-target.ts` used to hardcode it. My own grep had said `todo=29`; the AST says 27, because grep counts comments. The exposure is **custom** lineages: 20 of the 41 carry no `recoveryRehome` and would reject with unknown-column post-flip; 21 are exempt via the #1411 carve-out, which makes that carve-out load-bearing. I did not flip the flag. It is six seams, not the `789`/`837` pair every summary including mine described, and seam 2 turns on *new refusals* rather than swapping equivalent implementations — a green suite says nothing about that. #2639 pins the blast radius. ## Tests - `column-role-id-invariance.test.tsx` — hold traits fixed, vary only the column id across MERGED / LEGACY / RENAMED; every decision must agree. Drives the real consumers, so a component keeping an inline comparison fails it. Includes a unanimous-and-**false** case so it can't be satisfied by a predicate hardwired to true. **This is the test that caught defect 1 on main.** - `worktreeGrouping.test.ts` — includes two cards both in a column named `staging`, one hold and one not, asserting opposite answers. That assertion is impossible under a board-wide column-id set, which is why hold resolution is keyed per task via `getEffectiveTaskWorkflowId` (#2625 review). - `taskSorting.test.ts` — discriminates on the **tiebreak**, not priority: both branches sort by priority, so my first version passed for the wrong reason. Equal-priority cards whose `createdAt` order disagrees with their id order. - `no-hardcoded-lifecycle-columns.test.ts` — 16 detector cases: 11 shapes counted, 4 legitimate ignored, one asserting the sentinel path. Revert checks, all run: menu suppression → diff names the field; worktree → `expected [] to include 'FN-50'`; sort → `FN-2, FN-9` instead of `FN-9, FN-2`; ratchet → the 3 recovered guards disappear. ## One site that should never be converted `MissionControlPanel.tsx:46` — `{ id: "triage", match: (c) => c === "triage" || c === "signal" || c === "backlog" }` is a deliberate name-similarity heuristic for the SDLC funnel; it matches synonyms and folds unknown columns into an "other" bucket so custom columns still contribute. Converting it changes what the funnel displays. Like the `live-agent-count` fallbacks, it belongs in a documented floor — **the ratchet's target is that floor, not zero.** `DocumentsView.tsx:73` is convertible but the file has no column flags at all, so a real fix means plumbing board-workflow metadata into a view that doesn't fetch it — its own unit of work. ## Verification `pnpm lint` clean. `pnpm test:gate` green (10 / 482 / 71). `tsc -p packages/dashboard/tsconfig.app.json` and `packages/core/tsconfig.json` clean. Core ratchet + seam suites 24/24. Dashboard target suites 37/38 — the one failure is the pre-existing `"Back to In Progress"` label casing, confirmed identical on the base. --- ## Added after the initial push **5. `TaskCard` lost inline editing on renamed boards; `TaskDetailModal` kept it.** Still live on main: the modal resolved field editability from traits in U10/R8, the card used a hardcoded `{triage, todo}` set with **no trait path at all** — even though `taskColumnFlags` was already in scope. On a renamed board the title was editable in the modal and the pencil was missing from the card. Body moved unchanged into `isFieldEditableColumnRole` so the two surfaces cannot drift again. The veto traits are the substance: a column can legally carry `hold` **and** a WIP or review trait, and a plain `intake || hold` check would let an operator rewrite a description while a session executes against it. Coverage gap **measured, not assumed**: mutating `canEdit` back to the hardcoded set left `TaskCard*` at the same failure count as the unmutated run — nothing caught it. The four render cases assert the real `aria-label`; that mutation now fails with `Unable to find an accessible element ... name 'Edit task'`. **6. The ratchet's target is a documented FLOOR, not zero** — and this changes the completion bar. Zero is not reachable, and chasing it means breaking working code. Two categories are permanent, now protected as positive assertions so a future sweep cannot "finish the job" by deleting them: - `MissionControlPanel.tsx`'s `FUNNEL_STAGES` is a deliberate **name-similarity** heuristic — it matches `signal`, `backlog`, `to-do`, `ready`, `shipped` and folds unrecognised columns into an "other" bucket so a custom board still contributes counts. It is not asking whether a column has the intake trait; it buckets arbitrary column *names* for display. Asserted on the **synonym list**, because the synonyms are what prove it is name matching — if they disappear the site has changed character and the exemption stops applying. - `live-agent-count.ts`'s no-flags arm is reachable (a remote store is deliberately given an empty flag map; a card in an undeclared column has no flags at all) and deleting the literal makes such a card match **no** arm, so the queued total silently under-reports a stranded card. A count with an undocumented floor invites someone to drive it to zero. **Not done, and why:** `DocumentsView.tsx:73` is convertible but that file has no column flags anywhere, so a real fix means plumbing board-workflow metadata into a view that does not fetch it — its own unit of work, not something to smuggle into a conversion. **Re-verified after these commits:** `pnpm lint` clean, `pnpm test:gate` green (10 / 482 / 71), `tsc` clean on core and `tsconfig.app.json`, core ratchet suite 26/26, `columnRoles` 10/10, `TaskCard.test.tsx` 384/386 (the 2 are pre-existing CSS assertions). `TaskDetail*` is 130 failed / 551 passed **both with and without** this change — verified by stashing, so pre-existing and unrelated. --- ## Flag resolution: preconditions 1 and 2 are now DISCHARGED. Precondition 3 is blocked, and by evidence. **Precondition 1 — the side-effect equivalence proof — done.** `moves-flag-equivalence.test.ts` runs the same journey under both flag states against live PG and diffs the persisted row. **Result: identical** — whole-row equality across 128 fields plus an equal timing shape, over `todo → in-progress → in-review → todo → in-progress`. That test was **wrong twice** before it meant anything, and both times it was passing: 1. **It proved nothing.** `experimentalFeatures` is **global-only**, and `moves.ts` reads `getSettingsFast()`, which filters global-only keys out of the project layer. My `updateSettings` write was silently discarded, `useWorkflow` was false in *both* runs, and the "proof" compared the legacy path against itself. Found by stamping the flag-ON branch and observing the test still passed. Now written via `updateGlobalSettings`, and the helper **asserts the flag took effect** before the journey runs. 2. **The journey was forward-only**, so it never reached the reopen hook's field resets (`status`, `error`, `blockedBy`, pause clearing) — a mutation there passed. Extended with a backward move and a re-entry. Mutation-verified after both fixes: stamping seam 3, and diverging the reopen hook, each fail the comparison. **Precondition 2 — done, and its answer is a blocker.** The census says the default board is safe: all 41 literal engine move targets are declared by the default lineage. But **20 of those 41 carry no `recoveryRehome`**, so on a custom lineage that does not declare `todo` / `in-progress` / `done`, seam 2 would start rejecting them with unknown-column. That is a user-facing break on custom boards, not a theoretical one, and it is not fixed by the equivalence proof — seam 2 adds *new refusals* rather than swapping implementations. **So the flip is one step away, and the step is not mine to take alone:** those 20 call sites need to resolve their target from the task's workflow (or justify `recoveryRehome`), and they live across engine lanes in `moves.ts` caller territory — U2b/MAIN. Flipping before that trades a dormant flag for broken custom boards. What remains for precondition 3 once those land: flip both readers **atomically** (`moves.ts` + `workflow-task-create-ops.ts`, since the latter computes the preflight the former consumes), delete the flag-OFF branch with its guards, and drop the settings key. --- ## CORRECTION: seam 2 is not a blocker. My earlier claim was wrong. I stated in #2639 and above that "with the flag off there is **no** target-column validation on the move path", so flipping would introduce new refusals. **That is not what happens.** Reproduced against live PG: the identical custom-lineage move rejects with the flag **OFF** as well — ``` Error: Invalid transition: 'backlog' -> 'todo'. Valid targets: building ``` Transition validation is already in force on the flag-OFF path. So for the shape in question — an engine move to a column the task's own workflow does not declare — **the move already fails today**, and seam 2 introduces no new break for it. The 20 census sites lacking `recoveryRehome` are broken on a custom lineage *now*, not broken by the flip. I found this because the discriminator I added to prove "the flag is the cause" failed. Had I written the test to my assumption it would have passed and the false claim would have shipped — the same way the equivalence test passed while proving nothing until I tried to make it fail. **Revised precondition status:** | precondition | status | |---|---| | 1 — side-effect equivalence | **discharged** — identical rows, mutation-verified both directions | | 2 — seam-2 exposure census | **discharged, and it is not a blocker** — the rejection predates the flag | | 3 — flip both readers atomically, delete the flag-OFF branch, drop the settings key | **the remaining work** | So the flip is no longer gated on fixing 20 engine call sites. What it is still gated on is precondition 3 being done atomically across `moves.ts` and `workflow-task-create-ops.ts` (the latter computes the preflight the former consumes), which is `moves.ts` caller territory. Three cases now cover seam 2: the flag-ON rejection, the flag-OFF rejection (asserting the error *message*, so a change in which guard rejects stays visible rather than reading as agreement), and the #1411 `recoveryRehome` carve-out succeeding — pinning why that carve-out is load-bearing and must not be tidied away. --------- Co-authored-by: Claude Opus 5 (1M context) --- .changeset/u12-dashboard-column-roles.md | 7 + packages/core/package.json | 2 +- .../__tests__/moves-flag-equivalence.test.ts | 312 ++++++++++++++++++ .../no-hardcoded-lifecycle-columns.test.ts | 209 +++++++++++- .../app/__tests__/columnRoles.test.ts | 39 ++- packages/dashboard/app/components/Board.tsx | 38 ++- packages/dashboard/app/components/Column.tsx | 18 +- .../dashboard/app/components/TaskCard.tsx | 12 +- .../app/components/TaskDetailModal.tsx | 14 +- .../components/__tests__/TaskCard.test.tsx | 67 ++++ .../column-role-id-invariance.test.tsx | 154 +++++++++ .../components/__tests__/taskSorting.test.ts | 60 ++++ .../dashboard/app/components/taskSorting.ts | 16 +- .../utils/__tests__/worktreeGrouping.test.ts | 97 ++++++ packages/dashboard/app/utils/columnRoles.ts | 52 +++ .../dashboard/app/utils/worktreeGrouping.ts | 36 +- .../move-target-declared-census.test.ts | 192 +++++++++++ 17 files changed, 1297 insertions(+), 28 deletions(-) create mode 100644 .changeset/u12-dashboard-column-roles.md create mode 100644 packages/core/src/__tests__/moves-flag-equivalence.test.ts create mode 100644 packages/dashboard/app/components/__tests__/column-role-id-invariance.test.tsx create mode 100644 packages/engine/src/__tests__/move-target-declared-census.test.ts diff --git a/.changeset/u12-dashboard-column-roles.md b/.changeset/u12-dashboard-column-roles.md new file mode 100644 index 0000000000..3fa1c19027 --- /dev/null +++ b/.changeset/u12-dashboard-column-roles.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix board affordances that broke on renamed or merged column lineages. +category: fix +dev: Column-role helpers move to @fusion/core (column-roles.ts); dashboard resolves intake/hold/planner roles from traits instead of the literal `triage`/`todo` ids. Fixes empty actions menus on planning cards, missing first-paint quick-create, lost hold-lane FIFO ordering, and an empty worktree upcoming-work list on renamed boards. diff --git a/packages/core/package.json b/packages/core/package.json index 3b049e3eeb..ca427445d2 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -49,7 +49,7 @@ "test": "vitest run --silent=passed-only --reporter=dot", "test:embedded-postgres": "vitest run src/__tests__/postgres/embedded-lifecycle.test.ts --silent=passed-only --reporter=dot", "test:pg-gate": "vitest run --config vitest.pg.config.ts src/__tests__/postgres/handoff-to-review-atomicity.pg.test.ts src/__tests__/postgres/task-lifecycle-e2e.pg.test.ts --silent=passed-only --reporter=dot", - "test:unit-gate": "vitest run src/__tests__/task-merge.test.ts src/__tests__/legacy-adoption.test.ts --silent=passed-only --reporter=dot" + "test:unit-gate": "vitest run src/__tests__/task-merge.test.ts src/__tests__/legacy-adoption.test.ts src/__tests__/no-hardcoded-lifecycle-columns.test.ts --silent=passed-only --reporter=dot" }, "devDependencies": { "@earendil-works/pi-coding-agent": "0.82.1", diff --git a/packages/core/src/__tests__/moves-flag-equivalence.test.ts b/packages/core/src/__tests__/moves-flag-equivalence.test.ts new file mode 100644 index 0000000000..ce1225b9d4 --- /dev/null +++ b/packages/core/src/__tests__/moves-flag-equivalence.test.ts @@ -0,0 +1,312 @@ +/* +FNXC:WorkflowColumns 2026-07-31-02:00 (U12 — precondition 1 for flipping the move-path flag): +DO THE TWO COLUMN-SIDE-EFFECT IMPLEMENTATIONS AGREE? + +`moves.ts` gates six behaviours on the raw compatibility flag (#2639 pins all six). Seam 3 is the one +that is genuinely an EQUIVALENCE question: flag-OFF runs an inline legacy block, flag-ON routes the +same column side effects through the default-workflow trait hooks — timing accumulation, +reset-on-entry, abort-on-exit, `merge.onEnter`. Neither is the observed baseline, because they have +never both run in production, so "the suite is green after the flip" says nothing. + +WHY THIS IS BUILDABLE NOW, which I had assumed it was not. The flag reads +`settings.experimentalFeatures.workflowColumns`, and `updateSettings` is public — so a test can run +the SAME move under both flag states against a live store and diff the persisted row. No production +change, no mock of the thing under test. + +WHAT IT COMPARES. The full persisted task, minus fields whose difference carries no meaning +(identity, and wall-clock stamps that advance between two runs). Comparing whole rows rather than a +curated field list is deliberate: a curated list only proves the fields I already suspected, and the +entire risk here is a side effect nobody enumerated. `moves.ts` mutates ~15 fields in that branch. + +WHAT IT DOES NOT COVER, stated so this is not mistaken for a full clearance: + - `resetPromptCheckboxes` writes to the task DIRECTORY, not the row, so a row diff cannot see it. + - Plugin hooks (seam 5) and the transition-pending marker (seam 4) are separate seams. + - Seam 2 turns on NEW REJECTIONS rather than swapping implementations, so it is not an equivalence + question at all; #2647's `move-target-declared-census.test.ts` measures that exposure instead. +This test discharges seam 3 for the row state, which is the part that was pure assertion before. +*/ +import { afterAll, afterEach, beforeAll, beforeEach, expect, it } from "vitest"; +import { pgDescribe, createSharedPgTaskStoreTestHarness } from "../__test-utils__/pg-test-harness.js"; +import type { TaskDetail } from "../types.js"; +import type { TaskStore } from "../store.js"; + +/** + * Fields whose difference between two runs is meaningless: identity, and stamps that advance with + * wall clock. Everything else must match, including the timing ACCUMULATORS (`cumulativeActiveMs`), + * which are the interesting part — they are computed from deltas, so a divergence in how the two + * implementations anchor a segment shows up there rather than in a raw timestamp. + */ +const VOLATILE_FIELDS = new Set([ + "id", + // Per-task UUID; carries no behavioural meaning. + "lineageId", + "createdAt", + "updatedAt", + "columnMovedAt", + "executionStartedAt", + "executionCompletedAt", + "firstExecutionAt", + "cumulativeActiveMs", + "log", +]); + +/* +Wall-clock noise is normalised RECURSIVELY rather than by a flat key list, because it is nested: +`columnDwellMs` is a column -> milliseconds map and run-audit-ish entries carry their own `observedAt`. +A flat list missed both, and the first run of this test reported them as divergences. + +Durations become BOOLEANS (`>0`) rather than being dropped: whether time was attributed to a column at +all is exactly the behaviour under test, while the millisecond value differs between any two runs. +Dropping them would have hidden a real divergence; comparing them would have been permanently flaky. +*/ +function normalize(value: unknown, key?: string, taskId?: string): unknown { + /* + IDENTITY is substituted inside strings rather than the field being dropped. `prompt` embeds the task + id (`# KB-001` vs `# KB-002`), so a raw comparison always fails and dropping it would stop comparing + the spec content entirely — which is one of the things the reset-on-entry side effect can touch. + Replacing the id keeps the content under test. + */ + if (typeof value === "string" && taskId) return value.split(taskId).join(""); + if (typeof value === "number" && key !== undefined && /Ms$/.test(key)) return value > 0; + if (Array.isArray(value)) return value.map((entry) => normalize(entry, undefined, taskId)); + if (value && typeof value === "object") { + const out: Record = {}; + for (const [k, v] of Object.entries(value as Record)) { + if (VOLATILE_FIELDS.has(k) || /At$/.test(k)) continue; + // A duration MAP: keep the keys, reduce each value to "time was attributed here". + out[k] = /Ms$/.test(k) && v && typeof v === "object" && !Array.isArray(v) + ? Object.fromEntries(Object.entries(v as Record).map(([ck, cv]) => [ck, typeof cv === "number" ? cv > 0 : cv])) + : normalize(v, k, taskId); + } + return out; + } + return value; +} + +function comparableSnapshot(task: TaskDetail): Record { + return normalize(task, undefined, task.id) as Record; +} + +/** Which timing fields were SET (not their values), so anchoring behaviour is still compared. */ +function timingShape(task: TaskDetail): Record { + const t = task as unknown as Record; + return { + hasExecutionStartedAt: t.executionStartedAt != null, + hasExecutionCompletedAt: t.executionCompletedAt != null, + hasFirstExecutionAt: t.firstExecutionAt != null, + accumulatedActiveTime: typeof t.cumulativeActiveMs === "number" && (t.cumulativeActiveMs as number) > 0, + }; +} + +/* +FNXC:WorkflowColumns 2026-07-31-02:30 (U12 — the trap this test fell into first): +THE FLAG IS GLOBAL-ONLY, so it must be written through `updateGlobalSettings`. + +My first version used `updateSettings`, and the test PASSED — while proving nothing. `moves.ts` reads +`getSettingsFast()`, which filters `isGlobalOnlySettingsKey` out of the project layer, and +`experimentalFeatures` is exactly such a key (`isGlobalSettingsKey("experimentalFeatures") === true`). +So the project-scoped write was discarded, `useWorkflow` was false in BOTH runs, and the "equivalence +proof" was comparing the legacy path against itself. + +Caught by stamping the flag-ON branch of `moves.ts` and observing that the test still passed — i.e. by +checking that the mutation could be detected, not by trusting the green. Exactly the failure this +program keeps finding, produced by me this time. +*/ +async function setFlag(store: TaskStore, enabled: boolean): Promise { + const current = await store.globalSettingsStore.getSettings(); + await store.updateGlobalSettings({ + ...current, + experimentalFeatures: { ...(current.experimentalFeatures ?? {}), workflowColumns: enabled }, + } as never); + + // Prove the write took effect before relying on it — the whole point of this note. + const effective = await store.getSettingsFast(); + if ((effective.experimentalFeatures?.workflowColumns === true) !== enabled) { + throw new Error(`flag write did not take effect: wanted ${enabled}, moves.ts would read ${effective.experimentalFeatures?.workflowColumns === true}`); + } +} + +/** + * Drive one task through a column journey under a fixed flag state and return what persisted. + * The journey covers the transitions the legacy block special-cases: entering execution, leaving it + * (segment accumulation + abort-on-exit), and reaching review. + */ +async function runJourney(store: TaskStore, flagEnabled: boolean): Promise<{ snapshot: Record; timing: Record }> { + await setFlag(store, flagEnabled); + /* + IDENTICAL description in both runs. My first version interpolated the flag state and the whole-row + diff dutifully reported it — a self-inflicted failure that would have read as a real divergence. + */ + const created = await store.createTask({ description: "equivalence journey" }); + + /* + THE JOURNEY MUST GO BACKWARD TOO. My first version was forward-only (todo -> in-progress -> + in-review) and a mutation to the REOPEN hook did not fail it: those field resets (`status`, `error`, + `blockedBy`, pause clearing) only run when a card moves back out of a later column, so a + forward-only journey never reached them. The test passed while covering roughly half the branch. + + Now: enter execution, reach review, reopen to the hold column (reset-on-entry + abort-on-exit), and + re-enter execution so the second segment's timing accumulation is exercised on top of the first. + */ + await store.moveTask(created.id, "in-progress"); + await store.moveTask(created.id, "in-review"); + await store.moveTask(created.id, "todo"); + await store.moveTask(created.id, "in-progress"); + + const final = await store.getTask(created.id); + if (!final) throw new Error("task vanished mid-journey"); + return { snapshot: comparableSnapshot(final), timing: timingShape(final) }; +} + +pgDescribe("move-path side effects are equivalent with the compatibility flag OFF and ON (U12 seam 3)", () => { + const harness = createSharedPgTaskStoreTestHarness({ prefix: "fusion_moves_flag_equiv" }); + beforeAll(harness.beforeAll); + beforeEach(harness.beforeEach); + afterEach(harness.afterEach); + afterAll(harness.afterAll); + + /* + One test rather than two, because the assertion IS the comparison: neither run has meaning alone. + Ordered flag-OFF first so the legacy path — the one actually running in production today — is the + expected value, and any divergence reads as "the trait hooks differ from shipped behaviour". + */ + it("the persisted row is identical either way, and so is the timing shape", async () => { + const store = harness.store(); + + const legacy = await runJourney(store, false); + const traitHooks = await runJourney(store, true); + + /* + Whole-row equality. If this fails, the flip changes persisted state on every task move and the + diff names the field — which is the evidence precondition 1 asks for, in either direction. + */ + expect(traitHooks.snapshot).toEqual(legacy.snapshot); + + /* + Timing is compared as a SHAPE, not by value: the two runs happen at different wall-clock instants, + so equal millisecond counts would be coincidence and a mismatch would be noise. What must agree is + which anchors got set and whether active time accumulated at all — a divergence there means the + two implementations disagree about when execution starts or ends, which would silently corrupt + every task's duration. + */ + expect(traitHooks.timing).toEqual(legacy.timing); + }); + + /* + FNXC:WorkflowColumns 2026-07-31-03:00 (U12 — seam 2, demonstrated rather than inferred): + WHAT THE FLIP WOULD BREAK ON A CUSTOM BOARD. + + Precondition 2's census established that 20 of 41 literal engine `moveTask` targets carry no + `recoveryRehome`, and inferred that those would start rejecting on a lineage that does not declare + the legacy ids. Inference is not enough to hand someone as a work order, so this reproduces it. + + Seam 2 is NOT an equivalence question: with the flag off there is no target validation at all, so + flipping introduces refusals for moves that succeed today. This case is the refusal, and the second + case is the #1411 carve-out that the other 21 sites rely on — which is what makes that carve-out + load-bearing for the flip rather than incidental. + + If the first expectation ever starts passing, seam 2 stopped rejecting undeclared targets and the + 20 call sites are no longer a blocker — delete this and flip. + */ + it("REJECTS an engine move to a column the task's own workflow does not declare", async () => { + const store = harness.store(); + await setFlag(store, true); + + // A lineage with none of the legacy ids: no todo, no in-progress, no done. + const definition = await store.createWorkflowDefinition({ + name: "no-legacy-ids", + ir: { + version: "v2", + name: "no-legacy-ids", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }, { id: "end", kind: "end", column: "shipped" }], + edges: [{ from: "start", to: "end" }], + }, + } as never); + + const task = await store.createTask({ description: "custom lineage card", workflowId: definition.id } as never); + + /* + A plain engine-shaped move to `todo` — the shape 20 census sites use. It must reject, because + `todo` is not a column this workflow declares. This is the break the flip would ship. + */ + await expect(store.moveTask(task.id, "todo")).rejects.toThrow(); + }); + + it("REJECTS that same move with the flag OFF TOO — correcting the blast-radius claim", () => { + /* + FNXC:WorkflowColumns 2026-07-31-03:15 (U12 — I had this wrong, and it matters): + I wrote in #2639 and in the census that "with the flag off there is NO target-column validation on + the move path", so flipping would introduce new refusals. THAT IS NOT WHAT HAPPENS. Running the + identical custom-lineage move with the flag OFF also rejects: + + Error: Invalid transition: 'backlog' -> 'todo'. Valid targets: building + + Transition validation is already in force on the flag-OFF path. So for this shape — an engine move + to a column the task's workflow does not declare — the move is ALREADY failing today, and seam 2 + does not introduce a new break for it. The 20 census sites without `recoveryRehome` are therefore a + smaller risk than I reported: on a custom lineage they are broken now, not broken by the flip. + + I am asserting the CURRENT behaviour rather than the behaviour I expected, because a test written to + my assumption would have failed and I would have "fixed" the fixture until it agreed with a claim + that was false. The error message is asserted so a future change in WHICH guard rejects is visible + rather than silently reinterpreted as agreement. + */ + return (async () => { + const store = harness.store(); + await setFlag(store, false); + + const definition = await store.createWorkflowDefinition({ + name: "no-legacy-ids-flagoff", + ir: { + version: "v2", + name: "no-legacy-ids-flagoff", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }, { id: "end", kind: "end", column: "shipped" }], + edges: [{ from: "start", to: "end" }], + }, + } as never); + + const task = await store.createTask({ description: "custom lineage card, flag off", workflowId: definition.id } as never); + await expect(store.moveTask(task.id, "todo")).rejects.toThrow(/Invalid transition/); + })(); + }); + + it("ACCEPTS the same move when it carries the #1411 recoveryRehome carve-out", async () => { + const store = harness.store(); + await setFlag(store, true); + + const definition = await store.createWorkflowDefinition({ + name: "no-legacy-ids-rescue", + ir: { + version: "v2", + name: "no-legacy-ids-rescue", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }, { id: "end", kind: "end", column: "shipped" }], + edges: [{ from: "start", to: "end" }], + }, + } as never); + + const task = await store.createTask({ description: "rescued card", workflowId: definition.id } as never); + + /* + The carve-out exists so a custom-workflow card can still be rescued to a guaranteed-safe landing + column. The 21 census sites that pass it are already flip-safe; this pins that, so nobody "cleans + up" the carve-out and breaks recovery on custom boards. + */ + const moved = await store.moveTask(task.id, "todo", { recoveryRehome: true, bypassGuards: true } as never); + expect(moved.column).toBe("todo"); + }); +}); diff --git a/packages/core/src/__tests__/no-hardcoded-lifecycle-columns.test.ts b/packages/core/src/__tests__/no-hardcoded-lifecycle-columns.test.ts index 8ee01c75fd..22c92d14b3 100644 --- a/packages/core/src/__tests__/no-hardcoded-lifecycle-columns.test.ts +++ b/packages/core/src/__tests__/no-hardcoded-lifecycle-columns.test.ts @@ -92,15 +92,55 @@ function sourceFiles(): string[] { return out; } -/** The receiver's name: `task.column` -> "column"; a bare `toColumn` -> "toColumn". */ +/* +FNXC:LifecycleColumnRatchet 2026-07-30-23:00 (U12 — the fourth fail-open in this file): +AN UNNAMEABLE RECEIVER IS COUNTED, NOT DROPPED. + +The AST rewrite fixed quoting and line-orientation, but `receiverName` still understood only a +ONE-LEVEL property access or a bare identifier, and the caller `continue`d on `undefined`. So these +were silently uncounted: + + task["column"] === "triage" // ElementAccess + metadataColumn(entry, "to") === "in-review" // CallExpression + (flag ? from : to) === "triage" // ConditionalExpression + (task!.column) === "triage" // Parenthesized / NonNull wrapper + task.column === `triage` // NoSubstitutionTemplateLiteral + +Live, not theoretical: `metadataColumn(entry, "to") === "in-review"` in +`dashboard/src/reliability-metrics.ts` is THREE real lifecycle guards this ratchet was not counting. +Found by printing what the scanner reported as unnamed instead of trusting its total. + +That is the fourth time this file has failed OPEN — grep quoting, then the `/\w*[Cc]olumn/` name +pattern, then one-level property access. Same mechanism every time: a shape the scanner did not +understand became "not a column" and left the count. So the fix is the PROPERTY, not the case: walk +through wrappers, resolve a call to its callee name, and emit a `` SENTINEL for anything +still unnameable. A sentinel is counted AND trips the classification test, so a human judges it +instead of it vanishing. Fail closed — which is what makes this number safe to gate on at all. +*/ function receiverName(node: ts.Expression): string | undefined { - if (ts.isPropertyAccessExpression(node)) return node.name.text; - if (ts.isIdentifier(node)) return node.text; - return undefined; + let cur: ts.Node = node; + for (;;) { + if (ts.isPropertyAccessExpression(cur)) { cur = cur.name; continue; } + if (ts.isElementAccessExpression(cur)) { + const arg = cur.argumentExpression; + if (arg && (ts.isStringLiteral(arg) || ts.isNoSubstitutionTemplateLiteral(arg))) return arg.text; + cur = cur.expression; + continue; + } + // A helper that RETURNS a column is classified by the function's name. + if (ts.isCallExpression(cur)) { cur = cur.expression; continue; } + if (ts.isParenthesizedExpression(cur) || ts.isNonNullExpression(cur) || ts.isAsExpression(cur)) { + cur = cur.expression; + continue; + } + break; + } + if (ts.isIdentifier(cur)) return cur.text; + return `<${ts.SyntaxKind[cur.kind]}>`; } /** Walk one parsed file for `X === ""` / `X !== ""` where X names something column-like. */ -function collect(sf: ts.SourceFile, columnId: string, file: string, sites: Site[]): void { +export function collect(sf: ts.SourceFile, columnId: string, file: string, sites: Site[]): void { const visit = (node: ts.Node): void => { if ( ts.isBinaryExpression(node) @@ -110,8 +150,16 @@ function collect(sf: ts.SourceFile, columnId: string, file: string, sites: Site[ const pairs = [[node.right, node.left], [node.left, node.right]] as const; for (const [lit, other] of pairs) { // `.text` is the DECODED value, so single and double quotes are indistinguishable here. - if (!ts.isStringLiteral(lit) || lit.text !== columnId) continue; + // Backtick literals are string literals to a reader and to the runtime; excluding them left + // `task.column === \`triage\`` uncounted for no reason anyone would defend. + const isLiteral = ts.isStringLiteral(lit) || ts.isNoSubstitutionTemplateLiteral(lit); + if (!isLiteral || lit.text !== columnId) continue; const receiver = receiverName(other); + /* + `receiverName` no longer returns undefined for an expression it cannot name — it returns a + `` sentinel that is COUNTED and must be classified. Dropping on undefined is what + made this fail open; the undefined arm is kept only as a defensive no-op. + */ if (receiver === undefined || NON_COLUMN_RECEIVERS.has(receiver)) continue; sites.push({ file, @@ -148,11 +196,30 @@ function comparisonSites(columnId: string): Site[] { * A raise means a guard came back — convert it, or if the receiver genuinely is not a column, add it * to NON_COLUMN_RECEIVERS with a reason. */ +/* +FNXC:LifecycleColumnRatchet 2026-07-31-01:30 (PR #2647 review — greptile, and worse than reported): +TIGHTENED TO THE MEASURED COUNTS, because slack made this a ratchet in name only. + +The review's finding was that this test is not in the blocking gate, so a regression can merge. True, +and fixed (`packages/core/package.json` -> `test:unit-gate`). But admitting it to the gate proved the +bigger problem: I reintroduced `task.column === 'triage'` and the gate STAYED GREEN at 158 passed. + +The ceilings carried 26 free `triage` slots (11 measured vs 37), plus 18 for `todo`, 4 for +`in-progress` and 4 for `in-review`. A ceiling with slack does not ratchet — it permits exactly as +many regressions as the gap, silently, which is the same "guard that cannot fire" this whole program +keeps finding. Being in the gate was necessary and not sufficient. + +Every number here is now the measured count, so ANY reintroduction fails. Proven, not assumed: with +these values the same single-guard probe takes the gate red. + +LOWER these as conversions land; a raise means a literal came back OR detection improved — and if it +is the latter, say which sites in the commit, as previous revisions of this file did. +*/ const CEILINGS: Record = { - triage: 37, - todo: 82, - "in-progress": 201, - "in-review": 217, + triage: 11, + todo: 64, + "in-progress": 197, + "in-review": 213, }; describe("lifecycle-column literal ratchet (AST)", () => { @@ -241,3 +308,125 @@ describe("lifecycle-column literal ratchet (AST)", () => { expect(KNOWN_COLUMN_RECEIVERS.size).toBeGreaterThan(0); }); }); + +/* +FNXC:LifecycleColumnRatchet 2026-07-30-23:00 (the fourth fail-open, pinned so it is the last): +PROOF THAT EVERY REINTRODUCTION SHAPE IS COUNTED. + +Each shape below was silently dropped by at least one earlier revision of this file: quoting by the +grep, short bindings by the name pattern, and wrappers/calls/brackets by the one-level +`receiverName`. Three of the four were found by review rather than by the ratchet itself, which is +the argument for testing the detector directly instead of trusting its total. + +MEASURED IMPACT of the wrapper/call fix, on current main: `in-progress` 196 -> 197 and `in-review` +211 -> 213. Those three were real guards nobody was counting, including +`metadataColumn(entry, "to") === "in-review"` in dashboard/src/reliability-metrics.ts. +*/ +describe("the detector counts every reintroduction shape (fail closed)", () => { + function sitesFor(source: string, columnId: string) { + const sf = ts.createSourceFile("probe.ts", source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const sites: Site[] = []; + collect(sf, columnId, "probe.ts", sites); + return sites; + } + + const RED: Array<[string, string]> = [ + ["double-quoted", `if (task.column === "triage") return;`], + ["single-quoted", `if (task.column === 'triage') return;`], + ["backtick literal", "if (task.column === `triage`) return;"], + ["multiline", `if (task.column ===\n "triage") return;`], + ["negated", `if (task.column !== "triage") return;`], + ["reversed operands", `if ("triage" === task.column) return;`], + ["bracket access", `if (task["column"] === "triage") return;`], + ["deep qualified", `if (state.board.tasks[i].column === "triage") return;`], + ["parenthesised non-null", `if ((task!.column) === "triage") return;`], + ["helper call returning a column", `if (metadataColumn(entry, "to") === "triage") return;`], + ["bare binding", `if (toColumn === "triage") return;`], + ]; + + for (const [label, source] of RED) { + it(`counts ${label}`, () => { + expect(sitesFor(source, "triage")).toHaveLength(1); + }); + } + + it("counts an unnameable receiver under a sentinel rather than dropping it", () => { + /* + THE FAIL-CLOSED PROPERTY ITSELF. Every earlier revision answered "not a column" for a shape it + did not understand, so the site left the count in silence. A ternary receiver must be COUNTED + with a `<...>` sentinel, which then trips the classification guard and forces a human judgement. + + If this returns 0, the ratchet has gone back to failing open and its number stops being + trustworthy — which is the only reason it is allowed to gate. + */ + const sites = sitesFor(`if ((flag ? from : to) === "triage") return;`, "triage"); + expect(sites).toHaveLength(1); + expect(sites[0]!.receiver).toMatch(/^<.*>$/); + }); + + const GREEN: Array<[string, string, string]> = [ + ["agent role", `if (role === "triage") return;`, "triage"], + ["agent display name", `if (entry.agent === "triage") return;`, "triage"], + ["a line comment", `// if (task.column === "triage") return;`, "triage"], + ["a block comment", `/* task.column === "triage" */`, "triage"], + ]; + + for (const [label, source, id] of GREEN) { + it(`ignores ${label}`, () => { + /* + The other half: a detector that counted everything would "catch" reintroductions and be + useless, because converting `role === "triage"` breaks prompt-template resolution and the count + could never reach its floor. Comments are excluded structurally — the AST has none. + */ + expect(sitesFor(source, id)).toHaveLength(0); + }); + } +}); + +/* +FNXC:LifecycleColumnRatchet 2026-07-31-00:45 (U12 — the bar is a FLOOR, not zero): +SITES THAT MUST NEVER BE CONVERTED, asserted as a positive. + +The program has been tracking these counts toward zero. Zero is not reachable, and pursuing it means +breaking working code — the same trap as demanding conversion of `role === "triage"`, one level up. +Two categories are permanent, each for a stated reason, and both are protected here so a future sweep +cannot "finish the job" by removing them: + +1. `dashboard/app/components/command-center/MissionControlPanel.tsx` — `FUNNEL_STAGES` is a deliberate + NAME-SIMILARITY heuristic for the SDLC funnel. It matches synonyms (`signal`, `backlog`, `to-do`, + `ready`, `shipped`) and folds anything unrecognised into an "other" bucket precisely so a custom + board still contributes counts. It is not asking "does this column have the intake trait" — it is + bucketing arbitrary column NAMES for display. Resolving it to traits would change what the funnel + shows and would drop the synonym coverage that makes it work on boards Fusion has never seen. + +2. `core/live-agent-count.ts` — the no-flags arm. Documented in that file and in earlier revisions of + this one: it is REACHABLE (a remote store is deliberately given an empty flag map, and a card in a + column its workflow no longer declares has no flags at all), and deleting the literal would make + such a card match NO arm, so the footer's queued total would silently under-report a stranded card. + +This test exists because a count with an undocumented floor invites someone to drive it to zero. The +honest target is: these sites, and nothing else. +*/ +describe("the reachable floor: sites that must stay", () => { + it("keeps the Mission Control funnel's name-similarity heuristic", () => { + const funnel = readFileSync( + join(REPO_ROOT, "packages/dashboard/app/components/command-center/MissionControlPanel.tsx"), + "utf-8", + ); + /* + Asserted on the SYNONYM list rather than on the `triage` comparison alone: the synonyms are the + evidence that this is name matching and not a lifecycle guard, so if they disappear the site has + changed character and the exemption below no longer applies. + */ + expect(funnel).toContain("FUNNEL_STAGES"); + expect(funnel).toContain('"signal"'); + expect(funnel).toContain('"backlog"'); + }); + + it("keeps live-agent-count's no-flags arm", () => { + const liveAgentCount = readFileSync(join(REPO_ROOT, "packages/core/src/live-agent-count.ts"), "utf-8"); + // Reachable via an empty remote flag map and via a card in an undeclared column; removing it + // makes such a card match no arm at all and the queued total under-reports it. + expect(liveAgentCount).toContain("columnIsIntakeOrHold"); + }); +}); diff --git a/packages/dashboard/app/__tests__/columnRoles.test.ts b/packages/dashboard/app/__tests__/columnRoles.test.ts index dfb7d7a83d..f25ae8710b 100644 --- a/packages/dashboard/app/__tests__/columnRoles.test.ts +++ b/packages/dashboard/app/__tests__/columnRoles.test.ts @@ -20,7 +20,7 @@ LEGACY_….has(columnId)`) fails the "traits win" cases; making it ignore the id (`return Boolean(flags?.intake)`) fails the degraded cases. */ import { describe, expect, it } from "vitest"; -import { isIntakeColumnRole, isPreImplementationColumnRole } from "../utils/columnRoles"; +import { isFieldEditableColumnRole, isIntakeColumnRole, isPreImplementationColumnRole } from "../utils/columnRoles"; describe("isIntakeColumnRole", () => { it("uses the intake TRAIT when the column resolved", () => { @@ -71,3 +71,40 @@ describe("isPreImplementationColumnRole", () => { expect(isPreImplementationColumnRole(undefined, "in-review")).toBe(false); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-00:15 (U12 — one affordance, two components): +FIELD EDITABILITY. TaskDetailModal resolved this from traits in U10/R8; TaskCard kept a hardcoded +`{triage, todo}` set with no trait path, so a renamed board lost the pencil on the card while the +modal kept it — the same one-surface-missed shape as the FN-6115 chevron chain. + +The VETO cases are the ones worth pinning: a column can legally carry `hold` AND a WIP or review +trait, and a plain `intake || hold` check would let an operator rewrite a description while a session +executes against it. +*/ +describe("isFieldEditableColumnRole", () => { + it("allows editing in a renamed pre-implementation column", () => { + expect(isFieldEditableColumnRole({ intake: true }, "backlog")).toBe(true); + expect(isFieldEditableColumnRole({ hold: true }, "parked")).toBe(true); + }); + + it("VETOES editing when a terminal, executing or review trait is also present", () => { + expect(isFieldEditableColumnRole({ hold: true, countsTowardWip: true }, "parked")).toBe(false); + expect(isFieldEditableColumnRole({ intake: true, mergeBlocker: true }, "backlog")).toBe(false); + expect(isFieldEditableColumnRole({ intake: true, humanReview: true }, "backlog")).toBe(false); + expect(isFieldEditableColumnRole({ intake: true, complete: true }, "backlog")).toBe(false); + expect(isFieldEditableColumnRole({ intake: true, archived: true }, "backlog")).toBe(false); + }); + + it("refuses a resolved column with no pre-implementation trait", () => { + expect(isFieldEditableColumnRole({ countsTowardWip: true }, "building")).toBe(false); + }); + + it("falls back to the legacy id pair when traits are absent", () => { + // First paint, and what every caller got before the conversion. + expect(isFieldEditableColumnRole(undefined, "todo")).toBe(true); + expect(isFieldEditableColumnRole(undefined, "triage")).toBe(true); + expect(isFieldEditableColumnRole(undefined, "backlog")).toBe(false); + expect(isFieldEditableColumnRole(undefined, "in-progress")).toBe(false); + }); +}); diff --git a/packages/dashboard/app/components/Board.tsx b/packages/dashboard/app/components/Board.tsx index 9b3d661e4d..f58a3901bc 100644 --- a/packages/dashboard/app/components/Board.tsx +++ b/packages/dashboard/app/components/Board.tsx @@ -549,6 +549,37 @@ export function Board({ tasks, projectId, maxConcurrent, showWorktreeGrouping, o return map; }, [boardWorkflows]); + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The TASK IDS whose own column is a hold lane IN THEIR OWN WORKFLOW. + + Resolved per task rather than as a board-wide set of hold column IDS (PR #2625 review — + greptile). Column ids are namespaced per workflow, so two workflows can both declare + `staging` with one marking it `hold` and the other `countsTowardWip`. A unioned id set + cannot tell those apart, and every executing card in the second workflow would have shown + up under Up Next as waiting work — a wrong answer presented confidently, which is worse + than the renamed-board emptiness this change set out to fix. + + Resolving through `getEffectiveTaskWorkflowId` removes the ambiguity instead of narrowing + it: the question "is this card waiting?" is answered by the card's own workflow, which is + the only workflow that can answer it. + */ + const holdTaskIds = useMemo(() => { + const holdColumnsByWorkflowId = new Map>(); + for (const workflow of boardWorkflows?.workflows ?? []) { + holdColumnsByWorkflowId.set( + workflow.id, + new Set(workflow.columns.filter((col) => col.flags.hold === true).map((col) => col.id)), + ); + } + const ids = new Set(); + for (const task of tasks) { + const workflowId = getEffectiveTaskWorkflowId(task); + if (workflowId && holdColumnsByWorkflowId.get(workflowId)?.has(task.column)) ids.add(task.id); + } + return ids; + }, [boardWorkflows, tasks, getEffectiveTaskWorkflowId]); + const selectedWorkflowContextMenuColumns = useMemo(() => ( selectedWorkflow ? workflowContextMenuColumnsByWorkflowId.get(selectedWorkflow.id) : undefined ), [selectedWorkflow, workflowContextMenuColumnsByWorkflowId]); @@ -586,7 +617,7 @@ export function Board({ tasks, projectId, maxConcurrent, showWorktreeGrouping, o const isWorkflowDoneLikeColumn = column.flags.complete === true && column.flags.archived !== true; grouped[column.id] = isWorkflowDoneLikeColumn ? sortTasksForDisplayColumn(grouped[column.id] ?? [], "done", doneSortMode) - : sortTasksForDisplayColumn(grouped[column.id] ?? [], column.id as ColumnType, doneSortMode, column.flags.archived === true); + : sortTasksForDisplayColumn(grouped[column.id] ?? [], column.id as ColumnType, doneSortMode, column.flags.archived === true, column.flags.hold === true); } return grouped; }, [doneSortMode, selectedWorkflow, selectedWorkflowCreateColumnId, selectedWorkflowTasks]); @@ -770,7 +801,7 @@ export function Board({ tasks, projectId, maxConcurrent, showWorktreeGrouping, o const isDoneLikeColumn = column.flags.complete === true && column.flags.archived !== true; grouped[column.id] = isDoneLikeColumn ? sortTasksForDisplayColumn(grouped[column.id] ?? [], "done", doneSortMode) - : sortTasksForDisplayColumn(grouped[column.id] ?? [], column.id as ColumnType, doneSortMode, column.flags.archived === true); + : sortTasksForDisplayColumn(grouped[column.id] ?? [], column.id as ColumnType, doneSortMode, column.flags.archived === true, column.flags.hold === true); } return grouped; }, [aggregateBoardColumns, aggregateQuickCreateTarget, boardWorkflows, doneSortMode, getEffectiveTaskWorkflowId, tasks, workflowColumnsByWorkflowId]); @@ -890,6 +921,7 @@ export function Board({ tasks, projectId, maxConcurrent, showWorktreeGrouping, o columnDisplayName={columnDef.name} columnDescription={columnDef.description} columnFlags={columnDef.flags} + holdTaskIds={holdTaskIds} taskContextMenuColumnsByTaskId={taskContextMenuColumnsByTaskId} tasks={aggregateTasksByColumn[columnDef.id] ?? []} projectId={projectId} @@ -972,6 +1004,7 @@ export function Board({ tasks, projectId, maxConcurrent, showWorktreeGrouping, o columnDisplayName={columnDef.name} columnDescription={columnDef.description} columnFlags={columnDef.flags} + holdTaskIds={holdTaskIds} workflowContextMenuColumns={selectedWorkflowContextMenuColumns} tasks={selectedWorkflowTasksByColumn[columnDef.id] ?? []} allTasks={selectedWorkflowTasks} @@ -1032,6 +1065,7 @@ export function Board({ tasks, projectId, maxConcurrent, showWorktreeGrouping, o columnDisplayName={selectedWorkflowArchivedColumn.name} columnDescription={selectedWorkflowArchivedColumn.description} columnFlags={selectedWorkflowArchivedColumn.flags} + holdTaskIds={holdTaskIds} workflowContextMenuColumns={selectedWorkflowContextMenuColumns} tasks={selectedWorkflowTasksByColumn[selectedWorkflowArchivedColumn.id] ?? []} allTasks={selectedWorkflowTasks} diff --git a/packages/dashboard/app/components/Column.tsx b/packages/dashboard/app/components/Column.tsx index f058129a71..962d5ff99a 100644 --- a/packages/dashboard/app/components/Column.tsx +++ b/packages/dashboard/app/components/Column.tsx @@ -197,6 +197,14 @@ interface ColumnProps { /** True when the board is in multi-lane workflow mode (flag ON). Switches * column behavior (label, bulk actions, archived detection) from legacy * literals to trait-flag predicates. Flag OFF leaves all behavior legacy. */ + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The ids of tasks whose own column is a hold lane in THEIR OWN workflow, for the worktree + grouping's upcoming-work list. Task ids rather than column ids because column ids are + namespaced per workflow and two workflows can disagree about the same name (PR #2625 + review). Board resolves it; Lane does not pass it and keeps the legacy-id fallback. + */ + holdTaskIds?: ReadonlySet; workflowMode?: boolean; /** Workflow id for column-aware task creation in workflow mode. */ workflowId?: string; @@ -230,7 +238,7 @@ interface ColumnProps { getDraggingTaskId?: () => string | null; } -function ColumnComponent({ column, tasks, projectId, maxConcurrent, showWorktreeGrouping, onMoveTask, onPauseTask, onUnpauseTask, onResetTask, onDuplicateTask, onMergeTask, onOpenDetail, onOpenRefine, onOpenGroupModal, addToast, onQuickCreate, onNewTask, autoMerge, mergeStrategy = "direct", onToggleAutoMerge, planAutoApproveEnabled, onTogglePlanAutoApprove, globalPaused, onUpdateTask, onRetryTask, onArchiveTask, onUnarchiveTask, onRevertTask, onDeleteTask, onArchiveAllDone, doneSortMode, onDoneSortModeChange, collapsed, onToggleCollapse, archivedHasMore, archivedLoadingMore, onLoadMoreArchived, allTasks, availableModels, onPlanningMode, onSubtaskBreakdown, onOpenDetailWithTab, favoriteProviders, favoriteModels, onToggleFavorite, onToggleModelFavorite, isSearchActive, taskStuckTimeoutMs, onOpenMission, lastFetchTimeMs, taskCardFieldDefs, taskWorkflowBadges, blockerFanoutMap, prAuthAvailable, workflowMode, workflowId, workflowOptions, defaultWorkflowId, columnDisplayName, columnDescription, columnFlags, workflowContextMenuColumns, taskContextMenuColumnsByTaskId, onPromote, canDropTask, getDraggingTaskId }: ColumnProps) { +function ColumnComponent({ column, tasks, projectId, maxConcurrent, showWorktreeGrouping, onMoveTask, onPauseTask, onUnpauseTask, onResetTask, onDuplicateTask, onMergeTask, onOpenDetail, onOpenRefine, onOpenGroupModal, addToast, onQuickCreate, onNewTask, autoMerge, mergeStrategy = "direct", onToggleAutoMerge, planAutoApproveEnabled, onTogglePlanAutoApprove, globalPaused, onUpdateTask, onRetryTask, onArchiveTask, onUnarchiveTask, onRevertTask, onDeleteTask, onArchiveAllDone, doneSortMode, onDoneSortModeChange, collapsed, onToggleCollapse, archivedHasMore, archivedLoadingMore, onLoadMoreArchived, allTasks, availableModels, onPlanningMode, onSubtaskBreakdown, onOpenDetailWithTab, favoriteProviders, favoriteModels, onToggleFavorite, onToggleModelFavorite, isSearchActive, taskStuckTimeoutMs, onOpenMission, lastFetchTimeMs, taskCardFieldDefs, taskWorkflowBadges, blockerFanoutMap, prAuthAvailable, holdTaskIds, workflowMode, workflowId, workflowOptions, defaultWorkflowId, columnDisplayName, columnDescription, columnFlags, workflowContextMenuColumns, taskContextMenuColumnsByTaskId, onPromote, canDropTask, getDraggingTaskId }: ColumnProps) { const { t } = useTranslation("app"); // Anchor the board.rejection.* catalog keys for the i18next extractor (it // scopes `t` to the useTranslation binding, so the shared translateRejection @@ -555,8 +563,12 @@ function ColumnComponent({ column, tasks, projectId, maxConcurrent, showWorktree const worktreeGroups = useMemo(() => { if (!showWorktreeGroups) return []; - return groupByWorktree(tasks, allTasks ?? tasks, maxConcurrent); - }, [showWorktreeGroups, tasks, allTasks, maxConcurrent]); + return groupByWorktree(tasks, allTasks ?? tasks, maxConcurrent, holdTaskIds); + // `holdTaskIds` IS a dependency: the board resolves it after the workflows fetch, so + // omitting it would pin the first-paint value and the upcoming-work list would keep + // using the legacy-id fallback for the rest of the session. This repo has no + // react-hooks/exhaustive-deps rule, so nothing catches that but reading it. + }, [showWorktreeGroups, tasks, allTasks, maxConcurrent, holdTaskIds]); const visibleTasks = useMemo(() => { if (!shouldPaginate) return tasks; diff --git a/packages/dashboard/app/components/TaskCard.tsx b/packages/dashboard/app/components/TaskCard.tsx index bbb54f6bbc..64d95a6f64 100644 --- a/packages/dashboard/app/components/TaskCard.tsx +++ b/packages/dashboard/app/components/TaskCard.tsx @@ -34,6 +34,7 @@ import { useTaskDiffStats } from "../hooks/useTaskDiffStats"; import { useAgentsMapCache } from "../hooks/useAgentsMapCache"; import { useLiveTimeTicker } from "../hooks/useLiveTimeTicker"; import { isTaskStuck } from "../utils/taskStuck"; +import { isFieldEditableColumnRole } from "../utils/columnRoles"; import { hasPendingAutomaticRecovery, isTaskManuallyRetryable } from "../utils/taskRecovery"; import { getRevertOfId, isTaskReverted } from "../utils/taskRevert"; import { getStalledReviewSignal } from "../utils/taskStalledReview"; @@ -309,7 +310,6 @@ function isSameAgentIdentity( // Issue 1403: widened to ColumnId so `.has(task.column)` accepts custom column ids // (which are not members and correctly resolve to false). -const EDITABLE_COLUMNS: Set = new Set(["triage", "todo"]); const ACTIVE_MERGE_STATUSES = new Set( [...ACTIVE_STATUSES].filter((status) => ["merging", "merging-pr", "merging-fix", "reviewing", "landing"].includes(status)), @@ -1498,7 +1498,15 @@ function TaskCardComponent({ const isDraggable = !disableDrag && !queued && !isPaused && !isEditing && !isArchived && !isCoarsePointer; // Disable drag during edit/archived, host embedding, or touch // Check if this card can be edited inline - const canEdit = EDITABLE_COLUMNS.has(task.column) && !isAgentActive && !isPaused && !queued && onUpdateTask; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-00:15 (U12 — R8 drift conversion): + THE ASYMMETRY: this read a hardcoded `{triage, todo}` set with no trait path, while + TaskDetailModal resolved the SAME affordance from column traits (U10/R8). So on a renamed board an + operator could edit a task's title in the detail modal but the pencil was missing from its card, + and after #2515 the `triage` half was dead weight. `taskColumnFlags` was already in scope here — + the card simply never asked. + */ + const canEdit = isFieldEditableColumnRole(taskColumnFlags, task.column) && !isAgentActive && !isPaused && !queued && onUpdateTask; const githubTrackedIssue = task.githubTracking?.issue; const hasGithubTrackingLink = Boolean(githubTrackedIssue); const isGitHubImportedTask = task.sourceType === "github_import"; diff --git a/packages/dashboard/app/components/TaskDetailModal.tsx b/packages/dashboard/app/components/TaskDetailModal.tsx index ae90720aca..772cc53ea8 100644 --- a/packages/dashboard/app/components/TaskDetailModal.tsx +++ b/packages/dashboard/app/components/TaskDetailModal.tsx @@ -24,6 +24,7 @@ import { resolveEffectivePlannerOversightLevel } from "../../../core/src/workflo import { resolveTaskSessionAdvisorEnabled } from "../../../core/src/session-advisor"; import { isNearDuplicateCanonicalInactive } from "../../../core/src/near-duplicate-canonical"; import { getRevertOfId, findOpenUndoTaskForSource } from "../utils/taskRevert"; +import { isFieldEditableColumnRole } from "../utils/columnRoles"; import { resolveEffectiveAutoMerge } from "../../../core/src/task-merge"; import { uploadAttachment, deleteAttachment, updateTask, repairOverlapBlocker, pauseTask, unpauseTask, fetchTaskDetail, fetchTaskVerificationRequest, fetchSettings, fetchTaskEffectiveSettings, fetchGlobalSettings, requestSpecRevision, rebuildTaskSpec, approvePlan, rejectPlan, refineTask, fetchWorkflowResults, assignTask, fetchAgents, fetchAgent, refreshPrStatus, fetchBoardWorkflows, updateTaskCustomFields, summarizeTitle, fetchWorkflowSettingValues, nudgeOverseer, stopOverseer, explainOverseer, fetchModels, fetchNodes, api } from "../api"; import type { RevertTaskOptions, RevertTaskResult, ModelInfo, NodeInfo } from "../api"; @@ -662,7 +663,6 @@ function getProvenanceLabel(task: Task | TaskDetail, options: ProvenanceLabelOpt // #1403: widened to ColumnId so `.has(task.column)` accepts custom column ids // (non-members correctly resolve to false → not editable). -const EDITABLE_COLUMNS: Set = new Set(["triage", "todo"]); /* FNXC:WorkflowResolvedColumns 2026-07-27-15:30 (U10 / R8): @@ -675,11 +675,13 @@ before the board-workflows payload resolves and for a column the workflow does n where the traits are unknown rather than known-false. */ function isTaskFieldEditableColumn(column: ColumnId, flags?: TaskContextMenuColumnFlags): boolean { - if (!flags) return EDITABLE_COLUMNS.has(column); - if (flags.complete || flags.archived || flags.countsTowardWip || flags.mergeBlocker || flags.humanReview) { - return false; - } - return flags.intake === true || flags.hold === true; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-00:15 (U12): body moved UNCHANGED to + `isFieldEditableColumnRole`, so this and TaskCard cannot drift apart again. TaskCard implemented the + same affordance with the raw id set and no trait path, which is how a renamed board lost inline + editing on the card while this surface kept it. + */ + return isFieldEditableColumnRole(flags, column); } const GITHUB_TRACKING_EDITABLE_COLUMNS: Set = new Set(["triage", "todo", "in-progress", "in-review", "ideas"]); const CODING_IDEAS_WORKFLOW_ID = "builtin:coding-ideas"; diff --git a/packages/dashboard/app/components/__tests__/TaskCard.test.tsx b/packages/dashboard/app/components/__tests__/TaskCard.test.tsx index 72f2965cf9..f97c66355d 100644 --- a/packages/dashboard/app/components/__tests__/TaskCard.test.tsx +++ b/packages/dashboard/app/components/__tests__/TaskCard.test.tsx @@ -8177,3 +8177,70 @@ describe("TaskCard Start affordance (FN-7596)", () => { await waitFor(() => expect(addToast).toHaveBeenCalledWith("move blocked", "error")); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-00:15 (U12 — the affordance this file never covered): +THE EDIT BUTTON ON A RENAMED BOARD. + +TaskDetailModal resolved field editability from column traits in U10/R8. TaskCard implemented the +same affordance with a hardcoded `{triage, todo}` id set and NO trait path, even though +`taskColumnFlags` was already in scope — so on a board whose pre-implementation column is renamed, +the title was editable in the detail modal and the pencil was absent from the card. + +VERIFIED UNCOVERED rather than assumed: mutating `canEdit` back to the hardcoded set left +`app/components/__tests__/TaskCard*` at exactly the same failure count as the unmutated run, so +nothing caught it. These four assert the real `aria-label`, and that mutation now fails with +"Unable to find an accessible element ... name 'Edit task'". +*/ +describe("TaskCard field editability resolves column traits (U12 — R8)", () => { + const EDIT_LABEL = { name: "Edit task" }; + + it("renders the edit button for a RENAMED pre-implementation column", () => { + render( + , + ); + // Fails with the hardcoded id set: `backlog` is not in it. + expect(screen.getByRole("button", EDIT_LABEL)).toBeInTheDocument(); + }); + + it("does NOT render it for a resolved mid-flight column", () => { + // The narrowing guard: without it the case above passes for a card that always shows the pencil, + // letting an operator rewrite a description while a session executes against it. + render( + , + ); + expect(screen.queryByRole("button", EDIT_LABEL)).not.toBeInTheDocument(); + }); + + it("vetoes editing when a hold column ALSO carries a review trait", () => { + // A legal shape a plain `intake || hold` check gets wrong. + render( + , + ); + expect(screen.queryByRole("button", EDIT_LABEL)).not.toBeInTheDocument(); + }); + + it("still renders it for a legacy `todo` card with no flags resolved", () => { + // The pre-load window, and what every board did before the conversion. + render(); + expect(screen.getByRole("button", EDIT_LABEL)).toBeInTheDocument(); + }); +}); diff --git a/packages/dashboard/app/components/__tests__/column-role-id-invariance.test.tsx b/packages/dashboard/app/components/__tests__/column-role-id-invariance.test.tsx new file mode 100644 index 0000000000..e137b1c1bf --- /dev/null +++ b/packages/dashboard/app/components/__tests__/column-role-id-invariance.test.tsx @@ -0,0 +1,154 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8, completion criterion 3): +EVIDENCE that the converted dashboard surfaces behave on a RENAMED board and on the +MERGED board — stated as an invariance property rather than a pile of per-site cases. + +THE CLAIM THE CONVERSION MAKES. After resolving roles from traits, a surface's behaviour +is a function of the column's TRAITS, not of its id. So the test is: hold the traits +fixed, change only the id, and every decision must be identical. That is one assertion +covering every site at once, and it fails for any site that still consults an id — which +per-site cases cannot promise, because a per-site case only proves the site it names. + +WHY THIS IS NOT A RESTATEMENT OF THE HELPER TESTS. `columnRoles.test.ts` proves the +helpers answer correctly. It cannot prove the CONSUMERS ask them: a component that kept an +inline id comparison passes every helper test. These cases drive the real consumers — +`buildTaskActionMenuModel` and `isTaskAgentActive`, the predicates behind the actions menu, +the pulsing badge, the row border and the column header's executing count — and compare +their outputs across three lineages that differ only in naming. + +THE THREE LINEAGES. + MERGED — post-#2515 default: one pre-implementation column, id `todo`, "Planning". + LEGACY — the pre-merge shape, id `triage`, same traits. + RENAMED — a custom board: `backlog` / `building` / `shipped`, same traits. +An id-sensitive site behaves differently on at least one of the three; a trait-driven one +cannot tell them apart. + +REVERT CHECK, measured. Restoring `task.column !== "triage"` in `shouldShowActionsMenu` +fails the actions-menu invariance case (MERGED and RENAMED start showing a menu that +LEGACY suppresses). Restoring the intake-id comparison in `taskActivity`'s planner-lane +check fails the agent-active case the same way. Both were run. +*/ +import { describe, expect, it, vi } from "vitest"; +import type { Task } from "@fusion/core"; +import { buildTaskActionMenuModel } from "../TaskContextMenu"; +import { isTaskAgentActive } from "../../utils/taskActivity"; + +const t = ((_key: string, fallback?: string) => fallback ?? _key) as never; +const columnLabel = ((column: string) => column) as never; + +/** + * The same PRE-IMPLEMENTATION column expressed three ways. Traits are byte-identical; + * only `id` differs, which is the whole point. + */ +const PRE_IMPLEMENTATION_TRAITS = { intake: true, hold: true } as const; +const LINEAGES = [ + { label: "MERGED (post-#2515 default)", columnId: "todo" }, + { label: "LEGACY (pre-merge)", columnId: "triage" }, + { label: "RENAMED (custom board)", columnId: "backlog" }, +] as const; + +/** A mid-flight column, likewise expressed under three names. */ +const WIP_TRAITS = { countsTowardWip: true } as const; +const WIP_LINEAGES = [ + { label: "MERGED", columnId: "in-progress" }, + { label: "RENAMED", columnId: "building" }, +] as const; + +function mkTask(overrides: Partial & { id: string; column: string }): Task { + return { + title: overrides.id, + description: "Task", + dependencies: [], + steps: [], + currentStep: 0, + status: undefined, + paused: false, + log: [], + createdAt: "2026-07-27T00:00:00.000Z", + updatedAt: "2026-07-27T00:00:00.000Z", + ...overrides, + } as unknown as Task; +} + +/** Every decision the actions menu exposes, as a comparable shape. */ +function menuDecisions(columnId: string, flags: Record, task: Partial = {}) { + const model = buildTaskActionMenuModel({ + task: mkTask({ id: "FN-1", column: columnId, ...task }), + t, + columnLabel, + currentColumnFlags: flags as never, + onPlan: vi.fn(), + } as never); + return { + shouldShowActionsMenu: model.shouldShowActionsMenu, + actionIds: model.actions.map((action: { id: string }) => action.id), + }; +} + +describe("column-role decisions are invariant under column RENAMING (U12 evidence)", () => { + it("the actions menu decides identically on merged, legacy and renamed lineages", () => { + const decisions = LINEAGES.map((lineage) => ({ + label: lineage.label, + ...menuDecisions(lineage.columnId, PRE_IMPLEMENTATION_TRAITS as never), + })); + + /* + Compared against the FIRST lineage rather than a hardcoded expectation: the property + under test is agreement, and hardcoding would quietly bake in whichever shape happened + to be current. If a site consults an id, the entries diverge and the diff names it. + */ + const [first, ...rest] = decisions; + for (const other of rest) { + expect({ ...other, label: first!.label }).toEqual(first); + } + + // And the shape is not vacuous: a pre-implementation card really does offer Plan. + expect(first!.actionIds).toContain("plan"); + }); + + it("a mid-flight card decides identically whether its column is `in-progress` or `building`", () => { + const decisions = WIP_LINEAGES.map((lineage) => ({ + label: WIP_LINEAGES[0]!.label, + ...menuDecisions(lineage.columnId, WIP_TRAITS as never), + })); + expect(decisions[1]).toEqual(decisions[0]); + // The inversion guard, stated positively: executing cards are never planning targets. + expect(decisions[0]!.actionIds).not.toContain("plan"); + }); + + it("planner activity reads as agent-active identically across all three lineages", () => { + /* + `isTaskAgentActive` drives three separate surfaces from one predicate, so an id-sensitive + fallback here reports planning work as idle board-wide — the failure that motivated the + `taskActivity` conversion, and one that throws nothing. + */ + const recent = new Date(Date.now() - 5_000).toISOString(); + const verdicts = LINEAGES.map((lineage) => + isTaskAgentActive( + mkTask({ id: "FN-2", column: lineage.columnId, recentAgentActivityAt: recent } as never), + { columnFlags: PRE_IMPLEMENTATION_TRAITS as never }, + ), + ); + + expect(new Set(verdicts).size).toBe(1); + // Non-vacuous: fresh planner activity on a pre-implementation card IS active. + expect(verdicts[0]).toBe(true); + }); + + it("a stale planner card is inactive on all three lineages, so the invariance is not just `always true`", () => { + /* + Without this, the case above would pass for a predicate hardwired to `true`. Both + verdicts must be unanimous AND opposite each other for the invariance to mean anything. + */ + const stale = new Date(Date.now() - 60 * 60 * 1000).toISOString(); + const verdicts = LINEAGES.map((lineage) => + isTaskAgentActive( + mkTask({ id: "FN-3", column: lineage.columnId, recentAgentActivityAt: stale } as never), + { columnFlags: PRE_IMPLEMENTATION_TRAITS as never }, + ), + ); + + expect(new Set(verdicts).size).toBe(1); + expect(verdicts[0]).toBe(false); + }); +}); diff --git a/packages/dashboard/app/components/__tests__/taskSorting.test.ts b/packages/dashboard/app/components/__tests__/taskSorting.test.ts index fff8221141..8c7f8de398 100644 --- a/packages/dashboard/app/components/__tests__/taskSorting.test.ts +++ b/packages/dashboard/app/components/__tests__/taskSorting.test.ts @@ -139,3 +139,63 @@ describe("sortTasksForDisplayColumn", () => { ]); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): +The hold lane's QUEUE ORDER must follow the hold trait, not the id `todo`. + +Priority-then-FIFO is what makes a high-priority card visibly next in the lane where work +waits for capacity. Keyed on the id it degrades to the generic sort on any board whose +hold column is renamed — the card order is simply wrong, and nothing fails. U11 is what +separated the two questions: `todo` GAINED the hold trait, so "is this column named todo" +and "is this the lane work waits in" stopped being the same test. + +REVERT CHECK, measured: restoring `if (column === "todo")` fails the renamed case below — +the high-priority card comes back second instead of first. +*/ +describe("hold-lane queue order follows the hold TRAIT", () => { + const mk = (id: string, priority: string, createdAt: string, column = "backlog") => + ({ id, title: id, priority, createdAt, column, steps: [], dependencies: [], log: [] } as never); + + /* + THE DISCRIMINATOR IS THE TIEBREAK, not the priority. Both branches sort by priority + first, so a priority difference proves nothing about which branch ran — my first attempt + at this asserted exactly that and passed for the wrong reason. + + What is unique to the hold lane is FIFO: equal priority tiebreaks on `createdAt`, where + the generic sort tiebreaks on task id. So these two carry the same priority and have + createdAt order DISAGREEING with id order — FN-9 is older, FN-2 has the lower id. + hold branch -> FIFO -> FN-9 first + generic branch -> id ascending -> FN-2 first + */ + const older = mk("FN-9", "normal", "2026-07-01T00:00:00.000Z"); + const newer = mk("FN-2", "normal", "2026-07-05T00:00:00.000Z"); + + it("applies FIFO order on a RENAMED hold column when the trait is supplied", () => { + const sorted = sortTasksForDisplayColumn([newer, older], "backlog" as never, undefined, false, true); + expect(sorted.map((task) => task.id)).toEqual(["FN-9", "FN-2"]); + }); + + it("still applies it to the legacy `todo` id by default, for callers that resolve no flags", () => { + // Lane and ListView do not pass flags; the default keeps their behaviour unchanged. + const sorted = sortTasksForDisplayColumn( + [mk("FN-2", "normal", "2026-07-05T00:00:00.000Z", "todo"), mk("FN-9", "normal", "2026-07-01T00:00:00.000Z", "todo")], + "todo" as never, + ); + expect(sorted.map((task) => task.id)).toEqual(["FN-9", "FN-2"]); + }); + + it("does NOT apply FIFO to a non-hold column, which sorts by id", () => { + // The narrowing guard: without it, a function that FIFO-sorted everything would pass + // the cases above while silently reordering the in-progress and review lanes. + const sorted = sortTasksForDisplayColumn([newer, older], "building" as never, undefined, false, false); + expect(sorted.map((task) => task.id)).toEqual(["FN-2", "FN-9"]); + }); + + it("keeps priority ahead of FIFO in the hold lane", () => { + // FIFO is the tiebreak, not the primary key: a newer high-priority card still leads. + const high = mk("FN-3", "high", "2026-07-09T00:00:00.000Z"); + const sorted = sortTasksForDisplayColumn([older, high], "backlog" as never, undefined, false, true); + expect(sorted.map((task) => task.id)).toEqual(["FN-3", "FN-9"]); + }); +}); diff --git a/packages/dashboard/app/components/taskSorting.ts b/packages/dashboard/app/components/taskSorting.ts index 427780b7c9..f23566e6fc 100644 --- a/packages/dashboard/app/components/taskSorting.ts +++ b/packages/dashboard/app/components/taskSorting.ts @@ -54,6 +54,20 @@ export function sortTasksForDisplayColumn( column: Column, doneSortMode: DoneColumnSortMode = "completion-date-desc", isArchivedColumn: boolean = column === "archived", + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + Does this column HOLD planned work waiting for capacity? Follows the same + caller-supplies-the-trait shape `isArchivedColumn` already established, and defaults to + the legacy id so the callers that do not resolve flags (Lane, ListView) keep today's + behaviour exactly. + + The priority-then-FIFO order below is the hold lane's queue order — it is what makes a + high-priority card visibly next. Keyed on the id, it silently degrades to the generic + sort on any board whose hold column is not named `todo`, so a renamed lineage loses + priority ordering with nothing failing. Not a rename: `todo` gained the hold trait in + U11, so the id and the role stopped being the same question. + */ + isHoldColumn: boolean = column === "todo", ): Task[] { /* FNXC:ArchivePagination 2026-07-08-00:00: @@ -70,7 +84,7 @@ export function sortTasksForDisplayColumn( return [...tasks]; } - if (column === "todo") { + if (isHoldColumn) { return [...tasks].sort((a, b) => { const priorityCmp = compareTaskPriority(a.priority, b.priority); if (priorityCmp !== 0) return priorityCmp; diff --git a/packages/dashboard/app/utils/__tests__/worktreeGrouping.test.ts b/packages/dashboard/app/utils/__tests__/worktreeGrouping.test.ts index 51fb8df7a6..6461887833 100644 --- a/packages/dashboard/app/utils/__tests__/worktreeGrouping.test.ts +++ b/packages/dashboard/app/utils/__tests__/worktreeGrouping.test.ts @@ -147,3 +147,100 @@ describe("groupByWorktree", () => { expect(upNext!.queuedTasks).toEqual([queued]); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): +The upcoming-work list must find the HOLD lane by trait, not by the id `todo`. + +WHY THIS ONE HID. On the default board the id and the role coincide — U11 gave `todo` the +hold trait — so every existing case here passed and the site looked healthy. Rename the +hold column and the filter matched nothing: the worktree view showed no upcoming work at +all and read as idle. A whole panel silently empty, nothing thrown. + +Board resolves the hold ids across ALL workflows on the board (a card in another +workflow's hold lane is still upcoming work); Lane passes nothing and keeps the legacy +fallback, which is why the default case below omits the argument entirely. + +REVERT CHECK, measured: dropping the parameter back to `t.column === "todo"` fails the +renamed case with an empty queue. +*/ +describe("upcoming-work queue resolves the hold lane by trait", () => { + const mkTask = (id: string, column: string, extra: Record = {}) => + ({ + id, title: id, description: "", column, dependencies: [], steps: [], currentStep: 0, + log: [], createdAt: "2026-07-01T00:00:00.000Z", updatedAt: "2026-07-01T00:00:00.000Z", + ...extra, + } as never); + + it("finds waiting cards in a RENAMED hold column when the ids are supplied", () => { + const waiting = mkTask("FN-50", "backlog"); + const groups = groupByWorktree([], [waiting], 3, new Set(["FN-50"])); + const queued = groups.flatMap((group) => group.queuedTasks ?? []); + expect(queued.map((task: { id: string }) => task.id)).toContain("FN-50"); + }); + + it("finds nothing in a renamed hold column when the ids are NOT supplied", () => { + /* + Pins the fallback's real limit rather than pretending it covers custom boards: with no + resolved ids the legacy guess is all there is, and it cannot know about `backlog`. This + is the case that used to be the ONLY behaviour, on every board. + */ + const groups = groupByWorktree([], [mkTask("FN-51", "backlog")], 3); + const queued = groups.flatMap((group) => group.queuedTasks ?? []); + expect(queued.map((task: { id: string }) => task.id)).not.toContain("FN-51"); + }); + + it("still finds legacy `todo` cards with no ids supplied, so Lane is unaffected", () => { + const groups = groupByWorktree([], [mkTask("FN-52", "todo")], 3); + const queued = groups.flatMap((group) => group.queuedTasks ?? []); + expect(queued.map((task: { id: string }) => task.id)).toContain("FN-52"); + }); + + it("does not treat a task outside the resolved set as waiting", () => { + /* + The narrowing guard: a lookup that ignored its set would pass the first case. It is keyed + on TASK id, so this also pins the per-workflow scoping — a card whose own workflow does + not mark its column `hold` is absent from the set even if another workflow reuses the id + (PR #2625 review). + */ + const groups = groupByWorktree([], [mkTask("FN-53", "building")], 3, new Set(["FN-99"])); + const queued = groups.flatMap((group) => group.queuedTasks ?? []); + expect(queued.map((task: { id: string }) => task.id)).not.toContain("FN-53"); + }); +}); + +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (PR #2625 review — greptile): +TWO WORKFLOWS, ONE COLUMN NAME, DIFFERENT TRAITS — the case that killed the first design. + +My first version passed a board-wide set of hold COLUMN ids. Column ids are namespaced per +workflow, so workflow A can declare `staging` as its hold lane while workflow B declares +`staging` as executing. A unioned id set answers "is `staging` a hold column?" — a question +with no single answer — and every executing card in workflow B would have been listed under +Up Next as waiting work. Confidently wrong, which is worse than the renamed-board emptiness +the change set out to fix. + +Keying on TASK id moves the decision to the only place that can make it: the caller, which +knows each task's own workflow. This case is the regression test for that, expressed the way +the helper now sees it — one card in the set, one not, both in a column called `staging`. +*/ +describe("hold resolution is scoped per workflow, not per column name", () => { + const mkTask = (id: string, column: string) => + ({ + id, title: id, description: "", column, dependencies: [], steps: [], currentStep: 0, + log: [], createdAt: "2026-07-01T00:00:00.000Z", updatedAt: "2026-07-01T00:00:00.000Z", + } as never); + + it("lists only the card whose OWN workflow marks `staging` as hold", () => { + const waitingInA = mkTask("FN-60", "staging"); + const executingInB = mkTask("FN-61", "staging"); + + // What Board computes: FN-60's workflow declares `staging` hold; FN-61's does not. + const groups = groupByWorktree([], [waitingInA, executingInB], 3, new Set(["FN-60"])); + const queued = groups.flatMap((group) => group.queuedTasks ?? []).map((task: { id: string }) => task.id); + + expect(queued).toContain("FN-60"); + // The assertion the column-id design could not satisfy: same column name, opposite answer. + expect(queued).not.toContain("FN-61"); + }); +}); diff --git a/packages/dashboard/app/utils/columnRoles.ts b/packages/dashboard/app/utils/columnRoles.ts index 0f4b9fb861..b56f5406dd 100644 --- a/packages/dashboard/app/utils/columnRoles.ts +++ b/packages/dashboard/app/utils/columnRoles.ts @@ -63,3 +63,55 @@ export function isPreImplementationColumnRole(flags: ColumnRoleFlags | undefined ? Boolean(flags.intake || flags.hold) : LEGACY_PRE_IMPLEMENTATION_COLUMN_IDS.has(columnId); } + +/** + * Does this column play the HOLD role — a lane a card WAITS in rather than works in? + * + * FNXC:WorkflowResolvedColumns 2026-07-30-23:30 (U12 — worktree upcoming-work list): + * Distinct from {@link isPreImplementationColumnRole}, whose degraded id set is `{todo, triage}`. + * This one degrades to `todo` ALONE, because `triage` was the intake lane and never the + * wait-for-capacity lane — listing an intake card as "upcoming work" in the worktree view would + * report a card that has no plan yet as ready to run. + * + * Same shape, different degraded answer — the asymmetry U11 verified for + * `isPreExecutionHoldColumn` and this file already documents elsewhere. + */ +export function isHoldColumnRole(flags: ColumnRoleFlags | undefined, columnId: string): boolean { + return flags ? flags.hold === true : columnId === "todo"; +} + +/** Legacy pre-implementation id pair, used only when a column has no resolved traits. */ +const LEGACY_FIELD_EDITABLE_COLUMN_IDS: ReadonlySet = new Set(["triage", "todo"]); + +/** + * May a card's title/description be edited in this column? + * + * FNXC:WorkflowResolvedColumns 2026-07-31-00:15 (U12 — one affordance, two components): + * Editing belongs to PRE-IMPLEMENTATION lanes: the card has no session, no worktree, and no plan + * being executed against the text. Any terminal, executing or review trait VETOES it even when + * `intake`/`hold` is also present — a column carrying both is still one where work is underway, and + * rewriting a description out from under a running plan is the failure being prevented. + * + * Extracted because `TaskDetailModal` resolved this from traits (U10/R8) while `TaskCard` kept a + * hardcoded `{triage, todo}` set with NO trait path at all, even though `taskColumnFlags` was already + * in scope there. On a renamed board the operator could edit a task's title in the detail modal but + * the pencil button was absent from its card. One affordance living in two components with one of + * them converted is the FN-6115 -> FN-6118 -> FN-6123 shape; a shared definition is what stops it + * recurring, not a second conversion. + */ +export function isFieldEditableColumnRole( + flags: (ColumnRoleFlags & { + readonly complete?: boolean; + readonly archived?: boolean; + readonly countsTowardWip?: boolean; + readonly mergeBlocker?: boolean; + readonly humanReview?: boolean; + }) | undefined, + columnId: string, +): boolean { + if (!flags) return LEGACY_FIELD_EDITABLE_COLUMN_IDS.has(columnId); + if (flags.complete || flags.archived || flags.countsTowardWip || flags.mergeBlocker || flags.humanReview) { + return false; + } + return flags.intake === true || flags.hold === true; +} diff --git a/packages/dashboard/app/utils/worktreeGrouping.ts b/packages/dashboard/app/utils/worktreeGrouping.ts index 2554c25693..731c10f19b 100644 --- a/packages/dashboard/app/utils/worktreeGrouping.ts +++ b/packages/dashboard/app/utils/worktreeGrouping.ts @@ -1,5 +1,6 @@ import type { Task } from "@fusion/core"; import { getPathBasename } from "./pathDisplay"; +import { isHoldColumnRole } from "./columnRoles"; export interface WorktreeGroupData { label: string; @@ -58,6 +59,21 @@ export function groupByWorktree( inProgressTasks: Task[], allTasks: Task[], maxConcurrent: number, + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The ids of TASKS whose own column is a hold lane in their own workflow, when the caller + resolved them. + + Task ids, not column ids (PR #2625 review — greptile). This helper scans `allTasks`, which + can span workflows, and column ids are namespaced per workflow — two workflows may both + declare `staging` with only one of them marking it `hold`. A unioned column-id set cannot + tell those apart and would list every executing card of the second workflow as waiting. + Only a card's own workflow can answer "is this waiting?", so the caller answers it per task + and passes the result. + + Board resolves this; Lane does not pass it and keeps the legacy-id fallback. + */ + holdTaskIds?: ReadonlySet, ): WorktreeGroupData[] { // Separate assigned vs unassigned in-progress tasks const assigned = inProgressTasks.filter((t) => t.worktree); @@ -72,9 +88,25 @@ export function groupByWorktree( worktreeMap.set(key, list); } - // Find queued todo tasks: "todo" tasks with all deps satisfied (done or in-review) + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The filter wants "cards waiting for capacity" — the HOLD role, resolved from the board's + columns rather than the id `todo`. + + THE BUG THIS CLOSES, measured rather than assumed: on the default board the id and the + role coincide (U11 gave `todo` the hold trait), so this looked healthy. On a board whose + hold column is renamed the filter matched NOTHING, so the worktree view showed no upcoming + work at all and read as idle — a whole panel silently empty, with nothing failing. + + Dependency satisfaction below still names terminal ids. That is a separate question from + the hold role and is left alone deliberately: it needs `complete`/`mergeBlocker`/`archived` + traits for the DEPENDENCY's column, which is another lookup and another unit of work. + */ + // Find queued hold-lane tasks: cards in the hold column with all deps satisfied. const taskById = new Map(allTasks.map((t) => [t.id, t])); - const todoTasks = allTasks.filter((t) => t.column === "todo"); + const isWaitingTask = (task: Task): boolean => + holdTaskIds ? holdTaskIds.has(task.id) : isHoldColumnRole(undefined, task.column); + const todoTasks = allTasks.filter(isWaitingTask); const eligible = todoTasks.filter((t) => !t.paused && (t.dependencies || []).every((depId) => { diff --git a/packages/engine/src/__tests__/move-target-declared-census.test.ts b/packages/engine/src/__tests__/move-target-declared-census.test.ts new file mode 100644 index 0000000000..2069bdcb8b --- /dev/null +++ b/packages/engine/src/__tests__/move-target-declared-census.test.ts @@ -0,0 +1,192 @@ +/* +FNXC:WorkflowColumns 2026-07-30-21:00 (U12 — precondition 2 for flipping the move-path flag): +WHICH ENGINE MOVES WOULD SEAM 2 NEWLY REJECT? + +`moves.ts` gates target-column validation on the retired compatibility flag (see +`packages/core/src/__tests__/moves-workflow-flag-seams.test.ts` for all six seams). With the flag +off there is NO validation: any `moveTask(id, column)` is accepted. Flipping it turns on +unknown-column and adjacency rejections, so a move whose target the task's workflow does not declare +starts FAILING on the path every engine lane uses. That is new refusals, not an equivalence +question, and it is the precondition that has to be measured rather than asserted. + +WHAT THIS MEASURES, and the finding. Every engine `moveTask` call whose target is a string LITERAL, +against the columns the default workflow declares: + + todo 27, in-progress 7, done 6, archived 1 (41 calls) -> all four ARE declared by the default lineage. + +Of those 41, twenty carry no `recoveryRehome` (todo 7, in-progress 7, done 6) and twenty-one do. The +numbers are the AST census's, not a grep's: a line-oriented grep reported todo=29 because it cannot +tell a call from a comment mentioning one, which is exactly the class of error this file exists to +stop repeating. + +So the default board is not the exposure. `triage` appears only inside a comment recording that +`replan-target.ts` "used to hardcode moveTask(id, 'triage')" — a live call would have been the first +thing to break, and there isn't one. + +THE EXPOSURE IS CUSTOM WORKFLOWS. Those same hardcoded legacy ids are not guaranteed to exist in a +custom lineage, so post-flip an engine recovery moving a custom-workflow card to `todo` rejects with +unknown-column unless it carries `recoveryRehome` — the #1411 carve-out that exempts legacy landing +columns precisely so a custom-workflow card can still be rescued. That carve-out is therefore +load-bearing for the flip, and this test pins which call sites depend on it. + +A rejection here is not a crash — it is a recovery that silently stops recovering, which is the +failure class this whole program has been finding. Hence a census with names, not a count. +*/ +import { describe, expect, it } from "vitest"; +import { existsSync, readFileSync, readdirSync, statSync } from "node:fs"; +import { join } from "node:path"; +import ts from "typescript"; +import { getBuiltinWorkflow, parseWorkflowIr } from "@fusion/core"; + +const ENGINE_SRC = join(import.meta.dirname, ".."); + +interface MoveCall { + file: string; + line: number; + target: string; + /** Options-object keys we can see statically, e.g. `recoveryRehome`, `bypassGuards`. */ + optionKeys: string[]; +} + +function collectFiles(dir: string, out: string[]): void { + if (!existsSync(dir)) return; + for (const entry of readdirSync(dir)) { + if (entry === "__tests__" || entry === "node_modules" || entry === "dist") continue; + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + collectFiles(full, out); + continue; + } + if (entry.endsWith(".ts") && !entry.includes(".test.")) out.push(full); + } +} + +/** Rightmost name of a possibly-qualified callee, so `store.moveTask` and `this.store.moveTask` both match. */ +function calleeName(node: ts.Expression): string | undefined { + let cur: ts.Node = node; + while (ts.isPropertyAccessExpression(cur)) cur = cur.name; + return ts.isIdentifier(cur) ? cur.text : undefined; +} + +function literalText(node: ts.Node | undefined): string | undefined { + if (!node) return undefined; + if (ts.isStringLiteral(node) || ts.isNoSubstitutionTemplateLiteral(node)) return node.text; + return undefined; +} + +function collectMoveCalls(): MoveCall[] { + const files: string[] = []; + collectFiles(ENGINE_SRC, files); + // Guards the guard: a broken path would make every assertion below vacuously true. + if (files.length < 20) throw new Error(`census scanned only ${files.length} engine files; path resolution is broken`); + + const calls: MoveCall[] = []; + for (const file of files) { + const source = readFileSync(file, "utf-8"); + const sf = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + /* + `createSourceFile` is error-tolerant, so a syntax error yields a PARTIAL tree whose calls are + simply absent. Reporting that as "no undeclared targets" would report not-inspected as inspected. + */ + const parseErrors = (sf as unknown as { parseDiagnostics?: readonly ts.Diagnostic[] }).parseDiagnostics ?? []; + if (parseErrors.length > 0) { + throw new Error(`census could not parse ${file}: ${ts.flattenDiagnosticMessageText(parseErrors[0]!.messageText, " ")}`); + } + + const visit = (node: ts.Node): void => { + if (ts.isCallExpression(node) && calleeName(node.expression) === "moveTask") { + const target = literalText(node.arguments[1]); + if (target !== undefined) { + const optionsArg = node.arguments[2]; + const optionKeys = + optionsArg && ts.isObjectLiteralExpression(optionsArg) + ? optionsArg.properties + .map((prop) => (prop.name && ts.isIdentifier(prop.name) ? prop.name.text : undefined)) + .filter((name): name is string => name !== undefined) + : []; + calls.push({ + file: file.slice(ENGINE_SRC.length + 1), + line: sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1, + target, + optionKeys, + }); + } + } + ts.forEachChild(node, visit); + }; + visit(sf); + } + return calls; +} + +function defaultColumnIds(): Set { + /* + Read through the builtin registry rather than a private default helper: this is the same lookup + `resolveWorkflowIrById` uses for a builtin id, so the census is judged against the IR the product + actually resolves. + */ + const builtin = getBuiltinWorkflow("builtin:stepwise-coding"); + if (!builtin) throw new Error("census could not resolve the default builtin workflow"); + const resolved = typeof builtin.ir === "string" ? parseWorkflowIr(builtin.ir) : builtin.ir; + return new Set(resolved.columns.map((column: { id: string }) => column.id)); +} + +describe("engine move targets vs the columns a workflow declares (U12 flip precondition)", () => { + it("finds engine moveTask calls with literal targets, so the census is not vacuous", () => { + const calls = collectMoveCalls(); + expect(calls.length).toBeGreaterThan(10); + }); + + it("every literal engine move target IS declared by the DEFAULT workflow", () => { + /* + The reassuring half, and the reason the flip is not immediately fatal: nothing in the engine moves + a card to a column the default lineage lacks. If this fails, a call site was added whose target + the default workflow does not declare, and it will reject the moment the flag flips — on the + default board, for every user. + */ + const declared = defaultColumnIds(); + const undeclared = collectMoveCalls().filter((call) => !declared.has(call.target)); + + expect( + undeclared.map((call) => `${call.file}:${call.line} -> "${call.target}"`), + undeclared.length + ? `These engine moves target a column the DEFAULT workflow does not declare, so seam 2 will\n` + + `reject them once the move-path flag flips:\n` + + undeclared.map((c) => ` ${c.file}:${c.line} "${c.target}" options: ${c.optionKeys.join(", ") || "(none)"}`).join("\n") + : undefined, + ).toEqual([]); + }); + + it("records which call sites depend on the #1411 recoveryRehome carve-out", () => { + /* + THE ACTUAL EXPOSURE. The default board is safe; a CUSTOM lineage need not declare `todo`, + `in-progress`, `done` or `archived` at all. Post-flip, an engine move sending a custom-workflow + card to one of those hardcoded ids rejects with unknown-column UNLESS it carries + `recoveryRehome`, which exempts legacy landing columns so a custom-workflow card can still be + rescued (#1411). + + So this is not "are we safe" — it is the list of moves whose safety RESTS ENTIRELY on that + carve-out, which is the thing the flip PR has to argue about. Asserted as a non-empty inventory + rather than a pass/fail, because the answer is a list a human must read, and a green tick would + hide it. + */ + const calls = collectMoveCalls(); + const rescueMoves = calls.filter((call) => call.optionKeys.includes("recoveryRehome")); + const plainMoves = calls.filter((call) => !call.optionKeys.includes("recoveryRehome")); + + // Both groups exist; if either were empty the distinction below would be meaningless. + expect(rescueMoves.length).toBeGreaterThan(0); + expect(plainMoves.length).toBeGreaterThan(0); + const all = new Map(); + for (const call of calls) all.set(call.target, (all.get(call.target) ?? 0) + 1); + console.info(`ALL literal engine move targets: ${[...all.entries()].sort((a,b)=>b[1]-a[1]).map(([t,n])=>`${t}=${n}`).join(", ")} (total ${calls.length})`); + + const byTarget = new Map(); + for (const call of plainMoves) byTarget.set(call.target, (byTarget.get(call.target) ?? 0) + 1); + console.info( + `engine moves with a literal target, NO recoveryRehome (reject on a custom lineage post-flip):\n` + + [...byTarget.entries()].sort((a, b) => b[1] - a[1]).map(([t, n]) => ` ${String(n).padStart(3)} -> "${t}"`).join("\n") + + `\nwith recoveryRehome (exempt via #1411): ${rescueMoves.length}`, + ); + }); +});