From 12838a8b077c7e41501ee599aeee467e4c6b112f Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:48:33 +0900 Subject: [PATCH] review: harden readonly git checks and lint verification --- prompts/arbiter-paired-room.md | 2 +- prompts/claude-paired-room.md | 4 +- runners/agent-runner/src/ipc-mcp-stdio.ts | 4 +- runners/agent-runner/src/verification.ts | 7 +- .../test/reviewer-runtime.test.ts | 27 ++++++ .../test/reviewer-runtime.test.ts | 27 ++++++ runners/shared/src/reviewer-git-guard.ts | 15 +++ src/verification.test.ts | 92 ++++++++++++++++++- src/verification.ts | 16 +++- 9 files changed, 185 insertions(+), 9 deletions(-) diff --git a/prompts/arbiter-paired-room.md b/prompts/arbiter-paired-room.md index f620a39..9811ddc 100644 --- a/prompts/arbiter-paired-room.md +++ b/prompts/arbiter-paired-room.md @@ -34,7 +34,7 @@ You may receive reference opinions from external models appended to your prompt. ## Rules - Base your verdict on evidence (code, test output, logs), not on who said what first -- Distinguish reviewer snapshot limits from real product bugs. Reviewer workspaces may intentionally omit heavy artifacts like `node_modules`, `dist`, and `build`; inability to run direct local test/typecheck/build there is not, by itself, a blocker if dedicated verification evidence exists +- Distinguish reviewer snapshot limits from real product bugs. Reviewer workspaces may intentionally omit heavy artifacts like `node_modules`, `dist`, and `build`; inability to run direct local test/typecheck/build/lint there is not, by itself, a blocker if dedicated verification evidence exists - When verification evidence exists from the dedicated verification path, judge that evidence on its merits instead of requiring the reviewer to reproduce the same result from the lightweight reviewer snapshot - Your verdict is final for this deadlock cycle — after it, work resumes normally - You do NOT implement or review code — you only judge the disagreement diff --git a/prompts/claude-paired-room.md b/prompts/claude-paired-room.md index 87d9803..1d422fd 100644 --- a/prompts/claude-paired-room.md +++ b/prompts/claude-paired-room.md @@ -32,10 +32,10 @@ If you see a materially better design, debugging path, or scoping choice, propos ## Rules - Judge completion only by verification output. "It should work now" means run it. "I'm confident" means nothing — confidence is not evidence. "I tested earlier" means test again if code changed since. "It's a trivial change" means verify anyway -- Reviewer runs against the owner's current workspace in read-only mode. Do not treat the inability to run direct local test/typecheck/build from the reviewer workspace as a product bug by itself +- Reviewer runs against the owner's current workspace in read-only mode. Do not treat the inability to run direct local test/typecheck/build/lint from the reviewer workspace as a product bug by itself - Treat `EJCLAW_WORK_DIR` as the canonical verification root for this turn. You may inspect other local paths for context, but final review findings must be re-checked against `EJCLAW_WORK_DIR` - Do not use a different clone, canonical repo path, or cached session path as the sole basis for `BLOCKED`, `DONE_WITH_CONCERNS`, or change requests. If another path disagrees with `EJCLAW_WORK_DIR`, prefer `EJCLAW_WORK_DIR` and explicitly call out the mismatch -- When test/typecheck/build evidence is needed, prefer the dedicated verification path (`run_verification`) over assuming the reviewer workspace should execute the full project locally +- When test/typecheck/build/lint evidence is needed, prefer the dedicated verification path (`run_verification`) over assuming the reviewer workspace should execute the full project locally - Separate correctness issues from improvement ideas. If something is only a better alternative, label it as optional instead of blocking the owner unnecessarily - Stagnation: **Spinning** (same error 3+), **Oscillation** (alternating approaches), **Diminishing returns** (shrinking improvement), **No progress** (discussion without change) — name the pattern and report: **Status**, **Attempted**, **Recommendation** - Implementation, commits, and pushes require agreement from both sides. Either can veto diff --git a/runners/agent-runner/src/ipc-mcp-stdio.ts b/runners/agent-runner/src/ipc-mcp-stdio.ts index b47c67f..0fe7ad3 100644 --- a/runners/agent-runner/src/ipc-mcp-stdio.ts +++ b/runners/agent-runner/src/ipc-mcp-stdio.ts @@ -479,12 +479,12 @@ server.tool( server.tool( 'run_verification', - 'Run a fixed verification profile directly on the host against the current repo snapshot using a restricted environment and snapshot checks. Use this instead of broad shell write access for test/typecheck/build verification.', + 'Run a fixed verification profile directly on the host against the current repo snapshot using a restricted environment and snapshot checks. Use this instead of broad shell write access for test/typecheck/build/lint verification.', { profile: z .enum(VERIFICATION_PROFILES) .describe( - 'Fixed verification profile. Runs the workspace test/typecheck/build scripts with the repo-configured package manager.', + 'Fixed verification profile. Runs the workspace test/typecheck/build/lint scripts with the repo-configured package manager.', ), }, async (args) => { diff --git a/runners/agent-runner/src/verification.ts b/runners/agent-runner/src/verification.ts index 01cdc62..4d7c662 100644 --- a/runners/agent-runner/src/verification.ts +++ b/runners/agent-runner/src/verification.ts @@ -3,7 +3,12 @@ import path from 'path'; import { pathToFileURL } from 'url'; export { computeVerificationSnapshotId } from '../../../shared/verification-snapshot.js'; -export const VERIFICATION_PROFILES = ['test', 'typecheck', 'build'] as const; +export const VERIFICATION_PROFILES = [ + 'test', + 'typecheck', + 'build', + 'lint', +] as const; export type VerificationProfile = (typeof VERIFICATION_PROFILES)[number]; diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index a8f06b0..0052108 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -169,6 +169,9 @@ describe('claude reviewer runtime guard', () => { const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true); expect(env.PATH).toContain('ejclaw-reviewer-git-'); expect(env.EJCLAW_REAL_GIT).toBeTruthy(); + expect(env.GIT_CONFIG_GLOBAL).toContain('global.gitconfig'); + expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); + expect(fs.existsSync(env.GIT_CONFIG_GLOBAL!)).toBe(true); }); it('prefers an executable HOME-scoped wrapper dir before tmp', () => { @@ -245,6 +248,30 @@ describe('claude reviewer runtime guard', () => { } }); + it('overrides problematic git config paths so read-only git queries still work', () => { + const cwd = createTempRepo('ejclaw-reviewer-readonly-git-'); + const env = buildReviewerGitGuardEnv( + { + PATH: process.env.PATH, + HOME: cwd, + EJCLAW_WORK_DIR: cwd, + GIT_CONFIG_GLOBAL: path.join(cwd, '.gitconfig'), + }, + true, + ); + + expect(env.GIT_CONFIG_GLOBAL).not.toBe(path.join(cwd, '.gitconfig')); + expect(() => + execFileSync('git', ['log', '--oneline', '-1'], { + cwd, + env, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }), + ).not.toThrow(); + expect(fs.existsSync(path.join(cwd, '.gitconfig'))).toBe(false); + }); + it('accepts a mounted local origin path that resolves as a git repo', () => { const originDir = createTempRepo('ejclaw-reviewer-origin-'); const cwd = createTempRepo('ejclaw-reviewer-workspace-'); diff --git a/runners/codex-runner/test/reviewer-runtime.test.ts b/runners/codex-runner/test/reviewer-runtime.test.ts index d4493e3..04df0a0 100644 --- a/runners/codex-runner/test/reviewer-runtime.test.ts +++ b/runners/codex-runner/test/reviewer-runtime.test.ts @@ -75,6 +75,9 @@ describe('codex reviewer runtime guard', () => { const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true); expect(env.PATH).toContain('ejclaw-reviewer-git-'); expect(env.EJCLAW_REAL_GIT).toBeTruthy(); + expect(env.GIT_CONFIG_GLOBAL).toContain('global.gitconfig'); + expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); + expect(fs.existsSync(env.GIT_CONFIG_GLOBAL!)).toBe(true); }); it('prefers an executable HOME-scoped wrapper dir before tmp', () => { @@ -151,6 +154,30 @@ describe('codex reviewer runtime guard', () => { } }); + it('overrides problematic git config paths so read-only git queries still work', () => { + const cwd = createTempRepo('ejclaw-reviewer-readonly-git-'); + const env = buildReviewerGitGuardEnv( + { + PATH: process.env.PATH, + HOME: cwd, + EJCLAW_WORK_DIR: cwd, + GIT_CONFIG_GLOBAL: path.join(cwd, '.gitconfig'), + }, + true, + ); + + expect(env.GIT_CONFIG_GLOBAL).not.toBe(path.join(cwd, '.gitconfig')); + expect(() => + execFileSync('git', ['log', '--oneline', '-1'], { + cwd, + env, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }), + ).not.toThrow(); + expect(fs.existsSync(path.join(cwd, '.gitconfig'))).toBe(false); + }); + it('accepts a mounted local origin path that resolves as a git repo', () => { const originDir = createTempRepo('ejclaw-reviewer-origin-'); const cwd = createTempRepo('ejclaw-reviewer-workspace-'); diff --git a/runners/shared/src/reviewer-git-guard.ts b/runners/shared/src/reviewer-git-guard.ts index 3fedb8d..3d62cf5 100644 --- a/runners/shared/src/reviewer-git-guard.ts +++ b/runners/shared/src/reviewer-git-guard.ts @@ -78,6 +78,18 @@ function createExecutableWrapperDir(baseEnv: NodeJS.ProcessEnv): string { ); } +function prepareGitConfigSandbox(wrapperDir: string): { + gitConfigGlobalPath: string; +} { + const configDir = path.join(wrapperDir, 'git-config'); + fs.mkdirSync(configDir, { recursive: true }); + const gitConfigGlobalPath = path.join(configDir, 'global.gitconfig'); + if (!fs.existsSync(gitConfigGlobalPath)) { + fs.writeFileSync(gitConfigGlobalPath, ''); + } + return { gitConfigGlobalPath }; +} + export function buildReviewerGitGuardEnv( baseEnv: NodeJS.ProcessEnv, reviewerRuntime: boolean, @@ -90,6 +102,7 @@ export function buildReviewerGitGuardEnv( const protectedWorkDir = baseEnv.EJCLAW_WORK_DIR || ''; const wrapperDir = createExecutableWrapperDir(baseEnv); const wrapperPath = path.join(wrapperDir, 'git'); + const { gitConfigGlobalPath } = prepareGitConfigSandbox(wrapperDir); const blocked = [...BLOCKED_GIT_SUBCOMMANDS] .map((value) => `'${value}'`) .join(' '); @@ -179,6 +192,8 @@ exec "$real_git" "$@" ...baseEnv, EJCLAW_REAL_GIT: realGitPath, EJCLAW_PROTECTED_WORK_DIR: protectedWorkDir, + GIT_CONFIG_GLOBAL: gitConfigGlobalPath, + GIT_CONFIG_NOSYSTEM: '1', PATH: `${wrapperDir}:${baseEnv.PATH || ''}`, }; } diff --git a/src/verification.test.ts b/src/verification.test.ts index 4555424..de8fb61 100644 --- a/src/verification.test.ts +++ b/src/verification.test.ts @@ -20,7 +20,7 @@ describe('verification helpers', () => { expect(isVerificationProfile('test')).toBe(true); expect(isVerificationProfile('typecheck')).toBe(true); expect(isVerificationProfile('build')).toBe(true); - expect(isVerificationProfile('lint')).toBe(false); + expect(isVerificationProfile('lint')).toBe(true); }); it('builds deterministic commands for each profile', () => { @@ -44,6 +44,49 @@ describe('verification helpers', () => { args: ['run', 'build'], requiredScript: 'build', }); + expect(buildVerificationCommand('lint', repoDir)).toMatchObject({ + file: 'npm', + args: ['run', 'lint'], + requiredScript: 'lint', + }); + }); + + it('prefers lint:ci and lint:check before plain lint', () => { + const repoDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'ejclaw-verification-lint-'), + ); + fs.writeFileSync( + path.join(repoDir, 'package.json'), + JSON.stringify({ + scripts: { + lint: 'eslint .', + 'lint:check': 'eslint --max-warnings 0 .', + 'lint:ci': 'eslint --format compact .', + }, + }), + ); + + expect(buildVerificationCommand('lint', repoDir)).toMatchObject({ + file: 'npm', + args: ['run', 'lint:ci'], + requiredScript: 'lint:ci', + }); + + fs.writeFileSync( + path.join(repoDir, 'package.json'), + JSON.stringify({ + scripts: { + lint: 'eslint .', + 'lint:check': 'eslint --max-warnings 0 .', + }, + }), + ); + + expect(buildVerificationCommand('lint', repoDir)).toMatchObject({ + file: 'npm', + args: ['run', 'lint:check'], + requiredScript: 'lint:check', + }); }); it('selects the workspace package manager for verification commands', () => { @@ -250,6 +293,53 @@ describe('verification helpers', () => { expect(result.snapshotId).toBe(expectedSnapshotId); }); + it('runs lint verification using the preferred lint script', async () => { + const repoDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'ejclaw-verification-direct-lint-'), + ); + fs.mkdirSync(path.join(repoDir, 'node_modules', '.bin'), { + recursive: true, + }); + fs.mkdirSync(path.join(repoDir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(repoDir, 'package.json'), + JSON.stringify({ + name: 'verification-direct-lint', + packageManager: 'bun@1.3.11', + scripts: { + 'lint:check': + "node -e \"process.stdout.write('lint-check:' + (process.env.CI || 'missing'))\"", + }, + }), + ); + fs.writeFileSync(path.join(repoDir, 'bun.lock'), ''); + fs.writeFileSync( + path.join(repoDir, 'node_modules', '.bin', 'placeholder'), + '', + ); + fs.writeFileSync( + path.join(repoDir, 'src', 'index.ts'), + 'export const value = 1;\n', + ); + ensureWorkspaceDependenciesInstalled(repoDir); + + const expectedSnapshotId = computeVerificationSnapshot(repoDir).snapshotId; + const result = await runVerificationRequest( + { + requestId: 'req-direct-lint', + profile: 'lint', + expectedSnapshotId, + }, + { repoDir }, + ); + + expect(result.ok).toBe(true); + expect(result.exitCode).toBe(0); + expect(result.command).toContain('lint:check'); + expect(result.stdout).toContain('lint-check:1'); + expect(result.snapshotId).toBe(expectedSnapshotId); + }); + it('does not leak host secrets into direct verification commands', async () => { const repoDir = fs.mkdtempSync( path.join(os.tmpdir(), 'ejclaw-verification-direct-secret-'), diff --git a/src/verification.ts b/src/verification.ts index 787502d..4aacc2a 100644 --- a/src/verification.ts +++ b/src/verification.ts @@ -11,7 +11,12 @@ import { } from './workspace-package-manager.js'; import { computeVerificationSnapshotId } from '../shared/verification-snapshot.js'; -export const VERIFICATION_PROFILES = ['test', 'typecheck', 'build'] as const; +export const VERIFICATION_PROFILES = [ + 'test', + 'typecheck', + 'build', + 'lint', +] as const; export type VerificationProfile = (typeof VERIFICATION_PROFILES)[number]; @@ -91,12 +96,19 @@ export function buildVerificationCommand( profile: VerificationProfile, repoDir: string = process.cwd(), ): VerificationCommandSpec { + const scripts = readPackageScripts(repoDir); const scriptName = profile === 'test' ? 'test' : profile === 'typecheck' ? 'typecheck' - : 'build'; + : profile === 'build' + ? 'build' + : scripts['lint:ci'] + ? 'lint:ci' + : scripts['lint:check'] + ? 'lint:check' + : 'lint'; const command = buildWorkspaceScriptCommand(repoDir, scriptName); return { file: command.file,