Possible naming improvements via codegen changes#1043
Possible naming improvements via codegen changes#1043SteveSandersonMS wants to merge 10 commits intomainfrom
Conversation
Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1043
nodejs/src/generated/rpc.ts
Outdated
| /** | ||
| * The elicitation response (accept with form values, decline, or cancel) | ||
| */ | ||
| export interface UiElicitationResponse { |
There was a problem hiding this comment.
Duplicate interface declaration
UiElicitationResponse is declared twice in this file (also at line 1073). TypeScript's declaration merging means this compiles, but it's a codegen artifact indicating that the same schema structure appears in two places without a shared $ref. The first occurrence (line 1073) is the result of session.ui.elicitation, and this second occurrence (line 1204) is the type for the result field in HandlePendingElicitationRequest.
In Go and Python, these two occurrences generate separate types (UIElicitationResponse and ResultClass). The duplicate here suggests the two schema locations should share the same definition reference, which would also fix the ResultClass naming in Go and Python.
There was a problem hiding this comment.
Fixed in add04df. TypeScript now deduplicates repeated exported declarations emitted by per-method json-schema-to-typescript compiles, so this no longer produces a second UiElicitationResponse declaration.
… side Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1043
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the review findings in add04df. Specifically: (1) TS now deduplicates repeated named export blocks from per-method |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1043
|
|
||
|
|
||
| class HostType(Enum): | ||
| class ContextChangedHostType(Enum): |
There was a problem hiding this comment.
Naming inconsistency: This enum is named ContextChangedHostType here but StartContextHostType in Go and .NET. Consider adding a title/titleSuggestion to this schema node so quicktype produces StartContextHostType to match.
|
|
||
|
|
||
| class Operation(Enum): | ||
| class ChangedOperation(Enum): |
There was a problem hiding this comment.
Naming inconsistency: Python collapses both the plan.changed and workspace.fileChanged operation enums into a single ChangedOperation, while Go and .NET produce two separate enums PlanChangedOperation and WorkspaceFileChangedOperation. If they should be distinct, schema-level title/titleSuggestion overrides on each individual enum schema would resolve this.
|
|
||
|
|
||
| class PermissionRequestKind(Enum): | ||
| class Kind(Enum): |
There was a problem hiding this comment.
Naming inconsistency: Kind is too generic and lacks context. Go names this PermissionRequestKind (renamed from PermissionRequestedDataPermissionRequestKind in this PR). Adding a titleSuggestion: "PermissionRequestKind" to this schema node would align the Python output with Go.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1043
| ExtensionsLoadedExtensionStatusStarting ExtensionsLoadedExtensionStatus = "starting" | ||
| ) | ||
|
|
||
| // Type aliases for convenience. |
There was a problem hiding this comment.
The convenience type/const aliases below this line still reference the old generated type names that no longer exist in this file after the renames. For example:
// TYPE_ALIASES in scripts/codegen/go.ts — still points to OLD names:
PermissionRequest = PermissionRequestedDataPermissionRequest // ❌ undefined; PermissionRequest is now a struct directly
PermissionRequestKind = PermissionRequestedDataPermissionRequestKind // ❌ redeclared (PermissionRequestKind is now a string type at line ~2227)
PermissionRequestCommand = PermissionRequestedDataPermissionRequestCommandsItem // ❌ undefined
PossibleURL = PermissionRequestedDataPermissionRequestPossibleUrlsItem // ❌ undefined
Attachment = UserMessageDataAttachmentsItem // ❌ undefined
AttachmentType = UserMessageDataAttachmentsItemType // ❌ undefinedAnd the CONST_ALIASES block similarly references UserMessageDataAttachmentsItemTypeFile, PermissionRequestedDataPermissionRequestKindShell, etc. — all of which have been renamed.
The TYPE_ALIASES and CONST_ALIASES maps in scripts/codegen/go.ts (around line 800) were not updated as part of this PR. They need to be updated to reflect the new generated names, e.g.:
// Suggested fixes:
PossibleURL = PermissionRequestShellPossibleUrl
Attachment = UserMessageAttachment
AttachmentType = UserMessageAttachmentType
// Const aliases:
AttachmentTypeFile = UserMessageAttachmentTypeFile
// etc.Types like PermissionRequest and PermissionRequestKind are now direct struct/type declarations, so their alias entries can simply be removed (or the alias should map to the new canonical name if needed for backward compat).
| type SessionPermissionsHandlePendingPermissionRequestParams struct { | ||
| RequestID string `json:"requestId"` | ||
| Result SessionPermissionsHandlePendingPermissionRequestParamsResult `json:"result"` | ||
| type PermissionDecisionRequest struct { |
There was a problem hiding this comment.
This PR renames SessionPermissionsHandlePendingPermissionRequestParams → PermissionDecisionRequest (and similarly for other types), but the non-generated Go SDK code in go/session.go and go/types.go still references the old names. A full list of ~43 identifiers from the rpc package that are missing includes:
| Old name (used in non-generated code) | New name (in this PR) |
|---|---|
rpc.SessionCommandsHandlePendingCommandParams |
rpc.CommandsHandlePendingCommandRequest |
rpc.SessionUIHandlePendingElicitationParams |
rpc.HandlePendingElicitationRequest |
rpc.SessionUIHandlePendingElicitationParamsResult |
rpc.UIElicitationResponse |
rpc.SessionPermissionsHandlePendingPermissionRequestParams |
rpc.PermissionDecisionRequest |
rpc.ActionAccept / rpc.ActionCancel |
rpc.UIElicitationActionAccept / rpc.UIElicitationActionCancel |
rpc.PropertyTypeBoolean / rpc.PropertyTypeString |
rpc.UIElicitationSchemaPropertyNumberTypeBoolean / ...String |
rpc.SessionUIElicitationParams |
(structure changed) |
rpc.ConventionsPosix / rpc.ConventionsWindows |
rpc.SessionFSSetProviderConventionsPosix / ...Windows |
rpc.EntryTypeDirectory / rpc.EntryTypeFile |
rpc.SessionFSReaddirWithTypesEntryTypeDirectory / ...File |
rpc.ModeInteractive / rpc.ModePlan |
rpc.SessionModeInteractive / rpc.SessionModePlan |
rpc.LevelError etc. |
rpc.LogLevelError etc. |
All rpc.SessionFS*Params |
rpc.SessionFS*Request |
go/session.go, go/types.go, and other non-generated files will need to be updated to use the new names. This is expected to be a significant follow-up change to make the Go SDK compile.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Cross-SDK Consistency ReviewAll four SDK implementations (Node.js/TypeScript, Python, Go, .NET) are updated in this PR, and the changes are well-coordinated. What was checked:
Language conventions are properly respected:
One Python-only change in No cross-SDK consistency issues found. 🎉
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Draft of possible changes to RPC naming convention changes to fix #1035
This PR improves generated public type names across the SDKs by moving naming decisions into the shared schema/codegen pipeline instead of relying on language-specific quirks.
The goal is to make generated names:
Main naming rules
The naming is now mostly mechanical rather than hand-curated:
Derive names from the RPC method / event identity first
account.getQuotabecomes anAccountQuotaroot instead ofAccountGetQuotaResult.Drop generic namespace and filler words
session.get,read, andlistwhen they are only structuralSingularize list entities
models.listusesModelfor nested item types andModelListfor the top-level resultKeep top-level wrapper words only when they are actually useful
Request/Resultremain for top-level params/results...Request...Result...Value...ItemnoiseDrop purely structural wrapper property names in nested helpers
data,result,request,response,kind,value, etc. are removed when they are just containers, not meaningful domain termsNormalize overlapping words
requested+requestUse explicit schema-local names for true API surface decisions
Before / after examples
Previously too long
AccountGetQuotaResultQuotaSnapshotsValueAccountQuotaSnapshotSessionPermissionsHandlePendingPermissionRequestResultPermissionRequestResultSessionToolsHandlePendingToolCallResultHandleToolCallResultSessionUiElicitationRequestRequestedSchemaUiElicitationSchemaSessionUiHandlePendingElicitationRequestResultUiElicitationResponseSessionUiHandlePendingElicitationResultUiElicitationResultToolExecutionCompleteDataResultContentsItemResourceLinkIconsItemThemeToolExecutionCompleteContentResourceLinkIconThemeUserMessageDataAttachmentsItemSelectionSelectionEndUserMessageAttachmentSelectionDetailsEndPreviously too short / too generic
EntrySessionFsReaddirWithTypesEntry/SessionFSReaddirWithTypesEntryEndUserMessageAttachmentSelectionDetailsEndModeSessionModeSessionModeGetResultModeSessionModeA few especially notable cases
The worst "path explosion" example was the old icon theme helper name:
ToolExecutionCompleteDataResultContentsItemResourceLinkIconsItemThemeToolExecutionCompleteContentResourceLinkIconThemeThe worst "too generic to understand in isolation" examples were names like:
EntryEndModeThose now carry enough domain context to be understandable in generated SDKs without needing to inspect the surrounding method/property path.
API-specific naming that is now explicit
A few names are intentionally chosen as API surface names rather than just synthesized mechanically, for example:
UiElicitationResponseUiElicitationResultHandlePendingElicitationRequestPermissionDecisionPermissionDecisionRequestPermissionRequestResultModeGetResultModeSetResultHandleToolCallResultThat keeps the generator logic generic while still letting the schema declare clear public names where needed.
TypeScript note
TypeScript still avoids exploding the surface area with every synthesized helper as a named export. Explicit public names are preserved where intended, while synthetic helper naming is mainly used to improve the non-TS SDKs and shared generation consistency.