fix(install): a binary install gets a guarded update.sh too, and both modes check for secret.key - #348
Merged
Merged
Conversation
… modes check for secret.key (#347) write_helper_scripts opened with `[ "$MODE" = "docker" ] || return 0`, so the generated update.sh existed only for Docker installs. Docker operators got a script that REFUSES to upgrade when the backup would be a lie; systemd operators got the same procedure written in UPGRADING.md, where nothing checks whether they ran it or whether it worked. Migrations are forward-only in both modes. The install that most needed the guard rail had the least. BOTH MODES NOW CHECK FOR secret.key, which neither did. Counting entries proves an archive is not empty; it does not prove it holds the one file whose absence cannot be recovered from. 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 restore reads as completely successful until the moment someone goes live. That is the same failure the first-run screen now warns about. A warning is the right place to explain it; an upgrade script is the right place to enforce it. THE TEST FOUND A BUG IN THE SCRIPT I HAD JUST WRITTEN. `cp -a src dest` NESTS when dest already exists, so a second run inside the same minute produced data.bak-<stamp>/data/ and then checked for secret.key in the wrong directory -- where it legitimately was not. It now refuses when the destination exists rather than guessing which of the two the operator meant. Found by running the generated script twice, which is the thing a real operator does after a failed upgrade and the thing I would not have thought to try. Verified by generating the script and running it against three staged directories: no secret.key refuses, both files present passes, and an empty data directory refuses before copying anything.
There was a problem hiding this comment.
Pull request overview
This PR aims to add the same “guarded upgrade” safety rails to binary/systemd installs that Docker installs already get, and to ensure both install modes refuse to proceed with an upgrade backup that’s missing secret.key (which is required to restore a usable instance post-0.7.0).
Changes:
- Add a generated
update.shscript intended for binary/systemd installs, with checks for non-empty backups and required files (secret.key,polyemesis.db). - Add a
secret.keypresence check to the Docker-modeupdate.shbackup verification.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1040
to
+1044
| # forward-only in both modes. See #347. | ||
| if [ "$MODE" != "docker" ]; then | ||
| write_binary_update_script | ||
| return 0 | ||
| fi |
Comment on lines
+1077
to
+1086
| # AND THE ONE FILE THE COUNT CANNOT VOUCH FOR. A non-empty archive proves the | ||
| # volume held something, not that it held the file that makes the database | ||
| # usable. Restoring without secret.key brings every destination back DISABLED | ||
| # -- correctly, since a key that will not open disables its destination rather | ||
| # than failing open -- and that reads as a successful restore until go-live. | ||
| if ! tar tzf "$INSTALL_DIR/backup-\${stamp}.tar.gz" | grep -q "secret\.key"; then | ||
| echo "ERROR: the backup contains no secret.key. Restoring without it leaves" >&2 | ||
| echo "every destination disabled. Refusing to upgrade." >&2 | ||
| exit 1 | ||
| fi |
This was referenced Aug 14, 2026
rainmanjam
added a commit
that referenced
this pull request
Aug 14, 2026
…e's palette, not this one (#352) It was never a flaky test. I called it transient twice and re-ran CI to green, which is the exact behaviour a real failure hiding behind an intermittent one depends on. TestColourUtilitiesNameADeclaredToken was right every time it failed: pages/AuthScreen.tsx bg-raised/40, border-line, bg-cross, bg-line these name no --color-* token, so Tailwind emits nothing All four are real tokens -- in web/src/styles/global.css, the MARKETING SITE's stylesheet. ui/src/index.css is a different palette with different names, and I carried the wrong vocabulary across when writing the password meter and its notice in #346. Tailwind emits nothing for a class whose token does not exist, so the notice box rendered with no border and no background, and two of the four meter segments had no colour at all. border-line -> border-border bg-raised/40 -> bg-card-raised/40 (already used 3x in this UI) bg-cross -> bg-warn bg-line -> bg-border WHY IT LOOKED INTERMITTENT, which is the part worth recording. #346 introduced it and merged on a re-run that went green -- so the bug reached main. #348 then inherited it by updating its branch, and failed on a job its own diff could not possibly break: it touches only scripts/install.sh. That mismatch is what made it read as infrastructure noise rather than a regression, and it is exactly the signal I should have followed instead of dismissing. The guard did its job. I overrode it twice. Verified across three consecutive runs at 115/115 rather than one, because "it passed once" is the evidence that produced this in the first place.
|
rainmanjam
added a commit
that referenced
this pull request
Aug 14, 2026
…no test (#348) (#353) #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
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.
Closes #347.
write_helper_scriptsopened with[ "$MODE" = "docker" ] || return 0, so the generatedupdate.shexisted only for Docker installs. Docker operators got a script that refuses to upgrade when the backup would be a lie; systemd operators got the same procedure written inUPGRADING.md, where nothing checks whether they ran it or whether it worked.Migrations are forward-only in both modes. The install that most needed the guard rail had the least.
Both modes now check for
secret.key, which neither didCounting entries proves an archive is not empty. It does not prove it holds the one file whose absence cannot be recovered from.
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 restore reads as completely successful until the moment someone goes live.It's the same failure the first-run screen now warns about. A warning is the right place to explain it; an upgrade script is the right place to enforce it.
The test found a bug in the script I had just written
cp -a src destnests whendestalready exists, so a second run inside the same minute produceddata.bak-<stamp>/data/and then checked forsecret.keyin the wrong directory — where it legitimately wasn't. It now refuses when the destination exists rather than guessing which of the two the operator meant.Found by running the generated script twice, which is exactly what a real operator does after a failed upgrade, and exactly what I wouldn't have thought to try.
Verified
Generated the script and ran it against three staged directories:
secret.keysecret.keypresentbackup verified