fix(verdict): findings resolved by the change under review never block
The review model routinely narrates the pre-existing bug a PR fixes as a critical/confirmed correctness finding; deriveVerdict then failed the very change that fixes it (three consecutive A-grade reviews of shreai#188 verdicted fail this way). Findings now carry resolved_by_this_change (prompted, schema'd, normalized); resolved findings are excluded from fail/warn derivation, and a model-level warn whose findings are all resolved normalizes to pass. Introduced-or-remaining defects still block. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01AD1cPDz563Fyppc7T4ibhU
This commit is contained in:
@@ -19,12 +19,14 @@ Output schema (every key required):
|
|||||||
"findings": [
|
"findings": [
|
||||||
{"title": "...", "file": "path", "line": 123, "severity": "critical"|"high"|"medium"|"low",
|
{"title": "...", "file": "path", "line": 123, "severity": "critical"|"high"|"medium"|"low",
|
||||||
"confidence": "confirmed"|"plausible", "axis": "correctness"|"security"|"tests_coverage"|"design_simplicity"|"performance"|"style"|"breaking_changes"|"docs",
|
"confidence": "confirmed"|"plausible", "axis": "correctness"|"security"|"tests_coverage"|"design_simplicity"|"performance"|"style"|"breaking_changes"|"docs",
|
||||||
|
"resolved_by_this_change": true|false,
|
||||||
"detail": "what is wrong, concrete failure scenario, and the fix"}
|
"detail": "what is wrong, concrete failure scenario, and the fix"}
|
||||||
],
|
],
|
||||||
"verdict": "pass"|"warn"|"fail"
|
"verdict": "pass"|"warn"|"fail"
|
||||||
}
|
}
|
||||||
|
|
||||||
Rules:
|
Rules:
|
||||||
|
- A finding must describe a defect that EXISTS IN THE RESULTING CODE after this diff is applied. The pre-existing bug that this diff FIXES is not a finding — acknowledge it in the correctness rationale instead. If you still list it (e.g. for the record), you MUST set "resolved_by_this_change": true on that finding; findings the diff introduces or leaves unfixed get "resolved_by_this_change": false.
|
||||||
- "confirmed" means you can trace a concrete failure from the diff alone; anything needing unseen context is "plausible".
|
- "confirmed" means you can trace a concrete failure from the diff alone; anything needing unseen context is "plausible".
|
||||||
- severity critical/high is reserved for real correctness bugs, security holes, data loss, or breaking API changes.
|
- severity critical/high is reserved for real correctness bugs, security holes, data loss, or breaking API changes.
|
||||||
- verdict "fail" ONLY when a confirmed critical/high correctness or security finding exists; "warn" for notable but non-blocking issues; otherwise "pass".
|
- verdict "fail" ONLY when a confirmed critical/high correctness or security finding exists; "warn" for notable but non-blocking issues; otherwise "pass".
|
||||||
|
|||||||
+11
-2
@@ -7,6 +7,10 @@ const BLOCKING_AXES = new Set(['correctness', 'security']);
|
|||||||
|
|
||||||
export function isBlockingFinding(f) {
|
export function isBlockingFinding(f) {
|
||||||
if (!f || typeof f !== 'object') return false;
|
if (!f || typeof f !== 'object') return false;
|
||||||
|
// A finding the diff itself resolves describes the PRE-change state; it
|
||||||
|
// cannot block the change that fixes it (models routinely narrate the fixed
|
||||||
|
// bug as a critical/confirmed finding, which failed correct fixes).
|
||||||
|
if (f.resolved_by_this_change === true) return false;
|
||||||
const sev = String(f.severity || '').toLowerCase();
|
const sev = String(f.severity || '').toLowerCase();
|
||||||
const conf = String(f.confidence || '').toLowerCase();
|
const conf = String(f.confidence || '').toLowerCase();
|
||||||
if (!BLOCKING_SEVERITIES.has(sev)) return false;
|
if (!BLOCKING_SEVERITIES.has(sev)) return false;
|
||||||
@@ -21,12 +25,16 @@ export function deriveVerdict(review) {
|
|||||||
const findings = Array.isArray(review?.findings) ? review.findings : [];
|
const findings = Array.isArray(review?.findings) ? review.findings : [];
|
||||||
if (findings.some(isBlockingFinding)) return 'fail';
|
if (findings.some(isBlockingFinding)) return 'fail';
|
||||||
const hasWarn = findings.some(f => {
|
const hasWarn = findings.some(f => {
|
||||||
|
if (f?.resolved_by_this_change === true) return false;
|
||||||
const sev = String(f?.severity || '').toLowerCase();
|
const sev = String(f?.severity || '').toLowerCase();
|
||||||
return sev === 'critical' || sev === 'high' || sev === 'medium';
|
return sev === 'critical' || sev === 'high' || sev === 'medium';
|
||||||
});
|
});
|
||||||
if (hasWarn) return 'warn';
|
if (hasWarn) return 'warn';
|
||||||
// trust the model's verdict if it said warn without matching findings
|
// Trust the model's verdict if it said warn without matching findings —
|
||||||
if (review?.verdict === 'warn') return 'warn';
|
// unless every finding it gave is resolved_by_this_change, in which case its
|
||||||
|
// warn is about the pre-change state and the resulting code is clean.
|
||||||
|
const allResolved = findings.length > 0 && findings.every(f => f?.resolved_by_this_change === true);
|
||||||
|
if (review?.verdict === 'warn' && !allResolved) return 'warn';
|
||||||
return 'pass';
|
return 'pass';
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -49,6 +57,7 @@ export function normalizeReview(raw) {
|
|||||||
severity: pick(String(f?.severity || '').toLowerCase(), ['critical', 'high', 'medium', 'low'], 'low'),
|
severity: pick(String(f?.severity || '').toLowerCase(), ['critical', 'high', 'medium', 'low'], 'low'),
|
||||||
confidence: pick(String(f?.confidence || '').toLowerCase(), ['confirmed', 'plausible'], 'plausible'),
|
confidence: pick(String(f?.confidence || '').toLowerCase(), ['confirmed', 'plausible'], 'plausible'),
|
||||||
axis: f?.axis ? String(f.axis).toLowerCase() : null,
|
axis: f?.axis ? String(f.axis).toLowerCase() : null,
|
||||||
|
resolved_by_this_change: f?.resolved_by_this_change === true,
|
||||||
detail: String(f?.detail ?? '').slice(0, 2000)
|
detail: String(f?.detail ?? '').slice(0, 2000)
|
||||||
}));
|
}));
|
||||||
const review = {
|
const review = {
|
||||||
|
|||||||
Reference in New Issue
Block a user