Enhance C# generator with richer typing and data annotations#1067
Enhance C# generator with richer typing and data annotations#1067stephentoub wants to merge 4 commits intomainfrom
Conversation
- integer -> long, number -> double (was double for both) - format: date-time -> DateTimeOffset, uuid -> Guid, duration -> TimeSpan - Add MillisecondsTimeSpanConverter for TimeSpan JSON serialization - Emit [Range], [RegularExpression], [Url], [MinLength], [MaxLength] - Emit [StringSyntax(Uri)], [StringSyntax(Regex)], [Base64String] - Change all public collections from concrete to interface types (List<T> -> IList<T>, Dictionary<K,V> -> IDictionary<K,V>) - Lazy-initialize collection properties via field ??= pattern Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR upgrades the C# code generator and the .NET SDK surface to emit/consume richer types and more idiomatic collections, improving schema fidelity and public API flexibility across generated RPC/session-event models and hand-written types.
Changes:
- Updated
scripts/codegen/csharp.tsto map additional schema formats/types (e.g., integer→long, uri annotations) and to emit data-annotation attributes + duration handling hooks. - Switched many .NET public APIs/models from
List<T>/Dictionary<K,V>toIList<T>/IDictionary<K,V>with lazy initialization patterns. - Added a
TimeSpanJSON converter (milliseconds-based) and updated generated code + tests to match the new collection shapes.
Show a summary per file
| File | Description |
|---|---|
| scripts/codegen/csharp.ts | Adds richer type mapping, annotation emission, and collection interface generation in the C# generator. |
| dotnet/src/MillisecondsTimeSpanConverter.cs | Introduces a JsonConverter<TimeSpan> for millisecond-encoded durations. |
| dotnet/src/Types.cs | Updates hand-written SDK types to use IList/IDictionary and lazy init; adjusts copy semantics. |
| dotnet/src/Client.cs | Adjusts client API signatures to return/accept IList<T> and updates caching logic accordingly. |
| dotnet/src/Session.cs | Updates internal request/response DTOs to use interface collections. |
| dotnet/src/Generated/SessionEvents.cs | Regenerates session event models with updated integer typing and URI annotations. |
| dotnet/src/Generated/Rpc.cs | Regenerates RPC models to use interface collections and updated numeric typing where applicable. |
| dotnet/test/SessionTests.cs | Updates list search to LINQ (FirstOrDefault) to match interface collection return types. |
| dotnet/test/ElicitationTests.cs | Uses concrete dictionary construction where interface-typed properties can’t use target-typed new(). |
| dotnet/test/ClientTests.cs | Updates explicit types to IList<T> where needed for invariance/compatibility. |
Copilot's findings
Comments suppressed due to low confidence (1)
dotnet/src/Types.cs:1932
- Same as
SessionConfig: cloningMcpServersvianew Dictionary<string, McpServerConfig>(other.McpServers)drops any custom comparer on the original dictionary. If key comparison semantics matter (e.g., case-insensitive server names), preserve the comparer when possible (or explicitly define/normalize key casing).
InfiniteSessions = other.InfiniteSessions;
McpServers = other.McpServers is not null
? new Dictionary<string, McpServerConfig>(other.McpServers)
: null;
- Files reviewed: 9/11 changed files
- Comments generated: 4
scripts/codegen/csharp.ts
Outdated
| // [Range]for minimum/maximum | ||
| const hasMin = typeof schema.minimum === "number"; | ||
| const hasMax = typeof schema.maximum === "number"; | ||
| if (hasMin || hasMax) { | ||
| const min = hasMin ? String(schema.minimum) : (schema.type === "integer" ? "long.MinValue" : "double.MinValue"); | ||
| const max = hasMax ? String(schema.maximum) : (schema.type === "integer" ? "long.MaxValue" : "double.MaxValue"); | ||
| const namedArgs: string[] = []; | ||
| if (schema.exclusiveMinimum === true) namedArgs.push("MinimumIsExclusive = true"); | ||
| if (schema.exclusiveMaximum === true) namedArgs.push("MaximumIsExclusive = true"); | ||
| const namedSuffix = namedArgs.length > 0 ? `, ${namedArgs.join(", ")}` : ""; | ||
| attrs.push(`${indent}[Range(${min}, ${max}${namedSuffix})]`); |
There was a problem hiding this comment.
Fixed -- now uses Range(typeof(long), "min", "max") overload for integer types, which properly handles long values.
| // [Base64String] for base64-encoded string properties | ||
| if (format === "byte" || (schema as Record<string, unknown>).contentEncoding === "base64") { | ||
| attrs.push(`${indent}[Base64String]`); | ||
| } |
There was a problem hiding this comment.
Base64StringAttribute is a standard .NET 8+ attribute in System.ComponentModel.DataAnnotations (added alongside AllowedValues, DeniedValues, Length, etc.). This project targets net8.0 so it compiles fine. Currently no schema fields trigger it, but it will activate if the schema adds contentEncoding: "base64" or format: "byte" markup.
|
|
||
| lines.push(...xmlDocPropertyComment(prop.description, propName, " ")); | ||
| lines.push(...emitDataAnnotations(prop, " ")); | ||
| if (isDurationProperty(prop)) lines.push(` [JsonConverter(typeof(MillisecondsTimeSpanConverter))]`); |
There was a problem hiding this comment.
Fixed -- added using GitHub.Copilot.SDK; to the Rpc.cs header so the converter type resolves correctly when duration fields are present.
dotnet/src/Types.cs
Outdated
| InfiniteSessions = other.InfiniteSessions; | ||
| McpServers = other.McpServers is not null | ||
| ? new Dictionary<string, McpServerConfig>(other.McpServers, other.McpServers.Comparer) | ||
| ? new Dictionary<string, McpServerConfig>(other.McpServers) |
There was a problem hiding this comment.
Already fixed in an earlier commit (56ba3cf). Both SessionConfig and ResumeSessionConfig clone constructors now check if the underlying IDictionary is a Dictionary<string, McpServerConfig> and preserve its Comparer when cloning.
This comment has been minimized.
This comment has been minimized.
Preserve dictionary comparer in SessionConfig/ResumeSessionConfig Clone() by checking for Dictionary<> and passing its Comparer. Fix byok.md to use Task.FromResult<IList<ModelInfo>>() for the updated delegate signature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Use Range(typeof(long), ...) overload since RangeAttribute has no long constructor. Add 'using GitHub.Copilot.SDK' to Rpc.cs header so MillisecondsTimeSpanConverter resolves when duration fields exist. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅This PR is entirely focused on the .NET/C# SDK (codegen script + generated/hand-written files). Here's a quick cross-SDK check:
Notable: The No cross-SDK consistency concerns found.
|
The C# code generator (
scripts/codegen/csharp.ts) emits types from JSON Schema but was leaving a lot of type information on the table -- usingdoublefor all numerics,stringfor dates and durations, no validation attributes, and concrete collection types in public APIs. This PR teaches the generator to produce idiomatic, well-annotated C# and applies the same patterns to the hand-written public API surface.What changed
Richer type mappings in the generator:
integer->long(JSON Schema integers can be up to 2^53, sointwould be lossy)number->double(unchanged, but now distinct from integer)format: "date-time"->DateTimeOffsetformat: "uuid"->Guidformat: "duration"on numeric fields ->TimeSpanwith a[JsonConverter]that converts milliseconds (the runtime's convention for duration fields differs from JSON Schema's ISO 8601 string definition -- see comment onisDurationProperty())Data annotation attributes:
[Range]fromminimum/maximum/exclusiveMinimum/exclusiveMaximum[RegularExpression]frompattern[Url]and[StringSyntax(StringSyntaxAttribute.Uri)]fromformat: "uri"[StringSyntax(StringSyntaxAttribute.Regex)]fromformat: "regex"[MinLength]/[MaxLength]fromminLength/maxLength[Base64String]fromcontentEncoding: "base64"orformat: "byte"(no current schema fields use this yet, but the generator is wired up)Collection interfaces in the public API:
List<T>properties/returns/parameters ->IList<T>Dictionary<K,V>->IDictionary<K,V>Types.cs,Client.cs,Session.cs)field ??= []/field ??= new Dictionary<>()) instead of eager allocationNew file:
dotnet/src/MillisecondsTimeSpanConverter.cs-- a standaloneJsonConverter<TimeSpan>that serializes TimeSpan as milliseconds. Marked[EditorBrowsable(Never)]since it's an implementation detail.Things to note
format: "date-time"andformat: "uri", soSessionEvents.csgets fewer annotations thanRpc.cs(which has ranges, patterns, durations, etc.).IList<T>types whereTask<T>invariance requires it,new Dictionary<>()where[]doesn't work forIDictionary, and.FirstOrDefault()replacingList.Find().[DefaultValue]and[AllowedValues]attributes were investigated but not added -- the schemas have almost nodefaultvalues and string enums are already mapped to C# enum types.