diff --git a/docs/doctoring/opencode-peer-check-reevaluation.md b/docs/doctoring/opencode-peer-check-reevaluation.md new file mode 100644 index 0000000000..56509accb5 --- /dev/null +++ b/docs/doctoring/opencode-peer-check-reevaluation.md @@ -0,0 +1,56 @@ +# OpenCode peer-check fallback reevaluation + +Status: Proposed. Owner issue: #2125. + +Protected main `fb17ef556f94f673234aa557254ae52779e9a7b0` accepted a +current-head CHANGES_REQUESTED review carrying its canonical model-unavailable +fallback marker as a substantive receipt. The required caller consequently +skipped another review after peer CodeQL checks recovered (#2113, +job 103581933696). + +The receipt helper matches the complete canonical failed-peer-check payload: +fixed producer prose, the current head SHA, failed-check rows, and an optional +generated Mermaid evidence map. Extra prose, mixed findings, and unknown formats +remain formal blockers; a heading or fallback marker cannot remove a product +finding. A diagram heading alone cannot hide additional prose. A newer fallback blocks reuse of an older same-head +receipt; a later substantive product finding still deduplicates normally. +Approval eligibility and downstream gates are unchanged. This does not approve +any PR or establish model availability. PR #1706 edits a separate verdict lookup +in the same caller; this repair changes only the shared receipt helper. + +Regression: six fallback-marker cases failed before the change; a substantive +finding case already passed. The receipt, live-draft and required-verdict tests +exercise the shared boundary and existing caller behavior. Actual fresh receiver +execution and a formal current-head review remain deployment acceptance gates. + +## Concurrent owner integration + +Ordinary merge preserves both aa6150c and 96704bc7 histories. The remote +substantive-finding repair's original mixed-review case is retained as an +additional regression. Its broad heading regex is superseded by the exact +producer envelope because unknown headings must remain blockers, not silently +qualify for reevaluation. No approval criterion is relaxed. + +### Historical remote-branch validation (not merged-head proof) + +Regression evidence is bound to exact commits. Commit +`9cd835df682b74ead1e5306b92280da941e45040` is RED because a mixed fallback +and authorization finding returns no receipt. Commits +`7b528279720af353ba91fbff5d43384ad466bb8b` and +`f5be0fb8f21d9fad64c8576b5e980d5eeb9a1b1e` introduce and correct the +minimal finding-heading classifier. On the corrected source blob +`310f05d956382d4ba525907ccba64c633c1c2a6c` and test blob +`05955d2e2181aefe957f239d8aae8012a0ad04d9`, the receipt, +live-Draft, required-verdict, and coverage-publication suites report 86 passed; +Ruff and compileall are green. Hosted exact-head checks, independent review, and +a fresh #2113 receiver/formal review remain deployment acceptance gates. + +## Unstructured finding regression + +The db8058e9 test exposes a product finding appended as ordinary prose beneath +the canonical heading. The former heading-only classifier discarded it. The +replacement matches the full producer envelope and fails closed on added prose, +including text after the diagram. Tests execute the actual producer printf block +and the graph emitter rather than reproducing shortened synthetic envelopes. +Receipt-module validation: 25 passed, 100% statements and branches; hosted +review and deployment evidence remain outstanding. diff --git a/scripts/ci/opencode_review_receipt_gate.py b/scripts/ci/opencode_review_receipt_gate.py index 4dcb24af88..279634a40e 100644 --- a/scripts/ci/opencode_review_receipt_gate.py +++ b/scripts/ci/opencode_review_receipt_gate.py @@ -110,6 +110,36 @@ def is_mention_or_malformed(body: str) -> bool: return not any(marker in stripped for marker in PRODUCT_MARKERS) +def is_peer_check_only_fallback(body: str, head_sha: str) -> bool: + """Match the complete producer payload; retain unknown prose as a blocker.""" + prefix = ( + "## Pull request overview\n\n" + "OpenCode could not approve from deterministic current-head evidence " + "because GitHub Checks have failed.\n\n" + "## Findings\n\n" + "### 1. HIGH Current-head GitHub Checks - Fix failed required checks before approval\n" + f"- Problem: Failed same-head checks remain for `{head_sha}`.\n" + "- Root cause: The model-unavailable evidence fallback is allowed only " + "when peer GitHub Checks are complete and clean.\n" + "- Fix: Read and fix the failed check logs below, then rerun the current-head checks.\n" + "- Regression test: Keep the model-unavailable fallback gated on an empty " + "failed-check rollup.\n\nFailed checks:\n" + ) + check_line = ( + r"- [^\n\r]+: (?i:FAILURE|ERROR|TIMED_OUT|ACTION_REQUIRED|CANCELLED|STARTUP_FAILURE)" + r"(?: \(https://[^\s()]+\))?" + ) + graph = ( + r"\n\n## Changed-File Evidence Map\n\n```mermaid\n" + r"(?:flowchart LR|classDiagram|sequenceDiagram)\n(?: [^\n`]+\n)+```" + ) + return re.fullmatch( + re.escape(prefix) + check_line + r"(?:\n" + check_line + r")*" + + r"(?:" + graph + r")?\n?", + body, + ) is not None + + def is_formal_receipt( review: Mapping[str, Any], head_sha: str, @@ -134,6 +164,10 @@ def is_formal_receipt( marker in body.casefold() for marker in FALLBACK_APPROVAL_MARKERS ): return False, "fallback approval is not a substantive formal review" + if state == "CHANGES_REQUESTED" and is_peer_check_only_fallback(body, head_sha) and any( + marker in body.casefold() for marker in FALLBACK_APPROVAL_MARKERS + ): + return False, "fallback changes request requires fresh substantive review" if is_draft and state == "APPROVED": return False, "draft must never receive bot APPROVE" return True, "current-head formal review" @@ -161,7 +195,7 @@ def evaluate_receipts( return review, reason if "never receive bot APPROVE" in reason: return None, reason - if "fallback approval" in reason: + if "fallback approval" in reason or "fallback changes request" in reason: return None, reason if reason.startswith("stale"): stale_hits += 1 diff --git a/tests/test_opencode_review_receipt_gate.py b/tests/test_opencode_review_receipt_gate.py index c971e2128a..099154bd35 100644 --- a/tests/test_opencode_review_receipt_gate.py +++ b/tests/test_opencode_review_receipt_gate.py @@ -342,3 +342,162 @@ def unexpected_run(args, **kwargs): )(), ) assert receipt.load_reviews("-")[0]["commit_id"] == receipt.AFIPC_230_HEAD + + +def canonical_peer_fallback(head): + """Execute the producer's printf block with a realistic failed-check row.""" + import subprocess + source = Path(".github/workflows/opencode-review-dispatch.yml").read_text() + # Locate the unique canonical producer directly; other fallbacks stay independent. + marker = source.index("printf 'OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.") + start = source.rfind(" printf '## Pull request overview", 0, marker) + end = source.index(' cat "$failed_checks_file"', marker) + program = source[start:end] + "printf '%s\\n' '- CodeQL PR/CodeQL compatibility analysis (python): FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/1)'" + return subprocess.check_output(["bash", "-c", program], env={"HEAD_SHA": head}, text=True) + + +def test_fallback_changes_request_requires_fresh_review(): + """The complete producer envelope triggers review without reusing approval.""" + head = receipt.AFIPC_230_HEAD + fallback = review(commit=head, body=canonical_peer_fallback(head)) + older_approval = review(commit=head, state="APPROVED") + found, reason = receipt.evaluate_receipts([older_approval, fallback], head) + assert found is None + assert "fallback" in reason + + +def test_substantive_changes_request_still_deduplicates(): + """Actual product findings remain a receipt even after a prior fallback.""" + head = receipt.AFIPC_230_HEAD + fallback = review(commit=head, body="## Pull request overview\nmodel-unavailable evidence fallback") + substantive = review(commit=head, body=( + "## Pull request overview\n## Findings\n" + "### 1. HIGH Missing authorization\n" + "The changed endpoint allows anonymous writes.\n" + )) + found, _ = receipt.evaluate_receipts([fallback, substantive], head) + assert found == substantive + + + +def test_fallback_marker_does_not_hide_substantive_finding() -> None: + """A fallback marker cannot suppress a real product blocker in the same review.""" + head = receipt.AFIPC_230_HEAD + mixed = review( + commit=head, + body=( + "## Pull request overview\n" + "model-unavailable evidence fallback\n" + "OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.\n## Findings\n" + "### 1. HIGH Current-head GitHub Checks - Fix failed required checks before approval\n" + "### 2. HIGH Missing authorization\n" + "The changed endpoint allows anonymous writes.\n" + ), + ) + found, reason = receipt.evaluate_receipts([mixed], head) + assert found == mixed + assert reason == "current-head formal review" + + +@pytest.mark.parametrize("suffix", [ + "\n### 2. HIGH Missing authorization\nAnonymous writes are allowed.\n", + "\n### Unexpected finding shape\n", + "\n#### Missing authorization\n", + "\n###Missing authorization\n", +]) +def test_unknown_or_mixed_finding_is_retained(suffix): + """Only an exact peer-check-only finding list may trigger reevaluation.""" + head = receipt.AFIPC_230_HEAD + body = ("## Pull request overview\nmodel-unavailable evidence fallback\n" + "OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.\n## Findings\n" + "### 1. HIGH Current-head GitHub Checks - Fix failed required checks before approval\n") + candidate = review(commit=head, body=body + suffix) + assert receipt.evaluate_receipts([candidate], head)[0] == candidate + + +def test_evidence_map_headings_are_not_product_findings(): + """Only the generated fenced diagram may follow the complete finding.""" + from scripts.ci.opencode_review_surfaces import emit_mermaid + head = receipt.AFIPC_230_HEAD + body = canonical_peer_fallback(head) + "\n## Changed-File Evidence Map\n\n" + emit_mermaid([]) + assert receipt.evaluate_receipts([review(commit=head, body=body)], head)[0] is None + for extra in ("\nThe endpoint allows anonymous writes.", "\n## Findings\nOther finding"): + candidate = review(commit=head, body=body + extra) + assert receipt.evaluate_receipts([candidate], head)[0] == candidate + + +def test_unknown_fallback_format_is_retained(): + """A fallback marker alone cannot classify an unknown review as peer-only.""" + head = receipt.AFIPC_230_HEAD + candidate = review(commit=head, body="## Pull request overview\nmodel-unavailable evidence fallback") + assert receipt.evaluate_receipts([candidate], head)[0] == candidate + + +def test_peer_fallback_literals_remain_bound_to_canonical_producer(): + """Producer wording drift requires an explicit receipt-contract update.""" + source = Path(".github/workflows/opencode-review-dispatch.yml").read_text() + assert "OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed." in source + assert "### 1. HIGH Current-head GitHub Checks - Fix failed required checks before approval" in source + + +def test_mixed_finding_without_canonical_overview_remains_blocking(): + """Preserve the concurrent owner's original mixed-review counterexample.""" + head = receipt.AFIPC_230_HEAD + candidate = review(commit=head, body=( + "## Pull request overview\nmodel-unavailable evidence fallback\n" + "## Findings\n" + "### 1. HIGH Current-head GitHub Checks - Fix failed required checks before approval\n" + "### 2. HIGH Missing authorization\nThe changed endpoint allows anonymous writes.\n" + )) + assert receipt.evaluate_receipts([candidate], head)[0] == candidate + + +@pytest.mark.parametrize("sections", ["", "## Findings\n### 1. HIGH Missing authorization\n## Findings\n### 2. HIGH Missing authorization\n"]) +def test_missing_or_duplicate_findings_remain_blocking(sections): + """Ambiguous section structure must never discard an active change request.""" + head = receipt.AFIPC_230_HEAD + candidate = review(commit=head, body=( + "## Pull request overview\nmodel-unavailable evidence fallback\n" + "OpenCode could not approve from deterministic current-head evidence " + "because GitHub Checks have failed.\n" + sections + )) + assert receipt.evaluate_receipts([candidate], head)[0] == candidate + + +def test_peer_fallback_with_unstructured_product_finding_remains_blocking() -> None: + """Substantive prose cannot hide under the canonical peer-check heading.""" + head = receipt.AFIPC_230_HEAD + candidate = review( + commit=head, + body=( + "## Pull request overview\n\n" + "OpenCode could not approve from deterministic current-head evidence " + "because GitHub Checks have failed.\n\n" + "model-unavailable evidence fallback\n\n" + "## Findings\n\n" + "### 1. HIGH Current-head GitHub Checks - Fix failed required checks before approval\n" + f"- Problem: Failed same-head checks remain for `{head}`.\n" + "- Root cause: The model-unavailable evidence fallback is allowed only " + "when peer GitHub Checks are complete and clean.\n" + "- Fix: Read and fix the failed check logs below, then rerun the current-head checks.\n" + "- Regression test: Keep the model-unavailable fallback gated on an empty " + "failed-check rollup.\n\n" + "Failed checks:\n" + "- CodeQL PR/CodeQL compatibility analysis (python): FAILURE " + "(https://github.com/ContextualWisdomLab/.github/actions/runs/1)\n\n" + "The changed endpoint also allows anonymous writes.\n" + ), + ) + + found, _ = receipt.evaluate_receipts([candidate], head) + + assert found == candidate + + +@pytest.mark.parametrize("extra", ["The endpoint allows anonymous writes.\n", "- Missing authorization\n", "## Other finding\n"]) +def test_canonical_payload_with_extra_prose_is_retained(extra): + """Unknown prose anywhere in an otherwise valid payload remains blocking.""" + head = receipt.AFIPC_230_HEAD + for body in (extra + canonical_peer_fallback(head), canonical_peer_fallback(head) + extra): + candidate = review(commit=head, body=body) + assert receipt.evaluate_receipts([candidate], head)[0] == candidate