Skip to content

Dor Tools Phase B: the tool Surface — a terminal and a browser on one Session - #493

Open
nedtwigg wants to merge 47 commits into
mainfrom
phase-b
Open

Dor Tools Phase B: the tool Surface — a terminal and a browser on one Session#493
nedtwigg wants to merge 47 commits into
mainfrom
phase-b

Conversation

@nedtwigg

Copy link
Copy Markdown
Member

Implements Phase B of docs/specs/dor-tool.md: the tool Surface — one Session with a terminal and, once its command binds a port, a browser. Behind dormouse.flags.tools, off by default.

dor tool storybook            → pane runs `pnpm storybook`, grows an iframe on :6006
dor tool standalone-harness   → same, but ab-screencast, drivable via dor ab --surface

Both are declared in a new repo-root dormouse.yml and are the first two Dor Tools.

The design changed before the code did

The ledger this PR started from had dor open as Phase B and the OSC announcement as the thing that minted a tool. Walking pnpm storybook and pnpm dev:standalone:ab through as user stories moved four decisions; each is recorded in dor-tool.md with its evidence in the new dor-tool.rationale.md.

The atom moved ahead of dor open. dor open needs a glob table, viewer pages, and a loopback file endpoint, none of which the atom stands on. It is a parallel feature that shares a word.

The port scan is the trigger; OSC 367 only disambiguates. An announcement states intent, the scan states the result, and under parallel-worktree contention they diverge — storybook launched with -p 6006 in a second checkout lands on 6007, and vite under strictPort does not start at all. So a multi-port tool says which port to frame and the scan supplies the number; an announced port nothing bound frames nothing.

Identity is opt-in only. No key is derived from command+cwd. Command strings are spelling-unstable (pnpm storybook / pnpm run storybook / pnpm storybook --quiet are three keys for one tool, and an agent won't spell it the same way twice), dor ensure already is command+cwd idempotency, and declaring a tool to get a short name shouldn't silently switch deduping on. A tool has an identity if and only if prespawn_dedupe gave it one.

Repo-local config is inert until approved. dormouse.yml executes, so it is gated on a trust record keyed by repo root — granted only by a dialog in Dormouse's own chrome, because an agent holding the control token can dor send keystrokes indistinguishable from typing. Agents cannot grant trust. Path-level, never content-hashed: re-prompting on every edit and every git pull trains the user to click through it.

What's here

dor tool -- <cmd> / dor tool <name> designate a command as a tool, with or without an identity
dormouse.yml run, render, prespawn_dedupe with a closed substitution set
Trust per-repo-root, dialog-only, both answers remembered
Serving port scan grows the browser in place; command exit retires it
OSC 367 serve disambiguates the port, re-keys at runtime, registered in terminal-escapes.md
ab-screencast tools agent-drivable; dor ab --surface already reached them via the capability gate

ToolPanel keeps both halves mounted for the Surface's life and flips visibility — never display, which would leave the terminal measuring zero and let the fit addon resize the PTY under a running command.

Review passes

/simplify and a correctness review both ran as separate Opus agents. The review found 13 issues, 11 real, all fixed in 67bebcd7 with regression tests. Two were blockers I'd have shipped:

  • The Display modal destroyed a tool and orphaned its PTY. onSwapRenderMode had no tool branch, so it routed through replaceSurface: new id, terminal half and toolKey gone, old session never disposed. The spec states the opposite invariant; the code just didn't have it.
  • Dedupe keys were never namespaced, though a comment claimed they were. Two tools declaring scope-only keys collided, and an OSC re-key could name another tool's key — so a later dor tool storybook would adopt, Ctrl+C, and re-run the announcing pane.

/simplify also caught standalone/sidecar/tool-host.cjs landing as a tracked build artifact when its three siblings are gitignored.

Not built, and now stated as such

Two things the spec described as current behaviour have moved to ## Future rather than being overclaimed:

  • Pane take-over. dor tool typed alone at a prompt should run in that pane. The three gate conditions are readable today; what's missing is the handshake — dor is itself the foreground process when it answers, so the command can only be typed once its own shell returns to a prompt. Everything splits focus-neutrally for now.
  • The announced name is parsed, sanitized, and recorded, but nothing consumes it yet.

Verification

pnpm test green across every package; pnpm lint:specs OK. Budgets raised deliberately in-PR: dor-tool.md 2450→3000 (three new normative sections plus Source of truth: pointers), dor-cli.md 5900→5975, terminal-escapes.md 3950→4000, plus 1100 for the new rationale file.

One unrelated commit rides along: a9c59f2e splits AGENTS.md's word budget into prose + a per-index-line cap, so adding a spec no longer competes with the conventions prose for the same ceiling.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 31, 2026

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: a131c0e
Status: ✅  Deploy successful!
Preview URL: https://6d37524b.mouseterm.pages.dev
Branch Preview URL: https://phase-b.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big, careful change — the spec/rationale split, the namespaced dedupe keys, and the visibility-not-display flip in ToolPanel all land well. Four inline findings below, plus one that sits outside the diff.

The Door path bypasses persistableLeafMeta, so a minimized tool does persist its URL

persistableLeafMeta is applied only inside serializeLayout, and lathLayoutFromStore keeps only the leaves the tree places — a Doored leaf is deliberately excluded there and persisted as its own row instead. That row is built in use-session-persistence.ts (the doSave callback, const doors: PersistedDoor[] = (doorsRef.current ?? []).map(...)) from lath.getMeta(door.id) verbatim: component, tabComponent, and params: meta?.params. saveSession passes it straight through to doors:, and leafMetaFromPersistedDoor restores params: item.params unchanged.

So minimizing a serving tool before quitting round-trips url, session, wsPort, renderMode, and showTerminal — which docs/specs/dor-tool.md → Persistence and hosts states as "The URL is never persisted."

Concretely: minimize standalone-harness while it is serving, quit, relaunch. If the host's session restore gets the command running again before useToolServing's first tick, the tick takes the hasUrl || !running → continue branch and the stale url is never cleared, so the tool frames last run's port. If the command is not running, the retire branch clears url and showTerminal — but never renderMode, session, or wsPort, so an ab-screencast tool reattaches bound to a daemon session that died with the previous process.

The fix is one line at the door projection — run the meta through persistableLeafMeta before splitting it into the row:

const meta = lath.getMeta(door.id);
const persistable = meta ? persistableLeafMeta(meta) : undefined;
return {
  id: door.id,
  title: persistable?.title?.trim() || UNNAMED_PANEL_TITLE,
  component: persistable?.component,
  tabComponent: persistable?.tabComponent,
  params: persistable?.params,
  token: door.token,
};

use-session-persistence.ts isn't in this diff so I've left it out of the inline set — happy to push it with a regression test alongside the two persistableLeafMeta cases in tool-surface.test.ts if you want it in this PR.

Applying the help-text suggestions

The dor/src/commands/tool.ts and dor/test/snapshots/help/tool.md suggestions are the same sentence and have to be applied together, or cli-output.test.mjs goes red on the snapshot.

Comment thread lib/src/components/Wall.tsx
Comment thread dor/src/commands/tool.ts Outdated
Comment thread dor/test/snapshots/help/tool.md Outdated
Comment thread lib/src/host/tool-trust.ts Outdated
Comment thread lib/src/components/wall/use-dor-control.ts Outdated
Comment thread docs/specs/dor-tool.md Outdated
nedtwigg added a commit that referenced this pull request Aug 31, 2026
Seven findings from dormouse-bot, all confirmed.

- **A tool render swap leaked its agent-browser session.** The tool branch I
  added to `onSwapRenderMode` deliberately skips `replaceSurface` — but that is
  also where `closeAgentBrowserSession` + `disposeAgentBrowserSurfaceController`
  live, so clearing `session` from params left the daemon running and
  unreachable: `killPaneImmediately` reads the session back out of those params,
  so even killing the pane afterwards could not reap it. The branch now does the
  teardown itself.
- **A minimized tool still persisted its URL.** `persistableLeafMeta` was
  applied only in `serializeLayout`, and a Doored leaf is deliberately excluded
  from the tree snapshot and persisted as its own row built straight from
  `lath.getMeta`. Minimizing a serving tool before quitting round-tripped `url`,
  `session`, `wsPort` and `renderMode` — an `ab-screencast` tool reattached bound
  to a daemon that died with the previous process. The door projection now runs
  the same filter.
- **The size cap could not do what its comment claimed.** It measured after
  `readTextFile` had already loaded the file, so an untrusted repo's giant
  `dormouse.yml` was resident before the check — and above Node's string limit
  `readFile` throws, which the surrounding catch swallowed as "keep walking",
  silently skipping a file that exists. It now stats first, and the fallback
  check counts bytes rather than UTF-16 code units.
- `dor tool --help` still described pane take-over as current behaviour, which
  this PR moved to `## Future`; a user would have typed it at a prompt, got a
  background split, and concluded the gate misfired. Help and snapshot updated
  together.
- The spec's `dor tool <name> [args]` was above the fold while `validateToolArgs`
  rejects arguments.
- A doc comment for `toolCommandFromParams` had been orphaned above the spawn
  lock, where it read as a wrong description of it.

Full `pnpm test` green; spec-lint OK.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The four earlier findings are addressed and the teardown is safe in both directions — closeAgentBrowserSession and disposeAgentBrowserSurfaceController are no-ops when the tool is iframe-rendered, and a swap back re-creates the controller, whose ensureStarted clears the closedSessions mark for the same deterministic tool.<id> name, so the round-trip isn't poisoned. Two things left, both inline.

The bigger one is that the new tool-trust.test.ts block doesn't pin either half of the fix it names: revert readTextFile's default back to (path) => readFile(path, 'utf-8') and Buffer.byteLength back to text.length, and all three assertions still pass.

Comment thread lib/src/host/tool-trust.test.ts Outdated
Comment thread lib/src/host/tool-trust.test.ts
Comment thread lib/src/components/Wall.tsx

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The size-cap tests are load-bearing now — reverting the stat or the Buffer.byteLength turns each of them red, which the previous pair didn't. The pop-out gate is only half in place, though.

The ab-screencast half still offers pop-out

IframePanel is one of two places that register a screen. The other is AgentBrowserSurfaceController's ensureStarted, which registers canPopOut: !!getPlatform().agentBrowserPopOut with no tool gate. A tool declaring render: ab-screencast gets renderMode: 'ab-screencast' written by useToolServing, so BrowserPanel mounts AgentBrowserPanel — and ToolPaneHeader's own header states the consequence: "a tool's browser gets the same URL editor, nav buttons, and Display modal a plain browser Surface has."

So on standalone, pop-out on a serving render: ab-screencast tool is one click, not the two the earlier finding described — and it is destructive rather than inert. onSwapRenderMode's tool branch runs in full: closeAgentBrowserSession(params), disposeAgentBrowserSurfaceController(id), then updateParams writing toolRender: 'ab-screencast' (the mode it was already in) and clearing url / session / wsPort. The browser tears down and useToolServing re-derives it as the same screencast. The user asked for a native window and got a reload.

The controller already receives the surface's params — acquireAgentBrowserSurfaceController(id, { ...params, renderMode: seededMode }) in AgentBrowserPanel — so the same !isToolParams(...) gate fits there, and surfaceType never changes over a surface's life, so seeding it once in the constructor is safe. AgentBrowserSurfaceParams doesn't declare surfaceType, so it needs the field (or an explicit isTool flag) rather than riding the untyped spread.

Happy to push that together with the IframePanel.test.tsx assertion noted inline, if you want them in this PR.

Comment thread lib/src/components/wall/IframePanel.tsx
Comment thread lib/src/components/wall/tool-surface.test.ts Outdated
Comment thread lib/src/host/tool-trust.ts Outdated
Comment thread lib/src/host/tool-trust.test.ts Outdated
Comment thread docs/specs/dor-tool.md Outdated

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both registration sites are gated now, and all four gates are load-bearing — I reverted each one locally and the matching test went red: canPopOut in IframePanel.tsx, canPopOut in AgentBrowserSurfaceController.ensureStarted, the stat in readToolFile, and Buffer.byteLength. The two error-message wordings are distinguishable and only the reachable one is user-facing. tsc -b and the four touched suites pass. Two small things inline.

One note outside the diff: docs/specs/dor-browser.md -> "Display Modal And Render Swaps" still lists ab-popout as "Hidden if the host lacks agentBrowserPopOut", which now reads as the whole gate. dor-tool.md owns the tool rule, so at most a one-line pointer belongs there — but that file is at 4834 of its 4850-word budget, so I've left it as a note rather than a suggestion.

Comment thread lib/src/components/wall/agent-browser-surface-controller.ts Outdated
Comment thread lib/src/components/wall/ToolPaneHeader.tsx Outdated

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autobind is a good call and the refusal-over-tie-break stance matches surface.resolveOpen and the Dev-Server Chip, so the outlier is gone. The settle window is the right shape — the harness regression test earns its place, and listenerUrlsByPort sorting ascending is what makes the element-wise settle comparison deterministic, so it can't fail to converge. Five things inline, one of which is a real gap between the spec sentence and the code.

The one worth reading first: an announcement that arrives after the conflict has been committed never wins. dor-tool.md -> Serving states "An announcement, when present, wins over either" without qualification, and tool-registry.ts's ToolPortMode doc repeats it, but once toolPortConflict is written the tick short-circuits on hasConflict before it ever reads getToolAnnounce. With port: auto on a tool that binds two ports and emits OSC 367 more than ~3s after the second bind (two POLL_MS ticks), the pane sits on the conflict face permanently even though the tool said which port to frame. The lets an announcement override even in auto mode test only pins the early case — it calls recordToolAnnounce before mount — so the late path is unpinned. Details and the two-part fix are inline.

One reuse note that didn't earn an inline suggestion: ToolPaneHeader's canToggle re-derives toolFace's internals (browserUrlFromParams(...) !== null || toolPortConflictFromParams(...) !== null) rather than asking browser-surface.ts, which claims that classification as its own — "so the classification never drifts between them." It can't be toolFace(params) !== 'terminal', because the pinned-terminal case has to stay toggleable, so it wants its own exported predicate (toolHasSecondHalf, say) beside toolFace. Same shape as the isToolParams re-implementation flagged earlier in this PR, just milder: a fourth face would silently not reach the chip.

I could not run the suite — this runner has no pnpm — so the commit message's mutation-testing claims are CI's to confirm, not mine.

Comment thread lib/src/components/wall/use-tool-serving.ts Outdated
Comment thread lib/src/components/wall/ToolPortConflict.tsx Outdated
Comment thread lib/src/components/wall/use-tool-serving.ts Outdated
Comment thread docs/specs/dor-tool.md Outdated
Comment thread lib/src/components/wall/browser-surface.ts Outdated
Comment thread lib/src/components/wall/use-dor-control.ts Outdated
Comment thread lib/src/components/wall/use-dor-control.ts Outdated
Comment thread lib/src/components/wall/use-dor-control.ts Outdated

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad8006d landed while I was reading and I reviewed it too: the announcement override is right — the gate now falls through to the announce?.port != null branch and both framing writes clear the stale verdict, which toolFace's conflict-before-url order made necessary. All four open threads from the last pass are addressed.

The upstream key is the right identity and git-remote-url.ts is careful in the ways a security key has to be — lookalike hosts kept apart, userinfo dropped, unparseable input failing closed rather than guessing. The migration that drops v0 denials, and the test that pins the drop, are the right call.

But the headline feature does not work at runtime, and the test suite can't see it.

toolPending is written without cwd, so the pending face never renders

toolPendingFromParams requires five strings — ['name', 'run', 'path', 'projectRoot', 'cwd'] — and the object use-dor-control.ts writes has four: name, run, path, projectRoot, upstreamUrl. No cwd. So the validator returns null for every pending tool the product actually creates.

I replayed the validator against both shapes in node:

production params -> null
test fixture      -> ToolPending

The fixture in ToolPanel.test.tsx hand-writes cwd: '/repo', which is why all five new assertions pass over a shape production never produces. Nothing pins the producer against the consumer — use-dor-control.ts's untrusted branch has no test at all (grep -rn untrusted lib/src --include=*.test.ts finds only host-side hits).

What a user gets today, running dor tool storybook in an unapproved repo:

  1. toolPendingFromParamsnull, so toolFace falls through to 'terminal'.
  2. ToolPanel takes the two-halves path and mounts TerminalPanelgetOrCreateTerminalspawnPty in the repo's cwd. A shell starts in the unapproved repo — the thing docs/specs/dor-tool.md → Trust now states in bold as impossible ("no PTY is spawned, so not even a shell starts").
  3. The approval buttons never render, and resolveToolApproval bails at if (!pending) return;, so there is no way to grant from the pane. dor tool returns pending forever.

Findings are against bad8006d. Three more inline, one of which (--minimize) spawns a PTY in an unapproved repo even after the above is fixed.

How I verified

This runner has no pnpm and no node_modules, so I could not run the suite — the commit message's mutation-testing claims are CI's to confirm, not mine. Finding 1 I checked by copying toolPendingFromParams verbatim into a node script and calling it with the object literal from use-dor-control.ts:818 and the fixture from ToolPanel.test.tsx:124. The rest are read from the merged tree: getOrCreateTerminal at lib/src/lib/terminal-lifecycle.ts (getPlatform().spawnPty on the create path, pendingShellOpts.delete(id) before it), createSplitSurface's if (minimized) { getOrCreateTerminal(newId); … }, and the tool:control case in vscode-ext/src/message-router.ts, which maps a host-side rejection to a resolved { status: 'error' }.

Comment thread lib/src/components/wall/use-dor-control.ts Outdated
Comment thread lib/src/components/Wall.tsx Outdated
Comment thread lib/src/components/wall/use-dor-control.ts Outdated
Comment thread lib/src/host/tool-trust.ts
Comment thread lib/src/host/tool-host.ts

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three of the five open threads are addressed, and the cwd fix is the right shape — typing the producer's literal as ToolPending is what stops it drifting again. git-remote-url.ts's per-scheme default ports and the settle-memory reset are both correct and their tests are load-bearing (drop the ssh: entry from defaultPorts and the first new case goes red; drop if (!running) seenPorts.current.delete(leaf.id) and the new regression test goes red).

But the approval path still cannot start the tool.

Clearing toolPending before two awaits spawns a bare shell, and the command never runs

resolveToolApproval now writes { toolPending: undefined } first, then awaits the trust op and the re-lookup. That clear is exactly what flips toolFace to 'terminal'toolPending is checked before everything else in toolFace — so ToolPanel leaves its pending-approval early return and mounts TerminalPanel -> TerminalPane, whose mount effect calls getOrCreateTerminal(id) unconditionally. React flushes that render long before two host round-trips resolve.

getOrCreateTerminal reads pendingShellOpts (empty — deferTerminal staged nothing), spawns a PTY with the default shell and no cwd, and registers the entry. The setPendingShellOpts at the end of resolveToolApproval then lands on an id getOrCreateTerminal will never consult again, because it returns existing first. So clicking Always allow gives a bare shell in the default directory, titled storybook, with the tool's command never typed — the same "the registry entry already existed" failure the commit message describes fixing on the minimize path, relocated into the approval path itself.

The fix is to keep the clear in the same synchronous block as setPendingShellOpts, where bad8006d had it; two inline suggestions do that. The double-grant the current comment guards against is benign either way — both writes are idempotent, and a second setPendingShellOpts before any mount just overwrites the first. If you want a hard guard, a useRef<Set<string>> of in-flight ids is the place for it, not the param write.

Nothing pins this: resolveToolApproval has no test at all, and the five new ToolPanel assertions render the pending face without ever resolving it.

Separately, the comment above the callback still says "The two steps are ordered so a failed write never leaves a running command in an unapproved repo". The order is now trust-then-re-lookup, and it is the resolved?.status !== 'ok' bail that holds the invariant — a failed tool-trust.json write leaves the repo untrusted, so the lookup comes back untrusted and the pane is killed rather than run. Sound guard; the sentence describes the old one.

Three more inline, plus one spec note.

docs/specs/dor-tool.md -> Serving is now behind the code

The settle bullet — "since ports appear one at a time during boot and a framed leaf is never re-scanned" — and the matching rationale paragraph are no longer true: with an announcement present, a framed leaf is now re-scanned on every tick. The new rule (an announcement outranks an already-framed URL, not just a conflict) isn't stated in Serving either; the port field's "an announcement always wins over either" is about auto vs announced, not about re-pointing a live pane. Budget headroom is 46 words.

How I verified

This runner has no pnpm and no node_modules, so the commit message's mutation-testing and pnpm test claims are CI's to confirm, not mine — CI is green at e3be7ef. Everything above is read from the merged tree: toolFace's pending-first ordering in browser-surface.ts, ToolPanel's if (face === 'pending-approval') return <ToolApproval/> early return, TerminalPane's mount effect, and getOrCreateTerminal's const existing = registry.get(id); if (existing) return existing;. The doored-reference reachability is resolveSplitTarget -> resolveListedSurface -> buildDorSurfaceList(), whose own comment says "minimized ones included".

One thing not raised inline: useDorControl's main handler useCallback gained two revealSurface call sites in this PR, but its dependency array lists neither revealSurface nor enterTerminalMode (which revealSurface closes over). nav is listed, so it only goes stale if enterTerminalMode's identity changes without nav's — and there's no react-hooks/exhaustive-deps in the lint config to catch it.

Comment thread lib/src/components/Wall.tsx Outdated
Comment thread lib/src/components/Wall.tsx Outdated
Comment thread lib/src/components/wall/use-tool-serving.ts Outdated
Comment thread lib/src/components/wall/use-dor-control.ts
Comment thread docs/specs/dor-tool.md Outdated

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nine commits since the last pass, and the two headline fixes both land: resolveToolApproval now stages the shell before clearing toolPending, so the bare-PTY chain is closed, and the trust file is committed under a cross-process lock with the cache dropped — the new serializes concurrent grants test is load-bearing for both halves (remove the lock and the last write wins; restore the cache and the first.isTrusted([upstream]) assertion goes red). The readToolFile rewrite is a real improvement over stat-then-readFile: one descriptor, fstat on it, and a capped read that survives post-fstat growth.

Two things first, both about the environment rather than the code.

The branch conflicts with main (pnpm-lock.yaml), so no CI has run on 2e78b29. CI and Chromatic both went green on e3be7ef; on this head only Cloudflare Pages registered, because GitHub cannot compute the merge ref. That leaves the entire 9-commit increment — ~760 added lines across trust, persistence, and terminal lifecycle — unverified. I could not run the suite either (no pnpm on this runner), so everything below is read from the merged tree.

Five findings, all inline. The first two are the ones worth reading.

--minimize approval stages a command that never spawns

resolveToolApproval ends with setPendingShellOpts(id, …) then if (pending.minimized) minimizePane(id), all in one synchronous block. React never renders between them, so the leaf goes straight from the approval prompt to a Door without ever mounting TerminalPanel — and getOrCreateTerminal is what consumes the staged opts. createSplitSurface has exactly this hazard and handles it explicitly: both its minimize branches call getOrCreateTerminal(newId) before addDoor / minimizePane. The approval path doesn't.

So dor tool storybook --minimize in an unapproved repo shows the prompt (correct — rule 3), the user clicks Always allow, and the pane minimizes into a Door named storybook with no PTY, no command, and nothing running. It only starts if the user happens to reattach it. Nothing pins it: the new Wall.test.tsx approval test passes minimized: false.

Making a tool untouched unguards two destructive paths

untouched: leafMeta?.component === 'tool' is right about what untouched means, but three call sites read it and only one of them is kind-aware:

  • createContentSurface — gated: !hasBrowser(reference.kind) && isUntouched(reference.id), and hasBrowser('tool') is true, so a tool is safe here.
  • wallActions.onKillif (isUntouched(id)) killPaneImmediately(id), no gate. A tool serving pnpm storybook that you navigated to but never typed into is untouched (the command is typed through typeCommandWhenPromptReady, which bypasses xterm's onData, so it never marks the Session touched). Clicking the pane's close button kills it outright — the random-char confirm that any hand-started dev server gets is skipped.
  • The dormouse:new-terminal handler's shouldReplaceUntoucheddetail.replaceUntouched === true && selectedPaneVisible && isUntouched(selectedPaneId!), no gate either. selectShell dispatches with replaceUntouched: true, so picking a different shell while an untouched tool pane is selected runs replaceLeaf(toolId, newId, terminalLeafMeta()) + disposeSession(toolId). The running tool is replaced by a bare shell, and its agent-browser daemon session leaks — useToolServing's retirement branch needs the leaf to still exist to issue the close.

Command-mode navigation selects a pane without touching it, so both are reachable without doing anything unusual. Before this commit untouched: false made all three unreachable for tools. Either gate the two ungated readers on the Surface kind the way createContentSurface does, or keep tools touched and find another way to express "the user hasn't engaged with this yet".

Three smaller ones inline: the pending-reuse reveal didn't get the survivor re-lookup that 2e78b29 just added to the trusted-reuse branch beside it, the trust lock can spin forever on a stale record whose pid was recycled, and O_NOFOLLOW is undefined on Windows so the symlink refusal doesn't exist there.

Comment thread lib/src/components/Wall.tsx Outdated
Comment thread lib/src/components/Wall.tsx
Comment thread lib/src/components/wall/use-dor-control.ts Outdated
Comment thread lib/src/components/wall/use-dor-control.ts Outdated
Comment thread lib/src/host/tool-trust.ts Outdated
Comment thread lib/src/host/tool-trust.ts Outdated
nedtwigg added a commit that referenced this pull request Aug 31, 2026
Seven findings from dormouse-bot, all confirmed.

- **A tool render swap leaked its agent-browser session.** The tool branch I
  added to `onSwapRenderMode` deliberately skips `replaceSurface` — but that is
  also where `closeAgentBrowserSession` + `disposeAgentBrowserSurfaceController`
  live, so clearing `session` from params left the daemon running and
  unreachable: `killPaneImmediately` reads the session back out of those params,
  so even killing the pane afterwards could not reap it. The branch now does the
  teardown itself.
- **A minimized tool still persisted its URL.** `persistableLeafMeta` was
  applied only in `serializeLayout`, and a Doored leaf is deliberately excluded
  from the tree snapshot and persisted as its own row built straight from
  `lath.getMeta`. Minimizing a serving tool before quitting round-tripped `url`,
  `session`, `wsPort` and `renderMode` — an `ab-screencast` tool reattached bound
  to a daemon that died with the previous process. The door projection now runs
  the same filter.
- **The size cap could not do what its comment claimed.** It measured after
  `readTextFile` had already loaded the file, so an untrusted repo's giant
  `dormouse.yml` was resident before the check — and above Node's string limit
  `readFile` throws, which the surrounding catch swallowed as "keep walking",
  silently skipping a file that exists. It now stats first, and the fallback
  check counts bytes rather than UTF-16 code units.
- `dor tool --help` still described pane take-over as current behaviour, which
  this PR moved to `## Future`; a user would have typed it at a prompt, got a
  background split, and concluded the gate misfired. Help and snapshot updated
  together.
- The spec's `dor tool <name> [args]` was above the fold while `validateToolArgs`
  rejects arguments.
- A doc comment for `toolCommandFromParams` had been orphaned above the spawn
  lock, where it read as a wrong description of it.

Full `pnpm test` green; spec-lint OK.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force-pushed since the last pass, so I read 398bba9 fresh; the rebase carried the tool code across unchanged, and the nine commits on top are the five findings from 2e78b29 plus the URL-bar-navigation fix. CI is green at this head (Build & Test, both platform checks, Standalone Smoketest, Webview Boot Smoketest, Chromatic, Cloudflare), so the conflict that left the last increment unverified is gone.

The two headline fixes land. resolveToolApproval now calls getOrCreateTerminal(id) before minimizePane(id), closing the --minimize no-PTY path for the same reason createSplitSurface spawns before addDoor, and the new Wall.test.tsx case is load-bearing for it. appliedAnnouncedPorts is the right shape for the announcement gate — keying "already applied" on the port the poll last acted on rather than on params.url is what lets URL-bar navigation survive, and the retirement branch drops it alongside seenPorts so a re-run re-points from scratch. Resolving the upstream host-side instead of trusting request.upstreamUrl off the wire is the correct boundary, and it degrades to a folder grant — narrower, not wider — when git answers nothing.

Three things, all inline. The first is the one worth reading.

The keyboard kill path was not gated, and two spec sentences now say it was

2e8cfd02 gated wallActions.onKill and shouldReplaceUntouched, which is what the previous review enumerated — but that enumeration was incomplete. handlePaneShortcuts in lib/src/components/wall/keyboard/handle-pane-shortcuts.ts is a fourth reader of isUntouched, and it owns the x/k binding that docs/specs/layout.md names first: if (isUntouched(sid)) { ctx.killPaneImmediately(sid); return; } for a pane, and afterRestore: isUntouched(sid) ? 'kill-immediately' : 'confirm-kill' for a Door. Neither consults the Surface kind, so both hazards the commit describes closing are still open through the keyboard.

Reachable exactly as the commit message describes: command-mode navigation selects a pane without touching it, and a tool's command is typed by typeCommandWhenPromptReady, which calls getPlatform().writePty(id, ...) directly rather than going through xterm's onData, so a tool serving pnpm storybook stays untouched: true for its whole life. Select it, press x, and it dies with no random-char confirm — and for an ab-screencast tool the daemon session leaks, since useToolServing's retirement branch needs the leaf to still exist to issue the close. The click path is pinned by the new requires confirmation before killing an untouched tool test; the keyboard path has no equivalent, and handle-pane-shortcuts.test.ts is where one would go.

ctx.nav.paneParams(sid) is already on WallKeyboardCtx, documented as the surface-type classification seam, so the pane branch gates with one clause. The Door branch has no meta accessor on ctx — either add one, or pass 'confirm-kill' unconditionally for a tool Door, since after restore the leaf is an ordinary pane again. That file is outside this diff, so I have left no suggestion on it; happy to push both gates plus the handle-pane-shortcuts.test.ts cases if you want them in this PR.

Comment thread docs/specs/layout.md
Comment thread lib/src/lib/platform/tool-types.ts Outdated
Comment thread lib/src/components/Wall.tsx Outdated
Comment thread lib/src/host/tool-trust.ts Outdated

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All five open threads are addressed. The keyboard gate is the right shape — ctx.nav.paneParams reads the store, which holds a Door's params too (the sibling handle-mouse-selection-keys.ts already relies on that), so the Door branch needs no new accessor, and both new handle-pane-shortcuts.test.ts cases are load-bearing. isUntouched now has four readers and every one is kind-gated. Dropping upstreamUrl from ToolHostRequest is scoped correctly: the field stays on the response, which is what ToolApproval renders, so only the untrusted inbound direction is gone.

The bakery lock is sound as a mutual-exclusion algorithm — unique participant paths mean no waiter can unlink a pathname a newer owner would reuse, which is the race the last pass flagged; the choosing marker closes the read-max window, and same-ticket ties break on token, so the order is total. merges concurrent grants after reclaiming one aged lock is load-bearing for it (drop the lock and the eight grants collapse to one key), and the orphan-reclaim test still hangs without the pid/mtime reap.

Two things inline. The lock one is the one worth reading.

Two spec sentences the keyboard fix inverted

61b35679 made docs/specs/layout.md -> "Kill confirmation" true, but two quick-reference lines that state the same rule went the other way — both are outside this diff's hunks, so no suggestion on them:

  • docs/specs/layout.md, the **x** / **k** (command mode) bullet under Doors: "an untouched Surface is killed outright". That parenthetical describes the exact Door branch this commit changed, and "Surface" is the glossary term that now includes tools. an untouched plain terminal is killed outright matches the section it links to. Budget headroom is 30 words.
  • docs/specs/shortcuts.md, the k or x row: "untouched (never-typed-in) panes and doors are killed immediately without the prompt". That file's own header says to keep the table in sync when a binding changes; headroom is 29 words.

Happy to push both plus the two inline fixes if you want them in this PR.

How I verified

This runner has no pnpm, so the commit messages' test claims are CI's to confirm, not mine. pnpm lint:specs I did run (node scripts/spec-lint.mjs → OK), and word counts are wc -w against scripts/spec-word-budgets.json.

The mkdir finding is a direct repro, not a reading: fs.mkdirSync(existingFilePath, { recursive: true }) throws EEXIST on this Node. The rest is read from the merged tree — nav.paneParams as lath.getMeta(id)?.params in Wall.tsx, getMeta as snapshot().leafMeta.get(id) in lath-wall-engine.ts, and use-session-persistence.ts calling lath.getMeta(door.id) as the standing proof that Doored leaves keep their meta.

Two candidates I dropped at verify rather than raising: #hasEarlierParticipant runs #reapIfStale on the caller's own ticket file before the value.token === token self-check, but the same event loop that runs the wait loop runs the heartbeat, so a self-reap needs utimes to fail silently — at which point the holder's lease has collapsed anyway. And the bakery is a lot of machinery for a file written on a human click, but I could not name a simpler primitive that actually fixes the reclaim race, so it is not a finding.

Comment thread lib/src/host/tool-trust.ts Outdated
Comment thread lib/src/components/wall/keyboard/handle-pane-shortcuts.ts Outdated
dormouse-bot
dormouse-bot previously approved these changes Aug 31, 2026
nedtwigg and others added 12 commits August 31, 2026 16:07
…x-line cap

A single whole-file ceiling made a new spec's index line compete with the
Specs/Spec lifecycle prose, and the index always lost — it is the easier thing
to shave, and shaving it makes routing lines vaguer rather than shorter. A
budget may now be a number (whole file) or {prose, indexLine}.

Authored in the working tree during the dor-tool design session; committed
separately so it is not buried under an unrelated docs message.
…erving, repo trust

Rewrites everything below the fold, and splits the evidence into a paired
rationale file per the new house form.

- Ledger re-cut to B1 (the atom) / B2 (OSC 367) / B3 (ab-* rendering) / C
  (glob table + `dor open`). The atom moves ahead of `dor open`, whose viewer
  pages, glob table, and loopback file endpoint are a parallel feature rather
  than the layer the atom stands on.
- Identity is opt-in only, via `dormouse.yml`'s `prespawn_dedupe`. No key is
  derived from command+cwd: command strings are spelling-unstable, `dor ensure`
  already is command+cwd idempotency, and declaring a tool to get a short name
  should not silently switch deduping on.
- Dedupe at spawn time only; a runtime re-key re-labels and never kills.
- The port scan is the primary serving trigger and OSC 367 the disambiguator.
  Under parallel-worktree contention an announcement states intent while the
  scan states what actually bound — storybook drifts to 6007, vite under
  strictPort does not start at all.
- New Trust section: repo-local `dormouse.yml` is inert until a gesture in
  Dormouse's own chrome approves the repo root. Path-level, never hashed.
- `dor tool` is no longer routed to the VS Code editor: a verb returning a
  handle on one host and a note on another is one command with two types.

dor-tool.md's budget rises 2450 -> 2800 for three new normative sections, plus
1100 for the new rationale file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
The pure half of the tool atom (docs/specs/dor-tool.md -> Declaring tools,
Identity and dedupe, Trust). Node-side because the spec makes the registry
host-resolved: one source of truth for `dor tool` and any later GUI gesture,
and a caller cannot hand the host a command while claiming the file authorized
it. The yaml dependency stays in the host bundle.

- `tool-registry.ts` parses entries, validates the closed substitution set, and
  renders keys. An unrecognized `$NAME` is a parse error rather than a literal:
  a `$PROJECTROOT` typo kept as a constant string dedupes across every worktree
  on the machine and kills one of them. An unknown `prespawn_*` is an error too
  — silently dropping a dedupe directive is the destructive failure, where
  failing to parse is loud. A repo-local key with no `$PROJECT_ROOT` warns.
- `tool-trust.ts` walks up for the nearest `dormouse.yml` (its directory is
  `$PROJECT_ROOT`, free and git-independent) and records per-root decisions.
  Denials are remembered so a hostile repo cannot re-ask every invocation; a
  corrupt store starts empty rather than failing every tool. Granting is
  deliberately absent — only Dormouse's own chrome may grant.

Parsing precedes the trust check because parsing is inert and the approval
dialog has to name the command it is approving.

33 tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
The CLI half of the atom (docs/specs/dor-tool.md -> CLI). Two forms:
`dor tool <name>` runs a dormouse.yml entry, `dor tool -- <command>`
designates any command as a tool with no key at all.

- `tool` joins KIND_CAPABILITIES with both capabilities, so `--kind` parsing
  and its help placeholder pick it up from the one table.
- `surface.tool` on the wire. The CLI never reads dormouse.yml: resolution is
  host-side so a caller cannot hand the host a command while claiming the file
  authorized it.
- A named tool takes no extra arguments yet. Argument passing waits for phase
  C's substitution, which is where args have to reach the dedupe key —
  accepting them now would key a per-target tool on its name alone and
  collapse every target into one pane.
- dormouse.yml lint output goes to stderr so `--json` stays parseable.

Where the tool lands is decided host-side rather than here: the host already
knows the calling pane's OSC 633 command line, which is the signal the spec
names, and a CLI-side flag would be forgeable by `dor send`.

136 tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
…ability pane

Everything a `tool` Surface needs to exist and render, short of the control
handler that creates one.

- `toolControl` on PlatformAdapter: one method, two ops (resolve a name,
  record a trust decision), plumbed through both hosts — VS Code extension
  host, Tauri/Rust/sidecar, and the dev:standalone:ab bridge. Absent on hosts
  with no filesystem, where `dor tool -- <command>` still works.
- `PersistedSurfaceType` gains 'tool'. The compiler caught the persistence
  seam on its own; `surfaceKindFromParams` is the one it cannot force, and now
  classifies tools ahead of the browser test — a serving tool also carries a
  renderMode, so order matters.
- `ToolPanel` keeps the terminal and the browser both mounted for the
  Surface's whole life and flips visibility. Unmounting the terminal would
  drop the xterm buffer the command is still writing to; unmounting the
  browser would reload the framed document on every toggle. That invariant is
  what lets a tool keep one id while its capabilities come and go.
- `ToolPaneHeader` delegates to the header for whichever half is forward, so a
  tool's browser gets the same URL editor, nav, and Display modal a plain
  browser Surface has, behind a leading toggle chip.

lib typecheck clean; 1833 lib tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
…t dialog

The atom works end to end: `dor tool storybook` spawns a tool Surface, the
port scan grows its browser in place when the command serves, and the pane
flips back to a prompt when it exits.

- The `surface.tool` control handler resolves a name through the host (never
  the CLI), dedupes at spawn time only and only on a declared key, and creates
  a tool leaf through the same shell-hosted spawn path `dor ensure` uses —
  without its command+cwd matching.
- `useToolServing` polls only tool-designated Sessions. An ordinary terminal
  that opens a port never transforms; that stays the Dev-Server Chip's job.
  Lowest port wins for now — B2's OSC 367 is the disambiguator for multi-port
  tools, and the header's URL editor is the escape hatch until then.
- `ToolTrustDialog` is the only way trust is granted, drawn in Dormouse's own
  chrome. A prompt rendered in the terminal would be answerable by `dor send`,
  i.e. by the very caller it gates. Escape and the focused button both decline,
  so a reflexive Enter never approves unread code.
- `dormouse.yml` ships `storybook` and `standalone-harness`, both scoped with
  `$PROJECT_ROOT` so parallel worktrees are separate tools. A test pins the
  file against the parser.

The new classifier test caught the exact seam the spec flags as
compiler-unforceable: `surfaceKindFromParams` had not learned `tool`, so a
serving tool would have classified as a plain browser.

lib typecheck clean; 1848 tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
B2. A tool that binds several ports says which one to frame; the scan still
supplies the number, so an announced port nothing bound frames nothing. That
ordering is the whole point — under parallel-worktree contention the
announcement states intent and the scan states the result.

- `tool-announce.ts` parses only the `serve` verb. `dehydrate` is D2's and is
  consumed rather than half-honored. Payload is size-capped before JSON.parse,
  ports range-checked, keys capped in length and element count.
- `osc-sanitize.ts` lifts the shared sanitizer out of terminal-protocol so
  OSC 367 answers to exactly the rules OSC 9/99/777 do, rather than growing a
  second one.
- The sequence is stripped whether or not it parses: a malformed announcement
  must not print itself into the user's scrollback.
- `tool-announce-store.ts` records per Session. A module store rather than
  adapter plumbing, because every adapter already funnels PTY data through
  `applyTerminalProtocolEvents`. Recording is not acting: an ordinary
  terminal's announcement lands there and does nothing.
- A runtime re-key re-labels its own Surface and never dedupes. Once a key can
  change, both Surfaces may hold work, and resolving a collision by killing
  either destroys some of it.

Registered in terminal-escapes.md; its budget rises 3950 -> 4000 for the two
rows. No replay filter: 367 elicits no response, and replaying the hint after a
reconnect is what restores it.

1863 lib tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
…rowser

B3. `dor ab --surface surface:N` already reaches a tool — that falls out of
the capability gate, since `tool` has a browser. What was missing was a tool
whose browser is a real one rather than an iframe.

- `render: iframe | ab-screencast` in dormouse.yml. The repo declares it, not
  the tool: which renderer suits a tool is a Dormouse-side judgement, and the
  harness case in particular is nothing storybook could know about itself.
- On serving, an ab-rendered tool binds an agent-browser session to its *own*
  Surface rather than creating a second one. A tool's browser is a param of its
  own leaf, which is what keeps its id stable while its capabilities come and
  go. The URL lands first so the panel shows the destination while the daemon
  boots, then the session arrives — the same ordering connect-port uses to keep
  the controller from racing the boot.
- `standalone-harness` is the ab-rendered example and announces its vite port,
  because it binds three and no scan can guess. Its escape sequence is pinned
  by a test: invisible in a terminal, so a framing typo would fail silently —
  the harness would keep working and Dormouse would frame the wrong port.

Full `pnpm test` green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
…flags.tools

Promotion is part of done: B1/B2/B3 move above `## Future`, rewritten in
present tense with `Source of truth:` pointers, and their ledger bullets are
deleted. What remains staged is C (the glob table and `dor open`), D1, D2, and
Later.

Two gaps this pass closed rather than documented around:

- The flag the ledger promised did not exist. `dormouse.flags.tools` is now
  real and enforced at the `surface.tool` handler — off by default, so nothing
  is designated a tool, the serving trigger has nothing to watch, and no pane
  can transform.
- A keyed match whose command had exited was revealed but never re-run, so
  `dor tool storybook` after a crash surfaced a dead pane. It now re-runs in
  place, keeping position and scrollback, and reports `adopted`. That was one
  of the spec's two open questions; the other (where the renderer choice
  lives) was answered by shipping it as a `dormouse.yml` field.

`dor-cli.md` gains `dor tool` in its command section. Budgets raised
deliberately: dor-tool.md 2800 -> 2950 for the pointers, dor-cli.md 5900 ->
5975.

Full `pnpm test` green; spec-lint OK.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
`hidden` sets display:none, which gives the container no box — the fit addon
then measures zero and resizes the PTY to a degenerate size, reflowing the
output of the command still running behind the browser. Both halves are
absolutely positioned over the same area, so `visibility` keeps each measuring
the pane's real dimensions whichever one is forward.

`inert` + `aria-hidden` keep the hidden half out of the tab order and the
accessibility tree, which `display: none` had been doing for free.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
Quality-only pass over the Dor Tools Phase B commits. No behaviour changes.

Reuse / dead code
- .gitignore: stop tracking standalone/sidecar/tool-host.cjs. It is a build
  artifact of scripts/build-sidecar-proxy.mjs, and its three siblings
  (iframe-proxy / agent-browser-host / remote-host) are all ignored. -7607 lines.
- connect-port.ts: extract attachAgentBrowserSession — the open + stream-status
  + one-params-write tail — and call it from both connectPortToDefaultBrowser
  and use-tool-serving.ts, which had reimplemented it.
- browser-surface.ts: one exported toolKeysEqual replaces the three copies
  (use-dor-control.ts, use-tool-serving.ts, and the never-called
  dedupeKeysEqual in host/tool-registry.ts, now deleted). Its cases move to
  tool-surface.test.ts; the registry test asserts key inequality directly.
- ToolPaneHeader / use-tool-serving: read the url through the existing
  browserUrlFromParams instead of an inline cast.
- feature-flags.ts: setWorkspacesEnabled/setToolsEnabled share a writeBoolFlag,
  mirroring readBoolFlag.
- tool-host.ts: drop the unused type re-export, and the impossible
  "vanished during resolution" branch — lookupTool now returns the ToolEntry it
  already found instead of the caller re-looking it up by name.
- tool-trust.ts: findToolFile had two loop terminators (`dir === root` and
  `parent === dir`); keep the latter, dropping the parsePath import.

Correctness of documentation
- use-dor-control.ts: waitForTerminalState's JSDoc had been orphaned above
  toolKeysEqual; move it back.
- AGENTS.md: the dor-tool index line said "Only the capability gating is
  implemented" twice, and both were stale — the tool Surface ships behind
  dormouse.flags.tools.
- tool-registry.ts header pointed at a tool-lookup.ts that does not exist
  (it is tool-trust.ts).

Prose
- Trim restatements of rules the spec/rationale already own: the scan-vs-
  announce split (tool-announce.ts, use-tool-serving.ts, dev-agent-browser.mjs),
  "recording is not acting" (terminal-protocol.ts), the trust ceremony
  (use-dor-control.ts), path-level trust (tool-trust.ts), and the both-halves-
  mounted invariant (ToolPanel.tsx, browser-surface.ts) — each now stated once,
  at the code it constrains.
- use-dor-control.ts: fold the trust-recorded pre-check into the status switch.
- dev-agent-browser-announce.test.mjs: the framing assertions re-tested strings
  the test itself had just built; keep the one check that reads the harness
  source, and drop the tautologies.

Deliberately skipped: renderToolResponse duplicates renderEnsureResponse almost
exactly, but the shared shape is ~12 lines and unifying it would need a helper
general over two JSON payloads — noise for the size.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
…r swap, derived state

Eleven confirmed findings from the review pass, verified against the code
before fixing.

Blockers:
- **The Display modal destroyed a tool.** `onSwapRenderMode` had no tool branch,
  so swapping renderer on a serving tool routed through `replaceSurface`: new
  id, leaf swapped to a plain browser, terminal half and `toolKey` gone, and the
  old session never disposed — leaving the command running with no pane and no
  door. It now mutates `toolRender` in place and drops the derived browser so
  the serving trigger re-derives it, which is the invariant the spec states.
- **Dedupe keys were never namespaced.** The doc comment claimed the host
  namespaced them; nothing did. Two tools in one repo declaring scope-only keys
  (`[$PROJECT_ROOT]`) collided, and an OSC 367 re-key could name another tool's
  key — so a later `dor tool <that tool>` would adopt, Ctrl+C, and re-run the
  announcing pane. `namespacedToolKey` prefixes the host-resolved tool name at
  both mint and re-key; an identityless tool namespaces to null, so a re-key
  cannot mint an identity it was never given.

Also fixed:
- A tool's `url` / `session` / `wsPort` / `renderMode` were persisted verbatim,
  so a cold restart framed a dead address. `persistableLeafMeta` strips them at
  the serialization boundary, per the spec's `Reserved:` line.
- `clearToolAnnounce` had no callers: a stale port hint outlived its command and
  permanently stopped a tool from ever growing a browser again.
- The `adopted` re-run waited on the *caller's* cwd while `surfaceRunsCommand`
  compares the Surface's own, so running `dor tool` from a subdirectory timed
  out after 15s and reported failure on a tool that had in fact restarted.
- A key match never revealed the survivor, so matching a minimized tool did
  nothing visible.
- Concurrent keyed spawns are serialized on a promise chain; two of them
  previously both cleared the key check and both created.
- Killing an ab-rendered tool mid-launch orphaned an agent-browser daemon with
  no teardown path.
- Overlapping ticks in the serving loop could double-issue `agent-browser open`.
- A Doored tool persisted as `'terminal'`.
- The untrusted `dormouse.yml` read — which happens *before* the trust check, so
  the dialog can name the command — is capped at 256KB; it could OOM the host.
- `dor list --kind`'s custom usage is derived from `SURFACE_KINDS`, so it cannot
  drift again (the snapshot test caught it).

Spec corrected rather than overclaimed: pane take-over and the announced `name`
are parsed/designed but not built, and both move to `## Future`.

Full `pnpm test` green; spec-lint OK.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XDByKnEGLaNj45bNZjzjeF
@dormouse-bot

Copy link
Copy Markdown
Collaborator

One caveat on the approval I just posted: the branch conflicts with main on pnpm-lock.yaml, so GitHub cannot compute the merge ref and no CI workflow ran on a97cef6 — only Cloudflare Pages registered. The approval is a read of the code, not a CI-verified one. Same condition as the 2e78b29 pass; a rebase onto main clears it.

How I checked

gh pr view 493 --json mergeable,mergeStateStatus returns CONFLICTING / DIRTY, and actions/runs?head_sha=a97cef6 lists only this review's own tend-review run. Merging origin/main into the head locally conflicts in pnpm-lock.yaml and nothing else.

This runner has no pnpm and no node_modules, so I could not run the suite either — the three new commits' test claims are unverified by anything at this head.

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recovering a review that never posted. The tend-review session at this head (33450294913) ran its full pass, computed three ranked findings, and then ended its turn without submitting them — so a131c0e1 has carried no review since 23:29Z yesterday while the standing APPROVED below sits on a97cef63, one commit earlier. The daily review-runs sweep found the abort in that session's log. I re-verified findings 2 and 3 against a131c0e1 from scratch before posting; finding 1's discarded-result mechanism I confirmed, its failure window I did not reproduce.

1. lib/src/components/Wall.tsx:1226 — the trust grant's result is discarded, so a slow grant kills the approved paneplausible, not reproduced

resolveToolApproval awaits toolControl({ op: 'trust', … }) and throws the answer away, resting the whole verdict on the immediate re-lookup; a non-ok answer runs killPaneImmediately(id) with no message. The two sides of that race have mismatched budgets: tool_control gives the op 5 s (Duration::from_secs(5), standalone/src-tauri/src/lib.rs:532) while FileToolTrustStore.grant can block on the bakery lock for the full TRUST_LOCK_STALE_MS = 30_000 (lib/src/host/tool-trust.ts:75) behind a participant it cannot reap. If the grant is still blocked when Rust times out, the discarded error is invisible and the re-lookup — which takes no lock — reads a trust file the grant has not committed yet, returns untrusted, and the pane the user just approved vanishes. Checking trusted?.status === 'trust-recorded' and surfacing an error would close it without touching either constant. resolveToolApproval has no test covering a failed grant.

2. lib/src/lib/session-save.ts:80PersistedPane.tool is written and validated but never readconfirmed

toolMetadataFromParams builds it, saveSession writes it, isPersistedToolMetadataShape validates it on read (lib/src/lib/session-types.ts:151) — and the only .tool read in the tree is the write-side ?? previousPane?.tool fallback on that same line 80. restoreSession consumes pane.command and pane.surfaceType only; the tool params that actually come back (toolName, toolRender, toolPort, toolKey) ride the Lath layout's leafMeta. The cost lands where the field looks like it should help: when the Lath blob fails to validate, LathWallEngine.seed falls back to a fresh tree of plain terminals, while restoreSession still spawns the PTY and types the command because surfaceType === 'tool' — so the user gets the tool's command running in a pane that will never grow a browser, and the persisted row that could have rebuilt it is ignored. The ?? previousPane?.tool fallback beside it is also unreachable: toolMetadataFromParams never returns null for a tool pane, since it defaults render: 'iframe' / port: 'announced'.

3. lib/src/components/wall/ToolPortConflict.tsx:27 — the conflict panel prescribes a fix that doesn't exist for the invocation that most often hits itconfirmed

The panel says "Have the tool announce its port, or set port: announced in dormouse.yml." dor tool -- <command> has no dormouse.yml entry, and use-dor-control.ts:771 hardcodes let port: 'announced' | 'auto' = 'auto' for that form — the comment right above it says so ("dor tool -- <command> has nowhere to declare a strategy, so it autobinds") — with no flag to override. So dor tool -- pnpm dev on anything binding two ports (an API plus a client, vite plus its HMR socket) lands on this panel with both remedies unreachable: there is no entry to edit, and the user does not control an arbitrary command's announcements. The only escape is to stop using the -- form. Either name that in the panel's copy, or give the -- form a way to ask for announced.

Why this posted from a sweep, and what I verified

tend-review 33450294913 ($5.34, 49 turns, job success) emitted the findings above as its terminal assistant message and never called a posting tool — the same abort shape recorded twice before on trivial diffs (#440, #443), here for the first time on a substantive one. Because the job goes green either way, an aborted review is externally indistinguishable from a clean one, which is why nothing surfaced this until the daily sweep read the log.

Verification I ran myself against a131c0e1, not relayed:

  • Finding 2: git grep for every .tool read across lib/, standalone/, vscode-ext/ returns exactly the three sites named above — one writer, one validator, one write-side fallback. No consumer.
  • Finding 3: read ToolPortConflict.tsx and use-dor-control.ts:768-771 at this head; the hardcoded 'auto' and the absent override are both as described.
  • Finding 1: confirmed the discarded await, the killPaneImmediately on non-ok, TRUST_LOCK_STALE_MS = 30_000, and the 5 s tool_control budget. I did not reproduce the timing window, so it stays plausible.

I also dismissed the bot's own APPROVED from a97cef63. It was a correct verdict for the commit it was posted on, but GitHub keeps the most recent approval as the standing decision no matter how many COMMENTED reviews follow it, so gh pr view 493 --json reviewDecision was reading APPROVED for a head with two confirmed defects on it. Nothing was at risk of merging — the branch is BLOCKED on a pnpm-lock.yaml conflict — but the repo's public review state said the bot approves code the bot says is broken. Re-approving is one push away once these are answered.

@dormouse-bot
dormouse-bot dismissed their stale review September 1, 2026 12:58

Dismissing my own approval: it was posted on a97cef6, and the head has advanced to a131c0e, where the review I just posted reports two confirmed defects. GitHub keeps the newest APPROVED as the standing decision regardless of later COMMENTED reviews, so leaving it would have the repo reading APPROVED for a head the bot says is broken. Nothing was at risk of merging (the branch is BLOCKED on a pnpm-lock.yaml conflict). Re-approving is one push away.

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.

2 participants