fix: audit remediation priorities 3-9#137
Conversation
- 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.
- 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.
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.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const result = await findReferences(snapshot.index, { def }, { context: "line" }); | ||
| if (result.status !== "ok") return emptyReferenceContext(); | ||
|
|
||
| const references = result.references |
There was a problem hiding this comment.
WARNING: Unbounded findReferences call may cause performance issues for widely-referenced symbols.
Removing maxReferences means findReferences can return an arbitrarily large result set before the local sort-and-truncate logic runs. The previous code bounded collection at Math.max(referenceLimit, snippetLimit) + 1, which prevented excessive memory use for symbols with thousands of references. Consider adding a generous finite cap (e.g., maxReferences: Math.max(referenceLimit, snippetLimit) * 10) so the sort-before-truncate fix remains safe at scale.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| export function closeDiskCacheDatabase(projectRoot: string, opts?: BuildOptions): void { | ||
| clearMemoryCache(); |
There was a problem hiding this comment.
WARNING: closeDiskCacheDatabase clears the global memoryCache, evicting entries for ALL projects.
memoryCache is a module-level singleton keyed by ${projectRoot}::${file}. Calling clearMemoryCache() inside closeDiskCacheDatabase(projectRoot) closes one project's disk cache but invalidates memory cache entries for every other project too. In multi-project usage this is an overly broad side effect. Consider clearing only entries whose key starts with the matching projectRoot, or moving the global clear to an explicit teardown function.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -0,0 +1,21 @@ | |||
| export function lruMapGet<K, V>(map: Map<K, V>, key: K): V | undefined { | |||
| const value = map.get(key); | |||
| if (value === undefined) { | |||
There was a problem hiding this comment.
SUGGESTION: lruMapGet skips LRU refresh when a Map entry's value is literally undefined.
If a caller ever stores undefined as a legitimate value, lruMapGet returns early without re-inserting the key at the end of the Map iteration order. That entry would incorrectly be evicted first despite being the most recently accessed. Current callers don't hit this, but the helper should handle it correctly.
| if (value === undefined) { | |
| if (!map.has(key)) { | |
| return undefined; | |
| } |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const result = await findReferences(snapshot.index, { def }, { context: "line" }); | ||
| if (result.status !== "ok") return emptyReferenceContext(); | ||
|
|
||
| const references = result.references |
There was a problem hiding this comment.
WARNING: Unbounded findReferences call may cause performance issues for widely-referenced symbols.
Removing maxReferences means findReferences can return an arbitrarily large result set before the local sort-and-truncate logic runs. The previous code bounded collection at Math.max(referenceLimit, snippetLimit) + 1, which prevented excessive memory use for symbols with thousands of references. Consider adding a generous finite cap (e.g., maxReferences: Math.max(referenceLimit, snippetLimit) * 10) so the sort-before-truncate fix remains safe at scale.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| export function closeDiskCacheDatabase(projectRoot: string, opts?: BuildOptions): void { | ||
| clearMemoryCache(); |
There was a problem hiding this comment.
WARNING: closeDiskCacheDatabase clears the global memoryCache, evicting entries for ALL projects.
memoryCache is a module-level singleton keyed by ${projectRoot}::${file}. Calling clearMemoryCache() inside closeDiskCacheDatabase(projectRoot) closes one project's disk cache but invalidates memory cache entries for every other project too. In multi-project usage this is an overly broad side effect. Consider clearing only entries whose key starts with the matching projectRoot, or moving the global clear to an explicit teardown function.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -0,0 +1,21 @@ | |||
| export function lruMapGet<K, V>(map: Map<K, V>, key: K): V | undefined { | |||
| const value = map.get(key); | |||
| if (value === undefined) { | |||
There was a problem hiding this comment.
SUGGESTION: lruMapGet skips LRU refresh when a Map entry's value is literally undefined.
If a caller ever stores undefined as a legitimate value, lruMapGet returns early without re-inserting the key at the end of the Map iteration order. That entry would incorrectly be evicted first despite being the most recently accessed. Current callers don't hit this, but the helper should handle it correctly.
| if (value === undefined) { | |
| if (!map.has(key)) { | |
| return undefined; | |
| } |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge 2 previous issues were resolved in this revision:
Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summaries (6 snapshots, latest commit 252e901)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 252e901)Status: 3 Issues Found | Recommendation: Address before merge 1 previous issue was resolved in this revision:
Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Previous review (commit 71eb429)Status: 3 Issues Found | Recommendation: Address before merge 2 previous issues were resolved in this revision:
Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Previous review (commit 4402af0)Status: No Issues Found | Recommendation: Merge The previous Files Reviewed (2 files)
Previous review (commit 66f6744)Status: No Issues Found | Recommendation: Merge The previous Files Reviewed (2 files)
Previous review (commit 4a6812b)Status: No Issues Found | Recommendation: Merge All 3 issues from the previous review have been addressed in the latest commits:
Files Reviewed (23 files)
Previous review (commit be0b2c7)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (20 files)
Reviewed by kimi-k2.6-20260420 · Input: 23.1K · Output: 13.6K · Cached: 67.8K |
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (20 files)
|
There was a problem hiding this comment.
Pull request overview
Completes technical-audit remediation items 3–9 by tightening correctness/determinism (impact BFS + agent outputs), bounding in-memory caches, introducing explicit SQLite schema versioning/migration tests, improving CLI argument validation, and addressing performance issues in cycle detection/adjacency building with targeted refactors/shared helpers.
Changes:
- Adds explicit SQLite
schema_versionreading + v1→v2 migration path with regression tests. - Introduces shared LRU map helper and applies caps/clearing to previously unbounded caches (resolution/module/lazy-symbols).
- Improves determinism/perf: order-stable reference iteration, sort-before-truncate for agent outputs, iterative Tarjan SCC detection, and memoized graph adjacency.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/sqlite.test.ts | Adds regression coverage for SQLite schema migration + rejecting future schema versions. |
| tests/resolution-cache-lru.test.ts | Verifies resolveSpecifier caching behavior and explicit cache clearing. |
| tests/module-cache-lru.test.ts | Validates module memory-cache eviction and project-scoped clearing on disk-cache close. |
| tests/lru-map.test.ts | Adds unit tests for lruMapGet/lruMapSet eviction and refresh behavior. |
| tests/lazy-symbols.test.ts | Adds coverage for LazyProjectIndex.clear() and trimming unloaded entries. |
| tests/incremental-plan.test.ts | Formatting-only adjustments to existing dependent-collection tests. |
| tests/include-roots.test.ts | Adds tests for include-roots normalization/membership and graph restriction. |
| tests/impact-transitive-order.test.ts | Adds order-independence and copy-on-write reasons regression tests for transitive impact. |
| tests/impact-call-compatibility/parse-resilience.test.ts | Removes stray whitespace lines (no behavior change). |
| tests/graph-queries.test.ts | Updates edge-iteration expectations and adds SCC-heavy cycle-reporting regression. |
| tests/graph-cycles-iterative.test.ts | Adds long-chain cycle test to guard against recursive stack overflow. |
| tests/graph-adjacency-cache.test.ts | Adds test ensuring graphAdjacencyFor memoizes per-graph adjacency. |
| tests/git-revision-safety.test.ts | Minor assertion formatting adjustments. |
| tests/cli-options-validation.test.ts | Adds tests for strict integer parsing, enum validation, and value-option guarding. |
| tests/agent-tools-order.test.ts | Verifies dependency results are sorted before truncation in agent tool output. |
| tests/agent-explain.test.ts | Adds regression test ensuring references are sorted before truncation in explain output. |
| src/util/resolution.ts | Caps resolveSpecifier cache via LRU helper. |
| src/util/lruMap.ts | Introduces shared LRU get/set helpers for capped Map caches. |
| src/util/lazySymbols.ts | Adds clear() and trims unloaded module entries; clamps maxCached to non-negative. |
| src/util/includeRoots.ts | Introduces shared include-roots helpers and graph restriction utility. |
| src/sqlite/schema.ts | Adds schema version read/write + migration logic and future-version rejection. |
| src/languages/definitions/typescript.ts | Deduplicates ECMAScript structure blocks via shared jsFamily definitions. |
| src/languages/definitions/jsFamily.ts | Adds shared ECMAScript block/split-point definitions for JS/TS family. |
| src/languages/definitions/javascript.ts | Switches to shared ECMAScript structure blocks/split points. |
| src/indexer/navigation.ts | Makes verified-reference scan deterministic by sorting file iteration. |
| src/indexer/incremental-plan.ts | Removes trailing whitespace (no behavior change). |
| src/indexer/build-cache/module-cache.ts | Adds bounded per-process memory cache with per-project clearing + LRU behavior. |
| src/indexer/build-cache.ts | Re-exports clearMemoryCache for cache management. |
| src/impact/transitive.ts | Reworks transitive BFS to track best depth and avoid shared-array mutation. |
| src/impact/reportShared.ts | Extracts shared cycle mapping helpers for full/compact report generation. |
| src/impact/reportFull.ts | Uses shared cycle mapping helper for display mapping. |
| src/impact/reportCompact.ts | Uses shared cycle mapping helper for compact mapping. |
| src/impact/callCompatibility.ts | Extracts reusable hint builder + reset helper to simplify main flow. |
| src/graphs/cycles.ts | Replaces recursive Tarjan with iterative version + indexes incoming edges for SCC reporting. |
| src/graphs/adjacency.ts | Memoizes adjacency index per graph instance via WeakMap. |
| src/drift/snapshot.ts | Replaces local include-root filtering with shared include-roots utilities. |
| src/cli/options.ts | Implements strict integer parsing + validated enum option parsers. |
| src/cli/inspect.ts | Uses shared restrictGraphToIncludeRoots implementation. |
| src/cli/impact.ts | Validates --scope and --ref-context via new parsers. |
| src/cli/graph.ts | Validates --symbols-detailed-scope via new parser. |
| src/cli/context.ts | Prevents value-options from consuming following flags; allows negative integer tokens. |
| src/cli.ts | Uses shared include-roots membership helper. |
| src/agent/orient.ts | Uses shared include-roots normalization/membership helpers. |
| src/agent/explain.ts | Increases reference collection limit to enable stable sort-before-truncate output. |
| src/agent-tools.ts | Sorts dependency entries before truncation for deterministic agent output. |
| docs/plans/2026-06-21-codegraph-technical-audit.md | Updates audit plan/checklist formatting and marks addressed items as complete. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/indexer/build-cache/module-cache.ts:255
lruMapGet()refreshes the memory-cache entry order even when the cache lookup is a miss due to a signature mismatch. That can keep stale entries “hot” and evict genuinely useful ones, reducing hit rate under churn.
Consider only refreshing LRU order when the entry is actually used (sig matches).
if (mode === "memory") {
const entry = lruMapGet(memoryCache, memoryCacheKey(projectRoot, file));
if (entry && entry.sig === sig) {
if (cacheEnabled && cacheReport) cacheReport.hits += 1;
return entry.mod;
}
if (cacheEnabled && cacheReport) cacheReport.misses += 1;
return null;
| 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(); |
| 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(` |
| 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; |
Summary
Completes audit remediation checklist items 3–9 on top of the already-merged C1–C3 work.
Priority 3 — H1/H2 transitive BFS
reasonsPriority 5 — P5/P6/P7 unbounded caches
lruMaphelper + LRU caps for module/resolution/lazy-symbol cachesPriority 6 — SQLite schema migration
schema_versionand explicit v1→v2 migratorPriority 7 — U1/U2–U4 usability
/^-?\d+$/)--scope,--ref-context,--symbols-detailed-scope--threads --jsonrejected)Priority 8 — P1/P2 performance
graphs/cycles.ts(avoids deep-graph stack overflow)WeakMapingraphs/adjacency.tsPriority 9 — S1–S6 structural (scoped)
languages/definitions/jsFamily.tsimpact/reportShared.tsutil/includeRoots.tscallCompatibility.tsduplicates.ts/cli.tsgod-module splits (S1/S2) — too large for this PRTest plan
npm run check