Skip to content
This repository was archived by the owner on Sep 23, 2025. It is now read-only.

Commit ecc0436

Browse files
nikomatsakisnikomatsakis
committed
Refactor to clean request-response pattern for MCP tools
Major simplification of the blocking pattern: **New Flow:** 1. Update review state (add comment, change status, etc.) 2. Send updated state to VSCode extension via IPC 3. Block until extension responds with user feedback 4. Return user response directly to LLM **Changes:** - Replace complex blocking mechanism with simple send_review_update() - Remove format_user_feedback_message() - extension formats response - Update both request_review and update_review to use same pattern - Make send_review_update() generic to handle any serializable review data - Remove unused UserFeedback imports and complex feedback handling **Result:** Much cleaner, more predictable request-response cycle. Extension receives update, shows to user, responds back. Addresses #22 - implements clean IPC request-response for LLM roundtrip
1 parent 3e8cccf commit ecc0436

File tree

3 files changed

+123
-133
lines changed

3 files changed

+123
-133
lines changed

‎server/src/ipc.rs‎

Lines changed: 90 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
use crate::types::{
77
FindAllReferencesPayload, GetSelectionResult, GoodbyePayload, IPCMessage, IPCMessageType,
88
LogLevel, LogParams, PoloPayload, PresentReviewParams, PresentReviewResult,
9-
ResolveSymbolByNamePayload, ResponsePayload, SyntheticPRPayload, UserFeedbackPayload,
9+
ResolveSymbolByNamePayload, ResponsePayload, UserFeedbackPayload,
1010
};
1111
use futures::FutureExt;
1212
use serde_json;
@@ -361,7 +361,10 @@ impl IPCCommunicator {
361361
}
362362

363363
/// Send synthetic PR data to VSCode extension
364-
pub async fn send_create_synthetic_pr(&self, review_response: &crate::synthetic_pr::ReviewResponse) -> Result<()> {
364+
pub async fn send_create_synthetic_pr(
365+
&self,
366+
review_response: &crate::synthetic_pr::ReviewData,
367+
) -> Result<()> {
365368
if self.test_mode {
366369
info!("Synthetic PR creation message sent (test mode)");
367370
return Ok(());
@@ -389,15 +392,24 @@ impl IPCCommunicator {
389392
id: Uuid::new_v4().to_string(),
390393
};
391394

392-
debug!("Sending create_synthetic_pr message for review: {}", review_response.review_id);
395+
debug!(
396+
"Sending create_synthetic_pr message for review: {}",
397+
review_response.review_id
398+
);
393399
self.send_message_without_reply(message).await
394400
}
395401

396402
/// Wait for user feedback on a specific review
397403
/// This method blocks until the user provides feedback via the VSCode extension
398-
pub async fn wait_for_user_feedback(&self, review_id: &str) -> Result<crate::synthetic_pr::UserFeedback> {
404+
pub async fn wait_for_user_feedback(
405+
&self,
406+
review_id: &str,
407+
) -> Result<crate::synthetic_pr::UserFeedback> {
399408
if self.test_mode {
400-
info!("Wait for user feedback called (test mode) for review: {}", review_id);
409+
info!(
410+
"Wait for user feedback called (test mode) for review: {}",
411+
review_id
412+
);
401413
// Return mock feedback for testing
402414
return Ok(crate::synthetic_pr::UserFeedback {
403415
review_id: Some(review_id.to_string()),
@@ -407,7 +419,11 @@ impl IPCCommunicator {
407419
comment_text: Some("This is a test comment".to_string()),
408420
completion_action: None,
409421
additional_notes: None,
410-
context_lines: Some(vec!["fn test() {".to_string(), " // Line 42".to_string(), "}".to_string()]),
422+
context_lines: Some(vec![
423+
"fn test() {".to_string(),
424+
" // Line 42".to_string(),
425+
"}".to_string(),
426+
]),
411427
});
412428
}
413429

@@ -434,12 +450,53 @@ impl IPCCommunicator {
434450
inner.pending_feedback.remove(review_id);
435451
Err(IPCError::ConnectionFailed {
436452
path: "user_feedback".to_string(),
437-
source: std::io::Error::new(std::io::ErrorKind::BrokenPipe, "User feedback channel closed")
453+
source: std::io::Error::new(
454+
std::io::ErrorKind::BrokenPipe,
455+
"User feedback channel closed",
456+
),
438457
})
439458
}
440459
}
441460
}
442461

462+
/// Send review update to VSCode extension and wait for user response
463+
/// This method blocks until the user provides feedback via the VSCode extension
464+
pub async fn send_review_update<T: serde::Serialize>(&self, review: &T) -> Result<String> {
465+
if self.test_mode {
466+
info!("Send review update called (test mode)");
467+
return Ok("User responded: This looks good to me!".to_string());
468+
}
469+
470+
let shell_pid = {
471+
let inner = self.inner.lock().await;
472+
inner.terminal_shell_pid
473+
};
474+
475+
// Create update payload
476+
let payload = serde_json::to_value(review)?;
477+
478+
let message = IPCMessage {
479+
shell_pid,
480+
message_type: IPCMessageType::UpdateSyntheticPr,
481+
payload,
482+
id: Uuid::new_v4().to_string(),
483+
};
484+
485+
// Send message and wait for response
486+
let response = self.send_message_with_reply(message).await?;
487+
488+
// Extract user response text from the response
489+
if let Some(data) = response.data {
490+
if let Some(user_text) = data.get("user_response").and_then(|v| v.as_str()) {
491+
Ok(user_text.to_string())
492+
} else {
493+
Ok("User completed the review.".to_string())
494+
}
495+
} else {
496+
Ok("User completed the review.".to_string())
497+
}
498+
}
499+
443500
/// Gracefully shutdown the IPC communicator, sending Goodbye discovery message
444501
pub async fn shutdown(&self) -> Result<()> {
445502
if self.test_mode {
@@ -805,13 +862,14 @@ impl IPCCommunicator {
805862
info!("Received user feedback message");
806863

807864
// Parse the user feedback payload
808-
let feedback_payload: UserFeedbackPayload = match serde_json::from_value(message.payload) {
809-
Ok(payload) => payload,
810-
Err(e) => {
811-
error!("Failed to parse user feedback payload: {}", e);
812-
return;
813-
}
814-
};
865+
let feedback_payload: UserFeedbackPayload =
866+
match serde_json::from_value(message.payload) {
867+
Ok(payload) => payload,
868+
Err(e) => {
869+
error!("Failed to parse user feedback payload: {}", e);
870+
return;
871+
}
872+
};
815873

816874
// Convert to UserFeedback struct
817875
let user_feedback = crate::synthetic_pr::UserFeedback {
@@ -823,26 +881,36 @@ impl IPCCommunicator {
823881
file_path: feedback_payload.file_path,
824882
line_number: feedback_payload.line_number,
825883
comment_text: feedback_payload.comment_text,
826-
completion_action: feedback_payload.completion_action.as_deref().and_then(|action| {
827-
match action {
828-
"request_changes" => Some(crate::synthetic_pr::CompletionAction::RequestChanges),
884+
completion_action: feedback_payload.completion_action.as_deref().and_then(
885+
|action| match action {
886+
"request_changes" => {
887+
Some(crate::synthetic_pr::CompletionAction::RequestChanges)
888+
}
829889
"checkpoint" => Some(crate::synthetic_pr::CompletionAction::Checkpoint),
830890
"return" => Some(crate::synthetic_pr::CompletionAction::Return),
831891
_ => None,
832-
}
833-
}),
892+
},
893+
),
834894
additional_notes: feedback_payload.additional_notes,
835895
context_lines: feedback_payload.context_lines,
836896
};
837897

838898
// Send to waiting MCP tool
839899
let mut inner_guard = inner.lock().await;
840-
if let Some(sender) = inner_guard.pending_feedback.remove(&feedback_payload.review_id) {
900+
if let Some(sender) = inner_guard
901+
.pending_feedback
902+
.remove(&feedback_payload.review_id)
903+
{
841904
if let Err(_) = sender.send(user_feedback) {
842-
warn!("Failed to send user feedback to waiting MCP tool - receiver dropped");
905+
warn!(
906+
"Failed to send user feedback to waiting MCP tool - receiver dropped"
907+
);
843908
}
844909
} else {
845-
warn!("Received user feedback for unknown review ID: {}", feedback_payload.review_id);
910+
warn!(
911+
"Received user feedback for unknown review ID: {}",
912+
feedback_payload.review_id
913+
);
846914
}
847915
}
848916
_ => {

‎server/src/server.rs‎

Lines changed: 27 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use tracing::info;
1717

1818
use crate::dialect::DialectInterpreter;
1919
use crate::ipc::IPCCommunicator;
20-
use crate::synthetic_pr::{RequestReviewParams, UpdateReviewParams, UserFeedback};
20+
use crate::synthetic_pr::{RequestReviewParams, UpdateReviewParams};
2121
use crate::types::{LogLevel, PresentReviewParams};
2222
use serde::{Deserialize, Serialize};
2323

@@ -84,67 +84,6 @@ impl DialecticServer {
8484
&self.ipc
8585
}
8686

87-
/// Format user feedback into clear instructions for the LLM
88-
fn format_user_feedback_message(&self, feedback: &UserFeedback) -> String {
89-
match feedback.feedback_type {
90-
crate::synthetic_pr::FeedbackType::Comment => {
91-
let file_path = feedback.file_path.as_deref().unwrap_or("unknown file");
92-
let line_number = feedback.line_number.unwrap_or(0);
93-
let comment_text = feedback.comment_text.as_deref().unwrap_or("(no comment)");
94-
95-
let context = if let Some(lines) = &feedback.context_lines {
96-
format!("\n\nCode context:\n```\n{}\n```", lines.join("\n"))
97-
} else {
98-
String::new()
99-
};
100-
101-
format!(
102-
"The user reviewed your code changes and left a comment on file `{}` at line {}:\n\n\
103-
User comment: '{}'{}\n\n\
104-
Please analyze the user's feedback and prepare a thoughtful response addressing their concern. \
105-
Do NOT modify any files on disk.\n\n\
106-
When ready, invoke the update_review tool with:\n\
107-
- review_id: '{}'\n\
108-
- action: AddComment\n\
109-
- comment: {{ response: 'Your response text here' }}\n\n\
110-
After responding, invoke update_review again with action: WaitForFeedback to continue the conversation.",
111-
file_path, line_number, comment_text, context, feedback.review_id.as_deref().unwrap_or("unknown")
112-
)
113-
}
114-
crate::synthetic_pr::FeedbackType::CompleteReview => {
115-
let action = feedback.completion_action.as_ref();
116-
let notes = feedback.additional_notes.as_deref().unwrap_or("");
117-
118-
let notes_section = if !notes.is_empty() {
119-
format!("\nAdditional notes: '{}'\n", notes)
120-
} else {
121-
String::new()
122-
};
123-
124-
match action {
125-
Some(crate::synthetic_pr::CompletionAction::RequestChanges) => format!(
126-
"User completed their review and selected: 'Request agent to make changes'{}\n\
127-
Based on the review discussion, please implement the requested changes. \
128-
You may now edit files as needed.\n\n\
129-
When finished, invoke: update_review(review_id: '{}', action: Approve)",
130-
notes_section, feedback.review_id.as_deref().unwrap_or("unknown")
131-
),
132-
Some(crate::synthetic_pr::CompletionAction::Checkpoint) => format!(
133-
"User completed their review and selected: 'Request agent to checkpoint this work'{}\n\
134-
Please commit the current changes and document the work completed.\n\n\
135-
When finished, invoke: update_review(review_id: '{}', action: Approve)",
136-
notes_section, feedback.review_id.as_deref().unwrap_or("unknown")
137-
),
138-
_ => format!(
139-
"User completed their review and selected: 'Return to agent without explicit request'{}\n\
140-
The review is complete. You may proceed as you see fit.",
141-
notes_section
142-
)
143-
}
144-
}
145-
}
146-
}
147-
14887
/// Ensure the message bus daemon is running for the given VSCode PID
14988
async fn ensure_daemon_running(vscode_pid: u32) -> Result<()> {
15089
crate::daemon::spawn_daemon_process(vscode_pid).await
@@ -451,33 +390,32 @@ impl DialecticServer {
451390
)
452391
.await;
453392

454-
// BLOCK until user provides initial feedback
455-
let user_feedback = self.ipc
456-
.wait_for_user_feedback(&result.review_id)
393+
// Send initial review to VSCode extension and wait for user response
394+
let user_response = self.ipc
395+
.send_review_update(&result)
457396
.await
458397
.map_err(|e| {
459398
McpError::internal_error(
460-
"Failed to wait for user feedback",
399+
"Failed to send initial review",
461400
Some(serde_json::json!({
462401
"error": e.to_string()
463402
})),
464403
)
465404
})?;
466405

467-
// Return structured message telling LLM what user did and what to do next
468-
let message = self.format_user_feedback_message(&user_feedback);
469-
470-
Ok(CallToolResult::success(vec![Content::text(message)]))
406+
Ok(CallToolResult::success(vec![Content::text(user_response)]))
471407
}
472408

409+
// ANCHOR: update_review_tool
473410
/// Update an existing synthetic pull request or wait for user feedback
474411
///
475412
/// Supports actions: wait_for_feedback, add_comment, approve, request_changes.
476413
/// Used for iterative review workflows between AI and developer.
477-
// ANCHOR: update_review_tool
478-
#[tool(description = "Update an existing synthetic pull request or wait for user feedback. \
414+
#[tool(
415+
description = "Update an existing synthetic pull request or wait for user feedback. \
479416
This tool is used to interact with the user through their IDE. \
480-
Do not invoke it except when asked to do so by other tools within dialectic.")]
417+
Do not invoke it except when asked to do so by other tools within dialectic."
418+
)]
481419
async fn update_review(
482420
&self,
483421
Parameters(params): Parameters<UpdateReviewParams>,
@@ -489,7 +427,8 @@ impl DialecticServer {
489427
)
490428
.await;
491429

492-
let result = crate::synthetic_pr::update_review(params.clone())
430+
// 1. Update the review state based on action
431+
let updated_review = crate::synthetic_pr::update_review(params)
493432
.await
494433
.map_err(|e| {
495434
McpError::internal_error(
@@ -500,38 +439,21 @@ impl DialecticServer {
500439
)
501440
})?;
502441

503-
// Handle WaitForFeedback action - this blocks until user responds
504-
if let crate::synthetic_pr::UpdateReviewAction::WaitForFeedback = params.action {
505-
let user_feedback = self.ipc
506-
.wait_for_user_feedback(&params.review_id)
507-
.await
508-
.map_err(|e| {
509-
McpError::internal_error(
510-
"Failed to wait for user feedback",
511-
Some(serde_json::json!({
512-
"error": e.to_string()
513-
})),
514-
)
515-
})?;
516-
517-
let message = self.format_user_feedback_message(&user_feedback);
518-
return Ok(CallToolResult::success(vec![Content::text(message)]));
519-
}
520-
521-
self.ipc
522-
.send_log(LogLevel::Info, format!("Review updated: {}", result.status))
523-
.await;
524-
525-
let json_content = Content::json(result).map_err(|e| {
526-
McpError::internal_error(
527-
"Serialization failed",
528-
Some(serde_json::json!({
529-
"error": format!("Failed to serialize update result: {}", e)
530-
})),
531-
)
532-
})?;
442+
// 2. Send updated state to VSCode extension via IPC and wait for response
443+
let user_response = self.ipc
444+
.send_review_update(&updated_review)
445+
.await
446+
.map_err(|e| {
447+
McpError::internal_error(
448+
"Failed to send review update",
449+
Some(serde_json::json!({
450+
"error": e.to_string()
451+
})),
452+
)
453+
})?;
533454

534-
Ok(CallToolResult::success(vec![json_content]))
455+
// 3. Return user's response to LLM
456+
Ok(CallToolResult::success(vec![Content::text(user_response)]))
535457
}
536458

537459
/// Get the status of the current synthetic pull request

0 commit comments

Comments
 (0)