From 506cbc18c442a2574e9dc05a3d62f63fbeeead63 Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 19 Aug 2025 10:57:25 +0300 Subject: [PATCH] Simplify fork PR review to single workflow with pull_request_target - Remove complex two-stage workflow approach - Use pull_request_target with security safeguards - Add first-time contributor check and approval requirement - Never checkout PR code - only analyze diff - Mirror full review format from main claude-review workflow - Manual trigger via @claude-review-fork for maintainers --- .github/workflows/README-EXTERNAL-PRS.md | 150 -------- .../workflows/claude-review-ext-stage1.yml | 281 --------------- .github/workflows/claude-review-ext.yml | 314 ----------------- .github/workflows/claude-review-fork.yml | 323 ++++++++++++++++++ 4 files changed, 323 insertions(+), 745 deletions(-) delete mode 100644 .github/workflows/README-EXTERNAL-PRS.md delete mode 100644 .github/workflows/claude-review-ext-stage1.yml delete mode 100644 .github/workflows/claude-review-ext.yml create mode 100644 .github/workflows/claude-review-fork.yml diff --git a/.github/workflows/README-EXTERNAL-PRS.md b/.github/workflows/README-EXTERNAL-PRS.md deleted file mode 100644 index e76cb0f0..00000000 --- a/.github/workflows/README-EXTERNAL-PRS.md +++ /dev/null @@ -1,150 +0,0 @@ -# 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 diff --git a/.github/workflows/claude-review-ext-stage1.yml b/.github/workflows/claude-review-ext-stage1.yml deleted file mode 100644 index 8ddf19fb..00000000 --- a/.github/workflows/claude-review-ext-stage1.yml +++ /dev/null @@ -1,281 +0,0 @@ -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 by authorized users - 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') && - contains(fromJSON('["Wirasm", "coleam00", "sean-eskerium"]'), github.event.comment.user.login) - ) - - runs-on: ubuntu-latest - - permissions: - contents: read - pull-requests: write # Need write to post comments - - 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 - continue-on-error: true # Don't fail the workflow if comment posting fails - 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.`; - - try { - // Post comment on the PR - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: prNumber, - body: statusMessage - }); - console.log(`Successfully posted status comment on PR #${prNumber}`); - } catch (error) { - console.log(`Warning: Could not post comment on PR #${prNumber}: ${error.message}`); - console.log('This is expected for some permission scenarios. Stage 2 will still run.'); - } - - - 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 - - unauthorized-message: - # Post message for unauthorized users - if: | - ( - github.event_name == 'issue_comment' || - github.event_name == 'pull_request_review_comment' - ) && - contains(github.event.comment.body, '@claude-review-ext') && - !contains(fromJSON('["Wirasm", "coleam00", "sean-eskerium"]'), github.event.comment.user.login) - - runs-on: ubuntu-latest - - permissions: - issues: write - pull-requests: write - - steps: - - name: Post unauthorized message - uses: actions/github-script@v7 - with: - script: | - const comment = { - owner: context.repo.owner, - repo: context.repo.repo, - body: `❌ @${context.actor} - You are not authorized to trigger Claude reviews on external PRs.\n\nOnly the following maintainers can trigger Claude: @Wirasm, @coleam00, @sean-eskerium\n\nPlease ask a maintainer for review.` - }; - - if (context.eventName === 'issue_comment') { - await github.rest.issues.createComment({ - ...comment, - issue_number: context.issue.number - }); - } else if (context.eventName === 'pull_request_review_comment') { - await github.rest.pulls.createReplyForReviewComment({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - comment_id: context.payload.comment.id, - body: comment.body - }); - } \ No newline at end of file diff --git a/.github/workflows/claude-review-ext.yml b/.github/workflows/claude-review-ext.yml deleted file mode 100644 index 81a43bd5..00000000 --- a/.github/workflows/claude-review-ext.yml +++ /dev/null @@ -1,314 +0,0 @@ -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: - claude-review: - # Only run if Stage 1 completed successfully - # Stage 1 already handles authorization, so we trust it - if: github.event.workflow_run.conclusion == 'success' - - 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 - # 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 - 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 using execSync instead of exec to avoid declaration issues - const childProcess = require('child_process'); - try { - childProcess.execSync('unzip -o pr-info.zip'); - console.log('Artifact extracted successfully'); - } catch (error) { - core.setFailed(`Failed to extract artifact: ${error.message}`); - return; - } - - // 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); - 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 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)" - - - 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: Create Event Context for Claude - run: | - # Create a fake event.json that simulates an issue_comment event - cat > /tmp/event.json << EOF - { - "action": "created", - "issue": { - "number": ${{ env.PR_NUMBER }}, - "title": "${{ env.PR_TITLE }}", - "user": { - "login": "${{ env.PR_AUTHOR }}" - }, - "pull_request": { - "url": "https://api.github.com/repos/${{ github.repository }}/pulls/${{ env.PR_NUMBER }}" - } - }, - "comment": { - "body": "@claude-review-ext", - "user": { - "login": "github-actions" - } - }, - "repository": { - "name": "$(echo ${{ github.repository }} | cut -d'/' -f2)", - "owner": { - "login": "$(echo ${{ github.repository }} | cut -d'/' -f1)" - } - } - } - EOF - echo "Created issue_comment event context for PR #${{ env.PR_NUMBER }}" - - - name: Run Claude Code Review - id: claude - uses: anthropics/claude-code-action@beta - timeout-minutes: 15 - env: - # Set environment variables to provide PR context - GITHUB_EVENT_NAME: 'issue_comment' - GITHUB_EVENT_PATH: '/tmp/event.json' - with: - claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - - # Explicitly provide GitHub token since OIDC fails in workflow_run context - github_token: ${{ secrets.GITHUB_TOKEN }} - - # Use the external review trigger phrase - trigger_phrase: "@claude-review-ext" - - # Direct prompt to review the diff file - direct_prompt: | - Please review the code changes in the pr-diff.patch file and post your review as a comment on PR #${{ env.PR_NUMBER }}. - - The diff file contains all changes from the pull request. Analyze it thoroughly. - - # Review-specific instructions (same as claude-review.yml) - custom_instructions: | - You are performing a CODE REVIEW for an EXTERNAL PULL REQUEST #${{ env.PR_NUMBER }} - 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 using the Read tool. - - ## Important - Start by reading the pr-diff.patch file to see all the changes. - Post your review as a comment on issue #${{ env.PR_NUMBER }}. - - ## 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* \ No newline at end of file diff --git a/.github/workflows/claude-review-fork.yml b/.github/workflows/claude-review-fork.yml new file mode 100644 index 00000000..d5254841 --- /dev/null +++ b/.github/workflows/claude-review-fork.yml @@ -0,0 +1,323 @@ +name: Claude Review for Forked PRs + +# This workflow handles Claude reviews for PRs from forks +# Uses pull_request_target for security with first-time contributor checks + +on: + pull_request_target: + types: [opened, synchronize, reopened] + issue_comment: + types: [created] + +jobs: + claude-review: + # Trigger on: + # 1. Any PR from a fork (automatic review) + # 2. Comment with @claude-review-fork from authorized users + if: | + ( + github.event_name == 'pull_request_target' && + github.event.pull_request.head.repo.fork == true + ) || + ( + github.event_name == 'issue_comment' && + github.event.issue.pull_request && + contains(github.event.comment.body, '@claude-review-fork') && + contains(fromJSON('["Wirasm", "coleam00", "sean-eskerium"]'), github.event.comment.user.login) + ) + + 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: + # SECURITY: Check if this is a first-time contributor + - name: Check First-Time Contributor + id: first-time + uses: actions/github-script@v7 + with: + script: | + const pr = context.payload.pull_request || + (context.payload.issue?.pull_request ? + await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.issue.number + }).then(r => r.data) : null); + + if (!pr) { + core.setFailed('Could not find pull request'); + return; + } + + // Check if author has previous merged PRs + const author = pr.user.login; + const { data: prs } = await github.rest.pulls.list({ + owner: context.repo.owner, + repo: context.repo.repo, + state: 'closed', + per_page: 1 + }); + + const previousPRs = prs.filter(p => + p.user.login === author && + p.merged_at !== null + ); + + const isFirstTime = previousPRs.length === 0; + + if (isFirstTime) { + // Post warning comment for first-time contributors + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pr.number, + body: `⚠️ **First-Time Contributor Notice**\n\n` + + `Hi @${author}! This appears to be your first contribution to this repository.\n\n` + + `For security reasons, automated Claude review requires manual approval for first-time contributors.\n` + + `A maintainer will review your PR and may trigger Claude review if appropriate.\n\n` + + `Maintainers can trigger review with: \`@claude-review-fork\`` + }); + + // Skip the rest of the workflow for first-time contributors + core.setOutput('is_first_time', 'true'); + console.log(`First-time contributor detected: ${author}`); + } else { + core.setOutput('is_first_time', 'false'); + console.log(`Returning contributor: ${author}`); + } + + # Skip review for first-time contributors unless manually triggered + - name: Check if Should Continue + id: should-continue + run: | + if [[ "${{ steps.first-time.outputs.is_first_time }}" == "true" ]] && [[ "${{ github.event_name }}" == "pull_request_target" ]]; then + echo "Skipping automatic review for first-time contributor" + echo "should_continue=false" >> $GITHUB_OUTPUT + else + echo "Proceeding with review" + echo "should_continue=true" >> $GITHUB_OUTPUT + fi + + # SECURITY: Checkout base branch, NOT the PR branch + # This prevents executing untrusted code from forks + - name: Checkout Base Branch + if: steps.should-continue.outputs.should_continue == 'true' + uses: actions/checkout@v4 + with: + # Explicitly checkout the base branch, not the PR head + ref: ${{ github.event.pull_request.base.ref || github.event.repository.default_branch }} + fetch-depth: 0 + + # Fetch PR diff for analysis (without checking out PR code) + - name: Fetch PR Diff + if: steps.should-continue.outputs.should_continue == 'true' + id: pr-info + run: | + # Get PR number + if [[ "${{ github.event_name }}" == "pull_request_target" ]]; then + PR_NUMBER="${{ github.event.pull_request.number }}" + else + PR_NUMBER="${{ github.event.issue.number }}" + fi + + echo "PR_NUMBER=$PR_NUMBER" >> $GITHUB_ENV + + # Fetch the PR branch without checking it out + git fetch origin pull/$PR_NUMBER/head:pr-branch + + # Create diff file for Claude to analyze + git diff ${{ github.event.pull_request.base.ref || github.event.repository.default_branch }}...pr-branch > pr-diff.patch + + echo "Created diff for PR #$PR_NUMBER" + echo "Diff size: $(wc -l < pr-diff.patch) lines" + + - name: Run Claude Code Review + if: steps.should-continue.outputs.should_continue == 'true' + id: claude + uses: anthropics/claude-code-action@beta + timeout-minutes: 15 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + + # Trigger phrase for manual reviews + trigger_phrase: "@claude-review-fork" + + # Custom instructions for fork review + custom_instructions: | + You are performing a CODE REVIEW ONLY for a PULL REQUEST FROM A FORK. You cannot make any changes to files. + + ## CRITICAL: Fork Security Review + This PR is from an external fork. First, check the pr-diff.patch file for any security concerns: + - Attempts to access secrets or environment variables + - Suspicious network requests or data exfiltration + - Hardcoded credentials or API keys + - SQL injection or command injection attempts + - Malicious file system operations + - Changes to GitHub Actions workflows + + ## 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** + - Read 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 + + ## Required Output Format + + ## Summary + [2-3 sentence overview of what the changes do and their impact] + + ## Security Review (Fork PR) + ⚠️ This is an external PR from a fork. Enhanced 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 + - [ ] No malicious workflow changes + [List any security concerns found or state "No security issues found"] + + ## Previous Review Comments + - [If this is a follow-up review, summarize unaddressed comments] + - [If first review, state: "First review - no previous comments"] + + ## 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] + + ## Questionable Practices + - [Design decisions that might need reconsideration] + - [Architectural concerns for discussion] + + ## 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 additional security review + + **Priority Actions:** + 1. [Most important fix needed, if any] + 2. [Second priority, if applicable] + + **Fork PR Note:** + This PR is from an external fork. Maintainers should manually verify critical changes before merging. + + --- + *Review based on Archon V2 Alpha guidelines with enhanced security checks for fork PRs* + + unauthorized-comment: + # Post message for unauthorized users trying to trigger review + if: | + github.event_name == 'issue_comment' && + github.event.issue.pull_request && + contains(github.event.comment.body, '@claude-review-fork') && + !contains(fromJSON('["Wirasm", "coleam00", "sean-eskerium"]'), github.event.comment.user.login) + + runs-on: ubuntu-latest + + permissions: + issues: write + pull-requests: write + + steps: + - name: Post Unauthorized Message + uses: actions/github-script@v7 + with: + script: | + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body: `❌ @${context.actor} - You are not authorized to trigger Claude reviews.\n\n` + + `Only maintainers can trigger Claude: @Wirasm, @coleam00, @sean-eskerium\n\n` + + `Please wait for a maintainer to review your PR.` + }); \ No newline at end of file