Repository navigation
feat: assert that every target repository will actually start a review - #21
Conversation
`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 <repo>...` 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.
⛔ Blocked — Code reviewThis change adds a CLI and scheduled workflow that audit whether configured repositories can trigger code reviews, with API-backed state checks and extensive tests. The pinned-ref validation only proves that Rating patch 3/6 (incomplete) · proof 5/6 (reproducible) · overall 3/6 Before merge
FindingsBlocking
Nit
Since last review
Reviewed |
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`.
|
The
If that rule is wanted it belongs in Live fleet re-checked after the shape changes: all 7 still report |
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.
|
Nothing in the fleet uses either filter today, and all 7 still report
A per-pull-request nit cannot settle a whole-repository convention — it can only ask each new file to differ from the files around it. If the rule is wanted, it belongs in |
…ot 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.
|
And one neither of us caught, which the live fleet did. My first Live fleet after all three fixes: all 7 report
|
…ccepted 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 <repos...>`. 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.
|
Now
Live fleet after all three: all 7 report |
…orkflows
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.
|
I removed Live fleet: all 7 report |
…itch 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.
|
I have not claimed this solves the inactive-repository case, and the comment in the file says so: if nothing is pushed here for 60 days, the schedule stops and no push replaces it. What it does buy is that the audit cannot be quietly off while anyone is working, and drift is reported at merge time rather than the next morning. I also restated the security test as the property that actually matters — no trigger may let the caller choose the ref — rather than "schedule only", since
You are right that a writer can push a secret-reading step to The remedy you name — protect I have raised branch protection with the maintainer as a separate action. Live fleet: all 7 report |
… 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.
|
Closing it properly needs a scheduler outside this repository, which this branch cannot add. The residual gap opens only when the entire repository has been dormant for sixty days — a state in which the fleet is not changing either. Weighed against that, blocking this pull request leaves no audit at all, which is strictly worse than one with a bounded and documented gap. If an external heartbeat is wanted, it is worth doing as its own change, where it can cover every scheduled workflow here rather than this one. Live fleet: all 7 report |
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.
A caller's
on.workflow_run.workflowslists workflowname: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.Two live instances, both found while arming the five held-back SDK repos:
workflows: ["Tests", "Lint"]into repositories with no workflow namedLint. Reviews ran viaTests, so the dead half survived from installation until now.sdk-js's pull-request workflow was once namedCI,sdk-java'sPull Request. Both areTeststoday. Renaming either back switches that repo's reviews off in silence.reviewbot audit-callersNegative control:
It detects: a missing or commented-out
workflow_run,typeswithoutcompleted, an emptyworkflowslist, 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.
Design
reviewbot/callers.pyis pure — caller text and workflow files in, verdict out. The default test run keeps its promise of no token and no socket;cli.audit_callersdoes the fetching.The scheduled workflow runs daily over both installations, one step per owner because an org installation token cannot read the user account's repositories, with
if: always()on the second so one bad repository cannot hide the rest.The target list is explicit, not 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
31 new, TDD, each watched failing first.
688 passed, ruff clean. Includes guards on the audit itself — an audit that checks nothing would pass vacuously — asserting the list covers every SDK repo, holds no duplicates, omitscode-review-bot(which usesself-review.ymland has nocode-review.yml), runs on a schedule, and uses a distinct token per owner.One
on:subtlety is pinned by test:onis a YAML 1.1 boolean, so PyYAML returns the keyTrue, not"on". Reading only"on"would have reported the entire fleet as broken.