From db5763b4c2fb658a4f2058268d2d59c1208e32eb Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 13:40:52 -0400 Subject: [PATCH 1/9] fix: remediate audit priorities 3, 5, and 6 - H1/H2: make transitive BFS depth-aware with copy-on-write reasons - P5/P6/P7: add LRU caps and teardown for module/resolution caches - P7: trim unloaded lazy symbol modules and add clear() - SQLite: read on-disk schema_version and migrate v1 to v2 explicitly Adds regression tests for order-independent transitive impact, cache eviction/clear, lazy module trimming, and schema version upgrades. --- .../2026-06-21-codegraph-technical-audit.md | 6 +- src/impact/transitive.ts | 45 +++-- src/indexer/build-cache.ts | 1 + src/indexer/build-cache/module-cache.ts | 21 ++- src/sqlite/schema.ts | 164 +++++++++++------- src/util/lazySymbols.ts | 28 ++- src/util/lruMap.ts | 21 +++ src/util/resolution.ts | 36 ++-- tests/impact-transitive-order.test.ts | 97 +++++++++++ tests/lazy-symbols.test.ts | 28 +++ tests/lru-map.test.ts | 27 +++ tests/module-cache-lru.test.ts | 40 +++++ tests/resolution-cache-lru.test.ts | 31 ++++ tests/sqlite.test.ts | 65 +++++++ 14 files changed, 507 insertions(+), 103 deletions(-) create mode 100644 src/util/lruMap.ts create mode 100644 tests/impact-transitive-order.test.ts create mode 100644 tests/lru-map.test.ts create mode 100644 tests/module-cache-lru.test.ts create mode 100644 tests/resolution-cache-lru.test.ts diff --git a/docs/plans/2026-06-21-codegraph-technical-audit.md b/docs/plans/2026-06-21-codegraph-technical-audit.md index 149536a1..20fa5ce1 100644 --- a/docs/plans/2026-06-21-codegraph-technical-audit.md +++ b/docs/plans/2026-06-21-codegraph-technical-audit.md @@ -273,10 +273,10 @@ Ordered checklist. Each item should land with a regression test in the same chan - [x] **1. C1 git injection** (`util/git.ts`) -- `assertSafeRevision` + `--end-of-options`; test crafted `--output=`/`--upload-pack=` revisions are rejected. - [x] **2. C3 unguarded parse** (`impact/callCompatibility.ts`) -- wrap the three `ensureParsedContext` calls in try/catch + `incrementSkippedReason`; test a deleted/unparseable file no longer zeroes the report. -- [ ] **3. H1/H2 transitive BFS** (`impact/transitive.ts`) -- copy-on-write `reasons`, upgrade severity/reasons on a stronger path; test order-independence. +- [x] **3. H1/H2 transitive BFS** (`impact/transitive.ts`) -- copy-on-write `reasons`, upgrade severity/reasons on a stronger path; test order-independence. - [x] **4. C2 stale incremental graph** (`indexer/build-index.ts`, `incremental-plan.ts`) -- reverse-dependency closure over all changed files; test dependents re-resolve after an export change. -- [ ] **5. P6/P7/P5 unbounded caches** (`module-cache.ts`, `lazySymbols.ts`, `resolution.ts`) -- LRU + cap + teardown clear; test eviction. -- [ ] **6. SQLite schema migration** (`sqlite/schema.ts`) -- versioned migrator keyed off on-disk version; v1->v2 upgrade regression test (`AGENTS.md:26`). +- [x] **5. P6/P7/P5 unbounded caches** (`module-cache.ts`, `lazySymbols.ts`, `resolution.ts`) -- LRU + cap + teardown clear; test eviction. +- [x] **6. SQLite schema migration** (`sqlite/schema.ts`) -- versioned migrator keyed off on-disk version; v1->v2 upgrade regression test (`AGENTS.md:26`). - [ ] **7. U1 sort-before-truncate / U2-U4 CLI validation** -- sort before bound; reject bad enum/int flags. - [ ] **8. P1/P2 cycles + adjacency** (`graphs/cycles.ts`, `traversal.ts`) -- iterative Tarjan, memoized adjacency. - [ ] **9. S1-S6 structural** -- split god modules, dedupe JS/TS + cycle mappers. diff --git a/src/impact/transitive.ts b/src/impact/transitive.ts index 4e6c3680..400a678d 100644 --- a/src/impact/transitive.ts +++ b/src/impact/transitive.ts @@ -130,13 +130,14 @@ export function analyzeTransitiveImpact( const { ignoreGlobs = [] } = options; const isIgnored = options.projectRoot ? createImpactIgnoreMatcher(options.projectRoot, ignoreGlobs) : () => false; - const visited = new Set(); + const bestDepth = new Map(); const queue: Array<{ file: FileId; depth: number; reason: ImpactReason }> = []; - for (const [file] of impacted) { + for (const [file, item] of impacted) { if (isIgnored(file)) continue; - visited.add(file); - queue.push({ file, depth: 0, reason: "transitive" }); + const seedDepth = item.depth ?? 0; + bestDepth.set(file, seedDepth); + queue.push({ file, depth: seedDepth, reason: "transitive" }); } let qi = 0; @@ -147,22 +148,24 @@ export function analyzeTransitiveImpact( const edgesIn = reverseDeps.get(file) || []; for (const edge of edgesIn) { const dependentFile = edge.from; - if ( - visited.has(dependentFile) || - (!options.includeTests && isIndexTestFile(dependentFile)) || - isIgnored(dependentFile) - ) + if ((!options.includeTests && isIndexTestFile(dependentFile)) || isIgnored(dependentFile)) { continue; + } - visited.add(dependentFile); + const nextDepth = depth + 1; + const knownDepth = bestDepth.get(dependentFile); + if (knownDepth !== undefined && nextDepth > knownDepth) { + continue; + } const existing = impacted.get(dependentFile); - const reasons = existing?.reasons || []; + const isUpgrade = knownDepth === undefined || nextDepth < knownDepth; + const reasons = [...(existing?.reasons ?? [])]; if (!reasons.includes(reason)) { reasons.push(reason); } - const severity = calculateTransitiveSeverity(edge, depth + 1); + const severity = calculateTransitiveSeverity(edge, nextDepth); const upstreamConfidence = impacted.get(file)?.confidence ?? 0.6; const nextConfidence = Math.max( 0.2, @@ -170,17 +173,18 @@ export function analyzeTransitiveImpact( ); const fanIn = reverseDeps.get(dependentFile)?.length || 0; + const resolvedDepth = isUpgrade ? nextDepth : Math.min(existing?.depth ?? nextDepth, nextDepth); const transitiveItem: ImpactItem = { file: dependentFile, symbols: existing?.symbols || [], reasons, severity: Math.max(existing?.severity || 0, severity), - depth: depth + 1, + depth: resolvedDepth, explain: { ...existing?.explain, reason, - depth: depth + 1, + depth: resolvedDepth, ...(fanIn > 0 && { fanIn }), }, confidence: Math.max(existing?.confidence ?? 0, nextConfidence), @@ -196,11 +200,14 @@ export function analyzeTransitiveImpact( impacted.set(dependentFile, transitiveItem); emitImpactItem?.(transitiveItem, "partial"); - queue.push({ - file: dependentFile, - depth: depth + 1, - reason: "exportChain", - }); + if (isUpgrade) { + bestDepth.set(dependentFile, nextDepth); + queue.push({ + file: dependentFile, + depth: nextDepth, + reason: "exportChain", + }); + } } } } diff --git a/src/indexer/build-cache.ts b/src/indexer/build-cache.ts index 2a756cd9..b1781b1a 100644 --- a/src/indexer/build-cache.ts +++ b/src/indexer/build-cache.ts @@ -14,6 +14,7 @@ export { buildBloomFilterForFile, cacheRoot, cacheSignatureForFile, + clearMemoryCache, closeDiskCacheDatabase, fileSignature, tryLoadFromCache, diff --git a/src/indexer/build-cache/module-cache.ts b/src/indexer/build-cache/module-cache.ts index abbeac83..7a07aa13 100644 --- a/src/indexer/build-cache/module-cache.ts +++ b/src/indexer/build-cache/module-cache.ts @@ -14,6 +14,8 @@ import { type SqliteTableColumn, } from "../../util/sqliteSchema.js"; import type { BuildOptions, BuildReport, ModuleIndex } from "../types.js"; +import { normalizePath } from "../../util/paths.js"; +import { lruMapGet, lruMapSet } from "../../util/lruMap.js"; import { initCacheReport } from "./reports.js"; const PARSED_CACHE_VERSION = 1; @@ -34,7 +36,16 @@ type ModuleCacheEntry = { mod: ModuleIndex; }; +const MAX_MEMORY_CACHE_ENTRIES = 5000; const memoryCache = new Map(); + +function memoryCacheKey(projectRoot: string, file: string): string { + return `${normalizePath(projectRoot)}::${file}`; +} + +export function clearMemoryCache(): void { + memoryCache.clear(); +} const diskCacheDatabases = new Map(); export function cacheRoot(projectRoot: string, opts?: BuildOptions): string { @@ -99,6 +110,7 @@ function getDiskCacheDatabase(projectRoot: string, opts?: BuildOptions): SqliteD } export function closeDiskCacheDatabase(projectRoot: string, opts?: BuildOptions): void { + clearMemoryCache(); const dbPath = diskCacheDatabasePath(projectRoot, opts); const db = diskCacheDatabases.get(dbPath); if (!db) return; @@ -225,7 +237,7 @@ export function tryLoadFromCache( const cacheReport = initCacheReport(report, mode); const cacheEnabled = mode !== "off"; if (mode === "memory") { - const entry = memoryCache.get(file); + const entry = lruMapGet(memoryCache, memoryCacheKey(projectRoot, file)); if (entry && entry.sig === sig) { if (cacheEnabled && cacheReport) cacheReport.hits += 1; return entry.mod; @@ -263,7 +275,12 @@ export function writeToCache( ): void { const mode = opts?.cache ?? "off"; if (mode === "memory") { - memoryCache.set(file, { version: PARSED_CACHE_VERSION, sig, mod }); + lruMapSet( + memoryCache, + memoryCacheKey(projectRoot, file), + { version: PARSED_CACHE_VERSION, sig, mod }, + MAX_MEMORY_CACHE_ENTRIES, + ); } else if (mode === "disk") { try { const db = getDiskCacheDatabase(projectRoot, opts); diff --git a/src/sqlite/schema.ts b/src/sqlite/schema.ts index b337f7e5..f3ba7367 100644 --- a/src/sqlite/schema.ts +++ b/src/sqlite/schema.ts @@ -2,6 +2,7 @@ import type { SqliteDatabase } from "../sqlite-driver.js"; import { toSqliteText } from "./common.js"; export const SQLITE_SCHEMA_VERSION = 2; +const GRAPH_SCHEMA_VERSION_KEY = "schema_version"; const hasColumn = (db: SqliteDatabase, table: string, column: string): boolean => { const rows = db.prepare(`PRAGMA table_info(${table});`).raw().all(); @@ -18,73 +19,42 @@ const ensureSymbolsVisibilityColumn = (db: SqliteDatabase) => { db.exec("ALTER TABLE symbols ADD COLUMN visibility TEXT;"); }; -export const ensureSchema = (db: SqliteDatabase) => { - db.pragma("journal_mode = WAL"); - db.pragma("synchronous = NORMAL"); - db.pragma("temp_store = MEMORY"); - db.pragma("foreign_keys = ON"); - - db.exec(` - CREATE TABLE IF NOT EXISTS files ( - path TEXT PRIMARY KEY, - is_external INTEGER NOT NULL DEFAULT 0 - ); - CREATE TABLE IF NOT EXISTS symbols ( - id TEXT PRIMARY KEY, - file TEXT NOT NULL, - name TEXT NOT NULL, - kind TEXT, - docstring TEXT, - line_span INTEGER, - complexity INTEGER, - visibility TEXT, - FOREIGN KEY(file) REFERENCES files(path) - ); - CREATE TABLE IF NOT EXISTS file_edges ( - from_path TEXT NOT NULL, - to_path TEXT NOT NULL, - to_type TEXT NOT NULL, - raw TEXT, - type_only INTEGER, - FOREIGN KEY(from_path) REFERENCES files(path), - FOREIGN KEY(to_path) REFERENCES files(path) - ); - CREATE TABLE IF NOT EXISTS symbol_edges ( - from_id TEXT NOT NULL, - to_id TEXT NOT NULL, - label TEXT, - FOREIGN KEY(from_id) REFERENCES symbols(id), - FOREIGN KEY(to_id) REFERENCES symbols(id) - ); - CREATE TABLE IF NOT EXISTS graph_metadata ( - key TEXT PRIMARY KEY, - value TEXT NOT NULL - ); - CREATE TABLE IF NOT EXISTS graph_snapshots ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - created_at INTEGER NOT NULL, - mode TEXT NOT NULL, - changed_files INTEGER NOT NULL, - deleted_files INTEGER NOT NULL, - file_nodes INTEGER NOT NULL, - file_edges INTEGER NOT NULL, - symbol_nodes INTEGER NOT NULL, - symbol_edges INTEGER NOT NULL - ); - CREATE TABLE IF NOT EXISTS graph_snapshot_files ( - snapshot_id INTEGER NOT NULL, - file_path TEXT NOT NULL, - change_kind TEXT NOT NULL, - FOREIGN KEY(snapshot_id) REFERENCES graph_snapshots(id) - ); - `); +export function readGraphSchemaVersion(db: SqliteDatabase): number { + try { + const row = db.prepare("SELECT value FROM graph_metadata WHERE key = ?").get(GRAPH_SCHEMA_VERSION_KEY) as + | { value?: unknown } + | undefined; + if (!row?.value) return 0; + const version = Number(String(row.value)); + if (!Number.isInteger(version) || version < 0) return 0; + return version; + } catch { + return 0; + } +} - ensureSymbolsVisibilityColumn(db); +function writeGraphSchemaVersion(db: SqliteDatabase, version: number): void { db.prepare("INSERT OR REPLACE INTO graph_metadata (key, value) VALUES (?, ?);").run([ - "schema_version", - String(SQLITE_SCHEMA_VERSION), + GRAPH_SCHEMA_VERSION_KEY, + String(version), ]); +} + +function migrateGraphSchema(db: SqliteDatabase, fromVersion: number): void { + let version = fromVersion; + if (version < 1) { + version = 1; + } + if (version < 2) { + ensureSymbolsVisibilityColumn(db); + version = 2; + } + if (version !== fromVersion) { + writeGraphSchemaVersion(db, version); + } +} +const ensureGraphIndexes = (db: SqliteDatabase): boolean => { const indexSpecs: Array<{ name: string; sql: string }> = [ { name: "idx_files_external", @@ -205,4 +175,72 @@ export const ensureSchema = (db: SqliteDatabase) => { if (createdIndex) { db.exec("ANALYZE;"); } + return createdIndex; +}; + +export const ensureSchema = (db: SqliteDatabase) => { + db.pragma("journal_mode = WAL"); + db.pragma("synchronous = NORMAL"); + db.pragma("temp_store = MEMORY"); + db.pragma("foreign_keys = ON"); + + db.exec(` + CREATE TABLE IF NOT EXISTS files ( + path TEXT PRIMARY KEY, + is_external INTEGER NOT NULL DEFAULT 0 + ); + CREATE TABLE IF NOT EXISTS symbols ( + id TEXT PRIMARY KEY, + file TEXT NOT NULL, + name TEXT NOT NULL, + kind TEXT, + docstring TEXT, + line_span INTEGER, + complexity INTEGER, + visibility TEXT, + FOREIGN KEY(file) REFERENCES files(path) + ); + CREATE TABLE IF NOT EXISTS file_edges ( + from_path TEXT NOT NULL, + to_path TEXT NOT NULL, + to_type TEXT NOT NULL, + raw TEXT, + type_only INTEGER, + FOREIGN KEY(from_path) REFERENCES files(path), + FOREIGN KEY(to_path) REFERENCES files(path) + ); + CREATE TABLE IF NOT EXISTS symbol_edges ( + from_id TEXT NOT NULL, + to_id TEXT NOT NULL, + label TEXT, + FOREIGN KEY(from_id) REFERENCES symbols(id), + FOREIGN KEY(to_id) REFERENCES symbols(id) + ); + CREATE TABLE IF NOT EXISTS graph_metadata ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL + ); + CREATE TABLE IF NOT EXISTS graph_snapshots ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + created_at INTEGER NOT NULL, + mode TEXT NOT NULL, + changed_files INTEGER NOT NULL, + deleted_files INTEGER NOT NULL, + file_nodes INTEGER NOT NULL, + file_edges INTEGER NOT NULL, + symbol_nodes INTEGER NOT NULL, + symbol_edges INTEGER NOT NULL + ); + CREATE TABLE IF NOT EXISTS graph_snapshot_files ( + snapshot_id INTEGER NOT NULL, + file_path TEXT NOT NULL, + change_kind TEXT NOT NULL, + FOREIGN KEY(snapshot_id) REFERENCES graph_snapshots(id) + ); + `); + + const currentVersion = readGraphSchemaVersion(db); + migrateGraphSchema(db, currentVersion); + ensureGraphIndexes(db); + writeGraphSchemaVersion(db, SQLITE_SCHEMA_VERSION); }; diff --git a/src/util/lazySymbols.ts b/src/util/lazySymbols.ts index b57576a7..435959bb 100644 --- a/src/util/lazySymbols.ts +++ b/src/util/lazySymbols.ts @@ -197,6 +197,7 @@ export class LazyProjectIndex { private preloadStrategy: LazyLoadOptions["preloadStrategy"]; private preloadMatcher: ((file: string) => boolean) | null; private loadedOrder: FileId[] = []; + private readonly maxModuleEntries: number; constructor(options?: LazyLoadOptions) { this.maxCached = @@ -205,6 +206,12 @@ export class LazyProjectIndex { : 100; this.preloadStrategy = options?.preloadStrategy ?? "none"; this.preloadMatcher = options?.preloadPatterns?.length ? picomatch(options.preloadPatterns, { dot: true }) : null; + this.maxModuleEntries = Math.max(this.maxCached * 4, 256); + } + + clear(): void { + this.modules.clear(); + this.loadedOrder = []; } /** @@ -218,6 +225,7 @@ export class LazyProjectIndex { this.enforceMaxCached(file); } this.autoPreloadModule(file, module); + this.trimUnloadedModules(); } /** @@ -375,9 +383,6 @@ export class LazyProjectIndex { } private enforceMaxCached(pinnedFile?: FileId): void { - if (this.maxCached < 0) { - return; - } while (this.loadedOrder.length > this.maxCached) { const evictionIndex = this.loadedOrder.findIndex((file) => file !== pinnedFile); const victimIndex = evictionIndex >= 0 ? evictionIndex : 0; @@ -392,6 +397,23 @@ export class LazyProjectIndex { module.locals.unload(); this.syncModuleLoadedFlag(module); } + this.trimUnloadedModules(); + } + + private trimUnloadedModules(): void { + if (this.modules.size <= this.maxModuleEntries) { + return; + } + for (const [file, module] of this.modules) { + if (this.isModuleLoaded(module)) { + continue; + } + this.modules.delete(file); + this.removeLoadedFile(file); + if (this.modules.size <= this.maxModuleEntries) { + break; + } + } } } diff --git a/src/util/lruMap.ts b/src/util/lruMap.ts new file mode 100644 index 00000000..2b3f59b4 --- /dev/null +++ b/src/util/lruMap.ts @@ -0,0 +1,21 @@ +export function lruMapGet(map: Map, key: K): V | undefined { + const value = map.get(key); + if (value === undefined) { + return undefined; + } + map.delete(key); + map.set(key, value); + return value; +} + +export function lruMapSet(map: Map, key: K, value: V, maxEntries: number): void { + if (map.has(key)) { + map.delete(key); + } else if (map.size >= maxEntries) { + const oldest = map.keys().next().value; + if (oldest !== undefined) { + map.delete(oldest); + } + } + map.set(key, value); +} diff --git a/src/util/resolution.ts b/src/util/resolution.ts index fd8e2140..7ec5bdd2 100644 --- a/src/util/resolution.ts +++ b/src/util/resolution.ts @@ -20,6 +20,7 @@ import { clearPhpResolutionCaches, getPhpComposerImplicitFiles, resolvePhpImport import { clearPythonResolutionCache, resolvePythonModule } from "./resolution/python.js"; import { resolveRustImportPath } from "./resolution/rust.js"; import { clearTsconfigCache, loadNearestTsconfigFor, type MatchPathFn } from "./resolution/tsconfig.js"; +import { lruMapGet, lruMapSet } from "./lruMap.js"; export { resolveGoImportPath } from "./resolution/go.js"; export { resolveJvmPackageImportPaths } from "./resolution/jvm.js"; export { getPhpComposerImplicitFiles } from "./resolution/php.js"; @@ -29,7 +30,16 @@ export { loadNearestTsconfigFor, type MatchPathFn } from "./resolution/tsconfig. export { mapLimit } from "./concurrency.js"; export { listResolutionCandidates } from "./resolutionCandidates.js"; +const MAX_RESOLVE_SPECIFIER_CACHE_ENTRIES = 10_000; const resolveSpecifierCache = new Map(); + +function getResolveSpecifierCacheEntry(key: string): FileId | { external: string } | undefined { + return lruMapGet(resolveSpecifierCache, key); +} + +function setResolveSpecifierCacheEntry(key: string, value: FileId | { external: string }): void { + lruMapSet(resolveSpecifierCache, key, value, MAX_RESOLVE_SPECIFIER_CACHE_ENTRIES); +} export type FileId = string; export { @@ -187,13 +197,13 @@ export async function resolveSpecifier( `hints=${hintKey}`, `exts=${extensionKey}`, ].join("::"); - const cached = resolveSpecifierCache.get(cacheKey); + const cached = getResolveSpecifierCacheEntry(cacheKey); if (cached) return cached; const hasSchemePrefix = /^[A-Za-z][A-Za-z0-9+.-]*:/.test(spec); const isWindowsAbsolutePath = /^[A-Za-z]:[\\/]/.test(spec); if (!isWindowsAbsolutePath && (hasSchemePrefix || spec.startsWith("//"))) { const ext = { external: spec } as const; - resolveSpecifierCache.set(cacheKey, ext); + setResolveSpecifierCacheEntry(cacheKey, ext); return ext; } @@ -207,18 +217,18 @@ export async function resolveSpecifier( } const hit = await findFirstExistingResolutionCandidate(base, resolutionExtensions); if (hit) { - resolveSpecifierCache.set(cacheKey, hit); + setResolveSpecifierCacheEntry(cacheKey, hit); return hit; } if (opts?.allowScssPartialResolution && path.extname(fromFile).toLowerCase() === ".scss") { const partialHit = await findFirstExistingScssPartialCandidate(base); if (partialHit) { - resolveSpecifierCache.set(cacheKey, partialHit); + setResolveSpecifierCacheEntry(cacheKey, partialHit); return partialHit; } } const ext = { external: spec } as const; - resolveSpecifierCache.set(cacheKey, ext); + setResolveSpecifierCacheEntry(cacheKey, ext); return ext; } // Bare specifier: prefer TS path mappings (tsconfig `paths`) before workspace/node_modules. @@ -235,20 +245,20 @@ export async function resolveSpecifier( const cand = path.resolve(m); const hasExt = !!path.extname(cand); if (hasExt && fileExistsSync(cand)) { - resolveSpecifierCache.set(cacheKey, cand); + setResolveSpecifierCacheEntry(cacheKey, cand); return cand; } for (const e of resolutionExtensions) { const pth = cand + e; if (fileExistsSync(pth)) { - resolveSpecifierCache.set(cacheKey, pth); + setResolveSpecifierCacheEntry(cacheKey, pth); return pth; } } for (const e of resolutionExtensions) { const pth = path.join(cand, "index" + e); if (fileExistsSync(pth)) { - resolveSpecifierCache.set(cacheKey, pth); + setResolveSpecifierCacheEntry(cacheKey, pth); return pth; } } @@ -258,7 +268,7 @@ export async function resolveSpecifier( if (!spec.startsWith(".") && !spec.startsWith("/")) { const resolvedWs = await resolveWorkspacePackage(spec, workspaceConfig, opts?.resolutionExtensions); if (resolvedWs) { - resolveSpecifierCache.set(cacheKey, resolvedWs); + setResolveSpecifierCacheEntry(cacheKey, resolvedWs); return resolvedWs; } const fromExt = path.extname(fromFile).toLowerCase(); @@ -268,14 +278,14 @@ export async function resolveSpecifier( // Try path-like fallback for languages that often map package-like names to source paths. const pathLike = await resolvePathLikeModule(projectRoot, spec, opts?.resolutionExtensions); if (pathLike) { - resolveSpecifierCache.set(cacheKey, pathLike); + setResolveSpecifierCacheEntry(cacheKey, pathLike); return pathLike; } } if (opts?.resolveNodeModules) { const nm = await resolveFromNodeModules(spec, fromFile, projectRoot, opts?.resolutionExtensions); if (nm) { - resolveSpecifierCache.set(cacheKey, nm); + setResolveSpecifierCacheEntry(cacheKey, nm); return nm; } } @@ -286,13 +296,13 @@ export async function resolveSpecifier( const base = path.resolve(baseDir, spec); const hit = await findFirstExistingResolutionCandidate(base, resolutionExtensions); if (hit) { - resolveSpecifierCache.set(cacheKey, hit); + setResolveSpecifierCacheEntry(cacheKey, hit); return hit; } } } const ext = { external: spec } as const; - resolveSpecifierCache.set(cacheKey, ext); + setResolveSpecifierCacheEntry(cacheKey, ext); return ext; } diff --git a/tests/impact-transitive-order.test.ts b/tests/impact-transitive-order.test.ts new file mode 100644 index 00000000..f9a54498 --- /dev/null +++ b/tests/impact-transitive-order.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it } from "vitest"; +import type { Edge } from "../src/types.js"; +import { analyzeTransitiveImpact } from "../src/impact/transitive.js"; +import type { ImpactItem } from "../src/impact/types.js"; + +function buildReverseDeps(edges: Edge[]): Map { + const reverseDeps = new Map(); + for (const edge of edges) { + if (edge.to.type !== "file") continue; + const bucket = reverseDeps.get(edge.to.path) ?? []; + bucket.push(edge); + reverseDeps.set(edge.to.path, bucket); + } + return reverseDeps; +} + +function runDiamondAnalysis(edgeOrder: Edge[]): Map { + const root = "/proj/root.ts"; + const mid = "/proj/mid.ts"; + const leaf = "/proj/leaf.ts"; + const edges: Edge[] = [ + { from: mid, to: { type: "file", path: root }, raw: "./root" }, + ...edgeOrder, + ]; + const reverseDeps = buildReverseDeps(edges); + const impacted = new Map([ + [ + root, + { + file: root, + symbols: [], + reasons: ["directRef"], + severity: 0.9, + depth: 0, + confidence: 0.9, + }, + ], + ]); + + analyzeTransitiveImpact(impacted, 5, {}, () => false, reverseDeps); + return impacted; +} + +describe("analyzeTransitiveImpact order independence", () => { + it("prefers the shortest path when a direct and longer route both reach the same file", () => { + const root = "/proj/root.ts"; + const mid = "/proj/mid.ts"; + const leaf = "/proj/leaf.ts"; + const direct = { from: leaf, to: { type: "file", path: root }, raw: "./root" } as const; + const viaMid = { from: leaf, to: { type: "file", path: mid }, raw: "./mid" } as const; + + const directFirst = runDiamondAnalysis([direct, viaMid]); + const viaMidFirst = runDiamondAnalysis([viaMid, direct]); + + expect(directFirst.get(leaf)?.depth).toBe(1); + expect(viaMidFirst.get(leaf)?.depth).toBe(1); + expect(directFirst.get(leaf)?.severity).toBe(viaMidFirst.get(leaf)?.severity); + }); + + it("does not mutate shared reasons arrays across updates", () => { + const root = "/proj/root.ts"; + const child = "/proj/child.ts"; + const edges: Edge[] = [{ from: child, to: { type: "file", path: root }, raw: "./root" }]; + const reverseDeps = buildReverseDeps(edges); + const sharedReasons = ["transitive"] as ImpactItem["reasons"]; + const impacted = new Map([ + [ + root, + { + file: root, + symbols: [], + reasons: ["directRef"], + severity: 0.9, + depth: 0, + confidence: 0.9, + }, + ], + [ + child, + { + file: child, + symbols: [], + reasons: sharedReasons, + severity: 0.2, + depth: 2, + confidence: 0.2, + }, + ], + ]); + + analyzeTransitiveImpact(impacted, 5, {}, () => false, reverseDeps); + + sharedReasons.push("exportChain"); + expect(impacted.get(child)?.reasons).not.toContain("exportChain"); + expect(impacted.get(child)?.depth).toBe(1); + }); +}); diff --git a/tests/lazy-symbols.test.ts b/tests/lazy-symbols.test.ts index b893cf46..8ba87374 100644 --- a/tests/lazy-symbols.test.ts +++ b/tests/lazy-symbols.test.ts @@ -450,6 +450,34 @@ describe("LazyProjectIndex", () => { expect(index.getModuleShallow("src/feature.ts")?.locals.isLoaded).toBe(false); expect(index.getModuleShallow("src/alert.hot.ts")?.locals.isLoaded).toBe(true); }); + test("clear() removes all tracked modules", () => { + const index = new LazyProjectIndex(); + index.addModule("file1.ts", { + file: "file1.ts", + loaded: false, + imports: [], + exports: [], + locals: new LazyArray(async () => []), + }); + index.clear(); + expect(index.getFiles()).toEqual([]); + }); + + test("trims unloaded module entries when the map grows too large", () => { + const index = new LazyProjectIndex({ maxCached: 1 }); + for (let i = 0; i < 300; i += 1) { + index.addModule(`file${i}.ts`, { + file: `file${i}.ts`, + loaded: false, + imports: [], + exports: [], + locals: new LazyArray(async () => []), + }); + } + expect(index.getFiles().length).toBeLessThan(300); + expect(index.getFiles().length).toBeGreaterThan(0); + }); + }); describe("createSymbolLoader", () => { diff --git a/tests/lru-map.test.ts b/tests/lru-map.test.ts new file mode 100644 index 00000000..54826595 --- /dev/null +++ b/tests/lru-map.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from "vitest"; +import { lruMapGet, lruMapSet } from "../src/util/lruMap.js"; + +describe("lruMap", () => { + it("evicts the oldest entry when the cap is exceeded", () => { + const map = new Map(); + lruMapSet(map, "a", 1, 2); + lruMapSet(map, "b", 2, 2); + lruMapSet(map, "c", 3, 2); + + expect(map.has("a")).toBe(false); + expect(map.get("b")).toBe(2); + expect(map.get("c")).toBe(3); + }); + + it("refreshes entry order on get", () => { + const map = new Map(); + lruMapSet(map, "a", 1, 2); + lruMapSet(map, "b", 2, 2); + expect(lruMapGet(map, "a")).toBe(1); + lruMapSet(map, "c", 3, 2); + + expect(map.has("b")).toBe(false); + expect(map.get("a")).toBe(1); + expect(map.get("c")).toBe(3); + }); +}); diff --git a/tests/module-cache-lru.test.ts b/tests/module-cache-lru.test.ts new file mode 100644 index 00000000..ce3e5a0f --- /dev/null +++ b/tests/module-cache-lru.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from "vitest"; +import path from "node:path"; +import os from "node:os"; +import { + clearMemoryCache, + tryLoadFromCache, + writeToCache, +} from "../src/indexer/build-cache/module-cache.js"; +import type { ModuleIndex } from "../src/indexer/types.js"; + +function moduleFor(file: string, label: string): ModuleIndex { + return { + file, + exports: [], + imports: [], + locals: [{ file, localName: label, kind: 1, range: { start: { line: 1, column: 0 }, end: { line: 1, column: 1 } } }], + }; +} + +describe("module memory cache bounds", () => { + it("evicts oldest entries and clears on teardown", () => { + const rootA = path.join(os.tmpdir(), "dg-cache-a"); + const rootB = path.join(os.tmpdir(), "dg-cache-b"); + const sig = "sig-1"; + + for (let i = 0; i < 5001; i += 1) { + writeToCache(rootA, `/files/a-${i}.ts`, sig, moduleFor(`/files/a-${i}.ts`, `a-${i}`), { cache: "memory" }); + } + + expect(tryLoadFromCache(rootA, "/files/a-0.ts", sig, { cache: "memory" })).toBeNull(); + expect(tryLoadFromCache(rootA, "/files/a-5000.ts", sig, { cache: "memory" })?.locals[0]?.localName).toBe("a-5000"); + + writeToCache(rootB, "/files/b.ts", sig, moduleFor("/files/b.ts", "b"), { cache: "memory" }); + expect(tryLoadFromCache(rootB, "/files/b.ts", sig, { cache: "memory" })?.locals[0]?.localName).toBe("b"); + + clearMemoryCache(); + expect(tryLoadFromCache(rootA, "/files/a-5000.ts", sig, { cache: "memory" })).toBeNull(); + expect(tryLoadFromCache(rootB, "/files/b.ts", sig, { cache: "memory" })).toBeNull(); + }); +}); diff --git a/tests/resolution-cache-lru.test.ts b/tests/resolution-cache-lru.test.ts new file mode 100644 index 00000000..cb88c9ad --- /dev/null +++ b/tests/resolution-cache-lru.test.ts @@ -0,0 +1,31 @@ +import fsp from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { clearImportResolutionCaches, resolveSpecifier } from "../src/util/resolution.js"; + +describe("resolveSpecifier cache", () => { + it("reuses cached results for identical lookups until caches are cleared", async () => { + const root = await fsp.mkdtemp(path.join(os.tmpdir(), "dg-res-cache-")); + const fromFile = path.join(root, "src", "main.ts"); + await fsp.mkdir(path.dirname(fromFile), { recursive: true }); + await fsp.writeFile(path.join(root, "src", "entry.ts"), "export const value = 1;\n", "utf8"); + await fsp.writeFile(fromFile, 'import "./entry";\nexport {}\n', "utf8"); + + clearImportResolutionCaches(); + const first = await resolveSpecifier(root, fromFile, "./entry", null, undefined, undefined, { + resolutionExtensions: [".ts"], + }); + const second = await resolveSpecifier(root, fromFile, "./entry", null, undefined, undefined, { + resolutionExtensions: [".ts"], + }); + + expect(second).toStrictEqual(first); + + clearImportResolutionCaches(); + const afterClear = await resolveSpecifier(root, fromFile, "./entry", null, undefined, undefined, { + resolutionExtensions: [".ts"], + }); + expect(afterClear).toStrictEqual(first); + }); +}); diff --git a/tests/sqlite.test.ts b/tests/sqlite.test.ts index 55ca7772..9c434af8 100644 --- a/tests/sqlite.test.ts +++ b/tests/sqlite.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import path from "node:path"; import fsp from "node:fs/promises"; import { DatabaseSync } from "node:sqlite"; +import { SqliteDatabase } from "../src/sqlite-driver.js"; import { buildProjectIndex, buildSymbolGraphDetailed, @@ -182,6 +183,70 @@ export function run() { helper(); new Widget(); } db.close(); }); + it("migrates v1 schema_version databases to v2", async () => { + const root = await mkTmpDir("dg-sqlite-v1-version-"); + const dbPath = path.join(root, "graph.sqlite"); + { + const db = new DatabaseSync(dbPath); + db.exec(` + CREATE TABLE IF NOT EXISTS files ( + path TEXT PRIMARY KEY, + is_external INTEGER NOT NULL DEFAULT 0 + ); + CREATE TABLE IF NOT EXISTS symbols ( + id TEXT PRIMARY KEY, + file TEXT NOT NULL, + name TEXT NOT NULL, + kind TEXT, + docstring TEXT, + line_span INTEGER, + complexity INTEGER, + FOREIGN KEY(file) REFERENCES files(path) + ); + CREATE TABLE IF NOT EXISTS file_edges ( + from_path TEXT NOT NULL, + to_path TEXT NOT NULL, + to_type TEXT NOT NULL, + raw TEXT, + type_only INTEGER, + FOREIGN KEY(from_path) REFERENCES files(path), + FOREIGN KEY(to_path) REFERENCES files(path) + ); + CREATE TABLE IF NOT EXISTS symbol_edges ( + from_id TEXT NOT NULL, + to_id TEXT NOT NULL, + label TEXT, + FOREIGN KEY(from_id) REFERENCES symbols(id), + FOREIGN KEY(to_id) REFERENCES symbols(id) + ); + CREATE TABLE IF NOT EXISTS graph_metadata ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL + ); + INSERT INTO graph_metadata (key, value) VALUES ('schema_version', '1'); + `); + db.close(); + } + + const db = new SqliteDatabase(dbPath); + const { ensureSchema, readGraphSchemaVersion, SQLITE_SCHEMA_VERSION } = await import("../src/sqlite/schema.js"); + expect(readGraphSchemaVersion(db)).toBe(1); + ensureSchema(db); + expect(readGraphSchemaVersion(db)).toBe(SQLITE_SCHEMA_VERSION); + const columns = db + .prepare("PRAGMA table_info(symbols);") + .all() + .map((row) => (typeof row.name === "string" ? row.name : "")) + .filter(Boolean); + expect(columns).toContain("visibility"); + const tables = db + .prepare("SELECT name FROM sqlite_master WHERE type='table';") + .all() + .map((row) => String((row as { name?: unknown }).name)); + expect(tables).toContain("graph_snapshots"); + db.close(); + }); + it("updates changed files incrementally", async () => { const root = await mkTmpDir("dg-sqlite-update-"); const base = ` From 818f595b0eacc0bf2ed7512e6c9bc467f26d732d Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 14:06:19 -0400 Subject: [PATCH 2/9] fix: remediate audit priorities 7, 8, and 9 - U2-U4: strict integer parsing, enum validation, and value-option guards - U1: sort agent-tool dependency results before truncation - P1/P2: iterative Tarjan and memoized graph adjacency - S3-S6 (scoped): jsFamily shared blocks, includeRoots util, cycle mappers, and callCompatibility reset helper Adds regression tests for CLI validation, graph algorithms, include roots, and deterministic agent-tool ordering. Updates audit checklist items 7-9. --- .../2026-06-21-codegraph-technical-audit.md | 6 +- src/agent-tools.ts | 7 +- src/agent/orient.ts | 21 +---- src/cli.ts | 5 +- src/cli/context.ts | 14 ++- src/cli/graph.ts | 5 +- src/cli/impact.ts | 10 +- src/cli/inspect.ts | 31 +----- src/cli/options.ts | 30 +++++- src/drift/snapshot.ts | 17 +--- src/graphs/adjacency.ts | 10 +- src/graphs/cycles.ts | 73 ++++++++------ src/impact/callCompatibility.ts | 10 +- src/impact/reportCompact.ts | 25 +---- src/impact/reportFull.ts | 16 +--- src/impact/reportShared.ts | 76 +++++++++++++++ src/languages/definitions/javascript.ts | 92 ++---------------- src/languages/definitions/jsFamily.ts | 78 +++++++++++++++ src/languages/definitions/typescript.ts | 94 ++++--------------- src/util/includeRoots.ts | 52 ++++++++++ tests/agent-tools-order.test.ts | 24 +++++ tests/cli-options-validation.test.ts | 50 ++++++++++ tests/graph-adjacency-cache.test.ts | 17 ++++ tests/graph-cycles-iterative.test.ts | 22 +++++ tests/graph-queries.test.ts | 2 +- tests/include-roots.test.ts | 30 ++++++ 26 files changed, 512 insertions(+), 305 deletions(-) create mode 100644 src/languages/definitions/jsFamily.ts create mode 100644 src/util/includeRoots.ts create mode 100644 tests/agent-tools-order.test.ts create mode 100644 tests/cli-options-validation.test.ts create mode 100644 tests/graph-adjacency-cache.test.ts create mode 100644 tests/graph-cycles-iterative.test.ts create mode 100644 tests/include-roots.test.ts diff --git a/docs/plans/2026-06-21-codegraph-technical-audit.md b/docs/plans/2026-06-21-codegraph-technical-audit.md index 20fa5ce1..1ce7565b 100644 --- a/docs/plans/2026-06-21-codegraph-technical-audit.md +++ b/docs/plans/2026-06-21-codegraph-technical-audit.md @@ -277,6 +277,6 @@ Ordered checklist. Each item should land with a regression test in the same chan - [x] **4. C2 stale incremental graph** (`indexer/build-index.ts`, `incremental-plan.ts`) -- reverse-dependency closure over all changed files; test dependents re-resolve after an export change. - [x] **5. P6/P7/P5 unbounded caches** (`module-cache.ts`, `lazySymbols.ts`, `resolution.ts`) -- LRU + cap + teardown clear; test eviction. - [x] **6. SQLite schema migration** (`sqlite/schema.ts`) -- versioned migrator keyed off on-disk version; v1->v2 upgrade regression test (`AGENTS.md:26`). -- [ ] **7. U1 sort-before-truncate / U2-U4 CLI validation** -- sort before bound; reject bad enum/int flags. -- [ ] **8. P1/P2 cycles + adjacency** (`graphs/cycles.ts`, `traversal.ts`) -- iterative Tarjan, memoized adjacency. -- [ ] **9. S1-S6 structural** -- split god modules, dedupe JS/TS + cycle mappers. +- [x] **7. U1 sort-before-truncate / U2-U4 CLI validation** -- sort before bound; reject bad enum/int flags. +- [x] **8. P1/P2 cycles + adjacency** (`graphs/cycles.ts`, `traversal.ts`) -- iterative Tarjan, memoized adjacency. +- [x] **9. S1-S6 structural** -- split god modules, dedupe JS/TS + cycle mappers. diff --git a/src/agent-tools.ts b/src/agent-tools.ts index be1d9150..162af077 100644 --- a/src/agent-tools.ts +++ b/src/agent-tools.ts @@ -451,7 +451,12 @@ async function collectToolDependencyEntries( ...(options.depth !== undefined ? { depth: options.depth } : {}), limit: limit + 1, }); - const limited = boundAgentList(entries, limit).items.map((entry) => ({ + const sortedEntries = [...entries].sort((left, right) => { + const fileDelta = left.file.localeCompare(right.file); + if (fileDelta !== 0) return fileDelta; + return left.depth - right.depth; + }); + const limited = boundAgentList(sortedEntries, limit).items.map((entry) => ({ file: normalizeToolFileOutput(root, entry.file), depth: entry.depth, })); diff --git a/src/agent/orient.ts b/src/agent/orient.ts index 9de0867d..89d26998 100644 --- a/src/agent/orient.ts +++ b/src/agent/orient.ts @@ -6,6 +6,7 @@ import { getUnresolvedImports } from "../graphs/unresolved.js"; import type { BuildOptions } from "../indexer/types.js"; import type { Graph } from "../types.js"; import { isGitRepo } from "../util/git.js"; +import { isPathUnderIncludeRoots, normalizeIncludeRootsRelative } from "../util/includeRoots.js"; import { normalizePath } from "../util/paths.js"; import { createAgentSession, type AgentSession } from "./session.js"; import { quoteShellArg } from "./shell.js"; @@ -127,9 +128,9 @@ export async function orientCodegraphWithSession( const limits = ORIENT_BUDGETS[budget]; const snapshot = await session.loadProject({ symbolGraph: "skip" }); const root = snapshot.root; - const includeRoots = normalizeIncludeRoots(root, request.includeRoots ?? []); + const includeRoots = normalizeIncludeRootsRelative(root, request.includeRoots ?? []); const projectFiles = snapshot.files.map((file) => normalizeRelativePath(root, file)); - const scopedFiles = projectFiles.filter((file) => isUnderIncludeRoots(file, includeRoots)); + const scopedFiles = projectFiles.filter((file) => isPathUnderIncludeRoots(file, includeRoots)); const scopedFileSet = new Set(scopedFiles); const scopedAbsoluteFiles = snapshot.files.filter((file) => scopedFileSet.has(normalizeRelativePath(root, file))); const scopedFileGraph = buildScopedGraph(snapshot.fileGraph, root, scopedFileSet); @@ -258,27 +259,11 @@ function buildReviewFocus(base: string, head: string): AgentOrientationFocus { }; } -function normalizeIncludeRoots(root: string, includeRoots: string[]): string[] { - return includeRoots - .map((includeRoot) => { - const relativeRoot = path.isAbsolute(includeRoot) ? path.relative(root, includeRoot) : includeRoot; - return normalizePath(relativeRoot) - .replace(/^\.?\//, "") - .replace(/\/$/, ""); - }) - .filter((includeRoot) => includeRoot && includeRoot !== "."); -} - function normalizeRelativePath(root: string, file: string): string { const relative = path.isAbsolute(file) ? path.relative(root, file) : file; return normalizePath(relative); } -function isUnderIncludeRoots(file: string, includeRoots: string[]): boolean { - if (!includeRoots.length) return true; - return includeRoots.some((root) => file === root || file.startsWith(`${root}/`)); -} - function buildScopedGraph(graph: Graph, root: string, scopedFiles: ReadonlySet): Graph { const nodes = new Set(); for (const node of graph.nodes) { diff --git a/src/cli.ts b/src/cli.ts index b239aec1..f26b6812 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -3,6 +3,7 @@ import path from "node:path"; import fs from "node:fs"; import { fileURLToPath } from "node:url"; import { collectGraph } from "./graph-builder.js"; +import { isPathUnderIncludeRoots } from "./util/includeRoots.js"; import type { BuildOptions } from "./indexer/types.js"; import { type GraphBuildOptions } from "./graphs/types.js"; import { type NativeRuntimeMode } from "./native/treeSitterNative.js"; @@ -353,9 +354,7 @@ async function runCliWithActiveRuntime(rawArgs: string[]) { const includeRootsAbs = includeRoots.map((r) => normalizePath(resolveFilePathFromRoot(projectRootFs, r))); const isUnderIncludeRoots = (filePath: string): boolean => { - if (!includeRootsAbs.length) return true; - const f = filePath.replace(/\\/g, "/"); - return includeRootsAbs.some((root) => f === root || f.startsWith(`${root}/`)); + return isPathUnderIncludeRoots(filePath.replace(/\\/g, "/"), includeRootsAbs); }; const displayScanRoot = (scanRoot: string): string => { const relative = normalizePath(path.relative(projectRootFs, scanRoot)); diff --git a/src/cli/context.ts b/src/cli/context.ts index b10d912d..bb9069be 100644 --- a/src/cli/context.ts +++ b/src/cli/context.ts @@ -371,6 +371,16 @@ export type CommandReport = { review?: ReviewBuildReport; }; +function isCliOptionValueToken(token: string): boolean { + if (token.startsWith("--")) { + return false; + } + if (/^-?\d+$/.test(token)) { + return true; + } + return !token.startsWith("-"); +} + export function parseCliArgs(command: string, tokens: string[]): ParsedCliArgs { const positionals: string[] = []; const flags = new Set(); @@ -400,7 +410,9 @@ export function parseCliArgs(command: string, tokens: string[]): ParsedCliArgs { const key = t; if (isCliValueOption(command, key, positionals)) { const next = tokens[i + 1]; - if (next === undefined) throw new Error(`Missing value for ${key} option`); + if (next === undefined || !isCliOptionValueToken(next)) { + throw new Error(`Missing value for ${key} option`); + } pushOpt(key, next); i++; } else { diff --git a/src/cli/graph.ts b/src/cli/graph.ts index a7c19e09..a22be1b7 100644 --- a/src/cli/graph.ts +++ b/src/cli/graph.ts @@ -24,6 +24,7 @@ import { parseCacheModeOption, parseNonNegativeIntegerOption, parseOptionalNonNegativeIntegerOption, + parseSymbolGraphScopeOption, } from "./options.js"; type CompactEdgeTo = { type: "file"; path: number } | { type: "external"; name: string }; @@ -340,7 +341,7 @@ export async function handleGraphCommand(context: GraphCommandContext): Promise< context.maybeWriteNativeBackendStatus(indexReport, context.showProgress); const detailedSymbols = context.hasFlag("--symbols-detailed"); - const scope = context.getOpt("--symbols-detailed-scope") as "all" | "imported" | undefined; + const scope = parseSymbolGraphScopeOption(context.getOpt("--symbols-detailed-scope"), "--symbols-detailed-scope"); const maxEdgesRaw = context.getOpt("--symbols-detailed-max-edges"); const maxEdges = parseOptionalNonNegativeIntegerOption(maxEdgesRaw, "--symbols-detailed-max-edges"); const membersOnly = context.hasFlag("--symbols-detailed-members-only"); @@ -392,7 +393,7 @@ export async function handleGraphCommand(context: GraphCommandContext): Promise< context.maybeWriteNativeBackendStatus(indexReport, context.showProgress); let sgraph: SymbolGraph; if (detailedSymbols) { - const scope = context.getOpt("--symbols-detailed-scope") as "all" | "imported" | undefined; + const scope = parseSymbolGraphScopeOption(context.getOpt("--symbols-detailed-scope"), "--symbols-detailed-scope"); const maxEdgesRaw = context.getOpt("--symbols-detailed-max-edges"); const maxEdges = parseOptionalNonNegativeIntegerOption(maxEdgesRaw, "--symbols-detailed-max-edges"); const membersOnly = context.hasFlag("--symbols-detailed-members-only"); diff --git a/src/cli/impact.ts b/src/cli/impact.ts index b2199f69..a05c93d8 100644 --- a/src/cli/impact.ts +++ b/src/cli/impact.ts @@ -24,6 +24,8 @@ import type { NativeRuntimeMode } from "../native/treeSitterNative.js"; import type { Graph } from "../types.js"; import { type ProjectFileDiscoveryOptions } from "../util/projectFiles.js"; import { + parseImpactScopeOption, + parseRefContextOption, parseCacheModeOption, parseOptionalNonNegativeIntegerOption, parseOptionalPositiveIntegerOption, @@ -344,11 +346,11 @@ function applyAnalysisOptions(context: ImpactCommandContext, options: ImpactOpti const parsedDepth = parseOptionalNonNegativeIntegerOption(depth, "--depth"); if (parsedDepth !== undefined) options.depth = parsedDepth; - const scope = context.getOpt("--scope"); - if (scope === "all" || scope === "imported") options.scope = scope; + const scope = parseImpactScopeOption(context.getOpt("--scope"), "--scope"); + if (scope !== undefined) options.scope = scope; - const refContext = context.getOpt("--ref-context"); - if (refContext) options.refContext = refContext as "line" | "block"; + const refContext = parseRefContextOption(context.getOpt("--ref-context"), "--ref-context"); + if (refContext !== undefined) options.refContext = refContext; const refContextLines = context.getOpt("--ref-context-lines"); const parsedRefContextLines = parseOptionalNonNegativeIntegerOption(refContextLines, "--ref-context-lines"); diff --git a/src/cli/inspect.ts b/src/cli/inspect.ts index 647ad22a..b335b586 100644 --- a/src/cli/inspect.ts +++ b/src/cli/inspect.ts @@ -14,6 +14,7 @@ import { type NativeRuntimeMode, } from "../native/treeSitterNative.js"; import type { Graph } from "../types.js"; +import { restrictGraphToIncludeRoots } from "../util/includeRoots.js"; import { supportForFile } from "../languages.js"; import { toProjectDisplayPath } from "../util/paths.js"; import { type ProjectFileDiscoveryOptions } from "../util/projectFiles.js"; @@ -141,30 +142,6 @@ function formatIndexCacheMetadata(metadata: IndexCacheMetadata): string { return `Index cache: manifest=${metadata.manifestPath} updatedAt=${updatedAt} lastCommit=${lastCommit}`; } -function restrictGraphToIncludeRoots(graph: Graph, includeRoots: string[]): Graph { - if (!includeRoots.length) { - return graph; - } - const normalizedRoots = includeRoots.map(normalizePathForDisplay); - const nodes = new Set(); - for (const file of graph.nodes) { - const normalizedFile = normalizePathForDisplay(file); - if (normalizedRoots.some((root) => normalizedFile === root || normalizedFile.startsWith(`${root}/`))) { - nodes.add(normalizedFile); - } - } - const edges = graph.edges.filter((edge) => { - if (!nodes.has(normalizePathForDisplay(edge.from))) { - return false; - } - return edge.to.type === "external" || nodes.has(normalizePathForDisplay(edge.to.path)); - }); - return { - nodes, - edges, - }; -} - async function buildScopedReportGraph( projectRoot: string, includeRoots: string[], @@ -195,7 +172,7 @@ async function buildScopedReportGraph( ...(opts.report ? { report: opts.report } : {}), }); return { - graph: restrictGraphToIncludeRoots(index.graph, includeRoots), + graph: restrictGraphToIncludeRoots(index.graph, includeRoots, normalizePathForDisplay), indexCache, }; } @@ -205,7 +182,7 @@ async function buildScopedReportGraph( ...(opts.report ? { report: opts.report } : {}), }); return { - graph: restrictGraphToIncludeRoots(sourceGraph, includeRoots), + graph: restrictGraphToIncludeRoots(sourceGraph, includeRoots, normalizePathForDisplay), }; } @@ -292,7 +269,7 @@ async function buildInspectReport( ...workerOpts, ...(graphOptions ? { graph: graphOptions } : {}), }); - const graph = restrictGraphToIncludeRoots(index.graph, includeRoots); + const graph = restrictGraphToIncludeRoots(index.graph, includeRoots, normalizePathForDisplay); const hotspots = getHotspots(graph, { limit }); const unresolved = getUnresolvedImports(graph, { projectRoot }); const cycles = findDetailedCycles(graph); diff --git a/src/cli/options.ts b/src/cli/options.ts index 71ab6013..b3759d35 100644 --- a/src/cli/options.ts +++ b/src/cli/options.ts @@ -528,6 +528,8 @@ export function parseCacheModeOption(rawValue: string | undefined): CacheModeOpt throw new Error(`Invalid --cache value "${rawValue}". Expected one of: off, memory, disk.`); } +const STRICT_INTEGER_PATTERN = /^-?\d+$/; + function parseIntegerOptionValue( rawValue: string, optionName: string, @@ -535,7 +537,11 @@ function parseIntegerOptionValue( minValue: number, maxValue?: number, ): number { - const parsedValue = Number(rawValue); + const trimmed = rawValue.trim(); + if (!STRICT_INTEGER_PATTERN.test(trimmed)) { + throw new Error(`Invalid ${optionName} value "${rawValue}". Expected ${expectedDescription}.`); + } + const parsedValue = Number(trimmed); const isAboveMinimum = parsedValue >= minValue; const isBelowMaximum = maxValue === undefined || parsedValue <= maxValue; if (!Number.isInteger(parsedValue) || !isAboveMinimum || !isBelowMaximum) { @@ -544,6 +550,28 @@ function parseIntegerOptionValue( return parsedValue; } +export type SymbolGraphScopeOption = "all" | "imported"; +export type RefContextOption = "line" | "block"; + +export function parseSymbolGraphScopeOption( + rawValue: string | undefined, + optionName: string, +): SymbolGraphScopeOption | undefined { + if (rawValue === undefined) return undefined; + if (rawValue === "all" || rawValue === "imported") return rawValue; + throw new Error(`Invalid ${optionName} value "${rawValue}". Expected one of: all, imported.`); +} + +export function parseRefContextOption(rawValue: string | undefined, optionName: string): RefContextOption | undefined { + if (rawValue === undefined) return undefined; + if (rawValue === "line" || rawValue === "block") return rawValue; + throw new Error(`Invalid ${optionName} value "${rawValue}". Expected one of: line, block.`); +} + +export function parseImpactScopeOption(rawValue: string | undefined, optionName: string): SymbolGraphScopeOption | undefined { + return parseSymbolGraphScopeOption(rawValue, optionName); +} + function parseDefaultedIntegerOption( rawValue: string | undefined, optionName: string, diff --git a/src/drift/snapshot.ts b/src/drift/snapshot.ts index a0306745..9a5c9c3a 100644 --- a/src/drift/snapshot.ts +++ b/src/drift/snapshot.ts @@ -6,6 +6,7 @@ import { buildProjectIndex, buildProjectIndexFromFiles } from "../indexer/build- import { getApiSurface } from "../indexer/symbols.js"; import type { Edge } from "../types.js"; import { DEFAULT_PROJECT_PATTERNS, listProjectFiles } from "../util/projectFiles.js"; +import { isPathUnderIncludeRoots, normalizeIncludeRootsAbsolute } from "../util/includeRoots.js"; import { normalizePath, resolveFilePathFromRoot, toProjectDisplayPath } from "../util/paths.js"; import { countFilesByLanguage } from "./languages.js"; import type { @@ -25,21 +26,11 @@ function normalizeRoot(root: string): string { return normalizePath(path.resolve(root)); } -function normalizeIncludeRoot(root: string, includeRoot: string): string { - return normalizePath(resolveFilePathFromRoot(root, includeRoot)); -} - -function isUnderIncludeRoots(filePath: string, roots: readonly string[]): boolean { - if (!roots.length) return true; - const normalizedFile = normalizePath(filePath); - return roots.some((root) => normalizedFile === root || normalizedFile.startsWith(`${root}/`)); -} - async function listFilesForSnapshot(root: string, options: ArchitectureSnapshotOptions): Promise { if (!options.includeRoots?.length) return undefined; - const roots = options.includeRoots.map((entry) => normalizeIncludeRoot(root, entry)); + const roots = normalizeIncludeRootsAbsolute(root, options.includeRoots); const files = await listProjectFiles(root, DEFAULT_PROJECT_PATTERNS, options.discovery); - return files.filter((file) => isUnderIncludeRoots(file, roots)).sort(); + return files.filter((file) => isPathUnderIncludeRoots(normalizePath(file), roots)).sort(); } function cycleKey(files: readonly string[]): string { @@ -169,7 +160,7 @@ export async function buildArchitectureSnapshot( const index = files ? await buildProjectIndexFromFiles(root, files, indexOptions) : await buildProjectIndex(root, indexOptions); - const includeRoots = options.includeRoots?.map((entry) => normalizeIncludeRoot(root, entry)) ?? []; + const includeRoots = options.includeRoots ? normalizeIncludeRootsAbsolute(root, options.includeRoots) : []; const indexedFiles = [...index.byFile.keys()].sort(); return { diff --git a/src/graphs/adjacency.ts b/src/graphs/adjacency.ts index ed27a7c7..42615a21 100644 --- a/src/graphs/adjacency.ts +++ b/src/graphs/adjacency.ts @@ -25,8 +25,16 @@ export function buildGraphAdjacency(graph: Graph): GraphAdjacencyIndex { return { forward, reverse }; } +const adjacencyByGraph = new WeakMap(); + export function graphAdjacencyFor(graph: Graph): GraphAdjacencyIndex { - return buildGraphAdjacency(graph); + const cached = adjacencyByGraph.get(graph); + if (cached) { + return cached; + } + const built = buildGraphAdjacency(graph); + adjacencyByGraph.set(graph, built); + return built; } export function getForwardNeighbors(adjacency: GraphAdjacencyIndex, file: FileId): readonly FileId[] { diff --git a/src/graphs/cycles.ts b/src/graphs/cycles.ts index b9ac7471..3c993a16 100644 --- a/src/graphs/cycles.ts +++ b/src/graphs/cycles.ts @@ -81,38 +81,55 @@ export function findDetailedCycles( let nextIndex = 0; const stronglyConnectedComponents: number[][] = []; - function strongconnect(vertex: number) { - indices[vertex] = nextIndex; - lowlink[vertex] = nextIndex; - nextIndex++; - stack.push(vertex); - onStack[vertex] = true; - - for (const neighbor of adjacency[vertex]!) { - if (indices[neighbor] === -1) { - strongconnect(neighbor); - lowlink[vertex] = Math.min(lowlink[vertex], lowlink[neighbor]!); - } else if (onStack[neighbor]) { - lowlink[vertex] = Math.min(lowlink[vertex], indices[neighbor]!); + type TarjanFrame = { vertex: number; nextNeighbor: number }; + + for (let start = 0; start < nodeCount; start++) { + if (indices[start] !== -1) continue; + const callStack: TarjanFrame[] = [{ vertex: start, nextNeighbor: 0 }]; + + while (callStack.length > 0) { + const frame = callStack[callStack.length - 1]!; + const vertex = frame.vertex; + + if (indices[vertex] === -1) { + indices[vertex] = nextIndex; + lowlink[vertex] = nextIndex; + nextIndex += 1; + stack.push(vertex); + onStack[vertex] = true; } - } - if (lowlink[vertex] === indices[vertex]) { - const component: number[] = []; - let popped: number; - do { - popped = stack.pop()!; - onStack[popped] = false; - component.push(popped); - } while (popped !== vertex); - if (component.length > 1 || adjacency[vertex]!.includes(vertex)) { - stronglyConnectedComponents.push(component); + const neighbors = adjacency[vertex]!; + if (frame.nextNeighbor < neighbors.length) { + const neighbor = neighbors[frame.nextNeighbor]!; + frame.nextNeighbor += 1; + if (indices[neighbor] === -1) { + callStack.push({ vertex: neighbor, nextNeighbor: 0 }); + } else if (onStack[neighbor]) { + lowlink[vertex] = Math.min(lowlink[vertex]!, indices[neighbor]!); + } + continue; } - } - } - for (let index = 0; index < nodeCount; index++) { - if (indices[index] === -1) strongconnect(index); + callStack.pop(); + if (callStack.length > 0) { + const parent = callStack[callStack.length - 1]!.vertex; + lowlink[parent] = Math.min(lowlink[parent]!, lowlink[vertex]!); + } + + if (lowlink[vertex]! === indices[vertex]!) { + const component: number[] = []; + let popped: number; + do { + popped = stack.pop()!; + onStack[popped] = false; + component.push(popped); + } while (popped !== vertex); + if (component.length > 1 || adjacency[vertex]!.includes(vertex)) { + stronglyConnectedComponents.push(component); + } + } + } } const cycleDetails: DetailedCycle[] = []; diff --git a/src/impact/callCompatibility.ts b/src/impact/callCompatibility.ts index f2436020..c828f4ab 100644 --- a/src/impact/callCompatibility.ts +++ b/src/impact/callCompatibility.ts @@ -1412,6 +1412,12 @@ function incrementSkippedReason(diagnostics: ImpactDiagnostics["callCompatibilit diagnostics.skippedByReason[reason] = (diagnostics.skippedByReason[reason] ?? 0) + 1; } +function resetCallCompatibilityHints(changedSymbols: ChangedSymbol[]): void { + for (const changedSymbol of changedSymbols) { + delete changedSymbol.callCompatibility; + } +} + export async function attachCallCompatibilityHints( index: ProjectIndex, changedSymbols: ChangedSymbol[], @@ -1423,9 +1429,7 @@ export async function attachCallCompatibilityHints( referenceCache?: ReferenceLookupCache; }, ): Promise { - for (const changedSymbol of changedSymbols) { - delete changedSymbol.callCompatibility; - } + resetCallCompatibilityHints(changedSymbols); if (options.maxRefs <= 0) { return; diff --git a/src/impact/reportCompact.ts b/src/impact/reportCompact.ts index 1a401ddb..f42304b6 100644 --- a/src/impact/reportCompact.ts +++ b/src/impact/reportCompact.ts @@ -5,6 +5,7 @@ import { buildOptionalTopImpacts, mapFileEdges, mapSuggestions, + mapImpactCyclesToCompact, mapSurfaceArea, } from "./reportShared.js"; import type { ImpactReportPartsBase } from "./reportParts.js"; @@ -183,28 +184,6 @@ function buildCompactCycles( ): Pick { if (!cycles.length) return {}; return { - cycles: cycles.map((cycle) => ({ - files: cycle.files.map((file) => fileId(file)), - entryEdges: cycle.entryEdges.map((edge) => ({ - from: fileId(edge.from), - to: fileId(edge.to), - raw: edge.raw, - ...(edge.typeOnly !== undefined ? { typeOnly: edge.typeOnly } : {}), - })), - internalEdges: cycle.internalEdges.map((edge) => ({ - from: fileId(edge.from), - to: fileId(edge.to), - raw: edge.raw, - ...(edge.typeOnly !== undefined ? { typeOnly: edge.typeOnly } : {}), - })), - fileCount: cycle.fileCount, - internalEdgeCount: cycle.internalEdgeCount, - fanInFromOutside: cycle.fanInFromOutside, - priorityScore: cycle.priorityScore, - remediationHint: cycle.remediationHint, - touchesChangedFile: cycle.touchesChangedFile, - touchesImpactedFile: cycle.touchesImpactedFile, - severity: cycle.severity, - })), + cycles: mapImpactCyclesToCompact(cycles, fileId), }; } diff --git a/src/impact/reportFull.ts b/src/impact/reportFull.ts index 843d14f2..a663bddf 100644 --- a/src/impact/reportFull.ts +++ b/src/impact/reportFull.ts @@ -4,6 +4,7 @@ import { buildOptionalReexportChains, buildOptionalTopImpacts, mapFileEdges, + mapImpactCyclesForDisplay, } from "./reportShared.js"; import type { ImpactReportPartsBase } from "./reportParts.js"; import { IMPACT_SCHEMA_VERSION } from "./types.js"; @@ -73,19 +74,6 @@ function buildFullSuggestions( function buildFullCycles(cycles: ImpactCycle[], displayFile: (file: FileId) => FileId): Pick { if (!cycles.length) return {}; return { - cycles: cycles.map((cycle) => ({ - ...cycle, - files: cycle.files.map((file) => displayFile(file)), - entryEdges: cycle.entryEdges.map((edge) => ({ - ...edge, - from: displayFile(edge.from), - to: displayFile(edge.to), - })), - internalEdges: cycle.internalEdges.map((edge) => ({ - ...edge, - from: displayFile(edge.from), - to: displayFile(edge.to), - })), - })), + cycles: mapImpactCyclesForDisplay(cycles, displayFile), }; } diff --git a/src/impact/reportShared.ts b/src/impact/reportShared.ts index ec456968..b78e5a53 100644 --- a/src/impact/reportShared.ts +++ b/src/impact/reportShared.ts @@ -2,12 +2,88 @@ import type { FileId } from "../types.js"; import type { CallCompatibilityHint, ExportSummaryEntry, + ImpactCycle, + ImpactCycleEdge, ImpactSuggestion, ImpactSurfaceAreaFile, ImpactTopItem, ReexportChainEntry, + CompactImpactReport, } from "./types.js"; +type MappedImpactCycleEdge = { + from: TFile; + to: TFile; + raw: string; + typeOnly?: boolean; +}; + +type MappedImpactCycleFiles = { + files: TFile[]; + entryEdges: MappedImpactCycleEdge[]; + internalEdges: MappedImpactCycleEdge[]; +}; + +function mapImpactCycleEdge( + edge: ImpactCycleEdge, + mapFile: (file: FileId) => TFile, +): MappedImpactCycleEdge { + const mapped: MappedImpactCycleEdge = { + from: mapFile(edge.from), + to: mapFile(edge.to), + raw: edge.raw, + }; + if (edge.typeOnly !== undefined) { + mapped.typeOnly = edge.typeOnly; + } + return mapped; +} + +export function mapImpactCycleFiles( + cycle: ImpactCycle, + mapFile: (file: FileId) => TFile, +): MappedImpactCycleFiles { + return { + files: cycle.files.map((file) => mapFile(file)), + entryEdges: cycle.entryEdges.map((edge) => mapImpactCycleEdge(edge, mapFile)), + internalEdges: cycle.internalEdges.map((edge) => mapImpactCycleEdge(edge, mapFile)), + }; +} + +export function mapImpactCyclesForDisplay( + cycles: readonly ImpactCycle[], + mapFile: (file: FileId) => FileId, +): ImpactCycle[] { + return cycles.map((cycle) => ({ + ...cycle, + ...mapImpactCycleFiles(cycle, mapFile), + })); +} + +export type CompactImpactCycle = NonNullable[number]; + +export function mapImpactCyclesToCompact( + cycles: readonly ImpactCycle[], + fileId: (file: FileId) => number, +): CompactImpactCycle[] { + return cycles.map((cycle) => { + const mapped = mapImpactCycleFiles(cycle, fileId); + return { + files: mapped.files, + entryEdges: mapped.entryEdges, + internalEdges: mapped.internalEdges, + fileCount: cycle.fileCount, + internalEdgeCount: cycle.internalEdgeCount, + fanInFromOutside: cycle.fanInFromOutside, + priorityScore: cycle.priorityScore, + remediationHint: cycle.remediationHint, + touchesChangedFile: cycle.touchesChangedFile, + touchesImpactedFile: cycle.touchesImpactedFile, + severity: cycle.severity, + }; + }); +} + export type MappedExportSummaryEntry = Omit & { file: TFile; }; diff --git a/src/languages/definitions/javascript.ts b/src/languages/definitions/javascript.ts index 87cb4394..d0676a93 100644 --- a/src/languages/definitions/javascript.ts +++ b/src/languages/definitions/javascript.ts @@ -1,6 +1,11 @@ import type { LanguageDefinition } from "../types.js"; import { loadTreeSitterLanguage } from "./loadLanguage.js"; import { registerLanguage } from "../registry.js"; +import { + ECMASCRIPT_CONTROL_SPLIT_POINTS, + ECMASCRIPT_CORE_FUNCTION_BLOCKS, + ECMASCRIPT_MODULE_VAR_BLOCKS, +} from "./jsFamily.js"; const JS_OBJECT_METHOD_EXPORT_PATTERN = ` ;; CJS: module.exports = { helper () {} } @@ -23,91 +28,8 @@ export const JAVASCRIPT_DEF: LanguageDefinition = { extensions: [".js", ".jsx", ".mjs", ".cjs"], grammar: () => loadTreeSitterLanguage("tree-sitter-javascript"), structure: { - blocks: [ - { - type: "class_declaration", - nameQuery: "name: (identifier) @chunk.name", - captureId: "class", - }, - { - type: "function_declaration", - nameQuery: "name: (identifier) @chunk.name", - captureId: "function", - }, - { - type: "generator_function_declaration", - nameQuery: "name: (identifier) @chunk.name", - captureId: "function", - }, - { - type: "method_definition", - nameQuery: "name: (_) @chunk.name body: (statement_block) @chunk.block.method", - captureId: "method", - }, - - // Variable assignments (functions/arrows) - { - type: "lexical_declaration", - nameQuery: `(variable_declarator name: (identifier) @chunk.name value: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ])`, - captureId: "function", - }, - { - type: "variable_declaration", - nameQuery: `(variable_declarator name: (identifier) @chunk.name value: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ])`, - captureId: "function", - }, - { - type: "assignment_expression", - nameQuery: `left: (_) @chunk.name right: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ]`, - captureId: "function", - }, - - // Remaining functions - { - type: "arrow_function", - nameQuery: "body: (statement_block) @chunk.block.function", - captureId: "function", - }, - { - type: "function_expression", - nameQuery: "body: (statement_block) @chunk.block.function", - captureId: "function", - }, - - // Data & JSX - // { type: "object", captureId: "data" }, - // { type: "jsx_element", captureId: "jsx" }, - // { type: "jsx_self_closing_element", captureId: "jsx" }, - - // Top level vars - { type: "import_statement", captureId: "imports" }, - { - type: "lexical_declaration", - nameQuery: `(variable_declarator name: (identifier) @chunk.name)`, - captureId: "module_var", - parentType: "program", - }, - { - type: "variable_declaration", - nameQuery: `(variable_declarator name: (identifier) @chunk.name)`, - captureId: "module_var", - parentType: "program", - }, - ], - splitPoints: [ - "if_statement", - "else_clause", - "switch_statement", - // "switch_case", - // "switch_default", - "for_statement", - "for_in_statement", - "while_statement", - "do_statement", - "try_statement", - "catch_clause", - "finally_clause", - ], + blocks: [...ECMASCRIPT_CORE_FUNCTION_BLOCKS, ...ECMASCRIPT_MODULE_VAR_BLOCKS], + splitPoints: [...ECMASCRIPT_CONTROL_SPLIT_POINTS], comments: ["comment"], }, graph: { diff --git a/src/languages/definitions/jsFamily.ts b/src/languages/definitions/jsFamily.ts new file mode 100644 index 00000000..cdf9ea7e --- /dev/null +++ b/src/languages/definitions/jsFamily.ts @@ -0,0 +1,78 @@ +import type { BlockDefinition } from "../types.js"; + +export const ECMASCRIPT_CONTROL_SPLIT_POINTS = [ + "if_statement", + "else_clause", + "switch_statement", + "for_statement", + "for_in_statement", + "while_statement", + "do_statement", + "try_statement", + "catch_clause", + "finally_clause", +] as const; + +export const ECMASCRIPT_CORE_FUNCTION_BLOCKS: BlockDefinition[] = [ + { + type: "class_declaration", + nameQuery: "name: (identifier) @chunk.name", + captureId: "class", + }, + { + type: "function_declaration", + nameQuery: "name: (identifier) @chunk.name", + captureId: "function", + }, + { + type: "generator_function_declaration", + nameQuery: "name: (identifier) @chunk.name", + captureId: "function", + }, + { + type: "method_definition", + nameQuery: "name: (_) @chunk.name body: (statement_block) @chunk.block.method", + captureId: "method", + }, + { + type: "lexical_declaration", + nameQuery: `(variable_declarator name: (identifier) @chunk.name value: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ])`, + captureId: "function", + }, + { + type: "variable_declaration", + nameQuery: `(variable_declarator name: (identifier) @chunk.name value: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ])`, + captureId: "function", + }, + { + type: "assignment_expression", + nameQuery: `left: (_) @chunk.name right: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ]`, + captureId: "function", + }, + { + type: "arrow_function", + nameQuery: "body: (statement_block) @chunk.block.function", + captureId: "function", + }, + { + type: "function_expression", + nameQuery: "body: (statement_block) @chunk.block.function", + captureId: "function", + }, +]; + +export const ECMASCRIPT_MODULE_VAR_BLOCKS: BlockDefinition[] = [ + { type: "import_statement", captureId: "imports" }, + { + type: "lexical_declaration", + nameQuery: `(variable_declarator name: (identifier) @chunk.name)`, + captureId: "module_var", + parentType: "program", + }, + { + type: "variable_declaration", + nameQuery: `(variable_declarator name: (identifier) @chunk.name)`, + captureId: "module_var", + parentType: "program", + }, +]; diff --git a/src/languages/definitions/typescript.ts b/src/languages/definitions/typescript.ts index ecd2f9d1..047e3f85 100644 --- a/src/languages/definitions/typescript.ts +++ b/src/languages/definitions/typescript.ts @@ -1,6 +1,11 @@ import type { LanguageDefinition, SyntaxNodeLike } from "../types.js"; import { loadTypeScriptGrammars } from "./loadLanguage.js"; import { registerLanguage } from "../registry.js"; +import { + ECMASCRIPT_CONTROL_SPLIT_POINTS, + ECMASCRIPT_CORE_FUNCTION_BLOCKS, + ECMASCRIPT_MODULE_VAR_BLOCKS, +} from "./jsFamily.js"; function normalizeTypeScriptNativeQuery(kind: string, query: string): string { let normalized = query.replace( @@ -14,58 +19,16 @@ function normalizeTypeScriptNativeQuery(kind: string, query: string): string { return normalized; } +const TYPESCRIPT_CLASS_BLOCK = { + type: "class_declaration", + nameQuery: "name: (type_identifier) @chunk.name", + captureId: "class", +} as const; + const BASE_STRUCTURE = { blocks: [ - { - type: "class_declaration", - nameQuery: "name: (type_identifier) @chunk.name", - captureId: "class", - }, - { - type: "function_declaration", - nameQuery: "name: (identifier) @chunk.name", - captureId: "function", - }, - { - type: "generator_function_declaration", - nameQuery: "name: (identifier) @chunk.name", - captureId: "function", - }, - { - type: "method_definition", - nameQuery: "name: (_) @chunk.name body: (statement_block) @chunk.block.method", - captureId: "method", - }, - - // Variable assignments (functions/arrows) - { - type: "lexical_declaration", - nameQuery: `(variable_declarator name: (identifier) @chunk.name value: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ])`, - captureId: "function", - }, - { - type: "variable_declaration", - nameQuery: `(variable_declarator name: (identifier) @chunk.name value: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ])`, - captureId: "function", - }, - { - type: "assignment_expression", - nameQuery: `left: (_) @chunk.name right: [ (function_expression body: (statement_block) @chunk.block.function) (arrow_function body: (statement_block) @chunk.block.function) ]`, - captureId: "function", - }, - - // Remaining functions - { - type: "arrow_function", - nameQuery: "body: (statement_block) @chunk.block.function", - captureId: "function", - }, - { - type: "function_expression", - nameQuery: "body: (statement_block) @chunk.block.function", - captureId: "function", - }, - + TYPESCRIPT_CLASS_BLOCK, + ...ECMASCRIPT_CORE_FUNCTION_BLOCKS.filter((block) => block.type !== "class_declaration"), // TS Specifics { type: "interface_declaration", @@ -97,34 +60,11 @@ const BASE_STRUCTURE = { { type: "object", captureId: "data" }, // Top level vars - { type: "import_statement", captureId: "imports", parentType: "program" }, - { - type: "lexical_declaration", - nameQuery: `(variable_declarator name: (identifier) @chunk.name)`, - captureId: "module_var", - parentType: "program", - }, - { - type: "variable_declaration", - nameQuery: `(variable_declarator name: (identifier) @chunk.name)`, - captureId: "module_var", - parentType: "program", - }, - ], - splitPoints: [ - "if_statement", - "else_clause", - "switch_statement", - "switch_case", - "switch_default", - "for_statement", - "for_in_statement", - "while_statement", - "do_statement", - "try_statement", - "catch_clause", - "finally_clause", + ...ECMASCRIPT_MODULE_VAR_BLOCKS.map((block) => + block.type === "import_statement" ? { ...block, parentType: "program" as const } : block, + ), ], + splitPoints: [...ECMASCRIPT_CONTROL_SPLIT_POINTS, "switch_case", "switch_default"], comments: ["comment"], }; diff --git a/src/util/includeRoots.ts b/src/util/includeRoots.ts new file mode 100644 index 00000000..6fa901c2 --- /dev/null +++ b/src/util/includeRoots.ts @@ -0,0 +1,52 @@ +import path from "node:path"; +import type { Graph } from "../types.js"; +import { normalizePath, resolveFilePathFromRoot } from "./paths.js"; + +export function isPathUnderIncludeRoots(normalizedPath: string, normalizedRoots: readonly string[]): boolean { + if (!normalizedRoots.length) return true; + const file = normalizedPath.replace(/\\/g, "/"); + return normalizedRoots.some((root) => file === root || file.startsWith(`${root}/`)); +} + +export function normalizeIncludeRootsRelative(projectRoot: string, includeRoots: readonly string[]): string[] { + return includeRoots + .map((includeRoot) => { + const relativeRoot = path.isAbsolute(includeRoot) ? path.relative(projectRoot, includeRoot) : includeRoot; + return normalizePath(relativeRoot) + .replace(/^\.?\//, "") + .replace(/\/$/, ""); + }) + .filter((includeRoot) => includeRoot && includeRoot !== "."); +} + +export function normalizeIncludeRootsAbsolute(projectRoot: string, includeRoots: readonly string[]): string[] { + return includeRoots.map((entry) => normalizePath(resolveFilePathFromRoot(projectRoot, entry))); +} + +export function restrictGraphToIncludeRoots( + graph: Graph, + includeRoots: readonly string[], + normalizeFile: (file: string) => string = normalizePath, +): Graph { + if (!includeRoots.length) { + return graph; + } + const normalizedRoots = includeRoots.map(normalizeFile); + const nodes = new Set(); + for (const file of graph.nodes) { + const normalizedFile = normalizeFile(file); + if (isPathUnderIncludeRoots(normalizedFile, normalizedRoots)) { + nodes.add(normalizedFile); + } + } + const edges = graph.edges.filter((edge) => { + if (!nodes.has(normalizeFile(edge.from))) { + return false; + } + return edge.to.type === "external" || nodes.has(normalizeFile(edge.to.path)); + }); + return { + nodes, + edges, + }; +} diff --git a/tests/agent-tools-order.test.ts b/tests/agent-tools-order.test.ts new file mode 100644 index 00000000..d594e2c6 --- /dev/null +++ b/tests/agent-tools-order.test.ts @@ -0,0 +1,24 @@ +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { tool_getDependencies } from "../src/agent-tools.js"; + +describe("tool_getDependencies ordering", () => { + const samplePath = path.resolve(process.cwd(), "tests", "samples", "typescript"); + + it("returns dependencies sorted by file then depth before truncation", async () => { + const baseline = await tool_getDependencies(samplePath, "main.ts", { depth: 1, limit: 20 }); + expect(baseline.status).toBe("ok"); + if (baseline.status !== "ok") return; + + const limited = await tool_getDependencies(samplePath, "main.ts", { depth: 1, limit: 1 }); + expect(limited.status).toBe("ok"); + if (limited.status !== "ok") return; + + const sorted = [...baseline.dependencies].sort((left, right) => { + const fileDelta = left.file.localeCompare(right.file); + if (fileDelta !== 0) return fileDelta; + return left.depth - right.depth; + }); + expect(limited.dependencies[0]).toEqual(sorted[0]); + }); +}); diff --git a/tests/cli-options-validation.test.ts b/tests/cli-options-validation.test.ts new file mode 100644 index 00000000..e5b3399d --- /dev/null +++ b/tests/cli-options-validation.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from "vitest"; +import { parseCliArgs } from "../src/cli/context.js"; +import { + parseImpactScopeOption, + parseNonNegativeIntegerOption, + parseRefContextOption, + parseSymbolGraphScopeOption, +} from "../src/cli/options.js"; + +describe("parseIntegerOptionValue strictness", () => { + it("rejects hex, scientific, and empty integer strings", () => { + expect(() => parseNonNegativeIntegerOption("0x5", "--depth", 0)).toThrow(/Invalid --depth value "0x5"/); + expect(() => parseNonNegativeIntegerOption("1e2", "--depth", 0)).toThrow(/Invalid --depth value "1e2"/); + expect(() => parseNonNegativeIntegerOption("", "--depth", 0)).toThrow(/Invalid --depth value ""/); + }); + + it("accepts plain decimal integers", () => { + expect(parseNonNegativeIntegerOption("5", "--depth", 0)).toBe(5); + expect(parseNonNegativeIntegerOption("0", "--depth", 0)).toBe(0); + }); +}); + +describe("CLI enum option parsers", () => { + it("validates symbol graph scope values", () => { + expect(parseSymbolGraphScopeOption("all", "--symbols-detailed-scope")).toBe("all"); + expect(parseSymbolGraphScopeOption(undefined, "--symbols-detailed-scope")).toBeUndefined(); + expect(() => parseSymbolGraphScopeOption("bogus", "--symbols-detailed-scope")).toThrow( + /Invalid --symbols-detailed-scope value "bogus"/, + ); + }); + + it("validates ref context values", () => { + expect(parseRefContextOption("block", "--ref-context")).toBe("block"); + expect(() => parseRefContextOption("raw", "--ref-context")).toThrow(/Invalid --ref-context value "raw"/); + expect(parseImpactScopeOption("imported", "--scope")).toBe("imported"); + }); +}); + +describe("parseCliArgs value-option guard", () => { + it("does not consume a following flag as a value", () => { + expect(() => parseCliArgs("graph", ["--threads", "--json"])).toThrow( + /Missing value for --threads option/, + ); + }); + + it("allows negative decimal values for integer options", () => { + const parsed = parseCliArgs("hotspots", ["--limit", "-1"]); + expect(parsed.options.get("--limit")).toEqual(["-1"]); + }); +}); diff --git a/tests/graph-adjacency-cache.test.ts b/tests/graph-adjacency-cache.test.ts new file mode 100644 index 00000000..d856fb37 --- /dev/null +++ b/tests/graph-adjacency-cache.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from "vitest"; +import { buildGraphAdjacency, graphAdjacencyFor } from "../src/graphs/adjacency.js"; +import type { Graph } from "../src/types.js"; + +describe("graphAdjacencyFor memoization", () => { + it("returns the same adjacency index for repeated lookups on one graph", () => { + const graph: Graph = { + nodes: new Set(["/a.ts", "/b.ts"]), + edges: [{ from: "/a.ts", to: { type: "file", path: "/b.ts" }, raw: "./b" }], + }; + + const first = graphAdjacencyFor(graph); + const second = graphAdjacencyFor(graph); + expect(second).toBe(first); + expect(first).not.toBe(buildGraphAdjacency(graph)); + }); +}); diff --git a/tests/graph-cycles-iterative.test.ts b/tests/graph-cycles-iterative.test.ts new file mode 100644 index 00000000..608f2d6e --- /dev/null +++ b/tests/graph-cycles-iterative.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from "vitest"; +import { findDetailedCycles } from "../src/graphs/cycles.js"; +import type { Graph } from "../src/types.js"; + +describe("findDetailedCycles iterative Tarjan", () => { + it("finds a cycle in a long chain graph without recursive stack overflow", () => { + const nodes = new Set(); + const edges: Graph["edges"] = []; + const count = 2500; + for (let i = 0; i < count; i += 1) { + const current = `/chain/${i}.ts`; + nodes.add(current); + const next = `/chain/${(i + 1) % count}.ts`; + edges.push({ from: current, to: { type: "file", path: next }, raw: "./next" }); + } + const graph: Graph = { nodes, edges }; + + const cycles = findDetailedCycles(graph); + expect(cycles.length).toBeGreaterThan(0); + expect(cycles[0]?.fileCount).toBe(count); + }); +}); diff --git a/tests/graph-queries.test.ts b/tests/graph-queries.test.ts index 609f40a8..3c2fa309 100644 --- a/tests/graph-queries.test.ts +++ b/tests/graph-queries.test.ts @@ -110,7 +110,7 @@ describe("graph queries", () => { `${perfRoot}/4.ts`, `${perfRoot}/5.ts`, ]); - expect(edgeIterations).toBe(3); + expect(edgeIterations).toBe(1); }); it("should find cycles", () => { diff --git a/tests/include-roots.test.ts b/tests/include-roots.test.ts new file mode 100644 index 00000000..c40c6514 --- /dev/null +++ b/tests/include-roots.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from "vitest"; +import { + isPathUnderIncludeRoots, + normalizeIncludeRootsRelative, + restrictGraphToIncludeRoots, +} from "../src/util/includeRoots.js"; +import type { Graph } from "../src/types.js"; + +describe("includeRoots helpers", () => { + it("normalizes relative include roots", () => { + expect(normalizeIncludeRootsRelative("/proj", ["./src/", "lib"])).toEqual(["src", "lib"]); + }); + + it("checks path membership", () => { + expect(isPathUnderIncludeRoots("src/a.ts", ["src"])).toBe(true); + expect(isPathUnderIncludeRoots("lib/a.ts", ["src"])).toBe(false); + }); + + it("restricts graphs to include roots", () => { + const graph: Graph = { + nodes: new Set(["/proj/src/a.ts", "/proj/lib/b.ts"]), + edges: [ + { from: "/proj/src/a.ts", to: { type: "file", path: "/proj/lib/b.ts" }, raw: "../lib/b" }, + ], + }; + const scoped = restrictGraphToIncludeRoots(graph, ["src"], (file) => file.replace("/proj/", "")); + expect([...scoped.nodes]).toEqual(["src/a.ts"]); + expect(scoped.edges).toHaveLength(0); + }); +}); From be0b2c70100078c7ec64afbe43bd6cabcd838eac Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 14:12:07 -0400 Subject: [PATCH 3/9] fix: collect all explain references before sort-and-bound Remove the upstream findReferences cap in collectReferenceContext so displayed references are chosen after sorting, not from iteration order. Add a regression test and clarify checklist item 9 defers S1/S2 splits. --- .../2026-06-21-codegraph-technical-audit.md | 2 +- src/agent/explain.ts | 3 +-- tests/agent-explain.test.ts | 24 +++++++++++++++++++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/docs/plans/2026-06-21-codegraph-technical-audit.md b/docs/plans/2026-06-21-codegraph-technical-audit.md index 1ce7565b..8554628a 100644 --- a/docs/plans/2026-06-21-codegraph-technical-audit.md +++ b/docs/plans/2026-06-21-codegraph-technical-audit.md @@ -279,4 +279,4 @@ Ordered checklist. Each item should land with a regression test in the same chan - [x] **6. SQLite schema migration** (`sqlite/schema.ts`) -- versioned migrator keyed off on-disk version; v1->v2 upgrade regression test (`AGENTS.md:26`). - [x] **7. U1 sort-before-truncate / U2-U4 CLI validation** -- sort before bound; reject bad enum/int flags. - [x] **8. P1/P2 cycles + adjacency** (`graphs/cycles.ts`, `traversal.ts`) -- iterative Tarjan, memoized adjacency. -- [x] **9. S1-S6 structural** -- split god modules, dedupe JS/TS + cycle mappers. +- [x] **9. S3-S6 structural (S1/S2 deferred)** -- dedupe JS/TS shared blocks, cycle mappers, include-root util, callCompatibility reset helper; full `duplicates.ts`/`cli.ts` splits deferred. diff --git a/src/agent/explain.ts b/src/agent/explain.ts index c7140b13..eca895b7 100644 --- a/src/agent/explain.ts +++ b/src/agent/explain.ts @@ -731,8 +731,7 @@ async function collectReferenceContext( referenceLimit: number, snippetLimit: number, ): Promise { - const collectionLimit = Math.max(referenceLimit, snippetLimit) + 1; - const result = await findReferences(snapshot.index, { def }, { context: "line", maxReferences: collectionLimit }); + const result = await findReferences(snapshot.index, { def }, { context: "line" }); if (result.status !== "ok") return emptyReferenceContext(); const references = result.references diff --git a/tests/agent-explain.test.ts b/tests/agent-explain.test.ts index bfb7db3c..0a9d3361 100644 --- a/tests/agent-explain.test.ts +++ b/tests/agent-explain.test.ts @@ -44,6 +44,20 @@ async function mkDuplicateRepo(): Promise { return root; } + +async function mkManyReferenceRepo(): Promise { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "cg-agent-explain-refs-")); + await fs.writeFile(path.join(root, "target.ts"), "export function sharedTarget() { return 1; }\n"); + for (let index = 0; index < 8; index += 1) { + const name = `ref-${String(index).padStart(2, "0")}.ts`; + await fs.writeFile( + path.join(root, name), + `import { sharedTarget } from './target';\nexport const value${index} = sharedTarget();\n`, + ); + } + return root; +} + async function mkManyDuplicateRepo(): Promise { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cg-agent-explain-many-dups-")); await fs.mkdir(path.join(root, "src"), { recursive: true }); @@ -251,6 +265,16 @@ describe("agent explain", () => { expect(explanation.omittedCounts.snippets).toBeGreaterThan(0); }); + + it("returns references sorted by file before truncation", async () => { + const root = await mkManyReferenceRepo(); + const explanation = await explainCodegraphTarget({ root, target: "sharedTarget", maxReferences: 2 }); + + expect(explanation.references).toHaveLength(2); + expect(explanation.omittedCounts.references).toBeGreaterThan(0); + expect(explanation.references.map((reference) => reference.file)).toEqual(["ref-00.ts", "ref-01.ts"]); + }); + it("bounds file symbols and reports omitted counts", async () => { const root = await mkRepo(); await fs.writeFile( From 4a6812b34341589524c53a0a0ad954233529764d Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 15:55:39 -0400 Subject: [PATCH 4/9] fix: harden audit remediation follow-ups --- .../2026-06-21-codegraph-technical-audit.md | 119 ++++++++++-------- src/agent/explain.ts | 4 +- src/cli/options.ts | 5 +- src/graphs/cycles.ts | 37 +++--- src/impact/callCompatibility.ts | 103 ++++++++------- src/indexer/build-cache/module-cache.ts | 11 +- src/indexer/incremental-plan.ts | 1 - src/indexer/navigation.ts | 2 +- src/sqlite/schema.ts | 5 + src/util/lruMap.ts | 10 +- tests/agent-explain.test.ts | 2 - tests/cli-options-validation.test.ts | 4 +- tests/git-revision-safety.test.ts | 4 +- tests/graph-queries.test.ts | 24 ++++ .../parse-resilience.test.ts | 3 - tests/impact-transitive-order.test.ts | 5 +- tests/include-roots.test.ts | 4 +- tests/incremental-plan.test.ts | 9 +- tests/lazy-symbols.test.ts | 1 - tests/lru-map.test.ts | 23 ++++ tests/module-cache-lru.test.ts | 20 ++- tests/sqlite.test.ts | 19 +++ 22 files changed, 263 insertions(+), 152 deletions(-) diff --git a/docs/plans/2026-06-21-codegraph-technical-audit.md b/docs/plans/2026-06-21-codegraph-technical-audit.md index 8554628a..83936375 100644 --- a/docs/plans/2026-06-21-codegraph-technical-audit.md +++ b/docs/plans/2026-06-21-codegraph-technical-audit.md @@ -8,80 +8,81 @@ Severity legend: **C** critical · **H** high · **M** medium · **L** low. ## 1. Accuracy & Stability -| ID | File | Finding | -|----|------|---------| -| C1 | `src/util/git.ts` | **Git argument injection.** `gitDiffArgs` and the `changedSince` path emit user-controlled revisions (`--git-base/--git-head/--changed-since`) as standalone argv tokens before `--`. A value like `--output=/etc/x` or `--upload-pack=...` is parsed as a git flag. | -| C2 | `src/indexer/build-index.ts` | **Stale incremental graph.** Incremental rebuild flags only self-changed files; transitive *dependents* of a modified file reuse cached edges/symbols, yielding stale graph edges and wrong navigation/impact results. | -| C3 | `src/impact/callCompatibility.ts` | **Unguarded parse aborts analysis.** Three `ensureParsedContext` calls (1331, 1429, 1493) throw on unparseable/deleted files; unlike `map.ts` (which guards every call), one bad file aborts the entire impact run -> zero results. | -| H1 | `src/impact/transitive.ts` | **Shared-array mutation.** BFS mutates the `reasons` array in place across aliased `ImpactItem` references (159-163); emitted items can gain reasons from later traversal. | -| H2 | `src/impact/transitive.ts` | **Order-dependent propagation.** `visited.add` (157) locks a node on the first path that reaches it; severity/confidence on the *item* are merged via `Math.max`, but the node is never re-enqueued, so a shorter/stronger later path cannot re-propagate downstream depth/reasons -> traversal order affects results. | -| H3 | `src/indexer/build-workers.ts` | **Swallowed teardown failure.** `pool.destroy()` failure caught by empty `catch` -> leaked worker threads reported as success. | -| H4 | `src/cli.ts` | **No embedder error boundary.** Exported `runCli` (977) lacks the top-level try/catch the direct-exec path has; dispatcher throws become unhandled rejections for library callers. | -| M1 | `src/util/concurrency.ts` | `Semaphore.release()` has no over-release guard (corrupts shared global IO semaphore); `acquire()` is non-cancelable (timeout/abort leaks a dead waiter -> off-by-one permit deadlock). | -| M2 | `src/util/comments.ts` | **ReDoS** in `stripPythonCommentsAndStrings` (393-398): backreferenced lazy triple-quote + classic catastrophic string regex backtrack on adversarial unterminated input. | -| M3 | `src/native/bindingLoader.ts` | Fallback silently loads a stale published addon when the local build fails, discarding the actionable local-build error (`error ?? lastError` prefers the wrong one). | -| M4 | `src/impact/parse.ts` | `decodeGitPath` decodes octal escapes via per-escape `fromCharCode`, corrupting multibyte UTF-8 paths; malformed hunk headers drop files/hunks silently. | -| M5 | `src/languages/sfc.ts` | Closing-tag scan matches `` inside strings/comments and `break`s, abandoning all remaining SFC blocks on one bad tag. | +| ID | File | Finding | +| --- | --------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| C1 | `src/util/git.ts` | **Git argument injection.** `gitDiffArgs` and the `changedSince` path emit user-controlled revisions (`--git-base/--git-head/--changed-since`) as standalone argv tokens before `--`. A value like `--output=/etc/x` or `--upload-pack=...` is parsed as a git flag. | +| C2 | `src/indexer/build-index.ts` | **Stale incremental graph.** Incremental rebuild flags only self-changed files; transitive _dependents_ of a modified file reuse cached edges/symbols, yielding stale graph edges and wrong navigation/impact results. | +| C3 | `src/impact/callCompatibility.ts` | **Unguarded parse aborts analysis.** Three `ensureParsedContext` calls (1331, 1429, 1493) throw on unparseable/deleted files; unlike `map.ts` (which guards every call), one bad file aborts the entire impact run -> zero results. | +| H1 | `src/impact/transitive.ts` | **Shared-array mutation.** BFS mutates the `reasons` array in place across aliased `ImpactItem` references (159-163); emitted items can gain reasons from later traversal. | +| H2 | `src/impact/transitive.ts` | **Order-dependent propagation.** `visited.add` (157) locks a node on the first path that reaches it; severity/confidence on the _item_ are merged via `Math.max`, but the node is never re-enqueued, so a shorter/stronger later path cannot re-propagate downstream depth/reasons -> traversal order affects results. | +| H3 | `src/indexer/build-workers.ts` | **Swallowed teardown failure.** `pool.destroy()` failure caught by empty `catch` -> leaked worker threads reported as success. | +| H4 | `src/cli.ts` | **No embedder error boundary.** Exported `runCli` (977) lacks the top-level try/catch the direct-exec path has; dispatcher throws become unhandled rejections for library callers. | +| M1 | `src/util/concurrency.ts` | `Semaphore.release()` has no over-release guard (corrupts shared global IO semaphore); `acquire()` is non-cancelable (timeout/abort leaks a dead waiter -> off-by-one permit deadlock). | +| M2 | `src/util/comments.ts` | **ReDoS** in `stripPythonCommentsAndStrings` (393-398): backreferenced lazy triple-quote + classic catastrophic string regex backtrack on adversarial unterminated input. | +| M3 | `src/native/bindingLoader.ts` | Fallback silently loads a stale published addon when the local build fails, discarding the actionable local-build error (`error ?? lastError` prefers the wrong one). | +| M4 | `src/impact/parse.ts` | `decodeGitPath` decodes octal escapes via per-escape `fromCharCode`, corrupting multibyte UTF-8 paths; malformed hunk headers drop files/hunks silently. | +| M5 | `src/languages/sfc.ts` | Closing-tag scan matches `` inside strings/comments and `break`s, abandoning all remaining SFC blocks on one bad tag. | --- ## 2. Performance -| ID | File | Finding (Big-O) | -|----|------|-----------------| -| P1 | `src/graphs/cycles.ts` | Recursive Tarjan `strongconnect` -> **stack overflow** on deep graphs; per-component edge rescan is **O(C·E)** (quadratic on heavily-cyclic graphs). | -| P2 | `src/graphs/traversal.ts` | Adjacency index rebuilt **per query** in `getDependencies/getReverseDependencies/getShortestPath` -> O(V·E) for multi-query callers. Graph is immutable post-build; memoize via `WeakMap`. | -| P3 | `src/indexer/navigation.ts` | `findReferences` receiver-method fallback is **O(F·N·cost(goto))** -- sweeps every file, runs per-node `goToDefinition`. | -| P4 | `src/indexer/navigation-references.ts` | `filesExportingDefinition` sweeps the entire index per query (first call uncached). Precompute an inverted def->exporting-files index at finalize. | -| P5 | `src/util/resolution.ts` | Unbounded process-global `resolveSpecifierCache` keyed per-`fromFile` -> near-zero hit rate for relative specs + memory growth; path-map probing uses sync `fileExistsSync`, blocking the event loop inside "parallel" resolution. | -| P6 | `src/indexer/build-cache/module-cache.ts` | Process-global `memoryCache` Map is **unbounded, never evicted, not scoped to projectRoot/build** -> leaks full `ModuleIndex` objects in long-lived processes (MCP server, sessions). | -| P7 | `src/util/lazySymbols.ts` | `LazyProjectIndex.modules` effectively unbounded: eviction only unloads `locals` arrays, never deletes Map entries; `maxCached < 0` silently disables eviction. | -| P8 | `src/util/bloomFilter.ts` | `getHashes` derives only 7 distinct offsets from sha256 -> `hashCount` 8-10 silently degrades; size clamped *after* `k` computed (desync), understating true FPR. `add()` OOB guard `return`s mid-insert (latent false negative). | -| P9 | `src/duplicates.ts` | Per-unit `text` + `normalizedTokens` stored/serialized but never read after construction (~3x memory); `astContextCache` holds all parse trees for the whole run. | -| P10 | `src/sqlite/canned-query.ts` | `bfsFileTraversal` walks the entire reachable graph (no depth/node cap), one query per node; `IN (...)` list can exceed SQLite's ~999-variable limit. | +| ID | File | Finding (Big-O) | +| --- | ----------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| P1 | `src/graphs/cycles.ts` | Recursive Tarjan `strongconnect` -> **stack overflow** on deep graphs; per-component edge rescan is **O(C·E)** (quadratic on heavily-cyclic graphs). | +| P2 | `src/graphs/traversal.ts` | Adjacency index rebuilt **per query** in `getDependencies/getReverseDependencies/getShortestPath` -> O(V·E) for multi-query callers. Graph is immutable post-build; memoize via `WeakMap`. | +| P3 | `src/indexer/navigation.ts` | `findReferences` receiver-method fallback is **O(F·N·cost(goto))** -- sweeps every file, runs per-node `goToDefinition`. | +| P4 | `src/indexer/navigation-references.ts` | `filesExportingDefinition` sweeps the entire index per query (first call uncached). Precompute an inverted def->exporting-files index at finalize. | +| P5 | `src/util/resolution.ts` | Unbounded process-global `resolveSpecifierCache` keyed per-`fromFile` -> near-zero hit rate for relative specs + memory growth; path-map probing uses sync `fileExistsSync`, blocking the event loop inside "parallel" resolution. | +| P6 | `src/indexer/build-cache/module-cache.ts` | Process-global `memoryCache` Map is **unbounded, never evicted, not scoped to projectRoot/build** -> leaks full `ModuleIndex` objects in long-lived processes (MCP server, sessions). | +| P7 | `src/util/lazySymbols.ts` | `LazyProjectIndex.modules` effectively unbounded: eviction only unloads `locals` arrays, never deletes Map entries; `maxCached < 0` silently disables eviction. | +| P8 | `src/util/bloomFilter.ts` | `getHashes` derives only 7 distinct offsets from sha256 -> `hashCount` 8-10 silently degrades; size clamped _after_ `k` computed (desync), understating true FPR. `add()` OOB guard `return`s mid-insert (latent false negative). | +| P9 | `src/duplicates.ts` | Per-unit `text` + `normalizedTokens` stored/serialized but never read after construction (~3x memory); `astContextCache` holds all parse trees for the whole run. | +| P10 | `src/sqlite/canned-query.ts` | `bfsFileTraversal` walks the entire reachable graph (no depth/node cap), one query per node; `IN (...)` list can exceed SQLite's ~999-variable limit. | --- ## 3. Usability (Humans & Agents) -| ID | File | Finding | -|----|------|---------| -| U1 | `src/agent/explain.ts` | **Non-deterministic agent output.** `collectReferenceContext` (734-748) truncates references to `collectionLimit` *before* sorting -> the displayed subset is index-iteration-order dependent when matches exceed the cap. Same pattern in `agent-tools.ts:449-463`. Agents cannot rely on stable references. | -| U2 | `src/cli/impact.ts`, `src/cli/graph.ts` | Unvalidated enum casts: `--scope`, `--ref-context`, `--symbols-detailed-scope` are `as`-cast without validation; bad values are silently ignored rather than rejected. | -| U3 | `src/cli/context.ts` | Long value-options consume the next token even if it is another `--flag` (no leading-dash guard like `-o` has) -> silent wrong values. | -| U4 | `src/cli/options.ts` | `parseIntegerOptionValue` uses bare `Number()`: `--threads ''` -> 0, `--depth 0x5` -> 5, `1e2` accepted. Pre-validate `/^-?\d+$/`. | -| U5 | `src/mcp/http.ts` | HTTP MCP transport has **no authentication**; wildcard binds (`0.0.0.0`/`::`) plus external-interface host headers expose an unauthenticated tool surface to the network. | -| U6 | `src/impact/streaming.ts` | `impactError` is checked only *after* the drain loop -> partial `impactItem` chunks are emitted before the terminal `error` chunk, contradicting the streaming contract. Also unbounded `createAsyncQueue` = no backpressure. | +| ID | File | Finding | +| --- | --------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| U1 | `src/agent/explain.ts` | **Non-deterministic agent output.** `collectReferenceContext` (734-748) truncates references to `collectionLimit` _before_ sorting -> the displayed subset is index-iteration-order dependent when matches exceed the cap. Same pattern in `agent-tools.ts:449-463`. Agents cannot rely on stable references. | +| U2 | `src/cli/impact.ts`, `src/cli/graph.ts` | Unvalidated enum casts: `--scope`, `--ref-context`, `--symbols-detailed-scope` are `as`-cast without validation; bad values are silently ignored rather than rejected. | +| U3 | `src/cli/context.ts` | Long value-options consume the next token even if it is another `--flag` (no leading-dash guard like `-o` has) -> silent wrong values. | +| U4 | `src/cli/options.ts` | `parseIntegerOptionValue` uses bare `Number()`: `--threads ''` -> 0, `--depth 0x5` -> 5, `1e2` accepted. Pre-validate `/^-?\d+$/`. | +| U5 | `src/mcp/http.ts` | HTTP MCP transport has **no authentication**; wildcard binds (`0.0.0.0`/`::`) plus external-interface host headers expose an unauthenticated tool surface to the network. | +| U6 | `src/impact/streaming.ts` | `impactError` is checked only _after_ the drain loop -> partial `impactItem` chunks are emitted before the terminal `error` chunk, contradicting the streaming contract. Also unbounded `createAsyncQueue` = no backpressure. | --- ## 4. Simplicity & Organization -| ID | File | Finding | -|----|------|---------| -| S1 | `src/duplicates.ts` | **2770-line god module.** Should split into ~8 modules (types, unit-cache, fingerprint, units, candidates, scoring, clustering, orchestrator). | -| S2 | `src/cli.ts` | `runCliWithActiveRuntime` is a ~870-line function with a 25-branch `cmd===` ladder, repeated handler-context boilerplate, and triple-duplicated version handling. | -| S3 | `src/languages/definitions/{javascript,typescript}.ts` | ~200 lines duplicated (blocks/splitPoints/locals/importBindings/classify/scope, `isTypeOnly` x3). `cFamily.ts` already proves the correct shared-factory pattern. | -| S4 | `src/impact/{reportFull,reportCompact}.ts` | `buildFullCycles`/`buildCompactCycles` duplicate cycle file/edge mapping -- extract a generic `mapCycles` into `reportShared.ts`. | -| S5 | `src/cli.ts`, `inspect.ts`, `orient.ts` | Include-root filtering logic duplicated across three files; extract one util. | -| S6 | `src/impact/callCompatibility.ts` | `attachCallCompatibilityHints` is a ~172-line function with 6+ nesting levels. | +| ID | File | Finding | +| --- | ------------------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| S1 | `src/duplicates.ts` | **2770-line god module.** Should split into ~8 modules (types, unit-cache, fingerprint, units, candidates, scoring, clustering, orchestrator). | +| S2 | `src/cli.ts` | `runCliWithActiveRuntime` is a ~870-line function with a 25-branch `cmd===` ladder, repeated handler-context boilerplate, and triple-duplicated version handling. | +| S3 | `src/languages/definitions/{javascript,typescript}.ts` | ~200 lines duplicated (blocks/splitPoints/locals/importBindings/classify/scope, `isTypeOnly` x3). `cFamily.ts` already proves the correct shared-factory pattern. | +| S4 | `src/impact/{reportFull,reportCompact}.ts` | `buildFullCycles`/`buildCompactCycles` duplicate cycle file/edge mapping -- extract a generic `mapCycles` into `reportShared.ts`. | +| S5 | `src/cli.ts`, `inspect.ts`, `orient.ts` | Include-root filtering logic duplicated across three files; extract one util. | +| S6 | `src/impact/callCompatibility.ts` | `attachCallCompatibilityHints` is a ~172-line function with 6+ nesting levels. | --- ## 5. Usefulness (Dead / Redundant Code) -| ID | File | Finding | -|----|------|---------| -| D1 | `src/util/bloomFilter.ts` | `toBuffer`/`fromBuffer` omit `size`/`hashCount` and are dead/unsafe -- filters are rebuilt per-process, never persisted. | -| D2 | `src/duplicates.ts` | Per-unit `text`/`normalizedTokens` fields are written and serialized but never read post-construction (see P9). | -| D3 | `src/native/projectedTree.ts` | Empty-source clamp makes the `??` fallback (line 36) dead code. | -| D4 | `src/util/partialResults.ts` | Dead `indexOf` in the rejected branch. | +| ID | File | Finding | +| --- | ----------------------------- | ------------------------------------------------------------------------------------------------------------------------ | +| D1 | `src/util/bloomFilter.ts` | `toBuffer`/`fromBuffer` omit `size`/`hashCount` and are dead/unsafe -- filters are rebuilt per-process, never persisted. | +| D2 | `src/duplicates.ts` | Per-unit `text`/`normalizedTokens` fields are written and serialized but never read post-construction (see P9). | +| D3 | `src/native/projectedTree.ts` | Empty-source clamp makes the `??` fallback (line 36) dead code. | +| D4 | `src/util/partialResults.ts` | Dead `indexOf` in the rejected branch. | --- ## Issue Matrix -- Critical & High (root cause -> impact -> fix) ### C1 -- Git argument injection (`src/util/git.ts`) + **Root cause:** revisions pushed as standalone argv with no leading-dash guard. **Systemic impact:** crafted `--base`/`--head`/`--changed-since` becomes a git flag (arbitrary file write via `--output=`, RCE-adjacent via `--upload-pack=`) in `impact`/`review`/`drift`. @@ -101,10 +102,12 @@ export function gitDiffArgs(base: string, head: string, extraArgs: string[] = [] return ["diff", "--end-of-options", ...extraArgs, `${base}..${head}`]; } ``` + Apply `assertSafeRevision` to `changedSince` in `listChangedFiles`/`getUnifiedDiff` too. ### C2 -- Stale incremental graph (`src/indexer/build-index.ts` + `incremental-plan.ts`) -**Root cause:** `changedFiles` = self-changed only; dependents of *modified* files are not invalidated (`collectDeletedTrackedFileDependents` covers only deleted targets). + +**Root cause:** `changedFiles` = self-changed only; dependents of _modified_ files are not invalidated (`collectDeletedTrackedFileDependents` covers only deleted targets). **Systemic impact:** after editing an exported symbol, dependents keep stale edges -> wrong go-to-def/refs/impact until full rebuild. **Fix:** compute a reverse-dependency closure over all changed (not just deleted) files and add them to the rebuild set. @@ -126,6 +129,7 @@ function expandWithDependents(reverseDeps: Map, changed: Set unstable agent + CI output. **Fix:** copy-on-write reasons and upgrade on stronger path. @@ -161,6 +166,7 @@ if (existing) { ``` ### H3 -- Swallowed worker teardown (`src/indexer/build-workers.ts`) + ```ts try { await setup.pool.destroy(); @@ -172,6 +178,7 @@ try { ``` ### H4 -- Embedder error boundary (`src/cli.ts`) + ```ts export async function runCli(argv: string[] = process.argv.slice(2)): Promise { try { @@ -184,17 +191,22 @@ export async function runCli(argv: string[] = process.argv.slice(2)): Promise ({ file: normalizeAgentFilePath(snapshot.root, r.file), range: r.range })) .sort((a, b) => a.file.localeCompare(b.file) || a.range.start.line - b.range.start.line); const boundedReferences = boundAgentList(references, referenceLimit); // bound AFTER sort ``` + (Raise the `findReferences` `maxReferences` cap above the display limit so the cap itself isn't order-dependent.) ### P6/P7/P5 -- Unbounded memory caches (`module-cache.ts`, `lazySymbols.ts`, `resolution.ts`) + **Fix:** LRU with a cap, keyed by `projectRoot::file`, cleared on teardown. + ```ts const MAX_MEMORY_CACHE = 5000; function setMemoryCache(key: string, entry: ModuleCacheEntry): void { @@ -204,10 +216,13 @@ function setMemoryCache(key: string, entry: ModuleCacheEntry): void { } memoryCache.set(key, entry); } -export function clearMemoryCache(): void { memoryCache.clear(); } +export function clearMemoryCache(): void { + memoryCache.clear(); +} ``` ### SQLite schema migration gap (`src/sqlite/schema.ts`) + **Root cause:** `SQLITE_SCHEMA_VERSION = 2` but only the `visibility` column has an `ALTER`; new tables/indexes rely on `CREATE ... IF NOT EXISTS` and the version is stamped unconditionally with nothing reading it. Violates `AGENTS.md:26`. **Fix:** read on-disk `graph_metadata.schema_version`, run an explicit migrator for each version step, stamp the new version only after success, and add a v1->v2 upgrade regression test. @@ -279,4 +294,4 @@ Ordered checklist. Each item should land with a regression test in the same chan - [x] **6. SQLite schema migration** (`sqlite/schema.ts`) -- versioned migrator keyed off on-disk version; v1->v2 upgrade regression test (`AGENTS.md:26`). - [x] **7. U1 sort-before-truncate / U2-U4 CLI validation** -- sort before bound; reject bad enum/int flags. - [x] **8. P1/P2 cycles + adjacency** (`graphs/cycles.ts`, `traversal.ts`) -- iterative Tarjan, memoized adjacency. -- [x] **9. S3-S6 structural (S1/S2 deferred)** -- dedupe JS/TS shared blocks, cycle mappers, include-root util, callCompatibility reset helper; full `duplicates.ts`/`cli.ts` splits deferred. +- [x] **9. S3-S6 structural (S1/S2 deferred)** -- dedupe JS/TS shared blocks, cycle mappers, include-root util, callCompatibility hint/reset helpers; full `duplicates.ts`/`cli.ts` splits deferred. diff --git a/src/agent/explain.ts b/src/agent/explain.ts index eca895b7..26fe859c 100644 --- a/src/agent/explain.ts +++ b/src/agent/explain.ts @@ -192,6 +192,7 @@ type ResolvedExplainTarget = const SQL_FACT_READ_CONCURRENCY = 32; const AGENT_DUPLICATE_MAX_PAIRS = 20_000; +const AGENT_EXPLAIN_REFERENCE_COLLECTION_MULTIPLIER = 10; export async function explainCodegraphTarget(request: AgentExplainTarget): Promise { const session = createAgentSession({ @@ -731,7 +732,8 @@ async function collectReferenceContext( referenceLimit: number, snippetLimit: number, ): Promise { - const result = await findReferences(snapshot.index, { def }, { context: "line" }); + const collectionLimit = Math.max(referenceLimit, snippetLimit) * AGENT_EXPLAIN_REFERENCE_COLLECTION_MULTIPLIER; + const result = await findReferences(snapshot.index, { def }, { context: "line", maxReferences: collectionLimit }); if (result.status !== "ok") return emptyReferenceContext(); const references = result.references diff --git a/src/cli/options.ts b/src/cli/options.ts index b3759d35..5734c582 100644 --- a/src/cli/options.ts +++ b/src/cli/options.ts @@ -568,7 +568,10 @@ export function parseRefContextOption(rawValue: string | undefined, optionName: throw new Error(`Invalid ${optionName} value "${rawValue}". Expected one of: line, block.`); } -export function parseImpactScopeOption(rawValue: string | undefined, optionName: string): SymbolGraphScopeOption | undefined { +export function parseImpactScopeOption( + rawValue: string | undefined, + optionName: string, +): SymbolGraphScopeOption | undefined { return parseSymbolGraphScopeOption(rawValue, optionName); } diff --git a/src/graphs/cycles.ts b/src/graphs/cycles.ts index 3c993a16..28350140 100644 --- a/src/graphs/cycles.ts +++ b/src/graphs/cycles.ts @@ -64,12 +64,19 @@ export function findDetailedCycles( nodes.forEach((node, index) => indexByNode.set(node, index)); const adjacency = nodes.map(() => [] as number[]); + const incomingFileEdges = nodes.map(() => [] as CycleInternalEdge[]); for (const edge of graph.edges) { if (edge.to.type !== "file") continue; const fromIndex = indexByNode.get(edge.from); const toIndex = indexByNode.get(edge.to.path); if (fromIndex !== undefined && toIndex !== undefined) { adjacency[fromIndex]!.push(toIndex); + incomingFileEdges[toIndex]!.push({ + from: edge.from, + to: edge.to.path, + raw: edge.raw, + ...(edge.typeOnly !== undefined ? { typeOnly: edge.typeOnly } : {}), + }); } } @@ -144,27 +151,15 @@ export function findDetailedCycles( let internalEdgeCount = 0; let fanInFromOutside = 0; - for (const edge of graph.edges) { - if (edge.to.type !== "file") continue; - const fromInComponent = componentFiles.has(edge.from); - const toInComponent = componentFiles.has(edge.to.path); - if (fromInComponent && toInComponent) { - internalEdgeCount += 1; - internalEdges.push({ - from: edge.from, - to: edge.to.path, - raw: edge.raw, - ...(edge.typeOnly !== undefined ? { typeOnly: edge.typeOnly } : {}), - }); - } - if (!fromInComponent && toInComponent) { - fanInFromOutside += 1; - entryEdges.push({ - from: edge.from, - to: edge.to.path, - raw: edge.raw, - ...(edge.typeOnly !== undefined ? { typeOnly: edge.typeOnly } : {}), - }); + for (const nodeIndex of component) { + for (const edge of incomingFileEdges[nodeIndex]!) { + if (componentFiles.has(edge.from)) { + internalEdgeCount += 1; + internalEdges.push(edge); + } else { + fanInFromOutside += 1; + entryEdges.push(edge); + } } } diff --git a/src/impact/callCompatibility.ts b/src/impact/callCompatibility.ts index c828f4ab..cdb6c0b1 100644 --- a/src/impact/callCompatibility.ts +++ b/src/impact/callCompatibility.ts @@ -1391,7 +1391,6 @@ function classifyCompatibility( return { status: "compatible", reason: "compatible_argument_count" }; } - async function tryEnsureParsedContext( file: string, parsedEntry: Parameters[1], @@ -1412,6 +1411,57 @@ function incrementSkippedReason(diagnostics: ImpactDiagnostics["callCompatibilit diagnostics.skippedByReason[reason] = (diagnostics.skippedByReason[reason] ?? 0) + 1; } +async function buildCallCompatibilityHintForReference(input: { + index: ProjectIndex; + changedSymbol: ChangedSymbol; + signature: CallableSignature; + ref: Reference; + diagnostics?: ImpactDiagnostics["callCompatibility"] | undefined; + projectRoot?: string | undefined; +}): Promise { + const { index, changedSymbol, signature, ref, diagnostics, projectRoot } = input; + if (ref.file === changedSymbol.file && sameRangeStart(ref.range, changedSymbol.range)) { + return null; + } + + const calleeStartIndex = ref.range.start.index; + if (calleeStartIndex === undefined) { + return null; + } + + const parsedCallsite = await tryEnsureParsedContext(ref.file, index.parsed?.get(ref.file), diagnostics); + if (!parsedCallsite) { + return null; + } + const callsiteRequest: ExtractCallsiteArgumentsRequest = { + languageId: parsedCallsite.sup.id, + source: parsedCallsite.source, + calleeStartIndex, + tree: parsedCallsite.tree, + ...(ref.range.end.index !== undefined ? { calleeEndIndex: ref.range.end.index } : {}), + }; + const actual = extractCallsiteArguments(callsiteRequest); + if (!actual) { + if (diagnostics) { + diagnostics.unknownCallsites += 1; + } + return null; + } + + const compatibility = classifyCompatibility(signature, actual); + const callerSymbolId = findCallerSymbolId(index, ref); + const callsiteFile = projectRoot ? path.relative(projectRoot, ref.file).replace(/\\/g, "/") : ref.file; + return { + ...compatibility, + changedSymbolId: changedSymbol.id, + callsiteFile, + callsiteRange: ref.range, + ...(callerSymbolId ? { callerSymbolId } : {}), + expected: signature, + actual, + }; +} + function resetCallCompatibilityHints(changedSymbols: ChangedSymbol[]): void { for (const changedSymbol of changedSymbols) { delete changedSymbol.callCompatibility; @@ -1507,53 +1557,22 @@ export async function attachCallCompatibilityHints( let consideredCallsites = 0; const addHintForReference = async (ref: Reference): Promise => { - if (ref.file === changedSymbol.file && sameRangeStart(ref.range, changedSymbol.range)) { - return; - } if (consideredCallsites >= options.maxRefs) { return; } - - const calleeStartIndex = ref.range.start.index; - if (calleeStartIndex === undefined) { - return; - } - - const parsedCallsite = await tryEnsureParsedContext(ref.file, index.parsed?.get(ref.file), diagnostics); - if (!parsedCallsite) { - return; - } - const callsiteRequest: ExtractCallsiteArgumentsRequest = { - languageId: parsedCallsite.sup.id, - source: parsedCallsite.source, - calleeStartIndex, - tree: parsedCallsite.tree, - ...(ref.range.end.index !== undefined ? { calleeEndIndex: ref.range.end.index } : {}), - }; - const actual = extractCallsiteArguments(callsiteRequest); - if (!actual) { - if (diagnostics) { - diagnostics.unknownCallsites += 1; - } + const hint = await buildCallCompatibilityHintForReference({ + index, + changedSymbol, + signature, + ref, + diagnostics, + ...(options.projectRoot ? { projectRoot: options.projectRoot } : {}), + }); + if (!hint) { return; } consideredCallsites += 1; - - const compatibility = classifyCompatibility(signature, actual); - const callerSymbolId = findCallerSymbolId(index, ref); - let callsiteFile = ref.file; - if (options.projectRoot) { - callsiteFile = path.relative(options.projectRoot, ref.file).replace(/\\/g, "/"); - } - hints.push({ - ...compatibility, - changedSymbolId: changedSymbol.id, - callsiteFile, - callsiteRange: ref.range, - ...(callerSymbolId ? { callerSymbolId } : {}), - expected: signature, - actual, - }); + hints.push(hint); }; for (const ref of refs) { diff --git a/src/indexer/build-cache/module-cache.ts b/src/indexer/build-cache/module-cache.ts index 7a07aa13..66d9570d 100644 --- a/src/indexer/build-cache/module-cache.ts +++ b/src/indexer/build-cache/module-cache.ts @@ -46,6 +46,15 @@ function memoryCacheKey(projectRoot: string, file: string): string { export function clearMemoryCache(): void { memoryCache.clear(); } + +function clearMemoryCacheForProject(projectRoot: string): void { + const prefix = `${normalizePath(projectRoot)}::`; + for (const key of memoryCache.keys()) { + if (key.startsWith(prefix)) { + memoryCache.delete(key); + } + } +} const diskCacheDatabases = new Map(); export function cacheRoot(projectRoot: string, opts?: BuildOptions): string { @@ -110,7 +119,7 @@ function getDiskCacheDatabase(projectRoot: string, opts?: BuildOptions): SqliteD } export function closeDiskCacheDatabase(projectRoot: string, opts?: BuildOptions): void { - clearMemoryCache(); + clearMemoryCacheForProject(projectRoot); const dbPath = diskCacheDatabasePath(projectRoot, opts); const db = diskCacheDatabases.get(dbPath); if (!db) return; diff --git a/src/indexer/incremental-plan.ts b/src/indexer/incremental-plan.ts index 21b5ebb2..4782c16c 100644 --- a/src/indexer/incremental-plan.ts +++ b/src/indexer/incremental-plan.ts @@ -95,4 +95,3 @@ export function collectTrackedFileDependents( return dependents; } - diff --git a/src/indexer/navigation.ts b/src/indexer/navigation.ts index a7a4d779..939e1614 100644 --- a/src/indexer/navigation.ts +++ b/src/indexer/navigation.ts @@ -396,7 +396,7 @@ export async function findReferences( } if (shouldScanVerifiedReferences(def, phpQualifiedNames, parsedContext)) { - for (const fileId of index.byFile.keys()) { + for (const fileId of Array.from(index.byFile.keys()).sort((left, right) => left.localeCompare(right))) { if (hasReachedMaxReferences()) break; const filter = index.bloomFilters?.get(fileId); if (filter && !filter.mightContain(def.localName)) continue; diff --git a/src/sqlite/schema.ts b/src/sqlite/schema.ts index f3ba7367..1d447c55 100644 --- a/src/sqlite/schema.ts +++ b/src/sqlite/schema.ts @@ -240,6 +240,11 @@ export const ensureSchema = (db: SqliteDatabase) => { `); const currentVersion = readGraphSchemaVersion(db); + if (currentVersion > SQLITE_SCHEMA_VERSION) { + throw new Error( + `Unsupported codegraph SQLite schema version ${currentVersion}; this version supports up to ${SQLITE_SCHEMA_VERSION}.`, + ); + } migrateGraphSchema(db, currentVersion); ensureGraphIndexes(db); writeGraphSchemaVersion(db, SQLITE_SCHEMA_VERSION); diff --git a/src/util/lruMap.ts b/src/util/lruMap.ts index 2b3f59b4..818e95e7 100644 --- a/src/util/lruMap.ts +++ b/src/util/lruMap.ts @@ -1,8 +1,8 @@ export function lruMapGet(map: Map, key: K): V | undefined { - const value = map.get(key); - if (value === undefined) { + if (!map.has(key)) { return undefined; } + const value = map.get(key) as V; map.delete(key); map.set(key, value); return value; @@ -12,9 +12,9 @@ export function lruMapSet(map: Map, key: K, value: V, maxEntries: nu if (map.has(key)) { map.delete(key); } else if (map.size >= maxEntries) { - const oldest = map.keys().next().value; - if (oldest !== undefined) { - map.delete(oldest); + const oldest = map.keys().next(); + if (!oldest.done) { + map.delete(oldest.value); } } map.set(key, value); diff --git a/tests/agent-explain.test.ts b/tests/agent-explain.test.ts index 0a9d3361..71cd1217 100644 --- a/tests/agent-explain.test.ts +++ b/tests/agent-explain.test.ts @@ -44,7 +44,6 @@ async function mkDuplicateRepo(): Promise { return root; } - async function mkManyReferenceRepo(): Promise { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cg-agent-explain-refs-")); await fs.writeFile(path.join(root, "target.ts"), "export function sharedTarget() { return 1; }\n"); @@ -265,7 +264,6 @@ describe("agent explain", () => { expect(explanation.omittedCounts.snippets).toBeGreaterThan(0); }); - it("returns references sorted by file before truncation", async () => { const root = await mkManyReferenceRepo(); const explanation = await explainCodegraphTarget({ root, target: "sharedTarget", maxReferences: 2 }); diff --git a/tests/cli-options-validation.test.ts b/tests/cli-options-validation.test.ts index e5b3399d..0bac43dd 100644 --- a/tests/cli-options-validation.test.ts +++ b/tests/cli-options-validation.test.ts @@ -38,9 +38,7 @@ describe("CLI enum option parsers", () => { describe("parseCliArgs value-option guard", () => { it("does not consume a following flag as a value", () => { - expect(() => parseCliArgs("graph", ["--threads", "--json"])).toThrow( - /Missing value for --threads option/, - ); + expect(() => parseCliArgs("graph", ["--threads", "--json"])).toThrow(/Missing value for --threads option/); }); it("allows negative decimal values for integer options", () => { diff --git a/tests/git-revision-safety.test.ts b/tests/git-revision-safety.test.ts index fa32fd9a..ef97a974 100644 --- a/tests/git-revision-safety.test.ts +++ b/tests/git-revision-safety.test.ts @@ -3,9 +3,7 @@ import { assertSafeRevision, gitDiffArgs, getUnifiedDiff, listChangedFiles } fro describe("git revision safety", () => { it("rejects revisions that start with -", () => { - expect(() => assertSafeRevision("--output=/tmp/evil", "base")).toThrow( - /must not start with "-"/, - ); + expect(() => assertSafeRevision("--output=/tmp/evil", "base")).toThrow(/must not start with "-"/); expect(() => assertSafeRevision("", "head")).toThrow(/must not be empty/); }); diff --git a/tests/graph-queries.test.ts b/tests/graph-queries.test.ts index 3c2fa309..56dc8ec1 100644 --- a/tests/graph-queries.test.ts +++ b/tests/graph-queries.test.ts @@ -113,6 +113,30 @@ describe("graph queries", () => { expect(edgeIterations).toBe(1); }); + it("indexes cycle edges once when reporting many SCCs", () => { + const cycleNodes = new Set(); + const cycleEdges: Edge[] = []; + for (let index = 0; index < 20; index += 1) { + const a = `${root}/cycle-${index}-a.ts`; + const b = `${root}/cycle-${index}-b.ts`; + cycleNodes.add(a); + cycleNodes.add(b); + cycleEdges.push({ from: a, to: { type: "file", path: b }, raw: "./b" }); + cycleEdges.push({ from: b, to: { type: "file", path: a }, raw: "./a" }); + } + + let edgeIterations = 0; + const trackedEdges: Edge[] = [...cycleEdges]; + const originalIterator = trackedEdges[Symbol.iterator].bind(trackedEdges); + trackedEdges[Symbol.iterator] = function (): ArrayIterator { + edgeIterations += 1; + return originalIterator(); + }; + + expect(findDetailedCycles({ nodes: cycleNodes, edges: trackedEdges })).toHaveLength(20); + expect(edgeIterations).toBe(1); + }); + it("should find cycles", () => { const cycles = findCycles(graph); expect(cycles.length).toBe(1); diff --git a/tests/impact-call-compatibility/parse-resilience.test.ts b/tests/impact-call-compatibility/parse-resilience.test.ts index 2797e18d..38096d85 100644 --- a/tests/impact-call-compatibility/parse-resilience.test.ts +++ b/tests/impact-call-compatibility/parse-resilience.test.ts @@ -23,7 +23,6 @@ function rangeFor(source: string, needle: string): Range { }; } - vi.mock( "../../src/indexer/navigation.js", async (importOriginal: () => Promise) => { @@ -236,6 +235,4 @@ describe("call compatibility parse resilience", () => { expect(diagnostics.callCompatibility?.skippedByReason["parse-failed"]).toBe(1); vi.mocked(findReferences).mockReset(); }); - }); - diff --git a/tests/impact-transitive-order.test.ts b/tests/impact-transitive-order.test.ts index f9a54498..cf1d3b46 100644 --- a/tests/impact-transitive-order.test.ts +++ b/tests/impact-transitive-order.test.ts @@ -18,10 +18,7 @@ function runDiamondAnalysis(edgeOrder: Edge[]): Map { const root = "/proj/root.ts"; const mid = "/proj/mid.ts"; const leaf = "/proj/leaf.ts"; - const edges: Edge[] = [ - { from: mid, to: { type: "file", path: root }, raw: "./root" }, - ...edgeOrder, - ]; + const edges: Edge[] = [{ from: mid, to: { type: "file", path: root }, raw: "./root" }, ...edgeOrder]; const reverseDeps = buildReverseDeps(edges); const impacted = new Map([ [ diff --git a/tests/include-roots.test.ts b/tests/include-roots.test.ts index c40c6514..2d6e1535 100644 --- a/tests/include-roots.test.ts +++ b/tests/include-roots.test.ts @@ -19,9 +19,7 @@ describe("includeRoots helpers", () => { it("restricts graphs to include roots", () => { const graph: Graph = { nodes: new Set(["/proj/src/a.ts", "/proj/lib/b.ts"]), - edges: [ - { from: "/proj/src/a.ts", to: { type: "file", path: "/proj/lib/b.ts" }, raw: "../lib/b" }, - ], + edges: [{ from: "/proj/src/a.ts", to: { type: "file", path: "/proj/lib/b.ts" }, raw: "../lib/b" }], }; const scoped = restrictGraphToIncludeRoots(graph, ["src"], (file) => file.replace("/proj/", "")); expect([...scoped.nodes]).toEqual(["src/a.ts"]); diff --git a/tests/incremental-plan.test.ts b/tests/incremental-plan.test.ts index eadd3fc6..aa490f2b 100644 --- a/tests/incremental-plan.test.ts +++ b/tests/incremental-plan.test.ts @@ -1,8 +1,5 @@ import { describe, expect, it } from "vitest"; -import { - collectDeletedTrackedFileDependents, - collectTrackedFileDependents, -} from "../src/indexer/incremental-plan.js"; +import { collectDeletedTrackedFileDependents, collectTrackedFileDependents } from "../src/indexer/incremental-plan.js"; import type { ManifestFileEntry } from "../src/indexer/build-cache.js"; import type { Edge } from "../src/types.js"; @@ -23,9 +20,7 @@ describe("incremental-plan dependents", () => { }; const changed = new Set(["/proj/c.ts"]); - expect(collectTrackedFileDependents(trackedEntries, changed)).toEqual( - new Set(["/proj/b.ts", "/proj/a.ts"]), - ); + expect(collectTrackedFileDependents(trackedEntries, changed)).toEqual(new Set(["/proj/b.ts", "/proj/a.ts"])); }); it("still collects dependents of deleted tracked files", () => { diff --git a/tests/lazy-symbols.test.ts b/tests/lazy-symbols.test.ts index 8ba87374..b2a6a837 100644 --- a/tests/lazy-symbols.test.ts +++ b/tests/lazy-symbols.test.ts @@ -477,7 +477,6 @@ describe("LazyProjectIndex", () => { expect(index.getFiles().length).toBeLessThan(300); expect(index.getFiles().length).toBeGreaterThan(0); }); - }); describe("createSymbolLoader", () => { diff --git a/tests/lru-map.test.ts b/tests/lru-map.test.ts index 54826595..f154c759 100644 --- a/tests/lru-map.test.ts +++ b/tests/lru-map.test.ts @@ -24,4 +24,27 @@ describe("lruMap", () => { expect(map.get("a")).toBe(1); expect(map.get("c")).toBe(3); }); + + it("refreshes undefined-valued entries on get", () => { + const map = new Map(); + lruMapSet(map, "a", undefined, 2); + lruMapSet(map, "b", 2, 2); + expect(lruMapGet(map, "a")).toBeUndefined(); + lruMapSet(map, "c", 3, 2); + + expect(map.has("a")).toBe(true); + expect(map.has("b")).toBe(false); + expect(map.get("c")).toBe(3); + }); + + it("evicts an undefined key when it is the oldest entry", () => { + const map = new Map(); + lruMapSet(map, undefined, 1, 2); + lruMapSet(map, "b", 2, 2); + lruMapSet(map, "c", 3, 2); + + expect(map.has(undefined)).toBe(false); + expect(map.get("b")).toBe(2); + expect(map.get("c")).toBe(3); + }); }); diff --git a/tests/module-cache-lru.test.ts b/tests/module-cache-lru.test.ts index ce3e5a0f..7bd48fd2 100644 --- a/tests/module-cache-lru.test.ts +++ b/tests/module-cache-lru.test.ts @@ -3,6 +3,7 @@ import path from "node:path"; import os from "node:os"; import { clearMemoryCache, + closeDiskCacheDatabase, tryLoadFromCache, writeToCache, } from "../src/indexer/build-cache/module-cache.js"; @@ -13,7 +14,9 @@ function moduleFor(file: string, label: string): ModuleIndex { file, exports: [], imports: [], - locals: [{ file, localName: label, kind: 1, range: { start: { line: 1, column: 0 }, end: { line: 1, column: 1 } } }], + locals: [ + { file, localName: label, kind: 1, range: { start: { line: 1, column: 0 }, end: { line: 1, column: 1 } } }, + ], }; } @@ -37,4 +40,19 @@ describe("module memory cache bounds", () => { expect(tryLoadFromCache(rootA, "/files/a-5000.ts", sig, { cache: "memory" })).toBeNull(); expect(tryLoadFromCache(rootB, "/files/b.ts", sig, { cache: "memory" })).toBeNull(); }); + + it("clears only the closed project from the memory cache", () => { + const rootA = path.join(os.tmpdir(), "dg-cache-close-a"); + const rootB = path.join(os.tmpdir(), "dg-cache-close-b"); + const sig = "sig-2"; + + writeToCache(rootA, "/files/a.ts", sig, moduleFor("/files/a.ts", "a"), { cache: "memory" }); + writeToCache(rootB, "/files/b.ts", sig, moduleFor("/files/b.ts", "b"), { cache: "memory" }); + + closeDiskCacheDatabase(rootA); + + expect(tryLoadFromCache(rootA, "/files/a.ts", sig, { cache: "memory" })).toBeNull(); + expect(tryLoadFromCache(rootB, "/files/b.ts", sig, { cache: "memory" })?.locals[0]?.localName).toBe("b"); + clearMemoryCache(); + }); }); diff --git a/tests/sqlite.test.ts b/tests/sqlite.test.ts index 9c434af8..99524f1e 100644 --- a/tests/sqlite.test.ts +++ b/tests/sqlite.test.ts @@ -247,6 +247,25 @@ export function run() { helper(); new Widget(); } db.close(); }); + it("rejects future schema_version databases without downgrading them", async () => { + const root = await mkTmpDir("dg-sqlite-future-version-"); + const dbPath = path.join(root, "graph.sqlite"); + const db = new SqliteDatabase(dbPath); + const { ensureSchema, readGraphSchemaVersion, SQLITE_SCHEMA_VERSION } = await import("../src/sqlite/schema.js"); + db.exec(` + CREATE TABLE IF NOT EXISTS graph_metadata ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL + ); + INSERT INTO graph_metadata (key, value) VALUES ('schema_version', '999'); + `); + + expect(() => ensureSchema(db)).toThrow(`Unsupported codegraph SQLite schema version 999`); + expect(readGraphSchemaVersion(db)).toBe(999); + expect(SQLITE_SCHEMA_VERSION).toBeLessThan(999); + db.close(); + }); + it("updates changed files incrementally", async () => { const root = await mkTmpDir("dg-sqlite-update-"); const base = ` From 66f6744541fb1454c49e8a224f6c1f85e01bf225 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 16:15:54 -0400 Subject: [PATCH 5/9] fix: normalize scoped graph edges --- src/util/includeRoots.ts | 16 ++++++++++++---- tests/include-roots.test.ts | 19 +++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/util/includeRoots.ts b/src/util/includeRoots.ts index 6fa901c2..9fc1a866 100644 --- a/src/util/includeRoots.ts +++ b/src/util/includeRoots.ts @@ -39,11 +39,19 @@ export function restrictGraphToIncludeRoots( nodes.add(normalizedFile); } } - const edges = graph.edges.filter((edge) => { - if (!nodes.has(normalizeFile(edge.from))) { - return false; + const edges = graph.edges.flatMap((edge) => { + const from = normalizeFile(edge.from); + if (!nodes.has(from)) { + return []; } - return edge.to.type === "external" || nodes.has(normalizeFile(edge.to.path)); + if (edge.to.type === "external") { + return [{ ...edge, from }]; + } + const toPath = normalizeFile(edge.to.path); + if (!nodes.has(toPath)) { + return []; + } + return [{ ...edge, from, to: { type: "file" as const, path: toPath } }]; }); return { nodes, diff --git a/tests/include-roots.test.ts b/tests/include-roots.test.ts index 2d6e1535..4d890dc6 100644 --- a/tests/include-roots.test.ts +++ b/tests/include-roots.test.ts @@ -25,4 +25,23 @@ describe("includeRoots helpers", () => { expect([...scoped.nodes]).toEqual(["src/a.ts"]); expect(scoped.edges).toHaveLength(0); }); + + it("normalizes retained edge endpoints with scoped nodes", () => { + const graph: Graph = { + nodes: new Set(["/proj/src/a.ts", "/proj/src/b.ts", "/proj/lib/c.ts"]), + edges: [ + { from: "/proj/src/a.ts", to: { type: "file", path: "/proj/src/b.ts" }, raw: "./b" }, + { from: "/proj/src/b.ts", to: { type: "external", name: "react" }, raw: "react" }, + { from: "/proj/src/a.ts", to: { type: "file", path: "/proj/lib/c.ts" }, raw: "../lib/c" }, + ], + }; + + const scoped = restrictGraphToIncludeRoots(graph, ["src"], (file) => file.replace("/proj/", "")); + + expect([...scoped.nodes]).toEqual(["src/a.ts", "src/b.ts"]); + expect(scoped.edges).toEqual([ + { from: "src/a.ts", to: { type: "file", path: "src/b.ts" }, raw: "./b" }, + { from: "src/b.ts", to: { type: "external", name: "react" }, raw: "react" }, + ]); + }); }); From 4402af0f4fb9acf4c3441936823a960a7e619d12 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 16:35:49 -0400 Subject: [PATCH 6/9] fix: propagate stronger transitive paths --- src/impact/transitive.ts | 27 +++++++++++++++---------- tests/impact-transitive-order.test.ts | 29 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/impact/transitive.ts b/src/impact/transitive.ts index 400a678d..0c5901d7 100644 --- a/src/impact/transitive.ts +++ b/src/impact/transitive.ts @@ -154,14 +154,10 @@ export function analyzeTransitiveImpact( const nextDepth = depth + 1; const knownDepth = bestDepth.get(dependentFile); - if (knownDepth !== undefined && nextDepth > knownDepth) { - continue; - } - const existing = impacted.get(dependentFile); - const isUpgrade = knownDepth === undefined || nextDepth < knownDepth; const reasons = [...(existing?.reasons ?? [])]; - if (!reasons.includes(reason)) { + const addsReason = !reasons.includes(reason); + if (addsReason) { reasons.push(reason); } @@ -171,15 +167,22 @@ export function analyzeTransitiveImpact( 0.2, Math.min(1, upstreamConfidence * (edge.typeOnly ? 0.75 : 0.85) * Math.pow(0.95, depth)), ); + const existingSeverity = existing?.severity ?? 0; + const existingConfidence = existing?.confidence ?? 0; + const improvesDepth = knownDepth === undefined || nextDepth < knownDepth; + const improvesStrength = severity > existingSeverity || nextConfidence > existingConfidence || addsReason; + if (!improvesDepth && !improvesStrength) { + continue; + } const fanIn = reverseDeps.get(dependentFile)?.length || 0; - const resolvedDepth = isUpgrade ? nextDepth : Math.min(existing?.depth ?? nextDepth, nextDepth); + const resolvedDepth = improvesDepth ? nextDepth : Math.min(existing?.depth ?? nextDepth, nextDepth); const transitiveItem: ImpactItem = { file: dependentFile, symbols: existing?.symbols || [], reasons, - severity: Math.max(existing?.severity || 0, severity), + severity: Math.max(existingSeverity, severity), depth: resolvedDepth, explain: { ...existing?.explain, @@ -187,7 +190,7 @@ export function analyzeTransitiveImpact( depth: resolvedDepth, ...(fanIn > 0 && { fanIn }), }, - confidence: Math.max(existing?.confidence ?? 0, nextConfidence), + confidence: Math.max(existingConfidence, nextConfidence), }; if (edge.typeOnly !== undefined) { @@ -200,8 +203,10 @@ export function analyzeTransitiveImpact( impacted.set(dependentFile, transitiveItem); emitImpactItem?.(transitiveItem, "partial"); - if (isUpgrade) { - bestDepth.set(dependentFile, nextDepth); + if (improvesDepth || improvesStrength) { + if (improvesDepth) { + bestDepth.set(dependentFile, nextDepth); + } queue.push({ file: dependentFile, depth: nextDepth, diff --git a/tests/impact-transitive-order.test.ts b/tests/impact-transitive-order.test.ts index cf1d3b46..7b4f4c3e 100644 --- a/tests/impact-transitive-order.test.ts +++ b/tests/impact-transitive-order.test.ts @@ -54,6 +54,35 @@ describe("analyzeTransitiveImpact order independence", () => { expect(directFirst.get(leaf)?.severity).toBe(viaMidFirst.get(leaf)?.severity); }); + it("keeps the stronger path when a deeper value edge beats a shallow type-only edge", () => { + const root = "/proj/root.ts"; + const mid = "/proj/mid.ts"; + const leaf = "/proj/leaf.ts"; + const edges: Edge[] = [ + { from: leaf, to: { type: "file", path: root }, raw: "./root", typeOnly: true }, + { from: mid, to: { type: "file", path: root }, raw: "./root" }, + { from: leaf, to: { type: "file", path: mid }, raw: "./mid" }, + ]; + const impacted = new Map([ + [ + root, + { + file: root, + symbols: [], + reasons: ["directRef"], + severity: 0.9, + depth: 0, + confidence: 0.9, + }, + ], + ]); + + analyzeTransitiveImpact(impacted, 5, {}, () => false, buildReverseDeps(edges)); + + expect(impacted.get(leaf)?.depth).toBe(1); + expect(impacted.get(leaf)?.severity).toBeGreaterThan(0.14); + }); + it("does not mutate shared reasons arrays across updates", () => { const root = "/proj/root.ts"; const child = "/proj/child.ts"; From 71eb4298f50b241ec7ce61f1def3957efab546b6 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 16:43:51 -0400 Subject: [PATCH 7/9] fix: handle review edge cases --- src/agent/explain.ts | 6 +++++- src/sqlite/schema.ts | 13 +++++++------ tests/agent-explain.test.ts | 15 +++++++++++++++ tests/sqlite.test.ts | 5 +++++ 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/agent/explain.ts b/src/agent/explain.ts index 26fe859c..d4099d30 100644 --- a/src/agent/explain.ts +++ b/src/agent/explain.ts @@ -732,7 +732,11 @@ async function collectReferenceContext( referenceLimit: number, snippetLimit: number, ): Promise { - const collectionLimit = Math.max(referenceLimit, snippetLimit) * AGENT_EXPLAIN_REFERENCE_COLLECTION_MULTIPLIER; + const displayLimit = Math.max(referenceLimit, snippetLimit); + if (displayLimit <= 0) { + return emptyReferenceContext(); + } + const collectionLimit = displayLimit * AGENT_EXPLAIN_REFERENCE_COLLECTION_MULTIPLIER; const result = await findReferences(snapshot.index, { def }, { context: "line", maxReferences: collectionLimit }); if (result.status !== "ok") return emptyReferenceContext(); diff --git a/src/sqlite/schema.ts b/src/sqlite/schema.ts index 1d447c55..b38c5226 100644 --- a/src/sqlite/schema.ts +++ b/src/sqlite/schema.ts @@ -179,6 +179,13 @@ const ensureGraphIndexes = (db: SqliteDatabase): boolean => { }; export const ensureSchema = (db: SqliteDatabase) => { + const currentVersion = readGraphSchemaVersion(db); + if (currentVersion > SQLITE_SCHEMA_VERSION) { + throw new Error( + `Unsupported codegraph SQLite schema version ${currentVersion}; this version supports up to ${SQLITE_SCHEMA_VERSION}.`, + ); + } + db.pragma("journal_mode = WAL"); db.pragma("synchronous = NORMAL"); db.pragma("temp_store = MEMORY"); @@ -239,12 +246,6 @@ export const ensureSchema = (db: SqliteDatabase) => { ); `); - const currentVersion = readGraphSchemaVersion(db); - if (currentVersion > SQLITE_SCHEMA_VERSION) { - throw new Error( - `Unsupported codegraph SQLite schema version ${currentVersion}; this version supports up to ${SQLITE_SCHEMA_VERSION}.`, - ); - } migrateGraphSchema(db, currentVersion); ensureGraphIndexes(db); writeGraphSchemaVersion(db, SQLITE_SCHEMA_VERSION); diff --git a/tests/agent-explain.test.ts b/tests/agent-explain.test.ts index 71cd1217..9b87dfbb 100644 --- a/tests/agent-explain.test.ts +++ b/tests/agent-explain.test.ts @@ -273,6 +273,21 @@ describe("agent explain", () => { expect(explanation.references.map((reference) => reference.file)).toEqual(["ref-00.ts", "ref-01.ts"]); }); + it("skips reference collection when reference and snippet limits are zero", async () => { + const root = await mkRepo(); + const explanation = await explainCodegraphTarget({ + root, + target: "validateUser", + maxReferences: 0, + maxSnippets: 0, + }); + + expect(explanation.references).toEqual([]); + expect(explanation.snippets).toEqual([]); + expect(explanation.limits.references).toBe(0); + expect(explanation.limits.snippets).toBe(0); + }); + it("bounds file symbols and reports omitted counts", async () => { const root = await mkRepo(); await fs.writeFile( diff --git a/tests/sqlite.test.ts b/tests/sqlite.test.ts index 99524f1e..1c698d95 100644 --- a/tests/sqlite.test.ts +++ b/tests/sqlite.test.ts @@ -263,6 +263,11 @@ export function run() { helper(); new Widget(); } expect(() => ensureSchema(db)).toThrow(`Unsupported codegraph SQLite schema version 999`); expect(readGraphSchemaVersion(db)).toBe(999); expect(SQLITE_SCHEMA_VERSION).toBeLessThan(999); + const tables = db + .prepare("SELECT name FROM sqlite_master WHERE type='table' ORDER BY name;") + .all() + .map((row) => String((row as { name?: unknown }).name)); + expect(tables).toEqual(["graph_metadata"]); db.close(); }); From 252e90116dfc21776bcdf902d63e8b3612160348 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 17:29:57 -0400 Subject: [PATCH 8/9] fix: evict stale memory cache entries --- src/indexer/build-cache/module-cache.ts | 13 +++++++++---- tests/module-cache-lru.test.ts | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/indexer/build-cache/module-cache.ts b/src/indexer/build-cache/module-cache.ts index 66d9570d..e3a513f9 100644 --- a/src/indexer/build-cache/module-cache.ts +++ b/src/indexer/build-cache/module-cache.ts @@ -246,10 +246,15 @@ export function tryLoadFromCache( const cacheReport = initCacheReport(report, mode); const cacheEnabled = mode !== "off"; if (mode === "memory") { - const entry = lruMapGet(memoryCache, memoryCacheKey(projectRoot, file)); - if (entry && entry.sig === sig) { - if (cacheEnabled && cacheReport) cacheReport.hits += 1; - return entry.mod; + const key = memoryCacheKey(projectRoot, file); + const entry = memoryCache.get(key); + if (entry) { + if (entry.sig === sig) { + lruMapGet(memoryCache, key); + if (cacheEnabled && cacheReport) cacheReport.hits += 1; + return entry.mod; + } + memoryCache.delete(key); } if (cacheEnabled && cacheReport) cacheReport.misses += 1; return null; diff --git a/tests/module-cache-lru.test.ts b/tests/module-cache-lru.test.ts index 7bd48fd2..3b964b52 100644 --- a/tests/module-cache-lru.test.ts +++ b/tests/module-cache-lru.test.ts @@ -41,6 +41,24 @@ describe("module memory cache bounds", () => { expect(tryLoadFromCache(rootB, "/files/b.ts", sig, { cache: "memory" })).toBeNull(); }); + it("deletes stale signature mismatches instead of refreshing them", () => { + const root = path.join(os.tmpdir(), "dg-cache-stale-signature"); + clearMemoryCache(); + + writeToCache(root, "/files/stale.ts", "old-sig", moduleFor("/files/stale.ts", "stale"), { cache: "memory" }); + for (let i = 0; i < 4999; i += 1) { + const file = `/files/current-${i}.ts`; + writeToCache(root, file, "sig", moduleFor(file, `current-${i}`), { cache: "memory" }); + } + + expect(tryLoadFromCache(root, "/files/stale.ts", "new-sig", { cache: "memory" })).toBeNull(); + writeToCache(root, "/files/extra.ts", "sig", moduleFor("/files/extra.ts", "extra"), { cache: "memory" }); + + expect(tryLoadFromCache(root, "/files/stale.ts", "old-sig", { cache: "memory" })).toBeNull(); + expect(tryLoadFromCache(root, "/files/extra.ts", "sig", { cache: "memory" })?.locals[0]?.localName).toBe("extra"); + clearMemoryCache(); + }); + it("clears only the closed project from the memory cache", () => { const rootA = path.join(os.tmpdir(), "dg-cache-close-a"); const rootB = path.join(os.tmpdir(), "dg-cache-close-b"); From 0ea5575f06c8f56e738d1bb04552ede6b3068784 Mon Sep 17 00:00:00 2001 From: Luke Zehrung Date: Sun, 21 Jun 2026 17:47:07 -0400 Subject: [PATCH 9/9] fix: handle dash-prefixed CLI values --- src/cli/context.ts | 12 +++++------- src/util/lruMap.ts | 2 +- tests/cli-options-validation.test.ts | 11 +++++++++++ 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/cli/context.ts b/src/cli/context.ts index bb9069be..90277f7a 100644 --- a/src/cli/context.ts +++ b/src/cli/context.ts @@ -371,14 +371,12 @@ export type CommandReport = { review?: ReviewBuildReport; }; +function isSupportedShortFlagToken(token: string): boolean { + return token === "-h" || token === "-v" || token === "-o"; +} + function isCliOptionValueToken(token: string): boolean { - if (token.startsWith("--")) { - return false; - } - if (/^-?\d+$/.test(token)) { - return true; - } - return !token.startsWith("-"); + return !token.startsWith("--") && !isSupportedShortFlagToken(token); } export function parseCliArgs(command: string, tokens: string[]): ParsedCliArgs { diff --git a/src/util/lruMap.ts b/src/util/lruMap.ts index 818e95e7..ea4f5507 100644 --- a/src/util/lruMap.ts +++ b/src/util/lruMap.ts @@ -2,7 +2,7 @@ export function lruMapGet(map: Map, key: K): V | undefined { if (!map.has(key)) { return undefined; } - const value = map.get(key) as V; + const value = map.get(key)!; map.delete(key); map.set(key, value); return value; diff --git a/tests/cli-options-validation.test.ts b/tests/cli-options-validation.test.ts index 0bac43dd..01905056 100644 --- a/tests/cli-options-validation.test.ts +++ b/tests/cli-options-validation.test.ts @@ -41,6 +41,17 @@ describe("parseCliArgs value-option guard", () => { expect(() => parseCliArgs("graph", ["--threads", "--json"])).toThrow(/Missing value for --threads option/); }); + it("allows dash-prefixed option values that are not supported short flags", () => { + const parsed = parseCliArgs("graph", ["--ignore-glob", "-generated/**", "--pattern", "-bar"]); + expect(parsed.options.get("--ignore-glob")).toEqual(["-generated/**"]); + expect(parsed.options.get("--pattern")).toEqual(["-bar"]); + }); + + it("still rejects supported short flags as missing long-option values", () => { + expect(() => parseCliArgs("graph", ["--threads", "-v"])).toThrow(/Missing value for --threads option/); + expect(() => parseCliArgs("graph", ["--output", "-o"])).toThrow(/Missing value for --output option/); + }); + it("allows negative decimal values for integer options", () => { const parsed = parseCliArgs("hotspots", ["--limit", "-1"]); expect(parsed.options.get("--limit")).toEqual(["-1"]);