Skip to content

fix: deserialize permission_suggestions into PermissionUpdate instances#920

Open
sarahdeaton wants to merge 1 commit intomainfrom
fix/permission-suggestions-roundtrip
Open

fix: deserialize permission_suggestions into PermissionUpdate instances#920
sarahdeaton wants to merge 1 commit intomainfrom
fix/permission-suggestions-roundtrip

Conversation

@sarahdeaton
Copy link
Copy Markdown
Contributor

Problem

ToolPermissionContext.suggestions is typed as list[PermissionUpdate], but _internal/query.py assigns the raw permission_suggestions list from the control protocol without deserializing, so at runtime it's list[dict].

This breaks the round-trip pattern where a can_use_tool callback echoes a suggestion back to persist it:

async def can_use_tool(tool_name, input_data, context):
    persist = [s for s in context.suggestions if s.destination == "localSettings"]
    # AttributeError: 'dict' object has no attribute 'destination'
    return PermissionResultAllow(updated_permissions=persist)
    # and even with dict access, the response path calls .to_dict() on each item:
    # AttributeError: 'dict' object has no attribute 'to_dict'

The equivalent works in the TypeScript SDK.

Fix

  • Add PermissionUpdate.from_dict() as the inverse of to_dict(), rebuilding PermissionRuleValue entries from the camelCase wire keys.
  • Map permission_suggestions through it when constructing ToolPermissionContext.

Suggestions now arrive as dataclass instances and can be returned directly in PermissionResultAllow(updated_permissions=...).

Tests

  • TestPermissionUpdate round-trip unit tests for addRules, setMode, addDirectories.
  • test_permission_callback_suggestions_roundtrip sends a wire-format suggestion, asserts the callback receives a PermissionUpdate, echoes it back, and checks the serialized response matches the input.
  • Fixed a fixture in test_permission_callback_deny that passed ["deny"] as a suggestion; it only worked before because nothing parsed the list.

Related: #624 tightens the type annotation on the control-protocol TypedDict but doesn't change runtime behavior; this PR makes the runtime match the annotation.

ToolPermissionContext.suggestions was typed as list[PermissionUpdate] but
populated with raw wire-protocol dicts, so attribute access failed and
echoing a suggestion back via updated_permissions threw on .to_dict().

Add PermissionUpdate.from_dict() as the inverse of to_dict() and map
permission_suggestions through it when building ToolPermissionContext.
Suggestions can now be filtered by attribute and returned directly in
PermissionResultAllow(updated_permissions=...).

Also fixes a test fixture that passed a bare string in
permission_suggestions, which only worked because nothing parsed it.
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

LGTM — straightforward fix making runtime match the declared type, with solid round-trip test coverage.

Extended reasoning...

Overview

This PR fixes a type-vs-runtime mismatch: ToolPermissionContext.suggestions is annotated list[PermissionUpdate] but was being populated with raw dicts from the control protocol. The fix adds PermissionUpdate.from_dict() (a ~15-line classmethod that is the inverse of the existing to_dict()) and maps incoming permission_suggestions through it in _internal/query.py. Touches 2 source files (~25 production LOC) plus 2 test files.

Security risks

None. This is pure deserialization of structured data already arriving over the trusted CLI control protocol (subprocess stdin/stdout). No user-controlled input, no shell/eval, no auth or permission evaluation logic — it only changes the shape of data handed to the user's callback. from_dict uses .get() for all optional fields and data["type"] / r["toolName"] for required ones, which correctly mirrors what to_dict() emits.

Level of scrutiny

Low. This is a targeted bug fix that brings runtime behavior in line with an existing type annotation, matching TypeScript SDK parity. The from_dict implementation is mechanical field mapping (camelCase wire → snake_case dataclass) and the round-trip test assert update.to_dict() == wire directly verifies it inverts to_dict() for all three variant types.

Other factors

Test coverage is thorough: unit round-trip tests for addRules/setMode/addDirectories, plus an integration test that drives a wire-format suggestion through _handle_control_request, asserts the callback receives a PermissionUpdate instance, echoes it back via updated_permissions, and verifies the serialized response equals the original wire dict. The incidental fix to the test_permission_callback_deny fixture (which previously passed an invalid ["deny"] that only worked because nothing parsed it) is correct collateral cleanup. No outstanding reviewer comments.

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