diff --git a/.changeset/u12-resolve-move-path-flag.md b/.changeset/u12-resolve-move-path-flag.md new file mode 100644 index 0000000000..c3ecdf64f1 --- /dev/null +++ b/.changeset/u12-resolve-move-path-flag.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Task moves now validate against the board's own workflow, so cards cannot land in a column it does not declare. +category: fix +dev: Deletes the experimentalFeatures.workflowColumns gate on the move path (6 seams) and its inline legacy branch; column side effects run through default-workflow trait hooks unconditionally. Move rejections now report workflow-resolved targets rather than the legacy adjacency table, which no longer advertises the removed `triage` column. The settings key stays schema-tolerated and hidden for upgraded projects. diff --git a/packages/core/src/__tests__/coding-ideas-move.test.ts b/packages/core/src/__tests__/coding-ideas-move.test.ts index 984dbdcbd1..0410416ed0 100644 --- a/packages/core/src/__tests__/coding-ideas-move.test.ts +++ b/packages/core/src/__tests__/coding-ideas-move.test.ts @@ -114,11 +114,19 @@ pgDescribe("Coding (Ideas) custom-column moves (workflow-columns graduation)", ( await expect( store.moveTask(task.id, "ideas", { moveSource: "user" }), ).rejects.toThrow(/Invalid transition: '.*' → 'ideas'/); - // ...and so is a legacy-but-non-adjacent target, with the verbatim legacy target list. + /* + FNXC:WorkflowColumns 2026-07-31-04:30 (U12 — the move-path flag is resolved): + ...and so is a non-adjacent target. The advertised target list changed from + `in-progress, triage, archived` to `archived, in-progress`, and that is the FIX rather than a + regression: the old list came from the hardcoded legacy adjacency table, which still offered + `triage` — a column the default lineage stopped declaring at #2515. The move path now resolves + adjacency from the task's own workflow, so it can no longer advertise a column that does not + exist. An operator following the old message would have been told to move somewhere impossible. + */ await store.moveTask(task.id, "todo", { moveSource: "user" }); await expect( store.moveTask(task.id, "in-review", { moveSource: "user" }), - ).rejects.toThrow("Invalid transition: 'todo' → 'in-review'. Valid targets: in-progress, triage, archived"); + ).rejects.toThrow("Invalid transition: 'todo' → 'in-review'. Valid targets: archived, in-progress"); }); it("cancels an active task continuation when a user sends implementation back to todo", async () => { diff --git a/packages/core/src/__tests__/live-move-path-undeclared-target.test.ts b/packages/core/src/__tests__/live-move-path-undeclared-target.test.ts index 3ea79476d0..a6e8580008 100644 --- a/packages/core/src/__tests__/live-move-path-undeclared-target.test.ts +++ b/packages/core/src/__tests__/live-move-path-undeclared-target.test.ts @@ -105,15 +105,27 @@ pgDescribe("live move path — which targets it accepts after the Planning merge expect(workflowHasColumn(ir, "triage")).toBe(false); expect(workflowHasColumn(ir, "todo")).toBe(true); - await store.moveTask(task.id, "triage" as never, { moveSource: "user" } as never); + /* + FNXC:WorkflowColumns 2026-07-31-04:30 (U12 — the move-path flag is RESOLVED; this is the fix): + THE DEFECT IS GONE, so this case now asserts the refusal instead of the acceptance, and the + `it.todo` below it — "should REFUSE a move into a column the task's workflow does not declare + (U2b)" — is fulfilled rather than left dangling. - // Today's behavior. The card is now in a column its workflow does not declare, carrying no - // trait flags, invisible to every trait-driven sweep until reconciliation re-homes it. - expect(await column(task.id)).toBe("triage"); + Before: the move was ACCEPTED and the card landed in a column carrying no trait flags, invisible + to every trait-driven sweep until reconciliation re-homed it. Now the move path resolves the + target against the task's own workflow unconditionally, so it is refused at the boundary. + + The premise assertions above are deliberately kept: they prove `triage` really is undeclared, so + this cannot pass for the wrong reason if the default lineage ever declares it again. + */ + await expect( + store.moveTask(task.id, "triage" as never, { moveSource: "user" } as never), + ).rejects.toThrow(/Unknown column for this workflow/); + + // And the card never moved. + expect(await column(task.id)).toBe("todo"); }); - it.todo("should REFUSE a move into a column the task's workflow does not declare (U2b)"); - it("still permits every move the workflow DOES declare", async () => { /* The regression direction that matters most. A guard that refused too much would break the diff --git a/packages/core/src/__tests__/moves-flag-equivalence.test.ts b/packages/core/src/__tests__/moves-flag-equivalence.test.ts index ce1225b9d4..3671ea1226 100644 --- a/packages/core/src/__tests__/moves-flag-equivalence.test.ts +++ b/packages/core/src/__tests__/moves-flag-equivalence.test.ts @@ -1,4 +1,18 @@ /* +FNXC:WorkflowColumns 2026-07-31-04:15 (U12 — THIS FILE'S EQUIVALENCE CASES ARE RETIRED, ON PURPOSE): +The compatibility flag is deleted in this same PR, so the two-flag-states comparison below cannot +run any more — there is only one path now. Its result is preserved in the commit that added it and +in the deletion's own comment: identical persisted rows across 128 fields plus an equal timing shape, +mutation-verified in both directions. That was the evidence the flip needed, and it discharged. + +What SURVIVES here are the seam-2 cases, which never depended on the flag being flippable: a move to +a column the task's workflow does not declare is rejected, and the #1411 `recoveryRehome` carve-out +still lets a stranded custom-workflow card be rescued. Those remain live behaviour worth pinning +after the flip — the carve-out especially, because it is the thing that keeps recovery working on +custom boards and it looks like dead weight to anyone tidying up. + +--- original header, kept for the record --- + FNXC:WorkflowColumns 2026-07-31-02:00 (U12 — precondition 1 for flipping the move-path flag): DO THE TWO COLUMN-SIDE-EFFECT IMPLEMENTATIONS AGREE? @@ -30,133 +44,11 @@ import { pgDescribe, createSharedPgTaskStoreTestHarness } from "../__test-utils_ 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" }); @@ -165,33 +57,6 @@ pgDescribe("move-path side effects are equivalent with the compatibility flag OF 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. @@ -210,7 +75,6 @@ pgDescribe("move-path side effects are equivalent with the compatibility flag OF */ 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({ @@ -237,52 +101,8 @@ pgDescribe("move-path side effects are equivalent with the compatibility flag OF 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", diff --git a/packages/core/src/__tests__/moves-workflow-flag-seams.test.ts b/packages/core/src/__tests__/moves-workflow-flag-seams.test.ts deleted file mode 100644 index f2268d6111..0000000000 --- a/packages/core/src/__tests__/moves-workflow-flag-seams.test.ts +++ /dev/null @@ -1,200 +0,0 @@ -/* -FNXC:WorkflowColumns 2026-07-30-19:00 (U12 — the move-path flag, blast radius pinned): -`moves.ts` still asks the RETIRED question. `useWorkflow` reads -`isWorkflowColumnsCompatibilityFlagEnabled` — the raw compatibility flag that nothing in -production source writes — and it gates the hottest lifecycle path in the system: every task move. - -WHY THIS TEST EXISTS INSTEAD OF A FLIP. The flag looks like one switch and is six. Flipping it does -not "enable workflow columns"; it simultaneously turns on validation, capacity markers, plugin -hooks, and an audit field, and swaps the implementation of every column side effect. Each seam is -enumerated below with what it turns on, and the test FAILS if the count changes — so the next person -to touch this cannot under-scope it the way it has been under-scoped in every summary so far -(including mine: I described it as the 789/837 pair). - -THE RISK IS NOT THE SIDE EFFECTS, IT IS SEAM 2. With the flag off there is NO target-column -validation on the move path at all. Flipping introduces typed rejections — unknown-column, adjacency -— for moves that succeed today. That is not an equivalence question, it is new refusals on the path -every engine lane uses, and it is why "the suite is green after the flip" is not evidence. - -WHAT WOULD MAKE THE FLIP SAFE, recorded so the obligation survives this session: - 1. An equivalence proof for seam 3, comparing the inline legacy side effects against the trait - hooks for timing, reset-on-entry, abort-on-exit and merge.onEnter. The two implementations have - NEVER both run in production, so neither is the observed baseline. - 2. A census of moves that seam 2 would newly reject — every engine caller that moves a card to a - column its workflow does not declare. `recoveryRehome` already carves out legacy targets - (#1411); nothing proves the other callers are covered. - 3. Both raw-flag readers flipped ATOMICALLY. `workflow-task-create-ops.ts` computes the - `movePolicyPreflight` that `moves.ts` consumes, so un-gating either alone starts evaluating - workflow move policies — with their plugin-gate side effects — while the consumer stays off. - Pinned separately by `raw-workflow-columns-flag-census.test.ts`. - -This test asserts (1) the seam count, (2) that every seam reads the SAME flag rather than drifting -onto separate conditions, and (3) that the flag-off branch is still present and inline — because the -agreed sequencing is to DELETE it with the branch rather than convert its guards, and a deletion -needs to know the branch is still there. -*/ -import { describe, expect, it } from "vitest"; -import { readFileSync } from "node:fs"; -import { join } from "node:path"; -import ts from "typescript"; - -const MOVES_PATH = join(import.meta.dirname, "..", "task-store", "moves.ts"); - -/** - * The six decision points, measured on `main` at 2026-07-30. `line` is documentation only — the - * assertions below are position-independent so ordinary edits above a seam do not fail this test. - */ -const SEAMS: ReadonlyArray<{ line: number; turnsOn: string }> = [ - { line: 392, turnsOn: "resolves the task's workflow IR (undefined when off, so every IR-dependent guard below is inert)" }, - { line: 489, turnsOn: "typed REJECTIONS: unknown-column and adjacency validation. Off = no target validation at all — the riskiest seam" }, - { line: 789, turnsOn: "column side effects route through the default-workflow TRAIT HOOKS instead of the inline legacy block (timing, reset-on-entry, abort-on-exit, merge.onEnter)" }, - { line: 1092, turnsOn: "writes the transition-pending marker, which capacity counting reads — load-bearing for the in-transaction capacity gate" }, - { line: 1330, turnsOn: "runs PLUGIN hooks on column change (skipped for engine/recovery moves and same-column no-ops)" }, - { line: 1395, turnsOn: "records `workflowId` on the emitted move payload" }, -]; - -/** Parse `moves.ts` once. Comments are absent from the tree, which is the whole point. */ -function parseMoves(): ts.SourceFile { - const source = readFileSync(MOVES_PATH, "utf-8"); - const sf = ts.createSourceFile(MOVES_PATH, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); - - /* - `parseDiagnostics` is not on the PUBLIC `SourceFile` type, so it needs a cast. It is still the - right signal rather than a try/catch: `createSourceFile` is error-tolerant and returns diagnostics - instead of throwing, which is what makes a catch-based check unreachable. - */ - const parseErrors = (sf as unknown as { parseDiagnostics?: readonly ts.Diagnostic[] }).parseDiagnostics ?? []; - if (parseErrors.length > 0) { - /* - Fail loudly rather than counting a partial tree. `ts.createSourceFile` is error-TOLERANT — it - returns diagnostics instead of throwing — so a syntax error would otherwise yield a smaller - count and read as "seams were removed", which is the opposite of the truth. - */ - throw new Error(`could not parse moves.ts: ${ts.flattenDiagnosticMessageText(parseErrors[0]!.messageText, " ")}`); - } - return sf; -} - -/** Every `useWorkflow` reference, declaration excluded, so the number is "reads". */ -function useWorkflowReferences(): number { - const sf = parseMoves(); - let count = 0; - const visit = (node: ts.Node): void => { - // Identifier references only: the declaration itself is excluded so the number is "reads". - if (ts.isIdentifier(node) && node.text === "useWorkflow") { - const isDeclarationName = node.parent && ts.isVariableDeclaration(node.parent) && node.parent.name === node; - if (!isDeclarationName) count++; - } - ts.forEachChild(node, visit); - }; - visit(sf); - return count; -} - -describe("the move-path workflow flag (U12's actual completion criterion)", () => { - it("still gates the move path at SIX seams, not one", () => { - /* - If this fails LOW, seams were removed — either the flip landed (then delete this test with the - flag) or someone narrowed the gate without accounting for what stopped happening. If it fails - HIGH, a seventh behaviour was hung off a retired flag, which means it has never run. - */ - expect(useWorkflowReferences()).toBe(SEAMS.length); - }); - - it("documents what each seam turns on, so the flip cannot be under-scoped", () => { - // Cheap, but it forces the next person to state the effect when they add or remove a seam. - expect(SEAMS).toHaveLength(6); - for (const seam of SEAMS) { - expect(seam.turnsOn.length).toBeGreaterThan(40); - } - }); - - it("reads ONE flag, so the six seams cannot drift apart", () => { - /* - The property that makes a single flip coherent. If a seam were rewritten to consult the settings - object directly, the flip would move five behaviours and leave one behind — and nothing else in - the suite would notice, because both states are individually valid. - */ - /* - FNXC:WorkflowColumns 2026-07-30-22:00 (PR #2639 review — greptile, and it is the same defect one - level up): these were `toContain` substring checks against raw source, so they matched text in a - comment or a dead branch just as happily as real code. A test about structural drift that cannot - see structure is the exact failure this PR is documenting. Asserted on the AST now. - */ - const sf = parseMoves(); - const declarations: ts.VariableDeclaration[] = []; - const findDeclarations = (node: ts.Node): void => { - if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name) && node.name.text === "useWorkflow") { - declarations.push(node); - } - ts.forEachChild(node, findDeclarations); - }; - findDeclarations(sf); - - expect(declarations).toHaveLength(1); - // And it is initialized from the raw compatibility-flag reader, not something else. - const initializer = declarations[0]!.initializer; - expect(initializer && ts.isCallExpression(initializer)).toBe(true); - const callee = (initializer as ts.CallExpression).expression; - expect(ts.isIdentifier(callee) ? callee.text : undefined).toBe("isWorkflowColumnsCompatibilityFlagEnabled"); - }); - - it("still has the inline flag-OFF branch that the agreed sequencing DELETES", () => { - /* - The sequencing agreed with the coordinator is: U12 resolves the flag first, then the flag-off - branch is deleted wholesale rather than having its guards converted — converting code we intend - to delete is waste and leaves a second definition alive to drift. - - This asserts the branch is still present, so if someone converts its lifecycle guards instead, - the deletion step has a test naming the plan. It is deliberately a source assertion: the branch's - behaviour is what the equivalence proof in seam 3 must cover, and that proof does not exist yet. - */ - /* - Structural, not a comment match (PR #2639 review). The previous assertion looked for the string - "Flag-OFF legacy inline side effects" — which is a COMMENT. Deleting the entire legacy branch - while leaving its header comment in place would have passed, and the comment is exactly the kind - of prose this program deliberately keeps after deleting code. - - What actually matters is that some `if (useWorkflow)` still has an ELSE: that else IS the legacy - inline path, and its existence is what the delete-with-the-branch sequencing depends on. - */ - const sf = parseMoves(); - let seamsWithElse = 0; - const findIfElse = (node: ts.Node): void => { - if ( - ts.isIfStatement(node) && - ts.isIdentifier(node.expression) && - node.expression.text === "useWorkflow" && - node.elseStatement !== undefined - ) { - seamsWithElse++; - } - ts.forEachChild(node, findIfElse); - }; - findIfElse(sf); - expect(seamsWithElse).toBeGreaterThan(0); - }); - - it("names the second reader that must flip atomically with this one", () => { - /* - Recorded here because it is the constraint most likely to be forgotten: the preflight in - `workflow-task-create-ops.ts` is computed under the same flag and CONSUMED by moves.ts. Flipping - one without the other either evaluates workflow move policies whose result is ignored, or - validates against a preflight that was never computed. - */ - /* - An IDENTIFIER reference, not a substring (PR #2639 review): this symbol is discussed by name in - the comments around the preflight, so a text match proved nothing about whether the code still - consumes it — and "moves.ts consumes the preflight" is the entire reason the two readers must - flip together. - */ - const sf = parseMoves(); - let references = 0; - const findReferences = (node: ts.Node): void => { - if (ts.isIdentifier(node) && node.text === "movePolicyPreflight") references++; - ts.forEachChild(node, findReferences); - }; - findReferences(sf); - expect(references).toBeGreaterThan(0); - }); -}); diff --git a/packages/core/src/__tests__/optionless-move-does-not-bypass-guards.test.ts b/packages/core/src/__tests__/optionless-move-does-not-bypass-guards.test.ts new file mode 100644 index 0000000000..28af3a15dd --- /dev/null +++ b/packages/core/src/__tests__/optionless-move-does-not-bypass-guards.test.ts @@ -0,0 +1,64 @@ +/* +FNXC:TaskMovement 2026-07-31-11:30 (PR #2655 review — the contract that was only a comment): +AN OPTIONLESS `moveTask(id, toColumn)` MUST NOT INHERIT GUARD BYPASS. + +`moves.ts` resolves `moveSource = options?.moveSource ?? "engine"` for the EMITTED source, while +`resolveWorkflowBypassGuardsImpl` reads `options?.moveSource` — so an optionless call is reported as +engine-sourced but does not skip workflow guards, plugin gates or the merge blocker. That asymmetry +is deliberate and was documented at `moves.ts` in a comment. It was not tested. + +WHY IT NEEDED A TEST. The `void moveSource;` beside that read looks exactly like someone silencing an +unused-parameter lint, so I "fixed" it to use the resolved value — which handed guard bypass to the +eight optionless `moveTask` calls in dashboard HTTP routes (reset, rebound, respecify, unassign). It +took two rounds of review to get back to the shipped behaviour. A comment could not stop that; this +can. + +The ambiguity underneath is real and unresolved: 18 optionless calls in `packages/engine` are genuine +engine moves that arguably SHOULD bypass, and 8 in `packages/dashboard` are operator-initiated and +must not. The fix is to make `moveSource` explicit at those 26 sites so the default is never guessed. +Until then, this pins the safe half — a public caller never silently acquires privilege — because +that is the direction whose failure is a security problem rather than an inconvenience. +*/ +import { describe, expect, it } from "vitest"; +import { resolveWorkflowBypassGuardsImpl } from "../task-store/task-store-helpers.js"; +import type { TaskStore, MoveTaskOptions } from "../store.js"; + +/** The helper ignores the store; it is a pure policy decision over (moveSource, options). */ +const STORE = {} as unknown as TaskStore; + +describe("optionless moveTask does not inherit guard bypass", () => { + it("does NOT bypass when no options are supplied", () => { + /* + The call site resolves `moveSource` to "engine" for the emitted event. Passing that resolved value + here is exactly the mistake this test exists to prevent: it must not be enough on its own. + */ + expect(resolveWorkflowBypassGuardsImpl(STORE, "engine", undefined)).toBe(false); + }); + + it("does NOT bypass when options are supplied without an explicit source", () => { + // A caller that passes unrelated options is still not declaring itself engine-sourced. + expect(resolveWorkflowBypassGuardsImpl(STORE, "engine", { preserveProgress: true } as MoveTaskOptions)).toBe(false); + }); + + it("DOES bypass when a call site opts in explicitly", () => { + /* + The other half — without this the helper could return false unconditionally and still pass. Engine, + scheduler, handoff and recovery sites opt in by naming their source or asking for it directly. + */ + expect(resolveWorkflowBypassGuardsImpl(STORE, "engine", { moveSource: "engine" } as MoveTaskOptions)).toBe(true); + expect(resolveWorkflowBypassGuardsImpl(STORE, "engine", { moveSource: "scheduler" } as MoveTaskOptions)).toBe(true); + expect(resolveWorkflowBypassGuardsImpl(STORE, "user", { skipMergeBlocker: true } as MoveTaskOptions)).toBe(true); + expect(resolveWorkflowBypassGuardsImpl(STORE, "user", { recoveryRehome: true } as MoveTaskOptions)).toBe(true); + }); + + it("honours an explicit bypassGuards=false even from an engine source", () => { + // `??` on the option means an explicit false wins over the source-derived default. + expect( + resolveWorkflowBypassGuardsImpl(STORE, "engine", { moveSource: "engine", bypassGuards: false } as MoveTaskOptions), + ).toBe(false); + }); + + it("does NOT bypass for a user-sourced move", () => { + expect(resolveWorkflowBypassGuardsImpl(STORE, "user", { moveSource: "user" } as MoveTaskOptions)).toBe(false); + }); +}); diff --git a/packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts b/packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts deleted file mode 100644 index 408b7f9a4e..0000000000 --- a/packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts +++ /dev/null @@ -1,518 +0,0 @@ -/* -FNXC:MovePathConvergence 2026-07-27-18:30 (Phase A2 — workflow-owned lifecycle): -DIFFERENTIAL CHARACTERIZATION of the two move-side-effect implementations in -`moveTaskInternalImpl`, run through ONE shared fixture. - -WHY THIS SUITE EXISTS. `moves.ts` branches on `useWorkflow` = -`isWorkflowColumnsCompatibilityFlagEnabled(settings)`, which reads the RAW -`experimentalFeatures.workflowColumns` key. Nothing in production writes that -key, so: - - - the INLINE branch is the LIVE path for essentially every project; - - `default-workflow-hooks.ts` (the trait-hook path) is DEAD. - -`moves.ts` says so itself at the flag-OFF adjacency branch. Only one -implementation runs in production, so equivalence CANNOT be observed by running -the suite normally — the dead path is never entered. Each case here therefore -FORCES both paths explicitly through the same fixture: - - flag ABSENT → the production shape (inline branch) - workflowColumns: true → the hooks path - -and asserts on observable state, not on which functions were called. Equivalence -asserted by reading code is not equivalence. - -SCOPE NOTE. The `useWorkflow` flag gates far more than the side-effect block: -validation, in-transaction capacity, the transitionPending marker, and plugin -column gates are all inside it. Those divergences are characterized in the -companion describe at the bottom, which is what makes the convergence decision an -operator call rather than a refactor. -*/ - -import { afterEach, beforeEach, expect, it, beforeAll, afterAll } from "vitest"; -import { - pgDescribe, - createSharedPgTaskStoreTestHarness, - type SharedPgTaskStoreHarness, -} from "../../__test-utils__/pg-test-harness.js"; -import type { Task } from "../../types.js"; - -const pgTest = pgDescribe; - -/** The observable surface a move can change. Compared field-by-field so a - * divergence names the field rather than dumping two task objects. */ -interface MoveObservation { - column: string; - status: string | undefined; - error: string | undefined; - paused: boolean | undefined; - userPaused: boolean | undefined; - pausedReason: string | undefined; - blockedBy: string | undefined; - overlapBlockedBy: string | undefined; - worktree: string | undefined; - branch: string | undefined; - summary: string | undefined; - baseCommitSha: string | undefined; - /* - Timestamps are compared by PRESENCE, not value. The two paths necessarily run - at different wall-clock instants (same fixture, two sequential runs), so the - ISO strings differ by milliseconds every time. What must match is whether the - path stamped the field at all — a path that forgets to set - `executionCompletedAt`, or wrongly clears `firstExecutionAt`, still fails here. - */ - executionStartedAtSet: boolean; - executionCompletedAtSet: boolean; - firstExecutionAtSet: boolean; - /* - FNXC:MovePathEquivalence 2026-07-27-08:20 (PR #2468 review — greptile P2): - A boolean "is it a number" stays green when the two paths compute DIFFERENT durations, which is - exactly the accounting bug this case exists to catch. The absolute value is wall-clock dependent, - so compare a QUANTISED bucket: both paths must land in the same 100ms bucket for the same seeded - segment, which is stable against scheduling jitter while still failing when one path drops or - double-counts a segment. - */ - cumulativeActiveMsBucket: number | undefined; - recoveryRetryCount: number | undefined; - nextRecoveryAtSet: boolean; - stepStatuses: string[]; - workflowStepResultCount: number | undefined; -} - -function observe(task: Task): MoveObservation { - return { - column: task.column, - status: task.status, - error: task.error, - paused: task.paused, - userPaused: task.userPaused, - pausedReason: task.pausedReason, - blockedBy: task.blockedBy, - overlapBlockedBy: task.overlapBlockedBy, - worktree: task.worktree, - branch: task.branch, - summary: task.summary, - baseCommitSha: task.baseCommitSha, - executionStartedAtSet: task.executionStartedAt !== undefined, - executionCompletedAtSet: task.executionCompletedAt !== undefined, - firstExecutionAtSet: task.firstExecutionAt !== undefined, - cumulativeActiveMsBucket: - typeof task.cumulativeActiveMs === "number" ? Math.floor(task.cumulativeActiveMs / 100) : undefined, - recoveryRetryCount: task.recoveryRetryCount, - nextRecoveryAtSet: task.nextRecoveryAt !== undefined, - stepStatuses: (task.steps ?? []).map((s) => s.status), - workflowStepResultCount: task.workflowStepResults?.length, - }; -} - -pgTest("move-path equivalence — side effects (Phase A2)", () => { - const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ - prefix: "fusion_move_equiv", - }); - - beforeAll(h.beforeAll); - beforeEach(h.beforeEach); - afterEach(h.afterEach); - afterAll(h.afterAll); - - /* - Force the path. Absent key = production shape (inline); `true` = hooks. - - MUST be updateGlobalSettings, NOT updateSettings. `experimentalFeatures` is a - GLOBAL-scoped key, and `moves.ts` reads it through `getSettingsFast()` (merged - global + project) for exactly that reason. Writing it via the project store is - silently accepted and never reaches `useWorkflow` — the first version of this - suite did that and reported NINE passing "equivalence" cases while running the - inline path twice. Hence `assertPathActive` below: a forcing mechanism that can - fail silently makes every assertion downstream worthless. - */ - async function setPath(path: "inline" | "hooks"): Promise { - const store = h.store(); - await store.updateGlobalSettings( - path === "hooks" - ? { experimentalFeatures: { workflowColumns: true } } - // Explicit `false`, NOT `{}`: updateGlobalSettings MERGES, so an empty - // object leaves a previously-set `true` in place and the next case runs - // the hooks path while believing it is on inline. `assertPathActive` - // caught exactly that leak across seven cases. - : { experimentalFeatures: { workflowColumns: false } }, - ); - await assertPathActive(path); - } - - /* - Prove the flip took effect, using a behavior only ONE path produces: an - undeclared target column. The inline path validates against the legacy - `VALID_TRANSITIONS` map and reports "Valid targets: …"; the flag-ON path - validates against the task's workflow and reports "Unknown column for this - workflow." A path-selection regression therefore fails HERE, loudly, instead of - turning every equivalence case into a tautology. - */ - async function assertPathActive(path: "inline" | "hooks"): Promise { - const store = h.store(); - const probe = await store.createTask({ description: `path probe ${path}` }); - const err = await store - .moveTask(probe.id, "not-a-column-any-workflow-declares") - .then(() => null, (e: unknown) => e as Error); - expect(err, `${path}: probe move should have been rejected`).toBeInstanceOf(Error); - if (path === "hooks") { - expect(err!.message, "hooks path not active — check updateGlobalSettings").toContain( - "Unknown column for this workflow", - ); - } else { - expect(err!.message, "inline path not active").toContain("Valid targets:"); - } - await store.deleteTask(probe.id); - } - - /** - * Run `scenario` once per path over a FRESH task each time, and return both - * observations. The scenario receives the task id so it can drive whatever - * move sequence the case needs. - */ - async function bothPaths( - seed: () => Promise, - scenario: (taskId: string) => Promise, - ): Promise<{ inline: MoveObservation; hooks: MoveObservation }> { - const store = h.store(); - - await setPath("inline"); - const inlineId = await seed(); - await scenario(inlineId); - const inline = observe((await store.getTask(inlineId))!); - - await setPath("hooks"); - const hooksId = await seed(); - await scenario(hooksId); - const hooks = observe((await store.getTask(hooksId))!); - - return { inline, hooks }; - } - - /** A task parked in `in-progress` with timing + progress state on it. */ - async function seedInProgress(): Promise { - const store = h.store(); - const task = await store.createTask({ description: "equivalence fixture" }); - await store.moveTask(task.id, "todo"); - await store.moveTask(task.id, "in-progress"); - return task.id; - } - - it("EQUIVALENCE: in-progress → todo reopen (user) clears the same fields on both paths", async () => { - const store = h.store(); - const { inline, hooks } = await bothPaths(seedInProgress, async (id) => { - await store.updateTask(id, { status: "failed", error: "boom", blockedBy: "FN-9" }); - await store.moveTask(id, "todo", { moveSource: "user" }); - }); - - // The whole point: field-by-field, not "it moved". - expect(hooks).toEqual(inline); - // Pin the behavior itself so a change that breaks BOTH paths identically - // still fails here (equal-but-wrong is not equivalence worth having). - expect(inline.status).toBeUndefined(); - expect(inline.error).toBeUndefined(); - expect(inline.blockedBy).toBeUndefined(); - expect(inline.userPaused).toBe(true); // user-source reopen to todo parks - }); - - it("EQUIVALENCE: engine-source reopen does NOT set userPaused on either path", async () => { - const store = h.store(); - const { inline, hooks } = await bothPaths(seedInProgress, async (id) => { - await store.moveTask(id, "todo", { moveSource: "engine" }); - }); - expect(hooks).toEqual(inline); - expect(inline.userPaused).toBeUndefined(); - }); - - it("EQUIVALENCE: preserveStatus keeps status/error on both paths", async () => { - const store = h.store(); - const { inline, hooks } = await bothPaths(seedInProgress, async (id) => { - await store.updateTask(id, { status: "failed", error: "branch conflict" }); - await store.moveTask(id, "todo", { preserveStatus: true }); - }); - expect(hooks).toEqual(inline); - expect(inline.status).toBe("failed"); - expect(inline.error).toBe("branch conflict"); - }); - - it("EQUIVALENCE: preservePause keeps an operator park through a teardown move (FN-7851)", async () => { - const store = h.store(); - const { inline, hooks } = await bothPaths(seedInProgress, async (id) => { - await store.updateTask(id, { paused: true, pausedReason: "operator" }); - await store.moveTask(id, "todo", { moveSource: "engine", preservePause: true }); - }); - expect(hooks).toEqual(inline); - expect(inline.paused).toBe(true); - expect(inline.pausedReason).toBe("operator"); - }); - - it("EQUIVALENCE: timing accounting runs identically on in-progress exit and re-entry", async () => { - const store = h.store(); - const { inline, hooks } = await bothPaths(seedInProgress, async (id) => { - await store.moveTask(id, "todo", { moveSource: "engine" }); - await store.moveTask(id, "in-progress"); - }); - expect(hooks).toEqual(inline); - // Both paths accounted the SAME segment, not merely "some number". - expect(inline.cumulativeActiveMsBucket).toBeTypeOf("number"); - expect(hooks.cumulativeActiveMsBucket).toBe(inline.cumulativeActiveMsBucket); - expect(inline.firstExecutionAtSet).toBe(true); - expect(inline.executionStartedAtSet).toBe(true); - }); - - /* - FNXC:MovePathEquivalence 2026-07-27-08:20 (PR #2468 review — greptile P2): - Seed NON-DEFAULT progress before the move. With default steps, both paths observe the same empty - progress and the case passes without characterising `preserveResumeState` at all — two paths that - both wiped progress would agree just as happily. Seeding a mixed done/in-progress/pending shape - plus a workflow-step result means equality now asserts that the progress SURVIVED, and the - explicit post-conditions fail loudly if either path resets it. - */ - it("EQUIVALENCE: preserveProgress/preserveResumeState keep step progress on both paths", async () => { - const store = h.store(); - const { inline, hooks } = await bothPaths(seedInProgress, async (id) => { - await store.updateTask(id, { - steps: [ - { name: "Step 0", status: "done" }, - { name: "Step 1", status: "in-progress" }, - { name: "Step 2", status: "pending" }, - ], - currentStep: 1, - workflowStepResults: [ - { - workflowStepId: "plan-review", - workflowStepName: "Plan Review", - status: "passed", - source: "node", - phase: "pre-merge", - }, - ], - } as never); - await store.moveTask(id, "todo", { moveSource: "engine", preserveResumeState: true }); - }); - expect(hooks).toEqual(inline); - // Post-conditions, so "equal" cannot mean "both wiped it". - expect(inline.stepStatuses).toEqual(["done", "in-progress", "pending"]); - expect(inline.workflowStepResultCount).toBe(1); - }); - - it("EQUIVALENCE: preserveWorktree keeps the worktree on both paths", async () => { - const store = h.store(); - const { inline, hooks } = await bothPaths(seedInProgress, async (id) => { - await store.updateTask(id, { worktree: "/tmp/wt/FN-X" }); - await store.moveTask(id, "todo", { moveSource: "engine", preserveWorktree: true }); - }); - expect(hooks).toEqual(inline); - expect(inline.worktree).toBe("/tmp/wt/FN-X"); - }); - - it("EQUIVALENCE: worktree is CLEARED by default on reopen on both paths", async () => { - const store = h.store(); - const { inline, hooks } = await bothPaths(seedInProgress, async (id) => { - await store.updateTask(id, { worktree: "/tmp/wt/FN-Y" }); - await store.moveTask(id, "todo", { moveSource: "engine" }); - }); - expect(hooks).toEqual(inline); - expect(inline.worktree).toBeUndefined(); - }); -}); - -pgTest("move-path equivalence — the flag gates MORE than side effects (Phase A2)", () => { - const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ - prefix: "fusion_move_diverge", - }); - - beforeAll(h.beforeAll); - beforeEach(h.beforeEach); - afterEach(h.afterEach); - afterAll(h.afterAll); - - async function setPath(path: "inline" | "hooks"): Promise { - // See the note on the sibling setPath: GLOBAL settings, not project. - await h.store().updateGlobalSettings( - path === "hooks" - ? { experimentalFeatures: { workflowColumns: true } } - : { experimentalFeatures: { workflowColumns: false } }, - ); - } - - /* - These are NOT equivalence assertions — they are the measured divergences that - make convergence an operator decision rather than a mechanical refactor. Each - one is a behavior that would turn ON for every project the moment the hooks - path becomes authoritative. - */ - - /* - FNXC:MovePathConvergence 2026-07-30-22:10 (U2b — second measured divergence, and it is not a - message shape): - - U11 removed `triage` from the default coding lineage, so rows left there sit in a column their own - workflow no longer declares. #2515 added an escape hatch to `resolveAllowedColumns` so such a card - has a legal move (its workflow's rebound target) instead of "Valid targets: none". - - That hatch is INSIDE the `useWorkflow` block, so it only runs on the hooks path. Mutation-verified - in #2597: stubbing it back to `[]` left an operator-move test green, because the inline path - answers from the legacy VALID_TRANSITIONS map instead — whose `triage` row happens to permit the - move for unrelated reasons. - - WHY THIS ONE MATTERS MORE THAN THE MESSAGE DIVERGENCE ABOVE. It is not a shape difference in a - rejection; it is a difference in WHICH MOVES ARE LEGAL, and it is workflow-dependent. Flipping - `useWorkflow` on changes move VALIDATION for every stranded card, not just side-effect routing — - so "make the flag always-on, then delete the flag-OFF branch" is not the mechanical cleanup it - looks like. Recorded here so the flip is an operator decision with the behaviour change visible, - which is what U2b exists to make possible. - */ - it("DIVERGENCE: legal targets for a card in an UNDECLARED column come from different sources", async () => { - const store = h.store(); - - const targetsFor = async (path: "inline" | "hooks", id: string): Promise => { - await setPath(path); - // Ask for a target NO workflow declares, and read the reported legal set out of the - // rejection. Both paths reject; the question is what each one considers legal. - const err = await store - .moveTask(id, "not-a-column-any-workflow-declares") - .then(() => null, (e: unknown) => e as Error); - const match = /Valid targets: (.*)$/.exec(err?.message ?? ""); - return match ? match[1]!.split(",").map((t) => t.trim()).filter(Boolean) : []; - }; - - await setPath("inline"); - const inlineTask = await store.createTask({ description: "undeclared-source inline" }); - const inlineTargets = await targetsFor("inline", inlineTask.id); - - /* - FNXC:MovePathConvergence 2026-07-31-10:45 (PR #2638 review — greptile P2): - The HOOKS path with a card genuinely STRANDED, which the first version never exercised: it - created the card in a declared column and only ran the inline path, so a regression in the hooks - rebound resolution would not have failed anything. - - Stranding needs a direct row write — `moveTask` refuses to take a card into an undeclared column, - which is the transition policy working. The corrupt post-upgrade state IS the fixture, and it is - the state U11 leaves behind for every row still sitting in `triage`. - */ - await setPath("hooks"); - const strandedTask = await store.createTask({ description: "undeclared-source hooks" }); - await h.adminSql()`UPDATE project.tasks SET "column" = 'a-column-no-workflow-declares' WHERE id = ${strandedTask.id}`; - store.taskCache.delete(strandedTask.id); - - const hooksErr = await store - .moveTask(strandedTask.id, "in-progress") - .then(() => null, (e: unknown) => e as Error); - - /* - On the hooks path the SOURCE column is undeclared, so `resolveAllowedColumns` has no adjacency to - read and U11's escape hatch supplies the workflow's rebound target instead. Whatever the outcome, - it is reached through workflow resolution — asserted as "not the legacy VALID_TRANSITIONS answer", - because the legacy table is what the inline path consults and the two must be distinguishable. - - Deliberately not asserting a specific message: the point is that the two paths answer from - DIFFERENT SOURCES for the same stranded card. Pinning hooks' exact wording here would duplicate - the rejection-shape divergence above and make this case fail for the wrong reason. - */ - expect(hooksErr === null || !/Valid targets: in-progress, triage, archived/.test(hooksErr.message)).toBe(true); - - /* - The inline path enumerates the LEGACY table, so it reports legacy ids regardless of what the - task's workflow declares. The hooks path does not report a target list at all for an unknown - column — it throws the typed unknown-column rejection first, asserted above. - - Asserted as a POSITIVE about the inline path rather than a comparison of two lists, because the - two paths do not even reach the same rejection: that asymmetry IS the divergence, and a - comparison would hide it behind two empty arrays. - */ - expect(inlineTargets.length).toBeGreaterThan(0); - expect(inlineTargets).toContain("in-progress"); - }); - - it("DIVERGENCE: rejection TYPE and MESSAGE differ — the legacy bare-Error contract is inline-only", async () => { - const store = h.store(); - - await setPath("inline"); - const a = await store.createTask({ description: "reject shape inline" }); - const inlineErr = await store - .moveTask(a.id, "not-a-column-any-workflow-declares") - .then(() => null, (e: unknown) => e as Error); - - await setPath("hooks"); - const b = await store.createTask({ description: "reject shape hooks" }); - const hooksErr = await store - .moveTask(b.id, "not-a-column-any-workflow-declares") - .then(() => null, (e: unknown) => e as Error); - - // Both reject — but not with the same type or the same message. The inline - // path validates against the legacy VALID_TRANSITIONS map and throws a BARE - // Error; the hooks path validates against the task's own workflow and throws - // a typed TransitionRejectionError carrying a machine-readable `rejection`. - expect(inlineErr).toBeInstanceOf(Error); - expect(hooksErr).toBeInstanceOf(Error); - expect((inlineErr as unknown as { rejection?: unknown }).rejection).toBeUndefined(); - expect((hooksErr as unknown as { rejection?: unknown }).rejection).toBeDefined(); - expect(inlineErr!.message).toContain("Valid targets:"); - expect(hooksErr!.message).toContain("Unknown column for this workflow"); - }); - - it("CONVERGED: in-transaction capacity now rejects on BOTH paths", async () => { - /* - FNXC:WorkflowCapacity 2026-07-28-19:40 (pool-id sentinel fix): - WAS `UNPROVEN: … did NOT reject on EITHER path`. That test recorded an honest - negative result and left the cause open: "something further in - (`resolveColumnCapacity`'s limit resolution, or what - `countActiveInCapacitySlotAsync` counts as an occupant) keeps the check from - firing even when the flag is on. This suite does not establish which." It - also predicted its own obsolescence: "if a future change makes this reject, - that is the capacity gate coming alive." - - THE ANSWER, established by the sentinel fix: neither of those guesses. The - counter and the limit resolution were both fine. `moves.ts` asked the counter - for occupants of pool `"builtin:coding"` while the counter buckets - selection-less rows under `DEFAULT_WORKFLOW_POOL_ID`, so the count came back - 0 for a pool nothing is ever placed in. Both sides now derive the pool - through `resolveCapacityPoolId`, and the hooks path rejects. - - The blast-radius question this test was holding open is therefore ANSWERED for - the hooks path and STILL OPEN for the inline one: the inline path remains - structurally unable to run the block (`if (useWorkflow && …)`), so converging - the paths still turns store-level capacity rejection on for every project. - That convergence stays an operator decision — see the R2 note in - workflow-capacity-invariant.pg.test.ts. - */ - const store = h.store(); - await store.updateSettings({ maxConcurrent: 1 }); - - /* Each phase starts from an EMPTY wip column. Before the gate bound, the two phases could share - one fixture because nothing ever counted occupants; now they cannot — the inline phase leaves - two cards in wip, and the hooks phase's own HOLDER move would trip the cap before the - contended move under test ever runs (observed: "column at capacity (2/1)"). Evacuating is - what keeps this a test of the contender's move rather than of fixture residue. */ - async function fillThenMoveSecond(): Promise { - for (const stale of await store.listTasks({ includeArchived: false })) { - if (stale.column === "in-progress") await store.deleteTask(stale.id); - } - const first = await store.createTask({ description: "capacity holder" }); - await store.moveTask(first.id, "todo"); - await store.moveTask(first.id, "in-progress"); - const second = await store.createTask({ description: "capacity contender" }); - await store.moveTask(second.id, "todo"); - return store.moveTask(second.id, "in-progress").then(() => null, (e: unknown) => e as Error); - } - - await setPath("inline"); - /* FNXC:WorkflowCapacity 2026-07-28-10:20 (R2 fix): was `toBeNull()` — the block used - to be unreachable here. Un-gating the capacity check is what converged the two - paths on this behavior; the OTHER divergences in this file (rejection type and - message) are deliberately untouched, because only the capacity check was - un-gated, not transition validation. */ - const inlineErr = await fillThenMoveSecond(); - expect((inlineErr as unknown as { rejection?: { code?: string } })?.rejection?.code).toBe( - "capacity-exhausted", - ); - - await setPath("hooks"); - const hooksErr = await fillThenMoveSecond(); - expect(hooksErr).toBeInstanceOf(Error); - expect((hooksErr as unknown as { rejection?: { code?: string } }).rejection?.code).toBe( - "capacity-exhausted", - ); - }); -}); diff --git a/packages/core/src/__tests__/postgres/store-movement.pg.test.ts b/packages/core/src/__tests__/postgres/store-movement.pg.test.ts index 3b9d90a381..38f1083ffc 100644 --- a/packages/core/src/__tests__/postgres/store-movement.pg.test.ts +++ b/packages/core/src/__tests__/postgres/store-movement.pg.test.ts @@ -54,14 +54,26 @@ pgTest("TaskStore moveTask column transitions (PostgreSQL)", () => { expect(done.column).toBe("done"); }); - it("allows moving an in-progress task back to triage", async () => { + it("moves an in-progress task back to the workflow's planning column, and REFUSES `triage`", async () => { + /* + FNXC:WorkflowColumns 2026-07-31-04:45 (U12 — the move-path flag is resolved): + Was "allows moving an in-progress task back to triage". The default lineage stopped declaring + `triage` at #2515, and the move path now resolves targets against the task's own workflow instead + of a hardcoded legacy adjacency table — so that move is refused rather than stranding the card in + a column with no trait flags, invisible to every trait-driven sweep. + + Both halves are asserted: the backward move that SHOULD work still works, so this reads as a + narrowing rather than a blanket refusal. + */ const store = h.store(); const task = await store.createTask({ description: "backward move" }); await store.moveTask(task.id, "todo", { moveSource: "user" }); await store.moveTask(task.id, "in-progress", { moveSource: "user" }); - const moved = await store.moveTask(task.id, "triage"); - expect(moved.column).toBe("triage"); + await expect(store.moveTask(task.id, "triage")).rejects.toThrow(/Unknown column for this workflow/); + + const moved = await store.moveTask(task.id, "todo"); + expect(moved.column).toBe("todo"); }); it("updates columnMovedAt timestamp on each move", async () => { diff --git a/packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts b/packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts index 6e1f14f62c..7fb6f56c23 100644 --- a/packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts +++ b/packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts @@ -72,21 +72,23 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { tautological "passes" respectively. A both-paths suite without this probe reports what it assumed, not what happened. */ - async function setPath(path: "inline" | "hooks"): Promise { + /* + FNXC:WorkflowColumns 2026-07-31-04:45 (U12 — the move-path flag is resolved): + There is only ONE path now, so this no longer selects between them. It is kept rather than deleted + because the probe half is still worth doing: it proves the move path is live and rejecting before + a capacity case draws conclusions from a rejection, so a capacity test cannot pass because moves + were broken for some unrelated reason. + + The `inline`/`hooks` parameter and the `updateGlobalSettings` flag write are gone with the flag. + */ + async function assertMovePathLive(): Promise { const store = h.store(); - await store.updateGlobalSettings( - path === "hooks" - ? { experimentalFeatures: { workflowColumns: true } } - : { experimentalFeatures: { workflowColumns: false } }, - ); - const probe = await store.createTask({ description: `path probe ${path}` }); + const probe = await store.createTask({ description: "path probe" }); const err = await store .moveTask(probe.id, "not-a-column-any-workflow-declares") .then(() => null, (e: unknown) => e as Error); - expect(err, `${path}: probe move should have been rejected`).toBeInstanceOf(Error); - expect(err!.message, `${path} path not active`).toContain( - path === "hooks" ? "Unknown column for this workflow" : "Valid targets:", - ); + expect(err, "probe move should have been rejected").toBeInstanceOf(Error); + expect(err!.message, "move path not active").toContain("Unknown column for this workflow"); await store.deleteTask(probe.id); } @@ -129,7 +131,7 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { */ const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); - await setPath("inline"); + await assertMovePathLive(); const { error, secondColumn } = await fillWipThenAdmitSecond(); @@ -150,7 +152,7 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { */ const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); - await setPath("hooks"); + await assertMovePathLive(); const { error, secondColumn } = await fillWipThenAdmitSecond(); @@ -172,7 +174,7 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { */ const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); - await setPath("hooks"); + await assertMovePathLive(); const { error, secondColumn } = await fillWipThenAdmitSecond({ selectWorkflow: "builtin:coding" }); @@ -207,7 +209,7 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { async () => { const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); - await setPath("hooks"); + await assertMovePathLive(); const { error, secondColumn } = await fillWipThenAdmitSecond(); @@ -258,7 +260,7 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { it("RATCHET: a workflow-selection change mid-move cannot split the limit from the counting pool", async () => { const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); - await setPath("inline"); + await assertMovePathLive(); // Fill builtin:coding's wip pool to its limit of 1. const holder = await store.createTask({ description: "split-snapshot holder" }); @@ -335,7 +337,7 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { it("RATCHET: the capacity read is taken UNDER the per-task lock, not merely inside the transaction", async () => { const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); - await setPath("inline"); + await assertMovePathLive(); const holder = await store.createTask({ description: "xproc holder" }); await store.selectTaskWorkflow(holder.id, "builtin:coding"); diff --git a/packages/core/src/__tests__/raw-workflow-columns-flag-census.test.ts b/packages/core/src/__tests__/raw-workflow-columns-flag-census.test.ts deleted file mode 100644 index 9516f4549f..0000000000 --- a/packages/core/src/__tests__/raw-workflow-columns-flag-census.test.ts +++ /dev/null @@ -1,184 +0,0 @@ -/* -FNXC:WorkflowColumns 2026-07-29-00:00 (U12 — R9): -CENSUS RATCHET for the raw `experimentalFeatures.workflowColumns` compatibility flag. - -U12's headline goal is deleting this flag and the settings key behind it. That cannot -happen while anything reads it, and "does anything still read it?" has been answered by -hand three times over the life of the unit — each time by grepping, each time producing -a number nobody can re-derive later. This test makes the answer a fact the suite -maintains. - -WHAT THE FLAG IS. `isWorkflowColumnsCompatibilityFlagEnabled` (store.ts) returns -`experimentalFeatures.workflowColumns === true`. It is the RAW key, distinct from the -public runtime helper that treats stale `false` as enabled. No module hardcodes -the key, so it reads false for every project that never carried a stale persisted value -— which is why every branch behind it has been silently inert, and why U12 spent its length finding features that looked -enforced and were not. - -WHAT REMAINS, and why it is not mine to remove. Both surviving reads are on the MOVE -PATH and belong to U2b (move-path convergence), which carries an equivalence-proof -obligation because the two implementations it arbitrates have never both run in -production. They are also not separable from each other: the preflight computes the -`movePolicyPreflight` that `moves.ts` consumes and validates, so un-gating it alone -would start evaluating workflow move policies — with their plugin-gate side effects — -while the branch that consumes the result stays off. - -THIS TEST FAILS IN BOTH DIRECTIONS, deliberately: - - a NEW read appears -> someone is re-gating behaviour on a retired flag; - - the LAST read disappears -> U2b has landed, and the settings key can finally go. -The second is the one that matters. It converts "remember to delete the key someday" -into a failing test at the exact moment that becomes possible. -*/ -import { describe, expect, it } from "vitest"; -import { existsSync, readdirSync, readFileSync, statSync } from "node:fs"; -import { join, resolve } from "node:path"; - -const REPO_ROOT = resolve(import.meta.dirname, "../../../.."); -const SOURCE_ROOTS = ["packages/core/src", "packages/engine/src", "packages/dashboard/src", "packages/cli/src"]; - -/** The raw-flag reader. Not the always-on public runtime helper. */ -const RAW_FLAG_READER = "isWorkflowColumnsCompatibilityFlagEnabled"; - -/** - * Every file permitted to reference the raw reader, and why. Paths are repo-relative. - * `store.ts` declares it; the other two are U2b's move path. - */ -const ALLOWED: ReadonlyArray<{ file: string; occurrences: number; why: string }> = [ - { - file: "packages/core/src/store.ts", - occurrences: 1, - why: "declares the helper; it goes with the last reader", - }, - { - file: "packages/core/src/task-store/moves.ts", - occurrences: 2, - why: "U2b: the import, plus `useWorkflow` selecting between the two move-side-effect implementations", - }, - { - file: "packages/core/src/task-store/workflow-task-create-ops.ts", - occurrences: 2, - why: "U2b: the import, plus the gate on the move-policy preflight that moves.ts consumes", - }, -]; - -/* -Strip comments AND string/template literals before scanning (PR #2537 review — greptile). -Comments alone were not enough: this flag is discussed by name in diagnostics, error -copy and fixtures, and a substring scan would classify any such TEXT as a reader — a -ratchet that fails on prose is a ratchet people learn to edit around. What remains after -this is executable code, where the symbol appearing means it is genuinely referenced. -*/ -function stripCommentsAndStrings(source: string): string { - return source - .replace(/\/\*[\s\S]*?\*\//g, " ") - .replace(/(^|[^:])\/\/[^\n]*/g, "$1 ") - /* - Template literals: keep the ${...} EXPRESSIONS, drop only the literal text - (PR #2537 review — greptile). Erasing whole templates would have removed executable - interpolations with them, so a reader written inside `${...}` would have escaped the - census entirely — a hole in the direction that matters, since it hides a read. - */ - .replace(/`(?:[^`\\]|\\.)*`/g, (template) => - (template.match(/\$\{[\s\S]*?\}/g) ?? []).join(" ")) - .replace(/'(?:[^'\\\n]|\\.)*'/g, '""') - .replace(/"(?:[^"\\\n]|\\.)*"/g, '""'); -} - -function collectSourceFiles(dir: string, out: string[]): void { - if (!existsSync(dir)) return; - for (const entry of readdirSync(dir)) { - if (entry === "__tests__" || entry === "node_modules" || entry === "dist" || entry === "__test-utils__") continue; - const full = join(dir, entry); - if (statSync(full).isDirectory()) { - collectSourceFiles(full, out); - continue; - } - if (entry.endsWith(".ts") || entry.endsWith(".tsx")) out.push(full); - } -} - -describe("raw workflowColumns flag census (U12)", () => { - const files: string[] = []; - for (const root of SOURCE_ROOTS) collectSourceFiles(join(REPO_ROOT, root), files); - - it("scans a non-trivial production source set, so an empty sweep cannot pass", () => { - // Without this, a broken path glob would make every assertion below vacuously true — - // the "guard that reports success without checking anything" failure mode. - expect(files.length).toBeGreaterThan(200); - }); - - it("the raw flag is read ONLY by the known move-path sites, at the known COUNT", () => { - /* - COUNTS, not just file names (PR #2537 review — CodeRabbit). A per-file allowlist has - a hole exactly where it matters least visibly: a NEW raw-flag read added inside - `moves.ts` — already an allowed file — would have passed silently. Pinning the - occurrence count per file means the census notices a third read in a file that is - permitted two. - - Whole-word matching, so a longer identifier that merely contains this one is not - counted. Deliberately NOT a full AST parse: that is a heavy lift for a guard whose - job is to notice movement, and the count already fails on the case that motivated - it. If this ever needs to distinguish a call from a re-export, parse then. - */ - const wholeWord = new RegExp(`\\b${RAW_FLAG_READER}\\b`, "g"); - const readers = files - .map((file) => ({ - file: file.slice(REPO_ROOT.length + 1), - occurrences: (stripCommentsAndStrings(readFileSync(file, "utf8")).match(wholeWord) ?? []).length, - })) - .filter((entry) => entry.occurrences > 0) - .sort((a, b) => a.file.localeCompare(b.file)); - - const allowed = ALLOWED - .map((entry) => ({ file: entry.file, occurrences: entry.occurrences })) - .sort((a, b) => a.file.localeCompare(b.file)); - - /* - Equality, not subset. A subset check would let the last reader vanish silently and - leave the settings key orphaned forever, which is precisely the outcome this exists - to prevent. - - If this fails because a reader was ADDED — a new file, or a higher count in an - existing one: do not edit ALLOWED to make it pass. A new read re-gates behaviour on - a flag that is false for every project that never carried a stale value, so the - feature behind it will not run. - - If this fails because a reader was REMOVED: U2b has landed. Delete - `isWorkflowColumnsCompatibilityFlagEnabled`, drop `workflowColumns` from - `HIDDEN_EXPERIMENTAL_FEATURE_KEYS` in the dashboard SettingsModal only after - confirming stale persisted values still render nothing, and delete this file. - */ - expect(readers).toEqual(allowed); - }); - - it("no production SOURCE LITERAL writes the key", () => { - /* - SCOPE, corrected (PR #2537 review — greptile). An earlier version of this claimed - "no production code writes the key", which overclaims and contradicts a correction - made earlier in this same unit (PR #2512, greptile P1): `settings-schema.ts` - explicitly TOLERATES stale persisted values, and the generic settings-update path — - settings import, configuration rollback — persists experimental-feature entries - assembled from RUNTIME data. A `true` can absolutely reach storage that way, on an - upgraded project that carried one. - - A source scan cannot see that and must not pretend to. What it does prove is - narrower and still worth pinning: no module hardcodes the key, so nothing in the - product deliberately turns the flag on. That is the property behind "every read is - false for a project that never carried a stale value" — not an absolute. - */ - const writers = files.filter((file) => { - const code = stripCommentsAndStrings(readFileSync(file, "utf8")); - return /workflowColumns\s*:\s*(true|false)/.test(code); - }).map((file) => file.slice(REPO_ROOT.length + 1)); - - expect(writers).toEqual([]); - }); - - it("records why each remaining reader survives, so the list cannot become folklore", () => { - // Cheap, but it forces the next person to state a reason when they touch the list. - for (const entry of ALLOWED) { - expect(entry.why.length).toBeGreaterThan(20); - expect(existsSync(join(REPO_ROOT, entry.file))).toBe(true); - } - }); -}); diff --git a/packages/core/src/live-agent-count.ts b/packages/core/src/live-agent-count.ts index ea7cc6f89e..0a8659a263 100644 --- a/packages/core/src/live-agent-count.ts +++ b/packages/core/src/live-agent-count.ts @@ -183,3 +183,5 @@ export function deriveRunningAgentCounts(perProject: Record): Ru } return { currentlyActive, projectsActive }; } + + diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 6b319dc4d3..ea90bb5134 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -35,13 +35,6 @@ export interface RepairOverlapBlockerResult { } /** @internal Extracted modules use this compatibility flag */ -export function isWorkflowColumnsCompatibilityFlagEnabled(settings: Pick | undefined): boolean { - /* - FNXC:WorkflowColumns 2026-06-22-00:00: - TaskStore still needs the raw compatibility flag for legacy movement characterization, v1 workflow-IR rollback persistence, and ON→OFF custom-column evacuation tests. This is narrower than the public runtime helper, which treats stale false values as enabled after workflow-column cutover. - */ - return settings?.experimentalFeatures?.workflowColumns === true; -} import { type PluginGateVerdict } from "./plugin-gate-verdict.js"; import type { PluginOnSchemaInit, PluginPostgresSchemaDefinition } from "./plugin-types.js"; import { assertLoadedPluginSchemaInitHooksSupported, type LoadedPluginSchemaContract } from "./postgres/plugin-schema-hook.js"; @@ -2406,11 +2399,14 @@ Issue #2149 requires read-only type filtering to occur in the file-store before the three U5 reconciliation guards (now unconditional) and the three v1-IR rollback-compat persistence sites (now unconditional, same stored bytes). - The underlying `isWorkflowColumnsCompatibilityFlagEnabled` SURVIVES for now: it is - still read by `moves.ts` and `workflow-task-create-ops.ts`, and removing those reads - IS the U2b move-path convergence, which carries an equivalence-proof obligation. - Deleting this wrapper is what makes the remaining reads easy to enumerate: after - this change, every surviving read of the raw flag is on the move path. + FNXC:WorkflowColumns 2026-07-31-04:00 (U12): `isWorkflowColumnsCompatibilityFlagEnabled` is now + DELETED TOO. Its last two readers were the move path — `moves.ts` and the preflight in + `workflow-task-create-ops.ts` — and both were un-gated in one commit because the preflight computes + what `moves.ts` consumes. + + The equivalence-proof obligation that held this back is discharged, not waived: + `moves-flag-equivalence.test.ts` diffs the persisted row after the same journey under both flag + states against live PostgreSQL and finds them identical, mutation-verified in both directions. */ public async listWorkflowOccupantTaskIds(workflowId: string, includeNullSelection: boolean): Promise { return listWorkflowOccupantTaskIdsImpl(this, workflowId, includeNullSelection); diff --git a/packages/core/src/task-store/moves.ts b/packages/core/src/task-store/moves.ts index 0b9555bdce..072cad6f3d 100644 --- a/packages/core/src/task-store/moves.ts +++ b/packages/core/src/task-store/moves.ts @@ -6,7 +6,7 @@ * behavior-preserving refactor. Each function receives the TaskStore * instance as its first parameter and performs byte-identical work. */ -import {type TaskStore, type MoveTaskOptions, type MoveTaskInternalOptions, storeLog, isWorkflowColumnsCompatibilityFlagEnabled} from "../store.js"; +import {type TaskStore, type MoveTaskOptions, type MoveTaskInternalOptions, storeLog} from "../store.js"; import * as schema from "../postgres/schema/index.js"; import {TaskDeletedError, HandoffInvariantViolationError, TransitionRejectionError} from "./errors.js"; import {and, eq, sql} from "drizzle-orm"; @@ -361,7 +361,6 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum // project) via getSettingsFast(). This is an async read taken before the // lock-sensitive transaction; it does not touch the task lock. const mergedSettingsForMove = await store.getSettingsFast(); - const useWorkflow = isWorkflowColumnsCompatibilityFlagEnabled(mergedSettingsForMove); // bypassGuards (KTD-9): engine-sourced moves + the existing skipMergeBlocker // call sites map onto it. Capacity (KTD-10) is NEVER bypassed by this — the // capacity check is not a guard (U6 fills the enforcement; U4 leaves a @@ -389,10 +388,16 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum is allowed to be stale; a capacity decision is not. */ const workflowSelectionForMove = await store.getTaskWorkflowSelectionAsync(id); - const effectiveWorkflowIdForMove = workflowSelectionForMove?.workflowId ?? DEFAULT_WORKFLOW_ID; - const workflowIr: WorkflowIr | undefined = useWorkflow - ? await resolveTaskWorkflowIrForMove(store, id) - : undefined; + /* + FNXC:WorkflowColumns 2026-07-31-05:25 (PR #2655 review — greptile P2 follow-through): + `effectiveWorkflowIdForMove` is DELETED. It existed only to feed the emitted `workflowId`, and it + applied a `?? DEFAULT_WORKFLOW_ID` fallback — so keeping it would have meant either an unused + binding or the very fallback-as-authoritative stamp that review flagged. The emit site now reads + the SELECTION directly, so the absence signal is preserved and there is nothing left to guess. + */ + // FNXC:WorkflowColumns 2026-07-31-04:00 (U12): resolved unconditionally — the gate is gone, so + // `undefined` now means only "no IR on this path or a v1 column-less IR", never "flag off". + const workflowIr: WorkflowIr | undefined = await resolveTaskWorkflowIrForMove(store, id); if (task.column === toColumn) { if (internal.fromHandoff && toColumn === "in-review") { @@ -487,7 +492,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum const fromColumn = task.column; - if (useWorkflow && workflowIr) { + if (workflowIr) { // ── Flag-ON validation + sync guards (typed rejections, KTD-3/R13) ───── // 1. Target column must exist in the task's workflow → unknown-column. // #1411: a recoveryRehome move to a LEGACY column (todo/archived/…) is @@ -787,7 +792,27 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum task.columnMovedAt = movedAt; task.updatedAt = movedAt; - if (useWorkflow) { + /* + FNXC:WorkflowColumns 2026-07-31-04:00 (U12 — the compatibility flag is RESOLVED): + Column side effects run through the default-workflow TRAIT HOOKS unconditionally. The + `if (useWorkflow)` gate and its inline legacy `else` branch are DELETED, not converted — + converting a branch we intended to delete would have left a second definition of every + column side effect alive to drift. + + WHY THIS WAS SAFE TO FLIP, evidenced rather than asserted: + - `moves-flag-equivalence.test.ts` runs the SAME journey under both flag states against live + PostgreSQL and diffs the persisted row: identical across 128 fields plus an equal timing + shape, over todo -> in-progress -> in-review -> todo -> in-progress. Mutation-verified in + both directions (stamping this branch, and diverging the reopen hook, each fail it). + - The flag was read by NOTHING in production: `experimentalFeatures.workflowColumns` is + global-only and no module writes it, so this branch had never run for any project that did + not carry a stale persisted value. That is also why "the suite is green" was never evidence + on its own — both paths were individually valid and only one was live. + - Target-column validation was NOT introduced by this flip. I claimed it was, twice, and + reproduced the opposite: a move to a column the task's workflow does not declare already + rejects on the legacy path with `Invalid transition: ... Valid targets: ...`. See the seam-2 + cases in that same test file. + */ /* FNXC:WorkflowLifecycleColumns 2026-07-30-08:10 (Phase C convergence): Resolved ONCE for both the hook context and the store's own reopen check, so the two @@ -855,130 +880,6 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum if (isReopenToTodoOrTriage && !preserveStepProgress) { await store.resetPromptCheckboxes(dir); } - } else { - // ── Flag-OFF legacy inline side effects (UNCHANGED — the flag-off path) ── - if (fromColumn === "in-progress" && toColumn !== "in-progress") { - const segmentStartMs = Date.parse(task.executionStartedAt ?? task.columnMovedAt); - const segmentEndMs = Date.parse(task.columnMovedAt); - const segmentDeltaMs = - Number.isFinite(segmentStartMs) && Number.isFinite(segmentEndMs) - ? Math.max(0, segmentEndMs - segmentStartMs) - : 0; - task.cumulativeActiveMs = Math.max(0, task.cumulativeActiveMs ?? 0) + segmentDeltaMs; - } - - if (toColumn === "in-progress") { - task.cumulativeActiveMs ??= 0; - if (!task.firstExecutionAt) { - task.firstExecutionAt = task.columnMovedAt; - } - if (!task.executionStartedAt) { - task.executionStartedAt = task.columnMovedAt; - } - task.userPaused = undefined; - } - if (toColumn === "done" && !task.executionCompletedAt) { - task.executionCompletedAt = task.columnMovedAt; - } - - if (toColumn === "done") { - store.clearDoneTransientFields(task); - } - - const isReopenToTodoOrTriage = - (fromColumn === "in-progress" || fromColumn === "done" || fromColumn === "in-review") - && (toColumn === "todo" || toColumn === "triage"); - - if (isReopenToTodoOrTriage) { - // FNXC:WorkflowLifecycle 2026-07-12-09:05 (merge port from main): keep - // this flag-OFF inline block in sync with applyResetOnEntryEffects - // (default-workflow-hooks.ts) — `preservePause` keeps a pause-caused - // teardown move from clearing the user's park (FN-7851 pause-bounce loop). - if (!options?.preserveStatus) { - task.status = undefined; - task.error = undefined; - if (!options?.preservePause) { - task.pausedReason = undefined; - } - } - task.blockedBy = undefined; - task.overlapBlockedBy = undefined; - if (!options?.preservePause) { - task.paused = undefined; - task.pausedByAgentId = undefined; - } - if (moveSource === "user" && toColumn === "todo") { - task.userPaused = true; - } else if (!options?.preservePause) { - task.userPaused = undefined; - } - - const hasNonPendingStepProgress = task.steps.some((step) => step.status !== "pending"); - const preserveStepProgress = - options?.preserveResumeState || (options?.preserveProgress === true && hasNonPendingStepProgress); - - if (!options?.preserveWorktree) { - task.worktree = undefined; - } - - if (!options?.preserveResumeState) { - task.executionStartedAt = undefined; - task.executionCompletedAt = undefined; - } else { - task.executionCompletedAt = undefined; - } - - if (!preserveStepProgress) { - store.resetAllStepsToPending(task); - await store.resetPromptCheckboxes(dir); - } - } - - if (toColumn === "in-review") { - // Keep this flag-OFF inline path in sync with applyInReviewEnterEffects. - // Do not snapshot global autoMerge: undefined follows the live setting, - // while explicit per-task true/false overrides remain sticky. - task.recoveryRetryCount = undefined; - task.nextRecoveryAt = undefined; - // Clear scheduler-side dispatch state: `queued`, `blockedBy`, and - // `overlapBlockedBy` are stamped while the task waits in `todo`. If - // they survive the transition into `in-review` they permanently block - // the merge gate (see getTaskMergeBlocker's BLOCKING_TASK_STATUSES). - if (task.status === "queued") { - task.status = undefined; - } - task.blockedBy = undefined; - task.overlapBlockedBy = undefined; - } - - /* - FNXC:WorkflowReviewGates 2026-07-26-14:25: - Parity mirror of the gate in `applyReopenFieldClears` (default-workflow-hooks.ts) — these two - blocks are deliberately kept byte-equivalent in behavior. The graph's own - in-review -> in-progress crossing (the remediation node entry, routine now that the pre-merge - review gates live in `in-review`) must retain `workflowStepResults`; every other reopen still - clears. See the hook for the full rationale. - */ - const graphOwnedReviewToWip = options?.workflowMoveSource === "workflow-graph" - && fromColumn === "in-review" - && toColumn === "in-progress"; - if ( - !graphOwnedReviewToWip - && ((fromColumn === "in-review" && (toColumn === "todo" || toColumn === "in-progress" || toColumn === "triage")) - || (fromColumn === "done" && (toColumn === "todo" || toColumn === "triage"))) - ) { - task.workflowStepResults = undefined; - } - - if (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "triage")) { - task.branch = undefined; - task.executionStartBranch = undefined; - task.baseCommitSha = undefined; - task.summary = undefined; - task.recoveryRetryCount = undefined; - task.nextRecoveryAt = undefined; - } - } if (toColumn === "in-progress" && !task.worktree && options?.allocateWorktree) { const allocator = options.allocateWorktree; @@ -1110,7 +1011,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum // crash-safe transitionPending marker in the SAME transaction as the // column change (KTD-2). countActiveInCapacitySlotAsync already counts // pending markers in PG, so this is load-bearing for capacity too. - if (useWorkflow) { + { await writeTransitionPendingAsync( tx, id, @@ -1348,7 +1249,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum // per-hook completion in the marker's hooksRemaining. A throwing plugin hook // DEGRADES (audit) and never wedges the lock or strands the marker — the // marker is always cleared at the end regardless of hook failures. - if (useWorkflow) { + { // Plugin hooks are skipped on engine/recovery-sourced moves (KTD-9 — those // bypass trait effects) and on same-column no-ops. if (!bypassGuards && fromColumn !== toColumn && workflowIr) { @@ -1403,17 +1304,22 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum ...(internal.runContext?.runId ? { runId: internal.runContext.runId } : {}), /* FNXC:WorkflowEvents 2026-07-27-15:10 (U3, PR #2467 review): - OMIT rather than guess. `effectiveWorkflowIdForMove` reads the task's real - selection only when `useWorkflow` is true; otherwise it is hardcoded to - `builtin:coding`. That compat flag is off for effectively every real - project (see the note at the flag-OFF adjacency branch above), so - emitting it unconditionally would stamp `builtin:coding` onto moves of - tasks on a custom workflow — a wrong value baked into a brand-new wire - field, latent only because no subscriber reads it yet. An absent - `workflowId` means "not resolved here"; a subscriber that needs it reads - the selection itself. + OMIT rather than guess. An absent `workflowId` means "not resolved here"; + a subscriber that needs it reads the selection itself. + + FNXC:WorkflowColumns 2026-07-31-05:20 (PR #2655 review — greptile P2): + The flag deletion nearly destroyed that signal. `effectiveWorkflowIdForMove` + is `selection?.workflowId ?? DEFAULT_WORKFLOW_ID`, so emitting it + unconditionally would stamp `builtin:coding` onto every task that has no + explicit selection — reporting a FALLBACK as an authoritative choice, which + is the same "wrong value baked into a wire field" this note was written to + prevent, arrived at from the other direction. + + So the condition survives the flag: emit only when the selection genuinely + resolved. `builtin:coding` still appears here for tasks that really select + it, and is absent for tasks that merely default to it. */ - ...(useWorkflow ? { workflowId: effectiveWorkflowIdForMove } : {}), + ...(workflowSelectionForMove?.workflowId ? { workflowId: workflowSelectionForMove.workflowId } : {}), }); } if (toColumn === "done") { diff --git a/packages/core/src/task-store/task-store-helpers.ts b/packages/core/src/task-store/task-store-helpers.ts index 20d4c8290a..147e1a4d35 100644 --- a/packages/core/src/task-store/task-store-helpers.ts +++ b/packages/core/src/task-store/task-store-helpers.ts @@ -86,6 +86,44 @@ export function resolveWorkflowBypassGuardsImpl(store: TaskStore, moveSource: NonNullable, options?: MoveTaskOptions, ): boolean { + /* + FNXC:WorkflowColumns 2026-07-31-11:00 (PR #2655 review — BOTH findings are right, and they are + the same defect seen from two sides. REVERTED to reading `options?.moveSource`.) + + Round 1 said an optionless `moveTask(id, target)` LOSES bypass, because the call site resolves + `moveSource` to "engine" while this read the absent option. I switched to the resolved value. + Round 2 said that grants privileged bypass to PUBLIC callers. Both are correct, because an absent + `moveSource` is genuinely ambiguous — measured on this tree: + + 18 optionless calls in packages/engine (self-healing, project-engine) — genuine engine moves + 8 optionless calls in packages/dashboard HTTP routes (reset, rebound, respecify, unassign) + — operator-initiated, and they must NOT skip merge blockers or plugin gates + + Reading the resolved value hands bypass to those eight routes. Reading the option leaves the + eighteen engine calls unbypassed, which is what has shipped all along. + + So this reverts to the shipped read. A flag-resolution PR is the wrong place to change who gets to + skip merge blockers: it is a behaviour change with a security shape, it is not required by the + flip, and "the tests pass" is not evidence for it. The real fix is to make `moveSource` EXPLICIT at + those 26 call sites so the default never has to be guessed — filed as follow-up work, not smuggled + in here. + + The `void moveSource;` below is kept for the same reason it existed: the parameter is part of the + signature and deliberately unused until that follow-up lands. + + `void moveSource;` discarded the parameter and re-read the raw option, so an OPTIONLESS call — + `moveTask(id, target)` — resolved `moveSource` to "engine" at every call site and then computed + `bypassGuards === false`, because `options` was undefined. The two disagreed about what kind of + move it was. + + Harmless while the move-path flag gated validation, because nothing consumed the answer. The flag + is gone, so seam 2's guards now run for these calls and an internal executor/merger/recovery move + made without an options object would be judged as if a user had made it. + + The `void` was a deliberate unused-parameter suppression, i.e. someone noticed the argument was + unused and silenced the lint instead of wiring it up. Using it aligns bypass with the `moveSource` + every caller already resolves the same way. + */ void moveSource; return options?.recoveryRehome === true || (options?.bypassGuards ?? diff --git a/packages/core/src/task-store/workflow-task-create-ops.ts b/packages/core/src/task-store/workflow-task-create-ops.ts index 57fdaf3117..105f54fa0a 100644 --- a/packages/core/src/task-store/workflow-task-create-ops.ts +++ b/packages/core/src/task-store/workflow-task-create-ops.ts @@ -8,7 +8,7 @@ * behavior-preserving refactor. Each function receives the TaskStore * instance as its first parameter and performs byte-identical work. */ -import {TaskStore, isWorkflowColumnsCompatibilityFlagEnabled} from "../store.js"; +import {TaskStore} from "../store.js"; import {resolveEntryColumnId} from "../workflow-reconciliation.js"; import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; import * as schema from "../postgres/schema/index.js"; @@ -349,8 +349,14 @@ export async function getTaskColumnsImpl(store: TaskStore, ids: string[]): Promi export async function prepareWorkflowMovePolicyPreflightImpl(store: TaskStore, id: string, toColumn: ColumnId, options: MoveTaskOptions | undefined, internal: MoveTaskInternalOptions,): Promise { const task = await store.readTaskForMove(id); const moveSource = options?.moveSource ?? "engine"; - const mergedSettingsForMove = await store.getSettingsFast(); - if (!isWorkflowColumnsCompatibilityFlagEnabled(mergedSettingsForMove)) return undefined; + /* + FNXC:WorkflowColumns 2026-07-31-04:00 (U12 — flipped ATOMICALLY with moves.ts): + The compatibility-flag gate is DELETED. This preflight computes the `movePolicyPreflight` that + `moves.ts` consumes and validates, so the two could never be flipped independently: un-gating + this alone would evaluate workflow move policies — with their plugin-gate side effects — while + the branch consuming the result stayed off, and un-gating `moves.ts` alone would validate against + a preflight that was never computed. Both readers go in the same commit for that reason. + */ if (task.column === toColumn) return undefined; /* FNXC:WorkflowModelLanes 2026-07-14-16:31: PostgreSQL move preflight must validate against the task's migrated workflow selection, not the synchronous builtin:coding fallback. */ diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index d85df18ef3..0ad04c794f 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -1,27 +1,26 @@ { "generatedFrom": "node scripts/lifecycle-column-census.mjs --strict --update-baseline", "totals": { - "column": 746, + "column": 722, "role": 5, "status": 186, "deliberate": 17 }, "byColumnId": { - "done": 199, - "in-progress": 144, - "in-review": 205, + "done": 195, + "in-progress": 138, + "in-review": 200, "archived": 147, - "todo": 47, - "triage": 4 + "todo": 42 }, "byFile": { "packages/engine/src/self-healing.ts": 110, "packages/engine/src/executor.ts": 85, "packages/dashboard/app/components/TaskCard.tsx": 42, - "packages/core/src/task-store/moves.ts": 39, "packages/dashboard/app/components/TaskDetailModal.tsx": 30, "packages/engine/src/scheduler.ts": 28, "packages/dashboard/src/routes/register-task-workflow-routes.ts": 20, + "packages/core/src/task-store/moves.ts": 15, "packages/core/src/store.ts": 12, "packages/engine/src/project-engine.ts": 12, "packages/engine/src/mission-execution-loop.ts": 10,