Skip to content

fix(build): resolve CARGO_MANIFEST_DIR at run time, not compile time - #614

Merged
colinhacks merged 4 commits into
mainfrom
buildrs-path-pinning
Jul 29, 2026
Merged

fix(build): resolve CARGO_MANIFEST_DIR at run time, not compile time#614
colinhacks merged 4 commits into
mainfrom
buildrs-path-pinning

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

Three build scripts resolved their inputs through env!("CARGO_MANIFEST_DIR"), which bakes an absolute path into the compiled build-script binary. Cargo caches those per target dir, and this repo shares one target dir across worktrees — so the path stays pinned to whichever worktree compiled it first.

While both worktrees exist, a build silently reads the other tree's files. Once that worktree is deleted, every tree sharing the cache fails on a path absent from the current checkout — a build error on correct source, naming a directory the developer has never seen.

Both modes were live in the shared cache: an aube-settings script pinned to a deleted worktree, and two nub-cli scripts pinned to different live worktrees. nub-cli is the worst — it bakes site/content/docs, which feeds nub agent skill. nub-native stamps the git SHA into NUB_NATIVE_BUILD_ID, the transpile-cache key.

Cargo sets CARGO_MANIFEST_DIR in the build script's environment at run time, so the runtime lookup resolves per invocation. cargo publish --verify is unaffected — it still resolves inside target/package/<crate>-<ver>/. nub-core and aube-resolver already did this.

Recovery for an already-poisoned cache: rm -rf <target>/*/build/{aube-settings,nub-cli,nub-native}-*. Note cargo clean -p aube-settings from the repo root is a silent no-op — it reports "Removed 0 files" and exits 0, because the aube crates are not root workspace members.

Verified: rustfmt clean; nub-cli/build.rs type-checks standalone under rustc; the String-to-path conversions type-check. A full cargo build was OOM-killed four times by concurrent load on the dev host, so compilation of all three is left to CI.

Three build scripts resolved their inputs through `env!("CARGO_MANIFEST_DIR")`,
which bakes an absolute path into the compiled build-script binary. Cargo
caches those binaries per target dir, and this repo shares ONE target dir
across git worktrees, so the path stays pinned to whichever worktree compiled
it first.

Two failure modes follow. While both worktrees exist, a build reads the OTHER
tree's files and succeeds with the wrong content. Once that worktree is
deleted, every tree sharing the cache fails outright on a path absent from the
current checkout — a build error on provably correct source, naming a
directory the developer has never seen.

Both were live in the shared cache: an `aube-settings` script pinned to a
deleted worktree (the reported failure), and two `nub-cli` scripts pinned to
different live worktrees. `nub-cli` is the worst of the three — it bakes
`site/content/docs`, which feeds `nub agent skill`, so the wrong-content case
ships another worktree's docs. `nub-native` stamps the git SHA into
`NUB_NATIVE_BUILD_ID`, the transpile-cache key, so it can key the cache to a
sibling worktree's commit.

Cargo sets `CARGO_MANIFEST_DIR` in the build script's environment at run time,
so the runtime lookup resolves per invocation. `cargo publish --verify` is
unaffected: cargo sets it from the package being built, so it still points
inside `target/package/<crate>-<ver>/`. That property comes from `settings.toml`
living inside the crate, not from how the path is resolved — the comment
claiming otherwise is corrected.

`nub-core` and `aube-resolver` already used the runtime form; this brings the
other three in line.

vendor/aube hunk is default-preserving: the resolved value is byte-identical on
every path standalone aube takes, differing only when a build-script binary is
reused across manifest dirs, which is the bug.
Copilot AI review requested due to automatic review settings July 29, 2026 16:56
@vercel

vercel Bot commented Jul 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview Jul 29, 2026 10:05pm

Request Review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The run-time lookup is correct and necessary, but on its own it does not fix the silent cross-worktree read the doc comment claims it fixes. I reproduced both modes with cargo 1.97.1 — details inline.

Reviewed changes — swaps compile-time env!("CARGO_MANIFEST_DIR") for a run-time std::env::var lookup in three build scripts, so a build-script binary cached in a shared target dir no longer carries a path pinned to whichever git worktree compiled it first.

  • Run-time manifest dir in nub-cli/build.rs — the docs tree baked into DOCS (which backs nub agent docs / nub agent skill) is now located per invocation.
  • Run-time manifest dir in nub-native/build.rsrun_git's working directory and git_dir's relative-path absolutization, both feeding the NUB_NATIVE_BUILD_ID transpile-cache key.
  • Run-time manifest dir in aube-settings/build.rssettings.toml lookup, with the surrounding comment clarified to say the publish-tarball property comes from the file's location rather than the resolution style.
  • Shared manifest_dir() helper per crate — an identical helper plus an identical 11-line rationale comment added to each of the three scripts.

ℹ️ The shared-target-dir isolation rule treats a leaf binary as safe, and a build script breaks that premise

scripts/rust-build.sh decides between the shared target dir and a private one by checking whether this worktree diverges a depended-on crate, explicitly excluding crates/nub-cli because "a divergent leaf binary just rebuilds cleanly and is safe to share." That premise holds for the crate's own rlib, but not for its build script: nub-cli's build script reads inputs from outside the crate and writes generated code into the shared build/<pkg>-<hash>/ slot, which every sibling worktree also uses.

The consequence is that the class of bug this PR is about is not confined to crates other worktrees link:

  • a leaf crate with a build script that reads out-of-tree inputs collides in the shared slot exactly like a depended-on library does
  • the wrapper's divergence check never fires for nub-cli, so the isolation escape hatch is unavailable for precisely the crate this PR names as the worst case
  • .claude/skills/rust-build/SKILL.md records the same model, so the next reader inherits the gap
Technical details
# Shared-target-dir isolation model does not account for build-script slots

## Affected sites
- `scripts/rust-build.sh` — divergence check runs `git diff --name-only "$base" -- vendor/aube crates ':(exclude)crates/nub-cli'`, so a worktree that changes only `crates/nub-cli` (or `site/content/docs`, which `nub-cli`'s build script reads) always stays on the shared target dir.
- `scripts/rust-build.sh` header comment — "It only bites crates that OTHER crates link; a divergent leaf binary (nub-cli) just rebuilds cleanly and is safe to share." Incomplete: the build-script run unit and its `OUT_DIR` are shared per package id, independent of whether anything links the crate.
- `.claude/skills/rust-build/SKILL.md` — documents the same depended-on-crates-only model.

## Required outcome
- The isolation decision (or the documented model) accounts for build scripts whose inputs live outside the crate directory, not just for crates that other crates link.
- A reader of the script and the skill doc comes away with an accurate picture of which shared-slot collisions are possible.

## Suggested approach
Two directions, not mutually exclusive:
- Narrow the problem instead of widening isolation: with `cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR` in place (see the inline finding on `crates/nub-cli/build.rs`), the build-script slot self-corrects on a worktree switch, and the leaf-binary exemption becomes accurate again. This is the cheaper and more targeted route.
- Independently, correct the header comment in `scripts/rust-build.sh` and the model in `.claude/skills/rust-build/SKILL.md` so the "leaf binary is safe" claim is qualified by "unless its build script reads outside the crate."

## Open questions for the human
- Is extending the divergence check to `site/content/docs` (a non-crate path that `nub-cli`'s build script consumes) worth the cold builds it would cost docs-only worktrees, or is the fingerprint fix sufficient on its own?

ℹ️ Nitpicks

  • The 11-line rationale comment is duplicated verbatim in all three build scripts. Since build scripts cannot share code without a build-dependency crate (and vendor/aube should not depend on a nub crate), duplicating the tiny helper is reasonable, but the full rationale would read better in one place with a one-line pointer in the other two.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread crates/nub-cli/build.rs Outdated
Comment on lines +28 to +38
/// Cargo's `CARGO_MANIFEST_DIR` read at RUN time, not baked in at compile time.
///
/// `env!` freezes whichever manifest dir compiled the build script into the
/// binary. Build-script binaries are cached per target dir, and this repo shares
/// ONE target dir across git worktrees, so an `env!` path stays pinned to the
/// worktree that happened to build it first: later builds from a sibling
/// worktree silently read THAT tree's files, and once it is deleted they fail
/// outright on a path absent from the current checkout. The runtime lookup
/// resolves per invocation instead. `cargo publish --verify` is unaffected —
/// cargo sets the variable from the package being built, so it still points
/// inside `target/package/<crate>-<ver>/`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ The claim that the run-time lookup fixes the silent cross-worktree read does not hold — I reproduced it. With a shared target dir, cargo does not rerun the build script from a sibling worktree at all (it reports Finished with nothing recompiled), because the recorded cargo:rerun-if-changed entry holds the first tree's absolute path and is still fresh, so OUT_DIR keeps that tree's generated code. Adding cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR alongside the existing rerun-if-changed closes it, and only works because of this PR's run-time lookup.

Technical details
# Run-time lookup fixes the hard-failure mode only; the silent-wrong-tree mode needs a fingerprint fix

## Empirical basis (cargo 1.97.1, two trees sharing one CARGO_TARGET_DIR, identical package id)

Build script reads `<manifest_dir>/../data.txt` and emits `cargo:rerun-if-changed=<canonicalized absolute path>`.

| variant | build A | then build B | then A again |
| --- | --- | --- | --- |
| `env!` (before) | `from-tree-A` | `from-tree-A`||
| `env::var` (this PR) | `from-tree-A` | `from-tree-A`| `from-tree-A` |
| `env::var` + relative `rerun-if-changed` | `from-tree-A` | `from-tree-A`| `from-tree-A` |
| `env::var` + `rerun-if-env-changed=CARGO_MANIFEST_DIR` | `from-tree-A` | `from-tree-B`| `from-tree-A`|
| `env!` + `rerun-if-env-changed=CARGO_MANIFEST_DIR` | `from-tree-A` | `from-tree-A`||

Additional observations:

- On the `env::var` build from tree B, cargo printed only `Finished` — the build script never ran. The recorded output file contained `cargo:rerun-if-changed=/tmp/probe3/A/data.txt`.
- Touching tree B's own input did not trigger a rerun either, because B's path is not in the fingerprint.
- Deleting tree A did make B rebuild correctly, which is the mode this PR genuinely fixes.
- The last row confirms the run-time lookup is a prerequisite: with `env!`, the script reruns but the baked path is still the sibling's.

## Affected sites
- `crates/nub-cli/build.rs:28-38` — doc comment asserts "later builds from a sibling worktree silently read THAT tree's files" is resolved by the run-time lookup; it is not.
- `crates/nub-cli/build.rs:71``cargo:rerun-if-changed={docs_dir}` is canonicalized, therefore absolute, therefore pinned to the tree that ran the script. Consequence: `DOCS` (and so `nub agent docs` / `nub agent skill`) can serve a sibling worktree's docs.
- `crates/nub-native/build.rs:6-16` and `:47-57` — same comment claim; the `.git/HEAD`, `.git/index`, `.git/refs`, `.git/packed-refs` watches are absolute and per-worktree, so `NUB_NATIVE_BUILD_ID` can carry a sibling tree's short SHA and mis-key the transpile cache.
- `vendor/aube/crates/aube-settings/build.rs:100-110` and `:124` — same comment claim; `settings.toml` path is absolute.

## Required outcome
- A build from worktree B never consumes generated output derived from worktree A's sources, while both trees exist.
- The rationale comment describes only what the change actually guarantees.

## Suggested approach
Add one line to each of the three build scripts, next to their existing `cargo:rerun-if-changed` emissions:

```rust
println!("cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR");
```

`nub-native/build.rs` already emits a `rerun-if-env-changed` line, so it slots in directly beside it. Cost in normal single-tree use is nil — the value is stable — and the rerun on a worktree switch is the desired behavior. The `aube-settings` change stays default-preserving for a standalone build, since the value only ever differs when target dirs are shared across trees.

Then tighten the comment so it distinguishes the two mechanisms: the run-time lookup makes the *resolution* correct, the env-changed fingerprint makes cargo actually *rerun* the script when the tree changes.

## Open questions for the human
- `crates/nub-cli/build.rs:71` watches the docs *directory* only, so nested `.mdx` edits do not invalidate it even within a single tree. Pre-existing and out of scope here, but worth deciding whether it belongs in this PR or a follow-up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to the suggested fix here: re-running the repro with the input OUTSIDE the crate directory (the shape nub-cli and nub-native have) shows rerun-if-env-changed=CARGO_MANIFEST_DIR does not trigger a rerun on cargo 1.97.1 — details in the new review.

Pullfrog  | View workflow run | via Pullfrog | Using Claude Opus𝕏

Comment thread crates/nub-cli/build.rs Outdated
The run-time lookup alone fixes only the hard-failure mode. Cargo will not
rerun a build script it considers fresh, and the recorded `rerun-if-changed`
paths are the first tree's absolutes — so a sibling worktree kept consuming
that tree's generated output without the script ever running. Review on #614
reproduced this on cargo 1.97.1: `env::var` alone still served tree A's
content to tree B; adding the env fingerprint served B's.

Emit `cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR` next to each script's
existing rerun-if-changed lines. The two halves are interdependent: the
fingerprint makes cargo rerun the script, the run-time lookup makes that rerun
resolve to the current tree. Neither works alone — with `env!` the script
reruns and still reads the sibling's path.

Cost in single-tree use is nil, since the value is stable.

Also correct the rationale comment, which claimed the lookup fixed the silent
cross-worktree read, and return `PathBuf` from `var_os` rather than `String`
from `var` — that drops three `PathBuf::from` wraps at call sites and avoids a
panic message blaming cargo when the manifest path is not UTF-8. Matches the
existing shape in aube-resolver's build script.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR lines added in c9797f0 have no effect on cargo 1.97.1 (the toolchain in this checkout), so the silent cross-worktree read is still live and the new doc comment asserts a pairing that does not hold. The run-time lookup from c7dac01 stands on its own and is worth keeping.

Reviewed changes — the delta since the prior review is c9797f0, which acts on both threads from that review.

  • Added cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR to all three build scripts, next to their existing rerun-if-changed emissions, intended to make cargo rerun the script on a worktree switch.
  • Changed manifest_dir() to return PathBuf from var_os in all three scripts, dropping the PathBuf::from(...) wrapping at the call sites.
  • Rewrote the tripled rationale comment to split the fix into a resolution half and a rerun half, and to state that neither works without the other.

ℹ️ crates/nub-core/build.rs has the same unfixed shape, and the cheap route to closing the rust-build.sh gap does not exist

nub-core's embed-runtime build script reads <repo>/runtime — outside the crate directory, like nub-cli's docs tree — and emits an absolute rerun-if-changed on it. That is the exact shape that silently reads a sibling tree's inputs, so the PR's premise that the two untouched build scripts were "already correct" holds only for the hard-failure mode, not the silent one. Exposure is narrower than nub-cli's (the feature is off in the dev fast profile), but it is the same defect and it embeds both the runtime blob and the baked cache key.

Separately, the prior review suggested that a working fingerprint fix would make scripts/rust-build.sh's "a divergent leaf binary just rebuilds cleanly and is safe to share" claim accurate again. Since the fingerprint route turns out not to work, that option is off the table and the script's model plus .claude/skills/rust-build/SKILL.md still describe a hazard narrower than the real one.

Technical details
# Remaining exposure and the wrapper-level model

## Affected sites
- `crates/nub-core/build.rs:29-71` — run-time `env::var("CARGO_MANIFEST_DIR")`, staging dir defaults to `manifest_dir/../../runtime`, `cargo:rerun-if-changed={staging}` is canonicalized and absolute. Under a shared target dir a feature-on build from tree B can embed tree A's `runtime.tar.zst` together with the matching baked `runtime-<version>-<blobhash8>` key.
- `vendor/aube/crates/aube-resolver/build.rs:33-51` — same run-time lookup, but the default input is in-crate (`manifest_dir/data/...`), which is the benign shape; only an `AUBE_PRIMER_PATH` override moves it out of the crate, and that override is already `rerun-if-env-changed`.
- `scripts/rust-build.sh` — divergence check is `git diff --name-only "$base" -- vendor/aube crates ':(exclude)crates/nub-cli'`, so neither `site/content/docs` nor repo-root `runtime/` nor `crates/nub-cli` itself can trigger isolation.
- `.claude/skills/rust-build/SKILL.md` — records the same depended-on-crates-only model.

## Required outcome
- Whatever mechanism ends up fixing `nub-cli` and `nub-native` also covers `nub-core`, or the PR states explicitly which scripts remain exposed and why that is acceptable.
- The wrapper's header comment and the skill doc describe the collision class accurately: a leaf crate's build script that reads outside its crate dir collides in the shared slot just like a depended-on library does.

## Open questions for the human
- Is `crates/nub-core` worth covering now, given `embed-runtime` is release/CI-only and CI builds are single-tree? The local `NUB_RUNTIME_STAGING_DIR` packaging flow is the one place a developer would hit it.
- Should the divergence check grow to cover build-script inputs (`site/content/docs`, `runtime/`, `crates/nub-cli`), accepting cold builds for docs-only worktrees, or is a per-worktree env var the preferred lever?

ℹ️ Nitpicks

  • vendor/aube/crates/aube-settings/build.rs:132-135settings.toml is inside the crate directory, and in that shape the two-tree switch already reruns the script without any added directive (measured). So both the added line and the worktree rationale above it are inert for this crate, while the vendored file now carries nub-worktree-specific reasoning for a hazard it does not have.
  • The rationale comment is still duplicated verbatim in all three scripts; whatever it ends up saying, saying it once with a one-line pointer in the other two would keep the three copies from drifting apart.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread crates/nub-cli/build.rs
Comment on lines +80 to +83
// Paired with the run-time lookup in `manifest_dir`: the recorded
// rerun-if-changed path above is absolute, so without this cargo keeps a
// sibling worktree's generated DOCS and never reruns the script.
println!("cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This directive does not fire, so the silent cross-worktree read it is meant to close is still live. Cargo evaluates rerun-if-env-changed against cargo's own process environment, and CARGO_MANIFEST_DIR is never in it — so the value reads as absent on both builds, the fingerprint is considered unchanged, and the script is not rerun. I measured this on cargo 1.97.1: tree B printed only Finished (0.01s), the recorded output file still held tree A's absolute path, and the binary still carried tree A's docs.

Technical details
# `rerun-if-env-changed=CARGO_MANIFEST_DIR` is inert; the tree-switch rerun needs a variable cargo can see

## Empirical basis (cargo 1.97.1, two path packages with the same name+version at different absolute paths, one shared `CARGO_TARGET_DIR`)

The `build/<pkg>-<hash>/` and `.fingerprint/<pkg>-<hash>/` slot hashes were byte-identical across the two trees, confirming the collision premise. The deciding variable is whether the build script's INPUT lives inside or outside the crate directory.

| input location | directives | A → B → A → B |
| --- | --- | --- |
| inside crate dir | `rerun-if-changed` only | A, **B**, A, B — reruns anyway |
| inside crate dir | `+ rerun-if-env-changed=CARGO_MANIFEST_DIR` | A, **B**, A, B — indistinguishable from above |
| OUTSIDE crate dir | `rerun-if-changed` only | A, **A**, A, A — silent cross-tree read |
| OUTSIDE crate dir | `+ rerun-if-env-changed=CARGO_MANIFEST_DIR` | A, **A**, A, A — no effect |
| OUTSIDE crate dir | `+ rerun-if-env-changed=PROBE_TOKEN`, token changed | A, **B** — positive control, the directive mechanism itself works |
| OUTSIDE crate dir | `CARGO_MANIFEST_DIR` exported into **cargo's** process env | A, **B** — confirms where cargo reads the value from |

The earlier repro that showed this directive working used an in-crate input, where the rerun happens for an unrelated reason (rows 1-2), which is why it looked effective.

## Affected sites
- `crates/nub-cli/build.rs:80-83` — directive is inert; `../../site/content/docs` is out-of-crate, so `DOCS` (and `nub agent docs` / `nub agent skill`) can still serve a sibling worktree's docs.
- `crates/nub-cli/build.rs:35-40`, `crates/nub-native/build.rs:13-18`, `vendor/aube/crates/aube-settings/build.rs:107-112` — the doc comment states "the paired `cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR` below is what makes cargo rerun on a tree switch; neither half works without the other." The second half does nothing.
- `crates/nub-native/build.rs:66-68` — directive is inert; the `.git/{HEAD,index,refs,packed-refs}` watches are per-worktree absolutes, so `NUB_NATIVE_BUILD_ID` can still carry a sibling tree's short SHA and mis-key the transpile cache.
- `vendor/aube/crates/aube-settings/build.rs:132-135``settings.toml` is in-crate, so this crate was never in the failing shape and the directive changes nothing either way.

## Required outcome
- Either a build from worktree B provably never consumes generated output derived from worktree A's sources while both trees exist, or the doc comment claims only what the change actually guarantees (the hard-failure mode) and the silent mode is recorded as still open.
- No file asserts a rerun mechanism that does not fire.

## Suggested approach
Two mechanisms cargo actually evaluates:
- Have `scripts/rust-build.sh` export a per-worktree variable into cargo's own environment (it already computes `root=$(git rev-parse --show-toplevel)`), and emit `cargo:rerun-if-env-changed` on THAT variable in the build scripts. The positive-control row proves the mechanism works for a non-cargo variable. Caveat: a raw `cargo build` that bypasses the wrapper gets no protection, so the wrapper becomes load-bearing for correctness rather than just speed.
- Or extend the wrapper's isolation rule to cover build-script inputs (`site/content/docs`, `runtime/`, and `crates/nub-cli` itself), trading cold builds for a mechanism that does not depend on cargo's fingerprint semantics.

Either way, the run-time lookup from `c7dac01` should stay — it is what fixes the hard failure once the first worktree is deleted.

Comment on lines +66 to +68
// The .git watches above are absolute and per-worktree; without this a
// sibling tree reuses this tree's short SHA and mis-keys the transpile cache.
println!("cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same mechanism as the nub-cli finding — this line does not fire, so NUB_NATIVE_BUILD_ID can still take a sibling worktree's short SHA and mis-key the transpile cache, and the comment above it describes a fix that is not in effect.

Self-review found the comment's central claim false. "Neither half works
without the other" does not hold: on cargo 1.95 the run-time lookup alone
fixes both modes, because the `rerun-if-changed` path the script then emits
differs per tree and cargo invalidates on that. Reproduced independently here
— two trees, one shared target dir, no env fingerprint, and the second tree
still built its own content.

A review on cargo 1.97 measured the opposite. Rather than pick a winner
across versions CI resolves at `stable`, keep the fingerprint as
version-insurance and say so. It is independently load-bearing in nub-native,
which emits no path-dependent instruction when there is no git checkout.

Also de-duplicates: the same 17-line block appeared verbatim in all three
scripts, ~51 lines of comment for ~6 of code, against the sparse-and-dense
bar. One full rationale in nub-cli, short self-contained notes elsewhere.

The aube copy no longer describes this repo's worktree convention or cites a
nub path — that was fork delta which would conflict on the next upstream sync
and is false of standalone aube anyway.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

a3f8eaa changes only comments, and it replaces one incorrect claim about the rerun mechanism with another. I installed cargo 1.95.0 alongside the checkout's 1.97.1 and re-ran the two-tree experiment on both: the results are identical, so the disagreement the comment attributes to a cargo version difference does not exist. The run-time lookup from c7dac01 is still the right change and worth keeping.

Reviewed changes — the delta since the prior review is a3f8eaa, a comment-only rewrite of the rationale added in c7dac01/c9797f0.

  • Rewrote the rationale in nub-cli/build.rs — drops "neither half works without the other" in favour of a cargo-version split (1.95 sufficient, 1.97 not) and keeps cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR as "version-insurance" that is "independently load-bearing in nub-native".
  • De-duplicated the tripled commentnub-native now carries a three-line summary pointing at crates/nub-cli/build.rs, addressing the drift nitpick from both prior reviews.
  • Made the aube-settings comment checkout-neutral — it no longer reasons about nub worktrees specifically, which fits the vendored file better; its env!-pinning description is accurate for that crate.
  • No code change — the three manifest_dir() helpers, every call site, and all three rerun-if-env-changed emissions are byte-identical to c9797f0.

ℹ️ Nitpicks

  • The PR description still describes only c7dac01. It never mentions the cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR lines, which are the contested part of the change, so the merge record will not carry their rationale either way.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread crates/nub-cli/build.rs
Comment on lines +36 to +42
/// Measured on cargo 1.95: the run-time lookup alone is sufficient, because the
/// `rerun-if-changed` path this script emits then differs per tree and cargo
/// invalidates on that. A review on cargo 1.97 measured the opposite, so the
/// paired `cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR` below is kept as
/// version-insurance; it costs nothing in a single tree, where the value never
/// changes. It is independently load-bearing in `nub-native`, which emits no
/// path-dependent instruction when there is no git checkout.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I installed cargo 1.95.0 next to the checkout's 1.97.1 and re-ran the two-tree experiment on both — the results are identical, so this is not a version difference. With an out-of-crate input (this script's ../../site/content/docs), the run-time lookup alone does not make the sibling tree rerun; it silently links the first tree's generated docs, and adding rerun-if-env-changed=CARGO_MANIFEST_DIR changes nothing on either toolchain. The "independently load-bearing in nub-native" sub-claim is refuted too: with that directive as the only rerun instruction, the tree switch still does not rerun.

Technical details
# The rationale now attributes an inert directive to a cargo version difference that does not exist

## Empirical basis

Two path packages, same name+version, different absolute paths, one shared `CARGO_TARGET_DIR` (the worktree situation). `A → B → A → B`; rerun judged by a `Compiling` line, correctness by what the produced binary reports. `rustup toolchain install 1.95.0 --profile minimal`, then each row run under `cargo +1.95.0` and `cargo +1.97.1`.

| input location | directives | 1.95.0 | 1.97.1 |
| --- | --- | --- | --- |
| out-of-crate file | `rerun-if-changed` only | A, **A**, A, A | A, **A**, A, A |
| out-of-crate file | `+ rerun-if-env-changed=CARGO_MANIFEST_DIR` | A, **A**, A, A | A, **A**, A, A |
| out-of-crate DIRECTORY (this script's shape) | `+ rerun-if-env-changed=CARGO_MANIFEST_DIR` | A, **A**, A | A, **A**, A |
| out-of-crate, directive as the ONLY instruction (`nub-native` with no git checkout) | `rerun-if-env-changed=CARGO_MANIFEST_DIR` | A, **A**, A | A, **A**, A |
| in-crate file (`aube-settings` shape) | `rerun-if-changed` only | A, **B**, A | A, **B**, A |

The last row is the likely origin of the "cargo 1.95 sufficient" reading: an in-crate input does rerun on a tree switch, but for a reason unrelated to the path the script emits, so it does not generalize to an out-of-crate input. `rerun-if-env-changed` is evaluated against cargo's OWN process environment, where `CARGO_MANIFEST_DIR` is absent on both builds — the prior review's positive control (a non-cargo `PROBE_TOKEN`, and exporting `CARGO_MANIFEST_DIR` into cargo's env) showed the directive mechanism itself works.

## Affected sites
- `crates/nub-cli/build.rs:36-42` — attributes the disagreement to cargo 1.95 vs 1.97, calls the directive version-insurance, and calls it independently load-bearing in `nub-native`. All three are refuted by the table above.
- `crates/nub-cli/build.rs:81-83` and `crates/nub-native/build.rs:52-53` — the inline comments were not touched by this commit and still assert the effect outright ("without this cargo keeps a sibling worktree's generated DOCS and never reruns the script"), so each file now contradicts its own doc comment.

## Required outcome
- No comment in the tree asserts, hedges toward, or version-gates an effect the directive does not have on any toolchain measured.
- The comment states what the change does guarantee (the hard-failure mode: the first tree is deleted and the build no longer fails on a path absent from the current checkout) and records the silent wrong-tree read as still open.

## Suggested approach
Either drop the three `rerun-if-env-changed=CARGO_MANIFEST_DIR` lines and their inline comments, or keep them and say plainly that they are unproven no-ops on 1.95/1.97 rather than describing an effect. Both are consistent with keeping the run-time lookup, which is what fixes the hard failure.

## Open questions for the human
- Is closing the silent mode in scope for this PR (a per-worktree variable exported into cargo's env by `scripts/rust-build.sh`, or target-dir isolation), or should it ship as hard-failure-only with the silent mode written down as known-open?

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the code — the only new commit is a merge of main, and it leaves all three build scripts' PR-authored hunks intact. One comment-accuracy suggestion inline, plus one observation about a mechanism the merge changed underneath this PR's rationale.

Reviewed changes — the delta since the prior review is 4344c27, a merge of main into the branch. No commit has modified a PR-authored line since a3f8eaa.

  • Merged main — the diff against the new merge base is still exactly the manifest_dir() change in nub-cli, nub-native, and aube-settings, so the three open threads from earlier reviews are untouched.
  • Preserved main's aube-settings/build.rs editcacheDir joins the no-default list in default_expr (line 496) alongside this PR's helper, with no conflict residue. After the merge, no build script in the tree contains env!("CARGO_MANIFEST_DIR").
  • Pulled in a rewritten scripts/rust-build.sh — the shared target dir is now content-keyed into shared-target-<hash> buckets, and an isolating worktree's private target/ is CoW-seeded from a bucket instead of built cold.

ℹ️ The merge replaces the cache model this PR reasons about, and seeding gives the silent wrong-tree read a new route

Two things changed under this PR. First, the content key that names a bucket hashes vendor/aube and crates while excluding crates/nub-cli and crates/nub-native, and it never reaches the top-level site/content/docs at all — so the two crates whose build scripts read out-of-crate inputs are exactly the ones keying does not separate, and nub-native divergence, which used to force isolation, no longer does. The run-time lookup this PR adds is still needed for both, and its exposure grew rather than shrank.

Second, isolation is no longer a clean escape. seed_from is an unfiltered whole-directory clone, so a private target/ inherits the seed bucket's build/<pkg>-<hash>/{output,out/} and .fingerprint/. The script's own header records that a cloned warm dir "rebuilt 0 crates", the copied rerun-if-changed path points at the seed worktree and is unchanged there, and rerun-if-env-changed=CARGO_MANIFEST_DIR is inert on both 1.95.0 and 1.97.1 — so a seeded worktree links the seed tree's baked DOCS and its short SHA. That is the same silent mode the earlier threads describe, now reachable in the isolated case that previously built cold.

None of this asks for a change to this diff. It does mean the "what remains open" question from the prior review now has a second surface.

Technical details
# The merged target-dir model changes both the exposure and the escape hatch

## Affected sites
- `scripts/rust-build.sh:81``leaves=":(exclude)crates/nub-cli :(exclude)crates/nub-native :(exclude)crates/nub-phantom"`, threaded into the divergence check (`:101-106`) and the bucket key (`:112`). The key's pathspec is `vendor/aube crates $leaves`, so `site/content/docs` is outside it entirely and both leaf crates are excluded by name. `vendor/aube/crates/aube-settings/settings.toml` IS covered, which is the one input of the three that keying protects.
- `scripts/rust-build.sh:127-153` (`seed_from`), called at `:179` (isolation) and `:190` (legacy migration) — `cp -c -a "$1/." "$_claim/"` with no exclusions, so build-script binaries, recorded `output`, `OUT_DIR`, and `.fingerprint/` all cross into the private dir.
- `scripts/rust-build.sh:15-22` — records the measurement that a warm dir cloned to a new path "rebuilt 0 crates", which is what makes the cloned build-script fingerprint validate instead of rerunning.
- `crates/nub-cli/build.rs:81-84`, `crates/nub-native/build.rs:52-54` — the emitted `rerun-if-env-changed=CARGO_MANIFEST_DIR` that would otherwise break the reuse chain. Measured inert on 1.95.0 and 1.97.1: cargo evaluates the directive against its own process environment, where the variable is absent on both builds.
- `scripts/rust-build.sh:24-31` — the header still says "a divergent leaf binary (nub-cli) just rebuilds cleanly and is safe to share", now covering `nub-native` too.

## Required outcome
- The record for this change reflects the post-merge model: which worktrees can share a bucket, and that a seeded private dir is not a fresh build-script run.
- Whatever eventually closes the silent mode covers the seeded-isolation path, not only the shared-bucket path.

## Open questions for the human
- Is the seeding interaction in scope here, or does it belong to whoever owns `scripts/rust-build.sh`? Adding `site/content/docs` (and repo-root `runtime/`, for `nub-core`) to the hashed pathspec would separate the buckets and give seeding a sound source; a per-worktree variable exported into cargo's environment would fix the rerun side instead.
- Does this change the earlier answer on shipping hard-failure-only, given isolation no longer sidesteps the silent mode?

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread crates/nub-cli/build.rs
Comment on lines +31 to +32
/// Cargo caches those binaries per target dir and this repo shares ONE target
/// dir across worktrees, so the path outlives the tree that produced it: a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge makes "shares ONE target dir" stale: scripts/rust-build.sh now content-keys the shared dir into shared-target-<hash> buckets. The rationale still holds here — the key hashes vendor/aube and crates while excluding crates/nub-cli, and it never covers site/content/docs — but a reader who follows the claim to the script will find keying and could conclude it already separates this crate.

Suggested change
/// Cargo caches those binaries per target dir and this repo shares ONE target
/// dir across worktrees, so the path outlives the tree that produced it: a
/// Cargo caches those binaries per target dir, and the worktrees here share one
/// content-keyed bucket whose key covers neither `crates/nub-cli` nor
/// `site/content/docs` (`scripts/rust-build.sh`), so the path outlives the tree
/// that produced it: a

@colinhacks
colinhacks merged commit 3ee7b5d into main Jul 29, 2026
53 checks passed
colinhacks added a commit that referenced this pull request Jul 30, 2026
…an pin paths

Two shared-target-dir footguns that each cost a real debugging cycle.

`vendor/aube/.cargo/config.toml` pins RUST_TEST_THREADS=1 workspace-wide,
deliberately, over per-test mutexes — several aube-util tests mutate the process
environment and setenv/getenv are not thread-safe. Cargo discovers config from
the CWD, not from --manifest-path, so running the suite from the repo root
silently drops the pin and runs those tests in parallel. The resulting failures
(set_allow_builds_*, pnpmfile::tests::detect_*) look like real bugs and CI never
sees them, because CI uses working-directory: vendor/aube. Document the correct
invocation in both skills.

Also record the build-script path-pinning shape in rust-build, beside the
phantom-E0063 entry it resembles: a compile-time env!("CARGO_MANIFEST_DIR")
bakes the compiling worktree's path into the cached build-script binary, which
then reads another tree's files or fails on a path absent from the checkout.
Fixed at source in #614; the recovery command is the rm -rf of the build dir,
because cargo clean -p from the root silently removes nothing for crates that
are not root workspace members.
colinhacks added a commit that referenced this pull request Jul 30, 2026
…al probe

The prior commit message and code comments claimed cmd.exe cannot work inside
an AppContainer without a privileged setup step. That is wrong and is removed.
The measurement it rested on used a bare zero-capability AppContainer whose
ancestor tier only wrote ReadAndExecute on the fixture root and %USERPROFILE%;
nub-sandbox's production launch is strictly stronger -- it writes a
non-inherited traverse ACE where it can, and where it cannot it REQUESTS the
capability SID Windows already granted on that ancestor, harvested off the
DACL. Requesting a raw capability SID is unprivileged.

The rationale that survives, all independently measured: one shell across nub
surfaces (nub run already defaults to busybox); zero compatibility cost (0 of
363 corpus script bodies use cmd-only syntax); it FIXES detox-recorder and
svf-lib, which invoke ./*.sh; and busybox needs no ancestor repair at all,
where cmd.exe depends on a repair that is best-effort by design.

Verification:
- tests/busybox-lifecycle-probe/ -- branch-scoped Windows differential. One
  binary, each case installed twice, and the only variable is whether the
  busybox.exe sidecar is present; its absence reproduces the pre-change cmd.exe
  path exactly. Asserts the split in both directions, so a case that behaves
  the same under both shells is reported as a failure rather than a pass.
  Three broken controls were found and fixed while building it: a shared
  fixture let a lifecycle failure abort the JoinSet and truncate sibling
  markers; the side-effects cache keys on (name, version, engine, input hash)
  with the shell absent from the key, so the second arm restored the first
  arm's build; and a shared package name made the CAS serve one case's built
  tree to all three.
- pm_augment.rs -- a committed cross-platform test whose root postinstall body
  is POSIX-only, so cmd.exe fails the install instead of passing quietly.
- pm_engine unit test pinning the `sh -c` applet form, which is a
  Windows-only breakage no other test on this suite would see.

Refs #614
@colinhacks

Copy link
Copy Markdown
Contributor Author

Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0

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