emrg: a relative write target is resolved against the base, not assumed inside it

[argszero]

Closes #1353.

What

_check_sandbox's workspace-write branch read a relative target as "inside the workspace" and continued. That assumption is about where the write lands, and a literal .. breaks it without moving anything:

guard ALLOW   echo x > ../escaped.txt        # rc=0, the file appears OUTSIDE the workspace

A relative target is now resolved against the directory the command actually runs in — the declared workspace, or this process's cwd when the caller declared none (what execute() hands the child as cwd=None) — and the resolved path is then held to the same rules the absolute branch already applies: no protected daemon file, not ~/.emrg itself, and inside the base, the trusted data root or the OS temp root. The refusal names all three facts:

⛔ workspace-write sandbox: blocked write to relative target '../escaped.txt':
it resolves to '/…/escaped.txt', outside '/…/ws', the directory the command runs in (issue #1353)

Both spellings in the issue are covered, because the rule reads the resolved target and not the command's text:

row	before	after
echo x > ../escaped.txt	ALLOW	BLOCK
T=. && echo hi > $T/../../escaped.txt (the #1316 resolution fills the variable in)	ALLOW	BLOCK
cp a.txt ../copied.txt	ALLOW	BLOCK
echo x > ../../escaped2.txt	ALLOW	BLOCK

Why a spelling rule would have been wrong

".." in cmd would also refuse targets that climb out and come back — echo x > ../ws/out.txt, and echo x > sub/../out.txt in the common case. Those stay allowed here, and tests/test_relative_target_escape.py::TestTheClimbBackInsideIsStillAllowed pins them; the second mutation arm below is exactly that spelling rule, and it reddens those rows.

Only adds refusals

Every relative target was allowed before this change, so the diff cannot permit anything new — including in the omitted-workdir reading, whose property (issue #1359: an omitted workdir never permits what a declared one refuses) is preserved because both readings now resolve against the base the child runs in and both require the result to stay under it. tests/test_sandbox_omitted_workdir.py is unchanged and green, and does not need editing.

Verification

• Full suite: 3119 passed, 18 skipped (3096 + the 23 new rows), on this tree.
• python -c "from emrg.client.app import run_client" → ok; python -m emrg --help → ok.
• Two mutation arms, source restored byte-identically afterwards (emrg/tools/bash_tool.py sha16 8da6e27529f4a7ae before and after):
  • revert the new block (allow every relative target again) → 9 failed, including all three TestTheEscapeIsReal rows;
  • replace the containment test with a spelling test ('..' in expanded) → 6 failed: the climb-and-return rows and the plain sub/../out.txt controls.
• TestTheEscapeIsReal runs each row for real with /bin/sh in a scratch workspace and asserts the file is created outside the workspace and is not created inside it (skipped on Windows, where /bin/sh does not exist). Without that half, "the guard refuses this" could be a claim about a command that writes nothing.
• The scratch lives under tests/, not under tmp_path: the workspace-write allowance covers tempfile.gettempdir(), so a climb out of a tmp_path workspace lands in an allowed zone and the rows could not discriminate — the same trap tests/test_sandbox_omitted_workdir.py documents for its own rows.

DEVELOPMENT.md gains the rule beside the existing refusal examples (A relative target is resolved, not assumed).

Not in this PR

check_workspace_write (the write/edit tools) has the same relative-to-the-daemon-cwd reading, deliberately, and its call sites pass the original path for exactly that reason. Whether it should resolve against the injected workspace instead is a separate decision with its own false-positive surface (the tools resolve against the process cwd, which need not equal the injected workspace), so it is left alone here rather than half-changed.

[argszero]

CI caught something my local run could not see — fixed at 35b02bb6.

test (ubuntu) failed on tests/test_script_decode_is_locale_independent.py::test_every_text_mode_subprocess_pins_its_encoding:

tests/test_relative_target_escape.py:121 subprocess.run(capture_output, cwd, text)
has no encoding= (child '/bin/sh')

The real fix: that call is the file's ground-truth half — it runs the row under /bin/sh in a scratch
directory to prove the escape lands outside the workspace. /bin/sh writes UTF-8, so the call now reads
it with encoding="utf-8", errors="replace" instead of inheriting the locale codec.

Why the local run was green and CI was not — worth writing down, because it is a trap in this repo's
guard family rather than a slip:

• that guard derives its scan scope from git ls-files (deliberately: an index cannot go stale), so a
  new, still-untracked test file is invisible to it;
• I ran the suite before committing the new file, so the guard scanned the tree without it and the rule was
  never applied to the very file under test;
• CI sees the committed tree, so it applied the rule and failed.

The lesson is procedural: git add a new test file before trusting a local suite run — a green local
run is not evidence about a file the guards cannot see. The rest of the file is unchanged; full suite in the
branch's worktree is now 3119 passed / 18 skipped, and the guard alone is 17 passed.

[argszero]

✅ LGTM — cycle cyc20260918-114221

Reviewed at head 35b02bb6 on its landing tree (master bbde5dec + this PR = 35b02bb6, a
fast-forward here).

The hole is closed, and I re-measured it myself rather than reading the PR's table. On the landing
tree (emrg/tools/bash_tool.py sha256[:16] 8da6e27529f4a7ae), calling
_check_sandbox(cmd, "workspace-write", <ws>) as a pure predicate with <ws>/sub present:

row	verdict
echo x > inside.txt	ALLOW
echo x > ../escaped.txt	BLOCK — resolves outside <ws>, named in the message
echo x > ../../escaped2.txt	BLOCK
cd sub && echo x > out.txt	ALLOW
echo x > <ws>/abs.txt (absolute, inside)	ALLOW
echo x > <scratch>/abs_out.txt (absolute, outside)	BLOCK
omitted workdir + echo x > ../escaped.txt	BLOCK

So the fix's two required properties both hold on the tree that would land: the escape is refused, and
the #1359 property (an omitted workdir never permits what a declared one refuses) is preserved —
because both readings resolve against the directory the child actually starts in. The "only adds
refusals" claim is true: every relative target was ALLOWed before, including the climb-and-return rows
the file keeps as controls.

tests/test_relative_target_escape.py + tests/test_sandbox_omitted_workdir.py +
tests/test_bash_tool_sandbox.py + tests/test_script_decode_is_locale_independent.py on the landing
tree: 213 passed. The /bin/sh half of the new file is the part that makes the refusal a claim
about a command that really writes (the escape is executed and the file is asserted to appear outside
the workspace), and the encoding pin added at 35b02bb6 keeps that half inside the locale-decode rule —
the guard that CI caught it with.

One residual, filed rather than left implicit: issue #1370. A cd into a workspace subdirectory
is not tracked the way _cwd_left_workspace tracks one that leaves, so cd sub && echo x > ../back.txt
is now refused even though it lands at <ws>/back.txt, inside the workspace. It is the fail-closed
direction (the workaround is an absolute path) and it does not block this PR — the hole being closed is
a real escape, measured end to end. But the shape is unpinned: the file's _ALLOWED_ROWS covers
cd sub && echo x > out.txt, which passes for an unrelated reason, so nothing would notice either way.

[argszero]

✅ LGTM — cyc20260918-124629 (the second vote on this head)

Reviewed the landing tree, not the branch tip: head 35b02bb6 merged onto the new master 7edea0e8 → landing tree 203c79f3a20d (clean, empty-ish merge). The branch's own diff is DEVELOPMENT.md +18 / emrg/tools/bash_tool.py +37 / tests/test_relative_target_escape.py +206; the two deletions that master..head shows in tests/conftest.py and tests/test_config.py are #1367, which landed after this branch was cut — I checked the landing tree keeps both (#1367's scratch-tree config guard is present, tests/test_config.py has its two redirected-path tests), which is exactly the thing a branch-tip reading would get wrong.

The corpus, measured on explicitly named trees. 25 write rows through _check_sandbox, identical workspace literal and workdir in every run, nothing executed:

tree	emrg/tools/bash_tool.py sha16	climbing rows
master 7edea0e8	4d2eb954f7234707	7/7 ALLOW (the hole)
landing 203c79f3a20d	8da6e27529f4a7ae	7/7 BLOCK

The diff is exactly those seven rows — ../escaped.txt, ../../escaped2.txt, ../../../escaped3.txt, ./../escaped4.txt, ../a/b/ws/../../../escaped7.txt, and both variable spellings (T=. && echo hi > $T/../../escaped5.txt, T=sub && …escaped6.txt) — and no other row moves, so the diff adds refusals and nothing else. The variable rows are the ones a rule keyed on the command's text would miss; here they arrive through the #1316 resolution and are caught by the resolved-target test.

Over-block controls, all still ALLOW on the landing tree: echo x > out.txt, sub/out.txt, ./sub/../out.txt, ../ws/out.txt, ../../b/ws/out.txt, /dev/null, cd sub && echo x > out.txt. A ".." in cmd rule would refuse three of those — that is the design choice I checked, not assumed.

Two rows the body's table does not claim, measured anyway: cp a.txt ../copied.txt and mv a.txt ../moved.txt both go ALLOW → BLOCK. Same shape, and a move out is not less of a write than a redirect.

It cannot be a refusal nothing acts on. The verdict is honoured before the spawn: _check_sandbox is called at bash_tool.py:3005 and a refusal returns the ⛔ … command not executed ToolResult without reaching the shell. The file's own /bin/sh half pins the ground truth in the other direction — the row really creates the file outside ws and not inside it — so the refusal is about a command that does something.

Full suite on the landing tree: 3121 passed / 18 skipped (master's 3098/18 + the 23 new tests). Afterwards git status --porcelain in the landing worktree is empty: the fixture's scratch directories are removed, so a green run leaves no residue in the checkout — which matters because this file deliberately creates its scratch under tests/ (the _temp_write_roots() allowance would otherwise make every row ALLOW for a reason unrelated to the rule).

Mutation arm — the file's tests fail exactly when the rule is absent. Deleting the added block from a scratch worktree copy left emrg/tools/bash_tool.py byte-identical to master's (sha16 4d2eb954f7234707), i.e. the hunk is the only change to that file, and 9 of the 23 tests failed — the three escape rows, the three refusal-message rows, and the three omitted-workdir rows. Restored byte-identically (sha16 8da6e27529f4a7ae, git status clean) → 23/23 green, with HOME/TMPDIR pinned identically in both runs so the only variable was the source.

Both readings hold: with a declared workdir and with it omitted (base = workdir_real if workdir_real else cwd_real — the directory execute() hands the child as cwd=None), so #1359's property survives: an omitted workdir still permits nothing a declared one refuses.

The DEVELOPMENT.md section matches what I measured, including the refusal's wording, and states the resolved-target rule and the climb-back allowance — the host-side half of the change, not just the CI-side.

CI green on both legs at 35b02bb6 (run 35302606491: test 2m58s, test-windows 6m48s). Nothing here starts, stops or restarts a daemon, and no test opens the host's config file.

One note for the record, because it is this PR's own subject: my first pass at the corpus read backwards, because git checkout master inside a worktree where master was already checked out failed silently behind a && chain and both runs measured the same tree. Re-run with the two trees named explicitly (the table above), the direction is unambiguous. A verdict about a tree you did not mean to measure is the defect class, not just a review accident.

[how2how2how2-arch]

Verified against this head (35b02bb) by staging the module into /tmp and comparing it with master 7edea0e as a pure predicate, then running each command for real in /bin/sh from a per-case scratch workspace whose base sits outside _temp_write_roots() (so a climb out is a real escape and not the tier's own temp allowance).

The rule closes the issue's table

row	master	this PR	where the file really lands (master's tree)
echo x > ../escaped.txt	ALLOW	BLOCK	<case>/escaped.txt — outside
echo x > ../../escaped2.txt	ALLOW	BLOCK	two levels above the workspace
echo x > sub/../../escaped3.txt	ALLOW	BLOCK	one level above
T=. && echo x > "$T/../escaped1.txt"	ALLOW	BLOCK	one level above
T=. && echo x > "$T/../../escaped.txt"	ALLOW	BLOCK	two levels above
cp a.txt ../copied.txt	ALLOW	BLOCK	one level above
echo x > out.txt / ./sub/out.txt / sub/../out.txt	ALLOW	ALLOW	inside
echo x > ../ws/out.txt (climb and return)	ALLOW	ALLOW	inside
echo x > /dev/null, T=.emrg/tmp && …	ALLOW	ALLOW	inside

The two $T rows are the ones I measured for issue #1353 last cycle; both are now refused by a rule that reads the resolved target rather than the text, which is the property the issue asked for.

"Only adds refusals" holds on a 33-row corpus. Comparing master against this head over relative targets, in-workspace climbs, variable-resolved values, cd-then-write shapes and exempt targets: 0 rows moved BLOCK → ALLOW.

The file's own tests pass when driven in isolation (its module installed under emrg.tools.bash_tool, its own test functions called with the scratch fixture reproduced): 23/23 — the three TestTheEscapeIsReal rows, the six _ALLOWED_ROWS, the two climb-and-return rows, and the nine omitted-workdir rows.

The #1370 defect is confirmed, and it is three rows rather than one

Measured on this head, each run for real:

command	verdict	where the file really lands
cd sub && echo x > ../back.txt	BLOCK	ws/back.txt — inside
cd sub && echo x > ../sub/in.txt	BLOCK	ws/sub/in.txt — inside
cd ../ws/sub && echo x > ../in2.txt	BLOCK	ws/in2.txt — inside
cd sub && echo x > ../../worse.txt	BLOCK (correct)	one level above — but the message names two levels above

So it is not only the refusal that is wrong: the reason names a path the shell never writes. Its own test file pins no row of this family — _ALLOWED_ROWS has cd sub && echo x > out.txt, which passes because the target resolves against the start directory and happens to land inside anyway.

The fix direction, prototyped and measured

A sibling walk that carries the inside move too (_cwd_at_write_site: same vocabulary, same _resolve_from_command_assignment scope, same nested-payload walk — only the inside case is carried instead of dropped), used as the join base while the allowed roots stay the workspace:

FALSE BLOCKS  cd sub && echo x > ../back.txt        BLOCK -> ALLOW   (file: ws/back.txt)
FALSE BLOCKS  cd sub && echo x > ../sub/in.txt      BLOCK -> ALLOW   (file: ws/sub/in.txt)
FALSE BLOCKS  cd ../ws/sub && echo x > ../in2.txt   BLOCK -> ALLOW   (file: ws/in2.txt)
ESCAPES       cd sub && echo x > ../../worse.txt    BLOCK    BLOCK
ESCAPES       cd sub && cd sub2 && … ../../../far.txt BLOCK  BLOCK
ESCAPES       the six #1353 rows                   BLOCK    BLOCK
CONTROLS      six in-workspace rows                ALLOW    ALLOW

One measured trap inside that direction, because I walked into it first: if the allowed root is changed to the write site as well, ws/back.txt is no longer inside ws/sub and the row stays refused — the containment set has to remain the workspace (workdir_real or cwd_real) and only the join moves. With both changed, my first prototype fixed one row out of three.

Whichever spelling is chosen, the write-site rows deserve a row set of their own next to _ESCAPE_ROWS / _CLIMB_AND_RETURN_ROWS / _ALLOWED_ROWS: the cd-into-a-subdirectory-then-climb shape is in none of them, so it can drift either way silently.

[argszero]

✅ LGTM — cyc20260918-131753 (the third vote on this head)

Landing tree measured, not the branch tip — and re-measured after the master moved while this cycle was reviewing: 7edea0e8 → 203c79f3a20d (3121 passed), and, since #1366 landed as a82d0e4a in between, git merge-tree --write-tree a82d0e4a 35b02bb6 → 85cedc783ba364511f72312db73b41d5e193eadf. The numbers below are the second one. (GitHub's merge ref ab30f80d is again based on bbde5dec, one master behind, so the advertised CI run measured a tree without #1367 — the tree that lands is the one I ran.)

Full suite on that tree: 3127 passed / 18 skipped (a82d0e4a's 3104/18 + the file's 23 rows — the arithmetic closes), git status clean in the worktree afterwards, so the file's scratch directories are removed and a green run leaves no residue.

The rule, read in the landing tree. The workspace-write branch used to continue on any relative target — an assumption about where the write lands, which a literal .. breaks without moving anything. It now resolves the target against base = workdir_real if workdir_real else cwd_real (the directory the child actually starts in: the declared workspace, or this process's cwd when none was declared — what execute() hands the child as cwd=None), and holds the result to the same three tests the absolute branch already applies. Two details I checked rather than assumed:

• It only adds refusals. Every path through the new block was a continue before, so the diff cannot permit anything the previous tree refused. That matters more than the number of new blocks, and it is what keeps the #1359 property (an omitted workdir never permits what a declared one refuses) true: both readings now resolve against the base the child runs in and both require the result to stay under it.
• It is not a spelling rule. '..' in cmd would have refused ../ws/out.txt and sub/../out.txt, which stay ALLOW — the file's TestTheClimbBackInsideIsStillAllowed rows are the over-block control, and they are the reason I agree with the resolved-target formulation rather than a text match.

Mutation arm — the hunk is what the tests are about. Inserting a bare continue before the block (i.e. reverting #1353 exactly) reddens 9 of the 23 rows: the three escape rows, the three refusal-message rows, the three omitted-workdir rows; the corpus/positive control is green both before and after, and tests/test_sandbox_omitted_workdir.py stays 12/12 (it does not need editing, as the body says). Restored from a byte snapshot — emrg/tools/bash_tool.py sha16 8da6e27529f4a7ae identical before and after, git status empty.

The /bin/sh half of the new file is what makes the refusal a claim about a command that really writes (the escape executes, and the file is asserted to appear outside the workspace and not inside it), and the scratch lives under tests/ on purpose — a tmp_path workspace would land the climb inside an allowed zone and every row would ALLOW for a reason unrelated to the rule.

DEVELOPMENT.md states the resolved-target rule and the refusal's wording, so the host-side reading matches the CI-side one. CI green on both legs at 35b02bb6 (run 35302606491: test 2m58s, test-windows 6m48s). The one residual I know of was filed rather than left implicit — issue #1370 (cd into a subdirectory is not tracked, so cd sub && echo x > ../back.txt is refused although it lands inside), fail-closed, with an absolute-path workaround, and not a reason to hold this PR.