fix(daemon): forward the caller CLI's PATH per request for bare-name tool spawns - #1231
Closed
zackees wants to merge 2 commits into
Closed
fix(daemon): forward the caller CLI's PATH per request for bare-name tool spawns#1231zackees wants to merge 2 commits into
zackees wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (44)
📝 WalkthroughWalkthroughThe change captures the CLI caller’s ChangesCaller PATH propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Daemon
participant BuildEngine
participant ToolProcess
CLI->>Daemon: send caller_path
Daemon->>BuildEngine: create BuildParams
BuildEngine->>ToolProcess: resolve bare tool with caller PATH
ToolProcess-->>BuildEngine: execute tool
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/fbuild-daemon/src/models.rs (1)
688-710: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest the request serialization boundary.
The
build_request_caller_path_round_tripsanddeploy_request_caller_path_round_tripstests only deserialize JSON. They do not verify that the CLI request types emitcaller_pathor omit it when it isNone.Add assertions for both
Some(...)andNoneusing the client request types. Rename the tests if they remain deserialization-only.As per path instructions, tests must define the supported specification and cover caller-PATH propagation and backward-compatible request handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-daemon/src/models.rs` around lines 688 - 710, Extend the request tests around BuildRequest and DeployRequest to serialize client request instances and assert caller_path is emitted for Some(...) and omitted for None, while retaining deserialization coverage for backward-compatible payloads. Rename the existing round-trip tests if they only validate deserialization, and cover caller-PATH propagation for both request types.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-build-esp/src/esp32/esp32_linker.rs`:
- Around line 480-491: Update the ESP32 BIN cache flow around can_reuse_bin and
BinArtifactCache to account for caller_path or the effective bare-name esptool
executable selected by bare_name_path_overlay. Include a stable executable
identity or caller-PATH hash in the cache key, bump the cache version, and add a
regression test proving different PATH resolutions do not reuse the same
firmware.bin.
In `@crates/fbuild-core/src/subprocess.rs`:
- Around line 1029-1089: Add a subprocess-level test near bare_name_path_overlay
that creates a temporary executable in tempfile::TempDir, invokes it through
run_command by bare name with a caller PATH overlay, and asserts the
executable’s output. In the same test, invoke the executable via its absolute
path and verify the overlay is not applied, testing observable command behavior
rather than only bare_name_path_overlay’s return value.
In `@crates/fbuild-daemon/src/handlers/operations/deploy.rs`:
- Line 255: Propagate BuildParams.caller_path through deployer
selection/construction and the Deployer::deploy API into deploy-time subprocess
execution, including helper spawns that currently pass None for the environment
overlay. Ensure both normal deployment and the skip_build path apply
caller_path, and add coverage verifying the environment is propagated in each
case.
---
Nitpick comments:
In `@crates/fbuild-daemon/src/models.rs`:
- Around line 688-710: Extend the request tests around BuildRequest and
DeployRequest to serialize client request instances and assert caller_path is
emitted for Some(...) and omitted for None, while retaining deserialization
coverage for backward-compatible payloads. Rename the existing round-trip tests
if they only validate deserialization, and cover caller-PATH propagation for
both request types.
🪄 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 Plus
Run ID: 59f28d34-56ea-468c-9006-2ea64fd41b39
📒 Files selected for processing (43)
crates/fbuild-build-arm/src/generic_arm/arm_linker.rscrates/fbuild-build-arm/src/nrf52/nrf52_linker.rscrates/fbuild-build-arm/src/renesas/renesas_linker.rscrates/fbuild-build-arm/src/sam/sam_linker.rscrates/fbuild-build-arm/src/silabs/silabs_linker.rscrates/fbuild-build-arm/src/teensy/teensy_linker.rscrates/fbuild-build-engine/src/lib.rscrates/fbuild-build-engine/src/linker.rscrates/fbuild-build-engine/src/pipeline/context.rscrates/fbuild-build-engine/src/script_runtime.rscrates/fbuild-build-engine/src/script_runtime_tests.rscrates/fbuild-build-esp/src/esp32/esp32_linker.rscrates/fbuild-build-esp/src/esp32/orchestrator/boot_artifacts.rscrates/fbuild-build-esp/src/esp32/orchestrator/build.rscrates/fbuild-build-esp/src/esp8266/esp8266_linker.rscrates/fbuild-build-esp/src/esp8266/orchestrator.rscrates/fbuild-build-mcu/src/avr/avr_linker.rscrates/fbuild-build-mcu/src/ch32v/ch32v_linker.rscrates/fbuild-build-mcu/src/ch32v/orchestrator.rscrates/fbuild-build/src/compile_many.rscrates/fbuild-build/tests/avr_build.rscrates/fbuild-build/tests/clangd_check_parity.rscrates/fbuild-build/tests/eh_frame_strip_esp32.rscrates/fbuild-build/tests/esp32_build.rscrates/fbuild-build/tests/lite_scons_acceptance.rscrates/fbuild-build/tests/nxplpc_build_flags.rscrates/fbuild-build/tests/nxplpc_core_compile_commands.rscrates/fbuild-build/tests/stm32_acceptance.rscrates/fbuild-build/tests/teensy30_acceptance.rscrates/fbuild-build/tests/teensy_build.rscrates/fbuild-build/tests/teensylc_acceptance.rscrates/fbuild-cli/src/cli/build.rscrates/fbuild-cli/src/cli/clean.rscrates/fbuild-cli/src/cli/deploy.rscrates/fbuild-cli/src/daemon_client.rscrates/fbuild-cli/src/daemon_client/types.rscrates/fbuild-cli/src/mcp/tools.rscrates/fbuild-core/src/subprocess.rscrates/fbuild-daemon/src/handlers/emulator/select.rscrates/fbuild-daemon/src/handlers/emulator/tests_process.rscrates/fbuild-daemon/src/handlers/operations/build.rscrates/fbuild-daemon/src/handlers/operations/deploy.rscrates/fbuild-daemon/src/models.rs
zackees
added a commit
that referenced
this pull request
Aug 2, 2026
…): end-to-end bare-name overlay resolution CodeRabbit follow-ups on #1231: * `BinArtifactCache` gains a serde-defaulted `esptool_fingerprint` field: empty for a provisioned absolute-path esptool (resolution cannot drift, so pre-existing cache records stay valid and caching behaves exactly as before), a short SHA-256 of the caller's PATH for bare-name spawns. Two requests whose caller PATHs differ can resolve different esptool binaries, so they must never share a cached firmware.bin. The shared BUILD_FINGERPRINT_VERSION constant is left untouched — bumping it would invalidate every unrelated cache, and the serde default already yields the correct match semantics. * New subprocess test proves `run_command` actually resolves a bare executable from an overlay PATH (staged uniquely named probe in a TempDir), that absolute-path spawns work without an overlay, and that the bare name fails without the overlay — i.e. resolution came from the overlay, not ambient PATH. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Aug 2, 2026
…): end-to-end bare-name overlay resolution CodeRabbit follow-ups on #1231: * `BinArtifactCache` gains a serde-defaulted `esptool_fingerprint` field: empty for a provisioned absolute-path esptool (resolution cannot drift, so pre-existing cache records stay valid and caching behaves exactly as before), a short SHA-256 of the caller's PATH for bare-name spawns. Two requests whose caller PATHs differ can resolve different esptool binaries, so they must never share a cached firmware.bin. The shared BUILD_FINGERPRINT_VERSION constant is left untouched — bumping it would invalidate every unrelated cache, and the serde default already yields the correct match semantics. * New subprocess test proves `run_command` actually resolves a bare executable from an overlay PATH (staged uniquely named probe in a TempDir), that absolute-path spawns work without an overlay, and that the bare name fails without the overlay — i.e. resolution came from the overlay, not ambient PATH. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zackees
force-pushed
the
fix/1219-request-path
branch
from
August 2, 2026 00:50
fe39f44 to
fa07577
Compare
Member
Author
|
@coderabbitai review — both actionable findings are fixed in fe39f44 (BIN-cache esptool fingerprint + end-to-end overlay resolution test) and the deploy-side threading is tracked in #1234; branch rebased onto current main. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- commit metadata and changed files ---'
git show --no-ext-diff --format=fuller --stat fe39f44c
git show --no-ext-diff --format= --name-only fe39f44c
echo '--- relevant changed code ---'
git show --no-ext-diff --format= --unified=80 fe39f44c -- \
crates/fbuild-build-esp/src/esp32/esp32_linker.rs \
crates/fbuild-build-esp/src/esp32/esp32_linker_tests.rs \
crates/fbuild-core/src/subprocess.rs \
crates/fbuild-core/src/subprocess_tests.rs || true
echo '--- esptool cache key/fingerprint references ---'
rg -n -C 4 -i 'esptool|fingerprint|cache.*bin|bin.*cache' crates/fbuild-build-esp crates/fbuild-core
echo '--- caller PATH overlay tests and call sites ---'
rg -n -C 4 'bare_name_path_overlay|caller_path|PATH overlay|overlay' \
crates/fbuild-build-esp crates/fbuild-core crates/fbuild-daemonLength of output: 1387 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh issue view 1234 \
--repo FastLED/fbuild \
--json number,title,state,body,url,author,assignees \
--jq '{number, title, state, url, author: .author.login, assignees: [.assignees[].login], body}'Length of output: 2668 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- current revision and PR base candidates ---'
git status --short --branch
git rev-parse --short HEAD
git branch --all --no-color
git remote -v
echo '--- recent current-branch commits ---'
git log --oneline -n 30
echo '--- changed files relative to likely base refs ---'
for base in origin/main main origin/master master; do
if git rev-parse --verify --quiet "$base" >/dev/null; then
echo "### $base"
merge_base="$(git merge-base HEAD "$base")"
git diff --name-status "$merge_base"..HEAD
fi
done
echo '--- candidate source and test files ---'
fd -t f -E target -E .git | rg -i '(esp32.*linker|linker.*esp32|esptool|subprocess).*\.rs$'Length of output: 8435 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- exact follow-up commit patch ---'
git show --no-ext-diff --format=fuller --unified=100 fa07577 -- \
crates/fbuild-build-engine/src/build_fingerprint/mod.rs \
crates/fbuild-build-esp/src/esp32/esp32_linker.rs \
crates/fbuild-core/src/subprocess.rs
echo '--- structure of relevant files ---'
ast-grep outline crates/fbuild-build-engine/src/build_fingerprint/mod.rs --items all
ast-grep outline crates/fbuild-build-esp/src/esp32/esp32_linker.rs --items all
ast-grep outline crates/fbuild-core/src/subprocess.rs --items all
echo '--- focused symbols and assertions ---'
rg -n -C 8 -i \
'caller_path|fingerprint|bin.*reuse|reuse.*bin|esptool|bare_name_path_overlay|overlay.*resolv|resolv.*overlay' \
crates/fbuild-build-engine/src/build_fingerprint/mod.rs \
crates/fbuild-build-esp/src/esp32/esp32_linker.rs \
crates/fbuild-core/src/subprocess.rsLength of output: 125283
No further actionable findings from this follow-up review. You are interacting with an AI system. |
…tool spawns The daemon resolved bare-name tool spawns (esptool, python, objcopy) against whichever PATH it inherited at spawn time — up to hours stale when the user's venv or toolchain PATH changed since. Fixes: * Build/Deploy requests now carry an optional `caller_path` snapshot of the calling CLI's PATH (serde-defaulted for backward compatibility; older CLIs simply send nothing). The CLI populates it from the same case-insensitive PATH lookup used at daemon spawn (`launcher_path`). * `compute_env` (fbuild-core subprocess) now removes case-variant keys before applying an overlay on Windows: an overlay "PATH" previously coexisted with the inherited "Path" and lost the last-insert-wins race inside the child's env map, silently discarding the override. * New `bare_name_path_overlay` helper builds a PATH overlay only for bare-name programs; absolute/relative tool paths are unaffected. * The five known bare-name spawn sites now apply the overlay: esp32 esptool elf2image, esp8266 esptool elf2image, `find_python` probes (and the extra_scripts harness spawn), esp32 boot-artifact esptool/python spawns, and `LinkerBase::objcopy_firmware`. * The daemon build handler logs one line when the caller's PATH differs from its own (lengths only, no PATH values dumped). Closes #1219 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…): end-to-end bare-name overlay resolution CodeRabbit follow-ups on #1231: * `BinArtifactCache` gains a serde-defaulted `esptool_fingerprint` field: empty for a provisioned absolute-path esptool (resolution cannot drift, so pre-existing cache records stay valid and caching behaves exactly as before), a short SHA-256 of the caller's PATH for bare-name spawns. Two requests whose caller PATHs differ can resolve different esptool binaries, so they must never share a cached firmware.bin. The shared BUILD_FINGERPRINT_VERSION constant is left untouched — bumping it would invalidate every unrelated cache, and the serde default already yields the correct match semantics. * New subprocess test proves `run_command` actually resolves a bare executable from an overlay PATH (staged uniquely named probe in a TempDir), that absolute-path spawns work without an overlay, and that the bare name fails without the overlay — i.e. resolution came from the overlay, not ambient PATH. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zackees
force-pushed
the
fix/1219-request-path
branch
from
August 2, 2026 02:04
c0cbae9 to
ba51e5d
Compare
Member
Author
|
Superseded by #1236, which merged the same per-request caller-PATH design (including the equivalent case-insensitive PATH overlay fix in compute_env) while this PR was in CI. One piece of this PR is NOT in #1236 and is being re-landed separately: the BIN-artifact-cache esptool fingerprint (CodeRabbit's confirmed cache-soundness finding here — on current main, can_reuse_bin can hand a firmware.bin produced by a differently-resolved esptool to a request with a different caller PATH). Follow-up issue incoming. |
Member
Author
|
Superseded: #1219 was closed by PR #1236 (merged as e85fb71), which landed the per-request caller-PATH threading for the build path with the same request->BuildParams->spawn-site design. This branch now hard-conflicts with main across the same files. The deploy-side scope lives in #1234, which will be implemented against main's merged shapes and will adopt this PR's bare_name_path_overlay idea (absolute-path no-op helper) where it fits. Closing to avoid two divergent implementations of the same issue. |
This was referenced Aug 2, 2026
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.
Closes #1219
What
The daemon resolved bare-name tool spawns (
esptool,python/python3/py -3,objcopy) against whichever PATH it inherited at spawn — and a daemon lives up to 12 h, so byte-identical builds resolved different toolchains depending only on which shell started the daemon and when. This forwards the calling CLI's PATH per request and uses it for bare-name resolution at every spawn site the issue enumerated.How
caller_path: Option<String>on the CLI and daemonBuildRequest/DeployRequest(serde-defaulted for backward compatibility, skip-serialized when absent), captured via the samelauncher_pathlogic already used at daemon spawn, and carried intoBuildParamsnext to the existingpio_envper-request forwarder.compute_envPATH-override fix (fbuild-core): the Windows branch merged overlays into a case-sensitive map seeded from the daemon env, so an overlay("PATH", …)coexisted with an inherited"Path"— and since std's Command env map is case-insensitive last-insert-wins with BTreeMap yielding"PATH"before"Path", the daemon's value silently won. Overlay keys now evict case-insensitive matches first. Without this, the whole feature would be a no-op on Windows.bare_name_path_overlayhelper: applies the caller PATH only when the program is a bare name — provisioned absolute tool paths are untouched.esp32_linker.rs), esp8266 esptool (esp8266_linker.rs),find_python()probes and the extra-scripts harness spawn (script_runtime.rs), boot-artifactspython/esptool (boot_artifacts.rs), andLinkerBase::objcopy_firmware(threaded throughCh32vLinker/Esp8266Linker; linkers with absolute toolchain paths passNone).tracing::info!line (lengths only, not values) when the caller PATH differs from the daemon's.RED → GREEN
RED captured before the fix (deterministic test seeding a
"Path"-cased inherited key):GREEN after: that test plus
path_overlay_replaces_inherited_path_case_insensitivelyandbare_name_path_overlay_applies_only_to_bare_namesall pass.Tests
soldr cargo test -p fbuild-core -p fbuild-build-engine -p fbuild-daemon -p fbuild-cli -p fbuild-build-esp -p fbuild-build-mcu— all green (25 test binaries, 0 failures), including new serde back-compat tests (request JSON withoutcaller_pathstill deserializes).soldr cargo clippy --workspace --all-targets -- -D warningsclean;cargo fmtapplied.3e9dc5cc.This closes the mechanism behind the #1217 CI outage class: a venv-installed tool is now visible to builds run from that venv, regardless of which shell spawned the daemon.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests