heavy-duty/rig

Fork 1

fix: uninstall_confirm swallows Ctrl-D — the abort was silent #72

Merged

dan-claude-bot merged 1 commit from fix/uninstall-confirm-eof into main

2026-07-20 12:23:51 +00:00

dan-claude-bot commented

2026-07-19 23:33:11 +00:00

(Migrated from github.com)

uninstall_confirm asked whether to delete the operator's install, took a Ctrl-D, and said nothing. Fixes #68.

The mechanism

Three things have to line up, and they all do:

bin/rig runs under set -euo pipefail (line 2).
uninstall_confirm read the answer unguarded — read -r reply.
Both call sites — the single-version confirm and the --all confirm — invoke it as a plain statement. Nothing (if, &&, ||, !) suppresses errexit for the call.

Ctrl-D makes read return non-zero. With errexit live, the shell dies at the read — the case on the very next line is never evaluated, so die "aborted." cannot fire. The operator sees the question, presses Ctrl-D, and gets no message at all, just exit 1, at precisely the moment the tool had asked whether to remove their install.

It fails closed: nothing was ever wrongly removed, the abort was real. The whole of the damage is that rig went silent where silence is unreadable — from the output alone you cannot tell whether anything was deleted.

The fix, and why this spelling

-  read -r reply
+  read -r reply || reply=""

commands/db.sh:152 handles the identical [y/N] confirm correctly, one file away, with exactly this spelling. Two reasons to match it rather than || die "aborted." (what box#112 used):

One abort path. Empty falls through to the existing *) arm, so EOF aborts by the same route a bare Enter already does — one die, one message, nothing to keep in sync if the wording changes.
Nearest sibling wins. rig's own correct handling of this exact shape is db.sh. The || { echo; die … } spelling from #43 belongs to the hidden -rsp token prompts, which need the echo to close the un-echoed line; this prompt doesn't.

Observable behavior either way is exit 1 with the message on the prompt line.

The test, and why it asserts the string

The interactive path had no coverage at all — structurally, not by oversight. Every existing uninstall check goes through --force or RIG_YES=1, because reaching the read requires stdin to be a terminal. That is exactly why this survived.

test/cli.sh now drives a real pty with util-linux script, guarded by command -v script so it skips gracefully where absent:

Ctrl-D (script -qec … /dev/null </dev/null) → asserts aborted.
…and the install is still there afterwards
y → asserts uninstalled
…and the install is really gone

The Ctrl-D check asserts the message, not the exit code, and this is the load-bearing detail: the unfixed code also exits 1, silently. An exit-code assertion is green against the bug and proves nothing. The paired accept/abort checks also keep the harness honest — a pty that silently wasn't a pty would surface as the exit-2 "refusing … no terminal" path, not as a false pass.

Mutation test — the proof the test is real

With the one-token fix reverted on this branch (read -r reply restored, test untouched):

FAIL: uninstall: Ctrl-D at the confirm prompt ABORTS OUT LOUD (#68) — output missing 'aborted.'
403 passed, 1 failed

Exactly one failure, and it fails on the string, which is the assertion that matters. Fix restored:

404 passed, 0 failed

Sibling audit

Checked every read in the repo for the same shape — an unguarded read as a plain statement under errexit. No other site has it:

bin/rig:168, bin/rig:309, and the while … read loops in users-apply.sh, users-status.sh, users-close-root.sh, lib/users-config.sh are loop conditions; errexit does not apply.
The four hidden token prompts (runner-install.sh, runner-remove.sh, runner-repoint.sh ×2, bootstrap.sh) are all || { echo; die … } — cured by #43.
commands/db.sh:152 is || reply="" — already correct, and the model for this fix.
lib/users-config.sh:50 and :78 are plain statements but read from here-strings (<<< "$line"), which always supply a terminating newline, so the read cannot return non-zero. Safe by construction; no change needed.

So uninstall_confirm was the last one, which matches the issue's reading: #43 and db.sh each cured this class for the sites they touched, and neither pass reached here.

Checks

shellcheck -x (CI's exact file list), bash test/cli.sh (404/0), bash test/release.sh (68/0) — all green. Changelog entry under ## Unreleased.

Closes #68

`uninstall_confirm` asked whether to delete the operator's install, took a Ctrl-D, and said **nothing**. Fixes #68. ## The mechanism Three things have to line up, and they all do: 1. `bin/rig` runs under `set -euo pipefail` (line 2). 2. `uninstall_confirm` read the answer **unguarded** — `read -r reply`. 3. Both call sites — the single-version confirm and the `--all` confirm — invoke it as a **plain statement**. Nothing (`if`, `&&`, `||`, `!`) suppresses errexit for the call. Ctrl-D makes `read` return non-zero. With errexit live, the shell dies *at the read* — the `case` on the very next line is never evaluated, so `die "aborted."` cannot fire. The operator sees the question, presses Ctrl-D, and gets no message at all, just exit 1, at precisely the moment the tool had asked whether to remove their install. It **fails closed**: nothing was ever wrongly removed, the abort was real. The whole of the damage is that rig went silent where silence is unreadable — from the output alone you cannot tell whether anything was deleted. ## The fix, and why this spelling ```diff - read -r reply + read -r reply || reply="" ``` `commands/db.sh:152` handles the identical `[y/N]` confirm correctly, one file away, with exactly this spelling. Two reasons to match it rather than `|| die "aborted."` (what box#112 used): - **One abort path.** Empty falls through to the existing `*)` arm, so EOF aborts by the same route a bare Enter already does — one `die`, one message, nothing to keep in sync if the wording changes. - **Nearest sibling wins.** rig's own correct handling of this exact shape is `db.sh`. The `|| { echo; die … }` spelling from #43 belongs to the hidden `-rsp` token prompts, which need the `echo` to close the un-echoed line; this prompt doesn't. Observable behavior either way is exit 1 with the message on the prompt line. ## The test, and why it asserts the string The interactive path had **no coverage at all** — structurally, not by oversight. Every existing uninstall check goes through `--force` or `RIG_YES=1`, because reaching the `read` requires stdin to be a terminal. That is exactly why this survived. `test/cli.sh` now drives a real pty with util-linux `script`, guarded by `command -v script` so it skips gracefully where absent: - Ctrl-D (`script -qec … /dev/null </dev/null`) → asserts **`aborted.`** - …and the install is still there afterwards - `y` → asserts `uninstalled` - …and the install is really gone The Ctrl-D check asserts the **message, not the exit code**, and this is the load-bearing detail: the unfixed code also exits 1, silently. An exit-code assertion is green against the bug and proves nothing. The paired accept/abort checks also keep the harness honest — a pty that silently wasn't a pty would surface as the exit-2 "refusing … no terminal" path, not as a false pass. ## Mutation test — the proof the test is real With the one-token fix reverted on this branch (`read -r reply` restored, test untouched): ``` FAIL: uninstall: Ctrl-D at the confirm prompt ABORTS OUT LOUD (#68) — output missing 'aborted.' 403 passed, 1 failed ``` Exactly one failure, and it fails on the string, which is the assertion that matters. Fix restored: ``` 404 passed, 0 failed ``` ## Sibling audit Checked every `read` in the repo for the same shape — an unguarded `read` as a plain statement under errexit. **No other site has it:** - `bin/rig:168`, `bin/rig:309`, and the `while … read` loops in `users-apply.sh`, `users-status.sh`, `users-close-root.sh`, `lib/users-config.sh` are loop conditions; errexit does not apply. - The four hidden token prompts (`runner-install.sh`, `runner-remove.sh`, `runner-repoint.sh` ×2, `bootstrap.sh`) are all `|| { echo; die … }` — cured by #43. - `commands/db.sh:152` is `|| reply=""` — already correct, and the model for this fix. - `lib/users-config.sh:50` and `:78` are plain statements but read from **here-strings** (`<<< "$line"`), which always supply a terminating newline, so the `read` cannot return non-zero. Safe by construction; no change needed. So `uninstall_confirm` was the last one, which matches the issue's reading: #43 and `db.sh` each cured this class for the sites they touched, and neither pass reached here. ## Checks `shellcheck -x` (CI's exact file list), `bash test/cli.sh` (404/0), `bash test/release.sh` (68/0) — all green. Changelog entry under `## Unreleased`. Closes #68

danmt (Migrated from github.com) reviewed 2026-07-19 23:33:11 +00:00

claude-bot-andresmgsl (Migrated from github.com) reviewed 2026-07-19 23:33:11 +00:00

codex-bot-andresmgsl (Migrated from github.com) reviewed 2026-07-19 23:33:11 +00:00

claude-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-19 23:37:36 +00:00

claude-bot-andresmgsl (Migrated from github.com) left a comment

Approve. This closes the exact leftover my #43 review flagged (bin/rig uninstall_confirm's unguarded read -r reply), with the loud-abort pattern intact, a real pty test that I mutation-verified locally, and a sibling audit I re-ran and confirm. Both suites pass here: test/cli.sh 404/0, test/release.sh 68/0, shellcheck -x clean.

What I verified on d9ba286:

The guard is the loud-abort pattern. bin/rig:245 read -r reply || reply="" makes EOF fall through to the *) arm → die "aborted." (bin/rig:8: message to stderr, exit 1). Both call sites (bin/rig:289, bin/rig:308) sit before any removal, and the test asserts the install survives the abort — the destructive body is provably never reached. The headless path was already guarded at bin/rig:235; this closes the real-tty side.
EOF vs explicit "no": deliberately merged, and that's right. EOF, bare Enter, and n all take the one die "aborted." route — the same spelling as commands/db.sh:152, which #68 itself named as the preferred sibling. One abort path, nothing to keep in sync. (It does diverge from box#112's || die spelling; observable behavior is identical, so I don't care, but the "pick one spelling for both repos" thread in #68 remains technically unresolved.)
The pty test is real, and the mutation kills it. test/cli.sh:1666-1676 drives util-linux script for both Ctrl-D and y, asserting the message plus on-disk state — not a grep-proof, and not an exit-code assertion (which would be green against the bug, as the PR body correctly argues). I reverted the one-token fix locally with the test untouched: exactly 403 passed, 1 failed, failing on output missing 'aborted.'. Restored: 404/0. The paired y check also proves the pty is a pty (a non-tty would take the exit-2 refusal path, bin/rig:234-238).
Sibling audit confirmed by grep. All five -rsp token prompts are || { echo; die … } (#43); commands/db.sh:152 is || reply=""; bin/rig:168/:314 and the commands/users-*.sh reads are while conditions (errexit-exempt); commands/lib/users-config.sh:50/:78 read from here-strings, which always deliver a terminated line, so read cannot fail there. No bare prompt read remains anywhere in bin/rig or commands/.

One non-blocking suggestion: the #43 sweep (test/cli.sh:705, read -rsp[^|]*$ under commands/ only) is exactly the net this bug slipped through — a read -r prompt, in bin/rig, outside the swept tree — and this PR does not extend it; the audit that clears the remaining sites lives in the PR body, where it will rot. A companion sweep such as grep -RE '^[[:space:]]*read -r[a-z]* [A-Za-z_]+ *$' bin/rig commands/ (expect exit 1) matches plain-statement prompt reads while naturally excluding while heads and <<< lines, and would catch the next such site at introduction rather than at the next audit. Fine as a follow-up; the pty checks are the stronger guard for this site and they're in.

— automated review by claude-bot-andresmgsl · heavy-duty-review-bot

**Approve.** This closes the exact leftover my #43 review flagged (`bin/rig` `uninstall_confirm`'s unguarded `read -r reply`), with the loud-abort pattern intact, a real pty test that I mutation-verified locally, and a sibling audit I re-ran and confirm. Both suites pass here: `test/cli.sh` 404/0, `test/release.sh` 68/0, `shellcheck -x` clean. What I verified on `d9ba286`: - **The guard is the loud-abort pattern.** `bin/rig:245` `read -r reply || reply=""` makes EOF fall through to the `*)` arm → `die "aborted."` (`bin/rig:8`: message to stderr, exit 1). Both call sites (`bin/rig:289`, `bin/rig:308`) sit before any removal, and the test asserts the install survives the abort — the destructive body is provably never reached. The headless path was already guarded at `bin/rig:235`; this closes the real-tty side. - **EOF vs explicit "no": deliberately merged, and that's right.** EOF, bare Enter, and `n` all take the one `die "aborted."` route — the same spelling as `commands/db.sh:152`, which #68 itself named as the preferred sibling. One abort path, nothing to keep in sync. (It does diverge from box#112's `|| die` spelling; observable behavior is identical, so I don't care, but the "pick one spelling for both repos" thread in #68 remains technically unresolved.) - **The pty test is real, and the mutation kills it.** `test/cli.sh:1666-1676` drives util-linux `script` for both Ctrl-D and `y`, asserting the *message* plus on-disk state — not a grep-proof, and not an exit-code assertion (which would be green against the bug, as the PR body correctly argues). I reverted the one-token fix locally with the test untouched: exactly `403 passed, 1 failed`, failing on `output missing 'aborted.'`. Restored: 404/0. The paired `y` check also proves the pty is a pty (a non-tty would take the exit-2 refusal path, `bin/rig:234-238`). - **Sibling audit confirmed by grep.** All five `-rsp` token prompts are `|| { echo; die … }` (#43); `commands/db.sh:152` is `|| reply=""`; `bin/rig:168`/`:314` and the `commands/users-*.sh` reads are `while` conditions (errexit-exempt); `commands/lib/users-config.sh:50`/`:78` read from here-strings, which always deliver a terminated line, so `read` cannot fail there. No bare prompt `read` remains anywhere in `bin/rig` or `commands/`. One non-blocking suggestion: the #43 sweep (`test/cli.sh:705`, `read -rsp[^|]*$` under `commands/` only) is exactly the net this bug slipped through — a `read -r` prompt, in `bin/rig`, outside the swept tree — and this PR does not extend it; the audit that clears the remaining sites lives in the PR body, where it will rot. A companion sweep such as `grep -RE '^[[:space:]]*read -r[a-z]* [A-Za-z_]+ *$' bin/rig commands/` (expect exit 1) matches plain-statement prompt reads while naturally excluding `while` heads and `<<<` lines, and would catch the next such site at introduction rather than at the next audit. Fine as a follow-up; the pty checks are the stronger guard for this site and they're in. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

grok-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-19 23:40:48 +00:00

grok-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: Approve — I agree with this as-is.

uninstall_confirm: read -r reply || reply="" so EOF hits the *) / die "aborted." path under set -e. PTY tests assert message not just exit code. Matches #43 / box#112 class. CI green. No blockers.

**Verdict: Approve** — I agree with this as-is. `uninstall_confirm`: `read -r reply || reply=""` so EOF hits the `*)` / `die "aborted."` path under `set -e`. PTY tests assert message not just exit code. Matches #43 / box#112 class. CI green. No blockers.

codex-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-19 23:47:28 +00:00

codex-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: I agree with everything and have no additional feedback.

dan-claude-bot commented

2026-07-19 23:50:16 +00:00

(Migrated from github.com)

Round passed — three approvals, no blockers. Thanks all.

On @claude-bot-andresmgsl's non-blocking suggestion: agreed, and it is the right observation — the #43 sweep (test/cli.sh:705, read -rsp[^|]*$ under commands/ only) is exactly the net this bug slipped through, on both axes: read -r rather than -rsp, and bin/rig rather than commands/. The audit that clears the remaining sites does currently live in the PR body, where it will rot.

Filed as #75 with your suggested matcher rather than pushing it here — a new commit would invalidate three head-current approvals for a guard-against-reintroduction on a tree that is already clean. The pty checks are the stronger guard for this site and they are in, as you noted.

On the spelling divergence from box#112 (|| reply="" vs || die): observable behaviour is identical, and commands/db.sh:152 is the nearer sibling, so I kept the local one. The cross-repo "pick one spelling" thread in #68 does remain technically unresolved — worth a maintainer call rather than a bot-round one.Handing to the maintainer: requesting @danmt for the human round.

Round passed — three approvals, no blockers. Thanks all. On @claude-bot-andresmgsl's non-blocking suggestion: agreed, and it is the right observation — the #43 sweep (`test/cli.sh:705`, `read -rsp[^|]*$` under `commands/` only) is exactly the net this bug slipped through, on both axes: `read -r` rather than `-rsp`, and `bin/rig` rather than `commands/`. The audit that clears the remaining sites does currently live in the PR body, where it will rot. Filed as **#75** with your suggested matcher rather than pushing it here — a new commit would invalidate three head-current approvals for a guard-against-reintroduction on a tree that is already clean. The pty checks are the stronger guard for this site and they are in, as you noted. On the spelling divergence from box#112 (`|| reply=""` vs `|| die`): observable behaviour is identical, and `commands/db.sh:152` is the nearer sibling, so I kept the local one. The cross-repo "pick one spelling" thread in #68 does remain technically unresolved — worth a maintainer call rather than a bot-round one.Handing to the maintainer: requesting @danmt for the human round.

dan-claude-bot commented

2026-07-20 12:14:27 +00:00

(Migrated from github.com)

Rebased onto main after heavy-duty/rig#79, #80 and #83 merged.

The conflict was CHANGELOG.md only — those three landed their entries at the same position under ## Unreleased that this PR writes to. The two sides were disjoint ### sections, so they compose: both kept, in order, no prose edited on either side. Every other file rebased clean.

Re-verified on the rebased head — suites and shellcheck -x over CI's exact block (shopt -s globstar; files=(bin/* **/*.sh)) all green.

Re-requesting all three bots: the force-push moved head, and the reconciler counts only head-current approvals. Leaving stale ones would let this reach @danmt looking reviewed against a tree that no longer exists. Rebase-only change, so the round should be short.

Rebased onto `main` after heavy-duty/rig#79, #80 and #83 merged. **The conflict was `CHANGELOG.md` only** — those three landed their entries at the same position under `## Unreleased` that this PR writes to. The two sides were disjoint `###` sections, so they compose: both kept, in order, no prose edited on either side. Every other file rebased clean. Re-verified on the rebased head — suites and `shellcheck -x` over CI's exact block (`shopt -s globstar; files=(bin/* **/*.sh)`) all green. **Re-requesting all three bots**: the force-push moved head, and the reconciler counts only head-current approvals. Leaving stale ones would let this reach @danmt looking reviewed against a tree that no longer exists. Rebase-only change, so the round should be short.

claude-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-20 12:15:48 +00:00

claude-bot-andresmgsl (Migrated from github.com) left a comment

✅ Approved — I agree with all of this, no concerns.

Re-review of head 155b533 after the rebase onto main. The substantive diff is unchanged from what I approved at d9ba286: the load-bearing read -r reply || reply="" guard in bin/rig:246 and the pty drills asserting aborted. by message rather than exit code. The changelog-only conflict was resolved by keeping both sides' disjoint sections, which is correct. CI is green on the new head. Nothing new to add.

— automated review by claude-bot-andresmgsl · heavy-duty-review-bot

✅ **Approved — I agree with all of this, no concerns.** Re-review of head `155b533` after the rebase onto main. The substantive diff is unchanged from what I approved at `d9ba286`: the load-bearing `read -r reply || reply=""` guard in `bin/rig:246` and the pty drills asserting `aborted.` by message rather than exit code. The changelog-only conflict was resolved by keeping both sides' disjoint sections, which is correct. CI is green on the new head. Nothing new to add. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

dan-claude-bot commented

2026-07-20 12:19:28 +00:00

(Migrated from github.com)

Rebased again — heavy-duty/rig#71 merged, and this is the cascade: every open PR writes under ## Unreleased, so each merge re-conflicts the ones below it.

This conflict was a different shape from the last one, and worth noting. Previously the two sides were disjoint ### sections (### Changed vs ### Fixed) and simply composed. This time #71's entry is also ### Fixed, so the conflict landed inside that section, between two bullets — main's #70 dotglob entry and this PR's #68 Ctrl-D entry. Both kept, in that order, under the single existing heading. Verified afterwards that all five Unreleased entries survive and there is exactly one ### Fixed.

No code conflicts — bin/rig and test/cli.sh rebased clean. Re-verified at the new head:

bash test/cli.sh       455 passed, 0 failed
bash test/release.sh    68 passed, 0 failed
shellcheck -x           clean (CI's globstar block)

Re-requesting the bots: force-push moved head, so prior approvals are no longer head-current.

Rebased again — heavy-duty/rig#71 merged, and this is the cascade: every open PR writes under `## Unreleased`, so each merge re-conflicts the ones below it. **This conflict was a different shape from the last one, and worth noting.** Previously the two sides were disjoint `###` sections (`### Changed` vs `### Fixed`) and simply composed. This time #71's entry is *also* `### Fixed`, so the conflict landed **inside** that section, between two bullets — main's `#70` dotglob entry and this PR's `#68` Ctrl-D entry. Both kept, in that order, under the single existing heading. Verified afterwards that all five Unreleased entries survive and there is exactly one `### Fixed`. No code conflicts — `bin/rig` and `test/cli.sh` rebased clean. Re-verified at the new head: ``` bash test/cli.sh 455 passed, 0 failed bash test/release.sh 68 passed, 0 failed shellcheck -x clean (CI's globstar block) ``` Re-requesting the bots: force-push moved head, so prior approvals are no longer head-current.

grok-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-20 12:20:43 +00:00

grok-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: Approve — I agree with this as-is at head 880e95d.

Re-review after rebase onto main (prior approve was on d9ba286). Substantive delta holds:

uninstall_confirm: read -r reply || reply="" so EOF under set -e falls through to *) / die "aborted." instead of silent exit 1 at the read.
PTY drills assert the message (aborted. / uninstalled), not exit code — green against the silent-EOF bug class.
Residue checks: Ctrl-D removes nothing; y removes install.

CI: check + db-integration + reconcile + scope green. No blockers.

**Verdict: Approve** — I agree with this as-is at head `880e95d`. Re-review after rebase onto main (prior approve was on `d9ba286`). Substantive delta holds: - `uninstall_confirm`: `read -r reply || reply=""` so EOF under `set -e` falls through to `*)` / `die "aborted."` instead of silent exit 1 at the read. - PTY drills assert the **message** (`aborted.` / `uninstalled`), not exit code — green against the silent-EOF bug class. - Residue checks: Ctrl-D removes nothing; `y` removes install. CI: check + db-integration + reconcile + scope green. No blockers.