diff --git a/runners/agent-runner/src/reviewer-runtime.ts b/runners/agent-runner/src/reviewer-runtime.ts index 48c9fe3..f0899f7 100644 --- a/runners/agent-runner/src/reviewer-runtime.ts +++ b/runners/agent-runner/src/reviewer-runtime.ts @@ -37,107 +37,6 @@ export function isReviewerRuntime( return roomRoleContext?.role === 'reviewer'; } -function findGitSubcommand(args: string[]): string { - let skipNext = false; - for (const arg of args) { - if (skipNext) { - skipNext = false; - continue; - } - switch (arg) { - case '-c': - case '-C': - case '--git-dir': - case '--work-tree': - case '--namespace': - case '--exec-path': - case '--config-env': - skipNext = true; - continue; - default: - break; - } - if ( - arg.startsWith('-c') || - arg.startsWith('-C') || - arg.startsWith('--git-dir=') || - arg.startsWith('--work-tree=') || - arg.startsWith('--namespace=') || - arg.startsWith('--exec-path=') || - arg.startsWith('--config-env=') - ) { - continue; - } - if (arg.startsWith('-')) { - continue; - } - return arg; - } - return ''; -} - -function tokenizeShellWords(segment: string): string[] { - const tokens: string[] = []; - let current = ''; - let quote: '"' | "'" | null = null; - let escaped = false; - - for (const char of segment) { - if (escaped) { - current += char; - escaped = false; - continue; - } - if (char === '\\' && quote !== "'") { - escaped = true; - continue; - } - if (quote) { - if (char === quote) { - quote = null; - } else { - current += char; - } - continue; - } - if (char === '"' || char === "'") { - quote = char; - continue; - } - if (/\s/.test(char)) { - if (current) { - tokens.push(current); - current = ''; - } - continue; - } - current += char; - } - - if (current) { - tokens.push(current); - } - return tokens; -} - -function isBlockedGitCommand(command: string): boolean { - const segments = command - .split(/&&|\|\||[;\n]/) - .map((segment) => segment.trim()) - .filter(Boolean); - for (const segment of segments) { - const tokens = tokenizeShellWords(segment); - if (tokens[0] !== 'git') { - continue; - } - const subcommand = findGitSubcommand(tokens.slice(1)); - if (BLOCKED_GIT_SUBCOMMANDS.has(subcommand)) { - return true; - } - } - return false; -} - function resolveGitBinary(baseEnv: NodeJS.ProcessEnv): string { return execFileSync('bash', ['-lc', 'command -v git'], { encoding: 'utf-8', @@ -155,6 +54,7 @@ export function buildReviewerGitGuardEnv( } const realGitPath = resolveGitBinary(baseEnv); + const protectedWorkDir = baseEnv.EJCLAW_WORK_DIR || ''; const wrapperDir = fs.mkdtempSync( path.join(os.tmpdir(), 'ejclaw-reviewer-git-'), ); @@ -166,15 +66,46 @@ export function buildReviewerGitGuardEnv( const script = `#!/usr/bin/env bash set -euo pipefail real_git=${JSON.stringify(realGitPath)} +protected_work_dir=${JSON.stringify(protectedWorkDir)} blocked_subcommands=(${blocked}) subcmd="" skip_next=0 +target_dir="$(pwd -P)" +capture_next_dir=0 for arg in "$@"; do + if [[ "$capture_next_dir" == "1" ]]; then + if [[ "$arg" == /* ]]; then + target_dir="$arg" + else + target_dir="$target_dir/$arg" + fi + target_dir="$(cd "$target_dir" 2>/dev/null && pwd -P || printf '%s' "$target_dir")" + capture_next_dir=0 + continue + fi if [[ "$skip_next" == "1" ]]; then skip_next=0 continue fi case "$arg" in + -C) + capture_next_dir=1 + continue + ;; + -C*) + target_dir="\${arg#-C}" + target_dir="$(cd "$target_dir" 2>/dev/null && pwd -P || printf '%s' "$target_dir")" + continue + ;; + --work-tree) + capture_next_dir=1 + continue + ;; + --work-tree=*) + target_dir="\${arg#--work-tree=}" + target_dir="$(cd "$target_dir" 2>/dev/null && pwd -P || printf '%s' "$target_dir")" + continue + ;; -c|-C|--git-dir|--work-tree|--namespace|--exec-path|--config-env) skip_next=1 continue @@ -194,8 +125,17 @@ for arg in "$@"; do ;; esac done +is_protected_target=0 +if [[ -n "$protected_work_dir" ]]; then + protected_real="$(cd "$protected_work_dir" 2>/dev/null && pwd -P || printf '%s' "$protected_work_dir")" + case "$target_dir" in + "$protected_real"|"$protected_real"/*) + is_protected_target=1 + ;; + esac +fi for blocked in "\${blocked_subcommands[@]}"; do - if [[ "$subcmd" == "$blocked" ]]; then + if [[ "$is_protected_target" == "1" && "$subcmd" == "$blocked" ]]; then echo "EJClaw reviewer runtime blocks mutating git subcommands: $subcmd" >&2 exit 1 fi @@ -207,6 +147,7 @@ exec "$real_git" "$@" return { ...baseEnv, EJCLAW_REAL_GIT: realGitPath, + EJCLAW_PROTECTED_WORK_DIR: protectedWorkDir, PATH: `${wrapperDir}:${baseEnv.PATH || ''}`, }; } @@ -214,7 +155,6 @@ exec "$real_git" "$@" export function isReviewerMutatingShellCommand(command: string): boolean { const normalized = command.trim(); return ( - isBlockedGitCommand(normalized) || MUTATING_SHELL_PATTERNS.some((pattern) => pattern.test(normalized)) ); } diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index beef64c..49181c9 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -11,6 +11,26 @@ import { isReviewerRuntime, } from '../src/reviewer-runtime.js'; +function createTempRepo(prefix: string): string { + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + execFileSync('git', ['init'], { + cwd, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + execFileSync('git', ['config', 'user.email', 'test@example.com'], { + cwd, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + execFileSync('git', ['config', 'user.name', 'EJClaw Test'], { + cwd, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + return cwd; +} + describe('claude reviewer runtime guard', () => { it('detects reviewer room metadata', () => { expect( @@ -25,10 +45,10 @@ describe('claude reviewer runtime guard', () => { }); it('flags mutating shell commands', () => { - expect(isReviewerMutatingShellCommand('git commit -m "x"')).toBe(true); - expect( - isReviewerMutatingShellCommand('git -c color.ui=false commit -m "x"'), - ).toBe(true); + expect(isReviewerMutatingShellCommand('git commit -m "x"')).toBe(false); + expect(isReviewerMutatingShellCommand('git -c color.ui=false commit -m "x"')).toBe( + false, + ); expect(isReviewerMutatingShellCommand('sed -i s/a/b/ file.ts')).toBe( true, ); @@ -42,9 +62,39 @@ describe('claude reviewer runtime guard', () => { expect(env.EJCLAW_REAL_GIT).toBeTruthy(); }); - it('blocks mutating git subcommands even when git options come first', () => { - const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true); + it('allows mutating git commands in temp repos outside the protected workspace', () => { + const protectedDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'ejclaw-protected-workspace-'), + ); + const env = buildReviewerGitGuardEnv( + { + PATH: process.env.PATH, + EJCLAW_WORK_DIR: protectedDir, + }, + true, + ); + const cwd = createTempRepo('ejclaw-reviewer-temp-repo-'); + fs.writeFileSync(path.join(cwd, 'note.txt'), 'ok\n'); + + expect(() => + execFileSync('git', ['add', 'note.txt'], { + cwd, + env, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }), + ).not.toThrow(); + }); + + it('blocks mutating git subcommands inside the protected reviewer workspace', () => { const cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'ejclaw-reviewer-test-')); + const env = buildReviewerGitGuardEnv( + { + PATH: process.env.PATH, + EJCLAW_WORK_DIR: cwd, + }, + true, + ); try { execFileSync('git', ['-c', 'color.ui=false', 'commit', '-m', 'x'], { diff --git a/runners/codex-runner/src/reviewer-runtime.ts b/runners/codex-runner/src/reviewer-runtime.ts index 5f98e31..4947926 100644 --- a/runners/codex-runner/src/reviewer-runtime.ts +++ b/runners/codex-runner/src/reviewer-runtime.ts @@ -48,6 +48,7 @@ export function buildReviewerGitGuardEnv( } const realGitPath = resolveGitBinary(baseEnv); + const protectedWorkDir = baseEnv.EJCLAW_WORK_DIR || ''; const wrapperDir = fs.mkdtempSync( path.join(os.tmpdir(), 'ejclaw-reviewer-git-'), ); @@ -59,15 +60,46 @@ export function buildReviewerGitGuardEnv( const script = `#!/usr/bin/env bash set -euo pipefail real_git=${JSON.stringify(realGitPath)} +protected_work_dir=${JSON.stringify(protectedWorkDir)} blocked_subcommands=(${blocked}) subcmd="" skip_next=0 +target_dir="$(pwd -P)" +capture_next_dir=0 for arg in "$@"; do + if [[ "$capture_next_dir" == "1" ]]; then + if [[ "$arg" == /* ]]; then + target_dir="$arg" + else + target_dir="$target_dir/$arg" + fi + target_dir="$(cd "$target_dir" 2>/dev/null && pwd -P || printf '%s' "$target_dir")" + capture_next_dir=0 + continue + fi if [[ "$skip_next" == "1" ]]; then skip_next=0 continue fi case "$arg" in + -C) + capture_next_dir=1 + continue + ;; + -C*) + target_dir="\${arg#-C}" + target_dir="$(cd "$target_dir" 2>/dev/null && pwd -P || printf '%s' "$target_dir")" + continue + ;; + --work-tree) + capture_next_dir=1 + continue + ;; + --work-tree=*) + target_dir="\${arg#--work-tree=}" + target_dir="$(cd "$target_dir" 2>/dev/null && pwd -P || printf '%s' "$target_dir")" + continue + ;; -c|-C|--git-dir|--work-tree|--namespace|--exec-path|--config-env) skip_next=1 continue @@ -87,8 +119,17 @@ for arg in "$@"; do ;; esac done +is_protected_target=0 +if [[ -n "$protected_work_dir" ]]; then + protected_real="$(cd "$protected_work_dir" 2>/dev/null && pwd -P || printf '%s' "$protected_work_dir")" + case "$target_dir" in + "$protected_real"|"$protected_real"/*) + is_protected_target=1 + ;; + esac +fi for blocked in "\${blocked_subcommands[@]}"; do - if [[ "$subcmd" == "$blocked" ]]; then + if [[ "$is_protected_target" == "1" && "$subcmd" == "$blocked" ]]; then echo "EJClaw reviewer runtime blocks mutating git subcommands: $subcmd" >&2 exit 1 fi @@ -100,6 +141,7 @@ exec "$real_git" "$@" return { ...baseEnv, EJCLAW_REAL_GIT: realGitPath, + EJCLAW_PROTECTED_WORK_DIR: protectedWorkDir, PATH: `${wrapperDir}:${baseEnv.PATH || ''}`, }; } diff --git a/runners/codex-runner/test/reviewer-runtime.test.ts b/runners/codex-runner/test/reviewer-runtime.test.ts index 43e751c..8c4678d 100644 --- a/runners/codex-runner/test/reviewer-runtime.test.ts +++ b/runners/codex-runner/test/reviewer-runtime.test.ts @@ -10,6 +10,26 @@ import { isReviewerRuntime, } from '../src/reviewer-runtime.js'; +function createTempRepo(prefix: string): string { + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + execFileSync('git', ['init'], { + cwd, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + execFileSync('git', ['config', 'user.email', 'test@example.com'], { + cwd, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + execFileSync('git', ['config', 'user.name', 'EJClaw Test'], { + cwd, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + return cwd; +} + describe('codex reviewer runtime guard', () => { it('detects reviewer room metadata', () => { expect( @@ -29,9 +49,39 @@ describe('codex reviewer runtime guard', () => { expect(env.EJCLAW_REAL_GIT).toBeTruthy(); }); - it('blocks mutating git subcommands even when git options come first', () => { - const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true); + it('allows mutating git commands in temp repos outside the protected workspace', () => { + const protectedDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'ejclaw-protected-workspace-'), + ); + const env = buildReviewerGitGuardEnv( + { + PATH: process.env.PATH, + EJCLAW_WORK_DIR: protectedDir, + }, + true, + ); + const cwd = createTempRepo('ejclaw-reviewer-temp-repo-'); + fs.writeFileSync(path.join(cwd, 'note.txt'), 'ok\n'); + + expect(() => + execFileSync('git', ['add', 'note.txt'], { + cwd, + env, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }), + ).not.toThrow(); + }); + + it('blocks mutating git subcommands inside the protected reviewer workspace', () => { const cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'ejclaw-reviewer-test-')); + const env = buildReviewerGitGuardEnv( + { + PATH: process.env.PATH, + EJCLAW_WORK_DIR: cwd, + }, + true, + ); try { execFileSync('git', ['-c', 'color.ui=false', 'commit', '-m', 'x'], { diff --git a/src/paired-workspace-manager.test.ts b/src/paired-workspace-manager.test.ts index c8a0210..4083394 100644 --- a/src/paired-workspace-manager.test.ts +++ b/src/paired-workspace-manager.test.ts @@ -319,4 +319,76 @@ describe('paired workspace manager', () => { ), ).toBe(false); }); + + it('keeps reviewer git status clean while hiding denied tracked files', async () => { + const { db, manager } = await loadModules(); + db._initTestDatabase(); + + const canonicalDir = path.join(tempRoot, 'canonical'); + fs.mkdirSync(path.join(canonicalDir, '.claude'), { recursive: true }); + runGit(['init'], canonicalDir); + runGit(['config', 'user.email', 'test@example.com'], canonicalDir); + runGit(['config', 'user.name', 'EJClaw Test'], canonicalDir); + fs.writeFileSync(path.join(canonicalDir, 'tracked.ts'), 'export const ok = 1;\n'); + fs.writeFileSync( + path.join(canonicalDir, '.claude', 'settings.json'), + '{"secret":true}\n', + ); + fs.writeFileSync( + path.join(canonicalDir, '.env.production'), + 'TRACKED_SECRET=1\n', + ); + fs.writeFileSync(path.join(canonicalDir, '.env.example'), 'EXAMPLE=1\n'); + runGit( + ['add', 'tracked.ts', '.claude/settings.json', '.env.production', '.env.example'], + canonicalDir, + ); + runGit(['commit', '-m', 'initial'], canonicalDir); + + const now = '2026-03-28T00:00:00.000Z'; + db.upsertPairedProject({ + chat_jid: 'dc:test', + group_folder: 'paired-room', + canonical_work_dir: canonicalDir, + workspace_topology: 'shadow-snapshot', + created_at: now, + updated_at: now, + }); + db.createPairedTask({ + id: 'paired-task-4', + chat_jid: 'dc:test', + group_folder: 'paired-room', + owner_service_id: 'codex-main', + reviewer_service_id: 'codex-review', + title: null, + source_ref: 'HEAD', + review_requested_at: null, + status: 'draft', + created_at: now, + updated_at: now, + }); + + const reviewerWorkspace = + manager.refreshReviewerSnapshotForPairedTask('paired-task-4'); + + expect( + runGit(['status', '--short'], reviewerWorkspace.workspace_dir), + ).toBe(''); + expect( + fs.existsSync( + path.join(reviewerWorkspace.workspace_dir, '.claude', 'settings.json'), + ), + ).toBe(false); + expect( + fs.existsSync( + path.join(reviewerWorkspace.workspace_dir, '.env.production'), + ), + ).toBe(false); + expect( + fs.readFileSync( + path.join(reviewerWorkspace.workspace_dir, '.env.example'), + 'utf-8', + ), + ).toBe('EXAMPLE=1\n'); + }); }); diff --git a/src/paired-workspace-manager.ts b/src/paired-workspace-manager.ts index c6025ad..ec079ca 100644 --- a/src/paired-workspace-manager.ts +++ b/src/paired-workspace-manager.ts @@ -92,6 +92,19 @@ function runGit(args: string[], cwd?: string): string { }).trim(); } +function runGitWithInput( + args: string[], + cwd: string, + input: string, +): string { + return execFileSync('git', args, { + cwd, + input, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }).trim(); +} + function ensureGitRepository(repoDir: string): void { const insideWorkTree = runGit( ['rev-parse', '--is-inside-work-tree'], @@ -167,24 +180,35 @@ function shouldIncludeUntrackedReviewerPath(relativePath: string): boolean { ); } -function copySelectedSnapshotTree(sourceDir: string, targetDir: string): void { - resetDirectoryExceptGit(targetDir); - - const trackedFiles = listGitPaths(sourceDir, [ +function listAllowedTrackedFiles(sourceDir: string): string[] { + return listGitPaths(sourceDir, [ 'ls-files', '--cached', '-z', ]).filter((relativePath) => !isReviewerSnapshotDeniedPath(relativePath)); - const untrackedFiles = listGitPaths(sourceDir, [ +} + +function listDeletedTrackedFiles(sourceDir: string): string[] { + return listGitPaths(sourceDir, ['ls-files', '--deleted', '-z']).filter( + (relativePath) => !isReviewerSnapshotDeniedPath(relativePath), + ); +} + +function listAllowedUntrackedFiles(sourceDir: string): string[] { + return listGitPaths(sourceDir, [ 'ls-files', '--others', '--exclude-standard', '-z', ]).filter(shouldIncludeUntrackedReviewerPath); +} - const filesToCopy = [...new Set([...trackedFiles, ...untrackedFiles])].sort(); - - for (const relativePath of filesToCopy) { +function copySnapshotPaths( + sourceDir: string, + targetDir: string, + relativePaths: string[], +): void { + for (const relativePath of [...new Set(relativePaths)].sort()) { const sourcePath = path.join(sourceDir, relativePath); if (!fs.existsSync(sourcePath)) continue; @@ -194,6 +218,31 @@ function copySelectedSnapshotTree(sourceDir: string, targetDir: string): void { } } +function removeSnapshotPaths(targetDir: string, relativePaths: string[]): void { + for (const relativePath of [...new Set(relativePaths)].sort()) { + fs.rmSync(path.join(targetDir, relativePath), { + recursive: true, + force: true, + }); + } +} + +function applyReviewerSparseCheckout( + reviewerDir: string, + allowedTrackedFiles: string[], +): void { + runGit(['sparse-checkout', 'init', '--no-cone'], reviewerDir); + const patterns = + allowedTrackedFiles.length > 0 + ? `${allowedTrackedFiles.join('\n')}\n` + : '/*\n!/*\n'; + runGitWithInput( + ['sparse-checkout', 'set', '--no-cone', '--stdin'], + reviewerDir, + patterns, + ); +} + function configureReviewerGitIsolation(workspaceDir: string): void { try { runGit(['config', '--local', 'push.default', 'nothing'], workspaceDir); @@ -307,9 +356,26 @@ export function refreshReviewerSnapshotForPairedTask( runGit(['clone', '--shared', ownerWorkspace.workspace_dir, reviewerDir]); } + const allowedTrackedFiles = listAllowedTrackedFiles(ownerWorkspace.workspace_dir); + const deletedTrackedFiles = listDeletedTrackedFiles(ownerWorkspace.workspace_dir); + const allowedUntrackedFiles = listAllowedUntrackedFiles( + ownerWorkspace.workspace_dir, + ); + runGit(['reset', '--hard', 'HEAD'], reviewerDir); runGit(['clean', '-fdx'], reviewerDir); - copySelectedSnapshotTree(ownerWorkspace.workspace_dir, reviewerDir); + applyReviewerSparseCheckout(reviewerDir, allowedTrackedFiles); + copySnapshotPaths( + ownerWorkspace.workspace_dir, + reviewerDir, + allowedTrackedFiles, + ); + removeSnapshotPaths(reviewerDir, deletedTrackedFiles); + copySnapshotPaths( + ownerWorkspace.workspace_dir, + reviewerDir, + allowedUntrackedFiles, + ); configureReviewerGitIsolation(reviewerDir); const refreshedAt = new Date().toISOString();