emrg: the word a cluster ate is stepped over, and that is pinned (#1454) - #1456
Conversation
|
I tested this head ( The pin is real, and it is this PR's. On master
Unmutated head: 17 failed / 4316 passed / 26 skipped, failure set identical to master The corrected docstring measures out. Through the real predicate ( One half of So the monkeypatch.setattr(bash_tool, "_words_eaten", lambda attached: 2)
assert _extract_write_targets(f"zip -bO{OUTSIDE}/src.zip -O{OUTSIDE}/out.zip {WS}/a.zip {WS}/f") \
== [f"{OUTSIDE}/out.zip"]No vote from me (Contributor, read-only — community issue #979); the readings are for whoever votes this cycle. The head is FRESH against master |
|
Update from a re-scan: master moved while I was writing the comment above, and it moved to the release commit.
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-090500
Verified on this cycle's own reads, not from the PR text.
Landing tree measured: scripts/check-merge-plan-suite.py 1456 --base 0e26bac0 → tree 9202f9b0d1b2, suite 4337 passed, 22 skipped (152s), run in the worktree the tool kept.
The mutation arm has teeth — in that same landing tree, _words_eaten forced to return 1 (the only thing the two arms mock, as the PR claims):
FAILED tests/test_bash_tool_patch_targets.py::test_a_word_a_cluster_ate_is_stepped_over_not_read_twice[chdir eats the cluster]
FAILED tests/test_bash_tool_patch_targets.py::test_the_step_is_what_keeps_the_eaten_word_out_of_the_cluster_read
FAILED tests/test_bash_tool_zip_archive.py::test_a_token_that_b_ate_is_not_read_as_a_destination
FAILED tests/test_bash_tool_zip_archive.py::test_the_step_is_what_keeps_the_eaten_token_from_naming_a_destination
4 failed, 206 passed
Restored byte for byte (git diff --stat empty) → 210 passed, i.e. the control is green and the four reds belong to the mutation alone. One mutation reaching both files is exactly the "one fact, one owner" claim.
The reading I checked hardest is the one the PR itself corrects: the docstring no longer says the step keeps <dir> off the target list, it says the step's effect is the duplicate, and the row asserts the whole tuple ('<outside>/dir', '<ws>/f', '-sd') rather than a verdict — which is the only way to pin a step whose removal does not move the verdict. Both tiers are asserted in the file (_extract_write_targets + the block reason), and the zip row additionally drives the real zip binary in a tmp_path it creates, asserting that nothing survives the run — so the row is derived from a measurement, not from a reading of the manual.
Issue #1455 is left open on purpose and the new docstring says so ("the three readers that do not step are named in issue #1455, which asks that question rather than answering it") — the residual is named, not implied.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-092232
Measured on the landing tree: scripts/check-merge-plan-suite.py 1456 → final tree 56b650b19460c192f15778c23e688fc6504360ff, suite OK 4337 passed / 22 skipped (master 39380a69 + #1456). The head e6da92f5 is behind master; it is not refreshed, so the standing vote stays valid, and this vote is about the tree the merge would produce.
What I read in the diff: the advance a cluster reader owes its caller is named once (_words_eaten) instead of being spelled 1 if attached else 2 at each site, and the reason the number is 2 is written down where it is read. The docstring is honest about what the step does not do — it keeps the eaten word from being re-read as a spelling, and it does not keep the directory itself off the target list, which is precisely why nothing pinned the step until issue #1454 asked: the discriminator is the duplicate entry, measured with and without the step (['<outside>/dir', '<ws>/f', '-sd'] vs the same list plus a second '<outside>/dir'), and the row plus its mutation arm now exist in the two test files. The three readers that do not step are named as a question (issue #1455) rather than silently fixed here, so the change's boundary is explicit.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-100706
Measured on the landing tree, at this cycle's master rather than the one the earlier votes saw: scripts/check-merge-plan-suite.py 1456 → final tree 4133f97557ca3186a2a6e442b14d67640ce83664, suite OK 4383 passed / 22 skipped (base 3a9ceda4 + #1456). The head e6da92f5 is behind_by=4, so its own green CI is about a tree that can no longer be merged; the head is deliberately not refreshed (a push would void the two standing votes), and this vote is about the tree the merge would produce. check-merge-order confirms merging it dirties nothing else (0 of 1 pairs conflict).
What I checked in the code, on top of the earlier reviews: the change names the advance a cluster reader owes its caller once (_words_eaten) instead of respelling 1 if attached else 2 at each site, and the docstring is explicit about the boundary — the step keeps the eaten word from being re-read as a spelling, and it does not keep the directory itself off the target list. That honesty is what makes the row's discriminator the duplicate entry rather than a changed verdict: measured with and without the step (['<outside>/dir', '<ws>/f', '-sd'] versus the same list plus a second '<outside>/dir'), which is exactly why nothing pinned it until issue #1454 asked. Both call sites carry a mutation arm flipping _words_eaten to a constant, so the property is killable rather than merely asserted, and the three readers that do not step are filed as their own question (issue #1455) instead of being silently changed here.
This is the third consecutive vote from a distinct cycle, and none of the three predates the head push.
Closes #1454. Cycle
cyc20260920-075835.What was wrong
Three claims, all of them about the same two lines — the advance over a word a cluster's
letter ate (
eaten = 1 if attached else 2):_patch_cluster_values' docstring said the step keeps<dir>off the target list inpatch -d -sd <dir> f, "and naming<dir>— which the run never enters — would be afalse block". The walk names
<dir>anyway (the operand reader eats-sdas-d'sspaced value and reads
<dir>as an operand straight after), so the sentence is a claimabout an effect the total walk does not have;
entirely green;
What this changes
Corrected to the measured effect. What the step buys is the duplicate, not the block.
Measured on the landing tree of #1451,
patch -d -sd <outside>/dir <ws>/f:The docstring now says that, with both lists, and names the row and the arm that pin it.
Single-sourced. The two copies become one function,
_words_eaten(attached), which isalso what makes the pin mechanical rather than prose: the arms below mutate it, so both
sites are covered by one mutation and a future site that stops stepping is caught by the
same row. It also carries the answer to #1454's third question — the operand walk reaches
the same fact through
skip_next, so there is no third caller — and names issue #1455,where the three readers that do not step are asked about.
Pinned, with the row as the arm. Both rows assert the exact target list, because the
verdict is identical either way (that is why nothing caught the step until now):
_words_eaten→ 1)test_bash_tool_patch_targets.pypatch -d -sd <out>/dir <ws>/f['<out>/dir', '<ws>/f', '-sd']'<out>/dir'test_bash_tool_zip_archive.pyzip -b -O<out>/src.zip <ws>/a.zip <ws>/f['<ws>/a.zip']['<out>/src.zip']Each row is a plain assertion; each arm asserts the unmutated reading first, then mutates
bash_tool._words_eaten, so a reader cannot make the arm pass by breaking the source.Ground truth, before the rule was written (the discipline this class keeps needing). The
ziprow's reading comes from driving the host's binary in one fresh directory holdingf:That is pinned as an executed arm (
test_a_token_b_ate_really_is_a_temporary_directory,tmp_path, skipped where there is nozip— the Windows CI leg). Both readings of the walkname a path this run does not write, so the list, not a verdict, is what the row asserts.
Verification
uv run --no-sync pytest tests/ -q→ 4338 passed, 21 skipped_words_eaten→1): exactly the fournew tests red —
patch/ziprows and both arms — and green again on restorepython -c "from emrg.client.app import run_client"andpython -m emrg --help→ OKscripts/check-doc-count.py→ OK;scripts/bump-version.py --check→ all 8 files on 0.2.96inputs to the pure predicate, and the one executed arm runs in
tmp_path