fix(ingestion): address PR review — Python class-const prune + golden regen

Resolves the two blocking review findings on #2065:

- Python class-level constants/attributes were silently pruned.
  tree-sitter-python models a class body as a `block` node, so
  `determineScope` classified class attributes as function-locals
  (scope='block'). Value labels get no owner edge, so they carried only
  the structural File->DEFINES edge and the prune pass deleted any
  unreferenced one. `determineScope` now skips a `block` whose parent is
  a class/type declaration, so class members resolve to their true
  enclosing scope and survive. Other languages use dedicated class-body
  node types and were already safe.
- Regenerated the stale `pipeline-graph-golden` snapshot for the 4 pruned
  block-local Const (symbols 37->33, DEFINES 20->16) — the drift that
  turned CI red. Regenerated from the git-tracked fixture only.

Non-blocking review items:
- Thread `keepLocalValueSymbols` through `PipelineOptions` so long-running
  hosts can opt out per-call without mutating `process.env`.
- Drop the always-false `isFileDefinesEdge` call in the pruner's
  candidate-as-source branch (any outgoing edge => keep) and simplify the
  helper now that it is only called for incoming edges.
- Declare `pruneLocalSymbols` explicitly on `communities`/`processes` so
  they always read the trimmed graph even if `mro` is ever skipped.
- Document the new phase + env var/option in ARCHITECTURE.md and README;
  note pruning still runs under `skipGraphPhases`.
- Tests: Python class-constant survival (unit determineScope + pipeline
  integration), candidate-as-source, unset-scope, and the option hatch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
abhigyanpatwari 2026-06-07 18:20:15 +05:30
parent 03e2a15fba
commit d616787abb
12 changed files with 201 additions and 32 deletions

View file

@ -77,11 +77,11 @@ Monorepo: **CLI/MCP** (`gitnexus/`) + **browser UI** (`gitnexus-web/`).
## Pipeline Phase DAG
12 phases defined in `gitnexus/src/core/ingestion/pipeline-phases/`, each with explicit `deps` and typed output.
14 phases defined in `gitnexus/src/core/ingestion/pipeline-phases/`, each with explicit `deps` and typed output.
```
scan → structure → [markdown, cobol] → parse → [routes, tools, orm]
→ crossFile → mro → communities → processes
→ crossFile → scopeResolution → pruneLocalSymbols → mro → communities → processes
```
| Phase | File | Deps | Output |
@ -95,9 +95,11 @@ scan → structure → [markdown, cobol] → parse → [routes, tools, orm]
| `tools` | `tools.ts` | `parse` | Tool nodes + HANDLES_TOOL edges |
| `orm` | `orm.ts` | `parse` | QUERIES edges (Prisma, Supabase) |
| `crossFile` | `cross-file.ts` + `cross-file-impl.ts` | `parse`, `routes`, `tools`, `orm` | Cross-file type propagation in topological import order |
| `mro` | `mro.ts` | `crossFile`, `structure` | METHOD_OVERRIDES + METHOD_IMPLEMENTS edges |
| `communities` | `communities.ts` | `mro`, `structure` | Community nodes + MEMBER_OF edges (Leiden algorithm) |
| `processes` | `processes.ts` | `communities`, `routes`, `tools`, `structure` | Process nodes + STEP_IN_PROCESS edges |
| `scopeResolution` | `scope-resolution/pipeline/phase.ts` | `parse`, `crossFile`, `structure` | Binding/reference + inheritance edges; disposes BindingAccumulator |
| `pruneLocalSymbols` | `prune-local-symbols.ts` | `scopeResolution` | Drops inert block-local `Const`/`Variable`/`Static` nodes (only a `File→DEFINES` edge) post-resolution |
| `mro` | `mro.ts` | `crossFile`, `scopeResolution`, `pruneLocalSymbols`, `structure` | METHOD_OVERRIDES + METHOD_IMPLEMENTS edges |
| `communities` | `communities.ts` | `mro`, `pruneLocalSymbols`, `structure` | Community nodes + MEMBER_OF edges (Leiden algorithm) |
| `processes` | `processes.ts` | `communities`, `routes`, `tools`, `pruneLocalSymbols`, `structure` | Process nodes + STEP_IN_PROCESS edges |
**Non-phase files in the same directory:** `parse-impl.ts`, `cross-file-impl.ts` (implementation), `wildcard-synthesis.ts` (whole-module import expansion), `orm-extraction.ts` (sequential ORM fallback), `types.ts`, `runner.ts`, `index.ts`.
@ -119,7 +121,8 @@ scan → structure → [markdown, cobol] → parse → [routes, tools, orm]
- **Single graph accumulator** — all phases mutate the same `KnowledgeGraph` in `ctx`; the graph is the primary output.
- **Typed phase access**`getPhaseOutput<T>(deps, 'name')` for type-safe upstream results.
- **Binding accumulator lifecycle** — created in `parse`, disposed by `crossFile` (in `finally`). No other phase should take ownership.
- **Skippable phases**`skipGraphPhases` omits MRO/communities/processes (faster tests). `skipWorkers` forces sequential parsing.
- **Skippable phases**`skipGraphPhases` omits MRO/communities/processes (faster tests); `pruneLocalSymbols` still runs (it is graph cleanup, not analysis). `skipWorkers` forces sequential parsing.
- **Local-symbol pruning**`pruneLocalSymbols` removes inert block-local value symbols after scope resolution has consumed them. Opt out per-call with `PipelineOptions.keepLocalValueSymbols` or globally with the `GITNEXUS_KEEP_LOCAL_VALUE_SYMBOLS` env var.
### How to add a new phase

View file

@ -423,6 +423,16 @@ Three env vars expose the pool's resilience layers (respawn budget, cumulative-t
| `GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS` | `5 × subBatchTimeoutMs` | Total retry wall-time budget per job before quarantining. Bounds exponentially-growing retry waits. |
| `GITNEXUS_WORKER_CONSECUTIVE_FAILURE_THRESHOLD` | `max(3, poolSize)` | Per-slot consecutive deaths before the pool's circuit breaker trips. After tripping, dispatches require a fresh pool. |
### Graph cleanup tuning
After scope resolution, analyze prunes inert block-local value symbols (a function-local `const`/`let`/`var` that ends up with only its structural `File→DEFINES` edge) to keep the graph focused on cross-symbol relationships. Module/file-scope symbols, class members, and any local with a real edge are always kept.
| Variable | Default | Effect |
| ------------------------------------ | ------- | ------------------------------------------------------------------------------------------------------- |
| `GITNEXUS_KEEP_LOCAL_VALUE_SYMBOLS` | unset | Set to `1`/`true` to keep inert block-local value symbols instead of pruning them. |
Programmatic callers can pass `keepLocalValueSymbols: true` in `PipelineOptions` instead of setting the env var.
## Privacy
- All processing happens locally on your machine

View file

@ -27,13 +27,11 @@ const isLocalValueCandidate = (node: GraphNode): boolean => {
return node.properties.scope === 'block';
};
const isFileDefinesEdgeToCandidate = (
graph: KnowledgeGraph,
rel: GraphRelationship,
candidateId: string,
): boolean => {
// True when `rel` is the structural `File -> DEFINES -> candidate` edge. Callers
// guard on the candidate already being the edge target, so only the source label
// needs checking here.
const isFileDefinesEdge = (graph: KnowledgeGraph, rel: GraphRelationship): boolean => {
if (rel.type !== 'DEFINES') return false;
if (rel.targetId !== candidateId) return false;
return graph.getNode(rel.sourceId)?.label === 'File';
};
@ -54,14 +52,16 @@ export const pruneLocalValueSymbols = (
const candidatesWithSemanticEdges = new Set<string>();
for (const rel of graph.iterRelationships()) {
// Any outgoing edge from a candidate is a semantic edge: the only structural
// edge a block-local value symbol carries is the incoming File -> DEFINES, on
// which the candidate is the target, never the source.
if (candidateIds.has(rel.sourceId)) {
if (!isFileDefinesEdgeToCandidate(graph, rel, rel.sourceId)) {
candidatesWithSemanticEdges.add(rel.sourceId);
}
candidatesWithSemanticEdges.add(rel.sourceId);
}
// An incoming edge is semantic unless it is the structural File -> DEFINES.
if (candidateIds.has(rel.targetId)) {
if (!isFileDefinesEdgeToCandidate(graph, rel, rel.targetId)) {
if (!isFileDefinesEdge(graph, rel)) {
candidatesWithSemanticEdges.add(rel.targetId);
}
}

View file

@ -4,7 +4,7 @@
* Detects code communities via Leiden algorithm and creates
* Community nodes + MEMBER_OF edges.
*
* @deps mro
* @deps mro, pruneLocalSymbols
* @reads graph (all nodes and relationships)
* @writes graph (Community nodes, MEMBER_OF edges)
*/
@ -22,7 +22,10 @@ export interface CommunitiesOutput {
export const communitiesPhase: PipelinePhase<CommunitiesOutput> = {
name: 'communities',
deps: ['mro', 'structure'],
// `pruneLocalSymbols` is declared explicitly (not just transitively via `mro`)
// so community detection always reads the trimmed graph even if a future option
// drops `mro` from the phase list.
deps: ['mro', 'pruneLocalSymbols', 'structure'],
async execute(
ctx: PipelineContext,

View file

@ -4,7 +4,7 @@
* Detects execution flows (processes) and creates Process nodes +
* STEP_IN_PROCESS edges. Also links Route/Tool nodes to processes.
*
* @deps communities, routes, tools
* @deps communities, routes, tools, pruneLocalSymbols
* @reads graph (all nodes and relationships), communityResult, routeRegistry, toolDefs
* @writes graph (Process nodes, STEP_IN_PROCESS edges, ENTRY_POINT_OF edges)
*/
@ -27,8 +27,10 @@ export interface ProcessesOutput {
export const processesPhase: PipelinePhase<ProcessesOutput> = {
name: 'processes',
// `structure` supplies `totalFiles` (progress counter) without the spurious
// structural data dependency on `parse`.
deps: ['communities', 'routes', 'tools', 'structure'],
// structural data dependency on `parse`. `pruneLocalSymbols` is declared
// explicitly so process extraction always reads the trimmed graph even if a
// future option drops the intervening `mro`/`communities` phases.
deps: ['communities', 'routes', 'tools', 'pruneLocalSymbols', 'structure'],
async execute(
ctx: PipelineContext,

View file

@ -21,7 +21,9 @@ export const pruneLocalSymbolsPhase: PipelinePhase<PruneLocalSymbolsOutput> = {
deps: ['scopeResolution'],
async execute(ctx: PipelineContext): Promise<PruneLocalSymbolsOutput> {
const stats = pruneLocalValueSymbols(ctx.graph);
const stats = pruneLocalValueSymbols(ctx.graph, {
keepLocalValueSymbols: ctx.options?.keepLocalValueSymbols,
});
if (isDev && !stats.skippedByEnv && stats.prunedNodes > 0) {
logger.info(`Pruned ${stats.prunedNodes}/${stats.candidateNodes} inert local value symbols`);

View file

@ -42,7 +42,12 @@ import {
} from './pipeline-phases/index.js';
export interface PipelineOptions {
/** Skip MRO, community detection, and process extraction for faster test runs. */
/**
* Skip MRO, community detection, and process extraction for faster test runs.
* The `pruneLocalSymbols` phase still runs it is graph construction (it cleans
* up inert local symbols), not graph analysis so set `keepLocalValueSymbols`
* to retain those nodes under `skipGraphPhases`.
*/
skipGraphPhases?: boolean;
/**
* Request parsing with the worker pool disabled. The sequential parser was
@ -115,6 +120,14 @@ export interface PipelineOptions {
* without leaking `process.env` state across invocations.
*/
chunkByteBudget?: number;
/**
* Keep inert block-local value symbols (Const/Variable/Static) that the
* `pruneLocalSymbols` phase would otherwise drop. Mirrors the
* `GITNEXUS_KEEP_LOCAL_VALUE_SYMBOLS` env var, but threaded per-call so
* long-running hosts (eval-server, MCP daemon) can opt out without leaking
* `process.env` state across invocations. When undefined, the env var decides.
*/
keepLocalValueSymbols?: boolean;
}
// ── Phase registry ─────────────────────────────────────────────────────────

View file

@ -18,6 +18,26 @@ import type {
VariableScope,
} from '../variable-types.js';
/**
* Type-declaration node types whose body can be a bare `block`. tree-sitter-python
* models a class body as a `block` node the same node type used for function and
* control-flow bodies so a class attribute would otherwise look block-scoped. Most
* other grammars give class bodies dedicated node types (`class_body`,
* `declaration_list`, `body_statement`), which are not in the block-scope list below,
* so this guard is a no-op for them but keeps the rule language-agnostic.
*/
const CLASS_LIKE_CONTAINERS = new Set<string>([
'class_definition',
'class_declaration',
'class_specifier',
'struct_item',
'impl_item',
'trait_item',
'interface_declaration',
'enum_declaration',
'object_declaration',
]);
/**
* Create a VariableExtractor from a declarative config.
*/
@ -48,7 +68,7 @@ export function createVariableExtractor(config: VariableExtractionConfig): Varia
) {
return 'module';
}
// Function/method/block boundaries indicate block scope
// Function/method boundaries indicate block scope
if (
t === 'function_declaration' ||
t === 'function_definition' ||
@ -58,12 +78,18 @@ export function createVariableExtractor(config: VariableExtractionConfig): Varia
t === 'arrow_function' ||
t === 'function_expression' ||
t === 'lambda' ||
t === 'block' ||
t === 'function_body' ||
t === 'compound_statement'
) {
return 'block';
}
// A bare `block` is block scope UNLESS it is a class body. A class member
// (e.g. Python `class C: MAX = 100`) is not an inert function-local — keep
// walking so it resolves to its true enclosing scope ('module' for a
// top-level class) instead of being misclassified and pruned.
if (t === 'block' && !(current.parent && CLASS_LIKE_CONTAINERS.has(current.parent.type))) {
return 'block';
}
current = current.parent;
}
return 'file';

View file

@ -2,13 +2,12 @@
"capture": "initial capture (U8, post-U1U7)",
"fixture": "mini-repo",
"totalFileCount": 7,
"symbols": 37,
"relationships": 73,
"symbols": 33,
"relationships": 69,
"processes": 4,
"byType": {
"Class": 1,
"Community": 4,
"Const": 4,
"File": 7,
"Folder": 1,
"Function": 12,
@ -19,11 +18,11 @@
"byRelType": {
"CALLS": 9,
"CONTAINS": 7,
"DEFINES": 20,
"DEFINES": 16,
"HAS_METHOD": 1,
"IMPORTS": 12,
"MEMBER_OF": 12,
"STEP_IN_PROCESS": 12
},
"edgeDigest": "196198eb9a900721aec892400d141189aa1e874722917843c7f1206805d817f1"
"edgeDigest": "1e80aba78cb1784276d387e9debd006ae4e82e60ce33c59e338aac4886d98f61"
}

View file

@ -9,10 +9,10 @@ const describeIfWorkerBuilt = distWorkerExists() ? describe : describe.skip;
let tmpDirs: string[] = [];
const makeRepo = (source: string): string => {
const makeRepo = (source: string, filename = 'sample.ts'): string => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-local-prune-'));
tmpDirs.push(dir);
fs.writeFileSync(path.join(dir, 'sample.ts'), source, 'utf-8');
fs.writeFileSync(path.join(dir, filename), source, 'utf-8');
return dir;
};
@ -68,4 +68,40 @@ class Client {
});
expect(keepsResolvedClientCall).toBe(true);
});
it('keeps Python class-level constants while pruning function-locals', async () => {
// Regression: tree-sitter-python models the class body as a `block` node, so
// `determineScope` classified an untyped class attribute as block-scope. Python
// emits such assignments as `Variable` (the `@definition.variable` capture), and
// value labels get no owner edge (needsOwner excludes them) — only File->DEFINES.
// The prune pass would therefore silently delete unreferenced class attributes.
// A class-level symbol is NOT a function-local and must survive.
const repo = makeRepo(
`MODULE_CONST = 1
class Settings:
MAX_SIZE = 100
def run():
boring = 1
return boring
`,
'sample.py',
);
const result = await runPipelineFromRepo(repo, () => {}, {
skipGraphPhases: true,
workerPoolSize: 1,
workerUrlForTest: DIST_WORKER_URL,
});
// Unreferenced class-level attribute must survive the prune.
expect(findNode(result, 'Variable', 'MAX_SIZE')).toBeDefined();
// Module-level symbol is preserved as before.
expect(findNode(result, 'Variable', 'MODULE_CONST')).toBeDefined();
// Genuine function-local is still pruned.
expect(findNode(result, 'Variable', 'boring')).toBeUndefined();
});
});

View file

@ -106,6 +106,36 @@ describe('pruneLocalValueSymbols', () => {
expect(graph.relationshipCount).toBe(2);
});
it('keeps block-scope value symbols that are the source of an outgoing edge', () => {
const graph = createKnowledgeGraph();
graph.addNode(fileNode());
graph.addNode(node('Element:thing', 'CodeElement'));
graph.addNode(node('Const:config', 'Const', { scope: 'block' }));
graph.addRelationship(rel('rel:file-def', 'file:src/app.ts', 'Const:config'));
// Candidate is the SOURCE of an outgoing DEFINES edge — any outgoing edge is
// semantic, so the node must be kept (guards the source-branch simplification).
graph.addRelationship(rel('rel:out', 'Const:config', 'Element:thing', 'DEFINES'));
const stats = pruneLocalValueSymbols(graph);
expect(stats.prunedNodes).toBe(0);
expect(stats.keptWithSemanticEdges).toBe(1);
expect(graph.getNode('Const:config')).toBeDefined();
});
it('does not treat value symbols without a scope property as candidates', () => {
const graph = createKnowledgeGraph();
graph.addNode(fileNode());
graph.addNode(node('Const:noScope', 'Const'));
graph.addRelationship(rel('rel:def', 'file:src/app.ts', 'Const:noScope'));
const stats = pruneLocalValueSymbols(graph);
expect(stats.candidateNodes).toBe(0);
expect(stats.prunedNodes).toBe(0);
expect(graph.getNode('Const:noScope')).toBeDefined();
});
it('prunes block-scope value symbols even when parser metadata marks them exported', () => {
const graph = createKnowledgeGraph();
graph.addNode(fileNode());
@ -184,4 +214,26 @@ describe('pruneLocalSymbolsPhase', () => {
expect(stats.prunedNodes).toBe(1);
expect(graph.getNode('Const:tmp')).toBeUndefined();
});
it('honors the keepLocalValueSymbols option without reading process.env', async () => {
const graph = createKnowledgeGraph();
graph.addNode(fileNode());
graph.addNode(node('Const:tmp', 'Const', { scope: 'block' }));
graph.addRelationship(rel('rel:def', 'file:src/app.ts', 'Const:tmp'));
const stats = await pruneLocalSymbolsPhase.execute(
{
repoPath: '/repo',
graph,
onProgress: () => {},
pipelineStart: Date.now(),
options: { keepLocalValueSymbols: true },
},
new Map(),
);
expect(stats.skippedByEnv).toBe(true);
expect(stats.prunedNodes).toBe(0);
expect(graph.getNode('Const:tmp')).toBeDefined();
});
});

View file

@ -686,6 +686,29 @@ describe('VariableExtractor — block-scoped declarations', () => {
expect(info!.scope).toBe('block');
});
it('Python: class-body attribute is not block-scoped (class body is a `block` node)', () => {
// Regression: tree-sitter-python models the class body as a `block` node, the
// same node type as a function body. A class attribute must NOT be classified
// as a function-local block, or the pruner would silently drop it.
const extractor = createVariableExtractor(pythonVariableConfig);
const ctx: VariableExtractorContext = {
filePath: 'test.py',
language: SupportedLanguages.Python,
};
parser.setLanguage(Python);
const tree = parser.parse('class Settings:\n MAX_SIZE = 100');
// module > class_definition > block > expression_statement > assignment
const classDef = tree.rootNode.namedChildren.find((c) => c.type === 'class_definition')!;
const body = classDef.childForFieldName('body')!;
const exprStmt = body.namedChildren.find((c) => c.type === 'expression_statement');
expect(exprStmt).toBeDefined();
const info = extractor.extract(exprStmt!, ctx);
expect(info).not.toBeNull();
expect(info!.name).toBe('MAX_SIZE');
expect(info!.scope).not.toBe('block');
expect(info!.scope).toBe('module');
});
it('Python: rejects non-assignment expression statements (e.g. function calls)', () => {
const extractor = createVariableExtractor(pythonVariableConfig);
const ctx: VariableExtractorContext = {