diff --git a/rust/README.md b/rust/README.md index 87484defec..38e6a155cc 100644 --- a/rust/README.md +++ b/rust/README.md @@ -110,6 +110,17 @@ When allowed and performed, `session.transcript_recovery()` returns the planned backup path, invalid line numbers, and whether `session.start` was moved; the backup is written on the next append rather than during resume. +Rust resumes await a best-effort `session.skills.reload` by default: returned +errors are logged, but an unanswered request delays resume completion. +`ResumeSessionConfig::with_reload_skills(false)` omits that automatic request +when the application owns required skill reconciliation before skill-dependent +work. It is SDK-local, does not disable skills, and does not prove the catalog +or disabled-skill preferences are current. Explicit +`session.rpc().skills().reload().await` remains available. This option does not +gate continued pending work or work already running on another connection; +applications must account for those paths in their own reconciliation barrier. +It does not cancel or settle a reload that was already issued. + After `Client::start` succeeds, inspect its startup cost without parsing logs: ```rust,ignore @@ -1456,6 +1467,10 @@ larger results return a filesystem error before encoding or decoding. ### Rust-only API +Rust also exposes `ResumeSessionConfig::with_reload_skills` to control its +automatic post-resume skill reload; other SDKs such as Node and .NET do not +issue that automatic request. The Rust default is unchanged. + A handful of conveniences exist only on the Rust SDK as of 0.1.0. These are surface areas where Rust idiom (newtypes, enums, trait objects) gives a clearly nicer shape than Node/Python/Go/.NET currently expose. Rust diff --git a/rust/src/ahp_host/factory_tests.rs b/rust/src/ahp_host/factory_tests.rs index bb328c7acd..894f06131c 100644 --- a/rust/src/ahp_host/factory_tests.rs +++ b/rust/src/ahp_host/factory_tests.rs @@ -677,6 +677,24 @@ fn host_settings_round_trip_without_callbacks_or_losing_constraints() { assert!(wire.get("mcpOauthTokenStorage").is_none()); } +#[test] +fn host_resume_settings_do_not_forward_local_skill_reload_policy() { + let config = crate::ResumeSessionConfig::new(crate::SessionId::from("skills")); + let expected = resume_config_for_host(&config).unwrap(); + for reload in [false, true] { + assert_eq!( + resume_config_for_host(&config.clone().with_reload_skills(reload)).unwrap(), + expected + ); + } + let decoded = + resume_config_from_host(&serde_json::from_value(expected.clone()).unwrap()).unwrap(); + assert_eq!(decoded.reload_skills, None); + let mut unsupported = expected; + unsupported["reloadSkills"] = json!(false); + assert!(resume_config_from_host(&serde_json::from_value(unsupported).unwrap()).is_err()); +} + #[tokio::test] async fn release_before_request_poll_prevents_factory_invocation() { let (client, mut peer) = fixture(); diff --git a/rust/src/session.rs b/rust/src/session.rs index 1ba8325095..ca2b6fe14b 100644 --- a/rust/src/session.rs +++ b/rust/src/session.rs @@ -1701,8 +1701,10 @@ impl Client { /// Resume an existing session on the CLI. /// - /// Sends `session.resume` and `session.skills.reload`, registers the - /// session on the router, and spawns the event loop. + /// Sends `session.resume`, registers the session on the router, and + /// spawns the event loop. By default, also awaits a best-effort + /// `session.skills.reload`; [`ResumeSessionConfig::with_reload_skills(false)`](ResumeSessionConfig::with_reload_skills) + /// omits that request when the caller owns skill reconciliation. /// /// All callbacks (event handler, hooks, transform) are configured /// via [`ResumeSessionConfig`] using its `with_*` builder methods. @@ -2084,8 +2086,10 @@ impl Client { /// Resume an existing session on the CLI. /// - /// Sends `session.resume` and `session.skills.reload`, registers the - /// session on the router, and spawns the event loop. + /// Sends `session.resume`, registers the session on the router, and + /// spawns the event loop. By default, also awaits a best-effort + /// `session.skills.reload` unless [`ResumeSessionConfig::reload_skills`] + /// is explicitly `false`. /// /// All callbacks (event handler, hooks, transform) are configured /// via [`ResumeSessionConfig`] using its `with_*` builder methods. @@ -2111,6 +2115,7 @@ impl Client { None }; let session_id = config.session_id.clone(); + let reload_skills = config.reload_skills.unwrap_or(true); if config.hooks_handler.is_some() && config.hooks.is_none() { config.hooks = Some(true); } @@ -2327,27 +2332,28 @@ impl Client { registration.cleanup(event_loop).await; return Err(error); } - // Reload skills after resume (best-effort). - let skills_reload_start = Instant::now(); - if let Err(e) = self - .call( - "session.skills.reload", - Some(serde_json::json!({ "sessionId": session_id })), - ) - .await - { - warn!( - elapsed_ms = skills_reload_start.elapsed().as_millis(), - session_id = %session_id, - error = %e, - "Client::resume_session skills reload request failed" - ); - } else { - tracing::debug!( - elapsed_ms = skills_reload_start.elapsed().as_millis(), - session_id = %session_id, - "Client::resume_session skills reload request completed successfully" - ); + if reload_skills { + let skills_reload_start = Instant::now(); + if let Err(e) = self + .call( + "session.skills.reload", + Some(serde_json::json!({ "sessionId": session_id })), + ) + .await + { + warn!( + elapsed_ms = skills_reload_start.elapsed().as_millis(), + session_id = %session_id, + error = %e, + "Client::resume_session skills reload request failed" + ); + } else { + tracing::debug!( + elapsed_ms = skills_reload_start.elapsed().as_millis(), + session_id = %session_id, + "Client::resume_session skills reload request completed successfully" + ); + } } *capabilities.write() = resume_result.capabilities.unwrap_or_default(); diff --git a/rust/src/types.rs b/rust/src/types.rs index 9917693110..fbc58c2870 100644 --- a/rust/src/types.rs +++ b/rust/src/types.rs @@ -3604,6 +3604,20 @@ impl SessionConfig { pub struct ResumeSessionConfig { /// ID of the session to resume. pub session_id: SessionId, + /// Whether the SDK automatically reloads skills after the runtime resumes. + /// + /// Unset or `true` preserves the awaited, best-effort + /// `session.skills.reload` call: returned errors are logged, but an + /// unanswered request still delays resume completion. Set `false` only + /// when the caller owns any required skill reconciliation before + /// skill-dependent work. + /// + /// This SDK-local option is not sent to the runtime. It does not disable + /// skills or imply that the catalog or disabled-skill preferences are + /// current. It does not gate continued pending work or work already + /// running on another connection, nor cancel or settle an already-issued + /// reload. + pub reload_skills: Option, /// Model to use for this session (e.g. `"gpt-4"`, `"claude-sonnet-4"`). /// Can change the model when resuming. pub model: Option, @@ -3900,6 +3914,7 @@ impl std::fmt::Debug for ResumeSessionConfig { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("ResumeSessionConfig") .field("session_id", &self.session_id) + .field("reload_skills", &self.reload_skills) .field("model", &self.model) .field("allowed_models", &self.allowed_models) .field("client_name", &self.client_name) @@ -4206,6 +4221,7 @@ impl ResumeSessionConfig { pub fn new(session_id: SessionId) -> Self { Self { session_id, + reload_skills: None, model: None, allowed_models: None, client_name: None, @@ -4302,6 +4318,12 @@ impl ResumeSessionConfig { } } + /// Set [`Self::reload_skills`], controlling the SDK's automatic skill reload. + pub fn with_reload_skills(mut self, reload_skills: bool) -> Self { + self.reload_skills = Some(reload_skills); + self + } + /// Install a [`PermissionHandler`] for the resumed session. pub fn with_permission_handler(mut self, handler: Arc) -> Self { self.permission_handler = Some(handler); diff --git a/rust/src/types/tests.rs b/rust/src/types/tests.rs index 0749d946bd..215f1ffa63 100644 --- a/rust/src/types/tests.rs +++ b/rust/src/types/tests.rs @@ -1149,6 +1149,31 @@ fn resume_session_config_serializes_continue_pending_work_to_camel_case() { assert!(json.get("continuePendingWork").is_none()); } +#[test] +fn resume_reload_skills_is_local_only_and_preserves_skill_configuration() { + let config = ResumeSessionConfig::new(SessionId::from("skills")) + .with_enable_skills(true) + .with_enable_config_discovery(true) + .with_skill_directories([PathBuf::from("skills")]) + .with_disabled_skills(["disabled"]); + assert_eq!(config.reload_skills, None); + let (wire, _) = config.clone().into_wire().unwrap(); + let expected = serde_json::to_value(wire).unwrap(); + assert_eq!(expected["enableSkills"], true); + assert_eq!(expected["enableConfigDiscovery"], true); + assert_eq!(expected["skillDirectories"], json!(["skills"])); + assert_eq!(expected["disabledSkills"], json!(["disabled"])); + + for reload in [false, true] { + let configured = config.clone().with_reload_skills(reload); + assert_eq!(configured.reload_skills, Some(reload)); + assert_eq!(configured.clone().reload_skills, Some(reload)); + assert!(format!("{configured:?}").contains(&format!("reload_skills: Some({reload})"))); + let (wire, _) = configured.into_wire().unwrap(); + assert_eq!(serde_json::to_value(wire).unwrap(), expected); + } +} + #[test] fn resume_policy_and_recovery_report_round_trip() { let config = diff --git a/rust/tests/prepared_session_test.rs b/rust/tests/prepared_session_test.rs index 1ae60cbbe1..aec924cb37 100644 --- a/rust/tests/prepared_session_test.rs +++ b/rust/tests/prepared_session_test.rs @@ -213,6 +213,11 @@ impl McpAuthHandler for CancelMcpAuthHandler { } } +struct NoopHooks; + +#[async_trait] +impl github_copilot_sdk::hooks::SessionHooks for NoopHooks {} + fn make_client() -> (Client, FakeServer) { let (client_write, server_read) = duplex(1 << 20); let (server_write, client_read) = duplex(1 << 20); @@ -1077,6 +1082,220 @@ async fn resume_session_wrapper_keeps_rpc_sequence() { drop(session); } +#[tokio::test] +async fn resume_without_automatic_skill_reload() { + check_resume_without_automatic_skill_reload(false).await; +} + +#[tokio::test] +async fn prepared_resume_without_automatic_skill_reload() { + check_resume_without_automatic_skill_reload(true).await; +} + +async fn check_resume_without_automatic_skill_reload(prepared: bool) { + let (client, mut server) = make_client(); + let session_id = SessionId::new("caller-owned-skills"); + let published = Arc::new(tokio::sync::Notify::new()); + let config = ResumeSessionConfig::new(session_id.clone()) + .with_reload_skills(false) + .with_enable_skills(true) + .with_permission_handler(Arc::new(PublicationFence(published.clone()))) + .with_hooks(Arc::new(NoopHooks)) + .with_mcp_auth_handler(Arc::new(CancelMcpAuthHandler)) + .with_coauthor_enabled(false); + let prepared = prepared.then(|| client.prepare_resume_session(config.clone()).unwrap()); + let mut events = prepared.as_ref().map(|prepared| prepared.subscribe()); + let mut start = Box::pin(async { + match prepared { + Some(prepared) => prepared.start().await, + None => client.resume_session(config).await, + } + }); + + let resume = tokio::select! { + result = &mut start => panic!("resume finished before its request: {:?}", result.map(|_| ())), + request = server.read_request() => request, + }; + assert_eq!(resume["method"], "session.resume"); + assert_eq!(resume["params"]["enableSkills"], true); + assert_eq!(resume["params"]["hooks"], true); + assert!(resume["params"].get("reloadSkills").is_none()); + assert!(start.as_mut().now_or_never().is_none()); + server + .send_event(session_id.as_str(), "before-resume", "session.idle", true) + .await; + server + .await_publication(session_id.as_str(), &published) + .await; + server + .respond(&resume, json!({ "sessionId": session_id.as_str() })) + .await; + + let interest = tokio::select! { + result = &mut start => panic!("resume skipped MCP-auth readiness: {:?}", result.map(|_| ())), + request = server.read_request() => request, + }; + assert_eq!(interest["method"], "session.eventLog.registerInterest"); + assert!(start.as_mut().now_or_never().is_none()); + let hook = json!({ + "jsonrpc": "2.0", "id": 9020, "method": "hooks.invoke", + "params": { + "sessionId": session_id.as_str(), "hookType": "sessionEnd", + "input": { + "sessionId": session_id.as_str(), "timestamp": 1234567890, + "cwd": "/tmp", "reason": "complete" + } + } + }); + write_framed(&mut server.write, &serde_json::to_vec(&hook).unwrap()).await; + let response = server.read_request().await; + assert_eq!(response["id"], 9020); + assert_eq!(response["result"]["output"], json!({})); + server + .send_event(session_id.as_str(), "during-interest", "session.idle", true) + .await; + server + .await_publication(session_id.as_str(), &published) + .await; + server.respond(&interest, json!({})).await; + + let options = tokio::select! { + result = &mut start => panic!("resume skipped options readiness: {:?}", result.map(|_| ())), + request = server.read_request() => request, + }; + assert_eq!( + options["method"], "session.options.update", + "opting out must not issue session.skills.reload" + ); + assert!(start.as_mut().now_or_never().is_none()); + server + .send_event(session_id.as_str(), "during-options", "session.idle", true) + .await; + server + .await_publication(session_id.as_str(), &published) + .await; + server.respond(&options, json!({ "success": true })).await; + let session = timeout(TIMEOUT, start).await.unwrap().unwrap(); + let mut events = events.take().unwrap_or_else(|| session.subscribe()); + for id in [ + "before-resume", + "publication-fence", + "during-interest", + "publication-fence", + "during-options", + "publication-fence", + ] { + expect_event_id(&mut events, id).await; + } + + let skills = session.rpc().skills(); + let mut explicit_reload = Box::pin(skills.reload()); + let reload = tokio::select! { + result = &mut explicit_reload => panic!("explicit reload finished before its request: {result:?}"), + request = server.read_request() => request, + }; + assert_eq!(reload["method"], "session.skills.reload"); + assert!(explicit_reload.as_mut().now_or_never().is_none()); + server + .respond(&reload, json!({ "errors": [], "warnings": [] })) + .await; + let diagnostics = timeout(TIMEOUT, explicit_reload).await.unwrap().unwrap(); + assert!(diagnostics.errors.is_empty()); + assert!(diagnostics.warnings.is_empty()); + server.expect_quiet().await; + session.stop_event_loop().await; + drop(session); + expect_closed(&mut events).await; + await_no_registrations(&client).await; +} + +#[tokio::test] +async fn automatic_skill_reload_waits_for_settlement_and_tolerates_returned_errors() { + for reload_skills in [None, Some(true)] { + for reload_error in [false, true] { + let (client, mut server) = make_client(); + let session_id = SessionId::new("automatic-skills"); + let mut config = ResumeSessionConfig::new(session_id.clone()); + config.reload_skills = reload_skills; + let mut start = Box::pin(client.resume_session(config)); + let resume = tokio::select! { + result = &mut start => panic!("resume finished before its request: {:?}", result.map(|_| ())), + request = server.read_request() => request, + }; + assert_eq!(resume["method"], "session.resume"); + server + .respond(&resume, json!({ "sessionId": session_id.as_str() })) + .await; + let reload = tokio::select! { + result = &mut start => panic!("resume did not await its reload: {:?}", result.map(|_| ())), + request = server.read_request() => request, + }; + assert_eq!(reload["method"], "session.skills.reload"); + assert!(start.as_mut().now_or_never().is_none()); + if reload_error { + server + .respond_error(&reload, -32003, "skills reload failed") + .await; + } else { + server.respond(&reload, json!({})).await; + } + let session = timeout(TIMEOUT, start).await.unwrap().unwrap(); + assert_eq!(session.id(), &session_id); + server.expect_quiet().await; + session.stop_event_loop().await; + drop(session); + await_no_registrations(&client).await; + } + } +} + +#[tokio::test] +async fn resume_without_skill_reload_cleans_up_on_owner_connection_loss() { + for phase in [ + "session.resume", + "session.eventLog.registerInterest", + "session.options.update", + ] { + let (client, mut server) = make_client(); + let session_id = SessionId::new("skills-owner-loss"); + let prepared = client + .prepare_resume_session( + ResumeSessionConfig::new(session_id.clone()) + .with_reload_skills(false) + .with_mcp_auth_handler(Arc::new(CancelMcpAuthHandler)) + .with_coauthor_enabled(false), + ) + .unwrap(); + let mut events = prepared.subscribe(); + let start = tokio::spawn(prepared.start()); + for expected in [ + "session.resume", + "session.eventLog.registerInterest", + "session.options.update", + ] { + let request = server.read_request().await; + assert_eq!(request["method"], expected); + if expected == phase { + break; + } + server + .respond( + &request, + json!({ "sessionId": session_id.as_str(), "success": true }), + ) + .await; + } + client.force_stop(); + let error = expect_error(timeout(TIMEOUT, start).await.unwrap().unwrap()); + assert_eq!( + error.kind(), + &ErrorKind::Protocol(github_copilot_sdk::ProtocolErrorKind::RequestCancelled), + ); + expect_closed(&mut events).await; + await_no_registrations(&client).await; + } +} + #[tokio::test] async fn resume_bootstrap_retains_all_startup_phases_and_keeps_later_subscribers_live() { let (client, mut server) = make_client(); diff --git a/rust/tests/session_test.rs b/rust/tests/session_test.rs index 40c455549c..53c5d03221 100644 --- a/rust/tests/session_test.rs +++ b/rust/tests/session_test.rs @@ -8636,6 +8636,7 @@ enum StartupRoute { Local, Deferred, Resume, + ResumeWithoutSkillReload, } #[derive(Clone, Copy, Debug)] @@ -8662,17 +8663,22 @@ async fn check_startup_callback_ownership( write: server_write, session_id: "startup-callbacks".into(), }; - let prepared = if route == StartupRoute::Resume { - client - .prepare_resume_session( - github_copilot_sdk::ResumeSessionConfig::new(SessionId::new(&server.session_id)) - .with_permission_handler(permission.clone()) - .with_session_fs_provider(fs.clone()) - .with_elicitation_handler(elicitation.clone()) - .with_mcp_auth_handler(mcp_auth.clone()) - .with_coauthor_enabled(false), - ) - .unwrap() + let resume = matches!( + route, + StartupRoute::Resume | StartupRoute::ResumeWithoutSkillReload + ); + let prepared = if resume { + let mut config = + github_copilot_sdk::ResumeSessionConfig::new(SessionId::new(&server.session_id)) + .with_permission_handler(permission.clone()) + .with_session_fs_provider(fs.clone()) + .with_elicitation_handler(elicitation.clone()) + .with_mcp_auth_handler(mcp_auth.clone()) + .with_coauthor_enabled(false); + if route == StartupRoute::ResumeWithoutSkillReload { + config = config.with_reload_skills(false); + } + client.prepare_resume_session(config).unwrap() } else { let mut config = SessionConfig::default() .with_permission_handler(permission.clone()) @@ -8694,7 +8700,7 @@ async fn check_startup_callback_ownership( let mut request = timeout(TIMEOUT, server.read_request()).await.unwrap(); assert_eq!( request["method"], - if route == StartupRoute::Resume { + if resume { "session.resume" } else { "session.create" @@ -8875,20 +8881,26 @@ async fn check_startup_callback_ownership( #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn startup_callbacks_owned_until_resume_response() { - for outcome in [StartupOutcome::Failure, StartupOutcome::Cancellation] { - Box::pin(check_startup_callback_ownership( - StartupRoute::Resume, - "session.resume", - outcome, - true, - )) - .await; + for route in [StartupRoute::Resume, StartupRoute::ResumeWithoutSkillReload] { + for outcome in [StartupOutcome::Failure, StartupOutcome::Cancellation] { + Box::pin(check_startup_callback_ownership( + route, + "session.resume", + outcome, + true, + )) + .await; + } } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn startup_callbacks_owned_through_mcp_interest() { - for route in [StartupRoute::Local, StartupRoute::Resume] { + for route in [ + StartupRoute::Local, + StartupRoute::Resume, + StartupRoute::ResumeWithoutSkillReload, + ] { for outcome in [StartupOutcome::Failure, StartupOutcome::Cancellation] { for before_response in [false, true] { Box::pin(check_startup_callback_ownership( @@ -8922,6 +8934,7 @@ async fn startup_callbacks_owned_through_mode_patch() { StartupRoute::Local, StartupRoute::Deferred, StartupRoute::Resume, + StartupRoute::ResumeWithoutSkillReload, ] { for outcome in [StartupOutcome::Failure, StartupOutcome::Cancellation] { Box::pin(check_startup_callback_ownership( @@ -8954,6 +8967,7 @@ async fn startup_callbacks_survive_successful_setup() { StartupRoute::Local, StartupRoute::Deferred, StartupRoute::Resume, + StartupRoute::ResumeWithoutSkillReload, ] { Box::pin(check_startup_callback_ownership( route, diff --git a/rust/tests/skill_provider_test.rs b/rust/tests/skill_provider_test.rs index 71494ee65c..c85cc68d71 100644 --- a/rust/tests/skill_provider_test.rs +++ b/rust/tests/skill_provider_test.rs @@ -520,6 +520,55 @@ async fn serves_early_create_and_resume_callbacks() { let _ = timeout(TIMEOUT, resume).await.unwrap().unwrap(); } +#[tokio::test] +async fn serves_skill_provider_callbacks_without_automatic_reload() { + let (client, mut server) = make_client(); + let provider = Arc::new(RecordingProvider::new()); + let calls = provider.calls.clone(); + let resume = tokio::spawn({ + let client = client.clone(); + async move { + client + .resume_session( + ResumeSessionConfig::new(SessionId::new("caller-owned-provider")) + .with_reload_skills(false) + .with_skill_provider(provider), + ) + .await + .unwrap() + } + }); + let request = server.read_request().await; + assert_eq!(request["method"], "session.resume"); + assert_eq!(request["params"]["hasSkillProvider"], true); + server + .send_request( + 12, + "skillProvider.list", + json!({ "sessionId": "caller-owned-provider" }), + ) + .await; + let list = server.read_response().await; + assert_eq!(list["id"], 12); + assert_eq!(list["result"]["skills"][0]["name"], "review"); + server + .respond(&request, json!({ "sessionId": "caller-owned-provider" })) + .await; + let session = timeout(TIMEOUT, resume).await.unwrap().unwrap(); + server + .send_request( + 13, + "skillProvider.read", + json!({ "sessionId": "caller-owned-provider", "name": "review" }), + ) + .await; + let read = server.read_response().await; + assert_eq!(read["id"], 13); + assert_eq!(read["result"]["markdown"], "Review carefully."); + assert_eq!(calls.snapshot(), ["list", "read:review"]); + session.stop_event_loop().await; +} + #[tokio::test] async fn cloud_sessions_reject_provider_before_rpc() { let (client, mut server) = make_client();