Uh oh!
There was an error while loading. Please reload this page.
Remove Rust tool wrapper shims - #182
Conversation
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR removes repo-local Rust tool wrapper scripts ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/DEVELOPMENT.md (1)
5-18:⚠️ Potential issue | 🟡 MinorTesting section may be inconsistent with the new enforcement.
Line 7 now says the workspace uses
cargo testviasoldr, but the examples immediately below still invokeuv run test/uv run test --full/uv run test -p <crate>. Per the PR description,uv run testfails becausetestis not exposed as a uv console script (the fix was to call the checked-intestBash script directly). Readers who follow these exact examples will hit the same failure.Consider updating the examples to either
bash test ..././test ...(the checked-in script) or the directuv run soldr cargo test ...form that matches the new guard, so the section is self-consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DEVELOPMENT.md` around lines 5 - 18, The Testing section is inconsistent: replace the example invocations of "uv run test", "uv run test --full", and "uv run test -p <crate> -- <test_name>" with commands that match the new enforcement (either the checked-in script or the soldr wrapper); update them to use the checked-in test runner like "./test", "bash test --full", and "./test -p <crate> -- <test_name>" or the direct soldr invocation "uv run soldr cargo test", "uv run soldr cargo test --all --tests" and "uv run soldr cargo test -p <crate> -- <test_name>" so the examples in the Testing section and the described use of cargo via soldr are consistent.
🧹 Nitpick comments (2)
PLAN.md (1)
279-281: LGTM! Phase 3 verification commands now useuv run soldr cargo ....Note: narrative references remain at Line 226 ("
cargo check+cargo testmust pass"), Line 233 ("cargo check + clippy + test --workspace"), and Line 272 ("cargo check + clippy + test"). These are descriptive but optionally could be updated for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PLAN.md` around lines 279 - 281, The verification commands in Phase 3 were updated to use the wrapper `uv run soldr cargo ...` but the narrative descriptions earlier in the document still reference plain `cargo check`, `cargo test`, and combined phrases; update the descriptive text at the referenced narrative spots (the lines mentioning "cargo check + cargo test must pass", "cargo check + clippy + test --workspace", and "cargo check + clippy + test") to consistently mention the wrapper form (`uv run soldr cargo check`, `uv run soldr cargo clippy`, `uv run soldr cargo test`) so the prose matches the commands shown in the Phase 3 diff.ci/hooks/test_tool_guard.py (1)
15-29: Consider adding coverage foruv run <flag> cargo …edge cases.The two tested forms (
uv run cargo testanduv run -- cargo test) cover the common paths, but as noted in thetool_guard.pyreview comment, shapes likeuv run --offline cargo testoruv run --with foo cargo buildcurrently slip past the check. Once the guard is hardened, locking those in with assertions here would prevent future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/hooks/test_tool_guard.py` around lines 15 - 29, Extend test_blocks_uv_run_rust_tool_shim to cover variants where flags appear between "uv run" and the tool: call check_command with inputs like "uv run --offline cargo test", "uv run --with foo cargo build", "uv run -q cargo test" and "uv run -- --offline cargo test" (and any multi-flag combos you think relevant) and assert the result is not None and result[0] == "cargo"; use the existing test helper check_command and the same assertion pattern so these edge cases will fail if tool_guard.py later allows flags to bypass the Rust tool shim.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/hooks/tool_guard.py`:
- Around line 67-81: The uv run handler in tool_guard.py only checks
parts[2]/[3] for the run target which misses cases where uv run is followed by
flags (e.g., --offline, --with <pkg>, --project .) that shift the actual
command; update the block that computes run_target (the code using
seg.startswith(UV_RUN_PREFIX), parts, and run_target) to walk forward over
leading flag tokens (treating tokens that start with "-" and a standalone "--"
sentinel which may consume the next token for flags like "--with <pkg>") until
you hit the first non-flag token and use that as run_target to compare against
RUST_TOOLS, and add regression tests in ci/hooks/test_tool_guard.py that assert
rejection for inputs like "uv run --offline cargo build" and "uv run --with foo
cargo test" to prevent future regressions.
---
Outside diff comments:
In `@docs/DEVELOPMENT.md`:
- Around line 5-18: The Testing section is inconsistent: replace the example
invocations of "uv run test", "uv run test --full", and "uv run test -p <crate>
-- <test_name>" with commands that match the new enforcement (either the
checked-in script or the soldr wrapper); update them to use the checked-in test
runner like "./test", "bash test --full", and "./test -p <crate> -- <test_name>"
or the direct soldr invocation "uv run soldr cargo test", "uv run soldr cargo
test --all --tests" and "uv run soldr cargo test -p <crate> -- <test_name>" so
the examples in the Testing section and the described use of cargo via soldr are
consistent.
---
Nitpick comments:
In `@ci/hooks/test_tool_guard.py`:
- Around line 15-29: Extend test_blocks_uv_run_rust_tool_shim to cover variants
where flags appear between "uv run" and the tool: call check_command with inputs
like "uv run --offline cargo test", "uv run --with foo cargo build", "uv run -q
cargo test" and "uv run -- --offline cargo test" (and any multi-flag combos you
think relevant) and assert the result is not None and result[0] == "cargo"; use
the existing test helper check_command and the same assertion pattern so these
edge cases will fail if tool_guard.py later allows flags to bypass the Rust tool
shim.
In `@PLAN.md`:
- Around line 279-281: The verification commands in Phase 3 were updated to use
the wrapper `uv run soldr cargo ...` but the narrative descriptions earlier in
the document still reference plain `cargo check`, `cargo test`, and combined
phrases; update the descriptive text at the referenced narrative spots (the
lines mentioning "cargo check + cargo test must pass", "cargo check + clippy +
test --workspace", and "cargo check + clippy + test") to consistently mention
the wrapper form (`uv run soldr cargo check`, `uv run soldr cargo clippy`, `uv
run soldr cargo test`) so the prose matches the commands shown in the Phase 3
diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76cd0fa7-3cab-4b2a-8e1d-e1a54864f708
📒 Files selected for processing (34)
CLAUDE.mdCODEX.mdPLAN.md_cargo_rustc_rustfmtci/README.mdci/dev-tools/README.mdci/dev-tools/pyproject.tomlci/env.pyci/hooks/README.mdci/hooks/board_context.pyci/hooks/test_tool_guard.pyci/hooks/tool_guard.pyci/trampoline.pyci/validate_boards.pycrates/fbuild-build/tests/README.mdcrates/fbuild-build/tests/avr_build.rscrates/fbuild-build/tests/esp32_build.rscrates/fbuild-build/tests/flag_escaping_lint.rscrates/fbuild-build/tests/teensy_build.rscrates/fbuild-config/src/bin/README.mdcrates/fbuild-config/src/bin/enrich_boards.rscrates/fbuild-daemon/tests/process_containment.rscrates/fbuild-deploy/src/esp32.rscrates/fbuild-python/src/lib.rsdocs/DESIGN_DECISIONS.mddocs/DEVELOPMENT.mddocs/PERF_WARM_BUILD.mddocs/ROADMAP.mdinstallpython/README.mdtasks/lessons.mdtest
💤 Files with no reviewable changes (3)
- _cargo
- _rustc
- _rustfmt
Uh oh!
There was an error while loading. Please reload this page.
zackees
commented
Apr 22, 2026
Addressed the uv run flag bypass in 05f538e. The hook now walks past leading uv run flags, including value-taking flags like --with and --project, before checking the command target. Added regression coverage for --offline, --with, --isolated, --project, -q, and -- paths. Also updated stale docs/plan examples away from uv run test and bare cargo verification. Verified with: uv run python ci/hooks/test_tool_guard.py; git diff --check. |
Uh oh!
There was an error while loading. Please reload this page.
Summary
_cargo,_rustc, and_rustfmtshell wrapperscargo/rustc/rustfmt/clippy-driverdev-tool console shims so Rust tooling goes throughsoldrdirectlyuv run cargo-style Rust invocations and requiresoldr ...oruv run soldr ...Closes#181.
Testing
uv run python ci/hooks/test_tool_guard.pyuv run soldr cargo fmt --all --checkuv run soldr cargo check --workspace --all-targetsbash test -p fbuild-coreNote:
uv run test -p fbuild-corewas attempted first, but this repo does not currently exposetestas a uv console script in this environment, so it failed before invoking the checked-in test wrapper. The checked-intestscript was then validated directly with Bash.Summary by CodeRabbit
Documentation
soldrfor Rust tool invocation instead of directuv run cargo.Chores
soldroruv run soldr.