test: make git isolation structural via test preload - #20
Merged
aorumbayev merged 5 commits intoJul 10, 2026
Conversation
Greptile SummaryThis PR moves git test isolation into a preload and updates the affected tests.
Confidence Score: 4/5The changed flow looks mergeable after hardening the new worktree-root override. The preload-based git isolation is synchronous and covers normal Bun test runs. The worktree-root override still accepts empty or relative values and can create worktrees in unintended locations. No security issue was identified in the changed code. src/git/runner.ts
What T-Rex did
|
| Filename | Overview |
|---|---|
| .githooks/pre-commit | Removes hook-local git environment cleanup in favor of test preload isolation. |
| bunfig.toml | Adds the git isolation preload to Bun test startup. |
| src/git/runner.ts | Adds the test worktree-root override; empty or relative override values need hardening. |
| test/preload/git-isolation.ts | Adds process-wide git environment and config isolation for tests. |
| test/guards/git-isolation.test.ts | Adds behavioral guard coverage for git environment and config isolation. |
| test/git/runner.test.ts | Updates runner tests to use the production git runner under the preload. |
| test/tui/board/commands.test.ts | Rewrites the merge-failure test to use a real conflicting worktree. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/git/runner.ts:90
**Relative Worktree Root**
When `KAGAN_WORKTREE_ROOT` is set to an empty string or a relative path, `taskWorktreePath()` builds a relative worktree directory and `createTaskWorktree()` creates it from the plugin process cwd. A test or deployment that exports this value can place managed git worktrees inside the checkout or another unintended directory instead of the per-user worktree root.
Reviews (1): Last reviewed commit: "test: rename hermeticity wording to git ..." | Re-trigger Greptile
| function taskWorktreePath(mainWorktree: string, slug: string): string { | ||
| return join(homedir(), ".kagan", "worktrees", Bun.hash(mainWorktree).toString(16), slug) | ||
| // KAGAN_WORKTREE_ROOT exists for test isolation (see test/preload/git-isolation.ts). | ||
| const root = process.env.KAGAN_WORKTREE_ROOT ?? join(homedir(), ".kagan", "worktrees") |
There was a problem hiding this comment.
Relative Worktree Root
When KAGAN_WORKTREE_ROOT is set to an empty string or a relative path, taskWorktreePath() builds a relative worktree directory and createTaskWorktree() creates it from the plugin process cwd. A test or deployment that exports this value can place managed git worktrees inside the checkout or another unintended directory instead of the per-user worktree root.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/git/runner.ts
Line: 90
Comment:
**Relative Worktree Root**
When `KAGAN_WORKTREE_ROOT` is set to an empty string or a relative path, `taskWorktreePath()` builds a relative worktree directory and `createTaskWorktree()` creates it from the plugin process cwd. A test or deployment that exports this value can place managed git worktrees inside the checkout or another unintended directory instead of the per-user worktree root.
**Context Used:** AGENTS.md ([source](https://app.greptile.com/kagan/github/kagan-sh/kagan/-/custom-context?memory=88a14340-9a15-4297-925d-3656d144ad2a))
How can I resolve this? If you propose a fix, please make it concise.
aorumbayev
force-pushed
the
fix/test-hermeticity-structural
branch
from
July 10, 2026 08:44
cc13ad1 to
6278686
Compare
…order Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…21) * fix: make update promotion crash-safe and trim speculative machinery Restore interrupted promotions on launch, self-heal stale cache state, surface local update failures in the footer, and remove unused concurrency/path checks. * fix: harden update cleanup against unvalidated deletes and false ready Finding 1: removeStaleMarker only deletes marker.prepared when it is a kagan@<x.y.z> sibling of the current wrapper in the same scope cache; a corrupted marker naming any other path now leaves that path alone. Finding 2: removed dead restoreCurrentFromBackup / interruptedPromotion branch (the host re-downloads latest before the plugin loads, so it can never run), aligned R18.8 and design.md to the real contract, and closed the prepared-dir leak in the matched-marker cleanup path. Finding 3: prepare failure on a ready check now sets broken status and suppresses the ready toast instead of promising a restart that applies nothing; a failed cleanup now returns broken before the network check. * test: skip the host-dedupe pin when the vendored source is absent references/opencode is git-ignored, so CI checkouts never have it.
aorumbayev
added a commit
that referenced
this pull request
Jul 11, 2026
* fix: enforce all verifyx checks and check-only pre-commit Install knip, skott, and jscpd so verifyx all runs every built-in gate, dedupe status resolution and break git/domain layering cycles flagged by the new checks, and make the pre-commit hook check-only so staged content is never rewritten. * test: make git isolation structural via test preload (#20) * test: make git hermeticity structural via preload * test: rename hermeticity wording to git isolation * test: wait for the merge-conflict notice instead of asserting notice order * test: allow slow CI git spawns to deliver the merge-conflict notice * fix: make update promotion crash-safe and trim speculative machinery (#21) * fix: make update promotion crash-safe and trim speculative machinery Restore interrupted promotions on launch, self-heal stale cache state, surface local update failures in the footer, and remove unused concurrency/path checks. * fix: harden update cleanup against unvalidated deletes and false ready Finding 1: removeStaleMarker only deletes marker.prepared when it is a kagan@<x.y.z> sibling of the current wrapper in the same scope cache; a corrupted marker naming any other path now leaves that path alone. Finding 2: removed dead restoreCurrentFromBackup / interruptedPromotion branch (the host re-downloads latest before the plugin loads, so it can never run), aligned R18.8 and design.md to the real contract, and closed the prepared-dir leak in the matched-marker cleanup path. Finding 3: prepare failure on a ready check now sets broken status and suppresses the ready toast instead of promising a restart that applies nothing; a failed cleanup now returns broken before the network check. * test: skip the host-dedupe pin when the vendored source is absent references/opencode is git-ignored, so CI checkouts never have it. --------- * refactor: simplify update marker and cleanup machinery * refactor: merge single-importer modules * refactor: extract duplicated TUI blocks * chore: drop redundant verify overrides * ci: run check workflow on pull requests only * refactor: inline single-use list-editor row wrapper renderListEditorRows only forwarded its five arguments to <ListEditorRows> and had a single caller; render it directly and drop the now-unused JSX import. * test: make merge-dialog command tests deterministic across file order The two "approving …" tests in test/tui/board/commands.test.ts flaked only on CI. Root cause: bun's mock.module is process-global and persistent, and test/tui/session/tasks.test.ts mocks src/git/runner (currentBranch -> "kagan/x") and src/git/merge (mergeTaskBranch -> ok:true). ESM imports hoist above mock.module, so whichever test file loads first wins; on the Linux runner tasks.test.ts loaded first, so the merge-dialog tests saw the leaked mocks (wrong branch, no conflict) and failed. macOS load order hid it locally. - commands.test.ts now declares its own git/runner + git/merge mocks and drives the three merge-dialog tests through reset-per-test vars (currentBranchValue, localBranches, mergeResult), so its values win for its own tests regardless of which file ran first. - tasks.test.ts's git/runner mock was incomplete (missing listLocalBranches and baseBranchFreshness); stub them so binding those exports elsewhere while the mock is active no longer throws "Export not found". - The merge dialog's onSelect returns its handler promise so tests await the real work instead of racing a render/timer. Reproduced the exact leak order locally (bun test tasks.test.ts commands.test.ts) and via a Linux-container full-suite run; green there, commands-only, and reverse. * fix: surface both promotion and restore errors on update rollback promotePreparedUpdate runs in api.lifecycle.onDispose, whose thrown errors the host logs via console.error rather than discarding. Swallowing a failed restore therefore hid it: a lost kagan@latest wrapper logged only as a promotion failure. Rethrow an AggregateError carrying both, and add a test covering the double-failure path (the single-failure restore was already tested).
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.
Summary
test/preload/git-isolation.ts) that scrubs everyGIT_*env var, routes global/system git config to the null device, pointsXDG_CONFIG_HOMEat a temp dir, and setsKAGAN_WORKTREE_ROOTto a temp dir — so every git spawn in the test process is isolated, including src-internalbunGitRunner()calls (the leak vector the old fixture missed).taskWorktreePathinsrc/git/runner.tshonorsKAGAN_WORKTREE_ROOT; product behavior without it is unchanged (homedir()). Tests no longer touch the real~/.kagan/worktrees.test/fixtures/git.ts— the two consumer test files now use the productionbunGitRunner()directly, so the real runner is what gets exercised.test/guards/git-isolation.test.tsfrom a grep scan into behavioral assertions: env is scrubbed, git can't see the user's real global identity/config, and a planted global gitignore underXDG_CONFIG_HOMEproves dotfile isolation.unset GIT_DIR ...compensating block from.githooks/pre-commit(the preload supersedes it).commands.test.tsuse a real conflicting repo + worktree instead of relying on a nonexistent/repopath throwing.HOMEis not redirected — onlyXDG_CONFIG_HOME— because worktree-path tests assert onhomedir()-based defaults and redirectingHOMEprocess-wide would changehomedir()everywhere; the config-home redirect covers git's dotfile reads (~/.config/git/ignore,attributes).Verification
Poisoned environment —
GIT_DIR=/nonexistent GIT_CONFIG_GLOBAL=$HOME/.gitconfig bun run test:Real home untouched —
ls ~/.kagan/worktreessnapshots before/after a full run: identical.git statusandgit worktree listin the repo: unchanged.Guard is a real gate — with the preload removed from
bunfig.toml,bun test test/guards/git-isolation.test.ts --conditions browser:Hook safe without the unset block —
GIT_DIR=.git GIT_INDEX_FILE=.git/index sh .githooks/pre-commitexits 0.Full gate —
bun run verify -- --check --measure: all 12 checks pass, none skipped.Also removed an orphaned
test-hermeticity/worktree stub in the repo root left over from the original test-pollution incident (its parent repo no longer exists).🤖 Generated with Claude Code