mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-10 22:43:40 +00:00
fix(SM-14): address PR #743 post-fix review findings
Four items from the deep review on commit d3c25d20 — three Low
findings plus one informational note.
Low #1 — Expose `get disposed(): boolean` for API symmetry
- BindingAccumulator's `_disposed` field was set but never read or
exposed. `_finalized` had a public getter (`get finalized()`) but
`_disposed` did not. Added the matching `get disposed()` getter so
debug tooling and future Phase 9 consumers can detect a disposed
accumulator without inspecting empty state heuristically.
- JSDoc notes that disposal and finalization are orthogonal lifecycle
dimensions — a disposed accumulator may or may not be finalized.
Low #2 — Test for Tier 0 "don't overwrite" protection
- Production enrichment loop at pipeline.ts:1104-1108 has a priority
guard:
if (!fileExports.has(name)) { fileExports.set(name, type); }
preventing a worker-path binding from clobbering a higher-quality
Tier 0 SymbolTable entry. The existing `runEnrichmentLoop` test
helper in binding-accumulator.test.ts was missing this guard, and
no test exercised the priority branch.
- Fixed the helper to mirror the production guard.
- Added a new test: "does not overwrite existing SymbolTable entry
(Tier 0 priority)" — pre-populates exportedTypeMap with an
"SymbolTableAuthoritativeType" entry, runs the enrichment loop
against an accumulator with "WorkerInferredType" for the same name,
asserts the authoritative type survives.
Low #3 — Move finalize() to before the enrichment loop
- Previously, finalize() was called at pipeline.ts:1715 (inside
runPipelineFromRepo, AFTER runChunkedParseAndResolve had already
returned). The enrichment loop at pipeline.ts:1087 (inside
runChunkedParseAndResolve) consumed the still-mutable accumulator.
The `finalized` state was therefore not a reliable "all reads are
done" signal — it was a "no more writes" signal that arrived later
than the actual last read.
- Moved finalize() to immediately before the enrichment loop at line
1087. By that point all worker-path appends (line 934) and all
sequential-path flushes (via processCalls at line 1051, also inside
runChunkedParseAndResolve) have completed. Grep confirmed no further
`bindingAccumulator.appendFile` calls exist outside runChunkedParseAndResolve.
- Lifecycle contract is now explicit:
append phase → finalize → consume → dispose
- Replaced the old finalize() call at line 1715 with an explanatory
comment pointing to the new seam.
Informational — parsing-processor.ts TypeEnv clarification
- parsing-processor.ts builds a FieldExtractor-only TypeEnv that is
intentionally NOT flushed into the accumulator — the accumulator
feed happens later in call-processor.ts via its own flush() call.
A future reader might see `buildTypeEnv()` here and try to add a
flush call, double-counting entries and tripping the single-use
invariant.
- Added a multi-line comment explaining the ownership rule and
cross-referencing PR #743 and plan 2026-04-09-005.
Verification
- `tsc --noEmit` clean
- 3110 unit tests pass (+1 new Tier 0 priority test)
- 1766 resolver integration tests pass — critically, the finalize()
relocation did not regress any real pipeline path, proving all
writes complete before the new finalize point
- Zero regressions
Plan: docs/plans/2026-04-09-005-fix-sm14-sequential-path-memory-regression-plan.md
Review: https://github.com/abhigyanpatwari/GitNexus/pull/743#issuecomment-4216262583
This commit is contained in:
parent
d3c25d2093
commit
448a4b22a6
4 changed files with 82 additions and 4 deletions
|
|
@ -174,6 +174,19 @@ export class BindingAccumulator {
|
|||
return this._finalized;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the accumulator has been disposed. Exposed for symmetry with
|
||||
* `finalized` so debug tooling and future Phase 9 consumers can detect a
|
||||
* disposed accumulator without inspecting empty state heuristically.
|
||||
*
|
||||
* Disposal and finalization are orthogonal: a disposed accumulator may or
|
||||
* may not be finalized, and vice versa. See `dispose()` for the full
|
||||
* lifecycle contract.
|
||||
*/
|
||||
get disposed(): boolean {
|
||||
return this._disposed;
|
||||
}
|
||||
|
||||
/**
|
||||
* Rough memory estimate in bytes (intentionally pessimistic).
|
||||
* Formula: sum of (ENTRY_OVERHEAD + char bytes of scope+varName+typeName) per entry
|
||||
|
|
|
|||
|
|
@ -355,7 +355,14 @@ const processParsingSequential = async (
|
|||
continue;
|
||||
}
|
||||
|
||||
// Build per-file type environment for FieldExtractor context (lightweight — skipped if no fieldExtractor)
|
||||
// Build per-file type environment for FieldExtractor context (lightweight — skipped if no fieldExtractor).
|
||||
//
|
||||
// Note: this TypeEnv is intentionally NOT flushed into the BindingAccumulator.
|
||||
// The accumulator feed happens later in `call-processor.ts` via its own
|
||||
// `typeEnv.flush(accumulator)` call. Flushing here would double-count
|
||||
// file-scope bindings and break the single-use invariant of `flush()`.
|
||||
// See PR #743 (SM-14) and plan `docs/plans/2026-04-09-005-*.md` for the
|
||||
// full accumulator lifecycle and flush-site ownership rules.
|
||||
const typeEnv = provider.fieldExtractor
|
||||
? buildTypeEnv(tree, language, {
|
||||
enclosingFunctionFinder: provider.enclosingFunctionFinder,
|
||||
|
|
|
|||
|
|
@ -1083,6 +1083,16 @@ async function runChunkedParseAndResolve(
|
|||
);
|
||||
}
|
||||
|
||||
// ── Finalize the accumulator before the read phase begins. All worker-path
|
||||
// appends (line ~934) and sequential-path flushes (via `processCalls` →
|
||||
// `typeEnv.flush()` earlier in this function) have completed by here,
|
||||
// so the finalize-write-lock is correct at this seam. Making the
|
||||
// lifecycle contract explicit — `append → finalize → consume → dispose`
|
||||
// — was a Codex adversarial review follow-up (plan 2026-04-09-005).
|
||||
// Previously `finalize()` was called much later in `runPipelineFromRepo`
|
||||
// after the enrichment loop had already read the mutable accumulator.
|
||||
bindingAccumulator.finalize();
|
||||
|
||||
// ── Worker path quality enrichment: merge file-scope bindings into ExportedTypeMap ──
|
||||
if (bindingAccumulator.fileCount > 0) {
|
||||
let enriched = 0;
|
||||
|
|
@ -1711,8 +1721,12 @@ export const runPipelineFromRepo = async (
|
|||
processORMQueries(graph, allORMQueries, isDev);
|
||||
}
|
||||
|
||||
// Finalize — no more appends allowed
|
||||
bindingAccumulator.finalize();
|
||||
// `bindingAccumulator.finalize()` was moved inside `runChunkedParseAndResolve`
|
||||
// to immediately precede the enrichment loop — see the comment there for
|
||||
// the PR #743 Codex adversarial review rationale. By the time execution
|
||||
// reaches this point, the accumulator has already been finalized, consumed
|
||||
// by the enrichment loop, and is ready for dispose() below after the dev
|
||||
// telemetry log captures peak state.
|
||||
|
||||
if (isDev && bindingAccumulator.totalBindings > 0) {
|
||||
const memKB = Math.round(bindingAccumulator.estimateMemoryBytes() / 1024);
|
||||
|
|
|
|||
|
|
@ -403,7 +403,13 @@ describe('BindingAccumulator', () => {
|
|||
fileMap = new Map();
|
||||
exportedTypeMap.set(filePath, fileMap);
|
||||
}
|
||||
fileMap.set(name, type);
|
||||
// Tier 0 priority guard: if the SymbolTable already populated an
|
||||
// entry for this name, don't overwrite it. Mirrors
|
||||
// `pipeline.ts:1104-1108` — without this, a worker-path binding
|
||||
// could clobber a higher-quality Tier 0 SymbolTable entry.
|
||||
if (!fileMap.has(name)) {
|
||||
fileMap.set(name, type);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -497,6 +503,44 @@ describe('BindingAccumulator', () => {
|
|||
expect(() => runEnrichmentLoop(acc, nodesById, exportedTypeMap)).not.toThrow();
|
||||
expect(exportedTypeMap.has('src/missing.ts')).toBe(false);
|
||||
});
|
||||
|
||||
it('does not overwrite existing SymbolTable entry (Tier 0 priority)', () => {
|
||||
// When the SymbolTable's tier-0 extraction pass has already populated
|
||||
// an entry for a name, the accumulator enrichment loop must NOT
|
||||
// overwrite it with a (lower-quality) worker-path binding. Guards
|
||||
// against `pipeline.ts:1104-1108` regressing the priority check.
|
||||
const acc = new BindingAccumulator();
|
||||
acc.appendFile('src/utils.ts', [
|
||||
{ scope: '', varName: 'helper', typeName: 'WorkerInferredType' },
|
||||
]);
|
||||
acc.finalize();
|
||||
|
||||
// Pre-populate exportedTypeMap to simulate what SymbolTable would
|
||||
// have written in the tier-0 pass.
|
||||
const exportedTypeMap = new Map<string, Map<string, string>>([
|
||||
['src/utils.ts', new Map([['helper', 'SymbolTableAuthoritativeType']])],
|
||||
]);
|
||||
|
||||
const nodesById = new Map<string, MockGraphNode>([
|
||||
[
|
||||
'Function:src/utils.ts:helper',
|
||||
{
|
||||
id: 'Function:src/utils.ts:helper',
|
||||
label: 'Function',
|
||||
name: 'helper',
|
||||
filePath: 'src/utils.ts',
|
||||
isExported: true,
|
||||
},
|
||||
],
|
||||
]);
|
||||
|
||||
runEnrichmentLoop(acc, nodesById, exportedTypeMap);
|
||||
|
||||
// Tier 0 wins — the authoritative SymbolTable type survives.
|
||||
expect(exportedTypeMap.get('src/utils.ts')?.get('helper')).toBe(
|
||||
'SymbolTableAuthoritativeType',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue