Isolate worktree Rust builds and share the agent runtime - #361
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 cold-build regression; details and the bounded fix are inline.
Reviewed head d212cf523982811e631a7c85c8616b13359b5275 against base 1f71ee94f2dadb743a6529ded436b266e3c93045. Verified 140/140 Node integration tests, a real pinned-runtime cold build, restore into a fresh sibling worktree with all five hashes matching, and isolated tool startup/protocol smoke checks on macOS arm64. The real-Cargo regression probe passes with the base script, fails at this head, and passes with the proposed explicit-target change.
Remaining gates: the two-worktree native desktop launch is still deferred. Hosted Rust/tool integration passed. CI remains red: JavaScript failed in unchanged src/features/relay/store-discovery.test.ts, and WebKit shard 3 failed in unchanged thread-unread.spec.mjs navigation (Search Buzz stayed open). No causal link from either failure to this diff was established. Windows validation was skipped. No approval submitted.
| ); | ||
| env.CARGO_TARGET_DIR = join(stage, "target"); | ||
| await run(join(root, "bin/cargo"), buildArgs, false, source); | ||
| await publish(join(stage, "target/release"), destination); |
There was a problem hiding this comment.
[P2] Make the runtime target explicit before reading its output directory
A user Cargo config containing [build] target = "aarch64-apple-darwin" (even the machine’s native host triple) is still read through the newly shared CARGO_HOME. Scrubbing CARGO_BUILD_TARGET does not remove that file setting. Cargo successfully builds into stage/target/<triple>/release, then this unconditional stage/target/release lookup fails with ENOENT. On a cold cache, both just desktop and packaged resource preparation abort after compilation; the prior cargo install --root path successfully staged the same configuration.
Reproduced with real pinned Cargo 1.97.1, an isolated Cargo home, and a tiny local Git workspace exposing the same five package/bin names: base script exits 0, unchanged head script exits 1 at this copy, and an experimental explicit-target fix exits 0. The existing fake Cargo always writes directly to release, so it cannot catch this.
Pass --target with the already resolved host target and publish from join(stage, "target", target, "release") (or equivalently normalize the effective target/output contract). Add coverage for a Cargo-home build.target setting, including one equal to the native host. This is a host-runtime build, not a request to add cross-compilation support.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 The worktree isolation and cross-worktree runtime reuse make sense. Requesting changes for the interrupted cold-build regression below: preserve incremental compilation progress rather than placing the release target in a disposable stage. Normal cold-build and sibling-restore checks passed; native desktop launch remains untested.
| false, | ||
| source, | ||
| ); | ||
| env.CARGO_TARGET_DIR = join(stage, "target"); |
There was a problem hiding this comment.
🤖 [P2] Preserve the build target across interrupted cold builds. SIGINT/SIGTERM exits Node without executing the cleanup finally, leaving this temporary release target behind. Interrupting the script's process group after 45 seconds left 760 MB in $TMPDIR; every retry creates a new stage and recompiles from scratch. Previously, target/agent-runtime-build retained incremental progress. Keep a persistent checkout-local or per-clone CARGO_TARGET_DIR, relying on Cargo's build lock, while keeping the source checkout outside the app worktree. Add an interrupt/retry regression check; signal cleanup alone would fix the leak but not lost progress.
| await readFile(join(root, "runtime/agent-runtime.json"), "utf8"), | ||
| ); | ||
| // Build inputs come only from the pin and toolchain, so cached bundles are | ||
| // interchangeable: drop injected credentials and per-shell compiler overrides. |
There was a problem hiding this comment.
🤖 [P3] Narrow the cache input-isolation claims. The fetched source's Cargo config and user-level Cargo config still apply, while user release settings and native compiler inputs such as CC/CFLAGS are not fully represented in the cache key. Please qualify the interchangeable-inputs claim here and the later comment that no checkout's Cargo config reaches the build. The per-user/per-clone cache is not a guarantee that all effective build inputs match.
A repo-level .cargo/config.toml pins target-dir to each checkout, so a user-level shared target-dir can no longer mix app binaries and resources between worktrees. Hermit now uses the shared ~/.cargo for downloads. The agent runtime builds all five tools in one cargo build from a shallow fetch of the pinned revision, in a temporary target that is removed afterwards, and caches the verified bundle under the Git common directory keyed by pin, tool list, build arguments and rustc -vV. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…nputs Verified cache entries are never removed, since other worktrees may be copying them; a finished build only fills a missing entry or replaces a corrupt one by rename, and restores that do not match the cached manifest rebuild. desktop and desktop-bundle drop an inherited CARGO_TARGET_DIR or CARGO_BUILD_TARGET_DIR so the repo config always applies. The runtime is built outside the checkout with per-shell compiler overrides scrubbed, so every entry for a key is built from the same inputs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Cache entries are only ever published whole by rename, so an existing entry that fails verification is removed before the build and the finished build publishes with one rename, keeping any entry a concurrent build published first. A missing key is never removed. A restore that fails to copy rebuilds. This replaces the .old repair path and the post-restore manifest comparison. The desktop launchers again honor an explicitly exported CARGO_TARGET_DIR; the repo .cargo/config.toml still overrides a user-level target-dir, and the runtime build still scrubs both. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: drone2 <538af22ae6ba32ed6e86938bbbd536bd128ec0520127cb0f8ecc4661579d51ec@buzz.block.builderlab.xyz> Signed-off-by: Matt Toohey <contact@matttoohey.com>
A key that was missing when verification ran could be published by a concurrent build before the existence check, and then removed. Checking existence first means only a whole, pre-existing entry that fails verification is removed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: drone2 <538af22ae6ba32ed6e86938bbbd536bd128ec0520127cb0f8ecc4661579d51ec@buzz.block.builderlab.xyz> Signed-off-by: Matt Toohey <contact@matttoohey.com>
d212cf5 to
b6cf60b
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6cf60bde4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| env.CARGO_TARGET_DIR = join(stage, "target"); | ||
| await run(join(root, "bin/cargo"), buildArgs, false, source); | ||
| await publish(join(stage, "target/release"), destination); |
There was a problem hiding this comment.
Locate binaries under Cargo's configured target
When ~/.cargo/config.toml defines [build] target, this detached source checkout still reads that configuration through the shared CARGO_HOME, so Cargo writes binaries under target/<triple>/release; the hard-coded target/release source then raises ENOENT after compilation and blocks just desktop. Force the host target explicitly and use its corresponding output directory, or obtain the artifact paths from Cargo.
Useful? React with 👍 / 👎.
| } else if (existing) { | ||
| // Seen before verifying: entries appear whole by rename, so this one was | ||
| // edited. A key missing at check time is never removed, since a concurrent | ||
| // build may publish it. | ||
| await rm(cache, { recursive: true, force: true }); |
There was a problem hiding this comment.
Delete only the cache entry that failed verification
When two worktrees repair a corrupt cache concurrently, existing only proves that some entry occupied the path before verification. One process can replace that entry with a valid rebuilt directory before the other reaches this unconditional rm, causing the second process to delete the fresh cache; if its subsequent build fails or is interrupted, later worktrees rebuild unnecessarily. Confirm that the path still identifies the verified corrupt entry, or atomically claim that directory before deleting it.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,5 @@ | |||
| # Each checkout builds into its own target/, even when a user-level Cargo config | |||
There was a problem hiding this comment.
The reviewed commit message contains no Signed-off-by: trailer, so it violates the repository's per-commit DCO requirement and will not pass the hosted DCO check. Add a sign-off using the actual author's verified Git identity before submission.
AGENTS.md reference: AGENTS.md:L151-L156
Useful? React with 👍 / 👎.
A user-level Cargo config that sets build.target nested the output under target/<triple>/release, so publishing from target/release failed after a cold build. The build now passes --target for the resolved host and reads from that triple's directory. The release target lived in the disposable stage, so an interrupted cold build leaked it into TMPDIR and every retry started from scratch. It now persists at target/agent-runtime-build in the checkout, relying on Cargo's lock; only the fetched source stays in a temporary stage, which SIGINT and SIGTERM now remove. The cache-input comments no longer claim that user-level Cargo config and native compiler inputs are keyed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Summary
A fresh worktree's
just desktopcompiled Rust several times over. There were two causes:target-dircollisions. A user-level~/.cargo/config.tomltarget-dirmade every worktree share one target, so the app binaries and Tauri resources of different worktrees overwrote each other and forced rebuilds (and could breakresource_dir).build-agent-runtime.mjsran a separatecargo installfor each of the five pinned tools in every new checkout.Changes:
.cargo/config.tomlpinstarget-dir = "target", so each checkout builds into its owntarget/whatever the user config says. An explicitly exportedCARGO_TARGET_DIRis still honored.bin/hermit.hclsharesCARGO_HOME=~/.cargo, so there's no per-worktree registry or git download.cargo buildof all five tools from a shallow fetch of the pin. It runs in a temporary target with per-shell compiler overrides (RUSTFLAGS,RUSTC_*,CARGO_PROFILE_*, target-dir vars) scrubbed. The verified bundle is cached under the Git common dir atbuzz-agent-runtime/<hash of pin + tools + build args + rustc -vV>, so sibling worktrees of one clone restore it instead of rebuilding.docs/contributing.mdanddocs/agent-control.mddescribe the cache.Validation
Full Node integration suite,
PATH=$PWD/bin:$PATH node --test tests/integration/*.test.mjs: 140/140 atd212cf52(rebased onto main1f71ee94), macOS arm64, Hermit node.New tests in
tests/integration/agent-runtime.test.mjscover cache reuse across worktrees plus corrupt-entry repair, "first publish wins, no leftover staging entries", and the compiler-override scrub. Each fails when its fix is reverted.git diff --check, andbiome format/biome lint --error-on-warningsonscriptsandtests/integration, are clean.Real runs of
node scripts/build-agent-runtime.mjs(local, macOS arm64, not CI):cargo installs per worktreecargo buildtarget-dirtarget/A build under
RUSTFLAGS=-C target-cpu=nativeproduced a bundle identical apart from the Mach-O UUID and signature, confirming the scrub.Agent review (drone1) and a simplification pass (drone2) ran, and their blockers were fixed.
Deferred
just desktopin two fresh worktrees from this branch. The second should printVerified inputs restored from …and launch normally. This is why the PR is still a draft, andbuzz-review-completedisn't added yet.just desktoplaunch wasn't run by an agent (it needs Keychain approval on this machine).Note: the Node integration tests fail with spawn EACCES if you run them with the Buzz-bundled
node, because of the space in its path. That was already the case before this PR. Putbin/first onPATH.🤖 Generated with Claude Code