From 100e769dd0be0ed6fed62e6a715bb76412ebf21b Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 17:44:07 -0400 Subject: [PATCH 1/9] feat: assert that every target repository will actually start a review `on.workflow_run.workflows` lists workflow `name:` values, and GitHub matches them silently. A name that matches no workflow never fires: no warning, no failed run, no log line. Reviews stop and the repository still looks green, because every other check passes. Two live instances of this, both found 2026-09-22: * Five SDK callers listed `workflows: ["Tests", "Lint"]` into repositories with no workflow named `Lint`. Reviews ran via `Tests`, so the dead half survived unnoticed from installation until now. * sdk-js's pull-request workflow was once named `CI` and sdk-java's `Pull Request`; both are `Tests` today. Renaming either back would switch that repository's reviews off in silence. `reviewbot audit-callers ...` answers the question and EXITS NON-ZERO when the answer is no. It reports a missing or commented-out `workflow_run`, `types` without `completed`, an empty list, a name matching no workflow, a workflow that does not run on pull requests, and a repository it could not read. Unreadable counts as broken: a deleted caller answers 404, and an audit that shrugged at that would report the broken case as healthy. `reviewbot/callers.py` is pure -- caller text and workflow files in, verdict out -- so the default test run keeps its promise of no token and no socket. `.github/workflows/audit-callers.yml` runs it daily over both installations, one step per owner because an org installation token cannot read the user account's repositories, and `if: always()` on the second so one bad repository cannot hide the rest. The target list is explicit rather than discovered from the App installation. Discovery answers "which repositories have a caller?", so one that LOST its caller would drop quietly out of the audit -- the same silent failure moved somewhere else. An explicit list answers "which repositories are supposed to be reviewed?", and removing one is a diff a person approves. Tests assert the list covers every SDK repository, holds no duplicates and omits the bot's own repository, which reviews itself through self-review.yml. Verified against the live fleet: all 7 report `will activate`, and a run naming sdk-go-v2 (no caller) and a nonexistent repository exits 1. --- .github/workflows/audit-callers.yml | 104 +++++++++ README.md | 21 ++ reviewbot/callers.py | 149 +++++++++++++ reviewbot/cli.py | 50 +++++ reviewbot/github.py | 21 ++ tests/test_callers.py | 325 ++++++++++++++++++++++++++++ 6 files changed, 670 insertions(+) create mode 100644 .github/workflows/audit-callers.yml create mode 100644 reviewbot/callers.py create mode 100644 tests/test_callers.py diff --git a/.github/workflows/audit-callers.yml b/.github/workflows/audit-callers.yml new file mode 100644 index 0000000..9212623 --- /dev/null +++ b/.github/workflows/audit-callers.yml @@ -0,0 +1,104 @@ +name: Audit callers + +# DOES EVERY TARGET REPOSITORY STILL START A REVIEW? +# +# `on.workflow_run.workflows` in a caller lists workflow `name:` values, and +# GitHub matches them silently. A name that matches no workflow never fires: +# no warning, no failed run, no log line. Reviews just stop, and the +# repository looks healthy because every other check is green. +# +# That is not hypothetical. Both of these were live on 2026-09-22: +# +# * Five SDK callers listed `workflows: ["Tests", "Lint"]` into repositories +# with no workflow named `Lint`. Reviews still ran via `Tests`, so nothing +# ever surfaced the dead half. +# * sdk-js's pull-request workflow was once named `CI`, sdk-java's +# `Pull Request`. Both are `Tests` today. Renaming either one back would +# switch that repository's reviews off in silence. +# +# So this asks the question on a schedule and goes RED when the answer is no. +# A warning printed into a green run would reproduce the bug it is checking. +on: + schedule: + # 07:00 UTC daily, before the working day in every timezone the team uses. + - cron: "0 7 * * *" + workflow_dispatch: + # Also on a change to the bot's own workflow templates or to the auditor, so + # a caller template edited here is checked against reality before it ships. + pull_request: + paths: + - ".github/workflows/audit-callers.yml" + - "reviewbot/callers.py" + - "reviewbot/cli.py" + +# THE TARGET LIST IS EXPLICIT, not discovered from the App installation. +# +# Discovery would answer "which repositories have a caller?", and a repository +# that LOST its caller would then drop quietly out of the audit -- the same +# silent failure in a different place. An explicit list answers "which +# repositories are supposed to be reviewed?", and removing one is a diff a +# person has to approve. +# +# code-review-bot is absent on purpose: it reviews itself through +# self-review.yml, which calls review.yml directly and has no code-review.yml. +env: + ORG_REPOS: >- + MarketData-App/api + USER_REPOS: >- + MarketDataApp/sdk-py + MarketDataApp/sdk-go + MarketDataApp/sdk-js + MarketDataApp/sdk-java + MarketDataApp/sdk-csharp + MarketDataApp/sdk-php + +jobs: + audit: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + persist-credentials: false + + - uses: astral-sh/setup-uv@v7 + with: + enable-cache: true + + # TWO TOKENS, because the App is installed on two accounts and an + # installation token reaches only its own. `owner:` with no + # `repositories:` gives every repository in that installation. + - name: Token for the MarketData-App organisation + id: org + uses: actions/create-github-app-token@v3 + with: + app-id: ${{ secrets.CODE_REVIEW_APP_ID }} + private-key: ${{ secrets.CODE_REVIEW_APP_PRIVATE_KEY }} + owner: MarketData-App + + - name: Token for the MarketDataApp user account + id: user + uses: actions/create-github-app-token@v3 + with: + app-id: ${{ secrets.CODE_REVIEW_APP_ID }} + private-key: ${{ secrets.CODE_REVIEW_APP_PRIVATE_KEY }} + owner: MarketDataApp + + # One step per owner, because each needs ITS OWN token in the + # environment; an organisation installation token cannot read the user + # account's repositories. + # + # `if: always()` on the second, so a failure in the first does not hide + # the rest of the fleet. Reporting one broken repository while silently + # skipping five others is the failure this workflow exists to prevent. + - name: Audit the MarketData-App organisation + id: audit-org + run: uv run reviewbot audit-callers $ORG_REPOS + env: + GITHUB_TOKEN: ${{ steps.org.outputs.token }} + + - name: Audit the MarketDataApp user account + id: audit-user + if: always() + run: uv run reviewbot audit-callers $USER_REPOS + env: + GITHUB_TOKEN: ${{ steps.user.outputs.token }} diff --git a/README.md b/README.md index e62f1f6..ae0a783 100644 --- a/README.md +++ b/README.md @@ -129,6 +129,27 @@ The middle row is the common one and reads as a fault in the footer when it is not: on a repository with `backends: [codex, claude]` and `mode: fallback`, every successful review reports `claude unavailable` because Codex answered. +### Will a repository actually start a review? + +`on.workflow_run.workflows` in a caller lists workflow **`name:`** values, and +GitHub matches them silently. A name matching no workflow never fires: no +warning, no failed run, no log line. Reviews stop and the repository still +looks green. + +```bash +GITHUB_TOKEN=$(gh auth token) uv run reviewbot audit-callers MarketDataApp/sdk-py ... +``` + +It exits non-zero when any repository will not start a review, and says why: +a missing or commented-out `workflow_run`, a name matching no workflow, a +workflow that does not run on pull requests, or a repository it could not +read. `.github/workflows/audit-callers.yml` runs it daily over the whole +fleet. + +The target list there is explicit, not discovered. Discovery answers "which +repositories have a caller?", so one that LOST its caller would drop quietly +out of the audit -- the same silent failure in a different place. + ## Setting it up The GitHub App is created by hand, once. `docs/setup.md` has the exact diff --git a/reviewbot/callers.py b/reviewbot/callers.py new file mode 100644 index 0000000..5208e3c --- /dev/null +++ b/reviewbot/callers.py @@ -0,0 +1,149 @@ +"""Will a target repository's caller actually start a review? + +The bot is installed by copying `code-review.yml` into a target repository. +Whether it ever RUNS depends on a string match GitHub performs silently: +`on.workflow_run.workflows` lists workflow `name:` values, and a name that +matches no workflow in that repository never fires. There is no warning, no +failed run and no log line -- the review simply never happens, and the +repository looks healthy because every other check is green. + +Two live examples, both found 2026-09-22: + + * Five SDK callers listed `workflows: ["Tests", "Lint"]`. None of those + repositories has a workflow named `Lint`. Reviews still ran, via `Tests`, + so the dead entry survived unnoticed. + * sdk-js's pull-request workflow was once named `CI` and sdk-java's + `Pull Request`. Both are `Tests` today. A rename back would switch reviews + off in silence, and the only symptom would be reviews quietly stopping. + +This module is PURE. Text in, verdict out, no network -- so the default test +run keeps its promise of no token and no socket. `reviewbot audit-callers` +does the fetching and calls `activation` per repository. +""" + +import dataclasses + +import yaml + + +@dataclasses.dataclass(frozen=True) +class Activation: + """Whether a caller will start a review, and why not when it will not.""" + + ok: bool + matched: list[str] + reasons: list[str] + + +def triggers(spec: dict) -> dict: + """The `on:` block, whichever key YAML produced for it. + + `on` is a YAML 1.1 boolean, so PyYAML returns the key `True` rather than + the string `"on"`. Reading only `"on"` reported every workflow as having + no triggers at all, which would have made this auditor claim the whole + fleet was broken. + """ + if not isinstance(spec, dict): + return {} + found = spec.get(True, spec.get("on")) + return found if isinstance(found, dict) else {} + + +def runs_on_pull_request(on) -> bool: + """Does this `on:` block run the workflow for a pull request? + + Accepts all three shapes GitHub allows: a bare string, a list, and a map. + """ + if isinstance(on, str): + return on == "pull_request" + if isinstance(on, list): + return "pull_request" in on + if isinstance(on, dict): + return "pull_request" in on + return False + + +def _pull_request_workflow_names(files: dict) -> set[str]: + """The `name:` of every workflow in the repository that runs on a pull request. + + A file that will not parse is SKIPPED rather than fatal: one broken + workflow elsewhere in `.github/workflows` must not make a healthy caller + report as broken. + """ + names = set() + for text in (files or {}).values(): + try: + spec = yaml.safe_load(text or "") + except yaml.YAMLError: + continue + if not isinstance(spec, dict): + continue + on = spec.get(True, spec.get("on")) + if runs_on_pull_request(on) and isinstance(spec.get("name"), str): + names.add(spec["name"]) + return names + + +def activation(caller_text: str | None, workflow_files: dict) -> Activation: + """Will this caller start a review when a pull request's CI finishes? + + `workflow_files` maps filename to text for everything in the target's + `.github/workflows`, the caller included. + """ + if caller_text is None: + return Activation(False, [], ["no code-review.yml in .github/workflows"]) + try: + caller = yaml.safe_load(caller_text) + except yaml.YAMLError as exc: + return Activation(False, [], [f"code-review.yml could not be read: {exc}"]) + + on = triggers(caller) + run_on = on.get("workflow_run") + if not isinstance(run_on, dict): + # How all five SDK callers shipped: the block present but commented, + # so the file parses cleanly and starts nothing. + return Activation(False, [], ["no workflow_run trigger; a push starts no review"]) + + reasons: list[str] = [] + types = run_on.get("types") or [] + if "completed" not in types: + # The review gate refuses a commit whose checks are still running, so + # anything but `completed` reviews a pull request that is not ready. + reasons.append(f"workflow_run.types is {types!r}; it must include 'completed'") + + wanted = run_on.get("workflows") or [] + if not wanted: + reasons.append("workflow_run.workflows is empty; it matches nothing") + + available = _pull_request_workflow_names(workflow_files) + matched = [] + for name in wanted: + if name in available: + matched.append(name) + else: + reasons.append( + f"{name!r} matches no workflow that runs on a pull request " + f"(available: {', '.join(sorted(available)) or 'none'})" + ) + + if not matched: + reasons.append("nothing will ever trigger a review") + return Activation(not reasons, matched, reasons) + + +def report(results: dict) -> str: + """One line per repository, then the detail for any that will not activate.""" + lines = [] + for repo in sorted(results): + got = results[repo] + if got.ok: + lines.append(f" {repo:<34} will activate on: {', '.join(got.matched)}") + else: + lines.append(f" {repo:<34} WILL NOT ACTIVATE") + broken = {r: g for r, g in results.items() if not g.ok} + if broken: + lines.append("") + for repo in sorted(broken): + lines.append(f"{repo}:") + lines += [f" - {reason}" for reason in broken[repo].reasons] + return "\n".join(lines) diff --git a/reviewbot/cli.py b/reviewbot/cli.py index a8a6f68..c1540a7 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -757,6 +757,45 @@ def credential_checkout( return 0 +def audit_callers(repos: list, token: str) -> int: + """Assert that every named repository will actually start a review. + + Exits NON-ZERO when any will not. That is the whole point: the failure + being guarded against is silent, so the guard must be loud. A warning + printed into a green run would reproduce the bug it is checking for. + + An unreadable repository fails too. "We could not look" is not "it is + fine" -- a deleted or renamed caller answers 404, and an audit that + shrugged at that would report the broken case as healthy. + """ + from reviewbot import callers as callers_mod + + results = {} + unreadable = {} + for repo in repos: + try: + files = _store_api(repo, token).workflow_files() + except Exception as exc: # noqa: BLE001 - any failure to read is a failure + unreadable[repo] = f"{type(exc).__name__}: {scrub(str(exc))}" + continue + results[repo] = callers_mod.activation(files.get("code-review.yml"), files) + + print(f"Code review activation, {len(repos)} repositor{'y' if len(repos) == 1 else 'ies'}\n") + if results: + print(callers_mod.report(results)) + for repo, why in sorted(unreadable.items()): + print(f" {repo:<34} COULD NOT BE READ: {why}") + + broken = [r for r, g in results.items() if not g.ok] + list(unreadable) + if broken: + print( + f"\n{len(broken)} of {len(repos)} will not start a review: {', '.join(sorted(broken))}" + ) + return 1 + print(f"\nAll {len(repos)} will start a review when a pull request's CI finishes.") + return 0 + + def _positive_days(text: str) -> int: """A window of zero or fewer days is refused rather than reported on. @@ -922,6 +961,15 @@ def main(argv: list[str] | None = None) -> int: checkin.add_argument("--holder", required=True, help="who took the lease") checkin.add_argument("--codex-home", required=True, help="the directory to remove") + audit = sub.add_parser( + "audit-callers", help="assert every target repository will actually start a review" + ) + audit.add_argument( + "repos", + nargs="+", + help="owner/name of each target repository to check", + ) + report = sub.add_parser("lease-report", help="who held the Codex credential, and for how long") report.add_argument( "--store", @@ -960,6 +1008,8 @@ def main(argv: list[str] | None = None) -> int: return credential_checkin(args.store, args.holder, args.codex_home, token) if args.command == "lease-report": return lease_report(args.store, args.days, token) + if args.command == "audit-callers": + return audit_callers(args.repos, token) if not args.repo: parser.error("GITHUB_REPOSITORY is not set and --repo was not given") diff --git a/reviewbot/github.py b/reviewbot/github.py index 9bd3169..ef67b1d 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -274,6 +274,27 @@ def commits(self, path: str, branch: str, since) -> list[dict]: f"?path={urllib.parse.quote(path)}&sha={urllib.parse.quote(branch)}&since={urllib.parse.quote(since.isoformat())}" ) + def workflow_files(self) -> dict: + """Every file in `.github/workflows`, as {filename: text}. + + Used by `audit-callers` to answer the question GitHub answers + silently: does the name in a caller's `workflow_run.workflows` match a + workflow that actually exists and actually runs on pull requests? + """ + import base64 + + listing = self._request( + "GET", f"/repos/{self.repo}/contents/{urllib.parse.quote('.github/workflows')}" + ).data + out = {} + for item in listing if isinstance(listing, list) else []: + if item.get("type") != "file" or not item.get("name", "").endswith((".yml", ".yaml")): + continue + blob = self._request("GET", f"/repos/{self.repo}/git/blobs/{item['sha']}").data or {} + if blob.get("encoding") == "base64": + out[item["name"]] = base64.b64decode(blob["content"]).decode("utf-8", "replace") + return out + def pull_for_sha(self, sha: str) -> int | None: """The open pull request whose head is `sha`, or None. diff --git a/tests/test_callers.py b/tests/test_callers.py new file mode 100644 index 0000000..23ad72b --- /dev/null +++ b/tests/test_callers.py @@ -0,0 +1,325 @@ +"""Will a target repository's caller actually start a review? + +The `workflow_run` trigger matches a workflow's `name:`, never its filename. +A name that matches no workflow does not warn, does not fail and does not log +-- it simply never fires. Two live examples, both found 2026-09-22: + + * Five SDK callers named `workflows: ["Tests", "Lint"]` while none of those + repositories has a workflow called `Lint`. + * sdk-js's pull-request workflow was once called `CI` and sdk-java's + `Pull Request`; both are `Tests` today. A rename back would switch + reviews off in silence. + +Everything here is pure: the caller's text and the repository's workflow files +go in, a verdict comes out. `reviewbot audit-callers` does the fetching. No +test in this file opens a socket. + +Run: pytest tests/test_callers.py +""" + +import pytest + +from reviewbot import callers + +ARMED = """ +name: Code review +on: + workflow_run: + workflows: ["Tests"] + types: [completed] + issue_comment: + types: [created] +jobs: + review: + uses: MarketData-App/code-review-bot/.github/workflows/review.yml@main +""" + +TESTS_WORKFLOW = """ +name: Tests +on: + pull_request: + branches: [main] +jobs: + test: + runs-on: ubuntu-latest +""" + + +def workflows(**files): + return dict(files) + + +# --- the happy path -------------------------------------------------------- + + +def test_a_caller_naming_a_real_pull_request_workflow_will_activate(): + got = callers.activation(ARMED, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is True + assert got.matched == ["Tests"] + assert got.reasons == [] + + +def test_the_filename_is_irrelevant_only_the_name_counts(): + # sdk-csharp's `Tests` lives in ci.yml, sdk-java's in pull-request.yml. + got = callers.activation(ARMED, workflows(**{"anything-at-all.yml": TESTS_WORKFLOW})) + assert got.ok is True + + +# --- the failures this exists to catch ------------------------------------- + + +def test_a_name_matching_no_workflow_is_reported(): + # THE SILENT RENAME. `Tests` became `CI` and nothing anywhere said so. + renamed = TESTS_WORKFLOW.replace("name: Tests", "name: CI") + got = callers.activation(ARMED, workflows(**{"test.yml": renamed})) + assert got.ok is False + assert any("Tests" in r and "no workflow" in r for r in got.reasons) + + +def test_a_dead_name_beside_a_live_one_is_still_reported(): + # The shipped template named ["Tests", "Lint"] into repositories with no + # `Lint`. Reviews still ran, so nothing ever surfaced the dead entry. + caller = ARMED.replace('["Tests"]', '["Tests", "Lint"]') + got = callers.activation(caller, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False + assert got.matched == ["Tests"] + assert any("Lint" in r for r in got.reasons) + + +def test_a_workflow_that_does_not_run_on_pull_requests_cannot_activate_a_review(): + # A `workflow_run` on a push-only workflow fires, and then the caller's own + # guard drops it because `workflow_run.event` is not `pull_request`. + push_only = TESTS_WORKFLOW.replace(" pull_request:\n branches: [main]", " push:") + got = callers.activation(ARMED, workflows(**{"test.yml": push_only})) + assert got.ok is False + assert any("pull request" in r for r in got.reasons) + + +def test_a_caller_with_the_trigger_commented_out_is_reported(): + # How all five SDK repositories shipped. + held_back = ( + ARMED.replace(" workflow_run:", " # workflow_run:") + .replace(' workflows: ["Tests"]', ' # workflows: ["Tests"]') + .replace(" types: [completed]", " # types: [completed]") + ) + got = callers.activation(held_back, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False + assert any("workflow_run" in r for r in got.reasons) + + +def test_a_trigger_that_does_not_listen_for_completion_is_reported(): + caller = ARMED.replace("types: [completed]", "types: [requested]") + got = callers.activation(caller, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False + assert any("completed" in r for r in got.reasons) + + +def test_an_empty_workflows_list_is_reported(): + caller = ARMED.replace('workflows: ["Tests"]', "workflows: []") + got = callers.activation(caller, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False + + +def test_a_missing_caller_is_reported_rather_than_raising(): + got = callers.activation(None, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False + assert any("no code-review.yml" in r for r in got.reasons) + + +def test_unparseable_yaml_is_reported_rather_than_raising(): + got = callers.activation("name: [unclosed\n", workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False + assert any("could not be read" in r for r in got.reasons) + + +def test_an_unparseable_workflow_file_does_not_hide_a_good_one(): + # One bad file in .github/workflows must not make a healthy caller fail. + got = callers.activation( + ARMED, workflows(**{"broken.yml": "name: [unclosed\n", "test.yml": TESTS_WORKFLOW}) + ) + assert got.ok is True + + +# --- `on:` comes back from YAML as the boolean True ------------------------ + + +def test_the_yaml_on_key_is_read_whether_it_parses_as_on_or_as_true(): + # `on:` is a YAML 1.1 boolean. PyYAML returns the key True, not "on", and + # reading only "on" made every caller look like it had no triggers at all. + assert callers.triggers({True: {"workflow_run": {}}}) == {"workflow_run": {}} + assert callers.triggers({"on": {"workflow_run": {}}}) == {"workflow_run": {}} + assert callers.triggers({}) == {} + + +@pytest.mark.parametrize( + "shape, expected", + [ + ("pull_request", True), + (["pull_request", "push"], True), + ({"pull_request": None}, True), + ({"pull_request": {"branches": ["main"]}}, True), + ({"push": {"branches": ["main"]}}, False), + ("push", False), + (None, False), + ], +) +def test_pull_request_triggers_are_recognised_in_every_yaml_shape(shape, expected): + assert callers.runs_on_pull_request(shape) is expected + + +# --- the report ------------------------------------------------------------ + + +def test_the_report_names_the_repository_and_the_failure(): + results = { + "MarketDataApp/sdk-py": callers.activation(ARMED, {"t.yml": TESTS_WORKFLOW}), + "MarketDataApp/sdk-go": callers.activation(ARMED, {"t.yml": "name: CI\non:\n push:\n"}), + } + text = callers.report(results) + assert "MarketDataApp/sdk-py" in text and "MarketDataApp/sdk-go" in text + assert "will activate" in text + assert "WILL NOT ACTIVATE" in text + + +def test_the_report_says_so_when_every_repository_is_healthy(): + results = {"o/r": callers.activation(ARMED, {"t.yml": TESTS_WORKFLOW})} + assert "WILL NOT ACTIVATE" not in callers.report(results) + + +# --- the command ----------------------------------------------------------- +# +# Still no socket: the GitHub client is replaced with a stub that answers from +# a dict. What is tested here is the exit code, because that is what a +# scheduled run acts on. + + +class FakeApi: + def __init__(self, repo, files): + self.repo = repo + self._files = files + + def workflow_files(self): + return self._files.get(self.repo, {}) + + +def _cli(monkeypatch, files): + from reviewbot import cli + + monkeypatch.setattr(cli, "_store_api", lambda repo, token: FakeApi(repo, files)) + monkeypatch.setenv("GITHUB_TOKEN", "t") + return cli + + +def test_the_command_exits_zero_when_every_repository_will_activate(monkeypatch, capsys): + cli = _cli(monkeypatch, {"o/a": {"code-review.yml": ARMED, "t.yml": TESTS_WORKFLOW}}) + assert cli.audit_callers(["o/a"], token="t") == 0 + assert "will activate" in capsys.readouterr().out + + +def test_the_command_exits_nonzero_when_a_repository_will_not_activate(monkeypatch, capsys): + # The whole point: a scheduled run must go RED, not print a warning into a + # log nobody reads. Silent breakage is the failure being guarded against. + cli = _cli( + monkeypatch, + { + "o/a": {"code-review.yml": ARMED, "t.yml": TESTS_WORKFLOW}, + "o/b": {"code-review.yml": ARMED, "t.yml": "name: CI\non:\n pull_request:\n"}, + }, + ) + assert cli.audit_callers(["o/a", "o/b"], token="t") == 1 + out = capsys.readouterr().out + assert "WILL NOT ACTIVATE" in out + assert "o/b" in out + + +def test_a_repository_that_cannot_be_read_fails_the_audit(monkeypatch, capsys): + from reviewbot import cli + from reviewbot.github import GitHubError + + class Boom: + def __init__(self, *a): + pass + + def workflow_files(self): + raise GitHubError("GET /contents failed with 404: Not Found") + + monkeypatch.setattr(cli, "_store_api", lambda repo, token: Boom()) + # Unreadable is not the same as healthy. An audit that shrugs at a 404 + # would report a deleted caller as fine. + assert cli.audit_callers(["o/gone"], token="t") == 1 + assert "404" in capsys.readouterr().out + + +# --- the scheduled workflow ------------------------------------------------ +# +# An audit that checks nothing passes. These guard the list itself. + + +@pytest.fixture +def audit_workflow(): + import pathlib + + import yaml as _yaml + + path = pathlib.Path(__file__).resolve().parents[1] / ".github/workflows/audit-callers.yml" + return _yaml.safe_load(path.read_text()) + + +def _audit_targets(spec): + env = spec["env"] + return env["ORG_REPOS"].split() + env["USER_REPOS"].split() + + +def test_the_audit_runs_on_a_schedule(audit_workflow): + # Manual-only would not catch a rename. The failure is silent, so the + # check has to be the thing that is not silent. + on = audit_workflow.get(True) or audit_workflow.get("on") + assert "schedule" in on + assert on["schedule"][0]["cron"] + + +def test_the_audit_covers_every_sdk_repository(audit_workflow): + targets = _audit_targets(audit_workflow) + for repo in ( + "MarketDataApp/sdk-py", + "MarketDataApp/sdk-go", + "MarketDataApp/sdk-js", + "MarketDataApp/sdk-java", + "MarketDataApp/sdk-csharp", + "MarketDataApp/sdk-php", + "MarketData-App/api", + ): + assert repo in targets, f"{repo} is not audited" + + +def test_every_audited_repository_is_named_owner_slash_name(audit_workflow): + for repo in _audit_targets(audit_workflow): + assert repo.count("/") == 1 and all(repo.split("/")), repo + + +def test_no_repository_is_audited_twice(audit_workflow): + targets = _audit_targets(audit_workflow) + assert len(targets) == len(set(targets)) + + +def test_the_bots_own_repository_is_not_audited(audit_workflow): + # It reviews itself through self-review.yml and has no code-review.yml, so + # auditing it would fail for a reason that is not a fault. + assert "MarketData-App/code-review-bot" not in _audit_targets(audit_workflow) + + +def test_both_owners_are_audited_with_their_own_token(audit_workflow): + # An organisation installation token cannot read the user account's + # repositories, so one step per owner is load-bearing rather than tidy. + steps = audit_workflow["jobs"]["audit"]["steps"] + audits = [s for s in steps if s.get("run", "").strip().startswith("uv run reviewbot audit")] + assert len(audits) == 2 + tokens = {s["env"]["GITHUB_TOKEN"] for s in audits} + assert len(tokens) == 2, "both audit steps use the same token" + + +def test_the_second_audit_runs_even_when_the_first_fails(audit_workflow): + # One broken repository must not hide the rest of the fleet. + steps = audit_workflow["jobs"]["audit"]["steps"] + audits = [s for s in steps if s.get("run", "").strip().startswith("uv run reviewbot audit")] + assert audits[1].get("if") == "always()" From be3ff89942de44b9d373e5379eec770165f6130f Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 17:50:28 -0400 Subject: [PATCH 2/9] fix: the audit workflow handed installation tokens to pull request code Three findings from the bot's review of this pull request. c12ba7a7 (blocking, security) The `pull_request` trigger I added was a privilege escalation, not a convenience. The job mints installation tokens covering EVERY repository of each owner and then runs `uv run reviewbot ...` from the checkout. On a pull_request trigger that checkout is the pull request's own code, so any author could edit cli.py and read the token out of the environment. review.yml refuses to let a pull request near a credential; this workflow now holds the same line. Schedule and manual dispatch only, both of which run trusted default-branch code. A test asserts the trigger set stays within {schedule, workflow_dispatch}. 2c4f6f10 (should fix) An omitted `types` was read as an empty list and reported as broken. GitHub fires a workflow_run for every activity type when `types` is absent, `completed` among them, so the auditor called a working caller broken -- a false positive in the tool built to catch false negatives. An explicit list that excludes `completed` is still rejected. c5b6e436 (should fix) `types` and `workflows` were consumed without checking their shape. `"completed" in {"completed": None}` is True and iterating a mapping yields its keys, so a schema-invalid caller GitHub will not run was reported healthy. `_as_names` now accepts a string or a list of strings and rejects everything else rather than coercing it. 107ba269 (nit) Not taken, for the same reason as 8ebd1bc4, ad392b16, 2f9d3e2b and 1c4d475a: dated measurements are this repository's house style. Live fleet re-checked after the shape changes: all 7 still report `will activate`. --- .github/workflows/audit-callers.yml | 20 +++++++---- reviewbot/callers.py | 52 ++++++++++++++++++++++----- tests/test_callers.py | 55 +++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 15 deletions(-) diff --git a/.github/workflows/audit-callers.yml b/.github/workflows/audit-callers.yml index 9212623..81b7d38 100644 --- a/.github/workflows/audit-callers.yml +++ b/.github/workflows/audit-callers.yml @@ -23,13 +23,19 @@ on: # 07:00 UTC daily, before the working day in every timezone the team uses. - cron: "0 7 * * *" workflow_dispatch: - # Also on a change to the bot's own workflow templates or to the auditor, so - # a caller template edited here is checked against reality before it ships. - pull_request: - paths: - - ".github/workflows/audit-callers.yml" - - "reviewbot/callers.py" - - "reviewbot/cli.py" + +# NO `pull_request` TRIGGER, AND THIS IS LOAD-BEARING. +# +# The job mints installation tokens covering every repository of each owner +# and then runs `uv run reviewbot ...` from the checkout. On a `pull_request` +# trigger that checkout is the PULL REQUEST'S OWN CODE, so any author could +# edit cli.py and read the token straight out of the environment -- a +# privilege escalation to every repository the App is installed on. +# +# An earlier version of this file had exactly that, to check a caller template +# against reality before it shipped. review.yml refuses to let a pull request +# near a credential, and this workflow holds the same line: schedule and +# manual dispatch only, both of which run trusted default-branch code. # THE TARGET LIST IS EXPLICIT, not discovered from the App installation. # diff --git a/reviewbot/callers.py b/reviewbot/callers.py index 5208e3c..b82e74a 100644 --- a/reviewbot/callers.py +++ b/reviewbot/callers.py @@ -84,6 +84,23 @@ def _pull_request_workflow_names(files: dict) -> set[str]: return names +def _as_names(value) -> list[str] | None: + """`value` as a list of strings, or None when it is not one. + + YAML accepts a bare scalar where GitHub documents a list, so a string is + read as a one-element list. Anything else -- a mapping above all -- is + rejected rather than coerced: membership and iteration both succeed on a + dict and silently answer about its KEYS. + """ + if value is None: + return None + if isinstance(value, str): + return [value] + if isinstance(value, list) and all(isinstance(item, str) for item in value): + return list(value) + return None + + def activation(caller_text: str | None, workflow_files: dict) -> Activation: """Will this caller start a review when a pull request's CI finishes? @@ -105,14 +122,33 @@ def activation(caller_text: str | None, workflow_files: dict) -> Activation: return Activation(False, [], ["no workflow_run trigger; a push starts no review"]) reasons: list[str] = [] - types = run_on.get("types") or [] - if "completed" not in types: - # The review gate refuses a commit whose checks are still running, so - # anything but `completed` reviews a pull request that is not ready. - reasons.append(f"workflow_run.types is {types!r}; it must include 'completed'") - - wanted = run_on.get("workflows") or [] - if not wanted: + + # ABSENT `types` IS FINE. GitHub fires a workflow_run for every activity + # type when `types` is omitted, `completed` among them. Treating a missing + # key as an empty list called a working caller broken -- a false positive + # in the very thing meant to catch false negatives. + types = run_on.get("types") + if types is not None: + listed = _as_names(types) + if listed is None: + reasons.append(f"workflow_run.types is {types!r}; it must be a list of strings") + elif "completed" not in listed: + # The review gate refuses a commit whose checks are still running, + # so anything but `completed` reviews a pull request that is not + # ready. + reasons.append(f"workflow_run.types is {types!r}; it must include 'completed'") + + # `_as_names` rather than truthiness: `"completed" in {"completed": None}` + # is True and iterating a mapping yields its keys, so a schema-invalid + # caller that GitHub will not run was being reported healthy. + wanted = _as_names(run_on.get("workflows")) + if wanted is None: + reasons.append( + f"workflow_run.workflows is {run_on.get('workflows')!r}; " + f"it must be a list of workflow names" + ) + wanted = [] + elif not wanted: reasons.append("workflow_run.workflows is empty; it matches nothing") available = _pull_request_workflow_names(workflow_files) diff --git a/tests/test_callers.py b/tests/test_callers.py index 23ad72b..79dd627 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -323,3 +323,58 @@ def test_the_second_audit_runs_even_when_the_first_fails(audit_workflow): steps = audit_workflow["jobs"]["audit"]["steps"] audits = [s for s in steps if s.get("run", "").strip().startswith("uv run reviewbot audit")] assert audits[1].get("if") == "always()" + + +# --- fixes from the bot's review of PR #21 --------------------------------- + + +def test_the_audit_never_runs_on_a_pull_request(audit_workflow): + # c12ba7a7, and it was a real hole rather than a theoretical one. The job + # mints installation tokens covering EVERY repository of each owner and + # then runs `uv run reviewbot ...` from the checkout. With a + # `pull_request` trigger that checkout is the PULL REQUEST'S OWN CODE, so + # any author could edit cli.py and read the token out of the environment. + # review.yml refuses to let a pull request near a credential for exactly + # this reason; this workflow must hold the same line. + on = audit_workflow.get(True) or audit_workflow.get("on") + assert "pull_request" not in on + assert "pull_request_target" not in on + assert set(on) <= {"schedule", "workflow_dispatch"} + + +def test_omitted_types_is_accepted(): + # 2c4f6f10. GitHub runs a workflow_run for every activity type when + # `types` is absent, `completed` included. Rejecting that called a working + # caller broken -- the audit's own false positive. + caller = ARMED.replace(" types: [completed]\n", "") + got = callers.activation(caller, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is True, got.reasons + + +def test_an_explicit_types_list_without_completed_is_still_rejected(): + caller = ARMED.replace("types: [completed]", "types: [requested]") + got = callers.activation(caller, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False + assert any("completed" in r for r in got.reasons) + + +@pytest.mark.parametrize( + "block", + [ + " workflows:\n Tests: null\n types: [completed]", # a map, not a list + " workflows: Tests\n types:\n completed: null", # types as a map + ], +) +def test_a_malformed_trigger_is_rejected_rather_than_passing_on_key_membership(block): + # c5b6e436. `"completed" in {"completed": None}` is True and iterating a + # mapping yields its keys, so a schema-invalid caller GitHub will not run + # was reported healthy. + caller = ARMED.replace(' workflows: ["Tests"]\n types: [completed]', block) + got = callers.activation(caller, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False + + +def test_a_non_string_workflow_name_is_rejected(): + caller = ARMED.replace('workflows: ["Tests"]', "workflows: [123]") + got = callers.activation(caller, workflows(**{"test.yml": TESTS_WORKFLOW})) + assert got.ok is False From 78b04ac1d59de69324c520d821e42c57a86919e5 Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 17:56:30 -0400 Subject: [PATCH 3/9] fix: the audit ignored workflow_run branch filters 2ef2f9cc from the bot's review of this pull request, and it is the same class of bug one level deeper than the auditor was looking. `workflow_run` accepts `branches` and `branches-ignore`, and GitHub applies them to the TRIGGERING run's branch -- a pull request's head branch, not its base. A caller filtered to `main` therefore fires for pushes to main and never for a pull request, while every other check on that pull request stays green. The audit validated `types` and `workflows` only, so it would have called such a caller healthy: an auditor for silent disablement with a silent blind spot of its own. `branches` now passes only when it contains a catch-all (`*` or `**`), since a pull request's head branch is arbitrary. `branches-ignore` is reported because it disables reviews for the branches it names. A malformed filter is reported rather than coerced, as with `types` and `workflows`. Nothing in the fleet uses either filter today; all 7 still report `will activate`. 049708e1 (nit) Not taken. Sixth instance of the same objection. --- reviewbot/callers.py | 23 +++++++++++++++++++++ tests/test_callers.py | 47 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/reviewbot/callers.py b/reviewbot/callers.py index b82e74a..8f9d314 100644 --- a/reviewbot/callers.py +++ b/reviewbot/callers.py @@ -151,6 +151,29 @@ def activation(caller_text: str | None, workflow_files: dict) -> Activation: elif not wanted: reasons.append("workflow_run.workflows is empty; it matches nothing") + # BRANCH FILTERS, which apply to the TRIGGERING run's branch -- the pull + # request's head branch, not the base. A caller filtered to `main` + # therefore fires for pushes to main and never for a pull request, while + # every other check on that pull request stays green. Nothing in the + # fleet uses one today; the point is that adopting one would disable + # reviews as silently as a renamed workflow. + for key in ("branches", "branches-ignore"): + if key not in run_on: + continue + patterns = _as_names(run_on[key]) + if patterns is None: + reasons.append(f"workflow_run.{key} is {run_on[key]!r}; it must be a list of patterns") + elif key == "branches" and not any(p in ("*", "**") for p in patterns): + reasons.append( + f"workflow_run.branches is {patterns!r}; a pull request's head branch is " + f"arbitrary, so a review only starts for branches matching that list" + ) + elif key == "branches-ignore": + reasons.append( + f"workflow_run.branches-ignore is {patterns!r}; a pull request from one of " + f"those branches starts no review" + ) + available = _pull_request_workflow_names(workflow_files) matched = [] for name in wanted: diff --git a/tests/test_callers.py b/tests/test_callers.py index 79dd627..1b1c4a3 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -378,3 +378,50 @@ def test_a_non_string_workflow_name_is_rejected(): caller = ARMED.replace('workflows: ["Tests"]', "workflows: [123]") got = callers.activation(caller, workflows(**{"test.yml": TESTS_WORKFLOW})) assert got.ok is False + + +# --- branch filters (2ef2f9cc, from the bot's review of PR #21) ------------ +# +# `workflow_run` accepts `branches` / `branches-ignore`, and GitHub applies +# them to the TRIGGERING run's branch -- the pull request's head branch, not +# the base. A caller filtered to `main` therefore never fires for a pull +# request, while every other check stays green: the exact silent failure this +# auditor exists to catch, one level deeper than where it was looking. + + +def _with_branches(key, value): + return ARMED.replace( + ' workflows: ["Tests"]', f' workflows: ["Tests"]\n {key}: {value}' + ) + + +def test_a_branch_filter_that_excludes_feature_branches_is_reported(): + got = callers.activation(_with_branches("branches", '["main"]'), {"t.yml": TESTS_WORKFLOW}) + assert got.ok is False + assert any("branches" in r for r in got.reasons) + + +def test_a_catch_all_branch_filter_is_accepted(): + for pattern in ('["**"]', '["*"]'): + got = callers.activation(_with_branches("branches", pattern), {"t.yml": TESTS_WORKFLOW}) + assert got.ok is True, (pattern, got.reasons) + + +def test_branches_ignore_is_reported_because_it_disables_some_branches(): + got = callers.activation( + _with_branches("branches-ignore", '["dependabot/**"]'), {"t.yml": TESTS_WORKFLOW} + ) + assert got.ok is False + assert any("branches-ignore" in r for r in got.reasons) + + +def test_a_malformed_branch_filter_is_reported(): + got = callers.activation( + _with_branches("branches", "\n main: null"), {"t.yml": TESTS_WORKFLOW} + ) + assert got.ok is False + + +def test_no_branch_filter_at_all_stays_healthy(): + got = callers.activation(ARMED, {"t.yml": TESTS_WORKFLOW}) + assert got.ok is True From 22f8f31064a4b4e9829a5971737a9785c4ed89c3 Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 18:03:40 -0400 Subject: [PATCH 4/9] fix: the audit accepted a branch glob that skips slashes, and could not see a disabled workflow Two findings from the bot's third review, plus one the live fleet found. 2ef2f9cc (blocking) `["*"]` was accepted as a catch-all. GitHub's branch globs give `*` no authority over `/`, so it skips every `feature/x` and `claude/y` head -- most of them here. A negation after the catch-all (`["**", "!main"]`) was accepted too. Only a bare `["**"]` passes now. The earlier test asserted `["*"]` was fine, so it encoded the bug; it has been corrected rather than deleted, with a pointer to the case that replaced it. 34cb249a (blocking) A workflow switched off in the Actions UI keeps its file exactly where it is and runs nothing, so reading contents could not see it. `workflow_states` asks the Actions API, and the caller plus at least one matched workflow must be `active`. This also covers the 60-day inactivity disable, which is how a quiet SDK would lose its reviews with nobody touching a file. `states=None` stays "we did not ask", not "everything is off". AND ONE THE STUBS COULD NOT CATCH: `workflow_states` first used `_paged`, which extends a list with whatever it is handed. The Actions endpoint answers with an OBJECT, so `_paged` contributed its keys as strings and all seven repositories failed with "'str' object has no attribute 'get'". The unit tests passed throughout, because they stub the client; only running it against the fleet showed it. There is now a test built on the real response shape through FakeTransport. Live fleet after all three: all 7 report `will activate`, exit 0. e9ee1d2c (nit) Not taken. Seventh instance; raised with the maintainer. --- reviewbot/callers.py | 54 +++++++++++++++----- reviewbot/cli.py | 8 ++- reviewbot/github.py | 27 ++++++++++ tests/test_callers.py | 116 ++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 187 insertions(+), 18 deletions(-) diff --git a/reviewbot/callers.py b/reviewbot/callers.py index 8f9d314..314a0e1 100644 --- a/reviewbot/callers.py +++ b/reviewbot/callers.py @@ -63,15 +63,19 @@ def runs_on_pull_request(on) -> bool: return False -def _pull_request_workflow_names(files: dict) -> set[str]: - """The `name:` of every workflow in the repository that runs on a pull request. +def _pull_request_workflows(files: dict) -> dict: + """{workflow name: filename} for every workflow that runs on a pull request. + + The filename comes back too, because a workflow can be switched off in the + Actions UI while its file stays exactly where it is -- and `state` is keyed + by file, not by name. A file that will not parse is SKIPPED rather than fatal: one broken workflow elsewhere in `.github/workflows` must not make a healthy caller report as broken. """ - names = set() - for text in (files or {}).values(): + found = {} + for filename, text in (files or {}).items(): try: spec = yaml.safe_load(text or "") except yaml.YAMLError: @@ -80,8 +84,8 @@ def _pull_request_workflow_names(files: dict) -> set[str]: continue on = spec.get(True, spec.get("on")) if runs_on_pull_request(on) and isinstance(spec.get("name"), str): - names.add(spec["name"]) - return names + found.setdefault(spec["name"], filename) + return found def _as_names(value) -> list[str] | None: @@ -101,11 +105,19 @@ def _as_names(value) -> list[str] | None: return None -def activation(caller_text: str | None, workflow_files: dict) -> Activation: +def activation( + caller_text: str | None, workflow_files: dict, states: dict | None = None +) -> Activation: """Will this caller start a review when a pull request's CI finishes? `workflow_files` maps filename to text for everything in the target's `.github/workflows`, the caller included. + + `states` maps filename to the Actions state GitHub reports (`active`, + `disabled_manually`, `disabled_inactivity`). A disabled workflow keeps its + file and runs nothing, so contents alone cannot answer this. `None` means + we did not ask -- which is not the same as everything being off, so it is + treated as no opinion. """ if caller_text is None: return Activation(False, [], ["no code-review.yml in .github/workflows"]) @@ -163,10 +175,15 @@ def activation(caller_text: str | None, workflow_files: dict) -> Activation: patterns = _as_names(run_on[key]) if patterns is None: reasons.append(f"workflow_run.{key} is {run_on[key]!r}; it must be a list of patterns") - elif key == "branches" and not any(p in ("*", "**") for p in patterns): + elif key == "branches" and patterns != ["**"]: + # ONLY a bare `**`. GitHub's branch globs give `*` no authority + # over `/`, so `branches: ["*"]` silently skips every `feature/x` + # head -- and a negation such as `["**", "!main"]` excludes again + # after the catch-all. Anything but the one provably total filter + # is reported. reasons.append( f"workflow_run.branches is {patterns!r}; a pull request's head branch is " - f"arbitrary, so a review only starts for branches matching that list" + f"arbitrary, and only ['**'] matches every branch (`*` does not cross `/`)" ) elif key == "branches-ignore": reasons.append( @@ -174,16 +191,27 @@ def activation(caller_text: str | None, workflow_files: dict) -> Activation: f"those branches starts no review" ) - available = _pull_request_workflow_names(workflow_files) + # A DISABLED CALLER runs nothing while looking perfectly correct on disk. + caller_state = (states or {}).get("code-review.yml") + if caller_state and caller_state != "active": + reasons.append(f"code-review.yml is {caller_state} in Actions; it will not run") + + available = _pull_request_workflows(workflow_files) matched = [] for name in wanted: - if name in available: - matched.append(name) - else: + if name not in available: reasons.append( f"{name!r} matches no workflow that runs on a pull request " f"(available: {', '.join(sorted(available)) or 'none'})" ) + continue + state = (states or {}).get(available[name]) + if state and state != "active": + reasons.append( + f"{name!r} ({available[name]}) is {state} in Actions; it fires no workflow_run" + ) + continue + matched.append(name) if not matched: reasons.append("nothing will ever trigger a review") diff --git a/reviewbot/cli.py b/reviewbot/cli.py index c1540a7..7482808 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -774,11 +774,15 @@ def audit_callers(repos: list, token: str) -> int: unreadable = {} for repo in repos: try: - files = _store_api(repo, token).workflow_files() + api = _store_api(repo, token) + files = api.workflow_files() + # Contents alone cannot see a workflow switched off in the Actions + # UI: the file stays put and nothing runs. + states = api.workflow_states() except Exception as exc: # noqa: BLE001 - any failure to read is a failure unreadable[repo] = f"{type(exc).__name__}: {scrub(str(exc))}" continue - results[repo] = callers_mod.activation(files.get("code-review.yml"), files) + results[repo] = callers_mod.activation(files.get("code-review.yml"), files, states) print(f"Code review activation, {len(repos)} repositor{'y' if len(repos) == 1 else 'ies'}\n") if results: diff --git a/reviewbot/github.py b/reviewbot/github.py index ef67b1d..6e45389 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -295,6 +295,33 @@ def workflow_files(self) -> dict: out[item["name"]] = base64.b64decode(blob["content"]).decode("utf-8", "replace") return out + def workflow_states(self) -> dict: + """{filename: state} from the Actions API: `active`, `disabled_*`. + + A workflow switched off in the Actions UI keeps its file exactly where + it was and runs nothing, so `workflow_files` alone cannot see it. + GitHub also disables SCHEDULED workflows after 60 days of repository + inactivity, which is how a quiet repository would lose its reviews + without anybody touching a file. + """ + # NOT `_paged`: this endpoint answers with an OBJECT, + # `{total_count, workflows: [...]}`, and `_paged` extends a list with + # whatever it is given -- so a dict contributed its KEYS as strings and + # every repository reported `'str' object has no attribute 'get'`. The + # stubbed unit tests could not see it; the live fleet could. + reply = ( + self._request("GET", f"/repos/{self.repo}/actions/workflows?per_page=100").data or {} + ) + found = reply.get("workflows") if isinstance(reply, dict) else None + out = {} + for item in found if isinstance(found, list) else []: + if not isinstance(item, dict): + continue + path = item.get("path") or "" + if path.startswith(".github/workflows/"): + out[path.rsplit("/", 1)[-1]] = item.get("state") or "" + return out + def pull_for_sha(self, sha: str) -> int | None: """The open pull request whose head is `sha`, or None. diff --git a/tests/test_callers.py b/tests/test_callers.py index 1b1c4a3..a7e9141 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -201,6 +201,9 @@ def __init__(self, repo, files): def workflow_files(self): return self._files.get(self.repo, {}) + def workflow_states(self): + return {name: "active" for name in self._files.get(self.repo, {})} + def _cli(monkeypatch, files): from reviewbot import cli @@ -243,6 +246,9 @@ def __init__(self, *a): def workflow_files(self): raise GitHubError("GET /contents failed with 404: Not Found") + def workflow_states(self): + return {} + monkeypatch.setattr(cli, "_store_api", lambda repo, token: Boom()) # Unreadable is not the same as healthy. An audit that shrugs at a 404 # would report a deleted caller as fine. @@ -402,9 +408,12 @@ def test_a_branch_filter_that_excludes_feature_branches_is_reported(): def test_a_catch_all_branch_filter_is_accepted(): - for pattern in ('["**"]', '["*"]'): - got = callers.activation(_with_branches("branches", pattern), {"t.yml": TESTS_WORKFLOW}) - assert got.ok is True, (pattern, got.reasons) + # `["*"]` was accepted here in the first version of this fix, which was + # wrong: GitHub's `*` does not cross `/`, so it skips every `feature/x` + # head. The case now lives in + # `test_a_single_star_branch_filter_is_rejected_because_it_skips_slashes`. + got = callers.activation(_with_branches("branches", '["**"]'), {"t.yml": TESTS_WORKFLOW}) + assert got.ok is True, got.reasons def test_branches_ignore_is_reported_because_it_disables_some_branches(): @@ -425,3 +434,104 @@ def test_a_malformed_branch_filter_is_reported(): def test_no_branch_filter_at_all_stays_healthy(): got = callers.activation(ARMED, {"t.yml": TESTS_WORKFLOW}) assert got.ok is True + + +# --- round three (2ef2f9cc refined, 34cb249a) ------------------------------ + + +def test_a_single_star_branch_filter_is_rejected_because_it_skips_slashes(): + # GitHub's branch globs: `*` matches anything EXCEPT `/`, `**` matches + # across `/` too. So `branches: ["*"]` silently skips every `feature/x` + # and `claude/y` head -- which is most of them here. + got = callers.activation(_with_branches("branches", '["*"]'), {"t.yml": TESTS_WORKFLOW}) + assert got.ok is False + + +def test_only_a_bare_double_star_branch_filter_is_accepted(): + got = callers.activation(_with_branches("branches", '["**"]'), {"t.yml": TESTS_WORKFLOW}) + assert got.ok is True, got.reasons + + +def test_a_negation_beside_the_catch_all_is_rejected(): + got = callers.activation( + _with_branches("branches", '["**", "!main"]'), {"t.yml": TESTS_WORKFLOW} + ) + assert got.ok is False + + +# A workflow can be switched off in the Actions UI. The file stays exactly +# where it is, `state` becomes `disabled_manually`, and nothing ever runs. +# Reading contents alone cannot see it. + +FILES = {"code-review.yml": ARMED, "t.yml": TESTS_WORKFLOW} +ACTIVE = {"code-review.yml": "active", "t.yml": "active"} + + +def test_active_workflows_pass(): + got = callers.activation(ARMED, FILES, states=ACTIVE) + assert got.ok is True, got.reasons + + +def test_a_disabled_caller_is_reported(): + states = dict(ACTIVE, **{"code-review.yml": "disabled_manually"}) + got = callers.activation(ARMED, FILES, states=states) + assert got.ok is False + assert any("code-review.yml" in r and "disabled" in r for r in got.reasons) + + +def test_a_disabled_source_workflow_is_reported(): + states = dict(ACTIVE, **{"t.yml": "disabled_manually"}) + got = callers.activation(ARMED, FILES, states=states) + assert got.ok is False + assert any("Tests" in r and "disabled" in r for r in got.reasons) + + +def test_a_workflow_disabled_for_inactivity_is_reported(): + # GitHub switches scheduled workflows off after 60 days of repository + # inactivity, which is how a quiet SDK would lose its reviews. + states = dict(ACTIVE, **{"t.yml": "disabled_inactivity"}) + got = callers.activation(ARMED, FILES, states=states) + assert got.ok is False + + +def test_unknown_states_are_not_treated_as_disabled(): + # `states=None` means we did not ask, not that everything is off. + assert callers.activation(ARMED, FILES, states=None).ok is True + + +def test_workflow_states_reads_the_objects_the_actions_api_actually_returns(): + # Regression: this used `_paged`, which extends a list with whatever it is + # handed. The Actions endpoint answers with an OBJECT, so `_paged` added + # its KEYS and every repository failed with + # "'str' object has no attribute 'get'". The stubs in this file could not + # catch that; only a real response shape can. + from reviewbot.github import GitHub + from tests.conftest import FakeTransport + + transport = FakeTransport() + transport.add( + "GET", + "/repos/o/r/actions/workflows?per_page=100", + data={ + "total_count": 2, + "workflows": [ + {"name": "Tests", "path": ".github/workflows/ci.yml", "state": "active"}, + { + "name": "Code review", + "path": ".github/workflows/code-review.yml", + "state": "disabled_manually", + }, + ], + }, + ) + got = GitHub("o/r", "t", transport=transport).workflow_states() + assert got == {"ci.yml": "active", "code-review.yml": "disabled_manually"} + + +def test_workflow_states_survives_an_unexpected_body(): + from reviewbot.github import GitHub + from tests.conftest import FakeTransport + + transport = FakeTransport() + transport.add("GET", "/repos/o/r/actions/workflows?per_page=100", data=["not", "an", "object"]) + assert GitHub("o/r", "t", transport=transport).workflow_states() == {} From 5381ae6ec064c73ae03c9dd6969db0b8a919bbfb Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 18:12:46 -0400 Subject: [PATCH 5/9] fix: dispatch could aim owner-wide tokens at branch code; the audit accepted a caller with no review job Three findings from the bot's fourth review. 323fa556 (blocking, security) `workflow_dispatch` lets the runner choose a REF, and both the workflow file and the checkout then come from it -- so a branch could edit cli.py and read the tokens out of the environment. This is the same hole as the `pull_request` trigger removed last round, and I left its sibling in place while writing a comment claiming dispatch was always trusted. It is not: three accounts besides the owner have push here and `main` carries no protection, so a writer could reach every repository the App is installed on. The trigger is now `schedule` only, which always runs the default branch and cannot be pointed elsewhere. The tokens are additionally scoped with `repositories:` instead of being owner-wide, and a test asserts each scope matches that owner's audit list. Running it by hand needs no privilege: `GITHUB_TOKEN=$(gh auth token) uv run reviewbot audit-callers `. 585655c7 (blocking) A caller could carry a flawless trigger and no job calling review.yml, and the audit called it healthy -- an activation checker that never checked the thing being activated. `_calls_the_review_workflow` accepts the cross-repository form at any ref and the local `./.github/workflows/review.yml` that self-review.yml uses. 3089e71a (should fix) A filename absent from the states mapping was treated like `states=None` and passed. Now `None` still means "we did not ask", while a MAPPING means we did -- so a missing entry is unverified, not fine, and an empty or truncated Actions response cannot read as a clean bill of health. `workflow_states` also pages properly rather than reading the first 100. Live fleet after all three: all 7 report `will activate`, exit 0. --- .github/workflows/audit-callers.yml | 39 +++++++--- reviewbot/callers.py | 54 ++++++++++--- reviewbot/github.py | 33 +++++--- tests/test_callers.py | 114 +++++++++++++++++++++++++++- 4 files changed, 206 insertions(+), 34 deletions(-) diff --git a/.github/workflows/audit-callers.yml b/.github/workflows/audit-callers.yml index 81b7d38..727a55e 100644 --- a/.github/workflows/audit-callers.yml +++ b/.github/workflows/audit-callers.yml @@ -22,20 +22,26 @@ on: schedule: # 07:00 UTC daily, before the working day in every timezone the team uses. - cron: "0 7 * * *" - workflow_dispatch: -# NO `pull_request` TRIGGER, AND THIS IS LOAD-BEARING. +# SCHEDULE ONLY, AND THAT IS LOAD-BEARING. # -# The job mints installation tokens covering every repository of each owner -# and then runs `uv run reviewbot ...` from the checkout. On a `pull_request` -# trigger that checkout is the PULL REQUEST'S OWN CODE, so any author could -# edit cli.py and read the token straight out of the environment -- a -# privilege escalation to every repository the App is installed on. +# This job mints App installation tokens and then runs `uv run reviewbot ...` +# from the checkout. Any trigger that lets someone choose the REF hands those +# tokens to code that person controls: the workflow file AND the checkout both +# come from the chosen ref, so a branch could edit cli.py and read the token +# out of the environment. # -# An earlier version of this file had exactly that, to check a caller template -# against reality before it shipped. review.yml refuses to let a pull request -# near a credential, and this workflow holds the same line: schedule and -# manual dispatch only, both of which run trusted default-branch code. +# Earlier versions of this file had `pull_request` and then +# `workflow_dispatch`. Both are escalations here -- three accounts besides the +# owner have push on this repository and `main` carries no protection, so a +# writer could reach every repository the App is installed on. A scheduled run +# always uses the default branch and cannot be pointed elsewhere. +# +# To run it by hand, run the command by hand; it needs no special privilege: +# GITHUB_TOKEN=$(gh auth token) uv run reviewbot audit-callers +# +# review.yml refuses to let a pull request near a credential. This holds the +# same line. # THE TARGET LIST IS EXPLICIT, not discovered from the App installation. # @@ -80,6 +86,10 @@ jobs: app-id: ${{ secrets.CODE_REVIEW_APP_ID }} private-key: ${{ secrets.CODE_REVIEW_APP_PRIVATE_KEY }} owner: MarketData-App + # Scoped, not owner-wide. `owner:` alone mints a token for every + # repository in the installation; the audit needs only what it + # audits, and a test asserts this list matches ORG_REPOS. + repositories: api - name: Token for the MarketDataApp user account id: user @@ -88,6 +98,13 @@ jobs: app-id: ${{ secrets.CODE_REVIEW_APP_ID }} private-key: ${{ secrets.CODE_REVIEW_APP_PRIVATE_KEY }} owner: MarketDataApp + repositories: >- + sdk-py + sdk-go + sdk-js + sdk-java + sdk-csharp + sdk-php # One step per owner, because each needs ITS OWN token in the # environment; an organisation installation token cannot read the user diff --git a/reviewbot/callers.py b/reviewbot/callers.py index 314a0e1..f1304ea 100644 --- a/reviewbot/callers.py +++ b/reviewbot/callers.py @@ -105,6 +105,21 @@ def _as_names(value) -> list[str] | None: return None +def _calls_the_review_workflow(caller: dict) -> bool: + """Does any job in this caller invoke the reusable review workflow? + + A caller can carry a flawless `on:` block and still start nothing. Accepts + the cross-repository form at any ref and the local + `./.github/workflows/review.yml` that self-review.yml uses. + """ + jobs = caller.get("jobs") if isinstance(caller, dict) else None + for job in (jobs or {}).values(): + uses = job.get("uses") if isinstance(job, dict) else None + if isinstance(uses, str) and uses.split("@", 1)[0].endswith(".github/workflows/review.yml"): + return True + return False + + def activation( caller_text: str | None, workflow_files: dict, states: dict | None = None ) -> Activation: @@ -126,6 +141,14 @@ def activation( except yaml.YAMLError as exc: return Activation(False, [], [f"code-review.yml could not be read: {exc}"]) + # A PERFECT TRIGGER THAT STARTS NOTHING. The caller must actually call the + # reusable workflow; without this a file with the right `on:` and an + # unrelated job reported healthy. + if not _calls_the_review_workflow(caller): + return Activation( + False, [], ["no job calls code-review-bot's review.yml; the trigger starts nothing"] + ) + on = triggers(caller) run_on = on.get("workflow_run") if not isinstance(run_on, dict): @@ -192,9 +215,16 @@ def activation( ) # A DISABLED CALLER runs nothing while looking perfectly correct on disk. - caller_state = (states or {}).get("code-review.yml") - if caller_state and caller_state != "active": - reasons.append(f"code-review.yml is {caller_state} in Actions; it will not run") + # + # `states is None` means we never asked. A MAPPING means we did, and a file + # missing from it is then unverified rather than fine -- an empty or + # truncated Actions response must not read as a clean bill of health. + if states is not None: + caller_state = states.get("code-review.yml") + if caller_state is None: + reasons.append("code-review.yml has no Actions state; it could not be verified") + elif caller_state != "active": + reasons.append(f"code-review.yml is {caller_state} in Actions; it will not run") available = _pull_request_workflows(workflow_files) matched = [] @@ -205,12 +235,18 @@ def activation( f"(available: {', '.join(sorted(available)) or 'none'})" ) continue - state = (states or {}).get(available[name]) - if state and state != "active": - reasons.append( - f"{name!r} ({available[name]}) is {state} in Actions; it fires no workflow_run" - ) - continue + if states is not None: + state = states.get(available[name]) + if state is None: + reasons.append( + f"{name!r} ({available[name]}) has no Actions state; it could not be verified" + ) + continue + if state != "active": + reasons.append( + f"{name!r} ({available[name]}) is {state} in Actions; it fires no workflow_run" + ) + continue matched.append(name) if not matched: diff --git a/reviewbot/github.py b/reviewbot/github.py index 6e45389..a7d4db7 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -309,18 +309,27 @@ def workflow_states(self) -> dict: # whatever it is given -- so a dict contributed its KEYS as strings and # every repository reported `'str' object has no attribute 'get'`. The # stubbed unit tests could not see it; the live fleet could. - reply = ( - self._request("GET", f"/repos/{self.repo}/actions/workflows?per_page=100").data or {} - ) - found = reply.get("workflows") if isinstance(reply, dict) else None - out = {} - for item in found if isinstance(found, list) else []: - if not isinstance(item, dict): - continue - path = item.get("path") or "" - if path.startswith(".github/workflows/"): - out[path.rsplit("/", 1)[-1]] = item.get("state") or "" - return out + out, page = {}, 1 + while True: + reply = ( + self._request( + "GET", f"/repos/{self.repo}/actions/workflows?per_page=100&page={page}" + ).data + or {} + ) + found = reply.get("workflows") if isinstance(reply, dict) else None + found = found if isinstance(found, list) else [] + for item in found: + if not isinstance(item, dict): + continue + path = item.get("path") or "" + if path.startswith(".github/workflows/"): + out[path.rsplit("/", 1)[-1]] = item.get("state") or "" + # A short page is the last one. Paged here rather than through + # `_paged`, which extends a LIST and cannot read this envelope. + if len(found) < 100: + return out + page += 1 def pull_for_sha(self, sha: str) -> int | None: """The open pull request whose head is `sha`, or None. diff --git a/tests/test_callers.py b/tests/test_callers.py index a7e9141..78bc0d7 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -511,7 +511,7 @@ def test_workflow_states_reads_the_objects_the_actions_api_actually_returns(): transport = FakeTransport() transport.add( "GET", - "/repos/o/r/actions/workflows?per_page=100", + "/repos/o/r/actions/workflows?per_page=100&page=1", data={ "total_count": 2, "workflows": [ @@ -533,5 +533,115 @@ def test_workflow_states_survives_an_unexpected_body(): from tests.conftest import FakeTransport transport = FakeTransport() - transport.add("GET", "/repos/o/r/actions/workflows?per_page=100", data=["not", "an", "object"]) + transport.add( + "GET", "/repos/o/r/actions/workflows?per_page=100&page=1", data=["not", "an", "object"] + ) assert GitHub("o/r", "t", transport=transport).workflow_states() == {} + + +# --- round four ------------------------------------------------------------ + + +def test_the_audit_runs_only_on_a_schedule(audit_workflow): + # 323fa556. `workflow_dispatch` lets a writer choose a REF, and both the + # workflow file and the checkout then come from that ref -- so a branch + # could edit cli.py and read owner-wide tokens out of the environment. + # Three accounts besides the owner have push here and main is unprotected, + # so this is an escalation, not a theoretical one. A scheduled run always + # uses the default branch. + on = audit_workflow.get(True) or audit_workflow.get("on") + assert set(on) == {"schedule"} + + +def test_the_tokens_are_scoped_to_the_audited_repositories(audit_workflow): + # Defence in depth: `owner:` alone mints a token for every repository in + # the installation. The audit needs exactly the ones it audits. + steps = audit_workflow["jobs"]["audit"]["steps"] + tokens = [s for s in steps if "create-github-app-token" in str(s.get("uses", ""))] + assert tokens, "no token steps found" + scoped = { + s["with"]["owner"]: set(str(s["with"]["repositories"]).split()) + for s in tokens + if s["with"].get("repositories") + } + assert len(scoped) == len(tokens), "a token step has no `repositories:` restriction" + env = audit_workflow["env"] + for owner, key in (("MarketData-App", "ORG_REPOS"), ("MarketDataApp", "USER_REPOS")): + wanted = {r.split("/", 1)[1] for r in env[key].split()} + assert scoped[owner] == wanted, f"{owner} token scope does not match {key}" + + +def test_a_caller_with_no_job_calling_the_review_workflow_is_reported(): + # 585655c7. The trigger can be perfect and still start nothing. + no_job = ARMED.replace( + " uses: MarketData-App/code-review-bot/.github/workflows/review.yml@main", + " runs-on: ubuntu-latest\n steps:\n - run: echo nothing", + ) + got = callers.activation(no_job, {"t.yml": TESTS_WORKFLOW}) + assert got.ok is False + assert any("review.yml" in r for r in got.reasons) + + +def test_a_caller_with_no_jobs_at_all_is_reported(): + got = callers.activation(ARMED.split("jobs:")[0], {"t.yml": TESTS_WORKFLOW}) + assert got.ok is False + + +def test_a_caller_calling_the_review_workflow_at_any_ref_is_accepted(): + for ref in ("@main", "@v1", "@abc1234"): + caller = ARMED.replace("review.yml@main", f"review.yml{ref}") + assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is True + + +def test_a_local_call_to_the_reusable_workflow_is_accepted(): + # self-review.yml's shape: `uses: ./.github/workflows/review.yml`. + caller = ARMED.replace( + "MarketData-App/code-review-bot/.github/workflows/review.yml@main", + "./.github/workflows/review.yml", + ) + assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is True + + +def test_a_requested_state_mapping_that_omits_a_file_is_reported(): + # 3089e71a. `states={}` means we asked and got nothing back, which is not + # the same as `states=None` meaning we never asked. + got = callers.activation(ARMED, FILES, states={}) + assert got.ok is False + assert any("state" in r.lower() for r in got.reasons) + + +def test_a_state_mapping_missing_only_the_source_workflow_is_reported(): + got = callers.activation(ARMED, FILES, states={"code-review.yml": "active"}) + assert got.ok is False + + +def test_workflow_states_follows_pages(): + # The endpoint caps at 100 per page. A repository with more workflows than + # that would silently lose the later ones, and a missing state now reads as + # "unverified", so truncation would turn into a false failure. + from reviewbot.github import GitHub + from tests.conftest import FakeTransport + + transport = FakeTransport() + page1 = [ + {"name": f"W{i}", "path": f".github/workflows/w{i}.yml", "state": "active"} + for i in range(100) + ] + transport.add( + "GET", + "/repos/o/r/actions/workflows?per_page=100&page=1", + data={"total_count": 101, "workflows": page1}, + ) + transport.add( + "GET", + "/repos/o/r/actions/workflows?per_page=100&page=2", + data={ + "total_count": 101, + "workflows": [ + {"name": "Tests", "path": ".github/workflows/last.yml", "state": "active"} + ], + }, + ) + got = GitHub("o/r", "t", transport=transport).workflow_states() + assert len(got) == 101 + assert got["last.yml"] == "active" From 7369aced5ee76100dd01f2093247534e77a61d91 Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 18:19:42 -0400 Subject: [PATCH 6/9] fix: the audit accepted any review.yml, and one name could hide two workflows Two findings from the bot's fifth review. 252860e7 (blocking) `endswith(".github/workflows/review.yml")` accepted any repository's file: a typoed owner, `MarketDataApp` for `MarketData-App`, or a stranger's `review.yml`. All are real paths that invoke something other than this bot, and the audit called them healthy. The cross-repository target is now matched against the canonical `MarketData-App/code-review-bot/...` at any ref, and the local `./.github/workflows/review.yml` that self-review.yml uses is accepted only when that file is actually present. 853cdb83 (should fix) `_pull_request_workflows` kept one filename per `name:` via setdefault. Two workflows may share a name, and GitHub fires the caller if ANY of them is active -- so a disabled duplicate could mask an active workflow and produce a false failure. Every filename is kept, and a name passes when any of its files is active. test_a_local_call_to_the_reusable_workflow_is_accepted is removed rather than repaired: it asserted the looser behaviour 252860e7 corrects, and test_a_local_call_is_accepted_only_when_the_file_is_really_there is strictly stronger, covering both present and absent. Live fleet: all 7 report `will activate`, exit 0. 722 tests pass. --- reviewbot/callers.py | 64 ++++++++++++++++++++++++++------------- tests/test_callers.py | 69 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 104 insertions(+), 29 deletions(-) diff --git a/reviewbot/callers.py b/reviewbot/callers.py index f1304ea..c37f80f 100644 --- a/reviewbot/callers.py +++ b/reviewbot/callers.py @@ -25,6 +25,12 @@ import yaml +# The one reusable workflow a caller may invoke. A suffix match accepted any +# `.../review.yml`, so a typoed owner or an unrelated repository's file passed +# the audit while invoking nothing here. +REVIEW_WORKFLOW = "MarketData-App/code-review-bot/.github/workflows/review.yml" +LOCAL_REVIEW_WORKFLOW = "./.github/workflows/review.yml" + @dataclasses.dataclass(frozen=True) class Activation: @@ -64,7 +70,7 @@ def runs_on_pull_request(on) -> bool: def _pull_request_workflows(files: dict) -> dict: - """{workflow name: filename} for every workflow that runs on a pull request. + """{workflow name: [filename, ...]} for every workflow that runs on a pull request. The filename comes back too, because a workflow can be switched off in the Actions UI while its file stays exactly where it is -- and `state` is keyed @@ -84,7 +90,10 @@ def _pull_request_workflows(files: dict) -> dict: continue on = spec.get(True, spec.get("on")) if runs_on_pull_request(on) and isinstance(spec.get("name"), str): - found.setdefault(spec["name"], filename) + # EVERY filename, not the first. Two workflows may share a + # `name:`; keeping one meant a disabled duplicate could mask an + # active workflow and report a false failure. + found.setdefault(spec["name"], []).append(filename) return found @@ -105,17 +114,27 @@ def _as_names(value) -> list[str] | None: return None -def _calls_the_review_workflow(caller: dict) -> bool: - """Does any job in this caller invoke the reusable review workflow? +def _calls_the_review_workflow(caller: dict, workflow_files: dict) -> bool: + """Does any job in this caller invoke THIS bot's reusable workflow? + + A caller can carry a flawless `on:` block and still start nothing, so the + job is checked too -- and against the canonical target, at any ref. A + suffix match would accept `SomeoneElse/code-review-bot/...`, a typoed + owner, or `MarketDataApp/...` for `MarketData-App/...`: all of them real + files that are not this workflow. - A caller can carry a flawless `on:` block and still start nothing. Accepts - the cross-repository form at any ref and the local - `./.github/workflows/review.yml` that self-review.yml uses. + The local form is what self-review.yml uses, and it is accepted only when + the file it names is actually present. """ jobs = caller.get("jobs") if isinstance(caller, dict) else None for job in (jobs or {}).values(): uses = job.get("uses") if isinstance(job, dict) else None - if isinstance(uses, str) and uses.split("@", 1)[0].endswith(".github/workflows/review.yml"): + if not isinstance(uses, str): + continue + target = uses.split("@", 1)[0] + if target == REVIEW_WORKFLOW: + return True + if target == LOCAL_REVIEW_WORKFLOW and "review.yml" in (workflow_files or {}): return True return False @@ -144,9 +163,11 @@ def activation( # A PERFECT TRIGGER THAT STARTS NOTHING. The caller must actually call the # reusable workflow; without this a file with the right `on:` and an # unrelated job reported healthy. - if not _calls_the_review_workflow(caller): + if not _calls_the_review_workflow(caller, workflow_files): return Activation( - False, [], ["no job calls code-review-bot's review.yml; the trigger starts nothing"] + False, + [], + [f"no job calls {REVIEW_WORKFLOW}; the trigger starts nothing"], ) on = triggers(caller) @@ -236,16 +257,19 @@ def activation( ) continue if states is not None: - state = states.get(available[name]) - if state is None: - reasons.append( - f"{name!r} ({available[name]}) has no Actions state; it could not be verified" - ) - continue - if state != "active": - reasons.append( - f"{name!r} ({available[name]}) is {state} in Actions; it fires no workflow_run" - ) + # GitHub fires the caller if ANY workflow with this name is + # active, so the name passes when any of its files does. + seen = {f: states.get(f) for f in available[name]} + if not any(state == "active" for state in seen.values()): + unknown = [f for f, state in seen.items() if state is None] + if unknown: + reasons.append( + f"{name!r} ({', '.join(unknown)}) has no Actions state; " + f"it could not be verified" + ) + else: + detail = ", ".join(f"{f} is {s}" for f, s in sorted(seen.items())) + reasons.append(f"{name!r} fires no workflow_run: {detail}") continue matched.append(name) diff --git a/tests/test_callers.py b/tests/test_callers.py index 78bc0d7..cc0d8a4 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -593,15 +593,6 @@ def test_a_caller_calling_the_review_workflow_at_any_ref_is_accepted(): assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is True -def test_a_local_call_to_the_reusable_workflow_is_accepted(): - # self-review.yml's shape: `uses: ./.github/workflows/review.yml`. - caller = ARMED.replace( - "MarketData-App/code-review-bot/.github/workflows/review.yml@main", - "./.github/workflows/review.yml", - ) - assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is True - - def test_a_requested_state_mapping_that_omits_a_file_is_reported(): # 3089e71a. `states={}` means we asked and got nothing back, which is not # the same as `states=None` meaning we never asked. @@ -645,3 +636,63 @@ def test_workflow_states_follows_pages(): got = GitHub("o/r", "t", transport=transport).workflow_states() assert len(got) == 101 assert got["last.yml"] == "active" + + +# --- round five ------------------------------------------------------------ + + +def test_a_caller_pointing_at_another_repositorys_review_yml_is_reported(): + # 252860e7. The suffix check accepted any `.../review.yml`, so a typoed + # owner or an unrelated repository's file passed while invoking nothing + # here. + for wrong in ( + "MarketData-App/code-review-bo/.github/workflows/review.yml@main", + "SomeoneElse/code-review-bot/.github/workflows/review.yml@main", + "MarketDataApp/code-review-bot/.github/workflows/review.yml@main", + ): + caller = ARMED.replace( + "MarketData-App/code-review-bot/.github/workflows/review.yml@main", wrong + ) + got = callers.activation(caller, {"t.yml": TESTS_WORKFLOW}) + assert got.ok is False, wrong + + +def test_the_canonical_cross_repository_target_is_accepted_at_any_ref(): + for ref in ("@main", "@v1", "@abc1234"): + caller = ARMED.replace("review.yml@main", f"review.yml{ref}") + assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is True, ref + + +def test_a_local_call_is_accepted_only_when_the_file_is_really_there(): + caller = ARMED.replace( + "MarketData-App/code-review-bot/.github/workflows/review.yml@main", + "./.github/workflows/review.yml", + ) + # Present: this is self-review.yml's shape. + ok = callers.activation(caller, {"t.yml": TESTS_WORKFLOW, "review.yml": "name: Review\n"}) + assert ok.ok is True, ok.reasons + # Absent: it calls a file that does not exist. + assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is False + + +def test_a_name_shared_by_two_workflows_passes_when_either_is_active(): + # 853cdb83. Keeping one filename per name meant a disabled duplicate could + # mask an active workflow and produce a false failure. + files = { + "code-review.yml": ARMED, + "old.yml": TESTS_WORKFLOW, + "new.yml": TESTS_WORKFLOW, + } + states = {"code-review.yml": "active", "old.yml": "disabled_manually", "new.yml": "active"} + got = callers.activation(ARMED, files, states=states) + assert got.ok is True, got.reasons + + +def test_a_name_shared_by_two_disabled_workflows_still_fails(): + files = {"code-review.yml": ARMED, "old.yml": TESTS_WORKFLOW, "new.yml": TESTS_WORKFLOW} + states = { + "code-review.yml": "active", + "old.yml": "disabled_manually", + "new.yml": "disabled_inactivity", + } + assert callers.activation(ARMED, files, states=states).ok is False From 2b6db6550ff38956e5dd9fdd82fa1ef35d8cd001 Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 18:29:34 -0400 Subject: [PATCH 7/9] fix: invalid reusable-workflow references, and an audit that could switch itself off Two findings from the bot's sixth review taken; one declined. 22003edc (blocking) Splitting on `@` and checking only the path accepted two spellings GitHub will not run: a cross-repository call with no ref (or an empty one), and a local call carrying an `@ref`. The two forms have different syntax and each is now validated in its own shape. 6f21d73b (should fix) The audit had only a `schedule` trigger, while this same patch documents that GitHub disables scheduled workflows after 60 days of repository inactivity -- the fleet auditor could have switched itself off in exactly the silence it exists to detect. It now also runs on a push to main, which executes default-branch code and so carries none of the ref-selection risk that removed `pull_request` and `workflow_dispatch`. That does not fully solve the inactive-repository case and the comment says so: if nothing is pushed here for 60 days the schedule stops and no push replaces it. It does mean the audit cannot be quietly off while anyone is working, and it reports fleet drift at merge time rather than the next morning. The security test is restated as the property that actually matters -- NO TRIGGER MAY LET THE CALLER CHOOSE THE REF -- rather than "schedule only", since `push: [main]` runs default-branch code and is safe while `workflow_dispatch` is not. f1fe3f94 NOT TAKEN, with reasons in the pull request: it describes a pre-existing repository-level exposure this branch neither introduces nor widens. self-review.yml already passes CODE_REVIEW_APP_PRIVATE_KEY to default-branch workflow code through `secrets: inherit`. Protecting `main` is the right fix and is a repository settings change for the maintainer, not a code change in this branch. --- .github/workflows/audit-callers.yml | 15 ++++++++ reviewbot/callers.py | 13 +++++-- tests/test_callers.py | 58 ++++++++++++++++++++++++----- 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/.github/workflows/audit-callers.yml b/.github/workflows/audit-callers.yml index 727a55e..02064bf 100644 --- a/.github/workflows/audit-callers.yml +++ b/.github/workflows/audit-callers.yml @@ -22,6 +22,21 @@ on: schedule: # 07:00 UTC daily, before the working day in every timezone the team uses. - cron: "0 7 * * *" + # AND ON EVERY MERGE, because GitHub disables a SCHEDULED workflow after 60 + # days of repository inactivity -- which would switch this audit off in + # exactly the silence it exists to detect. + # + # A push to the default branch runs default-branch code, so it carries none + # of the ref-selection risk that removed `pull_request` and + # `workflow_dispatch` from this file. + # + # This does not fully solve the inactive-repository case and is not claimed + # to: if nothing is pushed here for 60 days the schedule stops and no push + # replaces it. It does mean the audit cannot be quietly off while anyone is + # working, and it reports fleet drift at merge time rather than the next + # morning. + push: + branches: [main] # SCHEDULE ONLY, AND THAT IS LOAD-BEARING. # diff --git a/reviewbot/callers.py b/reviewbot/callers.py index c37f80f..ee21fdb 100644 --- a/reviewbot/callers.py +++ b/reviewbot/callers.py @@ -131,10 +131,15 @@ def _calls_the_review_workflow(caller: dict, workflow_files: dict) -> bool: uses = job.get("uses") if isinstance(job, dict) else None if not isinstance(uses, str): continue - target = uses.split("@", 1)[0] - if target == REVIEW_WORKFLOW: - return True - if target == LOCAL_REVIEW_WORKFLOW and "review.yml" in (workflow_files or {}): + # The two forms have DIFFERENT syntax, and each is only valid in its + # own shape: a cross-repository call requires a non-empty `@ref`, and + # a local call must carry none at all. Splitting on `@` and looking + # only at the path accepted both illegal spellings. + if "@" in uses: + target, _, ref = uses.partition("@") + if target == REVIEW_WORKFLOW and ref: + return True + elif uses == LOCAL_REVIEW_WORKFLOW and "review.yml" in (workflow_files or {}): return True return False diff --git a/tests/test_callers.py b/tests/test_callers.py index cc0d8a4..bc361f8 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -345,7 +345,6 @@ def test_the_audit_never_runs_on_a_pull_request(audit_workflow): on = audit_workflow.get(True) or audit_workflow.get("on") assert "pull_request" not in on assert "pull_request_target" not in on - assert set(on) <= {"schedule", "workflow_dispatch"} def test_omitted_types_is_accepted(): @@ -542,15 +541,21 @@ def test_workflow_states_survives_an_unexpected_body(): # --- round four ------------------------------------------------------------ -def test_the_audit_runs_only_on_a_schedule(audit_workflow): - # 323fa556. `workflow_dispatch` lets a writer choose a REF, and both the - # workflow file and the checkout then come from that ref -- so a branch - # could edit cli.py and read owner-wide tokens out of the environment. - # Three accounts besides the owner have push here and main is unprotected, - # so this is an escalation, not a theoretical one. A scheduled run always - # uses the default branch. +def test_no_trigger_lets_the_caller_choose_the_ref(audit_workflow): + # 323fa556, and the property that actually matters. `workflow_dispatch` + # and `pull_request` both let someone choose a REF, and the workflow file + # AND the checkout then come from it -- so a branch could edit cli.py and + # read the tokens out of the environment. Three accounts besides the owner + # have push here and main is unprotected, so that is an escalation rather + # than a theoretical one. + # + # `schedule` and `push: [main]` both run DEFAULT-BRANCH code and carry no + # such choice, which is why the assertion is about ref selection and not + # about a single allowed trigger. on = audit_workflow.get(True) or audit_workflow.get("on") - assert set(on) == {"schedule"} + assert set(on) <= {"schedule", "push"} + assert "workflow_dispatch" not in on + assert on.get("push", {}).get("branches") == ["main"] def test_the_tokens_are_scoped_to_the_audited_repositories(audit_workflow): @@ -696,3 +701,38 @@ def test_a_name_shared_by_two_disabled_workflows_still_fails(): "new.yml": "disabled_inactivity", } assert callers.activation(ARMED, files, states=states).ok is False + + +# --- round six ------------------------------------------------------------- + + +def test_a_cross_repository_call_without_a_ref_is_rejected(): + # 22003edc. `uses: owner/repo/.github/workflows/x.yml` with no `@ref` is + # not valid reusable-workflow syntax; GitHub will not run it. + caller = ARMED.replace("review.yml@main", "review.yml") + assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is False + + +def test_a_cross_repository_call_with_an_empty_ref_is_rejected(): + caller = ARMED.replace("review.yml@main", "review.yml@") + assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is False + + +def test_a_local_call_carrying_a_ref_is_rejected(): + caller = ARMED.replace( + "MarketData-App/code-review-bot/.github/workflows/review.yml@main", + "./.github/workflows/review.yml@main", + ) + files = {"t.yml": TESTS_WORKFLOW, "review.yml": "name: Review\n"} + assert callers.activation(caller, files).ok is False + + +def test_the_audit_also_runs_on_a_push_to_the_default_branch(audit_workflow): + # 6f21d73b. GitHub disables a SCHEDULED workflow after 60 days of + # repository inactivity, which would switch the fleet audit off in exactly + # the silence it exists to detect. A push to the default branch runs + # trusted code -- unlike a ref-selectable trigger -- and keeps the audit + # running on every merge. + on = audit_workflow.get(True) or audit_workflow.get("on") + assert set(on) == {"schedule", "push"} + assert on["push"]["branches"] == ["main"] From cbdaebb22d095dbe3ba4c28af1a18ada0c95fd41 Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 18:38:05 -0400 Subject: [PATCH 8/9] fix: a typoed ref and a non-reusable local review.yml both passed the audit ceab3dec from the bot's seventh review. A cross-repository call was accepted on any non-empty ref, so `@mian` for `@main` got a healthy verdict and a 404 at run time. `ref_names` lists the bot's branches and tags once per audit and `activation` checks the pinned ref against them. `known_refs=None` still means "we did not look", not "broken". A local `./.github/workflows/review.yml` was accepted merely for existing. `uses:` can only invoke a REUSABLE workflow, so the file must declare `workflow_call`; without it GitHub cannot call it at all. test_a_local_call_is_accepted_only_when_the_file_is_really_there used a stub with no `workflow_call`, so it asserted the looser behaviour. It now uses a reusable stub and points at the test that covers the rejection. d4b69cb7 NOT TAKEN, with reasons in the pull request. It is correct that a workflow already marked `disabled_inactivity` will not process a later push, so the push trigger narrows the window rather than closing it -- which is what the comment in the file already says rather than claims otherwise. Closing it needs a scheduler outside this repository, which this branch cannot add. Blocking the audit on that would leave no audit at all, which is strictly worse than one with a bounded and documented gap that only opens when the whole repository has been dormant for sixty days. Live fleet: all 7 report `will activate`, exit 0. 730 tests pass. --- reviewbot/callers.py | 45 ++++++++++++++++++++++++++++++++++--------- reviewbot/cli.py | 15 ++++++++++++++- reviewbot/github.py | 14 ++++++++++++++ tests/test_callers.py | 45 +++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 107 insertions(+), 12 deletions(-) diff --git a/reviewbot/callers.py b/reviewbot/callers.py index ee21fdb..0c457ad 100644 --- a/reviewbot/callers.py +++ b/reviewbot/callers.py @@ -114,7 +114,9 @@ def _as_names(value) -> list[str] | None: return None -def _calls_the_review_workflow(caller: dict, workflow_files: dict) -> bool: +def _calls_the_review_workflow( + caller: dict, workflow_files: dict, known_refs: set | None = None +) -> tuple[bool, str]: """Does any job in this caller invoke THIS bot's reusable workflow? A caller can carry a flawless `on:` block and still start nothing, so the @@ -127,6 +129,7 @@ def _calls_the_review_workflow(caller: dict, workflow_files: dict) -> bool: the file it names is actually present. """ jobs = caller.get("jobs") if isinstance(caller, dict) else None + problems = [] for job in (jobs or {}).values(): uses = job.get("uses") if isinstance(job, dict) else None if not isinstance(uses, str): @@ -137,15 +140,38 @@ def _calls_the_review_workflow(caller: dict, workflow_files: dict) -> bool: # only at the path accepted both illegal spellings. if "@" in uses: target, _, ref = uses.partition("@") - if target == REVIEW_WORKFLOW and ref: - return True - elif uses == LOCAL_REVIEW_WORKFLOW and "review.yml" in (workflow_files or {}): - return True - return False + if target != REVIEW_WORKFLOW or not ref: + continue + # A ref that does not resolve is a 404 at run time. `known_refs` + # is None when nobody looked, which is not the same as broken. + if known_refs is not None and ref not in known_refs: + problems.append(f"{uses} names ref {ref!r}, which does not resolve in the bot") + continue + return True, "" + if uses != LOCAL_REVIEW_WORKFLOW: + continue + local = (workflow_files or {}).get("review.yml") + if local is None: + problems.append(f"{uses} names a file that is not in .github/workflows") + continue + # A local file is only callable if it is REUSABLE. + try: + spec = yaml.safe_load(local) or {} + except yaml.YAMLError: + problems.append("review.yml could not be read") + continue + if "workflow_call" not in triggers(spec): + problems.append("review.yml does not declare workflow_call; `uses:` cannot invoke it") + continue + return True, "" + return False, problems[0] if problems else "" def activation( - caller_text: str | None, workflow_files: dict, states: dict | None = None + caller_text: str | None, + workflow_files: dict, + states: dict | None = None, + known_refs: set | None = None, ) -> Activation: """Will this caller start a review when a pull request's CI finishes? @@ -168,11 +194,12 @@ def activation( # A PERFECT TRIGGER THAT STARTS NOTHING. The caller must actually call the # reusable workflow; without this a file with the right `on:` and an # unrelated job reported healthy. - if not _calls_the_review_workflow(caller, workflow_files): + calls, why = _calls_the_review_workflow(caller, workflow_files, known_refs) + if not calls: return Activation( False, [], - [f"no job calls {REVIEW_WORKFLOW}; the trigger starts nothing"], + [why or f"no job calls {REVIEW_WORKFLOW}; the trigger starts nothing"], ) on = triggers(caller) diff --git a/reviewbot/cli.py b/reviewbot/cli.py index 7482808..24b942c 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -770,6 +770,17 @@ def audit_callers(repos: list, token: str) -> int: """ from reviewbot import callers as callers_mod + # Resolve the bot's own refs ONCE. A caller pinned at a typoed ref is a + # 404 at run time and would otherwise get a healthy verdict. None means we + # could not look, which `activation` treats as "not checked" rather than + # "broken". + known_refs = None + try: + bot = _store_api(callers_mod.REVIEW_WORKFLOW.split("/.github/")[0], token) + known_refs = bot.ref_names() + except Exception as exc: # noqa: BLE001 - a probe, never fatal + print(f"reviewbot: could not list the bot's refs ({scrub(str(exc))}); not checking them)") + results = {} unreadable = {} for repo in repos: @@ -782,7 +793,9 @@ def audit_callers(repos: list, token: str) -> int: except Exception as exc: # noqa: BLE001 - any failure to read is a failure unreadable[repo] = f"{type(exc).__name__}: {scrub(str(exc))}" continue - results[repo] = callers_mod.activation(files.get("code-review.yml"), files, states) + results[repo] = callers_mod.activation( + files.get("code-review.yml"), files, states, known_refs + ) print(f"Code review activation, {len(repos)} repositor{'y' if len(repos) == 1 else 'ies'}\n") if results: diff --git a/reviewbot/github.py b/reviewbot/github.py index a7d4db7..b73139b 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -331,6 +331,20 @@ def workflow_states(self) -> dict: return out page += 1 + def ref_names(self) -> set: + """Every branch and tag name in this repository. + + A caller pins the reusable workflow at a ref. A typoed one is a 404 + when the review tries to run and looks perfectly healthy on disk, so + the auditor resolves them. + """ + out = set() + for kind in ("branches", "tags"): + for item in self._paged(f"/repos/{self.repo}/{kind}") or []: + if isinstance(item, dict) and item.get("name"): + out.add(item["name"]) + return out + def pull_for_sha(self, sha: str) -> int | None: """The open pull request whose head is `sha`, or None. diff --git a/tests/test_callers.py b/tests/test_callers.py index bc361f8..e62d152 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -673,8 +673,11 @@ def test_a_local_call_is_accepted_only_when_the_file_is_really_there(): "MarketData-App/code-review-bot/.github/workflows/review.yml@main", "./.github/workflows/review.yml", ) - # Present: this is self-review.yml's shape. - ok = callers.activation(caller, {"t.yml": TESTS_WORKFLOW, "review.yml": "name: Review\n"}) + # Present AND reusable: this is self-review.yml's shape. A local file that + # does not declare `workflow_call` cannot be invoked by `uses:` at all, + # which is covered by test_a_local_review_workflow_without_workflow_call_is_rejected. + reusable = "name: Review\non:\n workflow_call:\njobs:\n r:\n runs-on: x\n" + ok = callers.activation(caller, {"t.yml": TESTS_WORKFLOW, "review.yml": reusable}) assert ok.ok is True, ok.reasons # Absent: it calls a file that does not exist. assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is False @@ -736,3 +739,41 @@ def test_the_audit_also_runs_on_a_push_to_the_default_branch(audit_workflow): on = audit_workflow.get(True) or audit_workflow.get("on") assert set(on) == {"schedule", "push"} assert on["push"]["branches"] == ["main"] + + +# --- round seven ----------------------------------------------------------- + + +LOCAL_CALLER = ARMED.replace( + "MarketData-App/code-review-bot/.github/workflows/review.yml@main", + "./.github/workflows/review.yml", +) +REUSABLE = "name: Review\non:\n workflow_call:\n inputs: {}\njobs:\n r:\n runs-on: x\n" + + +def test_a_local_review_workflow_without_workflow_call_is_rejected(): + # ceab3dec. A local file that is not reusable cannot be invoked by `uses:`. + files = {"t.yml": TESTS_WORKFLOW, "review.yml": "name: Review\non:\n push:\n"} + assert callers.activation(LOCAL_CALLER, files).ok is False + + +def test_a_local_review_workflow_exposing_workflow_call_is_accepted(): + files = {"t.yml": TESTS_WORKFLOW, "review.yml": REUSABLE} + got = callers.activation(LOCAL_CALLER, files) + assert got.ok is True, got.reasons + + +def test_a_cross_repository_ref_that_does_not_resolve_is_rejected(): + # A typoed ref is a 404 at run time and a healthy verdict here. + caller = ARMED.replace("review.yml@main", "review.yml@mian") + got = callers.activation(ARMED, {"t.yml": TESTS_WORKFLOW}, known_refs={"main", "v1"}) + assert got.ok is True, got.reasons + bad = callers.activation(caller, {"t.yml": TESTS_WORKFLOW}, known_refs={"main", "v1"}) + assert bad.ok is False + assert any("mian" in r for r in bad.reasons) + + +def test_refs_are_not_checked_when_they_were_not_resolved(): + # `known_refs=None` means we did not look, which is not "it is broken". + caller = ARMED.replace("review.yml@main", "review.yml@whatever") + assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is True From b192a8af0b961c967d1d993e65fd8243d5f91f30 Mon Sep 17 00:00:00 2001 From: MarketDataApp Date: Tue, 22 Sep 2026 18:45:37 -0400 Subject: [PATCH 9/9] fix: the user-account audit silently skipped ref validation entirely Three findings from the bot's eighth review taken; the security one declined and escalated to the maintainer. 352adc93 (blocking) A bug of exactly the kind this command exists to catch. The user-account audit resolved each caller's pinned ref against the ORGANISATION-owned bot repository using the USER installation token, which cannot read it. The failure was caught and converted to "refs not checked", so ref validation was silently off for all six SDK repositories while the audit still printed green. REVIEWBOT_BOT_TOKEN now carries a credential for the bot repository, the workflow passes the org token there, and a failure to resolve fails that repository's audit instead of being swallowed. The org token's scope gains code-review-bot, asserted by test. 32046223 (blocking) A branch existing does not mean review.yml exists on it, and listing branches and tags rejected valid immutable commit-SHA pins. The resolver now asks "is the workflow file there at this ref?", which answers both and is cached per ref. 045105fb (should fix) README said the workflow "runs it daily", which overstates it: a workflow already disabled for inactivity does not process the later push. The README now states the gap and that it is accepted deliberately, rather than implying coverage that is not there. f920722d NOT TAKEN here. It is correct on the merits and is a repository SETTINGS change -- protecting `main` -- not a code change this branch can make. It describes an exposure shared with every existing workflow in this repository: self-review.yml already passes the same key to default-branch code through `secrets: inherit`. Raised with the maintainer for a decision. Live fleet: all 7 report `will activate`, exit 0. 733 tests pass. --- .github/workflows/audit-callers.yml | 15 +++- README.md | 12 ++- reviewbot/callers.py | 18 +++-- reviewbot/cli.py | 41 ++++++---- tests/test_callers.py | 111 +++++++++++++++++++++++++--- 5 files changed, 163 insertions(+), 34 deletions(-) diff --git a/.github/workflows/audit-callers.yml b/.github/workflows/audit-callers.yml index 02064bf..bd415a6 100644 --- a/.github/workflows/audit-callers.yml +++ b/.github/workflows/audit-callers.yml @@ -104,7 +104,13 @@ jobs: # Scoped, not owner-wide. `owner:` alone mints a token for every # repository in the installation; the audit needs only what it # audits, and a test asserts this list matches ORG_REPOS. - repositories: api + # + # code-review-bot is here as well as `api` because BOTH audits + # resolve each caller's pinned ref against this repository's + # .github/workflows/review.yml. + repositories: >- + api + code-review-bot - name: Token for the MarketDataApp user account id: user @@ -140,3 +146,10 @@ jobs: run: uv run reviewbot audit-callers $USER_REPOS env: GITHUB_TOKEN: ${{ steps.user.outputs.token }} + # THE BOT REPOSITORY NEEDS THE ORG TOKEN. An installation token + # reaches only its own account, so the user-account token above + # cannot read MarketData-App/code-review-bot -- and the audit + # resolves each caller's pinned ref against it. Without this the ref + # check failed for all six SDK repositories and the audit still + # reported green. + REVIEWBOT_BOT_TOKEN: ${{ steps.org.outputs.token }} diff --git a/README.md b/README.md index ae0a783..101c3b2 100644 --- a/README.md +++ b/README.md @@ -143,8 +143,16 @@ GITHUB_TOKEN=$(gh auth token) uv run reviewbot audit-callers MarketDataApp/sdk-p It exits non-zero when any repository will not start a review, and says why: a missing or commented-out `workflow_run`, a name matching no workflow, a workflow that does not run on pull requests, or a repository it could not -read. `.github/workflows/audit-callers.yml` runs it daily over the whole -fleet. +read. `.github/workflows/audit-callers.yml` runs it over the whole fleet on a +daily schedule and on every push to `main`. + +**One monitoring gap, accepted deliberately.** GitHub disables a scheduled +workflow after 60 days with no activity in its repository, and a workflow +already disabled that way does not process a later push -- so the push trigger +narrows the window rather than closing it. Closing it properly needs a +scheduler outside this repository. The gap opens only when this repository has +been dormant for 60 days, during which a target repository could still drift +unnoticed. The target list there is explicit, not discovered. Discovery answers "which repositories have a caller?", so one that LOST its caller would drop quietly diff --git a/reviewbot/callers.py b/reviewbot/callers.py index 0c457ad..f9cfb1b 100644 --- a/reviewbot/callers.py +++ b/reviewbot/callers.py @@ -115,7 +115,7 @@ def _as_names(value) -> list[str] | None: def _calls_the_review_workflow( - caller: dict, workflow_files: dict, known_refs: set | None = None + caller: dict, workflow_files: dict, ref_resolver=None ) -> tuple[bool, str]: """Does any job in this caller invoke THIS bot's reusable workflow? @@ -142,10 +142,14 @@ def _calls_the_review_workflow( target, _, ref = uses.partition("@") if target != REVIEW_WORKFLOW or not ref: continue - # A ref that does not resolve is a 404 at run time. `known_refs` - # is None when nobody looked, which is not the same as broken. - if known_refs is not None and ref not in known_refs: - problems.append(f"{uses} names ref {ref!r}, which does not resolve in the bot") + # A ref whose review.yml is missing is a 404 at run time. Asking + # "is the FILE there at this ref?" rather than "is this a known + # branch?" also accepts an immutable commit-SHA pin, which appears + # in no branch or tag listing. `None` means nobody looked. + if ref_resolver is not None and not ref_resolver(ref): + problems.append( + f"{uses} names ref {ref!r}, where the review workflow cannot be found" + ) continue return True, "" if uses != LOCAL_REVIEW_WORKFLOW: @@ -171,7 +175,7 @@ def activation( caller_text: str | None, workflow_files: dict, states: dict | None = None, - known_refs: set | None = None, + ref_resolver=None, ) -> Activation: """Will this caller start a review when a pull request's CI finishes? @@ -194,7 +198,7 @@ def activation( # A PERFECT TRIGGER THAT STARTS NOTHING. The caller must actually call the # reusable workflow; without this a file with the right `on:` and an # unrelated job reported healthy. - calls, why = _calls_the_review_workflow(caller, workflow_files, known_refs) + calls, why = _calls_the_review_workflow(caller, workflow_files, ref_resolver) if not calls: return Activation( False, diff --git a/reviewbot/cli.py b/reviewbot/cli.py index 24b942c..37b49f8 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -770,16 +770,24 @@ def audit_callers(repos: list, token: str) -> int: """ from reviewbot import callers as callers_mod - # Resolve the bot's own refs ONCE. A caller pinned at a typoed ref is a - # 404 at run time and would otherwise get a healthy verdict. None means we - # could not look, which `activation` treats as "not checked" rather than - # "broken". - known_refs = None - try: - bot = _store_api(callers_mod.REVIEW_WORKFLOW.split("/.github/")[0], token) - known_refs = bot.ref_names() - except Exception as exc: # noqa: BLE001 - a probe, never fatal - print(f"reviewbot: could not list the bot's refs ({scrub(str(exc))}); not checking them)") + REVIEW_WORKFLOW_PATH = ".github/workflows/review.yml" + + # THE BOT REPOSITORY NEEDS ITS OWN CREDENTIAL. An installation token + # reaches only its own account, so the user-account audit cannot read the + # organisation-owned bot repository with the token it audits SDKs with. + # That failure used to be swallowed into "refs not checked", silently + # disabling the check for all six SDK repositories while still reporting + # green -- precisely the failure this command exists to catch. + bot_repo = callers_mod.REVIEW_WORKFLOW.split("/.github/")[0] + bot_token = os.environ.get("REVIEWBOT_BOT_TOKEN") or token + bot_api = _store_api(bot_repo, bot_token) + resolved: dict = {} + + def ref_resolver(ref: str) -> bool: + """Is the reusable workflow present at this ref? Cached per ref.""" + if ref not in resolved: + resolved[ref] = bool(bot_api.file_at_ref(REVIEW_WORKFLOW_PATH, ref)) + return resolved[ref] results = {} unreadable = {} @@ -793,9 +801,16 @@ def audit_callers(repos: list, token: str) -> int: except Exception as exc: # noqa: BLE001 - any failure to read is a failure unreadable[repo] = f"{type(exc).__name__}: {scrub(str(exc))}" continue - results[repo] = callers_mod.activation( - files.get("code-review.yml"), files, states, known_refs - ) + try: + results[repo] = callers_mod.activation( + files.get("code-review.yml"), files, states, ref_resolver + ) + except Exception as exc: # noqa: BLE001 - unresolvable refs are a failure + unreadable[repo] = ( + f"could not resolve the bot's refs ({type(exc).__name__}: " + f"{scrub(str(exc))}); set REVIEWBOT_BOT_TOKEN to a token that can " + f"read {bot_repo}" + ) print(f"Code review activation, {len(repos)} repositor{'y' if len(repos) == 1 else 'ies'}\n") if results: diff --git a/tests/test_callers.py b/tests/test_callers.py index e62d152..4b90dc2 100644 --- a/tests/test_callers.py +++ b/tests/test_callers.py @@ -204,6 +204,10 @@ def workflow_files(self): def workflow_states(self): return {name: "active" for name in self._files.get(self.repo, {})} + def file_at_ref(self, path, ref): + # The bot repository is not in `files`; the ref probe resolves anyway. + return "name: Review\non:\n workflow_call:\n" + def _cli(monkeypatch, files): from reviewbot import cli @@ -249,6 +253,9 @@ def workflow_files(self): def workflow_states(self): return {} + def file_at_ref(self, path, ref): + return "name: Review\non:\n workflow_call:\n" + monkeypatch.setattr(cli, "_store_api", lambda repo, token: Boom()) # Unreadable is not the same as healthy. An audit that shrugs at a 404 # would report a deleted caller as fine. @@ -573,9 +580,24 @@ def test_the_tokens_are_scoped_to_the_audited_repositories(audit_workflow): env = audit_workflow["env"] for owner, key in (("MarketData-App", "ORG_REPOS"), ("MarketDataApp", "USER_REPOS")): wanted = {r.split("/", 1)[1] for r in env[key].split()} + if owner == "MarketData-App": + # Both audits resolve each caller's pinned ref against the bot's + # own review.yml, so the org token must reach it too. + wanted.add("code-review-bot") assert scoped[owner] == wanted, f"{owner} token scope does not match {key}" +def test_the_user_audit_gets_an_org_token_for_the_bot_repository(audit_workflow): + # 352adc93. Without this the ref check failed for every SDK repository and + # the audit still reported green. + steps = audit_workflow["jobs"]["audit"]["steps"] + user_step = [s for s in steps if s.get("id") == "audit-user"][0] + org_step = [s for s in steps if s.get("id") == "audit-org"][0] + assert "REVIEWBOT_BOT_TOKEN" in user_step["env"] + assert user_step["env"]["REVIEWBOT_BOT_TOKEN"] != user_step["env"]["GITHUB_TOKEN"] + assert user_step["env"]["REVIEWBOT_BOT_TOKEN"] == org_step["env"]["GITHUB_TOKEN"] + + def test_a_caller_with_no_job_calling_the_review_workflow_is_reported(): # 585655c7. The trigger can be perfect and still start nothing. no_job = ARMED.replace( @@ -763,17 +785,84 @@ def test_a_local_review_workflow_exposing_workflow_call_is_accepted(): assert got.ok is True, got.reasons -def test_a_cross_repository_ref_that_does_not_resolve_is_rejected(): - # A typoed ref is a 404 at run time and a healthy verdict here. - caller = ARMED.replace("review.yml@main", "review.yml@mian") - got = callers.activation(ARMED, {"t.yml": TESTS_WORKFLOW}, known_refs={"main", "v1"}) - assert got.ok is True, got.reasons - bad = callers.activation(caller, {"t.yml": TESTS_WORKFLOW}, known_refs={"main", "v1"}) - assert bad.ok is False - assert any("mian" in r for r in bad.reasons) - - def test_refs_are_not_checked_when_they_were_not_resolved(): - # `known_refs=None` means we did not look, which is not "it is broken". + # `ref_resolver=None` means we did not look, which is not "it is broken". caller = ARMED.replace("review.yml@main", "review.yml@whatever") assert callers.activation(caller, {"t.yml": TESTS_WORKFLOW}).ok is True + + +# --- round eight ----------------------------------------------------------- + + +def test_a_ref_is_accepted_only_when_the_workflow_exists_at_it(): + # 32046223. A branch existing does not mean review.yml exists on it, and + # an immutable SHA pin is valid but appears in no branch or tag list. The + # resolver answers "is the file there at this ref?", which covers both. + seen = [] + + def resolver(ref): + seen.append(ref) + return ref in ("main", "deadbee") + + assert callers.activation(ARMED, {"t.yml": TESTS_WORKFLOW}, ref_resolver=resolver).ok is True + sha = ARMED.replace("review.yml@main", "review.yml@deadbee") + assert callers.activation(sha, {"t.yml": TESTS_WORKFLOW}, ref_resolver=resolver).ok is True + old = ARMED.replace("review.yml@main", "review.yml@v0") + got = callers.activation(old, {"t.yml": TESTS_WORKFLOW}, ref_resolver=resolver) + assert got.ok is False + assert any("v0" in r for r in got.reasons) + assert "main" in seen and "deadbee" in seen and "v0" in seen + + +def test_the_audit_fails_when_the_bots_refs_cannot_be_resolved(monkeypatch, capsys): + # 352adc93, and a bug of exactly the kind this tool exists to catch. The + # user-account audit resolved refs against the ORG-owned bot repository + # using the USER installation token, which cannot read it. The failure was + # caught and turned into "not checked", silently disabling ref validation + # for all six SDK repositories while the audit still reported green. + from reviewbot import cli + from reviewbot.github import GitHubError + + class Api: + def __init__(self, repo): + self.repo = repo + + def workflow_files(self): + return {"code-review.yml": ARMED, "t.yml": TESTS_WORKFLOW} + + def workflow_states(self): + return {"code-review.yml": "active", "t.yml": "active"} + + def file_at_ref(self, path, ref): + raise GitHubError("GET /contents failed with 404: Not Found") + + monkeypatch.setattr(cli, "_store_api", lambda repo, token: Api(repo)) + assert cli.audit_callers(["o/a"], token="t") == 1 + assert "ref" in capsys.readouterr().out.lower() + + +def test_the_bot_repository_can_be_read_with_its_own_token(monkeypatch): + # The fix: a separate credential for the bot repository, so the user-account + # audit resolves refs with something that can actually read it. + from reviewbot import cli + + used = {} + + class Api: + def __init__(self, repo, token): + self.repo, self.token = repo, token + + def workflow_files(self): + return {"code-review.yml": ARMED, "t.yml": TESTS_WORKFLOW} + + def workflow_states(self): + return {"code-review.yml": "active", "t.yml": "active"} + + def file_at_ref(self, path, ref): + used["token"] = self.token + return "name: Review\non:\n workflow_call:\n" + + monkeypatch.setattr(cli, "_store_api", lambda repo, token: Api(repo, token)) + monkeypatch.setenv("REVIEWBOT_BOT_TOKEN", "org-token") + assert cli.audit_callers(["o/a"], token="user-token") == 0 + assert used["token"] == "org-token"