diff --git a/.github/review-policy/README.md b/.github/review-policy/README.md new file mode 100644 index 000000000..fdf4c13e1 --- /dev/null +++ b/.github/review-policy/README.md @@ -0,0 +1,250 @@ +# Native Codex approval adapter + +The [adapter workflow](../workflows/codex-approval-adapter.yml) runs on +GitHub-hosted Ubuntu. It observes the existing Codex GitHub integration and can +submit an `APPROVED` review through a dedicated GitHub App. It starts in read-only +audit mode. Merging this implementation does not enable approval writes. + +## Contributor flow + +Bot approval is available only when the person who opened the PR currently has +Write, Maintain, or Admin access to `zakura-core/zakura`. Authors with Read, +Triage, or no repository access use the normal human review process, even if someone with +more access requests Codex review on their PR. Bot-authored PRs also use normal +review. This check uses the author's live repository permissions, including team +and organization grants; membership labels and the event sender do not qualify. + +Use the normal automatic Codex review, or comment exactly `@codex review`. +After a clean review of the current commit, eligible PRs receive an approval from +the adapter App. The contributor can merge when GitHub's other requirements pass. +The adapter does not merge PRs or start another model review. + +After fixes, push the commits and request `@codex review` again, or let the existing +automatic review setting trigger it. Resolve addressed Codex threads before the +new clean review. Resolving a finding by itself does not qualify the PR. +Personal automatic-review settings and native Codex comments stay as they are. +The adapter does not need an OpenAI API key or consume a second review. + +The App supplies an approval through the repository's normal approval process. +No dedicated reviewer team or additional path-based reviewer rule is required. +The adapter enforces its own file eligibility policy. If it cannot approve a PR, +the contributor requests review from the usual reviewers under the existing +repository rules. Existing reviewer or code-owner requirements still apply. + +If a PR does not qualify, get a normal human approval. The adapter never submits +`REQUEST_CHANGES` or installs a mandatory Codex status check. Its Actions summary +explains why it withheld approval. Scoped prompts, security-only reviews, drafts, +and native output formats it cannot verify take the human-review path. + +## Eligible files + +[policy.json](policy.json) is the authoritative scope. Existing files under +`deploy/`, `.github/workflows/`, and `.github/scripts/` qualify, except for: + +- Release creation, preparation, publishing, readiness, drafting, and release-state workflows. +- Release-state fetch/import scripts, checkpoint validation, and `deploy/release-state/`. +- The adapter workflow itself. + +Everything outside those roots, including `.github/review-policy/`, +`.github/actions/`, root `scripts/`, and application code, requires human review. +One excluded file makes the whole PR ineligible. Both names in a rename are +checked; additions and renames require human classification before they can be +treated as existing eligible files in later PRs. + +There is one addition exception: an otherwise eligible PR may add its own +`docs/changelog/unreleased/.md` fragment. For example, PR #123 may +modify `deploy/zakura-watchdog/src/main.rs` and add +`docs/changelog/unreleased/123.md`. Later edits to that new fragment within the +same PR still qualify because it remains an addition relative to the base. +The adapter requires this fragment when an eligible PR changes a Rust source +file or `Cargo.toml`, including internal changes with a no-changelog fragment. +The fragment must be a regular text file without `release-readiness` directives; +release-policy waivers still need human review. The root `CHANGELOG.md`, other +PRs' fragments, existing fragment edits/deletions, and changelog-only PRs remain +on the human-review path. Changelog CI continues to validate fragment syntax. + +Release exclusions cover the existing release gates and publishing helpers. +Ordinary fleet deployment and the advisory VCT canary remain in the deployment +scope; the release workflow treats these as advisory operations. When adding or +moving a release helper into an eligible root, add it to `human_only` in the same +human-reviewed PR. + +## Native review evidence + +The adapter requires all of the following from live GitHub API reads: + +- The immutable Codex App and Bot IDs on the summary comment, plus Bot-authored + GraphQL edit history showing the current `Running` → `Completed` episode. +- A reviewed commit abbreviation that GitHub resolves to the full current PR + head, with the same commit and trigger throughout the episode. +- A new PR-level thumbs-up from that Bot after completion, with no running-review + reaction. `Completed` alone also describes reviews that found problems. +- No Codex findings submitted during that episode and no unresolved Codex threads, + including outdated threads. Older resolved findings allow a new clean review. +- For manual reviews, a visible, recent, unedited `@codex review` request from + an account with current Write, Maintain, or Admin access. Commands from other + accounts are ignored, including for invalidation of an existing approval. + A newer visible request from an authorized account invalidates the previous receipt. + +The parser supports the native Code Review summary table. A changed format, +missing history, additional unsupported review rows, ambiguous commit resolution, +or incomplete pagination withholds approval. The public integration does not +provide a versioned machine verdict or a run ID tying a manual request to its +result, so this adapter is deliberately conservative about observable evidence. +It does not treat comment text as a cryptographic attestation of review coverage. +In particular, deleting a scoped manual request can make its result appear to +belong to an older ordinary request. This limitation is accepted for PRs opened +by trusted authors; the author permission gate restricts eligible PRs but does +not prove which comment triggered a native review. Normal `@codex review` remains +supported without a separate request journal. + +Comment and PR metadata events trigger evaluation. Because reactions have no +webhook event, a completion event gets one short retry for the thumbs-up. Hourly +reconciliation catches missed events, removed reactions, changed policy, and +interrupted runners. Manual dispatch on `main` rechecks evidence without asking +Codex for another review. +PR events are not filtered by base branch, and periodic reconciliation includes +all open PRs. If an approved PR moves away from `main`, the adapter withdraws its +approval; PRs on other bases cannot receive a new approval. + +A read-only permission preflight skips the App job when no target PR has an +authorized author. The exception is cleanup: a PR with an existing App approval +still reaches the writer so it can withdraw that approval if access was revoked +or cannot be verified. Pending Slack withdrawal notifications also reach the +writer after access is revoked. The writer independently checks the author's access on +every evaluation before and after approval, including scheduled runs. + +## Slack notifications + +The first App approval creates a message in **#gh-alerts** (`C0BCQ7PP32A`). +Withdrawals and subsequent approvals reply in that same thread, with links to +the PR, the GitHub review, and the reviewed commit. For example, approval of +commit A starts the thread; withdrawal after a push and approval of commit B +add two replies. Unchanged reconciliations send nothing. A review withdrawn +before its approval could be announced does not create an approval alert. + +The workflow reuses the repository's `SLACK_BOT_TOKEN`. Its Slack bot needs +`chat:write` and membership in **#gh-alerts**. No Slack history permissions, +GitHub App webhook, new channel variable, or separate service is needed. +It uses `chat.postMessage` because that API returns the timestamp needed for +thread replies; an incoming webhook does not return it. + +Thread timestamps and delivery receipts are stored in hidden +`zakura-codex-slack:v1` markers in the App's GitHub reviews. Only the dedicated +App's marked reviews are updated; the approval receipt remains intact. +Slack failure appears as a separate `notification_error` in Actions and never +changes the GitHub approval decision. Definite Slack rejections can retry on +the next reconciliation after configuration or rate-limit recovery. + +Before sending, the adapter saves a `pending` checkpoint. If delivery is +uncertain, such as a timeout after Slack accepted the message, it stops sending +for that PR instead of risking duplicate threads. An operator must inspect +**#gh-alerts** and repair that review's Slack marker using the approval App: +if delivered, set `thread_ts` to the root message timestamp, append the pending +status to `sent`, and clear `pending` to `null`; if confirmed undelivered, +clear only `pending`. Preserve the approval marker and review state. +Do not clear an uncertain checkpoint without checking Slack first. +Then dispatch reconciliation for that PR. Hourly retries cover open PRs; +closed PRs require an explicit dispatch if notification delivery is outstanding. + +## Administrator setup + +After this PR is merged, keep `CODEX_APPROVAL_ENABLED` unset or `false` while +configuring the credentials. No GitHub environment or review-rule changes are +needed. + +1. Create a dedicated GitHub App with repository **Pull requests: read and write** + permission and webhooks disabled. Install it only on `zakura-core/zakura` and + generate a private key. The App needs no additional permissions beyond the + mandatory metadata access; do not grant administration access or ruleset bypass. +2. Under repository **Settings → Secrets and variables → Actions**, add the + repository secret `CODEX_APPROVAL_APP_PRIVATE_KEY` with the full PEM key + contents. Keep the key in a dedicated Infisical scope and sync it here. + Reuse the existing `SLACK_BOT_TOKEN`; confirm its bot has `chat:write` and + invite it to **#gh-alerts** if it is not already a member. +3. On the **Variables** tab, add these repository variables: + + | Variable | Value | + | --- | --- | + | `CODEX_APPROVAL_APP_CLIENT_ID` | The dedicated App's client ID | + | `CODEX_APPROVAL_APP_ID` | Its numeric App ID | + | `CODEX_APPROVAL_BOT_ID` | Numeric ID of its `[bot]` account | + + To get the bot ID, replace `APP_SLUG` with the App's actual slug: + + ```sh + gh api 'users/APP_SLUG[bot]' --jq '.id' + ``` + +4. Set repository variable `CODEX_APPROVAL_ENABLED=true` and run the live + validation below. Confirm that a clean eligible PR receives an App approval + and an excluded PR does not. Set the variable back to `false` if validation + fails. Personal Codex settings and the normal review process stay unchanged. + +The key is a repository Actions secret, so no environment approval or branch +restriction gates access to it. The adapter still checks out trusted `main`. +The existing `main` ruleset already supplies the one required approval, +`test success` check, and empty bypass list; leave those settings unchanged. + +The writer independently rereads the ruleset and checks its approval requirement +and required test check before every approval. Missing or +weakened configuration prevents new approvals. A trusted `main` checkout is used +in both jobs; PR code, artifacts, and commands never execute with the App token. +Keep GitHub Actions' general permission to approve PRs disabled; this workflow +uses its own narrowly scoped App token. + +## Validation and operations + +Run the offline tests with: + +```sh +python3 .github/review-policy/test_adapter.py +``` + +In a repository with matching rules and App permissions, validate a clean +eligible PR, a PR with findings, fixes followed by another review, and a push +after approval. Also test a mixed PR, a release helper edit, and a source file +renamed into an eligible directory. The App's review must count for the eligible +PR while excluded paths receive no adapter approval and need normal review. +Test an eligible change accompanied by its own new changelog fragment, plus +rejections for another PR's fragment and a fragment containing a release waiver. +Verify that Write, Maintain, and Admin authors qualify, while Read, Triage, and +outside authors do not, regardless of who requested review. Also verify that +revoking an author's access withdraws an existing App approval on reconciliation. +Confirm that approval, withdrawal, and reapproval appear in one **#gh-alerts** +thread, and that repeating reconciliation adds no messages. Verify that a Slack +delivery failure reports an Actions error without undoing a valid App approval. + +Verify that the adapter withholds a new approval when Codex reviewed an older +commit, and withdraws an existing approval after detecting a push. The adapter +submits the full reviewed `commit_id` and rereads state before and after approval. +The review API has no atomic expected-current-head precondition, so these checks +and later withdrawal do not create a synchronous merge restriction. + +The existing repository policy intentionally permits approval to remain valid +after changes. For example, after the App approves commit A, a contributor may +push commit B and merge before the adapter detects it and withdraws its approval. +Enabling GitHub's stale-review settings is not a prerequisite for this adapter; +their behavior remains the maintainer's existing choice. + +Only this App's marked approvals are withdrawn. An explicit dismissal of an +episode is respected until a fresh clean Codex review supplies a new receipt. +The adapter can restore its own automatic withdrawal after all evidence qualifies +again. It verifies the dismissal's actor and message in GitHub's timeline; +missing or ambiguous dismissal history keeps the approval withheld. +An unavailable API withholds approval and attempts to withdraw an existing one; +failed withdrawal surfaces as a failed Actions run. Updates caused by review +comments and reaction changes are asynchronous, with hourly reconciliation as a +backstop. There is no synchronous Codex merge check. + +To stop new approval writes, set `CODEX_APPROVAL_ENABLED=false`. Dismiss existing +adapter approvals if they must stop counting immediately; disabling the workflow +or revoking the key does not erase reviews already submitted. Retain the global +approval requirement so ordinary human review keeps working. + +## References + +- [Native Codex GitHub reviews](https://learn.chatgpt.com/docs/third-party/github) +- [GitHub PR approval and freshness rules](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/available-rules-for-rulesets) +- [GitHub pull request review API](https://docs.github.com/en/rest/pulls/reviews#create-a-review-for-a-pull-request) +- [Slack message and thread API](https://docs.slack.dev/reference/methods/chat.postMessage/) diff --git a/.github/review-policy/adapter.py b/.github/review-policy/adapter.py new file mode 100644 index 000000000..cd6376008 --- /dev/null +++ b/.github/review-policy/adapter.py @@ -0,0 +1,647 @@ +#!/usr/bin/env python3 +"""Translate an authenticated, current native Codex review into a PR approval. + +The default is read-only. This program reads GitHub metadata, never PR code. +Approval policy and this executable must come from the trusted default branch. +""" + +from __future__ import annotations + +import argparse +import base64 +import binascii +from dataclasses import dataclass +from datetime import datetime, timedelta, timezone +import hashlib +import http.client +import json +import os +from pathlib import Path +import re +import sys +import time +import urllib.error +import urllib.parse +import urllib.request + +import slack_notify + + +POLICY_PATH = Path(__file__).with_name("policy.json") +SUMMARY_MARKER = "" +RECEIPT_MARKER = "" + episode_keys = ("head", "summary", "completed", "reaction") + dismissed = False + for review in owned: + if review["state"] != "DISMISSED" or review["id"] in automatic: + continue + try: + prior = json.loads(review["body"].splitlines()[0][len(RECEIPT_MARKER):-4]) + dismissed |= all(prior[k] == receipt[k] for k in episode_keys) + except (ValueError, KeyError, TypeError): + # Do not manufacture a new approval when our prior receipt can + # no longer be interpreted. A human can review the PR instead. + dismissed = True + if dismissed: + for review in existing: + self.dismiss(review["id"]) + return {"approved": False, "reason": "This review was dismissed; request a fresh Codex review"} + keep = [r for r in existing if r["commit_id"] == receipt["head"] + and r["body"].splitlines()[0] == marker] + for review in existing: + if not keep or review["id"] != keep[0]["id"]: + self.dismiss(review["id"]) + if keep: + return {"approved": True, "reason": "Current adapter approval already exists"} + + # No PR-provided files, commands, artifacts, or strings are executed. + # A second snapshot catches pushes/new review requests during API reads. + try: + require(self.evaluate() == receipt, "Review evidence changed before approval") + self.check_trusted_revision() + except (Ineligible, APIError, KeyError, TypeError) as exc: + return {"approved": False, "reason": str(exc)} + body = (marker + "\n\nNative Codex completed a clean review of `" + receipt["head"] + + "`. Every changed path is eligible for Codex approval.\n\n" + + f"[Native review summary](https://github.com/{self.repo}/pull/{self.number}" + + f"#issuecomment-{receipt['summary']}). Existing merge requirements still apply.") + created = None + try: + created = self.writer.request(self.pull_path + "/reviews", "POST", { + "event": "APPROVE", "commit_id": receipt["head"], "body": body, + }) + require(created.get("user", {}).get("id") == self.bot_id + and created.get("user", {}).get("type") == "Bot", + "Approval token does not belong to the configured App") + require(self.evaluate() == receipt, "Review evidence changed while approving") + self.check_trusted_revision() + except (Ineligible, APIError, KeyError, TypeError): + if created is not None: + self.dismiss(created["id"]) + else: + # A timed-out POST may have succeeded. Never retry it blindly. + for review in self.owned_reviews(): + if review["state"] == "APPROVED": + self.dismiss(review["id"]) + raise + return {"approved": True, "reason": "Approved the current native Codex review", + "review_id": created["id"]} + + +def target_numbers(api, policy, event): + if event.get("pull_request"): + return [int(event["pull_request"]["number"])] + if event.get("issue", {}).get("pull_request"): + return [int(event["issue"]["number"])] + if event.get("inputs", {}).get("pr"): + return [int(event["inputs"]["pr"])] + # Include other bases so a missed retarget event cannot leave our approval + # active outside main. evaluate() still forbids approvals on other bases. + pulls = api.pages(f"/repos/{policy.data['repository']}/pulls?state=open") + require(len(pulls) <= 100, "Too many PRs for one bounded reconciliation run") + return [p["number"] for p in pulls] + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--pr", type=int) + mode = parser.add_mutually_exclusive_group() + mode.add_argument("--apply", action="store_true") + mode.add_argument("--check-authors", action="store_true", + help="Read-only permission preflight for the approval job") + parser.add_argument("--event", type=Path) + parser.add_argument("--wait-seconds", type=int, default=0, choices=range(0, 61), metavar="0..60") + args = parser.parse_args() + policy = Policy.load() + require(os.environ.get("GITHUB_REPOSITORY", policy.data["repository"]) == policy.data["repository"], + "This policy is only for the canonical repository") + api = GitHub(os.environ["GH_TOKEN"]) + writer = None + if args.apply: + require(os.environ.get("CODEX_APPROVAL_ENABLED") == "true", "Approval writes are disabled") + # The pinned create-github-app-token action supplies the slug of the App + # for which it minted the token; check the configured immutable IDs too. + slug = os.environ["CODEX_APPROVAL_APP_SLUG"] + require(re.fullmatch(r"[a-z0-9-]+", slug) is not None, "Invalid approval App slug") + app = api.request(f"/apps/{slug}") + bot = api.request(f"/users/{slug}%5Bbot%5D") + require(app["id"] == int(os.environ["CODEX_APPROVAL_APP_ID"]) + and app["client_id"] == os.environ["CODEX_APPROVAL_APP_CLIENT_ID"] + and bot["id"] == int(os.environ["CODEX_APPROVAL_BOT_ID"]) + and bot["type"] == "Bot", "Approval App configuration does not match its identity") + writer = GitHub(os.environ["GH_APPROVAL_TOKEN"]) + event = json.loads(args.event.read_text()) if args.event else {} + numbers = [args.pr] if args.pr is not None else target_numbers(api, policy, event) + require(all(n > 0 for n in numbers), "PR number must be positive") + results = [] + for number in numbers: + adapter = Adapter(api, policy, number, + writer=writer, app_id=int(os.environ.get("CODEX_APPROVAL_APP_ID") or 0), + bot_id=int(os.environ.get("CODEX_APPROVAL_BOT_ID") or 0), + trusted_sha=os.environ.get("TRUSTED_SHA")) + for attempt in range(2): + try: + if args.check_authors: + result = adapter.author_gate() + elif writer: + result = adapter.reconcile() + else: + receipt = adapter.evaluate(enforce_rules=False) + result = {"approved": False, "eligible": True, "receipt": receipt, + "reason": "Read-only: evidence and paths qualify; activation rules not checked"} + except Ineligible as exc: + result = {"approved": False, "eligible": False, "reason": str(exc)} + except (APIError, KeyError, TypeError) as exc: + result = {"approved": False, "error": type(exc).__name__, "reason": "Could not verify GitHub state"} + # Reactions have no webhook and usually follow the summary by a few + # seconds. Wait once, only for a single-PR event, then use the schedule. + if (attempt == 0 and len(numbers) == 1 and args.wait_seconds + and "Waiting for a fresh Codex thumbs-up" in result.get("reason", "")): + time.sleep(args.wait_seconds) + else: + break + if writer: + # Slack delivery must never withdraw an otherwise valid approval. + # Also report withdrawals made while reconciliation handled a failure. + try: + result["slack_messages"] = slack_notify.notify( + adapter, os.environ.get("SLACK_BOT_TOKEN", ""), approved=result.get("approved") is True) + except (slack_notify.NotificationError, Ineligible, APIError, KeyError, TypeError) as exc: + result["notification_error"] = (str(exc) if isinstance(exc, slack_notify.NotificationError) + else "Could not verify or save Slack notification state") + results.append({"pr": number, **result}) + print(json.dumps(results[-1], sort_keys=True), flush=True) + if args.check_authors and os.environ.get("GITHUB_OUTPUT"): + with open(os.environ["GITHUB_OUTPUT"], "a") as output: + output.write("run_reconcile=" + str(any(r.get("reconcile") for r in results)).lower() + "\n") + if os.environ.get("GITHUB_STEP_SUMMARY"): + with open(os.environ["GITHUB_STEP_SUMMARY"], "a") as output: + output.write("### Codex approval adapter\n\n```json\n" + + json.dumps(results, indent=2, sort_keys=True) + "\n```\n") + return int(any("error" in r or "notification_error" in r for r in results)) + + +if __name__ == "__main__": + try: + sys.exit(main()) + except (Ineligible, APIError, KeyError, ValueError) as error: + print(f"Adapter stopped: {type(error).__name__}: {error}", file=sys.stderr) + sys.exit(1) diff --git a/.github/review-policy/policy.json b/.github/review-policy/policy.json new file mode 100644 index 000000000..0a6ba8d63 --- /dev/null +++ b/.github/review-policy/policy.json @@ -0,0 +1,27 @@ +{ + "repository": "zakura-core/zakura", + "base_branch": "main", + "slack_channel": "C0BCQ7PP32A", + "changelog_fragment_root": "docs/changelog/unreleased/", + "eligible_roots": [ + "deploy/", + ".github/workflows/", + ".github/scripts/" + ], + "human_only": [ + ".github/workflows/codex-approval-adapter.yml", + ".github/workflows/create-release.yml", + ".github/workflows/prepare-release-pr.yml", + ".github/workflows/publish-crates.yml", + ".github/workflows/release-binaries.yml", + ".github/workflows/release-drafter.yml", + ".github/workflows/release-pr-readiness.yml", + ".github/workflows/update-release-state.yml", + ".github/scripts/fetch-release-state.py", + ".github/scripts/import-release-state.py", + ".github/scripts/validate-checkpoints.sh", + "deploy/release-state/" + ], + "codex_user_id": 199175422, + "codex_app_id": 1144995 +} diff --git a/.github/review-policy/slack_notify.py b/.github/review-policy/slack_notify.py new file mode 100644 index 000000000..86e7069ac --- /dev/null +++ b/.github/review-policy/slack_notify.py @@ -0,0 +1,158 @@ +"""Report approval transitions in one Slack thread per PR. + +Delivery receipts live in the App's GitHub review bodies. A checkpoint before +each POST prevents blind retries when Slack may have accepted a lost response. +""" + +import http.client +import json +import re +import urllib.error +import urllib.request + + +MARKER = ""): + raise ValueError + state = json.loads(lines[0][len(MARKER):-4]) + if (not isinstance(state, dict) + or set(state) != {"channel", "thread_ts", "sent", "pending"} + or not isinstance(state["channel"], str) + or re.fullmatch(r"C[A-Z0-9]+", state["channel"]) is None + or (state["thread_ts"] is not None + and (not isinstance(state["thread_ts"], str) + or TIMESTAMP.fullmatch(state["thread_ts"]) is None)) + or state["sent"] not in ([], ["APPROVED"], ["APPROVED", "DISMISSED"]) + or state["pending"] not in (None, "APPROVED", "DISMISSED") + or state["pending"] in state["sent"] + or (state["sent"] and state["thread_ts"] is None) + or (state["pending"] == "DISMISSED" and state["sent"] != ["APPROVED"])): + raise ValueError + return state + except (TypeError, ValueError, KeyError): + raise NotificationError("Malformed Slack checkpoint in an App review") from None + + +def needs_notification(review): + """Keep withdrawal delivery reachable after the PR author loses access.""" + try: + state = checkpoint(review) + return bool(state and (state["pending"] or ( + review["state"] == "DISMISSED" and state["sent"] == ["APPROVED"]))) + except NotificationError: + return True + + +class Slack: + def __init__(self, token): + self.token = token + + def post(self, channel, text, thread_ts): + payload = {"channel": channel, "text": text, + "unfurl_links": False, "unfurl_media": False} + if thread_ts: + payload["thread_ts"] = thread_ts + request = urllib.request.Request( + "https://slack.com/api/chat.postMessage", method="POST", + data=json.dumps(payload).encode(), + headers={"Authorization": f"Bearer {self.token}", + "Content-Type": "application/json; charset=utf-8"}, + ) + try: + with urllib.request.urlopen(request, timeout=30) as response: + raw = response.read(1024 * 1024 + 1) + if len(raw) > 1024 * 1024: + raise NotificationError("Slack response exceeded the size limit; delivery is uncertain") + result = json.loads(raw) + except urllib.error.HTTPError as exc: + if exc.code == 429: + raise Rejected("Slack rate limited the notification; a later run can retry") from None + raise NotificationError(f"Slack HTTP {exc.code}; delivery is uncertain") from None + except (OSError, http.client.HTTPException, ValueError): + raise NotificationError("Slack response unavailable; delivery is uncertain") from None + if isinstance(result, dict) and result.get("ok") is False: + # Never log arbitrary response bodies, which may contain credentials. + code = result.get("error", "unknown_error") + safe_code = code if isinstance(code, str) and re.fullmatch(r"[a-z_]+", code) else "unknown_error" + raise Rejected(f"Slack rejected the notification ({safe_code})") + if (not isinstance(result, dict) or result.get("ok") is not True + or result.get("channel") != channel + or not isinstance(result.get("ts"), str) + or TIMESTAMP.fullmatch(result["ts"]) is None): + raise NotificationError("Unexpected Slack response; delivery is uncertain") + return result["ts"] + + +def save(worker, review, state): + body = "\n".join(line for line in review["body"].splitlines() + if not line.startswith(MARKER)).rstrip() + body += "\n\n" + MARKER + json.dumps(state, sort_keys=True, separators=(",", ":")) + " -->" + worker.writer.request(f"{worker.pull_path}/reviews/{int(review['id'])}", "PUT", {"body": body}) + review["body"] = body + + +def notify(worker, token, *, approved): + """Reconcile Slack with verified, owned reviews without changing approval state.""" + channel = worker.policy.data["slack_channel"] + reviews = sorted(worker.owned_reviews(), key=lambda review: review["id"]) + states = {review["id"]: checkpoint(review) for review in reviews} + roots = {state["thread_ts"] for state in states.values() if state and state["thread_ts"]} + if len(roots) > 1 or any(state and state["channel"] != channel for state in states.values()): + raise NotificationError("Conflicting Slack thread checkpoints; inspect the App reviews") + if any(state and state["pending"] for state in states.values()): + raise NotificationError("Slack delivery is uncertain; inspect and repair the pending App review checkpoint") + root = next(iter(roots), None) + sent = 0 + slack = Slack(token) + for review in reviews: + status = review["state"] + state = states[review["id"]] + if (status not in STATES or (status == "APPROVED" and not approved) + or (state and status in state["sent"]) + or (status == "DISMISSED" and not (state and "APPROVED" in state["sent"]))): + continue + if not token: + raise NotificationError("SLACK_BOT_TOKEN is required to notify #gh-alerts") + if re.fullmatch(r"[0-9a-f]{40}", review["commit_id"]) is None: + raise NotificationError("Cannot notify an approval with an invalid commit ID") + url = f"https://github.com/{worker.repo}/pull/{worker.number}" + heading = "✅ Codex approval" if status == "APPROVED" else "â†Šī¸ Codex approval withdrawn" + action = "Approved" if status == "APPROVED" else "Withdrew approval of" + text = (f"{heading}: <{url}|Zakura #{worker.number}>\n" + f"{action} commit . " + f"<{url}#pullrequestreview-{review['id']}|GitHub review>.") + if status == "APPROVED": + text += " Other merge requirements still apply." + state = state or {"channel": channel, "thread_ts": root, "sent": [], "pending": None} + state["pending"] = status + save(worker, review, state) + try: + timestamp = slack.post(channel, text, root) + except Rejected: + state["pending"] = None + save(worker, review, state) + raise + root = root or timestamp + state.update(thread_ts=root, pending=None, sent=[*state["sent"], status]) + save(worker, review, state) + sent += 1 + return sent diff --git a/.github/review-policy/test_adapter.py b/.github/review-policy/test_adapter.py new file mode 100644 index 000000000..dd6d6b887 --- /dev/null +++ b/.github/review-policy/test_adapter.py @@ -0,0 +1,1161 @@ +#!/usr/bin/env python3 +"""Regression tests for the approval boundary; no credentials or network used.""" + +from copy import deepcopy +import base64 +import http.client +import json +import os +from pathlib import Path +import tempfile +import unittest +from unittest.mock import Mock, patch + +import adapter +import slack_notify + + +HEAD = "a" * 40 +BASE = "b" * 40 +BOT_ID = 12345 +APP_ID = 6789 +POLICY = adapter.Policy.load() +NATIVE = {"id": POLICY.data["codex_user_id"], "type": "Bot"} +ACTOR = {"databaseId": POLICY.data["codex_user_id"], "login": "chatgpt-codex-connector"} +START = "2026-09-01T10:00:00.123456Z" +FINISH = "2026-09-01T10:02:00.123456Z" + + +def summary_body(completed=True, sha=HEAD[:7], trigger="Draft marked ready"): + # This is the native summary table observed on both auto and manual reviews. + status = "✅ **Completed**" if completed else "🔄 **Running** since" + timestamp = FINISH if completed else START + return (adapter.SUMMARY_MARKER + "\n\n## Codex Review Summary\n\n" + "| Review | Status | Commit | Review trigger |\n" + "| --- | --- | --- | --- |\n" + f'| 📝 **Code Review** | {status} ' + f"{timestamp} | `{sha}` | {trigger} |\n") + + +def evidence(trigger="Draft marked ready"): + body = summary_body(trigger=trigger) + return { + "policy": POLICY, + "pull": {"head": {"sha": HEAD}}, + "comments": [{"id": 100, "node_id": "IC_test", "body": body, + "user": deepcopy(NATIVE), + "performed_via_github_app": {"id": POLICY.data["codex_app_id"]}}], + "summary": {"databaseId": 100, "body": body, + "author": deepcopy(ACTOR), "editor": deepcopy(ACTOR), + "lastEditedAt": "2026-09-01T10:02:00Z", + "userContentEdits": {"pageInfo": {"hasNextPage": False}, "nodes": [ + {"diff": body, "editedAt": "2026-09-01T10:02:00Z", "editor": deepcopy(ACTOR)}, + {"diff": summary_body(False, trigger=trigger), + "editedAt": "2026-09-01T10:00:02Z", "editor": deepcopy(ACTOR)}, + ]}}, + # GitHub's reaction endpoint reports this Bot's type as User; its stable + # numeric ID matches the authenticated App summary and GraphQL Bot. + "reactions": [{"id": 200, "content": "+1", "user": {**NATIVE, "type": "User"}, + "created_at": "2026-09-01T10:02:03Z"}], + "reviews": [], "threads": [], "resolved_sha": HEAD, "authorized_requesters": {900}, + } + + +def command(body="@codex review", created="2026-09-01T09:59:58Z"): + return {"id": 99, "body": body, "created_at": created, "updated_at": created, + "user": {"id": 900, "type": "User", "login": "contributor"}} + + +def native_review(when="2026-09-01T10:01:30Z", commit=HEAD): + # GitHub REST reviews have no performed_via_github_app field. + return {"id": 300, "state": "COMMENTED", "user": deepcopy(NATIVE), + "commit_id": commit, "submitted_at": when} + + +def thread(resolved=False, native=True): + return {"isResolved": resolved, "isOutdated": True, + "comments": {"nodes": [{"author": deepcopy(ACTOR) if native else {"login": "human"}}], + "pageInfo": {"hasNextPage": False}}} + + +class EvidenceTests(unittest.TestCase): + def setUp(self): + self.data = evidence() + + def reject(self, message): + with self.assertRaisesRegex(adapter.Ineligible, message): + adapter.check_evidence(**self.data) + + def replace_current_body(self, body): + self.data["comments"][0]["body"] = body + self.data["summary"]["body"] = body + self.data["summary"]["userContentEdits"]["nodes"][0]["diff"] = body + + def test_native_automatic_review_yields_commit_and_episode_receipt(self): + receipt = adapter.check_evidence(**self.data) + self.assertEqual(receipt["head"], HEAD) + self.assertEqual(receipt["reaction"], 200) + self.assertEqual(receipt["policy"], POLICY.digest) + + def test_native_new_commits_review(self): + self.assertEqual(adapter.check_evidence(**evidence("New commits"))["head"], HEAD) + + def test_author_can_request_another_normal_review(self): + self.data = evidence("Manual request") + self.data["comments"].extend([command(created="2026-09-01T09:30:00Z"), command()]) + self.data["reviews"] = [native_review("2026-09-01T09:31:30Z", "c" * 40)] + self.data["threads"] = [thread(resolved=True)] + self.assertEqual(adapter.check_evidence(**self.data)["head"], HEAD) + + def test_copying_bot_text_does_not_make_a_native_review(self): + self.data["comments"][0]["user"] = {"id": 900, "type": "User", "login": ACTOR["login"]} + self.reject("exactly one") + + def test_wrong_app_cannot_supply_summary(self): + self.data["comments"][0]["performed_via_github_app"]["id"] = 1 + self.reject("exactly one") + + def test_human_editor_cannot_replace_summary(self): + self.data["summary"]["editor"] = {"login": "human"} + self.reject("last editor") + + def test_historical_human_edit_is_not_hidden_by_new_bot_edit(self): + self.data["summary"]["userContentEdits"]["nodes"][1]["editor"] = {"login": "human"} + self.reject("non-Codex editor") + + def test_reaction_update_timestamp_is_not_a_content_edit(self): + self.data["comments"][0]["updated_at"] = "2026-09-07T20:00:00Z" + self.assertEqual(adapter.check_evidence(**self.data)["head"], HEAD) + + def test_missing_or_truncated_history(self): + for change in ("missing", "truncated"): + with self.subTest(change=change): + self.data = evidence() + history = self.data["summary"]["userContentEdits"] + if change == "missing": + history["nodes"] = history["nodes"][:1] + else: + history["pageInfo"]["hasNextPage"] = True + self.reject("history") + + def test_completed_without_immediately_preceding_running(self): + self.data["summary"]["userContentEdits"]["nodes"][1]["diff"] = summary_body() + self.reject("preceding Running") + + def test_changed_summary_between_rest_and_graphql_reads(self): + self.data["summary"]["body"] = summary_body(False) + self.reject("changed while") + + def test_running_failed_and_unknown_formats_withhold_approval(self): + for body in (summary_body(False), summary_body().replace("Completed", "Failed"), + summary_body() + "| 🔒 **Security Review** | Running | `aaaaaaa` | Manual request |\n"): + with self.subTest(body=body): + self.data = evidence() + self.replace_current_body(body) + self.reject("Running|unsupported|Unknown") + + def test_mixed_commit_episode_is_rejected(self): + self.data["summary"]["userContentEdits"]["nodes"][1]["diff"] = summary_body(False, sha="ccccccc") + self.reject("changed commit") + + def test_old_head_or_abbreviated_sha_collision_is_rejected(self): + for resolved in ("c" * 40, HEAD[:7] + "c" * 33, HEAD[:7]): + with self.subTest(resolved=resolved): + self.data["resolved_sha"] = resolved + self.reject("ambiguous commit") + + def test_new_same_head_review_request_invalidates_clean_receipt(self): + self.data["comments"].append(command(created="2026-09-01T10:03:00Z")) + self.reject("newer or edited") + + def test_edited_command_invalidates_prior_receipt(self): + request = command() + request["updated_at"] = "2026-09-01T10:03:00Z" + self.data["comments"].append(request) + self.reject("newer or edited") + + def test_scoped_manual_review_cannot_approve_whole_pr(self): + for body in ("@codex review only the README", "@codex security review"): + with self.subTest(body=body): + self.data = evidence("Manual request") + self.data["comments"].append(command(body)) + self.reject("Scoped review") + + def test_missing_manual_request(self): + self.data = evidence("Manual request") + self.reject("request is missing") + + def test_unknown_auto_trigger(self): + self.data = evidence("Future scoped review") + self.reject("Unrecognized automatic") + + def test_old_missing_wrong_author_or_late_thumbs_up_is_not_clean(self): + for change in ("old", "missing", "human", "late"): + with self.subTest(change=change): + self.data = evidence() + reaction = self.data["reactions"][0] + if change == "old": + reaction["created_at"] = "2026-09-01T09:59:59Z" + elif change == "missing": + self.data["reactions"] = [] + elif change == "human": + reaction["user"]["id"] = 900 + else: + reaction["created_at"] = "2026-09-01T10:10:00Z" + self.reject("fresh Codex thumbs-up") + + def test_running_reaction_with_lingering_thumbs_up(self): + self.data["reactions"].append({"content": "eyes", "user": NATIVE}) + self.reject("running-review reaction") + + def test_resolving_current_findings_does_not_turn_review_clean(self): + self.data["reviews"] = [native_review()] + self.data["threads"] = [thread(resolved=True)] + self.reject("posted findings") + + def test_outdated_unresolved_native_findings_still_block(self): + self.data["threads"] = [thread()] + self.reject("Unresolved Codex") + + def test_old_resolved_findings_allow_fresh_clean_review(self): + self.data["reviews"] = [native_review("2026-09-01T09:00:00Z", "c" * 40)] + self.data["threads"] = [thread(resolved=True), thread(native=False)] + self.assertEqual(adapter.check_evidence(**self.data)["head"], HEAD) + + def test_incomplete_threads_fail_closed(self): + self.data["threads"] = [thread(resolved=True)] + self.data["threads"][0]["comments"]["pageInfo"]["hasNextPage"] = True + self.reject("Incomplete review thread") + + +class PathTests(unittest.TestCase): + def test_rust_and_cargo_changes_require_own_fragment(self): + for path in ("deploy/zakura-watchdog/src/main.rs", "deploy/zakura-watchdog/Cargo.toml"): + for status in ("modified", "removed"): + with self.subTest(path=path, status=status): + files = [{"filename": path, "status": status}] + with self.assertRaisesRegex(adapter.Ineligible, "require this PR's changelog"): + POLICY.check_files(files, 1, 123) + files.append({"filename": "docs/changelog/unreleased/123.md", "status": "added"}) + self.assertEqual(POLICY.check_files(files, 2, 123), files[1]["filename"]) + + def test_eligible_watchdog_change_can_add_its_own_fragment(self): + files = [{"filename": "deploy/zakura-watchdog/src/main.rs", "status": "modified"}, + {"filename": "docs/changelog/unreleased/123.md", "status": "added"}] + self.assertEqual(POLICY.check_files(files, 2, 123), "docs/changelog/unreleased/123.md") + + def test_fragment_does_not_make_application_or_release_changes_eligible(self): + for path in ("crates/zakura-chain/src/lib.rs", ".github/workflows/create-release.yml"): + with self.subTest(path=path): + with self.assertRaises(adapter.Ineligible): + POLICY.check_files([{"filename": path, "status": "modified"}, + {"filename": "docs/changelog/unreleased/123.md", "status": "added"}], 2, 123) + + def test_only_own_new_fragment_is_exempt(self): + for path, status in (("docs/changelog/unreleased/124.md", "added"), + ("docs/changelog/unreleased/123.md", "modified"), + ("docs/changelog/unreleased/123.md", "removed"), + ("docs/changelog/unreleased/123.md", "renamed"), + ("docs/changelog/unreleased/123.md", "copied"), + ("docs/changelog/unreleased/123-extra.md", "added"), + ("docs/changelog/unreleased/README.md", "modified"), + ("CHANGELOG.md", "modified")): + with self.subTest(path=path, status=status): + with self.assertRaises(adapter.Ineligible): + POLICY.check_files([{"filename": "deploy/a.py", "status": "modified"}, + {"filename": path, "status": status}], 2, 123) + + def test_fragment_alone_or_without_pr_identity_does_not_qualify(self): + fragment = {"filename": "docs/changelog/unreleased/123.md", "status": "added"} + with self.assertRaises(adapter.Ineligible): + POLICY.check_files([fragment], 1, 123) + with self.assertRaises(adapter.Ineligible): + POLICY.check_files([{"filename": "deploy/a.py", "status": "modified"}, fragment], 2) + + def test_existing_files_in_all_three_roots(self): + files = [{"filename": p, "status": "modified"} for p in ( + "deploy/deployer/deploy.py", ".github/workflows/lint.yml", + ".github/scripts/upstream-sync-run.sh")] + POLICY.check_files(files, 3) + + def test_release_and_adapter_controls_always_need_humans(self): + paths = [p + "new-file.sh" if p.endswith("/") else p for p in POLICY.data["human_only"]] + paths += ["scripts/sign-release.sh", ".github/review-policy/policy.json", ".github/CODEOWNERS", + ".github/actions/setup-zakura-build/action.yml", "crates/zakura-chain/src/lib.rs"] + for path in paths: + with self.subTest(path=path): + with self.assertRaises(adapter.Ineligible): + POLICY.check_files([{"filename": path, "status": "modified"}], 1) + + def test_mixed_pr_needs_human(self): + with self.assertRaises(adapter.Ineligible): + POLICY.check_files([{"filename": "deploy/a.py", "status": "modified"}, + {"filename": "Cargo.toml", "status": "modified"}], 2) + + def test_source_to_eligible_rename_cannot_hide_source_change(self): + with self.assertRaisesRegex(adapter.Ineligible, "human review"): + POLICY.check_files([{"filename": "deploy/notes.md", "previous_filename": "crates/lib.rs", + "status": "renamed"}], 1) + + def test_release_rename_cannot_hide_release_change(self): + with self.assertRaisesRegex(adapter.Ineligible, "human review"): + POLICY.check_files([{"filename": ".github/workflows/ordinary.yml", "status": "renamed", + "previous_filename": ".github/workflows/create-release.yml"}], 1) + + def test_new_and_renamed_files_need_classification(self): + for status in ("added", "copied", "changed", "renamed"): + with self.subTest(status=status): + with self.assertRaises(adapter.Ineligible): + POLICY.check_files([{"filename": "deploy/new.py", "status": status, + "previous_filename": "deploy/old.py"}], 1) + + def test_path_representation_and_prefix_confusion(self): + for path in ("deploy-other/a", "deploy/../Cargo.toml", "deploy//a", "/deploy/a", "deploy/a\n", + "deploy/./a", "deploy/dir\\a", ".github/workflows-evil/a"): + with self.subTest(path=path): + self.assertFalse(POLICY.eligible_path(path)) + + def test_empty_duplicate_and_truncated_file_lists(self): + file = {"filename": "deploy/a", "status": "modified"} + for files, count in (([], 0), ([file], 2), ([file, file], 2), ([file], 3000)): + with self.subTest(count=count): + with self.assertRaises(adapter.Ineligible): + POLICY.check_files(files, count) + + +class FragmentTests(unittest.TestCase): + def setUp(self): + self.api = Mock() + self.worker = adapter.Adapter(self.api, POLICY, 123) + self.content = "\n\nInternal watchdog tests only.\n" + self.responses = [] + for index, part in enumerate(("docs", "changelog", "unreleased", "123.md")): + self.responses.append({"truncated": False, "tree": [{ + "path": part, "sha": str(index + 1) * 40, + "type": "blob" if index == 3 else "tree", + "mode": "100644" if index == 3 else "040000", + }]}) + self.responses.append({"sha": "4" * 40, "encoding": "base64", "size": len(self.content), + "content": base64.b64encode(self.content.encode()).decode() + "\n"}) + + def check(self): + self.api.request.side_effect = self.responses + self.worker.check_fragment("docs/changelog/unreleased/123.md", HEAD) + + def test_reads_regular_fragment_from_exact_head_tree(self): + self.check() + self.assertTrue(self.api.request.call_args_list[0].args[0].endswith("/git/trees/" + HEAD)) + self.assertTrue(self.api.request.call_args_list[-1].args[0].endswith("/git/blobs/" + "4" * 40)) + + def test_release_waiver_requires_human_review(self): + text = "\n" + self.responses[-1].update(size=len(text), content=base64.b64encode(text.encode()).decode()) + with self.assertRaisesRegex(adapter.Ineligible, "release-policy"): + self.check() + + def test_symlink_executable_and_submodule_cannot_be_fragments(self): + for mode in ("120000", "100755", "160000"): + with self.subTest(mode=mode): + self.responses[3]["tree"][0]["mode"] = mode + with self.assertRaisesRegex(adapter.Ineligible, "regular non-executable"): + self.check() + + def test_parent_symlink_and_truncated_tree_fail_closed(self): + for change in ("symlink", "truncated"): + with self.subTest(change=change): + self.setUp() + if change == "symlink": + self.responses[0]["tree"][0].update(type="blob", mode="120000") + else: + self.responses[0]["truncated"] = True + with self.assertRaises(adapter.Ineligible): + self.check() + + def test_malformed_oversized_and_non_utf8_blob_fail_closed(self): + for change in ("sha", "size", "base64", "utf8"): + with self.subTest(change=change): + self.setUp() + blob = self.responses[-1] + if change == "sha": + blob["sha"] = "5" * 40 + elif change == "size": + blob["size"] = 65537 + elif change == "base64": + blob["content"] = "not base64!" + else: + blob.update(size=1, content=base64.b64encode(b"\xff").decode()) + with self.assertRaises(adapter.Ineligible): + self.check() + + def test_evaluation_checks_fragment_before_native_approval_evidence(self): + pull = {"state": "open", "draft": False, "head": {"sha": HEAD}, "changed_files": 2, + "base": {"ref": "main", "sha": BASE, "repo": {"full_name": POLICY.data["repository"]}}} + self.api.request.return_value = pull + self.api.pages.return_value = [ + {"filename": "deploy/a.py", "status": "modified"}, + {"filename": "docs/changelog/unreleased/123.md", "status": "added"}, + ] + self.worker.check_fragment = Mock(side_effect=adapter.Ineligible("Release waiver")) + self.worker.check_author = Mock() + self.worker.evidence = Mock() + with self.assertRaisesRegex(adapter.Ineligible, "Release waiver"): + self.worker.evaluate(enforce_rules=False) + self.worker.check_fragment.assert_called_once_with("docs/changelog/unreleased/123.md", HEAD) + self.worker.evidence.assert_not_called() + + +def rules_fixture(): + return [{"type": "pull_request", "ruleset_id": 1, "ruleset_source_type": "Repository", + "parameters": {"required_approving_review_count": 1, + "dismiss_stale_reviews_on_push": False, "require_last_push_approval": False}}, + {"type": "required_status_checks", + "parameters": {"required_status_checks": [{"context": "test success"}]}}] + + +class AuthorTests(unittest.TestCase): + def setUp(self): + self.author = {"id": 900, "login": "contributor", "type": "User"} + self.pull = {"state": "open", "draft": False, "head": {"sha": HEAD}, + "changed_files": 1, "user": self.author, + "base": {"ref": "main", "sha": BASE, + "repo": {"full_name": POLICY.data["repository"]}}} + self.access = {"permission": "write", "role_name": "write", "user": self.author.copy()} + self.api, self.writer = Mock(), Mock() + self.worker = adapter.Adapter(self.api, POLICY, 123, writer=self.writer, + app_id=APP_ID, bot_id=BOT_ID, trusted_sha=BASE) + self.api.request.side_effect = self.request + self.reviews = [] + self.api.pages.side_effect = lambda path: ( + [{"filename": "deploy/a.py", "status": "modified"}] if path.endswith("/files") + else self.reviews) + self.worker.evidence = Mock(return_value=receipt()) + self.writer.request.return_value = {"id": 500, "user": {"id": BOT_ID, "type": "Bot"}} + + def request(self, path): + if path == self.worker.pull_path: + return self.pull + if path.endswith("/collaborators/contributor/permission"): + return self.access + if path.endswith("/rules/branches/main"): + return rules_fixture() + if path.endswith("/rulesets/1"): + return {"enforcement": "active", "bypass_actors": []} + if path.endswith("/commits/main"): + return {"sha": BASE} + raise AssertionError(path) + + def test_write_maintain_admin_and_custom_write_roles_qualify(self): + for permission, role in (("write", "write"), ("write", "maintain"), + ("admin", "admin"), ("write", "custom-developer")): + with self.subTest(role=role): + self.access.update(permission=permission, role_name=role) + self.assertTrue(self.worker.author_gate()["reconcile"]) + self.assertEqual(self.worker.evaluate()["head"], HEAD) + + def test_outsider_read_triage_and_unknown_roles_cannot_approve(self): + for permission in ("read", "none", "triage", "maintain", "unknown", None): + with self.subTest(permission=permission): + self.access["permission"] = permission + # Neither organization association nor an admin event sender is authorization. + self.pull.update(author_association="MEMBER", sender={"login": "admin"}) + self.assertFalse(self.worker.author_gate()["reconcile"]) + self.assertFalse(self.worker.reconcile()["approved"]) + self.worker.evidence.assert_not_called() + self.writer.request.assert_not_called() + + def test_permission_response_must_match_author_immutable_id(self): + self.access["user"]["id"] = 901 + with self.assertRaisesRegex(adapter.Ineligible, "does not match"): + self.worker.evaluate() + + def test_author_gate_preserves_automatic_and_ordinary_manual_reviews(self): + for trigger in ("New commits", "Manual request"): + with self.subTest(trigger=trigger): + self.setUp() + data = evidence(trigger) + if trigger == "Manual request": + data["comments"].append(command()) + self.worker.evidence.side_effect = lambda _: adapter.check_evidence(**data) + self.assertTrue(self.worker.reconcile()["approved"]) + self.writer.reset_mock() + self.access["permission"] = "read" + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_bots_and_deleted_authors_cannot_qualify(self): + for author in (None, {}, {**self.author, "type": "Bot"}): + self.pull["user"] = author + self.assertFalse(self.worker.author_gate()["reconcile"]) + + def test_unavailable_permissions_never_qualify(self): + self.api.request.side_effect = adapter.APIError("GitHub GET failed with HTTP 404") + self.assertFalse(self.worker.author_gate()["reconcile"]) + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_revoked_author_keeps_cleanup_job_and_withdraws_approval(self): + self.reviews = [owned_review()] + self.access["permission"] = "read" + self.assertTrue(self.worker.author_gate()["reconcile"]) + self.assertEqual(self.worker.reconcile()["dismissed"], 1) + self.assertEqual([c.args[1] for c in self.writer.request.call_args_list], ["PUT"]) + + def test_retargeted_pr_keeps_cleanup_job_but_cannot_receive_approval(self): + self.pull["base"]["ref"] = "release/v1" + self.reviews = [owned_review()] + self.assertTrue(self.worker.author_gate()["reconcile"]) + self.assertEqual(self.worker.reconcile()["dismissed"], 1) + self.assertEqual([c.args[1] for c in self.writer.request.call_args_list], ["PUT"]) + self.reviews = [] + self.writer.reset_mock() + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_unavailable_permissions_keep_cleanup_of_existing_approval(self): + self.reviews = [owned_review()] + self.api.request.side_effect = adapter.APIError("Unavailable") + self.assertTrue(self.worker.author_gate()["reconcile"]) + self.assertEqual(self.worker.reconcile()["dismissed"], 1) + + def test_preflight_does_not_authorize_a_later_approval(self): + self.assertTrue(self.worker.author_gate()["reconcile"]) + self.access["permission"] = "none" + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_revocation_during_evaluation_prevents_approval(self): + def evidence(_): + self.access["permission"] = "none" + return receipt() + self.worker.evidence.side_effect = evidence + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_revocation_during_post_withdraws_new_approval(self): + def post(*_): + self.access["permission"] = "none" + return {"id": 500, "user": {"id": BOT_ID, "type": "Bot"}} + self.writer.request.side_effect = post + with self.assertRaises(adapter.Ineligible): + self.worker.reconcile() + self.assertEqual([c.args[1] for c in self.writer.request.call_args_list], ["POST", "PUT"]) + + +class RequesterTests(unittest.TestCase): + def setUp(self): + self.case = AuthorTests() + self.case.setUp() + self.worker = self.case.worker + self.data = evidence() + self.outsider = {"id": 901, "login": "outsider", "type": "User"} + self.permissions = {"permission": "read", "role_name": "triage", "user": self.outsider} + self.case.api.request.side_effect = self.request + self.case.api.pages.side_effect = self.pages + self.case.api.graphql.return_value = {"node": self.data["summary"]} + self.worker.threads = Mock(return_value=[]) + self.worker.evidence = adapter.Adapter.evidence.__get__(self.worker) + + def request(self, path): + if path.endswith("/collaborators/outsider/permission"): + if isinstance(self.permissions, Exception): + raise self.permissions + return self.permissions + if path.endswith("/commits/" + HEAD[:7]): + return {"sha": HEAD} + return self.case.request(path) + + def pages(self, path): + if path.endswith("/comments"): + return self.data["comments"] + if path.endswith("/reactions"): + return self.data["reactions"] + if path.endswith("/reviews"): + return self.case.reviews + if path.endswith("/files"): + return [{"filename": "deploy/a.py", "status": "modified"}] + raise AssertionError(path) + + def outsider_command(self): + comment = command(created="2026-09-01T10:03:00Z") + comment.update(user=self.outsider, author_association="MEMBER") + self.data["comments"].append(comment) + return comment + + def test_outsider_new_or_edited_command_cannot_withdraw_approval(self): + self.case.reviews = [owned_review()] + comment = self.outsider_command() + for created in ("2026-09-01T10:03:00Z", "2026-09-01T09:59:00Z"): + with self.subTest(created=created): + comment["created_at"] = created + self.assertTrue(self.worker.reconcile()["approved"]) + self.case.writer.request.assert_not_called() + + def test_missing_collaborator_is_ignored(self): + self.permissions = adapter.APIError("Not found", status=404) + self.case.reviews = [owned_review()] + self.outsider_command() + self.assertTrue(self.worker.reconcile()["approved"]) + self.case.writer.request.assert_not_called() + + def test_outsider_cannot_supply_manual_request_evidence(self): + self.data = evidence("Manual request") + self.case.api.graphql.return_value = {"node": self.data["summary"]} + comment = self.outsider_command() + comment.update(created_at="2026-09-01T09:59:58Z", updated_at="2026-09-01T09:59:58Z") + self.assertIn("Manual review request is missing", self.worker.reconcile()["reason"]) + self.case.writer.request.assert_not_called() + + def test_authorized_request_still_withdraws_old_approval(self): + self.case.reviews = [owned_review()] + self.data["comments"].append(command(created="2026-09-01T10:03:00Z")) + self.assertEqual(self.worker.reconcile()["dismissed"], 1) + self.assertEqual(self.case.writer.request.call_args.args[1], "PUT") + + def test_requester_permissions_are_cached_only_within_one_evaluation(self): + self.outsider_command() + self.outsider_command() + self.worker.evidence(self.case.pull) + lookups = [c for c in self.case.api.request.call_args_list + if c.args[0].endswith("/collaborators/outsider/permission")] + self.assertEqual(len(lookups), 1) + self.permissions["permission"] = "write" + with self.assertRaisesRegex(adapter.Ineligible, "newer or edited"): + self.worker.evidence(self.case.pull) + + def test_permission_service_failure_is_not_treated_as_denied_access(self): + self.outsider_command() + self.permissions = adapter.APIError("Unavailable", status=503) + with self.assertRaises(adapter.APIError): + self.worker.evidence(self.case.pull) + + +class RulesTests(unittest.TestCase): + def setUp(self): + self.rules = rules_fixture() + self.full = {"enforcement": "active", "bypass_actors": []} + self.api = Mock() + self.api.request.side_effect = lambda path: self.full if "/rulesets/" in path else self.rules + + def test_normal_approval_rules_work_without_a_reviewer_team(self): + adapter.check_rules(self.api, POLICY) + + def test_stale_review_settings_are_not_required_fields(self): + del self.rules[0]["parameters"]["dismiss_stale_reviews_on_push"] + del self.rules[0]["parameters"]["require_last_push_approval"] + adapter.check_rules(self.api, POLICY) + + def test_existing_stale_review_preferences_are_accepted_and_preserved(self): + for dismiss in (False, True): + for last_push in (False, True): + with self.subTest(dismiss=dismiss, last_push=last_push): + self.rules = rules_fixture() + self.rules[0]["parameters"]["dismiss_stale_reviews_on_push"] = dismiss + self.rules[0]["parameters"]["require_last_push_approval"] = last_push + before = deepcopy(self.rules) + adapter.check_rules(self.api, POLICY) + self.assertEqual(self.rules, before) + + def test_existing_reviewer_rules_are_preserved(self): + self.rules[0]["parameters"]["required_reviewers"] = [ + {"file_patterns": ["crates/**"], "minimum_approvals": 1, + "reviewer": {"id": 42, "type": "Team"}}] + self.rules[0]["parameters"]["require_code_owner_review"] = True + before = deepcopy(self.rules) + adapter.check_rules(self.api, POLICY) + self.assertEqual(self.rules, before) + + def test_missing_approval_test_gate_or_bypass_stops_approval(self): + for change in ("approval", "ci", "bypass", "evaluate"): + with self.subTest(change=change): + self.setUp() + if change == "approval": + self.rules[0]["parameters"]["required_approving_review_count"] = 0 + if change == "ci": + self.rules.pop() + if change == "bypass": + self.full["bypass_actors"] = [{"actor_type": "Integration", "actor_id": APP_ID}] + if change == "evaluate": + self.full["enforcement"] = "evaluate" + with self.assertRaises(adapter.Ineligible): + adapter.check_rules(self.api, POLICY) + + +def receipt(): + return {**adapter.check_evidence(**evidence()), "base": BASE} + + +def owned_review(state="APPROVED", expected=None, identity=BOT_ID): + expected = expected or receipt() + return {"id": 400, "state": state, "user": {"id": identity, "type": "Bot"}, + "commit_id": expected["head"], + "body": adapter.RECEIPT_MARKER + json.dumps(expected, sort_keys=True, separators=(",", ":")) + " -->\n"} + + +class ReconcileTests(unittest.TestCase): + def setUp(self): + self.api, self.writer = Mock(), Mock() + self.api.pages.return_value = [] + self.worker = adapter.Adapter(self.api, POLICY, 1, writer=self.writer, + app_id=APP_ID, bot_id=BOT_ID, trusted_sha=BASE) + self.worker.check_trusted_revision = Mock() + self.worker.evaluate = Mock(return_value=receipt()) + self.writer.request.return_value = {"id": 500, "user": {"id": BOT_ID, "type": "Bot"}} + + def writes(self): + return [(c.args[1], c.args[0]) for c in self.writer.request.call_args_list] + + def test_approve_exact_commit_once(self): + result = self.worker.reconcile() + self.assertTrue(result["approved"]) + self.assertEqual(self.writer.request.call_args.args[2]["commit_id"], HEAD) + self.assertEqual(self.writes(), [("POST", self.worker.pull_path + "/reviews")]) + self.assertEqual(self.worker.evaluate.call_count, 3) + + def test_existing_current_approval_is_idempotent(self): + self.api.pages.return_value = [owned_review()] + self.assertTrue(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_stale_approval_is_dismissed_without_touching_human_or_other_bot(self): + self.api.pages.return_value = [owned_review(), owned_review(identity=900), + {"id": 600, "state": "APPROVED", "body": "Looks good", + "user": {"id": 901, "type": "User"}, "commit_id": HEAD}] + self.worker.evaluate.side_effect = adapter.Ineligible("New head is not reviewed") + result = self.worker.reconcile() + self.assertFalse(result["approved"]) + self.assertEqual(self.writes(), [("PUT", self.worker.pull_path + "/reviews/400/dismissals")]) + + def test_missing_rules_or_api_failure_withdraws_existing_approval(self): + for error in (adapter.Ineligible("Rules missing"), adapter.APIError("Unavailable")): + with self.subTest(error=error): + self.setUp() + self.api.pages.return_value = [owned_review()] + self.worker.evaluate.side_effect = error + self.assertEqual(self.worker.reconcile()["dismissed"], 1) + + def test_changed_receipt_is_replaced_after_dismissal(self): + stale = {**receipt(), "head": "c" * 40} + self.api.pages.return_value = [owned_review(expected=stale)] + self.assertTrue(self.worker.reconcile()["approved"]) + self.assertEqual([method for method, _ in self.writes()], ["PUT", "POST"]) + + def test_do_not_reapprove_an_explicitly_dismissed_episode(self): + self.api.pages.return_value = [owned_review("DISMISSED")] + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def dismissal_event(self, actor=BOT_ID): + return {"event": "review_dismissed", "actor": {"id": actor, "type": "Bot"}, + "dismissed_review": {"review_id": 400, "state": "approved", + "dismissal_message": adapter.WITHDRAWAL_MESSAGE}} + + def set_dismissal_history(self, events): + self.api.pages.side_effect = lambda path: ( + events if path.endswith("/timeline") else [owned_review("DISMISSED")]) + + def test_restore_own_withdrawal_after_api_recovers(self): + self.api.pages.return_value = [owned_review()] + self.worker.evaluate.side_effect = adapter.APIError("Temporary outage") + self.assertEqual(self.worker.reconcile()["dismissed"], 1) + self.worker.evaluate.side_effect = None + self.writer.reset_mock() + self.set_dismissal_history([self.dismissal_event()]) + self.assertTrue(self.worker.reconcile()["approved"]) + self.assertEqual(self.writes(), [("POST", self.worker.pull_path + "/reviews")]) + + def test_dismissal_recovery_requires_unambiguous_adapter_actor_and_message(self): + own = self.dismissal_event() + wrong_message = deepcopy(own) + wrong_message["dismissed_review"]["dismissal_message"] = "Operator dismissal" + wrong_review = deepcopy(own) + wrong_review["dismissed_review"]["review_id"] = 999 + missing_actor = {**own, "actor": None} + for events in ([], [self.dismissal_event(900)], [wrong_message], [wrong_review], [missing_actor], + [own, own], [own, self.dismissal_event(900)]): + with self.subTest(events=events): + self.set_dismissal_history(events) + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_restore_own_withdrawal_after_trusted_checkout_refresh(self): + self.api.pages.return_value = [owned_review()] + self.worker.check_trusted_revision.side_effect = adapter.Ineligible("Trusted branch advanced") + self.assertEqual(self.worker.reconcile()["dismissed"], 1) + self.worker.check_trusted_revision.side_effect = None + self.set_dismissal_history([self.dismissal_event()]) + self.assertTrue(self.worker.reconcile()["approved"]) + + def test_human_dismissal_blocks_even_with_an_older_automatic_withdrawal(self): + human = {**owned_review("DISMISSED"), "id": 401} + self.api.pages.side_effect = lambda path: ( + [self.dismissal_event()] if path.endswith("/timeline") + else [owned_review("DISMISSED"), human]) + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_unavailable_dismissal_history_cannot_restore_approval(self): + self.api.pages.side_effect = [[owned_review("DISMISSED")], adapter.APIError("Unavailable")] + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_base_or_policy_update_does_not_override_episode_dismissal(self): + for key in ("base", "policy"): + with self.subTest(key=key): + self.setUp() + self.api.pages.return_value = [owned_review("DISMISSED", {**receipt(), key: "c" * 40})] + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_push_or_new_review_before_post_never_approves(self): + self.worker.evaluate.side_effect = [receipt(), adapter.Ineligible("New review is running")] + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_push_during_post_dismisses_just_created_approval(self): + self.worker.evaluate.side_effect = [receipt(), receipt(), {**receipt(), "head": "c" * 40}] + with self.assertRaises(adapter.Ineligible): + self.worker.reconcile() + self.assertEqual([method for method, _ in self.writes()], ["POST", "PUT"]) + self.assertTrue(self.writes()[-1][1].endswith("/500/dismissals")) + + def test_ambiguous_post_is_reconciled_without_post_retry(self): + self.writer.request.side_effect = [adapter.APIError("Timed out"), None] + self.api.pages.side_effect = [[], [owned_review()]] + with self.assertRaises(adapter.APIError): + self.worker.reconcile() + self.assertEqual([method for method, _ in self.writes()], ["POST", "PUT"]) + + def test_different_bot_token_is_detected_and_its_review_removed(self): + self.writer.request.return_value = {"id": 500, "user": {"id": 999, "type": "Bot"}} + with self.assertRaisesRegex(adapter.Ineligible, "configured App"): + self.worker.reconcile() + self.assertEqual([method for method, _ in self.writes()], ["POST", "PUT"]) + + def test_trusted_main_advancing_before_post_withholds_approval(self): + self.worker.check_trusted_revision.side_effect = [None, adapter.Ineligible("Main advanced")] + self.assertFalse(self.worker.reconcile()["approved"]) + self.writer.request.assert_not_called() + + def test_closed_draft_or_retargeted_pr_is_ineligible(self): + original = {"state": "open", "draft": False, "head": {"sha": HEAD}, + "base": {"ref": "main", "repo": {"full_name": POLICY.data["repository"]}}} + for change in ("closed", "draft", "base"): + with self.subTest(change=change): + pull = deepcopy(original) + if change == "closed": + pull["state"] = "closed" + elif change == "draft": + pull["draft"] = True + else: + pull["base"]["ref"] = "release/v1" + self.api.request.return_value = pull + with self.assertRaises(adapter.Ineligible): + adapter.Adapter(self.api, POLICY, 1).evaluate() + + + +class NotificationTests(unittest.TestCase): + def setUp(self): + self.reviews = [owned_review()] + self.api, self.writer = Mock(), Mock() + self.api.pages.side_effect = lambda _: deepcopy(self.reviews) + self.worker = adapter.Adapter(self.api, POLICY, 123, writer=self.writer, + app_id=APP_ID, bot_id=BOT_ID, trusted_sha=BASE) + self.worker.check_trusted_revision = Mock() + self.worker.evaluate = Mock(return_value=receipt()) + self.writer.request.side_effect = self.write + self.post_patch = patch("slack_notify.Slack.post", return_value="1788909592.000001") + self.post = self.post_patch.start() + self.addCleanup(self.post_patch.stop) + + def write(self, path, method, payload): + self.assertEqual(method, "PUT") + review = next(r for r in self.reviews if path.endswith(f"/reviews/{r['id']}")) + review["body"] = payload["body"] + return deepcopy(review) + + def notify(self): + return slack_notify.notify(self.worker, "unused", approved=True) + + def test_failed_approval_verification_only_allows_withdrawal_notifications(self): + self.assertEqual(slack_notify.notify(self.worker, "unused", approved=False), 0) + self.post.assert_not_called() + self.notify() + self.reviews[0]["state"] = "DISMISSED" + self.assertEqual(slack_notify.notify(self.worker, "unused", approved=False), 1) + self.assertIn("withdrawn", self.post.call_args.args[1]) + + def test_approve_withdraw_reapprove_stays_in_one_thread_across_runs(self): + original = self.reviews[0]["body"].splitlines()[0] + self.assertEqual(self.notify(), 1) + self.assertIsNone(self.post.call_args.args[2]) + root = slack_notify.checkpoint(self.reviews[0])["thread_ts"] + self.assertEqual(self.notify(), 0) + self.reviews[0]["state"] = "DISMISSED" + self.assertEqual(self.notify(), 1) + self.assertEqual(self.post.call_args.args[2], root) + self.assertIn("withdrawn", self.post.call_args.args[1]) + self.assertEqual(self.notify(), 0) + self.reviews.append({**owned_review(), "id": 500}) + self.assertEqual(self.notify(), 1) + self.assertEqual(self.post.call_args.args[2], root) + self.assertIn("Zakura #123", self.post.call_args.args[1]) + self.assertEqual(self.notify(), 0) + self.assertEqual(self.post.call_count, 3) + self.assertEqual(self.reviews[0]["body"].splitlines()[0], original) + self.worker.automatic_withdrawals = Mock(return_value={400}) + self.assertTrue(self.worker.reconcile()["approved"]) + self.assertNotIn("review_id", self.worker.reconcile()) + + def test_delayed_withdrawal_is_sent_before_reapproval(self): + self.notify() + self.reviews[0]["state"] = "DISMISSED" + self.reviews.append({**owned_review(), "id": 500}) + self.assertEqual(self.notify(), 2) + calls = self.post.call_args_list + self.assertIn("withdrawn", calls[-2].args[1]) + self.assertNotIn("withdrawn", calls[-1].args[1]) + self.assertEqual(calls[-2].args[2], calls[-1].args[2]) + + def test_no_messages_for_human_native_or_unannounced_dismissed_reviews(self): + self.reviews = [owned_review(identity=999), native_review(), owned_review("DISMISSED")] + self.assertEqual(self.notify(), 0) + self.post.assert_not_called() + self.writer.request.assert_not_called() + + def test_missing_token_does_not_checkpoint_or_change_approval(self): + with self.assertRaisesRegex(slack_notify.NotificationError, "SLACK_BOT_TOKEN"): + slack_notify.notify(self.worker, "", approved=True) + self.writer.request.assert_not_called() + self.post.assert_not_called() + self.assertEqual(self.reviews[0]["state"], "APPROVED") + + def test_definite_rejection_can_retry_without_a_duplicate_thread(self): + self.post.side_effect = slack_notify.Rejected("not_in_channel") + with self.assertRaises(slack_notify.Rejected): + self.notify() + self.assertIsNone(slack_notify.checkpoint(self.reviews[0])["pending"]) + self.post.side_effect = None + self.assertEqual(self.notify(), 1) + self.assertIsNone(self.post.call_args.args[2]) + self.assertEqual(self.notify(), 0) + + def test_uncertain_post_is_not_repeated_even_after_approval_withdrawal(self): + self.post.side_effect = slack_notify.NotificationError("delivery is uncertain") + with self.assertRaises(slack_notify.NotificationError): + self.notify() + self.reviews[0]["state"] = "DISMISSED" + self.post.side_effect = None + with self.assertRaisesRegex(slack_notify.NotificationError, "pending"): + self.notify() + self.assertEqual(self.post.call_count, 1) + + def test_failed_initial_checkpoint_never_posts(self): + self.writer.request.side_effect = adapter.APIError("GitHub unavailable") + with self.assertRaises(adapter.APIError): + self.notify() + self.post.assert_not_called() + + def test_lost_receipt_write_does_not_repeat_successful_slack_post(self): + calls = 0 + + def write(path, method, payload): + nonlocal calls + calls += 1 + if calls == 2: + raise adapter.APIError("GitHub unavailable") + return self.write(path, method, payload) + + self.writer.request.side_effect = write + with self.assertRaises(adapter.APIError): + self.notify() + self.writer.request.side_effect = self.write + with self.assertRaisesRegex(slack_notify.NotificationError, "pending"): + self.notify() + self.assertEqual(self.post.call_count, 1) + + def test_author_revocation_keeps_pending_withdrawal_delivery_reachable(self): + self.notify() + self.reviews[0]["state"] = "DISMISSED" + self.worker.check_author = Mock(side_effect=adapter.Ineligible("Access revoked")) + self.assertTrue(self.worker.author_gate()["reconcile"]) + self.notify() + self.assertFalse(self.worker.author_gate()["reconcile"]) + + def test_corrupted_or_conflicting_checkpoint_stops_delivery(self): + self.notify() + saved = deepcopy(self.reviews[0]) + for body in (saved["body"] + slack_notify.MARKER, + saved["body"].replace('"sent":["APPROVED"]', '"sent":["DISMISSED"]'), + saved["body"].replace(POLICY.data["slack_channel"], "COTHER")): + with self.subTest(body=body): + self.reviews[0]["body"] = body + with self.assertRaises(slack_notify.NotificationError): + self.notify() + self.reviews = [saved, {**saved, "id": 500, + "body": saved["body"].replace("1788909592.000001", "1788909592.000002")}] + with self.assertRaisesRegex(slack_notify.NotificationError, "Conflicting"): + self.notify() + self.assertEqual(self.post.call_count, 1) + + +class SlackAPITests(unittest.TestCase): + def test_post_returns_timestamp_and_uses_thread_without_broadcast(self): + with patch("urllib.request.urlopen") as open_url: + response = open_url.return_value.__enter__.return_value + response.read.return_value = json.dumps({"ok": True, "channel": "C123", + "ts": "1788909592.000002"}).encode() + self.assertEqual(slack_notify.Slack("unused").post("C123", "message", "1788909592.000001"), + "1788909592.000002") + request = open_url.call_args.args[0] + self.assertEqual(request.full_url, "https://slack.com/api/chat.postMessage") + payload = json.loads(request.data) + self.assertEqual(payload["thread_ts"], "1788909592.000001") + self.assertFalse(payload.get("reply_broadcast")) + self.assertFalse(payload["unfurl_links"]) + self.assertEqual(open_url.call_args.kwargs["timeout"], 30) + + def test_definite_slack_rejection_is_retryable(self): + with patch("urllib.request.urlopen") as open_url: + open_url.return_value.__enter__.return_value.read.return_value = b'{"ok":false,"error":"not_in_channel"}' + with self.assertRaisesRegex(slack_notify.Rejected, "not_in_channel"): + slack_notify.Slack("unused").post("C123", "message", None) + + def test_rate_limit_is_retryable_but_server_failure_is_uncertain(self): + for status in (429, 503): + with self.subTest(status=status), patch("urllib.request.urlopen", side_effect= + adapter.urllib.error.HTTPError("https://slack.com", status, "error", {}, None)): + with self.assertRaises(slack_notify.NotificationError) as caught: + slack_notify.Slack("unused").post("C123", "message", None) + self.assertEqual(isinstance(caught.exception, slack_notify.Rejected), status == 429) + + def test_malformed_or_incomplete_success_is_uncertain(self): + for raw in (b"{", b"[]", b'{"ok":true}', b'x' * (1024 * 1024 + 1), + b'{"ok":true,"channel":"COTHER","ts":"1788909592.000001"}'): + with self.subTest(raw=raw[:80]), patch("urllib.request.urlopen") as open_url: + open_url.return_value.__enter__.return_value.read.return_value = raw + with self.assertRaises(slack_notify.NotificationError) as caught: + slack_notify.Slack("unused").post("C123", "message", None) + self.assertNotIsInstance(caught.exception, slack_notify.Rejected) + + def test_timeout_does_not_expose_request_details(self): + with patch("urllib.request.urlopen", side_effect=TimeoutError("secret request detail")): + with self.assertRaises(slack_notify.NotificationError) as caught: + slack_notify.Slack("unused").post("C123", "message", None) + self.assertNotIn("secret", str(caught.exception)) + + +class APITests(unittest.TestCase): + def test_http_status_survives_as_api_error(self): + with adapter.urllib.error.HTTPError( + "https://api.github.com/test", 404, "Not found", {}, None) as error, \ + patch("urllib.request.urlopen", side_effect=error): + with self.assertRaises(adapter.APIError) as result: + adapter.GitHub("unused").request("/test") + self.assertEqual(result.exception.status, 404) + + def test_partial_network_response_is_an_api_error(self): + with patch("urllib.request.urlopen") as open_url: + open_url.return_value.__enter__.return_value.read.side_effect = http.client.IncompleteRead(b"") + with self.assertRaises(adapter.APIError): + adapter.GitHub("unused").request("/test", "POST", {}) + + def test_rest_pagination_is_complete(self): + api = adapter.GitHub("unused") + api.request = Mock(side_effect=[[{}] * 100, [{"id": 1}]]) + self.assertEqual(len(api.pages("/test")), 101) + self.assertIn("page=2", api.request.call_args.args[0]) + + def test_excessive_pagination_fails_closed(self): + api = adapter.GitHub("unused") + api.request = Mock(return_value=[{}] * 100) + with self.assertRaises(adapter.Ineligible): + api.pages("/test") + self.assertEqual(api.request.call_count, adapter.MAX_PAGES) + + def test_graphql_partial_data_is_an_error(self): + api = adapter.GitHub("unused") + api.request = Mock(return_value={"data": {"node": {}}, "errors": [{"message": "unavailable"}]}) + with self.assertRaises(adapter.APIError): + api.graphql("query { node }") + + def test_event_targets_use_metadata_not_comment_text(self): + api = Mock() + numbers = adapter.target_numbers(api, POLICY, {"issue": {"number": 17, "pull_request": {"url": "unused"}}, + "comment": {"body": "@codex review PR 1234"}}) + self.assertEqual(numbers, [17]) + api.pages.assert_not_called() + + def test_retargeted_pr_is_checked_by_event_and_periodic_cleanup(self): + api = Mock() + moved = {"number": 17, "base": {"ref": "release/v1"}} + event = {"action": "edited", "pull_request": moved, + "changes": {"base": {"ref": {"from": "main"}}}} + self.assertEqual(adapter.target_numbers(api, POLICY, event), [17]) + api.pages.assert_not_called() + api.pages.return_value = [moved, {"number": 18, "base": {"ref": "main"}}] + self.assertEqual(adapter.target_numbers(api, POLICY, {}), [17, 18]) + api.pages.assert_called_once_with(f"/repos/{POLICY.data['repository']}/pulls?state=open") + + +class CLITests(unittest.TestCase): + def test_slack_failure_reports_error_without_repeating_or_reversing_approval(self): + environment = {"GH_TOKEN": "unused", "GH_APPROVAL_TOKEN": "unused", + "CODEX_APPROVAL_ENABLED": "true", "CODEX_APPROVAL_APP_SLUG": "approval", + "CODEX_APPROVAL_APP_ID": str(APP_ID), "CODEX_APPROVAL_APP_CLIENT_ID": "client", + "CODEX_APPROVAL_BOT_ID": str(BOT_ID)} + with (patch.dict(os.environ, environment, clear=True), + patch("sys.argv", ["adapter.py", "--pr", "1", "--apply"]), + patch("adapter.GitHub") as github, patch("adapter.Adapter") as worker, + patch("slack_notify.notify", side_effect=slack_notify.NotificationError("delivery failed")), + patch("builtins.print") as output): + github.return_value.request.side_effect = [ + {"id": APP_ID, "client_id": "client"}, {"id": BOT_ID, "type": "Bot"}] + worker.return_value.reconcile.return_value = {"approved": True} + self.assertEqual(adapter.main(), 1) + result = json.loads(output.call_args.args[0]) + self.assertTrue(result["approved"]) + self.assertEqual(result["notification_error"], "delivery failed") + worker.return_value.reconcile.assert_called_once() + worker.return_value.dismiss.assert_not_called() + + def test_author_preflight_gates_single_and_scheduled_batches_without_writer(self): + for decisions, expected in (([False], "false"), ([True], "true"), + ([False, True], "true"), ([False, False], "false"), ([], "false")): + with self.subTest(decisions=decisions), tempfile.TemporaryDirectory() as directory: + output = Path(directory) / "output" + with (patch.dict(os.environ, {"GH_TOKEN": "unused", "GITHUB_OUTPUT": str(output)}, clear=True), + patch("sys.argv", ["adapter.py", "--check-authors"]), + patch("adapter.GitHub") as github, + patch("adapter.target_numbers", return_value=list(range(1, len(decisions) + 1))), + patch("adapter.Adapter") as worker, patch("builtins.print")): + worker.return_value.author_gate.side_effect = [{"reconcile": d} for d in decisions] + self.assertEqual(adapter.main(), 0) + self.assertEqual(output.read_text(), f"run_reconcile={expected}\n") + self.assertEqual(github.call_count, 1) + worker.return_value.reconcile.assert_not_called() + worker.return_value.evaluate.assert_not_called() + + def test_default_mode_never_constructs_writer_or_reconciles(self): + with (patch.dict(os.environ, {"GH_TOKEN": "unused"}, clear=True), + patch("sys.argv", ["adapter.py", "--pr", "1"]), + patch("adapter.GitHub") as github, + patch("adapter.Adapter") as worker, + patch("builtins.print")): + worker.return_value.evaluate.return_value = receipt() + self.assertEqual(adapter.main(), 0) + self.assertEqual(github.call_count, 1) + worker.return_value.reconcile.assert_not_called() + self.assertIsNone(worker.call_args.kwargs["writer"]) + + def test_apply_flag_cannot_override_disabled_repository_setting(self): + with (patch.dict(os.environ, {"GH_TOKEN": "unused", "CODEX_APPROVAL_ENABLED": "false"}, clear=True), + patch("sys.argv", ["adapter.py", "--pr", "1", "--apply"]), + patch("adapter.GitHub") as github): + with self.assertRaisesRegex(adapter.Ineligible, "disabled"): + adapter.main() + github.return_value.request.assert_not_called() + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 1281ae8e3..36a69dedc 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -29,6 +29,7 @@ These workflows run on pull requests, pushes to `main` / `feat/**` / `release/** | `zakura-e2e.yml` | The heaviest PR-path job, isolated in its own workflow: regtest docker-compose end-to-end gate, multi-node testkit test, block-sync fuzz on every push to `main`, and long four-node modes nightly. PR runs are gated by a `changes` job or the `run-zakura-e2e` label. | PR/push (self-gated), merge queue, nightly, manual | | `docs-check.yml` | markdownlint, codespell, and lychee link checking over all Markdown. | PR/push on Markdown paths | | `changelog.yml` | Requires one fragment for Rust/Cargo.toml PRs and tests release assembly. | Every PR/push/merge group | +| `codex-approval-adapter.yml` | Converts a current, clean native Codex review into an approval for eligible CI/deploy changes. Defaults to a read-only audit; release automation requires human review. See the [approval policy](../review-policy/README.md). | PR metadata/comments; policy changes; hourly reconciliation; manual | | `coverage.yml` | llvm-cov + nextest coverage uploaded to Codecov. A 120-minute instrumented build, kept off the PR path. | Push to `main`/`release/**`, nightly, manual | | `benchmarks.yml` | Criterion benchmarks. Runs on PRs carrying the `C-benchmark` label; results publish to the dashboard data on `gh-pages/dev/bench`. | Labeled PRs, manual | | `zcashd-compat-regtest.yml` | zcashd interoperability regtest suite (spawns fresh `zakurad` + `zcashd`, no external infrastructure). **Temporarily manual-only**: see the workflow header for the sidecar-zcashd re-enable condition. | Manual | diff --git a/.github/workflows/codex-approval-adapter.yml b/.github/workflows/codex-approval-adapter.yml new file mode 100644 index 000000000..4ef4ebccc --- /dev/null +++ b/.github/workflows/codex-approval-adapter.yml @@ -0,0 +1,93 @@ +name: Codex approval adapter + +on: + issue_comment: + types: [created, edited, deleted] + pull_request_target: # zizmor: ignore[dangerous-triggers] -- reads PR metadata only; both jobs check out trusted main and never run PR code + # Retargeting away from main must still withdraw an existing approval. + types: [opened, reopened, synchronize, ready_for_review, converted_to_draft, edited] + push: + branches: [main] + paths: + - .github/review-policy/** + - .github/workflows/codex-approval-adapter.yml + schedule: + - cron: "17 * * * *" + workflow_dispatch: + inputs: + pr: + description: PR number to recheck (blank includes all open base branches for cleanup; does not start a Codex review) + type: string + required: false + +# Serialize writers, including scheduled reconciliation and per-PR events. +# Do not cancel a runner between an approval POST and its verification. +concurrency: + group: codex-approval-adapter + cancel-in-progress: false + +permissions: + contents: read + issues: read + pull-requests: read + +jobs: + audit: + if: >- + github.repository == 'zakura-core/zakura' && + (github.event_name != 'issue_comment' || github.event.issue.pull_request) && + (github.event_name != 'workflow_dispatch' || github.ref == 'refs/heads/main') + runs-on: ubuntu-latest + timeout-minutes: 15 + outputs: + run_reconcile: ${{ steps.authors.outputs.run_reconcile }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 + with: + ref: main + persist-credentials: false + sparse-checkout: .github/review-policy + - name: Check PR authors' repository access + id: authors + env: + GH_TOKEN: ${{ github.token }} + CODEX_APPROVAL_BOT_ID: ${{ vars.CODEX_APPROVAL_BOT_ID }} + run: python3 .github/review-policy/adapter.py --check-authors --event "$GITHUB_EVENT_PATH" + - name: Inspect native Codex evidence + env: + GH_TOKEN: ${{ github.token }} + run: python3 .github/review-policy/adapter.py --event "$GITHUB_EVENT_PATH" --wait-seconds 10 + + reconcile: + needs: audit + if: >- + !cancelled() && needs.audit.result != 'skipped' && + needs.audit.outputs.run_reconcile == 'true' && + vars.CODEX_APPROVAL_ENABLED == 'true' + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 + with: + ref: main + persist-credentials: false + sparse-checkout: .github/review-policy + - name: Mint approval App token + id: approval-token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 #v3.2.0 + with: + client-id: ${{ vars.CODEX_APPROVAL_APP_CLIENT_ID }} + private-key: ${{ secrets.CODEX_APPROVAL_APP_PRIVATE_KEY }} + permission-pull-requests: write + # Omitted owner/repositories scopes the token to this repository. + - name: Revalidate and reconcile approval + env: + GH_TOKEN: ${{ github.token }} + GH_APPROVAL_TOKEN: ${{ steps.approval-token.outputs.token }} + SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} + CODEX_APPROVAL_ENABLED: ${{ vars.CODEX_APPROVAL_ENABLED }} + CODEX_APPROVAL_APP_ID: ${{ vars.CODEX_APPROVAL_APP_ID }} + CODEX_APPROVAL_APP_CLIENT_ID: ${{ vars.CODEX_APPROVAL_APP_CLIENT_ID }} + CODEX_APPROVAL_APP_SLUG: ${{ steps.approval-token.outputs.app-slug }} + CODEX_APPROVAL_BOT_ID: ${{ vars.CODEX_APPROVAL_BOT_ID }} + run: TRUSTED_SHA="$(git rev-parse HEAD)" python3 .github/review-policy/adapter.py --apply --event "$GITHUB_EVENT_PATH" --wait-seconds 10 diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 5260d6d3b..788ce6239 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -14,6 +14,8 @@ on: - deny.toml - rust-toolchain.toml - .github/workflows/lint.yml + - .github/workflows/codex-approval-adapter.yml + - .github/review-policy/** - .github/workflows/cross-platform.yml - docs/specs/fork-aware-header-chain-engine.md - qa/supply-chain/** @@ -38,6 +40,8 @@ on: - deny.toml - rust-toolchain.toml - .github/workflows/lint.yml + - .github/workflows/codex-approval-adapter.yml + - .github/review-policy/** - .github/workflows/cross-platform.yml - docs/specs/fork-aware-header-chain-engine.md - qa/supply-chain/** @@ -191,6 +195,10 @@ jobs: # droplets, where a shell bug costs a full provisioning cycle to find. run: shellcheck scripts/*.sh scripts/lib/*.sh .github/workflows/scripts/*.sh # Stdlib-only, so there is nothing to install. + - name: Codex approval policy tests + if: ${{ !cancelled() }} + run: python3 .github/review-policy/test_adapter.py + - name: Mempool load harness tests if: ${{ !cancelled() }} run: python3 .github/workflows/scripts/test_mempool_load.py