Possible naming improvements via codegen changes#1043
Possible naming improvements via codegen changes#1043SteveSandersonMS wants to merge 11 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>
This comment has been minimized.
This comment has been minimized.
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>
Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency ReviewThis PR does thorough work improving generated type names across all four SDKs. Previous review runs identified several issues (duplicate I found two additional inconsistencies not flagged in prior runs:
|
| /// <summary>RPC data type for SessionModeGet operations.</summary> | ||
| public class SessionModeGetResult | ||
| /// <summary>RPC data type for ModeGet operations.</summary> | ||
| public class ModeGetResult |
There was a problem hiding this comment.
Do we expect there to be additional data that's returned from this operation in the future? If not, could it just return a SessionMode instead of a wrapper around one?
| /// <summary>RPC data type for SessionModeSet operations.</summary> | ||
| public class SessionModeSetResult | ||
| /// <summary>RPC data type for ModeSet operations.</summary> | ||
| public class ModeSetResult |
There was a problem hiding this comment.
Is this needed (rather than void) because the set operation might fail?
| /// <summary>RPC data type for SessionPlanUpdate operations.</summary> | ||
| public class SessionPlanUpdateResult | ||
| /// <summary>RPC data type for PlanUpdate operations.</summary> | ||
| public class PlanUpdateResult |
There was a problem hiding this comment.
Some of these namings sound a little odd to my ears, e.g. PlanUpdateResult vs UpdatePlanResult. But not a big deal.
| [Experimental(Diagnostics.Experimental)] | ||
| public class SessionAgentListResult | ||
| public class AgentList |
There was a problem hiding this comment.
For C#, we could consider nesting types, having the request/response types for an RPC API be nested within the type that API is defined on, e.g. have AgentList be nested inside of AgentApi. That way, we minimize chances of conflict by giving them a namespace, e.g. when we add an API for getting a list of all subagents, if we were to just call them "agent", we'd have a conflict, but not if that listing method and the one for defined custom agents were on different types.
| [Experimental(Diagnostics.Experimental)] | ||
| public class SessionAgentGetCurrentResult | ||
| public class AgentCurrent |
There was a problem hiding this comment.
Another case where I'm wondering if the wrapper is adding any value. For cases where we already need to return multiple pieces of data, of course, and for cases where we think we will, makes sense... but in a case like this, I struggle to think what else we might need to return, in which case this needn't be in the public API surface area and the relevant method can just return an AgentCurrentAgent (which itself is a questionable name)
| @@ -711,13 +747,29 @@ internal class SessionAgentDeselectRequest | |||
| public string SessionId { get; set; } = string.Empty; | |||
| } | |||
|
|
|||
| /// <summary>RPC data type for SessionAgentReload operations.</summary> | |||
| /// <summary>RPC data type for AgentReloadAgent operations.</summary> | |||
| public class AgentReloadAgent | |||
There was a problem hiding this comment.
We seem to have multiple types that are identical other than in name, e.g. select agent, current agent, reload agent... can we do something to dedup those in the SDK? That would then also help with naming, as we wouldn't need to have multiple names for the exact same thing.
| [Experimental(Diagnostics.Experimental)] | ||
| public class SessionMcpDisableResult | ||
| public class McpDisableResult |
There was a problem hiding this comment.
Unless we plan to add more to a type like this, can we model that in the SDK by just having the method return void / Task instead of an empty dummy object?
|
|
||
| /// <summary>Per-model token and request metrics, keyed by model identifier.</summary> | ||
| [JsonPropertyName("modelMetrics")] | ||
| public Dictionary<string, UsageMetricsModelMetric> ModelMetrics { get => field ??= []; set; } |
There was a problem hiding this comment.
Separate from the naming, but seeing here makes me think of it, I think we'll want to update the generator and the manually-written models to use interfaces rather than these concrete types. It gives us more flexibility for what we choose to implement in the future (neither Dictionary or List allows for derivation, overriding, etc., which means we can't do things like modification tracking, throwing on things we consider misuse, etc.) and it allows consumers when setting these to use other types without needing to do a translation.
| { | ||
| /// <summary>Entry name.</summary> | ||
| [JsonPropertyName("name")] | ||
| public string Name { get; set; } = string.Empty; | ||
|
|
||
| /// <summary>Entry type.</summary> | ||
| [JsonPropertyName("type")] | ||
| public EntryType Type { get; set; } | ||
| public SessionFsReaddirWithTypesEntryType Type { get; set; } |
There was a problem hiding this comment.
For .NET, I expect we'd want "Readdir" to be "ReadDir" or "ReadDirectory".
| Plugin, | ||
| /// <summary>The <c>builtin</c> variant.</summary> | ||
| [JsonStringEnumMemberName("builtin")] | ||
| Builtin, |
|
Full disclosure 01: using my human brain Java subject matter expertise, Opus 4.6, and existing context, I arrived at these comments, in light of the comments Stephen posted to Slack. Full disclosure 02: The current agentic workflow derives the Java SDK directly from the post-code-generated parts of the reference implementation. I have an outstanding work item ( https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2751554 ) to make it so the Java SDK also uses the Zod schema, in addition to the agentic workflow to reduce toil in keeping the Java SDK in synch with the .NET and TypeScript reference implementations (See agentic workflow). Therefore, my comments are to be taken in light of what I would like to see before I undertake the work to implement dd-2751554-do-code-gen-for-java. Disclosures aside, I stand by the following comments. 1. Reinforce @stephentoub 's point on empty types — with a Java-specific angle
2. Reinforce the duplicate types point — with specific examples
3. Namespacing: Java has a different approach than C# nested types
4. Java-specific observation: the MCP filter mapping type explosion
5. Stability guarantee + Java sealed types interaction
6. Single-property wrappers: a different perspective from the Java side
|
There was a problem hiding this comment.
I know this isn't naming-related, but lots of APIs use double when they would be better represented as int, TimeSpan, etc. It would be nice if we could support a richer set of numeric types in generated code.
| /// <summary>RPC data type for ModelList operations.</summary> | ||
| public class ModelList |
There was a problem hiding this comment.
Would this be a good time to make these classes sealed?
| @@ -2297,17 +2513,17 @@ internal UiApi(JsonRpc rpc, string sessionId) | |||
| } | |||
|
|
|||
| /// <summary>Calls "session.ui.elicitation".</summary> | |||
| public async Task<SessionUiElicitationResult> ElicitationAsync(string message, SessionUiElicitationRequestRequestedSchema requestedSchema, CancellationToken cancellationToken = default) | |||
| public async Task<UiElicitationResponse> ElicitationAsync(string message, UiElicitationSchema requestedSchema, CancellationToken cancellationToken = default) | |||
There was a problem hiding this comment.
I think ElicitAsync might be a better name for this.
There was a problem hiding this comment.
Seems like this is a wrapper type that probably doesn't have to exist (could we just use List<Skill> directly)?
There was a problem hiding this comment.
I see similar patterns elsewhere (like McpList)
| public class McpList | ||
| { | ||
| /// <summary>Configured MCP servers.</summary> | ||
| [JsonPropertyName("servers")] | ||
| public List<Server> Servers { get => field ??= []; set; } | ||
| public List<McpServer> Servers { get => field ??= []; set; } | ||
| } |
There was a problem hiding this comment.
Even if we decide to keep these wrapper types, it seems like this one should be McpServerList.
| /// <summary>RPC data type for SessionUiElicitation operations.</summary> | ||
| public class SessionUiElicitationResult | ||
| /// <summary>The elicitation response (accept with form values, decline, or cancel).</summary> | ||
| public class UiElicitationResponse |
There was a problem hiding this comment.
Maybe a nit, but I think UI feels more natural than Ui.
| } | ||
|
|
||
| /// <summary>Token usage metrics for this model.</summary> | ||
| public class UsageMetricsModelMetricUsage |
There was a problem hiding this comment.
Some of these names still seem a bit unwieldy (though for cases like this I anticipate we could just use .withTypeName and override it).
| public class UsageMetricsModelMetric | ||
| { | ||
| /// <summary>Request count and cost metrics for this model.</summary> | ||
| [JsonPropertyName("requests")] | ||
| public UsageMetricsModelMetricRequests Requests { get => field ??= new(); set; } | ||
|
|
||
| /// <summary>Token usage metrics for this model.</summary> | ||
| [JsonPropertyName("usage")] | ||
| public UsageMetricsModelMetricUsage Usage { get => field ??= new(); set; } | ||
| } |
There was a problem hiding this comment.
I wonder if some of these would make sense as nested types.
4 participants
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.