fix(gate): the lane-wiring census counted { reviewColumns: undefined } as wired (#2984)
## 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) <noreply@anthropic.com>
This commit is contained in:
123
scripts/__tests__/check-lane-wiring.test.mjs
Normal file
123
scripts/__tests__/check-lane-wiring.test.mjs
Normal file
@@ -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> }): 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> }): 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), []);
|
||||
});
|
||||
@@ -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));
|
||||
|
||||
Reference in New Issue
Block a user