From 4de7dcb3f4efb9d177514ba498c1f24aa1da3b34 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 27 Jan 2026 23:16:02 +0000 Subject: [PATCH] feat(ci): enhance automated PR review scripts - Update `scripts/automated-pr-review.mjs` to include missing sensitive paths (`js/auth/`, `js/magnetUtils.js`, `js/nostr/adapters/nip07Adapter.js`) and improve commit messaging for automated fixes. - Add release channel guidance checks for `unstable` branch in `automated-pr-review.mjs`. - Create `scripts/agent/review-open-prs.mjs` to robustly batch process open PRs using the updated review script. - Remove redundant scripts `scripts/agent/automated-pr-review.mjs` and `scripts/agent/review-pr.mjs`. Co-authored-by: PR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> --- scripts/agent/automated-pr-review.mjs | 127 --------- scripts/agent/review-open-prs.mjs | 6 +- scripts/agent/review-pr.mjs | 363 -------------------------- scripts/automated-pr-review.mjs | 10 + 4 files changed, 12 insertions(+), 494 deletions(-) delete mode 100644 scripts/agent/automated-pr-review.mjs delete mode 100644 scripts/agent/review-pr.mjs diff --git a/scripts/agent/automated-pr-review.mjs b/scripts/agent/automated-pr-review.mjs deleted file mode 100644 index 19ed4dfd..00000000 --- a/scripts/agent/automated-pr-review.mjs +++ /dev/null @@ -1,127 +0,0 @@ -import { spawnSync, execSync } from 'child_process'; -import fs from 'fs'; -import path from 'path'; - -const REPORT_FILE = 'PR_REVIEW.md'; -const APPLY_FIXES = process.argv.includes('--apply-fixes'); - -function run(command, args, opts = {}) { - console.log(`> ${command} ${args.join(' ')}`); - return spawnSync(command, args, { encoding: 'utf-8', stdio: 'pipe', ...opts }); -} - -function getDiffFiles() { - try { - // Try to compare against origin/main, fallback to HEAD~1 or just check current changes if no upstream - // Since this runs on a checkout, we want to see what this branch touches compared to main. - // If origin/main is not available, we might fail. - const hasMain = run('git', ['rev-parse', '--verify', 'origin/main']).status === 0; - const base = hasMain ? 'origin/main' : 'HEAD'; - - // Check if we are on main? No, we are on a PR branch usually. - // We want the diff of the *current* code vs main. - const res = run('git', ['diff', '--name-only', base, 'HEAD']); - if (res.error || res.status !== 0) return []; - return res.stdout.trim().split('\n').filter(Boolean); - } catch (e) { - console.error("Error getting diff:", e); - return []; - } -} - -function main() { - const report = []; - report.push('# Automated PR Review'); - report.push(`Date: ${new Date().toISOString()}`); - report.push(''); - - // 1. npm ci - console.log('Running npm ci...'); - const ci = run('npm', ['ci']); - if (ci.status !== 0) { - report.push('## 🚨 Dependency Install Failed'); - report.push('```'); - report.push(ci.stderr || ci.stdout); - report.push('```'); - // If ci fails, we probably can't run other scripts, but we try. - } else { - report.push('## ✅ Dependencies Installed'); - } - - // 2. Format - console.log('Running npm run format...'); - run('npm', ['run', 'format']); // This modifies files - - const status = run('git', ['status', '--porcelain']).stdout; - const formattedFiles = status.split('\n').filter(l => l.trim()).map(l => l.slice(3)); - - if (formattedFiles.length > 0) { - report.push('## ⚠ī¸ Formatting Changes'); - report.push('The following files were reformatted:'); - report.push(formattedFiles.map(f => `- ${f}`).join('\n')); - - if (APPLY_FIXES) { - console.log('Applying formatting fixes...'); - run('git', ['add', '.']); - run('git', ['commit', '-m', 'fix(ai): automated formatting (agent)']); - report.push('\n**Changes have been automatically committed.**'); - } else { - report.push('\n**Run `npm run format` to fix these issues.**'); - } - } else { - report.push('## ✅ Formatting Passed'); - } - - // 3. Lint - console.log('Running npm run lint...'); - const lint = run('npm', ['run', 'lint']); - if (lint.status !== 0) { - report.push('## ⚠ī¸ Lint Warnings/Errors'); - report.push('
Show Lint Output'); - report.push(''); - report.push('```'); - report.push(lint.stdout + '\n' + lint.stderr); - report.push('```'); - report.push('
'); - } else { - report.push('## ✅ Lint Passed'); - } - - // 4. Unit Tests - console.log('Running npm run test:unit...'); - const test = run('npm', ['run', 'test:unit']); - if (test.status !== 0) { - report.push('## ❌ Unit Tests Failed'); - report.push('
Show Test Output'); - report.push(''); - report.push('```'); - report.push(test.stdout + '\n' + test.stderr); - report.push('```'); - report.push('
'); - } else { - report.push('## ✅ Unit Tests Passed'); - } - - // 5. Guardrails - const diffFiles = getDiffFiles(); - const sensitivePatterns = [ - /^js\/nostr\//, - /^js\/services\/storageService/, - /^js\/services\/r2Service/, - /crypto/i - ]; - - const sensitiveChanges = diffFiles.filter(f => sensitivePatterns.some(p => p.test(f))); - - if (sensitiveChanges.length > 0) { - report.push('## 🛡ī¸ Security & Protocol Review Required'); - report.push('The following sensitive files were modified:'); - report.push(sensitiveChanges.map(f => `- ${f}`).join('\n')); - report.push('\n**Flags:** `requires-security-review`, `requires-protocol-review`'); - } - - fs.writeFileSync(REPORT_FILE, report.join('\n')); - console.log(`Report generated at ${REPORT_FILE}`); -} - -main(); diff --git a/scripts/agent/review-open-prs.mjs b/scripts/agent/review-open-prs.mjs index d5c79858..09b36e5f 100644 --- a/scripts/agent/review-open-prs.mjs +++ b/scripts/agent/review-open-prs.mjs @@ -21,8 +21,6 @@ function getOpenPRs() { return []; } if (res.status !== 0) { - // Only log stderr if it's not just "no pull requests found" or similar non-critical output - // But gh usually exits 0 even if empty. console.error('Error fetching PRs:', res.stderr); return []; } @@ -62,8 +60,8 @@ async function main() { ...process.env, PR_NUMBER: pr.number.toString(), GITHUB_BASE_REF: pr.baseRefName, - // Assuming origin is the base repo for simplicity in this agent script - GITHUB_BASE_REPO_URL: '' // automated-pr-review handles missing URL by using origin + // Assuming origin is the base repo or gh pr checkout handled the remote setup. + // automated-pr-review.mjs handles missing GITHUB_BASE_REPO_URL by defaulting to upstream/origin. }; // Run Review Script diff --git a/scripts/agent/review-pr.mjs b/scripts/agent/review-pr.mjs deleted file mode 100644 index bdfe1a56..00000000 --- a/scripts/agent/review-pr.mjs +++ /dev/null @@ -1,363 +0,0 @@ -import { spawnSync } from 'child_process'; -import fs from 'fs'; -import path from 'path'; - -const LOG_FILE = 'pr_review_report.md'; -const SENSITIVE_PATTERNS = [ - /^js\/nostr\//, - /^js\/storage\//, - /crypto/i, - /nip44/i -]; - -function log(message) { - console.log(`[ReviewAgent] ${message}`); -} - -function runCommand(command, args = [], options = {}) { - log(`> Running: ${command} ${args.join(' ')}`); - return spawnSync(command, args, { - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'pipe'], - shell: false, - maxBuffer: 10 * 1024 * 1024, - ...options - }); -} - -function checkGhAvailability() { - try { - const res = runCommand('gh', ['--version']); - return res.status === 0; - } catch (e) { - return false; - } -} - -function getChangedFiles(baseBranch = 'origin/main') { - try { - const res = runCommand('git', ['diff', '--name-only', `${baseBranch}...HEAD`]); - if (res.status !== 0) { - // Fallback: use local changes - const local = runCommand('git', ['diff', '--name-only', 'HEAD']); - return local.stdout ? local.stdout.trim().split('\n').filter(Boolean) : []; - } - return res.stdout.trim().split('\n').filter(Boolean); - } catch (error) { - log(`Warning: Could not determine changed files. Using local changes if any.`); - return []; - } -} - -async function reviewContext(contextName, options) { - const report = []; - report.push(`# Automated PR Review & Suggested Improvements`); - report.push(`**Context:** ${contextName}`); - report.push(`**Date:** ${new Date().toISOString()}`); - report.push(''); - - // Release Channel Guidance - if (options.baseBranch) { - report.push(`## ℹī¸ Release Channel Guidance`); - if (options.baseBranch === 'main') { - report.push('**Target: Main (Production)**'); - report.push('- Commits must be atomic.'); - report.push('- No destructive migrations.'); - report.push('- Feature flags must default to `false`.'); - report.push('- Ensure UX and magnet safety guarantees are preserved.'); - } else if (options.baseBranch === 'unstable') { - report.push('**Target: Unstable (Experimentation)**'); - report.push('- Gate risky behavior behind feature flags (`js/constants.js`).'); - report.push('- Document toggle/rollback plans in PR description.'); - } else { - report.push(`Target: ${options.baseBranch}`); - report.push('Please refer to `AGENTS.md` for release channel guidance.'); - } - report.push(''); - } - - let failed = false; - - try { - // 1. Install dependencies - if (!options.noInstall) { - log('Installing dependencies (npm ci)...'); - const ciResult = runCommand('npm', ['ci'], { 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 - if (!failed) { - log('Running formatter...'); - runCommand('npm', ['run', 'format'], { shell: true }); - const gitStatus = runCommand('git', ['status', '--porcelain']); - 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:`); - report.push(''); - report.push(''); - - // 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', '.']); - // Explicit wording: fix(ai): (agent) - 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}`); - - 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) { - log('Creating fallback PR...'); - 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}`); - } - } - } - } - } catch (e) { - log(`Error applying fixes: ${e.message}`); - report.push(`**Error:** Failed to apply formatting fixes: ${e.message}`); - } - } else { - // If we're not applying fixes, we should revert the formatting changes so they don't affect subsequent steps? - // However, keeping them might be good for running tests on clean code? - // But if tests rely on formatting (unlikely), it matters. - // Usually we revert so the working tree matches the PR state for further analysis. - runCommand('git', ['checkout', '.'], { stdio: 'ignore' }); - } - } else { - report.push(`## ✅ Formatting`); - report.push(`No formatting issues found.`); - } - } - - // 3. Lint - if (!failed) { - log('Running linter...'); - const lintResult = runCommand('npm', ['run', 'lint'], { shell: true }); - if (lintResult.status !== 0) { - report.push(`## ⚠ī¸ Linter Warnings/Errors`); - report.push('`npm run lint` reported issues:'); - - // Extract lines with "Error" or "Warning" - const lines = (lintResult.stdout + '\n' + lintResult.stderr).split('\n'); - const issues = lines.filter(l => l.match(/error|warning/i)).slice(0, 20); // Limit to 20 - - report.push('
Show Lint Output'); - report.push(''); - report.push('```'); - if (issues.length > 0) { - report.push(issues.join('\n')); - if (lines.length > issues.length) report.push('...'); - } else { - report.push(lintResult.stdout.slice(-1000)); - report.push(lintResult.stderr.slice(-1000)); - } - 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 - if (!failed) { - log('Running unit tests...'); - const testResult = runCommand('npm', ['run', 'test:unit'], { 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.'); - - // Analyze stdout for failing test file - // Expecting "→ Running tests/..." lines. - const output = testResult.stdout || ''; - const runningLines = output.match(/→ Running (.*)/g); - let lastTest = 'Unknown'; - if (runningLines && runningLines.length > 0) { - lastTest = runningLines[runningLines.length - 1].replace('→ Running ', '').trim(); - } - - report.push(`**Possible culprit:** \`${lastTest}\``); - report.push('Suggested area to inspect: check the stack trace below for the failing assertion.'); - - report.push('
Show Test Output'); - report.push(''); - report.push('```'); - // Capture context around the failure? - // Usually the end of stdout has the failure. - report.push(output.slice(-3000)); - report.push((testResult.stderr || '').slice(-3000)); - report.push('```'); - report.push('
'); - } else { - report.push(`## ✅ Tests`); - report.push(`Unit tests passed.`); - } - } - - // 5. Sensitive Files - const changedFiles = getChangedFiles(); - const sensitiveChanges = changedFiles.filter(file => - SENSITIVE_PATTERNS.some(pattern => file.match(pattern)) - ); - - if (sensitiveChanges.length > 0) { - report.push(`## 🔒 Security Review Required`); - report.push(`This PR modifies sensitive files.`); - report.push(`**Flags:** \`requires-security-review\`, \`requires-protocol-review\``); - report.push(''); - } - - return report.join('\n'); - - } catch (error) { - 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); - } - log('Fetching open PRs...'); - try { - const prsJson = runCommand('gh', ['pr', 'list', '--json', 'number,headRefName,baseRefName,url']); - 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 { - runCommand('gh', ['pr', 'checkout', pr.number]); - // Pass baseBranch from PR details - options.baseBranch = pr.baseRefName; - const report = await reviewContext(`PR #${pr.number}`, options); - const tempFile = `pr_review_${pr.number}.md`; - fs.writeFileSync(tempFile, report); - log(`Posting comment to PR #${pr.number}...`); - runCommand('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 { - // Fetch base branch first - const viewRes = runCommand('gh', ['pr', 'view', options.pr, '--json', 'baseRefName']); - if (viewRes.status === 0) { - const prDetails = JSON.parse(viewRes.stdout); - options.baseBranch = prDetails.baseRefName; - } - - const res = runCommand('gh', ['pr', 'checkout', options.pr]); - 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 { - let originalBranch; - if (branchArg) { - try { - originalBranch = runCommand('git', ['branch', '--show-current']).stdout.trim(); - const res = runCommand('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 = runCommand('git', ['branch', '--show-current']).stdout.trim(); - // Logic for local run: can we guess the target? - // Usually local run is 'against' something. - // But we can just skip or default. - - 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) { - runCommand('git', ['checkout', originalBranch]); - } - } -} - -main(); diff --git a/scripts/automated-pr-review.mjs b/scripts/automated-pr-review.mjs index 7e11be0c..c672d4ab 100644 --- a/scripts/automated-pr-review.mjs +++ b/scripts/automated-pr-review.mjs @@ -6,7 +6,10 @@ import path from 'node:path'; const SENSITIVE_PATHS = [ /^js\/nostr\//, /^js\/storage\//, + /^js\/auth\//, /^js\/dmDecryptor\.js$/, + /^js\/magnetUtils\.js$/, + /^js\/nostr\/adapters\/nip07Adapter\.js$/, /^js\/services\/storageService\.js$/, /^js\/services\/attachmentService\.js$/, /^js\/services\/hashtagPreferencesService\.js$/, @@ -86,6 +89,13 @@ function checkReleaseChannel(changedFiles) { warnings.push(`⚠ī¸ **Release Channel Warning**: This PR targets \`main\` but modifies the following critical configuration files: ${modifiedCriticalFiles.map(f => `\`${f}\``).join(', ')}.`); warnings.push(`> **Guidance:** \`main\` is the production track. Ensure feature flags in \`js/constants.js\` are set to safe defaults (usually \`false\`) and \`config/instance-config.js\` has production-appropriate settings (e.g. \`IS_DEV_MODE\`).`); } + } else if (baseRef === 'unstable') { + const criticalFiles = ['js/constants.js']; + const modifiedCriticalFiles = changedFiles.filter(f => criticalFiles.includes(f)); + if (modifiedCriticalFiles.length > 0) { + warnings.push(`ℹī¸ **Release Channel Info**: This PR targets \`unstable\`.`); + warnings.push(`> **Guidance:** \`unstable\` is the experimentation lane. If you are adding risky behavior, gate it behind feature flags in \`js/constants.js\` and document the toggle/rollback plan. See \`AGENTS.md\` for details.`); + } } return warnings; }