mirror of
https://github.com/tiennm99/ccs.git
synced 2026-09-03 00:17:47 +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:
|
- Can this change break an existing caller, workflow, or default behavior?
|
||||||
- How can this be null/undefined when the code assumes it isn't?
|
- Can null, empty, or unexpected external data reach a path that assumes success?
|
||||||
- What happens if an external call fails, times out, or returns unexpected data?
|
- Does untrusted input reach a risky boundary such as shell, file paths, HTTP requests, or HTML?
|
||||||
- Can user input reach this path unsanitized?
|
- Is there an ordering, race, or stale-state assumption that can fail under real usage?
|
||||||
- Is there a race condition or ordering assumption?
|
- Are tests, docs, or `--help` updates missing for newly introduced behavior?
|
||||||
- Does this break existing callers or backward compatibility?
|
|
||||||
- Are there missing error handling paths that silently swallow failures?
|
|
||||||
|
|
||||||
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):
|
- `high`: security issue, data loss, broken release/install flow, or behavior that is likely wrong in normal use
|
||||||
- Focus on correctness only. Skip architecture/performance analysis.
|
- `medium`: meaningful edge case, missing guard, missing test/docs/help update, or maintainability issue that can cause user-facing bugs
|
||||||
- Still check the critical checklists below.
|
- `low`: smaller follow-up worth tracking, but not a release blocker
|
||||||
|
|
||||||
**Standard review** (most PRs):
|
Output expectations:
|
||||||
- Full adversarial analysis across all checklist areas.
|
|
||||||
|
|
||||||
**Deep review** (ANY of these conditions):
|
- Return confirmed findings only.
|
||||||
- Files in: auth/, middleware/, security/, crypto/, commands/, shared/, .github/
|
- Every finding must cite a file path and, when practical, a line number.
|
||||||
- New dependencies added (package.json/lockfile changed)
|
- Keep the total finding count small unless the PR genuinely has several distinct problems.
|
||||||
- CI/CD workflow files changed
|
- If there are no confirmed findings, say so in the summary and return an empty findings array.
|
||||||
- Environment variables added/changed
|
- Use `approved` only when the diff is ready to merge as-is.
|
||||||
- API routes added/changed
|
- Use `approved_with_notes` when only non-blocking follow-ups remain.
|
||||||
- Database schema modified
|
- Use `changes_requested` when any blocking issue remains.
|
||||||
- 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.
|
|
||||||
|
|||||||
@@ -151,6 +151,8 @@ jobs:
|
|||||||
MAX_THINKING_TOKENS: '16000'
|
MAX_THINKING_TOKENS: '16000'
|
||||||
REVIEW_OUTPUT_FILE: pr_review.md
|
REVIEW_OUTPUT_FILE: pr_review.md
|
||||||
REVIEW_COMMENT_FILE: .ccs-ai-review-comment.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:
|
steps:
|
||||||
- name: Prepare isolated Claude runtime
|
- name: Prepare isolated Claude runtime
|
||||||
@@ -224,7 +226,8 @@ jobs:
|
|||||||
anthropic_api_key: ${{ secrets.GLM_API_KEY }}
|
anthropic_api_key: ${{ secrets.GLM_API_KEY }}
|
||||||
github_token: ${{ steps.app-token.outputs.token }}
|
github_token: ${{ steps.app-token.outputs.token }}
|
||||||
allowed_non_write_users: ${{ needs.prepare.outputs.contributor_source == 'external' && '*' || '' }}
|
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
|
track_progress: false # Disabled - no progress comments, just final review
|
||||||
prompt: |
|
prompt: |
|
||||||
think
|
think
|
||||||
@@ -242,55 +245,32 @@ jobs:
|
|||||||
|
|
||||||
${{ steps.review-prompt.outputs.content }}
|
${{ steps.review-prompt.outputs.content }}
|
||||||
|
|
||||||
## IMPORTANT: Writing the Review
|
## Runtime Rules
|
||||||
After completing your analysis, use the `Write` tool to write the final review markdown to `${{ env.REVIEW_OUTPUT_FILE }}`.
|
- This is a READ-ONLY review. Do not edit files.
|
||||||
Do NOT use `Edit` tool — use `Write` tool directly to create the file in one shot.
|
- Use the checked-out PR branch plus surrounding repository context before reporting a finding.
|
||||||
Do NOT post any GitHub comments yourself. The workflow will publish the saved file.
|
- Return only structured output that matches the provided JSON schema.
|
||||||
Do NOT modify any source code files — this is a READ-ONLY review.
|
- Do NOT write files.
|
||||||
|
- Do NOT post GitHub comments yourself.
|
||||||
End your review with:
|
- If no confirmed issues remain, return an empty findings array instead of inventing low-value feedback.
|
||||||
> 🤖 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
|
|
||||||
|
|
||||||
claude_args: |
|
claude_args: |
|
||||||
--bare
|
--bare
|
||||||
--model ${{ env.REVIEW_MODEL }}
|
--model ${{ env.REVIEW_MODEL }}
|
||||||
--permission-mode bypassPermissions
|
--permission-mode bypassPermissions
|
||||||
--max-turns 40
|
--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: Render review comment
|
||||||
- name: Extract review from execution output (fallback)
|
|
||||||
if: always() && steps.claude-review.outcome != 'cancelled'
|
if: always() && steps.claude-review.outcome != 'cancelled'
|
||||||
run: |
|
run: |
|
||||||
EXEC_LOG="$RUNNER_TEMP/claude-execution-output.json"
|
node scripts/github/normalize-ai-review-output.mjs
|
||||||
if [ -s "$REVIEW_OUTPUT_FILE" ]; then
|
env:
|
||||||
echo "[i] Review file exists, skipping fallback extraction"
|
AI_REVIEW_EXECUTION_FILE: ${{ runner.temp }}/claude-execution-output.json
|
||||||
exit 0
|
AI_REVIEW_MODEL: ${{ env.REVIEW_MODEL }}
|
||||||
fi
|
AI_REVIEW_OUTPUT_FILE: ${{ env.REVIEW_OUTPUT_FILE }}
|
||||||
if [ ! -f "$EXEC_LOG" ]; then
|
AI_REVIEW_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
|
||||||
echo "::warning::No execution output found at $EXEC_LOG"
|
AI_REVIEW_STRUCTURED_OUTPUT: ${{ steps.claude-review.outputs.structured_output }}
|
||||||
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"
|
|
||||||
|
|
||||||
- name: Publish review comment
|
- name: Publish review comment
|
||||||
if: always() && steps.claude-review.outcome != 'cancelled'
|
if: always() && steps.claude-review.outcome != 'cancelled'
|
||||||
|
|||||||
@@ -0,0 +1,204 @@
|
|||||||
|
import fs from 'node:fs';
|
||||||
|
import path from 'node:path';
|
||||||
|
import { fileURLToPath } from 'node:url';
|
||||||
|
|
||||||
|
const ASSESSMENTS = {
|
||||||
|
approved: '✅ APPROVED',
|
||||||
|
approved_with_notes: '⚠️ APPROVED WITH NOTES',
|
||||||
|
changes_requested: '❌ CHANGES REQUESTED',
|
||||||
|
};
|
||||||
|
|
||||||
|
const SEVERITY_ORDER = ['high', 'medium', 'low'];
|
||||||
|
const SEVERITY_HEADERS = {
|
||||||
|
high: '### 🔴 High',
|
||||||
|
medium: '### 🟡 Medium',
|
||||||
|
low: '### 🟢 Low',
|
||||||
|
};
|
||||||
|
|
||||||
|
function cleanText(value) {
|
||||||
|
return typeof value === 'string' ? value.trim().replace(/\s+/g, ' ') : '';
|
||||||
|
}
|
||||||
|
|
||||||
|
function escapeMarkdownText(value) {
|
||||||
|
return cleanText(value).replace(/\\/g, '\\\\').replace(/([`*_{}\[\]<>])/g, '\\$1');
|
||||||
|
}
|
||||||
|
|
||||||
|
function renderCode(value) {
|
||||||
|
const text = cleanText(value);
|
||||||
|
const longestFence = Math.max(...[...text.matchAll(/`+/g)].map((match) => match[0].length), 0);
|
||||||
|
const fence = '`'.repeat(longestFence + 1);
|
||||||
|
return `${fence}${text}${fence}`;
|
||||||
|
}
|
||||||
|
|
||||||
|
function readExecutionMetadata(executionFile) {
|
||||||
|
if (!executionFile || !fs.existsSync(executionFile)) {
|
||||||
|
return {};
|
||||||
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
|
const turns = JSON.parse(fs.readFileSync(executionFile, 'utf8'));
|
||||||
|
const init = turns.find((turn) => turn?.type === 'system' && turn?.subtype === 'init');
|
||||||
|
const result = [...turns].reverse().find((turn) => turn?.type === 'result');
|
||||||
|
return {
|
||||||
|
runtimeTools: Array.isArray(init?.tools) ? init.tools : [],
|
||||||
|
turnsUsed: typeof result?.num_turns === 'number' ? result.num_turns : null,
|
||||||
|
};
|
||||||
|
} catch {
|
||||||
|
return {};
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
export function normalizeStructuredOutput(raw) {
|
||||||
|
if (!raw) {
|
||||||
|
return { ok: false, reason: 'missing structured output' };
|
||||||
|
}
|
||||||
|
|
||||||
|
let parsed;
|
||||||
|
try {
|
||||||
|
parsed = typeof raw === 'string' ? JSON.parse(raw) : raw;
|
||||||
|
} catch {
|
||||||
|
return { ok: false, reason: 'structured output is not valid JSON' };
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
|
||||||
|
return { ok: false, reason: 'structured output must be an object' };
|
||||||
|
}
|
||||||
|
|
||||||
|
const summary = cleanText(parsed.summary);
|
||||||
|
const overallAssessment = cleanText(parsed.overallAssessment);
|
||||||
|
const overallRationale = cleanText(parsed.overallRationale);
|
||||||
|
const notes = Array.isArray(parsed.notes) ? parsed.notes.map(cleanText).filter(Boolean) : [];
|
||||||
|
const findings = Array.isArray(parsed.findings) ? parsed.findings : null;
|
||||||
|
|
||||||
|
if (!summary || !ASSESSMENTS[overallAssessment] || !overallRationale || findings === null) {
|
||||||
|
return { ok: false, reason: 'structured output is missing required review fields' };
|
||||||
|
}
|
||||||
|
|
||||||
|
const normalizedFindings = [];
|
||||||
|
for (const finding of findings) {
|
||||||
|
const severity = cleanText(finding?.severity);
|
||||||
|
const title = cleanText(finding?.title);
|
||||||
|
const file = cleanText(finding?.file);
|
||||||
|
const what = cleanText(finding?.what);
|
||||||
|
const why = cleanText(finding?.why);
|
||||||
|
const fix = cleanText(finding?.fix);
|
||||||
|
const line =
|
||||||
|
typeof finding?.line === 'number' && Number.isInteger(finding.line) && finding.line > 0
|
||||||
|
? finding.line
|
||||||
|
: null;
|
||||||
|
|
||||||
|
if (!SEVERITY_HEADERS[severity] || !title || !file || !what || !why || !fix) {
|
||||||
|
return { ok: false, reason: 'structured output contains an invalid finding' };
|
||||||
|
}
|
||||||
|
|
||||||
|
normalizedFindings.push({ severity, title, file, line, what, why, fix });
|
||||||
|
}
|
||||||
|
|
||||||
|
return {
|
||||||
|
ok: true,
|
||||||
|
value: {
|
||||||
|
summary,
|
||||||
|
findings: normalizedFindings,
|
||||||
|
overallAssessment,
|
||||||
|
overallRationale,
|
||||||
|
notes,
|
||||||
|
},
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
export function renderStructuredReview(review, { model }) {
|
||||||
|
const lines = ['## Summary', escapeMarkdownText(review.summary), '', '## Findings'];
|
||||||
|
|
||||||
|
if (review.findings.length === 0) {
|
||||||
|
lines.push('No confirmed issues found after reviewing the diff and surrounding code.');
|
||||||
|
} else {
|
||||||
|
for (const severity of SEVERITY_ORDER) {
|
||||||
|
const findings = review.findings.filter((finding) => finding.severity === severity);
|
||||||
|
if (findings.length === 0) continue;
|
||||||
|
|
||||||
|
lines.push(SEVERITY_HEADERS[severity]);
|
||||||
|
for (const finding of findings) {
|
||||||
|
const location = finding.line ? `${finding.file}:${finding.line}` : finding.file;
|
||||||
|
lines.push(`- **${renderCode(location)} — ${escapeMarkdownText(finding.title)}**`);
|
||||||
|
lines.push(` Problem: ${escapeMarkdownText(finding.what)}`);
|
||||||
|
lines.push(` Why it matters: ${escapeMarkdownText(finding.why)}`);
|
||||||
|
lines.push(` Suggested fix: ${escapeMarkdownText(finding.fix)}`);
|
||||||
|
}
|
||||||
|
lines.push('');
|
||||||
|
}
|
||||||
|
if (lines[lines.length - 1] === '') {
|
||||||
|
lines.pop();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
if (review.notes.length > 0) {
|
||||||
|
lines.push('', '## Notes');
|
||||||
|
for (const note of review.notes) {
|
||||||
|
lines.push(`- ${escapeMarkdownText(note)}`);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
lines.push(
|
||||||
|
'',
|
||||||
|
'## Overall Assessment',
|
||||||
|
`**${ASSESSMENTS[review.overallAssessment]}** — ${escapeMarkdownText(review.overallRationale)}`,
|
||||||
|
'',
|
||||||
|
`> Reviewed by \`${model}\``
|
||||||
|
);
|
||||||
|
|
||||||
|
return lines.join('\n');
|
||||||
|
}
|
||||||
|
|
||||||
|
export function renderIncompleteReview({ model, reason, runUrl, runtimeTools, turnsUsed }) {
|
||||||
|
const lines = [
|
||||||
|
'## AI Review Incomplete',
|
||||||
|
'',
|
||||||
|
'Claude did not return validated structured review output, so this workflow did not publish raw scratch text.',
|
||||||
|
'',
|
||||||
|
`- Reason: ${escapeMarkdownText(reason)}`,
|
||||||
|
];
|
||||||
|
|
||||||
|
if (runtimeTools?.length) {
|
||||||
|
lines.push(`- Runtime tools: ${runtimeTools.map(renderCode).join(', ')}`);
|
||||||
|
}
|
||||||
|
if (typeof turnsUsed === 'number') {
|
||||||
|
lines.push(`- Turns used: ${turnsUsed}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
lines.push('', `Re-run \`/review\` or inspect [the workflow run](${runUrl}).`, '', `> Reviewed by \`${model}\``);
|
||||||
|
return lines.join('\n');
|
||||||
|
}
|
||||||
|
|
||||||
|
export function writeReviewFromEnv(env = process.env) {
|
||||||
|
const outputFile = env.AI_REVIEW_OUTPUT_FILE || 'pr_review.md';
|
||||||
|
const model = env.AI_REVIEW_MODEL || 'unknown-model';
|
||||||
|
const runUrl = env.AI_REVIEW_RUN_URL || '#';
|
||||||
|
const validation = normalizeStructuredOutput(env.AI_REVIEW_STRUCTURED_OUTPUT);
|
||||||
|
const metadata = readExecutionMetadata(env.AI_REVIEW_EXECUTION_FILE);
|
||||||
|
const content = validation.ok
|
||||||
|
? renderStructuredReview(validation.value, { model })
|
||||||
|
: renderIncompleteReview({
|
||||||
|
model,
|
||||||
|
reason: validation.reason,
|
||||||
|
runUrl,
|
||||||
|
runtimeTools: metadata.runtimeTools,
|
||||||
|
turnsUsed: metadata.turnsUsed,
|
||||||
|
});
|
||||||
|
|
||||||
|
fs.mkdirSync(path.dirname(outputFile), { recursive: true });
|
||||||
|
fs.writeFileSync(outputFile, `${content}\n`, 'utf8');
|
||||||
|
|
||||||
|
if (!validation.ok) {
|
||||||
|
console.warn(`::warning::AI review output normalization fell back to incomplete comment: ${validation.reason}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
return { usedFallback: !validation.ok, content };
|
||||||
|
}
|
||||||
|
|
||||||
|
const isMain =
|
||||||
|
process.argv[1] &&
|
||||||
|
path.resolve(process.argv[1]) === path.resolve(fileURLToPath(import.meta.url));
|
||||||
|
|
||||||
|
if (isMain) {
|
||||||
|
writeReviewFromEnv();
|
||||||
|
}
|
||||||
@@ -0,0 +1,117 @@
|
|||||||
|
import { describe, expect, test } from 'bun:test';
|
||||||
|
import fs from 'node:fs';
|
||||||
|
import os from 'node:os';
|
||||||
|
import path from 'node:path';
|
||||||
|
|
||||||
|
const reviewOutput = await import('../../../../scripts/github/normalize-ai-review-output.mjs');
|
||||||
|
|
||||||
|
describe('normalize-ai-review-output', () => {
|
||||||
|
test('renders validated structured output into stable markdown', () => {
|
||||||
|
const validation = reviewOutput.normalizeStructuredOutput(
|
||||||
|
JSON.stringify({
|
||||||
|
summary: 'The PR is mostly correct, but one blocking regression remains.',
|
||||||
|
findings: [
|
||||||
|
{
|
||||||
|
severity: 'high',
|
||||||
|
title: 'Ambiguous account lookup drops valid matches',
|
||||||
|
file: 'src/cliproxy/accounts/query.ts',
|
||||||
|
line: 61,
|
||||||
|
what: 'Exact email matches can return null when duplicate accounts exist.',
|
||||||
|
why: 'That breaks normal selection flows for users with multiple Codex sessions.',
|
||||||
|
fix: 'Match by stable account identity first and keep ambiguous email lookups out of exact-match paths.',
|
||||||
|
},
|
||||||
|
],
|
||||||
|
overallAssessment: 'changes_requested',
|
||||||
|
overallRationale: 'The blocking lookup regression should be fixed before merge.',
|
||||||
|
notes: ['Docs update is present and looks aligned with the code changes.'],
|
||||||
|
})
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(validation.ok).toBe(true);
|
||||||
|
const markdown = reviewOutput.renderStructuredReview(validation.value, { model: 'glm-5.1' });
|
||||||
|
|
||||||
|
expect(markdown).toContain('## Summary');
|
||||||
|
expect(markdown).toContain('### 🔴 High');
|
||||||
|
expect(markdown).toContain('`src/cliproxy/accounts/query.ts:61`');
|
||||||
|
expect(markdown).toContain('**❌ CHANGES REQUESTED**');
|
||||||
|
expect(markdown).toContain('Why it matters: That breaks normal selection flows for users with multiple Codex sessions.');
|
||||||
|
expect(markdown).toContain('> Reviewed by `glm-5.1`');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('writes a safe incomplete comment instead of leaking raw assistant text', () => {
|
||||||
|
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ai-review-'));
|
||||||
|
const executionFile = path.join(tempDir, 'claude-execution-output.json');
|
||||||
|
const outputFile = path.join(tempDir, 'pr_review.md');
|
||||||
|
|
||||||
|
fs.writeFileSync(
|
||||||
|
executionFile,
|
||||||
|
JSON.stringify([
|
||||||
|
{ type: 'system', subtype: 'init', tools: ['Bash', 'Edit', 'Read'] },
|
||||||
|
{
|
||||||
|
type: 'result',
|
||||||
|
subtype: 'success',
|
||||||
|
num_turns: 25,
|
||||||
|
result: 'Now let me verify the findings before I finalize the review...',
|
||||||
|
},
|
||||||
|
])
|
||||||
|
);
|
||||||
|
|
||||||
|
const result = reviewOutput.writeReviewFromEnv({
|
||||||
|
AI_REVIEW_EXECUTION_FILE: executionFile,
|
||||||
|
AI_REVIEW_MODEL: 'glm-5.1',
|
||||||
|
AI_REVIEW_OUTPUT_FILE: outputFile,
|
||||||
|
AI_REVIEW_RUN_URL: 'https://github.com/kaitranntt/ccs/actions/runs/23758377592',
|
||||||
|
AI_REVIEW_STRUCTURED_OUTPUT: '',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result.usedFallback).toBe(true);
|
||||||
|
|
||||||
|
const markdown = fs.readFileSync(outputFile, 'utf8');
|
||||||
|
expect(markdown).toContain('## AI Review Incomplete');
|
||||||
|
expect(markdown).toContain('Runtime tools: `Bash`, `Edit`, `Read`');
|
||||||
|
expect(markdown).toContain('Turns used: 25');
|
||||||
|
expect(markdown).not.toContain('Now let me verify the findings');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('escapes markdown-looking content and ignores malformed execution metadata', () => {
|
||||||
|
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ai-review-'));
|
||||||
|
const executionFile = path.join(tempDir, 'claude-execution-output.json');
|
||||||
|
const outputFile = path.join(tempDir, 'pr_review.md');
|
||||||
|
|
||||||
|
fs.writeFileSync(executionFile, '{not valid json');
|
||||||
|
|
||||||
|
const result = reviewOutput.writeReviewFromEnv({
|
||||||
|
AI_REVIEW_EXECUTION_FILE: executionFile,
|
||||||
|
AI_REVIEW_MODEL: 'glm-5.1',
|
||||||
|
AI_REVIEW_OUTPUT_FILE: outputFile,
|
||||||
|
AI_REVIEW_RUN_URL: 'https://github.com/kaitranntt/ccs/actions/runs/1',
|
||||||
|
AI_REVIEW_STRUCTURED_OUTPUT: JSON.stringify({
|
||||||
|
summary: 'Summary with `code` and ## heading markers.',
|
||||||
|
findings: [
|
||||||
|
{
|
||||||
|
severity: 'low',
|
||||||
|
title: 'Title with `ticks`',
|
||||||
|
file: 'src/example.ts',
|
||||||
|
line: 9,
|
||||||
|
what: 'Problem text uses **bold** markers.',
|
||||||
|
why: 'Why text uses [link] syntax.',
|
||||||
|
fix: 'Fix text uses <html> markers.',
|
||||||
|
},
|
||||||
|
],
|
||||||
|
overallAssessment: 'approved_with_notes',
|
||||||
|
overallRationale: 'Rationale keeps `_formatting_` stable.',
|
||||||
|
notes: ['Note with `inline code`.'],
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result.usedFallback).toBe(false);
|
||||||
|
|
||||||
|
const markdown = fs.readFileSync(outputFile, 'utf8');
|
||||||
|
expect(markdown).toContain('Summary with \\`code\\` and ## heading markers.');
|
||||||
|
expect(markdown).toContain('**`src/example.ts:9` — Title with \\`ticks\\`**');
|
||||||
|
expect(markdown).toContain('Problem: Problem text uses \\*\\*bold\\*\\* markers.');
|
||||||
|
expect(markdown).toContain('Why it matters: Why text uses \\[link\\] syntax.');
|
||||||
|
expect(markdown).toContain('Suggested fix: Fix text uses \\<html\\> markers.');
|
||||||
|
expect(markdown).toContain('**⚠️ APPROVED WITH NOTES**');
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user