fix(pr-proof): resolve real Cargo executable outside version-manager shims - #1624
fix(pr-proof): resolve real Cargo executable outside version-manager shims#1624kjgbot wants to merge 1 commit into
Conversation
…shims The 1602-parentless-worker-inventory case runner's resolveCargo() rejects rustup's symlink-proxy (basename after realpath becomes "rustup") but had no way to reach the real toolchain cargo in a Cloud sandbox whose PATH contains only shims. When RUSTUP_HOME is unset and no PATH entry ends in /.cargo/bin or /.local/share/mise/shims, the toolchain walk gets no home to work from and the resolver throws. Broaden the resolver: - Detect known shim directories (rustup/mise/asdf /shims/, .volta/bin/) explicitly and reject them alongside the basename check. - Ask the version manager for the toolchain-selected cargo path via `rustup which cargo`, `mise which cargo`, `asdf which cargo`. This is the authoritative resolution when only shims sit on PATH. - Infer likely rustup homes from HOME, CARGO_HOME, RUSTUP_HOME, and asdf shim locations in addition to the existing mise/.cargo hints. - Extend the hardcoded system-install fallbacks with /root/.cargo/bin/ and /home/daytona/.cargo/bin/ so a rustup install without any PATH hint is still found. - On failure, include the ordered list of attempted paths so the next debug pass has ground truth instead of just a rejection message. The resolver now takes an options bag with injectable dependencies so unit tests can drive it with a mocked filesystem and a stubbed version manager. Main() is guarded by the invoked-as-CLI check so importing the module for tests does not execute the probe pipeline. Add resolve-cargo.test.mjs with `node --test` coverage for: - rustup symlink proxy skipped, real toolchain cargo picked up - `rustup which cargo` output preferred over a shim on PATH - shim-only PATH throws with the diagnostic listing - direct non-shim cargo on PATH short-circuits the fallbacks - isShimPath heuristics for rustup, mise, asdf, and volta layouts
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/relayflows/cases/1602-parentless-worker-inventory/resolve-cargo.test.mjs">
<violation number="1" location="tests/relayflows/cases/1602-parentless-worker-inventory/resolve-cargo.test.mjs:99">
P2: The new resolver in run.mjs has several distinct branches that this test file leaves uncovered. Step 2 only mocks `rustup which cargo`; the identical `mise which cargo` and `asdf which cargo` lookups are never exercised. Step 3's home inference from CARGO_HOME, RUSTUP_HOME, and the `/.cargo/bin`/`/.asdf/shims` PATH-suffix is untested (only HOME is exercised), and step 4's extraSystemPaths fallback is not covered at all — every call passes `extraSystemPaths: []`. Since the whole point of this PR is robust resolution across shim managers, add at least one test asserting the extraSystemPaths fallback is hit, and one covering a non-HOME toolchain-home inference, so a regression in those branches cannot slip through.</violation>
</file>
<file name="tests/relayflows/cases/1602-parentless-worker-inventory/run.mjs">
<violation number="1" location="tests/relayflows/cases/1602-parentless-worker-inventory/run.mjs:66">
P2: resolveCargo step 2 probes toolchains by spawning `rustup/mise/asdf which cargo` in defaultRunOnce without a timeout. A hung shim (lock or network wait) leaves the promise unresolved, so resolveCargo never returns, the probe never starts, and the whole case is killed only by the outer cloud timeout. Bound the probe with an AbortSignal timeout so a stuck shim fails fast and falls through to the toolchain walk or diagnostic; also bound the accumulated stdout/stderr (run() already uses boundedAppend for this).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,153 @@ | |||
| // Unit tests for the Cargo resolver used by the 1602 PR-proof case runner. | |||
There was a problem hiding this comment.
P2: The new resolver in run.mjs has several distinct branches that this test file leaves uncovered. Step 2 only mocks rustup which cargo; the identical mise which cargo and asdf which cargo lookups are never exercised. Step 3's home inference from CARGO_HOME, RUSTUP_HOME, and the /.cargo/bin//.asdf/shims PATH-suffix is untested (only HOME is exercised), and step 4's extraSystemPaths fallback is not covered at all — every call passes extraSystemPaths: []. Since the whole point of this PR is robust resolution across shim managers, add at least one test asserting the extraSystemPaths fallback is hit, and one covering a non-HOME toolchain-home inference, so a regression in those branches cannot slip through.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/relayflows/cases/1602-parentless-worker-inventory/resolve-cargo.test.mjs, line 99:
<comment>The new resolver in run.mjs has several distinct branches that this test file leaves uncovered. Step 2 only mocks `rustup which cargo`; the identical `mise which cargo` and `asdf which cargo` lookups are never exercised. Step 3's home inference from CARGO_HOME, RUSTUP_HOME, and the `/.cargo/bin`/`/.asdf/shims` PATH-suffix is untested (only HOME is exercised), and step 4's extraSystemPaths fallback is not covered at all — every call passes `extraSystemPaths: []`. Since the whole point of this PR is robust resolution across shim managers, add at least one test asserting the extraSystemPaths fallback is hit, and one covering a non-HOME toolchain-home inference, so a regression in those branches cannot slip through.</comment>
<file context>
@@ -0,0 +1,153 @@
+ runOnce,
+ extraSystemPaths: [],
+ });
+ assert.equal(resolved, realCargo);
+ } finally {
+ await rm(root, { recursive: true, force: true });
</file context>
|
|
||
| async function defaultRunOnce(command, args) { | ||
| return new Promise((resolve) => { | ||
| const child = spawn(command, args, { |
There was a problem hiding this comment.
P2: resolveCargo step 2 probes toolchains by spawning rustup/mise/asdf which cargo in defaultRunOnce without a timeout. A hung shim (lock or network wait) leaves the promise unresolved, so resolveCargo never returns, the probe never starts, and the whole case is killed only by the outer cloud timeout. Bound the probe with an AbortSignal timeout so a stuck shim fails fast and falls through to the toolchain walk or diagnostic; also bound the accumulated stdout/stderr (run() already uses boundedAppend for this).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/relayflows/cases/1602-parentless-worker-inventory/run.mjs, line 66:
<comment>resolveCargo step 2 probes toolchains by spawning `rustup/mise/asdf which cargo` in defaultRunOnce without a timeout. A hung shim (lock or network wait) leaves the promise unresolved, so resolveCargo never returns, the probe never starts, and the whole case is killed only by the outer cloud timeout. Bound the probe with an AbortSignal timeout so a stuck shim fails fast and falls through to the toolchain walk or diagnostic; also bound the accumulated stdout/stderr (run() already uses boundedAppend for this).</comment>
<file context>
@@ -46,39 +47,158 @@ async function isExecutable(filePath) {
+
+async function defaultRunOnce(command, args) {
+ return new Promise((resolve) => {
+ const child = spawn(command, args, {
+ stdio: ['ignore', 'pipe', 'pipe'],
+ env: process.env,
</file context>
1 participant
Summary
Broadens
resolveCargo()intests/relayflows/cases/1602-parentless-worker-inventory/run.mjsso the PR-proof case runner can find the real toolchain cargo in a Cloud sandbox whose PATH holds only version-manager shims.Also adds
resolve-cargo.test.mjs(node --test) that exercises the failure and success paths.REVISED HYPOTHESIS
The brief pointed at
scripts/pr-proof/run-arm.mjsonorigin/main. That file has no Cargo resolution logic — it is a language-agnostic case-runner harness that runsinput.manifest.runner.command. The bug lives one layer down, in the case-specific runner introduced on this PR's own branch:tests/relayflows/cases/1602-parentless-worker-inventory/run.mjsfix/1602-authoritative-live-workers(this PR is targeted at that branch, notmain, since the buggy code has not merged).Confirmed by fetching the failed run log for
relay#1611GHA run 33174758939, which surfaces the exact error from that file'sresolveCargo():The Cloud sandbox exits after ~2.5s at the Cargo lookup, before any Rust compile could run.
Before
resolveCargo()onfix/1602-authoritative-live-workers:cargo; accepts if the realpath's basename is stillcargo. Rejects rustup's symlink-proxy (basename becomesrustup) but silently accepts hard-linked / scripted shims.homeonly when a PATH entry ends in/.local/share/mise/shimsor/.cargo/bin, then walks<home>/.rustup/toolchains/*/bin/cargo./usr/local/cargo/bin/cargoand/opt/rust/bin/cargo.The Daytona sandbox on this run had neither
RUSTUP_HOMEnor a PATH entry with the expected suffixes, so step 2 was a no-op and step 3 missed the actual install location — leaving no way to reach any real cargo even whenrustup which cargowould have printed one.After
resolveCargo()on this branch:isShimPath) rejects any candidate under a/shims/directory or.volta/bin/, in addition to the existing basename check.rustup which cargo, thenmise which cargo, thenasdf which cargo. This is the authoritative resolution when only shims sit on PATH — it works even ifRUSTUP_HOMEis unset.HOME,CARGO_HOME,RUSTUP_HOME, and any PATH entry ending in/.asdf/shims./root/.cargo/bin/cargoand/home/daytona/.cargo/bin/cargo.attempts: /usr/local/bin/cargo -> /usr/local/bin/rustup (rejected: proxy, not named cargo); rustup: not found on PATH; ...).The resolver takes an options bag with injectable
env,pathEntries,isExecutable,realpath,readdir,runOnce, andextraSystemPathsso tests can drive it hermetically.main()is guarded by the invoked-as-CLI check so importing the module does not execute the probe pipeline.Test
node --test tests/relayflows/cases/1602-parentless-worker-inventory/resolve-cargo.test.mjsFive cases, all passing on this branch:
isShimPathrecognizes rustup / mise / asdf/shims/and.volta/bin/, and does not misclassify real toolchain / mise-installs / asdf-installs cargos.resolveCargoskips a rustup symlink-proxy on PATH and walks the toolchain directory to find the real cargo.resolveCargoprefers the path printed byrustup which cargoover a shim already on PATH.resolveCargothrows a diagnostic listing when nothing resolves (locks the improved error contract).resolveCargoreturns a direct non-shim cargo on PATH when present and does not shell out.The listing / options-bag surface does not exist on the pre-fix code, so the test file itself is the regression fence — reverting
run.mjsbreaks the import.Context
bun-chain-followup-0828finding surfaced in#generalearlier today (~15:55Z), which flagged the shim-resolution hypothesis.Test plan
node --test tests/relayflows/cases/1602-parentless-worker-inventory/resolve-cargo.test.mjslocally and confirms 5/5 pass.fix/1602-authoritative-live-workersand confirmsprove-basegets past the cargo-lookup step (either succeeds or fails downstream with a real toolchain error, not the shim message).