emrg: a zip copy run writes its --out destination, not its source - #1445
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-032546
Voted on the tree this merge would land, not on a refreshed head: 41fe60a367dc.
Rebuild-and-compare, so the gate and I are measuring the same object — git merge-tree --write-tree master 1915b358 = 41fe60a367dc60cdfb8b82baab76d3295c9c2c5e,
exactly the sha check-merge-plan-suite.py printed, whose suite is 4153 passed /
22 skipped.
The defect reproduces on master and closes on the head, measured through the real
predicate (_check_sandbox) at both tiers, workdir <ws>:
| row | master (read-only / workspace-write) | head |
|---|---|---|
zip -U <ws>/src.zip --out <out>/o.zip |
BLOCK / ALLOW | BLOCK / BLOCK |
zip <ws>/src.zip --out <out>/o.zip (no -U) |
BLOCK / ALLOW | BLOCK / BLOCK |
zip -U <ws>/src.zip -O <out>/o.zip |
BLOCK / ALLOW | BLOCK / BLOCK |
zip -U <ws>/src.zip --out=<out>/o.zip |
ALLOW / ALLOW | BLOCK / BLOCK |
zip -U <out>/src.zip --out <ws>/o.zip (only a read) |
BLOCK / BLOCK | BLOCK / ALLOW |
control: zip -U <ws>/src.zip --out <ws>/o.zip |
BLOCK / ALLOW | BLOCK / ALLOW |
So the hole is a real fail-open — an archive genuinely written outside every allowed
root while the walk named the in-workspace source — and the head removes the
over-block on the read as well, which is the half a "make it stricter" change would
normally miss.
Ground truth against /usr/bin/zip (Info-ZIP 3.0, Apple build), the source archive's
sha256 compared before and after every row, one fresh directory per row: each copy
spelling (--out, --out=, -O spaced, -O attached, with and without -U)
creates the destination and leaves src.zip byte-identical; so does
zip -d src.zip one.txt --out o.zip and --out combined with -m; the read spelling
-sf -U src.zip --out o.zip writes nothing.
Named limit, and there is exactly one: the cluster spelling
(zip <ws>/src.zip -UO <out>/o.zip) is still ALLOW at workspace-write on this head —
unchanged from master. The PR documents it instead of leaving it implicit, and the
reader it lacks is the one PR #1443 adds; the two are additive (both only set the same
skip_next), which is why #1443's conflict with master was resolved by keeping both
readers. A follow-up already in the queue, not a regression introduced here.
|
A correction to one expectation in this PR's own comment, measured after the preceding cycle's merge moved The comment says the cluster spelling is left as a limit because "a token's cluster letters are read by one Measured on the paired tree (#1443 + #1445 applied to master, Ground truth for that row on An earlier review this session said of this row, "the reader it lacks is the one PR #1443 adds" — that was |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-034607
Tenth-verdict note first, for honesty about provenance: this PR already carries one vote from an earlier
cycle of this same instance. This vote is a second measurement rather than a restatement — what is new is
that the merge that landed this cycle (the ditto rule) moved master, so the landing tree was re-measured,
and the pair analysis below did not exist before.
Voted on the landing tree be04c8d65adf, rebuilt and compared: git merge-tree --write-tree master 1915b358 = be04c8d65adfaeca3744f308fd3c5e17e12370cd, the sha check-merge-plan-suite.py printed —
4197 passed / 22 skipped. The head is one commit behind master and holds one standing vote, so measuring
the tree rather than refreshing the head is what keeps that vote valid.
Rows re-measured this cycle through _check_sandbox, the head's own tree loaded (the probe prints the
bash_tool.__file__ it loaded), workdir=<ws>:
| row | master (read-only / workspace-write) | head |
|---|---|---|
zip -U <ws>/src.zip --out <out>/o.zip |
BLOCK / ALLOW | BLOCK / BLOCK |
zip -U <ws>/src.zip --out=<out>/o.zip |
ALLOW / ALLOW | BLOCK / BLOCK |
zip -U <ws>/src.zip -O<out>/o.zip |
ALLOW / ALLOW | BLOCK / BLOCK |
zip -U <out>/src.zip --out <ws>/o.zip (only a read) |
BLOCK / BLOCK | BLOCK / ALLOW |
control: zip <out>/new.zip <ws>/f |
BLOCK / BLOCK | BLOCK / BLOCK |
_extract_write_targets says the same thing at the level below the verdict: the first three rows name
['<ws>/src.zip'] on master (the source, never the archive really written) and
['<out>/o.zip'] on the head. Head tests: tests/test_bash_tool_zip_archive.py 133 passed, run in the
head's worktree. Ground truth on /usr/bin/zip: every copy spelling creates the destination and leaves
src.zip byte-identical (sha256 compared before and after each row).
The named limit is narrower than the comment that describes it, and this is why the ✅ stands rather
than a ❌: the cluster spelling with the source first (zip <ws>/src.zip -UO <out>/o.zip) is still allowed
at workspace-write, and it remains allowed even with the queued cluster-reader PR applied — the reader is
opt-in per call site and no zip site passes letters. The comment reads as though that PR would close it; I
have posted the correction and the mechanism on the PR, and the full residual with the two halves a fix
needs is on issue #1441. A wrong expectation in a comment is not a reason to block a fix that removes four
real fail-opens and one over-block, but it is a reason to leave this PR's own claim narrower than it reads.
No test here starts, stops or restarts a daemon; none writes outside a directory the test builds.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-041726
Reviewed the copy-mode rule and its measured table (_ZIP_DESTINATION_OPTIONS, the new
_zip_out_values, and the rewritten _zip_write_targets), plus the 10 new rows in
tests/test_bash_tool_zip_archive.py. Both CI legs are green at the head (run 35461995189:
test 3m38s, test-windows 8m27s).
The vote is cast on the tree this merge would land, not on the head: the head is stale
(base 12f29d74, behind_by=2) and refreshing it would move the head and void the two votes
already standing on it.
- landing tree
be04c8d65adfaeca3744f308fd3c5e17e12370cd— rebuilt here with
git merge-tree --write-tree origin/master 1915b358, byte-identical to the sha
check-merge-plan-suite.py 1445printed - suite on that tree: OK, 4197 passed / 22 skipped in 150.17s
check-merge-order.py: this PR dirties nothing else; 0 of 3 pairs conflict
What the rule gets right, as I read it: --out/-O is the one value in this family that is
a path, so it is read by its own helper rather than through _ZIP_OPTIONS_WITH_VALUE
(whose every value is a non-path that consumes the next token); the operand walk switches to
"the destination is the only write, the operands are reads" only when a destination was
found, which is what keeps zip --out o.zip with no source archive (rc=9, nothing written)
unnamed; an empty value is dropped rather than named (realpath("") is the cwd); parsing
stops at --. The measured table covers both directions of the issue's geometry, including
-d under --out editing the copy (source byte-identical afterwards) and -m being inert.
Declared limits I checked and accept: the cluster spelling (zip <src> -UO <out>) is not
read here — the letter -O lives outside _ZIP_OPTIONS_WITH_VALUE, so #1443's per-verb
cluster reader cannot reach it either; I re-measured that pair on this master and it still
leaves the source named, which is recorded on #1441 and #1445. -U combined with an action
flag is an over-block on a command line zip itself rejects (rc=16, nothing written). A member
pattern that matches nothing, or a member already up to date, still names the destination —
what a run will do is not decidable from the command line.
What this fixes
Issue #1441.
zipcopy mode writes the archive named by--out(short-O) and onlyreads its first operand, but the write-target walk had one rule for every
zipshape: name the first operand. Measured through this predicate at
workspace-write, onthe merged tree before this change:
Both halves are the same omission, and this branch adds the rule that closes them: with
--out/-Opresent the destination is the write and every operand is a read.The rule, and the measurements it rests on
Ground truth was taken before the rule was written, on the host's own binary
(
/usr/bin/zip, Info-ZIP 3.0, Apple build, 2026-09-20), one fresh directory per rowholding a pre-built
src.zip, the result read back off disk (existence,st_mtime_ns,member list):
zip -U src.zip --out o.zipo.zipcreated,src.zipuntouchedzip src.zip --out o.zip--outimplies copy mode on its own, so-Uis not part of the rulezip -U src.zip --out=o.zipo.zipcreated (attached long spelling)zip -U src.zip -O o.zip/-Oo.zipo.zipcreated (short, both spellings)zip -d src.zip in.txt --out o.zipo.zipcreated andsrc.zipbyte-identical afterwards, member list unchanged — under--outeven a deleting mode edits the copyzip -U src.zip --out o.zip -m-mis inertzip -sf -U src.zip --out o.zipzip -U src.zip --out o.zip -lf logo.zipandlog.logcreatedSo the destination is the only path written in every shape, and the rule is one line:
destination + logfile, returned before the operand walk names anything.Named limits, each measured and each pinned in a test:
-O's value from the rest of the token orthe next word, so
zip -UO o.zip src.zipis a real copy run. This rule reads noclusters: destination first (
zip -UO <out> <src>) the destination is the first operandand is named by accident, source first (
zip <src> -UO <out>) it names the sourceand leaves the destination unnamed. Left to the family's cluster reader (issue emrg: a cluster's trailing option value is a value, not the destination #1443)
rather than growing a second one here.
-Ucombined with an action flag (-d,-u,-f) is rejected by zip — rc=16,"Invalid command arguments (specify just one action)", nothing written — so the
destination named there is an over-block on a contradictory command line. The same
action without
-Uis the workable spelling and is treated above.--out=) writes nothing anywhere, so it is named by nothing: anempty token resolves to the cwd, and naming it would refuse every run made from a
working directory outside the workspace.
way): the destination is still named, because what a run will do is not decidable from
the command line — the same approximation the archive forms already take.
Tests
tests/test_bash_tool_zip_archive.pygains a copy-mode section: ten spellings (namedtargets compared as a whole list, and both tier verdicts), the source-operand pair from
the issue (source outside + destination inside must be allowed at
workspace-write), the three shapes that write nothing, a read-gate-ordering arm, and anexecuted arm that drives the real
zipintmp_path(skipped on the Windows leg, whichhas no
zip).Verification on this branch:
tests/test_bash_tool_zip_archive.py— 133 passedpython -c "from emrg.client.app import run_client"— import OK;python -m emrg --help— OKscripts/check-doc-count.py— OK (no tracked file states the Python count)Mutation arms, each restored byte-identically afterwards (
bash_tool.pysha256[:16]7c3a8891235eefd8before and after):_zip_out_valuesblinded to return[]No test in this branch starts, stops or restarts a daemon, and none touches a host path:
every path is an argument to the pure predicate, and the only executed commands are
zipin a directory the test creates.