diff --git a/DECISIONS.md b/DECISIONS.md index 4da1d46fb..086bb6242 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -537,3 +537,87 @@ reference AND the hedge, and why that is still the right trade. `callable-value-flow` failed once on its TIMING budget (widening overhead 2.006 > 1.9) with a byte-identical fingerprint, then passed twice at 1.788 / 1.813 — machine load, not a regression. + +--- + +## Review round 4 — local preflight, before pushing + +`gitnexus-check[bot]` is a hosted GitHub App and cannot be run locally, so the +`.local-preflight/` harness (out of git) reproduces the two halves its output is +built from: the deterministic gates and blast radius off this checkout's own +graph, and an LLM reviewer pointed at the failure classes this bot has actually +reported here. Run on `3bd1337a` it returned five findings; one was a real hole +in R2-2, which is the point of running it before pushing rather than after. + +**R4-1 (valid, REPRODUCED, fixed) — the container channel preempted the file's +own `@import`.** R2-2 added the module channel as a FALLBACK after +`findClassBindingInScope`. That is the wrong order, because +`findClassBindingInScope` does not stop at the scope chain: when its `isClassLike` +walk misses — and a namespace handle binds a Module, so it always misses — it +falls back to `scopes.qualifiedNames`, a workspace-wide index, and answers with +the unique def of that name anywhere in the repo. So: + + // decoy.zig — never imported by Element.zig + pub const dom_utils = struct { pub fn compare(a: u8, b: u8) u8 {…} }; + + // Element.zig + const dom_utils = @import("dom_utils.zig"); + pub const comparator = bridge.accessor(dom_utils.compare, null, .{}); + +bound `Method:src/webapi/decoy.zig:dom_utils.compare#2` — a wrong edge, and the +module channel that would have answered correctly was never reached. R3-1's +shadow guard cannot catch it either: the import binds at MODULE scope, which the +guard treats as the floor. + +Fixed by trying the module channel FIRST. An import written in this file is the +strongest available statement about what the name means here, and it outranks a +global uniqueness guess; when the handle is not an import of this file the +channel answers nothing and the container path runs exactly as before. Pinned by +`decoy.zig` plus a strengthened assertion on the existing module-owner test, +which fails on the old order. + +**R4-2 (valid, fixed) — the cause documentation named shapes nothing captures.** +`tools.ts` illustrated `callableValueReferences` with "a callback argument", "a +stored function pointer" and `qsort(xs, n, sz, compareItems)`. Only Zig captures +a call argument or a const initialiser; JS/TS capture only object-literal +property values, and C has no value-ref rule at all, so the `qsort` example is +counted in no language. Both cause blocks now name the shapes that are actually +captured and say plainly that a bare JS/TS callback argument is not among them, +so a 0 does not rule it out. + +**R4-3 (valid, fixed) — the same block exempted itself from the re-index caveat +this PR proves it needs.** "Read from the graph, so it needs no index-time +metadata" is true of the probe and false of the edges: an index built before a +language emitted these captures has none and reports 0 — which is exactly the +warm-cache failure R2-1 bumped `SCHEMA_BUMP` for. The doc now says to re-analyze +before reading a 0 as measured. + +**R4-4 (valid, fixed) — the dispatchability canary was narrower than its own +promise.** Its header said it fails on "a fourth language emitting `value-ref` at +all"; it reads `languages//query.ts`, so Vue — which owns no query and +borrows `emitTsScopeCaptures` / `emitJsScopeCaptures` — emits value-refs while +the assertion lists three languages, and a capture synthesized in code (the +mechanism `@reference.static-gated` uses) is invisible to it entirely. The case +now asserts on query OWNERS, which is what it actually checks and is sound +because a delegating language inherits the classification of the rules it +borrows, and the header states the synthesized-capture gap rather than leaving a +green tick to imply it away. + +**R4-5 (valid, fixed as documentation) — the BARE docstring described a lexical +walk that is not one.** "resolved up the lexical chain, which is what an +unqualified name means" — `findCallableBindingInScope` applies the callable +predicate WHILE walking, so a nearer parameter or local is stepped over. Same +defect R3-1 fixed on the container channel, unguarded here, pre-existing since +#2437 and reachable in JS/TS. Out of scope for #3399, so the behaviour is +unchanged and the sentence now says what the walk does instead of implying a +guarantee it does not give. + +### Gates after review round 4 + +- `tsc --noEmit` clean, `npm run build` clean, `prettier --check` clean. +- `test/integration/resolvers` — 3,632 passed / 3 skipped (70 files). +- `test/unit/scope-resolution` — 2,015 passed (120 files). +- `impact-callable-value-references` under `lbug-db` — 7 passed. +- Bench `--check`: `receiver-resolution`, `zig-cross-file-resolution`, + `scope-capture` (15 languages), `scope-emission`, `callable-value-flow` all + PASS, no baseline edited. diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/property-dispatch.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/property-dispatch.ts index bee90b895..bd1b939c1 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/property-dispatch.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/property-dispatch.ts @@ -82,7 +82,16 @@ export const PROPERTY_DISPATCH_CONFIDENCE = 0.7; * Two shapes, and the difference matters: * * - BARE (`{ handler: onClick }`, `register(onTick)`) — the name is resolved - * up the lexical chain, which is what an unqualified name means. + * up the lexical chain, which is what an unqualified name means. Be exact + * about what that walk does, because it is not a plain lexical lookup: + * `findCallableBindingInScope` applies the callable predicate WHILE walking, + * so a scope binding the name to a parameter or a local contributes nothing + * and the walk continues outward. A nearer non-callable binding is stepped + * over — the same shape the qualified path guards against below, unguarded + * here. Pre-existing (#2437) and out of scope for #3399; reachable in JS/TS + * (`function outer(handler) { return { h: handler }; }` beside a top-level + * `function handler`), and narrow in Zig, which rejects a local shadowing a + * container declaration. * * - QUALIFIED (`bridge.accessor(Element.getNamespaceUri, …)`) — the source * WROTE the owner, so the lexical chain is the wrong instrument. It gives @@ -94,12 +103,13 @@ export const PROPERTY_DISPATCH_CONFIDENCE = 0.7; * allows a same-named callable in a nested container, Zig included. * * So a qualified site resolves through its receiver: name the owner, then take - * the member off that owner. An owner is either a CLASS-like container or a - * MODULE — `Element.getNamespaceUri` and `utils.compare` are the same shape + * the member off that owner. An owner is either a MODULE or a CLASS-like + * container — `utils.compare` and `Element.getNamespaceUri` are the same shape * written against the two kinds of namespace a language has, and the member-call - * path already resolves both (receiver-bound-calls Case 2 / Case 1). Both are - * tried here for the same reason: see `findNamespaceValueRefTarget` for why a - * module receiver cannot simply be declined. + * path already resolves both (receiver-bound-calls Case 1 / Case 2). Both are + * tried here, MODULE FIRST: see `findNamespaceValueRefTarget` for why a module + * receiver cannot simply be declined, and the comment on the call below for why + * the container channel must not go first. * * If neither channel names the owner, or the owner is named but owns no such * callable, this DECLINES rather than falling back to the lexical walk. @@ -130,6 +140,25 @@ function resolveValueRefTarget( if (receiverName === undefined) { return findCallableBindingInScope(site.inScope, site.name, scopes); } + // NAMESPACE FIRST, and the order is load-bearing. `findClassBindingInScope` + // does not stop at the scope chain: when its `isClassLike` walk misses — and a + // namespace handle binds a Module, so it always misses — it falls back to + // `scopes.qualifiedNames`, a WORKSPACE-wide index, and answers with the unique + // def of that name anywhere in the repo. Trying it first therefore lets a + // same-named container in a file this one never imported preempt the `@import` + // this file actually wrote: + // + // const dom_utils = @import("dom_utils.zig"); // namespace-only module + // … bridge.accessor(dom_utils.compare, …) // → decoy.zig's compare + // + // The shadow guard below cannot catch it: the import binds at MODULE scope, + // which the guard treats as the floor. An import written in this file is the + // strongest statement about what the name means here, so it outranks a global + // guess — and when the handle is not an import of this file, this answers + // nothing and the container channel runs exactly as before. + const viaNamespace = findNamespaceValueRefTarget(site, filePath, receiverName, scopes); + if (viaNamespace !== undefined) return viaNamespace; + const owner = findClassBindingInScope(site.inScope, receiverName, scopes); if (owner !== undefined) { // The container lookup is a CLASS-ONLY walk: `walkScopeChain` filters by @@ -155,9 +184,7 @@ function resolveValueRefTarget( if (member === undefined || !CALL_TARGET_TYPES.has(member.type)) return undefined; return member; } - // A receiver that is not class-like may still be a MODULE — the other kind of - // owner a qualified name can have. See `findNamespaceValueRefTarget`. - return findNamespaceValueRefTarget(site, filePath, receiverName, scopes); + return undefined; } /** diff --git a/gitnexus/src/mcp/tools.ts b/gitnexus/src/mcp/tools.ts index 41e9ff485..5e7791a10 100644 --- a/gitnexus/src/mcp/tools.ts +++ b/gitnexus/src/mcp/tools.ts @@ -299,7 +299,7 @@ COMPLETENESS OF incoming: alongside symbol/incoming/outgoing the result carries - causes.externalBoundary (unit: call sites) > 0 — the calls left the indexed program (System.out.println, fetch(...)). NOT a defect: no in-graph node could have been reached. An epistemic:'exact' result can carry this. - causes.dispatchBoundary (unit: symbols) > 0 — DI or interface dispatch: that many symbols sit on or beyond a boundary static analysis cannot cross. Irreducible. A symbol count, not a site count — per-site multiplicity is not retained for these edges — so compare its magnitude with receiverTyping, not its exact value. A framework runtime-proxy boundary can make epistemic lower-bound while this value remains 0 because endpoint metadata proves the gap but cannot count omitted symbols. - causes.undecidedSatisfaction (unit: unjudged interface/type pairs) > 0 — the analyzer could not decide whether a type satisfies an interface, so no IMPLEMENTS edge exists and no dispatch boundary was left for the walk to notice. Usually fixable by making the missing dependency available to analysis. - - causes.callableValueReferences (unit: symbols) > 0 — that many symbols name this callable as a VALUE instead of calling it (a registration table, a callback argument, a stored function pointer). The reference is in the graph as a USES edge; the call made THROUGH the value is not, because it is dispatched later from wherever the value was stored. incoming.calls is therefore a floor. Follow the USES edges to find the registration, then the code that reads it. It is 0 when the analyzer DID synthesize the dispatch through a registered property key. That exclusion is per SYMBOL, not per registration: a target with BOTH a followed registration and an unfollowed escape reads 0 here, so a 0 means 'no unfollowed registration was proven', not 'this symbol escapes nowhere'. A 0 alongside epistemic 'lower-bound' can also mean the probe itself could not run — read boundaries for which. + - causes.callableValueReferences (unit: symbols) > 0 — that many symbols name this callable as a VALUE instead of calling it (a Zig registration table or const initialiser, a JS/TS object-literal property value). A bare callback argument in JS/TS is not captured today and is not counted, so a 0 does not rule that shape out; nor does it, on an index built before the language emitted these captures — re-analyze first. The reference is in the graph as a USES edge; the call made THROUGH the value is not, because it is dispatched later from wherever the value was stored. incoming.calls is therefore a floor. Follow the USES edges to find the registration, then the code that reads it. It is 0 when the analyzer DID synthesize the dispatch through a registered property key. That exclusion is per SYMBOL, not per registration: a target with BOTH a followed registration and an unfollowed escape reads 0 here, so a 0 means 'no unfollowed registration was proven', not 'this symbol escapes nowhere'. A 0 alongside epistemic 'lower-bound' can also mean the probe itself could not run — read boundaries for which. REQUIRES RE-INDEX: causes.scopeExtractionFiles, causes.receiverTyping, causes.externalBoundary, causes.undecidedSatisfaction, and framework runtime-proxy boundary detection depend on index-time metadata that only a current analyzer writes. Against an older index the metadata can be absent, which is indistinguishable from "nothing was dropped" unless the schema probe detects the stale index — re-run \`gitnexus analyze\` before trusting a zero or an apparently exact result. @@ -498,7 +498,7 @@ Output includes: - causes.dispatchBoundary (unit: symbols) > 0 — DI or interface dispatch: that many symbols sit on or beyond a boundary a static walk cannot cross. Irreducible. A symbol count, not a site count — per-site multiplicity is not retained for these edges — so compare its magnitude with receiverTyping, not its exact value. A framework runtime-proxy boundary can make epistemic lower-bound while this value remains 0 because endpoint metadata proves the gap but cannot count omitted symbols. - causes.undecidedSatisfaction (unit: unjudged interface/type pairs) > 0 — the analyzer could not DECIDE whether a type satisfies an interface (a type in a required signature named a package it could not resolve), so no IMPLEMENTS edge exists and no dispatch boundary was left for the walk to notice. Distinct from every cause above, which count decided facts that could not be attributed; this one counts questions never answered. It is the only cause that shortens a result WITHOUT leaving a trace in the graph, so an unhedged zero on a symbol reached only through such an interface would otherwise read as 'nobody calls this'. Usually fixable: it most often means a dependency is missing from the analyzed tree. - - causes.callableValueReferences (unit: symbols) > 0 — that many symbols name this callable as a VALUE rather than calling it: 'bridge.accessor(Element.getNamespaceUri, ...)', '{ onClick: handler }', a comparator handed to a sort. The registration IS modelled (a USES edge); the invocation through the stored value is NOT, because it happens later via a struct field, a registry lookup or comptime reflection. So impactedCount is a floor and a LOW risk verdict on such a symbol is a floor too. Unlike dispatchBoundary this is often reducible — it usually means the language provider does not yet follow that store/load — but until it is, do NOT read an empty or small caller set as 'safe to change'. It is an exact count, not a capped sample. It is 0 when the analyzer DID synthesize the dispatch through a registered property key, and epistemic stays 'exact' on that account. That exclusion is symbol-level, not edge-level — the graph does not record which registration produced which synthesized call — so a symbol with a mix of followed and unfollowed registrations also reads 0: treat a 0 as 'no unfollowed registration was proven', not as proof the value escapes nowhere. A 0 alongside epistemic 'lower-bound' can instead mean the probe could not run at all, so read boundaries to tell those apart. Read from the graph, so it needs no index-time metadata. + - causes.callableValueReferences (unit: symbols) > 0 — that many symbols name this callable as a VALUE rather than calling it: 'bridge.accessor(Element.getNamespaceUri, ...)' and 'pub const h = onReset;' in Zig, '{ onClick: handler }' in JS/TS. Those are the shapes actually captured today — a bare callback argument in JS/TS ('qsort'-style, 'setTimeout(tick)') is NOT one of them and is not counted, so a 0 here does not rule that shape out. The registration IS modelled (a USES edge); the invocation through the stored value is NOT, because it happens later via a struct field, a registry lookup or comptime reflection. So impactedCount is a floor and a LOW risk verdict on such a symbol is a floor too. Unlike dispatchBoundary this is often reducible — it usually means the language provider does not yet follow that store/load — but until it is, do NOT read an empty or small caller set as 'safe to change'. It is an exact count, not a capped sample. It is 0 when the analyzer DID synthesize the dispatch through a registered property key, and epistemic stays 'exact' on that account. That exclusion is symbol-level, not edge-level — the graph does not record which registration produced which synthesized call — so a symbol with a mix of followed and unfollowed registrations also reads 0: treat a 0 as 'no unfollowed registration was proven', not as proof the value escapes nowhere. A 0 alongside epistemic 'lower-bound' can instead mean the probe could not run at all, so read boundaries to tell those apart. Read from the graph, so it needs no index-time metadata BEYOND the edges being there: an index built by an analyzer that did not yet emit this language's value-ref captures has none, and reports 0. Re-analyze before reading a 0 as measured. REQUIRES RE-INDEX: causes.scopeExtractionFiles, causes.receiverTyping, causes.externalBoundary, causes.undecidedSatisfaction, and framework runtime-proxy boundary detection depend on index-time metadata that only a current analyzer writes. Against an older index the metadata can be absent, which is indistinguishable from "nothing was dropped" unless the schema probe detects the stale index — re-run \`gitnexus analyze\` before trusting a zero or an apparently exact result. diff --git a/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/decoy.zig b/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/decoy.zig new file mode 100644 index 000000000..b6e2a1d47 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/decoy.zig @@ -0,0 +1,14 @@ +// A container whose NAME collides with `Element.zig`'s `dom_utils` import +// handle, in a file `Element.zig` never imports. +// +// `findClassBindingInScope` does not stop at the scope chain: when its +// `isClassLike` walk misses — and a namespace import binds a Module, not a +// class — it falls back to `scopes.qualifiedNames`, a WORKSPACE-wide index, and +// answers with the unique def of that name. This struct is that unique def. A +// registration written `dom_utils.compare` in `Element.zig` must still bind +// `dom_utils.zig`'s function, not this one: the file said which module it meant. +pub const dom_utils = struct { + pub fn compare(a: u8, b: u8) u8 { + return if (a < b) a else b; + } +}; diff --git a/gitnexus/test/integration/resolvers/zig.test.ts b/gitnexus/test/integration/resolvers/zig.test.ts index e9dbe1eb1..668a3ab0a 100644 --- a/gitnexus/test/integration/resolvers/zig.test.ts +++ b/gitnexus/test/integration/resolvers/zig.test.ts @@ -348,6 +348,13 @@ describe.skipIf(!zigAvailable)('Zig idioms (zig-idioms fixture)', () => { // The member-CALL path already resolves `dom_utils.compare()` through the // file's namespace import; the registration reads the same channel. expect(valueRefs).toContain('JsApi → compare'); + // And it must be dom_utils.zig's `compare`, not `decoy.zig`'s. That file + // declares a CONTAINER also called `dom_utils`, with its own `compare`, + // and `Element.zig` never imports it. `findClassBindingInScope` does not + // stop at the scope chain: a namespace handle binds a Module, so the + // `isClassLike` walk misses and its workspace-wide `qualifiedNames` + // fallback answers with that unique container — preempting the `@import` + // this very file wrote. The written import has to outrank a global guess. expect(valueRefTargetIds.filter((id) => id.includes('compare'))).toEqual([ 'Function:src/webapi/dom_utils.zig:compare', ]); diff --git a/gitnexus/test/unit/scope-resolution/value-ref-dispatchability.test.ts b/gitnexus/test/unit/scope-resolution/value-ref-dispatchability.test.ts index 4d1e71ce8..2c8df245f 100644 --- a/gitnexus/test/unit/scope-resolution/value-ref-dispatchability.test.ts +++ b/gitnexus/test/unit/scope-resolution/value-ref-dispatchability.test.ts @@ -31,6 +31,15 @@ * This test fails instead. If it fails, do not relax it: go and decide what * `callableValueReferenceBoundaries` should do about a mixed symbol (the * options are recorded at the exclusion site), then update this file. + * + * WHAT IT DOES NOT COVER, stated so the green tick is not read as more than it + * is. It reads query SOURCES, so a capture synthesized in code rather than + * matched by a rule — the mechanism `@reference.static-gated` uses — can break + * the partition with this test green. A provider adding one has to come here by + * hand. Languages that own no query and delegate to another's captures (Vue → + * `emitTsScopeCaptures` / `emitJsScopeCaptures`) are covered transitively, by + * the rules they borrow, which is why the last case asserts on query OWNERS + * rather than on the set of languages that can emit a value-ref. */ import { describe, it, expect } from 'vitest'; import fs from 'node:fs'; @@ -132,10 +141,14 @@ describe('value-ref dispatchability partition', () => { expect(rules.filter((r) => r.includes(PROPERTY_KEY))).toEqual([]); }); - it('no OTHER language emits a value-ref capture', () => { - // The three above are hand-classified. A fourth language emitting + it('no OTHER language OWNS a value-ref rule', () => { + // The three above are hand-classified. A fourth query declaring // `value-ref` has not been classified by anyone, so the exclusion's premise // is unverified for it — classify it here and in the exclusion's comment. + // "Owns", not "emits": Vue has no query of its own and borrows TypeScript's + // and JavaScript's captures, so it inherits their classification rather than + // needing one. A capture synthesized in code owns no rule either and is + // invisible here — see the header. const languagesDir = path.join( path.dirname(fileURLToPath(import.meta.url)), '../../../src/core/ingestion/languages',