fix(selfedit): a config-only restart leaves an undo behind - #302
Open
CrazyWillBear wants to merge 3 commits into
Open
fix(selfedit): a config-only restart leaves an undo behind#302CrazyWillBear wants to merge 3 commits into
CrazyWillBear wants to merge 3 commits into
Conversation
_commit_if_dirty returned early when nothing was dirty, so it never wrote the rollback marker — meaning a config-only restart had no boot-failure recovery for *anything*, not just for the config. A bad value that parsed (so validate_live_config passed) and then broke boot left the box crash-looping with nothing to roll back to. The marker is now always written and records `committed`, because the two things a restart changes need different undos. Committed repo files rewind with git reset --hard. config.yaml cannot: it is gitignored, so git steps past it. Its undo is the newest config-history snapshot (#301), which by construction is the last config that booted — the bad one never got recorded, so newest is right here, not second-to-last. Always writing `rollback_to` alone would have been wrong: with nothing committed, a bad boot would git reset --hard over uncommitted work. Hence the flag rather than a bare marker. rollback_if_marked returns True only when something was actually undone. A marker with no undo left must return False — rebooting into an identical tree and identical config reproduces the same failed boot forever. restore_newest keeps the config it replaces (.config-failed.yaml): a failed boot is not a reason to silently discard an owner's hand edit. The history dir comes from the caller, not the marker — entrypoint resolves it the same way on the boot that writes and the boot that reads, so recording it would only let the two drift. It falls back to the documented default when the boot dies before load_config, where db_path is unknown anyway. A marker written by the previous version has no `committed` key; it is treated as committed, which is the only thing the old code ever wrote one for. That path is live across exactly the upgrade this ships in.
.config-failed.yaml holds owner_handles just like config.yaml does. Without this line the next self-edit's `git add -A` would commit it into a published repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither undo half may raise. The marker is consumed before either runs, so an escape spends the seatbelt and undoes nothing — and a failing git reset used to skip the config half, which may be the undo the boot actually needs. Both are now logged and stepped over. restore_newest gets the same OSError guard record_config already has, for the same reason: a full disk must not turn a recoverable boot failure into an unrecoverable one. rollback_if_marked returns what it undid rather than a bare bool, and that text rides the restart notice to the owner. The old notice claimed "back up on the previous commit" even for a config-only rollback, where no commit moved, and never said where the config that failed went. restore_newest and snapshot now write through one helper: created at 0600 (the plain write left a file holding owner_handles world-readable until the chmod), renamed into place (the restore overwrites the live config at the moment the marker is already gone), and never widening a config the owner chmod'd down. recovery.py went over the 200-line cap, so it splits along the seam its own docstring named: chief.selfedit.restart holds the drain/exec side, recovery.py the boot-failure side. Import-only change for the eleven callers. docs/CONFIG.md said the seatbelt cannot roll back a bad config write, which this branch makes false; the layout table missed .config-failed.yaml. snapshots()' docstring and its test still asserted the second-to-last contract restore_newest contradicts. New tests: both halves undoing together, a failing git reset, a restore that cannot write, the failed config being kept where the owner is told, and the ordering in amain that the newest-not-second-to-last choice rests on. Not taken: the `run` seam stays (it is what makes the git-failure test cheap), the cross-module test-helper import stays over duplicating the helpers, and the config half stays unconditional — the pipeline cannot tell an owner's untested hand edit from a restart's own config change, and reverting an untested config on a failed boot is the intended behavior. The harm was that it was silent, which the notice now fixes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The bug
_commit_if_dirtyreturned early when nothing was dirty, so it never wrote the rollbackmarker. A config-only restart therefore had no boot-failure recovery for anything —
not the config, not the repo.
The fix
The marker is now always written, and records
committed: bool.Two undos, because a restart changes two things the seatbelt covers differently:
git reset --hard.config.yamlcannot rewind that way (it is gitignored, so git steps straight past it) →restored from the newest
data/config-history/snapshot. Newest, notsecond-to-last: by construction a snapshot is only recorded for a config that
booted, so the bad one never got recorded.
Always writing a bare
rollback_towould be wrong: with nothing committed, a bad bootwould
reset --hardover uncommitted work. Hence thecommittedflag.rollback_if_markedreturnsTrueonly when something was actually undone —otherwise rebooting into an identical tree + config crash-loops forever.
restore_newestsaves the config it replaces to.config-failed.yamlso an owner handedit is never silently discarded. That file holds
owner_handleslikeconfig.yamldoes, so it is gitignored too (second commit) — otherwise the next self-edit's
git add -Awould commit phone numbers into a published repo.The history dir comes from the caller (
entrypoint), not the marker, so the bootthat writes and the boot that reads resolve it identically and cannot drift. Falls back
to
repo_root/data/config-historywhen the boot dies beforeload_config().Legacy markers (no
committedkey) are treated as committed — the only thing the oldcode ever wrote one for. Lives across exactly the upgrade this ships in.
For the reviewer
.config-failed.yamlplacement at repo root.Tests
New
tests/test_config_rollback.py(9 tests).tests/test_selfedit.py::test_noop_restart_is_allowedis inverted: it used to assert no marker was written, and that assertion was the bug.
Done-check green: 811 passed,
ruff checkclean,mypyclean.🤖 Generated with Claude Code