Skip to content

fix: wrap permission auto-reply in outcome object and select optionId dynamically#148

Closed
DrVictorChen wants to merge 1 commit intoopenabdev:mainfrom
DrVictorChen:fix/permission-auto-reply
Closed

fix: wrap permission auto-reply in outcome object and select optionId dynamically#148
DrVictorChen wants to merge 1 commit intoopenabdev:mainfrom
DrVictorChen:fix/permission-auto-reply

Conversation

@DrVictorChen
Copy link
Copy Markdown

Summary

Combined fix for #130 and #111 — both touch the same auto-reply block in connection.rs.

Changes

  1. Select the most permissive option from the request's options array by kind priority: allow_always > allow_once > first non-reject_once
  2. Wrap the response in { outcome: { outcome: "selected", optionId } } per the ACP spec

Tested with

Agent Before After
Claude Code (ACP) ❌ all tool calls rejected
Cursor agent (ACP) Cannot read properties of undefined (reading 'outcome')
Codex (ACP) ✅ (was working due to loose parsing)

Fixes #130, fixes #111

🤖 Generated with Claude Code

… dynamically

The auto-reply for session/request_permission had two bugs:

1. Response was missing the required `outcome` wrapper — agents that
   validate the ACP spec (Claude Code, Cursor) treated every tool call
   as rejected (#130).

2. optionId was hardcoded to "allow_always", which is a `kind`, not an
   `optionId`.  Different agents use different ids (e.g. Cursor uses
   "allow-always" with a hyphen; ExitPlanMode uses "bypassPermissions").
   The agent receives an unknown id → treats it as rejected (#111).

Now we:
- Select the most permissive option from the request's `options` array
  by `kind` priority: allow_always > allow_once > first non-reject_once
- Wrap the response in `{ outcome: { outcome: "selected", optionId } }`
  per the ACP spec

Fixes #130, fixes #111

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@the3mi
Copy link
Copy Markdown

the3mi commented Apr 9, 2026

PR Review ✅

Thanks @DrVictorChen for the comprehensive PR description — the What/Why/Impact table makes this easy to review.

Looks good

Minor note

If an option object exists but has no field, falls back to — this is conservative and correct.

Approve. Ready to merge.

@masami-agent
Copy link
Copy Markdown

Thanks for the combined fix! Since we're merging #147 first for the outcome wrapper, could you rebase on top of that and keep this PR scoped to just the dynamic optionId selection logic for #111? That way each PR stays single-responsibility and easier to review/revert independently.

@DrVictorChen
Copy link
Copy Markdown
Author

Sure, will rebase once #147 is merged. Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude code Claude Code

Projects

None yet

4 participants