From f65bb1c22483720677911cef17e3df0af7a0ec15 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 27 Feb 2026 21:51:15 +0000 Subject: [PATCH] pending-review: Add replace flag, extended-help, dry-run, fix null serialization Fix a bug where ReviewComment serialized null for optional line/side fields, causing GitHub to reject with 422. Add skip_serializing_if. The create operation now checks for existing pending service-gator reviews (identified by marker token) and errors if one exists, telling the caller to set replace=true. This makes the destructive action explicit. With replace=true, all existing pending marker-bearing reviews are deleted before creating the new one. Add extended-help operation documenting the tool interface, comment conventions from perform-forge-review, and the marker token mechanism. Add dry_run flag on the create operation for input validation without submission. Tested end-to-end against the GitHub API: extended-help, dry_run, create with inline comments, create-without-replace error, create-with-replace, and delete all pass. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: Colin Walters --- src/github.rs | 42 +++++++++++++++ src/mcp.rs | 146 +++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 174 insertions(+), 14 deletions(-) diff --git a/src/github.rs b/src/github.rs index 435d779..fe8b48d 100644 --- a/src/github.rs +++ b/src/github.rs @@ -466,6 +466,48 @@ pub fn validate_review_pending(review_json: &serde_json::Value) -> Result<()> { Ok(()) } +/// Extended help text for the pending review tool. +pub const REVIEW_TOOL_HELP: &str = "\ +# Pending Review Tool + +Manage pending (draft) PR reviews on GitHub. Reviews are created in PENDING \ +state and must be submitted by a human through the GitHub UI. + +## Operations + +- create: Create a pending review. Errors if a pending service-gator review \ +already exists on this PR; set replace=true to delete the existing one first. \ +Provide 'body' (review summary) and optionally 'comments' (inline comments \ +on specific lines). +- list: List all reviews on a PR. +- get: Get a specific review by review_id. +- update: Update the body text of an existing pending review (by review_id). \ +Cannot modify inline comments — use create to replace the whole review instead. +- delete: Delete a pending review by review_id. +- extended-help: Show this help text. + +## Dry Run + +Set dry_run: true with the 'create' operation to validate inputs without \ +submitting. Returns a summary of what would be created. + +## Example (create) + + operation: \"create\" + repo: \"owner/repo\" + pull_number: 42 + body: \"Review summary text.\" + comments: + - path: \"src/lib.rs\", line: 42, body: \"Consider adding error handling here.\" + - path: \"src/main.rs\", line: 15, body: \"Use eprintln! for error output.\" + +## Marker Token + +All reviews created by this tool include a hidden marker token in the body. \ +This allows the tool to identify its own pending reviews for idempotent \ +replacement. Only reviews with this marker can be updated or deleted. +"; + /// Analyze a `gh` command to determine target repo and read/write classification. /// Note: In API-only mode, prefer using `analyze_api()` directly. pub fn analyze(args: &[String]) -> GhAnalysis { diff --git a/src/mcp.rs b/src/mcp.rs index f8a0a9b..44af53c 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -57,7 +57,7 @@ fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { use crate::forgejo; use crate::forgejo_client::{self, ForgejoClient}; use crate::git::{BranchDescription, CommitSha, PullRequestNumber, RepoName}; -use crate::github::{self, PendingReviewOp, REVIEW_MARKER_TOKEN}; +use crate::github::{self, PendingReviewOp, REVIEW_MARKER_TOKEN, REVIEW_TOOL_HELP}; use crate::gitlab; use crate::jira::{self, JiraSubcommand}; use crate::jira_client::JiraClient; @@ -121,7 +121,8 @@ fn default_true() -> bool { /// Input schema for managing pending PR reviews. #[derive(Debug, Clone, Deserialize, JsonSchema)] pub struct GithubPendingReviewInput { - /// The operation to perform: "list", "create", "get", "update", "delete" + /// The operation to perform: "list", "create", "get", "update", "delete", + /// or "extended-help" pub operation: String, /// Repository in "owner/repo" format pub repo: String, @@ -136,6 +137,14 @@ pub struct GithubPendingReviewInput { /// Review comments for create operation #[serde(default)] pub comments: Option>, + /// If true, validate the inputs without submitting (create operation only). + #[serde(default)] + pub dry_run: bool, + /// If true, delete any existing pending service-gator review before + /// creating a new one (create operation only). Without this, create + /// will fail if a pending review already exists. + #[serde(default)] + pub replace: bool, } /// Input schema for GitLab CLI tool. @@ -234,10 +243,10 @@ pub struct ReviewComment { /// The relative path to the file pub path: String, /// The line number in the file (new version) - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub line: Option, /// The side of the diff (LEFT or RIGHT) - #[serde(default)] + #[serde(default, skip_serializing_if = "Option::is_none")] pub side: Option, /// The comment body pub body: String, @@ -1283,6 +1292,8 @@ impl ServiceGatorServer { review_id: Option, body: Option<&str>, comments: Option>, + dry_run: bool, + replace: bool, ) -> Result { // Check permission if !config @@ -1303,11 +1314,24 @@ impl ServiceGatorServer { "delete" => PendingReviewOp::Delete, other => { return Ok(CallToolResult::error(vec![Content::text(format!( - "Unknown review-operation: {other}. Use: list, create, get, update, delete" + "Unknown operation: {other}. \ + Use: list, create, get, update, delete, extended-help" ))])); } }; + // dry_run and replace only valid for create + if dry_run && op != PendingReviewOp::Create { + return Ok(CallToolResult::error(vec![Content::text( + "dry_run is only supported with the 'create' operation", + )])); + } + if replace && op != PendingReviewOp::Create { + return Ok(CallToolResult::error(vec![Content::text( + "replace is only supported with the 'create' operation", + )])); + } + // Validate review_id for operations that need it if matches!( op, @@ -1371,13 +1395,6 @@ impl ServiceGatorServer { } PendingReviewOp::Create => { - tracing::info!( - operation = "github_pending_review", - action = "create", - repo = %repo, - pull_number = pull_number, - "creating pending review" - ); let body_text = body.unwrap_or(""); let body_with_marker = if body_text.contains(REVIEW_MARKER_TOKEN) { body_text.to_string() @@ -1393,6 +1410,92 @@ impl ServiceGatorServer { payload["comments"] = serde_json::to_value(comments).unwrap_or_default(); } + let comment_count = comments.as_ref().map_or(0, |c| c.len()); + + // Dry run: return a summary without submitting + if dry_run { + return Ok(CallToolResult::success(vec![Content::text(format!( + "Dry run: would create pending review on {repo}#{pull_number}\n\ + Body: {} chars\n\ + Comments: {comment_count}", + body_text.len(), + ))])); + } + + // Check for existing pending service-gator reviews. + let list_endpoint = format!("repos/{}/pulls/{}/reviews", repo, pull_number); + let list_args = vec!["api".to_string(), "--method=GET".to_string(), list_endpoint]; + + let existing_ids: Vec = match self.exec_command("gh", &list_args).await { + Ok(output) => serde_json::from_str::>(&output) + .unwrap_or_default() + .iter() + .filter_map(|r| { + let state = r.get("state")?.as_str()?; + let rbody = r.get("body")?.as_str()?; + let id = r.get("id")?.as_u64()?; + if state == "PENDING" && github::review_has_marker(rbody) { + Some(id) + } else { + None + } + }) + .collect(), + Err(ref e) => { + tracing::warn!( + operation = "github_pending_review", + action = "list_existing", + repo = %repo, + pull_number = pull_number, + error = %e, + "failed to list existing reviews, proceeding with create" + ); + Vec::new() + } + }; + + if !existing_ids.is_empty() && !replace { + return Ok(CallToolResult::error(vec![Content::text(format!( + "A pending service-gator review already exists on \ + {repo}#{pull_number} (review id: {}). \ + Set replace=true to delete it and create a new one.", + existing_ids[0] + ))])); + } + + // Delete existing reviews if replace is set + for old_id in &existing_ids { + tracing::info!( + operation = "github_pending_review", + action = "replace_delete", + repo = %repo, + pull_number = pull_number, + review_id = old_id, + "deleting existing pending review (replace=true)" + ); + let del_endpoint = + format!("repos/{}/pulls/{}/reviews/{}", repo, pull_number, old_id); + let del_args = vec![ + "api".to_string(), + "--method=DELETE".to_string(), + del_endpoint, + ]; + if let Err(e) = self.exec_command("gh", &del_args).await { + return Ok(CallToolResult::error(vec![Content::text(format!( + "Failed to delete existing review {old_id}: {e}" + ))])); + } + } + + tracing::info!( + operation = "github_pending_review", + action = "create", + repo = %repo, + pull_number = pull_number, + comment_count = comment_count, + "creating pending review" + ); + let args = vec![ "api".to_string(), "--method=POST".to_string(), @@ -1547,10 +1650,16 @@ impl ServiceGatorServer { /// Manage pending PR reviews on GitHub. /// - /// Supports operations: list, create, get, update, delete. + /// Supports operations: list, create, get, update, delete, extended-help. + /// The create operation errors if a pending service-gator review already + /// exists; set replace=true to delete and recreate. /// Requires pending-review permission for the target repository. #[tool(description = "Manage pending PR reviews on GitHub. \ - Operations: list, create, get, update, delete. \ + Operations: list, create, get, update, delete, extended-help. \ + The 'create' operation errors if a pending review already exists; \ + set replace=true to delete and recreate. \ + Set dry_run=true to validate without submitting. \ + Use 'extended-help' for detailed documentation. \ Requires pending-review permission for the target repository. \ Use the 'status' tool to view your current permissions.")] async fn github_pending_review_tool( @@ -1559,6 +1668,13 @@ impl ServiceGatorServer { Parameters(input): Parameters, ) -> Result { let config = get_scopes_from_parts(&parts)?; + + if input.operation.eq_ignore_ascii_case("extended-help") { + return Ok(CallToolResult::success(vec![Content::text( + REVIEW_TOOL_HELP, + )])); + } + self.github_pending_review_impl( &config, &input.operation, @@ -1567,6 +1683,8 @@ impl ServiceGatorServer { input.review_id, input.body.as_deref(), input.comments, + input.dry_run, + input.replace, ) .await }