diff --git a/.changeset/glasses-completion-lanes.md b/.changeset/glasses-completion-lanes.md new file mode 100644 index 0000000000..2ef1476ad2 --- /dev/null +++ b/.changeset/glasses-completion-lanes.md @@ -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. diff --git a/packages/engine/src/__tests__/unwired-lane-parameter-guard.test.ts b/packages/engine/src/__tests__/unwired-lane-parameter-guard.test.ts index 127bd3868d..643d957730 100644 --- a/packages/engine/src/__tests__/unwired-lane-parameter-guard.test.ts +++ b/packages/engine/src/__tests__/unwired-lane-parameter-guard.test.ts @@ -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; 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; completeColumns?: ReadonlySet }) { return opts; }", + "/repo/plugins/p/src/caller.ts": "diffSnapshots(prev, next, { notifyOnColumns });", + } as Record; + + 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 }) { return opts; }", + "/repo/plugins/p/src/caller.ts": "diffSnapshots(prev, { completeColumns });", + } as Record; + + 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 }) { return opts; }", + "/repo/plugins/p/src/caller.ts": "diffSnapshots(other);", + } as Record; + + 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"); diff --git a/plugins/fusion-plugin-even-realities-glasses/src/__tests__/diff.test.ts b/plugins/fusion-plugin-even-realities-glasses/src/__tests__/diff.test.ts index d0a3e138ae..8ec068b60f 100644 --- a/plugins/fusion-plugin-even-realities-glasses/src/__tests__/diff.test.ts +++ b/plugins/fusion-plugin-even-realities-glasses/src/__tests__/diff.test.ts @@ -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")], { diff --git a/plugins/fusion-plugin-even-realities-glasses/src/notifications/diff.ts b/plugins/fusion-plugin-even-realities-glasses/src/notifications/diff.ts index 9fe0e25850..a3a03a7294 100644 --- a/plugins/fusion-plugin-even-realities-glasses/src/notifications/diff.ts +++ b/plugins/fusion-plugin-even-realities-glasses/src/notifications/diff.ts @@ -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({ diff --git a/plugins/fusion-plugin-even-realities-glasses/src/notifier.ts b/plugins/fusion-plugin-even-realities-glasses/src/notifier.ts index 28fd1c1a52..2ed19d91f7 100644 --- a/plugins/fusion-plugin-even-realities-glasses/src/notifier.ts +++ b/plugins/fusion-plugin-even-realities-glasses/src/notifier.ts @@ -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> | undefined; + if (alsoNotifyOnDone) { + const irCache = new Map(); + completeColumnsByTaskId = new Map(); + for (const task of tasks) { + try { + const complete = (await resolveTaskLifecycleColumns( + deps.taskStore as Parameters[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) { diff --git a/scripts/lib/unwired-lane-parameter.mjs b/scripts/lib/unwired-lane-parameter.mjs index dbb54bd8e9..d121afc287 100644 --- a/scripts/lib/unwired-lane-parameter.mjs +++ b/scripts/lib/unwired-lane-parameter.mjs @@ -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; 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;