hotfix(ci): expand ai review comment layout

This commit is contained in:
Tam Nhu Tran
2026-04-02 03:14:49 -04:00
parent 25dddf4707
commit 0be4ef7a0d
3 changed files with 38 additions and 37 deletions
+2 -1
View File
@@ -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. - 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 `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 `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 `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 `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 `ccsCompliance` for concise CCS-specific rule checks. Provide at least 1 row, and use 2-5 when possible. `status` = `pass` | `fail` | `na`.
+19 -20
View File
@@ -646,23 +646,21 @@ function renderFindingSnippets(snippets) {
const lines = []; const lines = [];
for (const snippet of snippets) { for (const snippet of snippets) {
const label = snippet.label ? `Evidence: ${renderInlineText(snippet.label)}` : 'Evidence:'; const label = snippet.label ? `Evidence: ${renderInlineText(snippet.label)}` : 'Evidence:';
lines.push('', ` ${label}`, ''); if (lines.length > 0) {
lines.push( lines.push('');
...renderCodeBlock(snippet.code, snippet.language) }
.split('\n') lines.push(label, '', ...renderCodeBlock(snippet.code, snippet.language).split('\n'));
.map((line) => ` ${line}`)
);
} }
return lines; return lines;
} }
function renderDetailsBlock(summary, bodyLines) { function renderSection(title, bodyLines) {
if (!bodyLines.length) { if (!bodyLines.length) {
return []; return [];
} }
return ['', '<details>', `<summary>${escapeMarkdown(summary)}</summary>`, '', ...bodyLines, '', '</details>']; return ['', title, '', ...bodyLines];
} }
function renderFindingReference(finding) { function renderFindingReference(finding) {
@@ -705,11 +703,12 @@ function renderDetailedFindings(findings) {
if (scopedFindings.length === 0) continue; if (scopedFindings.length === 0) continue;
lines.push(`**${SEVERITY_SUMMARY_LABELS[severity]} (${scopedFindings.length})**`, ''); lines.push(`**${SEVERITY_SUMMARY_LABELS[severity]} (${scopedFindings.length})**`, '');
for (const finding of scopedFindings) { for (const [index, finding] of scopedFindings.entries()) {
lines.push(`- **${renderCode(renderFindingReference(finding))} ${renderInlineText(finding.title)}**`); lines.push(`#### ${index + 1}. ${renderInlineText(finding.title)}`);
lines.push(` Problem: ${renderInlineText(finding.what)}`); lines.push(`- Location: ${renderCode(renderFindingReference(finding))}`);
lines.push(` Why it matters: ${renderInlineText(finding.why)}`); lines.push(`- Impact: ${renderInlineText(finding.why)}`);
lines.push(` Suggested fix: ${renderInlineText(finding.fix)}`); lines.push(`- Problem: ${renderInlineText(finding.what)}`);
lines.push(`- Fix: ${renderInlineText(finding.fix)}`);
lines.push(...renderFindingSnippets(finding.snippets)); lines.push(...renderFindingSnippets(finding.snippets));
lines.push(''); lines.push('');
} }
@@ -738,21 +737,21 @@ export function renderStructuredReview(review, { model, rendering: renderOptions
} }
lines.push('', '### Top Findings', '', ...renderTopFindings(review.findings)); 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( lines.push(
...renderDetailsBlock( ...renderSection(
`Security Checklist (${review.securityChecklist.length})`, `### Security Checklist (${review.securityChecklist.length})`,
renderChecklistTable('Check', 'check', review.securityChecklist) renderChecklistTable('Check', 'check', review.securityChecklist)
) )
); );
lines.push( lines.push(
...renderDetailsBlock( ...renderSection(
`CCS Compliance (${review.ccsCompliance.length})`, `### CCS Compliance (${review.ccsCompliance.length})`,
renderChecklistTable('Rule', 'rule', review.ccsCompliance) renderChecklistTable('Rule', 'rule', review.ccsCompliance)
) )
); );
lines.push(...renderDetailsBlock(`Informational (${review.informational.length})`, renderBulletSection(review.informational))); lines.push(...renderSection(`### Informational (${review.informational.length})`, renderBulletSection(review.informational)));
lines.push(...renderDetailsBlock(`What's Done Well (${review.strengths.length})`, renderBulletSection(review.strengths))); lines.push(...renderSection(`### What's Done Well (${review.strengths.length})`, renderBulletSection(review.strengths)));
lines.push( lines.push(
'', '',
@@ -57,16 +57,17 @@ describe('normalize-ai-review-output', () => {
expect(markdown).toContain('### Verdict'); expect(markdown).toContain('### Verdict');
expect(markdown).toContain('### Top Findings'); expect(markdown).toContain('### Top Findings');
expect(markdown).toContain('- 🔴 High `src/cliproxy/accounts/query.ts:61` — Ambiguous account lookup drops valid matches'); expect(markdown).toContain('- 🔴 High `src/cliproxy/accounts/query.ts:61` — Ambiguous account lookup drops valid matches');
expect(markdown).toContain('<summary>Full Findings (1)</summary>'); expect(markdown).toContain('### Detailed Findings (1)');
expect(markdown).toContain('**`src/cliproxy/accounts/query.ts:61` — Ambiguous account lookup drops valid matches**'); expect(markdown).toContain('#### 1. Ambiguous account lookup drops valid matches');
expect(markdown).toContain('<summary>Security Checklist (1)</summary>'); 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('| Injection safety | ✅ | No user-controlled input reaches a shell, SQL, or HTML boundary in this diff. |');
expect(markdown).toContain('<summary>CCS Compliance (1)</summary>'); 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('| No emojis in CLI | N/A | This change affects GitHub PR comments only, not CLI stdout. |');
expect(markdown).toContain('<summary>Informational (1)</summary>'); expect(markdown).toContain('### Informational (1)');
expect(markdown).toContain("<summary>What's Done Well (1)</summary>"); expect(markdown).toContain("### What's Done Well (1)");
expect(markdown).toContain('**❌ CHANGES REQUESTED**'); 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`'); expect(markdown).toContain('> 🤖 Reviewed by `glm-5-turbo`');
}); });
@@ -452,10 +453,11 @@ describe('normalize-ai-review-output', () => {
const markdown = fs.readFileSync(outputFile, 'utf8'); const markdown = fs.readFileSync(outputFile, 'utf8');
expect(markdown).toContain('Summary with \\`code\\` and ## heading markers.'); 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('Problem: Problem text uses \\*\\*bold\\*\\* markers.');
expect(markdown).toContain('Why it matters: Why text uses \\[link\\] syntax.'); expect(markdown).toContain('Impact: Why text uses \\[link\\] syntax.');
expect(markdown).toContain('Suggested fix: Fix text uses \\<html\\> markers.'); expect(markdown).toContain('Fix: Fix text uses \\<html\\> markers.');
expect(markdown).toContain('Notes with a pipe \\| still render safely in table cells.'); expect(markdown).toContain('Notes with a pipe \\| still render safely in table cells.');
expect(markdown).toContain('- Informational item with \\`inline code\\`.'); expect(markdown).toContain('- Informational item with \\`inline code\\`.');
expect(markdown).toContain('- Strength with \\*\\*bold\\*\\* markers.'); expect(markdown).toContain('- Strength with \\*\\*bold\\*\\* markers.');
@@ -512,11 +514,11 @@ describe('normalize-ai-review-output', () => {
expect(markdown).toContain('### Top Findings'); expect(markdown).toContain('### Top Findings');
expect(markdown).toContain('No confirmed issues found after reviewing the diff and surrounding code.'); expect(markdown).toContain('No confirmed issues found after reviewing the diff and surrounding code.');
expect(markdown).toContain('<summary>Security Checklist (1)</summary>'); expect(markdown).toContain('### Security Checklist (1)');
expect(markdown).toContain( expect(markdown).toContain(
'| Injection safety | ✅ | No user-controlled data crosses a risky boundary in the reviewed diff. |' '| Injection safety | ✅ | No user-controlled data crosses a risky boundary in the reviewed diff. |'
); );
expect(markdown).toContain('<summary>CCS Compliance (1)</summary>'); 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('| 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.'); 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); expect(validation.ok).toBe(true);
const markdown = reviewOutput.renderStructuredReview(validation.value, { model: 'glm-5-turbo' }); const markdown = reviewOutput.renderStructuredReview(validation.value, { model: 'glm-5-turbo' });
expect(markdown).toContain( expect(markdown).toContain('#### 1. Missing empty-state coverage');
'**`tests/unit/scripts/github/normalize-ai-review-output.test.ts` — 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:`'); expect(markdown).not.toContain('normalize-ai-review-output.test.ts:`');
}); });
@@ -579,7 +580,7 @@ describe('normalize-ai-review-output', () => {
{ model: 'glm-5-turbo' } { 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', () => { test('rejects empty checklist sections instead of synthesizing placeholder rows', () => {