Conversation
Adds the Copilot Rust SDK (`copilot-sdk` crate) under `rust/`, alongside Rust codegen plumbed into `scripts/codegen/` and CI under `.github/workflows/rust-sdk-tests.yml`. The crate ships a JSON-RPC client, session lifecycle management, system message transforms, permission policy helpers, the `define_tool` adapter, and per-event `SessionHandler`/`SessionHooks` traits. Includes: - 14 ported E2E scenarios under `rust/tests/` driving the replay-proxy harness, plus a hand-curated set of unit tests. - A rust-coding-skill (`.github/skills/rust-coding-skill/`) capturing conventions for error handling, async/concurrency, tracing, and the intentional trait exceptions in the SDK's public API. - Release tooling: `rust-publish-release.yml`, `RELEASING.md`, and protocol-version generation wired into the existing automation. - `PermissionResult` extended with `Deferred` and `Custom` variants for richer permission decisions. Public API is held at 0.1.0-pre. Marked protocol-evolving public enums `#[non_exhaustive]` so additive variants stay non-breaking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Christopher Schleiden <cschleiden@github.com> Co-authored-by: David Dossett <25163139+daviddossett@users.noreply.github.com> Co-authored-by: Devraj Mehta <devm33@github.com> Co-authored-by: Dmytro Struk <13853051+dmytrostruk@users.noreply.github.com> Co-authored-by: Evan Boyle <EvanBoyle@users.noreply.github.com> Co-authored-by: Jeremy Moseley <jemoseley@microsoft.com> Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
- **Broadcast subscriptions for lifecycle and session events.** `Client::subscribe_lifecycle()` and `Session::subscribe()` return `tokio::sync::broadcast::Receiver`; dropping the receiver unsubscribes. Replaces the prior callback-based `Client::on`, `Client::on_event_type`, `Session::on`, and `Unsubscribe` API. Spawned consumer tasks isolate panics naturally. - **`PermissionResult` gains `Deferred` and `Custom` variants.** `Deferred` lets handlers resolve a request asynchronously via `session.permissions.handlePendingPermissionRequest` (notification path only — falls back to `Approved` on the direct RPC path). `Custom(Value)` lets handlers send arbitrary response payloads beyond the standard `approve-once` / `reject` shapes. - **`#[non_exhaustive]` on protocol-evolving public enums** (`PermissionResult`, `SessionLifecycleEventType`, `GitHubReferenceType`, others) so additive variants stay non-breaking. - **`ToolHandlerRouter` overrides per-event `SessionHandler` methods** so consumers can call `router.on_external_tool(...)` directly without unwrapping `HandlerResponse`. - **`define_tool` accepts bare `async fn` items** in addition to closures, matching `tower::service_fn` / `hyper::service::service_fn` conventions. Documented in rustdoc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ypes Generated code emitted `pub session_id: String` for every schema field named `sessionId` and likewise for `requestId`, leaving consumers with mixed types: `Session::id()` returned `SessionId` but `session.events_subscribe()` events exposed `session_id: String`. Same papercut for request IDs in permission and elicitation event payloads. The newtypes are `#[serde(transparent)]` so the wire format is unchanged. This adds a property-name override map to `scripts/codegen/rust.ts` that maps `sessionId`, `remoteSessionId`, and `requestId` to the hand-authored types in `crate::types`, and emits the matching `use` statement in both generated modules. `mc_session_id` (MCP protocol metadata, not a Copilot session) stays as `String`. After regeneration: 27 fields converted to `SessionId` (including the handoff event's `remoteSessionId`) and 25 to `RequestId`. The existing `PartialEq<str>` / `PartialEq<String>` impls on both newtypes mean test code like Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
define_tool's Fn(P) -> Fut bound gave closures only the deserialized
arguments, leaving session_id, tool_call_id, and tool_name unreachable.
That blocked the helper for any tool that needs to scope DB lookups to
a session, emit per-tool-call telemetry, or stream UI updates back to
the originating session — patterns that hit dozens of sites across
realistic tool suites.
Change the closure bound to Fn(ToolInvocation, P) -> Fut. The arguments
are moved out via mem::take before deserialization, so there is no
clone cost on the hot path. Closures that don't need the metadata
write |_inv, params|.
Also add ToolInvocation::params<P>() so long-form impl ToolHandler
blocks can deserialize without naming serde_json directly:
async fn call(&self, inv: ToolInvocation) -> Result<ToolResult, Error> {
let params: MyParams = inv.params()?;
// …use inv.session_id alongside params…
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Node, Python, and .NET all expose ping with an optional message. Go requires it only because Go has no Option type — Rust has one, so the API should match the languages with the same expressive power rather than the one without. Change ping(&self, message: &str) to ping(&self, message: Option<&str>). When None, the message field is omitted from the request payload rather than sent as an empty string. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new Rust SDK crate (copilot-sdk, library name copilot) to the monorepo, mirroring the existing SDKs’ JSON-RPC client/session model, and wires it into repo workflows and scenario coverage.
Changes:
- Introduces the Rust SDK crate with protocol types, JSON-RPC transport, session/handler abstractions, examples, and tests.
- Extends scenario samples and verification scripts to build/run Rust implementations alongside TS/Python/Go/C#.
- Updates repo automation: codegen,
justtasks, scenario-build CI, Rust SDK CI, and release/publish workflows.
Show a summary per file
| File | Description |
|---|---|
| test/scenarios/verify.sh | Shows Rust status in the aggregated scenario runner UI. |
| test/scenarios/transport/tcp/verify.sh | Builds/runs the Rust TCP transport scenario. |
| test/scenarios/transport/tcp/rust/Cargo.toml | Rust TCP scenario crate manifest. |
| test/scenarios/transport/tcp/rust/src/main.rs | Rust TCP sample using external host:port CLI server. |
| test/scenarios/transport/stdio/verify.sh | Builds/runs the Rust stdio transport scenario. |
| test/scenarios/transport/stdio/rust/Cargo.toml | Rust stdio scenario crate manifest. |
| test/scenarios/transport/stdio/rust/src/main.rs | Rust stdio sample spawning the CLI child process. |
| test/scenarios/transport/stdio/README.md | Documents Rust sample location/package name. |
| test/scenarios/tools/tool-overrides/verify.sh | Builds/runs Rust tool override scenario. |
| test/scenarios/tools/tool-overrides/rust/Cargo.toml | Rust tool-overrides scenario crate manifest (+derive). |
| test/scenarios/tools/tool-overrides/rust/src/main.rs | Rust sample overriding built-in tool behavior. |
| test/scenarios/tools/tool-filtering/verify.sh | Builds/runs Rust tool filtering scenario. |
| test/scenarios/tools/tool-filtering/rust/Cargo.toml | Rust tool-filtering scenario crate manifest. |
| test/scenarios/tools/tool-filtering/rust/src/main.rs | Rust sample limiting available tools. |
| test/scenarios/tools/skills/verify.sh | Builds/runs Rust skills scenario. |
| test/scenarios/tools/skills/rust/Cargo.toml | Rust skills scenario crate manifest. |
| test/scenarios/tools/skills/rust/src/main.rs | Rust sample configuring skill directories + hooks. |
| test/scenarios/tools/no-tools/verify.sh | Builds/runs Rust no-tools scenario. |
| test/scenarios/tools/no-tools/rust/Cargo.toml | Rust no-tools scenario crate manifest. |
| test/scenarios/tools/no-tools/rust/src/main.rs | Rust sample disabling tools + replacing system prompt. |
| test/scenarios/tools/mcp-servers/verify.sh | Builds/runs Rust MCP servers scenario. |
| test/scenarios/tools/mcp-servers/rust/Cargo.toml | Rust mcp-servers scenario crate manifest. |
| test/scenarios/tools/mcp-servers/rust/src/main.rs | Rust sample passing MCP servers config to CLI. |
| test/scenarios/tools/custom-agents/verify.sh | Builds/runs Rust custom agents scenario. |
| test/scenarios/tools/custom-agents/rust/Cargo.toml | Rust custom-agents scenario crate manifest (+derive). |
| test/scenarios/tools/custom-agents/rust/src/main.rs | Rust sample defining custom agents + custom tool. |
| test/scenarios/sessions/streaming/verify.sh | Builds/runs Rust streaming scenario. |
| test/scenarios/sessions/streaming/rust/Cargo.toml | Rust streaming scenario crate manifest. |
| test/scenarios/sessions/streaming/rust/src/main.rs | Rust sample counting streaming delta events. |
| test/scenarios/sessions/session-resume/verify.sh | Builds/runs Rust session resume scenario. |
| test/scenarios/sessions/session-resume/rust/Cargo.toml | Rust session-resume scenario crate manifest. |
| test/scenarios/sessions/session-resume/rust/src/main.rs | Rust sample creating + resuming session by ID. |
| test/scenarios/sessions/infinite-sessions/verify.sh | Builds/runs Rust infinite sessions scenario. |
| test/scenarios/sessions/infinite-sessions/rust/Cargo.toml | Rust infinite-sessions scenario crate manifest. |
| test/scenarios/sessions/infinite-sessions/rust/src/main.rs | Rust sample exercising infinite session thresholds. |
| test/scenarios/sessions/concurrent-sessions/verify.sh | Builds/runs Rust concurrent sessions scenario. |
| test/scenarios/sessions/concurrent-sessions/rust/Cargo.toml | Rust concurrent-sessions scenario crate manifest. |
| test/scenarios/sessions/concurrent-sessions/rust/src/main.rs | Rust sample running two sessions concurrently. |
| test/scenarios/prompts/system-message/verify.sh | Builds/runs Rust system-message scenario. |
| test/scenarios/prompts/system-message/rust/Cargo.toml | Rust system-message scenario crate manifest. |
| test/scenarios/prompts/system-message/rust/src/main.rs | Rust sample replacing system message. |
| test/scenarios/prompts/reasoning-effort/verify.sh | Builds/runs Rust reasoning-effort scenario. |
| test/scenarios/prompts/reasoning-effort/rust/Cargo.toml | Rust reasoning-effort scenario crate manifest. |
| test/scenarios/prompts/reasoning-effort/rust/src/main.rs | Rust sample setting reasoning_effort. |
| test/scenarios/modes/default/verify.sh | Builds/runs Rust default mode scenario. |
| test/scenarios/modes/default/rust/Cargo.toml | Rust default-mode scenario crate manifest. |
| test/scenarios/modes/default/rust/src/main.rs | Rust sample using default tool-enabled mode. |
| test/scenarios/callbacks/user-input/verify.sh | Builds/runs Rust user-input callback scenario. |
| test/scenarios/callbacks/user-input/rust/Cargo.toml | Rust user-input scenario crate manifest. |
| test/scenarios/callbacks/user-input/rust/src/main.rs | Rust sample handling ask_user prompts. |
| test/scenarios/callbacks/permissions/verify.sh | Builds/runs Rust permission callback scenario. |
| test/scenarios/callbacks/permissions/rust/Cargo.toml | Rust permissions scenario crate manifest. |
| test/scenarios/callbacks/permissions/rust/src/main.rs | Rust sample logging/approving permissions. |
| test/scenarios/callbacks/hooks/verify.sh | Builds/runs Rust hooks callback scenario. |
| test/scenarios/callbacks/hooks/rust/Cargo.toml | Rust hooks scenario crate manifest. |
| test/scenarios/callbacks/hooks/rust/src/main.rs | Rust sample implementing SessionHooks logging. |
| test/scenarios/RUST_COVERAGE.md | Documents Rust scenario parity coverage/gaps. |
| scripts/codegen/package.json | Adds Rust generation to codegen scripts. |
| nodejs/scripts/update-protocol-version.ts | Generates Rust SDK protocol version constant. |
| justfile | Adds Rust format/lint/test/codegen tasks. |
| .gitignore | Ignores Rust scenario build artifacts/lockfiles. |
| .github/workflows/scenario-builds.yml | Adds CI job building all Rust scenario crates. |
| .github/workflows/rust-sdk-tests.yml | Adds Rust SDK CI (fmt/clippy/doc/test/semver-checks). |
| .github/workflows/rust-release-pr.yml | Adds release-plz workflow to open Rust release PRs. |
| .github/workflows/rust-publish-release.yml | Adds release-plz workflow to publish Rust crate. |
| .github/workflows/codegen-check.yml | Ensures Rust codegen + protocol version regen in CI. |
| .github/skills/rust-coding-skill/SKILL.md | Adds repo-specific Rust engineering guidance. |
| .github/skills/rust-coding-skill/examples.md | Adds Rust SDK examples/patterns for contributors. |
| .github/copilot-instructions.md | Updates repo guidance to include Rust SDK + Rust skill. |
| rust/Cargo.toml | Defines the new copilot-sdk crate (features, deps, MSRV). |
| rust/Cargo.lock | Locks Rust dependencies for deterministic builds. |
| rust/README.md | Documents Rust SDK usage, architecture, and API surface. |
| rust/CHANGELOG.md | Establishes initial changelog and release-plz plan. |
| rust/RELEASING.md | Documents release/publish operations for maintainers. |
| rust/LICENSE | Rust crate license file. |
| rust/rust-toolchain.toml | Pins Rust toolchain version/components for the crate. |
| rust/release-plz.toml | Configures release-plz behavior for the Rust crate. |
| rust/clippy.toml | Configures Rust clippy rules (e.g., disallowed macros). |
| rust/.rustfmt.toml | Stable rustfmt config (edition 2024). |
| rust/.rustfmt.nightly.toml | Nightly rustfmt config enabling unstable formatting opts. |
| rust/.gitignore | Ignores Rust target dir and backup lock files. |
| rust/build.rs | Build-time CLI bundling/extraction codegen support. |
| rust/src/sdk_protocol_version.rs | Generated SDK protocol version constant for Rust. |
| rust/src/generated/mod.rs | Rust generated-type module root + re-exports. |
| rust/src/jsonrpc.rs | Content-Length framed JSON-RPC transport implementation. |
| rust/src/router.rs | Per-session routing of notifications/requests. |
| rust/src/handler.rs | Session handler traits/events + default handlers. |
| rust/src/permission.rs | Permission policy wrappers over SessionHandler. |
| rust/src/transforms.rs | System message transform extension point + dispatcher. |
| rust/src/session.rs | Session lifecycle/event loop plumbing (core runtime). |
| rust/tests/jsonrpc_test.rs | Tests for JSON-RPC framing/routing (feature-gated). |
| rust/tests/protocol_version_test.rs | Tests protocol version negotiation behavior. |
| rust/tests/integration_test.rs | Ignored integration tests against real CLI. |
| rust/examples/chat.rs | Interactive streaming chat example. |
| rust/examples/hooks.rs | Hooks example for logging/auditing. |
| rust/examples/tool_server.rs | Tool server example (feature-gated on derive). |
| rust/examples/lifecycle_observer.rs | Observer example for lifecycle/session event streams. |
Copilot's findings
- Files reviewed: 102/107 changed files
- Comments generated: 5
cargo doc was running with --features test-support, which left the derive feature off and made intra-doc links to define_tool and schema_for resolve to nothing — failing under the crate's deny(rustdoc::broken_intra_doc_links). docs.rs already uses all-features (see Cargo.toml's [package.metadata.docs.rs]); align CI with that so the docs job matches what users will see on docs.rs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
emitted from the loop correlate to a session in traces. Matches
the pattern documented in the rust-coding-skill.
- README.md / embeddedcli.rs: correct the embedded-CLI documentation
to match what build.rs and embeddedcli.rs actually do — archives
come from the github/copilot-cli GitHub Releases, integrity is
SHA-256 against SHA256SUMS.txt, and the runtime cache path is
~/.cache/copilot-sdk-{version}/copilot.
- test/scenarios/sessions/streaming/verify.sh: drop a duplicate
'# Go: build' comment.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Picks up the new model.call_failure session event (with its ModelCallFailureData payload and ModelCallFailureSource enum) and the new optional 'tip' field on session_info. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Removes path triggers and the regenerate step for other languages' protocol-version files. Those drift checks are a pre-existing gap on main and out of scope for the Rust SDK port. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 23-line setup checklist duplicated content already in rust/RELEASING.md. One-line pointer is enough. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two scenarios still used the old `Fn(P) -> Fut` shape and broke when the SDK switched to `Fn(ToolInvocation, P) -> Fut`. They don't use the invocation field, so just bind it as `_inv`. 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 #1164 · ● 2.5M
Cross-SDK consistency: every other SDK (Node, Python, Go, .NET) uses `send`/`Send`/`SendAsync` plus `MessageOptions` as the public parameter type. Rust was the outlier with `send_message` and `SendOptions`, and the asymmetry with the existing `send_and_wait` method made it read awkwardly. - Rename `Session::send_message` -> `Session::send` (and the private helper `send_message_inner` -> `send_inner`). - Rename the public `SendOptions` type -> `MessageOptions`. - Delete the previous wire-level `MessageOptions` struct: it had no internal callers (the wire payload is hand-rolled in send_inner) and freeing the name was the cleanest path to parity. Pre-1.0 type rename, no protocol or behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for getting this up. It'd be helpful to run an agent over the other SDKs and this new one to look for any inconsistencies / gaps / divergences, so that we can then evaluate each and decide whether it's acceptable or should be addressed. The more consistent we can be across the SDKs, the easier it'll be to maintain them moving forward, the more information / docs about one will translate to consumption of the others, the better we'll be able to evolve with reduced concerns for how something we want to add may not fit well in a particular SDK, etc. |
This comment has been minimized.
This comment has been minimized.
Previously Session::subscribe and Client::subscribe_lifecycle returned
raw tokio::sync::broadcast::Receiver<T> values. A survey of mature Rust
crates (tonic, lapin, rdkafka, redis-rs, tokio-tungstenite, iroh-gossip,
tokio-stream's BroadcastStream itself) found that none of them expose a
raw broadcast::Receiver in their public API; the dominant pattern is a
named newtype implementing futures::Stream, with overflow surfaced
explicitly in the item type.
Introduce a copilot::subscription module with:
- EventSubscription / LifecycleSubscription newtypes
- Inherent recv() returning Result<T, RecvError> for existing
while-let loop ergonomics
- Stream impl yielding Result<T, Lagged> so callers can use
tokio_stream::StreamExt or futures::StreamExt combinators
- Lagged / RecvError types owned by the SDK so consumers no longer
import tokio's broadcast error types
Net effect: the channel choice is now an internal implementation detail.
We can swap broadcast for async-broadcast / flume / a custom backpressure
policy, or convert lag into an Event::Lagged variant, without a breaking
change to the public surface.
Existing while-let loops in tests and examples continue to compile and
behave identically: close and lag both exit the loop, matching
tokio::sync::broadcast::Receiver.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Local cargo +nightly fmt --check passed without `--config-path .rustfmt.nightly.toml`, but CI runs with the explicit config and flagged two diffs: import group flattening and test-mod import order. Applied with the same flags CI uses. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The default `cargo test` job in rust-sdk-tests.yml runs on all three supported platforms but does not set `COPILOT_CLI_VERSION` — which means `build.rs` short-circuits immediately and the embedded-cli bundle path (download + SHA-256 verify + extract + zstd compress + embed) is never exercised in CI. After the recent switch from shelling out to `curl` over to `ureq` with retry logic in `build.rs`, that gap matters: the new HTTP client + cross-platform TLS code path needs cross-platform CI coverage before it ships. Adds a parallel `bundle` job to the same workflow: - 3-OS matrix: ubuntu-latest, macos-latest, windows-latest. - Reads the pinned CLI version from `nodejs/package.json` so the bundle test always tracks the same version the rest of the SDK ships against — no separate version source to drift. - Caches the downloaded archive in `./rust/.bundled-cli-cache` keyed on OS + CLI version, so steady-state CI doesn't refetch ~130 MB on every run. Cache miss (e.g. first run after a CLI bump) exercises the full download path end-to-end, which is the regression surface this job is meant to catch. - Runs `cargo build --features embedded-cli` to drive `build.rs` through the full pipeline. Doesn't run `cargo test` — the test job already covers runtime behavior; this job is specifically validating the bundle build. 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 #1164 · ● 2.7M
The CLI sends two messages for every user-input prompt:
1. A `user_input.requested` session-event notification, intended
for observers (UI streaming, telemetry, logging).
2. A `userInput.request` JSON-RPC method call, which is the
actual ask-and-wait prompt that drives the handler.
The Rust SDK's session event loop was wiring up `HandlerEvent::UserInput`
dispatch on BOTH paths. Result: the consumer's `on_event` ran twice for
every prompt, which surfaced as duplicate `ask_user` / `exit_plan`
widgets in github-app's session UI (#4249).
Other SDKs (Node, Python, Go, .NET) only register the JSON-RPC
handler; the notification is purely observational. Aligning with
that convention.
Changes:
1. `session.rs`: drop the `SessionEventType::UserInputRequested`
auto-dispatch arm. The CLI's follow-up `userInput.request`
JSON-RPC call still drives the handler. Replaced with a
short comment block explaining why the notification is
intentionally a no-op (and citing #4249 so the next person
tempted to add it back understands the trade).
2. `tests/session_test.rs`: replaced the
`user_input_requested_event_dispatches_to_handler_and_responds`
test (which asserted the broken behavior) with a regression
test (`user_input_requested_notification_does_not_double_dispatch`)
that:
- sends a `user_input.requested` notification and asserts no
wire response, no handler invocation;
- then sends a `userInput.request` JSON-RPC call and asserts
the handler fires exactly once.
This locks the fix in place and makes a future re-introduction
surface immediately as a test failure.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
|
||
| `HandlerEvent::PermissionRequest` and `HandlerEvent::ExternalTool` are dispatched | ||
| on spawned tasks (see `rust/src/session.rs:973` and `:1022`). Implementations | ||
| must be safe for concurrent invocation. |
There was a problem hiding this comment.
Is this expected? I assume this means we've buried some unsafe usage and we're then putting it on the developer to know when they're using shared state and the borrowed checker won't help them?
There was a problem hiding this comment.
No, this is confusing.
Not unsafe, that has a different meaning... this section is confusing... asking copilot to rewrite now.
There was a problem hiding this comment.
Good catch on the framing -- it's misleading. The compiler does enforce this; nothing buried.
SessionHandler declares Send + Sync + 'static (rust/src/handler.rs:280), and Arc<dyn SessionHandler> is cloned into tokio::spawn(async move { ... }) for the concurrent dispatch paths. The RefCell example in the doc literally won't compile -- RefCell<T> is !Sync, so the impl SessionHandler for MyHandler line fails the trait bound. The borrow checker is doing the work; the doc is just teaching "use parking_lot::Mutex here so you don't waste time reaching for RefCell first."
Updated examples.md to make this explicit -- renamed the anti-pattern to "Won't compile", showed the actual trait-bound error, and added a one-paragraph note saying the compiler is what enforces it. The rejection should be obvious from the doc now without a reader having to mentally trace spawn -> Send + Sync -> RefCell !Sync.
| ## Idioms that don't port from Go or Node | ||
|
|
||
| The most common pitfall when adapting code from the Node and Go SDKs is the |
There was a problem hiding this comment.
Curious the lack of C# mentioned
There was a problem hiding this comment.
This is probably keying off of my guidance and copilot giving up after looking at only a couple of examples. No python mentioned either.
Honestly, I'm not sure how helpful this is, but I do know that copilot's first pass at this was to write some very non-idiomatic rust. You have to prompt it to look at what the community projects do and what they avoid. This language can probably be tightened up...
There was a problem hiding this comment.
Good catch — and your hunch that the framing was lopsided is right. Let me come back with a thorough rewrite.
Doing a proper research pass across the section's four claims (event subscription, cancellation, optional fields, serde) and auditing the SDK's current code against them. The cancellation guidance in particular is too absolute — tokio_util::sync::CancellationToken is real and idiomatic for SDK-internal task coordination (it's what tonic does). Will follow up here once the doc rewrite + matching SDK refactor land.
There was a problem hiding this comment.
Done. Two commits landed addressing this and your original "lopsided framing" callout:
e96aea6-- rewrites the SKILL "Idioms that don't port" section.7202dfd-- refactorsSession.shutdownfromArc<Notify>totokio_util::sync::CancellationTokenand exposesSession::cancellation_token().
Section is now language-agnostic -- title is "Idioms that don't port from other languages", lists Node / Python / C# / Go equivalents inline where pattern divergence is notable.
Cancellation guidance rewritten. The previous framing ("drop the future or call abort() -- there is no ctx.Done() analogue") was wrong. Drop-to-cancel is the primitive, but tokio_util::sync::CancellationToken IS the Rust analog to ctx.Done() / .NET's CancellationToken, just optional and explicit per-API. The right framing is two cases:
- Caller-owned futures (
send_message,send_and_wait, subscription streams): drop the future.select!/timeout/ drop give the caller everything they need; adding a token parameter just duplicates that. This is whatreqwest/sqlx/aws-sdk-*/tonic's client side do. - SDK-internal task coordination (event loop, subprocess reader, anything
tokio::spawned): useCancellationToken. This is whattonicdoes for client-disconnect propagation.tokio-graceful-shutdownbuilds an entire framework on it.
The SDK now follows pattern 2 internally: Session.shutdown is a CancellationToken (refactored from the previous Arc<Notify>), and there's a public Session::cancellation_token() returning a child token so power users can bind their own work to the session lifetime via select!. Cancelling the child does NOT shut down the session -- isolation by design.
Other three idioms (event subscription, optional fields, serde JSON) -- guidance was correct but incomplete; doc enrichment landed in e96aea6 with the full primitive matrix, #[non_exhaustive] + builder pairing, and skip_serializing_if/default notes. No code refactor needed for those -- already idiomatic.
Citations for the cancellation framing:
- tokio-util CancellationToken docs
- tonic cancellation example
- withoutboats: Asynchronous clean-up
- Cybernetist: Rust tokio task cancellation patterns
Validation green: 237 tests (was 233; +4 from new builder + cancellation_token tests), full clippy / fmt / doc -D warnings clean.
|
|
||
| impl<'a> SessionRpcAgent<'a> { | ||
| /// Wire method: `session.agent.list`. | ||
| /// Stability: `experimental`. |
There was a problem hiding this comment.
Is this just documentation, or does this get recognized by the tooling in some way to let someone know they're using something experimental?
There was a problem hiding this comment.
I'll have my agent respond too. It's just docs, there are some options to make it stronger, but it sounds like this sort of annotation isn't done consistently in the other languages right now...
There was a problem hiding this comment.
Today, just documentation -- the schema's "stability": "experimental" flag is read by codegen and emitted as a rustdoc comment.
I just upgraded the rendering: instead of a single-line Stability: experimental note, codegen now emits a styled rustdoc warning admonition (<div class="warning">...</div>) so docs.rs renders a yellow callout box on every experimental method. Matches the visibility Python's Sphinx output already gets.
Stable Rust doesn't have a first-class #[experimental] attribute (#[unstable] is rustc-internal, std-only). If we want compile-time opt-in friction, the ecosystem patterns are:
- Cargo feature gate (e.g.
tokio_unstable-stylefeatures = ["experimental"]) -- consumers can't call experimental methods without explicitly enabling the feature. Strongest signal. unstablemodule path -- e.g.session.rpc().unstable().agent().list(). Visible in the import.
Both are bigger codegen changes that affect every experimental RPC across all SDKs' surface. Punting on those for 0.1.0; happy to revisit in a follow-up if we want the harder opt-in.
| /// Unique event identifier (UUID v4). | ||
| pub id: String, | ||
| /// ISO 8601 timestamp when the event was created. | ||
| pub timestamp: String, |
There was a problem hiding this comment.
Similarly here: is there a datetime type that could be used rather than string? The schema includes a format specifier to indicate when that'd be appropriate.
There was a problem hiding this comment.
Same as uuid, we could bring in a dependency... if we think it's worth it. std::time is intentionally minimal and free of calendar complexity, timezones, etc.
Is this important for the sdk?
There was a problem hiding this comment.
I think these timestamp ones are more worthwhile, so I'd go for this one if it's easy enough. Folks are more likely to actually use this data, compare time stamps, etc.
There was a problem hiding this comment.
We'll need chrono for that.. which is a pretty big dependency to bring in for this capability... Let me audit in a bit more detail how many timestamps we're dealing with here and keep in might usage (like comparison) and see if I can propose something a bit more lightweight.
Ideally, we push a few 3rd party deps as possible on consumers.
There was a problem hiding this comment.
https://crates.io/crates/time-format is the other options... though I kinda wonder if serde already has a dependency on one of these...
There was a problem hiding this comment.
If it's going to be a problem, we can skip. C#, Go, and Python do all special-case this, though (TypeScript doesn't).
There was a problem hiding this comment.
OK. If you feel strongly that we should make this a real type and not a string. My recommendation is time-format - zero dependencies, tiny (575 SLoC). chrono is 20K SLoC with a big list of dependencies.
There was a problem hiding this comment.
I don't feel strongly about it. Up to you whether you think it's worth it.
stephentoub and tclem agreed the Skills section is unnecessary prompt-pollution. Skills are auto-discovered by Copilot CLI without needing to be enumerated in the instructions, and the one-line description of each skill duplicates what the SKILL.md frontmatter already says. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codegen previously emitted a single bare `/// Stability: experimental.` line on each experimental wire method. stephentoub flagged this as weak signal -- there's no way for a consumer to tell from the rustdoc that they're using something that may change. Switch the rustdoc emit to a `<div class="warning">...</div>` block, which renders on docs.rs as a yellow-bordered callout (the standard rustdoc admonition convention). Matches the visibility Python's generated docs already get via Sphinx's `.. warning::` directive. This is documentation-quality only; no compile-time opt-in. Stable Rust has no first-class `#[experimental]` attribute (`#[unstable]` is rustc-internal, std-only). If we ever want compile-time friction the ecosystem patterns are a Cargo feature gate (e.g. `tokio_unstable`- style `features = ["experimental"]`) or an `unstable` module path; both are larger codegen changes affecting every experimental RPC across all SDK surfaces. Punted for 0.1.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The "anti-pattern" framing in examples.md was misleading -- it implied the RefCell example might compile but produce bugs at runtime. In reality, SessionHandler's `Send + Sync + 'static` trait bound rejects any handler whose state contains a non-Sync type (RefCell, Cell, Rc), so the example fails at the impl site, not at use. Reframe as "Won't compile", show the actual trait-bound error inline, and add a one-paragraph note explaining the compiler-level enforcement so a reader doesn't have to mentally trace `spawn -> Send + Sync -> RefCell !Sync`. Addresses stephentoub's review feedback that the prior framing read as "we've buried some unsafe usage." Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Replaces the per-session `Arc<Notify>` shutdown signal with
`tokio_util::sync::CancellationToken`, the canonical primitive for
SDK-internal task coordination in async Rust. Same cooperative-shutdown
semantics, materially better fit for the use case:
- `tonic` uses `CancellationToken` for the equivalent task coordination
case (request cancel propagated into spawned server handlers).
- Parent/child token tree gives us a clean way to expose a public
`Session::cancellation_token()` accessor that returns a child token —
consumers can `select!` on `child.cancelled()` to bind their own work
to the session lifetime, but cancelling the child does NOT shut down
the session itself (child cancel is isolated).
- `Drop` impl just calls `cancel()`, no buffering quirks (Notify's
`notify_one` was already documented as buffer-on-no-listeners; token
cancellation is sticky, which is the simpler mental model).
Plumbing:
- `tokio-util = { version = "0.7", default-features = false }` added as
a runtime dep. No new transitive deps the SDK didn't already pull
through tokio.
- `Session.shutdown` field type changes from `Arc<Notify>` to
`CancellationToken` (which is itself internally `Arc`-backed and
`Clone`).
- `stop_event_loop()` calls `cancel()` instead of `notify_one()`.
- `Drop` impl calls `cancel()` instead of `notify_one()`.
- Event-loop `select!` arm becomes `_ = shutdown.cancelled() => break`.
- New public `Session::cancellation_token()` returning a `child_token()`
— power-user API for binding external tasks to session lifetime.
Tests:
- `cancellation_token_fires_on_session_drop`: dropping the session
fires the child token within 2s.
- `cancellation_token_child_cancel_does_not_kill_session`: cancelling
a child does not propagate up to the parent.
Closes the cancellation idiom gap stephentoub flagged on the SKILL doc.
The framing "drop the future or call abort()" was too absolute — drop-
to-cancel is the right primitive for caller-owned futures, but for
SDK-internal task coordination CancellationToken is the canonical
answer (citations: tokio-util docs, tonic cancellation example,
withoutboats "Asynchronous clean-up", `tokio-graceful-shutdown` crate).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
stephentoub flagged that the "Idioms that don't port from Go or Node" section was lopsided (no C# / Python mention) and that the cancellation guidance was too absolute (it dismissed `ctx.Done()` analogs without mentioning tokio_util's CancellationToken). Both true. Replaces the section with a thorough rewrite covering all four idioms with citations and decision matrices: 1. Event subscription: keep "channels not callbacks" baseline; add the full primitive matrix (mpsc / oneshot / broadcast / watch); explain that the public API should expose `impl Stream<Item = T>` (the canonical IObservable<T> analog), citing tonic / reqwest / sqlx as precedent. 2. Cancellation: rewrite. "Drop = cancel" is the primitive for caller-owned futures. `tokio_util::sync::CancellationToken` is the canonical answer for SDK-internal task coordination — citations to tonic's cancellation example, tokio-util docs, withoutboats blog post, and Cybernetist's task cancellation patterns survey. Notes the SDK's own `Session.shutdown: CancellationToken` field and the `Session::cancellation_token()` accessor. 3. Optional fields: confirm Option<T> + Default baseline; add non_exhaustive + builder pairing (AWS SDK convention); note when typestate / Result-from-build is the right answer. 4. serde JSON: confirm rename_all + per-field rename baseline; add skip_serializing_if = "Option::is_none" for output omission and #[serde(default)] for input tolerance (LSP/JSON-RPC convention). Section title is also de-language-coupled: "Idioms that don't port from other languages" instead of "...from Go or Node". Lists Node / Python / C# / Go equivalents inline where pattern divergence is notable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
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 #1164 · ● 3.4M
| /// System-message transform. See [`SessionConfig::transform`]. | ||
| #[serde(skip)] | ||
| pub transform: Option<Arc<dyn SystemMessageTransform>>, | ||
| } |
There was a problem hiding this comment.
Cross-SDK consistency gap: continue_pending_work missing from ResumeSessionConfig
All other SDKs expose a continuePendingWork flag on their resume config that instructs the CLI to preserve in-flight tool calls and permission requests across reconnections rather than treating them as interrupted. This field is absent from the Rust ResumeSessionConfig.
Other SDK equivalents:
- Node.js:
ResumeSessionConfig.continuePendingWork?: boolean(nodejs/src/types.ts:1435) - Go:
ResumeSessionConfig.ContinuePendingWork bool(go/types.go:815) - Python:
continue_pending_work: bool | NoneinResumeSessionConfig(python/copilot/client.py:1516) - .NET:
bool? ContinuePendingWorkonResumeSessionConfig(dotnet/src/Types.cs:2189)
The generated SessionResumeData event does have a continue_pending_work field (to reflect the server's decision at runtime), but the request side — the config the caller sends to opt in — is missing here.
Suggested addition (right before disable_resume):
/// When `true`, the runtime continues any tool calls or permission requests
/// that were still pending when the session was last disconnected.
/// When `false` (the default), pending work is treated as interrupted on resume.
///
/// For permission requests, the runtime re-emits `permission.requested`; for
/// external tool calls, supply the result via the corresponding low-level RPC.
#[serde(skip_serializing_if = "Option::is_none")]
pub continue_pending_work: Option<bool>,A matching with_continue_pending_work(bool) builder method near with_disable_resume would complete the parity. This gap is not called out in the README's "Differences From Other SDKs" section, so it appears unintentional.
The wire-protocol schema added top-level `agentId?: string` to every session event envelope in commit f8cf846 ("Derive session event envelopes from schema") for sub-agent attribution. Every other SDK carries it; Rust silently drops it at the deserialization boundary. Concretely: the schema describes `agentId` as "Sub-agent instance identifier. Absent for events from the root/main agent and session- level events." Without the field, Rust consumers can't distinguish events emitted by a sub-agent from events emitted by the root agent. Adds: - `pub agent_id: Option<String>` on `types::SessionEvent` (hand- authored consumer-facing). - `pub agent_id: Option<String>` on `generated::session_events::TypedSessionEvent` via codegen update in `scripts/codegen/rust.ts`. - `#[serde(rename = "agentId", skip_serializing_if = "Option::is_none")]` via the existing container `rename_all = "camelCase"` (covered) plus skip-on-None for clean wire output. Round-trip tests cover both struct shapes — sub-agent event with the field set, root-agent event without — to lock the parity in place against future schema changes or codegen regressions. Caught by a fresh parity audit against Node/Python/Go/.NET as part of the post-main-merge gap review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Accidentally committed by the parity audit run; not intended as repo artifacts. 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 #1164 · ● 2.2M
…n pattern
Two changes prompted by review:
1. SKILL.md was too prose-heavy for an agent to scan. The "Idioms that
don't port from other languages" section had grown to ~110 lines of
essay-shaped advice with key rules buried in narrative. Replaced
with three tight rule sections (Concurrency primitives, Optional
fields and serde, plus a one-paragraph cross-language porting note
that points back to the rule sections). Net: 345 -> 255 lines, no
rule lost.
2. The trait-vs-callback-fields rule was implicit in the SessionHandler
description but not explicit. Codified in "Traits and conversions"
as a primary directive: prefer one trait with one default-impl
method per event over per-event Box<dyn Fn> fields. Cited the three
precedent traits in async Rust:
- tower_lsp::LanguageServer (63 methods, default impls; LSP wire
protocol)
- rmcp::ServerHandler (26 methods, all default; MCP wire protocol)
- notify::EventHandler (single on_event(enum) for uniform-shape
events)
Confirmed via research that no major async Rust crate ships
per-event Box<dyn Fn> callback fields as its primary API; the
pattern fights Send + Sync + 'static, fragments consumer state
across closures, and skips exhaustiveness. The SDK's SessionHandler
already uses the recommended shape (per-method with defaults plus a
default on_event dispatcher); the doc just hadn't named the pattern.
Section ordering tightened: Traits/conversions now leads into
Extension points, then Concurrency primitives (channels matrix +
cancellation), then Optional fields/serde. Reads top-to-bottom for
an agent picking up a fresh task.
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 #1164 · ● 1.9M
| /// Force-fail resume if the session does not exist on disk, instead of | ||
| /// silently starting a new session. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub disable_resume: Option<bool>, |
There was a problem hiding this comment.
Cross-SDK consistency gap: continue_pending_work missing from ResumeSessionConfig
All other SDKs expose a continuePendingWork / continue_pending_work / ContinuePendingWork field on their resume config, but the Rust ResumeSessionConfig doesn't include it:
| SDK | Field |
|---|---|
| Node.js | continuePendingWork?: boolean |
| Python | continue_pending_work: bool |
| Go | ContinuePendingWork bool |
| .NET | bool? ContinuePendingWork |
| Rust | ❌ missing |
This field is important for the "pending work resume" pattern — when a session is abandoned mid-tool-execution (e.g. via force_stop), setting continue_pending_work: true on resume instructs the CLI to replay and complete those pending tool/permission requests.
Suggested addition (after disable_resume):
/// When `true`, instructs the runtime to continue any pending tool calls
/// or permission requests that were outstanding when the session was
/// last disconnected. Use in combination with [`Client::force_stop`] to
/// hand off a live session to a new client without losing in-flight work.
/// See [`SessionConfig`] for the equivalent field on initial session creation.
#[serde(skip_serializing_if = "Option::is_none")]
pub continue_pending_work: Option<bool>,And a corresponding builder method in impl ResumeSessionConfig:
pub fn with_continue_pending_work(mut self, value: bool) -> Self {
self.continue_pending_work = Some(value);
self
}Don't forget to include it in ResumeSessionConfig::new (as None) and in the Debug impl.
Cross-SDK parity gap caught by the SDK consistency reviewer. All four other SDKs (Node, Python, Go, .NET) expose this field; Rust omitted it. The field opts the runtime into continuing any tool calls or permission requests that were pending when the previous connection was dropped — it's the key enabler of the pending-work-handoff pattern used together with `Client::force_stop` to migrate a session from one process to another without losing in-flight work. Plumbing: - `pub continue_pending_work: Option<bool>` on `ResumeSessionConfig` with `#[serde(skip_serializing_if = "Option::is_none")]`. Container `rename_all = "camelCase"` covers the wire-name `continuePendingWork`. No manual payload-construction code is needed; resume_session serializes config -> wire via `serde_json::to_value(&config)`. - `with_continue_pending_work(bool)` builder. - Default impl: `None`. - Debug impl: includes the field. Tests: - `resume_session_config_builder_composes` extended to cover the new field. - `resume_session_config_serializes_continue_pending_work_to_camel_case` asserts the wire shape (`continuePendingWork: true`) and that unset values are omitted (skip_serializing_if). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review: Rust SDK Addition ✅Reviewed PR #1164 (Rust SDK technical preview) for cross-SDK consistency against the existing Node.js/TypeScript, Python, Go, and .NET implementations. Overall AssessmentThe Rust SDK is well-aligned with the other SDKs at the protocol and architecture level. The JSON-RPC wire protocol, session lifecycle, session config/resume patterns, permissions, hooks, tools, elicitation, and streaming behavior are all consistent. The Identified Parity Gaps (all documented in
|
| Rust method | Other SDKs (via generated RPC only) |
|---|---|
session.get_model() |
Node: rpc.model.getCurrent() / Go: Rpc.Model.GetCurrent() / .NET: Rpc.Model.GetCurrentAsync() |
session.set_mode() / get_mode() |
Node: rpc.mode.set() / Go: Rpc.Mode.Set() |
session.get_name() / set_name() |
Node: rpc.name.get/set() / Go: Rpc.Name.Get/Set() |
session.read_plan() / update_plan() / delete_plan() |
Node: rpc.plan.read/update/delete() / Go: Rpc.Plan.Read/Update/Delete() |
session.list_workspace_files() / read_workspace_file() / create_workspace_file() |
Node: rpc.workspaces.listFiles/readFile/createFile() |
session.start_fleet() |
Node: rpc.fleet.start() / Go: Rpc.Fleet.Start() |
Client::get_quota() |
Node: rpc.account.getQuota() / Go: Rpc.Account.GetQuota() |
Suggestion (post-release): Adding these as convenience wrappers on Session/Client in Go, Python, Node.js, and .NET would improve ergonomics uniformly. Not a blocker.
2. Client::send_telemetry() and Session::send_telemetry() — Not in any other SDK
These call sendTelemetry / session.sendTelemetry RPCs that are not exposed even in the generated RPC namespace of the other SDKs. This is the only true protocol-level gap (vs a convenience wrapper gap).
Suggestion (post-release): Add sendTelemetry / session.sendTelemetry to the other SDKs' generated RPC layer and expose as first-class methods for parity.
3. SessionHandler::on_auto_mode_switch — Not in other SDKs
Rust has a typed handler for the autoModeSwitch.request callback. Other SDKs observe it as a raw event and must drive the wire response manually.
Suggestion (post-release): Port on_auto_mode_switch handling to Go, Python, Node.js, and .NET.
Rust-Only by Design (no parity needed)
These are idiomatic Rust features that don't translate to other languages:
SessionId/RequestIdnewtypes (strong typing vs bare strings in other SDKs)enum Transport { Stdio, Tcp, External }(other SDKs use conditional config)permission::approve_all/deny_all/approve_ifbuilder helpersClient::from_streams()(arbitraryAsyncRead/AsyncWritefor testing)CancellationToken/ broadcast channel subscription model (subscribe()vs callback-basedon())Session::stop_event_loop()/Session::cancellation_token()(Rust async lifecycle concern)Arc<dyn SessionFsProvider>direct registration (vs factory-closure in other SDKs)
Summary
The Rust SDK follows the same architecture and protocol as the other SDKs. All identified parity gaps are intentional and documented in rust/README.md. No consistency blockers found. The "Rust-only API" features (especially the convenience wrappers and telemetry methods) are good candidates for backporting to other SDKs as a post-release effort.
Generated by SDK Consistency Review Agent for issue #1164 · ● 1.9M · ◷
Adds a Rust SDK alongside the existing Node, Python, Go, and .NET SDKs in this repo. Same JSON-RPC client model, same protocol, same session lifecycle — just in Rust.
Important
Technical preview. This is published as
github-copilot-sdk = "0.1"(pre-1.0) and the public API is subject to breaking changes as we iterate. Pin to an exact version, expect churn, and please file issues for friction or missing parity.See
rust/README.mdfor the full overview, examples, and the build/test commands. Generated types follow the same schema-driven flow used by the other SDKs (scripts/codegen/rust.ts).CI for the new crate runs in
.github/workflows/rust-sdk-tests.yml.