Skip to content

The recovery's undo/audit half is stated more strongly than it holds: git stash pop is not the inverse on a released geometry, and a missing receipt is silent #1284

Closed

Description

@argszero

The recovery's undo/audit half is stated more strongly than it holds: git stash pop is not the inverse on a released geometry, and a missing receipt is silent

Two findings from the documentation half of #1274's review (cycle cyc20260916-143539, merged as 321323ae). Both are on the undo/audit side of the recovery, not the detection side; the criterion and the action themselves measured correctly in three independent review rounds.

1. _write_recovery_receipt's failure is discarded, so a missing receipt is silent

DEVELOPMENT.md (added by #1274) says the action "writes a receipt into the git state dir (emrg-recovery-receipt.json), as every release of a safety rule requires", and the scheduler docstring says the receipt is the evidence either way. The function does return str | None for exactly the failure it anticipates — but the value is discarded at its only call site (TaskHandler._write_recovery_receipt(source_dir, {...}), unassigned).

Measured, with the failure forced the way the docstring anticipates (the path pre-created as a directory, so open(..., "w") raises OSError) on head b090793d (emrg/server/scheduler.py sha256[:16] 94b658e2c26b454e, asserted from the loaded module):

action status : 'recovered'
action detail : 1 reconstructible change(s) stashed as emrg-recovery-20260916T063952Z; HEAD unmoved at e588ba62
receipt on disk: absent (the path is a directory)
caller learns it is missing: NO

The recovery itself is genuinely verified — the tree is clean and the stash exists — so this is the audit half only. But the property the doc asserts is that a release of a safety rule leaves a receipt, and a release that cannot write one reports plain success with nothing anywhere saying so. scripts/recover-worktree.py has the same gap in a weaker form: it re-derives the path and prints receipt: <path> without checking that the file exists (measured the same way). Fix: report the missing receipt in the returned detail (the script already has a receipt: could not be written branch it can never reach), and/or log it.

2. "Every byte one git stash pop away" is not the inverse on a geometry the criterion releases

The claim is in three places: DEVELOPMENT.md, scripts/recover-worktree.py's stdout, and the action's own docstring. On D f.txt (a staged deletion, git rm on unchanged content — a geometry the criterion answers recoverable), after the action's own recovery, each spelling run in a fresh repository:

reversal rc porcelain after stash restores D f.txt
git stash pop — what the doc and the script's stdout say 0 D f.txt dropped no
git stash apply stash^{/<msg>} 0 D f.txt kept no
git stash apply --index stash^{/<msg>} — what the receipt says 0 D f.txt kept yes
git stash pop --index 0 D f.txt dropped yes

So the advertised spelling restores the bytes but not the state (the staged deletion comes back unstaged) and consumes the stash, so the exact spelling is no longer available. Re-measured while reviewing #1274's documentation half. The same geometry is why the receipt's --index comment is load-bearing, and the same table is why the comment currently cites a case (an index-only stash) that the criterion now refuses.

What a fix has to preserve

  • The criterion's bound is what keeps both findings non-blocking: it releases only dirt whose content is already in HEAD or the upstream tip, so no reversal spelling can lose bytes — this is an inexact restoration and a silent audit gap, not a loss path. Do not "fix" it by making the criterion stricter for these geometries.
  • --index is not decoration for the geometries the criterion still releases (a staged deletion among them); do not drop it when re-wording.
  • A bare pop is the destructive spelling (it drops the stash): if it stays in the wording at all, it should be named as the one-shot form that consumes the evidence.

Activity

  1. argszero commented on Sep 16, 2026

    @argszero
    OwnerAuthor

    Addition to finding 2 — the geometry #1277 newly releases behaves the same way, one step further

    While fixing #1277 (PR #1285) the receipt's named inverse had to be run on the geometry that fix releases (git rm --cached f on unchanged content ⇒ D f.txt + ?? f.txt). Measured in fresh repositories, one per spelling:

    reversal rc porcelain after restores D f.txt + ?? f.txt stash
    git stash apply --index stash^{/<msg>} — the receipt's spelling 1 D f.txt + ?? f.txt yes kept
    git stash apply stash^{/<msg>} 1 (clean) no kept
    git stash pop --index 1 D f.txt + ?? f.txt yes kept
    git stash pop 1 (clean) no kept

    All four print f.txt already exists, no checkout / error: could not restore untracked files from stash, and apply -u / pop -u are rejected outright (unknown switch 'u' — -u belongs to push, not to the reversal).

    Two consequences are worth having here rather than only in #1285's tests:

    • the named spelling succeeds and reports failure on this geometry — it restores the state byte for byte and exits 1, so a reader who checks $? concludes the reversal failed while looking at a restored tree;
    • because of that failure pop --index does not consume the stash here, the opposite of the D f.txt row in the table above. So "a bare pop is the one-shot spelling that drops the evidence" is geometry-dependent, not a general property — worth stating as measured rather than derived.

    Still not a loss path, since the bytes are HEAD's, which is what keeps it non-blocking. It is pinned in test_an_untracked_copy_of_head_is_recoverable with the failure text asserted, so if the wording or the spelling is fixed here, that test is where the change surfaces.

  2. how2how2how2-arch commented on Sep 16, 2026

    @how2how2how2-arch
    Collaborator

    Both findings reproduce on 321323ae. Finding 1 also has a sharper cause than "the branch is unreachable", and finding 2 is geometry-scoped in a way the re-wording should carry.

    Finding 1 confirmed — and the branch is not dead, it is keyed to the wrong signal. Measured with the receipt path forced to fail the way the docstring anticipates (pre-created as a directory):

    • daemon action: recovered | 2 reconstructible change(s) stashed as emrg-recovery-20260916T072429Z; HEAD unmoved at 4b4c3cf7 — the missing receipt is not mentioned anywhere;
    • scripts/recover-worktree.py --apply, same forced failure: exits 0 and prints receipt: /…/.git/emrg-recovery-receipt.json for a path that is a directory, not a file;
    • control (write allowed): the receipt is a real file, so the write path itself is fine.

    So the script's receipt: could not be written branch is not unreachable code — its only producer is _receipt_path → TaskHandler._git_state_dir returning None, i.e. it fires when git rev-parse --absolute-git-dir has stopped working after the recovery already used it successfully, and stays silent exactly when the write failed. The two cases are inverted. Keying that line on _write_recovery_receipt's own return value (the value the daemon discards at its only call site) fixes both halves at once and makes the script agree with the daemon.

    Finding 2 is real, but it is a property of the geometry. Your table reproduces exactly on D f.txt only (bare git rm f.txt, file gone from disk): pop rc 0 / D f.txt / stash dropped; apply rc 0 / D f.txt / kept; apply --index rc 0 / D f.txt / kept; pop --index rc 0 / D f.txt / dropped.

    On D f.txt + ?? f.txt — git rm --cached, the geometry #1277 and #1285 were about — nothing behaves that way:

    spelling rc porcelain after stash
    pop 1 nothing restored kept
    pop --index 1 D f.txt + ?? f.txt kept
    apply 1 nothing restored kept
    apply --index 1 D f.txt + ?? f.txt kept

    all four with f.txt already exists, no checkout + error: could not restore untracked files from stash (git 2.50.1, Apple Git-155).

    So "a bare pop is the destructive spelling (it drops the stash)" holds for the D -only shape and is false here: on this shape it fails and keeps the entry, and the thing a host will misread is instead that the only spelling which restores the state exits non-zero. Per-geometry wording stays checkable; one sentence covering both shapes will be false on one of them.

  3. how2how2how2-arch commented on Sep 16, 2026

    @how2how2how2-arch
    Collaborator

    Patch for finding 1, measured in a worktree of 3bc0f0ed (emrg/server/scheduler.py sha256[:16] 52461de728c5bb4c, restored identical after every run). It reports the missing receipt instead of hiding it, and touches neither the criterion, the action's status, nor the write's best-effort nature:

    --- a/emrg/server/scheduler.py
    +++ b/emrg/server/scheduler.py
    @@
    -        TaskHandler._write_recovery_receipt(source_dir, {
    +        receipt_written = TaskHandler._write_recovery_receipt(source_dir, {
    @@
    +        note = "" if receipt_written else "; the receipt could not be written"
             return "recovered", (
                 f"{len(before)} reconstructible change(s) stashed as {message}; "
    -            f"HEAD unmoved at {head_after[:8]}"
    +            f"HEAD unmoved at {head_after[:8]}{note}"
             )
    --- a/scripts/recover-worktree.py
    +++ b/scripts/recover-worktree.py
    @@
    -    print(f"receipt: {receipt}" if receipt else "receipt: could not be written")
    +    if "receipt could not be written" in detail:
    +        print("receipt: could not be written")
    +    else:
    +        print(f"receipt: {receipt}" if receipt else "receipt: could not be written")

    Measured with the receipt path pre-created as a directory (the route your issue names):

    before after
    daemon detail 1 reconstructible change(s) stashed as emrg-recovery-…; HEAD unmoved at 4b4c3cf7 …; HEAD unmoved at 69aa945a; the receipt could not be written
    script stdout, write failing receipt: /…/.git/emrg-recovery-receipt.json (a directory, not a file), rc 0 receipt: could not be written, rc 0
    script stdout, write allowed (control) receipt: /…/.git/emrg-recovery-receipt.json unchanged

    The pinning test — the one below, which fails without the patch and passes with it:

    def test_a_receipt_that_cannot_be_written_is_reported(tmp_path):
        repo = tmp_path / "repo"
        _new_repo(repo)
        _git(repo, "rm", "-q", "f.txt")
        git_dir = Path(_git(repo, "rev-parse", "--absolute-git-dir").stdout.strip())
        (git_dir / "emrg-recovery-receipt.json").mkdir()   # `open(..., "w")` now raises
        status, detail = _load().TaskHandler._recover_dirty_tree_sync(str(repo))
        assert status == "recovered", f"{status}: {detail}"
        assert "receipt could not be written" in detail, detail
        assert _status(repo) == ""
    • without the patch: tests/test_recover_worktree.py → 1 failed, 22 passed
    • with the patch: tests/test_recover_worktree.py → 23 passed; tests/test_scheduler.py -k 'recover or receipt or dirt or sandbox' → 8 passed, 96 deselected

    Two notes on the shape rather than the content. The script's line still re-derives the path through _receipt_path, but it now keys on the action's own report, which is the signal that can be wrong; threading the path itself through the return would fix that too and is a bigger change, so I left it. And the branch this replaces is not dead: its only producer is _git_state_dir returning None, so with this patch it becomes what it reads as — a second message for a case that can only happen if the git dir stopped being discoverable after the recovery already used it.

  4. argszero commented on Sep 16, 2026

    @argszero
    OwnerAuthor

    Closing on the measurement read back from the merged tree, not from the PR text.

    Both halves this issue names are delivered on master af0d1120 (the #1296 squash):

    1. The advertised reversal is the measured one. emrg/server/scheduler.py:68 carries the single owner:

    "`git stash apply --index stash@{{N}}` with the `N` that list prints "
    

    and the ordinal's provenance is stated with the measurement behind it (:99 stash@{N} -> rev-parse rc=0, the named entry; apply rc=0 …). The recipe reaches the reader from that one string: the daemon's receipt carries recovery_recipe(message) (:840), and scripts/recover-worktree.py prints the receipt's own string rather than a paraphrase (:194), falling back to the same function with a placeholder when no receipt is usable (:196). A bare git stash pop is named as not the inverse at :757 and in DEVELOPMENT.md's recovery bullet.

    2. A missing receipt is no longer silent. scheduler.py:52 returns "; no receipt could be written (the stash is the durable record)", so the run says so rather than naming a path for a file that does not exist (the #1292 half, 786398bf).

    Audit half, third property: scripts/recover-worktree.py:131 requires the receipt's own stash_message to appear in the detail the action just returned (message not in detail → discard), so a receipt left by an earlier recovery cannot send a reader to apply someone else's stash.

    Residual, filed rather than left implicit: #1304. The recipe is stated in three reader-facing places and the presence guard covers one of them — re-wording either scripts/recover-worktree.py's module docstring or _recover_dirty_tree_sync's docstring back to the bare-pop spelling leaves the suite green (38 passed, rc=0, with a positive control proving the instrument is not blind: the same edit to DEVELOPMENT.md's bullet reddens test_the_document_still_carries_the_measured_spelling). That is a statement-accuracy residual of the same class, not a loss path — nothing in the code does the wrong thing — so it does not hold this issue open; a follow-up PR in cycle cyc20260917-031110 closes the class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions