feat(terminal): add Open with AI agent actions - #1094
spandan11106 wants to merge 2 commits into
Conversation
screenrecording-2026-09-17_19-53-35.mp4Visual evidence for the AI agent actions. What the video shows, in order:
This closes #93. |
UI made better |
|
Three follow-up commits from testing this by hand. Selected paths can now be placed in the command (c94c57f). Appending paths as arguments only suits agents that take files that way, like aider. Claude Code and Codex take a single prompt instead, so selecting several files reached them as one path with the rest dropped — the selection looked like it was being ignored. The command now says where the paths go:
A bare The agent command entry now confirms a save (d979472). It saved on Enter without changing anything on screen, so there was no way to tell whether the typed value had been taken. The row shows "Press Enter to save" while the field differs from the stored command, and "Saved" briefly once it is stored. A failed agent stays on screen (d979472). The panel hid itself whenever the child exited, so an agent dying on a bad flag or a missing credential took its own error message with it. Only a shell you exited closes the panel now; an agent leaves its output in place with a line reporting how it ended. The saved/unsaved notice no longer wraps mid-word (d47b203). It sat beside the entry, where the row's title column squeezed it until "Saved" broke across two lines. It now sits under the field, on one line. Settings copy and |
|
I reviewed the current #1094 head as the third PR in the terminal stack, both against its actual PR2 base and cumulatively against The merge-base between PR2 and PR3 is exactly the current PR2 head, so the PR3 delta is clean and attributable. Overall, I still like the architecture here. The agent feature stays mostly provider-agnostic, uses the existing window-scoped terminal, keeps selected paths out of shell command strings, disables browser cwd synchronization while an agent owns the session, and adds the action in the places we discussed in #93. The results surfaced five confirmed runtime issues, one coverage gap, and two contract/scope concerns. 1. Selected native paths can be silently changed before reaching the agentThe agent path handling currently converts native paths through I reproduced this with a selected filename containing the native byte How to reproduceCreate a file with an invalid UTF-8 byte in its native name, configure a small argv-dumping agent, select the file and choose The probe compared the original native bytes with what the agent actually received. Expected native suffix: Received by the agent: The original The agent still started successfully and no error was shown. So the selected file path was silently changed before reaching the process. finding-1-agent-non-utf8-path.mp4
Suggested directionI would preserve native paths until the process boundary. If the VTE/GLib API cannot represent a path exactly, fail safely: The important invariant is that a selected native path must never silently become a different path. The resolved executable path should follow the same rule. 2. A shell builtin waiting for input can be killed and replaced by the agentThis is an interaction with the PromptReady problem from PR2, but PR3 makes the consequence significantly more destructive. PR3 uses The problem is that A builtin such as How to reproduceOpen the embedded terminal and run: echo $$ > .read-shell-pid
read -p 'waiting for input> ' valueDo not provide any input. Once the terminal visibly shows: use the browser context menu and choose Observed: The busy-terminal guard does not activate. The shell is simply terminated and replaced. finding-2-busy-builtin-agent-replacement.mp4Suggested directionI would not use the current Until PR2 has a reliable fresh-prompt signal, the conservative behavior would be: Longer term, safe replacement could require both: This is inherited from PR2, but PR3 amplifies the impact from "command injected in the wrong place" to "active session can be destroyed". 3. A real asynchronous spawn failure can kill the old shell and crash StrataThis turned out to be more serious than the initial code review suggested. The synchronous preflight catches things such as a missing executable, but a command can pass that validation and still fail when VTE actually tries to execute it. How to reproduceCreate an executable file whose interpreter does not exist: printf '%s\n' '#!/definitely/not/a/real/interpreter' 'echo impossible' > /tmp/strata-bad-agent
chmod +x /tmp/strata-bad-agentConfigure: as the AI agent command. Open a normal shell first and establish that it is alive. Then choose Observed: Strata terminated with The previous shell was already a zombie and no user-facing error dialog appeared. The recording ends with the application gone. As a control, starting the same invalid agent without an existing session did not crash Strata. It remained alive and only logged: internally. finding-3-agent-spawn-failure-loses-shell.mp4Suggested directionAgent replacement should behave transactionally. Conceptually: A failed spawn should result in: Ideally, the previous session should not be irreversibly destroyed before the replacement is known to have started successfully. At minimum, every asynchronous spawn failure must become a user-visible failure and return the panel to a valid lifecycle state. 4. The main agent process boundary still has no committed E2E testThis is not a runtime bug, but I think it should be addressed before this stack is considered ready. I checked the actual PR2 to PR3 delta: git diff --name-only <PR2> <PR3> | rg '^tests/e2e/'No E2E file is added or modified. I also searched the PR3 tree for agent-specific E2E coverage and found none. The existing The temporary review probe exercised exactly that boundary and was able to uncover the runtime failures above, but it was removed afterward and obviously does not count as committed regression coverage. Suggested directionI would keep a very small fixture agent in E2E that records: and cover at least: It does not need to be a large test suite. One real integration scenario would provide much more protection here than additional unit tests around argv construction. 5.
|
Add a provider-agnostic agent action on top of the embedded terminal. A
configurable command, empty by default, runs in the panel; Strata never
names, ships, or requires a particular agent, and the action stays
hidden until a command is set.
The folder background runs the agent in that folder. A selection runs it
in the folder that holds the selection. Where the selected paths land is
up to the command: a bare {} word becomes one argument per path, for a
tool that takes files, and {} inside a word substitutes them into that
word, for a tool that takes a single prompt. Without {} they follow the
command. They are process arguments throughout, so a name containing
spaces, quotes or shell metacharacters is never reinterpreted. The
program is resolved on PATH before spawning, because VTE spawns without
a path search.
An agent replaces the panel's session, but only when nothing is running
in it; a busy terminal reports that instead of losing the work. An agent
that exits leaves its output on screen with a line saying how it ended,
so one that fails at startup does not take its own error message away.
The command entry says whether what is typed has been saved, since
committing on Enter was otherwise invisible.
Refs lgse#93
cb23646 to
e3cbb90
Compare
|
Retargeted onto #1089 and rebuilt on top of it, now that #1093 is closed. The branch no longer carries the cwd synchronisation work, so the diff here is just the agent actions: one commit, 14 files. Nothing in the feature itself changed — the earlier follow-up comment still describes it accurately. The |
Selected paths went through a lossy conversion, so a native name that is not valid UTF-8 reached the agent as a different path and the launch still reported success. Paths and the command's own words are now only ever passed exactly; one that cannot be is reported and stops the launch. A configured command containing a slash resolved against Strata's own working directory, so ./agent next to the folder being browsed was reported as missing. Relative commands now resolve against the folder the agent will run in, absolute ones are used directly, and a bare name still comes from PATH. An agent no longer replaces a live session. Nothing here can tell a shell waiting at its prompt from one waiting inside read, so replacing one meant killing whatever it was doing; overlapping the old child's exit with the new spawn could also leave the panel with no session, or take Strata down with it. The action now asks for the session to be closed first, which removes the kill, the race and the crash together. A spawn that fails inside VTE arrives long after the menu has closed. It used to hide the panel, taking the agent's own error with it; it now leaves the panel open with the failure, and the terminal stays usable. Adds end-to-end coverage of the whole agent boundary: the background action's folder, a selection's arguments, the refusal, and a launch that fails late. Refs lgse#93
|
Thanks — this was a very useful pass. Findings 1, 2, 3, 4, 7 and 8 are fixed in a8afc8f. 6 is done. 5 I have kept, deliberately, and called out below. Note the stack changed under you while you were reviewing: #1093 is closed, and this branch has been rebuilt on top of #1089. The reasoning is on that PR, but the short version is your own conclusion — without a fresh-prompt signal, and with VTE 0.76 on the current base image, there is no sound way to inject into a live shell, so the synchronisation work is deferred to #1103. 1. Lossy native pathsFixed. Paths and the command's own words now go through 2, 3 and 8. ReplacementYou were right that this was the dangerous one, and the three findings share a root: replacement itself. Your suggested direction — require an explicit close — removes all three at once, so that is what it does now.
The idle predicate that authorised replacement is deleted, along with the rustix 3. Late spawn failuresAlso fixed on its own terms. A spawn that passes validation and then fails inside VTE used to hide the panel, taking the agent's own error off screen. It now leaves the panel open with the failure reported in it, and the panel stays usable — F4 twice gives a working shell. Covered end to end with a bad-interpreter agent. 4. E2E coverageAdded
Four scenarios, passing alongside the two from #1089. 5.
|
|
Correction on the video above: it was recorded before a8afc8f, so two points in its description no longer match the branch.
Everything else in the recording still holds. I will re-record the replacement section rather than leave the stale take standing. |
2 participants
Description
Stacked on
feat/93-embedded-terminal(#1089). Review the diff against that base, notmain.The feature #93 asked for: Open with AI agent. #1093, which used to sit between these two, is closed; the browser-to-terminal synchronisation it carried is deferred to #1103.
G_SPAWN_SEARCH_PATH. A bare name comes from PATH, an absolute path is used directly, and a relative one resolves against the folder the agent will run in.Two things worth calling out for review:
sparkles.{}inside a word substitutes the paths into that single argument, which expands the original one-path-per-argument contract from Explore an embedded terminal pane for AI agent actions #93. It is there becauseclaudeandcodextake a single prompt positional, so appending paths silently drops all but the first. Discussion in the review thread below.Visual evidence
Posted in a comment below, with a correction noted underneath it: the recording predates the review fixes in a8afc8f, so its session-replacement section no longer matches the branch.
How to test
You need a command to run. Anything works;
shis enough to see the arguments arrive.×. Select two or three files, right-click → Open with AI agent. It starts in their folder and receives each selected path as a separate argument.;. The names arrive intact.×and the action works.F4twice still gives a working shell../agentin the folder you are browsing and configure./agent. It resolves against that folder, not Strata's own working directory.Expected result: the agent starts in the right folder with one argument per selected path, replaces an idle session but never a busy one, is never written into by navigation, and reports every failure instead of silently doing nothing.
Related issue
Closes #93