emrg: name the zip archive a lone -@ operand writes (issue #1529) - #1531
Conversation
|
I tested this PR against the issue it fixes and every row holds, plus two cluster spellings the table does not spell and a measurement of the named limit's scope. Setup: master The rows the fix is about, all moving the way the PR says: and the head's target list for those rows is Two spellings I added to the cluster claim — the PR argues The controls do not move: The named limit is scoped exactly as the docstring says, and I checked the direction that would have mattered. The over-block ( So the over-block cannot reach a real archive run: with two operands the exemption is not consulted at all, and the inside-with-a-list row is untouched in both trees. Worth keeping in the docstring exactly as written, since that is the row a later reader will test. On the head, |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-020120
Reviewed the landing tree: check-merge-landing-diff.py 1531 says every path in diff(base, head) is landed (2 paths: bash_tool.py, tests/test_bash_tool_zip_archive.py), and check-merge-plan-suite.py 1531 measures tree 7a7e5e8006a3 at 4776 passed / 22 skipped. CI: both legs pass (run 35621239621).
Ground truth reproduced on this host, not taken from the issue (Info-ZIP 3.0, /usr/bin/zip, each row in a fresh mktemp -d):
printf 'f\n' | zip -@ a.zip rc=0 a.zip created
zip b.zip rc=12 nothing created
zip --@ c.zip f rc=16 nothing created
zip -@- d.zip f rc=16 nothing created
So the two rows the PR asks about are exactly as it states them: the same lone operand means two different runs depending on the name source, and the two refused spellings cannot write.
Predicate, read on both trees (module imported from the head's own worktree, and from master for the control):
master head
zip -@ <outside>/a.zip targets [] targets ['<outside>/a.zip'] read-only BLOCK
zip <outside>/a.zip targets [] targets [] (the exemption's own row)
zip --@ / zip -@- <outside>/a.zip targets [] targets [] (zip rejects both, rc=16)
zip -q@ / -@q <outside>/a.zip targets named (the cluster spellings)
That is the hole issue #1529 reported (ALLOW at both tiers while the run creates the archive) closed at the point it exists, without touching the neighbouring exemption — and the file's own blinding arm restores the hole on purpose, so the claim hangs on _zip_takes_names_from_stdin rather than on the operand count.
One residual, stated for the record rather than as a blocker: the new refusal is unconditional on -@, so zip -@ a.zip with empty stdin (rc=12, nothing created — measured above with < /dev/null) is refused too. That is a false block in the fail-closed direction on a spelling whose names arrive on a stream no static walk can read, and it is the correct direction for this guard; the PR's docstring already frames the operand count as "not the question" under -@.
Nothing else in the diff needs changing.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-032844
Re-measured this cycle on the PR's own tree (detached worktree at 85b5589b, module path asserted) and on the host's own /usr/bin/zip (Info-ZIP 3.0, Apple), one fresh directory per row:
Ground truth, the reason this rule exists:
| row (stdin carries a name list) | rc | on disk |
|---|---|---|
zip -@ a.zip < list |
0 | a.zip written — the operand is the archive, the names come from stdin |
zip -@ < list (no operand) |
16 | nothing written, and an error |
zip -@ a.zip < empty |
12 | nothing written — the same "nothing to do" as the bare form |
So -@ a.zip is not the "nothing to do" shape: the archive is named and -@ moves only the name source to stdin.
The rule's verdicts on that tree (both tiers, plus the negative controls the PR keeps):
| row | read-only / workspace-write | named target |
|---|---|---|
zip -@ <outside>/a.zip |
BLOCK / BLOCK | the archive |
zip -@ <workspace>/a.zip |
BLOCK / ALLOW | the archive |
zip -@ (no operand) |
ALLOW / ALLOW | none — the negative control |
zip --@ |
ALLOW / ALLOW | none |
zip -@- <outside>/a.zip |
ALLOW / ALLOW | none — -@- is the form that reads from a file, so the operand is not an archive |
zip <outside>/a.zip <workspace>/in |
BLOCK / BLOCK | the archive (unchanged form) |
The two controls that matter are preserved: the no-operand spelling stays allowed (it writes nothing, measured rc=16/12 above), and the existing writing form is untouched. check-merge-freshness.py 1531 → FRESH (merge base was master's tip when the run was made); CI green on both legs.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-051231
Measured on the tree this merge would land, not on the head's own CI (the head's base is 1f2feef, three commits behind master):
uv run --no-sync python3 scripts/check-merge-plan-suite.py 1531
base 6126273d (refs/remotes/origin/master), 1 PR(s) planned
final tree efcdc1ed9ad1 (efcdc1ed9ad1062efe843a517e596f99db45ed1e)
suite OK: 4823 passed, 22 skipped in 148.13s
scripts/check-merge-landing-diff.py 1531 → the landing change is two paths (M emrg/tools/bash_tool.py, M tests/test_bash_tool_zip_archive.py); the seven other paths in diff(base, head) are the base's own later commits, which it names as reversals this PR does not make — including emrg/tools/bash_tool.py itself, so the diff is not the landing there.
The ground truth, re-measured on this host (Info-ZIP 3.0, Apple)
zip arch rc=12 nothing on disk
printf 'f.txt\n' | zip -@ arch2 rc=0 creates arch2.zip, "adding: f.txt"
Same operand, different run: the count is not the question, the name source is. The head is the file I drove below.
Differential, master's file against the landed file
Master's bash_tool.py out of the object store at 6126273d and the landed one out of the merged tree (git merge-tree --write-tree master FETCH_HEAD → efcdc1ed9ad1…, identical to the plan suite's tree, so the file I read is the file that lands), loaded side by side as two modules, _extract_write_targets + _check_sandbox at both tiers, one workdir this probe created, nothing executed:
row targets (master → landed) master landed
zip -@ /outside/emrg/a.zip [] -> ['/outside/emrg/a.zip'] ALLOW/ALLOW BLOCK/BLOCK
printf 'f\n' | zip -@ /outside/emrg/a.zip [] -> ['/outside/emrg/a.zip'] ALLOW/ALLOW BLOCK/BLOCK
zip -q@ /outside/emrg/a.zip [] -> ['/outside/emrg/a.zip'] ALLOW/ALLOW BLOCK/BLOCK
zip -@q /outside/emrg/a.zip [] -> ['/outside/emrg/a.zip'] ALLOW/ALLOW BLOCK/BLOCK
zip -@ -m /outside/emrg/a.zip [] -> ['/outside/emrg/a.zip'] ALLOW/ALLOW BLOCK/BLOCK
zip -@ <workspace>/a.zip [] -> ['<ws>/a.zip'] ALLOW/ALLOW BLOCK/ALLOW
zip -P -@ /outside/emrg/a.zip [] -> ['/outside/emrg/a.zip'] ALLOW/ALLOW BLOCK/BLOCK
zip -x -@ /outside/emrg/a.zip [] -> ['/outside/emrg/a.zip'] ALLOW/ALLOW BLOCK/BLOCK
zip -@a /outside/emrg/a.zip [] -> ['/outside/emrg/a.zip'] ALLOW/ALLOW BLOCK/BLOCK
zip -@ /outside/emrg/a.zip README.md unchanged BLOCK/BLOCK BLOCK/BLOCK
zip /outside/emrg/a.zip unchanged ALLOW/ALLOW ALLOW/ALLOW
zip -d /outside/emrg/a.zip unchanged ALLOW/ALLOW ALLOW/ALLOW
zip --@ /outside/emrg/a.zip unchanged ALLOW/ALLOW ALLOW/ALLOW
zip -@- /outside/emrg/a.zip unchanged ALLOW/ALLOW ALLOW/ALLOW
zip <workspace>/a.zip / -d <ws>/a.zip unchanged (controls) ALLOW/ALLOW ALLOW/ALLOW
7z a -@ … / tar -cf … unchanged ALLOW/ALLOW ALLOW/ALLOW
Nine rows move and every one is the -@ family. Six are the issue's closings; the other three (-P -@, -x -@, -@a) are the same envelope, and I measured them against real zip rather than taking the direction on trust:
zip -P -@ arch rc=12 writes nothing -> now BLOCK (false block, documented by the PR)
zip -x -@ arch rc=16 writes nothing -> now BLOCK (same shape; my row, not the PR's)
zip -@a arch rc=16 writes nothing -> now BLOCK (same shape; my row, not the PR's)
zip --@ arch rc=16 writes nothing -> stays ALLOW (the exemption ke kept)
zip -@- arch rc=16 writes nothing -> stays ALLOW
So the direction is a loud false block on runs that write nothing, never a silent allow — the trade this file makes everywhere, and the same direction the earlier -@ docstring's own "limit" clause was wrong about.
The predicate is load-bearing, not the observation
Re-ran the PR's blinded control in process on the landed file: _zip_takes_names_from_stdin answered normally → BLOCK with targets=['/outside/emrg/a.zip']; patched to answer False → the same row returns to ALLOW with targets=[]; restored → BLOCK and the target named again. So the test asserts the predicate rather than one run's output.
The suite on the landing tree is green (4823P/22S) and includes the three replaced arms. The over-block is written into the docstring instead of left to be discovered, which is the right place for it.
Fixes #1529.
sandbox: a lone operand is not always "nothing to do" —zip -@writes the archive while both tiers allow itThe bug
_zip_write_targetsexempts a run with fewer than two operands, becausezip a.zipalone is rc=12 ("Nothing to do!") and creates nothing — that exemption is what keepszip a.zipandzip -d a.zipallowed. Its own docstring then named-@as a limit: "-@reads its names from stdin, which the walk cannot see: that spelling stays unnamed".Both statements are true; they do not compose.
-@is exactly the option that gives the lone operand a name source, so the exemption returns before the first operand is ever looked at, and the write it exempts is a write nothing names. Measured by the issue's reporter on Info-ZIP, and re-measured here:Same operand, two runs — the count is not the question.
The fix
-@is a name source, not an option, and it is carried as one letter of a short option word (zipcombines short options, so-@,-q@,-@qare the same request). The new_zip_takes_names_from_stdin(words)reads the option words for that letter, and the lone-operand exemption now also requires that it answers no.Differential, master's file (loaded out of the object store at this branch's base
1f2feef) against this branch's, driven through_extract_write_targets+_check_sandboxat both tiers,<outside>=/outside/emrg:zip -@ <outside>/a.zip[]printf 'f\n' | zip -@ <outside>/a.zip[]zip -q@ <outside>/a.zip[]zip -@q <outside>/a.zip[]zip -@ -m <outside>/a.zip[]zip -@ /workspace/a.zip[]zip -@ <outside>/a.zip README.md['/outside/emrg/a.zip']zip <outside>/a.zip[]zip -d <outside>/a.zip[]zip --@ <outside>/a.zip[]zip -@- <outside>/a.zip[]7 of 12 rows move; every row that moves is one the fix is about, and the two spellings that merely look like the hole keep the exemption because
zipitself refuses them (rc=16, nothing on disk):The last row of the table is the
-@case inside the workspace: it still BLOCKs read-only and still ALLOWs workspace-write, which is the tier split every other named write gets.Tests
tests/test_bash_tool_zip_archive.py— the arm that pinned the opposite (test_names_read_from_stdin_are_the_documented_residual, which asserted_extract_write_targets('zip -@ <outside>/a.zip') == []) is replaced by three arms:_zip_takes_names_from_stdinpatched toFalsemakes the hole come back and the same row ALLOW again, then restored and re-asserted, so the claim is about the predicate rather than about one observation of the command.uv run pytest tests/test_bash_tool_zip_archive.py tests/test_bash_tool_compressor_enumeration.py tests/test_command_position_contexts.py -q→ 321 passed, 9 skipped.Named limit, in the other direction
The scan reads the option words as written, so a
-@that is really another option's value (zip -P -@ a.zip, a password spelled like the flag) is read as the name source and the archive is named — an over-block, and one that lands on a run with no list, which writes nothing. Written into the docstring rather than left to be discovered.Caller trace cannot answer this one: the name source is a property of the option set, not of the operand count, and
stdinis not visible to the walk at any depth.