mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-03 02:21:44 +00:00
fix(ingestion): address tri-review + CFG-expert findings (#2081)
Corroborated findings from the tri-review (Codex + CE personas + GitNexus swarm
+ a CFG/program-analysis domain-expert lane). The OFF-path stays byte-identical;
all fixes are within the --pdg path or the benchmark.
- [Codex+CFG-expert] Exceptional `throw` edges now wire EVERY block in a try's
protected region to the handler, not just the body ENTRY. A branched try body
(`try { if (x) { use(t); } } catch`) previously left interior blocks with no
path to `catch` — a taint false-negative into the handler for the M2 PDG pass.
- [Codex+CFG-expert] An unresolved labeled jump (a stacked outer label or a
labeled non-loop block) now routes to the function EXIT instead of leaving a
dangling sink — restores the single-exit invariant post-dominator/PDG
computation needs.
- [Codex] computeChunkHash now folds pdgMaxFunctionLines/pdgMaxEdgesPerFunction
into the chunk key (not just the pdg boolean), so a warm cache built under one
cap is never served to a run with a different cap (#2038 class, extended to
the budgets). Adds PdgCacheKey; boolean form kept for back-compat.
- [perf] visitTry resolves catch/finally in a single namedChild pass (the double
`namedChildren.find` allocated two throwaway arrays).
- [adversarial] The bench `straight-line` scenario now runs at 2000->8000:
output is a constant 4 blocks so disk/heap can't see the concat path, and at
the old N a genuine O(n²) was masked by V8 cons-strings. Verified at the new N:
the array-join impl ~1.0, a rope-optimized `+=` ~1.0 (correctly not flagged),
a real O(n²) (re-join-every-append) ~3.8 — budget tightened 2.0->1.5.
- [adversarial+Codex] The bench `--check` now FAILS LOUDLY when run without
`--expose-gc` instead of silently skipping the retained-heap gate.
- Doc: re-labeled the finally-bypass as a SOUNDNESS (false-negative) limitation
tracked for M2, not mere "precision."
3 new regression tests (branched-try interior→handler, stacked-label→EXIT,
cap-fold key). 99 CFG tests pass; build clean; bench gate green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
ebfb62c1a9
commit
b51152c05d
7 changed files with 161 additions and 34 deletions
|
|
@ -1,10 +1,10 @@
|
|||
{
|
||||
"straight-line": {
|
||||
"fingerprint": "f5524690b5b7d484573710938c5e9a28e08ef0882fea95111f01575c71f4a66a",
|
||||
"scaling_budget": 2.0,
|
||||
"scaling_budget": 1.5,
|
||||
"disk_bytes_budget": 1.2,
|
||||
"heap_budget": 1.3,
|
||||
"_note": "#2081 M1: ONE function, N coalescing statements (extendBlock path). Time ~1.4-1.5 after the O(n)-fragment-join fix; disk ~1.03, retained heap ~0.87 (both linear/sub-linear). Time budget 2.0 is a coarse O(n²) tripwire (a true quadratic is ~4.0) with CI-noise headroom; the disk/heap budgets carry the tight regression detection. Re-baseline the fingerprint only on an intentional CFG-shape change."
|
||||
"_note": "#2081 M1: ONE function, N coalescing statements (extendBlock text accumulation). Runs at 2000->8000 (larger than the other scenarios — output is constant 4 blocks, so disk/heap can't see this path; the TIME ratio is the sole guard). Verified at this N: the array-join impl is ~1.0, a V8-rope-optimized `+=` is also ~1.0 (correctly NOT a real regression — ropes keep naive concat linear), but a genuine O(n²) accumulation (e.g. re-join-the-array-every-append) is ~3.8 — so budget 1.5 catches a true superlinear regression while passing linear concat. disk ~1.03, retained heap ~0.98. Re-baseline the fingerprint only on an intentional CFG-shape change."
|
||||
},
|
||||
"many-functions": {
|
||||
"fingerprint": "c167ccd83086254e2b71eca153ca4a833be14b2d2a3827ab76b49f643aad13d5",
|
||||
|
|
|
|||
|
|
@ -64,6 +64,16 @@ const SCENARIOS = [
|
|||
name: 'straight-line',
|
||||
// One function, N coalescing simple statements → all fold into one basic
|
||||
// block whose text is accumulated statement-by-statement (extendBlock).
|
||||
// Uses LARGER sizes than the other scenarios: this scenario's only cost
|
||||
// dimension is text accumulation (output size is constant — 4 blocks at any
|
||||
// N — so the disk/heap ratios can't see it), so the TIME ratio is the sole
|
||||
// guard against an extendBlock O(n²)-concat re-regression. At small N a
|
||||
// quadratic is masked by V8 cons-strings + the linear tree-walk and slips
|
||||
// under the budget; these larger sizes make a real quadratic separate
|
||||
// cleanly (verified: a `+=` regression here exceeds the budget, the
|
||||
// array-join impl stays ~1).
|
||||
small: 2000,
|
||||
large: 8000,
|
||||
gen: (n) => {
|
||||
let s = 'function f() {\n';
|
||||
for (let i = 0; i < n; i++) s += ` let v${i} = ${i} + 1;\n`;
|
||||
|
|
@ -177,15 +187,19 @@ function fingerprint(scenario) {
|
|||
}
|
||||
|
||||
function measureScenario(scenario) {
|
||||
const small = measureCollect(scenario.gen(SMALL), `${scenario.name}.ts`, REPS);
|
||||
const large = measureCollect(scenario.gen(LARGE), `${scenario.name}.ts`, REPS);
|
||||
const sizeRatio = LARGE / SMALL;
|
||||
// Per-scenario sizes (straight-line needs larger N to separate a concat
|
||||
// quadratic from noise — see its comment); the rest default to the globals.
|
||||
const nSmall = scenario.small ?? SMALL;
|
||||
const nLarge = scenario.large ?? LARGE;
|
||||
const small = measureCollect(scenario.gen(nSmall), `${scenario.name}.ts`, REPS);
|
||||
const large = measureCollect(scenario.gen(nLarge), `${scenario.name}.ts`, REPS);
|
||||
const sizeRatio = nLarge / nSmall;
|
||||
const scalingRatio = small.ms > 0 ? large.ms / small.ms / sizeRatio : 0;
|
||||
const diskRatio = small.diskBytes > 0 ? large.diskBytes / small.diskBytes / sizeRatio : 0;
|
||||
|
||||
// Memory growth (only when --expose-gc gave us a forced GC).
|
||||
const heapSmall = retainedHeapBytes(scenario.gen(SMALL), `${scenario.name}.ts`);
|
||||
const heapLarge = retainedHeapBytes(scenario.gen(LARGE), `${scenario.name}.ts`);
|
||||
const heapSmall = retainedHeapBytes(scenario.gen(nSmall), `${scenario.name}.ts`);
|
||||
const heapLarge = retainedHeapBytes(scenario.gen(nLarge), `${scenario.name}.ts`);
|
||||
const heapRatio =
|
||||
heapSmall !== null && heapLarge !== null && heapSmall > 0
|
||||
? heapLarge / heapSmall / sizeRatio
|
||||
|
|
@ -211,6 +225,18 @@ function measureScenario(scenario) {
|
|||
// ---- run ----
|
||||
|
||||
const CHECK = process.argv.includes('--check');
|
||||
|
||||
// The retained-heap budget is a primary regression detector, but it can only be
|
||||
// measured with a forced GC. Rather than let `--check` silently PASS with the
|
||||
// heap gate skipped (a green no-op if someone drops --expose-gc), fail loudly.
|
||||
if (CHECK && !GC) {
|
||||
process.stderr.write(
|
||||
'[cfg --check] FAIL: retained-heap gate requires --expose-gc. ' +
|
||||
'Run: node --expose-gc --import tsx bench/cfg/measure.mjs --check\n',
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
const results = SCENARIOS.map(measureScenario);
|
||||
|
||||
if (!CHECK) {
|
||||
|
|
|
|||
|
|
@ -20,16 +20,21 @@
|
|||
* `throw` with no catch propagates through finally to the enclosing handler.
|
||||
* - labeled `break`/`continue` resolve against the labeled loop's frame.
|
||||
*
|
||||
* Known M1 limitations (conservative under-approximations — sound for a CFG, to
|
||||
* be tightened when the downstream taint analysis needs the precision):
|
||||
* - A non-local jump (`break`/`continue`/`return`) out of a `try` that has a
|
||||
* `finally` edges directly to its target rather than routing THROUGH the
|
||||
* `finally` block first. The general fix duplicates `finally` per exit path;
|
||||
* deferred past M1. Normal completion and `throw` DO route through `finally`.
|
||||
* Known M1 limitations:
|
||||
* - SOUNDNESS GAP (M2 blocker, not mere precision): a non-local jump
|
||||
* (`break`/`continue`/`return`) out of a `try` that has a `finally` edges
|
||||
* directly to its target rather than routing THROUGH the `finally` block
|
||||
* first. A future taint/PDG pass will therefore MISS flow mediated by a
|
||||
* `finally` on the early-exit path (e.g. a value the `finally` taints or
|
||||
* sanitizes before the `return` reaches its target) — a false negative. The
|
||||
* general fix duplicates `finally` per exit path; deferred past M1 and
|
||||
* tracked for M2. Normal completion and `throw` DO route through `finally`.
|
||||
* - A `break`/`continue` to a label on a non-loop/non-switch block, and the
|
||||
* OUTER label of a doubly-labeled construct (`outer: inner: for (...)`), are
|
||||
* not modeled — the jump becomes a CFG sink (no mis-routing, just a missing
|
||||
* edge). Single-labeled loops/switches resolve correctly.
|
||||
* not modeled. The jump is conservatively routed to the function EXIT (a
|
||||
* sound over-approximation that keeps the graph single-exit — see visitBreak)
|
||||
* rather than left as a dangling sink; only the precise labeled target is
|
||||
* unmodeled. Single-labeled loops/switches resolve correctly.
|
||||
*
|
||||
* Block/edge accounting and reachability are pinned in
|
||||
* `test/unit/cfg/cfg-builder.test.ts` (core) and
|
||||
|
|
@ -206,14 +211,21 @@ class TsCfgWalk {
|
|||
private visitBreak(stmt: SyntaxNode): TraversalResult {
|
||||
const idx = this.builder.newBlock(startLineOf(stmt), endLineOf(stmt), stmt.text);
|
||||
const target = this.cfc.breakTarget(this.labelOf(stmt));
|
||||
if (target !== undefined) this.builder.edge(idx, target, 'break');
|
||||
// An unresolved target — a label this M1 visitor doesn't model (a stacked
|
||||
// outer label like `outer: inner: for`, or a labeled non-loop block) —
|
||||
// would otherwise leave this block with NO out-edge, stranding it and
|
||||
// breaking the single-exit invariant a downstream post-dominator / PDG pass
|
||||
// relies on. Conservatively route an unresolved jump to the function EXIT
|
||||
// ("escapes the function"): sound over-approximation, keeps single-exit.
|
||||
this.builder.edge(idx, target ?? this.builder.exitIndex, 'break');
|
||||
return { entry: idx, exits: [] };
|
||||
}
|
||||
|
||||
private visitContinue(stmt: SyntaxNode): TraversalResult {
|
||||
const idx = this.builder.newBlock(startLineOf(stmt), endLineOf(stmt), stmt.text);
|
||||
const target = this.cfc.continueTarget(this.labelOf(stmt));
|
||||
if (target !== undefined) this.builder.edge(idx, target, 'continue');
|
||||
// See visitBreak: an unresolved label routes to EXIT to preserve single-exit.
|
||||
this.builder.edge(idx, target ?? this.builder.exitIndex, 'continue');
|
||||
return { entry: idx, exits: [] };
|
||||
}
|
||||
|
||||
|
|
@ -424,8 +436,15 @@ class TsCfgWalk {
|
|||
|
||||
private visitTry(stmt: SyntaxNode): SeqResult {
|
||||
const bodyNode = stmt.childForFieldName('body');
|
||||
const catchClause = stmt.namedChildren.find((c) => c.type === 'catch_clause');
|
||||
const finallyClause = stmt.namedChildren.find((c) => c.type === 'finally_clause');
|
||||
// Single pass over named children — tree-sitter's `namedChildren` getter
|
||||
// allocates a fresh array on every access, so avoid the double `.find`.
|
||||
let catchClause: SyntaxNode | undefined;
|
||||
let finallyClause: SyntaxNode | undefined;
|
||||
for (let i = 0; i < stmt.namedChildCount; i++) {
|
||||
const c = stmt.namedChild(i);
|
||||
if (c?.type === 'catch_clause') catchClause = c;
|
||||
else if (c?.type === 'finally_clause') finallyClause = c;
|
||||
}
|
||||
|
||||
// Build finally first so its entry is known as both a normal join and a
|
||||
// handler target. The finally body runs in the OUTER handler context.
|
||||
|
|
@ -443,17 +462,23 @@ class TsCfgWalk {
|
|||
|
||||
// Handler for the try body: catch if present, else finally, else outer.
|
||||
const tryHandler = catchRes?.entry ?? finallyRes?.entry ?? this.currentHandler();
|
||||
const protectedStart = this.builder.blockCount;
|
||||
this.handlers.push(tryHandler);
|
||||
const bodyRes = bodyNode ? this.visitSeq(this.statementsOf(bodyNode)) : null;
|
||||
this.handlers.pop();
|
||||
|
||||
// Conservative exceptional edge: any statement in the protected region may
|
||||
// raise to the handler, not just an explicit `throw`. Without this the
|
||||
// catch/finally would be unreachable for the common `try { call() } catch`
|
||||
// shape (the exception originates inside a callee). Explicit `throw`s in the
|
||||
// body also add their own precise edge to the same handler (idempotent).
|
||||
if (bodyRes && (catchClause || finallyClause)) {
|
||||
this.builder.edge(bodyRes.entry, tryHandler, 'throw');
|
||||
// Conservative exceptional edges: ANY block in the protected region may raise
|
||||
// to the handler — not just an explicit `throw`, and not just the body ENTRY.
|
||||
// Edging every block created during the try-body walk keeps exception flow
|
||||
// sound when the body BRANCHES: an `if` / nested-try / post-branch block whose
|
||||
// interior blocks would otherwise have no path to the handler — i.e. a taint
|
||||
// false-negative into `catch` for the downstream PDG analysis. The
|
||||
// per-function edge cap bounds the count; explicit `throw`s add their own
|
||||
// (idempotent) edge to the same handler.
|
||||
if (catchClause || finallyClause) {
|
||||
for (let b = protectedStart; b < this.builder.blockCount; b++) {
|
||||
this.builder.edge(b, tryHandler, 'throw');
|
||||
}
|
||||
}
|
||||
|
||||
const exits: number[] = [];
|
||||
|
|
|
|||
|
|
@ -741,7 +741,16 @@ export async function runChunkedParseAndResolve(
|
|||
filePath: f.path,
|
||||
contentHash: fileContentHash(f.content),
|
||||
}));
|
||||
chunkHash = computeChunkHash(entries, options?.pdg === true);
|
||||
chunkHash = computeChunkHash(
|
||||
entries,
|
||||
options?.pdg === true
|
||||
? {
|
||||
pdg: true,
|
||||
maxFunctionLines: options?.pdgMaxFunctionLines,
|
||||
maxEdgesPerFunction: options?.pdgMaxEdgesPerFunction,
|
||||
}
|
||||
: false,
|
||||
);
|
||||
}
|
||||
|
||||
const cachedRaw =
|
||||
|
|
|
|||
|
|
@ -141,18 +141,37 @@ export const fileContentHash = (content: Buffer | string): string => sha256Hex(c
|
|||
* in the chunk. We sort by filePath before hashing so chunks composed of
|
||||
* the same files in different order produce the same key.
|
||||
*/
|
||||
/** PDG/CFG cache namespace (#2081 M1) — every input that changes the emitted
|
||||
* `cfgSideChannel` must be folded into the chunk key. */
|
||||
export interface PdgCacheKey {
|
||||
readonly pdg?: boolean;
|
||||
/** Per-function source-line cap (changes WHICH functions get a CFG). */
|
||||
readonly maxFunctionLines?: number;
|
||||
/** Per-function edge cap (changes how many edges a function's CFG emits). */
|
||||
readonly maxEdgesPerFunction?: number;
|
||||
}
|
||||
|
||||
export const computeChunkHash = (
|
||||
entries: Array<{ filePath: string; contentHash: string }>,
|
||||
pdg = false,
|
||||
pdg: boolean | PdgCacheKey = false,
|
||||
): string => {
|
||||
const sorted = [...entries].sort((a, b) => (a.filePath < b.filePath ? -1 : 1));
|
||||
const joined = sorted.map((e) => `${e.filePath}:${e.contentHash}`).join('\n');
|
||||
// Fold the `--pdg` opt-in into the key (#2081 M1) so a chunk cached WITHOUT a
|
||||
// CFG (`cfgSideChannel`) is NOT reused on a `--pdg` run, and vice-versa — the
|
||||
// #2038-class warm-cache trap where an option-blind key silently serves
|
||||
// field-less shards. Only prefixed when `pdg` is on, so the default (pdg-off)
|
||||
// path keeps its existing keys and warm caches survive this change.
|
||||
return sha256Hex(pdg ? `pdg\n${joined}` : joined);
|
||||
const opts: PdgCacheKey = typeof pdg === 'boolean' ? { pdg } : pdg;
|
||||
// pdg-off path keeps its pre-#2081 keys verbatim, so existing warm caches
|
||||
// survive this change untouched.
|
||||
if (!opts.pdg) return sha256Hex(joined);
|
||||
// Fold the FULL --pdg configuration into the key — not just the boolean, but
|
||||
// the budgets that change the emitted CFG (`maxFunctionLines` decides which
|
||||
// functions get a CFG at all; `maxEdgesPerFunction` decides how many edges
|
||||
// each emits). Without the budgets a warm chunk built under one cap is served
|
||||
// to a run with a different cap → a stale/under-built CFG: the #2038-class
|
||||
// option-blind-key trap, extended to the caps. `def` marks an unset (default)
|
||||
// value so two default-cap runs share a key.
|
||||
const ns =
|
||||
`pdg:1;maxFn=${opts.maxFunctionLines ?? 'def'};` +
|
||||
`maxEdge=${opts.maxEdgesPerFunction ?? 'def'}`;
|
||||
return sha256Hex(`${ns}\n${joined}`);
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -116,4 +116,23 @@ describe('U3 — parse-cache key folds the --pdg flag (R4, #2038-class guard)',
|
|||
it('default (no flag arg) equals the explicit pdg-off key — warm caches survive the change', () => {
|
||||
expect(computeChunkHash(entries)).toBe(computeChunkHash(entries, false));
|
||||
});
|
||||
|
||||
it('the boolean form equals the object form with the same flag (back-compat)', () => {
|
||||
expect(computeChunkHash(entries, true)).toBe(computeChunkHash(entries, { pdg: true }));
|
||||
expect(computeChunkHash(entries, false)).toBe(computeChunkHash(entries, { pdg: false }));
|
||||
});
|
||||
|
||||
it('the cap budgets are folded into the key — a different maxFunctionLines/edges re-dispatches', () => {
|
||||
// Guards the #2038-class trap for the caps: a warm chunk built under one cap
|
||||
// must NOT be served to a --pdg run with a different cap (the emitted CFG
|
||||
// differs). Different cap value ⇒ different key.
|
||||
const base = computeChunkHash(entries, { pdg: true });
|
||||
expect(computeChunkHash(entries, { pdg: true, maxFunctionLines: 500 })).not.toBe(base);
|
||||
expect(computeChunkHash(entries, { pdg: true, maxEdgesPerFunction: 100 })).not.toBe(base);
|
||||
// Same cap values ⇒ same key (deterministic, order-independent).
|
||||
const reordered = [...entries].reverse();
|
||||
expect(computeChunkHash(entries, { pdg: true, maxFunctionLines: 500 })).toBe(
|
||||
computeChunkHash(reordered, { pdg: true, maxFunctionLines: 500 }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -239,6 +239,21 @@ describe('TS/JS CfgVisitor — try/catch/finally (R10)', () => {
|
|||
expect(reaches(cfg, block(cfg, 'risky();'), fin)).toBe(true);
|
||||
expect(reaches(cfg, fin, block(cfg, 'afterTry();'))).toBe(true);
|
||||
});
|
||||
|
||||
it('an INTERIOR block of a branched try body reaches the handler (not just the body entry)', () => {
|
||||
// Regression guard: the exceptional edge must cover every protected-region
|
||||
// block, else a throw from inside a branch is invisible to the catch (a
|
||||
// taint false-negative into `catch` for the downstream PDG analysis).
|
||||
const cfg = cfgOf(`function f(x) {
|
||||
try {
|
||||
guardEntry();
|
||||
if (x) { deep(); }
|
||||
} catch (e) { handler(e); }
|
||||
}`);
|
||||
const handler = block(cfg, 'handler(e);');
|
||||
expect(reaches(cfg, block(cfg, 'deep();'), handler)).toBe(true); // interior → handler
|
||||
expect(reaches(cfg, block(cfg, 'guardEntry();'), handler)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('TS/JS CfgVisitor — non-local jumps (R10)', () => {
|
||||
|
|
@ -285,6 +300,20 @@ describe('TS/JS CfgVisitor — non-local jumps (R10)', () => {
|
|||
).toBe(true);
|
||||
});
|
||||
|
||||
it('an unresolved labeled jump (stacked outer label) routes to EXIT, not a dangling sink', () => {
|
||||
// `break outer` can't resolve (the outer label is unmodeled in M1), but the
|
||||
// block must still reach EXIT so the graph stays single-exit for the
|
||||
// downstream post-dominator / PDG computation — never a stranded sink.
|
||||
const cfg = cfgOf(`function f(xs, ys) {
|
||||
outer: inner: for (const x of xs) {
|
||||
for (const y of ys) { if (x === y) { break outer; } body(); }
|
||||
}
|
||||
}`);
|
||||
const brk = block(cfg, 'break outer;');
|
||||
expect(edgeKinds(cfg).has('break')).toBe(true);
|
||||
expect(reaches(cfg, brk, cfg.exitIndex)).toBe(true); // not stranded
|
||||
});
|
||||
|
||||
it('a standalone throw (no enclosing try) wires to EXIT and ends its block', () => {
|
||||
const cfg = cfgOf(`function f(x) { if (x) { throw new Error(); } done(); }`);
|
||||
const thr = block(cfg, 'throw new Error();');
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue