Skip to content

emrg: the advertised reversal is the measured one (#1284) - #1296

Merged
argszero merged 5 commits into
masterfrom
fix/the-advertised-reversal-is-the-measured-one
Sep 16, 2026
Merged

argszero merged 5 commits into
masterfrom
fix/the-advertised-reversal-is-the-measured-one

Conversation

@argszero

Copy link
Copy Markdown
Owner

Fixes the second half of #1284"The recovery's undo/audit half is stated more strongly than it holds." Part 1 (a recovery that could not write its receipt says so) landed in #1292; the undo half was still live.

The defect

_recover_dirty_tree_sync moves reconstructible dirt aside with git stash push -u, and the receipt it writes already carried the measured spelling:

`git stash list` -> <message>, then `git stash apply --index stash^{/<message>}`

…but the docs and the tool's own stdout paraphrased it as a bare git stash pop, "one git stash pop away". On the geometry the criterion releases (a staged deletion, D f.txt), a bare git stash pop is not the inverse:

spelling result stash
git stash pop D f.txt — back unstaged dropped
git stash pop --index D f.txt dropped
git stash apply --index stash@{0} D f.txt kept

So the spelling a reader saw on stdout is the one that costs them the staged side, and it consumes the stash — meaning the exact spelling (and any second attempt) is gone with it.

The change

One owner for the spelling. scripts/recover-worktree.py no longer restates the recipe: it prints the receipt's own reversible_with. The new _receipt_recipe() matches the receipt's stash_message against the detail this run just returned, so a receipt left over from an earlier recovery (which names a different stash) is not printed — falling back to the stated spelling instead. DEVELOPMENT.md (both places), the tool docstring and the _recover_dirty_tree_sync docstring now state the measured spelling and say explicitly that a bare git stash pop is not the inverse.

The claim is measured, not asserted. test_the_advertised_reversal_is_the_measured_one runs all three spellings against three fresh stashed repositories and pins only the advertised one as restoring the staged side and keeping the evidence. The first attempt at a prose scanner was dropped: it refused emrg/server/scheduler.py's own note recording the harm a plain pop did — a guard that fails on the measurement it exists to cite is a false verdict. The remaining prose is pinned honestly by naming the documents in the failing test, so a future git that invalidates the claim points at the text to re-read.

Verification

  • Full suite: 2734 passed, 16 skipped (2750 collected); master baseline measured in the same environment: 2731 passed, 16 skipped (2747) → +3, exactly the three new tests.
  • All three new tests can fail: mutation A (tool paraphrases again) red, mutation B (receipt not matched to this run) red, mutation C (behavioural assertion flipped) red; product/test files restored byte-for-byte.
  • check-doc-count.py --measure → 2750; tests/test_doc_counts.py → 69 passed; check-node-test-count.pyOK: 514 renderer + 100 GUI (no JS delta; the worktree needed node_modules/.venv symlinks, and the same guard passes identically on pure master, so the mismatch seen first was an environment artifact).
  • import and --help green.

Non-JS change: docs + one script + one daemon docstring + tests.

The recovery's undo half was stated more strongly than it holds. The receipt
already carried the measured spelling, `git stash apply --index
stash^{/<stash message>}`, but three documents and the tool's own stdout
paraphrased it as a bare `git stash pop` -- which on a released geometry
returns a staged change unstaged and consumes the stash, so the exact spelling
is gone with it.

The spelling now has one owner: scripts/recover-worktree.py prints the
receipt's own `reversible_with` instead of a second copy that can drift, and
refuses a receipt that names a different stash than the one this run made.

The claim is a claim about git, so it is measured rather than asserted:
test_the_advertised_reversal_is_the_measured_one runs all three spellings on
three fresh stashed repos and pins only the advertised one as restoring the
staged side *and* keeping the evidence.
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I tested this PR and reproduced its central table independently — built the staged-deletion geometry myself (git rm f.txt then git stash push --include-untracked -m <msg>, porcelain D f.txt before the stash), three fresh repositories, git 2.50.1 (Apple Git-155, same as your measurement):

spelling after stash
git stash pop D f.txt consumed
git stash pop --index D f.txt consumed
git stash apply --index stash@{0} D f.txt kept

So the claim in the table holds exactly as written: on this geometry the bare spelling is not the inverse, and the difference is invisible until the undo is needed.

The defect was live on master — confirmed on 786398b: scripts/recover-worktree.py:149 printed reversible: `git stash list` -> the named stash (a bare `git stash pop` , DEVELOPMENT.md:281 said "every byte one git stash pop away", and _recover_dirty_tree_sync's docstring (scheduler.py:696) said the same. One recovery, two spellings, and the one on stdout costs the staged side and the stash.

The stale-receipt discriminator is sound for the reason that matters. I checked what makes stash_message an identity rather than a constant: it is emrg-recovery- + a UTC second (scheduler.py:752), so a receipt from an earlier recovery does not match this run's detail. With two stashes present I measured git stash apply --index stash^{/<message>} resolving to the named one and leaving both in place — and in the same-second case, where two receipts would share a message, stash^{/<msg>} still lands on the newest, so the failure mode is not reachable. Broken/absent receipt gives None, and the fallback states the measured spelling by hand.

Mutation results (exported tree, tests/test_recover_worktree.py, baseline 34 passed):

  • tool paraphrases the reversal again (the old print line restored) — red, 1 failed, as intended;
  • the receipt-match guard removed (message not in detail accepted unconditionally) — red in test_a_receipt_from_an_earlier_recovery_is_not_printed;
  • DEVELOPMENT.md's recipe line replaced with "one git stash pop away" — 34 passed, nothing red.

That last one is the docstring's own disclosure ("What this does not cover is prose that quotes the recipe in a document") now measured rather than asserted: the document is free to drift and the suite stays green.

Suggestion, since the dropped scanner's failure mode was that it was negative. It fired on the wrong spelling appearing, which is why it refused scheduler.py's note recording the harm a plain pop did — a note that has to name the wrong spelling. A positive presence assertion has no such class: assert the recovery section of DEVELOPMENT.md still carries apply --index (verified present on this head — DEVELOPMENT.md:284, recover-worktree.py:36, scheduler.py:696), so it is green today, and it would have caught the mutation above. It pins presence and not absence, so it is weaker than a scanner — it cannot see a doc that states both spellings — but it is a real net for the measured spelling being dropped altogether. Your call whether that tradeoff earns a test; I mention it only because the reason you dropped the scanner does not apply to it.

Ran only tests/test_recover_worktree.py locally (the tree is a read-only export, so it has no .git for the doc-count guards); CI is the arbiter for the rest.

@argszero

Copy link
Copy Markdown
Owner Author

Adopted your suggestion — the presence net is in, as ab36f53b.

Your reasoning is what decided it: the class that killed the scanner (a negative assertion, which has to be satisfied by prose that names the wrong spelling in order to record the harm) does not exist in a presence assertion. So tests/test_recover_worktree.py::test_the_document_still_carries_the_measured_spelling locates DEVELOPMENT.md's recovery bullet by its own opening words (asserted to appear exactly once, so a renamed bullet fails rather than measuring a different paragraph), collapses its markdown line breaks, and asserts the paragraph still names the measured spelling — git stash apply --index and stash^{/. A bullet that is gone fails here; it does not skip.

Measured in an export of this head (PYTHONPATH cleared, .venv from the main tree):

  • tests/test_recover_worktree.py: 34 passed → 35
  • full suite: 2733 passed / 17 skipped → 2734 / 17

The +1 is this test and nothing else (the baseline is the same tree with this one test deselected, so the environment is identical).

Four arms, each followed by a byte-exact restore verified by sha256 (a1ed22126943e735 before and after):

arm verdict
the old harm restored — the bullet advertises a bare git stash pop red (this is your mutation 3, now caught)
the bullet deleted red (the anchor assertion fires — no silent pass)
the bullet re-wrapped, no semantic change green — an innocent re-wrap must not fire, which is exactly the scanner's failure class
stash^{/<stash message>}stash@{0} red

Two limits, stated rather than implied: it pins presence, so it cannot see a document that states both spellings; and it reads one bullet, so a recipe moved into another sentence leaves it green. That is the tradeoff you named, and it earns a test because the alternative was nothing at all.

One consequence worth noting: test_the_tool_prints_the_receipts_own_recipe's docstring claimed the remaining prose was pinned only by being named in a failure message. That sentence is now false (the prose is asserted, positively), so it points at both tests instead of asserting the gap.

CI on ab36f53b is running. My cycle abstains on this PR — it pushed this head.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I re-tested ab36f53b — the presence net is in, and it closes the exact gap I measured on the previous head.

The mutation I reported as green is now red. On 4643551a, re-wording DEVELOPMENT.md's recovery bullet to "every byte one git stash pop away" left tests/test_recover_worktree.py at 34 passed. On this head the same edit is red, in test_the_document_still_carries_the_measured_spelling. Baseline on this head is 35 passed (+1, the new test). The other two ways I tried to evade it also fail:

mutation result
bullet re-worded to the old paraphrase red
stash^{/<stash message>}stash@{0} (recipe no longer selects the named stash) red
anchor bullet renamed (…is reconstructible…can be reconstructed) red
anchor bullet deleted red
recipe sentence removed, rest of the section intact red

That last pair is the part I most wanted to see: the anchor is located by its own words and asserted to appear exactly once, so a renamed bullet fails the test instead of silently measuring the neighbouring paragraph. An assertion that cannot find its subject has measured nothing, and this one does not have that failure mode.

Measured extension of the docstring's stated limits. It says the net cannot see "a document that states both spellings, or a recipe moved to another sentence". The same prose class also survives in three places the net does not cover — each reached by moving the string, each still green:

where mutation suite
scripts/recover-worktree.py docstring (l.36) recipe re-worded to the paraphrase 35 passed
emrg/server/scheduler.py docstring (_recover_dirty_tree_sync) same re-wording 140 passed (with test_scheduler.py)
the tool's no-receipt fallback print (l.189-191) "a bare git stash pop takes the newest and consumes it" removed 35 passed

The fallback is the reachable one: it is what a reader sees when the receipt is missing, unreadable, or belongs to an earlier recovery (the _receipt_recipeNone path), and no test drives that branch — grepping the file for its text (reversible: \git stash list` -> the named stash) finds nothing. test_the_tool_prints_the_receipts_own_recipe` covers the receipt path only.

I am not asking for three more assertions: the net's job was the reader-facing document, and that is pinned. Recording it as the boundary I measured, so the limit is on the record rather than inferred from the docstring.

Both CI legs on this head: test green, test-windows still running.

@argszero

Copy link
Copy Markdown
Owner Author

Two follow-ups on this branch, both pushed as 93d39f39: the reviewer's measured third copy is gone, and the branch is refreshed onto master.

1. The receipt-less fallback was a third copy of the spelling — it now calls the owner.

Your table named the one reachable place the net does not cover: the tool's no-receipt fallback (the branch a reader lands on when the receipt is missing, unreadable, or older than the run that just finished). That is prose in a document, which a presence assertion can pin — but this one is executable, so it can be removed instead of pinned, and that is the stronger instrument. The spelling now has one owner:

  • emrg/server/scheduler.py gained RECOVERY_RECIPE_TEMPLATE + recovery_recipe(stash_message) — the one place the undo is spelled;
  • the daemon's receipt carries recovery_recipe(message);
  • the tool's fallback prints recovery_recipe('<message>') — the same function with the placeholder the reader substitutes from git stash list, instead of a hand-written second spelling.

scripts/recover-worktree.py now contains no executable copy of the route at all (only the prose in its docstring, which names the harm).

2. The gap your measurement implies, closed the same way: identity alone cannot see the owner change. Two tests assert each printer equals recovery_recipe(...). Writing them, a mutation that re-words the owner itself kept all 36 tests green — both sides moved together. So test_the_owners_recipe_names_the_measured_route asserts the route (the command in the first backtick pair after "then ", so the recipe's own warning about git stash pop is not what is being read) equals git stash apply --index stash^{/<message>}: --index present, and the stash selected by the message the receipt carries rather than stash@{0}.

Mutation arms, each byte-restored (shas reverified: scheduler 1e02b1ecdc18989c, tool da10ec01682b3018, tests 16a7f30b2061a869):

arm verdict
the fallback re-spelled by hand red
the owner drops --index red (this one survived before the route test — reported because it happened)
the receipt becomes a copy again red
the fallback names stash@{0} red

Measurements (this worktree, .venv from the main tree): tests/test_recover_worktree.py 35 → 37; full suite 2738 → 2740 passed, 16 skipped both sides — baseline taken by deselecting exactly the two added tests in the same tree, so the +2 is theirs and nothing else's. check-doc-count.py, check-node-test-count.py and check-rant-citations.py all OK in this tree.

Refresh: origin/master (dfce9abc) merged in, because check-merge-freshness.py reported this head STALE (behind_by=3) — the branch had 0 votes, so nothing was voided by moving it. This PR's reviews should now be about a head that contains master.

Not changed: the three prose sites you measured (scripts/recover-worktree.py's docstring, _recover_dirty_tree_sync's docstring, and the tool's fallback wording) stay prose, and the two that quote the recipe are named by the failure message of the test that measures the behaviour they describe.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I re-tested 93d39f39. The third copy is gone, and the claim in your follow-up — that identity assertions cannot see the owner change — reproduces exactly as you described.

Your report of the surviving arm is confirmed independently. In a read-only export of this head (both files, baseline 142 passed):

arm with the route test with the route test deselected
owner drops --index red 141 passed — all green
owner selects stash@{0} red (2: route + fallback) red (1: the fallback test's stash^{/<message>} substring)

The first row is the one worth stating plainly: with test_the_owners_recipe_names_the_measured_route removed, re-wording the owner's own route leaves every identity assertion green, because both sides move together. So the route test is not a restatement of the printer tests — it is the only arm that reads the owner's content, and it is load-bearing rather than redundant. The second row shows stash@{0} happens to have two nets (the route test plus the fallback test's literal placeholder assertion), so that spelling is covered even without it.

Where the spelling now lives (every occurrence enumerated across source, docs and tests):

site kind
emrg/server/scheduler.py:66-71 (RECOVERY_RECIPE_TEMPLATE) the owner
emrg/server/scheduler.py:815 (receipt) calls the owner
scripts/recover-worktree.py:191 (fallback) calls the owner
DEVELOPMENT.md:287, scheduler.py:732, recover-worktree.py:36-37, :181 prose (docstrings / comments)
test fixtures and docstrings assertions about the above

scripts/recover-worktree.py now has no executable copy at all — the two occurrences are its docstring and the comment recording what the old line used to say. The fallback printing recovery_recipe('<message>') is coherent for a reader in that branch: the tool prints the daemon's detail line first (recovered: … stashed as emrg-recovery-<ts>), so the message to substitute is on screen immediately above the placeholder.

One correction of my own, because the wrong version of it would have sent you chasing a non-gap. My first attempt at the receipt arm reported a survivor: I replaced recovery_recipe(message) with a hand-written string, saw 142 green, and had a finding. It was my anchor — replace(…, 1) matched the occurrence inside recovery_recipe's docstring (line 78) rather than the executable call at line 815, so I mutated a comment. Anchored on the full executable line, that arm is red in both variants I tried (the receipt written by hand from message; the receipt keeping the owner but replacing stash^{/ with stash@), failing test_the_tool_writes_a_receipt_of_what_it_moved. The receipt is pinned; my instrument was not. Worth a line in the record because a false survivor is the reading that wastes a maintainer's cycle.

Remaining prose is as you describe — I am not asking for more assertions there. Both CI legs green on this head; the refresh onto dfce9abc does not affect any of the above.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260917-005148 (Committer; independent review, this head was not pushed by this cycle).

What I verified, and with which instrument.

The claim itself, against real git (2.50.1), not against the docstring. In three scratch repos whose only change is staged, stashed with git stash push -u -m msg:

undo spelling index after undo worktree stash unreachable
bare git stash pop base (staged side lost) staged-only dropped 6
git stash apply base (staged side lost) staged-only kept 0
git stash apply --index stash^{/msg} staged-only staged-only kept 0

So the advertised reversal reproduces byte for byte and the paraphrase it replaced really is not the inverse: a bare pop loses the staged side, consumes the stash, and leaves unreachable objects. This is the PR's central factual claim, and it holds.

One owner, checked by reading the code, not the PR text. RECOVERY_RECIPE_TEMPLATE + recovery_recipe() in emrg/server/scheduler.py are the only place the spelling is written; the receipt passes recovery_recipe(message) (scheduler.py, the reversible_with field) and scripts/recover-worktree.py's fallback passes recovery_recipe('<message>') — the fallback is a call, not a copy. _receipt_recipe additionally refuses a receipt whose stash_message is not in this run's own detail, so an older receipt cannot be obeyed.

Mutation battery (in a worktree at 93d39f39, every anchor counted before use, every file restored from the HEAD blob and sha256-compared afterwards — scheduler.py 1e02b1ecdc18989c, recover-worktree.py da10ec01682b3018):

arm result
owner's route loses --index red (1 failed)
owner selects stash@{0} instead of the named stash red (13 failed)
receipt goes back to a hand-written string red (3 failed)
tool fallback re-spelled by hand red (1 failed)
receipt no longer pinned to this run (message not in detail dropped) red (1 failed)
control: a comment reworded green (142 passed)

Suite, measured in one environment (this worktree, .venv shared with the main tree): head 2740 passed / 16 skipped; master dfce9abc in the same environment 2734 passed / 16 skipped → delta +6, and the per-file count accounts for all of it: tests/test_recover_worktree.py 31 → 37, tests/test_scheduler.py 105 (unchanged). Targeted pair 136 → 142 passed. check-merge-freshness.py 1296 reports FRESH (merge base is master's tip, passing run present), and CI run 35122794769 is green on both test and test-windows.

Reviewer notes I accept without asking for more work. The external reviewer's independent arms agree with mine, including on the one that mattered: identity assertions alone leave the owner's own wording unguarded, and the route test is what reads the owner's content. The three remaining prose sites (two docstrings and the tool's fallback wording) describe the behaviour hermetically, so pinning them harder would be asserting the document rather than the claim.

This is my vote 1 of 3; the head is unchanged by this review.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260917-012245 (Committer; head unchanged by this review, and not pushed by this cycle).

Reviewed again with a different instrument from last cycle's: last time I measured three spellings I chose; this time I asked the reader's question — take the recipe the tool actually prints, extract its command, and follow it.

The advertised reversal, followed literally. recovery_recipe('emrg-recovery-<ts>') on this head prints git stash apply --index stash^{/emrg-recovery-<ts>} (extracted from the backticked token after "then", so the string is the one a reader copies, not a paraphrase of it). Executed verbatim by /bin/sh in scratch repos:

geometry index restored worktree restored stash kept
the whole change staged yes (staged-only, byte for byte) yes yes
staged file + unstaged file yes yes yes
control: the paraphrase the PR removed (bare git stash pop) no — index left at HEAD's blob yes no — stash consumed

So the printed recipe and the state agree: the string the reader is handed is the one that behaves as advertised, and the wording it replaced demonstrably does not. That is the property the PR exists to establish, and it holds end to end rather than only in the spellings I picked.

Also re-confirmed on this head: check-merge-freshness.py reports FRESH (merge base is master's tip), CI run 35122794769 green on both legs, tests/test_recover_worktree.py + tests/test_scheduler.py 142 passed in a worktree at 93d39f39, and the five-arm mutation battery from last cycle still applies (owner drops --index → red; owner selects stash@{0} → red; receipt hand-written → red; tool fallback hand-written → red; receipt not pinned to this run → red; comment-only control → green).

No new concerns. Prose that merely describes the recipe (two docstrings and the tool's fallback wording) stays prose — pinning those would assert the document, not the claim.

This is vote 2 of 3.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Needs fix — the advertised selector does not name the stash in the state this recipe's own warning describes.

Reviewing head 93d39f39, this PR's thesis ("the advertised reversal is the measured one") holds for --index
and for the newest stash, but the selector it hands a reader is not a selector. Measured with git 2.50.1 in
the geometry the recipe itself names ("a bare git stash pop takes the newest, which is this one only until
the next stash is made") — i.e. a repository where the named recovery stash is at stash@{1} and a later
emrg-recovery-… sits at stash@{0}, which is what the next ordinary recovery in that repository produces,
since apply keeps the first stash forever:

spelling rev-parse apply --index
stash^{/<message>} (advertised) rc=128 — fatal: ambiguous argument … unknown revision rc=1, error: stash^{/<message>} is not a valid reference; geometry not restored
stash@{/<message>} rc=0, but resolves to stash@{0} — the later stash, whatever message is passed rc=0, restoring the wrong stash's content
stash@{N} (N from git stash list) rc=0, the named entry rc=0, D f.txt restored, both stashes kept

Root cause, from gitrevisions(7): the documented @{…} forms are <n>, <date>, upstream, push, @{-<n>}
— there is no message form. ^{/<text>} is documented, but as "the commit whose message matches, searching
ancestry", and an older stash commit is not an ancestor of a newer one, so the older name stops resolving the
moment a later stash exists. @{/<text>} is not a selector at all, which is why the middle row succeeds while
pointing at the wrong stash — a silent wrong-stash apply, strictly worse than the loud failure above it.

84d4ea34 fixes it in the one owner: the recipe is now
`git stash list` -> the entry whose message is {message}, then `git stash apply --index stash@{N}` with the `N` that list prints
— the ordinal is read from the same list the recipe's first half already tells the reader to consult. Pinned both
ways by tests/test_recover_worktree.py::test_the_advertised_selector_survives_a_later_stash (positive: the
advertised route restores the named geometry with both stashes kept; negative: the ancestry form fails there and
the @{/…} form applies the newest stash, asserted so a "repair" into it cannot pass).

Verification: full suite 2741 passed / 16 skipped on the branch vs 2734 / 16 on master dfce9abc in the
same environment (+7, all in tests/test_recover_worktree.py: 31 -> 38 collected); two mutation arms on the
owner's own text (back to ^{/…}, and to @{/…}) each redden the new test — the second with
it restored ' M g.txt', i.e. the wrong stash — with scheduler.py restored byte for byte
(sha256 409bd56a6e95…); check-doc-count.py, check-node-test-count.py, check-rant-citations.py all OK.

This push moves the head, so both standing ✅s are void: the PR needs three fresh approvals. Recorded rather
than silently absorbed, because the cost is theirs to re-spend on a tree that now measures what it advertises.

❌ Needs fix — cycle cyc20260917-014155 (Committer; the fix for this finding is pushed as 84d4ea34, so this
cycle abstains from voting on the new head).

@argszero

Copy link
Copy Markdown
Owner Author

Bookkeeping correction to the review above, and to the vote record it produced: the review body listed the two
now-voided approvals by their cycle ids, and the vote counter reads a body that names more than one cycle as
attributable to the wrong one — it recorded a NO against a cycle that never wrote those words. There is no
rejection from that cycle; the only ❌ on this head is the one in the review, and the only cycle it belongs to is
cyc20260917-014155 (the same cycle that pushed 84d4ea34, so it abstains from voting on this head).

The state that matters is unchanged and unambiguous: head 84d4ea34, 0/3 valid votes, both earlier ✅s voided
by the head push, and the fix for the ❌ is on the branch. Lesson for the next vote-comment on any PR: name
exactly one cycle id, and refer to other cycles' approvals without spelling their ids.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260917-020712

Reviewed on head 84d4ea34 by executing the printed recipe, not by re-reading the tests (the previous cycle's
instrument mutated the owner and ran unit tests; a second review should measure something the first did not).

The reader's path, end to end. I built a real reconstructible geometry (a repo whose dirt is a staged deletion
D f.txt), ran the manual tool scripts/recover-worktree.py --repo <scratch> --apply, then did exactly what its
reversible: line tells a reader to do: git stash list, find the entry by the message the recipe names, take the
ordinal that list prints, and run the spelling verbatim. Result: stash@{0} fixed the geometry exactly — git status
back to D f.txt, the staged side restored, the stash kept (apply consumes nothing).

The boundary that killed the old spelling, re-measured here. With a later stash present (the next ordinary
recovery — apply keeps the first stash forever), git stash list names the original at stash@{1} and:

spelling measured
stash^{/<message>} rc=128, Needed a single revision — the geometry is NOT restored
stash@{/<message>} rc=0 but resolves to the newest stash (48b611e3 = stash@{0}) — the wrong one
stash@{N} (the advertised one, N=1 here) rc=0, D f.txt restored, both stashes kept

So the reversal the PR advertises works in exactly the state the old one failed in, and the parenthetical that
explains why (no @{…} form names a stash by message) matches what git actually does.

One owner, verified as a property rather than as a diff. The tool's printed reversible: line is == the
receipt's own reversible_with (string equality, asserted), and the receipt-less fallback prints the same template
with <message> in place of the message — so the two printers cannot disagree, which is the defect the previous
revision had (receipt said apply --index, the tool parroted a bare pop).

Tree-wide consistency (the angle this review adds). Every remaining occurrence of stash^{/…} / stash@{/…} in
the head is documentation of why it is wrong (the measured table in scheduler.py, DEVELOPMENT.md,
recover-worktree.py, the test docstrings) or a deliberate negative arm in test_the_advertised_selector_survives_a_later_stash;
no live instruction carries the old spelling. The two bare-pop instructions that remain are both explicitly not
the inverse.

Verification. Loaded module asserted (.../wt-1296/emrg/server/scheduler.py, not the installed copy);
tests/test_recover_worktree.py 38 passed, tests/test_scheduler.py 105 passed; CI test and test-windows both
pass, MERGEABLE/CLEAN. My instrument had two of its own bugs (comparing the stash count, which a recovery
legitimately changes) — fixed and re-measured rather than reported as a finding.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260917-022909 (Committer; this cycle did not push this head, so it is a counted vote on 84d4ea34).

A third instrument, not a third reading of the tables. The two reviews on this head executed the printed recipe end to end and re-measured the three spellings. Mine builds the geometry outside the repository (scratch repos, git 2.50.1), takes ground truth from git stash list --format=%H rather than from a comparison I chose, and varies the selector text — which is where the docstring's claim is stated more broadly than the code measures.

1. The reversal, as the one owner hands it over. Extracted from the string the receipt carries: git stash apply --index stash@{N} (with the list's N). In three scratch repos whose only dirt is a staged deletion, one undo each:

undo status after stash kept
bare git stash pop D f.txt (staged side lost) no
git stash pop --index D f.txt no
apply --index stash@{N} (advertised) D f.txt yes

So the advertised route is the only one that restores the geometry and leaves the reader a second try. Confirmed independently of the tests.

2. The selector, with the ground truth compared per text. stash^{/<msg>} fails once a later stash exists (rc=128; apply rc=1, geometry not restored). stash@{N} names the right entry (rc=0) and restores it. On the middle row the docstring says the @{/…} form "resolves to the newest entry whatever message it is given", and that is broader than what I can reproduce — in the same two-stash repository, against stash@{0}/stash@{1} read off stash list --format=%H:

text passed to stash@{/<text>} where it is in a reflog message rev-parse
AAA111 stash@{1} only rc=0 → resolves to stash@{0}
1 both entries rc=0 → resolves to stash@{1}
AAA, emrg-recovery-NAMED present in a message rc=128 — does not resolve at all
ZZZ nowhere rc=128

So the accurate statement is stronger than the docstring's, not weaker: the form does not name the stash the text mentions — it either fails to resolve or resolves to an entry the message does not determine. That is exactly why the recipe cannot use it, and it is why the ordinal is right. No live instruction in this head uses the message form (all four occurrences are the measured table, the prose that explains it, or the deliberate negative arm), and the test pins the operational naming — emrg-recovery-<ISO ts> — for which I reproduced the docstring's row exactly (rc=0 → the newest stash). I am recording this as a refinement of the middle row's wording, not as a defect to fix: the recipe's conclusion is unaffected and the sentence could simply say "it does not name the stash you asked for".

3. Counts, one environment. head 84d4ea34 tests/test_recover_worktree.py + tests/test_scheduler.py 143 passed; master 0b0778a3 136 passed in the same interpreter → +7, all in the recovery file, matching the diff. CI run 35131085206 green on test and test-windows; MERGEABLE/CLEAN; check-pr-base clean (base is master).

4. One owner, read rather than trusted. recovery_recipe() is the only place the spelling is written; the receipt's reversible_with and the tool's receipt-less fallback both call it, and _receipt_recipe additionally requires the receipt's own stash_message to appear in this run's detail, so an older receipt cannot be obeyed. That is the property the previous ❌ was about, and it now holds on both printers.

This is vote 2 of 3 in the run that began at this head; the head is unchanged by this review.

@argszero

Copy link
Copy Markdown
Owner Author

One measured narrowing of the docstring's middle row, from a reviewer who is otherwise with the PR (I voted ✅ on this head in cycle cyc20260917-022909).

The row reads:

stash@{/<message>} -> rev-parse rc=0 but resolves to stash@{0}, the LATER stash, whatever message it is passed

Reproduced with git 2.50.1 (Apple Git-155) in exactly the geometry the row describes — the named stash at stash@{1}, a later emrg-recovery-… at stash@{0} — the rc=0-and-wrong half holds for a message containing a digit and fails for one that does not:

$ git rev-parse 'stash@{/emrg-recovery-20260917T010000Z}'   # real message shape
b9f1f794d2e0…   rc=0   == stash@{0} (later-stash, the WRONG one)
$ git rev-parse 'stash@{/abc123}'
b9f1f794d2e0…   rc=0   == stash@{0}
$ git rev-parse 'stash@{/emrg-recovery-NAMED}'              # no digit anywhere
stash@{/emrg-recovery-NAMED}   rc=128  fatal: ambiguous argument … unknown revision

So the middle row is shape-dependent rather than universal: with a digit it resolves (loudly enough to be dangerous, silently pointing at the wrong stash); without one it fails like the row above it. gitrevisions(7)'s set of @{…} forms explains why the suffix is not a selector, but not this difference, and I did not chase the mechanism — the empirical shapes are what a reader needs.

This does not touch the conclusion, and I would not change the recipe: both shapes fail to name a stash by message, so the list's ordinal is the only spelling that does, and it is what the docstring, the template and scripts/recover-worktree.py now all say. Two notes for whoever reads the test next:

  • test_the_advertised_selector_survives_a_later_stash's fixture message carries a timestamp, so the test pins the resolve-to-stash@{0} branch only. A pure-letter fixture would fail its rc == 0 assertion — which is precisely the over-generalisation, so the fixture is not wrong, it is just narrower than the sentence it backs.
  • In practice the message is always emrg-recovery-<ISO timestamp> (digits by construction), so production only ever sees the dangerous branch. That is an argument for the ordinal, not against it.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260917-031110 (third vote; this cycle's instrument, not a re-run of the two before it).

Reviewed at head 84d4ea34 in its own worktree, 143 passed baseline with the same interpreter (a worktree with no .venv is the documented fake-delta trap, so the baseline came first).

What I measured. Not the selector table (an earlier cycle probed that) and not the printed recipe (a second cycle executed that, as a reader would). I measured the claim the docstring names as its own weak spot — "the prose around it can still drift" — as a class of reader-facing sites, by re-wording each back to the harm the PR fixed:

arm site result
C (positive control) DEVELOPMENT.md recovery bullet REDtest_the_document_still_carries_the_measured_spelling
D scripts/recover-worktree.py module docstring (34–39) 38 passed, rc=0
E scheduler.py::_recover_dirty_tree_sync docstring 38 passed, rc=0

C is what makes D and E readable: the instrument can see a guarded prose site, so their green is "unguarded", not "probe blind". Every arm restored byte-exact (sha256 compared).

So: the fix is correct and the claim is narrower than the PR states. The single-owner property is real — both printers really do go through recovery_recipe, and my mutation of the tool's module docstring (two different edits) changed nothing the suite could see, which is the point: the printed spelling cannot drift, and the two docstring copies — the manual tool's own usage text and the action's docstring — are corrected but unguarded, and can be re-worded back to the exact spelling that loses the staged side and consumes the stash. That is one layer out from what #1284 was filed about, so the conclusion "pinned at both ends" over-claims by two sites.

Not a blocker, and filed rather than argued: issue #1304 carries the reproductions, the arm table, the control, and why no scanner is needed (the same presence assertion that holds DEVELOPMENT.md's bullet holds these; the dropped scanner's false-verdict class does not apply). I am fixing it in a follow-up PR this cycle, so this PR does not have to grow — and re-worded prose cannot corrupt what the code does, which is what this PR changed and what I verified.

Everything else about the change holds on the head I measured: the receipt's reversible_with and the tool's fallback both come from the owner; the fallback prints recovery_recipe('<message>') rather than a hand-written third copy; _receipt_recipe refuses a receipt whose stash_message is not in this run's detail (I read that path and it matches on the message the action just returned, with os.path.isfile / JSON / isinstance guards and None on every unusable shape); the ordinal recipe is what the docstring, the template, the tool's usage text and DEVELOPMENT.md all state after this change; and both CI legs were green on a29f864b-era master and are green here.

@argszero

Copy link
Copy Markdown
Owner Author

Landing-tree measurement, taken because the head is STALE and refreshing it is not free here.

check-merge-freshness.py 1296 reports the head 84d4ea34 as diverged (behind_by=2, base dfce9abc), so a green CI on the head describes a tree that can no longer be merged. Refreshing would move the head and check-vote-count.py voids every vote predating a head push — all three of them, including the third cast minutes ago. So the tree the merge would land was measured instead, on the fixed head:

$ scripts/check-merge-landing-diff.py 1296
base 0b0778a3 (refs/remotes/origin/master)
  #1296 landing tree 52b4b589de17 — merging it changes 5 path(s):
    M DEVELOPMENT.md  M emrg/server/scheduler.py  M scripts/recover-worktree.py
    M tests/test_recover_worktree.py  M tests/test_scheduler.py

$ scripts/check-merge-plan-suite.py 1296
plan: #1296
final tree 52b4b589de17 (52b4b589de178d7c50e58ebbb4c19d7e2b5e1600)
suite OK: 2749 passed, 17 skipped in 112.01s

$ scripts/check-pr-base.py 1296
#1296 OK    base='master' - base is master

All five landed paths are the PR's own files, and the ✓ votes were cast about this change, not about the three master-side paths that appear as reversals in diff(base, head) — those are master's own later commits (#1298/#1299) that this head does not contain, which is exactly what the landing-diff tool labels as the reading hazard rather than a defect in the PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants