From ccdde892c565433c8e10af6ee35f00164d1fa8ad Mon Sep 17 00:00:00 2001 From: "Dina Berry (She/her)" Date: Sat, 4 Apr 2026 07:18:13 -0700 Subject: [PATCH] feat(ci): add file list with line stats, scope badge, and check subtitles to PR readiness - Add file list table with per-file +additions/-deletions stats - Add PR scope classification (Product/Infrastructure/Mixed) - Rename Architectural Review and Security Review checks with descriptive subtitles Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .changeset/readiness-file-list.md | 7 + .github/workflows/squad-repo-health.yml | 4 +- CONTRIBUTING.md | 2 + scripts/pr-readiness.mjs | 103 +++++++++- test/pr-readiness.test.ts | 249 +++++++++++++++++++++++- 5 files changed, 358 insertions(+), 7 deletions(-) create mode 100644 .changeset/readiness-file-list.md diff --git a/.changeset/readiness-file-list.md b/.changeset/readiness-file-list.md new file mode 100644 index 000000000..5cc36c951 --- /dev/null +++ b/.changeset/readiness-file-list.md @@ -0,0 +1,7 @@ +--- +--- + +ci: add file list with line stats to PR readiness comment + +The PR readiness bot now shows changed files with per-file addition/deletion +counts, scope classification (Product/Infrastructure/Mixed), and totals. diff --git a/.github/workflows/squad-repo-health.yml b/.github/workflows/squad-repo-health.yml index a559fc353..4fd722927 100644 --- a/.github/workflows/squad-repo-health.yml +++ b/.github/workflows/squad-repo-health.yml @@ -113,7 +113,7 @@ jobs: # ─── Architectural Review (INFORMATIONAL) ─────────────────────────── architectural-review: - name: Architectural Review + name: Architectural Review — Structure & Design Rules runs-on: ubuntu-latest timeout-minutes: 5 if: github.actor != 'dependabot[bot]' @@ -150,7 +150,7 @@ jobs: # ─── Security Review (INFORMATIONAL) ──────────────────────────────── security-review: - name: Security Review + name: Security Review — Permissions & Secrets runs-on: ubuntu-latest timeout-minutes: 5 if: github.actor != 'dependabot[bot]' diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d64591dbc..ff58fd677 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -170,6 +170,8 @@ An automated readiness check runs on every PR and posts a checklist comment. Add | **No merge conflicts** | Resolve any conflicts with the target branch | | **CI passing** | All CI checks (build, test, lint) must be green | +The readiness comment also includes a **file list with line stats** — each changed file is shown with per-file addition/deletion counts, a scope classification (Product/Infrastructure/Mixed), and totals. This helps reviewers quickly gauge PR size and impact. + The readiness check is **informational** — it helps you self-serve before a human reviewer looks at your PR. It automatically re-runs after Squad CI completes, so the checklist stays up to date without manual intervention. See `.github/PR_REQUIREMENTS.md` for the full requirements spec. ## Code Style & Conventions diff --git a/scripts/pr-readiness.mjs b/scripts/pr-readiness.mjs index 20ca9d851..7b49cbdd5 100644 --- a/scripts/pr-readiness.mjs +++ b/scripts/pr-readiness.mjs @@ -246,6 +246,86 @@ export function checkCIStatus(checkRuns, statuses) { return { pass: true, detail: 'All checks passing' }; } +// --------------------------------------------------------------------------- +// Scope classification +// --------------------------------------------------------------------------- + +/** + * Classify the PR scope based on changed files. + * @param {Array<{ filename: string }>} files + * @returns {{ label: string, emoji: string }} + */ +export function classifyScope(files) { + const hasProduct = (files || []).some((f) => SOURCE_PATTERN.test(f.filename)); + const hasInfra = (files || []).some((f) => !SOURCE_PATTERN.test(f.filename)); + + if (hasProduct && hasInfra) { + return { label: 'Mixed (product + infrastructure)', emoji: '📦🔧' }; + } + if (hasProduct) { + return { label: 'Product', emoji: '📦' }; + } + return { label: 'Infrastructure', emoji: '🔧' }; +} + +// --------------------------------------------------------------------------- +// File list builder +// --------------------------------------------------------------------------- + +/** Maximum files shown in the file list before truncation. */ +export const MAX_FILE_LIST = 50; + +/** + * Sanitize a filename for safe inclusion in a markdown table cell. + * Escapes pipe characters, replaces backticks, and collapses newlines. + * @param {string} name + * @returns {string} + */ +export function sanitizeFilename(name) { + return name + .replace(/\|/g, '\\|') + .replace(/`/g, "'") + .replace(/[\r\n]+/g, ' '); +} + +/** + * Build a markdown section listing changed files with per-file line stats. + * @param {Array<{ filename: string, additions?: number, deletions?: number }>} files + * @returns {string} + */ +export function buildFileList(files) { + if (!files || files.length === 0) { + return ''; + } + + const totalAdded = files.reduce((sum, f) => sum + (f.additions || 0), 0); + const totalDeleted = files.reduce((sum, f) => sum + (f.deletions || 0), 0); + + const displayed = files.slice(0, MAX_FILE_LIST); + const truncated = files.length > MAX_FILE_LIST; + + const rows = displayed.map((f) => { + const added = f.additions || 0; + const deleted = f.deletions || 0; + return `| \`${sanitizeFilename(f.filename)}\` | +${added} −${deleted} |`; + }); + + if (truncated) { + const remaining = files.length - MAX_FILE_LIST; + rows.push(`| ... | **+${remaining} more files** | |`); + } + + return [ + `### Files Changed (${files.length} file${files.length === 1 ? '' : 's'}, +${totalAdded} −${totalDeleted})`, + '', + '| File | +/− |', + '|------|-----|', + ...rows, + '', + `**Total: +${totalAdded} −${totalDeleted}**`, + ].join('\n'); +} + // --------------------------------------------------------------------------- // Checklist markdown builder // --------------------------------------------------------------------------- @@ -257,9 +337,10 @@ export function checkCIStatus(checkRuns, statuses) { * @param {string} repo * @param {string} baseRef * @param {string} [headSha] — commit SHA that triggered the check + * @param {Array<{ filename: string, additions?: number, deletions?: number }>} [files] * @returns {string} */ -export function buildChecklist(checks, owner, repo, baseRef, headSha) { +export function buildChecklist(checks, owner, repo, baseRef, headSha, files) { const allPass = checks.every((c) => c.pass); const passCount = checks.filter((c) => c.pass).length; @@ -272,23 +353,37 @@ export function buildChecklist(checks, owner, repo, baseRef, headSha) { return `| ${icon} | **${c.name}** | ${c.detail} |`; }); - return [ + const scope = classifyScope(files); + + const sections = [ COMMENT_MARKER, '## 🛫 PR Readiness Check', ...(headSha ? [`> ℹ️ This comment updates on each push. Last checked: commit \`${headSha.slice(0, 7)}\``] : []), '', + `**PR Scope:** ${scope.emoji} ${scope.label}`, + '', status, '', '| Status | Check | Details |', '|--------|-------|---------|', ...rows, + ]; + + const fileList = buildFileList(files); + if (fileList) { + sections.push('', fileList); + } + + sections.push( '', '---', '*This check runs automatically on every push. Fix any ❌ items and push again.*', `*See [CONTRIBUTING.md](https://github.com/${owner}/${repo}/blob/${baseRef}/CONTRIBUTING.md#pr-readiness-checklist) and [PR Requirements](https://github.com/${owner}/${repo}/blob/${baseRef}/.github/PR_REQUIREMENTS.md) for details.*`, - ].join('\n'); + ); + + return sections.join('\n'); } // --------------------------------------------------------------------------- @@ -492,7 +587,7 @@ export async function run({ env = process.env, fetchFn = globalThis.fetch } = {} checks.push({ name: 'CI passing', ...checkCIStatus(checkRuns, statusEntries) }); // ── Build checklist and upsert comment ── - const body = buildChecklist(checks, owner, repo, prBaseRef, prHeadSha); + const body = buildChecklist(checks, owner, repo, prBaseRef, prHeadSha, files); // Find existing comment const existingComments = await paginate( diff --git a/test/pr-readiness.test.ts b/test/pr-readiness.test.ts index 05333c723..c78425cfe 100644 --- a/test/pr-readiness.test.ts +++ b/test/pr-readiness.test.ts @@ -17,6 +17,10 @@ import { checkCopilotThreads, checkCIStatus, buildChecklist, + buildFileList, + sanitizeFilename, + MAX_FILE_LIST, + classifyScope, paginate, run, COMMENT_MARKER, @@ -495,6 +499,229 @@ describe('buildChecklist', () => { expect(body).toContain('| ✅ | **A** | OK |'); expect(body).toContain('| ❌ | **B** | Bad |'); }); + + it('includes file list when files are provided', () => { + const checks = [{ name: 'Test', pass: true, detail: 'OK' }]; + const files = [ + { filename: 'src/index.ts', additions: 10, deletions: 3 }, + { filename: 'README.md', additions: 2, deletions: 1 }, + ]; + const body = buildChecklist(checks, 'o', 'r', 'dev', undefined, files); + expect(body).toContain('### Files Changed (2 files, +12 −4)'); + expect(body).toContain('| `src/index.ts` | +10 −3 |'); + expect(body).toContain('| `README.md` | +2 −1 |'); + expect(body).toContain('**Total: +12 −4**'); + }); + + it('omits file list when files are undefined', () => { + const checks = [{ name: 'Test', pass: true, detail: 'OK' }]; + const body = buildChecklist(checks, 'o', 'r', 'dev'); + expect(body).not.toContain('Files Changed'); + }); + + it('omits file list when files array is empty', () => { + const checks = [{ name: 'Test', pass: true, detail: 'OK' }]; + const body = buildChecklist(checks, 'o', 'r', 'dev', undefined, []); + expect(body).not.toContain('Files Changed'); + }); +}); + +// --------------------------------------------------------------------------- +// buildFileList +// --------------------------------------------------------------------------- + +describe('buildFileList', () => { + it('returns empty string for null/undefined input', () => { + expect(buildFileList(null)).toBe(''); + expect(buildFileList(undefined)).toBe(''); + }); + + it('returns empty string for empty array', () => { + expect(buildFileList([])).toBe(''); + }); + + it('renders a single file with correct stats', () => { + const files = [{ filename: 'scripts/moderate-spam.mjs', additions: 142, deletions: 0 }]; + const result = buildFileList(files); + expect(result).toContain('### Files Changed (1 file, +142 −0)'); + expect(result).toContain('| `scripts/moderate-spam.mjs` | +142 −0 |'); + expect(result).toContain('**Total: +142 −0**'); + }); + + it('renders multiple files with totals', () => { + const files = [ + { filename: 'scripts/moderate-spam.mjs', additions: 142, deletions: 0 }, + { filename: 'test/scripts/moderate-spam.test.ts', additions: 98, deletions: 0 }, + { filename: '.github/workflows/squad-comment-moderation.yml', additions: 45, deletions: 0 }, + ]; + const result = buildFileList(files); + expect(result).toContain('### Files Changed (3 files, +285 −0)'); + expect(result).toContain('| `scripts/moderate-spam.mjs` | +142 −0 |'); + expect(result).toContain('| `test/scripts/moderate-spam.test.ts` | +98 −0 |'); + expect(result).toContain('| `.github/workflows/squad-comment-moderation.yml` | +45 −0 |'); + expect(result).toContain('**Total: +285 −0**'); + }); + + it('handles files with 0 additions and 0 deletions', () => { + const files = [{ filename: 'empty-change.ts', additions: 0, deletions: 0 }]; + const result = buildFileList(files); + expect(result).toContain('| `empty-change.ts` | +0 −0 |'); + expect(result).toContain('**Total: +0 −0**'); + }); + + it('handles files with both additions and deletions', () => { + const files = [ + { filename: 'src/refactored.ts', additions: 50, deletions: 30 }, + { filename: 'src/old.ts', additions: 0, deletions: 100 }, + ]; + const result = buildFileList(files); + expect(result).toContain('### Files Changed (2 files, +50 −130)'); + expect(result).toContain('| `src/refactored.ts` | +50 −30 |'); + expect(result).toContain('| `src/old.ts` | +0 −100 |'); + expect(result).toContain('**Total: +50 −130**'); + }); + + it('treats missing additions/deletions as 0', () => { + const files = [{ filename: 'binary-file.png' }]; + const result = buildFileList(files); + expect(result).toContain('| `binary-file.png` | +0 −0 |'); + expect(result).toContain('**Total: +0 −0**'); + }); + + it('uses singular "file" for single file', () => { + const files = [{ filename: 'one.ts', additions: 1, deletions: 0 }]; + const result = buildFileList(files); + expect(result).toContain('1 file,'); + expect(result).not.toContain('1 files,'); + }); + + it('includes table headers', () => { + const files = [{ filename: 'a.ts', additions: 1, deletions: 0 }]; + const result = buildFileList(files); + expect(result).toContain('| File | +/− |'); + expect(result).toContain('|------|-----|'); + }); + + it('truncates file list beyond MAX_FILE_LIST and shows summary row', () => { + const files = Array.from({ length: 60 }, (_, i) => ({ + filename: `src/file-${i}.ts`, + additions: 1, + deletions: 0, + })); + const result = buildFileList(files); + // Header should show the total count (60), not the truncated count + expect(result).toContain('### Files Changed (60 files, +60 −0)'); + // First 50 files should be present + expect(result).toContain('`src/file-0.ts`'); + expect(result).toContain('`src/file-49.ts`'); + // File 50 should NOT be present + expect(result).not.toContain('`src/file-50.ts`'); + // Summary row + expect(result).toContain('**+10 more files**'); + // Totals should include ALL files + expect(result).toContain('**Total: +60 −0**'); + }); + + it('does not truncate when file count equals MAX_FILE_LIST', () => { + const files = Array.from({ length: MAX_FILE_LIST }, (_, i) => ({ + filename: `src/file-${i}.ts`, + additions: 1, + deletions: 0, + })); + const result = buildFileList(files); + expect(result).not.toContain('more files'); + expect(result).toContain(`${MAX_FILE_LIST} files`); + }); + + it('sanitizes pipe characters in filenames', () => { + const files = [{ filename: 'path/with|pipe.ts', additions: 1, deletions: 0 }]; + const result = buildFileList(files); + expect(result).toContain("path/with\\|pipe.ts"); + expect(result).not.toContain('| path/with|pipe.ts'); + }); + + it('sanitizes backticks in filenames', () => { + const files = [{ filename: 'file`name.ts', additions: 1, deletions: 0 }]; + const result = buildFileList(files); + expect(result).toContain("file'name.ts"); + }); + + it('sanitizes newlines in filenames', () => { + const files = [{ filename: 'file\nname.ts', additions: 1, deletions: 0 }]; + const result = buildFileList(files); + expect(result).toContain('file name.ts'); + // The sanitized filename should not contain the literal newline + expect(result).not.toContain('file\nname.ts'); + }); +}); + +// --------------------------------------------------------------------------- +// sanitizeFilename +// --------------------------------------------------------------------------- + +describe('sanitizeFilename', () => { + it('escapes pipe characters', () => { + expect(sanitizeFilename('a|b|c')).toBe('a\\|b\\|c'); + }); + + it('replaces backticks with single quotes', () => { + expect(sanitizeFilename('file`name`test')).toBe("file'name'test"); + }); + + it('replaces newlines with spaces', () => { + expect(sanitizeFilename('line1\nline2\r\nline3')).toBe('line1 line2 line3'); + }); + + it('handles all special characters together', () => { + expect(sanitizeFilename('a|b`c\nd')).toBe("a\\|b'c d"); + }); + + it('returns normal filenames unchanged', () => { + expect(sanitizeFilename('src/components/App.tsx')).toBe('src/components/App.tsx'); + }); +}); + +// --------------------------------------------------------------------------- +// classifyScope +// --------------------------------------------------------------------------- + +describe('classifyScope', () => { + it('returns Infrastructure for only infrastructure files', () => { + const files = [ + { filename: '.github/workflows/ci.yml' }, + { filename: 'scripts/build.mjs' }, + { filename: 'test/foo.test.ts' }, + ]; + const result = classifyScope(files); + expect(result.label).toBe('Infrastructure'); + expect(result.emoji).toBe('🔧'); + }); + + it('returns Product for only product source files', () => { + const files = [ + { filename: 'packages/squad-sdk/src/index.ts' }, + { filename: 'packages/squad-cli/src/main.ts' }, + ]; + const result = classifyScope(files); + expect(result.label).toBe('Product'); + expect(result.emoji).toBe('📦'); + }); + + it('returns Mixed for both product and infrastructure files', () => { + const files = [ + { filename: 'packages/squad-sdk/src/index.ts' }, + { filename: 'scripts/build.mjs' }, + ]; + const result = classifyScope(files); + expect(result.label).toBe('Mixed (product + infrastructure)'); + expect(result.emoji).toBe('📦🔧'); + }); + + it('returns Infrastructure for empty array', () => { + const result = classifyScope([]); + expect(result.label).toBe('Infrastructure'); + expect(result.emoji).toBe('🔧'); + }); }); // --------------------------------------------------------------------------- @@ -596,7 +823,7 @@ describe('run()', () => { commits: [{ sha: 'abc123' }], compare: { behind_by: 0 }, reviews: [{ user: { login: 'copilot-pull-request-reviewer' }, state: 'APPROVED', submitted_at: '2025-01-01T00:00:00Z' }], - files: [{ filename: '.changeset/feat.md' }], + files: [{ filename: '.changeset/feat.md', additions: 5, deletions: 0 }], pr: { mergeable: true }, checkRuns: { check_runs: [{ name: 'build', conclusion: 'success', status: 'completed' }] }, status: { statuses: [{ state: 'success' }] }, @@ -861,4 +1088,24 @@ describe('run()', () => { expect(draftCheck.pass).toBe(false); expect(draftCheck.detail).toContain('draft'); }); + + it('includes file list with line stats in upserted comment', async () => { + const mockFetch = createMockFetch({ + files: [ + { filename: 'src/index.ts', additions: 25, deletions: 10 }, + { filename: 'test/index.test.ts', additions: 50, deletions: 0 }, + ], + }); + await run({ env: baseEnv, fetchFn: mockFetch }); + + const postCall = mockFetch.mock.calls.find( + ([url, opts]) => opts && opts.method === 'POST' && url.includes('/comments'), + ); + expect(postCall).toBeDefined(); + const body = JSON.parse(postCall[1].body).body; + expect(body).toContain('### Files Changed (2 files, +75 −10)'); + expect(body).toContain('| `src/index.ts` | +25 −10 |'); + expect(body).toContain('| `test/index.test.ts` | +50 −0 |'); + expect(body).toContain('**Total: +75 −10**'); + }); });