From 0b72ecd36f0277f1c5407ff4bbe442305477a60e Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 1 Apr 2026 13:50:32 -0400 Subject: [PATCH] hotfix(ci): harden ai review workflow for release PRs --- .github/review-prompt.md | 164 ++------ .github/workflows/ai-review.yml | 105 ++--- scripts/github/normalize-ai-review-output.mjs | 328 ++++++++++++++++ .../github/normalize-ai-review-output.test.ts | 367 ++++++++++++++++++ 4 files changed, 795 insertions(+), 169 deletions(-) create mode 100644 scripts/github/normalize-ai-review-output.mjs create mode 100644 tests/unit/scripts/github/normalize-ai-review-output.test.ts diff --git a/.github/review-prompt.md b/.github/review-prompt.md index fd47b4a0..bd0c802b 100644 --- a/.github/review-prompt.md +++ b/.github/review-prompt.md @@ -1,136 +1,52 @@ -# 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. +- Fill the structured fields only. The renderer owns the markdown layout. +- Keep `summary` to plain prose only. Do not include the PR title, a separate verdict line, markdown tables, file inventories, or custom section headings there. +- Keep `what`, `why`, and `fix` concise plain text. Do not emit headings, tables, or fenced code blocks inside those fields. +- Use `securityChecklist` for concise review rows about security-sensitive checks. Provide at least 1 row, and use 2-5 when possible. `status` = `pass` | `fail` | `na`. +- Use `ccsCompliance` for concise CCS-specific rule checks. Provide at least 1 row, and use 2-5 when possible. `status` = `pass` | `fail` | `na`. +- Use `informational` for small non-blocking observations that are worth calling out. +- Use `strengths` for specific things done well. No generic praise. diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index cdd917ba..581be040 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -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,"maxLength":600},"findings":{"type":"array","maxItems":6,"items":{"type":"object","additionalProperties":false,"properties":{"severity":{"type":"string","enum":["high","medium","low"]},"title":{"type":"string","minLength":1,"maxLength":180},"file":{"type":"string","minLength":1,"maxLength":240},"line":{"type":["integer","null"],"minimum":1},"what":{"type":"string","minLength":1,"maxLength":500},"why":{"type":"string","minLength":1,"maxLength":500},"fix":{"type":"string","minLength":1,"maxLength":500}},"required":["severity","title","file","what","why","fix"]}},"securityChecklist":{"type":"array","minItems":1,"maxItems":5,"items":{"type":"object","additionalProperties":false,"properties":{"check":{"type":"string","minLength":1,"maxLength":80},"status":{"type":"string","enum":["pass","fail","na"]},"notes":{"type":"string","minLength":1,"maxLength":180}},"required":["check","status","notes"]}},"ccsCompliance":{"type":"array","minItems":1,"maxItems":5,"items":{"type":"object","additionalProperties":false,"properties":{"rule":{"type":"string","minLength":1,"maxLength":80},"status":{"type":"string","enum":["pass","fail","na"]},"notes":{"type":"string","minLength":1,"maxLength":180}},"required":["rule","status","notes"]}},"informational":{"type":"array","maxItems":4,"items":{"type":"string","minLength":1,"maxLength":220}},"strengths":{"type":"array","maxItems":4,"items":{"type":"string","minLength":1,"maxLength":220}},"overallAssessment":{"type":"string","enum":["approved","approved_with_notes","changes_requested"]},"overallRationale":{"type":"string","minLength":1,"maxLength":320}},"required":["summary","findings","securityChecklist","ccsCompliance","informational","strengths","overallAssessment","overallRationale"]} steps: - name: Prepare isolated Claude runtime @@ -200,16 +202,51 @@ jobs: env: CONTRIBUTOR_SOURCE: ${{ needs.prepare.outputs.contributor_source }} BASE_REF: ${{ github.base_ref || 'dev' }} + USE_CHECKED_OUT_REVIEW_ASSETS: >- + ${{ github.event_name == 'workflow_dispatch' && needs.prepare.outputs.contributor_source == 'internal' && '1' || '' }} run: | - # Always load prompt from base branch to prevent PR-controlled prompt injection. - # External PRs could modify review-prompt.md to suppress security findings. PROMPT_CONTENT="" - git fetch origin "$BASE_REF" --depth=1 2>/dev/null || true - PROMPT_CONTENT=$(git show "origin/${BASE_REF}:.github/review-prompt.md" 2>/dev/null || echo "") + if [ -n "$USE_CHECKED_OUT_REVIEW_ASSETS" ]; then + # workflow_dispatch on an internal PR is the trusted pre-merge replay path. + # Use the checked-out branch assets so maintainers can verify the exact formatter under test. + PROMPT_CONTENT=$(cat .github/review-prompt.md 2>/dev/null || echo "") + else + # pull_request_target and issue_comment must stay pinned to the base branch to prevent prompt injection. + git fetch origin "$BASE_REF" --depth=1 2>/dev/null || true + PROMPT_CONTENT=$(git show "origin/${BASE_REF}:.github/review-prompt.md" 2>/dev/null || echo "") + fi if [ -z "$PROMPT_CONTENT" ]; then echo "::warning::.github/review-prompt.md not found on base branch ${BASE_REF} — using fallback" PROMPT_CONTENT="You are a red-team code reviewer. Find every way this code can fail, be exploited, or produce incorrect results. Flag security issues, logic errors, missing error handling, race conditions, and injection risks. Follow the repository CLAUDE.md for project-specific guidelines. Output findings grouped by severity: High (must fix), Medium (should fix), Low (track). Use strict approval criteria." fi + + NORMALIZER_PATH="$RUNNER_TEMP/normalize-ai-review-output.mjs" + if [ -n "$USE_CHECKED_OUT_REVIEW_ASSETS" ]; then + cp scripts/github/normalize-ai-review-output.mjs "$NORMALIZER_PATH" + elif ! git show "origin/${BASE_REF}:scripts/github/normalize-ai-review-output.mjs" > "$NORMALIZER_PATH" 2>/dev/null; then + echo "::warning::scripts/github/normalize-ai-review-output.mjs not found on base branch ${BASE_REF} — using safe fallback normalizer" + printf '%s\n' \ + "import fs from 'node:fs';" \ + "" \ + "const outputFile = process.env.AI_REVIEW_OUTPUT_FILE || 'pr_review.md';" \ + "const model = process.env.AI_REVIEW_MODEL || 'unknown-model';" \ + "const runUrl = process.env.AI_REVIEW_RUN_URL || '#';" \ + "const content = [" \ + " '### ⚠️ AI Review Incomplete'," \ + " ''," \ + " 'The trusted base-branch normalizer was unavailable, so this workflow skipped rendering any PR-controlled review output.'," \ + " ''," \ + " '- Reason: trusted normalizer missing on base branch'," \ + " ''," \ + " \`Re-run \\\`/review\\\` or inspect [the workflow run](\${runUrl}).\`," \ + " ''," \ + " \`> 🤖 Reviewed by \\\`\${model}\\\`\`," \ + "].join('\\n');" \ + "fs.writeFileSync(outputFile, \`\${content}\\n\`, 'utf8');" \ + > "$NORMALIZER_PATH" + fi + echo "AI_REVIEW_NORMALIZER=$NORMALIZER_PATH" >> "$GITHUB_ENV" + DELIMITER="REVIEW_PROMPT_$(openssl rand -hex 16)" { echo "content<<${DELIMITER}" @@ -224,7 +261,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 +280,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 "$AI_REVIEW_NORMALIZER" + 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' diff --git a/scripts/github/normalize-ai-review-output.mjs b/scripts/github/normalize-ai-review-output.mjs new file mode 100644 index 00000000..9ece60c6 --- /dev/null +++ b/scripts/github/normalize-ai-review-output.mjs @@ -0,0 +1,328 @@ +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', +}; + +const STATUS_LABELS = { + pass: '✅', + fail: '⚠️', + na: 'N/A', +}; + +const RENDERER_OWNED_MARKUP_PATTERNS = [ + { pattern: /^#{1,6}\s/u, reason: 'markdown heading' }, + { pattern: /^\s*Verdict\s*:/iu, reason: 'verdict label' }, + { pattern: /^\s*PR\s*#?\d+\s*Review(?:\s*[:.-]|$)/iu, reason: 'ad hoc PR heading' }, + { pattern: /\|\s*[-:]+\s*\|/u, reason: 'markdown table' }, + { pattern: /```/u, reason: 'code fence' }, +]; + +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 validatePlainTextField(fieldName, value) { + const text = cleanText(value); + if (!text) { + return { ok: false, reason: `${fieldName} is required` }; + } + + const match = RENDERER_OWNED_MARKUP_PATTERNS.find(({ pattern }) => pattern.test(text)); + if (match) { + return { ok: false, reason: `${fieldName} contains ${match.reason}` }; + } + + return { ok: true, value: text }; +} + +function normalizeStringList(fieldName, raw) { + if (!Array.isArray(raw)) { + return { ok: false, reason: `${fieldName} must be an array` }; + } + + const values = []; + for (const [index, item] of raw.entries()) { + const validation = validatePlainTextField(`${fieldName}[${index}]`, item); + if (!validation.ok) return validation; + values.push(validation.value); + } + + return { ok: true, value: values }; +} + +function normalizeChecklistRows(fieldName, labelField, raw) { + if (!Array.isArray(raw)) { + return { ok: false, reason: `${fieldName} must be an array` }; + } + + const rows = []; + for (const [index, item] of raw.entries()) { + const label = validatePlainTextField(`${fieldName}[${index}].${labelField}`, item?.[labelField]); + if (!label.ok) return label; + + const notes = validatePlainTextField(`${fieldName}[${index}].notes`, item?.notes); + if (!notes.ok) return notes; + + const status = cleanText(item?.status).toLowerCase(); + if (!STATUS_LABELS[status]) { + return { ok: false, reason: `${fieldName}[${index}].status is invalid` }; + } + + rows.push({ [labelField]: label.value, status, notes: notes.value }); + } + + if (rows.length === 0) { + return { ok: false, reason: `${fieldName} must contain at least 1 item` }; + } + + return { ok: true, value: rows }; +} + +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 = validatePlainTextField('summary', parsed.summary); + if (!summary.ok) return summary; + + const overallAssessment = cleanText(parsed.overallAssessment).toLowerCase(); + const overallRationale = validatePlainTextField('overallRationale', parsed.overallRationale); + if (!overallRationale.ok) return overallRationale; + + const findings = Array.isArray(parsed.findings) ? parsed.findings : null; + const securityChecklist = normalizeChecklistRows('securityChecklist', 'check', parsed.securityChecklist); + if (!securityChecklist.ok) return securityChecklist; + + const ccsCompliance = normalizeChecklistRows('ccsCompliance', 'rule', parsed.ccsCompliance); + if (!ccsCompliance.ok) return ccsCompliance; + + const informational = normalizeStringList('informational', parsed.informational); + if (!informational.ok) return informational; + + const strengths = normalizeStringList('strengths', parsed.strengths); + if (!strengths.ok) return strengths; + + if (!ASSESSMENTS[overallAssessment] || findings === null) { + return { ok: false, reason: 'structured output is missing required review fields' }; + } + + const normalizedFindings = []; + for (const [index, finding] of findings.entries()) { + const severity = cleanText(finding?.severity).toLowerCase(); + const title = validatePlainTextField(`findings[${index}].title`, finding?.title); + if (!title.ok) return title; + + const file = validatePlainTextField(`findings[${index}].file`, finding?.file); + if (!file.ok) return file; + + const what = validatePlainTextField(`findings[${index}].what`, finding?.what); + if (!what.ok) return what; + + const why = validatePlainTextField(`findings[${index}].why`, finding?.why); + if (!why.ok) return why; + + const fix = validatePlainTextField(`findings[${index}].fix`, finding?.fix); + if (!fix.ok) return fix; + + let line = null; + if (finding && Object.hasOwn(finding, 'line')) { + if (finding.line === null) { + line = null; + } else if (typeof finding.line === 'number' && Number.isInteger(finding.line) && finding.line > 0) { + line = finding.line; + } else { + return { ok: false, reason: `findings[${index}].line is invalid` }; + } + } + + if (!SEVERITY_HEADERS[severity]) { + return { ok: false, reason: `findings[${index}].severity is invalid` }; + } + + normalizedFindings.push({ + severity, + title: title.value, + file: file.value, + line, + what: what.value, + why: why.value, + fix: fix.value, + }); + } + + return { + ok: true, + value: { + summary: summary.value, + findings: normalizedFindings, + overallAssessment, + overallRationale: overallRationale.value, + securityChecklist: securityChecklist.value, + ccsCompliance: ccsCompliance.value, + informational: informational.value, + strengths: strengths.value, + }, + }; +} + +function renderChecklistTable(title, labelHeader, labelKey, rows) { + const lines = ['', title, '', `| ${labelHeader} | Status | Notes |`, '|---|---|---|']; + for (const row of rows) { + lines.push( + `| ${escapeMarkdownText(row[labelKey])} | ${STATUS_LABELS[row.status]} | ${escapeMarkdownText(row.notes)} |` + ); + } + return lines; +} + +function renderBulletSection(title, items) { + if (items.length === 0) return []; + return ['', title, ...items.map((item) => `- ${escapeMarkdownText(item)}`)]; +} + +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(); + } + + lines.push(...renderChecklistTable('### 🔒 Security Checklist', 'Check', 'check', review.securityChecklist)); + lines.push(...renderChecklistTable('### 📊 CCS Compliance', 'Rule', 'rule', review.ccsCompliance)); + lines.push(...renderBulletSection('### 💡 Informational', review.informational)); + lines.push(...renderBulletSection("### ✅ What's Done Well", review.strengths)); + + 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(); +} diff --git a/tests/unit/scripts/github/normalize-ai-review-output.test.ts b/tests/unit/scripts/github/normalize-ai-review-output.test.ts new file mode 100644 index 00000000..b3a8c11d --- /dev/null +++ b/tests/unit/scripts/github/normalize-ai-review-output.test.ts @@ -0,0 +1,367 @@ +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'); + +function withTempDir(prefix: string, run: (tempDir: string) => void) { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + try { + run(tempDir); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } +} + +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.', + }, + ], + securityChecklist: [ + { + check: 'Injection safety', + status: 'pass', + notes: 'No user-controlled input reaches a shell, SQL, or HTML boundary in this diff.', + }, + ], + ccsCompliance: [ + { + rule: 'No emojis in CLI', + status: 'na', + notes: 'This change affects GitHub PR comments only, not CLI stdout.', + }, + ], + informational: ['The renderer still escapes markdown before publishing comment content.'], + strengths: ['The formatter owns the output shape instead of trusting the model to author markdown.'], + overallAssessment: 'changes_requested', + overallRationale: 'The blocking lookup regression should be fixed before merge.', + }) + ); + + 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` — Ambiguous account lookup drops valid matches**'); + expect(markdown).toContain('### 🔒 Security Checklist'); + expect(markdown).toContain('| Injection safety | ✅ | No user-controlled input reaches a shell, SQL, or HTML boundary in this diff. |'); + expect(markdown).toContain('### 📊 CCS Compliance'); + expect(markdown).toContain('| No emojis in CLI | N/A | This change affects GitHub PR comments only, not CLI stdout. |'); + expect(markdown).toContain('### 💡 Informational'); + expect(markdown).toContain('### ✅ What\'s Done Well'); + expect(markdown).toContain('### 🎯 Overall Assessment'); + 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', () => { + withTempDir('ai-review-', (tempDir) => { + 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', () => { + withTempDir('ai-review-', (tempDir) => { + 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 markers.', + }, + ], + securityChecklist: [ + { + check: 'Injection safety', + status: 'pass', + notes: 'Notes with a pipe | still render safely in table cells.', + }, + ], + ccsCompliance: [ + { + rule: 'Cross-platform', + status: 'pass', + notes: 'Applies equally across macOS, Linux, and Windows.', + }, + ], + informational: ['Informational item with `inline code`.'], + strengths: ['Strength with **bold** markers.'], + overallAssessment: 'approved_with_notes', + overallRationale: 'Rationale keeps `_formatting_` stable.', + }), + }); + + 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 \\ markers.'); + expect(markdown).toContain('Notes with a pipe \\| still render safely in table cells.'); + expect(markdown).toContain('- Informational item with \\`inline code\\`.'); + expect(markdown).toContain('- Strength with \\*\\*bold\\*\\* markers.'); + expect(markdown).toContain('**⚠️ APPROVED WITH NOTES** — Rationale keeps \\`\\_formatting\\_\\` stable.'); + }); + }); + + test('rejects ad hoc layout markup inside structured fields', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: '# PR #860 Review', + findings: [], + securityChecklist: [{ check: 'Injection safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'ASCII-only CLI output', status: 'pass', notes: 'Unaffected.' }], + informational: [], + strengths: [], + overallAssessment: 'approved_with_notes', + overallRationale: 'The review is otherwise valid.', + }) + ); + + expect(validation.ok).toBe(false); + expect(validation.reason).toContain('summary contains'); + }); + + test('renders approved reviews with substantive checklist rows when optional arrays are empty', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: 'The diff is ready to merge as-is.', + findings: [], + securityChecklist: [ + { + check: 'Injection safety', + status: 'pass', + notes: 'No user-controlled data crosses a risky boundary in the reviewed diff.', + }, + ], + ccsCompliance: [ + { + rule: 'Help/docs alignment', + status: 'na', + notes: 'No CLI behavior changed, so there was nothing to update.', + }, + ], + informational: [], + strengths: [], + overallAssessment: 'approved', + overallRationale: 'No confirmed regressions or missing verification remain.', + }) + ); + + expect(validation.ok).toBe(true); + const markdown = reviewOutput.renderStructuredReview(validation.value, { model: 'glm-5.1' }); + + expect(markdown).toContain('### 🔍 Findings'); + expect(markdown).toContain('No confirmed issues found after reviewing the diff and surrounding code.'); + expect(markdown).toContain('### 🔒 Security Checklist'); + expect(markdown).toContain( + '| Injection safety | ✅ | No user-controlled data crosses a risky boundary in the reviewed diff. |' + ); + expect(markdown).toContain('### 📊 CCS Compliance'); + expect(markdown).toContain('| Help/docs alignment | N/A | No CLI behavior changed, so there was nothing to update. |'); + expect(markdown).toContain('**✅ APPROVED** — No confirmed regressions or missing verification remain.'); + }); + + test('renders findings without line numbers using the file path only', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: 'One follow-up remains.', + findings: [ + { + severity: 'medium', + title: 'Missing empty-state coverage', + file: 'tests/unit/scripts/github/normalize-ai-review-output.test.ts', + line: null, + what: 'The empty-findings branch is not covered by a regression test.', + why: 'That leaves the highest-frequency render path vulnerable to silent regressions.', + fix: 'Add a test that passes an approved review with an empty findings array.', + }, + ], + securityChecklist: [{ check: 'Injection safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'ASCII-only CLI output', status: 'pass', notes: 'Unaffected.' }], + informational: [], + strengths: [], + overallAssessment: 'approved_with_notes', + overallRationale: 'The remaining gap is test coverage only.', + }) + ); + + expect(validation.ok).toBe(true); + const markdown = reviewOutput.renderStructuredReview(validation.value, { model: 'glm-5.1' }); + + expect(markdown).toContain( + '**`tests/unit/scripts/github/normalize-ai-review-output.test.ts` — Missing empty-state coverage**' + ); + expect(markdown).not.toContain('normalize-ai-review-output.test.ts:`'); + }); + + test('renders inline code safely when the location includes backticks', () => { + const markdown = reviewOutput.renderStructuredReview( + { + summary: 'Rendering stays stable.', + findings: [ + { + severity: 'low', + title: 'Backtick-safe locations stay readable', + file: 'src/weird`path.ts', + line: null, + what: 'Location formatting needs a longer fence when input contains backticks.', + why: 'Otherwise GitHub markdown can break the inline code span.', + fix: 'Pick a fence one tick longer than the longest run in the input.', + }, + ], + securityChecklist: [{ check: 'Injection safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'ASCII-only CLI output', status: 'pass', notes: 'Unaffected.' }], + informational: [], + strengths: [], + overallAssessment: 'approved_with_notes', + overallRationale: 'This is a formatting-only follow-up.', + }, + { model: 'glm-5.1' } + ); + + expect(markdown).toContain('**``src/weird`path.ts`` — Backtick-safe locations stay readable**'); + }); + + test('rejects empty checklist sections instead of synthesizing placeholder rows', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: 'The diff is ready to merge as-is.', + findings: [], + securityChecklist: [], + ccsCompliance: [], + informational: [], + strengths: [], + overallAssessment: 'approved', + overallRationale: 'No confirmed regressions remain.', + }) + ); + + expect(validation.ok).toBe(false); + expect(validation.reason).toContain('securityChecklist must contain at least 1 item'); + }); + + test('allows plain prose that references section labels without starting with them', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: 'The Security Checklist: row is now required, but the prose summary remains valid.', + findings: [], + securityChecklist: [{ check: 'Injection safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'ASCII-only CLI output', status: 'pass', notes: 'Unaffected.' }], + informational: ['PR #860 review logic is unchanged after this formatter-only update.'], + strengths: [], + overallAssessment: 'approved_with_notes', + overallRationale: 'The renderer still blocks actual ad hoc headings.', + }) + ); + + expect(validation.ok).toBe(true); + }); + + test('allows plain prose that starts with natural language label phrases', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: 'Overall assessment: ready to merge after the renderer applies the shared layout.', + findings: [], + securityChecklist: [{ check: 'Injection safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'ASCII-only CLI output', status: 'pass', notes: 'Unaffected.' }], + informational: ['Security Checklist: rows still escape pipes safely in markdown tables.'], + strengths: [], + overallAssessment: 'approved_with_notes', + overallRationale: 'The prose can mention those phrases without becoming layout markup.', + }) + ); + + expect(validation.ok).toBe(true); + }); + + test('rejects invalid non-null finding line numbers', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: 'One finding remains.', + findings: [ + { + severity: 'medium', + title: 'Location data must stay valid', + file: 'src/example.ts', + line: 0, + what: 'The location line number is not a positive integer.', + why: 'Bad location data weakens the review signal and can hide where the issue lives.', + fix: 'Reject malformed non-null line values during normalization.', + }, + ], + securityChecklist: [{ check: 'Injection safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'ASCII-only CLI output', status: 'pass', notes: 'Unaffected.' }], + informational: [], + strengths: [], + overallAssessment: 'changes_requested', + overallRationale: 'Malformed location data should not pass validation.', + }) + ); + + expect(validation.ok).toBe(false); + expect(validation.reason).toContain('findings[0].line is invalid'); + }); +});