review: harden readonly git checks and lint verification
This commit is contained in:
@@ -34,7 +34,7 @@ You may receive reference opinions from external models appended to your prompt.
|
|||||||
## Rules
|
## Rules
|
||||||
|
|
||||||
- Base your verdict on evidence (code, test output, logs), not on who said what first
|
- 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
|
- 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
|
- 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
|
- You do NOT implement or review code — you only judge the disagreement
|
||||||
|
|||||||
@@ -32,10 +32,10 @@ If you see a materially better design, debugging path, or scoping choice, propos
|
|||||||
## Rules
|
## 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
|
- 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`
|
- 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
|
- 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
|
- 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**
|
- 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
|
- Implementation, commits, and pushes require agreement from both sides. Either can veto
|
||||||
|
|||||||
@@ -479,12 +479,12 @@ server.tool(
|
|||||||
|
|
||||||
server.tool(
|
server.tool(
|
||||||
'run_verification',
|
'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
|
profile: z
|
||||||
.enum(VERIFICATION_PROFILES)
|
.enum(VERIFICATION_PROFILES)
|
||||||
.describe(
|
.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) => {
|
async (args) => {
|
||||||
|
|||||||
@@ -3,7 +3,12 @@ import path from 'path';
|
|||||||
import { pathToFileURL } from 'url';
|
import { pathToFileURL } from 'url';
|
||||||
export { computeVerificationSnapshotId } from '../../../shared/verification-snapshot.js';
|
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];
|
export type VerificationProfile = (typeof VERIFICATION_PROFILES)[number];
|
||||||
|
|
||||||
|
|||||||
@@ -169,6 +169,9 @@ describe('claude reviewer runtime guard', () => {
|
|||||||
const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true);
|
const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true);
|
||||||
expect(env.PATH).toContain('ejclaw-reviewer-git-');
|
expect(env.PATH).toContain('ejclaw-reviewer-git-');
|
||||||
expect(env.EJCLAW_REAL_GIT).toBeTruthy();
|
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', () => {
|
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', () => {
|
it('accepts a mounted local origin path that resolves as a git repo', () => {
|
||||||
const originDir = createTempRepo('ejclaw-reviewer-origin-');
|
const originDir = createTempRepo('ejclaw-reviewer-origin-');
|
||||||
const cwd = createTempRepo('ejclaw-reviewer-workspace-');
|
const cwd = createTempRepo('ejclaw-reviewer-workspace-');
|
||||||
|
|||||||
@@ -75,6 +75,9 @@ describe('codex reviewer runtime guard', () => {
|
|||||||
const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true);
|
const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true);
|
||||||
expect(env.PATH).toContain('ejclaw-reviewer-git-');
|
expect(env.PATH).toContain('ejclaw-reviewer-git-');
|
||||||
expect(env.EJCLAW_REAL_GIT).toBeTruthy();
|
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', () => {
|
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', () => {
|
it('accepts a mounted local origin path that resolves as a git repo', () => {
|
||||||
const originDir = createTempRepo('ejclaw-reviewer-origin-');
|
const originDir = createTempRepo('ejclaw-reviewer-origin-');
|
||||||
const cwd = createTempRepo('ejclaw-reviewer-workspace-');
|
const cwd = createTempRepo('ejclaw-reviewer-workspace-');
|
||||||
|
|||||||
@@ -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(
|
export function buildReviewerGitGuardEnv(
|
||||||
baseEnv: NodeJS.ProcessEnv,
|
baseEnv: NodeJS.ProcessEnv,
|
||||||
reviewerRuntime: boolean,
|
reviewerRuntime: boolean,
|
||||||
@@ -90,6 +102,7 @@ export function buildReviewerGitGuardEnv(
|
|||||||
const protectedWorkDir = baseEnv.EJCLAW_WORK_DIR || '';
|
const protectedWorkDir = baseEnv.EJCLAW_WORK_DIR || '';
|
||||||
const wrapperDir = createExecutableWrapperDir(baseEnv);
|
const wrapperDir = createExecutableWrapperDir(baseEnv);
|
||||||
const wrapperPath = path.join(wrapperDir, 'git');
|
const wrapperPath = path.join(wrapperDir, 'git');
|
||||||
|
const { gitConfigGlobalPath } = prepareGitConfigSandbox(wrapperDir);
|
||||||
const blocked = [...BLOCKED_GIT_SUBCOMMANDS]
|
const blocked = [...BLOCKED_GIT_SUBCOMMANDS]
|
||||||
.map((value) => `'${value}'`)
|
.map((value) => `'${value}'`)
|
||||||
.join(' ');
|
.join(' ');
|
||||||
@@ -179,6 +192,8 @@ exec "$real_git" "$@"
|
|||||||
...baseEnv,
|
...baseEnv,
|
||||||
EJCLAW_REAL_GIT: realGitPath,
|
EJCLAW_REAL_GIT: realGitPath,
|
||||||
EJCLAW_PROTECTED_WORK_DIR: protectedWorkDir,
|
EJCLAW_PROTECTED_WORK_DIR: protectedWorkDir,
|
||||||
|
GIT_CONFIG_GLOBAL: gitConfigGlobalPath,
|
||||||
|
GIT_CONFIG_NOSYSTEM: '1',
|
||||||
PATH: `${wrapperDir}:${baseEnv.PATH || ''}`,
|
PATH: `${wrapperDir}:${baseEnv.PATH || ''}`,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -20,7 +20,7 @@ describe('verification helpers', () => {
|
|||||||
expect(isVerificationProfile('test')).toBe(true);
|
expect(isVerificationProfile('test')).toBe(true);
|
||||||
expect(isVerificationProfile('typecheck')).toBe(true);
|
expect(isVerificationProfile('typecheck')).toBe(true);
|
||||||
expect(isVerificationProfile('build')).toBe(true);
|
expect(isVerificationProfile('build')).toBe(true);
|
||||||
expect(isVerificationProfile('lint')).toBe(false);
|
expect(isVerificationProfile('lint')).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('builds deterministic commands for each profile', () => {
|
it('builds deterministic commands for each profile', () => {
|
||||||
@@ -44,6 +44,49 @@ describe('verification helpers', () => {
|
|||||||
args: ['run', 'build'],
|
args: ['run', 'build'],
|
||||||
requiredScript: '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', () => {
|
it('selects the workspace package manager for verification commands', () => {
|
||||||
@@ -250,6 +293,53 @@ describe('verification helpers', () => {
|
|||||||
expect(result.snapshotId).toBe(expectedSnapshotId);
|
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 () => {
|
it('does not leak host secrets into direct verification commands', async () => {
|
||||||
const repoDir = fs.mkdtempSync(
|
const repoDir = fs.mkdtempSync(
|
||||||
path.join(os.tmpdir(), 'ejclaw-verification-direct-secret-'),
|
path.join(os.tmpdir(), 'ejclaw-verification-direct-secret-'),
|
||||||
|
|||||||
@@ -11,7 +11,12 @@ import {
|
|||||||
} from './workspace-package-manager.js';
|
} from './workspace-package-manager.js';
|
||||||
import { computeVerificationSnapshotId } from '../shared/verification-snapshot.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];
|
export type VerificationProfile = (typeof VERIFICATION_PROFILES)[number];
|
||||||
|
|
||||||
@@ -91,12 +96,19 @@ export function buildVerificationCommand(
|
|||||||
profile: VerificationProfile,
|
profile: VerificationProfile,
|
||||||
repoDir: string = process.cwd(),
|
repoDir: string = process.cwd(),
|
||||||
): VerificationCommandSpec {
|
): VerificationCommandSpec {
|
||||||
|
const scripts = readPackageScripts(repoDir);
|
||||||
const scriptName =
|
const scriptName =
|
||||||
profile === 'test'
|
profile === 'test'
|
||||||
? 'test'
|
? 'test'
|
||||||
: profile === 'typecheck'
|
: profile === 'typecheck'
|
||||||
? 'typecheck'
|
? 'typecheck'
|
||||||
: 'build';
|
: profile === 'build'
|
||||||
|
? 'build'
|
||||||
|
: scripts['lint:ci']
|
||||||
|
? 'lint:ci'
|
||||||
|
: scripts['lint:check']
|
||||||
|
? 'lint:check'
|
||||||
|
: 'lint';
|
||||||
const command = buildWorkspaceScriptCommand(repoDir, scriptName);
|
const command = buildWorkspaceScriptCommand(repoDir, scriptName);
|
||||||
return {
|
return {
|
||||||
file: command.file,
|
file: command.file,
|
||||||
|
|||||||
Reference in New Issue
Block a user