fix: respect workdir path for generate: writes and hook-triggered terraform

[zack-is-cool]

Summary

Fixes a cluster of bugs in provision.workdir.enabled: true mode covering file generation, hook dispatch, store hook correctness, and repeated-apply terraform init prompts.

---

Bug 1 – generate: writes to base component directory instead of workdir

resolveAndProvisionComponentPath called autoGenerateComponentFiles before provisionComponentSource. Generated files (e.g. locals_override.tf) were written to components/terraform/<component>/ instead of the JIT workdir.

Fix: swap call order — provision source first, then generate into the returned (workdir) path.

---

Bug 2 – hooks and output executor used base component directory

extractComponentPath always returned the base component directory because _workdir_path is a runtime key absent from freshly-described sections. Hooks calling terraform output would fail with "no such file or directory" when trying to write backend.tf.json to a path that doesn't exist.

Fix: check provision.workdir.enabled in sections and rebuild the deterministic workdir path via workdir.BuildPath.

---

Bug 3 – hooks fired on every event regardless of events: list

RunAll had no event matching — all hooks ran regardless of their events: list. YAML uses hyphens (after-terraform-apply) but Go HookEvent constants use dots (after.terraform.apply).

Fix: added MatchesEvent() with hyphen→dot normalisation. Hooks with no events: field match all events to preserve backward compatibility with configs written before event filtering existed.

---

Bug 4 – store hook used wrong output getter and wrong error sentinels

The store hook always used GetOutput (which runs terraform init) regardless of when it fires. Running init after apply with a closed stdin triggers state-migration prompts. Additionally, errors used ErrNilTerraformOutput for both retrieval failures and missing keys, and included no context about which hook or event caused the failure.

Fix: RunE now selects the getter based on the event — after- events use GetOutputSkipInit (workdir already initialised); before- events use GetOutput (init may not have run yet). IsPostExecution() helper on HookEvent encodes the contract. Error messages now include hook name, event, output key, component, and stack. Correct sentinels: ErrTerraformOutputFailed for retrieval errors, ErrTerraformOutputNotFound for missing keys.

---

Bug 5 – "Do you want to migrate all workspaces?" prompt on every apply

This was caused by three interacting problems:

1. -reconfigure added whenever WorkdirPathKey was set — WorkdirPathKey is set for both a preserved workdir (TTL not expired) and a wiped/re-provisioned workdir (TTL=0s or expired). Checking it unconditionally added -reconfigure even when .terraform/ was intact.

2. init_run_reconfigure: true overriding the preserved-workdir guard — even after scoping -reconfigure to WorkdirReprovisionedKey, the global InitRunReconfigure flag bypassed the check and always added -reconfigure.

3. cleanTerraformWorkspace deleting .terraform/environment for workdir components — this function was designed for backend-switching on non-workdir components. For workdir components it deleted the active workspace record before every init, causing OpenTofu to see orphaned terraform.tfstate.d/<workspace>/ directories with no active workspace and prompt for migration.

When combined: -reconfigure tells OpenTofu to ignore the saved backend and treat init as fresh. A fresh-init with existing workspace state dirs triggers the migration prompt even when the backend is unchanged.

Fix (three parts):

• Introduce WorkdirReprovisionedKey (_workdir_reprovisioned), set only by vendorToTarget (source wiped) or SyncDir with file changes (workdir synced). This is the correct signal that .terraform/ was actually cleared.
• For workdir components with a preserved workdir, ignore InitRunReconfigure — the backend is always generated deterministically from the same stack config and never changes between runs. -reconfigure is only added when WorkdirReprovisionedKey is set or the subcommand is workspace.
• Skip cleanTerraformWorkspace for workdir-enabled components — the backend is consistent, so there is no reason to clear the workspace record.

---

Tested end-to-end

Full producer → store → consumer pipeline:

1. null-label applies with JIT workdir + generate: override
2. after-terraform-apply hook reads .id output and writes it to Redis (no init re-run, no migration prompt)
3. consumer reads the value via !store local/redis null-label label_id, injects it into its own generate: template, applies successfully
4. Repeated applies do not prompt for workspace migration, with or without init_run_reconfigure: true and with or without ttl: "0s"

Reproduction
This worked successfully for the deployment that I was initially having this issue with. Local reproduction below.

cat << 'SCRIPT' > repro.sh
#!/usr/bin/env bash
# ============================================================
# ATMOS REPRO: generate: writes orphaned override to base
#              component directory; hook-triggered terraform fails;
#              consumer reads store value into JIT workdir generate:
#
# Stack name:  demo       (from vars.name + name_template)
# Components:  null-label (producer), consumer (reads from store)
#
# Requires: atmos, tofu, docker
# ============================================================

set -euo pipefail

WORKDIR="$(mktemp -d -t atmos-repro-XXXXXX)"
echo "Working in: ${WORKDIR}"
cd "${WORKDIR}"

echo "== starting redis =="
docker stop atmos-repro-redis 2>/dev/null || true
docker run -d --rm --name atmos-repro-redis -p 6379:6379 redis:7-alpine
trap 'docker stop atmos-repro-redis 2>/dev/null || true' EXIT
sleep 1

cat <<'EOF' > atmos.yaml
base_path: "."

stores:
  local/redis:
    type: redis
    options:
      url: "redis://localhost:6379"

components:
  terraform:
    base_path: "components/terraform"
    command: "tofu"
    workspaces_enabled: true
    apply_auto_approve: false
    deploy_run_init: true
    init_run_reconfigure: true
    auto_generate_backend_file: true
    auto_generate_files: true

stacks:
  name_template: "{{ .vars.name }}"
  base_path: "stacks"
  included_paths:
    - "**/*"
EOF

mkdir -p stacks

cleanup() {
  echo "-- cleanup --"
  atmos terraform workdir clean --all 2>/dev/null || true
  echo "-- cleanup done --"
}

show_dirs() {
  local label="${1:-}"
  echo
  if [[ -n "$label" ]]; then
    echo "-- directories: $label --"
  fi
  echo "components/terraform/null-label"
  ls -la components/terraform/null-label/ 2>/dev/null || echo "(does not exist)"
  echo ".workdir/terraform/demo-null-label"
  ls -la .workdir/terraform/demo-null-label/ 2>/dev/null || echo "(does not exist)"
  echo ".workdir/terraform/demo-consumer"
  ls -la .workdir/terraform/demo-consumer/ 2>/dev/null || echo "(does not exist)"
}

# ============================================================
# SCENARIO 1: JIT + generate, no hook.
# Verifies generate: writes to the workdir only (not the base
# component directory), and that apply succeeds.
# ============================================================
echo
echo "================================================="
echo "SCENARIO 1: init + apply WITHOUT hook (expect success)"
echo "  - generate: must write only to workdir, not base component dir"
echo "================================================="
cleanup

cat <<'EOF' > stacks/demo.yaml
vars:
  name: demo

terraform:
  backend_type: local

components:
  terraform:
    null-label:
      vars:
        namespace: "eg"
        stage: "test"
        name: "demo"
        enabled: true
      source:
        uri: "git::https://github.com/cloudposse/terraform-null-label.git"
        version: "0.25.0"
        ttl: "0s"
      provision:
        workdir:
          enabled: true
      generate:
        locals_override.tf: |
          # override file generated by atmos
          locals {
            name = "THISISANOVERRIDE"
          }
EOF

echo "== init =="
atmos terraform init null-label -s demo

show_dirs "after init"

echo "== apply =="
atmos terraform apply null-label -s demo -- -auto-approve

show_dirs "after apply"

echo
echo "SCENARIO 1: PASSED"

# ============================================================
# SCENARIO 2: JIT + generate + after-apply hook writes to Redis.
# The hook fires after apply, reads terraform output, and stores
# it in Redis. Tests that the hook does not re-run init (which
# would prompt for workspace migration with a closed stdin).
# ============================================================
echo
echo "================================================="
echo "SCENARIO 2: init + apply WITH after-apply store hook (expect success)"
echo "  - hook reads .id output and stores it in Redis"
echo "  - hook must NOT re-run terraform init"
echo "================================================="
cleanup

cat <<'EOF' > stacks/demo.yaml
vars:
  name: demo

terraform:
  backend_type: local

components:
  terraform:
    null-label:
      vars:
        namespace: "eg"
        stage: "test"
        name: "demo"
        enabled: true
      source:
        uri: "git::https://github.com/cloudposse/terraform-null-label.git"
        version: "0.25.0"
        ttl: "0s"
      provision:
        workdir:
          enabled: true
      generate:
        locals_override.tf: |
          # override file generated by atmos
          locals {
            name = "THISISANOVERRIDE"
          }
      hooks:
        store-outputs:
          events:
            - after-terraform-apply
          command: store
          name: local/redis
          outputs:
            label_id: .id
EOF

echo "== init =="
atmos terraform init null-label -s demo

echo "== apply =="
atmos terraform apply null-label -s demo -- -auto-approve

show_dirs "after apply"

echo
echo "== verifying Redis contains label_id =="
STORED=$(docker exec atmos-repro-redis redis-cli KEYS "*label_id*")
if [[ -z "$STORED" ]]; then
  echo "SCENARIO 2: FAILED — no label_id key found in Redis"
  exit 1
fi
echo "Redis keys: $STORED"
echo
echo "SCENARIO 2: PASSED"

# ============================================================
# SCENARIO 3: Consumer reads label_id from Redis via !store,
# injects it into a generate: template inside its own JIT workdir.
# Tests the full producer → store → consumer pipeline.
# ============================================================
echo
echo "================================================="
echo "SCENARIO 3: consumer reads store value into JIT workdir generate:"
echo "  - consumer.vars.label_id: !store local/redis null-label label_id"
echo "  - generate: uses {{ .vars.label_id }} in a locals override"
echo "  - both components use JIT workdir with ttl: 0s"
echo "================================================="

cat <<'EOF' > stacks/demo.yaml
vars:
  name: demo

terraform:
  backend_type: local

components:
  terraform:
    null-label:
      vars:
        namespace: "eg"
        stage: "test"
        name: "demo"
        enabled: true
      source:
        uri: "git::https://github.com/cloudposse/terraform-null-label.git"
        version: "0.25.0"
        ttl: "0s"
      provision:
        workdir:
          enabled: true
      generate:
        locals_override.tf: |
          # override file generated by atmos
          locals {
            name = "THISISANOVERRIDE"
          }
      hooks:
        store-outputs:
          events:
            - after-terraform-apply
          command: store
          name: local/redis
          outputs:
            label_id: .id

    consumer:
      vars:
        namespace: "eg"
        stage: "test"
        enabled: true
        label_id: !store local/redis null-label label_id
      source:
        uri: "git::https://github.com/cloudposse/terraform-null-label.git"
        version: "0.25.0"
        ttl: "0s"
      provision:
        workdir:
          enabled: true
      generate:
        name_override.tf: |
          # override file generated by atmos — value comes from Redis via !store
          locals {
            name = "{{ .vars.label_id }}-derpderpderp"
          }
EOF

echo "== apply consumer =="
atmos terraform apply consumer -s demo -- -auto-approve

show_dirs "after consumer apply"

echo
echo "== verifying consumer output contains the store value =="
CONSUMER_ID=$(atmos terraform output consumer -s demo 2>/dev/null |  grep "id =" | head -1)
echo "Consumer id output line: $CONSUMER_ID"

if echo "$CONSUMER_ID" | grep -q "derpderpderp"; then
  echo
  echo "SCENARIO 3: PASSED — consumer label contains store-derived value"
else
  echo
  echo "SCENARIO 3: FAILED — consumer output does not contain expected suffix"
  echo "  Expected 'derpderpderp' in id output"
  exit 1
fi

echo
echo "================================================="
echo "ALL SCENARIOS PASSED"
echo "Working directory preserved at: ${WORKDIR}"
echo "================================================="
SCRIPT
bash repro.sh 2>&1 | tee repro.log

Test plan

• TestHook_MatchesEvent — hyphen/dot formats, no match, nil/empty events (backward compat), multiple events
• TestRunAll_EventFiltering — store called/skipped based on event matching
• TestExecutor_GetOutputWithOptions_SkipInit — terraform init NOT called when SkipInit: true
• TestBuildInitArgs_ReconfigureWhenWorkdirReprovisioned — -reconfigure added when workdir wiped
• TestBuildInitArgs_NoReconfigureWhenWorkdirPreserved — -reconfigure NOT added for preserved workdir
• TestBuildInitArgs_NoReconfigureWhenWorkdirPreserved_InitRunReconfigureIgnored — global InitRunReconfigure: true does not override the preserved-workdir guard
• TestBuildInitArgs_ReconfigureForNonWorkdir_InitRunReconfigure — InitRunReconfigure still works for non-workdir components
• TestPrepareInitExecution_SkipsCleanWorkspaceForWorkdir — .terraform/environment preserved for workdir components
• TestPrepareInitExecution_CleansWorkspaceForNonWorkdir — .terraform/environment still cleaned for non-workdir components
• TestIsWorkdirEnabled / TestExtractComponentPath/workdir_enabled_* — workdir path resolution
• Full pkg/hooks, pkg/terraform/output, internal/exec test suites pass

Closes #2308
Closes #2307

Summary by CodeRabbit

• New Features

  • Workdir-aware provisioning that targets JIT workdirs and signals reprovisioning
  • Hook event normalization, post-execution detection, and event-matching/filtering
  • New output APIs including skip-init retrieval and advanced output options

• Improvements

  • Smarter terraform init (-reconfigure) behavior for workdir flows
  • Preserve workspace files for workdir components to avoid unintended deletions
  • More robust output caching and clearer CLI success/error messaging

• Tests

  • Expanded coverage for workdirs, init args, hooks, output paths, and store commands

[atmos-pro]

Tip

Atmos Pro  

No affected stacks workflow was detected for this pull request.
If this is expected, no action is needed.
Learn More.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Reorders workdir provisioning to occur before file generation and hooks; components with workdir enabled return a deterministic workdir path and signal reprovisioning; terraform init reconfigure and workspace cleanup now respect workdir reprovision state; hooks are filtered by event and store-output retrieval is event-aware.

Changes

Cohort / File(s)	Summary
Execution helpers & args internal/exec/terraform_execute_helpers.go, internal/exec/terraform_execute_helpers_args.go, internal/exec/terraform_execute_helpers_test.go	Provisioning is invoked before auto-generation; file generation gated on componentPathExists from provisioner; skip workspace cleanup for workdir components; -reconfigure only added when workdir was reprovisioned; tests updated.
Component path resolution & tests pkg/terraform/output/config.go, pkg/terraform/output/config_test.go	extractComponentPath returns a deterministic workdir path via provWorkdir.BuildPath when workdir enabled and normalizes to absolute path; tests added/extended to cover workdir cases.
Provisioner workdir & source hook pkg/provisioner/workdir/..., pkg/provisioner/source/provision_hook.go, pkg/provisioner/source/source.go, pkg/provisioner/source/provision_hook_test.go	Workdir enable check exported as IsWorkdirEnabled; workdir provisioning sets WorkdirPathKey and now sets WorkdirReprovisionedKey when files changed; source hook uses exported workdir helper.
Terraform init/execute & output API pkg/terraform/output/executor.go, pkg/terraform/output/executor_test.go, pkg/terraform/output/get.go, pkg/terraform/output/config.go	Added GetOutputWithOptions and GetOutputSkipInit; output cache key changed to a null-delimited key; improved error wrapping and TUI messaging; tests updated.
Hooks: event matching, filtering & store command changes pkg/hooks/event.go, pkg/hooks/hook.go, pkg/hooks/hook_test.go, pkg/hooks/hooks.go, pkg/hooks/hooks_test.go, pkg/hooks/store_cmd.go, pkg/hooks/store_cmd_test.go, pkg/hooks/store_cmd_nil_handling_test.go	Added HookEvent.Normalize and IsPostExecution; Hook.MatchesEvent added; Hooks.RunAll filters hooks by event; StoreCommand and helpers became event-aware and select output getter accordingly; tests added/modified.
CLI command hook invocation cmd/terraform/deploy.go	Added PreRunE to invoke runHooks(h.BeforeTerraformDeploy, ...) before command run.
Workdir public types pkg/provisioner/workdir/types.go	Added exported constant WorkdirReprovisionedKey = "_workdir_reprovisioned".

Sequence Diagram(s)

sequenceDiagram
    participant CLI as rgba(66,133,244,0.5) CLI
    participant Exec as rgba(52,168,83,0.5) Executor
    participant Prov as rgba(255,193,7,0.5) Provisioner/Workdir
    participant FS as rgba(244,67,54,0.5) Filesystem
    participant Hooks as rgba(156,39,176,0.5) Hooks/Store
    participant Terraform as rgba(33,150,243,0.5) Terraform

    CLI->>Exec: run terraform command (init/apply/etc.)
    Exec->>Prov: provisionComponentSource (resolve JIT workdir)
    Prov->>FS: create/write workdir files & metadata
    Prov-->>Exec: return componentPath + WorkdirReprovisionedKey (if changed)
    Exec->>FS: autoGenerateComponentFiles -> write to returned componentPath
    Exec->>Hooks: run pre-execution hooks (use returned componentPath)
    Hooks->>Exec: request terraform output (event-aware getter)
    Exec->>Terraform: init/apply (include -reconfigure only if reprovisioned)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat: Implement component workdir provisioning and CRUD commands #1876 — closely related workdir provisioning and init/reconfigure behavior changes.
• fix: Use atmos_component for source provisioner workdir paths #2137 — touches workdir path construction and JIT workdir consumption.
• fix: add JIT vendoring support to generate varfile and generate backend #2054 — overlaps on provisioning-before-generate sequencing and workdir lifecycle changes.

Suggested reviewers

• aknysh
• osterman

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically identifies the main fix: ensuring workdir paths are respected for generated files and hook-triggered terraform operations.
Linked Issues check	✅ Passed	The PR fully addresses both #2308 and #2307: it reorders source provisioning before auto-generation to write into the JIT workdir, rebuilds workdir paths for hooks and output commands, implements MatchesEvent filtering with event normalization, uses WorkdirReprovisionedKey to control init-reconfigure behavior, and skips cleanTerraformWorkspace for workdir components.
Out of Scope Changes check	✅ Passed	All changes directly address the linked issues: workdir path resolution, hook event matching, terraform output caching/getter selection, and init-reconfigure logic for workdir-enabled components. No unrelated refactoring or scope creep detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

pkg/terraform/output/executor_test.go (1)

1517-1519: Optional test-isolation tweak: clean cache after execution too.

You clear the key before running; adding a deferred delete after setup keeps global cache state fully isolated for future tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/terraform/output/executor_test.go` around lines 1517 - 1519, The test
currently clears terraformOutputsCache for stackSlug before running but doesn't
guarantee cleanup afterwards; add a deferred cache delete to fully isolate
global state by calling terraformOutputsCache.Delete(stackSlug) in a defer
immediately after the initial Delete (using the same stackSlug variable) so the
key is removed again when the test exits.

pkg/hooks/hooks_test.go (1)

346-357: Assert exact stored key/value in matching cases.

assert.NotEmpty can still pass if the wrong key is written. Prefer asserting the expected "stack/comp/label_id" entry and value directly.

Based on learnings: "Test behavior, not implementation. Never test stub functions. Avoid tautological tests. Make code testable via dependency injection. No coverage theater. Remove always-skipped tests. Use errors.Is() for error checking."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/hooks/hooks_test.go` around lines 346 - 357, Replace the loose
assert.NotEmpty checks in the two tests that call makeHooks and h.RunAll (the
"after-apply hook runs on after-apply event" and "hook with dot-format event
matches correctly" cases) with exact assertions that the store (use
getStore(h).GetData()) contains the specific key "stack/comp/label_id" and that
its value equals the expected value produced by the hook; keep require.NoError
on RunAll, then assert the map has the key (e.g., via
assert.Contains/assertTrue) and assert.Equal for the exact value so the test
verifies the behavior rather than just non-emptiness.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/hooks/hook.go`:
- Around line 27-28: The comment above the MatchesEvent method on type Hook is
truncated and missing terminal punctuation; update the doc comment for func (h
Hook) MatchesEvent(event HookEvent) bool to complete the sentence and end with a
period (for example: "If the hook has no events configured, it matches all
events to preserve backward compatibility."), ensuring the comment is a complete
sentence and ends with a period to satisfy the linter.

In `@pkg/terraform/output/executor.go`:
- Around line 340-348: In GetOutputSkipInit, the DescribeComponent call
hard-codes ProcessYamlFunctions: true which can evaluate YAML functions when
opts.SkipInit is set and authManager is nil; change the DescribeComponentParams
to set ProcessYamlFunctions to false when opts.SkipInit && authManager == nil
(matching the guard used in fetchAndCacheOutputs) or add the same conditional
guard before calling DescribeComponent so that DescribeComponent(...) uses
ProcessYamlFunctions: false in that case to avoid evaluating auth-backed YAML
functions without an authManager.

---

Nitpick comments:
In `@pkg/hooks/hooks_test.go`:
- Around line 346-357: Replace the loose assert.NotEmpty checks in the two tests
that call makeHooks and h.RunAll (the "after-apply hook runs on after-apply
event" and "hook with dot-format event matches correctly" cases) with exact
assertions that the store (use getStore(h).GetData()) contains the specific key
"stack/comp/label_id" and that its value equals the expected value produced by
the hook; keep require.NoError on RunAll, then assert the map has the key (e.g.,
via assert.Contains/assertTrue) and assert.Equal for the exact value so the test
verifies the behavior rather than just non-emptiness.

In `@pkg/terraform/output/executor_test.go`:
- Around line 1517-1519: The test currently clears terraformOutputsCache for
stackSlug before running but doesn't guarantee cleanup afterwards; add a
deferred cache delete to fully isolate global state by calling
terraformOutputsCache.Delete(stackSlug) in a defer immediately after the initial
Delete (using the same stackSlug variable) so the key is removed again when the
test exits.

🪄 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: 09a78763-c6e2-492d-b96d-93e20afd6d8b

📥 Commits

Reviewing files that changed from the base of the PR and between 328392f and 5fc10c5.

📒 Files selected for processing (17)

• internal/exec/terraform_execute_helpers.go
• internal/exec/terraform_execute_helpers_args.go
• internal/exec/terraform_execute_helpers_test.go
• pkg/hooks/event.go
• pkg/hooks/hook.go
• pkg/hooks/hook_test.go
• pkg/hooks/hooks.go
• pkg/hooks/hooks_test.go
• pkg/hooks/store_cmd.go
• pkg/hooks/store_cmd_nil_handling_test.go
• pkg/hooks/store_cmd_test.go
• pkg/provisioner/source/provision_hook.go
• pkg/provisioner/workdir/types.go
• pkg/provisioner/workdir/workdir.go
• pkg/terraform/output/executor.go
• pkg/terraform/output/executor_test.go
• pkg/terraform/output/get.go

✅ Files skipped from review due to trivial changes (1)

• pkg/provisioner/workdir/types.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/exec/terraform_execute_helpers.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/terraform/output/executor.go`:
- Around line 381-384: The error return uses dynamic fmt.Errorf; replace it with
the repository's static sentinel from errors/errors.go (use the appropriate
sentinel for Terraform output failures) and attach the original err as the
cause/context (using the project's error-wrapping helper such as
errors.Wrap/WithMessage or the repo's convention) so the returned error is built
from the static sentinel plus context about component and stack; update the
return in the executor.go block that currently references component, stack, err
and fmt.Errorf, and keep the u.PrintfMessageToTUI logging as-is.

🪄 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: 202da824-0a30-4f8a-8558-bc6482ec27c2

📥 Commits

Reviewing files that changed from the base of the PR and between 5fc10c5 and c7ef142.

📒 Files selected for processing (3)

• pkg/hooks/hook.go
• pkg/terraform/output/executor.go
• pkg/terraform/output/executor_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/hooks/hook.go
• pkg/terraform/output/executor_test.go

[coderabbitai]

🧹 Nitpick comments (1)

pkg/terraform/output/executor.go (1)

305-389: Consider extracting shared logic to reduce duplication.

GetOutputWithOptions and GetOutput share ~80% of their code (authManager validation, cache check, spinner setup, static remote state handling, execute call, caching, output extraction). A private helper could consolidate this, with GetOutput calling it with opts = nil.

Not blocking — the current structure is readable and backward-compatible. Something to consider if this area grows.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/terraform/output/executor.go` around lines 305 - 389, Extract the shared
logic between GetOutputWithOptions and GetOutput into a new unexported helper
(e.g., fetchTerraformOutput) that accepts parameters: atmosConfig, stack,
component, output, skipCache, authContext, authManager, opts *OutputOptions;
move duplicated steps (authManager type check, cache lookup using
terraformOutputsCache and stackSlug, startSpinnerOrLog/defer stopSpinner,
DescribeComponent via componentDescriber.DescribeComponent with
ProcessYamlFunctions logic, static remote-state handling via
staticRemoteStateGetter.GetStaticRemoteStateOutputs and
GetStaticRemoteStateOutput, context creation, and the call to e.execute) into
that helper, returning (any, bool, error), then have GetOutputWithOptions and
GetOutput simply call this helper (GetOutput passes nil for opts) so all caching
and error handling remains centralized while preserving existing symbols like
execute, componentDescriber, terraformOutputsCache, getOutputVariable and
GetStaticRemoteStateOutput.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/terraform/output/executor.go`:
- Around line 305-389: Extract the shared logic between GetOutputWithOptions and
GetOutput into a new unexported helper (e.g., fetchTerraformOutput) that accepts
parameters: atmosConfig, stack, component, output, skipCache, authContext,
authManager, opts *OutputOptions; move duplicated steps (authManager type check,
cache lookup using terraformOutputsCache and stackSlug, startSpinnerOrLog/defer
stopSpinner, DescribeComponent via componentDescriber.DescribeComponent with
ProcessYamlFunctions logic, static remote-state handling via
staticRemoteStateGetter.GetStaticRemoteStateOutputs and
GetStaticRemoteStateOutput, context creation, and the call to e.execute) into
that helper, returning (any, bool, error), then have GetOutputWithOptions and
GetOutput simply call this helper (GetOutput passes nil for opts) so all caching
and error handling remains centralized while preserving existing symbols like
execute, componentDescriber, terraformOutputsCache, getOutputVariable and
GetStaticRemoteStateOutput.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f26bf817-f439-4b97-b9b1-4f5b55f5735e

📥 Commits

Reviewing files that changed from the base of the PR and between c7ef142 and 8c1de03.

📒 Files selected for processing (1)

• pkg/terraform/output/executor.go

[Erik Osterman (Cloud Posse) (osterman)]

Bug 3 – hooks fired on every event regardless of events: list

CodeRabbit (@coderabbitai) I'm concerned about this conclusion. I just want to be clear that hooks are used, not just from the YAML in configs, but as the core plumbing within Atmos that provisioning ties into. So the events list in YAML isn't the exhaustive list.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me look at how hooks are constructed and used internally in the provisioner plumbing.

---

🏁 Script executed:

#!/bin/bash
# Find all places where Hook structs are instantiated programmatically (not from YAML deserialization)
rg -n 'Hook{' --type go -C3

echo "---"
# Find where RunAll is called
rg -n 'RunAll\(' --type go -C5

echo "---"
# Find hook-related provisioner code
rg -n 'hooks\.' pkg/provisioner/ -C3 --type go

echo "---"
# Check what the Hook struct looks like
ast-grep --pattern 'type Hook struct {
  $$$
}'

echo "---"
# Find how hooks are wired in provisioning
fd -e go . pkg/provisioner/ --exec grep -l 'hook\|Hook'

Length of output: 10259

---

Let me dig deeper into how provisioners tie into the hook system vs user YAML hooks.

---

🏁 Script executed:

#!/bin/bash
# Check the provisioner registry and how provisioners are invoked as hooks
cat pkg/provisioner/registry.go

echo "=== backend_hook.go ==="
cat pkg/provisioner/backend_hook.go

echo "=== source/provision_hook.go ==="
cat pkg/provisioner/source/provision_hook.go

echo "=== hooks/hooks.go (full) ==="
cat pkg/hooks/hooks.go

echo "=== how NewHooks loads items ==="
rg -n 'NewHooks\|items\[' pkg/hooks/ --type go -C3

echo "=== how provisioners are triggered ==="
rg -n 'RunProvisioners\|runProvisioners\|provisioner.*RunAll\|registry.*Run' --type go -C5

Length of output: 32394

---

@osterman Thanks for flagging this — it's an important architectural distinction worth being precise about.

After looking at the code, there are actually two entirely separate hook systems in Atmos, and the PR's MatchesEvent() filtering only touches one of them:

System 1 — User YAML hooks (pkg/hooks):

• Loaded from component YAML hooks: sections via GetHooks() → stored in Hooks.items
• Executed via Hooks.RunAll() — this is where MatchesEvent() filtering was added
• Called from cmd/terraform/utils.go for user-visible hook lifecycle events

System 2 — Internal provisioner hooks (pkg/provisioner):

• Registered programmatically in init() via RegisterProvisioner() → stored in provisionersByEvent map
• Executed via ExecuteProvisioners(event, ...) — completely independent of Hooks.RunAll
• Backend and source provisioners (backend_hook.go, source/provision_hook.go) self-register here and are the "core plumbing" you're referring to

The new MatchesEvent() check lives entirely inside Hooks.RunAll iterating h.items. It has zero effect on ExecuteProvisioners or anything registered via RegisterProvisioner. So the internal provisioning plumbing is not affected.

The original "Bug 3" concern was narrowly about user-configured YAML hooks ignoring their own events: field — which is the right scope for this fix. Backward compatibility is preserved: a Hook with nil/empty Events still matches all events.

That said, your clarification is a useful call-out for documentation/comments — worth making explicit in the Hooks.RunAll godoc that this method only operates on user-defined YAML hooks, not internal provisioners.

---

✏️ Learnings added

Learnt from: osterman
URL: https://github.com/cloudposse/atmos/pull/2309

Timestamp: 2026-04-10T20:40:50.916Z
Learning: In cloudposse/atmos, there are two separate hook systems: (1) user YAML hooks loaded from component `hooks:` sections via `GetHooks()` into `Hooks.items`, executed via `Hooks.RunAll()` in `pkg/hooks/hooks.go`; and (2) internal provisioner hooks registered via `provisioner.RegisterProvisioner()` into `provisionersByEvent` map and executed via `provisioner.ExecuteProvisioners()` in `pkg/provisioner/registry.go`. The provisioner system is the "core plumbing" wired into Terraform/Tofu execution (backend, source provisioners). These two systems are entirely independent; `MatchesEvent()` filtering only applies to YAML-defined hooks in `Hooks.RunAll` and has no effect on internal provisioner hooks.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-25T12:05:49.543Z
Learning: Preference (cloudposse/atmos PR `#2246`): nitrocode would rather extend the existing `generate` feature with an ephemeral/workdir lifecycle instead of introducing a separate "overlays" feature to avoid maintaining two similar surfaces.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-18T23:56:50.624Z
Learning: In cloudposse/atmos, when auth.TerraformPreHook fails in internal/exec/terraform_execute_helpers_exec.go (prepareComponentExecution function), execution is TERMINATED by returning the error. This is the correct behavior — the code comment reads "Pre-hook failures terminate execution — this matches the original terraform.go behavior. Authentication setup failures must not silently produce unauthenticated terraform commands." The previously stored learning (ID 11a3bc42) that said "execution continues" was stale and incorrect. The original terraform.go always returned on TerraformPreHook error.

---

Learnt from: aknysh
Repo: cloudposse/atmos PR: 759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.

---

Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.

[coderabbitai]

@zack-is-cool On it — triggering a fresh review across all those commits. The cache-key null-byte fix and the IsWorkdirEnabled consolidation are exactly the kind of tightening I'd want to see. Let's see how the coverage lands too.

---

🧠 Learnings used

Learnt from: aknysh
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-23T19:10:29.312Z
Learning: cloudposse/atmos: When adding a recovery path that depends on ExecuteShellCommand exit-code semantics, always include a unit test that asserts errors.As(err, errUtils.ExitCodeError) so future refactors cannot break the wrapping contract.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-22T13:00:09.573Z
Learning: cloudposse/atmos: GetGlobMatches has intentionally divergent semantics across packages — pkg/filesystem.GetGlobMatches always returns a non-nil slice (empty on no matches), while pkg/utils.GetGlobMatches treats no matches as an error or nil depending on doublestar.Glob result. Tests and docs should call this out to avoid confusion in reviews.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-23T05:38:47.060Z
Learning: In cloudposse/atmos PR `#2175`, SeedArchivedRepoCache, ResetArchivedRepoCache, newGitHubClientHook, and newGitHubClientHookMu are intentionally kept in the production package pkg/github/repo.go (not export_test.go) because they manipulate unexported package-level state that must be accessible from tests in multiple packages (e.g., internal/exec). Go's export_test.go mechanism is limited to the package under test and is invisible when another package imports the production package during its own tests. These are documented with "NOTE: test utility — cannot move to export_test.go" comments. Do not flag these as production API pollution in future reviews for this repository.

---

Learnt from: johncblandii
Repo: cloudposse/atmos PR: 2262
File: cmd/describe_component.go:179-199
Timestamp: 2026-03-27T17:24:19.073Z
Learning: In `cmd/describe_component.go` (cloudposse/atmos), when the pre-auth component config probe (`g.executeDescribeComponent` with `ProcessYamlFunctions=false`) returns an error, only `errUtils.ErrInvalidComponent` is treated as fatal and returned immediately. All other errors (e.g., transient permission issues reading stack files) are intentionally swallowed so execution falls back to the global auth config. This matches the canonical pattern in `internal/exec/utils_auth.go` lines 121-124. Do not flag this as a bug or suggest returning all errors — the graceful degradation is by design.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-22T04:19:23.617Z
Learning: cloudposse/atmos: pkg/filesystem exposes test-only ResetGlobMatchesCache() and ResetPathMatchCache() helpers; use these (with t.Cleanup) instead of direct global var assignment to avoid data races and inter-test coupling.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-22T17:50:56.785Z
Learning: cloudposse/atmos: pkg/utils now exposes ResetPathMatchCache() and ResetGlobMatchesCache() (export_test.go). In tests, prefer these helpers with t.Cleanup over direct global var assignment to avoid races and inter-test coupling.

---

Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T00:42:36.692Z
Learning: Applies to **/*_test.go : Include negative-path tests for recovery logic: whenever a test verifies that a recovery/fallback triggers under condition X, add a corresponding test that verifies the recovery does NOT trigger when condition X is absent (e.g., mismatched workspace name).

---

Learnt from: osterman
Repo: cloudposse/atmos PR: 1874
File: internal/exec/describe_affected_utils_test.go:436-468
Timestamp: 2025-12-17T20:55:47.884Z
Learning: In the Atmos codebase, there are two different paths for the `locked` flag: (1) filtering logic in `internal/exec/component_utils.go` (`isComponentLocked()`) reads from `componentSection["metadata"]["locked"]` to determine which components to include/exclude, and (2) extraction/rendering logic in `pkg/list/extract/affected.go` reads from `settings.metadata.locked` to display the locked status in output. Tests for filtering behavior should use `metadata.locked`.

---

Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-22T15:27:52.596Z
Learning: Atmos test pattern: Use os.Executable()+TestMain with an env flag (_ATMOS_TEST_EXIT_ONE) to create a cross‑platform “exit 1” subprocess in tests; avoid Unix-only binaries (false/true/sh). Also add a prerequisite test to prove env propagation to ExecuteShellCommand before relying on it in recovery tests.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-23T04:09:45.164Z
Learning: When adding a recovery path that depends on ExecuteShellCommand exit-code semantics, always include a unit test that asserts errors.As(err, errUtils.ExitCodeError) so future refactors cannot break the wrapping contract unnoticed.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-18T23:56:50.624Z
Learning: In cloudposse/atmos, when auth.TerraformPreHook fails in internal/exec/terraform_execute_helpers_exec.go (prepareComponentExecution function), execution is TERMINATED by returning the error. This is the correct behavior — the code comment reads "Pre-hook failures terminate execution — this matches the original terraform.go behavior. Authentication setup failures must not silently produce unauthenticated terraform commands." The previously stored learning (ID 11a3bc42) that said "execution continues" was stale and incorrect. The original terraform.go always returned on TerraformPreHook error.

---

Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-24T00:42:36.692Z
Learning: Applies to **/*.go : Achieve minimum 80% test coverage (CodeCov enforced). All features need tests. Use `make testacc-coverage` for reports.

---

Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Ensure all tests pass, verify code coverage meets targets, run golangci-lint and fix any issues, and update documentation before submitting pull requests

---

Learnt from: aknysh
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-23T19:10:29.312Z
Learning: cloudposse/atmos: Add a guardrail test that asserts ExecuteTerraform invokes ExecuteShellCommand exactly once per call to prevent double-execution regressions.

---

Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-04-10T20:40:50.916Z
Learning: In cloudposse/atmos, there are two separate hook systems: (1) user YAML hooks loaded from component `hooks:` sections via `GetHooks()` into `Hooks.items`, executed via `Hooks.RunAll()` in `pkg/hooks/hooks.go`; and (2) internal provisioner hooks registered via `provisioner.RegisterProvisioner()` into `provisionersByEvent` map and executed via `provisioner.ExecuteProvisioners()` in `pkg/provisioner/registry.go`. The provisioner system is the "core plumbing" wired into Terraform/Tofu execution (backend, source provisioners). These two systems are entirely independent; `MatchesEvent()` filtering only applies to YAML-defined hooks in `Hooks.RunAll` and has no effect on internal provisioner hooks.

---

Learnt from: nitrocode
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2026-03-25T12:05:49.543Z
Learning: Preference (cloudposse/atmos PR `#2246`): nitrocode would rather extend the existing `generate` feature with an ephemeral/workdir lifecycle instead of introducing a separate "overlays" feature to avoid maintaining two similar surfaces.

---

Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/terraform/output/executor.go (1)

239-329: Consider collapsing GetOutput into GetOutputWithOptions to avoid drift.

These two paths now duplicate most orchestration logic (cache, describe, static state, execute, UI/error handling). A single implementation would reduce future divergence risk.

Also applies to: 331-435

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/terraform/output/executor.go` around lines 239 - 329, GetOutput
duplicates most orchestration logic found in GetOutputWithOptions; refactor by
consolidating the shared flow (cache check using stackComponentKey,
componentDescriber.DescribeComponent, staticRemoteStateGetter handling, execute
call, caching, getOutputVariable, and UI/error handling) into a single canonical
implementation (e.g., move that flow into GetOutputWithOptions or a private
helper like fetchTerraformOutput) and have GetOutput delegate to it with default
options (preserving behavior around skipCache, authContext, authManager). Ensure
references to componentDescriber, staticRemoteStateGetter, execute,
terraformOutputsCache, getOutputVariable, and UI/error wrapping remain identical
and only option-specific differences are handled by parameters to the shared
function so both original call sites (including the other block at lines
331-435) call the same implementation.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/terraform/output/config.go`:
- Line 172: The call to provWorkdir.BuildPath currently passes baseComponent
which is used as the fallback when atmos_component is absent, causing wrong
workdir for instance-specific components; change the argument so BuildPath
receives the instance component (e.g., instanceComponent or
instance.Name/instance.Component as used in this file) instead of baseComponent
(keep basePath, componentType, stack, sections unchanged) so the fallback uses
the instance component value when atmos_component is not present.

---

Nitpick comments:
In `@pkg/terraform/output/executor.go`:
- Around line 239-329: GetOutput duplicates most orchestration logic found in
GetOutputWithOptions; refactor by consolidating the shared flow (cache check
using stackComponentKey, componentDescriber.DescribeComponent,
staticRemoteStateGetter handling, execute call, caching, getOutputVariable, and
UI/error handling) into a single canonical implementation (e.g., move that flow
into GetOutputWithOptions or a private helper like fetchTerraformOutput) and
have GetOutput delegate to it with default options (preserving behavior around
skipCache, authContext, authManager). Ensure references to componentDescriber,
staticRemoteStateGetter, execute, terraformOutputsCache, getOutputVariable, and
UI/error wrapping remain identical and only option-specific differences are
handled by parameters to the shared function so both original call sites
(including the other block at lines 331-435) call the same implementation.

🪄 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: 88d1ffc3-a6f8-4d4d-805d-0f9263852376

📥 Commits

Reviewing files that changed from the base of the PR and between e842c4e and af7001e.

📒 Files selected for processing (15)

• cmd/terraform/deploy.go
• pkg/hooks/event.go
• pkg/hooks/hook.go
• pkg/hooks/hook_test.go
• pkg/hooks/hooks_test.go
• pkg/hooks/store_cmd_test.go
• pkg/provisioner/source/provision_hook.go
• pkg/provisioner/source/provision_hook_test.go
• pkg/provisioner/source/source.go
• pkg/provisioner/workdir/workdir.go
• pkg/provisioner/workdir/workdir_test.go
• pkg/terraform/output/config.go
• pkg/terraform/output/config_test.go
• pkg/terraform/output/executor.go
• pkg/terraform/output/executor_test.go

✅ Files skipped from review due to trivial changes (2)

• pkg/hooks/hooks_test.go
• pkg/hooks/hook_test.go

🚧 Files skipped from review as they are similar to previous changes (5)

• pkg/hooks/event.go
• pkg/provisioner/source/provision_hook.go
• pkg/hooks/store_cmd_test.go
• pkg/terraform/output/config_test.go
• pkg/provisioner/workdir/workdir.go

[atmos-pro]

Note

Atmos Pro  

Waiting for your GitHub Actions workflow to upload affected stacks.
Learn More.

[github-actions]

These changes were released in v1.216.0-rc.1.