mirror of
https://github.com/agentclientprotocol/claude-agent-acp.git
synced 2026-11-09 19:02:05 +00:00
refactor: serve permission requests on the experimental ACP v2 surface (#1234)
First part of step 5 of opt-in ACP v2 support (after #1231, #1232): the v2 surface now sends the agent's permission requests. Nothing changes on ACP v1: no v1 golden file changes. ## What differs in v2 A v1 request asks about a tool call, and clients show the tool call's title as the prompt. A v2 request gives the prompt a required `title` and an optional `description` of its own, apart from the tool call. It asks about a `subject`, which can be a tool call or a command. ## Change - **Prompt text.** The agent already computes the prompt's heading and explanation, but only AIR gets them, in `_meta.jetbrains.air.permission`. The agent's request type, `AcpPermissionRequest`, now carries them as `title` and `description` for a v2 client. It's the same rule #1232 uses for diffs: build what v1 can't express only for a v2 client. So v1 requests are byte-identical, and the v1 scenario golden files, which record what the agent hands `requestPermission`, don't change. The v2 surface fails on a request without a title, so a missed site fails loudly. - **Subject.** Always `{ type: "tool_call", toolCall }`, where `toolCall` is the same patch a `tool_call_update` carries (`v2ToolCallPatch`). The `command` subject would suit Bash once display terminals exist, since its `terminalId` refers to one. - **Approval preview.** The exact patch before an Edit or Write runs is now built in v2 form for a v2 client, and the request shows it. This fixed a real gap: the agent pins the preview as content the client holds, but only AIR's request shape included it. Without the fix, a v2 client saw no diff at all, because the hook's identical patch was then dropped as unchanged. - **Answer.** v2's outcomes map back unchanged. An outcome v1 doesn't know, which the draft says is no approval, becomes `cancelled`, and the agent reads that as an aborted tool use. - **`requires_action`** while a request is open was already in place since #1226. ## Scenario suite All 12 scenarios that stopped at a permission request now pass: Bash, Edit (approved and rejected), Write, NotebookEdit, WebFetch, MCP, network, ExitPlanMode (approved and rejected), and two subagent scenarios. Each recording shows `requires_action`, the request, then `running`. Now only replay and the two plan scenarios remain. ## Verification - **End to end:** a new test drives the agent's real `canUseTool` through a v2 client. Allow, reject, and an unknown outcome become allow, deny, and an aborted tool use. - **Unit tests:** the prompt's title is separate from the tool call's (ExitPlanMode: "Ready to code?" vs "Approve Plan"), the preview shows in the request, the v2 preview fallback works, and the title guard rejects a missing title. - **Mutations:** each rule fails a test when removed. Sending `title`/`description` to every client fails 37 tests, including v1 golden files. - Build, lint and all 2380 tests pass.
This commit is contained in:
35
src/diff.ts
35
src/diff.ts
@@ -108,8 +108,8 @@ type V2DiffContent = v2.Diff & { type: "diff" };
|
||||
* normalizes quotes or fails.
|
||||
*
|
||||
* A Write always gets content that shows the change: the patch, the standard
|
||||
* diff of a text that cannot have an exact patch, or a notice that the Write
|
||||
* overwrites a file that the adapter cannot show.
|
||||
* diff (in v2, the change alone) of a text that cannot have an exact patch, or
|
||||
* a notice that the Write overwrites a file that the adapter cannot show.
|
||||
*
|
||||
* The preview uses the tool input as it is. Claude can still remove the
|
||||
* trailing whitespace of a line before it writes. The PostToolUse hook then
|
||||
@@ -119,6 +119,7 @@ export async function previewPatchContent(
|
||||
toolName: string,
|
||||
input: Record<string, unknown>,
|
||||
cwd?: string,
|
||||
form: PatchForm = "air",
|
||||
): Promise<ToolCallContent[] | undefined> {
|
||||
if (toolName === "Edit") {
|
||||
const edit = input as EditPreviewInput;
|
||||
@@ -142,7 +143,7 @@ export async function previewPatchContent(
|
||||
if (oldText !== null && (oldText.trim() !== "" || oldText.includes("\uFEFF"))) {
|
||||
return undefined;
|
||||
}
|
||||
return airPatch(filePath, oldText, newString);
|
||||
return exactPatch(filePath, oldText, newString, form);
|
||||
}
|
||||
if (oldText === null) return undefined;
|
||||
const occurrences = oldText.split(oldString).length - 1;
|
||||
@@ -151,7 +152,7 @@ export async function previewPatchContent(
|
||||
return undefined;
|
||||
}
|
||||
const newText = replacedText(oldText, oldString, newString, edit.replace_all === true);
|
||||
return airPatch(filePath, oldText, newText);
|
||||
return exactPatch(filePath, oldText, newText, form);
|
||||
}
|
||||
|
||||
if (toolName === "Write") {
|
||||
@@ -177,11 +178,13 @@ export async function previewPatchContent(
|
||||
}
|
||||
if (oldText === content) return undefined;
|
||||
const standard: ToolCallContent[] = [
|
||||
{ type: "diff", path: write.file_path, oldText, newText: content },
|
||||
form === "v2"
|
||||
? v2DiffContent(filePath, oldText === null ? "create" : "update")
|
||||
: { type: "diff", path: write.file_path, oldText, newText: content },
|
||||
];
|
||||
if (!isPatchableText(content)) return standard;
|
||||
// A dense change can exceed the diff budget. The approval then shows the standard diff.
|
||||
return airPatch(filePath, oldText, content) ?? standard;
|
||||
return exactPatch(filePath, oldText, content, form) ?? standard;
|
||||
}
|
||||
|
||||
return undefined;
|
||||
@@ -276,11 +279,7 @@ export async function patchUpdateFromDiffToolResponse(
|
||||
const patch = filePatch(oldText, newText);
|
||||
if (!patch) return undefined;
|
||||
return {
|
||||
content: [
|
||||
form === "v2"
|
||||
? v2DiffContent(response.filePath, patch.change, patch.hunks)
|
||||
: airPatchContent(response.filePath, patch),
|
||||
],
|
||||
content: [patchContent(response.filePath, patch, form)],
|
||||
// A created file keeps the location of its Write tool call.
|
||||
locations:
|
||||
oldText === null
|
||||
@@ -633,14 +632,22 @@ function isPatchableText(text: string): boolean {
|
||||
);
|
||||
}
|
||||
|
||||
/** The AIR patch content of a change, or undefined when it has no patch. */
|
||||
function airPatch(
|
||||
/** The exact patch content of a change, or undefined when it has no patch. */
|
||||
function exactPatch(
|
||||
filePath: string,
|
||||
oldText: string | null,
|
||||
newText: string,
|
||||
form: PatchForm,
|
||||
): ToolCallContent[] | undefined {
|
||||
const patch = filePatch(oldText, newText);
|
||||
return patch && [airPatchContent(filePath, patch)];
|
||||
return patch && [patchContent(filePath, patch, form)];
|
||||
}
|
||||
|
||||
/** The content that carries a patch, in the given form. */
|
||||
function patchContent(filePath: string, patch: FilePatch, form: PatchForm): ToolCallContent {
|
||||
return form === "v2"
|
||||
? v2DiffContent(filePath, patch.change, patch.hunks)
|
||||
: airPatchContent(filePath, patch);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user