feat: Add ability to download specific tool versions used by hooks. TLDR: add --hook-config=--tool-version=1.2.3 for pinning wrapped tool version. Works for all tools (even terraform and tofu) except checkov - #1002
Conversation
Adds openspec/config.yaml plus the propose/explore/apply/update/ sync/archive skills and opsx slash commands, then registers the new skills in the AGENTS.md skill routing table so agents load them automatically. Assisted-by: Sisyphus:claude-sonnet-5 claude
|
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:
📝 WalkthroughWalkthroughHooks now support version-pinned tool resolution, cached installers, and resolved executable paths across Terraform, Terragrunt, and auxiliary tools. Documentation covers version pinning, Renovate configuration, cache persistence, and runtime installer usage. ChangesVersion-aware hook execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Hook as Hook
participant Common as common::per_dir_hook
participant Resolver as common::resolve_tool_path
participant Tool as Resolved tool
Hook->>Common: pass tool_name and HOOK_CONFIG
Common->>Resolver: resolve version and executable
Resolver-->>Common: return tool_path
Common->>Tool: execute per-directory or whole-repository command
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
b55dcfc to
e24e21b
Compare
yermulnik
left a comment
There was a problem hiding this comment.
I didn't read any of the Spec DMs — that (along with coding part of the PR) is too much of text for a single PR to be read be a human. Please split into separate PRs to ease review process and to separate entities. Thanks.
Co-authored-by: George Yermulnik (Georgii Iermulnik) <yz@yz.kiev.ua>
Co-authored-by: George Yermulnik (Georgii Iermulnik) <yz@yz.kiev.ua>
Co-authored-by: Maksym Vlasov <MaxymVlasov@users.noreply.github.com>
.agents/commands/opsx/, .agents/skills/openspec-*/, and openspec/ are personal local tooling, not meant to be shared via this repo's history. They stay on disk, untracked, ignored via a per-repo excludesFile configured outside this repo (~/.gitconfig includeIf). Assisted-by: Sisyphus:claude-sonnet-5 opencode
There was a problem hiding this comment.
Pull request overview
This PR adds opt-in, per-hook tool version pinning via --hook-config=--tool-version=... for hooks that wrap downloadable binaries, resolving/caching pinned versions on demand by reusing the existing tools/install/<tool>.sh installers. It also updates docs to explain usage (including a Docker cache mount pattern) and adjusts hook plumbing so wrapped tools are invoked via a resolved tool_path.
Changes:
- Extend
common::per_dir_hookto resolve a wrapped tool binary path (optionally pinned + cached) and pass it into hook implementations. - Add
common::resolve_tool_path+ supporting helpers (--tool-version,--tool-version-mode, OS/arch detection) and wire affected hooks to use the resolved tool path. - Document the new pinning feature and add contributor guidance about the new runtime installer call site.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
tools/install/_common.sh |
Make /.env sourcing conditional to allow installer scripts to run outside Docker builds. |
README.md |
Document --tool-version / --tool-version-mode, Renovate custom manager, and Docker cache mounting. |
hooks/_common.sh |
Add tool resolution + caching utilities and update common::per_dir_hook signature/behavior to pass a resolved tool_path. |
hooks/terraform_fmt.sh |
Route hook execution through resolved Terraform/OpenTofu binary path. |
hooks/terraform_validate.sh |
Route validate/init flow through resolved Terraform/OpenTofu binary path. |
hooks/terraform_providers_lock.sh |
Route providers lock through resolved Terraform/OpenTofu binary path. |
hooks/terraform_tflint.sh |
Resolve tflint path (incl. for --init) and pass into per-dir execution. |
hooks/terraform_docs.sh |
Resolve terraform-docs path (optionally pinned) before building the command. |
hooks/terraform_tfsec.sh |
Resolve tfsec path and pass into per-dir/whole-repo execution. |
hooks/terraform_trivy.sh |
Resolve trivy path and pass into per-dir/whole-repo execution. |
hooks/terrascan.sh |
Resolve terrascan path and pass into per-dir/whole-repo execution. |
hooks/tfupdate.sh |
Resolve tfupdate path and pass into per-dir/whole-repo execution. |
hooks/terragrunt_fmt.sh |
Resolve terragrunt path and pass into per-dir/whole-repo execution. |
hooks/terragrunt_validate.sh |
Resolve terragrunt path and pass into per-dir/whole-repo execution. |
hooks/terragrunt_validate_inputs.sh |
Resolve terragrunt path and pass into per-dir/whole-repo execution. |
hooks/terragrunt_providers_lock.sh |
Resolve terragrunt path and pass into per-dir/whole-repo execution. |
hooks/terraform_checkov.sh |
Adapt to new per_dir_hook signature while keeping checkov unpinned/no resolved binary. |
hooks/infracost_breakdown.sh |
Resolve infracost path (optionally pinned) before running breakdown. |
AGENTS.md |
Extend skill routing table with OpenSpec-related skills. |
.github/CONTRIBUTING.md |
Note that tools/install/<tool>.sh is now used at runtime for --tool-version pinning. |
.github/AI_POLICY.md |
Add a “Recommended tooling” section referencing OpenSpec workflow. |
Comments suppressed due to low confidence (2)
README.md:460
- This YAML example is not valid as written because args must be indented under the hook item. Update the indentation so users can copy/paste it.
- id: terraform_validate
args:
- --hook-config=--tf-path=opentofu
- --hook-config=--tool-version=1.12.0
```
README.md:481
- This YAML example is not valid as written because args must be indented under the hook item. The current indentation will fail YAML parsing if copied.
- id: terraform_tflint
args:
- --hook-config=--tool-version=0.50.0
- --hook-config=--tool-version-mode=prefer-local
```
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
hooks/_common.sh (1)
615-628: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnrecognized
--tool-version-modevalues fall through to strict silently.A typo (
prefer_local,preferlocal) is indistinguishable fromstrict, so users get a download instead of the local binary with no hint why. A small validation on the accepted set would surface config mistakes.🤖 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 `@hooks/_common.sh` around lines 615 - 628, Validate tool_version_mode in the configuration handling around common::get_hook_config_value and accept only the supported modes, including prefer-local and strict. For any unrecognized value, emit a clear configuration warning or error instead of silently proceeding through the strict download behavior; preserve the existing behavior for valid values.hooks/terragrunt_fmt.sh (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTerragrunt hooks document
tool_pathas a Terraform/OpenTofu binary. Thetf_path→tool_pathrename carried the Terraform-specific wording into all four Terragrunt wrappers, which actually receive a resolvedterragruntbinary; each file'srun_hook_on_whole_repoblock already uses the correct generic wording.
hooks/terragrunt_fmt.sh#L40-L40: change the description to "resolved path to the wrapped tool's binary".hooks/terragrunt_providers_lock.sh#L42-L42: apply the same wording change.hooks/terragrunt_validate.sh#L42-L42: apply the same wording change.hooks/terragrunt_validate_inputs.sh#L42-L42: apply the same wording change.🤖 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 `@hooks/terragrunt_fmt.sh` at line 40, Update the tool_path description in the run_hook_on_whole_repo blocks to say “resolved path to the wrapped tool's binary” instead of Terraform/OpenTofu-specific wording. Apply this change in hooks/terragrunt_fmt.sh (line 40), hooks/terragrunt_providers_lock.sh (line 42), hooks/terragrunt_validate.sh (line 42), and hooks/terragrunt_validate_inputs.sh (line 42).
🤖 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 333-340: Check the exit status of common::resolve_tool_path
immediately after the tool_path assignment in the hook flow, and return or exit
non-zero before invoking run_hook_on_whole_repo when resolution fails. Apply the
same propagation to the early tool-path resolution in terraform_tflint.sh, and
ensure resolve_tool_path consistently returns a non-zero status for every
failure path.
In `@hooks/terraform_docs.sh`:
- Around line 24-26: Update the argument-rewriting loop in terraform_docs.sh so
the --config value is prefixed with the repository path only when it is
relative. Preserve values beginning with / unchanged, while retaining the
existing conversion for relative --config paths.
In `@README.md`:
- Line 443: Update the version-pinning description near the wrapped-tool
download behavior to replace the GitHub-specific wording with “the tool’s
upstream release assets” or equivalent, while preserving the existing caching
and first-use download behavior.
- Around line 447-460: Correct the indentation of args in both YAML examples,
terraform_tflint and terraform_validate, so args is nested beneath its
corresponding - id entry and the examples form valid pre-commit configuration.
---
Nitpick comments:
In `@hooks/_common.sh`:
- Around line 615-628: Validate tool_version_mode in the configuration handling
around common::get_hook_config_value and accept only the supported modes,
including prefer-local and strict. For any unrecognized value, emit a clear
configuration warning or error instead of silently proceeding through the strict
download behavior; preserve the existing behavior for valid values.
In `@hooks/terragrunt_fmt.sh`:
- Line 40: Update the tool_path description in the run_hook_on_whole_repo blocks
to say “resolved path to the wrapped tool's binary” instead of
Terraform/OpenTofu-specific wording. Apply this change in
hooks/terragrunt_fmt.sh (line 40), hooks/terragrunt_providers_lock.sh (line 42),
hooks/terragrunt_validate.sh (line 42), and hooks/terragrunt_validate_inputs.sh
(line 42).
🪄 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 Plus
Run ID: bc8e303e-4ccc-4c41-bad1-d1e6a6a03c0a
📒 Files selected for processing (21)
.github/AI_POLICY.md.github/CONTRIBUTING.mdAGENTS.mdREADME.mdhooks/_common.shhooks/infracost_breakdown.shhooks/terraform_checkov.shhooks/terraform_docs.shhooks/terraform_fmt.shhooks/terraform_providers_lock.shhooks/terraform_tflint.shhooks/terraform_tfsec.shhooks/terraform_trivy.shhooks/terraform_validate.shhooks/terragrunt_fmt.shhooks/terragrunt_providers_lock.shhooks/terragrunt_validate.shhooks/terragrunt_validate_inputs.shhooks/terrascan.shhooks/tfupdate.shtools/install/_common.sh
|
|
||
| > All hooks, which wrap a tool distributed as a downloadable release asset. Not supported for `checkov`/`terraform_checkov` (distributed via PyPi) and for deprecated `terraform_docs_replace` hook. | ||
|
|
||
| 1. You can pin a specific version of the wrapped tool per hook, independent of whatever is on your `$PATH` or baked into the Docker image. If that version isn't already cached locally, it's downloaded from the tool's GitHub releases on first use, then reused (without re-downloading) on every subsequent run. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid claiming every binary is downloaded from GitHub releases.
Terraform/OpenTofu resolution uses HashiCorp release installers, so this should say “the tool’s upstream release assets” (or similar) rather than specifically “GitHub releases.”
🤖 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 `@README.md` at line 443, Update the version-pinning description near the
wrapped-tool download behavior to replace the GitHub-specific wording with “the
tool’s upstream release assets” or equivalent, while preserving the existing
caching and first-use download behavior.
There was a problem hiding this comment.
I don't see an issue here. "Github release" is more undersandable, IMO.
@yermulnik ?
There was a problem hiding this comment.
"GitHub releases" describes the feature, but e.g. "GitHub releases page" (or with capitalized "Releases" to mark the section on the web-site) describes the actual location where we download assets from.
So I'd add "page" after "releases" to provide clarity. However, tech-savvies do indeed often say "Github releases" meaning "Github Releases page" =)
Up to you 👍
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
`local -r tool_path=$(common::resolve_tool_path ...)` discarded the resolver's exit code, so a failed lookup (missing binary, bad --tool-version) left tool_path empty instead of aborting the hook. Split into declare/plain-assign-with-guard/readonly so the failure actually propagates. Applies to per_dir_hook, terraform_tflint, terraform_docs and infracost_breakdown. Also corrects stale tool_path doc comments (9 hooks, still described as "Terraform/OpenTofu binary" after the tf_path->tool_path refactor) and invalid YAML indentation in 3 README --tool-version examples. Assisted-by: Sisyphus:claude-sonnet-5 opencode
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/terragrunt_validate_inputs.sh (1)
25-28: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the configured Terragrunt binary for version-gated subcommands.
common::terragrunt_version_ge_0.78checks theterragruntbinary in PATH, butterragrunt_validate_inputs.shselectsSUBCOMMAND/RUN_ALL_SUBCOMMANDbeforecommon::per_dir_hookresolves--tool-versionintotool_path. A pinned Terragrunt target crossing the0.78boundary can use the wrong CLI syntax; gate the subcommand selection against the configured/resolved tool version, or perform it wheretool_pathis available.🤖 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 `@hooks/terragrunt_validate_inputs.sh` around lines 25 - 28, Update terragrunt_validate_inputs.sh so SUBCOMMAND and RUN_ALL_SUBCOMMAND selection uses the configured or resolved Terragrunt binary represented by tool_path, rather than the PATH terragrunt checked by common::terragrunt_version_ge_0.78. Move the version-gated selection into the flow where tool_path is available, or pass that binary explicitly, preserving the correct CLI syntax across the 0.78 boundary.
🤖 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.
Outside diff comments:
In `@hooks/terragrunt_validate_inputs.sh`:
- Around line 25-28: Update terragrunt_validate_inputs.sh so SUBCOMMAND and
RUN_ALL_SUBCOMMAND selection uses the configured or resolved Terragrunt binary
represented by tool_path, rather than the PATH terragrunt checked by
common::terragrunt_version_ge_0.78. Move the version-gated selection into the
flow where tool_path is available, or pass that binary explicitly, preserving
the correct CLI syntax across the 0.78 boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 12a46dd1-a722-4db4-ae27-3d3c5d421e45
📒 Files selected for processing (13)
README.mdhooks/_common.shhooks/infracost_breakdown.shhooks/terraform_docs.shhooks/terraform_tflint.shhooks/terraform_tfsec.shhooks/terraform_trivy.shhooks/terragrunt_fmt.shhooks/terragrunt_providers_lock.shhooks/terragrunt_validate.shhooks/terragrunt_validate_inputs.shhooks/terrascan.shhooks/tfupdate.sh
🚧 Files skipped from review as they are similar to previous changes (12)
- hooks/infracost_breakdown.sh
- hooks/terraform_trivy.sh
- hooks/terraform_tflint.sh
- hooks/tfupdate.sh
- README.md
- hooks/terragrunt_validate.sh
- hooks/terraform_tfsec.sh
- hooks/terragrunt_fmt.sh
- hooks/terrascan.sh
- hooks/terraform_docs.sh
- hooks/terragrunt_providers_lock.sh
- hooks/_common.sh
Split out of #1002's OpenSpec-adjacent bits so that PR's tool-version pinning work doesn't have to wait on this. Adds an AI_POLICY.md section recommending contributors scope AI-assisted changes with OpenSpec's propose/apply workflow before code gets written, wires the openspec-* skills into AGENTS.md's skill-routing table, and ignores `.agents/skills/openspec-*`/`.agents/commands/opsx/` since `openspec init` generates those locally and they must never be committed. See: #1002 (review) Assisted-by: Sisyphus:claude-sonnet-5 opencode
.github/AI_POLICY.md, .gitignore and AGENTS.md's openspec-* additions now live in #1005 (split out per the review at #1002 (review)), so this branch no longer needs to wait on #1000 or carry them. Assisted-by: Sisyphus:claude-sonnet-5 opencode
common::terragrunt_version_ge_0.78 ran bare `terragrunt --version`, picking the CLI syntax (pre/post-0.78) based on whatever's on $PATH - unrelated to the resolved/pinned binary that --tool-version actually executes. A pinned version on the other side of the 0.78 boundary from $PATH (or nothing on $PATH at all) picks the wrong subcommand syntax and the hook fails. Give the function a required tool_path argument and resolve it early in each of the 4 terragrunt_*.sh hooks before the version-gate check, mirroring terraform_tflint.sh's existing early-resolution precedent. Assisted-by: Sisyphus:claude-sonnet-5 opencode
--hook-config=--tool-version=1.2.3 for pinning wrapped tool version. Works for all tools (even terraform and tofu) except checkov
yermulnik
left a comment
There was a problem hiding this comment.
Other than the below comment, the PR looks good to me 👍🏻
Warning
Do not even try to merge this PR before test from #1004 will be merged to this PR
Important
Testing that it is actually working as expected was not done by me yet. But some tests were already generated in #1004
Description of your changes
What
This section was generated by AI.
--hook-config=--tool-version=X.Y.Zversion pinning to every hook that wraps a downloadable release binary (terraform_tflint,terraform_docs,terraform_tfsec/terraform_trivy,terrascan,infracost_breakdown,tfupdate,terragrunt_*, and Terraform/OpenTofu viaterraform_validate/terraform_fmt/terraform_providers_lock), resolved and cached on demand by invoking the existingtools/install/<tool>.shinstaller scripts - opt-in, zero effect on hooks that don't set it.--hook-config=--tool-version-mode=strict|prefer-localto control whether a pinned version always wins (strict, default) or an already-on-$PATHbinary is preferred when present (prefer-local).terraform_validate/terraform_fmt/terraform_providers_lock,--tf-pathcan now also be set to the literal valueterraform,opentofu, ortofu(instead of an actual binary path) when combined with--tool-version, to explicitly pick which of the two to pin rather than relying on$PATH-based auto-detection.--hook-config=--tool-version=) when a hook's tool is neither pinned nor found on$PATH, instead of deferring to a generic "command not found" failure later.checkov/terraform_checkovis explicitly excluded (pip-distributed, not a GitHub-release binary - forcing it through this path would fight its actual, already-working distribution model).README.mdsection documenting usage, a cache-directory-mounting note for persisting across Docker container runs, and a RenovatecustomManagersrecipe for keeping pins auto-bumped..github/CONTRIBUTING.md's "Add new hook" checklist to mention the new runtime call site alongside the existing Docker build-time one.tools/install/_common.sh:source /.envis now conditional on the file existing, so the installer scripts also run correctly when invoked directly at hook run-time (not just inside a Docker build).common::resolve_tool_path) was silently swallowed bylocal -r tool_path=$(...), lettingterraform_tflint,terraform_docs,infracost_breakdown, and the sharedper_dir_hookcontinue with an empty tool path instead of aborting with the resolver's own error message.tool_pathdoc comments left over from thetf_path->tool_pathrename in 9 hooks, and fixes invalid YAML indentation in 3--tool-versionREADME examples.terragrunt_validate/terragrunt_validate_inputs/terragrunt_fmt/terragrunt_providers_lockpicking their pre/post-0.78 CLI syntax off whateverterragruntis on$PATHinstead of the resolved/pinnedtool_pathbinary that actually runs - a pinned version on the other side of the 0.78 boundary from$PATHpicked the wrong syntax and broke.Why
This enables multi-version testing and the ability to pin the same versions of underlying tools by users, which is a decent improvement towards reproducibility.
Fix: #188, #270
Related: #722
How can we test changes
This section was generated by AI.
Run
pre-commit run terraform_tflint- first run downloads and caches tflint0.50.0into$XDG_CACHE_HOME/pre-commit-terraform(or$HOME/.cache/pre-commit-terraform), subsequent runs hit the cache with no network call. Same pattern works forterraform_validate/terraform_fmt/terraform_providers_lock(pins Terraform/OpenTofu), optionally combined with--hook-config=--tf-path=opentofuto force OpenTofu specifically regardless of what's on$PATH.Assisted-by
Specific models used per commit are specified in the commit messages.