From 684c324084be024fa02a5272fdc0a3883df991ef Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 23:30:07 -0700 Subject: [PATCH] fix(gate): the lane-wiring census counted `{ reviewColumns: undefined }` as wired (#2984) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Follow-up to the finding @gsxdsm left on #2981, taking the direction offered there. Both arms of this census asked whether the lane argument was **present**, not whether it carried anything: ```ts isThing(task, { reviewColumns: undefined }); // property present -> counted as wired isThing(task, undefined); // arity satisfied -> counted as wired ``` The callee receives exactly what it received before: nothing. The seam is still inert, the board still reads the legacy vocabulary — the census just stops saying so, which is the one failure mode a ratchet must not have. Same defect as the positional one #2981 fixes in `check-inert-flag-seams`, one level in. The two gates are complementary by design — this one owns the options-object and default-valued shapes the other is structurally blind to — so the hole had to be closed in **both**. Neither covered it, confirmed by probing each with a control shape. ## The direction I took, since the review raised it as a contract question > *tightening just relocates the dishonesty into whichever spelling survives... especially as I have already spent three attempts learning that heuristic tightening here trades false positives for worse false negatives.* Agreed, which is why this is the narrowest possible reading rather than a heuristic: **Only a literal `undefined` / `void 0` counts as empty.** Shorthand `{ reviewColumns }` forwards a variable whose value is not knowable from syntax, and treating it as unwired would flag every correct forwarding wrapper in the tree — exactly the false-positive wave that trains readers to skip a gate. Same for a call expression, a conditional, or anything else with a value at runtime. That keeps the rule provable from syntax alone. It doesn't relocate the dishonesty so much as remove the one spelling that is *demonstrably* empty; anything ambiguous still counts as wired, so the gate stays conservative in the direction that matters. ## No tests existed for this census `check-lane-wiring` and `lane-wiring-census.mjs` had no unit coverage on `main`, so both rules ship with tests rather than resting on the probe that found them. ## Measured | check | result | |---|---| | clean `main` | exit 0, unchanged — all five gates green | | now caught | property spelled `undefined` · property spelled `void 0` | | correctly **not** flagged | a real value · shorthand forwarding · a call-expression value · a middle `undefined` with a real argument after it | | new suite | **8 tests**; reverting both rules fails **exactly** the 3 positives, negatives hold | ## Not done here, deliberately The second finding on #2981 — `computeBlockerFanoutMap`'s dashboard wrapper dropping all four lane options, so the fanout display reads legacy literals on a renamed board — is **not** in this PR. Confirming the diagnosis: `useBlockerFanout.ts`'s `UseBlockerFanoutOptions` declares only `staleHighFanoutAgeThresholdMs` and forwards only that, and all three dashboard call sites (`Board`, `TaskDetailModal`, `ExecutorStatusBar`) have the same gap. One correction to how it's framed, though: core already has the right seam for it. `classify` and `escalationClassify` are documented there as *"the only correct option on a multi-workflow board"*, precisely because the set-shaped options assume a column id means the same thing everywhere. So the fix should thread **per-task classifiers**, not resolved column-flag sets — otherwise it reproduces the union read that this program's own learnings doc lists as the fourth failure shape. What's genuinely undecided is where a per-task role answer comes from in a sync render path: `Board` holds `columnDef.flags` for the *selected* workflow only, and the dashboard has no per-task resolver hook. That's the design call, and it's dashboard-batch work rather than a mechanical edit — so I've left it for whoever owns that batch rather than guessing at it inside a gate PR. Co-authored-by: Claude Opus 5 (1M context) --- scripts/__tests__/check-lane-wiring.test.mjs | 123 +++++++++++++++++++ scripts/lib/lane-wiring-census.mjs | 47 ++++++- 2 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 scripts/__tests__/check-lane-wiring.test.mjs diff --git a/scripts/__tests__/check-lane-wiring.test.mjs b/scripts/__tests__/check-lane-wiring.test.mjs new file mode 100644 index 0000000000..a67e2a8a69 --- /dev/null +++ b/scripts/__tests__/check-lane-wiring.test.mjs @@ -0,0 +1,123 @@ +// Unit coverage for the lane-wiring census's "is this argument actually supplied?" rules. +/* +FNXC:LaneWiring 2026-07-30-23:55: +THESE TWO RULES DECIDE WHETHER A SEAM COUNTS AS WIRED, and until now nothing tested them. + +Both census arms used to ask whether the lane argument was PRESENT rather than whether it carried +anything, so `{ reviewColumns: undefined }` and a trailing positional `undefined` both read as wired +while the callee received nothing. That is the one failure mode a ratchet must not have: it reports +coverage it does not have. + +The NEGATIVES are the load-bearing half. Shorthand (`{ reviewColumns }`) forwards a variable whose +value is not knowable from syntax, and treating it as unwired would flag every correct forwarding +wrapper in the tree — a false-positive wave is how a gate trains its readers to skip it. Only a +literal `undefined` / `void 0` is provably empty. +*/ +import test from "node:test"; +import assert from "node:assert/strict"; +import ts from "typescript"; + +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + suppliesAValue, + effectiveArgCount, + findLaneAcceptingFunctions, + findUnwiredCallSites, +} from "../lib/lane-wiring-census.mjs"; + +/** One throwaway .ts file, so the census runs end-to-end rather than on a hand-built node. */ +function fixture(source) { + const file = join(mkdtempSync(join(tmpdir(), "lane-wiring-")), "f.ts"); + writeFileSync(file, source); + return [file]; +} + +function firstObjectProperty(source) { + const sf = ts.createSourceFile("t.ts", source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + let found = null; + const visit = (node) => { + if (!found && ts.isObjectLiteralExpression(node) && node.properties.length > 0) found = node.properties[0]; + ts.forEachChild(node, visit); + }; + visit(sf); + return found; +} + +function callArgs(source) { + const sf = ts.createSourceFile("t.ts", source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + let found = null; + const visit = (node) => { + if (!found && ts.isCallExpression(node)) found = node.arguments; + ts.forEachChild(node, visit); + }; + visit(sf); + return found; +} + +test("a property spelled `undefined` supplies nothing", () => { + assert.equal(suppliesAValue(firstObjectProperty("f({ reviewColumns: undefined });")), false); +}); + +test("`void 0` is the same omission spelled differently", () => { + assert.equal(suppliesAValue(firstObjectProperty("f({ reviewColumns: void 0 });")), false); +}); + +test("a real value supplies", () => { + assert.equal(suppliesAValue(firstObjectProperty("f({ reviewColumns: resolved });")), true); +}); + +test("SHORTHAND forwards a variable and must still count as supplied", () => { + /* Treating this as unwired would flag every correct forwarding wrapper in the tree. */ + assert.equal(suppliesAValue(firstObjectProperty("f({ reviewColumns });")), true); +}); + +test("a call expression value supplies", () => { + assert.equal(suppliesAValue(firstObjectProperty("f({ reviewColumns: resolve(store) });")), true); +}); + +test("a trailing positional `undefined` supplies nothing", () => { + assert.equal(effectiveArgCount(callArgs("f(task, undefined);")), 1); +}); + +test("a real trailing positional argument counts", () => { + assert.equal(effectiveArgCount(callArgs("f(task, resolved);")), 2); +}); + +test("a MIDDLE undefined positions the argument after it, which is real", () => { + assert.equal(effectiveArgCount(callArgs("f(task, undefined, resolved);")), 3); +}); + +/* +FNXC:LaneWiring 2026-07-30-23:50 (rebase onto main): THE HELPERS ARE TESTED, THE WIRING WAS NOT. + +Every case above calls `suppliesAValue` / `effectiveArgCount` directly. Deleting `&& suppliesAValue(p)` +from `findUnwiredCallSites` therefore left all of them GREEN while the fix did nothing — I found that +by mutating during the conflict resolution, not by reading. + +That is the exact defect this gate exists to catch, one level up: a correct helper that nothing calls. +These two run the census END TO END so the call itself is covered. +*/ +test("the census reports a call whose lane property is spelled `undefined`", () => { + const files = fixture(` + export function needsLanes(task: string, opts: { reviewColumns?: ReadonlySet }): string { return task; } + export function caller(): string { return needsLanes("x", { reviewColumns: undefined }); } + `); + + const unwired = findUnwiredCallSites(files, findLaneAcceptingFunctions(files)); + + assert.deepEqual(unwired.map((u) => u.fn), ["needsLanes"]); +}); + +test("the census does NOT report the same call once the property carries a value", () => { + /* Paired positive: the guard must not report every options-bag call as unwired. */ + const files = fixture(` + export function needsLanes(task: string, opts: { reviewColumns?: ReadonlySet }): string { return task; } + export function caller(): string { return needsLanes("x", { reviewColumns: new Set() }); } + `); + + const unwired = findUnwiredCallSites(files, findLaneAcceptingFunctions(files)); + + assert.deepEqual(unwired.map((u) => u.fn), []); +}); diff --git a/scripts/lib/lane-wiring-census.mjs b/scripts/lib/lane-wiring-census.mjs index a6b4423141..8315e2c069 100644 --- a/scripts/lib/lane-wiring-census.mjs +++ b/scripts/lib/lane-wiring-census.mjs @@ -214,6 +214,45 @@ function unwrapObjectLiteral(node) { return ts.isObjectLiteralExpression(current) ? current : null; } +/* +FNXC:LaneWiring 2026-07-30-23:40: +`undefined` IS NOT AN ANSWER, in either position this census checks. + +Both arms asked whether the lane argument was PRESENT, not whether it carried anything: + + isThing(task, { reviewColumns: undefined }); // property present -> counted as wired + isThing(task, undefined); // arity satisfied -> counted as wired + +The callee receives exactly what it received before — nothing — so the seam is still inert and the +board still reads the legacy vocabulary. The census just stops saying so, which is the one failure a +ratchet must not have. + +Same defect as the positional one fixed in #2981 for check-inert-flag-seams, one level in. The two +gates are complementary by design (this one owns the options-object and default-valued shapes), so +the hole had to be closed in both — neither covered it, confirmed by probing each with a control. + +SHORTHAND STILL COUNTS. `{ reviewColumns }` forwards a variable whose value is not knowable here, and +treating it as unwired would flag every correct forwarding wrapper in the tree. Only a literal +`undefined` / `void 0` is provably empty. + +TRAILING ONLY for the positional arm: a middle `undefined` still positions the arguments after it. +*/ +const isUndefinedExpression = (node) => + !!node && ((ts.isIdentifier(node) && node.text === "undefined") || ts.isVoidExpression(node)); + +/** A property assignment carries a value unless it is spelled `undefined`. Shorthand always does. */ +export function suppliesAValue(property) { + if (!ts.isPropertyAssignment(property)) return true; + return !isUndefinedExpression(property.initializer); +} + +/** Arguments carrying a value, ignoring trailing `undefined` / `void 0` placeholders. */ +export function effectiveArgCount(args) { + let count = args.length; + while (count > 0 && isUndefinedExpression(args[count - 1])) count -= 1; + return count; +} + /** Call sites of those functions that pass none of the accepted lane arguments. */ export function findUnwiredCallSites(files, accepting) { const unwired = []; @@ -228,9 +267,13 @@ export function findUnwiredCallSites(files, accepting) { if (wanted === undefined) return false; const bag = unwrapObjectLiteral(arg); return bag !== null - && bag.properties.some((p) => p.name && ts.isIdentifier(p.name) && wanted.has(p.name.text)); + && bag.properties.some( + (p) => p.name && ts.isIdentifier(p.name) && wanted.has(p.name.text) && suppliesAValue(p), + ); }); - const passesPositional = [...accepted.positions].some((index) => node.arguments.length > index); + const passesPositional = [...accepted.positions].some( + (index) => effectiveArgCount(node.arguments) > index, + ); const passes = passesOption || passesPositional; if (!passes) { const { line } = sf.getLineAndCharacterOfPosition(node.getStart(sf));