From f9412140fdca01878af79643c151bb267d43f18c Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 19 Jan 2026 23:38:23 +0000 Subject: [PATCH] feat: add automated PR review script - Adds `scripts/agent/review-pr.mjs` to automate PR checks. - Supports iterating open PRs via `gh` CLI. - Runs `npm ci`, `format`, `lint`, and `test:unit`. - Reports results to PR comments. - Offers auto-fix for formatting changes. - Adds guardrails for sensitive files (Nostr, crypto). --- pr_review_report.md | 46 +++++ scripts/agent/review-pr.mjs | 382 ++++++++++++++++++++++++------------ 2 files changed, 302 insertions(+), 126 deletions(-) create mode 100644 pr_review_report.md diff --git a/pr_review_report.md b/pr_review_report.md new file mode 100644 index 00000000..f35aa228 --- /dev/null +++ b/pr_review_report.md @@ -0,0 +1,46 @@ +# Automated PR Review +**Context:** jules-17818997485755159011-27e733a1 +**Date:** 2026-01-19T23:31:08.588Z + +## ✅ Formatting +No formatting issues found. +## ⚠️ Linter Warnings/Errors +`npm run lint` reported issues: +
Show Lint Output + +``` + +> bitvid@1.0.0 lint +> npm run lint:css && npm run lint:hex && npm run lint:inline-styles && npm run lint:tokens && npm run lint:tailwind-brackets && npm run lint:tailwind-colors + + +> bitvid@1.0.0 lint:css +> stylelint css/tailwind.source.css css/tokens.css + + +> bitvid@1.0.0 lint:hex +> node scripts/check-hex.js + + +> bitvid@1.0.0 lint:inline-styles +> node scripts/check-inline-styles.mjs + +No inline style usage found. + +> bitvid@1.0.0 lint:tokens +> node scripts/check-design-tokens.mjs --check=tokens + + +Design token lint failed: raw measurements detected outside tokens. + +css/tailwind.source.css:5598 → 2rem + min-width: 2rem; +css/tailwind.source.css:5613 → 8rem + min-width: 8rem; + +Move these measurements into css/tokens.css or use the metrics helper. + +``` +
+## ❌ Test Timeout +`npm run test:unit` timed out after 5 minutes. \ No newline at end of file diff --git a/scripts/agent/review-pr.mjs b/scripts/agent/review-pr.mjs index 85fac75f..1cc93ef6 100644 --- a/scripts/agent/review-pr.mjs +++ b/scripts/agent/review-pr.mjs @@ -11,7 +11,7 @@ const SENSITIVE_PATTERNS = [ ]; function log(message) { - console.log(message); + console.log(`[ReviewAgent] ${message}`); } function runCommand(command, args = [], options = {}) { @@ -19,130 +19,191 @@ function runCommand(command, args = [], options = {}) { return spawnSync(command, args, { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], - shell: true, + shell: false, // Turn off shell for security unless needed, but spawnSync defaults to false. + // However, if I used shell: true before, args might need care. + // If shell is false, args are safe. ...options }); } +function checkGhAvailability() { + try { + const res = spawnSync('gh', ['--version'], { encoding: 'utf-8' }); + return res.status === 0; + } catch (e) { + return false; + } +} + function getChangedFiles(baseBranch = 'origin/main') { try { - const output = execSync(`git diff --name-only ${baseBranch}...HEAD`, { - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'ignore'] - }); - return output.trim().split('\n').filter(Boolean); + const res = spawnSync('git', ['diff', '--name-only', `${baseBranch}...HEAD`], { encoding: 'utf-8' }); + if (res.status !== 0) throw new Error(res.stderr); + return res.stdout.trim().split('\n').filter(Boolean); } catch (error) { log(`Warning: Could not determine changed files against ${baseBranch}. Using local changes if any.`); return []; } } -async function main() { - const branchArg = process.argv[2]; - let originalBranch; - - if (branchArg && !branchArg.startsWith('--')) { - try { - originalBranch = execSync('git branch --show-current', { encoding: 'utf-8' }).trim(); - log(`Switching to branch: ${branchArg}`); - execSync(`git checkout ${branchArg}`); - } catch (e) { - log(`Error: Could not checkout branch ${branchArg}. ${e.message}`); - process.exit(1); - } - } - +async function reviewContext(contextName, options) { const report = []; report.push(`# Automated PR Review`); - report.push(`**Branch:** ${branchArg || 'Current'}`); + report.push(`**Context:** ${contextName}`); report.push(`**Date:** ${new Date().toISOString()}`); report.push(''); + let failed = false; + try { - // 1. npm ci - log('Installing dependencies...'); - const ciResult = runCommand('npm', ['ci']); - if (ciResult.status !== 0) { - report.push(`## 🚨 Dependency Installation Failed`); - report.push('`npm ci` failed. Please check `package-lock.json`.'); - report.push('```'); - report.push(ciResult.stderr.slice(-1000)); // Last 1000 chars - report.push('```'); - // If ci fails, we can't really continue safely + // 1. Install dependencies + if (!options.noInstall) { + log('Installing dependencies (npm ci)...'); + // On Windows, npm is a cmd. Using shell: true might be needed for 'npm' command, + // or use 'npm.cmd'. Linux is fine. + // Safe to use shell: true for static commands. + const ciResult = spawnSync('npm', ['ci'], { encoding: 'utf-8', shell: true }); + if (ciResult.status !== 0) { + report.push(`## 🚨 Dependency Installation Failed`); + report.push('`npm ci` failed. Please check `package-lock.json`.'); + report.push('```'); + report.push(ciResult.stderr ? ciResult.stderr.slice(-1000) : 'Unknown error'); + report.push('```'); + failed = true; + } + } else { + log('Skipping dependency installation (--no-install).'); } // 2. Format - log('Running formatter...'); - runCommand('npm', ['run', 'format']); - const gitStatus = execSync('git status --porcelain', { encoding: 'utf-8' }); - const formattedFiles = gitStatus.split('\n') - .filter(line => line.match(/^ M/)) // Modified files - .map(line => line.substring(3)); + if (!failed) { + log('Running formatter...'); + spawnSync('npm', ['run', 'format'], { shell: true }); + const gitStatus = spawnSync('git', ['status', '--porcelain'], { encoding: 'utf-8' }); + const formattedFiles = gitStatus.stdout.split('\n') + .filter(line => line.match(/^ M/)) // Modified files + .map(line => line.substring(3).trim()); - if (formattedFiles.length > 0) { - report.push(`## 🎨 Formatting Suggestions`); - report.push(`The following files need formatting. Run \`npm run format\` to fix.`); - report.push(''); - report.push(''); + if (formattedFiles.length > 0) { + report.push(`## 🎨 Formatting Suggestions`); + report.push(`The following files need formatting. Run \`npm run format\` to fix.`); + report.push(''); + report.push(''); - if (process.argv.includes('--apply-fixes')) { - try { - log('Applying formatting fixes...'); - execSync('git add .'); - execSync('git commit -m "fix(ai): formatting (agent)"'); - report.push('**Action:** Applied formatting fixes.'); - // Clear formattedFiles list since they are now committed (and presumably fixed in the working tree) - // But wait, if we commit them, the next steps run on clean working tree but with new commit. - // That is fine. - } catch (e) { - log(`Error applying fixes: ${e.message}`); - report.push(`**Error:** Failed to apply formatting fixes: ${e.message}`); + // Fast-path suggestion + report.push('**Suggestion:** Apply `npm run format` and re-run tests.'); + + if (options.applyFixes) { + try { + log('Applying formatting fixes...'); + runCommand('git', ['add', '.']); + runCommand('git', ['commit', '-m', 'fix(ai): formatting (agent)']); + + log('Pushing changes...'); + const pushResult = runCommand('git', ['push']); + + if (pushResult.status === 0) { + report.push('**Action:** Applied and pushed formatting fixes (agent).'); + } else { + log(`Push failed: ${pushResult.stderr}`); + report.push('**Action:** Applied formatting fixes locally, but failed to push.'); + report.push(`**Error:** ${pushResult.stderr}`); + + // Attempt fallback: Create new branch? + // If we are in a detached HEAD or locked branch, pushing might fail. + // "open a follow-up PR if you could not directly update the PR branch" + // If we have GH CLI... + if (checkGhAvailability()) { + const newBranch = `fix/ai-formatting-${Date.now()}`; + log(`Attempting fallback: creating new branch ${newBranch}...`); + runCommand('git', ['checkout', '-b', newBranch]); + const pushNew = runCommand('git', ['push', '-u', 'origin', newBranch]); + if (pushNew.status === 0) { + // Create PR + log('Creating fallback PR...'); + // We need to target the original branch? Or main? + // Let's target the current context if possible. + // But we don't know the original target easily without GH CLI. + // We'll just create a PR against default. + const prRes = spawnSync('gh', ['pr', 'create', '--title', 'fix(ai): formatting (agent)', '--body', 'Automated formatting fixes.', '--fill'], { encoding: 'utf-8' }); + if (prRes.status === 0) { + report.push(`**Action:** Created follow-up PR: ${prRes.stdout.trim()}`); + } else { + report.push(`**Error:** Failed to create follow-up PR: ${prRes.stderr}`); + } + } else { + report.push(`**Error:** Failed to push fallback branch: ${pushNew.stderr}`); + } + + // Switch back? + // reviewContext usually assumes we stay on the branch or restore. + } + } + + } catch (e) { + log(`Error applying fixes: ${e.message}`); + report.push(`**Error:** Failed to apply formatting fixes: ${e.message}`); + } + } else { + // Revert changes so checking other things is clean? + // If we don't apply fixes, we should revert. } + } else { + report.push(`## ✅ Formatting`); + report.push(`No formatting issues found.`); } - } else { - report.push(`## ✅ Formatting`); - report.push(`No formatting issues found.`); } // 3. Lint - log('Running linter...'); - const lintResult = runCommand('npm', ['run', 'lint']); - if (lintResult.status !== 0) { - report.push(`## ⚠️ Linter Warnings/Errors`); - report.push('`npm run lint` reported issues:'); - report.push('
Show Lint Output'); - report.push(''); - report.push('```'); - report.push(lintResult.stdout); - report.push(lintResult.stderr); - report.push('```'); - report.push('
'); - } else { - report.push(`## ✅ Linter`); - report.push(`All lint checks passed.`); + if (!failed) { + log('Running linter...'); + const lintResult = spawnSync('npm', ['run', 'lint'], { encoding: 'utf-8', shell: true }); + if (lintResult.status !== 0) { + report.push(`## ⚠️ Linter Warnings/Errors`); + report.push('`npm run lint` reported issues:'); + report.push('
Show Lint Output'); + report.push(''); + report.push('```'); + report.push(lintResult.stdout); + report.push(lintResult.stderr); + report.push('```'); + report.push('
'); + + if (lintResult.stdout && lintResult.stdout.includes('Design token lint failed')) { + report.push('**Note:** Design token violations detected. Please check `css/tokens.css`.'); + } + } else { + report.push(`## ✅ Linter`); + report.push(`All lint checks passed.`); + } } // 4. Unit Tests - log('Running unit tests...'); - const testResult = runCommand('npm', ['run', 'test:unit']); - if (testResult.status !== 0) { - report.push(`## ❌ Test Failures`); - report.push('`npm run test:unit` failed. Inspect the stack traces below.'); - report.push('
Show Test Output'); - report.push(''); - report.push('```'); - report.push(testResult.stdout.slice(-5000)); // Last 5000 chars - report.push(testResult.stderr.slice(-5000)); - report.push('```'); - report.push('
'); - } else { - report.push(`## ✅ Tests`); - report.push(`Unit tests passed.`); + if (!failed) { + log('Running unit tests...'); + const testResult = spawnSync('npm', ['run', 'test:unit'], { encoding: 'utf-8', shell: true, timeout: 300000 }); + if (testResult.error && testResult.error.code === 'ETIMEDOUT') { + report.push(`## ❌ Test Timeout`); + report.push('`npm run test:unit` timed out after 5 minutes.'); + } else if (testResult.status !== 0) { + report.push(`## ❌ Test Failures`); + report.push('`npm run test:unit` failed. Inspect the stack traces below.'); + report.push('
Show Test Output'); + report.push(''); + report.push('```'); + report.push(testResult.stdout ? testResult.stdout.slice(-5000) : 'No stdout'); + report.push(testResult.stderr ? testResult.stderr.slice(-5000) : 'No stderr'); + report.push('```'); + report.push('
'); + } else { + report.push(`## ✅ Tests`); + report.push(`Unit tests passed.`); + } } - // 5. Sensitive Files Check + // 5. Sensitive Files const changedFiles = getChangedFiles(); const sensitiveChanges = changedFiles.filter(file => SENSITIVE_PATTERNS.some(pattern => file.match(pattern)) @@ -156,44 +217,113 @@ async function main() { report.push(''); } - // Summary Recommendation - report.push('## Summary'); - if (formattedFiles.length > 0 && lintResult.status === 0 && testResult.status === 0) { - report.push('**Suggestion:** Apply `npm run format` and re-run tests.'); - report.push('Changes are only formatting related.'); - } else if (lintResult.status !== 0 || testResult.status !== 0) { - report.push('**Status:** Changes requested. Please fix linter/test failures.'); - } else { - report.push('**Status:** Checks passed. Ready for review.'); + // Cleanup formatting if not applied + if (!options.applyFixes) { + try { + spawnSync('git', ['checkout', '.'], { stdio: 'ignore' }); + } catch (e) {} } - // Output - fs.writeFileSync(LOG_FILE, report.join('\n')); - log(`Report generated at ${LOG_FILE}`); - console.log('\n--- REPORT PREVIEW ---\n'); - console.log(report.join('\n')); - - // Cleanup changes if we don't want to keep them? - // The instructions say "offer to auto-commit". - // I won't cleanup automatically. I'll leave it to the user. - // But if I switched branches, I should switch back? - // If I switched branches, the changes are now in that branch's working tree. - // Switching back with dirty tree might fail. - - if (formattedFiles.length > 0 && !process.argv.includes('--apply-fixes')) { - log('\nRunning: git checkout . (reverting formatting changes for safety)'); - execSync('git checkout .'); - } - - if (originalBranch) { - log(`Switching back to original branch: ${originalBranch}`); - execSync(`git checkout ${originalBranch}`); - } + return report.join('\n'); } catch (error) { - console.error('An error occurred during review:', error); - if (originalBranch) execSync(`git checkout ${originalBranch}`); - process.exit(1); + log(`Error reviewing ${contextName}: ${error.message}`); + return `Error reviewing ${contextName}: ${error.message}`; + } +} + +async function main() { + const args = process.argv.slice(2); + const options = { + noInstall: args.includes('--no-install'), + applyFixes: args.includes('--apply-fixes'), + all: args.includes('--all'), + pr: args.indexOf('--pr') > -1 ? args[args.indexOf('--pr') + 1] : null, + }; + + const branchArg = args.find(a => !a.startsWith('--') && args.indexOf(a) !== args.indexOf('--pr') + 1); + + const ghAvailable = checkGhAvailability(); + + if (options.all) { + if (!ghAvailable) { + console.error('Error: `gh` CLI not found. Cannot list open PRs.'); + process.exit(1); + } + // Fetch PRs + log('Fetching open PRs...'); + try { + const prsJson = spawnSync('gh', ['pr', 'list', '--json', 'number,headRefName,url'], { encoding: 'utf-8' }); + if (prsJson.status !== 0) throw new Error(prsJson.stderr); + const prs = JSON.parse(prsJson.stdout); + log(`Found ${prs.length} open PRs.`); + + for (const pr of prs) { + log(`Checking out PR #${pr.number} (${pr.headRefName})...`); + try { + spawnSync('gh', ['pr', 'checkout', pr.number]); + const report = await reviewContext(`PR #${pr.number}`, options); + + // Post comment + const tempFile = `pr_review_${pr.number}.md`; + fs.writeFileSync(tempFile, report); + log(`Posting comment to PR #${pr.number}...`); + spawnSync('gh', ['pr', 'comment', pr.number, '-F', tempFile]); + fs.unlinkSync(tempFile); + + } catch (e) { + log(`Failed to process PR #${pr.number}: ${e.message}`); + } + } + } catch (e) { + console.error('Error listing/processing PRs:', e); + process.exit(1); + } + } else if (options.pr) { + if (!ghAvailable) { + console.error('Error: `gh` CLI not found.'); + process.exit(1); + } + log(`Checking out PR #${options.pr}...`); + try { + const res = spawnSync('gh', ['pr', 'checkout', options.pr], { encoding: 'utf-8' }); + if (res.status !== 0) throw new Error(res.stderr); + + const report = await reviewContext(`PR #${options.pr}`, options); + console.log(report); + + fs.writeFileSync(LOG_FILE, report); + log(`Report saved to ${LOG_FILE}`); + + } catch (e) { + console.error(e); + process.exit(1); + } + } else { + // Current branch or specified branch + let originalBranch; + if (branchArg) { + try { + originalBranch = spawnSync('git', ['branch', '--show-current'], { encoding: 'utf-8' }).stdout.trim(); + const res = spawnSync('git', ['checkout', branchArg]); + if (res.status !== 0) throw new Error(`Could not checkout ${branchArg}`); + } catch(e) { + console.error(e.message); + process.exit(1); + } + } + + const currentBranch = spawnSync('git', ['branch', '--show-current'], { encoding: 'utf-8' }).stdout.trim(); + const report = await reviewContext(currentBranch, options); + + console.log('\n--- REPORT ---\n'); + console.log(report); + fs.writeFileSync(LOG_FILE, report); + log(`Report saved to ${LOG_FILE}`); + + if (originalBranch) { + spawnSync('git', ['checkout', originalBranch]); + } } }