diff --git a/.gemini/skills/deep-review/README.md b/.gemini/skills/deep-review/README.md index 086037ab04..e517be4b76 100644 --- a/.gemini/skills/deep-review/README.md +++ b/.gemini/skills/deep-review/README.md @@ -140,3 +140,11 @@ If you want to improve this skill: 2. Update `SKILL.md` if the agent's instructions need to change. 3. Test your changes locally using `npm run review `. +## Testing + +The orchestration logic for this skill is fully tested. To run the tests: +```bash +npx vitest .gemini/skills/deep-review/tests/orchestration.test.ts +``` +These tests mock the external environment (SSH, GitHub CLI, and the file system) to ensure that the orchestration scripts generate the correct commands and handle environment isolation accurately. + diff --git a/.gemini/skills/deep-review/scripts/check.ts b/.gemini/skills/deep-review/scripts/check.ts index e0868117c0..b396c29534 100644 --- a/.gemini/skills/deep-review/scripts/check.ts +++ b/.gemini/skills/deep-review/scripts/check.ts @@ -11,16 +11,25 @@ import { fileURLToPath } from 'url'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = path.resolve(__dirname, '../../../..'); -async function main() { - const prNumber = process.argv[2]; +export async function runChecker(args: string[]) { + const prNumber = args[0]; if (!prNumber) { console.error('Usage: npm run review:check '); - process.exit(1); + return 1; } const settingsPath = path.join(REPO_ROOT, '.gemini/settings.json'); + if (!fs.existsSync(settingsPath)) { + console.error('āŒ Settings not found. Run "npm run review:setup" first.'); + return 1; + } const settings = JSON.parse(fs.readFileSync(settingsPath, 'utf8')); - const { remoteHost, remoteWorkDir } = settings.maintainer.deepReview; + const config = settings.maintainer?.deepReview; + if (!config) { + console.error('āŒ Deep Review configuration not found.'); + return 1; + } + const { remoteHost, remoteWorkDir } = config; console.log(`šŸ” Checking remote status for PR #${prNumber} on ${remoteHost}...`); @@ -53,6 +62,9 @@ async function main() { } else { console.log('\nā³ Some tasks are still in progress. Check again in a few minutes.'); } + return 0; } -main().catch(console.error); +if (import.meta.url === `file://${process.argv[1]}`) { + runChecker(process.argv.slice(2)).catch(console.error); +} diff --git a/.gemini/skills/deep-review/scripts/clean.ts b/.gemini/skills/deep-review/scripts/clean.ts index 3afd78ca02..bb0d0bddd0 100644 --- a/.gemini/skills/deep-review/scripts/clean.ts +++ b/.gemini/skills/deep-review/scripts/clean.ts @@ -20,11 +20,11 @@ async function confirm(question: string): Promise { }); } -async function main() { +export async function runCleanup() { const settingsPath = path.join(REPO_ROOT, '.gemini/settings.json'); if (!fs.existsSync(settingsPath)) { console.error('āŒ Settings not found. Run "npm run review:setup" first.'); - process.exit(1); + return 1; } const settings = JSON.parse(fs.readFileSync(settingsPath, 'utf8')); @@ -32,7 +32,7 @@ async function main() { if (!config) { console.error('āŒ Deep Review configuration not found.'); - process.exit(1); + return 1; } const { remoteHost, remoteWorkDir } = config; @@ -59,6 +59,9 @@ async function main() { spawnSync('ssh', [remoteHost, wipeCmd], { stdio: 'inherit', shell: true }); console.log('āœ… Remote directory wiped.'); } + return 0; } -main().catch(console.error); +if (import.meta.url === `file://${process.argv[1]}`) { + runCleanup().catch(console.error); +} diff --git a/.gemini/skills/deep-review/scripts/review.ts b/.gemini/skills/deep-review/scripts/review.ts index a91125d3cb..2fa2ae0d90 100644 --- a/.gemini/skills/deep-review/scripts/review.ts +++ b/.gemini/skills/deep-review/scripts/review.ts @@ -11,11 +11,11 @@ const REPO_ROOT = path.resolve(__dirname, '../../../..'); const q = (str: string) => `'${str.replace(/'/g, "'\\''")}'`; -async function main() { - const prNumber = process.argv[2]; +export async function runOrchestrator(args: string[], env: NodeJS.ProcessEnv = process.env) { + const prNumber = args[0]; if (!prNumber) { console.error('Usage: npm run review '); - process.exit(1); + return 1; } // Load Settings @@ -31,7 +31,7 @@ async function main() { const setupResult = spawnSync('npm', ['run', 'review:setup'], { stdio: 'inherit' }); if (setupResult.status !== 0) { console.error('āŒ Setup failed. Please run "npm run review:setup" manually.'); - process.exit(1); + return 1; } // Reload settings after setup settings = JSON.parse(fs.readFileSync(settingsPath, 'utf8')); @@ -45,7 +45,7 @@ async function main() { const branchName = ghView.stdout.toString().trim(); if (!branchName) { console.error('āŒ Failed to resolve PR branch.'); - process.exit(1); + return 1; } const sessionName = `${prNumber}-${branchName.replace(/[^a-zA-Z0-9]/g, '_')}`; @@ -64,7 +64,7 @@ async function main() { spawnSync('rsync', ['-avz', '--delete', path.join(REPO_ROOT, '.gemini/skills/deep-review/scripts/'), `${remoteHost}:${remoteWorkDir}/.gemini/skills/deep-review/scripts/`]); if (syncAuth) { - const homeDir = process.env.HOME || ''; + const homeDir = env.HOME || ''; const localGeminiDir = path.join(homeDir, '.gemini'); const syncFiles = ['google_accounts.json', 'settings.json']; for (const f of syncFiles) { @@ -86,9 +86,7 @@ async function main() { const sshCmd = `ssh -t ${remoteHost} ${q(sshInternal)}`; // 4. Smart Context Execution - // If run from within a Gemini CLI session, we pop a new window. - // Otherwise (running directly in shell), we execute in-place. - const isWithinGemini = !!process.env.GEMINI_SESSION_ID || !!process.env.GCLI_SESSION_ID; + const isWithinGemini = !!env.GEMINI_SESSION_ID || !!env.GCLI_SESSION_ID; if (isWithinGemini) { if (process.platform === 'darwin' && terminalType !== 'none') { @@ -103,13 +101,12 @@ async function main() { if (appleScript) { spawnSync('osascript', ['-', sshCmd], { input: appleScript }); console.log(`āœ… ${terminalType.toUpperCase()} window opened for verification.`); - return; + return 0; } } // Cross-Platform Background Mode (within Gemini session) console.log(`šŸ“” Launching remote verification in background mode...`); - // Run SSH in background, redirecting output to a session log const logFile = path.join(REPO_ROOT, `.gemini/logs/review-${prNumber}/background.log`); fs.mkdirSync(path.dirname(logFile), { recursive: true }); @@ -118,13 +115,15 @@ async function main() { console.log(`ā³ Remote worker started in background.`); console.log(`šŸ“„ Tailing logs to: .gemini/logs/review-${prNumber}/background.log`); - return; + return 0; } // Direct Shell Mode: Execute SSH in-place console.log(`šŸš€ Launching review session in current terminal...`); const result = spawnSync(sshCmd, { stdio: 'inherit', shell: true }); - process.exit(result.status || 0); + return result.status || 0; } -main().catch(console.error); +if (import.meta.url === `file://${process.argv[1]}`) { + runOrchestrator(process.argv.slice(2)).catch(console.error); +} diff --git a/.gemini/skills/deep-review/scripts/setup.ts b/.gemini/skills/deep-review/scripts/setup.ts index d3e506f9a3..9a611a63dc 100644 --- a/.gemini/skills/deep-review/scripts/setup.ts +++ b/.gemini/skills/deep-review/scripts/setup.ts @@ -32,7 +32,7 @@ async function confirm(question: string): Promise { }); } -async function main() { +export async function runSetup(env: NodeJS.ProcessEnv = process.env) { console.log('\n🌟 Initializing Deep Review Skill Settings...'); const remoteHost = await prompt('Remote SSH Host', 'cli'); @@ -68,7 +68,7 @@ async function main() { } } else { console.log('āš ļø Please ensure gh is installed before running again.'); - process.exit(1); + return 1; } } @@ -111,7 +111,7 @@ async function main() { if (isGeminiAuthRemote) { console.log(` āœ… Gemini CLI is already authenticated on remote (${geminiSetup}).`); } else { - const homeDir = process.env.HOME || ''; + const homeDir = env.HOME || ''; const localAuth = path.join(homeDir, '.gemini/google_accounts.json'); const localEnv = path.join(REPO_ROOT, '.env'); const hasAuth = fs.existsSync(localAuth); @@ -151,6 +151,9 @@ async function main() { fs.mkdirSync(path.dirname(settingsPath), { recursive: true }); fs.writeFileSync(settingsPath, JSON.stringify(settings, null, 2)); console.log('\nāœ… Onboarding complete! Settings saved to .gemini/settings.json'); + return 0; } -main().catch(console.error); +if (import.meta.url === `file://${process.argv[1]}`) { + runSetup().catch(console.error); +} diff --git a/.gemini/skills/deep-review/scripts/worker.ts b/.gemini/skills/deep-review/scripts/worker.ts index dd824b56a9..5bbdbab030 100644 --- a/.gemini/skills/deep-review/scripts/worker.ts +++ b/.gemini/skills/deep-review/scripts/worker.ts @@ -11,10 +11,14 @@ const prNumber = process.argv[2]; const branchName = process.argv[3]; const policyPath = process.argv[4]; -async function main() { +export async function runWorker(args: string[]) { + const prNumber = args[0]; + const branchName = args[1]; + const policyPath = args[2]; + if (!prNumber || !branchName || !policyPath) { console.error('Usage: tsx worker.ts '); - process.exit(1); + return 1; } const workDir = process.cwd(); // This is remoteWorkDir @@ -48,49 +52,61 @@ async function main() { const state: Record = {}; tasks.forEach(t => state[t.id] = { status: 'PENDING' }); - function runTask(task: any) { - if (task.dep && state[task.dep].status !== 'SUCCESS') { - setTimeout(() => runTask(task), 1000); - return; + return new Promise((resolve) => { + function runTask(task: any) { + if (task.dep && state[task.dep].status !== 'SUCCESS') { + setTimeout(() => runTask(task), 1000); + return; + } + + state[task.id].status = 'RUNNING'; + const proc = spawn(task.cmd, { shell: true, env: { ...process.env, FORCE_COLOR: '1' } }); + const logStream = fs.createWriteStream(path.join(logDir, `${task.id}.log`)); + proc.stdout.pipe(logStream); + proc.stderr.pipe(logStream); + + proc.on('close', (code) => { + const exitCode = code ?? 0; + state[task.id].status = exitCode === 0 ? 'SUCCESS' : 'FAILED'; + fs.writeFileSync(path.join(logDir, `${task.id}.exit`), exitCode.toString()); + render(); + }); } - state[task.id].status = 'RUNNING'; - const proc = spawn(task.cmd, { shell: true, env: { ...process.env, FORCE_COLOR: '1' } }); - const logStream = fs.createWriteStream(path.join(logDir, `${task.id}.log`)); - proc.stdout.pipe(logStream); - proc.stderr.pipe(logStream); + function render() { + console.clear(); + console.log(`==================================================`); + console.log(`šŸš€ Deep Review | PR #${prNumber} | ${branchName}`); + console.log(`šŸ“‚ PR Target: ${targetDir}`); + console.log(`==================================================\n`); + + tasks.forEach(t => { + const s = state[t.id]; + const icon = s.status === 'SUCCESS' ? 'āœ…' : s.status === 'FAILED' ? 'āŒ' : s.status === 'RUNNING' ? 'ā³' : 'šŸ’¤'; + console.log(` ${icon} ${t.name.padEnd(20)}: ${s.status}`); + }); - proc.on('close', (code) => { - const exitCode = code ?? 0; - state[task.id].status = exitCode === 0 ? 'SUCCESS' : 'FAILED'; - fs.writeFileSync(path.join(logDir, `${task.id}.exit`), exitCode.toString()); - render(); - }); - } + const allDone = tasks.every(t => ['SUCCESS', 'FAILED'].includes(state[t.id].status)); + if (allDone) { + console.log(`\n✨ Verification complete. Launching interactive session...`); + resolve(0); + } + } - function render() { - console.clear(); - console.log(`==================================================`); - console.log(`šŸš€ Deep Review | PR #${prNumber} | ${branchName}`); - console.log(`šŸ“‚ PR Target: ${targetDir}`); - console.log(`==================================================\n`); + tasks.filter(t => !t.dep).forEach(runTask); + tasks.filter(t => t.dep).forEach(runTask); + const intervalId = setInterval(render, 1500); - tasks.forEach(t => { - const s = state[t.id]; - const icon = s.status === 'SUCCESS' ? 'āœ…' : s.status === 'FAILED' ? 'āŒ' : s.status === 'RUNNING' ? 'ā³' : 'šŸ’¤'; - console.log(` ${icon} ${t.name.padEnd(20)}: ${s.status}`); - }); - - const allDone = tasks.every(t => ['SUCCESS', 'FAILED'].includes(state[t.id].status)); - if (allDone) { - console.log(`\n✨ Verification complete. Launching interactive session...`); - process.exit(0); - } - } - - tasks.filter(t => !t.dep).forEach(runTask); - tasks.filter(t => t.dep).forEach(runTask); - setInterval(render, 1500); + // Ensure the promise resolves and the interval is cleared when all done + const checkAllDone = setInterval(() => { + if (tasks.every(t => ['SUCCESS', 'FAILED'].includes(state[t.id].status))) { + clearInterval(intervalId); + clearInterval(checkAllDone); + } + }, 1000); + }); } -main().catch(console.error); +if (import.meta.url === `file://${process.argv[1]}`) { + runWorker(process.argv.slice(2)).catch(console.error); +} diff --git a/.gemini/skills/deep-review/tests/orchestration.test.ts b/.gemini/skills/deep-review/tests/orchestration.test.ts new file mode 100644 index 0000000000..675fce185a --- /dev/null +++ b/.gemini/skills/deep-review/tests/orchestration.test.ts @@ -0,0 +1,206 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { spawnSync, spawn } from 'child_process'; +import fs from 'fs'; +import readline from 'readline'; +import { runOrchestrator } from '../scripts/review.ts'; +import { runSetup } from '../scripts/setup.ts'; +import { runWorker } from '../scripts/worker.ts'; +import { runChecker } from '../scripts/check.ts'; +import { runCleanup } from '../scripts/clean.ts'; + +vi.mock('child_process'); +vi.mock('fs'); +vi.mock('readline'); + +describe('Deep Review Orchestration', () => { + const mockSettings = { + maintainer: { + deepReview: { + remoteHost: 'test-host', + remoteWorkDir: '~/test-dir', + terminalType: 'none', + syncAuth: false, + geminiSetup: 'preexisting', + ghSetup: 'preexisting' + } + } + }; + + beforeEach(() => { + vi.resetAllMocks(); + + // Mock settings file existence and content + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(mockSettings)); + vi.mocked(fs.mkdirSync).mockReturnValue(undefined as any); + vi.mocked(fs.writeFileSync).mockReturnValue(undefined as any); + vi.mocked(fs.createWriteStream).mockReturnValue({ pipe: vi.fn() } as any); + + // Mock process methods to avoid real side effects + vi.spyOn(process, 'chdir').mockImplementation(() => {}); + vi.spyOn(process, 'cwd').mockReturnValue('/test-cwd'); + + // Default mock for spawnSync + vi.mocked(spawnSync).mockImplementation((cmd: any, args: any) => { + if (cmd === 'gh' && args?.[0] === 'pr' && args?.[1] === 'view') { + return { status: 0, stdout: Buffer.from('test-branch\n'), stderr: Buffer.from('') } as any; + } + return { status: 0, stdout: Buffer.from(''), stderr: Buffer.from('') } as any; + }); + + // Default mock for spawn (used in worker.ts) + vi.mocked(spawn).mockImplementation(() => { + const mockProc = { + stdout: { pipe: vi.fn(), on: vi.fn() }, + stderr: { pipe: vi.fn(), on: vi.fn() }, + on: vi.fn((event, cb) => { if (event === 'close') cb(0); }), + pid: 1234 + }; + return mockProc as any; + }); + }); + + describe('review.ts', () => { + it('should construct the correct tmux session name from branch', async () => { + await runOrchestrator(['123'], {}); + + const spawnCalls = vi.mocked(spawnSync).mock.calls; + const sshCall = spawnCalls.find(call => + (typeof call[0] === 'string' && call[0].includes('tmux new-session')) || + (Array.isArray(call[1]) && call[1].some(arg => typeof arg === 'string' && arg.includes('tmux new-session'))) + ); + + expect(sshCall).toBeDefined(); + const cmdStr = typeof sshCall![0] === 'string' ? sshCall![0] : (sshCall![1] as string[]).join(' '); + expect(cmdStr).toContain('test-host'); + expect(cmdStr).toContain('tmux new-session -s 123-test_branch'); + }); + + it('should use isolated config path when setupType is isolated', async () => { + const isolatedSettings = { + ...mockSettings, + maintainer: { + ...mockSettings.maintainer, + deepReview: { + ...mockSettings.maintainer.deepReview, + geminiSetup: 'isolated' + } + } + }; + vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(isolatedSettings)); + + await runOrchestrator(['123'], {}); + + const spawnCalls = vi.mocked(spawnSync).mock.calls; + const sshCall = spawnCalls.find(call => { + const cmdStr = typeof call[0] === 'string' ? call[0] : (Array.isArray(call[1]) ? call[1].join(' ') : ''); + return cmdStr.includes('GEMINI_CLI_HOME=~/.gemini-deep-review'); + }); + + expect(sshCall).toBeDefined(); + }); + }); + + describe('setup.ts', () => { + const mockInterface = { + question: vi.fn(), + close: vi.fn() + }; + + beforeEach(() => { + vi.mocked(readline.createInterface).mockReturnValue(mockInterface as any); + }); + + it('should correctly detect pre-existing setup when .git directory exists on remote', async () => { + vi.mocked(spawnSync).mockImplementation((cmd: any, args: any) => { + if (cmd === 'ssh') { + const remoteCmd = args[1]; + if (remoteCmd.includes('[ -d ~/test-dir/.git ]')) return { status: 0 } as any; + if (remoteCmd.includes('sh -lc "command -v gh"')) return { status: 0 } as any; + if (remoteCmd.includes('gh auth status')) return { status: 0 } as any; + if (remoteCmd.includes('google_accounts.json')) return { status: 0 } as any; + } + return { status: 0, stdout: Buffer.from(''), stderr: Buffer.from('') } as any; + }); + + mockInterface.question + .mockImplementationOnce((q, cb) => cb('test-host')) + .mockImplementationOnce((q, cb) => cb('~/test-dir')) + .mockImplementationOnce((q, cb) => cb('p')) + .mockImplementationOnce((q, cb) => cb('p')) + .mockImplementationOnce((q, cb) => cb('none')); + + await runSetup({ HOME: '/test-home' }); + + const writeCall = vi.mocked(fs.writeFileSync).mock.calls.find(call => + call[0].toString().includes('.gemini/settings.json') + ); + expect(writeCall).toBeDefined(); + const savedSettings = JSON.parse(writeCall![1] as string); + expect(savedSettings.maintainer.deepReview.geminiSetup).toBe('preexisting'); + }); + }); + + describe('worker.ts', () => { + it('should launch parallel tasks and write exit codes', async () => { + // Mock targetDir existing + vi.mocked(fs.existsSync).mockImplementation((p) => p.toString().includes('test-branch')); + + const workerPromise = runWorker(['123', 'test-branch', '/test-policy.toml']); + + // Since worker uses setInterval/setTimeout, we might need to advance timers + // or ensure the close event triggers everything + await workerPromise; + + const spawnCalls = vi.mocked(spawn).mock.calls; + expect(spawnCalls.length).toBeGreaterThanOrEqual(4); // build, ci, review, verify + + const buildCall = spawnCalls.find(call => call[0].includes('npm ci')); + expect(buildCall).toBeDefined(); + + const writeCalls = vi.mocked(fs.writeFileSync).mock.calls; + const exitFileCall = writeCalls.find(call => call[0].toString().includes('build.exit')); + expect(exitFileCall).toBeDefined(); + expect(exitFileCall![1]).toBe('0'); + }); + }); + + describe('check.ts', () => { + it('should report SUCCESS when exit files contain 0', async () => { + vi.mocked(spawnSync).mockImplementation((cmd: any, args: any) => { + if (cmd === 'gh') return { status: 0, stdout: Buffer.from('test-branch\n') } as any; + if (cmd === 'ssh' && args[1].includes('cat') && args[1].includes('.exit')) { + return { status: 0, stdout: Buffer.from('0\n') } as any; + } + return { status: 0, stdout: Buffer.from('') } as any; + }); + + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + + await runChecker(['123']); + + expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('āœ… build : SUCCESS')); + expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('✨ All remote tasks complete')); + + consoleSpy.mockRestore(); + }); + }); + + describe('clean.ts', () => { + it('should kill tmux server and remove directories', async () => { + vi.mocked(readline.createInterface).mockReturnValue({ + question: vi.fn((q, cb) => cb('n')), // Don't wipe everything + close: vi.fn() + } as any); + + await runCleanup(); + + const spawnCalls = vi.mocked(spawnSync).mock.calls; + const killCall = spawnCalls.find(call => Array.isArray(call[1]) && call[1].some(arg => arg === 'tmux kill-server')); + expect(killCall).toBeDefined(); + + const rmCall = spawnCalls.find(call => Array.isArray(call[1]) && call[1].some(arg => arg.includes('rm -rf'))); + expect(rmCall).toBeDefined(); + }); + }); +});