Repository navigation
ci(dev-lead): guard the caller↔reusable permission contract (prevents startup_failure) - #455
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test script, test_caller_permissions.py, which validates the GITHUB_TOKEN permission contract between a caller workflow and a reusable workflow by comparing local configurations against fetched templates. The review feedback highlights several robust error-handling improvements, specifically addressing potential AttributeError crashes if the YAML parser returns None for empty files, and handling cases where GitHub Actions permissions are defined as strings (such as write-all or read-all) rather than dictionaries.
There was a problem hiding this comment.
Pull request overview
Adds an integration guard to prevent org-wide startup_failure outages caused by permission drift between the dev-lead reusable workflow (requested permissions) and the published caller stub template (granted jobs.<id>.permissions).
Changes:
- Introduces
tests/dev-lead/integration/test_caller_permissions.pyto compute required permission maxima from the reusable and compare them against the published caller template inpetry-projects/.github. - Wires the new check into CI via a
caller-permissionsjob in.github/workflows/test-dev-lead.yml.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/dev-lead/integration/test_caller_permissions.py | New integration test that validates the caller↔reusable GITHUB_TOKEN permission contract against the published template. |
| .github/workflows/test-dev-lead.yml | Adds a CI job to install PyYAML and run the new permission-contract validation script. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df8ffea5c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f602078d1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
There was a problem hiding this comment.
💡 Codex Review
Remove generated Python bytecode
This adds generated __pycache__ artifacts alongside the new source test. Because these .pyc files are CPython-version-specific outputs from running/importing the test, future local or CI runs with a different interpreter or after editing the test can leave tracked bytecode churn unrelated to the actual contract check; please remove the cache files from the commit and keep only the Python source.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
@dev-lead Please action the unresolved Codex review comments on this PR. (Automated re-trigger sweep — #445 now trusts chatgpt-codex-connector[bot].) Generated by Claude Code |
Dev-Lead — on-mention (no-changes)Engine ran but made no changes. |
Dev-Lead — rate-limited (intent: fix-reviews)PR: #455 |
|
Auto-rebase failed — merge conflict — this branch has conflicts with Claude will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
A reusable workflow job can only use token permissions the caller grants at jobs.<id>.permissions; if a job requests more, every consumer fails at startup (startup_failure) with no runtime error. #435 added statuses:read to the reusable but not the caller template, breaking every @main consumer (.github 15/15) — petry-projects/.github#402. Add test_caller_permissions.py: parse the union of all reusable job permissions and assert the published caller template (petry-projects/.github standards/workflows/dev-lead.yml) grants >= each scope. Wire it as a new test-dev-lead job. This fails the build at the PR that edits the reusable — forcing the template grant to land first (the safe ordering). Verified: catches a tampered reusable requesting packages:write; the negative message names the exact scope and file to fix. NOTE: this job stays red until the template gains statuses:read (petry-projects/.github#403) — that's the contract gap it correctly detects. Refs petry-projects/.github#402 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
@donpetry-bot review |
|
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
@donpetry-bot review |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 680720a4a5dec2263fee4c079a5edb81098581c5
Review mode: triage-approved (single reviewer)
Summary
Confirming the triage tier's LOW-risk assessment. This PR adds a CI guard test that computes the union (max level per scope) of permissions requested by every job in dev-lead-reusable.yml and asserts the published caller template (petry-projects/.github → standards/workflows/dev-lead.yml) grants ≥ each scope. It correctly enforces the safe ordering: the caller grant must merge first, then the reusable may request it — converting a previously silent org-wide startup_failure (the petry-projects/.github#402 incident) into a red check on the PR that causes it.
The implementation is careful about edge cases: it handles the write-all / read-all string shorthands with a wildcard "*" key on both sides, falls back to workflow-level permissions when a job omits its own block, and emits specific, actionable failure messages naming the exact scope, file, and job key to fix.
Linked issue analysis
No closing-issue link, but the PR body references petry-projects/.github#402 (the incident this guards against) and #403 (the template-side fix that must merge first). The PR body explicitly flags the merge-ordering expectation. Scope matches the stated intent.
Findings
- Test-only addition — no production runtime code changed. Only files touched are a new test (
tests/dev-lead/integration/test_caller_permissions.py), a new CI job in.github/workflows/test-dev-lead.yml, and a local.gitignorefor__pycache__. - Pinned action SHA —
actions/checkoutis pinned by SHA (df4cb1c…/v6.0.3), consistent with repo convention. - Minimal job permissions — the new
caller-permissionsjob declares onlycontents: read. The remote template is fetched from the publicpetry-projects/.githubrepo viagithub.token, which is appropriate. - Prior bot feedback incorporated — earlier review iterations addressed Gemini/Copilot/Codex notes on
None-yaml defensiveness, string-shorthand permission handling, and the__pycache__/ignore. CodeRabbit's latest review is APPROVED. - No transient deps in the test —
pip install pyyamlis installed at job runtime (unpinned), which is acceptable for an ephemeral test environment. Not a blocker.
CI status
All checks SUCCESS: CodeQL, SonarCloud, CodeRabbit, agent-shield, gitleaks, gh-aw-compile, validate-agent-profiles, Agent Security Scan, unit-tests, ShellCheck, bats, Lint, Analyze (actions), dependency-audit (skipped where N/A). No unresolved review threads or unanswered human-reviewer questions.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
The dispatch job in dev-lead-reusable.yml already requests statuses: read (added in #435), but the caller stub template was never updated to grant it. This caused every @main consumer to fail at startup (startup_failure). Syncing the template unblocks the new caller-permissions CI guard being added in petry-projects/.github-private#455.
… startup_failure) (#455) * ci(dev-lead): guard the caller<->reusable permission contract A reusable workflow job can only use token permissions the caller grants at jobs.<id>.permissions; if a job requests more, every consumer fails at startup (startup_failure) with no runtime error. #435 added statuses:read to the reusable but not the caller template, breaking every @main consumer (.github 15/15) — petry-projects/.github#402. Add test_caller_permissions.py: parse the union of all reusable job permissions and assert the published caller template (petry-projects/.github standards/workflows/dev-lead.yml) grants >= each scope. Wire it as a new test-dev-lead job. This fails the build at the PR that edits the reusable — forcing the template grant to land first (the safe ordering). Verified: catches a tampered reusable requesting packages:write; the negative message names the exact scope and file to fix. NOTE: this job stays red until the template gains statuses:read (petry-projects/.github#403) — that's the contract gap it correctly detects. Refs petry-projects/.github#402 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: apply manual instructions [skip ci-relay] * chore: apply manual instructions [skip ci-relay] * fix(reviews): address review comments [skip ci-relay] * chore: apply manual instructions [skip ci-relay] * chore: apply manual instructions [skip ci-relay] --------- Co-authored-by: Claude Code Bot <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
… startup_failure) (#455) * ci(dev-lead): guard the caller<->reusable permission contract A reusable workflow job can only use token permissions the caller grants at jobs.<id>.permissions; if a job requests more, every consumer fails at startup (startup_failure) with no runtime error. #435 added statuses:read to the reusable but not the caller template, breaking every @main consumer (.github 15/15) — petry-projects/.github#402. Add test_caller_permissions.py: parse the union of all reusable job permissions and assert the published caller template (petry-projects/.github standards/workflows/dev-lead.yml) grants >= each scope. Wire it as a new test-dev-lead job. This fails the build at the PR that edits the reusable — forcing the template grant to land first (the safe ordering). Verified: catches a tampered reusable requesting packages:write; the negative message names the exact scope and file to fix. NOTE: this job stays red until the template gains statuses:read (petry-projects/.github#403) — that's the contract gap it correctly detects. Refs petry-projects/.github#402 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: apply manual instructions [skip ci-relay] * chore: apply manual instructions [skip ci-relay] * fix(reviews): address review comments [skip ci-relay] * chore: apply manual instructions [skip ci-relay] * chore: apply manual instructions [skip ci-relay] --------- Co-authored-by: Claude Code Bot <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
Why
A reusable workflow's jobs can only use
GITHUB_TOKENpermissions the caller grants atjobs.<id>.permissions. If a reusable job requests more, every consumer fails at startup (startup_failure) — no runtime error, the run is just invalid.This is exactly petry-projects/.github#402: #435 added
statuses: readto the reusable's job permissions but never to the caller template, so every@mainconsumer broke (.githubwas 15/15startup_failure). Nothing caught it.What
tests/dev-lead/integration/test_caller_permissions.pycomputes the union (max level per scope) of all reusable job permissions and asserts the published caller template (petry-projects/.github→standards/workflows/dev-lead.yml) grants ≥ each scope. Wired as a newcaller-permissionsjob intest-dev-lead.yml.Because it checks the reusable's PR against the published template, it enforces the safe ordering: the caller grant must merge first, then the reusable may request it. A future
permissions:addition to the reusable fails this check until the template is updated — converting a silent org-wide outage into a red check on the PR that causes it.Verification
packages: writeis flagged, naming the exact scope + file to fix.statuses: readMISSING — because the live template on.github@maindoesn't have it yet.This job stays red until petry-projects/.github#403 merges (which adds
statuses: readto the template). That red is the contract gap it correctly detects. Merge #403 first, then this goes green.Refs petry-projects/.github#402
🤖 Generated with Claude Code