Skip to content

Commit d1fbd19

Browse files
mchwangclaude
andauthored
Run history Git with an allowlisted environment (#83)
* 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> * Run history Git with an allowlisted environment readHistory removed only GIT_* names from the inherited environment. Git still used HOME and XDG_CONFIG_HOME to find the user's attributes file, so a user-level `-diff` attribute removed every hunk range from the reviewed diff. Pass only PATH and the hardening variables. 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> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent a63539c commit d1fbd19

2 files changed

Lines changed: 12 additions & 3 deletions

File tree

‎git/history.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,15 +28,15 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim
2828
if (diffBytes > maxDiffBytes) throw new Error('Review history exceeds the cumulative diff byte budget; choose a narrower base.');
2929
return data;
3030
};
31-
// Inherited Git variables can redirect repository, index, config, and object lookup.
32-
const environment = Object.fromEntries(Object.entries(process.env).filter(([key]) => !/^GIT_/i.test(key)));
3331
const run = (...args: string[]) => {
3432
const timeout = remaining();
3533
let result: Buffer;
3634
try {
3735
result = execFileSync('git', ['--no-pager', '--no-replace-objects', '-c', 'core.hooksPath=/dev/null', '-c', 'protocol.allow=never', ...args], {
3836
cwd: repo, maxBuffer: 32 * 1024 * 1024, timeout, killSignal: 'SIGKILL',
39-
env: { ...environment, GIT_OPTIONAL_LOCKS: '0', GIT_TERMINAL_PROMPT: '0',
37+
// Allowlisted: inherited variables can redirect repository, index, config and object lookup, and Git reads
38+
// variables outside the GIT_ namespace too, such as HOME and XDG_CONFIG_HOME for user attributes.
39+
env: { PATH: process.env.PATH ?? '', GIT_OPTIONAL_LOCKS: '0', GIT_TERMINAL_PROMPT: '0',
4040
GIT_NO_LAZY_FETCH: '1', GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', GIT_GRAFT_FILE: '/dev/null' },
4141
stdio: ['ignore', 'pipe', 'pipe'],
4242
});

‎test/history.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,15 @@ it('Git built-ins cannot be overridden by repository shell aliases', () => {
168168
expect(f.segments().length).toBeGreaterThan(0);
169169
expect(existsSync(join(f.dir, 'alias-executed'))).toBe(false);
170170
});
171+
it('ignores inherited user Git attributes outside the GIT_ namespace', () => {
172+
const f = fixture(); f.write('a.txt', 'ONE\ntwo\nthree\n'); f.commit('P1');
173+
const read = () => readHistory(f.dir, f.base).final.map(file => file.contexts);
174+
const expected = read(); expect(expected.flat()).not.toHaveLength(0);
175+
const home = mkdtempSync(join(tmpdir(), 'codeboost-history-home-')); dirs.push(home);
176+
mkdirSync(join(home, 'git')); writeFileSync(join(home, 'git', 'attributes'), '* -diff\n');
177+
vi.stubEnv('XDG_CONFIG_HOME', home); vi.stubEnv('HOME', home);
178+
try { expect(read()).toEqual(expected); } finally { vi.unstubAllEnvs(); }
179+
});
171180
it('isolates repository selection from inherited Git environment variables', () => {
172181
const expected = fixture(); expected.write('a.txt', 'expected repo\n'); expected.commit('P1');
173182
const foreign = fixture(); foreign.write('a.txt', 'foreign repo\n'); foreign.commit();

0 commit comments

Comments
 (0)