bolin feedback

codex-rs/mcp-server/src/codex_message_processor.rs

@@ -17,6 +17,7 @@ use codex_core::protocol::ReviewDecision;
 use codex_login::AuthManager;
 use codex_protocol::mcp_protocol::AuthMode;
 use codex_protocol::mcp_protocol::GitDiffToRemoteResponse;
+use codex_protocol::mcp_protocol::RunSubagentParams;
 use mcp_types::JSONRPCErrorError;
 use mcp_types::RequestId;
 use tokio::sync::Mutex;
@@ -27,6 +28,7 @@ use uuid::Uuid;
 use crate::error_code::INTERNAL_ERROR_CODE;
 use crate::error_code::INVALID_REQUEST_ERROR_CODE;
 use crate::json_to_toml::json_to_toml;
+use crate::mock_data::subagent_mock_response_from_diff;
 use crate::outgoing_message::OutgoingMessageSender;
 use crate::outgoing_message::OutgoingNotification;
 use codex_core::protocol::InputItem as CoreInputItem;
@@ -146,6 +148,9 @@ impl CodexMessageProcessor {
             ClientRequest::GetAuthStatus { request_id, params } => {
                 self.get_auth_status(request_id, params).await;
             }
+            ClientRequest::RunSubagent { request_id, params } => {
+                self.run_subagent(request_id, params).await;
+            }
         }
     }
 
@@ -354,6 +359,33 @@ impl CodexMessageProcessor {
         self.outgoing.send_response(request_id, response).await;
     }
 
+    async fn run_subagent(&self, request_id: RequestId, params: RunSubagentParams) {
+        let RunSubagentParams {
+            conversation_id,
+            subagant,
+            input: _input,
+        } = params;
+        let Ok(_conversation) = self
+            .conversation_manager
+            .get_conversation(conversation_id.0)
+            .await
+        else {
+            let error = JSONRPCErrorError {
+                code: INVALID_REQUEST_ERROR_CODE,
+                message: format!("conversation not found: {conversation_id}"),
+                data: None,
+            };
+            self.outgoing.send_error(request_id, error).await;
+            return;
+        };
+        // Build findings exclusively from the actual git diff used by the TUI.
+        let response = match git_diff_to_remote(&self.config.cwd).await {
+            Some(diff) => subagent_mock_response_from_diff(subagant, &self.config.cwd, &diff.diff),
+            None => subagent_mock_response_from_diff(subagant, &self.config.cwd, ""),
+        };
+        self.outgoing.send_response(request_id, response).await;
+    }
+
     async fn process_new_conversation(&self, request_id: RequestId, params: NewConversationParams) {
         let config = match derive_config_from_params(params, self.codex_linux_sandbox_exe.clone()) {
             Ok(config) => config,

codex-rs/mcp-server/src/lib.rs

@@ -27,6 +27,7 @@ mod error_code;
 mod exec_approval;
 mod json_to_toml;
 pub(crate) mod message_processor;
+mod mock_data;
 mod outgoing_message;
 mod patch_approval;
 

codex-rs/mcp-server/src/mock_data.rs

@@ -0,0 +1,214 @@
+use std::path::Path;
+use std::path::PathBuf;
+
+use codex_protocol::mcp_protocol::CodeLocation;
+use codex_protocol::mcp_protocol::Finding;
+use codex_protocol::mcp_protocol::LineRange;
+use codex_protocol::mcp_protocol::ReviewOutput;
+use codex_protocol::mcp_protocol::RunSubagentResponse;
+use codex_protocol::mcp_protocol::Subagent;
+use codex_protocol::mcp_protocol::SubagentOutput;
+
+/// Build a mock response for Review subagent using actual unified diff text.
+pub(crate) fn subagent_mock_response_from_diff(
+    subagent: Subagent,
+    cwd: &Path,
+    diff: &str,
+) -> RunSubagentResponse {
+    match subagent {
+        Subagent::Review => {
+            let findings = review_findings_from_unified_diff(cwd, diff);
+            RunSubagentResponse {
+                output: SubagentOutput::Review(ReviewOutput { findings }),
+            }
+        }
+    }
+}
+
+/// Parse a unified diff and generate representative findings mapped to changed hunks.
+fn review_findings_from_unified_diff(cwd: &Path, diff: &str) -> Vec<Finding> {
+    const TITLES: &[&str] = &[
+        "Add a clarifying comment",
+        "Consider extracting a helper function",
+        "Prefer descriptive variable names",
+        "Validate inputs and handle errors early",
+        "Document the intent of this change",
+        "Consider reducing nesting with early returns",
+        "Add unit tests for this branch",
+        "Ensure consistent logging and levels",
+    ];
+    const BODIES: &[&str] = &[
+        "Add a comment to this line to explain the rationale.",
+        "This logic could be extracted for readability and reuse.",
+        "Use a more descriptive identifier to clarify the purpose.",
+        "Add a guard clause to handle invalid or edge inputs.",
+        "Add a doc comment describing the behavior for maintainers.",
+        "Flatten control flow using early returns where safe.",
+        "Add a focused test that covers this behavior.",
+        "Use the shared logger and appropriate log level.",
+    ];
+
+    let mut findings: Vec<Finding> = Vec::new();
+    let mut current_file: Option<PathBuf> = None;
+    let mut in_hunk: bool = false;
+    let mut new_line: u32 = 1;
+    let mut template_index: usize = 0;
+
+    for line in diff.lines() {
+        if line.starts_with("diff --git ") {
+            current_file = None;
+            in_hunk = false;
+            continue;
+        }
+
+        if let Some(rest) = line.strip_prefix("+++ b/") {
+            current_file = Some(cwd.join(rest.trim()));
+            continue;
+        }
+        if line.starts_with("+++ ") || line.starts_with("--- ") {
+            continue;
+        }
+
+        if let Some(hunk_header) = line.strip_prefix("@@") {
+            if let Some((_, after_plus)) = hunk_header.split_once('+') {
+                let mut range_text = after_plus.trim();
+                if let Some((seg, _)) = range_text.split_once(' ') {
+                    range_text = seg;
+                }
+                let (start, _count) = parse_start_count(range_text);
+                new_line = start;
+                in_hunk = true;
+            }
+            continue;
+        }
+
+        if in_hunk {
+            if line.starts_with(' ') {
+                new_line = new_line.saturating_add(1);
+            } else if line.starts_with('-') {
+                // deletion: no advance of new_line
+            } else if line.starts_with('+') && !line.starts_with("+++") {
+                if let Some(path) = &current_file {
+                    let title = TITLES[template_index % TITLES.len()].to_string();
+                    let mut body = BODIES[template_index % BODIES.len()].to_string();
+                    let snippet = line.trim_start_matches('+').trim();
+                    if !snippet.is_empty() {
+                        body.push_str("\nSnippet: ");
+                        let truncated = if snippet.len() > 140 {
+                            let mut s = snippet[..140].to_string();
+                            s.push('…');
+                            s
+                        } else {
+                            snippet.to_string()
+                        };
+                        body.push_str(&truncated);
+                    }
+
+                    findings.push(Finding {
+                        title,
+                        body,
+                        confidence_score: confidence_for_index(template_index),
+                        code_location: CodeLocation {
+                            absolute_file_path: to_forward_slashes(path),
+                            line_range: LineRange {
+                                start: new_line,
+                                end: new_line,
+                            },
+                        },
+                    });
+                    template_index += 1;
+                }
+                new_line = new_line.saturating_add(1);
+            }
+        }
+    }
+
+    if findings.len() > 50 {
+        findings.truncate(50);
+    }
+    findings
+}
+
+fn confidence_for_index(i: usize) -> f32 {
+    let base = 0.72f32;
+    let step = (i as f32 % 7.0) * 0.03;
+    (base + step).min(0.95)
+}
+
+fn parse_start_count(text: &str) -> (u32, u32) {
+    // Formats: "123,45" or just "123"
+    if let Some((start_str, count_str)) = text.split_once(',') {
+        let start = start_str
+            .trim()
+            .trim_start_matches('+')
+            .parse()
+            .unwrap_or(1);
+        let count = count_str.trim().parse().unwrap_or(1);
+        (start as u32, count as u32)
+    } else {
+        let start = text.trim().trim_start_matches('+').parse().unwrap_or(1);
+        (start as u32, 1)
+    }
+}
+
+fn to_forward_slashes(path: &Path) -> String {
+    path.to_string_lossy().replace('\\', "/")
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::path::Path;
+
+    #[test]
+    fn returns_empty_findings_for_empty_diff() {
+        let cwd = Path::new("/tmp");
+        let resp = subagent_mock_response_from_diff(Subagent::Review, cwd, "");
+        match resp.output {
+            SubagentOutput::Review(ReviewOutput { findings }) => {
+                assert!(findings.is_empty(), "Expected no findings for empty diff");
+            }
+        }
+    }
+
+    #[test]
+    fn generates_findings_for_added_lines_with_correct_locations() {
+        let cwd = Path::new("/repo");
+        let diff = r#"diff --git a/src/lib.rs b/src/lib.rs
+index 1111111..2222222 100644
+--- a/src/lib.rs
++++ b/src/lib.rs
+@@ -10,2 +10,4 @@
+ context
++let x = 1;
++// TODO: add docs
+ context
+@@ -50,3 +52,5 @@
+-context
++context changed
++fn new_fn() {}
++// comment
+"#;
+
+        let resp = subagent_mock_response_from_diff(Subagent::Review, cwd, diff);
+        match resp.output {
+            SubagentOutput::Review(ReviewOutput { findings }) => {
+                // Added lines: 2 in first hunk, 3 in second hunk => 5 findings
+                assert_eq!(findings.len(), 5, "Expected one finding per added line");
+
+                // Validate file path and line numbers for the first two additions
+                let file_path = "/repo/src/lib.rs".to_string();
+                assert_eq!(findings[0].code_location.absolute_file_path, file_path);
+                assert_eq!(findings[0].code_location.line_range.start, 11);
+                assert_eq!(findings[0].code_location.line_range.end, 11);
+                assert_eq!(findings[1].code_location.absolute_file_path, file_path);
+                assert_eq!(findings[1].code_location.line_range.start, 12);
+                assert_eq!(findings[1].code_location.line_range.end, 12);
+
+                // Validate second hunk first two additions start at 52, then 53
+                assert_eq!(findings[2].code_location.line_range.start, 52);
+                assert_eq!(findings[3].code_location.line_range.start, 53);
+            }
+        }
+    }
+}

codex-rs/protocol/src/mcp_protocol.rs

@@ -101,8 +101,67 @@ pub enum ClientRequest {
         request_id: RequestId,
         params: GetAuthStatusParams,
     },
+    RunSubagent {
+        #[serde(rename = "id")]
+        request_id: RequestId,
+        params: RunSubagentParams,
+    },
 }
 
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)]
+#[serde(rename_all = "camelCase")]
+pub struct RunSubagentParams {
+    pub conversation_id: ConversationId,
+    pub subagant: Subagent,
+    pub input: Option<Vec<InputItem>>,
+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)]
+#[serde(rename_all = "camelCase")]
+pub enum Subagent {
+    Review,
+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)]
+#[serde(tag = "type", content = "data", rename_all = "camelCase")]
+pub enum SubagentOutput {
+    Review(ReviewOutput),
+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)]
+#[serde(rename_all = "camelCase")]
+pub struct RunSubagentResponse {
+    pub output: SubagentOutput,
+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)]
+#[serde(rename_all = "snake_case")]
+pub struct ReviewOutput {
+    pub findings: Vec<Finding>,
+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)]
+#[serde(rename_all = "snake_case")]
+pub struct Finding {
+    pub title: String,
+    pub body: String,
+    pub confidence_score: f32,
+    pub code_location: CodeLocation,
+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)]
+#[serde(rename_all = "snake_case")]
+pub struct CodeLocation {
+    pub absolute_file_path: String,
+    pub line_range: LineRange,
+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)]
+#[serde(rename_all = "snake_case")]
+pub struct LineRange {
+    pub start: u32,
+    pub end: u32,
+}
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, TS)]
 #[serde(rename_all = "camelCase")]
 pub struct NewConversationParams {