From 04b13f99ee4dfe57a4e5c6e58009648fb730ae09 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Fri, 23 Oct 2026 08:41:11 -0700 Subject: [PATCH 01/17] test(claude): pin the CLI's permission suggestions and plan approvals (D0) A real-CLI contract over the production sidecar, the pinned SDK 0.3.237 and its CLI 2.1.237 against the local provider: the suggestion shapes for Bash, Write, a Read outside the workspace, a compound command and ls /; that a localSettings rule sent back as updatedPermissions writes .claude/settings.local.json and stops the next ask for that command only; that a session rule does the same with no file; that ExitPlanMode carries the plan file's text and path when the model wrote one; and that a plain, acceptEdits or bypassPermissions plan approval leaves plan mode for that mode, stated once by the sidecar, with the next Write asking or not to match. Co-Authored-By: Claude Opus 5.5 --- .../providers/claude-local-provider.mjs | 2 + .../providers/claude-permission-contract.mjs | 237 ++++++++++++++++++ .../claude-permission-contract.test.ts | 163 ++++++++++++ 3 files changed, 402 insertions(+) create mode 100644 test/integration/providers/claude-permission-contract.mjs create mode 100644 test/integration/providers/claude-permission-contract.test.ts diff --git a/test/integration/providers/claude-local-provider.mjs b/test/integration/providers/claude-local-provider.mjs index 73290be35..4a786e212 100644 --- a/test/integration/providers/claude-local-provider.mjs +++ b/test/integration/providers/claude-local-provider.mjs @@ -14,6 +14,8 @@ // committed fixtures' paths depend on) // - claude-live-frames.mjs (the real sidecar's live frames; it brings its // own model through the `respond` hook) +// - claude-permission-contract.mjs (permission suggestions and plan +// approvals through the real sidecar; its own model, the `respond` hook) // // The model's behavior is chosen by prompt markers (see // test/fixtures/claude-cli-sim/prompt-markers.mjs), the same language the CLI diff --git a/test/integration/providers/claude-permission-contract.mjs b/test/integration/providers/claude-permission-contract.mjs new file mode 100644 index 000000000..6348fcca1 --- /dev/null +++ b/test/integration/providers/claude-permission-contract.mjs @@ -0,0 +1,237 @@ +// Contract harness (client-alerts plan PR-C, D0): what the pinned CLI 2.1.237 +// does with permission suggestions and plan approvals, as the REAL Claude +// sidecar (crates/freshell-claude-sidecar/index.mjs) relays it. The sidecar +// runs over the pinned SDK and its bundled native CLI (resolvePinnedClaude) in +// a throwaway fixture (createClaudeFixture), against the local provider +// endpoint (startLocalProvider; the real Anthropic API is never contacted). +// +// The approval card, the plan card and the pane's permission mode rely on +// these facts; each scenario runs in its own fixture and sidecar: +// +// remember-local Bash `npm test` in default mode: allowed with the CLI's +// own suggestions as updatedPermissions (localSettings); +// then `npm test` again, then `ls /` +// remember-session the same, with every suggestion's destination rewritten +// to `session` +// shapes the suggestions offered for a Write in the workspace, a +// Read outside it, a compound Bash command and `ls /` +// (every request denied) +// plan-default plan mode, ExitPlanMode allowed plainly; then a Write +// plan-accept-edits plan mode, ExitPlanMode allowed with setMode acceptEdits; +// then a Write +// plan-bypass plan mode, ExitPlanMode allowed with setMode +// bypassPermissions; then a Write +// plan-file plan mode: the model writes its plan file, then calls +// ExitPlanMode with no input +// plan-empty plan mode: ExitPlanMode with no input and no plan file +// +// Usage: node claude-permission-contract.mjs . Prints ONE JSON line +// (the receipt), with every fixture path replaced by ``; structured +// progress and failures go to stderr as JSONL. +import fs from 'node:fs/promises' +import path from 'node:path' +import { spawn } from 'node:child_process' +import { createInterface } from 'node:readline' +import { randomUUID } from 'node:crypto' +import { createClaudeFixture, FAKE_MODEL, resolvePinnedClaude, startLocalProvider } from './claude-local-provider.mjs' + +const repoRoot = process.argv[2] +if (!repoRoot) throw new Error('repository root argument is required') + +const TURN_TIMEOUT_MS = 45_000 +export const PLAN = '# Plan\n\n1. Edit foo.ts\n2. Run the tests' +export const PLAN_FILE_TEXT = '# Real plan\n\n- step A\n- step B' + +const log = (severity, event, fields = {}) => { + process.stderr.write(`${JSON.stringify({ ts: new Date().toISOString(), severity, component: 'claude-permission-contract', event, ...fields })}\n`) +} +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)) + +const pinned = await resolvePinnedClaude(repoRoot) + +const bash = (command) => ({ name: 'Bash', input: { command, description: 'contract command' } }) +const write = (filePath) => ({ name: 'Write', input: { file_path: filePath, content: 'written\n' } }) +const allowWith = (updatedPermissions) => (updatedPermissions ? { behavior: 'allow', updatedPermissions } : { behavior: 'allow' }) +const DENY = { behavior: 'deny', message: 'denied by the contract', interrupt: false } + +/** + * Each scenario: the boot mode, whether it switches to plan mode first, its + * turns (the model's tool calls for each prompt; `ctx` has the fixture paths + * and the plan file path the CLI named) and the answer to each permission + * request. + */ +const SCENARIOS = { + 'remember-local': { + turns: [() => [bash('npm test')], () => [bash('npm test')], () => [bash('ls /')]], + decide: (request) => (request.tool.input.command === 'npm test' ? allowWith(request.suggestions) : DENY), + }, + 'remember-session': { + turns: [() => [bash('npm test')], () => [bash('npm test')], () => [bash('ls /')]], + decide: (request) => (request.tool.input.command === 'npm test' + ? allowWith(request.suggestions.map((update) => ({ ...update, destination: 'session' }))) + : DENY), + }, + shapes: { + turns: [ + (ctx) => [write(path.join(ctx.cwd, 'foo.txt'))], + (ctx) => [{ name: 'Read', input: { file_path: ctx.outsideFile } }], + (ctx) => [bash(`cat ${ctx.outsideFile} && git status`)], + () => [bash('ls /')], + ], + decide: () => DENY, + }, + 'plan-default': { + plan: true, + turns: [() => [{ name: 'ExitPlanMode', input: { plan: PLAN } }], (ctx) => [write(path.join(ctx.cwd, 'after.txt'))]], + decide: (request) => (request.tool.name === 'ExitPlanMode' ? allowWith() : DENY), + }, + 'plan-accept-edits': { + plan: true, + turns: [() => [{ name: 'ExitPlanMode', input: { plan: PLAN } }], (ctx) => [write(path.join(ctx.cwd, 'after.txt'))]], + decide: (request) => (request.tool.name === 'ExitPlanMode' + ? allowWith([{ type: 'setMode', mode: 'acceptEdits', destination: 'session' }]) + : DENY), + }, + 'plan-bypass': { + plan: true, + turns: [() => [{ name: 'ExitPlanMode', input: { plan: PLAN } }], (ctx) => [write(path.join(ctx.cwd, 'after.txt'))]], + decide: (request) => (request.tool.name === 'ExitPlanMode' + ? allowWith([{ type: 'setMode', mode: 'bypassPermissions', destination: 'session' }]) + : DENY), + }, + 'plan-file': { + plan: true, + turns: [(ctx) => [{ name: 'Write', input: { file_path: ctx.planPath, content: PLAN_FILE_TEXT } }, { name: 'ExitPlanMode', input: {} }]], + decide: () => DENY, + }, + 'plan-empty': { + plan: true, + turns: [() => [{ name: 'ExitPlanMode', input: {} }]], + decide: () => DENY, + }, +} + +/** A frame as the receipt keeps it: the big catalogs (commands, tools) by size only. */ +function slim(frame) { + const out = { ...frame } + if (Array.isArray(out.commands)) out.commands = out.commands.length + if (Array.isArray(out.tools)) out.tools = out.tools.length + return out +} + +async function readJson(file) { + try { + return JSON.parse(await fs.readFile(file, 'utf8')) + } catch (error) { + if (error?.code === 'ENOENT') return null + throw error + } +} + +async function runScenario(name, spec) { + const fixture = await createClaudeFixture({ prefix: `freshell-claude-perm-${name}-` }) + const outside = path.join(fixture.root, 'outside') + await fs.mkdir(outside, { recursive: true }) + const ctx = { cwd: fixture.cwd, outsideFile: path.join(outside, 'notes.txt'), planPath: null } + await fs.writeFile(ctx.outsideFile, 'notes\n') + + // The model: the current turn's tool calls in order, then a short reply. + const state = { turn: -1, step: 0 } + const respond = ({ messages, tools }) => { + // A side request with no tools (a title, a summary): the markers answer it. + if (tools.length === 0 || state.turn < 0) return undefined + const named = JSON.stringify(messages).match(/\/[^\s"\\]*\/plans\/[^\s"\\]+\.md/) + if (named && !ctx.planPath) ctx.planPath = named[0] + const calls = spec.turns[state.turn](ctx) + if (state.step < calls.length) { + const call = calls[state.step] + state.step += 1 + return { blocks: [{ type: 'tool_use', name: call.name, input: call.input }] } + } + return { blocks: [{ type: 'text', text: 'done' }] } + } + + const frames = [] + const answered = [] + let provider + let sidecar + let sidecarExit + let failure + let settingsLocal = null + const written = {} + try { + provider = await startLocalProvider({ respond }) + sidecar = spawn(process.execPath, [path.join(repoRoot, 'crates/freshell-claude-sidecar/index.mjs')], { + cwd: path.join(repoRoot, 'crates/freshell-claude-sidecar'), + env: fixture.env(provider.url, { CLAUDE_CMD: pinned.nativeCli, FRESHELL_CLAUDE_ONECLI_API_KEY: '1', npm_config_update_notifier: 'false' }), + stdio: ['pipe', 'pipe', 'pipe'], + }) + sidecar.stdin.on('error', () => {}) + const exited = new Promise((resolve) => sidecar.once('exit', (code, signal) => { sidecarExit = { code, signal }; resolve() })) + createInterface({ input: sidecar.stderr }).on('line', () => {}) + const send = (message) => sidecar.stdin.write(`${JSON.stringify(message)}\n`) + const waiters = [] + let sessionId + createInterface({ input: sidecar.stdout }).on('line', (line) => { + let frame + try { frame = JSON.parse(line) } catch { frame = { type: '', line } } + frames.push(frame) + if (frame.type === 'sdk.permission.request') { + const decision = spec.decide(frame) + answered.push({ tool: frame.tool.name, decision }) + send({ type: 'permission.respond', sessionId, requestId: frame.requestId, decision }) + } + for (const waiter of [...waiters]) if (waiter.match(frame)) { waiters.splice(waiters.indexOf(waiter), 1); waiter.resolve(frame) } + }) + const waitFor = (match, what) => new Promise((resolve, reject) => { + const hit = frames.find(match) + if (hit) return resolve(hit) + const timer = setTimeout(() => reject(new Error(`${name}: timed out waiting for ${what}`)), TURN_TIMEOUT_MS) + waiters.push({ match, resolve: (frame) => { clearTimeout(timer); resolve(frame) } }) + }) + + const requestId = randomUUID() + send({ type: 'create', protocol: 2, boot: { kind: 'fresh' }, requestId, cwd: fixture.cwd, model: FAKE_MODEL, permissionMode: 'default' }) + const created = await waitFor((frame) => frame.requestId === requestId && (frame.type === 'created' || frame.type === 'create.failed'), 'created') + if (created.type !== 'created') throw new Error(`the sidecar refused the create: ${created.message}`) + sessionId = created.sessionId + if (spec.plan) { + send({ type: 'configure', sessionId, requestId: 'to-plan', settings: { permissionMode: 'plan' } }) + const answer = await waitFor((frame) => frame.type === 'sdk.configured' && frame.requestId === 'to-plan', 'the plan-mode configure') + if (answer.ok !== true) throw new Error(`configure to plan failed: ${answer.message}`) + } + for (let turn = 0; turn < spec.turns.length; turn += 1) { + state.turn = turn + state.step = 0 + const results = frames.filter((frame) => frame.type === 'sdk.result').length + log('info', 'turn.start', { scenario: name, turn }) + send({ type: 'send', sessionId, text: `contract turn ${turn + 1}` }) + await waitFor(() => frames.filter((frame) => frame.type === 'sdk.result').length > results, `the result of turn ${turn + 1}`) + } + // A status the CLI reports after a result still reaches the sidecar. + await sleep(300) + settingsLocal = await readJson(path.join(fixture.cwd, '.claude', 'settings.local.json')) + for (const file of ['foo.txt', 'after.txt']) { + written[file] = (await fs.stat(path.join(fixture.cwd, file)).catch(() => null)) !== null + } + send({ type: 'shutdown' }) + sidecar.stdin.end() + await Promise.race([exited, sleep(10_000)]) + } catch (error) { + failure = String(error?.message ?? error).slice(0, 1000) + log('error', 'scenario.failed', { scenario: name, error: failure }) + } finally { + if (sidecar && !sidecarExit) sidecar.kill('SIGKILL') + if (provider) await provider.close() + await fixture.cleanup() + } + const fixtureRemoved = !(await fs.stat(fixture.root).catch(() => null)) + const relabel = (value) => JSON.parse(JSON.stringify(value).replaceAll(fixture.root, '')) + return relabel({ failure, fixtureRemoved, sidecarExit, frames: frames.map(slim), answered, settingsLocal, written }) +} + +const entries = await Promise.all(Object.entries(SCENARIOS).map(async ([name, spec]) => [name, await runScenario(name, spec)])) +const scenarios = Object.fromEntries(entries) +const failed = entries.some(([, result]) => result.failure || !result.fixtureRemoved) +const receipt = { sdkVersion: pinned.sdkVersion, bundledCliVersion: pinned.cliVersion, plan: PLAN, planFileText: PLAN_FILE_TEXT, scenarios } +process.stdout.write(`${JSON.stringify(receipt)}\n`, () => process.exit(failed ? 1 : 0)) diff --git a/test/integration/providers/claude-permission-contract.test.ts b/test/integration/providers/claude-permission-contract.test.ts new file mode 100644 index 000000000..2ef6fedf7 --- /dev/null +++ b/test/integration/providers/claude-permission-contract.test.ts @@ -0,0 +1,163 @@ +// @vitest-environment node +// +// Real-CLI contract for Claude's permission suggestions and plan approvals +// (client-alerts plan PR-C, D0): the production sidecar +// (crates/freshell-claude-sidecar/index.mjs) over the pinned SDK 0.3.237 and +// its bundled CLI 2.1.237, against the local provider endpoint (never the real +// API). claude-permission-contract.mjs runs each scenario in its own fixture +// and reports every stdout frame, the decisions it sent, and what the CLI left +// on disk. +// +// The approval card ("Allow and don't ask again"), the plan card and the +// pane's permission mode rely on these facts. This is a characterization of +// an external dependency: a pinned-CLI bump that changes any of them fails +// here first. When the pinned CLI is missing the harness fails +// (PinnedClaudeMissingError): nothing here is ever skipped. + +import { spawn, type ChildProcess } from 'node:child_process' +import path from 'node:path' +import { fileURLToPath } from 'node:url' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { PINNED_CLI_VERSION, PINNED_SDK_VERSION } from './claude-local-provider.mjs' + +const __dirname = path.dirname(fileURLToPath(import.meta.url)) +const REPO_ROOT = path.resolve(__dirname, '../../..') +const HARNESS = path.join(__dirname, 'claude-permission-contract.mjs') + +type Frame = Record +interface ScenarioReceipt { + failure?: string + fixtureRemoved: boolean + sidecarExit?: { code: number | null; signal: string | null } + frames: Frame[] + answered: Array<{ tool: string; decision: Frame }> + settingsLocal: unknown + written: Record +} +interface Receipt { + sdkVersion: string + bundledCliVersion: string + plan: string + planFileText: string + scenarios: Record +} + +let child: ChildProcess | undefined +let receipt: Receipt +let stderr = '' + +beforeAll(async () => { + child = spawn(process.execPath, [HARNESS, REPO_ROOT], { + cwd: REPO_ROOT, + env: { PATH: process.env.PATH ?? '/usr/bin:/bin', LANG: 'C.UTF-8' }, + stdio: ['ignore', 'pipe', 'pipe'], + }) + let stdout = '' + child.stdout!.setEncoding('utf8').on('data', (chunk: string) => { stdout += chunk }) + child.stderr!.setEncoding('utf8').on('data', (chunk: string) => { stderr += chunk }) + await new Promise((resolve, reject) => { + const timer = setTimeout(() => { + child?.kill('SIGTERM') + reject(new Error(`the permission contract harness timed out; stderr=${stderr.slice(-2000)}`)) + }, 140_000) + timer.unref() + child!.once('error', (error) => { clearTimeout(timer); reject(error) }) + child!.once('exit', () => { clearTimeout(timer); resolve() }) + }) + const lines = stdout.trim().split('\n').filter(Boolean) + if (lines.length !== 1) throw new Error(`expected one receipt line; stdout=${stdout.slice(-1000)} stderr=${stderr.slice(-2000)}`) + receipt = JSON.parse(lines[0]!) +}, 150_000) + +afterAll(() => { + if (child && child.exitCode === null && child.signalCode === null) child.kill('SIGKILL') +}) + +const scenario = (name: string): ScenarioReceipt => { + const recorded = receipt.scenarios[name] + expect(recorded, `scenario ${name} ran`).toBeDefined() + expect(recorded!.failure, `${name}: ${stderr.slice(-2000)}`).toBeUndefined() + return recorded! +} +const ofType = (list: Frame[], type: string) => list.filter((frame) => frame.type === type) +const requests = (name: string) => ofType(scenario(name).frames, 'sdk.permission.request') +const modes = (name: string) => ofType(scenario(name).frames, 'sdk.session.metadata').map((frame) => frame.permissionMode) +const NPM_TEST_RULE = { type: 'addRules', rules: [{ toolName: 'Bash', ruleContent: 'npm test *' }], behavior: 'allow', destination: 'localSettings' } +const OUTSIDE_READ_RULE = { type: 'addRules', rules: [{ toolName: 'Read', ruleContent: '//outside/**' }], behavior: 'allow', destination: 'session' } +const ACCEPT_EDITS = { type: 'setMode', mode: 'acceptEdits', destination: 'session' } + +describe(`Claude permission suggestions and plan approvals over the pinned CLI ${PINNED_CLI_VERSION} and a local provider`, () => { + it('runs every scenario on the pinned SDK and CLI, and each sidecar then exits cleanly', () => { + expect(receipt).toMatchObject({ sdkVersion: PINNED_SDK_VERSION, bundledCliVersion: PINNED_CLI_VERSION }) + expect(Object.keys(receipt.scenarios)).toEqual([ + 'remember-local', 'remember-session', 'shapes', 'plan-default', 'plan-accept-edits', 'plan-bypass', 'plan-file', 'plan-empty', + ]) + for (const name of Object.keys(receipt.scenarios)) { + expect(scenario(name), name).toMatchObject({ fixtureRemoved: true, sidecarExit: { code: 0, signal: null } }) + expect(ofType(scenario(name).frames, 'sdk.error'), name).toEqual([]) + } + }) + + it('a project rule the CLI suggests, sent back as updatedPermissions, is saved to .claude/settings.local.json and stops the next ask for that command only', () => { + const asked = requests('remember-local') + expect(asked[0]).toMatchObject({ tool: { name: 'Bash', input: { command: 'npm test' } }, suggestions: [NPM_TEST_RULE] }) + expect(scenario('remember-local').answered[0]!.decision).toEqual({ behavior: 'allow', updatedPermissions: [NPM_TEST_RULE] }) + // The second `npm test` runs without asking; a different command still asks. + expect(asked.map((request) => request.tool.input.command)).toEqual(['npm test', 'ls /']) + expect(ofType(scenario('remember-local').frames, 'sdk.result')).toHaveLength(3) + expect(scenario('remember-local').settingsLocal).toEqual({ permissions: { allow: ['Bash(npm test *)'] } }) + }) + + it('the same rule for this conversation only (destination session) stops the next ask and writes no file', () => { + expect(requests('remember-session').map((request) => request.tool.input.command)).toEqual(['npm test', 'ls /']) + expect(scenario('remember-session').answered[0]!.decision.updatedPermissions).toEqual([{ ...NPM_TEST_RULE, destination: 'session' }]) + expect(scenario('remember-session').settingsLocal).toBeNull() + }) + + it('the suggestion shapes: a Write offers acceptEdits, a Read outside the workspace offers a directory rule, a compound command offers both, `ls /` offers none', () => { + const [writeRequest, readRequest, compound, listRoot] = requests('shapes') + expect(writeRequest).toMatchObject({ tool: { name: 'Write' }, suggestions: [ACCEPT_EDITS] }) + expect(readRequest).toMatchObject({ tool: { name: 'Read' }, suggestions: [OUTSIDE_READ_RULE] }) + expect(readRequest!.decisionReason).toEqual(expect.any(String)) + expect(compound!.suggestions).toEqual([ + { type: 'addRules', rules: [{ toolName: 'Bash', ruleContent: 'cat /outside/notes.txt && git status' }], behavior: 'allow', destination: 'localSettings' }, + OUTSIDE_READ_RULE, + ]) + expect(compound!.blockedPath).toBe('/outside/notes.txt') + expect(listRoot).toMatchObject({ tool: { input: { command: 'ls /' } }, suggestions: [], blockedPath: '/' }) + }) + + it('ExitPlanMode offers no suggestions and carries the plan the model gave', () => { + for (const name of ['plan-default', 'plan-accept-edits', 'plan-bypass']) { + const [exit] = requests(name) + expect(exit, name).toMatchObject({ tool: { name: 'ExitPlanMode', input: { plan: receipt.plan } } }) + expect(exit!.suggestions, name).toBeUndefined() + } + }) + + it('a plain plan approval returns the CLI to the mode it had before plan mode (default here): the sidecar states it once, and a Write asks again', () => { + expect(scenario('plan-default').answered[0]!.decision).toEqual({ behavior: 'allow' }) + expect(modes('plan-default')).toEqual(['default']) + expect(requests('plan-default').map((request) => request.tool.name)).toEqual(['ExitPlanMode', 'Write']) + expect(scenario('plan-default').written['after.txt']).toBe(false) + }) + + it('a plan approval with setMode acceptEdits leaves plan mode for acceptEdits: stated once, and the next Write runs without asking', () => { + expect(modes('plan-accept-edits')).toEqual(['acceptEdits']) + expect(requests('plan-accept-edits').map((request) => request.tool.name)).toEqual(['ExitPlanMode']) + expect(scenario('plan-accept-edits').written['after.txt']).toBe(true) + }) + + it('a plan approval with setMode bypassPermissions leaves plan mode for bypassPermissions: stated once, and the next Write runs without asking', () => { + expect(modes('plan-bypass')).toEqual(['bypassPermissions']) + expect(requests('plan-bypass').map((request) => request.tool.name)).toEqual(['ExitPlanMode']) + expect(scenario('plan-bypass').written['after.txt']).toBe(true) + }) + + it('ExitPlanMode with no input: the CLI fills in the plan file\'s text and path when the model wrote one, and sends an empty input when it did not', () => { + const [fromFile] = requests('plan-file') + expect(fromFile!.tool.input).toEqual({ plan: receipt.planFileText, planFilePath: expect.stringMatching(/^\/home\/\.claude\/plans\/[^/]+\.md$/) }) + const [empty] = requests('plan-empty') + expect(empty!.tool.input).toEqual({}) + }) +}) From 47ce06a783786c3bcda4cebbf8c39e52a7e5d1fe Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Fri, 23 Oct 2026 08:44:49 -0700 Subject: [PATCH 02/17] feat(claude): normalize Claude's permission updates (D2) claude_permission_update.rs keeps only well-formed updates of the SDK's documented variants (addRules, replaceRules, removeRules, setMode, addDirectories, removeDirectories), rebuilt with exactly their documented keys, so a suggestion relayed into the strict snapshot can never fail the client's parse. validate_decision accepts a decision whose updatedPermissions is absent or a list of such updates, and refuses anything else. The next commits wire both in. Co-Authored-By: Claude Opus 5.5 --- .../src/claude_permission_update.rs | 286 ++++++++++++++++++ crates/freshell-freshagent/src/lib.rs | 1 + 2 files changed, 287 insertions(+) create mode 100644 crates/freshell-freshagent/src/claude_permission_update.rs diff --git a/crates/freshell-freshagent/src/claude_permission_update.rs b/crates/freshell-freshagent/src/claude_permission_update.rs new file mode 100644 index 000000000..832cc2856 --- /dev/null +++ b/crates/freshell-freshagent/src/claude_permission_update.rs @@ -0,0 +1,286 @@ +//! Claude permission updates (the SDK's `PermissionUpdate`, client-alerts plan +//! G20): the CLI's own suggestions on a permission request +//! (`sdk.permission.request.suggestions`) and the `updatedPermissions` a +//! client sends back to accept them. +//! +//! [`normalize_permission_updates`] keeps only well-formed updates of the +//! documented variants, with exactly the documented keys. The snapshot's +//! `pendingApprovals[].suggestions` is parsed by a `.strict()` client schema +//! (`FreshAgentPermissionUpdateSchema`), so one unknown variant or extra key +//! relayed raw would fail the whole snapshot and hide every card (the +//! AGENT-06 lesson behind `normalize_question_definitions`). +//! +//! [`validate_decision`] refuses an approval decision whose +//! `updatedPermissions` is not such a list: the sidecar resolves a decision +//! verbatim, and a malformed update reaching the CLI would fail or misapply +//! the user's choice. + +use serde_json::{Map, Value}; + +/// Where an update applies (the SDK's `PermissionUpdateDestination`). +const DESTINATIONS: [&str; 5] = [ + "userSettings", + "projectSettings", + "localSettings", + "session", + "cliArg", +]; +/// The SDK's `PermissionBehavior`. +const BEHAVIORS: [&str; 3] = ["allow", "deny", "ask"]; +/// The SDK's `PermissionMode` (a wider set than the four a pane can show). +const MODES: [&str; 6] = [ + "default", + "acceptEdits", + "bypassPermissions", + "plan", + "dontAsk", + "auto", +]; + +/// Normalize a list of permission updates: every well-formed update of a +/// documented variant, rebuilt with exactly its documented keys, in order. +/// Anything else (a non-list, an unknown variant, a bad destination, mode or +/// behavior, an empty rule or directory list) is dropped. `None` when nothing +/// survives. +pub(crate) fn normalize_permission_updates(value: Option<&Value>) -> Option> { + let updates: Vec = value? + .as_array()? + .iter() + .filter_map(normalize_permission_update) + .collect(); + (!updates.is_empty()).then_some(updates) +} + +/// Check an approval decision's `updatedPermissions`: absent is fine; +/// present, it must be a list every member of which is already a +/// well-formed update (it normalizes to itself). +pub(crate) fn validate_decision(decision: &Value) -> Result<(), String> { + let Some(updates) = decision.get("updatedPermissions") else { + return Ok(()); + }; + let Some(list) = updates.as_array() else { + return Err("updatedPermissions must be a list of permission updates".to_string()); + }; + for update in list { + if normalize_permission_update(update).as_ref() != Some(update) { + return Err(format!( + "updatedPermissions holds an update Claude does not accept: {update}" + )); + } + } + Ok(()) +} + +/// One update, rebuilt with exactly the keys of its variant, or `None`. +fn normalize_permission_update(update: &Value) -> Option { + let object = update.as_object()?; + let kind = object.get("type")?.as_str()?; + let destination = one_of(object.get("destination"), &DESTINATIONS)?; + let mut out = Map::new(); + out.insert("type".to_string(), Value::String(kind.to_string())); + match kind { + "addRules" | "replaceRules" | "removeRules" => { + let rules = object + .get("rules")? + .as_array()? + .iter() + .map(normalize_rule) + .collect::>>()?; + if rules.is_empty() { + return None; + } + out.insert("rules".to_string(), Value::Array(rules)); + out.insert( + "behavior".to_string(), + one_of(object.get("behavior"), &BEHAVIORS)?, + ); + } + "setMode" => { + out.insert("mode".to_string(), one_of(object.get("mode"), &MODES)?); + } + "addDirectories" | "removeDirectories" => { + let directories = object.get("directories")?.as_array()?; + if directories.is_empty() || !directories.iter().all(Value::is_string) { + return None; + } + out.insert("directories".to_string(), Value::Array(directories.clone())); + } + _ => return None, + } + out.insert("destination".to_string(), destination); + Some(Value::Object(out)) +} + +/// One rule (`PermissionRuleValue`): `toolName`, and `ruleContent` when given. +fn normalize_rule(rule: &Value) -> Option { + let object = rule.as_object()?; + let mut out = Map::new(); + let tool_name = object.get("toolName")?.as_str()?; + out.insert("toolName".to_string(), Value::String(tool_name.to_string())); + match object.get("ruleContent") { + None => {} + Some(Value::String(content)) => { + out.insert("ruleContent".to_string(), Value::String(content.clone())); + } + Some(_) => return None, + } + Some(Value::Object(out)) +} + +/// `value` when it is one of `allowed`. +fn one_of(value: Option<&Value>, allowed: &[&str]) -> Option { + let text = value?.as_str()?; + allowed + .contains(&text) + .then(|| Value::String(text.to_string())) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn normalized(value: Value) -> Option> { + normalize_permission_updates(Some(&value)) + } + + /// One of every documented variant, as the pinned CLI and SDK shape them. + fn every_variant() -> Vec { + vec![ + json!({ "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": "npm test *" }], "behavior": "allow", "destination": "localSettings" }), + json!({ "type": "replaceRules", "rules": [{ "toolName": "Read" }], "behavior": "deny", "destination": "projectSettings" }), + json!({ "type": "removeRules", "rules": [{ "toolName": "Edit", "ruleContent": "src/**" }], "behavior": "ask", "destination": "userSettings" }), + json!({ "type": "setMode", "mode": "acceptEdits", "destination": "session" }), + json!({ "type": "setMode", "mode": "bypassPermissions", "destination": "cliArg" }), + json!({ "type": "addDirectories", "directories": ["/srv/data"], "destination": "session" }), + json!({ "type": "removeDirectories", "directories": ["/srv/old", "/srv/older"], "destination": "localSettings" }), + ] + } + + #[test] + fn every_documented_variant_is_kept_exactly() { + let updates = every_variant(); + assert_eq!(normalized(Value::Array(updates.clone())), Some(updates)); + } + + #[test] + fn every_permission_mode_the_sdk_names_is_kept() { + for mode in [ + "default", + "acceptEdits", + "bypassPermissions", + "plan", + "dontAsk", + "auto", + ] { + let update = json!({ "type": "setMode", "mode": mode, "destination": "session" }); + assert_eq!( + normalized(json!([update.clone()])), + Some(vec![update]), + "{mode}" + ); + } + } + + #[test] + fn an_unknown_variant_a_bad_destination_or_a_bad_mode_is_dropped_and_the_rest_kept() { + let good = json!({ "type": "setMode", "mode": "acceptEdits", "destination": "session" }); + let list = json!([ + { "type": "grantEverything", "destination": "session" }, + { "type": "addRules", "rules": [{ "toolName": "Bash" }], "behavior": "allow", "destination": "everywhere" }, + { "type": "setMode", "mode": "yolo", "destination": "session" }, + good, + ]); + assert_eq!(normalized(list), Some(vec![good])); + } + + #[test] + fn malformed_members_are_dropped() { + assert_eq!( + normalized(json!([ + "addRules", + null, + { "rules": [{ "toolName": "Bash" }], "behavior": "allow", "destination": "session" }, + { "type": "addRules", "behavior": "allow", "destination": "session" }, + { "type": "addRules", "rules": [], "behavior": "allow", "destination": "session" }, + { "type": "addRules", "rules": [{ "ruleContent": "x" }], "behavior": "allow", "destination": "session" }, + { "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": 7 }], "behavior": "allow", "destination": "session" }, + { "type": "addRules", "rules": [{ "toolName": "Bash" }], "behavior": "always", "destination": "session" }, + { "type": "addRules", "rules": [{ "toolName": "Bash" }], "behavior": "allow" }, + { "type": "addDirectories", "directories": [], "destination": "session" }, + { "type": "addDirectories", "directories": ["/a", 3], "destination": "session" }, + { "type": "setMode", "destination": "session" }, + ])), + None + ); + } + + #[test] + fn extra_keys_are_stripped_at_every_level() { + assert_eq!( + normalized(json!([ + { "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": "ls", "note": "x" }], "behavior": "allow", "destination": "session", "id": 4 }, + { "type": "setMode", "mode": "plan", "destination": "session", "reason": "plan first" }, + { "type": "addDirectories", "directories": ["/a"], "destination": "session", "recursive": true }, + ])), + Some(vec![ + json!({ "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": "ls" }], "behavior": "allow", "destination": "session" }), + json!({ "type": "setMode", "mode": "plan", "destination": "session" }), + json!({ "type": "addDirectories", "directories": ["/a"], "destination": "session" }), + ]) + ); + } + + #[test] + fn an_empty_absent_or_non_list_value_is_none() { + assert_eq!(normalized(json!([])), None); + assert_eq!(normalize_permission_updates(None), None); + assert_eq!(normalized(Value::Null), None); + assert_eq!( + normalized(json!({ "type": "setMode", "mode": "plan", "destination": "session" })), + None + ); + } + + #[test] + fn a_decision_without_updated_permissions_or_with_an_empty_list_is_valid() { + assert_eq!(validate_decision(&json!({ "behavior": "allow" })), Ok(())); + assert_eq!( + validate_decision(&json!({ "behavior": "deny", "message": "no", "interrupt": false })), + Ok(()) + ); + assert_eq!( + validate_decision(&json!({ "behavior": "allow", "updatedPermissions": [] })), + Ok(()) + ); + } + + #[test] + fn a_decision_with_well_formed_updates_is_valid() { + assert_eq!( + validate_decision( + &json!({ "behavior": "allow", "updatedPermissions": every_variant() }) + ), + Ok(()) + ); + } + + #[test] + fn a_decision_whose_updated_permissions_is_not_a_well_formed_list_is_refused() { + for bad in [ + json!(null), + json!("addRules"), + json!(3), + json!({ "type": "setMode", "mode": "plan", "destination": "session" }), + json!([{ "type": "grantEverything", "destination": "session" }]), + json!([{ "type": "setMode", "mode": "plan", "destination": "session", "extra": 1 }]), + json!([ + { "type": "setMode", "mode": "plan", "destination": "session" }, + { "type": "setMode", "mode": "yolo", "destination": "session" }, + ]), + ] { + let decision = json!({ "behavior": "allow", "updatedPermissions": bad }); + assert!(validate_decision(&decision).is_err(), "accepted {decision}"); + } + } +} diff --git a/crates/freshell-freshagent/src/lib.rs b/crates/freshell-freshagent/src/lib.rs index 460b7d627..8fdc23bfb 100644 --- a/crates/freshell-freshagent/src/lib.rs +++ b/crates/freshell-freshagent/src/lib.rs @@ -41,6 +41,7 @@ pub(crate) mod claude_dead_sessions; pub(crate) mod claude_handshake; pub(crate) mod claude_launch; pub(crate) mod claude_live_events; +pub(crate) mod claude_permission_update; pub(crate) mod claude_projection; pub(crate) mod claude_projection_cache; pub(crate) mod claude_sidecar_exit; From 7357f37d35faf7b1d4a328004ee2d9187b26767a Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Fri, 23 Oct 2026 08:47:00 -0700 Subject: [PATCH 03/17] feat(claude): pending approvals carry the CLI's suggestions (D3) The pending-approval fold keeps an sdk.permission.request's suggestions, normalized at fold time, and the snapshot overlay emits them as pendingApprovals[].suggestions (absent when none survive). The shared contract gains FreshAgentPermissionUpdateSchema, a strict discriminated union mirroring the server's normalizer, so a well-formed list parses and one malformed suggestion fails the strict snapshot parse, which is why the server normalizes first. Co-Authored-By: Claude Opus 5.5 --- crates/freshell-freshagent/src/claude.rs | 22 +++++-- crates/freshell-freshagent/src/snapshot.rs | 64 +++++++++++++++++++ shared/fresh-agent-contract.ts | 37 +++++++++++ test/unit/shared/fresh-agent-contract.test.ts | 27 ++++++++ 4 files changed, 146 insertions(+), 4 deletions(-) diff --git a/crates/freshell-freshagent/src/claude.rs b/crates/freshell-freshagent/src/claude.rs index 57a4fac54..55d960587 100644 --- a/crates/freshell-freshagent/src/claude.rs +++ b/crates/freshell-freshagent/src/claude.rs @@ -520,12 +520,17 @@ struct PendingApprovalEntry { blocked_path: Option, decision_reason: Option, input: Option, + /// The CLI's own suggestions (what "Allow and don't ask again" sends back), + /// normalized to the strict contract shape on entry + /// ([`crate::claude_permission_update::normalize_permission_updates`]); `None` + /// when the request offered none that survive. + suggestions: Option>, } impl PendingApprovalEntry { /// The `.strict()` contract shape the Task 3 snapshot overlay serializes - /// (`{requestId, toolName?, toolUseID?, blockedPath?, decisionReason?, input?}` — - /// keys omitted when absent, never null). + /// (`{requestId, toolName?, toolUseID?, blockedPath?, decisionReason?, input?, + /// suggestions?}` — keys omitted when absent, never null). fn to_json(&self) -> Value { let mut obj = Map::new(); obj.insert("requestId".to_string(), json!(self.request_id)); @@ -544,6 +549,9 @@ impl PendingApprovalEntry { if let Some(input) = &self.input { obj.insert("input".to_string(), input.clone()); } + if let Some(suggestions) = &self.suggestions { + obj.insert("suggestions".to_string(), Value::Array(suggestions.clone())); + } Value::Object(obj) } } @@ -4977,8 +4985,8 @@ impl FreshClaudeState { /// The session's live pending approvals + questions as overlay JSON (approvals, /// questions) — Task 3's snapshot route consumes this. Entry keys are exactly the /// `.strict()` contract shape (`{requestId, toolName?, toolUseID?, blockedPath?, - /// decisionReason?, input?}` / `{requestId, questions}`, no extra keys, absent keys - /// omitted). `any_id` resolves like every handler here: the map key OR the durable + /// decisionReason?, input?, suggestions?}` / `{requestId, questions}`, no extra keys, + /// absent keys omitted). `any_id` resolves like every handler here: the map key OR the durable /// Claude UUID via `cli_index`. An untracked id yields two empty vecs. pub(crate) async fn snapshot_pending_overlay(&self, any_id: &str) -> (Vec, Vec) { let Some(map_key) = self.resolve_session_key(any_id).await else { @@ -7554,6 +7562,11 @@ fn fold_pending_frame(pending: &std::sync::Mutex, value: &Value) .and_then(Value::as_str) .map(str::to_string), input: value.get("tool").and_then(|t| t.get("input")).cloned(), + // Normalized like the questions below: the snapshot's strict + // parse must never see an unknown variant or key (AGENT-06). + suggestions: crate::claude_permission_update::normalize_permission_updates( + value.get("suggestions"), + ), }; let mut p = pending.lock().expect("pending lock"); p.permissions.retain(|e| e.request_id != entry.request_id); @@ -17816,6 +17829,7 @@ rl.on('line', (line) => { blocked_path: None, decision_reason: None, input: None, + suggestions: None, }], questions: Vec::new(), }; diff --git a/crates/freshell-freshagent/src/snapshot.rs b/crates/freshell-freshagent/src/snapshot.rs index dea80f6cc..e0d049a24 100644 --- a/crates/freshell-freshagent/src/snapshot.rs +++ b/crates/freshell-freshagent/src/snapshot.rs @@ -890,6 +890,70 @@ mod tests { assert_eq!(value["capabilities"]["questions"], json!(true)); } + /// Client-alerts G20 (D3): a pending approval carries the CLI's own suggestions + /// (the updates "Allow and don't ask again" sends back), normalized at fold time + /// to the strict `FreshAgentPermissionUpdateSchema` shape: well-formed updates + /// keep exactly their documented keys, anything else is dropped, and an approval + /// with none left has no `suggestions` key at all. + #[tokio::test] + async fn claude_pending_approvals_carry_the_clis_suggestions_normalized() { + let _guard = CLAUDE_ENV_LOCK.lock().await; + let durable = "82828282-8282-4828-8828-828282828282"; + let (_home, _content) = stage_transcript(durable); + let claude = claude_state(); + crate::claude::tests::insert_fake_claude_session_with_pending( + &claude, + "client-nanoid-suggest", + Some(durable), + &[ + json!({ "type": "sdk.permission.request", "sessionId": "s", "requestId": "req-npm", + "subtype": "can_use_tool", + "tool": { "name": "Bash", "input": { "command": "cat /srv/notes && npm test" } }, + "suggestions": [ + { "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": "npm test *", "source": "cli" }], + "behavior": "allow", "destination": "localSettings" }, + { "type": "addRules", "rules": [{ "toolName": "Read", "ruleContent": "//srv/**" }], + "behavior": "allow", "destination": "session" }, + { "type": "grantEverything", "destination": "session" }, + ] }), + json!({ "type": "sdk.permission.request", "sessionId": "s", "requestId": "req-root", + "subtype": "can_use_tool", + "tool": { "name": "Bash", "input": { "command": "ls /" } }, + "suggestions": [], "blockedPath": "/" }), + ], + ) + .await; + + let state = SnapshotState::new( + Arc::new("tok".to_string()), + codex_state(), + opencode_state(), + claude, + ); + let (status, value) = get_json_with_state(state, "freshclaude", durable).await; + std::env::remove_var("CLAUDE_CONFIG_DIR"); + assert_eq!(status, StatusCode::OK); + assert_eq!( + value["pendingApprovals"], + json!([ + { + "requestId": "req-npm", "toolName": "Bash", + "input": { "command": "cat /srv/notes && npm test" }, + "suggestions": [ + { "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": "npm test *" }], + "behavior": "allow", "destination": "localSettings" }, + { "type": "addRules", "rules": [{ "toolName": "Read", "ruleContent": "//srv/**" }], + "behavior": "allow", "destination": "session" }, + ], + }, + { + "requestId": "req-root", "toolName": "Bash", "blockedPath": "/", + "input": { "command": "ls /" }, + }, + ]) + ); + } + /// Delta-review round 5 (AGENT-06): an SDK-valid question whose options carry the /// documented `preview` field (plus other extras the sidecar preserves verbatim via /// `permission-channel.mjs`'s `...o`) must still produce a snapshot whose diff --git a/shared/fresh-agent-contract.ts b/shared/fresh-agent-contract.ts index 83e27e2ed..15166df97 100644 --- a/shared/fresh-agent-contract.ts +++ b/shared/fresh-agent-contract.ts @@ -261,6 +261,40 @@ export const FreshAgentTurnSchema = z.object({ restorable: z.boolean().optional(), }).strict() +// Claude's permission update (the SDK's `PermissionUpdate`): what the CLI +// suggests on an approval, and what "Allow and don't ask again" sends back as +// `updatedPermissions`. Mirrors the server's normalizer +// (crates/freshell-freshagent/src/claude_permission_update.rs), which drops +// anything outside this shape before it reaches a snapshot. +const FreshAgentPermissionUpdateDestinationSchema = z.enum(['userSettings', 'projectSettings', 'localSettings', 'session', 'cliArg']) +const FreshAgentPermissionRulesSchema = z.array(z.object({ + toolName: z.string(), + ruleContent: z.string().optional(), +}).strict()).min(1) +const FreshAgentPermissionRulesUpdateSchema = (type: 'addRules' | 'replaceRules' | 'removeRules') => z.object({ + type: z.literal(type), + rules: FreshAgentPermissionRulesSchema, + behavior: z.enum(['allow', 'deny', 'ask']), + destination: FreshAgentPermissionUpdateDestinationSchema, +}).strict() +const FreshAgentPermissionDirectoriesUpdateSchema = (type: 'addDirectories' | 'removeDirectories') => z.object({ + type: z.literal(type), + directories: z.array(z.string()).min(1), + destination: FreshAgentPermissionUpdateDestinationSchema, +}).strict() +export const FreshAgentPermissionUpdateSchema = z.discriminatedUnion('type', [ + FreshAgentPermissionRulesUpdateSchema('addRules'), + FreshAgentPermissionRulesUpdateSchema('replaceRules'), + FreshAgentPermissionRulesUpdateSchema('removeRules'), + z.object({ + type: z.literal('setMode'), + mode: z.enum(['default', 'acceptEdits', 'bypassPermissions', 'plan', 'dontAsk', 'auto']), + destination: FreshAgentPermissionUpdateDestinationSchema, + }).strict(), + FreshAgentPermissionDirectoriesUpdateSchema('addDirectories'), + FreshAgentPermissionDirectoriesUpdateSchema('removeDirectories'), +]) + export const FreshAgentPendingApprovalSchema = z.object({ requestId: FreshAgentRequestIdSchema, toolName: z.string().optional(), @@ -268,6 +302,8 @@ export const FreshAgentPendingApprovalSchema = z.object({ blockedPath: z.string().optional(), decisionReason: z.string().optional(), input: z.record(z.string(), z.unknown()).optional(), + // Claude only: the CLI's own suggestions for not asking again. + suggestions: z.array(FreshAgentPermissionUpdateSchema).optional(), providerRequest: z.record(z.string(), z.unknown()).optional(), }).strict() @@ -427,6 +463,7 @@ export type FreshAgentRequestId = z.infer export type FreshAgentTranscriptItem = z.infer export type FreshAgentTurn = z.infer export type FreshAgentPendingApproval = z.infer +export type FreshAgentPermissionUpdate = z.infer export type FreshAgentPendingQuestion = z.infer export type FreshAgentSessionCommand = z.infer export type FreshAgentSnapshot = z.infer diff --git a/test/unit/shared/fresh-agent-contract.test.ts b/test/unit/shared/fresh-agent-contract.test.ts index 190a74ee5..f7a6ace7b 100644 --- a/test/unit/shared/fresh-agent-contract.test.ts +++ b/test/unit/shared/fresh-agent-contract.test.ts @@ -38,6 +38,33 @@ describe('fresh-agent shared contract schemas', () => { expect(FreshAgentSnapshotSchema.parse(codexContractSnapshot).turns[0].summaryKind).toBe('echo') }) + it('a claude pending approval carries the CLI\'s permission suggestions, and one malformed suggestion fails the strict parse', () => { + const suggestions = [ + { type: 'addRules', rules: [{ toolName: 'Bash', ruleContent: 'npm test *' }], behavior: 'allow', destination: 'localSettings' }, + { type: 'replaceRules', rules: [{ toolName: 'Read' }], behavior: 'deny', destination: 'projectSettings' }, + { type: 'removeRules', rules: [{ toolName: 'Edit', ruleContent: 'src/**' }], behavior: 'ask', destination: 'userSettings' }, + { type: 'setMode', mode: 'acceptEdits', destination: 'session' }, + { type: 'setMode', mode: 'dontAsk', destination: 'cliArg' }, + { type: 'addDirectories', directories: ['/srv/data'], destination: 'session' }, + { type: 'removeDirectories', directories: ['/srv/old'], destination: 'localSettings' }, + ] + const withSuggestions = (list: unknown[]) => ({ + ...claudeContractSnapshot, + pendingApprovals: [{ requestId: 'req-1', toolName: 'Bash', input: { command: 'npm test' }, suggestions: list }], + }) + expect(FreshAgentSnapshotSchema.parse(withSuggestions(suggestions)).pendingApprovals[0].suggestions).toEqual(suggestions) + for (const bad of [ + { type: 'grantEverything', destination: 'session' }, + { type: 'setMode', mode: 'yolo', destination: 'session' }, + { type: 'addRules', rules: [{ toolName: 'Bash' }], behavior: 'allow', destination: 'everywhere' }, + { type: 'addRules', rules: [{ toolName: 'Bash', note: 'x' }], behavior: 'allow', destination: 'session' }, + { type: 'setMode', mode: 'plan', destination: 'session', reason: 'extra key' }, + { type: 'addDirectories', directories: [], destination: 'session' }, + ]) { + expect(FreshAgentSnapshotSchema.safeParse(withSuggestions([suggestions[0], bad])).success, JSON.stringify(bad)).toBe(false) + } + }) + it('parses turn pages and turn bodies with the full session locator', () => { expect(FreshAgentTurnPageSchema.parse(claudeContractTurnPage).provider).toBe('claude') expect(FreshAgentTurnPageSchema.parse(codexContractTurnPage).provider).toBe('codex') From 6f1909e9acc52f2f3f288f4f575fcf13472b2704 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Fri, 23 Oct 2026 08:52:16 -0700 Subject: [PATCH 04/17] feat(claude): refuse an approval whose updatedPermissions Claude would not accept (D4) handle_approval_respond now checks the decision with validate_decision before writing it to the sidecar. A malformed updatedPermissions list is answered with freshAgent.error{code:'INVALID_APPROVAL_DECISION'} and a warn log (freshagent.claude.approval_decision_refused); nothing reaches the CLI and the request stays pending, so a retry with well-formed updates lands verbatim. A new WS test over the real sidecar and the CLI simulator pins the whole path: the CLI's suggestions reach the snapshot normalized, a malformed list is refused with the card kept, and the suggestions sent back reach the CLI exactly, whose accepted setMode then reaches every client as the session's mode. Co-Authored-By: Claude Opus 5.5 --- crates/freshell-freshagent/src/claude.rs | 89 +++++++++ .../tests/freshagent_claude_approvals.rs | 180 ++++++++++++++++++ 2 files changed, 269 insertions(+) create mode 100644 crates/freshell-ws/tests/freshagent_claude_approvals.rs diff --git a/crates/freshell-freshagent/src/claude.rs b/crates/freshell-freshagent/src/claude.rs index 55d960587..9712dceb3 100644 --- a/crates/freshell-freshagent/src/claude.rs +++ b/crates/freshell-freshagent/src/claude.rs @@ -4795,6 +4795,30 @@ impl FreshClaudeState { ); return; } + // Client-alerts G20: the decision's `updatedPermissions` (the CLI's own + // suggestions the user accepted) reaches the CLI verbatim, so a malformed + // one would fail or misapply the user's choice there. Refuse it with the + // same discipline as above: nothing forwarded, the request stays pending. + if let Err(reason) = crate::claude_permission_update::validate_decision(&msg.decision) { + drop(session); + tracing::warn!( + target: "freshell_freshagent::claude", + event = "freshagent.claude.approval_decision_refused", + provider = PROVIDER, + session_id = %session_id, + session_type, + request_id = %request_id, + reason = %reason, + "a claude approval decision carried malformed permission updates; nothing was sent" + ); + self.emit_fresh_agent_error( + &session_id, + session_type, + "INVALID_APPROVAL_DECISION", + &format!("Claude approval {request_id} was not sent: {reason}"), + ); + return; + } // Address the sidecar by ITS id for this session (== the map key for created // sessions; differs for resumed-on-attach sessions, Task 6). let respond_req = json!({ @@ -17901,6 +17925,71 @@ rl.on('line', (line) => { drop(env); } + /// Client-alerts G20 (D4): a decision whose `updatedPermissions` is not a list of + /// well-formed permission updates is refused with `INVALID_APPROVAL_DECISION` + /// before anything reaches the sidecar, and the request stays pending; a retry + /// with well-formed updates is written VERBATIM and resolves it. + #[tokio::test] + async fn approval_respond_with_malformed_updated_permissions_is_refused_and_a_valid_retry_lands() + { + let _guard = CLAUDE_ENV_LOCK.lock().await; + let env = FakeClaudeSidecarEnv::install(); + let (st, mut rx) = state_with_bus(); + st.handle_create(dedup_create_msg("req-approval-bad-updates"), None) + .await; + let created = await_claude_created(&mut rx, "req-approval-bad-updates").await; + let session_id = created["sessionId"].as_str().unwrap().to_string(); + st.handle_send(send_msg(&session_id, "__raise_permission__")) + .await; + await_pending_permission(&st, &session_id, "req-1").await; + + st.handle_approval_respond(approval_respond_msg( + &session_id, + SessionType::Freshclaude, + "req-1", + json!({ "behavior": "allow", "updatedPermissions": [ + { "type": "grantEverything", "destination": "session" }, + ] }), + )) + .await; + + let frame = await_frame_of_inner_type(&mut rx, "freshAgent.error").await; + assert_eq!(frame["event"]["code"], "INVALID_APPROVAL_DECISION"); + assert_eq!(frame["sessionId"], json!(session_id)); + let (permissions, _) = pending_request_ids(&st, &session_id).await; + assert_eq!( + permissions, + vec!["req-1".to_string()], + "a refused decision leaves the request pending" + ); + assert!( + env.respond_log_frames(0).await.is_empty(), + "a refused decision never reaches the sidecar" + ); + + let decision = json!({ "behavior": "allow", "updatedPermissions": [ + { "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": "npm test *" }], + "behavior": "allow", "destination": "localSettings" }, + { "type": "setMode", "mode": "acceptEdits", "destination": "session" }, + ] }); + st.handle_approval_respond(approval_respond_msg( + &session_id, + SessionType::Freshclaude, + "req-1", + decision.clone(), + )) + .await; + let frames = env.respond_log_frames(1).await; + let respond = frames + .iter() + .find(|f| f["type"] == "permission.respond") + .expect("the valid retry reaches the sidecar"); + assert_eq!(respond["decision"], decision, "forwarded verbatim"); + let (permissions, _) = pending_request_ids(&st, &session_id).await; + assert!(permissions.is_empty(), "the valid retry resolves the request"); + drop(env); + } + /// Task 2 (b): a requestId outside the pending set is refused LOUDLY with the parity /// message on the nested hub-frame — and NOTHING is written to the sidecar (Rust /// validates against its pending set first; the sidecar's unknown-id path is diff --git a/crates/freshell-ws/tests/freshagent_claude_approvals.rs b/crates/freshell-ws/tests/freshagent_claude_approvals.rs new file mode 100644 index 000000000..ce6f7ad7d --- /dev/null +++ b/crates/freshell-ws/tests/freshagent_claude_approvals.rs @@ -0,0 +1,180 @@ +//! Claude approvals with the CLI's own permission suggestions (client-alerts +//! plan G20), end to end over the REAL claude sidecar and the CLI simulator +//! (RealSidecarSim, `common::claude_rig`), a real freshell-ws server, its +//! snapshot REST route and a WS client. +//! +//! - The approval's suggestions reach the snapshot the card renders from, +//! normalized to the strict contract shape. +//! - "Allow and don't ask again" sends them back as `updatedPermissions`, and +//! the CLI receives exactly that decision; a malformed list is refused with +//! `INVALID_APPROVAL_DECISION`, nothing reaches the CLI and the card stays. +//! - A permission mode the CLI moves to by itself (an accepted `setMode`) is +//! the session's mode from then on, across a server restart too. + +mod common; + +use serde_json::{json, Value}; + +use common::claude_rig::{ClaudeRig, RigConnection, RigKind, RigOptions, RIG_FRAME_BUDGET}; + +const APPROVAL_PROMPT: &str = "run the tests"; + +/// The CLI's suggestions for `npm test`, plus one no client could parse. +fn offered_suggestions() -> Value { + json!([ + { "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": "npm test *" }], + "behavior": "allow", "destination": "localSettings" }, + { "type": "setMode", "mode": "acceptEdits", "destination": "session" }, + { "type": "grantEverything", "destination": "session" }, + ]) +} + +/// What the snapshot states: the well-formed suggestions only. +fn normalized_suggestions() -> Value { + json!([ + { "type": "addRules", "rules": [{ "toolName": "Bash", "ruleContent": "npm test *" }], + "behavior": "allow", "destination": "localSettings" }, + { "type": "setMode", "mode": "acceptEdits", "destination": "session" }, + ]) +} + +async fn approvals_rig() -> ClaudeRig { + let program = json!({ "rules": [ + { "on": "msg:prompt", "match": { "text": APPROVAL_PROMPT }, "emit": [ + { "kind": "approval", "data": { + "tool": "Bash", "input": { "command": "npm test" }, + "suggestions": offered_suggestions(), + } }, + { "kind": "text", "data": { "text": "tests ran" } }, + ] }, + ] }) + .to_string(); + ClaudeRig::start_with_options( + RigKind::Sim, + RigOptions { + env: vec![("FRESHELL_CLAUDE_SIM_PROGRAM".into(), program)], + durable_ledger: true, + ..RigOptions::default() + }, + ) + .await +} + +/// Create a session in `default` mode (where the CLI asks before a Bash call) +/// and raise the approval; returns (pane id, durable id, request id). +async fn raise_approval(conn: &mut RigConnection) -> (String, String, Value) { + let created = conn + .request_create("approvals", json!({ "permissionMode": "default" })) + .await; + assert_eq!(created["type"], "freshAgent.created", "{created}"); + let pane = created["sessionId"].as_str().unwrap().to_string(); + conn.send_text(&pane, "send-approval", APPROVAL_PROMPT) + .await; + let request = conn.wait_for_event("freshAgent.permission.request").await; + let durable = conn.init_session_id(&pane); + (pane, durable, request["event"]["requestId"].clone()) +} + +fn respond(pane: &str, request_id: &Value, decision: Value) -> Value { + json!({ + "type": "freshAgent.approval.respond", + "provider": "claude", + "sessionType": "freshclaude", + "sessionId": pane, + "requestId": request_id, + "decision": decision, + }) +} + +/// Poll the snapshot route until `ready` holds. +async fn snapshot_when( + rig: &ClaudeRig, + thread_id: &str, + what: &str, + ready: impl Fn(&Value) -> bool, +) -> Value { + let deadline = tokio::time::Instant::now() + RIG_FRAME_BUDGET; + loop { + let (status, snapshot) = rig.snapshot(thread_id).await; + if status == 200 && ready(&snapshot) { + return snapshot; + } + assert!( + tokio::time::Instant::now() < deadline, + "the snapshot never showed {what}; last answer {status}: {snapshot}" + ); + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } +} + +/// What the simulated CLI's canUseTool resolved with, in order. +fn cli_decisions(rig: &ClaudeRig) -> Vec { + rig.ledger_rows("can_use_tool") + .into_iter() + .map(|row| row["decision"].clone()) + .collect() +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_remembered_approval_sends_the_clis_suggestions_and_a_malformed_list_is_refused() { + let rig = approvals_rig().await; + let mut conn = rig.connect().await; + let (pane, durable, request_id) = raise_approval(&mut conn).await; + + let pending = snapshot_when(&rig, &durable, "the pending approval", |snapshot| { + snapshot["pendingApprovals"] + .as_array() + .is_some_and(|list| !list.is_empty()) + }) + .await; + assert_eq!( + pending["pendingApprovals"][0]["suggestions"], + normalized_suggestions() + ); + + // A malformed list: refused, nothing reaches the CLI, the card stays. + conn.send(respond( + &pane, + &request_id, + json!({ "behavior": "allow", "updatedPermissions": offered_suggestions() }), + )) + .await; + let refusal = conn.wait_for_event("freshAgent.error").await; + assert_eq!(refusal["event"]["code"], "INVALID_APPROVAL_DECISION"); + let (_, still) = rig.snapshot(&durable).await; + assert_eq!( + still["pendingApprovals"][0]["requestId"], request_id, + "{still}" + ); + assert!(cli_decisions(&rig).is_empty()); + + // The suggestions as the card has them: the CLI gets exactly this decision. + let decision = json!({ + "behavior": "allow", + "updatedPermissions": pending["pendingApprovals"][0]["suggestions"], + }); + conn.send(respond(&pane, &request_id, decision.clone())) + .await; + let deadline = tokio::time::Instant::now() + RIG_FRAME_BUDGET; + while cli_decisions(&rig).is_empty() { + assert!( + tokio::time::Instant::now() < deadline, + "the CLI never got the decision" + ); + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert_eq!(cli_decisions(&rig), vec![decision]); + // The accepted setMode moves the CLI to acceptEdits; every client hears it. + conn.wait_for("the CLI's own mode change", |frame| { + frame["sessionId"] == pane.as_str() + && frame["event"]["type"] == "freshAgent.session.metadata" + && frame["event"]["permissionMode"] == "acceptEdits" + }) + .await; + snapshot_when(&rig, &durable, "the approval answered", |snapshot| { + snapshot["pendingApprovals"] == json!([]) + && snapshot["settings"]["permissionMode"] == "acceptEdits" + }) + .await; + conn.kill(&pane).await; +} From e59cc9cdb722a801e424f860858561c61f604e3b Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Fri, 23 Oct 2026 08:53:56 -0700 Subject: [PATCH 05/17] feat(claude): the CLI's own permission mode is saved with the session (D5) When the CLI changes its own mode (an approved ExitPlanMode, or a setMode suggestion the user accepted), the stdout consumer already folded it into the session's live settings. It now also writes the session's binding row, through the same adoption a settings apply uses, so a resume after a server restart boots the conversation in the mode the CLI was in rather than the mode the pane created it with. Pinned over the real sidecar and the CLI simulator with a durable ledger and a restart. Co-Authored-By: Claude Opus 5.5 --- crates/freshell-freshagent/src/claude.rs | 36 +++++++++++++--- .../tests/freshagent_claude_approvals.rs | 43 +++++++++++++++++++ 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/crates/freshell-freshagent/src/claude.rs b/crates/freshell-freshagent/src/claude.rs index 9712dceb3..4e3dc5d4b 100644 --- a/crates/freshell-freshagent/src/claude.rs +++ b/crates/freshell-freshagent/src/claude.rs @@ -6761,11 +6761,33 @@ impl FreshClaudeState { // mode). The sidecar already runs in it; the session's // settings take it too, so a later configure stating the // same mode applies nothing and the snapshot states it. + // The binding row takes it the way a configure's settings + // do (client-alerts G20), so a resume after a restart + // boots the conversation in the mode the CLI is in. Some("sdk.session.metadata") => { if let Some(mode) = value.get("permissionMode").and_then(Value::as_str) { - if let Some(session) = sessions.lock().await.get_mut(&session_id) { - session.configuration.settings.permission_mode = - Some(mode.to_string()); + let adopted = + sessions.lock().await.get_mut(&session_id).map(|session| { + session.configuration.settings.permission_mode = + Some(mode.to_string()); + ( + session.cli_session_id.clone(), + session.configuration.settings.clone(), + ) + }); + if let Some((Some(durable), settings)) = adopted { + state + .adopt_session_init( + &durable, + &session_id, + &session_type, + Some(&settings), + identity_sink.clone(), + // Conn-less, like a settings apply: the + // ledger merge keeps the row's stamps. + None, + ) + .await; } } } @@ -17930,8 +17952,7 @@ rl.on('line', (line) => { /// before anything reaches the sidecar, and the request stays pending; a retry /// with well-formed updates is written VERBATIM and resolves it. #[tokio::test] - async fn approval_respond_with_malformed_updated_permissions_is_refused_and_a_valid_retry_lands() - { + async fn approval_respond_refuses_malformed_updated_permissions_until_a_valid_retry() { let _guard = CLAUDE_ENV_LOCK.lock().await; let env = FakeClaudeSidecarEnv::install(); let (st, mut rx) = state_with_bus(); @@ -17986,7 +18007,10 @@ rl.on('line', (line) => { .expect("the valid retry reaches the sidecar"); assert_eq!(respond["decision"], decision, "forwarded verbatim"); let (permissions, _) = pending_request_ids(&st, &session_id).await; - assert!(permissions.is_empty(), "the valid retry resolves the request"); + assert!( + permissions.is_empty(), + "the valid retry resolves the request" + ); drop(env); } diff --git a/crates/freshell-ws/tests/freshagent_claude_approvals.rs b/crates/freshell-ws/tests/freshagent_claude_approvals.rs index ce6f7ad7d..360a97248 100644 --- a/crates/freshell-ws/tests/freshagent_claude_approvals.rs +++ b/crates/freshell-ws/tests/freshagent_claude_approvals.rs @@ -178,3 +178,46 @@ async fn a_remembered_approval_sends_the_clis_suggestions_and_a_malformed_list_i .await; conn.kill(&pane).await; } + +/// The mode the CLI moved to by itself is saved with the session: after a +/// server restart the pane's attach resumes the conversation in that mode, +/// not in the mode the pane created it with. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn the_mode_the_cli_moved_to_is_the_mode_a_resume_boots_in() { + let mut rig = approvals_rig().await; + let mut conn = rig.connect().await; + let (pane, durable, request_id) = raise_approval(&mut conn).await; + conn.send(respond( + &pane, + &request_id, + json!({ "behavior": "allow", "updatedPermissions": normalized_suggestions() }), + )) + .await; + conn.wait_for("the CLI's own mode change", |frame| { + frame["sessionId"] == pane.as_str() + && frame["event"]["type"] == "freshAgent.session.metadata" + && frame["event"]["permissionMode"] == "acceptEdits" + }) + .await; + conn.wait_for_event("freshAgent.result").await; + rig.wait_for_transcript(&durable).await; + drop(conn); + + rig.restart().await; + let mut conn = rig.connect().await; + conn.attach(&durable).await; + let boots: Vec = rig + .ledger_rows("query") + .into_iter() + .map(|row| row["options"].clone()) + .collect(); + assert_eq!( + boots.len(), + 2, + "the create's boot and the resume's: {boots:?}" + ); + assert_eq!(boots[0]["permissionMode"], "default", "{:?}", boots[0]); + assert_eq!(boots[1]["resume"], durable.as_str(), "{:?}", boots[1]); + assert_eq!(boots[1]["permissionMode"], "acceptEdits", "{:?}", boots[1]); + conn.kill(&durable).await; +} From 954b18b01b15791bd1068ee370ceb0d7d2981834 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Fri, 23 Oct 2026 08:55:20 -0700 Subject: [PATCH 06/17] feat(fresh-agent): one builder for approval decisions, and plain-language suggestion lines (D8) src/lib/fresh-agent-permission-updates.ts builds every approval answer: a plain allow (never updatedInput), an allow that applies exactly the suggestions shown, a deny that keeps the turn going, and the plan card's four choices. describePermissionUpdate says in one line what accepting a suggestion does and where it is kept (this conversation, the project's .claude/settings.local.json or settings.json, or every project), and rememberLabel names the remember button. Co-Authored-By: Claude Opus 5.5 --- src/lib/fresh-agent-permission-updates.ts | 119 ++++++++++++++++++ .../fresh-agent-permission-updates.test.ts | 111 ++++++++++++++++ 2 files changed, 230 insertions(+) create mode 100644 src/lib/fresh-agent-permission-updates.ts create mode 100644 test/unit/client/lib/fresh-agent-permission-updates.test.ts diff --git a/src/lib/fresh-agent-permission-updates.ts b/src/lib/fresh-agent-permission-updates.ts new file mode 100644 index 000000000..769a352b3 --- /dev/null +++ b/src/lib/fresh-agent-permission-updates.ts @@ -0,0 +1,119 @@ +// Claude approval decisions and the plain-language lines for Claude's own +// permission suggestions (client-alerts plan G20). +// +// Every approval answer a client sends is built here, so the wire shapes live +// in one place: a plain allow never carries `updatedInput` (a defined one, +// even {}, wholesale replaces the tool input — the sidecar resolves the +// decision verbatim), a remembered allow carries exactly the suggestions the +// card showed as `updatedPermissions`, and a deny keeps the turn going. + +import type { FreshAgentPermissionUpdate } from '@shared/fresh-agent-contract' + +export type FreshAgentApprovalDecision = + | { behavior: 'allow'; updatedPermissions?: FreshAgentPermissionUpdate[] } + | { behavior: 'deny'; message: string; interrupt: false } + +/** What "Keep planning" tells Claude when the user wrote nothing. */ +export const KEEP_PLANNING_MESSAGE = 'The user wants to keep planning.' +const DENY_MESSAGE = 'Denied by user' + +/** Allow the tool call; with `updates`, also apply those permission updates. */ +export function buildAllowDecision(updates?: readonly FreshAgentPermissionUpdate[]): FreshAgentApprovalDecision { + return updates && updates.length > 0 + ? { behavior: 'allow', updatedPermissions: [...updates] } + : { behavior: 'allow' } +} + +/** Deny the tool call and let Claude carry on with `message` (or the default). */ +export function buildDenyDecision(message?: string): FreshAgentApprovalDecision { + return { behavior: 'deny', message: message?.trim() || DENY_MESSAGE, interrupt: false } +} + +/** The plan card's four answers. */ +export type PlanApprovalChoice = 'approve' | 'approve-accept-edits' | 'approve-bypass' | 'keep-planning' + +/** + * A plan answer. A plain approval lets the CLI return to the mode it had before + * plan mode; the other two approvals set the mode for the rest of the + * conversation; "Keep planning" denies with the user's note. + */ +export function buildPlanDecision(choice: PlanApprovalChoice, feedback?: string): FreshAgentApprovalDecision { + switch (choice) { + case 'approve': + return buildAllowDecision() + case 'approve-accept-edits': + return buildAllowDecision([{ type: 'setMode', mode: 'acceptEdits', destination: 'session' }]) + case 'approve-bypass': + return buildAllowDecision([{ type: 'setMode', mode: 'bypassPermissions', destination: 'session' }]) + case 'keep-planning': + return buildDenyDecision(feedback?.trim() || KEEP_PLANNING_MESSAGE) + } +} + +type Destination = FreshAgentPermissionUpdate['destination'] +type RuleUpdate = Extract + +/** Whether the update outlives this conversation (it is saved to a settings file). */ +function isSaved(destination: Destination): boolean { + return destination === 'localSettings' || destination === 'projectSettings' || destination === 'userSettings' +} + +function scopeText(destination: Destination): string { + switch (destination) { + case 'localSettings': return 'in this project (saved to .claude/settings.local.json)' + case 'projectSettings': return 'in this project (saved to .claude/settings.json)' + case 'userSettings': return 'in every project (saved to ~/.claude/settings.json)' + case 'session': + case 'cliArg': + return 'for the rest of this conversation' + } +} + +function rulesText(rules: RuleUpdate['rules']): string { + return rules.map((rule) => (rule.ruleContent ? `${rule.toolName}(${rule.ruleContent})` : rule.toolName)).join(', ') +} + +const BEHAVIOR_VERB: Record = { allow: 'allow', deny: 'deny', ask: 'ask before' } +const BEHAVIOR_LIST: Record = { allow: 'allowed', deny: 'denied', ask: 'ask-first' } + +const MODE_TEXT: Record['mode'], string> = { + acceptEdits: 'Auto-accept file edits', + bypassPermissions: 'Stop asking for permission', + default: 'Ask for permission as usual', + plan: 'Switch to plan mode', + dontAsk: 'Deny anything not already allowed, without asking', + auto: 'Let an automatic check decide what to allow', +} + +function capitalize(text: string): string { + return text.charAt(0).toUpperCase() + text.slice(1) +} + +/** One line, in plain language, saying what accepting `update` does and where it is kept. */ +export function describePermissionUpdate(update: FreshAgentPermissionUpdate): string { + const scope = scopeText(update.destination) + switch (update.type) { + case 'addRules': { + const verb = BEHAVIOR_VERB[update.behavior] + return `${isSaved(update.destination) ? `Always ${verb}` : capitalize(verb)} ${rulesText(update.rules)} ${scope}` + } + case 'replaceRules': + return `Make ${rulesText(update.rules)} the only ${BEHAVIOR_LIST[update.behavior]} rules ${scope}` + case 'removeRules': + return `Remove ${rulesText(update.rules)} from the ${BEHAVIOR_LIST[update.behavior]} rules ${scope}` + case 'setMode': + return `${MODE_TEXT[update.mode]} ${scope}` + case 'addDirectories': + return `${isSaved(update.destination) ? 'Always allow' : 'Allow'} access to ${update.directories.join(', ')} ${scope}` + case 'removeDirectories': + return `Remove access to ${update.directories.join(', ')} ${scope}` + } +} + +/** The remember button's label for these suggestions. */ +export function rememberLabel(updates: readonly FreshAgentPermissionUpdate[]): string { + const [only] = updates + return updates.length === 1 && only?.type === 'setMode' && only.mode === 'acceptEdits' + ? 'Allow and auto-accept edits' + : 'Allow and don\'t ask again' +} diff --git a/test/unit/client/lib/fresh-agent-permission-updates.test.ts b/test/unit/client/lib/fresh-agent-permission-updates.test.ts new file mode 100644 index 000000000..d6f4c9e42 --- /dev/null +++ b/test/unit/client/lib/fresh-agent-permission-updates.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it } from 'vitest' +import type { FreshAgentPermissionUpdate } from '@shared/fresh-agent-contract' +import { + buildAllowDecision, + buildDenyDecision, + buildPlanDecision, + describePermissionUpdate, + rememberLabel, +} from '@/lib/fresh-agent-permission-updates' + +const npmRule = (destination: string): FreshAgentPermissionUpdate => ({ + type: 'addRules', + rules: [{ toolName: 'Bash', ruleContent: 'npm test *' }], + behavior: 'allow', + destination, +} as FreshAgentPermissionUpdate) +const acceptEdits: FreshAgentPermissionUpdate = { type: 'setMode', mode: 'acceptEdits', destination: 'session' } + +describe('describePermissionUpdate', () => { + it('a project rule names the rule and the file it is saved to', () => { + const line = describePermissionUpdate(npmRule('localSettings')) + expect(line).toContain('Bash(npm test *)') + expect(line).toContain('.claude/settings.local.json') + }) + + it('a conversation rule names the rule and no file', () => { + const line = describePermissionUpdate(npmRule('session')) + expect(line).toContain('Bash(npm test *)') + expect(line).not.toMatch(/settings/) + }) + + it('each destination reads differently, and the shared and user files are named', () => { + const lines = ['localSettings', 'projectSettings', 'userSettings', 'session'].map((destination) => describePermissionUpdate(npmRule(destination))) + expect(new Set(lines).size).toBe(4) + expect(lines[1]).toContain('.claude/settings.json') + expect(lines[2]).toContain('~/.claude/settings.json') + }) + + it('a rule with no content names the whole tool, and several rules are all named', () => { + const line = describePermissionUpdate({ + type: 'addRules', + rules: [{ toolName: 'Read' }, { toolName: 'Grep', ruleContent: 'src/**' }], + behavior: 'allow', + destination: 'session', + }) + expect(line).toContain('Read') + expect(line).toContain('Grep(src/**)') + }) + + it('allow, deny and ask rules read differently', () => { + const lines = (['allow', 'deny', 'ask'] as const).map((behavior) => describePermissionUpdate({ ...npmRule('session'), behavior } as FreshAgentPermissionUpdate)) + expect(new Set(lines).size).toBe(3) + }) + + it('a mode change and a directory are described in their own words', () => { + const modes = (['acceptEdits', 'bypassPermissions', 'default', 'plan', 'dontAsk', 'auto'] as const) + .map((mode) => describePermissionUpdate({ type: 'setMode', mode, destination: 'session' })) + expect(new Set(modes).size).toBe(6) + const directory = describePermissionUpdate({ type: 'addDirectories', directories: ['/srv/data'], destination: 'session' }) + expect(directory).toContain('/srv/data') + expect(describePermissionUpdate({ type: 'removeDirectories', directories: ['/srv/data'], destination: 'session' })).not.toBe(directory) + }) + + it('replacing and removing rules read differently from adding them', () => { + const lines = (['addRules', 'replaceRules', 'removeRules'] as const) + .map((type) => describePermissionUpdate({ ...npmRule('localSettings'), type } as FreshAgentPermissionUpdate)) + expect(new Set(lines).size).toBe(3) + for (const line of lines) expect(line).toContain('Bash(npm test *)') + }) +}) + +describe('rememberLabel', () => { + it('names what the button does: auto-accepting edits when that is the only update, otherwise not asking again', () => { + expect(rememberLabel([acceptEdits])).toBe('Allow and auto-accept edits') + expect(rememberLabel([npmRule('localSettings')])).toBe('Allow and don\'t ask again') + expect(rememberLabel([npmRule('localSettings'), acceptEdits])).toBe('Allow and don\'t ask again') + }) +}) + +describe('decision builders', () => { + it('a plain allow states only the behavior: a defined updatedInput would replace the tool input', () => { + expect(buildAllowDecision()).toEqual({ behavior: 'allow' }) + expect(buildAllowDecision([])).toEqual({ behavior: 'allow' }) + }) + + it('an allow with updates sends exactly those updates', () => { + const updates = [npmRule('localSettings'), acceptEdits] + expect(buildAllowDecision(updates)).toEqual({ behavior: 'allow', updatedPermissions: updates }) + }) + + it('a deny keeps the turn going, with the user\'s message or the default one', () => { + expect(buildDenyDecision()).toEqual({ behavior: 'deny', message: 'Denied by user', interrupt: false }) + expect(buildDenyDecision(' not that file ')).toEqual({ behavior: 'deny', message: 'not that file', interrupt: false }) + expect(buildDenyDecision(' ')).toEqual({ behavior: 'deny', message: 'Denied by user', interrupt: false }) + }) + + it('each plan choice is its exact decision', () => { + expect(buildPlanDecision('approve')).toEqual({ behavior: 'allow' }) + expect(buildPlanDecision('approve-accept-edits')).toEqual({ + behavior: 'allow', + updatedPermissions: [{ type: 'setMode', mode: 'acceptEdits', destination: 'session' }], + }) + expect(buildPlanDecision('approve-bypass')).toEqual({ + behavior: 'allow', + updatedPermissions: [{ type: 'setMode', mode: 'bypassPermissions', destination: 'session' }], + }) + expect(buildPlanDecision('keep-planning', ' Split step 2 ')).toEqual({ behavior: 'deny', message: 'Split step 2', interrupt: false }) + expect(buildPlanDecision('keep-planning')).toEqual({ behavior: 'deny', message: 'The user wants to keep planning.', interrupt: false }) + expect(buildPlanDecision('keep-planning', ' ')).toEqual({ behavior: 'deny', message: 'The user wants to keep planning.', interrupt: false }) + }) +}) From c4f3284ac03fd6422c4bad9414210ab12ef0330b Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Fri, 23 Oct 2026 08:59:08 -0700 Subject: [PATCH 07/17] feat(fresh-agent): Claude's own "Allow and don't ask again" replaces the claude client-only bypass (D8, D10) The approval card answers through one onRespond(decision) built by the shared helpers. When the CLI suggested permission updates, it offers "Allow and don't ask again" (or "Allow and auto-accept edits" for a lone acceptEdits switch), spells each suggestion out underneath (the button's description), and sends exactly those suggestions as updatedPermissions, so Claude itself stops asking and enforces the scoped rule. The client-only bypass renders only when the caller passes onAlwaysAllow: the view does so for codex and opencode, relabelled "Always allow in this pane" with the tooltip "Until this pane reloads" (decision D3), and its auto-answer effect never runs for a claude pane, so no claude request is answered without a click. Co-Authored-By: Claude Opus 5.5 --- .../fresh-agent/FreshAgentApprovalCard.tsx | 76 ++++-- src/components/fresh-agent/FreshAgentView.tsx | 28 ++- .../FreshAgentApprovalCard.test.tsx | 128 +++++----- .../FreshAgentView.approvals.test.tsx | 221 ++++++++++++++++++ 4 files changed, 367 insertions(+), 86 deletions(-) create mode 100644 test/unit/client/components/fresh-agent/FreshAgentView.approvals.test.tsx diff --git a/src/components/fresh-agent/FreshAgentApprovalCard.tsx b/src/components/fresh-agent/FreshAgentApprovalCard.tsx index 3b8dc790b..148affa2d 100644 --- a/src/components/fresh-agent/FreshAgentApprovalCard.tsx +++ b/src/components/fresh-agent/FreshAgentApprovalCard.tsx @@ -1,32 +1,53 @@ +import { useId } from 'react' import DiffView from '@/components/fresh-agent/shared/DiffView' import type { FreshAgentPendingApproval } from '@shared/fresh-agent-contract' +import { + buildAllowDecision, + buildDenyDecision, + describePermissionUpdate, + rememberLabel, + type FreshAgentApprovalDecision, +} from '@/lib/fresh-agent-permission-updates' + +// Touch targets ≥44px at base; compact on sm+. flex-1 lets the buttons share +// the full row width on narrow panes for easy thumbing. +const BUTTON = 'fresh-agent-approval-button min-h-[2.75rem] flex-1 rounded-md px-4 text-sm disabled:cursor-not-allowed disabled:opacity-50 sm:min-h-0 sm:flex-none sm:px-3 sm:py-1 sm:text-xs' /** * Approval card with an inline preview of what is being approved — the exact - * Bash command, an Edit diff, or pretty-printed input for other tools — plus a - * session-scoped "always allow" escape hatch handled by the caller. + * Bash command, an Edit diff, or pretty-printed input for other tools. Every + * answer goes out through `onRespond` as a decision from the shared builders: + * + * - "Allow" and "Deny"; + * - when the CLI suggested permission updates (Claude only), the remember + * button sends exactly those suggestions, each spelled out in plain language + * underneath, so Claude itself stops asking; + * - `onAlwaysAllow`, when the caller provides it, adds the client-only "Always + * allow in this pane" (codex): the caller remembers the tool and + * answers later requests until the pane reloads. */ export function FreshAgentApprovalCard({ approval, disabled, - onAllow, + onRespond, onAlwaysAllow, - onDeny, }: { approval: FreshAgentPendingApproval disabled?: boolean - onAllow: () => void - onAlwaysAllow: (toolName: string) => void - onDeny: () => void + onRespond: (decision: FreshAgentApprovalDecision) => void + onAlwaysAllow?: (toolName: string) => void }) { + const suggestionsId = useId() const toolName = approval.toolName ?? 'Tool' const input = approval.input ?? {} + const suggestions = approval.suggestions ?? [] const command = toolName === 'Bash' && typeof input.command === 'string' ? input.command : null const editDiff = toolName === 'Edit' && typeof input.old_string === 'string' && typeof input.new_string === 'string' ? { oldStr: input.old_string, newStr: input.new_string, filePath: typeof input.file_path === 'string' ? input.file_path : undefined } : null + const remember = suggestions.length > 0 ? rememberLabel(suggestions) : null return (
) : null} - {/* Touch targets ≥44px at base; compact on sm+. flex-1 lets the buttons - share the full row width on narrow panes for easy thumbing. */}
- {approval.toolName ? ( + {remember ? ( + + ) : null} + {onAlwaysAllow && approval.toolName ? ( ) : null}
+ {remember ? ( +
    + {suggestions.map((update, index) => ( +
  • {describePermissionUpdate(update)}
    • + ))} +
+ ) : null}
) } diff --git a/src/components/fresh-agent/FreshAgentView.tsx b/src/components/fresh-agent/FreshAgentView.tsx index a2639a112..34bd9a7d9 100644 --- a/src/components/fresh-agent/FreshAgentView.tsx +++ b/src/components/fresh-agent/FreshAgentView.tsx @@ -126,6 +126,7 @@ import { useTranscriptLandings } from './useTranscriptLandings' import { landedTranscriptOf } from '@/lib/fresh-agent-landed-transcript' import { withSharedTurns } from '@/lib/fresh-agent-turn-sharing' import { formatFreshAgentNotice } from '@/lib/fresh-agent-notice' +import { buildAllowDecision, type FreshAgentApprovalDecision } from '@/lib/fresh-agent-permission-updates' import type { FreshAgentTurnRewindAvailability } from './FreshAgentTurnActions' import { FreshAgentComposer, type FreshAgentComposerHandle } from './FreshAgentComposer' import { FreshAgentDiffPanel } from './FreshAgentDiffPanel' @@ -1585,7 +1586,9 @@ export function FreshAgentView({ // RebindQueue). Jobs check this ref so a pane closed while its create job // was queued does not spawn a server session no pane owns. const isMountedRef = useRef(true) - // Session-scoped "always allow" tool names; reset with the pane, never persisted. + // The pane-scoped "Always allow in this pane" tool names (codex and + // opencode only: a claude pane answers through Claude's own suggestions and + // is never auto-answered here). Reset with the pane, never persisted. const alwaysAllowToolsRef = useRef>(new Set()) // Auto-title state tracks four things: // 1. whether this mounted pane has already consumed first-message auto-title, @@ -4211,13 +4214,13 @@ export function FreshAgentView({ setQueuedMessages((queue) => queue.slice(1)) }, [agentSession?.statusVersion, canSend, connectionStatus, hasRestoreFailure, isBusy, isRestoring, outgoingTurnVersion, ownerDivergence, queuedMessages, sendUserText]) - // Session-scoped auto-approval: any pending approval whose tool the user - // marked "always allow" is answered immediately. + // Pane-scoped auto-approval (not claude): any pending approval whose tool + // the user marked "Always allow in this pane" is answered at once. const pendingApprovalsFromSnapshot = snapshot?.pendingApprovals useEffect(() => { if (!pendingApprovalsFromSnapshot || pendingApprovalsFromSnapshot.length === 0) return const current = paneContentRef.current - if (!current.sessionId) return + if (!current.sessionId || current.provider === 'claude') return const cwd = getFreshOpenCodeRouteCwd(current, { sessionCwd: freshOpenCodeRouteCwdRef.current }) for (const approval of pendingApprovalsFromSnapshot) { if (approval.toolName && alwaysAllowToolsRef.current.has(approval.toolName)) { @@ -4228,9 +4231,7 @@ export function FreshAgentView({ provider: current.provider, ...(cwd ? { cwd } : {}), requestId: approval.requestId, - // A defined updatedInput (even {}) wholesale REPLACES the tool input server-side - // (sdk-bridge resolves the decision verbatim). Omit the key entirely. - decision: { behavior: 'allow' }, + decision: buildAllowDecision(), }) } } @@ -4469,7 +4470,7 @@ export function FreshAgentView({ ...(freshOpenCodeRouteCwd ? { cwd: freshOpenCodeRouteCwd } : {}), }) } - const respondToApproval = (requestId: string | number, allow: boolean) => { + const respondToApproval = (requestId: string | number, decision: FreshAgentApprovalDecision) => { dispatch(dismissTabGreen(tabId)) if (!paneContent.sessionId) return sendFreshAgentMessage({ @@ -4479,9 +4480,7 @@ export function FreshAgentView({ provider: paneContent.provider, ...(freshOpenCodeRouteCwd ? { cwd: freshOpenCodeRouteCwd } : {}), requestId, - decision: allow - ? { behavior: 'allow' } - : { behavior: 'deny', message: 'Denied by user', interrupt: false }, + decision, }) } @@ -4729,12 +4728,11 @@ export function FreshAgentView({ key={String(approval.requestId)} approval={approval} disabled={!paneContent.sessionId} - onAllow={() => respondToApproval(approval.requestId, true)} - onAlwaysAllow={(toolName) => { + onRespond={(decision) => respondToApproval(approval.requestId, decision)} + onAlwaysAllow={paneContent.provider === 'claude' ? undefined : (toolName) => { alwaysAllowToolsRef.current.add(toolName) - respondToApproval(approval.requestId, true) + respondToApproval(approval.requestId, buildAllowDecision()) }} - onDeny={() => respondToApproval(approval.requestId, false)} /> ))} {pendingQuestions.map((question) => ( diff --git a/test/unit/client/components/fresh-agent/FreshAgentApprovalCard.test.tsx b/test/unit/client/components/fresh-agent/FreshAgentApprovalCard.test.tsx index f178c8fdc..46f626788 100644 --- a/test/unit/client/components/fresh-agent/FreshAgentApprovalCard.test.tsx +++ b/test/unit/client/components/fresh-agent/FreshAgentApprovalCard.test.tsx @@ -1,78 +1,100 @@ import { afterEach, describe, expect, it, vi } from 'vitest' -import { cleanup, fireEvent, render, screen } from '@testing-library/react' +import { cleanup, fireEvent, render, screen, within } from '@testing-library/react' import { FreshAgentApprovalCard } from '@/components/fresh-agent/FreshAgentApprovalCard' +import type { FreshAgentPendingApproval } from '@shared/fresh-agent-contract' + +const NPM_RULE = { type: 'addRules', rules: [{ toolName: 'Bash', ruleContent: 'npm test *' }], behavior: 'allow', destination: 'localSettings' } as const +const READ_RULE = { type: 'addRules', rules: [{ toolName: 'Read', ruleContent: '//etc/**' }], behavior: 'allow', destination: 'session' } as const +const ACCEPT_EDITS = { type: 'setMode', mode: 'acceptEdits', destination: 'session' } as const +const SRV_DIRECTORY = { type: 'addDirectories', directories: ['/srv/data'], destination: 'session' } as const + +function renderCard(approval: FreshAgentPendingApproval, extra: { onAlwaysAllow?: (toolName: string) => void } = {}) { + const onRespond = vi.fn() + render() + return { onRespond } +} describe('FreshAgentApprovalCard', () => { afterEach(() => cleanup()) it('previews the exact Bash command being approved', () => { - render( - , - ) + renderCard({ requestId: 'req-1', toolName: 'Bash', input: { command: 'git push origin main' } }) expect(screen.getByRole('alert', { name: 'Permission request for Bash' })).toBeInTheDocument() expect(screen.getByText('git push origin main')).toBeInTheDocument() }) it('previews Edit approvals as a diff', () => { - render( - , - ) + renderCard({ requestId: 'req-2', toolName: 'Edit', input: { file_path: 'a.ts', old_string: 'old line', new_string: 'new line' } }) expect(screen.getByRole('figure', { name: 'diff view' })).toHaveAttribute('data-file-path', 'a.ts') }) - it('wires allow / always-allow / deny', () => { - const onAllow = vi.fn() - const onAlwaysAllow = vi.fn() - const onDeny = vi.fn() - render( - , - ) + it('Allow sends a plain allow and Deny a deny that keeps the turn going', () => { + const { onRespond } = renderCard({ requestId: 'req-3', toolName: 'Bash', input: { command: 'ls' } }) fireEvent.click(screen.getByRole('button', { name: 'Allow tool use' })) - fireEvent.click(screen.getByRole('button', { name: 'Always allow Bash this session' })) fireEvent.click(screen.getByRole('button', { name: 'Deny tool use' })) - expect(onAllow).toHaveBeenCalledTimes(1) + expect(onRespond.mock.calls).toEqual([ + [{ behavior: 'allow' }], + [{ behavior: 'deny', message: 'Denied by user', interrupt: false }], + ]) + }) + + it('offers Claude\'s own "don\'t ask again", one plain-language line per suggestion, and sends exactly those suggestions', () => { + const suggestions = [NPM_RULE, READ_RULE, ACCEPT_EDITS, SRV_DIRECTORY] + const { onRespond } = renderCard({ requestId: 'req-4', toolName: 'Bash', input: { command: 'npm test' }, suggestions: [...suggestions] }) + + const remember = screen.getByRole('button', { name: 'Allow and don\'t ask again' }) + const lines = within(screen.getByRole('list', { name: 'What "Allow and don\'t ask again" saves' })).getAllByRole('listitem') + expect(lines).toHaveLength(4) + expect(lines[0]).toHaveTextContent('Bash(npm test *)') + expect(lines[0]).toHaveTextContent('.claude/settings.local.json') + expect(lines[1]).toHaveTextContent('Read(//etc/**)') + expect(lines[1]).not.toHaveTextContent('settings') + expect(lines[3]).toHaveTextContent('/srv/data') + expect(remember).toHaveAccessibleDescription(lines.map((line) => line.textContent).join(' ')) + + fireEvent.click(remember) + expect(onRespond).toHaveBeenCalledWith({ behavior: 'allow', updatedPermissions: suggestions }) + }) + + it('a suggestion that only switches to accepting edits names the button for it', () => { + renderCard({ requestId: 'req-5', toolName: 'Write', input: { file_path: 'a.ts', content: 'x' }, suggestions: [ACCEPT_EDITS] }) + + expect(screen.getByRole('button', { name: 'Allow and auto-accept edits' })).toBeInTheDocument() + }) + + it('offers no remember button when the CLI suggested nothing', () => { + renderCard({ requestId: 'req-6', toolName: 'Bash', input: { command: 'ls /' }, blockedPath: '/' }) + + expect(screen.queryByRole('button', { name: /don't ask again|auto-accept/i })).not.toBeInTheDocument() + expect(screen.queryByRole('list')).not.toBeInTheDocument() + }) + + it('the pane-only "always allow" exists only when the caller provides it, says it lasts until the pane reloads, and reports the tool', () => { + renderCard({ requestId: 'req-7', toolName: 'Bash', input: { command: 'ls' } }) + expect(screen.queryByRole('button', { name: /always allow/i })).not.toBeInTheDocument() + cleanup() + + const onAlwaysAllow = vi.fn() + const { onRespond } = renderCard({ requestId: 'req-8', toolName: 'Bash', input: { command: 'ls' } }, { onAlwaysAllow }) + const button = screen.getByRole('button', { name: 'Always allow Bash in this pane' }) + expect(button).toHaveAttribute('title', 'Until this pane reloads') + fireEvent.click(button) expect(onAlwaysAllow).toHaveBeenCalledWith('Bash') - expect(onDeny).toHaveBeenCalledTimes(1) + expect(onRespond).not.toHaveBeenCalled() }) - it('keeps mobile and desktop touch targets available for approval actions', () => { - render( - , - ) - - expect(screen.getByRole('button', { name: 'Allow tool use' })).toBeEnabled() - expect(screen.getByRole('button', { name: 'Always allow Bash this session' })).toBeEnabled() - expect(screen.getByRole('button', { name: 'Deny tool use' })).toBeEnabled() + it('every action is disabled while the pane has no session', () => { + render() + + for (const button of screen.getAllByRole('button')) expect(button).toBeDisabled() }) + }) diff --git a/test/unit/client/components/fresh-agent/FreshAgentView.approvals.test.tsx b/test/unit/client/components/fresh-agent/FreshAgentView.approvals.test.tsx new file mode 100644 index 000000000..a5cf8879b --- /dev/null +++ b/test/unit/client/components/fresh-agent/FreshAgentView.approvals.test.tsx @@ -0,0 +1,221 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { act, cleanup, fireEvent, render, screen } from '@testing-library/react' +import { Provider } from 'react-redux' +import { configureStore } from '@reduxjs/toolkit' +import { appReducers } from '@/store/appReducers' +import { initLayout } from '@/store/panesSlice' +import { sessionCreated } from '@/store/freshAgentSlice' +import { setStatus } from '@/store/connectionSlice' +import { useAppSelector } from '@/store/hooks' +import { FreshAgentView } from '@/components/fresh-agent/FreshAgentView' +import { handleFreshAgentMessage } from '@/lib/fresh-agent-ws' +import { resetSnapshotSchedulerForTests, SNAPSHOT_DEBOUNCE_MS } from '@/lib/fresh-agent-snapshot-scheduler' + +// Client-alerts plan G20 (D10): how a pane answers approvals. A claude +// approval offers Claude's own "don't ask again" (its suggestions go back as +// updatedPermissions) and never a client-only bypass, so no claude request is +// ever answered without a click; codex keeps its pane-scoped "Always allow". + +const wsMock = vi.hoisted(() => ({ + handlers: new Set<(message: unknown) => void>(), + send: vi.fn(), +})) + +const apiMock = vi.hoisted(() => ({ + getFreshAgentThreadSnapshot: vi.fn(), +})) + +vi.mock('@/lib/ws-client', () => ({ + getWsClient: () => ({ + send: wsMock.send, + onMessage: (handler: (message: unknown) => void) => { + wsMock.handlers.add(handler) + return () => { wsMock.handlers.delete(handler) } + }, + onReconnect: () => () => {}, + }), +})) + +vi.mock('@/lib/api', async () => { + const actual = await vi.importActual('@/lib/api') + return { + ...actual, + api: { + ...actual.api, + get: vi.fn().mockRejectedValue(new Error('no server in tests')), + post: vi.fn().mockRejectedValue(new Error('no server in tests')), + }, + getFreshAgentThreadSnapshot: apiMock.getFreshAgentThreadSnapshot, + getFreshAgentCheckpointOutcomePage: vi.fn().mockResolvedValue({ outcomes: [], nextCursor: null }), + getFreshAgentModelCapabilities: vi.fn().mockRejectedValue(new Error('no server in tests')), + setSessionMetadata: vi.fn().mockResolvedValue(undefined), + } +}) + +vi.mock('@/store/settingsThunks', () => ({ + saveServerSettingsPatch: (patch: unknown) => ({ type: 'settings/saveServerSettingsPatch', payload: patch }), +})) + +type Locator = { sessionId: string; sessionType: 'freshclaude' | 'kilroy' | 'freshcodex'; provider: 'claude' | 'codex' } +const CLAUDE: Locator = { sessionId: '550e8400-e29b-41d4-a716-44665544a001', sessionType: 'freshclaude', provider: 'claude' } +const KILROY: Locator = { sessionId: '550e8400-e29b-41d4-a716-44665544a002', sessionType: 'kilroy', provider: 'claude' } +const CODEX: Locator = { sessionId: 'thread-approvals-1', sessionType: 'freshcodex', provider: 'codex' } + +const NPM_RULE = { type: 'addRules', rules: [{ toolName: 'Bash', ruleContent: 'npm test *' }], behavior: 'allow', destination: 'localSettings' } +const ACCEPT_EDITS = { type: 'setMode', mode: 'acceptEdits', destination: 'session' } + +type Approval = { requestId: string; toolName: string; input: Record; suggestions?: unknown[] } + +function snapshot(locator: Locator, pendingApprovals: Approval[], revision: number) { + return { + sessionType: locator.sessionType, + provider: locator.provider, + threadId: locator.sessionId, + sessionId: locator.sessionId, + revision, + latestTurnId: null, + status: 'running', + summary: '', + capabilities: { send: true, interrupt: true, approvals: true, questions: true, fork: false }, + tokenUsage: { inputTokens: 0, outputTokens: 0, totalTokens: 0 }, + pendingApprovals, + pendingQuestions: [], + worktrees: [], + diffs: [], + childThreads: [], + turns: [], + extensions: {}, + } +} + +/** Serve these pending sets in order (the last one repeats); every read is a fresh object. */ +function serve(locator: Locator, ...pendingSets: Approval[][]) { + const queue = pendingSets.map((pending, index) => () => snapshot(locator, pending, index + 1)) + apiMock.getFreshAgentThreadSnapshot.mockImplementation(async () => (queue.length > 1 ? queue.shift()! : queue[0])()) +} + +function createStore(locator: Locator) { + const store = configureStore({ + reducer: appReducers, + middleware: (getDefault) => getDefault({ serializableCheck: false, immutableCheck: false }), + }) + store.dispatch(setStatus('ready')) + store.dispatch(initLayout({ + tabId: 'tab-1', + paneId: 'pane-1', + content: { kind: 'fresh-agent', ...locator, createRequestId: 'req-approvals', status: 'running' }, + })) + // The session's record, as a create leaves it (the WS events fold into it). + store.dispatch(sessionCreated({ requestId: 'req-approvals', ...locator })) + return store +} + +type Store = ReturnType + +function StoreBackedView() { + const content = useAppSelector((state) => { + const layout = state.panes.layouts['tab-1'] + if (!layout || layout.type !== 'leaf' || layout.content.kind !== 'fresh-agent') throw new Error('missing pane') + return layout.content + }) + return +} + +async function advance(ms: number) { + await act(async () => { + await vi.advanceTimersByTimeAsync(ms) + }) +} + +async function mount(store: Store) { + render() + await advance(0) + await advance(SNAPSHOT_DEBOUNCE_MS) +} + +/** The server raises another approval: the pane refetches its snapshot. */ +async function raise(store: Store, locator: Locator, requestId: string) { + const message = { + type: 'freshAgent.event', + ...locator, + event: { type: 'freshAgent.permission.request', sessionId: locator.sessionId, requestId, subtype: 'can_use_tool' }, + } + act(() => { + handleFreshAgentMessage(store.dispatch, message) + for (const handler of [...wsMock.handlers]) handler(message) + }) + await advance(SNAPSHOT_DEBOUNCE_MS) + await advance(0) +} + +const responds = () => wsMock.send.mock.calls + .map(([message]) => message) + .filter((message) => message?.type === 'freshAgent.approval.respond') + +beforeEach(() => { + vi.useFakeTimers() + resetSnapshotSchedulerForTests() + wsMock.handlers.clear() + wsMock.send.mockReset() + apiMock.getFreshAgentThreadSnapshot.mockReset() +}) + +afterEach(() => { + cleanup() + resetSnapshotSchedulerForTests() + vi.useRealTimers() +}) + +describe('FreshAgentView approvals (client-alerts G20)', () => { + it.each([CLAUDE, KILROY])('a $sessionType "Allow and don\'t ask again" sends the CLI\'s suggestions as updatedPermissions', async (locator) => { + serve(locator, [{ requestId: 'req-1', toolName: 'Bash', input: { command: 'npm test' }, suggestions: [NPM_RULE, ACCEPT_EDITS] }]) + const store = createStore(locator) + await mount(store) + + fireEvent.click(screen.getByRole('button', { name: 'Allow and don\'t ask again' })) + expect(responds()).toEqual([{ + type: 'freshAgent.approval.respond', + sessionId: locator.sessionId, + sessionType: locator.sessionType, + provider: 'claude', + requestId: 'req-1', + decision: { behavior: 'allow', updatedPermissions: [NPM_RULE, ACCEPT_EDITS] }, + }]) + }) + + it('a claude pane has no client-only bypass: a second request for the same tool waits for a click', async () => { + const first = { requestId: 'req-1', toolName: 'Bash', input: { command: 'npm test' }, suggestions: [NPM_RULE] } + const second = { requestId: 'req-2', toolName: 'Bash', input: { command: 'npm test -- --watch' } } + serve(CLAUDE, [first], [second]) + const store = createStore(CLAUDE) + await mount(store) + + expect(screen.queryByRole('button', { name: /always allow/i })).not.toBeInTheDocument() + fireEvent.click(screen.getByRole('button', { name: 'Allow and don\'t ask again' })) + await raise(store, CLAUDE, 'req-2') + + expect(screen.getByRole('alert', { name: 'Permission request for Bash' })).toHaveTextContent('npm test -- --watch') + expect(responds().map((message) => message.requestId)).toEqual(['req-1']) + expect(screen.queryByRole('button', { name: /always allow/i })).not.toBeInTheDocument() + fireEvent.click(screen.getByRole('button', { name: 'Allow tool use' })) + expect(responds().at(-1)).toMatchObject({ requestId: 'req-2', decision: { behavior: 'allow' } }) + }) + + it('a codex pane keeps "Always allow in this pane": it answers the next request for that tool without a click', async () => { + serve( + CODEX, + [{ requestId: 'req-1', toolName: 'Bash', input: { command: 'ls' } }], + [{ requestId: 'req-2', toolName: 'Bash', input: { command: 'pwd' } }], + ) + const store = createStore(CODEX) + await mount(store) + + fireEvent.click(screen.getByRole('button', { name: 'Always allow Bash in this pane' })) + await raise(store, CODEX, 'req-2') + + expect(responds()).toEqual([ + expect.objectContaining({ provider: 'codex', requestId: 'req-1', decision: { behavior: 'allow' } }), + expect.objectContaining({ provider: 'codex', requestId: 'req-2', decision: { behavior: 'allow' } }), + ]) + }) +}) From daf201dac27aa4ab052373993d2726785440bb9a Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Fri, 23 Oct 2026 09:00:56 -0700 Subject: [PATCH 08/17] feat(fresh-agent): a plan card for Claude's ExitPlanMode (D9) ExitPlanMode is now reviewed as a plan instead of a tool call showing raw JSON. FreshAgentPlanApprovalCard (role=alert, "Plan approval request") renders input.plan as markdown, shows planFilePath, says so when no plan text came, and offers the four choices of decision D4: "Approve plan" (a plain allow, so the CLI returns to its pre-plan mode), "Approve and auto-accept edits" and "Approve and stop asking for permission" (a session setMode), and "Keep planning" with an optional note sent as the deny reason. Every choice is a 44 px touch target. The approval card hands ExitPlanMode requests to it. Co-Authored-By: Claude Opus 5.5 --- .../fresh-agent/FreshAgentApprovalCard.tsx | 18 ++- .../FreshAgentPlanApprovalCard.tsx | 110 ++++++++++++++++++ .../FreshAgentPlanApprovalCard.test.tsx | 93 +++++++++++++++ .../FreshAgentView.approvals.test.tsx | 14 +++ 4 files changed, 234 insertions(+), 1 deletion(-) create mode 100644 src/components/fresh-agent/FreshAgentPlanApprovalCard.tsx create mode 100644 test/unit/client/components/fresh-agent/FreshAgentPlanApprovalCard.test.tsx diff --git a/src/components/fresh-agent/FreshAgentApprovalCard.tsx b/src/components/fresh-agent/FreshAgentApprovalCard.tsx index 148affa2d..3bbe010f2 100644 --- a/src/components/fresh-agent/FreshAgentApprovalCard.tsx +++ b/src/components/fresh-agent/FreshAgentApprovalCard.tsx @@ -8,6 +8,7 @@ import { rememberLabel, type FreshAgentApprovalDecision, } from '@/lib/fresh-agent-permission-updates' +import { FreshAgentPlanApprovalCard } from './FreshAgentPlanApprovalCard' // Touch targets ≥44px at base; compact on sm+. flex-1 lets the buttons share // the full row width on narrow panes for easy thumbing. @@ -25,8 +26,23 @@ const BUTTON = 'fresh-agent-approval-button min-h-[2.75rem] flex-1 rounded-md px * - `onAlwaysAllow`, when the caller provides it, adds the client-only "Always * allow in this pane" (codex): the caller remembers the tool and * answers later requests until the pane reloads. + * + * Claude's ExitPlanMode request is a plan to review, not a tool call: it gets + * the plan card ({@link FreshAgentPlanApprovalCard}). */ -export function FreshAgentApprovalCard({ +export function FreshAgentApprovalCard(props: { + approval: FreshAgentPendingApproval + disabled?: boolean + onRespond: (decision: FreshAgentApprovalDecision) => void + onAlwaysAllow?: (toolName: string) => void +}) { + if (props.approval.toolName === 'ExitPlanMode') { + return + } + return +} + +function FreshAgentToolApprovalCard({ approval, disabled, onRespond, diff --git a/src/components/fresh-agent/FreshAgentPlanApprovalCard.tsx b/src/components/fresh-agent/FreshAgentPlanApprovalCard.tsx new file mode 100644 index 000000000..e09d6426d --- /dev/null +++ b/src/components/fresh-agent/FreshAgentPlanApprovalCard.tsx @@ -0,0 +1,110 @@ +import { useId, useState } from 'react' +import type { FreshAgentPendingApproval } from '@shared/fresh-agent-contract' +import { + buildPlanDecision, + type FreshAgentApprovalDecision, + type PlanApprovalChoice, +} from '@/lib/fresh-agent-permission-updates' +import { FreshAgentMarkdownBody } from './FreshAgentItemCard' + +// Touch targets ≥44px at base; compact on sm+ (the approval card's sizing). +const BUTTON = 'fresh-agent-plan-button min-h-[2.75rem] flex-1 rounded-md px-4 text-sm disabled:cursor-not-allowed disabled:opacity-50 sm:min-h-0 sm:flex-none sm:px-3 sm:py-1 sm:text-xs' + +const APPROVALS: Array<{ choice: Exclude; label: string; primary?: boolean }> = [ + { choice: 'approve', label: 'Approve plan', primary: true }, + { choice: 'approve-accept-edits', label: 'Approve and auto-accept edits' }, + { choice: 'approve-bypass', label: 'Approve and stop asking for permission' }, +] + +/** + * Claude's ExitPlanMode request, reviewed as a plan (client-alerts G20): the + * plan the CLI sent (`input.plan`, the plan file's text when the model wrote + * one) rendered as markdown, the file it lives in, and four answers. A plain + * approval lets the CLI return to the mode it had before plan mode; the other + * two set the mode for the rest of the conversation; "Keep planning" denies + * with the user's optional note. + */ +export function FreshAgentPlanApprovalCard({ + approval, + disabled, + onRespond, +}: { + approval: FreshAgentPendingApproval + disabled?: boolean + onRespond: (decision: FreshAgentApprovalDecision) => void +}) { + const feedbackId = useId() + const [feedback, setFeedback] = useState('') + const input = approval.input ?? {} + const plan = typeof input.plan === 'string' && input.plan.trim() ? input.plan : null + const planFilePath = typeof input.planFilePath === 'string' ? input.planFilePath : null + const answer = (choice: PlanApprovalChoice) => onRespond(buildPlanDecision(choice, feedback)) + + return ( +
+
+ + plan + + Plan ready for review +
+ {planFilePath ? ( +

+ {planFilePath} +

+ ) : null} +
+ {plan ? ( + + ) : ( +

No plan text was provided. You can still approve it or keep planning.

+ )} +
+ +
+ {APPROVALS.map(({ choice, label, primary }) => ( + + ))} +
+
+ +
+