Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI (DocsArgs)
participant Resolver as Resolver
participant ModuleParser as ModuleParser
participant Cache as Local Cache
participant Registry as crates.io
participant HTTP as HTTP Client
User->>CLI: Query (e.g., "crate::mod::Item")
CLI->>Resolver: init(workspace_path, registry, client)
Resolver->>ModuleParser: try resolve from local metadata
alt Local metadata found
ModuleParser->>Resolver: ResolvedMetadataPath
Resolver->>CLI: return result
else Not found locally
Resolver->>Cache: check package cache
alt Cache hit
Cache->>Resolver: cached crate root
Resolver->>ModuleParser: resolve in cached crate
ModuleParser->>Resolver: ResolvedMetadataPath
Resolver->>CLI: return result
else Cache miss
Resolver->>HTTP: fetch crate info & tarball
HTTP->>Registry: GET /crates/...
Registry-->>HTTP: meta & tarball
HTTP-->>Resolver: download archive
Resolver->>Cache: extract & cache crate
Resolver->>ModuleParser: resolve in cached crate
ModuleParser->>Resolver: ResolvedMetadataPath
Resolver->>CLI: return result
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/module-parser/src/metadata.rs (1)
266-279: DuplicatedTempDirExttest helper.The
TempDirExttrait and its implementation are identical in bothsource.rsandmetadata.rstest modules. Consider extracting this to a shared test utilities module to reduce duplication.♻️ Suggested approach
Create a shared test utility, for example in
crates/module-parser/src/test_utils.rs:#[cfg(test)] pub(crate) mod test_utils { use std::fs; use tempfile::TempDir; pub trait TempDirExt { fn write(&self, relative_path: &str, content: &str); } impl TempDirExt for TempDir { fn write(&self, relative_path: &str, content: &str) { let path = self.path().join(relative_path); if let Some(parent) = path.parent() { fs::create_dir_all(parent).expect("failed to create parent dir"); } fs::write(path, content).expect("failed to write test file"); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/metadata.rs` around lines 266 - 279, Duplicate TempDirExt test helper found in source.rs and metadata.rs; extract it into a shared test utility module (e.g., create a cfg(test) pub(crate) mod test_utils) and move the TempDirExt trait and its impl for tempfile::TempDir (the write(&self, relative_path: &str, content: &str) method) there; then replace the duplicated definitions in source.rs and metadata.rs with use of the shared test_utils (import the trait) so both test modules call TempDirExt::write on TempDir without duplication.crates/module-parser/Cargo.toml (1)
17-22: Consider usingcargo addfor new dependencies.New dependencies (
prettyplease,tempfile) were added by manually editingCargo.toml. As per coding guidelines: "Always prefercargo addover manually editing Cargo.toml for adding dependencies."For future additions:
cargo add prettyplease@0.2.37 -p module-parser cargo add tempfile@3.27.0 -p module-parser --dev🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/Cargo.toml` around lines 17 - 22, The new deps were added by hand—replace the manual additions of prettyplease = "0.2.37" and tempfile = "3.27.0" in the module-parser Cargo.toml with additions made via cargo add; specifically remove the manual entries for prettyplease and tempfile and run cargo add prettyplease@0.2.37 -p module-parser and cargo add tempfile@3.27.0 -p module-parser --dev so the dependency metadata and lockfile are correctly updated (targets: entries named "prettyplease" and the dev-dep "tempfile").crates/module-parser/src/source.rs (1)
252-258: Test attribute detection may have false positives.The
is_test_attrfunction checks if"test"appears anywhere in the token string for#[cfg(...)]or#[cfg_attr(...)]attributes. This could match unintended cases like#[cfg(feature = "contest")]or#[cfg(attestation)].Consider a more precise check:
♻️ Suggested improvement
fn is_test_attr(attr: &Attribute) -> bool { if attr.path().is_ident("test") { return true; } match &attr.meta { Meta::List(list) if list.path.is_ident("cfg") || list.path.is_ident("cfg_attr") => { - list.tokens.to_string().contains("test") + let tokens = list.tokens.to_string(); + // Match "test" as a standalone identifier, not as a substring + tokens.split(|c: char| !c.is_alphanumeric() && c != '_') + .any(|word| word == "test") } _ => false, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/source.rs` around lines 252 - 258, The current is_test_attr implementation is too fuzzy (it just checks list.tokens.to_string().contains("test")) and can match substrings like "contest"; instead parse the Meta::List contents precisely: iterate list.nested and look for NestedMeta::Meta entries where the inner Meta is either Meta::Path or Meta::NameValue whose path/ident equals "test", and for cfg_attr handle the same nested structure; update the match arm that inspects Meta::List (in is_test_attr) to examine list.nested and return true only when a nested Meta::Path/NameValue has path.is_ident("test") (avoid stringifying tokens).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/docs/mod.rs`:
- Around line 592-600: The current code around
cached_package_versions/cached_versions and the update_latest_symlink call can
race if multiple cyberfabric docs processes run concurrently; to fix, serialize
access when updating the latest symlink by acquiring a process- or file-based
lock on the package_root (or a dedicated lock file in package_root) before
checking latest_link and calling update_latest_symlink, release the lock after
the update, and ensure errors still propagate; alternatively add a comment/doc
note near cached_package_versions/update_latest_symlink documenting the
concurrency limitation if you prefer not to implement locking.
- Around line 414-421: The code in parse_dependency_spec currently uses
Version::parse on the dependency string and silently drops failures (setting
version to None), which loses version requirement info like "^1.0" or ">=1.0";
update parse_dependency_spec to attempt parsing as an exact semver with
Version::parse and, if that fails, parse the string as a semver::VersionReq and
store that requirement (or a new field) so registry fallbacks can use the
requirement; specifically modify the DependencySpec representation or add a
version_req field and change parse_dependency_spec to try
Version::parse(value.as_str()) then fall back to
semver::VersionReq::parse(value.as_str()), preserving either the parsed Version
or VersionReq instead of silently discarding parse errors.
---
Nitpick comments:
In `@crates/module-parser/Cargo.toml`:
- Around line 17-22: The new deps were added by hand—replace the manual
additions of prettyplease = "0.2.37" and tempfile = "3.27.0" in the
module-parser Cargo.toml with additions made via cargo add; specifically remove
the manual entries for prettyplease and tempfile and run cargo add
prettyplease@0.2.37 -p module-parser and cargo add tempfile@3.27.0 -p
module-parser --dev so the dependency metadata and lockfile are correctly
updated (targets: entries named "prettyplease" and the dev-dep "tempfile").
In `@crates/module-parser/src/metadata.rs`:
- Around line 266-279: Duplicate TempDirExt test helper found in source.rs and
metadata.rs; extract it into a shared test utility module (e.g., create a
cfg(test) pub(crate) mod test_utils) and move the TempDirExt trait and its impl
for tempfile::TempDir (the write(&self, relative_path: &str, content: &str)
method) there; then replace the duplicated definitions in source.rs and
metadata.rs with use of the shared test_utils (import the trait) so both test
modules call TempDirExt::write on TempDir without duplication.
In `@crates/module-parser/src/source.rs`:
- Around line 252-258: The current is_test_attr implementation is too fuzzy (it
just checks list.tokens.to_string().contains("test")) and can match substrings
like "contest"; instead parse the Meta::List contents precisely: iterate
list.nested and look for NestedMeta::Meta entries where the inner Meta is either
Meta::Path or Meta::NameValue whose path/ident equals "test", and for cfg_attr
handle the same nested structure; update the match arm that inspects Meta::List
(in is_test_attr) to examine list.nested and return true only when a nested
Meta::Path/NameValue has path.is_ident("test") (avoid stringifying tokens).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d4887a8-bb1a-4404-94a0-bc3b7e19393c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlSKILLS.mdcrates/cli/src/docs/mod.rscrates/cli/src/lib.rscrates/module-parser/Cargo.tomlcrates/module-parser/src/lib.rscrates/module-parser/src/metadata.rscrates/module-parser/src/source.rs
Bechma
force-pushed
the
add-doc-retrieval
branch
from
6ddc7a5 to
99fece5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/module-parser/src/metadata.rs (2)
141-167: Consider simplifying the flat_map iteration.The current pattern with
Vec::new().into_iter()andcollect::<Vec<_>>().into_iter()works but is verbose. Afilter_mapwithflattenor usingflat_mapwithOptioncould be cleaner.♻️ Suggested simplification
fn select_library_target(packages: &[Package], name: &str) -> Option<LibraryTarget> { let mut candidates = packages .iter() - .flat_map(|package| { - if package.name != name { - return Vec::new().into_iter(); - } - - package - .targets - .iter() - .filter(|target| target.is_lib()) - .map(move |target| to_library_target(package, target)) - .collect::<Vec<_>>() - .into_iter() - }) + .filter(|package| package.name == name) + .flat_map(|package| { + package + .targets + .iter() + .filter(|target| target.is_lib()) + .map(move |target| to_library_target(package, target)) + }) .collect::<Vec<_>>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/metadata.rs` around lines 141 - 167, The iterator building in select_library_target is verbose: replace the current packages.iter().flat_map{... Vec::new().into_iter / collect().into_iter } pattern with a direct iterator chain that first filters packages by name and then flat_maps each package's targets into library targets via to_library_target; e.g., use packages.iter().filter(|p| p.name == name).flat_map(|p| p.targets.iter().filter(|t| t.is_lib()).map(|t| to_library_target(p, t))) to produce candidates before sorting, which removes the temporary Vec constructions and clarifies intent while keeping the existing sort and return logic.
63-76: Consider error handling semantics for invalid path segments.If
select_library_targetfinds the package butresolve_rust_pathfails (e.g., the path segments point to a non-existent module), this returns an error rather thanNone. This might be intentional, but consider whether callers would benefit from distinguishing between "package not found" (None) and "package found but path invalid" (error), or if both should returnNone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/metadata.rs` around lines 63 - 76, The code currently propagates errors from resolve_rust_path (via the ? after calling it) which mixes "package found but path invalid" with real errors; change this to explicitly handle resolve_rust_path failures and return Ok(None) instead of propagating the error so callers can distinguish "package not found" (None) from "package found but path invalid" (also None in this model). Concretely, replace the direct call using ? to a match or map_err-handling around resolve_rust_path in the block that builds ResolvedMetadataPath (the ResolvedRustPath destructuring and subsequent Ok(Some(ResolvedMetadataPath { ... }))) so that on Err(_) you return Ok(None), and on Ok(resolved) you continue constructing and returning Ok(Some(...)).crates/module-parser/Cargo.toml (1)
17-23: Consider using workspace dependencies for consistency.The new dependencies use hardcoded versions while existing dependencies (
serde,syn) reference the workspace. For consistency and centralized version management, consider adding these to the workspaceCargo.tomland referencing them with{ workspace = true }. (Versions verified:prettyplease@0.2.37,proc-macro2@1.0.106, andtempfile@3.27.0are all current.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/Cargo.toml` around lines 17 - 23, The dependency entries prettyplease = "0.2.37", proc-macro2 = "1.0.106", and dev-dependency tempfile = "3.27.0" should be moved into the workspace Cargo.toml and referenced here with { workspace = true } for consistency with serde and syn; update the module-parser/Cargo.toml to replace those versioned lines with prettyplease = { workspace = true }, proc-macro2 = { workspace = true }, and tempfile = { workspace = true } (keeping them under [dependencies] or [dev-dependencies] as appropriate) and add the corresponding entries with the exact versions in the workspace Cargo.toml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/docs/mod.rs`:
- Around line 610-614: The condition currently compares latest_link (a path like
.../latest) to latest_root (a path like .../<version>), which is always
different; change the comparison to compare the symlink's target basename or
resolved target to the latest version instead. Specifically, when you have
cached_versions.first() -> (latest_version, latest_root), resolve or read the
symlink target stored in latest_link and compare its filename or basename (or
the resolved path's last component) to latest_version.to_string(); only call
update_latest_symlink(&package_root, &latest_version.to_string())? when they
differ. Use the existing symbols cached_versions, latest_link, latest_root,
latest_version, and update_latest_symlink to locate and modify the logic.
- Around line 268-292: The code indexes target_segments[0] without checking for
an empty vector; add an early guard for target_segments.is_empty() before
attempting to index: if empty, construct a NextStep like the other branches
(preferred_path.to_path_buf(), query: build_query(package_name, &[]),
requested_version: None) and return it, otherwise proceed to look up
dependencies.get(&target_segments[0]) and compute remaining_segments and
next_query as before (affecting the logic around parse_dependencies,
resolve_bare_relative_reexport, build_query, and NextStep).
- Around line 1-932: Add a CI workflow that runs on pull requests to enforce
cargo fmt, cargo clippy, and cargo test: create a GitHub Actions job that checks
out the repo, installs the Rust toolchain (stable), runs cargo fmt -- --check,
cargo clippy -- -D warnings, and cargo test (optionally with cached
cargo/registry directories), and fails the run on any non-zero exit so PRs
cannot merge without fixes; include the workflow trigger for pull_request and
optionally push, and ensure the job runs for the crate containing the
DocsArgs/resolve_query_recursive/next_reexport_step code so formatting, lints,
and tests covering those functions are validated automatically.
---
Nitpick comments:
In `@crates/module-parser/Cargo.toml`:
- Around line 17-23: The dependency entries prettyplease = "0.2.37", proc-macro2
= "1.0.106", and dev-dependency tempfile = "3.27.0" should be moved into the
workspace Cargo.toml and referenced here with { workspace = true } for
consistency with serde and syn; update the module-parser/Cargo.toml to replace
those versioned lines with prettyplease = { workspace = true }, proc-macro2 = {
workspace = true }, and tempfile = { workspace = true } (keeping them under
[dependencies] or [dev-dependencies] as appropriate) and add the corresponding
entries with the exact versions in the workspace Cargo.toml.
In `@crates/module-parser/src/metadata.rs`:
- Around line 141-167: The iterator building in select_library_target is
verbose: replace the current packages.iter().flat_map{... Vec::new().into_iter /
collect().into_iter } pattern with a direct iterator chain that first filters
packages by name and then flat_maps each package's targets into library targets
via to_library_target; e.g., use packages.iter().filter(|p| p.name ==
name).flat_map(|p| p.targets.iter().filter(|t| t.is_lib()).map(|t|
to_library_target(p, t))) to produce candidates before sorting, which removes
the temporary Vec constructions and clarifies intent while keeping the existing
sort and return logic.
- Around line 63-76: The code currently propagates errors from resolve_rust_path
(via the ? after calling it) which mixes "package found but path invalid" with
real errors; change this to explicitly handle resolve_rust_path failures and
return Ok(None) instead of propagating the error so callers can distinguish
"package not found" (None) from "package found but path invalid" (also None in
this model). Concretely, replace the direct call using ? to a match or
map_err-handling around resolve_rust_path in the block that builds
ResolvedMetadataPath (the ResolvedRustPath destructuring and subsequent
Ok(Some(ResolvedMetadataPath { ... }))) so that on Err(_) you return Ok(None),
and on Ok(resolved) you continue constructing and returning Ok(Some(...)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29980eb5-886d-4b6a-89bb-3367cdcd3e4f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlSKILLS.mdcrates/cli/src/docs/mod.rscrates/cli/src/lib.rscrates/module-parser/Cargo.tomlcrates/module-parser/src/lib.rscrates/module-parser/src/metadata.rscrates/module-parser/src/source.rscrates/module-parser/src/test_utils.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- Cargo.toml
- SKILLS.md
- crates/module-parser/src/source.rs
- crates/module-parser/src/lib.rs
- crates/cli/src/lib.rs
Bechma
force-pushed
the
add-doc-retrieval
branch
2 times, most recently
from
4c4c8c0 to
c0603ea
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/cli/src/docs/mod.rs (1)
322-346:⚠️ Potential issue | 🟠 MajorGuard against empty re-export targets before indexing.
At Line 323,
target_segments[0]can panic if the extracted target is empty. Add an earlysplit_first()guard and branch safely.Proposed fix
- let dependencies = parse_dependencies(crate_root)?; - let Some(dep) = find_dependency_spec(&dependencies, &target_segments[0]) else { + let Some((first_target, remaining_segments)) = target_segments.split_first() else { + return Ok(None); + }; + + let dependencies = parse_dependencies(crate_root)?; + let Some(dep) = find_dependency_spec(&dependencies, first_target) else { if let Some(next_query) = resolve_bare_relative_reexport( crate_root, package_name, &target_segments, &containing_module_segments, )? { return Ok(Some(NextStep { preferred_path: preferred_path.to_path_buf(), query: next_query, requested_version: None, })); } let next_query = build_query(package_name, &target_segments); return Ok(Some(NextStep { preferred_path: preferred_path.to_path_buf(), query: next_query, requested_version: None, })); }; - let remaining_segments = &target_segments[1..]; let next_query = build_query(&dep.package_name, remaining_segments);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/docs/mod.rs` around lines 322 - 346, Replace the direct indexing of target_segments[0] with a safe split_first() guard: use let Some((first_segment, remaining_segments)) = target_segments.split_first() else { return Ok(None); } and then call find_dependency_spec(&dependencies, first_segment); if find_dependency_spec finds no dependency still call resolve_bare_relative_reexport(...) as before (using target_segments where appropriate), and when a dep is found use remaining_segments to build the next_query via build_query(&dep.package_name, remaining_segments) and construct the NextStep; this prevents panics when target_segments is empty while keeping the existing resolve_bare_relative_reexport and build_query logic intact.
🧹 Nitpick comments (2)
crates/module-parser/src/source.rs (1)
83-91: Consider documenting the use-fallback behavior.The fallback to re-export
useitems when a direct item isn't found is intentional and tested, but a brief inline comment would help future maintainers understand why re-exports are captured as a fallback rather than matched immediately.📝 Suggested documentation
+ // If the target name matches a use-item (re-export), capture it as a fallback. + // Direct items (modules, structs, etc.) take precedence over re-exports. if segments.len() == 1 && let Item::Use(use_item) = item && use_tree_contains_name(&use_item.tree, segment)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/source.rs` around lines 83 - 91, Add an inline comment explaining why re-exported `use` items are captured as a fallback rather than matched immediately: annotate the block that checks segments.len() == 1 && let Item::Use(use_item) = item && use_tree_contains_name(&use_item.tree, segment) (where it sets use_fallback = Some(ResolvedRustPath { source_path: current_file.to_path_buf(), source: render_item(Item::Use(use_item)) })) to state that re-exports are recorded only as a fallback to avoid shadowing direct definitions and to allow preference for direct matches first when resolving paths.crates/module-parser/src/metadata.rs (1)
69-71: Consider distinguishing "not found" from resolution errors.Converting all
resolve_rust_patherrors toOk(None)may hide genuine failures (e.g., I/O errors, parse errors). The caller cannot distinguish between "query doesn't match anything" and "resolution failed unexpectedly."💡 Suggested approach
- let Ok(resolved) = resolve_rust_path(&library_target.root_source_path, &segments) else { - return Ok(None); - }; + let resolved = match resolve_rust_path(&library_target.root_source_path, &segments) { + Ok(resolved) => resolved, + Err(e) => { + // Log at debug level for troubleshooting, but treat as "not found" + // since the query may reference items that don't exist in this version + eprintln!("debug: path resolution failed: {e}"); + return Ok(None); + } + };Alternatively, propagate errors and let callers decide whether to fall back to other resolution strategies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/module-parser/src/metadata.rs` around lines 69 - 71, The current code swallows all errors from resolve_rust_path by turning any Err into Ok(None); change the handling in metadata.rs so you match on the Result from resolve_rust_path instead of using let Ok(...) = ..., and only convert a true "not found" outcome to Ok(None) while propagating other errors (return Err(e) or use ?). Specifically, inspect the error variant returned by resolve_rust_path (or update it to return a distinguishable NotFound variant) and in the match for resolve_rust_path(&library_target.root_source_path, &segments) map NotFound => Ok(None) and all other Err cases => propagate the error so callers can decide on fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/cli/src/docs/mod.rs`:
- Around line 322-346: Replace the direct indexing of target_segments[0] with a
safe split_first() guard: use let Some((first_segment, remaining_segments)) =
target_segments.split_first() else { return Ok(None); } and then call
find_dependency_spec(&dependencies, first_segment); if find_dependency_spec
finds no dependency still call resolve_bare_relative_reexport(...) as before
(using target_segments where appropriate), and when a dep is found use
remaining_segments to build the next_query via build_query(&dep.package_name,
remaining_segments) and construct the NextStep; this prevents panics when
target_segments is empty while keeping the existing
resolve_bare_relative_reexport and build_query logic intact.
---
Nitpick comments:
In `@crates/module-parser/src/metadata.rs`:
- Around line 69-71: The current code swallows all errors from resolve_rust_path
by turning any Err into Ok(None); change the handling in metadata.rs so you
match on the Result from resolve_rust_path instead of using let Ok(...) = ...,
and only convert a true "not found" outcome to Ok(None) while propagating other
errors (return Err(e) or use ?). Specifically, inspect the error variant
returned by resolve_rust_path (or update it to return a distinguishable NotFound
variant) and in the match for
resolve_rust_path(&library_target.root_source_path, &segments) map NotFound =>
Ok(None) and all other Err cases => propagate the error so callers can decide on
fallback behavior.
In `@crates/module-parser/src/source.rs`:
- Around line 83-91: Add an inline comment explaining why re-exported `use`
items are captured as a fallback rather than matched immediately: annotate the
block that checks segments.len() == 1 && let Item::Use(use_item) = item &&
use_tree_contains_name(&use_item.tree, segment) (where it sets use_fallback =
Some(ResolvedRustPath { source_path: current_file.to_path_buf(), source:
render_item(Item::Use(use_item)) })) to state that re-exports are recorded only
as a fallback to avoid shadowing direct definitions and to allow preference for
direct matches first when resolving paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fc13353-2286-48ab-aede-f0086a66359a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.ymlCargo.tomlSKILLS.mdcrates/cli/Cargo.tomlcrates/cli/src/docs/mod.rscrates/cli/src/lib.rscrates/module-parser/Cargo.tomlcrates/module-parser/src/lib.rscrates/module-parser/src/metadata.rscrates/module-parser/src/source.rscrates/module-parser/src/test_utils.rsrust-toolchain.toml
✅ Files skipped from review due to trivial changes (1)
- rust-toolchain.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/module-parser/src/lib.rs
- crates/module-parser/src/test_utils.rs
Bechma
force-pushed
the
add-doc-retrieval
branch
from
c0603ea to
1ea8d4f
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/cli/src/docs/mod.rs (2)
627-635: Inherit workspace dependencypathalong with package/version.
parse_dependency_spec_with_workspacecurrently mergespackage_nameandversionbut notpath. That can degrade preferred-path routing for workspace-inherited path deps.Proposed patch
if dependency_uses_workspace_inheritance(value) && let Some(workspace_spec) = workspace_deps.and_then(|deps| deps.get(alias)) { if spec.package_name == alias { spec.package_name.clone_from(&workspace_spec.package_name); } if spec.version.is_none() { spec.version.clone_from(&workspace_spec.version); } + if spec.path.is_none() { + spec.path.clone_from(&workspace_spec.path); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/docs/mod.rs` around lines 627 - 635, In parse_dependency_spec_with_workspace, when dependency_uses_workspace_inheritance(...) finds a workspace_spec, we currently clone package_name and version from workspace_spec into spec but omit the path; add logic to also inherit the path by copying workspace_spec.path into spec.path when spec.path is None (or empty as appropriate), mirroring how package_name and version are handled (use the same clone_from call pattern referencing spec.path and workspace_spec.path) so workspace-inherited dependencies preserve their preferred path routing.
677-693: Consider bounded retry/backoff for registry HTTP calls.These requests fail fast on transient 429/5xx/network blips; a small retry policy would improve CLI reliability without changing behavior on hard failures.
Also applies to: 721-730
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/docs/mod.rs` around lines 677 - 693, The registry HTTP call using client.get(&crate_url).send().await (and the similar block at lines 721–730) should use a bounded retry with exponential backoff for transient failures: wrap the send()/error_for_status()/json::<ExactCrateResponse>().await sequence in a retry loop that retries on transient network errors, timeouts, 429, and 5xx responses up to a small max attempts with increasing delay, but immediately return Ok(None) when response.status() == StatusCode::NOT_FOUND and propagate hard errors after retries; ensure each failure preserves the original with_context messages (e.g., "request failed for '{crate_name}'", "registry returned an error for '{crate_name}'", "invalid crate metadata for '{crate_name}'") so logs/context remain meaningful..github/workflows/ci.yml (1)
15-16: Pin GitHub Actions to immutable SHAs.Line 15 and Line 16 use floating refs (
@v6,@v2). Pinning to commit SHAs improves supply-chain integrity and reproducibility.#!/bin/bash # Read-only check: list workflow actions and flag non-SHA refs. set -euo pipefail rg -n '^\s*-\s*uses:\s*[^@]+@([^\s]+)' .github/workflows | while IFS=: read -r file line text; do ref="$(printf '%s' "$text" | sed -E 's/.*@([[:alnum:]_.-]+).*/\1/')" if ! printf '%s' "$ref" | rg -q '^[0-9a-fA-F]{40}$'; then printf '%s:%s -> %s\n' "$file" "$line" "$text" fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 15 - 16, Replace the floating refs used in the workflow (actions/checkout@v6 and Swatinem/rust-cache@v2) with their immutable commit SHAs: locate the two uses: the string "actions/checkout@v6" and "Swatinem/rust-cache@v2" in the workflow and update each to use the exact 40-char git commit SHA for the desired release (e.g., actions/checkout@<commit-sha>), fetching the correct SHA from the action's official GitHub repository tags or commit history and verify the checksum before committing to ensure reproducibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 15-16: Replace the floating refs used in the workflow
(actions/checkout@v6 and Swatinem/rust-cache@v2) with their immutable commit
SHAs: locate the two uses: the string "actions/checkout@v6" and
"Swatinem/rust-cache@v2" in the workflow and update each to use the exact
40-char git commit SHA for the desired release (e.g.,
actions/checkout@<commit-sha>), fetching the correct SHA from the action's
official GitHub repository tags or commit history and verify the checksum before
committing to ensure reproducibility.
In `@crates/cli/src/docs/mod.rs`:
- Around line 627-635: In parse_dependency_spec_with_workspace, when
dependency_uses_workspace_inheritance(...) finds a workspace_spec, we currently
clone package_name and version from workspace_spec into spec but omit the path;
add logic to also inherit the path by copying workspace_spec.path into spec.path
when spec.path is None (or empty as appropriate), mirroring how package_name and
version are handled (use the same clone_from call pattern referencing spec.path
and workspace_spec.path) so workspace-inherited dependencies preserve their
preferred path routing.
- Around line 677-693: The registry HTTP call using
client.get(&crate_url).send().await (and the similar block at lines 721–730)
should use a bounded retry with exponential backoff for transient failures: wrap
the send()/error_for_status()/json::<ExactCrateResponse>().await sequence in a
retry loop that retries on transient network errors, timeouts, 429, and 5xx
responses up to a small max attempts with increasing delay, but immediately
return Ok(None) when response.status() == StatusCode::NOT_FOUND and propagate
hard errors after retries; ensure each failure preserves the original
with_context messages (e.g., "request failed for '{crate_name}'", "registry
returned an error for '{crate_name}'", "invalid crate metadata for
'{crate_name}'") so logs/context remain meaningful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d20a0354-d4e7-4c5c-9c8a-f134e0bdc4d3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.ymlCargo.tomlSKILLS.mdcrates/cli/Cargo.tomlcrates/cli/src/docs/mod.rscrates/cli/src/lib.rscrates/module-parser/Cargo.tomlcrates/module-parser/src/lib.rscrates/module-parser/src/metadata.rscrates/module-parser/src/source.rscrates/module-parser/src/test_utils.rsrust-toolchain.toml
✅ Files skipped from review due to trivial changes (1)
- rust-toolchain.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/cli/Cargo.toml
- crates/module-parser/src/lib.rs
- crates/module-parser/src/test_utils.rs
- crates/cli/src/lib.rs
- crates/module-parser/src/metadata.rs
- SKILLS.md
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Bechma
force-pushed
the
add-doc-retrieval
branch
from
1ea8d4f to
8a0aa57
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/module-parser/src/metadata.rs`:
- Around line 9-13: The public API loses PackageId identity: update the public
structs and selection to preserve exact package identity by either (A) adding a
PackageId field to LibraryMapping and ResolvedMetadataPath so callers can see
the selected identity (modify the definitions of LibraryMapping and
ResolvedMetadataPath and propagate PackageId from the internal LibraryTarget),
or (B) change select_library_target to accept a PackageId and match by exact
identity instead of matching only by package_name + version sort; ensure all
call sites and any construction of LibraryMapping/ResolvedMetadataPath use the
PackageId from LibraryTarget so downstream resolution can unambiguously
reference the chosen package identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ae805f7-a9b1-4891-9dc6-65d7de2602a7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.ymlCargo.tomlSKILLS.mdcrates/cli/Cargo.tomlcrates/cli/src/docs/mod.rscrates/cli/src/lib.rscrates/module-parser/Cargo.tomlcrates/module-parser/src/lib.rscrates/module-parser/src/metadata.rscrates/module-parser/src/source.rscrates/module-parser/src/test_utils.rsrust-toolchain.toml
✅ Files skipped from review due to trivial changes (1)
- SKILLS.md
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/module-parser/src/test_utils.rs
- crates/cli/src/lib.rs
- crates/module-parser/Cargo.toml
- crates/module-parser/src/source.rs
- rust-toolchain.toml
- crates/cli/Cargo.toml
- .github/workflows/ci.yml
- Cargo.toml
- crates/cli/src/docs/mod.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Dependencies
CI / Tests