Skip to content

Commit d0f0ad6

Browse files
mpercyclaude
andauthored
fix(export): scrub git hook env and skip cross-worktree git-add (GH#3311) (gastownhall#3347)
When export.git-add=true (the v1.0.1+ default), the pre-commit hook in a worktree stages a spurious issues.jsonl at the repo root instead of (or in addition to) .beads/issues.jsonl. The ghost file lands in the commit's diff but not on disk — easy to miss and painful to clean up. Root cause (not what the issue body guessed): the absolute path passed to `git add` was always correct. The bug is env inheritance. Git invokes the pre-commit hook with GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE/ GIT_COMMON_DIR/GIT_PREFIX set in the environment, and bd's git-add subprocesses inherit them. Both call sites set `cmd.Dir = filepath.Dir(fullPath)` (= `.beads/`). With GIT_DIR set in the environment and GIT_WORK_TREE unset, git defaults the work-tree to cwd. So the subprocess's git sees `.beads/` as the work-tree root and records `.beads/issues.jsonl` at the root of that broken view — which ends up in the worktree's actual index as a bare `issues.jsonl`. Verified empirically: with GIT_DIR set and cwd=`.beads/`, `git rev-parse --show-toplevel` returns the `.beads/` directory itself. Fix, in two parts: 1. scrubGitHookEnv() strips the vars that poison repo/worktree auto-discovery, index routing, object routing, and config injection: - discovery: GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR, GIT_PREFIX, GIT_CEILING_DIRECTORIES, GIT_DISCOVERY_ACROSS_FILESYSTEM - index: GIT_INDEX_FILE - objects: GIT_OBJECT_DIRECTORY, GIT_ALTERNATE_OBJECT_DIRECTORIES - config: the whole GIT_CONFIG* family (covers the case where the parent invoked `git -c core.worktree=… commit`, which sets GIT_CONFIG_PARAMETERS / GIT_CONFIG_COUNT in the child env) With these cleared, git rediscovers the repo/worktree from cwd. 2. hookWorkTreeRoot() reads the inherited GIT_DIR to determine the worktree whose hook is currently firing. If the target path is outside that worktree, gitAddFile returns without staging. This prevents a narrower but real regression that would otherwise land after part 1 alone: in the .beads/redirect configuration, fullPath points into the main repo (not the worktree), and post-scrub git would silently stage the file into main's index from a worktree hook. The existing preCommitHookBody template (init_git_hooks.go) documents the same intent: "For worktrees: .beads is in the main repo's working tree... we skip it." The guard uses pathInsideDir which handles the macOS symlinked-parent case: on /tmp → /private/tmp, a fresh-file target expressed as /tmp/.../new.txt compared against a worktree root resolved to /private/tmp/... would otherwise misclassify as outside the worktree. pathInsideDir resolves the parent of each side via EvalSymlinks and reattaches the basename — sufficient for the real caller shapes, since gitAddFile's parent directory (beadsDir or the export output dir) always exists at call time. Also fixes the variant reported in a follow-up comment where the .beads/redirect workaround did not resolve the root ghost: the env pollution broke git-add inside the `bd export -o` subprocess spawned from exportJSONLForCommit (that subprocess has BD_GIT_HOOK stripped, but git env vars preserved, so its PostRun auto-export hits the same bug via the shared gitAddFile). Deduplicates the parallel git-add invocation in exportJSONLForCommit by delegating to gitAddFile, so both call sites share the env scrub and the cross-worktree guard. Adds: - TestGitAddFile_InWorktreeHook_StagesCorrectPath — end-to-end worktree regression test; fails on main, passes with the fix. - TestGitAddFile_RedirectCase_DoesNotStageInMainRepo — verifies the cross-worktree skip does not pollute main's index when a worktree has .beads/redirect pointing into main. - TestGitAddFile_NonHookContext_GuardDoesNotFire — regression guard confirming the cross-worktree skip is a no-op outside git hooks. - TestScrubGitHookEnv — unit test for the env scrub helper, covering the discovery/index/object vars and the GIT_CONFIG* family, and verifying non-discovery vars (author/committer/editor/pager) pass through untouched. - TestPathInsideDir — covers structural cases plus the macOS fresh-file + symlinked-parent regression. - TestHookWorkTreeRoot — covers linked-worktree gitdir file, plain-repo .git, bare/unrecognized, and not-a-hook contexts. Workaround for users on older releases remains `BD_EXPORT_GIT_ADD=false`. Closes GH#3311 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 0dce51a commit d0f0ad6

3 files changed

Lines changed: 616 additions & 5 deletions

File tree

cmd/bd/export_auto.go

Lines changed: 129 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,9 +272,137 @@ func saveExportAutoState(beadsDir string, state *exportAutoState) {
272272
}
273273
}
274274

275-
// gitAddFile stages a file in the enclosing git repo.
275+
// gitAddFile stages a file in the enclosing git repo. When called from
276+
// inside a git hook, it scrubs inherited GIT_* env vars (so git
277+
// rediscovers the repo from cwd rather than treating cmd.Dir as the
278+
// worktree root) and skips staging when the target is outside the hook's
279+
// worktree (the .beads/redirect case, where staging would pollute the
280+
// main repo's index). See GH#3311, scrubGitHookEnv, hookWorkTreeRoot.
276281
func gitAddFile(path string) error {
282+
if wt := hookWorkTreeRoot(); wt != "" && !pathInsideDir(path, wt) {
283+
// Running inside a hook AND target is outside the hook's worktree.
284+
// Staging here would pollute a different repo's index; skip.
285+
return nil
286+
}
277287
cmd := exec.Command("git", "add", path)
278288
cmd.Dir = filepath.Dir(path)
289+
cmd.Env = scrubGitHookEnv(os.Environ())
279290
return cmd.Run()
280291
}
292+
293+
// scrubGitHookEnv returns env with the GIT_* variables that can poison
294+
// git's repo/worktree auto-discovery or object-store resolution removed,
295+
// so git falls back to auto-discovery from cwd. The scrub is
296+
// unconditional: if a user has intentionally exported any of these vars
297+
// for scripting purposes, they will be stripped from the git-add child
298+
// process. That is the correct trade-off here; we never want beads'
299+
// auto-stage to honor a GIT_DIR pointing at an unrelated repo.
300+
//
301+
// Covered vars:
302+
// - Repo/worktree discovery: GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR,
303+
// GIT_PREFIX, GIT_CEILING_DIRECTORIES, GIT_DISCOVERY_ACROSS_FILESYSTEM
304+
// - Index routing: GIT_INDEX_FILE
305+
// - Object routing: GIT_OBJECT_DIRECTORY, GIT_ALTERNATE_OBJECT_DIRECTORIES
306+
// - Config injection (any GIT_CONFIG* — e.g. GIT_CONFIG_PARAMETERS set
307+
// when the parent ran `git -c core.worktree=… commit`): the whole
308+
// GIT_CONFIG namespace, which includes _COUNT, _KEY_n, _VALUE_n,
309+
// _GLOBAL, _SYSTEM, _NOSYSTEM, and the legacy GIT_CONFIG itself.
310+
func scrubGitHookEnv(env []string) []string {
311+
// The GIT_CONFIG prefix (no trailing "=") is intentional: it matches
312+
// GIT_CONFIG=, GIT_CONFIG_COUNT=, GIT_CONFIG_KEY_n=, GIT_CONFIG_VALUE_n=,
313+
// GIT_CONFIG_PARAMETERS=, GIT_CONFIG_GLOBAL=, GIT_CONFIG_SYSTEM=, and
314+
// GIT_CONFIG_NOSYSTEM= — the whole family — in one entry. No standard
315+
// git env var starts with GIT_CONFIG that we want to preserve.
316+
prefixes := []string{
317+
"GIT_DIR=",
318+
"GIT_WORK_TREE=",
319+
"GIT_INDEX_FILE=",
320+
"GIT_COMMON_DIR=",
321+
"GIT_PREFIX=",
322+
"GIT_OBJECT_DIRECTORY=",
323+
"GIT_ALTERNATE_OBJECT_DIRECTORIES=",
324+
"GIT_CEILING_DIRECTORIES=",
325+
"GIT_DISCOVERY_ACROSS_FILESYSTEM=",
326+
"GIT_CONFIG",
327+
}
328+
out := make([]string, 0, len(env))
329+
for _, e := range env {
330+
skip := false
331+
for _, p := range prefixes {
332+
if strings.HasPrefix(e, p) {
333+
skip = true
334+
break
335+
}
336+
}
337+
if !skip {
338+
out = append(out, e)
339+
}
340+
}
341+
return out
342+
}
343+
344+
// hookWorkTreeRoot returns the root of the worktree whose git hook we
345+
// are running inside, based on the inherited GIT_DIR env var. Returns ""
346+
// when GIT_DIR is not set (the normal non-hook case) or cannot be
347+
// resolved to a work-tree.
348+
//
349+
// Resolution rules:
350+
// - In a linked worktree, GIT_DIR points at main/.git/worktrees/<name>
351+
// and that directory contains a "gitdir" file whose contents are the
352+
// absolute path to the worktree's .git FILE. The worktree root is
353+
// the parent of that .git file.
354+
// - In a non-worktree, GIT_DIR is typically ".git" or "<repo>/.git";
355+
// the worktree root is its parent.
356+
func hookWorkTreeRoot() string {
357+
gitDir := os.Getenv("GIT_DIR")
358+
if gitDir == "" {
359+
return ""
360+
}
361+
var root string
362+
if data, err := os.ReadFile(filepath.Join(gitDir, "gitdir")); err == nil {
363+
if dotGit := strings.TrimSpace(string(data)); dotGit != "" {
364+
root = filepath.Dir(dotGit)
365+
}
366+
}
367+
if root == "" && filepath.Base(gitDir) == ".git" {
368+
root = filepath.Dir(gitDir)
369+
}
370+
if root == "" {
371+
return ""
372+
}
373+
abs, err := filepath.Abs(root)
374+
if err != nil {
375+
return ""
376+
}
377+
return abs
378+
}
379+
380+
// pathInsideDir reports whether path is the same as dir or a descendant
381+
// of dir, after resolving symlinks on both sides. Returns false on any
382+
// resolution error (conservative: when in doubt, treat as outside).
383+
//
384+
// Resolves the PARENT of path rather than path itself, which handles the
385+
// common "target file does not yet exist" case: on macOS /tmp is a
386+
// symlink to /private/tmp, so asymmetric EvalSymlinks on a nonexistent
387+
// file vs its existing parent would otherwise produce a spurious false.
388+
// Callers (gitAddFile) always pass a path whose parent exists (either
389+
// beadsDir, which FindBeadsDir verified, or a directory just created by
390+
// the export write), so this single-level resolution is sufficient.
391+
func pathInsideDir(path, dir string) bool {
392+
absPath, err := filepath.Abs(path)
393+
if err != nil {
394+
return false
395+
}
396+
absDir, err := filepath.Abs(dir)
397+
if err != nil {
398+
return false
399+
}
400+
if r, err := filepath.EvalSymlinks(filepath.Dir(absPath)); err == nil {
401+
absPath = filepath.Join(r, filepath.Base(absPath))
402+
}
403+
if r, err := filepath.EvalSymlinks(absDir); err == nil {
404+
absDir = r
405+
}
406+
sep := string(filepath.Separator)
407+
return absPath == absDir || strings.HasPrefix(absPath, absDir+sep)
408+
}

0 commit comments

Comments
 (0)