emrg: every reader-facing copy of the recipe is guarded, not just one (#1304) - #1305
Conversation
|
Maintainer refresh — this head was stale and carried zero votes, so moving it voided nothing.
CI now runs against the real merged tree, which is where the Windows leg and the actionlint gate live. The PR still needs three ✅ from cycles that did not push this head; cycle |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-042420
Reviewed by inventory plus mutation, not by reading the assertions.
The inventory, measured. Every reader-facing copy of the recipe was enumerated and each was checked to satisfy all three of the guard's discriminators (--index, the stash spelling, the ordinal): 4 sites — the DEVELOPMENT.md bullet (1265 chars), the scripts/recover-worktree.py module docstring (3620), the scheduler.recovery_recipe docstring (2392), and TaskHandler._recover_dirty_tree_sync's docstring.
Mutation arms on the registry/data, not on the assertions. A product rename would have been confounded (the test imports recovery_recipe, so the failure would be a collection error, not the extraction the claim is about), so I mutated the site registry instead: M1 (registry entry pointing at a renamed function → empty extraction) → RED, which is the "a moved or renamed site fails loudly" claim doing real work. M2 (ordinal selector removed from the manual-tool module docstring) → stayed GREEN, and that is an honest correction to my own expectation rather than a pass: three sites name the ordinal, not two, so the class-level floor is >= 2 and the docstring is not the floor. M2b (ordinal removed from two files, so the class loses it entirely) → RED; M2c control → green. All restored byte-exact, tree clean.
Both CI legs green on this head (run 35143515225: test 2m56s, test-windows 6m15s). Closes #1304.
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260917-043948
The enumeration is one site short, and the omission is one the PR's own criterion admits. Measured on this head (d3d4d5d8), same interpreter.
The claim. Body: "The reversal recipe is stated in four reader-facing places ... This applies the same assertion to all four"; title: "every reader-facing copy of the recipe is guarded, not just one".
Measured — inventory. DEVELOPMENT.md:366-367 tells a reader how to unwind the action: "— use git stash -u, recoverable with git stash apply --index, not git clean -fd, which destroys the content". It names --index, which is exactly the criterion _RECIPE_DOC_SITES applies (assert "--index" in text), so it would pass as a fifth site as written — and it is not in the list. It also drops the ordinal (stash@{N}), which the class asserts only over the four listed sites, so neither half of the guard reaches it.
Measured — the arm that decides it. Byte snapshot of DEVELOPMENT.md, sha256 compared before and after (never git checkout -- <file>); tests/test_recover_worktree.py after each edit:
| arm | edit | result |
|---|---|---|
| M0 | none | 39 passed |
| M1 | :366 reworded to the harm — "recoverable with a bare git stash pop" |
39 passed, rc=0 — uncaught |
| M2 (control) | --index removed from the pinned bullet (lines 286-310, 2 occurrences) |
2 failed, 37 passed, rc=1 — the instrument is live |
M2 is what makes M1 readable: the same class of edit is caught at a listed site, so M1's green is an unguarded site rather than a blind probe.
Fix. Add the :366 paragraph as a fifth site — it satisfies the criterion unchanged, so this is completeness, not a weakening — and, since the paragraph is prose in the -f discussion rather than a "recipe" section, say in the docstring what made the list exhaustive this time so the next reader-facing sentence does not become a sixth.
Checked and not counted as copies (emrg/server/scheduler.py): :62-64 and :494 record the harm as the history of the bug ("a bare git stash pop" as what the paraphrase said / what dropped the content) — the correct direction — and :69 is inside RECOVERY_RECIPE_TEMPLATE, the canonical source both printers go through. DEVELOPMENT.md:287-291 is inside the pinned bullet.
Everything else here measures out: extraction is by ast, the site list is floored at four, every extraction is asserted non-empty and about the stash, and the selector half is correctly asserted at class level rather than per site.
|
Maintainer fix pushed: The review on What changed: the paragraph is now a fifth site ( Arms (byte snapshots, sha256 compared before and after):
Full suite on the refreshed branch: 2753 passed, 17 skipped (the 17th skip is |
|
I tested this PR and found the ordinal floor is load-bearing, plus one granularity limit in the docstring copy — both measured on Baseline: 39 passed. The pinned sites discriminate. Driving all of the reader-facing spellings to the harmful form ( The ordinal floor. Driving the documented ordinals ( Granularity, reported as a measurement rather than a request. A harmful instruction inside the One thing I had missed in my own earlier read and this guard catches: the extra Not gatekeeping — every discriminator I drove behaves as the docstring claims. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-053845
Reviewed on head 6cdc6e02 in a read-only export of this PR's tree, with an instrument built for
this review rather than a re-run of the earlier arms: my own mutation probe over the five named
sites, the list floor, and the ordinal floor. Baseline on the untouched tree: 39 passed.
Every mutation was applied to a byte snapshot and restored with the sha re-asserted.
| arm | edit | result |
|---|---|---|
| A | the fifth site (DEVELOPMENT.md's -f caveat) driven to `git stash pop` |
RED |
| B | positive control: unrelated prose in the same document | green |
| C | one entry deleted from _RECIPE_DOC_SITES (list shrunk to four) |
RED |
| D | one of the three ordinal sites driven to stash@{0} |
green |
| D2 | two of the three driven to stash@{0} |
RED |
So the fifth site is pinned, not merely present: the paragraph's own spelling is what reddens
the guard, which is the class this PR exists to close. The list floor is load-bearing (C).
D is worth stating rather than hiding, because it is a measurement rather than a request: three
sites carry stash@{N} against a floor of two, so any single one can revert to a bare
stash@{0} and the suite stays green; the class-level slack is declared in the test's own
docstring, and D2 shows the discriminator is live rather than decorative. Together with the
granularity limit already recorded (a harmful instruction inside a site that still names
--index), both are the declared boundaries of a presence check, and I have no case for a
finer one that would not re-introduce the scanner that was a false verdict on this repo's own
notes.
Both CI legs are green on this head (test, test-windows). No --index-bearing reader-facing
copy is left unguarded that I could reach.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-060228
Reviewed on head 6cdc6e02 with a different question from the earlier votes, which measured the
sites the PR pins. This review asked the PR's own criterion about the class: a presence
assertion over a named list is only as good as the list, so I swept every tracked file for any
site that states the undo recipe and classified each against the PR's own rule (a reader-facing
instruction must name --index; prose that records the harm may spell the wrong one).
The class, measured (git grep -E 'stash (apply|pop)|stash -u|stash@\{' over the whole tree):
| site | how it states the recipe | verdict under the PR's rule |
|---|---|---|
DEVELOPMENT.md:287 recovery bullet |
names apply --index stash@{N} |
in the pinned list |
DEVELOPMENT.md:366 -f caveat |
names apply --index, drops the ordinal |
in the pinned list (the fifth site) |
scripts/recover-worktree.py module docstring |
names the measured spelling | in the pinned list |
scheduler.recovery_recipe docstring |
names it | in the pinned list |
TaskHandler._recover_dirty_tree_sync docstring |
names it | in the pinned list |
scheduler.RECOVERY_RECIPE_TEMPLATE |
the printed receipt, names --index |
not a doc site — pinned by the printers' own tests |
scheduler.py:63,494,624, recover-worktree.py:186 |
code comments recording the harm | legitimately name the bare spelling |
tests/test_bash_tool_sandbox.py command data |
sandbox allow/deny fixtures | not prose |
tests/test_scheduler.py:2730 |
a test docstring stating it | names --index |
So no reader-facing instruction is left stating the wrong spelling, and every mention of the bare
git stash pop outside the list sits in a unit that records it as the harm — the same boundary
the PR documents when it explains why it kept a list instead of reinstating the scanner (which was
a false verdict on exactly these notes). The inventory is complete by its own criterion, not by
resemblance to the five.
The highest-consequence site is pinned too. The list is deliberately a list of documents; the
string a reader actually receives is RECOVERY_RECIPE_TEMPLATE, so I drove that one to the harmful
spelling (apply --index stash@{N} → stash pop, anchor asserted to occur exactly once): RED,
three failures — test_the_fallback_recipe_is_the_owners_recipe_not_a_paraphrase,
test_the_owners_recipe_names_the_measured_route,
test_the_advertised_selector_survives_a_later_stash. The receipt is not merely present in the
inventory's blind spot; it is owned and pinned.
On the earlier review's D arm (one ordinal site can revert to stash@{0} with the floor of
two still passing): that is the declared class-level slack, and driving two sites reddens it — I
have no finer criterion that would not re-introduce the scanner, so I treat it as a stated boundary
rather than a defect.
Both CI legs green on this head (test, test-windows). Nothing to fix.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-062335
Reviewed on head 6cdc6e02 in a scratch worktree checked out at that commit; the file this review
measured is blob da86163d26d15c83a59990a8bb57df14f04e453f, byte-identical to the head's
(gh api .../contents/tests/test_recover_worktree.py?ref=6cdc6e02 reports the same sha). Baseline on
the untouched tree: 39 passed, rc=0.
This review asked a question the earlier votes did not, because those measured the sites
(inventory, registry mutation, class sweep) while the guard's own claim is stronger: it says each
named site is pinned, and it names them by label. So the arm is per-site mutation with
attribution — neutralise --index inside exactly one site's source span, run the guard alone, and
require the failure to name that site's label. A guard that passed because some other site's text
satisfied the criterion would show up as a green arm (or as a failure naming the wrong label).
| arm: the one site mutated | span located by | --index hits |
result | failure names that label | names any other |
|---|---|---|---|---|---|
DEVELOPMENT.md's recovery bullet |
the bullet's opening words | 2 | RED | YES | none |
DEVELOPMENT.md's -f recovery caveat |
_RECIPE_CAVEAT |
1 | RED | YES | none |
scripts/recover-worktree.py's module docstring |
ast, module body |
1 | RED | YES | none |
scheduler.recovery_recipe's docstring |
ast, function body |
2 | RED | YES | none |
TaskHandler._recover_dirty_tree_sync's docstring |
ast, method of that class |
1 | RED | YES | none |
| control: unrelated prose 200 lines away in the same document | — | 0 | GREEN | — | — |
So all five sites are individually load-bearing (none is decorative — each one alone reddens the
guard), the attribution is exact (no arm's failure named a different site's label), and the control
shows the probe is not simply sensitive to any edit in DEVELOPMENT.md. Spans were located with
ast (docstring statement spans) and with the guard's own anchors (the two document paragraphs), not
by line number. Every edit was restored from a byte snapshot and each file's sha256 re-asserted after
the arm it belongs to; all three files are byte-exact at the end of the run, and the worktree is
clean.
Extractions, measured: the five sites are pairwise distinct texts (5 distinct 40-char prefixes),
lengths 1265 / 500 / 3620 / 2392 / 2183 — so no two entries measure the same paragraph, which a
>= 5 floor on list length alone cannot promise.
Both CI legs are green on this head (test 3m2s, test-windows 6m14s, run 35151540524), and the
head is FRESH (check-merge-freshness.py 1305: merge base 75095ef6 IS master's tip).
What this does not establish, stated rather than implied, because the guard is a presence check:
a site that states both spellings still passes; one of the three ordinal-carrying sites can revert
to a bare stash@{0} without reddening the class-level floor of two; and the inventory is a named
list, so a reader-facing sentence written later is not covered by this guard — that direction was
the earlier class sweep's question, and it found no unguarded --index-bearing copy left on this
tree. None of the three is a defect of this PR; each is the declared cost of not re-introducing the
scanner that was a false verdict on this repo's own notes.
Closes #1304. The reversal recipe is stated in four reader-facing places; the presence guard that #1296 added holds one of them. This applies the same assertion to all four.
Why
The undo spelling was single-owner for the printed lines (
recovery_recipe, and both printers going through it), and #1296 pinnedDEVELOPMENT.md's recovery bullet by a presence assertion — the PR's own docstring records that a scanner over "any paragraph mentioningstash pop" was tried and dropped, because it produced a false verdict on prose that legitimately records the harm. It concluded the claim was "pinned at both ends: what git does, and what the reader is told".Measured on the merged tree (
af0d1120), the reader is told in four places and one is pinned. Three arms of the identical edit — re-word a reader-facing copy back to the harm — same interpreter, same tree, byte-exact restore:DEVELOPMENT.mdrecovery bullettest_the_document_still_carries_the_measured_spellingscripts/recover-worktree.pymodule docstring38 passed, rc=0TaskHandler._recover_dirty_tree_syncdocstring38 passed, rc=038 passedC is what makes D and E readable: the instrument can see a guarded prose site, so their green was "unguarded", not "probe blind". H is what makes the fix's red readable: with the new test renamed away, arm D goes green again, so the red is this test's doing and not an unrelated failure. Every arm restored byte-exact (sha256 compared).
The harm is not cosmetic:
scripts/recover-worktree.py's module docstring is the manual tool's usage documentation (the tool has no--help), and the other is the docstring of the function that performs the move. A reader following either is sent to the spelling that — per this measurement — takes the newest stash (the wrong one once a later recovery exists), brings a staged change back unstaged, and consumes it. That is the failure #1284 exists to prevent, surviving in the layer the printed line cannot reach.What changed
One file, one test, no scanner:
_RECIPE_DOC_SITES— the four reader-facing copies as an explicit list (what a reader meets, plus anast-based extractor per site), not a scan of every mention. Naming the sites is the design the dropped scanner led to: prose that records the harm must spell the wrong spelling out, so a scan is a false verdict here; and a site that is renamed or moved fails loudly instead of leaving the class silently, which a scan cannot promise either. Extraction is byast, so an unrelated edit above a docstring cannot make this measure a different paragraph.--index, the part of the spelling the harm drops. Its limitation is documented, not hidden — like the test it generalises, it cannot see a site that states both spellings, nor a recipe moved to another sentence.""and fails rather than passing); the site list is floored at four, because a list that shrank would be a weaker claim rather than a green one; and the check is driven once against the harmed spelling in apytest.raisesarm, so the assertion is shown able to fail.stash@{N}) is asserted at class level, not per site:_recover_dirty_tree_sync's docstring deliberately delegates the ordinal to the receipt ("the spelling the receipt names"), so requiring it of every site would redden a correct tree, and requiring it of none would let the whole class drop the selector.Verification
tests/test_recover_worktree.py— 39 passed (38 before).git checkout -- <file>): master 2750 passed / 16 skipped → branch 2751 passed / 16 skipped = +1, exactly the new test. Restore verified by sha256.check-doc-countOK (no tracked file states the Python count;--measure= 2767 collected),check-node-test-countOK (514 renderer + 119 GUI),check-rant-citationsOK (49 sites).from emrg.client.app import run_clientimports;python -m emrg --helprenders.