Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
The emulator now reports "ESC ] 133 ; <kind> ..." marks to its session instead of dropping them, and lets a caller tap the bytes it processes. The Terminal uses both to find where a plugin command's output starts and ends, and to record only that command's output.
f6fb1e5 to
15e0280
Compare
Conflicts in IdeTerminalServiceImpl(Test): port ADFA-6373's round-4 unload guard (closed flag) onto the LaunchedTerminalCommand model. runInTerminal checks it before launching and again after joining running/waiting, so cancelAll either sees the run or the run sees closed.
Addresses PR #2113 review F07 and F08. No behaviour change. - CommandIntentRouter.route called claim() then start() back to back on the main thread, so the Claimed and Cancelled states were unreachable. start(id, factory) now takes a queued command directly; State is Queued -> Running -> Ended. - Drop PluginSessionPool.owners/sessionsOf and TerminalCommandRequests.sessionsOf, which only tests used; the tests now check start/read/find instead.
Addresses PR #2113 review F01-F06. - F01: a session idle by its last finish mark is reused only if its shell holds the terminal's foreground (tpgid from /proc/<shell>/stat), or the runner that just finished still does (agent-run now puts cogo-pid in its marks). One where the user started vim, top or a REPL counts as busy instead of having the run line typed into it. - F02: agent-run is written only when missing or stale, as a temp file renamed over the old one, so a bash still reading it in another session is not cut short. - F03: a command interrupted by a cancelled caller or plugin unload has 5 s to exit after Ctrl-C, then its session is killed; the session stays on screen and the exit is reported, so it no longer holds a slot for good. - F04: stopSession finds the command by (plugin, session name) through TerminalCommandRequests, so it reaches commands of cancelled callers and of the plugin before a reload. - F05: a busy session pruned because it left the Terminal reports its command as exited, so a waiting caller no longer hangs. - F06: a session keeps the name it was opened with; a user rename no longer hides it from readSession/stopSession or reissues the name. Each new regression test was checked to fail with its fix reverted.
…eat/ADFA-6385-plugin-terminal-session-reuse
F09 - address commands by id, not session name. Sessions are reused, so
the session named in a Running result could later hold another command
of the same plugin: stopSession("P 1") could Ctrl-C another caller's
Gradle run, and readSession handed it that command's output. Running now
carries a commandId, and readSession/stopSession become
readCommand(commandId)/stopCommand(commandId, waitMillis). The last 8
exited results per plugin are kept, so a command's own result is still
readable after its session moves on. The API is unreleased (26.41).
F10 - end a command whose end mark never arrives (runner SIGKILLed, mark
swallowed by an unterminated escape sequence, or the typed run line taken
by a `read`/PS2 prompt), which otherwise kept its session busy for good.
Each look at a busy command checks, off the main thread, whether the
shell holds the terminal again and the runner is gone:
- runner reported its pid (the C mark carries cogo-pid): gone once that
pid is no longer a child of the shell; ended with exit code -1 only if
still gone 5 s later, so an end mark still in flight wins.
- typed run line no runner took after 5 s: the command file is taken
back and the command ended. The runner claims its file with a rename,
so exactly one side gets it and a withdrawn command never starts late.
A new session's command is never taken back; its profile may be loading.
ForegroundProcessGroup becomes ProcStat, which also reads the parent pid.
F11 - start a new session's first command from a login shell
(`bash -l -c`), so it sees the profile like every later command typed
into the session's interactive `bash -l`. Commands still inherit what the
user exports in the session's shell; the API doc now says so.
…eat/ADFA-6385-plugin-terminal-session-reuse Brings in the ADFA-6373 round-5 review fixes (876adce). Conflict in PluginTerminalLauncher.kt was imports only: kept HandlerCompat from this branch and the Lifecycle/LifecycleOwner imports for the new isStarted() guard, which applies cleanly to the reworked open(). PluginTerminalLauncherTest arrived written against the base's callback launch API; ported to the suspend launch() returning LaunchedTerminalCommand, with an AgentRunner on a temp dir. The backgrounded-IDE case still fails without the isStarted() guard.
…ks-and-terminal' into feat/ADFA-6385-plugin-terminal-session-reuse # Conflicts: # plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/services/IdeTerminalServiceImpl.kt
Daniel-ADFA
left a comment
Daniel-ADFA
left a comment
There was a problem hiding this comment.
Approving at 293dc4033, on top of Hal's approval at c56ac21a3. This is the top of the stack (#2110 <- #2113).
The merge of #2110's 1e12a585b kept the re-enable fix through this PR's rewrite: reopen() at IdeTerminalServiceImpl.kt:217, called from enablePlugin (PluginManager.kt:1136), pinned by reopenAfterCancelAllRunsAgain.
Hal's earlier findings (F01-F08 and the three IMPORTANT from the second round) are fixed at this head. The PR's unit tests pass locally: plugin-manager IdeTerminalServiceImplTest 32, termux-app terminal tests 97, termux-emulator ShellIntegrationTest 8. Nothing exercised on a device. A few non-blocking findings from my review pass follow separately, if they survive verification.
Daniel-ADFA
left a comment
Daniel-ADFA
left a comment
There was a problem hiding this comment.
Reviewed at 293dc4033 against ADFA-6385. My approval at this head stands; these findings are for this PR or a follow-up. Hal approved earlier, at c56ac21a3.
Stack: this is the top of #2110 <- #2113, so this head is the tip and claims were checked here. It includes #2110's 1e12a585b, and reopen() survived the merge.
Severity index
IMPORTANT
TerminalCommandRequests.kt:203- a new session whose runner never starts leaves its command Running forever
MINOR
TerminalActivity.kt:113- a second request before the service connects replaces the firstTerminalActivity.kt:147-/procread on the main thread underallowThreadDiskReadsAgentRunner.kt:97- the runner path depends on the profile's TMPDIRTerminalCommandRequests.kt:169- a killed session reports -9 where the API documents -1- PR description - names
readSession/stopSession(below)
NITPICK - 1, below
Findings without a diff anchor
MINOR: The PR description still describes readSession and stopSession, which c56ac21a3 replaced with readCommand(commandId) and stopCommand(commandId, waitMillis). QA working from it will look for methods that no longer exist. Update it to the current API.
NITPICK: The PR body opens with "## Description"; CLAUDE.md (Pull requests) asks for the Jira link as its first line.
Earlier rounds, checked at head
- F01, run line typed into a user's program: fixed, a session counts as idle only when its shell or last runner holds the foreground (
PluginSessionPool.kt:62,TerminalCommandRequests.kt:82). - F02, runner rewritten in place: fixed, written only when stale, through a temp file and a rename (
AgentRunner.kt:43-53). - F03, no guaranteed kill: fixed, Ctrl-C, then
killafter 5 s (PluginTerminalLauncher.kt:114-118,TerminalCommandRequests.kt:114-116). - F04, stop limited to this instance's commands: fixed,
stopCommandgoes through the launcher (IdeTerminalServiceImpl.kt:190). - F05, pruned sessions never reported: fixed,
prunereturns them and each is reported (TerminalCommandRequests.kt:78,:165-171). - F06, a rename hides the session: fixed, the name is kept from when it opened (
PluginSessionPool.kt:27-30). - F07, unreachable states: fixed, Queued -> Running -> Ended (
TerminalCommand.kt:74-84). - F08, test-only API: fixed,
ownersand bothsessionsOfare gone. - Session reuse confusing read and stop: fixed, by command id, with each plugin's last 8 exits kept (
TerminalCommandRequests.kt:146-153,:242-244). - Lost end mark: fixed for a typed run and for a runner that reported in (
TerminalCommandRequests.kt:179-223); a first run whose runner never starts is the IMPORTANT finding. - First command without the profile: fixed,
bash -l -c(AgentRunner.kt:77). - Re-enable cancelling every later run (#2110):
reopen()kept through the merge (IdeTerminalServiceImpl.kt:217,PluginManager.kt:1136).
Evidence
| Area | Result |
|---|---|
| Ticket | The acceptance criteria map to code and tests; readSession in them became readCommand/stopCommand by command id at Hal's request. |
| §1 Exceptions | Failures reach plugins as NotStarted, Running or Completed(-1); cancellation is rethrown after the interrupt. |
| §2 Leaks | Commands are interrupted, then killed, on cancel and unload; exited results are capped at 8 per plugin. |
| §3 Threading | /proc and file I/O go through io except the idle check (inline). |
| §4 Security | SYSTEM_COMMANDS on run, read and stop; reads and stops are limited to the plugin's own commands; the command reaches bash through a file, not the command line. |
| §5 Tests | Run locally, all passing: plugin-manager IdeTerminalServiceImplTest 32 at this head; termux-app terminal tests 97 and termux-emulator ShellIntegrationTest 8 at 19f4e2b03 (the merge since touched no termux files). Not run: app PluginTerminalLauncherTest. CI green. Nothing exercised on a device, and the large-screen intent ordering is from the flags, not observed. |
| §13 Plugins | runInTerminal gained waitMillis, Running, readCommand and stopCommand, all in the 26.41 changelog. AI-Core's shell tool (plugin-examples feat/ADFA-6339-agent-run-shell-command) is the caller. |
Checked and not reported: a null foreground group counts as at-prompt, but an open session's shell is same-uid and its /proc entry is readable, and dead shells are pruned first; a dead runner is noticed only on a later read, which then reports Completed(-1); cancelling during prepare leaves the command's files in Termux's $TMPDIR until its service stops; each byte of a running command is rendered in a second emulator, which is the design that keeps its output apart.
Verdict rule: REVIEW.md, CLAUDE.md and CONTRIBUTING.md have no written approve/request-changes rule. The approval was a deliberate decision with the IMPORTANT finding open.
| if (runnerPid != null) return if (processes.parentOf(runnerPid) == shellPid) Runner.ALIVE else Runner.GONE | ||
| // The runner has not reported in. Only a typed run line can have gone elsewhere, and only | ||
| // once it had time to start is that worth checking. | ||
| if (typedAt == null || now() - typedAt < GONE_GRACE_MS) return Runner.ALIVE |
There was a problem hiding this comment.
IMPORTANT: A command given to a new session whose runner never starts stays Running forever.
A new session runs bash -l -c "trap : INT; bash \"$TMPDIR/agent-run\" <id>; exec bash -l", and a login shell sources the user's profile before the -c string. If ~/.bash_profile ends in exec zsh (one way users switch to zsh), the runner never runs and no mark arrives. Each check then returns ALIVE here: runnerPid is null, and typedAt is null because the line was not typed. runInTerminal returns Running with no output, readCommand keeps saying Running, and after three commands every call is refused with AllSessionsBusy until the user closes those sessions.
Fix: give a new session's first command the same grace and runner.withdraw a typed line gets.
There was a problem hiding this comment.
@jatezzz please fix this one before merging. With a profile that execs another shell, every new session's first command stays Running forever, and after three of them the plugin is locked out with AllSessionsBusy until the user closes those sessions by hand.
Giving a new session's first command the same grace and runner.withdraw a typed line gets should cover it. Please add a test where the runner never reports in for a command that was not typed.
| val commandRequestId = CommandIntentRouter.shared.requestId(intent) | ||
| if (commandRequestId != null) { | ||
| val service = mTermuxService | ||
| if (service != null) runCommand(service, commandRequestId) else pendingCommandRequestId = commandRequestId |
There was a problem hiding this comment.
MINOR: A second command request that arrives before the Terminal's service connects replaces the first in pendingCommandRequestId.
On a large screen, applyMultiWindowFlags adds SINGLE_TOP, so a second runInTerminal while the Terminal is still binding arrives through onNewIntent and overwrites the id onCreate stored. The first command is never routed, and its caller gets NotStarted("The Terminal did not open") after 15 s although the Terminal opened. On a phone each request opens its own activity, so it does not happen there. The single slot comes from #2110 (line 109 there); session reuse makes back-to-back commands more likely.
Fix: keep the pending ids in a list and route each one in onServiceConnected.
| override fun isOpen(session: TerminalSession) = service.getIndexOfSession(session) >= 0 && session.isRunning | ||
|
|
||
| override fun foregroundProcessGroup(session: TerminalSession) = | ||
| allowThreadDiskReads("/proc is in memory, not on disk") { ProcStat().foregroundProcessGroup(session.pid) } |
There was a problem hiding this comment.
MINOR: The idle-session check reads /proc on the main thread and silences StrictMode with allowThreadDiskReads.
REVIEW.md §3: "Don't reach for allowThreadDiskReads() / permitAll() to silence a violation in our code." start runs on the main thread (CommandIntentRouter.route from runCommand), and slotFor calls this once per idle session of the plugin. The read is cheap, but the rule asks for the read to move rather than the policy to bend.
Fix: probe the idle sessions' foreground groups on io and pick the slot when the result posts back, as checkGone already does.
| } | ||
|
|
||
| /** Runs commands from Termux's `$TMPDIR`, which Termux clears when its service stops. */ | ||
| val termux = AgentRunner(File(TermuxConstants.TERMUX_TMP_PREFIX_DIR_PATH), "\$TMPDIR") |
There was a problem hiding this comment.
MINOR: The runner path is the literal $TMPDIR, which the session's shell expands after the profile, while the files go to the fixed TERMUX_TMP_PREFIX_DIR_PATH.
A profile that sets its own TMPDIR (export TMPDIR=$HOME/.tmp) makes bash "$TMPDIR/agent-run" fail with "No such file". A typed run then ends with -1 and no output after the 5 s grace, and a new session's first run never ends (the IMPORTANT finding at TerminalCommandRequests.kt:203). MINOR because Termux sets TMPDIR itself and few profiles change it.
Fix: pass the absolute directory.path as shellDirectory; the typed line gets longer but stops depending on the user's environment.
| val terminal = session.terminal | ||
| terminal.setShellIntegrationListener(null) | ||
| if (session.isIdle) return false | ||
| exited(session, if (terminal.isRunning) CommandMarkListener.UNKNOWN_EXIT_CODE else terminal.exitStatus) |
There was a problem hiding this comment.
MINOR: A command whose session dies is reported with the shell's exit status, not the -1 the API documents.
TerminalCommandResult.Completed says exitCode is -1 "when the Terminal could not tell it, e.g. the session closed". Here an ended session reports terminal.exitStatus, which is -9 after kill (the SIGKILL that follows an ignored Ctrl-C) or after the user kills the session. A plugin testing exitCode == -1 reads -9 as the command's own exit code.
Fix: report UNKNOWN_EXIT_CODE here, or document that a killed session reports the negated signal.
3 participants
Description
Implemented session reuse and command isolation for the Plugin Terminal API. Commands now run in up to 3 reused sessions per plugin via an
agent-run.shscript to prevent environment changes (likecdorexport) from carrying over to the next command. IntroducedTerminalCommandResult.Runningfor long-running commands, allowing plugins to resume work if a command does not finish within the wait time. AddedCommandRecorderandCommandMarkListenerto parse OSC 133 shell-integration marks, which ensures plugins only receive the exact, clean output of their specific commands instead of mixed session outputs. Additionally, addedreadSessionto check on running commands andstopSessionto gracefully interrupt commands using Ctrl-C.Details
Logic-related backend changes for terminal session pooling, intent routing, and byte stream tapping.
Screen_Recording_20261007_161740_Code.on.the.Go.mp4
Ticket
ADFA-6385
Observation
#2110 Must be merged first.