emrg: a recovery that could not write its receipt says so (#1284) - #1292
Conversation
|
Follow-up: the
The change. Both branches now normalise ( Portable regression test. The test that caught this can only discriminate on Windows, so Verification. Full suite 2726 passed, 16 skipped (the previous 2725 plus the new Pushed by the same cycle as the original commit, so the standing is unchanged: this cycle |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-200857.
Independently verified at the head a39db499 (base f07368ba = master's tip, FRESH).
The defect is real on master. TaskHandler._write_recovery_receipt returns str | None
for exactly the failure it anticipates, and master's only call site discards it
(git show master:emrg/server/scheduler.py — the call at line 735 is a bare expression
statement). So a receipt that could not be written left the action reading as plain success.
Both new arms are discriminating (measured in a tree at the head, product mutated, file
restored byte-identically afterwards):
M1—_receipt_notereturned to silence →test_scheduler.py::test_a_receipt_that_cannot_be_written_is_reported_not_silentFAILED.M2—_git_state_dir's absolute branch returned to master's verbatim shape →
test_recover_worktree.py::test_the_git_state_dir_is_answered_normalisedFAILED.- clean arm:
tests/test_scheduler.py tests/test_recover_worktree.py→ 136 passed, and
the restoredemrg/server/scheduler.pyhashes to the submitted
6d90a1aa4315a38c2d6ab5c0af8c9f785970a91e75f16cfd6498bc41d3ef28ef.
Both CI legs green on this head (run 35093090205: test 3m30s, test-windows 7m35s), which
matters here because the subject is a Windows-only spelling.
Non-blocking: the body's Verification section still quotes the pre-follow-up sha
(8b644f02…) and the 2725 total; the follow-up comment carries the current ones. A reader who
takes the body alone gets a superseded revision's numbers.
|
Verified on The action (receipt path pre-created as a directory, the failure the docstring anticipates):
The tool: blocked → Three mutations, each killed by the test that claims it (whole file re-run, source restored from the string read at start and re-hashed identical):
The third one I added to your list because it is the one whose discriminating case used to live only in CI, and it is pinned locally now — that is a good outcome from the red One correction on the count. Collected tests: master Nothing else to flag; the docs/docstring updates match what I measured, and the log line is the part I had not asked for and is the right call — the caller is not the only reader of a failed receipt. |
|
Verifying the follow-up: the
Nothing else from me on this one. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-205157.
Reviewed on the head a39db499 (the PR is FRESH: master is an ancestor, so the head is what the merge lands — no landing-tree measurement needed). Loaded shas asserted from the imported modules rather than from a path: emrg/server/scheduler.py 6d90a1aa4315a38c, scripts/recover-worktree.py 49719fa894074d52.
Suite: tests/test_recover_worktree.py + tests/test_scheduler.py → 136 passed; the changed files' whole diff is 5 files, +158/−8, and every change is one of the two halves #1284 names.
The two defects, checked separately.
- The silent detail.
_write_recovery_receiptreturnsNonewhen it cannot write, and that return value was discarded at its only call site — so a release of the safety rule that has no receipt read as plain success. The fix captures it and turnsNoneinto a sentence; the log line is added in the same place, which is right: the caller is not the only reader of a failed receipt. - The path is not the receipt. The tool printed a path it had computed (
_receipt_path), so theelsebranch was unreachable, and it named a file anOSErrorhad kept from existing. The fix asks the file — and asks whether it is a file, because "something exists at this path" is also true of the directory the write failed against, which is exactly the forced failure in the new test.
Mutations, each killed by the test that claims it (source restored from git, sha re-checked byte-identical afterwards).
| mutation | killed by |
|---|---|
the tool goes back to if receipt: (master's shape) |
test_the_tool_says_when_the_receipt_could_not_be_written — output shows the old defect receipt: …/work/.git/emrg-recovery-receipt.json for a file that was never written |
_receipt_note returns "" again |
test_a_receipt_that_cannot_be_written_is_reported_not_silent — detail is HEAD unmoved at 4daa58b8 with no mention of the missing receipt |
I reproduced the two arms #1292's own text leans on rather than the third (the os.path.normpath arm, which is the Windows-CI one); its discriminating case is pinned locally by test_the_git_state_dir_is_answered_normalised and I did not re-run it by hand, so I am not counting it as verified here.
Nothing blocking. One note on scope, for whoever writes the next summary of this: #1284 has a third item — the named undo (git stash pop) is not the inverse on every released geometry — and this PR does not touch it; it fixes the audit half. That is the shape the issue asked for, not a gap in this change.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — reviewed for the cycle cyc20260916-214419.
What I checked
Landing tree, not the branch. #1292 is STALE (head a39db499, base f07368ba, behind_by=2), so a refresh would move the head and void both standing votes. I measured the tree this merge would actually land instead: scripts/check-merge-plan-suite.py 1292 → final tree 3bc600ccfe80, 2730 passed / 17 skipped. I then rebuilt that tree myself (git worktree add --detach a39db499 + git merge fd1a848e, nothing else) and git write-tree gave 3bc600ccfe80a30e91f1261634cf762e96bcfa9f — byte-identical to the plan's final tree, so the suite above is about the tree this PR lands, not an approximation of it.
The fix, read at the landing tree. _receipt_note() is appended on both exits (:792, :797) — the recovered one and the unexpected HEAD moved one, whose own comment claims "the receipt above is the evidence either way" and would otherwise point at a file that does not exist. _git_state_dir now normalises both branches, so the answer is a property of the file rather than of which branch answered — the mixed-separator path (C:/…/.git\…) that reddened the windows-2025 leg can no longer be produced. scripts/recover-worktree.py asks os.path.isfile, not the computed path, so its could not be written branch is reachable.
Mutation arm, run here on the landing tree. _receipt_note returning "" again (the pre-fix silence) → tests/test_scheduler.py::test_a_receipt_that_cannot_be_written_is_reported_not_silent FAILED; the same tree unmodified → 1 passed. emrg/server/scheduler.py restored to sha256 6d90a1aa4315a38c before the suite run for the record. So the sentence is a discriminator, not a constant.
Tests I ran on the landing tree: tests/test_recover_worktree.py tests/test_scheduler.py → 136 passed. Both CI legs green on the head (run 35093090205).
The recovery itself stays best-effort by design — the stash is the durable record — and this reports the missing receipt rather than undoing a verified recovery, which is the right direction for a best-effort guarantee.
Fixes item 1 of issue #1284.
The defect
TaskHandler._write_recovery_receiptreturnsstr | Nonefor exactly the failure itanticipates — and the value was discarded at its only call site. So with the receipt
path pre-created as a directory (
open(..., "w")raisesOSError), the action reported:scripts/recover-worktree.pyhad the same gap in a weaker form: it re-derived the path andprinted
receipt: <path>without asking whether a file was there, so itsreceipt: could not be writtenbranch was unreachable.Both matters because the property the documentation claims is that a release of a safety
rule leaves a receipt. The recovery itself is best-effort by design — the stash is the
durable record — so the fix is to report the missing receipt, not to undo a verified
recovery over it.
The change
emrg/server/scheduler.py: capture the return value; a new module-level_receipt_note()appends
"; no receipt could be written (the stash is the durable record)"to theaction's detail on both exits (the success one and the unexpected
HEAD movedone,whose text claims "the receipt above is the evidence" either way); the two ways the
receipt can go missing are also logged (
logger.warning).scripts/recover-worktree.py:os.path.isfile(receipt)instead of trusting the computedpath — asking whether something exists there is also true of the directory the write
failed against, which the test caught while the first version of this line used
os.path.exists.DEVELOPMENT.md+ both docstrings: the receipt contract now says it is best-effort andthat the report says so when one could not be written.
Verification
Full suite on the change: 2725 passed, 16 skipped (master's tree with
node_modulesis2723/16; the worktree arms report one extra skip because a fresh worktree has no
node_modules, and the difference here is +2 new tests).importcheck andemrg --helpOK;scripts/check-doc-count.pyOK (no tracked file states a count).Each new test carries its control arm so the sentence is a discriminator and not a
constant, and each was mutation-tested in the direction it exists for:
_receipt_notereturns""again (the pre-fix silence)test_a_receipt_that_cannot_be_written_is_reported_not_silentFAILED; clean arm passestest_the_tool_says_when_the_receipt_could_not_be_writtenFAILED; clean arm passesFiles restored byte for byte after each arm (sha256 recorded before, compared after).
Item 2 of #1284 (the advertised reversal spelling is not the inverse on a geometry the
criterion releases) is left open: it is a wording change in three places that has to
preserve
--index, and it is measured in the issue's own table.