test(install): the guard that stops an upgrade eating secret.key had no test (#348) - #353
Merged
Conversation
…no test (#348) #348 gave the binary install an update.sh with five refusals in it. None of them had a test, on any machine, and the suite that would have been their home was not wired into CI either. The refusal that matters is secret.key. Since 0.7.0 seals destination stream keys at rest, a database restored WITHOUT secret.key comes back with every destination DISABLED -- correctly, because a key that will not open disables its destination rather than failing open. That is the whole problem: nothing about the restore looks wrong. The server starts, the database loads, every destination is still listed with its name and its URL. The operator finds out what the backup was missing at the moment they go live and nothing publishes, by which point the pre-upgrade data directory is gone. If this guard regresses, the upgrade that ate the key still exits 0 and still prints "backup verified". So sections 7 and 8 GENERATE update.sh by sourcing install.sh -- both branches, binary and docker -- and run it against a temporary directory staged into each failing shape: no data directory, an empty one, a database with no key beside it, a key with nothing to unseal, and both files present. A test carrying its own transcription of update.sh would keep passing after install.sh stopped writing the check, which is the failure this is meant to catch. Mutation-tested rather than asserted: deleting the secret.key branch from install.sh turns "a backup with a database but NO secret.key is refused" into "update.sh exited 0 -- the upgrade would have gone ahead". Deleting the docker branch's archive check does the same to section 8. Deleting the dest-exists refusal reproduces the nesting bug it was written for: the second run reports data.bak-STAMP/data, a directory the checks would then have passed against. The repeat-run case is the one an operator reaches by hand, after an upgrade has already gone wrong, and it is why the nesting refusal exists at all. It is driven the way they drive it -- the same script, twice -- rather than by pre-creating the collision, so it retries if the clock crosses a minute mid-case instead of quietly testing nothing. The suite now runs in the installer workflow. It had only ever run on a laptop, which is most of how five untested refusals shipped. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
There was a problem hiding this comment.
Pull request overview
This PR expands the installer acceptance coverage to include the generated update.sh upgrade guard logic (both binary/systemd and docker modes), ensuring the critical secret.key refusal behavior is exercised and preventing regressions that would allow “backup verified” on an unrecoverable backup.
Changes:
- Add end-to-end acceptance cases that generate
update.shfrominstall.shand drive each refusal path, including repeat-invocation behavior for the binary branch. - Add docker-mode guard coverage using a stub
dockerimplementation (no daemon required) to validate archive checks and key presence messaging. - Update the
installerGitHub Actions workflow to runscripts/acceptance-install.shin CI.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/acceptance-install.sh | Adds acceptance coverage for generated update.sh guard rails (binary + docker), including key-missing refusals and repeat-run behavior. |
| .github/workflows/installer.yml | Runs the installer acceptance suite in CI and includes it in path filters. |
Suppressed comments (1)
scripts/acceptance-install.sh:328
- Same subshell trap-inheritance issue as gen_binary_update: if
load_install_defsreturns before it disarms traps, the inherited$workcleanup trap can run on subshell exit and remove the workspace while the suite continues.
( load_install_defs || exit 1
INSTALL_DIR="$1"
MODE=docker
COMPOSE_CMD="echo [stub compose]"
write_helper_scripts >/dev/null )
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+150
to
+155
| ( load_install_defs || exit 1 | ||
| INSTALL_DIR="$1" | ||
| DATA_DIR="$2" | ||
| BIN_PATH="$1/polyemesis" | ||
| SERVICE_NAME="polyemesis-acceptance" | ||
| write_binary_update_script ) |
|
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
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.



What an operator loses if this regresses
#348 gave the binary install an
update.shwith five refusals in it, and a test for none of them. The one that matters issecret.key.Since 0.7.0 seals destination stream keys at rest, a database restored without
secret.keycomes back with every destination disabled — correctly, because a key that will not open disables its destination rather than failing open. That is exactly what makes it dangerous: nothing about the restore looks wrong. The server starts. The database loads. Every destination is still there, with its name and its URL. The operator finds out what the backup was missing when they go live and nothing publishes — and by then the pre-upgrade data directory has been replaced.If the guard regresses, the upgrade that ate the key still exits 0 and still prints
backup verified. There is no second signal. That is what these cases exist to hold.What is here
Sections 7 and 8 of
scripts/acceptance-install.sh:install.shis sourced (with itsmain "$@"invocation stripped, and the strip proved beforeeval— the failure mode of assuming it is a developer's laptop growing apolyemesisuser and a unit file), thenwrite_binary_update_script/write_helper_scriptsare called withDATA_DIR,INSTALL_DIR,BIN_PATHandSERVICE_NAMEpointed at a temp directory. A test holding its own copy ofupdate.shwould keep passing for years afterinstall.shstopped writing the check.secret.keycase additionally requires the message to say the restore comes back disabled, because an operator told only that a file is missing restores anyway.dockerthat understands the two invocations the generated script makes. No daemon.Nothing touches a real
/var/lib/polyemesis. Runs in about a second.Mutation-tested
Not "the assertions look right" — each was watched fail, by reading the output rather than the exit code, with
install.shrestored from a file copy afterwards (shasummatched,git diffclean):secret.keybranchFAIL a backup with a database but NO secret.key is refused: update.sh exited 0 — the upgrade would have gone ahead, plus the message-consequence assertion[ -e "$dest" ]refusalFAIL the second run nested the copy: …/data.bak-2026-08-14-1217/data exists, so the checks read the wrong directoryThat last one reproduces the bug refusal 3 was written for, which is the point of driving it as a repeat run.
Repeat invocation
The nesting bug was found by running the generated script twice, which is what an operator does after a failed upgrade. That is how it is driven here — the same script, twice, not a pre-created collision — so it retries if the clock crosses a minute mid-case rather than reporting a pass it did not earn.
One repeat-invocation case is left uncovered on purpose and named in the file: a second docker run inside the same minute rewrites
backup-STAMP.tar.gz, andtartruncates. The binary branch refuses that collision; the docker branch overwrites the pre-upgrade backup with whatever the half-migrated volume holds now — at the moment an operator has least to spare. Asserting the current behaviour would pin a bug as correct, so it is a comment and a follow-up, not a test. The fix belongs ininstall.sh.Also
The suite is now run by the
installerworkflow. It had only ever run on a laptop, which is most of how five untested refusals shipped.Verification
scripts/acceptance-install.sh: 38 passed, 0 failed — twicebash -n scripts/install.shclean;scripts/install.shbyte-identical tomainshellcheck --severity=warningclean on the changed filego test ./internal/testenv/ -count=1ok — no skip sites added,testdata/skips.jsonuntouched./scripts/termination-guard.shokhttps://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX