test(deep-review): add comprehensive orchestration tests and refactor for testability

.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 <PR>`.
 
+## 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.
+

.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 <PR_NUMBER>');
-    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);
+}

.gemini/skills/deep-review/scripts/clean.ts

@@ -20,11 +20,11 @@ async function confirm(question: string): Promise<boolean> {
   });
 }
 
-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);
+}

.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 <PR_NUMBER>');
-    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);
+}

.gemini/skills/deep-review/scripts/setup.ts

@@ -32,7 +32,7 @@ async function confirm(question: string): Promise<boolean> {
   });
 }
 
-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);
+}

.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 <PR_NUMBER> <BRANCH_NAME> <POLICY_PATH>');
-    process.exit(1);
+    return 1;
   }
 
   const workDir = process.cwd(); // This is remoteWorkDir
@@ -48,49 +52,61 @@ async function main() {
   const state: Record<string, any> = {};
   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);
+}

.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();
+    });
+  });
+});