From 3c4fe3181b57bb4d4a86ee49b315adada29714a1 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 15 Sep 2026 16:24:28 -0700 Subject: [PATCH 01/10] Add named Tool inputs and user configuration --- docs/specs/dor-tool.md | 23 ++++++----- docs/specs/dor-tool.rationale.md | 2 + docs/specs/security-local.md | 2 + dor/src/cli.ts | 1 + dor/src/commands/tool.ts | 43 ++++++++++----------- dor/src/commands/types.ts | 3 ++ dor/test/cli-output.test.mjs | 23 +++++++---- dor/test/snapshots/help/dor.md | 3 +- dor/test/snapshots/help/tool.md | 8 ++-- dor/test/snapshots/tool-name-and-tail.snap | 5 --- dor/test/snapshots/tool-name-args.snap | 5 --- lib/src/components/Wall.test.tsx | 32 +++++++++++++++ lib/src/components/Wall.tsx | 14 ++++--- lib/src/components/wall/browser-surface.ts | 4 +- lib/src/components/wall/use-dor-control.ts | 28 +++++++++----- lib/src/components/wall/use-tool-serving.ts | 2 +- lib/src/host/tool-host.test.ts | 41 +++++++++++++++++++- lib/src/host/tool-host.ts | 36 ++++++++++++----- lib/src/host/tool-input.ts | 40 +++++++++++++++++++ lib/src/host/tool-registry.ts | 25 ++++++++---- lib/src/host/tool-trust.ts | 4 +- lib/src/host/tool-user-config.ts | 18 +++++++++ lib/src/lib/platform/tool-types.ts | 7 ++-- lib/src/lib/session-restore.ts | 2 +- lib/src/lib/session-save.ts | 2 +- lib/src/lib/session-types.ts | 2 + scripts/spec-word-budgets.json | 4 +- 27 files changed, 282 insertions(+), 97 deletions(-) delete mode 100644 dor/test/snapshots/tool-name-and-tail.snap delete mode 100644 dor/test/snapshots/tool-name-args.snap create mode 100644 lib/src/host/tool-input.ts create mode 100644 lib/src/host/tool-user-config.ts diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index 179b7f3a1..8ed1ae3e8 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -32,21 +32,27 @@ Source of truth: `surfaceKindFromParams` / `isToolParams` in `lib/src/components ## Declaring tools -**Must resolve a named Tool from the nearest ancestor `dormouse.yml`.** The host owns discovery, bounded reads, YAML parsing, and substitutions; the renderer receives the resolved result. Canonical field shapes are `ToolEntry` in `lib/src/host/tool-registry.ts`. +**Must resolve a named Tool from the nearest ancestor `dormouse.yml`, then fall back to user Tools when that name is absent.** `--global` skips project discovery. A malformed project file fails lookup rather than falling back. The host owns discovery, bounded reads, YAML parsing, and substitutions; the renderer receives the resolved result. Canonical field shapes are `ToolEntry` in `lib/src/host/tool-registry.ts`. + +**Must read user Tools from `$XDG_CONFIG_HOME/dormouse/dormouse.yml` when that environment value is absolute, otherwise `~/.config/dormouse/dormouse.yml`.** Both local hosts use this location. User Tools require no project grant; malformed or unreadable user configuration fails lookup. Project and user Tools occupy separate reuse scopes. | Field | Behavior | | --- | --- | -| `run` | Required command, typed into the configured shell after integration readiness | +| `run` | Required shell command string or argument list, typed into the configured shell after integration readiness | | `render` | `iframe` by default, or `ab-screencast` | | `port` | `announced` by default, or `auto`; [Serving](#serving) owns selection | | `prespawn_dedupe` | Optional scalar or list of literal key elements with substitutions | -- **Must reject unknown `prespawn_*` fields and unknown substitutions**; unknown ordinary fields produce warnings. The substitution set is `$PROJECT_ROOT`, the declaring directory, and `$CWD`, the caller's resolved directory. (rationale) +- **Must reject unknown `prespawn_*` fields and unknown substitutions**; unknown ordinary fields produce warnings. `$PROJECT_ROOT` is the declaring directory, `$CWD` the caller's resolved directory, and `$TARGET` the canonical local file input. (rationale) - **Must preserve scalar `prespawn_dedupe` as a one-element literal list**, never interpret it as a command to execute. Reserve separate fields for future computed keys. (rationale) - **Must warn when a repo-local key omits `$PROJECT_ROOT`**, while allowing intentional cross-checkout dedupe. -- Reserved: **Must reject `$PROJECT_ROOT` in the future user-global configuration**, which has no project root; see scope **dor-tools** under [Future](#future). +- **Must reject `$PROJECT_ROOT` in user configuration**, which has no project root. + +**Must pass named-tool inputs as argument values, never substitute them into a shell-command string.** String `run` accepts no arguments and remains literal shell syntax. List `run` expands `$TARGET`, `$CWD`, and `$PROJECT_ROOT` within elements; a whole `$ARGS` element expands all input arguments. Without `$ARGS` or `$TARGET` in the list, append the inputs. The renderer quotes the resulting argv for its configured shell. + +**Must require exactly one existing regular local file when `$TARGET` appears in the run list or dedupe key.** Resolve relative paths against the invocation CWD and follow symlinks to a canonical absolute path before substitution and reuse. Reject URLs, directories, and missing files. Pending approval retains the original arguments and distinguishes requests with different inputs; approval re-resolves them before launch. -Source of truth: `lookupTool` in `lib/src/host/tool-trust.ts`; `parseToolFile` / `resolveDedupeKey` in `lib/src/host/tool-registry.ts`; `lib/src/host/tool-registry.test.ts`. +Source of truth: `lookupTool` in `lib/src/host/tool-trust.ts`; `parseToolFile` / `resolveDedupeKey` in `lib/src/host/tool-registry.ts`; `resolveToolInput` in `lib/src/host/tool-input.ts`; `readUserToolFile` in `lib/src/host/tool-user-config.ts`; `lib/src/host/tool-host.test.ts`, `lib/src/host/tool-registry.test.ts`. ## Identity and dedupe @@ -187,10 +193,9 @@ Source of truth: `PersistedToolMetadata` in `lib/src/lib/session-types.ts`; `sav **Scope: dor-tools** — remaining design, in implementation order. -- **C — glob table + `dor open`.** The user-global tools file, glob rules - (pattern → tool name), `dor open ` as sugar over `dor tool`, argument - substitution in `prespawn_dedupe` so per-target viewers do not collapse into - one pane, and the loopback file/viewer endpoint a local *file* needs (the +- **C — glob table + `dor open`.** User-global glob rules + (pattern → tool name), `dor open ` as sugar over `dor tool`, + and the loopback file/viewer endpoint a local *file* needs (the iframe proxy instruments only `http://` upstreams). - **D1 — reaping without cooperation.** Idle-threshold reap + rehydrate-from-args + `persist: "never"`: every stateless tool, no new API, diff --git a/docs/specs/dor-tool.rationale.md b/docs/specs/dor-tool.rationale.md index 8c3b08992..78d4cfcb5 100644 --- a/docs/specs/dor-tool.rationale.md +++ b/docs/specs/dor-tool.rationale.md @@ -8,6 +8,8 @@ YAML authors naturally collapse one-element lists to scalars. Overloading a scal A misspelled substitution such as `$PROJECTROOT` retained as a literal silently makes distinct checkouts share a key. Rejecting unknown substitutions exposes the typo before reuse can target another checkout. +Argument-list commands let the renderer quote each value for the actual target shell. Keeping shell strings literal avoids needing a shell-template parser to distinguish an author-provided pipeline from punctuation in a filename. Canonical file targets make symlink aliases reuse the same document viewer. + ## Identity and dedupe `pnpm storybook`, `pnpm run storybook`, and `pnpm storybook --quiet` are different command strings for the same intended tool. `dor ensure` already supplies exact-command/CWD identity. An explicit Tool key allows authors to choose their own scope without making the declaration of a short command name implicitly enable dedupe. diff --git a/docs/specs/security-local.md b/docs/specs/security-local.md index d3fde1cb5..5700d8b7b 100644 --- a/docs/specs/security-local.md +++ b/docs/specs/security-local.md @@ -202,6 +202,8 @@ Source of truth: `context` in `standalone/sidecar/pty-core.js`; `attachRouter` i **Must keep repo-local named Tools inert until the user grants trust through Dormouse chrome.** The control socket exposes lookup and launch, never a trust-grant verb. Pending approval spawns neither its terminal nor a helper. Approval workflow belongs to `docs/specs/dor-tool.md` → Trust. +**Must keep named-tool inputs as argv until the renderer quotes them for the target shell.** User configuration is the local user's authority; a project name cannot replace a user Tool during user-only lookup. Resolution belongs to `docs/specs/dor-tool.md` → Declaring tools. + **Must derive the grant key in the host**, using the canonical upstream URL or project-root folder; a renderer request cannot supply an arbitrary grant URL. **Must bound config reads and refuse symlinks on every host.** **An upstream grant trusts the claimed URL, not authenticated checkout provenance.** A supplied directory containing its own `.git/config` can claim an already-granted upstream; folder-only grants limit this sharing. **Must not describe the chrome gesture as a boundary against other processes running as the user**; the local account model is The dor control socket above. diff --git a/dor/src/cli.ts b/dor/src/cli.ts index 40fdfb16a..faf7d6b07 100644 --- a/dor/src/cli.ts +++ b/dor/src/cli.ts @@ -218,6 +218,7 @@ export async function runCli(rawArgv: string[], options: CliOptions = {}): Promi process: capture.process, options, hasArgumentEscape, + commandArgs: args, }), }); diff --git a/dor/src/commands/tool.ts b/dor/src/commands/tool.ts index 3b80b4e1c..080058e69 100644 --- a/dor/src/commands/tool.ts +++ b/dor/src/commands/tool.ts @@ -21,6 +21,7 @@ import { interface ToolFlags { readonly json?: boolean; + readonly global?: boolean; readonly minimize?: boolean; readonly fresh?: boolean; readonly surface?: string; @@ -33,10 +34,10 @@ interface ToolFlags { const TOOL_TIMEOUT_MS = 20_000; const FLAGS_WITH_VALUES = new Set(['--cwd', '--surface', '--workspace']); -const BOOLEAN_FLAGS = new Set(['--json', '--minimize', '--fresh']); +const BOOLEAN_FLAGS = new Set(['--json', '--minimize', '--fresh', '--global']); /** - * `dor tool` takes either a registered name or a `--` command tail, never both. + * `dor tool` takes a registered name with inputs, or a nameless `--` command tail. * stricli cannot express that, so the shape is checked before it parses — the * same pre-parse contract `dor ensure` uses. Keep the flag lists above in sync * with `parameters.flags`. @@ -63,19 +64,11 @@ export function validateToolArgs(args: string[]): ParseResult { if (positionals.length === 0) { return { ok: false, message: 'dor tool requires a tool name or -- ' }; } - // Arguments for a named tool wait for phase C, where substitution has 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. - if (positionals.length > 1) { - return { ok: false, message: `dor tool takes no arguments (got '${positionals[1]}')` }; - } return { ok: true, value: undefined }; } - // `dor tool -- ` would leave two sources for one command. - if (positionals.length > 0) { - return { ok: false, message: `unexpected argument '${positionals[0]}' before --` }; - } + if (positionals.length > 0) return { ok: true, value: undefined }; + if (head.includes('--global')) return { ok: false, message: '--global requires a named tool' }; if (args.slice(delimiterIndex + 1).join(' ').trim() === '') { return { ok: false, message: 'dor tool requires a command after --' }; } @@ -89,15 +82,15 @@ export const toolCommand: Command = { { scope: 'root', findReplace: [ - ' dor tool [--json] [--minimize] [--fresh] [--surface id|ref] [--cwd path] [--workspace ref]', - ' dor tool [--json] [--minimize] [--fresh] [--surface id|ref] [--cwd path] [--workspace ref] \n dor tool [--json] [--minimize] [--surface id|ref] [--cwd path] [--workspace ref] -- ...\n', + ' dor tool [--global] [--json] [--minimize] [--fresh] [--surface id|ref] [--workspace ref] [--cwd path]', + ' dor tool [--global] [--json] [--minimize] [--fresh] [--surface id|ref] [--cwd path] [--workspace ref] [args...]\n dor tool [--json] [--minimize] [--surface id|ref] [--cwd path] [--workspace ref] -- ...\n', ], }, { scope: 'command-usage', findReplace: [ - ' dor tool [--json] [--minimize] [--fresh] [--surface id|ref] [--cwd path] [--workspace ref]', - ' dor tool [--json] [--minimize] [--fresh] [--surface id|ref] [--cwd path] [--workspace ref] \n dor tool [--json] [--minimize] [--surface id|ref] [--cwd path] [--workspace ref] -- ...\n', + ' dor tool [--global] [--json] [--minimize] [--fresh] [--surface id|ref] [--workspace ref] [--cwd path]', + ' dor tool [--global] [--json] [--minimize] [--fresh] [--surface id|ref] [--cwd path] [--workspace ref] [args...]\n dor tool [--json] [--minimize] [--surface id|ref] [--cwd path] [--workspace ref] -- ...\n', ], }, { @@ -110,13 +103,13 @@ export const toolCommand: Command = { brief: 'Run a command as a Dor Tool.', fullDescription: `Runs a command in a new surface and watches the ports it opens. When the command starts serving, the surface grows a browser in place — same surface, same id, no second pane — and the pane flips to it with the terminal behind the header's far-left chip. When the command exits the browser retires and the pane flips back. -Two forms. \`dor tool \` runs an entry from the nearest dormouse.yml, walking up from the working directory. \`dor tool -- \` designates any command as a tool without a registry entry. A named tool takes no extra arguments yet. +Two forms. \`dor tool \` runs an entry from the nearest dormouse.yml, walking up from the working directory, falling back to user-global tools. --global skips project discovery. The user file is $XDG_CONFIG_HOME/dormouse/dormouse.yml, or ~/.config/dormouse/dormouse.yml. \`dor tool -- \` designates any command as a tool without a registry entry. Named tools accept arguments: \`dor tool [args...]\`, or \`dor tool -- \` for arguments beginning with a dash. Use an argument-list \`run\` in the configuration to accept inputs. A tool has an identity if and only if its dormouse.yml entry gave it one, via prespawn_dedupe. With a key, a second invocation whose key matches reveals the running surface instead of starting a duplicate. Without one — and for every \`dor tool -- \` — each invocation creates a fresh surface. Nothing is keyed on the command or the working directory: run the same command twice and you get two tools. --fresh ignores a declared key and always creates. -A dormouse.yml is repo-controlled and its entries execute, so it is inert until you approve it in Dormouse itself. For an unapproved repo the surface is created and reports "pending": its pane shows what would run and waits for you to allow the upstream, allow just this folder, or close it. Nothing from the repo runs until you choose, and declining records nothing. +A project dormouse.yml is repo-controlled and its entries execute, so it is inert until you approve it in Dormouse itself. For an unapproved repo the surface is created and reports "pending": its pane shows what would run and waits for you to allow the upstream, allow just this folder, or close it. Nothing from the repo runs until you choose, and declining records nothing. Approving an upstream covers every worktree and clone of that repo. Approving a folder covers that checkout only, which is what you want for a branch you have not read. @@ -142,6 +135,7 @@ JSON output: }, parameters: { flags: { + global: { kind: 'boolean', brief: 'Resolve only user-global tools.', optional: true, withNegated: false }, json: { kind: 'boolean', brief: 'Print JSON output.', optional: true, withNegated: false }, minimize: { kind: 'boolean', brief: 'Create the surface minimized.', optional: true, withNegated: false }, fresh: { kind: 'boolean', brief: 'Ignore a declared key and always create.', optional: true, withNegated: false }, @@ -160,9 +154,14 @@ JSON output: }; async function runToolCommand(this: DorCommandContext, flags: ToolFlags, ...rest: string[]): Promise { - // `--` is discarded by stricli, so the two forms are indistinguishable from - // the positionals alone; `hasArgumentEscape` is captured pre-parse for it. - const named = !this.hasArgumentEscape; + // The name precedes `--` for a named invocation; an anonymous command has + // only flags before it. stricli discards the separator, so inspect raw argv. + const head = this.commandArgs.slice(0, this.hasArgumentEscape ? this.commandArgs.indexOf('--') : undefined); + let named = false; + for (let i = 0; i < head.length; i++) { + if (FLAGS_WITH_VALUES.has(head[i])) i++; + else if (!BOOLEAN_FLAGS.has(head[i])) named = true; + } if (named && rest.length === 0) { return new Error('dor tool requires a tool name or -- '); } @@ -172,7 +171,7 @@ async function runToolCommand(this: DorCommandContext, flags: ToolFlags, ...rest try { const response = await client.toolSurface({ - ...(named ? { name: rest[0] } : { command: rest }), + ...(named ? { name: rest[0], args: rest.slice(1), global: flags.global === true } : { command: rest }), ...workspaceParam(flags.workspace), fresh: flags.fresh === true, minimized: flags.minimize === true, diff --git a/dor/src/commands/types.ts b/dor/src/commands/types.ts index 554b13129..33a42d7bc 100644 --- a/dor/src/commands/types.ts +++ b/dor/src/commands/types.ts @@ -257,6 +257,8 @@ export interface EnsureSurfaceResponse { export interface ToolSurfaceRequest extends WorkspaceScopedRequest { /** Registered tool name (`dor tool `). */ name?: string; + args?: string[]; + global?: boolean; /** Raw argv (`dor tool -- `); the host quotes it for the shell. */ command?: string[]; /** Ignore any declared key and always create — `--fresh`. */ @@ -502,6 +504,7 @@ export interface DorCommandContext extends CommandContext { * only way a command can tell `dor split --` (empty tail) from a bare * `dor split`. Computed once in `cli.ts` from the pre-parse argv. */ readonly hasArgumentEscape: boolean; + readonly commandArgs: readonly string[]; } export interface Command { diff --git a/dor/test/cli-output.test.mjs b/dor/test/cli-output.test.mjs index 8624b064c..8a9930798 100644 --- a/dor/test/cli-output.test.mjs +++ b/dor/test/cli-output.test.mjs @@ -1719,6 +1719,8 @@ test('tool sends the name, never a command', async () => { method: 'toolSurface', request: { name: 'storybook', + args: [], + global: false, fresh: false, minimized: false, surface: undefined, @@ -1727,8 +1729,13 @@ test('tool sends the name, never a command', async () => { }]); }); -test('tool rejects arguments after a name', async () => { - await snapshot('tool-name-args', await runCli(['tool', 'storybook', 'extra'], { client: fixtureClient() })); +test('tool forwards named arguments as separate argv values', async () => { + const client = fixtureClient(); + const result = await runCli(['tool', '--global', 'viewer', 'a b; $(bad).md'], { client }); + assert.equal(result.exitCode, 0); + assert.equal(client.requests[0].request.name, 'viewer'); + assert.equal(client.requests[0].request.global, true); + assert.deepEqual(client.requests[0].request.args, ['a b; $(bad).md']); }); test('tool -- sends argv as a command, never a name', async () => { @@ -1766,11 +1773,13 @@ test('tool with neither a name nor a command tail', async () => { await snapshot('tool-missing-target', await runCli(['tool'], { client: fixtureClient() })); }); -test('tool rejects a name and a command tail together', async () => { - await snapshot( - 'tool-name-and-tail', - await runCli(['tool', 'storybook', '--', 'pnpm', 'dev'], { client: fixtureClient() }), - ); +test('tool keeps escaped named arguments separate from anonymous commands', async () => { + const client = fixtureClient(); + const result = await runCli(['tool', 'viewer', '--', '--flag', 'a b'], { client }); + assert.equal(result.exitCode, 0); + assert.equal(client.requests[0].request.name, 'viewer'); + assert.deepEqual(client.requests[0].request.args, ['--flag', 'a b']); + assert.equal(client.requests[0].request.command, undefined); }); test('tool rejects an empty command tail', async () => { diff --git a/dor/test/snapshots/help/dor.md b/dor/test/snapshots/help/dor.md index ca68c3415..9d445e507 100644 --- a/dor/test/snapshots/help/dor.md +++ b/dor/test/snapshots/help/dor.md @@ -6,7 +6,8 @@ Invocation: `dor --help` USAGE dor split [--left|--right|--up|--down|--auto] [--json] [--minimize] [--surface id|ref] [--workspace ref] [-- ...] dor ensure [--json] [--minimize] [--restart] [--surface id|ref] [--cwd path] [--workspace ref] -- ... - dor tool [--json] [--minimize] [--fresh] [--surface id|ref] [--workspace ref] [--cwd path] ... + dor tool [--global] [--json] [--minimize] [--fresh] [--surface id|ref] [--cwd path] [--workspace ref] [args...] + dor tool [--json] [--minimize] [--surface id|ref] [--cwd path] [--workspace ref] -- ... dor version [--json] dor skill [--install] [--json] dor send ([--text value] [--key value] | --stdin | --sequence json) [--json] [--raw] [--workspace ref] diff --git a/dor/test/snapshots/help/tool.md b/dor/test/snapshots/help/tool.md index d61f4620a..e328f6158 100644 --- a/dor/test/snapshots/help/tool.md +++ b/dor/test/snapshots/help/tool.md @@ -4,18 +4,19 @@ Invocation: `dor tool --help` ```text USAGE - dor tool [--json] [--minimize] [--fresh] [--surface id|ref] [--workspace ref] [--cwd path] ... + dor tool [--global] [--json] [--minimize] [--fresh] [--surface id|ref] [--cwd path] [--workspace ref] [args...] + dor tool [--json] [--minimize] [--surface id|ref] [--cwd path] [--workspace ref] -- ... dor tool --help Runs a command in a new surface and watches the ports it opens. When the command starts serving, the surface grows a browser in place — same surface, same id, no second pane — and the pane flips to it with the terminal behind the header's far-left chip. When the command exits the browser retires and the pane flips back. -Two forms. `dor tool ` runs an entry from the nearest dormouse.yml, walking up from the working directory. `dor tool -- ` designates any command as a tool without a registry entry. A named tool takes no extra arguments yet. +Two forms. `dor tool ` runs an entry from the nearest dormouse.yml, walking up from the working directory, falling back to user-global tools. --global skips project discovery. The user file is $XDG_CONFIG_HOME/dormouse/dormouse.yml, or ~/.config/dormouse/dormouse.yml. `dor tool -- ` designates any command as a tool without a registry entry. Named tools accept arguments: `dor tool [args...]`, or `dor tool -- ` for arguments beginning with a dash. Use an argument-list `run` in the configuration to accept inputs. A tool has an identity if and only if its dormouse.yml entry gave it one, via prespawn_dedupe. With a key, a second invocation whose key matches reveals the running surface instead of starting a duplicate. Without one — and for every `dor tool -- ` — each invocation creates a fresh surface. Nothing is keyed on the command or the working directory: run the same command twice and you get two tools. --fresh ignores a declared key and always creates. -A dormouse.yml is repo-controlled and its entries execute, so it is inert until you approve it in Dormouse itself. For an unapproved repo the surface is created and reports "pending": its pane shows what would run and waits for you to allow the upstream, allow just this folder, or close it. Nothing from the repo runs until you choose, and declining records nothing. +A project dormouse.yml is repo-controlled and its entries execute, so it is inert until you approve it in Dormouse itself. For an unapproved repo the surface is created and reports "pending": its pane shows what would run and waits for you to allow the upstream, allow just this folder, or close it. Nothing from the repo runs until you choose, and declining records nothing. Approving an upstream covers every worktree and clone of that repo. Approving a folder covers that checkout only, which is what you want for a branch you have not read. @@ -40,6 +41,7 @@ JSON output: } FLAGS + [--global] Resolve only user-global tools. [--json] Print JSON output. [--minimize] Create the surface minimized. [--fresh] Ignore a declared key and always create. diff --git a/dor/test/snapshots/tool-name-and-tail.snap b/dor/test/snapshots/tool-name-and-tail.snap deleted file mode 100644 index 302e1eddd..000000000 --- a/dor/test/snapshots/tool-name-and-tail.snap +++ /dev/null @@ -1,5 +0,0 @@ -exitCode: 1 -stdout: - -stderr: -Error: unexpected argument 'storybook' before -- diff --git a/dor/test/snapshots/tool-name-args.snap b/dor/test/snapshots/tool-name-args.snap deleted file mode 100644 index 515ced20b..000000000 --- a/dor/test/snapshots/tool-name-args.snap +++ /dev/null @@ -1,5 +0,0 @@ -exitCode: 1 -stdout: - -stderr: -Error: dor tool takes no arguments (got 'extra') diff --git a/lib/src/components/Wall.test.tsx b/lib/src/components/Wall.test.tsx index 13d0e6caa..b041614f3 100644 --- a/lib/src/components/Wall.test.tsx +++ b/lib/src/components/Wall.test.tsx @@ -1457,6 +1457,38 @@ describe('Wall on the Lath engine', () => { } }); + it('keeps pending file inputs distinct and quotes argv after approval', async () => { + setToolsEnabled(true); + let trusted = false; + const toolControl = vi.fn(async (request: { op: string; args?: string[] }) => { + if (request.op === 'trust') { trusted = true; return { status: 'trust-recorded' as const }; } + const common = { projectRoot: '/repo', path: '/repo/dormouse.yml', name: 'viewer', run: ['view', ...(request.args ?? [])] }; + return trusted ? { ...common, status: 'ok' as const, render: 'iframe' as const, port: 'auto' as const, key: request.args ?? [], warnings: [] } + : { ...common, status: 'untrusted' as const, upstreamUrl: null }; + }); + Object.assign(fake, { toolControl }); + try { + await act(async () => root.render()); + await flush(); + const ids: string[] = []; + for (const target of ['a b;$(bad).md', 'second.md', 'a b;$(bad).md']) { + const respond = vi.fn(); + await act(async () => window.dispatchEvent(new CustomEvent('dormouse:control-request', { detail: { + method: SURFACE_CONTROL_METHODS.tool, params: { name: 'viewer', cwd: '/repo', args: [target] }, respond, + } }))); + ids.push(respond.mock.calls[0][0].result.surfaceId); + } + expect(ids[0]).not.toBe(ids[1]); + expect(ids[2]).toBe(ids[0]); + const allow = [...container.querySelectorAll('button')].find(button => button.textContent?.includes('Always allow for folder'))!; + await act(async () => allow.click()); + await flush(); + expect(toolControl).toHaveBeenLastCalledWith({ op: 'lookup', name: 'viewer', cwd: '/repo', args: ['a b;$(bad).md'] }); + expect(pendingShellOpts.get(ids[0])?.command).toBe("view 'a b;$(bad).md'"); + ids.forEach(id => pendingShellOpts.delete(id)); + } finally { setToolsEnabled(false); } + }); + it('keeps an approved tool deferred until trust lookup and shell staging finish', async () => { setToolsEnabled(true); let toolId: string | undefined; diff --git a/lib/src/components/Wall.tsx b/lib/src/components/Wall.tsx index 28d7b5b34..e4bbec3ca 100644 --- a/lib/src/components/Wall.tsx +++ b/lib/src/components/Wall.tsx @@ -1,3 +1,4 @@ +import { buildShellCommandForKind, shellCommandKind } from 'dor/commands/shell-quote'; import { captureToolParams } from './wall/tool-transfer'; import { isWorkspaceTransferPending } from '../lib/window-session-aggregator'; import { TerminalContextContext, type TerminalContextOpenOptions, type TerminalContextState } from './wall/wall-context'; @@ -59,7 +60,7 @@ import { createTerminalPaneState, deriveSurfaceLabel, } from '../lib/terminal-state'; -import { getPlatform } from '../lib/platform'; +import { getPlatform, PLATFORM_STRING } from '../lib/platform'; import type { Surface as DorSurface, ResolvedSplitDirection as DorResolvedSplitDirection, @@ -1585,18 +1586,21 @@ export function Wall({ // asking again is what gives an approved tool the config its dormouse.yml // declared, rather than silently running it as a keyless default iframe. const cwd = typeof meta?.params?.cwd === 'string' ? meta.params.cwd : pending.projectRoot; - const resolved = await platform.toolControl?.({ op: 'lookup', name: pending.name, cwd }); + const resolved = await platform.toolControl?.({ op: 'lookup', name: pending.name, cwd, args: pending.args }); if (resolved?.status !== 'ok') { await closeSurface(id); return; } if (closingWorkspaceRef.current || isWorkspaceTransferPending(effectiveWorkspaceId) || !lath.getMeta(id) || lath.isDying(id) || isSurfaceClosing(id)) return; + const command = typeof resolved.run === 'string' ? resolved.run + : buildShellCommandForKind(shellCommandKind(getDefaultShellOpts()?.shell, PLATFORM_STRING), resolved.run); lath.store.updateParams(id, { - command: resolved.run, + command, + toolScope: resolved.scope, toolRender: resolved.render, toolPort: resolved.port, - ...(resolved.key ? { toolKey: namespacedToolKey(resolved.name, resolved.key) } : {}), + ...(resolved.key ? { toolKey: namespacedToolKey(resolved.name, resolved.key, resolved.scope) } : {}), }); // Hand the leaf its command only now. The approval marker stays in place // until after this write, so TerminalPanel cannot consume default options @@ -1607,7 +1611,7 @@ export function Wall({ args: defaults?.args, cwd, untouched: true, - command: resolved.run, + command, requireIntegration: true, }); lath.store.updateParams(id, { toolPending: undefined }); diff --git a/lib/src/components/wall/browser-surface.ts b/lib/src/components/wall/browser-surface.ts index a118727a2..8e4351869 100644 --- a/lib/src/components/wall/browser-surface.ts +++ b/lib/src/components/wall/browser-surface.ts @@ -60,6 +60,7 @@ export function toolPortConflictFromParams(params: unknown): number[] | null { export interface ToolPending { readonly name: string; readonly run: string; + readonly args?: string[]; readonly path: string; readonly projectRoot: string; /** Requested at launch; applied after approval, since a pane the user cannot @@ -135,9 +136,10 @@ export function toolKeysEqual(paramsKey: unknown, key: readonly string[] | null) export function namespacedToolKey( toolName: string | null, key: readonly string[] | null, + scope?: unknown, ): string[] | null { if (!toolName || key === null) return null; - return [toolName, ...key]; + return scope === 'user' ? ['user', toolName, ...key] : [toolName, ...key]; } /** Whether params describe a plain browser surface (vs a terminal): the unified diff --git a/lib/src/components/wall/use-dor-control.ts b/lib/src/components/wall/use-dor-control.ts index 193b4007f..8d8531e63 100644 --- a/lib/src/components/wall/use-dor-control.ts +++ b/lib/src/components/wall/use-dor-control.ts @@ -79,6 +79,8 @@ export type DorControlParams = { wsPort?: unknown; name?: unknown; fresh?: unknown; + args?: unknown; + global?: unknown; }; // The webview view of a control request: the shared wire payload, but with @@ -866,6 +868,8 @@ export function useDorControl({ const toolName = stringParam(params.name)?.trim(); let command: string; let key: string[] | null = null; + let toolScope: 'user' | undefined; + const toolArgs = stringArrayParam(params.args) ?? []; let warnings: string[] = []; let render: 'iframe' | 'ab-screencast' = 'iframe'; // `dor tool -- ` has nowhere to declare a strategy, so it @@ -883,7 +887,7 @@ export function useDorControl({ detail.respond({ ok: false, error: 'this host cannot read a dormouse.yml; use `dor tool -- `' }); return; } - const lookup = await toolControl({ op: 'lookup', name: toolName, cwd }); + const lookup = await toolControl({ op: 'lookup', name: toolName, cwd, args: toolArgs, global: booleanParam(params.global) }); if (unavailable()) return; switch (lookup.status) { case 'trust-recorded': @@ -891,11 +895,12 @@ export function useDorControl({ detail.respond({ ok: false, error: 'unexpected tool host response' }); return; case 'ok': - command = lookup.run; + command = typeof lookup.run === 'string' ? lookup.run : dorCommandString([...lookup.run])!; + toolScope = lookup.scope; // Namespaced under the host-resolved tool name, so two tools in // one repo with scope-only keys stay distinct and a runtime // re-key cannot name another tool's key. - key = namespacedToolKey(lookup.name, lookup.key); + key = namespacedToolKey(lookup.name, lookup.key, lookup.scope); render = lookup.render; port = lookup.port; warnings = lookup.warnings; @@ -912,6 +917,7 @@ export function useDorControl({ }); return; case 'untrusted': { + const pendingCommand = typeof lookup.run === 'string' ? lookup.run : dorCommandString([...lookup.run])!; // Approval can only lead to a command gated on OSC 633. Reject // a shell known never to emit it before offering a prompt that // would otherwise approve, spawn, then silently drop the command. @@ -931,7 +937,8 @@ export function useDorControl({ // it keys on what the prompt is about. const matchesPending = (candidate: unknown) => { const waiting = toolPendingFromParams(candidate); - return waiting?.name === lookup.name && waiting.projectRoot === lookup.projectRoot; + return waiting?.name === lookup.name && waiting.projectRoot === lookup.projectRoot + && JSON.stringify(waiting.args ?? []) === JSON.stringify(toolArgs); }; const already = findSurfaceByParams(matchesPending); if (already) { @@ -942,7 +949,7 @@ export function useDorControl({ status: 'pending', surfaceId: already.id, surfaceRef: surfaceRefForId(already.id), - command: lookup.run, + command: pendingCommand, cwd, minimized: findSurfaceByParams(matchesPending)?.minimized ?? false, key: null, @@ -957,7 +964,8 @@ export function useDorControl({ // carried and applied once they do. const pendingMeta: ToolPending = { name: lookup.name, - run: lookup.run, + run: pendingCommand, + args: toolArgs, path: lookup.path, projectRoot: lookup.projectRoot, minimized: booleanParam(params.minimized), @@ -975,7 +983,7 @@ export function useDorControl({ deferTerminal: true, leafMeta: toolLeafMeta(lookup.name, { surfaceType: 'tool', - command: lookup.run, + command: pendingCommand, cwd, toolName: lookup.name, toolPending: pendingMeta, @@ -995,7 +1003,7 @@ export function useDorControl({ status: 'pending', surfaceId: pending.value.id, surfaceRef: pending.value.ref, - command: lookup.run, + command: pendingCommand, cwd, minimized: findSurfaceByParams(matchesPending)?.minimized ?? false, key: null, @@ -1021,6 +1029,7 @@ export function useDorControl({ command, cwd, toolRender: render, + ...(toolScope ? { toolScope } : {}), toolPort: port, ...(key ? { toolKey: key } : {}), ...(toolName ? { toolName } : {}), @@ -1047,7 +1056,8 @@ export function useDorControl({ // (docs/specs/dor-tool.md -> Identity and dedupe). if (key && !booleanParam(params.fresh)) { const matchesToolKey = (candidate: unknown) => - toolKeysEqual((candidate as { toolKey?: unknown } | null | undefined)?.toolKey, key); + (candidate as { toolScope?: unknown } | null | undefined)?.toolScope === toolScope + && toolKeysEqual((candidate as { toolKey?: unknown } | null | undefined)?.toolKey, key); const match = findSurfaceByParams(matchesToolKey); if (match) { const matchedCommand = toolCommandFromParams(lath.getMeta(match.id)?.params) || command; diff --git a/lib/src/components/wall/use-tool-serving.ts b/lib/src/components/wall/use-tool-serving.ts index da720008d..2b80d0b1d 100644 --- a/lib/src/components/wall/use-tool-serving.ts +++ b/lib/src/components/wall/use-tool-serving.ts @@ -98,7 +98,7 @@ export function useToolServing({ // dedupes (docs/specs/dor-tool.md -> Identity and dedupe). The // namespace that keeps process output from claiming another tool's key // is `namespacedToolKey`'s job; see its doc comment. - const announcedKey = namespacedToolKey(toolNameFromParams(leaf.params), announce?.key ?? null); + const announcedKey = namespacedToolKey(toolNameFromParams(leaf.params), announce?.key ?? null, leaf.params?.toolScope); if (announcedKey && !toolKeysEqual(leaf.params?.toolKey, announcedKey)) { lath.store.updateParams(leaf.id, { toolKey: announcedKey }); } diff --git a/lib/src/host/tool-host.test.ts b/lib/src/host/tool-host.test.ts index c76ce5691..4bc851559 100644 --- a/lib/src/host/tool-host.test.ts +++ b/lib/src/host/tool-host.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { mkdtemp, realpath, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; @@ -20,7 +20,7 @@ let repo = ''; let stateDir = ''; beforeEach(async () => { - repo = await mkdtemp(join(tmpdir(), 'dor-tool-host-')); + repo = await realpath(await mkdtemp(join(tmpdir(), 'dor-tool-host-'))); stateDir = join(repo, '.state'); await writeFile(join(repo, 'dormouse.yml'), YML); }); @@ -118,3 +118,40 @@ describe('createToolHost', () => { expect(result).toMatchObject({ status: 'error' }); }); }); + +it('resolves user tools without repo approval and keeps explicit project names ahead of them', async () => { + const path = join(repo, 'user.yml'); + await writeFile(path, `tools:\n storybook:\n run: [user-storybook, $ARGS]\n viewer:\n run: [viewer, $TARGET]\n prespawn_dedupe: [$TARGET]\n`); + const target = join(repo, 'a b; $(echo bad).md'); + await writeFile(target, 'hello'); + const host = createToolHost({ stateDir, userConfigPath: path }); + expect(await host.handle({ op: 'lookup', name: 'storybook', cwd: repo })).toMatchObject({ status: 'untrusted', run: 'pnpm storybook' }); + expect(await host.handle({ op: 'lookup', name: 'storybook', global: true, cwd: repo, args: ['--port', '1234'] })) + .toMatchObject({ status: 'ok', scope: 'user', run: ['user-storybook', '--port', '1234'] }); + expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: ['a b; $(echo bad).md'] })) + .toMatchObject({ status: 'ok', scope: 'user', run: ['viewer', target], key: [target] }); + expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: ['https://example.com/a.md'] })) + .toMatchObject({ status: 'error', message: expect.stringContaining('local file') }); + expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: [] })) + .toMatchObject({ status: 'error', message: expect.stringContaining('exactly one') }); +}); + +it('re-resolves input argv after project approval without shell interpolation', async () => { + await writeFile(join(repo, 'dormouse.yml'), 'tools:\n viewer:\n run: [viewer, $TARGET]\n prespawn_dedupe: [$PROJECT_ROOT, $TARGET]\n'); + const target = join(repo, "quoted ' document.md"); + await writeFile(target, 'hello'); + const host = createToolHost({ stateDir }); + const request = { op: 'lookup' as const, name: 'viewer', cwd: repo, args: [target] }; + expect(await host.handle(request)).toMatchObject({ status: 'untrusted', run: ['viewer', target] }); + await host.handle({ op: 'trust', kind: 'folder', projectRoot: repo }); + expect(await host.handle(request)).toMatchObject({ status: 'ok', run: ['viewer', target], key: [repo, target] }); +}); + +it('does not silently ignore malformed user configuration or input to shell strings', async () => { + const path = join(repo, 'user.yml'); + const host = createToolHost({ stateDir, userConfigPath: path }); + await writeFile(path, 'tools:\n viewer:\n run: [viewer, $PROJECT_ROOT]\n'); + expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo })).toMatchObject({ status: 'error' }); + expect(await host.handle({ op: 'lookup', name: 'storybook', cwd: repo, args: ['input'] })) + .toMatchObject({ status: 'error', message: expect.stringContaining('argument-list run') }); +}); diff --git a/lib/src/host/tool-host.ts b/lib/src/host/tool-host.ts index 8289defe4..7b07f4ddf 100644 --- a/lib/src/host/tool-host.ts +++ b/lib/src/host/tool-host.ts @@ -10,7 +10,8 @@ */ import type { ToolControlResult, ToolHostRequest } from '../lib/platform/tool-types'; import { resolveUpstreamUrl } from './git-upstream'; -import { resolveDedupeKey } from './tool-registry'; +import { resolveToolInput } from './tool-input'; +import { readUserToolFile, userToolConfigPath } from './tool-user-config'; import { FileToolTrustStore, MemoryToolTrustStore, @@ -30,7 +31,7 @@ export interface ToolHost { * run, which is annoying but never wrong, where inventing a location could put * a security decision somewhere the user cannot find to revoke it. */ -export function createToolHost(options: { stateDir?: string } = {}): ToolHost { +export function createToolHost(options: { stateDir?: string; userConfigPath?: string } = {}): ToolHost { const trust: ToolTrustStore = options.stateDir ? new FileToolTrustStore(options.stateDir) : new MemoryToolTrustStore(); @@ -52,22 +53,37 @@ export function createToolHost(options: { stateDir?: string } = {}): ToolHost { return { status: 'trust-recorded' }; } - const lookup = await lookupTool(request.name, request.cwd, trust); - if (lookup.status !== 'ok') { - // Every non-ok arm is already wire-shaped. - return lookup; - } - const { entry } = lookup; try { + const args = request.args ?? []; + const lookup = request.global ? { status: 'no-file' as const } : await lookupTool(request.name, request.cwd, trust); + if (lookup.status === 'no-file' || lookup.status === 'unknown-tool') { + const path = options.userConfigPath ?? userToolConfigPath(); + const file = await readUserToolFile(path); + const entry = file?.tools.get(request.name); + if (file && entry) { + const input = await resolveToolInput(entry, { projectRoot: null, cwd: request.cwd, args }); + return { status: 'ok', projectRoot: file.dir, path, name: entry.name, + ...input, scope: 'user', render: entry.render, port: entry.port, warnings: [...file.warnings] }; + } + if (request.global && file) return { status: 'unknown-tool', projectRoot: file.dir, path, names: [...file.tools.keys()].sort() }; + return lookup; + } + if (lookup.status === 'untrusted') { + const input = await resolveToolInput({ name: lookup.name, run: lookup.run, dedupeTemplate: null }, + { projectRoot: lookup.projectRoot, cwd: request.cwd, args }); + return { ...lookup, run: input.run }; + } + if (lookup.status !== 'ok') return lookup; + const { entry } = lookup; + const input = await resolveToolInput(entry, { projectRoot: lookup.projectRoot, cwd: request.cwd, args }); return { status: 'ok', projectRoot: lookup.projectRoot, path: lookup.path, name: entry.name, - run: entry.run, + ...input, render: entry.render, port: entry.port, - key: resolveDedupeKey(entry, { projectRoot: lookup.projectRoot, cwd: request.cwd }), warnings: [...lookup.file.warnings], }; } catch (error) { diff --git a/lib/src/host/tool-input.ts b/lib/src/host/tool-input.ts new file mode 100644 index 000000000..f95e7738c --- /dev/null +++ b/lib/src/host/tool-input.ts @@ -0,0 +1,40 @@ +import { realpath, stat } from 'node:fs/promises'; +import { isAbsolute, resolve } from 'node:path'; +import { resolveDedupeKey, ToolFileError, type ToolEntry } from './tool-registry'; + +/** A target is one existing regular file on this host. Resolve symlinks before + * keying, so two paths to the same document reveal the same Tool. */ +export async function resolveLocalToolTarget(input: string, cwd: string): Promise { + if (!input || input.includes('\0') || (!isAbsolute(input) && /^[a-z][a-z\d+.-]*:/i.test(input))) { + throw new ToolFileError('expected a local file path, not a URL or Surface handle'); + } + const target = await realpath(resolve(cwd, input)); + if (!(await stat(target)).isFile()) throw new ToolFileError(`not a regular file: ${input}`); + return target; +} + +export async function resolveToolInput( + entry: Pick, + context: { cwd: string; projectRoot: string | null; args: readonly string[] }, +): Promise<{ run: string | readonly string[]; key: string[] | null }> { + const { args } = context; + if (args.some(arg => typeof arg !== 'string' || arg.includes('\0'))) throw new ToolFileError('invalid tool arguments'); + const templates = [...(typeof entry.run === 'string' ? [] : entry.run), ...(entry.dedupeTemplate ?? [])]; + const needsTarget = templates.some(arg => /\$TARGET\b/.test(arg)); + if (needsTarget && args.length !== 1) throw new ToolFileError('$TARGET requires exactly one local file argument'); + const target = needsTarget ? await resolveLocalToolTarget(args[0], context.cwd) : undefined; + const key = resolveDedupeKey(entry, { ...context, target }); + if (typeof entry.run === 'string') { + if (args.length) throw new ToolFileError(`tool '${entry.name}': use an argument-list run to accept arguments`); + return { run: entry.run, key }; + } + const run = entry.run.flatMap(arg => arg === '$ARGS' ? [...args] : [arg.replace(/\$[A-Za-z_][A-Za-z0-9_]*/g, token => { + if (token === '$TARGET') return target!; + if (token === '$CWD') return context.cwd; + if (token === '$PROJECT_ROOT' && context.projectRoot !== null) return context.projectRoot; + throw new ToolFileError(`unknown substitution '${token}'`); + })]); + if (!entry.run.some(arg => arg === '$ARGS' || /\$TARGET\b/.test(arg))) run.push(...args); + if (!run[0]?.trim()) throw new ToolFileError('tool argument list must name an executable'); + return { run, key }; +} diff --git a/lib/src/host/tool-registry.ts b/lib/src/host/tool-registry.ts index cbf0b7c07..f0be9b9ca 100644 --- a/lib/src/host/tool-registry.ts +++ b/lib/src/host/tool-registry.ts @@ -29,7 +29,7 @@ const TOOL_PORT_MODES: readonly ToolPortMode[] = ['announced', 'auto']; export interface ToolEntry { readonly name: string; /** Command typed into the spawned shell, exactly as `dor ensure` types one. */ - readonly run: string; + readonly run: string | readonly string[]; /** Renderer for its browser; `iframe` when unstated. */ readonly render: ToolRender; /** Port-selection strategy; `announced` when unstated. */ @@ -56,7 +56,7 @@ export class ToolFileError extends Error {} /** Substitutions a `prespawn_dedupe` element may use. Closed set: an * unrecognized `$NAME` is a parse error, never a literal, because a typo kept * as a constant string dedupes across every worktree on the machine. */ -const SUBSTITUTIONS = ['$PROJECT_ROOT', '$CWD'] as const; +const SUBSTITUTIONS = ['$PROJECT_ROOT', '$CWD', '$TARGET'] as const; export type Substitution = (typeof SUBSTITUTIONS)[number]; // `$` followed by an identifier. Matches the whole token so an unknown one can @@ -89,7 +89,7 @@ function readDedupeTemplate(value: unknown, where: string): string[] { } /** Reject unknown `$NAME` tokens, and `$PROJECT_ROOT` outside a repo scope. */ -function validateSubstitutions(template: readonly string[], scope: ToolScope, where: string): void { +export function validateSubstitutions(template: readonly string[], scope: ToolScope, where: string): void { for (const element of template) { for (const token of element.match(SUBSTITUTION_TOKEN) ?? []) { if (!(SUBSTITUTIONS as readonly string[]).includes(token)) { @@ -147,8 +147,13 @@ export function parseToolFile( } const run = rawEntry.run; - if (typeof run !== 'string' || run.trim() === '') { - throw new ToolFileError(`${where}: 'run' is required and must be a non-empty string`); + if (Array.isArray(run)) { + if (!run.length || !run.every(arg => typeof arg === 'string' && !arg.includes('\0')) || !run[0].trim()) { + throw new ToolFileError(`${where}: 'run' must be a non-empty argument list`); + } + validateSubstitutions(run.filter(arg => arg !== '$ARGS'), scope, where); + } else if (typeof run !== 'string' || run.trim() === '') { + throw new ToolFileError(`${where}: 'run' is required and must be a non-empty string or argument list`); } let dedupeTemplate: string[] | null = null; @@ -178,7 +183,7 @@ export function parseToolFile( } const port = (rawPort as ToolPortMode | undefined) ?? 'announced'; - tools.set(name, { name, run: run.trim(), render, port, dedupeTemplate }); + tools.set(name, { name, run: typeof run === 'string' ? run.trim() : run, render, port, dedupeTemplate }); } return { scope, dir, tools, warnings }; @@ -190,13 +195,17 @@ export function parseToolFile( * one, so a null key means a fresh Surface every time. */ export function resolveDedupeKey( - entry: ToolEntry, - context: { projectRoot: string | null; cwd: string }, + entry: Pick, + context: { projectRoot: string | null; cwd: string; target?: string }, ): string[] | null { if (!entry.dedupeTemplate) return null; return entry.dedupeTemplate.map((element) => element.replace(SUBSTITUTION_TOKEN, (token) => { if (token === '$CWD') return context.cwd; + if (token === '$TARGET') { + if (!context.target) throw new ToolFileError(`tool '${entry.name}': $TARGET requires one local file argument`); + return context.target; + } if (token === '$PROJECT_ROOT') { // Unreachable via parseToolFile, which rejects $PROJECT_ROOT outside a // repo scope; guard anyway so a caller assembling entries by hand diff --git a/lib/src/host/tool-trust.ts b/lib/src/host/tool-trust.ts index d7e6819a2..036db211d 100644 --- a/lib/src/host/tool-trust.ts +++ b/lib/src/host/tool-trust.ts @@ -27,7 +27,7 @@ const TOOL_FILE_MAX_BYTES = 256 * 1024; /** Refuse stable symlinks on every host, then fstat and cap one descriptor. * POSIX also opens no-follow, closing the lstat/open replacement race there. */ -async function readToolFile(path: string): Promise { +export async function readToolFile(path: string): Promise { const entry = await lstat(path); if (entry.isSymbolicLink()) { throw new ToolFileError(`${path}: tool file must be a regular file, not a symbolic link`); @@ -423,7 +423,7 @@ export type ToolLookup = projectRoot: string; path: string; name: string; - run: string; + run: string | readonly string[]; /** Canonical upstream URL, or null when there is no resolvable remote — * the approval UI then offers only the folder grant. */ upstreamUrl: string | null; diff --git a/lib/src/host/tool-user-config.ts b/lib/src/host/tool-user-config.ts new file mode 100644 index 000000000..f3bf49060 --- /dev/null +++ b/lib/src/host/tool-user-config.ts @@ -0,0 +1,18 @@ +import { homedir } from 'node:os'; +import { dirname, isAbsolute, join } from 'node:path'; +import { parseToolFile, type ToolFile } from './tool-registry'; +import { readToolFile } from './tool-trust'; + +export function userToolConfigPath(): string { + const xdg = process.env.XDG_CONFIG_HOME; + return join(xdg && isAbsolute(xdg) ? xdg : join(homedir(), '.config'), 'dormouse', 'dormouse.yml'); +} + +export async function readUserToolFile(path: string): Promise { + try { + return parseToolFile(await readToolFile(path), { path, dir: dirname(path), scope: 'user' }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; + throw error; + } +} diff --git a/lib/src/lib/platform/tool-types.ts b/lib/src/lib/platform/tool-types.ts index 19e007e97..0f81528c9 100644 --- a/lib/src/lib/platform/tool-types.ts +++ b/lib/src/lib/platform/tool-types.ts @@ -7,7 +7,7 @@ */ export type ToolHostRequest = - | { op: 'lookup'; name: string; cwd: string } + | { op: 'lookup'; name: string; cwd: string; args?: string[]; global?: boolean } | { op: 'trust'; kind: 'upstream' | 'folder'; projectRoot: string }; /** Result of resolving a tool name. `ok` carries the rendered dedupe key: the @@ -20,7 +20,7 @@ export type ToolLookupResult = projectRoot: string; path: string; name: string; - run: string; + run: string | readonly string[]; /** Canonical upstream URL, or null when there is no resolvable remote. */ upstreamUrl: string | null; } @@ -30,9 +30,10 @@ export type ToolLookupResult = projectRoot: string; path: string; name: string; - run: string; + run: string | readonly string[]; /** Renderer for the tool's browser once it serves; 'iframe' by default. */ render: 'iframe' | 'ab-screencast'; + scope?: 'user'; /** How to pick the port to frame absent an announcement; 'announced' by * default, meaning nothing is framed without OSC 367. */ port: 'announced' | 'auto'; diff --git a/lib/src/lib/session-restore.ts b/lib/src/lib/session-restore.ts index 4931ce256..0ff952709 100644 --- a/lib/src/lib/session-restore.ts +++ b/lib/src/lib/session-restore.ts @@ -54,7 +54,7 @@ export function restoreSession(platform: PlatformAdapter, sources: RestoreSource leafMeta: Object.fromEntries(recoverable.map(pane => [pane.id, pane.surfaceType === 'tool' ? { component: 'tool', tabComponent: 'tool', title: pane.title, params: { surfaceType: 'tool', command: pane.command, cwd: pane.cwd, - toolName: pane.tool?.name, toolRender: pane.tool?.render ?? 'iframe', + toolScope: pane.tool?.scope, toolName: pane.tool?.name, toolRender: pane.tool?.render ?? 'iframe', toolPort: pane.tool?.port ?? 'announced', toolKey: pane.tool?.key }, } : { component: 'terminal', tabComponent: 'terminal', title: pane.title }])), }; diff --git a/lib/src/lib/session-save.ts b/lib/src/lib/session-save.ts index 430bbc6da..4dda12c97 100644 --- a/lib/src/lib/session-save.ts +++ b/lib/src/lib/session-save.ts @@ -181,7 +181,7 @@ function toolMetadataFromParams(params: Record | undefined): Pe const key = Array.isArray(params.toolKey) && params.toolKey.every((part) => typeof part === 'string') ? params.toolKey as string[] : undefined; - return { ...(name ? { name } : {}), render, port, ...(key ? { key } : {}) }; + return { ...(name ? { name } : {}), ...(params.toolScope === 'user' ? { scope: 'user' as const } : {}), render, port, ...(key ? { key } : {}) }; } function persistedVisiblePaneTitle(title: string): string { diff --git a/lib/src/lib/session-types.ts b/lib/src/lib/session-types.ts index 798df74d5..62171fc64 100644 --- a/lib/src/lib/session-types.ts +++ b/lib/src/lib/session-types.ts @@ -16,6 +16,7 @@ export type PersistedSurfaceType = 'terminal' | 'browser' | 'tool'; * is respawned. Derived browser state (URL/session/port conflict) never enters * this projection. */ export interface PersistedToolMetadata { + scope?: 'user'; name?: string; render: 'iframe' | 'ab-screencast'; port: 'announced' | 'auto'; @@ -168,6 +169,7 @@ function isPersistedToolMetadataShape(value: unknown): boolean { if (!isRecord(value)) return false; return ( (value.name === undefined || typeof value.name === 'string') && + (value.scope === undefined || value.scope === 'user') && (value.render === 'iframe' || value.render === 'ab-screencast') && (value.port === 'announced' || value.port === 'auto') && (value.key === undefined || (Array.isArray(value.key) && value.key.every((part) => typeof part === 'string'))) diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 5a16b62d7..016f50ed0 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -7,7 +7,7 @@ "docs/specs/deploy.md": 1900, "docs/specs/dor-browser.md": 4600, "docs/specs/dor-cli.md": 5950, - "docs/specs/dor-tool.md": 2700, + "docs/specs/dor-tool.md": 2850, "docs/specs/glossary.md": 3000, "docs/specs/layout.md": 8800, "docs/specs/mobile-terminal-ui.md": 1950, @@ -19,7 +19,7 @@ "docs/specs/remote-security-model.md": 4800, "docs/specs/security-audit.md": 1750, "docs/specs/security-ci.md": 2500, - "docs/specs/security-local.md": 2850, + "docs/specs/security-local.md": 2900, "docs/specs/security-remote.md": 5850, "docs/specs/security-supply-chain.md": 1200, "docs/specs/security.md": 1900, From b253955c9d5c306ff876315a635ce0cdb6cb321a Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 15 Sep 2026 16:47:17 -0700 Subject: [PATCH 02/10] Validate Tool inputs before approval and explain failed launches --- docs/specs/dor-tool.md | 2 +- docs/specs/security-local.md | 2 +- lib/src/components/Wall.test.tsx | 25 ++++++++++++++++++++ lib/src/components/Wall.tsx | 9 +++++++- lib/src/components/wall/ToolApproval.tsx | 2 ++ lib/src/components/wall/browser-surface.ts | 1 + lib/src/host/tool-host.test.ts | 27 +++++++++++++++++++++- lib/src/host/tool-host.ts | 13 ++++------- lib/src/host/tool-registry.test.ts | 6 +++++ lib/src/host/tool-registry.ts | 3 +++ lib/src/host/tool-trust.ts | 27 +++++++++++++++------- lib/src/host/tool-user-config.ts | 2 +- scripts/spec-word-budgets.json | 2 +- 13 files changed, 98 insertions(+), 23 deletions(-) diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index 8ed1ae3e8..9c288b3d5 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -50,7 +50,7 @@ Source of truth: `surfaceKindFromParams` / `isToolParams` in `lib/src/components **Must pass named-tool inputs as argument values, never substitute them into a shell-command string.** String `run` accepts no arguments and remains literal shell syntax. List `run` expands `$TARGET`, `$CWD`, and `$PROJECT_ROOT` within elements; a whole `$ARGS` element expands all input arguments. Without `$ARGS` or `$TARGET` in the list, append the inputs. The renderer quotes the resulting argv for its configured shell. -**Must require exactly one existing regular local file when `$TARGET` appears in the run list or dedupe key.** Resolve relative paths against the invocation CWD and follow symlinks to a canonical absolute path before substitution and reuse. Reject URLs, directories, and missing files. Pending approval retains the original arguments and distinguishes requests with different inputs; approval re-resolves them before launch. +**Must require exactly one existing regular local file when `$TARGET` appears in the run list or dedupe key.** Resolve relative paths against the invocation CWD and follow symlinks to a canonical absolute path before substitution and reuse. Reject URLs, directories, and missing files. Validate run and key inputs before showing approval. Pending approval retains the original arguments and distinguishes requests with different inputs; approval re-resolves them before launch. A failed re-resolution leaves the pane pending with an error and no PTY, allowing retry or closure. Source of truth: `lookupTool` in `lib/src/host/tool-trust.ts`; `parseToolFile` / `resolveDedupeKey` in `lib/src/host/tool-registry.ts`; `resolveToolInput` in `lib/src/host/tool-input.ts`; `readUserToolFile` in `lib/src/host/tool-user-config.ts`; `lib/src/host/tool-host.test.ts`, `lib/src/host/tool-registry.test.ts`. diff --git a/docs/specs/security-local.md b/docs/specs/security-local.md index 5700d8b7b..b21172888 100644 --- a/docs/specs/security-local.md +++ b/docs/specs/security-local.md @@ -204,7 +204,7 @@ Source of truth: `context` in `standalone/sidecar/pty-core.js`; `attachRouter` i **Must keep named-tool inputs as argv until the renderer quotes them for the target shell.** User configuration is the local user's authority; a project name cannot replace a user Tool during user-only lookup. Resolution belongs to `docs/specs/dor-tool.md` → Declaring tools. -**Must derive the grant key in the host**, using the canonical upstream URL or project-root folder; a renderer request cannot supply an arbitrary grant URL. **Must bound config reads and refuse symlinks on every host.** +**Must derive the grant key in the host**, using the canonical upstream URL or project-root folder; a renderer request cannot supply an arbitrary grant URL. **Must bound config reads and refuse repo-config symlinks on every host.** The user config may follow a dotfiles symlink; its opened descriptor must still be a bounded regular file. **An upstream grant trusts the claimed URL, not authenticated checkout provenance.** A supplied directory containing its own `.git/config` can claim an already-granted upstream; folder-only grants limit this sharing. **Must not describe the chrome gesture as a boundary against other processes running as the user**; the local account model is The dor control socket above. diff --git a/lib/src/components/Wall.test.tsx b/lib/src/components/Wall.test.tsx index b041614f3..9e37ef773 100644 --- a/lib/src/components/Wall.test.tsx +++ b/lib/src/components/Wall.test.tsx @@ -1457,6 +1457,31 @@ describe('Wall on the Lath engine', () => { } }); + it('retains the approval pane and explains an input that disappeared before approval', async () => { + setToolsEnabled(true); + let calls = 0; + Object.assign(fake, { toolControl: vi.fn(async (request: { op: string }) => { + if (request.op === 'trust') return { status: 'trust-recorded' }; + if (calls++) return { status: 'error', message: 'The selected file is missing' }; + return { status: 'untrusted', projectRoot: '/repo', path: '/repo/dormouse.yml', name: 'viewer', run: ['view', '/repo/file.md'], upstreamUrl: null }; + }) }); + try { + await act(async () => root.render()); + await flush(); + const respond = vi.fn(); + await act(async () => window.dispatchEvent(new CustomEvent('dormouse:control-request', { detail: { + method: SURFACE_CONTROL_METHODS.tool, params: { name: 'viewer', cwd: '/repo', args: ['file.md'] }, respond, + } }))); + const id = respond.mock.calls[0][0].result.surfaceId; + const allow = [...container.querySelectorAll('button')].find(button => button.textContent?.includes('Always allow for folder'))!; + await act(async () => allow.click()); + await flush(); + expect(container.querySelector('[role="alert"]')?.textContent).toBe('The selected file is missing'); + expect(container.querySelector(`[data-lath-leaf="${id}"]`)).not.toBeNull(); + expect(container.querySelector(`[data-session-id="${id}"]`)).toBeNull(); + } finally { setToolsEnabled(false); } + }); + it('keeps pending file inputs distinct and quotes argv after approval', async () => { setToolsEnabled(true); let trusted = false; diff --git a/lib/src/components/Wall.tsx b/lib/src/components/Wall.tsx index e4bbec3ca..3c156ee6f 100644 --- a/lib/src/components/Wall.tsx +++ b/lib/src/components/Wall.tsx @@ -1572,6 +1572,11 @@ export function Wall({ } if (toolApprovalsInFlightRef.current.has(id)) return; toolApprovalsInFlightRef.current.add(id); + const showFailure = (message: string) => { + if (!closingWorkspaceRef.current && lath.getMeta(id) && !lath.isDying(id) && !isSurfaceClosing(id)) { + lath.store.updateParams(id, { toolPending: { ...pending, error: message } }); + } + }; try { const platform = getPlatform(); @@ -1588,7 +1593,7 @@ export function Wall({ const cwd = typeof meta?.params?.cwd === 'string' ? meta.params.cwd : pending.projectRoot; const resolved = await platform.toolControl?.({ op: 'lookup', name: pending.name, cwd, args: pending.args }); if (resolved?.status !== 'ok') { - await closeSurface(id); + showFailure(resolved?.status === 'error' ? resolved.message : 'The Tool is no longer available. Check its configuration and try again.'); return; } @@ -1623,6 +1628,8 @@ export function Wall({ getOrCreateTerminal(id); minimizePane(id); } + } catch (error) { + showFailure(error instanceof Error ? error.message : String(error)); } finally { toolApprovalsInFlightRef.current.delete(id); } diff --git a/lib/src/components/wall/ToolApproval.tsx b/lib/src/components/wall/ToolApproval.tsx index c7861fb04..e19389e66 100644 --- a/lib/src/components/wall/ToolApproval.tsx +++ b/lib/src/components/wall/ToolApproval.tsx @@ -30,6 +30,8 @@ export function ToolApproval({ params, id, onResolve }: PaneProps & {
and then open a browser
+ {pending.error ?
{pending.error}
: null} +
{/* Omitted when git named no remote: there is no URL to key a grant on, so the folder is the only honest scope. */} diff --git a/lib/src/components/wall/browser-surface.ts b/lib/src/components/wall/browser-surface.ts index 8e4351869..d6c16d81a 100644 --- a/lib/src/components/wall/browser-surface.ts +++ b/lib/src/components/wall/browser-surface.ts @@ -61,6 +61,7 @@ export interface ToolPending { readonly name: string; readonly run: string; readonly args?: string[]; + readonly error?: string; readonly path: string; readonly projectRoot: string; /** Requested at launch; applied after approval, since a pane the user cannot diff --git a/lib/src/host/tool-host.test.ts b/lib/src/host/tool-host.test.ts index 4bc851559..672e9891a 100644 --- a/lib/src/host/tool-host.test.ts +++ b/lib/src/host/tool-host.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, realpath, rm, writeFile } from 'node:fs/promises'; +import { mkdtemp, realpath, rm, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; @@ -155,3 +155,28 @@ it('does not silently ignore malformed user configuration or input to shell stri expect(await host.handle({ op: 'lookup', name: 'storybook', cwd: repo, args: ['input'] })) .toMatchObject({ status: 'error', message: expect.stringContaining('argument-list run') }); }); + + +it('validates target-only dedupe inputs before asking for project approval', async () => { + await writeFile(join(repo, 'dormouse.yml'), 'tools:\n view:\n run: [viewer, $ARGS]\n prespawn_dedupe: [$TARGET]\n'); + const host = createToolHost({ stateDir }); + expect(await host.handle({ op: 'lookup', name: 'view', cwd: repo, args: ['missing.md'] })) + .toMatchObject({ status: 'error' }); + expect(await host.handle({ op: 'lookup', name: 'view', cwd: repo, args: [] })) + .toMatchObject({ status: 'error', message: expect.stringContaining('exactly one') }); +}); + +it('names the actual user path when --global has no config', async () => { + const path = join(repo, 'absent-user.yml'); + expect(await createToolHost({ userConfigPath: path }).handle({ op: 'lookup', name: 'view', cwd: repo, global: true })) + .toMatchObject({ status: 'unknown-tool', path, names: [] }); +}); + +it.skipIf(process.platform === 'win32')('follows user-config dotfile symlinks while keeping project symlinks inert', async () => { + const path = join(repo, 'user-link.yml'); + const source = join(repo, 'dotfiles.yml'); + await writeFile(source, 'tools:\n scratch:\n run: echo hi\n'); + await symlink(source, path); + const result = await createToolHost({ userConfigPath: path }).handle({ op: 'lookup', name: 'scratch', cwd: repo, global: true }); + expect(result).toMatchObject({ status: 'ok', scope: 'user', run: 'echo hi' }); +}); diff --git a/lib/src/host/tool-host.ts b/lib/src/host/tool-host.ts index 7b07f4ddf..c2f838788 100644 --- a/lib/src/host/tool-host.ts +++ b/lib/src/host/tool-host.ts @@ -8,6 +8,7 @@ * chrome. Everything crossing back to the webview is plain JSON — the * standalone path goes through Rust. */ +import { dirname } from 'node:path'; import type { ToolControlResult, ToolHostRequest } from '../lib/platform/tool-types'; import { resolveUpstreamUrl } from './git-upstream'; import { resolveToolInput } from './tool-input'; @@ -55,7 +56,7 @@ export function createToolHost(options: { stateDir?: string; userConfigPath?: st try { const args = request.args ?? []; - const lookup = request.global ? { status: 'no-file' as const } : await lookupTool(request.name, request.cwd, trust); + const lookup = request.global ? { status: 'no-file' as const } : await lookupTool(request.name, request.cwd, trust, undefined, undefined, args); if (lookup.status === 'no-file' || lookup.status === 'unknown-tool') { const path = options.userConfigPath ?? userToolConfigPath(); const file = await readUserToolFile(path); @@ -65,17 +66,11 @@ export function createToolHost(options: { stateDir?: string; userConfigPath?: st return { status: 'ok', projectRoot: file.dir, path, name: entry.name, ...input, scope: 'user', render: entry.render, port: entry.port, warnings: [...file.warnings] }; } - if (request.global && file) return { status: 'unknown-tool', projectRoot: file.dir, path, names: [...file.tools.keys()].sort() }; + if (request.global) return { status: 'unknown-tool', projectRoot: dirname(path), path, names: [...(file?.tools.keys() ?? [])].sort() }; return lookup; } - if (lookup.status === 'untrusted') { - const input = await resolveToolInput({ name: lookup.name, run: lookup.run, dedupeTemplate: null }, - { projectRoot: lookup.projectRoot, cwd: request.cwd, args }); - return { ...lookup, run: input.run }; - } if (lookup.status !== 'ok') return lookup; - const { entry } = lookup; - const input = await resolveToolInput(entry, { projectRoot: lookup.projectRoot, cwd: request.cwd, args }); + const { entry, input } = lookup; return { status: 'ok', projectRoot: lookup.projectRoot, diff --git a/lib/src/host/tool-registry.test.ts b/lib/src/host/tool-registry.test.ts index a0030a546..bef504fb8 100644 --- a/lib/src/host/tool-registry.test.ts +++ b/lib/src/host/tool-registry.test.ts @@ -194,3 +194,9 @@ describe("this repo's own dormouse.yml", () => { } }); }); + + +it('rejects a shell command with a target-only dedupe key at declaration time', () => { + expect(() => parse('tools:\n viewer:\n run: view\n prespawn_dedupe: [$TARGET]\n')) + .toThrow('$TARGET in prespawn_dedupe requires an argument-list run'); +}); diff --git a/lib/src/host/tool-registry.ts b/lib/src/host/tool-registry.ts index f0be9b9ca..57b5911bb 100644 --- a/lib/src/host/tool-registry.ts +++ b/lib/src/host/tool-registry.ts @@ -160,6 +160,9 @@ export function parseToolFile( if (rawEntry.prespawn_dedupe !== undefined && rawEntry.prespawn_dedupe !== null) { dedupeTemplate = readDedupeTemplate(rawEntry.prespawn_dedupe, where); validateSubstitutions(dedupeTemplate, scope, where); + if (typeof run === 'string' && dedupeTemplate.some(arg => /\$TARGET\b/.test(arg))) { + throw new ToolFileError(`${where}: $TARGET in prespawn_dedupe requires an argument-list run`); + } // A repo-local key with no project scope dedupes across every checkout // that declares the name, so a second worktree's tool would reveal the // first instead of starting. Warn, not error: a repo-declared diff --git a/lib/src/host/tool-trust.ts b/lib/src/host/tool-trust.ts index 036db211d..d97fd0f7b 100644 --- a/lib/src/host/tool-trust.ts +++ b/lib/src/host/tool-trust.ts @@ -15,6 +15,7 @@ import { randomUUID } from 'node:crypto'; import { dirname, join, resolve } from 'node:path'; import { ToolFileError, parseToolFile, type ToolEntry, type ToolFile } from './tool-registry'; import { resolveUpstreamUrl } from './git-upstream'; +import { resolveToolInput } from './tool-input'; export const TOOL_FILE_NAME = 'dormouse.yml'; /** @@ -25,18 +26,20 @@ export const TOOL_FILE_NAME = 'dormouse.yml'; */ const TOOL_FILE_MAX_BYTES = 256 * 1024; -/** Refuse stable symlinks on every host, then fstat and cap one descriptor. +/** Refuse repo-config symlinks; user config may follow a dotfiles link. + * Both paths fstat and cap one descriptor. * POSIX also opens no-follow, closing the lstat/open replacement race there. */ -export async function readToolFile(path: string): Promise { +export async function readToolFile(path: string, allowSymlink = false): Promise { const entry = await lstat(path); - if (entry.isSymbolicLink()) { + if (entry.isSymbolicLink() && !allowSymlink) { throw new ToolFileError(`${path}: tool file must be a regular file, not a symbolic link`); } + if (!entry.isSymbolicLink() && !entry.isFile()) throw new ToolFileError(`${path}: tool file must be a regular file`); let file; try { - const noFollow = typeof constants.O_NOFOLLOW === 'number' ? constants.O_NOFOLLOW : 0; - file = await open(path, constants.O_RDONLY | noFollow); + const noFollow = !allowSymlink && typeof constants.O_NOFOLLOW === 'number' ? constants.O_NOFOLLOW : 0; + file = await open(path, constants.O_RDONLY | noFollow | (constants.O_NONBLOCK ?? 0)); } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code === 'ELOOP' || code === 'EMLINK') { @@ -429,7 +432,7 @@ export type ToolLookup = upstreamUrl: string | null; } | { status: 'error'; message: string } - | { status: 'ok'; projectRoot: string; path: string; file: ToolFile; entry: ToolEntry }; + | { status: 'ok'; projectRoot: string; path: string; file: ToolFile; entry: ToolEntry; input: Awaited> }; /** * Find, parse, and trust-check the entry named `name` for a caller in `cwd`. @@ -444,6 +447,7 @@ export async function lookupTool( trust: ToolTrustStore, readTextFile?: (path: string) => Promise, resolveUpstream: (dir: string) => Promise = resolveUpstreamUrl, + args: readonly string[] = [], ): Promise { let found; try { @@ -473,20 +477,27 @@ export async function lookupTool( }; } + let input: Awaited>; + try { + input = await resolveToolInput(entry, { projectRoot: found.dir, cwd, args }); + } catch (error) { + return { status: 'error', message: error instanceof Error ? error.message : String(error) }; + } + // Either grant covers this project: the upstream every worktree shares, or // this folder alone. Resolved before the check so the approval UI can offer // both, and so a hit on either short-circuits identically. const upstreamUrl = await resolveUpstream(found.dir); const keys = [folderGrantKey(found.dir), ...(upstreamUrl ? [upstreamGrantKey(upstreamUrl)] : [])]; if (await trust.isTrusted(keys)) { - return { status: 'ok', projectRoot: found.dir, path: found.path, file, entry }; + return { status: 'ok', projectRoot: found.dir, path: found.path, file, entry, input }; } return { status: 'untrusted', projectRoot: found.dir, path: found.path, name: entry.name, - run: entry.run, + run: input.run, upstreamUrl, }; } diff --git a/lib/src/host/tool-user-config.ts b/lib/src/host/tool-user-config.ts index f3bf49060..0e8896e2b 100644 --- a/lib/src/host/tool-user-config.ts +++ b/lib/src/host/tool-user-config.ts @@ -10,7 +10,7 @@ export function userToolConfigPath(): string { export async function readUserToolFile(path: string): Promise { try { - return parseToolFile(await readToolFile(path), { path, dir: dirname(path), scope: 'user' }); + return parseToolFile(await readToolFile(path, true), { path, dir: dirname(path), scope: 'user' }); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; throw error; diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 016f50ed0..61f5f9f60 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -7,7 +7,7 @@ "docs/specs/deploy.md": 1900, "docs/specs/dor-browser.md": 4600, "docs/specs/dor-cli.md": 5950, - "docs/specs/dor-tool.md": 2850, + "docs/specs/dor-tool.md": 2900, "docs/specs/glossary.md": 3000, "docs/specs/layout.md": 8800, "docs/specs/mobile-terminal-ui.md": 1950, From 9a4dca209935950a5e358d10b12abc8056219603 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 15 Sep 2026 17:56:20 -0700 Subject: [PATCH 03/10] Simplify Tool input plumbing after review 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 --- dor/src/cli.ts | 6 -- dor/src/commands/split.ts | 2 +- dor/src/commands/tool.ts | 53 +++++----- dor/src/commands/types.ts | 10 +- lib/src/components/Wall.tsx | 26 ++--- lib/src/components/wall/ToolApproval.tsx | 2 +- lib/src/components/wall/browser-surface.ts | 17 ++- lib/src/components/wall/use-dor-control.ts | 19 +++- lib/src/components/wall/use-tool-serving.ts | 2 +- lib/src/host/tool-host.test.ts | 109 ++++++++++---------- lib/src/host/tool-host.ts | 61 ++++++----- lib/src/host/tool-input.ts | 41 +++++--- lib/src/host/tool-registry.test.ts | 1 - lib/src/host/tool-registry.ts | 61 +++++++---- lib/src/host/tool-trust.test.ts | 20 ++-- lib/src/host/tool-trust.ts | 29 +++--- lib/src/host/tool-user-config.ts | 2 +- 17 files changed, 262 insertions(+), 199 deletions(-) diff --git a/dor/src/cli.ts b/dor/src/cli.ts index faf7d6b07..707973972 100644 --- a/dor/src/cli.ts +++ b/dor/src/cli.ts @@ -206,18 +206,12 @@ export async function runCli(rawArgv: string[], options: CliOptions = {}): Promi if (!check.ok) return fail(check.message); } - // stricli discards the `--` escape sequence during parsing, so capture its - // presence here (pre-parse) for commands that must distinguish an empty - // command tail from none — e.g. `dor split --` vs bare `dor split`. - const hasArgumentEscape = args.includes('--'); - const capture = createCaptureProcess(options.env); await runStricli(APPLICATION, commandName ? [commandName, ...args] : [], { process: capture.process, forCommand: (): DorCommandContext => ({ process: capture.process, options, - hasArgumentEscape, commandArgs: args, }), }); diff --git a/dor/src/commands/split.ts b/dor/src/commands/split.ts index 3b962ae9c..598e85d9d 100644 --- a/dor/src/commands/split.ts +++ b/dor/src/commands/split.ts @@ -148,7 +148,7 @@ async function runSplitCommand(this: DorCommandContext, flags: SplitFlags, ...co // Only a bare `dor split` (no `--`, no command) steals focus; a `--` tail // and an initial command alike leave it on the caller. The CLI owns the // whole decision so the host can honor the field as sent. - focusNeutral: this.hasArgumentEscape || command !== undefined, + focusNeutral: this.commandArgs.includes('--') || command !== undefined, ...workspaceParam(flags.workspace), }); writeStdout(this, renderSplitResponse(response, flags.json === true)); diff --git a/dor/src/commands/tool.ts b/dor/src/commands/tool.ts index e2d153a9c..2dd1097f7 100644 --- a/dor/src/commands/tool.ts +++ b/dor/src/commands/tool.ts @@ -42,10 +42,33 @@ const BOOLEAN_FLAGS = new Set(['--json', '--minimize', '--fresh', '--global']); * same pre-parse contract `dor ensure` uses. Keep the flag lists above in sync * with `parameters.flags`. */ -export function validateToolArgs(args: string[]): ParseResult { +export function validateToolArgs(args: readonly string[]): ParseResult { const delimiterIndex = args.indexOf('--'); - const head = delimiterIndex === -1 ? args : args.slice(0, delimiterIndex); + const head = headPositionals(args); + if (!head.ok) return head; + const { value: positionals } = head; + + if (delimiterIndex === -1) { + if (positionals.length === 0) { + return { ok: false, message: 'dor tool requires a tool name or -- ' }; + } + return { ok: true, value: undefined }; + } + if (positionals.length > 0) return { ok: true, value: undefined }; + if (args.slice(0, delimiterIndex).includes('--global')) return { ok: false, message: '--global requires a named tool' }; + if (args.slice(delimiterIndex + 1).join(' ').trim() === '') { + return { ok: false, message: 'dor tool requires a command after --' }; + } + return { ok: true, value: undefined }; +} + +/** The positionals before `--`: a tool name and its dash-free inputs. Non-empty + * means the named form; stricli discards the separator, so this is the one + * walk that can tell `dor tool viewer -- --flag` from `dor tool -- viewer`. */ +function headPositionals(args: readonly string[]): ParseResult { + const delimiterIndex = args.indexOf('--'); + const head = delimiterIndex === -1 ? args : args.slice(0, delimiterIndex); const positionals: string[] = []; for (let index = 0; index < head.length; index += 1) { const arg = head[index]; @@ -59,20 +82,7 @@ export function validateToolArgs(args: string[]): ParseResult { if (arg.startsWith('-')) return { ok: false, message: `unknown option '${arg}'` }; positionals.push(arg); } - - if (delimiterIndex === -1) { - if (positionals.length === 0) { - return { ok: false, message: 'dor tool requires a tool name or -- ' }; - } - return { ok: true, value: undefined }; - } - - if (positionals.length > 0) return { ok: true, value: undefined }; - if (head.includes('--global')) return { ok: false, message: '--global requires a named tool' }; - if (args.slice(delimiterIndex + 1).join(' ').trim() === '') { - return { ok: false, message: 'dor tool requires a command after --' }; - } - return { ok: true, value: undefined }; + return { ok: true, value: positionals }; } export const toolCommand: Command = { @@ -154,14 +164,9 @@ JSON output: }; async function runToolCommand(this: DorCommandContext, flags: ToolFlags, ...rest: string[]): Promise { - // The name precedes `--` for a named invocation; an anonymous command has - // only flags before it. stricli discards the separator, so inspect raw argv. - const head = this.commandArgs.slice(0, this.hasArgumentEscape ? this.commandArgs.indexOf('--') : undefined); - let named = false; - for (let i = 0; i < head.length; i++) { - if (FLAGS_WITH_VALUES.has(head[i])) i++; - else if (!BOOLEAN_FLAGS.has(head[i])) named = true; - } + // `validateToolArgs` already accepted this argv, so the walk cannot fail. + const head = headPositionals(this.commandArgs); + const named = head.ok && head.value.length > 0; if (named && rest.length === 0) { return new Error('dor tool requires a tool name or -- '); } diff --git a/dor/src/commands/types.ts b/dor/src/commands/types.ts index 33a42d7bc..aeec82296 100644 --- a/dor/src/commands/types.ts +++ b/dor/src/commands/types.ts @@ -499,11 +499,11 @@ export interface DorCommandContext extends CommandContext { * command's wins. */ readonly process: StricliProcess; readonly options: CliOptions; - /** Whether the raw argv carried the `--` argument-escape sequence. stricli - * consumes `--` and leaves no trace in the parsed positionals, so this is the - * only way a command can tell `dor split --` (empty tail) from a bare - * `dor split`. Computed once in `cli.ts` from the pre-parse argv. */ - readonly hasArgumentEscape: boolean; + /** The raw argv after the command name, as `cli.ts` saw it before stricli + * parsed it. stricli consumes `--` and leaves no trace in the parsed + * positionals, so this is the only way a command can tell `dor split --` + * (empty tail) from a bare `dor split`, or a named `dor tool` from an + * anonymous one. */ readonly commandArgs: readonly string[]; } diff --git a/lib/src/components/Wall.tsx b/lib/src/components/Wall.tsx index 74442c016..d03fabdf6 100644 --- a/lib/src/components/Wall.tsx +++ b/lib/src/components/Wall.tsx @@ -1,4 +1,3 @@ -import { buildShellCommandForKind, shellCommandKind } from 'dor/commands/shell-quote'; import { captureToolParams } from './wall/tool-transfer'; import { isWorkspaceTransferPending } from '../lib/window-session-aggregator'; import { TerminalContextContext, type TerminalContextOpenOptions, type TerminalContextState } from './wall/wall-context'; @@ -60,7 +59,7 @@ import { createTerminalPaneState, deriveSurfaceLabel, } from '../lib/terminal-state'; -import { getPlatform, PLATFORM_STRING } from '../lib/platform'; +import { getPlatform } from '../lib/platform'; import type { Surface as DorSurface, ResolvedSplitDirection as DorResolvedSplitDirection, @@ -107,7 +106,8 @@ import { useWallKeyboard } from './wall/use-wall-keyboard'; import { useSessionPersistence } from './wall/use-session-persistence'; import { useDevServerPortCorrelation } from './wall/use-dev-server-ports'; import { useAlertSpeech } from './wall/use-alert-speech'; -import { useDorControl } from './wall/use-dor-control'; +import { toolRunCommand, useDorControl } from './wall/use-dor-control'; +import { errorText } from './wall/dor-control-shared'; import { useWindowFocused } from './wall/use-window-focused'; import { DialogKeyboardContext, @@ -1573,10 +1573,13 @@ export function Wall({ } if (toolApprovalsInFlightRef.current.has(id)) return; toolApprovalsInFlightRef.current.add(id); + // Whether the prompt's pane is still ours to write to after an await. + const stillPending = () => !closingWorkspaceRef.current && !isWorkspaceTransferPending(effectiveWorkspaceId) + && !!lath.getMeta(id) && !lath.isDying(id) && !isSurfaceClosing(id); + // A failed launch keeps the prompt up with its reason, so the user can fix + // the input and approve again or close the pane. const showFailure = (message: string) => { - if (!closingWorkspaceRef.current && lath.getMeta(id) && !lath.isDying(id) && !isSurfaceClosing(id)) { - lath.store.updateParams(id, { toolPending: { ...pending, error: message } }); - } + if (stillPending()) lath.store.updateParams(id, { toolPending: { ...pending, error: message } }); }; try { @@ -1599,15 +1602,14 @@ export function Wall({ return; } - if (closingWorkspaceRef.current || isWorkspaceTransferPending(effectiveWorkspaceId) || !lath.getMeta(id) || lath.isDying(id) || isSurfaceClosing(id)) return; - const command = typeof resolved.run === 'string' ? resolved.run - : buildShellCommandForKind(shellCommandKind(getDefaultShellOpts()?.shell, PLATFORM_STRING), resolved.run); + if (!stillPending()) return; + const command = toolRunCommand(resolved.run); lath.store.updateParams(id, { command, - toolScope: resolved.scope, + ...(resolved.scope ? { toolScope: resolved.scope } : {}), toolRender: resolved.render, toolPort: resolved.port, - ...(resolved.key ? { toolKey: namespacedToolKey(resolved.name, resolved.key, resolved.scope) } : {}), + ...(resolved.key ? { toolKey: namespacedToolKey(resolved.name, resolved.key) } : {}), }); // Hand the leaf its command only now. The approval marker stays in place // until after this write, so TerminalPanel cannot consume default options @@ -1631,7 +1633,7 @@ export function Wall({ minimizePane(id); } } catch (error) { - showFailure(error instanceof Error ? error.message : String(error)); + showFailure(errorText(error)); } finally { toolApprovalsInFlightRef.current.delete(id); } diff --git a/lib/src/components/wall/ToolApproval.tsx b/lib/src/components/wall/ToolApproval.tsx index e19389e66..cd4176db4 100644 --- a/lib/src/components/wall/ToolApproval.tsx +++ b/lib/src/components/wall/ToolApproval.tsx @@ -30,7 +30,7 @@ export function ToolApproval({ params, id, onResolve }: PaneProps & {
and then open a browser
- {pending.error ?
{pending.error}
: null} + {pending.error ?
{pending.error}
: null}
{/* Omitted when git named no remote: there is no URL to key a grant on, diff --git a/lib/src/components/wall/browser-surface.ts b/lib/src/components/wall/browser-surface.ts index d6c16d81a..9e737b008 100644 --- a/lib/src/components/wall/browser-surface.ts +++ b/lib/src/components/wall/browser-surface.ts @@ -18,6 +18,8 @@ type BrowserParamsLike = { url?: unknown; /** Tool only: the ports found when autobind refused to choose. */ toolPortConflict?: unknown; + /** Tool only: `user` when the user-global config declared it. */ + toolScope?: unknown; /** Tool only: the approval this Surface is waiting on before it runs. */ toolPending?: unknown; syncEngaged?: unknown; @@ -60,7 +62,9 @@ export function toolPortConflictFromParams(params: unknown): number[] | null { export interface ToolPending { readonly name: string; readonly run: string; + /** Inputs as invoked; approval re-resolves them. */ readonly args?: string[]; + /** Why the last approval attempt launched nothing; the prompt stays up. */ readonly error?: string; readonly path: string; readonly projectRoot: string; @@ -79,6 +83,8 @@ export function toolPendingFromParams(params: unknown): ToolPending | null { if (!strings.every((field) => typeof pending[field] === 'string')) return null; if (typeof pending.minimized !== 'boolean') return null; if (pending.upstreamUrl !== null && typeof pending.upstreamUrl !== 'string') return null; + if (pending.args !== undefined && !(Array.isArray(pending.args) && pending.args.every((arg) => typeof arg === 'string'))) return null; + if (pending.error !== undefined && typeof pending.error !== 'string') return null; return pending as unknown as ToolPending; } @@ -137,10 +143,17 @@ export function toolKeysEqual(paramsKey: unknown, key: readonly string[] | null) export function namespacedToolKey( toolName: string | null, key: readonly string[] | null, - scope?: unknown, ): string[] | null { if (!toolName || key === null) return null; - return scope === 'user' ? ['user', toolName, ...key] : [toolName, ...key]; + return [toolName, ...key]; +} + +/** Which file declared a tool: `user` for the user-global config, undefined for + * a project `dormouse.yml`. Project and user Tools are separate reuse scopes + * (`docs/specs/dor-tool.md` -> Declaring tools), so dedupe compares this + * alongside the key. */ +export function toolScopeFromParams(params: unknown): 'user' | undefined { + return asParams(params).toolScope === 'user' ? 'user' : undefined; } /** Whether params describe a plain browser surface (vs a terminal): the unified diff --git a/lib/src/components/wall/use-dor-control.ts b/lib/src/components/wall/use-dor-control.ts index e037620c1..007157451 100644 --- a/lib/src/components/wall/use-dor-control.ts +++ b/lib/src/components/wall/use-dor-control.ts @@ -46,6 +46,7 @@ import { surfaceKindFromParams, toolKeysEqual, toolPendingFromParams, + toolScopeFromParams, type ToolPending, } from './browser-surface'; @@ -484,6 +485,14 @@ function dorCommandString(args: string[] | undefined): string | undefined { return buildShellCommandForKind(shellCommandKind(shell, PLATFORM_STRING), args); } +/** The command a resolved Tool types into its shell: a string `run` is literal + * shell syntax, an argument-list `run` is quoted here for the default shell + * (`docs/specs/dor-tool.md` -> Declaring tools). The host guarantees a list + * names an executable, so the empty-argv case cannot arise. */ +export function toolRunCommand(run: string | readonly string[]): string { + return typeof run === 'string' ? run : dorCommandString([...run])!; +} + /** * The `dor` control plane: the webview handler for `dormouse:control-request` * events (the `surface.*` methods that back the `dor` CLI) plus its private @@ -928,12 +937,12 @@ export function useDorControl({ detail.respond({ ok: false, error: 'unexpected tool host response' }); return; case 'ok': - command = typeof lookup.run === 'string' ? lookup.run : dorCommandString([...lookup.run])!; + command = toolRunCommand(lookup.run); toolScope = lookup.scope; // Namespaced under the host-resolved tool name, so two tools in // one repo with scope-only keys stay distinct and a runtime // re-key cannot name another tool's key. - key = namespacedToolKey(lookup.name, lookup.key, lookup.scope); + key = namespacedToolKey(lookup.name, lookup.key); render = lookup.render; port = lookup.port; warnings = lookup.warnings; @@ -950,7 +959,7 @@ export function useDorControl({ }); return; case 'untrusted': { - const pendingCommand = typeof lookup.run === 'string' ? lookup.run : dorCommandString([...lookup.run])!; + const pendingCommand = toolRunCommand(lookup.run); // Approval can only lead to a command gated on OSC 633. Reject // a shell known never to emit it before offering a prompt that // would otherwise approve, spawn, then silently drop the command. @@ -971,7 +980,7 @@ export function useDorControl({ const matchesPending = (candidate: unknown) => { const waiting = toolPendingFromParams(candidate); return waiting?.name === lookup.name && waiting.projectRoot === lookup.projectRoot - && JSON.stringify(waiting.args ?? []) === JSON.stringify(toolArgs); + && toolKeysEqual(waiting.args ?? [], toolArgs); }; const already = findSurfaceByParams(matchesPending); if (already) { @@ -1064,7 +1073,7 @@ export function useDorControl({ // (docs/specs/dor-tool.md -> Identity and dedupe). if (key && !booleanParam(params.fresh)) { const matchesToolKey = (candidate: unknown) => - (candidate as { toolScope?: unknown } | null | undefined)?.toolScope === toolScope + toolScopeFromParams(candidate) === toolScope && toolKeysEqual((candidate as { toolKey?: unknown } | null | undefined)?.toolKey, key); const match = findSurfaceByParams(matchesToolKey); if (match) { diff --git a/lib/src/components/wall/use-tool-serving.ts b/lib/src/components/wall/use-tool-serving.ts index 2b80d0b1d..da720008d 100644 --- a/lib/src/components/wall/use-tool-serving.ts +++ b/lib/src/components/wall/use-tool-serving.ts @@ -98,7 +98,7 @@ export function useToolServing({ // dedupes (docs/specs/dor-tool.md -> Identity and dedupe). The // namespace that keeps process output from claiming another tool's key // is `namespacedToolKey`'s job; see its doc comment. - const announcedKey = namespacedToolKey(toolNameFromParams(leaf.params), announce?.key ?? null, leaf.params?.toolScope); + const announcedKey = namespacedToolKey(toolNameFromParams(leaf.params), announce?.key ?? null); if (announcedKey && !toolKeysEqual(leaf.params?.toolKey, announcedKey)) { lath.store.updateParams(leaf.id, { toolKey: announcedKey }); } diff --git a/lib/src/host/tool-host.test.ts b/lib/src/host/tool-host.test.ts index 672e9891a..276eea91c 100644 --- a/lib/src/host/tool-host.test.ts +++ b/lib/src/host/tool-host.test.ts @@ -117,66 +117,63 @@ describe('createToolHost', () => { const result = await createToolHost({ stateDir }).handle({ op: 'lookup', name: 't', cwd: repo }); expect(result).toMatchObject({ status: 'error' }); }); -}); - -it('resolves user tools without repo approval and keeps explicit project names ahead of them', async () => { - const path = join(repo, 'user.yml'); - await writeFile(path, `tools:\n storybook:\n run: [user-storybook, $ARGS]\n viewer:\n run: [viewer, $TARGET]\n prespawn_dedupe: [$TARGET]\n`); - const target = join(repo, 'a b; $(echo bad).md'); - await writeFile(target, 'hello'); - const host = createToolHost({ stateDir, userConfigPath: path }); - expect(await host.handle({ op: 'lookup', name: 'storybook', cwd: repo })).toMatchObject({ status: 'untrusted', run: 'pnpm storybook' }); - expect(await host.handle({ op: 'lookup', name: 'storybook', global: true, cwd: repo, args: ['--port', '1234'] })) - .toMatchObject({ status: 'ok', scope: 'user', run: ['user-storybook', '--port', '1234'] }); - expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: ['a b; $(echo bad).md'] })) - .toMatchObject({ status: 'ok', scope: 'user', run: ['viewer', target], key: [target] }); - expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: ['https://example.com/a.md'] })) - .toMatchObject({ status: 'error', message: expect.stringContaining('local file') }); - expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: [] })) - .toMatchObject({ status: 'error', message: expect.stringContaining('exactly one') }); -}); -it('re-resolves input argv after project approval without shell interpolation', async () => { - await writeFile(join(repo, 'dormouse.yml'), 'tools:\n viewer:\n run: [viewer, $TARGET]\n prespawn_dedupe: [$PROJECT_ROOT, $TARGET]\n'); - const target = join(repo, "quoted ' document.md"); - await writeFile(target, 'hello'); - const host = createToolHost({ stateDir }); - const request = { op: 'lookup' as const, name: 'viewer', cwd: repo, args: [target] }; - expect(await host.handle(request)).toMatchObject({ status: 'untrusted', run: ['viewer', target] }); - await host.handle({ op: 'trust', kind: 'folder', projectRoot: repo }); - expect(await host.handle(request)).toMatchObject({ status: 'ok', run: ['viewer', target], key: [repo, target] }); -}); + it('resolves user tools without repo approval and keeps explicit project names ahead of them', async () => { + const path = join(repo, 'user.yml'); + await writeFile(path, `tools:\n storybook:\n run: [user-storybook, $ARGS]\n viewer:\n run: [viewer, $TARGET]\n prespawn_dedupe: [$TARGET]\n`); + const target = join(repo, 'a b; $(echo bad).md'); + await writeFile(target, 'hello'); + const host = createToolHost({ stateDir, userConfigPath: path }); + expect(await host.handle({ op: 'lookup', name: 'storybook', cwd: repo })).toMatchObject({ status: 'untrusted', run: 'pnpm storybook' }); + expect(await host.handle({ op: 'lookup', name: 'storybook', global: true, cwd: repo, args: ['--port', '1234'] })) + .toMatchObject({ status: 'ok', scope: 'user', run: ['user-storybook', '--port', '1234'] }); + expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: ['a b; $(echo bad).md'] })) + .toMatchObject({ status: 'ok', scope: 'user', run: ['viewer', target], key: [target] }); + expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: ['https://example.com/a.md'] })) + .toMatchObject({ status: 'error', message: expect.stringContaining('local file') }); + expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo, args: [] })) + .toMatchObject({ status: 'error', message: expect.stringContaining('exactly one') }); + }); -it('does not silently ignore malformed user configuration or input to shell strings', async () => { - const path = join(repo, 'user.yml'); - const host = createToolHost({ stateDir, userConfigPath: path }); - await writeFile(path, 'tools:\n viewer:\n run: [viewer, $PROJECT_ROOT]\n'); - expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo })).toMatchObject({ status: 'error' }); - expect(await host.handle({ op: 'lookup', name: 'storybook', cwd: repo, args: ['input'] })) - .toMatchObject({ status: 'error', message: expect.stringContaining('argument-list run') }); -}); + it('re-resolves input argv after project approval without shell interpolation', async () => { + await writeFile(join(repo, 'dormouse.yml'), 'tools:\n viewer:\n run: [viewer, $TARGET]\n prespawn_dedupe: [$PROJECT_ROOT, $TARGET]\n'); + const target = join(repo, "quoted ' document.md"); + await writeFile(target, 'hello'); + const host = createToolHost({ stateDir }); + const request = { op: 'lookup' as const, name: 'viewer', cwd: repo, args: [target] }; + expect(await host.handle(request)).toMatchObject({ status: 'untrusted', run: ['viewer', target] }); + await host.handle({ op: 'trust', kind: 'folder', projectRoot: repo }); + expect(await host.handle(request)).toMatchObject({ status: 'ok', run: ['viewer', target], key: [repo, target] }); + }); + it('does not silently ignore malformed user configuration or input to shell strings', async () => { + const path = join(repo, 'user.yml'); + const host = createToolHost({ stateDir, userConfigPath: path }); + await writeFile(path, 'tools:\n viewer:\n run: [viewer, $PROJECT_ROOT]\n'); + expect(await host.handle({ op: 'lookup', name: 'viewer', cwd: repo })).toMatchObject({ status: 'error' }); + expect(await host.handle({ op: 'lookup', name: 'storybook', cwd: repo, args: ['input'] })) + .toMatchObject({ status: 'error', message: expect.stringContaining('argument-list run') }); + }); -it('validates target-only dedupe inputs before asking for project approval', async () => { - await writeFile(join(repo, 'dormouse.yml'), 'tools:\n view:\n run: [viewer, $ARGS]\n prespawn_dedupe: [$TARGET]\n'); - const host = createToolHost({ stateDir }); - expect(await host.handle({ op: 'lookup', name: 'view', cwd: repo, args: ['missing.md'] })) - .toMatchObject({ status: 'error' }); - expect(await host.handle({ op: 'lookup', name: 'view', cwd: repo, args: [] })) - .toMatchObject({ status: 'error', message: expect.stringContaining('exactly one') }); -}); + it('validates target-only dedupe inputs before asking for project approval', async () => { + await writeFile(join(repo, 'dormouse.yml'), 'tools:\n view:\n run: [viewer, $ARGS]\n prespawn_dedupe: [$TARGET]\n'); + const host = createToolHost({ stateDir }); + expect(await host.handle({ op: 'lookup', name: 'view', cwd: repo, args: ['missing.md'] })) + .toMatchObject({ status: 'error', message: 'no such file: missing.md' }); + }); -it('names the actual user path when --global has no config', async () => { - const path = join(repo, 'absent-user.yml'); - expect(await createToolHost({ userConfigPath: path }).handle({ op: 'lookup', name: 'view', cwd: repo, global: true })) - .toMatchObject({ status: 'unknown-tool', path, names: [] }); -}); + it('names the actual user path when --global has no config', async () => { + const path = join(repo, 'absent-user.yml'); + expect(await createToolHost({ userConfigPath: path }).handle({ op: 'lookup', name: 'view', cwd: repo, global: true })) + .toMatchObject({ status: 'unknown-tool', path, names: [] }); + }); -it.skipIf(process.platform === 'win32')('follows user-config dotfile symlinks while keeping project symlinks inert', async () => { - const path = join(repo, 'user-link.yml'); - const source = join(repo, 'dotfiles.yml'); - await writeFile(source, 'tools:\n scratch:\n run: echo hi\n'); - await symlink(source, path); - const result = await createToolHost({ userConfigPath: path }).handle({ op: 'lookup', name: 'scratch', cwd: repo, global: true }); - expect(result).toMatchObject({ status: 'ok', scope: 'user', run: 'echo hi' }); + it.skipIf(process.platform === 'win32')('follows a user-config dotfiles symlink', async () => { + const path = join(repo, 'user-link.yml'); + const source = join(repo, 'dotfiles.yml'); + await writeFile(source, 'tools:\n scratch:\n run: echo hi\n'); + await symlink(source, path); + const result = await createToolHost({ userConfigPath: path }).handle({ op: 'lookup', name: 'scratch', cwd: repo, global: true }); + expect(result).toMatchObject({ status: 'ok', scope: 'user', run: 'echo hi' }); + }); }); diff --git a/lib/src/host/tool-host.ts b/lib/src/host/tool-host.ts index c2f838788..dee8cf5c3 100644 --- a/lib/src/host/tool-host.ts +++ b/lib/src/host/tool-host.ts @@ -11,7 +11,8 @@ import { dirname } from 'node:path'; import type { ToolControlResult, ToolHostRequest } from '../lib/platform/tool-types'; import { resolveUpstreamUrl } from './git-upstream'; -import { resolveToolInput } from './tool-input'; +import { resolveToolInput, type ToolInput } from './tool-input'; +import type { ToolEntry } from './tool-registry'; import { readUserToolFile, userToolConfigPath } from './tool-user-config'; import { FileToolTrustStore, @@ -26,6 +27,25 @@ export interface ToolHost { handle(request: ToolHostRequest): Promise; } +/** The one wire shape for a resolved Tool, whichever file declared it. */ +function okResult( + entry: ToolEntry, + input: ToolInput, + source: { projectRoot: string; path: string; warnings: readonly string[]; scope?: 'user' }, +): ToolControlResult { + return { + status: 'ok', + projectRoot: source.projectRoot, + path: source.path, + name: entry.name, + ...input, + ...(source.scope ? { scope: source.scope } : {}), + render: entry.render, + port: entry.port, + warnings: [...source.warnings], + }; +} + /** * `stateDir` is where the trust record lives. Without one the decision is * in-memory and dies with the host: a host with no durable state re-asks each @@ -56,31 +76,22 @@ export function createToolHost(options: { stateDir?: string; userConfigPath?: st try { const args = request.args ?? []; - const lookup = request.global ? { status: 'no-file' as const } : await lookupTool(request.name, request.cwd, trust, undefined, undefined, args); - if (lookup.status === 'no-file' || lookup.status === 'unknown-tool') { - const path = options.userConfigPath ?? userToolConfigPath(); - const file = await readUserToolFile(path); - const entry = file?.tools.get(request.name); - if (file && entry) { - const input = await resolveToolInput(entry, { projectRoot: null, cwd: request.cwd, args }); - return { status: 'ok', projectRoot: file.dir, path, name: entry.name, - ...input, scope: 'user', render: entry.render, port: entry.port, warnings: [...file.warnings] }; - } - if (request.global) return { status: 'unknown-tool', projectRoot: dirname(path), path, names: [...(file?.tools.keys() ?? [])].sort() }; - return lookup; + const project = request.global ? null : await lookupTool(request.name, request.cwd, trust, { args }); + if (project?.status === 'ok') { + return okResult(project.entry, project.input, { projectRoot: project.projectRoot, path: project.path, warnings: project.file.warnings }); + } + if (project && project.status !== 'no-file' && project.status !== 'unknown-tool') return project; + + // A project miss falls through to the user's own Tools, which need no grant. + const path = options.userConfigPath ?? userToolConfigPath(); + const file = await readUserToolFile(path); + const entry = file?.tools.get(request.name); + if (file && entry) { + const input = await resolveToolInput(entry, { projectRoot: null, cwd: request.cwd, args }); + return okResult(entry, input, { projectRoot: file.dir, path, warnings: file.warnings, scope: 'user' }); } - if (lookup.status !== 'ok') return lookup; - const { entry, input } = lookup; - return { - status: 'ok', - projectRoot: lookup.projectRoot, - path: lookup.path, - name: entry.name, - ...input, - render: entry.render, - port: entry.port, - warnings: [...lookup.file.warnings], - }; + if (project) return project; + return { status: 'unknown-tool', projectRoot: dirname(path), path, names: [...(file?.tools.keys() ?? [])].sort() }; } catch (error) { return { status: 'error', message: error instanceof Error ? error.message : String(error) }; } diff --git a/lib/src/host/tool-input.ts b/lib/src/host/tool-input.ts index f95e7738c..c7a4819c5 100644 --- a/lib/src/host/tool-input.ts +++ b/lib/src/host/tool-input.ts @@ -1,14 +1,28 @@ import { realpath, stat } from 'node:fs/promises'; import { isAbsolute, resolve } from 'node:path'; -import { resolveDedupeKey, ToolFileError, type ToolEntry } from './tool-registry'; +import { resolveDedupeKey, substituteToolTokens, ToolFileError, usesTarget, type ToolEntry } from './tool-registry'; + +/** What one invocation's inputs resolved an entry to: the argv (or literal + * shell string) to run, and the rendered dedupe key. */ +export interface ToolInput { + readonly run: string | readonly string[]; + readonly key: string[] | null; +} /** A target is one existing regular file on this host. Resolve symlinks before * keying, so two paths to the same document reveal the same Tool. */ -export async function resolveLocalToolTarget(input: string, cwd: string): Promise { - if (!input || input.includes('\0') || (!isAbsolute(input) && /^[a-z][a-z\d+.-]*:/i.test(input))) { +async function resolveLocalToolTarget(input: string, cwd: string): Promise { + if (!input || (!isAbsolute(input) && /^[a-z][a-z\d+.-]*:/i.test(input))) { throw new ToolFileError('expected a local file path, not a URL or Surface handle'); } - const target = await realpath(resolve(cwd, input)); + let target: string; + try { + target = await realpath(resolve(cwd, input)); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ENOENT' || code === 'ENOTDIR') throw new ToolFileError(`no such file: ${input}`); + throw error; + } if (!(await stat(target)).isFile()) throw new ToolFileError(`not a regular file: ${input}`); return target; } @@ -16,25 +30,22 @@ export async function resolveLocalToolTarget(input: string, cwd: string): Promis export async function resolveToolInput( entry: Pick, context: { cwd: string; projectRoot: string | null; args: readonly string[] }, -): Promise<{ run: string | readonly string[]; key: string[] | null }> { +): Promise { const { args } = context; if (args.some(arg => typeof arg !== 'string' || arg.includes('\0'))) throw new ToolFileError('invalid tool arguments'); - const templates = [...(typeof entry.run === 'string' ? [] : entry.run), ...(entry.dedupeTemplate ?? [])]; - const needsTarget = templates.some(arg => /\$TARGET\b/.test(arg)); + const runList = typeof entry.run === 'string' ? [] : entry.run; + const runHasTarget = usesTarget(runList); + const needsTarget = runHasTarget || usesTarget(entry.dedupeTemplate ?? []); if (needsTarget && args.length !== 1) throw new ToolFileError('$TARGET requires exactly one local file argument'); const target = needsTarget ? await resolveLocalToolTarget(args[0], context.cwd) : undefined; - const key = resolveDedupeKey(entry, { ...context, target }); + const substitution = { ...context, target }; + const key = resolveDedupeKey(entry, substitution); if (typeof entry.run === 'string') { if (args.length) throw new ToolFileError(`tool '${entry.name}': use an argument-list run to accept arguments`); return { run: entry.run, key }; } - const run = entry.run.flatMap(arg => arg === '$ARGS' ? [...args] : [arg.replace(/\$[A-Za-z_][A-Za-z0-9_]*/g, token => { - if (token === '$TARGET') return target!; - if (token === '$CWD') return context.cwd; - if (token === '$PROJECT_ROOT' && context.projectRoot !== null) return context.projectRoot; - throw new ToolFileError(`unknown substitution '${token}'`); - })]); - if (!entry.run.some(arg => arg === '$ARGS' || /\$TARGET\b/.test(arg))) run.push(...args); + const run = runList.flatMap(arg => arg === '$ARGS' ? [...args] : [substituteToolTokens(arg, substitution, entry.name)]); + if (!runHasTarget && !runList.includes('$ARGS')) run.push(...args); if (!run[0]?.trim()) throw new ToolFileError('tool argument list must name an executable'); return { run, key }; } diff --git a/lib/src/host/tool-registry.test.ts b/lib/src/host/tool-registry.test.ts index bef504fb8..ec695c40f 100644 --- a/lib/src/host/tool-registry.test.ts +++ b/lib/src/host/tool-registry.test.ts @@ -195,7 +195,6 @@ describe("this repo's own dormouse.yml", () => { }); }); - it('rejects a shell command with a target-only dedupe key at declaration time', () => { expect(() => parse('tools:\n viewer:\n run: view\n prespawn_dedupe: [$TARGET]\n')) .toThrow('$TARGET in prespawn_dedupe requires an argument-list run'); diff --git a/lib/src/host/tool-registry.ts b/lib/src/host/tool-registry.ts index 57b5911bb..b322ac378 100644 --- a/lib/src/host/tool-registry.ts +++ b/lib/src/host/tool-registry.ts @@ -63,6 +63,11 @@ export type Substitution = (typeof SUBSTITUTIONS)[number]; // be named in the error rather than silently surviving as text. const SUBSTITUTION_TOKEN = /\$[A-Za-z_][A-Za-z0-9_]*/g; +/** Whether any element names the `$TARGET` input. */ +export function usesTarget(elements: readonly string[]): boolean { + return elements.some((element) => /\$TARGET\b/.test(element)); +} + // The reserved namespace. An unknown member is an error rather than an ignored // field: silently dropping a dedupe directive the author wrote is the // destructive failure (two tools, one port), where failing to parse is loud. @@ -89,7 +94,7 @@ function readDedupeTemplate(value: unknown, where: string): string[] { } /** Reject unknown `$NAME` tokens, and `$PROJECT_ROOT` outside a repo scope. */ -export function validateSubstitutions(template: readonly string[], scope: ToolScope, where: string): void { +function validateSubstitutions(template: readonly string[], scope: ToolScope, where: string): void { for (const element of template) { for (const token of element.match(SUBSTITUTION_TOKEN) ?? []) { if (!(SUBSTITUTIONS as readonly string[]).includes(token)) { @@ -160,7 +165,7 @@ export function parseToolFile( if (rawEntry.prespawn_dedupe !== undefined && rawEntry.prespawn_dedupe !== null) { dedupeTemplate = readDedupeTemplate(rawEntry.prespawn_dedupe, where); validateSubstitutions(dedupeTemplate, scope, where); - if (typeof run === 'string' && dedupeTemplate.some(arg => /\$TARGET\b/.test(arg))) { + if (typeof run === 'string' && usesTarget(dedupeTemplate)) { throw new ToolFileError(`${where}: $TARGET in prespawn_dedupe requires an argument-list run`); } // A repo-local key with no project scope dedupes across every checkout @@ -192,6 +197,36 @@ export function parseToolFile( return { scope, dir, tools, warnings }; } +export interface SubstitutionContext { + readonly projectRoot: string | null; + readonly cwd: string; + /** The canonical local file, present only when the invocation resolved one. */ + readonly target?: string; +} + +/** + * Expand every `$NAME` in one template element. Closed set: an unknown token + * throws rather than surviving as text (see `SUBSTITUTIONS`), and a token + * whose value is absent from `context` throws so a caller assembling entries + * by hand cannot produce a literal `$PROJECT_ROOT` in a key or a command. + */ +export function substituteToolTokens(element: string, context: SubstitutionContext, toolName: string): string { + return element.replace(SUBSTITUTION_TOKEN, (token) => { + if (token === '$CWD') return context.cwd; + if (token === '$TARGET') { + if (!context.target) throw new ToolFileError(`tool '${toolName}': $TARGET requires one local file argument`); + return context.target; + } + if (token === '$PROJECT_ROOT') { + if (context.projectRoot === null) { + throw new ToolFileError(`tool '${toolName}': $PROJECT_ROOT is not defined here`); + } + return context.projectRoot; + } + throw new ToolFileError(`tool '${toolName}': unknown substitution '${token}'`); + }); +} + /** * Render an entry's key for one invocation. Returns `null` when the entry * declared no template — a tool has an identity if and only if it was given @@ -199,26 +234,8 @@ export function parseToolFile( */ export function resolveDedupeKey( entry: Pick, - context: { projectRoot: string | null; cwd: string; target?: string }, + context: SubstitutionContext, ): string[] | null { if (!entry.dedupeTemplate) return null; - return entry.dedupeTemplate.map((element) => - element.replace(SUBSTITUTION_TOKEN, (token) => { - if (token === '$CWD') return context.cwd; - if (token === '$TARGET') { - if (!context.target) throw new ToolFileError(`tool '${entry.name}': $TARGET requires one local file argument`); - return context.target; - } - if (token === '$PROJECT_ROOT') { - // Unreachable via parseToolFile, which rejects $PROJECT_ROOT outside a - // repo scope; guard anyway so a caller assembling entries by hand - // cannot produce a key with a literal '$PROJECT_ROOT' in it. - if (context.projectRoot === null) { - throw new ToolFileError(`tool '${entry.name}': $PROJECT_ROOT is not defined here`); - } - return context.projectRoot; - } - return token; - }), - ); + return entry.dedupeTemplate.map((element) => substituteToolTokens(element, context, entry.name)); } diff --git a/lib/src/host/tool-trust.test.ts b/lib/src/host/tool-trust.test.ts index 39aca2d29..f1f7259a7 100644 --- a/lib/src/host/tool-trust.test.ts +++ b/lib/src/host/tool-trust.test.ts @@ -192,13 +192,13 @@ describe('lookupTool', () => { const write = (text = YML) => writeFile(join(root, 'dormouse.yml'), text); it('reports no-file when there is nothing to read', async () => { - expect(await lookupTool('storybook', root, new MemoryToolTrustStore(), undefined, noUpstream)) + expect(await lookupTool('storybook', root, new MemoryToolTrustStore(), { resolveUpstream: noUpstream })) .toEqual({ status: 'no-file' }); }); it('asks for trust before running anything, naming the command', async () => { await write(); - expect(await lookupTool('storybook', root, new MemoryToolTrustStore(), undefined, noUpstream)) + expect(await lookupTool('storybook', root, new MemoryToolTrustStore(), { resolveUpstream: noUpstream })) .toMatchObject({ status: 'untrusted', projectRoot: root, @@ -211,7 +211,7 @@ describe('lookupTool', () => { it('offers the upstream when git resolves one', async () => { await write(); const upstream = async () => 'https://github.com/diffplug/dormouse'; - expect(await lookupTool('storybook', root, new MemoryToolTrustStore(), undefined, upstream)) + expect(await lookupTool('storybook', root, new MemoryToolTrustStore(), { resolveUpstream: upstream })) .toMatchObject({ status: 'untrusted', upstreamUrl: 'https://github.com/diffplug/dormouse' }); }); @@ -220,14 +220,14 @@ describe('lookupTool', () => { const trust = new MemoryToolTrustStore(); await trust.grant(upstreamGrantKey('https://github.com/diffplug/dormouse'), 'upstream'); const upstream = async () => 'https://github.com/diffplug/dormouse'; - expect((await lookupTool('storybook', root, trust, undefined, upstream)).status).toBe('ok'); + expect((await lookupTool('storybook', root, trust, { resolveUpstream: upstream })).status).toBe('ok'); }); it('resolves once the folder is granted', async () => { await write(); const trust = new MemoryToolTrustStore(); await trust.grant(folderGrantKey(root), 'folder'); - const result = await lookupTool('storybook', root, trust, undefined, noUpstream); + const result = await lookupTool('storybook', root, trust, { resolveUpstream: noUpstream }); expect(result.status).toBe('ok'); if (result.status !== 'ok') return; expect(result.entry.run).toBe('pnpm storybook'); @@ -237,7 +237,7 @@ describe('lookupTool', () => { it('reports an unknown tool with the names it does know, before any trust check', async () => { await write(); - expect(await lookupTool('nope', root, new MemoryToolTrustStore(), undefined, noUpstream)).toMatchObject({ + expect(await lookupTool('nope', root, new MemoryToolTrustStore(), { resolveUpstream: noUpstream })).toMatchObject({ status: 'unknown-tool', names: ['once', 'storybook'], }); @@ -245,7 +245,7 @@ describe('lookupTool', () => { it('surfaces a parse error as an error rather than throwing', async () => { await write('tools:\n t:\n run: x\n prespawn_dedupe: [$NOPE]\n'); - const result = await lookupTool('t', root, new MemoryToolTrustStore(), undefined, noUpstream); + const result = await lookupTool('t', root, new MemoryToolTrustStore(), { resolveUpstream: noUpstream }); expect(result).toMatchObject({ status: 'error' }); if (result.status !== 'error') return; expect(result.message).toMatch(/unknown substitution '\$NOPE'/); @@ -268,7 +268,7 @@ describe('the pre-approval read (regression: review finding 13, PR #493 review)' it('still reads a normal file', async () => { await writeFile(join(root, 'dormouse.yml'), YML); - expect((await lookupTool('storybook', root, new MemoryToolTrustStore(), undefined, noUpstream)).status).toBe('untrusted'); + expect((await lookupTool('storybook', root, new MemoryToolTrustStore(), { resolveUpstream: noUpstream })).status).toBe('untrusted'); }); it('refuses a symlink instead of following it before trust', async () => { @@ -276,7 +276,7 @@ describe('the pre-approval read (regression: review finding 13, PR #493 review)' await writeFile(target, YML); await symlink(target, join(root, 'dormouse.yml')); - const result = await lookupTool('storybook', root, new MemoryToolTrustStore(), undefined, noUpstream); + const result = await lookupTool('storybook', root, new MemoryToolTrustStore(), { resolveUpstream: noUpstream }); expect(result).toMatchObject({ status: 'error' }); if (result.status !== 'error') return; expect(result.message).toMatch(/must be a regular file, not a symbolic link$/); @@ -287,7 +287,7 @@ describe('the pre-approval read (regression: review finding 13, PR #493 review)' // check standing. 100k four-byte characters: well under the cap by // `.length`, well over it by bytes. Counting code units would let it through. const oversized = `# ${'\u{1F600}'.repeat(100_000)}\n`; - const result = await lookupTool('storybook', root, new MemoryToolTrustStore(), async () => oversized, noUpstream); + const result = await lookupTool('storybook', root, new MemoryToolTrustStore(), { readTextFile: async () => oversized, resolveUpstream: noUpstream }); expect(result).toMatchObject({ status: 'error' }); if (result.status !== 'error') return; expect(result.message).toMatch(/after reading$/); diff --git a/lib/src/host/tool-trust.ts b/lib/src/host/tool-trust.ts index d97fd0f7b..904319977 100644 --- a/lib/src/host/tool-trust.ts +++ b/lib/src/host/tool-trust.ts @@ -15,7 +15,7 @@ import { randomUUID } from 'node:crypto'; import { dirname, join, resolve } from 'node:path'; import { ToolFileError, parseToolFile, type ToolEntry, type ToolFile } from './tool-registry'; import { resolveUpstreamUrl } from './git-upstream'; -import { resolveToolInput } from './tool-input'; +import { resolveToolInput, type ToolInput } from './tool-input'; export const TOOL_FILE_NAME = 'dormouse.yml'; /** @@ -26,19 +26,19 @@ export const TOOL_FILE_NAME = 'dormouse.yml'; */ const TOOL_FILE_MAX_BYTES = 256 * 1024; -/** Refuse repo-config symlinks; user config may follow a dotfiles link. - * Both paths fstat and cap one descriptor. +/** Refuse repo-config symlinks; user config may follow a dotfiles link + * (`followSymlink`). Both paths fstat and cap one descriptor, and open + * non-blocking so a FIFO at the path fails the fstat check instead of hanging. * POSIX also opens no-follow, closing the lstat/open replacement race there. */ -export async function readToolFile(path: string, allowSymlink = false): Promise { +export async function readToolFile(path: string, options: { followSymlink?: boolean } = {}): Promise { const entry = await lstat(path); - if (entry.isSymbolicLink() && !allowSymlink) { + if (entry.isSymbolicLink() && !options.followSymlink) { throw new ToolFileError(`${path}: tool file must be a regular file, not a symbolic link`); } - if (!entry.isSymbolicLink() && !entry.isFile()) throw new ToolFileError(`${path}: tool file must be a regular file`); let file; try { - const noFollow = !allowSymlink && typeof constants.O_NOFOLLOW === 'number' ? constants.O_NOFOLLOW : 0; + const noFollow = options.followSymlink ? 0 : (constants.O_NOFOLLOW ?? 0); file = await open(path, constants.O_RDONLY | noFollow | (constants.O_NONBLOCK ?? 0)); } catch (error) { const code = (error as NodeJS.ErrnoException).code; @@ -432,7 +432,7 @@ export type ToolLookup = upstreamUrl: string | null; } | { status: 'error'; message: string } - | { status: 'ok'; projectRoot: string; path: string; file: ToolFile; entry: ToolEntry; input: Awaited> }; + | { status: 'ok'; projectRoot: string; path: string; file: ToolFile; entry: ToolEntry; input: ToolInput }; /** * Find, parse, and trust-check the entry named `name` for a caller in `cwd`. @@ -445,10 +445,15 @@ export async function lookupTool( name: string, cwd: string, trust: ToolTrustStore, - readTextFile?: (path: string) => Promise, - resolveUpstream: (dir: string) => Promise = resolveUpstreamUrl, - args: readonly string[] = [], + options: { + /** Invocation inputs for `$ARGS` / `$TARGET`; none by default. */ + args?: readonly string[]; + /** Test seams. */ + readTextFile?: (path: string) => Promise; + resolveUpstream?: (dir: string) => Promise; + } = {}, ): Promise { + const { args = [], readTextFile, resolveUpstream = resolveUpstreamUrl } = options; let found; try { found = await findToolFile(cwd, readTextFile); @@ -477,7 +482,7 @@ export async function lookupTool( }; } - let input: Awaited>; + let input: ToolInput; try { input = await resolveToolInput(entry, { projectRoot: found.dir, cwd, args }); } catch (error) { diff --git a/lib/src/host/tool-user-config.ts b/lib/src/host/tool-user-config.ts index 0e8896e2b..dfd82dc33 100644 --- a/lib/src/host/tool-user-config.ts +++ b/lib/src/host/tool-user-config.ts @@ -10,7 +10,7 @@ export function userToolConfigPath(): string { export async function readUserToolFile(path: string): Promise { try { - return parseToolFile(await readToolFile(path, true), { path, dir: dirname(path), scope: 'user' }); + return parseToolFile(await readToolFile(path, { followSymlink: true }), { path, dir: dirname(path), scope: 'user' }); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; throw error; From 92ccba5ce1498bc98bdd64c43973e1e534194699 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 15 Sep 2026 17:53:51 -0700 Subject: [PATCH 04/10] Handle Tool approval failures without repeating grants --- docs/specs/dor-tool.md | 6 +- lib/src/components/Wall.test.tsx | 74 +++++++++++++++++++--- lib/src/components/Wall.tsx | 46 ++++++++------ lib/src/components/wall/ToolApproval.tsx | 40 +++++++----- lib/src/components/wall/browser-surface.ts | 2 + lib/src/components/wall/wall-context.tsx | 2 +- scripts/spec-word-budgets.json | 2 +- 7 files changed, 126 insertions(+), 46 deletions(-) diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index dd227975d..ce6556ec1 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -50,7 +50,7 @@ Source of truth: `surfaceKindFromParams` / `isToolParams` in `lib/src/components **Must pass named-tool inputs as argument values, never substitute them into a shell-command string.** String `run` accepts no arguments and remains literal shell syntax. List `run` expands `$TARGET`, `$CWD`, and `$PROJECT_ROOT` within elements; a whole `$ARGS` element expands all input arguments. Without `$ARGS` or `$TARGET` in the list, append the inputs. The renderer quotes the resulting argv for its configured shell. -**Must require exactly one existing regular local file when `$TARGET` appears in the run list or dedupe key.** Resolve relative paths against the invocation CWD and follow symlinks to a canonical absolute path before substitution and reuse. Reject URLs, directories, and missing files. Validate run and key inputs before showing approval. Pending approval retains the original arguments and distinguishes requests with different inputs; approval re-resolves them before launch. A failed re-resolution leaves the pane pending with an error and no PTY, allowing retry or closure. +**Must require exactly one existing regular local file when `$TARGET` appears in the run list or dedupe key.** Resolve relative paths against the invocation CWD and follow symlinks to a canonical absolute path before substitution and reuse. Reject URLs, directories, and missing files. Validate run and key inputs before showing approval. Pending approval retains the original arguments and distinguishes requests with different inputs; [Trust](#trust) owns re-resolution and recovery. Source of truth: `lookupTool` in `lib/src/host/tool-trust.ts`; `parseToolFile` / `resolveDedupeKey` in `lib/src/host/tool-registry.ts`; `resolveToolInput` in `lib/src/host/tool-input.ts`; `readUserToolFile` in `lib/src/host/tool-user-config.ts`; `lib/src/host/tool-host.test.ts`, `lib/src/host/tool-registry.test.ts`. @@ -74,11 +74,13 @@ Source of truth: `acquireToolSpawnLock` / the `surface.tool` handler in `lib/src 1. **Must derive grant keys host-side from the canonical upstream remote URL or project-root folder.** Either recorded key satisfies lookup; upstream trust spans clones and worktrees. (rationale) 2. **Must present unapproved named invocations in a visible pending Tool pane**, returning `pending` without spawning a PTY. Defer requested minimization until approval. Pending approval is never persisted as a runnable Tool. 3. **Must grant only through the approval controls in Dormouse chrome**, never through a `dor` verb or terminal output. The prompt names the proposed command; it is not itself executable terminal content. (rationale) -4. **Must require `trust-recorded` before re-resolving the named entry**, retaining the pending pane after a rejected grant, then stage the resolved command, renderer, port strategy, and key before exposing its terminal. A Surface closed during the host calls must not start later. +4. **Must require the host's `trust-recorded` result before re-resolving the named entry**, then stage the command, renderer, port strategy, and key before exposing its terminal. Failed grants retain approval choices and display errors. Closed Surfaces must not start later. 5. **Must close a declined approval through the ordinary close coordinator and record no denial.** Archive failure may retain the pane. (rationale) 6. **Must share grant updates safely across host processes**, merging against the latest file under the existing lock and atomic-write protocol. 7. **Never content-hash grants or re-prompt solely because the config changed.** (rationale) +**Must offer Retry and Close after post-grant lookup failure**, preserving the error with no PTY. Retry repeats only lookup; Close retains permission; the footer states both. **Never restore pending approval once launch clears its marker**, including after PTY/minimization failure. + Reserved: **Must keep future implicit glob dispatch user-global and limited to user-global Tools**, and gate any future repo `prespawn_*` execution on the same approval; see scope **dor-tools** under [Future](#future). Source of truth: `createToolHost` in `lib/src/host/tool-host.ts`; `FileToolTrustStore` / `lookupTool` in `lib/src/host/tool-trust.ts`; `resolveUpstreamUrl` in `lib/src/host/git-upstream.ts`; `ToolApproval` in `lib/src/components/wall/ToolApproval.tsx`; `resolveToolApproval` in `lib/src/components/Wall.tsx`. Tests: `lib/src/host/tool-trust.test.ts`, `lib/src/components/Wall.test.tsx`. diff --git a/lib/src/components/Wall.test.tsx b/lib/src/components/Wall.test.tsx index ec46dadbe..4e077df89 100644 --- a/lib/src/components/Wall.test.tsx +++ b/lib/src/components/Wall.test.tsx @@ -1477,14 +1477,18 @@ describe('Wall on the Lath engine', () => { } }); - it('retains the approval pane and explains an input that disappeared before approval', async () => { + it('retries failed post-grant lookup without recording permission again', async () => { setToolsEnabled(true); let calls = 0; - Object.assign(fake, { toolControl: vi.fn(async (request: { op: string }) => { + const toolControl = vi.fn(async (request: { op: string }) => { if (request.op === 'trust') return { status: 'trust-recorded' }; - if (calls++) return { status: 'error', message: 'The selected file is missing' }; - return { status: 'untrusted', projectRoot: '/repo', path: '/repo/dormouse.yml', name: 'viewer', run: ['view', '/repo/file.md'], upstreamUrl: null }; - }) }); + const common = { projectRoot: '/repo', path: '/repo/dormouse.yml', name: 'viewer', run: ['view', '/repo/file.md'] }; + if (calls++ === 0) return { ...common, status: 'untrusted', upstreamUrl: null }; + if (calls === 2) return { status: 'error', message: 'The selected file is missing' }; + return { ...common, status: 'ok', render: 'iframe', port: 'auto', key: null, warnings: [] }; + }); + Object.assign(fake, { toolControl }); + let id: string | undefined; try { await act(async () => root.render()); await flush(); @@ -1492,13 +1496,57 @@ describe('Wall on the Lath engine', () => { await act(async () => window.dispatchEvent(new CustomEvent('dormouse:control-request', { detail: { method: SURFACE_CONTROL_METHODS.tool, params: { name: 'viewer', cwd: '/repo', args: ['file.md'] }, respond, } }))); - const id = respond.mock.calls[0][0].result.surfaceId; + id = respond.mock.calls[0][0].result.surfaceId; const allow = [...container.querySelectorAll('button')].find(button => button.textContent?.includes('Always allow for folder'))!; await act(async () => allow.click()); await flush(); expect(container.querySelector('[role="alert"]')?.textContent).toBe('The selected file is missing'); expect(container.querySelector(`[data-lath-leaf="${id}"]`)).not.toBeNull(); expect(container.querySelector(`[data-session-id="${id}"]`)).toBeNull(); + const pane = container.querySelector(`[data-lath-leaf="${id}"]`)!; + expect(pane.textContent).toContain('Permission is saved'); + expect(pane.textContent).not.toContain('Always allow'); + expect(pane.textContent).not.toContain('Declining records nothing'); + expect([...pane.querySelectorAll('button')].some(button => button.textContent === 'Close')).toBe(true); + const retry = [...pane.querySelectorAll('button')].find(button => button.textContent === 'Retry')!; + await act(async () => retry.click()); + await flush(); + expect(toolControl.mock.calls.filter(([request]) => request.op === 'trust')).toHaveLength(1); + expect(toolControl.mock.calls.filter(([request]) => request.op === 'lookup')).toHaveLength(3); + expect(container.querySelector(`[data-session-id="${id}"]`)).not.toBeNull(); + expect(container.querySelector('[role="alert"]')).toBeNull(); + expect(pendingShellOpts.get(id!)?.command).toBe('view /repo/file.md'); + } finally { if (id) pendingShellOpts.delete(id); setToolsEnabled(false); } + }); + + it.each(['error', 'missing', 'throws'] as const)('keeps approval choices and stops before lookup when recording trust %s', async failure => { + setToolsEnabled(true); + const toolControl = vi.fn(async (request: { op: string }) => { + if (request.op === 'trust') { + if (failure === 'throws') throw new Error('Permission storage is unavailable'); + return failure === 'missing' ? undefined : { status: 'error', message: 'Permission storage is unavailable' }; + } + return { status: 'untrusted', projectRoot: '/repo', path: '/repo/dormouse.yml', name: 'viewer', run: 'view', upstreamUrl: 'https://example.com/repo.git' }; + }); + Object.assign(fake, { toolControl }); + try { + await act(async () => root.render()); + await flush(); + const respond = vi.fn(); + await act(async () => window.dispatchEvent(new CustomEvent('dormouse:control-request', { detail: { + method: SURFACE_CONTROL_METHODS.tool, params: { name: 'viewer', cwd: '/repo' }, respond, + } }))); + const id = respond.mock.calls[0][0].result.surfaceId; + const allow = [...container.querySelectorAll('button')].find(button => button.textContent?.includes('Always allow for folder'))!; + await act(async () => allow.click()); + await flush(); + expect(toolControl.mock.calls.filter(([request]) => request.op === 'lookup')).toHaveLength(1); + expect(container.querySelector('[role="alert"]')?.textContent).toContain(failure === 'missing' ? 'could not be saved' : 'Permission storage is unavailable'); + expect(container.textContent).toContain('Always allow for upstream'); + expect(container.textContent).toContain('Always allow for folder'); + expect(container.textContent).not.toContain('Permission is saved'); + expect(container.querySelector(`[data-session-id="${id}"]`)).toBeNull(); + expect(pendingShellOpts.has(id)).toBe(false); } finally { setToolsEnabled(false); } }); @@ -1628,7 +1676,7 @@ describe('Wall on the Lath engine', () => { } }); - it('starts an approved tool before applying its deferred minimize', async () => { + it.each([false, true])('starts an approved tool before deferred minimize and never resurrects approval after a PTY creation error (%s)', async failAfterSpawn => { setToolsEnabled(true); let toolId: string | undefined; let consumedOpts: (typeof pendingShellOpts extends Map ? T : never) | undefined; @@ -1636,6 +1684,7 @@ describe('Wall on the Lath engine', () => { consumedOpts = pendingShellOpts.get(id); pendingShellOpts.delete(id); fake.spawnPty(id); + if (failAfterSpawn) throw new Error('PTY setup failed after spawning'); return {} as ReturnType; }); let lookupCount = 0; @@ -1685,8 +1734,15 @@ describe('Wall on the Lath engine', () => { expect(getTerminalSpy).toHaveBeenCalledWith(toolId); expect(consumedOpts).toMatchObject({ cwd: '/repo', command: 'pnpm storybook', untouched: true }); expect(pendingShellOpts.has(toolId)).toBe(false); - expect(container.querySelector(`[data-door-id="${toolId}"]`)).not.toBeNull(); - expect(container.querySelector(`[data-lath-leaf="${toolId}"]`)?.hasAttribute('data-lath-parked')).toBe(true); + expect(container.textContent).not.toContain('Always allow'); + expect(container.querySelector('[role="alert"]')).toBeNull(); + if (failAfterSpawn) { + expect(container.querySelector(`[data-session-id="${toolId}"]`)).not.toBeNull(); + expect(container.querySelector(`[data-door-id="${toolId}"]`)).toBeNull(); + } else { + expect(container.querySelector(`[data-door-id="${toolId}"]`)).not.toBeNull(); + expect(container.querySelector(`[data-lath-leaf="${toolId}"]`)?.hasAttribute('data-lath-parked')).toBe(true); + } } finally { if (toolId && fake.hasPty(toolId)) act(() => fake.killPty(toolId)); getTerminalSpy.mockRestore(); diff --git a/lib/src/components/Wall.tsx b/lib/src/components/Wall.tsx index d03fabdf6..7d902ae8c 100644 --- a/lib/src/components/Wall.tsx +++ b/lib/src/components/Wall.tsx @@ -1561,35 +1561,45 @@ export function Wall({ // Approving a pending tool: record the grant, then start the command in the // pane that has been showing the prompt. The two steps are ordered so a // failed write never leaves a running command in an unapproved repo. - const resolveToolApproval = useCallback(async (id: string, choice: 'upstream' | 'folder' | 'decline') => { + const resolveToolApproval = useCallback(async (id: string, choice: 'upstream' | 'folder' | 'decline' | 'retry') => { const meta = lath.getMeta(id); - const pending = toolPendingFromParams(meta?.params); + let pending = toolPendingFromParams(meta?.params); if (!pending || closingWorkspaceRef.current || isWorkspaceTransferPending(effectiveWorkspaceId)) return; if (choice === 'decline') { - // A refusal writes nothing: it closes the pane and leaves no record, so a - // reflexive decline cannot permanently disable tools for this repo. + // Closing writes no denial and does not revoke an already saved grant. await closeSurface(id); return; } if (toolApprovalsInFlightRef.current.has(id)) return; toolApprovalsInFlightRef.current.add(id); - // Whether the prompt's pane is still ours to write to after an await. - const stillPending = () => !closingWorkspaceRef.current && !isWorkspaceTransferPending(effectiveWorkspaceId) - && !!lath.getMeta(id) && !lath.isDying(id) && !isSurfaceClosing(id); - // A failed launch keeps the prompt up with its reason, so the user can fix - // the input and approve again or close the pane. + const isCurrent = () => !closingWorkspaceRef.current + && !isWorkspaceTransferPending(effectiveWorkspaceId) + && toolPendingFromParams(lath.getMeta(id)?.params) === pending + && !lath.isDying(id) && !isSurfaceClosing(id); const showFailure = (message: string) => { - if (stillPending()) lath.store.updateParams(id, { toolPending: { ...pending, error: message } }); + if (isCurrent()) { + lath.store.updateParams(id, { toolPending: { ...pending, error: message } }); + } }; try { const platform = getPlatform(); - const grant = await platform.toolControl?.({ - op: 'trust', - kind: choice, - projectRoot: pending.projectRoot, - }); - if (grant?.status !== 'trust-recorded') return; + if (!pending.trustRecorded) { + // A stale Retry action cannot grant trust. + if (choice === 'retry') return; + const grant = await platform.toolControl?.({ + op: 'trust', + kind: choice, + projectRoot: pending.projectRoot, + }); + if (grant?.status !== 'trust-recorded') { + showFailure(grant?.status === 'error' ? grant.message : 'The Tool permission could not be saved. Try allowing it again.'); + return; + } + if (!isCurrent()) return; + pending = { ...pending, trustRecorded: true, error: undefined }; + lath.store.updateParams(id, { toolPending: pending }); + } // Re-resolve now that the grant exists. The untrusted lookup deliberately // withholds `render` / `port` / `key` — they live only in the `ok` arm — so @@ -1602,7 +1612,7 @@ export function Wall({ return; } - if (!stillPending()) return; + if (!isCurrent()) return; const command = toolRunCommand(resolved.run); lath.store.updateParams(id, { command, @@ -2006,7 +2016,7 @@ export function Wall({ resolveSurfaceRef: surfaceRefForId, // Pin the terminal forward past serving, or release it. Visibility only — // ToolPanel keeps both halves mounted (docs/specs/dor-tool.md). - onResolveToolApproval: (id: string, choice: 'upstream' | 'folder' | 'decline') => { + onResolveToolApproval: (id: string, choice: 'upstream' | 'folder' | 'decline' | 'retry') => { void resolveToolApproval(id, choice); }, }), [addSplitPanel, minimizePane, enterTerminalMode, exitTerminalMode, requestKill, replaceSurface, buildDorSurfaces, createContentSurface, surfaceRefForId, updateSurfaceParams, resolveToolApproval, lath, nav]); diff --git a/lib/src/components/wall/ToolApproval.tsx b/lib/src/components/wall/ToolApproval.tsx index cd4176db4..2d6ea5e5b 100644 --- a/lib/src/components/wall/ToolApproval.tsx +++ b/lib/src/components/wall/ToolApproval.tsx @@ -16,7 +16,7 @@ import { toolPendingFromParams } from './browser-surface'; import type { PaneProps } from './pane-props'; export function ToolApproval({ params, id, onResolve }: PaneProps & { - onResolve: (id: string, choice: 'upstream' | 'folder' | 'decline') => void; + onResolve: (id: string, choice: 'upstream' | 'folder' | 'decline' | 'retry') => void; }) { const pending = toolPendingFromParams(params); if (!pending) return null; @@ -33,9 +33,15 @@ export function ToolApproval({ params, id, onResolve }: PaneProps & { {pending.error ?
{pending.error}
: null}
- {/* Omitted when git named no remote: there is no URL to key a grant on, - so the folder is the only honest scope. */} - {pending.upstreamUrl ? ( + {pending.trustRecorded ? ( + + ) : pending.upstreamUrl ? ( + {!pending.trustRecorded ? ( + + ) : null}
- {pending.path} decides what this runs. Allowing the upstream covers every - worktree of it; allowing the folder covers this checkout only. Declining - records nothing. + {pending.trustRecorded ? 'Permission is saved. Retry checks the Tool configuration again. Closing this pane keeps the permission.' : ( + <>{pending.path} decides what this runs. Allowing the upstream covers every + worktree of it; allowing the folder covers this checkout only. Declining + records nothing. + )}
); diff --git a/lib/src/components/wall/browser-surface.ts b/lib/src/components/wall/browser-surface.ts index 9e737b008..86d808040 100644 --- a/lib/src/components/wall/browser-surface.ts +++ b/lib/src/components/wall/browser-surface.ts @@ -66,6 +66,8 @@ export interface ToolPending { readonly args?: string[]; /** Why the last approval attempt launched nothing; the prompt stays up. */ readonly error?: string; + /** The host confirmed the grant; subsequent attempts only repeat lookup. */ + readonly trustRecorded?: boolean; readonly path: string; readonly projectRoot: string; /** Requested at launch; applied after approval, since a pane the user cannot diff --git a/lib/src/components/wall/wall-context.tsx b/lib/src/components/wall/wall-context.tsx index 6d3db858c..2f8e91209 100644 --- a/lib/src/components/wall/wall-context.tsx +++ b/lib/src/components/wall/wall-context.tsx @@ -64,7 +64,7 @@ export interface WallActions { resolveSurfaceRef: (id: string) => string; /** Resolve a pending tool's approval: grant and start it, or close its pane * (docs/specs/dor-tool.md -> Trust). */ - onResolveToolApproval?: (id: string, choice: 'upstream' | 'folder' | 'decline') => void; + onResolveToolApproval?: (id: string, choice: 'upstream' | 'folder' | 'decline' | 'retry') => void; } export const WallActionsContext = createContext({ diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index add79bd6a..4114da6ae 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -7,7 +7,7 @@ "docs/specs/deploy.md": 1900, "docs/specs/dor-browser.md": 4600, "docs/specs/dor-cli.md": 5950, - "docs/specs/dor-tool.md": 2900, + "docs/specs/dor-tool.md": 2950, "docs/specs/glossary.md": 3000, "docs/specs/layout.md": 8800, "docs/specs/mobile-terminal-ui.md": 1950, From b5f2dfbfb61c8b5b9c1037ab629ef632ed5829e9 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 15 Sep 2026 18:00:24 -0700 Subject: [PATCH 05/10] Retain early rejection of special Tool configuration files --- lib/src/host/tool-trust.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/src/host/tool-trust.ts b/lib/src/host/tool-trust.ts index 904319977..5982cd12d 100644 --- a/lib/src/host/tool-trust.ts +++ b/lib/src/host/tool-trust.ts @@ -35,6 +35,9 @@ export async function readToolFile(path: string, options: { followSymlink?: bool if (entry.isSymbolicLink() && !options.followSymlink) { throw new ToolFileError(`${path}: tool file must be a regular file, not a symbolic link`); } + // Avoid opening known devices/FIFOs; fstat below also checks the actual + // descriptor after a symlink follow or concurrent path replacement. + if (!entry.isSymbolicLink() && !entry.isFile()) throw new ToolFileError(`${path}: tool file must be a regular file`); let file; try { From 28d4703f459643f328d9b639fc4ada3997346cc6 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 15 Sep 2026 18:15:52 -0700 Subject: [PATCH 06/10] Protect Tool argv and quote for the destination shell --- docs/specs/dor-tool.md | 6 +- docs/specs/dor-tool.rationale.md | 2 + docs/specs/security-local.md | 2 + dor/src/commands/shell-quote.ts | 6 ++ lib/src/components/Wall.test.tsx | 66 +++++++++++++++++++++- lib/src/components/wall/use-dor-control.ts | 24 ++++++-- lib/src/host/tool-input.test.ts | 46 +++++++++++++++ lib/src/host/tool-input.ts | 10 +++- lib/src/host/tool-registry.ts | 4 +- scripts/spec-word-budgets.json | 2 +- 10 files changed, 155 insertions(+), 13 deletions(-) create mode 100644 lib/src/host/tool-input.test.ts diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index c9b4989d9..2b24b75c0 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -48,11 +48,11 @@ Source of truth: `surfaceKindFromParams` / `isToolParams` in `lib/src/components - **Must warn when a repo-local key omits `$PROJECT_ROOT`**, while allowing intentional cross-checkout dedupe. - **Must reject `$PROJECT_ROOT` in user configuration**, which has no project root. -**Must pass named-tool inputs as argument values, never substitute them into a shell-command string.** String `run` accepts no arguments and remains literal shell syntax. List `run` expands `$TARGET`, `$CWD`, and `$PROJECT_ROOT` within elements; a whole `$ARGS` element expands all input arguments. Without `$ARGS` or `$TARGET` in the list, append the inputs. The renderer quotes the resulting argv for its configured shell. +**Must pass named-tool inputs as argument values, never substitute them into a shell-command string.** String `run` accepts no arguments and remains literal shell syntax. List `run` expands `$TARGET`, `$CWD`, and `$PROJECT_ROOT` within elements; a whole `$ARGS` element expands all input arguments. Without `$ARGS` or `$TARGET` in the list, append the inputs. **Must quote argv for the destination Session's shell**, using the current default only for new Sessions; takeover stores that quoted command for reruns. -**Must require exactly one existing regular local file when `$TARGET` appears in the run list or dedupe key.** Resolve relative paths against the invocation CWD and follow symlinks to a canonical absolute path before substitution and reuse. Reject URLs, directories, and missing files. Validate run and key inputs before showing approval. Pending approval retains the original arguments and distinguishes requests with different inputs; [Trust](#trust) owns re-resolution and recovery. +**Must require exactly one existing regular local file when `$TARGET` appears in the run list or dedupe key.** Resolve relative paths against the invocation CWD and follow symlinks to a canonical absolute path before substitution and reuse. Reject URLs, directories, and missing files. Validate run and key inputs before showing approval. Pending approval distinguishes the original arguments and invocation CWD; [Trust](#trust) owns re-resolution and recovery. Input control-character restrictions belong to `docs/specs/security-local.md` → Dor Tool configuration. -Source of truth: `lookupTool` in `lib/src/host/tool-trust.ts`; `parseToolFile` / `resolveDedupeKey` in `lib/src/host/tool-registry.ts`; `resolveToolInput` in `lib/src/host/tool-input.ts`; `readUserToolFile` in `lib/src/host/tool-user-config.ts`; `lib/src/host/tool-host.test.ts`, `lib/src/host/tool-registry.test.ts`. +Source of truth: `lookupTool` in `lib/src/host/tool-trust.ts`; `parseToolFile` / `resolveDedupeKey` in `lib/src/host/tool-registry.ts`; `resolveToolInput` in `lib/src/host/tool-input.ts`; `readUserToolFile` in `lib/src/host/tool-user-config.ts`; `toolRunCommand` in `lib/src/components/wall/use-dor-control.ts`; `lib/src/host/tool-host.test.ts`, `lib/src/components/Wall.test.tsx`. ## Identity and dedupe diff --git a/docs/specs/dor-tool.rationale.md b/docs/specs/dor-tool.rationale.md index 62b274e64..ea4402480 100644 --- a/docs/specs/dor-tool.rationale.md +++ b/docs/specs/dor-tool.rationale.md @@ -10,6 +10,8 @@ A misspelled substitution such as `$PROJECTROOT` retained as a literal silently Argument-list commands let the renderer quote each value for the actual target shell. Keeping shell strings literal avoids needing a shell-template parser to distinguish an author-provided pipeline from punctuation in a filename. Canonical file targets make symlink aliases reuse the same document viewer. +A Session can keep running PowerShell after its user's default changes to Bash. Takeover therefore cannot use the default's quotation rules: apostrophes and quoted executable paths differ between those shells. Pending invocations also depend on their CWD, since identical relative filenames in two subdirectories identify different documents. + ## Identity and dedupe `pnpm storybook`, `pnpm run storybook`, and `pnpm storybook --quiet` are different command strings for the same intended tool. `dor ensure` already supplies exact-command/CWD identity. An explicit Tool key allows authors to choose their own scope without making the declaration of a short command name implicitly enable dedupe. diff --git a/docs/specs/security-local.md b/docs/specs/security-local.md index b6e673986..65327c9de 100644 --- a/docs/specs/security-local.md +++ b/docs/specs/security-local.md @@ -204,6 +204,8 @@ Source of truth: `context` in `standalone/sidecar/pty-core.js`; `attachRouter` i **Must keep named-tool inputs as argv until the renderer quotes them for the target shell.** User configuration is the local user's authority; a project name cannot replace a user Tool during user-only lookup. Resolution belongs to `docs/specs/dor-tool.md` → Declaring tools. +**Must reject C0 and DEL characters in Tool argv, substituted argv, and local-file targets before launch**, including controls exposed by canonicalizing symlinks. Shell quotes do not protect terminal editing keys. String `run` remains explicit shell code. Source of truth: `hasShellInputControls` in `dor/src/commands/shell-quote.ts`; `resolveToolInput` in `lib/src/host/tool-input.ts`; `useDorControl` in `lib/src/components/wall/use-dor-control.ts`. Tests: `lib/src/host/tool-input.test.ts`, `lib/src/components/Wall.test.tsx`. + **Must derive the grant key in the host**, using the canonical upstream URL or project-root folder; a renderer request cannot supply an arbitrary grant URL. **Must bound config reads and refuse repo-config symlinks on every host.** The user config may follow a dotfiles symlink; its opened descriptor must still be a bounded regular file. **An upstream grant trusts the claimed URL, not authenticated checkout provenance.** A supplied directory containing its own `.git/config` can claim an already-granted upstream; folder-only grants limit this sharing. **Must not describe the chrome gesture as a boundary against other processes running as the user**; the local account model is The dor control socket above. diff --git a/dor/src/commands/shell-quote.ts b/dor/src/commands/shell-quote.ts index 8576748f7..d9b7b7cc9 100644 --- a/dor/src/commands/shell-quote.ts +++ b/dor/src/commands/shell-quote.ts @@ -7,6 +7,12 @@ export type ShellCommandKind = 'cmd' | 'posix' | 'powershell'; +/** Shell quotes cannot protect bytes that an interactive terminal interprets + * as editing keys, escape sequences, or line submission before shell parsing. */ +export function hasShellInputControls(value: string): boolean { + return /[\x00-\x1f\x7f]/.test(value); +} + const POSIX_SAFE_ARG = /^[A-Za-z0-9_@%+=:,./-]+$/; // No `,` or `@`, unlike the posix set: PowerShell's argument mode reads a comma // as the array operator (`cat a,b.txt` passes two arguments), while an initial diff --git a/lib/src/components/Wall.test.tsx b/lib/src/components/Wall.test.tsx index 1d04725ef..1cf7c6f17 100644 --- a/lib/src/components/Wall.test.tsx +++ b/lib/src/components/Wall.test.tsx @@ -1564,15 +1564,16 @@ describe('Wall on the Lath engine', () => { await act(async () => root.render()); await flush(); const ids: string[] = []; - for (const target of ['a b;$(bad).md', 'second.md', 'a b;$(bad).md']) { + for (const [cwd, target] of [['/repo', 'a b;$(bad).md'], ['/repo', 'second.md'], ['/repo', 'a b;$(bad).md'], ['/repo/subdir', 'a b;$(bad).md']]) { const respond = vi.fn(); await act(async () => window.dispatchEvent(new CustomEvent('dormouse:control-request', { detail: { - method: SURFACE_CONTROL_METHODS.tool, params: { name: 'viewer', cwd: '/repo', args: [target] }, respond, + method: SURFACE_CONTROL_METHODS.tool, params: { name: 'viewer', cwd, args: [target] }, respond, } }))); ids.push(respond.mock.calls[0][0].result.surfaceId); } expect(ids[0]).not.toBe(ids[1]); expect(ids[2]).toBe(ids[0]); + expect(ids[3]).not.toBe(ids[0]); const allow = [...container.querySelectorAll('button')].find(button => button.textContent?.includes('Always allow for folder'))!; await act(async () => allow.click()); await flush(); @@ -1974,6 +1975,67 @@ describe('Wall on the Lath engine', () => { } }); + it('rejects anonymous Tool argv containing terminal editing controls before launching', async () => { + setToolsEnabled(true); + try { + await act(async () => root.render()); + await flush(); + const write = vi.spyOn(fake, 'writePty'); + const respond = vi.fn(); + await act(async () => window.dispatchEvent(new CustomEvent('dormouse:control-request', { detail: { + method: SURFACE_CONTROL_METHODS.tool, surfaceId: 'pane-a', + params: { command: ['view', '/tmp/\x15printf unwanted\n#'], cwd: '/repo' }, respond, + } }))); + expect(respond).toHaveBeenCalledWith({ ok: false, error: 'tool arguments cannot contain terminal control characters' }); + expect(write).not.toHaveBeenCalled(); + expect(leafCount()).toBe(1); + } finally { setToolsEnabled(false); } + }); + + it.each([ + { kind: 'powershell' as const, defaultShell: '/bin/bash', command: "& 'program path' 'it''s.txt'" }, + { kind: 'posix' as const, defaultShell: 'pwsh.exe', command: "'program path' 'it'\\''s.txt'" }, + ])('quotes takeover and keyed rerun for the existing $kind Session after changing defaults', async ({ kind, defaultShell, command }) => { + setToolsEnabled(true); + const controller = new AbortController(); + const typed: string[] = []; + vi.spyOn(terminalRegistry, 'getTerminalShellKind').mockImplementation(id => id === 'pane-a' ? kind : null); + vi.spyOn(terminalRegistry, 'getDefaultShellOpts').mockReturnValue({ shell: defaultShell }); + Object.assign(fake, { toolControl: vi.fn(async () => ({ ...okToolLookup(['/repo']), run: ['program path', "it's.txt"] })) }); + try { + await act(async () => root.render()); + await flush(); + act(() => fake.spawnPty('pane-a')); + fake.setInputHandler('pane-a', data => typed.push(data)); + terminalRegistry.seedTerminalManualCwd('pane-a', '/repo'); + for (const status of ['takeover', 'adopted']) { + act(() => reportRunning('pane-a', 'dor tool storybook')); + const respond = vi.fn(); + await act(async () => window.dispatchEvent(new CustomEvent('dormouse:control-request', { detail: { + method: SURFACE_CONTROL_METHODS.tool, surfaceId: 'pane-a', + params: { name: 'storybook', cwd: '/repo' }, signal: controller.signal, respond, + } }))); + await waitUntil(() => respond.mock.calls.length > 0); + expect(respond).toHaveBeenCalledWith(expect.objectContaining({ result: expect.objectContaining({ status, command }) })); + const before = typed.length; + act(() => promptBack('pane-a')); + await waitUntil(() => typed.length > before); + expect(typed.at(-1)).toBe(`${command}\r`); + act(() => { + reportRunning('pane-a', command); + terminalRegistry.applyTerminalSemanticEvents('pane-a', [{ type: 'commandFinish', exitCode: 0 }, { type: 'promptStart' }]); + }); + await act(async () => { await new Promise(resolve => setTimeout(resolve, 150)); }); + } + expect(leafCount()).toBe(1); + } finally { + await act(async () => { controller.abort(); await new Promise(resolve => setTimeout(resolve, 125)); }); + fake.clearInputHandler('pane-a'); + act(() => terminalRegistry.removeTerminalPaneState('pane-a')); + setToolsEnabled(false); + } + }); + it('takes over the calling pane when `dor tool` is typed alone at a prompt', async () => { setToolsEnabled(true); const typed: string[] = []; diff --git a/lib/src/components/wall/use-dor-control.ts b/lib/src/components/wall/use-dor-control.ts index 59f2ec171..c9601f4d2 100644 --- a/lib/src/components/wall/use-dor-control.ts +++ b/lib/src/components/wall/use-dor-control.ts @@ -17,12 +17,13 @@ import { hasBrowser, hasTerminal } from 'dor/commands/types'; import { MAX_AWAIT_TIMEOUT_MS } from '../../lib/alert-manager'; import { TOOLS_FLAG_KEY, isToolsEnabled } from '../../lib/feature-flags'; import type { OpenPort } from '../../lib/platform/types'; -import { buildShellCommandForKind, shellCommandKind } from 'dor/commands/shell-quote'; +import { buildShellCommandForKind, hasShellInputControls, shellCommandKind } from 'dor/commands/shell-quote'; import { UNNAMED_PANEL_TITLE, getDefaultShellOpts, getTerminalInstance, getTerminalPaneState, + getTerminalShellKind, isPaneOscDriven, } from '../../lib/terminal-registry'; import { cwdPathsEqual, surfaceRunsCommand, type TerminalPaneState } from '../../lib/terminal-state'; @@ -474,11 +475,14 @@ function dorCommandString(args: string[] | undefined): string | undefined { } /** The command a resolved Tool types into its shell: a string `run` is literal - * shell syntax, an argument-list `run` is quoted here for the default shell + * shell syntax, an argument-list `run` uses the destination Session's shell + * or the default shell when launching a new Session * (`docs/specs/dor-tool.md` -> Declaring tools). The host guarantees a list * names an executable, so the empty-argv case cannot arise. */ -export function toolRunCommand(run: string | readonly string[]): string { - return typeof run === 'string' ? run : dorCommandString([...run])!; +export function toolRunCommand(run: string | readonly string[], terminalId?: string): string { + const kind = (terminalId ? getTerminalShellKind(terminalId) : null) + ?? shellCommandKind(getDefaultShellOpts()?.shell, PLATFORM_STRING); + return typeof run === 'string' ? run : buildShellCommandForKind(kind, run); } /** @@ -864,6 +868,7 @@ export function useDorControl({ } const toolName = stringParam(params.name)?.trim(); let command: string; + let toolRun: string | readonly string[]; let key: string[] | null = null; let toolScope: 'user' | undefined; const toolArgs = stringArrayParam(params.args) ?? []; @@ -931,6 +936,7 @@ export function useDorControl({ detail.respond({ ok: false, error: 'unexpected tool host response' }); return; case 'ok': + toolRun = lookup.run; command = toolRunCommand(lookup.run); toolScope = lookup.scope; // Namespaced under the host-resolved tool name, so two tools in @@ -968,6 +974,7 @@ export function useDorControl({ const matchesPending = (candidate: unknown) => { const waiting = toolPendingFromParams(candidate); return waiting?.name === lookup.name && waiting.projectRoot === lookup.projectRoot + && cwdPathsEqual(stringParam((candidate as { cwd?: unknown } | null)?.cwd), cwd) && toolKeysEqual(waiting.args ?? [], toolArgs); }; const already = findSurfaceByParams(matchesPending); @@ -1036,11 +1043,16 @@ export function useDorControl({ } } else { const argv = stringArrayParam(params.command); + if (argv?.some(hasShellInputControls)) { + detail.respond({ ok: false, error: 'tool arguments cannot contain terminal control characters' }); + return; + } command = dorCommandString(argv) ?? ''; if (!command) { detail.respond({ ok: false, error: 'command cannot be empty' }); return; } + toolRun = argv!; } const toolParams = { @@ -1065,7 +1077,7 @@ export function useDorControl({ && toolKeysEqual((candidate as { toolKey?: unknown } | null | undefined)?.toolKey, key); const match = findSurfaceByParams(matchesToolKey); if (match) { - const matchedCommand = toolCommandFromParams(lath.getMeta(match.id)?.params) ?? command; + const matchedCommand = toolCommandFromParams(lath.getMeta(match.id)?.params) ?? toolRunCommand(toolRun, match.id); const matchState = getTerminalPaneState(match.id); // The tool's own cwd, not the caller's: `surfaceRunsCommand` // compares against the matched Surface's `cwdAtStart`, so waiting @@ -1135,6 +1147,8 @@ export function useDorControl({ // below the pending-approval and key-match returns above: both of those // placements win over this one. if (callerId && callerGate && toolTakesOverCaller(callerGate)) { + command = toolRunCommand(toolRun, callerId); + toolParams.command = command; // Answered before the tool starts, because answering is what frees // the shell to run it. respondTool('takeover', { surfaceId: callerId, command, cwd, minimized: false }); diff --git a/lib/src/host/tool-input.test.ts b/lib/src/host/tool-input.test.ts new file mode 100644 index 000000000..da272f656 --- /dev/null +++ b/lib/src/host/tool-input.test.ts @@ -0,0 +1,46 @@ +import { mkdtemp, realpath, rm, symlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { resolveToolInput } from './tool-input'; +import { parseToolFile } from './tool-registry'; + +const context = { cwd: '/repo', projectRoot: '/repo', args: [] as string[] }; +const entry = { name: 'viewer', run: ['viewer', '$ARGS'], dedupeTemplate: null }; + +describe('Tool argv safety', () => { + it.each([...Array.from({ length: 32 }, (_, code) => code), 127])('rejects terminal control byte %i before quoting', async code => { + await expect(resolveToolInput(entry, { ...context, args: [`file${String.fromCharCode(code)}name`] })) + .rejects.toThrow('terminal control characters'); + }); + + it('rejects controls introduced by directory substitutions', async () => { + await expect(resolveToolInput({ ...entry, run: ['viewer', '$CWD'] }, { ...context, cwd: '/repo/\x15printf unwanted\n#' })) + .rejects.toThrow('terminal control characters'); + await expect(resolveToolInput({ ...entry, run: ['viewer', '$PROJECT_ROOT'] }, { ...context, projectRoot: '/repo/\x1b[2J' })) + .rejects.toThrow('terminal control characters'); + }); + + it('rejects control-bearing argument-list configuration but preserves literal shell scripts', () => { + const parse = (run: string | string[]) => parseToolFile(JSON.stringify({ tools: { viewer: { run } } }), { path: '/repo/dormouse.yml', dir: '/repo', scope: 'repo' }); + expect(() => parse(['viewer', 'first\nsecond'])).toThrow('terminal control characters'); + expect(parse('echo first\necho second').tools.get('viewer')?.run).toBe('echo first\necho second'); + }); + + it('rejects controls in file inputs and their resolving directory', async () => { + const targetEntry = { ...entry, run: ['viewer', '$TARGET'] }; + await expect(resolveToolInput(targetEntry, { ...context, args: ['\x15printf unwanted\n#'] })).rejects.toThrow('terminal control characters'); + await expect(resolveToolInput(targetEntry, { ...context, cwd: '/repo/\tpath', args: ['file.txt'] })).rejects.toThrow('terminal control characters'); + }); + + it.skipIf(process.platform === 'win32')('rejects controls hidden behind an ordinary symlink name', async () => { + const dir = await realpath(await mkdtemp(join(tmpdir(), 'dor-input-controls-'))); + try { + const target = join(dir, '\x15printf unwanted\n#'); + await writeFile(target, 'ordinary document'); + await symlink(target, join(dir, 'safe-name.txt')); + await expect(resolveToolInput({ ...entry, run: ['viewer', '$TARGET'] }, { ...context, cwd: dir, args: ['safe-name.txt'] })) + .rejects.toThrow('terminal control characters'); + } finally { await rm(dir, { recursive: true, force: true }); } + }); +}); diff --git a/lib/src/host/tool-input.ts b/lib/src/host/tool-input.ts index c7a4819c5..eaef0f195 100644 --- a/lib/src/host/tool-input.ts +++ b/lib/src/host/tool-input.ts @@ -1,5 +1,6 @@ import { realpath, stat } from 'node:fs/promises'; import { isAbsolute, resolve } from 'node:path'; +import { hasShellInputControls } from 'dor/commands/shell-quote'; import { resolveDedupeKey, substituteToolTokens, ToolFileError, usesTarget, type ToolEntry } from './tool-registry'; /** What one invocation's inputs resolved an entry to: the argv (or literal @@ -12,6 +13,9 @@ export interface ToolInput { /** A target is one existing regular file on this host. Resolve symlinks before * keying, so two paths to the same document reveal the same Tool. */ async function resolveLocalToolTarget(input: string, cwd: string): Promise { + if (hasShellInputControls(input) || hasShellInputControls(cwd)) { + throw new ToolFileError('local file paths cannot contain terminal control characters'); + } if (!input || (!isAbsolute(input) && /^[a-z][a-z\d+.-]*:/i.test(input))) { throw new ToolFileError('expected a local file path, not a URL or Surface handle'); } @@ -23,6 +27,7 @@ async function resolveLocalToolTarget(input: string, cwd: string): Promise { const { args } = context; - if (args.some(arg => typeof arg !== 'string' || arg.includes('\0'))) throw new ToolFileError('invalid tool arguments'); + if (args.some(arg => typeof arg !== 'string' || hasShellInputControls(arg))) { + throw new ToolFileError('tool arguments cannot contain terminal control characters'); + } const runList = typeof entry.run === 'string' ? [] : entry.run; const runHasTarget = usesTarget(runList); const needsTarget = runHasTarget || usesTarget(entry.dedupeTemplate ?? []); @@ -46,6 +53,7 @@ export async function resolveToolInput( } const run = runList.flatMap(arg => arg === '$ARGS' ? [...args] : [substituteToolTokens(arg, substitution, entry.name)]); if (!runHasTarget && !runList.includes('$ARGS')) run.push(...args); + if (run.some(hasShellInputControls)) throw new ToolFileError('tool arguments cannot contain terminal control characters'); if (!run[0]?.trim()) throw new ToolFileError('tool argument list must name an executable'); return { run, key }; } diff --git a/lib/src/host/tool-registry.ts b/lib/src/host/tool-registry.ts index 25003e805..500eaa20a 100644 --- a/lib/src/host/tool-registry.ts +++ b/lib/src/host/tool-registry.ts @@ -8,6 +8,7 @@ */ import { parse as parseYaml } from 'yaml'; import { isRecord } from '../lib/is-record'; +import { hasShellInputControls } from 'dor/commands/shell-quote'; /** Where a tool file came from. `$PROJECT_ROOT` exists only for `repo`. */ export type ToolScope = 'repo' | 'user'; @@ -150,9 +151,10 @@ export function parseToolFile( const run = rawEntry.run; if (Array.isArray(run)) { - if (!run.length || !run.every(arg => typeof arg === 'string' && !arg.includes('\0')) || !run[0].trim()) { + if (!run.length || !run.every(arg => typeof arg === 'string') || !run[0].trim()) { throw new ToolFileError(`${where}: 'run' must be a non-empty argument list`); } + if (run.some(hasShellInputControls)) throw new ToolFileError(`${where}: run arguments cannot contain terminal control characters`); validateSubstitutions(run.filter(arg => arg !== '$ARGS'), scope, where); } else if (typeof run !== 'string' || run.trim() === '') { throw new ToolFileError(`${where}: 'run' is required and must be a non-empty string or argument list`); diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 4114da6ae..d4e2b04ca 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -7,7 +7,7 @@ "docs/specs/deploy.md": 1900, "docs/specs/dor-browser.md": 4600, "docs/specs/dor-cli.md": 5950, - "docs/specs/dor-tool.md": 2950, + "docs/specs/dor-tool.md": 3000, "docs/specs/glossary.md": 3000, "docs/specs/layout.md": 8800, "docs/specs/mobile-terminal-ui.md": 1950, From 897899738c48b9af096221928fc26541bd706046 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 15 Sep 2026 18:24:40 -0700 Subject: [PATCH 07/10] Requote persisted Tool argv for the restored shell --- docs/specs/dor-tool.md | 2 ++ docs/specs/dor-tool.rationale.md | 2 ++ lib/src/components/Wall.test.tsx | 9 +++++ lib/src/components/Wall.tsx | 1 + lib/src/components/wall/use-dor-control.ts | 1 + lib/src/lib/session-restore.test.ts | 41 ++++++++++++++++++++++ lib/src/lib/session-restore.ts | 34 ++++++++++++++---- lib/src/lib/session-save.test.ts | 12 +++++++ lib/src/lib/session-save.ts | 5 +-- lib/src/lib/session-types.ts | 11 ++++++ 10 files changed, 110 insertions(+), 8 deletions(-) diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index 2b24b75c0..eec7607a0 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -180,6 +180,8 @@ The Tool-specific local boundaries are `docs/specs/security-local.md` → Dor To **Must persist the command and stable Tool metadata with `surfaceType: 'tool'`**, retaining the ordinary CWD field. Never persist a derived URL, browser session binding, conflict, or pending approval as runnable Tool state. Live notes follow `docs/specs/notepad.md` → Live resume. +**Must retain resolved argv for argument-list Tools and re-quote it for the shell selected at cold restore.** Update the restored command in terminal options and Tool pane/door metadata. Literal shell-string commands retain their saved text. Reject persisted argv containing terminal controls before restoring any PTY. + **Must cold-restore an approved Tool by starting its saved command through integration-gated shell readiness**, then rediscover its port. Agent-resume commands do not override the saved Tool command. Pending approvals restore as ordinary terminals and execute nothing. **Must rebuild visible Tool metadata from its pane row when layout geometry is unusable**, rather than starting the command in a plain terminal with no serving behavior. **Must retain live Tool browser params and OSC announcements in volatile Workspace-transfer content**, applying them to the destination plan without mutating the durable record. A serving iframe Tool participates in the ordinary iframe move confirmation. **Must refuse transfer while a Tool awaits approval or its browser startup has no session binding.** diff --git a/docs/specs/dor-tool.rationale.md b/docs/specs/dor-tool.rationale.md index ea4402480..4c148963d 100644 --- a/docs/specs/dor-tool.rationale.md +++ b/docs/specs/dor-tool.rationale.md @@ -52,6 +52,8 @@ Hostile text printed by the designated command can contain an announcement. The ## Persistence and hosts +A Tool may take over a PowerShell Session even while the selected default is Bash, and the selected default may change before restart. Its already-quoted command string cannot safely move between those shells. Retaining resolved argv preserves literal filenames and lets cold restore quote for its actual shell without retaining an obsolete shell executable. + A derived URL or browser daemon binding belongs to one execution. Reusing it after cold restore can connect a Tool to another process that obtained the old port. The saved command and declaration metadata are sufficient to start again and discover the new endpoint. Routing `dor tool` to a native editor on one host would change its result from a Surface handle to a host-specific side effect. Native file opening remains a separate operation. diff --git a/lib/src/components/Wall.test.tsx b/lib/src/components/Wall.test.tsx index 1cf7c6f17..0a4de0f9d 100644 --- a/lib/src/components/Wall.test.tsx +++ b/lib/src/components/Wall.test.tsx @@ -17,6 +17,7 @@ import { getAgentBrowserScreenController } from './wall/agent-browser-screen'; import { setPlatform } from '../lib/platform'; import { FakePtyAdapter } from '../lib/platform/fake-adapter'; import type { PlatformAdapter } from '../lib/platform/types'; +import type { PersistedSession } from '../lib/session-types'; import * as terminalRegistry from '../lib/terminal-registry'; import { UNNAMED_PANEL_TITLE } from '../lib/terminal-registry'; import { pendingShellOpts } from '../lib/terminal-store'; @@ -1579,6 +1580,9 @@ describe('Wall on the Lath engine', () => { await flush(); expect(toolControl).toHaveBeenLastCalledWith({ op: 'lookup', name: 'viewer', cwd: '/repo', args: ['a b;$(bad).md'] }); expect(pendingShellOpts.get(ids[0])?.command).toBe("view 'a b;$(bad).md'"); + await act(async () => window.dispatchEvent(new Event('pagehide'))); + await flush(); + expect((fake.getState() as PersistedSession).panes.find(pane => pane.id === ids[0])?.tool?.argv).toEqual(['view', 'a b;$(bad).md']); ids.forEach(id => pendingShellOpts.delete(id)); } finally { setToolsEnabled(false); } }); @@ -2028,6 +2032,11 @@ describe('Wall on the Lath engine', () => { await act(async () => { await new Promise(resolve => setTimeout(resolve, 150)); }); } expect(leafCount()).toBe(1); + await act(async () => window.dispatchEvent(new Event('pagehide'))); + await flush(); + expect((fake.getState() as PersistedSession).panes.find(pane => pane.id === 'pane-a')).toMatchObject({ + command, tool: { argv: ['program path', "it's.txt"] }, + }); } finally { await act(async () => { controller.abort(); await new Promise(resolve => setTimeout(resolve, 125)); }); fake.clearInputHandler('pane-a'); diff --git a/lib/src/components/Wall.tsx b/lib/src/components/Wall.tsx index ae11e6fc2..7291c53e2 100644 --- a/lib/src/components/Wall.tsx +++ b/lib/src/components/Wall.tsx @@ -1627,6 +1627,7 @@ export function Wall({ const command = toolRunCommand(resolved.run); lath.store.updateParams(id, { command, + toolArgv: typeof resolved.run === 'string' ? undefined : [...resolved.run], ...(resolved.scope ? { toolScope: resolved.scope } : {}), toolRender: resolved.render, toolPort: resolved.port, diff --git a/lib/src/components/wall/use-dor-control.ts b/lib/src/components/wall/use-dor-control.ts index c9601f4d2..7ff312ca8 100644 --- a/lib/src/components/wall/use-dor-control.ts +++ b/lib/src/components/wall/use-dor-control.ts @@ -1058,6 +1058,7 @@ export function useDorControl({ const toolParams = { surfaceType: 'tool', command, + ...(typeof toolRun === 'string' ? {} : { toolArgv: [...toolRun] }), cwd, toolRender: render, ...(toolScope ? { toolScope } : {}), diff --git a/lib/src/lib/session-restore.test.ts b/lib/src/lib/session-restore.test.ts index 389ede0ee..9763db83d 100644 --- a/lib/src/lib/session-restore.test.ts +++ b/lib/src/lib/session-restore.test.ts @@ -220,6 +220,47 @@ describe('restoreSession', () => { })); }); + it.each([ + { shell: '/bin/bash', oldCommand: "& 'program path' 'it''s.txt'", command: "'program path' 'it'\\''s.txt'" }, + { shell: 'pwsh.exe', oldCommand: "'program path' 'it'\\''s.txt'", command: "& 'program path' 'it''s.txt'" }, + ])('re-quotes a Tool from another shell for cold restore with $shell', ({ shell, oldCommand, command }) => { + terminalRegistryMocks.getDefaultShellOpts.mockReturnValue({ shell }); + const argv = ['program path', "it's.txt"]; + for (const placement of ['layout', 'fallback', 'door']) { + const saved: PersistedSession = { + version: 3, + panes: [{ id: 'tool', title: 'Viewer', cwd: '/repo', untouched: false, surfaceType: 'tool', command: oldCommand, + tool: { render: 'iframe', port: 'announced', argv } }], + ...(placement === 'layout' ? { lathLayout: { + version: 1, tree: { root: { kind: 'leaf', id: 'tool' } }, + leafMeta: { tool: { component: 'tool', tabComponent: 'tool', title: 'Viewer', params: { surfaceType: 'tool', command: oldCommand, toolArgv: argv } } }, + } } : {}), + ...(placement === 'door' ? { doors: [{ id: 'tool', title: 'Viewer', component: 'tool', params: { surfaceType: 'tool', command: oldCommand, toolArgv: argv } }] } : {}), + }; + const before = JSON.stringify(saved); + const result = restoreSession(createPlatform(saved)); + expect(terminalRegistryMocks.restoreTerminal).toHaveBeenLastCalledWith('tool', expect.objectContaining({ shell, command, requireIntegration: true })); + const params = placement === 'door' ? result?.doors[0].params : result?.lathLayout?.leafMeta.tool.params; + expect(params).toMatchObject({ command, toolArgv: argv }); + expect(JSON.stringify(saved)).toBe(before); // rebuilding must not change the durable record + } + }); + + it('retains literal shell commands when the selected restore shell changes', () => { + terminalRegistryMocks.getDefaultShellOpts.mockReturnValue({ shell: 'pwsh.exe' }); + const command = 'echo "$HOME" | cat'; + restoreSession(createPlatform({ version: 3, panes: [{ id: 'tool', title: 'Literal', cwd: '/repo', untouched: false, surfaceType: 'tool', command }] })); + expect(terminalRegistryMocks.restoreTerminal).toHaveBeenCalledWith('tool', expect.objectContaining({ command, shell: 'pwsh.exe' })); + }); + + it.each(['\t', '\n', '\r', '\x1b'])('rejects control-bearing saved Tool argv before any terminal is restored (%j)', control => { + const saved: PersistedSession = { version: 3, panes: [{ id: 'tool', title: 'Unsafe', cwd: '/repo', untouched: false, surfaceType: 'tool', command: 'safe fallback', + tool: { render: 'iframe', port: 'announced', argv: ['program', `file${control}command`] } }] }; + expect(restoreSession(createPlatform(saved))).toBeNull(); + expect(restoreSession(createPlatform(null), { savedSession: saved })).toBeNull(); + expect(terminalRegistryMocks.restoreTerminal).not.toHaveBeenCalled(); + }); + it.each([undefined, { version: 1 }, { version: 1, tree: { root: { kind: 'leaf', id: 'stale-pane' } }, diff --git a/lib/src/lib/session-restore.ts b/lib/src/lib/session-restore.ts index 0ff952709..bba166857 100644 --- a/lib/src/lib/session-restore.ts +++ b/lib/src/lib/session-restore.ts @@ -1,6 +1,8 @@ import type { LathNode } from './lath/model'; import { type LathPersistedLayout, isLathPersistedLayout } from './lath/persistence'; import type { PlatformAdapter } from './platform/types'; +import { PLATFORM_STRING } from './platform'; +import { buildShellCommandForKind, shellCommandKind } from 'dor/commands/shell-quote'; import { carrySurfaceRefs, readPersistedSession, type PersistedDoor, type PersistedSession, type PersistedSurfaceRefs } from './session-types'; import { getDefaultShellOpts, restoreBrowserSurfaceTodo, restoreTerminal } from './terminal-registry'; @@ -31,18 +33,38 @@ export interface RestoreSources { } export function restoreSession(platform: PlatformAdapter, sources: RestoreSources = {}): RestoredSession | null { - const saved = sources.savedSession !== undefined + const saved = readPersistedSession(sources.savedSession !== undefined ? sources.savedSession - : readPersistedSession(platform.getState()); + : platform.getState()); if (!saved || !saved.panes || saved.panes.length === 0) return null; - const doors = saved.doors ?? []; + const shellOpts = getDefaultShellOpts(); + const shellKind = shellCommandKind(shellOpts?.shell, PLATFORM_STRING); + // The saved string belongs to the previous Session's shell. Preserve literal + // commands, but quote saved argv anew for the shell this restore will spawn. + const panes = saved.panes.map(pane => pane.surfaceType === 'tool' && pane.tool?.argv + ? { ...pane, command: buildShellCommandForKind(shellKind, pane.tool.argv) } : pane); + const panesById = new Map(panes.map(pane => [pane.id, pane])); + const doors = (saved.doors ?? []).map(door => { + const pane = panesById.get(door.id); + return pane?.surfaceType === 'tool' && pane.tool?.argv + ? { ...door, params: { ...door.params, command: pane.command, toolArgv: pane.tool.argv } } : door; + }); const doorIds = new Set(doors.map((item) => item.id)); - const visiblePanes = saved.panes.filter((pane) => !doorIds.has(pane.id)); + const visiblePanes = panes.filter((pane) => !doorIds.has(pane.id)); const visibleIds = new Set(visiblePanes.map((pane) => pane.id)); const candidateLayout = persistedLathLayout(saved); const leafIds = candidateLayout ? Object.keys(candidateLayout.leafMeta) : []; let lathLayout = candidateLayout && leafIds.length === visibleIds.size && leafIds.every((id) => visibleIds.has(id)) ? candidateLayout : undefined; + if (lathLayout) { + const leafMeta = { ...lathLayout.leafMeta }; + for (const pane of visiblePanes) { + if (pane.surfaceType !== 'tool' || !pane.tool?.argv) continue; + const meta = leafMeta[pane.id]; + leafMeta[pane.id] = { ...meta, params: { ...meta.params, command: pane.command, toolArgv: pane.tool.argv } }; + } + lathLayout = { ...lathLayout, leafMeta }; + } // Tool commands remain runnable when geometry is corrupt. Rebuild their kind // and stable metadata from the pane projection instead of seeding plain shells. if (!lathLayout && visiblePanes.some(pane => pane.surfaceType === 'tool')) { @@ -54,12 +76,12 @@ export function restoreSession(platform: PlatformAdapter, sources: RestoreSource leafMeta: Object.fromEntries(recoverable.map(pane => [pane.id, pane.surfaceType === 'tool' ? { component: 'tool', tabComponent: 'tool', title: pane.title, params: { surfaceType: 'tool', command: pane.command, cwd: pane.cwd, + ...(pane.tool?.argv ? { toolArgv: pane.tool.argv } : {}), toolScope: pane.tool?.scope, toolName: pane.tool?.name, toolRender: pane.tool?.render ?? 'iframe', toolPort: pane.tool?.port ?? 'announced', toolKey: pane.tool?.key }, } : { component: 'terminal', tabComponent: 'terminal', title: pane.title }])), }; } - const shellOpts = getDefaultShellOpts(); // Host-owned and single-use, and read here rather than off the pane: the // session blob the webview saves must never carry one, or a later restore // would replay it (docs/specs/transport.md -> "Consuming it"). Restore-only — @@ -67,7 +89,7 @@ export function restoreSession(platform: PlatformAdapter, sources: RestoreSource // agent is still Live and has nothing to resume. const recoveryCommands = platform.getRecoveryCommands?.() ?? {}; - for (const pane of saved.panes) { + for (const pane of panes) { // Browser surfaces have no PTY or xterm; the persisted layout recreates them // (docs/specs/transport.md). Calling restoreTerminal here would mint a stray // PTY + xterm for the pane id that never gets mounted. diff --git a/lib/src/lib/session-save.test.ts b/lib/src/lib/session-save.test.ts index a6f1fa229..e2df85599 100644 --- a/lib/src/lib/session-save.test.ts +++ b/lib/src/lib/session-save.test.ts @@ -257,6 +257,18 @@ describe('saveSession', () => { }); }); + it('keeps resolved argv independently of the live shell command for panes and Doors', async () => { + const platform = createPlatform(null); + const toolArgv = ['program path', "it's.txt", '$(literal)']; + const params = { surfaceType: 'tool', command: "& 'program path' 'it''s.txt' '$(literal)'", toolArgv }; + await saveSession(platform, [{ id: 'visible', title: 'Viewer', surfaceType: 'tool', params }], [ + { id: 'hidden', title: 'Viewer', component: 'tool', params }, + ]); + const saved = vi.mocked(platform.saveState).mock.calls[0]![0] as PersistedSession; + expect(saved.panes.map(pane => pane.tool?.argv)).toEqual([toolArgv, toolArgv]); + expect(saved.panes.every(pane => pane.command === params.command)).toBe(true); + }); + it('persists neither a transcript nor a recovery command', async () => { // Both are absent by construction now: `PlatformAdapter` has no scrollback // reader, and the recovery command is host-owned and rides the boot payload diff --git a/lib/src/lib/session-save.ts b/lib/src/lib/session-save.ts index 77095e24c..a3b4fab46 100644 --- a/lib/src/lib/session-save.ts +++ b/lib/src/lib/session-save.ts @@ -1,6 +1,6 @@ import { normalizeAlertDeliveryOverrides, type AlertDeliveryOverrides } from './alert-delivery-model'; import type { PlatformAdapter } from './platform/types'; -import { browserPersistedPane, readPersistedSession, toPersistedAlertState, type PersistedDoor, type PersistedPane, type PersistedSession, type PersistedSurfaceRefs, type PersistedToolMetadata, type PersistedSurfaceType } from './session-types'; +import { browserPersistedPane, isToolCommandArgv, readPersistedSession, toPersistedAlertState, type PersistedDoor, type PersistedPane, type PersistedSession, type PersistedSurfaceRefs, type PersistedToolMetadata, type PersistedSurfaceType } from './session-types'; import { getActivity, getLivePersistedAlertState, getTerminalPaneState, isUntouched } from './terminal-registry'; import { UNNAMED_PANEL_TITLE } from './terminal-state'; @@ -187,7 +187,8 @@ function toolMetadataFromParams(params: Record | undefined): Pe const key = Array.isArray(params.toolKey) && params.toolKey.every((part) => typeof part === 'string') ? params.toolKey as string[] : undefined; - return { ...(name ? { name } : {}), ...(params.toolScope === 'user' ? { scope: 'user' as const } : {}), render, port, ...(key ? { key } : {}) }; + const argv = isToolCommandArgv(params.toolArgv) ? [...params.toolArgv] : undefined; + return { ...(argv ? { argv } : {}), ...(name ? { name } : {}), ...(params.toolScope === 'user' ? { scope: 'user' as const } : {}), render, port, ...(key ? { key } : {}) }; } function persistedVisiblePaneTitle(title: string): string { diff --git a/lib/src/lib/session-types.ts b/lib/src/lib/session-types.ts index da13f4406..1ed809d56 100644 --- a/lib/src/lib/session-types.ts +++ b/lib/src/lib/session-types.ts @@ -1,6 +1,7 @@ import { normalizeAlertDeliveryOverrides, type AlertDeliveryOverrides } from './alert-delivery-model'; import { isRecord } from './is-record'; import type { SessionStatus } from './alert-manager'; +import { hasShellInputControls } from 'dor/commands/shell-quote'; import { ACTIVITY_NOTIFICATION_SOURCES, type ActivityNotification, type TodoState } from './alert-manager'; /** Only TODO/detail restore; `status` is diagnostic and never resurrects a ring. */ @@ -17,6 +18,8 @@ export type PersistedSurfaceType = 'terminal' | 'browser' | 'tool'; * is respawned. Derived browser state (URL/session/port conflict) never enters * this projection. */ export interface PersistedToolMetadata { + /** Resolved arguments, re-quoted for the shell selected at cold restore. */ + argv?: string[]; scope?: 'user'; name?: string; render: 'iframe' | 'ab-screencast'; @@ -172,6 +175,7 @@ function isPersistedPaneShape(value: unknown): boolean { function isPersistedToolMetadataShape(value: unknown): boolean { if (!isRecord(value)) return false; return ( + (value.argv === undefined || isToolCommandArgv(value.argv)) && (value.name === undefined || typeof value.name === 'string') && (value.scope === undefined || value.scope === 'user') && (value.render === 'iframe' || value.render === 'ab-screencast') && @@ -180,6 +184,13 @@ function isPersistedToolMetadataShape(value: unknown): boolean { ); } +/** Typed into a terminal after quoting: controls cannot be made safe by quotes. */ +export function isToolCommandArgv(value: unknown): value is string[] { + return Array.isArray(value) && value.length > 0 + && value.every(arg => typeof arg === 'string' && !hasShellInputControls(arg)) + && value[0].trim().length > 0; +} + function isPersistedDoor(value: unknown): value is PersistedDoor { if (!isRecord(value)) return false; return ( From 7c515b73fd0c526b0689b70978cac4033eac1fc3 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 15 Sep 2026 19:03:25 -0700 Subject: [PATCH 08/10] Validate pending Tool grant state as a boolean --- docs/specs/dor-tool.md | 2 +- lib/src/components/wall/browser-surface.ts | 1 + lib/src/components/wall/tool-surface.test.ts | 9 +++++++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index a70411698..35acf2ee8 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -89,7 +89,7 @@ Source of truth: `queueToolSpawn` / the `surface.tool` handler in `lib/src/compo Reserved: **Must keep future implicit glob dispatch user-global and limited to user-global Tools**, and gate any future repo `prespawn_*` execution on the same approval; see scope **dor-tools** under [Future](#future). -Source of truth: `createToolHost` in `lib/src/host/tool-host.ts`; `FileToolTrustStore` / `lookupTool` in `lib/src/host/tool-trust.ts`; `resolveUpstreamUrl` in `lib/src/host/git-upstream.ts`; `ToolApproval` in `lib/src/components/wall/ToolApproval.tsx`; `resolveToolApproval` in `lib/src/components/Wall.tsx`. Tests: `lib/src/host/tool-trust.test.ts`, `lib/src/components/Wall.test.tsx`. +Source of truth: `createToolHost` in `lib/src/host/tool-host.ts`; `FileToolTrustStore` / `lookupTool` in `lib/src/host/tool-trust.ts`; `resolveUpstreamUrl` in `lib/src/host/git-upstream.ts`; `ToolApproval` in `lib/src/components/wall/ToolApproval.tsx`; `resolveToolApproval` in `lib/src/components/Wall.tsx`; `toolPendingFromParams` in `lib/src/components/wall/browser-surface.ts`. Tests: `lib/src/host/tool-trust.test.ts`, `lib/src/components/Wall.test.tsx`, `lib/src/components/wall/tool-surface.test.ts`. ## Serving diff --git a/lib/src/components/wall/browser-surface.ts b/lib/src/components/wall/browser-surface.ts index c9d516124..7d698dcef 100644 --- a/lib/src/components/wall/browser-surface.ts +++ b/lib/src/components/wall/browser-surface.ts @@ -90,6 +90,7 @@ export function toolPendingFromParams(params: unknown): ToolPending | null { if (pending.upstreamUrl !== null && typeof pending.upstreamUrl !== 'string') return null; if (pending.args !== undefined && !(Array.isArray(pending.args) && pending.args.every((arg) => typeof arg === 'string'))) return null; if (pending.error !== undefined && typeof pending.error !== 'string') return null; + if (pending.trustRecorded !== undefined && typeof pending.trustRecorded !== 'boolean') return null; return pending as unknown as ToolPending; } diff --git a/lib/src/components/wall/tool-surface.test.ts b/lib/src/components/wall/tool-surface.test.ts index f38e3f194..8622c45fb 100644 --- a/lib/src/components/wall/tool-surface.test.ts +++ b/lib/src/components/wall/tool-surface.test.ts @@ -202,6 +202,15 @@ describe('the pending-approval shape (regression: PR #493 review)', () => { it('allows a null upstream, which is how a repo with no remote arrives', () => { expect(toolPendingFromParams({ surfaceType: 'tool', toolPending: pending })).not.toBeNull(); }); + + it('accepts boolean grant state but rejects truthy non-boolean values', () => { + for (const trustRecorded of [false, true]) { + expect(toolPendingFromParams({ toolPending: { ...pending, trustRecorded } })?.trustRecorded).toBe(trustRecorded); + } + for (const trustRecorded of ['false', 'true', 1, {}]) { + expect(toolPendingFromParams({ toolPending: { ...pending, trustRecorded } })).toBeNull(); + } + }); }); describe('a pending tool is not persisted (regression: PR #493 review)', () => { From cd5162d5a6fc91620a892b893eda87dbcced3d4b Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 16 Sep 2026 16:38:27 -0700 Subject: [PATCH 09/10] Clarify user Tool misses and cross-file reuse warnings --- docs/specs/dor-tool.md | 4 ++-- lib/src/host/tool-host.test.ts | 8 ++++++++ lib/src/host/tool-host.ts | 2 +- lib/src/host/tool-registry.test.ts | 13 +++++++++++++ lib/src/host/tool-registry.ts | 3 +++ 5 files changed, 27 insertions(+), 3 deletions(-) diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index bd2a4afba..0d8b1590e 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -32,7 +32,7 @@ Source of truth: `surfaceKindFromParams` / `isToolParams` in `lib/src/components ## Declaring tools -**Must resolve a named Tool from the nearest ancestor `dormouse.yml`, then fall back to user Tools when that name is absent.** `--global` skips project discovery. A malformed project file fails lookup rather than falling back. The host owns discovery, bounded reads, YAML parsing, and substitutions; the renderer receives the resolved result. Canonical field shapes are `ToolEntry` in `lib/src/host/tool-registry.ts`. +**Must resolve a named Tool from the nearest ancestor `dormouse.yml`, then fall back to user Tools when that name is absent.** `--global` skips project discovery. Malformed project files fail lookup. If only the user file exists, an unknown name reports that file and its Tool names. The host owns discovery, bounded reads, YAML parsing, and substitutions; the renderer receives the resolved result. Canonical field shapes are `ToolEntry` in `lib/src/host/tool-registry.ts`. **Must read user Tools from `$XDG_CONFIG_HOME/dormouse/dormouse.yml` when that environment value is absolute, otherwise `~/.config/dormouse/dormouse.yml`.** Both local hosts use this location. User Tools require no project grant; malformed or unreadable user configuration fails lookup. Project and user Tools occupy separate reuse scopes. @@ -45,7 +45,7 @@ Source of truth: `surfaceKindFromParams` / `isToolParams` in `lib/src/components - **Must reject unknown `prespawn_*` fields and unknown substitutions**; unknown ordinary fields produce warnings. `$PROJECT_ROOT` is the declaring directory, `$CWD` the caller's resolved directory, and `$TARGET` the canonical local file input. (rationale) - **Must preserve scalar `prespawn_dedupe` as a one-element literal list**, never interpret it as a command to execute. Reserve separate fields for future computed keys. (rationale) -- **Must warn when a repo-local key omits `$PROJECT_ROOT`**, while allowing intentional cross-checkout dedupe. +- **Must warn when a repo-local key omits `$PROJECT_ROOT`, or a `$TARGET` run has a key without `$TARGET`.** Allow intentional cross-checkout or cross-file dedupe. - **Must reject `$PROJECT_ROOT` in user configuration**, which has no project root. **Must pass named-tool inputs as argument values, never substitute them into a shell-command string.** String `run` accepts no arguments and remains literal shell syntax. List `run` expands `$TARGET`, `$CWD`, and `$PROJECT_ROOT` within elements; a whole `$ARGS` element expands all input arguments. Without `$ARGS` or `$TARGET` in the list, append the inputs. **Must quote argv for the destination Session's shell**, using the current default only for new Sessions; takeover stores that quoted command for reruns. diff --git a/lib/src/host/tool-host.test.ts b/lib/src/host/tool-host.test.ts index 276eea91c..d0acce09e 100644 --- a/lib/src/host/tool-host.test.ts +++ b/lib/src/host/tool-host.test.ts @@ -112,6 +112,14 @@ describe('createToolHost', () => { } }); + it('names the user configuration and available tools after a project-file miss', async () => { + await rm(join(repo, 'dormouse.yml')); + const path = join(repo, 'user.yml'); + await writeFile(path, 'tools:\n viewer:\n run: viewer\n'); + expect(await createToolHost({ stateDir, userConfigPath: path }).handle({ op: 'lookup', name: 'viewr', cwd: repo })) + .toEqual({ status: 'unknown-tool', projectRoot: repo, path, names: ['viewer'] }); + }); + it('returns a parse error rather than throwing across the wire', async () => { await writeFile(join(repo, 'dormouse.yml'), 'tools:\n t:\n run: x\n prespawn_dedupe: [$NOPE]\n'); const result = await createToolHost({ stateDir }).handle({ op: 'lookup', name: 't', cwd: repo }); diff --git a/lib/src/host/tool-host.ts b/lib/src/host/tool-host.ts index dee8cf5c3..ffea1853c 100644 --- a/lib/src/host/tool-host.ts +++ b/lib/src/host/tool-host.ts @@ -90,7 +90,7 @@ export function createToolHost(options: { stateDir?: string; userConfigPath?: st const input = await resolveToolInput(entry, { projectRoot: null, cwd: request.cwd, args }); return okResult(entry, input, { projectRoot: file.dir, path, warnings: file.warnings, scope: 'user' }); } - if (project) return project; + if (project && (project.status !== 'no-file' || !file)) return project; return { status: 'unknown-tool', projectRoot: dirname(path), path, names: [...(file?.tools.keys() ?? [])].sort() }; } catch (error) { return { status: 'error', message: error instanceof Error ? error.message : String(error) }; diff --git a/lib/src/host/tool-registry.test.ts b/lib/src/host/tool-registry.test.ts index ec695c40f..89033f504 100644 --- a/lib/src/host/tool-registry.test.ts +++ b/lib/src/host/tool-registry.test.ts @@ -105,6 +105,19 @@ tools: expect(parse('tools:\n t:\n run: x\n prespawn_dedupe: [t]\n', USER).warnings).toEqual([]); }); + it('warns when different target files would reuse one Tool', () => { + const file = parse('tools:\n viewer:\n run: [viewer, --file=$TARGET]\n prespawn_dedupe: [viewer]\n', USER); + expect(file.warnings).toEqual([expect.stringContaining('no $TARGET')]); + expect(file.tools.get('viewer')?.dedupeTemplate).toEqual(['viewer']); + }); + + it.each([ + ' prespawn_dedupe: [viewer, file=$TARGET]\n', + '', + ])('does not warn for target-aware reuse or a fresh Tool: %s', (dedupe) => { + expect(parse('tools:\n viewer:\n run: [viewer, $TARGET]\n' + dedupe, USER).warnings).toEqual([]); + }); + it('requires a non-empty run', () => { expect(() => parse('tools:\n t:\n prespawn_dedupe: [t]\n')).toThrow(/'run' is required/); expect(() => parse('tools:\n t:\n run: " "\n')).toThrow(/'run' is required/); diff --git a/lib/src/host/tool-registry.ts b/lib/src/host/tool-registry.ts index 500eaa20a..069f3eaba 100644 --- a/lib/src/host/tool-registry.ts +++ b/lib/src/host/tool-registry.ts @@ -167,6 +167,9 @@ export function parseToolFile( if (typeof run === 'string' && usesTarget(dedupeTemplate)) { throw new ToolFileError(`${where}: $TARGET in prespawn_dedupe requires an argument-list run`); } + if (Array.isArray(run) && usesTarget(run) && !usesTarget(dedupeTemplate)) { + warnings.push(`${where}: prespawn_dedupe has no $TARGET, so different files reuse the first file's Tool`); + } // A repo-local key with no project scope dedupes across every checkout // that declares the name, so a second worktree's tool would reveal the // first instead of starting. Warn, not error: a repo-declared From 07947c1d8edc9fc87b7617b67ab8d1d005cb2bee Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 16 Sep 2026 16:46:57 -0700 Subject: [PATCH 10/10] Exercise saved-permission retry after stack integration --- lib/src/components/Wall.test.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/src/components/Wall.test.tsx b/lib/src/components/Wall.test.tsx index 17d3f4bf0..4faa9bb10 100644 --- a/lib/src/components/Wall.test.tsx +++ b/lib/src/components/Wall.test.tsx @@ -1834,10 +1834,11 @@ describe('Wall on the Lath engine', () => { const retry = Promise.withResolvers(); toolControl.mockImplementation(async request => request.op === 'trust' ? { status: 'trust-recorded' } : retry.promise); - await act(async () => allow.click()); + await act(async () => [...container.querySelectorAll('button')].find(button => button.textContent === 'Retry')!.click()); expect(container.querySelector('[role="alert"]')).toBeNull(); expect(toolControl.mock.calls.filter(([request]) => request.op === 'lookup')).toHaveLength(3); - const decline = [...container.querySelectorAll('button')].find(button => button.textContent === 'Disallow and close')!; + expect(toolControl.mock.calls.filter(([request]) => request.op === 'trust')).toHaveLength(1); + const decline = [...container.querySelectorAll('button')].find(button => button.textContent === 'Close')!; await act(async () => decline.click()); await flush(); await act(async () => retry.resolve(failed));