ci: route review requests with CODEOWNERS - #600
Conversation
Branch protection decides whether a pull request may merge; nothing decided who should look at it. Requesting the mapped team is documented in divine-context PR_REVIEW.md, but it is a step the author has to remember, and a skipped request is indistinguishable from a completed one. This repository had 9 authors and 128 commits in the last 90 days, the widest contributor spread of any divine repo, so it is where unrouted review costs the most. CODEOWNERS is applied by GitHub when the pull request opens, needs no author or agent action, and treats both identically. wrangler.toml routes to @divinevideo/platform because it carries the Worker's routes, bindings and environment: a wrong final state there misdirects live traffic rather than breaking one feature. Only paths that exist here are listed, since a pattern matching nothing is silently inert, as is a team without write access. Not paired with require_code_owner_reviews yet: let it request for a week before it can block a merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deploying divine-web with
|
| Latest commit: |
9b53626
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d0fde25a.divine-web.pages.dev |
| Branch Preview URL: | https://ci-codeowners.divine-web.pages.dev |
🚀 Preview DeploymentLast updated:
|
realmeylisdev
left a comment
There was a problem hiding this comment.
The catch-all works. The three @divinevideo/platform rules don't — GitHub already flags every one of them:
$ gh api "repos/divinevideo/divine-web/codeowners/errors?ref=ci/codeowners"
Unknown owner on line 19: /.github/workflows/ @divinevideo/platform
Unknown owner on line 20: /wrangler.toml @divinevideo/platform
Unknown owner on line 23: /.github/CODEOWNERS @divinevideo/platform
The team has write access to 32 repos, including every repo in the PR_REVIEW_TEAMS.md mapping — but not divine-web, and not divine-mobile. Per GitHub's docs, a code-owner team "must be visible and it must have write permissions, even if all the individual members of the team already have write permissions directly."
So * @divinevideo/reviewers is the only line with any effect, and the comments around it describe routing that isn't happening. This is failure mode #1 from your own description — live rather than hypothetical.
Your call which way to fix it: grant platform write access here, or drop the three lines until it has it. Either way, /codeowners/errors returning {"errors":[]} is the check — and since it takes a ref, it works before merge, which beats the throwaway PR in After merge.
Two smaller things inline: wrangler.toml doesn't carry what its comment claims, and the header's "GitHub reports neither" is wrong for the team case.
divinevideo/divine-mobile#7599 shipped the same file this morning and has the same four Unknown owner errors, so one access grant would fix both repos.
|
Concur with realmeylisdev's review. Re: the line-20 question: an owner without write access doesn't fall back to
|
@divinevideo/platform has no write access to divine-web, so all three lines naming it were inert — GitHub reports each as Unknown owner at codeowners/errors — and divine-web is not in the per-repo platform mapping in PR_REVIEW_TEAMS.md, so the default reviewer team owns routing here. Header corrected: an unknown-owner line is reported, not silent, and the check works pre-merge via codeowners/errors?ref=. Platform routing guidance kept for whoever adds it once access exists, pointing at compute-js/ (the Fastly edge config) rather than wrangler.toml.
dcadenas
left a comment
There was a problem hiding this comment.
✅ The catch-all is the line that works, and after this push it is also the whole file.
Changes I pushed
- Dropped the three
@divinevideo/platformlines. The team has no write access to divine-web, so GitHub reported each as an unknown owner and they routed no one. With the last match winning, those paths resolved to nobody instead of falling back to the catch-all. - Corrected the file header: an unknown-owner line is reported, on the file's page and at the
codeowners/errorsendpoint, not silent. The endpoint takes aref, so it works on a branch before merge. - Kept the guidance for re-adding platform routing once the team has write access, naming
compute-js/as the edge config that earns the live-traffic rationale.wrangler.tomlhere is a three-key Cloudflare Pages build config. - Updated the PR description to match the file as it now stands.
Checks
gh api repos/divinevideo/divine-web/codeowners/errors?ref=ci/codeownersreturns{"errors":[]}on the pushed head.- The repo's declared checks are all CI-only and none of them read
.github/CODEOWNERS; they were still running on this push when this review was posted.
Follow-ups
- Platform routing here needs an org-level grant of write access for
@divinevideo/platform. divinevideo/divine-mobile#7599 merged the same file and itsmainnow shows the same four unknown-owner errors, so one access grant would fix both repos. That decision is outside this PR. - @realmeylisdev the fix you asked for is commit 9b53626; your review is the open change request on this head.
realmeylisdev
left a comment
There was a problem hiding this comment.
Verified on 9b53626. All three change-request items are resolved.
- The three
@divinevideo/platformlines are gone. Re-ran the check on this head rather than reading it off the earlier comment:gh api "repos/divinevideo/divine-web/codeowners/errors?ref=ci/codeowners"returns{"errors":[]}. - The header no longer claims the unknown-owner case is silent, and it now carries the endpoint plus the
refparameter, so the check is runnable before merge instead of after. - The
wrangler.tomlrouting line is gone, andcompute-js/is named as the path that would earn platform routing. Confirmed againstcompute-js/fastly.toml: a liveservice_id, two[[backends]]pointing atrelay.divine.video, and KV plus secret stores.
Also checked: divine-web is absent from the PR_REVIEW_TEAMS.md mapping, so the catch-all to @divinevideo/reviewers is the correct default, and all five checks pass on this head.
The question I could not settle — whether a last-matching rule with an unknown owner is skipped or wins with zero owners — is moot now that the catch-all is the whole file. It only needs answering if platform routing comes back.
One wording nit left, inline as a suggestion: line 22 says three-line, but wrangler.toml is six lines and three keys. The PR description already says "three-key". It is a suggestion rather than a commit because .github/CODEOWNERS is review-routing, and PR_REVIEW_TEAMS.md restricts direct pushes on permission-boundary paths to @divinevideo/platform members; I am not one.
Not blocking, so this supersedes my change request.
One note for the After merge check: codeowners/errors?ref=main returns 404 today because the file is not on main yet. It should start returning {"errors":[]} once this lands — the 404 is the pre-merge state, not a failure.
| # If platform routing is added later, the team needs write access here first | ||
| # (the "Unknown owner" check above covers that), and compute-js/ — the Fastly | ||
| # edge config with the live service id, backends, KV and secret stores — is | ||
| # the path that earns it. wrangler.toml is a three-line Cloudflare Pages |
There was a problem hiding this comment.
wrangler.toml is six lines and three keys — the two ABOUTME comments and a blank line come first. "three-key" is what the PR description says, and it is the accurate half.
| # the path that earns it. wrangler.toml is a three-line Cloudflare Pages | |
| # the path that earns it. wrangler.toml is a three-key Cloudflare Pages |
Summary
Adds
.github/CODEOWNERSso review requests are routed automatically when a pull request opens.Why here first
9 authors, 128 commits in the last 90 days — the widest contributor spread of any divine repo, so unrouted review costs the most here.
Branch protection decides whether a pull request may merge. Nothing decided who should look, so pull requests wait on someone noticing. Requesting the mapped team is documented in
divine-context/PR_REVIEW.md, but it is a step the author must remember, and a skipped request looks exactly like a completed one.CODEOWNERS is applied by GitHub the moment the PR opens — no author action, no agent action, identical for both.
Routing
divine-web is not in the per-repo mapping in
PR_REVIEW_TEAMS.md, so the default reviewer team owns the whole repository.An earlier revision of this PR also routed
.github/workflows/,wrangler.tomland this file to@divinevideo/platform. That team has no write access to divine-web, so GitHub reported all three lines as unknown owners and they routed no one. If platform routing is added later, the team needs write access here first.compute-js/— the Fastly edge config carrying the live service id, backends, KV and secret stores — is the path that earns the "misdirects live traffic" rationale.wrangler.tomlin this repo is a three-key Cloudflare Pages build config (name, output dir, compatibility date).Two failure modes worth a reviewer's eye
/repos/divinevideo/divine-web/codeowners/errors, which takes aref, so a branch can be checked before merge.Last matching pattern wins, so the catch-all is deliberately first.
On this branch the errors endpoint returns
{"errors":[]}.Not included
require_code_owner_reviewson branch protection, which would make these approvals mandatory. Worth doing after a week of this simply requesting — if the routing is wrong, the failure should be "wrong person asked", not "nobody can merge".After merge
Confirm routing two ways: the errors endpoint above returning empty on
main, and a throwaway PR to see reviewers appear unprompted. The first works pre-merge and catches owner problems; the second demonstrates the request behavior.