diff --git a/.github/review-prompt.md b/.github/review-prompt.md index 4f91e5ff..c3b2ff9b 100644 --- a/.github/review-prompt.md +++ b/.github/review-prompt.md @@ -58,7 +58,8 @@ 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, ideally 2-4 sentences. Do not include the PR title, a separate verdict line, markdown tables, file inventories, or custom section headings there. - Keep `overallRationale` to 1 sentence. -- Keep `what`, `why`, and `fix` concise plain text. Do not emit headings, tables, or fenced code blocks inside those fields. +- Keep `title`, `what`, `why`, and `fix` concise plain text. Prefer 1 short sentence per field so the rendered review stays readable in expanded long-form format. +- Do not emit headings, tables, or fenced code blocks inside `title`, `what`, `why`, or `fix`. - 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`. diff --git a/scripts/github/normalize-ai-review-output.mjs b/scripts/github/normalize-ai-review-output.mjs index 06d2436c..c1a6f30a 100644 --- a/scripts/github/normalize-ai-review-output.mjs +++ b/scripts/github/normalize-ai-review-output.mjs @@ -646,23 +646,21 @@ function renderFindingSnippets(snippets) { 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}`) - ); + if (lines.length > 0) { + lines.push(''); + } + lines.push(label, '', ...renderCodeBlock(snippet.code, snippet.language).split('\n')); } return lines; } -function renderDetailsBlock(summary, bodyLines) { +function renderSection(title, bodyLines) { if (!bodyLines.length) { return []; } - return ['', '
', `${escapeMarkdown(summary)}`, '', ...bodyLines, '', '
']; + return ['', title, '', ...bodyLines]; } function renderFindingReference(finding) { @@ -705,11 +703,12 @@ function renderDetailedFindings(findings) { if (scopedFindings.length === 0) continue; lines.push(`**${SEVERITY_SUMMARY_LABELS[severity]} (${scopedFindings.length})**`, ''); - for (const finding of scopedFindings) { - lines.push(`- **${renderCode(renderFindingReference(finding))} — ${renderInlineText(finding.title)}**`); - lines.push(` Problem: ${renderInlineText(finding.what)}`); - lines.push(` Why it matters: ${renderInlineText(finding.why)}`); - lines.push(` Suggested fix: ${renderInlineText(finding.fix)}`); + for (const [index, finding] of scopedFindings.entries()) { + lines.push(`#### ${index + 1}. ${renderInlineText(finding.title)}`); + lines.push(`- Location: ${renderCode(renderFindingReference(finding))}`); + lines.push(`- Impact: ${renderInlineText(finding.why)}`); + lines.push(`- Problem: ${renderInlineText(finding.what)}`); + lines.push(`- Fix: ${renderInlineText(finding.fix)}`); lines.push(...renderFindingSnippets(finding.snippets)); lines.push(''); } @@ -738,21 +737,21 @@ export function renderStructuredReview(review, { model, rendering: renderOptions } lines.push('', '### Top Findings', '', ...renderTopFindings(review.findings)); - lines.push(...renderDetailsBlock(`Full Findings (${review.findings.length})`, renderDetailedFindings(review.findings))); + lines.push(...renderSection(`### Detailed Findings (${review.findings.length})`, renderDetailedFindings(review.findings))); lines.push( - ...renderDetailsBlock( - `Security Checklist (${review.securityChecklist.length})`, + ...renderSection( + `### Security Checklist (${review.securityChecklist.length})`, renderChecklistTable('Check', 'check', review.securityChecklist) ) ); lines.push( - ...renderDetailsBlock( - `CCS Compliance (${review.ccsCompliance.length})`, + ...renderSection( + `### CCS Compliance (${review.ccsCompliance.length})`, renderChecklistTable('Rule', 'rule', review.ccsCompliance) ) ); - lines.push(...renderDetailsBlock(`Informational (${review.informational.length})`, renderBulletSection(review.informational))); - lines.push(...renderDetailsBlock(`What's Done Well (${review.strengths.length})`, renderBulletSection(review.strengths))); + lines.push(...renderSection(`### Informational (${review.informational.length})`, renderBulletSection(review.informational))); + lines.push(...renderSection(`### What's Done Well (${review.strengths.length})`, renderBulletSection(review.strengths))); lines.push( '', 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 c34d45e0..ffdde71a 100644 --- a/tests/unit/scripts/github/normalize-ai-review-output.test.ts +++ b/tests/unit/scripts/github/normalize-ai-review-output.test.ts @@ -57,16 +57,17 @@ describe('normalize-ai-review-output', () => { expect(markdown).toContain('### Verdict'); expect(markdown).toContain('### Top Findings'); expect(markdown).toContain('- 🔴 High `src/cliproxy/accounts/query.ts:61` — Ambiguous account lookup drops valid matches'); - expect(markdown).toContain('Full Findings (1)'); - expect(markdown).toContain('**`src/cliproxy/accounts/query.ts:61` — Ambiguous account lookup drops valid matches**'); - expect(markdown).toContain('Security Checklist (1)'); + expect(markdown).toContain('### Detailed Findings (1)'); + expect(markdown).toContain('#### 1. Ambiguous account lookup drops valid matches'); + expect(markdown).toContain('- Location: `src/cliproxy/accounts/query.ts:61`'); + expect(markdown).toContain('### Security Checklist (1)'); expect(markdown).toContain('| Injection safety | ✅ | No user-controlled input reaches a shell, SQL, or HTML boundary in this diff. |'); - expect(markdown).toContain('CCS Compliance (1)'); + expect(markdown).toContain('### CCS Compliance (1)'); expect(markdown).toContain('| No emojis in CLI | N/A | This change affects GitHub PR comments only, not CLI stdout. |'); - expect(markdown).toContain('Informational (1)'); - expect(markdown).toContain("What's Done Well (1)"); + expect(markdown).toContain('### Informational (1)'); + expect(markdown).toContain("### What's Done Well (1)"); 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('Impact: That breaks normal selection flows for users with multiple Codex sessions.'); expect(markdown).toContain('> 🤖 Reviewed by `glm-5-turbo`'); }); @@ -452,10 +453,11 @@ describe('normalize-ai-review-output', () => { 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('#### 1. Title with \\`ticks\\`'); + expect(markdown).toContain('- Location: `src/example.ts:9`'); 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('Impact: Why text uses \\[link\\] syntax.'); + expect(markdown).toContain('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.'); @@ -512,11 +514,11 @@ describe('normalize-ai-review-output', () => { expect(markdown).toContain('### Top Findings'); expect(markdown).toContain('No confirmed issues found after reviewing the diff and surrounding code.'); - expect(markdown).toContain('Security Checklist (1)'); + expect(markdown).toContain('### Security Checklist (1)'); expect(markdown).toContain( '| Injection safety | ✅ | No user-controlled data crosses a risky boundary in the reviewed diff. |' ); - expect(markdown).toContain('CCS Compliance (1)'); + expect(markdown).toContain('### CCS Compliance (1)'); 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.'); }); @@ -548,9 +550,8 @@ describe('normalize-ai-review-output', () => { expect(validation.ok).toBe(true); const markdown = reviewOutput.renderStructuredReview(validation.value, { model: 'glm-5-turbo' }); - expect(markdown).toContain( - '**`tests/unit/scripts/github/normalize-ai-review-output.test.ts` — Missing empty-state coverage**' - ); + expect(markdown).toContain('#### 1. Missing empty-state coverage'); + expect(markdown).toContain('- Location: `tests/unit/scripts/github/normalize-ai-review-output.test.ts`'); expect(markdown).not.toContain('normalize-ai-review-output.test.ts:`'); }); @@ -579,7 +580,7 @@ describe('normalize-ai-review-output', () => { { model: 'glm-5-turbo' } ); - expect(markdown).toContain('**``src/weird`path.ts`` — Backtick-safe locations stay readable**'); + expect(markdown).toContain('- Location: ``src/weird`path.ts``'); }); test('rejects empty checklist sections instead of synthesizing placeholder rows', () => {