Skip to content

pre-commit hook fails the test suite: git exports GIT_DIR into hooks, which overrides git -C in tests/test_pipeline.c #2003

Closed

Description

@metehanulusoy

What happens

CONTRIBUTING.md tells contributors to install the repo hooks (git config core.hooksPath scripts/hooks). The pre-commit hook runs the full test suite. Git exports GIT_DIR into hook environments, GIT_DIR overrides git -C, and the tests in tests/test_pipeline.c that shell out to git therefore operate on the real repository instead of their temp fixture.

The result: with the documented hook installed, git commit fails the suite every time.

Reproduction — no hook needed, one variable

$ scripts/test.sh --suites pipeline
  264 passed

$ GIT_DIR="$PWD/.git" scripts/test.sh --suites pipeline
  git_context_linked_worktree   FAIL tests/test_pipeline.c:7444: run_cmd(cmd) == 32768, expected 0 == 0
  261 passed, 3 failed

Verified on main with no local changes (3627eff), macOS 15 arm64, git 2.50.1.

Line 7444 is:

snprintf(cmd, sizeof(cmd), "git -C \"%s\" checkout -b main >%s 2>&1", repo, null_dev);
ASSERT_EQ(run_cmd(cmd), 0);

With GIT_DIR set, -C no longer selects the repository: the command runs against the real checkout, where main already exists, so git exits 128.

It is not only a failing assert — the suite writes to the real repository

While tracing this I ran the test's own worktree add line with GIT_DIR set:

$ GIT_DIR=/path/to/codebase-memory-mcp/.git \
    git -C "$TMP/repo with space" worktree add -b feat2 "$TMP/wt with space"
Preparing worktree (new branch 'feat2')
Updating files: 100% (2118/2118), done.
HEAD is now at 17786374 Merge pull request #1993 from DeusData/fix/unify-type-short-name-index

It checked out 2118 files of the actual repository into the temp path and created a branch in the contributor's clone. So a contributor who follows the documented setup gets branches and worktree registrations created in their working repo by the test suite. git worktree prune plus git branch -D cleans it up, but nothing warns them it happened.

Why it is easy to miss

canonical_root_linked_worktree guards itself and reports SKIP (platform: git worktree add unavailable (git 2.5+ required)) under the same conditions — so the environment problem is rendered as a platform capability message rather than as the env-var collision it is. git_context_linked_worktree has no such guard and fails outright.

Suggested fix

Clear the git-supplied environment at the top of the hook — or in scripts/test.sh, so the suite is immune however it is invoked:

unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_PREFIX GIT_OBJECT_DIRECTORY

Belt-and-braces alternative for the tests themselves: build the git commands as env -u GIT_DIR -u GIT_WORK_TREE git -C ..., so a stray GIT_DIR in any caller's environment can never redirect a test at the real repository. git_available() at tests/test_pipeline.c:7380 is the natural place to also assert a clean env once, rather than each site defending itself.

I am happy to open a PR for whichever of the two you prefer — the hook/test.sh unset is the one-line version, the env -u wrapper is the one that holds no matter who invokes the suite. Flagging first per CONTRIBUTING, since this touches build/CI configuration.

Environment

  • main @ 3627eff, built from source with scripts/build.sh
  • macOS 15 (Darwin 25.6.0), arm64, clang, git 2.50.1

Activity

  1. added
    bugSomething isn't working
    securitySecurity vulnerabilities, hardening
    priority/highNeeds near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.
    on Sep 3, 2026
  2. AmirF194 commented on Sep 4, 2026

    @AmirF194
    Contributor

    Reproduced this on a fresh HEAD clone (8f64c36). Plain ./build/c/test-runner pipeline passes, 273 passed. With GIT_DIR=$PWD/.git set the same binary doesn't fail fast though, it just doesn't finish: 18+ minutes, sustained ~150% CPU, one early burst of block I/O (about 1.1GB) then flat, no output, so I killed it.

    My clone is a full unshallowed history, which is probably why: once -C <fixture> stops scoping the git calls, checkout/worktree add end up working against the real tree instead of a couple of files, and on a repo this size that's minutes of work instead of an instant exit 128. So the failure mode isn't only the fast assert you saw, it can also be a plain hang, size-dependent on whoever's clone the hook runs in.

    Didn't try either of your two fixes, just wanted to flag the hang before you pick one.

  3. DeusData commented on Sep 25, 2026

    @DeusData
    Owner

    Thank you @metehanulusoy for the careful report. Tracing it to GIT_DIR overriding git -C, and showing that the suite actually writes branches and worktrees into the contributor's clone, made this quick to pin down. Thanks also @AmirF194 for reproducing it and spotting the hang on a full clone.

    The hook half was already covered: the pre-commit hook clears git rev-parse --local-env-vars since 7883507 (thanks @bmcnaboe). On top of that we went with your belt-and-braces option in #2337: the test runner and the repro runner now clear git's repository-local variables at startup, so the suite is safe however it's started. A new contract test runs the git fixtures with GIT_DIR (and the full hook set) aimed at a decoy repo, and checks that results match a clean run and the decoy stays untouched. That also catches the quieter failure you noted, where the environment problem showed up as "git not available" skips.

    The same variables affected cbm itself when it runs from a hook or an editor that exports GIT_DIR, so #2341 gives every git child cbm spawns its own environment with exactly those variables removed.

  4. added 2 commits that reference this issue on Sep 30, 2026
  5. DeusData commented on Sep 30, 2026

    @DeusData
    Owner

    Thank you again, @metehanulusoy! #2341 is now merged to main as 94097f0 and covers part of this issue. We're keeping the issue open for the remaining work and will post here as it lands.

  6. DeusData commented on Oct 2, 2026

    @DeusData
    Owner

    Thank you again, @metehanulusoy, and @AmirF194 for the reproduction! #2337 is now merged to main as 96c3f41. The test runner and the repro runner clear the repository-local git variables at startup, and the new contract test runs the git fixtures with GIT_DIR pointed at a decoy repository to keep it that way. Together with #2341 this closes both halves of the report.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority/highNeeds near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.securitySecurity vulnerabilities, hardening

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions