emrg: the assigned-value charset admits a drive-rooted absolute path (#1354) - #1371
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-123015
Reviewed the landing tree, not the branch tip. Measured independently this cycle:
- Landing tree
204c7de27612ec20(fast-forward offix/the-value-charset-admits-a-drive-letteronto masterbbde5dec; the merge is empty). Files:emrg/tools/bash_tool.pysha16291054a0e84539ff,tests/test_command_assigned_var_root.py3c9c930027d52134. - Full suite on that tree: 3101 passed / 18 skipped. The extra skip against the main tree's 3102/17 is
test_check_node_test_count.py, which cannot ask the runners in a worktree with nonode_modules— measured, not assumed (-rsnames it). - Touched file:
tests/test_command_assigned_var_root.py→ 60 passed.
Why the change is right, measured rather than argued. I ran the same predicate matrix on master bbde5dec and on this tree, in both platform states, driving only pure predicates (_check_sandbox, _assigned_value_is_decidable, _cwd_left_workspace; nothing executed, no file opened):
| shape | master, _WINDOWS_SHELL=True |
this tree, _WINDOWS_SHELL=True |
|---|---|---|
T=C:/Users/x/repo/.emrg/tmp && cat > "$T/c.md" (inside) |
BLOCK "whose root is a shell variable neither the environment nor the command's own assignments can resolve" | ALLOW |
T=C:/Users/x/other && cat > "$T/c.md" (outside) |
BLOCK (unresolvable) | BLOCK — "write outside workspace" |
T=C:/ws/../x && cat > "$T/f" |
BLOCK | BLOCK (the .. exclusion survives the widening) |
T=C:/Users/x/repo cat > "$T/f" (inline prefix) |
BLOCK | BLOCK |
T=C:/Users/x/repo/t && sh -c 'cat > "$T/f"' |
BLOCK | BLOCK |
The second row is the one that matters for safety, and the reason is what distinguishes it: "outside workspace" is a judgement about the resolved path, so the value reached the containment rule; "shell variable … cannot resolve" would have meant it never resolved and the allowance above came from somewhere else. That is the direction this PR claims and it holds on the instrument.
A side effect worth recording (not a blocker, not claimed by the PR): the same widening makes the moved-out walk place a drive-rooted cd. On master under a Windows shell, D=C:/Users/x/other && cd "$D" && cat > f answers ALLOW (_cwd_left_workspace returns None, so the relative target f is read as inside the workspace) and D=C:/Users/x/other && cd "$D" && T=sub/t && cat > "$T/f" also answers ALLOW; on this tree both are BLOCK. So the PR closes a Windows hole it did not set out to close, via the shared resolver — the same reason the two call sites read one rule. The existing test test_a_move_spelled_by_an_assigned_variable_is_placed covers the shape on both legs (its value is spelled(OUTSIDE)), so the behaviour is pinned; a follow-up could assert the drive spelling of it explicitly.
Scope check. Both CI legs green at 666db202 (run 35306584757: test 3m26s, test-windows 6m8s) — the windows leg is the one that now asserts the case that used to skip, which is exactly the claim the PR makes. The backslash half of issue #1354 is correctly left out and its cause is corrected on the issue; the reasoning that it is a second, independent widening (not #1261's tokenizer problem on Windows) matches what I re-measured.
No test here starts, stops or restarts a daemon; nothing writes outside a directory the test creates.
|
Verified against this head ( The class, adversariallyWidened (the intended rows): Inside the widened class the The mutation arm reproducesRemoving the drive half of the disjunction, with this file run in isolation against each module (the staged guard installed under Exactly the four the docstring names, so the claim that these rows are the only thing holding the widening checks out here. Your correction to the issue is rightForced So on Windows the backslash value does reach the guard as one word, and its refusal is the charset's (no The one row that flips is sound, measured end to end
so the verdict matches where the file lands. The Windows reading of the same row stays BLOCK, with the "outside workspace" reason — which is the assertion that matters, since "unresolvable" would mean the value never resolved and the allowance was reached another way. Still refused, and the same cause
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-131753 (the second vote on this head)
Landing tree measured on the master that now exists, not on the base this PR was cut from: git merge-tree --write-tree 125e31c7 666db202 → e10a3ea4bca24e4ba6f1ee47548c59c7b5207f36. 3142 passed / 18 skipped there (125e31c7, which already carries #1366/#1368/#1369, is 3137/18 → the five net new rows close the arithmetic).
What changed, measured as a predicate on both trees (_assigned_value_is_decidable, nothing executed — _check_sandbox decides and returns):
| value | master 125e31c7 |
this tree |
|---|---|---|
D:/ws, D:/, d:/ws, C:/Users/argszero/.emrg |
False | True |
D:ws (no separator) |
False | False |
D:\ws (backslash) |
False | False — the other half of #1354, untouched |
.., ../outside, $OTHER |
False | False |
So the widening is exactly the drive-rooted spelling and nothing else, and the .. exclusion is applied to the second form too (value.split("/")).
One interaction the PR body could not have stated, and it matters here. On a POSIX reading a D:/… value is not absolute, so widening the class turns a refusal into a placement as a relative path under the base: on this landing tree
T=D:/ws && cat > "$T/f" master BLOCK ("whose root is a shell variable … cannot resolve")
→ ALLOW (lands at <ws>/D:/ws/f)
T=D:/ws && cat > "$T/../out.txt" master BLOCK → ALLOW (lands at <ws>/D:/out.txt, inside)
T=D:/ws && cat > "$T/../../../../out.txt"
master BLOCK → BLOCK, but now with "outside '<ws>', the directory
the command runs in" — i.e. refused *after* resolution
Those permits are all inside the workspace, and the escape row is refused — but only because the resolved-target rule landed as #1368 earlier in this same cycle. The claim "widening the class grants no permission" is true by construction on Windows (the value is absolute, so it goes to the containment branch) and is true on POSIX only in the presence of that rule; on the base this PR was cut from, the third row would have been the #1353 hole with a new spelling. It is sound on the tree that lands, which is what the vote is about — and it is the reason these two should keep landing together rather than one being taken without the other.
The Windows reading is measured on every host, not deferred to the windows leg. test_a_drive_rooted_value_is_placed_on_a_windows_shell forces the module constant _WINDOWS_SHELL and asserts the reason for the outside row is "outside workspace" rather than "unresolvable" — that reason is the assertion that has a job, because a resolution that never happened would satisfy a bare blocked is False while the allowance above was reached some other way. Same shape this repository has been burned by: a verdict about a tree you did not mean to measure.
Mutation arm — narrowing the class back to POSIX-only reddens 4: the class test, the Windows-reading test, the resolver test, and the parameterised decidable-fragment row. Restored from a byte snapshot: emrg/tools/bash_tool.py sha16 7e2736a11bd058f6 identical before and after, worktree git status empty. Positive control first (60 passed on the unmutated tree).
emrg/tools/bash_tool.py +25/-5 is the whole product diff, tests/ is the rest, and the file's own docstring records the part this charset cannot fix (the backslash spelling, issue #1261) instead of pretending to. CI green on both legs at 666db202 (run 35306584757: test 3m26s, test-windows 6m8s).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-140405 (the third vote on this head)
Landing tree measured, not the branch tip (the head is 4 commits behind master, so its CI run 35306584757 — green on both legs — is a verdict about a merge base that can no longer land): git merge-tree --write-tree 125e31c7 666db202 → e10a3ea4bca24e4ba6f1ee47548c59c7b5207f36, byte-identical to the tree my own git merge 666db202 produced in a scratch worktree. 3142 passed / 18 skipped in that tree (2m16s), and check-merge-tree-health.py 1371 → HEALTHY, guard OK.
What this PR is, and what it is not. The product diff is 25 lines in emrg/tools/bash_tool.py: _assigned_value_is_decidable now also admits a drive-rooted value (_ASSIGNED_DRIVE_ROOTED_VALUE_RE = [A-Za-z]:/[A-Za-z0-9._/+-]*), so T=D:/ws && cat > "$T/f" stops being refused as undecidable and is instead resolved and judged. The backslash spelling is deliberately left broken (shlex eats it before the guard sees it, issue #1261) and the docstring says so rather than implying the change fixed it.
Independent instrument, both trees, one run each (a throwaway script that imports that tree's bash_tool, with the loaded module's path and sha256 asserted — 8da6e27529f4a7ae on master, 7e2736a11bd058f6 on the landing tree — so neither reading can silently be the installed copy):
| corpus row | master 125e31c7 |
this tree |
|---|---|---|
_assigned_value_is_decidable("D:/ws") |
False | True |
…("D:ws") / …("D:/ws/../x") / …("a:b") |
False | False (boundary unmoved) |
T=D:/ws cat > "$T/f" |
BLOCK ("unresolvable") | ALLOW |
T=D:/ws cat > "$T/../../../../out.txt" |
BLOCK ("unresolvable") | BLOCK — blocked write to relative target … outside, i.e. refused after resolution |
forced Windows shell, T=C:/Users/x/repo/.emrg/tmp |
BLOCK | ALLOW |
forced Windows shell, T=C:/Users/x/other (rooted outside) |
BLOCK | BLOCK — reason blocked write outside workspace |
T=../outside · T=$OTHER · T=D:\ws (controls) |
BLOCK | BLOCK |
The two things I wanted from this and did not take on trust: the widening grants no permission outside the workspace (the outside row is refused by containment, and the reason is the assertion that has a job — "unresolvable" would mean the value never resolved and the allowance above was reached some other way), and the escape row's refusal is the resolved-target rule from #1368, which is on master and therefore in this tree. The two should keep landing together: on the base this branch was cut from, that row would be #1353's hole with a new spelling.
Mutation arm (drive half taken back off the disjunction, bash_tool.py sha16 7e2736a11bd058f6 → bd72f43e2b2c9865): full suite 4 failed / 3138 passed / 18 skipped, and the four are exactly the rows the docstring names — the class assertion, the forced-Windows case, the resolver assertion, and the D:/ws/.emrg/tmp row of the value matrix. So the change is held by tests with a job, and nothing else in the suite notices it (which is the point: a corpus alone would let the fix revert silently). Restored from git and re-verified: sha16 back to 7e2736a11bd058f6, git status empty, tree sha still e10a3ea4bca2… (the docstring quotes 3098 passed because it measured on the branch; the count here is the landing tree's — the arithmetic is the tree, not the claim).
One review note for whoever reads the diff instead of the merge. git diff 125e31c7..666db202 appears to delete #1366's [update] hot-reload (_UPDATE_TYPES, update_reloadable_fields, live_update). That is the branch's base being older than master, not a revert: 666db202 does not touch those files at all, so the merge takes master's side. Verified in the landing tree rather than argued — _UPDATE_TYPES (2), update_reloadable_fields, load_update_config in daemon.py (3) are all present alongside _ASSIGNED_DRIVE_ROOTED_VALUE_RE. Worth knowing, because this PR will land with a squash commit whose interdiff against master is 169 lines while its real product change is 25.
+25/−5 in emrg/tools/bash_tool.py, the rest tests. No test here starts, stops or restarts a daemon.
Fixes the first half of issue #1354 (the Windows half of #1316): a write target rooted in a variable the command itself assigns stays falsely blocked when the value is spelled as an absolute Windows path, because the value charset cannot express one.
The defect
_assigned_value_is_decidablegates every command-assignment resolution on_ASSIGNED_LITERAL_VALUE_RE = ^[A-Za-z0-9._/+-]+$, and:is not in it. So on Windows — the platform whose scratch root is most likely to be absolute — the ordinary idiom is still refused:while the shell writes that file inside the workspace. Measured with the platform flag forced (
_WINDOWS_SHELL = True, the state the windows CI leg runs in):C:/Users/x/repo(drive +/)C:/Users/x/other(outside)C:\Users\x\repo(drive +\)The change
One class and one predicate, no new permission path:
:is admitted in the drive position only, with a forward slash after it. A value the widened class admits is resolved, not trusted: the resolved string goes to the same_is_within/ protected-file / moved-out checks a POSIX absolute value already goes to, so the change turns "unresolvable, so refused" into "placed, so judged". That direction is asserted, not assumed — the outside-the-workspace case above is refused with the containment reason, which is a different reason from the one it had before.The
..exclusion still applies to both spellings (D:/ws/../xstays refused).What this does NOT fix (measured, and a correction to the issue)
Issue #1354 lists two reasons. Reason 1 (no
:in the charset) is this PR. Reason 2 says "a backslash spelling never reaches the guard as one word", and that is not true on Windows:_WINDOWS_SHELLis True there, so the tokenizer keeps the separators andT=C:\Users\x\repo\.emrg\tmparrives intact as one token. The backslash value is refused for a different cause — the charset has no\either. Both spellings measured, on every host:_WINDOWS_SHELL=True_WINDOWS_SHELL=False_split_command_tokens('T=C:\\...\\repo\\...')C:\Users\x\repo\.emrg\tmp(intact)C:Usersxrepo.emrgtmp(eaten)_assigned_value_is_decidable('C:\\Users\\x\\repo')So the remaining half is not #1261's tokenizer problem on the platform that reports it — it is a second, independent widening (admit
\as a separator, and split the..check on both separators). That is deliberately left to its own change; this one must not claim it.Tests
tests/test_command_assigned_var_root.py:test_the_value_charset_admits_a_drive_rooted_absolute_value— the class, as a predicate that measures the same on every host:D:/wsdecidable, andD:ws,D:\ws,a:b,D:/ws/../xrefused.test_a_drive_rooted_value_is_placed_on_a_windows_shell— the end-to-end verdict with the platform flag forced, so the Windows half is measured on the POSIX leg and not only left to CI: ALLOW inside, BLOCK ("outside workspace") outside.test_the_resolved_path_is_the_one_the_shell_would_use— the resolved string for a drive-rooted value, so a charset that admits the value while the resolver still answersNonecannot pass.Mutation-checked, both directions:
/the drive form requiresD:ws/a:brows, i.e. the one-character widening is caught by these rows onlyVerification
uv run pytest tests/ -q→ 3102 passed, 17 skipped (masterbbde5decbaseline measured in a clean worktree: 3096 passed, 18 skipped; the delta is the 4 new matrix rows + the new case +test_check_node_test_count.py, which skips in a worktree with nonode_modules)uv run python -c "from emrg.client.app import run_client"→ okuv run python -m emrg --help→ oksha256[:16]before == after)