mirror of
https://github.com/tiennm99/ccs.git
synced 2026-07-15 22:21:20 +00:00
fix(ci): harden ai review output
- replace raw assistant-text fallback with structured output normalization - simplify the review prompt toward a tighter findings-only contract - add tests for malformed output, safe fallback, and markdown escaping
This commit is contained in:
Tam Nhu Tran
+33
-124
@@ -1,136 +1,45 @@
|
||||
# Adversarial Code Review Prompt
|
||||
# PR Review Prompt
|
||||
|
||||
You are a red-team code reviewer. Your job is to find every way this code can fail, be exploited, or produce incorrect results. Assume the implementer made mistakes. Prove it.
|
||||
You are a pull request reviewer. Focus on correctness, security, regressions, and missing verification.
|
||||
|
||||
DO NOT start with strengths or praise. Start with problems. If you genuinely find none after thorough analysis, state why — don't fill space with compliments.
|
||||
Follow the repository `CLAUDE.md` instructions before judging the change.
|
||||
|
||||
Follow the repository's CLAUDE.md for project-specific guidelines and constraints.
|
||||
Review discipline:
|
||||
|
||||
## Review Mindset
|
||||
- Read the full diff first.
|
||||
- Read surrounding code before turning an observation into a finding.
|
||||
- Prefer a short list of real findings over a long list of speculative ones.
|
||||
- If a concern is uncertain after checking the nearby code, omit it.
|
||||
- Do not pad the review with praise or generic best-practice commentary.
|
||||
|
||||
Phase 1 — **Understand**: Read the full diff. Understand what the PR does, what it changes, and what it touches.
|
||||
Core questions:
|
||||
|
||||
Phase 2 — **Attack**: For every changed function, module, or code path, ask:
|
||||
- How can this be null/undefined when the code assumes it isn't?
|
||||
- What happens if an external call fails, times out, or returns unexpected data?
|
||||
- Can user input reach this path unsanitized?
|
||||
- Is there a race condition or ordering assumption?
|
||||
- Does this break existing callers or backward compatibility?
|
||||
- Are there missing error handling paths that silently swallow failures?
|
||||
- Can this change break an existing caller, workflow, or default behavior?
|
||||
- Can null, empty, or unexpected external data reach a path that assumes success?
|
||||
- Does untrusted input reach a risky boundary such as shell, file paths, HTTP requests, or HTML?
|
||||
- Is there an ordering, race, or stale-state assumption that can fail under real usage?
|
||||
- Are tests, docs, or `--help` updates missing for newly introduced behavior?
|
||||
|
||||
Phase 3 — **Verify**: Cross-check findings against the actual codebase (not just the diff). Read surrounding code to confirm whether a finding is real or a false positive.
|
||||
CCS-specific checks:
|
||||
|
||||
## Scope-Aware Review Depth
|
||||
- CLI output in `src/` must stay ASCII-only: `[OK]`, `[!]`, `[X]`, `[i]`
|
||||
- CCS path access must use `getCcsDir()`, not `os.homedir()` plus `.ccs`
|
||||
- CLI behavior changes require matching `--help` and docs updates
|
||||
- Terminal color output must respect TTY detection and `NO_COLOR`
|
||||
- Code must not modify `~/.claude/settings.json` without explicit user action
|
||||
|
||||
Calibrate review depth based on PR scope. DO NOT give a trivial typo fix the same depth as an auth rewrite.
|
||||
Severity guide:
|
||||
|
||||
**Quick review** (changed files <= 2 AND lines <= 30 AND no security-sensitive files):
|
||||
- Focus on correctness only. Skip architecture/performance analysis.
|
||||
- Still check the critical checklists below.
|
||||
- `high`: security issue, data loss, broken release/install flow, or behavior that is likely wrong in normal use
|
||||
- `medium`: meaningful edge case, missing guard, missing test/docs/help update, or maintainability issue that can cause user-facing bugs
|
||||
- `low`: smaller follow-up worth tracking, but not a release blocker
|
||||
|
||||
**Standard review** (most PRs):
|
||||
- Full adversarial analysis across all checklist areas.
|
||||
Output expectations:
|
||||
|
||||
**Deep review** (ANY of these conditions):
|
||||
- Files in: auth/, middleware/, security/, crypto/, commands/, shared/, .github/
|
||||
- New dependencies added (package.json/lockfile changed)
|
||||
- CI/CD workflow files changed
|
||||
- Environment variables added/changed
|
||||
- API routes added/changed
|
||||
- Database schema modified
|
||||
- External contributor PR
|
||||
|
||||
## Security Checklist (MUST Flag If Found)
|
||||
|
||||
- **Injection & Command Safety** — string interpolation in shell commands via child_process (use argument arrays), user input in file paths (path traversal), template literal injection in SQL/DB, unsanitized HTML
|
||||
- **Authentication & Authorization** — missing auth checks on new endpoints, privilege escalation (IDOR), secrets in logs/errors/client code, JWT comparison using == instead of constant-time
|
||||
- **Race Conditions** — read-check-write without atomic ops, shared mutable state without sync, TOCTOU in file ops, async operations with implicit ordering
|
||||
- **Supply Chain** (when deps change) — postinstall scripts, maintainer reputation, lockfile drift, transitive vulns
|
||||
|
||||
## Quality Checklist (MUST Flag If Found)
|
||||
|
||||
- **Error Handling** — swallowed errors (catch {} with no log), missing error handling on spawn/exec, unbounded ops from user input, missing cleanup on error paths, process.exit() without cleanup
|
||||
- **False Assumptions (ACTIVELY HUNT)** — "never null" (prove it can be), "array always has elements" (find empty case), "A before B" (find out-of-order path), "config exists" (find missing env var), "API returns 200" (find failure mode)
|
||||
- **AI-Generated Code** — hallucinated imports, deprecated APIs, over-abstraction, plausible but wrong logic (off-by-one, inverted conditions)
|
||||
- **Performance** — O(n*m) in loops (use Map/Set), missing pagination on unbounded list endpoints, N+1 patterns
|
||||
|
||||
## CCS-Specific Rules (MUST Enforce — Violations Are Automatic Findings)
|
||||
|
||||
1. **NO emojis in CLI output** — src/ code printing to stdout/stderr must use ASCII only: [OK], [!], [X], [i]
|
||||
2. **Test isolation** — code accessing CCS paths MUST use getCcsDir(), NOT os.homedir() + '.ccs'
|
||||
3. **Cross-platform parity** — bash/PowerShell/Node.js must behave identically
|
||||
4. **--help updated** — if CLI command behavior changed, respective help handler must be updated
|
||||
5. **Synchronous fs APIs** — avoid in async paths (tracked by maintainability baseline)
|
||||
6. **Settings format** — all env values MUST be strings (not booleans/objects)
|
||||
7. **Conventional commit** — PR title must follow conventional commit format
|
||||
8. **Non-invasive** — code must NOT modify ~/.claude/settings.json without explicit user confirmation
|
||||
9. **TTY-aware colors** — respect NO_COLOR env var; detect TTY before using colors
|
||||
10. **Idempotent installs** — all install/setup ops must be safe to run multiple times
|
||||
11. **Dashboard parity** — configuration features MUST have both CLI and Dashboard interfaces
|
||||
12. **Documentation mandatory** — CLI/config changes require --help update AND docs update
|
||||
|
||||
## Suppressions — DO NOT Flag These
|
||||
|
||||
- Style/formatting issues (linter handles this)
|
||||
- "Consider using X instead of Y" when Y works correctly AND has no security/correctness/CCS implications
|
||||
- Redundancy that aids readability
|
||||
- Issues already addressed in the diff being reviewed (read the FULL diff first)
|
||||
- "Add a comment" suggestions — code should be self-documenting
|
||||
- Harmless no-ops that don't affect correctness
|
||||
- Consistency-only suggestions with no functional impact
|
||||
|
||||
## Output Structure
|
||||
|
||||
### 📋 Summary
|
||||
2-3 sentences describing what the PR does and overall assessment.
|
||||
|
||||
### 🔍 Findings
|
||||
Group by severity. Each finding must include `file:line` reference and concrete explanation.
|
||||
|
||||
**🔴 High** (must fix before merge):
|
||||
- Security vulnerabilities, data corruption risks, breaking changes without migration
|
||||
|
||||
**🟡 Medium** (should fix before merge):
|
||||
- Missing error handling, edge cases, test gaps for new behavior
|
||||
|
||||
**🟢 Low** (track for follow-up):
|
||||
- Minor improvements, non-blocking suggestions with clear rationale
|
||||
|
||||
For each finding:
|
||||
1. **What**: The specific problem
|
||||
2. **Why**: How it can be triggered or why it matters
|
||||
3. **Fix**: Concrete fix approach (describe, don't write implementation code)
|
||||
|
||||
### 🔒 Security Checklist
|
||||
| Check | Status | Notes |
|
||||
|-------|--------|-------|
|
||||
| Injection safety | ✅/❌ | ... |
|
||||
| Auth checks | ✅/❌/N/A | ... |
|
||||
| Race conditions | ✅/❌/N/A | ... |
|
||||
| Secrets exposure | ✅/❌ | ... |
|
||||
| Supply chain | ✅/❌/N/A | ... |
|
||||
|
||||
### 📊 CCS Compliance
|
||||
| Rule | Status | Notes |
|
||||
|------|--------|-------|
|
||||
| No emojis in CLI | ✅/❌/N/A | ... |
|
||||
| Test isolation | ✅/❌/N/A | ... |
|
||||
| Cross-platform | ✅/❌/N/A | ... |
|
||||
| --help updated | ✅/❌/N/A | ... |
|
||||
| Settings strings | ✅/❌/N/A | ... |
|
||||
| Conventional commit | ✅/❌ | ... |
|
||||
| Docs mandatory | ✅/❌/N/A | ... |
|
||||
|
||||
### 💡 Informational
|
||||
Non-blocking observations.
|
||||
|
||||
### ✅ What's Done Well
|
||||
2-3 items max, only if genuinely noteworthy. OPTIONAL — skip if nothing stands out.
|
||||
|
||||
### 🎯 Overall Assessment
|
||||
|
||||
**✅ APPROVED** — zero High, zero security Medium, all CCS rules respected, tests exist for new behavior.
|
||||
**⚠️ APPROVED WITH NOTES** — zero High, only non-security Medium or Low remain, findings documented.
|
||||
**❌ CHANGES REQUESTED** — ANY High, OR security Medium, OR CCS violation, OR missing tests/docs.
|
||||
|
||||
When in doubt between APPROVED WITH NOTES and CHANGES REQUESTED, choose CHANGES REQUESTED.
|
||||
- Return confirmed findings only.
|
||||
- Every finding must cite a file path and, when practical, a line number.
|
||||
- Keep the total finding count small unless the PR genuinely has several distinct problems.
|
||||
- If there are no confirmed findings, say so in the summary and return an empty findings array.
|
||||
- Use `approved` only when the diff is ready to merge as-is.
|
||||
- Use `approved_with_notes` when only non-blocking follow-ups remain.
|
||||
- Use `changes_requested` when any blocking issue remains.
|
||||
|
||||
@@ -151,6 +151,8 @@ jobs:
|
||||
MAX_THINKING_TOKENS: '16000'
|
||||
REVIEW_OUTPUT_FILE: pr_review.md
|
||||
REVIEW_COMMENT_FILE: .ccs-ai-review-comment.md
|
||||
REVIEW_OUTPUT_SCHEMA: >-
|
||||
{"type":"object","additionalProperties":false,"properties":{"summary":{"type":"string","minLength":1},"findings":{"type":"array","maxItems":6,"items":{"type":"object","additionalProperties":false,"properties":{"severity":{"type":"string","enum":["high","medium","low"]},"title":{"type":"string","minLength":1},"file":{"type":"string","minLength":1},"line":{"type":["integer","null"],"minimum":1},"what":{"type":"string","minLength":1},"why":{"type":"string","minLength":1},"fix":{"type":"string","minLength":1}},"required":["severity","title","file","what","why","fix"]}},"overallAssessment":{"type":"string","enum":["approved","approved_with_notes","changes_requested"]},"overallRationale":{"type":"string","minLength":1},"notes":{"type":"array","maxItems":4,"items":{"type":"string","minLength":1}}},"required":["summary","findings","overallAssessment","overallRationale"]}
|
||||
|
||||
steps:
|
||||
- name: Prepare isolated Claude runtime
|
||||
@@ -224,7 +226,8 @@ jobs:
|
||||
anthropic_api_key: ${{ secrets.GLM_API_KEY }}
|
||||
github_token: ${{ steps.app-token.outputs.token }}
|
||||
allowed_non_write_users: ${{ needs.prepare.outputs.contributor_source == 'external' && '*' || '' }}
|
||||
show_full_output: true # Visible logs for debugging slow/failing reviews
|
||||
display_report: false # Keep all public review output on the normalized comment path
|
||||
show_full_output: false # Keep scratch output out of public logs
|
||||
track_progress: false # Disabled - no progress comments, just final review
|
||||
prompt: |
|
||||
think
|
||||
@@ -242,55 +245,32 @@ jobs:
|
||||
|
||||
${{ steps.review-prompt.outputs.content }}
|
||||
|
||||
## IMPORTANT: Writing the Review
|
||||
After completing your analysis, use the `Write` tool to write the final review markdown to `${{ env.REVIEW_OUTPUT_FILE }}`.
|
||||
Do NOT use `Edit` tool — use `Write` tool directly to create the file in one shot.
|
||||
Do NOT post any GitHub comments yourself. The workflow will publish the saved file.
|
||||
Do NOT modify any source code files — this is a READ-ONLY review.
|
||||
|
||||
End your review with:
|
||||
> 🤖 Reviewed by `${{ env.REVIEW_MODEL }}`
|
||||
|
||||
IMPORTANT RULES:
|
||||
- Use `Write` tool to overwrite `${{ env.REVIEW_OUTPUT_FILE }}` with the complete review
|
||||
- Do NOT use shell operators like || or && in bash commands
|
||||
- Do NOT use heredoc (<<) syntax in bash commands
|
||||
- Use simple, single-purpose bash commands only
|
||||
## Runtime Rules
|
||||
- This is a READ-ONLY review. Do not edit files.
|
||||
- Use the checked-out PR branch plus surrounding repository context before reporting a finding.
|
||||
- Return only structured output that matches the provided JSON schema.
|
||||
- Do NOT write files.
|
||||
- Do NOT post GitHub comments yourself.
|
||||
- If no confirmed issues remain, return an empty findings array instead of inventing low-value feedback.
|
||||
|
||||
claude_args: |
|
||||
--bare
|
||||
--model ${{ env.REVIEW_MODEL }}
|
||||
--permission-mode bypassPermissions
|
||||
--max-turns 40
|
||||
--allowedTools "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 *)"
|
||||
--allowedTools "Read,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:*),Bash(grep:*)"
|
||||
--json-schema '${{ env.REVIEW_OUTPUT_SCHEMA }}'
|
||||
|
||||
# Fallback: if Claude didn't write the review file, extract from execution output
|
||||
- name: Extract review from execution output (fallback)
|
||||
- name: Render review comment
|
||||
if: always() && steps.claude-review.outcome != 'cancelled'
|
||||
run: |
|
||||
EXEC_LOG="$RUNNER_TEMP/claude-execution-output.json"
|
||||
if [ -s "$REVIEW_OUTPUT_FILE" ]; then
|
||||
echo "[i] Review file exists, skipping fallback extraction"
|
||||
exit 0
|
||||
fi
|
||||
if [ ! -f "$EXEC_LOG" ]; then
|
||||
echo "::warning::No execution output found at $EXEC_LOG"
|
||||
exit 0
|
||||
fi
|
||||
# Extract last assistant text message as fallback review
|
||||
EXTRACTED=$(jq -r '
|
||||
[.[] | select(.type == "assistant") | .message.content[]?
|
||||
| select(.type == "text") | .text] | last // empty
|
||||
' "$EXEC_LOG" 2>/dev/null || true)
|
||||
if [ -z "$EXTRACTED" ]; then
|
||||
echo "::warning::Could not extract review content from execution output"
|
||||
printf '## AI Review (incomplete)\n\nClaude completed but did not produce a structured review.\nCheck the [execution log artifact](%s) for details.\n\n> Reviewed by `%s` (fallback extraction)\n' \
|
||||
"${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" \
|
||||
"$REVIEW_MODEL" > "$REVIEW_OUTPUT_FILE"
|
||||
else
|
||||
printf '%s\n' "$EXTRACTED" > "$REVIEW_OUTPUT_FILE"
|
||||
fi
|
||||
echo "[i] Fallback review extracted from execution output"
|
||||
node scripts/github/normalize-ai-review-output.mjs
|
||||
env:
|
||||
AI_REVIEW_EXECUTION_FILE: ${{ runner.temp }}/claude-execution-output.json
|
||||
AI_REVIEW_MODEL: ${{ env.REVIEW_MODEL }}
|
||||
AI_REVIEW_OUTPUT_FILE: ${{ env.REVIEW_OUTPUT_FILE }}
|
||||
AI_REVIEW_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
|
||||
AI_REVIEW_STRUCTURED_OUTPUT: ${{ steps.claude-review.outputs.structured_output }}
|
||||
|
||||
- name: Publish review comment
|
||||
if: always() && steps.claude-review.outcome != 'cancelled'
|
||||
|
||||
Reference in New Issue
Block a user