Add minimal chat samples for all SDK languages#492
Conversation
SteveSandersonMS
force-pushed
the
stevesa/add-samples
branch
3 times, most recently
from
ced2b4f to
f7f81c2
Compare
There was a problem hiding this comment.
Pull request overview
Adds minimal interactive “chat” samples for each language SDK in this mono-repo, plus README sections that explain how to run them. This fits the repo’s goal of providing SDK entry points that talk to the Copilot CLI via JSON-RPC by giving users a copy/paste starting point in each language.
Changes:
- Add interactive chat samples for Node.js, Python, Go, and .NET.
- Add “Run the Sample” sections to each language README.
- Add Node and Go sample module scaffolding (Node
package.json, Gogo.mod) to run samples standalone.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| python/samples/chat.py | New async chat loop sample (currently has API/typing mismatches and output issues). |
| python/README.md | Adds “Run the Sample” instructions for Python. |
| nodejs/samples/package.json | New sample package scaffolding (currently depends on built dist/ which isn’t present). |
| nodejs/samples/chat.ts | New TS chat loop sample (currently doesn’t enable streaming and uses wrong delta field name). |
| nodejs/README.md | Adds “Run the Sample” instructions for Node (currently missing required build step / approach). |
| go/samples/go.mod | New Go sample module with replace to local SDK. |
| go/samples/chat.go | New Go chat loop sample (currently ignores returned errors and doesn’t enable streaming). |
| go/README.md | Adds “Run the Sample” instructions for Go. |
| dotnet/samples/Chat.csproj | New .NET sample project referencing SDK project. |
| dotnet/samples/Chat.cs | New .NET top-level chat loop sample (currently doesn’t enable streaming; EOF handling/output issues). |
| dotnet/README.md | Adds “Run the Sample” instructions for .NET. |
Comments suppressed due to low confidence (4)
go/samples/chat.go:25
session.Destroy()returns an error; deferring it without checking can hide failures (and also makes debugging harder if destroy fails). Consider handling/logging the error in the deferred call.
defer session.Destroy()
go/samples/chat.go:50
SendAndWaitreturns(*SessionEvent, error). The current call ignores the error, so failures (including timeouts / session errors) will be missed and the prompt loop will continue as if nothing happened. Capture and handle the error (and optionally use the returned final message).
}
}
nodejs/samples/chat.ts:31
- There’s no graceful shutdown path (session destroy / client stop / readline close). Consider handling SIGINT/EOF to cleanly
await session.destroy(); await client.stop(); rl.close();before exiting, since the client may spawn a CLI subprocess.
main().catch(console.error);
dotnet/samples/Chat.cs:24
- The loop prints an "Assistant: " prefix before calling
SendAndWaitAsync, but the event handler also prints a\nAssistant: ...line forAssistantMessageEvent. This produces duplicated/garbled output; prefer letting the handler own assistant output (remove the pre-send prefix) or only printing deltas during the call.
Console.WriteLine();
}
| defer client.Stop() | ||
|
|
||
| session, err := client.CreateSession(ctx, &copilot.SessionConfig{Streaming: true}) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| defer session.Destroy() |
There was a problem hiding this comment.
client.Stop() returns an error (it can aggregate cleanup errors). Deferring it without checking means cleanup failures are silently lost; wrap the defer in a function and handle/log the returned error.
This issue also appears in the following locations of the same file:
- line 25
- line 49
| defer client.Stop() | |
| session, err := client.CreateSession(ctx, &copilot.SessionConfig{Streaming: true}) | |
| if err != nil { | |
| panic(err) | |
| } | |
| defer session.Destroy() | |
| defer func() { | |
| if err := client.Stop(); err != nil { | |
| fmt.Fprintln(os.Stderr, "failed to stop Copilot client:", err) | |
| } | |
| }() | |
| session, err := client.CreateSession(ctx, &copilot.SessionConfig{Streaming: true}) | |
| if err != nil { | |
| panic(err) | |
| } | |
| defer func() { | |
| if err := session.Destroy(); err != nil { | |
| fmt.Fprintln(os.Stderr, "failed to destroy Copilot session:", err) | |
| } | |
| }() |
python/samples/chat.py
Outdated
| async with CopilotClient() as client: | ||
| session = await client.create_session({"streaming": True}) |
There was a problem hiding this comment.
CopilotClient does not implement an async context manager (aenter/aexit), so async with CopilotClient() will raise at runtime. Use client = CopilotClient(); await client.start() and ensure await client.stop() is called in a finally block (and destroy the session as well).
python/samples/chat.py
Outdated
| async with CopilotClient() as client: | ||
| session = await client.create_session({"streaming": True}) | ||
|
|
There was a problem hiding this comment.
This sample expects assistant.message_delta events, but streaming is opt-in; create the session with {"streaming": True} (or equivalent) so the delta branch can ever run.
go/samples/chat.go
Outdated
| if event.Type == copilot.AssistantMessageDelta && event.Data.DeltaContent != nil { | ||
| fmt.Print(*event.Data.DeltaContent) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The loop prints an "Assistant: " prefix before sending, but the event handler also prints a full assistant line on AssistantMessage. This produces duplicated/garbled output (and duplicates content if deltas are printed). Prefer either printing only deltas and using the final message event just to end the line, or letting the handler print the final message and removing the pre-send prefix.
dotnet/samples/Chat.cs
Outdated
| using GitHub.Copilot.SDK; | ||
|
|
||
| await using var client = new CopilotClient(); | ||
| await using var session = await client.CreateSessionAsync(new SessionConfig { Streaming = true }); |
There was a problem hiding this comment.
The sample prints AssistantMessageDeltaEvent, but streaming is disabled by default. Create the session with a SessionConfig { Streaming = true } so delta events are actually emitted.
dotnet/samples/Chat.cs
Outdated
| if (evt is AssistantMessageDeltaEvent delta) | ||
| Console.Write(delta.Data?.DeltaContent); | ||
| }); | ||
|
|
There was a problem hiding this comment.
If streaming deltas are printed, printing the full AssistantMessageEvent content afterwards will duplicate the response. Consider ignoring the final content when streaming (or accumulate deltas and print once).
python/samples/chat.py
Outdated
| if event.type == "assistant.message_delta": | ||
| print(event.data.delta, end="", flush=True) | ||
|
|
||
| session.on(on_event) |
There was a problem hiding this comment.
event.type is a SessionEventType enum (not a string), and the delta field is event.data.delta_content (not delta). Update the comparisons to use SessionEventType.ASSISTANT_MESSAGE / .ASSISTANT_MESSAGE_DELTA (or compare event.type.value) and print event.data.delta_content.
python/samples/chat.py
Outdated
| if event.type == "assistant.message_delta": | ||
| print(event.data.delta, end="", flush=True) | ||
|
|
There was a problem hiding this comment.
When streaming is enabled, assistant.message_delta events arrive followed by a final assistant.message. Printing the full assistant.message content after printing deltas will duplicate the response. Consider ignoring assistant.message content when streaming (or accumulate deltas and print once).
| const client = new CopilotClient(); | ||
| const session = await client.createSession({ streaming: true }); | ||
|
|
There was a problem hiding this comment.
The sample handles assistant.message_delta, but streaming is disabled by default. Create the session with { streaming: true } (e.g., await client.createSession({ streaming: true })) so delta events are actually emitted.
nodejs/samples/chat.ts
Outdated
|
|
||
| session.on((event: SessionEvent) => { | ||
| if (event.type === "assistant.message_delta") { | ||
| process.stdout.write(event.data?.delta ?? ""); |
There was a problem hiding this comment.
For assistant.message_delta events the field is event.data.deltaContent (see generated session event types), not event.data.delta. Using delta will always print empty output.
| process.stdout.write(event.data?.delta ?? ""); | |
| process.stdout.write(event.data?.deltaContent ?? ""); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
python/samples/chat.py
Outdated
|
|
||
|
|
||
| async def main(): | ||
| async with CopilotClient() as client: |
There was a problem hiding this comment.
Critical Bug: The Python SDK does not implement async context manager (__aenter__ and __aexit__ methods), so this async with syntax will fail with AttributeError: __aenter__.
The Python sample should follow the explicit lifecycle pattern like Go:
async def main():
client = CopilotClient()
await client.start()
try:
session = await client.create_session({"streaming": True})
# ... rest of code
finally:
await client.stop()Cross-SDK Inconsistency: This reveals different lifecycle management patterns across SDKs:
- .NET: Uses
await usingfor automatic disposal ✅ - Go: Explicitly calls
Start()/Stop()withdefer✅ - Python: Should use explicit
start()/stop()(context manager not implemented) - Node.js: No cleanup shown (potential resource leak)
AI generated by SDK Consistency Review Agent for #492
python/samples/chat.py
Outdated
|
|
||
| def on_event(event): | ||
| if event.type == "assistant.message_delta": | ||
| print(event.data.delta, end="", flush=True) |
There was a problem hiding this comment.
Bug: The field name should be delta_content (snake_case), not delta.
According to python/copilot/generated/session_events.py:736, the field is named delta_content:
print(event.data.delta_content, end="", flush=True)Cross-SDK Note: Field names follow language conventions:
- Python:
event.data.delta_content(snake_case) - Node.js:
event.data.deltaContent(camelCase) - Go:
event.Data.DeltaContent(PascalCase) - .NET:
delta.Data.DeltaContent(PascalCase)
AI generated by SDK Consistency Review Agent for #492
Cross-SDK Consistency ReviewThis PR adds chat samples for all four SDKs, which is great! However, I've identified several critical bugs and consistency issues that need to be addressed: 🐛 Critical Bugs
|
| SDK | Pattern | Status |
|---|---|---|
| .NET | await using (automatic disposal) |
✅ Proper RAII |
| Go | Explicit Start()/Stop() + defer |
✅ Explicit cleanup |
| Python | Attempted async with |
❌ Not implemented |
| Node.js | No cleanup shown |
Recommendation: Each SDK should demonstrate proper cleanup following its language's idioms:
- Python should use explicit
await client.start()/await client.stop()with try/finally - Node.js should either handle SIGINT or document that cleanup happens on process exit
Field Naming (Working as Designed)
The event field names correctly follow language conventions:
- Python:
delta_content(snake_case) ✅ - Node.js:
deltaContent(camelCase) ✅ - Go:
DeltaContent(PascalCase) ✅ - .NET:
DeltaContent(PascalCase) ✅
However, the samples are accessing the wrong field names (see inline comments).
📝 Summary
The samples demonstrate equivalent functionality across languages, but the bugs will prevent Python and Node.js samples from running correctly. Once these are fixed, the samples will be excellent references for developers!
AI generated by SDK Consistency Review Agent
nodejs/samples/chat.ts
Outdated
|
|
||
| session.on((event: SessionEvent) => { | ||
| if (event.type === "assistant.message_delta") { | ||
| process.stdout.write(event.data?.delta ?? ""); |
There was a problem hiding this comment.
Bug: The field name should be deltaContent (camelCase), not delta.
According to nodejs/src/generated/session-events.ts, the field is named deltaContent:
process.stdout.write(event.data?.deltaContent ?? "");AI generated by SDK Consistency Review Agent for #492
| import { CopilotClient, type SessionEvent } from "@github/copilot-sdk"; | ||
|
|
||
| async function main() { | ||
| const client = new CopilotClient(); |
There was a problem hiding this comment.
Cross-SDK Inconsistency - Missing Cleanup: The Node.js sample doesn't close/cleanup the client, which may leave the CLI subprocess running.
For consistency with other SDKs and proper resource management, consider adding cleanup. While Node.js doesn't have a built-in disposal pattern like .NET's using or Python's context managers, you could:
- Handle process exit signals:
process.on('SIGINT', async () => {
await session.destroy();
await client.stop();
process.exit();
});- Or document that the CLI process terminates when the parent exits (if that's the intended behavior).
Note: Go uses explicit defer, .NET uses await using, but Node.js has no cleanup shown.
AI generated by SDK Consistency Review Agent for #492
- Add samples/chat.ts for Node.js/TypeScript - Add samples/chat.py for Python - Add samples/chat.go for Go - Add samples/Chat.cs for .NET - Update each SDK README with instructions to run the sample Each sample is a minimal (~30-50 lines) interactive chat loop that: - Prompts user for input - Sends to Copilot and waits for idle - Streams response deltas to console Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesa/add-samples
branch
from
f7f81c2 to
ee9df36
Compare
✅ Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations. Great work! The samples maintain excellent consistency while respecting language-specific idioms. Consistency SummaryAll four samples (Node.js, Python, Go, .NET) demonstrate: ✅ Parallel structure and flow:
✅ Consistent API naming (following language conventions):
✅ Consistent event types: ✅ README documentation: All four READMEs get "Run the Sample" sections with consistent formatting Language-Specific Idioms (Appropriate Differences)The samples correctly use language-specific patterns:
No consistency issues found. The PR successfully adds feature-equivalent samples to all four SDK languages! 🎉
|
python/samples/chat.py
Outdated
|
|
||
|
|
||
| async def main(): | ||
| async with CopilotClient() as client: |
There was a problem hiding this comment.
Inconsistency: Python sample uses unsupported async context manager
The Python SDK's CopilotClient does not implement __aenter__/__aexit__ methods, so using async with CopilotClient() as client: will fail at runtime.
Based on the E2E tests and SDK documentation, the correct pattern should be:
client = CopilotClient()
try:
await client.start()
session = await client.create_session()
# ... rest of the code
finally:
await client.stop()This matches the pattern used in the .NET, Go, and Node.js samples (accounting for language idioms). Each explicitly manages the client lifecycle with start/stop or using/defer statements.
AI generated by SDK Consistency Review Agent for #492
Cross-SDK Consistency ReviewThank you for adding chat samples for all four SDK languages! This is a great addition that helps developers get started quickly. I've reviewed the samples for cross-language consistency and found two issues: 🔴 Critical Issue: Python sample will not runThe Python sample uses Fix needed: Update the Python sample to use explicit 🟡 Minor Inconsistency: .NET sample missing tool argumentsThe .NET sample only logs ✅ Otherwise well done!The samples are remarkably consistent in:
Once the Python runtime issue is fixed, these will be excellent getting-started examples!
|
| Console.WriteLine($"[reasoning: {reasoning.Data.Content}]"); | ||
| break; | ||
| case ToolExecutionStartEvent tool: | ||
| Console.WriteLine($"[tool: {tool.Data.ToolName}]"); |
There was a problem hiding this comment.
Minor inconsistency: Missing tool arguments in output
The .NET sample only logs the tool name, while the Node.js, Python, and Go samples all include the tool arguments in their output.
For consistency across all SDK samples, consider adding the arguments:
case ToolExecutionStartEvent tool:
var args = System.Text.Json.JsonSerializer.Serialize(tool.Data.Arguments);
Console.WriteLine($"[tool: {tool.Data.ToolName} {args}]");
break;This would match the pattern in:
- Node.js (line 13):
`[tool: ${event.data.toolName} ${JSON.stringify(event.data.arguments)}]` - Python (line 18):
f"[tool: {event.data.tool_name} {json.dumps(event.data.arguments)}]" - Go (lines 40-41):
fmt.Sprintf("[tool: %s %s]", *event.Data.ToolName, args)
AI generated by SDK Consistency Review Agent for #492
event.type is a SessionEventType enum, not a string - use .value for comparison Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅I've reviewed this PR which adds minimal interactive chat samples for all four SDK languages (Node.js, Python, Go, .NET). Overall AssessmentThe samples are well-structured and demonstrate equivalent functionality across all SDKs:
Issue FoundOne consistency issue identified: The Python sample is missing explicit cleanup of the client and session, while Go uses README ConsistencyAll four README files have been updated with a "Run the Sample" section showing the appropriate commands for each language. ✅ Great work on maintaining feature parity across all SDKs! 🎉
|
|
|
||
| async def main(): | ||
| client = CopilotClient() | ||
| await client.start() |
There was a problem hiding this comment.
Cross-SDK consistency issue: The Python sample is missing cleanup of the client and session.
For consistency with Go (which uses defer client.Stop() and defer session.Destroy()) and .NET (which uses await using for automatic disposal), the Python sample should properly clean up resources.
Suggested fix - update main() to use proper cleanup:
async def main():
client = CopilotClient()
await client.start()
try:
session = await client.create_session()
# ... event handler and chat loop ...
finally:
await client.stop()This would match the explicit resource management patterns in Go and .NET SDKs.
AI generated by SDK Consistency Review Agent for #492
- Add CLIArgs option to ClientOptions for passing extra CLI arguments - Capture stderr from CLI process for better error messages - Add processDone channel to signal when CLI exits unexpectedly - Propagate process exit errors to pending JSON-RPC requests - Add E2E test verifying error reporting when CLI fails to start 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 ReviewThanks for adding chat samples across all four SDKs! The samples demonstrate excellent consistency in structure and functionality. However, I've identified one API inconsistency that this PR introduces. ✅ Chat Samples - Excellent ConsistencyThe chat samples across all four SDKs follow parallel patterns:
❌ API Inconsistency:
|
| SDK | Field Name | Status |
|---|---|---|
| Node.js | cliArgs |
✅ Already exists |
| .NET | CliArgs |
✅ Already exists |
| Go | CLIArgs |
✅ Added in this PR |
| Python | cli_args |
❌ Does not exist |
Impact: Users cannot pass custom CLI arguments when creating a CopilotClient in Python, while all other SDKs support this.
Recommendation: Add cli_args: list[str] to Python's CopilotClientOptions in python/copilot/types.py and implement the corresponding logic in python/copilot/client.py to maintain feature parity across all SDKs.
Note: The Go changes also include improved error handling (capturing stderr for better error messages when CLI fails to start). This is a quality improvement and doesn't need to be mirrored in other SDKs immediately, though it could be beneficial to add similar error reporting to the other SDKs in a future PR.
AI generated by SDK Consistency Review Agent
Node.js: - Add stderrBuffer and processExitPromise for early failure detection - Race verifyProtocolVersion against process exit - Add test: should report error with stderr when CLI fails to start Python: - Add cli_args option to CopilotClientOptions - Add ProcessExitedError and stderr capture in jsonrpc - Add test: test_should_report_error_with_stderr_when_cli_fails_to_start .NET: - Add StderrBuffer to capture CLI stderr output - Handle ConnectionLostException with stderr in error message - Add test: Should_Report_Error_With_Stderr_When_CLI_Fails_To_Start Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
✅ Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, .NET). Great work maintaining feature parity! SummaryThis PR adds minimal interactive chat samples and improves error handling across all SDKs. The changes demonstrate excellent cross-language consistency. ✅ What's Consistent1. Chat Samples - All four implementations demonstrate the same functionality:
2. Error Handling - All SDKs now capture and report CLI stderr consistently:
3. CLI Args Support - All SDKs support custom CLI arguments:
All prepend user args before SDK-managed flags consistently. 4. Test Coverage - All SDKs include E2E tests verifying stderr capture with the same approach (passing invalid 🎯 API NamingThe naming conventions properly follow each language's idioms:
📝 RecommendationNo changes needed - this PR maintains excellent cross-SDK consistency. The samples provide a great getting-started experience that's parallel across all languages, and the error handling improvements benefit all users equally. The only minor observation: README updates are consistent, all pointing users to run their respective samples. Nice touch adding the "Run the Sample" sections!
|
Go SDK: - Change processDone from buffered error channel to closed signal channel - Store processError separately and use mutex for thread-safe access - Check for process exit before sending requests (fail fast) - Subsequent requests after process exit now get the same stderr error All SDKs: - Enhanced CLI error tests to verify subsequent calls also fail properly - Tests now call createSession/send instead of just ping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Node.js: expect 'Connection is closed' - Python: expect 'Invalid argument' - .NET: expect 'exited' 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>
| Assert.Contains("exited", ex2.Message, StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| // Cleanup - ForceStop should handle the disconnected state gracefully | ||
| try { await client.ForceStopAsync(); } catch (Exception) { /* Expected */ } |
|
|
||
| def on_event(event): | ||
| output = None | ||
| if event.type.value == "assistant.reasoning": |
There was a problem hiding this comment.
Cross-SDK Consistency Issue: Python uses event.type.value to access the event type, while Node.js, Go, and .NET access event.type directly.
This inconsistency suggests either:
- Python's
event.typeis an enum that requires.valueaccess, or - The API should be normalized to allow direct string comparison like the other SDKs
Consider whether the Python SDK could support direct comparison (e.g., event.type == "assistant.reasoning") for consistency with other languages.
AI generated by SDK Consistency Review Agent for #492
| import { CopilotClient, type SessionEvent } from "@github/copilot-sdk"; | ||
|
|
||
| async function main() { | ||
| const client = new CopilotClient(); |
There was a problem hiding this comment.
Cross-SDK Consistency Issue: The Node.js sample doesn't call any start method on the client, while Python calls await client.start() and Go calls client.Start(ctx) before using the client.
For consistency, if Node.js clients don't require an explicit start call (auto-start on first use), this should be documented. Alternatively, if Node.js supports a start() method, it should be called here to match the pattern in Python and Go samples.
AI generated by SDK Consistency Review Agent for #492
| try: | ||
| asyncio.run(main()) | ||
| except KeyboardInterrupt: | ||
| print("\nBye!") |
There was a problem hiding this comment.
Cross-SDK Consistency Issue: The Python sample lacks resource cleanup. According to the memory, Python CopilotClient must be started/stopped explicitly.
For consistency with Go (which uses defer client.Stop()) and .NET (which uses await using), consider adding:
async def main():
client = CopilotClient()
try:
await client.start()
session = await client.create_session()
# ... rest of code ...
finally:
await client.stop()This ensures proper cleanup when the user presses Ctrl+C.
AI generated by SDK Consistency Review Agent for #492
| } | ||
| } | ||
|
|
||
| main().catch(console.error); |
There was a problem hiding this comment.
Cross-SDK Consistency Issue: The Node.js sample lacks resource cleanup, while:
- Go uses
defer client.Stop()anddefer session.Destroy() - .NET uses
await usingfor automatic disposal - Python should also add cleanup but currently doesn't
Consider adding cleanup in a try-finally block or using signal handlers for graceful shutdown to match the cleanup patterns in other SDKs.
AI generated by SDK Consistency Review Agent for #492
Cross-SDK Consistency ReviewThank you for adding samples across all four SDKs! This is a great addition for users. However, I've identified several consistency issues that should be addressed: 🔴 Critical: Client Lifecycle ManagementThe samples have inconsistent client initialization patterns:
Recommendation: Document whether auto-start is intentional for Node.js/.NET, or align all SDKs to require (or not require) explicit start calls. 🟡 Important: Resource CleanupThe samples have inconsistent cleanup patterns:
Recommendation: Add proper cleanup to Node.js and Python samples. Python's documentation indicates 🟡 Minor: Event Type Access
This may be a language-specific requirement, but consider whether Python's API could support direct comparison for consistency. Summary: While these are sample/demo files, they serve as the primary learning resource for SDK users. Ensuring consistency in lifecycle management and resource cleanup patterns across languages will help users avoid common pitfalls and understand best practices. I've added specific inline comments on each file with suggestions for alignment.
|
|
|
||
| func main() { | ||
| ctx := context.Background() | ||
| cliPath := filepath.Join("..", "..", "nodejs", "node_modules", "@github", "copilot", "index.js") |
There was a problem hiding this comment.
Note: This hardcoded CLI path is specific to the repo structure and won't work for users running the sample outside the repo. Consider documenting this or using the default CLI discovery mechanism (which would match the other SDK samples that don't specify CLIPath).
AI generated by SDK Consistency Review Agent for #492
- Go: Add sync.WaitGroup to ensure stderr is fully read before accessing buffer - Python: Accept platform-specific error messages (EINVAL on Windows, EPIPE on Linux) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Cross-SDK Consistency ReviewI've analyzed this PR across all four SDK implementations and found excellent consistency in both features and API design. The PR successfully implements two key improvements uniformly across all SDKs: 1. ✅ CLI Args SupportAll SDKs now properly support passing custom CLI arguments:
All implementations correctly prepend user args before SDK-managed args, maintaining consistent behavior. 2. ✅ Enhanced Error Handling for CLI Process ExitAll SDKs now capture and include Implementation pattern (consistent across all):
Node.js: Uses 3. ✅ Chat SamplesEach SDK includes a minimal chat sample (~30-50 lines) demonstrating:
Naming consistency (accounting for language conventions):
SummaryNo consistency issues found. The PR demonstrates excellent adherence to cross-SDK design principles:
Great work! 🎉
|
StderrPipe gets closed by cmd.Wait() before io.Copy can finish reading. Using cmd.Stderr = &buffer ensures all stderr is captured before Wait returns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use StderrPipe with explicit ReadAll in goroutine, wait for EOF signal before accessing buffer. This ensures all stderr data is captured even when the process exits quickly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅I've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, .NET). Overall, excellent work maintaining feature parity! 🎉 ✅ Consistent Implementations1. CLI Args Support
The implementation is consistent - user args are prepended before SDK-managed args in all languages. 2. Error Handling for CLI Termination
3. Chat Samples
4. Tests 📝 Minor IssueI've left one inline comment about adding XML documentation to the .NET Summary: This PR successfully maintains cross-SDK consistency while adding valuable features (CLI args support, better error handling) and examples (chat samples) to all four languages. The only minor issue is a missing XML doc comment in .NET.
|
✅ Cross-SDK Consistency ReviewI've completed a comprehensive review of this PR for consistency across all four SDK implementations (Node.js, Python, Go, and .NET). This PR demonstrates excellent cross-language consistency! Features Verified1. CLI Args Support ✅All SDKs now support passing extra CLI arguments with proper language-specific naming:
2. Error Handling for CLI Process Exit ✅All SDKs implement consistent error handling when the CLI terminates unexpectedly:
All implementations:
3. Test Coverage ✅All SDKs include parallel test cases:
4. Chat Samples ✅All four languages have consistent interactive chat samples (~30-50 lines):
5. Documentation ✅All READMEs updated with "Run the Sample" sections providing clear build/run instructions. SummaryNo consistency issues found. This PR adds two important features (CLI args support and improved error handling) plus helpful samples, and does so with excellent feature parity across all four SDK languages. Great work maintaining consistency! 🎉
|
1 participant
This PR adds minimal interactive chat samples for each SDK language (Node.js, Python, Go, .NET).
While doing this it became clear some other fundamentals needed to be fixed:
stderroutput written by the underlying CLI)Changes
Each sample is ~30-50 lines and demonstrates:
Also updates each language README with a Run the Sample section.
Testing
Verified builds compile: