diff --git a/.gitignore b/.gitignore index 8b8dd8e..2f83097 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,7 @@ __pycache__/ # Output files *.csv *.json +!claudecode/review-output.schema.json security_report.* # Virtual environments @@ -21,4 +22,4 @@ env/ claudecode/claudecode-prompt.txt eval_results/ -.env \ No newline at end of file +.env diff --git a/AGENTS.md b/AGENTS.md index 610abcc..ffa6a63 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,13 +2,16 @@ ## Project Overview -AI-powered code review tool using Claude to analyze PRs for code quality and security issues. Uses a unified multi-agent approach for comprehensive analysis in a single pass. +AI-powered code review tool using configurable Claude and OpenAI reviewers to analyze PRs for code quality and security issues. All model calls share one structured review schema and one GitHub review publisher. ## Architecture ``` claudecode/ ├── github_action_audit.py # Main orchestrator - entry point +├── review_ensemble.py # Parallel reviewer and synthesis orchestration +├── review-output.schema.json # Shared model output contract +├── review_schema.py # Python schema loader ├── prompts.py # Review prompt templates ├── findings_filter.py # False positive filtering ├── claude_api_client.py # Claude API client diff --git a/README.md b/README.md index a0862ea..94a43d5 100644 --- a/README.md +++ b/README.md @@ -1,12 +1,14 @@ # Nutrient Code Reviewer -An AI-powered code review GitHub Action using Claude to analyze code changes. Uses a unified multi-agent approach for both code quality (correctness, reliability, performance, maintainability, testing) and security in a single pass. This action provides intelligent, context-aware review for pull requests using Anthropic's Claude Code tool for deep semantic analysis. +An AI-powered code review GitHub Action that can run Claude, OpenAI Codex, or both against the same pull request. Every reviewer emits the same structured JSON document, and one provider-neutral publisher submits exactly one GitHub review. Based on the original work from [anthropics/claude-code-security-review](https://github.com/anthropics/claude-code-security-review). ## Features -- **AI-Powered Analysis**: Uses Claude's advanced reasoning to detect issues with deep semantic understanding +- **Configurable Reviewers**: Run Claude, OpenAI Codex, or both in parallel +- **One Review Contract**: Reviewers and the optional synthesizer emit the same JSON schema +- **One GitHub Review**: A single publisher combines the final summary and inline comments into one formal review - **Diff-Aware Scanning**: For PRs, only analyzes changed files - **PR Comments**: Automatically comments on PRs with findings - **Contextual Understanding**: Goes beyond pattern matching to understand code semantics and intent @@ -95,17 +97,63 @@ jobs: **Note**: The `app-slug` parameter enables the bot to detect when it's mentioned in PR comments (e.g., `@my-code-review-app`). Requires `actions/create-github-app-token@v1.9.0` or later. `publish-check` additionally requires the GitHub App to have **Checks: read and write**. The action reacts to an accepted `review` command and creates the in-progress Check Run before checking out the repository. +### Reviewer Selection and Synthesis + +Claude remains the default. Existing callers that only provide `claude-api-key` keep the same path: Claude emits the review JSON, filtering runs once, and the shared publisher posts it. Codex is not installed or invoked. + +```text +claude -> claude.json --------------------------> final JSON -> one publisher +openai -> openai.json --------------------------> final JSON -> one publisher +claude -> claude.json --+ + +-> synthesizer JSON ---> final JSON -> one publisher +openai -> openai.json --+ +``` + +The contract is defined in [`claudecode/review-output.schema.json`](claudecode/review-output.schema.json). With two reviewers, both run concurrently after the single checkout. The configured synthesizer receives both documents and emits that same schema. If either selected reviewer or the synthesizer fails, the action fails instead of posting a misleading partial review. Raw provider and synthesis documents are retained in the private workflow artifact for debugging. + +**OpenAI only:** + +```yaml +- uses: PSPDFKit-labs/nutrient-code-review@main + with: + reviewers: openai + openai-api-key: ${{ secrets.OPENAI_API_KEY }} + openai-model: gpt-5.6-sol +``` + +**Claude and OpenAI, synthesized by Codex:** + +```yaml +- uses: PSPDFKit-labs/nutrient-code-review@main + with: + reviewers: claude,openai + claude-api-key: ${{ secrets.CLAUDE_API_KEY }} + openai-api-key: ${{ secrets.OPENAI_API_KEY }} + openai-model: gpt-5.6-sol + synthesizer-provider: openai + synthesizer-model: gpt-5.6-terra +``` + +The synthesizer provider and model are configurable. A Claude synthesizer can be selected with `synthesizer-provider: claude` and a compatible `synthesizer-model`. Synthesis is skipped entirely when only one reviewer is selected. + ## Security Considerations This action is not hardened against prompt injection attacks and should only be used to review trusted PRs. We recommend [configuring your repository](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories) to use the "Require approval for all external contributors" option to ensure workflows only run after a maintainer has reviewed the PR. +Review agents do not receive the GitHub token. When both providers run, each child process receives only its own provider credential; the Claude process does not receive the OpenAI key and the Codex process does not receive the Anthropic key. Codex runs non-interactively with an ephemeral session and a read-only sandbox. These are process-level controls on the same Actions runner, not a separate VM security boundary. + ## Configuration Options ### Action Inputs | Input | Description | Default | Required | |-------|-------------|---------|----------| -| `claude-api-key` | Anthropic Claude API key for code review analysis.
*Note*: This API key needs to be enabled for both the Claude API and Claude Code usage. | None | Yes | +| `reviewers` | Comma-separated reviewers: `claude`, `openai`, or `claude,openai`. | `claude` | No | +| `claude-api-key` | Anthropic Claude API key. Required only when Claude is selected as a reviewer or synthesizer. | None | Conditional | +| `openai-api-key` | OpenAI API key. Required only when OpenAI is selected as a reviewer or synthesizer. | None | Conditional | +| `openai-model` | Codex model used by the OpenAI reviewer. | `gpt-5.6-sol` | No | +| `synthesizer-provider` | Provider used to combine multiple review documents: `openai` or `claude`. Ignored for one reviewer. | `openai` | No | +| `synthesizer-model` | Model used to combine multiple review documents. Ignored for one reviewer. | `gpt-5.6-terra` | No | | `comment-pr` | Whether to comment on PRs with findings | `true` | No | | `review-mode` | How to post the review on the PR. `approve-reject` submits an `APPROVE` or `REQUEST_CHANGES` verdict based on findings; `comment-only` posts the same inline comments and summary as a non-blocking `COMMENT` review with no verdict. | `approve-reject` | No | | `upload-results` | Whether to upload results as artifacts | `true` | No | @@ -356,6 +404,9 @@ This is especially important if you use `workflow_dispatch` or other event types ``` claudecode/ ├── github_action_audit.py # Main audit script for GitHub Actions +├── review_ensemble.py # Parallel reviewers and optional synthesis +├── review-output.schema.json # Shared reviewer/synthesizer contract +├── review_schema.py # Loads the shared schema for Python callers ├── prompts.py # Code review prompt templates ├── findings_filter.py # False positive filtering logic ├── claude_api_client.py # Claude API client for false positive filtering @@ -367,11 +418,11 @@ claudecode/ ### Workflow -1. **PR Analysis**: When a pull request is opened, Claude analyzes the diff to understand what changed -2. **Contextual Review**: Claude examines the code changes in context, understanding the purpose and potential impacts -3. **Finding Generation**: Issues are identified with detailed explanations, severity ratings, and remediation guidance -4. **False Positive Filtering**: Advanced filtering removes low-impact or false positive prone findings to reduce noise -5. **PR Comments**: Findings are posted as review comments on the specific lines of code +1. **PR Analysis**: Each selected reviewer receives the same diff and repository checkout +2. **Structured Output**: Every reviewer writes the shared review JSON format +3. **Optional Synthesis**: Multiple reviewer documents are combined into one document of the same format +4. **False Positive Filtering**: Filtering runs once over the final document +5. **PR Review**: The shared publisher submits one summary and one set of inline comments ## Review Capabilities diff --git a/action.yml b/action.yml index b93f53e..4d10009 100644 --- a/action.yml +++ b/action.yml @@ -1,5 +1,5 @@ -name: 'Claude Code Reviewer' -description: 'AI-powered code review GitHub Action using Claude. Unified multi-agent review for code quality and security.' +name: 'Nutrient Code Reviewer' +description: 'Configurable AI code review with one provider-neutral GitHub review.' author: 'Nutrient' inputs: @@ -25,7 +25,7 @@ inputs: claude-api-key: description: 'Anthropic Claude API key for code review analysis' - required: true + required: false default: '' claude-model: @@ -33,6 +33,31 @@ inputs: required: false default: '' + reviewers: + description: 'Comma-separated review agents to run: claude, openai, or claude,openai' + required: false + default: 'claude' + + openai-api-key: + description: 'OpenAI API key. Required only when openai is selected as a reviewer or synthesizer.' + required: false + default: '' + + openai-model: + description: 'Codex model used by the OpenAI reviewer.' + required: false + default: 'gpt-5.6-sol' + + synthesizer-provider: + description: 'Provider used to combine multiple review JSON documents: openai or claude.' + required: false + default: 'openai' + + synthesizer-model: + description: 'Model used to combine multiple reviews. Ignored when only one reviewer is selected.' + required: false + default: 'gpt-5.6-terra' + run-every-commit: description: 'DEPRECATED: Use trigger-on-commit instead. Run ClaudeCode on every commit (skips cache check). Warning: This may lead to more false positives on PRs with many commits as the AI analyzes the same code multiple times.' required: false @@ -242,9 +267,7 @@ runs: uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 with: path: .claudecode-marker - key: claudecode-${{ github.repository_id }}-pr-${{ github.event.pull_request.number || steps.pr-info.outputs.pr_number }}-${{ github.event.pull_request.head.sha || steps.pr-info.outputs.pr_sha }} - restore-keys: | - claudecode-${{ github.repository_id }}-pr-${{ github.event.pull_request.number || steps.pr-info.outputs.pr_number }}- + key: claudecode-${{ github.repository_id }}-pr-${{ github.event.pull_request.number || steps.pr-info.outputs.pr_number }}-${{ github.event.pull_request.head.sha || steps.pr-info.outputs.pr_sha }}-${{ inputs.reviewers }}-${{ inputs.claude-model || 'default' }}-${{ inputs.openai-model }}-${{ inputs.synthesizer-provider }}-${{ inputs.synthesizer-model }} - name: Detect trigger type id: trigger-detection @@ -391,7 +414,7 @@ runs: uses: actions/cache/save@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 with: path: .claudecode-marker - key: claudecode-${{ github.repository_id }}-pr-${{ github.event.pull_request.number || steps.pr-info.outputs.pr_number }}-${{ github.event.pull_request.head.sha || steps.pr-info.outputs.pr_sha || github.sha }} + key: claudecode-${{ github.repository_id }}-pr-${{ github.event.pull_request.number || steps.pr-info.outputs.pr_number }}-${{ github.event.pull_request.head.sha || steps.pr-info.outputs.pr_sha || github.sha }}-${{ inputs.reviewers }}-${{ inputs.claude-model || 'default' }}-${{ inputs.openai-model }}-${{ inputs.synthesizer-provider }}-${{ inputs.synthesizer-model }} - name: Set up Python if: steps.claudecode-check.outputs.enable_claudecode == 'true' @@ -460,14 +483,24 @@ runs: shell: bash env: ACTION_PATH: ${{ github.action_path }} + REVIEWERS: ${{ inputs.reviewers }} run: | + set -euo pipefail echo "::group::Install Deps" pip install -r "$ACTION_PATH/claudecode/requirements.txt" - npm install -g @anthropic-ai/claude-code + + NORMALIZED_REVIEWERS=",${REVIEWERS//[[:space:]]/}," + if [[ "$NORMALIZED_REVIEWERS" == *,claude,* ]]; then + npm install -g @anthropic-ai/claude-code + fi + if [[ "$NORMALIZED_REVIEWERS" == *,openai,* ]]; then + npm install -g @openai/codex + fi + sudo apt-get update && sudo apt-get install -y jq echo "::endgroup::" - - name: Run ClaudeCode scan + - name: Run configured code review id: claudecode-scan if: steps.claudecode-check.outputs.enable_claudecode == 'true' shell: bash @@ -476,6 +509,11 @@ runs: GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number || steps.pr-info.outputs.pr_number }} ANTHROPIC_API_KEY: ${{ inputs.claude-api-key }} + CODEX_API_KEY: ${{ inputs.openai-api-key }} + REVIEWERS: ${{ inputs.reviewers }} + OPENAI_MODEL: ${{ inputs.openai-model }} + SYNTHESIZER_PROVIDER: ${{ inputs.synthesizer-provider }} + SYNTHESIZER_MODEL: ${{ inputs.synthesizer-model }} ENABLE_CLAUDE_FILTERING: ${{ inputs.enable-claude-filtering }} ENABLE_HEURISTIC_FILTERING: ${{ inputs.enable-heuristic-filtering }} EXCLUDE_DIRECTORIES: ${{ inputs.exclude-directories }} @@ -487,8 +525,10 @@ runs: MAX_DIFF_CHARS: ${{ inputs.max-diff-chars }} MAX_DIFF_LINES: ${{ inputs.max-diff-lines }} ACTION_PATH: ${{ github.action_path }} + PROVIDER_RESULTS_DIR: ${{ github.action_path }}/claudecode/provider-results run: | - echo "Running ClaudeCode AI code review analysis..." + set -u + echo "Running configured AI code review analysis..." echo "----------------------------------------" # Initialize outputs @@ -501,37 +541,42 @@ runs: exit 0 fi - # Validate API key is provided - if [ -z "$ANTHROPIC_API_KEY" ]; then - echo "::error::ANTHROPIC_API_KEY is not set. Please provide the claude-api-key input to the action." - echo "Example usage:" - echo " - uses: PSPDFKit-labs/nutrient-code-review@main" - echo " with:" - echo " claude-api-key: \$\{{ secrets.ANTHROPIC_API_KEY }}" + NORMALIZED_REVIEWERS=",${REVIEWERS//[[:space:]]/}," + if [[ "$NORMALIZED_REVIEWERS" == *,claude,* ]] && [ -z "$ANTHROPIC_API_KEY" ]; then + echo "::error::claude is selected but claude-api-key is empty" + exit 1 + fi + if [[ "$NORMALIZED_REVIEWERS" == *,openai,* ]] && [ -z "$CODEX_API_KEY" ]; then + echo "::error::openai is selected but openai-api-key is empty" exit 1 fi # Set timeout export CLAUDE_TIMEOUT="$CLAUDECODE_TIMEOUT" - # Run ClaudeCode audit with verbose debugging + # Run the configured reviewer ensemble with verbose debugging export REPO_PATH=$(pwd) cd "$ACTION_PATH" # Enable verbose debugging - echo "::group::ClaudeCode Environment" + echo "::group::Review Environment" echo "Current directory: $(pwd)" echo "Python version: $(python --version)" - echo "Claude CLI version: $(claude --version 2>&1 || echo 'Claude CLI not found')" - echo "ANTHROPIC_API_KEY set: $(if [ -n "$ANTHROPIC_API_KEY" ]; then echo 'Yes'; else echo 'No'; fi)" + echo "Reviewers: $REVIEWERS" + if [[ "$NORMALIZED_REVIEWERS" == *,claude,* ]]; then + echo "Claude CLI version: $(claude --version 2>&1 || echo 'Claude CLI not found')" + fi + if [[ "$NORMALIZED_REVIEWERS" == *,openai,* ]]; then + echo "Codex CLI version: $(codex --version 2>&1 || echo 'Codex CLI not found')" + fi echo "GITHUB_REPOSITORY: $GITHUB_REPOSITORY" echo "PR_NUMBER: $PR_NUMBER" - echo "Python path: $PYTHONPATH" + echo "Python path: ${PYTHONPATH:-}" echo "Files in claudecode directory:" ls -la claudecode/ echo "::endgroup::" - echo "::group::ClaudeCode Execution" + echo "::group::Review Execution" # Add current directory to Python path so it can find the claudecode module export PYTHONPATH="${PYTHONPATH:+$PYTHONPATH:}$(pwd)" echo "Updated PYTHONPATH: $PYTHONPATH" @@ -539,50 +584,51 @@ runs: # Run from the action root directory so Python can find the claudecode module python -u claudecode/github_action_audit.py > claudecode/claudecode-results.json 2>claudecode/claudecode-error.log || CLAUDECODE_EXIT_CODE=$? - if [ -n "$CLAUDECODE_EXIT_CODE" ]; then - echo "::warning::ClaudeCode exited with code $CLAUDECODE_EXIT_CODE" + if [ -n "${CLAUDECODE_EXIT_CODE:-}" ]; then + echo "::warning::Code review exited with code $CLAUDECODE_EXIT_CODE" else - echo "ClaudeCode scan completed successfully" + echo "Code review completed successfully" fi - # Parse ClaudeCode results and count findings regardless of exit code + # Parse the provider-neutral final review and count findings regardless of exit code if [ -f claudecode/claudecode-results.json ]; then FILE_SIZE=$(wc -c < claudecode/claudecode-results.json) - echo "ClaudeCode results file size: $FILE_SIZE bytes" + echo "Review results file size: $FILE_SIZE bytes" # Check if file is empty or too small if [ "$FILE_SIZE" -lt 2 ]; then - echo "::warning::ClaudeCode results file is empty or invalid (size: $FILE_SIZE bytes)" - echo "::warning::ClaudeCode may have failed silently. Check claudecode-error.log" + echo "::warning::Review results file is empty or invalid (size: $FILE_SIZE bytes)" + echo "::warning::The review may have failed silently. Check claudecode-error.log" if [ -f claudecode/claudecode-error.log ]; then echo "Error log contents:" cat claudecode/claudecode-error.log fi echo "findings_count=0" >> $GITHUB_OUTPUT else - echo "ClaudeCode results preview:" + echo "Review results preview:" head -n 300 claudecode/claudecode-results.json || echo "Unable to preview results" # Check if the result is an error if jq -e '.error' claudecode/claudecode-results.json > /dev/null 2>&1; then ERROR_MSG=$(jq -r '.error' claudecode/claudecode-results.json) - echo "::warning::ClaudeCode error: $ERROR_MSG" + echo "::warning::Code review error: $ERROR_MSG" echo "findings_count=0" >> $GITHUB_OUTPUT else # Use -r to get raw output and handle potential null/missing findings array CLAUDECODE_FINDINGS_COUNT=$(jq -r '.findings | if . == null then 0 else length end' claudecode/claudecode-results.json 2>/dev/null || echo "0") echo "::debug::Extracted ClaudeCode findings count: $CLAUDECODE_FINDINGS_COUNT" echo "findings_count=$CLAUDECODE_FINDINGS_COUNT" >> $GITHUB_OUTPUT - echo "ClaudeCode found $CLAUDECODE_FINDINGS_COUNT review issues" + echo "Code review found $CLAUDECODE_FINDINGS_COUNT review issues" - # Also create findings.json and pr-summary.json for PR comment script + # Retain legacy split files for downstream callers. The built-in + # publisher reads the complete review JSON directly. jq '.findings // []' claudecode/claudecode-results.json > findings.json || echo '[]' > findings.json jq '.pr_summary // {}' claudecode/claudecode-results.json > pr-summary.json || echo '{}' > pr-summary.json jq '.analysis_summary // {}' claudecode/claudecode-results.json > analysis-summary.json || echo '{}' > analysis-summary.json fi fi else - echo "::warning::ClaudeCode results file not found" + echo "::warning::Review results file not found" if [ -f claudecode/claudecode-error.log ]; then echo "Error log contents:" cat claudecode/claudecode-error.log @@ -607,6 +653,9 @@ runs: if [ -f claudecode/claudecode-error.log ]; then cp claudecode/claudecode-error.log ${{ github.workspace }}/claudecode-error.log || true fi + if [ -d claudecode/provider-results ]; then + cp -R claudecode/provider-results ${{ github.workspace }}/provider-results || true + fi echo "::endgroup::" @@ -620,6 +669,7 @@ runs: findings.json claudecode-results.json claudecode-error.log + provider-results/ retention-days: 7 if-no-files-found: ignore @@ -631,6 +681,7 @@ runs: CLAUDECODE_FINDINGS: ${{ steps.claudecode-scan.outputs.findings_count }} SILENCE_CLAUDECODE_COMMENTS: ${{ steps.claudecode-check.outputs.silence_claudecode_comments }} ACTION_PATH: ${{ github.action_path }} + REVIEW_RESULTS_FILE: ${{ github.workspace }}/claudecode-results.json PR_HEAD_SHA: ${{ github.event.pull_request.head.sha || steps.pr-info.outputs.pr_sha }} REVIEW_MODE: ${{ inputs.review-mode }} run: | diff --git a/claudecode/constants.py b/claudecode/constants.py index 5bda41b..c430121 100644 --- a/claudecode/constants.py +++ b/claudecode/constants.py @@ -6,6 +6,9 @@ # API Configuration DEFAULT_CLAUDE_MODEL = os.environ.get('CLAUDE_MODEL') or 'claude-opus-4-8' +DEFAULT_GPT_MODEL = 'gpt-5.6-sol' +DEFAULT_SYNTHESIZER_PROVIDER = 'openai' +DEFAULT_SYNTHESIZER_MODEL = 'gpt-5.6-terra' DEFAULT_TIMEOUT_SECONDS = 180 # 3 minutes DEFAULT_MAX_RETRIES = 3 RATE_LIMIT_BACKOFF_MAX = 30 # Maximum backoff time for rate limits @@ -25,4 +28,3 @@ # Subprocess Configuration SUBPROCESS_TIMEOUT = 1200 # 20 minutes for Claude Code execution - diff --git a/claudecode/github_action_audit.py b/claudecode/github_action_audit.py index ff66ae7..147fcac 100644 --- a/claudecode/github_action_audit.py +++ b/claudecode/github_action_audit.py @@ -8,6 +8,7 @@ import sys import json import subprocess +import tempfile import requests from typing import Dict, Any, List, Tuple, Optional from pathlib import Path @@ -26,10 +27,21 @@ EXIT_GENERAL_ERROR, SUBPROCESS_TIMEOUT, DEFAULT_MAX_DIFF_CHARS, - CHARS_PER_LINE_ESTIMATE + CHARS_PER_LINE_ESTIMATE, + DEFAULT_GPT_MODEL, + DEFAULT_SYNTHESIZER_PROVIDER, + DEFAULT_SYNTHESIZER_MODEL, ) from claudecode.logger import get_logger -from claudecode.review_schema import REVIEW_OUTPUT_SCHEMA +from claudecode.review_schema import REVIEW_OUTPUT_SCHEMA, REVIEW_OUTPUT_SCHEMA_PATH +from claudecode.review_ensemble import ( + ReviewConfigurationError, + ReviewExecutionError, + parse_reviewers, + run_reviewers, + synthesize_reviews, + validate_synthesizer, +) logger = get_logger(__name__) @@ -504,7 +516,11 @@ def _filter_generated_files(self, diff_text: str) -> str: class SimpleClaudeRunner: """Simplified Claude Code runner for GitHub Actions.""" - def __init__(self, timeout_minutes: Optional[int] = None): + def __init__( + self, + timeout_minutes: Optional[int] = None, + model: Optional[str] = None, + ): """Initialize Claude runner. Args: @@ -514,6 +530,7 @@ def __init__(self, timeout_minutes: Optional[int] = None): self.timeout_seconds = timeout_minutes * 60 else: self.timeout_seconds = SUBPROCESS_TIMEOUT + self.model = model or DEFAULT_CLAUDE_MODEL def run_code_review(self, repo_dir: Path, prompt: str) -> Tuple[bool, str, Dict[str, Any]]: """Run Claude Code review. @@ -539,7 +556,7 @@ def run_code_review(self, repo_dir: Path, prompt: str) -> Tuple[bool, str, Dict[ cmd = [ 'claude', '--output-format', 'json', - '--model', DEFAULT_CLAUDE_MODEL, + '--model', self.model, '--disallowed-tools', 'Bash(ps:*)', '--json-schema', json.dumps(REVIEW_OUTPUT_SCHEMA) ] @@ -547,13 +564,23 @@ def run_code_review(self, repo_dir: Path, prompt: str) -> Tuple[bool, str, Dict[ # Run Claude Code with retry logic NUM_RETRIES = 3 for attempt in range(NUM_RETRIES): + child_env = os.environ.copy() + for secret_name in ( + 'CODEX_API_KEY', + 'OPENAI_API_KEY', + 'GITHUB_TOKEN', + 'GH_TOKEN', + ): + child_env.pop(secret_name, None) + result = subprocess.run( cmd, input=prompt, # Pass prompt via stdin cwd=repo_dir, capture_output=True, text=True, - timeout=self.timeout_seconds + timeout=self.timeout_seconds, + env=child_env, ) # Parse JSON output (even if returncode != 0, to detect specific errors) @@ -668,6 +695,105 @@ def validate_claude_available(self) -> Tuple[bool, str]: return False, f"Failed to check Claude Code: {str(e)}" +class SimpleCodexRunner: + """Non-interactive Codex runner that emits the shared review JSON schema.""" + + def __init__(self, model: str, timeout_minutes: Optional[int] = None): + self.model = model + self.timeout_seconds = ( + timeout_minutes * 60 if timeout_minutes is not None else SUBPROCESS_TIMEOUT + ) + + def run_code_review( + self, repo_dir: Path, prompt: str + ) -> Tuple[bool, str, Dict[str, Any]]: + if not repo_dir.exists(): + return False, f"Repository directory does not exist: {repo_dir}", {} + + try: + with tempfile.TemporaryDirectory(prefix="nutrient-codex-review-") as temp_dir: + output_path = Path(temp_dir) / "review.json" + cmd = [ + 'codex', + 'exec', + '--ephemeral', + '--ignore-user-config', + '--ignore-rules', + '--sandbox', + 'read-only', + '--model', + self.model, + '--output-schema', + str(REVIEW_OUTPUT_SCHEMA_PATH), + '--output-last-message', + str(output_path), + '-', + ] + + child_env = os.environ.copy() + for secret_name in ( + 'ANTHROPIC_API_KEY', + 'GITHUB_TOKEN', + 'GH_TOKEN', + 'OPENAI_API_KEY', + ): + child_env.pop(secret_name, None) + + result = subprocess.run( + cmd, + input=prompt, + cwd=repo_dir, + capture_output=True, + text=True, + timeout=self.timeout_seconds, + env=child_env, + ) + if result.returncode != 0: + return ( + False, + f"Codex execution failed with return code {result.returncode}: " + f"{result.stderr[-2000:]}", + {}, + ) + if not output_path.exists(): + return False, "Codex did not write its structured review output", {} + + success, parsed_result = parse_json_with_fallbacks( + output_path.read_text(encoding="utf-8"), "Codex review output" + ) + if not success or not isinstance(parsed_result, dict): + return False, "Failed to parse Codex structured review output", {} + if 'findings' not in parsed_result or 'pr_summary' not in parsed_result: + return False, "Codex output did not match the review schema", {} + return True, "", parsed_result + except subprocess.TimeoutExpired: + return ( + False, + f"Codex execution timed out after {self.timeout_seconds // 60} minutes", + {}, + ) + except Exception as error: + return False, f"Codex execution error: {error}", {} + + def validate_codex_available(self) -> Tuple[bool, str]: + try: + result = subprocess.run( + ['codex', '--version'], + capture_output=True, + text=True, + timeout=10, + ) + if result.returncode != 0: + return False, f"Codex returned exit code {result.returncode}" + if not os.environ.get('CODEX_API_KEY'): + return False, "CODEX_API_KEY environment variable is not set" + return True, "" + except FileNotFoundError: + return False, "Codex is not installed or not in PATH" + except Exception as error: + return False, f"Failed to check Codex: {error}" + + def get_environment_config() -> Tuple[str, int]: @@ -718,6 +844,77 @@ def initialize_clients() -> Tuple[GitHubActionClient, SimpleClaudeRunner]: return github_client, claude_runner +def initialize_review_runners(): + """Build the configured reviewer ensemble and synthesizer.""" + reviewer_names = parse_reviewers(os.environ.get('REVIEWERS', 'claude')) + claude_model = os.environ.get('CLAUDE_MODEL') or DEFAULT_CLAUDE_MODEL + openai_model = os.environ.get('OPENAI_MODEL') or DEFAULT_GPT_MODEL + + runners = {} + for name in reviewer_names: + try: + if name == 'claude': + runner = SimpleClaudeRunner(model=claude_model) + else: + runner = SimpleCodexRunner(model=openai_model) + except Exception as error: + display_name = 'Claude' if name == 'claude' else 'OpenAI' + raise ConfigurationError( + f"Failed to initialize {display_name} runner: {error}" + ) from error + runners[name] = runner + + synthesizer = None + if len(reviewer_names) > 1: + synthesis_provider, synthesis_model = validate_synthesizer( + os.environ.get('SYNTHESIZER_PROVIDER') + or DEFAULT_SYNTHESIZER_PROVIDER, + os.environ.get('SYNTHESIZER_MODEL') or DEFAULT_SYNTHESIZER_MODEL, + ) + if synthesis_provider == 'claude': + synthesizer = SimpleClaudeRunner(model=synthesis_model) + else: + synthesizer = SimpleCodexRunner(model=synthesis_model) + + return reviewer_names, runners, synthesizer + + +def validate_review_runners(review_runners, synthesizer) -> Tuple[bool, str]: + """Validate selected CLIs and credentials after the filtering setup.""" + for name, runner in review_runners.items(): + if name == 'claude': + available, error = runner.validate_claude_available() + else: + available, error = runner.validate_codex_available() + if not available: + if name == 'claude': + return False, f"Claude Code not available: {error}" + return False, f"OpenAI Codex not available: {error}" + + if synthesizer is not None: + if isinstance(synthesizer, SimpleClaudeRunner): + available, error = synthesizer.validate_claude_available() + name = 'claude' + else: + available, error = synthesizer.validate_codex_available() + name = 'openai' + if not available: + return False, f"{name} synthesizer not available: {error}" + return True, "" + + +def write_provider_results(results: Dict[str, Dict[str, Any]]) -> None: + """Persist private provider JSON documents for debugging and artifacts.""" + output_dir = Path( + os.environ.get('PROVIDER_RESULTS_DIR', 'claudecode/provider-results') + ) + output_dir.mkdir(parents=True, exist_ok=True) + for provider, result in results.items(): + (output_dir / f"{provider}.json").write_text( + json.dumps(result, indent=2), encoding='utf-8' + ) + + def initialize_findings_filter(custom_filtering_instructions: Optional[str] = None) -> FindingsFilter: """Initialize findings filter based on environment configuration. @@ -890,8 +1087,15 @@ def main(): # Initialize components try: - github_client, claude_runner = initialize_clients() - except ConfigurationError as e: + try: + github_client = GitHubActionClient() + except Exception as error: + raise ConfigurationError( + f"Failed to initialize GitHub client: {error}" + ) from error + reviewer_names, review_runners, synthesizer = initialize_review_runners() + logger.info(f"Configured reviewers: {', '.join(reviewer_names)}") + except (ConfigurationError, ReviewConfigurationError) as e: print(json.dumps({'error': str(e)})) sys.exit(EXIT_CONFIGURATION_ERROR) @@ -901,11 +1105,12 @@ def main(): except ConfigurationError as e: print(json.dumps({'error': str(e)})) sys.exit(EXIT_CONFIGURATION_ERROR) - - # Validate Claude Code is available - claude_ok, claude_error = claude_runner.validate_claude_available() - if not claude_ok: - print(json.dumps({'error': f'Claude Code not available: {claude_error}'})) + + runners_available, runner_error = validate_review_runners( + review_runners, synthesizer + ) + if not runners_available: + print(json.dumps({'error': runner_error})) sys.exit(EXIT_GENERAL_ERROR) # Parse max diff chars setting (with backward compatibility for max_diff_lines) @@ -1018,7 +1223,19 @@ def run_review(include_diff: bool, diff_metadata=None): review_context=review_context, diff_metadata=diff_metadata, ) - return claude_runner.run_code_review(repo_dir, prompt_text), len(prompt_text) + try: + provider_results = run_reviewers( + review_runners, repo_dir, prompt_text + ) + write_provider_results(provider_results) + final_result = synthesize_reviews( + provider_results, synthesizer, repo_dir + ) + if len(provider_results) > 1: + write_provider_results({'synthesized': final_result}) + return (True, "", final_result), len(prompt_text) + except ReviewExecutionError as error: + return (False, str(error), {}), len(prompt_text) all_findings = [] pr_summary_from_review = {} @@ -1038,7 +1255,7 @@ def run_review(include_diff: bool, diff_metadata=None): (success, error_msg, review_results), prompt_len = run_review(include_diff=True) # Fallback to full agentic if prompt still too long - if not success and error_msg == "PROMPT_TOO_LONG": + if not success and "PROMPT_TOO_LONG" in error_msg: logger.info(f"Prompt too long ({prompt_len} chars), falling back to full agentic mode") (success, error_msg, review_results), prompt_len = run_review(include_diff=False) diff --git a/claudecode/review-output.schema.json b/claudecode/review-output.schema.json new file mode 100644 index 0000000..cf3ef13 --- /dev/null +++ b/claudecode/review-output.schema.json @@ -0,0 +1,60 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/PSPDFKit-labs/nutrient-code-review/schemas/review-output.schema.json", + "title": "Nutrient code review result", + "type": "object", + "properties": { + "pr_summary": { + "type": "object", + "properties": { + "overview": { "type": "string" }, + "file_changes": { + "type": "array", + "items": { + "type": "object", + "properties": { + "label": { "type": "string" }, + "files": { "type": "array", "items": { "type": "string" } }, + "changes": { "type": "string" } + }, + "required": ["label", "files", "changes"] + } + } + }, + "required": ["overview", "file_changes"] + }, + "findings": { + "type": "array", + "items": { + "type": "object", + "properties": { + "file": { "type": "string" }, + "line": { "type": "integer" }, + "severity": { "type": "string", "enum": ["HIGH", "MEDIUM", "LOW"] }, + "category": { "type": "string" }, + "title": { "type": "string" }, + "description": { "type": "string" }, + "impact": { "type": "string" }, + "recommendation": { "type": "string" }, + "suggestion": { "type": "string" }, + "suggestion_start_line": { "type": "integer" }, + "suggestion_end_line": { "type": "integer" }, + "confidence": { "type": "number", "minimum": 0, "maximum": 1 }, + "sources": { "type": "array", "items": { "type": "string" } } + }, + "required": [ + "file", + "line", + "severity", + "category", + "title", + "description", + "impact", + "recommendation", + "confidence" + ] + } + } + }, + "required": ["pr_summary", "findings"] +} diff --git a/claudecode/review_ensemble.py b/claudecode/review_ensemble.py new file mode 100644 index 0000000..ddec64b --- /dev/null +++ b/claudecode/review_ensemble.py @@ -0,0 +1,153 @@ +"""Provider-neutral orchestration for one or more code review agents.""" + +from concurrent.futures import ThreadPoolExecutor, as_completed +from copy import deepcopy +import json +from pathlib import Path +from typing import Any, Dict, Iterable, Mapping, Tuple + +from claudecode.constants import DEFAULT_SYNTHESIZER_PROVIDER + + +SUPPORTED_REVIEWERS = ("claude", "openai") + + +class ReviewConfigurationError(ValueError): + """Raised when reviewer or synthesizer configuration is invalid.""" + + +class ReviewExecutionError(RuntimeError): + """Raised when a configured reviewer or synthesizer does not complete.""" + + +def parse_reviewers(value: str) -> Tuple[str, ...]: + """Parse a comma-separated reviewer list while preserving caller order.""" + reviewers = [] + for raw_name in (value or "claude").split(","): + name = raw_name.strip().lower() + if not name: + continue + if name not in SUPPORTED_REVIEWERS: + supported = ", ".join(SUPPORTED_REVIEWERS) + raise ReviewConfigurationError( + f"Unsupported reviewer '{name}'. Supported reviewers: {supported}" + ) + if name not in reviewers: + reviewers.append(name) + + if not reviewers: + raise ReviewConfigurationError("At least one reviewer must be configured") + return tuple(reviewers) + + +def validate_synthesizer(provider: str, model: str) -> Tuple[str, str]: + """Validate and normalize synthesizer configuration.""" + normalized_provider = (provider or DEFAULT_SYNTHESIZER_PROVIDER).strip().lower() + if normalized_provider not in SUPPORTED_REVIEWERS: + supported = ", ".join(SUPPORTED_REVIEWERS) + raise ReviewConfigurationError( + f"Unsupported synthesizer provider '{normalized_provider}'. " + f"Supported providers: {supported}" + ) + normalized_model = (model or "").strip() + if not normalized_model: + raise ReviewConfigurationError("A synthesizer model must be configured") + return normalized_provider, normalized_model + + +def validate_review_result(result: Any, producer: str) -> Dict[str, Any]: + """Validate the stable top-level contract shared by all model calls.""" + if not isinstance(result, dict): + raise ReviewExecutionError(f"{producer} did not return a JSON object") + if "pr_summary" not in result: + # Preserve the historical Claude path, which tolerated an omitted summary. + result["pr_summary"] = {"overview": "", "file_changes": []} + if not isinstance(result.get("pr_summary"), dict): + raise ReviewExecutionError(f"{producer} result has an invalid pr_summary") + if not isinstance(result.get("findings"), list): + raise ReviewExecutionError(f"{producer} result is missing findings") + return result + + +def _run_one(runner: Any, repo_dir: Path, prompt: str) -> Dict[str, Any]: + success, error_message, result = runner.run_code_review(repo_dir, prompt) + if not success: + raise ReviewExecutionError(error_message) + return result + + +def run_reviewers( + runners: Mapping[str, Any], repo_dir: Path, prompt: str +) -> Dict[str, Dict[str, Any]]: + """Run configured reviewers concurrently, or directly for one reviewer.""" + if not runners: + raise ReviewConfigurationError("At least one reviewer runner is required") + + if len(runners) == 1: + name, runner = next(iter(runners.items())) + result = _run_one(runner, repo_dir, prompt) + return {name: validate_review_result(result, name)} + + results: Dict[str, Dict[str, Any]] = {} + errors = [] + with ThreadPoolExecutor(max_workers=len(runners)) as executor: + futures = { + executor.submit(_run_one, runner, repo_dir, prompt): name + for name, runner in runners.items() + } + for future in as_completed(futures): + name = futures[future] + try: + results[name] = validate_review_result(future.result(), name) + except Exception as error: # Preserve all failures before returning. + errors.append(f"{name}: {error}") + + if errors: + raise ReviewExecutionError("; ".join(errors)) + return {name: results[name] for name in runners} + + +def _with_source_labels(results: Mapping[str, Dict[str, Any]]) -> Dict[str, Any]: + labeled = {} + for provider, result in results.items(): + provider_result = deepcopy(result) + for finding in provider_result.get("findings", []): + if isinstance(finding, dict): + finding["sources"] = [provider] + labeled[provider] = provider_result + return labeled + + +def build_synthesis_prompt(results: Mapping[str, Dict[str, Any]]) -> str: + """Build the one-shot prompt that combines provider JSON into the same schema.""" + labeled_results = _with_source_labels(results) + return f"""You are the final code-review synthesizer. + +Combine the provider review documents below into one final review document. +Return only JSON matching the configured review-output schema. + +Rules: +- Inspect the repository and current diff when needed to adjudicate a finding. +- Merge findings that describe the same underlying problem. +- Preserve a supported finding even if only one provider found it. +- Reject speculative or unsupported findings. +- Reconcile severity, wording, line anchors, and recommendations using the code as evidence. +- Do not invent a finding without a provider source. +- For every final finding, set sources to the provider names that contributed to it. +- Produce one coherent pr_summary; do not mention the synthesis process in public wording. + +Provider review documents: +{json.dumps(labeled_results, indent=2, sort_keys=True)} +""" + + +def synthesize_reviews( + results: Mapping[str, Dict[str, Any]], synthesizer: Any, repo_dir: Path +) -> Dict[str, Any]: + """Return one provider result unchanged, or synthesize multiple results.""" + if len(results) == 1: + # This is the compatibility path: Claude-only output is not rewritten. + return next(iter(results.values())) + prompt = build_synthesis_prompt(results) + result = _run_one(synthesizer, repo_dir, prompt) + return validate_review_result(result, "synthesizer") diff --git a/claudecode/review_schema.py b/claudecode/review_schema.py index cea9204..dbbd442 100644 --- a/claudecode/review_schema.py +++ b/claudecode/review_schema.py @@ -1,49 +1,8 @@ -"""JSON Schema for Claude Code review output.""" +"""Shared JSON schema for every reviewer and the review synthesizer.""" -# JSON Schema for validating review output structure -REVIEW_OUTPUT_SCHEMA = { - "type": "object", - "properties": { - "pr_summary": { - "type": "object", - "properties": { - "overview": {"type": "string"}, - "file_changes": { - "type": "array", - "items": { - "type": "object", - "properties": { - "label": {"type": "string"}, - "files": {"type": "array", "items": {"type": "string"}}, - "changes": {"type": "string"} - }, - "required": ["label", "files", "changes"] - } - } - }, - "required": ["overview", "file_changes"] - }, - "findings": { - "type": "array", - "items": { - "type": "object", - "properties": { - "file": {"type": "string"}, - "line": {"type": "integer"}, - "severity": {"type": "string", "enum": ["HIGH", "MEDIUM", "LOW"]}, - "category": {"type": "string"}, - "title": {"type": "string"}, - "description": {"type": "string"}, - "impact": {"type": "string"}, - "recommendation": {"type": "string"}, - "suggestion": {"type": "string"}, - "suggestion_start_line": {"type": "integer"}, - "suggestion_end_line": {"type": "integer"}, - "confidence": {"type": "number", "minimum": 0, "maximum": 1} - }, - "required": ["file", "line", "severity", "category", "title", "description", "impact", "recommendation", "confidence"] - } - } - }, - "required": ["pr_summary", "findings"] -} \ No newline at end of file +import json +from pathlib import Path + + +REVIEW_OUTPUT_SCHEMA_PATH = Path(__file__).with_name("review-output.schema.json") +REVIEW_OUTPUT_SCHEMA = json.loads(REVIEW_OUTPUT_SCHEMA_PATH.read_text(encoding="utf-8")) diff --git a/claudecode/test_codex_runner.py b/claudecode/test_codex_runner.py new file mode 100644 index 0000000..232acdd --- /dev/null +++ b/claudecode/test_codex_runner.py @@ -0,0 +1,128 @@ +"""Tests for the non-interactive OpenAI Codex reviewer.""" + +import json +import os +from unittest.mock import Mock, patch + +from claudecode.github_action_audit import ( + SimpleClaudeRunner, + SimpleCodexRunner, + initialize_review_runners, + write_provider_results, +) +from claudecode.constants import ( + DEFAULT_GPT_MODEL, + DEFAULT_SYNTHESIZER_MODEL, + DEFAULT_SYNTHESIZER_PROVIDER, +) + + +RESULT = { + "pr_summary": {"overview": "Reviewed", "file_changes": []}, + "findings": [], +} + + +@patch("subprocess.run") +def test_validate_codex_available_requires_cli_and_key(mock_run): + mock_run.return_value = Mock(returncode=0, stdout="codex-cli 1.0", stderr="") + with patch.dict(os.environ, {"CODEX_API_KEY": "key"}, clear=True): + assert SimpleCodexRunner("gpt-5.6-sol").validate_codex_available() == ( + True, + "", + ) + + with patch.dict(os.environ, {}, clear=True): + available, error = SimpleCodexRunner( + "gpt-5.6-sol" + ).validate_codex_available() + assert available is False + assert "CODEX_API_KEY" in error + + +@patch("subprocess.run") +def test_codex_runner_emits_shared_json_and_scopes_secrets(mock_run, tmp_path): + def run_command(command, **kwargs): + output_index = command.index("--output-last-message") + 1 + with open(command[output_index], "w", encoding="utf-8") as output: + json.dump(RESULT, output) + return Mock(returncode=0, stdout="", stderr="") + + mock_run.side_effect = run_command + environment = { + "CODEX_API_KEY": "openai-key", + "ANTHROPIC_API_KEY": "anthropic-key", + "GITHUB_TOKEN": "github-token", + "GH_TOKEN": "gh-token", + "OPENAI_API_KEY": "legacy-openai-key", + } + with patch.dict(os.environ, environment, clear=True): + success, error, result = SimpleCodexRunner("gpt-5.6-sol").run_code_review( + tmp_path, "review this" + ) + + assert success is True + assert error == "" + assert result == RESULT + command = mock_run.call_args.args[0] + assert command[:2] == ["codex", "exec"] + assert "--ephemeral" in command + assert "--ignore-user-config" in command + assert "--ignore-rules" in command + assert command[command.index("--sandbox") + 1] == "read-only" + assert command[command.index("--model") + 1] == "gpt-5.6-sol" + child_env = mock_run.call_args.kwargs["env"] + assert child_env["CODEX_API_KEY"] == "openai-key" + assert "ANTHROPIC_API_KEY" not in child_env + assert "GITHUB_TOKEN" not in child_env + assert "GH_TOKEN" not in child_env + assert "OPENAI_API_KEY" not in child_env + + +def test_claude_only_configuration_does_not_initialize_codex(): + with patch.dict(os.environ, {"REVIEWERS": "claude"}, clear=True): + names, runners, synthesizer = initialize_review_runners() + + assert names == ("claude",) + assert isinstance(runners["claude"], SimpleClaudeRunner) + assert synthesizer is None + + +def test_default_openai_and_synthesizer_configuration_uses_named_constants(): + with patch.dict( + os.environ, {"REVIEWERS": "claude,openai"}, clear=True + ): + _, runners, synthesizer = initialize_review_runners() + + assert runners["openai"].model == DEFAULT_GPT_MODEL + assert DEFAULT_SYNTHESIZER_PROVIDER == "openai" + assert isinstance(synthesizer, SimpleCodexRunner) + assert synthesizer.model == DEFAULT_SYNTHESIZER_MODEL + + +def test_two_reviewers_initialize_configured_synthesizer(): + environment = { + "REVIEWERS": "claude,openai", + "OPENAI_MODEL": "review-model", + "SYNTHESIZER_PROVIDER": "openai", + "SYNTHESIZER_MODEL": "synthesis-model", + } + with patch.dict(os.environ, environment, clear=True): + names, runners, synthesizer = initialize_review_runners() + + assert names == ("claude", "openai") + assert isinstance(runners["claude"], SimpleClaudeRunner) + assert isinstance(runners["openai"], SimpleCodexRunner) + assert runners["openai"].model == "review-model" + assert isinstance(synthesizer, SimpleCodexRunner) + assert synthesizer.model == "synthesis-model" + + +def test_provider_results_are_written_as_complete_json_documents(tmp_path): + with patch.dict( + os.environ, {"PROVIDER_RESULTS_DIR": str(tmp_path)}, clear=True + ): + write_provider_results({"claude": RESULT, "synthesized": RESULT}) + + assert json.loads((tmp_path / "claude.json").read_text()) == RESULT + assert json.loads((tmp_path / "synthesized.json").read_text()) == RESULT diff --git a/claudecode/test_review_ensemble.py b/claudecode/test_review_ensemble.py new file mode 100644 index 0000000..4f2b15b --- /dev/null +++ b/claudecode/test_review_ensemble.py @@ -0,0 +1,134 @@ +"""Tests for provider-neutral review orchestration.""" + +import json +import time + +import pytest + +from claudecode.review_ensemble import ( + ReviewConfigurationError, + ReviewExecutionError, + build_synthesis_prompt, + parse_reviewers, + run_reviewers, + synthesize_reviews, + validate_synthesizer, +) +from claudecode.review_schema import REVIEW_OUTPUT_SCHEMA, REVIEW_OUTPUT_SCHEMA_PATH + + +RESULT = { + "pr_summary": {"overview": "Summary", "file_changes": []}, + "findings": [], +} + + +class FakeRunner: + def __init__(self, result=None, delay=0, error=""): + self.result = result or RESULT + self.delay = delay + self.error = error + self.calls = [] + + def run_code_review(self, repo_dir, prompt): + self.calls.append((repo_dir, prompt)) + time.sleep(self.delay) + if self.error: + return False, self.error, {} + return True, "", self.result + + +def test_parse_reviewers_defaults_to_claude_and_deduplicates(): + assert parse_reviewers("") == ("claude",) + assert parse_reviewers(" claude, openai,claude ") == ("claude", "openai") + + +def test_parse_reviewers_rejects_unknown_provider(): + with pytest.raises(ReviewConfigurationError, match="Unsupported reviewer"): + parse_reviewers("claude,mystery") + + +def test_validate_synthesizer_requires_supported_provider_and_model(): + assert validate_synthesizer("openai", "gpt-5.6-terra") == ( + "openai", + "gpt-5.6-terra", + ) + with pytest.raises(ReviewConfigurationError): + validate_synthesizer("mystery", "model") + with pytest.raises(ReviewConfigurationError): + validate_synthesizer("openai", "") + + +def test_single_reviewer_result_is_returned_unchanged(tmp_path): + runner = FakeRunner(result=RESULT) + results = run_reviewers({"claude": runner}, tmp_path, "review") + final = synthesize_reviews(results, None, tmp_path) + assert final is RESULT + assert runner.calls == [(tmp_path, "review")] + + +def test_multiple_reviewers_run_concurrently_and_are_synthesized(tmp_path): + source_result = { + "pr_summary": {"overview": "Summary", "file_changes": []}, + "findings": [ + { + "file": "example.py", + "line": 1, + "severity": "MEDIUM", + "category": "correctness", + "title": "Issue", + "description": "Description", + "impact": "Impact", + "recommendation": "Recommendation", + "confidence": 0.9, + } + ], + } + claude = FakeRunner(result=source_result, delay=0.08) + openai = FakeRunner(result=source_result, delay=0.08) + combined = { + "pr_summary": {"overview": "Combined", "file_changes": []}, + "findings": [], + } + synthesizer = FakeRunner(result=combined) + + started = time.monotonic() + results = run_reviewers( + {"claude": claude, "openai": openai}, tmp_path, "review" + ) + elapsed = time.monotonic() - started + final = synthesize_reviews(results, synthesizer, tmp_path) + + assert elapsed < 0.14 + assert final == combined + synthesis_prompt = synthesizer.calls[0][1] + synthesis_payload = json.loads( + synthesis_prompt.split("Provider review documents:\n", 1)[1] + ) + assert synthesis_payload["claude"]["findings"][0]["sources"] == ["claude"] + assert synthesis_payload["openai"]["findings"][0]["sources"] == ["openai"] + + +def test_reviewer_failure_fails_the_ensemble(tmp_path): + with pytest.raises(ReviewExecutionError, match="openai: unavailable"): + run_reviewers( + { + "claude": FakeRunner(), + "openai": FakeRunner(error="unavailable"), + }, + tmp_path, + "review", + ) + + +def test_synthesis_prompt_contains_valid_provider_json(): + prompt = build_synthesis_prompt({"claude": RESULT, "openai": RESULT}) + encoded = prompt.split("Provider review documents:\n", 1)[1] + payload = json.loads(encoded) + assert set(payload) == {"claude", "openai"} + + +def test_shared_schema_is_loaded_from_the_committed_json_file(): + assert json.loads(REVIEW_OUTPUT_SCHEMA_PATH.read_text(encoding="utf-8")) == ( + REVIEW_OUTPUT_SCHEMA + ) diff --git a/scripts/comment-pr-findings.bun.test.js b/scripts/comment-pr-findings.bun.test.js index 3221d31..1461ccf 100644 --- a/scripts/comment-pr-findings.bun.test.js +++ b/scripts/comment-pr-findings.bun.test.js @@ -99,6 +99,92 @@ describe('comment-pr-findings.js', () => { }); describe('Finding Processing', () => { + test('should publish directly from the shared review result JSON', async () => { + process.env.REVIEW_RESULTS_FILE = 'final-review.json'; + readFileSyncSpy.mockImplementation((path) => { + if (path.includes('github-event.json')) { + return JSON.stringify({ + pull_request: { number: 123, head: { sha: 'abc123' } } + }); + } + if (path === 'final-review.json') { + return JSON.stringify({ + pr_summary: { + overview: 'Provider-neutral summary', + file_changes: [ + { label: 'test.py', files: ['test.py'], changes: 'Changed behavior' } + ] + }, + findings: [] + }); + } + }); + + let reviewDataCaptured = null; + spawnSyncSpy.mockImplementation((cmd, args, options) => { + if (cmd === 'gh' && args.includes('api')) { + const endpoint = args[1]; + const method = args[args.indexOf('--method') + 1] || 'GET'; + if (endpoint.includes('/pulls/123/reviews') && method === 'POST') { + reviewDataCaptured = JSON.parse(options.input); + } + return { status: 0, stdout: '{}', stderr: '' }; + } + return { status: 0, stdout: '{}', stderr: '' }; + }); + + await import('./comment-pr-findings.js'); + + expect(reviewDataCaptured.body).toContain('Provider-neutral summary'); + expect(reviewDataCaptured.body).toContain('1 file reviewed'); + expect(reviewDataCaptured.body).toContain('No issues found'); + }); + + test('should derive the review verdict from shared-result findings', async () => { + process.env.REVIEW_RESULTS_FILE = 'final-review.json'; + readFileSyncSpy.mockImplementation((path) => { + if (path.includes('github-event.json')) { + return JSON.stringify({ + pull_request: { number: 123, head: { sha: 'abc123' } } + }); + } + if (path === 'final-review.json') { + return JSON.stringify({ + pr_summary: { overview: 'Summary', file_changes: [] }, + findings: [{ + file: 'test.py', + line: 1, + severity: 'HIGH', + category: 'correctness', + title: 'Bug', + description: 'Broken behavior' + }] + }); + } + }); + + let reviewDataCaptured = null; + spawnSyncSpy.mockImplementation((cmd, args, options) => { + if (cmd === 'gh' && args.includes('api')) { + const endpoint = args[1]; + const method = args[args.indexOf('--method') + 1] || 'GET'; + if (endpoint.includes('/pulls/123/files')) { + return { status: 0, stdout: '[]', stderr: '' }; + } + if (endpoint.includes('/pulls/123/reviews') && method === 'POST') { + reviewDataCaptured = JSON.parse(options.input); + } + return { status: 0, stdout: '{}', stderr: '' }; + } + return { status: 0, stdout: '{}', stderr: '' }; + }); + + await import('./comment-pr-findings.js'); + + expect(reviewDataCaptured.event).toBe('REQUEST_CHANGES'); + expect(reviewDataCaptured.body).toContain('high-severity'); + }); + test('should exit early when no findings file exists', async () => { readFileSyncSpy.mockImplementation((path) => { if (path.includes('github-event.json')) { diff --git a/scripts/comment-pr-findings.js b/scripts/comment-pr-findings.js index 8f09028..5a54e4a 100755 --- a/scripts/comment-pr-findings.js +++ b/scripts/comment-pr-findings.js @@ -1,7 +1,7 @@ #!/usr/bin/env node /** - * Script to comment on PRs with code review findings from ClaudeCode + * Provider-neutral publisher for a structured code-review result. */ const fs = require('fs'); @@ -215,33 +215,46 @@ function formatPrSummary(prSummary, filesReviewed) { async function run() { try { - // Read the findings let newFindings = []; - try { - const findingsData = fs.readFileSync('findings.json', 'utf8'); - newFindings = JSON.parse(findingsData); - } catch (e) { - console.log('Could not read findings file'); - return; - } - - // Read the PR summary let prSummary = null; - try { - const summaryData = fs.readFileSync('pr-summary.json', 'utf8'); - prSummary = JSON.parse(summaryData); - } catch (e) { - console.log('Could not read PR summary file, continuing without it'); - } - - // Read the analysis summary (required - contains files_reviewed and severity counts) let analysisSummary; - try { - const analysisData = fs.readFileSync('analysis-summary.json', 'utf8'); - analysisSummary = JSON.parse(analysisData); - } catch (e) { - console.log('Could not read analysis summary file'); - return; + const reviewResultsFile = process.env.REVIEW_RESULTS_FILE; + + if (reviewResultsFile) { + const reviewResult = JSON.parse(fs.readFileSync(reviewResultsFile, 'utf8')); + if (!Array.isArray(reviewResult.findings) || typeof reviewResult.pr_summary !== 'object') { + throw new Error(`Review result ${reviewResultsFile} does not match the shared schema`); + } + newFindings = reviewResult.findings; + prSummary = reviewResult.pr_summary; + analysisSummary = reviewResult.analysis_summary || {}; + + if (analysisSummary.files_reviewed === undefined) { + const reviewedFiles = new Set(); + for (const change of prSummary.file_changes || []) { + for (const file of change.files || []) reviewedFiles.add(file); + } + analysisSummary.files_reviewed = reviewedFiles.size; + } + } else { + // Backward-compatible file layout for existing external callers. + try { + newFindings = JSON.parse(fs.readFileSync('findings.json', 'utf8')); + } catch (e) { + console.log('Could not read findings file'); + return; + } + try { + prSummary = JSON.parse(fs.readFileSync('pr-summary.json', 'utf8')); + } catch (e) { + console.log('Could not read PR summary file, continuing without it'); + } + try { + analysisSummary = JSON.parse(fs.readFileSync('analysis-summary.json', 'utf8')); + } catch (e) { + console.log('Could not read analysis summary file'); + return; + } } function buildReviewSummary(findings, prSummaryObj, analysisSummaryObj) { @@ -285,9 +298,9 @@ async function run() { // Build the findings summary body += `Found ${total} ${issueTypes} issue${total === 1 ? '' : 's'}. `; - // Recommendation based on severity from analysis summary - const high = analysisSummaryObj.high_severity || 0; - const medium = analysisSummaryObj.medium_severity || 0; + // Findings are the source of truth for severity and the review verdict. + const high = findings.filter(f => f.severity === 'HIGH').length; + const medium = findings.filter(f => f.severity === 'MEDIUM').length; if (high > 0) { body += 'Please address the high-severity issues before merging.'; @@ -300,7 +313,7 @@ async function run() { return body; } - const highSeverityCount = analysisSummary.high_severity || 0; + const highSeverityCount = newFindings.filter(f => f.severity === 'HIGH').length; const reviewEvent = COMMENT_ONLY_MODE ? 'COMMENT' : (highSeverityCount > 0 ? 'REQUEST_CHANGES' : 'APPROVE'); @@ -309,11 +322,11 @@ async function run() { // Prepare review comments const reviewComments = []; - // Check if ClaudeCode comments should be silenced + // Check if inline review comments should be silenced const silenceClaudeCodeComments = process.env.SILENCE_CLAUDECODE_COMMENTS === 'true'; if (silenceClaudeCodeComments) { - console.log(`ClaudeCode comments silenced - excluding ${newFindings.length} findings from inline comments`); + console.log(`Inline comments silenced - excluding ${newFindings.length} findings from inline comments`); } let fileMap = {};