Skip to content

fix(acp): include permission request context in ACP prompts#42

Merged
BunsDev merged 1 commit into
mainfrom
codex/fix-acp-permission-prompts-to-show-details
Jun 6, 2026
Merged

fix(acp): include permission request context in ACP prompts#42
BunsDev merged 1 commit into
mainfrom
codex/fix-acp-permission-prompts-to-show-details

Conversation

@BunsDev
Copy link
Copy Markdown
Member

@BunsDev BunsDev commented Jun 5, 2026

Motivation

  • The ACP bridge previously sent only a generic title like Tool 'Bash' requires approval which dropped security-critical context such as the command, target path, or detailed reason, making approval dialogs misleading.
  • Users must see PermissionRequest.description, PermissionRequest.details, and PermissionRequest.path (and working dir) so they can make informed decisions for dangerous tools.

Description

  • Use a synthesized permission title derived from the full PermissionRequest (tool name + details/description + path) via format_permission_title and wire it into AcpPermissionHandler::request_permission and forward_pending.
  • Construct an ACP ToolCallUpdate that includes an explicit textual content block containing Tool, Description, Details, Context, Target, and Working directory via build_permission_tool_call_update / format_permission_content so clients receive the actionable context before approval.
  • Add small unit tests validating that Bash command paths and FileRead targets are included in the generated title and the serialized ToolCallUpdate.

Testing

  • Ran cargo test --package claurst-acp permission_ and the new focused tests passed.
  • Ran cargo clippy --package claurst-acp --all-targets --no-deps -- -D warnings and it completed for the claurst-acp crate with no warnings.
  • Ran git diff --check (format/lint checks) with no leftover issues for the modified file.
  • cargo check --workspace was attempted but blocked by a missing system dependency (alsa.pc) required by alsa-sys in this environment; this is unrelated to the ACP change.

Codex Task

Copilot AI review requested due to automatic review settings June 5, 2026 23:58
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the ACP permission-approval UX by ensuring permission prompts carry actionable, security-relevant context (tool, description/details, targets, and working directory) instead of a generic approval title.

Changes:

  • Add format_permission_title to synthesize a more informative permission title from PermissionRequest.
  • Send an ACP ToolCallUpdate with an explicit text content block (tool/description/details/context/target/working dir) via build_permission_tool_call_update / format_permission_content.
  • Add unit tests asserting that risky Bash commands and file targets appear in the title/content serialization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 63 to 67
let title = if reason.is_empty() {
format!("Approve {}", request.tool_name)
format_permission_title(&request)
} else {
reason.clone()
reason
};
@BunsDev BunsDev merged commit 84c65f8 into main Jun 6, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants