feat(terraform_validate): Add support for running hook in git worktree#993
Merged
MaxymVlasov merged 9 commits intoJun 18, 2026
Merged
Conversation
Hooks that invoke `tofu init` / `terraform init` (terraform_validate,
terraform_tflint, terraform_docs) end up running `git clone <module>`
under the hood for each registry/git-source module. When the
operator's `git commit` runs from a linked worktree, the parent git
sets GIT_INDEX_FILE (pointing at `.git/worktrees/<name>/index`) in the
hook subprocess env. The child `git clone` inherits this and writes the
cloned module's blob OIDs into the parent worktree's index. The next
`git commit` then fails at tree-build:
error: invalid object 100644 <oid> for '<path>'
error: Error building trees
Reproduction:
git init && git commit --allow-empty -m init
mkdir stack
cat > stack/main.tf <<'TF'
module "test" {
source = "terraform-aws-modules/iam/aws"
version = "5.x"
}
TF
cat > .pre-commit-config.yaml <<'YAML'
repos:
- repo: https://github.com/antonbabenko/pre-commit-terraform
rev: v1.106.0
hooks:
- id: terraform_validate
args:
- --hook-config=--retry-once-with-cleanup=true
- --tf-init-args=-backend=false
YAML
git add -A && git commit -m baseline
git worktree add ../wt && cd ../wt
pre-commit install
echo "" >> stack/main.tf
git add stack/main.tf
git commit -m test # fails with 'Error building trees'
git fsck --no-progress # invalid sha1 pointer in cache-tree
Other GIT_* vars (GIT_DIR, GIT_WORK_TREE, GIT_OBJECT_DIRECTORY) are
overridden by `git clone`'s own target-dir setup; only GIT_INDEX_FILE
slips through. Scrubbing all four matches the four documented offenders
in pre-commit framework's own `no_git_env` helper
(`pre_commit/git.py:20-38`), which explicitly states:
# GIT_DIR: Causes git clone to clone wrong thing
# GIT_INDEX_FILE: Causes 'error invalid object ...' during commit
pre-commit framework deliberately does NOT scrub GIT_* from user hook
subprocesses (only its own internal git calls) per the maintainer in
pre-commit/pre-commit#1849: "they need the same code as in our
no_git_env helper if they are dealing with doing git writes". This
patch applies that recommendation in pre-commit-terraform's hooks
that perform git writes via `tofu init` / `terraform init`.
Verified by reproducing the failure on v1.106.0 from a worktree with
a module-using stack, then confirming the fix removes both the index
pollution and the tree-build error. SSH/TLS/config-related GIT_* env
vars are preserved (only the four documented "dangerous" ones are
unset) so SSH-source modules authenticate normally.
Net change: one new helper `common::scrub_git_env` in _common.sh,
called once from `main()` of each affected hook. Zero behavior change
for non-worktree users — their GIT_INDEX_FILE is empty for these hooks
so the unset is a no-op.
Refs: antonbabenko#992
yuriipolishchuk
requested review from
MaxymVlasov,
antonbabenko and
yermulnik
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesGit environment scrubbing across hooks
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hooks/_common.sh`:
- Around line 119-149: The comment above function common::scrub_git_env is
misleading because the function only unsets a targeted subset of GIT_* vars
(GIT_INDEX_FILE, GIT_DIR, GIT_WORK_TREE, GIT_OBJECT_DIRECTORY) rather than
reproducing pre-commit's no_git_env behavior; update the docblock to explicitly
state that this function intentionally clears only those specific offending
variables (and why each is included) and clarify that pre-commit's no_git_env
removes all GIT_* except a specific allowlist (e.g., GIT_CONFIG_KEY_*,
GIT_CONFIG_VALUE_*, GIT_EXEC_PATH, GIT_SSH, GIT_SSH_COMMAND, GIT_SSL_CAINFO,
GIT_SSL_NO_VERIFY, GIT_CONFIG_COUNT, GIT_HTTP_PROXY_AUTHMETHOD,
GIT_ALLOW_PROTOCOL, GIT_ASKPASS), so the comment does not claim parity with
no_git_env and instead documents the targeted subset behavior of
common::scrub_git_env.
🪄 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: ee9f744b-e72b-42de-9342-5072872f64a8
📒 Files selected for processing (4)
hooks/_common.shhooks/terraform_docs.shhooks/terraform_tflint.shhooks/terraform_validate.sh
Address CodeRabbit feedback on PR antonbabenko#993: the previous docstring read as though common::scrub_git_env mirrored pre-commit's no_git_env helper. It does not. no_git_env is allowlist-based: it scrubs all GIT_* except a whitelist of ~11 known-safe vars (GIT_EXEC_PATH, GIT_SSH, GIT_SSH_COMMAND, GIT_ASKPASS, GIT_SSL_*, GIT_CONFIG_KEY_*, GIT_CONFIG_VALUE_*, GIT_CONFIG_COUNT, GIT_HTTP_PROXY_AUTHMETHOD, GIT_ALLOW_PROTOCOL) and runs only on pre-commit's internal git calls. common::scrub_git_env is denylist-based: it unsets only four specific vars that pre_commit/git.py:20-38 documents as dangerous when leaked into child git (GIT_INDEX_FILE, GIT_DIR, GIT_WORK_TREE, GIT_OBJECT_DIRECTORY), and runs in user hook subprocesses. New docstring spells this out, explains why each of the four is included, and notes why we chose a denylist over an allowlist mirror (bash portability, observed bug fix scope, lower blast radius for hook authors with custom registry/proxy env vars).
yuriipolishchuk
changed the title
yermulnik
reviewed
Jun 11, 2026
MaxymVlasov
requested changes
Jun 11, 2026
| common::parse_cmdline "$@" | ||
| common::export_provided_env_vars "${ENV_VARS[@]}" | ||
| common::parse_and_export_env_vars | ||
| common::scrub_git_env |
Collaborator
There was a problem hiding this comment.
I can't reproduce the worktree bug in this hook.
Suggested change
| common::scrub_git_env |
| common::parse_cmdline "$@" | ||
| common::export_provided_env_vars "${ENV_VARS[@]}" | ||
| common::parse_and_export_env_vars | ||
| common::scrub_git_env |
Collaborator
There was a problem hiding this comment.
I can't reproduce the worktree bug in this hook.
Suggested change
| common::scrub_git_env |
Collaborator
There was a problem hiding this comment.
There another thing that shows, both for terraform_docs and terraform_fmt hooks.
But it's not an issue at all.
18:57 repro-hoo git:(repro-hoo +)
➜ gc
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Terraform fmt............................................................Failed
- hook id: terraform_fmt
- files were modified by this hook
main.tf
18:57 repro-hoo git:(repro-hoo !+)
✘1 ➜ gc
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Terraform fmt............................................................Passed
Terraform docs.......................................(no files to check)Skipped
Terraform validate with tflint.......................(no files to check)Skipped
Terraform fmt........................................(no files to check)Skipped
[repro-hoo 44cd287] fmt
2 files changed, 2 insertions(+), 6 deletions(-)
18:58 repro-hoo git:(repro-hoo) took 9s
➜ git fsck --no-progress | head
dangling tree bba236455e65937967031c5d25b02e11f5c252f5
18:58 repro-hoo git:(repro-hoo)
➜ git fsck --no-progress | head
dangling tree bba236455e65937967031c5d25b02e11f5c252f5
18:58 repro-hoo git:(repro-hoo)
➜ gc
Terraform docs...........................................................Failed
- hook id: terraform_docs
- files were modified by this hook
Terraform validate with tflint...........................................Passed
Terraform fmt............................................................Passed
18:58 repro-hoo git:(repro-hoo !+)
✘1 ➜ gc
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Terraform fmt............................................................Passed
Terraform docs.......................................(no files to check)Skipped
Terraform validate with tflint.......................(no files to check)Skipped
Terraform fmt........................................(no files to check)Skipped
[repro-hoo c115ce4] docs
2 files changed, 2 insertions(+), 2 deletions(-)
18:58 repro-hoo git:(repro-hoo) took 6s
➜ git fsck --no-progress | head
dangling tree a13f3782d35b8ce91a9d45f5b57acd390ea671a2
dangling tree bba236455e65937967031c5d25b02e11f5c252f5Per review on PR antonbabenko#993: - MaxymVlasov could not reproduce the worktree bug in terraform_tflint or terraform_docs (terraform-docs does not run init), so the common::scrub_git_env calls are removed from both. The confirmed reproduction path is terraform_validate -> tofu/terraform init -> git clone, which keeps its call. - Docstring reworded per yermulnik: drop 'dangerous' (the vars are incompatible with pre-commit's hook env, not dangerous), drop the var count, capitalize Git as a proper name, use full clickable URLs instead of repo-relative paths and issue shorthand, drop line-range references that rot as upstream moves. - GIT_OBJECT_DIRECTORY provenance corrected: it is not named in pre-commit's no_git_env docstring; it comes from Git's own repository-location environment variables (man 1 git). The docstring now cites that source for all four vars. - unset arguments alphabetized.
Contributor
Author
|
Thanks both for the thorough review - pushed 63ab2bc addressing it. Scope (@MaxymVlasov)Accepted both suggestions: The dangling trees you observed with Wording / references (@yermulnik)All applied in 63ab2bc:
Open design questions - deferring to you two
|
Contributor
Author
|
@MaxymVlasov @yermulnik gentle nudge - whenever you have a moment for a re-review. Recap of what changed since the review round, all in
I also left replies on the open design questions (central |
yermulnik
previously approved these changes
Jun 16, 2026
Collaborator
There was a problem hiding this comment.
@yuriipolishchuk Thanks for a nudge. The code looks good to me. However I'd defer the final review/approval to @MaxymVlasov as main dev.
UPD: since the PR targets Git worktrees only, I'd prefer the worktree gating there so that the code changes user env only where the problem exists rather than everywhere. @MaxymVlasov What's your view on this in particular and the PR overall?
Failure scenario, root-cause deep-dive, and no_git_env allowlist enumeration belong in git history, not in source. Keep only: - what the function does and why each var is included - the pre-commit framework design constraint + issue link - the denylist-vs-allowlist note Assisted-by: Sisyphus:claude-sonnet-4-6 opencode
Assisted-by: Sisyphus:claude-sonnet-4-6 opencode
MaxymVlasov
changed the title
terraform_validate): Add support for running hooks in git worktree
MaxymVlasov
changed the title
terraform_validate): Add support for running hooks in git worktreeterraform_validate): Add support for running hook in git worktree
MaxymVlasov
previously approved these changes
Jun 18, 2026
yermulnik
previously approved these changes
Jun 18, 2026
Collaborator
There was a problem hiding this comment.
@MaxymVlasov Code LGTM.
The two points which I'm concerned about, however I might be over-engineering these. @MaxymVlasov WDYT?
- Handle
unsetfailure? E.g. if by any reason the var was declared readonly outside of the hook? 🤔 Theunset … || trueshould be sufficient (not suppressing error output to let user know what's happening wrongly) - I'd also suggest to unset Git vars only when the hook is run from inside the Git worktree so that we don't mess up non-worktree envs.
- Here's how I identify this locally:
local _git_repo_git_file="$(git rev-parse --show-cdup).git" [[ -f $_git_repo_git_file ]] && $(grep -qE ^gitdir: "$_git_repo_git_file") && local _is_git_worktree="true"
MaxymVlasov
previously approved these changes
Jun 18, 2026
…vars Assisted-by: Sisyphus:claude-sonnet-4-6 opencode
yermulnik
reviewed
Jun 18, 2026
| # GIT_WORK_TREE pairs with GIT_DIR | ||
| ####################################################################### | ||
| function common::scrub_git_env { | ||
| local -r git_env_vars=( |
Collaborator
There was a problem hiding this comment.
Why readonly? And why not also -a to explicitly declare array type?
Collaborator
There was a problem hiding this comment.
As I don't expect that this array will be changed
…ience Assisted-by: Sisyphus:claude-sonnet-4-6 opencode
Assisted-by: Sisyphus:claude-sonnet-4-6 opencode
yermulnik
approved these changes
Jun 18, 2026
antonbabenko
pushed a commit
that referenced
this pull request
Jun 18, 2026
# [1.107.0](v1.106.0...v1.107.0) (2026-06-18) ### Features * **`terraform_validate`:** Add support for running hook in `git worktree` ([#993](#993)) ([3fba9d7](3fba9d7))
Owner
|
This PR is included in version 1.107.0 🎉 |
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
4 participants
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.
Description of your changes
Fixes #992.
What
This section was generated by AI.
common::scrub_git_envhelper inhooks/_common.sh: unsets 4 GIT_* vars that leak the parent repo location into child Git processeshooks/terraform_validate.sh: one-line call inmain()after env setup, beforecommon::per_dir_hookunsetis a no-op when the var is absentWhy
This section was generated by AI.
Hooks that invoke
tofu init/terraform init(terraform_validate) end up runninggit clone <module>for each registry/git-source module. Whengit commitruns from a linked worktree, the parent git setsGIT_INDEX_FILE(pointing at.git/worktrees/<name>/index) in the hook subprocess env. The childgit cloneinherits this and writes cloned module blob OIDs into the parent worktree's index. The next commit fails at tree-build:pre-commit framework deliberately does NOT scrub
GIT_*from user hook subprocesses (only its own internal git calls) per the maintainer in pre-commit/pre-commit#1849:asottile reaffirmed this in pre-commit/pre-commit#3492 — framework-level scrubbing breaks valid use cases (e.g., gitleaks-docker wants
GIT_INDEX_FILEpassed through). Per-hook-author opt-in is the design intent. This patch applies that recommendation.This is a targeted denylist (4 vars), NOT a mirror of pre-commit's allowlist-based
no_git_envhelper. Only the vars that leak the parent repository's location into child Git processes are unset; SSH/TLS/config-relatedGIT_*vars are preserved.How can we test changes
This section was generated by AI.
Same
.pre-commit-config.yamlshape, same staged.tfchange, samegit commitinvocation. Switchrepo:betweenantonbabenko/pre-commit-terraform@v1.106.0andyuriipolishchuk/pre-commit-terraform@fix/scrub-git-env:rev: v1.106.0Error building trees. 5 phantom blobs in worktree index.git fsckreports invalid sha1 in cache-tree.rev: fix/scrub-git-envgit fsckclean.Terraform validate: Passed.Full reproduction script
master(v1.106.0) from a worktree with a module-using stack → commit fails withError building trees, index has phantom blobsunsetis a no-op when env var absentGIT_SSH,GIT_ASKPASS,GIT_SSL_*etc. are NOT touchedshellcheckclean (no new warnings)Other notes
This section was generated by AI.
no_git_env(~15 linescaseblock in bash) but went with the minimal denylist since onlyGIT_INDEX_FILEactually causes the observed failure; the others are included as documented defensive offenders. Happy to expand to allowlist if preferred.git commitenv is unaffected.terraform_fmt,terraform_trivy,terraform_checkov, etc.) are not touched.References
pre_commit/git.py:20-38—no_git_envsource + docstringAssisted-by
Per AI_POLICY.md: the patch, docstring, and PR text were drafted with LLM assistance (Claude Code). The bug was hit, root-caused, and reproduced in my real infrastructure repo; every revision here was A/B-tested by me against a worktree reproduction before pushing (unpatched fails at tree-build, patched commits cleanly), including re-verification after scoping down to terraform_validate in 63ab2bc.