Skip to content

fix(install): detect cgroup-v1 PID limits inside containers; repair remote-build - #640

Merged
colinhacks merged 16 commits into
mainfrom
megapr-container-limits
Aug 4, 2026
Merged

fix(install): detect cgroup-v1 PID limits inside containers; repair remote-build#640
colinhacks merged 16 commits into
mainfrom
megapr-container-limits

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

cgroup-v1 PID limits go undetected in containers. cgroup_v1_pids_max joined the pids mount point with the host-absolute path from /proc/self/cgroup, but that mount is already rooted at our cgroup, so the read failed open and --pids-limit 64 got a 128-thread pool. Now strips the mountinfo root, longest-root first. v2 unaffected: Docker uses cgroupns=private there.

Verified on Ubuntu 20.04/Docker and Rocky 8/podman vs the shipped release and a merge-base build; 32 cgroup-v2 cells byte-identical.

remote-build never completed a job: it set AUBE_REQUIRE_PRIMER=1 (ci.yml does not; the primer is gitignored) and ran clippy without --profile fast. Adds --detach/--attach.

Colin McDonnell added 3 commits July 30, 2026 16:28
`cgroup_v1_pids_max` joined the mount point with the cgroup path read from
/proc/self/cgroup. Inside a container on a cgroup-v1 host that path is
host-absolute (`/docker/<id>`) while /sys/fs/cgroup/pids is a bind mount already
rooted at that same cgroup, so the join produced a path that does not exist, the
read failed open, and the box was reported unconstrained. With `--pids-limit 64`
nub then sized a 128-thread blocking pool inside a 64-PID container — the
`clone(2)` EAGAIN exhaustion this module exists to prevent.

Resolve the path the way num_cpus already does for the cpu controller (which is
why the CPU axis never had this bug): strip the mountinfo mount root off the
cgroup path and join the remainder onto the mount point. Where several `pids`
mounts are visible, select by longest matching root rather than by list order,
so the choice is not a positional accident.

Docker defaults to `--cgroupns=host` on v1 and `private` on v2, so v2 always
resolved correctly and a v2-only test could not have caught this.

Verified on a real cgroup-v1 host (Ubuntu 20.04, Docker 26.1.3) against builds
of both this branch and its merge-base, so the diff is the only variable:
`--pids-limit 64` and `128` go from unconstrained to correctly capped, while
--cgroupns=private, host systemd scopes, unconstrained, and --pids-limit 8192
are unchanged. strace confirms the openat moves from ENOENT on the joined path
to success on the mount point. A nested-cgroup cell (root limit 512, nested 100)
distinguishes this from the rejected mount-root fallback: it reads 100.

Also corrects two claims in the CPU-budget comments that measurement refuted:
std reads the cgroup v1 quota, and it re-applies the quota when
sched_getaffinity is unreadable rather than dropping it. cpu_budget() is
therefore near-unfireable; behavior is unchanged pending a separate decision.
…py invocation

Three defects, each of which prevented this tool from completing a gate.

1. A foreground run is SIGKILLed at the driving harness's timeout (two minutes
   by default, ten at most). SIGKILL cannot be caught, so the SIGINT/SIGTERM/
   SIGHUP handlers and the `finally` never run and the builder leaks until the
   server-side TTL reclaims it. A cold clippy plus spot-stockout failover does
   not fit that ceiling, so the local process must not have to outlive the job:
   `--detach` starts it under setsid on the VM and returns, `--attach` streams
   and polls in a bounded window and is safe to re-run until it completes.

2. The job exported AUBE_REQUIRE_PRIMER=1, which ci.yml never sets. That guard
   protects a shipped binary from an empty primer; a lint gate ships nothing,
   and the requirement is unsatisfiable here because the primer JSON is
   gitignored (so the `git ls-files`-driven sync cannot carry it) and
   regenerating it needs the networked registry crawl only release.yml runs.
   Every remote job died in aube-resolver's build.rs as a result.

3. Clippy ran without --profile fast while ci.yml uses it, so the run drove a
   second full dependency build under dev and could not reuse the golden
   image's warm artifacts.

Verified end-to-end: a detached clippy run on the branch reports rc=0, the
attach resumes correctly after being killed mid-job, and the VM is deleted on
completion.

Backgrounding note, since the obvious spelling is wrong: `a && b && setsid … &`
backgrounds the entire && chain, whose earlier commands inherit the ssh
session's stdout, so the channel never reaches EOF and ssh blocks even though
the job started. Only the fully redirected setsid may be backgrounded.
…est match

Adversarial testing on Ubuntu 20.04 and Rocky 8 found that the previous commit
was not, as claimed, strictly no worse than its predecessor. It resolved a
single path — the mount whose root is the longest prefix of our cgroup path —
and read it with `.ok()?`. When that mount happens to be unreadable, detection
was lost entirely even though a shorter-matching mount resolves the same cgroup
and would have answered.

Two topologies reproduced it identically on both distros: the best-matching
mount shadowed by a later overmount, and the same mount at mode 0700 under an
unprivileged process. In both, the pre-fix code read the limit (100) and the new
code reported unconstrained. That is the fail-open direction, which is the one
failure mode this module exists to prevent. The existing strip_prefix fallback
could not help: it fires when the strip fails, and there the strip succeeds and
the read fails.

Return an ordered candidate list instead — longest matching root first, the
pre-fix location appended last — and fall through only on a failed READ. A
successful read of the literal `max` stops the walk, because that is a real
answer ("this cgroup has no limit"); consulting a further mount there would
report a different cgroup's limit.

This does not reintroduce the rejected bare-mountpoint fallback. Every candidate
still resolves this process's own cgroup as seen through a different mount, so
the root-cgroup masquerade that the v2 probe refuses cannot occur here.

Neither topology was observed in the wild: Docker in both namespace modes,
rootful and rootless podman, host systemd scopes, and nested runtimes all expose
either one pids mount or several that agree. Both were constructed. Fixing it
anyway, because a known fail-open path in a fail-closed detector is not worth
carrying.

Also pins the component-wise prefix requirement in a test: a mount rooted
/tst/ab is a longer string prefix of /tst/abc than /tst is, so a str::starts_with
implementation would select it and resolve the wrong cgroup. Path::starts_with
compares components, which excludes it.
Copilot AI review requested due to automatic review settings July 31, 2026 02:05
@vercel

vercel Bot commented Jul 31, 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 Aug 4, 2026 12:19am

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 cgroup-v1 resolver holds up under scrutiny, but the new --detach path drops the try/finally that oneBuild and buildImage both use, so a failed detach leaks a VM for the full 45m TTL. Separately, the remote-build skill doc now contradicts the tool in two places and will route agents straight back to the broken foreground path.

Reviewed changes — the full diff at ddb104a (2 files, 3 commits), plus the surrounding resource_limits.rs module, .github/workflows/ci.yml, vendor/aube/.gitignore, and .claude/skills/remote-build/SKILL.md.

  • cgroup-v1 pids path resolutioncgroup_v1_pids_max no longer joins the mount point with the host-absolute path from /proc/self/cgroup. A new pure cgroup_v1_pids_candidates strips the mountinfo root (field 4) and joins the remainder onto the mount point (field 5), ordered longest-root-first, with the pre-fix location retained last.
  • Candidate walk instead of a single answercgroup_v1_pids_max tries each candidate and returns on the first readable one, so an unreadable longest match no longer loses detection outright.
  • Seven unit tests on the pure resolver — container root-strip, host nested path, unusable mountinfo, longest-root ordering, retry past an unreadable match, a component-wise-prefix decoy, and the pure-v2 no-op.
  • cpu_budget doc rewrite (comment only) — retracts the two "gaps in std" the gate was written to close and records that it is effectively unfireable.
  • AUBE_REQUIRE_PRIMER=1 dropped from PREPARE — the primer JSON is gitignored so the git ls-files-driven sync can never carry it, making the gate unsatisfiable; ci.yml never sets it.
  • --profile fast added to both clippy invocations — matches .github/workflows/ci.yml exactly.
  • New --detach / --attach modes — start the job under setsid on the VM and collect it in a separate, re-runnable invocation, so the local process no longer has to outlive a job that exceeds the harness timeout.

The cgroup change is the riskier half and it survived a dedicated topology sweep: Docker cgroupns host and private, privileged full-hierarchy mounts, rootless podman, LXC/LXD, Kubernetes v1 kubepods, systemd hybrid, docker-in-docker, and a second bind mount of the host cgroupfs all resolve to the calling process's own cgroup. Failures are always "missing", never "wrong". The cgroup_namespaces(7) un-remounted-mount anomaly (root=/..) is filtered out by the component-wise prefix check rather than silently accepted, and the mountinfo field indices match proc_pid_mountinfo(5). The tests all fail with the bug present, and the empty-rel join the container test depends on was confirmed against real PathBuf semantics.

⚠️ The remote-build skill still documents the two behaviors this PR reverses

.claude/skills/remote-build/SKILL.md tells agents the job script exports AUBE_REQUIRE_PRIMER=1 (line 65) and asserts that builds "run in the ssh foreground and are never detached", with no mention of --detach/--attach. AGENTS.md makes the skill the authoritative playbook an agent reads before using the tool, and this PR's own rationale is that the foreground form gets SIGKILLed at the harness cap — so the doc will keep sending agents down the path the PR exists to replace.

Technical details
# `remote-build` skill and file header contradict the tool after this PR

## Affected sites
- `.claude/skills/remote-build/SKILL.md:65` — states the job script exports `AUBE_REQUIRE_PRIMER=1`; `PREPARE` now deliberately does not.
- `.claude/skills/remote-build/SKILL.md` (orphan-proofing section, near the `--reap` paragraph) — "Builds run in the ssh **foreground** and are never detached. A detached build reparents to PID 1, outlives its launcher, holds locks, and is not reaped — see `rust-build-hygiene`." Directly contradicted by `--detach`.
- `scripts/remote-build.ts:48-49` (header, outside the diff) — the same absolute claim, now false within its own file. The new `DETACHED MODE` block at line 483 explains why detaching to a disposable VM is not the hazard a detached LOCAL build is; the header needs the same carve-out.

## Required outcome
- The skill documents `--detach` / `--attach` as the way to drive the tool from an agent harness, including the `EX_STILL_RUNNING` re-run contract.
- Neither the skill nor the file header claims the job script sets `AUBE_REQUIRE_PRIMER`.
- The "never detached" rule is narrowed to detached LOCAL builds, which is the hazard `rust-build-hygiene` actually covers.

ℹ️ cpu_budget is now documented as effectively dead but stays wired in

The rewritten doc comment retracts both gaps the gate was built to close and records that it returned None in every constrained cell of a ~44-cell v2 sweep and all 9 v1 cells, leaving "Keep/reshape/remove is a live design question". That is honest and worth landing, but it leaves live code whose stated justification the same diff refutes, with nothing tracking the decision. Only the maintainer can close it, given the Embedder::cpu_budget hook.

Technical details
# `cpu_budget` retains a gate its own docs say cannot fire

## Affected sites
- `crates/nub-cli/src/pm_engine/resource_limits.rs:191-206` — the MEASURED note; the only case that can still fire is cgroup v2 at a non-standard mountpoint.
- `crates/nub-cli/src/pm_engine/resource_limits.rs:214-233``cpu_budget()`, still called and still exposed through the embedder hook.

## Required outcome
- A decision, or an issue tracking one: keep as the non-standard-mountpoint fallback, narrow it to that case, or remove it along with `cpu_budget_from` and the `Embedder::cpu_budget` hook.

## Open questions for the human
- Is cgroup v2 at a non-standard mountpoint a topology nub needs to cover, or is it acceptable to inherit std's behavior there?

ℹ️ Nitpicks

  • --fanout N is silently ignored under --detach: startDetached always creates exactly one VM and hardcodes the -1- index in the instance name.
  • The new "Driving this from an agent harness?" paragraph in HELP is inserted between --reap-all and -h, --help, so --help output breaks the option list in half and then resumes it.
  • The in-code comment at line 327 says the walk falls through "ONLY on a failed READ", but a readable candidate whose contents parse_pids_max cannot parse also ends the search. Unreachable in practice for a real pids.max; the comment is just narrower than the behavior.

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

Comment thread scripts/remote-build.ts
Comment thread scripts/remote-build.ts
Comment thread scripts/remote-build.ts Outdated
Comment thread scripts/remote-build.ts Outdated
…th resolution

Records that strip-mount-root-then-join is runc's behaviour
(opencontainers/cgroups/v1_utils.go), not an invention here, and notes the two
deliberate differences: longest-matching root with retry rather than first match,
and a component-wise prefix test that rejects the ../-traversing candidates
filepath.Rel would produce.
…ctually run

The bake warmed `cargo build -p nub-cli --profile fast` while the clippy job runs
`cargo clippy --all-targets --all-features --profile fast`. Cargo fingerprints on
the command shape, so those artifacts were unusable for four independent reasons:
build and clippy are different drivers, one package is not the whole workspace,
and --all-features changes feature unification. Only `cargo fetch` carried over,
so every remote clippy recompiled the graph from scratch.

Warm with the real invocations instead, and add `cargo test --workspace --no-run`
for the test job, which runs on the default profile and so occupies a separate
target directory from `fast`.

The parity test asserted the clippy invocation WITHOUT --profile fast while its
comment claimed it was verified against ci.yml, which is how the drift survived.
Corrected, so the test now pins what CI runs.

Also anchors the test job's "does not run clippy" assertion to a command rather
than the raw script text: PREPARE is shared by both jobs and its comments
legitimately reference the other gate.

Drops an orphaned comment describing an aarch64-apple-darwin cross-build as the
bake's zig/SDK smoke test. That build was moved to scripts/mac-build.ts on a real
macOS runner, per this file's header, so the note documented a command that was
not present and contradicted the best-effort handling directly beneath it.

@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 bake rewrite fixes the clippy job's warmth but drops (cd crates/nub-native && cargo build) — the one warm-up line that already matched a jobScript invocation verbatim — so the test job now cold-compiles the addon's oxc/napi graph. The four inline threads from the previous review are also still open, including the startDetached VM leak.

Reviewed changes — the delta since the prior pullfrog review at ddb104a, i.e. 79e24a30 alone (resource_limits.rs is untouched since then and was verified in the earlier pass).

  • Rewrote the golden-image bake's warm-up block — replaced cargo build -p nub-cli --profile fast and (cd crates/nub-native && cargo build) with the two --profile fast clippy invocations plus cargo test --workspace --no-run, so the baked artifacts share cargo's fingerprint with what the jobs actually run.
  • Removed a stale "NOT best-effort" note describing an aarch64-apple-darwin cross-build that no longer exists in the bake; the .darwin-stubs copy is retained.
  • Updated the clippy job test to assert --profile fast on both legs, matching the invocation jobScript now emits.
  • Re-anchored the test job's negative clippy assertion from /clippy/ to /\ncargo clippy/, since the reworded PREPARE comment block now contains the word "clippy".

ℹ️ Nothing enforces the bake ↔ jobScript lockstep that this commit relies on

The warm block is an inline string inside buildImage, so no test can reach it, and the only mechanism keeping it aligned with jobScript is the new comment "Keep these lines in lockstep with jobScript() or the image silently goes cold again." That drift is what the commit exists to repair, and it recurred in the same commit — a comment is not holding this invariant.

Technical details
# The bake's warm commands and `jobScript`'s commands can drift undetected

## Affected sites
- `scripts/remote-build.ts:786-798` — the warm block, an inline template literal in `buildImage`, unreachable from `scripts/remote-build.test.mjs`.
- `scripts/remote-build.ts:425-445``jobScript`, which IS exported and heavily tested.

## Required outcome
- A mechanical guarantee that every cargo invocation `jobScript` emits for a given job is also emitted by the bake, so a future edit to one side fails a test rather than silently publishing a cold image.

## Suggested approach (optional)
- Export the per-job cargo command list (or the warm block itself) and have `buildImage` compose its warm string from the same source `jobScript` uses, then assert the correspondence in `remote-build.test.mjs` the way the existing `CARGO_TARGET_DIR` and cargo-env invariants are asserted.

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

Comment thread scripts/remote-build.ts
Comment thread scripts/remote-build.test.mjs Outdated
The seam printed only derived pool sizes, and every one of them saturates its own
cap: `blocking = min(128, max(h/3, 4))`, while workers and rayon additionally clamp
against the core count. So `workers=4 blocking=128 rayon=4` means "no constraint
detected" OR "detected, with headroom at or above 384" — the two are
indistinguishable from the line alone.

That ambiguity is not theoretical. A verification round reading a deliberately
large limit saw the saturated line, took it for lost detection, and only strace
showed the read had in fact succeeded. Two further cells needed strace for the
same reason.

Print the raw Option<usize> from spawn_headroom() alongside the derived numbers so
detection state is legible without a syscall tracer. Diagnostic only; nothing
parses this line, and the sizing itself is unchanged.
Six defects, all introduced by the --detach/--attach commit or the bake rewrite.

startDetached had no try/catch between createInstance succeeding and the job
starting. `live` is drained only by the signal handlers, so any throw in that
window reached main().catch and exited with the VM still running, leaking it for
the full 45m TTL. oneBuild and buildImage both guard the same window. A catch
rather than a finally, since on success the instance must survive.

`--attach` as the final argv element left argv[++i] undefined, so both dispatch
branches were falsy and a collect-my-result invocation fell through to
provisioning a fresh VM for a full cold job. Rejected with exit 2, matching the
guard --machine already carries for the same trap.

The attach stream could lose a line. A poll landing mid-write left a partial last
line, which was printed as complete and counted into `seen`, so the next window
resumed past it and the remainder was never shown. Drop the trailing element
while polling and re-read the line whole next time; drain fully on the terminal
pass, or the last line of every job would vanish.

`--keep` could not reach the attach path's delete, so `--attach <vm> --keep`
destroyed the box the operator asked to preserve for post-mortem debugging.

The bake rewrite dropped `(cd crates/nub-native && cargo build)`, which was the
only warm-up line already matching a jobScript invocation verbatim. nub-native is
an excluded workspace, so `cargo test --workspace --no-run` never reaches it and
the clippy line is both a different profile directory and a different driver;
without it the test job cold-compiles the addon's oxc/napi graph. Restored.

The re-anchored test assertion was too narrow: `\ncargo clippy` only matches at
line start, so the clippy job's own subshell leg would slip through. PREPARE says
"a clippy or test run" and never "cargo clippy", so the unanchored form covers
both shapes.

Also updates the skill doc, which contradicted the tool in two places and would
have routed agents straight back to the foreground path: it described the test
job as `-p nub-cli` when it runs the whole workspace, and stated builds are never
detached without noting that --detach detaches on the remote VM, not locally.

@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 critical issues — one minor suggestion inline, plus a doc-drift note.

Reviewed changes — the delta since the prior pullfrog review at 79e24a3, i.e. a0a0310 alone, which addresses all six open threads from the two earlier reviews. resource_limits.rs and mod.rs are untouched since 1712b14 and were verified in the earlier passes.

  • Restored the bake's addon warm-up(cd crates/nub-native && cargo build) is back, placed after cargo test --workspace --no-run with a note on why --workspace cannot reach an excluded workspace. The bake's cargo invocations now cover every one jobScript emits for both jobs.
  • Guarded the detached provisioning windowstartDetached wraps instanceIp/waitForSsh/syncSource/startDetachedJob in try/catch that deletes the instance and drains live before rethrowing, deliberately a catch rather than a finally so a successful detach still outlives the process.
  • Rejected a valueless --attachparseArgs now exits 2 when --attach appears without a name, closing the fall-through that would have provisioned a fresh VM for a full cold job.
  • Made --keep reach the attach-path deleteattachToJob takes a and gates deleteInstance on !a.keep.
  • Reworked log streaming into a drain(final) closure — the trailing element is dropped unconditionally while polling and printed only on the terminal pass, so a mid-write partial line is re-read whole instead of being lost.
  • Strengthened the test job's negative clippy anchor to /cargo clippy/, which also covers the subshell form the clippy job actually uses.
  • Documented --detach/--attach in the skill and narrowed its "never detached" rule to detached local builds.

I verified the drain arithmetic against six synthetic tail -n +N shapes (empty, trailing newline, no trailing newline, mid-write partial, the re-read-whole window, and the terminal pass): no line is lost or duplicated, and seen never advances past printed content. ssh() returning stdout un-trimmed is load-bearing to that, and it does. EX_STILL_RUNNING = 75 colliding with a genuine job exit code is not reachable for cargo/clippy/rustc or the brand lint, and deleteInstance swallows its own errors so the new catch cannot mask the original throw. On the bake side I also checked the one non-obvious warmth risk — the bake stages an 11-byte placeholder addon while the test job cp's the real .so — and it is a non-issue: the build.rs that hashes addons/nub-native.node sits behind embed-runtime, which is not a default feature, so bare cargo test never reruns on it. node --test scripts/remote-build.test.mjs is 18/18 green.

ℹ️ Two of the three stale-doc sites are still stale

a0a0310 added the --detach/--attach usage block and narrowed the skill's "never detached" rule, but .claude/skills/remote-build/SKILL.md:74 still tells the reader the job script exports AUBE_REQUIRE_PRIMER=1 — the var PREPARE now deliberately omits, and whose removal is half of what this PR fixes. scripts/remote-build.ts:48-49 still carries the unqualified "Builds run in the ssh FOREGROUND and are never detached", which is the exact sentence the skill just narrowed. Neither line is in this diff's hunks, which is why they were missed.

Technical details
# Two doc sites still describe the pre-PR behavior

## Affected sites
- `.claude/skills/remote-build/SKILL.md:74` — "the job script also exports **`AUBE_REQUIRE_PRIMER=1`** — the same var `release.yml` sets to make `build.rs` fail loud instead of degrading." `PREPARE` now carries a `DELIBERATELY NOT SET` block explaining the opposite, and the bullet's framing (three silent-degrade paths, `command -v node` catches only the first) currently ends on a mitigation the tool no longer applies.
- `scripts/remote-build.ts:48-49` — "Builds run in the ssh FOREGROUND and are never detached — a detached build reparents to PID 1, outlives its launcher, and is not reaped by the harness." Now false within its own file; the `DETACHED MODE` block at line 491 already carries the correct carve-out.

## Required outcome
- The skill's silent-degrade bullet reflects that the remote gate relies on `command -v node` alone, and states why `AUBE_REQUIRE_PRIMER` is a release-only concern.
- The file header's "never detached" claim is narrowed to detached LOCAL builds, matching the wording `SKILL.md:108-112` now uses.

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

Comment thread scripts/remote-build.ts Outdated
`seen` was process-local, so the resume it was written for only held inside one
poll loop — and the loop never re-reads a line anyway. Re-running `--attach` is
the normal path rather than the exception, since the attach window is
deliberately shorter than the harness timeout that would otherwise SIGKILL the
call, so a long job takes several invocations. Each one restarted at zero and
`tail -n +1` re-printed the whole transcript, which for the agent harness this
exists for means re-reading thousands of lines into a context window, growing
with every attempt.

Keep the offset in a tmp file keyed by instance name, read it on entry, write it
after each drain, and delete it alongside the instance so it cleans itself up.
The still-running message's line count now means something across calls too.

Also corrects two doc sites this PR made stale, neither of which was in an
earlier diff hunk:

The file header still claimed builds "are never detached", which became false
within its own file once DETACHED MODE landed below it. Narrowed to detached
LOCAL builds, matching the wording the skill already uses.

The skill still told the reader the job script exports AUBE_REQUIRE_PRIMER=1,
the variable PREPARE now deliberately omits and whose removal is half of what
this PR fixes. It now states why the guard is release-only and what setting it
here did.

@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 found. Both open items from the last review are closed.

Reviewed changes — the delta since the prior pullfrog review at a0a0310, i.e. 44e1f32 alone. resource_limits.rs and mod.rs are untouched since 1712b14 and were verified in the earlier passes.

  • Persisted the --attach resume offsetattachToJob now reads ${tmpdir()}/remote-build-seen-<name> on entry, writes it after every drain, and removes it on the terminal pass, so a re-attach resumes where the previous invocation stopped instead of replaying the whole transcript.
  • Narrowed the file header's "never detached" claim to detached LOCAL builds, with the disposable-remote-VM carve-out the DETACHED MODE block and the skill already carried.
  • Rewrote the skill's silent-degrade bullet — it now states that the job script deliberately does not set AUBE_REQUIRE_PRIMER=1, that the guard protects a shipped binary (hence release.yml and not ci.yml), and what setting it did to every remote job.

The offset persistence holds up: tmpdir/join were already imported for syncSource's list file, Number("") and NaN both fall back to 0 — the old replay behavior, never a skip — and the write sits immediately after the print, so a SIGKILL mid-window can only cost a small replay. The one way a persisted offset could silently skip output is a reset JOB_LOG under a name whose offset file survives; JOB_LOG is written by exactly one caller (startDetachedJob, single call site) against a freshly created, uniquely named instance, so that is unreachable. The doc claims check out against release.yml:476, mac-build.yml:92, and vendor/aube/.gitignore:11. node --test scripts/remote-build.test.mjs is 18/18 green.

ℹ️ AGENTS.md still routes agents to the foreground invocation

AGENTS.md:252 tells agents to run the heavy gates as nub scripts/remote-build.ts --job clippy and --job test — the foreground form this PR documents as SIGKILLed at the harness timeout (measured at 2m13s and again at 10m), leaking a builder until its server-side TTL each time. It is the third site of the class 44e1f32 just fixed, and it is the entry point an agent reads before the skill.

Technical details
# The pre-push loop's remote-gate instruction predates `--detach`/`--attach`

## Affected sites
- `AGENTS.md:252` — step 2b of the pre-push loop: "Run them on an ephemeral GCE spot VM instead: `nub scripts/remote-build.ts --job clippy` and `--job test`". No mention of `--detach`/`--attach` or the exit-75 re-run contract that `.claude/skills/remote-build/SKILL.md:37-42` now leads with.

## Required outcome
- An agent following AGENTS.md alone reaches the detached path, not the one this PR documents as unusable from a harness.

Pullfrog  | View workflow run | Using Claude Opus𝕏

# Conflicts:
#	.claude/skills/remote-build/SKILL.md

@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 critical issues — the merge is clean; one doc-drift note.

Reviewed changes — the delta since the prior pullfrog review at 44e1f32 is the merge commit a93a1dd alone. No PR-authored code changed in it.

  • Merged origin/main (5f526e4) into the branch — one conflict, .claude/skills/remote-build/SKILL.md, because main had reflowed the very paragraphs this PR rewrites.
  • Resolved that conflict in favour of this PR's wording on main's reflowed layout — the silent-degrade bullet, the --reap/never-detached paragraph, and the file-header sentence each appear exactly once, with nothing duplicated or dropped.

I checked the things a merge of this size can quietly invalidate under this PR's claims, and they all hold. resource_limits.rs and identity.rs are untouched on main, so the cgroup work merged with no interaction at all. pm_engine/mod.rs changed 1006 lines on main, yet the PR diff against main still shows only this PR's two hunks — so the merged file is main's plus those hunks, nothing dropped — and reading mod.rs:2810-2909 in the merged tree confirms raw_cpu, cpu_budget, workers, blocking, and rayon_target are all still in scope around the new detected_headroom capture at :2844. ci.yml's two clippy legs are still at lines 220 and 226 with the exact --profile fast invocations, so the citation at remote-build.ts:440 is still accurate. primer_required() still gates on AUBE_REQUIRE_PRIMER, vendor/aube/.gitignore:11 still ignores popular-top*.json, and the error string quoted in PREPARE is verbatim at aube-resolver/build.rs:277, so the DELIBERATELY NOT SET rationale survives main's change to that build.rs. node --test scripts/remote-build.test.mjs is 18/18 green post-merge. All four prior review threads read [RESOLVED]. I did not build the Rust workspace.

ℹ️ The skill still announces the bake gap this PR closes

.claude/skills/remote-build/SKILL.md:80 leads with "Known gap: the image is registry-warm, not artifact-warm" and tells the reader builders cold-compile at ~250s — the exact condition 79e24a3's bake rewrite exists to fix. The paragraph's stated mechanism is wrong independently of this PR too: the bake runs through the same file-based runJob path a job does, so bash -s stdin truncation was never why it was cold. It is the fourth site of the stale-doc class the last three reviews worked through, and it sits in a file this PR edits.

Technical details
# `SKILL.md`'s golden-image section documents the pre-`79e24a3` bake

## Affected sites
- `.claude/skills/remote-build/SKILL.md:80` — "**Known gap: the image is registry-warm, not artifact-warm.** The bake's warm step currently stops after `cargo fetch` (exit 0, image published, no compile); the `bash -s` stdin-truncation fix resolved this for the JOB path but not the bake … Consequence: builders cold-compile, so clippy takes ~250s rather than ~35s and a darwin build ~560s."
  - The gap is closed: the bake now runs `cargo clippy --all-targets --all-features --profile fast`, the addon clippy, `cargo test --workspace --no-run`, and `(cd crates/nub-native && cargo build)` — every cargo invocation `jobScript` emits, verbatim.
  - "stops after `cargo fetch`" and the `bash -s` attribution were already inaccurate before this PR: a `cargo build -p nub-cli --profile fast` line was present, and `buildImage` dispatches its warm script through `runJob``remoteJobCommand` (`scripts/remote-build.ts:854`), which lands the script on disk exactly as the job path does.
  - The "darwin build ~560s" figure, like `:84`'s arm64 Mach-O verification bullet, refers to the cross-build that moved to `mac-build.yml`; that staleness predates this PR.

## Required outcome
- The golden-image section states that the image is artifact-warm for both jobs, and does not attribute a cold image to `bash -s` stdin truncation.
- Any residual gap it keeps naming is one that actually exists in the current bake.

ℹ️ Nitpicks

  • Pre-existing, not introduced by this PR: CI's clippy job runs four legs, not three — tests/brand-lint/check-path-literals.sh (ci.yml:236-237) joined check-env-reads.sh before this branch was cut. jobScript("clippy") emits three, the test is named "reproduces all three legs of the CI clippy gate", and the comment above it claims to "Mirror .github/workflows/ci.yml EXACTLY". Worth folding in while the --profile fast parity work is fresh, but out of this PR's scope as it stands.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

…ng a closed gap

jobScript("clippy") emitted three legs under a comment claiming to mirror ci.yml
exactly, but the clippy job runs four: check-path-literals.sh (ci.yml:237) joined
check-env-reads.sh before this branch was cut and the mirror never picked it up.
A remote clippy could therefore report green on a path-literal violation that CI
then rejects — the same false-green class the nub-native leg exists to prevent.

This is pre-existing rather than introduced here, but it is the second parity
defect in the same function, found while fixing the first, and the test asserting
"all three legs" was pinning it in place exactly as it pinned the missing
--profile fast. Fixing one and leaving the other would leave the mirror claiming
an accuracy it does not have.

The skill's golden-image section also still led with "Known gap: the image is
registry-warm, not artifact-warm", the condition the bake rewrite closed. Its
stated mechanism was wrong independently: the bake dispatches its warm script
through the same file-based runJob path a job does, so bash -s stdin truncation
was never why the image was cold. Replaced with what the bake now does, plus the
reason the warm block has to stay in lockstep with jobScript — cargo fingerprints
on the command shape, so a warm-up differing by driver, profile, package scope or
feature set produces artifacts the job cannot use and the image goes silently
cold.

@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 critical issues — one minor doc-accuracy suggestion inline.

Reviewed changes — the delta since the prior pullfrog review at a93a1dd is c532408 alone, which closes both ℹ️ items that review raised.

  • Added CI's fourth clippy legjobScript("clippy") now emits tests/brand-lint/check-path-literals.sh after check-env-reads.sh, so a remote clippy can no longer report green on a path-literal violation ci.yml rejects.
  • Retitled and extended the parity test — "all four legs", with an assertion on the new lint and a comment recording both drifts this test previously pinned in place rather than caught.
  • Replaced the skill's stale golden-image paragraph — the "Known gap: registry-warm, not artifact-warm" text is gone, replaced with what the bake now does plus why its warm block has to stay in lockstep with jobScript.

I verified the parity claim end to end. ci.yml's clippy job runs exactly those four steps, at lines 220 (root clippy), 226 (addon clippy), 231 (check-env-reads.sh) and 237 (check-path-literals.sh), so every ci.yml:237 citation in the new comments is accurate and the mirror is now complete. Makefile:122-123 (the verify target) already ran both lints, so remote-build.ts was the last stale mirror in the tree — a repo-wide grep for check-env-reads finds no other site. The new leg will not red a remote gate spuriously: both lints run green here in ~1.5s using only bash/awk/grep/find/sed/mktemp, all present on the Ubuntu 24.04 builder, and rsyncPushArgs' -az preserves the exec bit they are invoked through. The new assertion genuinely fails against the previous jobScript, and node --test scripts/remote-build.test.mjs is 18/18 green. On the doc side, the bake's warm block does contain every cargo invocation jobScript emits — root clippy --profile fast, addon clippy --profile fast, cargo test --workspace --no-run, and (cd crates/nub-native && cargo build) — so the rewritten paragraph is accurate about the bake script; the one caveat is inline. resource_limits.rs and mod.rs are untouched since 1712b14 and were verified in earlier passes; I did not build the Rust workspace.

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

Comment thread .claude/skills/remote-build/SKILL.md Outdated
The rewritten golden-image paragraph asserted "the image is artifact-warm for
both jobs" in the present tense. That is true of the bake script and false of the
image a reader would actually boot: --build-image is manual-only, no workflow or
cron invokes it, and the live nub-builder family was baked on 2026-07-28, before
the warm block covered clippy. Someone sizing a run against that sentence would
expect ~35s clippy and get ~250s.

Say what holds when: the bake covers both jobs, an image is only as warm as its
own bake, and the family stays as last baked until someone re-bakes. Points at
`gcloud compute images list` so the date is checkable rather than assumed.

Also widens the re-bake trigger, which was keyed on the toolchain or dependency
graph moving substantially and so would not have fired for this change at all. A
warm block that no longer matches jobScript is exactly as cold as no warm block.

@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 critical issues — one minor doc-accuracy nit inline.

Reviewed changes — the delta since the prior pullfrog review at c532408 is 564694a alone, a two-line edit to .claude/skills/remote-build/SKILL.md that closes the inline finding that review raised. No code changed.

  • Tied the re-bake trigger to the warm blockSKILL.md:78 now says to re-bake when the warm block changes, not only when the toolchain or dependency graph moves substantially.
  • Scoped the artifact-warm claim to the bake rather than the live imageSKILL.md:80 replaces "the image is artifact-warm for both jobs" with "a given image is only as warm as its bake", records that --build-image is manual-only, and tells the reader to check the image date before sizing a run.

I re-verified both claims. A repo-wide grep for --build-image finds it only in scripts/remote-build.ts (help text at :109/:126, argv parsing at :170) and this skill — no workflow, Makefile target, or scheduled job invokes it — so "manual-only" is accurate, and PROJECT = "pullfrog" / IMAGE_FAMILY = "nub-builder" (remote-build.ts:62, :75) match the names the paragraph uses. The bake's warm block still contains every cargo invocation jobScript emits, so the lockstep sentence holds. resource_limits.rs, pm_engine/mod.rs, remote-build.ts, and remote-build.test.mjs are untouched since c532408 and were verified in earlier passes; I did not rebuild the Rust workspace.

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

Comment thread .claude/skills/remote-build/SKILL.md Outdated
The previous commit told the reader to check the image date before sizing a run
and handed them `gcloud compute images list --project pullfrog
--no-standard-images`, which prints NAME/PROJECT/FAMILY/DEPRECATED/STATUS and no
timestamp. Following it literally answers nothing.

Verified by running both forms: the bare command shows no date, and
`--format='table(name,family,creationTimestamp)' --sort-by=~creationTimestamp`
shows the live nub-builder image at 2026-07-28. Documented the working form.

@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 found. The one open item from the last review is closed.

Reviewed changes — the delta since the prior pullfrog review at 564694a is db37c8b alone, a one-line edit to .claude/skills/remote-build/SKILL.md. No code changed.

  • Made the image-date check executable as written — the parenthetical in the golden-image paragraph now passes --format='table(name,family,creationTimestamp)' --sort-by=~creationTimestamp, so the command actually prints the date the surrounding sentence tells the reader to check, newest first.

I verified the invocation against the google-cloud-sdk installed on this runner rather than from memory. The projection parses and renders headers NAME FAMILY CREATION_TIMESTAMP; gcloud compute images list --help carries --no-standard-images and --sort-by=[FIELD,…] (a ~ prefix is descending, and the flag is applied before formatting, in --flatten → --sort-by → --filter → --limit order); creationTimestamp is a real field of the Compute v1 Image message (compute_v1_messages.py:140); and the full command clears argument parsing, stopping only at the runner's missing credentials. The added caveat that the default columns carry no timestamp matches the SDK's own LIST_FORMAT. resource_limits.rs, pm_engine/mod.rs, remote-build.ts, and remote-build.test.mjs are untouched since c532408 and were verified in earlier passes; I did not rebuild the Rust workspace.

The three informational items earlier reviews raised are unchanged and still maintainer-owned: the cpu_budget keep/reshape/remove decision, the absence of mechanical enforcement on the bake ↔ jobScript lockstep, and AGENTS.md:252 still routing agents to the foreground invocation.

Pullfrog  | View workflow run | Using Claude Opus𝕏

…TS.md routing to the foreground form

Two items the last review left as informational.

AGENTS.md told agents to run the heavy gates as `remote-build.ts --job clippy`,
the foreground form. That is the invocation the agent harness SIGKILLs at its
timeout, which no handler can catch, so cleanup is skipped and the builder leaks
until its server-side TTL. It is the file every agent reads, so it was the
highest-traffic remaining route back to the broken path. Now points at
--detach/--attach and says why.

The bake/jobScript lockstep was enforced by three separate comments asking the
next person to keep them aligned, and none of them could fail. That is a
guardrail standing in for a fix, and the drift it guarded against is what made
the tool unusable: the bake warmed `cargo build -p nub-cli --profile fast` while
clippy ran `cargo clippy --all-targets --all-features --profile fast` — artifacts
unusable for four independent reasons at once, with the image advertising warm
while every builder cold-compiled.

The test now reads the bake block out of the source and asserts every cargo
invocation jobScript emits has a matching warm-up. Prefix matching, since the
bake may warm more than the job runs and `cargo test` is deliberately warmed as
`cargo test --workspace --no-run`.

It reads the source rather than importing an extracted function on purpose: the
warm script is a template literal containing escaped backticks, and extracting it
into an exported function silently truncated the body at the first one — caught
only because this test then reported the bake warming a single line.

Verified it can fail: deleting the addon warm-up reds it, restoring greens it.

@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 critical issues — two minor suggestions inline. The new lockstep test is real and I confirmed it can fail.

Reviewed changes — the delta since the prior pullfrog review at db37c8b is 1ed483f alone, which closes two of the three standing informational items. resource_limits.rs, pm_engine/mod.rs, remote-build.ts, and SKILL.md are untouched since c532408 and were verified in earlier passes.

  • Added a bake ↔ jobScript lockstep test — it slices the warm block out of remote-build.ts by source text, extracts the cargo invocations from both sides, and asserts every command jobScript emits for clippy and test has a prefix-matching warm-up, with a failure message that names the missing command.
  • Repointed AGENTS.md's remote-gate step at --detach/--attach — step 3 of the pre-push loop now shows the detached form, states that a foreground run is SIGKILLed at the harness timeout and leaks the builder, and gives the exit-75 re-run contract.

I checked the test rather than taking the commit message's word for it. It passes on this tree (19/19), and it genuinely reds: deleting the bake's (cd crates/nub-native && cargo build) line fails it with the right message, restoring it greens. The parse holds up end to end — PREPARE's # comment lines are filtered even though one of them quotes cargo build -p nub-cli --profile fast, .cargo/env and $CARGO_TARGET_DIR don't match /(^|\W)cargo\s/, the || cut strips the WARM-WARN tails, and warmed comes out as exactly cargo fetch plus the four bake commands. The subshell legs carry their closing paren into the comparison, so those match exactly rather than by prefix. Reading the bake out of the source instead of exporting it is the right call here for the reason the comment gives, and the from > 0 && to > from guard plus the >= 4 floor mean a broken slice fails loudly instead of vacuously passing. AGENTS.md's claims check out too: EX_STILL_RUNNING = 75 at remote-build.ts:80, returned by attachToJob after ATTACH_WINDOW_MS = 5 * 60_000.

ℹ️ Nitpicks

  • The prefix rule admits one drift class it is meant to catch: a bake-side flag that changes the artifact universe still passes on the non-subshell legs. Verified by editing the bake to cargo test --release --workspace --no-runjobScript("test")'s cargo test still prefix-matches and the test stays green, even though release is a different target directory. Not worth complicating the matcher over; worth knowing the guard proves prefix coverage, not fingerprint equivalence.

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

Comment thread scripts/remote-build.test.mjs
Comment thread AGENTS.md
The lockstep test added last commit could not run on the PRs it guards.
`pnpm run test:scripts` is invoked only by the docs-links job, gated on
run_site, whose paths-filter is `site/**` alone — and `scripts/**` was in no
filter group at all, while ci-gate counts a skipped job as success. So a PR
editing the bake or jobScript never ran their tests, including the PR that added
the test, and drift would have surfaced as trunk red on push:main instead of
failing the PR that caused it. A guard that cannot fire on the change it guards
is not a guard.

Adds a `scripts` filter group (`scripts/**`, `package.json`) and a `run_scripts`
output following the same fail-safe pattern as the others — default true, only a
clean pull_request filter may turn it off — and widens the docs-links gate to
either flag. Verified the workflow still parses and that run_scripts is a real
matrix-plan output.

The AGENTS.md sweep also stopped one hop short: `dev-loop/SKILL.md:154` carried
the near-verbatim twin of the old wording, and AGENTS.md sends agents to
`dev-loop` for the build loop, so the foreground route survived in the skill an
agent reads next. A repo-wide grep for the old invocation now finds only
remote-build's own usage block, where the foreground form is deliberate and
labelled.

Finally, the lockstep matcher compared prefixes only, so a bake-side flag that
changes the artifact universe still passed: `cargo test --release --workspace
--no-run` satisfies `cargo test` while targeting a different directory entirely —
precisely the drift class the test exists to catch. It now compares the resolved
profile on both sides. Verified with that exact counterexample: it reds the test
and restoring greens it.

@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 critical issues — two minor suggestions inline. Both threads from the last review are closed.

Reviewed changes — the delta since the prior pullfrog review at 1ed483f is 09badb5 alone, which closes both inline findings that review raised. resource_limits.rs, pm_engine/mod.rs, remote-build.ts, and remote-build/SKILL.md are untouched since c532408 and were verified in earlier passes.

  • Added a scripts paths-filter and a run_scripts planner outputscripts/** and package.json now feed a fourth group, carrying the same fail-safe shape as the other three (true unless a clean pull_request filter says otherwise, plus the true/false sanity guard), and docs-links is gated on run_site || run_scripts so test:scripts runs on the PRs that can break it.
  • Repointed dev-loop/SKILL.md at --detach/--attach — the last surviving twin of the foreground wording now carries the detached form and the SIGKILL/leak rationale.
  • Compared profiles, not just prefixes, in the lockstep test — the matched warm line is captured and its profile asserted equal to the job's, closing the drift class the previous review's nit demonstrated.

I checked the delta on the runner rather than reading it. node --test 'scripts/**/*.test.m[jt]s' is 120 tests / 119 pass / 1 skip / 0 fail, and remote-build.test.mjs alone is 19/19. The new profile assertion genuinely reds: rewriting the bake's cargo test --workspace --no-run to cargo test --release --workspace --no-run fails with "different profile, so different target directory", and restoring it greens. ci.yml parses and matrix-plan.outputs carries run_scripts. The doc sweep is now complete — a repo-wide markdown grep for --job clippy|--job test returns only AGENTS.md:282 and dev-loop/SKILL.md:154, both detached, plus remote-build/SKILL.md:25-29, where the foreground form is deliberate and labelled. No aggregator change was needed: docs-links was already in ci-gate's needs, and its three gate scripts run without a root pnpm install, so widening the gate costs a scripts-only PR only the site install and the docs-link check.

ℹ️ Nitpicks

  • profileOf's --release probe is /(^|\s)--release(\s|$)/, which the subshell form evades: (cd crates/nub-native && cargo build --release) returns "dev" because the flag is followed by ). Two of the four bake legs are subshells, so the class the new assertion closes is closed for the bare legs only. A [\s)]|$ tail, or trimming a trailing paren before the probe, would cover both.

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

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml
…hell hole in the profile probe

Three items from review, all introduced or left open by the previous commit.

The comment above the widened gate still described the old one — "Path-gated on
run_site … only runs when site/** changed" — sitting directly above the line that
made it false. Same doc-drift class as the last several rounds, one line from its
own cause.

The new filter reached `test:scripts` but left its two step-mates unreachable.
`pnpm run soak` and `pnpm run tools:check` share that single run block, and their
inputs sit in no filter group, so a PR touching `.npmrc`, `.github/renovate.json`,
`tools/`, `external-tools.json`, or `.cargo/config.toml` still skipped its own
gate and drift still surfaced as trunk red — the exact failure this commit's
predecessor fixed for the third script only. The group now covers all three
gates' inputs, and its comment says so, since the name alone no longer does.

The profile probe evaded the subshell form: `/(^|\s)--release(\s|$)/` reads
`(cd crates/nub-native && cargo build --release)` as profile "dev", because the
flag is followed by `)`. Two of the four bake legs are subshells, so the drift
class the assertion was added to close was closed for the bare legs only. Both
probes now accept a closing paren as a terminator.

Verified with both shapes: the subshell case reds where it previously passed, the
bare-leg case still reds, and restoring either greens. remote-build.ts is
byte-identical after the experiments; ci.yml still parses.

@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 critical issues — one minor suggestion inline. Both threads from the last review are closed.

Reviewed changes — the delta since the prior pullfrog review at 09badb5 is 21eb48c alone, which closes both inline findings that review raised. resource_limits.rs, pm_engine/mod.rs, remote-build.ts, and remote-build/SKILL.md are untouched since c532408 and were verified in earlier passes.

  • Widened the scripts paths-filter to all three gates it fronts — soak's five surfaces (.cargo/config.toml, .npmrc, tools/**, .github/renovate.json) and tools:check's two (external-tools.json, rust-toolchain*) joined scripts/** and package.json, so an edit to a soak or pin surface now fails its own PR instead of surfacing as trunk red.
  • Rewrote the docs-links gate comment for the run_site || run_scripts condition, naming which clause covers which steps.
  • Closed the subshell hole in the lockstep test's profile probeprofileOf's tails are now [\s)]|$ and [^\s)]+, so a flag terminated by ) is read correctly.

I checked the filter against the code rather than the comment, since a filter that looks complete is exactly the artifact that hides a gap. scripts/soak/paths.mts's SURFACES is exactly {.cargo/config.toml, .npmrc, tools/pnpm-workspace.yaml, tools/taze.config.mts, rust-toolchain.toml, .github/renovate.json}; soak.mts:394-398 consults five of them and external-tools.mts:508-511 consults external-tools.json plus toolchainToml — every one is now covered, and tools/** reaches both tools/-anchored files. .dockerignore is correctly absent: neither --check path reads DOCKERIGNORE, and DOCKER_PREBAKE is null for this repo. ci.yml parses, matrix-plan.outputs carries run_scripts, and docs-links has exactly one if: — the apparent duplicate in the range-diff is a rendering artifact, not a second key.

The regex change is real and I confirmed it can fail: rewriting the bake's (cd crates/nub-native && cargo build) to … cargo build --release) now reds the test with "different profile, so different target directory", where the old (\s|$) tail waved it through. I also hand-evaluated profileOf over all five bake legs and all four jobScript legs and found no remaining live hole — the --profile=X spelling would read as dev, but a one-sided = breaks the prefix match first, so it fails loudly rather than passing wrongly. node --test scripts/remote-build.test.mjs is 19/19 green.

ℹ️ The check that now guards soak and the tool pins is still named "Docs links & anchors"

On a PR touching only tools/**, external-tools.json, .npmrc, or .github/renovate.json, the sole check that runs is Docs links & anchors, and a pnpm run soak drift failure reports under that name. This is strictly better than the pre-PR state, where no gate ran at all, but the job's identity no longer matches what it enforces, and the widened gate also pays a site pnpm install plus the docs-link check on those PRs.

Technical details
# The `docs-links` job now fronts three gates its name does not describe

## Affected sites
- `.github/workflows/ci.yml:978-979``docs-links` / `name: Docs links & anchors`, now reached via `run_scripts` for `soak`, `tools:check` and `test:scripts`.
- `.github/workflows/ci.yml:991-995` — the single `run:` block holding all three non-site gates.
- `.github/workflows/ci.yml:1006-1012` — the site-only steps (`pnpm install --frozen-lockfile` in `site/`, `node scripts/check-docs-links.ts`) that a scripts-only PR now also runs.

## Required outcome
- A failing repo-gate check names what it gates, so a reader does not have to open the log to learn that a docs-named check failed on soak drift.

## Suggested approach (optional)
- Rename the job (e.g. `Repo gates & docs links`), or split the three non-site steps into their own `run_scripts`-gated job. Renaming is safe: `ci-gate`'s `needs` list uses job IDs, not display names, and `CI gate` is documented as the sole required status check — so neither change touches branch protection.

## Open questions for the human
- Is the extra site install on a scripts-only PR worth avoiding, or is the shared runner cheap enough to leave as is?

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

Comment thread .github/workflows/ci.yml Outdated
`rust-toolchain*` is in the `rust` group, so "None of these paths was in any
group" was wrong. The sharper statement is that no group REACHED the docs-links
job, because it is not gated on run_rust — which is the actual reason the gate
never ran.

The adjacent claim that docs-links is "Skipped on Rust/types-only PRs" was made
false by the same commit that wrote it: a `rust-toolchain.toml`-only PR now sets
both run_rust and run_scripts, so the job runs. That is deliberate, and it is
precisely the case the filter was widened to catch, since `tools:check` is what
pins that file. Both comments now say so.

Not changed, and worth a maintainer's call: the job is still named "Docs links &
anchors" while fronting soak, tools:check and test:scripts, so a soak drift
failure on a `tools/**`-only PR reports under that name, and such a PR also pays
a site pnpm install plus the docs-link check. Splitting the three non-site gates
into their own job is the clean fix; renaming this one is cheaper but changes a
check name, and `main` returns "Branch not protected" for the classic API, so the
required-context list sits in a ruleset this session cannot read. Left alone
rather than guessed at.
@colinhacks
colinhacks merged commit 4add582 into main Aug 4, 2026
63 checks passed
@colinhacks
colinhacks deleted the megapr-container-limits branch August 4, 2026 00:58
@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