Clone AdditionalProperties in AlwaysApprove tool-approval responses#741
Clone AdditionalProperties in AlwaysApprove tool-approval responses#741PratikDhanave wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes map aliasing in tool-approval “AlwaysApprove” response helpers so that emitted responses snapshot AdditionalProperties rather than sharing the same map instance with the request.
Changes:
- Update
AlwaysApproveToolResponseandAlwaysApproveToolWithArgumentsResponseto usemaps.Clone(t.AdditionalProperties)when populating response headers. - Add a regression test ensuring post-response mutation of the request’s
AdditionalPropertiesdoes not affect the always-approve responses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
message/content.go |
Clones AdditionalProperties when building always-approve approval response wrappers to avoid shared-map aliasing. |
message/content_test.go |
Adds coverage to assert always-approve helpers snapshot AdditionalProperties (matching existing CreateResponse semantics). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
Bot
added
the
parity-approved
This comment has been minimized.
This comment has been minimized.
AlwaysApproveToolResponse and AlwaysApproveToolWithArgumentsResponse copied the request's AdditionalProperties map by reference, so the emitted response shared one map instance with the request. Mutating the request afterward would leak into the already-created response, matching neither CreateResponse (which uses maps.Clone) nor the .NET/Python snapshot semantics. Clone the map so the response is an independent snapshot.
ffc15d3 to
b8a15a9
Compare
Parity Review — ApprovedThis PR fixes a reference-aliasing bug in Scope: Cross-repo parity: The PR description correctly identifies that this change restores alignment with .NET and Python SDK semantics, where approval responses capture request metadata as an independent snapshot rather than aliasing the live request object. Labels: No parity issues found. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
ToolApprovalRequestContent.AlwaysApproveToolResponseandAlwaysApproveToolWithArgumentsResponsesetContentHeader{AdditionalProperties: t.AdditionalProperties}, copying the request's map by reference. The request and the emitted response therefore share one map instance, so mutating the request'sAdditionalPropertiesafter creating the response silently leaks into the response.This changes both helpers to
maps.Clone(t.AdditionalProperties)so the response carries an independent snapshot.maps.Clone(nil)returnsnil, so no nil guard is needed;mapsis already imported.Why
CreateResponsealready clones withmaps.Clone, andTestToolApprovalRequestContent_CreateResponseSnapshotsFunctionCallasserts that a post-response mutation of the request must not affect the response. The AlwaysApprove helpers violated that same contract. This aligns them with the response-is-a-snapshot semantics used across the .NET and Python SDKs, where approval responses capture request metadata by value rather than aliasing the live request.How tested
Added
TestToolApprovalRequestContent_AlwaysApproveSnapshotsAdditionalProperties, which builds a request withAdditionalProperties, calls each AlwaysApprove helper, mutates the request's map afterward, and asserts the response value is unchanged. It fails before the fix (both subtests) and passes after.go build ./...,go vet ./message/..., andgo test ./message/...are green.