Skip to content

Commit 5f48087

Browse files
authored
fix: compare extension configs before skipping add_extension (#7650)
1 parent 3c28fbe commit 5f48087

1 file changed

Lines changed: 94 additions & 2 deletions

File tree

crates/goose/src/agents/extension_manager.rs

Lines changed: 94 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -538,8 +538,14 @@ impl ExtensionManager {
538538
) -> ExtensionResult<()> {
539539
let sanitized_name = config.key();
540540

541-
if self.extensions.lock().await.contains_key(&sanitized_name) {
542-
return Ok(());
541+
if let Some(existing) = self.extensions.lock().await.get(&sanitized_name) {
542+
if existing.config == config {
543+
return Ok(());
544+
}
545+
tracing::debug!(
546+
name = sanitized_name,
547+
"extension config changed, restarting with updated config"
548+
);
543549
}
544550

545551
let mut temp_dir = None;
@@ -2235,4 +2241,90 @@ mod tests {
22352241
assert!(tool_names.iter().any(|n| n.starts_with("ext_a__")));
22362242
assert!(!tool_names.iter().any(|n| n.starts_with("ext_b__")));
22372243
}
2244+
2245+
#[tokio::test]
2246+
async fn test_add_extension_noop_on_identical_config() {
2247+
// When add_extension is called with a config that is byte-for-byte identical to
2248+
// the already-loaded one, it must return Ok(()) without removing the extension.
2249+
let temp_dir = tempfile::tempdir().unwrap();
2250+
let em = Arc::new(ExtensionManager::new_without_provider(
2251+
temp_dir.path().to_path_buf(),
2252+
));
2253+
2254+
let config = ExtensionConfig::Frontend {
2255+
name: "test-ext".to_string(),
2256+
description: "original".to_string(),
2257+
tools: vec![],
2258+
instructions: None,
2259+
bundled: None,
2260+
available_tools: vec![],
2261+
};
2262+
2263+
em.add_client(
2264+
"test-ext".to_string(),
2265+
config.clone(),
2266+
Arc::new(MockClient {}),
2267+
None,
2268+
None,
2269+
)
2270+
.await;
2271+
assert_eq!(em.extensions.lock().await.len(), 1);
2272+
2273+
// Calling add_extension with the same config must be a no-op (Ok, count unchanged).
2274+
let result = em.add_extension(config, None, None, None).await;
2275+
assert!(result.is_ok(), "identical config should be a no-op");
2276+
assert_eq!(
2277+
em.extensions.lock().await.len(),
2278+
1,
2279+
"extension must not be removed on no-op"
2280+
);
2281+
}
2282+
2283+
#[tokio::test]
2284+
async fn test_add_extension_replaces_extension_on_config_change() {
2285+
// When add_extension is called with an updated config (same name, different fields),
2286+
// the existing extension must be removed so the caller can re-add with new config.
2287+
let temp_dir = tempfile::tempdir().unwrap();
2288+
let em = Arc::new(ExtensionManager::new_without_provider(
2289+
temp_dir.path().to_path_buf(),
2290+
));
2291+
2292+
let config_a = ExtensionConfig::Frontend {
2293+
name: "test-ext".to_string(),
2294+
description: "version-a".to_string(),
2295+
tools: vec![],
2296+
instructions: None,
2297+
bundled: None,
2298+
available_tools: vec![],
2299+
};
2300+
let config_b = ExtensionConfig::Frontend {
2301+
name: "test-ext".to_string(),
2302+
description: "version-b".to_string(), // changed
2303+
tools: vec![],
2304+
instructions: None,
2305+
bundled: None,
2306+
available_tools: vec![],
2307+
};
2308+
2309+
em.add_client(
2310+
"test-ext".to_string(),
2311+
config_a,
2312+
Arc::new(MockClient {}),
2313+
None,
2314+
None,
2315+
)
2316+
.await;
2317+
assert_eq!(em.extensions.lock().await.len(), 1);
2318+
2319+
// add_extension with changed config attempts to create a new client (fails here
2320+
// because Frontend configs cannot be added as server extensions), but must preserve
2321+
// the old extension so the session isn't left without it.
2322+
let result = em.add_extension(config_b, None, None, None).await;
2323+
assert!(result.is_err(), "Frontend add_extension must return Err");
2324+
assert_eq!(
2325+
em.extensions.lock().await.len(),
2326+
1,
2327+
"old extension must be preserved when replacement client creation fails"
2328+
);
2329+
}
22382330
}

0 commit comments

Comments
 (0)