From 2066e2f0ee54c29ffe03ee853cf29ee5ceb29a8a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 14:07:03 +0900 Subject: [PATCH 1/3] fix(review): canonicalize known-safe finding field drift instead of discarding the verdict gpt-5.6-luna (the only currently functional model in the review pool) emits otherwise-complete REQUEST_CHANGES control blocks that drift from the finding schema in two ways: 'priority' in place of 'severity', and a missing 'suggested_diff' where 'fix_direction' still states the remedy. valid_control() rejected the whole block for either drift, and because every fallback slot is currently unusable (deepseek budget-limited, github-models gpt-5/gpt-5-chat capped at 4000 tokens), the pool exhausted and the org-wide required check hard-failed instead of publishing the blocking review (observed on naruon PRs #964/#965, runs 29223144789/29222786095). Repair the drift instead: map priority->severity when severity is absent, and fall back to fix_direction when suggested_diff is missing or blank. Findings only exist on REQUEST_CHANGES blocks (APPROVE with findings is still rejected), so this leniency can only rescue blocking reviews - it can never loosen approval evidence. Both real rejected blocks from the naruon runs normalize to publishable REQUEST_CHANGES under the new code. Co-Authored-By: Claude Fable 5 --- .../ci/opencode_review_normalize_output.py | 31 +++++++++- .../test_opencode_review_normalize_output.py | 58 +++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/scripts/ci/opencode_review_normalize_output.py b/scripts/ci/opencode_review_normalize_output.py index f16ed6ec9..7d8bdd195 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -864,6 +864,32 @@ 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. Two + observed drifts from otherwise-complete blocks are repaired: ``priority`` + used in place of ``severity``, and a missing ``suggested_diff`` when + ``fix_direction`` still states the concrete remedy. + """ + + def non_empty(candidate: Any) -> bool: + """Return whether a candidate field value is a non-blank string.""" + return isinstance(candidate, str) and bool(candidate.strip()) + + finding = dict(finding) + priority = finding.pop("priority", None) + if not non_empty(finding.get("severity")) and non_empty(priority): + finding["severity"] = priority + if not non_empty(finding.get("suggested_diff")) and non_empty( + finding.get("fix_direction") + ): + finding["suggested_diff"] = finding["fix_direction"] + return finding + + def valid_control( value: Any, *, @@ -944,15 +970,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 +990,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..cf972a2fb 100644 --- a/tests/test_opencode_review_normalize_output.py +++ b/tests/test_opencode_review_normalize_output.py @@ -725,6 +725,64 @@ 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"] + normalized = norm.valid_control( + control(result="REQUEST_CHANGES", findings=[diffless]), **kwargs + ) + assert normalized is not None + assert normalized["findings"][0]["suggested_diff"] == "Restore the guard." + + blank_diff = finding(suggested_diff=" ") + normalized = norm.valid_control( + control(result="REQUEST_CHANGES", findings=[blank_diff]), **kwargs + ) + assert normalized is not None + assert normalized["findings"][0]["suggested_diff"] == "Restore the guard." + + 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_valid_control_repairs_approval_summary_from_bounded_evidence(tmp_path, monkeypatch): evidence = tmp_path / "bounded-review-evidence.md" evidence.write_text( From e38e75983804a2f057edd358bcf07a661e48475b Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 15:50:52 +0900 Subject: [PATCH 2/3] fix(opencode): keep request-change diffs source-backed --- .../ci/opencode_review_normalize_output.py | 23 +++--- .../test_opencode_review_normalize_output.py | 81 +++++++++++++++++-- 2 files changed, 84 insertions(+), 20 deletions(-) diff --git a/scripts/ci/opencode_review_normalize_output.py b/scripts/ci/opencode_review_normalize_output.py index 7d8bdd195..1f20a2084 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -869,24 +869,23 @@ def canonicalize_finding_fields(finding: dict[str, Any]) -> dict[str, Any]: 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. Two - observed drifts from otherwise-complete blocks are repaired: ``priority`` - used in place of ``severity``, and a missing ``suggested_diff`` when - ``fix_direction`` still states the concrete remedy. + 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 non_empty(candidate: Any) -> bool: - """Return whether a candidate field value is a non-blank string.""" - return isinstance(candidate, str) and bool(candidate.strip()) + 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 non_empty(finding.get("severity")) and non_empty(priority): - finding["severity"] = priority - if not non_empty(finding.get("suggested_diff")) and non_empty( - finding.get("fix_direction") + if not has_non_blank_text(finding.get("severity")) and has_non_blank_text( + priority ): - finding["suggested_diff"] = finding["fix_direction"] + finding["severity"] = priority return finding diff --git a/tests/test_opencode_review_normalize_output.py b/tests/test_opencode_review_normalize_output.py index cf972a2fb..e1dfa05e7 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 @@ -743,18 +746,20 @@ def test_valid_control_canonicalizes_known_safe_finding_field_drift(): diffless = finding() del diffless["suggested_diff"] - normalized = norm.valid_control( - control(result="REQUEST_CHANGES", findings=[diffless]), **kwargs + assert ( + norm.valid_control( + control(result="REQUEST_CHANGES", findings=[diffless]), **kwargs + ) + is None ) - assert normalized is not None - assert normalized["findings"][0]["suggested_diff"] == "Restore the guard." blank_diff = finding(suggested_diff=" ") - normalized = norm.valid_control( - control(result="REQUEST_CHANGES", findings=[blank_diff]), **kwargs + assert ( + norm.valid_control( + control(result="REQUEST_CHANGES", findings=[blank_diff]), **kwargs + ) + is None ) - assert normalized is not None - assert normalized["findings"][0]["suggested_diff"] == "Restore the guard." canonical_severity_wins = finding(priority="P2") normalized = norm.valid_control( @@ -783,6 +788,66 @@ def test_valid_control_canonicalizes_known_safe_finding_field_drift(): ) +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 0 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( From 1450e019ce746296943991fe3f2c442a3064568a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 16:11:38 +0900 Subject: [PATCH 3/3] test(review): match the approval gate's 1-based finding index The approval gate enumerates findings with start=1 (opencode_review_approve_gate.sh:163), so a missing suggested_diff is reported as "finding 1", not "finding 0". Fix the regression test's expected stderr to the gate's actual 1-based message; behavior (exit 4, NO_CONCLUSION) was already correct. Co-Authored-By: Claude Fable 5 --- tests/test_opencode_review_normalize_output.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_opencode_review_normalize_output.py b/tests/test_opencode_review_normalize_output.py index e1dfa05e7..5b48f22f0 100644 --- a/tests/test_opencode_review_normalize_output.py +++ b/tests/test_opencode_review_normalize_output.py @@ -843,7 +843,7 @@ def test_approval_gate_rejects_prose_fix_direction_without_suggested_diff(tmp_pa assert completed_process.returncode == 4 assert completed_process.stdout.strip() == "NO_CONCLUSION" assert ( - "finding 0 field suggested_diff must be a non-empty string" + "finding 1 field suggested_diff must be a non-empty string" in completed_process.stderr )