v0.23.0 feat(snapshot): team-shared graph artifact via GitHub Releases#18
Conversation
…ACUUM INTO TDD: red test first (create_writes_meta_and_drops_vec_table), then implementation. Promotes tempfile from dev-dependency to regular dep so snapshot::create() can use it in production code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… in [dependencies])
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Priority order: HTTPS url from .code-graph.toml > disabled=true guard > git-remote auto-detect via `gh api` GitHub releases. 4 TDD tests cover toml-url, disabled, http-rejection, and no-git fallback paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ame + 100MB cap Move reqwest from [dev-dependencies] to [dependencies] so try_install can use blocking HTTP in lib code; file:// path enables hermetic unit tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…install (Approach A) Investigation finding: self.db is a plain Database (not Arc<Database>), opened in from_project_root via open_db_for_role before any indexing runs. Installing a snapshot via POSIX rename(2) AFTER self.db opens would leave the connection pointing at the old empty inode — queries would silently see no data. Fix (Approach A — preferred): call maybe_install_snapshot() inside from_project_root BEFORE open_db_for_role, guarded by !db_path.exists(). This way the connection we open lands directly on the post-install file, avoiding the inode-swap entirely. The background indexing path (spawn_startup_indexing) already opens its own fresh Database connection from the path, so it was inode-safe regardless; only the construction-time and sync ensure_indexed paths needed the fix. Also updates ensure_indexed's synchronous fallback (when startup indexing hasn't run yet): it now checks has_existing via get_index_status before deciding between run_full_index and run_incremental_index. When snapshot data is present, it runs incremental drift-correction instead of a full reindex — matching the behavior of spawn_startup_indexing which already had the has_existing check at line 465. Adds try_install_snapshot() method on McpServer as a post-construction helper (marked dead_code for Task 11/12), and a test that proves the inode-safety contract: snapshot installed before from_project_root → self.db sees nodes without calling ensure_indexed. All 518 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a `snapshot` block to `cmd_health_check` JSON output with status/source_url/source_commit/fetched_at/commit_drift fields. Also emits a `Snapshot: <status>` line in the oneline format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… concurrent Also fix try_install to use per-invocation unique partial file names (via AtomicU64 counter) so concurrent callers don't clobber each other's in-progress partials; the final atomic rename serialises the winner. Error-path cleanup now guards final_db removal behind db_partial_gone to avoid evicting a concurrent winner's installed db. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…newer_schema (refactor-safe)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…unix), don't delete final_db on error
📝 WalkthroughWalkthroughThis PR introduces a team-shared graph snapshot feature that allows precomputed code-graph indexes to be published and automatically fetched by consumers. It adds snapshot creation/inspection CLI commands, MCP server auto-fetch on first start, TOML-based configuration, a GitHub Actions workflow template, and comprehensive tests covering creation, installation, validation, and integration with incremental indexing. ChangesSnapshot System Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/snapshot/meta.rs (1)
38-49: 💤 Low valueUse
rusqlite::OptionalExtension::optional()for cleaner None mapping.
query_row(...).optional()?is the idiomatic pattern for optional query results and replaces the manualQueryReturnedNoRowsdiscrimination with a single method call.♻️ Proposed refactor
pub fn read_meta(conn: &Connection, key: &str) -> Result<Option<String>> { + use rusqlite::OptionalExtension; conn.query_row( "SELECT value FROM meta WHERE key = ?1", rusqlite::params![key], |row| row.get::<_, String>(0), ) - .map(Some) - .or_else(|e| match e { - rusqlite::Error::QueryReturnedNoRows => Ok(None), - other => Err(anyhow::anyhow!(other)), - }) + .optional() + .with_context(|| format!("read_meta({key})")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/snapshot/meta.rs` around lines 38 - 49, The read_meta function currently handles QueryReturnedNoRows manually; replace that logic by using rusqlite::OptionalExtension::optional(): import/use OptionalExtension, call conn.query_row("SELECT value FROM meta WHERE key = ?1", rusqlite::params![key], |row| row.get::<_, String>(0)).optional()? and return the Option<String> directly from read_meta (keeping the Result return type), removing the manual or_else match on QueryReturnedNoRows; ensure read_meta still returns Result<Option<String>> and that OptionalExtension is in scope.src/mcp/server/mod.rs (1)
2488-2491: ⚡ Quick winUse the standard
src/lib.rslayout in this regression fixture.This test is about pre-open snapshot visibility, but both temp repos put the Rust file at the repo root. That adds an unrelated dependency on the indexer continuing to scan nonstandard Rust roots, so a future file-discovery change could break this inode-swap regression for the wrong reason.
Suggested change
- std::fs::write( - source.path().join("lib.rs"), + std::fs::create_dir_all(source.path().join("src")).unwrap(); + std::fs::write( + source.path().join("src/lib.rs"), "pub fn snapshot_sentinel() {}\npub fn snapshot_caller() { snapshot_sentinel(); }\n", ).unwrap(); @@ - std::fs::write( - consumer.path().join("lib.rs"), + std::fs::create_dir_all(consumer.path().join("src")).unwrap(); + std::fs::write( + consumer.path().join("src/lib.rs"), "pub fn snapshot_sentinel() {}\npub fn snapshot_caller() { snapshot_sentinel(); }\n", ).unwrap();Also applies to: 2506-2509
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server/mod.rs` around lines 2488 - 2491, The regression fixture currently writes the Rust file at the repository root via source.path().join("lib.rs"); change the fixture to create a standard src directory and write the file to source.path().join("src").join("lib.rs") (ensuring the "src" directory exists) so the test uses the conventional src/lib.rs layout; update the analogous occurrences around the other write calls that place lib.rs at repo root (the same write(...) call creating "lib.rs" and the test snippets that define snapshot_sentinel and snapshot_caller) to use the src/lib.rs location instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@claude-plugin/templates/code-graph-snapshot.yml`:
- Around line 33-35: The GH release upload step uses the `gh release upload`
command to push the artifact "code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst" but
is non-idempotent and will fail on re-run; update the run block that invokes `gh
release upload` (the command shown as `gh release upload "${{
github.event.release.tag_name }}"
"code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst"`) to include the `--clobber` flag
so repeated workflow runs replace the existing asset instead of erroring with
already_exists.
- Around line 27-29: The workflow is calling the wrong npm package name in the
shell step: replace the npx invocation string "npx -y code-graph-mcp@latest
snapshot create --out snapshot.db" with the correct package "@sdsrs/code-graph"
(e.g., use "npx -y `@sdsrs/code-graph`@latest snapshot create --out snapshot.db")
so the snapshot create command uses the published package; update any identical
occurrences of "code-graph-mcp" in the template to "@sdsrs/code-graph".
In `@README.md`:
- Around line 323-324: Update the README instruction that copies the workflow
template: replace the incorrect package path
"node_modules/code-graph-mcp/templates/code-graph-snapshot.yml" with the actual
published package path
"node_modules/@sdsrs/code-graph/templates/code-graph-snapshot.yml" so the cp
command matches the npm package referenced elsewhere (npx -y `@sdsrs/code-graph`)
and users won't get "cannot stat" errors.
In `@src/snapshot/install.rs`:
- Around line 132-134: The bail message in the conditional that checks the
snapshot URL (the if !(url.starts_with("https://") ||
url.starts_with("file://")) block) is misleading because it only mentions
"https://" even though "file://" is also accepted; update the anyhow::bail! call
to list both accepted schemes (e.g., "snapshot url must start with https:// or
file:// (got {url})") so the error accurately reflects the allowed URL schemes.
- Around line 8-25: The doc comment for resolve_snapshot_source lists resolution
order incorrectly vs the code: the function currently checks
cfg.snapshot.disabled before cfg.snapshot.url and thus short-circuits; either
update the doc to list "disabled" before "url" to match current behavior, or if
the intended behavior is that an explicit cfg.snapshot.url should override
disablement, move the cfg.snapshot.url check (the if let Some(url) =
cfg.snapshot.url) above the cfg.snapshot.disabled check so the URL is honored
first; adjust the doc or the order in resolve_snapshot_source accordingly.
- Around line 91-126: In wait_with_watchdog, the watchdog thread may call
libc::kill(pid, SIGTERM) after its loop even if the main thread just set cancel;
before sending the signal you must re-check the cancel flag to avoid killing a
recycled PID. Modify the closure spawned into watchdog (referencing pid and
cancel_w) to, after the loop exits due to timeout, test
cancel_w.load(Ordering::Relaxed) and return if true, only calling
libc::kill(pid, libc::SIGTERM) when cancel_w is still false; keep the existing
join/store behavior in the main thread (cancel.store(true) and join).
- Around line 194-211: The download() function currently buffers the entire HTTP
response via Response::bytes(), which can OOM; change it to stream the response
to disk and enforce a compressed-size cap by using Response::copy_to with a
capped writer instead of bytes(). Specifically, in download() (and the HTTP
branch where reqwest::blocking::Client is built and .get(url).send() is called)
create the destination file (std::fs::File::create(dest)), wrap it in the
existing CapWriter with a MAX_COMPRESSED_BYTES constant, and call
resp.copy_to(&mut capped_writer) (checking error_for_status() first) so the
compressed payload is limited and written incrementally to disk rather than
fully buffered in memory.
In `@src/snapshot/mod.rs`:
- Around line 96-103: The current inspect path leaks memory by reading the whole
compressed file and using zstd::decode_all then calling Database::open (which
may apply migrations); replace that with the install path’s capped, streaming
decompression routine (do not use zstd::decode_all or read the full file into a
Vec) to write at most the configured max bytes into the temp snapshot.db, and
open the resulting temp DB read-only (use the install helper or rusqlite
open-with-flags; replace Database::open(&decompressed)? with a read-only open
such as Database::open_read_only or opening via
rusqlite::Connection::open_with_flags(..., OpenFlags::SQLITE_OPEN_READ_ONLY)).
Ensure you reference and remove the use of the variables/steps
compressed/zstd::decode_all/raw and instead call the capped streaming
decompressor used by the install code and then open the temp DB in read-only
mode for inspection.
In `@tests/cli_snapshot.rs`:
- Around line 13-19: The git setup commands in the test (the Command::new calls
for "git init", "git config", "git add", "git commit") only call
status().unwrap(), which only ensures the process started but not that it exited
successfully; change these to assert the exit status succeeded by checking
status.success() (or replace repeated calls with a small helper like
run_and_assert(cmd) that runs the command, calls .status().expect("spawn
failed") and then assert!(status.success(), "git <action> failed: {:?}",
status)), referencing the Command invocations shown in the test (e.g., the
Command::new("git").args([...]).current_dir(p).status() calls) so any setup
failure fails the test immediately with a clear message.
---
Nitpick comments:
In `@src/mcp/server/mod.rs`:
- Around line 2488-2491: The regression fixture currently writes the Rust file
at the repository root via source.path().join("lib.rs"); change the fixture to
create a standard src directory and write the file to
source.path().join("src").join("lib.rs") (ensuring the "src" directory exists)
so the test uses the conventional src/lib.rs layout; update the analogous
occurrences around the other write calls that place lib.rs at repo root (the
same write(...) call creating "lib.rs" and the test snippets that define
snapshot_sentinel and snapshot_caller) to use the src/lib.rs location instead.
In `@src/snapshot/meta.rs`:
- Around line 38-49: The read_meta function currently handles
QueryReturnedNoRows manually; replace that logic by using
rusqlite::OptionalExtension::optional(): import/use OptionalExtension, call
conn.query_row("SELECT value FROM meta WHERE key = ?1", rusqlite::params![key],
|row| row.get::<_, String>(0)).optional()? and return the Option<String>
directly from read_meta (keeping the Result return type), removing the manual
or_else match on QueryReturnedNoRows; ensure read_meta still returns
Result<Option<String>> and that OptionalExtension is in scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb419406-4ae0-416e-a53f-693ec092f60e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.claude-plugin/marketplace.jsonCHANGELOG.mdCargo.tomlREADME.mdclaude-plugin/.claude-plugin/plugin.jsonclaude-plugin/templates/code-graph-snapshot.ymlnpm/darwin-arm64/package.jsonnpm/darwin-x64/package.jsonnpm/linux-arm64/package.jsonnpm/linux-x64/package.jsonnpm/win32-x64/package.jsonpackage.jsonsrc/cli.rssrc/lib.rssrc/main.rssrc/mcp/server/mod.rssrc/snapshot/config.rssrc/snapshot/install.rssrc/snapshot/meta.rssrc/snapshot/mod.rssrc/snapshot/tests.rstests/cli_snapshot.rstests/snapshot_bench.rstests/snapshot_integration.rs
| npx -y code-graph-mcp@latest snapshot create --out snapshot.db | ||
| zstd -9 snapshot.db -o snapshot.db.zst | ||
| mv snapshot.db.zst "code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
npmjs.com @sdsrs/code-graph package name registry
💡 Result:
The npm registry package name for “@sdsrs/code-graph” is exactly: @sdsrs/code-graph [1][2] Its npm registry page is: https://registry.npmjs.org/%40sdsrs%2Fcode-graph [1] And the package page on npmjs.com is: https://www.npmjs.com/package/@sdsrs/code-graph [2] This package describes itself as “MCP server that indexes codebases into an AST knowledge graph …” and provides install command npm i @sdsrs/code-graph``. [2]
Citations:
- 1: https://registry.npmjs.org/%40sdsrs%2Fcode-graph
- 2: https://www.npmjs.com/package/@sdsrs/code-graph
Fix incorrect npm package name — workflow will fail on every run.
The workflow references code-graph-mcp@latest, but the published npm package on the registry is @sdsrs/code-graph. Running npx -y code-graph-mcp@latest will produce a 404 error (npm error 404 Not Found - GET https://registry.npmjs.org/code-graph-mcp).
Use @sdsrs/code-graph instead:
Proposed fix
- name: Build snapshot
run: |
- npx -y code-graph-mcp@latest snapshot create --out snapshot.db
+ npx -y `@sdsrs/code-graph`@latest snapshot create --out snapshot.db
zstd -9 snapshot.db -o snapshot.db.zst
mv snapshot.db.zst "code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| npx -y code-graph-mcp@latest snapshot create --out snapshot.db | |
| zstd -9 snapshot.db -o snapshot.db.zst | |
| mv snapshot.db.zst "code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst" | |
| npx -y `@sdsrs/code-graph`@latest snapshot create --out snapshot.db | |
| zstd -9 snapshot.db -o snapshot.db.zst | |
| mv snapshot.db.zst "code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude-plugin/templates/code-graph-snapshot.yml` around lines 27 - 29, The
workflow is calling the wrong npm package name in the shell step: replace the
npx invocation string "npx -y code-graph-mcp@latest snapshot create --out
snapshot.db" with the correct package "@sdsrs/code-graph" (e.g., use "npx -y
`@sdsrs/code-graph`@latest snapshot create --out snapshot.db") so the snapshot
create command uses the published package; update any identical occurrences of
"code-graph-mcp" in the template to "@sdsrs/code-graph".
| run: | | ||
| gh release upload "${{ github.event.release.tag_name }}" \ | ||
| "code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst" |
There was a problem hiding this comment.
gh release upload is non-idempotent — add --clobber for re-runs.
Without --clobber, retrying this workflow on an already-published release (manual re-run, transient failure recovery, hot-fix) fails with HTTP 422: Validation Failed (already_exists). Since the asset name is content-addressed by short SHA, a fresh upload of the same artifact for the same release is the intended behavior on retry.
📝 Proposed fix
gh release upload "${{ github.event.release.tag_name }}" \
- "code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst"
+ "code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst" \
+ --clobber🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude-plugin/templates/code-graph-snapshot.yml` around lines 33 - 35, The GH
release upload step uses the `gh release upload` command to push the artifact
"code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst" but is non-idempotent and will
fail on re-run; update the run block that invokes `gh release upload` (the
command shown as `gh release upload "${{ github.event.release.tag_name }}"
"code-graph-snapshot-${GITHUB_SHA:0:7}.db.zst"`) to include the `--clobber` flag
so repeated workflow runs replace the existing asset instead of erroring with
already_exists.
| 1. Copy `node_modules/code-graph-mcp/templates/code-graph-snapshot.yml` | ||
| into your repo's `.github/workflows/`. |
There was a problem hiding this comment.
Path uses the binary name instead of the npm package name.
The published npm package is @sdsrs/code-graph (the rest of the README references npx -y @sdsrs/code-graph``), so the template lands at node_modules/@sdsrs/code-graph/templates/code-graph-snapshot.yml. Users following these instructions verbatim will hit `cp: cannot stat 'node_modules/code-graph-mcp/...': No such file or directory`.
📝 Proposed fix
**Setup (one-time):**
-1. Copy `node_modules/code-graph-mcp/templates/code-graph-snapshot.yml`
+1. Copy `node_modules/@sdsrs/code-graph/templates/code-graph-snapshot.yml`
into your repo's `.github/workflows/`.Note: the LanguageTool hint flagging "github" → "GitHub" on this line is a false positive — .github/workflows/ is the literal directory name and must stay lowercase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. Copy `node_modules/code-graph-mcp/templates/code-graph-snapshot.yml` | |
| into your repo's `.github/workflows/`. | |
| 1. Copy `node_modules/@sdsrs/code-graph/templates/code-graph-snapshot.yml` | |
| into your repo's `.github/workflows/`. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~324-~324: The official name of this software platform is spelled with a capital “H”.
Context: ...graph-snapshot.yml into your repo's.github/workflows/`. 2. Push a release tag. The...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 323 - 324, Update the README instruction that copies
the workflow template: replace the incorrect package path
"node_modules/code-graph-mcp/templates/code-graph-snapshot.yml" with the actual
published package path
"node_modules/@sdsrs/code-graph/templates/code-graph-snapshot.yml" so the cp
command matches the npm package referenced elsewhere (npx -y `@sdsrs/code-graph`)
and users won't get "cannot stat" errors.
| /// Resolve where the snapshot lives. Order: | ||
| /// 1. `.code-graph.toml` `[snapshot] url` (must be HTTPS) | ||
| /// 2. `[snapshot] disabled = true` → None | ||
| /// 3. Auto-detect from `git remote get-url origin` → GitHub release asset | ||
| pub fn resolve_snapshot_source(root: &Path) -> Option<String> { | ||
| let cfg = load_config(root).ok()?; | ||
| if cfg.snapshot.disabled { | ||
| return None; | ||
| } | ||
| if let Some(url) = cfg.snapshot.url { | ||
| if url.starts_with("https://") { | ||
| return Some(url); | ||
| } | ||
| tracing::warn!("snapshot url in .code-graph.toml is not https, skipping: {url}"); | ||
| return None; | ||
| } | ||
| resolve_from_github(root) | ||
| } |
There was a problem hiding this comment.
Doc comment lists resolution order in the wrong sequence.
The header lists url first, then disabled, but the code checks disabled first (line 14) and would short-circuit before reading the override URL. Either reorder the doc to match the code, or move the URL check above the disabled short-circuit if the documented behavior is intended.
📝 Proposed doc-only fix
/// Resolve where the snapshot lives. Order:
-/// 1. `.code-graph.toml` `[snapshot] url` (must be HTTPS)
-/// 2. `[snapshot] disabled = true` → None
-/// 3. Auto-detect from `git remote get-url origin` → GitHub release asset
+/// 1. `[snapshot] disabled = true` → None
+/// 2. `.code-graph.toml` `[snapshot] url` (must be HTTPS)
+/// 3. Auto-detect from `git remote get-url origin` → GitHub release asset📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Resolve where the snapshot lives. Order: | |
| /// 1. `.code-graph.toml` `[snapshot] url` (must be HTTPS) | |
| /// 2. `[snapshot] disabled = true` → None | |
| /// 3. Auto-detect from `git remote get-url origin` → GitHub release asset | |
| pub fn resolve_snapshot_source(root: &Path) -> Option<String> { | |
| let cfg = load_config(root).ok()?; | |
| if cfg.snapshot.disabled { | |
| return None; | |
| } | |
| if let Some(url) = cfg.snapshot.url { | |
| if url.starts_with("https://") { | |
| return Some(url); | |
| } | |
| tracing::warn!("snapshot url in .code-graph.toml is not https, skipping: {url}"); | |
| return None; | |
| } | |
| resolve_from_github(root) | |
| } | |
| /// Resolve where the snapshot lives. Order: | |
| /// 1. `[snapshot] disabled = true` → None | |
| /// 2. `.code-graph.toml` `[snapshot] url` (must be HTTPS) | |
| /// 3. Auto-detect from `git remote get-url origin` → GitHub release asset | |
| pub fn resolve_snapshot_source(root: &Path) -> Option<String> { | |
| let cfg = load_config(root).ok()?; | |
| if cfg.snapshot.disabled { | |
| return None; | |
| } | |
| if let Some(url) = cfg.snapshot.url { | |
| if url.starts_with("https://") { | |
| return Some(url); | |
| } | |
| tracing::warn!("snapshot url in .code-graph.toml is not https, skipping: {url}"); | |
| return None; | |
| } | |
| resolve_from_github(root) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/snapshot/install.rs` around lines 8 - 25, The doc comment for
resolve_snapshot_source lists resolution order incorrectly vs the code: the
function currently checks cfg.snapshot.disabled before cfg.snapshot.url and thus
short-circuits; either update the doc to list "disabled" before "url" to match
current behavior, or if the intended behavior is that an explicit
cfg.snapshot.url should override disablement, move the cfg.snapshot.url check
(the if let Some(url) = cfg.snapshot.url) above the cfg.snapshot.disabled check
so the URL is honored first; adjust the doc or the order in
resolve_snapshot_source accordingly.
| fn wait_with_watchdog( | ||
| child: std::process::Child, | ||
| cap: std::time::Duration, | ||
| ) -> Option<std::process::Output> { | ||
| #[cfg(unix)] | ||
| { | ||
| use std::sync::atomic::{AtomicBool, Ordering}; | ||
| use std::sync::Arc; | ||
|
|
||
| let pid = child.id() as i32; | ||
| let cancel = Arc::new(AtomicBool::new(false)); | ||
| let cancel_w = Arc::clone(&cancel); | ||
| let watchdog = std::thread::spawn(move || { | ||
| let start = std::time::Instant::now(); | ||
| while start.elapsed() < cap { | ||
| if cancel_w.load(Ordering::Relaxed) { | ||
| return; | ||
| } | ||
| std::thread::sleep(std::time::Duration::from_millis(100)); | ||
| } | ||
| // Cap exceeded: send SIGTERM. Pid is still alive because cancel | ||
| // is set immediately after wait_with_output returns. | ||
| unsafe { libc::kill(pid, libc::SIGTERM); } | ||
| }); | ||
| let result = child.wait_with_output().ok().filter(|o| o.status.success()); | ||
| cancel.store(true, Ordering::Relaxed); | ||
| let _ = watchdog.join(); | ||
| result | ||
| } | ||
| #[cfg(not(unix))] | ||
| { | ||
| // No watchdog on non-Unix: rely on `gh.exe`'s own timeout behavior. | ||
| let _ = cap; | ||
| child.wait_with_output().ok().filter(|o| o.status.success()) | ||
| } | ||
| } |
There was a problem hiding this comment.
SIGTERM path doesn't re-check cancel — could signal a recycled PID.
The watchdog's while start.elapsed() < cap loop exits unconditionally to libc::kill(pid, SIGTERM) once the cap elapses. If wait_with_output happens to complete near the cap boundary, the cancel.store(true) from the main thread can race with the watchdog's loop-exit decision: the watchdog has already left the loop, the child is reaped, the OS may reuse the PID, and the kill targets an unrelated process. The inline comment claims this is safe because cancel is set "immediately after wait_with_output returns," but the cancel flag is only consulted inside the loop, not after.
Add a final cancel check before libc::kill:
🔒 Proposed guard
while start.elapsed() < cap {
if cancel_w.load(Ordering::Relaxed) {
return;
}
std::thread::sleep(std::time::Duration::from_millis(100));
}
- // Cap exceeded: send SIGTERM. Pid is still alive because cancel
- // is set immediately after wait_with_output returns.
- unsafe { libc::kill(pid, libc::SIGTERM); }
+ // Cap exceeded — re-check cancel one more time so we don't SIGTERM
+ // a child that completed in the same instant the cap fired (PID
+ // could already be reaped and recycled).
+ if !cancel_w.load(Ordering::Relaxed) {
+ unsafe { libc::kill(pid, libc::SIGTERM); }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/snapshot/install.rs` around lines 91 - 126, In wait_with_watchdog, the
watchdog thread may call libc::kill(pid, SIGTERM) after its loop even if the
main thread just set cancel; before sending the signal you must re-check the
cancel flag to avoid killing a recycled PID. Modify the closure spawned into
watchdog (referencing pid and cancel_w) to, after the loop exits due to timeout,
test cancel_w.load(Ordering::Relaxed) and return if true, only calling
libc::kill(pid, libc::SIGTERM) when cancel_w is still false; keep the existing
join/store behavior in the main thread (cancel.store(true) and join).
| if !(url.starts_with("https://") || url.starts_with("file://")) { | ||
| anyhow::bail!("snapshot url must be https:// (got {url})"); | ||
| } |
There was a problem hiding this comment.
Error message contradicts the accepted scheme list.
The check accepts https:// and file://, but the bail message only mentions https. A user feeding e.g. http:// or s3:// will get a message that omits the legitimate file:// test path.
📝 Proposed message tweak
- if !(url.starts_with("https://") || url.starts_with("file://")) {
- anyhow::bail!("snapshot url must be https:// (got {url})");
- }
+ if !(url.starts_with("https://") || url.starts_with("file://")) {
+ anyhow::bail!("snapshot url must be https:// or file:// (got {url})");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !(url.starts_with("https://") || url.starts_with("file://")) { | |
| anyhow::bail!("snapshot url must be https:// (got {url})"); | |
| } | |
| if !(url.starts_with("https://") || url.starts_with("file://")) { | |
| anyhow::bail!("snapshot url must be https:// or file:// (got {url})"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/snapshot/install.rs` around lines 132 - 134, The bail message in the
conditional that checks the snapshot URL (the if !(url.starts_with("https://")
|| url.starts_with("file://")) block) is misleading because it only mentions
"https://" even though "file://" is also accepted; update the anyhow::bail! call
to list both accepted schemes (e.g., "snapshot url must start with https:// or
file:// (got {url})") so the error accurately reflects the allowed URL schemes.
| fn download(url: &str, dest: &Path) -> Result<()> { | ||
| if let Some(file_path) = url.strip_prefix("file://") { | ||
| // file:// is test-only and config-controlled; no path sanitisation. | ||
| std::fs::copy(file_path, dest).context("file:// copy")?; | ||
| return Ok(()); | ||
| } | ||
| // TODO: stream to disk (reqwest copy_to) and apply MAX_DECOMPRESSED_BYTES | ||
| // cap to the compressed payload too — currently buffers the whole response. | ||
| let bytes = reqwest::blocking::Client::builder() | ||
| .timeout(std::time::Duration::from_secs(30)) | ||
| .build()? | ||
| .get(url) | ||
| .send()? | ||
| .error_for_status()? | ||
| .bytes()?; | ||
| std::fs::write(dest, &bytes).context("write download to disk")?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
reqwest blocking Response copy_to writer streaming download
💡 Result:
In reqwest's blocking API, use Response::copy_to(&mut self, writer: &mut W) where W: std::io::Write to stream download a response body to a writer like a File. This internally calls std::io::copy, reading and writing chunks continuously until EOF without loading the full body into memory [1][2]. Example for downloading to a file: use std::fs::File; use reqwest::blocking::get; fn main -> Result<, Box> { let mut response = get("http://httpbin.org/range/1024")?; let mut file = File::create("downloaded.bin")?; let bytes_copied = response.copy_to(&mut file)?; println!("Copied {} bytes", bytes_copied); Ok() }[1][2] This is ideal for large files, avoiding memory issues from response.bytes which buffers everything [3]. The official docs confirm it streams data [1]. For async, use chunk.await in a loop instead [4][5].
Citations:
- 1: https://docs.rs/reqwest/latest/reqwest/blocking/struct.Response.html
- 2: https://docs.rs/reqwest/0.11.6/reqwest/blocking/struct.Response.html
- 3: https://users.rust-lang.org/t/file-download-with-reqwest-blocking-fails-with-file-bigger-than-10mbytes/107488
- 4: async equivalent of
std::io::copy(&mut response, &mut other)seanmonstar/reqwest#896 - 5: https://stackoverflow.com/questions/74283128/copy-reqwest-bytes-stream-in-to-tokio-file
🏁 Script executed:
# Search for MAX_DECOMPRESSED_BYTES constant
rg "MAX_DECOMPRESSED_BYTES" --type rsRepository: sdsrss/code-graph-mcp
Length of output: 92
🏁 Script executed:
# Search for CapWriter implementation
rg "CapWriter|cap.*writer" --type rs -iRepository: sdsrss/code-graph-mcp
Length of output: 92
🏁 Script executed:
# Look at the actual download function in snapshot/install.rs
fd "install.rs" src/ | head -1 | xargs wc -lRepository: sdsrss/code-graph-mcp
Length of output: 93
🏁 Script executed:
# Check for decompression logic that might use MAX_DECOMPRESSED_BYTES
rg "decompress|decompressed" --type rs -A 2 -B 2Repository: sdsrss/code-graph-mcp
Length of output: 92
🏁 Script executed:
# Search for MAX_DECOMPRESSED_BYTES constant
rg "MAX_DECOMPRESSED_BYTES"Repository: sdsrss/code-graph-mcp
Length of output: 362
🏁 Script executed:
# Search for CapWriter implementation
rg "CapWriter|cap.*writer" -iRepository: sdsrss/code-graph-mcp
Length of output: 368
🏁 Script executed:
# Read the download function from install.rs
cat -n src/snapshot/install.rs | sed -n '194,211p'Repository: sdsrss/code-graph-mcp
Length of output: 922
🏁 Script executed:
# Check for decompression logic
rg "decompress|decompressed" -A 2 -B 2Repository: sdsrss/code-graph-mcp
Length of output: 2375
download() buffers the entire response in memory before any cap applies.
reqwest::blocking::Response::bytes() reads the full payload into RAM before the cap-aware decoder runs. A malicious or misconfigured release endpoint serving a multi-GB body will OOM the process well before MAX_DECOMPRESSED_BYTES is consulted on the decompressed side. The TODO comment on line 200 already acknowledges this, but for a feature that runs unattended on MCP server startup it's worth implementing now rather than deferring.
reqwest::blocking::Response::copy_to(&mut writer) streams to a writer in chunks — wrapping the destination file in the existing CapWriter gives a compressed-side cap with no extra deps.
🛡️ Sketch of streaming + compressed cap
const MAX_COMPRESSED_BYTES: u64 = 200 * 1024 * 1024; // ~2× zstd worst case
let mut resp = reqwest::blocking::Client::builder()
.timeout(std::time::Duration::from_secs(30))
.build()?
.get(url)
.send()?
.error_for_status()?;
let mut out = std::fs::File::create(dest).context("create download file")?;
let mut capped = CapWriter::new(&mut out, MAX_COMPRESSED_BYTES);
resp.copy_to(&mut capped).context("stream download to disk")?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/snapshot/install.rs` around lines 194 - 211, The download() function
currently buffers the entire HTTP response via Response::bytes(), which can OOM;
change it to stream the response to disk and enforce a compressed-size cap by
using Response::copy_to with a capped writer instead of bytes(). Specifically,
in download() (and the HTTP branch where reqwest::blocking::Client is built and
.get(url).send() is called) create the destination file
(std::fs::File::create(dest)), wrap it in the existing CapWriter with a
MAX_COMPRESSED_BYTES constant, and call resp.copy_to(&mut capped_writer)
(checking error_for_status() first) so the compressed payload is limited and
written incrementally to disk rather than fully buffered in memory.
| // Decompress to a temp file so we can open with rusqlite | ||
| let tmp = tempfile::tempdir().context("inspect tempdir")?; | ||
| let decompressed = tmp.path().join("snapshot.db"); | ||
| let compressed = std::fs::read(file).context("read snapshot file")?; | ||
| let raw = zstd::decode_all(&compressed[..]).context("zstd decode")?; | ||
| std::fs::write(&decompressed, &raw).context("write decompressed snapshot")?; | ||
|
|
||
| let db = Database::open(&decompressed)?; |
There was a problem hiding this comment.
Keep snapshot inspect bounded and read-only.
This path reads the entire archive and the entire decompressed DB into memory, then opens it with Database::open(). A malformed or unexpectedly large asset can OOM the CLI, and Database::open() may apply migrations to the temp copy so the reported counts no longer strictly reflect the artifact bytes. Please reuse the install path’s capped decompression logic here and open the temp DB read-only for inspection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/snapshot/mod.rs` around lines 96 - 103, The current inspect path leaks
memory by reading the whole compressed file and using zstd::decode_all then
calling Database::open (which may apply migrations); replace that with the
install path’s capped, streaming decompression routine (do not use
zstd::decode_all or read the full file into a Vec) to write at most the
configured max bytes into the temp snapshot.db, and open the resulting temp DB
read-only (use the install helper or rusqlite open-with-flags; replace
Database::open(&decompressed)? with a read-only open such as
Database::open_read_only or opening via
rusqlite::Connection::open_with_flags(..., OpenFlags::SQLITE_OPEN_READ_ONLY)).
Ensure you reference and remove the use of the variables/steps
compressed/zstd::decode_all/raw and instead call the capped streaming
decompressor used by the install code and then open the temp DB in read-only
mode for inspection.
| Command::new("git").args(["init", "-q"]).current_dir(p).status().unwrap(); | ||
| Command::new("git").args(["config", "user.email", "t@t"]).current_dir(p).status().unwrap(); | ||
| Command::new("git").args(["config", "user.name", "t"]).current_dir(p).status().unwrap(); | ||
| std::fs::create_dir_all(p.join("src")).unwrap(); | ||
| std::fs::write(p.join("src/lib.rs"), "pub fn h() {}\n").unwrap(); | ||
| Command::new("git").args(["add", "."]).current_dir(p).status().unwrap(); | ||
| Command::new("git").args(["commit", "-q", "-m", "init"]).current_dir(p).status().unwrap(); |
There was a problem hiding this comment.
Assert the git setup commands actually succeeded.
status().unwrap() only proves the process spawned. If git init/git commit exits non-zero here, the repo fixture is half-built and the later snapshot assertions fail far away from the real cause. Please check status.success() (or wrap these calls in a tiny helper) so setup failures surface immediately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_snapshot.rs` around lines 13 - 19, The git setup commands in the
test (the Command::new calls for "git init", "git config", "git add", "git
commit") only call status().unwrap(), which only ensures the process started but
not that it exited successfully; change these to assert the exit status
succeeded by checking status.success() (or replace repeated calls with a small
helper like run_and_assert(cmd) that runs the command, calls
.status().expect("spawn failed") and then assert!(status.success(), "git
<action> failed: {:?}", status)), referencing the Command invocations shown in
the test (e.g., the Command::new("git").args([...]).current_dir(p).status()
calls) so any setup failure fails the test immediately with a clear message.
1 participant
Summary
Adds a producer/consumer pair for
code-graph-snapshot-<sha>.db.zstGitHub Release artifacts so fresh clones skip the initial full code-graph index. Snapshot is an optimization, not a dependency — every failure mode falls back to the existingrun_full_indexpath.Producer:
code-graph-mcp snapshot create|inspectCLI +claude-plugin/templates/code-graph-snapshot.ymlworkflow template (users copy to their.github/workflows/).Consumer:
Server::from_project_rootruns snapshot install BEFOREself.dbopens (Approach A — avoids POSIX rename inode-swap).ensure_indexedroutes to incremental when snapshot data is present.File: symbols + edges + FTS5 only (no
node_vectors) → decouples from embedding model choice. ~3-5MB compressed.Spec:
docs/superpowers/specs/2026-05-10-shared-graph-snapshot-design.md(local-only).What's in this PR
src/snapshot/{mod,meta,install,config}.rs+snapshot create|inspectCLI + workflow template + README docs.code-graph.tomlconfig +resolve_snapshot_source(toml override +gh apiauto-detect with 5s SIGTERM watchdog) +try_install(download → zstd cap 100MB → schema validate → atomic rename → meta) +ensure_indexedhook +reindex --from-snapshotCLIhealth-check --jsonexposes snapshot status / source / driftBench (--ignored, on 5K-node fixture)
create()runtimetry_install()(file://)Final review fixes (commit 552729d)
Whole-implementation review surfaced 3 Important issues, all addressed:
final_dbunder a post-rename meta-write racegh apiwatchdog is cancellation-aware (doesn't sleep 5s and SIGTERM a recycled PID after fast response)libc::killproperly#[cfg(unix)]-gated for Windows binaryKnown follow-ups (not blocking)
inspect()reads whole compressed file into RAM (CLI-only, bounded by 100MB cap)Server::try_install_snapshotis currently dead — Task 11cmd_reindexcallscrate::snapshot::*directly; can prune in a follow-upcmd_reindex --from-snapshotlacks dedicated e2e test (integration tests cover the install path)install.rs::download)Test plan
cargo test --no-default-features— 320 lib + 62 + 46 + 55 + 19 + 6 + 6 + 6 + 3 + 3 + 2 = all passcargo +1.95.0 clippy --no-default-features -- -D warnings— cleancargo +1.95.0 clippy --all-targets -- -D warnings— cleancargo test --test snapshot_bench -- --ignored— 3 pass, all targets metsnapshot create→snapshot inspectround-trip on real repo22 commits, +1670/-44, 25 files. Each commit individually reviewed (spec + code quality stages per task).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
snapshot createandsnapshot inspectCLI commands for managing compressed graph snapshots.reindex --from-snapshotcommand to bootstrap indexing from a published snapshot.Documentation
Chores