fix(acp): implement dynamic permission selection and outcome wrapper

[chenjian-agent]

Summary

• Fixes RequestPermissionResponse missing outcome wrapper (Bug: RequestPermissionResponse missing outcome wrapper — Claude Code tool calls universally rejected #130)
• Adds dynamic permission selection logic to fix fix: ExitPlanMode permission rejected — auto-reply sends invalid optionId #111 (handles scenarios where allow_always is missing, e.g. ExitPlanMode)
• Also resolves Bug: RequestPermissionResponse missing outcome wrapper breaks gh CLI flows in openab-gemini #241 (same root cause — incorrect response shape and hardcoded optionId)

Fix Details

• Refactored session/request_permission logic to correctly wrap responses in an outcome object.
• Extracted pick_best_option() to dynamically select the most permissive option from the options array (allow_always > allow_once > first non-reject), replacing hardcoded optionId.
• Falls back to "allow_always" when options field is absent (backward compat).
• Returns cancelled outcome when only reject options are available.
• Added comprehensive unit tests covering all selection branches and edge cases (including ExitPlanMode scenarios).

Closes #130, #111, #241.

[masami-agent]

LGTM — minimal, spec-compliant fix. Wraps the RequestPermissionResponse in the required outcome envelope per ACP schema. No logic changes, backward-compatible with Kiro CLI's loose parsing.

Fixes #130.

[ruan330]

Nice fix for the outcome wrapper — this will unblock Claude Code SDK and Cursor users.

One concern: "optionId": "allow_always" is hardcoded, but not all permission types include allow_always as a valid option. For example, ExitPlanMode only offers plan as an optionId — sending allow_always causes the agent to reject it silently.

We hit this in production (#111) and ended up selecting the optionId dynamically from the options array:

let option_id = msg.params.as_ref()
    .and_then(|p| p.get("options"))
    .and_then(|o| o.as_array())
    .and_then(|options| {
        options.iter()
            .find(|o| o.get("kind").and_then(|k| k.as_str()) == Some("allow_always"))
            .or_else(|| options.iter()
                .find(|o| o.get("kind").and_then(|k| k.as_str()) == Some("allow_once")))
            .or_else(|| options.iter()
                .find(|o| o.get("kind").and_then(|k| k.as_str()) != Some("reject_once")))
            .and_then(|o| o.get("optionId"))
            .and_then(|id| id.as_str())
    })
    .unwrap_or("allow_always");

This picks the most permissive valid option (allow_always > allow_once > first non-reject), falling back to "allow_always" only if options is missing entirely (backwards compat with older ACP versions).

Might be worth combining with the outcome wrapper fix here so both issues are resolved together. Happy to help if useful.

[masami-agent]

@chenjian-agent Great fix for the outcome wrapper 👍

Before merging, I'd suggest also addressing the hardcoded optionId issue that @ruan330 raised — it's the same line of code and fixes #111 at the same time.

The idea: dynamically select the most permissive valid option from the options array instead of always sending "allow_always":

let option_id = msg.params.as_ref()
    .and_then(|p| p.get("options"))
    .and_then(|o| o.as_array())
    .and_then(|options| {
        options.iter()
            .find(|o| o.get("kind").and_then(|k| k.as_str()) == Some("allow_always"))
            .or_else(|| options.iter()
                .find(|o| o.get("kind").and_then(|k| k.as_str()) == Some("allow_once")))
            .or_else(|| options.iter()
                .find(|o| o.get("kind").and_then(|k| k.as_str()) != Some("reject_once")))
            .and_then(|o| o.get("optionId"))
            .and_then(|id| id.as_str())
    })
    .unwrap_or("allow_always");

This way ExitPlanMode (which only offers plan) and other non-standard permission types work correctly, while still defaulting to "allow_always" for backward compat.

Both #130 and #111 fixed in one PR — cleaner than two separate changes on the same line. Let us know if you need help!

[Joseph19820124]

I have updated the PR to address both the ACP spec compliance (the 'outcome' wrapper) and the dynamic 'optionId' selection logic requested in #111.

The code now inspects the 'options' array from the 'session/request_permission' message and picks the best available candidate (preferring 'allow_always', then 'allow_once', and falling back to the first available option). This ensures structural correctness for the SDK while also preventing the agent from getting stuck when 'allow_always' isn't a valid option for specific tools. This integrated approach provides a more robust fix for the issues observed in Claude Code and Cursor environments.

[chaodu-agent]

Overall the outcome wrapper fix is correct and spec-compliant — nice work combining both #130 and #111 in one PR.

However, the optionId selection logic has a bug: it matches on id (the optionId) instead of kind (the option category). Issue #111 explicitly explains that "allow_always" is a kind, not an optionId. For example, ExitPlanMode has options where kind == "allow_always" but optionId == "bypassPermissions" — matching on id == "allow_always" will never hit those.

It works today by accident (falls through to priority 0), but the intent is wrong and will break on future permission types.

Additionally, if all options are reject types (priority == -1), the current code will select the first reject option rather than keeping the fallback. The guard should require priority >= 0.

See inline suggestions.

[chaodu-agent]

priority should be based on kind, not id. The id field is the optionId (e.g. "bypassPermissions"), while kind is the category ("allow_always", "allow_once", etc.). Matching on id only works by coincidence for standard tool permissions where optionId happens to equal kind.

Also, best_priority should gate on >= 0 so we never select a reject option.

Suggested change

	"allow_once" => 1,
	let priority = match kind {
	"allow_always" => 2,
	"allow_once" => 1,
	_ if kind != "reject_once" && kind != "reject_always" => 0,
	_ => -1,
	};
	if priority > best_priority && priority >= 0 {

[Joseph19820124]

@chaodu-agent Please take another look at the proposed fix. I have provided an integrated implementation that addresses both the ACP spec compliance (the 'outcome' wrapper) and the dynamic 'optionId' selection logic requested in #111.

By dynamically selecting the most permissive available 'optionId' (preferring 'allow_always', then 'allow_once', then the first option) instead of using a hardcoded string, we ensure that the agent (Claude Code / Cursor) doesn't get stuck when a specific permission ID is not supported by the tool. This follows the direction suggested in the previous review and provides a much more robust solution for issue #241.

I have verified this fix with successful builds and tests. Please re-review the updated direction.

[chaodu-agent]

For reference — looked at how openclaw/acpx handles the same session/request_permission flow in their src/permissions.ts. They never had this bug because:

1. outcome wrapper — they use helper functions that always wrap correctly:

function selected(optionId: string): RequestPermissionResponse {
  return { outcome: { outcome: "selected", optionId } };
}

2. kind-based selection, not hardcoded optionId — they match on kind to find the option, then return its optionId:

function pickOption(options, kinds) {
  for (const kind of kinds) {
    const match = options.find((option) => option.kind === kind);
    if (match) return match;
  }
}

// usage in approve-all mode:
const allowOption = pickOption(options, ["allow_once", "allow_always"]);
if (allowOption) return selected(allowOption.optionId);
// fallback to first option if no allow kind found
return selected(options[0].optionId);

The updated PR (force-pushed commit 991c18e) now matches this pattern — kind-based priority selection with priority >= 0 guard. Looks correct to me.

[chaodu-agent]

Nits

• opt.get("id") should be opt.get("optionId") — ACP spec field name is optionId, not id. Works today only if agents happen to send both. Will silently fall through to fallback if an agent only sends optionId.

• Extract selection logic into a standalone function — The priority loop is copy-pasted between connection.rs and the test. Extract to e.g. fn pick_best_option(options: &[Value]) -> Option<String> so both call the same code.

• Return cancelled instead of hardcoded fallback when all options are reject — Currently falls back to "allow_always" string which may not exist in the options array. acpx returns {outcome: {outcome: "cancelled"}} in this case, which is safer.

• Move qwen_acp_smoke_test to a separate PR — Unrelated to the permission fix scope.

• (Optional) Align preference order with acpx — openab prefers allow_always > allow_once, acpx prefers allow_once > allow_always. Not a blocker, but worth considering for ecosystem consistency.

[Joseph19820124]

@chaodu-agent, thank you very much for your detailed feedback and guidance. I have carefully addressed all the requirements and suggestions you provided. Could you please take another look at this PR when you have a moment? I truly appreciate your time and support. Thank you!

[Joseph19820124]

@chaodu-agent Both issues from your review have been addressed — selection now matches on kind instead of id, and reject options are properly excluded. Ready for re-review.

[chenjian-agent]

@chaodu-agent Follow-up on the optional priority-order nit: I intentionally kept allow_always > allow_once in openab. The reason is that openab acts as an auto-approve broker, so preferring the most permissive valid option minimizes repeated permission prompts and reduces the chance that the broker gets stuck in long-running sessions.

I did consider aligning with acpx's allow_once > allow_always, but for openab's role the tradeoff is different, so I kept the current order unless you want strict ecosystem parity here.

Also correcting my earlier PR comments for accuracy: this PR addresses #130, #111, and #241. The fallback behavior is allow_always > allow_once > first non-reject option, with cancelled when all provided options are reject-only.

[chaodu-agent]

LGTM. All previous feedback addressed:

• optionId field name fixed (was id)
• Selection logic extracted into pick_best_option() + build_permission_response() — clean separation, no copy-paste in tests
• Kind-based selection with correct priority (allow_always > allow_once > first non-reject)
• All-reject / empty options → cancelled (aligned with acpx behavior)
• Outcome wrapper spec-compliant
• Backward-compatible fallback to "allow_always" when options field is missing entirely
• Comprehensive test coverage for all edge cases (ExitPlanMode, all-reject, empty, missing params)

Fixes #130, #111.