Fix update-protocol-version.ts templates that break the Go and .NET builds - #2554
Open
rinceyuan wants to merge 1 commit into
Open
Fix update-protocol-version.ts templates that break the Go and .NET builds#2554rinceyuan wants to merge 1 commit into
rinceyuan wants to merge 1 commit into
Conversation
…mitted files The Go template emitted SdkProtocolVersion/GetSdkProtocolVersion and the C# template emitted namespace GitHub.Copilot.SDK. Both were corrected in the committed outputs by github#1527 (Go initialism casing) and github#1343 (C# API review) without updating this generator, so running it reverted them and broke the Go and .NET builds. Also add the Rust output, generated since github#1164, to the header comment.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes match existing call sites and regenerate all outputs exactly.
Pull request overview
Aligns the protocol-version generator with canonical Go and .NET APIs.
Changes:
- Fixes Go identifier casing and the C# namespace.
- Documents the Rust output.
- Verified all five templates match committed outputs.
File summaries
| File | Description |
|---|---|
nodejs/scripts/update-protocol-version.ts |
Corrects generated Go/C# code and output documentation. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
nodejs/scripts/update-protocol-version.tsis the only generator for the fiveSdkProtocolVersionconstant files, and it is wired up asnpm run update:protocol-version. Its Go and C# templates have drifted from the files they are supposed to produce, so running it today reverts two deliberate fixes and breaks the Go and .NET builds.SdkProtocolVersion/GetSdkProtocolVersionSDKProtocolVersion/GetSDKProtocolVersionnamespace GitHub.Copilot.SDK;namespace GitHub.Copilot;How it drifted
Both outputs are marked
DO NOT EDIT, but both were hand-corrected later — for good reasons — without updating the templates:GitHub.CopilotSdk→SDKNothing caught it:
codegen-check.ymlrunsscripts/codegen, which is a different generator and does not produce these files. No workflow runsupdate:protocol-version.Reproducing the breakage
On
main, before this change:SdkProtocolVersionis aninternal static class, andClient.csis inGitHub.Copilotwith nousingfor the nested namespace, so moving it breaks the reference on every target framework.Verification after this change
Regenerating is now a no-op — the generator reproduces all five committed files exactly:
(On a Windows checkout the five files show as modified after regenerating, because the generator writes LF while the working tree is CRLF. The content is identical — confirm with
git diff --ignore-all-space.)Builds pass with the regenerated files:
Also included
The header comment listed four output files; the generator has written five since #1164 added the Rust output. Added
rust/src/sdk_protocol_version.rsto the list.Not addressed here
java/sdk/src/main/java/com/github/copilot/SdkProtocolVersion.javacarries the sameCode generated by update-protocol-version.ts. DO NOT EDIT.header, but this script does not write it —codegen-check.ymlvalue-checks it separately instead. That looked like a distinct concern, so I left it alone.The generator's existing Prettier deviations are also untouched:
nodejsonly runsformat:check/lintoversrc/**andtest/**, and the file does not satisfyprettier --checkonmaineither, so reformatting would have been unrelated noise.