diff --git a/.github/review-prompt.md b/.github/review-prompt.md index 7dac7b22..9113ac58 100644 --- a/.github/review-prompt.md +++ b/.github/review-prompt.md @@ -49,6 +49,7 @@ Use only the review contract in this file plus the generated scope and packet fi - Return confirmed findings only. - Every finding must cite a file path and, when practical, a line number. +- Each finding may optionally include `snippets`: up to 2 short evidence blocks with `label`, `language`, and `code`. - 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. @@ -57,6 +58,7 @@ Use only the review contract in this file plus the generated scope and packet fi - 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 `snippets` only when a short literal excerpt materially clarifies a finding. Keep each snippet under 20 lines, and do not include markdown fences in `code`. - 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. diff --git a/scripts/github/normalize-ai-review-output.mjs b/scripts/github/normalize-ai-review-output.mjs index 23309af4..f22fe925 100644 --- a/scripts/github/normalize-ai-review-output.mjs +++ b/scripts/github/normalize-ai-review-output.mjs @@ -37,11 +37,21 @@ const RENDERER_OWNED_MARKUP_PATTERNS = [ const INLINE_CODE_TOKEN_PATTERN = /\b[A-Za-z_][A-Za-z0-9_.]*\([^()\n]*\)|(?|])/g, '\\$1'); } @@ -57,6 +67,14 @@ function renderCode(value) { return `${fence}${text}${fence}`; } +function renderCodeBlock(value, language) { + const text = cleanMultilineText(value); + const longestFence = Math.max(...[...text.matchAll(/`+/gu)].map((match) => match[0].length), 0); + const fence = '`'.repeat(Math.max(3, longestFence + 1)); + const info = cleanText(language); + return `${fence}${info}\n${text}\n${fence}`; +} + function renderInlineText(value) { const text = cleanText(value); if (!text) { @@ -330,6 +348,74 @@ function normalizeChecklistRows(fieldName, labelField, raw) { return { ok: true, value: rows }; } +function normalizeFindingSnippets(fieldName, raw) { + if (raw === null || raw === undefined) { + return { ok: true, value: [] }; + } + + if (!Array.isArray(raw)) { + return { ok: false, reason: `${fieldName} must be an array` }; + } + + if (raw.length > MAX_FINDING_SNIPPETS) { + return { + ok: false, + reason: `${fieldName} must contain at most ${MAX_FINDING_SNIPPETS} snippets`, + }; + } + + const snippets = []; + for (const [index, item] of raw.entries()) { + if (!item || typeof item !== 'object' || Array.isArray(item)) { + return { ok: false, reason: `${fieldName}[${index}] must be an object` }; + } + + let label = null; + if (Object.hasOwn(item, 'label') && item.label !== null && item.label !== undefined) { + const labelValidation = validatePlainTextField(`${fieldName}[${index}].label`, item.label); + if (!labelValidation.ok) return labelValidation; + label = labelValidation.value; + } + + let language = null; + if (Object.hasOwn(item, 'language') && item.language !== null && item.language !== undefined) { + const normalizedLanguage = cleanText(item.language).toLowerCase(); + if (normalizedLanguage) { + if (!CODE_BLOCK_LANGUAGE_PATTERN.test(normalizedLanguage)) { + return { ok: false, reason: `${fieldName}[${index}].language is invalid` }; + } + language = normalizedLanguage; + } + } + + const code = cleanMultilineText(item.code); + if (!code) { + return { ok: false, reason: `${fieldName}[${index}].code is required` }; + } + if (code.length > MAX_SNIPPET_CHARACTERS) { + return { + ok: false, + reason: `${fieldName}[${index}].code exceeds ${MAX_SNIPPET_CHARACTERS} characters`, + }; + } + + const lineCount = code.split('\n').length; + if (lineCount > MAX_SNIPPET_LINES) { + return { + ok: false, + reason: `${fieldName}[${index}].code exceeds ${MAX_SNIPPET_LINES} lines`, + }; + } + + const snippet = { code }; + if (label) snippet.label = label; + if (language) snippet.language = language; + snippets.push(snippet); + } + + return { ok: true, value: snippets }; +} + function readExecutionMetadata(executionFile) { if (!executionFile || !fs.existsSync(executionFile)) { return {}; @@ -472,6 +558,8 @@ export function normalizeStructuredOutput(raw) { const fix = validatePlainTextField(`findings[${index}].fix`, finding?.fix); if (!fix.ok) return fix; + const snippets = normalizeFindingSnippets(`findings[${index}].snippets`, finding?.snippets); + if (!snippets.ok) return snippets; let line = null; if (finding && Object.hasOwn(finding, 'line')) { @@ -496,6 +584,7 @@ export function normalizeStructuredOutput(raw) { what: what.value, why: why.value, fix: fix.value, + snippets: snippets.value, }); } @@ -532,6 +621,25 @@ function renderBulletSection(title, items) { return ['', title, ...items.map((item) => `- ${renderInlineText(item)}`)]; } +function renderFindingSnippets(snippets) { + if (!Array.isArray(snippets) || snippets.length === 0) { + return []; + } + + const lines = []; + for (const snippet of snippets) { + const label = snippet.label ? `Evidence: ${renderInlineText(snippet.label)}` : 'Evidence:'; + lines.push('', ` ${label}`, ''); + lines.push( + ...renderCodeBlock(snippet.code, snippet.language) + .split('\n') + .map((line) => ` ${line}`) + ); + } + + return lines; +} + export function renderStructuredReview(review, { model, rendering: renderOptions } = {}) { const rendering = mergeRenderingMetadata(review?.rendering, renderOptions); const lines = ['### 📋 Summary', '', renderInlineText(review.summary), '', '### 🔍 Findings']; @@ -555,6 +663,7 @@ export function renderStructuredReview(review, { model, rendering: renderOptions lines.push(` Problem: ${renderInlineText(finding.what)}`); lines.push(` Why it matters: ${renderInlineText(finding.why)}`); lines.push(` Suggested fix: ${renderInlineText(finding.fix)}`); + lines.push(...renderFindingSnippets(finding.snippets)); lines.push(''); } } diff --git a/scripts/github/run-ai-review-direct.mjs b/scripts/github/run-ai-review-direct.mjs index 4c263bda..e8c25c34 100644 --- a/scripts/github/run-ai-review-direct.mjs +++ b/scripts/github/run-ai-review-direct.mjs @@ -100,6 +100,14 @@ Return a single object with these keys only: - overallAssessment - overallRationale +Each finding may optionally include: +- snippets: an array of up to 2 objects with keys label, language, and code + +If snippets are present: +- keep code literal only, without markdown fences +- keep each snippet under 20 lines +- use snippets only for short evidence that materially clarifies the finding + Use empty arrays rather than inventing low-value feedback. Every finding must be confirmed by the review packet.`; } diff --git a/tests/unit/scripts/github/normalize-ai-review-output.test.ts b/tests/unit/scripts/github/normalize-ai-review-output.test.ts index c6b76cd5..e4afa2b0 100644 --- a/tests/unit/scripts/github/normalize-ai-review-output.test.ts +++ b/tests/unit/scripts/github/normalize-ai-review-output.test.ts @@ -168,6 +168,47 @@ describe('normalize-ai-review-output', () => { expect(markdown).toContain('`old_marker_path`'); }); + test('renders finding snippets as renderer-owned fenced code blocks', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: 'One workflow branch still uses the stale marker path.', + findings: [ + { + severity: 'medium', + title: 'Fallback branch still writes the stale marker file', + file: '.github/workflows/ai-review.yml', + line: 181, + what: 'One branch still writes the old marker file path.', + why: 'That can leave duplicate bot comments on reruns for the same PR SHA.', + fix: 'Keep the rerun marker keyed to PR plus head SHA in every publish branch.', + snippets: [ + { + label: 'Current publish branch', + language: 'bash', + code: 'marker_file=\"$RUNNER_TEMP/.ai-review-marker\"\nprintf \"%s\\n\" \"$REVIEW_MARKER\" > \"$marker_file\"', + }, + ], + }, + ], + securityChecklist: [{ check: 'Workflow safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'Renderer-owned markdown', status: 'pass', notes: 'Covered.' }], + informational: [], + strengths: [], + overallAssessment: 'approved_with_notes', + overallRationale: 'This is a deterministic formatting-only follow-up.', + }) + ); + + expect(validation.ok).toBe(true); + const markdown = reviewOutput.renderStructuredReview(validation.value, { model: 'glm-5.1' }); + + expect(markdown).toContain('Evidence: Current publish branch'); + expect(markdown).toContain('```bash'); + expect(markdown).toContain('marker_file="$RUNNER_TEMP/.ai-review-marker"'); + expect(markdown).toContain('printf "%s\\n" "$REVIEW_MARKER" > "$marker_file"'); + expect(markdown).toContain('```'); + }); + test('normalizes optional rendering metadata when present in structured output', () => { const validation = reviewOutput.normalizeStructuredOutput( JSON.stringify({ @@ -520,6 +561,41 @@ describe('normalize-ai-review-output', () => { expect(validation.reason).toContain('securityChecklist must contain at least 1 item'); }); + test('rejects finding snippets that exceed the renderer snippet budget', () => { + const validation = reviewOutput.normalizeStructuredOutput( + JSON.stringify({ + summary: 'The renderer should reject oversized snippet payloads.', + findings: [ + { + severity: 'low', + title: 'Oversized snippet', + file: 'scripts/github/normalize-ai-review-output.mjs', + line: 1, + what: 'The example snippet is intentionally too long.', + why: 'Oversized snippets would bloat the published review comment.', + fix: 'Keep snippets short and renderer-owned.', + snippets: [ + { + label: 'Too long', + language: 'txt', + code: Array.from({ length: 21 }, (_, index) => `line ${index + 1}`).join('\n'), + }, + ], + }, + ], + securityChecklist: [{ check: 'Injection safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'Renderer-owned markdown', status: 'pass', notes: 'Covered.' }], + informational: [], + strengths: [], + overallAssessment: 'approved_with_notes', + overallRationale: 'Oversized snippets should fail validation.', + }) + ); + + expect(validation.ok).toBe(false); + expect(validation.reason).toContain('findings[0].snippets[0].code exceeds 20 lines'); + }); + test('allows plain prose that references section labels without starting with them', () => { const validation = reviewOutput.normalizeStructuredOutput( JSON.stringify({ diff --git a/tests/unit/scripts/github/run-ai-review-direct.test.ts b/tests/unit/scripts/github/run-ai-review-direct.test.ts index aee13dd0..51331f3c 100644 --- a/tests/unit/scripts/github/run-ai-review-direct.test.ts +++ b/tests/unit/scripts/github/run-ai-review-direct.test.ts @@ -156,6 +156,82 @@ describe('run-ai-review-direct', () => { }); }); + test('renders finding snippets from the validated direct review response', async () => { + await withTempDir('ai-review-direct-', async (tempDir) => { + const outputFile = path.join(tempDir, 'review.md'); + const logFile = path.join(tempDir, 'attempts.json'); + const packetFile = path.join(tempDir, 'packet.md'); + const manifestFile = path.join(tempDir, 'selected-files.txt'); + const includedManifestFile = path.join(tempDir, 'included-files.txt'); + fs.writeFileSync(packetFile, '# AI Review Packet\n\npacket body\n'); + fs.writeFileSync(manifestFile, '.github/workflows/ai-review.yml\n'); + fs.writeFileSync(includedManifestFile, '.github/workflows/ai-review.yml\n'); + + const result = await directReview.writeDirectReviewFromEnv( + { + ANTHROPIC_BASE_URL: 'https://api.z.ai/api/anthropic', + ANTHROPIC_AUTH_TOKEN: 'test-token', + REVIEW_MODEL: 'glm-5.1', + GITHUB_REPOSITORY: 'kaitranntt/ccs', + AI_REVIEW_PROMPT: 'You are a reviewer.', + AI_REVIEW_PACKET_FILE: packetFile, + AI_REVIEW_SCOPE_MANIFEST_FILE: manifestFile, + AI_REVIEW_PACKET_INCLUDED_MANIFEST_FILE: includedManifestFile, + AI_REVIEW_OUTPUT_FILE: outputFile, + AI_REVIEW_LOG_FILE: logFile, + AI_REVIEW_RUN_URL: 'https://github.com/kaitranntt/ccs/actions/runs/1', + AI_REVIEW_MODE: 'fast', + AI_REVIEW_SELECTED_FILES: '1', + AI_REVIEW_REVIEWABLE_FILES: '1', + AI_REVIEW_SELECTED_CHANGES: '24', + AI_REVIEW_REVIEWABLE_CHANGES: '24', + AI_REVIEW_SCOPE_LABEL: 'reviewable files', + AI_REVIEW_PACKET_INCLUDED_FILES: '1', + AI_REVIEW_PACKET_TOTAL_FILES: '1', + AI_REVIEW_PACKET_OMITTED_FILES: '0', + AI_REVIEW_TIMEOUT_MINUTES: '8', + AI_REVIEW_PR_NUMBER: '888', + }, + async () => + createResponse( + JSON.stringify({ + summary: 'One non-blocking follow-up remains.', + findings: [ + { + severity: 'low', + title: 'Marker write path still has one stale branch', + file: '.github/workflows/ai-review.yml', + line: 181, + what: 'One branch still writes the stale marker file path.', + why: 'That can make rerun behavior harder to reason about.', + fix: 'Keep every publish branch aligned on the PR plus SHA marker.', + snippets: [ + { + label: 'Current branch body', + language: 'bash', + code: 'marker_file=\"$RUNNER_TEMP/.ai-review-marker\"\nprintf \"%s\\n\" \"$REVIEW_MARKER\" > \"$marker_file\"', + }, + ], + }, + ], + securityChecklist: [{ check: 'Injection safety', status: 'pass', notes: 'Covered.' }], + ccsCompliance: [{ rule: 'Renderer-owned markdown', status: 'pass', notes: 'Covered.' }], + informational: [], + strengths: ['The response validated on the first attempt.'], + overallAssessment: 'approved_with_notes', + overallRationale: 'The remaining change is formatter polish only.', + }) + ) + ); + + expect(result.usedFallback).toBe(false); + const markdown = fs.readFileSync(outputFile, 'utf8'); + expect(markdown).toContain('Evidence: Current branch body'); + expect(markdown).toContain('```bash'); + expect(markdown).toContain('marker_file="$RUNNER_TEMP/.ai-review-marker"'); + }); + }); + test('retries with a repair attempt when the first response is invalid', async () => { await withTempDir('ai-review-direct-', async (tempDir) => { const outputFile = path.join(tempDir, 'review.md');