Remove autoRestart feature across all SDKs#803
Conversation
The autoRestart option never worked correctly. This removes it from: - Node.js: types, client options, reconnect logic - Python: types, client options - Go: types, client options, struct field - .NET: types, clone copy, tests - Docs: setup, troubleshooting, READMEs - Agent config: docs-maintenance validation lists Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes the non-functional autoRestart / auto_restart / AutoRestart option across the Node.js, Python, Go, and .NET SDKs, and cleans up all related documentation and docs-maintenance validation to reflect the simplified client lifecycle behavior.
Changes:
- Removed the
autoRestartoption from all SDK option types/APIs and their initialization/default handling. - Removed Node.js reconnect-on-exit/on-close logic and the private
reconnect()helper tied toautoRestart. - Updated SDK READMEs and troubleshooting/setup docs plus docs-maintenance option lists to no longer mention auto-restart.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/copilot/types.py | Removes auto_restart from CopilotClientOptions TypedDict. |
| python/copilot/client.py | Removes auto_restart from normalized options initialization. |
| python/README.md | Removes auto_restart from examples and option documentation. |
| nodejs/src/types.ts | Removes autoRestart from CopilotClientOptions interface docs/type. |
| nodejs/src/client.ts | Removes autoRestart default, reconnect-on-exit/on-close behavior, and reconnect() implementation. |
| nodejs/README.md | Removes autoRestart from documented constructor options. |
| go/types.go | Removes AutoRestart from ClientOptions and updates related helper comment. |
| go/client.go | Removes autoRestart resolved option field and option plumbing. |
| go/README.md | Removes AutoRestart from option docs and helper function description. |
| dotnet/test/CloneTests.cs | Removes AutoRestart from clone test setup and assertions. |
| dotnet/src/Types.cs | Removes AutoRestart property and clone-copy behavior. |
| dotnet/README.md | Removes AutoRestart from documented options. |
| docs/troubleshooting/debugging.md | Removes auto-restart troubleshooting step. |
| docs/setup/local-cli.md | Removes autoRestart from local CLI setup snippet. |
| .github/agents/docs-maintenance.agent.md | Removes auto-restart mentions from “must match”/core-features lists. |
Comments suppressed due to low confidence (1)
docs/troubleshooting/debugging.md:305
- The numbered list under the "Connection refused" solution now skips from step 1 to step 3 after removing the auto-restart step. Renumber the remaining items so the sequence is continuous.
1. Check if the CLI runs correctly standalone:
```bash
copilot --server --stdio
- Check for port conflicts if using TCP mode:
const client = new CopilotClient({ useStdio: false, port: 0, // Use random available port });
</details>
✅ Cross-SDK Consistency Review: APPROVEDThis PR demonstrates exemplary cross-language consistency in removing the Changes Verified Across All SDKs:Node.js/TypeScript (
|
Instead of leaving onClose/onError as no-ops (which would leave the client in a stale 'connected' state), transition to 'disconnected' so callers fail fast or can re-start cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All SDKs now properly transition their connection state to 'disconnected' when the child process exits unexpectedly or the TCP connection drops: - Node.js: onClose/onError handlers in attachConnectionHandlers() - Go: onClose callback fired from readLoop() on unexpected exit - Python: on_close callback fired from _read_loop() on unexpected exit - .NET: rpc.Completion continuation sets _disconnected flag Includes unit tests for all four SDKs verifying the state transition. 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}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Cross-SDK Consistency Review: PASSEDThis PR successfully removes the Changes Verified:All SDKs removed:
All SDKs added:
Documentation:
API Naming Consistency:The removal follows proper language conventions:
No consistency issues found. The feature has been cleanly removed from all SDKs with equivalent behavior changes.
|
Mark the option as obsolete/deprecated in Go, .NET, and TypeScript so existing consumers continue to compile without changes. 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 demonstrates excellent cross-SDK consistency. The Summary by SDK
Consistent Changes Across All SDKs
Language-Specific AppropriatenessThe implementation differences are justified by language conventions:
No consistency issues found. This PR maintains excellent feature parity while respecting language-specific idioms. 🎉
|
The onClose callback acquires startStopMux, but Stop/ForceStop already hold that lock while waiting for readLoop to finish via wg.Wait(). Running the state update in a goroutine allows readLoop to complete, breaking the circular wait. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ SDK Consistency Review — PASSEDI've reviewed this PR for cross-SDK consistency. The removal of the What Was Checked✅ Option Deprecation (All Consistent)
All approaches are idiomatic for their respective languages. ✅ Behavior Removal (All Removed)
✅ State Management (All Implemented)All SDKs properly transition to "disconnected" state when the connection drops:
✅ Testing Coverage (Good)
.NET doesn't add a new disconnection test in this PR, but that's acceptable since the existing test suite likely covers connection state transitions, and this PR is focused on removing a feature rather than adding new behavior. SummaryNo consistency issues found. This PR maintains excellent parity across all SDK implementations. The feature removal is clean, properly documented, and the state management changes are uniformly applied. Great work! 🎉
|
The
autoRestartoption never worked correctly. This removes the partial implementation and marks the public APIs as obsolete. We'll remove the public APIs entirely in a week or so.Additionally I noticed that we were not tracking disconnected state across all the SDKs consistently. So this PR also now makes sure the state updates to "disconnected" consistently if the child process dies or the TCP connection is unexpectedly closed.