From 4e05488c195161792cdc0e74cbd21ed032697a3a Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 1 Apr 2026 22:20:55 -0400 Subject: [PATCH] hotfix(ci): keep ai review feedback fast and deterministic --- .github/review-prompt.md | 1 + .github/workflows/ai-review.yml | 6 +- scripts/github/normalize-ai-review-output.mjs | 74 ++++++++++++++++++- scripts/github/prepare-ai-review-scope.mjs | 29 +++++++- .../scripts/github/ai-review-workflow.test.ts | 6 ++ .../github/normalize-ai-review-output.test.ts | 18 +++++ .../github/prepare-ai-review-scope.test.ts | 64 ++++++++++++++++ 7 files changed, 192 insertions(+), 6 deletions(-) diff --git a/.github/review-prompt.md b/.github/review-prompt.md index 06274eb5..d2f8376b 100644 --- a/.github/review-prompt.md +++ b/.github/review-prompt.md @@ -20,6 +20,7 @@ Use only the review contract in this file plus the checked-out PR diff and nearb - Do not pad the review with praise or generic best-practice advice. - Read `.ccs-ai-review-scope.md` first when it is present. It defines the bounded review scope for this run. - If the mode is `triage`, be explicit in the summary that the review was hotspot-based rather than exhaustive. +- Do not spend turns narrating your process. Read the bounded inputs, confirm issues, and emit the schema. ## Core Questions diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 2e2e7071..17361ede 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -402,6 +402,7 @@ jobs: AI_REVIEW_PR_NUMBER: ${{ needs.prepare.outputs.pr_number }} AI_REVIEW_BASE_REF: ${{ needs.prepare.outputs.base_ref }} AI_REVIEW_MODE: ${{ needs.prepare.outputs.review_mode }} + AI_REVIEW_PR_SIZE_CLASS: ${{ needs.prepare.outputs.pr_size_class }} AI_REVIEW_MAX_TURNS: ${{ needs.prepare.outputs.max_turns }} AI_REVIEW_TIMEOUT_MINUTES: ${{ needs.prepare.outputs.claude_timeout_minutes }} AI_REVIEW_SCOPE_FILE: ${{ env.REVIEW_SCOPE_FILE }} @@ -518,6 +519,7 @@ jobs: - Respect the selected review mode budget and scope. Do not try to exhaustively map the repository on auto runs. - If the mode is `triage`, prioritize hotspots and be explicit that the review was bounded rather than exhaustive. - Do not reconstruct the full PR diff for `fast` or `triage` reviews, and do not look for omitted files outside the bounded workspace. + - Do not narrate your process or summarize interim reads. Spend turns on bounded file reads and the final JSON output only. - Return only structured output that matches the provided JSON schema. - Do NOT write files. - Do NOT post GitHub comments yourself. @@ -528,7 +530,8 @@ jobs: --model ${{ env.REVIEW_MODEL }} --permission-mode bypassPermissions --max-turns ${{ needs.prepare.outputs.max_turns }} - --allowedTools "Read" + --tools "Read" + --disallowedTools "Bash,Edit" --json-schema '${{ env.REVIEW_OUTPUT_SCHEMA }}' - name: Render review comment @@ -554,6 +557,7 @@ jobs: AI_REVIEW_TIMEOUT_MINUTES: ${{ needs.prepare.outputs.claude_timeout_minutes }} AI_REVIEW_STATUS: ${{ steps.claude-review.outcome }} AI_REVIEW_STRUCTURED_OUTPUT: ${{ steps.claude-review.outputs.structured_output }} + AI_REVIEW_SCOPE_MANIFEST_FILE: ${{ env.REVIEW_SCOPE_MANIFEST_FILE }} - name: Publish review comment if: always() && steps.app-token.outcome == 'success' diff --git a/scripts/github/normalize-ai-review-output.mjs b/scripts/github/normalize-ai-review-output.mjs index 5a7c304b..85c8e155 100644 --- a/scripts/github/normalize-ai-review-output.mjs +++ b/scripts/github/normalize-ai-review-output.mjs @@ -290,6 +290,66 @@ function readExecutionMetadata(executionFile) { } } +function readSelectedFiles(manifestFile) { + if (!manifestFile || !fs.existsSync(manifestFile)) { + return []; + } + + try { + return fs + .readFileSync(manifestFile, 'utf8') + .split('\n') + .map((line) => cleanText(line)) + .filter(Boolean); + } catch { + return []; + } +} + +function formatHotspotFiles(files) { + if (!files.length) { + return null; + } + + const visible = files.slice(0, 4).map(renderCode).join(', '); + return files.length > 4 ? `${visible}, and ${files.length - 4} more` : visible; +} + +function formatRemainingCoverage(rendering) { + if ( + typeof rendering.selectedFiles !== 'number' || + typeof rendering.reviewableFiles !== 'number' + ) { + return null; + } + + const remainingFiles = Math.max(rendering.reviewableFiles - rendering.selectedFiles, 0); + const hasChangeCounts = + typeof rendering.selectedChanges === 'number' && + typeof rendering.reviewableChanges === 'number'; + const remainingChanges = hasChangeCounts + ? Math.max(rendering.reviewableChanges - rendering.selectedChanges, 0) + : null; + + if (remainingFiles === 0 && (!hasChangeCounts || remainingChanges === 0)) { + return null; + } + + if (typeof remainingChanges === 'number') { + return `${remainingFiles} file${remainingFiles === 1 ? '' : 's'}; ${remainingChanges} changed lines`; + } + + return `${remainingFiles} file${remainingFiles === 1 ? '' : 's'}`; +} + +function formatFallbackFollowUp(rendering) { + if (rendering.mode === 'triage') { + return 'Focus manual review on the hotspot files above, and use `/review` for a deeper pass when release, auth, config, or workflow paths changed.'; + } + + return 'Use `/review` when you need a deeper maintainer rerun with more surrounding context.'; +} + export function normalizeStructuredOutput(raw) { if (!raw) { return { ok: false, reason: 'missing structured output' }; @@ -461,6 +521,7 @@ export function renderIncompleteReview({ runUrl, runtimeTools, turnsUsed, + selectedFiles, rendering: renderOptions, status, }) { @@ -468,7 +529,7 @@ export function renderIncompleteReview({ const lines = [ '### ⚠️ AI Review Incomplete', '', - 'Claude did not return validated structured review output, so this workflow did not publish raw scratch text.', + 'Claude did not return validated structured review output, so this workflow published deterministic hotspot context instead of raw scratch text.', '', `- Outcome: ${describeIncompleteOutcome({ reason, rendering, turnsUsed, status })}`, ]; @@ -484,6 +545,15 @@ export function renderIncompleteReview({ if (runtimeBudget) { lines.push(`- Runtime budget: ${escapeMarkdownText(runtimeBudget)}`); } + const hotspotFiles = formatHotspotFiles(selectedFiles || []); + if (hotspotFiles) { + lines.push(`- Hotspot files in this pass: ${hotspotFiles}`); + } + const remainingCoverage = formatRemainingCoverage(rendering); + if (remainingCoverage) { + lines.push(`- Remaining reviewable scope not fully covered: ${escapeMarkdownText(remainingCoverage)}`); + } + lines.push(`- Manual follow-up: ${escapeMarkdownText(formatFallbackFollowUp(rendering))}`); if (runtimeTools?.length) { lines.push(`- Runtime tools: ${runtimeTools.map(renderCode).join(', ')}`); } @@ -501,6 +571,7 @@ export function writeReviewFromEnv(env = process.env) { const runUrl = env.AI_REVIEW_RUN_URL || '#'; const validation = normalizeStructuredOutput(env.AI_REVIEW_STRUCTURED_OUTPUT); const metadata = readExecutionMetadata(env.AI_REVIEW_EXECUTION_FILE); + const selectedFiles = readSelectedFiles(env.AI_REVIEW_SCOPE_MANIFEST_FILE); const status = cleanText(env.AI_REVIEW_STATUS).toLowerCase() || null; const rendering = normalizeRenderingMetadata({ mode: env.AI_REVIEW_MODE, @@ -521,6 +592,7 @@ export function writeReviewFromEnv(env = process.env) { runUrl, runtimeTools: metadata.runtimeTools, turnsUsed: metadata.turnsUsed, + selectedFiles, rendering, status, }); diff --git a/scripts/github/prepare-ai-review-scope.mjs b/scripts/github/prepare-ai-review-scope.mjs index d0ef12a3..2c9cd281 100644 --- a/scripts/github/prepare-ai-review-scope.mjs +++ b/scripts/github/prepare-ai-review-scope.mjs @@ -4,10 +4,14 @@ import { fileURLToPath } from 'node:url'; const MODE_LIMITS = { fast: { maxFiles: 16, maxChangedLines: 900, maxPatchLines: 90, maxPatchChars: 7000 }, - triage: { maxFiles: 10, maxChangedLines: 700, maxPatchLines: 80, maxPatchChars: 6000 }, + triage: { maxFiles: 6, maxChangedLines: 520, maxPatchLines: 60, maxPatchChars: 4500 }, deep: { maxFiles: 20, maxChangedLines: 1600, maxPatchLines: 120, maxPatchChars: 9000 }, }; +const TRIAGE_SIZE_CLASS_LIMITS = { + xlarge: { maxFiles: 4, maxChangedLines: 360, maxPatchLines: 45, maxPatchChars: 3200 }, +}; + const MODE_LABELS = { fast: 'diff-focused bounded review', triage: 'hotspot-based bounded review (non-exhaustive)', @@ -35,6 +39,13 @@ function cleanText(value) { return typeof value === 'string' ? value.trim().replace(/\s+/g, ' ') : ''; } +function normalizeSizeClass(value) { + const sizeClass = cleanText(value).toLowerCase(); + return sizeClass === 'small' || sizeClass === 'medium' || sizeClass === 'large' || sizeClass === 'xlarge' + ? sizeClass + : null; +} + function escapeMarkdown(value) { return cleanText(value).replace(/\\/g, '\\\\').replace(/([`*_{}[\]<>|])/g, '\\$1'); } @@ -127,8 +138,17 @@ export function normalizePullFiles(files) { }).map((file) => ({ ...file, score: scoreFile(file) })); } -export function buildReviewScope(files, mode) { - const limits = MODE_LIMITS[mode] || MODE_LIMITS.fast; +function resolveModeLimits(mode, sizeClass) { + if (mode !== 'triage') { + return MODE_LIMITS[mode] || MODE_LIMITS.fast; + } + + return TRIAGE_SIZE_CLASS_LIMITS[sizeClass] || MODE_LIMITS.triage; +} + +export function buildReviewScope(files, mode, options = {}) { + const sizeClass = normalizeSizeClass(options.sizeClass); + const limits = resolveModeLimits(mode, sizeClass); const reviewable = files.filter((file) => file.reviewable); const lowSignal = files.filter((file) => !file.reviewable); const usingChangedFallback = reviewable.length === 0; @@ -260,6 +280,7 @@ export async function writeScopeFromEnv(env = process.env, request) { const prNumber = Number.parseInt(cleanText(env.AI_REVIEW_PR_NUMBER), 10); const baseRef = cleanText(env.AI_REVIEW_BASE_REF || 'dev'); const mode = cleanText(env.AI_REVIEW_MODE || 'fast').toLowerCase(); + const sizeClass = normalizeSizeClass(env.AI_REVIEW_PR_SIZE_CLASS); const turnBudget = Number.parseInt(cleanText(env.AI_REVIEW_MAX_TURNS || '0'), 10) || 0; const timeoutMinutes = Number.parseInt(cleanText(env.AI_REVIEW_TIMEOUT_MINUTES || '0'), 10) || 0; const outputFile = env.AI_REVIEW_SCOPE_FILE || '.ccs-ai-review-scope.md'; @@ -287,7 +308,7 @@ export async function writeScopeFromEnv(env = process.env, request) { const files = normalizePullFiles( await collectPullRequestFiles(`${apiUrl}/repos/${repository}/pulls/${prNumber}/files?per_page=100`, fetchPage) ); - const scope = buildReviewScope(files, mode); + const scope = buildReviewScope(files, mode, { sizeClass }); const markdown = renderReviewScope({ prNumber, baseRef, turnBudget, timeoutMinutes, scope }); fs.mkdirSync(path.dirname(outputFile), { recursive: true }); diff --git a/tests/unit/scripts/github/ai-review-workflow.test.ts b/tests/unit/scripts/github/ai-review-workflow.test.ts index e786e2a0..25afbd57 100644 --- a/tests/unit/scripts/github/ai-review-workflow.test.ts +++ b/tests/unit/scripts/github/ai-review-workflow.test.ts @@ -35,10 +35,16 @@ describe('ai-review workflow', () => { expect(claudeReviewStep?.with?.path_to_claude_code_executable).toBe( '${{ steps.toolchain.outputs.claude_path }}' ); + expect(claudeReviewStep?.with?.claude_args).toContain('--tools "Read"'); + expect(claudeReviewStep?.with?.claude_args).toContain('--disallowedTools "Bash,Edit"'); const promptStep = steps.find((step) => step.id === 'review-prompt'); expect(promptStep).toBeDefined(); expect(promptStep?.run).toContain("printf '%s\\n' \\"); expect(promptStep?.run).not.toContain("| sed 's/^ //'"); + + const reviewScopeStep = steps.find((step) => step.id === 'review-scope'); + expect(reviewScopeStep).toBeDefined(); + expect(reviewScopeStep?.env?.AI_REVIEW_PR_SIZE_CLASS).toBe('${{ needs.prepare.outputs.pr_size_class }}'); }); }); 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 1da49d26..1e1742ae 100644 --- a/tests/unit/scripts/github/normalize-ai-review-output.test.ts +++ b/tests/unit/scripts/github/normalize-ai-review-output.test.ts @@ -146,6 +146,7 @@ describe('normalize-ai-review-output', () => { test('writes a safe incomplete comment with mode and runtime context instead of leaking raw assistant text', () => { withTempDir('ai-review-', (tempDir) => { const executionFile = path.join(tempDir, 'claude-execution-output.json'); + const manifestFile = path.join(tempDir, 'selected-files.txt'); const outputFile = path.join(tempDir, 'pr_review.md'); fs.writeFileSync( @@ -160,6 +161,10 @@ describe('normalize-ai-review-output', () => { }, ]) ); + fs.writeFileSync( + manifestFile, + ['.github/workflows/ai-review.yml', 'scripts/github/prepare-ai-review-scope.mjs', 'src/ccs.ts'].join('\n') + ); const result = reviewOutput.writeReviewFromEnv({ AI_REVIEW_EXECUTION_FILE: executionFile, @@ -173,6 +178,7 @@ describe('normalize-ai-review-output', () => { AI_REVIEW_TIMEOUT_MINUTES: '5', AI_REVIEW_OUTPUT_FILE: outputFile, AI_REVIEW_RUN_URL: 'https://github.com/kaitranntt/ccs/actions/runs/23758377592', + AI_REVIEW_SCOPE_MANIFEST_FILE: manifestFile, AI_REVIEW_STRUCTURED_OUTPUT: '', }); @@ -186,6 +192,11 @@ describe('normalize-ai-review-output', () => { expect(markdown).toContain('- Review mode: `triage` (hotspot-based bounded review (non-exhaustive))'); expect(markdown).toContain('- Review scope: 10/46 reviewable files; 700/2310 reviewable changed lines'); expect(markdown).toContain('- Runtime budget: 25 turns / 5 minutes'); + expect(markdown).toContain( + '- Hotspot files in this pass: `.github/workflows/ai-review.yml`, `scripts/github/prepare-ai-review-scope.mjs`, `src/ccs.ts`' + ); + expect(markdown).toContain('- Remaining reviewable scope not fully covered: 36 files; 1610 changed lines'); + expect(markdown).toContain('- Manual follow-up: Focus manual review on the hotspot files above'); expect(markdown).toContain('Runtime tools: `Bash`, `Edit`, `Read`'); expect(markdown).toContain('Turns used: 25'); expect(markdown).not.toContain('Now let me verify the findings'); @@ -195,6 +206,7 @@ describe('normalize-ai-review-output', () => { test('uses a timeout-safe fallback message when the bounded review hits the workflow cap', () => { withTempDir('ai-review-', (tempDir) => { const executionFile = path.join(tempDir, 'claude-execution-output.json'); + const manifestFile = path.join(tempDir, 'selected-files.txt'); const outputFile = path.join(tempDir, 'pr_review.md'); fs.writeFileSync( @@ -209,6 +221,7 @@ describe('normalize-ai-review-output', () => { }, ]) ); + fs.writeFileSync(manifestFile, ['src/commands/help-command.ts', 'src/ccs.ts'].join('\n')); const result = reviewOutput.writeReviewFromEnv({ AI_REVIEW_EXECUTION_FILE: executionFile, @@ -223,6 +236,7 @@ describe('normalize-ai-review-output', () => { AI_REVIEW_STATUS: 'cancelled', AI_REVIEW_OUTPUT_FILE: outputFile, AI_REVIEW_RUN_URL: 'https://github.com/kaitranntt/ccs/actions/runs/23758377592', + AI_REVIEW_SCOPE_MANIFEST_FILE: manifestFile, AI_REVIEW_STRUCTURED_OUTPUT: '', }); @@ -235,6 +249,10 @@ describe('normalize-ai-review-output', () => { expect(markdown).toContain('- Review mode: `fast` (diff-focused bounded review)'); expect(markdown).toContain('- Review scope: 6/52 reviewable files; 640/2480 reviewable changed lines'); expect(markdown).toContain('- Runtime budget: 5 turns / 5 minutes'); + expect(markdown).toContain( + '- Hotspot files in this pass: `src/commands/help-command.ts`, `src/ccs.ts`' + ); + expect(markdown).toContain('- Remaining reviewable scope not fully covered: 46 files; 1840 changed lines'); expect(markdown).not.toContain('Partial draft that should never reach the published markdown.'); }); }); diff --git a/tests/unit/scripts/github/prepare-ai-review-scope.test.ts b/tests/unit/scripts/github/prepare-ai-review-scope.test.ts index 0865a301..10a5e691 100644 --- a/tests/unit/scripts/github/prepare-ai-review-scope.test.ts +++ b/tests/unit/scripts/github/prepare-ai-review-scope.test.ts @@ -113,6 +113,70 @@ describe('prepare-ai-review-scope', () => { expect(scope.scopeLabel).toBe('changed files'); }); + test('shrinks triage scope for xlarge PRs to keep hotspot reviews fast', () => { + const scope = reviewScope.buildReviewScope( + reviewScope.normalizePullFiles([ + { + filename: '.github/workflows/ai-review.yml', + status: 'modified', + additions: 130, + deletions: 30, + changes: 160, + patch: '@@ -1 +1 @@\n-old\n+new', + }, + { + filename: 'scripts/github/prepare-ai-review-scope.mjs', + status: 'modified', + additions: 120, + deletions: 20, + changes: 140, + patch: '@@ -1 +1 @@\n-old\n+new', + }, + { + filename: 'src/commands/help-command.ts', + status: 'modified', + additions: 70, + deletions: 10, + changes: 80, + patch: '@@ -1 +1 @@\n-old\n+new', + }, + { + filename: 'src/ccs.ts', + status: 'modified', + additions: 50, + deletions: 15, + changes: 65, + patch: '@@ -1 +1 @@\n-old\n+new', + }, + { + filename: 'tests/unit/commands/help-command.test.ts', + status: 'modified', + additions: 40, + deletions: 5, + changes: 45, + patch: '@@ -1 +1 @@\n-old\n+new', + }, + ]), + 'triage', + { sizeClass: 'xlarge' } + ); + + expect(scope.selected.length).toBeLessThanOrEqual(4); + expect(scope.selected.map((file: { filename: string }) => file.filename)).toEqual( + expect.arrayContaining([ + '.github/workflows/ai-review.yml', + 'scripts/github/prepare-ai-review-scope.mjs', + ]) + ); + expect(scope.selectedChanges).toBeLessThanOrEqual(360); + expect(scope.limits).toEqual({ + maxFiles: 4, + maxChangedLines: 360, + maxPatchLines: 45, + maxPatchChars: 3200, + }); + }); + test('renders deterministic scope metadata and fences patch content safely', () => { const oversizedPatch = ['+line 1', '```', ...Array.from({ length: 118 }, (_, index) => `+line ${index + 2}`)].join('\n'); const scope = reviewScope.buildReviewScope(