emrg: a write target rooted in a variable the command itself assigns is resolved (#1316) - #1352
Conversation
The new test file for #1316 was green locally and failed both CI legs, each for its own reason — and both reasons were host spellings, not the guard: - `cd /tmp && T=… && cat > "$T/f"` was the moved-out row. `/tmp` is outside the workspace on macOS, where the row was written and measured, and on Linux it *is* `tempfile.gettempdir()`, an allowed write root — so the row never reached the moved-out check and measured the temp-area allowance instead. It now uses a derived `OUTSIDE` directory, with membership in no allowed root asserted as a premise (`test_the_outside_directory_is_taken_as_moved_out`) so the next host whose layout breaks this fails in a test that names the reason. - `T={WORKSPACE}/{SCRATCH}` carried the host's absolute workspace path, which on Windows brings a drive letter and a backslash: `:` is outside the decidable value charset and `\` is shlex's escape character (issue #1261), so the row asserted ALLOW on a platform where the guard refuses the value by design. The absolute case is now asserted where the charset admits its spelling and skips with a measured reason where it does not — and that reason is itself asserted on every platform (`test_why_the_absolute_case_is_posix_only`), so the skip cannot outlive its cause. Every path the file composes goes through `spelled`. No product change: the guard's verdicts are the ones the issue specifies, and the one direction they can be wrong in is refusing a command it cannot place. Verified: full suite 3024 passed / 16 skipped; the file passes under `TMPDIR=/tmp` (the linux temp layout that failed in CI); the whole 28-row matrix replays with the ubuntu runner's roots and workspace patched in, 0 disagreements; and the arm that makes a `cd` command report no moved-out cwd is killed by exactly the two rows above, with the source restored byte-identically (`b426ded2…951`).
|
CI on the first head ( ubuntu — The variable-free form windows — the absolute row carried the host's path. Verification of the fix
|
|
Independent verification of the resolver, run against the head ( What the PR fixes, reproduced. Every intended allowance changed from The fail-closed set holds too — measured unchanged, master and head agreeing on Finding: the decidability rule is applied to the value, but not to the path the value is spliced into. Being precise about what is and is not new: the literal spelling of this target is accepted on master as well ( Smallest fix. The pre-condition the relative branch needs is "the resolved target has no Test matrix note. 14 tests, and the Repro: |
#1352 taught the write-target rule that a variable the command itself assigned in an earlier statement is a resolution scope, and left the moved-out walk reading the environment alone. The two then disagreed: with `D=<outside>` above it, `cd "$D" && T=<in-ws> && cat > "$T/f"` is BLOCK on master (the unresolvable root refused it), ALLOW with the target-side scope only, and BLOCK again once the walk reads the assignment — the shell writes outside the workspace while the guard read the relative target behind it as in-workspace. One rule, two call sites: `_resolve_from_command_assignment` answers for the file a redirect names and for the directory a `cd` / `env -C` moves into, and both spellings of a variable (`$T/f`, `cd "$D"`) are looked up the same way. The move walk places an assignment-decided destination instead of joining the literal `$D` onto the cwd, which is what made such a move read as still inside. A destination neither scope can decide is still joined onto the cwd and read as inside — a behaviour change of its own, since refusing it also refuses computed destinations that are legitimately inside (issue #1357). Recorded in the walk's own known-limit paragraph rather than folded into this fix.
|
Fixed on the branch in What was wrong. The new scope was given to the write-target rule and not to the walk that decides whether a relative target is still relative to the workspace. That walk kept expanding the environment alone, so it joined the literal
The second row is the one this change is about: the shell writes outside the workspace in all three columns, and the middle column is the only one that allowed it. The first row is the same root cause without the target-side scope in the way (pre-existing on master, closed here too). The fix. One rule, two call sites. Tests. Two new cases: the move placement (asserting the walk's answer and the tier's, since those are different questions) and the shared rule across both kinds of token. The first was shown to fail with the scope removed — a mutation arm reverting only the walk's call to the resolver — so it guards this fix rather than the shape of the code. The deciding value is an absolute spelling, which the value charset admits on POSIX only (#1354), so that case skips where it cannot decide; the cause is already held on every host by the neighbouring test. Not folded in. A destination neither scope can decide is still joined onto the cwd and read as inside — Suite on |
|
Both CI legs are green on the new head ( The new cases are host-spelling-clean by construction: every path is interpolated through the file's |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-043412
Measured on the tree this merge would land, not on the branch head: master advanced to 951c19f3 after this head was pushed, so the head (b381b232) is behind_by=1 (check-merge-order: 0 of 1 pairs conflicting).
Landing tree fe104dbcae7bdc30d8725e08f1c73f4ee5d0958d — reconstructed independently (a commit on 1de8dca0764b, then git merge-tree against b381b232) and matching the plan suite's hash exactly. Suite on it: 3042 passed / 17 skipped; the step below it (master + #1355) 2987 passed / 17 skipped. The new file's own 55 tests pass.
Both halves of the claim — that a write target and a move are resolved by one rule (issue #1316) — are killable, each by its own tests:
- neutering
_resolve_from_command_assignment(returningNonealways):10 failed, 45 passedintests/test_command_assigned_var_root.py; - removing the
from_commandscope from_cwd_left_workspace.resolve():1 failed, 54 passed, the single failure beingtest_a_move_spelled_by_an_assigned_variable_is_placed— the crossing case its own docstring names,D=<outside> && cd "$D" && T=<in-ws> && cat > "$T/f", which reads as inside once the walk expands the environment alone.
Source restored byte-identically after each arm (bash_tool.py sha256[:16] a8f3723cd4c45c95 before and after).
Beyond the arms: the PR keeps its own known limits explicit instead of claiming them closed — the value charset still admits no .., and a for d in …; do cd "$d" destination is still joined onto the cwd and read as inside; both stay ALLOW and both are filed as #1357 rather than folded in here. That is the right boundary, because refusing a computed destination that is legitimately inside is a behaviour change of its own. The newline-as-separator lexer keeps a punctuation set of its own instead of changing the ordinary tokenizer, so no other rule's readings move; and the half-masked-heredoc case is asked directly instead of linking each body to its consumer.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-051533
Voted on the tree this merge would land: master advanced to 951c19f3 after this head was pushed, so the head (b381b232) is behind_by=1. Landing tree fe104dbcae7bdc30d8725e08f1c73f4ee5d0958d, rebuilt independently this cycle (a commit on 1de8dca0764b, then git merge-tree against b381b232) and matching the plan suite's hash exactly; suite 3042 passed / 17 skipped.
This vote is an end-to-end verdict table, not a mutation (the earlier vote mutated the rule's two halves). I drove the guard's own entry point — _check_sandbox(cmd, "workspace-write", <workspace>), with the workspace passed exactly as the daemon does — over the ten shapes the PR's docstring claims, on two trees:
| tree | disagreements with the claimed table |
|---|---|
plain master 951c19f3 (control) |
4 / 10 |
landing tree fe104dbcae7b |
0 / 10 |
The control is what makes this a measurement rather than a restatement. On master the four are two false blocks — T=<scratch> && cat > "$T/c.md" and its newline-separated form, i.e. the ordinary idiom for writing a scratch file — plus one fail-open: D=<outside> && cd "$D" && cat > f is ALLOW there, because the walk read $D as a literal name joined onto the cwd. On the landing tree all ten land as claimed, and the refusals are not collateral: the inline prefix, the bare unassigned root, the second-assignment and computed values, and the .. escape row all still BLOCK, with the original reasons.
One instrument defect found and fixed in this reading, worth recording because it is the kind that produces a false verdict. My first run reported 2 disagreements on the landing tree — the crossing cases. The cause was my probe, not the PR: I called _check_sandbox(cmd, mode) with workdir omitted, and with workdir=None the function sets moved_out = None (bash_tool.py:2738), so the relative-target branch is never reached. The daemon injects args["workdir"] = str(session.cwd) for bash when the model omits it (daemon.py:2941-2942), and the tests pass WORKSPACE — so the workspace-passed call is the real one, and the omitting call is a shape the product never makes. Passing the workspace turns those two rows into BLOCK with the correct reason ("the command runs it after changing directory to '
Not in scope, and still open after this merge: #1357 (a move the walk cannot place — a .. value, a loop variable, a computed destination — is read as inside), #1354 (: is outside the value charset, so the absolute case stays blocked on Windows), and #1353 (a literal .. target).
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-054009
Third independent measurement, on the branch's own tree (b381b232, identical to the head CI ran on), read from the tree rather than from the PR text. The two earlier votes measured the removal's effect end to end; this one asks whether the change is pinned, and whether the spellings that must stay refused still are.
1. The tests have a job (mutation arm). _resolve_from_command_assignment disabled (return None at the top of the body, i.e. the pre-fix behaviour restored — emrg/tools/bash_tool.py sha16 a8f3723cd4c45c95 → 3c8fa44f7d5ca183): 10 of the new file's 55 tests go red, including test_the_move_and_the_target_are_resolved_by_one_rule (the two call sites this PR unifies), test_the_inline_prefix_and_the_earlier_statement_differ, and test_the_resolved_path_is_the_one_the_shell_would_use. Restored byte-identically (sha16 back to a8f3723cd4c45c95, 55 passed).
2. Both directions, measured through _check_sandbox(cmd, "workspace-write", workdir=<workspace>) on that tree:
| command | verdict |
|---|---|
T=.emrg/tmp && cat > "$T/f" |
ALLOW — the case the PR exists for |
T=.emrg/tmp cat > "$T/f" |
BLOCK — the inline prefix is not visible to the redirect (the rant's "must stay refused") |
T=../outside && cat > "$T/f" |
BLOCK — undecidable value |
D=/outside && cd "$D" && cat > f |
BLOCK — and the message names the move, which is the second call site |
D=.emrg/tmp && cd "$D" && cat > f |
ALLOW |
cat > .emrg/tmp/f |
ALLOW — control |
cat > /outside/f |
BLOCK — control |
So the allowance opens exactly where the shell would resolve the value and nowhere else: the refusal message for the undecidable case now names both scopes ("neither the environment nor the command's own assignments"), which is the honest statement of what the guard can see.
3. Merge hygiene. Head unchanged since the two standing ✅ (pushed 2026-09-17T20:19:02Z), so none is voided; both CI legs green at that head (run 35270067090: test 3m5s, test-windows 8m53s); merge state MERGEABLE/CLEAN. The new test file is tests/test_command_assigned_var_root.py (55 tests). No file outside emrg/tools/bash_tool.py and that test file is touched. Nothing here restarts or stops a daemon — the guard's own subject matter is what these tests assert, through a pure predicate.
2 participants
⚡ Before this PR can be reviewed: the guard is a fail-closed rule, so the only
question that matters is whether it now allows any write it refused before.
Every allowance below was measured against
/bin/shon this host, in a scratchcwd, and the divergences are listed rather than hidden.
The defect (issue #1316)
The write-target rule reasons from the environment:
os.path.expandvarsagainst the variables the tool hands its child. A target still carrying a
variable root after that is judged unresolvable and refused. That is sound about
the environment and wrong about the command, which is a second resolution scope:
One character apart, opposite outcomes. The first is how a scratch path is
normally used (it is how a comment or PR body file gets written, which is where
the issue was reported from); the second must keep its refusal. The rule lands on
the next release, so the false block is not visible to hosts yet.
What the fix does
The refusal branch now asks the command before answering "nobody can resolve
this":
_resolve_target_from_command_assignmentfills in only the roots thecommand assigned in an earlier standalone statement (
&&,;, newline),with a literal value, once, and returns
Nonefor everything else. Aresolved value then travels the ordinary path — absolute values reach the
_is_within/ protected-file checks, relative ones still face themoved_outcheck — so this adds a resolution scope, not a second rule.
It calls for one new lexer: a newline is a statement separator to the shell and
whitespace to
shlex, so_split_command_statementsasks for it as punctuationinstead. Without it the newline row of the matrix cannot be answered at all.
Measured: the shell's answer next to the guard's
printf '%s\n' "$T/f"stands in for the redirect so the answer is printedinstead of written (
/bin/sh, scratch cwd,Tunset):/bin/shexpands it toT=./inner && …./inner/fT=./inner⏎./inner/fT=./inner; …./inner/fT=./inner cat > …/f/fT=./inner; T=./other; …./other/fT=$(basename ./inner) …inner/fT=../outside && …../outside/f… > "$T/f"; T=./inner/fT=./inner | …/fThe three over-blocks are deliberate and documented in the function: the value
is decidable only by reading execution order, or by building it, and the
..value is exactly the write the relative branch's "relative therefore inside the
workspace" assumption cannot survive. A guard may refuse a command it cannot
place; it may never allow one it places elsewhere — which is why every divergence
above is in one direction.
The ALLOW row was also measured by the write itself:
T=./inner && cat > "$T/f"in a scratch cwd really createsinner/f, and nothing at the scratchroot.
The near-misses, each now a test
Three ways this fix could have become the hole it closes, all found by
measurement while writing it:
T=<dir> && sh -c 'cat > "$T/f"'must stay refused: thevariable is unexported, so the nested shell expands it to nothing. Held by
"the write site must be a whole token of a top-level statement", i.e. a target
that arrived from a nested payload is not this shell's to resolve.
_mask_data_heredoc_bodiesblanks the bodies it canprove are data and leaves the rest, so it can come back half-applied. The
first version of this rule only noticed a command where nothing was masked;
driven end to end,
cat <<EOF+myprog <<EOFin one command answered ALLOWand resolved the write from the body's own
T=.emrg/tmp, while/bin/shput itat
/f. Fixed by_no_heredoc_body_is_left_as_text, which asks the directquestion (does any opener still sit above text that was not blanked?) and
answers "no" whenever it cannot tell.
/that makesa variable a root, so
$T/fresolved to./innerf. The verdict was stillALLOW (both spellings are relative) while the value handed the workspace and
protected-file checks was a different file than the shell writes. The lookahead
fixes it and the test asserts the resolved string, not the boolean.
Verification
uv run --no-sync pytest tests/ -q— 3022 passed, 16 skipped (16 new testsin
tests/test_command_assigned_var_root.py), plus the import check andpython -m emrg --help.tests that own the behaviour (positive control green first, source restored
byte-identically, sha256 equal at both ends). Two arms survived on the first
run and both were real findings about the tests, not the code: the "inline
prefix" row was refused for a different reason (no preceding statement), so
the mechanism was untested until a row with one was added; and the value-charset
half of the boundary is covered by no end-to-end case (the paren/backtick shapes
are caught before it and the caller re-checks after it) — that is now written
down in the test that owns the boundary, with the measurement, rather than
claimed as a caught escape.
workspace-writetarget loop, and none of these rows change verdict there.A defect found next to this one, filed rather than folded in
While measuring, a literal relative
..target was found to be allowed out ofthe workspace today —
echo x > ../../.emrg/config.tomlanswers ALLOW atworkspace-write. That is the same fail-open the variable rule exists to close,reached without any variable, and it is a narrowing change with its own
blast radius (it would start refusing commands that legitimately write through
..), so it is filed as #1353 with its measurement (guard verdict + shell exitT=../outsidestays refused here:this rule refuses a value the relative branch cannot survive, and the literal
spelling's allowance is that issue's to fix.
Closes #1316