diff --git a/eval/workflow_bench/learnings.jsonl b/eval/workflow_bench/learnings.jsonl index 7d25e3359..5fab5746a 100644 --- a/eval/workflow_bench/learnings.jsonl +++ b/eval/workflow_bench/learnings.jsonl @@ -1,2 +1,6 @@ {"skill": "gitnexus-work", "date": "2026-07-25", "task": "#2687 const-arrow Const/Function twin fix in parse-worker + MCP impact envelope", "friction": "Phase 2's Build-current/index-current procedure indexes the repo-under-test, which makes CLI-spawning suites (skip-git-cli, cli/tool-no-index-stderr) time out because repo resolution then opens the 237k-node index from that cwd; they pass at the same commit in an unindexed worktree, so the procedure manufactures false regressions in its own final verification.", "suggestion": "Phase 4 should note that CLI-spawn suites can fail solely because the worktree became an indexed repo, and prescribe the A/B check (same commit, unindexed worktree) instead of leaving the executor to conclude a regression."} {"skill": "gitnexus-work", "date": "2026-07-25", "task": "#2687 same run", "friction": "Phase 2 requires top-level `status: up-to-date` before graph queries, but any uncommitted staged edit makes status report `stale` by design, so the gate is unsatisfiable in the stage -> detect_changes -> commit sequence Phase 3 mandates.", "suggestion": "Scope the up-to-date requirement to index.commit == HEAD + empty incompleteReasons + runnerIdentityStatus current, and state that a `stale` top-level status caused solely by uncommitted working-tree edits is expected at the detect_changes gate."} +{"skill": "gitnexus-plan", "date": "2026-07-28", "task": "#2699 part B — closure binding as a call SOURCE across PHP/Rust/Kotlin/Ruby/Dart", "friction": "The safe plan writer fails closed on a v9fs (9p) worktree because renameat2(RENAME_NOREPLACE) is unsupported, returning EINVAL, so no plan can ever be published there and Phase 2's 'commit the plan document' step is unreachable.", "suggestion": "Detect the EINVAL-on-renameat2 case explicitly and fall back to open(O_EXCL)+write+fsync, which preserves the no-clobber guarantee the flag exists for; failing that, say v9fs is unsupported instead of surfacing a generic write failure."} +{"skill": "gitnexus-work", "date": "2026-07-28", "task": "#2699 part B same run", "friction": "Every language query lives in a TypeScript template literal, so a backtick inside a `;;` comment silently terminates it and produces confusing TS1005/TS1128 parse errors far from the real edit. Hit this three separate times in one session.", "suggestion": "Phase 3 should warn that *.query.ts bodies are template literals and backticks in comments are a syntax error, or the repo should add a lint rule; the build catches it but the error location does not point at the comment."} +{"skill": "gitnexus-work", "date": "2026-07-28", "task": "#2699 part B same run", "friction": "A module-level `const` derived from another const declared LOWER in the same file passes tsc and builds a clean dist, then throws ReferenceError (temporal dead zone) at import. It presents as N test FILES failing with ZERO failing assertions, which reads like host/infra flake rather than a code defect.", "suggestion": "Phase 3's verification note should call out that file-level failures with zero test failures usually mean a module-load error, and to grep the run output for ReferenceError before blaming the host."} +{"skill": "gitnexus-work", "date": "2026-07-28", "task": "#2699 part B same run", "friction": "Two concurrent `vitest run` invocations on this host starve worker-pool startup: every test in both runs fails at ~5001ms against the default GITNEXUS_WORKER_READY_TIMEOUT_MS, which looks exactly like a real regression across the whole suite.", "suggestion": "Phase 3 should state that verification runs must be serial, and that a whole-suite failure at ~5001ms is worker-startup starvation, not signal."} diff --git a/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts b/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts index b98423ca6..4c0e360a8 100644 --- a/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts +++ b/gitnexus-shared/src/scope-resolution/registries/lookup-core.ts @@ -343,7 +343,9 @@ function resolveReceiverOwner( * That twin also lists `Me`, deliberately NOT mirrored here: no entry in * `SupportedLanguages` uses it, so it can only ever exempt a variable that * happens to be called `Me`. The two lists are otherwise the same set, and - * nothing enforces that — see the drift guard noted in #2714. + * that equality — plus the `Me` exemption in both directions — is now ENFORCED + * by `gitnexus/test/unit/receiver-twin-list-drift.test.ts`. Editing either list + * without the other fails there. */ const IMPLICIT_RECEIVERS: readonly string[] = Object.freeze(['self', 'this', '$this']); diff --git a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts index 033cb7441..caca1bee3 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts @@ -28,7 +28,10 @@ import { simpleKey, type GraphNodeLookup, } from '../graph-bridge/node-lookup.js'; -import { isOverloadableCallable } from '../../utils/callable-labels.js'; +import { + isOverloadableCallable, + isPositionQualifiedLocalLabel, +} from '../../utils/callable-labels.js'; import { templateConstraintsIdTag } from '../../utils/template-arguments.js'; import { parameterShapeIdTag } from '../../utils/method-props.js'; /** @@ -206,7 +209,7 @@ export function resolveDefGraphId( // 0-based, def ids 1-based. An `AMBIGUOUS_POSITION` tombstone (two // callables on one line) falls through to the name-based keys below. const line = defStartLine(def.nodeId); - if (line !== undefined && isOverloadableCallable(def.type)) { + if (line !== undefined && isPositionQualifiedLocalLabel(def.type)) { const simple = simpleNameOf(qn); const posHit = nodeLookup.get(positionKey(filePath, def.type, line - 1, simple)); if (posHit !== undefined && posHit !== AMBIGUOUS_POSITION) return posHit; diff --git a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts index 5e2789a87..1c6b94410 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts @@ -20,7 +20,10 @@ import type { NodeLabel, ParameterTypeClass } from 'gitnexus-shared'; import type { KnowledgeGraph } from '../../../graph/types.js'; -import { isOverloadableCallable } from '../../utils/callable-labels.js'; +import { + isOverloadableCallable, + isPositionQualifiedLocalLabel, +} from '../../utils/callable-labels.js'; import { templateConstraintsIdTag } from '../../utils/template-arguments.js'; import { parameterShapeIdTag } from '../../utils/method-props.js'; @@ -135,7 +138,7 @@ export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup { // Position key (#2699) — see `positionKey`. Second write on a key marks it // ambiguous rather than letting source order decide. const startLine = (props as { startLine?: number }).startLine; - if (startLine !== undefined && isOverloadableCallable(node.label)) { + if (startLine !== undefined && isPositionQualifiedLocalLabel(node.label)) { const posK = positionKey(props.filePath, node.label, startLine, props.name); lookup.set(posK, lookup.has(posK) ? AMBIGUOUS_POSITION : node.id); // A local-identity node carries `@:` on its last name segment. Record diff --git a/gitnexus/src/core/ingestion/utils/callable-labels.ts b/gitnexus/src/core/ingestion/utils/callable-labels.ts index a4b994f43..d9f051517 100644 --- a/gitnexus/src/core/ingestion/utils/callable-labels.ts +++ b/gitnexus/src/core/ingestion/utils/callable-labels.ts @@ -14,3 +14,37 @@ import type { NodeLabel } from 'gitnexus-shared'; export function isOverloadableCallable(label: NodeLabel | undefined): boolean { return label === 'Function' || label === 'Method' || label === 'Constructor'; } + +/** + * Labels whose FUNCTION-LOCAL declarations carry the enclosing-callable + + * position identity of #2699 (`Function:x.ts:run.save@3:2`). + * + * Wider than {@link isOverloadableCallable} on purpose. #2695 restricted the + * rule to callables because the collision that produced wrong CALLS edges was + * between callables, and widening churned ids for symbols the local-symbol + * pruner mostly deletes. But the issue's ORIGINAL complaint was about values: + * a top-level `const handler` and a function-local `const handler` collapsed + * onto one `Const:v.ts:handler`, and no callable gate ever reaches that. The + * limitation is closed here rather than carried. + * + * Only LOCALS are affected either way: the prefix comes from + * `enclosingCallablePrefix`, which returns `undefined` when nothing encloses + * the declaration, so top-level and class-member ids are untouched — that is + * what keeps this off the symbols other files and stored references address. + * A class field stays unqualified even inside a function, because the prefix + * walk boundaries on class-likes. + * + * ONE definition, deliberately: the id-building phase and the resolution phase + * must agree on this set or the caller attaches to a node that does not exist + * and the edge is silently dropped — the failure mode #2714 fixed, invisible + * from outside because "zero dangling edges" is what it looks like. + */ +export function isPositionQualifiedLocalLabel(label: NodeLabel | undefined): boolean { + return ( + isOverloadableCallable(label) || + label === 'Variable' || + label === 'Const' || + label === 'Property' || + label === 'Static' + ); +} diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index da51af1ac..c8c891fb6 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -100,6 +100,7 @@ import { LOCAL_SCOPE_BODY_NODE_TYPES, type SyntaxNode, } from '../utils/ast-helpers.js'; +import { isPositionQualifiedLocalLabel } from '../utils/callable-labels.js'; import { extractCallArgTypes, type MixedChainStep } from '../utils/call-analysis.js'; import { buildTypeEnv } from '../type-env.js'; import type { ConstructorBinding } from '../type-env.js'; @@ -2297,13 +2298,16 @@ const processFileGroup = ( // #2699: a callable nested inside another callable is qualified by the // enclosing callable, so a function-local closure stops colliding with a // same-named file-level function. Restricted to CALLABLE labels: the - // collision that produced wrong CALLS edges is between callables, and - // widening it to every function-local Variable/Property would churn ids - // for symbols the local-symbol pruner mostly deletes anyway. + // Applies to VALUES as well as callables since #2699 closed A1: a + // top-level `const handler` and a function-local `const handler` + // otherwise collapse onto one `Const:v.ts:handler`, which was the + // issue's original complaint and is unreachable from a callable-only + // gate. `isPositionQualifiedLocalLabel` is the single definition of that + // set, shared with resolution in `ids.ts` — the two phases disagreeing + // silently drops edges rather than failing (#2714). // Same helper as the caller-attribution phase — see `enclosingCallablePrefix`. const nestedCallablePrefix = - (nodeLabel === 'Function' || nodeLabel === 'Method' || nodeLabel === 'Constructor') && - definitionNode + isPositionQualifiedLocalLabel(nodeLabel) && definitionNode ? enclosingCallablePrefix(definitionNode, file.path, provider) : undefined; diff --git a/gitnexus/src/storage/parse-cache.ts b/gitnexus/src/storage/parse-cache.ts index 5f8584fd9..263b4aa24 100644 --- a/gitnexus/src/storage/parse-cache.ts +++ b/gitnexus/src/storage/parse-cache.ts @@ -55,6 +55,18 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j // the main thread (the #1983 OOM). Because the two stores share this version, // any future change to the `ParsedFile` serialization shape MUST bump // SCHEMA_BUMP so both invalidate in lockstep. +// v29: closure-binding declaration rules for PHP/Rust/Kotlin/Ruby/Dart, a Rust +// graph node for `let f = || …`, a Dart closure scope, and function-local VALUES +// (Variable/Const/Property/Static) qualified by their enclosing callable plus +// position (#2699 parts A1 + B). All parse-time, so a warm cache would replay +// the old captures and the pre-qualification ids verbatim. +// +// This is 29 and not 28 because of the exact collision the v21 note below warns +// about: this branch cut at 27 and bumped to 28, while #2415 bumped 27 -> 28 and +// merged FIRST. Re-checking against origin/main at merge time — not at branch +// time — is what caught it; leaving it at 28 would have shipped this change with +// NO parse-cache invalidation, so every warm cache keeps serving the pre-fix +// captures and ids. // v28: Java/Kotlin capture side-channels persist Spring condition facts and // annotation-source line numbers (#2415). // v26: the enclosing-callable walk stops at class bodies and anonymous-class @@ -103,7 +115,7 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j // JLS 13.1 immediate-host chains (#2555). // v18: Worker$N anonymous bodies. v17: callable-value-flow operand identity. // v16: direct callee identity. -const SCHEMA_BUMP = 28; +const SCHEMA_BUMP = 29; const GITNEXUS_PKG_VERSION = (() => { try { // package.json sits at gitnexus/package.json — two levels up from diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index eb46bfd82..e3abb9967 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -525,8 +525,16 @@ export interface RepoMeta { * reads it also covered are kept. A v19 index holds those false CALLS/ACCESSES on * every unchanged file and would keep serving them through the reuse gate; force a * full re-analyze instead. + * v21: a closure bound to a name is a call SOURCE in every language, not only a + * TARGET (#2699 part B). PHP/Rust/Kotlin/Ruby/Dart closure bindings gained the + * declaration rule, Rust gained the graph NODE it never emitted, and Dart locals + * gained the enclosing-callable + position identity that made two same-named + * closures collapse onto one node — which had them asserting a CALLS edge + * present nowhere in the source. All of that changes emitted node ids AND edges + * on files that did not themselves change, so a v20 index topped up + * incrementally keeps serving the old attribution; force a full re-analyze. */ -export const INCREMENTAL_SCHEMA_VERSION = 20; +export const INCREMENTAL_SCHEMA_VERSION = 21; export interface IndexedRepo { repoPath: string; diff --git a/gitnexus/test/integration/function-local-identity.test.ts b/gitnexus/test/integration/function-local-identity.test.ts index e39b29868..e8050718e 100644 --- a/gitnexus/test/integration/function-local-identity.test.ts +++ b/gitnexus/test/integration/function-local-identity.test.ts @@ -307,24 +307,32 @@ const valueNodeIdsFor = async ( } }; -describeIfWorkerBuilt('KNOWN LIMIT — the identity fix is callable-restricted (#2699)', () => { - it('a function-local VALUE still collapses onto the file-level node', async () => { - // Pinned as a limitation, not asserted as correct. #2695 gave function-local - // CALLABLES a position-bearing id; VALUES were deliberately excluded — - // widening it would re-key ~14,700 build-time nodes to change ~800 persisted - // ones, because the pruner deletes most of them (see the decision recorded - // in `workers/parse-worker.ts`). The guard that fails closed on a position - // miss is likewise gated on `isOverloadableCallable` - // (Function | Method | Constructor), so a value never reaches it. +describeIfWorkerBuilt('function-local VALUES carry their own identity (#2699 A1)', () => { + it('a function-local VALUE does not collapse onto the file-level node', async () => { + // FLIPPED, per this test's own former instruction. It previously pinned the + // collapse as a KNOWN LIMIT: #2695 gave function-local CALLABLES a + // position-bearing id and deliberately excluded VALUES, so a top-level + // `const handler` and a function-local `const handler` shared ONE node. + // That was the residual half of #2699's ORIGINAL complaint — the issue is + // about values first, and no callable-only gate could ever reach it. // - // Consequence, measured here: a top-level `const handler` and a - // function-local `const handler` share ONE node. That is the residual half - // of #2699's original complaint, and it is why the issue is not fully - // closed by the identity work alone. + // Widened here via `isPositionQualifiedLocalLabel`, the single definition + // shared by all THREE phases that must agree: id-building + // (`parse-worker.ts`), resolution (`ids.ts` position key) and registration + // (`node-lookup.ts`). Two of them disagreeing does not fail loudly — the + // caller attaches to a node that does not exist and the edge is silently + // dropped, which is the #2714 failure mode. // - // If this ever returns two ids, the identity model was widened to values — - // which is a deliberate, schema-bumping change, so this test should be - // updated as part of it rather than deleted. + // The churn this was deferred for is real and was accepted deliberately: + // it re-keys ~14,700 build-time nodes to change ~800 persisted ones, + // because `pruneLocalSymbols` deletes most locals. Hence the paired + // INCREMENTAL_SCHEMA_VERSION / parse-cache SCHEMA_BUMP bumps — without them + // a warm cache or an incremental top-up replays the old un-suffixed ids. + // + // Only LOCALS move. The prefix comes from `enclosingCallablePrefix`, which + // returns undefined when nothing encloses the declaration, so the + // file-level `handler` below keeps its bare id — that is what keeps this + // off the symbols other files and stored references address. const ids = await valueNodeIdsFor( 'v.ts', [ @@ -339,6 +347,8 @@ describeIfWorkerBuilt('KNOWN LIMIT — the identity fix is callable-restricted ( 'handler', ); - expect(ids).toEqual(['Const:v.ts:handler']); + // Two distinct nodes: the file-level one keeps its bare id, the local + // carries its enclosing callable AND declaration position. + expect(ids).toEqual(['Const:v.ts:handler', 'Const:v.ts:run.handler@3:2']); }); }); diff --git a/gitnexus/test/unit/callable-id-lockstep.test.ts b/gitnexus/test/unit/callable-id-lockstep.test.ts index d22266418..cf7726f6b 100644 --- a/gitnexus/test/unit/callable-id-lockstep.test.ts +++ b/gitnexus/test/unit/callable-id-lockstep.test.ts @@ -64,8 +64,13 @@ describe('no call site re-inlines the rule', () => { it('parse-worker.ts contains no inlined `.${localIdentity(...)}` template', () => { // The structural half. The unit assertions above would still pass if a // fourth phase appeared and spelled the rule out by hand — which is - // exactly how the divergence #2714 fixed came to exist. This fails if any - // site reconstructs the id instead of calling the shared function. + // exactly how the divergence #2714 fixed came to exist. + // + // Scope, stated honestly: this matches ONE template spelling — the + // `${prefix}.${localIdentity(...)}` form the divergence actually took. A + // hand-rolled id built by string concatenation, or with the interpolation + // spelled differently, still slips past. It is a tripwire for the known + // shape, not a proof that no site reconstructs the id. const source = readFileSync( fileURLToPath(new URL('../../src/core/ingestion/workers/parse-worker.ts', import.meta.url)), 'utf8', diff --git a/gitnexus/test/unit/receiver-twin-list-drift.test.ts b/gitnexus/test/unit/receiver-twin-list-drift.test.ts new file mode 100644 index 000000000..24e23987a --- /dev/null +++ b/gitnexus/test/unit/receiver-twin-list-drift.test.ts @@ -0,0 +1,99 @@ +/** + * The drift guard for the implicit-receiver twin lists (#2699 follow-up). + * + * TWO lists spell "this is an implicit receiver", in two packages: + * + * - `IMPLICIT_RECEIVERS` — gitnexus-shared `lookup-core.ts`. Two consumers: + * the Step-1 lexical skip (a NAMED receiver must not resolve its member + * through the lexical chain) and `resolveReceiverOwner`. + * - `THIS_RECEIVERS` — gitnexus `type-env.ts`. Decides whether a receiver + * rewrites to the enclosing type. + * + * They are the SIXTH twin-list instance found in this family of work, and the + * previous five each shipped a bug when one side moved. `$this` was added to + * the shared list in #2714 precisely because it was already in the other one; + * nothing but this test stops the next divergence. + * + * `Me` is the one deliberate asymmetry: `THIS_RECEIVERS` carries it (Visual + * Basic spelling) and the shared list does not, because no entry in + * `SupportedLanguages` uses it — mirroring it there could only ever exempt a + * variable that happens to be named `Me`. That exemption is asserted + * explicitly rather than tolerated, so RE-adding `Me` to the shared list, or + * dropping it from the local one, both fail loudly. + * + * Structural (source-parsed) rather than value-imported: both constants are + * module-private, and exporting them purely to be testable would widen two + * public surfaces to satisfy a test. Same idiom as + * `detect-changes-local-id-stability.test.ts`. + */ +import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'fs'; +import path from 'path'; +import { fileURLToPath } from 'url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + +/** + * String literals inside the first `[...]` following `marker` that actually + * CONTAINS a string literal. + * + * "First `[`" is not good enough: `IMPLICIT_RECEIVERS` is declared + * `: readonly string[] = Object.freeze([...])`, so the first bracket belongs to + * the TYPE annotation and yields an empty list — which would make every + * assertion below vacuously pass. That is exactly what the non-empty check in + * the first test exists to catch, and it did. + */ +const literalsAfter = (source: string, marker: string): string[] => { + const at = source.indexOf(marker); + expect(at, `${marker} not found — update this test`).toBeGreaterThan(-1); + for (let open = source.indexOf('[', at); open !== -1; open = source.indexOf('[', open + 1)) { + const close = source.indexOf(']', open); + if (close === -1) break; + const names = [...source.slice(open + 1, close).matchAll(/'([^']*)'|"([^"]*)"/g)] + .map((m) => m[1] ?? m[2] ?? '') + .filter((s) => s.length > 0); + if (names.length > 0) return names.sort(); + } + return []; +}; + +const sharedList = (): string[] => + literalsAfter( + readFileSync( + path.join( + __dirname, + '../../../gitnexus-shared/src/scope-resolution/registries/lookup-core.ts', + ), + 'utf-8', + ), + 'const IMPLICIT_RECEIVERS', + ); + +const typeEnvList = (): string[] => + literalsAfter( + readFileSync(path.join(__dirname, '../../src/core/ingestion/type-env.ts'), 'utf-8'), + 'const THIS_RECEIVERS', + ); + +describe('#2699 — implicit-receiver twin lists do not drift', () => { + it('both lists are non-empty and were actually parsed', () => { + // Guards the guard: a regex that silently matched nothing would make every + // assertion below vacuously true. + expect(sharedList().length).toBeGreaterThan(0); + expect(typeEnvList().length).toBeGreaterThan(0); + }); + + it('the shared list is exactly the type-env list minus the deliberate `Me`', () => { + expect(sharedList()).toEqual(typeEnvList().filter((name) => name !== 'Me')); + }); + + it('`Me` stays OUT of the shared list', () => { + // Stated separately so the intent survives even if the set comparison above + // is ever relaxed: this asymmetry is a decision, not an oversight. + expect(sharedList()).not.toContain('Me'); + }); + + it('`Me` stays IN the type-env list', () => { + expect(typeEnvList()).toContain('Me'); + }); +});