Fix toolchain invocation for Nix and resolve partial crate versions#48
Fix toolchain invocation for Nix and resolve partial crate versions#48christian-blades-cb wants to merge 14 commits intosnowmead:mainfrom
Conversation
|
NOTE: This depends on #47 to be useful, otherwise it's still outputting a broken binary. Wanted to keep these PR's separate, tho. |
There was a problem hiding this comment.
The rustup changes are good, but regarding the crate versions, I would prefer we add the semver crate and when we execute the path to download a crate, if the version is not found, we return a semver parsed ordered list of versions. That way the agents can pick and choose dynamically rather then us choosing for them.
This makes sense. I'll work on an implementation. |
7499ae2 to
f2f0f15
Compare
|
Also threw in another fix. sha256 on the toolchain was set to empty-string which prevents builds. Tried to stick to the spirit of the change by just telling it to use the latest nightly. |
9c045f7 to
52e2215
Compare
There was a problem hiding this comment.
🟡 Error message uses original unresolved version instead of resolved version
In get_dependencies, the error message at line 103-104 uses params.version (the original, potentially partial version like "1") instead of the resolved version variable (like "1.0.215"). When load_dependencies fails after successful version resolution, the user sees a confusing message like "Dependencies not available for serde-1" instead of "Dependencies not available for serde-1.0.215".
Was this helpful? React with 👍 or 👎 to provide feedback.
| match self.resolve_crates_io_version(name, version).await { | ||
| Ok(v) => Ok(v), | ||
| Err(e) => { | ||
| if e.to_string().contains("Available versions") { |
There was a problem hiding this comment.
🟡 resolve_version swallows "no matching version" error when all versions are yanked
The resolve_version method at rust-docs-mcp/src/cache/service.rs:53 decides whether to propagate an error from resolve_crates_io_version by checking if the error string contains "Available versions". However, format_available_versions at rust-docs-mcp/src/cache/downloader.rs:139-140 returns "No versions available." (without the "Available versions" substring) when the versions list is empty (e.g., all versions are yanked). This means the bail message "No matching version found for ... No versions available." does NOT contain "Available versions", so resolve_version falls through to Ok(version.to_string()), silently swallowing a valid error. The caller then proceeds with the unresolved partial version string, leading to a confusing download failure instead of the intended version-not-found error.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
This Devin thing is kinda neat. Definitely caught some good edge cases. |
Use `rustup run <toolchain>` instead of the `+toolchain` syntax for cargo/rustdoc invocations, since the latter only works when the binaries are rustup proxies (not the case in Nix environments). Add version resolution for crates.io so partial versions like "9" are resolved to the latest matching release (e.g. "9.3.1") via the crates.io API, instead of causing HTTP 400 errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of returning a generic error when a version is not found on crates.io, fetch all available versions, parse and sort them with the semver crate, and include the list in the error response. This lets agents dynamically pick a valid version rather than guessing. Also handle 403 responses from crates.io (returned for nonexistent versions) in addition to 404. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review notes from Devin: version resolution was only applied inside ensure_crate_docs, meaning the workspace member path in ensure_crate_or_member_docs and the download path in cache_crate_with_source both bypassed it. Partial versions like "1" or "1.0" would hit crates.io unresolved and fail with 403/404. Move resolution into ensure_crate_or_member_docs before the member/non-member branch so both paths benefit, and add resolution in cache_crate_with_source for crates.io sources before any download or cache-check logic runs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch from fromToolchainFile (which pins to the exact dated nightly in rust-toolchain.toml and requires a manual sha256 update on every bump) to fenix's rolling latest nightly with explicit components. This aligns the flake with the relaxed toolchain selection introduced in a175ca4 — the runtime's resolve_toolchain probe validates compatibility at startup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addresses review from Devin
Devin caught this, using the param here would cause a mismatch if resolved version differs from the input (ie "1" vs "1.0.0")
37049b0 to
23950da
Compare
…nsure_crate_or_member_docs The internal resolve_crates_io_version call bypassed the is_cached guard, causing unnecessary HTTP requests for already-cached or non-crates.io crates. Replace with resolve_version which handles all cases correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Runs nix flake check (build, clippy, fmt, nextest) and a dev shell smoke test on every push/PR to main. `nix flake check` executes tests in a sandbox that doesn't have network access. Here we add an ignore attribute to tests that need network access. We include these tests in the CI workflows since we know those environments have network access. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bc423c3 to
d160f82
Compare
|
@christian-blades-cb in my experience it is not too bad for catching edge cases |
…onments - Use `cargo +toolchain --version` instead of `rustdoc +toolchain --version` for the availability probe. Rustdoc exits 0 for --version even with an unrecognized +toolchain arg (false positive); cargo correctly errors. - Add native nightly fallback for Nix/fenix environments where the nightly toolchain is in PATH directly without rustup. Detects via `rustdoc --version` containing "nightly" and uses empty toolchain string (no +prefix in commands). - Add "error: could not document" to is_compilation_error so cargo rustdoc failures (which say "could not document" not "could not compile") trigger the feature fallback strategy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…coping Replace #[cfg_attr(not(target_os = "macos"), ignore)] on the bevy test with #[cfg(target_os = "macos")] so it is compile-excluded on Linux and cannot be reached by --include-ignored in CI. Add test_feature_fallback_with_broken_feature: creates a local crate fixture with a `broken` feature that uses compile_error!, caches it via the service, and asserts the fallback strategy succeeds. No network access required; works in the Nix sandbox. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 get_rustdoc_version_for_toolchain passes bogus "+" arg when toolchain is empty (Nix path)
The PR adds a new code path where resolve_toolchain() returns an empty string for Nix/non-rustup environments (rust-docs-mcp/src/rustdoc.rs:97). The functions generate_probe_json and run_cargo_rustdoc_json were updated to conditionally skip the +toolchain argument when empty, but get_rustdoc_version_for_toolchain was not. It unconditionally passes format!("+{toolchain}") which becomes just "+" — a bogus argument to rustdoc. While this happens to work in practice because rustdoc's --version flag overrides all other processing (as the PR's own comment at rust-docs-mcp/src/rustdoc.rs:151-153 notes), it causes visibly broken diagnostic output in doctor.rs:82: format!("{version} (selected: {toolchain}") renders as "... (selected: )" with an empty toolchain name.
(Refers to lines 277-290)
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Problem
Two issues when using the MCP server:
Failing cases
Changes
Test plan