Skip to content

Commit 4e8a2ec

Browse files
committed
fix(gate-manifest): prove the forwarded rule is only ever read, not enumerate binders
Addresses both P2s from the re-review at e5331aa, and replaces the approach that produced them. The binder list was the wrong shape of proof. It failed open one construct at a time — a `const` in a nested block, then a catch parameter, then a destructured binding, and now `class entry {}` — because there is always another declaration form to miss. Proving provenance only at callback ingress had the same defect from the other side: `entry = config` keeps the binding and replaces the value. onlyReadsAsProperty replaces both. Every occurrence of the parameter name inside the callback must be the object of a member access that is not an assignment target; the parameter itself is the only other permitted occurrence. That is total rather than enumerated: a re-declaration of ANY kind, a reassignment, a mutation of the entry, and passing the binding elsewhere all appear as an occurrence that is not a read, and each refuses the exemption. It over-refuses in principle and fails closed, which is the direction a conservative proof should err in. shadowsParameter, declaresName and patternNames are deleted — the enumeration they encoded is what kept springing leaks. Tests 50 → 54: class-declaration shadow, parameter reassignment, entry mutation (`entry.rule = computed`), and aliasing (`mutate(entry)`). The live `BUILD_OWNERSHIP.filter((entry) => entry.owns(file)).map((entry) => …)` still resolves — `entry.owns` is a read like the rest. Planted red against the live selector: wrapping its map body in `entry = { ...entry }` now fails with its line number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SkS4S8XXrfkJ8TD1VBKkvJ
1 parent e5331aa commit 4e8a2ec

2 files changed

Lines changed: 87 additions & 42 deletions

File tree

scripts/gate-manifest/selector-rules.test.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,56 @@ test('a value-preserving chain still proves provenance', () => {
177177
assert.deepEqual(selectorRuleIds('m.ts', source), ['gate:lint', 'own:swift']);
178178
});
179179

180+
test('a class declaration shadowing the parameter fails closed', () => {
181+
// Not in any binder enumeration — which is why the proof asks "is every use a property
182+
// read?" instead of listing the ways a name can be bound.
183+
const source = `
184+
const BUILD_OWNERSHIP = [{ check: 'swift-runner', rule: 'own:swift', detail: 'd', owns: () => true }];
185+
const bad = BUILD_OWNERSHIP.map((entry) => {
186+
{
187+
class entry {}
188+
return reason('lint', file, entry.rule, 'd');
189+
}
190+
});
191+
`;
192+
assert.throws(() => selectorRuleIds('m.ts', source), /rule argument is not a string literal/);
193+
});
194+
195+
test('reassigning the parameter before use fails closed', () => {
196+
// Same binding, different value. Proving provenance only at callback ingress would exempt a
197+
// rule that is no longer an ownership entry by the time it is read.
198+
const source = `
199+
const BUILD_OWNERSHIP = [{ check: 'swift-runner', rule: 'own:swift', detail: 'd', owns: () => true }];
200+
const bad = BUILD_OWNERSHIP.map((entry) => {
201+
entry = config;
202+
return reason('lint', file, entry.rule, 'd');
203+
});
204+
`;
205+
assert.throws(() => selectorRuleIds('m.ts', source), /rule argument is not a string literal/);
206+
});
207+
208+
test('mutating the entry before reading it fails closed', () => {
209+
const source = `
210+
const BUILD_OWNERSHIP = [{ check: 'swift-runner', rule: 'own:swift', detail: 'd', owns: () => true }];
211+
const bad = BUILD_OWNERSHIP.map((entry) => {
212+
entry.rule = computed;
213+
return reason('lint', file, entry.rule, 'd');
214+
});
215+
`;
216+
assert.throws(() => selectorRuleIds('m.ts', source), /rule argument is not a string literal/);
217+
});
218+
219+
test('passing the binding elsewhere before use fails closed', () => {
220+
const source = `
221+
const BUILD_OWNERSHIP = [{ check: 'swift-runner', rule: 'own:swift', detail: 'd', owns: () => true }];
222+
const bad = BUILD_OWNERSHIP.map((entry) => {
223+
mutate(entry);
224+
return reason('lint', file, entry.rule, 'd');
225+
});
226+
`;
227+
assert.throws(() => selectorRuleIds('m.ts', source), /rule argument is not a string literal/);
228+
});
229+
180230
test('a computed rule argument fails closed rather than being skipped', () => {
181231
// A category this reader cannot see would bypass the representative-sample and reachability
182232
// checks entirely — the exact hole the derived universe exists to close.

scripts/gate-manifest/selector-rules.ts

Lines changed: 37 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -163,52 +163,47 @@ function callbackFunctions(node: Node): Node[] {
163163
return args.filter((argument): argument is Node => isNode(argument) && isFunctionLike(argument));
164164
}
165165

166-
/** Every identifier a binding pattern introduces, including destructured ones. */
167-
function patternNames(node: unknown, into: Set<string>): void {
168-
if (!isNode(node)) return;
169-
const name = identifierName(node);
170-
if (name !== null) {
171-
into.add(name);
172-
return;
173-
}
174-
for (const child of children(node)) patternNames(child, into);
175-
}
176-
177-
/** Whether a node introduces `name` as a new binding of any kind. */
178-
function declaresName(node: Node, name: string): boolean {
179-
const names = new Set<string>();
180-
if (node.type === 'VariableDeclarator') patternNames(node['id'], names);
181-
else if (node.type === 'CatchClause') patternNames(node['param'], names);
182-
else if (isFunctionLike(node)) {
183-
patternNames(node['id'], names);
184-
const params = node['params'];
185-
if (Array.isArray(params)) for (const parameter of params) patternNames(parameter, names);
186-
}
187-
return names.has(name);
188-
}
189-
190166
/**
191-
* Any re-declaration of `name` inside `callback` other than the callback's own parameter.
167+
* Every use of `name` inside `callback`, other than the parameter itself, must be a plain
168+
* property READ — the object of a member access that is not being assigned to.
192169
*
193-
* Resolving to the innermost FUNCTION binder is not enough on its own: a block, catch or loop
194-
* binding shadows the ownership parameter without being a function parameter at all, and the
195-
* resolver would otherwise fall back to the ownership binding and exempt a dynamic rule —
196-
*
197-
* BUILD_OWNERSHIP.map((entry) => { const entry = config; reason(…, entry.rule, …); })
198-
*
199-
* Rather than model every scoping construct, this refuses the exemption whenever the name is
200-
* re-declared anywhere inside the callback. That over-refuses in principle (a re-declaration in
201-
* a sibling block cannot reach the call site) and fails closed, which is the right direction.
170+
* This replaces an enumeration of binder kinds, which kept failing open one construct at a
171+
* time: a `const` in a nested block, then a catch parameter, then a destructured binding, then
172+
* `class entry {}`. Enumerating is the wrong shape of proof — there is always another
173+
* declaration form. Asking instead "is every occurrence of this name a property read?" is
174+
* total: any re-declaration (of any kind), any reassignment, any mutation of the entry, and any
175+
* passing of the binding elsewhere all show up as an occurrence that is not a read, and refuse
176+
* the exemption. It over-refuses in principle and fails closed, which is the point.
202177
*/
203-
function shadowsParameter(program: Node, callback: Node, name: string): boolean {
178+
function onlyReadsAsProperty(
179+
program: Node,
180+
callback: Node,
181+
parameter: Node,
182+
name: string,
183+
): boolean {
204184
const span = spanOf(callback);
205-
if (span === null) return true;
206-
let shadowed = false;
185+
if (span === null) return false;
186+
187+
const assigned = new Set<unknown>();
188+
const reads = new Set<Node>();
189+
collect(program, (node) => {
190+
if (!within(span, node)) return;
191+
if (node.type === 'AssignmentExpression') assigned.add(node['left']);
192+
else if (node.type === 'UpdateExpression') assigned.add(node['argument']);
193+
});
194+
collect(program, (node) => {
195+
if (!within(span, node) || !isMemberAccess(node) || assigned.has(node)) return;
196+
const object = node['object'];
197+
if (isNode(object) && identifierName(object) === name) reads.add(object);
198+
});
199+
200+
let onlyReads = true;
207201
collect(program, (node) => {
208-
if (shadowed || node === callback) return;
209-
if (within(span, node) && declaresName(node, name)) shadowed = true;
202+
if (!onlyReads || !within(span, node)) return;
203+
if (identifierName(node) !== name || node === parameter || reads.has(node)) return;
204+
onlyReads = false;
210205
});
211-
return shadowed;
206+
return onlyReads;
212207
}
213208

214209
/**
@@ -237,8 +232,8 @@ function parameterBindings(program: Node, tables: ReadonlySet<string>): Paramete
237232
const owns = ownershipCallbacks.has(node);
238233
for (const parameter of params) {
239234
const name = identifierName(parameter);
240-
if (name === null) continue;
241-
const iteratesOwnership = owns && !shadowsParameter(program, node, name);
235+
if (name === null || !isNode(parameter)) continue;
236+
const iteratesOwnership = owns && onlyReadsAsProperty(program, node, parameter, name);
242237
bindings.push({ name, span, iteratesOwnership });
243238
}
244239
});

0 commit comments

Comments
 (0)