Skip to content

feat(tools): expose editable matches for sed range reads - #178

Open
furgalep wants to merge 1 commit into
mainfrom
fix/shelltools-sed-matches
Open

feat(tools): expose editable matches for sed range reads#178
furgalep wants to merge 1 commit into
mainfrom
fix/shelltools-sed-matches

Conversation

@furgalep

@furgalep furgalep commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • attach verified editable Match anchors to simple read-only sed -n numeric range commands
  • keep sed support fail-closed for transformations, regex addresses, stdin, pipes, multiple files/expressions, and mutations
  • shorten ShellTools prompt-facing documentation and rely on the runtime edit hint
  • remove the unused pyp skill and all remaining pyp references

Follow-on to #172. Retargeted from dev/tui to main so the anchors can carry
resolved_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. Match now gets resolved_path (from #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 repointed
it at a same-named file — 40/40 in a probe, 0/40 now.

A read-only sed can no longer crash run(). self.cwd comes from bash's
logical pwd while the file path is .resolve()d. Through a symlink those
disagree, so after cd /tmp/x a plain sed -n '1,2p' f.txt raised
ValueError out of run(). The root is resolved before relating and the call
is guarded. The existing tests miss this because pytest's tmp_path is already
resolved.

The cwd is captured with the command. run() used to learn the cwd from a
separate pwd. The session lock is per-command, so under asyncio.gather a
concurrent cd lands in between and the harvest resolves against a directory
the sed never ran in — 20/20 anchors lost in a two-task gather, 0/20 now.
BashSession already reports cwd on its control channel under the same lock, so
the extra pwd was redundant as well as racy; run_with_cwd() replaces it and
drops 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, and pushd sub && .... Two seds in one run(), by semicolon or
newline, 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 a
file 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/
  • each new test was re-run against a reverted fix to confirm it actually fails
  • git diff --check

@furgalep
furgalep force-pushed the fix/shelltools-sed-matches branch from 2c7899d to 8b8ec95 Compare August 21, 2026 06:03
@furgalep

Copy link
Copy Markdown
Collaborator Author

Rebased on dev/tui — clean, no conflicts.

I went looking for the same failure mode we fixed in #172. Found one crash, one
thing that only worked because nothing ran concurrently, and one rough edge.

1. A read-only sed range can crash run()

self.cwd comes from bash's logical pwd. The file path gets .resolve()d.
Through a symlink those disagree, and relative_to on the last line of
_harvest_sed_range is outside the try:

cd /tmp/x
sed -n '1,2p' f.txt
ValueError: '/private/tmp/x/f.txt' is not in the subpath of '/tmp/x'

That is every macOS agent that cds into /tmp, or any repo behind a symlink. A
read-only command should never take down run().

The tests miss it because pytest's tmp_path is already resolved, so the two
paths never disagree in CI.

Fixed: resolve the root before relating, and guard the call. Added
test_sed_range_in_a_symlinked_cwd_still_attaches_a_match.

2. asyncio.gather — the cwd was not the sed command's cwd

You asked. It did not work.

run() ran the command, then a separate pwd. The session lock is
per-command, so with two run() calls in flight a concurrent cd lands between
them and the pwd reports a directory the sed never ran in. The harvest then
looks in the wrong place, the content check fails, and the anchor is dropped.

Measured on a two-task gather: 20/20 anchors lost.

BashSession already reports cwd on its control channel (fd 3) under the same
lock as the command, so that extra pwd was redundant as well as racy. Added
run_with_cwd(), which reads it inside the lock, and dropped the round-trip.

0/20 after. It also removes one bash round-trip from every single run().

Covered by test_sed_range_cwd_is_captured_with_the_command. Note it has to
start the session first — otherwise lazy startup yields, cd sub wins the lock
every time, and the race never happens. I confirmed the test fails on the old
code and passes on the new one.

3. The anchor is still cwd-relative — needs #172

Match.path is relative and gets re-resolved when the Match is used, so a later
cd silently repoints it at a different file of the same name. I measured
40/40 mis-anchored.

This is exactly what #172 fixed on main by carrying an absolute
resolved_path. dev/tui has zero resolved_path — the whole of #172 is
missing here, and dev/tui is 74 commits behind main.

I did not fix it in this PR. Making only sed anchors absolute would be
inconsistent with read() and the rg harvest, which have the same flaw on this
branch, and it would turn the tidy a.py the agent sees into a long absolute
path. Re-doing #172 on dev/tui is not this PR's job either.

So: dev/tui should take main before this lands. Then this becomes a
one-liner — Match(normalized, start, end, text, resolved_path=resolved) — and
the anchor is correct by construction.

I left test_sed_range_match_stays_bound_to_its_file_after_a_cd as
xfail(strict=True). It flips to a failure the moment dev/tui gains
resolved_path, so nobody has to remember.

Also worth flagging: on main today, Match(...) without resolved_path is a
TypeError — it is a required keyword arg. So this code does not just need the
merge, it will not import until it is updated.

4. Multiple seds per run() — this part is solid

I threw 14 hostile commands at _parse_sed_range_command. Every one failed
closed:

> out.txt, >out.txt (no space), $(echo f.txt), $F, *.txt, - (stdin),
;, &, two &&, -ne bundled, 1,2p;4,4p, $p, -- f.txt, and
pushd sub && ... (the dir stack it prints trips the content check).

Two seds in one run() — semicolon or newline — is rejected on the operand
count. No complaints here.

5. Rough edge I did not fix

Output gets stripped, so the Match can be wider than what you saw:

$ sed -n '4,7p' x.py
def go():
    pass                 <- 2 lines on screen

Match spans 4-7, text = 'def go():\n    pass\n\n\n'

replace() on that deletes two trailing blank lines the agent never saw. It is
defensible — the range really is 4-7 — but it undercuts the "edit what you saw"
promise. Fixing it means deciding what a range means, so I left it.

Not viable: rebasing onto main

Tried it. The branch carries all of dev/tui's unmerged history, so it tries to
replay 74 commits and conflicts immediately in coding/activity.py. Merge
main into dev/tui instead.

@furgalep
furgalep force-pushed the fix/shelltools-sed-matches branch from 8b8ec95 to 5f3f31a Compare August 21, 2026 06:35
@furgalep
furgalep changed the base branch from dev/tui to main August 21, 2026 06:35
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>
@furgalep
furgalep force-pushed the fix/shelltools-sed-matches branch from 5f3f31a to d2f8fb5 Compare August 21, 2026 06:46
@furgalep

Copy link
Copy Markdown
Collaborator Author

Both fixed, and squashed to one commit.

The anchor now covers only what was displayed. Command output is stripped,
so blank lines at either edge of a range never reach the screen — anchoring the
whole range let replace() delete lines the agent never saw:

$ sed -n '4,7p' x.py      # lines 6-7 are blank
def go():
    pass

before: Match spans 4-7, replace() ate both blank lines
after:  Match spans 4-5, blank lines survive

A range that printed nothing visible now anchors nothing (matches == [])
rather than handing back an anchor over blank lines.

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
gone.

Full suite: 6652 passed, 7 skipped, 0 failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant