fix(gitexec): isolate git commands from hook env#27
Conversation
WalkthroughExecRunner now detects git command execution and strips inherited git repository environment variables from the process environment. A denylist, helper functions for command detection and filtering, and integration into RunWithInput prevent hook-local variables from interfering with sidecar repository operations. Unit and end-to-end tests verify the fix resolves linked worktree pre-commit failures. ChangesLinked Worktree Environment Isolation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/gitexec/git_test.go`:
- Around line 25-56: Wrap the body of
TestExecRunnerGitIgnoresInheritedLinkedWorktreeEnv in a t.Run subtest (e.g.,
t.Run("Should ignore inherited linked worktree env", func(t *testing.T) { ...
})) so the test follows the t.Run("Should...") pattern; keep the existing setup
and assertions intact, placing everything from ctx := context.Background()
through the final checks inside the anonymous func, and ensure any t.* calls use
the inner t to preserve proper subtest reporting (test name should describe the
expected behavior).
In `@test/e2e/skeeper_lifecycle_test.go`:
- Around line 81-115: The test function
TestSkeeperPreCommitHookWorksInLinkedWorktree should wrap its assertions in a
t.Run subtest; modify the body of TestSkeeperPreCommitHookWorksInLinkedWorktree
to call t.Run("Should ...", func(t *testing.T) { ... }) and move the existing
setup, git operations and assertions (all code currently inside
TestSkeeperPreCommitHookWorksInLinkedWorktree) into that inner function,
ensuring the inner func receives the testing.T parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c96f1946-5c25-45ca-9ea3-f8bfc1fe8257
📒 Files selected for processing (3)
internal/gitexec/git_test.gointernal/gitexec/runner.gotest/e2e/skeeper_lifecycle_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/gitexec/git_test.go (1)
25-25: ⚡ Quick winAdd
t.Parallel()to match the pattern of other tests in this file.The test function should call
t.Parallel()to run in parallel with other test functions, consistent with the pattern established byTestExecRunnerCommandErrorCapturesExitCode(line 13) andTestGitLocalOperationsWithGoGit(line 61). The test is independent and usest.TempDir()for isolation, making it safe to parallelize.♻️ Proposed fix
func TestExecRunnerGitIgnoresInheritedLinkedWorktreeEnv(t *testing.T) { + t.Parallel() + t.Run("Should ignore inherited linked worktree env", func(t *testing.T) { ctx := context.Background()As per coding guidelines: "Use
t.Parallel()for independent subtests in Go tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/gitexec/git_test.go` at line 25, The test function TestExecRunnerGitIgnoresInheritedLinkedWorktreeEnv is missing a t.Parallel() call; update the test by adding t.Parallel() as the first action inside TestExecRunnerGitIgnoresInheritedLinkedWorktreeEnv so it runs concurrently with other independent tests (it’s safe since the test uses t.TempDir() for isolation), mirroring the pattern used in TestExecRunnerCommandErrorCapturesExitCode and TestGitLocalOperationsWithGoGit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/gitexec/git_test.go`:
- Line 25: The test function TestExecRunnerGitIgnoresInheritedLinkedWorktreeEnv
is missing a t.Parallel() call; update the test by adding t.Parallel() as the
first action inside TestExecRunnerGitIgnoresInheritedLinkedWorktreeEnv so it
runs concurrently with other independent tests (it’s safe since the test uses
t.TempDir() for isolation), mirroring the pattern used in
TestExecRunnerCommandErrorCapturesExitCode and TestGitLocalOperationsWithGoGit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 386e389b-fdda-4057-90d6-20a1a744547c
📒 Files selected for processing (2)
internal/gitexec/git_test.gotest/e2e/skeeper_lifecycle_test.go
Summary
skeeper internal pre-commitruns from a linked Git worktree hook.Root Cause
Git hooks in linked worktrees export repository-local environment such as
GIT_DIR,GIT_WORK_TREE,GIT_INDEX_FILE, andGIT_PREFIX. Skeeper child Git commands run withcmd.Dirset to.skeeper, but previously inherited those hook variables from the main repository. That could make sidecar commands resolve refs against the main worktree gitdir instead of the sidecar repository.Validation
go test ./internal/gitexec -run TestExecRunnerGitIgnoresInheritedLinkedWorktreeEnv -count=1reproduced the failure before the fix and passes after it.go test ./test/e2e -run TestSkeeperPreCommitHookWorksInLinkedWorktree -count=1passes.go test ./internal/gitexec ./internal/sidecar ./test/e2e -count=1passes with 71 tests.make verifypasses with0 issuesand 216 tests.Fixes #26
Summary by CodeRabbit