feat(tools): expose editable matches for sed range reads - #178
Conversation
2c7899d to
8b8ec95
Compare
|
Rebased on I went looking for the same failure mode we fixed in #172. Found one crash, one 1. A read-only sed range can crash
|
8b8ec95 to
5f3f31a
Compare
Attach verified editable `Match` anchors to simple read-only `sed -n` numeric range commands, so a range read can be edited directly instead of being re-read through another tool. Support stays fail-closed. Transformations, regex addresses, stdin, pipes, redirects, globs, variables, command substitution, multiple files or expressions, and anything with `;`, `&` or a second `&&` all fall back to an ordinary shell result with `matches=None`. Anchor correctness: - The anchor carries `resolved_path` (#172), so the relative path is display-only and read/replace act on the file sed actually read. Without it the anchor was re-resolved on use and a later `cd` silently repointed it at a same-named file. - `run()` captures the cwd under the same session lock as the command via the new `BashSession.run_with_cwd()`. It previously learned the cwd from a separate `pwd`, so under `asyncio.gather` a concurrent `cd` landed in between and the harvest resolved against a directory the sed never ran in. BashSession already reports cwd on its control channel under that lock, so the extra `pwd` was redundant as well as racy; dropping it also removes one bash round-trip from every `run()`. - Relating the file to the cwd is guarded and resolves the root first. The cwd comes from bash's logical `pwd` while the file path is resolved, so through a symlink the two disagree and a plain `sed -n '1,2p' f.txt` after `cd /tmp/...` raised ValueError out of `run()`. File operations may also legitimately sit outside cwd (#172). - The anchor covers what was displayed, not the whole requested range. Command output is stripped, so blank lines at either edge were never shown, and anchoring the full range let replace() delete lines the agent never saw. Also shortens the ShellTools prompt-facing documentation in favour of the runtime edit hint, and removes the unused `pyp` skill and its remaining references. Each new test was re-run against a reverted fix to confirm it fails. Signed-off-by: Paul Furgale <pfurgale@nvidia.com>
5f3f31a to
d2f8fb5
Compare
|
Both fixed, and squashed to one commit. The anchor now covers only what was displayed. Command output is stripped, A range that printed nothing visible now anchors nothing ( Two tests for it, both checked against a reverted fix to confirm they fail. Squashed. One commit, so the middle-commit-doesn't-stand-alone problem is Full suite: 6652 passed, 7 skipped, 0 failed. |
Summary
Matchanchors to simple read-onlysed -nnumeric range commandspypskill and all remainingpypreferencesFollow-on to #172. Retargeted from
dev/tuitomainso the anchors can carryresolved_path, which is what makes them correct — see below.Anchor correctness
The first cut of this had three ways to hand back an anchor pointing at the
wrong file, or to fail outright. All three are fixed here.
The anchor is absolute.
Matchnow getsresolved_path(from #172), so therelative path is display-only and read/replace act on the file sed actually
read. Without it the anchor was re-resolved on use, and a later
cdrepointedit at a same-named file — 40/40 in a probe, 0/40 now.
A read-only sed can no longer crash
run().self.cwdcomes from bash'slogical
pwdwhile the file path is.resolve()d. Through a symlink thosedisagree, so after
cd /tmp/xa plainsed -n '1,2p' f.txtraisedValueErrorout ofrun(). The root is resolved before relating and the callis guarded. The existing tests miss this because pytest's
tmp_pathis alreadyresolved.
The cwd is captured with the command.
run()used to learn the cwd from aseparate
pwd. The session lock is per-command, so underasyncio.gatheraconcurrent
cdlands in between and the harvest resolves against a directorythe sed never ran in — 20/20 anchors lost in a two-task gather, 0/20 now.
BashSessionalready reports cwd on its control channel under the same lock, sothe extra
pwdwas redundant as well as racy;run_with_cwd()replaces it anddrops one bash round-trip from every
run().Fail-closed check
14 hostile commands, all rejected:
> out.txt,>out.txt,$(echo f.txt),$F,*.txt,-(stdin),;,&, two&&,-ne,1,2p;4,4p,$p,-- f.txt, andpushd sub && .... Two seds in onerun(), by semicolon ornewline, is rejected on operand count.
The anchor covers what was displayed. Command output is stripped, so blank
lines at either edge of the range never reach the screen. Anchoring the full
range let
replace()delete lines the agent never saw —sed -n '4,7p'on afile whose lines 6-7 are blank showed 2 lines but anchored 4. The anchor now
narrows to the span that actually produced the output, and a range that printed
nothing visible anchors nothing.
Testing
uv run ruff check .uv run pytest -q tests/tools/ packages/nooa-cli/tests/test_coding_activity.py(328 passed)uv run pytest -q tests/ packages/nooa-cli/tests/git diff --check