test(core): sync workflow resolution always returns the default IR — the proof and a call-site ratchet (#2759)
## A whole class of "conversions" in this program is inert, and the
census scores them as done
`resolveTaskWorkflowIrSync` returns the **default** workflow IR for
**every** task in production. `getTaskWorkflowSelectionImpl` is a
PostgreSQL-cutover stub that returns `undefined` unconditionally
(`workflow-definitions.ts:505` — *"Backend mode cannot synchronously
read PostgreSQL, so return undefined and let the sync reader fall back
to its default"*), so the sync resolver always takes its `!workflowId`
branch. Its return type is **non-optional**, so no caller can detect the
substitution.
**Proven, not argued.** The PG suite binds a task to a workflow whose
lanes are `drafting`/`building`/`checking`/`shipped`, asserts the
**async** reader sees that binding — or the next assertion would be
vacuous — and then shows the sync resolver answering `hold: "todo"` for
the same task. A third case pins the cause directly: the sync selection
reader returns `undefined` while the async one returns the workflow id.
The async resolver on the same task, in the same test, returns the real
lanes. So the remedy for any affected site is always *reach the async
resolver*, never *resolve synchronously and hope*.
### Why this is worse than an unconverted literal
```ts
resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(id))?.hold
```
That **reads** as converted. It resolves an IR, asks for a trait, and
the lifecycle-column census counts it as **progress** — while being
wrong for every custom workflow, silently. A plain `=== "todo"` is
strictly better, because it is at least honest about being a literal.
That is why this needs a guard rather than a comment: the failure mode
is code that *looks right in review*.
### The ratchet earned its place immediately
It allow-lists the six call sites, in the shape this repo already uses
for `engine-no-blocking-shellout` and `check-no-nohup`. On its first run
it corrected the list I had seeded by grep:
- **Found `replan-target.ts`**, which my grep missed — it calls through
an optional-property cast, so no textual search for
`store.resolveTaskWorkflowIrSync` matches it. **Its hazard is the
sharpest of the six:** `resolvePlannerLanes` 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.
- **Rejected `executor.ts`**, which my grep had matched on a *comment*
with no real call site.
It also carries a completeness case (fails if the scan finds nothing,
rather than passing vacuously) and a staleness case (an entry whose file
stops using the primitive must be removed in the same change, so the
list can't rot into unreviewed permission).
### Verified to fire
Adding a new unlisted call site fails with the offending file named:
```
+ "packages/engine/src/gridlock-detector.ts"
Tests 1 failed | 2 passed (3)
```
My first attempt at that proof landed the probe **inside a block
comment** and the guard correctly reported zero — the methodology was
wrong, not the guard. Worth stating, since a guard I couldn't make fail
is exactly the thing this PR is about.
### Scope
**No source changes.** This pins a fact and guards a primitive. The six
existing sites are deliberately left alone: each needs its own
async-reachability analysis, which is per-site work with real behaviour
risk, not a sweep. `scheduler.ts`'s entry is the honest case — it is
called from synchronous `task:moved` listeners where adding an `await`
would reorder handlers against a synchronous emitter.
### Verification
- new PG suite **3 passed** · new ratchet **3 passed**
- `pnpm test:gate` — **158 / 10 / 487 / 71** · `pnpm lint` clean · core
`tsc --noEmit` **0 errors** · `--strict` exits 0
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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",
|
||||
|
||||
@@ -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<string> {
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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<string, string> = 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<string> {
|
||||
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 `<expr>.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<string, number> {
|
||||
const byFile = new Map<string, number>();
|
||||
|
||||
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<string>();
|
||||
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([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user