Skip to content

fix(pm): stop nub writing aube-branded filesystem paths - #600

Merged
colinhacks merged 5 commits into
mainfrom
aube-path-leak
Jul 29, 2026
Merged

fix(pm): stop nub writing aube-branded filesystem paths#600
colinhacks merged 5 commits into
mainfrom
aube-path-leak

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

Nub created engine-branded paths in users' caches, stores, and node_modules. Two causes:

  1. embedder() falls back to the aube profile when identity is unregistered, and only the PM preflight registered it — so nub run wrote <cache>/aube/tools/node-gyp/lazy-bin/ and exported it as npm_config_node_gyp. main now registers identity first.
  2. ~16 hardcoded on-disk names now compose from aube_util::prog(). Real tree: 31 .aube-side-effects-cache, 3 pm/git/aube-git-*.

prog() is "aube" by default, so standalone aube is byte-identical (aube-scripts 87/87).

Guards: nub_never_writes_an_aube_branded_path, check-path-literals.sh. fmt, clippy, lints, pm_identity 12/12 pass.

Nub created paths carrying the embedded engine's brand in users' caches,
stores, and node_modules. Two independent mechanisms:

1. `aube_util::embedder()` falls back to the default aube profile whenever
   the identity OnceLock is unset, and identity was registered only inside
   the PM engine's preflight. `nub run` never reaches preflight, so it wrote
   the engine's lazy node-gyp shim to `<cache>/aube/tools/node-gyp/lazy-bin/`
   and handed that path to every script as `npm_config_node_gyp`. `main` now
   registers identity before anything else runs, which makes the fallback
   unreachable in the nub binary rather than fixing one call site.

2. On-disk marker, probe, and temp names inside the engine were hardcoded to
   `aube` instead of composed from the active profile, so they stayed
   brand-crossed even with identity correct. Observed in a real long-lived
   tree: `.aube-side-effects-cache` (inside the CAS store and inside project
   node_modules) and `~/.cache/nub/pm/git/aube-git-*`. Also covered: the
   linker's hardlink probe, the CAS temp prefix, `Store::at`'s sibling cache
   dir, the resolver/dlx/security-scanner/git-prepare/fetch temp prefixes,
   the `/tmp/aube-jail` script-sandbox root, the diag file, self-install's
   versions dir, and several temp-dir fallbacks.

Every engine-side name now routes through `aube_util::prog()` (or
`embedder().data_namespace`), which renders exactly `aube` under the default
profile — standalone aube's on-disk layout is byte-for-byte unchanged.

Guards, because this shipped for months with the path untested and the
literals unlinted:
- `pm_identity.rs::nub_never_writes_an_aube_branded_path` runs `nub run`
  under an isolated HOME/XDG root, asserts `npm_config_node_gyp` resolves
  inside nub's namespace, and walks the whole tree for any brand-crossed name.
- `tests/brand-lint/check-path-literals.sh`, the write-side twin of
  check-env-reads.sh, wired into `make verify` and the CI clippy job.

Existing cache entries under the old names go cold once and are rewritten.
Two follow-ups to cc07ef4, found while verifying it.

`unshare_one_file` wrote `.aube-unshare-<pid>-<file>.tmp` into the directory
it is unsharing — the caller's own node_modules — and the function's existing
cleanup path documents that a crashed run leaves the temp behind, so the name
is user-visible. Composed from `aube_util::prog()` like the other on-disk
names; `prog()` is `aube` under the default profile, so standalone aube is
unchanged, which the aube-scripts suite confirms (87 passed, including the
four break_cas_hardlinks tests that exercise this function).

`tests/brand-lint/check-path-literals.sh` did not catch it: the gate matches a
brand literal and a path-constructing call on the SAME line, and here the
literal sits inside a multi-line `format!` argument to `join(`. The gate is
otherwise sound — a violation planted at cas.rs:900, well past the first
`#[cfg(test)]` block, is reported correctly.

Also reformats the new assertion in `nub_never_writes_an_aube_branded_path`,
which failed `cargo fmt --check`.
The path-literal gate matched a brand literal and a path call only when they
sat on the same physical line. rustfmt splits a long call, so

    parent.join(format!(
        ".aube-unshare-{}-{}.tmp",

put the two on different lines and the gate reported clean. That exact shape
was a live leak into consumers' node_modules, and this gate passed over it.

Lines are now joined until parens balance, so the unit matched is a logical
statement. The brand literal must also contain no whitespace: every remaining
false positive was an English sentence reaching a `Vec::push` or a
`slice.join(", ")`, and no path literal in this tree contains a space — which
replaces several would-be allowlist entries with one rule.

Allowlist entries gained justifications, notably patch.rs: the engine's
`default_edit_parent` is unreachable under nub because `run_patch` always
injects `edit_dir` before calling it.

Verified with two positive controls: a same-line violation past the first
`#[cfg(test)]` block, and the multi-line shape above.
Copilot AI review requested due to automatic review settings July 29, 2026 02:23

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.

@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, Comment Jul 29, 2026 6:52am

Request Review

@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 fix itself is sound — I verified the read/write symmetry of every renamed name and confirmed early identity registration introduces no unintended behavior change. But both new guards have holes: the static gate silently skips ~7.4k lines it claims to cover, and the runtime test asserts a POSIX-shaped path so it fails on the Windows CI leg.

Reviewed changes — closes the write-side half of the brand boundary so no on-disk path nub creates carries the embedded engine's brand, via two independent fixes plus a runtime test and a new static gate.

  • Register embedder identity first in mainaube_util::embedder() falls back to the default AUBE profile whenever the OnceLock is unset, and registration previously happened only inside the PM engine's preflight, so nub run resolved cache_namespace = "aube" and handed scripts <cache>/aube/tools/node-gyp/lazy-bin/ as npm_config_node_gyp.
  • Compose ~16 hardcoded on-disk names from the active profile — the side-effects marker, the two git-cache entry prefixes, the linker probe files, the unshare temp leaf, the jail root, CAS/dlx/exec/scanner temp prefixes, aube-cache, aube-diag.jsonl, the .aube vstore fallback, and the temp_dir()/<data> store fallbacks now route through aube_util::prog() or embedder().data_namespace.
  • Add nub_never_writes_an_aube_branded_path — drives a real nub run against an isolated home, asserts npm_config_node_gyp positively, then recursively walks the fixture for any brand-crossed file name.
  • Add tests/brand-lint/check-path-literals.sh — the write-side twin of check-env-reads.sh, wired into the CI clippy job and make verify, scanning nub and the vendored engine for brand-carrying string literals in path-building statements.

⚠️ Nothing in CI checks the formatting of the vendored engine

Three of the sites this PR adds are not rustfmt-clean, and the misindented closure in state.rs is visible in the diff itself. It survived local verification because vendor/aube is its own Cargo workspace, excluded from the root one — so the root cargo fmt --check in make verify and in CI never sees those files, and neither does root cargo clippy --all-targets (path dependencies are compiled, not linted).

The gap is wider than this PR: every future edit under vendor/aube has the same blind spot, and aube-store/src/lib.rs:292 is already unformatted from an earlier change.

Technical details
# The vendored engine has no fmt or lint gate

## Affected sites
- `vendor/aube/crates/aube/src/state.rs`:1016-1021 — closure body indented to the `.unwrap_or_else` column instead of one level in.
- `vendor/aube/crates/aube-runtime/src/self_install.rs`:66, :70 — both over 100 columns; rustfmt wraps each into a multi-line `Some(...)`.
- `vendor/aube/crates/aube/src/commands/settings_context.rs`:261, :500 — both over 100 columns.
- `vendor/aube/crates/aube-store/src/lib.rs`:292 — pre-existing, untouched by this PR; listed only because it proves the gap predates this change.
- `Cargo.toml` `[workspace]` — the `exclude` of `vendor/aube` is load-bearing and correct; the missing piece is a second fmt invocation, not a workspace change.

## Required outcome
- The three PR-added sites are rustfmt-clean.
- A green local `make verify` and a green CI run imply the vendored engine is rustfmt-clean, rather than being silent about it.

## Suggested approach
Reproduce locally with `rustfmt --edition 2024 --check <file>` (plain `cargo fmt` from the root will not touch these). For the durable gate, `make verify` and the CI `clippy` job already shell out to a second workspace for `crates/nub-native`; the same shape works here:

```make
(cd vendor/aube && cargo fmt --check)
```

Whether to also add `(cd vendor/aube && cargo clippy --all-targets -- -D warnings)` is a bigger call — it lints the whole vendored tree, not just nub's delta, so it may surface a large pre-existing backlog.

## Open questions for the human
- Is a `vendor/aube` fmt gate in scope for this PR, or a follow-up? If a follow-up, the three sites above still want fixing here so the diff doesn't land misformatted.
- Should the gate cover clippy too, given the vendored tree's existing warning state is unknown?

⚠️ Branded paths already on disk are never cleaned up or migrated

The PR stops nub from creating engine-branded paths, but says nothing about the ones existing installs already have. Every user who has run nub run on a released build has a ~/.cache/aube/tools/node-gyp/ tree — the exact artifact the PR title describes — and it stays there indefinitely after upgrading.

Three families of already-written state, in rough order of how much they matter:

Already on disk Effect after this PR
<cache>/aube/tools/node-gyp/… Orphaned. The brand leak persists on every existing install, in the most user-visible location.
.aube-side-effects-cache markers in the CAS store and in node_modules No longer read, and no longer skipped by the directory-hash walk, so the recorded input hash can never match again. The side-effects cache fully invalidates once, silently, and the branded marker file is never removed.
aube-git-* / aube-codeload-* entries under <cache>/nub/pm/git/ Unreachable by the new prefix. Re-cloned on next use; the old copies are dead disk.

None of these is a correctness bug — the caches are self-healing. The question is whether shipping a brand-boundary fix that leaves the branded directory in place is the intended end state.

Technical details
# No migration path for previously-written branded paths

## Affected sites
- `crates/nub-cli/src/main.rs`:25-35 — the fix point for the node-gyp leak; nothing removes the pre-fix `<cache>/aube` tree.
- `vendor/aube/crates/aube/src/commands/install/side_effects_cache.rs`:269, :291 — read/write of the marker now use `side_effects_cache_marker()`.
- `vendor/aube/crates/aube/src/commands/install/side_effects_cache.rs`:324 — `hash_dir_inner` skips only the *current* marker name, so a leftover `.aube-side-effects-cache` in a package dir is now folded into the hash.
- `vendor/aube/crates/aube-store/src/git.rs`:336, :517, :661 — git/codeload cache entries keyed by the new prefix; no sweep removes entries under the old one.

## Required outcome
- A decision, recorded somewhere durable, on whether pre-existing branded paths are cleaned up or deliberately left to rot.
- If cleaned up: the `<cache>/aube` removal is the one that actually matters for the stated invariant, since it is the path a user can see in their own cache directory.

## Suggested approach
A one-shot, best-effort `remove_dir_all` of the legacy `$XDG_CACHE_HOME/aube` (and the Windows/`$HOME` equivalents) guarded so it only fires when the directory exists, is the narrow version. The side-effects markers are harder — they live inside package trees, so removal would have to ride the existing marker read path (delete the old name when the new one is absent) rather than a standalone sweep. The orphaned git-cache entries are probably not worth special-casing.

## Open questions for the human
- Is legacy-path cleanup in scope for this PR, a follow-up issue, or explicitly out of scope?
- Is a one-time silent side-effects-cache invalidation on upgrade acceptable, or should the old marker name still be honored for one release?

ℹ️ Nitpicks

  • vendor/aube/crates/aube/src/commands/install/git_prepare.rs:7,57 and vendor/aube/crates/aube-store/src/git.rs:292,545-546 still document the literal names /tmp/aube-git-*, aube-git-, and aube-codeload-, which are no longer the spelling under an embedder.
  • The new test's fixture tag is "aube-path-brand", so the fixture root's own directory name contains aube. It only passes because collect_brand_crossed_paths never inspects the root's file name — a brand-neutral tag would remove the landmine.
  • is_allowed()'s vendor/aube/crates/aube/src/argv.rs:* entry exempts every statement in the file rather than the one argv[0] literal the comment justifies.

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

Comment on lines +105 to +113
{
line = $0
sub(/^[[:space:]]+/, "", line)
if (line ~ /^(\/\/|\*)/) next
if (start == 0) start = FNR
buf = buf " " line
bal += gsub(/\(/, "(") - gsub(/\)/, ")")
if (bal <= 0) { print f ":" start ":" buf; buf = ""; start = 0; bal = 0 }
}

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 paren-balance joiner counts parens inside char and string literals, so one unmatched paren latches bal positive for the rest of the file and the END block drops the unflushed buffer — the tail is never emitted, so the gate cannot see a violation there.

I measured it: 14 of the 415 scanned files desync, ~7.4k lines total never reaching the matcher, including vendor/aube/crates/aube/src/commands/install/fetch.rs (a file this PR fixes) and crates/nub-cli/src/pm_engine/present.rs. This is the same "gate that passes by not looking" failure the comment above says the brace-counted test skip was written to avoid.

Technical details
# Statement joiner desyncs on parens inside literals and silently drops file tails

## Affected sites
- `tests/brand-lint/check-path-literals.sh`:111 — `bal += gsub(/\(/, "(") - gsub(/\)/, ")")` counts every paren character, including ones inside `'('`, `")"`, and `format!` templates.
- `tests/brand-lint/check-path-literals.sh`:112 — the buffer is printed only when `bal <= 0`; there is no `END` flush, so a latched-positive `bal` discards everything after the desync point.
- Desync triggers confirmed by reading the source: `vendor/aube/crates/aube-lockfile/src/pnpm/dep_path.rs`:15 (`value.split('(')`), `vendor/aube/crates/aube-lockfile/src/pnpm/write.rs`:44-46 (`bare.find('(')` and `"{}(patch_hash={hash}){}"`).

## Measured impact

Instrumenting the same awk program to report an unflushed buffer at EOF, over the exact file set the gate scans (415 files):

| file | first unscanned line | lines swallowed |
| --- | --- | --- |
| `vendor/aube/crates/aube-lockfile/src/pnpm/write.rs` | 44 | 1294 |
| `vendor/aube/crates/aube-resolver/src/peer_context.rs` | 839 | 1248 |
| `vendor/aube/crates/aube-lockfile/src/pnpm/read.rs` | 165 | 1034 |
| `vendor/aube/crates/aube-lockfile/src/source.rs` | 376 | 1034 |
| `vendor/aube/crates/aube-lockfile/src/io.rs` | 750 | 542 |
| `crates/nub-cli/src/pm_engine/present.rs` | 138 | 418 |
| `vendor/aube/crates/aube/src/commands/peers.rs` | 158 | 328 |
| `crates/nub-cli/src/pm_engine/vite_compat.rs` | 341 | 310 |
| `vendor/aube/crates/aube-lockfile/src/pnpm/dep_path.rs` | 15 | 303 |
| `vendor/aube/crates/aube/src/commands/install/fetch.rs` | 1231 | 259 |
| `vendor/aube/crates/aube-resolver/src/resolve/driver.rs` | 2499 | 219 |
| `vendor/aube/crates/aube-lockfile/src/merge.rs` | 462 | 183 |
| `vendor/aube/crates/aube-codes/src/warnings.rs` | 544 | 80 |
| `vendor/aube/crates/aube-lockfile/src/npm/layout.rs` | 238 | 47 |

Reproduction — appending a planted violation past the desync point and re-running the gate:

```console
$ printf '\nfn __planted() { let _ = std::path::PathBuf::from("/x").join(".aube-planted"); }\n' \
    >> vendor/aube/crates/aube-lockfile/src/pnpm/write.rs
$ tests/brand-lint/check-path-literals.sh; echo "exit=$?"
brand-lint: no aube-branded path literals
exit=0
```

The same plant appended to a file that does not desync is reported correctly, so the matcher and the allowlist are fine — only the joiner is broken.

## Required outcome
- Every non-test line of every scanned file reaches the matcher; no file is silently truncated.
- The rustfmt-split-call case the joiner exists for (`join(format!(\n  ".aube-…"`) stays covered.
- A planted violation appended to any scanned file is detected, and it is cheap to re-verify that claim.

## Suggested approach
Two directions, roughly in order of robustness:

1. Strip string and char literals from the line before counting parens, then count on the residue. This fixes the root cause and keeps the joiner. A crude `gsub(/"([^"\\]|\\.)*"/, "")` plus `gsub(/'([^'\\]|\\.)*'/, "")` handles every shape in this tree, but the literal must be matched *before* it is erased, so the matching and the balance-counting need separate copies of the line.
2. Drop paren-balancing entirely and use a fixed-size sliding window — join each line with the previous N (2-3 is enough for the rustfmt split shape) and match on the window. Immune to desync by construction, at the cost of a duplicate report when a hit falls in overlapping windows, which is easy to dedupe on `file:line`.

Either way, add an `END { if (buf != "") print ... }` flush so a residual desync surfaces as a scanned statement rather than a silent drop.

A self-check worth committing alongside: assert the gate flags a planted literal appended to a known-desyncing file, so this regression cannot come back unnoticed.

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 joiner desync is fixed — I re-ran the planted-violation repro on pnpm/write.rs and the gate now reports it — but the new dedup step reintroduces a silent false negative, so leaving this open; details in the new comment on the same file.

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

Comment thread crates/nub-cli/tests/pm_identity.rs Outdated
Comment thread vendor/aube/crates/aube/src/state.rs Outdated
Comment thread vendor/aube/crates/aube/src/commands/install/side_effects_cache.rs Outdated
The gate's paren-balancing joiner could not lex Rust: a paren inside
`split('(')`, a raw string, or a lifetime tick latched the balance positive and
every later statement in the file was dropped. Measured on this tree that
silently skipped ~7.4k lines across 14 of 415 files, including `fetch.rs` —
a file this PR fixes. Erasing string and char literals first recovered 8 of the
14 and still lost 6.

Replaced with a fixed 3-line sliding window, which cannot under-scan by
construction: every line appears in WINDOW consecutive windows whatever the
syntax. Overlapping windows are deduped on file + literal. Verified against
three planted violations: same-line past a test module, the rustfmt-split
multi-line shape, and one appended to a file that previously desynced.

The window reaches code the earlier versions never did, which surfaced
`aube-tool-node-gyp` as the `name` in the scratch manifest written to
`<cache>/pm/tools/node-gyp/v12/package.json` — content rather than a path, but
still nub-written, so it is composed from `prog()` now.

Also from review:
- The `npm_config_node_gyp` assertion was POSIX-shaped. `cache_namespace` is
  the literal "nub/pm", so on Windows the value is `…\xdg-cache\nub/pm\tools\…`
  and `contains("/nub/")` fails. Separators are normalized before comparing.
- The three vendor sites this PR added were not rustfmt-clean. `vendor/aube` is
  its own workspace, so the root `cargo fmt --check` never saw them. Fixed by
  hand; no blanket vendor fmt gate here, because 41 files it does not touch are
  also unformatted.
- The hash walk skipped only the active brand's marker, so a marker left by an
  earlier build would fold into the digest and invalidate every cached entry
  once. It now skips any `.<tool>-side-effects-cache`.
- Doc comments naming `/tmp/aube-git-*`, `aube-git-`, and `aube-codeload-` no
  longer matched the code; updated.
- The test fixture tag no longer contains "aube" (the walk never inspects the
  root's own name, so the old tag was a latent false pass).
- The argv.rs allowlist entry is scoped to the `args[0]` assignment instead of
  exempting the whole file.

Pre-existing branded paths are deliberately left in place. `<cache>/aube` is
standalone aube's real cache directory, so deleting it would destroy another
tool's data on any machine that runs both; the stale git-cache entries are dead
disk that is re-cloned on next use.

@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 paren-balance desync is genuinely fixed — I re-ran the planted-violation repro and the gate now catches it. But the dedup step added alongside it reintroduces a silent false negative in the same gate: a real violation sharing a literal with an allowlisted window in the same file is dropped, and the gate exits 0.

Reviewed changes — the delta since the prior review reworks both new guards and cleans up the vendored-engine formatting, in response to that review's findings.

  • Replaced the statement joiner with a fixed sliding windowcheck-path-literals.sh now emits every run of WINDOW=3 consecutive production lines instead of paren-balanced statements, so no Rust construct can shrink coverage; a mktemp scratch file holds the scan and a new awk stage dedups overlapping window hits.
  • Narrowed the argv.rs allowlist entry and added two more — the whole-file exemption became *args[0]*, joined by self_install.rs:*exe_name* and recursive.rs:*vec![*, both needed because the wider window now joins adjacent statements.
  • Normalized separators in the runtime probenub_never_writes_an_aube_branded_path compares against a backslash-normalized copy, and the fixture tag is now brand-path rather than carrying the brand itself.
  • Made the side-effects marker skip brand-agnosticis_side_effects_marker_name matches .<tool>-side-effects-cache under any brand, which both removes the per-entry format! from the hash walk and stops a previous build's marker from invalidating every cached entry once.
  • Brought the PR's vendored-engine edits to rustfmtstate.rs, self_install.rs, and settings_context.rs are clean; I re-checked all 20 touched files and only the pre-existing aube-store/src/lib.rs:292 still differs.
  • Brand-scoped the node-gyp scratch manifest name — the generated package.json name is now {tool}-tool-node-gyp; not a path, but consistent with the rest of the sweep.

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

Comment on lines +160 to +168
| awk -F: '
# Overlapping windows re-report one violation. Key on file + the brand
# literal itself and keep the lowest line number.
{ rest = $0; sub(/^[^:]+:[0-9]+:/, "", rest)
if (match(rest, /"[^",()[:space:]]*aube[^",()[:space:]]*"/)) {
key = $1 "|" substr(rest, RSTART, RLENGTH)
if (!seen[key]++) print
} else print
}'

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.

⚠️ Deduping before the allowlist filter lets an exempt window absorb a real violation. The key is file|literal and only the lowest-line hit survives, so in argv.rs the allowlisted args[0] window wins and every later hit carrying the literal "aube" is discarded — including a genuine one.

I verified it: appending fn __planted_join(p: &std::path::Path) -> std::path::PathBuf { p.join("aube") } to vendor/aube/crates/aube/src/argv.rs leaves the gate at exit 0. This also undoes the narrowing of that allowlist entry in this same commit, which is otherwise a real improvement.

Technical details
# Dedup runs before `is_allowed`, so an allowlisted hit suppresses real ones sharing its literal

## Affected sites
- `tests/brand-lint/check-path-literals.sh`:160-168 — the dedup awk sits inside the producer pipeline, upstream of the `is_allowed` loop at :152-156.
- Every file with an `is_allowed()` entry is exposed: `argv.rs` (literal `"aube"` — the broadest possible key), `patch.rs`, `self_install.rs`, `aube-scripts/src/lib.rs`, `settings.rs`, `recursive.rs`.

## Reproduction

```console
$ printf '\nfn __planted_join(p: &std::path::Path) -> std::path::PathBuf { p.join("aube") }\n' \
    >> vendor/aube/crates/aube/src/argv.rs
$ tests/brand-lint/check-path-literals.sh; echo "exit=$?"
brand-lint: no aube-branded path literals
exit=0
```

Deleting only the dedup stage and re-running reports the plant three times (once per overlapping window), which isolates the dedup as the cause rather than the window or the matcher.

## Required outcome
- A planted violation in a file that has an allowlist entry is reported, even when it shares a literal with the exempt hit.
- Overlapping windows still collapse to one report per real violation.
- The clean tree still exits 0.

## Suggested approach
Run the dedup on the allowlist-filtered stream instead of the raw one — collect non-exempt hits first, then pipe those through the existing awk. I confirmed this ordering keeps the clean tree green and reports the `argv.rs` plant.

A second, smaller issue in the same block: the two new allowlist entries key on `*exe_name*` and `*vec![*`, which are common enough tokens that any future violation landing within `WINDOW` lines of one is exempt. Keying on the brand literal itself (as the `patch.rs` and `aube-scripts` entries do) is tighter.

## Open questions for the human
- Is a committed self-check worth adding here — the plant-and-expect-failure loop is what caught both this and the previous desync, and it is three lines of bash.

# sentence reaching a `Vec::push` or a `slice.join(", ")`, and no path literal
# in this tree contains a space.
BRAND_LITERAL='"[^",()[:space:]]*aube[^",()[:space:]]*"'
PATH_CALL='join\(|push\(|prefix\(|with_file_name\(|set_file_name\(|create_dir|PathBuf::from|OsString::from|tempdir_in'

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.

ℹ️ Because a brand literal and a path call must co-occur inside one 3-line window, a literal bound to a const/static and joined at a distant call site is invisible — which is exactly the shape of the two consts this PR removes (SIDE_EFFECTS_CACHE_MARKER, CACHE_DIR_NAME). I confirmed a planted const __MARKER: &str = ".aube-side-effects-cache"; plus a separate p.join(__MARKER) goes unreported.

There are only six const-bound brand literals in the scanned tree and none is a path leaf (present.rs:292, self_install.rs:25,40,44, update_check.rs:18, ticket_cache.rs:43), so treating a const/static binding as a path call would cost a handful of allowlist entries or a :// exclusion.

Six vendor files go back to upstream verbatim. Their aube-branded names cannot
be reached under nub, so composing them from `prog()` bought nothing and only
widened the delta an upstream merge has to reconcile:

- self_install.rs — aube's self-update versions dir; `self_update_enabled` is
  false in the NUB profile and every `self_dir()` caller is self-update code.
- fs_atomic.rs, unlink.rs — fallbacks taken only when the path has no file
  name; every caller passes a real file.
- state.rs, settings_context.rs — store-dir fallbacks taken only when neither
  XDG nor HOME resolves.
- aube-store/lib.rs — `Store::at`'s sibling cache dir, which never appeared on
  disk across months of real use.

Each is now an allowlist entry in the path-literal gate carrying its
reachability claim, so the gate still fails if one becomes reachable and
someone re-adds a hardcoded name.

startup.rs is deliberately KEPT: `diag_env_prefix` is `Some("NUB")`, so the
diagnostics layer is live under nub and the `aube-diag.jsonl` fallback really
is reachable.

Vendor delta: 18 files / 121 lines -> 12 files / 84 lines.

Differential on the same workload (isolated HOME/XDG, install esbuild@0.24.2
with its postinstall, then a script run):
  shipped v0.5.0 -> <root>/cache/aube, plus .aube-side-effects-cache in both
                    the side-effects cache and the CAS store
  this branch    -> no aube-named path anywhere, and .nub-side-effects-cache
                    present, which proves the guarded code ran rather than
                    being skipped

@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 latest commit trades five vendored fixes for allowlist entries under an UNREACHABLE UNDER NUB header. Four of those claims hold — I traced each call graph. Two do not: the $TMPDIR/aube store and cache fallbacks are reachable from nub install and do write to disk.

Reviewed changes — the delta since the prior review shrinks the vendored diff by reverting six brand-scoping edits and replacing five of them with reachability claims in the static gate.

  • Reverted the self_dir() and sibling_tempdir() brand-scopingself_install.rs keeps aube/self and fs_atomic.rs keeps aube-tmp; both are genuinely unreachable under nub (aube's own CLI dispatch is dead, and every atomic_write caller passes a real file name).
  • Reverted the $TMPDIR store/cache fallbacks in settings_context.rs and state.rs — the no-HOME/no-XDG fallbacks are back to std::env::temp_dir().join("aube").
  • Reverted the unlink.rs virtual-store leaf and the aube-store cache_dir_name() helper.aube and the CACHE_DIR_NAME const are restored; the const needed no allowlist entry because the gate cannot see const-bound literals.
  • Added five is_allowed() entries with per-entry reachability justifications — grouped under a shared comment block explaining the delta-shrinking rationale and the drop-the-entry-if-it-becomes-reachable rule.

ℹ️ The aube-cache revert is the one reachability claim recorded nowhere

Five of the six reverted sites carry a justification in is_allowed(). The sixth — aube-store's CACHE_DIR_NAME const, restored to aube-cache — carries none, because the gate's matcher requires a brand literal and a path call inside one window and so cannot see a const binding at all. The claim is therefore load-bearing but unwritten and unlinted.

I confirmed it holds today: Store::at skips the legacy-index migration, index_dir() derives from the store root rather than the cache dir, and nub's only production Store::at caller calls just load_index. What makes it fragile is that the invariant is "nobody calls virtual_store_dir() or packument_cache_dir() on a Store::at handle" — a condition no test and no gate expresses.

Technical details
# `CACHE_DIR_NAME` revert has no recorded or enforceable reachability claim

## Affected sites
- `vendor/aube/crates/aube-store/src/lib.rs`:55 — `pub const CACHE_DIR_NAME: &str = "aube-cache";` restored.
- `vendor/aube/crates/aube-store/src/lib.rs`:142 — `Store::at` derives `cache_dir = root.parent()/CACHE_DIR_NAME`.
- `crates/nub-cli/src/pm_engine/phantom_closure.rs`:386 — nub's only production `Store::at` caller; invokes only `load_index`, which routes through `store_v1_dir()`.
- `vendor/aube/crates/aube-store/src/lib.rs`:204, :289, :321, :330 — the four readers of `cache_dir` (`legacy_index_dir`, `virtual_store_dir`, `packument_cache_dir`, `packument_full_cache_dir`); reaching any of them from a `Store::at` handle would materialize `<store>/aube-cache`.
- `tests/brand-lint/check-path-literals.sh`:41-42 — `BRAND_LITERAL` + `PATH_CALL` must co-occur in one window, so a `const` binding never matches.

## Required outcome
- The reachability claim for `aube-cache` is recorded where the other five live, or the site is pinned so a future `cache_dir` read on a `Store::at` handle cannot silently plant a branded directory.

## Suggested approach
Either add an `is_allowed()` entry for `aube-store/src/lib.rs:*"aube-cache"*` documenting the `Store::at`-only reachability (harmless today, becomes real coverage once the matcher treats a `const`/`static` binding as a path call — the open thread on line 42), or restore the `cache_dir_name()` helper, which costs one vendored function and removes the invariant entirely.

## Open questions for the human
- Is "reverted, unclaimed, invisible to the gate" acceptable for this one site, given the stated policy that every retained brand literal is an explicit reachability claim?

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

Comment on lines +86 to +87
vendor/aube/crates/aube/src/commands/settings_context.rs:*temp_dir*join*\"aube\"*) return 0 ;;
vendor/aube/crates/aube/src/state.rs:*temp_dir*join*\"aube\"*) return 0 ;;

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.

⚠️ These two are reachable, not unreachable. When XDG_DATA_HOME/XDG_CACHE_HOME are unset and $HOME doesn't resolve, nub pushes no storeDir default (nub_data_dir() returns None) and never pushes a cacheDir default, so dirs::store_dir()/dirs::cache_dir() return None and nub install writes CAS blobs to $TMPDIR/aube/store/v1/files, packuments to $TMPDIR/aube/packuments-v1, and no-integrity bindings to $TMPDIR/aube/store/v1/no-integrity. The literal bypasses embedder() entirely, so early identity registration doesn't cover it.

Technical details
# Two allowlisted `$TMPDIR/aube` fallbacks are reachable from `nub install`

## Affected sites
- `tests/brand-lint/check-path-literals.sh`:86-87 — the two entries, and the `UNREACHABLE UNDER NUB` header at :71 that scopes them.
- `vendor/aube/crates/aube/src/commands/settings_context.rs`:261 — `open_store`'s store-root fallback, `temp_dir().join("aube").join("store/v1/files")`.
- `vendor/aube/crates/aube/src/commands/settings_context.rs`:500 — `resolved_cache_dir`'s platform default, `temp_dir().join("aube")`; feeds `packument_cache_dir` / `packument_full_cache_dir` at :680-695.
- `vendor/aube/crates/aube/src/state.rs`:1016 — `no_integrity_dir`'s fallback, `temp_dir().join("aube").join("store").join("v1")`.

## Reachability
- `open_store``install/mod.rs:1198`, `install/fetch.rs:116`, `install/lifecycle.rs:284`, `rebuild.rs:148`, `ignored_builds.rs:180`, `store.rs`, `find_hash.rs:81`, `cat_index.rs:47`, `cat_file.rs:28` — all under verbs nub routes through `pm_engine::ENGINE_VERBS`.
- `no_integrity_dir``install/mod.rs:405`, `install/mod.rs:1843`, `install/fetch.rs:1115`. `write_no_integrity_bindings` (`state.rs`:1061) does `create_dir_all` then writes per-URL files, so this is a real write, not just a computed `PathBuf`.
- Condition: `dirs::store_dir()` / `dirs::cache_dir()` (`aube-store/src/dirs.rs`) return `None` only when the XDG var is unset AND `aube_util::env::home_dir()` (`$HOME`, then `$USERPROFILE` on Windows) yields nothing.
- Nub does not shadow the fallback in that state: `pm_engine/mod.rs`:2294-2298 pushes `storeDir` only `if let Some(data) = nub_data_dir()`, and the doc comment at :2041-2043 already concedes "Skipped when no home directory resolves — the engine then falls back to its own default, which fails the same way nub would." `cacheDir` is never pushed (`mod.rs`:3237 asserts it is `None`).
- The upstream comment on `open_store` (`settings_context.rs`:246-253) names the environment explicitly: "an install with HOME stripped from the env (e.g. pnpm's own test harness, or a minimal CI container)." An arbitrary-UID container with no passwd entry is the common real-world shape.
- `nub_never_writes_an_aube_branded_path` cannot catch this — it sets `HOME`, `XDG_CONFIG_HOME`, `XDG_DATA_HOME`, and `XDG_CACHE_HOME`, and probes `nub run`, which never reaches `open_store`.

## Required outcome
- With `HOME`, `XDG_DATA_HOME`, and `XDG_CACHE_HOME` all unset, `nub install` creates no path containing `aube`.
- The `UNREACHABLE UNDER NUB` block contains only entries whose paths cannot be reached, so the header stays a proof rather than a likelihood estimate.

## Suggested approach
Restore the `aube_util::prog()` composition at the three sites (exactly what `4fe02997` had) and drop the two entries. That is +3 lines of vendored delta against a leak in the one environment where the fallback exists to be used. The remaining three entries (`self_install.rs`, `fs_atomic.rs`, `unlink.rs`) hold up: aube's CLI dispatch is dead under nub, every `atomic_write` caller passes a real file name, and `unlink.rs`'s `.aube` is only ever compared, never written.

If the entries are kept instead, the header needs to say what it actually means — the paths are reachable but the environment is judged acceptable — since "unreachable" is what a future reader will audit against.

## Open questions for the human
- Is a HOME-less, XDG-less container in scope for the brand-boundary invariant, or explicitly out of scope? The answer decides between restoring the composition and rewording the header.

# neither XDG nor HOME resolves.
# unlink.rs — virtual-store leaf fallback, only when the resolved
# store path has no file name.
vendor/aube/crates/aube-runtime/src/self_install.rs:*\"aube/self\"*) return 0 ;;

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 five entries added here widen the open dedup-before-allowlist finding from 6 exposed files to 11, and two of them key on the bare literal "aube" — the broadest possible dedup key. Verified on this tree: appending p.join("aube") to settings_context.rs exits 0, same in state.rs, and p.join("aube-tmp") in fs_atomic.rs exits 0. Reordering the dedup to run on the allowlist-filtered stream fixes all three; I re-confirmed the clean tree still exits 0 under that ordering (0 survivors across all 53 raw hits).

@colinhacks
colinhacks merged commit e910aac into main Jul 29, 2026
65 checks passed
@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