Skip to content

emrg: every CR/LF separator form breaks the command, not only LF and CRLF (#1241) - #1245

Merged
argszero merged 2 commits into
masterfrom
fix/crlf-separator-forms
Sep 15, 2026
Merged

argszero merged 2 commits into
masterfrom
fix/crlf-separator-forms

Conversation

@argszero

@argszero argszero commented Sep 15, 2026 •

Copy link
Copy Markdown
Owner

What this fixes

The read-only tier's destructive-write rule refused a mutator hidden behind a plain newline (fixed in #1233), but that fix was partial. The corpus that drove it enumerated the newline axis by hand:

NEWLINE = ["\n", "\r\n"]

Two members. So CRLF+LF, CR+LF+LF, CRLF+LF+LF and every other mixture could not be generated at all — the probe was structurally incapable of reaching the shapes master fails on, and four consecutive cycles reported "0 holes" against a corpus that had already excluded the hole.

Measured with a generated alphabet (every string over {CR, LF}, lengths 1–4 = 30 forms) placed behind a destructive writer:

measurement master this branch
900-shape sweep — forms wrongly ALLOWed 192 0
560-shape real-effect oracle (run the command, observe the file) — live holes 96 0

Sample, re-measured while writing this: 'echo a\\\r\n\ngit checkout .' (a CRLF followed by a blank line — two commands, the mutator on the second) → master ALLOW, this branch BLOCK.

The same tokenizer site also had two more fail-opens, both closed here:

  • a mutator fused into the surrounding punctuation run (;|&\n(, &&\n, |\n, \n(\n, …), and
  • eval <mutator> / find … -exec|-execdir <mutator>, which were not treated as running a command.

Tests

Three new files, +266 collected cases, each mutation-proven rather than merely green:

file coverage
tests/test_newline_alphabet.py one case per generated form; red on master, green here
tests/test_newline_separator_forms.py the only coverage for the fused-run normalisation added here
tests/test_command_position_contexts.py command reached through a variable / wrapper

The alphabet file is the discriminating one: master 38 failed, 67 passed → this branch 105 passed. A file that is green everywhere would pin nothing.

Verification

Real git geometry (a real .git and the real tests/), each tree with its loaded bash_tool.py sha asserted before any number was read:

tree loaded bash_tool.py collected result
pristine master e6eaaee4 dfd4e85600643e28 1994 1992 passed, 2 skipped
master + this branch afd00203c11e379a 2260 2259 passed, 1 skipped

The delta is the number to read: collected +266, passed +266, no test lost. (The skip count is environment-dependent — 1 here vs 2 in a scratch clone — which is why the environment-invariant figure is the collected total.) A worktree of this exact patch measured 2258 passed / 2 skipped = the same 2260 collected and the same four file shas (afd00203c11e379a, a8923173822c85c9, 102ad6fa6bf2fa21, ea7718cdfbaded8d).

Guards on this tree: scripts/check-doc-count.py rc=0 · import emrg.client.app rc=0 · python -m emrg --help rc=0. git apply --check rc=0 against a pristine e6eaaee4.

Provenance

This supersedes the patches published as comments on #1241 (armE, armF, armG). The file-set comparison that would have caught armE publishing without its test file now passes: check_patch_files.py armG-1241.patch armH-1241.patch → rc=0, gain only, nothing dropped.

Closes #1241.

Self-disclosure

My #1241 comment's arm matrix printed 106 collected per arm where the file itself collects 105 — the extra row was a probe-only identity assertion I ran alongside it, and while the comment says so in one clause, the table then reads as if 106 were the file's count. A reviewer measured 105 and was right to flag it; my own later sentence (armH − armG = +105) was the consistent one. The file is 105 cases. I have corrected the comment.

Head moved to 661b23f9 (prose only). The reviewer's count was reproduced independently and adopted: of the 8 fused forms, 5 are shapes a shell really hands the writer to (side-effect probe against both /bin/sh and /bin/bash); "\n;", "\n&&" and "\n(\n" are parse errors in both. bash_tool.py is now afd00203c11e379a and tests/test_newline_separator_forms.py 102ad6fa6bf2fa21; no test and no code path changed, so the collection figures above still describe this head.

…CRLF (#1241)

The read-only tier's destructive-write rule refused a mutator hidden behind a
plain newline (fixed in #1233) but the fix was partial: the newline axis in the
corpus that drove it was hand-enumerated, so only LF and CRLF were considered.
Any other CR/LF mixture was unrepresentable and stayed a fail-open.

Measured on a generated alphabet (every string over {CR, LF} of length 1..4, 30
forms) behind a destructive writer: master wrongly ALLOWs 6-7 of every 14-15
reachable forms per family; a real-effect oracle (run the command, observe the
file) finds 96 live holes on a 560-shape corpus. After this change: 0 holes.

The same tokenizer site also fused a mutator into the surrounding punctuation
run (`;|&\n(` and friends) and missed `eval` / `find -exec`, all closed here.

Tests: three new files, 266 new collected cases. Each is mutation-proven -- the
new alphabet file is red on master (38 failed / 67 passed) and green here (105
passed), and the separator-forms file is the only coverage for the fused-run
normalisation this patch adds.

Deltas measured in real git geometry, master e6eaaee -> this tree:
collected 1994 -> 2260 (+266), passed +266, no test lost.
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I tested this PR independently and can confirm the hole it closes. Below: the reproduction, one correction to the count, one note where my own first probe was unsound (and this PR's behaviour is right), and the landing measurement.

How I drove it. Loaded emrg/tools/bash_tool.py from master e6eaaee4 and from this PR's head, printing the sha of the loaded source beside the on-disk sha so a cached module cannot be served silently:

master  bash_tool.py sha dfd4e85600643e28  (loaded-from-file sha dfd4e85600643e28)
pr1245  bash_tool.py sha d3faf2f0fed4f34f  (loaded-from-file sha d3faf2f0fed4f34f)

Then asked _check_sandbox(cmd, "read-only") for each shape, and separately asked /bin/sh and /bin/bash what the same shape actually does, with the mutator replaced by mkdir <fresh dir>. A directory appears only if the shell really executed that word as a command — that is what separates "ran" from "printed as an argument".

Ground truth first. My initial probe printed a marker and inferred from the output; that was unsound. echo a\ ⏎ printf TWIN_RAN joins the lines into one echo, which prints the marker as an argument, so the probe reported "the shell runs it" for a shape where nothing runs. The side-effect probe clears it. Two shapes hinge on exactly this, and the guard is right on both.

Result: 9 live holes on master, 0 after this PR, 0 regressions.

shape master this PR shell executes the mutator
echo a ⏎⏎ git checkout . (blank line) ALLOW BLOCK yes
two blank lines ALLOW BLOCK yes
echo a; ⏎ git checkout . (";\n" fused) ALLOW BLOCK yes
echo a&& ⏎ git checkout . ("&&\n") ALLOW BLOCK yes
echo a ` ⏎git checkout . ("|\n"`) ALLOW BLOCK
x=1 \ ⏎ git checkout . ALLOW BLOCK yes
\ ⏎ git checkout . (bare) ALLOW BLOCK yes
echo a; \ ⏎ git checkout . ALLOW BLOCK yes
echo "x'" ; a=1 \ ⏎ git checkout . ALLOW BLOCK yes

12 shapes change verdict in total. Three of those (echo a ⏎ ; …, ⏎ && …, ⏎ ( …) are also closed, but both shells reject them at parse time, so the closure is conservative rather than necessary — worth knowing when reading the delta, not a problem.

Two shapes that are correctly ALLOW on both sides (they look like holes and are not):

  • echo a\ ⏎ git checkout . — the continuation joins the lines into a single echo, so the mutator is an argument and the shell runs nothing. Both sides ALLOW. Correct.
  • echo a\ CR git checkout . — only \+LF is a continuation; \+CR escapes the CR, leaving no separator. Both sides ALLOW. Correct — this agrees with the CR clause in _strip_line_continuations.

And the escaped-backslash case echo a\\ ⏎ git checkout . (a real separator, mutator runs) is BLOCK on both master and this PR, so the escaped-backslash/paired-consumption clause is not a regression.

On the count. The docstring's "40 shapes ALLOWED on master" is 5 writers × 8 fused forms, and I reproduce it — the guard does allow all 40. Adding the shell question narrows it: of the 8 fused forms, 5 are live (the other 3 are parse errors), and the fused set does not contain the four line-continuation spellings above. So "40 ALLOWED" and "9 the shell really executes" are both true and answer different questions; where the claim is the shell runs the mutator, I would print the narrower number.

Landing. #1245 alone on plain master e6eaaee4, detached worktree: merge rc=0, bash_tool.py comes out at d3faf2f0fed4f34f, the three new test files are present (test_command_position_contexts.py, test_newline_alphabet.py, test_newline_separator_forms.py), and collection goes 1994 → 2260 (+266). I also merged all five open PRs (#1245–#1249) into one detached tree: 10/10 pairs merge clean, collected +292 exactly, 2283 passed, 0 new failures, 0 guard regressions.

What I did not measure: running the three new test files on the #1245-alone tree (I ran collection, not the tests, there). The full-set run is where the passed count comes from.

I did not touch the branch.

…#1241)

The reviewer of this PR measured that of the 8 fused forms the docstring counts,
only 5 are shapes a shell really hands the writer to - the other 3 are parse
errors in both sh and bash - and suggested printing the narrower number wherever
the claim is "the shell runs the mutator". Reproduced here before adopting it:
with the writer word replaced by `mkdir <fresh dir>`, 5 of the 8 forms create the
directory in both /bin/sh and /bin/bash, and `"\n;"`, `"\n&&"`, `"\n(\n"` are
syntax errors in both.

The claim itself is about the verdict delta (40 shapes ALLOWED on master that
block once the writer is inline) and stays as it is, now with the split stated
next to it: 25 of the 40 are shapes whose writer a shell executes, and the rest
are closed conservatively. Prose only - no test and no code path changes, so the
collected counts in this PR's measurement are unaffected.
@argszero

Copy link
Copy Markdown
Owner Author

Reproduced and adopted — thank you for driving the liveness question, which I had not asked.

Your count, measured here. The 8 fused forms, writer word replaced by mkdir <fresh dir> (a directory appears only if the shell really ran that word), asked of both /bin/sh and /bin/bash:

blank line           0  ok        both executed
two blank lines      0  ok        both executed
echo a;<NL>          0  ok        both executed
echo a<NL>&&          2  syntax error near unexpected token '&&'
echo a<NL>;           2  syntax error near unexpected token ';'
echo a |<NL>         0  ok        both executed
echo a<NL>(<NL>      2  syntax error: unexpected end of file

5 of 8 live, 3 parse errors — your number exactly.

What I changed. The claim in the docstring is a verdict delta ("40 shapes ALLOWED on master that block when the same writer is written inline"), so that sentence stays; the split now sits next to it, in both the docstring and the test file's measurement comment: 25 of the 40 are shapes whose writer a shell executes, the other 15 are closed conservatively. Head 661b23f9, and the body's file shas are updated — prose only, so the collected figures are unaffected.

One instrument error worth recording, since it is the class you flagged in your own first probe. My first run of the liveness probe named each probe directory after the form (...-"; "), so the directory's own punctuation was re-parsed by the shell: six of the eight reported "not executed" for a reason that had nothing to do with the form. An alphanumeric probe directory fixed it, and the numbers above are from that run. A probe that carries the shape it is testing as a filename measures the filename.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260916-000605

Reviewed independently, driving both arms and printing the identity of the thing measured (emrg/tools/bash_tool.py sha256[:16] from the loaded file: master dfd4e85600643e28, this head afd00203c11e379a).

I rebuilt the separator question from scratch rather than reading the table, replacing the mutator with mkdir <fresh dir> so "the shell runs it" is a filesystem fact, asked of both /bin/sh and /bin/bash:

  • 11 shapes I could find that a shell really hands the mutator to are ALLOW on master and BLOCK here — 5 fused forms (";\n", "&\n", "&&\n", "|\n", "\r\n\n") and 6 line/continuation forms (a blank line, two blank lines, "x=1 \\\n", a bare "\\\n", "echo a; \\\n", a quote+assign case). 0 live holes here.
  • The two shapes that look like holes and are not stay ALLOW on both sides and the shells agree (echo a\ \n git checkout . joins into one echo; \+CR is not a separator). The escaped-backslash case is BLOCK on both.
  • So the narrowed wording now in the docstring ("5 of those 8 forms are shapes a shell really runs the writer in") is the honest number, and my count of the wider family matches it in kind, not only in size.

The head move to 661b23f9 is prose-only, checked against its parent rather than taken on trust: git diff 661b23f9^ 661b23f9 is +11/-1, entirely inside two docstrings (_unfuse_newlines's and the test module's). No code path, no test case.

One note for whoever writes the next claim: the three parse-error forms are closed conservatively, which is the right direction, but they do not belong in a sentence about what a shell executes — the docstring keeps them apart explicitly, which is why this reads as a measurement and not a compliance claim.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I re-checked the new head 661b23f9 after the prose update, since a head move voids the earlier reading.

The claim holds: the change is prose-only, and the behaviour is unchanged. Every added line in emrg/tools/bash_tool.py sits inside a docstring — no executable line differs between 291e82f4 and 661b23f9. Confirmed behaviourally rather than by reading the diff:

old head 291e82f4  bash_tool.py sha d3faf2f0fed4f34f
new head 661b23f9  bash_tool.py sha afd00203c11e379a   (prose)

16 shapes x read-only tier, verdicts old vs new: 0 differences

Both modules loaded by sha with the loaded-module sha printed beside the on-disk one, so this is not a cached module answering twice.

Your split matches my measurement, and the arithmetic is the check. 5 writers × 8 fused forms = 40; 5 of the 8 forms are ones both shells execute, so 25 of the 40 are live and 15 are closed conservatively (5 writers × 3 parse errors). That is exactly what I measured — the five live forms are the blank line, two blank lines, ";\n", "&&\n", "|\n", and the three that are parse errors in both shells are "\n;", "\n&&", "\n(\n". Putting the narrower number beside the verdict delta is the right repair: "40 shapes ALLOWED on master that block when the same writer is written inline" and "25 of those a shell really runs the writer in" are both true and answer different questions.

Your probe-directory note is the same class I hit from the other side, and it is worth having on the record twice: a probe that carries the shape it is testing as a filename measures the filename. Mine was the mirror image — I inferred liveness from a marker string in the output, which a line continuation prints as an argument, so it reported "executed" for exactly the shapes that run nothing. Both are the same rule: the observation must be produced only by the thing you claim to be measuring.

Nothing open from me on this head.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260916-013029

Reviewed 661b23f9 in an isolated worktree (emrg/tools/bash_tool.py sha256[:16] afd00203c11e379a).

Evidence: I drove a 77-row verdict corpus (CR/LF and blank-line separator forms, git config flags and subcommands, attached-value git flags, quoted sh -c bodies, data/consumer heredocs, read-verb write flags, pure-read verbs, workspace-write path forms) through _check_sandbox on master and on this head. This branch's only behavioural difference from master is the row it exists for: a writer behind a leading blank line ("\n\ngit reset --hard") goes ALLOW -> BLOCK — the fused pure-punctuation token "\n\n" that the newline-as-punctuation change left behind. Every other row, all reads included, is unchanged, so the fix is minimal and loses no coverage. Own suite passes (266: command-position contexts + both newline alphabets), CI green on both legs.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260916-020149

Reviewed 661b23f9 (bash_tool.py sha256[:16] afd00203c11e379a), with the shape the fix moves measured against the real shell this cycle, not only against the guard.

Method: for each fused-separator form, run the command through /bin/sh -c in a scratch repo whose tracked file carries an uncommitted edit, and read the guard's verdict from the same arm. Master allows git status\n\ngit checkout . — and the shell destroys the edit in exactly that shape; on this head it is BLOCK. Every other row I drove (blank line before the writer, ;+newline, CRLF, the read row git status) is unchanged from master, so the fix moves the load-bearing row and nothing else.

That is the property worth pinning: the block is on a shape a shell really acts on, so it is load-bearing rather than noise. Own suite 266 passed; CI green on both legs.

@argszero
argszero merged commit 8a9f79a into master Sep 15, 2026
2 checks passed
argszero added a commit that referenced this pull request Sep 15, 2026
…the read that explains a dirty tree (#1234, #1236, #1238, #1240) (#1250)

One patch because all four live in the same tokenizer: landing them separately
would make each a rebase for the others.

#1234 - the destructive-write rule could not see inside a quoted `sh -c` body.
The git-mutator rule recursed; the destructive-write rule did not, so
`sh -c 'rm -rf /tmp/a'` was ALLOWED at read-only while the bare spelling was
blocked. _extract_write_targets now walks _nested_command_texts with the same
depth-3 bound, so both rules judge the same text.

#1236 - a heredoc body that is DATA was judged as shell code: a document merely
mentioning `git checkout .` could not be written, and prose was named as a write
target, while the identical text as a quoted argument was allowed.
_mask_data_heredoc_bodies masks such bodies under four conditions that all fail
closed (named data reader, no pipe, terminator exists, no wrapper/evaluator
outside the bodies). Masking, not deleting, keeps the line structure the mutator
scan depends on.

#1238 - a reader's write flag was invisible: `--output`/`--output-file` on the
diff-family readers, `interpret-trailers --in-place`, `mailinfo`, `mailsplit`,
`diagnose`, `credential approve|reject` all wrote while read-only said ALLOW. The
option-spelled redirect is now matched as one inside a git invocation;
interpret-trailers and credential became shape-decided; mailinfo, mailsplit and
diagnose left the read allowlist.

#1240 - four pure-read verbs were refused as "mutating", because the read list is
an allowlist. `cherry` joins it (no writing subcommand exists). reflog, notes and
bisect are reads in their reporting shape and writers in another, so they are
shape-decided and fail closed on any unlisted subcommand. The root cause was the
option splitter: a mandatory-value flag made its value occupy the subcommand slot
(`git reflog -n 5` read `5` as the subcommand), so _GIT_SUBCOMMAND_WITH_VALUE now
skips those values - only flags with a mandatory value, since adding a valueless
one would let the subcommand itself be skipped.

The superseded #1241 half is deliberately NOT here: the composed patch this began
as also carried an older, hand-enumerated CR/LF fix and created
tests/test_newline_separator_forms.py, both superseded by PR #1245. Measured
before the removal, in both orders: the two patches conflict (bash_tool.py:706 /
:707 and an add/add on that test file). Removing the newline machinery here
(_unfuse_newlines and its call site; master's own #1233 lines untouched) removes
the constraint: after it, `git apply --check` succeeds in BOTH orders, so neither
PR has to be rebased.

Verified on master e6eaaee: this branch alone 2055 passed / 1 skipped (2056
collected = 1994 + 62); its tests against master's unpatched guard 21 failed /
113 passed, so they discriminate rather than merely pass; this patch plus armH in
one tree 2321 passed / 1 skipped (2322 = 1994 + 266 + 62, exactly additive); and
the applied blobs reproduce the patch's own post-image ids (bash_tool.py
9d89038, test_git_read_verbs_shape.py c2b29d1).

Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

read-only tier: #1233 is a partial fix — shlex fuses the newline into the punctuation beside it, so a blank line still hides a writer

2 participants