From 6bc90ccbe26519dec2758fb9d7be6132408d8f44 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 08:45:31 -0700 Subject: [PATCH] =?UTF-8?q?fix(core):=20allow-list=20the=20legacy=20workfl?= =?UTF-8?q?ow=20IR=20=E2=80=94=20it=20found=20a=20fourth=20bug=20my=20grep?= =?UTF-8?q?=20missed=20(#3185)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The name is the defect `BUILTIN_CODING_WORKFLOW_IR` reads like the default and **is** the legacy workflow (`builtin:legacy-coding`). Post-U11 they differ by exactly one column — `triage` — the one a caller most often wants absent. **Four bugs have come from reaching for it by name:** 1. two move-path resolvers disagreed on the no-selection default → *"workflow move policy preflight is stale"* on every flag-on move (recorded in `resolveDefaultWorkflowIr`'s own header) 2. the TUI board rendered a `triage` lane the default board lacks — #3178 3. `deleteWorkflow` re-homed occupants into `triage` — #3183 4. **`board-workflows.ts`** described a *custom* workflow whose definition failed to load using legacy columns — the #3178 symptom through the dashboard route. **Fixed here.** It type-checks, it is the obvious identifier, and on the five shared columns it behaves correctly. The mistake only shows on the column that differs. ## I said the sweep was complete last round. It wasn't. My grep excluded paths and truncated at `head -10`; it missed two sites. **The allow-list found both on its first run.** That is the lesson the sibling sync-resolver ratchet already records — *"FOUND BY THIS RATCHET, not by the grep that seeded the list"* — and I had just quoted that file while repeating the mistake. ## One site is allow-listed rather than fixed, and I tried the fix first `workflow-graph-executor.run()`'s default `ir` is unreachable in production (both callers pass it explicitly). But `workflow-graph-executor-parity.test.ts`, in the **engine-core gate suite**, drives the method *without* the argument to assert the historical seam sequence. Switching it to the catalog default rewrites what "parity" means: **measured, 6 gate tests fail** with `expected 'failure' to be 'success'`. Reverted, and recorded at the call site *and* in the allow-list entry so nobody repeats the experiment. That is what an allow-list is for: a legitimate narrow use next to a plausible-looking wrong one. ## Guard construction Follows the repo's existing call-site allow-lists (sync resolver, engine blocking-shellout, detached-spawn script guard). - **Comments stripped before scanning** — `activity-analytics.ts` and `TaskContextMenu.tsx` name this constant in notes *about past bugs* while correctly avoiding it. Counting prose would train readers to allow-list mentions. - **Anti-vacuity**: the scan still sees the catalog's own uses, so a renamed constant or broken walker cannot make the guard pass by finding nothing. - **Stale-entry**: the list cannot rot into files that no longer touch it — the decay every ledger in this repo has hit. ## Measured - Guard **3/3**; `tsc --noEmit` clean in core, engine, dashboard. - census `--strict`, `check-fnxc-future-dates` clean. ## Census **No movement — that is the point.** This class has no column literal to count, which is why the census never saw any of the four bugs. Co-authored-by: Claude Opus 5 (1M context) --- ...acy-workflow-ir-callsite-allowlist.test.ts | 154 ++++++++++++++++++ .../dashboard/src/routes/board-workflows.ts | 12 +- .../engine/src/workflow-graph-executor.ts | 14 ++ 3 files changed, 178 insertions(+), 2 deletions(-) create mode 100644 packages/core/src/__tests__/legacy-workflow-ir-callsite-allowlist.test.ts diff --git a/packages/core/src/__tests__/legacy-workflow-ir-callsite-allowlist.test.ts b/packages/core/src/__tests__/legacy-workflow-ir-callsite-allowlist.test.ts new file mode 100644 index 0000000000..1663584b3a --- /dev/null +++ b/packages/core/src/__tests__/legacy-workflow-ir-callsite-allowlist.test.ts @@ -0,0 +1,154 @@ +/* +FNXC:WorkflowBuiltins 2026-07-31-23:59 (close the class, not the third instance): + +`BUILTIN_CODING_WORKFLOW_IR` READS LIKE THE DEFAULT AND IS THE LEGACY WORKFLOW. The catalog's default +is `builtin:coding` -> `resolveDefaultWorkflowIr()`; that constant is `builtin:legacy-coding`. Post-U11 +they differ by one column, and it is the one a caller most often wants: + + default todo, in-progress, in-review, done, archived + legacy triage, todo, in-progress, in-review, done, archived + +THREE SEPARATE BUGS HAVE COME FROM REACHING FOR IT BY NAME: + 1. the two move-path resolvers disagreed about the no-selection default, so every flag-ON move threw + "workflow move policy preflight is stale" (recorded in `resolveDefaultWorkflowIr`'s own header); + 2. the TUI board rendered a `triage` lane the default board does not have (#3178); + 3. `deleteWorkflow` re-homed every occupant of a deleted workflow into `triage` — a column the + default board does not declare — and slipped past `moveTask`'s undeclared-target rejection + because `triage` is a legacy id and the rehome runs under `recoveryRehome` (#3183). + +Each was found after it shipped, by someone tracing a symptom. The name is the defect: it is the +obvious identifier to reach for, it type-checks, and on the default board's five shared columns it +behaves correctly — so the mistake only shows on the column that differs. + +SO THIS IS A CALL-SITE ALLOW-LIST, the same shape the repo already uses for +`resolveTaskWorkflowIrSync`, the engine's blocking-shellout list, and the detached-spawn script guard, +and for the same reason: the primitive has a legitimate narrow use and a plausible-looking wrong one. + +TO ADD A SITE: say why the LEGACY workflow specifically is correct there — not "the built-in +workflow", which is the confusion this guards. If you want the catalog default, call +`resolveDefaultWorkflowIr()`. +*/ + +import { describe, expect, it } from "vitest"; +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join, relative, resolve } from "node:path"; + +const ALLOWED_CALL_SITES: ReadonlyMap = new Map([ + [ + "packages/core/src/builtin-workflows.ts", + "The catalog itself. It registers the constant AS `builtin:legacy-coding`, derives every linear " + + "built-in's columns from it via `canonicalBuiltinWorkflowColumns()` (deliberate — merging a " + + "column there propagates to all of them at once), and names it as the last-resort fallback " + + "inside `resolveDefaultWorkflowIr()`. This file is where 'legacy' is the right answer.", + ], + [ + "packages/engine/src/workflow-graph-executor.ts", + "`run()`'s default `ir`. Both production callers pass it explicitly, so the default is " + + "unreachable in production — but `workflow-graph-executor-parity.test.ts`, in the engine-core " + + "GATE suite, drives the method WITHOUT it to assert the historical seam sequence. Switching " + + "to the catalog default rewrites what 'parity' means: measured, 6 gate tests fail with " + + "\"expected 'failure' to be 'success'\". Tried, reverted, recorded here so the next reader " + + "does not repeat the experiment.", + ], +]); + +/** The constant's own declaration and the barrel re-exports are not call sites. */ +const EXCLUDED = [ + "packages/core/src/builtin-coding-workflow-ir.ts", + "packages/core/src/index.ts", + "packages/core/src/index.gate.ts", +]; + +const REPO_ROOT = resolve(__dirname, "../../../.."); +const SCAN_ROOTS = [ + "packages/core/src", + "packages/engine/src", + "packages/dashboard/src", + "packages/dashboard/app", + "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; + } +} + +/* +COMMENTS ARE STRIPPED FIRST, and that is load-bearing rather than tidiness. Several files discuss this +constant by name in FNXC notes explaining a past bug — `activity-analytics.ts` and +`TaskContextMenu.tsx` both do. Counting those would make the guard fire on files that mention the +hazard while correctly avoiding it, which trains readers to add allow-list entries for prose. The +sibling sync-resolver ratchet learned the same lesson. +*/ +function stripComments(source: string): string { + return source + .replace(/\/\*[\s\S]*?\*\//g, " ") + .replace(/(^|[^:])\/\/[^\n]*/g, "$1 "); +} + +function findUses(): 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("BUILTIN_CODING_WORKFLOW_IR")) continue; + const uses = (stripComments(source).match(/\bBUILTIN_CODING_WORKFLOW_IR\b/g) ?? []).length; + if (uses > 0) byFile.set(rel, uses); + } + } + return byFile; +} + +describe("the legacy workflow IR is reachable only where legacy is the right answer", () => { + it("has no unlisted call site", () => { + const unlisted = [...findUses().keys()].filter((file) => !ALLOWED_CALL_SITES.has(file)).sort(); + + expect(unlisted, [ + "", + "`BUILTIN_CODING_WORKFLOW_IR` is the LEGACY workflow (`builtin:legacy-coding`), not the", + "catalog default. It declares a `triage` column the default board does not have.", + "", + "If you want the no-selection default, call `resolveDefaultWorkflowIr()`.", + "If you genuinely need the legacy workflow, add an entry to ALLOWED_CALL_SITES in this file", + "saying why LEGACY specifically is correct there.", + "", + "Three shipped bugs came from reaching for this name: the move-path preflight mismatch,", + "the TUI board's phantom `triage` lane (#3178), and workflow-delete re-homing cards into", + "`triage` (#3183).", + "", + ].join("\n")).toEqual([]); + }); + + /* + ANTI-VACUITY. The case above passes trivially if the scan stops finding anything — a renamed + constant, a moved file, a broken walker. This pins that the one legitimate site is still seen. + */ + it("still finds the catalog's own uses, so the scan is not silently empty", () => { + const uses = findUses(); + + expect(uses.get("packages/core/src/builtin-workflows.ts")).toBeGreaterThan(0); + }); + + /* + The allow-list must not rot into a list of files that no longer touch the constant — a stale entry + reads as "someone considered this", which is the decay every ledger in this repo has hit. + */ + it("has no stale allow-list entry", () => { + const uses = findUses(); + const stale = [...ALLOWED_CALL_SITES.keys()].filter((file) => !uses.has(file)); + + expect(stale).toEqual([]); + }); +}); diff --git a/packages/dashboard/src/routes/board-workflows.ts b/packages/dashboard/src/routes/board-workflows.ts index 0118e4bdf0..765e8dc11e 100644 --- a/packages/dashboard/src/routes/board-workflows.ts +++ b/packages/dashboard/src/routes/board-workflows.ts @@ -19,7 +19,7 @@ const severityAuditLog = createLogger("dashboard-board-workflows"); */ import { - BUILTIN_CODING_WORKFLOW_IR, + resolveDefaultWorkflowIr, getBuiltinWorkflow, isBuiltinWorkflowId, parseWorkflowIr, @@ -190,7 +190,15 @@ async function describeWorkflow( } // Custom workflow: fetch the definition once and derive both IR and name from // it (previously getWorkflowDefinition was called twice per workflow). - let ir: WorkflowIr = BUILTIN_CODING_WORKFLOW_IR; + /* + FNXC:WorkflowBuiltins 2026-07-31-23:59: + THE CATALOG DEFAULT, NOT THE LEGACY IR. This is the placeholder a CUSTOM workflow's description + falls back to when `getWorkflowDefinition` returns nothing or throws. + `BUILTIN_CODING_WORKFLOW_IR` is `builtin:legacy-coding`, which declares a `triage` column the + default board does not have — so a workflow that failed to load was described with a phantom lane. + That is the #3178 symptom (TUI board rendering `triage`) reached through the dashboard route. + */ + let ir: WorkflowIr = resolveDefaultWorkflowIr(); let name = ir.name; let icon: string | undefined; try { diff --git a/packages/engine/src/workflow-graph-executor.ts b/packages/engine/src/workflow-graph-executor.ts index bb054d88f9..85579b031f 100644 --- a/packages/engine/src/workflow-graph-executor.ts +++ b/packages/engine/src/workflow-graph-executor.ts @@ -465,6 +465,20 @@ export class WorkflowGraphExecutor { public async run( task: TaskDetail, settings: (WorkflowNodeSettings & Partial>) | undefined, + /* + FNXC:WorkflowBuiltins 2026-07-31-23:59 (LEGACY ON PURPOSE — allow-listed, measured): + This default stays the LEGACY IR, and the reason is the parity suite. Both production callers + (`workflow-graph-task-runner.ts`, `workflow-task-runtime.ts`) pass `ir` explicitly, so the default + is unreachable in production — but `workflow-graph-executor-parity.test.ts` in the `engine-core` + GATE suite drives this method without it, asserting the historical seam sequence. Switching to the + catalog default rewrites what "parity" means: measured, 6 gate tests fail with + `expected 'failure' to be 'success'`. + + I tried the change first and reverted it. The entry in + `legacy-workflow-ir-callsite-allowlist.test.ts` records this so the next person does not repeat + the experiment — the constant is the right answer HERE, which is the distinction that allow-list + exists to preserve. + */ ir: WorkflowIr = BUILTIN_CODING_WORKFLOW_IR, startNodeId?: string, ): Promise {