From 5edb2b346015094cb28fc5ad0b6dd39f9b89c44a Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Mon, 20 Jul 2026 09:05:25 +0000 Subject: [PATCH 1/2] Report safe model-output validation categories --- src/llm.rs | 168 +++++++++++++++++++++++++++++++++++++++++++++----- src/review.rs | 144 ++++++++++++++++++++++++++++++------------- tests/e2e.rs | 6 ++ 3 files changed, 259 insertions(+), 59 deletions(-) diff --git a/src/llm.rs b/src/llm.rs index 83a4572..3751267 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -433,6 +433,33 @@ struct RawReview { findings: Vec, } +/// Caller-owned semantic validation keeps the detailed correction prompt +/// separate from the bounded diagnostic that can reach logs and run reports. +/// The safe detail must contain categories and counts only, never model prose, +/// repository paths, line text, or copied evidence. +#[derive(Debug, Clone)] +pub(crate) struct ReviewValidationFailure { + repair_detail: String, + safe_detail: String, +} + +impl ReviewValidationFailure { + pub(crate) fn new(repair_detail: String, safe_detail: String) -> Self { + Self { + repair_detail, + safe_detail, + } + } + + pub(crate) fn repair_detail(&self) -> &str { + &self.repair_detail + } + + pub(crate) fn safe_detail(&self) -> &str { + &self.safe_detail + } +} + #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] struct RawFinding { @@ -1634,7 +1661,7 @@ impl LlmClient { /// Run a review while treating caller-rejected content as invalid model /// output. Each model gets one bounded semantic correction before the /// configured cascade advances, and every consumed call remains in usage. - pub async fn review_validated( + pub(crate) async fn review_validated( &self, cfg: &Config, system: &str, @@ -1642,7 +1669,10 @@ impl LlmClient { validate: F, ) -> std::result::Result where - F: Fn(&ModelReview) -> std::result::Result<(), String> + Send + Sync + 'static, + F: Fn(&ModelReview) -> std::result::Result<(), ReviewValidationFailure> + + Send + + Sync + + 'static, { let validate = Arc::new(validate); let chain = cfg.model_chain(); @@ -2171,7 +2201,9 @@ impl LlmClient { model: &str, system: &str, user: &str, - validate: &(dyn Fn(&ModelReview) -> std::result::Result<(), String> + Send + Sync), + validate: &( + dyn Fn(&ModelReview) -> std::result::Result<(), ReviewValidationFailure> + Send + Sync + ), ) -> std::result::Result { let mut usage = Usage::default(); let mut call_usage = Vec::new(); @@ -2205,6 +2237,11 @@ impl LlmClient { let raw = match parsed_initial { Ok(raw) => raw, Err(parse_err) => { + eprintln!( + "postil: model {} review JSON validation failed shape={}", + log_text(model), + safe_review_output_shape(&content), + ); let incident = ModelIncident { phase: ModelIncidentPhase::Review, category: ModelIncidentCategory::InvalidOutput, @@ -2242,6 +2279,11 @@ impl LlmClient { } }; let parsed = parse_review(&repaired).map_err(|error| { + eprintln!( + "postil: model {} review JSON repair remained invalid shape={}", + log_text(model), + safe_review_output_shape(&repaired), + ); let mut error = ModelError::new( anyhow!("model output invalid after repair: {error}"), usage, @@ -2357,10 +2399,18 @@ impl LlmClient { } } - if let Err(reason) = validate(&review) { + if let Err(validation_failure) = validate(&review) { + eprintln!( + "postil: model {} output validation failed categories={}", + log_text(model), + validation_failure.safe_detail(), + ); if correction_used { let mut error = ModelError::new( - anyhow!("model output remained unusable after its correction call"), + anyhow!( + "model output remained unusable after its correction call (validation categories: {})", + validation_failure.safe_detail() + ), review.usage, review.usage_accounting_complete, ); @@ -2373,7 +2423,8 @@ impl LlmClient { log_text(model), ); let previous = truncate_utf8_bytes(&content, 16_384); - let retry_user = review_validation_retry_user(user, previous, &reason); + let retry_user = + review_validation_retry_user(user, previous, validation_failure.repair_detail()); let mut retry_usage = review.usage; let mut retry_accounting_complete = review.usage_accounting_complete; let retry = self @@ -2389,6 +2440,7 @@ impl LlmClient { LlmCallPhase::SemanticRetry, ) .await; + let mut retry_safe_detail = "invalidJson=1".to_string(); match retry { Ok(content) => match parse_review(&content) { Ok(raw) => { @@ -2406,17 +2458,21 @@ impl LlmClient { }); return Ok(candidate); } - Err(retry_reason) => eprintln!( - "postil: model {} semantic retry remained unusable: {}", - log_text(model), - log_text(&retry_reason), - ), + Err(retry_failure) => { + retry_safe_detail = retry_failure.safe_detail().to_string(); + eprintln!( + "postil: model {} semantic retry remained unusable categories={}", + log_text(model), + retry_failure.safe_detail(), + ); + } } } Err(parse_error) => eprintln!( - "postil: model {} semantic retry returned invalid JSON: {}", + "postil: model {} semantic retry returned invalid JSON: {} shape={}", log_text(model), log_text(&parse_error), + safe_review_output_shape(&content), ), }, Err(error) => { @@ -2438,7 +2494,9 @@ impl LlmClient { } let mut error = ModelError::new( - anyhow!("model output remained unusable after semantic retry"), + anyhow!( + "model output remained unusable after semantic retry (validation categories: {retry_safe_detail})" + ), retry_usage, retry_accounting_complete, ); @@ -4079,6 +4137,60 @@ fn parse_review(content: &str) -> Result { serde_json::from_str::(json_str).map_err(|e| e.to_string()) } +fn safe_review_output_shape(content: &str) -> String { + fn value_kind(value: Option<&serde_json::Value>) -> &'static str { + match value { + None => "missing", + Some(serde_json::Value::Null) => "null", + Some(serde_json::Value::Bool(_)) => "boolean", + Some(serde_json::Value::Number(_)) => "number", + Some(serde_json::Value::String(_)) => "string", + Some(serde_json::Value::Array(_)) => "array", + Some(serde_json::Value::Object(_)) => "object", + } + } + + let Some(json) = extract_json_object(content) else { + let status = if content.contains('{') { + "incomplete" + } else { + "missing" + }; + return format!("bytes={} jsonObject={status}", content.len()); + }; + let Ok(value) = serde_json::from_str::(json) else { + return format!("bytes={} jsonObject=malformed", content.len()); + }; + let summary = value_kind(value.get("summary")); + let findings = value.get("findings"); + let finding_count = findings + .and_then(serde_json::Value::as_array) + .map_or_else(|| "unknown".to_string(), |items| items.len().to_string()); + let invalid_finding_shapes = findings + .and_then(serde_json::Value::as_array) + .map(|items| { + items + .iter() + .filter(|item| { + let Some(item) = item.as_object() else { + return true; + }; + !matches!(item.get("path"), Some(serde_json::Value::String(_))) + || !matches!(item.get("line"), Some(serde_json::Value::Number(_))) + || !matches!(item.get("severity"), Some(serde_json::Value::String(_))) + || !matches!(item.get("body"), Some(serde_json::Value::String(_))) + }) + .count() + }) + .unwrap_or_default(); + format!( + "bytes={} jsonObject=complete summary={summary} findings={}:{} invalidFindingShapes={invalid_finding_shapes}", + content.len(), + value_kind(findings), + finding_count, + ) +} + fn parse_scores(content: &str, expected_len: usize) -> Result, String> { let json_str = extract_json_array(content).ok_or("no JSON array found")?; let raw = serde_json::from_str::>(json_str).map_err(|e| e.to_string())?; @@ -5027,18 +5139,42 @@ mod tests { .unwrap(); *client.http.lock().unwrap() = Some(reqwest::Client::new()); - let result = client + let error = client .review_validated(&config, "system", "user", |review| { review .findings .iter() .try_for_each(crate::envelope::validate_finding_publication) + .map_err(|reason| { + ReviewValidationFailure::new(reason, "publicationContract=1".to_string()) + }) }) - .await; - assert!(result.is_err()); + .await + .unwrap_err(); + let detail = format!("{error:#}"); + assert!(detail.contains("validation categories: publicationContract=1")); + assert!(!detail.contains("word word")); assert_eq!(server.received_requests().await.unwrap().len(), 2); } + #[test] + fn review_output_shape_reports_structure_without_model_text() { + let missing = safe_review_output_shape("model prose without JSON"); + assert_eq!(missing, "bytes=24 jsonObject=missing"); + + let malformed = safe_review_output_shape(r#"prefix {"summary":"private text""#); + assert!(malformed.contains("jsonObject=incomplete")); + assert!(!malformed.contains("private text")); + + let wrong_shape = safe_review_output_shape( + r#"{"summary":false,"findings":[{"path":"private.rs","line":"7","severity":"warn"}]}"#, + ); + assert!(wrong_shape.contains("summary=boolean")); + assert!(wrong_shape.contains("findings=array:1")); + assert!(wrong_shape.contains("invalidFindingShapes=1")); + assert!(!wrong_shape.contains("private.rs")); + } + #[tokio::test] async fn planner_model_cascade_marks_fallback_and_preserves_incidents() { let server = MockServer::start().await; diff --git a/src/review.rs b/src/review.rs index f7f9706..dbc5021 100644 --- a/src/review.rs +++ b/src/review.rs @@ -15,7 +15,7 @@ use crate::filter; use crate::forge::{ CheckState, Forge, PrMeta, azure::Azure, bitbucket::Bitbucket, github::GitHub, gitlab::GitLab, }; -use crate::llm::{FindingScore, LlmClient, add_usage}; +use crate::llm::{FindingScore, LlmClient, ReviewValidationFailure, add_usage}; use crate::local::{self, LocalSource}; use crate::output::{self, OutputFormat}; use crate::prompt::{self, PrContext}; @@ -89,16 +89,25 @@ fn conservative_context_tokens(model: &str) -> usize { } } +#[derive(Debug, Clone, PartialEq, Eq)] +struct ReviewBatchValidationReason { + category: &'static str, + repair_detail: String, +} + fn review_batch_validation_reason( finding: &Finding, annotated: &str, content_policy_prompt: Option<&str>, -) -> Option { +) -> Option { if let Err(reason) = crate::envelope::validate_finding_publication(finding) { - return Some(format!( - "finding at {}:{} violates the publication contract: {reason}", - finding.path, finding.line - )); + return Some(ReviewBatchValidationReason { + category: "publicationContract", + repair_detail: format!( + "finding at {}:{} violates the publication contract: {reason}", + finding.path, finding.line + ), + }); } if diff::review_batch_contains_exact_evidence( @@ -127,63 +136,98 @@ fn review_batch_validation_reason( }); let Some(evidence_source) = evidence_source else { - return Some(format!( - "finding at {}:{} does not cite a non-empty new-side line displayed in this review input; retract it or cite a displayed new-side line", - finding.path, finding.line - )); + return Some(ReviewBatchValidationReason { + category: "missingEvidenceAnchor", + repair_detail: format!( + "finding at {}:{} does not cite a non-empty new-side line displayed in this review input; retract it or cite a displayed new-side line", + finding.path, finding.line + ), + }); }; let Some(expected) = diff::review_batch_expected_evidence(evidence_source, &finding.path, finding.line) else { - return Some(format!( - "finding at {}:{} has multiple displayed new-side evidence strings; copy the exact supporting string or retract it", - finding.path, finding.line - )); + return Some(ReviewBatchValidationReason { + category: "ambiguousEvidence", + repair_detail: format!( + "finding at {}:{} has multiple displayed new-side evidence strings; copy the exact supporting string or retract it", + finding.path, finding.line + ), + }); }; - Some(format!( - "finding at {}:{} must set `evidence` to the exact JSON string {}", - finding.path, - finding.line, - serde_json::to_string(&expected).expect("evidence string is JSON-serializable") - )) + Some(ReviewBatchValidationReason { + category: "evidenceMismatch", + repair_detail: format!( + "finding at {}:{} must set `evidence` to the exact JSON string {}", + finding.path, + finding.line, + serde_json::to_string(&expected).expect("evidence string is JSON-serializable") + ), + }) } fn review_batch_validation_reasons( findings: &[Finding], annotated: &str, content_policy_prompt: Option<&str>, -) -> Option { +) -> Option { let mut reasons = String::new(); - for reason in findings.iter().filter_map(|finding| { - review_batch_validation_reason( - finding, - annotated, - (finding.kind == crate::envelope::Kind::ContentPolicy) - .then_some(content_policy_prompt) - .flatten(), - ) - }) { + let mut category_counts = HashMap::<&'static str, usize>::new(); + let validation_reasons = findings + .iter() + .filter_map(|finding| { + review_batch_validation_reason( + finding, + annotated, + (finding.kind == crate::envelope::Kind::ContentPolicy) + .then_some(content_policy_prompt) + .flatten(), + ) + }) + .collect::>(); + for reason in &validation_reasons { + *category_counts.entry(reason.category).or_default() += 1; + } + for reason in validation_reasons { let separator = if reasons.is_empty() { "" } else { "; " }; let remaining = MAX_REVIEW_VALIDATION_REASON_BYTES.saturating_sub(reasons.len()); - if separator.len().saturating_add(reason.len()) <= remaining { + if separator.len().saturating_add(reason.repair_detail.len()) <= remaining { reasons.push_str(separator); - reasons.push_str(&reason); + reasons.push_str(&reason.repair_detail); continue; } if remaining > separator.len() { reasons.push_str(separator); let available = remaining - separator.len(); let end = reason + .repair_detail .char_indices() .map(|(index, _)| index) .take_while(|index| *index <= available) .last() .unwrap_or(0); - reasons.push_str(&reason[..end]); + reasons.push_str(&reason.repair_detail[..end]); } break; } - (!reasons.is_empty()).then_some(reasons) + if reasons.is_empty() { + return None; + } + let safe_detail = [ + "publicationContract", + "missingEvidenceAnchor", + "ambiguousEvidence", + "evidenceMismatch", + ] + .into_iter() + .filter_map(|category| { + category_counts + .get(category) + .map(|count| format!("{category}={count}")) + }) + .collect::>() + .join(","); + Some(ReviewValidationFailure::new(reasons, safe_detail)) } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -2465,9 +2509,10 @@ mod tests { let reason = review_batch_validation_reason(&finding, annotated, None).unwrap(); assert_eq!( - reason, + reason.repair_detail, "finding at src/lib.rs:7 must set `evidence` to the exact JSON string \" changed();\"" ); + assert_eq!(reason.category, "evidenceMismatch"); finding.evidence = Some(" changed();".to_string()); assert_eq!( @@ -2483,9 +2528,10 @@ mod tests { finding.evidence = Some("changed();".to_string()); assert_eq!( - review_batch_validation_reason(&finding, annotated, None).as_deref(), + review_batch_validation_reason(&finding, annotated, None) + .map(|reason| reason.repair_detail), Some( - "finding at src/lib.rs:7 violates the publication contract: finding body must end with sentence punctuation" + "finding at src/lib.rs:7 violates the publication contract: finding body must end with sentence punctuation".to_string() ) ); } @@ -2500,10 +2546,21 @@ mod tests { let reason = review_batch_validation_reasons(&[first, second], annotated, None).unwrap(); - assert!(reason.contains("src/lib.rs:7")); - assert!(reason.contains("exact JSON string \"first();\"")); - assert!(reason.contains("src/lib.rs:8")); - assert!(reason.contains("exact JSON string \"second();\"")); + assert!(reason.repair_detail().contains("src/lib.rs:7")); + assert!( + reason + .repair_detail() + .contains("exact JSON string \"first();\"") + ); + assert!(reason.repair_detail().contains("src/lib.rs:8")); + assert!( + reason + .repair_detail() + .contains("exact JSON string \"second();\"") + ); + assert_eq!(reason.safe_detail(), "evidenceMismatch=2"); + assert!(!reason.safe_detail().contains("src/lib.rs")); + assert!(!reason.safe_detail().contains("first();")); } #[test] @@ -2531,9 +2588,10 @@ mod tests { finding.evidence = Some("approximate evidence".to_string()); assert_eq!( - review_batch_validation_reason(&finding, annotated, None).as_deref(), + review_batch_validation_reason(&finding, annotated, None) + .map(|reason| reason.repair_detail), Some( - "finding at src/lib.rs:7 has multiple displayed new-side evidence strings; copy the exact supporting string or retract it" + "finding at src/lib.rs:7 has multiple displayed new-side evidence strings; copy the exact supporting string or retract it".to_string() ) ); } diff --git a/tests/e2e.rs b/tests/e2e.rs index 4423c97..4256864 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -5534,6 +5534,12 @@ async fn ungrounded_output_fails_closed() { assert_eq!(env["modelUsage"][1]["phase"], "semanticRetry"); assert_eq!(env["modelIncidents"][0]["category"], "invalidOutput"); assert_eq!(env["modelIncidents"][0]["recovered"], false); + let body = env["findings"][0]["body"].as_str().unwrap(); + assert!(body.contains("validation categories: missingEvidenceAnchor=1")); + assert!(!body.contains("src/auth.rs:999")); + let stderr = String::from_utf8(out.get_output().stderr.clone()).unwrap(); + assert!(stderr.contains("output validation failed categories=missingEvidenceAnchor=1")); + assert!(stderr.contains("semantic retry remained unusable categories=missingEvidenceAnchor=1")); } #[tokio::test] From 842629ecf3bc7aade1c0ecb0cfc9d848b4399a58 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Mon, 20 Jul 2026 09:10:02 +0000 Subject: [PATCH 2/2] Preserve validated review API compatibility --- src/llm.rs | 25 ++++++++++++++++++++----- src/review.rs | 2 +- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/llm.rs b/src/llm.rs index 3751267..32c18b0 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -1661,7 +1661,25 @@ impl LlmClient { /// Run a review while treating caller-rejected content as invalid model /// output. Each model gets one bounded semantic correction before the /// configured cascade advances, and every consumed call remains in usage. - pub(crate) async fn review_validated( + pub async fn review_validated( + &self, + cfg: &Config, + system: &str, + user: &str, + validate: F, + ) -> std::result::Result + where + F: Fn(&ModelReview) -> std::result::Result<(), String> + Send + Sync + 'static, + { + self.review_validated_with_safe(cfg, system, user, move |review| { + validate(review).map_err(|repair_detail| { + ReviewValidationFailure::new(repair_detail, "callerValidation=1".to_string()) + }) + }) + .await + } + + pub(crate) async fn review_validated_with_safe( &self, cfg: &Config, system: &str, @@ -5145,14 +5163,11 @@ mod tests { .findings .iter() .try_for_each(crate::envelope::validate_finding_publication) - .map_err(|reason| { - ReviewValidationFailure::new(reason, "publicationContract=1".to_string()) - }) }) .await .unwrap_err(); let detail = format!("{error:#}"); - assert!(detail.contains("validation categories: publicationContract=1")); + assert!(detail.contains("validation categories: callerValidation=1")); assert!(!detail.contains("word word")); assert_eq!(server.received_requests().await.unwrap().len(), 2); } diff --git a/src/review.rs b/src/review.rs index dbc5021..7776616 100644 --- a/src/review.rs +++ b/src/review.rs @@ -1339,7 +1339,7 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> let validation_annotated = annotated.clone(); let validation_user = user.clone(); match client - .review_validated(cfg, &system, &user, move |review| { + .review_validated_with_safe(cfg, &system, &user, move |review| { review_batch_validation_reasons( &review.findings, &validation_annotated,