Finetune to reduce LLM token - #5
Conversation
WalkthroughThe change adds deadline-aware resumable graph searches with server-side session storage. MCP sessions now support configurable symbol detail and output formats, normalized paths, omitted default fields, and resumable search responses. Context Markdown rendering supports optional root-prefix removal. ChangesResumable search and session storage
MCP output and path handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPTools
participant GraphApi
participant SearchSessionStore
participant GraphIndex
Client->>MCPTools: codegraph_search_symbol(query, timeout, resume)
MCPTools->>GraphApi: search_symbol_paged_resumable(...)
GraphApi->>SearchSessionStore: get(resume)
GraphApi->>GraphIndex: search_symbol_paged_resumable(cursor, deadline)
GraphIndex-->>GraphApi: page, progress, continuation
GraphApi->>SearchSessionStore: put or remove session
GraphApi-->>MCPTools: ResumeSearchOutcome
MCPTools-->>Client: formatted response with resume metadata
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/codegraph-mcp/src/usage.rs (1)
120-128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDeduplicate on the normalized path, not the raw path.
The lookup normalizes with
strip_root_prefix, butseenstores the raw answer pathp. If the same file appears once as an absolute path and once already relativized, both forms map to the same entry and its bytes are counted twice.🐛 Proposed fix
let mut seen = std::collections::HashSet::new(); let mut total = 0u64; for p in paths { - if let Some(b) = bytes_by_path.get(crate::tools::strip_root_prefix(&p, root)) { - if seen.insert(p) { + let key = crate::tools::strip_root_prefix(&p, root).to_string(); + if let Some(b) = bytes_by_path.get(key.as_str()) { + if seen.insert(key) { total += b; } } }🤖 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 `@crates/codegraph-mcp/src/usage.rs` around lines 120 - 128, Update the deduplication in the path-totaling loop to use the normalized path produced by strip_root_prefix as the HashSet key, while using that same normalized value for bytes_by_path lookup. Preserve counting each file only once when absolute and relative representations resolve to the same path.crates/codegraph-mcp/src/session.rs (1)
184-213: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA re-run of
codegraph_initwithoutdetailresets the session detail level.Session::initacceptsdetailas a requiredDetailLeveland always writes it, while it acceptsformatas anOption<OutputStyle>and writes it only when present. The caller converts an absentdetailargument intoDetailLevel::default(), so the two settings behave differently for the same "argument omitted" case.
crates/codegraph-mcp/src/session.rs#L184-L213: change the parameter todetail: Option<DetailLevel>and writeself.detailonly when it isSome, matching the existingformathandling.crates/codegraph-mcp/src/lib.rs#L131-L153: drop.unwrap_or_default()so the parsed value staysOption<DetailLevel>, pass it through toinit, and reportself.session.detail().await.as_str()in the response instead of the localdetail.🤖 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 `@crates/codegraph-mcp/src/session.rs` around lines 184 - 213, Update Session::init in crates/codegraph-mcp/src/session.rs:184-213 to accept detail: Option<DetailLevel> and only write self.detail when it is Some, matching format handling. Update crates/codegraph-mcp/src/lib.rs:131-153 to remove unwrap_or_default(), pass the optional detail through to init, and report self.session.detail().await.as_str() rather than the local value.
🧹 Nitpick comments (5)
crates/codegraph-context/src/lib.rs (1)
146-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for prefix stripping.
The supplied
crates/codegraph-api/tests/api.rstest passesstrip_prefix: None. It checks only the unchanged behavior. Add a Markdown test withSome(root)and assert relative paths for the primary symbol, callers, and callees. Include a boundary case such as/repo-old/src/a.tswith/repo.Also applies to: 158-176
🤖 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 `@crates/codegraph-context/src/lib.rs` at line 146, Add regression coverage for prefix stripping in the Markdown/API test flow by using strip_prefix: Some(root). Assert that relative paths are correct for the primary symbol, callers, and callees, and add a boundary case such as /repo-old/src/a.ts with /repo to ensure only a complete path prefix is stripped.crates/codegraph-graph/src/lib.rs (3)
1759-1793: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
ExactandPrefixscan every distinct name.The
ScanNamesphase walkssorted_name_keyselement by element for all non-Containsmodes.Exactcan resolve throughname_indexin constant time.Prefixcan usepartition_pointon the sorted vector and stop at the first non-matching key. OnlySuffixneeds a full scan.The current form makes an exact-name lookup O(number of distinct names) per call, and it also creates unnecessary resume round-trips on large indexes.
♻️ Suggested fast paths
SearchCursorPhase::ScanNames { name_pos } => { + // Exact: hash lookup — không cần quét. + if mode == SymbolMatch::Exact { + let matched = if self.name_index.contains_key(&q) { + vec![q.clone()] + } else { + Vec::new() + }; + phase = SearchCursorPhase::Expand { + names: matched, + name_idx: 0, + id_idx: 0, + collected: Vec::new(), + }; + } else { let mut matched: Vec<String> = Vec::new(); let mut pos = *name_pos;For
Prefix, startposatself.sorted_name_keys.partition_point(|n| n.as_str() < q.as_str())and stop as soon as a key no longer starts withq.🤖 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 `@crates/codegraph-graph/src/lib.rs` around lines 1759 - 1793, Optimize the ScanNames branch in the search flow: resolve SymbolMatch::Exact directly through name_index instead of scanning sorted_name_keys, and for SymbolMatch::Prefix initialize pos with partition_point and stop when keys no longer start with q. Preserve the existing resumable scan behavior for timeouts and retain the full scan only for SymbolMatch::Suffix.
141-143: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sorted_name_keysduplicatesname_records.
rebuild_name_enginebuildssorted_name_keysfromdistinctand then pushes the same strings, in the same order, intoname_records. Both vectors hold identical content after the loop. On a large index this doubles the memory used for distinct names.The scan phase in
search_symbol_paged_resumablecan indexname_recordsdirectly, because it is already sorted by construction.♻️ Suggested consolidation
- let mut record = 0usize; - self.sorted_name_keys = distinct.iter().map(|s| s.to_string()).collect(); + let mut record = 0usize;Then replace
self.sorted_name_keysreads insearch_symbol_paged_resumablewithself.name_records, and drop the field plus its reset at line 650.Also applies to: 597-597
🤖 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 `@crates/codegraph-graph/src/lib.rs` around lines 141 - 143, Remove the redundant sorted_name_keys field and its reset, since rebuild_name_engine already populates name_records with the sorted distinct names. Update search_symbol_paged_resumable to use name_records for all existing sorted-name scan and indexing reads, preserving the current ordering and pagination behavior.
1798-1839: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCheck the deadline in batches, not per symbol id.
The phase B loop calls
Instant::now()for every single id it appends. On a large expansion this clock read dominates the useful work. A counter that checks the deadline every N iterations (for example 1024) keeps cancellation responsive and removes most clock reads.♻️ Suggested batching
loop { - if let Some(dl) = deadline - && Instant::now() >= dl - { - timed_out = true; - break; - } + steps += 1; + if steps % 1024 == 0 + && let Some(dl) = deadline + && Instant::now() >= dl + { + timed_out = true; + break; + }Declare
let mut steps = 0usize;before the loop. Apply the same pattern to theScanNamesloop.🤖 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 `@crates/codegraph-graph/src/lib.rs` around lines 1798 - 1839, Update the Phase B expansion loop in the SearchCursorPhase::Expand branch to track a steps counter and check the deadline only once per batch, such as every 1024 iterations, while preserving timeout behavior. Apply the same batched deadline-check pattern to the ScanNames loop, using a counter initialized before each loop and incremented for processed entries.crates/codegraph-api/tests/api.rs (1)
262-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for repeated paging through the session store.
This test performs three calls, so the store never reaches its 512-entry cap. Each successful call with remaining pages stores a new session entry (see
crates/codegraph-api/src/lib.rslines 261-271). A longer paging loop would expose the eviction of the still-usedresume_id.Add a test that pages through the full result set with the same
resume_idand asserts that the id stays valid until completion.🤖 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 `@crates/codegraph-api/tests/api.rs` around lines 262 - 316, Extend search_symbol_paged_resume_paging into a loop that requests every page using the original resume_id, advancing the offset by the page size and asserting each call succeeds with the expected page size until the final page. Verify the same resume_id remains valid throughout paging and that the final response has no resume token, covering session-store eviction beyond 512 entries.
🤖 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 `@crates/codegraph-api/src/lib.rs`:
- Around line 90-94: Update CodegraphCursorStore::get to enforce the configured
TTL before returning an entry, removing expired cursors and returning None when
their age exceeds the TTL; preserve the existing tuple and clone behavior for
valid entries. Keep the documentation consistent with this behavior.
- Around line 261-271: Update the session handling around resume_id so continued
pages refresh the existing session identified by resume in place when it is
valid, rather than always calling sessions.put to create a new id. Preserve
creation of a new session when no resume id was supplied, and continue removing
the session when out.cursor is None.
In `@crates/codegraph-context/src/lib.rs`:
- Around line 121-133: Update rel_path to use camino::Utf8Path::strip_prefix for
platform-aware prefix matching, handling both slash styles and trailing
separators while returning the original p when stripping fails. Add focused
tests covering trailing `/`, Windows `\` separators, and unmatched prefixes.
In `@crates/codegraph-graph/src/lib.rs`:
- Around line 1729-1758: Update the search flow around codegraph_search_symbol
and the SearchCursorPhase::Engine branch to detect an empty query when the
search mode is Contains, after validating the resume state, and return an empty
PagedSearchOutcome without invoking search_resumable. Preserve normal engine
behavior for non-empty queries and other search modes; alternatively, map the
resulting NotFound error to an empty result consistently with the legacy helper.
In `@crates/codegraph-graph/src/radix.rs`:
- Around line 837-855: Update the DfsState::Collect handling in
Radix::search_dfs so each node’s record is appended only on its initial stack
visit, while subsequent child-index resumes process children without rereading
or pushing the record. Preserve pre-order traversal, MAX_RESULTS accounting, and
checkpoint contents without duplicate records.
In `@crates/codegraph-graph/src/search.rs`:
- Around line 572-613: Fix the depth-filter resume flow in the search resolve
block so resume state preserves the original unfiltered record_ids while also
retaining the already-filtered output. Add a dedicated resolved-results field to
SearchResume and restore/use it across retries, or restart filtering from
resolve_idx zero; ensure resumed searches do not discard previously accepted
records or report premature completion.
In `@crates/codegraph-mcp/src/server-instructions.md`:
- Around line 85-89: Update the fenced code block in the instructions text to
declare the text language, using a text fence for the plain error message and
preserving its contents unchanged.
In `@crates/codegraph-mcp/src/tools.rs`:
- Around line 658-693: Normalize the not-found response in the
codegraph_function_scope handler so the "function" key has the same shape in
both branches. In the None branch, emit the resolved symbol through symbol_json
using the existing detail and format values, or use a distinct key for the plain
name instead; prefer preserving "function" with symbol_json.
---
Outside diff comments:
In `@crates/codegraph-mcp/src/session.rs`:
- Around line 184-213: Update Session::init in
crates/codegraph-mcp/src/session.rs:184-213 to accept detail:
Option<DetailLevel> and only write self.detail when it is Some, matching format
handling. Update crates/codegraph-mcp/src/lib.rs:131-153 to remove
unwrap_or_default(), pass the optional detail through to init, and report
self.session.detail().await.as_str() rather than the local value.
In `@crates/codegraph-mcp/src/usage.rs`:
- Around line 120-128: Update the deduplication in the path-totaling loop to use
the normalized path produced by strip_root_prefix as the HashSet key, while
using that same normalized value for bytes_by_path lookup. Preserve counting
each file only once when absolute and relative representations resolve to the
same path.
---
Nitpick comments:
In `@crates/codegraph-api/tests/api.rs`:
- Around line 262-316: Extend search_symbol_paged_resume_paging into a loop that
requests every page using the original resume_id, advancing the offset by the
page size and asserting each call succeeds with the expected page size until the
final page. Verify the same resume_id remains valid throughout paging and that
the final response has no resume token, covering session-store eviction beyond
512 entries.
In `@crates/codegraph-context/src/lib.rs`:
- Line 146: Add regression coverage for prefix stripping in the Markdown/API
test flow by using strip_prefix: Some(root). Assert that relative paths are
correct for the primary symbol, callers, and callees, and add a boundary case
such as /repo-old/src/a.ts with /repo to ensure only a complete path prefix is
stripped.
In `@crates/codegraph-graph/src/lib.rs`:
- Around line 1759-1793: Optimize the ScanNames branch in the search flow:
resolve SymbolMatch::Exact directly through name_index instead of scanning
sorted_name_keys, and for SymbolMatch::Prefix initialize pos with
partition_point and stop when keys no longer start with q. Preserve the existing
resumable scan behavior for timeouts and retain the full scan only for
SymbolMatch::Suffix.
- Around line 141-143: Remove the redundant sorted_name_keys field and its
reset, since rebuild_name_engine already populates name_records with the sorted
distinct names. Update search_symbol_paged_resumable to use name_records for all
existing sorted-name scan and indexing reads, preserving the current ordering
and pagination behavior.
- Around line 1798-1839: Update the Phase B expansion loop in the
SearchCursorPhase::Expand branch to track a steps counter and check the deadline
only once per batch, such as every 1024 iterations, while preserving timeout
behavior. Apply the same batched deadline-check pattern to the ScanNames loop,
using a counter initialized before each loop and incremented for processed
entries.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b81d5972-e9f1-43c3-8d25-031d64a0efa4
📒 Files selected for processing (16)
Cargo.tomlcrates/codegraph-api/src/lib.rscrates/codegraph-api/tests/api.rscrates/codegraph-context/src/lib.rscrates/codegraph-core/src/semgraph.rscrates/codegraph-graph/src/lib.rscrates/codegraph-graph/src/radix.rscrates/codegraph-graph/src/search.rscrates/codegraph-graph/src/shared.rscrates/codegraph-mcp/src/lib.rscrates/codegraph-mcp/src/server-instructions.mdcrates/codegraph-mcp/src/session.rscrates/codegraph-mcp/src/stdio.rscrates/codegraph-mcp/src/tools.rscrates/codegraph-mcp/src/usage.rscrates/codegraph/src/main.rs
| /// Đọc cursor theo id — `None` nếu không có / quá TTL. | ||
| pub fn get(&self, id: &str) -> Option<(u64, SearchCursor)> { | ||
| let map = self.inner.lock().unwrap(); | ||
| map.get(id).map(|s| (s.index_version, s.cursor.clone())) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
get does not apply the TTL.
The doc comment states that get returns None when the entry is older than the TTL. The body only performs a map lookup. Expiry happens only inside put. If no put runs, an expired cursor stays reachable.
Either enforce the TTL in get, or correct the doc comment.
🔧 Proposed fix
/// Đọc cursor theo id — `None` nếu không có / quá TTL.
pub fn get(&self, id: &str) -> Option<(u64, SearchCursor)> {
let map = self.inner.lock().unwrap();
- map.get(id).map(|s| (s.index_version, s.cursor.clone()))
+ let s = map.get(id)?;
+ if Instant::now().duration_since(s.created) >= self.ttl {
+ return None;
+ }
+ Some((s.index_version, s.cursor.clone()))
}📝 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.
| /// Đọc cursor theo id — `None` nếu không có / quá TTL. | |
| pub fn get(&self, id: &str) -> Option<(u64, SearchCursor)> { | |
| let map = self.inner.lock().unwrap(); | |
| map.get(id).map(|s| (s.index_version, s.cursor.clone())) | |
| } | |
| /// Đọc cursor theo id — `None` nếu không có / quá TTL. | |
| pub fn get(&self, id: &str) -> Option<(u64, SearchCursor)> { | |
| let map = self.inner.lock().unwrap(); | |
| let s = map.get(id)?; | |
| if Instant::now().duration_since(s.created) >= self.ttl { | |
| return None; | |
| } | |
| Some((s.index_version, s.cursor.clone())) | |
| } |
🤖 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 `@crates/codegraph-api/src/lib.rs` around lines 90 - 94, Update
CodegraphCursorStore::get to enforce the configured TTL before returning an
entry, removing expired cursors and returning None when their age exceeds the
TTL; preserve the existing tuple and clone behavior for valid entries. Keep the
documentation consistent with this behavior.
| // ── Quản lý session: lưu khi còn tiếp tục (timeout / còn page), xoá | ||
| // khi xong hẳn. ── | ||
| let resume_id = match &out.cursor { | ||
| Some(c) => Some(self.sessions.put(c.clone(), version)), | ||
| None => { | ||
| if let Some(id) = &resume { | ||
| self.sessions.remove(id); | ||
| } | ||
| None | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Each continued page stores a new session and never releases the previous one.
When out.cursor is Some, this code always calls put and returns a fresh id. The id supplied in resume is removed only when the search completes. The integration test crates/codegraph-api/tests/api.rs (lines 283-316) reuses the same resume_id across pages, so the caller keeps using the original id while the store fills with unused clones.
Two consequences:
- Every paged call consumes one slot of the 512-slot store.
- Eviction removes the oldest entry first. The original id that the caller still uses is the oldest, so it can be evicted by its own clones. The next retry then fails with "resume id expired or unknown".
Refresh the existing session in place instead of creating a new one when the caller passed a valid resume.
🔧 Proposed direction
let resume_id = match &out.cursor {
- Some(c) => Some(self.sessions.put(c.clone(), version)),
+ // Cùng một search đang tiếp tục → cập nhật session cũ, giữ nguyên id.
+ Some(c) => match &resume {
+ Some(id) => {
+ self.sessions.update(id, c.clone(), version);
+ Some(id.clone())
+ }
+ None => Some(self.sessions.put(c.clone(), version)),
+ },
None => {
if let Some(id) = &resume {
self.sessions.remove(id);
}
None
}
};🤖 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 `@crates/codegraph-api/src/lib.rs` around lines 261 - 271, Update the session
handling around resume_id so continued pages refresh the existing session
identified by resume in place when it is valid, rather than always calling
sessions.put to create a new id. Preserve creation of a new session when no
resume id was supplied, and continue removing the session when out.cursor is
None.
| /// Strip `root/` prefix khỏi path (boundary-aware) — `None` giữ nguyên. | ||
| fn rel_path<'a>(p: &'a str, strip: Option<&str>) -> &'a str { | ||
| if let Some(root) = strip { | ||
| if let Some(rest) = p.strip_prefix(root) { | ||
| if let Some(rest) = rest.strip_prefix('/') { | ||
| return rest; | ||
| } | ||
| } | ||
| } | ||
| p | ||
| } | ||
|
|
||
| fn render_markdown(resp: &ContextResponse, strip: Option<&str>) -> String { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(crates/codegraph-context|.*context.*|.*path.*|.*test.*)$' | head -200
printf '%s\n' '--- rel_path and related symbols ---'
rg -n -C 8 'fn rel_path|render_markdown|strip_prefix|ContextRequest|ContextResponse' crates/codegraph-context 2>/dev/null || true
printf '%s\n' '--- file outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline crates/codegraph-context/src/lib.rs
else
wc -l crates/codegraph-context/src/lib.rs
fiRepository: hungpham10/codegraph-rs
Length of output: 9201
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- rendering implementation ---'
sed -n '133,240p' crates/codegraph-context/src/lib.rs
printf '%s\n' '--- context request usages ---'
rg -n -C 5 'ContextRequest\s*\{|strip_prefix|file:' crates --glob '*.rs' --glob '*.md' --glob '*.toml' | head -300
printf '%s\n' '--- path conventions and targets ---'
rg -n -C 3 'target|cfg\(target|PathBuf|std::path::Path|replace\(.*\\\\|replace\(.*\/|canonical|workspace root|workspace_root' Cargo.toml crates docs --glob '*.rs' --glob '*.toml' --glob '*.md' | head -300
printf '%s\n' '--- package metadata ---'
cat crates/codegraph-context/Cargo.tomlRepository: hungpham10/codegraph-rs
Length of output: 44828
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import PurePosixPath, PureWindowsPath
def current(p, root):
rest = p[len(root):] if p.startswith(root) else None
if rest is not None and rest.startswith('/'):
return rest[1:]
return p
cases = [
("/workspace/src/a.ts", "/workspace/"),
("/workspace/src/a.ts", "/workspace"),
("/src/a.ts", "/"),
(r"C:\\workspace\\src\\a.ts", r"C:\\workspace\\"),
(r"C:\\workspace\\src\\a.ts", r"C:\\workspace"),
]
for p, root in cases:
print(f"current({p!r}, {root!r}) -> {current(p, root)!r}")
print(f"posix.relative_to -> ", end="")
try:
print(repr(str(PurePosixPath(p).relative_to(PurePosixPath(root)))))
except ValueError as e:
print(type(e).__name__)
print(f"windows.relative_to -> ", end="")
try:
print(repr(str(PureWindowsPath(p).relative_to(PureWindowsPath(root)))))
except ValueError as e:
print(type(e).__name__)
PYRepository: hungpham10/codegraph-rs
Length of output: 865
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MCP root state and initialization ---'
rg -n -C 8 'struct .*root|root:|Utf8PathBuf|codegraph_context|strip_root_prefix' crates/codegraph-mcp/src --glob '*.rs' | head -400
printf '%s\n' '--- extraction path flow ---'
sed -n '1,120p' crates/codegraph-extract/src/walker.rs
sed -n '1,120p' crates/codegraph-extract/src/orchestrator.rs
rg -n -C 5 'FileMatch|\.path|parse_file|parse_project|to_string\(\)|as_str\(\)' crates/codegraph-extract/src --glob '*.rs' | head -400
printf '%s\n' '--- workspace camino dependency ---'
rg -n -C 3 'camino' Cargo.toml crates --glob 'Cargo.toml'Repository: hungpham10/codegraph-rs
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
if command -v rustc >/dev/null 2>&1; then
cat >/tmp/rel_path_probe.rs <<'RS'
use std::path::Path;
fn main() {
let cases = [
("/workspace/src/a.ts", "/workspace/"),
("/workspace/src/a.ts", "/workspace"),
("/src/a.ts", "/"),
("C:\\workspace\\src\\a.ts", "C:\\workspace\\"),
("C:\\workspace\\src\\a.ts", "C:\\workspace"),
];
for (p, root) in cases {
let result = Path::new(p)
.strip_prefix(Path::new(root))
.and_then(|rest| rest.to_str())
.map(str::to_owned);
println!("{p:?} / {root:?} => {result:?}");
}
}
RS
rustc /tmp/rel_path_probe.rs -o /tmp/rel_path_probe
/tmp/rel_path_probe
else
printf '%s\n' 'rustc unavailable; exact Rust probe skipped'
fiRepository: hungpham10/codegraph-rs
Length of output: 752
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- focused root initialization ---'
rg -n -C 12 'pub async fn init|fn init|with_root_and_format|Session::with_root|root =|root:' crates/codegraph-mcp/src/session.rs crates/codegraph-mcp/src/lib.rs
printf '%s\n' '--- focused parse path handoff ---'
rg -n -C 10 'fn parse_one|parse_file\(' crates/codegraph-extract/src/orchestrator.rs crates/codegraph-extract/src/lib.rs
printf '%s\n' '--- context tests and workspace test declarations ---'
rg -n -C 5 'mod tests|#\[test\]|render_markdown|rel_path' crates/codegraph-context/src/lib.rs crates/codegraph-context/tests 2>/dev/null || true
printf '%s\n' '--- root normalization calls ---'
rg -n -C 4 'canonicalize|normalize|clean|trim_end_matches|Utf8PathBuf::from\(path\)|init\(' crates/codegraph-mcp/src/session.rs crates/codegraph-mcp/src/lib.rs crates/codegraph-mcp/src/main.rs 2>/dev/null || trueRepository: hungpham10/codegraph-rs
Length of output: 36694
🏁 Script executed:
#!/bin/bash
set -eu
if command -v rustc >/dev/null 2>&1; then
cat >/tmp/rel_path_probe.rs <<'RS'
use std::path::Path;
fn main() {
let cases = [
("/workspace/src/a.ts", "/workspace/"),
("/workspace/src/a.ts", "/workspace"),
("/src/a.ts", "/"),
("C:\\workspace\\src\\a.ts", "C:\\workspace\\"),
("C:\\workspace\\src\\a.ts", "C:\\workspace"),
];
for (p, root) in cases {
let result = Path::new(p)
.strip_prefix(Path::new(root))
.ok()
.and_then(|rest| rest.to_str())
.map(str::to_owned);
println!("{p:?} / {root:?} => {result:?}");
}
}
RS
rustc /tmp/rel_path_probe.rs -o /tmp/rel_path_probe
/tmp/rel_path_probe
else
printf '%s\n' 'rustc unavailable; exact Rust probe skipped'
fiRepository: hungpham10/codegraph-rs
Length of output: 422
Use platform-aware path prefix matching.
rel_path removes only /. It fails for a trailing separator, /, and Windows \ separators. Use camino::Utf8Path::strip_prefix and preserve p when stripping fails. Add tests for these cases.
🤖 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 `@crates/codegraph-context/src/lib.rs` around lines 121 - 133, Update rel_path
to use camino::Utf8Path::strip_prefix for platform-aware prefix matching,
handling both slash styles and trailing separators while returning the original
p when stripping fails. Add focused tests covering trailing `/`, Windows `\`
separators, and unmatched prefixes.
| match &mut phase { | ||
| SearchCursorPhase::Engine(sr) => { | ||
| let page = self | ||
| .names | ||
| .search_resumable(q.as_bytes(), None, Some(sr.clone()), deadline) | ||
| .await?; | ||
| if page.timed_out { | ||
| phase = SearchCursorPhase::Engine(page.resume.unwrap_or_default()); | ||
| timed_out = true; | ||
| } else { | ||
| // record → tên, sort → Expand. | ||
| let mut names: Vec<String> = page | ||
| .record_ids | ||
| .iter() | ||
| .filter_map(|&r| { | ||
| if r == 0 { | ||
| return None; | ||
| } | ||
| self.name_records.get(r - 1).cloned() | ||
| }) | ||
| .collect(); | ||
| names.sort(); | ||
| phase = SearchCursorPhase::Expand { | ||
| names, | ||
| name_idx: 0, | ||
| id_idx: 0, | ||
| collected: Vec::new(), | ||
| }; | ||
| for &id in name_ids { | ||
| if seen.insert(id) { | ||
| ids.push(id); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of the paged/resumable search and check query validation upstream.
rg -nP -C4 '\bsearch_symbol_paged(_resumable)?\s*\(' --type=rust
rg -nP -C4 '\bsearch_resumable\s*\(' --type=rust crates/codegraph-api crates/codegraph-mcpRepository: hungpham10/codegraph-rs
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== candidate files =='
fd -t f 'lib.rs|.*\.rs$' crates/codegraph-graph crates/codegraph-api crates/codegraph-mcp 2>/dev/null | head -200
printf '%s\n' '== paged search references =='
rg -n -C5 'search_symbol_paged|search_symbol_filtered|search_resumable|struct SearchCursor|PagedSearchOutcome' crates --glob '*.rs' || true
printf '%s\n' '== target outline =='
ast-grep outline crates/codegraph-graph/src/lib.rs --match 'search_symbol_paged_resumable' --view expanded 2>/dev/null || trueRepository: hungpham10/codegraph-rs
Length of output: 39514
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== generic search implementation =='
sed -n '430,590p' crates/codegraph-graph/src/search.rs
printf '%s\n' '== old helper =='
sed -n '1128,1170p' crates/codegraph-graph/src/lib.rs
printf '%s\n' '== paged implementation setup/finalization =='
sed -n '1660,1900p' crates/codegraph-graph/src/lib.rs
printf '%s\n' '== API forwarding =='
sed -n '150,270p' crates/codegraph-api/src/lib.rs
printf '%s\n' '== MCP query extraction =='
sed -n '285,340p' crates/codegraph-mcp/src/tools.rs
sed -n '465,515p' crates/codegraph-mcp/src/tools.rsRepository: hungpham10/codegraph-rs
Length of output: 26800
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
files = [
Path("crates/codegraph-graph/src/search.rs"),
Path("crates/codegraph-graph/src/lib.rs"),
Path("crates/codegraph-api/src/lib.rs"),
Path("crates/codegraph-mcp/src/tools.rs"),
]
terms = ("is_empty", "trim", "query", "search_symbol_paged_resumable", "search_resumable")
for path in files:
text = path.read_text()
print(f"== {path} ==")
for i, line in enumerate(text.splitlines(), 1):
if any(term in line for term in terms):
print(f"{i}: {line}")
PYRepository: hungpham10/codegraph-rs
Length of output: 8671
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== argument parser =='
sed -n '930,975p' crates/codegraph-mcp/src/tools.rs
printf '%s\n' '== match-mode parsing =='
rg -n -C8 'impl.*SymbolMatch|enum SymbolMatch|SymbolMatch::parse|fn parse' crates/codegraph-graph/src/lib.rs crates/codegraph-api/src/lib.rs crates/codegraph-mcp/src/tools.rs
printf '%s\n' '== focused behavioral verifier =='
python3 - <<'PY'
from pathlib import Path
search = Path("crates/codegraph-graph/src/search.rs").read_text()
graph = Path("crates/codegraph-graph/src/lib.rs").read_text()
mcp = Path("crates/codegraph-mcp/src/tools.rs").read_text()
api = Path("crates/codegraph-api/src/lib.rs").read_text()
assert "if pattern.is_empty() {\n return Err(Error::NotFound);\n }" in search
assert "let page = self.search_resumable(q.as_bytes(), None, Some(sr.clone()), deadline)" in graph
assert "let q = query.to_lowercase();" in graph
paged = graph[graph.index("pub async fn search_symbol_paged_resumable"):graph.index("/// Resumable + deadline-aware", graph.index("pub async fn search_symbol_paged_resumable"))]
assert "if q.is_empty()" not in paged
old = graph[graph.index("async fn search_symbol_filtered"):graph.index("/// Search symbol nâng cao", graph.index("async fn search_symbol_filtered"))]
assert "Err(_) => return Ok(Vec::new())" in old
assert "let q = arg_str(&args, \"query\")?;" in mcp
assert ".search_symbol_paged_resumable(q, kind, mode" in mcp
assert "SymbolMatch::Contains" in api and ".search_symbol_paged_resumable(" in api
print("Contains + empty query: Err(Error::NotFound)")
print("Prefix + empty query: scan path; starts_with(\"\") is true")
print("Suffix + empty query: scan path; ends_with(\"\") is true")
print("Exact + empty query: scan path; only name == \"\" can match")
print("Legacy helper: engine errors map to Ok(Vec::new())")
print("MCP/API callers: no empty-query validation found in the forwarding paths")
PYRepository: hungpham10/codegraph-rs
Length of output: 5198
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '== arg_str definition and calls =='
rg -n -C8 'fn arg_str|arg_str\s*\(' crates/codegraph-mcp/src/tools.rs || true
printf '%s\n' '== exact generic call lines =='
nl -ba crates/codegraph-graph/src/lib.rs | sed -n '1728,1736p'
printf '%s\n' '== exact source assertions =='
python3 - <<'PY'
from pathlib import Path
import re
search = Path("crates/codegraph-graph/src/search.rs").read_text()
graph = Path("crates/codegraph-graph/src/lib.rs").read_text()
mcp = Path("crates/codegraph-mcp/src/tools.rs").read_text()
api = Path("crates/codegraph-api/src/lib.rs").read_text()
checks = [
("generic empty pattern returns NotFound",
"pattern.is_empty()" in search and "return Err(Error::NotFound);" in search[search.index("pub async fn search_resumable"):]),
("paged calls generic search with q bytes",
re.search(r"\.search_resumable\(\s*q\.as_bytes\(\)", graph) is not None),
("paged has no empty guard",
"if q.is_empty()" not in graph[graph.index("pub async fn search_symbol_paged_resumable"):graph.index("/// Resumable + deadline-aware", graph.index("pub async fn search_symbol_paged_resumable"))]),
("legacy helper maps engine errors to empty",
"Err(_) => return Ok(Vec::new())" in graph[graph.index("async fn search_symbol_filtered"):]),
("MCP extracts query directly",
'let q = arg_str(&args, "query")?;' in mcp),
("MCP forwards paged query",
".search_symbol_paged_resumable(q, kind, mode" in mcp),
("API defaults resumable search to Contains",
"SymbolMatch::Contains" in api[api.index("pub async fn search_resumable"):api.index("pub async fn search_symbol_paged"):]),
]
for name, result in checks:
print(f"{'PASS' if result else 'FAIL'}: {name}")
assert result, name
PYRepository: hungpham10/codegraph-rs
Length of output: 8647
Handle empty Contains queries before calling the name engine.
arg_str accepts "", and codegraph_search_symbol defaults to Contains. The empty byte pattern reaches Search::search_resumable at line 1734, which returns Error::NotFound; the legacy helper maps this error to an empty result. Return an empty PagedSearchOutcome for empty Contains queries after resume validation, or map this error to an empty result.
🤖 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 `@crates/codegraph-graph/src/lib.rs` around lines 1729 - 1758, Update the
search flow around codegraph_search_symbol and the SearchCursorPhase::Engine
branch to detect an empty query when the search mode is Contains, after
validating the resume state, and return an empty PagedSearchOutcome without
invoking search_resumable. Preserve normal engine behavior for non-empty queries
and other search modes; alternatively, map the resulting NotFound error to an
empty result consistently with the legacy helper.
| DfsState::Collect { root, mut stack } => { | ||
| // Collect subtree theo pre-order (record của node trước, sau | ||
| // đó mới children — giống bản đệ quy cũ). | ||
| if let Some((node_id, child_idx)) = stack.pop() { | ||
| let (_prefix_bytes, record) = | ||
| { self.storage.read().await.get_node(node_id).await? }; | ||
| if record != EMPTY { | ||
| records.push(record); | ||
| } | ||
| let children = { self.storage.read().await.get_children(node_id).await? }; | ||
| if child_idx < children.len() { | ||
| stack.push((node_id, child_idx + 1)); | ||
| stack.push((children[child_idx], 0)); | ||
| } | ||
| Some(DfsState::Collect { root, stack }) | ||
| } else { | ||
| None // Collect xong — candidate đã có records, dừng. | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Collect pushes a node record once per child visit.
Each stack entry (node_id, child_idx) is re-pushed as (node_id, child_idx + 1) before descending into a child. When that entry pops again, the code re-reads the node and pushes record a second time. A node with k children therefore contributes its record k + 1 times (k times when child_idx < children.len(), plus the final pop).
The recursive implementation pushed the record exactly once. Search::search_resumable hides the effect because it deduplicates through seen, but Radix::search_dfs returns duplicates directly, MAX_RESULTS accounting inflates, and each duplicate is carried inside the checkpoint.
Push the record only on the first visit to the node.
🐛 Proposed fix
DfsState::Collect { root, mut stack } => {
// Collect subtree theo pre-order (record của node trước, sau
// đó mới children — giống bản đệ quy cũ).
if let Some((node_id, child_idx)) = stack.pop() {
let (_prefix_bytes, record) =
{ self.storage.read().await.get_node(node_id).await? };
- if record != EMPTY {
+ // Chỉ lần thăm đầu tiên (child_idx == 0) mới ghi record —
+ // các lần pop sau chỉ để duyệt tiếp children.
+ if child_idx == 0 && record != EMPTY {
records.push(record);
}📝 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.
| DfsState::Collect { root, mut stack } => { | |
| // Collect subtree theo pre-order (record của node trước, sau | |
| // đó mới children — giống bản đệ quy cũ). | |
| if let Some((node_id, child_idx)) = stack.pop() { | |
| let (_prefix_bytes, record) = | |
| { self.storage.read().await.get_node(node_id).await? }; | |
| if record != EMPTY { | |
| records.push(record); | |
| } | |
| let children = { self.storage.read().await.get_children(node_id).await? }; | |
| if child_idx < children.len() { | |
| stack.push((node_id, child_idx + 1)); | |
| stack.push((children[child_idx], 0)); | |
| } | |
| Some(DfsState::Collect { root, stack }) | |
| } else { | |
| None // Collect xong — candidate đã có records, dừng. | |
| } | |
| } | |
| DfsState::Collect { root, mut stack } => { | |
| // Collect subtree theo pre-order (record của node trước, sau | |
| // đó mới children — giống bản đệ quy cũ). | |
| if let Some((node_id, child_idx)) = stack.pop() { | |
| let (_prefix_bytes, record) = | |
| { self.storage.read().await.get_node(node_id).await? }; | |
| // Chỉ lần thăm đầu tiên (child_idx == 0) mới ghi record — | |
| // các lần pop sau chỉ để duyệt tiếp children. | |
| if child_idx == 0 && record != EMPTY { | |
| records.push(record); | |
| } | |
| let children = { self.storage.read().await.get_children(node_id).await? }; | |
| if child_idx < children.len() { | |
| stack.push((node_id, child_idx + 1)); | |
| stack.push((children[child_idx], 0)); | |
| } | |
| Some(DfsState::Collect { root, stack }) | |
| } else { | |
| None // Collect xong — candidate đã có records, dừng. | |
| } | |
| } |
🤖 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 `@crates/codegraph-graph/src/radix.rs` around lines 837 - 855, Update the
DfsState::Collect handling in Radix::search_dfs so each node’s record is
appended only on its initial stack visit, while subsequent child-index resumes
process children without rereading or pushing the record. Preserve pre-order
traversal, MAX_RESULTS accounting, and checkpoint contents without duplicate
records.
| // ── Resolve: filter `depth` (key length trong storage) — deadline-aware. | ||
| // Bỏ qua nếu không giới hạn depth (name engine — chiếm đa số query). | ||
| if let Some(m) = max_len { | ||
| let mut out = Vec::new(); | ||
| let mut ridx = resolve_idx; | ||
| let storage = self.storage.read().await; | ||
| for &rid in &record_ids { | ||
| loop { | ||
| if let Some(dl) = deadline | ||
| && Instant::now() >= dl | ||
| { | ||
| return Ok(SearchPage { | ||
| record_ids: out.clone(), | ||
| resume: Some(SearchResume { | ||
| cand_idx: candidates.len(), | ||
| dfs: None, | ||
| record_ids: out.clone(), | ||
| resolve_idx: ridx, | ||
| }), | ||
| timed_out: true, | ||
| }); | ||
| } | ||
| if ridx >= record_ids.len() { | ||
| break; | ||
| } | ||
| let rid = record_ids[ridx]; | ||
| ridx += 1; | ||
| if rid == EMPTY { | ||
| continue; | ||
| } | ||
| if let Some(m) = max_len | ||
| && storage.get_key_len(rid).await?.unwrap_or(usize::MAX) > m | ||
| { | ||
| if storage.get_key_len(rid).await?.unwrap_or(usize::MAX) > m { | ||
| continue; | ||
| } | ||
| let meta = storage.get_meta(rid).await?; | ||
| results.push((rid, meta)); | ||
| if results.len() >= MAX_RESULTS { | ||
| break; | ||
| } | ||
| out.push(rid); | ||
| } | ||
| record_ids = out; | ||
| } | ||
|
|
||
| if results.is_empty() { | ||
| Err(Error::NotFound) | ||
| } else { | ||
| Ok(results) | ||
| } | ||
| Ok(SearchPage { | ||
| record_ids, | ||
| resume: None, | ||
| timed_out: false, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The depth-resolve resume state loses every record.
At the resolve timeout the code stores record_ids: out.clone() together with resolve_idx: ridx. out holds only the records that passed the filter, so its length is at most ridx, while ridx indexes the original unfiltered record_ids.
On resume, record_ids is restored from out and ridx from resolve_idx. The condition ridx >= record_ids.len() is then true on the first iteration, the loop breaks immediately, and record_ids = out assigns a fresh empty vector. The call returns zero results and reports completion.
Today no caller combines depth = Some(..) with a deadline, so the path is latent. The state machine is still incorrect and will silently drop results as soon as such a caller appears.
Keep the unfiltered list in the resume state and carry the filtered prefix separately.
🐛 Proposed fix
if let Some(m) = max_len {
- let mut out = Vec::new();
+ // `out` = phần đã filter; `record_ids` giữ nguyên danh sách gốc để
+ // `resolve_idx` luôn trỏ đúng vị trí khi resume.
+ let mut out: Vec<usize> = Vec::new();
let mut ridx = resolve_idx;
let storage = self.storage.read().await;
loop {
if let Some(dl) = deadline
&& Instant::now() >= dl
{
return Ok(SearchPage {
record_ids: out.clone(),
resume: Some(SearchResume {
cand_idx: candidates.len(),
dfs: None,
- record_ids: out.clone(),
+ record_ids: record_ids.clone(),
resolve_idx: ridx,
}),
timed_out: true,
});
}This alone still discards the already-filtered out across the resume boundary. Either add a dedicated resolved: Vec<usize> field to SearchResume, or drop the incremental out and re-filter from resolve_idx = 0 on resume.
🤖 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 `@crates/codegraph-graph/src/search.rs` around lines 572 - 613, Fix the
depth-filter resume flow in the search resolve block so resume state preserves
the original unfiltered record_ids while also retaining the already-filtered
output. Add a dedicated resolved-results field to SearchResume and restore/use
it across retries, or restart filtering from resolve_idx zero; ensure resumed
searches do not discard previously accepted records or report premature
completion.
| ``` | ||
| codegraph_search_symbol timed out after 2000ms (collected 134 symbols so far). | ||
| Retry the same call with the same arguments plus "resume": "<id>" to continue | ||
| the search from where it stopped. | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block.
markdownlint reports MD040 for this block. The block holds plain error text, so text is appropriate.
📝 Proposed fix
-```
+```text
codegraph_search_symbol timed out after 2000ms (collected 134 symbols so far).
Retry the same call with the same arguments plus "resume": "<id>" to continue
the search from where it stopped.📝 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.
| ``` | |
| codegraph_search_symbol timed out after 2000ms (collected 134 symbols so far). | |
| Retry the same call with the same arguments plus "resume": "<id>" to continue | |
| the search from where it stopped. | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@crates/codegraph-mcp/src/server-instructions.md` around lines 85 - 89, Update
the fenced code block in the instructions text to declare the text language,
using a text fence for the plain error message and preserving its contents
unchanged.
Source: Linters/SAST tools
| "codegraph_function_scope" => { | ||
| let target = resolve_target(api, &args, "id", "func_name", &[]).await?; | ||
| match target { | ||
| Target::Ambiguous(v) => Ok(json_str(v)), | ||
| Target::Ambiguous(v) => emit_value(root.as_str(), v), | ||
| Target::Symbol(sym) => match api.function_scope(sym.id).await { | ||
| Some(scope) => serde_json::to_string_pretty(&scope) | ||
| .map_err(|e| Error::Invalid(e.to_string())), | ||
| None => Ok(serde_json::to_string_pretty(&json!({ | ||
| "function": sym.name, | ||
| "parameters": [], | ||
| "locals": [], | ||
| "total": 0, | ||
| })) | ||
| .map_err(|e| Error::Invalid(e.to_string()))?), | ||
| Some(scope) => { | ||
| let detail = detail_from_args(&args, session_detail); | ||
| let format = format_from_args(&args, session_format); | ||
| let parameters: Vec<Value> = scope | ||
| .parameters | ||
| .iter() | ||
| .map(|s| symbol_json(root.as_str(), s, detail, format)) | ||
| .collect(); | ||
| let locals: Vec<Value> = scope | ||
| .locals | ||
| .iter() | ||
| .map(|s| symbol_json(root.as_str(), s, detail, format)) | ||
| .collect(); | ||
| emit_value( | ||
| root.as_str(), | ||
| json!({ | ||
| "function": symbol_json(root.as_str(), &scope.function, detail, format), | ||
| "parameters": parameters, | ||
| "locals": locals, | ||
| }), | ||
| ) | ||
| } | ||
| None => emit_value( | ||
| root.as_str(), | ||
| json!({ | ||
| "function": sym.name, | ||
| "parameters": [], | ||
| "locals": [], | ||
| "total": 0, | ||
| }), | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
codegraph_function_scope returns two different shapes for the function key.
The Some(scope) branch sets "function" to the output of symbol_json, which is an array in minimize style or an object in medium style. The None branch sets "function" to a plain name string. A consumer that parses the response must handle three shapes for one key.
Use a distinct key for the not-found case, or emit the resolved symbol through symbol_json in both branches.
🔧 Proposed fix
None => emit_value(
root.as_str(),
json!({
- "function": sym.name,
+ "function": symbol_json(
+ root.as_str(),
+ &sym,
+ detail_from_args(&args, session_detail),
+ format_from_args(&args, session_format),
+ ),
"parameters": [],
"locals": [],
"total": 0,
}),
),📝 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.
| "codegraph_function_scope" => { | |
| let target = resolve_target(api, &args, "id", "func_name", &[]).await?; | |
| match target { | |
| Target::Ambiguous(v) => Ok(json_str(v)), | |
| Target::Ambiguous(v) => emit_value(root.as_str(), v), | |
| Target::Symbol(sym) => match api.function_scope(sym.id).await { | |
| Some(scope) => serde_json::to_string_pretty(&scope) | |
| .map_err(|e| Error::Invalid(e.to_string())), | |
| None => Ok(serde_json::to_string_pretty(&json!({ | |
| "function": sym.name, | |
| "parameters": [], | |
| "locals": [], | |
| "total": 0, | |
| })) | |
| .map_err(|e| Error::Invalid(e.to_string()))?), | |
| Some(scope) => { | |
| let detail = detail_from_args(&args, session_detail); | |
| let format = format_from_args(&args, session_format); | |
| let parameters: Vec<Value> = scope | |
| .parameters | |
| .iter() | |
| .map(|s| symbol_json(root.as_str(), s, detail, format)) | |
| .collect(); | |
| let locals: Vec<Value> = scope | |
| .locals | |
| .iter() | |
| .map(|s| symbol_json(root.as_str(), s, detail, format)) | |
| .collect(); | |
| emit_value( | |
| root.as_str(), | |
| json!({ | |
| "function": symbol_json(root.as_str(), &scope.function, detail, format), | |
| "parameters": parameters, | |
| "locals": locals, | |
| }), | |
| ) | |
| } | |
| None => emit_value( | |
| root.as_str(), | |
| json!({ | |
| "function": sym.name, | |
| "parameters": [], | |
| "locals": [], | |
| "total": 0, | |
| }), | |
| ), | |
| "codegraph_function_scope" => { | |
| let target = resolve_target(api, &args, "id", "func_name", &[]).await?; | |
| match target { | |
| Target::Ambiguous(v) => emit_value(root.as_str(), v), | |
| Target::Symbol(sym) => match api.function_scope(sym.id).await { | |
| Some(scope) => { | |
| let detail = detail_from_args(&args, session_detail); | |
| let format = format_from_args(&args, session_format); | |
| let parameters: Vec<Value> = scope | |
| .parameters | |
| .iter() | |
| .map(|s| symbol_json(root.as_str(), s, detail, format)) | |
| .collect(); | |
| let locals: Vec<Value> = scope | |
| .locals | |
| .iter() | |
| .map(|s| symbol_json(root.as_str(), s, detail, format)) | |
| .collect(); | |
| emit_value( | |
| root.as_str(), | |
| json!({ | |
| "function": symbol_json(root.as_str(), &scope.function, detail, format), | |
| "parameters": parameters, | |
| "locals": locals, | |
| }), | |
| ) | |
| } | |
| None => emit_value( | |
| root.as_str(), | |
| json!({ | |
| "function": symbol_json( | |
| root.as_str(), | |
| &sym, | |
| detail_from_args(&args, session_detail), | |
| format_from_args(&args, session_format), | |
| ), | |
| "parameters": [], | |
| "locals": [], | |
| "total": 0, | |
| }), | |
| ), |
🤖 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 `@crates/codegraph-mcp/src/tools.rs` around lines 658 - 693, Normalize the
not-found response in the codegraph_function_scope handler so the "function" key
has the same shape in both branches. In the None branch, emit the resolved
symbol through symbol_json using the existing detail and format values, or use a
distinct key for the plain name instead; prefer preserving "function" with
symbol_json.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/codegraph-mcp/src/tools.rs (6)
734-744: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not expose the limited result count as
total.
GraphApi::references(call_name, limit)truncates the result vector before this handler computeshits.len(). The field therefore reports the returned count, not the total number of matching callers. Return a real total from the API, or rename the field tocount. (raw.githubusercontent.com)🤖 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 `@crates/codegraph-mcp/src/tools.rs` around lines 734 - 744, Update the references handler around GraphApi::references and the emitted JSON so total does not use the truncated hits.len() value. Prefer extending the API result to provide the true total number of matching callers and emit that value; otherwise rename the field to count to accurately represent the limited result count.
459-475: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake path-prefix filtering boundary-aware.
starts_with(prefix)matches sibling names. For example,path="src"also matchessrc_old/..., and/workspace/srcmatches/workspace/src2/.... Match the exact path or a path followed by/for both absolute and relative candidates.🤖 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 `@crates/codegraph-mcp/src/tools.rs` around lines 459 - 475, Update the filtering closure in the "codegraph_files" branch to use boundary-aware prefix matching for both f.path and strip_root_prefix(&f.path, root.as_str()). Accept only an exact path match or a candidate beginning with the prefix followed by "/", preventing sibling names such as src_old or src2 from matching.
551-552: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winApply the active serializer to ambiguous target responses.
Target::Ambiguouscontains rawSymbolvalues, and these branches pass them directly toemit_value. Aformat="minimize"request therefore returns object-shaped matches, while resolved responses return minimized arrays. Serializematcheswithsymbol_jsonbefore emitting the ambiguous response. Thecodegraph_symbolpath already follows this pattern. (raw.githubusercontent.com)Also applies to: 602-603, 667-668
🤖 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 `@crates/codegraph-mcp/src/tools.rs` around lines 551 - 552, Update the Target::Ambiguous response branches to serialize each raw Symbol in matches with symbol_json before passing the result to emit_value, including the corresponding branches near the other referenced locations. Match the existing codegraph_symbol serialization pattern so format="minimize" produces the same minimized array shape as resolved responses.
325-345: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReturn the continuation token after a non-timeout page.
GraphApi::search_resumablereturnsresumewhen a search times out or when more pages remain. This branch uses the token only in the timeout error and discards it on the normal path. Queries with more matches thanlimittherefore lose their continuation token, and the server-side cursor becomes unreachable. Return a stable envelope withresults,total, andresume, and update the tool description. (raw.githubusercontent.com)🤖 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 `@crates/codegraph-mcp/src/tools.rs` around lines 325 - 345, Update the normal completion path of the search handler around GraphApi::search_resumable to return a stable envelope containing the mapped results, total, and the optional resume token instead of emitting the bare result array. Preserve the timeout error’s continuation token, include the non-timeout out.resume value in the response, and revise the tool description to document the new response shape and continuation behavior.
949-958: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve semantic
falsefields during default omission.
is_default_valueremoves every false boolean. This removescompact=false, even though the class-method schema defaultscompacttotrue. It can also removepresent=false, which identifies an absent simulation result. Restrict omission to fields with an explicit false default, or preserve protocol and status fields explicitly. (raw.githubusercontent.com)🤖 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 `@crates/codegraph-mcp/src/tools.rs` around lines 949 - 958, Update is_default_value and its callers to preserve semantic false booleans such as compact=false and present=false; only omit false values for fields with an explicit false default. Use the existing field/key context to distinguish defaulted fields, while retaining omission of other default values and existing protocol/status semantics.
563-589: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftHonor the declared
formatparameter incodegraph_class_methods.The tool schema exposes
format, but this handler only readscompactand always emits object values.format="minimize"and the sessionOutputStyle::Minimizeare ignored. Add format-awareMemberInfoserialization, or removeformatfrom the schema and documentation. (raw.githubusercontent.com)🤖 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 `@crates/codegraph-mcp/src/tools.rs` around lines 563 - 589, Update the codegraph_class_methods handler to honor the declared format parameter, including session OutputStyle::Minimize, when serializing MemberInfo results. Add format-aware minimized serialization alongside the existing compact/full behavior, or remove format from the tool schema and documentation if it is not intended to be supported; ensure the emitted methods match the selected format.
🤖 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 `@crates/codegraph-graph/src/lib.rs`:
- Around line 1714-1716: Update the ScanNames cursor flow to persist the
accumulated matched-name vector alongside name_pos in
SearchCursorPhase::ScanNames, restore it when handling resume, and continue
scanning without discarding prior matches. Add partial-scan deadline tests
covering Prefix, Suffix, and Exact modes, verifying resumed pages and totals
include matches found before timeout.
---
Outside diff comments:
In `@crates/codegraph-mcp/src/tools.rs`:
- Around line 734-744: Update the references handler around GraphApi::references
and the emitted JSON so total does not use the truncated hits.len() value.
Prefer extending the API result to provide the true total number of matching
callers and emit that value; otherwise rename the field to count to accurately
represent the limited result count.
- Around line 459-475: Update the filtering closure in the "codegraph_files"
branch to use boundary-aware prefix matching for both f.path and
strip_root_prefix(&f.path, root.as_str()). Accept only an exact path match or a
candidate beginning with the prefix followed by "/", preventing sibling names
such as src_old or src2 from matching.
- Around line 551-552: Update the Target::Ambiguous response branches to
serialize each raw Symbol in matches with symbol_json before passing the result
to emit_value, including the corresponding branches near the other referenced
locations. Match the existing codegraph_symbol serialization pattern so
format="minimize" produces the same minimized array shape as resolved responses.
- Around line 325-345: Update the normal completion path of the search handler
around GraphApi::search_resumable to return a stable envelope containing the
mapped results, total, and the optional resume token instead of emitting the
bare result array. Preserve the timeout error’s continuation token, include the
non-timeout out.resume value in the response, and revise the tool description to
document the new response shape and continuation behavior.
- Around line 949-958: Update is_default_value and its callers to preserve
semantic false booleans such as compact=false and present=false; only omit false
values for fields with an explicit false default. Use the existing field/key
context to distinguish defaulted fields, while retaining omission of other
default values and existing protocol/status semantics.
- Around line 563-589: Update the codegraph_class_methods handler to honor the
declared format parameter, including session OutputStyle::Minimize, when
serializing MemberInfo results. Add format-aware minimized serialization
alongside the existing compact/full behavior, or remove format from the tool
schema and documentation if it is not intended to be supported; ensure the
emitted methods match the selected format.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a498c66a-fa34-42ac-b7c9-2ee2af7da7a0
📒 Files selected for processing (6)
crates/codegraph-api/src/lib.rscrates/codegraph-api/tests/api.rscrates/codegraph-graph/src/lib.rscrates/codegraph-graph/src/radix.rscrates/codegraph-mcp/src/lib.rscrates/codegraph-mcp/src/tools.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/codegraph-api/src/lib.rs
- crates/codegraph-graph/src/radix.rs
- crates/codegraph-mcp/src/lib.rs
- crates/codegraph-api/tests/api.rs
| pagination: Pagination, | ||
| resume: Option<SearchCursor>, | ||
| deadline: Option<Instant>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Persist matched names in the ScanNames cursor.
When a Prefix, Suffix, or Exact scan reaches its deadline after finding matches, the cursor retains only name_pos. The next call creates a new matched vector and expands only names after that position. This drops earlier matches and returns an incorrect page and total.
Store the accumulated matched names in SearchCursorPhase::ScanNames, restore them on resume, and add a partial-scan timeout test for each scan-based mode.
🤖 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 `@crates/codegraph-graph/src/lib.rs` around lines 1714 - 1716, Update the
ScanNames cursor flow to persist the accumulated matched-name vector alongside
name_pos in SearchCursorPhase::ScanNames, restore it when handling resume, and
continue scanning without discarding prior matches. Add partial-scan deadline
tests covering Prefix, Suffix, and Exact modes, verifying resumed pages and
totals include matches found before timeout.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation