Skip to content

fix(desktop): prefer packaged sidecars in release builds#2655

Open
amanning3390 wants to merge 2 commits into
block:mainfrom
amanning3390:fix/release-sidecar-resolution
Open

fix(desktop): prefer packaged sidecars in release builds#2655
amanning3390 wants to merge 2 commits into
block:mainfrom
amanning3390:fix/release-sidecar-resolution

Conversation

@amanning3390

Copy link
Copy Markdown

Problem

In release builds, the desktop app's sidecar discovery (command_search_dirs) can find stale debug workspace artifacts instead of the bundled target-suffixed sidecars shipped beside the executable. This causes runtime failures when debug and release binaries are incompatible.

What This Changes

Refactors command_search_dirs into command_search_dirs_for_profile with explicit profile awareness:

  • Release builds prefer bundled sidecars beside the app executable first, then fall back to workspace artifacts
  • Debug builds continue preferring fresh workspace debug artifacts (unchanged behavior)
  • The Justfile release recipe now calls scripts/bundle-sidecars.sh before Tauri packaging

Scope

  • desktop/src-tauri/src/managed_agents/discovery.rscommand_search_dirs refactored
  • desktop/src-tauri/src/managed_agents/discovery/tests.rs — release/debug precedence tests
  • Justfile — release recipe uses canonical sidecar bundling

Verification

cargo test --manifest-path desktop/src-tauri/Cargo.toml managed_agents::discovery  →  77 passed (1 pre-existing)
cargo clippy --manifest-path desktop/src-tauri/Cargo.toml --lib -- -D warnings  →  clean

@amanning3390
amanning3390 requested a review from a team as a code owner July 24, 2026 00:42
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Bartok9

Bartok9 commented Jul 24, 2026

Copy link
Copy Markdown

Hey @amanning3390 — really glad you're pushing Buzz packaging quality here. Direction looks right: pure command_search_dirs_for_profile, release prefers beside-exe, and desktop-release-build using real cargo build + scripts/bundle-sidecars.sh instead of empty touch stubs is a solid fix.

Two intentional things that will help this land:

1) DCO is red (merge blocker)

Buzz requires a Signed-off-by: trailer. Quick fix for a single commit:

git commit --amend -s --no-edit
git push --force-with-lease

(or git rebase --signoff origin/main if you need to rewrite more than one).

2) Release still falls back into workspace target/ after the bundled dir

Even in release profile the search order is still:
exe-parent → workspace release/debug → cwd release/debug.

So if a packaged sidecar is missing or non-executable, a developer machine (or a mis-bundled release run from a checkout) can still silently pick a stale workspace binary — the same class of failure this PR is aiming to kill. The new tests lock order, not “bundled wins when both exist and when bundled is broken.”

Suggested minimal hardening (pick one):

  • When !debug_profile and current_exe_parent is present, do not append workspace/cwd target dirs (devs who need workspace sidecars can set something like BUZZ_ALLOW_WORKSPACE_SIDECARS=1), or
  • Keep the fallback, but add one resolution test with temp files proving: both dirs have the binary → bundled path wins.

scripts/bundle-sidecars.sh already mkdir -ps and validates sources — nice; the Justfile crate list should stay mirrored with that script’s SIDECARS / cargo -p list.

Happy to open a tiny follow-up PR for (2) if you'd rather keep shipping — totally fine if you want to own it. Excited to see Hermes + Buzz land cleanly; thank you for the work.

Co-authored-by: amanning3390 <adam.manning@pro-serveinc.com>
Signed-off-by: amanning3390 <adam.manning@pro-serveinc.com>
The existing search-order tests assert directory ordering with synthetic
paths, so they cannot catch a regression in the behavior this PR is
actually about: which binary gets picked when real files exist in more
than one candidate directory.

Adds two resolution tests that place real executables on disk and apply
the same first-executable-wins rule as resolve_workspace_command:

- both bundled and workspace dirs contain buzz-acp -> the packaged
  sidecar beside the app executable wins in release profile.
- the packaged sidecar exists but is not executable -> resolution falls
  through to the workspace artifact.

The second case documents the fallback contract explicitly rather than
leaving it incidental. The workspace fallback is deliberately retained:
removing it would change behavior for developers who run a release
profile from a checkout, which is outside this PR's scope.

Unix-gated, matching the existing permission-sensitive test in this file.

Co-authored-by: amanning3390 <adam.manning@pro-serveinc.com>
Signed-off-by: amanning3390 <adam.manning@pro-serveinc.com>
@amanning3390

Copy link
Copy Markdown
Author

Thanks @Bartok9 — both items addressed.

1) DCO — green

Signoff added via --amend per the DCO bot's instructions, plus a Co-authored-by trailer. DCO Check now passes on this PR and on #2656. That was the only force-push; the hardening below landed as an additive commit so the review diff stays readable, per CONTRIBUTING.

2) Release fallback — proof added, fallback deliberately kept

You identified the real hole precisely: the existing tests assert directory ordering with synthetic paths, so nothing verified which binary actually gets picked when real files exist in more than one candidate dir. Order tests can't catch that regression class.

I took your option 2 (keep the fallback, add the proof) rather than option 1, and I want to be explicit about why: dropping the workspace/cwd target dirs whenever !debug_profile && current_exe_parent.is_some() would change behavior for developers who run a release profile from a checkout. That's a real workflow, and gating it behind a new BUZZ_ALLOW_WORKSPACE_SIDECARS escape hatch is a behavior change plus new config surface — beyond what this PR set out to fix. If you'd rather have the stricter precedence, I'm happy to do it as a follow-up where it can be discussed on its own merits.

New commit: test(desktop): prove bundled sidecars win over workspace artifacts

Two resolution tests that place real executables on disk and apply the same first-executable-wins rule as resolve_workspace_command:

  • release_resolution_picks_bundled_sidecar_over_workspace_artifactbuzz-acp exists and is executable in both the bundled dir and workspace/target/release; asserts the packaged sidecar beside the app executable wins in release profile.
  • release_resolution_falls_back_when_bundled_sidecar_is_not_executable — the packaged sidecar exists but is mode 0644; asserts resolution falls through to the workspace artifact.

That second test is the one that directly answers your "bundled wins when both exist and when bundled is broken" point. It makes the fallback contract explicit and visible instead of incidental, so if someone later tightens the precedence, this test is the thing that tells them exactly what behavior they're changing.

Both are #[cfg(unix)]-gated and use the temp-dir + uuid + PermissionsExt pattern already established by explicit_path_resolution_ignores_non_executable_files in the same file.

cargo test --manifest-path desktop/src-tauri/Cargo.toml release_resolution
test result: ok. 2 passed; 0 failed

cargo fmt --all -- --check → clean.

On the Justfile ↔ bundle-sidecars.sh mirror

Agreed, and noted in the PR body. The crate list in the release recipe and SIDECARS in scripts/bundle-sidecars.sh have to stay in sync; the script's own source validation is what catches a drift today.

One transparency note on the test suite

Running the full managed_agents::discovery suite shows one unrelated failure, refresh_login_shell_path_clears_cache. I checked whether I caused it:

  • with my changes, run alone → passes
  • on the clean baseline (my changes stashed), full suite → fails

So it's a pre-existing test-isolation issue around the shared login-shell PATH cache under parallel execution, not something this PR introduces. Flagging it rather than quietly leaving a red test unexplained — happy to open a separate issue for it if that's useful.

Thanks for the careful review on both PRs.

@Bartok9

Bartok9 commented Jul 24, 2026

Copy link
Copy Markdown

Both addressed cleanly — thank you.

DCO green and the hardening landing as an additive commit (keeping the review diff readable) is the right shape. On the fallback: taking option 2 and keeping the workspace/cwd fallback is the correct conservative call — release-from-a-checkout is a real workflow, and gating it behind a new env var would be a behavior change plus config surface beyond this PR's scope. The two resolution tests with real executables on disk are exactly what was missing: release_resolution_picks_bundled_sidecar_over_workspace_artifact proves bundled wins when both exist, and ..._falls_back_when_bundled_sidecar_is_not_executable makes the "broken bundled → fall through" contract explicit and visible rather than incidental. That's the regression class the order-only tests couldn't catch.

Good transparency on the pre-existing refresh_login_shell_path_clears_cache isolation flake (passes alone, fails on clean baseline too) — agreed it's unrelated; a separate issue for the shared login-shell PATH cache under parallel exec would be useful but shouldn't block here.

Ready from my side. Deferring to Buzz maintainers on merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants