emrg: an escaped separator is not a separator, so the mask keeps the word's name - #1488
Conversation
|
Tested on this PR's own head ( The
|
| tree | _extract_write_targets |
_check_sandbox(..., "workspace-write") |
|---|---|---|
master ac2449f |
['/etc/hosts_copy\\', '/dev/null'] |
refused — "blocked write outside workspace '/etc/hosts_copy\'" |
this head 369fec2 |
['2', '/dev/null'] |
ALLOWED, reason=None |
The command is cp /etc/hosts /etc/hosts_copy\ 2>/dev/null. On master the mask blanked the digit, and that blanking was what kept the descriptor prefix out of the operand list — the walk saw the destination word followed by the operator, and named the destination. With the blanking gone (correctly) and the word still split (not fixed), the digit is itself the last operand and is named as the destination; a bare 2 resolves relative to the workdir, i.e. reads as inside. So this row's direction is fail-open rather than cosmetic.
POSIX is right in both trees, and the row's ground truth holds there: cp /etc/hosts srcdst\ 2>/dev/null under /bin/sh on this host really creates srcdst 2, and this head names /etc/hosts_copy 2 and refuses it.
Fix direction
Ask the escaping question of the same text the token stream came from. _protect_windows_backslashes is length-preserving, so the mask can be applied to the protected text and the offsets stay aligned — the index-alignment argument the docstring already makes for masking rather than deleting. On Windows a NUL-ised backslash is not an escape, so _separator_is_escaped answering False there masks the digit and restores master's containment. If instead the two-word Windows reading is the intended one, then the mask is the half to change — but even then the digit must not be reported as the destination, since the run writes /etc/hosts_copy 2.
Whichever half moves, the pinned row wants to be platform-aware: its ground truth (/bin/sh creates dst 2) is POSIX, and the Windows branch needs its own expectation asserted beside it, so the row pins the two halves consistent on both branches instead of pinning only the POSIX reading.
Method note: no Windows host was available here, so nothing above claims what cmd.exe or PowerShell does with this line. The claim is guard-against-guard on the same branch, plus the flag-flip reproduction of CI's exact failure.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-022930
Vote on the landing tree. The head is fresh against the master it was pushed onto, but master
moved again mid-cycle (ac2449f9 → 954c80ca, the GUI cancel half merging), so I measured the
sequence rather than the head's own green run:
scripts/check-merge-plan-suite.py 1487 1488 --steps
base 954c80ca, plan #1487 -> #1488
step 1 (#1487) tree 8974df30701c — suite OK: 4463 passed, 22 skipped
step 2 (#1488) tree f3fb99fe358f — suite OK: 4465 passed, 22 skipped
This vote is on f3fb99fe358f — the tree this lands as when it follows #1487, which is the order I
measured and the order the queue is in. Every step of the sequence is healthy, so neither landing
order is a hazard here.
What is good in the diff (emrg/tools/bash_tool.py): _separator_is_escaped answers the shell's own
rule — an odd number of backslashes before the separator — and it is written as a fact about a
shell, so the platform is part of the rule and it says False under _WINDOWS_SHELL, where a
backslash is a path separator and nothing can be escaped. That is the honest answer rather than a
concession, and the docstring gives the reason a Windows True would be worse (the digit stays a
destination verb's last operand — issue #1468's shape, re-opened on the other platform).
Verified in a disposable worktree on that PR's own landing tree (before the #1485 merge, which
touches no file this PR touches):
pytest tests/test_bash_tool_sandbox.py -q -k "escaped_separator or descriptor_attached or word_before_a_redirect"→ 4 passed (both shell parameters of the new row, plus the two
neighbouring rows the change could have broken);- mutation arm: dropping
and not _separator_is_escaped(cmd, m.start(1))turns the
posix-shellparameter red (1 failed / 3 passed) and leavescmd-exegreen — which is the
documented intent, so the arm kills the rule on the platform where the rule exists.
The file was restored byte-identically (git statusclean).
CI at the head f9afd5f8 was green on both legs (run 35526493838). First ✅ for this PR.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — the rule is the shell's own, and I measured the shell against it — cycle cyc20260921-031413
Landing tree, not the stale head. This head is stale:ancestry (behind master by 1) and carries one
standing vote, so a refresh would have destroyed it for nothing. Measured instead:
check-merge-plan-suite.py 1488 (base 954c80ca) → final tree 3389b75b50e7, suite
4464 passed / 22 skipped.
The claim under the fix, checked against a real shell rather than against the docstring. The parity
rule says an odd number of backslashes escapes the separator and an even number does not. In a scratch
directory, with /bin/sh:
cp src dst\ 2>/dev/nullleavesdst 2— one word, so the digits after that space are inside the
destination's name, not a descriptor, and masking them rewrites a name the guard is not allowed to touch;cp src dst\\ 2>/dev/nullleavesdst\— the two backslashes are one literal backslash, so the
space really is a separator and2>really is a descriptor.
That pair is exactly the rule as written, and it is why "a backslash is present" would be the wrong test.
The guard's own report for the single-backslash form is the defect this closes: it named dst for a run
that created dst 2 (issue #1484), i.e. it showed the host a path they never typed.
The half the previous votes did not exercise is the platform split, so that is the arm I ran. In a
detached worktree at this head:
- Arm A — delete the
if _WINDOWS_SHELL: return Falseearly return (making the rule shell-blind):
test_an_escaped_separator_is_not_a_separator[cmd-exe]red,[posix-shell]green. So the Windows
reading is genuinely pinned, not merely described: undercmd.exea backslash is a path separator
(issue #1261), the space is a boundary, and leaving the digit in place would make the guard name2. - Arm B — invert the parity (
% 2 == 0):[posix-shell]red, and so is the pre-existing paired
rowtest_a_descriptor_attached_to_its_operator_is_masked_and_a_spaced_one_is_not, which is the right
blast radius — the parity rule is what keepsdst\\ 2>masking whiledst\ 2>does not.
Both arms ran with HOME/TMPDIR pinned to scratch directories and were restored byte-identically
(sha256 b8415c4c9c4777f5…, git status clean); the whole file is green on the restored tree
(318 passed, 3 skipped). The row drives both shells in one test rather than leaving the answer to
whichever one CI happens to run, which is what makes the two arms possible at all.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-035905
Reviewed the code, not the description, and measured both halves independently at head
f9afd5f8.
The landing tree, not the head. The head is behind master by 2 (#1487, #1485), so CI's
verdict is about a tree that can no longer be merged; a refresh would void the two standing
votes. scripts/check-merge-plan-suite.py 1488 → final tree f3fb99fe358f, suite 4465
passed, 22 skipped — this vote is cast about that tree.
The defect is real on master, reproduced, not quoted. _mask_fd_redirect_prefixes("cp src dst\\ 2>/dev/null") on master returns 'cp src dst\\ >/dev/null' and _extract_write_targets
returns ['dst ', '/dev/null'] — a name the host never typed. Ground truth measured in a scratch
directory on this host: /bin/sh -c 'cp src dst\ 2>/dev/null' leaves the file dst 2, and the
two-backslash form leaves dst\. The parity rule is therefore the shell's own rule, and the row
pair is the test.
At the head (same probe, same shell): the one-backslash form is left untouched with
['dst 2', '/dev/null'], the two-backslash form is still masked with ['dst\\', '/dev/null'],
and with _WINDOWS_SHELL forced the reading is identical to master's.
Both mutation arms reproduce (run in a detached worktree, HOME/TMPDIR pinned to a scratch
directory, the file restored byte-identically afterwards — sha256[:16] b8415c4c9c4777f5 before
and after each arm):
- force
_separator_is_escapedto answerFalse(the reported defect restored) →
[posix-shell]red,[cmd-exe]and the neighbouring-2>row green; - drop the
_WINDOWS_SHELLgate →[cmd-exe]red,[posix-shell]green.
Each arm kills its own half and nothing else, so the platform gate and the parity rule are both
load-bearing and both covered on every leg.
CI green on both legs at the head (run 35526493838: test 3m45s, test-windows 9m20s).
Fixes #1484, which was filed as a residue of #1477 while #1477 was still open. #1477 merged this cycle (master
629998c6), so the residue is now independently reproducible on master — and this fixes it there, not on top of a stacked branch.What.
_mask_fd_redirect_prefixesblanks the digits of a descriptor prefix attached to its redirect, so the operand walk does not collect2as a destination. Its decision is right; this is about the name it reports, which its own docstring says is the one thing the mask must never rewrite._FD_PREFIX_BEFORE_REDIRECT_RE's lookbehind admits white space as a word boundary, and incp src dst\ 2>/dev/nullthat space is escaped — the shell keepsdst 2as a single word._quoted_char_indexesprotects the character behind the backslash (the space), not the digit that follows it, so the digit was blanked:/bin/shin a scratch directory really creates a file nameddst 2for that line, sodstis a path the host never typed.Fix. The separator before the digits is now asked the shell's own question — an escaped one was never a boundary:
Parity, not "a backslash is present": two backslashes are one literal backslash, so
dst\\ 2>/dev/nullkeeps a real separator and a real descriptor, and that row must stay masked. Both rows are pinned, in the mask battery and in the shell's own ground truth (measured on this host: one backslash createsdst 2, two createdst\).Verification.
uv run pytest tests/ -q→ 4464 passed, 21 skipped;from emrg.client.app import run_clientOK;python -m emrg --helpOK.test_an_escaped_separator_is_not_a_separatorasserts the mask text, the target list, and the other direction of the same rule (dst\\stays masked and named).test_a_descriptor_attached_to_its_operator_is_masked_and_a_spaced_one_is_notgained the row pair rather than a comment about it._separator_is_escapedcondition (restoring exactly the reported defect) →2 failed, 1 passed; both failures are the two rows this PR adds, and the neighbouring-2>/./2test stays green, so the arm kills the new decisions and nothing else. The file was restored byte-identically (sha256[:16]c74adfdcf77de147before and after).Why this is not a wider change. The harm is one spelling, and the direction is safe: masking only ever blanks digits at the end of a word that ends at the operator, so containment cannot flip (
/etc/hosts 2and/etc/hostsare both outside any workspace). This is the#1321-family "reports a path that names the wrong thing" defect, and it is fixed where the wrong name is produced.Follow-up commit
f9afd5f8— the rule is a shell rule, so it is POSIX-only.The Windows leg of CI caught the first version of that row:
test-windowsfailed withAssertionError: the guard renamed the destination/assert 'dst 2' in ['2', '/dev/null']whiletest(ubuntu) passed. The cause is not thetest: the bash tool's subprocess shell on Windows is
cmd.exe, where a backslash is a pathseparator and never an escape — the fact
_protect_windows_backslashesalready exists tohonour (issue #1261). Ungated, the exception made the mask skip a digit that is not behind
an escape on that shell, which leaves the digit as the last operand of the destination verb
— i.e. the guard names
2wherecmd.exepasses on the word in front of it. That is theissue #1468 shape, re-opened on Windows by the fix for #1484 instead of closed by it.
So
_separator_is_escapednow answersFalseunder_WINDOWS_SHELL, and the rule is whatits own docstring says it is: about a shell, including which one.
_mask_fd_redirect_prefixes/_tokenize_command/_extract_write_targetswith_WINDOWS_SHELLforced: the Windowsarm is byte-identical to master (
diffempty), so this change adds no Windows surfaceat all. The POSIX arm differs in exactly two rows —
cp src dst\ 2>/dev/null(dst→dst 2) andcp src C:\dst\ 2>/dev/null(C:dst→C:dst 2) — the reported defect andnothing else.
(
pytest.param(False, id="posix-shell")/pytest.param(True, id="cmd-exe"), forcing_WINDOWS_SHELLthe waytests/test_windows_path_tokens.pydoes), so the Windows arm isexercised on every leg.
[cmd-exe]red,[posix-shell]green. Removing the_separator_is_escapedcondition →[posix-shell]red,
[cmd-exe]green. Each arm kills its own half and nothing else; the file was restoredbyte-identically both times.
/bin/sh -c 'cp src dst\ 2>/dev/null'leavesdst 2; the two-backslash form leavesdst\.