diff --git a/specs/spec03/03-03-guide-harness.md b/specs/spec03/03-03-guide-harness.md index 18a35d4..22bae03 100644 --- a/specs/spec03/03-03-guide-harness.md +++ b/specs/spec03/03-03-guide-harness.md @@ -53,6 +53,7 @@ ## 3. Design - **Isolation preserved** (03-01 §2.1): `setting_sources=[]`, `strict_mcp_config=True`, no user skills/MCP. **But built-in tools are ENABLED** and allowlisted (`_GUIDE_BUILTINS` = Bash/Read/Write/Edit/Glob/Grep) — the bounded surface a repair task needs (contrast: find-music runs with `tools=[]`). This is the per-task tool-surface principle: each capability gets exactly what it needs. - **Flow**: startup / first music use → deterministic preflight → if broken, murmur tells the user plainly and offers the guide → on opt-in, `SetupGuide.fix_music` runs → Claude Code investigates (Bash), **asks before each change** (SDK `default` mode, routed to the CLI Host), applies the smallest safe fix, verifies → returns an explanation. + **Amended (2026-08-12, user decision — consent lands on the change, not the look)**: the offer's `y` already covers *investigation*, so `cliPermission` auto-allows pure reads instead of re-asking — the read-only builtins (Read/Glob/Grep) and Bash commands every segment of which matches a conservative read-only allowlist (`isReadOnlyCommand`, `src/guide.ts`: which/ls/echo/version-reads/brew info-list/…; redirects, backticks, parameter expansion (`$VAR` — only `$?` and an allowlisted `$(...)` pass), or one unknown head disqualify the whole command, falling back to the ask; `brew outdated` is excluded because default Homebrew auto-updates before answering it). Secret-bearing targets (`voice.json`, `.env*`) are never auto-allowed for any tool — an unconsented read would put a credential into the SDK transcript §7.2 keeps it out of. Anything that can mutate — installs, upgrades, Write/Edit, any unrecognized command — still gets the per-action y/N. `bypassPermissions` remains forbidden (the red line stands); auto-allows are recorded in the dev log. - **Off the live broadcast loop** (master §3.2 boundary ②): setup/repair is a foreground interaction (first-run, radio not yet broadcasting) or a background job — its exact relationship to the broadcast loop is an open question. - **Model**: Opus (repair is judgment-heavy and occasional; the token cost amortizes). diff --git a/src/guide.ts b/src/guide.ts index 6d30c94..b1d638e 100644 --- a/src/guide.ts +++ b/src/guide.ts @@ -99,11 +99,77 @@ export function lineReader(host: Host, quit?: QuitLatch): ReadLine { } } +// Builtins that can only look, never touch. Write/Edit stay out on purpose. +const READONLY_TOOLS = new Set(['Read', 'Glob', 'Grep']) + +// Where credentials live (spec 03-03 §7.2: voice.json holds the api key, +// .env* the remote-voice creds). A "read" of these puts a secret into the SDK +// transcript, so it is never auto-allowed, whatever the tool (codex review). +const SECRET_BEARING = /\.env|voice\.json/i + +// The read-only shapes the guide's diagnosis actually uses. Whole-segment +// anchors rather than command heads where the head alone is not enough: +// `yt-dlp --version` reads, `yt-dlp ` downloads. +const READONLY_SEGMENT = [ + /^which(\s|$)/, + /^type\s/, + /^command -v\s/, + /^ls(\s|$)/, + /^echo(\s|$)/, + /^printf\s/, + /^pwd$/, + /^uname(\s|$)/, + /^head(\s|$)/, + /^tail(\s|$)/, + /^wc(\s|$)/, + /^yt-dlp --version$/, + /^ffmpeg -version$/, + /^bun --version$/, + /^node --version$/, + // `brew outdated` is NOT here: default Homebrew auto-updates its own + // metadata before answering it, which mutates state (codex review). + /^brew (info|list|config|doctor|--version|--prefix)(\s|$)/, + /^uv (--version$|tool list)/, + /^pipx (list|--version)/, +] + +// A conservative classifier: is this Bash command incapable of changing the +// machine? Redirects and backticks disqualify outright; the rest is split at +// every separator (;, &&, ||, |, &, newline) AND around $(...) so each +// executable segment is judged on its own — one unknown head poisons the +// whole command. False negatives just fall back to asking; a false positive +// would execute silently, so every rule leans strict. +export function isReadOnlyCommand(command: string): boolean { + if (/[><`]/.test(command)) return false + // Parameter/env expansion can surface a credential into the transcript + // (`echo $MURMUR_TTS_API_KEY`), so `$` is allowed only as `$?` or a `$(...)` + // whose inner command is judged like any other segment (codex review). + if (/\$(?![?(])/.test(command)) return false + if (SECRET_BEARING.test(command)) return false + const segments = command + .split(/\|\||&&|[;&|\n]|\$\(|\)/) + .map((piece) => piece.trim().replace(/^["']+|["']+$/g, '').trim()) + .filter((piece) => piece !== '') + if (segments.length === 0) return false + return segments.every((segment) => READONLY_SEGMENT.some((rule) => rule.test(segment))) +} + // Ask the user via the CLI Host before each tool the guide wants to run, and // return the SDK's allow/deny result. Anything but an explicit yes denies. +// One carve-out (spec 03-03 §7.1): the card's 'y' already covered LOOKING, so +// pure reads flow without another ask and the per-action consent lands on the +// CHANGES — a wall of y/N for `which` and `--version` buries the one confirm +// that matters. The dev log keeps a record of what was auto-allowed. export function cliPermission(host: Host, read: ReadLine): CanUseTool { return async (toolName, input) => { const detail = typeof input.command === 'string' ? input.command : JSON.stringify(input) + if ( + (READONLY_TOOLS.has(toolName) && !SECRET_BEARING.test(JSON.stringify(input))) || + (toolName === 'Bash' && typeof input.command === 'string' && isReadOnlyCommand(input.command)) + ) { + host.debug?.(`guide auto-allowed read-only [${toolName}]: ${detail}`) + return { behavior: 'allow' } + } // One self-contained ask: a docked "allow?" with the command left behind // in the log would ask the user to approve something they cannot see. ask(host, `setup assistant wants to run [${toolName}]: ${detail}\nallow? [y/N]`, 'consent') diff --git a/test/guide.test.ts b/test/guide.test.ts index a326bce..5f94b0f 100644 --- a/test/guide.test.ts +++ b/test/guide.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it } from 'vitest' -import { cliConversation, cliPermission, lineReader, quitLatch } from '../src/guide.ts' +import { + cliConversation, + cliPermission, + isReadOnlyCommand, + lineReader, + quitLatch, +} from '../src/guide.ts' import type { AskKind, Host } from '../src/host.ts' // A host with scripted keyboard lines (the same stdin the Director uses). @@ -104,6 +110,106 @@ describe('cliPermission (spec 03-03 §2 — route the ask, never own the semanti }) }) +// The card's 'y' already covered LOOKING (the user asked to be walked through +// a fix); what still needs per-action consent is CHANGE. Pure investigation +// flows without another ask — a wall of y/N for `which` and `--version` reads +// as noise and buries the one confirm that matters. +describe('cliPermission — read-only investigation flows without asking (spec 03-03 §7.1)', () => { + it('auto-allows the diagnostics the guide actually runs, without touching the keyboard', async () => { + // No scripted lines: if any of these asked, the read would hang the test. + const { host, asks } = fakeHost([], { docked: true }) + const ask = cliPermission(host, lineReader(host)) + for (const command of [ + 'which -a yt-dlp; echo "---"; yt-dlp --version; echo "---"; ls -l "$(which yt-dlp)"', + 'echo "exit: $?"; echo "---"; brew info yt-dlp | head -5', + 'uv tool list && pipx list', + 'ffmpeg -version', + 'brew --prefix && brew list --versions yt-dlp', + ]) { + expect(await ask('Bash', { command }, askOptions)).toEqual({ behavior: 'allow' }) + } + expect(asks).toEqual([]) + }) + + it('secret-bearing reads stay behind consent, whatever the tool (codex review)', async () => { + // The out-of-band secret flow (§7.2) exists so credentials never enter the + // SDK transcript; an auto-allowed read of the config or the env would put + // them there without anyone agreeing to it. + const { host, asks } = fakeHost(['', '', '', ''], { docked: true }) + const ask = cliPermission(host, lineReader(host)) + for (const [tool, input] of [ + ['Bash', { command: 'echo $MURMUR_TTS_API_KEY' }], + ['Read', { file_path: '/Users/zach/.murmur/voice.json' }], + ['Read', { file_path: '/Users/zach/.personal/murmur/.env' }], + ['Grep', { pattern: 'apiKey', path: '/Users/zach/.murmur/voice.json' }], + ] as const) { + expect(await ask(tool, input, askOptions)).toMatchObject({ behavior: 'deny' }) + } + expect(asks).toHaveLength(4) + }) + + it('read-only builtins pass without a question; Write and Edit still ask', async () => { + const { host, asks } = fakeHost(['n'], { docked: true }) + const ask = cliPermission(host, lineReader(host)) + expect(await ask('Read', { file_path: '/tmp/x' }, askOptions)).toEqual({ behavior: 'allow' }) + expect(await ask('Glob', { pattern: '**/*.ts' }, askOptions)).toEqual({ behavior: 'allow' }) + expect(await ask('Grep', { pattern: 'x' }, askOptions)).toEqual({ behavior: 'allow' }) + expect(asks).toEqual([]) + expect(await ask('Write', { file_path: '/tmp/x' }, askOptions)).toMatchObject({ + behavior: 'deny', + }) + expect(asks).toHaveLength(1) + }) + + it('anything that can mutate still asks: the consent stays on the change', async () => { + const { host, infos } = fakeHost(['y', '']) + const ask = cliPermission(host, lineReader(host)) + expect(await ask('Bash', { command: 'brew upgrade yt-dlp' }, askOptions)).toEqual({ + behavior: 'allow', + }) + expect(await ask('Bash', { command: 'brew install ffmpeg' }, askOptions)).toMatchObject({ + behavior: 'deny', + }) + expect(infos.join('\n')).toContain('brew upgrade yt-dlp') + }) +}) + +describe('isReadOnlyCommand — the conservative classifier', () => { + it('refuses unknown heads, redirects, chained mutations, and mutating substitutions', () => { + for (const command of [ + 'brew upgrade yt-dlp', + 'rm -rf /tmp/x', + 'echo hi > /tmp/x', // a redirect writes + 'ls $(curl example.com)', // unknown head inside $() + 'which yt-dlp && brew upgrade yt-dlp', // one bad segment poisons the chain + 'ls & rm -rf /tmp/x', // a single & backgrounds; the second head must be seen + 'yt-dlp https://example.com', // beyond --version, yt-dlp downloads + 'echo `rm -rf /tmp/x`', // backticks are substitution too + '', // nothing is not a read + 'brew outdated yt-dlp', // brew auto-updates its own metadata first (codex review) + 'echo $MURMUR_TTS_API_KEY', // env expansion can surface a credential (codex review) + 'ls "$HOME/.murmur"', // same: parameter expansion is not provably a read + 'cat voice.json', // the secret-bearing config never auto-reads + ]) { + expect(isReadOnlyCommand(command), command).toBe(false) + } + }) + + it('accepts version reads and package queries', () => { + for (const command of [ + 'yt-dlp --version', + 'ffmpeg -version', + 'brew list', + 'brew --prefix', + 'pwd', + 'uname -a', + 'command -v yt-dlp', + ]) { + expect(isReadOnlyCommand(command), command).toBe(true) + } + }) +}) + describe('cliConversation', () => { it('returns the typed reply; empty or /done or q ends it', async () => { const { host } = fakeHost([' the quick fix please ', '', '/done', 'Q'])