Dor Tools: accept inputs and user-global definitions - #668
Conversation
Deploying mouseterm with
|
| Latest commit: |
7991e2d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://816c2d53.mouseterm.pages.dev |
| Branch Preview URL: | https://dor-tool-inputs.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 ready when you want the full pass.
Four findings from tracing the input-resolution path end to end; details and two suggestions are inline.
The one worth fixing first is the approval round-trip. createToolHost's untrusted arm resolves inputs against dedupeTemplate: null, so the pending prompt never enforces a $TARGET that appears only in prespawn_dedupe. With run: [viewer] and prespawn_dedupe: [$PROJECT_ROOT, $TARGET] in an unapproved checkout, dor tool viewer with no arguments shows the approval prompt; the re-lookup after "Always allow for folder" goes through the trusted arm with the full template, throws $TARGET requires exactly one local file argument, and Wall.tsx maps every non-ok status to closeSurface — so the pane the user just approved vanishes with no message, having spent a trust grant. Inputs make that silent close routine rather than exotic: a target moved or deleted between the prompt and the click lands on the same path.
Two cleanup notes, while the shape is still moving:
Wall.tsxre-derivesdorCommandString's body —buildShellCommandForKind(shellCommandKind(getDefaultShellOpts()?.shell, PLATFORM_STRING), …)— rather than calling it, so the quoting policy for a tool command now lives in two places that have to agree. Exporting it fromuse-dor-control.ts, or moving it besidegetDefaultShellOpts, keeps one.runToolCommandre-walks raw argv againstFLAGS_WITH_VALUES/BOOLEAN_FLAGSto recover the positional splitvalidateToolArgscomputed during pre-parse. That is a third place the flag lists have to stay in sync, against the "Keep the flag lists above in sync" note the file already carries; a shared helper returning the positionals would collapse it.
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict.
The four findings from the previous pass are all addressed, and the $TARGET-in-dedupe rejection moved to parseToolFile as suggested. Two points on the new failure state, inline.
The one worth deciding now: the failure state reuses the approval prompt verbatim, so after a failed launch the pane's only actions are to grant trust that has already been recorded, or to decline — and decline now discards a pane whose grant was written, contradicting the panel's own "Declining records nothing" and resolveToolApproval's decline-branch comment ("A refusal writes nothing ... leaves no record"). docs/specs/dor-tool.md says the failed pane allows "retry or closure", which is a different pair of buttons than the ones it currently shows.
# Conflicts: # lib/src/components/Wall.test.tsx # lib/src/components/wall/use-dor-control.ts
One substitution engine in tool-registry.ts serves both the dedupe key and the run list, so an unknown token fails the same way in either. The host assembles its ok result once for project and user Tools, and lookupTool takes an options object instead of a sixth positional. The renderer quotes a resolved run through one exported toolRunCommand, dedupes user Tools on the persisted scope rather than a key prefix, and compares pending inputs with toolKeysEqual. The dor CLI walks the argv head once for both validation and the named/anonymous decision, dropping the derived hasArgumentEscape context field. A missing $TARGET file reports a plain message instead of an errno, and the approval pane's error uses the error token. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
# Conflicts: # docs/specs/dor-tool.md # dor/src/commands/tool.ts # lib/src/components/Wall.tsx # lib/src/components/wall/use-dor-control.ts # lib/src/components/wall/wall-context.tsx # lib/src/host/tool-trust.ts # scripts/spec-word-budgets.json
# Conflicts: # docs/specs/dor-tool.md # lib/src/components/Wall.test.tsx # lib/src/components/Wall.tsx # lib/src/components/wall/use-dor-control.ts # scripts/spec-word-budgets.json
dormouse-bot
left a comment
There was a problem hiding this comment.
Both fixes confirmed on de6c2d4, and CI is green.
The resurrection guard holds because isCurrent() compares the pending marker by identity against the object captured at entry, and updateParams shallow-merges params, so the stored toolPending survives by reference — once launch writes toolPending: undefined, no later showFailure can match. The failAfterSpawn case pins it. The Retry/Close state now matches what dor-tool.md claims, and all three trust failure modes keep the approval buttons without reaching lookup, so a failed write can't strand the pane claiming a permission it doesn't have.
I also read the two commits that landed after my last pass (destination-shell quoting, persisted-argv requote). What I checked holds: restore re-quotes for the same getDefaultShellOpts() shell every restored pane actually spawns with, toolArgv is written on all three launch paths (approval, key match, take-over), and $TARGET alongside a string run is rejected at parse time now. One consistency point inline, not a blocker.
# Conflicts: # docs/specs/dor-tool.md # lib/src/components/Wall.test.tsx # lib/src/components/Wall.tsx
# Conflicts: # docs/specs/dor-tool.md
# Conflicts: # docs/specs/dor-tool.md # lib/src/components/Wall.tsx
# Conflicts: # docs/specs/dor-tool.md # lib/src/components/Wall.tsx # lib/src/components/wall/ToolApproval.tsx # scripts/spec-word-budgets.json
# Conflicts: # docs/specs/dor-tool.md # lib/src/components/wall/ToolApproval.tsx
dormouse-bot
left a comment
There was a problem hiding this comment.
Full pass now that this is ready for review. Two findings, both inline.
createToolHost falls back to the user file on a project miss, but if (project) return project; hands back the project's no-file whenever a project lookup ran at all. So with a populated ~/.config/dormouse/dormouse.yml and no repo in scope, dor tool viewr foo.md reports no dormouse.yml found in '<cwd>' or any parent directory — naming neither the user file that was read nor the tools in it. The unknown-tool arm immediately below already carries both, and is unreachable on that path.
Separately, a $TARGET run whose prespawn_dedupe omits $TARGET keys every target to one Surface: dor tool viewer b.md matches the pane still showing a.md, reports existing, and reveals it without re-running. parseToolFile already warns for the analogous $PROJECT_ROOT omission, so the same warning fits beside it — with a companion line under Declaring tools, which dor-tool.md is freshly at budget for.
dormouse-bot
left a comment
There was a problem hiding this comment.
Both findings from the last pass are fixed — the user-file fallback now routes a project no-file into the unknown-tool that names the real path, and the $TARGET-key warning sits inside the prespawn_dedupe block so a Tool with no key is untouched.
Build & Test is red at this head, and not for anything in this commit. pnpm test is an && chain with node scripts/spec-lint.mjs first, so the failure stops the run before any of the new tool-host / tool-registry cases execute — they have not run in CI yet.
The failure is a ratchet collision, invisible from the branch alone. CI lints the merge tree, and scripts/spec-md.mjs's countWords puts docs/specs/security-local.md at 2899 words on cd5162d5 (under the 2900 this PR ratcheted to) but 2925 on the merge with main — base commit 9c11aa87 ("test: pin Vite Host protection and document its security boundary") added a FAIL IF line and a Source of truth: continuation to the same spec. Both sides are individually under their own budget; only the merge is over. Suggestion inline bumps it to 2950, which is what --ratchet would write for 2925 and stays valid on the branch too, since the lint only fails on words > budget.
Happy to push that one-line bump if you'd rather not round-trip it.
# Conflicts: # lib/src/components/Wall.tsx
Named Tools accept arguments and user-global definitions. List-valued
runexpands$ARGSand canonical local-file$TARGETinputs as argument values, quoted for the destination shell. Shell-string commands stay literal and reject extra inputs. Argument values containing terminal control characters are rejected before injection.Resolved argv is saved independently of the live command string. Cold restore re-quotes it for the selected shell and updates terminal options plus pane/Door metadata, including when the original Tool took over a different shell. Invalid persisted argv is rejected before any PTY starts; literal shell-string commands keep their saved text.
Project lookup falls back to
$XDG_CONFIG_HOME/dormouse/dormouse.ymlor~/.config/dormouse/dormouse.yml;--globalskips project lookup. Project and user Tools have separate reuse scopes. User configuration supports dotfiles symlinks. Unknown names report the user file when no project file exists; target-based commands warn when their reuse key omits the target.Approval validates inputs before presenting the command and distinguishes pending requests by arguments and invocation directory, preserving
--freshthrough approval. If lookup fails after permission is saved, Retry repeats lookup and Close leaves permission intact; a failed lookup leaves the pane without a PTY.Validation coverage: host resolution and input validation, destination-shell quoting, changed-shell cold restore, persisted control rejection, approval failures/retries, pending-request identity, and CLI output. No dogfooding or installation.
Local validation: 149 Tool lookup, registry, and Wall integration tests, library TypeScript, and spec checks pass after parent integration. CI and Chromatic run on the pushed head.
Tool stack 3/7, based on #514. Next: #669. Merge bottom-up after its predecessors.