Run history Git with an allowlisted environment - #83
Merged
Merged
Conversation
…rives 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>
…erprint 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>
… 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>
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>
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>
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>
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>
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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erywhere # Conflicts: # runner/review.ts # test/fixture-git.test.ts # test/review.test.ts
…lowlist # Conflicts: # test/agent-container.test.ts
This was referenced Sep 29, 2026
mchwang
added a commit
that referenced
this pull request
Sep 30, 2026
* F1e: planning API for lane G and feedback from review actions Planning endpoints over E3's coordinator and the Store: import, suggestion start/read/cancel/apply, each through Store.userAction; E3's settlement writes use the shutdown capability and the coordinator closes at shutdown. Starting a suggestion is refused until a planning provider is injected. ReviewService.planContext builds the trusted plan context from the base tree. Review actions: change notes, segment accept and assign require an actionId and record their feedback event in the same transaction; a later choice links to the earlier event; choice sources are fixed-size fingerprints. The UI sends an actionId per action. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Start no planning agent for a suggestion request handled after shutdown began Ask and the runner stop admission in shutdown step 1, but the suggestion coordinator only closed in step 6. A request admitted before shutdown and handled during the drain allocated a pending request and started a planning invocation, which step 6 then cancelled. It now answers 503, records nothing, and the UI may resend it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Read the trusted plan context with main's hardened Git invocation main (#82, #83) replaced isolatedGitEnvironment with HARDENED_GIT_OPTIONS and hardenedGitEnvironment. planContext() now uses both. That also adds --no-replace-objects: before, a replace ref for the snapshot's base made ls-tree list the replacement's tree as the trusted base entries. A test replaces the base with the head commit and fails on the old invocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Targets
main. It follows #82, which was squash-merged as a63539c.origin/mainis merged in, so the diff shows only this PR's changes.What changes
readHistoryingit/history.tsbuilt its Git environment by removingGIT_*names fromprocess.env. AGENTS.md forbids that: "Name-based scrubbing of an inherited environment is not isolation." The environment is now an allowlist:PATHplus the hardening variables that were already set. This matchesgit/clone.ts.Why it matters
Git reads variables outside the
GIT_namespace.HOMEandXDG_CONFIG_HOMElocate the user'sgit/attributesfile, and Git reads that file even withGIT_CONFIG_GLOBAL=/dev/null. A user-level* -diffmakesgit diff --unified=0print "Binary files differ" and no@@headers.readHistorythen returned no hunk ranges or function names for any file. Built-in diff drivers such asdiff=cppcould also change the function names shown in hunk headers.Tests
test/history.test.ts› "ignores inherited user Git attributes outside the GIT_ namespace". It setsXDG_CONFIG_HOMEto a directory whosegit/attributesholds* -diff, then checks thatreadHistoryreturns the same hunk ranges as without it. It failed before the change ([ [] ]against one range) and passes now.Verification
npx tsc --noEmit: no errors.npx vitest runon the history, review, plant, demo and store test files: 101 of 101 pass.npm run test:browser: 65 of 65 pass.🤖 Generated with Claude Code