diff --git a/scripts/ci/opencode_review_normalize_output.py b/scripts/ci/opencode_review_normalize_output.py index f16ed6ec9..1f20a2084 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -864,6 +864,31 @@ def reject(reason: str) -> int: return 0 +def canonicalize_finding_fields(finding: dict[str, Any]) -> dict[str, Any]: + """Map known-safe model vocabulary drift onto the canonical finding schema. + + Findings only exist on REQUEST_CHANGES control blocks (valid_control rejects + APPROVE blocks that carry findings), so rescuing a drifted finding can only + publish a blocking review — it can never loosen approval evidence. The + observed safe drift is repaired: ``priority`` used in place of + ``severity``. Source-backed ``suggested_diff`` evidence must remain + explicit because the downstream publication gate verifies it against the + current-head diff. + """ + + def has_non_blank_text(field_candidate: Any) -> bool: + """Return whether a field candidate is a non-blank string.""" + return isinstance(field_candidate, str) and bool(field_candidate.strip()) + + finding = dict(finding) + priority = finding.pop("priority", None) + if not has_non_blank_text(finding.get("severity")) and has_non_blank_text( + priority + ): + finding["severity"] = priority + return finding + + def valid_control( value: Any, *, @@ -944,15 +969,18 @@ def valid_control( "regression_test_direction", "suggested_diff", ) + normalized_findings = [] for finding in findings: if not isinstance(finding, dict): return None line = finding.get("line") if isinstance(line, bool) or not isinstance(line, int) or line <= 0: return None + finding = canonicalize_finding_fields(finding) for field in required_finding_fields: if not isinstance(finding.get(field), str) or not finding[field].strip(): return None + normalized_findings.append(finding) normalized = { "head_sha": value["head_sha"], @@ -961,7 +989,7 @@ def valid_control( "result": result, "reason": reason, "summary": summary, - "findings": findings, + "findings": normalized_findings, } if isinstance(value.get("adversarial_validation"), dict): normalized["adversarial_validation"] = value["adversarial_validation"] diff --git a/tests/test_opencode_review_normalize_output.py b/tests/test_opencode_review_normalize_output.py index 2c817b63d..5b48f22f0 100644 --- a/tests/test_opencode_review_normalize_output.py +++ b/tests/test_opencode_review_normalize_output.py @@ -1,4 +1,7 @@ import json +import shutil +import subprocess +from pathlib import Path import pytest @@ -725,6 +728,126 @@ def test_valid_control_filters_shape_head_and_review_contract(): assert norm.valid_control(approve_without_findings_key, **kwargs)["findings"] == [] +def test_valid_control_canonicalizes_known_safe_finding_field_drift(): + kwargs = { + "expected_head_sha": "head", + "expected_run_id": "run", + "expected_run_attempt": "attempt", + } + + aliased = finding(priority="P1") + del aliased["severity"] + normalized = norm.valid_control( + control(result="REQUEST_CHANGES", findings=[aliased]), **kwargs + ) + assert normalized is not None + assert normalized["findings"][0]["severity"] == "P1" + assert "priority" not in normalized["findings"][0] + + diffless = finding() + del diffless["suggested_diff"] + assert ( + norm.valid_control( + control(result="REQUEST_CHANGES", findings=[diffless]), **kwargs + ) + is None + ) + + blank_diff = finding(suggested_diff=" ") + assert ( + norm.valid_control( + control(result="REQUEST_CHANGES", findings=[blank_diff]), **kwargs + ) + is None + ) + + canonical_severity_wins = finding(priority="P2") + normalized = norm.valid_control( + control(result="REQUEST_CHANGES", findings=[canonical_severity_wins]), + **kwargs, + ) + assert normalized is not None + assert normalized["findings"][0]["severity"] == "HIGH" + assert "priority" not in normalized["findings"][0] + + blank_alias = finding(priority=" ") + del blank_alias["severity"] + assert ( + norm.valid_control( + control(result="REQUEST_CHANGES", findings=[blank_alias]), **kwargs + ) + is None + ) + + no_remedy = finding(fix_direction="", suggested_diff="") + assert ( + norm.valid_control( + control(result="REQUEST_CHANGES", findings=[no_remedy]), **kwargs + ) + is None + ) + + +def test_approval_gate_rejects_prose_fix_direction_without_suggested_diff(tmp_path): + bash_command = shutil.which("bash") + if bash_command is None: + pytest.skip("bash is unavailable") + try: + subprocess.run( + [bash_command, "--version"], + capture_output=True, + text=True, + timeout=5, + check=True, + ) + except (OSError, subprocess.SubprocessError) as exc: + pytest.skip(f"bash is not usable for this regression test: {exc}") + + repo_root = Path(__file__).resolve().parents[1] + gate_script = repo_root / "scripts" / "ci" / "opencode_review_approve_gate.sh" + control_data = control( + result="REQUEST_CHANGES", + findings=[finding(fix_direction="Restore the guard.")], + ) + del control_data["findings"][0]["suggested_diff"] + comment_file = tmp_path / "comment.md" + comment_file.write_text( + "\n".join( + [ + "", + "", + "", + ] + ), + encoding="utf-8", + ) + + completed_process = subprocess.run( + [ + bash_command, + str(gate_script), + "head", + "run", + "attempt", + str(comment_file), + ], + cwd=repo_root, + capture_output=True, + text=True, + timeout=20, + check=False, + ) + + assert completed_process.returncode == 4 + assert completed_process.stdout.strip() == "NO_CONCLUSION" + assert ( + "finding 1 field suggested_diff must be a non-empty string" + in completed_process.stderr + ) + + def test_valid_control_repairs_approval_summary_from_bounded_evidence(tmp_path, monkeypatch): evidence = tmp_path / "bounded-review-evidence.md" evidence.write_text(