Refactor logic to remove old data strucuture and compare performance - #2
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (89)
WalkthroughThe change adds benchmark automation, configurable extraction, Redis and Bloom-filter graph storage, sandbox execution, diff analysis, MCP project tools, and a sandbox CLI command. It removes visualization support and legacy core model APIs. ChangesCodegraph platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant McpServer
participant GraphIndex
participant Sandbox
participant RhaiMocks
MCPClient->>McpServer: call sandbox or simulation tool
McpServer->>GraphIndex: load project or snapshot index
GraphIndex->>Sandbox: compile function flow
Sandbox->>RhaiMocks: dispatch configured mock
Sandbox->>McpServer: return result and trace
Possibly related PRs
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: 17
🧹 Nitpick comments (6)
crates/codegraph-graph/tests/sqlite.rs (1)
188-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage. Consider adding a reopen assertion.
The test proves that two same-named symbols from different
ParseResultvalues get distinct global ids at ingest time. That is the right thing to pin.This test lives in the SQLite integration file and opens a real database, but it never reopens it. The reopen path is where
remap_scope_typematters, as documented atcrates/codegraph-graph/src/lib.rsLine 586. Dropping the index and callingGraphIndex::open(&db_str)again would also coverrebuildandindex_symbolfor the duplicate-name case.💚 Proposed addition
let mut files: Vec<&str> = hits.iter().map(|s| s.file.as_str()).collect(); files.sort_unstable(); assert_eq!(files, vec!["cache/cache.go", "store/store.go"]); + + // Reopen — rebuild giữ nguyên id global + file của từng symbol. + drop(idx); + let idx = GraphIndex::open(&db_str).await.unwrap(); + assert_eq!(idx.stats().symbols, 2); + assert_eq!(idx.symbol_by_id(SYMBOL_BASE).unwrap().file, "store/store.go"); + assert_eq!( + idx.symbol_by_id(SYMBOL_BASE + 1).unwrap().file, + "cache/cache.go" + ); }🤖 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/tests/sqlite.rs` around lines 188 - 239, Extend ingest_same_function_name_across_files_stays_distinct to drop the current GraphIndex, reopen the same SQLite database with GraphIndex::open, and repeat the symbol, flow, and search assertions after reopening. Ensure the duplicate-name symbols retain distinct global IDs, files, chains, and search results across the rebuild path.crates/codegraph-graph/src/lib.rs (1)
551-575: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftDocument or guard the partial-ingest window created by deferring
save_next_id.
registerno longer persistsnext_idper symbol;ingest_with_progresswrites it once at Line 557 together withset_version. This removes a per-symbol write and is a good change for throughput.It also widens a failure window.
ingest_with_progresscallsclear_entities()first, then writes symbols, chains, call records, and files. If any step between the reset and Line 557 returnsErr, the store holds partially written entities whilenext_idandversionstill hold the pre-ingest values. A reader that opens the database in that state gets a partial graph with an inconsistentnext_id.The next successful ingest clears and rewrites everything, so the state self-heals. Consider marking the store as "ingest in progress" at the start and clearing that flag at Line 559, so
rebuildcan report an incomplete index instead of serving one.🤖 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 551 - 575, Guard the deferred-persistence window in ingest_with_progress by marking ingestion as in progress immediately after clear_entities() and clearing that marker only after save_next_id and set_version succeed. Update rebuild to detect the marker and report an incomplete index instead of serving partially written graph data, using the existing storage metadata mechanisms where available.crates/codegraph-extract/tests/go_extract_test.rs (1)
256-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test_extract_multi_packagedoes not test what its name claims.The test parses two files separately and checks that each contains a
processfunction. The comment at Lines 257-260 states that identical ids are accepted. So the test asserts nothing about multi-package distinctness.The distinctness property is covered at ingest level by
ingest_same_function_name_across_files_stays_distinctincrates/codegraph-graph/tests/sqlite.rsLine 189. Here, either assert a property that belongs to the extraction phase, or rename the test to match what it checks.A concrete extraction-phase property: each parse result records the correct
fileon itsprocesssymbol.💚 Proposed assertion
- // Verify the symbols have different IDs (they should be distinct) - // For now, we'll accept that the IDs might be the same during extraction - // The GraphIndex will handle proper scoping during ingestion - // This is expected behavior for the extraction phase + // Extraction giữ id local per-file — mỗi symbol phải trỏ đúng file nguồn. + // Remap sang id global là việc của GraphIndex::ingest. + assert_eq!(store_process.file, store_path.to_str().unwrap()); + assert_eq!(cache_process.file, cache_path.to_str().unwrap()); }🤖 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-extract/tests/go_extract_test.rs` around lines 256 - 261, Update test_extract_multi_package to assert the extraction-phase property that each parsed process symbol records the correct source file, using the parse results for the two package files; remove the comments accepting identical IDs. Keep ingest-level ID distinctness covered by ingest_same_function_name_across_files_stays_distinct rather than asserting it here.crates/codegraph-extract/tests/diag_ingest.rs (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecide whether this diagnostic test belongs in the merged branch.
The doc comment marks the test as
TEMPORARY diagnostic. The body is mostlyprintln!output, andflowon line 57 is computed but never asserted; onlyis_ok()is printed. The single real assertion is oncallees2.Pick one of two paths:
- Remove the file before merge.
- Convert it to a regression test: assert the expected callee names, and assert
flow(main_id)succeeds with the expected chain.Do you want me to write the regression-test version against the committed Go fixture?
Also applies to: 57-58
🤖 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-extract/tests/diag_ingest.rs` around lines 1 - 6, Decide the fate of the temporary diagnostic test in diag_ingest.rs: remove it before merging, or convert it into a regression test. If retaining it, replace println!-based checks with assertions for the expected callee names and verify flow(main_id) succeeds with the expected chain, using the committed Go fixture.crates/codegraph-graph/src/storage/redis.rs (2)
862-913: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe Redis tests need a live server and leave keys behind.
new_test_storageconnects toredis://127.0.0.1:6379/15. Without a running server the tests panic, socargo testfails on any machine or CI job that has no Redis. Gate them behind#[ignore]or a feature, or skip when the connection fails.The tests also never delete the
test:radix:{pid}:{n}keys. Database 15 accumulates keys across runs. Add a teardown that deletes the prefix, or callFLUSHDBon the dedicated test database.🤖 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/storage/redis.rs` around lines 862 - 913, Update the Redis test setup around new_test_storage and its tests so they do not fail when no Redis server is available: skip the integration tests via an appropriate ignore/feature gate or return a skipped result when connection setup fails. Add teardown for each test storage instance that removes the test:radix:{pid}:{n} keys, or flushes the dedicated database 15, ensuring test data is not retained between runs.
370-397: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBatch the deletes, and escape glob metacharacters in the prefix.
Two points:
- The loop issues one
DELper key. Each key costs a round trip. Collect the keys from eachSCANpage and delete them in oneUNLINK(or one pipelined batch) per page.patterninterpolates the caller-supplied prefix directly into a glob.open_redisincrates/codegraph-graph/src/lib.rstakes the prefix from the DSNprefixquery parameter. A prefix that contains*,?, or[produces a pattern that matches keys of other prefixes in the same database, and this loop deletes them. Escape those characters when you build the pattern, or reject them inKeyBuilder::new.♻️ Proposed batching change
for key in keys { - cmd("DEL") - .arg(key) - .query_async::<()>(&mut *conn) - .await - .map_err(|e: redis::RedisError| StorageError::Internal(e.to_string()))?; - } + if !keys.is_empty() { + let mut del = cmd("UNLINK"); + for key in &keys { + del.arg(key); + } + del.query_async::<()>(&mut *conn) + .await + .map_err(|e: redis::RedisError| StorageError::Internal(e.to_string()))?; + }🤖 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/storage/redis.rs` around lines 370 - 397, Update clear_shortcuts to batch-delete each SCAN page with a single UNLINK command or pipelined batch instead of issuing one DEL per key, while preserving the existing error handling. Escape glob metacharacters (*, ?, and [) in the shortcut prefix before constructing the SCAN MATCH pattern, using the prefix/key-building path consistently so prefixes from open_redis cannot match or delete unrelated keys.
🤖 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-bench/src/main.rs`:
- Around line 98-119: Update the main benchmark flow around the rows loop,
render_table, and criterion_pass so cli.json writes only the render_json output
to stdout. Send per-repository progress and timing messages to stderr, skip
render_table when JSON mode is enabled, and disable or redirect Criterion output
whenever cli.json is true.
In `@crates/codegraph-extract/examples/dump_tree.rs`:
- Around line 41-43: Update the truncation logic in the text-formatting block to
limit by Unicode characters rather than slicing at byte offset 60. Preserve the
ellipsis for text exceeding the limit and ensure the resulting slice always ends
on a valid UTF-8 character boundary.
In `@crates/codegraph-extract/src/languages/common.rs`:
- Around line 445-452: Update the branch around class_id_of so it is selected
whenever class_id_of(root, src, class_index) identifies an indexed container,
rather than only when spec.class_kinds contains root.kind(). Preserve the
existing minimal chain insertion and object-field scope behavior, allowing enum
and module symbols to receive chains as well.
In `@crates/codegraph-extract/tests/chains.rs`:
- Around line 175-183: Update diag_go_terraform_root_main to stop reading the
machine-specific absolute path; replace it with an inline Go source string or a
repository fixture while preserving the existing parser and diagnostic
assertions.
In `@crates/codegraph-extract/tests/diag_ingest.rs`:
- Around line 11-15: Replace the machine-specific MAIN_GO path used by
diag_ingest_terraform_root_main with the committed basic_functions.go fixture
path, using the repository’s established fixture-resolution mechanism so the
test runs consistently on CI and other machines. Keep the test’s existing read
and assertions unchanged.
In `@crates/codegraph-extract/tests/go_extract_test.rs`:
- Around line 53-97: Remove the duplicated realMain call assertion blocks in the
test, including the repeated comment and the later count-based check. Keep one
assertion using result.calls and main_symbol.id that verifies a realMain call
exists and is made by main.
- Around line 145-172: In the test around result.calls, remove the debug println
output and the duplicated main_calls computation/assertion, retaining one
call-count check. Replace the non-specific “calls from main” validation with an
assertion on the call_name value actually produced by the parser, preserving the
expected qualified or unqualified name observed in the test fixture.
In `@crates/codegraph-graph/src/bloom.rs`:
- Around line 172-186: Harden BloomFilter deserialization before allocating or
indexing: in the header parsing flow, validate that bits_len is nonzero and
bounded, use checked multiplication for its byte length, and ensure the
serialized payload contains that many bytes. Also require m to equal bits_len *
64, m_mask to equal m - 1, and k to be nonzero and within the supported bound
before constructing Self, so contains cannot index outside bits.
In `@crates/codegraph-graph/src/lib.rs`:
- Around line 681-689: Update step 4 around alias_qualified_name and the
candidates lookup to use the bare call name as the name_index key, then resolve
the resulting class symbol with the same qualified dotted name through
lookup_method_of_class. In alias_qualified_name’s scope loop, replace the
symbols presence check with is_some_and so missing entries continue scanning
instead of terminating the search.
In `@crates/codegraph-graph/src/radix.rs`:
- Line 235: Update split so the newly created leg_id retains the Bloom filter
data for the existing subtree moved beneath it, copying or rebuilding that
subtree filter before later maintenance indexes prefix. Add a regression test
covering two pre-existing sibling suffixes, splitting above them, and a
subsequent insert through one sibling, then verify searches still find the older
descendants.
- Around line 747-748: Update the cfg attribute on the follow_path method so it
is compiled for both test builds and the bloom-search feature, using the
existing method name as the anchor. Preserve bloom-search availability while
ensuring test_follow_path and search_dfs_* can call it in the default test
build.
In `@crates/codegraph-graph/src/shared.rs`:
- Around line 120-126: Merge the identical persistent-index initialization
branches in rebuild_inner under a single cfg(any(feature = "sqlite", feature =
"redis")) block, retaining the in-memory fallback otherwise. Add a Redis-only
current_version probe using RedisStorage for the configured path/prefix, and
widen is_fresh to use the probing arm for either sqlite or redis while keeping
the always-fresh fallback for neither feature. Add Redis coverage equivalent to
sqlite_stale_version_rebuilds.
In `@crates/codegraph-graph/src/storage.rs`:
- Around line 345-347: Add a bloom-search-gated clear_blooms default method to
the Storage trait that removes all entries from MemoryData.blooms, implement or
expose it through InMemoryStorage, and invoke it alongside the radix-tree
clearing path so re-ingestion resets stale bloom filters.
In `@crates/codegraph-graph/src/storage/redis.rs`:
- Around line 755-760: Fix RedisTx::new_node ID allocation so IDs cannot become
stale while buffered writes await commit. Either atomically reserve the
transaction’s node ID range during commit before applying AddChild/UpdateNode
references, or hold the connection lock for the entire RedisTx lifetime; ensure
concurrent RedisStorage::new_node calls and other transactions cannot claim the
same IDs.
- Around line 143-160: Update RedisStore::init to make branch/record seeding
atomic instead of using separate EXISTS and pipeline calls. Use a unique guard
key with SETNX or a Lua script that conditionally creates both sentinel entries
in one Redis operation, preserving the current initialization behavior while
preventing concurrent callers from inserting duplicate sentinels.
In `@crates/codegraph-graph/src/storage/sqlite.rs`:
- Around line 138-141: Update the reset flow around clear_entities to also clear
rt_node_blooms, adding a clear_node_blooms operation alongside the existing
radix node metadata reset and invoking it in the same path. Ensure recycled node
IDs cannot retain stale bloom rows after the radix data is cleared.
In `@crates/codegraph-viz/assets/app.js`:
- Around line 515-519: Propagate the search filters end to end: extend
SearchParams to parse kind and match, update api::search to forward them, and
update GraphApi::search and its callers to apply the requested kind and
exact-match behavior. Ensure the HTTP search path honors both controls before
relying on the frontend parameters.
---
Nitpick comments:
In `@crates/codegraph-extract/tests/diag_ingest.rs`:
- Around line 1-6: Decide the fate of the temporary diagnostic test in
diag_ingest.rs: remove it before merging, or convert it into a regression test.
If retaining it, replace println!-based checks with assertions for the expected
callee names and verify flow(main_id) succeeds with the expected chain, using
the committed Go fixture.
In `@crates/codegraph-extract/tests/go_extract_test.rs`:
- Around line 256-261: Update test_extract_multi_package to assert the
extraction-phase property that each parsed process symbol records the correct
source file, using the parse results for the two package files; remove the
comments accepting identical IDs. Keep ingest-level ID distinctness covered by
ingest_same_function_name_across_files_stays_distinct rather than asserting it
here.
In `@crates/codegraph-graph/src/lib.rs`:
- Around line 551-575: Guard the deferred-persistence window in
ingest_with_progress by marking ingestion as in progress immediately after
clear_entities() and clearing that marker only after save_next_id and
set_version succeed. Update rebuild to detect the marker and report an
incomplete index instead of serving partially written graph data, using the
existing storage metadata mechanisms where available.
In `@crates/codegraph-graph/src/storage/redis.rs`:
- Around line 862-913: Update the Redis test setup around new_test_storage and
its tests so they do not fail when no Redis server is available: skip the
integration tests via an appropriate ignore/feature gate or return a skipped
result when connection setup fails. Add teardown for each test storage instance
that removes the test:radix:{pid}:{n} keys, or flushes the dedicated database
15, ensuring test data is not retained between runs.
- Around line 370-397: Update clear_shortcuts to batch-delete each SCAN page
with a single UNLINK command or pipelined batch instead of issuing one DEL per
key, while preserving the existing error handling. Escape glob metacharacters
(*, ?, and [) in the shortcut prefix before constructing the SCAN MATCH pattern,
using the prefix/key-building path consistently so prefixes from open_redis
cannot match or delete unrelated keys.
In `@crates/codegraph-graph/tests/sqlite.rs`:
- Around line 188-239: Extend
ingest_same_function_name_across_files_stays_distinct to drop the current
GraphIndex, reopen the same SQLite database with GraphIndex::open, and repeat
the symbol, flow, and search assertions after reopening. Ensure the
duplicate-name symbols retain distinct global IDs, files, chains, and search
results across the rebuild path.
🪄 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: 91f3ca75-625d-4c1c-90f3-81f16b115c93
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (68)
.github/workflows/codspeed.yml.gitignoreCargo.tomlbenches/fetch_repos.shbenches/repos/sources.txtcrates/codegraph-api/src/lib.rscrates/codegraph-api/tests/api.rscrates/codegraph-bench/Cargo.tomlcrates/codegraph-bench/benches/codspeed.rscrates/codegraph-bench/src/lib.rscrates/codegraph-bench/src/main.rscrates/codegraph-bench/tests/pipeline.rscrates/codegraph-core/src/drafts.rscrates/codegraph-core/src/kinds.rscrates/codegraph-core/src/lib.rscrates/codegraph-core/src/model.rscrates/codegraph-core/src/semgraph.rscrates/codegraph-extract/Cargo.tomlcrates/codegraph-extract/examples/dump_tree.rscrates/codegraph-extract/examples/smoke.rscrates/codegraph-extract/src/languages/common.rscrates/codegraph-extract/src/languages/cpp.rscrates/codegraph-extract/src/languages/csharp.rscrates/codegraph-extract/src/languages/effects.rscrates/codegraph-extract/src/languages/java.rscrates/codegraph-extract/src/languages/javascript.rscrates/codegraph-extract/src/languages/lua.rscrates/codegraph-extract/src/languages/php.rscrates/codegraph-extract/src/languages/ruby.rscrates/codegraph-extract/src/languages/rust.rscrates/codegraph-extract/src/languages/scala.rscrates/codegraph-extract/src/languages/swift.rscrates/codegraph-extract/src/languages/typescript.rscrates/codegraph-extract/src/lib.rscrates/codegraph-extract/src/orchestrator.rscrates/codegraph-extract/src/project.rscrates/codegraph-extract/src/walker.rscrates/codegraph-extract/tests/chains.rscrates/codegraph-extract/tests/cpp_functions.rscrates/codegraph-extract/tests/diag_ingest.rscrates/codegraph-extract/tests/extract.rscrates/codegraph-extract/tests/fixtures/basic_functions.gocrates/codegraph-extract/tests/fixtures/control_flow.gocrates/codegraph-extract/tests/fixtures/multi_package_cache.gocrates/codegraph-extract/tests/fixtures/multi_package_store.gocrates/codegraph-extract/tests/fixtures/struct_methods.gocrates/codegraph-extract/tests/go_extract_test.rscrates/codegraph-graph/Cargo.tomlcrates/codegraph-graph/benches/search_bloom.rscrates/codegraph-graph/src/bloom.rscrates/codegraph-graph/src/lib.rscrates/codegraph-graph/src/radix.rscrates/codegraph-graph/src/shared.rscrates/codegraph-graph/src/storage.rscrates/codegraph-graph/src/storage/redis.rscrates/codegraph-graph/src/storage/sqlite.rscrates/codegraph-graph/tests/sqlite.rscrates/codegraph-mcp/Cargo.tomlcrates/codegraph-mcp/src/lib.rscrates/codegraph-mcp/src/server-instructions.mdcrates/codegraph-mcp/src/tools.rscrates/codegraph-viz/assets/app.jscrates/codegraph-viz/assets/index.htmlcrates/codegraph-viz/assets/styles.csscrates/codegraph-viz/src/server.rscrates/codegraph-viz/tests/http.rscrates/codegraph/src/main.rscrates/codegraph/src/watcher.rs
💤 Files with no reviewable changes (3)
- crates/codegraph-core/src/model.rs
- crates/codegraph-core/src/drafts.rs
- crates/codegraph-core/src/kinds.rs
| if t.len() > 60 { | ||
| format!("{}…", &t[..60]) | ||
| } else { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Truncate at a UTF-8 character boundary.
&t[..60] panics when byte 60 is inside a multi-byte character. Source text can contain Unicode identifiers, strings, or comments.
Truncate by characters instead of byte offsets.
Proposed fix
- if t.len() > 60 {
- format!("{}…", &t[..60])
+ if t.chars().count() > 60 {
+ format!("{}…", t.chars().take(60).collect::<String>())
} else {
t
}📝 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 t.len() > 60 { | |
| format!("{}…", &t[..60]) | |
| } else { | |
| if t.chars().count() > 60 { | |
| format!("{}…", t.chars().take(60).collect::<String>()) | |
| } else { |
🤖 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-extract/examples/dump_tree.rs` around lines 41 - 43, Update
the truncation logic in the text-formatting block to limit by Unicode characters
rather than slicing at byte offset 60. Preserve the ellipsis for text exceeding
the limit and ensure the resulting slice always ends on a valid UTF-8 character
boundary.
| fn diag_go_terraform_root_main() { | ||
| use codegraph_core::marker_name; | ||
| let src = std::fs::read_to_string("/Users/lap02921/Desktop/Workspace/terraform/main.go") | ||
| .expect("read terraform main.go"); | ||
| let parser = registry() | ||
| .into_iter() | ||
| .find(|p| p.name() == "go") | ||
| .expect("go parser"); | ||
| let res = parser.parse_file("main.go", &src).expect("parse"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the machine-specific diagnostic test input.
diag_go_terraform_root_main reads /Users/lap02921/Desktop/Workspace/terraform/main.go. This file is not a repository fixture. CI and other developer environments will fail at expect("read terraform main.go").
Use a repository fixture or an inline Go source string.
Proposed fix
- let src = std::fs::read_to_string("/Users/lap02921/Desktop/Workspace/terraform/main.go")
- .expect("read terraform main.go");
+ let src = "package main\nfunc main() {}\n";
@@
- let res = parser.parse_file("main.go", &src).expect("parse");
+ let res = parser.parse_file("main.go", src).expect("parse");📝 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.
| fn diag_go_terraform_root_main() { | |
| use codegraph_core::marker_name; | |
| let src = std::fs::read_to_string("/Users/lap02921/Desktop/Workspace/terraform/main.go") | |
| .expect("read terraform main.go"); | |
| let parser = registry() | |
| .into_iter() | |
| .find(|p| p.name() == "go") | |
| .expect("go parser"); | |
| let res = parser.parse_file("main.go", &src).expect("parse"); | |
| fn diag_go_terraform_root_main() { | |
| use codegraph_core::marker_name; | |
| let src = "package main\nfunc main() {}\n"; | |
| let parser = registry() | |
| .into_iter() | |
| .find(|p| p.name() == "go") | |
| .expect("go parser"); | |
| let res = parser.parse_file("main.go", src).expect("parse"); |
🤖 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-extract/tests/chains.rs` around lines 175 - 183, Update
diag_go_terraform_root_main to stop reading the machine-specific absolute path;
replace it with an inline Go source string or a repository fixture while
preserving the existing parser and diagnostic assertions.
| /// node id → serialize bloom filter (prune nhánh trong search_dfs). | ||
| #[cfg(feature = "bloom-search")] | ||
| blooms: HashMap<usize, Vec<u8>>, |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
blooms has no clear path, so it grows across re-ingests.
Every other stream in MemoryData that the index rebuilds has a reset: clear_edges at Line 599, clear_node_meta at Line 645, clear_chains at Line 671, clear_shortcuts at Line 571. blooms has none, and clear_entities at Line 830 does not touch it.
InMemoryStorage::next_id is never reset, so a re-ingest allocates new node ids and the old bloom entries become unreachable. In a long-lived in-memory server that re-ingests on every file change, the map grows without bound.
Add a clear_blooms default method on Storage and call it where the radix tree is cleared.
♻️ Proposed addition
#[cfg(feature = "bloom-search")]
async fn get_node_bloom(&self, _id: usize) -> Result<Option<Vec<u8>>> {
Ok(None)
}
+ /// Xoá toàn bộ bloom filters (dùng khi rebuild index). Mặc định: no-op.
+ #[cfg(feature = "bloom-search")]
+ async fn clear_blooms(&mut self) -> Result<()> {
+ Ok(())
+ }And in impl Storage for InMemoryStorage:
#[cfg(feature = "bloom-search")]
async fn get_node_bloom(&self, id: usize) -> Result<Option<Vec<u8>>> {
let d = self
.data
.read()
.map_err(|_| StorageError::Internal("poison".into()))?;
Ok(d.blooms.get(&id).cloned())
}
+
+ #[cfg(feature = "bloom-search")]
+ async fn clear_blooms(&mut self) -> Result<()> {
+ let mut d = self
+ .data
+ .write()
+ .map_err(|_| StorageError::Internal("poison".into()))?;
+ d.blooms.clear();
+ Ok(())
+ }Also applies to: 471-489
🤖 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/storage.rs` around lines 345 - 347, Add a
bloom-search-gated clear_blooms default method to the Storage trait that removes
all entries from MemoryData.blooms, implement or expose it through
InMemoryStorage, and invoke it alongside the radix-tree clearing path so
re-ingestion resets stale bloom filters.
| async fn init(&self) -> Result<()> { | ||
| let mut conn = self.lock().await; | ||
| let exists: bool = cmd("EXISTS") | ||
| .arg(self.kb.key("branch")) | ||
| .query_async(&mut *conn) | ||
| .await | ||
| .map_err(|e: redis::RedisError| StorageError::Internal(e.to_string()))?; | ||
| if !exists { | ||
| redis::pipe() | ||
| .atomic() | ||
| .rpush(self.kb.key("branch"), b"" as &[u8]) | ||
| .rpush(self.kb.key("record"), 0i64) | ||
| .exec_async(&mut *conn) | ||
| .await | ||
| .map_err(|e: redis::RedisError| StorageError::Internal(e.to_string()))?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
init performs a check-then-act across two round trips.
EXISTS and the seeding pipeline are separate commands. Two processes that open the same prefix concurrently can both observe exists == false and both push a sentinel. The branch list then holds two entries at indices 0 and 1, and every node id computed as len - 1 is off by one against the record list.
Make the seed atomic. SETNX on a guard key, or a Lua script that checks and seeds in one call, removes the window.
🤖 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/storage/redis.rs` around lines 143 - 160, Update
RedisStore::init to make branch/record seeding atomic instead of using separate
EXISTS and pipeline calls. Use a unique guard key with SETNX or a Lua script
that conditionally creates both sentinel entries in one Redis operation,
preserving the current initialization behavior while preventing concurrent
callers from inserting duplicate sentinels.
| async fn new_node(&mut self, prefix: Vec<u8>, record: usize) -> Result<usize> { | ||
| let base = self.node_len_checked().await?; | ||
| let id = base + self.nodes.len(); | ||
| self.nodes.push((id, prefix, record)); | ||
| Ok(id) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
RedisTx id allocation can collide with concurrent writes.
new_node reads LLEN live, but the tx buffers the RPUSH calls until commit. The connection mutex is released between every call. If RedisStorage::new_node or another tx appends nodes while this tx is open, base becomes stale and the returned ids point at nodes that already exist. The commit pipeline then RPUSHes at different positions, and the AddChild/UpdateNode ops reference the wrong ids.
The doc comment on line 742 states a single-connection assumption, but sharing one MultiplexedConnection does not serialize a transaction against interleaved direct writes.
Reserve the id range atomically at commit time, or hold the connection lock for the whole tx lifetime.
🤖 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/storage/redis.rs` around lines 755 - 760, Fix
RedisTx::new_node ID allocation so IDs cannot become stale while buffered writes
await commit. Either atomically reserve the transaction’s node ID range during
commit before applying AddChild/UpdateNode references, or hold the connection
lock for the entire RedisTx lifetime; ensure concurrent RedisStorage::new_node
calls and other transactions cannot claim the same IDs.
| "CREATE TABLE IF NOT EXISTS rt_node_blooms ( | ||
| id INTEGER PRIMARY KEY, | ||
| bloom BLOB NOT NULL | ||
| )", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find bloom persistence call sites and any radix reset/rebuild paths.
set -euo pipefail
echo "== Storage trait bloom methods =="
rg -nP -C4 '\bfn (set|get|clear)_node_bloom' crates/codegraph-graph/src
echo "== Any clear/delete of the bloom table =="
rg -nP -C3 'rt_node_blooms|node_bloom' crates/codegraph-graph/src
echo "== Radix reset / rebuild paths =="
rg -nP -C6 '\bfn (rebuild|reset|clear_nodes|clear_entities)\s*\(' crates/codegraph-graph/srcRepository: hungpham10/codegraph-rs
Length of output: 18854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sqlite.rs outline (bloom/clear/rebuild relevant symbols) =="
ast-grep outline crates/codegraph-graph/src/storage/sqlite.rs --view expanded 2>/dev/null \
| rg -n "bloom|clear|rebuild|version|next" || true
echo
echo "== sqlite.rs clear_entities implementation =="
sed -n '600,625p' crates/codegraph-graph/src/storage/sqlite.rs | cat -n
echo
echo "== sqlite.rs bloom impl =="
sed -n '270,305p' crates/codegraph-graph/src/storage/sqlite.rs | cat -n
echo
echo "== sqlite.rs clear_entities call sites =="
rg -n '\.clear_entities\s*\(' . || true
echo
echo "== radix.rs bloom insertion/read context =="
sed -n '618,635p' crates/codegraph-graph/src/radix.rs | cat -n
sed -n '828,852p' crates/codegraph-graph/src/radix.rs | cat -n
echo
echo "== lib.rs rebuild implementation and next_id == "
sed -n '240,290p' crates/codegraph-graph/src/lib.rs | cat -n
echo
echo "== storage trait clear_entities docs + all storage implementations clear_entities bodies =="
sed -n '310,320p' crates/codegraph-graph/src/storage.rs | cat -n
sed -n '824,845p' crates/codegraph-graph/src/storage.rs | cat -n
sed -n '707,722p' crates/codegraph-graph/src/storage/redis.rs | cat -nRepository: hungpham10/codegraph-rs
Length of output: 9946
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lib.rs rebuild continuation and call site =="
sed -n '249,340p' crates/codegraph-graph/src/lib.rs | cat -n
echo
rg -n -C6 'open\(|rebuild\(\)' crates/codegraph-graph/src/lib.rs
echo
echo "== sqlite.rs clear_node_meta implementation =="
sed -n '360,390p' crates/codegraph-graph/src/storage/sqlite.rs | cat -n
echo
echo "== sqlite.rs table creation order around rt_node_blooms/rt_* =="
sed -n '110,155p' crates/codegraph-graph/src/storage/sqlite.rs | cat -n
echo
echo "== broad clear/delete references in sqlite backend only =="
rg -n -C3 'DELETE FROM|clear_node_meta|rt_node_blooms|next_id|next' crates/codegraph-graph/src/storage/sqlite.rsRepository: hungpham10/codegraph-rs
Length of output: 20684
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== radix.rs next_id assignment / new_node call sites =="
rg -n -C5 'next_id\s*=|new_node\(|insert_symbol|rebuild_edges' crates/codegraph-graph/src/radix.rs
echo
echo "== radix.rs open/rebuild? not just storage trait =="
rg -n -C5 'async fn open|async fn rebuild|fn rebuild|rebuild_chains|rebuild_chain_engine|rebuild_name_engine|insert_symbol\(' crates/codegraph-graph/src
echo
echo "== Storage trait new_node signature =="
sed -n '100,124p' crates/codegraph-graph/src/storage.rs | cat -n
echo
echo "== Redis new_node implementation if present =="
sed -n '210,225p' crates/codegraph-graph/src/storage/redis.rs | cat -nRepository: hungpham10/codegraph-rs
Length of output: 15377
Clear node blooms when radix data is reset.
rt_node_blooms is upsert-only, and clear_entities only deletes entity-store rows. A stale bloom for a recycled node id can cause search to prune the subtree incorrectly when a stale bloom does not cover current elements. Add a clear_node_blooms operation and call it in the same reset path that clears radix node metadata.
🤖 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/storage/sqlite.rs` around lines 138 - 141, Update
the reset flow around clear_entities to also clear rt_node_blooms, adding a
clear_node_blooms operation alongside the existing radix node metadata reset and
invoking it in the same path. Ensure recycled node IDs cannot retain stale bloom
rows after the radix data is cleared.
e6cdddf to
feff7dd
Compare
f0ddcf6 to
ede272a
Compare
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.github/benches/fetch_repos.sh (1)
25-29: 🩺 Stability & Availability | 🔵 TrivialBound the Git network operations.
git cloneandgit fetchcan block on a stalled connection. The invoking CodSpeed job has no job-level timeout, so a network failure can hold the runner until the platform limit. Add Git low-speed settings and bounded retries, or configure a job timeout that covers both commands. (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 @.github/benches/fetch_repos.sh around lines 25 - 29, Bound the network operations in the repository setup flow around the git clone command, and apply the same protection to the script’s git fetch command: configure low-speed limits and bounded retries, or add an equivalent CodSpeed job timeout covering both operations. Keep the existing clone and fetch behavior unchanged aside from preventing indefinitely stalled connections.
🤖 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 @.github/benches/fetch_repos.sh:
- Around line 23-33: Update the checkout handling around dest and the git
clone/fetch commands to validate the existing repository’s origin URL against
$url; remove and reclone "$dest" when they differ. Before fetching and checking
out "$commit", force-clean the repository, including ignored and untracked
files, then append "$dest" to "$LIST" only after the reconciled checkout is
ready.
- Around line 21-25: Validate the trimmed name in the repository-reading loop
before assigning dest, accepting only safe single-component basename values and
rejecting path separators, traversal components, and other disallowed
characters; continue past invalid entries so dest="$OUT/$name" can never escape
the output directory or reference an unexpected location.
In @.github/workflows/codspeed.yml:
- Line 29: Update the actions/checkout@v4 step in the workflow to set
persist-credentials to false, while preserving the existing checkout behavior
and leaving any authenticated Git access to explicitly scoped later steps.
- Around line 46-52: Update the CodSpeed mode comment above the “Run benchmarks”
step to remove the unsupported “benchmark” mode and accurately describe the
supported simulation/walltime behavior, while preserving the existing simulation
configuration and result-upload behavior.
In `@Cargo.toml`:
- Line 10: Align the workspace configuration with the MSRV requirements
introduced by crates/codegraph-bench: either keep the workspace Rust
1.80/toolchain support by setting the benchmark crate to edition 2021 and
selecting a Redis version compatible with Rust 1.80, or consistently raise the
declared MSRV and toolchain configuration to support edition 2024 and redis
1.5.0.
---
Nitpick comments:
In @.github/benches/fetch_repos.sh:
- Around line 25-29: Bound the network operations in the repository setup flow
around the git clone command, and apply the same protection to the script’s git
fetch command: configure low-speed limits and bounded retries, or add an
equivalent CodSpeed job timeout covering both operations. Keep the existing
clone and fetch behavior unchanged aside from preventing indefinitely stalled
connections.
🪄 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: 83728610-aaa6-47a3-b880-ca98910e169d
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcrates/codegraph-viz/assets/vendor/3d-force-graph.min.jsis excluded by!**/*.min.jscrates/codegraph-viz/assets/vendor/force-graph.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (73)
.github/benches/fetch_repos.sh.github/benches/repos/sources.txt.github/workflows/codspeed.yml.gitignoreCargo.tomlcrates/codegraph-api/src/lib.rscrates/codegraph-api/tests/api.rscrates/codegraph-bench/Cargo.tomlcrates/codegraph-bench/benches/codspeed.rscrates/codegraph-bench/src/lib.rscrates/codegraph-bench/src/main.rscrates/codegraph-bench/tests/pipeline.rscrates/codegraph-core/src/drafts.rscrates/codegraph-core/src/kinds.rscrates/codegraph-core/src/lib.rscrates/codegraph-core/src/model.rscrates/codegraph-core/src/semgraph.rscrates/codegraph-extract/Cargo.tomlcrates/codegraph-extract/examples/dump_tree.rscrates/codegraph-extract/examples/smoke.rscrates/codegraph-extract/src/languages/common.rscrates/codegraph-extract/src/languages/cpp.rscrates/codegraph-extract/src/languages/csharp.rscrates/codegraph-extract/src/languages/effects.rscrates/codegraph-extract/src/languages/java.rscrates/codegraph-extract/src/languages/javascript.rscrates/codegraph-extract/src/languages/lua.rscrates/codegraph-extract/src/languages/php.rscrates/codegraph-extract/src/languages/ruby.rscrates/codegraph-extract/src/languages/rust.rscrates/codegraph-extract/src/languages/scala.rscrates/codegraph-extract/src/languages/swift.rscrates/codegraph-extract/src/languages/typescript.rscrates/codegraph-extract/src/lib.rscrates/codegraph-extract/src/orchestrator.rscrates/codegraph-extract/src/project.rscrates/codegraph-extract/src/walker.rscrates/codegraph-extract/tests/chains.rscrates/codegraph-extract/tests/cpp_functions.rscrates/codegraph-extract/tests/diag_ingest.rscrates/codegraph-extract/tests/extract.rscrates/codegraph-extract/tests/fixtures/basic_functions.gocrates/codegraph-extract/tests/fixtures/control_flow.gocrates/codegraph-extract/tests/fixtures/multi_package_cache.gocrates/codegraph-extract/tests/fixtures/multi_package_store.gocrates/codegraph-extract/tests/fixtures/struct_methods.gocrates/codegraph-extract/tests/go_extract_test.rscrates/codegraph-graph/Cargo.tomlcrates/codegraph-graph/benches/search_bloom.rscrates/codegraph-graph/src/bloom.rscrates/codegraph-graph/src/lib.rscrates/codegraph-graph/src/radix.rscrates/codegraph-graph/src/shared.rscrates/codegraph-graph/src/storage.rscrates/codegraph-graph/src/storage/redis.rscrates/codegraph-graph/src/storage/sqlite.rscrates/codegraph-graph/tests/sqlite.rscrates/codegraph-mcp/Cargo.tomlcrates/codegraph-mcp/src/lib.rscrates/codegraph-mcp/src/server-instructions.mdcrates/codegraph-mcp/src/tools.rscrates/codegraph-viz/Cargo.tomlcrates/codegraph-viz/assets/app.jscrates/codegraph-viz/assets/index.htmlcrates/codegraph-viz/assets/styles.csscrates/codegraph-viz/src/api.rscrates/codegraph-viz/src/assets.rscrates/codegraph-viz/src/lib.rscrates/codegraph-viz/src/server.rscrates/codegraph-viz/tests/http.rscrates/codegraph/Cargo.tomlcrates/codegraph/src/main.rscrates/codegraph/src/watcher.rs
💤 Files with no reviewable changes (12)
- crates/codegraph-viz/src/assets.rs
- crates/codegraph-viz/assets/styles.css
- crates/codegraph-viz/tests/http.rs
- crates/codegraph-core/src/kinds.rs
- crates/codegraph-core/src/model.rs
- crates/codegraph-viz/assets/index.html
- crates/codegraph-core/src/drafts.rs
- crates/codegraph-viz/src/lib.rs
- crates/codegraph-viz/src/api.rs
- crates/codegraph-viz/src/server.rs
- crates/codegraph-viz/assets/app.js
- crates/codegraph-viz/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (54)
- crates/codegraph-extract/src/languages/scala.rs
- crates/codegraph-extract/tests/fixtures/multi_package_store.go
- crates/codegraph-extract/src/languages/cpp.rs
- crates/codegraph/src/watcher.rs
- crates/codegraph-extract/src/languages/javascript.rs
- crates/codegraph-core/src/semgraph.rs
- crates/codegraph-extract/src/languages/rust.rs
- crates/codegraph-mcp/src/server-instructions.md
- crates/codegraph-extract/src/languages/ruby.rs
- crates/codegraph-extract/tests/extract.rs
- crates/codegraph-extract/tests/fixtures/basic_functions.go
- crates/codegraph-graph/tests/sqlite.rs
- crates/codegraph-extract/src/languages/lua.rs
- crates/codegraph-extract/src/languages/csharp.rs
- crates/codegraph-extract/tests/fixtures/control_flow.go
- crates/codegraph-extract/tests/go_extract_test.rs
- crates/codegraph-extract/src/walker.rs
- crates/codegraph-bench/tests/pipeline.rs
- crates/codegraph-extract/tests/diag_ingest.rs
- crates/codegraph-extract/examples/smoke.rs
- crates/codegraph-extract/tests/fixtures/multi_package_cache.go
- crates/codegraph-extract/src/languages/swift.rs
- crates/codegraph-graph/src/shared.rs
- crates/codegraph-extract/src/languages/typescript.rs
- crates/codegraph-extract/src/languages/effects.rs
- crates/codegraph-bench/Cargo.toml
- crates/codegraph-graph/src/storage/sqlite.rs
- crates/codegraph-extract/src/orchestrator.rs
- crates/codegraph-api/src/lib.rs
- crates/codegraph-core/src/lib.rs
- crates/codegraph-extract/examples/dump_tree.rs
- crates/codegraph-extract/Cargo.toml
- crates/codegraph-extract/src/lib.rs
- crates/codegraph-graph/benches/search_bloom.rs
- crates/codegraph-extract/src/languages/java.rs
- crates/codegraph-mcp/src/lib.rs
- crates/codegraph-extract/tests/cpp_functions.rs
- crates/codegraph-api/tests/api.rs
- crates/codegraph-bench/benches/codspeed.rs
- crates/codegraph-graph/Cargo.toml
- crates/codegraph-extract/tests/fixtures/struct_methods.go
- crates/codegraph-mcp/Cargo.toml
- crates/codegraph-extract/src/languages/common.rs
- .gitignore
- crates/codegraph-graph/src/bloom.rs
- crates/codegraph-graph/src/storage/redis.rs
- crates/codegraph-bench/src/main.rs
- crates/codegraph-mcp/src/tools.rs
- crates/codegraph-extract/tests/chains.rs
- crates/codegraph-extract/src/languages/php.rs
- crates/codegraph-bench/src/lib.rs
- crates/codegraph-graph/src/radix.rs
- crates/codegraph-graph/src/lib.rs
- crates/codegraph-extract/src/project.rs
| while IFS='|' read -r name url commit || [ -n "$name" ]; do | ||
| name="$(printf '%s' "$name" | xargs)" # trim | ||
| [ -z "$name" ] && continue | ||
| [[ "$name" == \#* ]] && continue | ||
| dest="$OUT/$name" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject path-like repository names.
name is trimmed but not limited to one path component. A value such as ../../outside can escape repos/checkout, and list.txt can then point the benchmark at an unexpected location. Validate the name with a safe basename allowlist before constructing dest. (raw.githubusercontent.com)
Suggested validation
[[ "$name" == \#* ]] && continue
+ if [[ ! "$name" =~ ^[A-Za-z0-9][A-Za-z0-9._-]*$ ]]; then
+ printf 'invalid repository name: %s\n' "$name" >&2
+ exit 1
+ fi
dest="$OUT/$name"📝 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.
| while IFS='|' read -r name url commit || [ -n "$name" ]; do | |
| name="$(printf '%s' "$name" | xargs)" # trim | |
| [ -z "$name" ] && continue | |
| [[ "$name" == \#* ]] && continue | |
| dest="$OUT/$name" | |
| while IFS='|' read -r name url commit || [ -n "$name" ]; do | |
| name="$(printf '%s' "$name" | xargs)" # trim | |
| [ -z "$name" ] && continue | |
| [[ "$name" == \#* ]] && continue | |
| if [[ ! "$name" =~ ^[A-Za-z0-9][A-Za-z0-9._-]*$ ]]; then | |
| printf 'invalid repository name: %s\n' "$name" >&2 | |
| exit 1 | |
| fi | |
| dest="$OUT/$name" |
🤖 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 @.github/benches/fetch_repos.sh around lines 21 - 25, Validate the trimmed
name in the repository-reading loop before assigning dest, accepting only safe
single-component basename values and rejecting path separators, traversal
components, and other disallowed characters; continue past invalid entries so
dest="$OUT/$name" can never escape the output directory or reference an
unexpected location.
| [ -z "$name" ] && continue | ||
| [[ "$name" == \#* ]] && continue | ||
| dest="$OUT/$name" | ||
| if [ ! -d "$dest/.git" ]; then | ||
| echo ">> clone $name ..." | ||
| git clone --quiet --filter=blob:none --no-checkout "$url" "$dest" | ||
| fi | ||
| echo ">> checkout $name @ ${commit:0:12}" | ||
| git -C "$dest" fetch --quiet --depth 1 origin "$commit" | ||
| git -C "$dest" checkout --quiet "$commit" | ||
| echo "$dest" >> "$LIST" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile reused checkouts before fetching.
When "$dest/.git" exists, the script skips cloning. It does not verify that origin matches $url, and checkout leaves untracked or ignored files from earlier local runs. A changed source row can therefore benchmark the wrong repository or stale files. Compare the remote URL, remove and reclone on mismatch, then force-clean the checkout before recording it. (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 @.github/benches/fetch_repos.sh around lines 23 - 33, Update the checkout
handling around dest and the git clone/fetch commands to validate the existing
repository’s origin URL against $url; remove and reclone "$dest" when they
differ. Before fetching and checking out "$commit", force-clean the repository,
including ignored and untracked files, then append "$dest" to "$LIST" only after
the reconciled checkout is ready.
| name: Bench | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow file =="
if [ -f .github/workflows/codspeed.yml ]; then
cat -n .github/workflows/codspeed.yml
else
echo "missing .github/workflows/codspeed.yml"
fi
echo
echo "== workflow references to checkout/persist/npm install/code execution =="
rg -n "actions/checkout|git (clone|commit|push|push-default|config|submodule|fetch|pull|push)\s|--depth|persist-credentials|npm install|npm run|pnpm|yarn|uv|python|pytest|codspeed|pull_request|run:" .github/workflows/codspeed.yml || trueRepository: hungpham10/codegraph-rs
Length of output: 2791
🌐 Web query:
actions/checkout v4 README persist-credentials default true
💡 Result:
In actions/checkout v4, the default value for the persist-credentials input is true [1][2][3]. When set to true, the action configures the GitHub Actions authentication token (or SSH key) into the local git configuration [1][2]. This allows subsequent steps in your workflow to execute authenticated git commands without additional configuration [1][4]. The token is automatically removed during the post-job cleanup process [1][3]. If you wish to prevent this behavior—for example, to enhance security or avoid persisting credentials in the local git config—you can explicitly set persist-credentials to false in your workflow file [1][5]: - uses: actions/checkout@v4 with: persist-credentials: false While there has been community discussion regarding changing this default to false to reduce the risk of unintentional credential exposure, it remains true in the current v4 release [1][6][3].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/README.md
- 2: https://github.com/actions/checkout/blob/v4/action.yml
- 3: https://github.com/actions/checkout/tree/v4.3.1
- 4: https://github.com/actions/checkout
- 5: https://github.com/actions/checkout/?tab=readme-ov-file
- 6: Change the default value of persist-credentials to false actions/checkout#1687
Sensitive Data Exposure (CWE-922)
Reachability: External
Disable checkout credential persistence.
actions/checkout@v4 persists the workflow GITHUB_TOKEN in local Git config by default. Since the workflow runs on pull_request and later steps execute repository code under .github/Cargo config, set persist-credentials: false; pass a scoped token only to steps that need authenticated Git access.
Suggested change
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 29-29: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/codspeed.yml at line 29, Update the actions/checkout@v4
step in the workflow to set persist-credentials to false, while preserving the
existing checkout behavior and leaving any authenticated Git access to
explicitly scoped later steps.
Sources: MCP tools, Linters/SAST tools
| # `mode: benchmark` đẩy kết quả lên CodSpeed Cloud (auto-provision bằng OIDC) | ||
| # để theo dõi trend. Muốn chạy khô (không lưu baseline) thì đổi `simulation`. | ||
| - name: Run benchmarks | ||
| uses: CodSpeedHQ/action@v4 | ||
| with: | ||
| mode: simulation | ||
| run: cargo codspeed run |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the out-of-date CodSpeed mode comment.
CodSpeedHQ/action@v4 documents simulation and walltime as valid modes; it does not document benchmark. The workflow already uses simulation and uploads CodSpeed benchmark results. Update the comment to avoid future use of an unsupported mode.
Suggested change
- # `mode: benchmark` đẩy kết quả lên CodSpeed Cloud (auto-provision bằng OIDC)
- # để theo dõi trend. Muốn chạy khô (không lưu baseline) thì đổi `simulation`.
+ # `simulation` measures and uploads CodSpeed benchmark results.
+ # Use `walltime` only when wall-time measurements are required.📝 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.
| # `mode: benchmark` đẩy kết quả lên CodSpeed Cloud (auto-provision bằng OIDC) | |
| # để theo dõi trend. Muốn chạy khô (không lưu baseline) thì đổi `simulation`. | |
| - name: Run benchmarks | |
| uses: CodSpeedHQ/action@v4 | |
| with: | |
| mode: simulation | |
| run: cargo codspeed run | |
| # `simulation` measures and uploads CodSpeed benchmark results. | |
| # Use `walltime` only when wall-time measurements are required. | |
| - name: Run benchmarks | |
| uses: CodSpeedHQ/action@v4 | |
| with: | |
| mode: simulation | |
| run: cargo codspeed run |
🤖 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 @.github/workflows/codspeed.yml around lines 46 - 52, Update the CodSpeed
mode comment above the “Run benchmarks” step to remove the unsupported
“benchmark” mode and accurately describe the supported simulation/walltime
behavior, while preserving the existing simulation configuration and
result-upload behavior.
d635bd5 to
0231bda
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (8)
crates/codegraph/src/main.rs (1)
569-569: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
rootdirectly toSboxConfig::load.
SboxConfig::loadtakes&Utf8Path(crates/codegraph-sboxes/src/config.rslines 85-89).rootis already&Utf8Path, soto_path_buf()allocates a copy for no reason.♻️ Proposed fix
- let config = SboxConfig::load(&root.to_path_buf()).unwrap_or_default(); + let config = SboxConfig::load(root).unwrap_or_default();🤖 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/src/main.rs` at line 569, Update the SboxConfig::load call to pass the existing root reference directly, removing the unnecessary to_path_buf allocation while preserving the unwrap_or_default behavior.crates/codegraph-mcp/src/tools.rs (2)
662-671: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the
dispatch_sandboxdocumentation todispatch_sandbox.The doc comment at lines 662-667 describes the sandbox dispatcher behavior (entry resolution, group building,
missing_mocks). It is attached totype SandboxRunOptions. The second paragraph (lines 668-670) is the one that describesparse_run_options.dispatch_sandboxat line 731 has no doc comment.♻️ Proposed fix
-/// Chạy sandbox trên flow của entry function. -/// -/// `node` (symbol id) hoặc `name` (substring → function match đầu tiên) chọn -/// entry; group = entry + mọi callee trong flow resolve được. `mocks` là map -/// callee → Rhai source (body được wrap tự động thành `fn <name>(args)`), override -/// file mock cùng tên — mocks thiếu được ghi vào `missing_mocks`. /// Parse các run-options dùng chung giữa `codegraph_sandbox`, /// `codegraph_diff_simulate`, `codegraph_origin_simulate`: `args` (i64 array), /// `mocks` (callee → rhai source), `branch_policy`, `loop_cap`. type SandboxRunOptions = (Vec<i64>, Vec<(String, String)>, SboxConfig);Then add the moved paragraph above
pub async fn dispatch_sandboxat line 731.🤖 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 662 - 671, Move the sandbox dispatcher documentation from the SandboxRunOptions type to the dispatch_sandbox function, preserving the entry resolution, callee grouping, and missing_mocks details. Keep the parse-run-options paragraph attached to SandboxRunOptions, and add the dispatcher documentation immediately above pub async fn dispatch_sandbox.
751-762: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the group-building loop.
This loop builds the compile group from
flow.chain.run_simat lines 823-835 repeats it, andcmd_sandboxincrates/codegraph/src/main.rsat lines 556-567 repeats it a third time. The three copies must stay in sync with the sandbox grouping rule. Extract one helper, for examplepub async fn flow_group(idx: &GraphIndex, entry_id: u64) -> Vec<u64>, and call it from all three sites.🤖 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 751 - 762, Extract the compile-group construction into a shared async helper such as flow_group, preserving marker filtering, symbol validation, deduplication, entry inclusion, and sorting. Replace the duplicated loops in the current site, run_sim, and cmd_sandbox with calls to this helper so all grouping follows one rule.crates/codegraph-sboxes/src/abi.rs (1)
1-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the 64-bit pointer assumption.
argsandretare pointers, but every signature declares them astypes::I64. The module header states that every value is ani64. On a 64-bit host that matches the pointer width. On a 32-bit host the JIT would truncate the pointers and the run would corrupt memory. Add a compile-time assertion so a 32-bit build fails at compile time instead of at run time.🛡️ Proposed change
use cranelift_codegen::ir::{types, AbiParam, Signature}; use cranelift_codegen::isa::CallConv; +// Pointers travel through the ABI as `types::I64`, so the host pointer must be +// 64-bit wide. +const _: () = assert!(std::mem::size_of::<usize>() == 8); + fn i64() -> AbiParam { AbiParam::new(types::I64) }🤖 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-sboxes/src/abi.rs` around lines 1 - 24, Add a compile-time assertion near the ABI helpers in abi.rs, such as alongside i64() or call_conv(), requiring pointer width to equal 64 bits. Ensure 32-bit targets fail during compilation before any ABI signatures using i64 for args and ret can be built.crates/codegraph-extract/src/config.rs (2)
204-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
tempfile::tempdir()instead of a fixed temp path.The test writes to
std::env::temp_dir().join("codegraph-extract-cfg-test"), a fixed shared path. If the test panics, the file stays on disk and can influence a later run.tempfileis already a dev-dependency of this crate, andwalker.rstests usetempfile::tempdir(). ATempDiralso removes the manual cleanup at lines 246-247.♻️ Proposed change
- let dir = std::env::temp_dir().join("codegraph-extract-cfg-test"); - std::fs::create_dir_all(&dir).unwrap(); - let path = dir.join("config.toml"); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.toml"); let path = Utf8Path::from_path(path.as_path()).unwrap();- let _ = std::fs::remove_file(path.as_std_path()); - let _ = std::fs::remove_dir(&dir); }Also applies to: 246-248
🤖 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-extract/src/config.rs` around lines 204 - 207, Update the affected test setup to use tempfile::tempdir() and derive config.toml from the returned TempDir path instead of the fixed std::env::temp_dir location. Keep the TempDir alive for the test duration and remove the manual cleanup at the end of the test.
65-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog a warning when the TOML fails to parse.
build_classifierwarns per invalid rule, so one badeffectvalue does not hide the rest. The enclosingload_frombehaves differently. At line 60, a TOML parse error returnsSelf::default()with no diagnostic.EffectCallPatternis#[serde(untagged)], so a misspelled matcher key such ascall = { prefx = "db." }fails deserialization of the whole file. The project then loseseffect_rulesandheaderswith no output. Add a warning on that path so the failure is visible.♻️ Proposed change
pub fn load_from(path: &Utf8Path) -> Self { let Ok(text) = fs::read_to_string(path.as_std_path()) else { return Self::default(); }; - let Ok(file) = toml::from_str::<ConfigFile>(&text) else { - return Self::default(); + let file = match toml::from_str::<ConfigFile>(&text) { + Ok(f) => f, + Err(e) => { + tracing::warn!("{path}: invalid config, using defaults: {e}"); + return Self::default(); + } };🤖 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-extract/src/config.rs` around lines 65 - 87, Update the TOML deserialization error path in load_from to emit a tracing warning containing the parse error before returning Self::default(). Preserve the existing fallback behavior, and leave build_classifier’s per-rule warnings unchanged.crates/codegraph-sboxes/src/codegen.rs (2)
208-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport unhandled markers instead of ignoring them.
The
_ => {}arm silently drops any marker that this lowering does not handle.codegraph-corereserves marker ids 13..=99 for future markers. When a new marker is added, this lowering will ignore it and still produce a module. The resulting trace will look valid but will not match the flow. Log the unhandled id so the gap is visible.♻️ Proposed change
MARKER_REC_CALL => { /* recursion is mocked; nothing to do */ } - _ => {} + other => { + debug_assert!(false, "unhandled marker {other} in chain lowering"); + }🤖 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-sboxes/src/codegen.rs` around lines 208 - 210, Update the fallback marker arm in the marker-lowering match near MARKER_REC_CALL to report the unhandled marker id through the existing logging mechanism instead of silently ignoring it. Preserve the current handling for known markers while ensuring future marker ids reserved by codegraph-core are visible in diagnostics.
568-578: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCompute
build_itemsonce per function.
build_itemsruns for every group function in Pass 0 at line 571 and again in Pass 2 at line 608. Each run walks the whole chain and allocates aStringfor every mock name. Build the items once before Pass 0 and reuse them.♻️ Proposed change
+ let all_items: Vec<Vec<Item>> = group.iter().map(|f| build_items(f, &ids)).collect(); + let mut missing = Vec::new(); let mut seen_names = HashSet::new(); - for f in group { - for it in build_items(f, &ids) { + for items in &all_items { + for it in items { if let ItemTag::MockCall { name, .. } = &it.tag { if seen_names.insert(name.clone()) && !mocks.has(name) { missing.push(name.clone());- for f in group { - let items = build_items(f, &ids); + for (f, items) in group.iter().zip(all_items) {Also applies to: 606-608
🤖 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-sboxes/src/codegen.rs` around lines 568 - 578, Compute and cache the result of build_items once for each function before Pass 0, then reuse the cached items in both the missing-mock scan and the Pass 2 logic around the existing group-processing flow. Update the loops near the visible missing and Pass 2 paths to consume the cached results while preserving their current ordering and behavior.
🤖 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/diff.rs`:
- Around line 116-135: Update the hunk-line classification in the diff parser so
an empty raw line is treated as a context line, recording its new-line number
and advancing new_n just like the Some(b' ') case. Keep the existing handling
for added, removed, and other unknown lines unchanged, and prevent empty lines
from invoking end_hunk!.
- Around line 461-468: Update the suffix fallback in the candidate path-matching
function so matching keys are collected and selected in a deterministic order
instead of using HashMap iteration order. Preserve the existing exact candidate
matching and suffix criteria, and ensure the same matched_path is chosen
consistently when multiple files share the relative suffix.
In `@crates/codegraph-mcp/src/server-instructions.md`:
- Around line 121-127: Update the documented flows example in the instructions
so each element contains id, name, file, and line directly, rather than nesting
them inside a flow object. Preserve the existing affected_calls, marker_window,
and called_by fields alongside these flat flow fields.
In `@crates/codegraph-mcp/src/tools.rs`:
- Around line 889-909: Move the blocking snapshot operations out of the async
runtime thread: update the git archive and tar process calls to use
tokio::process::Command with awaited status or tokio::task::spawn_blocking, and
run the synchronous Orchestrator::index_all work through spawn_blocking as well.
Preserve the existing success and error-result handling in the snapshot helper.
- Line 981: Update the note in dispatch_diff_simulate at
crates/codegraph-mcp/src/tools.rs:981-981 to use format! so base_ref is
interpolated, and update the note in dispatch_origin_simulate at
crates/codegraph-mcp/src/tools.rs:1027-1027 similarly so git_ref resolves
instead of being shown as a literal placeholder.
- Around line 861-873: Update the temp directory setup in build_before_index to
use tempfile::Builder::new().prefix("codegraph-sim-").tempdir() instead of
constructing a guessable SystemTime/PID path under std::env::temp_dir(). Keep
the cleanup contract by converting the kept TempDir into the existing
Utf8PathBuf with Utf8PathBuf::from_path_buf(dir.keep()).map_err(...), and
continue deriving tree and tar from that returned base path.
- Around line 712-729: Update sequence_delta so sequence_added and
sequence_removed compare per-token occurrence counts rather than Vec::contains
set membership, preserving token order in the reported entries where practical.
Ensure repeated tokens produce additions or removals when their counts differ,
and update the corresponding examples in server-instructions.md only if the
output shape changes.
- Around line 875-888: Validate base_ref before constructing or invoking the git
Command in the archive flow, rejecting any value that starts with "-" and
returning an appropriate error without running git. Apply this validation to
both codegraph_diff_simulate and codegraph_origin_simulate paths, using the
shared archive helper or equivalent visible base_ref handling.
In `@crates/codegraph-sboxes/Cargo.toml`:
- Around line 13-20: Update the rhai dependency used by the crate to require a
patched release at version 1.25 or newer instead of the broad version 1
constraint. If rhai is shared across the workspace, define the patched version
in the workspace dependency table and reference that declaration here.
In `@crates/codegraph-sboxes/src/codegen.rs`:
- Line 604: Make the entry function explicit rather than relying on group order:
update compile_group to accept entry_id and resolve that id within func_ids,
then thread entry_id through the compile and compile_with_mocks wrappers in
lib.rs. Preserve the existing group compilation behavior while ensuring
SandboxModule::run executes the requested entry even when it is not the lowest
symbol id, and add coverage for that ordering.
- Around line 328-346: Prevent mismatched frame markers from being removed
before variant validation. In crates/codegraph-sboxes/src/codegen.rs:328-346,
update emit_branch_end to inspect self.frames.last() with an If match and return
early before popping; in crates/codegraph-sboxes/src/codegen.rs:366-375, apply
the same guard in emit_loop_back for a Loop frame. Preserve the existing
handling after each validated pop.
- Around line 36-44: Raise the workspace rust-version declaration from 1.80 to
at least 1.81 so the #[allow(..., reason = "...")] attribute on Item::pos and
any other uses compile on the declared MSRV.
In `@crates/codegraph-sboxes/src/config.rs`:
- Around line 91-95: Update SboxConfig::load_from to match the
fs::read_to_string error kind: return Self::default() only for
ErrorKind::NotFound, and propagate all other failures as SboxConfigError::Io.
In `@crates/codegraph-sboxes/src/rhai.rs`:
- Around line 70-77: Update crates/codegraph-sboxes/src/rhai.rs:70-77 in load()
to propagate file-read and Rhai compilation errors instead of ignoring them;
update crates/codegraph-sboxes/src/rhai.rs:90-113 in load_with_mocks() to
propagate RhaiMockLib::register failures, including invalid mock signatures;
update crates/codegraph-sboxes/src/runtime.rs:173-182 in
mock_dispatch_trampoline() to return a distinct runtime error when a configured
mock fails, rather than converting every failure to the normal 0 result.
- Around line 53-56: Configure finite operation, depth, and allocation limits on
the Rhai Engine used by RhaiMockLib::load, RhaiMockLib::empty, and
RhaiMockLib::register before compiling mock files or inline sources; do not use
set_max_operations(0), since it disables the limit. Reuse a shared
bounded-engine setup so every compilation path receives the same limits.
---
Nitpick comments:
In `@crates/codegraph-extract/src/config.rs`:
- Around line 204-207: Update the affected test setup to use tempfile::tempdir()
and derive config.toml from the returned TempDir path instead of the fixed
std::env::temp_dir location. Keep the TempDir alive for the test duration and
remove the manual cleanup at the end of the test.
- Around line 65-87: Update the TOML deserialization error path in load_from to
emit a tracing warning containing the parse error before returning
Self::default(). Preserve the existing fallback behavior, and leave
build_classifier’s per-rule warnings unchanged.
In `@crates/codegraph-mcp/src/tools.rs`:
- Around line 662-671: Move the sandbox dispatcher documentation from the
SandboxRunOptions type to the dispatch_sandbox function, preserving the entry
resolution, callee grouping, and missing_mocks details. Keep the
parse-run-options paragraph attached to SandboxRunOptions, and add the
dispatcher documentation immediately above pub async fn dispatch_sandbox.
- Around line 751-762: Extract the compile-group construction into a shared
async helper such as flow_group, preserving marker filtering, symbol validation,
deduplication, entry inclusion, and sorting. Replace the duplicated loops in the
current site, run_sim, and cmd_sandbox with calls to this helper so all grouping
follows one rule.
In `@crates/codegraph-sboxes/src/abi.rs`:
- Around line 1-24: Add a compile-time assertion near the ABI helpers in abi.rs,
such as alongside i64() or call_conv(), requiring pointer width to equal 64
bits. Ensure 32-bit targets fail during compilation before any ABI signatures
using i64 for args and ret can be built.
In `@crates/codegraph-sboxes/src/codegen.rs`:
- Around line 208-210: Update the fallback marker arm in the marker-lowering
match near MARKER_REC_CALL to report the unhandled marker id through the
existing logging mechanism instead of silently ignoring it. Preserve the current
handling for known markers while ensuring future marker ids reserved by
codegraph-core are visible in diagnostics.
- Around line 568-578: Compute and cache the result of build_items once for each
function before Pass 0, then reuse the cached items in both the missing-mock
scan and the Pass 2 logic around the existing group-processing flow. Update the
loops near the visible missing and Pass 2 paths to consume the cached results
while preserving their current ordering and behavior.
In `@crates/codegraph/src/main.rs`:
- Line 569: Update the SboxConfig::load call to pass the existing root reference
directly, removing the unnecessary to_path_buf allocation while preserving the
unwrap_or_default behavior.
🪄 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: 22edc991-1ecb-4746-9b46-edea07c5f43b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomlcrates/codegraph-bench/benches/codspeed.rscrates/codegraph-core/src/error.rscrates/codegraph-core/src/lib.rscrates/codegraph-core/src/semgraph.rscrates/codegraph-extract/src/config.rscrates/codegraph-extract/src/languages/common.rscrates/codegraph-extract/src/languages/effects.rscrates/codegraph-extract/src/orchestrator.rscrates/codegraph-extract/src/walker.rscrates/codegraph-extract/tests/effects_config.rscrates/codegraph-graph/src/diff.rscrates/codegraph-graph/src/lib.rscrates/codegraph-graph/src/radix.rscrates/codegraph-mcp/Cargo.tomlcrates/codegraph-mcp/src/lib.rscrates/codegraph-mcp/src/server-instructions.mdcrates/codegraph-mcp/src/tools.rscrates/codegraph-sboxes/Cargo.tomlcrates/codegraph-sboxes/src/abi.rscrates/codegraph-sboxes/src/codegen.rscrates/codegraph-sboxes/src/config.rscrates/codegraph-sboxes/src/group.rscrates/codegraph-sboxes/src/lib.rscrates/codegraph-sboxes/src/rhai.rscrates/codegraph-sboxes/src/runtime.rscrates/codegraph-sboxes/src/trace.rscrates/codegraph-sboxes/tests/control_flow.rscrates/codegraph-sboxes/tests/end_to_end.rscrates/codegraph-sboxes/tests/mocks/order.rhaicrates/codegraph/Cargo.tomlcrates/codegraph/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/codegraph-bench/benches/codspeed.rs
- crates/codegraph/Cargo.toml
- crates/codegraph-extract/src/languages/common.rs
- crates/codegraph-extract/src/orchestrator.rs
- crates/codegraph-graph/src/radix.rs
- Cargo.toml
- crates/codegraph-graph/src/lib.rs
- crates/codegraph-mcp/Cargo.toml
- crates/codegraph-core/src/lib.rs
| } else if raw.starts_with('\\') { | ||
| // `\ No newline at end of file` — không phải dòng nội dung. | ||
| } else if let Some(h) = hunk.as_mut() { | ||
| match raw.as_bytes().first().copied() { | ||
| Some(b' ') => { | ||
| h.new_lines.push(new_n); | ||
| new_n += 1; | ||
| } | ||
| Some(b'+') => { | ||
| h.new_lines.push(new_n); | ||
| new_n += 1; | ||
| h.added += 1; | ||
| } | ||
| Some(b'-') => { | ||
| h.removed += 1; | ||
| } | ||
| // Dòng lạ trong lúc đang mở hunk — coi như hunk kết thúc. | ||
| _ => end_hunk!(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Treat an empty line inside a hunk as a context line.
At Line 119, raw.as_bytes().first() returns None for an empty line. The _ arm then closes the hunk. Many patch pipelines and mail transports strip the trailing space from an empty context line, so "" appears instead of " ". In that case the parser drops every remaining line of the hunk, and new_lines under-reports the changed region. The result is a silently incomplete impact report.
🐛 Proposed fix to accept empty context lines
match raw.as_bytes().first().copied() {
- Some(b' ') => {
+ Some(b' ') | None => {
h.new_lines.push(new_n);
new_n += 1;
}📝 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.
| } else if raw.starts_with('\\') { | |
| // `\ No newline at end of file` — không phải dòng nội dung. | |
| } else if let Some(h) = hunk.as_mut() { | |
| match raw.as_bytes().first().copied() { | |
| Some(b' ') => { | |
| h.new_lines.push(new_n); | |
| new_n += 1; | |
| } | |
| Some(b'+') => { | |
| h.new_lines.push(new_n); | |
| new_n += 1; | |
| h.added += 1; | |
| } | |
| Some(b'-') => { | |
| h.removed += 1; | |
| } | |
| // Dòng lạ trong lúc đang mở hunk — coi như hunk kết thúc. | |
| _ => end_hunk!(), | |
| } | |
| } | |
| } else if raw.starts_with('\\') { | |
| // `\ No newline at end of file` — không phải dòng nội dung. | |
| } else if let Some(h) = hunk.as_mut() { | |
| match raw.as_bytes().first().copied() { | |
| Some(b' ') | None => { | |
| h.new_lines.push(new_n); | |
| new_n += 1; | |
| } | |
| Some(b'+') => { | |
| h.new_lines.push(new_n); | |
| new_n += 1; | |
| h.added += 1; | |
| } | |
| Some(b'-') => { | |
| h.removed += 1; | |
| } | |
| // Dòng lạ trong lúc đang mở hunk — coi như hunk kết thúc. | |
| _ => end_hunk!(), | |
| } | |
| } |
🤖 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/diff.rs` around lines 116 - 135, Update the
hunk-line classification in the diff parser so an empty raw line is treated as a
context line, recording its new-line number and advancing new_n just like the
Some(b' ') case. Keep the existing handling for added, removed, and other
unknown lines unchanged, and prevent empty lines from invoking end_hunk!.
| for c in &candidates { | ||
| if let Some(k) = by_file.get_key_value(c.as_str()) { | ||
| return Some(k.0); | ||
| } | ||
| } | ||
| let suffix = format!("/{rel}"); | ||
| by_file.keys().find(|k| k.ends_with(&suffix)).copied() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the suffix fallback deterministic.
by_file.keys().find(...) iterates a HashMap, so the iteration order is not stable. If several indexed files end with the same relative path (for example crates/a/src/lib.rs and crates/b/src/lib.rs for the diff path src/lib.rs), the selected matched_path can change between runs. The report then attributes symbols and flows to a different file each time.
♻️ Proposed fix to select a stable match
let suffix = format!("/{rel}");
- by_file.keys().find(|k| k.ends_with(&suffix)).copied()
+ let mut hits: Vec<&'a str> = by_file
+ .keys()
+ .filter(|k| k.ends_with(&suffix))
+ .copied()
+ .collect();
+ // Deterministic pick: shortest path, then lexicographic.
+ hits.sort_unstable_by(|a, b| a.len().cmp(&b.len()).then(a.cmp(b)));
+ hits.first().copied()📝 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.
| for c in &candidates { | |
| if let Some(k) = by_file.get_key_value(c.as_str()) { | |
| return Some(k.0); | |
| } | |
| } | |
| let suffix = format!("/{rel}"); | |
| by_file.keys().find(|k| k.ends_with(&suffix)).copied() | |
| } | |
| for c in &candidates { | |
| if let Some(k) = by_file.get_key_value(c.as_str()) { | |
| return Some(k.0); | |
| } | |
| } | |
| let suffix = format!("/{rel}"); | |
| let mut hits: Vec<&'a str> = by_file | |
| .keys() | |
| .filter(|k| k.ends_with(&suffix)) | |
| .copied() | |
| .collect(); | |
| // Deterministic pick: shortest path, then lexicographic. | |
| hits.sort_unstable_by(|a, b| a.len().cmp(&b.len()).then(a.cmp(b))); | |
| hits.first().copied() | |
| } |
🤖 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/diff.rs` around lines 461 - 468, Update the suffix
fallback in the candidate path-matching function so matching keys are collected
and selected in a deterministic order instead of using HashMap iteration order.
Preserve the existing exact candidate matching and suffix criteria, and ensure
the same matched_path is chosen consistently when multiple files share the
relative suffix.
| fn sequence_delta(before: &Value, after: &Value) -> Value { | ||
| let seq = |v: &Value| -> Vec<String> { | ||
| v.get("sequence") | ||
| .and_then(|x| x.as_array()) | ||
| .map(|a| { | ||
| a.iter() | ||
| .filter_map(|x| x.as_str().map(str::to_string)) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default() | ||
| }; | ||
| let sb = seq(before); | ||
| let sa = seq(after); | ||
| json!({ | ||
| "sequence_added": sa.iter().filter(|s| !sb.contains(s)).cloned().collect::<Vec<_>>(), | ||
| "sequence_removed": sb.iter().filter(|s| !sa.contains(s)).cloned().collect::<Vec<_>>(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
sequence_delta misses repeat-count and order changes.
The closure compares with Vec::contains, so the delta is a set difference. Two traces with the same distinct tokens produce an empty delta even when the counts differ. A change in loop iterations produces exactly that case: ["call:x", "call:x"] versus ["call:x", "call:x", "call:x"] yields sequence_added: [] and sequence_removed: []. crates/codegraph-mcp/src/server-instructions.md (lines 169-175) presents loop iterations as part of the captured structure, and presents delta.sequence_added/removed as the reliable signal. Compare counts so an iteration-count change is visible.
🐛 Proposed fix using per-token counts
let sb = seq(before);
let sa = seq(after);
+ let count = |v: &[String]| {
+ let mut m: std::collections::BTreeMap<&str, usize> = std::collections::BTreeMap::new();
+ for s in v {
+ *m.entry(s.as_str()).or_default() += 1;
+ }
+ m
+ };
+ let (cb, ca) = (count(&sb), count(&sa));
+ let diff = |x: &std::collections::BTreeMap<&str, usize>,
+ y: &std::collections::BTreeMap<&str, usize>| {
+ x.iter()
+ .filter_map(|(k, n)| {
+ let m = y.get(k).copied().unwrap_or(0);
+ (*n > m).then(|| json!({ "token": k, "count": n - m }))
+ })
+ .collect::<Vec<_>>()
+ };
json!({
- "sequence_added": sa.iter().filter(|s| !sb.contains(s)).cloned().collect::<Vec<_>>(),
- "sequence_removed": sb.iter().filter(|s| !sa.contains(s)).cloned().collect::<Vec<_>>(),
+ "sequence_added": diff(&ca, &cb),
+ "sequence_removed": diff(&cb, &ca),
+ "order_changed": sa != sb,
})If you change the delta shape, update the documented examples in crates/codegraph-mcp/src/server-instructions.md (lines 158-167 and 197-205).
📝 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.
| fn sequence_delta(before: &Value, after: &Value) -> Value { | |
| let seq = |v: &Value| -> Vec<String> { | |
| v.get("sequence") | |
| .and_then(|x| x.as_array()) | |
| .map(|a| { | |
| a.iter() | |
| .filter_map(|x| x.as_str().map(str::to_string)) | |
| .collect() | |
| }) | |
| .unwrap_or_default() | |
| }; | |
| let sb = seq(before); | |
| let sa = seq(after); | |
| json!({ | |
| "sequence_added": sa.iter().filter(|s| !sb.contains(s)).cloned().collect::<Vec<_>>(), | |
| "sequence_removed": sb.iter().filter(|s| !sa.contains(s)).cloned().collect::<Vec<_>>(), | |
| }) | |
| } | |
| fn sequence_delta(before: &Value, after: &Value) -> Value { | |
| let seq = |v: &Value| -> Vec<String> { | |
| v.get("sequence") | |
| .and_then(|x| x.as_array()) | |
| .map(|a| { | |
| a.iter() | |
| .filter_map(|x| x.as_str().map(str::to_string)) | |
| .collect() | |
| }) | |
| .unwrap_or_default() | |
| }; | |
| let sb = seq(before); | |
| let sa = seq(after); | |
| let count = |v: &[String]| { | |
| let mut m: std::collections::BTreeMap<&str, usize> = std::collections::BTreeMap::new(); | |
| for s in v { | |
| *m.entry(s.as_str()).or_default() += 1; | |
| } | |
| m | |
| }; | |
| let (cb, ca) = (count(&sb), count(&sa)); | |
| let diff = |x: &std::collections::BTreeMap<&str, usize>, | |
| y: &std::collections::BTreeMap<&str, usize>| { | |
| x.iter() | |
| .filter_map(|(k, n)| { | |
| let m = y.get(k).copied().unwrap_or(0); | |
| (*n > m).then(|| json!({ "token": k, "count": n - m })) | |
| }) | |
| .collect::<Vec<_>>() | |
| }; | |
| json!({ | |
| "sequence_added": diff(&ca, &cb), | |
| "sequence_removed": diff(&cb, &ca), | |
| "order_changed": sa != sb, | |
| }) | |
| } |
🤖 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 712 - 729, Update
sequence_delta so sequence_added and sequence_removed compare per-token
occurrence counts rather than Vec::contains set membership, preserving token
order in the reported entries where practical. Ensure repeated tokens produce
additions or removals when their counts differ, and update the corresponding
examples in server-instructions.md only if the output shape changes.
| let millis = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .map(|d| d.as_millis()) | ||
| .unwrap_or(0); | ||
| let tmp = Utf8PathBuf::from_path_buf( | ||
| std::env::temp_dir().join(format!("codegraph-sim-{}-{millis}", std::process::id())), | ||
| ) | ||
| .map_err(|p| Error::Invalid(format!("temp path not UTF-8: {p:?}")))?; | ||
| let tree = tmp.join("tree"); | ||
| let tar = tmp.join("tree.tar"); | ||
| if let Err(e) = std::fs::create_dir_all(&tree) { | ||
| return Ok((None, tmp, format!("temp dir failed: {e}"))); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether tempfile is already available to codegraph-mcp, and confirm the keep/into_path API name for the pinned version.
fd -t f 'Cargo.toml' crates . --max-depth 3 | xargs -r rg -n 'tempfile'
rg -n 'tempfile' Cargo.lock | head -20Repository: hungpham10/codegraph-rs
Length of output: 943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== crates/codegraph-mcp/Cargo.toml deps ==="
sed -n '1,80p' crates/codegraph-mcp/Cargo.toml
echo
echo "=== locate tools.rs sections ==="
wc -l crates/codegraph-mcp/src/tools.rs
sed -n '830,890p' crates/codegraph-mcp/src/tools.rs
sed -n '950,1005p' crates/codegraph-mcp/src/tools.rs
sed -n '1015,1045p' crates/codegraph-mcp/src/tools.rs
echo
echo "=== imports / temp_dir / tar call sites in tools.rs ==="
sed -n '1,120p' crates/codegraph-mcp/src/tools.rs
rg -n 'temp_dir|create_dir_all|tar -xf|remove_dir_all|codegraph-sim|tempfile|TempDir|Builder' crates/codegraph-mcp/src/tools.rs
echo
echo "=== Cargo.lock tempfile entry ==="
rg -A 20 'name = "tempfile"$' Cargo.lock | head -80Repository: hungpham10/codegraph-rs
Length of output: 12937
Create the simulation temp directory with TempDir.
The current temp path contains a PID and millisecond guessable component in the system temp directory. If another process pre-creates that codegraph-sim-<pid>-<millis>/tree entry with a different type, create_dir_all can succeed, and later use of that path can write through or delete the wrong filesystem entry. Use tempfile::Builder::new().prefix("codegraph-sim-").tempdir() directly in build_before_index; keep the cleanup contract by returning Utf8PathBuf::from_path_buf(dir.keep()).map_err(...) from this function.
🤖 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 861 - 873, Update the temp
directory setup in build_before_index to use
tempfile::Builder::new().prefix("codegraph-sim-").tempdir() instead of
constructing a guessable SystemTime/PID path under std::env::temp_dir(). Keep
the cleanup contract by converting the kept TempDir into the existing
Utf8PathBuf with Utf8PathBuf::from_path_buf(dir.keep()).map_err(...), and
continue deriving tree and tar from that returned base path.
| fn emit_branch_end(&mut self) { | ||
| if let Some(Frame::If { | ||
| else_b, | ||
| merge_b, | ||
| seen_else, | ||
| }) = self.frames.pop() | ||
| { | ||
| if !self.terminated { | ||
| self.jump(merge_b); | ||
| } | ||
| if seen_else { | ||
| self.begin_block(merge_b); | ||
| } else { | ||
| self.begin_block(else_b); | ||
| self.jump(merge_b); | ||
| self.begin_block(merge_b); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Frame removal precedes the variant check in two emitters. Both emitters call self.frames.pop() in the if let scrutinee, so the frame leaves the stack even when the pattern does not match. A mismatched marker stream therefore discards a frame of the wrong kind and desynchronizes break_targets and continue_targets. The following LOOP_BACK, BRANCH_END, or SWITCH_END then targets the wrong blocks, and Cranelift reports an opaque verifier error from define_function. Inspect the top frame with self.frames.last() and return early when the variant does not match.
crates/codegraph-sboxes/src/codegen.rs#L328-L346: guardemit_branch_endwithmatches!(self.frames.last(), Some(Frame::If { .. }))before popping.crates/codegraph-sboxes/src/codegen.rs#L366-L375: guardemit_loop_backwithmatches!(self.frames.last(), Some(Frame::Loop { .. }))before popping.
📍 Affects 1 file
crates/codegraph-sboxes/src/codegen.rs#L328-L346(this comment)crates/codegraph-sboxes/src/codegen.rs#L366-L375
🤖 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-sboxes/src/codegen.rs` around lines 328 - 346, Prevent
mismatched frame markers from being removed before variant validation. In
crates/codegraph-sboxes/src/codegen.rs:328-346, update emit_branch_end to
inspect self.frames.last() with an If match and return early before popping; in
crates/codegraph-sboxes/src/codegen.rs:366-375, apply the same guard in
emit_loop_back for a Loop frame. Preserve the existing handling after each
validated pop.
| impl RhaiMockLib { | ||
| /// Load all `*.rhai` files under `dirs` (relative to `root`). | ||
| pub fn load(root: &Path, dirs: &[String]) -> Self { | ||
| let engine = Engine::new(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify the pinned Rhai version and existing resource-limit configuration.
rg -n -C 3 'rhai\s*=|name = "rhai"|set_max_operations|max_operations|set_max_call_levels' \
crates/codegraph-sboxes/Cargo.toml Cargo.lock crates/codegraph-sboxes/srcRepository: hungpham10/codegraph-rs
Length of output: 946
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant Rhai source and dependency metadata.
printf '--- crates/codegraph-sboxes/Cargo.toml ---\n'
sed -n '1,35p' crates/codegraph-sboxes/Cargo.toml
printf '\n--- Cargo.lock rhai package ---\n'
sed -n '2065,2085p' Cargo.lock
printf '\n--- crates/codegraph-sboxes/src/rhai.rs outline ---\n'
ast-grep outline crates/codegraph-sboxes/src/rhai.rs --view expanded || true
printf '\n--- Relevant Rhai source lines ---\n'
sed -n '1,180p' crates/codegraph-sboxes/src/rhai.rsRepository: hungpham10/codegraph-rs
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant Rhai source and dependency metadata.
echo '--- crates/codegraph-sboxes/Cargo.toml ---'
sed -n '1,35p' crates/codegraph-sboxes/Cargo.toml
echo
echo '--- Cargo.lock rhai package ---'
sed -n '2065,2085p' Cargo.lock
echo
echo '--- crates/codegraph-sboxes/src/rhai.rs outline ---'
ast-grep outline crates/codegraph-sboxes/src/rhai.rs --view expanded || true
echo
echo '--- Relevant Rhai source lines ---'
sed -n '1,180p' crates/codegraph-sboxes/src/rhai.rsRepository: hungpham10/codegraph-rs
Length of output: 8714
🌐 Web query:
docs.rs rhai 1.25.1 Engine set_max_operations zero unlimited
💡 Result:
In Rhai 1.25.1, the Engine::set_max_operations method is used to set the maximum number of operations a script is allowed to perform, which helps prevent scripts from consuming excessive system resources [1][2]. To set these limits: - Unlimited: Passing 0 to set_max_operations sets the limit to unlimited [1][2]. This is the default behavior [2]. - Specific Limit: Passing a non-zero u64 value defines the maximum number of operations permitted [1][2]. If a script exceeds this limit, it will terminate with an error [2]. Note that this functionality is not available if the Rhai Engine is compiled with the unchecked feature enabled, as that feature disables various safety checks for higher performance [1][2][3].
Citations:
- 1: https://docs.rs/crate/rhai/latest/source/src/api/limits.rs
- 2: https://rhai.rs/book/safety/max-operations.html
- 3: https://rhai.rs/book/safety/limits.html
Bound Rhai mock execution.
RhaiMockLib::load and RhaiMockLib::empty create Engine::new() with the default unlimited operation cap, and RhaiMockLib::register still uses this same engine before compiling inline source. Since set_max_operations(0) means unlimited in Rhai 1.25.1, configure a finite operation limit and related depth/allocation limits before compiling mock files and inline sources.
🤖 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-sboxes/src/rhai.rs` around lines 53 - 56, Configure finite
operation, depth, and allocation limits on the Rhai Engine used by
RhaiMockLib::load, RhaiMockLib::empty, and RhaiMockLib::register before
compiling mock files or inline sources; do not use set_max_operations(0), since
it disables the limit. Reuse a shared bounded-engine setup so every compilation
path receives the same limits.
| if let Ok(script) = std::fs::read_to_string(&path) { | ||
| if let Ok(compiled) = engine.compile(&script) { | ||
| for sig in compiled.iter_functions() { | ||
| names.insert(sig.name.to_string()); | ||
| } | ||
| ast = ast.merge(&compiled); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace every path that suppresses or maps mock errors.
rg -n -C 4 \
'load_with_mocks|register\(|MockError::Script|MockError::NotFound|call_fn|missing\.push' \
crates/codegraph-sboxes/src crates/codegraph-sboxes/tests
# Confirm the dependency declaration before selecting the AST metadata API.
rg -n -C 3 'rhai\s*=|name = "rhai"' crates/codegraph-sboxes/Cargo.toml Cargo.lockRepository: hungpham10/codegraph-rs
Length of output: 9840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== rhai.rs relevant implementation ==\n'
sed -n '1,160p' crates/codegraph-sboxes/src/rhai.rs
printf '\n== runtime mock calling path ==\n'
sed -n '150,185p' crates/codegraph-sboxes/src/runtime.rs
printf '\n== codegen mock validation path ==\n'
sed -n '550,595p' crates/codegraph-sboxes/src/codegen.rs
printf '\n== trace type and missing handling sites ==\n'
rg -n -C 3 'struct Trace|missing:|trace\.borrow_mut\(\)\.missing|missing\.push|fn mock\(' crates/codegraph-sboxes/srcRepository: hungpham10/codegraph-rs
Length of output: 11132
Propagate invalid/mock runtime failures instead of swallowing them.
load() swallows file read and Rhai compile errors, and load_with_mocks() ignores RhaiMockLib::register errors. Later, mock_dispatch_trampoline() maps every failed mock call to the normal 0 result. Treat these as real mock failures: fail mock setup on bad source/incorrect signature, and return a distinct runtime error when a configured mock does not execute successfully.
📍 Affects 2 files
crates/codegraph-sboxes/src/rhai.rs#L70-L77(this comment)crates/codegraph-sboxes/src/rhai.rs#L90-L113crates/codegraph-sboxes/src/runtime.rs#L173-L182
🤖 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-sboxes/src/rhai.rs` around lines 70 - 77, Update
crates/codegraph-sboxes/src/rhai.rs:70-77 in load() to propagate file-read and
Rhai compilation errors instead of ignoring them; update
crates/codegraph-sboxes/src/rhai.rs:90-113 in load_with_mocks() to propagate
RhaiMockLib::register failures, including invalid mock signatures; update
crates/codegraph-sboxes/src/runtime.rs:173-182 in mock_dispatch_trampoline() to
return a distinct runtime error when a configured mock fails, rather than
converting every failure to the normal 0 result.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Summary by CodeRabbit
visualizecommand (replaced by sandbox-based workflows).