diff --git a/.github/actions/pr-review/action.yml b/.github/actions/pr-review/action.yml index bf0dc1b..3961966 100644 --- a/.github/actions/pr-review/action.yml +++ b/.github/actions/pr-review/action.yml @@ -130,6 +130,17 @@ runs: # Bind a fresh provisional comment before the model runs. Completed # reports remain separate, CI-owned publication output. run: python3 ${{ github.action_path }}/scripts/prepare-review-summary.py + - name: Freeze prior blocking reviews for optional self-dismissal + id: reset-prepare + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ inputs.github_token }} + PR_NUMBER: ${{ inputs.pr_number }} + SUMMARY_MARKER: ${{ steps.review-config.outputs.summary_heading }} + REVIEW_RUN_STARTED_AT: ${{ steps.review-config.outputs.review_run_started_at }} + REVIEW_RESET_SNAPSHOT: ${{ runner.temp }}/pr-review-reset-${{ steps.summary-transport.outputs.comment_id }}-snapshot.json + run: python3 ${{ github.action_path }}/scripts/review-reset.py prepare - name: Run Claude PR Review id: claude_review uses: anthropics/claude-code-action@9171db3e57d6a3140a37ddc2ba92788584e0ead6 # v1.0.234: Claude Code 2.1.282 @@ -185,6 +196,29 @@ runs: # shell path) to create or edit comments. claude_args: --model claude-opus-5-5 --max-turns 100 --setting-sources user --strict-mcp-config --disallowedTools "Skill,ScheduleWakeup,CronCreate,CronDelete,CronList" --allowedTools "Read,Glob,Grep,Task,mcp__github_inline_comment__create_inline_comment,mcp__github_comment__update_claude_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh api:*)" prompt: ${{ env.REVIEW_PROMPT }} + - name: Capture optional terminal reset proof + id: reset-proof + if: steps.claude_review.outcome == 'success' && steps.reset-prepare.outcome == 'success' && steps.reset-prepare.outputs.has_targets == 'true' + continue-on-error: true + shell: bash + env: + SUMMARY_MARKER: ${{ steps.review-config.outputs.summary_heading }} + REVIEW_RESET_SNAPSHOT: ${{ runner.temp }}/pr-review-reset-${{ steps.summary-transport.outputs.comment_id }}-snapshot.json + REVIEW_EXECUTION_FILE: ${{ steps.claude_review.outputs.execution_file }} + REVIEW_SESSION_ID: ${{ steps.claude_review.outputs.session_id }} + REVIEW_RESET_FILE: ${{ runner.temp }}/pr-review-reset-${{ steps.summary-transport.outputs.comment_id }}.json + run: python3 ${{ github.action_path }}/scripts/review-reset.py capture + - name: Persist accepted reset evidence + id: reset-upload + if: steps.reset-proof.outcome == 'success' + continue-on-error: true + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 + with: + name: ${{ steps.reset-prepare.outputs.artifact_name }} + path: ${{ runner.temp }}/pr-review-reset-${{ steps.summary-transport.outputs.comment_id }}.json + if-no-files-found: error + # Repository retention defaults apply. Failure retains blocks but does not + # suppress the ordinary publisher; outcome below is NOT masked conclusion. - name: Publish review report and verdict id: publish # CI finalizes the run deterministically rather than relying on the agent @@ -235,3 +269,18 @@ runs: .github/review-criteria.md .github/review-criteria.json retention-days: 7 + - name: Dismiss exactly audited prior blocking reviews + if: steps.publish.outcome == 'success' && steps.reset-proof.outcome == 'success' && steps.reset-upload.outcome == 'success' + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ inputs.github_token }} + PR_NUMBER: ${{ inputs.pr_number }} + SUMMARY_MARKER: ${{ steps.review-config.outputs.summary_heading }} + REVIEW_RUN_STARTED_AT: ${{ steps.review-config.outputs.review_run_started_at }} + REVIEW_RESET_FILE: ${{ runner.temp }}/pr-review-reset-${{ steps.summary-transport.outputs.comment_id }}.json + REVIEW_RESET_SHA256: ${{ steps.reset-proof.outputs.sha256 }} + REVIEW_RESET_UPLOAD_OUTCOME: ${{ steps.reset-upload.outcome }} + REVIEW_RESET_ARTIFACT_ID: ${{ steps.reset-upload.outputs.artifact-id }} + REVIEW_RESET_ARTIFACT_DIGEST: ${{ steps.reset-upload.outputs.artifact-digest }} + run: python3 ${{ github.action_path }}/scripts/review-reset.py dismiss diff --git a/.github/actions/pr-review/prompts/base-pr-review.md b/.github/actions/pr-review/prompts/base-pr-review.md index c16d7c8..17a8de0 100644 --- a/.github/actions/pr-review/prompts/base-pr-review.md +++ b/.github/actions/pr-review/prompts/base-pr-review.md @@ -402,6 +402,38 @@ Your only obligation is an accurate count and a complete summary; a missing or malformed count turns the whole run red, so always post the summary in the exact template above. +### Optional terminal proof for clearing this reviewer's prior blocks + +If `.github/review-reset.json` is available and its `targets` list is nonempty, +read its frozen `audit` entries in addition to Step 3. Recheck EACH entry against +current code, including all original blocking report findings; duplicated entries +still need their own audit IDs. Missing/deleted/edited original evidence is not +proof of a fix. +Do not rely on resolved threads or earlier runs' “fixed” assertions. +If there are no eligible targets, skip this optional proof and publish normally. + +Post the usual final Markdown summary through the native comment tool first. +Then end your terminal response with one `review-reset-proof` fenced JSON block +(not a comment, file, tool call, or a second copy of the Markdown): + +```review-reset-proof +{"review_complete":true,"coverage_gaps":[],"blocking_issues":0, + "audit":[{"id":"","disposition":"fixed", + "evidence":[{"path":"path/to/file","location":"line range or symbol", + "explanation":"Current code evidence resolving this exact finding"}]}]} +``` + +CI supplies run/attempt, head/base, and session provenance from its frozen +snapshot and runner metadata; do not add provenance fields to this proof. + +Use `fixed`, `obsolete` (identify replacement code), `incorrect` (explain why the +original finding was wrong), `still_present`, or `unverified` honestly for every +entry. Set completeness, coverage gaps, and blocking count to the actual review +outcome, never to obtain dismissal. Material gaps keep the normal summary +provisional as already required. Finish all foreground audit agents before the +terminal response. If the reset snapshot/proof is unavailable, publish normally; +CI simply retains prior blocks. This proof never authorizes APPROVE. + ## Review Criteria Use these base criteria for every repository. Built-in mixins and trusted repo-local diff --git a/.github/actions/pr-review/scripts/review-reset.py b/.github/actions/pr-review/scripts/review-reset.py new file mode 100755 index 0000000..b5df2a2 --- /dev/null +++ b/.github/actions/pr-review/scripts/review-reset.py @@ -0,0 +1,473 @@ +#!/usr/bin/env python3 +"""Optional, fail-closed self-dismissal after the unchanged baseline publisher. + +The terminal runner proof plus the frozen inputs is the reset authority, not +editable report prose. This does not isolate the model's existing GitHub token. +""" + +import hashlib +import importlib.util +import json +import os +from pathlib import Path +import re +import sys +import tempfile +from urllib.parse import quote + +import _gh +import _review_state + +_spec = importlib.util.spec_from_file_location( + "reset_publisher", Path(__file__).with_name("publish-review-report.py") +) +pub = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(pub) +SNAPSHOT = Path(".github/review-reset.json") +PROOF_PATTERN = re.compile(r"```review-reset-proof\s*\n(.*?)\n```\s*$", re.DOTALL) +REVIEWER_KEYS = ("workflow_ref", "summary_marker", "verdict_mode") + + +def require(condition, message): + if not condition: + raise _gh.TerminalError(message) + + +def read_json(path, maximum=2 * 1024 * 1024): + raw = Path(path).read_bytes() + require(len(raw) <= maximum, "reset input exceeds size limit") + def pairs(items): + result = {} + for key, value in items: + require(key not in result, "duplicate JSON key") + result[key] = value + return result + return json.loads(raw, object_pairs_hook=pairs) + + +def write_json(path, value): + """Replace only our destination entry, never follow a planted file symlink.""" + path = Path(path) + path.parent.mkdir(parents=True, exist_ok=True) + temporary = None + try: + with tempfile.NamedTemporaryFile(mode="w", encoding="utf-8", dir=path.parent, + prefix=f".{path.name}.", delete=False) as stream: + temporary = Path(stream.name) + json.dump(value, stream, ensure_ascii=False, indent=2) + stream.write("\n") + os.replace(temporary, path) + finally: + if temporary is not None: + temporary.unlink(missing_ok=True) + + +def sanitize(text): + """Pinned native comment sanitizer/redactor rules, also for proof strings.""" + for _ in range(16): + previous = text + text = re.sub(r"", "", text) + text = re.sub(r"[\u200b\u200c\u200d\ufeff\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f\u00ad\u202a-\u202e\u2066-\u2069]", "", text) + text = re.sub(r"!\[[^\]]*\]\(", "![](", text) + text = re.sub(r"!\[[^\]]*\](\[[^\]]*\])", r"![]\1", text) + text = re.sub(r'''(\[[^\]]*\]\([^)]+)\s+["'][^"']*["']''', r"\1", text) + for attribute in ("alt", "title", "aria-label", r"data-[a-zA-Z0-9-]+", "placeholder"): + for value in (r'''"[^"]*"''', r"'[^']*'", r"[^\s>]+"): + text = re.sub(r"\s" + attribute + r"\s*=\s*" + value, "", text, flags=re.I) + def entity(match): + raw = match[0][2:-1] + number = int(raw[1:], 16) if raw.startswith("x") else int(raw) + return chr(number) if 32 <= number <= 126 else "" + text = re.sub(r"&#(?:x[0-9a-fA-F]+|\d+);", entity, text) + text = re.sub(r"gh[pousr]_[A-Za-z0-9]{36}\b|github_pat_[A-Za-z0-9_]{11,221}\b", "[REDACTED_GITHUB_TOKEN]", text) + text = re.sub(r"sk-ant-[A-Za-z0-9_-]{20,}", "[REDACTED_ANTHROPIC_KEY]", text) + text = re.sub(r'''(\b|\\[nrtbf"\\/]|\\u[0-9a-fA-F]{4}|\[[0-9;]*m)(?:AKIA|ASIA)[A-Z0-9]{16}\b''', lambda m: m[1] + "[REDACTED_AWS_KEY_ID]", text) + text = re.sub(r"xox[abpsr]-[A-Za-z0-9-]{10,}", "[REDACTED_SLACK_TOKEN]", text) + text = re.sub(r"eyJ[A-Za-z0-9_-]{10,2000}\.eyJ[A-Za-z0-9_-]{10,4000}\.[A-Za-z0-9_-]{10,2000}\b", "[REDACTED_JWT]", text) + if text == previous: + return text + raise _gh.TerminalError("reset evidence sanitization did not converge") + + +def sanitized(value): + if isinstance(value, str): + return sanitize(value) + if isinstance(value, list): + return [sanitized(item) for item in value] + if isinstance(value, dict): + return {key: sanitized(item) for key, item in value.items()} + return value + + +def marker(review): + matches = pub.VERDICT_MARKER_PATTERN.findall(review.get("body") or "") + if len(matches) != 1: + return None + try: + value = json.loads(matches[0]) + except ValueError: + return None + return value if isinstance(value, dict) else None + + +def owned(review, identity): + value = marker(review) + return ((review.get("user") or {}).get("login") in _review_state.BOT_LOGINS + and value is not None + and all(value.get(key) == identity[key] for key in REVIEWER_KEYS)) + + +def check_history(item, kind, repo, pr_number): + """Unknown/deleted/human edits retain; visible host-only report edits are OK.""" + require(item.get("node_id"), "missing original edit history") + query = """query($id:ID!) { node(id:$id) { ... on KIND { + id fullDatabaseId body author { login } lastEditedAt editor { login } includesCreatedEdit + pullRequest { number repository { nameWithOwner } } + userContentEdits(first:100) { totalCount + nodes { editor { login } deletedAt deletedBy { login } } + pageInfo { hasNextPage } + } + } } }""".replace("KIND", kind) + node = _gh.graphql(query, {"id": item["node_id"]}).get("node") + require(isinstance(node, dict) and node.get("id") == item["node_id"] + and str(node.get("fullDatabaseId")) == str(item["id"]) + and node.get("body") == item.get("body"), "original history binding mismatch") + binding = node.get("pullRequest") or {} + require(str(binding.get("number")) == str(pr_number) + and (binding.get("repository") or {}).get("nameWithOwner") == repo + and (node.get("author") or {}).get("login") in _review_state.BOT_LOGINS, + "foreign original evidence") + history = node.get("userContentEdits") or {} + edits = history.get("nodes") + require(isinstance(edits, list) and type(history.get("totalCount")) is int + and history["totalCount"] == len(edits) + and (history.get("pageInfo") or {}).get("hasNextPage") is False, + "incomplete original edit history") + require(all(isinstance(edit, dict) and edit.get("deletedAt") is None + and edit.get("deletedBy") is None + and (edit.get("editor") or {}).get("login") in _review_state.BOT_LOGINS + for edit in edits), "deleted/unknown/human original edit") + require(all(key in node for key in ("lastEditedAt", "editor", "includesCreatedEdit")), + "missing original edit metadata") + if kind == "PullRequestReview" or node["lastEditedAt"] is None: + require(not edits and node["lastEditedAt"] is None and node["editor"] is None + and node["includesCreatedEdit"] is False, "formal review edited or unknown history") + else: + require(edits and node["includesCreatedEdit"] is True + and (node["editor"] or {}).get("login") in _review_state.BOT_LOGINS, + "inconsistent original report history") + + +def blocking_findings(body): + """Group canonical top-level items without losing their supporting text.""" + sections = ("### Security Issues", "### Correctness Issues") + groups = [] + current = None + section = None + seen = [] + empty = False + fence_char = None + fence_len = 0 + error = "original blocking findings missing or not count-accounted" + for line in body.splitlines(): + # Preserve fences in a finding, including indented list-item examples. + # Their bullets/headings are evidence, never new findings or sections. + if fence_char is not None: + if section in sections: + require(current is not None, error) + current.append(line) + closer = pub._FENCE_CLOSE_PATTERN.match(line.lstrip(" \t")) + if closer: + delimiter = closer.group(1) + if delimiter[0] == fence_char and len(delimiter) >= fence_len: + fence_char = None + continue + opener = pub._FENCE_OPEN_PATTERN.match(line.lstrip(" \t")) + if opener and not (opener[1][0] == "`" and "`" in opener[2]): + fence_char, fence_len = opener[1][0], len(opener[1]) + if section in sections: + require(current is not None, error) + current.append(line) + continue + if re.match(r"#{1,3}[ \t]+", line): + section = line.strip() + current = None + empty = False + if section in sections: + seen.append(section) + continue + if section not in sections: + continue + if not line.strip(): + if current is not None: + current.append(line) + continue + if line == "None found.": + require(current is None and not empty, error) + empty = True + continue + require(not empty, error) + if re.match(r"(?:[-+*]|\d+[.)])[ \t]+\S", line): + current = [line] + groups.append(current) + else: + # Wrapped prose, nested items and quotes must have a real owner. + require(current is not None, error) + current.append(line) + require(fence_char is None + and sorted(seen) == ["### Correctness Issues", "### Security Issues"] + and groups, error) + return ["\n".join(group).strip() for group in groups] + + +def target_evidence(review, report, identity, repo, pr_number): + require(owned(review, identity), "unknown target ownership") + check_history(review, "PullRequestReview", repo, pr_number) + check_history(report, "IssueComment", repo, pr_number) + require(len(_review_state.REVIEW_STATE_PATTERN.findall(report.get("body", ""))) == 1, + "ambiguous original report marker") + state = _review_state.marker_state(report.get("body", "")) or {} + value = marker(review) + require(state.get("publication") == "completed" + and all(state.get(key) == value.get(key) and value.get(key) + for key in pub.publication_identity()) + and state.get("last_reviewed_sha") == review.get("commit_id"), + "original report/review binding mismatch") + require(pub._parse_ts(state["started_at"]) < pub.run_started_at(), "target is not older") + body = report["body"] + rows = blocking_findings(body) + require(pub.parse_blocking_count(body, identity["summary_marker"]) == len(rows), + "original blocking findings missing or not count-accounted") + return {"id": review["id"], "commit_id": review["commit_id"], + "marker": value, "report_id": report["id"], "findings": rows} + + +def check_pr(pr, snapshot): + context = snapshot["context"] + require(pr.get("state") == "open" and pr["head"]["sha"] == context["current_sha"] + and pr["base"]["sha"] == context["current_base_sha"] + and pr["base"].get("ref") == context["current_base_ref"] + and pr["head"].get("ref") == context["head_ref"] + and (pr["head"].get("repo") or {}).get("full_name") == context["repository"], + "PR closed or reviewed head/base/repository changed") + + +def prepare(): + # Clear only these new owned entries before any fallible preparation. A + # planted workspace snapshot must not masquerade as this run's input. + SNAPSHOT.unlink(missing_ok=True) + host_snapshot = Path(os.environ["REVIEW_RESET_SNAPSHOT"]) + host_snapshot.unlink(missing_ok=True) + context = read_json(pub.PR_CONTEXT_PATH) + identity = pub.publication_identity() + repo, number = context["repository"], context["pr_number"] + require(repo == os.environ["GITHUB_REPOSITORY"] and str(number) == os.environ["PR_NUMBER"] + and context["workflow_ref"] == identity["workflow_ref"] + and context["summary_heading"] == identity["summary_marker"] + and context["current_sha"] == pub.current_head_sha(), "reset context binding mismatch") + snapshot = {"identity": identity, "context": {key: context[key] for key in ( + "repository", "pr_number", "current_sha", "current_base_sha", "current_base_ref")}, + "started_at": pub.run_started_raw(), "targets": [], "audit": [], "retained": []} + pr = _gh.rest("GET", f"repos/{repo}/pulls/{number}") + require(isinstance(pr["head"].get("ref"), str) and pr["head"]["ref"], "missing PR head branch") + snapshot["context"]["head_ref"] = pr["head"]["ref"] + check_pr(pr, snapshot) + reviews = _gh.rest_paginate(f"repos/{repo}/pulls/{number}/reviews") + comments = {item["id"]: item for item in _gh.rest_paginate(f"repos/{repo}/issues/{number}/comments")} + for review in reviews: + if review.get("state") != "CHANGES_REQUESTED": + continue + if not owned(review, identity): + # Human and foreign reviews are not ours, including unmarked legacy bots. + continue + try: + report = comments[marker(review)["report_comment_id"]] + target = target_evidence(review, report, identity, repo, number) + except (_gh.GitHubError, KeyError, TypeError, ValueError) as error: + snapshot["retained"].append({"id": review["id"], "reason": str(error)}) + continue + snapshot["targets"].append(target) + for index, finding in enumerate(target["findings"]): + snapshot["audit"].append({"id": f"review-{review['id']}-{index}", "finding": finding, + "blocking": True}) + # Include every supplied prior entry, not only target-associated or unresolved ones. + prior = read_json(".github/prior-findings.json")["prior_findings"] + require(isinstance(prior, list), "missing current prior-finding inventory") + for index, finding in enumerate(prior): + require(isinstance(finding, dict) and finding.get("excerpt") and finding.get("severity") + in ("security", "bug", "suggestion"), "incomplete prior finding") + snapshot["audit"].append({"id": f"prior-{index}", "finding": finding, + "blocking": finding["severity"] != "suggestion"}) + snapshot = sanitized(snapshot) + write_json(SNAPSHOT, snapshot) + write_json(host_snapshot, snapshot) + name = hashlib.sha256(json.dumps(identity, sort_keys=True).encode()).hexdigest()[:16] + with open(os.environ["GITHUB_OUTPUT"], "a") as output: + output.write(f"has_targets={'true' if snapshot['targets'] else 'false'}\n") + output.write(f"artifact_name=pr-review-reset-{identity['run_id']}-{identity['run_attempt']}-{name}\n") + print(f"Reset snapshot: {len(snapshot['targets'])} eligible, {len(snapshot['retained'])} retained; unmarked legacy reviews require manual dismissal") + for retained in snapshot["retained"]: + reason = " ".join(retained["reason"].split())[:240] + print(f"Retained prior owned blocking review {retained['id']}: {reason}", file=sys.stderr) + + +def validate_proof(proof, snapshot): + fields = {"review_complete", "coverage_gaps", "blocking_issues", "audit"} + require(isinstance(proof, dict) and set(proof) == fields and proof.get("review_complete") is True + and proof.get("coverage_gaps") == [] and type(proof.get("blocking_issues")) is int + and proof["blocking_issues"] == 0, "incomplete coverage or remaining blockers") + audit = proof.get("audit") + require(isinstance(audit, list) and all(isinstance(item, dict) for item in audit), "missing current audit") + expected = {item["id"]: item for item in snapshot["audit"]} + require(len(expected) == len(snapshot["audit"]), "duplicate frozen audit IDs") + require(len(audit) == len(expected) and {item.get("id") for item in audit} == set(expected), + "current audit does not cover every frozen ID exactly once") + for item in audit: + require(set(item) == {"id", "disposition", "evidence"}, "unexpected audit fields") + allowed = ("fixed", "obsolete", "incorrect") if expected[item["id"]]["blocking"] else ("fixed", "obsolete", "incorrect", "still_present") + require(item.get("disposition") in allowed, "blocking/unverified prior obligation remains") + evidence = item.get("evidence") + require(isinstance(evidence, list) and evidence and all(isinstance(entry, dict) + and set(entry) == {"path", "location", "explanation"} + and all(isinstance(entry.get(key), str) and entry[key].strip() + for key in ("path", "location", "explanation")) for entry in evidence), + "missing affirmative current-code evidence") + return proof + + +def capture(): + snapshot = read_json(os.environ["REVIEW_RESET_SNAPSHOT"]) + require(snapshot["targets"], "no eligible targets need reset proof") + require(snapshot["identity"] == pub.publication_identity(), "stale reset snapshot") + messages = read_json(os.environ["REVIEW_EXECUTION_FILE"], 32 * 1024 * 1024) + require(isinstance(messages, list) and messages and isinstance(messages[-1], dict), "missing terminal runner result") + terminal = messages[-1] + session = os.environ.get("REVIEW_SESSION_ID") + init = [item for item in messages if isinstance(item, dict) and item.get("type") == "system" and item.get("subtype") == "init"] + require(session and len(init) == 1 and init[0].get("session_id") == session + and terminal.get("session_id") == session and terminal.get("type") == "result" + and terminal.get("subtype") == "success" and terminal.get("is_error") is False, + "unsuccessful or unbound terminal result") + text = terminal.get("result") + require(isinstance(text, str) and text.count("```review-reset-proof") == 1, "missing/ambiguous optional terminal proof") + match = PROOF_PATTERN.search(text) + require(match is not None, "malformed terminal proof") + # Parse via the same duplicate-key rejecting reader without a second evidence file. + def pairs(items): + result = {} + for key, value in items: + require(key not in result, "duplicate proof key") + result[key] = value + return result + proof = validate_proof(sanitized(json.loads(match[1], object_pairs_hook=pairs)), snapshot) + accepted = {"snapshot": snapshot, "proof": proof, "completion": {"session_id": session, "terminal_success": True}} + require(len(json.dumps(accepted).encode()) <= 2 * 1024 * 1024, "accepted reset evidence exceeds size limit") + path = Path(os.environ["REVIEW_RESET_FILE"]) + write_json(path, accepted) + digest = hashlib.sha256(path.read_bytes()).hexdigest() + with open(os.environ["GITHUB_OUTPUT"], "a") as output: + output.write(f"sha256={digest}\n") + + +def check_newer_runs(repo, number, snapshot): + identity = snapshot["identity"] + run = _gh.rest("GET", f"repos/{repo}/actions/runs/{identity['run_id']}") + require(str(run.get("run_attempt")) == identity["run_attempt"], "newer attempt supersedes this run") + workflow_url = run.get("workflow_url") + expected = re.escape(f"{_gh.API_ROOT}/repos/{repo}/actions/") + r"(?:workflows|required_workflows)/[0-9]+" + require(isinstance(workflow_url, str) and re.fullmatch(expected, workflow_url), + "missing or foreign workflow inventory URL") + branch = quote(snapshot["context"]["head_ref"], safe="") + pages = _gh.rest_paginate(f"{workflow_url}/runs?branch={branch}") + require(pages and all(isinstance(page, dict) and isinstance(page.get("workflow_runs"), list) + and type(page.get("total_count")) is int for page in pages), + "missing competing-run inventory") + entries = [item for page in pages for item in page["workflow_runs"]] + require(len({item["id"] for item in entries}) == len(entries), + "duplicate competing-run inventory") + require(all(page["total_count"] == len(entries) for page in pages), + "incomplete competing-run inventory") + started = pub._parse_ts(snapshot["started_at"]) + for other in entries: + if str(other.get("id")) == identity["run_id"]: + continue + require(pub._parse_ts(other["run_started_at"]) <= started, "newer branch review run exists") + + +def dismiss(accepted): + snapshot = accepted["snapshot"] + identity = pub.publication_identity() + require(snapshot["identity"] == identity and snapshot["started_at"] == pub.run_started_raw() + and accepted["completion"].get("terminal_success") is True, + "accepted evidence does not belong to this completed run") + validate_proof(accepted["proof"], snapshot) + repo, number = snapshot["context"]["repository"], snapshot["context"]["pr_number"] + require(repo == os.environ["GITHUB_REPOSITORY"] and str(number) == os.environ["PR_NUMBER"], "reset repository/PR mismatch") + if not snapshot["targets"]: + print("No positively owned eligible blocking reviews to dismiss") + return + comments = _gh.rest_paginate(f"repos/{repo}/issues/{number}/comments") + report = pub.find_published_report(pub.summary_comments(comments, identity["summary_marker"]), identity, snapshot["context"]["current_sha"]) + require(report is not None and not pub.is_pending(report), "ordinary publication did not complete") + reviews = _gh.rest_paginate(f"repos/{repo}/pulls/{number}/reviews") + current, conflict = pub.find_verdict_review(reviews, identity, report, snapshot["context"]["current_sha"], "COMMENT") + require(current is not None and conflict is None, "current publication is not a clean COMMENT") + for target in snapshot["targets"]: + check_pr(_gh.rest("GET", f"repos/{repo}/pulls/{number}"), snapshot) + check_newer_runs(repo, number, snapshot) + live_reviews = _gh.rest_paginate(f"repos/{repo}/pulls/{number}/reviews") + matched, conflict = pub.find_verdict_review( + live_reviews, identity, report, snapshot["context"]["current_sha"], "COMMENT" + ) + require(matched is not None and matched["id"] == current["id"] and conflict is None, + "current clean publication disappeared or changed") + require(not any(owned(item, identity) and item.get("id") != current["id"] + and pub._parse_ts(item["submitted_at"]) >= pub._parse_ts(snapshot["started_at"]) + for item in live_reviews), "competing publication exists") + review = _gh.rest("GET", f"repos/{repo}/pulls/{number}/reviews/{target['id']}") + require(review.get("id") == target["id"] and owned(review, identity) + and marker(review) == target["marker"] and review.get("commit_id") == target["commit_id"], + "frozen target changed ownership/binding") + if review.get("state") == "DISMISSED": + continue # Never recreate a human-dismissed block; retry is a no-op. + require(review.get("state") == "CHANGES_REQUESTED", "target no longer blocking") + original = _gh.rest("GET", f"repos/{repo}/issues/comments/{target['report_id']}") + require(original.get("id") == target["report_id"], "original report ID mismatch") + require(target_evidence(review, original, identity, repo, number) == target, "original obligation set changed") + check_pr(_gh.rest("GET", f"repos/{repo}/pulls/{number}"), snapshot) + path = f"repos/{repo}/pulls/{number}/reviews/{target['id']}/dismissals" + try: + _gh.rest("PUT", path, data={"message": "Superseded by a completed review with no blocking issues; current audit evidence persisted in CI.", "event": "DISMISS"}, max_attempts=1) + except _gh.TransientOutageError: + print("::warning::Ambiguous dismissal response; reading the exact target") + result = _gh.rest("GET", f"repos/{repo}/pulls/{number}/reviews/{target['id']}") + require(result.get("id") == target["id"] and result.get("state") == "DISMISSED" + and owned(result, identity) and marker(result) == target["marker"] + and result.get("commit_id") == target["commit_id"], + "dismissal did not reconcile to the exact target") + print(f"Dismissed prior owned blocking review {target['id']}") + + +def reset(): + # The action passes upload OUTCOME, not the continue-on-error conclusion. + require(os.environ.get("REVIEW_RESET_UPLOAD_OUTCOME") == "success" + and os.environ.get("REVIEW_RESET_ARTIFACT_ID", "").isdigit() + and re.fullmatch(r"(?:sha256:)?[0-9a-f]{64}", os.environ.get("REVIEW_RESET_ARTIFACT_DIGEST", "")), + "accepted proof upload did not succeed") + path = Path(os.environ["REVIEW_RESET_FILE"]) + require(hashlib.sha256(path.read_bytes()).hexdigest() == os.environ.get("REVIEW_RESET_SHA256"), + "accepted evidence changed after upload") + dismiss(read_json(path)) + + +def main(): + try: + {"prepare": prepare, "capture": capture, "dismiss": reset}[sys.argv[1]]() + except (_gh.GitHubError, OSError, KeyError, TypeError, ValueError) as error: + # Reset-only failure: no impact on the baseline publisher or its state. + print(f"::warning::Prior reviews retained: {sanitize(str(error))}", file=sys.stderr) + sys.exit(1) + + +if __name__ == "__main__": + main() diff --git a/.github/actions/pr-review/scripts/test_review_reset.py b/.github/actions/pr-review/scripts/test_review_reset.py new file mode 100755 index 0000000..4330666 --- /dev/null +++ b/.github/actions/pr-review/scripts/test_review_reset.py @@ -0,0 +1,746 @@ +#!/usr/bin/env python3 +"""Reset-only fixtures; not live workflow, model-quality or token-isolation proof.""" + +import copy +import hashlib +import importlib.util +import io +import json +import os +from pathlib import Path +import tempfile +import unittest +from unittest import mock + +import _gh +import test_verdict_scaffolding as baseline + +_spec = importlib.util.spec_from_file_location("review_reset", Path(__file__).with_name("review-reset.py")) +reset = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(reset) +pub = reset.pub + + +class ResetTest(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.directory = Path(self.temp.name) + self.env = dict(baseline.ENV, GITHUB_OUTPUT=str(self.directory / "outputs"), + REVIEW_RESET_FILE=str(self.directory / "accepted.json"), + REVIEW_RESET_SNAPSHOT=str(self.directory / "host-snapshot.json"), + REVIEW_EXECUTION_FILE=str(self.directory / "execution.json"), + REVIEW_SESSION_ID="current-session") + self.comments = {} + self.reviews = {} + self.calls = [] + self.history_edits = {} + self.ambiguous = False + self.ambiguous_lands = True + self.pr = {"state": "open", "head": {"sha": baseline.HEAD, "ref": "topic", "repo": {"full_name": "example/repo"}}, + "base": {"sha": baseline.BASE, "ref": "main"}} + self.runs = [] + self.inventory_incomplete = False + self.run_attempt = baseline.RUN_ATTEMPT + self.workflow_url = f"{_gh.API_ROOT}/repos/example/repo/actions/workflows/50" + self.context = {"repository": "example/repo", "pr_number": "42", "current_sha": baseline.HEAD, + "current_base_sha": baseline.BASE, "current_base_ref": "main", + "workflow_ref": baseline.WORKFLOW_REF, "summary_heading": baseline.HEADING, + "existing_findings": ["- `file.go:7`: A prior issue"]} + self.prior = [{"path": "file.go", "line": 7, "severity": "bug", "excerpt": "🟠 Bug: Original failure", + "thread_resolved": True, "thread_outdated": True}, + {"path": "file.go", "line": 12, "severity": "suggestion", "excerpt": "🟡 Suggestion: Consider a test", + "thread_resolved": False, "thread_outdated": False}] + self.old_review = self.add_old(10, 1) + reset.write_json(self.directory / "context.json", self.context) + self.real_read = reset.read_json + for patch in ( + mock.patch.dict(os.environ, self.env), + mock.patch.object(reset, "SNAPSHOT", self.directory / "snapshot.json"), + mock.patch.object(pub, "PR_CONTEXT_PATH", self.directory / "context.json"), + mock.patch.object(pub, "current_head_sha", return_value=baseline.HEAD), + mock.patch.object(pub, "current_base_sha", return_value=baseline.BASE), + mock.patch.object(_gh, "rest", side_effect=self.rest), + mock.patch.object(_gh, "rest_paginate", side_effect=self.paginate), + mock.patch.object(_gh, "graphql", side_effect=self.graphql), + mock.patch.object(reset, "read_json", side_effect=self.read), + ): + patch.start() + self.addCleanup(patch.stop) + + def read(self, path, *args): + if str(path) == ".github/prior-findings.json": + return {"prior_findings": copy.deepcopy(self.prior)} + return self.real_read(path, *args) + + def add_old(self, review_id, report_id, findings=None): + findings = findings or ["- **New** `file.go:7`: dereference can panic"] + identity = dict(baseline.IDENTITY, run_id=str(100 + review_id), run_attempt="1") + body = (baseline.working_body(len(findings)) + "\n### Security Issues\nNone found.\n" + + "### Correctness Issues\n" + "\n".join(findings) + "\n### Suggestions\nNone.\n") + working = {"id": 80 + report_id, "body": body, "updated_at": baseline.T1} + report = {"id": report_id, "node_id": f"C{report_id}", "user": {"login": "github-actions[bot]"}, + "body": pub.compose_report_body(working, identity, baseline.HEAD, baseline.BASE, baseline.T1), + "updated_at": baseline.T1, "html_url": f"https://example/report/{report_id}"} + state = reset._review_state.marker_state(report["body"]) + state["publication"] = "completed" + report["body"] = reset._review_state.REVIEW_STATE_PATTERN.sub(lambda m: f"", report["body"]) + value = dict(identity, report_comment_id=report_id) + review = {"id": review_id, "node_id": f"R{review_id}", "user": {"login": "github-actions[bot]"}, + "commit_id": baseline.HEAD, "state": "CHANGES_REQUESTED", "submitted_at": baseline.T1, + "body": f"Blocking issues found\n"} + self.comments[report_id] = report + self.reviews[review_id] = review + return review + + def paginate(self, path, **kwargs): + self.calls.append(("LIST", path, None)) + if path.endswith("/comments"): + return copy.deepcopy(list(self.comments.values())) + if path.endswith("/reviews"): + return copy.deepcopy(list(self.reviews.values())) + if "/actions/workflows/" in path or "/actions/required_workflows/" in path: + return [{"workflow_runs": copy.deepcopy(self.runs), + "total_count": len(self.runs) + int(self.inventory_incomplete)}] + self.fail(path) + + def rest(self, method, path, **kwargs): + data = kwargs.get("data") + self.calls.append((method, path, copy.deepcopy(data))) + if method == "GET": + if path.endswith("/pulls/42"): + return copy.deepcopy(self.pr) + if "/actions/runs/" in path: + return {"workflow_id": 50, "workflow_url": self.workflow_url, "run_attempt": self.run_attempt} + if "/actions/workflows/" in path: + return {"workflow_runs": copy.deepcopy(self.runs)} + if "/issues/comments/" in path: + return copy.deepcopy(self.comments[int(path.rsplit("/", 1)[1])]) + if "/reviews/" in path: + return copy.deepcopy(self.reviews[int(path.rsplit("/", 1)[1])]) + if method == "POST" and path.endswith("/comments"): + report = {"id": 900, "node_id": "C900", "user": {"login": "github-actions[bot]"}, + "body": data["body"], "updated_at": baseline.FRESH, "html_url": "https://example/report/900"} + self.comments[900] = report + return copy.deepcopy(report) + if method == "POST" and path.endswith("/reviews"): + review = {"id": 1000, "node_id": "R1000", "user": {"login": "github-actions[bot]"}, + "body": data["body"], "state": pub.REVIEW_STATE_FOR_EVENT[data["event"]], + "commit_id": data["commit_id"], "submitted_at": baseline.FRESH} + self.reviews[1000] = review + return copy.deepcopy(review) + if method == "PATCH": + report_id = int(path.rsplit("/", 1)[1]) + self.comments[report_id]["body"] = data["body"] + self.history_edits[self.comments[report_id]["node_id"]] = [{"editor": {"login": "github-actions[bot]"}, "deletedAt": None, "deletedBy": None}] + return copy.deepcopy(self.comments[report_id]) + if method == "PUT": + self.assertEqual(kwargs.get("max_attempts"), 1) + review_id = int(path.split("/")[-2]) + if not self.ambiguous or self.ambiguous_lands: + self.reviews[review_id]["state"] = "DISMISSED" + if self.ambiguous: + raise _gh.TransientOutageError("response lost") + return copy.deepcopy(self.reviews[review_id]) + self.fail(f"{method} {path}") + + def graphql(self, query, variables): + item = next(item for item in list(self.comments.values()) + list(self.reviews.values()) + if item["node_id"] == variables["id"]) + edits = self.history_edits.get(item["node_id"], []) + return {"node": {"id": item["node_id"], "fullDatabaseId": str(item["id"]), "body": item["body"], + "author": item["user"], "lastEditedAt": baseline.FRESH if edits else None, + "editor": edits[-1]["editor"] if edits else None, "includesCreatedEdit": bool(edits), + "pullRequest": {"number": 42, "repository": {"nameWithOwner": "example/repo"}}, + "userContentEdits": {"totalCount": len(edits), "nodes": edits, "pageInfo": {"hasNextPage": False}}}} + + def freeze(self): + reset.prepare() + return self.real_read(reset.SNAPSHOT) + + def proof(self, snapshot): + return {"review_complete": True, "coverage_gaps": [], "blocking_issues": 0, + "audit": [{"id": entry["id"], "disposition": "fixed", + "evidence": [{"path": "file.go", "location": "7-12", "explanation": "Current nil guard resolves the failure"}]} + for entry in snapshot["audit"]]} + + def capture(self, snapshot, proof=None, terminal_extra=None): + terminal = {"type": "result", "subtype": "success", "is_error": False, + "session_id": "current-session", + "result": "Review finished.\n```review-reset-proof\n" + json.dumps(proof or self.proof(snapshot)) + "\n```"} + terminal.update(terminal_extra or {}) + reset.write_json(self.env["REVIEW_EXECUTION_FILE"], [{"type": "system", "subtype": "init", "session_id": "current-session"}, terminal]) + reset.capture() + path = Path(self.env["REVIEW_RESET_FILE"]) + os.environ.update(REVIEW_RESET_SHA256=hashlib.sha256(path.read_bytes()).hexdigest(), + REVIEW_RESET_UPLOAD_OUTCOME="success", REVIEW_RESET_ARTIFACT_ID="777", + REVIEW_RESET_ARTIFACT_DIGEST="b" * 64) + self.calls.append(("UPLOAD", "accepted.json", None)) + return self.real_read(path) + + def publish(self, blockers=0, provisional=False): + self.comments[2] = {"id": 2, "node_id": "C2", "user": {"login": "github-actions[bot]"}, + "body": baseline.working_body(blockers, provisional=provisional), "updated_at": baseline.FRESH} + pub._run() + + def puts(self): + return [call for call in self.calls if call[0] == "PUT"] + + def test_complete_current_audit_dismisses_only_exact_targets_after_publication(self): + self.add_old(11, 3) + snapshot = self.freeze() + self.assertEqual({entry["id"] for entry in snapshot["audit"]}, {"review-10-0", "review-11-0", "prior-0", "prior-1"}) + accepted = self.capture(snapshot) + self.assertEqual(set(accepted), {"snapshot", "proof", "completion"}) + self.assertEqual(set(accepted["proof"]), {"review_complete", "coverage_gaps", "blocking_issues", "audit"}) + self.assertEqual(accepted["snapshot"]["identity"], pub.publication_identity()) + self.assertEqual(accepted["snapshot"]["context"]["current_sha"], baseline.HEAD) + self.assertEqual(accepted["snapshot"]["context"]["current_base_sha"], baseline.BASE) + self.assertEqual(accepted["completion"], {"session_id": "current-session", "terminal_success": True}) + self.assertFalse(self.puts()) + self.publish() + reset.reset() + self.assertEqual([call[1] for call in self.puts()], ["repos/example/repo/pulls/42/reviews/10/dismissals", "repos/example/repo/pulls/42/reviews/11/dismissals"]) + first_put = next(index for index, call in enumerate(self.calls) if call[0] == "PUT") + self.assertTrue(all(index < first_put for index, call in enumerate(self.calls) if call[0] in ("UPLOAD", "POST", "PATCH"))) + self.assertEqual(self.reviews[1000]["state"], "COMMENTED") + self.assertEqual(reset._review_state.marker_state(self.comments[900]["body"])["publication"], "completed") + reset.reset() # Exact same attempt retry does not recreate or re-dismiss. + self.assertEqual(len(self.puts()), 2) + + def test_no_prior_block_is_noop(self): + self.reviews.clear() + snapshot = self.freeze() + self.assertEqual(snapshot["targets"], []) + self.assertIn("has_targets=false\n", Path(self.env["GITHUB_OUTPUT"]).read_text()) + with self.assertRaises(_gh.TerminalError): + reset.capture() + self.assertFalse(Path(self.env["REVIEW_RESET_FILE"]).exists()) + self.publish() + self.assertEqual(self.reviews[1000]["state"], "COMMENTED") + self.assertFalse(self.puts()) + + def test_reset_failures_do_not_break_ordinary_publication(self): + snapshot = self.freeze() + for extra in ({"result": "No optional proof"}, {"result": "```review-reset-proof\n{}\n```"}, + {"subtype": "error_max_turns"}, {"session_id": "old-session"}, + {"is_error": True}, {"type": "assistant"}): + with self.subTest(extra=extra), self.assertRaises((_gh.TerminalError, ValueError)): + self.capture(snapshot, terminal_extra=extra) + self.publish() + self.assertEqual(self.reviews[1000]["state"], "COMMENTED") + self.assertEqual(reset._review_state.marker_state(self.comments[900]["body"])["publication"], "completed") + self.assertFalse(self.puts()) + + def test_failed_prepare_retains_and_publisher_still_requests_changes(self): + self.prior = None + with self.assertRaises(_gh.TerminalError): + self.freeze() + self.publish(1) + self.assertEqual(self.reviews[1000]["state"], "CHANGES_REQUESTED") + self.assertFalse(self.puts()) + + def test_every_audit_id_and_evidence_is_required(self): + snapshot = self.freeze() + proof = self.proof(snapshot) + mutations = [] + for key, value in (("review_complete", False), ("coverage_gaps", ["unread diff"]), ("blocking_issues", 1), + ("extra", "secret")): + changed = copy.deepcopy(proof) + changed[key] = value + mutations.append(changed) + for change in ("missing", "duplicate", "unknown", "unverified", "still_present", "empty_evidence", "empty_explanation"): + changed = copy.deepcopy(proof) + if change == "missing": + changed["audit"].pop() + elif change == "duplicate": + changed["audit"][1] = changed["audit"][0] + elif change == "unknown": + changed["audit"][0]["id"] = "unfrozen" + elif change in ("unverified", "still_present"): + changed["audit"][0]["disposition"] = change + elif change == "empty_evidence": + changed["audit"][0]["evidence"] = [] + else: + changed["audit"][0]["evidence"][0]["explanation"] = "" + mutations.append(changed) + for changed in mutations: + with self.subTest(proof=changed), self.assertRaises(_gh.TerminalError): + reset.validate_proof(changed, snapshot) + # An audited known suggestion need not be fixed to clear blocking reviews. + proof["audit"][2]["disposition"] = "still_present" + reset.validate_proof(proof, snapshot) + + def test_model_supplied_provenance_is_rejected_even_when_correct(self): + snapshot = self.freeze() + provenance = {"run_id": baseline.RUN_ID, "run_attempt": baseline.RUN_ATTEMPT, + "head_sha": baseline.HEAD, "base_sha": baseline.BASE} + for key, value in provenance.items(): + for supplied in (value, "incorrect"): + proof = dict(self.proof(snapshot), **{key: supplied}) + with self.subTest(key=key, value=supplied), self.assertRaises(_gh.TerminalError): + self.capture(snapshot, proof) + with self.assertRaises(_gh.TerminalError): + self.capture(snapshot, dict(self.proof(snapshot), **provenance)) + self.assertFalse(Path(self.env["REVIEW_RESET_FILE"]).exists()) + self.assertFalse(self.puts()) + + def test_capture_requires_current_host_run_identity(self): + snapshot = self.freeze() + for key in ("run_id", "run_attempt"): + changed = copy.deepcopy(snapshot) + changed["identity"][key] = "old" + reset.write_json(self.env["REVIEW_RESET_SNAPSHOT"], changed) + with self.subTest(key=key), self.assertRaises(_gh.TerminalError): + self.capture(snapshot) + self.assertFalse(Path(self.env["REVIEW_RESET_FILE"]).exists()) + self.assertFalse(self.puts()) + + def test_capture_requires_exact_runner_init_session(self): + snapshot = self.freeze() + terminal = {"type": "result", "subtype": "success", "is_error": False, + "session_id": "current-session", + "result": "```review-reset-proof\n" + json.dumps(self.proof(snapshot)) + "\n```"} + init = {"type": "system", "subtype": "init", "session_id": "current-session"} + for initials in ([], [dict(init, session_id="old-session")], [init, init]): + reset.write_json(self.env["REVIEW_EXECUTION_FILE"], initials + [terminal]) + with self.subTest(initials=initials), self.assertRaises(_gh.TerminalError): + reset.capture() + reset.write_json(self.env["REVIEW_EXECUTION_FILE"], [init, terminal]) + with mock.patch.dict(os.environ, REVIEW_SESSION_ID=""), self.assertRaises(_gh.TerminalError): + reset.capture() + self.assertFalse(Path(self.env["REVIEW_RESET_FILE"]).exists()) + self.assertFalse(self.puts()) + + def test_late_dismissal_requires_ci_owned_envelope_provenance(self): + accepted = self.capture(self.freeze()) + self.publish() + mutations = [ + ("identity", "run_id", "old"), ("identity", "run_attempt", "old"), + ("context", "current_sha", "old"), ("context", "current_base_sha", "old"), + ("context", "repository", "foreign/repo"), ("context", "pr_number", "99"), + ] + for section, key, value in mutations: + changed = copy.deepcopy(accepted) + changed["snapshot"][section][key] = value + with self.subTest(section=section, key=key), self.assertRaises(_gh.TerminalError): + reset.dismiss(changed) + changed = copy.deepcopy(accepted) + changed["snapshot"]["started_at"] = baseline.T1 + with self.assertRaises(_gh.TerminalError): + reset.dismiss(changed) + changed = copy.deepcopy(accepted) + changed["completion"]["terminal_success"] = False + with self.assertRaises(_gh.TerminalError): + reset.dismiss(changed) + self.assertFalse(self.puts()) + + def test_upload_failure_or_changed_accepted_file_retains_after_clean_publication(self): + self.capture(self.freeze()) + self.publish() + for extra in ({"REVIEW_RESET_UPLOAD_OUTCOME": "failure"}, {"REVIEW_RESET_ARTIFACT_ID": ""}, + {"REVIEW_RESET_ARTIFACT_DIGEST": ""}, {"REVIEW_RESET_SHA256": "c" * 64}): + with self.subTest(extra=extra), mock.patch.dict(os.environ, extra), self.assertRaises(_gh.TerminalError): + reset.reset() + self.assertEqual(self.reviews[1000]["state"], "COMMENTED") + self.assertFalse(self.puts()) + + def test_new_blocking_review_and_incomplete_summary_never_reset(self): + accepted = self.capture(self.freeze()) + with self.assertRaises(SystemExit): + self.publish(provisional=True) + self.assertNotIn(1000, self.reviews) + self.publish(1) + with self.assertRaises(_gh.TerminalError): + reset.dismiss(accepted) + self.assertFalse(self.puts()) + + def test_unknown_human_foreign_targets_are_not_adopted(self): + unknown = self.add_old(20, 3) + unknown["body"] = "legacy unmarked review" + human = self.add_old(21, 4) + human["user"] = {"login": "maintainer"} + foreign = self.add_old(22, 5) + foreign["body"] = foreign["body"].replace(baseline.HEADING, baseline.rs.GENERAL_REVIEW_SUMMARY_HEADING) + self.assertEqual([target["id"] for target in self.freeze()["targets"]], [10]) + self.capture(self.real_read(reset.SNAPSHOT)) + self.publish() + reset.reset() + self.assertEqual({review_id for review_id, review in self.reviews.items() if review["state"] == "DISMISSED"}, {10}) + + def test_nested_supporting_bullets_are_one_complete_finding(self): + finding = ("- **New** `handler.go:8`: missing input validation\n" + " - Empty input reaches the handler.\n" + " - The parser accepts the empty value.\n" + " - No guard checks the result.\n" + " - The caller then dereferences it.") + self.add_old(10, 1, [finding]) + snapshot = self.freeze() + self.assertEqual(snapshot["retained"], []) + self.assertEqual(snapshot["targets"][0]["findings"], [finding]) + self.assertEqual(snapshot["audit"][0], + {"id": "review-10-0", "finding": finding, "blocking": True}) + self.capture(snapshot) + self.publish() + reset.reset() + self.assertEqual(self.reviews[10]["state"], "DISMISSED") + + def test_multiple_findings_keep_their_own_continuations(self): + findings = ["- **New** `handler.go:8`: missing validation\n" + " Wrapped explanation of the failing path.\n\n" + "#### Failure details\n" + " - Supporting observation for the first issue.", + "- **New** `cache.go:12`: stale value reused\n" + "The explanation continues on an unindented line.\n" + " - Supporting observation for the second issue."] + self.add_old(10, 1, findings) + snapshot = self.freeze() + self.assertEqual(snapshot["retained"], []) + self.assertEqual(snapshot["targets"][0]["findings"], findings) + self.assertEqual(snapshot["audit"][:2], + [{"id": f"review-10-{index}", "finding": finding, "blocking": True} + for index, finding in enumerate(findings)]) + accepted = self.capture(snapshot) + self.publish() + self.comments[1]["body"] = self.comments[1]["body"].replace( + "Supporting observation for the second issue.", "Changed supporting obligation.") + with self.assertRaises(_gh.TerminalError): + reset.dismiss(accepted) + self.assertFalse(self.puts()) + + def test_one_line_findings_keep_schema_text_and_audit_ids(self): + findings = ["- **New** `handler.go:8`: missing authorization", + "- **New** `cache.go:12`: stale value reused"] + self.add_old(10, 1, findings) + body = self.comments[1]["body"] + body = body.replace("### Security Issues\nNone found.", + "### Security Issues\n" + findings[0]) + body = body.replace("### Correctness Issues\n" + "\n".join(findings), + "### Correctness Issues\n" + findings[1]) + self.comments[1]["body"] = body + snapshot = self.freeze() + target = snapshot["targets"][0] + self.assertEqual(set(target), {"id", "commit_id", "marker", "report_id", "findings"}) + self.assertEqual(target["findings"], findings) + self.assertEqual(snapshot["audit"][:2], + [{"id": f"review-10-{index}", "finding": finding, "blocking": True} + for index, finding in enumerate(findings)]) + + def test_fenced_and_quoted_bullets_remain_supporting_evidence(self): + for opener, closer in (("```text", "```"), ("~~~~", "~~~~"), + (" ```text", " ```")): + finding = ("- **New** `handler.go:8`: unsafe example is accepted\n" + " Reproduction:\n" + opener + "\n" + "- This example bullet is not a finding.\n" + "### Security Issues\n" + "```not-a-closer\n" + closer + "\n" + "> - Quoted output explains the failure.\n" + "> ### Correctness Issues\n" + " The example and quote are relevant to this issue.") + self.add_old(10, 1, [finding]) + with self.subTest(opener=opener): + snapshot = self.freeze() + self.assertEqual(snapshot["retained"], []) + self.assertEqual(snapshot["targets"][0]["findings"], [finding]) + self.assertEqual(snapshot["audit"][0]["finding"], finding) + + def test_count_mismatch_and_ambiguous_original_sections_retain(self): + original = self.comments[1]["body"] + finding = "- **New** `file.go:7`: dereference can panic" + variants = { + "zero_count": original.replace(baseline.count_row(1), baseline.count_row(0)), + "count_mismatch": original.replace(baseline.count_row(1), baseline.count_row(2)), + "duplicate_section": original.replace("### Suggestions", "### Correctness Issues\n### Suggestions"), + "missing_section": original.replace("### Security Issues", "### Other Issues"), + "orphan_prose": original.replace(finding, "Unaccounted issue text.\n" + finding), + "orphan_nested_item": original.replace(finding, " - An indented issue has no parent."), + "quoted_only": original.replace(finding, "> - A quote is not an original finding."), + "fenced_only": original.replace(finding, "```\n- An example is not an original finding.\n```"), + "mixed_empty": original.replace(finding, "None found.\n" + finding), + "trailing_empty": original.replace(finding, finding + "\nNone found."), + "unclosed_fence": original.replace(finding, finding + "\n```text\n- Example"), + } + for name, body in variants.items(): + self.comments[1]["body"] = body + with self.subTest(name=name), mock.patch("sys.stderr", new_callable=io.StringIO) as output: + snapshot = self.freeze() + self.assertEqual(snapshot["targets"], []) + self.assertEqual(snapshot["retained"], + [{"id": 10, "reason": "original blocking findings missing or not count-accounted"}]) + self.assertIn("Retained prior owned blocking review 10: " + "original blocking findings missing or not count-accounted", output.getvalue()) + self.assertFalse(self.puts()) + + def test_prepare_retained_reason_is_redacted_single_line_and_bounded(self): + token = "ghp_" + "a" * 36 + error = _gh.TerminalError("original guard failed: " + token + "\n" + "detail " * 100) + with mock.patch.object(reset, "target_evidence", side_effect=error), \ + mock.patch("sys.stderr", new_callable=io.StringIO) as output: + snapshot = self.freeze() + self.assertEqual(snapshot["targets"], []) + self.assertEqual(snapshot["retained"][0]["id"], 10) + self.assertNotIn(token, output.getvalue()) + self.assertNotIn(token, json.dumps(snapshot)) + lines = output.getvalue().splitlines() + self.assertEqual(len(lines), 1) + prefix = "Retained prior owned blocking review 10: " + self.assertTrue(lines[0].startswith(prefix + "original guard failed: [REDACTED_GITHUB_TOKEN]")) + self.assertEqual(len(lines[0]) - len(prefix), 240) + + def test_original_missing_edited_or_unaccounted_evidence_never_vacuously_succeeds(self): + original = copy.deepcopy(self.comments[1]) + for change in ("missing", "human_edit", "deleted_edit", "review_edit", "missing_findings", "missing_history"): + self.comments[1] = copy.deepcopy(original) + self.history_edits.clear() + patch = mock.patch.object(_gh, "graphql", side_effect=self.graphql) + if change == "missing": + del self.comments[1] + elif change in ("human_edit", "deleted_edit", "review_edit"): + self.history_edits["R10" if change == "review_edit" else "C1"] = [{"editor": {"login": "human" if change == "human_edit" else "github-actions[bot]"}, + "deletedAt": baseline.FRESH if change == "deleted_edit" else None, "deletedBy": None}] + elif change == "missing_findings": + self.comments[1]["body"] = self.comments[1]["body"].replace("- **New** `file.go:7`: dereference can panic", "None found.") + else: + patch = mock.patch.object(_gh, "graphql", return_value={"node": None}) + with self.subTest(change=change), patch: + snapshot = self.freeze() + self.assertEqual(snapshot["targets"], []) + self.assertEqual([entry["id"] for entry in snapshot["retained"]], [10]) + + def test_stale_head_base_and_newer_run_retain(self): + accepted = self.capture(self.freeze()) + self.publish() + for change in ("head", "base", "base_ref", "newer_run", "newer_attempt", "inventory"): + pr = copy.deepcopy(self.pr) + self.runs = [] + self.inventory_incomplete = False + self.run_attempt = baseline.RUN_ATTEMPT + if change in ("head", "base"): + self.pr[change]["sha"] = "changed" + elif change == "base_ref": + self.pr["base"]["ref"] = "other" + elif change == "newer_run": + self.runs = [{"id": 9999, "head_sha": baseline.HEAD, "pull_requests": [{"number": 42}], "run_started_at": baseline.T2}] + elif change == "newer_attempt": + self.run_attempt = "3" + else: + self.inventory_incomplete = True + with self.subTest(change=change), self.assertRaises(_gh.TerminalError): + reset.dismiss(accepted) + self.pr = pr + self.assertFalse(self.puts()) + + def test_original_changed_after_proof_retains(self): + accepted = self.capture(self.freeze()) + self.publish() + self.comments[1]["body"] = self.comments[1]["body"].replace("dereference can panic", "different obligation") + with self.assertRaises(_gh.TerminalError): + reset.dismiss(accepted) + self.assertFalse(self.puts()) + + def test_ambiguous_response_reconciles_exact_id_without_retry(self): + self.capture(self.freeze()) + self.publish() + self.ambiguous = True + reset.reset() + self.assertEqual(self.reviews[10]["state"], "DISMISSED") + self.assertEqual(len(self.puts()), 1) + + def test_ambiguous_response_without_dismissal_retains(self): + self.capture(self.freeze()) + self.publish() + self.ambiguous = True + self.ambiguous_lands = False + with self.assertRaises(_gh.TerminalError): + reset.reset() + self.assertEqual(self.reviews[10]["state"], "CHANGES_REQUESTED") + self.assertEqual(len(self.puts()), 1) + + def test_sanitization_redacts_all_new_proof_and_snapshot_strings(self): + token = "ghp_" + "a" * 36 + self.prior[0]["excerpt"] += token + snapshot = self.freeze() + proof = self.proof(snapshot) + proof["audit"][0]["evidence"][0]["explanation"] += token + "" + accepted = self.capture(snapshot, proof) + raw = Path(self.env["REVIEW_RESET_FILE"]).read_text() + self.assertNotIn(token, raw) + self.assertNotIn("", raw) + self.assertIn("[REDACTED_GITHUB_TOKEN]", raw) + reset.validate_proof(accepted["proof"], snapshot) + + def test_scraped_suggestions_and_fenced_context_do_not_block_reset(self): + suggestion = "- **Prior — still present** `file.go:12`: consider a test" + self.context["existing_findings"] = [suggestion, "- Around line 12: Add a test in `file.go`."] + reset.write_json(self.directory / "context.json", self.context) + self.comments[1]["body"] = self.comments[1]["body"].replace( + "### Suggestions\nNone.", "### Suggestions\n" + suggestion + + "\n```\n- Around line 12: Add a test in `file.go`.\n```" + ) + snapshot = self.freeze() + self.assertEqual({entry["id"] for entry in snapshot["audit"]}, {"review-10-0", "prior-0", "prior-1"}) + self.assertEqual(self.real_read(self.directory / "context.json")["existing_findings"], self.context["existing_findings"]) + proof = self.proof(snapshot) + next(item for item in proof["audit"] if item["id"] == "prior-1")["disposition"] = "still_present" + self.capture(snapshot, proof) + self.publish() + reset.reset() + self.assertEqual(self.reviews[10]["state"], "DISMISSED") + + def test_target_ownership_changed_after_freeze_is_retained(self): + accepted = self.capture(self.freeze()) + self.publish() + self.reviews[10]["user"] = {"login": "human"} + with self.assertRaises(_gh.TerminalError): + reset.dismiss(accepted) + self.assertFalse(self.puts()) + + def test_newer_formal_publication_or_deleted_current_result_retains(self): + accepted = self.capture(self.freeze()) + self.publish() + other = self.add_old(12, 3) + other["submitted_at"] = baseline.T2 + with self.assertRaises(_gh.TerminalError): + reset.dismiss(accepted) + del self.reviews[12] + del self.reviews[1000] + with self.assertRaises(_gh.TerminalError): + reset.dismiss(accepted) + self.assertFalse(self.puts()) + + def test_malformed_native_record_duplicate_proof_and_unknown_fields_retain(self): + snapshot = self.freeze() + proof = json.dumps(self.proof(snapshot)) + for text in ( + "```review-reset-proof\n" + proof[:-1] + ',"blocking_issues":0}\n```', + "```review-reset-proof\n" + proof + "\n```\n```review-reset-proof\n" + proof + "\n```", + "```review-reset-proof\n" + proof[:-1] + ',"extra_secret":"not permitted"}\n```', + ): + with self.subTest(text=text), self.assertRaises(_gh.TerminalError): + self.capture(snapshot, terminal_extra={"result": text}) + reset.write_json(self.env["REVIEW_EXECUTION_FILE"], {"result": proof}) + with self.assertRaises(_gh.TerminalError): + reset.capture() + self.assertFalse(self.puts()) + + def test_only_eligible_original_targets_clear_when_another_is_unverifiable(self): + self.add_old(11, 3) + del self.comments[3] + snapshot = self.freeze() + self.assertEqual([item["id"] for item in snapshot["retained"]], [11]) + self.capture(snapshot) + self.publish() + reset.reset() + self.assertEqual(self.reviews[10]["state"], "DISMISSED") + self.assertEqual(self.reviews[11]["state"], "CHANGES_REQUESTED") + + def test_required_workflow_inventory_supports_late_exact_dismissal(self): + self.capture(self.freeze()) + self.publish() + self.workflow_url = f"{_gh.API_ROOT}/repos/example/repo/actions/required_workflows/50" + reset.reset() + self.assertEqual(self.reviews[10]["state"], "DISMISSED") + self.assertIn(("LIST", self.workflow_url + "/runs?branch=topic", None), self.calls) + + def test_workflow_inventory_url_must_be_exact_api_and_repository(self): + snapshot = self.freeze() + prefix = f"{_gh.API_ROOT}/repos/example/repo/actions/" + for url in (None, "", "https://github.com/example/repo/actions/workflows/50", + "https://api.github.com.evil/repos/example/repo/actions/workflows/50", + "https://foreign.example/repos/example/repo/actions/workflows/50", + f"{_gh.API_ROOT}/repos/foreign/repo/actions/workflows/50", + prefix + "workflows/not-numeric", prefix + "required_workflows/50?repo=foreign", + prefix + "required_workflows/50#fragment", prefix + "workflows/50/runs", + prefix + "workflows/../50"): + self.workflow_url = url + first_call = len(self.calls) + with self.subTest(url=url), self.assertRaises(_gh.TerminalError): + reset.check_newer_runs("example/repo", "42", snapshot) + self.assertFalse(any(call[0] == "LIST" for call in self.calls[first_call:])) + + def test_both_workflow_url_shapes_preserve_complete_pagination_accounting(self): + snapshot = self.freeze() + for kind in ("workflows", "required_workflows"): + self.workflow_url = f"{_gh.API_ROOT}/repos/example/repo/actions/{kind}/50" + pages = [{"total_count": 2, "workflow_runs": [{"id": 1, "run_started_at": baseline.T1}]}, + {"total_count": 2, "workflow_runs": [{"id": 2, "run_started_at": baseline.T1}]}] + with self.subTest(kind=kind), mock.patch.object(_gh, "rest_paginate", return_value=pages) as paginate: + reset.check_newer_runs("example/repo", "42", snapshot) + paginate.assert_called_once_with(self.workflow_url + "/runs?branch=topic") + with mock.patch.object(_gh, "rest_paginate", return_value=pages[:1]), self.assertRaises(_gh.TerminalError): + reset.check_newer_runs("example/repo", "42", snapshot) + + def test_new_json_writes_replace_symlink_without_touching_sentinel(self): + sentinel = self.directory / "sentinel.txt" + sentinel.write_text("must remain unchanged") + destination = self.directory / "owned.json" + destination.symlink_to(sentinel) + reset.write_json(destination, {"current": True}) + self.assertEqual(sentinel.read_text(), "must remain unchanged") + self.assertFalse(destination.is_symlink()) + self.assertEqual(self.real_read(destination), {"current": True}) + + def test_failed_atomic_write_preserves_existing_file_and_removes_temporary(self): + destination = self.directory / "owned.json" + destination.write_text("old contents") + before = set(self.directory.iterdir()) + with mock.patch.object(reset.os, "replace", side_effect=OSError("replace failed")), self.assertRaises(OSError): + reset.write_json(destination, {"current": True}) + self.assertEqual(destination.read_text(), "old contents") + self.assertEqual(set(self.directory.iterdir()), before) + + def test_failed_prepare_clears_planted_snapshot_and_preserves_normal_context(self): + context = self.directory / "context.json" + original = context.read_bytes() + reset.SNAPSHOT.symlink_to(context) + host = Path(self.env["REVIEW_RESET_SNAPSHOT"]) + reset.write_json(host, {"stale": True}) + self.prior = None + with self.assertRaises(_gh.TerminalError): + self.freeze() + self.assertEqual(context.read_bytes(), original) + self.assertFalse(reset.SNAPSHOT.exists()) + self.assertFalse(reset.SNAPSHOT.is_symlink()) + self.assertFalse(host.exists()) + # A new workspace-only fake cannot replace the absent host input. + reset.write_json(reset.SNAPSHOT, {"fake": True}) + with self.assertRaises(FileNotFoundError): + reset.capture() + self.publish() + self.assertEqual(self.reviews[1000]["state"], "COMMENTED") + self.assertFalse(self.puts()) + + def test_capture_uses_host_snapshot_not_mutable_workspace_copy(self): + snapshot = self.freeze() + changed = copy.deepcopy(snapshot) + changed["audit"] = [] + reset.write_json(reset.SNAPSHOT, changed) + with self.assertRaises(_gh.TerminalError): + self.capture(changed) + accepted = self.capture(snapshot) + self.assertEqual(accepted["snapshot"], snapshot) + + +class ActionContractTest(unittest.TestCase): + def test_original_model_options_and_reset_failure_split(self): + action = Path(__file__).parents[1].joinpath("action.yml").read_text() + args = next(line.strip() for line in action.splitlines() if line.strip().startswith("claude_args:")) + self.assertEqual(args, 'claude_args: --model claude-opus-5-5 --max-turns 100 --setting-sources user --strict-mcp-config --disallowedTools "Skill,ScheduleWakeup,CronCreate,CronDelete,CronList" --allowedTools "Read,Glob,Grep,Task,mcp__github_inline_comment__create_inline_comment,mcp__github_comment__update_claude_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh api:*)"') + self.assertNotIn("--json-schema", action) + steps = action.split("\n - name: ")[1:] + publisher = next(step for step in steps if step.startswith("Publish review report")) + self.assertIn("if: steps.claude_review.conclusion == 'success'", publisher) + self.assertNotIn("reset-", publisher) + for name in ("Freeze prior", "Capture optional", "Persist accepted", "Dismiss exactly"): + step = next(step for step in steps if step.startswith(name)) + self.assertIn("continue-on-error: true", step) + dismiss = steps[-1] + self.assertIn("steps.reset-upload.outcome == 'success'", dismiss) + self.assertNotIn("reset-upload.conclusion", dismiss) + self.assertNotIn("retention-days:", next(step for step in steps if step.startswith("Persist accepted"))) + capture = next(step for step in steps if step.startswith("Capture optional")) + self.assertIn("steps.reset-prepare.outputs.has_targets == 'true'", capture) + + +if __name__ == "__main__": + unittest.main() diff --git a/README.md b/README.md index 8ce9401..475101c 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,47 @@ Active findings appear once in their severity section, labeled `New` or `Prior — still present`. A compact resolved section records fixed/obsolete prior findings with evidence; it does not repeat the active findings. +### Clearing this reviewer's previous blocks + +Both profiles attempt self-dismissal by default, never approval. Before the +model runs, CI freezes exact reviews positively owned by this workflow/profile, +their original count-accounted blocking findings, and ALL typed prior-finding +audit inputs. Known suggestions may remain nonblocking; scraped deduplication +context and fenced prompt bullets do not become reset obligations. +Top-level Security/Correctness list items are counted once, with nested bullets, +wrapped text, and supporting fenced/quoted examples preserved in each audit item. +Capture uses the authoritative frozen input in runner temporary storage; the +workspace copy is model-readable input only. New reset writes replace destination +entries atomically, and preparation clears stale reset snapshots without following +file symlinks. If there are no eligible targets, proof capture/upload are skipped. +A successful terminal response may supply a compact JSON audit +proof for those IDs with complete coverage, no blockers, and affirmative +current-code evidence. It supplements, rather than replaces, the normal review; +CI supplies run/attempt, head/base, and session provenance, not the model proof. +CI captures it from the runner execution file, sanitizes/redacts all persisted +evidence, and uploads one accepted reset artifact using repository retention +defaults. Editable summary prose is not reset authority. + +Only after normal publication completes with a neutral COMMENT does CI recheck +the PR head/base, frozen targets, original evidence, and competing/newer runs, +then dismiss those exact blocking reviews. A failed preparation, absent/malformed +proof, incomplete audit, failed upload, stale source, or ambiguous GitHub result +retains blocks without suppressing ordinary publication or changing its state +lifecycle. An incomplete review still keeps its provisional summary as before. +Reset warnings and short redacted per-target retain reasons are visible in the +workflow log; ambiguous writes are reconciled by reading the exact review, +never blindly retried. + +Human, foreign-profile, and unmarked legacy reviews are never targets. +Missing, edited-by-human, deleted-history, or count-unaccounted original finding +evidence requires manual handling; this is not a migration of every historical +block. +An incomplete edit/competing-run inventory also retains the affected block +(including more than 100 original edits or GitHub's capped run listing). +No new permissions or caller opt-in are needed. The model retains its existing +tools and write-capable GitHub token: this host ordering is not adversarial +credential isolation and does not replace required human approval. + ### Reusable Workflow Calls Callers of `pr-review.yaml` must pass `ANTHROPIC_API_KEY` as a named secret; do not