From 36e8cc47d934ca798d92637079ba39d101eba164 Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Sat, 28 Mar 2026 19:59:35 -0400 Subject: [PATCH] =?UTF-8?q?fix(ai-review):=20address=20all=20review=20find?= =?UTF-8?q?ings=20=E2=80=94=20restore=20allowedTools,=20add=20fallbacks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from ai-review bot feedback on PR #838: - [HIGH] Restore --allowedTools with Agent added (was incorrectly removed, not redundant with bypassPermissions — it's a tool whitelist) - [MED] Add inline fallback prompts when subagent files missing from base branch - [MED] Add graceful degradation in orchestrator for Agent tool failures and empty subagent prompts - [MED] Replace hardcoded PROMPT_EOF delimiters with random openssl-generated delimiters to prevent early heredoc termination - [LOW] Add per-subagent turn budget guidance (8-10 turns each, 50 total) - [LOW] Add "Do NOT fetch diff separately" instruction to all 4 subagent prompts --- .github/review-prompt.md | 7 ++++- .github/review-prompts/adversarial.md | 1 + .github/review-prompts/ccs-compliance.md | 1 + .github/review-prompts/quality.md | 1 + .github/review-prompts/security.md | 1 + .github/workflows/ai-review.yml | 40 +++++++++++++++++++----- 6 files changed, 42 insertions(+), 9 deletions(-) diff --git a/.github/review-prompt.md b/.github/review-prompt.md index cd2f8677..c4664c61 100644 --- a/.github/review-prompt.md +++ b/.github/review-prompt.md @@ -2,7 +2,10 @@ You are a review orchestrator. Your ONLY job is to dispatch subagent reviewers and merge their findings. -**MANDATORY RULE: You MUST use the Agent tool to spawn subagent reviewers for all standard and deep PRs. You are FORBIDDEN from reviewing code yourself — delegate ALL review work to subagents. The ONLY exception is trivial PRs (<=2 files, <=30 lines, no sensitive paths).** +**MANDATORY RULE: You MUST use the Agent tool to spawn subagent reviewers for all standard and deep PRs. Delegate ALL review work to subagents. The ONLY exceptions where you may review directly:** +- **Trivial PRs** (<=2 files, <=30 lines, no sensitive paths) +- **Agent tool unavailable** — if Agent tool calls fail or error, fall back to reviewing the diff yourself using the same checklist areas. Add a note at the top of your review: "⚠️ Subagent dispatch failed — falling back to single-agent review." +- **Empty subagent prompts** — if all `<*-review-prompt>` XML tags are empty, review directly and note: "⚠️ Subagent prompts not yet available on base branch — using single-agent review." Your workflow: 1. Triage the PR scope @@ -35,6 +38,8 @@ Read the diff ONCE with `gh pr diff`, then spawn all 3 agents in a SINGLE respon Do NOT make each agent read the diff separately — pass it in their prompt. +**Turn budget:** Each subagent should complete within 8-10 turns. The total budget is 50 turns shared across all agents + orchestration. If a subagent hasn't completed after 10 turns, proceed with whatever output it has produced. + ## Step 3: Adversarial Review (Sequential) After ALL 3 parallel reviewers complete, spawn ONE more subagent: diff --git a/.github/review-prompts/adversarial.md b/.github/review-prompts/adversarial.md index ab021b62..00f5761b 100644 --- a/.github/review-prompts/adversarial.md +++ b/.github/review-prompts/adversarial.md @@ -1,6 +1,7 @@ # Adversarial Red-Team Review Prompt You are an adversarial code reviewer. Your ONLY job is to find what 3 prior reviewers (security, quality, CCS compliance) MISSED. DO NOT repeat findings already reported by prior reviewers -- those are provided as context. Focus on ADDED/MODIFIED lines (+ prefix). DO NOT praise the code. ONLY report problems. +The full PR diff is provided at the end of this prompt. Do NOT fetch the diff separately — use what is provided. ## Context diff --git a/.github/review-prompts/ccs-compliance.md b/.github/review-prompts/ccs-compliance.md index fc9ca789..e58e476a 100644 --- a/.github/review-prompts/ccs-compliance.md +++ b/.github/review-prompts/ccs-compliance.md @@ -1,6 +1,7 @@ # CCS Project Compliance Review Prompt You are a CCS project compliance reviewer. Verify adherence to CCS-specific rules and conventions. These are project-specific constraints -- violations are automatic findings. Focus on ADDED/MODIFIED lines (+ prefix). +The full PR diff is provided at the end of this prompt. Do NOT fetch the diff separately — use what is provided. ## CCS Rules (ALL 12 must be checked) diff --git a/.github/review-prompts/quality.md b/.github/review-prompts/quality.md index 8997deab..ca84541b 100644 --- a/.github/review-prompts/quality.md +++ b/.github/review-prompts/quality.md @@ -1,6 +1,7 @@ # Code Quality & Correctness Review Prompt You are a code quality reviewer. Focus on correctness, robustness, and performance in the provided diff. Focus on ADDED/MODIFIED lines (+ prefix). +The full PR diff is provided at the end of this prompt. Do NOT fetch the diff separately — use what is provided. ## Checklist Areas diff --git a/.github/review-prompts/security.md b/.github/review-prompts/security.md index 21daaf74..2258361c 100644 --- a/.github/review-prompts/security.md +++ b/.github/review-prompts/security.md @@ -1,6 +1,7 @@ # Security & Injection Review Prompt You are a security-focused code reviewer. Analyze ONLY security concerns in the provided diff. Focus on ADDED/MODIFIED lines (+ prefix). Pre-existing code is out of scope unless the change makes it newly exploitable. +The full PR diff is provided at the end of this prompt. Do NOT fetch the diff separately — use what is provided. ## Checklist Areas diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index bb19bc2a..9aca3631 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -219,25 +219,48 @@ jobs: # Load subagent prompts (all from base branch for security) SECURITY_PROMPT=$(git show "origin/${BASE_REF}:.github/review-prompts/security.md" 2>/dev/null || echo "") + if [ -z "$SECURITY_PROMPT" ]; then + echo "::warning::security.md not found on base branch — using inline fallback" + SECURITY_PROMPT="You are a security reviewer. Check the diff for injection vulnerabilities, auth bypasses, race conditions, secrets exposure, and supply chain risks. Report findings as: #### [HIGH|MEDIUM|LOW] [SECURITY] file:line" + fi + QUALITY_PROMPT=$(git show "origin/${BASE_REF}:.github/review-prompts/quality.md" 2>/dev/null || echo "") + if [ -z "$QUALITY_PROMPT" ]; then + echo "::warning::quality.md not found on base branch — using inline fallback" + QUALITY_PROMPT="You are a code quality reviewer. Check for error handling gaps, false assumptions, performance issues, dead code, and test gaps. Report findings as: #### [HIGH|MEDIUM|LOW] [QUALITY] file:line" + fi + CCS_PROMPT=$(git show "origin/${BASE_REF}:.github/review-prompts/ccs-compliance.md" 2>/dev/null || echo "") + if [ -z "$CCS_PROMPT" ]; then + echo "::warning::ccs-compliance.md not found on base branch — using inline fallback" + CCS_PROMPT="You are a CCS compliance reviewer. Check for: no emojis in CLI output, getCcsDir() usage, cross-platform parity, --help updates, string-only settings, conventional commits. Report findings as: #### [HIGH|MEDIUM|LOW] [CCS] file:line" + fi + ADVERSARIAL_PROMPT=$(git show "origin/${BASE_REF}:.github/review-prompts/adversarial.md" 2>/dev/null || echo "") + if [ -z "$ADVERSARIAL_PROMPT" ]; then + echo "::warning::adversarial.md not found on base branch — using inline fallback" + ADVERSARIAL_PROMPT="You are an adversarial reviewer. Find what prior reviewers missed: interaction bugs, implicit coupling, missing rollback, boundary violations. Do NOT repeat prior findings. Report as: #### [HIGH|MEDIUM|LOW] [ADVERSARIAL] file:line" + fi - echo "security_prompt<> "$GITHUB_OUTPUT" + SECURITY_DELIM="SECURITY_$(openssl rand -hex 16)" + echo "security_prompt<<${SECURITY_DELIM}" >> "$GITHUB_OUTPUT" echo "$SECURITY_PROMPT" >> "$GITHUB_OUTPUT" - echo "PROMPT_EOF" >> "$GITHUB_OUTPUT" + echo "${SECURITY_DELIM}" >> "$GITHUB_OUTPUT" - echo "quality_prompt<> "$GITHUB_OUTPUT" + QUALITY_DELIM="QUALITY_$(openssl rand -hex 16)" + echo "quality_prompt<<${QUALITY_DELIM}" >> "$GITHUB_OUTPUT" echo "$QUALITY_PROMPT" >> "$GITHUB_OUTPUT" - echo "PROMPT_EOF" >> "$GITHUB_OUTPUT" + echo "${QUALITY_DELIM}" >> "$GITHUB_OUTPUT" - echo "ccs_prompt<> "$GITHUB_OUTPUT" + CCS_DELIM="CCS_$(openssl rand -hex 16)" + echo "ccs_prompt<<${CCS_DELIM}" >> "$GITHUB_OUTPUT" echo "$CCS_PROMPT" >> "$GITHUB_OUTPUT" - echo "PROMPT_EOF" >> "$GITHUB_OUTPUT" + echo "${CCS_DELIM}" >> "$GITHUB_OUTPUT" - echo "adversarial_prompt<> "$GITHUB_OUTPUT" + ADVERSARIAL_DELIM="ADVERSARIAL_$(openssl rand -hex 16)" + echo "adversarial_prompt<<${ADVERSARIAL_DELIM}" >> "$GITHUB_OUTPUT" echo "$ADVERSARIAL_PROMPT" >> "$GITHUB_OUTPUT" - echo "PROMPT_EOF" >> "$GITHUB_OUTPUT" + echo "${ADVERSARIAL_DELIM}" >> "$GITHUB_OUTPUT" - name: Run Claude Code Review id: claude-review @@ -300,6 +323,7 @@ jobs: --model ${{ env.REVIEW_MODEL }} --permission-mode bypassPermissions --max-turns 50 + --allowedTools "Agent,Glob,Grep,Read,Write,Bash(gh pr diff *),Bash(gh pr view *),Bash(git diff *),Bash(git log *),Bash(git show *),Bash(cat *),Bash(ls *),Bash(wc *),Bash(head *),Bash(tail *),Bash(find *)" # Fallback: if Claude didn't write the review file, extract from execution output - name: Extract review from execution output (fallback)