Skip to content
Merged
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
111 changes: 110 additions & 1 deletion crates/goose/src/agents/extension_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,15 +350,56 @@ impl ExtensionManager {
timeout,
headers,
name,
envs,
env_keys,
..
} => {
// Merge environment variables from direct envs and keychain-stored env_keys
let all_envs = merge_environments(envs, env_keys, &sanitized_name).await?;

// Helper function to substitute environment variables in a string
// Supports both ${VAR} and $VAR syntax
fn substitute_env_vars(value: &str, env_map: &HashMap<String, String>) -> String {
let mut result = value.to_string();

// First handle ${VAR} syntax (with optional whitespace)
let re_braces = regex::Regex::new(r"\$\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}")
.expect("valid regex");
Comment on lines +366 to +367
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiling regex patterns on every function call is inefficient. The regex patterns are constant and should be compiled once using lazy_static! or OnceLock (already used elsewhere in this codebase) and stored as static variables to avoid recompilation overhead.

Copilot uses AI. Check for mistakes.
for cap in re_braces.captures_iter(value) {
if let Some(var_name) = cap.get(1) {
if let Some(env_value) = env_map.get(var_name.as_str()) {
result = result.replace(&cap[0], env_value);
}
}
}

// Then handle $VAR syntax (simple variable without braces)
let re_simple =
regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex");
Comment on lines +377 to +378
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiling regex patterns on every function call is inefficient. The regex patterns are constant and should be compiled once using lazy_static! or OnceLock (already used elsewhere in this codebase) and stored as static variables to avoid recompilation overhead.

Copilot uses AI. Check for mistakes.
for cap in re_simple.captures_iter(&result.clone()) {
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary clone of result string. Since captures_iter only needs a reference and doesn't consume the string, you can pass &result directly without cloning.

Suggested change
for cap in re_simple.captures_iter(&result.clone()) {
for cap in re_simple.captures_iter(&result) {

Copilot uses AI. Check for mistakes.
if let Some(var_name) = cap.get(1) {
// Only substitute if it wasn't already part of ${VAR} syntax
if !value.contains(&format!("${{{}}}", var_name.as_str())) {
if let Some(env_value) = env_map.get(var_name.as_str()) {
result = result.replace(&cap[0], env_value);
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using replace will substitute all occurrences of the matched pattern in the entire string, not just the current match. If the same variable appears multiple times (e.g., $TOKEN and $TOKEN), the second iteration will fail to find the pattern since it was already replaced. Use replacen with a count of 1 or track positions to replace only the specific occurrence.

Suggested change
result = result.replace(&cap[0], env_value);
result = result.replacen(&cap[0], env_value, 1);

Copilot uses AI. Check for mistakes.
}
}
}
}

result
Comment on lines +363 to +390
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using replace will substitute all occurrences of the matched pattern in the entire string, not just the current match. If the same variable appears multiple times (e.g., ${TOKEN} and ${TOKEN}), the second iteration will fail to find the pattern since it was already replaced. Use replacen with a count of 1 or track positions to replace only the specific occurrence.

Suggested change
let mut result = value.to_string();
// First handle ${VAR} syntax (with optional whitespace)
let re_braces = regex::Regex::new(r"\$\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}")
.expect("valid regex");
for cap in re_braces.captures_iter(value) {
if let Some(var_name) = cap.get(1) {
if let Some(env_value) = env_map.get(var_name.as_str()) {
result = result.replace(&cap[0], env_value);
}
}
}
// Then handle $VAR syntax (simple variable without braces)
let re_simple =
regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex");
for cap in re_simple.captures_iter(&result.clone()) {
if let Some(var_name) = cap.get(1) {
// Only substitute if it wasn't already part of ${VAR} syntax
if !value.contains(&format!("${{{}}}", var_name.as_str())) {
if let Some(env_value) = env_map.get(var_name.as_str()) {
result = result.replace(&cap[0], env_value);
}
}
}
}
result
// First handle ${VAR} syntax (with optional whitespace)
let re_braces = regex::Regex::new(r"\$\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}")
.expect("valid regex");
let result = re_braces.replace_all(value, |caps: &regex::Captures| {
let var_name = &caps[1];
env_map.get(var_name).map(|v| v.as_str()).unwrap_or(caps.get(0).unwrap().as_str())
});
// Then handle $VAR syntax (simple variable without braces)
let re_simple = regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex");
// Only substitute $VAR if it wasn't already part of ${VAR} syntax in the original value
let result = re_simple.replace_all(&result, |caps: &regex::Captures| {
let var_name = &caps[1];
// Only substitute if not present as ${VAR} in the original value
if !value.contains(&format!("${{{}}}", var_name)) {
env_map.get(var_name).map(|v| v.as_str()).unwrap_or(caps.get(0).unwrap().as_str())
} else {
caps.get(0).unwrap().as_str()
}
});
result.into_owned()

Copilot uses AI. Check for mistakes.
}

let mut default_headers = HeaderMap::new();
for (key, value) in headers {
// Substitute environment variables in header values
let substituted_value = substitute_env_vars(value, &all_envs);

default_headers.insert(
HeaderName::try_from(key).map_err(|_| {
ExtensionError::ConfigError(format!("invalid header: {}", key))
})?,
value.parse().map_err(|_| {
substituted_value.parse().map_err(|_| {
ExtensionError::ConfigError(format!("invalid header value: {}", key))
})?,
);
Expand Down Expand Up @@ -1518,4 +1559,72 @@ mod tests {

assert!(result.is_ok());
}

#[tokio::test]
async fn test_streamable_http_header_env_substitution() {
use std::collections::HashMap;

// Test the substitute_env_vars helper function (which is defined inside add_extension)
// We'll recreate it here for testing purposes
fn substitute_env_vars(value: &str, env_map: &HashMap<String, String>) -> String {
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The substitute_env_vars function is duplicated between the production code (line 362) and the test code (line 1569). Consider extracting this function to module-level scope or creating a shared helper module to eliminate code duplication and ensure consistent behavior.

Copilot uses AI. Check for mistakes.
let mut result = value.to_string();

// First handle ${VAR} syntax (with optional whitespace)
let re_braces =
regex::Regex::new(r"\$\{\s*([A-Za-z_][A-Za-z0-9_]*)\s*\}").expect("valid regex");
Comment on lines +1573 to +1574
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiling regex patterns on every test invocation is inefficient. The regex patterns are constant and should be compiled once using lazy_static! or OnceLock and stored as static variables to avoid recompilation overhead.

Copilot uses AI. Check for mistakes.
for cap in re_braces.captures_iter(value) {
if let Some(var_name) = cap.get(1) {
if let Some(env_value) = env_map.get(var_name.as_str()) {
result = result.replace(&cap[0], env_value);
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using replace will substitute all occurrences of the matched pattern in the entire string, not just the current match. If the same variable appears multiple times (e.g., ${TOKEN} and ${TOKEN}), the second iteration will fail to find the pattern since it was already replaced. Use replacen with a count of 1 or track positions to replace only the specific occurrence.

Suggested change
result = result.replace(&cap[0], env_value);
result = result.replacen(&cap[0], env_value, 1);

Copilot uses AI. Check for mistakes.
}
}
}

// Then handle $VAR syntax (simple variable without braces)
let re_simple = regex::Regex::new(r"\$([A-Za-z_][A-Za-z0-9_]*)").expect("valid regex");
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiling regex patterns on every test invocation is inefficient. The regex patterns are constant and should be compiled once using lazy_static! or OnceLock and stored as static variables to avoid recompilation overhead.

Copilot uses AI. Check for mistakes.
for cap in re_simple.captures_iter(&result.clone()) {
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary clone of result string. Since captures_iter only needs a reference and doesn't consume the string, you can pass &result directly without cloning.

Suggested change
for cap in re_simple.captures_iter(&result.clone()) {
for cap in re_simple.captures_iter(&result) {

Copilot uses AI. Check for mistakes.
if let Some(var_name) = cap.get(1) {
// Only substitute if it wasn't already part of ${VAR} syntax
if !value.contains(&format!("${{{}}}", var_name.as_str())) {
if let Some(env_value) = env_map.get(var_name.as_str()) {
result = result.replace(&cap[0], env_value);
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using replace will substitute all occurrences of the matched pattern in the entire string, not just the current match. If the same variable appears multiple times (e.g., $TOKEN and $TOKEN), the second iteration will fail to find the pattern since it was already replaced. Use replacen with a count of 1 or track positions to replace only the specific occurrence.

Copilot uses AI. Check for mistakes.
}
}
}
}

result
}

let mut env_map = HashMap::new();
env_map.insert("AUTH_TOKEN".to_string(), "secret123".to_string());
env_map.insert("API_KEY".to_string(), "key456".to_string());

// Test ${VAR} syntax
let result = substitute_env_vars("Bearer ${ AUTH_TOKEN }", &env_map);
assert_eq!(result, "Bearer secret123");

// Test ${VAR} syntax without spaces
let result = substitute_env_vars("Bearer ${AUTH_TOKEN}", &env_map);
assert_eq!(result, "Bearer secret123");

// Test $VAR syntax
let result = substitute_env_vars("Bearer $AUTH_TOKEN", &env_map);
assert_eq!(result, "Bearer secret123");

// Test multiple substitutions
let result = substitute_env_vars("Key: $API_KEY, Token: ${AUTH_TOKEN}", &env_map);
assert_eq!(result, "Key: key456, Token: secret123");

// Test no substitution when variable doesn't exist
let result = substitute_env_vars("Bearer ${UNKNOWN_VAR}", &env_map);
assert_eq!(result, "Bearer ${UNKNOWN_VAR}");

// Test mixed content
let result = substitute_env_vars(
"Authorization: Bearer ${AUTH_TOKEN} and API ${API_KEY}",
&env_map,
);
assert_eq!(result, "Authorization: Bearer secret123 and API key456");
}
}