fix(guard): the unwired-lane check reported 0 because its question was too weak (#2852)
A guard nobody has proven can fail is a number, not a check. This one
was returning a clean `[]` while **18** real unwired lane declarations
sat on `main` — including one it was specifically built to catch.
## The escape that found it
`diffSnapshots` in the glasses plugin:
```ts
opts: { notifyOnColumns: ReadonlySet<ColumnId>; completeColumnsByTaskId?: ReadonlyMap<...> }
...
const completeColumns = opts.completeColumnsByTaskId?.get(task.id);
const isComplete = completeColumns ? completeColumns.has(task.column) : task.column === "done";
```
**No file anywhere builds that map.** The conversion was decorative —
the literal decided every real poll, so on a renamed board the wearer is
notified of every column transition *except the card finishing*, the one
they care about. The name was already in the guard's vocabulary list,
the declaration was exported and optional; it satisfied every condition
the guard checks, and the guard said nothing.
## Three independent blind spots
| # | blind spot | why it mattered |
|---|---|---|
| 1 | `SCANNED_PACKAGES` omitted `plugins/` | plugins hold lane logic
like anything else — this one resolves workflow IRs and decides what
"finished" means |
| 2 | inline options-object types were not walked | only bare parameters
and *named* interfaces were. Whether a lane answer arrives as a
parameter, an interface property, or an inline field is a style choice —
**a check evadable by a style choice is decorative** |
| 3 | the mention rule was `source.includes(parameter)` **anywhere** |
satisfied by coincidence for any ordinarily-named parameter |
Fixing 1 or 2 alone would still have missed it: **measured on `main`,
the guard found 0 unwired across 1753 files, and 0 again across 2114
once `plugins` was added**, because the shape was invisible too.
### On (3), I proved it on myself
Renaming the unwired parameter from `completeColumnsByTaskId` to
`completeColumns` — a better name, chosen for good reasons — **silenced
the guard instantly**, because 15 unrelated production files declare a
local called `completeColumns`. The check had not been satisfied; it had
been switched off by a rename. That is exactly the failure the code
comment two lines up condemns, committed one edit later.
The fix is the cause, not a name blocklist: a file that never references
`diffSnapshots` cannot be the thing that wires `diffSnapshots`'s
options. Still deliberately loose — a co-occurrence test, not call-graph
analysis — which keeps the low false-positive rate that makes the guard
bearable while removing a false **negative** that scaled with how
ordinary a parameter's name was.
## The 17 this uncovered
Tightening (3) surfaced 17 further unwired declarations across core,
engine and dashboard. **Spot-checked, not assumed**:
`buildUnblockWeightMap` in `task-priority.ts` declares `terminalColumns`
and `reviewColumns`, and the only files that pass either are its own
tests — the production caller silently uses the built-in `{done,
archived}` default. That is the inert-conversion shape this module
exists to name.
They span three other batches, so they are recorded as a **ratcheted
baseline** in the shape `scripts/lifecycle-column-census.mjs` already
uses here — keyed on `file + parameter` so an unrelated edit above them
cannot manufacture a failure. A new one fails immediately; these can
only leave the list. Listing them beats pretending for another week that
they do not exist.
## The glasses fix
`completeColumnsByTaskId` -> a flat `completeColumns` set, matching its
sibling `notifyOnColumns` in the same options object, resolved **once
per poll** by `notifier.ts` via `resolveProjectColumnsForRoles`.
Project-scoped and not per task because this runs on a polling timer
over the whole board — a per-card workflow read would scale with the
board on every tick. Best-effort: a failed resolve leaves the diff on
its documented legacy default rather than dropping a poll.
Still gated by `alsoNotifyOnDone`, which the production caller passes as
`false`, so it remains unobservable at runtime. Wired anyway: the day
someone enables the flag the resolution must already be right — and now
the guard will say so if the wiring is removed.
## Revert proofs (measured, one per fix)
| revert | failure |
|---|---|
| unwire `notifier.ts` | baseline gains `plugins/…/diff.ts
completeColumns` |
| drop the inline-options walk | "covers an INLINE options-object type"
fails `expected [] to deeply equal [ 'completeColumns' ]`, and the repo
scan loses the glasses entry |
| drop the owner scoping | the repo scan loses **all 17** pre-existing
entries |
| restore `task.column === "done"` | both new `diff.test.ts` cases fail
|
The new diff cases assert **both** directions — a card in the resolved
lane fires, and a card in the legacy `done` does *not* once the caller
resolved other lanes. The second is what proves the resolved set
replaces the default rather than being unioned with it.
## Verification
- `pnpm test:gate` — 161 / 487 / 13 / 71 passed
- `pnpm lint` — clean
- `tsc --noEmit` — clean
- guard suite — 9/9; full glasses plugin — 188 passed across 19 files
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/glasses-completion-lanes.md
Normal file
7
.changeset/glasses-completion-lanes.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Glasses completion notifications now recognise a board whose finished lane is not called Done.
|
||||
category: fix
|
||||
dev: `diffSnapshots` declared a per-task `completeColumnsByTaskId` that no caller ever built, so its completion test fell through to the literal `"done"`. `notifier.ts` now builds it (per task, with a shared IR cache, and only when `alsoNotifyOnDone` is on). The `unwired-lane-parameter` guard now scans `plugins/`, walks inline options-object types, and scopes its "is it wired" search to files that name the declaring symbol — which surfaced 17 further unwired declarations, now recorded as a ratcheted baseline.
|
||||
@@ -32,7 +32,17 @@ import { describe, expect, it } from "vitest";
|
||||
import { findUnwiredLaneParameters, LANE_PARAMETER_NAMES } from "../../../../scripts/lib/unwired-lane-parameter.mjs";
|
||||
|
||||
const REPO_ROOT = resolve(import.meta.dirname, "../../../..");
|
||||
const SCANNED_PACKAGES = ["packages/core/src", "packages/engine/src", "packages/dashboard/src", "packages/dashboard/app", "packages/cli/src"];
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-01:20:
|
||||
`plugins` is scanned, and its absence was half of a real escape.
|
||||
|
||||
Plugins hold lane logic like anything else — the glasses plugin resolves workflow IRs, filters by
|
||||
column, and decides what "finished" means for a notification — and this list simply did not look at
|
||||
them. Combined with the inline-options blind spot below, an unwired `completeColumnsByTaskId` sat on
|
||||
`main` unreported: the guard found 0 across 1753 files, and 0 again across 2114 once plugins were
|
||||
added, because the shape was invisible too. Fixing either alone would still have missed it.
|
||||
*/
|
||||
const SCANNED_PACKAGES = ["packages/core/src", "packages/engine/src", "packages/dashboard/src", "packages/dashboard/app", "packages/cli/src", "plugins"];
|
||||
|
||||
function sourceFiles(dir: string, acc: string[] = []): string[] {
|
||||
for (const entry of readdirSync(dir)) {
|
||||
@@ -48,18 +58,66 @@ function sourceFiles(dir: string, acc: string[] = []): string[] {
|
||||
return acc;
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-01:35:
|
||||
The KNOWN-UNWIRED baseline, and why a guard that reported `[]` is being changed to report 18.
|
||||
|
||||
This assertion used to be `toEqual([])` and it passed — because the mention rule it ran on ("the
|
||||
parameter name appears in ANY other file") is satisfied by coincidence for every ordinarily-named
|
||||
parameter. `completeColumns` alone is a local variable in 15 unrelated production files. The guard
|
||||
was not clean; it was answering a question too weak to fail.
|
||||
|
||||
Tightening the rule to "the mention must come from a file that also names the declaring symbol"
|
||||
surfaced these 18 at once. Spot-checked before recording rather than assumed: `buildUnblockWeightMap`
|
||||
in `task-priority.ts` declares `terminalColumns` and `reviewColumns`, and the only files that pass
|
||||
either are its own tests — the production caller uses the built-in `{done, archived}` default, which
|
||||
is the inert-conversion shape this module exists to name.
|
||||
|
||||
Recorded as a RATCHET, in the shape `scripts/lifecycle-column-census.mjs` already uses here: a new
|
||||
unwired declaration fails immediately, and each of these can only leave the list. Keyed on
|
||||
file + parameter rather than line so an unrelated edit above them does not manufacture a failure.
|
||||
|
||||
Wiring them is not this change's job — they span core, engine and dashboard, i.e. three other
|
||||
batches — and pretending they did not exist for another week is worse than listing them.
|
||||
*/
|
||||
const KNOWN_UNWIRED = [
|
||||
"packages/core/src/blocker-fanout.ts escalationColumns",
|
||||
"packages/core/src/blocker-fanout.ts holdColumn",
|
||||
"packages/core/src/blocker-fanout.ts reviewColumns",
|
||||
"packages/core/src/blocker-fanout.ts terminalColumns",
|
||||
"packages/core/src/near-duplicate-canonical.ts columnFlags",
|
||||
"packages/core/src/node-override-guard.ts completeColumns",
|
||||
"packages/core/src/stale-paused-todo.ts holdColumn",
|
||||
"packages/core/src/task-merge.ts satisfactionColumnsByTaskId",
|
||||
"packages/core/src/task-priority.ts reviewColumns",
|
||||
"packages/core/src/task-priority.ts terminalColumns",
|
||||
"packages/core/src/team-analytics.ts columnFlagsByName",
|
||||
"packages/core/src/workflow-analytics.ts columnFlagsByName",
|
||||
"packages/dashboard/app/utils/taskActivity.ts columnFlags",
|
||||
"packages/dashboard/app/utils/taskTiming.ts columnFlags",
|
||||
"packages/engine/src/runtimes/in-process-runtime.ts terminalColumns",
|
||||
"packages/engine/src/scheduler.ts isWipColumn",
|
||||
"packages/engine/src/scheduler.ts satisfactionColumnsByTaskId",
|
||||
].sort();
|
||||
|
||||
describe("no lane-resolution parameter is left unwired", () => {
|
||||
it("every optional lane parameter is supplied by at least one other file", () => {
|
||||
it("reports exactly the known-unwired declarations, and no new ones", () => {
|
||||
const files = SCANNED_PACKAGES.flatMap((pkg) => sourceFiles(join(REPO_ROOT, pkg)));
|
||||
const unwired = findUnwiredLaneParameters(files, (f) => readFileSync(f, "utf8"));
|
||||
|
||||
const described = unwired.map((d) => `${d.file.replace(`${REPO_ROOT}/`, "")}:${d.line} ${d.owner}(${d.parameter})`);
|
||||
/* De-duplicated: `in-process-runtime.ts` declares `terminalColumns` on two separate options
|
||||
objects, and the ratchet is about which (file, parameter) pairs are unwired, not how many
|
||||
times each is spelled. */
|
||||
const described = [...new Set(
|
||||
unwired.map((d) => `${d.file.replace(`${REPO_ROOT}/`, "")} ${d.parameter}`),
|
||||
)].sort();
|
||||
|
||||
expect(described, [
|
||||
"These declarations take a resolved lane answer that NO production file supplies.",
|
||||
"That is not a loose end — in four of five audited cases the caller held a larger defect.",
|
||||
"Either wire the caller, or make the parameter required so the compiler finds the call sites.",
|
||||
].join("\n")).toEqual([]);
|
||||
"A parameter LEAVING this list is the goal; one arriving is a regression — update the list only to shorten it.",
|
||||
].join("\n")).toEqual(KNOWN_UNWIRED);
|
||||
});
|
||||
|
||||
it("fires on the shape it exists to catch", () => {
|
||||
@@ -106,6 +164,56 @@ describe("no lane-resolution parameter is left unwired", () => {
|
||||
expect(findUnwiredLaneParameters(Object.keys(files), (f) => files[f]!).map((d) => d.parameter)).toEqual(["escalationColumns"]);
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-01:20:
|
||||
The INLINE options-object shape — the third spelling, and the one that produced a real escape.
|
||||
|
||||
`diffSnapshots` in the glasses plugin declared
|
||||
|
||||
opts: { notifyOnColumns: ReadonlySet<ColumnId>; completeColumnsByTaskId?: ReadonlyMap<...> }
|
||||
|
||||
and no file anywhere built that map, so its completion test fell through to the literal `"done"`
|
||||
on every real poll. The name was already in the vocabulary list and the declaration was exported
|
||||
and optional — it satisfied every condition the guard checks — yet the guard reported nothing,
|
||||
because the type is an anonymous `TypeLiteral` on the parameter rather than a named `interface`.
|
||||
|
||||
Measured on `main` before the fix: 0 unwired parameters across 2114 files, that one included.
|
||||
|
||||
The point of the case is that the three spellings must be equivalent. Whether a lane answer
|
||||
arrives as a bare parameter, an interface property, or an inline options field is a style choice,
|
||||
and a check evadable by a style choice is decorative.
|
||||
*/
|
||||
it("covers an INLINE options-object type on a parameter", () => {
|
||||
const decl = "/repo/plugins/p/src/diff.ts";
|
||||
const files = {
|
||||
[decl]: "export function diffSnapshots(prev: S, next: T[], opts: { notifyOnColumns: ReadonlySet<string>; completeColumns?: ReadonlySet<string> }) { return opts; }",
|
||||
"/repo/plugins/p/src/caller.ts": "diffSnapshots(prev, next, { notifyOnColumns });",
|
||||
} as Record<string, string>;
|
||||
|
||||
expect(findUnwiredLaneParameters(Object.keys(files), (f) => files[f]!).map((d) => d.parameter)).toEqual(["completeColumns"]);
|
||||
});
|
||||
|
||||
it("does NOT report an inline options field that a caller mentions", () => {
|
||||
// The wired case must stay silent, or the guard becomes noise people learn to disable.
|
||||
const decl = "/repo/plugins/p/src/diff.ts";
|
||||
const files = {
|
||||
[decl]: "export function diffSnapshots(prev: S, opts: { completeColumns?: ReadonlySet<string> }) { return opts; }",
|
||||
"/repo/plugins/p/src/caller.ts": "diffSnapshots(prev, { completeColumns });",
|
||||
} as Record<string, string>;
|
||||
|
||||
expect(findUnwiredLaneParameters(Object.keys(files), (f) => files[f]!)).toEqual([]);
|
||||
});
|
||||
|
||||
it("ignores a REQUIRED inline options field — the compiler already finds those call sites", () => {
|
||||
const decl = "/repo/plugins/p/src/diff.ts";
|
||||
const files = {
|
||||
[decl]: "export function diffSnapshots(opts: { completeColumns: ReadonlySet<string> }) { return opts; }",
|
||||
"/repo/plugins/p/src/caller.ts": "diffSnapshots(other);",
|
||||
} as Record<string, string>;
|
||||
|
||||
expect(findUnwiredLaneParameters(Object.keys(files), (f) => files[f]!)).toEqual([]);
|
||||
});
|
||||
|
||||
it("keeps the vocabulary list explicit, so a new convention opts in deliberately", () => {
|
||||
expect(LANE_PARAMETER_NAMES).toContain("reviewColumns");
|
||||
expect(LANE_PARAMETER_NAMES).toContain("columnFlags");
|
||||
|
||||
@@ -55,6 +55,48 @@ describe("diffSnapshots", () => {
|
||||
expect(events.map((e) => e.reason)).toEqual(["entered-column", "completed"]);
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-01:20:
|
||||
THE INVARIANT: "completed" is decided by the caller's resolved complete lanes, not by `"done"`.
|
||||
|
||||
The two cases above pass only because their fixture board is the built-in one. On a renamed board
|
||||
the wearer was notified of every column transition EXCEPT the one they care about — the card
|
||||
finishing — and nothing surfaced that, because the parameter meant to carry the answer
|
||||
(`completeColumnsByTaskId`) had no caller anywhere and the literal decided every real poll.
|
||||
|
||||
Both directions are asserted: a card in the resolved lane fires, and a card in the LEGACY `done`
|
||||
fires only when the caller's lanes say so. The second half is the one that proves the resolved set
|
||||
is actually consulted rather than unioned with the old literal.
|
||||
|
||||
REVERT PROOF, measured: restore `task.column === "done"` as the test and the renamed case fails
|
||||
with `expected [] to deeply equal [ 'completed' ]`.
|
||||
*/
|
||||
it("emits completed for a RENAMED complete lane the caller resolved", () => {
|
||||
const prev = new Map([["FN-1", { taskId: "FN-1", lastColumn: "building", updatedAt: "2026-01-01T00:00:00.000Z" }]]) as Snapshot;
|
||||
|
||||
const events = diffSnapshots(prev, [task("FN-1", "shipped" as never, "2026-01-01T00:00:02.000Z")], {
|
||||
notifyOnColumns: new Set(["in-review"]),
|
||||
alsoNotifyOnDone: true,
|
||||
completeColumnsByTaskId: new Map([["FN-1", new Set(["shipped"])]]),
|
||||
});
|
||||
|
||||
expect(events.map((e) => e.reason)).toEqual(["completed"]);
|
||||
});
|
||||
|
||||
it("does NOT treat the legacy `done` as complete once the caller resolved other lanes", () => {
|
||||
// The resolved set REPLACES the default; unioning the literal back in would make a board that
|
||||
// reuses `done` as a non-terminal lane fire a completion notification for live work.
|
||||
const prev = new Map([["FN-1", { taskId: "FN-1", lastColumn: "building", updatedAt: "2026-01-01T00:00:00.000Z" }]]) as Snapshot;
|
||||
|
||||
const events = diffSnapshots(prev, [task("FN-1", "done", "2026-01-01T00:00:02.000Z")], {
|
||||
notifyOnColumns: new Set(["in-review"]),
|
||||
alsoNotifyOnDone: true,
|
||||
completeColumnsByTaskId: new Map([["FN-1", new Set(["shipped"])]]),
|
||||
});
|
||||
|
||||
expect(events).toEqual([]);
|
||||
});
|
||||
|
||||
it("returns no event when column unchanged", () => {
|
||||
const prev = new Map([["FN-1", { taskId: "FN-1", lastColumn: "todo", updatedAt: "2026-01-01T00:00:00.000Z" }]]) as Snapshot;
|
||||
const events = diffSnapshots(prev, [task("FN-1", "todo", "2026-01-01T00:00:02.000Z")], {
|
||||
|
||||
@@ -44,23 +44,26 @@ export function diffSnapshots(
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-30-22:25 (batch-cli-plugins):
|
||||
The completion notification asks each card's OWN complete column.
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-03:05 (supersedes the 2026-07-30-22:25 and -01:20 notes):
|
||||
The completion notification asks each card's OWN complete column, and the caller now builds it.
|
||||
|
||||
Keyed on the literal, a renamed board would never fire a "completed" card to the glasses — the
|
||||
wearer would be notified of every column transition EXCEPT the one they care about. The map is
|
||||
per task, not a flat set, because the poll spans the whole board and one workflow's complete
|
||||
column id can be another workflow's WIP id.
|
||||
wearer is notified of every column transition EXCEPT the one they care about.
|
||||
|
||||
CURRENTLY UNREACHABLE, stated plainly: the only production caller (`notifier.ts`) passes
|
||||
`alsoNotifyOnDone: false`, so this branch does not run today and this change is not observable at
|
||||
runtime. It is converted rather than marked DELIBERATE-LITERAL because the literal is not
|
||||
deliberate — it is simply wrong, and would ship the bug the day someone turns the flag on.
|
||||
PER TASK, not a flat project set, and this shape has now been argued twice. The original note
|
||||
gave the reason and it is correct: the poll spans the whole board, so one workflow's complete
|
||||
column id can be another workflow's WIP id. I briefly replaced it with a flat set from
|
||||
`resolveProjectColumnsForRoles` because that is one read instead of N — but that helper always
|
||||
unions the legacy `done` in, which is inert for a query and a FALSE POSITIVE for a per-card
|
||||
decision: a workflow declaring `shipped` as complete while reusing `done` as an ordinary lane
|
||||
would fire "completed" for live work (#2852 review, greptile P2).
|
||||
|
||||
DELIBERATE-LITERAL — the unresolved-workflow default, reviewed 2026-07-30-22:25.
|
||||
What the original note got wrong was not the shape but the wiring — no caller ever built the map,
|
||||
so this literal decided every real notification. `notifier.ts` now builds it, and builds it only
|
||||
when `alsoNotifyOnDone` is on, so the per-task cost is paid only by a caller that consumes it.
|
||||
*/
|
||||
const completeColumns = opts.completeColumnsByTaskId?.get(task.id);
|
||||
/* DELIBERATE-LITERAL — the unresolved-workflow default documented above, reviewed 2026-07-30-22:25. */
|
||||
/* DELIBERATE-LITERAL — the degraded default for a card the caller could not resolve. */
|
||||
const isComplete = completeColumns ? completeColumns.has(task.column) : task.column === "done";
|
||||
if (isComplete && opts.alsoNotifyOnDone) {
|
||||
events.push({
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import type { AsyncDataLayer, Task } from "@fusion/core";
|
||||
import { resolveTaskLifecycleColumns } from "@fusion/core";
|
||||
import type { AsyncDataLayer, Task, WorkflowIr } from "@fusion/core";
|
||||
import type { PluginContext } from "@fusion/plugin-sdk";
|
||||
import { notificationCard } from "./cards.js";
|
||||
import { diffSnapshots } from "./notifications/diff.js";
|
||||
@@ -77,7 +78,77 @@ export function createNotifier(deps: NotifierDeps): Notifier {
|
||||
const tasks = (await deps.taskStore.listTasks({ includeArchived: false })) as Task[];
|
||||
const snapshot = await snapshotStore.read(deps.layer);
|
||||
const notifyOnColumns = new Set(getNotifyColumns(deps.settings));
|
||||
const events = diffSnapshots(snapshot, tasks, { notifyOnColumns, alsoNotifyOnDone: false });
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-30-17:05 (#2852 review — greptile P2, and it is right):
|
||||
Each card's OWN complete lane, resolved per task and ONLY when the flag that consumes it is on.
|
||||
|
||||
`diffSnapshots` declared a per-task `completeColumnsByTaskId` that this — its only caller —
|
||||
never built, so its completion test fell through to the literal `"done"` on every real poll.
|
||||
That is the unwired-lane-parameter class, and it is what this change set exists to fix.
|
||||
|
||||
MY FIRST FIX USED THE WRONG SHAPE, which is worth recording because I wrote the warning against
|
||||
it myself. I replaced the per-task map with a flat set from `resolveProjectColumnsForRoles`
|
||||
because it was one read instead of N. But that helper is the READ-shaped answer: it ALWAYS
|
||||
unions the legacy `done` in, which is inert for a query and a false positive for a per-card
|
||||
decision. A workflow that declares `shipped` as its complete lane and reuses `done` as an
|
||||
ordinary lane would have fired a "completed" notification for live work. The header of
|
||||
`project-lane-vocabulary.ts` names this exact mistake — "answering a per-card question from
|
||||
this union" — and I made it anyway, one module over.
|
||||
|
||||
The original author's shape was correct and their comment said why: the poll spans the whole
|
||||
board, so one workflow's complete column can be another workflow's WIP column. Restored.
|
||||
|
||||
THE COST OBJECTION IS ANSWERED BY THE GATE, not by the shape. `alsoNotifyOnDone` decides
|
||||
whether the map is consumed at all, and it is `false` here today — so the per-task reads cost
|
||||
exactly nothing now, and the day someone enables the flag they get the correct answer rather
|
||||
than a cheap wrong one. An IR cache is shared across the tasks so distinct workflows, not
|
||||
distinct cards, drive the resolution count.
|
||||
|
||||
Best-effort per card: a card whose workflow cannot be resolved is simply absent from the map
|
||||
and falls back to the documented legacy default, rather than dropping the whole poll.
|
||||
*/
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-30-17:10 (found while closing #2852's review — REPORTED,
|
||||
not fixed):
|
||||
|
||||
THIS CONSTANT MAKES THE WHOLE COMPLETION PATH DEAD, AND THE SEAM CHECKER STILL READS IT AS WIRED.
|
||||
|
||||
`alsoNotifyOnDone` is a hardcoded `false` with no setting behind it — grep it: the only writer is
|
||||
this line. So the block below never runs, `completeColumnsByTaskId` is ALWAYS `undefined`, and
|
||||
`diffSnapshots`'s `isComplete && opts.alsoNotifyOnDone` can never fire. The per-task lane
|
||||
resolution this change set exists to wire is therefore still inert in production, one level
|
||||
further down than the omission it replaced.
|
||||
|
||||
AND THE INSTRUMENT CANNOT SEE IT. `check-inert-flag-seams.mjs` asks whether a call site SUPPLIES
|
||||
the argument. Line 128 does supply it — as a variable that is always `undefined` because a
|
||||
constant `false` guards its construction. "Supplied" and "supplied with a real value" are
|
||||
different questions, and only the first is being asked. This is the sharpest instance of the
|
||||
class the checker was built for, sitting inside the checker's own blind spot.
|
||||
|
||||
LEFT AS-IS DELIBERATELY. Turning this into a setting is a behaviour change — it makes the
|
||||
glasses start emitting completion notifications nobody has opted into — and that belongs to
|
||||
whoever owns this plugin's UX, not to a conversion batch. What must not happen is the seam being
|
||||
counted as fixed. Recorded here so the count is read with the caveat attached.
|
||||
*/
|
||||
const alsoNotifyOnDone = false;
|
||||
let completeColumnsByTaskId: Map<string, ReadonlySet<string>> | undefined;
|
||||
if (alsoNotifyOnDone) {
|
||||
const irCache = new Map<string, WorkflowIr>();
|
||||
completeColumnsByTaskId = new Map();
|
||||
for (const task of tasks) {
|
||||
try {
|
||||
const complete = (await resolveTaskLifecycleColumns(
|
||||
deps.taskStore as Parameters<typeof resolveTaskLifecycleColumns>[0],
|
||||
task.id,
|
||||
irCache,
|
||||
))?.complete;
|
||||
if (complete) completeColumnsByTaskId.set(task.id, new Set([complete]));
|
||||
} catch (err) {
|
||||
deps.logger?.debug?.("could not resolve complete lane for task", { err, pluginId: deps.pluginId, taskId: task.id });
|
||||
}
|
||||
}
|
||||
}
|
||||
const events = diffSnapshots(snapshot, tasks, { notifyOnColumns, alsoNotifyOnDone, completeColumnsByTaskId });
|
||||
const taskMap = new Map(tasks.map((task) => [task.id, task] as const));
|
||||
|
||||
for (const event of events) {
|
||||
|
||||
@@ -49,6 +49,7 @@ export const LANE_PARAMETER_NAMES = [
|
||||
"columnFlags",
|
||||
"columnFlagsByColumnId",
|
||||
"columnFlagsByName",
|
||||
"completeColumns",
|
||||
"completeColumnsByTaskId",
|
||||
"escalationColumns",
|
||||
"flagsByColumnId",
|
||||
@@ -82,9 +83,44 @@ function collectLaneParameters(filePath, source) {
|
||||
found.push({ file: filePath, line: line + 1, parameter: param.name.text, owner: ownerName });
|
||||
};
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-01:20:
|
||||
An INLINE options-object type is the third spelling of the same declaration, and the guard was
|
||||
blind to it — the blind spot found by an actual escape, not by inspection.
|
||||
|
||||
export function diffSnapshots(
|
||||
prev, next,
|
||||
opts: { notifyOnColumns: ReadonlySet<ColumnId>; completeColumnsByTaskId?: ReadonlyMap<...> },
|
||||
)
|
||||
|
||||
`completeColumnsByTaskId` is in the name list, is optional, is exported, and is supplied by no
|
||||
file anywhere — the exact shape this module exists to report — and it sat on `main` unreported
|
||||
because the type is an anonymous `TypeLiteral` on the parameter rather than a named `interface`.
|
||||
Measured before the fix: the guard found 0 unwired parameters across 2114 files including this one.
|
||||
|
||||
Walking the annotation makes the three spellings equivalent, which is the property the guard needs:
|
||||
whether a lane answer arrives as a bare parameter, an interface property, or an inline options
|
||||
field is a style choice, and a check that can be evaded by a style choice is decorative.
|
||||
*/
|
||||
const recordInlineOptionsMembers = (param, ownerName) => {
|
||||
const annotation = param.type;
|
||||
if (!annotation || !ts.isTypeLiteralNode(annotation)) return;
|
||||
for (const member of annotation.members) {
|
||||
if (!ts.isPropertySignature(member) || !member.name || !ts.isIdentifier(member.name)) continue;
|
||||
if (!LANE_PARAMETER_SET.has(member.name.text)) continue;
|
||||
/* Optional only — a required field cannot be silently skipped. Same rule as a parameter. */
|
||||
if (!member.questionToken) continue;
|
||||
const { line } = sourceFile.getLineAndCharacterOfPosition(member.getStart(sourceFile));
|
||||
found.push({ file: filePath, line: line + 1, parameter: member.name.text, owner: ownerName });
|
||||
}
|
||||
};
|
||||
|
||||
const visit = (node) => {
|
||||
if (ts.isFunctionDeclaration(node) && isExported(node) && node.name) {
|
||||
for (const param of node.parameters) recordParam(param, node.name.text);
|
||||
for (const param of node.parameters) {
|
||||
recordParam(param, node.name.text);
|
||||
recordInlineOptionsMembers(param, node.name.text);
|
||||
}
|
||||
}
|
||||
/* An options-object property is the same fact wearing a different shape. */
|
||||
if (ts.isInterfaceDeclaration(node) && isExported(node)) {
|
||||
@@ -118,7 +154,25 @@ export function findUnwiredLaneParameters(files, readFile = (f) => readFileSync(
|
||||
return declarations.filter((declaration) => {
|
||||
for (const [file, source] of sources) {
|
||||
if (file === declaration.file) continue;
|
||||
/* A mention anywhere else counts as wired. Deliberately loose — see the header. */
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-01:35:
|
||||
The mention must come from a file that also names the DECLARING symbol.
|
||||
|
||||
The original rule — "the parameter name appears in any other file" — is unusable for any name
|
||||
that is also a common local variable, and I proved it on myself within one edit: renaming an
|
||||
unwired parameter from `completeColumnsByTaskId` to `completeColumns` made the guard go quiet
|
||||
immediately, because 15 unrelated production files happen to declare a local called
|
||||
`completeColumns`. The check had not been satisfied; it had been switched off by a rename.
|
||||
|
||||
That is precisely the "evadable by a style choice" failure this module condemns, so the fix is
|
||||
the cause rather than a name blocklist: a file that never references `diffSnapshots` cannot be
|
||||
the thing that wires `diffSnapshots`'s options.
|
||||
|
||||
Still deliberately loose — this is a co-occurrence test, not a call-graph analysis. It keeps
|
||||
the false-positive rate that makes the guard bearable while removing a false NEGATIVE that
|
||||
scaled with how ordinary the parameter's name was.
|
||||
*/
|
||||
if (declaration.owner && !source.includes(declaration.owner)) continue;
|
||||
if (source.includes(declaration.parameter)) return false;
|
||||
}
|
||||
return true;
|
||||
|
||||
Reference in New Issue
Block a user