chore: round-2 quality bundle — mandatory forward-upgrade leg, checkout preflight, polish sweep, honest baseline policy (#350, #353, #362, #354) - #365
Merged
Conversation
…aging traps, appliance disable --runtime, comment splices, reader dedup Six small debts from the 2026-08-14 audit, bundled per the issue: 1. CURRENT_STEP was only ever set inside main() (setup), so an unexpected failure in any other verb reported the stale "aborted while starting up" default. Set it once at dispatch time from the verb name itself — setup's own fine-grained per-phase steps still overwrite it a moment later. 2. _apply_plan's step 3 named 3 reconciles (autotune/API/firewall) while apply() actually runs 6 install_* calls, silently omitting watchdog and control from the printed plan. Named all of them; added the same name-vs-plan drift guard _setup_plan already has for main(). 3. backup/restore/support_bundle's mktemp -d staging (config.json, tokens) leaked on a set -e abort — the explicit `rm -rf` cleanup lines were never reached. Gave all three the EXIT-trap treatment tune() got in #135. (The trap has to reference a script-global, not a `local`: a `local` staging var is out of scope by the time the trap fires on a normal return, and dies "unbound variable" under set -u — caught by the existing backup/restore/support-bundle black-box tests.) 4. Verified empirically against a real systemd (255, enable/disable round-trip in a container) that a plain `systemctl disable` only ever removes the /etc-side wants-symlink: a --runtime-enabled unit's /run symlink survives, and `is-enabled` still reports "enabled-runtime". Every disable call now passes ${ENABLE_RUNTIME:+...}, mirroring its enable counterpart. 5. Two comment splices: xmrig_already_built()'s docstring was sitting above the unrelated _sha256() instead of itself; a section-header banner (`--- Auto-tuning: search strategies & seeding ---`) had landed mid- sentence inside _hillclimb()'s docstring. 6. _read_api_summary and _xmrig_summary_json were near-identical /2/summary readers, differing only in an API_CMD hook and whether a curl failure propagates or is swallowed. Merged into one function with a propagate/swallow mode argument, preserving each call site's original behavior exactly. Tests: tests/run.sh gains one section per item above (black-box + unit), 153 -> 158 sections. Full suite: 1685 passed, 0 failed (up from 1668 baseline). make lint clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pt-in (#350) tests/e2e-real.sh's upgrade phase only ever proved the two REFUSAL legs (noop, forged-tag rollback) unless an operator remembered to set E2E_UPGRADE_TARGET — so a broken fetch/rebuild path could pass the release gate silently. The forward leg is now on by default. It auto-derives the previous real release tag and the current/installed one from `git tag` (the release this gate is actually cutting has no tag yet at this point in RELEASING.md's flow, so "current" stands in for it), rewinds the checkout to the previous tag out of band (git checkout + `rigforge.sh upgrade`, the same two steps _control_upgrade_do takes — D10's anti-rollback in control_upgrade() refuses a downgrade POST, so this can't happen through the control channel itself), then drives a real forward upgrade back to current through the same wire/path-unit/oneshot chain the other legs use. _upgrade_cleanup restores the checkout to exactly where the phase started on any exit, success or a hard abort mid-leg — unlike the old opt-in leg (which permanently advanced the checkout past "current"), this one is repeatable, which is what lets it be the default. E2E_UPGRADE_TARGET=vX.Y.Z still works as an explicit override (same PERMANENT semantics as before — useful for a deliberate real deploy). E2E_UPGRADE_SKIP_REASON="..." is the new escape hatch: it skips the leg but requires (and logs) a reason, so a skip is never silent. RELEASING.md's `upgrade` line updated to describe the new default and the skip knob. Needs live-rig validation — this whole leg (the rewind, the real fetch/build through the control channel, the restore) has no automated coverage; it can only run for real on the release rig. See #362's require_traversable_checkout for the one piece of this file that IS unit-tested via extraction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…amicUser (#362) Found live (miner-2): running tests/e2e-real.sh from a checkout under $HOME (mode 750) makes the control phase fail 6 asserts confusingly — the receiver dies in a restart loop with "can't open file .../util/control- server.py: Permission denied", because rigforge-control.service and rigforge-api.service run as systemd DynamicUser and can't traverse the home directory. The phase then reads as a product failure (receiver down, POST 000, DONATION unchanged) when it's a harness-placement problem — nothing pointed at the path. require_traversable_checkout() walks every ancestor directory from the checkout up to /, checking the "others" execute bit on each (root can always stat regardless of permissions, so this never false-fails), and dies immediately with the fix (move the checkout to /opt) if any of them block traversal. Wired into require_linux_root(), which every phase already calls first — so a lone `control` or `upgrade` re-run gets the same immediate, honest failure, not just a fresh `provision`. Split into its own function (rather than inlined in require_linux_root) so it's unit-testable in isolation via the same extraction+eval technique tests/run.sh already uses for rig_lock/_perf_judge: `stat` is faked, so the test's result never depends on the real host's tmp/HOME permissions (which turned out to vary by OS during development — macOS's own mktemp -d default and its TMPDIR parent are both mode 700). One line added to the header comment block per the issue's ask. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tually delivers (#354) RELEASING.md said to record each rig's benchmark after every fleet re-tag. In practice only miner-0 (the dedicated release-gate rig, per tests/README.md) has a current entry every release; miner-1..7 stop at v1.11.1 — three releases of fleet perf history the fleet was never actually re-tagged for, or the recording step was skipped. Rewrote the paragraph to describe the real policy instead of inventing one nobody follows: record whichever rig(s) you actually re-tag, which in practice means miner-0 every time (the gate always runs there) plus whatever else happens to get touched that release. tests/perf-baselines/ legitimately carries gaps between releases for untouched rigs — that's now stated as expected, not implied to be a lapse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closed
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.
Four small issues that all raise the same release-quality bar, one branch.
#362 — e2e-real pre-flights its own checkout path. Found live yesterday: running the gate from a checkout under
$HOME(mode 750) made the control phase fail six asserts, because the receiver's DynamicUser cannot traverse the home directory — it died in a restart loop withPermission denied. The phase read as a product failure when it was placement.require_traversable_checkout()now walks every ancestor to/, checks the others-execute bit, and dies immediately naming the blocking path and the/opt/rigforge-e2eremedy. Wired intorequire_linux_root()so a lonecontrolre-run gets the same early failure, not just a freshprovision. Unit-tested with a fakedstat— deliberately, because real directory modes vary by host (macOSmktemp -dis 700), which would have made a real-tree test host-dependent.#350 — the forward upgrade leg is mandatory, not opt-in. The gate previously proved only the refusals (noop + forged-tag rollback) unless someone set an env var, so a broken fetch/rebuild path could pass. The leg now auto-derives the previous release tag, rewinds out-of-band (the control channel correctly refuses a downgrade POST, per D10), and drives a real forward upgrade back to current through the same wire/path-unit/oneshot chain as the other legs.
_upgrade_cleanuprestores the checkout on any exit — that repeatability is what makes it safe as a default.E2E_UPGRADE_TARGETkeeps its explicit semantics; the newE2E_UPGRADE_SKIP_REASONrequires and logs a reason, so a bypass is never silent.#353 — six-item polish sweep. Verb-entry
CURRENT_STEPfixed at the single shared dispatch site rather than per verb;_apply_plannow names all five reconcilesapply()runs, with a drift guard mirroring the_setup_planone;backup/restore/support_bundlegot EXIT traps for their secret-bearing staging dirs; the appliance--runtimedisable gap was empirically verified against real systemd 255 in a container — plainsystemctl disableleaves a--runtime-enabled unit's/runsymlink in place andis-enabledstill saysenabled-runtime, so all 12 disable sites now mirror their enables; two comment splices repaired; and the two near-duplicate API readers merged into one with a mode argument, preserving all four call sites' behavior.Note for reviewers, in the spirit of not hiding a bruise: the trap work initially broke under
set -ubecause alocalstaging var is already out of scope when the trap fires — caught by the full suite, fixed by making it a script-global likeTUNE_TMP, reverified.#354 — RELEASING.md promises what the fleet actually delivers. Checked the real baseline files: the dedicated bench rig has every recent release, one loaner was refreshed today, the rest stop at v1.11.1. The doc now states the real policy (record the rigs you actually re-tag; the bench rig every time, since the gate always runs there) instead of an unmet fleet-wide claim.
Evidence:
make lintclean; suite 1668 → 1690 passed, 0 failed (+7 sections). Coverage could not be computed locally (the pinned staticjqis amd64-only, this host is arm64) — CI's amd64 runner is the check.Needs live-rig validation before the next cut: #350's new default path end to end, #362's real under-
$HOMEscenario, and #353's appliance disable paths.Closes #350
Closes #353
Closes #354
Closes #362
🤖 Generated with Claude Code