Skip to content

fix(gitexec): isolate git commands from hook env#27

Open
haryelramalho wants to merge 3 commits into
compozy:mainfrom
haryelramalho:fix/linked-worktree-hook-env
Open

fix(gitexec): isolate git commands from hook env#27
haryelramalho wants to merge 3 commits into
compozy:mainfrom
haryelramalho:fix/linked-worktree-hook-env

Conversation

@haryelramalho
Copy link
Copy Markdown

@haryelramalho haryelramalho commented May 23, 2026

Summary

  • Clear repository-local Git environment variables before running managed Git commands through the command runner.
  • Preserve sidecar ref resolution when skeeper internal pre-commit runs from a linked Git worktree hook.
  • Add regression coverage for inherited linked-worktree hook environment and a real pre-commit flow in a linked worktree.

Root Cause

Git hooks in linked worktrees export repository-local environment such as GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, and GIT_PREFIX. Skeeper child Git commands run with cmd.Dir set 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=1 reproduced the failure before the fix and passes after it.
  • go test ./test/e2e -run TestSkeeperPreCommitHookWorksInLinkedWorktree -count=1 passes.
  • go test ./internal/gitexec ./internal/sidecar ./test/e2e -count=1 passes with 71 tests.
  • make verify passes with 0 issues and 216 tests.

Fixes #26

Summary by CodeRabbit

  • Bug Fixes
    • Fixed git operations in linked worktrees being affected by inherited environment variables so repository references resolve correctly.
  • Tests
    • Added unit and end-to-end tests validating git behavior in linked worktrees and the pre-commit hook lifecycle, ensuring sidecar files update correctly when specs change.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 23, 2026

Walkthrough

ExecRunner 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.

Changes

Linked Worktree Environment Isolation

Layer / File(s) Summary
Git repository environment filtering
internal/gitexec/runner.go
Adds gitRepositoryEnv denylist and helpers isGitCommand and withoutGitRepositoryEnv to detect git commands and remove denylisted variables (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_PREFIX) from the execution environment in RunWithInput.
Unit test for environment filtering
internal/gitexec/git_test.go
TestExecRunnerGitIgnoresInheritedLinkedWorktreeEnv sets up a main repo with linked worktree, injects hook-local git env vars, runs git rev-parse in sidecar, and asserts the sidecar ref resolves correctly despite inherited variables.
End-to-end test for linked worktree hooks
test/e2e/skeeper_lifecycle_test.go
TestSkeeperPreCommitHookWorksInLinkedWorktree verifies skeeper pre-commit hook behavior in a linked worktree: creates a linked worktree, writes and updates SPEC.md, and asserts sidecar branch files reflect both versions correctly.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(gitexec): isolate git commands from hook env' directly and concisely describes the main change: isolating git commands from hook environment variables to fix gitexec behavior.
Linked Issues check ✅ Passed The PR fully implements the requirements from issue #26: strips GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, and GIT_PREFIX environment variables from git commands in ExecRunner.Run, includes a unit test for inherited linked-worktree env, and adds an E2E test validating pre-commit hooks work in linked worktrees.
Out of Scope Changes check ✅ Passed All changes are directly aligned with issue #26 requirements: the runner.go modifications isolate git commands, git_test.go adds regression testing for inherited env vars, and the E2E test validates real hook scenarios in linked worktrees.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between a42240f and f5ea636.

📒 Files selected for processing (3)
  • internal/gitexec/git_test.go
  • internal/gitexec/runner.go
  • test/e2e/skeeper_lifecycle_test.go

Comment thread internal/gitexec/git_test.go
Comment thread test/e2e/skeeper_lifecycle_test.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/gitexec/git_test.go (1)

25-25: ⚡ Quick win

Add 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 by TestExecRunnerCommandErrorCapturesExitCode (line 13) and TestGitLocalOperationsWithGoGit (line 61). The test is independent and uses t.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

📥 Commits

Reviewing files that changed from the base of the PR and between f5ea636 and 7a7a125.

📒 Files selected for processing (2)
  • internal/gitexec/git_test.go
  • test/e2e/skeeper_lifecycle_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pre-commit fails in linked git worktrees due to inherited GIT_DIR

1 participant