From ed0df8b0e260136dda368fe44162646dcccafdf8 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 11:15:43 -0700 Subject: [PATCH] =?UTF-8?q?evidence:=20the=20self-healing=20sweeps=20do=20?= =?UTF-8?q?not=20RUN=20on=20a=20renamed=20board=20=E2=80=94=2049=20hardcod?= =?UTF-8?q?ed=20column=20QUERIES,=20and=2017/30=20fakes=20hide=20it=20(#28?= =?UTF-8?q?00)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Evidence only — no conversions, no behaviour change.** One doc, one test. It changes how the fleet should read the largest remaining file in the backlog. ## The finding, measured on `origin/main` `packages/engine/src/self-healing.ts` carries: - **97** lifecycle-column comparisons the census counts, and - **49** calls of the shape `this.store.listTasks({ column: "", … })`. `listTasks`' option is `column?: ColumnId` — **one literal column**, applied as a filter in the store. On a workflow whose lanes are renamed, every one of those 49 queries returns an **empty array**, so the sweep it feeds does nothing at all. **The self-healing sweeps are not mostly-correct-with-some-unconverted-guards. They never execute.** The `in-review` family alone is roughly half the calls: merge recovery, wedged merges, branch rebind, pending-step reconciliation. ## Why this matters to the census specifically ```ts const tasks = await this.store.listTasks({ column: "done", slim: true }); const candidates = tasks.filter((task) => task.column === "done" && // <-- the census counts THIS … ); ``` The census scores the **comparison**, not the query. Converting it is a legal-looking change that drops a count and changes **nothing an operator can observe** — the loop body still never runs, because the list was already empty. Roughly **31** of self-healing's remaining comparisons are this shape. Driving `self-healing.ts` to 0 would report the subsystem as converted while it stays inert on custom boards. In this file the census total is not merely a floor — it is actively misleading, and I'd rather the fleet know that before someone spends a week on the 97. ## Why the existing suite cannot see it Measured across `packages/engine/src/__tests__/self-healing*.test.ts`: - **30** files define a `listTasks` on their store fake. - **17** ignore the `column` option entirely. ```ts // representative of the 17 listTasks: vi.fn(async (options?: { limit?: number; offset?: number }) => { const all = [...tasksById.values()]; // options.column is never read return all.slice(offset, offset + limit); }), ``` The fake is **more permissive than production**. The sweep receives rows the real query would have filtered out, so the test proves the sweep's *logic* while saying nothing about whether the sweep is ever *reached*. A green self-healing suite is not evidence that self-healing runs. This is the mirror image of `store-fake-defects-that-masquerade-as-production-bugs.md`: there a fake is *missing* something production needs and the code looks broken; here it supplies *more* and the gap looks fixed. ## About the test It **pins a known defect** and is labelled as such in the file header — it asserts what the engine does today, which is the wrong thing. It asserts the **query argument**, not the outcome. The outcome is `0` either way, so an outcome assertion cannot distinguish *"nothing to do"* from *"asked the wrong question"*. Asserting the argument also avoids standing up the git-evidence path these sweeps enter once they have candidates. - **Ratchet proven to fire:** repointing `reconcileDoneTaskIntegrity`'s query at the renamed lane makes it fail — `1 failed | 2 passed`. A guard that reports success without checking anything is worse than no guard, so I ran it. - **Guard on the guard:** a first case asserts the renamed fixture really does resolve a complete lane that is not `done`. Without it, every later assertion could pass vacuously if the fixture ever collapsed to the default vocabulary. - **Control case:** shows the ignoring fake hands back a row whose column is `shipped` from a query that asked for `done` — the mechanism by which the suite stays green. When the query layer is fixed this test will fail, forcing an update. That is the intent. ## What I did NOT do, and why I did not fix it. `column?: ColumnId` takes one id, and the resolution is circular at the query layer — you need a task to know its workflow, and you are querying to find the tasks. A real fix is either a multi-column query option (`columns?: readonly ColumnId[]`) plus a resolved union across live workflow definitions, or dropping the filter and post-filtering by role in the engine. Either is a **behaviour change to a shared store API across 49 call sites**. That is a coordinator-level decision, not something a conversion PR should take unilaterally — the same reasoning that kept membership predicates out of the census. I'd take it on if you want it; it needs to be a deliberate call, not a side effect of a conversion sweep. ## Verification - `pnpm test:gate` — 161 + 487 + 13 + 71, green - `npx tsc -p packages/engine/tsconfig.json --noEmit` — clean - `pnpm lint` — clean --- ...ing-sweeps-are-blind-on-a-renamed-board.md | 91 ++++++++++ ...elf-healing-query-filter-blindness.test.ts | 163 ++++++++++++++++++ 2 files changed, 254 insertions(+) create mode 100644 docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md create mode 100644 packages/engine/src/__tests__/self-healing-query-filter-blindness.test.ts diff --git a/docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md b/docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md new file mode 100644 index 0000000000..4bf35d1276 --- /dev/null +++ b/docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md @@ -0,0 +1,91 @@ +--- +category: architecture-patterns +module: workflow-resolved-columns +date: 2026-07-30 +problem_type: systemic_gap +component: engine +severity: high +applies_when: + - "Converting a lifecycle-column literal in packages/engine/src/self-healing.ts" + - "Reading the lifecycle-column census total as the remaining work" + - "Writing or reviewing a test whose fake implements listTasks" +tags: + - workflow-resolved-columns + - column-census + - self-healing + - store-fake + - query-filter +--- + +# The self-healing sweeps do not run at all on a renamed board — and converting their 97 comparisons would not change that + +## The measurement + +`packages/engine/src/self-healing.ts` carries, on `origin/main` at the time of writing: + +- **97** lifecycle-column comparisons the census counts, and +- **49** calls of the shape `this.store.listTasks({ column: "", … })`. + +The second number is the one that matters. `listTasks`' option is `column?: ColumnId` — a **single literal column**, applied as a query filter in the store. On a workflow whose lanes are named anything else, every one of those 49 queries returns an **empty array**, so the sweep it feeds does nothing at all. + +That means the sweeps are not *mostly* correct with a few unconverted guards. They never execute. The `in-review` family alone accounts for roughly half the calls, which is the merge-recovery, wedged-merge, branch-rebind, and pending-step reconciliation surface. + +## Why the census points at the wrong thing here + +A sweep looks like this: + +```ts +const tasks = await this.store.listTasks({ column: "in-review", slim: true }); +for (const task of tasks) { + if (task.column !== "in-review") continue; // <-- the census counts THIS + … +} +``` + +The census scores the **comparison**, not the query. Converting the comparison to a resolved role is a legal-looking change that drops a census count and changes **nothing an operator can observe** — the loop body still never runs, because the list was already empty. + +This is the *query-filter-bounded* class. Roughly 31 of self-healing's remaining comparisons are re-assertions of a filter the query already applied. They are not conversion work; they are downstream of one architectural fix. + +**So the census total is a floor, and in this file it is actively misleading.** Driving `self-healing.ts` to 0 would report the subsystem as converted while it remains entirely inert on custom boards. + +## Why the test suite cannot see this + +Measured across `packages/engine/src/__tests__/self-healing*.test.ts`: + +- **30** test files define a `listTasks` on their store fake. +- **17** of them ignore the `column` option entirely — they return every seeded task regardless of what the sweep asked for. + +```ts +// self-healing-orphaned-pending-step-results.test.ts — representative of the 17 +listTasks: vi.fn(async (options?: { limit?: number; offset?: number }) => { + const all = [...tasksById.values()]; // `options.column` is not read + return all.slice(offset, offset + limit); +}), +``` + +The fake is **more permissive than production**. The sweep under test receives rows that the real query would have filtered out, so the test proves the sweep's *logic* while saying nothing about whether the sweep is ever *reached*. A green self-healing suite is therefore not evidence that self-healing runs. + +This is the mirror image of `store-fake-defects-that-masquerade-as-production-bugs.md`. There, a fake is missing a method the production path needs, so a branch silently does not run and the production code looks broken. Here the fake supplies **more** than production would, so the production gap looks fixed. + +## What an actual fix requires + +Not a literal conversion. `column?: ColumnId` accepts one id, and the resolution is circular at the query layer: you need a task to know its workflow, and you are querying to find the tasks. + +The two shapes that work: + +1. **Widen the query.** Add a multi-column option (`columns?: readonly ColumnId[]`), resolve the union of column ids carrying the wanted trait across all live workflow definitions, and pass that set. One extra read per sweep, no per-task resolution. +2. **Drop the filter and post-filter by role.** `listTasks({ slim: true })` then filter with the per-task resolved lane. Correct, but it moves a store-side filter into the engine for every sweep on every poll — a real cost on a large board. + +(1) is the better default. Either is a **behaviour change to a shared store API plus 49 call sites**, which is a coordinator-level decision, not something a conversion PR should take unilaterally — the same reasoning that kept membership predicates out of the census. + +## What to do until then + +- Do **not** convert a comparison that sits behind a column-filtered query in this file and report it as progress. Mark it, or leave it. +- When you touch a self-healing test, make its `listTasks` fake **honor `options.column`**. That is a one-line change per fake and it converts this whole class from invisible to failing-loudly. +- Read `self-healing.ts: N` in the census as "N comparisons", never as "N remaining defects" — in this file the two numbers are not related. + +## Related + +- `docs/solutions/test-failures/optional-flags-seam-hides-unconverted-column-guards.md` — the same lesson one level down: the census counts syntax, and a green suite that omits the new parameter carries no information about the change. +- `docs/solutions/architecture-patterns/sync-workflow-ir-readers-always-return-the-default.md` — the other way a conversion can look done and be inert. +- `docs/solutions/test-failures/store-fake-defects-that-masquerade-as-production-bugs.md` — the inverse fake defect. diff --git a/packages/engine/src/__tests__/self-healing-query-filter-blindness.test.ts b/packages/engine/src/__tests__/self-healing-query-filter-blindness.test.ts new file mode 100644 index 0000000000..bfd742e0e0 --- /dev/null +++ b/packages/engine/src/__tests__/self-healing-query-filter-blindness.test.ts @@ -0,0 +1,163 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-15:25 (batch-engine tail — the query-filter gap, made non-theoretical): + +THIS TEST PINS A KNOWN DEFECT. It asserts what the engine does TODAY, which is the wrong thing, and it +exists so the defect stops being invisible. See +`docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md`. + +THE DEFECT. `self-healing.ts` makes 49 calls of the shape +`this.store.listTasks({ column: "", … })`. `listTasks`' option is `column?: ColumnId` — ONE +literal column, applied as a filter in the store. On a workflow whose lanes are renamed, every one of +those queries returns an EMPTY array, so the sweep it feeds does nothing at all. The sweeps are not +mostly-correct-with-some-unconverted-guards; they never execute. + +WHY THIS ASSERTS THE QUERY AND NOT THE OUTCOME. The outcome is the same either way (the sweep returns +0), so an outcome assertion cannot distinguish "did nothing because there was nothing to do" from "did +nothing because it asked the wrong question". The QUERY ARGUMENT is where the defect actually lives, and +it is observable without standing up the git evidence path these sweeps run once they have candidates. + +WHY THE SUITE CANNOT SEE IT. Measured across `self-healing*.test.ts`: 30 files define a `listTasks` on +their store fake and 17 IGNORE the `column` option, returning every seeded task regardless of what the +sweep asked for. Those fakes are MORE PERMISSIVE than production, so the sweep under test receives rows +the real query would have filtered out. They prove the sweep's logic while saying nothing about whether +the sweep is ever reached — the mirror image of +`store-fake-defects-that-masquerade-as-production-bugs.md`. + +WHEN THE QUERY LAYER IS FIXED this test will fail, because the sweeps will stop asking for the bare +legacy literal. That is the intent — it is a ratchet on a known gap, not an endorsement of it. Rewrite +the expectations against the new query shape at that point and delete this note. + +The fix is NOT a literal conversion: `column?: ColumnId` takes one id, and resolution is circular at the +query layer (you need a task to know its workflow, and you are querying to find the tasks). It needs a +multi-column query option plus a resolved union across live workflows — a shared-store-API change across +49 call sites, which is a coordinator-level call. +*/ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import type { Settings, Task, TaskStore } from "@fusion/core"; +import { resolveLifecycleColumns } from "@fusion/core"; + +vi.mock("../run-audit.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + createRunAuditor: vi.fn(() => ({ database: vi.fn(async () => undefined), git: vi.fn(), filesystem: vi.fn(), sandbox: vi.fn() })), + }; +}); + +import { SelfHealingManager } from "../self-healing.js"; +import { executingTaskLock } from "../active-session-registry.js"; +import { RENAMED_VOCAB, lifecycleIr } from "./_workflow-vocabulary-fixture.js"; + +const RENAMED_IR = lifecycleIr(RENAMED_VOCAB, "self-healing-lifecycle", { mergeOrchestration: true }); + +/** + * A store fake that HONORS `options.column`, exactly as the real store does. + * + * That one line is the whole point of this file: the 17 self-healing fakes that drop the option on the + * floor are what keep this class invisible. + */ +function productionFaithfulStore(tasks: Task[]) { + const tasksById = new Map(tasks.map((entry) => [entry.id, entry])); + const listTasks = vi.fn(async (options?: { column?: string; limit?: number; offset?: number }) => { + let all = [...tasksById.values()]; + if (options?.column !== undefined) all = all.filter((entry) => entry.column === options.column); + const offset = options?.offset ?? 0; + return all.slice(offset, offset + (options?.limit ?? all.length)); + }); + const store = Object.assign(new EventEmitter(), { + getSettings: vi.fn(async () => ({ globalPause: false, enginePaused: false }) as Settings), + listTasks, + getTask: vi.fn(async (id: string) => tasksById.get(id)), + updateTask: vi.fn(async (id: string, patch: Partial) => { + const next = { ...tasksById.get(id)!, ...patch } as Task; + tasksById.set(id, next); + return next; + }), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "self-healing-lifecycle", stepIds: [] })), + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "self-healing-lifecycle", stepIds: [] })), + getWorkflowDefinition: vi.fn(async (id: string) => (id === "self-healing-lifecycle" ? { ir: RENAMED_IR } : undefined)), + }) as unknown as TaskStore & EventEmitter; + return { store, listTasks }; +} + +function shippedCard(): Task { + return { + id: "FN-BLIND", + title: "landed, but its merge evidence needs reconciling", + description: "", + column: RENAMED_VOCAB.complete, + dependencies: [], + steps: [], + currentStep: 0, + log: [], + modifiedFiles: ["packages/engine/src/x.ts"], + mergeDetails: {}, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as unknown as Task; +} + +describe("self-healing sweeps are bounded by a hardcoded column QUERY, not by their predicates", () => { + beforeEach(() => vi.clearAllMocks()); + afterEach(() => executingTaskLock._clearForTest()); + + it("the fixture's renamed board genuinely resolves a complete lane that is not `done`", async () => { + /* + Guard on the guard. If this ever stopped holding, every assertion below would pass vacuously — the + renamed board would BE the default board and the differential would mean nothing. + */ + const lifecycle = resolveLifecycleColumns(RENAMED_IR); + expect(lifecycle?.complete).toBe(RENAMED_VOCAB.complete); + expect(lifecycle?.complete).not.toBe("done"); + }); + + it("KNOWN DEFECT: the done-integrity sweep asks for the literal `done`, so a RENAMED board yields nothing", async () => { + /* + `reconcileDoneTaskIntegrity` opens with `listTasks({ column: "done", slim: true })` and then + re-asserts `task.column === "done"` on the rows it gets back. The census counts that re-assertion; + converting it would drop a count and change nothing, because the list was already empty. + + The card below HAS landed and HAS modified files with no recorded commit sha — exactly the state the + sweep exists to repair. It sits in `shipped`, so the store returns no rows and the repair never runs. + */ + const { store, listTasks } = productionFaithfulStore([shippedCard()]); + const manager = new SelfHealingManager(store, { rootDir: "/repo" }); + + expect(await manager.reconcileDoneTaskIntegrity()).toBe(0); + + // The query asked for the legacy literal, NOT this workflow's resolved complete lane. + expect(listTasks).toHaveBeenCalledWith(expect.objectContaining({ column: "done" })); + expect(listTasks).not.toHaveBeenCalledWith(expect.objectContaining({ column: RENAMED_VOCAB.complete })); + + // And the card is untouched: still no commit sha, still unreconciled. + expect((await store.getTask("FN-BLIND"))?.mergeDetails?.commitSha).toBeUndefined(); + }); + + it("proves the fake is what hides it: an ignoring `listTasks` hands the sweep rows production would not", async () => { + /* + The control, and the reason a green self-healing suite is not evidence that self-healing runs. This + fake is the shape 17 of the 30 self-healing suites use — it drops `options.column` on the floor, so + the renamed-board card comes back from a query that asked for `done`. + + Asserted on the RETURNED ROWS rather than on the sweep, so this stays true regardless of what the + sweep does with them. + */ + const card = shippedCard(); + /* The 17-fake shape: the option is declared so the call is realistic, and then never read. */ + const permissiveList = vi.fn(async (_options?: { column?: string }) => [card]); + + /* Asked for `done` — exactly what the sweep asks — and got back a card in `shipped`. */ + const rows = await permissiveList({ column: "done" }); + + expect(permissiveList).toHaveBeenCalledWith({ column: "done" }); + expect(rows).toHaveLength(1); + expect(rows[0].column).toBe(RENAMED_VOCAB.complete); + expect(rows[0].column).not.toBe("done"); + + /* The contrast that makes the point: the production-faithful fake, asked the same question, + returns nothing. Same card, same query, opposite answer — the fake IS the hiding mechanism. */ + const { store } = productionFaithfulStore([card]); + expect(await store.listTasks({ column: "done" as never })).toHaveLength(0); + }); +});