Dor Tools: take over eligible terminals while preserving helpers - #514
Dor Tools: take over eligible terminals while preserving helpers#514nedtwigg wants to merge 11 commits into
Conversation
`dor tool` typed alone at a prompt now runs in that pane instead of splitting — same Surface, same id, same scrollback. Typing a command at a prompt is how a terminal works; the split placement stays for every other caller (an agent, a script, `--surface`, `--minimize`, a `--cwd` elsewhere). The handshake is the part that needed building: `dor` is the caller's foreground process when the host answers it, so the host answers `takeover` first, waits for the shell to report itself back at a prompt, and only then types the command. Waiting first deadlocks. The spawn lock is held past the response until the command is live, so a second invocation of the same key dedupes against a running tool rather than racing this one. The leaf changes kind through one meta write (`setMeta`), so the component pair and params commit together and the leaf id — the SessionId — never moves. dor-tool.md's word budget goes up 425 words: the take-over design moves out of `## Future` into a `## Take-over` section, plus its rationale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166PG9g7V3kZ6Uo9EHrpoTD
Deploying mouseterm with
|
| Latest commit: |
dc3706d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://968549b8.mouseterm.pages.dev |
| Branch Preview URL: | https://tool-takeover.mouseterm.pages.dev |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
One interaction worth a look before this lands, plus a smaller note.
A taken-over pane can become its own key match, and the adopted re-run is unreachable there. The spawn-time dedupe branch in surface.tool runs before the new take-over arm and probes const idle = getTerminalPaneState(match.id).currentCommand === null;. Before this PR the matched surface was always a pane other than the caller, so that read the tool's own state. After a take-over the caller is the tool: dor tool storybook typed again in that pane — the natural "the dev server exited, restart it" gesture, and now the pane the user is sitting in — matches itself through findSurfaceByParams(matchesToolKey) (lath.listPanes() includes the caller), and currentCommand is the in-flight dor tool storybook line, so idle is false. Output is existing surface:1 "pnpm storybook" and nothing re-runs. Only a keyed tool reaches this; an unkeyed one falls through to the gate, fails component !== 'terminal', and splits, which is fine.
Two shapes of fix, both one condition. Skip the key match when match.id === detail.surfaceId and let the take-over arm handle it — it re-writes the meta and types the command, which is an in-place adopt. Or treat the caller as idle by construction, since dor running from that pane's own shell means the tool command has already exited. The first is the safer of the two: restartSurfaceInPlace opens with platform.writePty(id, '\x03'), and firing that into the pane where dor is the foreground process would interrupt dor before it is answered — which is what the second shape would do on any caller whose currentCommand happens to read null.
Happy to push either one as a commit if you want it in this PR rather than a follow-up.
Smaller: of the five handshake invariants under Take-over, "a shell that never comes back to its prompt is left alone: nothing typed, leaf still a terminal" is the only one with no test behind it — TAKEOVER_PROMPT_TIMEOUT_MS makes it awkward to pin, but it is the handshake's equivalent of the gate conditions tool-takeover.test.ts enumerates one by one.
Reuse the shared command parser (`commandArgv0` + a new `primaryCommandTokens` export) instead of a second whitespace tokenizer that did not know about quoting, and classify the caller with `surfaceKindFromParams` rather than reading `leafMeta.component` — the codebase's one params-level kind switch. Extract the handshake as `takeOverPaneWithTool` next to its sibling `restartSurfaceInPlace`, read the caller's terminal state once, and drop the gate's `callerId` field (the call site proves it). The spawn lock now releases once the pane is the tool rather than once the command is live: the key reaches the leaf's params in that write, so the extra 15s wait only pinned a module-global lock — every other `dor tool` in the app queued behind it. `RESTART_INTERRUPT_TIMEOUT_MS` and the take-over's own prompt timeout were the same 15s wait on the same predicate, now one `PROMPT_RETURN_TIMEOUT_MS`. Trim the restatements: the handshake and one-meta-write rationale live in dor-tool.md, with pointers at the code; the help text keeps only what a user can act on. Tests poll instead of sleeping a fixed 250ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166PG9g7V3kZ6Uo9EHrpoTD
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
The /simplify pass took two things out that were load-bearing. Both are in the new commit, both are narrow, and I'm happy to push either fix as a commit if you'd rather not hand-roll them.
Four real ones, all created or sharpened by take-over: - A Session that announced an OSC 367 port under an earlier command handed that port — and that key — to the tool taking its pane over. The announcement is cleared as the leaf changes kind. - A keyed re-invocation from the tool's own pane, which take-over makes the normal place to retype, matched itself and reported `existing` forever: its `dor` is what the shell is running, so the tool read as live. It is idle by construction there, and now re-runs through the same handshake, reported `adopted` — never through `restartSurfaceInPlace`, whose Ctrl+C would kill the `dor` still waiting for the answer. - The gate's facts were read before a wait of up to 15s and never re-checked, so a pane minimized in that window became a Doored tool with a command typed into it. - Releasing the spawn lock at the meta write (the previous /simplify pass) was too early: a pane typed into but not yet reporting reads as an idle tool, which a queued same-key invocation interrupts and retypes. Held until the command is live again, with the reason recorded so it does not get re-simplified. Also documented as an accepted limit: a listener the taken-over shell already owned is in the port scan's process tree, so `port: auto` can frame it or refuse the pair. A split-spawned tool cannot hit it. Skipped: `cwdPathsEqual` rejecting an empty-string cwd where the old inline compare accepted it (no CwdState can hold one — `cwdFrom*` returns null), and folding case on the `tool` verb to match the launcher (stricli parses verbs case-sensitively; the asymmetry is correct, now stated). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166PG9g7V3kZ6Uo9EHrpoTD
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
The re-run arm closes the case it targets. Two things the new commit leaves open, both in the fall-through around it.
…a dead tool Both from review of the last commit. A keyed match on the calling pane that failed the gate fell through to a branch that could only report `existing` and do nothing: with the match being the caller, `idle` reads the live `dor tool` line. Reachable by `cd`-ing inside the tool's pane before retyping, or a compound line. The re-run gate now drops the conditions that only govern placement — the pane already is the tool, so it re-runs in its own directory like an `adopted` match from anywhere else — and a caller that genuinely cannot be typed behind gets an error naming why, not a survivor it is sitting in. A self-match whose command really is live (the tool spawned this `dor`) still reports `existing`. The lock's tail waited for the command to be *live*, which a tool that dies on boot never is between two 100ms samples — a typo'd `dor tool --` pinned the module-global lock for the full 15s with every other `dor tool` queued behind it. It now ends on either outcome, watching for a newly finished run by id rather than by command line, so a previous run of the same command cannot satisfy it early. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166PG9g7V3kZ6Uo9EHrpoTD
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Both fixes read correctly to me — the re-run gate dropping only the placement conditions is the right cut, and snapshotting lastCommand?.id rather than matching on the command line is a better version of what I suggested. One gap in what pins the second one, plus a contract note. The Windows-path decision on the other thread is fine by me: commandArgv0 already mangles those paths for the WATCHING keys and the header, so a second parser here would be the disagreement, not the fix.
The disjunct added last commit had no coverage: nothing in `Wall.test.tsx` emits `commandFinish`, so `lastCommand` stayed null and only `surfaceRunsCommand` ever satisfied the wait — the fixed sleeps were what released the lock. The re-run now starts and dies inside one sample, the shape the disjunct exists for, and the request after it fails on `settle`'s deadline if the lock is not released on the finished run (verified by removing the disjunct). Two 150ms sleeps go with it. `adopted` also means two things about timing now, so the CLI contract says which: off the caller's pane the restart is awaited before the status goes out, on its own pane the answer precedes the re-run, for the same reason `takeover` does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166PG9g7V3kZ6Uo9EHrpoTD
nedtwigg
changed the title
dor tool typed at a prompt runs in that pane
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
One cleanup in the new gate module. The take-over handshake itself reads correctly to me: the answer-then-wait ordering, the post-wait rechecks against store.has (tree membership, so a Door is caught) / isDying / isSurfaceClosing / cwd / kind / helper, and the caller-is-keyed-match branch routing through the same handshake rather than restartSurfaceInPlace all hold up against the paths I traced.
| /** The launcher names `dor/bin/` ships, lowercased. */ | ||
| const DOR_ARGV0 = new Set(['dor', 'dor.cmd']); |
There was a problem hiding this comment.
'dor.cmd' can never match. commandArgv0 ends in commandProgramName, which strips WINDOWS_EXECUTABLE_SUFFIX (/\.(?:exe|cmd|bat|com|ps1)$/i) off the basename — the comment at that constant says so outright: "commandArgv0 cannot produce one." So dor.cmd tool storybook reaches this set as dor, and the assertion that looks like it pins the entry (expect(isNakedToolInvocation('dor.cmd tool storybook')).toBe(true)) is passing through 'dor'.
Worth removing rather than leaving: as written it reads as "enumerate the launcher spellings here", so the next platform launcher gets added to a set that already handles it. The test assertion is still worth keeping — it pins the suffix-stripping, which is the thing actually doing the work.
| /** The launcher names `dor/bin/` ships, lowercased. */ | |
| const DOR_ARGV0 = new Set(['dor', 'dor.cmd']); | |
| /** `commandArgv0` strips a PATHEXT suffix before this reads it, so `dor.cmd` | |
| * and `dor.exe` both arrive as `dor` — only the bare name is matchable. */ | |
| const DOR_ARGV0 = new Set(['dor']); |
Collapsing it to argv0 === 'dor' and dropping the constant is equally fine if you'd rather not keep a one-element set.
2 participants
Typing
dor toolalone in an integrated terminal takes over that pane, preserving its Session, Surface handle, scrollback, and notes. Calls from agents/scripts, compound command lines, explicit placement, different directories, and terminals with an existing auxiliary helper use the split path. Pending approval and keyed reuse retain precedence.The host answers before waiting for the caller’s prompt, then rechecks visibility, closing state, directory, kind, and helper presence before transforming the leaf and typing. A helper opened during the handshake prevents takeover. Keyed reruns in the caller use the same handshake without interrupting the waiting
dor; the spawn lock releases on both live commands and commands that exit between polling samples.Current main and the repaired #493 are integrated through merge commits, including its shared Terminal Context presentation. The implemented takeover contract is promoted out of Future into the spec body; CLI help documents the helper fallback.
Validation:
pnpm testpassed, including 2,938 library tests.cargo check --lockedpassed with the repository-pinned standalone Node runtime.Depends on #493. Keep this draft for later landing; do not merge independently.