evidence: the self-healing sweeps do not RUN on a renamed board — 49 hardcoded column QUERIES, and 17/30 fakes hide it (#2800)
**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: "<literal>",
… })`.
`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
This commit is contained in:
@@ -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: "<literal>", … })`.
|
||||
|
||||
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.
|
||||
@@ -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: "<literal>", … })`. `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<typeof import("../run-audit.js")>();
|
||||
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<Task>) => {
|
||||
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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user