fix(review): apply ce-code-review autofix feedback (#2081)

Review (10 reviewers) confirmed OFF-path byte-identity (adversarial + golden)
and found defects all within the --pdg path. Fixes:

- P1 same-line BasicBlock id collision: add a start-column disambiguator to
  FunctionCfg + the id (`BasicBlock:<file>:<line>:<col>:<idx>`) so two functions
  sharing a start line no longer collide under first-writer-wins addNode.
- P1 worker crash-cascade: per-file try/catch around collectFunctionCfgs so a
  CFG-build throw cannot escape to the language-group catch and silently drop
  every remaining file in the group.
- P2 edge-cap drop now logs unconditionally (input.onWarn is validator-gated/
  silent in prod) — upholds the no-silent-truncation guarantee.
- P2 Array.isArray guard before the cfgSideChannel cast in run.ts.
- P2 maxFunctionLines default: worker applies DEFAULT_PDG_MAX_FUNCTION_LINES=2000
  when unset; caps forwarded through run-analyze AnalyzeOptions (closes the
  server-path drop).
- P3 README duplicate paragraph removed; `0`-vs-default docstrings corrected;
  CLI --pdg flag made language-neutral; reachableBlocks JSDoc corrected.
- Documented the break-through-finally + stacked-label CFG limitations.
- Tests: same-line id-collision regression, standalone throw→EXIT, dead-code-
  after-return, async/generator/method coverage, strengthened labeled-continue.

Refuted: the HTTP-500 getNodeQuery finding — M0 already shipped the BasicBlock
branch + name-floor (R12/web-safety handled).

CFG + analyze-config suites: 95 tests green; golden parity (AC4) byte-identical.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Gergo Magyar 2026-06-08 20:18:02 +00:00
parent c57bbb05a0
commit e3d4cb6a87
14 changed files with 172 additions and 43 deletions

View file

@ -700,8 +700,6 @@ GitNexus builds a complete knowledge graph of your codebase through a multi-phas
**Control flow (CFG, opt-in `--pdg`)** — per-function control-flow graphs (`BasicBlock` nodes + `CFG` edges) feeding the PDG/taint substrate, currently **TypeScript & JavaScript** (#2081 M1); other languages planned. Off by default.
**Control flow (CFG, opt-in `--pdg`)** — per-function control-flow graphs (`BasicBlock` nodes + `CFG` edges) feeding the PDG/taint substrate, currently **TypeScript & JavaScript** (#2081 M1); other languages planned. Off by default.
---
## Tool Examples

View file

@ -47,7 +47,7 @@ program
.option(
'--pdg',
'Build the control-flow-graph / PDG substrate (BasicBlock nodes + CFG edges) ' +
'for TypeScript/JavaScript. Opt-in; off by default. (#2081 M1)',
'for supported languages. Opt-in; off by default. (#2081 M1)',
)
.option(
'--default-branch <branch>',

View file

@ -32,6 +32,10 @@ export class CfgBuilder {
private readonly filePath: string,
private readonly functionStartLine: number,
private readonly functionEndLine: number,
/** Start column of the owning function — disambiguates same-line functions
* in the BasicBlock ids (see {@link FunctionCfg.functionStartColumn}).
* Defaults to 0 for hand-built test CFGs that don't model columns. */
private readonly functionStartColumn: number = 0,
) {
this.entryIndex = this.newBlock(functionStartLine, functionStartLine, '', 'entry');
this.exitIndex = this.newBlock(functionEndLine, functionEndLine, '', 'exit');
@ -80,6 +84,7 @@ export class CfgBuilder {
filePath: this.filePath,
functionStartLine: this.functionStartLine,
functionEndLine: this.functionEndLine,
functionStartColumn: this.functionStartColumn,
entryIndex: this.entryIndex,
exitIndex: this.exitIndex,
blocks: this.blocks.map((b, index) => ({ index, ...b })),
@ -89,8 +94,8 @@ export class CfgBuilder {
}
/**
* Block indices reachable from `entryIndex` by following edges. Used by the
* reachability property test (R9) and as a self-check in the emit step.
* Block indices reachable from `entryIndex` by following edges. Backs the
* reachability property tests (R9) over hand-built and visitor-produced CFGs.
*/
export const reachableBlocks = (cfg: FunctionCfg): Set<number> => {
const adj = new Map<number, number[]>();

View file

@ -16,6 +16,14 @@
import type { SyntaxNode } from '../utils/ast-helpers.js';
import type { CfgVisitor, FunctionCfg } from './types.js';
/**
* Default per-function source-line cap used by the worker when the `--pdg` run
* does not specify `pdgMaxFunctionLines`. A function longer than this (almost
* always minified/generated code) is skipped rather than walked — its CFG is
* both expensive and low-value. Overridable via `PipelineOptions.pdgMaxFunctionLines`.
*/
export const DEFAULT_PDG_MAX_FUNCTION_LINES = 2000;
export interface CollectedCfgs {
readonly cfgs: readonly FunctionCfg[];
/** Functions skipped for exceeding `maxFunctionLines` (0 ⇒ none skipped). */

View file

@ -37,8 +37,12 @@ export interface CfgEmitResult {
cappedFunctions: number;
}
const basicBlockId = (filePath: string, functionStartLine: number, blockIndex: number): string =>
`BasicBlock:${filePath}:${functionStartLine}:${blockIndex}`;
const basicBlockId = (
filePath: string,
functionStartLine: number,
functionStartColumn: number,
blockIndex: number,
): string => `BasicBlock:${filePath}:${functionStartLine}:${functionStartColumn}:${blockIndex}`;
/**
* Emit BasicBlock nodes + CFG edges for every function CFG in `cfgs`.
@ -58,11 +62,11 @@ export function emitFileCfgs(
const cap = maxEdgesPerFunction > 0 ? maxEdgesPerFunction : Infinity;
for (const cfg of cfgs) {
const { filePath, functionStartLine } = cfg;
const { filePath, functionStartLine, functionStartColumn } = cfg;
for (const b of cfg.blocks) {
graph.addNode({
id: basicBlockId(filePath, functionStartLine, b.index),
id: basicBlockId(filePath, functionStartLine, functionStartColumn, b.index),
label: 'BasicBlock',
properties: {
name: '', // BasicBlock has no name column; identified by id + span
@ -87,8 +91,8 @@ export function emitFileCfgs(
);
break;
}
const sourceId = basicBlockId(filePath, functionStartLine, e.from);
const targetId = basicBlockId(filePath, functionStartLine, e.to);
const sourceId = basicBlockId(filePath, functionStartLine, functionStartColumn, e.from);
const targetId = basicBlockId(filePath, functionStartLine, functionStartColumn, e.to);
graph.addRelationship({
id: generateId('CFG', `${sourceId}->${targetId}:${e.kind}`),
type: 'CFG',

View file

@ -47,6 +47,15 @@ export interface FunctionCfg {
/** Source span of the owning function — anchors the BasicBlock node ids. */
readonly functionStartLine: number;
readonly functionEndLine: number;
/**
* Start COLUMN of the owning function. Combined with `functionStartLine` it
* disambiguates the BasicBlock node ids when two functions share a start line
* — e.g. `{ a: () => x(), b: () => y() }`, where both arrows begin on the same
* line and each restarts its block indices at 0. Without the column the ids
* collide and the graph's first-writer-wins `addNode` silently drops the
* second function's blocks and cross-wires its edges.
*/
readonly functionStartColumn: number;
readonly entryIndex: number;
readonly exitIndex: number;
readonly blocks: readonly BasicBlockData[];

View file

@ -20,6 +20,17 @@
* `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`.
* - 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.
*
* Block/edge accounting and reachability are pinned in
* `test/unit/cfg/cfg-builder.test.ts` (core) and
* `test/unit/cfg/typescript-visitor.test.ts` (this visitor, per hazard).
@ -488,7 +499,8 @@ function buildFunctionCfg(fnNode: SyntaxNode, filePath: string): FunctionCfg | u
if (!TS_FUNCTION_TYPES.has(fnNode.type)) return undefined;
const startLine = startLineOf(fnNode);
const endLine = endLineOf(fnNode);
const builder = new CfgBuilder(filePath, startLine, endLine);
const startColumn = fnNode.startPosition.column;
const builder = new CfgBuilder(filePath, startLine, endLine, startColumn);
const body = fnNode.childForFieldName('body');
if (!body) return undefined; // overload signature / abstract method — no body

View file

@ -59,15 +59,18 @@ export interface PipelineOptions {
*/
pdg?: boolean;
/**
* Per-function source-line cap for worker-side CFG construction
* (`undefined`/0 ⇒ no cap). Bounds the cost of a pathological mega-function;
* over-cap functions are skipped (no CFG emitted for them).
* Per-function source-line cap for worker-side CFG construction.
* `undefined` ⇒ the worker applies `DEFAULT_PDG_MAX_FUNCTION_LINES`; `0` ⇒ no
* cap (unlimited). Bounds the cost of a pathological mega-function; over-cap
* functions are skipped (no CFG emitted for them). No CLI flag in M1 —
* programmatic / server analyze-worker path only.
*/
pdgMaxFunctionLines?: number;
/**
* Per-function CFG edge cap for the scope-resolution emit step
* (`undefined`/0 ⇒ the emit default). Over-cap functions stop at the cap and
* log a structured drop warning (no silent truncation).
* Per-function CFG edge cap for the scope-resolution emit step.
* `undefined` ⇒ `DEFAULT_MAX_CFG_EDGES_PER_FUNCTION`; `0` ⇒ no cap (unlimited).
* Over-cap functions stop at the cap and log a structured drop warning (no
* silent truncation). No CLI flag in M1 — programmatic / server path only.
*/
pdgMaxEdgesPerFunction?: number;
/**

View file

@ -261,7 +261,8 @@ interface RunScopeResolutionInput {
* nodes or edges and a byte-identical graph.
*/
readonly pdg?: boolean;
/** Per-function CFG edge cap (0/undefined ⇒ {@link DEFAULT_MAX_CFG_EDGES_PER_FUNCTION}). */
/** Per-function CFG edge cap. `undefined` ⇒ {@link DEFAULT_MAX_CFG_EDGES_PER_FUNCTION};
* `0` ⇒ no cap (unlimited). */
readonly pdgMaxEdgesPerFunction?: number;
/**
* Optional graph-node lookup built ONCE by the caller and shared across
@ -700,22 +701,32 @@ export function runScopeResolution(
if (input.pdg === true) {
let cfgBlocks = 0;
let cfgEdges = 0;
let cfgDroppedEdges = 0;
for (const pf of emitParsedFiles) {
const cfgs = pf.cfgSideChannel as readonly FunctionCfg[] | undefined;
if (cfgs === undefined || cfgs.length === 0) continue;
const cfgs = pf.cfgSideChannel;
// Defensive: cfgSideChannel is opaque (`unknown`) and crosses the cache /
// durable store. A stale or wrong-shape value (e.g. a pre-SCHEMA_BUMP
// shard that slipped the version gate) must skip emission, not throw a
// TypeError mid-graph-build and abort scope-resolution for the language.
if (!Array.isArray(cfgs) || cfgs.length === 0) continue;
const emitted = emitFileCfgs(
graph,
cfgs,
cfgs as readonly FunctionCfg[],
input.pdgMaxEdgesPerFunction ?? DEFAULT_MAX_CFG_EDGES_PER_FUNCTION,
input.onWarn,
// Log cap-overflow drops UNCONDITIONALLY (not via input.onWarn, which is
// gated behind the semantic-model validator and silent in production) so
// the per-function edge cap never truncates the CFG silently (R6/KTD6).
(message) => logger.warn(message),
);
cfgBlocks += emitted.blocks;
cfgEdges += emitted.edges;
cfgDroppedEdges += emitted.droppedEdges;
}
if (cfgBlocks > 0) {
logger.debug(
`[scope-resolution] CFG emit (lang=${provider.language}): ` +
`${cfgBlocks} BasicBlock nodes, ${cfgEdges} CFG edges`,
`${cfgBlocks} BasicBlock nodes, ${cfgEdges} CFG edges` +
(cfgDroppedEdges > 0 ? `, ${cfgDroppedEdges} edges dropped (per-function cap)` : ''),
);
}
}

View file

@ -109,7 +109,7 @@ import {
persistDurableParsedFileShardSync,
} from '../../../storage/parsedfile-store.js';
import { extractLaravelRoutes, type ExtractedRoute } from '../route-extractors/laravel.js';
import { collectFunctionCfgs } from '../cfg/collect.js';
import { collectFunctionCfgs, DEFAULT_PDG_MAX_FUNCTION_LINES } from '../cfg/collect.js';
import { logger } from '../../logger.js';
export type { ExtractedRoute } from '../route-extractors/laravel.js';
@ -145,7 +145,8 @@ let shardSeq = 0;
// (0/undefined ⇒ no cap; see collectFunctionCfgs).
const PDG_ENABLED: boolean = (workerData as { pdg?: boolean } | undefined)?.pdg === true;
const PDG_MAX_FUNCTION_LINES: number =
(workerData as { pdgMaxFunctionLines?: number } | undefined)?.pdgMaxFunctionLines ?? 0;
(workerData as { pdgMaxFunctionLines?: number } | undefined)?.pdgMaxFunctionLines ??
DEFAULT_PDG_MAX_FUNCTION_LINES;
// ── Bootstrap-stage diagnostics (#1741) ────────────────────────────────────
// When GITNEXUS_WORKER_BOOTSTRAP=1 (or --verbose sets GITNEXUS_VERBOSE), each
@ -1224,13 +1225,25 @@ const processFileGroup = (
// carries captureSideChannel carries this — its coherence rests on the
// SCHEMA_BUMP + the pdg-folded chunk-hash key (see parse-cache.ts).
if (PDG_ENABLED && provider.cfgVisitor) {
const { cfgs } = collectFunctionCfgs(
tree.rootNode,
provider.cfgVisitor,
file.path,
PDG_MAX_FUNCTION_LINES,
);
if (cfgs.length) withChannels = { ...withChannels, cfgSideChannel: cfgs };
// Isolate the CFG build per file: a throw here (an unexpected tree-sitter
// node shape, a deep-nesting stack overflow) must NOT propagate — it
// would escape processFileGroup to the language-group catch, which treats
// any throw as "parser unavailable" and silently drops EVERY remaining
// file in the group. Skip CFG for this one file; parsing + scope
// resolution proceed unaffected (CFG is a strictly-additive opt-in).
try {
const { cfgs } = collectFunctionCfgs(
tree.rootNode,
provider.cfgVisitor,
file.path,
PDG_MAX_FUNCTION_LINES,
);
if (cfgs.length) withChannels = { ...withChannels, cfgSideChannel: cfgs };
} catch (err) {
const message = `CFG build failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`;
if (parentPort) parentPort.postMessage({ type: 'warning', message });
else logger.warn(message);
}
}
result.parsedFiles.push(withChannels);

View file

@ -121,6 +121,13 @@ export interface AnalyzeOptions {
* scope-resolution (BasicBlock/CFG emit gate). Off by default.
*/
pdg?: boolean;
/** Per-function source-line cap for worker-side CFG construction (#2081 M1).
* Forwarded to `PipelineOptions.pdgMaxFunctionLines`. No CLI flag in M1 —
* programmatic / server analyze-worker path only; the worker applies
* `DEFAULT_PDG_MAX_FUNCTION_LINES` when unset. */
pdgMaxFunctionLines?: number;
/** Per-function CFG edge cap. Forwarded to `PipelineOptions.pdgMaxEdgesPerFunction`. */
pdgMaxEdgesPerFunction?: number;
/**
* Default branch threaded into generated AGENTS.md / CLAUDE.md so the
* regression-compare example uses the configured branch instead of a
@ -515,6 +522,8 @@ export async function runFullAnalysis(
// CFG/PDG opt-in (#2081 M1). PipelineOptions.pdg fans out to the worker
// build gate (workerData.pdg) and the scope-resolution emit gate.
pdg: options.pdg === true,
pdgMaxFunctionLines: options.pdgMaxFunctionLines,
pdgMaxEdgesPerFunction: options.pdgMaxEdgesPerFunction,
},
);

View file

@ -63,10 +63,11 @@ describe('U4 — emitFileCfgs node/edge shape', () => {
expect(r.edges).toBe(rels.length);
expect(nodes.length).toBeGreaterThan(0);
// every node is a BasicBlock with the KTD3 id `BasicBlock:<file>:<funcStart>:<idx>`
// every node is a BasicBlock with the KTD3 id
// `BasicBlock:<file>:<funcStartLine>:<funcStartCol>:<idx>`
for (const n of nodes) {
expect(n.label).toBe('BasicBlock');
expect(n.id).toMatch(/^BasicBlock:src\/f\.ts:\d+:\d+$/);
expect(n.id).toMatch(/^BasicBlock:src\/f\.ts:\d+:\d+:\d+$/);
expect(n.properties.filePath).toBe('src/f.ts');
expect(n.properties.name).toBe(''); // no name column
}
@ -84,6 +85,21 @@ describe('U4 — emitFileCfgs node/edge shape', () => {
const ids = nodes.map((n) => n.id);
expect(new Set(ids).size).toBe(ids.length); // no collisions
});
it('two functions sharing a start LINE get distinct ids (start-column disambiguates)', () => {
// Both arrows begin on line 1; without the start-column segment in the id
// their block indices (each restarting at 0) collide and graph.addNode's
// first-writer-wins silently drops the second function's blocks.
const cfgs = cfgsOf(`const h = { a: () => foo(), b: () => bar() };`, 'one-line.ts');
expect(cfgs.length).toBe(2);
expect(cfgs[0].functionStartLine).toBe(cfgs[1].functionStartLine); // same line
expect(cfgs[0].functionStartColumn).not.toBe(cfgs[1].functionStartColumn); // diff column
const { graph, nodes } = recordingGraph();
emitFileCfgs(graph, cfgs);
const ids = nodes.map((n) => n.id);
expect(new Set(ids).size).toBe(ids.length); // no collision despite shared line
expect(nodes.length).toBe(cfgs[0].blocks.length + cfgs[1].blocks.length); // all blocks survive
});
});
describe('U4 — AC2: every BasicBlock is reachable from its function ENTRY', () => {
@ -108,10 +124,9 @@ describe('U4 — AC2: every BasicBlock is reachable from its function ENTRY', ()
(adj.get(e.sourceId) ?? adj.set(e.sourceId, []).get(e.sourceId)!).push(e.targetId);
for (const cfg of cfgs) {
const entryId = `BasicBlock:r.ts:${cfg.functionStartLine}:${cfg.entryIndex}`;
const fnNodeIds = nodes
.map((n) => n.id)
.filter((id) => id.startsWith(`BasicBlock:r.ts:${cfg.functionStartLine}:`));
const prefix = `BasicBlock:r.ts:${cfg.functionStartLine}:${cfg.functionStartColumn}:`;
const entryId = `${prefix}${cfg.entryIndex}`;
const fnNodeIds = nodes.map((n) => n.id).filter((id) => id.startsWith(prefix));
// BFS from ENTRY
const seen = new Set([entryId]);
const stack = [entryId];

View file

@ -88,7 +88,7 @@ describe('U7 — AC2: every BasicBlock reachable from its function ENTRY', () =>
(adj.get(e.sourceId) ?? adj.set(e.sourceId, []).get(e.sourceId)!).push(e.targetId);
for (const cfg of cfgs) {
const prefix = `BasicBlock:ten-functions.ts:${cfg.functionStartLine}:`;
const prefix = `BasicBlock:ten-functions.ts:${cfg.functionStartLine}:${cfg.functionStartColumn}:`;
const entryId = `${prefix}${cfg.entryIndex}`;
for (const id of nodeIds.filter((i) => i.startsWith(prefix))) {
expect(reaches(adj, entryId, id), `${id} unreachable from ENTRY`).toBe(true);
@ -143,11 +143,14 @@ describe('U7 — AC3: hazard topologies', () => {
expect(fn.edges.some((e) => e.kind === 'break')).toBe(true);
});
it('labeled continue returns to the outer loop header', () => {
it('labeled continue returns to the OUTER loop header (not the nearest)', () => {
const cfgs = cfgsOfFile('hazards.ts');
const fn = cfgs.find((c) => c.blocks.some((b) => b.text.includes('continue outer;')))!;
const cont = blockWith(fn, 'continue outer;');
// the outer for-of header is the first block reachable that contains "x"/"xs"
expect(fn.edges.some((e) => e.from === cont && e.kind === 'continue')).toBe(true);
// outer loop iterates `xs`; its header is the only block whose text holds "xs"
const outerHeader = blockWith(fn, 'xs');
expect(
fn.edges.some((e) => e.from === cont && e.to === outerHeader && e.kind === 'continue'),
).toBe(true);
});
});

View file

@ -284,6 +284,45 @@ describe('TS/JS CfgVisitor — non-local jumps (R10)', () => {
cfg.edges.some((e) => e.from === cont && e.to === outerHeader && e.kind === 'continue'),
).toBe(true);
});
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();');
expect(cfg.edges).toContainEqual({ from: thr, to: cfg.exitIndex, kind: 'throw' });
// the throw terminates its block — control does not fall into done()
expect(reaches(cfg, thr, block(cfg, 'done();'))).toBe(false);
// done() is still reachable via the if's false branch
expect(reachable(cfg, block(cfg, 'done();'))).toBe(true);
});
it('code after an unconditional return is emitted but unreachable from ENTRY', () => {
const cfg = cfgOf(`function f() { first(); return 1; dead(); }`);
const dead = block(cfg, 'dead();');
expect(reachable(cfg, dead)).toBe(false); // emitted, but no edge reaches it
expect(reachable(cfg, block(cfg, 'first();'))).toBe(true);
});
});
describe('TS/JS CfgVisitor — function-type coverage', () => {
// TS_FUNCTION_TYPES spans more than function_declaration/arrow. Confirm the
// body-walk produces a well-formed CFG for async / generator / method bodies.
it('builds a CFG for async functions, generators, and class methods', () => {
const code = `
async function af(x) { if (x) { await a(); } done(); }
function* gf(xs) { for (const x of xs) { yield x; } }
class C { m(x) { if (x) { p(); } else { q(); } } async am() { await z(); } }
`;
const fns = collectFunctions(parse(code));
// af, gf, m, am — four CFG-bearing functions
const cfgs = fns.map((fn) => visitor.buildFunctionCfg(fn, 'ft.ts')).filter((c) => c);
expect(cfgs.length).toBe(4);
for (const cfg of cfgs) {
expect(cfg).toBeDefined();
if (!cfg) continue;
expect(cfg.blocks[cfg.entryIndex].kind).toBe('entry');
expect(reaches(cfg, cfg.entryIndex, cfg.exitIndex)).toBe(true);
}
});
});
describe('TS/JS CfgVisitor — AC1: 10-function fixture', () => {