From 58c5607831b37e6bf9951a9bdf390425eebf10d5 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 22 May 2026 18:20:46 +0100 Subject: [PATCH 1/3] Address PR #1367 review follow-ups (#1380) Three fix-before-done items: * Repair broken intra-doc links in tool.rs ToolHandler doc and related sites so cargo doc --no-deps works on default features (without --all-features). Tool and define_tool were both feature-gated under derive, causing rustdoc::broken_intra_doc_links to fail. Switched to fully-qualified paths (Tool) and plain backticks (define_tool, schema_for). * Convert SessionConfig::to_wire and ResumeSessionConfig::to_wire into consuming into_wire(self) -> (Wire, SessionConfigRuntime). The new SessionConfigRuntime bundle holds the trait-object handlers, tool handlers map, session-fs provider, and slash commands drained out of the config. Eliminates the deep Vec / HashMap clone the old &self-based shape required, and makes the .take()-after-to_wire ordering hazard a compile-time impossibility. The duplicate-tool-handler check moves into into_wire as part of the drain. * Drop Serialize and Deserialize derives from SessionConfig and ResumeSessionConfig. Since every handler field had #[serde(skip)], serde round-tripping silently dropped all runtime behavior -- a footgun. The wire payload is now produced exclusively via into_wire on the dedicated wire::Session{Create,Resume}Wire structs. Also dropped the now-redundant env_value_mode field (the wire struct hard-codes "direct"). Two bucket_b coverage tests moved into the in-crate unit-test module since the wire types are pub(crate). Nice-to-haves: * Session struct rustdoc: get_messages -> get_events (the canonical Phase A name). * Simplify the Client::start connection-token resolution block (drop the shadowing inner block + redundant retyped let). * Tighten Tool::with_parameters to panic on non-object input via the existing tool_parameters helper, matching the loud-on-misuse semantics of the rest of the tool module instead of silently producing an empty parameter map. * Fix README typo: SystemMessageTransform replaces the duplicate ToolHandler in the handler-trait list. * Trim the unreachable tool_name.is_empty() branch in the ExternalToolRequested dispatch path (the outer guard already short-circuits when the tools map lookup fails). * Move Tool::handler from pub to pub(crate) and add a Tool::handler() accessor so external callers can no longer overwrite an already-attached handler by direct field assignment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- rust/README.md | 4 +- rust/src/lib.rs | 32 +-- rust/src/session.rs | 106 +++---- rust/src/tool.rs | 14 +- rust/src/types.rs | 569 ++++++++++++++++++++++--------------- rust/tests/session_test.rs | 63 ++-- 6 files changed, 420 insertions(+), 368 deletions(-) diff --git a/rust/README.md b/rust/README.md index b30dc1049..4dc808470 100644 --- a/rust/README.md +++ b/rust/README.md @@ -375,7 +375,7 @@ Tools are named types (not closures) — visible in stack traces and navigable v Tools without an attached handler (`Tool::with_handler` never called) are declaration-only: the SDK advertises them on the wire but doesn't dispatch invocations to anything. Useful when another connected client services the tool. -For trivial tools that don't need a named type, [`define_tool`](crate::tool::define_tool) collapses the definition to a single expression and returns a fully-formed `Tool` with handler attached: +For trivial tools that don't need a named type, the `define_tool` macro (available with the `derive` feature) collapses the definition to a single expression and returns a fully-formed `Tool` with handler attached: ```rust,ignore use github_copilot_sdk::tool::{define_tool, JsonSchema}; @@ -672,7 +672,7 @@ ergonomics the dynamically-typed SDKs don't. `Session` value to thread in, and the SDK already prefers traits over boxed closures for handler-shaped APIs (`PermissionHandler`, `ToolHandler`, `SessionHooks`, - `ToolHandler`). + `SystemMessageTransform`). ```rust,ignore use std::sync::Arc; diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 687c8c241..708f7864f 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -937,26 +937,20 @@ impl Client { // to the server. For Tcp, the SDK auto-generates one when the // caller leaves it unset so the loopback listener is safe by // default. - let (mut options, effective_connection_token) = { - let mut options = options; - let effective = match &mut options.transport { - Transport::Stdio => None, - Transport::Tcp { - connection_token, .. - } => { - if connection_token.is_none() { - *connection_token = Some(generate_connection_token()); - } - connection_token.clone() - } - Transport::External { - connection_token, .. - } => connection_token.clone(), - }; - (options, effective) + let mut options = options; + let effective_connection_token: Option = match &mut options.transport { + Transport::Stdio => None, + Transport::Tcp { + connection_token, .. + } => Some( + connection_token + .get_or_insert_with(generate_connection_token) + .clone(), + ), + Transport::External { + connection_token, .. + } => connection_token.clone(), }; - let _ = &mut options; - let effective_connection_token: Option = effective_connection_token; let session_fs_config = options.session_fs.clone(); let session_fs_sqlite_declared = session_fs_config .as_ref() diff --git a/rust/src/session.rs b/rust/src/session.rs index b07133eb7..f8a35ce0c 100644 --- a/rust/src/session.rs +++ b/rust/src/session.rs @@ -122,7 +122,7 @@ impl Drop for PendingSessionRegistration { /// Owns an internal event loop that dispatches events to the per-callback /// handlers installed on the session config. /// -/// Protocol methods (`send`, `get_messages`, `abort`, etc.) automatically +/// Protocol methods (`send`, `get_events`, `abort`, etc.) automatically /// inject the session ID into RPC params. /// /// Call [`destroy`](Self::destroy) for graceful cleanup (RPC + local). If dropped @@ -788,45 +788,27 @@ impl Client { if let Some(transforms) = config.system_message_transform.clone() { inject_transform_sections(&mut config, transforms.as_ref()); } - let wire = config.to_wire(session_id.clone()); + let (wire, mut runtime) = config.into_wire(session_id.clone())?; let permission_handler = crate::permission::resolve_handler( - config.permission_handler.take(), - config.permission_policy.take(), + runtime.permission_handler.take(), + runtime.permission_policy.take(), ); - let elicitation_handler = config.elicitation_handler.take(); - let user_input_handler = config.user_input_handler.take(); - let exit_plan_mode_handler = config.exit_plan_mode_handler.take(); - let auto_mode_switch_handler = config.auto_mode_switch_handler.take(); - let mut tool_map: HashMap> = HashMap::new(); - if let Some(tools) = config.tools.as_mut() { - for tool in tools.iter_mut() { - if let Some(handler) = tool.handler.take() { - if tool_map.contains_key(&tool.name) { - return Err(Error::InvalidConfig(format!( - "duplicate tool handler registered for name {:?}", - tool.name - ))); - } - tool_map.insert(tool.name.clone(), handler); - } - } - } let handlers = SessionHandlers { permission: permission_handler, - elicitation: elicitation_handler, - user_input: user_input_handler, - exit_plan_mode: exit_plan_mode_handler, - auto_mode_switch: auto_mode_switch_handler, - tools: Arc::new(tool_map), + elicitation: runtime.elicitation_handler.take(), + user_input: runtime.user_input_handler.take(), + exit_plan_mode: runtime.exit_plan_mode_handler.take(), + auto_mode_switch: runtime.auto_mode_switch_handler.take(), + tools: Arc::new(std::mem::take(&mut runtime.tool_handlers)), }; - let hooks = config.hooks_handler.take(); - let transforms = config.system_message_transform.take(); - let tools_count = config.tools.as_ref().map_or(0, Vec::len); - let commands_count = config.commands.as_ref().map_or(0, Vec::len); + let hooks = runtime.hooks_handler.take(); + let transforms = runtime.system_message_transform.take(); + let tools_count = wire.tools.as_ref().map_or(0, Vec::len); + let commands_count = runtime.commands.as_ref().map_or(0, Vec::len); let has_hooks = hooks.is_some(); - let command_handlers = build_command_handler_map(config.commands.as_deref()); - let session_fs_provider = config.session_fs_provider.take(); + let command_handlers = build_command_handler_map(runtime.commands.as_deref()); + let session_fs_provider = runtime.session_fs_provider.take(); if self.inner.session_fs_configured && session_fs_provider.is_none() { return Err(Error::Session(SessionError::SessionFsProviderRequired)); } @@ -943,45 +925,27 @@ impl Client { if let Some(transforms) = config.system_message_transform.clone() { inject_transform_sections_resume(&mut config, transforms.as_ref()); } - let wire = config.to_wire(); + let (wire, mut runtime) = config.into_wire()?; let permission_handler = crate::permission::resolve_handler( - config.permission_handler.take(), - config.permission_policy.take(), + runtime.permission_handler.take(), + runtime.permission_policy.take(), ); - let elicitation_handler = config.elicitation_handler.take(); - let user_input_handler = config.user_input_handler.take(); - let exit_plan_mode_handler = config.exit_plan_mode_handler.take(); - let auto_mode_switch_handler = config.auto_mode_switch_handler.take(); - let mut tool_map: HashMap> = HashMap::new(); - if let Some(tools) = config.tools.as_mut() { - for tool in tools.iter_mut() { - if let Some(handler) = tool.handler.take() { - if tool_map.contains_key(&tool.name) { - return Err(Error::InvalidConfig(format!( - "duplicate tool handler registered for name {:?}", - tool.name - ))); - } - tool_map.insert(tool.name.clone(), handler); - } - } - } let handlers = SessionHandlers { permission: permission_handler, - elicitation: elicitation_handler, - user_input: user_input_handler, - exit_plan_mode: exit_plan_mode_handler, - auto_mode_switch: auto_mode_switch_handler, - tools: Arc::new(tool_map), + elicitation: runtime.elicitation_handler.take(), + user_input: runtime.user_input_handler.take(), + exit_plan_mode: runtime.exit_plan_mode_handler.take(), + auto_mode_switch: runtime.auto_mode_switch_handler.take(), + tools: Arc::new(std::mem::take(&mut runtime.tool_handlers)), }; - let hooks = config.hooks_handler.take(); - let transforms = config.system_message_transform.take(); - let tools_count = config.tools.as_ref().map_or(0, Vec::len); - let commands_count = config.commands.as_ref().map_or(0, Vec::len); + let hooks = runtime.hooks_handler.take(); + let transforms = runtime.system_message_transform.take(); + let tools_count = wire.tools.as_ref().map_or(0, Vec::len); + let commands_count = runtime.commands.as_ref().map_or(0, Vec::len); let has_hooks = hooks.is_some(); - let command_handlers = build_command_handler_map(config.commands.as_deref()); - let session_fs_provider = config.session_fs_provider.take(); + let command_handlers = build_command_handler_map(runtime.commands.as_deref()); + let session_fs_provider = runtime.session_fs_provider.take(); if self.inner.session_fs_configured && session_fs_provider.is_none() { return Err(Error::Session(SessionError::SessionFsProviderRequired)); } @@ -1464,12 +1428,12 @@ async fn handle_notification( ); tokio::spawn( async move { - if data.tool_call_id.is_empty() || data.tool_name.is_empty() { - let error_msg = if data.tool_call_id.is_empty() { - "Missing toolCallId" - } else { - "Missing toolName" - }; + // `tool_name.is_empty()` would have produced a `None` + // lookup in `handlers.tools` and short-circuited at the + // outer guard above, so only the tool_call_id check is + // reachable here. + if data.tool_call_id.is_empty() { + let error_msg = "Missing toolCallId"; let rpc_start = Instant::now(); let _ = client .call( diff --git a/rust/src/tool.rs b/rust/src/tool.rs index 95ac16d68..326b06563 100644 --- a/rust/src/tool.rs +++ b/rust/src/tool.rs @@ -57,11 +57,13 @@ pub fn schema_for() -> serde_json::Value { } /// Convert a JSON Schema [`Value`](serde_json::Value) into the -/// [`Tool::parameters`] map shape expected by the protocol. +/// [`Tool::parameters`](crate::types::Tool::parameters) map shape +/// expected by the protocol. /// /// Panics if the input is not a JSON object — tool parameter schemas /// are always top-level objects (`{"type": "object", ...}`). Pair with -/// [`schema_for`] or a `serde_json::json!(...)` literal. +/// `schema_for` (available with the `derive` feature) or a +/// `serde_json::json!(...)` literal. /// /// Use [`try_tool_parameters`] when the schema comes from dynamic input and /// should return a recoverable error instead of panicking. @@ -179,13 +181,15 @@ pub fn convert_mcp_call_tool_result(value: &serde_json::Value) -> Option>, + pub(crate) handler: Option>, } #[inline] @@ -395,16 +400,19 @@ impl Tool { /// Set the JSON Schema for the tool's input parameters. /// - /// Accepts anything that converts into a JSON object, including a - /// `serde_json::Value` produced by `json!({...})`. Non-object values - /// are stored as an empty parameter map; callers that need direct - /// control over the field can construct a `HashMap` - /// and assign it to [`Tool::parameters`] via [`Default::default`]. + /// Accepts a JSON Schema as a `serde_json::Value`, typically built with + /// `serde_json::json!({...})` or returned by `schema_for` (available + /// with the `derive` feature). Tool parameter schemas are always + /// top-level JSON objects (`{"type": "object", ...}`). + /// + /// # Panics + /// + /// Panics if `parameters` is not a JSON object. Use + /// [`crate::tool::try_tool_parameters`] and assign to + /// [`Tool::parameters`] directly when the schema comes from dynamic + /// input and should produce a recoverable error instead. pub fn with_parameters(mut self, parameters: Value) -> Self { - self.parameters = parameters - .as_object() - .map(|m| m.iter().map(|(k, v)| (k.clone(), v.clone())).collect()) - .unwrap_or_default(); + self.parameters = crate::tool::tool_parameters(parameters); self } @@ -431,6 +439,14 @@ impl Tool { self.handler = Some(handler); self } + + /// Returns the attached runtime handler, if any. + /// + /// Read-only inspection — to install or replace a handler, use + /// [`Tool::with_handler`]. + pub fn handler(&self) -> Option<&Arc> { + self.handler.as_ref() + } } impl std::fmt::Debug for Tool { @@ -1004,14 +1020,6 @@ pub struct AzureProviderOptions { pub api_version: Option, } -/// Wire default for [`SessionConfig::env_value_mode`] / -/// [`ResumeSessionConfig::env_value_mode`]. The runtime understands -/// `"direct"` (literal values) or `"indirect"` (env-var lookup); the SDK -/// only ever sends `"direct"`. -fn default_env_value_mode() -> String { - "direct".into() -} - /// Configuration for creating a new session via the `session.create` RPC. /// /// All fields are optional — the CLI applies sensible defaults. @@ -1054,88 +1062,67 @@ fn default_env_value_mode() -> String { /// # Field naming across SDKs /// /// Rust field names are snake_case (`available_tools`, `system_message`); -/// they round-trip to the camelCase wire protocol via `#[serde(rename_all = -/// "camelCase")]`. When porting code from the TypeScript, Go, Python, or -/// .NET SDKs — or reading the raw JSON-RPC traces — fields appear as -/// `availableTools`, `systemMessage`, etc. -#[derive(Clone, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] +/// the wire protocol uses camelCase (`availableTools`, `systemMessage`). +/// The mapping happens inside `SessionConfig::into_wire` (crate-private), +/// which builds a separate `SessionCreateWire` payload. This config +/// struct is no longer itself serializable — the trait-object handler +/// fields (e.g. [`permission_handler`](Self::permission_handler)) could +/// never round-trip through serde, so the only legitimate serialization +/// path is now `into_wire`. When porting code from the TypeScript, Go, +/// Python, or .NET SDKs — or reading the raw JSON-RPC traces — fields +/// appear as `availableTools`, `systemMessage`, etc. +#[derive(Clone)] #[non_exhaustive] pub struct SessionConfig { /// Custom session ID. When unset, the CLI generates one. - #[serde(skip_serializing_if = "Option::is_none")] pub session_id: Option, /// Model to use (e.g. `"gpt-4"`, `"claude-sonnet-4"`). - #[serde(skip_serializing_if = "Option::is_none")] pub model: Option, /// Application name sent as `User-Agent` context. - #[serde(skip_serializing_if = "Option::is_none")] pub client_name: Option, /// Reasoning effort level (e.g. `"low"`, `"medium"`, `"high"`). - #[serde(skip_serializing_if = "Option::is_none")] pub reasoning_effort: Option, /// Enable streaming token deltas via `assistant.message_delta` events. - #[serde(skip_serializing_if = "Option::is_none")] pub streaming: Option, /// Custom system message configuration. - #[serde(skip_serializing_if = "Option::is_none")] pub system_message: Option, /// Client-defined tool declarations to expose to the agent. - #[serde(skip_serializing_if = "Option::is_none")] pub tools: Option>, /// Allowlist of built-in tool names the agent may use. - #[serde(skip_serializing_if = "Option::is_none")] pub available_tools: Option>, /// Blocklist of built-in tool names the agent must not use. - #[serde(skip_serializing_if = "Option::is_none")] pub excluded_tools: Option>, /// MCP server configurations passed through to the CLI. - #[serde(skip_serializing_if = "Option::is_none")] pub mcp_servers: Option>, - /// Wire-format hint for MCP `env` map values. The runtime understands - /// `"direct"` (literal values) and `"indirect"` (env-var lookup); the - /// SDK only ever sends `"direct"` and consumers don't have a knob. - #[serde(default = "default_env_value_mode", skip_deserializing)] - pub(crate) env_value_mode: String, /// When true, the CLI runs config discovery (MCP config files, skills, plugins). - #[serde(skip_serializing_if = "Option::is_none")] pub enable_config_discovery: Option, /// Skill directory paths passed through to the GitHub Copilot CLI. - #[serde(skip_serializing_if = "Option::is_none")] pub skill_directories: Option>, /// Additional directories to search for custom instruction files. /// Forwarded to the CLI; not the same as [`skill_directories`](Self::skill_directories). - #[serde(skip_serializing_if = "Option::is_none")] pub instruction_directories: Option>, /// Skill names to disable. Skills in this set will not be available /// even if found in skill directories. - #[serde(skip_serializing_if = "Option::is_none")] pub disabled_skills: Option>, /// Enable session hooks. When `true`, the CLI sends `hooks.invoke` /// RPC requests at key lifecycle points (pre/post tool use, prompt /// submission, session start/end, errors). - #[serde(skip_serializing_if = "Option::is_none")] pub hooks: Option, /// Custom agents (sub-agents) configured for this session. - #[serde(skip_serializing_if = "Option::is_none")] pub custom_agents: Option>, /// Configures the built-in default agent. Use `excluded_tools` to /// hide tools from the default agent while keeping them available /// to custom sub-agents that reference them in their `tools` list. - #[serde(skip_serializing_if = "Option::is_none")] pub default_agent: Option, /// Name of the custom agent to activate when the session starts. /// Must match the `name` of one of the agents in [`Self::custom_agents`]. - #[serde(skip_serializing_if = "Option::is_none")] pub agent: Option, /// Configures infinite sessions: persistent workspace + automatic /// context-window compaction. Enabled by default on the CLI. - #[serde(skip_serializing_if = "Option::is_none")] pub infinite_sessions: Option, /// Custom model provider (BYOK). When set, the session routes /// requests through this provider instead of the default Copilot /// routing. - #[serde(skip_serializing_if = "Option::is_none")] pub provider: Option, /// Enables or disables internal session telemetry for this session. /// @@ -1144,91 +1131,73 @@ pub struct SessionConfig { /// When a custom [`provider`](Self::provider) is configured, session /// telemetry is always disabled regardless of this setting. This is /// independent of [`ClientOptions::telemetry`](crate::ClientOptions::telemetry). - #[serde(skip_serializing_if = "Option::is_none")] pub enable_session_telemetry: Option, /// Per-property overrides for model capabilities, deep-merged over /// runtime defaults. - #[serde(skip_serializing_if = "Option::is_none")] pub model_capabilities: Option, /// Override the default configuration directory location. When set, /// the session uses this directory for storing config and state. - #[serde(skip_serializing_if = "Option::is_none")] pub config_dir: Option, /// Working directory for the session. Tool operations resolve /// relative paths against this directory. - #[serde(skip_serializing_if = "Option::is_none")] pub working_directory: Option, /// Per-session GitHub token. Distinct from /// [`ClientOptions::github_token`](crate::ClientOptions::github_token), /// which authenticates the CLI process itself; this token determines /// the GitHub identity used for content exclusion, model routing, and /// quota checks for *this session*. - #[serde(rename = "gitHubToken", skip_serializing_if = "Option::is_none")] pub github_token: Option, /// Per-session remote behavior control: /// - `Off` — local only, no remote export (default) /// - `Export` — export session events to GitHub without /// enabling remote steering /// - `On` — export to GitHub AND enable remote steering - #[serde(skip_serializing_if = "Option::is_none")] pub remote_session: Option, /// Creates a remote session in the cloud instead of a local session. /// The optional repository is associated with the cloud session. - #[serde(skip_serializing_if = "Option::is_none")] pub cloud: Option, /// Forward sub-agent streaming events to this connection. When false, /// only non-streaming sub-agent events and `subagent.*` lifecycle events /// are delivered. Defaults to true on the CLI. - #[serde(skip_serializing_if = "Option::is_none")] pub include_sub_agent_streaming_events: Option, /// Slash commands registered for this session. When the CLI has a TUI, /// each command appears as `/name` for the user to invoke and the /// associated [`CommandHandler`] is called when executed. - #[serde(skip_serializing_if = "Option::is_none", skip_deserializing)] pub commands: Option>, /// Custom session filesystem provider for this session. Required when /// the [`Client`](crate::Client) was started with /// [`ClientOptions::session_fs`](crate::ClientOptions::session_fs) set. /// See [`SessionFsProvider`]. - #[serde(skip)] pub session_fs_provider: Option>, /// Optional permission-request handler. When `None`, the SDK sends /// `requestPermission: false` on the wire so the runtime does not /// emit `permission.requested` broadcasts to this client. - #[serde(skip)] pub permission_handler: Option>, /// Optional elicitation-request handler. When `None`, /// `requestElicitation: false` goes on the wire. - #[serde(skip)] pub elicitation_handler: Option>, /// Optional user-input handler. When `None`, /// `requestUserInput: false` goes on the wire and the `ask_user` /// tool is disabled. - #[serde(skip)] pub user_input_handler: Option>, /// Optional exit-plan-mode handler. When `None`, /// `requestExitPlanMode: false` goes on the wire. - #[serde(skip)] pub exit_plan_mode_handler: Option>, /// Optional auto-mode-switch handler. When `None`, /// `requestAutoModeSwitch: false` goes on the wire. - #[serde(skip)] pub auto_mode_switch_handler: Option>, /// Session lifecycle hook handler (pre/post tool use, session /// start/end, etc.). When set, the SDK auto-enables the wire-level /// `hooks` flag. Use [`with_hooks`](Self::with_hooks) to install one. - #[serde(skip)] pub hooks_handler: Option>, /// Permission policy applied to the handler. Stored separately from /// `permission_handler` so the order of `with_permission_handler` and /// `approve_all_permissions` (and friends) is irrelevant. - #[serde(skip)] pub(crate) permission_policy: Option, /// System-message transform. When set, the SDK injects the matching /// `action: "transform"` sections into the system message and routes /// `systemMessage.transform` RPC callbacks to it during the session. /// Use [`with_system_message_transform`](Self::with_system_message_transform) to install one. - #[serde(skip)] pub system_message_transform: Option>, } @@ -1324,7 +1293,6 @@ impl Default for SessionConfig { available_tools: None, excluded_tools: None, mcp_servers: None, - env_value_mode: default_env_value_mode(), enable_config_discovery: None, skill_directories: None, instruction_directories: None, @@ -1357,57 +1325,125 @@ impl Default for SessionConfig { } } +/// Runtime-only bundle drained out of a [`SessionConfig`] or +/// [`ResumeSessionConfig`] by [`SessionConfig::into_wire`] / +/// [`ResumeSessionConfig::into_wire`]. Holds the trait-object handlers, +/// session-fs provider, and slash commands so the wire payload struct +/// stays a pure data shape. +pub(crate) struct SessionConfigRuntime { + pub permission_handler: Option>, + pub permission_policy: Option, + pub elicitation_handler: Option>, + pub user_input_handler: Option>, + pub exit_plan_mode_handler: Option>, + pub auto_mode_switch_handler: Option>, + pub hooks_handler: Option>, + pub system_message_transform: Option>, + pub tool_handlers: HashMap>, + pub session_fs_provider: Option>, + pub commands: Option>, +} + impl SessionConfig { - /// Build the [`SessionCreateWire`] payload for `session.create` from - /// this config. Derives the request_* wire flags from handler - /// presence and the policy field; clones plain fields. - pub(crate) fn to_wire(&self, session_id: SessionId) -> crate::wire::SessionCreateWire { + /// Consume this config to produce the [`SessionCreateWire`] payload + /// for `session.create` and a [`SessionConfigRuntime`] bundle holding + /// the runtime-only fields (handlers, transforms, providers). + /// + /// Wire-format flags are derived from handler presence and the policy + /// field; runtime fields are moved out into the returned runtime so + /// the deep `Vec` / `HashMap` clones the previous + /// `&self`-based shape required are eliminated, and the order of + /// reading-vs-moving is enforced at compile time. + /// + /// [`SessionCreateWire`]: crate::wire::SessionCreateWire + pub(crate) fn into_wire( + mut self, + session_id: SessionId, + ) -> Result<(crate::wire::SessionCreateWire, SessionConfigRuntime), crate::Error> { let permission_active = self.permission_handler.is_some() || self.permission_policy.is_some(); - crate::wire::SessionCreateWire { + let request_user_input = self.user_input_handler.is_some(); + let request_exit_plan_mode = self.exit_plan_mode_handler.is_some(); + let request_auto_mode_switch = self.auto_mode_switch_handler.is_some(); + let request_elicitation = self.elicitation_handler.is_some(); + let hooks_flag = self.hooks_handler.is_some(); + + let mut tool_handlers: HashMap> = HashMap::new(); + if let Some(tools) = self.tools.as_mut() { + for tool in tools.iter_mut() { + if let Some(handler) = tool.handler.take() + && tool_handlers.insert(tool.name.clone(), handler).is_some() + { + return Err(crate::Error::InvalidConfig(format!( + "duplicate tool handler registered for name {:?}", + tool.name + ))); + } + } + } + + let wire_commands = self.commands.as_ref().map(|cmds| { + cmds.iter() + .map(|c| crate::wire::CommandWireDefinition { + name: c.name.clone(), + description: c.description.clone(), + }) + .collect() + }); + + let wire = crate::wire::SessionCreateWire { session_id, - model: self.model.clone(), - client_name: self.client_name.clone(), - reasoning_effort: self.reasoning_effort.clone(), + model: self.model, + client_name: self.client_name, + reasoning_effort: self.reasoning_effort, streaming: self.streaming, - system_message: self.system_message.clone(), - tools: self.tools.clone(), - available_tools: self.available_tools.clone(), - excluded_tools: self.excluded_tools.clone(), - mcp_servers: self.mcp_servers.clone(), + system_message: self.system_message, + tools: self.tools, + available_tools: self.available_tools, + excluded_tools: self.excluded_tools, + mcp_servers: self.mcp_servers, env_value_mode: "direct", enable_config_discovery: self.enable_config_discovery, - request_user_input: self.user_input_handler.is_some(), + request_user_input, request_permission: permission_active, - request_exit_plan_mode: self.exit_plan_mode_handler.is_some(), - request_auto_mode_switch: self.auto_mode_switch_handler.is_some(), - request_elicitation: self.elicitation_handler.is_some(), - hooks: self.hooks_handler.is_some(), - skill_directories: self.skill_directories.clone(), - instruction_directories: self.instruction_directories.clone(), - disabled_skills: self.disabled_skills.clone(), - custom_agents: self.custom_agents.clone(), - default_agent: self.default_agent.clone(), - agent: self.agent.clone(), - infinite_sessions: self.infinite_sessions.clone(), - provider: self.provider.clone(), + request_exit_plan_mode, + request_auto_mode_switch, + request_elicitation, + hooks: hooks_flag, + skill_directories: self.skill_directories, + instruction_directories: self.instruction_directories, + disabled_skills: self.disabled_skills, + custom_agents: self.custom_agents, + default_agent: self.default_agent, + agent: self.agent, + infinite_sessions: self.infinite_sessions, + provider: self.provider, enable_session_telemetry: self.enable_session_telemetry, - model_capabilities: self.model_capabilities.clone(), - config_dir: self.config_dir.clone(), - working_directory: self.working_directory.clone(), - github_token: self.github_token.clone(), - remote_session: self.remote_session.clone(), - cloud: self.cloud.clone(), + model_capabilities: self.model_capabilities, + config_dir: self.config_dir, + working_directory: self.working_directory, + github_token: self.github_token, + remote_session: self.remote_session, + cloud: self.cloud, include_sub_agent_streaming_events: self.include_sub_agent_streaming_events, - commands: self.commands.as_ref().map(|cmds| { - cmds.iter() - .map(|c| crate::wire::CommandWireDefinition { - name: c.name.clone(), - description: c.description.clone(), - }) - .collect() - }), - } + commands: wire_commands, + }; + + let runtime = SessionConfigRuntime { + permission_handler: self.permission_handler, + permission_policy: self.permission_policy, + elicitation_handler: self.elicitation_handler, + user_input_handler: self.user_input_handler, + exit_plan_mode_handler: self.exit_plan_mode_handler, + auto_mode_switch_handler: self.auto_mode_switch_handler, + hooks_handler: self.hooks_handler, + system_message_transform: self.system_message_transform, + tool_handlers, + session_fs_provider: self.session_fs_provider, + commands: self.commands, + }; + + Ok((wire, runtime)) } /// Install a [`PermissionHandler`] for this session. When omitted, the @@ -1718,71 +1754,51 @@ impl SessionConfig { /// /// See [`SessionConfig`] for the construction patterns (chained `with_*` /// builder vs. direct field assignment for `Option` pass-through) and -/// the note on snake_case vs. camelCase field naming. -#[derive(Clone, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] +/// the note on snake_case vs. camelCase field naming. This config is not +/// itself serializable — call `ResumeSessionConfig::into_wire` +/// (crate-private) to produce the wire payload. +#[derive(Clone)] #[non_exhaustive] pub struct ResumeSessionConfig { /// ID of the session to resume. pub session_id: SessionId, /// Application name sent as User-Agent context. - #[serde(skip_serializing_if = "Option::is_none")] pub client_name: Option, /// Desired reasoning effort to apply after resuming the session. - #[serde(skip_serializing_if = "Option::is_none")] pub reasoning_effort: Option, /// Enable streaming token deltas. - #[serde(skip_serializing_if = "Option::is_none")] pub streaming: Option, /// Re-supply the system message so the agent retains workspace context /// across CLI process restarts. - #[serde(skip_serializing_if = "Option::is_none")] pub system_message: Option, /// Client-defined tool declarations to re-supply on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub tools: Option>, /// Allowlist of tool names the agent may use. - #[serde(skip_serializing_if = "Option::is_none")] pub available_tools: Option>, /// Blocklist of built-in tool names. - #[serde(skip_serializing_if = "Option::is_none")] pub excluded_tools: Option>, /// Re-supply MCP servers so they remain available after app restart. - #[serde(skip_serializing_if = "Option::is_none")] pub mcp_servers: Option>, - /// See [`SessionConfig::env_value_mode`]. Always `"direct"` on the wire. - #[serde(default = "default_env_value_mode", skip_deserializing)] - pub(crate) env_value_mode: String, /// Enable config discovery on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub enable_config_discovery: Option, /// Skill directory paths passed through to the GitHub Copilot CLI on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub skill_directories: Option>, /// Additional directories to search for custom instruction files on /// resume. Forwarded to the CLI; not the same as [`skill_directories`](Self::skill_directories). - #[serde(skip_serializing_if = "Option::is_none")] pub instruction_directories: Option>, /// Skill names to disable on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub disabled_skills: Option>, /// Enable session hooks on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub hooks: Option, /// Custom agents to re-supply on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub custom_agents: Option>, /// Configures the built-in default agent on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub default_agent: Option, /// Name of the custom agent to activate. - #[serde(skip_serializing_if = "Option::is_none")] pub agent: Option, /// Re-supply infinite session configuration on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub infinite_sessions: Option, /// Re-supply BYOK provider configuration on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub provider: Option, /// Enables or disables internal session telemetry for this session. /// @@ -1791,42 +1807,32 @@ pub struct ResumeSessionConfig { /// When a custom [`provider`](Self::provider) is configured, session /// telemetry is always disabled regardless of this setting. This is /// independent of [`ClientOptions::telemetry`](crate::ClientOptions::telemetry). - #[serde(skip_serializing_if = "Option::is_none")] pub enable_session_telemetry: Option, /// Per-property model capability overrides on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub model_capabilities: Option, /// Override the default configuration directory location on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub config_dir: Option, /// Per-session working directory on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub working_directory: Option, /// Per-session GitHub token on resume. See /// [`SessionConfig::github_token`]. - #[serde(rename = "gitHubToken", skip_serializing_if = "Option::is_none")] pub github_token: Option, /// Per-session remote behavior control on resume. See /// [`SessionConfig::remote_session`]. - #[serde(skip_serializing_if = "Option::is_none")] pub remote_session: Option, /// Forward sub-agent streaming events to this connection on resume. - #[serde(skip_serializing_if = "Option::is_none")] pub include_sub_agent_streaming_events: Option, /// Slash commands registered for this session on resume. See /// [`SessionConfig::commands`] — commands are not persisted server-side, /// so the resume payload re-supplies the registration. - #[serde(skip_serializing_if = "Option::is_none", skip_deserializing)] pub commands: Option>, /// Custom session filesystem provider. Required on resume when the /// [`Client`](crate::Client) was started with /// [`ClientOptions::session_fs`](crate::ClientOptions::session_fs). /// See [`SessionConfig::session_fs_provider`]. - #[serde(skip)] pub session_fs_provider: Option>, /// Force-fail resume if the session does not exist on disk, instead of /// silently starting a new session. Wire field name stays `disableResume`. - #[serde(rename = "disableResume", skip_serializing_if = "Option::is_none")] pub suppress_resume_event: Option, /// When `true`, instructs the runtime to continue any tool calls or /// permission requests that were pending when the previous connection @@ -1835,36 +1841,27 @@ pub struct ResumeSessionConfig { /// work. /// /// [`Client::force_stop`]: crate::Client::force_stop - #[serde(skip_serializing_if = "Option::is_none")] pub continue_pending_work: Option, /// Optional permission-request handler. See /// [`SessionConfig::permission_handler`]. - #[serde(skip)] pub permission_handler: Option>, /// Optional elicitation handler. See /// [`SessionConfig::elicitation_handler`]. - #[serde(skip)] pub elicitation_handler: Option>, /// Optional user-input handler. See /// [`SessionConfig::user_input_handler`]. - #[serde(skip)] pub user_input_handler: Option>, /// Optional exit-plan-mode handler. See /// [`SessionConfig::exit_plan_mode_handler`]. - #[serde(skip)] pub exit_plan_mode_handler: Option>, /// Optional auto-mode-switch handler. See /// [`SessionConfig::auto_mode_switch_handler`]. - #[serde(skip)] pub auto_mode_switch_handler: Option>, /// Session hook handler. See [`SessionConfig::hooks_handler`]. - #[serde(skip)] pub hooks_handler: Option>, /// Permission policy. See `SessionConfig::permission_policy`. - #[serde(skip)] pub(crate) permission_policy: Option, /// System-message transform. See [`SessionConfig::system_message_transform`]. - #[serde(skip)] pub system_message_transform: Option>, } @@ -1943,56 +1940,100 @@ impl std::fmt::Debug for ResumeSessionConfig { } impl ResumeSessionConfig { - /// Build the [`SessionResumeWire`] payload for `session.resume` from - /// this config. Derives the request_* wire flags from handler - /// presence and the policy field; clones plain fields. - pub(crate) fn to_wire(&self) -> crate::wire::SessionResumeWire { + /// Consume this config to produce the [`SessionResumeWire`] payload + /// for `session.resume` and a [`SessionConfigRuntime`] bundle holding + /// the runtime-only fields (handlers, transforms, providers). + /// + /// See [`SessionConfig::into_wire`] for the design rationale. + /// + /// [`SessionResumeWire`]: crate::wire::SessionResumeWire + pub(crate) fn into_wire( + mut self, + ) -> Result<(crate::wire::SessionResumeWire, SessionConfigRuntime), crate::Error> { let permission_active = self.permission_handler.is_some() || self.permission_policy.is_some(); - crate::wire::SessionResumeWire { - session_id: self.session_id.clone(), - client_name: self.client_name.clone(), - reasoning_effort: self.reasoning_effort.clone(), + let request_user_input = self.user_input_handler.is_some(); + let request_exit_plan_mode = self.exit_plan_mode_handler.is_some(); + let request_auto_mode_switch = self.auto_mode_switch_handler.is_some(); + let request_elicitation = self.elicitation_handler.is_some(); + let hooks_flag = self.hooks_handler.is_some(); + + let mut tool_handlers: HashMap> = HashMap::new(); + if let Some(tools) = self.tools.as_mut() { + for tool in tools.iter_mut() { + if let Some(handler) = tool.handler.take() + && tool_handlers.insert(tool.name.clone(), handler).is_some() + { + return Err(crate::Error::InvalidConfig(format!( + "duplicate tool handler registered for name {:?}", + tool.name + ))); + } + } + } + + let wire_commands = self.commands.as_ref().map(|cmds| { + cmds.iter() + .map(|c| crate::wire::CommandWireDefinition { + name: c.name.clone(), + description: c.description.clone(), + }) + .collect() + }); + + let wire = crate::wire::SessionResumeWire { + session_id: self.session_id, + client_name: self.client_name, + reasoning_effort: self.reasoning_effort, streaming: self.streaming, - system_message: self.system_message.clone(), - tools: self.tools.clone(), - available_tools: self.available_tools.clone(), - excluded_tools: self.excluded_tools.clone(), - mcp_servers: self.mcp_servers.clone(), + system_message: self.system_message, + tools: self.tools, + available_tools: self.available_tools, + excluded_tools: self.excluded_tools, + mcp_servers: self.mcp_servers, env_value_mode: "direct", enable_config_discovery: self.enable_config_discovery, - request_user_input: self.user_input_handler.is_some(), + request_user_input, request_permission: permission_active, - request_exit_plan_mode: self.exit_plan_mode_handler.is_some(), - request_auto_mode_switch: self.auto_mode_switch_handler.is_some(), - request_elicitation: self.elicitation_handler.is_some(), - hooks: self.hooks_handler.is_some(), - skill_directories: self.skill_directories.clone(), - instruction_directories: self.instruction_directories.clone(), - disabled_skills: self.disabled_skills.clone(), - custom_agents: self.custom_agents.clone(), - default_agent: self.default_agent.clone(), - agent: self.agent.clone(), - infinite_sessions: self.infinite_sessions.clone(), - provider: self.provider.clone(), + request_exit_plan_mode, + request_auto_mode_switch, + request_elicitation, + hooks: hooks_flag, + skill_directories: self.skill_directories, + instruction_directories: self.instruction_directories, + disabled_skills: self.disabled_skills, + custom_agents: self.custom_agents, + default_agent: self.default_agent, + agent: self.agent, + infinite_sessions: self.infinite_sessions, + provider: self.provider, enable_session_telemetry: self.enable_session_telemetry, - model_capabilities: self.model_capabilities.clone(), - config_dir: self.config_dir.clone(), - working_directory: self.working_directory.clone(), - github_token: self.github_token.clone(), - remote_session: self.remote_session.clone(), + model_capabilities: self.model_capabilities, + config_dir: self.config_dir, + working_directory: self.working_directory, + github_token: self.github_token, + remote_session: self.remote_session, include_sub_agent_streaming_events: self.include_sub_agent_streaming_events, - commands: self.commands.as_ref().map(|cmds| { - cmds.iter() - .map(|c| crate::wire::CommandWireDefinition { - name: c.name.clone(), - description: c.description.clone(), - }) - .collect() - }), + commands: wire_commands, suppress_resume_event: self.suppress_resume_event, continue_pending_work: self.continue_pending_work, - } + }; + + let runtime = SessionConfigRuntime { + permission_handler: self.permission_handler, + permission_policy: self.permission_policy, + elicitation_handler: self.elicitation_handler, + user_input_handler: self.user_input_handler, + exit_plan_mode_handler: self.exit_plan_mode_handler, + auto_mode_switch_handler: self.auto_mode_switch_handler, + hooks_handler: self.hooks_handler, + system_message_transform: self.system_message_transform, + tool_handlers, + session_fs_provider: self.session_fs_provider, + commands: self.commands, + }; + + Ok((wire, runtime)) } /// Construct a `ResumeSessionConfig` with the given session ID and all @@ -2010,7 +2051,6 @@ impl ResumeSessionConfig { available_tools: None, excluded_tools: None, mcp_servers: None, - env_value_mode: default_env_value_mode(), enable_config_discovery: None, skill_directories: None, instruction_directories: None, @@ -3447,9 +3487,9 @@ mod tests { } #[test] - fn tool_with_parameters_handles_non_object_value() { - let tool = Tool::new("noop").with_parameters(json!(null)); - assert!(tool.parameters.is_empty()); + #[should_panic(expected = "tool parameter schema must be a JSON object")] + fn tool_with_parameters_panics_on_non_object_value() { + let _ = Tool::new("noop").with_parameters(json!(null)); } #[test] @@ -3516,7 +3556,9 @@ mod tests { // Wire flags are derived from handler presence at create_session // time, not stored on the config. With no handlers installed, every // request_* flag should serialize as false. - let wire = cfg.to_wire(SessionId::from("default-flags")); + let (wire, _runtime) = cfg + .into_wire(SessionId::from("default-flags")) + .expect("default config has no duplicate handlers"); assert!(!wire.request_user_input); assert!(!wire.request_permission); assert!(!wire.request_elicitation); @@ -3528,7 +3570,9 @@ mod tests { #[test] fn resume_session_config_new_wire_flags_off_without_handlers() { let cfg = ResumeSessionConfig::new(SessionId::from("resume-flags")); - let wire = cfg.to_wire(); + let (wire, _runtime) = cfg + .into_wire() + .expect("default resume config has no duplicate handlers"); assert!(!wire.request_user_input); assert!(!wire.request_permission); assert!(!wire.request_elicitation); @@ -3537,6 +3581,79 @@ mod tests { assert!(!wire.hooks); } + #[test] + #[allow(clippy::field_reassign_with_default)] + fn session_config_into_wire_serializes_bucket_b_fields() { + use super::{CloudSessionOptions, CloudSessionRepository}; + use std::path::PathBuf; + + let mut cfg = SessionConfig::default(); + cfg.config_dir = Some(PathBuf::from("/tmp/cfg")); + cfg.working_directory = Some(PathBuf::from("/tmp/work")); + cfg.github_token = Some("ghs_secret".to_string()); + cfg.include_sub_agent_streaming_events = Some(false); + cfg.enable_session_telemetry = Some(false); + cfg.remote_session = Some(crate::generated::api_types::RemoteSessionMode::Export); + cfg.cloud = Some(CloudSessionOptions::with_repository( + CloudSessionRepository::new("github", "copilot-sdk").with_branch("main"), + )); + + let (wire, _runtime) = cfg + .into_wire(SessionId::from("custom-id")) + .expect("no duplicate handlers"); + let wire_json = serde_json::to_value(&wire).unwrap(); + assert_eq!(wire_json["sessionId"], "custom-id"); + assert_eq!(wire_json["configDir"], "/tmp/cfg"); + assert_eq!(wire_json["workingDirectory"], "/tmp/work"); + assert_eq!(wire_json["gitHubToken"], "ghs_secret"); + assert_eq!(wire_json["includeSubAgentStreamingEvents"], false); + assert_eq!(wire_json["enableSessionTelemetry"], false); + assert_eq!(wire_json["remoteSession"], "export"); + assert_eq!(wire_json["cloud"]["repository"]["owner"], "github"); + assert_eq!(wire_json["cloud"]["repository"]["name"], "copilot-sdk"); + assert_eq!(wire_json["cloud"]["repository"]["branch"], "main"); + + // Unset fields are omitted on the wire. + let (empty_wire, _) = SessionConfig::default() + .into_wire(SessionId::from("empty")) + .expect("default has no duplicate handlers"); + let empty_json = serde_json::to_value(&empty_wire).unwrap(); + assert!(empty_json.get("gitHubToken").is_none()); + assert!(empty_json.get("enableSessionTelemetry").is_none()); + assert!(empty_json.get("remoteSession").is_none()); + assert!(empty_json.get("cloud").is_none()); + } + + #[test] + fn resume_session_config_into_wire_serializes_bucket_b_fields() { + use std::path::PathBuf; + + let mut cfg = ResumeSessionConfig::new(SessionId::from("sess-1")); + cfg.working_directory = Some(PathBuf::from("/tmp/work")); + cfg.config_dir = Some(PathBuf::from("/tmp/cfg")); + cfg.github_token = Some("ghs_secret".to_string()); + cfg.include_sub_agent_streaming_events = Some(true); + cfg.enable_session_telemetry = Some(false); + cfg.remote_session = Some(crate::generated::api_types::RemoteSessionMode::On); + + let (wire, _) = cfg.into_wire().expect("no duplicate handlers"); + let wire_json = serde_json::to_value(&wire).unwrap(); + assert_eq!(wire_json["sessionId"], "sess-1"); + assert_eq!(wire_json["workingDirectory"], "/tmp/work"); + assert_eq!(wire_json["configDir"], "/tmp/cfg"); + assert_eq!(wire_json["gitHubToken"], "ghs_secret"); + assert_eq!(wire_json["includeSubAgentStreamingEvents"], true); + assert_eq!(wire_json["enableSessionTelemetry"], false); + assert_eq!(wire_json["remoteSession"], "on"); + + // Unset remote_session is omitted on the wire. + let (empty_wire, _) = ResumeSessionConfig::new(SessionId::from("sess-2")) + .into_wire() + .expect("default resume has no duplicate handlers"); + let empty_json = serde_json::to_value(&empty_wire).unwrap(); + assert!(empty_json.get("remoteSession").is_none()); + } + #[test] fn session_config_builder_composes() { use std::collections::HashMap; @@ -3655,13 +3772,16 @@ mod tests { fn resume_session_config_serializes_continue_pending_work_to_camel_case() { let cfg = ResumeSessionConfig::new(SessionId::from("sess-1")).with_continue_pending_work(true); - let wire = serde_json::to_value(&cfg).unwrap(); - assert_eq!(wire["continuePendingWork"], true); + let (wire, _) = cfg.into_wire().expect("no duplicate handlers"); + let json = serde_json::to_value(&wire).unwrap(); + assert_eq!(json["continuePendingWork"], true); // Unset case — skip_serializing_if must omit the field. - let cfg = ResumeSessionConfig::new(SessionId::from("sess-2")); - let wire = serde_json::to_value(&cfg).unwrap(); - assert!(wire.get("continuePendingWork").is_none()); + let (wire, _) = ResumeSessionConfig::new(SessionId::from("sess-2")) + .into_wire() + .expect("no duplicate handlers"); + let json = serde_json::to_value(&wire).unwrap(); + assert!(json.get("continuePendingWork").is_none()); } /// The Rust field is `suppress_resume_event`, but the wire field stays @@ -3671,17 +3791,10 @@ mod tests { fn resume_session_config_serializes_suppress_resume_event_to_disable_resume_on_wire() { let cfg = ResumeSessionConfig::new(SessionId::from("sess-1")).with_suppress_resume_event(true); - let wire = serde_json::to_value(&cfg).unwrap(); - assert_eq!(wire["disableResume"], true); - assert!(wire.get("suppressResumeEvent").is_none()); - - // Round-trip: deserialize from the wire shape. - let json = serde_json::json!({ - "sessionId": "sess-2", - "disableResume": true, - }); - let parsed: ResumeSessionConfig = serde_json::from_value(json).unwrap(); - assert_eq!(parsed.suppress_resume_event, Some(true)); + let (wire, _) = cfg.into_wire().expect("no duplicate handlers"); + let json = serde_json::to_value(&wire).unwrap(); + assert_eq!(json["disableResume"], true); + assert!(json.get("suppressResumeEvent").is_none()); } /// `instruction_directories` must serialize to wire as @@ -3690,16 +3803,21 @@ mod tests { fn session_config_serializes_instruction_directories_to_camel_case() { let cfg = SessionConfig::default().with_instruction_directories([PathBuf::from("/tmp/instr")]); - let wire = serde_json::to_value(&cfg).unwrap(); + let (wire, _) = cfg + .into_wire(SessionId::from("instr-on")) + .expect("no duplicate handlers"); + let json = serde_json::to_value(&wire).unwrap(); assert_eq!( - wire["instructionDirectories"], + json["instructionDirectories"], serde_json::json!(["/tmp/instr"]) ); // Unset case — skip_serializing_if must omit the field. - let cfg = SessionConfig::default(); - let wire = serde_json::to_value(&cfg).unwrap(); - assert!(wire.get("instructionDirectories").is_none()); + let (wire, _) = SessionConfig::default() + .into_wire(SessionId::from("instr-off")) + .expect("no duplicate handlers"); + let json = serde_json::to_value(&wire).unwrap(); + assert!(json.get("instructionDirectories").is_none()); } /// Same check on the resume path. Forwarded to the CLI on @@ -3708,15 +3826,18 @@ mod tests { fn resume_session_config_serializes_instruction_directories_to_camel_case() { let cfg = ResumeSessionConfig::new(SessionId::from("sess-1")) .with_instruction_directories([PathBuf::from("/tmp/instr")]); - let wire = serde_json::to_value(&cfg).unwrap(); + let (wire, _) = cfg.into_wire().expect("no duplicate handlers"); + let json = serde_json::to_value(&wire).unwrap(); assert_eq!( - wire["instructionDirectories"], + json["instructionDirectories"], serde_json::json!(["/tmp/instr"]) ); - let cfg = ResumeSessionConfig::new(SessionId::from("sess-2")); - let wire = serde_json::to_value(&cfg).unwrap(); - assert!(wire.get("instructionDirectories").is_none()); + let (wire, _) = ResumeSessionConfig::new(SessionId::from("sess-2")) + .into_wire() + .expect("no duplicate handlers"); + let json = serde_json::to_value(&wire).unwrap(); + assert!(json.get("instructionDirectories").is_none()); } #[test] diff --git a/rust/tests/session_test.rs b/rust/tests/session_test.rs index 5140d5413..ed3698951 100644 --- a/rust/tests/session_test.rs +++ b/rust/tests/session_test.rs @@ -2823,45 +2823,25 @@ fn session_config_serializes_bucket_b_fields() { CloudSessionOptions, CloudSessionRepository, SessionConfig, SessionId, }; - let cfg = { - let mut cfg = SessionConfig::default(); - cfg.session_id = Some(SessionId::from("custom-id")); - cfg.config_dir = Some(PathBuf::from("/tmp/cfg")); - cfg.working_directory = Some(PathBuf::from("/tmp/work")); - cfg.github_token = Some("ghs_secret".to_string()); - cfg.include_sub_agent_streaming_events = Some(false); - cfg.enable_session_telemetry = Some(false); - cfg.remote_session = - Some(github_copilot_sdk::generated::api_types::RemoteSessionMode::Export); - cfg.cloud = Some(CloudSessionOptions::with_repository( - CloudSessionRepository::new("github", "copilot-sdk").with_branch("main"), - )); - cfg - }; - let json = serde_json::to_value(&cfg).unwrap(); - assert_eq!(json["sessionId"], "custom-id"); - assert_eq!(json["configDir"], "/tmp/cfg"); - assert_eq!(json["workingDirectory"], "/tmp/work"); - assert_eq!(json["gitHubToken"], "ghs_secret"); - assert_eq!(json["includeSubAgentStreamingEvents"], false); - assert_eq!(json["enableSessionTelemetry"], false); - assert_eq!(json["remoteSession"], "export"); - assert_eq!(json["cloud"]["repository"]["owner"], "github"); - assert_eq!(json["cloud"]["repository"]["name"], "copilot-sdk"); - assert_eq!(json["cloud"]["repository"]["branch"], "main"); + let mut cfg = SessionConfig::default(); + cfg.session_id = Some(SessionId::from("custom-id")); + cfg.config_dir = Some(PathBuf::from("/tmp/cfg")); + cfg.working_directory = Some(PathBuf::from("/tmp/work")); + cfg.github_token = Some("ghs_secret".to_string()); + cfg.include_sub_agent_streaming_events = Some(false); + cfg.enable_session_telemetry = Some(false); + cfg.remote_session = Some(github_copilot_sdk::generated::api_types::RemoteSessionMode::Export); + cfg.cloud = Some(CloudSessionOptions::with_repository( + CloudSessionRepository::new("github", "copilot-sdk").with_branch("main"), + )); // Debug never leaks the token. let debug = format!("{cfg:?}"); assert!(!debug.contains("ghs_secret"), "leaked token: {debug}"); assert!(debug.contains(""), "missing redaction: {debug}"); - - // Unset fields are omitted on the wire. - let empty = serde_json::to_value(SessionConfig::default()).unwrap(); - assert!(empty.get("sessionId").is_none()); - assert!(empty.get("gitHubToken").is_none()); - assert!(empty.get("enableSessionTelemetry").is_none()); - assert!(empty.get("remoteSession").is_none()); - assert!(empty.get("cloud").is_none()); + // Wire-format coverage now lives in the in-crate unit tests next to + // `SessionConfig::into_wire` — the wire payload is `pub(crate)` so + // external integration tests can only inspect the user-facing config. } #[test] @@ -2877,22 +2857,11 @@ fn resume_session_config_serializes_bucket_b_fields() { cfg.include_sub_agent_streaming_events = Some(true); cfg.enable_session_telemetry = Some(false); cfg.remote_session = Some(github_copilot_sdk::generated::api_types::RemoteSessionMode::On); - let json = serde_json::to_value(&cfg).unwrap(); - assert_eq!(json["sessionId"], "sess-1"); - assert_eq!(json["workingDirectory"], "/tmp/work"); - assert_eq!(json["configDir"], "/tmp/cfg"); - assert_eq!(json["gitHubToken"], "ghs_secret"); - assert_eq!(json["includeSubAgentStreamingEvents"], true); - assert_eq!(json["enableSessionTelemetry"], false); - assert_eq!(json["remoteSession"], "on"); - - // Unset remote_session is omitted on the wire. - let empty = ResumeSessionConfig::new(SessionId::from("sess-2")); - let empty_json = serde_json::to_value(&empty).unwrap(); - assert!(empty_json.get("remoteSession").is_none()); let debug = format!("{cfg:?}"); assert!(!debug.contains("ghs_secret"), "leaked token: {debug}"); + // Wire-format coverage lives in the in-crate unit tests; see + // `ResumeSessionConfig::into_wire`. } // ===================================================================== From 878a404a7f66d9f0b6ce2acfdf066a8f8ab600f8 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 22 May 2026 18:27:24 +0100 Subject: [PATCH 2/3] Fix define_tool description: function, not macro Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- rust/README.md | 2 +- rust/src/tool.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/README.md b/rust/README.md index 4dc808470..2de08f35d 100644 --- a/rust/README.md +++ b/rust/README.md @@ -375,7 +375,7 @@ Tools are named types (not closures) — visible in stack traces and navigable v Tools without an attached handler (`Tool::with_handler` never called) are declaration-only: the SDK advertises them on the wire but doesn't dispatch invocations to anything. Useful when another connected client services the tool. -For trivial tools that don't need a named type, the `define_tool` macro (available with the `derive` feature) collapses the definition to a single expression and returns a fully-formed `Tool` with handler attached: +For trivial tools that don't need a named type, the `define_tool` helper function (available with the `derive` feature) collapses the definition to a single expression and returns a fully-formed `Tool` with handler attached: ```rust,ignore use github_copilot_sdk::tool::{define_tool, JsonSchema}; diff --git a/rust/src/tool.rs b/rust/src/tool.rs index 326b06563..b9b44bc0a 100644 --- a/rust/src/tool.rs +++ b/rust/src/tool.rs @@ -187,8 +187,8 @@ pub fn convert_mcp_call_tool_result(value: &serde_json::Value) -> Option Date: Fri, 22 May 2026 18:28:16 +0100 Subject: [PATCH 3/3] rustfmt nightly: order std before super imports Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- rust/src/types.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rust/src/types.rs b/rust/src/types.rs index 1db929895..df5767aa4 100644 --- a/rust/src/types.rs +++ b/rust/src/types.rs @@ -3584,9 +3584,10 @@ mod tests { #[test] #[allow(clippy::field_reassign_with_default)] fn session_config_into_wire_serializes_bucket_b_fields() { - use super::{CloudSessionOptions, CloudSessionRepository}; use std::path::PathBuf; + use super::{CloudSessionOptions, CloudSessionRepository}; + let mut cfg = SessionConfig::default(); cfg.config_dir = Some(PathBuf::from("/tmp/cfg")); cfg.working_directory = Some(PathBuf::from("/tmp/work"));