diff --git a/packages/core/package.json b/packages/core/package.json index ca427445d2..22a4407d20 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -48,8 +48,8 @@ "typecheck": "tsc --noEmit", "test": "vitest run --silent=passed-only --reporter=dot", "test:embedded-postgres": "vitest run src/__tests__/postgres/embedded-lifecycle.test.ts --silent=passed-only --reporter=dot", - "test:pg-gate": "vitest run --config vitest.pg.config.ts src/__tests__/postgres/handoff-to-review-atomicity.pg.test.ts src/__tests__/postgres/task-lifecycle-e2e.pg.test.ts --silent=passed-only --reporter=dot", - "test:unit-gate": "vitest run src/__tests__/task-merge.test.ts src/__tests__/legacy-adoption.test.ts src/__tests__/no-hardcoded-lifecycle-columns.test.ts --silent=passed-only --reporter=dot" + "test:pg-gate": "vitest run --config vitest.pg.config.ts src/__tests__/postgres/handoff-to-review-atomicity.pg.test.ts src/__tests__/postgres/task-lifecycle-e2e.pg.test.ts src/__tests__/postgres/sync-workflow-ir-is-always-default.pg.test.ts --silent=passed-only --reporter=dot", + "test:unit-gate": "vitest run src/__tests__/task-merge.test.ts src/__tests__/legacy-adoption.test.ts src/__tests__/no-hardcoded-lifecycle-columns.test.ts src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts --silent=passed-only --reporter=dot" }, "devDependencies": { "@earendil-works/pi-coding-agent": "0.82.1", diff --git a/packages/core/src/__tests__/postgres/sync-workflow-ir-is-always-default.pg.test.ts b/packages/core/src/__tests__/postgres/sync-workflow-ir-is-always-default.pg.test.ts new file mode 100644 index 0000000000..6f66cc8338 --- /dev/null +++ b/packages/core/src/__tests__/postgres/sync-workflow-ir-is-always-default.pg.test.ts @@ -0,0 +1,125 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-18:00 (fleet — a whole CLASS of conversions is inert): + +THE CLAIM, PROVEN HERE RATHER THAN ARGUED: `resolveTaskWorkflowIrSync` returns the DEFAULT workflow +IR for every task in production, no matter which workflow the task is actually bound to. + +`getTaskWorkflowSelectionImpl` returns `undefined` unconditionally — it is a PostgreSQL-cutover stub +(`workflow-definitions.ts:505`, "Backend mode cannot synchronously read PostgreSQL, so return +undefined and let the sync reader fall back to its default"). Every sync IR read therefore takes the +`!workflowId` branch. + +WHY THIS MATTERS MORE THAN AN UNCONVERTED LITERAL. A guard written as + + resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(id))?.hold + +READS as converted. It resolves an IR, it asks for a trait, the census counts it as progress, and it +is wrong for every custom workflow — silently, because the return type is non-optional so no caller +can detect the substitution. A plain `=== "todo"` is at least honest about being a literal. + +Ten call sites currently resolve this way (five in `packages/core/src/task-store/`, five in +`scheduler.ts` via `resolveTaskParkedColumnsSync`). This test does not fix them: it pins the +FACT they depend on, so the next person to write "convert it with the sync resolver" has a failing +assertion to read instead of a plausible-looking guard. + +If a future change makes the sync path authoritative, this test SHOULD fail — that is the signal to +revisit those ten sites, not to delete the assertion. +*/ +import { it, expect, beforeAll, beforeEach, afterEach, afterAll } from "vitest"; +import { + pgDescribe, + createSharedPgTaskStoreTestHarness, + type SharedPgTaskStoreHarness, +} from "../../__test-utils__/pg-test-harness.js"; +import { resolveLifecycleColumns } from "../../workflow-lifecycle-traits.js"; + +pgDescribe("resolveTaskWorkflowIrSync ignores a task's real workflow (PostgreSQL)", () => { + const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_sync_ir_default", + }); + + beforeAll(h.beforeAll); + beforeEach(h.beforeEach); + afterEach(h.afterEach); + afterAll(h.afterAll); + + /** A workflow with NO legacy column ids at all, so a default-IR answer is unmistakable. */ + async function seedRenamedWorkflow(): Promise { + const created = await h.store().createWorkflowDefinition({ + name: "Renamed", + kind: "workflow", + ir: { + version: "v2", + id: "custom:sync-ir-probe", + nodes: [ + { id: "start", kind: "start", column: "drafting" }, + { id: "gate", kind: "merge-gate", column: "checking", config: { gate: "auto-merge" } }, + { id: "end", kind: "end", column: "shipped" }, + ], + edges: [{ from: "start", to: "gate" }, { from: "gate", to: "end" }], + columns: [ + { id: "drafting", label: "Drafting", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", label: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "checking", label: "Checking", traits: [{ trait: "human-review" }, { trait: "merge" }] }, + { id: "shipped", label: "Shipped", traits: [{ trait: "complete" }] }, + ], + }, + } as never); + return (created as { id: string }).id; + } + + it("returns the DEFAULT lifecycle for a task bound to a renamed workflow", async () => { + const store = h.store(); + const workflowId = await seedRenamedWorkflow(); + const task = await store.createTask({ title: "probe", description: "t", column: "todo" }); + await store.writeTaskWorkflowSelection(task.id, workflowId, []); + + /* + Prove the binding actually landed, or the assertion below is vacuous — it would pass just as well + if the selection were never written, which is the failure mode that would make this test lie. + */ + const authoritative = await store.getTaskWorkflowSelectionAsync(task.id); + expect(authoritative?.workflowId, "the ASYNC reader must see the binding").toBe(workflowId); + + const syncLifecycle = resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(task.id)); + + /* The task is bound to a board whose hold lane is `drafting`. The sync reader says `todo`. */ + expect(syncLifecycle?.hold).toBe("todo"); + expect(syncLifecycle?.hold).not.toBe("drafting"); + expect(syncLifecycle?.review).not.toBe("checking"); + expect(syncLifecycle?.complete).not.toBe("shipped"); + }); + + /* + The contrast that makes the first case actionable: the ASYNC resolver on the same task, in the same + transaction, gets it right. So the fix for any of the ten sites is always "reach the async + resolver", never "resolve it synchronously and hope". + */ + it("the ASYNC resolver on the same task returns the task's real lifecycle", async () => { + const store = h.store(); + const workflowId = await seedRenamedWorkflow(); + const task = await store.createTask({ title: "probe", description: "t", column: "todo" }); + await store.writeTaskWorkflowSelection(task.id, workflowId, []); + + const { resolveTaskLifecycleColumns } = await import("../../workflow-lifecycle-traits.js"); + const asyncLifecycle = await resolveTaskLifecycleColumns(store as never, task.id); + + expect(asyncLifecycle?.hold).toBe("drafting"); + expect(asyncLifecycle?.review).toBe("checking"); + expect(asyncLifecycle?.complete).toBe("shipped"); + }); + + /* + And the reason it happens, pinned directly so the diagnosis does not have to be re-derived from + behaviour: the sync selection reader is a stub that always answers "no selection". + */ + it("the sync selection reader returns undefined even when a selection exists", async () => { + const store = h.store(); + const workflowId = await seedRenamedWorkflow(); + const task = await store.createTask({ title: "probe", description: "t", column: "todo" }); + await store.writeTaskWorkflowSelection(task.id, workflowId, []); + + expect(store.getTaskWorkflowSelection(task.id)).toBeUndefined(); + expect((await store.getTaskWorkflowSelectionAsync(task.id))?.workflowId).toBe(workflowId); + }); +}); diff --git a/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts b/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts new file mode 100644 index 0000000000..b4336f8890 --- /dev/null +++ b/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts @@ -0,0 +1,204 @@ +import { describe, expect, it } from "vitest"; +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join, relative, resolve } from "node:path"; +import ts from "typescript"; + +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-18:00 (fleet — stop the inert-conversion class growing): + +`resolveTaskWorkflowIrSync` returns the DEFAULT workflow IR for EVERY task in production. Proven in +`postgres/sync-workflow-ir-is-always-default.pg.test.ts`: the sync selection reader is a +PostgreSQL-cutover stub that answers `undefined` unconditionally, so the resolver always takes its +`!workflowId` branch. Its return type is non-optional, so no caller can detect the substitution. + +That makes it the most dangerous tool in this conversion program. A guard written as + + resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(id))?.hold + +reads as converted, resolves an IR, asks for a trait — and is wrong for every custom workflow, +silently. The lifecycle-column census scores it as PROGRESS. An unconverted `=== "todo"` is strictly +better, because it is at least honest about being a literal. + +So new call sites need a deliberate entry here rather than passing review on looking correct. This is +the same shape as the repo's other call-site allow-lists (the engine blocking-shellout list, and the +detached-spawn script guard under `scripts/`), and for the same reason: the primitive has a +legitimate narrow use and a plausible-looking wrong one. + +(Those two names are deliberately not spelled literally here: the spawn guard matches on raw text +across `packages/**`, so quoting its banned token in prose trips it. A guard that greps rather than +parses cannot tell a mention from a use — which is the same lesson this file is about, one level up.) + +TO ADD A SITE: prove the async resolver (`resolveTaskLifecycleColumns` / +`resolveWorkflowIrForTask`) is genuinely unreachable there — usually because you are inside a +synchronous event listener or a hot transaction — and say so in the entry. "It was easier" is not a +reason; a sync-resolved lifecycle guard is a guard that cannot fire. +*/ + +const ALLOWED_CALL_SITES: ReadonlyMap = new Map([ + [ + "packages/core/src/task-store/branch-and-pr-entities.ts", + "Inside a synchronous store entity read.", + ], + [ + "packages/core/src/task-store/lifecycle-ops.ts", + "Synchronous lifecycle bookkeeping inside a transaction.", + ], + [ + "packages/core/src/task-store/task-store-helpers.ts", + "Synchronous helper shared by txn-hot paths.", + ], + [ + "packages/core/src/task-store/workflow-task-create-ops.ts", + "Task creation runs before any selection exists, so the default IR is the correct answer here.", + ], + [ + "packages/engine/src/replan-target.ts", + "`resolvePlannerLanes`, a synchronous planner-lane read. FOUND BY THIS RATCHET, not by the grep " + + "that seeded the list — it calls through an optional-property cast " + + "(`(store as { resolveTaskWorkflowIrSync?: ... }).resolveTaskWorkflowIrSync?.(id)`), which no " + + "textual search for `store.resolveTaskWorkflowIrSync` matches. Its hazard is the sharpest of " + + "the six: it returns `resolvedFromWorkflow: true` whenever an IR came back, so on a renamed " + + "board a caller branching on that flag is told the lanes are workflow-resolved while being " + + "handed the DEFAULT ones.", + ], + [ + "packages/engine/src/scheduler.ts", + "`resolveTaskParkedColumnsSync`, called from synchronous `task:moved` / `task:updated` listeners " + + "where introducing an await would reorder handlers against a synchronous emitter.", + ], +]); + +/** The declaration and the resolver's own module are not call sites. */ +const EXCLUDED = [ + "packages/core/src/store.ts", + "packages/core/src/task-store/workflow-definitions.ts", + "packages/core/src/workflow-ir-resolver.ts", +]; + +const REPO_ROOT = resolve(__dirname, "../../../.."); +const SCAN_ROOTS = [ + "packages/core/src", + "packages/engine/src", + "packages/dashboard/src", + "packages/cli/src", +]; + +function* walk(dir: string): Generator { + let entries: string[]; + try { + entries = readdirSync(dir); + } catch { + return; + } + for (const entry of entries) { + if (entry === "node_modules" || entry === "dist" || entry === "__tests__") continue; + const full = join(dir, entry); + if (statSync(full).isDirectory()) yield* walk(full); + else if (/\.tsx?$/.test(full)) yield full; + } +} + +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-20:10 (PR #2759 review — greptile P2): +ALIASES COUNT. Matching only `.resolveTaskWorkflowIrSync(...)` left an opening: destructure or +rebind the method and the callee becomes a bare identifier, so a new synchronous resolution passes a +guard whose whole purpose is to catch it. + + const { resolveTaskWorkflowIrSync: resolveIr } = store; // callee is now an identifier + const ir = resolveIr(taskId); + +`replan-target.ts` already proves the family is used through non-obvious call shapes — it reaches the +method via an optional-property cast, which is why the grep that seeded this list missed it. So the +detector tracks the NAME through local aliases as well as property access, and additionally refuses +the alias-creating forms outright, which is cheaper to reason about than chasing every rebinding. +*/ +/** Call sites of the sync resolver, by property access OR through a local alias. Found by AST. */ +function findCallSites(): Map { + const byFile = new Map(); + + for (const root of SCAN_ROOTS) { + for (const file of walk(join(REPO_ROOT, root))) { + const rel = relative(REPO_ROOT, file).split("\\").join("/"); + if (EXCLUDED.includes(rel)) continue; + const source = readFileSync(file, "utf8"); + if (!source.includes("resolveTaskWorkflowIrSync")) continue; + + const sf = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + let count = 0; + /* Local names bound to the method, so `const f = store.resolveTaskWorkflowIrSync; f(id)` counts. */ + const aliases = new Set(); + const collectAliases = (node: ts.Node) => { + if (ts.isVariableDeclaration(node) && node.initializer && ts.isIdentifier(node.name)) { + const init = node.initializer; + const isMethodRef = (ts.isPropertyAccessExpression(init) || ts.isNonNullExpression(init)) + && init.getText(sf).includes("resolveTaskWorkflowIrSync"); + if (isMethodRef) aliases.add(node.name.text); + } + /* Destructuring: `const { resolveTaskWorkflowIrSync: alias } = store`. */ + if (ts.isVariableDeclaration(node) && ts.isObjectBindingPattern(node.name)) { + for (const element of node.name.elements) { + const source = (element.propertyName ?? element.name); + if (ts.isIdentifier(source) && source.text === "resolveTaskWorkflowIrSync" + && ts.isIdentifier(element.name)) { + aliases.add(element.name.text); + } + } + } + ts.forEachChild(node, collectAliases); + }; + collectAliases(sf); + + const visit = (node: ts.Node) => { + if (ts.isCallExpression(node)) { + const callee = node.expression; + const isPropertyCall = ts.isPropertyAccessExpression(callee) + && callee.name.text === "resolveTaskWorkflowIrSync"; + const isAliasCall = ts.isIdentifier(callee) && aliases.has(callee.text); + if (isPropertyCall || isAliasCall) count += 1; + } + ts.forEachChild(node, visit); + }; + visit(sf); + /* An alias declared but never called still counts: it exists to be called. */ + if (count === 0 && aliases.size > 0) count = aliases.size; + if (count > 0) byFile.set(rel, count); + } + } + return byFile; +} + +describe("resolveTaskWorkflowIrSync call sites are allow-listed", () => { + /* + Completeness: the allow-list is worthless if the scan finds nothing (a moved directory, a renamed + method). This fails loudly instead of passing vacuously. + */ + it("finds the known call sites", () => { + const found = findCallSites(); + + expect(found.size, "expected to find the documented sync-resolution call sites").toBeGreaterThan(0); + }); + + it("has no call site outside the allow-list", () => { + const found = findCallSites(); + const unlisted = [...found.keys()].filter((file) => !ALLOWED_CALL_SITES.has(file)).sort(); + + expect( + unlisted, + "resolveTaskWorkflowIrSync returns the DEFAULT workflow IR for every task, so a lifecycle " + + "guard resolved through it CANNOT fire correctly on a custom workflow — and reads as " + + "converted while doing it. Use the async resolver, or add an entry with the reason the " + + "async path is unreachable.", + ).toEqual([]); + }); + + /* + The other direction: an allow-list that outlives its entries rots into permission nobody reviewed. + A site that stops using the primitive should lose its entry in the same change. + */ + it("has no stale allow-list entry", () => { + const found = findCallSites(); + const stale = [...ALLOWED_CALL_SITES.keys()].filter((file) => !found.has(file)).sort(); + + expect(stale, "remove allow-list entries for files that no longer resolve synchronously").toEqual([]); + }); +});