fix: require actions: read for all setups, not just fork#11
Conversation
The reusable workflow declares actions: read at the workflow level and uses it unconditionally (cross-run artifact downloads for memory persistence and feedback processing). It is not limited to fork/ 2-workflow setups. Changes: - review-pr/README.md: add actions: read to the 1-workflow permissions block with clarifying comment - SKILL.md section 4a: same addition to the 1-workflow YAML example - SKILL.md section 5 upgrade checklist: add actions: read to the universal required-permissions list; remove the fork-only bullet - SKILL.md section 6 troubleshooting: broaden '403' heading and cause description to cover all setups - SKILL.md section 7 audit checklist: update actions: read annotation from 'required for fork setups only' to 'required for all setups'
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The changes correctly broaden actions: read from a fork-only requirement to a universal one across both review-pr/README.md and SKILL.md. The same-repo permissions examples, upgrade checklist, troubleshooting section, and audit checklist are all updated consistently. No bugs or functional issues were introduced.
Minor observations (no action required):
-
review-pr/README.mdline 115 andSKILL.mdline 208 each retain the old fork-specific comment# Download artifacts from trigger workflowon theactions: readline in their respective fork (pr-review.yml) YAML examples. The newly added same-repo lines both use the updated phrasing# Required by reusable workflow for artifact operations. These unchanged lines are out of this PR's scope but a future pass could align them for consistency. -
The removed fork-specific upgrade checklist bullet ("Fork repos:
actions: readpermission inpr-review.yml") previously explained the 403 symptom in context. That detail is still reachable via the troubleshooting section, so nothing is lost — but readers who only scan the checklist won't see the 403 callout anymore. Again, out of scope for this fix but worth noting.
Address minor observations from docker-agent review: - Align the actions: read comment in the fork pr-review.yml YAML examples in both files to match the same-repo wording, noting it also covers trigger artifact downloads - Add a 403 note back to the upgrade checklist permissions bullet so checklist-only readers still see the concrete failure symptom
|
Thanks for the review. Both observations were easy wins so I went ahead and addressed them in a follow-up commit: aligned the |
3 participants
The reusable workflow declares
actions: readat the workflow level and uses it unconditionally (cross-run artifact downloads for memory/feedback processing). It is not limited to fork/2-workflow setups — every caller needs it.Changes
review-pr/README.md: addactions: readto the 1-workflow (same-repo) permissions block with comment# Required by reusable workflow for artifact operationsSKILL.mdsection 4a: same addition to the 1-workflow YAML exampleSKILL.mdsection 5 upgrade checklist: addactions: readto the universal required-permissions list; remove the fork-onlyactions: readbulletSKILL.mdsection 6 troubleshooting: broaden the "Fork setup: artifact download fails with 403" entry to cover all setupsSKILL.mdsection 7 audit checklist: update theactions: readannotation from "required for fork setups only" to "required for all setups"