From 7c66489ab34f93893a637c937272ba669f704da5 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 7 Sep 2026 19:27:57 -0600 Subject: [PATCH 01/11] ci: add native Codex approval adapter for scoped changes --- .github/review-policy/README.md | 161 ++++++ .github/review-policy/adapter.py | 526 +++++++++++++++++++ .github/review-policy/policy.json | 25 + .github/review-policy/test_adapter.py | 507 ++++++++++++++++++ .github/workflows/README.md | 1 + .github/workflows/codex-approval-adapter.yml | 87 +++ .github/workflows/lint.yml | 8 + 7 files changed, 1315 insertions(+) create mode 100644 .github/review-policy/README.md create mode 100644 .github/review-policy/adapter.py create mode 100644 .github/review-policy/policy.json create mode 100644 .github/review-policy/test_adapter.py create mode 100644 .github/workflows/codex-approval-adapter.yml diff --git a/.github/review-policy/README.md b/.github/review-policy/README.md new file mode 100644 index 0000000000..911d576408 --- /dev/null +++ b/.github/review-policy/README.md @@ -0,0 +1,161 @@ +# 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 + +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. + +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. + +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. Update the GitHub reviewer patterns with every scope change. + +## 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. + A newer visible review request 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. + +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. + +## Administrator setup + +Keep `CODEX_APPROVAL_ENABLED` unset or `false` until setup and validation finish. + +1. Merge this PR through human review and inspect read-only audit results. +2. Create a dedicated GitHub App and install it only on `zakura-core/zakura`. + Grant repository **Pull requests: read and write** and the mandatory metadata + access. It needs no webhook, contents write, administration, Actions write, or + ruleset bypass. Do not reuse a release App or a person's token. +3. Create the `codex-approval` environment, restricted to the `main` branch. + Keep the App key in Infisical with a dedicated service scope, and sync it to + the environment secret `CODEX_APPROVAL_APP_PRIVATE_KEY`. Required environment + reviewers would make each adapter execution manual, so use the native PR + reviewer rules below for human approval requirements. +4. Set these repository variables from the App and human team metadata: + + | 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 | + | `CODEX_APPROVAL_HUMAN_TEAM_ID` | Numeric ID of the human reviewer team with repository write access | + +5. Update the active `main` repository ruleset. Retain the global one-approval + requirement and the existing required `test success` check. Enable both + **Dismiss stale pull request approvals when new commits are pushed** and + **Require approval of the most recent reviewable push**. Require one approval + from the human team on the ordered patterns printed by: + + ```sh + python3 .github/review-policy/adapter.py --patterns + ``` + + Add them as one **Required reviewers** entry. In the REST ruleset schema this + is `required_reviewers[{file_patterns, minimum_approvals: 1, + reviewer: {id: TEAM_ID, type: "Team"}}]`. Keep the ruleset active with no bypass + actors. Human team approval can satisfy the global requirement too; the App + cannot satisfy the human team requirement for release or application changes. +6. Run the validation below with disposable PRs before enabling normal use. + Finally set `CODEX_APPROVAL_ENABLED=true`. + +The writer independently rereads the ruleset and checks its patterns, team, +freshness controls, 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 protected paths still require a human team member. + +Specifically delay an approval request until after a new commit is pushed and +confirm GitHub will not permit merging based on that old-commit review. The +review API has no atomic expected-current-head precondition. The adapter submits +the full reviewed `commit_id`, rereads state before and after approval, and +withdraws its approval when those reads disagree; GitHub's native freshness +rules must enforce the merge boundary. Mocked tests cannot establish that server +behavior. Do not activate this adapter if that disposable-PR check fails. + +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. +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 and human path rules so ordinary human review keeps working. + +## References + +- [Native Codex GitHub reviews](https://learn.chatgpt.com/docs/third-party/github) +- [GitHub required reviewers 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) diff --git a/.github/review-policy/adapter.py b/.github/review-policy/adapter.py new file mode 100644 index 0000000000..dc798cc804 --- /dev/null +++ b/.github/review-policy/adapter.py @@ -0,0 +1,526 @@ +#!/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 +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 + + +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": + 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']}). Required CI and human path rules 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"])] + # Periodic/default-branch reconciliation also catches missed reaction events, + # revoked reactions, policy changes, and a previous interrupted runner. + branch = urllib.parse.quote(policy.data["base_branch"], safe="") + pulls = api.pages(f"/repos/{policy.data['repository']}/pulls?state=open&base={branch}") + 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) + parser.add_argument("--apply", action="store_true") + parser.add_argument("--patterns", action="store_true", help="Print required human reviewer patterns") + 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() + if args.patterns: + print(json.dumps(policy.human_patterns, indent=2)) + return 0 + 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, + team_id=int(os.environ.get("CODEX_APPROVAL_HUMAN_TEAM_ID") or 0), + 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 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 + results.append({"pr": number, **result}) + print(json.dumps(results[-1], sort_keys=True), flush=True) + 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 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 0000000000..d7b757340f --- /dev/null +++ b/.github/review-policy/policy.json @@ -0,0 +1,25 @@ +{ + "repository": "zakura-core/zakura", + "base_branch": "main", + "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/test_adapter.py b/.github/review-policy/test_adapter.py new file mode 100644 index 0000000000..e4b631c600 --- /dev/null +++ b/.github/review-policy/test_adapter.py @@ -0,0 +1,507 @@ +#!/usr/bin/env python3 +"""Regression tests for the approval boundary; no credentials or network used.""" + +from copy import deepcopy +import http.client +import json +import os +import unittest +from unittest.mock import Mock, patch + +import adapter + + +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, + } + + +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"}} + + +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_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) + + +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": True, "require_last_push_approval": True, + "required_reviewers": [{"file_patterns": POLICY.human_patterns, + "minimum_approvals": 1, + "reviewer": {"id": 42, "type": "Team"}}]}}, + {"type": "required_status_checks", + "parameters": {"required_status_checks": [{"context": "test success"}]}}] + + +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_native_human_paths_and_freshness_are_required(self): + adapter.check_rules(self.api, POLICY, 42) + + def test_current_live_rule_configuration_cannot_enable_adapter(self): + self.rules[0]["parameters"]["required_reviewers"] = [] + self.rules[0]["parameters"]["dismiss_stale_reviews_on_push"] = False + with self.assertRaises(adapter.Ineligible): + adapter.check_rules(self.api, POLICY, 42) + + def test_each_freshness_switch_is_required(self): + for key in ("dismiss_stale_reviews_on_push", "require_last_push_approval"): + with self.subTest(key=key): + self.rules = rules_fixture() + self.rules[0]["parameters"][key] = False + with self.assertRaises(adapter.Ineligible): + adapter.check_rules(self.api, POLICY, 42) + + def test_human_patterns_must_match_including_order_and_exceptions(self): + self.rules[0]["parameters"]["required_reviewers"][0]["file_patterns"] = ["*"] + with self.assertRaises(adapter.Ineligible): + adapter.check_rules(self.api, POLICY, 42) + + def test_missing_team_test_gate_or_bypass_stops_approval(self): + for change in ("team", "ci", "bypass", "evaluate"): + with self.subTest(change=change): + self.setUp() + 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, 0 if change == "team" else 42) + + +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, team_id=42, 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)] + 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 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 APITests(unittest.TestCase): + 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() + + +class CLITests(unittest.TestCase): + 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 1281ae8e3c..36a69dedc1 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 0000000000..265743c433 --- /dev/null +++ b/.github/workflows/codex-approval-adapter.yml @@ -0,0 +1,87 @@ +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 + branches: [main] + 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 checks all open PRs; 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 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 + with: + ref: main + persist-credentials: false + sparse-checkout: .github/review-policy + - 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' && + vars.CODEX_APPROVAL_ENABLED == 'true' + runs-on: ubuntu-latest + timeout-minutes: 20 + # Restrict this environment to main. The App must not be a member of the + # required human team or have a ruleset bypass or administration permission. + environment: codex-approval + 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 }} + 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 }} + CODEX_APPROVAL_HUMAN_TEAM_ID: ${{ vars.CODEX_APPROVAL_HUMAN_TEAM_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 5260d6d3b5..788ce6239d 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 From 3d9eaf78096d25dba79d91c6d147d70405c23260 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 7 Sep 2026 20:19:09 -0600 Subject: [PATCH 02/11] ci: allow PR-owned changelog additions in approval policy --- .github/review-policy/README.md | 14 ++++ .github/review-policy/adapter.py | 53 +++++++++++- .github/review-policy/policy.json | 1 + .github/review-policy/test_adapter.py | 114 ++++++++++++++++++++++++++ 4 files changed, 180 insertions(+), 2 deletions(-) diff --git a/.github/review-policy/README.md b/.github/review-policy/README.md index 911d576408..8580ca7a7a 100644 --- a/.github/review-policy/README.md +++ b/.github/review-policy/README.md @@ -38,6 +38,16 @@ 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 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 @@ -105,6 +115,8 @@ Keep `CODEX_APPROVAL_ENABLED` unset or `false` until setup and validation finish python3 .github/review-policy/adapter.py --patterns ``` + The patterns include the numbered-fragment exception; the adapter additionally + enforces PR ownership, addition-only status, and the release-directive exclusion. Add them as one **Required reviewers** entry. In the REST ruleset schema this is `required_reviewers[{file_patterns, minimum_approvals: 1, reviewer: {id: TEAM_ID, type: "Team"}}]`. Keep the ruleset active with no bypass @@ -133,6 +145,8 @@ 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 protected paths still require a human team member. +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. Specifically delay an approval request until after a new commit is pushed and confirm GitHub will not permit merging based on that old-commit review. The diff --git a/.github/review-policy/adapter.py b/.github/review-policy/adapter.py index dc798cc804..0df24a4565 100644 --- a/.github/review-policy/adapter.py +++ b/.github/review-policy/adapter.py @@ -8,6 +8,8 @@ from __future__ import annotations import argparse +import base64 +import binascii from dataclasses import dataclass from datetime import datetime, timedelta, timezone import hashlib @@ -76,6 +78,8 @@ def digest(self): def human_patterns(self): # GitHub required-reviewer patterns are ordered gitignore patterns. return ["*"] + [f"!/{root}**" for root in self.data["eligible_roots"]] + [ + f"!/{self.data['changelog_fragment_root']}[0-9]*.md" + ] + [ f"/{path}{'**' if path.endswith('/') else ''}" for path in self.data["human_only"] ] @@ -91,10 +95,19 @@ def eligible_path(self, path): for p in self.data["human_only"] ) - def check_files(self, files, expected_count): + def check_files(self, files, expected_count, pr_number=None): + """Return the optional new PR-owned fragment; other changes must qualify.""" require(0 < len(files) == expected_count < 3000, "Incomplete or empty changed-file list") require(len({f["filename"] for f in files}) == len(files), "Duplicate changed files") + fragment = None + own_fragment = (f"{self.data['changelog_fragment_root']}{pr_number}.md" + if isinstance(pr_number, int) and pr_number > 0 else None) for file in files: + if file["filename"] == own_fragment: + require(file.get("status") == "added" and not file.get("previous_filename"), + "Only a newly added changelog fragment for this PR qualifies") + fragment = own_fragment + continue require(file.get("status") in ("modified", "removed", "renamed"), "New or unclassified files require human review") paths = [file["filename"]] @@ -104,6 +117,9 @@ def check_files(self, files, expected_count): require(all(self.eligible_path(p) for p in paths), "Changed files include a path requiring human review") require(file["status"] != "renamed", "Renamed files require human classification") + require(len(files) > int(fragment is not None), + "A changelog fragment must accompany an eligible CI or deployment change") + return fragment def native(self, obj): return (obj.get("user", {}).get("id") == self.data["codex_user_id"] @@ -328,13 +344,46 @@ def evidence(self, pull): return check_evidence(self.policy, pull, comments, summary, reactions, reviews, self.threads(), resolved) + def check_fragment(self, path, head): + """Read the regular fragment blob at the reviewed head without executing it.""" + tree_sha = head + parts = path.split("/") + for index, part in enumerate(parts): + tree = self.api.request(f"{self.prefix}/git/trees/{tree_sha}") + require(tree.get("truncated") is False, "Incomplete changelog tree") + entries = [entry for entry in tree["tree"] if entry["path"] == part] + require(len(entries) == 1, "Changelog fragment is missing from the current commit") + entry = entries[0] + if index < len(parts) - 1: + require(entry["type"] == "tree" and entry["mode"] == "040000", + "Changelog parent must be a directory") + else: + require(entry["type"] == "blob" and entry["mode"] == "100644", + "Changelog fragment must be a regular non-executable file") + require(re.fullmatch(r"[0-9a-f]{40}", entry["sha"]) is not None, "Invalid changelog object ID") + tree_sha = entry["sha"] + blob = self.api.request(f"{self.prefix}/git/blobs/{tree_sha}") + require(blob["sha"] == tree_sha and blob["encoding"] == "base64" + and 0 < blob["size"] <= 65536 and isinstance(blob["content"], str), + "Unsupported changelog fragment blob") + try: + raw = base64.b64decode("".join(blob["content"].splitlines()), validate=True) + content = raw.decode("utf-8") + except (binascii.Error, UnicodeError, ValueError) as exc: + raise Ineligible("Changelog fragment is not valid UTF-8 text") from exc + require(len(raw) == blob["size"], "Incomplete changelog fragment blob") + require("release-readiness" not in content.casefold(), + "Changelog release-policy directives require human review") + def evaluate(self, enforce_rules=True): pull = self.api.request(self.pull_path) require(pull["state"] == "open" and not pull["draft"], "PR is closed or a draft") require(pull["base"]["repo"]["full_name"] == self.repo and pull["base"]["ref"] == self.policy.data["base_branch"], "Unsupported base branch") files = self.api.pages(self.pull_path + "/files") - self.policy.check_files(files, pull["changed_files"]) + fragment = self.policy.check_files(files, pull["changed_files"], self.number) + if fragment: + self.check_fragment(fragment, pull["head"]["sha"]) if enforce_rules: check_rules(self.api, self.policy, self.team_id) receipt = self.evidence(pull) diff --git a/.github/review-policy/policy.json b/.github/review-policy/policy.json index d7b757340f..0ca3f8ca44 100644 --- a/.github/review-policy/policy.json +++ b/.github/review-policy/policy.json @@ -1,6 +1,7 @@ { "repository": "zakura-core/zakura", "base_branch": "main", + "changelog_fragment_root": "docs/changelog/unreleased/", "eligible_roots": [ "deploy/", ".github/workflows/", diff --git a/.github/review-policy/test_adapter.py b/.github/review-policy/test_adapter.py index e4b631c600..ec6e19b0e7 100644 --- a/.github/review-policy/test_adapter.py +++ b/.github/review-policy/test_adapter.py @@ -2,6 +2,7 @@ """Regression tests for the approval boundary; no credentials or network used.""" from copy import deepcopy +import base64 import http.client import json import os @@ -225,6 +226,39 @@ def test_incomplete_threads_fail_closed(self): class PathTests(unittest.TestCase): + 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", @@ -276,6 +310,86 @@ def test_empty_duplicate_and_truncated_file_lists(self): 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.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, From 2673f1858e2e01588c7d78037d69e7fe19e18dda Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 7 Sep 2026 20:52:11 -0600 Subject: [PATCH 03/11] ci: use normal review process when adapter cannot approve --- .github/review-policy/README.md | 42 ++++++++---------- .github/review-policy/adapter.py | 46 ++++++-------------- .github/review-policy/test_adapter.py | 44 ++++++++++--------- .github/workflows/codex-approval-adapter.yml | 5 +-- 4 files changed, 58 insertions(+), 79 deletions(-) diff --git a/.github/review-policy/README.md b/.github/review-policy/README.md index 8580ca7a7a..0e7cf58273 100644 --- a/.github/review-policy/README.md +++ b/.github/review-policy/README.md @@ -18,6 +18,12 @@ 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, @@ -52,7 +58,7 @@ 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. Update the GitHub reviewer patterns with every scope change. +human-reviewed PR. ## Native review evidence @@ -94,38 +100,28 @@ Keep `CODEX_APPROVAL_ENABLED` unset or `false` until setup and validation finish 3. Create the `codex-approval` environment, restricted to the `main` branch. Keep the App key in Infisical with a dedicated service scope, and sync it to the environment secret `CODEX_APPROVAL_APP_PRIVATE_KEY`. Required environment - reviewers would make each adapter execution manual, so use the native PR - reviewer rules below for human approval requirements. -4. Set these repository variables from the App and human team metadata: + reviewers would make each adapter execution manual, so leave them unset for + normal automatic operation. +4. Set these repository variables from the App metadata: | 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 | - | `CODEX_APPROVAL_HUMAN_TEAM_ID` | Numeric ID of the human reviewer team with repository write access | 5. Update the active `main` repository ruleset. Retain the global one-approval requirement and the existing required `test success` check. Enable both **Dismiss stale pull request approvals when new commits are pushed** and - **Require approval of the most recent reviewable push**. Require one approval - from the human team on the ordered patterns printed by: - - ```sh - python3 .github/review-policy/adapter.py --patterns - ``` - - The patterns include the numbered-fragment exception; the adapter additionally - enforces PR ownership, addition-only status, and the release-directive exclusion. - Add them as one **Required reviewers** entry. In the REST ruleset schema this - is `required_reviewers[{file_patterns, minimum_approvals: 1, - reviewer: {id: TEAM_ID, type: "Team"}}]`. Keep the ruleset active with no bypass - actors. Human team approval can satisfy the global requirement too; the App - cannot satisfy the human team requirement for release or application changes. + **Require approval of the most recent reviewable push**. Keep the ruleset + active with no bypass actors. Preserve existing reviewer and code-owner + requirements; do not add a dedicated team or path-based reviewer rule for + this adapter. Release and other excluded changes get no adapter approval and + follow the normal approval process. 6. Run the validation below with disposable PRs before enabling normal use. Finally set `CODEX_APPROVAL_ENABLED=true`. -The writer independently rereads the ruleset and checks its patterns, team, +The writer independently rereads the ruleset and checks its approval requirement, freshness controls, 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. @@ -144,7 +140,7 @@ 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 protected paths still require a human team member. +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. @@ -166,10 +162,10 @@ 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 and human path rules so ordinary human review keeps working. +approval requirement so ordinary human review keeps working. ## References - [Native Codex GitHub reviews](https://learn.chatgpt.com/docs/third-party/github) -- [GitHub required reviewers and freshness rules](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/available-rules-for-rulesets) +- [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) diff --git a/.github/review-policy/adapter.py b/.github/review-policy/adapter.py index 0df24a4565..eca05280c4 100644 --- a/.github/review-policy/adapter.py +++ b/.github/review-policy/adapter.py @@ -74,16 +74,6 @@ def load(cls): def digest(self): return hashlib.sha256(json.dumps(self.data, sort_keys=True).encode()).hexdigest() - @property - def human_patterns(self): - # GitHub required-reviewer patterns are ordered gitignore patterns. - return ["*"] + [f"!/{root}**" for root in self.data["eligible_roots"]] + [ - f"!/{self.data['changelog_fragment_root']}[0-9]*.md" - ] + [ - f"/{path}{'**' if path.endswith('/') else ''}" - for path in self.data["human_only"] - ] - def eligible_path(self, path): # Reject paths whose representation could disagree with GitHub matching. if (not isinstance(path, str) or not path or "\\" in path @@ -262,8 +252,8 @@ def check_evidence(policy, pull, comments, summary, reactions, reviews, threads, "policy": policy.digest} -def check_rules(api, policy, team_id): - require(team_id > 0, "Human reviewer team is not configured") +def check_rules(api, policy): + """Check approval freshness without changing the repository's reviewer policy.""" repo = policy.data["repository"] branch = urllib.parse.quote(policy.data["base_branch"], safe="") rules = api.request(f"/repos/{repo}/rules/branches/{branch}") @@ -279,25 +269,20 @@ def check_rules(api, policy, team_id): and parameters.get("dismiss_stale_reviews_on_push") and parameters.get("require_last_push_approval")): continue - matching = any(r.get("file_patterns") == policy.human_patterns - and r.get("minimum_approvals", 0) >= 1 - and r.get("reviewer") == {"id": team_id, "type": "Team"} - for r in parameters.get("required_reviewers", [])) - if matching: - require(rule.get("ruleset_source_type") == "Repository", - "Expected a repository ruleset") - full = api.request(f"/repos/{repo}/rulesets/{int(rule['ruleset_id'])}") - require(full.get("enforcement") == "active" and not full.get("bypass_actors"), - "Review ruleset must be active without bypass actors") - return - raise Ineligible("Required human-path and stale-review protections are not active") + require(rule.get("ruleset_source_type") == "Repository", + "Expected a repository ruleset") + full = api.request(f"/repos/{repo}/rulesets/{int(rule['ruleset_id'])}") + require(full.get("enforcement") == "active" and not full.get("bypass_actors"), + "Review ruleset must be active without bypass actors") + return + raise Ineligible("Required approval and stale-review protections are not active") class Adapter: - def __init__(self, api, policy, number, team_id=0, writer=None, app_id=0, bot_id=0, + def __init__(self, api, policy, number, writer=None, app_id=0, bot_id=0, trusted_sha=None): self.api, self.policy, self.number = api, policy, number - self.team_id, self.writer = team_id, writer + self.writer = writer self.app_id, self.bot_id, self.trusted_sha = app_id, bot_id, trusted_sha self.repo = policy.data["repository"] self.prefix = f"/repos/{self.repo}" @@ -385,7 +370,7 @@ def evaluate(self, enforce_rules=True): if fragment: self.check_fragment(fragment, pull["head"]["sha"]) if enforce_rules: - check_rules(self.api, self.policy, self.team_id) + check_rules(self.api, self.policy) receipt = self.evidence(pull) again = self.api.request(self.pull_path) require(pull["head"]["sha"] == again["head"]["sha"] @@ -461,7 +446,7 @@ def reconcile(self): 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']}). Required CI and human path rules still apply.") + + f"#issuecomment-{receipt['summary']}). Existing merge requirements still apply.") created = None try: created = self.writer.request(self.pull_path + "/reviews", "POST", { @@ -504,14 +489,10 @@ def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--pr", type=int) parser.add_argument("--apply", action="store_true") - parser.add_argument("--patterns", action="store_true", help="Print required human reviewer patterns") 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() - if args.patterns: - print(json.dumps(policy.human_patterns, indent=2)) - return 0 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"]) @@ -535,7 +516,6 @@ def main(): results = [] for number in numbers: adapter = Adapter(api, policy, number, - team_id=int(os.environ.get("CODEX_APPROVAL_HUMAN_TEAM_ID") or 0), 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")) diff --git a/.github/review-policy/test_adapter.py b/.github/review-policy/test_adapter.py index ec6e19b0e7..ab3d1a887e 100644 --- a/.github/review-policy/test_adapter.py +++ b/.github/review-policy/test_adapter.py @@ -393,10 +393,7 @@ def test_evaluation_checks_fragment_before_native_approval_evidence(self): 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": True, "require_last_push_approval": True, - "required_reviewers": [{"file_patterns": POLICY.human_patterns, - "minimum_approvals": 1, - "reviewer": {"id": 42, "type": "Team"}}]}}, + "dismiss_stale_reviews_on_push": True, "require_last_push_approval": True}}, {"type": "required_status_checks", "parameters": {"required_status_checks": [{"context": "test success"}]}}] @@ -408,14 +405,13 @@ def setUp(self): self.api = Mock() self.api.request.side_effect = lambda path: self.full if "/rulesets/" in path else self.rules - def test_native_human_paths_and_freshness_are_required(self): - adapter.check_rules(self.api, POLICY, 42) + def test_normal_approval_rules_work_without_a_reviewer_team(self): + adapter.check_rules(self.api, POLICY) def test_current_live_rule_configuration_cannot_enable_adapter(self): - self.rules[0]["parameters"]["required_reviewers"] = [] self.rules[0]["parameters"]["dismiss_stale_reviews_on_push"] = False with self.assertRaises(adapter.Ineligible): - adapter.check_rules(self.api, POLICY, 42) + adapter.check_rules(self.api, POLICY) def test_each_freshness_switch_is_required(self): for key in ("dismiss_stale_reviews_on_push", "require_last_push_approval"): @@ -423,17 +419,23 @@ def test_each_freshness_switch_is_required(self): self.rules = rules_fixture() self.rules[0]["parameters"][key] = False with self.assertRaises(adapter.Ineligible): - adapter.check_rules(self.api, POLICY, 42) - - def test_human_patterns_must_match_including_order_and_exceptions(self): - self.rules[0]["parameters"]["required_reviewers"][0]["file_patterns"] = ["*"] - with self.assertRaises(adapter.Ineligible): - adapter.check_rules(self.api, POLICY, 42) - - def test_missing_team_test_gate_or_bypass_stops_approval(self): - for change in ("team", "ci", "bypass", "evaluate"): + adapter.check_rules(self.api, POLICY) + + 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": @@ -441,7 +443,7 @@ def test_missing_team_test_gate_or_bypass_stops_approval(self): if change == "evaluate": self.full["enforcement"] = "evaluate" with self.assertRaises(adapter.Ineligible): - adapter.check_rules(self.api, POLICY, 0 if change == "team" else 42) + adapter.check_rules(self.api, POLICY) def receipt(): @@ -459,7 +461,7 @@ 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, team_id=42, writer=self.writer, + 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()) @@ -481,7 +483,9 @@ def test_existing_current_approval_is_idempotent(self): 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)] + 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"]) diff --git a/.github/workflows/codex-approval-adapter.yml b/.github/workflows/codex-approval-adapter.yml index 265743c433..8d122bd193 100644 --- a/.github/workflows/codex-approval-adapter.yml +++ b/.github/workflows/codex-approval-adapter.yml @@ -57,8 +57,8 @@ jobs: vars.CODEX_APPROVAL_ENABLED == 'true' runs-on: ubuntu-latest timeout-minutes: 20 - # Restrict this environment to main. The App must not be a member of the - # required human team or have a ruleset bypass or administration permission. + # Restrict this environment to main. The App must not have a ruleset bypass + # or administration permission. environment: codex-approval steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 @@ -83,5 +83,4 @@ jobs: 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 }} - CODEX_APPROVAL_HUMAN_TEAM_ID: ${{ vars.CODEX_APPROVAL_HUMAN_TEAM_ID }} run: TRUSTED_SHA="$(git rev-parse HEAD)" python3 .github/review-policy/adapter.py --apply --event "$GITHUB_EVENT_PATH" --wait-seconds 10 From 3f5bf580666a517f6e97fd82670fc2f0b3a70cbc Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 7 Sep 2026 21:00:15 -0600 Subject: [PATCH 04/11] ci: preserve existing approval freshness settings --- .github/review-policy/README.md | 36 ++++++++++++++------------- .github/review-policy/adapter.py | 8 +++--- .github/review-policy/test_adapter.py | 25 +++++++++++-------- 3 files changed, 36 insertions(+), 33 deletions(-) diff --git a/.github/review-policy/README.md b/.github/review-policy/README.md index 0e7cf58273..a4d57ab0fa 100644 --- a/.github/review-policy/README.md +++ b/.github/review-policy/README.md @@ -110,19 +110,17 @@ Keep `CODEX_APPROVAL_ENABLED` unset or `false` until setup and validation finish | `CODEX_APPROVAL_APP_ID` | Its numeric App ID | | `CODEX_APPROVAL_BOT_ID` | Numeric ID of its `[bot]` account | -5. Update the active `main` repository ruleset. Retain the global one-approval - requirement and the existing required `test success` check. Enable both - **Dismiss stale pull request approvals when new commits are pushed** and - **Require approval of the most recent reviewable push**. Keep the ruleset - active with no bypass actors. Preserve existing reviewer and code-owner - requirements; do not add a dedicated team or path-based reviewer rule for - this adapter. Release and other excluded changes get no adapter approval and - follow the normal approval process. +5. Leave the current `main` ruleset unchanged: one required approval, the required + `test success` check, and no bypass actors. The adapter does not require + dismissing stale approvals or approval of the most recent reviewable push. + Preserve existing reviewer and code-owner requirements; no dedicated team or + path-based reviewer rule is needed. Release and other excluded changes get + no adapter approval and follow the normal approval process. 6. Run the validation below with disposable PRs before enabling normal use. Finally set `CODEX_APPROVAL_ENABLED=true`. -The writer independently rereads the ruleset and checks its approval requirement, -freshness controls, and required test check before every approval. Missing or +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 @@ -144,13 +142,17 @@ 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. -Specifically delay an approval request until after a new commit is pushed and -confirm GitHub will not permit merging based on that old-commit review. The -review API has no atomic expected-current-head precondition. The adapter submits -the full reviewed `commit_id`, rereads state before and after approval, and -withdraws its approval when those reads disagree; GitHub's native freshness -rules must enforce the merge boundary. Mocked tests cannot establish that server -behavior. Do not activate this adapter if that disposable-PR check fails. +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. diff --git a/.github/review-policy/adapter.py b/.github/review-policy/adapter.py index eca05280c4..a891bccf3a 100644 --- a/.github/review-policy/adapter.py +++ b/.github/review-policy/adapter.py @@ -253,7 +253,7 @@ def check_evidence(policy, pull, comments, summary, reactions, reviews, threads, def check_rules(api, policy): - """Check approval freshness without changing the repository's reviewer policy.""" + """Verify existing approval and CI requirements without changing review policy.""" repo = policy.data["repository"] branch = urllib.parse.quote(policy.data["base_branch"], safe="") rules = api.request(f"/repos/{repo}/rules/branches/{branch}") @@ -265,9 +265,7 @@ def check_rules(api, policy): if rule["type"] != "pull_request": continue parameters = rule["parameters"] - if not (parameters.get("required_approving_review_count", 0) >= 1 - and parameters.get("dismiss_stale_reviews_on_push") - and parameters.get("require_last_push_approval")): + if parameters.get("required_approving_review_count", 0) < 1: continue require(rule.get("ruleset_source_type") == "Repository", "Expected a repository ruleset") @@ -275,7 +273,7 @@ def check_rules(api, policy): require(full.get("enforcement") == "active" and not full.get("bypass_actors"), "Review ruleset must be active without bypass actors") return - raise Ineligible("Required approval and stale-review protections are not active") + raise Ineligible("The required approval rule is not active") class Adapter: diff --git a/.github/review-policy/test_adapter.py b/.github/review-policy/test_adapter.py index ab3d1a887e..a3f628c1dd 100644 --- a/.github/review-policy/test_adapter.py +++ b/.github/review-policy/test_adapter.py @@ -393,7 +393,7 @@ def test_evaluation_checks_fragment_before_native_approval_evidence(self): 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": True, "require_last_push_approval": True}}, + "dismiss_stale_reviews_on_push": False, "require_last_push_approval": False}}, {"type": "required_status_checks", "parameters": {"required_status_checks": [{"context": "test success"}]}}] @@ -408,18 +408,21 @@ def setUp(self): def test_normal_approval_rules_work_without_a_reviewer_team(self): adapter.check_rules(self.api, POLICY) - def test_current_live_rule_configuration_cannot_enable_adapter(self): - self.rules[0]["parameters"]["dismiss_stale_reviews_on_push"] = False - with self.assertRaises(adapter.Ineligible): - 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_each_freshness_switch_is_required(self): - for key in ("dismiss_stale_reviews_on_push", "require_last_push_approval"): - with self.subTest(key=key): - self.rules = rules_fixture() - self.rules[0]["parameters"][key] = False - with self.assertRaises(adapter.Ineligible): + 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"] = [ From 6a8efc2ef7e9269a47abb6ecf7d3c39579ee024a Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 7 Sep 2026 21:05:24 -0600 Subject: [PATCH 05/11] ci: use repository secret for approval App key --- .github/review-policy/README.md | 48 +++++++++++--------- .github/workflows/codex-approval-adapter.yml | 3 -- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/.github/review-policy/README.md b/.github/review-policy/README.md index a4d57ab0fa..df7e8c5332 100644 --- a/.github/review-policy/README.md +++ b/.github/review-policy/README.md @@ -90,19 +90,18 @@ Codex for another review. ## Administrator setup -Keep `CODEX_APPROVAL_ENABLED` unset or `false` until setup and validation finish. - -1. Merge this PR through human review and inspect read-only audit results. -2. Create a dedicated GitHub App and install it only on `zakura-core/zakura`. - Grant repository **Pull requests: read and write** and the mandatory metadata - access. It needs no webhook, contents write, administration, Actions write, or - ruleset bypass. Do not reuse a release App or a person's token. -3. Create the `codex-approval` environment, restricted to the `main` branch. - Keep the App key in Infisical with a dedicated service scope, and sync it to - the environment secret `CODEX_APPROVAL_APP_PRIVATE_KEY`. Required environment - reviewers would make each adapter execution manual, so leave them unset for - normal automatic operation. -4. Set these repository variables from the App metadata: +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. +3. On the **Variables** tab, add these repository variables: | Variable | Value | | --- | --- | @@ -110,14 +109,21 @@ Keep `CODEX_APPROVAL_ENABLED` unset or `false` until setup and validation finish | `CODEX_APPROVAL_APP_ID` | Its numeric App ID | | `CODEX_APPROVAL_BOT_ID` | Numeric ID of its `[bot]` account | -5. Leave the current `main` ruleset unchanged: one required approval, the required - `test success` check, and no bypass actors. The adapter does not require - dismissing stale approvals or approval of the most recent reviewable push. - Preserve existing reviewer and code-owner requirements; no dedicated team or - path-based reviewer rule is needed. Release and other excluded changes get - no adapter approval and follow the normal approval process. -6. Run the validation below with disposable PRs before enabling normal use. - Finally set `CODEX_APPROVAL_ENABLED=true`. + 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 diff --git a/.github/workflows/codex-approval-adapter.yml b/.github/workflows/codex-approval-adapter.yml index 8d122bd193..929cf14208 100644 --- a/.github/workflows/codex-approval-adapter.yml +++ b/.github/workflows/codex-approval-adapter.yml @@ -57,9 +57,6 @@ jobs: vars.CODEX_APPROVAL_ENABLED == 'true' runs-on: ubuntu-latest timeout-minutes: 20 - # Restrict this environment to main. The App must not have a ruleset bypass - # or administration permission. - environment: codex-approval steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 with: From 1f2b47f1df7b29740022fb7e2e1c0dd1f4c442f4 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 7 Sep 2026 22:00:12 -0600 Subject: [PATCH 06/11] fix(ci): require changelog fragments before Codex approval --- .github/review-policy/README.md | 2 ++ .github/review-policy/adapter.py | 4 ++++ .github/review-policy/test_adapter.py | 10 ++++++++++ 3 files changed, 16 insertions(+) diff --git a/.github/review-policy/README.md b/.github/review-policy/README.md index df7e8c5332..51e6c9278b 100644 --- a/.github/review-policy/README.md +++ b/.github/review-policy/README.md @@ -49,6 +49,8 @@ There is one addition exception: an otherwise eligible PR may add its own 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 diff --git a/.github/review-policy/adapter.py b/.github/review-policy/adapter.py index a891bccf3a..4f693c4c33 100644 --- a/.github/review-policy/adapter.py +++ b/.github/review-policy/adapter.py @@ -109,6 +109,10 @@ def check_files(self, files, expected_count, pr_number=None): require(file["status"] != "renamed", "Renamed files require human classification") require(len(files) > int(fragment is not None), "A changelog fragment must accompany an eligible CI or deployment change") + require(fragment is not None or not any( + f["filename"].endswith(".rs") or f["filename"].split("/")[-1] == "Cargo.toml" + for f in files + ), "Rust and Cargo.toml changes require this PR's changelog fragment") return fragment def native(self, obj): diff --git a/.github/review-policy/test_adapter.py b/.github/review-policy/test_adapter.py index a3f628c1dd..3a1d903480 100644 --- a/.github/review-policy/test_adapter.py +++ b/.github/review-policy/test_adapter.py @@ -226,6 +226,16 @@ def test_incomplete_threads_fail_closed(self): 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"}] From 3749820bfcf48bd6f2f16091411e4f102ff562bf Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Mon, 7 Sep 2026 22:01:58 -0600 Subject: [PATCH 07/11] fix(ci): restore approvals after automatic withdrawals --- .github/review-policy/README.md | 3 ++ .github/review-policy/adapter.py | 29 +++++++++++++- .github/review-policy/test_adapter.py | 54 +++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/.github/review-policy/README.md b/.github/review-policy/README.md index 51e6c9278b..ab09ec3176 100644 --- a/.github/review-policy/README.md +++ b/.github/review-policy/README.md @@ -164,6 +164,9 @@ 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 diff --git a/.github/review-policy/adapter.py b/.github/review-policy/adapter.py index 4f693c4c33..a57f03ca3e 100644 --- a/.github/review-policy/adapter.py +++ b/.github/review-policy/adapter.py @@ -28,6 +28,8 @@ POLICY_PATH = Path(__file__).with_name("policy.json") SUMMARY_MARKER = "" RECEIPT_MARKER = "" @@ -406,7 +408,8 @@ def author_gate(self): return {"reconcile": True, "reason": "PR author has write-level access"} except (Ineligible, APIError, KeyError, TypeError) as exc: cleanup = self.bot_id > 0 and any( - r["state"] == "APPROVED" for r in self.owned_reviews()) + r["state"] == "APPROVED" or slack_notify.needs_notification(r) + for r in self.owned_reviews()) return {"reconcile": bool(cleanup), "reason": str(exc)} def evaluate(self, enforce_rules=True): @@ -615,6 +618,15 @@ def main(): 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"): @@ -624,7 +636,7 @@ def main(): 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 for r in results)) + return int(any("error" in r or "notification_error" in r for r in results)) if __name__ == "__main__": diff --git a/.github/review-policy/policy.json b/.github/review-policy/policy.json index 0ca3f8ca44..0a6ba8d63c 100644 --- a/.github/review-policy/policy.json +++ b/.github/review-policy/policy.json @@ -1,6 +1,7 @@ { "repository": "zakura-core/zakura", "base_branch": "main", + "slack_channel": "C0BCQ7PP32A", "changelog_fragment_root": "docs/changelog/unreleased/", "eligible_roots": [ "deploy/", diff --git a/.github/review-policy/slack_notify.py b/.github/review-policy/slack_notify.py new file mode 100644 index 0000000000..86e7069acb --- /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 index 82377894e5..dd6d6b8879 100644 --- a/.github/review-policy/test_adapter.py +++ b/.github/review-policy/test_adapter.py @@ -12,6 +12,7 @@ from unittest.mock import Mock, patch import adapter +import slack_notify HEAD = "a" * 40 @@ -852,6 +853,198 @@ def test_closed_draft_or_retargeted_pr_is_ineligible(self): 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( @@ -906,6 +1099,26 @@ def test_retargeted_pr_is_checked_by_event_and_periodic_cleanup(self): 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")): diff --git a/.github/workflows/codex-approval-adapter.yml b/.github/workflows/codex-approval-adapter.yml index 352ee86d12..4ef4ebccc7 100644 --- a/.github/workflows/codex-approval-adapter.yml +++ b/.github/workflows/codex-approval-adapter.yml @@ -84,6 +84,7 @@ jobs: 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 }}