Skip to content

Commit a63539c

Browse files
mchwangclaude
andauthored
Run every test and script Git command with an allowlisted environment (#82)
* Open a stale item's comparison when the answer that makes it stale arrives after it was selected The review page chose "Since approval" or "Full change" once, in select(), from the data present at the click. When the reviewer selected P1 while an Assign request was still in flight, P1 was still approved in that data, so the page chose Full change. The Assign answer then made P1 stale and re-rendered its stale reasons, but it kept the earlier choice, so the "At approval" comparison never appeared. This is the intermittent failure of review.spec.ts "reviews real changes, persists approval and conversation, and assigns foreign code" (CI run 36498484408, attempt 1). The page now stores only an explicit toggle, keyed to the item and to the stale state it was made on (reasons, current segments and plan item). With no matching choice, a stale item always opens in Since approval, whatever answer made it stale. An explicit choice survives answers that leave that state unchanged, and ends when the item's code or plan changes or the reviewer selects another item. The new browser test holds the Assign answer with a promise, selects P1 while it is still approved, releases the answer, and asserts the comparison and the stored stale state. It fails on the previous app.js at the race step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Key the explicit view choice to every segment field the approval fingerprint covers The stale-state key used only path, operation and content, but core/approvals.ts also counts oldPath, kind, context and owners as a change. Moving unchanged text to another function changes only context and makes the item stale, yet the key stayed the same, so an earlier "Full change" choice hid the comparison for the new state. The key now uses the same fields. The new browser test changes context, owners and oldPath one at a time in the review answer and expects the comparison each time. It fails on the previous key, and dropping any one of the three fields from the key fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Name each stale state on the server instead of mirroring staleness in the page The page kept an explicit view choice by building its own key from the item's segments, reasons and plan item. That copy of the server's staleness rules missed inputs: the approval the comparison is drawn against, ambiguous segments the item owns, its dependencies' stale states, and the context of a replaced head after a queue attempt. In each case an old choice could apply to a stale state the reviewer had not seen. ReviewService.load() now returns staleKey for each stale item: a hash of the saved approval, the current approval fingerprint (fingerprint() is now exported from core/approvals.ts), the ambiguous segments it owns, its dependencies' stale keys, and the queue-replacement context when it applies. The page compares only this key. Tests: - test/review.test.ts: three unit tests cover a dependency change with the same reasons, re-approval while still stale, re-approval followed by an ambiguous change with identical own segments and reasons, an ambiguous change on a still-stale item, and a second replaced head. Removing any one of the five key inputs fails a test. - The browser test that edited segment fields is replaced by one that changes only staleKey in the answer; it fails if the page ignores the key. - The "explicit choice survives Refresh" step now waits until the Refresh answer is rendered, so it fails if Refresh resets the choice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Key a stale item's ambiguous segments by every field an approval covers The stale key identified the ambiguous segments an item owns by their choice keys, which leave out context and owners. An ambiguous segment that moved to another function or gained an owner therefore kept the same stale key, so an earlier "Full change" choice survived the new stale state. core/approvals.ts now has reviewedSegment(), the segment fields an approval covers, used by fingerprint() and by the ambiguous part of the stale key. The ambiguous stale-key test now has P3 edit the shared line and restore its text, so the segment keeps its choice key but gains an owner; the key must change. Using only choice keys fails that step. The two longer stale-key tests get the 30 s limit other multi-commit tests in this file use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Hash each ambiguous segment once per load in the stale key The stale key serialized an ambiguous segment's full reviewed fields once for every stale item that owns it. A large segment shared by many items made each synchronous load() repeat that work per owner. load() now hashes reviewedSegment() once per segment, memoized by segment key, and each owner's stale key includes that digest. The fields stay the ones an approval covers, so a change in context or owners still gives a new key. The ambiguous stale-key test now approves P1, P2 and P3 first, so the shared line belongs to three stale items, and counts reviewedSegment() calls from load() through a passthrough mock: one per segment. The previous per-owner form fails that count, and leaving owners out of the digest fails the owners step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run this PR's test Git commands with the hardened fixture invocation The Git commands added by this PR's tests inherited the caller's environment and user config and only disabled hooks. An inherited GIT_DIR or GIT_WORK_TREE could redirect their commits outside the fixture, and user config could change their behaviour, against the AGENTS.md rule on subprocess environments and hardened Git. test/fixtures/git.ts adds fixtureGit(): an allowlisted environment (PATH and the Git hardening variables only), no user or system config, no hooks, no replace objects, no lazy fetch and no network protocols, as git/history.ts does. The new stale-key unit tests and the in-flight browser test use it. test/fixture-git.test.ts sets GIT_DIR, GIT_WORK_TREE, GIT_CONFIG_GLOBAL and GIT_AUTHOR_NAME to point elsewhere and checks that a commit stays in the fixture with the fixture's identity. Passing process.env through fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Run every test and script Git command with an allowlisted environment Test Git helpers in review, browser, agent, store, history, plant and question tests inherited process.env. They now use fixtureGit(). scripts/demo.ts and scripts/plant.ts removed only GIT_* names from the inherited environment. Git still read HOME and XDG_CONFIG_HOME, so a user's git/ignore could make plant's `git add -A` skip the undeclared plant while sealed.json recorded it. Both scripts now pass only PATH and the hardening variables, and add --no-pager, --no-replace-objects and protocol.allow=never. Plant allows the file protocol for its one local clone only, as git/clone.ts does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Remove a saved vitest report committed by mistake, and ignore .vitest/ Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Build the fixture Git invocation from the scripts' hardened options and environment Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent f53227c commit a63539c

19 files changed

Lines changed: 89 additions & 77 deletions

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ dist/
66
.codeboost-local/
77
playwright-report/
88
test-results/
9+
.vitest/

‎scripts/demo.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { randomUUID } from 'node:crypto';
66
import { Store } from '../runner/store.ts';
77
import type { ReviewConfig } from '../runner/review.ts';
88
import type { Plan } from '../core/plan.ts';
9-
import { isolatedGitEnvironment } from './git-environment.ts';
9+
import { HARDENED_GIT_OPTIONS, hardenedGitEnvironment } from './git-environment.ts';
1010
/** Disposable fixture only. Never runs against the user's working repository. */
1111
export function createDemo(directory: string): ReviewConfig {
1212
const requested = resolve(directory), temporary = resolve(tmpdir());
@@ -35,7 +35,7 @@ export function createDemo(directory: string): ReviewConfig {
3535
}
3636
if (existsSync(root)) throw new Error('Demo directory exists without a configuration. Choose a new empty path.');
3737
mkdirSync(root, { recursive: true }); const repository = join(root, 'retry-service'); mkdirSync(repository);
38-
const git = (...args: string[]) => execFileSync('git', ['-c','core.hooksPath=/dev/null',...args], { cwd: repository, env: isolatedGitEnvironment(), encoding: 'utf8', stdio: ['ignore','pipe','pipe'] }).trim();
38+
const git = (...args: string[]) => execFileSync('git', [...HARDENED_GIT_OPTIONS,...args], { cwd: repository, env: hardenedGitEnvironment(), encoding: 'utf8', stdio: ['ignore','pipe','pipe'] }).trim();
3939
git('init','-b','main'); git('config','user.name','Codeboost Demo'); git('config','user.email','demo@example.invalid'); git('config','commit.gpgsign','false');
4040
const write = (path: string, text: string | Buffer) => writeFileSync(join(repository,path),text);
4141
const commit = (message: string) => { git('add','-A');git('commit','-m',message);return git('rev-parse','HEAD'); };

‎scripts/git-environment.ts‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
1-
/** Git environment names are case-insensitive on Windows. */
2-
export function isolatedGitEnvironment(): NodeJS.ProcessEnv {
3-
return {
4-
...Object.fromEntries(Object.entries(process.env).filter(([key]) => !/^GIT_/i.test(key))),
5-
GIT_CONFIG_NOSYSTEM: '1',
6-
GIT_CONFIG_GLOBAL: '/dev/null',
7-
GIT_TERMINAL_PROMPT: '0',
8-
};
1+
/** Options for the repository's hardened Git invocation: no pager, replace objects, hooks or network protocols. */
2+
export const HARDENED_GIT_OPTIONS = ['--no-pager', '--no-replace-objects', '-c', 'core.hooksPath=/dev/null', '-c', 'protocol.allow=never'];
3+
4+
/**
5+
* An allowlisted Git environment: PATH plus settings that disable user and system config, prompts and lazy fetch.
6+
* Nothing else is inherited, because Git also reads variables outside the GIT_ namespace, such as HOME and
7+
* XDG_CONFIG_HOME for the user's ignore and attributes files.
8+
*/
9+
export function hardenedGitEnvironment(): NodeJS.ProcessEnv {
10+
return { PATH: process.env.PATH ?? '', GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', GIT_TERMINAL_PROMPT: '0', GIT_NO_LAZY_FETCH: '1' };
911
}

‎scripts/plant.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { isolatedGitEnvironment } from './git-environment.ts';
1+
import { HARDENED_GIT_OPTIONS, hardenedGitEnvironment } from './git-environment.ts';
22
import { execFileSync } from 'node:child_process';
33
import { existsSync, mkdirSync, writeFileSync, appendFileSync, lstatSync, readFileSync, realpathSync } from 'node:fs';
44
import { resolve, join, dirname, basename, relative, isAbsolute } from 'node:path';
@@ -42,8 +42,8 @@ export function plant(config: ReviewConfig, destination: string, input: PlantInp
4242
const declared=candidates[randomInt(candidates.length)]!;
4343
const owned=history.commits.map((commit,index)=>({index,owner:owners.get(commit.sha)})).filter(entry=>entry.owner&&plan!.items.some(item=>item.id===entry.owner));
4444
const outside=owned[randomInt(owned.length)]!;
45-
const env=isolatedGitEnvironment();
46-
const gitRaw=(cwd:string,...args:string[])=>execFileSync('git',['-c','core.hooksPath=/dev/null','-c','commit.gpgsign=false',...args],{cwd,env,encoding:'utf8',stdio:['ignore','pipe','pipe'],timeout:30000,maxBuffer:32*1024*1024});
45+
const env=hardenedGitEnvironment();
46+
const gitRaw=(cwd:string,...args:string[])=>execFileSync('git',[...HARDENED_GIT_OPTIONS,'-c','commit.gpgsign=false',...args],{cwd,env,encoding:'utf8',stdio:['ignore','pipe','pipe'],timeout:30000,maxBuffer:32*1024*1024});
4747
const git=(cwd:string,...args:string[])=>gitRaw(cwd,...args).trim();
4848
// Refuse existing paths, symlink targets, and unsupported declared-file transitions before creating output.
4949
for(const sha of [snapshot.base,...history.commits.map(commit=>commit.sha)]){
@@ -54,7 +54,7 @@ export function plant(config: ReviewConfig, destination: string, input: PlantInp
5454
if(!/^100(?:644|755) blob /.test(git(config.repository,'ls-tree',commit.sha,'--',declared.path)))throw new Error('Declared plant needs a regular file retained through the remaining history.');
5555
}
5656
mkdirSync(root,{recursive:true});const repository=join(root,'repository');
57-
git(root,'clone','--no-local','--no-checkout',resolve(config.repository),repository);
57+
git(root,'-c','protocol.file.allow=always','clone','--no-local','--no-checkout',resolve(config.repository),repository);
5858
git(repository,'config','user.name','Codeboost Experiment');git(repository,'config','user.email','experiment@example.invalid');
5959
git(repository,'checkout','-b','review-experiment',snapshot.base);
6060
const baseEntries:BaseEntry[]=git(repository,'ls-tree','-rz',snapshot.base).split('\0').filter(Boolean).map(record=>{

‎test/agent-clone-async.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { execFileSync } from 'node:child_process';
2+
import { fixtureGit } from './fixtures/git.ts';
23
import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
34
import { tmpdir } from 'node:os';
45
import { join } from 'node:path';
@@ -21,8 +22,7 @@ import type { ProcessGroup } from '../agents/process-group.ts';
2122
// The asynchronous clone (#51 item 5): every Git call runs in its own process group, and an abort settles only after
2223
// that group has exited and the partial clone is gone.
2324
const roots: string[] = [];
24-
const git = (cwd: string, ...args: string[]) => execFileSync('git', ['-c', 'core.hooksPath=/dev/null', ...args],
25-
{ cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
25+
const git = fixtureGit;
2626
function fixture() {
2727
const root = mkdtempSync(join(tmpdir(), 'clone-async-')); roots.push(root);
2828
const source = join(root, 'source'), parent = join(root, 'tasks');

‎test/agent-clone.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { execFileSync } from 'node:child_process';
2+
import { fixtureGit } from './fixtures/git.ts';
23
import { mkdtempSync, mkdirSync, readFileSync, readdirSync, rmSync, lstatSync, symlinkSync, writeFileSync, renameSync, opendirSync } from 'node:fs';
34
import { tmpdir } from 'node:os';
45
import { join } from 'node:path';
@@ -15,8 +16,7 @@ vi.mock('node:child_process', async importOriginal => {
1516
});
1617

1718
const roots: string[] = [];
18-
const git = (cwd: string, ...args: string[]) => execFileSync('git', ['-c', 'core.hooksPath=/dev/null', ...args],
19-
{ cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
19+
const git = fixtureGit;
2020
function fixture() {
2121
const root = mkdtempSync(join(tmpdir(), 'clone-test-')); roots.push(root);
2222
const source = join(root, 'source'), parent = join(root, 'tasks');

‎test/agent-container.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { execFileSync, spawnSync } from 'node:child_process';
2+
import { fixtureGit } from './fixtures/git.ts';
23
import { createHash, randomBytes, randomUUID } from 'node:crypto';
34
import { chmodSync, existsSync, mkdtempSync, mkdirSync, readFileSync, renameSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs';
45
import { tmpdir } from 'node:os';
@@ -30,8 +31,7 @@ const containers = new Set<string>();
3031
const profiles: Awaited<ReturnType<typeof createContainerProfile>>[] = [];
3132
let imageId = '';
3233
const vendorNetworks: VendorNetwork[] = [];
33-
const git = (cwd: string, ...args: string[]) => execFileSync('git', ['-c', 'core.hooksPath=/dev/null', ...args],
34-
{ cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
34+
const git = fixtureGit;
3535
const docker = (...args: string[]) => execFileSync('docker', args, {
3636
encoding: 'utf8', timeout: 60_000, stdio: ['ignore', 'pipe', 'pipe'],
3737
}).trim();

‎test/agent-question.test.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { execFileSync } from 'node:child_process';
1+
import { fixtureGit } from './fixtures/git.ts';
22
import { randomBytes } from 'node:crypto';
33
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
44
import { tmpdir } from 'node:os';
@@ -10,8 +10,7 @@ import { QuestionWorker } from '../runner/question-agent.ts';
1010
// the vendor CLI in the "questions" phase. Runs with the other Docker suites, one file at a time.
1111
const roots: string[] = [];
1212
afterAll(() => { for (const root of roots) rmSync(root, { recursive: true, force: true }); });
13-
const git = (cwd: string, ...args: string[]) => execFileSync('git', ['-c', 'core.hooksPath=/dev/null', ...args],
14-
{ cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
13+
const git = fixtureGit;
1514

1615
function repository(secret: string) {
1716
const root = mkdtempSync(join(tmpdir(), 'question-container-')); roots.push(root);

‎test/agent-storage-async.test.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { execFileSync } from 'node:child_process';
1+
import { fixtureGit } from './fixtures/git.ts';
22
import { randomUUID } from 'node:crypto';
33
import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
44
import { tmpdir } from 'node:os';
@@ -128,8 +128,7 @@ if (a === 'rm' || (a === 'volume' && b === 'rm')) {
128128
console.error('fake docker: unsupported ' + args.join(' ')); process.exit(2);
129129
`;
130130

131-
const git = (cwd: string, ...args: string[]) => execFileSync('git', ['-c', 'core.hooksPath=/dev/null', ...args],
132-
{ cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
131+
const git = fixtureGit;
133132
const clone = () => {
134133
const source = join(root, `source-${randomUUID()}`), parent = join(root, `staging-${randomUUID()}`);
135134
mkdirSync(source); mkdirSync(parent);

‎test/agent-supervisor.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { randomUUID } from 'node:crypto';
22
import { execFileSync, spawnSync } from 'node:child_process';
3+
import { fixtureGit } from './fixtures/git.ts';
34
import { chmodSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs';
45
import { tmpdir } from 'node:os';
56
import { dirname, join } from 'node:path';
@@ -23,8 +24,7 @@ const testOwner = (attemptId = 'fixture') => ({ runnerOwner: TEST_RUNNER_OWNER,
2324
const roots: string[] = [], profiles: ContainerProfile[] = [];
2425
const allocations: ReturnType<typeof prepareTaskFilesystems>[] = [];
2526
let imageId = '';
26-
const git = (cwd: string, ...args: string[]) => execFileSync('git', ['-c', 'core.hooksPath=/dev/null', ...args],
27-
{ cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
27+
const git = fixtureGit;
2828

2929
function fixture() {
3030
const root = mkdtempSync(join(tmpdir(), 'agent-supervisor-')); roots.push(root);

0 commit comments

Comments
 (0)