Repository navigation
fix(hooks): resolve component path from metadata.component target - #2802
Merged
Merged
Conversation
…component target Hooks (infracost, trivy, checkov, kics, tflint, kind: command, kind: git, kind: step/steps) resolved $ATMOS_COMPONENT_PATH and their own working directory from the stack-facing component name instead of the resolved metadata.component target or provisioned workdir, so hooks on aliased components (the shared-module pattern) pointed at a directory that doesn't exist on disk. - Thread ProcessStacks-resolved component info into the hook execution context in prepareHookContext. - Export hooks.ComponentPath() as the single resolution point for all hook kinds, extended to cover every provisionable component type. - Set cmd.Dir on the hook subprocess so relative-path tools are also anchored to the resolved component directory, not just the env var. - Default kind: step/steps working_directory to the same resolved path when not explicitly set. Closes #2799
- github.com/google/cel-go v0.26.0 -> v0.29.0 (GHSA-gcjh-h69q-9w9g, medium): json:"-" struct fields were exposed to CEL expressions via dyn(obj)["-"] indexing and JSON struct conversion. Minor-version bump within a 0.x module, not blocked by the gomod semver-major ignore rule. go.mod/go.sum updated via `go get` + `go mod tidy`; NOTICE regenerated. - postcss (transitive, via website/pnpm-lock.yaml) 8.5.16 -> 8.5.23 (GHSA-r28c-9q8g-f849, high): path traversal via sourceMappingURL auto-loading could disclose arbitrary .map file contents. Bumped the existing pnpm.overrides pin from ^8.5.10 to ^8.5.18 in website/package.json; lockfile regenerated with `pnpm install --no-frozen-lockfile`. No CodeQL alerts were open at remediation time. Verified: go build ./..., go test on pkg/condition and its consumers (pkg/config/schema, pkg/project/config, pkg/generator/..., pkg/schema), website npm run build, and ./custom-gcl run --new-from-rev=origin/main all pass.
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
July 24, 2026 22:43
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Erik Osterman (Cloud Posse) (osterman)
had a problem deploying
to
screengrabs
July 24, 2026 22:44 — with 
GitHub Actions
Failure
GitHub Actions
Failure
Erik Osterman (Cloud Posse) (osterman)
had a problem deploying
to
screengrabs
July 24, 2026 22:44 — with
GitHub Actions
Failure
GitHub Actions
Failure
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Resource Changes Found for
|
Contributor
📝 WalkthroughWalkthroughHook context now resolves metadata components best-effort and provisions configured sources before eligible user hooks. A shared component-path resolver drives command, Git, and step hook working directories and ChangesHook component path resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TerraformHooks
participant ProcessStacks
participant ComponentPath
participant CommandHook
participant GitHook
participant StepHook
TerraformHooks->>ProcessStacks: resolve component metadata
TerraformHooks->>ComponentPath: provision or resolve component directory
ComponentPath->>CommandHook: set cmd.Dir and ATMOS_COMPONENT_PATH
ComponentPath->>GitHook: set current-repository workdir
ComponentPath->>StepHook: default missing working_directory
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@pkg/hooks/step_engine_test.go`:
- Around line 345-360: Update the “steps preserve explicit directory” test to
retain explicitDir through the final assertions and compare captured[2] directly
against explicitDir. Keep the existing componentDir assertions for the first two
steps unchanged.
🪄 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: 329b3180-d5f2-4484-98bb-8ff90f9c5a80
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumwebsite/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
NOTICEcmd/terraform/utils.gocmd/terraform/utils_hooks_test.godocs/fixes/2026-07-24-hook-component-path-metadata-component.mddocs/prd/custom-hooks.mddocs/prd/git-ops.mddocs/prd/hooks-step-types.mdgo.modpkg/hooks/command_engine.gopkg/hooks/command_engine_ci_summary_test.gopkg/hooks/command_engine_test.gopkg/hooks/kinds/git/engine.gopkg/hooks/kinds/git/engine_test.gopkg/hooks/main_test.gopkg/hooks/step_engine.gopkg/hooks/step_engine_test.gowebsite/package.json
…ix editorconfig Three CI failures from the previous hooks fix (#2799): - build: `atmos terraform plan --all` failed with "component is required". prepareHookContext's new ProcessStacks call unconditionally required ComponentFromArg, but multi-component invocations (--all/--affected/ --components/--query/--tags/--labels) fire the global before/after hook before any single component is resolved. Skip the call when ComponentFromArg is empty; per-component hooks inside the component walker already have a resolved component and are unaffected. - [floci] terraform-tests: a `type: atmos` step inside a `kind: steps` hook failed with "Stacks directory not found" because it re-invoked atmos as a subprocess with cmd.Dir forced to the resolved component directory. That step type must keep inheriting the ambient process working directory so the nested atmos run resolves the *project's* atmos.yaml/stacks, not the target component's own directory. setDefaultStepWorkingDirectory now excludes type: atmos. - Validation (affected): touching docs/prd/git-ops.md made the whole file "affected", surfacing pre-existing 3-space (odd) indentation under numbered-list continuations that fails editorconfig-checker's indent_size=2 rule for *.md. Re-indented the three affected blocks to 4 spaces. Also addresses CodeRabbit review feedback on PR #2802: the "steps preserve explicit directory" test compared captured[2] against itself; now asserts it directly against explicitDir.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 24, 2026 23:27 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/hooks/step_engine_test.go (1)
370-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven cases for the working-directory scenarios.
This test covers atmos-step exclusion, defaulting, and explicit preservation. Expressing these as table-driven cases will align with the repository’s Go test guidelines and keep the scenarios consistent.
🤖 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 `@pkg/hooks/step_engine_test.go` around lines 370 - 389, Refactor TestSetDefaultStepWorkingDirectory_ExcludesAtmosStepType into table-driven cases covering atmos-step exclusion, shell-step defaulting, and explicit working-directory preservation. Define each case’s input WorkflowStep and expected directory, then run them through a subtest that calls setDefaultStepWorkingDirectory and asserts the expected result.Source: Coding guidelines
🤖 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 `@pkg/hooks/step_engine_test.go`:
- Around line 387-389: Update the test around setDefaultStepWorkingDirectory to
construct the explicit working directory with t.TempDir() or filepath.Join
rather than hardcoding `/explicit`, and assert against the same platform-neutral
value while preserving verification that explicit paths are not overwritten.
---
Nitpick comments:
In `@pkg/hooks/step_engine_test.go`:
- Around line 370-389: Refactor
TestSetDefaultStepWorkingDirectory_ExcludesAtmosStepType into table-driven cases
covering atmos-step exclusion, shell-step defaulting, and explicit
working-directory preservation. Define each case’s input WorkflowStep and
expected directory, then run them through a subtest that calls
setDefaultStepWorkingDirectory and asserts the expected result.
🪄 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: 1d1e7aa7-f75f-41ea-96f7-f512255fc107
📒 Files selected for processing (6)
cmd/terraform/utils.gocmd/terraform/utils_hooks_test.godocs/prd/git-ops.mddocs/prd/hooks-step-types.mdpkg/hooks/step_engine.gopkg/hooks/step_engine_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/prd/hooks-step-types.md
- pkg/hooks/step_engine.go
- cmd/terraform/utils.go
A `before.terraform.*` hook (other than before.terraform.init, the source provisioner's own hook) is a "run before Terraform" event, not a "run before the component's source is provisioned" event. Previously, before-hooks fired in Cobra's PreRunE — before RunE performed JIT source provisioning — so a hook on a freshly-provisioned (first-run) `source:` component saw a component path that did not exist yet. Combined with the earlier #2799 fix (which sets the hook subprocess's cmd.Dir to that path), a kind: command hook would fail outright instead of silently running in the wrong ambient directory. prepareHookContext now calls component.ProvisionAndResolveComponentPath for the resolved single component before firing hooks, matching what ExecuteTerraform does moments later in RunE. This is a no-op for components with no `source:` configured (hooks run normally, as before) and for already-provisioned/non-expired components. Provisioning failures here are logged, not returned: the actual Terraform command performs the same provisioning and surfaces the authoritative error. Also addresses a CodeRabbit review comment on PR #2802: a test asserted a hardcoded "/explicit" path, which violates this repo's cross-platform test rule; replaced with t.TempDir().
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 24, 2026 23:38 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
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 `@cmd/terraform/utils.go`:
- Around line 280-290: Gate the ensureComponentSourceProvisioned call in the
surrounding Terraform hook flow so it is skipped when the command is init, while
preserving pre-provisioning for other Terraform commands. Add a regression test
covering before.terraform.init to verify the hook runs without prior source
provisioning.
In `@docs/prd/custom-hooks.md`:
- Line 101: Update the documentation around `prepareHookContext` and
`$ATMOS_COMPONENT_PATH` to state that the path is guaranteed to be real and
populated only when best-effort source provisioning succeeds. Document that a
provisioning failure prevents hook startup, is logged, and follows `on_failure`
to determine whether the parent command continues.
🪄 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: 5e3825c7-2949-487f-9c26-df7e77534781
📒 Files selected for processing (4)
cmd/terraform/utils.gocmd/terraform/utils_hooks_test.godocs/prd/custom-hooks.mdpkg/hooks/step_engine_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/hooks/step_engine_test.go
Addresses two CodeRabbit review findings on PR #2802: - Major: ensureComponentSourceProvisioned had no guard for `init`, so before.terraform.init — the source provisioner's own lifecycle event (HookEventBeforeTerraformInit) — ran after provisioning had already happened, inverting the one event whose entire point is to fire before the source exists. Gated the call on cmd_.Name() != "init". Added TestPrepareHookContextProvisionsSourceBeforeHooks and TestPrepareHookContextSkipsProvisioningForInit, exercising prepareHookContext end-to-end against a self-contained temp fixture with a local JIT source (no network dependency). - Minor: docs/prd/custom-hooks.md said $ATMOS_COMPONENT_PATH "always" resolves to a populated directory, but the same paragraph already documents that provisioning failures are only logged. Clarified that the guarantee holds only when provisioning succeeds, and that before.terraform.init is excluded from this pre-provisioning step.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 25, 2026 00:07 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
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 `@docs/prd/custom-hooks.md`:
- Line 101: Update the documentation’s failure behavior around ComponentPath()
to reflect that unresolved component paths fall back to the process working
directory, so the hook subprocess may still start there. Alternatively, if the
documented contract requires refusing to start when the component directory is
missing, change ComponentPath() to fail closed and preserve the stated
on_failure behavior.
🪄 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: 8bbc9c74-e46e-415a-b161-5cb7e3ba14ae
📒 Files selected for processing (3)
cmd/terraform/utils.gocmd/terraform/utils_hooks_test.godocs/prd/custom-hooks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/terraform/utils.go
…ake metadata resolution best-effort Two regressions from the prior hook-context fixes, found via the failing Acceptance Tests (linux/windows/macos) job: - TestJITSource_WorkdirWithLocalComponent_AllSubcommands failed for every subcommand after the first (show, state, taint, untaint, metadata, modules, providers, test, validate, ...) with "no such file or directory", and TestJITSource_GenerateVarfile/GenerateBackend failed outright. Root cause: ensureComponentSourceProvisioned ran unconditionally in prepareHookContext for every terraform subcommand, racing a second, independent provisioning attempt against the one RunE (ExecuteTerraform) already performs for every subcommand. The fixture component has no hooks configured at all, so this second attempt had nothing to gain and nothing to synchronize against. Moved the call into runUserHooks, gated on hooks.HasHooks() — it now only runs for components that actually have hooks, which is the only case that needed it in the first place. - Several TestCLICommands golden-snapshot tests (invalid stack, invalid component, workflow failures) regressed: prepareHookContext's ProcessStacks call (added to resolve metadata.component for hooks) requires a valid stack/component and, on failure, aborted PreRunE with an errors.Join(ErrInitializeCLIConfig, ...)-wrapped message — different from RunE's own, differently-formatted validation error for the exact same invalid input. Since PreRunE failing skips RunE entirely, the wrapped hook-layer message now won it over the expected one. Made this call best-effort: on failure, log and fall back to the unresolved component/stack. GetHooks already tolerates an empty Stack/Component by returning no hooks, so hook discovery no-ops cleanly and RunE's own validation produces the authoritative, correctly-formatted error. Verified against all three failure classes locally: full TestJITSource_WorkdirWithLocalComponent_AllSubcommands (21 subcommands), TestJITSource_GenerateVarfile/GenerateBackend, the 6 previously-failing TestCLICommands snapshot cases, and the broader terraform/workflow snapshot suites all pass.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 25, 2026 14:37 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Addresses a CodeRabbit review finding on PR #2802: the doc claimed a missing resolved directory unconditionally prevents the hook subprocess from starting, but ComponentPath()'s in-repo fallback only checks that a path was computed (via u.GetComponentPath), not that it exists on disk — it falls back to the process's own working directory only when the component type has no configured base path or the hook context is incomplete. Clarified both cases: an unprovisioned component still resolves to its (missing) would-be directory and fails to start there, while the narrower fallback-to-CWD case does not fail at all.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 25, 2026 14:39 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…sn't exist Two more regressions from the hook-path fixes, found via the failing Acceptance Tests (linux/windows/macos) job: - TestHelmfileRun_NodeHooksFallbackOnEarlyFailure (all 3 platforms): prepareSubprocess unconditionally set cmd.Dir to ComponentPath(ctx), so a kind: command hook's subprocess refused to start with "no such file or directory" whenever the component was never provisioned. This broke a `when: always` after-hook that must still fire on an early failure — before Terraform ever reaches the component (e.g. use_eks: true with no kubeconfig_path, rejected deterministically before any hook is touched) — so CI/user hooks can observe the failure. Added existingComponentDir: it returns ComponentPath(ctx) only when that directory actually exists on disk, otherwise "", so runSubprocess leaves cmd.Dir unset and the hook inherits the ambient process working directory instead of failing to start. $ATMOS_COMPONENT_PATH is unaffected — it still reports the resolved (possibly nonexistent) path unconditionally via buildAtmosEnv. - TestComponentPathFor_Fallbacks (windows only): a pre-existing test bug surfaced by switching ComponentPath's in-repo fallback to u.GetComponentPath (from a plain filepath.Join) earlier in this PR. The test built its "absolute" fixture paths with filepath.Join(string(filepath.Separator), "repo", ...), which on Windows is `\repo\...` — not absolute per filepath.IsAbs (no drive letter) — so u.GetComponentPath's filepath.Abs() call legitimately expanded it to `C:\repo\...`, breaking equality against the test's unexpanded `want` value. Rooted the fixture paths in a genuine t.TempDir() instead, which is truly absolute (with drive letter) on every platform. Verified: the exact TestHelmfileRun_NodeHooksFallbackOnEarlyFailure scenario now passes, the new TestCommandEngine_ FallsBackToAmbientCWDWhenComponentDirMissing regression test passes, TestComponentPathFor_Fallbacks passes, and the full pkg/hooks/... and cmd/helmfile/... suites pass.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 25, 2026 18:25 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2802 +/- ##
==========================================
+ Coverage 81.82% 81.84% +0.02%
==========================================
Files 1793 1793
Lines 173104 173173 +69
==========================================
+ Hits 141634 141730 +96
+ Misses 23667 23641 -26
+ Partials 7803 7802 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Codecov flagged PR #2802's patch coverage at 69.74% (target 85%), with the worst gaps in cmd/terraform/utils.go and pkg/hooks/command_engine.go. Adds tests for the previously-uncovered branches: - prepareHookContext: a ProcessStacks failure (no --stack given) is swallowed rather than surfaced as PreRunE's error, leaving info unresolved for RunE's own validation to handle. - ensureComponentSourceProvisioned: a JIT source that fails to provision (nonexistent local path) is logged, not returned/panicked — the function is void-returning by design so hooks still get a chance to run. - ComponentPath/resolveProvisionedWorkdir: prefers an actually provisioned workdir on disk over the in-repo fallback. - componentBasePath: covers the packer/ansible/kubernetes/helm base paths and the unrecognized-component-type default (falls back to the working directory). Two lines remain uncovered by design, noted inline: the u.GetComponentPath error branch in ensureComponentSourceProvisioned and resolveInRepoComponentPath's err/empty-path branch both require os.Getwd() to fail from inside a live test process, which isn't reliably reproducible cross-platform (verified experimentally on macOS) — this is genuinely defensive/unreachable code, not a coverage shortfall. Test-only change; no production code touched. Verified via `atmos fix coverage origin/main` (STATUS: OK) and the full cmd/terraform/... and pkg/hooks/... suites passing.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 25, 2026 23:21 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Andriy Knysh (aknysh)
temporarily deployed
to
screengrabs
July 26, 2026 13:33 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
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 `@cmd/terraform/utils_hooks_test.go`:
- Around line 1015-1042: Extend
TestEnsureComponentSourceProvisioned_ProvisioningFailureIsLoggedNotReturned to
assert that filepath.Join(terraformDir, "app") does not exist after
ensureComponentSourceProvisioned returns. Keep the existing NotPanics check and
use an appropriate filesystem assertion to detect any partially created
component directory.
🪄 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: 501ddbf4-ce3e-4b2b-adae-3947ac2a2d83
📒 Files selected for processing (5)
cmd/terraform/utils.gocmd/terraform/utils_hooks_test.godocs/prd/custom-hooks.mdpkg/hooks/command_engine.gopkg/hooks/command_engine_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/hooks/command_engine.go
- cmd/terraform/utils.go
…attempt CodeRabbit flagged that TestEnsureComponentSourceProvisioned_ProvisioningFailureIsLoggedNotReturned promised "a failed provisioning attempt must leave no component directory behind" without asserting it. Adding the assertion exposed a real bug: vendorToTarget runs os.MkdirAll(targetDir) before VendorSource, so a failed download left an empty target directory behind — and a partial copy failure would leave a non-empty one, which needsProvisioning treats as fully provisioned and silently skips re-provisioning on the next run. Track whether the attempt created the directory and remove it again on failure; a pre-existing directory is left untouched. Covered by the strengthened cmd/terraform test plus direct positive/negative unit tests in pkg/provisioner/source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Andriy Knysh (aknysh)
temporarily deployed
to
screengrabs
July 26, 2026 14:13 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
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)
pkg/provisioner/source/provision_hook.go (1)
204-225: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not delete a target this invocation did not create.
The
os.Stat/os.MkdirAllsequence is TOCTOU: another process can create or populatetargetDirafter Line 210, whilecreatedTargetremains true. If this provisioning then fails, Lines 222-225 can remove the other process’s source. Use a target-scoped lock or isolated staging directory with atomic publish, rather than inferring ownership from a priorStat.🤖 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 `@pkg/provisioner/source/provision_hook.go` around lines 204 - 225, Replace the TOCTOU-prone createdTarget check around os.Stat, os.MkdirAll, and VendorSource with a target-scoped lock or isolated staging directory followed by atomic publish. Ensure cleanup after provisioning failure only removes resources owned by this invocation, never an existing or concurrently created targetDir.
🧹 Nitpick comments (1)
pkg/provisioner/source/provision_hook_test.go (1)
1464-1505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the cleanup cases table-driven.
These tests differ only by target pre-existence and expected retention. Combine them into cases to keep the shared provisioning contract together. As per coding guidelines, “Use table-driven tests for testing multiple scenarios.”
🤖 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 `@pkg/provisioner/source/provision_hook_test.go` around lines 1464 - 1505, Combine TestAutoProvisionSource_FailedProvisioningCleansUpCreatedTargetDir and TestAutoProvisionSource_FailedProvisioningKeepsPreexistingTargetDir into one table-driven test. Define cases for a newly created target and a pre-existing empty target, including each case’s setup and expected directory retention, then run the shared AutoProvisionSource failure assertions through subtests.Source: Coding guidelines
🤖 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 `@pkg/provisioner/source/provision_hook.go`:
- Around line 204-225: Replace the TOCTOU-prone createdTarget check around
os.Stat, os.MkdirAll, and VendorSource with a target-scoped lock or isolated
staging directory followed by atomic publish. Ensure cleanup after provisioning
failure only removes resources owned by this invocation, never an existing or
concurrently created targetDir.
---
Nitpick comments:
In `@pkg/provisioner/source/provision_hook_test.go`:
- Around line 1464-1505: Combine
TestAutoProvisionSource_FailedProvisioningCleansUpCreatedTargetDir and
TestAutoProvisionSource_FailedProvisioningKeepsPreexistingTargetDir into one
table-driven test. Define cases for a newly created target and a pre-existing
empty target, including each case’s setup and expected directory retention, then
run the shared AutoProvisionSource failure assertions through subtests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6db7f7dc-fce4-4464-ac00-53b3cc780a17
📒 Files selected for processing (3)
cmd/terraform/utils_hooks_test.gopkg/provisioner/source/provision_hook.gopkg/provisioner/source/provision_hook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/terraform/utils_hooks_test.go
Andriy Knysh (aknysh)
approved these changes
Jul 26, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.224.1. |
This was referenced Aug 4, 2026
This was referenced Aug 17, 2026
This branch was successfully deployed
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
2 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.
what
infracost,trivy,checkov,kics,tflint,kind: command,kind: git,kind: step/kind: steps) now resolve$ATMOS_COMPONENT_PATH— and the hook subprocess's own working directory — from the component's resolvedmetadata.componenttarget or provisioned workdir, instead of the stack-facing component/alias name.hooks.ComponentPath(), extended to cover every provisionable component type (terraform/helmfile/packer/ansible/kubernetes/helm).kind: githooks with norepository:now use the resolved component directory (not the repo root) as their Git workdir, socommit.pathsare component-relative like other hook kinds.kind: step/kind: stepshooks defaultworking_directoryto the resolved component path when not explicitly set — excepttype: atmossteps, which re-invoke the atmos binary itself and must keep inheriting the ambient working directory so the nested run resolves the project'satmos.yaml/stacks, not the target component's own directory.source:and hooks configured now has its source provisioned before those hooks fire (before.terraform.*is a "run before Terraform" event, not a "run before provisioning" event) — exceptbefore.terraform.init, which is the source provisioner's own lifecycle event and must keep running first. This only triggers for components that actually have hooks; every terraform subcommand already provisions its own component independently inRunE, so gating onhooks.HasHooks()avoids a second, racing provisioning attempt for components with no hooks at all.when: alwaysafter-hooks that must still fire and report the failure.vendorToTargetcreated the target directory before downloading, so a failed download left an empty component directory — enough for path resolution to treat the component as existing — and a partial copy failure would leave a non-empty one thatneedsProvisioningtreats as fully provisioned, silently skipping re-provisioning on the next run. The provisioner now removes the directory only when this attempt created it; a pre-existing directory is left untouched (covered by positive/negative unit tests inpkg/provisioner/source).--all/--affected/--components/--query/--tags/--labels) no longer fail with "component is required": the metadata-resolution step is skipped for the global before/after hook (no single component is resolved yet at that point) and is best-effort everywhere else, so an intentionally invalid stack/component test scenario is rejected by Terraform's own validation with its normal message, not by hook-context preparation with a different one.docs/fixes/2026-07-24-hook-component-path-metadata-component.md) and updated thecustom-hooks,git-ops, andhooks-step-typesPRDs to document all of the above.TestComponentPathFor_Fallbacks) surfaced by this change: a hand-rolledfilepath.Separator-prefixed "absolute" path isn't actually absolute per Go'sfilepath.IsAbson Windows, sou.GetComponentPath'sAbs()call legitimately expanded it with the current drive letter, breaking the test's literal expectation. Rooted the fixture paths in a genuinet.TempDir()instead.docs/prd/git-ops.mdthat failededitorconfig-checker'sindent_size=2rule once this PR made the file "affected" by CI's affected-file validation.github.com/google/cel-gov0.26.0 → v0.29.0 (GHSA-gcjh-h69q-9w9g), and the website's transitivepostcsspin bumped from^8.5.10to^8.5.18(resolves to 8.5.23, GHSA-r28c-9q8g-f849).NOTICEandwebsite/pnpm-lock.yamlregenerated accordingly.why
metadata.component: nat-gatewayon a component named e.g.nat-gateway-alias), hooks pointed at a directory that doesn't exist on disk (.../nat-gateway-aliasinstead of.../nat-gateway). Forkind: infracostthis failed silently — it just logged "Could not autodetect any projects" and reported$0.00, looking like a normal (if boring) result instead of a broken cost-estimation run.atmos describe componentalready resolvedmetadata.componentcorrectly; hooks just weren't sharing that resolution.type: atmossteps,before.terraform.init's own provisioner event, andwhen: alwaysfallback hooks on early failures — each of which needed its own narrow fix to keep working the way it did before this PR, without losing the original fix.postcss(arbitrary.mapfile disclosure) and a medium-severity private-field-exposure issue incel-go, both flagged by Dependabot on this branch; folding them in here keeps this PR mergeable without a separate open vulnerability.references
metadata.componentpath resolution, fully subsumed by this PRsource.uricode path: JIT/workdir-provisioned components ignore metadata.component subpath #2364 / fix(jit): honor metadata.component subpath for JIT source-provisioned components #2371, JIT vendoring (source pull) writes to a different workdir thanterraform plan/initwhenmetadata.componentdiffers from the component instance name #2134 / fix: Use atmos_component for source provisioner workdir paths #2137, fix: respect workdir path for generate: writes and hook-triggered terraform #2309 (Bug 2), Bug: CI planfile upload silently skipped for auto-provisioned (.workdir) components — upload path resolved from source dir, not working dir #2684 — this PR covers the plain/localmetadata.componentbranch they didn't reachSummary by CodeRabbit
ATMOS_COMPONENT_PATHnow matches that resolved location.source:are provisioned (best-effort) before applicablebefore.terraform.*hooks; provisioning failures are non-blocking.working_directoryto the component directory while preserving explicit values and ambient behavior for Atmos steps.source:provisioning, and working-directory defaults.