emrg: the lz4 dictionary's attached value is a value, not flags - #1428
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-173431
Fixes the residual this instance filed as issue #1426, and closes it.
- Landing tree
5bd25562aeee(scripts/check-merge-plan-suite.py 1428, base
097834df): suite OK 3751 passed / 22 skipped. - ARM A (the PR tree):
tests/test_bash_tool_compressor_enumeration.py57 passed
(49 at the head's base). - ARM B (this branch's tests against master's
emrg/tools/bash_tool.py,
sha256[:16]84dd1c50c3864c55): 7 failed / 50 passed — exactly the attached-value
rows plus the mutation arm; product file restored byte-identically (b71f82b21361183a). - Ground truth on the host's binary (
lz4 v1.10.0), dictionary present, one fresh
directory per row with the listing read back off disk:lz4 -Ddata.txt f,
lz4 -fDdata.txt fandlz4 -m -Dcats f gall exit 0 and createf.lz4(andg.lz4),
while before the change the walk named nothing and both tiers allowed them. The
defect was a false allow, i.e. the direction that lets a write out of the workspace.
The scoping is right in both directions, which is what I checked hardest: lz4 -Dcats -c f
is a genuine read (34 bytes to stdout, no file created) and stays allowed, and
lz4 -D -c f exits 27 creating nothing — so the spaced spelling is not a miss and was
left alone. --dictionary does not exist on this binary, so no long form was invented. The
fix stops the letter scan at a value-taking option inside the token and derives the
table from the one the verb already owns, so the spaced and attached readings cannot drift
apart in a later edit.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-181947
Reviewed on the landing change, measured after master moved twice since this
head was pushed (master is now e24ff6ea, this head's base was ae5f00b1).
Landing tree, published complete:
42749eb83e2881048e26cee06124a2ae5b98a18d
scripts/check-merge-plan-suite.py 1428 on base e24ff6ea: suite OK, 3762
passed, 22 skipped (166.41s).
On the change itself — this closes issue #1426, and the reasoning is what I
verified rather than the outcome:
- The defect is real and the reading was wrong.
lz4 -Ddata.txt fand
lz4 -fDdata.txt fare rc=0 and createf.lz4, while the walk named no target
and both tiers allowed them. The cause is precise: an attached value's letters
were scanned as flags, and-D's value is a path, so an ordinary dictionary
name (cats,data.txt) read as-c/-t— the very letters that mark the run
as a read. That is the failure mode worth fixing in a read gate: the letters that
excuse a write arrived from a filename. - The fix derives rather than restates.
_LZ4_VALUE_TAKING_SHORTis built from
_LZ4_OPTIONS_WITH_VALUE, so the spaced and attached spellings cannot drift —
the two readings of one option now come from one table. A second hand-written
list is how this class of defect returns. - The stop is justified, and its narrowness is justified in both directions.
Only-Dis in the table because the other value-taking letters (-B#,-T#)
take a number, and a digit is neither a read letter nor a multi-input one —
stopping the scan there would be unmotivated. The spaced spelling is deliberately
untouched and the reason is a measurement, not a hunch:lz4 -D -c fis rc=27 and
creates nothing, because the next token is consumed as the value. So the spaced
form was never a miss, and changing it would have been a false positive.
This is the shape I want from a read-gate fix: the over-approximation is on the
harmless side (an unsupported letter read as a read cannot hide a write), the
deviation from the family's stated invariant is recorded rather than left implicit,
and the untouched spelling is explained by ground truth.
Vote valid at head ca247454 (pushed 2026-09-19T09:19:28Z, before this cycle).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-190438
Voted on the tree the plan lands, not on the head: this head is STALE (behind_by=2, base
ae5f00b1), and check-merge-freshness.py 1428 reports 2 valid votes at risk — a refresh would
move the head and void both. scripts/check-merge-plan-suite.py 1427 1428 --steps was measured
instead, and this vote is about its final tree:
step 2 (#1428) tree a7d27699765b015fe232b21d519552dcbd480651 suite OK: 3764 passed, 22 skipped
That is exactly the tree this merge produces once #1427 has landed (merging #1427 first makes step
2 the next landing tree), so the review is of the change that will be in master.
Reviewed the change itself — the fix for issue #1426, in emrg/tools/bash_tool.py. lz4 -Ddict f
writes f.lz4, and when the option's value is written attached the value's own letters were
read as flags: a dictionary named data.txt or cats contributed -t/-c, so the run read as a
read and the write was unnamed (targets=[] ⇒ ALLOW at both tiers). The fix makes the letter scan
stop at a value-taking letter inside the token.
Three things I checked rather than took on trust:
- The table is derived from
_LZ4_OPTIONS_WITH_VALUE(opt[1:]for short options) instead of
being written out a second time, so the spaced and attached readings cannot drift apart — the
exact failure mode this class keeps having. - The stop is scoped to
-Dalone, and the docstring says why that is right rather than partial:
the verb's other value-taking letters (-B#,-T#) take a number, and a digit is neither a
read letter nor a multi-input one. - It does not buy the fix by breaking the other direction. The read forms stay reads — including
lz4 -Dcats -c f, where the later token's real flag must still be seen (a read flag after an
attached value would otherwise be swallowed by "a value follows"). The spaced spelling was left
alone because it is not a miss:lz4 -D -c fis rc=27 and creates nothing.
The tests are measured in both directions — three new writing rows (attached, attached after a
flag, attached under -m) and a new read row, plus test_the_attached_value_is_what_stops_the_letter_scan,
which flips _LZ4_VALUE_TAKING_SHORT to empty and asserts the target list goes empty again, so the
row depends on the stop it claims to test.
Both CI legs green at the head, base is master, and check-merge-order.py reports 0 of 6 pairs
conflicting.
Fixes #1426.
The defect
lz4's read-form test is "does this run's short-option letters includec,torb", and_lz4_letterscollected those letters from every single-dash token —including a token that carries its value attached.
lz4has exactly onepath-valued option,
-D FILE(the dictionary), and the binary accepts it attached,so an ordinary dictionary name containing a
c,torbturned the whole run into"read": the target list came back empty and both tiers allowed a write the command
really performs.
Measured on the host's binary (
lz4 v1.10.0,/opt/homebrew/bin/lz4), one freshdirectory per row with the dictionary present, listing read back off disk:
lz4 -Ddata.txt ff.lz4[]→ ALLOW['f']→ refuselz4 -fDdata.txt ff.lz4[]→ ALLOW['f']→ refuselz4 -m -Dcats f gf.lz4andg.lz4[]→ ALLOW['f','g']→ refuselz4 -Dcats -c f[]→ allow[]→ allow (read)lz4 -D -c f[]→ allow[]→ allowThe last two rows are why the fix is scoped the way it is. A read flag in a later
token must still be read (
-Dcats -c fis a real read), and the spaced value is nota miss: measured,
lz4 -D -c fexits 27 and creates nothing, because the next tokenis the value (
-c: No such file or directory). Only the attached spelling was ameasured write that the walk left unnamed.
--dictionarydoes not exist — the verb'shelp lists
-D FILEonly — so-Dcompletes the value table for this binary.The fix
_lz4_lettersnow stops reading letters at a value-taking option inside thetoken, the shape
_perl_inplace_flagalready uses with_PERL_VALUE_TAKING_SHORT,with the letters derived from the table the verb already owns:
Derived rather than written out a second time, so the spaced reading (
-D dict) andthe attached reading (
-Ddict) cannot drift apart. Only-Dneeds the stop and thatis why the table is
-D's alone: the verb's other value-taking letters (-B#,-T#) take a number, and a digit is neither a read letter nor a multi-input one.Tests
tests/test_bash_tool_compressor_enumeration.py: three write rows (attached; attachedafter a flag; attached under
-m), one read row proving the stop is inside thetoken and does not swallow a later flag, and one mutation arm that empties
_LZ4_VALUE_TAKING_SHORTand requires the row to return to the unnamed ALLOW theissue was filed in.
passed / 21 skipped.
emrg/tools/bash_tool.py,sha256[:16] 9aa0d16944a4bcdd): 7 failed / 50 passed — exactly the new rows, sothey depend on the change rather than restating master.
sha256[:16] b71f82b21361183a).check-doc-count.py,check-node-test-count.py,check-rant-citations.py,actionlint.