From 919c423564fc6e76161a43487aa6dd63f83ec32c Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Fri, 12 Jun 2026 18:32:51 +0000 Subject: [PATCH 1/2] Fail closed on narrated risk and record review telemetry A summary that describes merge-relevant risk alongside an empty findings array previously produced a clean, silent review - the predecessor product posted exactly this contradiction on 9 of its last 16 'clean' reviews. The contract now requires summary and findings to agree: the prompt states it, the client gives the model one corrective retry to either structure the risk or retract it, and a surviving contradiction fails closed with a synthetic finding that preserves the narrated text. The envelope additionally records the ungrounded-finding count and the engine wall-clock duration for quality telemetry. --- src/envelope.rs | 35 ++++++++++++++++++ src/llm.rs | 41 ++++++++++++++++++--- src/plan.rs | 1 + src/prompt.rs | 7 +++- src/review.rs | 17 ++++++++- src/sarif.rs | 1 + tests/e2e.rs | 97 ++++++++++++++++++++++++++++++++++++++++++++++++- 7 files changed, 191 insertions(+), 8 deletions(-) diff --git a/src/envelope.rs b/src/envelope.rs index a66ccdb..a9f4416 100644 --- a/src/envelope.rs +++ b/src/envelope.rs @@ -73,6 +73,10 @@ pub struct Counts { pub warn: u32, pub error: u32, pub suppressed: u32, + /// Findings the model reported that did not cite a changed line and were + /// dropped. Nonzero values are a model-quality signal worth tracking. + #[serde(default)] + pub ungrounded: u32, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -104,6 +108,9 @@ pub struct Envelope { pub gate: Gate, pub model_used: String, pub usage: Usage, + /// Wall-clock duration of the review engine run in milliseconds. + #[serde(default)] + pub duration_ms: u64, pub base_sha: Option, pub head_sha: Option, pub since_sha: Option, @@ -164,6 +171,33 @@ pub fn fail_closed_finding(detail: &str) -> Finding { } } +/// The synthetic finding emitted when the model narrated merge-relevant risk +/// in its summary while reporting zero structured findings. The contradiction +/// means the output cannot be trusted as a pass; the narration is preserved so +/// the concern is not silently dropped. Uses OPERATIONAL_PATH: a malicious +/// diff can induce this shape via prompt injection, so it never bypasses the +/// gate. +pub fn narrated_risk_finding(summary: &str) -> Finding { + let quoted: String = summary.lines().map(|l| format!("> {l}\n")).collect(); + Finding { + path: OPERATIONAL_PATH.to_string(), + line: 1, + end_line: None, + severity: Severity::Error, + kind: Kind::Uncertainty, + confidence: 1.0, + title: "Model narrated risk without structured findings".to_string(), + body: format!( + "The model's summary describes merge-relevant risk but it reported no \ + structured findings, so the review cannot be trusted as a pass. Postil is \ + failing closed instead of posting a clean status above contradictory prose.\n\n\ + Narrated summary:\n\n{quoted}\n\ + Re-run the review; if the contradiction persists, inspect the areas the \ + summary names and address them or record findings manually." + ), + } +} + /// The synthetic finding emitted when the provider could not be reached at all. pub fn provider_error_finding(detail: &str) -> Finding { Finding { @@ -236,6 +270,7 @@ mod tests { }, model_used: "m".into(), usage: Usage::default(), + duration_ms: 0, base_sha: None, head_sha: None, since_sha: None, diff --git a/src/llm.rs b/src/llm.rs index ce95e09..6c1de91 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -180,8 +180,8 @@ impl LlmClient { ) -> Result { let mut usage = Usage::default(); let content = self.chat(model, system, user, &mut usage).await?; - match parse_review(&content) { - Ok(raw) => Ok(into_review(raw, model, usage)), + let raw = match parse_review(&content) { + Ok(raw) => raw, Err(parse_err) => { // One repair attempt: ask the same model to fix its own JSON. let repair_user = format!( @@ -198,11 +198,42 @@ impl LlmClient { ) .await .context("JSON repair call failed")?; - let raw = parse_review(&repaired) - .map_err(|e| anyhow!("model output invalid after repair: {e}"))?; - Ok(into_review(raw, model, usage)) + parse_review(&repaired) + .map_err(|e| anyhow!("model output invalid after repair: {e}"))? + } + }; + let mut review = into_review(raw, model, usage); + + // Semantic consistency retry: a summary that narrates risk next to an + // empty findings array is the contract violation behind "clean status, + // scary prose" reviews. Give the model one chance to either structure + // the risk or retract it; if the contradiction survives, the caller + // fails the review closed. + if review.findings.is_empty() && !review.summary.is_empty() { + let retry_user = format!( + "{user}\n\n[Your previous response]\n{content}\n\n[Correction] Your summary \ + describes merge-relevant risk but `findings` is empty, which is invalid. \ + Either report each risk as a structured finding citing its exact new-file \ + line from the diff above, or — if nothing is actually merge-relevant — \ + return exactly {{\"summary\": \"\", \"findings\": []}}." + ); + let mut retry_usage = usage; + if let Ok(retried) = self + .chat(model, system, &retry_user, &mut retry_usage) + .await + && let Ok(retried_raw) = parse_review(&retried) + { + let candidate = into_review(retried_raw, model, retry_usage); + let still_contradictory = + candidate.findings.is_empty() && !candidate.summary.is_empty(); + if still_contradictory { + review.usage = retry_usage; + } else { + review = candidate; + } } } + Ok(review) } async fn chat( diff --git a/src/plan.rs b/src/plan.rs index 17df403..7daa10b 100644 --- a/src/plan.rs +++ b/src/plan.rs @@ -151,6 +151,7 @@ mod tests { }, model_used: "m".into(), usage: Usage::default(), + duration_ms: 0, base_sha: None, head_sha: None, since_sha: None, diff --git a/src/prompt.rs b/src/prompt.rs index b25837e..2a79a90 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -80,7 +80,12 @@ pub fn system_prompt(cfg: &Config) -> String { \"findings\": [{\"path\": \"file path from the diff\", \"line\": ,\n \ \"endLine\": , \"severity\": \"info|warn|error\",\n \ \"kind\": \"risk|humanEscalation|guardrail|uncertainty\", \"confidence\": <0..1>,\n \ - \"title\": \"short imperative title\", \"body\": \"specific, evidence-based markdown\"}]}\n", + \"title\": \"short imperative title\", \"body\": \"specific, evidence-based markdown\"}]}\n\ + \n\ + The summary and findings must agree. Every risk the summary mentions MUST appear as \ + a structured finding with its diff line; if findings is empty, summary MUST be the \ + empty string. A summary that narrates problems alongside an empty findings array is \ + invalid output and will fail the review.\n", ); p } diff --git a/src/review.rs b/src/review.rs index 6e67dd1..0c99d1a 100644 --- a/src/review.rs +++ b/src/review.rs @@ -234,6 +234,7 @@ async fn review_diff( None => Vec::new(), }; + let review_started = std::time::Instant::now(); let parsed = diff::parse(diff_text); let index = DiffIndex::build(&parsed); let incremental = args.since_sha.is_some(); @@ -242,6 +243,7 @@ async fn review_diff( let mut model_used = "none (empty diff)".to_string(); let mut usage = Usage::default(); let mut suppressed = 0u32; + let mut ungrounded = 0u32; let mut findings: Vec = Vec::new(); if !cfg.enabled { @@ -265,15 +267,25 @@ async fn review_diff( let client = LlmClient::from_env(cfg)?; match client.review(cfg, &system, &user).await { Ok(model_review) => { + let raw_findings = model_review.findings.len(); let outcome = filter::apply(cfg, &index, model_review.findings)?; model_used = model_review.model_used; usage = model_review.usage; suppressed = outcome.suppressed; + ungrounded = outcome.ungrounded; if outcome.all_ungrounded { findings = vec![fail_closed_finding(&format!( "model reported {} finding(s), none grounded in the diff", outcome.ungrounded ))]; + } else if raw_findings == 0 && !model_review.summary.trim().is_empty() { + // Risk narrated in prose with zero structured findings + // (post-retry). Passing this as clean is the predecessor + // product's worst failure mode; fail closed instead and + // carry the narration into the finding so it is not lost. + findings = vec![crate::envelope::narrated_risk_finding( + &model_review.summary, + )]; } else { summary = model_review.summary; findings = outcome.kept; @@ -329,7 +341,8 @@ async fn review_diff( && !(advisory_on_error && f.path == crate::envelope::PROVIDER_PATH) }); let silent = findings.is_empty(); - let counts = Envelope::counts_of(&findings, suppressed); + let mut counts = Envelope::counts_of(&findings, suppressed); + counts.ungrounded = ungrounded; let buckets = Envelope::buckets_of(&findings); Ok(Envelope { @@ -346,6 +359,7 @@ async fn review_diff( }, model_used, usage, + duration_ms: review_started.elapsed().as_millis() as u64, base_sha: meta.map(|m| m.base_sha.clone()), head_sha, since_sha: args.since_sha.clone(), @@ -424,6 +438,7 @@ fn error_envelope(cfg: &Config, err: &anyhow::Error, head_sha: &str, meta: &PrMe }, model_used: cfg.model_chain().join(" -> "), usage: Usage::default(), + duration_ms: 0, base_sha: Some(meta.base_sha.clone()), head_sha: Some(head_sha.to_string()), since_sha: None, diff --git a/src/sarif.rs b/src/sarif.rs index ef492f7..bbec822 100644 --- a/src/sarif.rs +++ b/src/sarif.rs @@ -126,6 +126,7 @@ mod tests { }, model_used: "m".into(), usage: Usage::default(), + duration_ms: 0, base_sha: None, head_sha: None, since_sha: None, diff --git a/tests/e2e.rs b/tests/e2e.rs index 5d31b0d..1774fa8 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -17,15 +17,32 @@ diff --git a/src/auth.rs b/src/auth.rs "; fn llm_content(findings: Value) -> Value { + // The contract requires summary and findings to agree: an empty findings + // array must come with an empty summary. + let summary = if findings.as_array().is_none_or(|a| a.is_empty()) { + "" + } else { + "SQL injection risk in auth path." + }; json!({ "choices": [{"message": {"content": json!({ - "summary": "SQL injection risk in auth path.", + "summary": summary, "findings": findings }).to_string()}}], "usage": {"prompt_tokens": 100, "completion_tokens": 50} }) } +fn llm_contradictory() -> Value { + json!({ + "choices": [{"message": {"content": json!({ + "summary": "SQL injection risk in auth path.", + "findings": [] + }).to_string()}}], + "usage": {"prompt_tokens": 100, "completion_tokens": 50} + }) +} + fn llm_text(text: &str) -> Value { json!({ "choices": [{"message": {"content": text}}], @@ -290,6 +307,84 @@ async fn clean_diff_is_silent_and_exits_zero() { assert_eq!(env["gate"]["failing"], false); } +#[tokio::test] +async fn narrated_risk_without_findings_fails_closed() { + // The predecessor product's worst failure mode: risk prose in the summary, + // zero structured findings, green status. The model gets one corrective + // retry; if the contradiction persists the review fails closed. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_contradictory())) + .expect(2) // initial call + one semantic retry + .mount(&server) + .await; + + let dir = tempfile::tempdir().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) + .arg("--output-json") + .assert() + .code(1); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["silent"], false); + assert_eq!(env["gate"]["failing"], true); + assert_eq!(env["findings"][0]["path"], ".postil/model-output"); + assert_eq!( + env["findings"][0]["title"], + "Model narrated risk without structured findings" + ); + // The narrated concern is preserved, not silently dropped. + assert!( + env["findings"][0]["body"] + .as_str() + .unwrap() + .contains("SQL injection risk in auth path.") + ); +} + +#[tokio::test] +async fn narrated_risk_retry_recovers_structured_finding() { + // First response contradicts itself; the corrective retry returns a + // properly structured finding, which is used as the review result. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_contradictory())) + .up_to_n_times(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(llm_content(json!([finding_at(41, "error", 0.92)]))), + ) + .mount(&server) + .await; + + let dir = tempfile::tempdir().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) + .arg("--output-json") + .assert() + .code(1); + let env: Value = + serde_json::from_str(&String::from_utf8(out.get_output().stdout.clone()).unwrap()).unwrap(); + assert_eq!(env["findings"][0]["path"], "src/auth.rs"); + assert_eq!(env["findings"][0]["line"], 41); + assert_eq!(env["counts"]["error"], 1); +} + #[tokio::test] async fn low_confidence_findings_are_suppressed() { let server = MockServer::start().await; From 8f03fd6c520592d591637fe4a1bc4343aad695db Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Fri, 12 Jun 2026 18:33:01 +0000 Subject: [PATCH 2/2] Render the product statusline in PR comments and TTY output PR comments and check summaries now carry the brand status icons (postil.dev/status/*.svg) with the 'confidence 0.91 - kind: risk' metadata line the product page shows, and confidence renders as a bare decimal instead of a percentage. Terminal output gains the matching glyphs, per-finding confidence/kind trailer, a findings/suppressed tally, and the promised lowercase 'gate: failing (fail-on: error)' line. --- src/forge/github.rs | 4 +++- src/forge/mod.rs | 45 ++++++++++++++++++++++++++++++++++++--------- src/output.rs | 39 ++++++++++++++++++++++++--------------- 3 files changed, 63 insertions(+), 25 deletions(-) diff --git a/src/forge/github.rs b/src/forge/github.rs index afc2630..0947f95 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -162,10 +162,12 @@ impl Forge for GitHub { "line": f.line, "side": "RIGHT", "body": format!( - "**{}** ({} / {} confidence)\n\n{}", + "{} **{}**\n`{}` · confidence {} · kind: {}\n\n{}", + super::severity_icon(f.severity), f.title, f.severity.as_str(), super::format_confidence(f.confidence), + f.kind.as_str(), f.body ), }); diff --git a/src/forge/mod.rs b/src/forge/mod.rs index 516b6f0..cf84771 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -10,7 +10,28 @@ pub mod gitlab; use anyhow::Result; -use crate::envelope::{Envelope, Finding}; +use crate::envelope::{Envelope, Finding, Severity}; + +/// Base URL for the brand status icons rendered in PR comments and check +/// summaries. The four icons (error, warn, info, pass) are served by the +/// marketing site and mirror the product-page statusline. +pub const STATUS_ICON_BASE: &str = "https://postil.dev/status"; + +/// Markdown `` for a named status icon, sized to sit inline with text. +pub fn icon_md(name: &str) -> String { + format!( + "\"{name}\"" + ) +} + +pub fn severity_icon(severity: Severity) -> String { + icon_md(match severity { + Severity::Error => "error", + Severity::Warn => "warn", + Severity::Info => "info", + }) +} #[derive(Debug, Clone)] pub struct PrMeta { @@ -69,9 +90,11 @@ pub fn check_title(envelope: &Envelope) -> String { pub fn check_summary(envelope: &Envelope) -> String { let mut s = String::new(); if envelope.silent { - s.push_str( - "Postil reviewed this change and found nothing that affects the merge decision.\n", - ); + s.push_str(&format!( + "{} Postil reviewed this change and found nothing that affects the merge \ + decision.\n", + icon_md("pass") + )); } else { if !envelope.summary.is_empty() { s.push_str(&envelope.summary); @@ -79,19 +102,21 @@ pub fn check_summary(envelope: &Envelope) -> String { } for f in &envelope.findings { s.push_str(&format!( - "- **{}** `{}:{}` [{}/{}] {}\n", + "- {} **{}** `{}:{}` — {} · confidence {} · kind: {}\n", + severity_icon(f.severity), f.severity.as_str(), f.path, f.line, - f.kind.as_str(), + f.title, format_confidence(f.confidence), - f.title + f.kind.as_str(), )); } } if !envelope.resolved.is_empty() { s.push_str(&format!( - "\n{} finding(s) from the previous review resolved.\n", + "\n{} {} finding(s) from the previous review resolved.\n", + icon_md("pass"), envelope.resolved.len() )); } @@ -105,6 +130,8 @@ pub fn check_summary(envelope: &Envelope) -> String { s } +/// Confidence rendered as the product statusline shows it: a bare decimal +/// probability ("0.91"), not a percentage. pub fn format_confidence(c: f64) -> String { - format!("{:.0}%", c * 100.0) + format!("{:.2}", c) } diff --git a/src/output.rs b/src/output.rs index 27e22e7..c3215ca 100644 --- a/src/output.rs +++ b/src/output.rs @@ -19,7 +19,7 @@ pub fn print_pretty(envelope: &Envelope) { if envelope.silent { out.push_str(&paint( color, - "postil: no merge-relevant findings. Staying silent.\n", + "✓ postil: no merge-relevant findings. Staying silent.\n", Paint::Green, )); } else { @@ -28,46 +28,55 @@ pub fn print_pretty(envelope: &Envelope) { } for f in &envelope.findings { let (glyph, p) = match f.severity { - Severity::Error => ("error", Paint::Red), - Severity::Warn => ("warn ", Paint::Yellow), - Severity::Info => ("info ", Paint::Blue), + Severity::Error => ("✕ error", Paint::Red), + Severity::Warn => ("▲ warn ", Paint::Yellow), + Severity::Info => ("ℹ info ", Paint::Blue), }; out.push_str(&format!( - "{} {}:{} {}\n", + "{} {}:{}\n", paint(color, glyph, p), f.path, - f.line, - f.title + f.line )); + out.push_str(&format!(" {}\n", f.title)); for line in f.body.lines() { - out.push_str(&format!(" {line}\n")); + out.push_str(&format!(" {line}\n")); } out.push_str(&format!( - " kind: {} confidence: {:.0}%\n\n", - f.kind.as_str(), - f.confidence * 100.0 + " (confidence {:.2}, kind: {})\n\n", + f.confidence, + f.kind.as_str() )); } + let n = envelope.findings.len(); + let mut tally = format!("{n} finding{}", if n == 1 { "" } else { "s" }); + if envelope.counts.suppressed > 0 { + tally.push_str(&format!( + " · {} suppressed by policy", + envelope.counts.suppressed + )); + } + out.push_str(&format!("{tally}\n")); } if !envelope.resolved.is_empty() { out.push_str(&format!( - "{} finding(s) from the previous review resolved.\n", + "✓ {} finding(s) from the previous review resolved.\n", envelope.resolved.len() )); } - if envelope.counts.suppressed > 0 { + if envelope.silent && envelope.counts.suppressed > 0 { out.push_str(&format!( "{} finding(s) suppressed by policy.\n", envelope.counts.suppressed )); } let gate = if envelope.gate.failing { - paint(color, "gate: FAILING", Paint::Red) + paint(color, "gate: failing", Paint::Red) } else { paint(color, "gate: passing", Paint::Green) }; out.push_str(&format!( - "{gate} (failOn: {}) model: {}\n", + "{gate} (fail-on: {}) model: {}\n", envelope.gate.fail_on, envelope.model_used )); eprint!("{out}");