Skip to content

fix: clean reviewer output #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
morgaesis merged 1 commit into from
Jun 5, 2026
Merged

fix: clean reviewer output #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 69 additions & 18 deletions src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -278,7 +285,8 @@ struct PullReviewRequest<'a> {
#[serde(skip_serializing_if = "Option::is_none")]
commit_id: Option<String>,
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<Vec<ReviewComment>>,
}
Expand Down Expand Up @@ -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
Expand All @@ -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()
}
Expand All @@ -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()
Expand Down
56 changes: 23 additions & 33 deletions src/review.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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::<Vec<_>>()
.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 {
Expand All @@ -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."
Expand Down Expand Up @@ -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(),
Expand All @@ -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("!["));
}
}