Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rust/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions rust/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ rustdoc-args = ["--cfg", "docsrs"]

[dependencies]
async-trait = "0.1"
indexmap = { version = "2", features = ["serde"] }
schemars = { version = "1", optional = true }
serde = { version = "1", features = ["derive"] }
serde_json = "1"
Expand Down
6 changes: 5 additions & 1 deletion rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,11 @@ use std::sync::{Arc, OnceLock};
use std::time::{Duration, Instant};

use async_trait::async_trait;
// JSON-RPC wire types are internal transport details (like Go SDK's internal/jsonrpc2/).
/// Re-export of [`indexmap::IndexMap`], used for order-preserving maps in the
/// public API (e.g. [`Tool::parameters`](types::Tool::parameters) and
/// `SessionConfig::mcp_servers`) so serialized key order stays deterministic.
pub use indexmap::IndexMap;
// JSON-RPC wire types are internal transport details.
// External callers interact via Client/Session methods, not raw RPC.
pub(crate) use jsonrpc::{
JsonRpcClient, JsonRpcError, JsonRpcNotification, JsonRpcRequest, JsonRpcResponse, error_codes,
Expand Down
45 changes: 41 additions & 4 deletions rust/src/tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@
//! Enable the `derive` feature for `schema_for`, which generates JSON
//! Schema from Rust types via `schemars`.

use std::collections::HashMap;

use async_trait::async_trait;
use indexmap::IndexMap;
/// Re-export of [`schemars::JsonSchema`] for deriving tool parameter schemas.
#[cfg(feature = "derive")]
pub use schemars::JsonSchema;
Expand Down Expand Up @@ -80,14 +79,14 @@ pub fn schema_for<T: schemars::JsonSchema>() -> serde_json::Value {
/// tool.parameters = tool_parameters(serde_json::json!({"type": "object"}));
/// # let _ = tool;
/// ```
pub fn tool_parameters(schema: serde_json::Value) -> HashMap<String, serde_json::Value> {
pub fn tool_parameters(schema: serde_json::Value) -> IndexMap<String, serde_json::Value> {
try_tool_parameters(schema).expect("tool parameter schema must be a JSON object")
}

/// Fallible variant of [`tool_parameters`] for callers handling dynamic schema input.
pub fn try_tool_parameters(
schema: serde_json::Value,
) -> Result<HashMap<String, serde_json::Value>, serde_json::Error> {
) -> Result<IndexMap<String, serde_json::Value>, serde_json::Error> {
serde_json::from_value(schema)
}

Expand Down Expand Up @@ -403,6 +402,44 @@ mod tests {
assert!(err.is_data());
}

#[test]
fn tool_parameters_serialize_in_deterministic_order() {
// Regression: `Tool.parameters` was a `HashMap`, whose per-instance
// random iteration order made the serialized top-level schema keys
// differ between constructions (and between sessions), busting the
// model provider's prompt cache. `IndexMap` keeps the order stable.
let schema = serde_json::json!({
"type": "object",
"properties": {
"url": { "type": "string" },
"count": { "type": "integer" }
},
"required": ["url"],
"additionalProperties": false
});

let build = || Tool {
name: "fetch".to_string(),
parameters: tool_parameters(schema.clone()),
..Default::default()
};

let expected = serde_json::to_string(&build()).expect("serialize tool");
for _ in 0..64 {
let actual = serde_json::to_string(&build()).expect("serialize tool");
assert_eq!(actual, expected);
}

// Pin the exact top-level key order so a regression to any
// order-randomizing container is caught, not just internal drift.
let tool = build();
let keys: Vec<&str> = tool.parameters.keys().map(String::as_str).collect();
assert_eq!(
keys,
["additionalProperties", "properties", "required", "type"]
);
}

#[test]
fn convert_mcp_call_tool_result_collects_text_and_binary_content() {
let result = convert_mcp_call_tool_result(&serde_json::json!({
Expand Down
89 changes: 68 additions & 21 deletions rust/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use std::path::{Path, PathBuf};
use std::sync::Arc;
use std::time::Duration;

use indexmap::IndexMap;
use serde::{Deserialize, Serialize};
use serde_json::Value;

Expand Down Expand Up @@ -332,8 +333,8 @@ pub struct Tool {
#[serde(default, skip_serializing_if = "Option::is_none")]
pub instructions: Option<String>,
/// JSON Schema for the tool's input parameters.
#[serde(default, skip_serializing_if = "HashMap::is_empty")]
pub parameters: HashMap<String, Value>,
#[serde(default, skip_serializing_if = "IndexMap::is_empty")]
pub parameters: IndexMap<String, Value>,
/// When `true`, this tool replaces a built-in tool of the same name
/// (e.g. supplying a custom `grep` that the agent uses in place of the
/// CLI's built-in implementation).
Expand Down Expand Up @@ -614,7 +615,7 @@ pub struct CustomAgentConfig {
pub prompt: String,
/// MCP servers specific to this agent.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub mcp_servers: Option<HashMap<String, McpServerConfig>>,
pub mcp_servers: Option<IndexMap<String, McpServerConfig>>,
/// Whether the agent is available for model inference.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub infer: Option<bool>,
Expand Down Expand Up @@ -668,7 +669,7 @@ impl CustomAgentConfig {
}

/// Configure agent-specific MCP servers.
pub fn with_mcp_servers(mut self, mcp_servers: HashMap<String, McpServerConfig>) -> Self {
pub fn with_mcp_servers(mut self, mcp_servers: IndexMap<String, McpServerConfig>) -> Self {
self.mcp_servers = Some(mcp_servers);
self
}
Expand Down Expand Up @@ -929,8 +930,8 @@ impl ExtensionInfo {
///
/// ```
/// # use github_copilot_sdk::types::{McpServerConfig, McpStdioServerConfig, McpHttpServerConfig};
/// # use std::collections::HashMap;
/// let mut servers = HashMap::new();
/// # use github_copilot_sdk::IndexMap;
/// let mut servers = IndexMap::new();
/// servers.insert(
/// "playwright".to_string(),
/// McpServerConfig::Stdio(McpStdioServerConfig {
Expand Down Expand Up @@ -1609,7 +1610,7 @@ pub struct SessionConfig {
/// configured.
pub excluded_builtin_agents: Option<Vec<String>>,
/// MCP server configurations passed through to the CLI.
pub mcp_servers: Option<HashMap<String, McpServerConfig>>,
pub mcp_servers: Option<IndexMap<String, McpServerConfig>>,
/// Controls how MCP OAuth tokens are stored for this session.
///
/// - `"persistent"` — tokens are stored in the OS keychain (shared across sessions).
Expand Down Expand Up @@ -2057,7 +2058,7 @@ impl SessionConfig {
///
/// 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<Tool>` / `HashMap<String, Value>` clones the previous
/// the deep `Vec<Tool>` / `IndexMap<String, Value>` clones the previous
/// `&self`-based shape required are eliminated, and the order of
/// reading-vs-moving is enforced at compile time.
///
Expand Down Expand Up @@ -2421,7 +2422,7 @@ impl SessionConfig {
}

/// Set MCP server configurations passed through to the CLI.
pub fn with_mcp_servers(mut self, servers: HashMap<String, McpServerConfig>) -> Self {
pub fn with_mcp_servers(mut self, servers: IndexMap<String, McpServerConfig>) -> Self {
self.mcp_servers = Some(servers);
self
}
Expand Down Expand Up @@ -2793,7 +2794,7 @@ pub struct ResumeSessionConfig {
/// configured.
pub excluded_builtin_agents: Option<Vec<String>>,
/// Re-supply MCP servers so they remain available after app restart.
pub mcp_servers: Option<HashMap<String, McpServerConfig>>,
pub mcp_servers: Option<IndexMap<String, McpServerConfig>>,
/// Controls how MCP OAuth tokens are stored for this session.
/// See [`SessionConfig::mcp_oauth_token_storage`] for details.
pub mcp_oauth_token_storage: Option<String>,
Expand Down Expand Up @@ -3505,7 +3506,7 @@ impl ResumeSessionConfig {
}

/// Re-supply MCP server configurations on resume.
pub fn with_mcp_servers(mut self, servers: HashMap<String, McpServerConfig>) -> Self {
pub fn with_mcp_servers(mut self, servers: IndexMap<String, McpServerConfig>) -> Self {
self.mcp_servers = Some(servers);
self
}
Expand Down Expand Up @@ -5168,10 +5169,11 @@ mod tests {
AgentMode, Attachment, AttachmentLineRange, AttachmentSelectionPosition,
AttachmentSelectionRange, AzureProviderOptions, CapiSessionOptions, ConnectionState,
CustomAgentConfig, DeliveryMode, ExtensionInfo, GitHubReferenceType, InfiniteSessionConfig,
LargeToolOutputConfig, MemoryConfiguration, NamedProviderConfig, ProviderConfig,
ProviderModelConfig, ReasoningSummary, ResumeSessionConfig, SessionConfig, SessionEvent,
SessionId, SystemMessageConfig, Tool, ToolBinaryResult, ToolResult, ToolResultExpanded,
ToolResultResponse, ensure_attachment_display_names,
LargeToolOutputConfig, McpServerConfig, McpStdioServerConfig, MemoryConfiguration,
NamedProviderConfig, ProviderConfig, ProviderModelConfig, ReasoningSummary,
ResumeSessionConfig, SessionConfig, SessionEvent, SessionId, SystemMessageConfig, Tool,
ToolBinaryResult, ToolResult, ToolResultExpanded, ToolResultResponse,
ensure_attachment_display_names,
};
use crate::generated::session_events::TypedSessionEvent;

Expand Down Expand Up @@ -5720,7 +5722,7 @@ mod tests {

#[test]
fn session_config_builder_composes() {
use std::collections::HashMap;
use indexmap::IndexMap;

let cfg = SessionConfig::default()
.with_session_id(SessionId::from("sess-1"))
Expand All @@ -5733,7 +5735,7 @@ mod tests {
.with_tools([Tool::new("greet")])
.with_available_tools(["bash", "view"])
.with_excluded_tools(["dangerous"])
.with_mcp_servers(HashMap::new())
.with_mcp_servers(IndexMap::new())
.with_mcp_oauth_token_storage("persistent")
.with_enable_config_discovery(true)
.with_enable_on_demand_instruction_discovery(true)
Expand Down Expand Up @@ -5794,7 +5796,7 @@ mod tests {

#[test]
fn resume_session_config_builder_composes() {
use std::collections::HashMap;
use indexmap::IndexMap;

let cfg = ResumeSessionConfig::new(SessionId::from("sess-2"))
.with_client_name("test-app")
Expand All @@ -5804,7 +5806,7 @@ mod tests {
.with_tools([Tool::new("greet")])
.with_available_tools(["bash", "view"])
.with_excluded_tools(["dangerous"])
.with_mcp_servers(HashMap::new())
.with_mcp_servers(IndexMap::new())
.with_mcp_oauth_token_storage("persistent")
.with_enable_config_discovery(true)
.with_enable_on_demand_instruction_discovery(false)
Expand Down Expand Up @@ -5942,13 +5944,13 @@ mod tests {

#[test]
fn custom_agent_config_builder_composes() {
use std::collections::HashMap;
use indexmap::IndexMap;

let cfg = CustomAgentConfig::new("researcher", "You are a research assistant.")
.with_display_name("Research Assistant")
.with_description("Investigates technical questions.")
.with_tools(["bash", "view"])
.with_mcp_servers(HashMap::new())
.with_mcp_servers(IndexMap::new())
.with_infer(true)
.with_skills(["rust-coding-skill"]);

Expand All @@ -5971,6 +5973,51 @@ mod tests {
);
}

#[test]
fn mcp_servers_serialize_in_insertion_order() {
use indexmap::IndexMap;

// Regression: `mcp_servers` was a `HashMap`, so the server keys (and
// thus the `session.create` payload) serialized in a per-process
// random order; `IndexMap` pins them to insertion order. The long
// sequence makes a `HashMap` regression reproduce this exact order by
// chance only 1/N!, avoiding a flaky false pass.
let order = [
"zebra", "quartz", "delta", "ivy", "mango", "bravo", "xenon", "amber", "falcon",
"ceres", "nova", "kelp", "otter", "yodel", "plum", "garnet",
];
let mut servers = IndexMap::new();
for name in order {
servers.insert(
name.to_string(),
McpServerConfig::Stdio(McpStdioServerConfig {
command: "run".to_string(),
..Default::default()
}),
);
}

let (wire, _runtime) = SessionConfig::default()
.with_mcp_servers(servers)
.into_wire(None)
.expect("into_wire should succeed");
let json = serde_json::to_string(&wire).expect("serialize wire");

let positions: Vec<usize> = order
.iter()
.map(|name| {
json.find(&format!("\"{name}\""))
.unwrap_or_else(|| panic!("server {name} missing from wire JSON"))
})
.collect();
let mut ascending = positions.clone();
ascending.sort_unstable();
assert_eq!(
positions, ascending,
"mcp server keys must serialize in insertion order: {json}"
);
}

#[test]
fn infinite_session_config_builder_composes() {
let cfg = InfiniteSessionConfig::new()
Expand Down
6 changes: 3 additions & 3 deletions rust/src/wire.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
//! configs hold trait-object handlers, the wire structs hold only the
//! plain data the runtime needs.

use std::collections::HashMap;
use std::path::PathBuf;

use indexmap::IndexMap;
use serde::Serialize;

use crate::canvas::CanvasDeclaration;
Expand Down Expand Up @@ -83,7 +83,7 @@ pub(crate) struct SessionCreateWire {
/// naturally (everything matching X except Y).
pub tool_filter_precedence: &'static str,
#[serde(skip_serializing_if = "Option::is_none")]
pub mcp_servers: Option<HashMap<String, McpServerConfig>>,
pub mcp_servers: Option<IndexMap<String, McpServerConfig>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub mcp_oauth_token_storage: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down Expand Up @@ -213,7 +213,7 @@ pub(crate) struct SessionResumeWire {
/// SDK always sends `"excluded"`. See create-wire docs.
pub tool_filter_precedence: &'static str,
#[serde(skip_serializing_if = "Option::is_none")]
pub mcp_servers: Option<HashMap<String, McpServerConfig>>,
pub mcp_servers: Option<IndexMap<String, McpServerConfig>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub mcp_oauth_token_storage: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down
10 changes: 5 additions & 5 deletions rust/tests/e2e/mcp_oauth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use github_copilot_sdk::handler::{McpAuthHandler, McpAuthRequest, McpAuthResult}
use github_copilot_sdk::rpc::{McpAppsCallToolRequest, McpListToolsRequest};
use github_copilot_sdk::session::Session;
use github_copilot_sdk::session_events::{McpOauthRequestReason, McpServerStatus};
use github_copilot_sdk::{McpHttpServerConfig, McpServerConfig, RequestId, SessionId};
use github_copilot_sdk::{IndexMap, McpHttpServerConfig, McpServerConfig, RequestId, SessionId};
use parking_lot::Mutex;
use serde::Deserialize;
use serde_json::Value;
Expand Down Expand Up @@ -40,7 +40,7 @@ async fn should_satisfy_mcp_oauth_using_host_provided_token() {
.create_session(
ctx.approve_all_session_config()
.with_mcp_auth_handler(handler.clone())
.with_mcp_servers(HashMap::from([(
.with_mcp_servers(IndexMap::from([(
server_name.to_string(),
McpServerConfig::Http(McpHttpServerConfig {
tools: Some(vec!["*".to_string()]),
Expand Down Expand Up @@ -129,7 +129,7 @@ async fn should_request_replacement_tokens_across_mcp_oauth_lifecycle() {
ctx.approve_all_session_config()
.with_enable_mcp_apps(true)
.with_mcp_auth_handler(handler.clone())
.with_mcp_servers(HashMap::from([(
.with_mcp_servers(IndexMap::from([(
server_name.to_string(),
McpServerConfig::Http(McpHttpServerConfig {
tools: Some(vec!["*".to_string()]),
Expand Down Expand Up @@ -194,7 +194,7 @@ async fn should_cancel_pending_mcp_oauth_request() {
.create_session(
ctx.approve_all_session_config()
.with_mcp_auth_handler(handler.clone())
.with_mcp_servers(HashMap::from([(
.with_mcp_servers(IndexMap::from([(
server_name.to_string(),
McpServerConfig::Http(McpHttpServerConfig {
tools: Some(vec!["*".to_string()]),
Expand Down Expand Up @@ -250,7 +250,7 @@ async fn should_resolve_pending_mcp_oauth_request_through_rpc() {
ctx.approve_all_session_config()
.with_enable_mcp_apps(true)
.with_mcp_auth_handler(handler)
.with_mcp_servers(HashMap::from([(
.with_mcp_servers(IndexMap::from([(
server_name.to_string(),
McpServerConfig::Http(McpHttpServerConfig {
tools: Some(vec!["*".to_string()]),
Expand Down
Loading
Loading