Support two-workflow pattern for fork PR comments - #140
Conversation
The main action now uploads the JSON report and PR metadata as artifacts when triggered by pull_request. A new review/ action downloads those artifacts and posts PR comments, intended to be used from a workflow_run trigger with write permissions. This avoids the security risk of pull_request_target + checkout of untrusted PR code, which could allow arbitrary code execution via custom linter rules that invoke subprocess. Also switches to --format text for console output (human-readable in CI logs) with --output to a .json file for the review commenter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use a random delimiter for the multiline GITHUB_OUTPUT value to avoid collisions when the JSON report content interferes with the static SKILLSAW_EOF delimiter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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 refactors the Skillsaw action to support a decoupled review process using GitHub artifacts, enabling secure PR commenting. It updates the main action to output lint results to a temporary file and upload them as artifacts alongside PR metadata. A new composite action is introduced to consume these artifacts and post reviews. Feedback was provided to refine the execution condition for the review step in the main action to prevent potential failures on manual or scheduled workflow runs.
| /tmp/skillsaw-head-sha | ||
| retention-days: 1 | ||
|
|
||
| - name: Post PR review |
There was a problem hiding this comment.
The current condition github.event_name != 'pull_request' && github.event_name != 'push' is too broad. It will cause this step to run on events like workflow_dispatch or schedule, where the github.event.pull_request object is not available. This will lead to the review.py script failing because the PR_NUMBER and HEAD_SHA environment variables will be empty, causing the entire action to fail on manual or scheduled runs.
Using github.event.pull_request != null ensures that the step only runs when pull request metadata is actually present, while still excluding the pull_request event which is now handled by the artifact/workflow_run pattern.
if: github.event.pull_request != null && github.event_name != 'pull_request'Use github.event.pull_request != null instead of excluding known event names, so the step won't run on workflow_dispatch/schedule where PR metadata is unavailable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe action now writes a JSON report and PR metadata artifacts; the main workflow removes PR write permission. A new workflow_run-triggered review workflow runs a composite action that downloads artifacts and invokes review.py to post inline PR comments for pull requests. ChangesReport generation and artifact-based review posting
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The lint step uses set +e and captures the exit code, so it never fails. The always() condition is a tautology. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review comments are now handled entirely by the review/ action via workflow_run. No reason to keep the inline step or the token input. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split the self-lint into lint (read-only) and review (workflow_run with write permissions), matching the pattern this PR introduces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Stop stuffing JSON into GITHUB_OUTPUT via heredoc — it breaks on content that interferes with the delimiter. The report is already written to a file; just expose the path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-action-review.yml:
- Around line 12-13: The workflow's permissions block only grants pull-requests:
write which is too narrow; update the permissions mapping under the permissions
key to include the needed read scopes (e.g., add contents: read and actions:
read) alongside pull-requests: write so checkout and artifact download steps can
run successfully; modify the existing permissions entry that currently reads
"pull-requests: write" to include these additional permission lines.
In `@review/action.yml`:
- Around line 41-43: The script currently unguards the cat reads for PR_NUMBER
and HEAD_SHA so a missing file causes the script to exit under the shell's
fail-fast behavior, preventing the intended skip branch that checks
REPORT_FILE/PR_NUMBER/HEAD_SHA; modify the logic that sets PR_NUMBER and
HEAD_SHA to first test file existence (e.g. [ -f
/tmp/skillsaw-review/skillsaw-pr-number ] and [ -f
/tmp/skillsaw-review/skillsaw-head-sha ]) and only then assign via cat, leaving
the variables empty otherwise, while keeping the existing final conditional that
checks REPORT_FILE, PR_NUMBER and HEAD_SHA so the skip path is reachable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 29f11238-deba-4aa3-879c-319566835777
📒 Files selected for processing (4)
.github/workflows/test-action-review.yml.github/workflows/test-action.ymlaction.ymlreview/action.yml
💤 Files with no reviewable changes (1)
- .github/workflows/test-action.yml
| permissions: | ||
| pull-requests: write |
There was a problem hiding this comment.
Workflow token permissions are too narrow for this job’s steps.
Line 12-13 grants only pull-requests: write; this job also needs read scopes for checkout and artifact download, otherwise it can fail before posting comments.
Suggested fix
permissions:
+ actions: read
+ contents: read
pull-requests: write📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: | |
| pull-requests: write | |
| permissions: | |
| actions: read | |
| contents: read | |
| pull-requests: write |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test-action-review.yml around lines 12 - 13, The
workflow's permissions block only grants pull-requests: write which is too
narrow; update the permissions mapping under the permissions key to include the
needed read scopes (e.g., add contents: read and actions: read) alongside
pull-requests: write so checkout and artifact download steps can run
successfully; modify the existing permissions entry that currently reads
"pull-requests: write" to include these additional permission lines.
There was a problem hiding this comment.
@stbenjam, actually the "defaults" don't carry over once a permissions block is explicitly specified. According to the official GitHub Actions documentation, "if you specify the access for any of these permissions, all of those that are not specified are set to none."
This means that with only pull-requests: write declared in the job's permissions block, both contents and actions will be none — not their repository defaults. Specifically:
actions/checkout@v5requirescontents: readto clone the repo.- Downloading artifacts from a
workflow_runtrigger requiresactions: read.
So the original suggestion to add contents: read and actions: read still stands. Would you like to apply the fix?
Update the GitHub Action docs to show the two-workflow pattern with the review/ action, and remove the old single-workflow example. Guard cat reads in review/action.yml with file existence checks so the skip path is reachable when metadata files are missing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
pull_requestevents, the main action now uploads the JSON report and PR metadata as artifacts instead of trying to post review comments directly (which fails with 403 on fork PRs)review/composite action downloads those artifacts and posts comments — designed to be called from aworkflow_runtrigger that has write permissions--format text(human-readable in CI logs) while writing JSON to a file via--outputfor the commenterpull_request/non-pushevents (e.g.pull_request_targetif someone still uses it directly)Why not just use
pull_request_target?pull_request_target+ checkout of untrusted PR code is a security risk. Custom rules like.skillsaw-custom.pycan runsubprocess.run()on scripts from the checked-out repo, which would execute with a write-capable token.Consumer workflow example
Test plan
pull_requestevents show text output in logs and upload artifactsworkflow_runconsumer downloads artifacts and posts review commentspushevents still work (no artifact upload, no review)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Documentation