Do not send reason as content on permission request#171
Conversation
According to the specification, the permissionRequest contains a `ToolCallUpdate`, which - again according to the spec - "Replace[s] the content collection." https://agentclientprotocol.com/protocol/v1/schema#param-content-13 This means that if the agent sends a tool call with a diff for file edits and then the permission request, a typical client would replace the the diff with just the reason text. I'd say that either ACP should add a new field `reason` to `RequestPermissionRequest`, or we use the existing `_meta` field to include the reason for clients that want to handle it specially. This PR moves the reason into _meta.
| rawInput: params.command ? { command: stripShellPrefix(params.command), cwd: params.cwd } : null, | ||
| }, | ||
| options: APPROVAL_OPTIONS, | ||
| _meta: { codex: { params } } |
There was a problem hiding this comment.
Of course, we could also cherry-pick specific fields only.
|
Maybe silly question... but content is an array, can we not have both? |
benbrandt
left a comment
benbrandt
left a comment
There was a problem hiding this comment.
Either way, I think not overwriting the diff is the right choice here
|
I'm not sure what you're trying to say? If the permissionRequest contains I'd argue that the reason for a tool call request is something different than the tool call's content, so it shouldn't be mixed. |
2 participants
According to the specification, the permissionRequest contains a
ToolCallUpdate, which - again according to the spec - "Replace[s] the content collection."https://agentclientprotocol.com/protocol/v1/schema#param-content-13
This means that if the agent sends a tool call with a diff for file edits and then the permission request, a typical client would replace the the diff with just the reason text.
I'd say that either ACP should add a new field
reasontoRequestPermissionRequest, or we use the existing_metafield to include the reason for clients that want to handle it specially.(cc @benbrandt)
This PR moves the reason into _meta.
This also solves the problem I had initially in #126, where I didn't see any file content and Codex wrongly assumed that it needs to handle more events.