fix: post-merge review sweep — 8 fixes across security, perf, and correctness

src/agent/dispatcher.rs

@@ -1069,15 +1069,23 @@ pub(crate) fn extract_suggestions(text: &str) -> (String, Vec<String>) {
         Regex::new(r"(?s)<suggestions>\s*(.*?)\s*</suggestions>").expect("valid regex") // safety: constant pattern
     });
 
-    // Find the position of the last closing code fence to avoid matching inside code blocks
-    let last_code_fence = text.rfind("```").unwrap_or(0);
+    // Build a sorted list of code fence positions to determine open/close pairing.
+    // A position is "inside" a fenced block when it falls between an odd-numbered
+    // fence (opening) and the next even-numbered fence (closing).
+    let fence_positions: Vec<usize> = text.match_indices("```").map(|(pos, _)| pos).collect();
+
+    let is_inside_fence = |pos: usize| -> bool {
+        // Count how many fences appear before `pos`. If odd, we're inside a fence.
+        let count = fence_positions.iter().take_while(|&&fp| fp <= pos).count();
+        count % 2 == 1
+    };
 
-    // Find all matches, take the last one that's after the last code fence
+    // Find all matches, take the last one that's outside any code fence
     let mut best_match: Option<regex::Match<'_>> = None;
     let mut best_capture: Option<String> = None;
     for caps in RE.captures_iter(text) {
         if let (Some(full), Some(inner)) = (caps.get(0), caps.get(1))
-            && full.start() >= last_code_fence
+            && !is_inside_fence(full.start())
         {
             best_match = Some(full);
             best_capture = Some(inner.as_str().to_string());
@@ -2321,6 +2329,16 @@ mod tests {
         assert!(suggestions.is_empty()); // safety: test
     }
 
+    #[test]
+    fn test_extract_suggestions_inside_unclosed_code_fence() {
+        // Regression: odd number of fences (unclosed fence) must still be
+        // treated as "inside a code block".
+        let input = "```\ncode\n<suggestions>[\"bar\"]</suggestions>";
+        let (text, suggestions) = super::extract_suggestions(input);
+        assert_eq!(text, input); // safety: test
+        assert!(suggestions.is_empty()); // safety: test
+    }
+
     #[test]
     fn test_extract_suggestions_after_code_fence() {
         let input = "```\ncode\n```\nAnswer.\n<suggestions>[\"foo\"]</suggestions>";

src/agent/routine_engine.rs

@@ -1305,6 +1305,19 @@ async fn execute_lightweight(
     }
 }
 
+/// Sanitize a user-controlled string before interpolation into an LLM prompt.
+/// Strips newlines (which could break prompt structure) and truncates to a
+/// reasonable length to limit abuse surface.
+fn sanitize_prompt_field(value: &str) -> String {
+    const MAX_LEN: usize = 128;
+    value
+        .chars()
+        .filter(|&c| c != '\n' && c != '\r')
+        .take(MAX_LEN)
+        .map(|c| if c == '`' { '\'' } else { c })
+        .collect()
+}
+
 fn build_lightweight_prompt(
     prompt: &str,
     context_parts: &[String],
@@ -1323,14 +1336,16 @@ fn build_lightweight_prompt(
         );
 
         if let Some(channel) = notify.channel.as_deref() {
+            let sanitized = sanitize_prompt_field(channel);
             full_prompt.push_str(&format!(
-                "The configured delivery channel for this routine is `{channel}`.\n"
+                "The configured delivery channel for this routine is `{sanitized}`.\n"
             ));
         }
 
         if let Some(user) = notify.user.as_deref() {
+            let sanitized = sanitize_prompt_field(user);
             full_prompt.push_str(&format!(
-                "The configured delivery target for this routine is `{user}`.\n"
+                "The configured delivery target for this routine is `{sanitized}`.\n"
             ));
         }
 

src/channels/wasm/router.rs

@@ -333,6 +333,9 @@ async fn webhook_handler(
 
     let channel_name = channel.channel_name();
 
+    // Track whether any authentication was performed and passed.
+    let mut did_authenticate = false;
+
     // Check if secret is required
     if state.router.requires_secret(channel_name).await {
         // Get the secret header name for this channel (from capabilities or default)
@@ -382,6 +385,7 @@ async fn webhook_handler(
                     );
                 }
                 tracing::debug!(channel = %channel_name, "Webhook secret validated");
+                did_authenticate = true;
             }
             None => {
                 tracing::warn!(
@@ -433,6 +437,7 @@ async fn webhook_handler(
                     );
                 }
                 tracing::debug!(channel = %channel_name, "Ed25519 signature verified");
+                did_authenticate = true;
             }
             _ => {
                 tracing::warn!(
@@ -484,6 +489,7 @@ async fn webhook_handler(
                     );
                 }
                 tracing::debug!(channel = %channel_name, "HMAC-SHA256 signature verified");
+                did_authenticate = true;
             }
             _ => {
                 tracing::warn!(
@@ -510,8 +516,9 @@ async fn webhook_handler(
         })
         .collect();
 
-    // Call the WASM channel
-    let secret_validated = state.router.requires_secret(channel_name).await;
+    // Call the WASM channel. `did_authenticate` was set above by whichever
+    // auth guard (secret / Ed25519 / HMAC) successfully validated the request.
+    let secret_validated = did_authenticate;
 
     tracing::info!(
         channel = %channel_name,

src/config/llm.rs

@@ -383,7 +383,7 @@ impl LlmConfig {
         // Resolve extra headers
         let extra_headers = if let Some(env_var) = extra_headers_env {
             optional_env(env_var)?
-                .map(|val| parse_extra_headers(&val))
+                .map(|val| parse_extra_headers_with_key(&val, env_var))
                 .transpose()?
                 .unwrap_or_default()
         } else {
@@ -452,7 +452,10 @@ impl LlmConfig {
 ///
 /// Format: `Key1:Value1,Key2:Value2` (colon-separated, not `=`, because
 /// header values often contain `=`).
-fn parse_extra_headers(val: &str) -> Result<Vec<(String, String)>, ConfigError> {
+fn parse_extra_headers_with_key(
+    val: &str,
+    env_var_name: &str,
+) -> Result<Vec<(String, String)>, ConfigError> {
     if val.trim().is_empty() {
         return Ok(Vec::new());
     }
@@ -465,14 +468,14 @@ fn parse_extra_headers(val: &str) -> Result<Vec<(String, String)>, ConfigError>
         }
         let Some((key, value)) = pair.split_once(':') else {
             return Err(ConfigError::InvalidValue {
-                key: "LLM_EXTRA_HEADERS".to_string(),
+                key: env_var_name.to_string(),
                 message: format!("malformed header entry '{}', expected Key:Value", pair),
             });
         };
         let key = key.trim();
         if key.is_empty() {
             return Err(ConfigError::InvalidValue {
-                key: "LLM_EXTRA_HEADERS".to_string(),
+                key: env_var_name.to_string(),
                 message: format!("empty header name in entry '{}'", pair),
             });
         }
@@ -513,6 +516,11 @@ mod tests {
     use crate::settings::Settings;
     use crate::testing::credentials::*;
 
+    /// Convenience wrapper for tests — uses "TEST_HEADERS" as the env var name.
+    fn parse_extra_headers(val: &str) -> Result<Vec<(String, String)>, ConfigError> {
+        parse_extra_headers_with_key(val, "TEST_HEADERS")
+    }
+
     /// Clear all openai-compatible-related env vars.
     fn clear_openai_compatible_env() {
         // SAFETY: Only called under ENV_MUTEX in tests.

src/llm/github_copilot.rs

@@ -107,14 +107,21 @@ impl GithubCopilotProvider {
         body: &impl Serialize,
     ) -> Result<R, LlmError> {
         let url = self.api_url();
-        // Map token exchange failures to RequestFailed (retryable) rather than
-        // AuthFailed (non-retryable), since transient network errors during
-        // exchange should be retried by RetryProvider.
+        // Distinguish permanent auth errors (non-retryable) from transient
+        // network failures (retryable) so RetryProvider handles them correctly.
         let token = self.token_manager.get_token().await.map_err(|e| {
             tracing::warn!(error = %e, "Copilot: token exchange failed");
-            LlmError::RequestFailed {
-                provider: "github_copilot".to_string(),
-                reason: format!("Token exchange failed: {e}"),
+            match &e {
+                crate::llm::github_copilot_auth::GithubCopilotAuthError::AccessDenied
+                | crate::llm::github_copilot_auth::GithubCopilotAuthError::Expired => {
+                    LlmError::AuthFailed {
+                        provider: "github_copilot".to_string(),
+                    }
+                }
+                _ => LlmError::RequestFailed {
+                    provider: "github_copilot".to_string(),
+                    reason: format!("Token exchange failed: {e}"),
+                },
             }
         })?;
 
@@ -157,54 +164,14 @@ impl GithubCopilotProvider {
             );
 
             if status.as_u16() == 401 {
-                // Invalidate the cached session token and retry once with a
-                // fresh exchange — stale tokens are the most common 401 cause.
-                tracing::warn!("Copilot: 401 Unauthorized — invalidating session token, retrying");
+                // Invalidate the cached session token so the next attempt
+                // (driven by RetryProvider) gets a fresh one. We don't retry
+                // inline to avoid nested retries with the outer RetryProvider.
+                tracing::warn!("Copilot: 401 Unauthorized — invalidating session token for retry");
                 self.token_manager.invalidate().await;
-                let fresh = self.token_manager.get_token().await.map_err(|e| {
-                    tracing::warn!(error = %e, "Copilot: re-exchange after 401 failed");
-                    LlmError::RequestFailed {
-                        provider: "github_copilot".to_string(),
-                        reason: format!("Token re-exchange after 401 failed: {e}"),
-                    }
-                })?;
-                let mut retry_req = self
-                    .client
-                    .post(&url)
-                    .bearer_auth(fresh.expose_secret())
-                    .header("Content-Type", "application/json");
-                for (key, value) in &self.extra_headers {
-                    retry_req = retry_req.header(key.as_str(), value.as_str());
-                }
-                let retry =
-                    retry_req
-                        .json(body)
-                        .send()
-                        .await
-                        .map_err(|e| LlmError::RequestFailed {
-                            provider: "github_copilot".to_string(),
-                            reason: format!("Retry after 401 failed: {e}"),
-                        })?;
-                if retry.status().is_success() {
-                    let text = retry.text().await.map_err(|e| LlmError::RequestFailed {
-                        provider: "github_copilot".to_string(),
-                        reason: format!("Failed to read retry response body: {e}"),
-                    })?;
-                    return serde_json::from_str(&text).map_err(|e| {
-                        let truncated = crate::agent::truncate_for_preview(&text, 512);
-                        LlmError::InvalidResponse {
-                            provider: "github_copilot".to_string(),
-                            reason: format!("JSON parse error: {e}. Raw: {truncated}"),
-                        }
-                    });
-                }
-                let retry_status = retry.status();
-                tracing::warn!(
-                    status = %retry_status,
-                    "Copilot: 401 retry also failed"
-                );
-                return Err(LlmError::AuthFailed {
+                return Err(LlmError::RequestFailed {
                     provider: "github_copilot".to_string(),
+                    reason: "HTTP 401 Unauthorized".to_string(),
                 });
             }
             if status.as_u16() == 429 {

src/tools/builtin/routine.rs

@@ -605,7 +605,8 @@ fn routine_create_schema(include_compatibility_aliases: bool) -> Value {
 }
 
 pub(crate) fn routine_create_parameters_schema() -> Value {
-    routine_create_schema(false)
+    static CACHE: OnceLock<Value> = OnceLock::new();
+    CACHE.get_or_init(|| routine_create_schema(false)).clone()
 }
 
 fn routine_create_discovery_schema() -> Value {
@@ -1007,7 +1008,8 @@ fn event_emit_schema(include_source_alias: bool) -> Value {
 }
 
 pub(crate) fn event_emit_parameters_schema() -> Value {
-    event_emit_schema(false)
+    static CACHE: OnceLock<Value> = OnceLock::new();
+    CACHE.get_or_init(|| event_emit_schema(false)).clone()
 }
 
 fn event_emit_discovery_schema() -> Value {