From d64745991b1ffad9daa7fdb64a21bf31939e002d Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 19 Aug 2025 10:09:30 +0300 Subject: [PATCH 1/4] Add Stage 1 workflow for external PR info collection - Collects PR information without requiring secrets - Triggers on pull_request events and @claude-review-ext comments - Uploads PR details as artifact for secure processing --- .../workflows/claude-review-ext-stage1.yml | 231 ++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 .github/workflows/claude-review-ext-stage1.yml diff --git a/.github/workflows/claude-review-ext-stage1.yml b/.github/workflows/claude-review-ext-stage1.yml new file mode 100644 index 00000000..fa193dcb --- /dev/null +++ b/.github/workflows/claude-review-ext-stage1.yml @@ -0,0 +1,231 @@ +name: Claude Review External - Stage 1 (PR Info Collection) + +# This workflow runs on pull requests from forks +# It collects PR information and saves it as an artifact for Stage 2 +# No secrets are available or needed in this stage + +on: + pull_request: + types: [opened, synchronize, reopened] + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + +jobs: + collect-pr-info: + # Only trigger on specific conditions + # For PRs: always collect info + # For comments: only when @claude-review-ext is mentioned + if: | + github.event_name == 'pull_request' || + ( + (github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment') && + contains(github.event.comment.body, '@claude-review-ext') + ) + + runs-on: ubuntu-latest + + permissions: + contents: read + pull-requests: read + + steps: + - name: Checkout PR code + uses: actions/checkout@v4 + with: + # For PRs, this automatically checks out the merge commit + fetch-depth: 0 + + - name: Collect PR Information + id: pr-info + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + + let prNumber, prTitle, prBody, prAuthor, baseBranch, headBranch, headSha; + let triggerPhrase = ''; + let commentBody = ''; + let commentId = null; + let commentAuthor = ''; + + if (context.eventName === 'pull_request') { + prNumber = context.payload.pull_request.number; + prTitle = context.payload.pull_request.title; + prBody = context.payload.pull_request.body || ''; + prAuthor = context.payload.pull_request.user.login; + baseBranch = context.payload.pull_request.base.ref; + headBranch = context.payload.pull_request.head.ref; + headSha = context.payload.pull_request.head.sha; + } else if (context.eventName === 'issue_comment') { + // For issue comments on PRs + const issue = context.payload.issue; + if (!issue.pull_request) { + console.log('Not a PR comment, skipping'); + return; + } + + prNumber = issue.number; + triggerPhrase = '@claude-review-ext'; + commentBody = context.payload.comment.body; + commentId = context.payload.comment.id; + commentAuthor = context.payload.comment.user.login; + + // Fetch full PR details + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber + }); + + prTitle = pr.title; + prBody = pr.body || ''; + prAuthor = pr.user.login; + baseBranch = pr.base.ref; + headBranch = pr.head.ref; + headSha = pr.head.sha; + } else if (context.eventName === 'pull_request_review_comment') { + prNumber = context.payload.pull_request.number; + prTitle = context.payload.pull_request.title; + prBody = context.payload.pull_request.body || ''; + prAuthor = context.payload.pull_request.user.login; + baseBranch = context.payload.pull_request.base.ref; + headBranch = context.payload.pull_request.head.ref; + headSha = context.payload.pull_request.head.sha; + triggerPhrase = '@claude-review-ext'; + commentBody = context.payload.comment.body; + commentId = context.payload.comment.id; + commentAuthor = context.payload.comment.user.login; + } + + // Get list of changed files + const { data: files } = await github.rest.pulls.listFiles({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber + }); + + const changedFiles = files.map(f => ({ + filename: f.filename, + status: f.status, + additions: f.additions, + deletions: f.deletions, + changes: f.changes, + patch: f.patch + })); + + // Get diff + const { data: diff } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + mediaType: { + format: 'diff' + } + }); + + const prInfo = { + prNumber, + prTitle, + prBody, + prAuthor, + baseBranch, + headBranch, + headSha, + triggerPhrase, + commentBody, + commentId, + commentAuthor, + changedFiles, + diff, + eventName: context.eventName, + repository: `${context.repo.owner}/${context.repo.repo}`, + triggeredAt: new Date().toISOString() + }; + + // Save to file + fs.writeFileSync('pr-info.json', JSON.stringify(prInfo, null, 2)); + + console.log(`Collected info for PR #${prNumber} by ${prAuthor}`); + console.log(`Changed files: ${changedFiles.length}`); + console.log(`Event: ${context.eventName}`); + if (triggerPhrase) { + console.log(`Triggered by: ${commentAuthor} with phrase: ${triggerPhrase}`); + } + + // Set outputs for job summary + core.setOutput('pr_number', prNumber); + core.setOutput('pr_author', prAuthor); + core.setOutput('changed_files_count', changedFiles.length); + + - name: Run Basic Validation + id: validation + run: | + echo "Running basic validation checks..." + + # Check if this is from a fork + IS_FORK="${{ github.event.pull_request.head.repo.fork }}" + echo "Is from fork: $IS_FORK" + + # Count files changed + CHANGED_FILES=$(jq '.changedFiles | length' pr-info.json) + echo "Files changed: $CHANGED_FILES" + + # Check file types + echo "File types changed:" + jq -r '.changedFiles[].filename' pr-info.json | sed 's/.*\.//' | sort | uniq -c + + # Check for potential issues + LARGE_DIFF=false + if [ "$CHANGED_FILES" -gt 100 ]; then + LARGE_DIFF=true + echo "⚠️ Large PR detected: $CHANGED_FILES files changed" + fi + + echo "is_fork=$IS_FORK" >> $GITHUB_OUTPUT + echo "large_diff=$LARGE_DIFF" >> $GITHUB_OUTPUT + + - name: Upload PR Information + uses: actions/upload-artifact@v4 + with: + name: pr-info-${{ github.event.pull_request.number || github.event.issue.number }} + path: pr-info.json + retention-days: 1 + + - name: Post Status Comment + if: github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment' + uses: actions/github-script@v7 + with: + script: | + const prNumber = ${{ steps.pr-info.outputs.pr_number }}; + const isFork = '${{ steps.validation.outputs.is_fork }}' === 'true'; + const filesCount = ${{ steps.pr-info.outputs.changed_files_count }}; + + let statusMessage = `🔄 **Claude Review Stage 1 Complete**\n\n`; + statusMessage += `- PR: #${prNumber}\n`; + statusMessage += `- Author: @${{ steps.pr-info.outputs.pr_author }}\n`; + statusMessage += `- Files Changed: ${filesCount}\n`; + statusMessage += `- Fork Status: ${isFork ? '🔱 External Fork' : '✅ Direct Branch'}\n\n`; + statusMessage += `📋 Stage 2 (Claude Review) will run automatically after this workflow completes.\n`; + statusMessage += `This two-stage process ensures secure handling of forked PRs.`; + + // Post comment + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: statusMessage + }); + + - name: Job Summary + run: | + echo "## Claude Review Stage 1 Summary" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "- **PR Number**: #${{ steps.pr-info.outputs.pr_number }}" >> $GITHUB_STEP_SUMMARY + echo "- **Author**: @${{ steps.pr-info.outputs.pr_author }}" >> $GITHUB_STEP_SUMMARY + echo "- **Files Changed**: ${{ steps.pr-info.outputs.changed_files_count }}" >> $GITHUB_STEP_SUMMARY + echo "- **From Fork**: ${{ steps.validation.outputs.is_fork }}" >> $GITHUB_STEP_SUMMARY + echo "- **Large PR**: ${{ steps.validation.outputs.large_diff }}" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "✅ PR information collected and uploaded as artifact for Stage 2 processing." >> $GITHUB_STEP_SUMMARY \ No newline at end of file From fc97b4f5bd472d257c8f9696576db07cbdfb22cc Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 19 Aug 2025 10:09:45 +0300 Subject: [PATCH 2/4] Add Stage 2 secure review workflow for external PRs - Runs after Stage 1 via workflow_run trigger - Has access to repository secrets - Downloads PR artifact and performs review - Maintains security by never checking out fork code --- .github/workflows/claude-review-ext.yml | 385 ++++++++++++++++++++++++ 1 file changed, 385 insertions(+) create mode 100644 .github/workflows/claude-review-ext.yml diff --git a/.github/workflows/claude-review-ext.yml b/.github/workflows/claude-review-ext.yml new file mode 100644 index 00000000..7b3355c4 --- /dev/null +++ b/.github/workflows/claude-review-ext.yml @@ -0,0 +1,385 @@ +name: Claude Review External - Stage 2 (Secure Review) + +# This workflow runs after Stage 1 completes +# It has access to secrets and performs the actual Claude review +# Security: Runs in the context of the base repository, not the fork + +on: + workflow_run: + workflows: ["Claude Review External - Stage 1 (PR Info Collection)"] + types: [completed] + +jobs: + check-authorization: + # Only run if Stage 1 completed successfully + if: github.event.workflow_run.conclusion == 'success' + + runs-on: ubuntu-latest + + outputs: + authorized: ${{ steps.check.outputs.authorized }} + pr_number: ${{ steps.check.outputs.pr_number }} + comment_author: ${{ steps.check.outputs.comment_author }} + trigger_phrase: ${{ steps.check.outputs.trigger_phrase }} + + permissions: + actions: read + pull-requests: read + + steps: + - name: Download PR Info Artifact + uses: actions/github-script@v7 + id: download-artifact + with: + script: | + // Get artifacts from the workflow run + const artifacts = await github.rest.actions.listWorkflowRunArtifacts({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: ${{ github.event.workflow_run.id }} + }); + + const matchArtifact = artifacts.data.artifacts.find(artifact => + artifact.name.startsWith('pr-info-') + ); + + if (!matchArtifact) { + core.setFailed('No PR info artifact found'); + return; + } + + // Download the artifact + const download = await github.rest.actions.downloadArtifact({ + owner: context.repo.owner, + repo: context.repo.repo, + artifact_id: matchArtifact.id, + archive_format: 'zip' + }); + + const fs = require('fs'); + const path = require('path'); + fs.writeFileSync('pr-info.zip', Buffer.from(download.data)); + + // Extract the artifact + const { exec } = require('child_process'); + await new Promise((resolve, reject) => { + exec('unzip -o pr-info.zip', (error, stdout, stderr) => { + if (error) reject(error); + else resolve(stdout); + }); + }); + + console.log('Artifact downloaded and extracted'); + + - name: Check Authorization + id: check + run: | + # Read PR info + PR_INFO=$(cat pr-info.json) + PR_NUMBER=$(echo "$PR_INFO" | jq -r '.prNumber') + COMMENT_AUTHOR=$(echo "$PR_INFO" | jq -r '.commentAuthor // .prAuthor') + TRIGGER_PHRASE=$(echo "$PR_INFO" | jq -r '.triggerPhrase // ""') + EVENT_NAME=$(echo "$PR_INFO" | jq -r '.eventName') + + echo "PR Number: $PR_NUMBER" + echo "Comment Author: $COMMENT_AUTHOR" + echo "Trigger Phrase: $TRIGGER_PHRASE" + echo "Event: $EVENT_NAME" + + # Check if this was triggered by a comment with the right phrase + AUTHORIZED="false" + if [[ "$EVENT_NAME" == "pull_request" ]]; then + # Automatic review for all PRs + AUTHORIZED="true" + echo "Authorized: Automatic review for PR event" + elif [[ "$TRIGGER_PHRASE" == "@claude-review-ext" ]]; then + # Check if user is authorized + AUTHORIZED_USERS='["Wirasm", "coleam00", "sean-eskerium"]' + if echo "$AUTHORIZED_USERS" | jq -e --arg user "$COMMENT_AUTHOR" 'contains([$user])' > /dev/null; then + AUTHORIZED="true" + echo "Authorized: $COMMENT_AUTHOR is in authorized users list" + else + echo "Not authorized: $COMMENT_AUTHOR is not in authorized users list" + fi + fi + + echo "authorized=$AUTHORIZED" >> $GITHUB_OUTPUT + echo "pr_number=$PR_NUMBER" >> $GITHUB_OUTPUT + echo "comment_author=$COMMENT_AUTHOR" >> $GITHUB_OUTPUT + echo "trigger_phrase=$TRIGGER_PHRASE" >> $GITHUB_OUTPUT + + claude-review: + needs: check-authorization + if: needs.check-authorization.outputs.authorized == 'true' + + runs-on: ubuntu-latest + + permissions: + contents: read # Read repository contents + pull-requests: write # Comment on PRs + issues: write # Comment on issues + actions: read # Read CI results + id-token: write # Required for OIDC authentication + + steps: + - name: Checkout Repository (Base Branch) + uses: actions/checkout@v4 + with: + # SECURITY: Checkout base branch, not PR code + ref: ${{ github.event.workflow_run.head_branch }} + fetch-depth: 0 + + - name: Download PR Info + uses: actions/github-script@v7 + with: + script: | + // Get artifacts from the workflow run + const artifacts = await github.rest.actions.listWorkflowRunArtifacts({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: ${{ github.event.workflow_run.id }} + }); + + const matchArtifact = artifacts.data.artifacts.find(artifact => + artifact.name.startsWith('pr-info-') + ); + + if (!matchArtifact) { + core.setFailed('No PR info artifact found'); + return; + } + + // Download the artifact + const download = await github.rest.actions.downloadArtifact({ + owner: context.repo.owner, + repo: context.repo.repo, + artifact_id: matchArtifact.id, + archive_format: 'zip' + }); + + const fs = require('fs'); + fs.writeFileSync('pr-info.zip', Buffer.from(download.data)); + + // Extract the artifact + const { exec } = require('child_process'); + await new Promise((resolve, reject) => { + exec('unzip -o pr-info.zip', (error, stdout, stderr) => { + if (error) reject(error); + else resolve(stdout); + }); + }); + + // Read and parse PR info + const prInfo = JSON.parse(fs.readFileSync('pr-info.json', 'utf8')); + + // Save key information as environment variables + core.exportVariable('PR_NUMBER', prInfo.prNumber); + core.exportVariable('PR_TITLE', prInfo.prTitle); + core.exportVariable('PR_AUTHOR', prInfo.prAuthor); + core.exportVariable('HEAD_SHA', prInfo.headSha); + + console.log(`Loaded PR #${prInfo.prNumber} information`); + + - name: Fetch PR Branch for Analysis + run: | + # Fetch the PR branch to analyze (but don't checkout) + git fetch origin pull/${{ env.PR_NUMBER }}/head:pr-branch + + # Create a safe diff for analysis + git diff origin/${{ github.event.workflow_run.head_branch }}...pr-branch > pr-diff.patch + + echo "Fetched PR branch for analysis (not checked out for security)" + + - name: Post Starting Comment + uses: actions/github-script@v7 + with: + script: | + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: ${{ env.PR_NUMBER }}, + body: `🤖 **Claude Review (External PR) Starting...**\n\nAnalyzing PR changes in a secure environment. This may take a few minutes.` + }); + + - name: Run Claude Code Review + id: claude + uses: anthropics/claude-code-action@beta + timeout-minutes: 15 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + + # Use the external review trigger phrase + trigger_phrase: "@claude-review-ext" + + # Custom context for PR review + pr_number: ${{ env.PR_NUMBER }} + + # Review-specific instructions (same as claude-review.yml) + custom_instructions: | + You are performing a CODE REVIEW for an EXTERNAL PULL REQUEST. + This is a secure two-stage review process for PRs from forks. + + ## Security Context + You are running in a secure environment with access to repository secrets. + The PR code has been fetched but NOT checked out for security reasons. + Review the changes in pr-diff.patch file. + + ## Your Role + You are reviewing code for Archon V2 Alpha, a local-first AI knowledge management system in early alpha stage. + + ## Architecture Context + - Frontend: React + TypeScript + Vite (port 3737) + - Backend: FastAPI + Socket.IO + Python (port 8181) + - MCP Service: MCP protocol server (port 8051) + - Agents Service: PydanticAI agents (port 8052) + - Database: Supabase (PostgreSQL + pgvector) + + ## Review Process + 1. **Understand Changes** + - Review the pr-diff.patch file to see all changes + - Check what files were changed and understand the context + - Analyze the impact across all services (frontend, backend, MCP, agents) + - Consider interactions between components + + ## Review Focus Areas + + ### 1. Code Quality - Backend (Python) + - Type hints on all functions and classes + - Pydantic v2 models for data validation (ConfigDict not class Config, model_dump() not dict()) + - No print() statements (use logging instead) + - Proper error handling with detailed error messages + - Following PEP 8 + - Google style docstrings where appropriate + + ### 2. Code Quality - Frontend (React/TypeScript) + - Proper TypeScript types (avoid 'any') + - React hooks used correctly + - Component composition and reusability + - Proper error boundaries + - Following existing component patterns + + ### 3. Structure & Architecture + - Each feature self-contained with its own models, service, and tools + - Shared components only for things used by multiple features + - Proper separation of concerns across services + - API endpoints follow RESTful conventions + + ### 4. Testing + - Unit tests co-located with code in tests/ folders + - Edge cases covered + - Mocking external dependencies + - Frontend: Vitest tests for components + - Backend: Pytest tests for services + + ### 5. Alpha Project Principles (from CLAUDE.md) + - No backwards compatibility needed - can break things + - Fail fast with detailed errors (not graceful failures) + - Remove dead code immediately + - Focus on functionality over production patterns + + ### 6. Security Considerations for External PRs + - Check for any attempts to access secrets or environment variables + - Look for suspicious network requests or data exfiltration + - Verify no hardcoded credentials or API keys + - Check for SQL injection vulnerabilities + - Review any file system operations + + ## Required Output Format + + ## Summary + [2-3 sentence overview of what the changes do and their impact] + + ## Security Review (External PR) + ⚠️ This is an external PR from a fork. Additional security checks performed: + - [ ] No unauthorized access to secrets/env vars + - [ ] No suspicious network requests + - [ ] No hardcoded credentials + - [ ] No SQL injection risks + - [ ] Safe file system operations + [List any security concerns found or state "No security issues found"] + + ## Issues Found + Total: [X critical, Y important, Z minor] + + ### 🔴 Critical (Must Fix) + [Issues that will break functionality or cause data loss] + - **[Issue Title]** - `path/to/file.py:123` + Problem: [What's wrong] + Fix: [Specific solution] + + ### 🟡 Important (Should Fix) + [Issues that impact user experience or code maintainability] + - **[Issue Title]** - `path/to/file.tsx:45` + Problem: [What's wrong] + Fix: [Specific solution] + + ### 🟢 Minor (Consider) + [Nice-to-have improvements] + - **[Suggestion]** - `path/to/file.py:67` + [Brief description and why it would help] + + ## Performance Considerations + - Database query efficiency (no N+1 queries) + - Frontend bundle size impacts + - Async/await usage in Python + - React re-render optimization + [List any performance issues or state "No performance concerns"] + + ## Good Practices Observed + - [Highlight what was done well] + - [Patterns that should be replicated] + + ## Test Coverage + **Current Coverage:** [Estimate based on what you see] + **Missing Tests:** + + 1. **[Component/Function Name]** + - What to test: [Specific functionality] + - Why important: [Impact if it fails] + - Suggested test: [One sentence description] + + ## Recommendations + + **Merge Decision:** + - [ ] Ready to merge as-is + - [ ] Requires fixes before merging + - [ ] Needs security review by maintainer + + **Priority Actions:** + 1. [Most important fix needed, if any] + 2. [Second priority, if applicable] + + **External PR Note:** + This PR is from a fork. Maintainers should manually verify the changes before merging. + + --- + *Review based on Archon V2 Alpha guidelines and secure external PR process* + + unauthorized-message: + needs: check-authorization + if: needs.check-authorization.outputs.authorized == 'false' && needs.check-authorization.outputs.trigger_phrase == '@claude-review-ext' + + runs-on: ubuntu-latest + + permissions: + issues: write + pull-requests: write + + steps: + - name: Post Unauthorized Message + uses: actions/github-script@v7 + with: + script: | + const prNumber = '${{ needs.check-authorization.outputs.pr_number }}'; + const commentAuthor = '${{ needs.check-authorization.outputs.comment_author }}'; + + const comment = { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: parseInt(prNumber), + body: `❌ @${commentAuthor} - You are not authorized to trigger Claude reviews.\n\n` + + `Only the following maintainers can trigger Claude: @Wirasm, @coleam00, @sean-eskerium\n\n` + + `Please ask a maintainer for review.` + }; + + await github.rest.issues.createComment(comment); \ No newline at end of file From 0bb97d8e261ee3aeed92fca56a71178bfa99b882 Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 19 Aug 2025 10:09:59 +0300 Subject: [PATCH 3/4] Add documentation for external PR review workflows - Explains the two-stage security model - Provides usage instructions for contributors and maintainers - Includes troubleshooting and security considerations --- .github/workflows/README-EXTERNAL-PRS.md | 150 +++++++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 .github/workflows/README-EXTERNAL-PRS.md diff --git a/.github/workflows/README-EXTERNAL-PRS.md b/.github/workflows/README-EXTERNAL-PRS.md new file mode 100644 index 00000000..e76cb0f0 --- /dev/null +++ b/.github/workflows/README-EXTERNAL-PRS.md @@ -0,0 +1,150 @@ +# Claude Review for External PRs (Forked Repositories) + +## Overview + +This document explains the two-stage workflow system for running Claude Code reviews on pull requests from forked repositories. This approach solves the GitHub Actions security limitation where forked PRs cannot access repository secrets. + +## The Problem + +- **Direct PRs**: Contributors with write access push branches directly to the main repository. Workflows have full access to secrets. ✅ +- **Forked PRs**: External contributors fork the repo and submit PRs. GitHub blocks secret access for security. ❌ + +## The Solution: Two-Stage Workflow + +### Stage 1: PR Information Collection (`claude-review-ext-stage1.yml`) + +- **Trigger**: Runs on `pull_request` events or when someone comments `@claude-review-ext` +- **Permissions**: Read-only, no secrets +- **Purpose**: Safely collect PR information from untrusted code +- **Output**: Uploads PR details as an artifact + +### Stage 2: Secure Review (`claude-review-ext.yml`) + +- **Trigger**: Runs after Stage 1 completes via `workflow_run` +- **Permissions**: Has access to repository secrets +- **Purpose**: Downloads PR info and runs Claude review in secure context +- **Security**: Never checks out untrusted PR code directly + +## How to Use + +### For External Contributors + +1. Fork the repository +2. Create your feature branch +3. Submit a pull request +4. Wait for automatic review or ask a maintainer to comment `@claude-review-ext` + +### For Maintainers + +To trigger a review on an external PR: +``` +@claude-review-ext +``` + +Authorized users: +- @Wirasm +- @coleam00 +- @sean-eskerium + +## Security Model + +### Stage 1 (Untrusted Environment) +```yaml +permissions: + contents: read # Read-only + pull-requests: read # Read-only +# No secrets available +``` + +### Stage 2 (Trusted Environment) +```yaml +permissions: + contents: read # Read base repo + pull-requests: write # Post comments + issues: write # Post comments + id-token: write # OIDC auth +# Secrets ARE available +``` + +### Key Security Features + +1. **No Direct Code Execution**: Stage 2 never runs PR code directly +2. **Artifact-Based Communication**: PR information passed via artifacts, not code execution +3. **Authorization Check**: Only authorized users can trigger reviews via comments +4. **Base Branch Checkout**: Stage 2 checks out main branch, not PR branch +5. **Diff Analysis**: Reviews changes via git diff, not by running PR code + +## Workflow Diagram + +``` +┌──────────────────┐ +│ External Fork PR │ +└────────┬─────────┘ + │ + v +┌────────────────────────┐ +│ Stage 1: Collect Info │ +│ (No Secrets) │ +│ - Checkout PR code │ +│ - Gather PR details │ +│ - Upload as artifact │ +└────────┬───────────────┘ + │ + v +┌────────────────────────┐ +│ Stage 2: Claude Review │ +│ (Has Secrets) │ +│ - Download artifact │ +│ - Checkout base branch │ +│ - Analyze diff │ +│ - Post review │ +└────────────────────────┘ +``` + +## Comparison with Standard Workflows + +| Feature | Standard (`@claude-review`) | External (`@claude-review-ext`) | +|---------|----------------------------|--------------------------------| +| Trigger | `@claude-review` | `@claude-review-ext` | +| Works on direct branches | ✅ | ✅ | +| Works on forked PRs | ❌ | ✅ | +| Workflow files | 1 | 2 | +| Security model | Direct execution | Two-stage isolation | +| Setup complexity | Simple | Moderate | + +## Troubleshooting + +### Review Not Triggering + +1. Check if you used the correct trigger: `@claude-review-ext` (not `@claude-review`) +2. Verify you're an authorized user +3. Check if Stage 1 workflow completed successfully + +### Stage 1 Fails + +- Usually indicates a problem with the PR itself +- Check workflow logs for details + +### Stage 2 Fails + +- Often indicates an issue with Claude API or token +- Check if `CLAUDE_CODE_OAUTH_TOKEN` secret is configured +- Verify artifact was created in Stage 1 + +## Limitations + +1. **Slight Delay**: Two-stage process takes longer than direct review +2. **Artifact Size**: Large PRs might exceed artifact limits +3. **Manual Trigger**: Automatic reviews on PR open require maintainer action for forks + +## Future Improvements + +- GitHub App integration for better fork support +- Automatic security scanning before review +- Caching for faster reviews on PR updates + +## References + +- [GitHub Actions Security Best Practices](https://docs.github.com/en/actions/security-guides) +- [Preventing PWN Requests](https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/) +- [Claude Code GitHub Actions](https://docs.anthropic.com/en/docs/claude-code/github-actions) \ No newline at end of file From c79040ad4a0169b081e99c868ad24489de3f786b Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 19 Aug 2025 10:19:36 +0300 Subject: [PATCH 4/4] Fix base branch checkout in Stage 2 workflow - Extract PR base branch from artifact instead of using workflow branch - Add step to switch to correct base branch after downloading PR info - Use PR base branch for diff generation instead of workflow branch --- .github/workflows/claude-review-ext.yml | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/.github/workflows/claude-review-ext.yml b/.github/workflows/claude-review-ext.yml index 7b3355c4..3d19bd16 100644 --- a/.github/workflows/claude-review-ext.yml +++ b/.github/workflows/claude-review-ext.yml @@ -126,7 +126,7 @@ jobs: uses: actions/checkout@v4 with: # SECURITY: Checkout base branch, not PR code - ref: ${{ github.event.workflow_run.head_branch }} + # We'll checkout the default branch first, then switch to the PR's base branch after downloading PR info fetch-depth: 0 - name: Download PR Info @@ -177,16 +177,24 @@ jobs: core.exportVariable('PR_TITLE', prInfo.prTitle); core.exportVariable('PR_AUTHOR', prInfo.prAuthor); core.exportVariable('HEAD_SHA', prInfo.headSha); + core.exportVariable('PR_BASE_BRANCH', prInfo.baseBranch); console.log(`Loaded PR #${prInfo.prNumber} information`); + console.log(`Base branch: ${prInfo.baseBranch}`); + + - name: Switch to PR Base Branch + run: | + # Switch to the PR's actual target base branch + git checkout ${{ env.PR_BASE_BRANCH }} + echo "Switched to base branch: ${{ env.PR_BASE_BRANCH }}" - name: Fetch PR Branch for Analysis run: | # Fetch the PR branch to analyze (but don't checkout) git fetch origin pull/${{ env.PR_NUMBER }}/head:pr-branch - # Create a safe diff for analysis - git diff origin/${{ github.event.workflow_run.head_branch }}...pr-branch > pr-diff.patch + # Create a safe diff for analysis against the PR's target base branch + git diff origin/${{ env.PR_BASE_BRANCH }}...pr-branch > pr-diff.patch echo "Fetched PR branch for analysis (not checked out for security)"