From 1947ed2911ceed4d7c0333b6687b40c99eee3be8 Mon Sep 17 00:00:00 2001 From: Postil Maintainer Date: Sun, 16 Aug 2026 03:39:11 +0000 Subject: [PATCH] fix(review): preserve directly established findings --- Cargo.lock | 2 +- Cargo.toml | 2 +- bench/baseline.json | 2 +- bench/src/live.test.ts | 27 +++++++++++++++++++++++++++ bench/src/live.ts | 35 ++++++++++++++++++++++++++++++----- src/prompt.rs | 13 ++++++------- src/review.rs | 25 ++++++++++++++++++++++++- 7 files changed, 90 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab15ec3..4e90407 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1104,7 +1104,7 @@ checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" [[package]] name = "postil-cli" -version = "0.8.20" +version = "0.8.21" dependencies = [ "aho-corasick", "anyhow", diff --git a/Cargo.toml b/Cargo.toml index 4805387..a3a032c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "postil-cli" -version = "0.8.20" +version = "0.8.21" 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/bench/baseline.json b/bench/baseline.json index d199daf..86b8ccb 100644 --- a/bench/baseline.json +++ b/bench/baseline.json @@ -2,7 +2,7 @@ "schemaVersion": 1, "corpus": { "fixtureCorpusSha256": "8e4c2cb9ad5a7efdfe6a875566d20133e905155b6f693a873595adf6c069e065", - "evaluatorSha256": "e1c5b005a5e516b79866b4c6e98df54e806d424b923b8c9797e3930ff3026874" + "evaluatorSha256": "ebc72a8d7d60c08bf79759a0dfc5dbf821a9e6f79f4503c08ea9660dddece4d7" }, "profiles": { "z-ai/glm-5.2": { diff --git a/bench/src/live.test.ts b/bench/src/live.test.ts index 7a244a6..03cdddd 100644 --- a/bench/src/live.test.ts +++ b/bench/src/live.test.ts @@ -14,6 +14,7 @@ import { runLive, resolveLiveTimeoutOverrides, scorerOperationalFailure, + targetSuppressionReasons, validateLiveRunId, } from "./live"; @@ -49,6 +50,32 @@ async function onlyCaseAttempt(runRoot: string): Promise { } describe("live benchmark review mode", () => { + test("reports why an authored target was suppressed", () => { + const truth = { + clean: false, + path: "src/api/orders/route.ts", + startLine: 31, + endLine: 31, + severity: "error", + }; + const finding = { + path: truth.path, + line: truth.startLine, + severity: "warn" as const, + confidence: 0.8, + kind: "risk" as const, + title: "Restore the response field", + body: "The response field is missing.", + }; + + expect(targetSuppressionReasons({ + suppressedFindings: [ + { finding, reason: "repositoryClaimUnsupported" }, + { finding: { ...finding, line: 40 }, reason: "belowThreshold" }, + ], + }, truth)).toEqual(["repositoryClaimUnsupported"]); + }); + test("forwards explicit timeout overrides to the isolated child and records them", async () => { const root = await mkdtemp(join(tmpdir(), "postil-live-timeout-forwarding-")); const names = [ diff --git a/bench/src/live.ts b/bench/src/live.ts index c4a632c..47a4717 100644 --- a/bench/src/live.ts +++ b/bench/src/live.ts @@ -149,6 +149,9 @@ export interface LiveCaseResult { observedProviderCostUsdDecimal: string | null; costAccountingComplete: boolean; reviewCoverage: Envelope["reviewCoverage"] | null; + /** Suppression reasons for findings that overlapped the authored target. + * Diagnostic only: suppressed findings never count as detections. */ + suppressedTargetReasons?: string[]; exitCode: number | undefined; error?: string; /** Captured stderr of the binary run. Used internally to classify transient @@ -541,6 +544,7 @@ async function runLiveCase( observedProviderCostUsdDecimal: null, costAccountingComplete: false, reviewCoverage: null, + suppressedTargetReasons: [], exitCode: undefined, }; @@ -824,10 +828,9 @@ function scoreCase(base: LiveCaseResult, truth: GroundTruth, env: Envelope): Liv return base; } - const match = findings.find((finding) => - finding.path === truth.path && - Math.min(finding.line, finding.endLine ?? finding.line) <= truth.endLine! && - Math.max(finding.line, finding.endLine ?? finding.line) >= truth.startLine!); + base.suppressedTargetReasons = targetSuppressionReasons(env, truth); + + const match = findings.find((finding) => findingMatchesTruth(finding, truth)); base.detected = Boolean(match); if (match) { base.foundSeverity = match.severity; @@ -842,6 +845,25 @@ function scoreCase(base: LiveCaseResult, truth: GroundTruth, env: Envelope): Liv return base; } +function findingMatchesTruth( + finding: Pick, + truth: GroundTruth, +): boolean { + return finding.path === truth.path && + Math.min(finding.line, finding.endLine ?? finding.line) <= truth.endLine! && + Math.max(finding.line, finding.endLine ?? finding.line) >= truth.startLine!; +} + +export function targetSuppressionReasons( + env: Pick, + truth: GroundTruth, +): string[] { + if (truth.clean) return []; + return env.suppressedFindings + .filter(({ finding }) => findingMatchesTruth(finding, truth)) + .map(({ reason }) => reason); +} + function groundTruthOf(c: ReturnType): GroundTruth { const gt = c.groundTruth.findings[0]; if (!gt) return { clean: true, path: null, startLine: null, endLine: null, severity: null }; @@ -1013,7 +1035,10 @@ export function formatLiveReport(report: LiveReport): string { ? `${r.truthSeverity}->${r.foundSeverity}${severityTag(r)} conf=${fmt(r.confidence)}` : `${r.truthSeverity}`; const fp = r.falsePositives > 0 ? ` +${r.falsePositives} FP` : ""; - lines.push(`${tag} ${r.id}: ${sev}${fp}`); + const suppressed = r.detected || !r.suppressedTargetReasons?.length + ? "" + : ` suppressed-target=${r.suppressedTargetReasons.join(",")}`; + lines.push(`${tag} ${r.id}: ${sev}${fp}${suppressed}`); } } return lines.join("\n"); diff --git a/src/prompt.rs b/src/prompt.rs index 9650a45..2a980e2 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -144,9 +144,9 @@ pub fn review_contract(cfg: &Config) -> String { `.postil/change-metadata`. Findings citing other lines are discarded as \ ungrounded.\n\ \n\ - `repositoryContext` is optional and defaults to a diff-local finding. Omit it whenever \ - the exact cited changed line is sufficient to establish the defect, even if unchanged \ - code elsewhere could also be relevant. Include it only when the conclusion depends on \ + `repositoryContext` is optional. Omit it for bugs the cited line establishes, including \ + removed fields, bypassed guards, boundary errors, or lifecycle defects; caller or consumer \ + impact alone doesn't require it. Include it only when the conclusion depends on \ repository-wide evidence, using `claim: absence` for a construct missing from the \ complete reviewed head or `claim: mismatch` for a repository target whose expected \ value is not established by the cited changed line. When included, name the target in \ @@ -547,12 +547,11 @@ mod tests { #[test] fn generator_omits_repository_claims_for_diff_local_conclusions() { let prompt = system_prompt(&Config::default(), trusted_date()); - assert!( - prompt.contains("`repositoryContext` is optional and defaults to a diff-local finding") - ); + assert!(prompt.contains("`repositoryContext` is optional")); assert!(prompt.contains( - "Omit it whenever the exact cited changed line is sufficient to establish the defect" + "Omit it for bugs the cited line establishes, including removed fields, bypassed guards, boundary errors, or lifecycle defects" )); + assert!(prompt.contains("caller or consumer impact alone doesn't require it")); assert!( prompt.contains( "Include it only when the conclusion depends on repository-wide evidence" diff --git a/src/review.rs b/src/review.rs index 225f315..fa7beba 100644 --- a/src/review.rs +++ b/src/review.rs @@ -201,7 +201,7 @@ fn review_batch_validation_reason( if let Some(category) = crate::envelope::publication_evidence_boundary_category(finding) { let repair = match category { "delegatedEvidenceCollection" => { - "do not delegate repository inspection to a human; declare the exact absence or mismatch through `repositoryContext` so Postil can search the complete reviewed head, or retract the finding" + "if the cited changed line already establishes the defect, state it directly and omit `repositoryContext`; otherwise do not delegate repository inspection to a human, declare the exact absence or mismatch through `repositoryContext` so Postil can search the complete reviewed head, or retract the finding" } "reviewArtifactPhrase" | "reviewArtifactBoundary" => { "remove review-process terms such as `diff`, `patch`, `PR`, `MR`, `review input`, and `provided context`, state the defect directly from the concrete repository construct with an actionable fix, and retract the finding if the supplied evidence is insufficient" @@ -4195,6 +4195,29 @@ mod tests { assert_eq!(failure.safe_detail(), "reviewArtifactPhrase=1"); } + #[test] + fn delegated_evidence_repair_preserves_directly_established_defects() { + let annotated = "### src/lib.rs\n@@ fixture @@\n 7 + changed();\n"; + let mut finding = finding( + "src/lib.rs", + 7, + "Verify that `applyBulkEdit` internally enforces the permission check.", + ); + finding.evidence = Some("changed();".to_string()); + + let reason = review_batch_validation_reason(&finding, annotated, None).unwrap(); + + assert_eq!(reason.category, "delegatedEvidenceCollection"); + assert!(reason.repair_detail.contains( + "if the cited changed line already establishes the defect, state it directly and omit `repositoryContext`" + )); + assert!( + reason + .repair_detail + .contains("declare the exact absence or mismatch through `repositoryContext`") + ); + } + #[test] fn all_ungrounded_failure_uses_public_evidence_validation_language() { let finding = ungrounded_findings_failure(3);