Add audit fix plan for box new clean-checkout failures - #4
Merged
Conversation
Live-tested ./box new end-to-end (twice, in isolated copies) plus five parallel deep-read audits over provisioning, the Hermes supervisor, the migration tool, ops/docs, and the test suite. Root-caused why box creation fails today (stale HERMES_INSTALLER_SHA256 pin, then a silent set -e death in verify_required because the Hermes installer's launcher never lands on the agent user's PATH) and documented a prioritized, step-by-step fix plan with reproduction instructions for every finding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an audit plan document, updates the Hermes installer pin and check, tightens provisioning and gateway startup behavior, enforces actual-byte ZIP extraction limits, adds a regression test, and ignores Python bytecode caches. ChangesHermes audit and hardening
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/audit-2026-06-30-fixes.md`:
- Around line 99-104: Tighten the `verify_required` explanation to reflect the
actual control flow: when `INSTALL_HERMES` or `INSTALL_EXECUTOR` is set, a
failing `command -v` already stops execution under `set -e`, so do not describe
this path as a silent success. Update the diagnosis in `verify_required` to
focus on the real issue in that function: missing explicit failure handling and
the script’s silent exit behavior, and make sure the wording around the `git
rev-parse HEAD` fallback and the `command -v hermes` / `command -v executor`
checks matches the function’s actual return semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b14d1f5-dc14-44ba-b531-daecdf5257a6
📒 Files selected for processing (1)
docs/audit-2026-06-30-fixes.md
Comment on lines
+99
to
+104
| 3. **Separately** (a second, independent bug in the same function): even if (1)/(2) | ||
| didn't kill the script, `verify_required`'s actual return value is whatever | ||
| its *last* statement returns — the `git rev-parse HEAD` check — so the | ||
| `command -v hermes`/`command -v executor` checks on lines 213-214 were never | ||
| going to gate success/failure even if they survived `set -e`. Two bugs, same | ||
| six lines, masking each other. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the verify_required diagnosis.
The current text says the function can return success while hermes/executor are missing, but when those INSTALL_* flags are set the failing command -v already trips set -e before that fallback can happen. The real bug is the silent exit and missing explicit failure handling, not a successful pass on absent tools.
Also applies to: 165-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/audit-2026-06-30-fixes.md` around lines 99 - 104, Tighten the
`verify_required` explanation to reflect the actual control flow: when
`INSTALL_HERMES` or `INSTALL_EXECUTOR` is set, a failing `command -v` already
stops execution under `set -e`, so do not describe this path as a silent
success. Update the diagnosis in `verify_required` to focus on the real issue in
that function: missing explicit failure handling and the script’s silent exit
behavior, and make sure the wording around the `git rev-parse HEAD` fallback and
the `command -v hermes` / `command -v executor` checks matches the function’s
actual return semantics.
Upstream rotated hermes-agent.nousresearch.com/install.sh since the last pin, so box new failed checksum verification on a clean checkout. Skimmed the new installer (no suspicious network/eval patterns; same install flow plus newer FHS-layout-detection logic for root installs) before re-pinning. Also add `make check-hermes-pin`, a network-requiring target kept out of `make check` (which stays hermetic), so drift can be caught proactively instead of surfacing as a box new failure for a real user. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
29-29: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider adding a curl timeout.
curl -fsSLhas no--max-time/--connect-timeout, so a hung connection would block the target indefinitely. Low impact since this is a manual, network-gated target excluded frommake check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 29, The Makefile target that downloads the install script uses curl without any timeout, so a stalled network request can hang the target indefinitely. Update the curl invocation in the install flow to include a reasonable connection and/or overall timeout while keeping the existing checksum pipeline intact, so the manual target remains network-gated but won’t block forever.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 27-30: The check-hermes-pin recipe masks curl failures because the
curl output is piped into sha256sum, so a fetch error can look like a valid hash
mismatch. Update the Makefile target to detect the download failure directly in
the same shell block used by the pinned/live comparison, and only compute the
SHA256 after confirming curl succeeded. Keep the existing check-hermes-pin flow,
but make the live hash assignment fail fast with the intended fetch-failure
message instead of relying on the empty-output guard.
---
Nitpick comments:
In `@Makefile`:
- Line 29: The Makefile target that downloads the install script uses curl
without any timeout, so a stalled network request can hang the target
indefinitely. Update the curl invocation in the install flow to include a
reasonable connection and/or overall timeout while keeping the existing checksum
pipeline intact, so the manual target remains network-gated but won’t block
forever.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 705c5be1-b884-49f0-91ea-1361e0d56440
📒 Files selected for processing (3)
Makefilebox.envdocs/audit-2026-06-30-fixes.md
✅ Files skipped from review due to trivial changes (1)
- docs/audit-2026-06-30-fixes.md
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
The 100 GiB safety gate in _zip_layout only summed the ZIP central directory's declared file_size, which is attacker-controlled metadata that zipfile never cross-validates against the real inflate stream. A crafted archive could declare a small size while extracting far more. _extract now streams each member in 1 MiB chunks and tracks a running total of actual bytes written, aborting the moment it exceeds the cap regardless of what the archive claimed. Added a regression test that lowers the cap and confirms extraction aborts on real bytes, not the declared total. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two stacked bugs in provision.sh's verify_required(): (1) `command -v
hermes` lived in an if-THEN body with no `|| { ...; return 1; }`, so the
moment it failed, set -e killed the whole provision.sh process with zero
output beyond "verifying required tools" — confirmed live, twice, on a
clean checkout. (2) even surviving that, the function's return value was
whatever its last statement returned, so the hermes/executor checks were
never actually gating success.
Root cause for why `command -v hermes` failed at all: provision.sh passes
an explicit --dir to the Hermes installer, which makes the installer skip
its own root/FHS auto-detection and link the `hermes` command into
$HOME/.local/bin (root's home, since this runs as root) instead of
/usr/local/bin — contradicting the comment that assumed FHS layout.
Verified by tracing the live (newly re-pinned, see P0-1) installer's
resolve_install_layout().
Fixes:
- install_hermes() now symlinks the real launcher location into
$OPT/bin/hermes, which is on both the agent user's PATH (guest/profile.sh)
and, with this change, provision.sh's own PATH too (added $OPT/bin to the
top-of-script PATH export — needed for verify_required's `command -v
hermes` to see it in the same process).
- Every check in verify_required now has explicit `|| { log ...; return 1; }`
handling, matching the style already used for node/uv/nvim/claude/codex.
- Corrected the stale install-location comment.
Verified live end-to-end: `./box new` now reaches "provision complete" and
"Ready. Enter with: ./box enter <name>"; `./box doctor` passes all 17
checks; `./box rm --force` cleans up with no orphaned state.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_start_gateway checked _gateway_running and, if not running, spawned `hermes gateway run --replace` with no lock between the check and the spawn. hb-workload's 20s reconcile loop and a host-initiated `hb up` (via cmd_open/_prepare_runtime/cmd_repair) could both observe "not running" and both exec the gateway, directly contradicting the single-writer guarantee the gateway-enable confirmation ceremony exists to protect. Added an mkdir-based lock (same pattern as box's _try_acquire_lock, with the same stale-pid recovery) around the check-then-spawn in _start_gateway. Every caller of _start_gateway (hb up, hb reconcile, gateway_enable) goes through the same function, so one lock covers all paths without needing to duplicate it in hb-workload. Verified in isolation: 8 concurrent invocations of the locked check-then-spawn section raced against each other, and exactly 1 actually ran the "spawn" — confirms the lock serializes the window that was previously racy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+157
to
+158
| if [[ -x "$HOME/.local/bin/hermes" ]]; then | ||
| ln -sf "$HOME/.local/bin/hermes" "$OPT/bin/hermes" |
There was a problem hiding this comment.
This symlink points into root's home while the agent user is expected to run hermes through $OPT/bin. On a normal root-owned home directory, agent cannot traverse /root, so verify_required can pass as root while the box still cannot launch Hermes as agent.
Artifacts
- Contains supporting evidence from the run (text/x-shellscript; charset=utf-8).
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: provision/provision.sh
Line: 157-158
Comment:
**Keep launcher executable**
This symlink points into root's home while the `agent` user is expected to run `hermes` through `$OPT/bin`. On a normal root-owned home directory, `agent` cannot traverse `/root`, so `verify_required` can pass as root while the box still cannot launch Hermes as `agent`.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/hermes-state.sh`:
- Around line 357-366: The test setup in the hermes-state shell script is
leaking Python temp directories because the `mkdtemp()` calls for `d`, `stage`,
and `external_stage` are not tied to the shell `$tmp` cleanup. Update the
temp-dir handling in the `_zip_layout` test block to use `TemporaryDirectory()`
contexts or create the directories under the existing bash-managed temp root so
they are removed automatically. Keep the changes localized around the temp setup
used with `zipfile.ZipFile`, `module._zip_layout`, and the
`stage`/`external_stage` paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bc4264dd-0c07-41ea-9be6-e4d66082f9eb
📒 Files selected for processing (6)
.gitignoredocs/audit-2026-06-30-fixes.mdguest/hbguest/hermes-stateprovision/provision.shtests/hermes-state.sh
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- docs/audit-2026-06-30-fixes.md
| the latter is more in keeping with the project's existing host-side systemd | ||
| pattern (`ops/systemd/`), but the cheap fix is the restart loop. | ||
|
|
||
| ### P1-3: `gateway_is_disabled()` checks a marker file, not process state |
There was a problem hiding this comment.
🟡 Medium docs/audit-2026-06-30-fixes.md:286
The P1-3 entry describes a failure scenario that cannot occur in the current guest/hb code. gateway_enable() calls rm -f "$GATEWAY_DISABLED" before _start_gateway, and on failure re-touches $GATEWAY_DISABLED — but the described 'spawned, then later failed and re-touched as disabled' path does not exist, since there is no code after _start_gateway succeeds that can fail before the function returns. The real stale-marker case is gateway_disable(), which touches $GATEWAY_DISABLED before calling _stop_gateway; if _stop_gateway fails, box doctor reports 'intentionally disabled' while a gateway process is still alive. As written, this entry would send the fix to gateway_enable and miss the actual bug in gateway_disable. Consider correcting P1-3 to target gateway_disable()'s ordering and fix suggestion accordingly.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @docs/audit-2026-06-30-fixes.md around line 286:
The P1-3 entry describes a failure scenario that cannot occur in the current `guest/hb` code. `gateway_enable()` calls `rm -f "$GATEWAY_DISABLED"` before `_start_gateway`, and on failure re-touches `$GATEWAY_DISABLED` — but the described 'spawned, then later failed and re-touched as disabled' path does not exist, since there is no code after `_start_gateway` succeeds that can fail before the function returns. The real stale-marker case is `gateway_disable()`, which touches `$GATEWAY_DISABLED` *before* calling `_stop_gateway`; if `_stop_gateway` fails, `box doctor` reports 'intentionally disabled' while a gateway process is still alive. As written, this entry would send the fix to `gateway_enable` and miss the actual bug in `gateway_disable`. Consider correcting P1-3 to target `gateway_disable()`'s ordering and fix suggestion accordingly.
| before it calls `hb reconcile`, or simply rely on (1) since `hb reconcile` | ||
| already calls into `_start_gateway`. | ||
|
|
||
| ### P1-2: `hb-workload` itself is unsupervised |
There was a problem hiding this comment.
🟡 Medium docs/audit-2026-06-30-fixes.md:255
The P1-2 entry is factually wrong about the failure mode. box:_create_base starts hb-workload via exec runuser ... hb-workload, so hb-workload runs as PID 1 inside the guest — if it dies, PID 1 exits and the VM stops entirely. On normal deployments ops/systemd/tx9-box@.service invokes ops/tx9-host supervise %i, which restarts any VM whose smolvm machine status is no longer running. The current text claims the workload "persists unmanaged until the VM reboots" and recommends an internal while true; do ... hb-workload ...; done restart loop. That loop would suppress the VM-exit signal that the host-side supervisor depends on, hiding failures from the existing restart path. Correct the description to reflect that the VM exits (and is restarted by the host supervisor) rather than persisting unmanaged, and remove or revise the internal restart-loop recommendation so it does not mask failures from tx9-host supervise.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @docs/audit-2026-06-30-fixes.md around line 255:
The `P1-2` entry is factually wrong about the failure mode. `box:_create_base` starts `hb-workload` via `exec runuser ... hb-workload`, so `hb-workload` runs as PID 1 inside the guest — if it dies, PID 1 exits and the VM stops entirely. On normal deployments `ops/systemd/tx9-box@.service` invokes `ops/tx9-host supervise %i`, which restarts any VM whose `smolvm machine status` is no longer `running`. The current text claims the workload "persists unmanaged until the VM reboots" and recommends an internal `while true; do ... hb-workload ...; done` restart loop. That loop would suppress the VM-exit signal that the host-side supervisor depends on, hiding failures from the existing restart path. Correct the description to reflect that the VM exits (and is restarted by the host supervisor) rather than persisting unmanaged, and remove or revise the internal restart-loop recommendation so it does not mask failures from `tx9-host supervise`.
A failed `box new`/`box load` produced exactly one line of explanation
("box creation failed") before _rollback_created deleted the VM — the only
place the real error lived. Both live P0-1/P0-2 failures during this audit
were only diagnosable by bypassing the trap-driven rollback manually; a
normal user had no equivalent escape hatch.
- _provision_into now optionally tees the provisioning transcript (the part
that already shells guest output through the host process) to a caller-
supplied log file.
- cmd_new and cmd_load capture that transcript to a temp file, and on
failure persist it to backups/.failed/<name>-<timestamp>.log before
calling die, so the evidence survives rollback.
- Added BOX_KEEP_ON_FAILURE=1 (or `box new <name> --keep-on-failure`) to
skip rollback entirely and leave the VM up for direct inspection, with
hints printed for how to look at it and clean it up afterward.
- cmd_new's die message now names which stage failed (creating the base VM
/ provisioning / preparing the runtime) instead of collapsing all three
into one undifferentiated message.
Verified live: deliberately broke HERMES_GIT_SHA to force a real
provisioning failure under BOX_KEEP_ON_FAILURE=1. Confirmed the transcript
landed in backups/.failed/ with the actual git error, the die message said
"while provisioning", and the VM was left running (smolvm machine status
showed it alive) instead of being silently deleted.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…test Every box new/repair (full mode) installed whatever was currently `latest` on the npm registry for claude-code, codex, and executor — two boxes created a week apart, or the same box before/after a repair, could silently end up on different versions with no way to pin or reproduce. Added CLAUDE_CODE_VERSION/CODEX_VERSION/EXECUTOR_VERSION to box.env, empty by default (preserves current latest-on-install behavior). When set, install_claude/install_codex/install_executor in provision.sh now install the pinned version via npm's @Version syntax. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
box (1)
737-764: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the failing load stage.
cmd_newnow reports whether base creation, provisioning, or runtime prep failed, but thiscmd_loadbranch still collapses base creation and provisioning intodestination provisioning failed.Proposed fix
- local keep_on_failure=0 log_file failed_log="" + local keep_on_failure=0 log_file failed_log="" stage="" ... - if ! _create_base "$n" || ! _provision_into "$n" full "$log_file"; then + if ! _create_base "$n"; then + stage="creating the base VM" + elif ! _provision_into "$n" full "$log_file"; then + stage="provisioning" + fi + if [[ -n "$stage" ]]; then ... - die "destination provisioning failed" + die "box load failed while $stage" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@box` around lines 737 - 764, The cmd_load failure path still uses a generic message for two different steps, so update the branch around _create_base and _provision_into to report which stage failed. Split the combined check into separate handling for base creation versus provisioning, and make the final die message reflect the specific failing stage instead of always saying “destination provisioning failed.”
🧹 Nitpick comments (1)
box (1)
596-596: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve the provisioning exit status without relying on global
pipefail.The
teepipeline can reporttee’s status instead ofsmolvm’s ifpipefailis not active here. Process substitution keeps transcript capture while returning the guest command status.Proposed fix
- smolvm machine exec --name "$n" -- bash -lc "$cmd" 2>&1 | tee "$log_file" + smolvm machine exec --name "$n" -- bash -lc "$cmd" > >(tee "$log_file") 2>&1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@box` at line 596, The provisioning command pipeline is losing the guest command’s exit status because `smolvm machine exec` is piped through `tee`, which can mask failures unless global `pipefail` is set. Update the provisioning flow around the `smolvm machine exec` invocation to keep transcript capture while preserving the command’s real status, using process substitution or an equivalent approach instead of a plain pipe; locate the change in the shell logic that builds and runs `cmd` for the machine execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@box`:
- Around line 658-662: Make the transcript save path best-effort in the failure
handling block around the existing transcript copy logic in the
rollback/keep-on-failure flow (the `failed_log` save path used in the
provisioning handler). Guard the `mkdir -p backups/.failed` and `cp "$log_file"
"$failed_log"` steps so they cannot abort under `errexit`, and only emit the
“transcript saved” warning when the copy actually succeeds. Apply the same fix
to the duplicated save block referenced by the other occurrence so rollback
behavior still follows the original failure.
- Around line 812-817: The restore failure path in the box restore logic is
missing the `_provision_into "$n" assets` output in `log_file`, so later
provisioning errors are lost before rollback; update the restore/provision flow
to tee that call’s transcript to the log and ensure the failure handler
preserves the transcript before cleanup. Also adjust the keep-on-failure
messaging in the same restore handler to reflect that the incomplete destination
is being left in place for inspection, not that cleanup is still being
attempted.
---
Outside diff comments:
In `@box`:
- Around line 737-764: The cmd_load failure path still uses a generic message
for two different steps, so update the branch around _create_base and
_provision_into to report which stage failed. Split the combined check into
separate handling for base creation versus provisioning, and make the final die
message reflect the specific failing stage instead of always saying “destination
provisioning failed.”
---
Nitpick comments:
In `@box`:
- Line 596: The provisioning command pipeline is losing the guest command’s exit
status because `smolvm machine exec` is piped through `tee`, which can mask
failures unless global `pipefail` is set. Update the provisioning flow around
the `smolvm machine exec` invocation to keep transcript capture while preserving
the command’s real status, using process substitution or an equivalent approach
instead of a plain pipe; locate the change in the shell logic that builds and
runs `cmd` for the machine execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a2b6842-2586-4eb6-9d69-472f126668de
📒 Files selected for processing (2)
boxdocs/audit-2026-06-30-fixes.md
✅ Files skipped from review due to trivial changes (1)
- docs/audit-2026-06-30-fixes.md
hb-workload was exec'd as the VM's boot command, so if the loop process itself died (OOM-killed, an unguarded edge case under its `set -u`), nothing restarted it — health reconciliation and bridge socat processes stayed unmanaged until the next VM reboot. Dropped the `exec` and wrapped hb-workload in an outer restart loop as the VM's boot command instead, so the supervisor (PID 1 in the guest) restarts it on any exit. Verified live: created a box, confirmed hb-workload was running under the new supervisor loop, force-killed it (pkill -9), and confirmed a fresh hb-workload process came back up within the 2s restart delay. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gateway_is_disabled() only checked for the GATEWAY_DISABLED marker file. If gateway_enable() spawned the gateway via _start_gateway() and something later in that call path failed before returning, the marker was re-touched as disabled but nothing verified the already-spawned process was actually killed first — box doctor would then report "intentionally disabled" while a real gateway process was alive underneath that claim. gateway_is_disabled() now requires both the marker AND _gateway_running being false. gateway_enable()'s failure path explicitly calls _stop_gateway before re-touching the marker, instead of assuming the marker alone is sufficient. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
box.env (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the
executorpackage reference.
EXECUTOR_VERSIONdocuments itself vianpm view executor versions, but "executor" is a generic name on the npm registry. Worth a one-line comment clarifying the exact scoped/registry package this pin targets (matching whatever provisioning resolves), to avoid future confusion when someone runs that exactnpm viewcommand and gets an unrelated package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@box.env` at line 22, The EXECUTOR_VERSION comment is ambiguous because `npm view executor versions` points to a generic package name, so update the inline note on `EXECUTOR_VERSION` to explicitly name the exact package/repository scope that provisioning resolves to. Keep the reference near `EXECUTOR_VERSION` in box.env and make it clear which executor package the version pin applies to, so the lookup command is not misleading.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@box.env`:
- Line 22: The EXECUTOR_VERSION comment is ambiguous because `npm view executor
versions` points to a generic package name, so update the inline note on
`EXECUTOR_VERSION` to explicitly name the exact package/repository scope that
provisioning resolves to. Keep the reference near `EXECUTOR_VERSION` in box.env
and make it clear which executor package the version pin applies to, so the
lookup command is not misleading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd652058-9807-4c48-a92b-8f48751227d9
📒 Files selected for processing (5)
boxbox.envdocs/audit-2026-06-30-fixes.mdguest/hbprovision/provision.sh
✅ Files skipped from review due to trivial changes (1)
- docs/audit-2026-06-30-fixes.md
🚧 Files skipped from review as they are similar to previous changes (1)
- provision/provision.sh
assets_only() only ran make_agent, install_config, place_assets, seed_data,
and write-manifest — never install_hermes or install_executor. A box whose
Hermes or Executor install failed or went missing could only be fixed by
the full, multi-minute reinstall path (cmd_repair always calls
_provision_into full); there was no cheap "retry just the broken piece"
repair, and assets mode (only used by cmd_load's post-restore step) left
those tools broken if they were already broken.
install_hermes/install_executor are now idempotent (skip reinstall if
already on PATH and, for hermes, already at the pinned git SHA — the same
bar verify_required already holds them to) and assets_only calls both
right after make_agent, plus verify_required at the end to confirm the
repair actually worked.
Verified live: created a box, deleted the hermes and executor binaries to
simulate a failed install, ran assets-mode provisioning directly, and
confirmed both were reinstalled and resolvable again. Re-ran assets mode a
second time and confirmed the idempotent guards skipped reinstalling
already-healthy tools ("hermes already installed... skipping reinstall").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Five of thirteen box subcommands had zero test coverage (ls, rm, enter, stop, start), alongside untested code paths in already-tested commands: the _assign_port exhaustion loop and control-character archive rejection. New tests/cli-surface.sh covers: - box ls: empty registry, populated registry (running + missing-from-smolvm rows), and --all (which needed the smolvm fixture's bare `machine ls` filled in — it previously fell through to exit 2). - box rm: --force happy path, the non-interactive refusal path (run under setsid to genuinely detach the controlling terminal — `[[ -r /dev/tty ]]` on Linux is a permission-bits check, not proof a terminal exists, so a plain stdin redirect doesn't reach that code path the way it would on a real interactive shell), and the "type the name to confirm" mismatch vs. match cases via a real pty (python's pty.fork(), since that prompt reads from /dev/tty directly and can't be faked with ordinary redirection). - box stop/start: delegate to the right smolvm command, and a held per-box lock blocks a concurrent stop before smolvm is ever invoked. - box enter: execs the expected `smolvm machine exec -it ... -- login -f agent` command line. - _assign_port: pre-registers a 1-port range as occupied and confirms the documented "no free host port in lo-hi" exhaustion message. tests/lifecycle-smoke.sh's existing archive-safety fixture generator gained two more cases (control character in a member name, and in a symlink's link target) alongside the eleven it already covered, confirming _validate_archive's safe_text() check end to end through both the direct validator and the box load path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
make check is fast and fully hermetic (the tests/fixtures/smolvm stub means no real VM/KVM/network access is needed), so every regression the suite catches previously depended on a human remembering to run it locally before merging. Added .github/workflows/check.yml, installs shellcheck + jq (python3 is already on the runner image) and runs `make check`. Updated the README and the pinned assertion in tests/static.sh that previously asserted CI didn't exist yet — left it as a real assertion (checks the workflow file exists and actually runs `make check`) rather than quietly deleting the check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment on lines
+740
to
+743
| cmd_load() { | ||
| local file="${1:?usage: box load <archive.tar.gz.gpg> [name]}" n="${2:-restored}" raw guest_tmp | ||
| local keep_on_failure=0 log_file failed_log="" | ||
| [[ "${BOX_KEEP_ON_FAILURE:-0}" != 1 ]] || keep_on_failure=1 |
There was a problem hiding this comment.
cmd_load only reads BOX_KEEP_ON_FAILURE; the new --keep-on-failure CLI flag is not accepted for load. Running ./box load backup.tar.gz.gpg --keep-on-failure treats the flag as the restored box name and fails _validate_name, so the inspection path is unavailable for load failures unless callers set the env var.
Artifacts
Repro: shell harness that runs the failing load command with a dummy archive
- Contains supporting evidence from the run (text/x-shellscript; charset=utf-8).
Repro: command output showing --keep-on-failure rejected as an invalid box name
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: box
Line: 740-743
Comment:
**Parse load keep flag**
`cmd_load` only reads `BOX_KEEP_ON_FAILURE`; the new `--keep-on-failure` CLI flag is not accepted for `load`. Running `./box load backup.tar.gz.gpg --keep-on-failure` treats the flag as the restored box name and fails `_validate_name`, so the inspection path is unavailable for load failures unless callers set the env var.
How can I resolve this? If you propose a fix, please make it concise.
tests/regressions.sh had grown to 1131 lines covering ~10 unrelated concern areas in one flat file, so a single failing assertion meant scanning up to 1131 lines for context. The tempdir+trap+cleanup boilerplate was also independently reimplemented (with slightly different rigor) in three other test files, and make_repo() was duplicated inline in lifecycle-smoke.sh. Extracted tests/lib.sh: the canonical tmp-dir+trap setup (with the same signal-to-exit-code mapping box itself uses — 129/130/143 — including the self-check assertions that previously only lived in regressions.sh), wait_for_file/wait_for_pattern, and make_repo(). All four pre-existing test files (lifecycle-smoke.sh, hermes-state.sh, plus the new cli-surface.sh) now source it instead of duplicating their own version. Split regressions.sh into five files by concern, grouped by what they actually source/invoke (verified with a line-by-line diff against the original to confirm zero content was lost or duplicated across the split): - regressions-tx9-host.sh: host-preflight checks, ops/tx9-host health/supervise/backup-credential behavior - regressions-box-core.sh: provisioning ownership (own_hermes_home), restore permission modeling, resource defaults, Git bundle verification - regressions-hb-workload.sh: guest/hb + guest/hb-workload behavior (reconcile, bridges, gateway enable/disable, doctor, MCP wiring) - regressions-doctor-mcp.sh: box's cmd_doctor + the MCP protocol-health suite, plus cmd_open/cmd_repair lifecycle guards - regressions-lock-rollback.sh: registry/lock/rollback races, save/restore failure injection Makefile's `test` target and SHELL_FILES now glob tests/regressions-*.sh (and tests/static.sh mirrors the same glob for its own executable-bit and syntax checks), so adding a new regression-category file doesn't require touching either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The first CI run failed lint: apt's packaged shellcheck flagged `[[ cond ]] && cmd || true` as SC2015 across several pre-existing lines (box, guest/hb, provision/provision.sh) and the (now-split) test files. Current shellcheck deliberately doesn't apply SC2015 to `[[ ]]` tests (it's a clean boolean test, not a command whose own failure needs separate handling) — confirmed locally with shellcheck 0.11.0, which reports nothing for that pattern. Ubuntu's apt repos lag upstream releases, so `apt-get install shellcheck` pulled an older version with the stricter behavior. Download the pinned v0.11.0 release binary directly instead, matching what local development already runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This invocation of box ls was missing PATH="tests/fixtures:$PATH", so _host_preflight basic's `command -v smolvm` silently resolved to a real smolvm binary on dev machines that have one installed, masking the gap locally. CI has no real smolvm, so the first real run failed with "smolvm not found on PATH" — exactly the kind of gap make check running in CI (P2-3) exists to catch. Verified by re-running the full suite with the real smolvm hidden from PATH (filtering it out of every PATH directory before invoking make check), matching what CI sees; everything passes now, including this case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
HERMES_IMPORT_PATH_MAP defaulted to a Davis-specific macOS path (/Users/davis=...), shipped as the committed default in box.env. Anyone else cloning the repo got a path mapping wrong for them, silently applied to every import-hermes invocation. Defaults to empty now. The --map OLD=NEW per-invocation flag already covers this (docs/nexus-operations.md's migration runbook already demonstrates it), so a non-empty default isn't structurally required; operators who want a standing default can still set one in their own deployment's box.env. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| [[ ! -e "$QUIESCE_FILE" && ! -e "$GATEWAY_DISABLED" ]] || return 0 | ||
| command -v hermes >/dev/null 2>&1 || { echo "hermes not installed" >&2; return 1; } | ||
| _acquire_gateway_lock || { echo "could not acquire Hermes gateway lock" >&2; return 1; } | ||
| trap _release_gateway_lock RETURN |
There was a problem hiding this comment.
This RETURN trap is never unset, so after _start_gateway returns the shell keeps running _release_gateway_lock on later function returns. A later hb operation in the same process can remove gateway.lock while another process owns it, reopening the double-spawn race this lock is meant to close. Capture the previous trap or add trap - RETURN before returning from _start_gateway.
Artifacts
Repro: focused bash harness for lingering RETURN trap and lock deletion
- Contains supporting evidence from the run (text/x-shellscript; charset=utf-8).
Repro: harness output showing lingering RETURN trap deletes recreated gateway.lock
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: guest/hb
Line: 105
Comment:
**Clear return trap**
This `RETURN` trap is never unset, so after `_start_gateway` returns the shell keeps running `_release_gateway_lock` on later function returns. A later `hb` operation in the same process can remove `gateway.lock` while another process owns it, reopening the double-spawn race this lock is meant to close. Capture the previous trap or add `trap - RETURN` before returning from `_start_gateway`.
How can I resolve this? If you propose a fix, please make it concise.All three units already ran as the unprivileged tx9 user, but none set any of the standard cheap hardening directives, despite needing nothing outside /opt/tx9, /var/backups/tx9, /var/lib/tx9, and /etc/tx9. Added to each [Service] block: NoNewPrivileges=true, ProtectHome=true, ProtectSystem=strict, PrivateTmp=true, and ReadWritePaths scoped to those four paths. Verified rather than assumed: - systemd-analyze verify accepts all three units with the new directives (no syntax/semantic errors). - /dev/kvm read/write access is unaffected — confirmed live, since these directives only restrict filesystem mounts, not device nodes; KVM access stays governed by the existing SupplementaryGroups=kvm membership. - ReadWritePaths mechanics actually grant writes inside listed paths and block them outside, confirmed with systemd-run against scratch directories using the identical directive set. - Ran the real backup flow (mkdir destination, write .partial, sha256sum, atomic rename) end to end under the exact hardening directives via systemd-run with a stubbed box/tx9-host — succeeds. Found and fixed a real gap while doing this: TX9_NAS_DIR is an operator-configured path (e.g. a CIFS mount) that ProtectSystem=strict's static ReadWritePaths can't anticipate. Documented in docs/nexus-operations.md that enabling NAS replication now requires a `systemctl edit` drop-in adding that path to ReadWritePaths, or replication silently starts failing closed (the local backup still succeeds either way). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both /var/backups/tx9 and any configured NAS target grew unbounded
forever — neither box save nor tx9-backup@.service ever pruned anything,
and the CIFS automount was documentation-only (no .mount/.automount unit
in the repo).
- ops/systemd/mnt-davis\x2dvault-tx9.{mount,automount}: a starting-point
CIFS automount matching the /mnt/davis-vault/tx9 example already in
docs/nexus-operations.md. Verified clean with systemd-analyze verify.
Filename matters here — confirmed the correct systemd-escaped name via
`systemd-escape --path` (only the hyphen inside "davis-vault" escapes
to \x2d; the separator before "tx9" doesn't), since the audit notes for
this item had it subtly wrong.
- ops/tx9-backup-prune <box>: keeps the newest TX9_BACKUP_RETAIN_COUNT
backups (default 14) per box, with an optional TX9_BACKUP_RETAIN_DAYS
cap applied on top (never keep something older than N days even if
it's within the count). Prunes both TX9_BACKUP_DIR and TX9_NAS_DIR,
skips a missing NAS directory rather than failing, and never touches
another box's backups (matched by filename prefix).
- ops/systemd/tx9-backup-prune@.{service,timer}: same hardening
directives as the other units (P3-2), runs daily after the backup
timer.
Verified live: built synthetic backup sets spanning 20 days and confirmed
count-only pruning, the count+days intersection, idempotent re-runs,
graceful handling of a missing NAS directory, and input validation —
matching what the new automated regression test in
tests/regressions-tx9-host.sh now covers going forward. Also confirmed
ProtectSystem=strict's static ReadWritePaths can't anticipate an
operator-configured TX9_NAS_DIR, and documented the systemctl edit
drop-in needed to avoid silently-failing-closed replication.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tent README.md and docs/usage-guide.html independently maintained the same command table, config table, and backup/restore guarantees prose, with no generation step tying them together — a guaranteed drift source as either file gets updated. usage-guide.html also isn't actually rendered anywhere (no Pages config, no build step; GitHub shows raw .html as source), so keeping it self-contained for "offline" use wasn't buying anything that justified the duplication-maintenance cost. Picked README as canonical (it's what renders on GitHub and what tests/static.sh already greps against, per the audit's own reasoning). usage-guide.html keeps its distinct narrative/tutorial framing and walkthrough commands, but the three literally-duplicated, easy-to-drift pieces now link to README instead of restating it: the command table, the configuration table (pointed at box.env directly, since that's the actual source of truth for what each setting does — there wasn't a matching table in README to begin with), and the backup/restore guarantees. Verified the HTML still parses with balanced tags after trimming. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| ProtectHome=true | ||
| ProtectSystem=strict | ||
| PrivateTmp=true | ||
| ReadWritePaths=/opt/tx9 /var/backups/tx9 /var/lib/tx9 /etc/tx9 |
There was a problem hiding this comment.
🟡 Medium systemd/tx9-backup-prune@.service:14
ProtectSystem=strict makes the entire filesystem read-only except for the paths listed in ReadWritePaths. The prune script targets TX9_NAS_DIR (e.g. /mnt/davis-vault/tx9) in addition to /var/backups/tx9, but TX9_NAS_DIR is not in ReadWritePaths. Every rm -f the script issues against the NAS mount fails with EROFS, so NAS retention never runs even though the service installs without error. Add the NAS mount path to ReadWritePaths, or have the unit read TX9_NAS_DIR and include it dynamically.
-ReadWritePaths=/opt/tx9 /var/backups/tx9 /var/lib/tx9 /etc/tx9
+ReadWritePaths=/opt/tx9 /var/backups/tx9 /var/lib/tx9 /etc/tx9 /mnt/davis-vault/tx9Also found in 2 other location(s)
ops/tx9-backup-prune:70
TX9_NAS_DIRpruning is wired into the new script at line70, but the shippedtx9-backup-prune@.serviceruns withProtectSystem=strictandReadWritePaths=/opt/tx9 /var/backups/tx9 /var/lib/tx9 /etc/tx9. Any NAS path such as the documented/mnt/davis-vault/tx9is therefore read-only inside the service. Whenprune_dirreaches an old NAS backup and executesrm -f, the unit fails withRead-only file system, so scheduled pruning never works for NAS backups.
docs/nexus-operations.md:93
The new sandbox guidance is too narrow:
tx9-backup@.serviceonly allows writes under/opt/tx9 /var/backups/tx9 /var/lib/tx9 /etc/tx9, but this section tells readers to add a drop-in only whenTX9_NAS_DIRis outside those paths. A box that setsTX9_BACKUP_DIRto any other location will fail the same way—the backup job cannot write its primary archive there underProtectSystem=strict—yet the runbook never warns about that case or tells operators to add the custom backup directory toReadWritePaths=.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ops/systemd/tx9-backup-prune@.service around line 14:
`ProtectSystem=strict` makes the entire filesystem read-only except for the paths listed in `ReadWritePaths`. The prune script targets `TX9_NAS_DIR` (e.g. `/mnt/davis-vault/tx9`) in addition to `/var/backups/tx9`, but `TX9_NAS_DIR` is not in `ReadWritePaths`. Every `rm -f` the script issues against the NAS mount fails with `EROFS`, so NAS retention never runs even though the service installs without error. Add the NAS mount path to `ReadWritePaths`, or have the unit read `TX9_NAS_DIR` and include it dynamically.
Also found in 2 other location(s):
- ops/tx9-backup-prune:70 -- `TX9_NAS_DIR` pruning is wired into the new script at line `70`, but the shipped `tx9-backup-prune@.service` runs with `ProtectSystem=strict` and `ReadWritePaths=/opt/tx9 /var/backups/tx9 /var/lib/tx9 /etc/tx9`. Any NAS path such as the documented `/mnt/davis-vault/tx9` is therefore read-only inside the service. When `prune_dir` reaches an old NAS backup and executes `rm -f`, the unit fails with `Read-only file system`, so scheduled pruning never works for NAS backups.
- docs/nexus-operations.md:93 -- The new sandbox guidance is too narrow: `tx9-backup@.service` only allows writes under `/opt/tx9 /var/backups/tx9 /var/lib/tx9 /etc/tx9`, but this section tells readers to add a drop-in only when `TX9_NAS_DIR` is outside those paths. A box that sets `TX9_BACKUP_DIR` to any other location will fail the same way—the backup job cannot write its primary archive there under `ProtectSystem=strict`—yet the runbook never warns about that case or tells operators to add the custom backup directory to `ReadWritePaths=`.
Comment on lines
+14
to
+15
| ReadWritePaths=/opt/tx9 /var/backups/tx9 /var/lib/tx9 /etc/tx9 | ||
| ExecStart=/opt/tx9/ops/tx9-backup-prune %i |
There was a problem hiding this comment.
ops/tx9-backup-prune also deletes old files from TX9_NAS_DIR, but this unit runs with ProtectSystem=strict and only allows writes to the listed local paths. With the documented NAS example under /mnt/davis-vault/tx9, the timer can read the mount but rm fails, so NAS retention remains unbounded unless operators add a second drop-in that the docs only mention for tx9-backup@.
Artifacts
Repro: sandbox harness running the real prune script with NAS excluded from writable paths
- Contains supporting evidence from the run (text/x-shellscript; charset=utf-8).
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ops/systemd/tx9-backup-prune@.service
Line: 14-15
Comment:
**Allow NAS pruning**
`ops/tx9-backup-prune` also deletes old files from `TX9_NAS_DIR`, but this unit runs with `ProtectSystem=strict` and only allows writes to the listed local paths. With the documented NAS example under `/mnt/davis-vault/tx9`, the timer can read the mount but `rm` fails, so NAS retention remains unbounded unless operators add a second drop-in that the docs only mention for `tx9-backup@`.
How can I resolve this? If you propose a fix, please make it concise.Code review (local multi-angle review plus CodeRabbit/Macroscope/Greptile on the PR) surfaced real bugs in the P1-1 gateway lock and exposed that _executor_up had the identical unlocked check-then-spawn race the lock was built to fix, just for the other daemon: - guest/hb's mkdir+pid-file lock (and an mv-based "TOCTOU fix" attempt to it) both had a confirmed race in stale-lock reclaim: two contenders that both observe a dead owner can each believe they're entitled to reclaim, and one's cleanup can destroy a lock the other has already freshly re-acquired. Verified directly — 5-8 of 10 racing processes would "win" the critical section in repeated trials. Replaced with flock, which is kernel-enforced and needs no manual dead-owner detection at all (auto-released the instant every fd referencing it closes). - Generalized the lock into _acquire_daemon_lock/_release_daemon_lock and put _executor_up's check-then-spawn behind it too, closing the same double-spawn class of bug for Executor that P1-1 only closed for the gateway. - Found and fixed a second bug introduced while building the flock version: a long-lived daemon spawned while the lock fd is still open inherits it, and (since flock locks attach to the open file description, not a single fd) that inherited copy keeps the lock held for the daemon's entire lifetime if the spawning shell ever dies before explicitly releasing — wedging every future acquire attempt. Added _spawn_without_lock_fd, which closes the lock fd inside the spawned subshell before exec'ing the daemon. Verified: a stand-in long-lived process spawned without this wrapper kept the lock held after the spawner explicitly released it; with the wrapper, a fresh acquire succeeded in milliseconds. - Removed the RETURN trap _start_gateway used to release its lock — confirmed empirically that a RETURN trap set inside a function fires again on every subsequent return up its call stack (not just its own), so e.g. calling _start_gateway from gateway_enable's failure branch could release the lock a second time after _stop_gateway's up-to-30s retry loop, a window wide enough for another process to have legitimately re-acquired it. Replaced with explicit release calls at every return point. - gateway_enable's failure handler called _stop_gateway unconditionally, including when _start_gateway failed only because it lost the lock race to a concurrent legitimate operation — which would kill that other operation's gateway. _start_gateway now returns a distinct code (2) for "couldn't acquire the lock" vs. 1 for a real failure, and gateway_enable only stops the gateway for the latter. Verified live end-to-end in a real guest VM, not just locally: killed Executor, fired 10 concurrent `hb reconcile` calls, and confirmed via `ps -ef` (not just log lines) that exactly one process actually spawned it while the other nine correctly observed "already running." Added a regression test exercising the lock's mutual exclusion and the spawn-without-fd-leak property so this doesn't silently regress. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two real bugs caught by review bots: - `for f in tests/regressions-*.sh; do ./"$f"; done` doesn't fail fast: a failing script in the middle of the glob doesn't stop the loop, and make only sees the *last* iteration's exit status — an earlier regression failure could be silently swallowed by a later file succeeding, defeating the whole point of running this in CI. Added `|| exit 1`. - check-hermes-pin piped curl directly into sha256sum; a failed curl produces zero bytes, and sha256sum of zero bytes is still a valid-looking (just wrong) hash, so the existing `-z "$live"` empty check never caught it — a network failure would misleadingly report "does NOT match upstream" instead of "fetch failed". Now downloads to a temp file first and checks curl's own exit status before hashing. Also switched to detecting shasum as a fallback hasher, since plain sha256sum isn't on stock macOS (the project's other supported host). Verified: forced a curl failure against an unreachable host and confirmed the target now reports "failed to fetch" with exit 1, instead of quietly hashing zero bytes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…traversal Two bots (Macroscope, Greptile) flagged that install_hermes's symlink ($OPT/bin/hermes -> $HOME/.local/bin/hermes, where $HOME is root's home since the script runs as root) requires agent to traverse root's home directory, and that a default 700 root home would block that — citing it as a likely regression. Verified live both ways: on the project's actual ubuntu:24.04 base image, /root is 755 by default, and agent could already execute hermes through the symlink chain (not just resolve it via command -v — actually ran `hermes --version` as agent). Then verified the converse: manually set /root to 700 in the same running box, confirmed agent immediately lost the ability to run hermes (exit 127), then applied the fix and confirmed it restored execution. So the bots' specific claim didn't hold against the actual deployed image, but the underlying concern — relying on an upstream base image's default permissions rather than asserting what's actually required — is real and worth closing regardless of what's true today. install_hermes now explicitly chmod o+x's the exact path components agent needs to traverse, right after creating the symlink. Also added a clarifying note to the P1-5 audit entry: a bot flagged that its illustrative code snippet resets the byte counter inside the loop, which would let a bomb split across many small entries evade the cap. Confirmed the actual implementation in guest/hermes-state was never written that way (the counter is declared once, outside the per-entry loop) — the snippet was just illustrative shorthand that didn't show the necessary placement. Added a note so a future reader doesn't copy it literally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment on lines
+95
to
+97
| if command -v hermes >/dev/null 2>&1 && [[ "$(git -C "$install_dir" rev-parse HEAD 2>/dev/null)" == "$sha" ]]; then | ||
| log "hermes already installed at pinned $sha — skipping reinstall" | ||
| return 0 |
There was a problem hiding this comment.
🟡 Medium provision/provision.sh:95
The early-return guard at line 95 skips own_hermes_home, which is the only step that fixes /data/home/agent/.hermes ownership back to agent:agent after root-run installs. In assets repair mode, when Hermes is already pinned but its state dir is root-owned, provisioning succeeds yet the agent user still cannot write Hermes state. Consider calling own_hermes_home before returning, or moving it above the guard.
if command -v hermes >/dev/null 2>&1 && [[ "$(git -C "$install_dir" rev-parse HEAD 2>/dev/null)" == "$sha" ]]; then
+ own_hermes_home
log "hermes already installed at pinned $sha — skipping reinstall"
return 0🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @provision/provision.sh around lines 95-97:
The early-return guard at line 95 skips `own_hermes_home`, which is the only step that fixes `/data/home/agent/.hermes` ownership back to `agent:agent` after root-run installs. In `assets` repair mode, when Hermes is already pinned but its state dir is root-owned, provisioning succeeds yet the `agent` user still cannot write Hermes state. Consider calling `own_hermes_home` before returning, or moving it above the guard.
- tests/cli-surface.sh's no-controlling-terminal box rm test needs setsid/timeout (util-linux), not on stock macOS — the project's other supported host. Skip with a clear message instead of failing the whole suite when they're unavailable, matching the existing IPv6-unavailable skip pattern elsewhere in the suite. Verified the skip path actually triggers by building a PATH that excludes only those two binaries. - tests/hermes-state.sh's P1-5 regression test used three bare tempfile.mkdtemp() calls with no cleanup, leaking directories under /tmp on every test run. Switched to TemporaryDirectory() context managers. - check-hermes-pin's curl call had no timeout, so a hung connection would block the target indefinitely (low-impact since it's excluded from `make check`, but cheap to fix). - Clarified the EXECUTOR_VERSION comment in box.env: "executor" is a generic npm package name, so note which actual project it resolves to rather than just repeating the ambiguous `npm view` command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t saves Three real gaps in the P0-3 failure-handling work, caught by CodeRabbit and Greptile: - The transcript-save block (mkdir backups/.failed + cp) ran under set -e with no failure guard. A full disk or read-only checkout there would abort the handler *before* keep-on-failure's _clear_created ran — so the one thing an operator needs most (the box preserved for inspection) would silently not happen, in exactly the failure conditions where it matters most. Extracted _save_failed_transcript(), which never lets a logging failure propagate past a warning. - cmd_load's second failure path (the long _pause_box/restore/gateway- disable/verify-state/resume/prepare-runtime chain) never teed _provision_into "$n" assets's output and never saved any transcript before rollback, unlike cmd_new and cmd_load's first failure path. Now passes log_file through and calls _save_failed_transcript there too. - cmd_load only read BOX_KEEP_ON_FAILURE, not the --keep-on-failure flag cmd_new accepts — load <file> [name] had a free third positional slot for it, so there was no structural reason for the asymmetry. Added it, and fixed the keep-on-failure message in the second failure path (it previously said "attempting cleanup" even when keep-on-failure had just left the box in place specifically to skip cleanup). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Flagged independently by Macroscope and a local review pass: unlike install_hermes's idempotency guard (which checks both presence and the pinned git SHA), install_executor only checked `command -v executor` — once EXECUTOR_VERSION is pinned and bumped, `box repair` (assets mode) would log "executor already installed — skipping reinstall" and silently leave the box on the old version forever, exactly the kind of drift P1-4 was meant to close. Now compares the installed executor's actual version (parsed from `executor --version`) against EXECUTOR_VERSION when one is pinned, and only skips reinstall on a real match. Unpinned (empty EXECUTOR_VERSION) keeps the original presence-only/latest-on-install behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cross-reference from the historical audit entry to the later flock-based replacement, so a reader following P1-1's fix steps doesn't implement the version with the confirmed TOCTOU bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
|
||
| cmd_load() { | ||
| local file="${1:?usage: box load <archive.tar.gz.gpg> [name]}" n="${2:-restored}" raw guest_tmp | ||
| local file="${1:?usage: box load <archive.tar.gz.gpg> [name] [--keep-on-failure]}" n="${2:-restored}" raw guest_tmp |
There was a problem hiding this comment.
🟡 Medium box:755
cmd_load assigns n="${2:-restored}" before checking for --keep-on-failure, so the invocation ./box load <file> --keep-on-failure parses the flag as the box name. _validate_name then rejects --keep-on-failure and the restore never starts, even though the usage string advertises the flag as usable without an explicit name. Consider detecting --keep-on-failure in $2 before assigning it to n, or parsing positional and flag arguments separately.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @box around line 755:
`cmd_load` assigns `n="${2:-restored}"` before checking for `--keep-on-failure`, so the invocation `./box load <file> --keep-on-failure` parses the flag as the box name. `_validate_name` then rejects `--keep-on-failure` and the restore never starts, even though the usage string advertises the flag as usable without an explicit name. Consider detecting `--keep-on-failure` in `$2` before assigning it to `n`, or parsing positional and flag arguments separately.
added 2 commits
June 30, 2026 22:12
…d Go rewrite The initialize/validate/delete-session JSON-RPC sequence was byte-for-byte duplicated between box's _mcp_initialize and guest/hb's _mcp_auth_works (~55 lines). Extract it into guest/lib-mcp.sh, sourced by both, so the security-sensitive handshake logic can't drift between host and guest checks. Also record why a Go rewrite was evaluated and deferred (docs/future-go-port.md), so the reasoning doesn't need to be re-derived if it comes up again.
CodeRabbit flagged that cmd_load collapsed base-VM creation and provisioning into one generic "destination provisioning failed" message, unlike cmd_new (which already reports the specific failing stage). Apply the same stage pattern here.
Contributor
Author
|
Triaged the CodeRabbit/Macroscope/Greptile findings from earlier pushes against the current code (HEAD f564a22). Auto-resolved by the platform (marked "No longer relevant as of " or "✅ Addressed in commits...") once later
Manually verified as false positives / already covered (auto-resolution markers were coincidental line-drift from the unrelated
Fixed (f564a22): Skipped, documented (audit-doc narrative precision, not functional code issues):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
box newfailure path on a clean checkout.verify_requiredfailure.Testing
docs/audit-2026-06-30-fixes.mdand does not modify runtime code.Note
Add failure diagnostics and audit fixes for
box newclean-checkout failures--keep-on-failure/BOX_KEEP_ON_FAILURE=1tobox newandbox load, preserving the VM for inspection when provisioning fails and saving a transcript underbackups/.failed/boxandguest/hb, removing duplicate implementationsguest/hbto serialize gateway and executor startup and avoid concurrent double-start racestests/regressions-*.shscripts run bymake testmake checkon push and pull requests to mainops/tx9-backup-pruneto bound backup growth by count and optional age, with supporting systemd timer and CIFS automount unitshermes-stateextraction to enforce theMAX_UNCOMPRESSEDcap on actual decompressed bytes rather than trusting zip metadataMacroscope summarized f564a22.
Greptile Summary
This PR fixes the clean-checkout
box newandbox loadfailure path and adds supporting safeguards. The main changes are:--keep-on-failureandBOX_KEEP_ON_FAILURE=1inspection paths.backups/.failed/.Confidence Score: 5/5
The changes are merge-safe based on the documented scope and supporting safeguards.
The implementation is focused on improving failure observability, rollback control, installer verification, daemon locking, extraction bounds, and regression coverage without identified blocking issues.
What T-Rex did
Comments Outside Diff (7)
General comment
docs/audit-2026-06-30-fixes.mdand does not modify runtime code.git diff --name-only ded8e51a1491f757f96f50106e38cbb883d5899e 92cf51ca375812f4115e609bc44162eccaa5d5d8shows additional runtime files changed:Makefile,box.env,guest/hermes-state, andprovision/provision.sh, plus test filetests/hermes-state.sh. The diffstat confirms these files contain actual modifications, not just documentation changes.General comment
box new/box loadsave provisioning transcripts underbackups/.failed/before rollback. In the head run, bothbox new --keep-on-failureandBOX_KEEP_ON_FAILURE=1 ./box newfail during the provision-context streaming step, and bothbox loadfailure cases do the same, but every case reportsfailed_logs=with no transcript file created._provision_intoonly tees the secondsmolvm machine execprovisioning command tolog_file; failures in the preceding tar-to-guestsmolvm machine exec -i ... cat >/tmp/ctx.tgzreturn before anything is written to the log file. The later[[ -s "$log_file" ]]guard therefore skips creatingbackups/.failed/<name>-timestamp.log._provision_intofailure path, including the context streaming command, into the transcript file when a log path is provided, or create a transcript with at least the failing command/stage output even when the tee'd provisioning command is not reached.General comment
new/load; the CLI advertises--keep-on-failurefornewand the PR objective says bothbox newandbox loadaccept it. The executed head run of./box load valid.tar.gz.gpg loadopt --keep-on-failurefails provisioning, runs rollback (machine delete --name loadopt -f), and leavesstate_dir_exists=no.cmd_loadonly initializeskeep_on_failurefromBOX_KEEP_ON_FAILURE; it does not parse or accept a third positional--keep-on-failureargument for load.cmd_loadargument parsing and usage to accept--keep-on-failureafter the optional destination name, setkeep_on_failure=1, and reject unknown extra arguments so typoed flags do not silently fall back to rollback.General comment
ded8e51a1491f757f96f50106e38cbb883d5899eto head658c7cb032cf115e30201d5104beebdc45091fdeshows many non-documentation/runtime-adjacent files changed, includingbox,box.env,guest/hb,guest/hermes-state, andprovision/provision.sh, plus Makefile/workflow/test changes. This materially contradicts a docs-only PR description even though the audit-plan documentation itself is present and structured as claimed.General comment
_start_gatewaycalls to executehermes gateway run --replace, producinghb_concurrent_spawn_count=2in the after artifact. This violates the PR contract that the mkdir-based lock serializes concurrent gateway start attempts to prevent double spawn.guest/hb, the lock is held only around the check and immediatenohup hermes gateway run --replacespawn._start_gatewaywrites$!and returns without waiting for_gateway_running/pgrepto observe the spawned gateway. A second caller can acquire the lock immediately after release, still observe no running gateway, and spawn another process. The relevant head code isguest/hblines 104-115.gateway.pidas an in-progress start while holding the lock. For example, after spawning, poll_gateway_runningor the child PID for a short readiness/registration window before releasing the lock; subsequent callers should see the pending/running process and avoid spawning again.General comment
box newwithBOX_KEEP_ON_FAILURE=1does not leave the VM available for inspection. The command exits 1 and prints inspection guidance, but the captured smolvm calls includemachine delete --name failbox -f, the state listing contains only/tmp/kf-state, and the failed transcript listing is empty. This violates the claimed CLI/API contract thatBOX_KEEP_ON_FAILURE=1or--keep-on-failureskips rollback and saves a non-empty provisioning transcript underbackups/.failed/<name>-<timestamp>.log.cmd_new/_provision_intoinboxdoes not reliably suppress the EXIT-trap rollback or persist a transcript for this provisioning failure path. In the exercised path, cleanup still runs_rollback_createdafter the failure despite keep-on-failure being requested.cmd_newfailure path inboxso that whenkeep_on_failureis enabled it removes the box from the cleanup rollback list before any exit path can run, and ensure_provision_intocaptures a non-empty transcript for failures after the VM is created. Add a regression test using the smolvm fixture that asserts nomachine deletecall occurs, the VM state directory remains, guidance is printed, andbackups/.failed/*.logexists and is non-empty.General comment
box new/box loadprovisioning should save a host-side transcript underbackups/.failed/<name>-<UTC>.logbefore rollback. In the head run, all failing provisioning cases reportFAILED_LOG: <directory missing>, including default rollback cases and keep-on-failure cases for bothnewandload. The same run confirms other contract pieces are active: stage-specific errors and rollback skipping when requested._save_failed_transcriptonly copies when the temporary log is non-empty ([[ -s "$log_file" ]] || return 0). The exercised provisioning failure path fails before the transcript-producingtee "$log_file"command runs, so no log file is persisted even though the operation has a provisioning-stage transcript/diagnostic output and the user contract says failed new/load saves one.teeruns. For example, write the streaming/provisioning command output and failure diagnostics into the log file around both context upload and provisioning execution, and save even if only diagnostic text is available; then add regression assertions thatbackups/.failedcontains a non-empty log for failednewandloadboth with and without keep-on-failure.Reviews (12): Last reviewed commit: "fix(review): cmd_load reports which stag..." | Re-trigger Greptile