Conversation
# Conflicts: # docs/specs/dor-tool.md # dor/src/commands/open.ts # dor/test/snapshots/help/open.md # lib/src/host/tool-open.test.ts # scripts/spec-word-budgets.json
Deploying mouseterm with
|
| Latest commit: |
043dd90
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://26785d8d.mouseterm.pages.dev |
| Branch Preview URL: | https://dor-file-viewer.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want the full pass.
Three points from reading the viewer and the path dor open actually renders it through; the largest is that the CSP the new security-local.md section leans on never reaches the browser.
One design question that isn't a code finding: builtin:file is not reserved. parseToolFile still accepts a user tool named literally builtin:file, and after that dor tool builtin:file runs the user's entry while --tool builtin:file and any open rule naming it silently resolve to the viewer instead. Rejecting the name at parse time, or reserving the builtin: prefix in the spec, settles which one wins.
# Conflicts: # .github/audit/application-security.md # docs/specs/dor-tool.md # docs/specs/security-local.md # lib/src/components/wall/browser-surface.ts # lib/src/host/tool-open.test.ts # lib/src/host/tool-open.ts # lib/src/host/tool-registry.ts
Viewer (dor/src/file-viewer.ts): one `outsideRoot` predicate for both containment checks, a `ViewerLimitError` class instead of matching error text, an explicit descriptor hand-off in place of the `let file`/`finally` sentinel, `register` returning the resource so the main entry is not re-derived from the route formula, a stat-only 8 MiB precheck instead of reading and discarding the file, route dedupe before `realpath`, per-chunk buffers instead of alloc-plus-copy, and `node:http` / `node:fs/promises` loaded on demand so they no longer load on every `dor` invocation. CLI: `dor __view-file` dispatches inside `runCli` beside the other pre-stricli route, so `dor.ts` returns to its original shape; the `ProcessLike` and `URL` shims that `@types/node` made redundant are gone. `BUILTIN_FILE_TOOL` / `VIEW_FILE_ARGV` are exported from the pure format module and shared by `tool-open.ts`, `tool-registry.ts`, and `cli.ts`. lib: a named `ToolKeyScope` + `isToolKeyScope` replaces six hand-spelled `'user' | 'builtin'` unions; `announcementKey` builds the applied (port, path) memory in one place; the path re-validation in `use-tool-serving` documents why it is not redundant with the parser. Docs: merge the two `Source of truth:` paragraphs in Opening local files; point the viewer guard and the loopback lint header at the shared rule. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
# Conflicts: # docs/specs/dor-tool.md # lib/src/components/wall/use-dor-control.ts # lib/src/components/wall/use-tool-serving.ts # lib/src/host/tool-registry.ts # scripts/spec-word-budgets.json
# Conflicts: # lib/src/lib/session-save.ts # lib/src/lib/session-types.ts
# Conflicts: # docs/specs/dor-tool.md # lib/src/components/wall/use-tool-serving.ts # scripts/spec-word-budgets.json
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want the full pass.
Two points on the new dispatch and grant bounds. The three earlier ones are addressed, and lib/src/host/file-viewer-proxy.test.ts pinning the preserved policy against the real viewer rather than a stub is the right shape for it.
--tool builtin:file on an unsupported file points at a config that cannot express the fix. resolveOpenTool returns the built-in only when fileViewerFormat(target) is non-null, so dor open --tool builtin:file notes.zip falls through to no user Tool 'builtin:file' in <user dormouse.yml>. Acting on that is impossible: parseToolFile now throws the 'builtin:' prefix is reserved on exactly that name. An open rule naming builtin:file with a match that catches an unsupported extension lands the same way, reporting that no rule matched when one did. startFileViewer already carries the message that fits — unsupported file format; configure a user Tool association — but nothing reaches it from this path.
Exceeding the 256-file bound aborts the open, and the scan runs even where no grant is served. register rethrows ViewerLimitError whether or not the asset was required, so one reference past the bound fails the whole open: dor open report.html on a document with 257 local images errors with local preview exceeds 256 referenced files and renders nothing. That reads deliberate from the class comment and bounds the asset graph and keeps a grant on the opened file after path replacement, but the spec states the bound without the outcome, directly under the 8 MiB rule's explicit "still stream without dependency inspection" — so a reader infers truncation. Worth stating whichever way you settle it.
It also reaches a case with no grant to protect. fileViewerFormat marks .css as both text/css and text: true, so dor open theme.css opens a descriptor for every resolvable url() target while the only route served is the escaped view preview. A stylesheet with 256 local url() references therefore fails to open as source.
# Conflicts: # scripts/spec-word-budgets.json
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want the full pass.
Both of the last round's points are addressed: --tool builtin:file on an unsupported format now reaches a message naming the limitation, and the 256-file bound's outcome is stated in the spec. Skipping dependency inspection for source previews is the right cut — it removes the authority a CSS preview could never use, and dor/test/file-viewer.test.mjs pins both directions of it.
One finding, on the code that decision now rests on. inspectDependencies = !format.text makes fileViewerFormat's text flag load-bearing for a second decision, and that flag is set by a name heuristic that overrides the extension table. Details inline on dor/src/file-viewer-format.ts.
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want the full pass.
3ee75168 is already confirmed on its own thread. One point on the new toggle, and it is about the direction the copy doesn't describe.
Turning Dor Tools off leaves every open Tool running, and the dialog says the opposite. isToolsEnabled() is read in exactly one place — the launch gate in useDorControl's SURFACE_CONTROL_METHODS.tool arm — so it decides designation and nothing else. useToolServing never consults it, and a Tool's dor process is untouched, so after the switch goes off every open Tool keeps its pane, its port, and (for dor open) its loopback file-viewer listener serving the granted files until the Tool exits. That is exactly what Must gate tool creation on isToolsEnabled one line above already says, so the spec is right; "Changes apply immediately" is the sentence that isn't. The reader most likely to reach for this switch is one who wants the viewer to stop serving their files, and they get no signal that closing the panes is the part that does it.
Non-blocking: nothing pins the new Must expose the flag in Settings → Experimental → Dor Tools. SettingsDialog.test.tsx carries only the two archive cases, and a Storybook play that flipped the switch would leak into every other story in the file — the hazard clearPersistedShellSelection in lib/src/lib/shell-store.ts exists to work around — so a test asserting the switch writes TOOLS_FLAG_KEY is the cleaner place for it.
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want
the full pass.
One point on the new placement, and it is about the second dor open rather
than the first.
A pane that take-over converts is never a plain terminal again, so the next
dor open from that prompt splits. surfaceKindFromParams reads
surfaceType: 'tool' out of the leaf params, and nothing clears it when the
Tool's command exits — the pane shows its terminal and prompt again, but
kind === 'terminal' no longer holds, so toolTakesOverCaller fails there
forever. From one shell that reads: dor open report.html takes over; quit the
viewer; dor open notes.md splits into a second pane, while dor open report.html re-runs in place as adopted through toolRerunsInCaller. Which
of the two you get is decided by whether it is the same file, and the user's
original terminal is now permanently a Tool Surface.
The asymmetry was inert while dor tool was the only verb — relaunching a
different tool from the pane a tool already owns is rare. Opening a second file
from the shell you opened the first one in is the ordinary flow for a viewer, so
the gate's cost lands differently now. Either direction settles it: revert a
taken-over pane to a plain terminal when its Tool command exits, so the gate is
re-entered, or keep the one-shot and name it — the Take-over table's Caller row
says "integrated plain terminal", and both dor tool's and dor open's help
paragraphs list what splits without mentioning a pane that is already a Tool.
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want
the full pass.
Dropping the explicit visible is the right shape: both mechanisms this has to
survive hide an ancestor with visibility: hidden — WorkspaceWindow's
invisible pointer-events-none on the inactive wall, and the parked-leaf style
in LathHost — so a child that restates visible is the only thing that could
escape either.
One point, on the spec side, and it lands in the spec that owns the rule rather
than the one this commit edits.
tiling-engine.md still says only browser Surfaces park, and the new
dor-tool.md rule depends on the opposite. shouldParkOnMinimize gained
|| meta.component === 'tool' in this PR, but the spec that owns parking was
not moved with it. Three places there still exclude Tools: the
doorLeaf(id, { park: true }) row of the parking table qualifies its DOM cell
as "mounted — browser Surfaces only"; the Who parks bullet reads
"shouldParkOnMinimize — browser Surfaces, not terminals"; and
Never evict parked browser DOM to enforce a count limit says parking budgets
"minimized browser Surfaces only". The new Must hide Tools in inactive
Workspaces and minimized leaves without unmounting only holds for the
minimized half because a Tool now parks, so a reader who follows the overlap
rule to tiling-engine.md for parking — per What, not why in AGENTS.md,
each rule once in the spec that owns it — gets told a minimized Tool unmounts
and loses its framed document. Naming tool alongside browser in those three
lines settles it. (The inactive-Workspace half is already right there: "a hidden
Workspace parks nothing — its leaves stay mounted and merely stop painting.")
Non-blocking, inline on the new test.
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark it ready when you want the full pass.
One point on f196bea6, and it is about the PR body rather than the code.
This is now the PR that ships Dor Tools, and the title and body still scope it to the viewer. Removing dormouse.flags.tools, its Settings → Experimental row, and the launch gate in useDorControl is what makes dor tool, dor open, and the whole #493 → #514 → #668 → #669 stack reachable for every user on merge — including the loopback file-viewer listener and the project-Tool trust prompt. A maintainer deciding on the top of the stack from its description would not learn that from "Add a built-in local-file viewer for dor open". The Validation line pins 2d109d35, five commits back, predating both cfa1690c adding the toggle and f196bea6 removing the flag it exposed.
dormouse-bot
left a comment
There was a problem hiding this comment.
Full pass now that this is ready for review. Every automated check is green at 28165306; UI Tests is the Chromatic baseline gate, which only a maintainer clears.
The title and body rewrite closes the last round's point — the scope this actually ships is now stated, and the Validation line is pinned to this head. Two things from the draft rounds are still open, and they are what holds the verdict rather than anything new:
-
A pane that take-over converts is never a plain terminal again, so the next
dor openfrom that prompt splits.callerStillPlaceablerequireskind === 'terminal', andsurfaceKindFromParamsreadssurfaceType: 'tool'out of leaf params that nothing clears when the Tool's command exits — so the pane shows its prompt again but the gate is closed to it forever.tool-takeover.test.tspins this as['the caller is already a tool', { kind: 'tool' }], so it is a decision rather than an oversight; what is missing is anywhere a user could learn it. The Take-over table's Caller row says "integrated plain terminal", anddor open's help paragraph lists what splits without mentioning a pane that is already a Tool. That asymmetry was inert whiledor toolwas the only verb — relaunching a different tool from a pane a tool already owns is rare — but opening a second file from the shell you opened the first one in is the ordinary flow for a viewer, and from one prompt it now meansdor open notes.mdsplits whiledor open report.htmlre-runs in place. Either name the one-shot in both places, or revert a taken-over pane to a plain terminal on command exit so the gate is re-entered. -
tiling-engine.mdstill says only browser Surfaces park, and this PR's newdor-tool.mdrule depends on the opposite.shouldParkOnMinimizereturns true formeta.component === 'tool', but the spec that owns parking has three lines that exclude Tools: thedoorLeaf(id, { park: true })row's DOM cell ("mounted — browser Surfaces only"), the Who parks bullet ("browser Surfaces, not terminals"), and Never evict parked browser DOM to enforce a count limit ("minimized browser Surfaces only"). Must hide Tools in inactive Workspaces and minimized leaves without unmounting only holds for the minimized half because a Tool now parks, so a reader following the overlap rule totiling-engine.mdis told a minimized Tool unmounts and loses its framed document. The file is outside this diff — say the word and I will push the three-line edit.
One new note, non-blocking. dor/tsconfig.json gains "types": ["node"] and the package gains @types/node, which is what file-viewer.ts needs — but it applies package-wide, including commands/types, commands/shell-quote, and protocol, which the webview and the website bundle import as values. The hand-written ProcessLike shim this PR deletes from dor/src/dor.ts is the evidence that the absence was load-bearing: until now a node: import in a webview-shared dor/* module failed dor's own typecheck, and now it compiles and fails in the browser instead. file-viewer-format.ts's own "this module stays free of Node APIs" is left resting on the comment. Scoping the node types to the viewer files would cost a second tsconfig, which may well not be worth it — worth a decision either way rather than drifting into it.
Inline on browser-surface.ts.
# Conflicts: # docs/specs/dor-tool.rationale.md # lib/src/components/wall/tool-takeover.ts # scripts/spec-word-budgets.json
# Conflicts: # lib/src/components/Wall.test.tsx
# Conflicts: # scripts/spec-word-budgets.json
# Conflicts: # docs/specs/dor-tool.md # lib/src/host/tool-registry.ts
dor open <file>gains a built-in viewer for HTML, text/source, images, PDF, and media. User associations take precedence;--tool builtin:fileselects the viewer explicitly. The canonical filename supplies per-file reuse within a Workspace, separate from project and user Tools.Tools are available without a feature flag or experimental Settings toggle. A standalone
dor openinvocation follows the same inline take-over rules asdor tool; agent invocations and explicit placement still split. Tool faces inherit Workspace and parked-leaf visibility, preventing hidden Tools from painting over another Workspace.The viewer runs as a Tool-owned process. Requests require its URL capability, a loopback Host, an allowed Origin, and GET/HEAD. Grants contain the selected file and bounded statically referenced relative HTML/CSS assets. The capability stays out of durable state. Markdown displays as escaped source; text previews and dependency inspection are capped at 8 MiB. Root-relative, external, and dynamic references do not expand the grant. Atomic file replacement or dependency changes require restarting the viewer.
Local validation: production-flag removal, inline-open, Workspace visibility, host dispatch, and approval integration checks pass, including 171 integration/host tests and 50 focused registry/open tests after parent merges, library TypeScript, CLI build/help, browser-dependency checks, and spec lint. The visibility regression fails before its fix and passes afterward. CI and Chromatic run on the pushed head.
Stack predecessor: #669 (
dor-open-dispatch). Merge the stack bottom-up: #493 → #514 → #668 → #669 → this PR. QC #675 changes PDFs to require a user Tool and fixes narrow-header controls; dirty indication #676 follows QC. Full merge order: #493 → #514 → #668 → #669 → #670 → #675 → #676.