Skip to content

fix(acp): Constrain permission option kinds to the ACP schema - #358

Merged
shayne-snap merged 1 commit into
usewhale:mainfrom
reneleonhardt:fix/acp-mcp-kind-invalid
Aug 10, 2026
Merged

fix(acp): Constrain permission option kinds to the ACP schema#358
shayne-snap merged 1 commit into
usewhale:mainfrom
reneleonhardt:fix/acp-mcp-kind-invalid

Conversation

@reneleonhardt

@reneleonhardt reneleonhardt commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

PermissionOption.Kind was a free-form string, so nothing at compile time or runtime prevented sending a kind outside the ACP schema. The schema enum (agent-client-protocol-schema) defines exactly four canonical kinds: allow_once | allow_always | reject_once | reject_always.

  • v1 rejects unknown kinds at deserialization — the observed failure: Zed's strict serde refused allow_tool/allow_server, the dialog never produced a selected outcome, and the approval was silently denied (unknown permission outcome: "").
  • v2 tolerates unknown kinds via an untagged Other(String) fallback, but the dialog's option lookup never matches Other, so those kinds still cannot work — and would become a future hazard if whale ever negotiates v2.

Changes

  • Typed kind (types.go): Kind stringPermissionOptionKind + constants KindAllowOnce/KindAllowAlways/KindRejectOnce/KindRejectAlways + Valid() + String(), matching the schema enum.
  • Send-path guard (adapter.go): NewACPApprovalFunc denies loudly via invalidPermissionOptionKind instead of sending a payload the client would reject.
  • Options: inline once/always/reject options now use the typed constants (wire values unchanged).

Validation

  • go build ./..., go vet, gofmt clean.
  • -race on internal/acp.
  • Tests: serialize the real request_permission payload for every tool-name permutation and assert every kind is schema-valid; cross-check against a strict decoder mirroring the serde enum; Valid() guard; invalidPermissionOptionKind helper (nil, first-invalid, valid); decision-mapping downstream (once/always/reject/unknown/missing/cancelled/unknown-outcome/malformed); error envelope; transport close; 32 concurrent sessions (-race clean).

Review guidance

  • types.go PermissionOptionKind + Valid(): the four canonical kinds are the single source of truth; any new kind must first exist in the schema crate (v2's Other(String) fallback is an escape hatch, not a license to invent kinds).
  • adapter.go guard + invalidPermissionOptionKind: fail-loud deny before the wire, replacing the old silent denial.

User-visible impact

  • Prevents MCP approvals silently denied (tool result approval_denied, turn canceled) when the client rejects unserializable kinds — that failure can kill a session.
  • On main, current options were already schema-valid; this closes the systemic hole so an invalid kind can never reach the wire again.

Breaking changes

None. Wire values unchanged (allow_once/allow_always/reject_once); typed field marshals identically to the previous string.

Reproduction

  1. Failure mode: request_permission with kind: "allow_tool" → Zed.log unknown variant 'allow_tool', expected one of 'allow_once', 'allow_always', 'reject_once', 'reject_always'unknown permission outcome: "" — denying.
  2. Regression: TestPermissionOptionKindsAreSchemaValid serializes every option permutation and asserts schema-valid kinds on the wire.

Developed with carefully directed, manually reviewed AI assistance.

PermissionOption.Kind was a free-form string, so nothing at compile time or
runtime prevented sending a kind outside the ACP schema. The schema enum
(agent-client-protocol-schema) defines exactly four canonical kinds:
allow_once | allow_always | reject_once | reject_always. v1 rejects any other
kind at deserialization — the observed failure: Zed's strict serde refused
allow_tool/allow_server, the dialog never produced a selected outcome, and the
approval was silently denied (whale-acp logs `unknown permission outcome: ""`).
v2 tolerates unknown kinds via an untagged Other(String) fallback, but the
dialog's option lookup never matches Other, so those kinds still cannot work —
and would become a future hazard if whale ever negotiates v2.

Close the systemic hole so only schema-valid kinds can reach the wire:

- types.go: Kind string -> typed PermissionOptionKind + constants
  KindAllowOnce/KindAllowAlways/KindRejectOnce/KindRejectAlways + Valid() +
  String()
- adapter.go: NewACPApprovalFunc denies loudly via invalidPermissionOptionKind
  instead of sending a payload the client would reject
- tests: serialize the real request_permission payload for every tool-name
  permutation and assert every kind is schema-valid; cross-check against a
  strict decoder mirroring the serde enum; cover the decision-mapping
  downstream, error envelope, transport close, and 32 concurrent sessions
  (-race clean)

Co-authored-by: GPT-5.6 Sol <codex@openai.com>
@shayne-snap
shayne-snap merged commit 4febef3 into usewhale:main Aug 10, 2026
2 of 4 checks passed
@reneleonhardt
reneleonhardt deleted the fix/acp-mcp-kind-invalid branch August 10, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants