diff --git a/.github/workflows/audit-callers.yml b/.github/workflows/audit-callers.yml new file mode 100644 index 0000000..bd415a6 --- /dev/null +++ b/.github/workflows/audit-callers.yml @@ -0,0 +1,155 @@ +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 * * *" + # 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. +# +# 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. +# +# 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. +# +# 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 + # 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. + # + # 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 + 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 + 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 + # 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 }} + # 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 e62f1f6..101c3b2 100644 --- a/README.md +++ b/README.md @@ -129,6 +129,35 @@ 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 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 +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..f9cfb1b --- /dev/null +++ b/reviewbot/callers.py @@ -0,0 +1,332 @@ +"""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 + +# 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: + """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_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. + """ + found = {} + for filename, text in (files or {}).items(): + 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): + # 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 + + +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 _calls_the_review_workflow( + caller: dict, workflow_files: dict, ref_resolver=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 + 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. + + 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 + problems = [] + for job in (jobs or {}).values(): + uses = job.get("uses") if isinstance(job, dict) else None + if not isinstance(uses, str): + continue + # 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 or not ref: + continue + # 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: + 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, + ref_resolver=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"]) + try: + caller = yaml.safe_load(caller_text) + 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. + calls, why = _calls_the_review_workflow(caller, workflow_files, ref_resolver) + if not calls: + return Activation( + False, + [], + [why or f"no job calls {REVIEW_WORKFLOW}; the trigger starts nothing"], + ) + + 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] = [] + + # 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") + + # 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 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, and only ['**'] matches every branch (`*` does not cross `/`)" + ) + 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" + ) + + # A DISABLED CALLER runs nothing while looking perfectly correct on disk. + # + # `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 = [] + for name in wanted: + 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 + if states is not None: + # 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) + + 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..37b49f8 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -757,6 +757,77 @@ 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 + + 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 = {} + for repo in repos: + try: + 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 + 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: + 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 +993,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 +1040,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..b73139b 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -274,6 +274,77 @@ 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 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. + 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 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 new file mode 100644 index 0000000..4b90dc2 --- /dev/null +++ b/tests/test_callers.py @@ -0,0 +1,868 @@ +"""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 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 + + 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") + + 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. + 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()" + + +# --- 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 + + +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 + + +# --- 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(): + # `["*"]` 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(): + 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 + + +# --- 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&page=1", + 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&page=1", data=["not", "an", "object"] + ) + assert GitHub("o/r", "t", transport=transport).workflow_states() == {} + + +# --- round four ------------------------------------------------------------ + + +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", "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): + # 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()} + 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( + " 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_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" + + +# --- 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 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 + + +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 + + +# --- 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"] + + +# --- 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_refs_are_not_checked_when_they_were_not_resolved(): + # `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"