fix: tighten review-first workflows and cache reuse#139
Open
lzehrung wants to merge 12 commits into
Open
Conversation
Make review the default daily path, surface analysis metadata across search, explain, impact, and review, and keep long-lived sessions fresher and faster. Also persist snapshot bloom filters and make SQLite readonly access compatible with Node builds that omit setAuthorizer(). Co-authored-by: Cursor <cursoragent@cursor.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
|
|
||
| private captureFreshnessBaseline(index: ProjectIndex, reason: "initialization" | "manual" | "stale_check"): void { | ||
| const trackedEntries = index.manifestEntries |
Contributor
There was a problem hiding this comment.
WARNING: Empty manifestEntries Map bypasses file staleness tracking
index.manifestEntries can be an empty Map, which is truthy in JavaScript. When this happens, trackedFileSignatures is initialized empty, and refreshNeededFromTrackedFiles() only checks the config file — file changes are never detected. Use index.manifestEntries?.size to guard against empty Maps.
Suggested change
| const trackedEntries = index.manifestEntries | |
| const trackedEntries = index.manifestEntries?.size |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewNo new issues found in the incremental diff. Files Reviewed (2 files)
Previous Review Summaries (9 snapshots, latest commit f24b903)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f24b903)Status: No Issues Found | Recommendation: Merge Overview
Resolved Issues on Changed Lines
Files Reviewed (4 files)
Previous review (commit 58d8ea9)Status: No Issues Found | Recommendation: Merge Overview
Resolved Issues on Changed Lines
Files Reviewed (4 files)
Previous review (commit 52d510f)Status: No Issues Found | Recommendation: Merge Overview
Resolved Issues on Changed Lines
Files Reviewed (3 files)
Previous review (commit 7defa33)Status: No Issues Found | Recommendation: Merge Overview
Files Reviewed (4 files)
Previous review (commit dd9ca7d)Status: No Issues Found | Recommendation: Merge Overview
Files Reviewed (1 file)
Previous review (commit ff7d9b9)Status: No Issues Found | Recommendation: Merge Overview
Files Reviewed (3 files)
Previous review (commit 0dccb02)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 51fe610)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (13 files)
Fix these issues in Kilo Cloud Previous review (commit 21d5849)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (33 files)
Reviewed by kimi-k2.6-20260420 · Input: 293.7K · Output: 30.6K · Cached: 1.8M |
| try { | ||
| const entries = fs | ||
| .readdirSync(directory, { withFileTypes: true }) | ||
| .filter((entry) => entry.name !== ".codegraph-cache" && entry.name !== ".git") |
Contributor
There was a problem hiding this comment.
WARNING: directorySignature tracks node_modules changes, causing unnecessary full rebuilds
The directory signature includes every entry name, filtering out only .codegraph-cache and .git. When node_modules is present in a tracked directory (commonly the project root), any package install or update changes the directory listing and triggers a full session rebuild. Consider also ignoring node_modules, dist, build, target, .venv, and __pycache__ to match the default file discovery ignores.
Suggested change
| .filter((entry) => entry.name !== ".codegraph-cache" && entry.name !== ".git") | |
| .filter((entry) => entry.name !== ".codegraph-cache" && entry.name !== ".git" && entry.name !== "node_modules") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR recenters Codegraph’s default workflows around review-first daily change analysis, while surfacing analysis/provenance metadata across agent search/explain and impact/review outputs, and improving warm-session/cache reuse behavior (stale-session refresh + persisted bloom filters + SQLite readonly compatibility).
Changes:
- Add
AnalysisSummaryplumbing and exposeanalysis/provenancemetadata in search, explain, impact (batch + streaming), and review outputs/CLI formatting. - Add warm
CodeReviewSessionstale detection with auto-refresh before serving navigation/impact, plus richer session stats. - Persist bloom filters into the on-disk project snapshot and reuse them on unchanged incremental loads; adjust SQLite readonly authorizer handling for runtimes without
setAuthorizer().
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/session.test.ts | Adds coverage for stale-session detection, auto-refresh, and refresh stats. |
| tests/impact.test.ts | Asserts impact reports include analysis metadata (native semantic label). |
| tests/impact-streaming.test.ts | Ensures streaming impact summaries include analysis metadata. |
| tests/impact-cli.test.ts | Validates CLI JSON/pretty output includes analysis label/section. |
| tests/cli-regressions.test.ts | Formatting-only change to keep sqlite driver import regression check readable. |
| tests/cache-invalidation.test.ts | Adds tests for persisted snapshot bloom filter reuse and snapshot schema upgrade behavior. |
| tests/agent-search.test.ts | Updates agent search tests for top-level analysis + per-result provenance + code-first hybrid ranking. |
| src/sqlite-driver.ts | Makes setAuthorizer() optional for readonly mode when unavailable in node:sqlite. |
| src/session.ts | Implements stale detection + refresh orchestration + richer session stats and build report propagation. |
| src/review/types.ts | Adds optional analysis to ReviewReport output shape. |
| src/review/report.ts | Threads optional analysis metadata into assembled review reports. |
| src/review.ts | Produces review analysis metadata via summarizeAnalysis() and includes it in reports. |
| src/indexer/types.ts | Adds buildReport?: BuildReport to ProjectIndex for downstream provenance/analysis. |
| src/indexer/finalize.ts | Plumbs buildReport into finalized ProjectIndex. |
| src/indexer/build-index.ts | Ensures build runs always have a report object and attaches it to the resulting index; adjusts snapshot reuse behavior. |
| src/indexer/build-cache/project-snapshot.ts | Bumps snapshot version and serializes/deserializes bloom filters into project snapshots. |
| src/index.ts | Re-exports analysis summary types from the public entrypoint. |
| src/impact/types.ts | Adds optional analysis to impact report shapes (full/compact/stream-summary). |
| src/impact/streaming.ts | Adds analysis metadata generation and threads build report context through streaming. |
| src/impact/reportFull.ts | Includes analysis metadata in full impact report output. |
| src/impact/reportCompact.ts | Includes analysis metadata in compact impact report output. |
| src/impact/report.ts | Computes and injects analysis metadata when building impact reports. |
| src/impact/index.ts | Adds impact analysis context for passing build report into report-building. |
| src/cli/review.ts | Prints analysis label in review CLI summary output. |
| src/cli/impact.ts | Captures build report during indexing and prints analysis label in pretty output; includes analysis in full JSON conversion. |
| src/cli/help.ts | Reorders commands and updates help text toward review-first/orient→search→explain workflow and analysis/provenance messaging. |
| src/analysisSummary.ts | Introduces shared AnalysisSummary derivation + label formatting used across outputs. |
| src/agent/session.ts | Captures build report and analysis summary in agent session snapshots. |
| src/agent/search.ts | Adds top-level analysis and per-result provenance; retunes doc boosts to keep hybrid searches code-first by default. |
| src/agent/explain.ts | Adds top-level analysis metadata to explain packets and formatting. |
| src/agent-tools.ts | Switches tool indexing to incremental+disk cache defaults for better warm reuse. |
| README.md | Updates primary docs to emphasize review-first daily workflow and analysis visibility. |
| docs/plans/2026-06-27-roi-implementation-progress.md | Adds an implementation progress tracking doc for session handoff/context. |
| docs/library-api.md | Updates library guidance for warm session reuse and analysis/provenance behavior. |
| docs/how-it-works.md | Documents warm-cache/session reuse and stale refresh behavior. |
| docs/cli.md | Updates CLI docs for default workflow guidance and analysis/provenance behavior. |
| docs/agent-workflows.md | Updates agent workflow docs for review-first flow, analysis/provenance fields, and canonical session reuse. |
| codegraph-skill/codegraph/SKILL.md | Updates skill guidance to match review-first workflow and analysis/provenance behavior. |
Comments suppressed due to low confidence (1)
src/indexer/build-index.ts:422
createIndexBuildRunState()now always assigns a default{ timings: {} }report, which makesreporttruthy even when the caller did not provideopts.report. Downstream logic inbuildProjectIndexIncremental()usesreportas a signal (e.g.if (explicitFileSet.size && (!explicitFilesCoverAllFiles || report)) ...), so this change can unintentionally force files to be treated as changed and prevent snapshot/manifest reuse. Consider threading an explicitreportProvided = !!opts?.reportboolean (or switching those conditions to checkopts?.report) so the semantics remain stable while still populatingindex.buildReport.
const report = opts?.report ?? { timings: {} };
initNativeBackendReport(report);
const cacheMode = opts?.cache ?? "off";
return {
normalizedProjectRoot: normalizePath(projectRoot),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+240
to
+250
| function isSerializedBloomFilter(value: unknown): value is SerializedBloomFilter { | ||
| if (!value || typeof value !== "object") return false; | ||
| const filter = value as Partial<SerializedBloomFilter>; | ||
| return ( | ||
| typeof filter.size === "number" && | ||
| Number.isFinite(filter.size) && | ||
| typeof filter.hashCount === "number" && | ||
| Number.isFinite(filter.hashCount) && | ||
| typeof filter.bitsBase64 === "string" | ||
| ); | ||
| } |
| } | ||
| const maxBytes = Math.ceil(filter.size / 8); | ||
| const maxBase64Length = Math.ceil(maxBytes / 3) * 4; | ||
| return filter.bitsBase64.length <= maxBase64Length; |
Contributor
There was a problem hiding this comment.
WARNING: bitsBase64 upper bound alone allows too-short strings that decode to fewer bytes than size requires
A truncated or otherwise too-short bitsBase64 still passes the <= maxBase64Length check. When BloomFilter.fromBuffer receives a buffer shorter than Math.ceil(size / 8) bytes, mightContain silently returns false for bit indices beyond the buffer, causing false negatives in reference filtering and potentially missing valid results. Consider using === to match the exact padded base64 length produced by Buffer.toString("base64").
Suggested change
| return filter.bitsBase64.length <= maxBase64Length; | |
| return filter.bitsBase64.length === maxBase64Length; |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ...(payload.nativeMode ? { nativeMode: payload.nativeMode } : {}), | ||
| exportCache: new Map(), | ||
| scopeCache: new Map(), | ||
| ...(payload.bloomFilters ? { bloomFilters: deserializeBloomFilterCache(payload.bloomFilters) } : {}), |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/agent-tools.ts:520
getToolIndex()now forcescache: "disk", which will attempt to create.codegraph-cache/index-v1under the project root (viafs.mkdirSync(...)in the disk cache implementation). This can break agent/tool usage on read-only checkouts (containers, sandboxed hosts, or repos mountedro), where the previous behavior could still work.
Recommend falling back to an in-memory cache (or cache: "off") when disk cache initialization fails with EACCES/EROFS/EPERM, so navigation tools remain usable even without write access.
async function getToolIndex(root: string, options: ToolRuntimeOptions): Promise<ProjectIndex> {
return (
options.index ??
(await buildProjectIndexIncremental(root, {
cache: "disk",
keepParsed: true,
useBloomFilters: true,
logLevel: "error",
...(options.native ? { native: options.native } : {}),
}))
);
Comment on lines
+369
to
+373
| for (const [file, signature] of this.trackedFileSignatures) { | ||
| if (this.statSignature(file) !== signature) { | ||
| return "tracked_files_changed"; | ||
| } | ||
| } |
Comment on lines
241
to
247
| if (options.forceFull) { | ||
| const projectFiles = await this.currentProjectFiles(); | ||
| const report: BuildReport = { timings: {} }; | ||
| const buildOptions: IncrementalBuildOptions = { ...this.buildOptions, files: projectFiles, report }; | ||
| const index = await buildProjectIndexIncremental(this.root, buildOptions); | ||
| return { index, report: index.buildReport ?? report, projectFiles }; | ||
| } |
Comment on lines
1002
to
1005
| if (!changedFiles.size && !deletedTrackedFiles.size && !opts?.report) { | ||
| const filesSignature = projectSnapshotFilesSignature(new Map(Object.entries(trackedEntries))); | ||
| const snapshot = await tryLoadProjectIndexSnapshot(projectRoot, opts, filesSignature); | ||
| if (snapshot) { |
Comment on lines
+406
to
+418
| private checkForStalenessNow(): void { | ||
| if (this.status !== "ready" || !this.index) return; | ||
| this.lastStaleCheckAt = Date.now(); | ||
| const trackedReason = this.refreshNeededFromTrackedFiles(); | ||
| if (trackedReason) { | ||
| this.staleReason = trackedReason; | ||
| this.forceFullRefreshOnNextStaleCheck = false; | ||
| return; | ||
| } | ||
| const projectFilesChanged = this.projectDirectoriesChanged(); | ||
| this.staleReason = projectFilesChanged ? "tracked_files_changed" : undefined; | ||
| this.forceFullRefreshOnNextStaleCheck = projectFilesChanged; | ||
| } |
| const directories = new Set<string>([this.root]); | ||
| for (const file of files) { | ||
| let directory = path.dirname(path.resolve(file)); | ||
| while (directory.startsWith(this.root)) { |
Comment on lines
+667
to
+668
| stale: !!this.staleReason, | ||
| ...(this.staleReason ? { staleReason: this.staleReason } : {}), |
Comment on lines
+540
to
+544
| private async ensureFreshIndex(options: { force?: boolean; file?: string } = {}): Promise<ProjectIndex> { | ||
| this.checkExpiration(); | ||
| this.checkForStalenessNow(options); | ||
| const index = this.getIndex(); | ||
| if (!this.staleReason) { |
Comment on lines
+1027
to
+1029
| if (report) { | ||
| snapshot.buildReport = report; | ||
| } |
Comment on lines
+409
to
+413
| const configSignature = this.statSignature(this.configFilePath()); | ||
| if (configSignature !== this.configSignature) { | ||
| this.staleReason = "config_changed"; | ||
| this.forceFullRefreshOnNextStaleCheck = false; | ||
| return; |
Comment on lines
+267
to
+273
| const buildOptions: BuildOptions = { ...currentBuildOptions }; | ||
| const index = this.incremental | ||
| ? await buildProjectIndexIncremental(this.root, buildOptions) | ||
| : await buildProjectIndex(this.root, buildOptions); | ||
| const projectFiles = this.indexedProjectFiles(index); | ||
| const report = index.buildReport ?? { timings: {} }; | ||
| return { index, report, projectFiles }; |
Comment on lines
+394
to
+398
| for (const [file, signature] of this.trackedFileSignatures) { | ||
| if (this.statSignature(file) !== signature) { | ||
| return "tracked_files_changed"; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
review,impact, andorient -> search -> explainnode:sqliteomitssetAuthorizer()Test plan
npx vitest run tests/impact-cli.test.ts tests/impact-streaming.test.ts tests/impact.test.tsnpx vitest run tests/cache-invalidation.test.ts tests/parsed-cache-reuse.test.ts tests/bloom-filter-integration.test.tsnpx vitest run tests/sqlite.test.ts tests/cli-command-modules.test.ts tests/cli-regressions.test.tsnpm run checkMade with Cursor