From 7f1f0eb635b3172146499b341967428df600216e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Fri, 5 Jun 2026 12:36:16 +0000 Subject: [PATCH] fix: clean reviewer output --- src/github.rs | 87 ++++++++++++++++++++++++++++++++++++++++----------- src/review.rs | 56 ++++++++++++++------------------- 2 files changed, 92 insertions(+), 51 deletions(-) diff --git a/src/github.rs b/src/github.rs index 20f57f1..e03c88e 100644 --- a/src/github.rs +++ b/src/github.rs @@ -6,7 +6,7 @@ use serde_json::json; use crate::{ config::{RepoReviewConfig, ReviewTarget, translate_coderabbit, translate_kodo}, - review::{Finding, ReviewEnvelope}, + review::{Finding, ReviewEnvelope, severity_marks}, text::limit_text, }; @@ -138,7 +138,11 @@ impl GithubClient { } else { "COMMENT" }, - body, + body: if body.trim().is_empty() { + None + } else { + Some(body) + }, comments: if comments.is_empty() { None } else { @@ -169,6 +173,9 @@ impl GithubClient { } pub async fn post_issue_comment(&self, target: &ReviewTarget, body: &str) -> Result<()> { + if body.trim().is_empty() { + return Ok(()); + } let res = self .http .post(format!( @@ -278,7 +285,8 @@ struct PullReviewRequest<'a> { #[serde(skip_serializing_if = "Option::is_none")] commit_id: Option, event: &'a str, - body: &'a str, + #[serde(skip_serializing_if = "Option::is_none")] + body: Option<&'a str>, #[serde(skip_serializing_if = "Option::is_none")] comments: Option>, } @@ -309,21 +317,11 @@ impl CheckOutput { } pub fn from_envelope(envelope: &ReviewEnvelope) -> Self { - let mut errors = 0; - let mut warnings = 0; - for finding in &envelope.findings { - match finding.severity { - crate::config::Severity::Error => errors += 1, - crate::config::Severity::Warn => warnings += 1, - crate::config::Severity::Info => {} - } - } - let title = if errors > 0 { - format!("{errors} error{}", if errors == 1 { "" } else { "s" }) - } else if warnings > 0 { - format!("{warnings} warning{}", if warnings == 1 { "" } else { "s" }) - } else { + let marks = severity_marks(&envelope.findings); + let title = if marks.is_empty() { "No merge-relevant findings".to_string() + } else { + marks }; let text = if envelope.findings.is_empty() { None @@ -334,7 +332,7 @@ impl CheckOutput { title, summary: if envelope.summary.trim().is_empty() { if envelope.findings.is_empty() { - "No merge-relevant findings.".to_string() + String::new() } else { "See inline review comments.".to_string() } @@ -346,6 +344,59 @@ impl CheckOutput { } } +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + config::Severity, + review::{FindingKind, TokenUsage}, + }; + + #[test] + fn clean_check_output_has_no_recap_body() { + let output = CheckOutput::from_envelope(&ReviewEnvelope { + summary: String::new(), + findings: Vec::new(), + usage: TokenUsage::default(), + model_used: "m".into(), + }); + + assert_eq!(output.title, "No merge-relevant findings"); + assert_eq!(output.summary, ""); + assert_eq!(output.text, None); + } + + #[test] + fn dirty_check_output_uses_repeated_severity_marks() { + let output = CheckOutput::from_envelope(&ReviewEnvelope { + summary: String::new(), + findings: vec![ + finding(Severity::Error), + finding(Severity::Warn), + finding(Severity::Warn), + finding(Severity::Info), + ], + usage: TokenUsage::default(), + model_used: "m".into(), + }); + + assert_eq!(output.title, "!!! !! !! !"); + assert!(!output.title.contains("warning")); + assert!(!output.title.contains("error")); + assert!(!output.summary.contains("No merge-relevant")); + } + + fn finding(severity: Severity) -> Finding { + Finding { + path: "src/lib.rs".into(), + line: 1, + severity, + kind: Some(FindingKind::Risk), + body: "risk".into(), + } + } +} + fn render_findings(findings: &[Finding]) -> String { findings .iter() diff --git a/src/review.rs b/src/review.rs index 67d76dc..5969011 100644 --- a/src/review.rs +++ b/src/review.rs @@ -5,8 +5,6 @@ use serde_json::Value; use crate::config::{RepoReviewConfig, Severity}; -const STATUS_ICON_BASE_URL: &str = "https://postil.dev/status"; - const BASE_SYSTEM_PROMPT: &str = r#"You are Postil, a low-noise review gate for agent-speed development. You receive a unified diff for a pull request and produce structured findings as JSON. Product doctrine: @@ -207,35 +205,25 @@ pub fn apply_config( Ok(envelope) } -pub fn status_line(envelope: &ReviewEnvelope, _inline_comments: usize, label: &str) -> String { - let mut errors = 0; - let mut warnings = 0; - let mut infos = 0; - for finding in &envelope.findings { - match finding.severity { - Severity::Error => errors += 1, - Severity::Warn => warnings += 1, - Severity::Info => infos += 1, - } - } - let mut status = String::new(); - for _ in 0..errors { - status.push_str(&status_icon("error")); - } - for _ in 0..warnings { - status.push_str(&status_icon("warn")); - } - for _ in 0..infos { - status.push_str(&status_icon("info")); - } - if status.is_empty() { - status.push_str(&status_icon(if label == "clean" { "pass" } else { "warn" })); - } - format!("status: {status}") +pub fn severity_marks(findings: &[Finding]) -> String { + findings + .iter() + .map(|finding| match finding.severity { + Severity::Error => "!!!", + Severity::Warn => "!!", + Severity::Info => "!", + }) + .collect::>() + .join(" ") } -fn status_icon(kind: &str) -> String { - format!("![{kind}]({STATUS_ICON_BASE_URL}/{kind}.svg)") +pub fn status_line(envelope: &ReviewEnvelope, _inline_comments: usize, _label: &str) -> String { + let marks = severity_marks(&envelope.findings); + if marks.is_empty() { + "status: clean".to_string() + } else { + format!("status: {marks}") + } } pub fn append_status(body: &str, status: &str) -> String { @@ -248,6 +236,9 @@ pub fn append_status(body: &str, status: &str) -> String { } pub fn review_body(envelope: &ReviewEnvelope, inline_comments: usize, label: &str) -> String { + if envelope.findings.is_empty() { + return String::new(); + } append_status( if envelope.summary.trim().is_empty() && !envelope.findings.is_empty() { "Postil found merge-relevant review findings." @@ -367,10 +358,7 @@ mod tests { usage: TokenUsage::default(), model_used: "m".into(), }; - assert_eq!( - review_body(&clean, 0, "clean"), - "status: ![pass](https://postil.dev/status/pass.svg)" - ); + assert_eq!(review_body(&clean, 0, "clean"), ""); let dirty = ReviewEnvelope { summary: String::new(), @@ -385,5 +373,7 @@ mod tests { model_used: "m".into(), }; assert!(review_body(&dirty, 1, "needs-attention").contains("merge-relevant")); + assert!(review_body(&dirty, 1, "needs-attention").contains("status: !!")); + assert!(!review_body(&dirty, 1, "needs-attention").contains("![")); } }