Skip to content

P0: refresh MCP index data before queries#142

Open
lzehrung wants to merge 10 commits into
mainfrom
p0/mcp-freshness-on-query
Open

P0: refresh MCP index data before queries#142
lzehrung wants to merge 10 commits into
mainfrom
p0/mcp-freshness-on-query

Conversation

@lzehrung

@lzehrung lzehrung commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • validate MCP session freshness before index-backed handlers return data
  • auto-refresh bounded file changes and report bounded stale metadata when refresh is unsafe
  • protect SQLite artifact queries with signature metadata and safe rebuild rules

Verification

  • npm run build
  • npx vitest run tests/mcp-server.test.ts
  • npx vitest run tests/sqlite.test.ts tests/sql-artifact-graph.test.ts
  • npx vitest run tests/agent-session.test.ts
  • npm run check
  • odw review-and-correct verify-fixes: 3/3 prior findings resolved, 0 regressions

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens MCP query handlers by validating (and, when safe, refreshing) session/index freshness before serving index-backed responses, and adds file-signature metadata to SQLite artifacts so staleness can be detected and handled safely.

Changes:

  • Add agent session freshness policies (manual/check/auto) with changed-file/byte thresholds and bounded stale reporting.
  • Attach freshness metadata to MCP handler responses and auto-refresh the warm session for small workspace edits.
  • Persist and validate SQLite artifact file-signature baselines, enabling safe refresh/rebuild rules for query_sqlite (and refusing stale reads when refresh is unsafe).

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/sqlite.test.ts Adds coverage ensuring SQLite artifact freshness metadata is cleared on unsigned full rewrites.
tests/mcp-server.test.ts Adds/updates MCP tests for auto-refresh behavior and SQLite freshness enforcement.
tests/helpers/agent.ts Updates test session wrapper to preserve new discovery/freshness behavior.
tests/agent-session.test.ts Adds coverage for check freshness policy behavior and cache invalidation expectations.
src/sqlite/write.ts Writes/clears SQLite artifact file-signature metadata during full exports.
src/sqlite/types.ts Extends SQLite write options to accept file-signature inputs.
src/sqlite.ts Re-exports the SQLite artifact signature metadata key for callers/tests.
src/mcp/server.ts Wraps MCP tools with freshness checks, refresh logic, and SQLite artifact staleness protection.
src/index.ts Exports new session freshness types from the main library surface.
src/agent/session.ts Implements freshness policies and signature baselines for session snapshots.
src/agent/artifact.ts Threads session file signatures into SQLite artifact builds.
src/agent.ts Re-exports new freshness types from the agent entrypoint.
docs/mcp.md Documents freshness metadata behavior and stale/refresh expectations for MCP tools.
docs/library-api.md Documents new session freshness options and shows freshness usage in examples.
docs/cli.md Updates CLI docs to describe MCP freshness behavior and stale recovery guidance.
codegraph-skill/codegraph/SKILL.md Updates agent skill guidance to account for MCP freshness metadata.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/agent/session.ts
Comment on lines +126 to +146
async function collectAgentFileSignatures(files: readonly string[]): Promise<Map<string, AgentFileSignature>> {
const signatures = new Map<string, AgentFileSignature>();
await Promise.all(
files.map(async (file) => {
const resolvedFile = path.resolve(file);
try {
const stat = await fsp.stat(resolvedFile);
if (!stat.isFile()) return;
signatures.set(resolvedFile, {
file: resolvedFile,
size: stat.size,
mtimeMs: stat.mtimeMs,
});
} catch (error) {
if (isMissingStatRace(error)) return;
throw new Error(`Unable to verify freshness for ${resolvedFile}`, { cause: error });
}
}),
);
return signatures;
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8762795 — replaced the unbounded Promise.all with mapLimit at a bounded concurrency of 64 (FILE_SIGNATURE_STAT_CONCURRENCY) so large workspaces no longer risk EMFILE / I/O saturation while still parallelizing.

Comment thread src/mcp/server.ts
Comment thread src/mcp/server.ts
Comment on lines +272 to +275
const checkMcpFreshness = async (): Promise<AgentFreshnessResult> => {
if (session.checkFreshness) return await session.checkFreshness();
return staleFreshness([], "freshness check unavailable");
};

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8762795checkMcpFreshness now returns { state: 'fresh' } (manual-mode semantics) when a prebuilt session has no checkFreshness, instead of the misleading stale label. Added regression test 'reports fresh when a prebuilt MCP session has no freshness check'.

Comment thread src/mcp/server.ts Outdated
Comment on lines +377 to +391
const signatures = new Map<string, SqliteArtifactFileSignature>();
await Promise.all(
currentFiles.map(async (file) => {
try {
const stat = await fs.stat(file);
if (!stat.isFile()) return;
signatures.set(relative(file), { path: file, size: stat.size, mtimeMs: stat.mtimeMs });
} catch (error) {
if (error instanceof Error && "code" in error && (error.code === "ENOENT" || error.code === "ENOTDIR")) {
return;
}
throw error;
}
}),
);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8762795 — bounded with mapLimit at SQLITE_ARTIFACT_STAT_CONCURRENCY (64), so freshness checks no longer fan out unbounded stat calls on large workspaces.

Comment thread src/sqlite/write.ts Outdated
Comment on lines +17 to +28
async function collectSqliteArtifactFileSignatures(files: Iterable<string>): Promise<SqliteArtifactFileSignature[]> {
const signatures: SqliteArtifactFileSignature[] = [];
await Promise.all(
[...files].map(async (file) => {
const stat = await fs.stat(file);
if (!stat.isFile()) return;
signatures.push({ path: file, size: stat.size, mtimeMs: stat.mtimeMs });
}),
);
signatures.sort((left, right) => left.path.localeCompare(right.path));
return signatures;
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8762795 — removed the unused collectSqliteArtifactFileSignatures helper and the now-unnecessary node:fs/promises import from src/sqlite/write.ts.

@lzehrung lzehrung requested a review from Copilot July 3, 2026 14:48
@lzehrung lzehrung marked this pull request as ready for review July 3, 2026 14:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Comment thread src/agent/session.ts
Comment on lines +129 to +147
async function collectAgentFileSignatures(files: readonly string[]): Promise<Map<string, AgentFileSignature>> {
const signatures = new Map<string, AgentFileSignature>();
await mapLimit([...files], FILE_SIGNATURE_STAT_CONCURRENCY, async (file) => {
const resolvedFile = path.resolve(file);
try {
const stat = await fsp.stat(resolvedFile);
if (!stat.isFile()) return;
signatures.set(resolvedFile, {
file: resolvedFile,
size: stat.size,
mtimeMs: stat.mtimeMs,
});
} catch (error) {
if (isMissingStatRace(error)) return;
throw new Error(`Unable to verify freshness for ${resolvedFile}`, { cause: error });
}
});
return signatures;
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ecea3d1 — signature keys are now normalizePath(path.resolve(file)), so they use forward slashes and match snapshot.fileGraph.nodes on Windows. This prevents the 'freshness signature is missing' failure during artifact_build.

Comment thread src/agent/artifact.ts
Comment thread tests/sqlite.test.ts Outdated
@kilo-code-bot

kilo-code-bot Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • src/mcp/server.ts
  • tests/mcp-server.test.ts
Previous Review Summaries (2 snapshots, latest commit ecea3d1)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit ecea3d1)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)
  • src/agent/artifact.ts
  • src/agent/session.ts
  • src/index.ts
  • tests/sqlite.test.ts

Notes

  • All 3 prior Copilot review findings from commit 8762795 are resolved in commit ecea3d1 (head ecea3d14957b3114aed70e658bbba60480e9349d).
  • fileSignatures is passed directly to writeGraphSqlite instead of a redundant conditional spread.
  • collectAgentFileSignatures now normalizes resolved paths with normalizePath so signature keys match snapshot.fileGraph.nodes on Windows.
  • SQLITE_ARTIFACT_FILE_SIGNATURES_METADATA_KEY is re-exported from src/index.ts so tests can reference the canonical constant instead of hard-coding the key string.
  • No additional bugs, security issues, or regressions were found in the changed code.

Previous review (commit 8762795)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (16 files)
  • codegraph-skill/codegraph/SKILL.md
  • docs/cli.md
  • docs/library-api.md
  • docs/mcp.md
  • src/agent.ts
  • src/agent/artifact.ts
  • src/agent/session.ts
  • src/index.ts
  • src/mcp/server.ts
  • src/sqlite.ts
  • src/sqlite/types.ts
  • src/sqlite/write.ts
  • tests/agent-session.test.ts
  • tests/helpers/agent.ts
  • tests/mcp-server.test.ts
  • tests/sqlite.test.ts

Notes

  • All 5 prior Copilot review findings are resolved in commit 8762795 (head 876279560ba58873598ac716162965b11f914a5c).
  • Unbounded Promise.all calls are replaced with mapLimit at bounded concurrency (FILE_SIGNATURE_STAT_CONCURRENCY and SQLITE_ARTIFACT_STAT_CONCURRENCY, both 64).
  • JSON.parse in readSqliteArtifactSignatures is now guarded with try/catch; malformed metadata returns null and falls back to the missing-baseline path.
  • Prebuilt MCP sessions without checkFreshness now return { state: "fresh" } instead of misleadingly labeling them stale.
  • The unused collectSqliteArtifactFileSignatures helper and its node:fs/promises import are removed from src/sqlite/write.ts.
  • No additional bugs, security issues, or regressions were found in the changed code.

Reviewed by kimi-k2.6-20260420 · Input: 79.1K · Output: 11.8K · Cached: 985.3K

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Comment thread src/mcp/server.ts Outdated
Comment on lines +614 to +621
let freshness = await checkMcpFreshness();
freshness = await refreshSqliteArtifactForQuery(freshness);
let realSqlitePath = await assertRealPathCandidateWithinRoot(await realRoot, sqlitePath, "SQLite artifact");
const artifactFreshness = await checkSqliteArtifactFreshness(realSqlitePath);
if (artifactFreshness.state !== "fresh") {
freshness = await refreshSqliteArtifactForQuery(artifactFreshness, { allowStaleRebuild: true });
realSqlitePath = await assertRealPathCandidateWithinRoot(await realRoot, sqlitePath, "SQLite artifact");
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ab35d9cquery_sqlite now decides serve/refuse from the SQLite artifact's own baseline (checkSqliteArtifactFreshness). Session freshness only gates automatic rebuilds: a stale session blocks an auto-rebuild, but a fresh artifact is served even when the in-memory session snapshot is stale/refreshed. The returned freshness now reflects the artifact. Added regression test 'serves a fresh SQLite artifact even when the session snapshot is stale' (verified with teeth: the old ordering threw in read-only).

Comment thread src/mcp/server.ts Outdated
Comment on lines +485 to +487
get_file: async (request) =>
await withFreshness(async () => {
const resolvedFile = await resolveReadableFile(await realRoot, root, request.file);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ab35d9cget_file no longer runs through withFreshness. It reads live bytes directly from disk and never consults the indexed session, so it no longer re-discovers/re-stats the whole workspace (or triggers an index rebuild) on every call. It now returns freshness: { state: 'fresh' } since the returned bytes are always current. Updated the get_file test accordingly and re-pointed the missing-checkFreshness regression to search (still withFreshness-wrapped) so that fallback stays covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants