fix(go): remove hand-written PermissionRequest that conflicts with generated type#685
Conversation
…nerated type The @github/copilot 0.0.421 schema update (PR #684) caused quicktype to generate a PermissionRequest struct in generated_session_events.go that conflicts with the hand-written one in types.go. The generated type has all 23+ typed fields while the hand-written one was a simplified wrapper with an Extra map catch-all. Remove the hand-written type and its custom UnmarshalJSON in favor of the generated one. Update the E2E test that accessed Extra["toolName"] to use the typed ToolName field instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8c789d6 to
1b958f9
Compare
There was a problem hiding this comment.
Pull request overview
Fixes the Go build break caused by a name collision between a newly-generated PermissionRequest (from the updated @github/copilot schema) and a hand-written PermissionRequest in go/types.go, by removing the hand-written type and updating affected tests to use the generated typed fields.
Changes:
- Remove the hand-written
PermissionRequeststruct and its customUnmarshalJSONfromgo/types.go. - Update Go E2E tools test to validate
toolNamevia the generatedPermissionRequest.ToolNamefield.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
go/types.go |
Removes the conflicting hand-written PermissionRequest so the generated type can be used. |
go/internal/e2e/tools_test.go |
Updates assertions to use generated typed fields (ToolName) instead of Extra. |
| return nil | ||
| } | ||
|
|
||
| // PermissionRequestResultKind represents the kind of a permission request result. |
There was a problem hiding this comment.
Removing the hand-written PermissionRequest (with Extra map) is a breaking change for Go SDK consumers who were reading unknown/forward-compatible fields via request.Extra. If this is an intentional API change, it should be explicitly called out in Go release notes / README (and ideally provide a migration hint toward the generated typed fields), since user code will no longer be able to access new/unknown permission-request properties until the schema/typegen is updated.
| // PermissionRequestResultKind represents the kind of a permission request result. | |
| // PermissionRequestResultKind represents the kind of a permission request result. | |
| // | |
| // Breaking change note: | |
| // Earlier versions of the Go SDK exposed a hand-written PermissionRequest type | |
| // with an Extra map (for example, `Extra map[string]any`) to allow callers to | |
| // read forward-compatible / unknown permission-request properties directly from | |
| // the JSON payload. | |
| // | |
| // Starting with this version, permission request types are generated from the | |
| // Copilot schema and no longer expose the Extra map. New fields added to the | |
| // schema are surfaced as typed struct fields once you update to a newer SDK | |
| // / schema generation, rather than via request.Extra. | |
| // | |
| // Migration guidance: | |
| // - Prefer using the typed fields on PermissionRequest (and related types) | |
| // instead of accessing arbitrary keys via request.Extra. | |
| // - If you previously depended on unknown keys in Extra for forward- | |
| // compatibility, you should update the SDK when the schema adds new | |
| // properties you care about, and then consume those properties through | |
| // the generated, strongly-typed fields. |
Cross-SDK Consistency ReviewThis PR fixes a build break in the Go SDK by removing the hand-written However, this change creates a cross-SDK inconsistency that should be addressed: Current State After This PR
EvidenceGo (after this PR): if req.ToolName != nil {
toolName = *req.ToolName
}Python: permission_log.append(f"approved:{request.tool_name}")Node.js: permissionLog.push(`approved:${request.toolName}`); // No type checking!.NET: var toolName = request.ExtensionData?.TryGetValue("toolName", out var value) == true
? value?.ToString() ?? "unknown"
: "unknown";RecommendationFor consistency and better developer experience across all SDKs, consider:
The inconsistency isn't a blocker for this PR since it fixes a real build issue, but the multi-SDK nature of this repo means these differences will confuse users who work across multiple languages. Note: This review focuses solely on cross-SDK API consistency as requested. The code changes themselves look correct for fixing the Go build issue.
|
|
As for cross-SDK consistency, we'll be changing how |
Problem
The
@github/copilot0.0.421 schema update (#684) caused quicktype to generate aPermissionRequeststruct ingenerated_session_events.gothat conflicts with the hand-written one intypes.go, breaking the Go build.Fix
Remove the hand-written
PermissionRequesttype (and its customUnmarshalJSON) in favor of the generated one. The generated type has all 23+ typed fields (ToolName,Diff,Path, etc.) while the hand-written one was a simplified wrapper with a genericExtra map[string]anycatch-all.Changes
go/types.go: RemovePermissionRequeststruct andUnmarshalJSONmethod (-33 lines)go/internal/e2e/tools_test.go: Update test to use typedreq.ToolNamefield instead ofreq.Extra["toolName"]Why not a subpackage?
Other SDKs avoid this collision because generated code lives in separate modules (
generated/). We considered moving Go's generated events to ago/events/subpackage, but that would split the public API across two imports for every user (copilot.SessionEventHandlertakesevents.SessionEvent). Since the hand-written type was planned for removal anyway, deleting it is the simplest fix.