From 0b8caf97b1d8985a7c50b72836d50a5d3f6b1b28 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sun, 12 Jul 2026 02:52:10 +0000 Subject: [PATCH] Fix review comment formatting and add humanEscalation confidence floor Addresses PR comment formatting gaps by removing redundant path:line prefixes from top-level summaries (already shown in inline comments), wrapping review metadata in a foldable
section with commit link and dashboard run URL, and making gate outcomes (pass/fail) explicit and unambiguous. Implements a 0.30 confidence floor for humanEscalation-kind findings to prevent weak boilerplate findings from blocking gates while preserving visibility for human review. The confidence floor is documented in code and tested. Findings below the floor still appear in summaries with a note indicating they do not block. All backends (GitHub, GitLab, Bitbucket, Azure) updated to use the new SummaryContext for consistent behavior. Gate logic refactored into a dedicated finding_blocks_gate() function to ensure live reviews, envelope replay, and forge summaries share identical blocking semantics. --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/config.rs | 5 +- src/envelope.rs | 59 +++++++++++++ src/forge/azure.rs | 2 +- src/forge/bitbucket.rs | 2 +- src/forge/github.rs | 138 +++++++++++++++++++++++++++--- src/forge/gitlab.rs | 2 +- src/forge/mod.rs | 189 ++++++++++++++++++++++++++++++++++++++--- src/plan.rs | 42 ++++++++- src/review.rs | 36 ++++---- tests/e2e.rs | 111 +++++++++++++++++++++++- 12 files changed, 535 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d5bff32..fb19284 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1041,7 +1041,7 @@ checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" [[package]] name = "postil-cli" -version = "0.4.3" +version = "0.4.4" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 4354eab..c9e7fa8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "postil-cli" -version = "0.4.3" +version = "0.4.4" edition = "2024" description = "Postil: a low-noise AI review gate. Silent on clean PRs, hard gate on real risk." license = "Apache-2.0" diff --git a/src/config.rs b/src/config.rs index 152aa20..31cc65c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -91,7 +91,8 @@ pub struct Config { pub gate_fail_on: GateLevel, /// Gate behavior on operational error. Default: fail closed. pub gate_on_error: OnError, - /// Finding kinds that block the gate regardless of severity. Default: [HumanEscalation]. + /// Finding kinds that block regardless of severity. Human escalations must + /// also meet the calibrated 0.30 gate confidence floor. Default: [HumanEscalation]. pub block_on_kinds: Vec, pub model: String, pub cascade: Vec, @@ -499,7 +500,7 @@ review: gate: failOn: error # the postil/gate check fails at/above: info | warn | error | never - # blockOnKinds: # finding kinds that block regardless of severity (default: [humanEscalation]) + # blockOnKinds: # kinds that block regardless of severity; humanEscalation also requires confidence >= 0.30 # - humanEscalation # onError: block # block (default, fail closed) | advisory — gate outcome when # # the review itself errors (model outage). advisory keeps an diff --git a/src/envelope.rs b/src/envelope.rs index 4414a12..30ef527 100644 --- a/src/envelope.rs +++ b/src/envelope.rs @@ -47,6 +47,15 @@ pub enum Kind { ContentPolicy, } +/// Minimum confidence at which a human-escalation finding can block a merge. +/// +/// Escalations are intentionally kept visible below this floor so reviewers can +/// inspect weak signals, but a very weak escalation must not turn into a hard +/// gate merely because of its kind or severity. A 0.30 floor is conservative: +/// it filters the near-zero boilerplate observed in production while retaining +/// uncertain but concrete concerns for human attention. +pub const HUMAN_ESCALATION_GATE_MIN_CONFIDENCE: f64 = 0.30; + impl Kind { pub fn parse(s: &str) -> Option { match s.trim().to_ascii_lowercase().as_str() { @@ -192,6 +201,33 @@ impl Envelope { } } +/// Apply the serialized gate contract to one finding. +/// +/// Keeping this beside the envelope makes live reviews, stored-envelope replay, +/// and forge summaries share exactly the same blocking semantics. +pub fn finding_blocks_gate( + finding: &Finding, + fail_on: &str, + block_on_kinds: &[String], + provider_error_is_advisory: bool, +) -> bool { + if finding.kind == Kind::HumanEscalation + && finding.confidence < HUMAN_ESCALATION_GATE_MIN_CONFIDENCE + { + return false; + } + + let severity_blocks = if provider_error_is_advisory || fail_on.eq_ignore_ascii_case("never") { + false + } else { + Severity::parse(fail_on).is_some_and(|threshold| finding.severity >= threshold) + }; + let kind_blocks = block_on_kinds + .iter() + .any(|kind| Kind::parse(kind).is_some_and(|kind| kind == finding.kind)); + severity_blocks || kind_blocks +} + /// Path marker for synthetic unusable-output findings (the model answered but /// the output could not be validated). A malicious diff can induce this class /// via prompt injection, so it always fails the gate — even under @@ -347,6 +383,29 @@ mod tests { assert_eq!(Kind::parse("unknown"), None); } + #[test] + fn weak_human_escalation_never_blocks_but_floor_does() { + let mut escalation = finding(Severity::Error, 0.05); + escalation.kind = Kind::HumanEscalation; + let block_on_kinds = vec!["humanEscalation".to_string()]; + + assert!(!finding_blocks_gate( + &escalation, + "error", + &block_on_kinds, + false + )); + + escalation.severity = Severity::Warn; + escalation.confidence = HUMAN_ESCALATION_GATE_MIN_CONFIDENCE; + assert!(finding_blocks_gate( + &escalation, + "error", + &block_on_kinds, + false + )); + } + #[test] fn envelope_serializes_camel_case() { let env = Envelope { diff --git a/src/forge/azure.rs b/src/forge/azure.rs index 83a8d13..4dc9465 100644 --- a/src/forge/azure.rs +++ b/src/forge/azure.rs @@ -424,7 +424,7 @@ impl Forge for Azure { &head, "postil/review", map(advisory), - &check_summary(envelope, false), + &check_summary(envelope, false, Default::default()), ) .await?; let gate_desc = if envelope.gate.failing { diff --git a/src/forge/bitbucket.rs b/src/forge/bitbucket.rs index f02f81e..31f826b 100644 --- a/src/forge/bitbucket.rs +++ b/src/forge/bitbucket.rs @@ -308,7 +308,7 @@ impl Forge for Bitbucket { &head, "postil/review", map(advisory), - &check_summary(envelope, false), + &check_summary(envelope, false, Default::default()), ) .await?; let gate_desc = if envelope.gate.failing { diff --git a/src/forge/github.rs b/src/forge/github.rs index a7138a1..c1ffab2 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -4,7 +4,10 @@ use anyhow::{Context, Result, anyhow}; use serde::Deserialize; use serde_json::json; -use super::{CheckState, Forge, PrMeta, ThreadKind, check_summary, check_title, wrap_plain_text}; +use super::{ + CheckState, Forge, PrMeta, SummaryContext, ThreadKind, check_summary, check_title, + wrap_plain_text, +}; use crate::envelope::{Envelope, Finding, Severity}; use crate::filter; @@ -16,6 +19,7 @@ pub struct GitHub { owner: String, repo: String, pr: u64, + web_base: String, } impl GitHub { @@ -28,6 +32,8 @@ impl GitHub { let api_base = std::env::var("GITHUB_API_URL") .unwrap_or_else(|_| "https://api.github.com".to_string()); let details_url = valid_details_url(std::env::var("POSTIL_DETAILS_URL").ok()); + let web_base = valid_details_url(std::env::var("GITHUB_SERVER_URL").ok()) + .unwrap_or_else(|| "https://github.com".to_string()); Ok(GitHub { http: reqwest::Client::builder() .timeout(std::time::Duration::from_secs(60)) @@ -38,6 +44,7 @@ impl GitHub { owner: owner.to_string(), repo: repo.to_string(), pr, + web_base: web_base.trim_end_matches('/').to_string(), }) } @@ -89,7 +96,7 @@ fn gate_title(envelope: &Envelope) -> &'static str { fn gate_summary(envelope: &Envelope) -> String { if !envelope.gate.failing { return format!( - "Merge gate passed: no findings at or above the blocking threshold ({}).\n", + "Merge gate passed: no findings block under the configured policy (failOn: {}).\n", envelope.gate.fail_on ); } @@ -98,20 +105,28 @@ fn gate_summary(envelope: &Envelope) -> String { .findings .iter() .filter(|f| { - crate::config::GateLevel::parse(&envelope.gate.fail_on) - .map(|g| g.fails(f.severity)) - .unwrap_or(false) + crate::envelope::finding_blocks_gate( + f, + &envelope.gate.fail_on, + &envelope.gate.block_on_kinds, + false, + ) }) .map(|f| format!("- `{}:{}` {}", f.path, f.line, f.title)) .collect(); - let noun = if failing.len() == 1 { - "finding" + if failing.is_empty() { + return format!( + "Merge gate failed under the configured operational error policy (failOn: {}).\n", + envelope.gate.fail_on + ); + } + let subject = if failing.len() == 1 { + "1 finding blocks".to_string() } else { - "findings" + format!("{} findings block", failing.len()) }; format!( - "Merge gate failed: {} {noun} at or above the blocking threshold ({}).\n\n{}\n", - failing.len(), + "Merge gate failed: {subject} under the configured policy (failOn: {}).\n\n{}\n", envelope.gate.fail_on, failing.join("\n") ) @@ -140,6 +155,23 @@ impl Forge for GitHub { true } + fn review_summary(&self, envelope: &Envelope) -> String { + let commit_url = envelope.head_sha.as_deref().map(|sha| { + format!( + "{}/{}/{}/commit/{sha}", + self.web_base, self.owner, self.repo + ) + }); + check_summary( + envelope, + true, + SummaryContext { + commit_url: commit_url.as_deref(), + details_url: self.details_url.as_deref(), + }, + ) + } + async fn fetch_pr_meta(&self) -> Result { let resp = self .request( @@ -305,7 +337,7 @@ impl Forge for GitHub { let gate_note = if name == "postil/gate" { gate_summary(envelope) } else { - check_summary(envelope, true) + self.review_summary(envelope) }; let title = if name == "postil/gate" { gate_title(envelope).to_string() @@ -373,7 +405,8 @@ impl Forge for GitHub { #[cfg(test)] mod tests { - use super::valid_details_url; + use super::{gate_summary, valid_details_url}; + use crate::envelope::{Envelope, Finding, Gate, Kind, Severity, Usage}; #[test] fn details_url_accepts_only_http_and_https_urls() { @@ -390,4 +423,85 @@ mod tests { assert_eq!(valid_details_url(Some("not a URL".into())), None); assert_eq!(valid_details_url(None), None); } + + #[test] + fn gate_summary_counts_qualified_kind_blocks_from_stored_envelope() { + let finding = Finding { + path: "src/auth.rs".into(), + line: 41, + end_line: None, + severity: Severity::Warn, + kind: Kind::HumanEscalation, + confidence: 0.30, + generator_confidence: None, + scorer_confidence: None, + generator_kind: None, + scorer_kind: None, + scorer_reason: None, + title: "Human judgment required".into(), + body: "Concrete compatibility concern.".into(), + id: None, + }; + let env = Envelope { + version: 1, + summary: String::new(), + silent: false, + findings: vec![finding], + resolved: vec![], + counts: Default::default(), + confidence_buckets: [0; 5], + gate: Gate { + fail_on: "error".into(), + failing: true, + block_on_kinds: vec!["humanEscalation".into()], + }, + model_used: "m".into(), + scorer_model: None, + scorer_error: None, + scorer_disagreements: None, + usage: Usage::default(), + duration_ms: 0, + base_sha: None, + head_sha: None, + since_sha: None, + }; + + let summary = gate_summary(&env); + assert!(summary.contains("1 finding blocks under the configured policy")); + assert!(summary.contains("src/auth.rs:41")); + } + + #[test] + fn gate_summary_reports_operational_failure_without_inventing_blockers() { + let mut env = Envelope { + version: 1, + summary: String::new(), + silent: false, + findings: vec![], + resolved: vec![], + counts: Default::default(), + confidence_buckets: [0; 5], + gate: Gate { + fail_on: "never".into(), + failing: true, + block_on_kinds: vec![], + }, + model_used: "m".into(), + scorer_model: None, + scorer_error: None, + scorer_disagreements: None, + usage: Usage::default(), + duration_ms: 0, + base_sha: None, + head_sha: None, + since_sha: None, + }; + env.findings + .push(crate::envelope::provider_error_finding("timeout")); + + assert_eq!( + gate_summary(&env), + "Merge gate failed under the configured operational error policy (failOn: never).\n" + ); + } } diff --git a/src/forge/gitlab.rs b/src/forge/gitlab.rs index d668b5d..cd1f3c5 100644 --- a/src/forge/gitlab.rs +++ b/src/forge/gitlab.rs @@ -322,7 +322,7 @@ impl Forge for GitLab { &head, "postil/review", map(advisory), - &check_summary(envelope, true), + &check_summary(envelope, true, Default::default()), ) .await?; let gate_desc = if envelope.gate.failing { diff --git a/src/forge/mod.rs b/src/forge/mod.rs index 5079c3a..e57c410 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -72,6 +72,11 @@ pub trait Forge { fn rich_markdown(&self) -> bool { false } + /// Compose the top-level review body. Forges can add validated links to the + /// otherwise forge-neutral envelope metadata. + fn review_summary(&self, envelope: &Envelope) -> String { + check_summary(envelope, self.rich_markdown(), SummaryContext::default()) + } async fn fetch_pr_meta(&self) -> Result; /// Unified diff of the full PR. async fn fetch_diff(&self) -> Result; @@ -233,8 +238,19 @@ pub fn clean_review_message(envelope: &Envelope) -> &'static str { } } -pub fn check_summary(envelope: &Envelope, rich: bool) -> String { +#[derive(Default)] +pub struct SummaryContext<'a> { + pub commit_url: Option<&'a str>, + pub details_url: Option<&'a str>, +} + +pub fn check_summary(envelope: &Envelope, rich: bool, context: SummaryContext<'_>) -> String { let mut s = String::new(); + s.push_str(if envelope.gate.failing { + "Gate: failed\n\n" + } else { + "Gate: passed\n\n" + }); let pass = |s: &mut String| { if rich { s.push_str(&icon_md("pass")); @@ -256,15 +272,24 @@ pub fn check_summary(envelope: &Envelope, rich: bool) -> String { } else { String::new() }; + let escalation_note = if f.kind == crate::envelope::Kind::HumanEscalation + && f.confidence < crate::envelope::HUMAN_ESCALATION_GATE_MIN_CONFIDENCE + { + format!( + " · non-blocking below {:.2} escalation floor", + crate::envelope::HUMAN_ESCALATION_GATE_MIN_CONFIDENCE + ) + } else { + String::new() + }; s.push_str(&format!( - "- {}**{}** `{}:{}` — {} · confidence {} · kind: {}\n", + "- {}**{}** · severity: `{}` · confidence {} · kind: {}{}\n", icon, - f.severity.as_str(), - f.path, - f.line, f.title, + f.severity.as_str(), format_confidence(f.confidence), f.kind.as_str(), + escalation_note, )); } } @@ -282,10 +307,59 @@ pub fn check_summary(envelope: &Envelope, rich: bool) -> String { envelope.counts.suppressed )); } - s.push_str(&format!("\nModel: {}\n", envelope.model_used)); + if rich { + s.push_str("\n
Review metadata\n\n"); + } else { + s.push_str("\nReview metadata:\n"); + } + s.push_str(&format!("- Model: `{}`\n", envelope.model_used)); + s.push_str(&format!( + "- Review duration: {}\n", + format_duration(envelope.duration_ms) + )); + if let Some(head_sha) = envelope.head_sha.as_deref() { + let short_sha: String = head_sha.chars().take(7).collect(); + if let Some(commit_url) = context.commit_url { + s.push_str(&format!("- Commit: [`{short_sha}`]({commit_url})\n")); + } else { + s.push_str(&format!("- Commit: `{short_sha}`\n")); + } + } + if let Some(details_url) = context.details_url { + s.push_str(&format!( + "- Dashboard run: [View in Postil]({details_url})\n" + )); + } + s.push_str(&format!( + "- Tokens: {} prompt, {} completion\n", + envelope.usage.prompt_tokens, envelope.usage.completion_tokens + )); + if let Some(scorer_model) = envelope.scorer_model.as_deref() { + s.push_str(&format!("- Scorer: `{scorer_model}`")); + if let Some(disagreements) = envelope.scorer_disagreements { + s.push_str(&format!(" ({disagreements} disagreement(s))")); + } + s.push('\n'); + } + if envelope.scorer_error.is_some() { + // Provider response bodies can contain arbitrary text. Keep the public + // metadata useful without copying raw upstream errors into Markdown. + s.push_str("- Scorer status: unavailable\n"); + } + if rich { + s.push_str("\n
\n"); + } s } +fn format_duration(duration_ms: u64) -> String { + if duration_ms < 1_000 { + format!("{duration_ms} ms") + } else { + format!("{:.2} s", duration_ms as f64 / 1_000.0) + } +} + /// The body of one inline finding comment: icon (rich forges), bold title, /// severity / confidence / kind statusline, then the finding body. pub fn finding_comment_body(f: &Finding, rich: bool) -> String { @@ -342,6 +416,92 @@ mod tests { assert!(body.contains("confidence 0.91 · kind: risk")); } + #[test] + fn summary_is_explicit_path_free_and_marks_weak_escalations_non_blocking() { + let mut escalation = finding(); + escalation.kind = Kind::HumanEscalation; + escalation.confidence = 0.05; + let env = Envelope { + version: 1, + summary: "A weak signal needs review.".into(), + silent: false, + findings: vec![escalation], + resolved: vec![], + counts: Default::default(), + confidence_buckets: [1, 0, 0, 0, 0], + gate: crate::envelope::Gate { + fail_on: "error".into(), + failing: false, + block_on_kinds: vec!["humanEscalation".into()], + }, + model_used: "review-model".into(), + scorer_model: Some("scorer-model".into()), + scorer_error: None, + scorer_disagreements: Some(1), + usage: crate::envelope::Usage { + prompt_tokens: 10, + completion_tokens: 5, + }, + duration_ms: 1_250, + base_sha: None, + head_sha: Some("abcdef123456".into()), + since_sha: None, + }; + + let summary = check_summary( + &env, + true, + SummaryContext { + commit_url: Some("https://github.com/acme/api/commit/abcdef123456"), + details_url: Some("https://postil.dev/orgs/acme/runs/run-1"), + }, + ); + + assert!(summary.starts_with("Gate: passed\n\n")); + assert!(summary.contains("**Unsanitized input reaches query** · severity: `error`")); + assert!(!summary.contains("src/auth.rs:41")); + assert!(summary.contains("non-blocking below 0.30 escalation floor")); + assert!(summary.contains("
Review metadata")); + assert!(summary.contains("[`abcdef1`](https://github.com/acme/api/commit/abcdef123456)")); + assert!(summary.contains("[View in Postil](https://postil.dev/orgs/acme/runs/run-1)")); + assert!(summary.contains("- Review duration: 1.25 s")); + assert!(summary.contains("- Tokens: 10 prompt, 5 completion")); + assert!(summary.contains("- Scorer: `scorer-model` (1 disagreement(s))")); + } + + #[test] + fn plain_summary_uses_plain_metadata_and_hides_raw_scorer_errors() { + let env = Envelope { + version: 1, + summary: String::new(), + silent: true, + findings: vec![], + resolved: vec![], + counts: Default::default(), + confidence_buckets: [0; 5], + gate: crate::envelope::Gate { + fail_on: "error".into(), + failing: false, + block_on_kinds: vec![], + }, + model_used: "review-model".into(), + scorer_model: None, + scorer_error: Some("[click me](https://attacker.invalid)".into()), + scorer_disagreements: None, + usage: Default::default(), + duration_ms: 0, + base_sha: None, + head_sha: None, + since_sha: None, + }; + let summary = check_summary(&env, false, Default::default()); + + assert!(summary.contains("\nReview metadata:\n")); + assert!(!summary.contains("
")); + assert!(summary.contains("- Scorer status: unavailable")); + assert!(!summary.contains("attacker.invalid")); + } + #[test] fn plain_comment_has_statusline_without_html() { let body = finding_comment_body(&finding(), false); @@ -394,8 +554,8 @@ mod tests { head_sha: None, since_sha: None, }; - assert!(check_summary(&env, true).contains("status/pass.svg")); - assert!(!check_summary(&env, false).contains(" Result { if kept.len() > cfg.max_findings { suppressed.extend(kept.split_off(cfg.max_findings)); } - let gate_after = kept.iter().any(|f| cfg.gate_fail_on.fails(f.severity)); + let block_on_kinds: Vec = cfg + .block_on_kinds + .iter() + .map(|kind| kind.as_str().to_string()) + .collect(); + let gate_after = kept.iter().any(|finding| { + if finding.path == crate::envelope::PROVIDER_PATH { + // Provider-only pre-review failures encode their onError outcome + // only in gate.failing. Envelope v1 does not carry enough detail + // to reconstruct that decision from failOn, so preserve it rather + // than reporting a fabricated candidate-policy result. + env.gate.failing + } else { + crate::envelope::finding_blocks_gate( + finding, + cfg.gate_fail_on.as_str(), + &block_on_kinds, + false, + ) + } + }); Ok(PlanRow { name: path .file_name() @@ -209,4 +229,24 @@ mod tests { assert_eq!(rows[0].newly_suppressed.len(), 2); let _ = Counts::default(); } + + #[test] + fn replay_uses_human_escalation_confidence_floor() { + let dir = tempfile::tempdir().unwrap(); + let mut weak = f("a.rs", Severity::Error, 0.05); + weak.kind = Kind::HumanEscalation; + let env = envelope_with(vec![weak], true); + std::fs::write( + dir.path().join("r1.json"), + serde_json::to_string(&env).unwrap(), + ) + .unwrap(); + + let mut cfg = Config::default(); + cfg.min_confidence = 0.0; + let rows = run(dir.path(), &cfg).unwrap(); + + assert_eq!(rows[0].findings_after, 1); + assert!(!rows[0].gate_after); + } } diff --git a/src/review.rs b/src/review.rs index e5b05f8..5df66d8 100644 --- a/src/review.rs +++ b/src/review.rs @@ -10,8 +10,7 @@ use crate::diff::{self, DiffIndex}; use crate::envelope::{Envelope, Finding, Gate, Kind, Usage, fail_closed_finding}; use crate::filter; use crate::forge::{ - CheckState, Forge, PrMeta, azure::Azure, bitbucket::Bitbucket, check_summary, - clean_review_message, github::GitHub, gitlab::GitLab, + CheckState, Forge, PrMeta, azure::Azure, bitbucket::Bitbucket, github::GitHub, gitlab::GitLab, }; use crate::llm::{FindingScore, LlmClient}; use crate::local::{self, LocalSource}; @@ -790,10 +789,19 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> // output (OPERATIONAL_PATH) never bypasses the gate: a malicious diff can // induce it via prompt injection. let advisory_on_error = cfg.gate_on_error == OnError::Advisory; + let gate_fail_on = cfg.gate_fail_on.as_str(); + let gate_block_on_kinds: Vec = cfg + .block_on_kinds + .iter() + .map(|kind| kind.as_str().to_string()) + .collect(); let gate_failing = findings.iter().any(|f| { - (cfg.gate_fail_on.fails(f.severity) - && !(advisory_on_error && f.path == crate::envelope::PROVIDER_PATH)) - || cfg.block_on_kinds.contains(&f.kind) + crate::envelope::finding_blocks_gate( + f, + gate_fail_on, + &gate_block_on_kinds, + advisory_on_error && f.path == crate::envelope::PROVIDER_PATH, + ) }); let silent = findings.is_empty(); let mut counts = Envelope::counts_of(&findings, suppressed); @@ -814,11 +822,7 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> gate: Gate { fail_on: cfg.gate_fail_on.as_str().to_string(), failing: gate_failing, - block_on_kinds: cfg - .block_on_kinds - .iter() - .map(|k| k.as_str().to_string()) - .collect(), + block_on_kinds: gate_block_on_kinds, }, model_used, scorer_model, @@ -864,17 +868,7 @@ async fn finish( || matches!(cfg.on_clean, crate::config::OnClean::Comment)) && !duplicate_of_baseline; if should_comment { - let rich = forge.rich_markdown(); - let summary = if envelope.silent { - let icon = if rich { - format!("{} ", crate::forge::icon_md("pass")) - } else { - String::new() - }; - format!("{icon}{}", clean_review_message(&envelope)) - } else { - check_summary(&envelope, rich) - }; + let summary = forge.review_summary(&envelope); let head = envelope.head_sha.clone().unwrap_or_default(); // A posting failure here (rate limit, transient 5xx, network blip) // must not discard a review that already computed and persisted its diff --git a/tests/e2e.rs b/tests/e2e.rs index 5fc54e2..3639f69 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -109,6 +109,7 @@ fn postil() -> Command { .env_remove("POSTIL_API_KEY") .env_remove("POSTIL_API_BASE") .env_remove("POSTIL_DETAILS_URL") + .env_remove("GITHUB_SERVER_URL") .env_remove("POSTIL_ENABLE_BITBUCKET_INCREMENTAL") .env("MODEL_API_KEY", "test-key"); cmd @@ -195,6 +196,59 @@ async fn local_review_reports_grounded_finding_and_gates() { assert_eq!(request["messages"].as_array().unwrap().len(), 2); } +#[tokio::test] +async fn weak_human_escalation_remains_visible_without_blocking_gate() { + let server = MockServer::start().await; + mock_review( + &server, + json!([finding_at_with_kind(41, "error", "humanEscalation", 0.05)]), + ) + .await; + + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join(".postil.yaml"), "minConfidence: 0\n").unwrap(); + let diff = write_diff(dir.path()); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json"]) + .assert() + .code(0); + let env: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); + + assert_eq!(env["findings"][0]["kind"], "humanEscalation"); + assert_eq!(env["findings"][0]["confidence"], 0.05); + assert_eq!(env["gate"]["failing"], false); +} + +#[tokio::test] +async fn human_escalation_at_floor_blocks_even_at_warn_severity() { + let server = MockServer::start().await; + mock_review( + &server, + json!([finding_at_with_kind(41, "warn", "humanEscalation", 0.30)]), + ) + .await; + + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join(".postil.yaml"), "minConfidence: 0\n").unwrap(); + let diff = write_diff(dir.path()); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json"]) + .assert() + .code(1); + let env: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); + + assert_eq!(env["findings"][0]["severity"], "warn"); + assert_eq!(env["gate"]["failing"], true); +} + #[tokio::test] async fn scorer_lowers_confidence_and_stores_both_values() { let server = MockServer::start().await; @@ -1675,7 +1729,7 @@ async fn github_flow_posts_review_and_completes_both_checks() { assert_eq!(gate_patch["output"]["title"], "Merge gate failed"); assert_eq!( gate_patch["output"]["summary"], - "Merge gate failed: 1 finding at or above the blocking threshold (error).\n\n- `src/auth.rs:41` Unsanitized input reaches query\n" + "Merge gate failed: 1 finding blocks under the configured policy (failOn: error).\n\n- `src/auth.rs:41` Unsanitized input reaches query\n" ); // Inline review posted with the finding at the cited line. let review = reqs @@ -1685,6 +1739,20 @@ async fn github_flow_posts_review_and_completes_both_checks() { let body: Value = review.body_json().unwrap(); assert_eq!(body["comments"][0]["path"], "src/auth.rs"); assert_eq!(body["comments"][0]["line"], 41); + let summary = body["body"].as_str().unwrap(); + assert!(summary.starts_with("Gate: failed\n\n")); + assert!(summary.contains("**Unsanitized input reaches query** · severity: `error`")); + assert!(!summary.contains("`src/auth.rs:41`")); + assert!(summary.contains("
Review metadata")); + assert!( + summary.contains("- Commit: [`headsha`](https://github.com/acme/api/commit/headsha111)") + ); + assert!( + summary.contains( + "- Dashboard run: [View in Postil](https://postil.dev/orgs/acme/runs/review-7)" + ) + ); + assert!(summary.contains("- Tokens: 300 prompt, 150 completion")); } // An LLM response with a caller-provided summary and findings (used for @@ -1802,7 +1870,7 @@ async fn content_policy_pr_body_finding_survives_grounding() { body["body"] .as_str() .unwrap() - .contains(".postil/pr-description") + .contains("AI-authorship residue in PR description") ); } @@ -1910,8 +1978,45 @@ async fn github_clean_pr_stays_silent_but_completes_checks() { assert_eq!(gate_patch["output"]["title"], "Merge gate passed"); assert_eq!( gate_patch["output"]["summary"], - "Merge gate passed: no findings at or above the blocking threshold (error).\n" + "Merge gate passed: no findings block under the configured policy (failOn: error).\n" ); + + // The explicit onClean mode uses the same unified summary as finding-bearing + // reviews instead of falling back to the former one-line clean message. + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + std::fs::write( + dir.path().join(".postil.yaml"), + "review:\n onClean: comment\n", + ) + .unwrap(); + postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .env( + "POSTIL_DETAILS_URL", + "https://postil.dev/orgs/acme/runs/clean-7", + ) + .args(["review", "--repo", "acme/api", "--pr", "7"]) + .assert() + .code(0); + let reqs = server.received_requests().await.unwrap(); + let clean_review = reqs + .iter() + .rev() + .find(|request| request.url.path().ends_with("/reviews")) + .expect("onClean review posted"); + let clean_body: Value = clean_review.body_json().unwrap(); + let clean_summary = clean_body["body"].as_str().unwrap(); + assert!(clean_summary.starts_with("Gate: passed\n\n")); + assert!(clean_summary.contains("Postil reviewed this change and found nothing")); + assert!(clean_summary.contains("
Review metadata")); + assert!(clean_summary.contains("[View in Postil](https://postil.dev/orgs/acme/runs/clean-7)")); } #[tokio::test]