From 827386dde6e4d3dc2c41d692c059f281aa6ef8da Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 04:25:48 -0700 Subject: [PATCH] =?UTF-8?q?test(core):=20the=20sync=20IR=20path=20is=20blo?= =?UTF-8?q?cked=20TWICE,=20not=20once=20=E2=80=94=20every=20note=20in=20th?= =?UTF-8?q?e=20repo=20undercounts=20it=20(#3103)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every remaining census cluster I could not convert — `executor.ts` (4), `scheduler.ts` (2), `triage.ts` (1), and the four-guard fan-out I withdrew from my own PR — is waiting on the same thing. So I went to unblock it, and found the record is wrong. ## The repo says one blocker. There are two, plus a constraint The call-site allow-list header, the live-PG proof, and a dozen FNXC notes across engine and core — **several of which I wrote** — all say: `resolveTaskWorkflowIrSync` is inert because the sync selection reader returns `undefined`, and the fix is "a sync-capable workflow-selection reader". That understates the work by half, and the undercount is load-bearing: it makes the unblock read like a caching job, so the next person ships a selection cache and finds the rest at integration time. ### Blocker 2 — the IR read is dead too `resolveTaskWorkflowIrSyncImpl` loads a **custom** workflow's IR through `store.db.prepare("SELECT ir FROM workflows WHERE id = ?")`. `TaskStore.db` is not "SQLite-only". Its implementation (`dbImpl`, `task-id-integrity.ts`) is an **unconditional throw with no mode branch at all**. That read always throws into the surrounding `catch`, which always returns the default IR. The consequence is precisely the one this program cares about: | workflow kind | after a perfect selection reader | |---|---| | built-in | resolves — that branch never touches `store.db` | | **custom** | **still the default IR, always** | **A renamed lane is by definition a custom workflow.** So the sync path cannot serve the renamed-board case *at all* until this second read is replaced. Fixing the selection reader alone would produce a change that looks like it works — on default boards. ### Blocker 3 — a node-local cache is unsafe here Not a bug; a constraint that bounds the fix's shape. Multiple Fusion nodes run their own engines against **one shared PostgreSQL** (`docs/multi-project.md` → "Shared Postgres multi-node runbook"). A node-local synchronous cache of `task_workflow_selection` therefore goes stale whenever *another node* rewrites a selection — and answers with full confidence. That is **worse than today's default**, which is at least uniformly wrong rather than intermittently wrong. Any sync reader needs an invalidation story that survives a writer on a different host. ## Why a test rather than a comment A comment saying "db always throws" decays the moment someone adds a mode branch, and the whole argument silently inverts — which is the same decay mode this conversion program keeps hitting with allow-list entries and stale notes. The assertions are deliberately about `dbImpl`'s **source** rather than a call. Calling it proves one construction path throws; the fix depends on the stronger claim that **no mode returns a database**. Reintroducing an `if`/`return` there fails the test, which is the correct outcome: the premise really has changed and the file must be re-read. ## Measured - 4 new cases pass; the allow-list file's own 7 still pass with its corrected header. - **MUTATION**: adding a mode branch to `dbImpl` fails the first case. - An **anti-vacuity** case pins that the resolver is still live and still allow-listed, so these source assertions cannot keep passing after the concern is deleted. - `tsc --noEmit -p packages/core` clean; census `--strict`, `check-inert-sync-lane-conversions`, `check-fnxc-future-dates` clean. ## Census **No movement — this converts nothing.** It corrects the record about what the remaining conversions are waiting on, and it corrects notes I authored. I would rather spend a PR making the next attempt cheap than leave a half-true blocker in place that costs someone a full cycle to rediscover. ## What I did not do I did not build the sync reader. With blockers 2 and 3 in view it is a store-substrate change — a second read to replace, and an invalidation story that survives a writer on another host — not a fleet conversion, and starting it mid-sweep on a shared file would repeat the collision pattern that has already cost this branch three rebuilds. It remains unclaimed, and now it is fully specified. Co-authored-by: Claude Opus 5 (1M context) --- ...ync-workflow-ir-callsite-allowlist.test.ts | 13 +++ .../sync-workflow-ir-second-blocker.test.ts | 100 ++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 packages/core/src/__tests__/sync-workflow-ir-second-blocker.test.ts 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 index 4a1168e14f..2649b6b408 100644 --- a/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts +++ b/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts @@ -11,6 +11,19 @@ FNXC:WorkflowLifecycleColumns 2026-07-31-18:00 (fleet — stop the inert-convers 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. +FNXC:WorkflowLifecycleColumns 2026-07-31-23:10 (CORRECTION — this note named ONE blocker; there are +TWO, and I am the author of several of the notes elsewhere that repeat the same undercount): +Fixing the selection reader alone would NOT un-inert this path for the boards this program exists +for. `resolveTaskWorkflowIrSyncImpl` loads a CUSTOM workflow's IR through `store.db.prepare(...)`, +and `TaskStore.db` (`dbImpl`, task-id-integrity.ts) is an UNCONDITIONAL throw with no mode branch — +so that read always throws into the surrounding `catch` and always yields the default IR. A built-in +workflow could resolve once the selection reader works; a CUSTOM one never can, and a renamed lane +is by definition a custom workflow. +A third constraint bounds the fix's shape: multiple nodes run their own engines against ONE shared +PostgreSQL (`docs/multi-project.md`), so a node-local sync cache of the selection goes stale when +another node writes one — confidently wrong, which is worse than today's uniformly-wrong default. +All three are proved and kept honest by `sync-workflow-ir-second-blocker.test.ts`. + That makes it the most dangerous tool in this conversion program. A guard written as resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(id))?.hold diff --git a/packages/core/src/__tests__/sync-workflow-ir-second-blocker.test.ts b/packages/core/src/__tests__/sync-workflow-ir-second-blocker.test.ts new file mode 100644 index 0000000000..5c3361ae50 --- /dev/null +++ b/packages/core/src/__tests__/sync-workflow-ir-second-blocker.test.ts @@ -0,0 +1,100 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-23:05: +THE SYNC IR PATH HAS TWO INDEPENDENT BLOCKERS, AND EVERY NOTE IN THIS REPO NAMES ONLY ONE. + +The existing record — this file's sibling `sync-workflow-ir-callsite-allowlist.test.ts`, the +`postgres/sync-workflow-ir-is-always-default.pg.test.ts` proof, and a dozen FNXC notes across engine +and core, several of which I wrote — all say the same thing: `resolveTaskWorkflowIrSync` is inert +because `getTaskWorkflowSelectionImpl` returns `undefined` unconditionally, and the fix is "a +sync-capable workflow-selection reader". + +That understates the work by half. Fixing the selection reader alone would NOT un-inert the sync +path for the boards this program cares about. + +BLOCKER 2, proved below: `resolveTaskWorkflowIrSyncImpl` loads a CUSTOM workflow's IR with + + store.db.prepare("SELECT ir FROM workflows WHERE id = ?") + +and `TaskStore.db` is not "SQLite-only" — its implementation (`dbImpl`, task-id-integrity.ts) is an +UNCONDITIONAL throw with no mode branch at all. So that read always throws, is always swallowed by +the surrounding `catch`, and always returns the default IR. + +Consequence, and it is the one that matters here: a task bound to a BUILT-IN workflow could resolve +through the sync path today (that branch never touches `store.db`), but a task bound to a CUSTOM +workflow can never resolve, whatever the selection reader returns. Renamed lanes are by definition a +custom workflow. So the sync path cannot serve the renamed-board case at all until this read is +replaced too. + +BLOCKER 3 is a design constraint rather than a bug, recorded here because it bounds the shape of any +fix: Fusion supports multiple nodes running their own engines against ONE shared PostgreSQL +(`docs/multi-project.md` → "Shared Postgres multi-node runbook"). A node-local synchronous cache of +`task_workflow_selection` therefore goes stale whenever ANOTHER node rewrites a selection, and it +would answer with full confidence. That is strictly worse than today's honest default, which is at +least uniformly wrong rather than intermittently wrong. Any sync reader needs an invalidation story +that survives a writer on a different host. + +This file exists so the next person to attempt the unblock reads all three before starting, instead +of shipping a selection cache and discovering the second read at integration time. +*/ + +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { join, resolve } from "node:path"; + +const REPO_ROOT = resolve(__dirname, "../../../.."); + +function source(relPath: string): string { + return readFileSync(join(REPO_ROOT, relPath), "utf8"); +} + +describe("the sync workflow-IR path is blocked twice, not once", () => { + /* + Asserted against `dbImpl`'s SOURCE rather than by calling it, because calling it proves only that + one construction path throws. The claim is stronger and is what the fix depends on: there is no + mode in which this accessor returns a database, so no selection reader can rescue the read that + uses it. + */ + it("TaskStore.db is an unconditional throw — no SQLite branch survives", () => { + const body = source("packages/core/src/task-store/task-id-integrity.ts"); + const start = body.indexOf("export function dbImpl("); + expect(start).toBeGreaterThan(-1); + const fn = body.slice(start, body.indexOf("\n}", start)); + + expect(fn).toContain("throw new Error"); + /* If a mode branch is ever reintroduced, this file's premise changes and it must be re-read. */ + expect(fn).not.toMatch(/\bif\s*\(/); + expect(fn).not.toMatch(/\breturn\b/); + }); + + it("the sync IR resolver still loads custom workflows through that dead accessor", () => { + const body = source("packages/core/src/task-store/workflow-definitions.ts"); + const start = body.indexOf("export function resolveTaskWorkflowIrSyncImpl("); + expect(start).toBeGreaterThan(-1); + const fn = body.slice(start, body.indexOf("\n}", start)); + + /* The custom-workflow branch. Reaching it guarantees the catch, hence the default IR. */ + expect(fn).toContain("store.db"); + expect(fn).toContain("SELECT ir FROM workflows"); + expect(fn).toContain("catch"); + }); + + /* + ANTI-VACUITY. The two assertions above are about source text, so they would keep passing if the + sync resolver were deleted or renamed and the whole concern became moot. This pins that the + resolver is still exported and still the thing the allow-list guards. + */ + it("the resolver this file is about is still live", () => { + expect(source("packages/core/src/store.ts")).toContain("resolveTaskWorkflowIrSync"); + expect(source("packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts")) + .toContain("resolveTaskWorkflowIrSync"); + }); + + /* + The multi-node constraint is a documented product fact, not an inference. If that runbook ever goes + away, blocker 3 goes with it and a node-local cache becomes viable — so the fix's shape depends on + this line still being true. + */ + it("multiple nodes still share one PostgreSQL, which is what makes a node-local cache unsafe", () => { + expect(source("docs/multi-project.md")).toContain("Shared Postgres multi-node runbook"); + }); +});