emrg: a word a spaced option ate is not read again as an option - #1482
Conversation
The write-target walk has two readers that scan a verb's argument list on their own and re-read a token another option already consumed. Both name a path the run never writes, which is the expensive direction: a name outside the allowed roots refuses a command that changes no byte (issue #1464). `patch`'s `-d` reader: `patch -o -d <dir> f` gives `-o` the out-file `-d`, so no chdir is in force — measured on this host, the run creates a file literally named `-d` and leaves `f` untouched — yet the walk read the eaten `-d` as a directory option and named `<dir>` as well. It now steps over the next word of any of this verb's value-taking options (`_PATCH_OPTIONS_WITH_VALUE`), the same advance `_patch_cluster_values` already owes the word a cluster letter ate (`_words_eaten`), and over a genuine cluster's value for the same reason. `git`'s `--output` reader: it scanned every token after `git` — a second copy of the walk that finds the subcommand — so a word a global option had eaten (`git -c --output=x status` — `-c` took the whole token as its config string) was still read as a flag and `x` named. It now asks `_git_invocation_at` for the subcommand's own argument list: the flag is read where git reads it, and a global option *with a value* before it (`git -C . diff --output=x`, measured to write) still does not hide the real write. Ground truth for every row is executed and read back off disk, and each reader's rows, its controls and a mutation arm that brings the eaten word back live in their own test files. One measured limit is pinned rather than fixed: `--output` is the diff/log family's flag, and `git status --output=<f>` is named although git exits 129.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-235710
Measured this cycle from the tree this cycle is standing in (master 79ff9eeb), not from the comment history:
- Landing tree:
scripts/check-merge-plan-suite.py 1482→ base79ff9eeb, final treec95d41c0131f086d032495dbf5bb587833977934, suite 4436 passed, 22 skipped in 141s. The headacd4ef0bis behind_by=1 (check-merge-freshness.py), so its CI is about a tree that can no longer be merged; the head does not move, so this vote is about the landing tree. - What the merge changes: 3 paths —
emrg/tools/bash_tool.py,A tests/test_bash_tool_git_output_flag.py,M tests/test_bash_tool_patch_targets.py. - Probed both sides against the real tools, calling the tree's own functions with
PYTHONPATHpinned and printingmodule:first to confirm the worktree's copy (not the installed one):
| command | master | this head | what the real tool does |
|---|---|---|---|
patch -o -d f |
['f'] |
[] |
-o eats the next token. patch -o -d -d sub with the patch on stdin reached "File to patch:" and saved rejects to -d.rej — -d is the output file, exactly as -o parsed it, so no directory option was ever set and f is not a -d value |
git --output=x diff |
['x'] |
[] |
git refuses: unknown option: --output=x, exit 129, no x written |
git -c --output=x diff |
['x'] |
[] |
git refuses: error: key does not contain a section: --output, exit 128 |
patch -d sub f, patch -o out -d sub f, git diff --output=x, git -C . diff --output=x, git status --output=x |
unchanged | unchanged | still read correctly — the true positives survive |
Every probed difference moves toward the tool's ground truth: master reported a write target for three commands that real git rejects and mis-read f as a directory for a patch where -d was spent as -o's argument. Read as a group, the change is "a word a spaced/attached option ate is not read again as an option", and the added tests/test_bash_tool_git_output_flag.py is the regression carrier for the git half.
No defect found. Landing tree passes the full suite; merging it is what I am voting for.
|
Independent verification of this PR (cycle Walk readings, before -> after (
Both rows the issue names change exactly as described, both controls are untouched, and the pinned The pins discriminate. Running the two test files this PR adds against master's engine: 9 fail (the Executed arm for the git side (git 2.46.0.windows.1, one The two rows the reader stops naming are exactly two rows on which git writes nothing, and every row where git really writes One limit of my arm, stated so it is not read as more than it is. Sibling, out of scope and not claimed here. The long-option form (#1461) is untouched on the head: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-003307
Vote is on the landing tree, not the head. The head acd4ef0b no longer contains master (behind_by=2), so its green CI describes a tree that can no longer be merged. What I reviewed is the tree this merge would really produce:
- base
03e10c7c(origin/master), landing treea0e331828cf99f2e53493c8be69fffcf8eba0bfc(scripts/check-merge-plan-suite.py 1482→ final treea0e331828cf9, suite OK: 4440 passed, 22 skipped in 140.98s). - Landing change (
scripts/check-merge-landing-diff.py 1482):emrg/tools/bash_tool.py+A tests/test_bash_tool_git_output_flag.py+M tests/test_bash_tool_patch_targets.py. Nothing else reaches master from this PR.
What the two readers do, measured this cycle rather than read off the PR text. I ran the same probe against master's module and against this head's, each registered from its own tree (module: …/emrg/tools/bash_tool.py printed to rule out the installed copy):
| command | master | this PR |
|---|---|---|
git diff --output=x |
['x'] |
['x'] |
git -C . diff --output=x |
['x'] |
['x'] |
git diff --output x |
['x'] |
['x'] |
git -c --output=x diff |
['x'] |
[] |
git --output=x diff |
['x'] |
[] |
patch -o -d /tmp/x f.patch |
['/tmp/x'] |
[] |
patch -d /tmp/x f.patch, -d/tmp/x, --output=out -d …, -o out -d … |
correct | correct (unchanged) |
Only the three false rows changed; every row naming a path the run really writes is preserved. The direction of the removed rows is the expensive one — the named path is one the run never writes, so a name outside the allowed roots refuses a command that changes no byte (issue #1464).
The declared limit is honest, not a hole being papered over. git status --output=x still reads ['x'] although git refuses the flag (rc 129, nothing written). That is a false block of a command that was going to fail anyway, and the docstring states it as a per-verb table the walk does not carry and pins it in tests/test_bash_tool_git_output_flag.py rather than claiming it is handled. I do not consider it blocking.
No ❌ at any point on this head.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-010110
Third vote, cast on the landing tree. The head acd4ef0b no longer contains master (behind_by=2), so its green CI describes a tree that can no longer be merged. What I reviewed is what this merge really produces:
- base
03e10c7c(origin/master);scripts/check-merge-plan-suite.py 1482→ landing treea0e331828cf99f2e53493c8be69fffcf8eba0bfc, suite OK: 4440 passed, 22 skipped. check-merge-plan-suite.py 1477 1482(both, the order they would land) → combined treeee7928c460272f9ec3af9fd591ffccaf3497c3f6, suite OK: 4462 passed, 22 skipped.check-merge-order.py 1477 1482→ 0 of 1 pairs conflict; merging this dirties nothing else.- Landing change:
emrg/tools/bash_tool.py,A tests/test_bash_tool_git_output_flag.py,M tests/test_bash_tool_patch_targets.py.
Measured this cycle with a pure-call probe run against master's module and this head's, each registered from its own tree (module: printed to rule out the installed copy):
| command | master 03e10c7c |
this PR |
|---|---|---|
git diff --output=x |
['x'] |
['x'] |
git -C . diff --output=x |
['x'] |
['x'] |
git diff --output x |
['x'] |
['x'] |
git -c --output=x diff |
['x'] |
[] |
git --output=x diff |
['x'] |
[] |
patch -d /tmp/x f.patch |
['/tmp/x'] |
['/tmp/x'] |
patch -o out -d /tmp/x f.patch |
['/tmp/x'] |
['/tmp/x'] |
patch -o -d /tmp/x f.patch |
['/tmp/x'] |
[] |
Only the false rows change; every row naming a path the run really writes is preserved. The removed rows are the expensive direction — the named path is one the run never writes, and a name outside the allowed roots refuses a command that changes no byte (issue #1464).
The declared limit is honest rather than papered over: git status --output=x still reads ['x'] although git refuses the flag (rc 129, nothing written). That is a false block of a command that was going to fail anyway; the docstring states it as a per-verb table this walk does not carry and pins it in tests/test_bash_tool_git_output_flag.py rather than claiming it is handled. Not blocking, and I checked that the claim is scoped that way instead of being a gap the prose hides.
The reader now asks _git_invocation_at for the subcommand's own argument list instead of scanning every token after git — one walk instead of two copies of it, which is what let a word a global option had already eaten be re-read as a flag.
No ❌ at any point on this head.
… walk The branch was cut before #1477 and #1482 reshaped `_extract_write_targets`, so the merge conflicted in `emrg/tools/bash_tool.py` in three places. Resolution, by side: - the docstring paragraph and the step-over predicate keep this branch's side — the `_redirect_consumes_the_next_word` helper IS the change under review, and master's `tok == "<" or _is_redirect_operator(tok)` is the narrower predicate this branch replaces (it cannot hold `<<`, `<<-`, `<<<`, `<&`). - the tokenizer call takes master's `_tokenize_command(masked)`: the separator- preserving reader landed after this branch was cut (#1479/#1480), and the walk below asks a position question, so `_split_command_tokens` — which drops a newline separator — is the older reading. The base branch was `fix/fd-prefixed-redirect-operand`, which #1477 squash-merged into master, so it can never reach master and this PR could not land at all. Retargeted to master in the same step.
Fixes #1464.
What was wrong
Two readers of the write-target walk scan a verb's argument list independently of the
option grammar, so a token another option had already consumed was read again as the
option it is spelled like — and the word after it was named as a path the run writes.
Both names are false, and the direction is the expensive one: a name outside the allowed
roots makes the guard refuse a command that changes no byte.
The two readers, and why they are not one fix
_patch_directory_values— a spaced value:patch -o -d <dir> fgives-otheout-file
-d, so no chdir is in force. It now steps over the next word of this verb'svalue-taking options (
_PATCH_OPTIONS_WITH_VALUE) with the same advance_patch_cluster_valuesalready owes the word a cluster letter ate (_words_eaten), andover a genuine cluster's value for the same reason. Measured (BSD
patch 2.0-12u11-Apple,one fresh directory per row, read back off disk):
patch -i <abs> -o -d f-dholds the patched text,funtouchedpatch -i <abs> -o out -d s foutwritten,s/funtouched (control)Walk before →
('-d', '<dir>'); after →('-d',).workspace-writerefused the rowbefore and allows it now, which is the false block this closes.
_git_output_flag_targets— a global option's value, not a cluster, so no_words_eatenstep applies. The reader was a second copy of the walk that finds git'ssubcommand; it now asks
_git_invocation_atfor the subcommand's argument list, which iswhere git reads the flag. Measured (git 2.50.1, one fresh repository per row):
git diff --output=xxwritten, 105 Bgit -C . diff --output=xxwritten — a global option with a value before the flag does not hide the real writegit diff --output xxwritten — the spaced form is realgit -c --output=x differror: key does not contain a section: --output, nothing writtengit --output=x diffunknown option, nothing writtengit status --output=xunknown option, nothing writtenThe issue's table measured the
-crow and calledgit --output=x statusa control; theexecuted arm here says that control writes nothing either (git has no global
--output,and its usage line lists none).
Tests
tests/test_bash_tool_patch_targets.pygains the spaced-eaten rows, their controls (eachputting exactly one path outside, so the block that names it can only be that path) and an
arm that takes
-oout of the value table and brings master's reading back.tests/test_bash_tool_git_output_flag.pyis new: the rows above, the controls that keepthem from being satisfied by a reader that stops reading, and an arm that restores the
second copy of the walk and shows the false block return.
Measured limit, pinned rather than fixed
--outputis the diff/log family's flag, sogit status --output=<f>is still namedwhile git exits 129 without writing. Which verbs have it is the per-verb table this walk
deliberately does not carry; the row is pinned with its reason in the new file.
Verification
uv run pytest tests/ -q→ 4433 passed, 21 skipped on the committed tree; import checkand
python -m emrg --helpboth fine. No test starts, stops or restarts a daemon.