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 4 commits 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.
All reported issues were addressed 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.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Two cubic review threads on fix/pr-proof-cargo-shim: 1) resolveCargo step 2 spawned `rustup|mise|asdf which cargo` with no timeout. A hung shim (file-lock or network wait) left the promise pending, and the workflow only failed at its outer timeout. Wrap each runOnce call with a 5s race (configurable via `probeTimeoutMs`) and kill the underlying child in `defaultRunOnce` when a `timeoutMs` option is supplied. On timeout, log via the `log` option and fall through to the next resolver rather than blocking the whole probe. 2) resolve-cargo.test.mjs only exercised rustup (step 2) and the empty / direct-PATH shapes. Add coverage for the mise-which and asdf-which branches, the CARGO_HOME / RUSTUP_HOME / asdf-shims home-inference variants of step 3, and a hung `mise which cargo` that must fall through to a working asdf resolver via the new timeout path. All 11 tests pass locally with `node --test`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Cubic thread 1 also called out that step 4 (`extraSystemPaths`) had no coverage — every existing test passed `extraSystemPaths: []`. Add a targeted test with empty PATH, no HOME/CARGO_HOME/RUSTUP_HOME, and one fixture path in extraSystemPaths; assert it resolves. 12 tests pass locally with `node --test`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
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.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
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.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The previous resolveCargo layered shim detection, version-manager `which
cargo` invocations, and a manual timeout + sentinel race + SIGTERM/SIGKILL
escalation. Every layer was a place for cubic to find a real bug: a
sentinel race leaked timers, `probeTimeoutMs` was unbounded/unnormalized,
SIGTERM-ignoring shims outlived the probe, `cancelledByParent` was
lost, and the timeout test itself hit `node --test`'s harness cancel.
Rewrite as a fail-fast probe. Enumerate candidates in preference order
(CARGO_HOME, ~/.cargo, ~/.rustup/toolchains/*, every cargo on PATH,
documented system fallbacks), then for each one run
`execFileSync(candidate, ['--version'], { timeout: 3000, killSignal:
'SIGKILL' })`. Node's execFileSync `timeout` + SIGKILL is the OS calling
kill(9), so a hung/blocking child cannot out-live the probe. The ground
truth is the `/^cargo \d+\.\d+\.\d+/` regex on stdout — no shim
heuristics needed. Every failure mode (timeout, non-zero exit, unexpected
output) collapses to "try the next candidate." When none work, throw
CargoNotResolvableError carrying the attempts array.
Drops: isShimPath, defaultRunOnce, probeTimeoutMs racing, rustup/mise/asdf
which invocations, HOME/CARGO_HOME/RUSTUP_HOME suffix-stripping, isExec
async helper. Tests now cover: PATH hit, CARGO_HOME hit, rustup toolchain
enumeration, extraSystemPaths fallback, unexpected-output-then-skip,
all-timeout-then-CargoNotResolvableError, and cross-source dedup.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 2 files (changes from recent commits).
You’re at about 95% 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/run.mjs">
<violation number="1" location="tests/relayflows/cases/1602-parentless-worker-inventory/run.mjs:108">
P2: The timeout only kills the process that execFileSync launched directly; any child that a version-manager shim spawned (the exact hung-shim-on-network/lock scenario this PR targets) survives SIGKILL as an orphan and keeps running after resolveCargo returns. The comment's claim that "a hung/blocking child can't out-live the probe" is only true for the direct child, not its descendants. If the orphan holds the cargo home lock prompting the hang, later builds can still block. Spawn with `detached: true` and signal the whole process group (`process.kill(-pid, 'SIGKILL')`) to reap descendants.</violation>
<violation number="2" location="tests/relayflows/cases/1602-parentless-worker-inventory/run.mjs:117">
P3: When a candidate dies from a signal for any reason (not just the timeout), the diagnostic reports it as a timeout: `killed by ${signal} after ${timeoutMs}ms`. A real crash (SIGSEGV) or an external SIGKILL is mislabeled as "after 3000ms", which is misleading for the exact debugging this resolver is meant to support. Use `error.killed` (true only when the timeout killSignal fired) to emit "after Xms" only for actual timeouts.</violation>
</file>
<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:202">
P3: The dedupe test's stated intent (that the identical candidate is not executed twice) is not verified. The test only asserts the returned path equals `cargo`; if the `seen` dedup regressed, the candidate would still be probed and returned with the same value, so the test would pass and would not act as a regression fence for the dedup it claims to cover. Record execution count (e.g., have the fake cargo append to a marker file and assert it ran exactly once) or soften the comment.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| let stdout; | ||
| try { | ||
| stdout = execFileSync(candidate, ['--version'], { | ||
| timeout: timeoutMs, |
There was a problem hiding this comment.
P2: The timeout only kills the process that execFileSync launched directly; any child that a version-manager shim spawned (the exact hung-shim-on-network/lock scenario this PR targets) survives SIGKILL as an orphan and keeps running after resolveCargo returns. The comment's claim that "a hung/blocking child can't out-live the probe" is only true for the direct child, not its descendants. If the orphan holds the cargo home lock prompting the hang, later builds can still block. Spawn with detached: true and signal the whole process group (process.kill(-pid, 'SIGKILL')) to reap descendants.
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 108:
<comment>The timeout only kills the process that execFileSync launched directly; any child that a version-manager shim spawned (the exact hung-shim-on-network/lock scenario this PR targets) survives SIGKILL as an orphan and keeps running after resolveCargo returns. The comment's claim that "a hung/blocking child can't out-live the probe" is only true for the direct child, not its descendants. If the orphan holds the cargo home lock prompting the hang, later builds can still block. Spawn with `detached: true` and signal the whole process group (`process.kill(-pid, 'SIGKILL')`) to reap descendants.</comment>
<file context>
@@ -38,214 +38,96 @@ function proofChildEnvironment() {
- timer = setTimeout(() => resolve(timeoutSentinel), probeTimeoutMs);
- if (typeof timer.unref === 'function') timer.unref();
+ stdout = execFileSync(candidate, ['--version'], {
+ timeout: timeoutMs,
+ killSignal: 'SIGKILL',
+ stdio: ['ignore', 'pipe', 'pipe'],
</file context>
| const signal = error?.signal; | ||
| const status = error?.status; | ||
| const reason = signal | ||
| ? `killed by ${signal} after ${timeoutMs}ms` |
There was a problem hiding this comment.
P3: When a candidate dies from a signal for any reason (not just the timeout), the diagnostic reports it as a timeout: killed by ${signal} after ${timeoutMs}ms. A real crash (SIGSEGV) or an external SIGKILL is mislabeled as "after 3000ms", which is misleading for the exact debugging this resolver is meant to support. Use error.killed (true only when the timeout killSignal fired) to emit "after Xms" only for actual timeouts.
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 117:
<comment>When a candidate dies from a signal for any reason (not just the timeout), the diagnostic reports it as a timeout: `killed by ${signal} after ${timeoutMs}ms`. A real crash (SIGSEGV) or an external SIGKILL is mislabeled as "after 3000ms", which is misleading for the exact debugging this resolver is meant to support. Use `error.killed` (true only when the timeout killSignal fired) to emit "after Xms" only for actual timeouts.</comment>
<file context>
@@ -38,214 +38,96 @@ function proofChildEnvironment() {
+ const signal = error?.signal;
+ const status = error?.status;
+ const reason = signal
+ ? `killed by ${signal} after ${timeoutMs}ms`
+ : `exit ${status ?? 'unknown'}${error?.code ? ` (${error.code})` : ''}`;
+ attempts.push({ path: candidate, reason });
</file context>
| await makeFakeCargo(cargo); | ||
|
|
||
| // CARGO_HOME/bin/cargo, HOME/.cargo/bin/cargo, and the PATH-based candidate | ||
| // all resolve to the same string. Dedup must not double-execute it. |
There was a problem hiding this comment.
P3: The dedupe test's stated intent (that the identical candidate is not executed twice) is not verified. The test only asserts the returned path equals cargo; if the seen dedup regressed, the candidate would still be probed and returned with the same value, so the test would pass and would not act as a regression fence for the dedup it claims to cover. Record execution count (e.g., have the fake cargo append to a marker file and assert it ran exactly once) or soften the comment.
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 202:
<comment>The dedupe test's stated intent (that the identical candidate is not executed twice) is not verified. The test only asserts the returned path equals `cargo`; if the `seen` dedup regressed, the candidate would still be probed and returned with the same value, so the test would pass and would not act as a regression fence for the dedup it claims to cover. Record execution count (e.g., have the fake cargo append to a marker file and assert it ran exactly once) or soften the comment.</comment>
<file context>
@@ -242,133 +110,106 @@ test('resolveCargo step 3 infers home from CARGO_HOME when PATH lacks shims', as
+ await makeFakeCargo(cargo);
+
+ // CARGO_HOME/bin/cargo, HOME/.cargo/bin/cargo, and the PATH-based candidate
+ // all resolve to the same string. Dedup must not double-execute it.
+ const resolved = resolveCargo({
+ 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).