From 31cad5c2066e368e55b87e834ba782c0c7dea6c1 Mon Sep 17 00:00:00 2001 From: dbarr5 Date: Fri, 9 Oct 2026 12:16:06 -0400 Subject: [PATCH] feat: carry denial feedback in tool approval results --- COMMANDS.md | 9 ++ README.md | 5 ++ src/commands/chat.ts | 29 +++++-- src/commands/code.ts | 35 +++++--- src/core/context.ts | 2 + src/core/tool_approval.ts | 51 +++++++++-- src/main.ts | 3 +- src/ui/approval_feedback.ts | 109 +++++++++++++++++++++++ test/approval_feedback.test.ts | 73 ++++++++++++++++ test/tool_approval.test.ts | 153 +++++++++++++++++++++++++++++++-- 10 files changed, 439 insertions(+), 30 deletions(-) create mode 100644 src/ui/approval_feedback.ts create mode 100644 test/approval_feedback.test.ts diff --git a/COMMANDS.md b/COMMANDS.md index eec2f3e5..8e0d5ebd 100644 --- a/COMMANDS.md +++ b/COMMANDS.md @@ -574,6 +574,15 @@ aether config set autoApply true | `autoApply` | bool | Apply streamed edits without a per-edit prompt. | | `telemetry` | bool | Anonymous usage telemetry opt-in. | +When an interactive coding run asks to execute a tool, answering no opens a +separate optional instruction field for that one declined call. Type a note +such as `Use the offline unit suite.`, then press Enter; an empty Enter skips +it, and Escape or Ctrl+C cancels the note. The note is limited to 512 UTF-8 +bytes and reaches the model once inside that call's failed tool result. It +never approves or runs the tool. The main draft, prompt history, queued tasks, +and reviewed shell attachment are untouched. Non-TTY decisions and `--yes` +retain their existing behavior and do not produce an instruction. + --- ## Slash commands (inside the REPL) diff --git a/README.md b/README.md index a88622e6..fb465519 100644 --- a/README.md +++ b/README.md @@ -155,6 +155,11 @@ Ctrl+R searches this workspace's submitted prompts newest first. Type to narrow matches, repeat Ctrl+R for older ones, Enter to insert a match for editing, or Escape to restore your draft. `AETHER_NO_HISTORY=1` disables prompt history. +If you decline a model-requested tool call, you can add one short instruction +for that denial, such as `Use the offline unit suite.` The tool stays blocked; +the instruction is returned once with its failed result. Enter skips the note, +and Escape cancels it. [Approval details](COMMANDS.md#aether-config-showgetset--local-settings). + Type `/` at an idle raw terminal prompt to browse described commands. Use arrows or Tab to choose, Enter to insert editable command text, then Enter again to run it; Escape restores your earlier draft. diff --git a/src/commands/chat.ts b/src/commands/chat.ts index 8bf40854..0bc7a1de 100644 --- a/src/commands/chat.ts +++ b/src/commands/chat.ts @@ -68,7 +68,7 @@ import { ConsoleQueue, describeEntry, entryKind, parseQueueCommand, renderDispos import { HostRenderer } from "../ui/host_render.js"; import type { TaskCommand } from "../core/brain.js"; import { getRegistry } from "../core/context_registry.js"; -import { prepareToolApproval, requestToolApproval, terminalSafeReview } from "../core/tool_approval.js"; +import { bindToolApprovalVerdict, deniedToolResult, prepareToolApproval, requestToolApproval, terminalSafeReview, type ToolApprovalVerdict } from "../core/tool_approval.js"; import { openRunSession, refusalToolResult } from "../core/skills/run_session.js"; import { TRANSIENT_READ_AUTO_RETRIES, @@ -859,25 +859,29 @@ export async function runLocalTurn( }); const brain = deps.brain ?? new OllamaBrain({ model, ...(deps.advertisedTools ? { tools: deps.advertisedTools } : {}) }); const exec = deps.exec ?? new ToolExecutor(cwd); + const seenCallIds = new Set(); + const controller = new AbortController(); const renderer = new HostRenderer({ poolGb: 5, json: ctx.flags.json }); const pulse = new ThinkingPulse({ enabled: Boolean(process.stderr.isTTY) && !ctx.flags.json && process.env["AETHER_NO_ANIM"] !== "1", write: (s) => process.stderr.write(errTheme.dim(s)), onPaint: deps.onPulsePaint, }); - const approveTool = async (name: string, args: Record): Promise => { + const approveTool = async (callId: string, name: string, args: Record): Promise => { let patchPreview: string | undefined; if (name === "patch_file" && exec.previewPatch) { const preview = exec.previewPatch(args); process.stderr.write(terminalSafeReview(preview.output) + "\n"); - if (preview.exitCode !== 0) return true; + if (preview.exitCode !== 0) return { callId, approved: true }; patchPreview = preview.output; } return requestToolApproval({ + callId, name, args: args as Record, permissionMode: ctx.cfg.permissionMode, autoApply: ctx.cfg.autoApply, yes: ctx.flags.yes, isTty: Boolean(process.stdin.isTTY), shellCwd: exec.shellCwd ?? cwd, fileRoot: cwd, confirm: ctx.confirm, + ...(ctx.approvalFeedback ? { feedback: () => ctx.approvalFeedback!(controller.signal) } : {}), patchPreview, onDeny: () => process.stderr.write(`blocked ${name}: confirmation required; use --yes or permissionMode skip\n`), }); @@ -893,7 +897,6 @@ export async function runLocalTurn( let partialOutput = false; let terminalError: ChatTurnError | null = null; const timeoutMs = deps.meaningfulProgressTimeoutMs ?? defaultStreamTimeoutMs(); - const controller = new AbortController(); const forwardAbort = (): void => controller.abort(signal?.reason ?? new DOMException("turn cancelled", "AbortError")); const closeBrain = (): void => { try { brain.close(); } catch { /* cleanup cannot replace the primary outcome */ } @@ -953,6 +956,8 @@ export async function runLocalTurn( break; } if (ev.type === "tool_call") { + if (seenCallIds.has(ev.id)) continue; // one result and no second execution for one call ID + seenCallIds.add(ev.id); noteWaitingForTool(lifecycle); renderer.event(ev); // Same ordering as hostLoop (commands/code.ts): the skill narrowing is @@ -1028,13 +1033,13 @@ export async function runLocalTurn( continue; } const approvalContext = exec.shellContext; - const approved = await boundedLocalOperation( - () => approveTool(ev.name, prepared.args), + const approval = bindToolApprovalVerdict(ev.id, await boundedLocalOperation( + () => approveTool(ev.id, ev.name, prepared.args), controller.signal, timeoutMs, lastMeaningfulAt, timeout, - ); + )); const execute = (): Promise => { const remaining = timeoutMs > 0 ? Math.max(1, timeoutMs - (Date.now() - lastMeaningfulAt)) @@ -1053,8 +1058,8 @@ export async function runLocalTurn( timeout, ); }; - if (!approved) { - deliver({ output: `[tool ${ev.name} blocked: permission denied]`, exitCode: 1 }, "approval"); + if (!approval.approved) { + deliver(deniedToolResult(ev.name, approval), "approval"); } else if (!supersededBySteer()) { // Approval can take a while; a steer accepted during the prompt is // checked above, so an approved-but-stale write still never runs. @@ -1330,6 +1335,12 @@ export async function repl(ctx: AppContext, skillOpts: TurnSkillOptions = {}): P try { return await ctx.confirm(question); } finally { approvalOwnsInput = false; } }, + approvalFeedback: async (signal) => { + buf.endRecoveryScope(); + approvalOwnsInput = true; + try { return await ctx.approvalFeedback?.(signal) ?? null; } + finally { approvalOwnsInput = false; } + }, }; const renderHudLine = (): void => { if (!process.stdout.isTTY) return; diff --git a/src/commands/code.ts b/src/commands/code.ts index 2b9c77e0..2a9f0c0e 100644 --- a/src/commands/code.ts +++ b/src/commands/code.ts @@ -57,7 +57,7 @@ import { parseRepoSpec, ensureLocalClone, type RepoSpec } from "../core/repo.js" import { chooseBackend, chooseLocalBrain } from "../core/backend.js"; import { ModelTextProgress } from "../core/model_text_progress.js"; import { ModelOutputBudget } from "../core/model_output_budget.js"; -import { prepareToolApproval, requestToolApproval, terminalSafeReview } from "../core/tool_approval.js"; +import { bindToolApprovalVerdict, deniedToolResult, prepareToolApproval, requestToolApproval, terminalSafeReview, type ToolApprovalVerdict } from "../core/tool_approval.js"; import { openRunSession, refusalToolResult } from "../core/skills/run_session.js"; import type { SessionContext } from "../core/session_resume.js"; import type { SkillSessionProvenance } from "../core/skills/skill_session.js"; @@ -159,7 +159,7 @@ export function codeMeaningfulProgressTimeoutMs( } /** Approve (or refuse) one brain-emitted tool call before the host executes it. */ -export type ToolGate = (call: { name: string; args: Record }) => Promise; +export type ToolGate = (call: { id: string; name: string; args: Record }) => Promise; /** Read-only correlation hooks for an explicit saved-goal phase run. */ export interface CodeRunStarted { @@ -1116,33 +1116,36 @@ export async function cmdCode( if (result.exitCode !== 0 && hostRefusals.length < 16 && (/^\[(?:denied:|refused by host policy:|tool .* rejected:|unknown tool:)/i.test(result.output) || result.exitCode === 127)) { - hostRefusals.push(redactInline(sanitizeServerText(result.output)).slice(0, 240)); + hostRefusals.push(redactInline(sanitizeServerText(result.output.split("\n", 1)[0]!)).slice(0, 240)); } }; let confirmToolReview = ctx.confirm; + let confirmToolFeedback = ctx.approvalFeedback; // Permission gate: every brain-emitted mutating/shell tool call is approved // here before the host runs it. Honors the configured permission mode + auto- // apply; in `ask` (the default) on a TTY the user gets a y/N prompt, and on a // non-TTY (CI/pipe) an un-pre-approved call FAILS CLOSED rather than running // unattended. `--yes` or `permissionMode: skip` opt out. - const gate: ToolGate = async ({ name, args }) => { + const gate: ToolGate = async ({ id, name, args }) => { if (opts.forbidPublication && isPublicationToolCall(name, args)) { process.stderr.write("✗ publishing is a separate ship action; this goal phase tool call was refused.\n"); - return false; + return { callId: id, approved: false }; } let patchPreview: string | undefined; if (name === "patch_file") { const preview = exec.previewPatch(args); process.stderr.write(terminalSafeReview(preview.output) + "\n"); - if (preview.exitCode !== 0) return true; // executor returns the same conflict to the brain + if (preview.exitCode !== 0) return { callId: id, approved: true }; // executor returns the same conflict to the brain patchPreview = preview.output; } return requestToolApproval({ + callId: id, name, args: args as Record, permissionMode: ctx.cfg.permissionMode, autoApply: ctx.cfg.autoApply, yes: ctx.flags.yes, isTty: Boolean(process.stdin.isTTY), shellCwd: exec.shellCwd, fileRoot: cwd, confirm: confirmToolReview, + ...(confirmToolFeedback ? { feedback: () => confirmToolFeedback!(commandAbort.signal) } : {}), patchPreview, onDeny: () => process.stderr.write( `✗ blocked ${name} — permission mode "${ctx.cfg.permissionMode}" needs confirmation but there is no TTY.\n` + @@ -1191,6 +1194,14 @@ export async function cmdCode( try { return await ctx.confirm(review); } finally { resume(); } }; + if (confirmToolFeedback) { + const requestFeedback = confirmToolFeedback; + confirmToolFeedback = async (signal) => { + const resume = sr.pauseForPrompt(); + try { return await requestFeedback(signal); } + finally { resume(); } + }; + } replay((line) => sr.log(line)); const anim = new AnimationController({ onFrame: (_stage, art) => sr.setAnim(art), @@ -1523,6 +1534,7 @@ export async function hostLoop( const timeoutMs = options.meaningfulProgressTimeoutMs ?? DEFAULT_CODE_MEANINGFUL_PROGRESS_TIMEOUT_MS; const segmentTimeoutMs = options.modelSegmentTimeoutMs ?? codeModelSegmentTimeoutMs(); const signal = options.signal; + const seenCallIds = new Set(); const failures = options.failureBudget === false ? null : (options.failureBudget ?? defaultToolFailureBudget(task.cwd, exec)); let lastMeaningfulAt = Date.now(); @@ -1555,6 +1567,8 @@ export async function hostLoop( let terminal = false; switch (ev.type) { case "tool_call": { + if (seenCallIds.has(ev.id)) break; // never re-execute or re-deliver one call ID + seenCallIds.add(ev.id); // The host owns execution + the path-guard. A tool call is gated FIRST // (permission mode); a denied call is never executed — the brain gets a // synthetic refusal result so the loop continues without running it. @@ -1628,9 +1642,9 @@ export async function hostLoop( break; } const approvalContext = exec.shellContext; - const approved = gate + const gateDecision = gate ? await boundedCodeOperation( - () => gate({ name: ev.name, args: prepared.args }), + () => gate({ id: ev.id, name: ev.name, args: prepared.args }), timeoutMs, lastMeaningfulAt, signal, @@ -1638,8 +1652,9 @@ export async function hostLoop( modelSegmentStartedAt, ) : true; - if (!approved) { - deliver({ output: `[denied: ${ev.name} not approved by user]`, exitCode: 1 }, "approval"); + const approval = bindToolApprovalVerdict(ev.id, gateDecision); + if (!approval.approved) { + deliver(deniedToolResult(ev.name, approval), "approval"); break; } const execute = (): Promise => { diff --git a/src/core/context.ts b/src/core/context.ts index d7c6465f..427376dc 100644 --- a/src/core/context.ts +++ b/src/core/context.ts @@ -51,4 +51,6 @@ export interface AppContext { /** Ask the user a yes/no question (readline y/N; `--yes` short-circuits). * Injected so slash-command confirmations are testable. */ confirm: (q: string) => Promise; + /** One declined tool call's optional instruction; a separate input lease. */ + approvalFeedback?: (signal?: AbortSignal) => Promise; } diff --git a/src/core/tool_approval.ts b/src/core/tool_approval.ts index 7d6fb167..576899b0 100644 --- a/src/core/tool_approval.ts +++ b/src/core/tool_approval.ts @@ -57,10 +57,47 @@ export function formatToolApprovalReview( const diff = name === "patch_file" && patchPreview ? `\nAffected diff (escaped for terminal safety):\n${terminalSafeReview(patchPreview)}\n` : ""; - return `\nModel-requested tool approval\nTool: ${name}\nShell cwd: ${terminalSafeReview(shellCwd, false)}\nFile root: ${terminalSafeReview(fileRoot, false)}\n${field} (numbered rows preserve line breaks; controls and backslashes are escaped):\n${lines}${diff}\nRun this exact tool call? [y/N] `; + return `\nModel-requested tool approval\nTool: ${name}\nShell cwd: ${terminalSafeReview(shellCwd, false)}\nFile root: ${terminalSafeReview(fileRoot, false)}\n${field} (numbered rows preserve line breaks; controls and backslashes are escaped):\n${lines}${diff}\nDeclining offers an optional one-call instruction.\nRun this exact tool call? [y/N] `; +} + +export const MAX_DENIAL_FEEDBACK_BYTES = 512; + +/** Feedback is inert tool-result text, never a command or an approval. */ +export function boundedDenialFeedback(raw: string | null | undefined): string | null { + if (!raw) return null; + const cleaned = raw.replace(/[\r\n\t]+/g, " ") + .replace(/[\x00-\x1f\x7f-\x9f\u2028\u2029\p{Cf}]/gu, "").trim(); + let bounded = ""; + for (const character of cleaned) { + if (Buffer.byteLength(bounded + character, "utf8") > MAX_DENIAL_FEEDBACK_BYTES) break; + bounded += character; + } + return bounded || null; +} + +export interface ToolApprovalVerdict { + readonly callId: string; + readonly approved: boolean; + /** Present only for a declined, interactive call. */ + readonly feedback?: string; +} + +/** Existing embedders may still return a boolean; bind it to this exact call. */ +export function bindToolApprovalVerdict(callId: string, decision: ToolApprovalVerdict | boolean): ToolApprovalVerdict { + if (typeof decision === "boolean") return { callId, approved: decision }; + if (decision.callId !== callId) return { callId, approved: false }; + if (decision.approved) return { callId, approved: true }; + const feedback = boundedDenialFeedback(decision.feedback); + return { callId, approved: false, ...(feedback ? { feedback } : {}) }; +} + +export function deniedToolResult(name: string, verdict: ToolApprovalVerdict): { output: string; exitCode: number } { + const base = `[denied: ${name} not approved by user]`; + return { output: verdict.feedback ? `${base}\nOperator instruction for this denied call: ${verdict.feedback}` : base, exitCode: 1 }; } export interface ToolApprovalRequest { + callId: string; name: string; args: ValidatedToolArgs; permissionMode: PermissionMode; @@ -71,20 +108,24 @@ export interface ToolApprovalRequest { fileRoot: string; patchPreview?: string; confirm: (review: string) => Promise; + feedback?: () => Promise; onDeny: () => void; } -export async function requestToolApproval(request: ToolApprovalRequest): Promise { +export async function requestToolApproval(request: ToolApprovalRequest): Promise { const outcome = decideGate(request.name, request.permissionMode, request.autoApply, { yes: request.yes, isTty: request.isTty, }); - if (outcome === "allow") return true; + if (outcome === "allow") return { callId: request.callId, approved: true }; if (outcome === "deny") { request.onDeny(); - return false; + return { callId: request.callId, approved: false }; } - return request.confirm(formatToolApprovalReview( + const approved = await request.confirm(formatToolApprovalReview( request.name, request.args, request.shellCwd, request.fileRoot, request.patchPreview, )); + if (approved) return { callId: request.callId, approved: true }; + const feedback = boundedDenialFeedback(await request.feedback?.()); + return { callId: request.callId, approved: false, ...(feedback ? { feedback } : {}) }; } diff --git a/src/main.ts b/src/main.ts index c9d23dcb..5b08715a 100644 --- a/src/main.ts +++ b/src/main.ts @@ -21,6 +21,7 @@ import { cmdRun } from "./commands/run.js"; import { cmdCode } from "./commands/code.js"; import { promptFileConflict, readPromptFile } from "./commands/prompt_file.js"; import { errTheme } from "./ui/theme.js"; +import { promptDenialFeedback } from "./ui/approval_feedback.js"; // VERSION is imported ONCE from the generated version.js — main.ts must never // hardcode a duplicate version string that can drift from package.json. import { VERSION } from "./version.js"; @@ -179,7 +180,7 @@ export async function main(argv: string[]): Promise { }); const ctx: AppContext = { cfg, api, tokens, driveStaffTokens: new FileTokenStore(".drive-staff-session"), - flags, confirm, + flags, confirm, approvalFeedback: (signal) => promptDenialFeedback({ signal }), }; const loginOpts: LoginOpts = { diff --git a/src/ui/approval_feedback.ts b/src/ui/approval_feedback.ts new file mode 100644 index 00000000..47c8bfaa --- /dev/null +++ b/src/ui/approval_feedback.ts @@ -0,0 +1,109 @@ +// Short-lived input lease for a declined tool call. It uses its own InputBuffer +// and never touches the composer, prompt history, queue, or reviewed shell item. +import { StringDecoder } from "node:string_decoder"; +import type { Readable, Writable } from "node:stream"; +import { MAX_DENIAL_FEEDBACK_BYTES, boundedDenialFeedback } from "../core/tool_approval.js"; +import { InputBuffer } from "./input_line.js"; +import { renderInputView } from "./input_render.js"; +import { decodeKey, splitKeys } from "./keys.js"; + +export interface ApprovalFeedbackIO { + input?: Readable & { isRaw?: boolean; setRawMode?: (raw: boolean) => unknown }; + output?: Writable; + signal?: AbortSignal; +} + +function printable(fragment: string): string { + return fragment.replace(/[\r\n\t]+/g, " ") + .replace(/[\x00-\x1f\x7f-\x9f\u2028\u2029\p{Cf}]/gu, ""); +} + +/** Enter skips an empty note; Escape/Ctrl+C cancel the note, not the denial. */ +export function promptDenialFeedback(io: ApprovalFeedbackIO = {}): Promise { + const input = io.input ?? process.stdin; + const output = io.output ?? process.stderr; + if (io.signal?.aborted) return Promise.resolve(null); + const buffer = new InputBuffer(); + const decoder = new StringDecoder("utf8"); + const priorRaw = Boolean(input.isRaw); + const wasPaused = input.isPaused(); + let changedRaw = false; + if (!priorRaw && input.setRawMode) { + try { input.setRawMode(true); } + catch { return Promise.resolve(null); } + changedRaw = true; + } + const label = `Declined. Optional instruction (${MAX_DENIAL_FEEDBACK_BYTES} bytes; Enter skips, Esc cancels): `; + const paint = (): void => { + const view = renderInputView(label, buffer.value, buffer.pos, + (output as Writable & { columns?: number }).columns ?? 80); + output.write(`\r\x1b[2K${view.text}\x1b[${view.cursorCol}G`); + }; + paint(); + return new Promise((resolve) => { + let settled = false; + let pasting = false; + let paste = ""; + const finish = (value: string | null): void => { + if (settled) return; + settled = true; + input.off("data", onData); + io.signal?.removeEventListener("abort", onAbort); + if (changedRaw) { try { input.setRawMode?.(false); } catch { /* terminal may have closed */ } } + if (wasPaused) input.pause(); + output.write("\n"); + resolve(boundedDenialFeedback(value)); + }; + const onAbort = (): void => finish(null); + const fit = (base: string, raw: string): string => { + let accepted = ""; + for (const char of printable(raw)) { + if (Buffer.byteLength(base + accepted + char, "utf8") > MAX_DENIAL_FEEDBACK_BYTES) break; + accepted += char; + } + return accepted; + }; + const insert = (raw: string): void => { + const accepted = fit(buffer.value, raw); + if (accepted) buffer.insert(accepted); + paint(); + }; + const onData = (chunk: Buffer | string): void => { + const sequences = splitKeys(typeof chunk === "string" ? chunk : decoder.write(chunk)); + const returnTail = (index: number): void => { + const tail = sequences.slice(index + 1).join(""); + if (tail) setImmediate(() => input.emit("data", Buffer.from(tail, "utf8"))); + }; + for (const [index, seq] of sequences.entries()) { + if (seq === "\x03") { finish(null); returnTail(index); return; } + const key = decodeKey(seq); + if (pasting) { + if (key.kind === "paste-end") { pasting = false; insert(paste); paste = ""; } + else paste += fit(buffer.value + paste, seq); + continue; + } + switch (key.kind) { + case "paste-start": pasting = true; paste = ""; break; + case "char": insert(key.value); break; + case "backspace": buffer.backspace(); paint(); break; + case "delete": buffer.deleteForward(); paint(); break; + case "left": buffer.left(); paint(); break; + case "right": buffer.right(); paint(); break; + case "home": buffer.home(); paint(); break; + case "end": buffer.end(); paint(); break; + case "undo": buffer.undo(); paint(); break; + case "submit": + case "newline": finish(buffer.value); returnTail(index); return; + case "escape": + case "interrupt": + case "eof": finish(null); returnTail(index); return; + default: break; + } + } + }; + input.on("data", onData); + io.signal?.addEventListener("abort", onAbort, { once: true }); + if (io.signal?.aborted) onAbort(); + if (!settled) input.resume(); + }); +} diff --git a/test/approval_feedback.test.ts b/test/approval_feedback.test.ts new file mode 100644 index 00000000..fbc2a8cb --- /dev/null +++ b/test/approval_feedback.test.ts @@ -0,0 +1,73 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { PassThrough } from "node:stream"; +import { promptDenialFeedback } from "../src/ui/approval_feedback.js"; +import { InputBuffer } from "../src/ui/input_line.js"; +import { MAX_DENIAL_FEEDBACK_BYTES } from "../src/core/tool_approval.js"; + +function lease() { + const input = new PassThrough() as PassThrough & { isRaw?: boolean; setRawMode?: (raw: boolean) => void }; + const modes: boolean[] = []; + input.isRaw = false; + input.setRawMode = raw => { input.isRaw = raw; modes.push(raw); }; + const output = new PassThrough(); + let text = ""; + output.on("data", chunk => { text += String(chunk); }); + return { input, output, modes, text: () => text }; +} + +test("feedback owns a separate short-lived buffer and restores the prior raw mode", async () => { + const main = new InputBuffer(); + main.insert("queued draft"); + main.left(); + const before = { value: main.value, cursor: main.pos }; + const queued = ["pending task"]; + const io = lease(); + const pending = promptDenialFeedback(io); + io.input.write("Use offlien"); + io.input.write("\x7f\x7fne suite\r"); + assert.equal(await pending, "Use offline suite"); + assert.deepEqual(io.modes, [true, false]); + assert.deepEqual({ value: main.value, cursor: main.pos }, before); + assert.deepEqual(queued, ["pending task"]); + assert.match(io.text(), /Optional instruction/); +}); + +test("Enter skips, Escape cancels, and a pasted note cannot exceed the byte cap", async () => { + const skipped = lease(); + const first = promptDenialFeedback(skipped); + skipped.input.write("\r"); + assert.equal(await first, null); + const cancelled = lease(); + const second = promptDenialFeedback(cancelled); + cancelled.input.write("temporary\x1b"); + assert.equal(await second, null); + const pasted = lease(); + const third = promptDenialFeedback(pasted); + pasted.input.write("\x1b[200~" + "é".repeat(10_000) + "\x1b[201~\r"); + const result = await third; + assert.equal(Buffer.byteLength(result!, "utf8"), MAX_DENIAL_FEEDBACK_BYTES); + assert.equal(pasted.input.listenerCount("data"), 0); +}); + +test("input batched after feedback submission returns to the next owner", async () => { + const io = lease(); + const pending = promptDenialFeedback(io); + io.input.write("Use offline tests\rqueued draft"); + assert.equal(await pending, "Use offline tests"); + let handedBack = ""; + io.input.on("data", chunk => { handedBack += String(chunk); }); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(handedBack, "queued draft"); +}); + +test("turn cancellation releases the feedback lease without a note", async () => { + const io = lease(); + const controller = new AbortController(); + const pending = promptDenialFeedback({ ...io, signal: controller.signal }); + io.input.write("unfinished note"); + controller.abort(); + assert.equal(await pending, null); + assert.deepEqual(io.modes, [true, false]); + assert.equal(io.input.listenerCount("data"), 0); +}); diff --git a/test/tool_approval.test.ts b/test/tool_approval.test.ts index 52649f7e..1c222382 100644 --- a/test/tool_approval.test.ts +++ b/test/tool_approval.test.ts @@ -4,11 +4,19 @@ import { existsSync, readFileSync, rmSync } from "node:fs"; import { join } from "node:path"; import { hostLoop } from "../src/commands/code.js"; import { runLocalTurn } from "../src/commands/chat.js"; +import { CloudBrain } from "../src/core/brain_cloud.js"; +import { ApiClient } from "../src/core/transport.js"; import type { Brain, TaskCommand } from "../src/core/brain.js"; import type { BrainEvent } from "../src/core/brain_protocol.js"; import type { AppContext } from "../src/core/context.js"; +import type { TokenStore } from "../src/core/auth.js"; import { ToolExecutor, type ToolResult } from "../src/core/tool_executor.js"; +import { ToolFailureBudget, operationKey } from "../src/core/tool_failure_budget.js"; import { + MAX_DENIAL_FEEDBACK_BYTES, + bindToolApprovalVerdict, + boundedDenialFeedback, + deniedToolResult, formatToolApprovalReview, prepareToolApproval, requestToolApproval, @@ -64,32 +72,72 @@ test("shared gate shows the whole command and denial does not execute", async () let reviewed = ""; let executed = 0; const approved = await requestToolApproval({ + callId: "reviewed", name: "run_shell", args: prepared.args, permissionMode: "ask", autoApply: false, yes: false, isTty: true, shellCwd: "/cwd", fileRoot: "/root", confirm: async (text) => { reviewed = text; return false; }, onDeny: () => assert.fail("TTY must prompt"), }); - if (approved) executed++; + if (approved.approved) executed++; assert.equal(executed, 0); assert.ok(reviewed.includes("SUFFIX_AFTER_200")); + assert.match(reviewed, /optional one-call instruction/); let prompted = false; const nonTty = await requestToolApproval({ + callId: "non-tty", name: "run_shell", args: prepared.args, permissionMode: "ask", autoApply: false, yes: false, isTty: false, shellCwd: "/cwd", fileRoot: "/root", confirm: async () => { prompted = true; return true; }, onDeny: () => {}, }); - assert.equal(nonTty, false); + assert.deepEqual(nonTty, { callId: "non-tty", approved: false }); assert.equal(prompted, false); const preapproved = await requestToolApproval({ + callId: "preapproved", name: "run_shell", args: prepared.args, permissionMode: "ask", autoApply: false, yes: true, isTty: false, shellCwd: "/cwd", fileRoot: "/root", - confirm: async () => { prompted = true; return false; }, onDeny: () => {}, + confirm: async () => { prompted = true; return false; }, + feedback: async () => assert.fail("--yes cannot request denial feedback"), onDeny: () => {}, }); - assert.equal(preapproved, true); + assert.deepEqual(preapproved, { callId: "preapproved", approved: true }); assert.equal(prompted, false); }); +test("a declined call carries one bounded instruction under its original ID", async () => { + const prepared = prepareToolApproval("run_tests", { command: "npm test" }); + assert.equal(prepared.ok, true); + if (!prepared.ok) return; + const verdict = await requestToolApproval({ + callId: "call-42", name: "run_tests", args: prepared.args, + permissionMode: "ask", autoApply: false, yes: false, isTty: true, + shellCwd: ".", fileRoot: ".", confirm: async () => false, + feedback: async () => "Use the offline unit suite.", onDeny: () => assert.fail("interactive"), + }); + assert.deepEqual(verdict, { callId: "call-42", approved: false, feedback: "Use the offline unit suite." }); + assert.match(deniedToolResult("run_tests", verdict).output, /Operator instruction for this denied call: Use the offline unit suite\./); + assert.deepEqual(bindToolApprovalVerdict("other-call", verdict), { callId: "other-call", approved: false }); + assert.deepEqual(bindToolApprovalVerdict("call-42", { callId: "call-42", approved: true, feedback: "should disappear" }), + { callId: "call-42", approved: true }); + assert.equal(boundedDenialFeedback(" "), null); + assert.equal(Buffer.byteLength(boundedDenialFeedback("é".repeat(400))!, "utf8"), MAX_DENIAL_FEEDBACK_BYTES); + assert.equal(boundedDenialFeedback("!echo ok\n/clear\x1b[2J"), "!echo ok /clear[2J"); +}); + +test("denial feedback does not reset or widen the repeated-failure budget", () => { + const call = { name: "run_shell", args: { command: "echo denied" } }; + const key = operationKey(call); + const budget = new ToolFailureBudget({ budgets: { permission_refused: 1 } }); + budget.noteModelRound(1); + assert.equal(budget.check(key, call).action, "allow"); + budget.record(key, call, deniedToolResult(call.name, { callId: "first", approved: false, feedback: "Try offline." }), "approval"); + budget.noteModelRound(2); + const repeated = budget.check(key, call); + assert.equal(repeated.action, "refuse"); + if (repeated.action === "refuse") assert.doesNotMatch(repeated.result.output, /Try offline/); + budget.noteModelRound(3); + assert.equal(budget.check(key, call).action, "stop"); +}); + test("configured run_tests command is the exact reviewed and executed command", () => { const fallback = `${longCommand}\necho configured suffix`; const prepared = prepareToolApproval("run_tests", {}, fallback); @@ -155,6 +203,7 @@ test("approved long command executes its distinguishing suffix", async (t) => { class OneToolBrain implements Brain { results: ToolResult[] = []; + ids: string[] = []; constructor(private readonly name: string, private readonly args: Record) {} run(_task: TaskCommand): AsyncIterable { const name = this.name; @@ -164,7 +213,7 @@ class OneToolBrain implements Brain { yield { type: "done", ok: true, result: "done", remaining: 0, reason: "" }; })(); } - sendToolResult(_id: string, result: ToolResult): void { this.results.push(result); } + sendToolResult(id: string, result: ToolResult): void { this.ids.push(id); this.results.push(result); } control(): void {} close(): void {} } @@ -183,6 +232,100 @@ test("coding host loop denial never calls the executor", async () => { assert.equal(brain.results[0]?.exitCode, 1); }); +test("host loop delivers denial feedback once for one call ID, even if the event repeats", async () => { + const events: BrainEvent[] = [ + { type: "tool_call", id: "repeat-id", name: "run_shell", args: { command: "echo never" } }, + { type: "tool_call", id: "repeat-id", name: "run_shell", args: { command: "echo never" } }, + { type: "done", ok: true, result: "done", remaining: 0, reason: "" }, + ]; + const delivered: Array<{ id: string; result: ToolResult }> = []; + const brain = { + run: async function* () { for (const event of events) yield event; }, + sendToolResult: (id: string, result: ToolResult) => { delivered.push({ id, result }); }, + control() {}, close() {}, + } as unknown as Brain; + let executions = 0; + let reviews = 0; + const exec = { executeAsync: async () => { executions++; return { output: "unexpected", exitCode: 0 }; } } as unknown as ToolExecutor; + await hostLoop(brain, exec, () => {}, { type: "task", text: "test", cwd: ".", poolGb: 5 }, undefined, + async ({ id }) => { reviews++; return { callId: id, approved: false, feedback: "Use the offline unit suite." }; }, + undefined, { failureBudget: false }); + assert.equal(executions, 0); + assert.equal(reviews, 1); + assert.equal(delivered.length, 1); + assert.equal(delivered[0]?.id, "repeat-id"); + assert.equal(delivered[0]?.result.output.match(/Use the offline unit suite\./g)?.length, 1); +}); + +test("local chat returns declined feedback under the same call without running it", async () => { + const priorTty = Object.getOwnPropertyDescriptor(process.stdin, "isTTY"); + Object.defineProperty(process.stdin, "isTTY", { value: true, configurable: true }); + try { + const brain = new OneToolBrain("run_shell", { command: "echo never" }); + let executions = 0; + let feedbackPrompts = 0; + const ctx = { + cfg: { permissionMode: "ask", autoApply: false }, + flags: { cwd: process.cwd(), yes: false, json: true }, + confirm: async () => false, + approvalFeedback: async () => { feedbackPrompts++; return "Use the offline unit suite."; }, + } as unknown as AppContext; + await runLocalTurn(ctx, "test", undefined, { brain, exec: { executeAsync: async () => { + executions++; + return { output: "unexpected", exitCode: 0 }; + } } }); + assert.equal(executions, 0); + assert.equal(feedbackPrompts, 1); + assert.equal(brain.results.length, 1); + assert.deepEqual(brain.ids, ["one"]); + assert.match(brain.results[0]!.output, /Operator instruction for this denied call: Use the offline unit suite\./); + } finally { + if (priorTty) Object.defineProperty(process.stdin, "isTTY", priorTty); + else delete (process.stdin as { isTTY?: boolean }).isTTY; + } +}); + +test("hosted dev-session receives the same one-call denial result and never runs the tool", async () => { + const posts: Record[] = []; + const fetchImpl = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + if (url.endsWith("/agent/dev/sessions")) { + return Response.json({ session_id: "approval-hosted", protocol_version: 1 }); + } + if (url.endsWith("/tool-results")) { + posts.push(JSON.parse(String(init?.body)) as Record); + return Response.json({}); + } + if (url.includes("/stream")) { + const frames = [ + { type: "tool_call", seq: 1, tool_call_id: "hosted-id", name: "run_shell", args: { command: "echo never" } }, + { type: "done", seq: 2, ok: true }, + ].map(frame => `data: ${JSON.stringify(frame)}\n\n`).join(""); + return new Response(frames, { status: 200, headers: { "content-type": "text/event-stream" } }); + } + return Response.json({}); + }) as typeof fetch; + const token = { get: async () => "aek_fixture" } as unknown as TokenStore; + const api = new ApiClient("https://example.invalid", token); + (api as unknown as { fetchImpl: typeof fetch }).fetchImpl = fetchImpl; + const priorFetch = globalThis.fetch; + globalThis.fetch = fetchImpl; + let executions = 0; + const exec = { executeAsync: async () => { executions++; return { output: "unexpected", exitCode: 0 }; } } as unknown as ToolExecutor; + try { + const brain = new CloudBrain(api, undefined, { requireLocalAuthority: true }); + await hostLoop(brain, exec, () => {}, { type: "task", text: "test", cwd: process.cwd(), poolGb: 5 }, undefined, + async ({ id }) => ({ callId: id, approved: false, feedback: "Use the offline unit suite." }), + undefined, { failureBudget: false }); + await new Promise(resolve => setTimeout(resolve, 20)); + } finally { globalThis.fetch = priorFetch; } + assert.equal(executions, 0); + assert.equal(posts.length, 1); + assert.equal(posts[0]?.["tool_call_id"], "hosted-id"); + assert.equal(posts[0]?.["exit_code"], 1); + assert.match(String(posts[0]?.["output"]), /Operator instruction for this denied call: Use the offline unit suite\./); +}); + test("local chat executes the validated snapshot with an argument binding", async () => { const brain = new OneToolBrain("run_shell", { command: longCommand }); let seenArgs: Record | undefined;