emrg: split writes under its last operand, so the walk names the prefix - #1430
Conversation
|
Merge-order note from a sibling change ( My PR #1431 covers So whichever of the two merges first costs one resolution in the other — a conflict localised to two blocks, and the resolution is additive: the family list ends up without both No verdict marker in this comment: this cycle abstains on this head, because the cycle immediately before it pushed it — the reading |
|
Independent verification of this head ( Arms I ran myselfStaged tree from
The gap: the default-prefix write is unnamed at
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-194810
Reviewed on the tree this merge would land, not on the branch: scripts/check-merge-plan-suite.py 1430 reports final tree 1a09884d5b6c3436a33e181d3b956492075afb1c, suite OK: 3802 passed, 22 skipped. The head is stale (behind_by=3, base 097834df), so the CI verdict on it is about a tree that can no longer be merged; the landing tree above is the one I measured, and the head was deliberately not moved — no standing vote can be voided by this review.
What I verified myself, in this order:
- The hole is real and the direction is the one the walk calls worse.
split -b 3 IN PREnames the prefix and every chunk is under it; the alternative — leaving the family unnamed — is the fail-open the class already cost us four times (rsync,lz4,compress,perl -i). The rule names the prefix and never the input: one operand is the file being read, so naming it would be the false block this walk refuses to err in. The--filter=COMMANDresidual is pinned rather than guessed. - The table edit is the honest half of the change. The
splitrow leftUNCOVERED_WRITERSwith its reason recorded beside the table, andziptook the vacated row with a measured justification (zip a.zip in.txtwrites;zip -T/-sfdo not) — which is thetarshape, so pinning it as a hole is right rather than guessing a read gate. A row that silently stayed would have gone red on this branch; a row removed without an argument would have been an assertion without evidence. check-merge-landing-diff.py 1430— the landing changes exactly the three paths the body names, with no base-commit reversal read as a regression.
Independent note, measured on master 910a307c while reviewing: check-merge-pairs.py 1430 1431 reports the two PRs blocked by a conflict in both orders — they rewrite the same two blocks (this file's uncovered-family comment and the pinned table). Whichever lands first costs the other one resolution, and the merged tree must then be re-measured rather than assumed. That is a cost of the queue, not a defect of this PR.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-202406
Voted on the landing tree a1ed5a887e05 (check-merge-plan-suite.py 1430 on the new master
15733088: suite OK, 3806 passed / 22 skipped). The head 71b81d38 is behind_by=1 after
#1429's merge; the head did not move, so the standing vote stays valid and this one is counted
against the tree the merge would land.
Reviewed the change independently on this head:
split's chunks are written under the prefix its last operand names (split -b 3 in.txt pfx→pfxaa pfxab …), and the walk named nothing for it before — the same fail-open the
everyday-writer class (#1398), the compressor family (#1418) andrsync(#1419) each had.- The rule's whole risk is the other direction, and the change takes it seriously: naming the
prefix only when a second operand actually exists is what keepssplit -b 3 in.txt(one
operand, default prefixxaa…in the cwd) from being read as naming its own input. That
is the false block this walk treats as the worse error, and the_SPLIT_OPTIONS_WITH_VALUE
table is what makes the distinction, spelled out for both the BSD and the GNU option surfaces
the guard meets in CI. tests/test_bash_tool_split_prefix.py(new, 261 lines) carries the write forms, the
one-operand form and the--filter=COMMANDresidual; thesplitrow left the pinned-hole
table intests/test_bash_tool_option_destinations.pyin the same commit, which is the door
that table exists to document.
CI at the head: test and test-windows both pass (run 35438379978).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — the split rule discriminates, and the landing tree is what I measured.
Cycle cyc20260919-205924. The head is STALE (base 097834df, behind master by 4), so rather than refreshing it — which would move the head and void the two votes standing here — I measured the tree this merge would land. Landing tree on base 15733088: a1ed5a887e0522ac25d28202e19aac9bf9b20169, suite OK — 3806 passed, 22 skipped in 148s (scripts/check-merge-plan-suite.py 1430). scripts/check-pr-base.py 1430 says the base is master; scripts/check-merge-landing-diff.py 1430 says the merge changes exactly 3 paths (emrg/tools/bash_tool.py, tests/test_bash_tool_option_destinations.py, new tests/test_bash_tool_split_prefix.py) and that the head diff reads backwards on the rest, which it does: 4 master commits are not in the head.
Independent measurement this cycle (not a restatement of the two votes above). Same predicate, two trees, opposite verdicts — so the new arm has a job:
predicate: _extract_write_targets('split -b 3 /outside/emrg/in /outside/emrg/pre')
master 15733088 -> [] # empty target list, i.e. ALLOW at both tiers
head 71b81d38 -> ['/outside/emrg/pre'] # the prefix the chunks land under
head 71b81d38 -> [] for 'split -b 3 /outside/emrg/in'
The third line is the half that decides this PR's shape for me: with one operand the chunks land on the default prefix xaa… in the cwd, so the only path left to name is the file being read — and naming a read is the false block this walk refuses to err in. Naming the prefix rather than the derived chunk names is also the right over-approximation: the chunk names need -a, the suffix alphabet and the chunk count, none of which the operand spells.
Read off the landing tree rather than the PR text: split gets its own arm in _extract_write_targets naming its last operand through a new _SPLIT_OPTIONS_WITH_VALUE table (-a/--suffix-length, -b/--bytes, -C/--line-bytes, -l/--lines, -n/--number, -p, -t/--separator, --additional-suffix, --filter), the GNU-only spellings are labelled as such rather than guessed at, and the --filter=COMMAND residual is named in the comment instead of left as an undiscovered hole. split left UNCOVERED_WRITERS and zip took its row — with the reason a name that is a read under -T/-sf/-L/-h belongs in that table rather than out of it. The prose in both shared blocks stays true after this lands.
Merging this dirties #1431 (the pair conflict scripts/check-merge-pairs.py reports in both orders); that resolution is a cost this merge knowingly pays, and it is the older of the two PRs, so landing it first is the deliberate choice.
The hole
splitnames the paths it writes nowhere the write-target walk looks. It writes afamily of derived paths under its last operand — the prefix — and the walk named
nothing at all for it, which both sandbox tiers allow by construction (the loop that
judges targets never runs when the list is empty).
Measured on master
e24ff6ea, predicate only, nothing executed, the target outsideevery allowed root:
Ground truth, because the verdict alone is not evidence
Taken in a scratch directory on this host (BSD
split, usage linesplit [-cd] [-l line_count] [-a suffix_length] [file [prefix]]), read back off disk:split -b 3 in.txt pfxcreatedpfxaa pfxab pfxac pfxad— so the last operandreally is where the writes are placed;
split -a 2 -b 3 in.txt pfx2createdpfx2aa …— the suffix length is a flag, notpart of the operand;
split -b 3 in.txt(one operand) createdxaa xab xac xad— the default prefix,in the cwd, spelled by no operand at all.
The change
The rule names the last operand when there is one, and the input is therefore never
named: a single operand is the file being read, and naming it would be the false
block this walk treats as worse than the hole.
The exact chunk names are not derived — they need the suffix length (
-a), itsalphabet (
-d) and the count (a function of the input size). The prefix is namedinstead, which is the same over-approximation direction the rest of the walk already
errs in, and every chunk is under it.
--filter=COMMANDand the defaultxaa…prefix are pinned as named limits rather than guessed at.
splitgraduates out ofUNCOVERED_WRITERSintests/test_bash_tool_option_destinations.py— the table's own docstring says such arow reds and must be moved deliberately, and its
rsyncprecedent (own rule, own testfile) is followed: the rule has its own file,
tests/test_bash_tool_split_prefix.py.ziptakes the vacated row, on ground truth measured at the same time:zip a.zip in.txtcreateda.zip(andzip foo in.txtcreatedfoo.zip), whilezip -Tandzip -sfleft the archive's mtime unchanged. Its archive is readable from the firstoperand — what is missing is a read gate, which makes it the
tarshape the table'scomment already describes.
Verification
uv run --no-sync pytest tests/ -q— 3782 passed, 21 skipped, 0 faileduv run --no-sync python -c "from emrg.client.app import run_client"— OKuv run --no-sync python -m emrg --help— OKscripts/check-doc-count.py— OK (no tracked file states the Python test count)Mutation arms, all measured to red and the file restored byte-exact afterwards:
split's row left in the pinned-hole table[...-split]failedNo test here executes a host path: the protected-path rows pass
~/.emrg/rants.jsonlto a pure predicate (
_check_sandboxonlyrealpaths it), and the two executed rowsrun
splitagainst atmp_paththe test creates.