review fixes

Author: jgarzik

fixtures/mcp_calc_server/src/main.rs

@@ -6,8 +6,6 @@ use std::io::{self, BufRead, Write};
 
 #[derive(Deserialize)]
 struct JsonRpcRequest {
-    #[allow(dead_code)]
-    jsonrpc: String,
     id: Option<u64>,
     method: String,
     params: Option<Value>,

src/agent.rs

@@ -262,6 +262,13 @@ pub fn run_turn(ctx: &Context, user_input: &str, messages: &mut Vec<Value>) -> R
         let choice = &response.choices[0];
         let msg = &choice.message;
 
+        // Warn if response was truncated due to length limit
+        if choice.finish_reason.as_deref() == Some("length") {
+            eprintln!(
+                "⚠️  Response truncated (max tokens reached). Consider increasing max_tokens or using /compact."
+            );
+        }
+
         if let Some(content) = &msg.content {
             if !content.is_empty() {
                 println!("{}", content);

src/commands.rs

@@ -97,11 +97,16 @@ impl CommandIndex {
         }
     }
 
-    fn load_command(&self, path: &Path, name: &str, source: CommandSource) -> Result<Command> {
+    fn load_command(&mut self, path: &Path, name: &str, source: CommandSource) -> Result<Command> {
         let content = std::fs::read_to_string(path)?;
 
         // Parse optional YAML frontmatter
-        let (meta, content) = parse_frontmatter(&content);
+        let (meta, content, warning) = parse_frontmatter(&content);
+
+        // Record warning but still load the command
+        if let Some(warn) = warning {
+            self.errors.push((path.to_path_buf(), warn));
+        }
 
         Ok(Command {
             name: name.to_string(),
@@ -130,11 +135,12 @@ impl CommandIndex {
 }
 
 /// Parse optional YAML frontmatter from markdown content
-fn parse_frontmatter(content: &str) -> (CommandMeta, String) {
+/// Returns (metadata, body, optional_warning)
+fn parse_frontmatter(content: &str) -> (CommandMeta, String, Option<String>) {
     let trimmed = content.trim_start();
 
     if !trimmed.starts_with("---") {
-        return (CommandMeta::default(), content.to_string());
+        return (CommandMeta::default(), content.to_string(), None);
     }
 
     // Find the closing ---
@@ -143,11 +149,15 @@ fn parse_frontmatter(content: &str) -> (CommandMeta, String) {
         let rest = &trimmed[3 + end_pos + 4..].trim_start();
 
         match serde_yaml::from_str(yaml_content) {
-            Ok(meta) => (meta, rest.to_string()),
-            Err(_) => (CommandMeta::default(), content.to_string()),
+            Ok(meta) => (meta, rest.to_string(), None),
+            Err(e) => (
+                CommandMeta::default(),
+                content.to_string(),
+                Some(format!("invalid YAML frontmatter: {}", e)),
+            ),
         }
     } else {
-        (CommandMeta::default(), content.to_string())
+        (CommandMeta::default(), content.to_string(), None)
     }
 }
 
@@ -158,9 +168,10 @@ mod tests {
     #[test]
     fn test_parse_frontmatter_no_frontmatter() {
         let content = "Just some content";
-        let (meta, body) = parse_frontmatter(content);
+        let (meta, body, warning) = parse_frontmatter(content);
         assert!(meta.description.is_none());
         assert_eq!(body, "Just some content");
+        assert!(warning.is_none());
     }
 
     #[test]
@@ -173,13 +184,14 @@ allowed_tools:
 ---
 
 The actual command content"#;
-        let (meta, body) = parse_frontmatter(content);
+        let (meta, body, warning) = parse_frontmatter(content);
         assert_eq!(meta.description, Some("A test command".to_string()));
         assert_eq!(
             meta.allowed_tools,
             Some(vec!["Read".to_string(), "Grep".to_string()])
         );
         assert_eq!(body, "The actual command content");
+        assert!(warning.is_none());
     }
 
     #[test]

src/config.rs

@@ -620,13 +620,25 @@ impl Config {
             }
         }
 
-        // Validate MCP server configs
+        // Validate MCP server configs based on transport type
         for (name, server) in &self.mcp.servers {
-            if server.command.is_empty() {
-                errors.push(ValidationError {
-                    field: format!("mcp.servers.{}.command", name),
-                    message: "Command must not be empty".to_string(),
-                });
+            match server.transport {
+                McpTransport::Stdio => {
+                    if server.command.is_empty() {
+                        errors.push(ValidationError {
+                            field: format!("mcp.servers.{}.command", name),
+                            message: "Command required for stdio transport".to_string(),
+                        });
+                    }
+                }
+                McpTransport::Http | McpTransport::Sse => {
+                    if server.url.is_none() {
+                        errors.push(ValidationError {
+                            field: format!("mcp.servers.{}.url", name),
+                            message: "URL required for http/sse transport".to_string(),
+                        });
+                    }
+                }
             }
         }
 
@@ -716,6 +728,7 @@ mod tests {
         let errors = config.validate().unwrap_err();
         assert_eq!(errors.len(), 1);
         assert!(errors[0].field.contains("auto_compact_threshold"));
+        assert!(errors[0].message.contains("between 0.0 and 1.0"));
     }
 
     #[test]

src/cost.rs

@@ -261,7 +261,7 @@ impl PricingTable {
             ModelPricing::new(15.00, 75.00),
         );
 
-        // Venice / free tier models (approximate as free)
+        // Venice.ai models - free tier default, override via [pricing] config if needed
         models.insert(
             "qwen3-235b-a22b-instruct-2507".to_string(),
             ModelPricing::new(0.00, 0.00),

src/llm.rs

@@ -19,10 +19,6 @@ pub struct Usage {
     pub prompt_tokens: u64,
     #[serde(default)]
     pub completion_tokens: u64,
-    /// Total tokens from API (may be redundant with prompt_tokens + completion_tokens)
-    #[serde(default)]
-    #[allow(dead_code)]
-    pub total_tokens: u64,
 }
 
 #[derive(Debug, Deserialize)]
@@ -35,7 +31,6 @@ pub struct ChatResponse {
 #[derive(Debug, Deserialize)]
 pub struct Choice {
     pub message: Message,
-    #[allow(dead_code)]
     pub finish_reason: Option<String>,
 }
 

src/mcp/client.rs

@@ -20,10 +20,6 @@ struct JsonRpcRequest {
 /// JSON-RPC response structure
 #[derive(Deserialize)]
 struct JsonRpcResponse {
-    #[allow(dead_code)]
-    jsonrpc: String,
-    #[allow(dead_code)]
-    id: Option<u64>,
     result: Option<Value>,
     error: Option<JsonRpcError>,
 }
@@ -92,12 +88,14 @@ impl McpClient {
         let result = self.call("initialize", Some(params))?;
 
         // Send initialized notification (no response expected)
-        // For HTTP/SSE this is a fire-and-forget, result is ignored
+        // For HTTP/SSE this is a fire-and-forget, but log errors for debugging
         let notification = json!({
             "jsonrpc": "2.0",
             "method": "notifications/initialized"
         });
-        let _ = self.transport.send(&notification);
+        if let Err(e) = self.transport.send(&notification) {
+            eprintln!("MCP: Failed to send initialized notification: {}", e);
+        }
 
         Ok(result)
     }

src/mcp/transport.rs

@@ -225,9 +225,15 @@ impl SseTransport {
 
         match resp {
             Ok(r) => {
-                // For simple SSE implementations, the response comes back directly
-                let body: Value = r.into_json()?;
-                Ok(body)
+                // Check if response is SSE stream or direct JSON
+                let content_type = r.header("content-type").unwrap_or("").to_lowercase();
+                if content_type.contains("text/event-stream") {
+                    self.parse_sse_response(request_id, r)
+                } else {
+                    // For simple implementations, the response comes back as JSON directly
+                    let body: Value = r.into_json()?;
+                    Ok(body)
+                }
             }
             Err(ureq::Error::Status(code, resp)) => {
                 // Try to get SSE response from the event endpoint
@@ -238,48 +244,64 @@ impl SseTransport {
         }
     }
 
+    /// Parse SSE event stream from a response
+    fn parse_sse_response(&self, request_id: Option<u64>, resp: ureq::Response) -> Result<Value> {
+        let mut reader = BufReader::new(resp.into_reader());
+        let mut line = String::new();
+        let mut data = String::new();
+        let mut events_read = 0;
+        const MAX_EVENTS: usize = 1000; // Prevent infinite loops
+
+        loop {
+            line.clear();
+            match reader.read_line(&mut line) {
+                Ok(0) => break, // EOF
+                Ok(_) => {
+                    let line = line.trim();
+                    if let Some(stripped) = line.strip_prefix("data:") {
+                        data = stripped.trim().to_string();
+                    } else if line.is_empty() && !data.is_empty() {
+                        // End of event, parse the data
+                        events_read += 1;
+                        if events_read > MAX_EVENTS {
+                            return Err(anyhow::anyhow!(
+                                "SSE stream exceeded {} events without matching response",
+                                MAX_EVENTS
+                            ));
+                        }
+                        if let Ok(value) = serde_json::from_str::<Value>(&data) {
+                            // Check if this is the response we're waiting for
+                            if let Some(id) = request_id {
+                                if value.get("id").and_then(|v| v.as_u64()) == Some(id) {
+                                    return Ok(value);
+                                }
+                            } else {
+                                return Ok(value);
+                            }
+                        }
+                        data.clear();
+                    }
+                }
+                Err(e) => return Err(anyhow::anyhow!("SSE read error: {}", e)),
+            }
+        }
+
+        Err(anyhow::anyhow!(
+            "SSE stream ended without matching response"
+        ))
+    }
+
     fn try_sse_fallback(
         &self,
         request_id: Option<u64>,
         http_code: u16,
         resp: ureq::Response,
     ) -> Result<Value> {
-        // If the POST returned a redirect or the response is chunked,
-        // try to read it as SSE
+        // If the POST returned an error status, check if it's an SSE stream
         let content_type = resp.header("content-type").unwrap_or("").to_lowercase();
 
         if content_type.contains("text/event-stream") {
-            // Parse SSE events
-            let mut reader = BufReader::new(resp.into_reader());
-            let mut line = String::new();
-            let mut data = String::new();
-
-            loop {
-                line.clear();
-                match reader.read_line(&mut line) {
-                    Ok(0) => break, // EOF
-                    Ok(_) => {
-                        let line = line.trim();
-                        if let Some(stripped) = line.strip_prefix("data:") {
-                            data = stripped.trim().to_string();
-                        } else if line.is_empty() && !data.is_empty() {
-                            // End of event, parse the data
-                            if let Ok(value) = serde_json::from_str::<Value>(&data) {
-                                // Check if this is the response we're waiting for
-                                if let Some(id) = request_id {
-                                    if value.get("id").and_then(|v| v.as_u64()) == Some(id) {
-                                        return Ok(value);
-                                    }
-                                } else {
-                                    return Ok(value);
-                                }
-                            }
-                            data.clear();
-                        }
-                    }
-                    Err(e) => return Err(anyhow::anyhow!("SSE read error: {}", e)),
-                }
-            }
+            return self.parse_sse_response(request_id, resp);
         }
 
         Err(anyhow::anyhow!(

src/skillpacks/activation.rs

@@ -10,8 +10,6 @@ pub struct SkillActivation {
     pub name: String,
     pub description: String,
     pub allowed_tools: Option<Vec<String>>,
-    #[allow(dead_code)]
-    pub instructions: String,
 }
 
 /// Manages the set of active skills
@@ -44,7 +42,6 @@ impl ActiveSkills {
             name: pack.name.clone(),
             description: pack.description.clone(),
             allowed_tools: pack.allowed_tools.clone(),
-            instructions: pack.instructions.clone(),
         };
 
         self.active.insert(pack.name.clone(), pack);

src/subagent.rs

@@ -324,6 +324,16 @@ pub fn run_subagent(
         let choice = &response.choices[0];
         let msg = &choice.message;
 
+        // Warn if response was truncated due to length limit
+        if choice.finish_reason.as_deref() == Some("length") {
+            trace(
+                ctx,
+                agent_name,
+                "WARN",
+                "Response truncated (max tokens reached)",
+            );
+        }
+
         // Collect assistant text
         if let Some(content) = &msg.content {
             if !content.is_empty() {