fix(pr-policy): grant required permissions + harden parser load (F2/F10 hotfix)#22
Merged
Merged
Conversation
When `permissions:` is declared on a job, every unlisted scope becomes `none`. The evidence-check step calls `checks.listForRef` (needs `checks: read`) and `git.getCommit` (needs `contents: read`), both of which would 403 against any consumer repo with the previous `pull-requests: read` only block. Refs #10 #12
Dynamic `import()` of `scripts/parse-evidence.mjs` previously had no error handling — a missing parser (wrong `workflow-library-ref`, skipped checkout) would crash the step even though Phase 1 is warning-only. Degrade cleanly to a job-summary warning and return. Even when `enforce-evidence` is true we still `core.warning` rather than `core.setFailed` because a missing parser is an ecosystem config problem, not a PR-author problem. Refs #10 #12
Draft PRs early-return in the evidence-check step, so cloning github-workflows just to dynamic-import a parser that never runs is pure waste (~5-10s per draft run). Gate the checkout on `github.event.pull_request.draft == false`. Doc-only PRs still pay the checkout cost because doc-only detection requires an API call we can't make at `if:`-evaluation time. Refs #10 #12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hotfix for three latent bugs introduced by PR #19 (
d0a5fed) that would have caused ecosystem-wide red-CI on first consumer adoption of the F2/F10 evidence parser. Parser logic itself is unchanged — only the workflow wiring around it.Refs #10 #12
Closes C2, M1, M2 from
docs/phase2/f2-f10-rollout-followups.md(in the docs root). C1 (re-cutv1tag) and C3 (integration test) are deferred to the next rollout steps and explicitly out of scope here.Summary
policy:job in.github/workflows/pr-policy.ymldeclaredpermissions:with onlypull-requests: read. Because declaringpermissions:collapses every unlisted scope tonone, the new evidence-check step would 403 onchecks.listForRefandgit.getCommit. Now grantspull-requests: read,checks: read,contents: read. Caller exampleexamples/pr-policy.ymlalso gainschecks: readwith a comment explaining the parser dependency.import()ofscripts/parse-evidence.mjsis now wrapped in try/catch. On failure (missing parser, wrongworkflow-library-ref, skipped checkout) the step writes a job-summary warning and returns. Even withenforce-evidence: truewe emitcore.warningrather thancore.setFailedbecause a missing parser is an ecosystem config problem, not a PR-author problem.github.event.pull_request.draft == false. Draft PRs skip the ~5-10s clone since the evidence step early-returns for them anyway. Doc-only PRs still pay the checkout cost (detection requires an API call we cannot make atif:-evaluation time); documented in a comment for a future refactor.Phase 1 is still warning-only —
enforce-evidencedefault remainsfalse. Parser logic (scripts/parse-evidence.mjs) and its tests (scripts/parse-evidence.test.mjs) are untouched.Verification
npm testlocally on this branch; outputTest Files 1 passed (1) / Tests 16 passed (16). No parser source was modified.pr.draftearly-return is at line ~175, well before the dynamic-import at line ~238, so a draft PR never attempts the import and the try/catch is never exercised on the draft path. On a non-draft PR pointed at a pre-parserworkflow-library-ref, the checkout still runs but the parser file is absent → try/catch emits a warning and returns cleanly.examples/pr-policy.ymlnow lists every scope the reusable workflow declares, since a reusable workflow can never grant itself more than its caller does.Verification Notes
npm test→ 16/16 unit tests pass (parser untouched).pr-policy.ymlstep order to confirm M2 catch never fires on the draft path (draft early-return precedes import).🤖 Dogfooding the new evidence shape — this PR is warning-only by default so a malformed block would not block merge anyway, but the block above is intentionally well-formed.