Skip to content

Add two-stage workflow for secure Claude reviews on forked PRs#325

Merged
Wirasm merged 4 commits intomainfrom
feature/external-pr-claude-review
Aug 19, 2025
Merged

Add two-stage workflow for secure Claude reviews on forked PRs#325
Wirasm merged 4 commits intomainfrom
feature/external-pr-claude-review

Conversation

@Wirasm
Copy link
Copy Markdown
Collaborator

@Wirasm Wirasm commented Aug 19, 2025

Pull Request

Summary

Implements a secure two-stage workflow system to enable Claude Code reviews on pull requests from forked repositories, addressing the GitHub Actions security limitation where forked PRs cannot access repository secrets.

Changes Made

  • Created Stage 1 workflow (claude-review-ext-stage1.yml) to collect PR information without secrets
  • Created Stage 2 workflow (claude-review-ext.yml) to perform secure Claude review with secrets access
  • Added comprehensive documentation explaining the security model and usage
  • Implemented @claude-review-ext trigger command for external PR reviews

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Performance improvement
  • Code refactoring

Affected Services

  • Frontend (React UI)
  • Server (FastAPI backend)
  • MCP Server (Model Context Protocol)
  • Agents (PydanticAI service)
  • Database (migrations/schema)
  • Docker/Infrastructure
  • Documentation site

Testing

  • All existing tests pass
  • Added new tests for new functionality
  • Manually tested affected user flows
  • Docker builds succeed for all services

Test Evidence

# Validated YAML syntax for both workflow files
# Workflows will be tested on actual forked PRs after merge

Checklist

  • My code follows the service architecture patterns
  • If using an AI coding assistant, I used the CLAUDE.md rules
  • I have added tests that prove my fix/feature works
  • All new and existing tests pass locally
  • My changes generate no new warnings
  • I have updated relevant documentation
  • I have verified no regressions in existing features

Breaking Changes

None - this adds new functionality without modifying existing workflows.

Additional Notes

Security Model

The two-stage approach maintains GitHub's security model:

  • Stage 1: Runs on fork code but has no secrets access (safe)
  • Stage 2: Has secrets access but never checks out fork code (secure)

How It Works

  1. External contributor submits PR from fork
  2. Stage 1 workflow collects PR information and uploads as artifact
  3. Stage 2 workflow downloads artifact and runs Claude review in secure context
  4. Review is posted as PR comment

Usage

  • Trigger command: @claude-review-ext
  • Authorized users: Wirasm, coleam00, sean-eskerium
  • Works alongside existing @claude-review for direct branches

Key Security Features

  • No direct execution of untrusted fork code with secrets
  • Artifact-based communication between stages
  • Authorization checks for manual triggers
  • Reviews changes via git diff, not code execution

This solution addresses the limitation where @claude-review works for maintainers but not for external contributors from forks.

Wirasm added 3 commits August 19, 2025 10:09
- Collects PR information without requiring secrets
- Triggers on pull_request events and @claude-review-ext comments
- Uploads PR details as artifact for secure processing
- 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
- Explains the two-stage security model
- Provides usage instructions for contributors and maintainers
- Includes troubleshooting and security considerations
@Wirasm
Copy link
Copy Markdown
Collaborator Author

Wirasm commented Aug 19, 2025

@claude-review

@claude
Copy link
Copy Markdown

claude Bot commented Aug 19, 2025

Claude finished @Wirasm's task —— View job


Claude Code Review

Todo List:

  • Read and analyze documentation file
  • Review Stage 1 workflow implementation
  • Review Stage 2 workflow implementation
  • Evaluate security model and implementation
  • Check workflow syntax and GitHub Actions best practices
  • Provide comprehensive review feedback

Summary

Implements a sophisticated two-stage workflow system that enables secure Claude Code reviews on external PRs from forks by separating PR information collection (Stage 1, no secrets) from the actual review execution (Stage 2, with secrets). The implementation correctly addresses GitHub Actions' security limitations while maintaining review functionality.

Previous Review Comments

First review - no previous comments

Issues Found

Total: 0 critical, 2 important, 3 minor

🔴 Critical (Must Fix)

None found.

🟡 Important (Should Fix)

  • Stage 1 Checkout Security - .github/workflows/claude-review-ext-stage1.yml:35-38
    Problem: Stage 1 checks out PR code (actions/checkout@v4 with default behavior) which executes in the fork's context but still has read access to the main repo
    Fix: Consider adding explicit token: ${{ secrets.GITHUB_TOKEN }} and document that this is acceptable since no secrets are available in Stage 1

  • Hard-coded Base Branch Reference - .github/workflows/claude-review-ext.yml:129
    Problem: Uses ${{ github.event.workflow_run.head_branch }} to checkout, but this refers to the trigger workflow's branch, not necessarily the target base branch
    Fix: Should fetch the base branch from PR info: ref: ${{ github.event.repository.default_branch }} or extract from PR data

🟢 Minor (Consider)

  • Artifact Retention - .github/workflows/claude-review-ext-stage1.yml:194
    Suggestion: 1-day retention is very short; consider 3-7 days for debugging failed reviews

  • Error Handling Enhancement - .github/workflows/claude-review-ext.yml:72
    Suggestion: Add error handling for malformed pr-info.json files with jq -e checks before processing

  • Documentation Completeness - .github/workflows/README-EXTERNAL-PRS.md:137
    Suggestion: Add artifact size limits section (GitHub has ~500MB limit per artifact)

Security Assessment

Excellent security model implementation. Key security features verified:

Two-stage isolation: Stage 1 has no secrets, Stage 2 never executes PR code
Proper permissions: Each stage has minimal required permissions
Authorization checks: Only authorized users can trigger manual reviews
Artifact-based communication: No direct code execution in Stage 2
Base branch checkout: Stage 2 safely analyzes diffs without running PR code
No secrets exposure: PR information collection happens without secret access

The implementation follows GitHub's security best practices for handling untrusted fork PRs.

Performance Considerations

  • Workflow chaining delay: Expected 30-60 second delay between stages due to workflow_run trigger
  • Artifact download overhead: Acceptable for JSON payload sizes
  • Diff analysis efficiency: Git diff approach is optimal for large PRs
  • Timeout handling: 15-minute timeout is appropriate for Claude API calls

No performance concerns identified.

Good Practices Observed

  • Comprehensive documentation with clear security model explanation and troubleshooting guide
  • Excellent error messaging with detailed job summaries and status comments
  • Proper YAML structure with clear comments explaining security considerations
  • Event filtering logic correctly handles different trigger scenarios (PR events vs comments)
  • Authorization model with explicit user allowlisting
  • Fallback handling with unauthorized user messaging
  • Artifact naming with PR number for easy identification

Questionable Practices

  • Duplicate artifact download code between authorization check and main review job could be extracted to a reusable action
  • Manual user list maintenance in YAML could become outdated (consider GitHub teams)

Test Coverage

Current Coverage: Documentation and workflow configuration only

Missing Tests:
Since these are GitHub Actions workflows, traditional unit testing isn't applicable. However, consider:

  1. Integration Testing

    • What to test: End-to-end workflow execution with test PRs
    • Why important: Validates the complete two-stage process
    • Suggested test: Create test fork PR and verify review posting
  2. Security Validation

    • What to test: Unauthorized user attempts and malicious PR handling
    • Why important: Ensures security model holds under attack
    • Suggested test: Test with non-authorized user triggering review

Recommendations

Merge Decision:

  • Ready to merge as-is

Priority Actions:

  1. Consider fixing the base branch checkout issue in Stage 2 (line 129)
  2. Document artifact size limitations for very large PRs

Rationale:
This is an excellent implementation of a secure two-stage review system. The security model is sound, the code quality is high, and the documentation is comprehensive. The identified issues are minor and don't affect core functionality. The implementation correctly solves the challenging problem of enabling Claude reviews on fork PRs while maintaining GitHub's security guarantees.

The approach is well-architected for an alpha project, focusing on functionality while maintaining security best practices. The comprehensive documentation will help maintainers and contributors understand the system.


Review based on Archon V2 Alpha guidelines and CLAUDE.md principles

- 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
@Wirasm Wirasm merged commit 00a8157 into main Aug 19, 2025
8 checks passed
POWERFULMOVES added a commit to POWERFULMOVES/PMOVES-Archon that referenced this pull request Feb 12, 2026
* feat(ui): add error boundaries and error utilities

- Add root error boundary (app/error.tsx) for global error handling
- Add dashboard error boundary (app/dashboard/error.tsx)
- Add errorUtils.ts with logError, logForDebugging, Result type
- Provides consistent error handling patterns for the UI

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix: address PR review feedback for error boundaries

- Use logError() instead of console.error() for production visibility
- Remove html/body tags from root error boundary (not global-error.tsx)
- Add role="alert" and aria-live for accessibility
- Add aria-hidden="true" to decorative icon
- Add error context (component, digest) to log calls

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Codex Agent <codex-agent@example.com>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
POWERFULMOVES pushed a commit to POWERFULMOVES/PMOVES-Archon that referenced this pull request Feb 12, 2026
- Add UI Development Checklist section to CLAUDE.md with security,
  privacy, accessibility, and code quality guidelines
- Create ui-patterns.md context doc with error handling, authentication,
  and accessibility patterns from PRs coleam00#325, coleam00#326, coleam00#327
- Document CodeRabbit review learnings for future reference

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
POWERFULMOVES pushed a commit to POWERFULMOVES/PMOVES-Archon that referenced this pull request Feb 12, 2026
- Update status date to 2025-12-18
- Add Prosodic TTS sidecar (PR coleam00#328): boundary-aware chunking, TTFS optimization
- Add Pipecat multimodal layer Phase 1 (PR coleam00#332): STT/LLM/TTS processors
- Add Flute Gateway Voice Agent Infrastructure section with API examples
- Document UI security hardening (PRs coleam00#325-331)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
coleam00 pushed a commit that referenced this pull request Apr 7, 2026
* docs: Add Phase 5 CLI binary distribution plan

- Create detailed implementation plan for binary distribution
- Add Phase 5.0: Bundle defaults for binary (depends on #322)
- Add Phase 5.1-5.7: Build scripts, GitHub Actions, curl install,
  Homebrew formula, Windows docs, version command, release guide
- Update research doc with Phase 6 (auto-update command)
- Renumber dashboard to Phase 7, workflow builder to Phase 8
- Mark Phases 1-4 as complete in research doc

* feat: Phase 5 - CLI binary distribution

Implement standalone binary distribution for Archon CLI:

- Bundle default commands and workflows into binaries at compile time
- Add build scripts for cross-platform compilation (macOS/Linux, ARM64/x64)
- Create GitHub Actions release workflow triggered on version tags
- Add curl install script with checksum verification
- Create Homebrew formula for macOS/Linux installation
- Update version command to show platform, build type, and database info
- Add developer release guide documentation
- Update README with CLI installation instructions

Binary compilation uses Bun's --compile flag to create standalone
executables that include the Bun runtime and all dependencies.
Default workflows and commands are imported as text at compile time
and embedded directly into the binary.

* fix: Pin Dockerfile to Bun 1.3.4 to match lockfile version

The Docker build was failing because oven/bun:1-slim resolved to 1.3.6
while the lockfile was created with 1.3.4, causing --frozen-lockfile to fail.

* docs: Clarify binary vs source builds for default commands/workflows

* fix: Address PR review issues for CLI binary distribution

Security fixes:
- install.sh: Require SKIP_CHECKSUM=true to bypass checksum verification
  instead of silently skipping (addresses security vulnerability)
- install.sh: Show actual error output when version check fails instead
  of falsely reporting success

Validation improvements:
- checksums.sh: Validate all 4 expected binaries exist before generating
  checksums to prevent releasing incomplete builds
- build-binaries.sh: Verify binary exists and has reasonable size (>1MB)
  after each build step
- update-homebrew.sh: Validate extracted checksums are non-empty and
  look like valid SHA256 hashes (64 hex chars)
- update-homebrew.sh: Fix sed patterns to use URL context for updating
  checksums on subsequent runs

Bug fixes:
- homebrew/archon.rb: Fix test to expect exit code 0 (success) instead
  of 1 for `archon version`
- loader.ts: Log error when bundled workflow fails to parse (indicates
  build-time corruption)

Test coverage:
- Add bundled-defaults.test.ts for isBinaryBuild() and content validation
- Add connection.test.ts for getDatabaseType() function
- Add binary build bundled workflow tests to loader.test.ts
- Add binary build bundled command tests to executor.test.ts

All 959 tests pass.
Tyone88 pushed a commit to Tyone88/Archon that referenced this pull request Apr 16, 2026
* docs: Add Phase 5 CLI binary distribution plan

- Create detailed implementation plan for binary distribution
- Add Phase 5.0: Bundle defaults for binary (depends on coleam00#322)
- Add Phase 5.1-5.7: Build scripts, GitHub Actions, curl install,
  Homebrew formula, Windows docs, version command, release guide
- Update research doc with Phase 6 (auto-update command)
- Renumber dashboard to Phase 7, workflow builder to Phase 8
- Mark Phases 1-4 as complete in research doc

* feat: Phase 5 - CLI binary distribution

Implement standalone binary distribution for Archon CLI:

- Bundle default commands and workflows into binaries at compile time
- Add build scripts for cross-platform compilation (macOS/Linux, ARM64/x64)
- Create GitHub Actions release workflow triggered on version tags
- Add curl install script with checksum verification
- Create Homebrew formula for macOS/Linux installation
- Update version command to show platform, build type, and database info
- Add developer release guide documentation
- Update README with CLI installation instructions

Binary compilation uses Bun's --compile flag to create standalone
executables that include the Bun runtime and all dependencies.
Default workflows and commands are imported as text at compile time
and embedded directly into the binary.

* fix: Pin Dockerfile to Bun 1.3.4 to match lockfile version

The Docker build was failing because oven/bun:1-slim resolved to 1.3.6
while the lockfile was created with 1.3.4, causing --frozen-lockfile to fail.

* docs: Clarify binary vs source builds for default commands/workflows

* fix: Address PR review issues for CLI binary distribution

Security fixes:
- install.sh: Require SKIP_CHECKSUM=true to bypass checksum verification
  instead of silently skipping (addresses security vulnerability)
- install.sh: Show actual error output when version check fails instead
  of falsely reporting success

Validation improvements:
- checksums.sh: Validate all 4 expected binaries exist before generating
  checksums to prevent releasing incomplete builds
- build-binaries.sh: Verify binary exists and has reasonable size (>1MB)
  after each build step
- update-homebrew.sh: Validate extracted checksums are non-empty and
  look like valid SHA256 hashes (64 hex chars)
- update-homebrew.sh: Fix sed patterns to use URL context for updating
  checksums on subsequent runs

Bug fixes:
- homebrew/archon.rb: Fix test to expect exit code 0 (success) instead
  of 1 for `archon version`
- loader.ts: Log error when bundled workflow fails to parse (indicates
  build-time corruption)

Test coverage:
- Add bundled-defaults.test.ts for isBinaryBuild() and content validation
- Add connection.test.ts for getDatabaseType() function
- Add binary build bundled workflow tests to loader.test.ts
- Add binary build bundled command tests to executor.test.ts

All 959 tests pass.
joaobmonteiro pushed a commit to joaobmonteiro/Archon that referenced this pull request Apr 26, 2026
* docs: Add Phase 5 CLI binary distribution plan

- Create detailed implementation plan for binary distribution
- Add Phase 5.0: Bundle defaults for binary (depends on coleam00#322)
- Add Phase 5.1-5.7: Build scripts, GitHub Actions, curl install,
  Homebrew formula, Windows docs, version command, release guide
- Update research doc with Phase 6 (auto-update command)
- Renumber dashboard to Phase 7, workflow builder to Phase 8
- Mark Phases 1-4 as complete in research doc

* feat: Phase 5 - CLI binary distribution

Implement standalone binary distribution for Archon CLI:

- Bundle default commands and workflows into binaries at compile time
- Add build scripts for cross-platform compilation (macOS/Linux, ARM64/x64)
- Create GitHub Actions release workflow triggered on version tags
- Add curl install script with checksum verification
- Create Homebrew formula for macOS/Linux installation
- Update version command to show platform, build type, and database info
- Add developer release guide documentation
- Update README with CLI installation instructions

Binary compilation uses Bun's --compile flag to create standalone
executables that include the Bun runtime and all dependencies.
Default workflows and commands are imported as text at compile time
and embedded directly into the binary.

* fix: Pin Dockerfile to Bun 1.3.4 to match lockfile version

The Docker build was failing because oven/bun:1-slim resolved to 1.3.6
while the lockfile was created with 1.3.4, causing --frozen-lockfile to fail.

* docs: Clarify binary vs source builds for default commands/workflows

* fix: Address PR review issues for CLI binary distribution

Security fixes:
- install.sh: Require SKIP_CHECKSUM=true to bypass checksum verification
  instead of silently skipping (addresses security vulnerability)
- install.sh: Show actual error output when version check fails instead
  of falsely reporting success

Validation improvements:
- checksums.sh: Validate all 4 expected binaries exist before generating
  checksums to prevent releasing incomplete builds
- build-binaries.sh: Verify binary exists and has reasonable size (>1MB)
  after each build step
- update-homebrew.sh: Validate extracted checksums are non-empty and
  look like valid SHA256 hashes (64 hex chars)
- update-homebrew.sh: Fix sed patterns to use URL context for updating
  checksums on subsequent runs

Bug fixes:
- homebrew/archon.rb: Fix test to expect exit code 0 (success) instead
  of 1 for `archon version`
- loader.ts: Log error when bundled workflow fails to parse (indicates
  build-time corruption)

Test coverage:
- Add bundled-defaults.test.ts for isBinaryBuild() and content validation
- Add connection.test.ts for getDatabaseType() function
- Add binary build bundled workflow tests to loader.test.ts
- Add binary build bundled command tests to executor.test.ts

All 959 tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant