Merge pull request #864 from kaitranntt/kai/fix/ai-review-comment-formatting

fix: harden ai review comment formatting
2026-07-16 10:16:49 +00:00 · 2026-03-30 16:13:35 -04:00
parent c45b2a1138 fa37f391c1
commit 60e72b670c
4 changed files with 524 additions and 108 deletions
@@ -43,3 +43,10 @@ Output expectations:
 - 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.
@@ -152,7 +152,7 @@ jobs:
      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"]}
+        {"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
@@ -202,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}"
@@ -264,7 +299,7 @@ jobs:
      - name: Render review comment
        if: always() && steps.claude-review.outcome != 'cancelled'
        run: |
-          node scripts/github/normalize-ai-review-output.mjs
+          node "$AI_REVIEW_NORMALIZER"
        env:
          AI_REVIEW_EXECUTION_FILE: ${{ runner.temp }}/claude-execution-output.json
          AI_REVIEW_MODEL: ${{ env.REVIEW_MODEL }}
@@ -15,12 +15,26 @@ const SEVERITY_HEADERS = {
  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');
+  return cleanText(value).replace(/\\/g, '\\\\').replace(/([`*_{}[\]<>|])/g, '\\$1');
 }

 function renderCode(value) {
@@ -30,6 +44,63 @@ function renderCode(value) {
  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 {};
@@ -64,50 +135,106 @@ export function normalizeStructuredOutput(raw) {
    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;
+  const summary = validatePlainTextField('summary', parsed.summary);
+  if (!summary.ok) return summary;

-  if (!summary || !ASSESSMENTS[overallAssessment] || !overallRationale || findings === null) {
+  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 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;
+  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;

-    if (!SEVERITY_HEADERS[severity] || !title || !file || !what || !why || !fix) {
-      return { ok: false, reason: 'structured output contains an invalid finding' };
+    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` };
+      }
    }

-    normalizedFindings.push({ severity, title, file, line, what, why, fix });
+    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: summary.value,
      findings: normalizedFindings,
      overallAssessment,
-      overallRationale,
-      notes,
+      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'];
+  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.');
@@ -116,34 +243,31 @@ export function renderStructuredReview(review, { model }) {
      const findings = review.findings.filter((finding) => finding.severity === severity);
      if (findings.length === 0) continue;

-      lines.push(SEVERITY_HEADERS[severity]);
+      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('');
      }
-      lines.push('');
-    }
-    if (lines[lines.length - 1] === '') {
-      lines.pop();
    }
+    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(...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',
+    '### 🎯 Overall Assessment',
+    '',
    `**${ASSESSMENTS[review.overallAssessment]}** — ${escapeMarkdownText(review.overallRationale)}`,
    '',
-    `> Reviewed by \`${model}\``
+    `> 🤖 Reviewed by \`${model}\``
  );

  return lines.join('\n');
@@ -151,7 +275,7 @@ export function renderStructuredReview(review, { model }) {

 export function renderIncompleteReview({ model, reason, runUrl, runtimeTools, turnsUsed }) {
  const lines = [
-    '## AI Review Incomplete',
+    '### ⚠️ AI Review Incomplete',
    '',
    'Claude did not return validated structured review output, so this workflow did not publish raw scratch text.',
    '',
@@ -165,7 +289,7 @@ export function renderIncompleteReview({ model, reason, runUrl, runtimeTools, tu
    lines.push(`- Turns used: ${turnsUsed}`);
  }

-  lines.push('', `Re-run \`/review\` or inspect [the workflow run](${runUrl}).`, '', `> Reviewed by \`${model}\``);
+  lines.push('', `Re-run \`/review\` or inspect [the workflow run](${runUrl}).`, '', `> 🤖 Reviewed by \`${model}\``);
  return lines.join('\n');
 }

@@ -5,6 +5,15 @@ 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(
@@ -21,97 +30,338 @@ describe('normalize-ai-review-output', () => {
            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.',
-        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('### 📋 Summary');
    expect(markdown).toContain('### 🔴 High');
-    expect(markdown).toContain('`src/cliproxy/accounts/query.ts:61`');
+    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`');
+    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');
+    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...',
-        },
-      ])
-    );
+      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: '',
+      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');
    });
-
-    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');
+    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');
+      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.',
+      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.',
+            },
+          ],
+          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 \\<html\\> 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: '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.',
+            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: 'Rationale keeps `_formatting_` stable.',
-        notes: ['Note with `inline code`.'],
-      }),
-    });
+        overallRationale: 'This is a formatting-only follow-up.',
+      },
+      { model: 'glm-5.1' }
+    );

-    expect(result.usedFallback).toBe(false);
+    expect(markdown).toContain('**``src/weird`path.ts`` — Backtick-safe locations stay readable**');
+  });

-    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**');
+  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');
  });
 });