Add github action to verify compat flags have docs#6518
Conversation
26351b1 to
5d20d94
Compare
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds a GitHub Action that verifies new compat flags with default-on dates have corresponding documentation in cloudflare-docs. The Python script parses compatibility-date.capnp, diffs against the base branch, and uses gh CLI to check for docs on the production branch or in approved open PRs. A test flag (justATestThisWontBeLanded) is included to exercise the check.
Findings (highest severity first):
-
[HIGH]
github.tokenlacks cross-repo read access — The defaultGITHUB_TOKENis scoped tocloudflare/workerd. Calls togh api repos/cloudflare/cloudflare-docs/contents/...andgh search prs --repo cloudflare/cloudflare-docswill likely 404 or 403 because the token has no read permission oncloudflare/cloudflare-docs. You'll need a PAT or a GitHub App token with cross-repocontents:readscope. If this already works due to an org-level policy, a comment explaining that would be helpful. -
[MED]
merge_groupevent setsGITHUB_BASE_SHAto empty string — Formerge_groupevents,github.event.pull_request.base.shais not populated and will evaluate to an empty string. The script'sresolve_base_sha()correctly falls back throughGITHUB_EVENT_PATHandgit merge-base, but passing an empty-string env var means the first branch (os.environ.get("GITHUB_BASE_SHA")) returns""which is truthy in Python, sogit show :src/workerd/io/compatibility-date.capnpruns (no commit ref) and the fallback never triggers. -
[MED] Script collects
experimentalstatus but never uses it —parse_compat_flagsrecords"experimental": is_experimentalfor each flag, but this value is never checked. Should experimental flags be exempt from the documentation requirement? If so, the check should skip them; if not, the field should be removed to avoid confusion. -
[LOW]
$compatEnableAllDatesis not detected — The script checks for$compatEnableDateand$impliedByAfterDateas triggers for requiring docs, but$compatEnableAllDates(which makes a flag default-on for all dates) is not handled. Currently only one flag uses it (r2PublicBetaApi), so this is low-impact, but worth noting for completeness. -
[LOW] Missing
permissionsblock in workflow — CodeQL already flagged this. Adding an explicitpermissionsblock (e.g.contents: read) is best practice for least-privilege.
This review was generated by an AI assistant and may contain inaccuracies.
|
I'm Bonk, and I've done a quick review of your PR. Review posted on PR #6518 with 5 findings and inline comments with suggestions:
|
|
I thought the idea sounded familiar ;-) ... I'm good with either one landing. |
harrishancock
left a comment
There was a problem hiding this comment.
Looks reasonable to me, though Bonk seems to have some smart comments.
No description provided.