Repository navigation
fix(terraform): prevent concurrent output corruption - #2898
Merged
Erik Osterman (Cloud Posse) (osterman) merged 21 commits intoAug 11, 2026
Merged
Erik Osterman (Cloud Posse) (osterman) merged 21 commits into
Erik Osterman (Cloud Posse) (osterman) merged 21 commits into
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Contributor
|
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:
📝 WalkthroughWalkthroughConcurrent Terraform runs now propagate suppression contexts and component output writers through scheduling, execution, hooks, provisioners, and step handlers. Sequential runs retain visible output. Tests and documentation cover the behavior. ChangesOutput suppression propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TerraformScheduler
participant TerraformExecution
participant HookEngine
participant StepExecutor
participant Provisioner
TerraformScheduler->>TerraformExecution: start concurrent run with suppressed context
TerraformExecution->>HookEngine: execute hooks with context and writers
HookEngine->>StepExecutor: run steps with suppression and output writers
TerraformExecution->>Provisioner: provision with suppression and stderr writer
Provisioner-->>TerraformExecution: return result and component output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
internal/exec/terraform_execute_helpers_exec.go (1)
420-420: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
optsto explicit-init post-init provisioners.Line 451 calls
dispatchAfterInitFnwithoutopts. An explicitterraform inittherefore drops the shell-command context before post-init provider locking. Concurrent runs can emit unsuppressed provisioner output.Pass
opts...todispatchAfterInitFn. Add a regression test that supplies a suppressed shell-command context and verifies that the dispatch callback receives it.Proposed fix
- dispatchAfterInitFn(atmosConfig, info, componentPath) + dispatchAfterInitFn(atmosConfig, info, componentPath, opts...)🤖 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 `@internal/exec/terraform_execute_helpers_exec.go` at line 420, Forward the variadic opts from the explicit terraform init path into dispatchAfterInitFn at the call site around line 451, preserving the shell-command context for post-init provisioners. Add a regression test using a suppressed shell-command option and verify the dispatch callback receives that option.
🧹 Nitpick comments (1)
pkg/hooks/step_engine_test.go (1)
371-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for multi-step and unsuppressed hooks.
This test covers only
stepEnginewithStdoutconfigured. Add table-driven cases forstepsEngine,Stderr, and no configured writer. This protects the changed multi-step path and the false suppression branch.As per coding guidelines, every new feature must include comprehensive unit tests targeting greater than 80% coverage, and tests should be behavior-focused and table-driven.
🤖 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 371 - 384, Expand TestStepEngineSuppressesTransientOutputWhenWritersAreSet into table-driven cases covering both stepEngine and stepsEngine, with Stdout, Stderr, and no configured writer. Assert suppression occurs only when a writer is configured, including the unsuppressed branch, while preserving the existing handler setup and error assertions.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 `@internal/exec/terraform_execute_helpers_exec.go`:
- Line 420: Forward the variadic opts from the explicit terraform init path into
dispatchAfterInitFn at the call site around line 451, preserving the
shell-command context for post-init provisioners. Add a regression test using a
suppressed shell-command option and verify the dispatch callback receives that
option.
---
Nitpick comments:
In `@pkg/hooks/step_engine_test.go`:
- Around line 371-384: Expand
TestStepEngineSuppressesTransientOutputWhenWritersAreSet into table-driven cases
covering both stepEngine and stepsEngine, with Stdout, Stderr, and no configured
writer. Assert suppression occurs only when a writer is configured, including
the unsuppressed branch, while preserving the existing handler setup and error
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 18fa770d-e265-49ce-8706-a402d96ea242
📒 Files selected for processing (20)
docs/fixes/2026-08-06-jit-output-suppression.mdinternal/exec/shell_utils.gointernal/exec/terraform.gointernal/exec/terraform_execute_helpers.gointernal/exec/terraform_execute_helpers_args.gointernal/exec/terraform_execute_helpers_coverage_test.gointernal/exec/terraform_execute_helpers_exec.gointernal/exec/terraform_execute_helpers_pipeline_test.gointernal/exec/terraform_execute_helpers_test.gointernal/exec/terraform_execute_helpers_workspace_test.gopkg/hooks/step_engine.gopkg/hooks/step_engine_test.gopkg/provisioner/backend_hook.gopkg/provisioner/registry.gopkg/provisioner/workdir/workdir.gopkg/runner/step/clear.gopkg/runner/step/executor.gopkg/runner/step/spin.gopkg/scheduler/adapters/terraform.gopkg/scheduler/adapters/terraform_test.go
Contributor
Author
|
Addressed the review. Explicit terraform init now forwards shell-command options to post-init provisioners, preserving concurrent-output suppression. The hook regression test now covers single-step stdout, multi-step stderr, and no-writer behavior. Validated with focused internal/exec and hooks tests, go build ./..., and atmos lint --changed. |
Contributor
Author
|
CodeRabbit (@coderabbitai) full and extremely comprehensive full review please, everything in this one go so you don't keep finding issues |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/provisioner/backend_hook.go (1)
111-115: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve backend warnings when output is suppressed.
When
OutputSuppressed(ctx)is true, theresult.Warningsloop is skipped.autoProvisionBackendreturns only an error, so warnings from a successful backend provision are dropped unless another path emits them. Suppress spinner and line-control UI, but route warnings through component-prefixed output or return them to a layer that owns that output.🤖 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/backend_hook.go` around lines 111 - 115, Update autoProvisionBackend’s result.Warnings handling so warnings are preserved even when OutputSuppressed(ctx) is true. Keep spinner and line-control UI suppressed, but route suppressed-mode warnings through the component-prefixed output path or return them to the owning layer; retain ui.Warning for normal output.
🤖 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/provisioner/backend_hook.go`:
- Around line 95-97: Update the backend creation error return in the createFunc
invocation to wrap createErr with meaningful backend operation or type context
before returning. Preserve the original error for unwrapping by using %w or the
repository’s established error builder.
---
Outside diff comments:
In `@pkg/provisioner/backend_hook.go`:
- Around line 111-115: Update autoProvisionBackend’s result.Warnings handling so
warnings are preserved even when OutputSuppressed(ctx) is true. Keep spinner and
line-control UI suppressed, but route suppressed-mode warnings through the
component-prefixed output path or return them to the owning layer; retain
ui.Warning for normal output.
🪄 Autofix
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: 34621d6a-ab05-45ec-9488-053e9a418f24
📒 Files selected for processing (20)
docs/fixes/2026-08-06-jit-output-suppression.mdinternal/exec/shell_utils.gointernal/exec/terraform.gointernal/exec/terraform_execute_helpers.gointernal/exec/terraform_execute_helpers_args.gointernal/exec/terraform_execute_helpers_coverage_test.gointernal/exec/terraform_execute_helpers_exec.gointernal/exec/terraform_execute_helpers_pipeline_test.gointernal/exec/terraform_execute_helpers_test.gointernal/exec/terraform_execute_helpers_workspace_test.gopkg/hooks/step_engine.gopkg/hooks/step_engine_test.gopkg/provisioner/backend_hook.gopkg/provisioner/registry.gopkg/provisioner/workdir/workdir.gopkg/runner/step/clear.gopkg/runner/step/executor.gopkg/runner/step/spin.gopkg/scheduler/adapters/terraform.gopkg/scheduler/adapters/terraform_test.go
Contributor
Author
|
I agree that suppressed backend warnings must not be lost. The provisioner API currently has no component-scoped output sink, and emitting ui.Warning here would reintroduce concurrent terminal corruption. I am leaving this concern open rather than redirecting warnings to global output; the correct follow-up is a node-scoped warning channel that the scheduler can flush with the component output. |
zack-is-cool
temporarily deployed
to
screengrabs
August 7, 2026 17:28 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Member
CodeRabbit (@coderabbitai) it sounds instead like the problem is we do not pass the proper IO channels so that the steps/hooks can emit output correctly. We handle it for subprocesses but not within the process. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Candidate files ==='
fd -t f -e go . \
| rg '(^|/)(terraform|step_engine|backend_hook|registry|shell_utils|executor|spin|clear)\.go$' \
| sort
echo '=== Symbol outlines ==='
for f in \
pkg/scheduler/adapters/terraform.go \
internal/exec/terraform.go \
internal/exec/terraform_execute_helpers.go \
pkg/hooks/step_engine.go \
pkg/provisioner/backend_hook.go \
pkg/provisioner/registry.go \
pkg/runner/step/executor.go \
pkg/runner/step/spin.go \
pkg/runner/step/clear.go
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all 2>/dev/null || true
fi
done
echo '=== Output and suppression call paths ==='
rg -n -C 5 --type go \
'(WithOutputSuppressed|OutputSuppressed|WithStdout|WithStderr|Stdout|Stderr|io\.Writer|ui\.(Warning|Info|Error)|ExecuteTerraform|Execute\()' \
pkg/scheduler/adapters/terraform.go \
internal/exec/terraform.go \
internal/exec/terraform_execute_helpers.go \
internal/exec/terraform_execute_helpers_args.go \
internal/exec/terraform_execute_helpers_exec.go \
pkg/hooks/step_engine.go \
pkg/provisioner/backend_hook.go \
pkg/provisioner/registry.go \
pkg/runner/step/executor.go \
pkg/runner/step/spin.go \
pkg/runner/step/clear.go \
2>/dev/null || trueLength of output: 64948 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Hook context, output, and writer use ==='
ast-grep outline pkg/hooks --items all --type class,struct,function --match 'ExecContext|Output|Run|Write|Hook' 2>/dev/null || true
rg -n -C 8 --type go \
'(type ExecContext|type Output|ComponentNodeHookWriters|\.Stdout|\.Stderr|func .*runBeforeNodeHooks|func .*runAfterNodeHooks|NodeHooks)' \
pkg/hooks pkg/scheduler pkg/schema 2>/dev/null || true
echo '=== Provisioner API and call sites ==='
sed -n '1,175p' pkg/provisioner/registry.go
sed -n '36,125p' pkg/provisioner/backend_hook.go
rg -n -C 6 --type go \
'(ExecuteProvisioners\s*\(|type ProvisionerFunc|TerraformExecContext|autoProvisionBackend)' \
internal/exec pkg 2>/dev/null || true
echo '=== Shell command writer options and scheduler executor bridge ==='
ast-grep outline internal/exec --items all --type function --match 'With.*(Stdout|Stderr)|ShellCommand|ExecuteTerraform' 2>/dev/null || true
rg -n -C 7 --type go \
'(func With(Stdout|Stderr)|type ShellCommandOption|WithStdout|WithStderr|execution\.(Stdout|Stderr)|TerraformExecution\{|opts \.\.\.ShellCommandOption)' \
internal/exec pkg/scheduler/adapters/terraform.go 2>/dev/null || trueLength of output: 184737
The scheduler already passes component-scoped writers to lifecycle hooks. The remaining gap is in-process output:
Suppression prevents terminal corruption. It also prevents output from reaching the component stream. The correct fix is to propagate component writers into the step executor and provisioner API, then make in-process UI write through those writers. Global terminal UI must not run for concurrent nodes. I will treat the current suppression-only approach as incomplete output routing, not as the final solution. ✏️ Learnings added
You are interacting with an AI system. |
Contributor
Author
|
CodeRabbit (@coderabbitai) review Implemented the requested component-scoped output routing in ea94fe2. Concurrent Terraform lifecycle output now routes provisioner status and warnings plus spin-step stdout/stderr through the scheduler node writers, preserving line prefixes without transient terminal UI. Added focused coverage for backend, source, workdir, step hooks, spin steps, and Terraform init context propagation. Validation: go build ./..., atmos lint --changed, and focused package tests passed. The full atmos test suite timed out in an unrelated CLI cleanup scan of this linked worktree. |
Contributor
|
|
Contributor
|
|
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status origin/main...HEAD 2>/dev/null || git diff --name-status HEAD~1...HEAD
echo '=== Changed Go symbols ==='
for f in $(git diff --name-only origin/main...HEAD 2>/dev/null | rg '\.go$' || true); do
[ -f "$f" ] || continue
echo "--- $f ---"
ast-grep outline "$f" --items all 2>/dev/null || true
done
echo '=== Output routing definitions and call sites ==='
rg -n -C 5 --type go \
'(WithOutputSuppressed|OutputSuppressed|WithStdout|WithStderr|Stdout|Stderr|io\.Writer|ui\.(Warning|Info|Error|ClearLine)|SpinHandler|ExecuteProvisioners|ProvisionerFunc|ComponentNodeHooksWithOutput)' \
pkg internal/exec 2>/dev/null || true
echo '=== Diff for reviewed execution paths ==='
git diff --unified=80 origin/main...HEAD -- \
pkg/scheduler/adapters/terraform.go \
pkg/hooks \
pkg/provisioner \
pkg/runner/step \
internal/exec \
2>/dev/null || git diff --unified=80 HEAD~1...HEAD -- pkg/scheduler/adapters/terraform.go pkg/hooks pkg/provisioner pkg/runner/step internal/execLength of output: 85342 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Scheduler context and shell-option propagation ==='
rg -n -C 12 --type go \
'(WithOutputWriters|WithOutputSuppressed|ShellCommandOption|WithStdout|WithStderr|ExecuteProvisioners\s*\()' \
pkg/scheduler/adapters/terraform.go \
internal/exec/terraform.go \
internal/exec/terraform_execute_helpers.go \
internal/exec/terraform_execute_helpers_args.go \
internal/exec/terraform_execute_helpers_exec.go \
internal/exec/shell_utils.go 2>/dev/null || true
echo '=== Hook step context propagation ==='
rg -n -C 12 --type go \
'(WithOutputWriters|WithOutputSuppressed|ExecContext|StepExecutor|\.Execute\(|Stdout|Stderr)' \
pkg/hooks/step_engine.go \
pkg/hooks/step_engine_test.go \
pkg/hooks/command_engine.go 2>/dev/null || true
echo '=== Provisioner UI calls and their suppression branches ==='
rg -n -C 10 --type go \
'(ui\.(Warning|Warningf|Info|Success|ClearLine)|OutputWritersFromContext|OutputSuppressed)' \
pkg/provisioner/backend_hook.go \
pkg/provisioner/source/provision_hook.go \
pkg/provisioner/workdir/workdir.go \
pkg/provisioner/workdir/hash.go \
pkg/provisioner/workdir 2>/dev/null || true
echo '=== New and changed tests that exercise output routing ==='
rg -n -C 8 --type go \
'(OutputWriters|componentOutput|WritesWarnings|StreamsOutput|SuppressesUI|WithOutputSuppressed)' \
pkg/provisioner pkg/hooks pkg/runner/step internal/exec pkg/scheduler/adapters 2>/dev/null || trueLength of output: 219263
Warning routing is incomplete during concurrent provisioning. In
Route these warnings to the component stderr writer when output is suppressed. Keep The context propagation is otherwise correct. The scheduler writers reach provisioners and step hooks.
|
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 `@pkg/provisioner/source/provision_hook.go`:
- Around line 247-253: Update the cleanup-failure handling in the VendorSource
flow around operation() and os.RemoveAll so the suppressed path also writes the
cleanup warning to provisioner.OutputWritersFromContext(ctx).Stderr when that
writer is available. Preserve the existing UI warning behavior and ensure the
stale-target-directory diagnostic is emitted through the component writer
without duplicating output unnecessarily.
In `@pkg/runner/step/spin_test.go`:
- Around line 460-482: Update TestSpinHandler_StreamsOutputToComponentWriters to
avoid the POSIX shell command by launching a Go helper process via
os.Executable(). Configure the helper to write distinct markers to stdout and
stderr, then preserve the existing assertions for both writer output and
result.Metadata.
🪄 Autofix
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: 5a396808-0e09-443f-8059-565261021d68
📒 Files selected for processing (17)
internal/exec/shell_utils.gointernal/exec/terraform.gointernal/exec/terraform_execute_helpers.gointernal/exec/terraform_execute_helpers_args.gointernal/exec/terraform_execute_helpers_workspace_test.gopkg/hooks/step_engine.gopkg/hooks/step_engine_test.gopkg/provisioner/backend_hook.gopkg/provisioner/provisioner_test.gopkg/provisioner/registry.gopkg/provisioner/source/provision_hook.gopkg/provisioner/source/provision_hook_test.gopkg/provisioner/workdir/workdir.gopkg/provisioner/workdir/workdir_test.gopkg/runner/step/executor.gopkg/runner/step/spin.gopkg/runner/step/spin_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/exec/terraform_execute_helpers_args.go
- pkg/hooks/step_engine.go
- internal/exec/terraform_execute_helpers_workspace_test.go
- internal/exec/terraform.go
- pkg/provisioner/backend_hook.go
- internal/exec/terraform_execute_helpers.go
- pkg/provisioner/workdir/workdir.go
- pkg/hooks/step_engine_test.go
Contributor
Author
|
Addressed the remaining CodeRabbit concerns in 36848ac.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/provisioner/workdir/workdir.go (1)
179-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
ctxas the single source of suppression state.
syncLocalToWorkdirreceives bothctxandsuppressOutput. It uses different values for sync messages and hash warnings. Remove the boolean parameter and derive it fromctxinside the helper.Suggested refactor.
- metadata, changed, err := s.syncLocalToWorkdir(ctx, atmosConfig, componentConfig, workdirPath, workdirComponent, sourceComponent, stack, suppressOutput) + metadata, changed, err := s.syncLocalToWorkdir(ctx, atmosConfig, componentConfig, workdirPath, workdirComponent, sourceComponent, stack) - workdirPath, workdirComponent, sourceComponent, stack string, suppressOutput bool, + workdirPath, workdirComponent, sourceComponent, stack string, ) (*WorkdirMetadata, bool, error) { + suppressOutput := OutputSuppressed(ctx)Also applies to: 252-252
🤖 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/workdir/workdir.go` at line 179, Update syncLocalToWorkdir and all its callers to remove the suppressOutput boolean parameter, including the invocation in the surrounding workdir flow. Derive suppression state exclusively from ctx inside syncLocalToWorkdir, and use that same context-derived value for both sync messages and hash warnings.
🤖 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/provisioner/workdir/workdir.go`:
- Around line 322-328: Update Service.computeContentHash so hash failures still
call ui.Warning when output is suppressed but
provisioner.OutputWritersFromContext(ctx).Stderr is nil; retain stderr output
when available and normal unsuppressed behavior. Add coverage for a suppressed
context without a stderr writer, verifying the warning is emitted while
provisioning continues with an empty hash.
---
Nitpick comments:
In `@pkg/provisioner/workdir/workdir.go`:
- Line 179: Update syncLocalToWorkdir and all its callers to remove the
suppressOutput boolean parameter, including the invocation in the surrounding
workdir flow. Derive suppression state exclusively from ctx inside
syncLocalToWorkdir, and use that same context-derived value for both sync
messages and hash warnings.
🪄 Autofix
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: 3df4080a-d0f5-467e-ba14-7e0455fea8ea
📒 Files selected for processing (6)
pkg/provisioner/source/provision_hook.gopkg/provisioner/source/provision_hook_test.gopkg/provisioner/workdir/workdir.gopkg/provisioner/workdir/workdir_test.gopkg/runner/step/spin_test.gopkg/runner/step/testmain_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/runner/step/spin_test.go
- pkg/provisioner/source/provision_hook.go
- pkg/provisioner/workdir/workdir_test.go
- pkg/provisioner/source/provision_hook_test.go
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base_ref="origin/main"
if ! git rev-parse --verify "$base_ref" >/dev/null 2>&1; then
base_ref="HEAD~1"
fi
echo '=== PR commits ==='
git log --oneline "${base_ref}..HEAD" --max-count=30
echo '=== Changed files ==='
git diff --name-status "${base_ref}...HEAD"
echo '=== Changed execution-path symbols ==='
for f in $(git diff --name-only "${base_ref}...HEAD" | rg '\.go$' || true); do
[ -f "$f" ] || continue
echo "--- $f ---"
ast-grep outline "$f" --items all 2>/dev/null || true
done
echo '=== Output routing and warning call sites ==='
rg -n -C 7 --type go \
'(WithOutputWriters|OutputWritersFromContext|WithOutputSuppressed|OutputSuppressed|ui\.(Warning|Warningf|Info|Success|ClearLine)|SpinHandler|runCommand|ExecuteProvisioners|prepareInitExecution|ShellCommandOption)' \
pkg/provisioner pkg/hooks pkg/runner/step pkg/scheduler/adapters internal/exec 2>/dev/null || true
echo '=== Current diff: routing-related files ==='
git diff --unified=45 "${base_ref}...HEAD" -- \
internal/exec/shell_utils.go \
internal/exec/terraform.go \
internal/exec/terraform_execute_helpers.go \
internal/exec/terraform_execute_helpers_args.go \
internal/exec/terraform_execute_helpers_exec.go \
pkg/hooks/step_engine.go \
pkg/provisioner \
pkg/runner/step \
pkg/scheduler/adapters/terraform.go
echo '=== Output-routing test assertions ==='
rg -n -C 6 --type go \
'(OutputWriters|OutputSuppressed|componentOutput|stderr|stdout|Warning|spin-output|ExplicitInit)' \
pkg/provisioner pkg/hooks pkg/runner/step internal/exec pkg/scheduler/adapters 2>/dev/null || trueLength of output: 185148 🏁 Script executed: #!/bin/bash
set -euo pipefail
base_ref="origin/main"
if ! git rev-parse --verify "$base_ref" >/dev/null 2>&1; then
base_ref="HEAD~1"
fi
echo '=== Targeted changed-file diff ==='
git diff --name-only "${base_ref}...HEAD" -- \
pkg/scheduler/adapters/terraform.go \
pkg/hooks/step_engine.go \
pkg/provisioner/backend_hook.go \
pkg/provisioner/source/provision_hook.go \
pkg/provisioner/workdir/workdir.go \
pkg/runner/step/spin.go \
pkg/runner/step/clear.go \
internal/exec/terraform.go \
internal/exec/terraform_execute_helpers.go \
internal/exec/terraform_execute_helpers_args.go \
internal/exec/terraform_execute_helpers_exec.go \
internal/exec/shell_utils.go
git diff --unified=18 "${base_ref}...HEAD" -- \
pkg/scheduler/adapters/terraform.go \
pkg/hooks/step_engine.go \
pkg/provisioner/backend_hook.go \
pkg/provisioner/source/provision_hook.go \
pkg/provisioner/workdir/workdir.go \
pkg/runner/step/spin.go \
pkg/runner/step/clear.go \
internal/exec/terraform.go \
internal/exec/terraform_execute_helpers.go \
internal/exec/terraform_execute_helpers_args.go \
internal/exec/terraform_execute_helpers_exec.go \
internal/exec/shell_utils.go
echo '=== Function locations ==='
rg -n --type go \
'^(func |type )?(autoProvisionBackend|vendorToTarget|writeWarning|syncLocal|syncContent|Execute|runCommand|ExecuteTerraform|shellCommandProvisionerContext|prepareInitExecution|resolveAndProvisionComponentPath|prepareComponentExecution)\b' \
pkg/provisioner/backend_hook.go \
pkg/provisioner/source/provision_hook.go \
pkg/provisioner/workdir/workdir.go \
pkg/hooks/step_engine.go \
pkg/runner/step/spin.go \
pkg/runner/step/clear.go \
pkg/scheduler/adapters/terraform.go \
internal/exec/shell_utils.go \
internal/exec/terraform_execute_helpers.go \
internal/exec/terraform_execute_helpers_exec.go
echo '=== All UI writes in modified provisioner implementations ==='
rg -n -C 4 --type go 'ui\.(Warning|Warningf|Info|Infof|Success|Successf|Error|Errorf)|fmt\.(Fprint|Fprintf|Fprintln)' \
pkg/provisioner/backend_hook.go \
pkg/provisioner/source/provision_hook.go \
pkg/provisioner/workdir/workdir.go
echo '=== Focused routing tests ==='
rg -n -C 5 --type go \
'(WritesWarningsToOutputWriter|OutputWriters|OutputSuppressed|StreamsOutput|component.*[Oo]utput|spin-output|ExplicitInit)' \
pkg/provisioner/provisioner_test.go \
pkg/provisioner/source/provision_hook_test.go \
pkg/provisioner/workdir/workdir_test.go \
pkg/hooks/step_engine_test.go \
pkg/runner/step/spin_test.go \
internal/exec/terraform_execute_helpers_test.go \
internal/exec/terraform_execute_helpers_workspace_test.go \
pkg/scheduler/adapters/terraform_test.goLength of output: 87109
During concurrent execution, two provisioner status messages are still lost:
Both paths run only when output is not suppressed. The concurrent scheduler suppresses that output. Route these messages to Add focused assertions that verify these status messages reach the component writer during suppressed execution. You are interacting with an AI system. |
Contributor
Author
|
CodeRabbit (@coderabbitai) resume CodeRabbit (@coderabbitai) review Please run a full review of the current branch, including output routing, suppressed warning fallbacks, cross-platform tests, and context propagation. |
zack-is-cool
temporarily deployed
to
screengrabs
August 11, 2026 00:03 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
zack-is-cool
temporarily deployed
to
screengrabs
August 11, 2026 00:03 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Erik Osterman (Cloud Posse) (osterman)
enabled auto-merge
August 11, 2026 01:16
Erik Osterman (Cloud Posse) (osterman)
previously approved these changes
Aug 11, 2026
Member
|
zack-is-cool please review the failed Windows test: https://github.com/cloudposse/atmos/actions/runs/31444584259/job/93647514571?pr=2898 |
Andriy Knysh (aknysh)
temporarily deployed
to
screengrabs
August 11, 2026 02:20 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
auto-merge was automatically disabled
August 11, 2026 17:35
Head branch was pushed to by a user without write access
zack-is-cool
temporarily deployed
to
screengrabs
August 11, 2026 17:59 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
zack-is-cool
temporarily deployed
to
screengrabs
August 11, 2026 19:27 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Erik Osterman (Cloud Posse) (osterman)
enabled auto-merge
August 11, 2026 19:27
Erik Osterman (Cloud Posse) (osterman)
approved these changes
Aug 11, 2026
Andriy Knysh (aknysh)
approved these changes
Aug 11, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (83.33%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2898 +/- ##
==========================================
+ Coverage 82.93% 82.96% +0.02%
==========================================
Files 1879 1879
Lines 182633 182749 +116
==========================================
+ Hits 151474 151613 +139
+ Misses 23343 23315 -28
- Partials 7816 7821 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.226.0-rc.5. |
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 12, 2026
Merging origin/main's fix(terraform): prevent concurrent output corruption (#2898) added a fourth provisioner.OutputWriters parameter to (*Service).Provision. Every call site main's own history knew about was updated by that commit, but this branch's own TestServiceProvision_NestedComponentName_SanitizesLikeBuildPath (added by an earlier, unrelated fix on this branch) didn't exist in main's history, so git's auto-merge had nothing to reconcile it against and left the stale three-argument call in place -- a compile failure only visible once this branch actually merges main, which is exactly what GitHub Actions' implicit PR-merge checkout does on every CI run regardless of whether this branch has locally merged yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Michael Pursifull (arcaven)
pushed a commit
to arcaven/atmos
that referenced
this pull request
Aug 19, 2026
…cloudposse#2879) * test(container): cover combined buildx driver/cache/tags/context args Strengthens pkg/container's pure arg-building test with a case combining engine, driver, cache, custom dockerfile/context, and tags in a single config, closing the one remaining gap versus per-field-only coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(git): tolerate config errors for CI git-clone bootstrap pre-Cobra atmos git clone in a fresh CI workspace (no atmos.yaml yet, e.g. a profile referenced by CI config) failed with "profile not found" before ever attempting the clone, and ATMOS_CI=true had no effect. Execute() runs an initial cfg.InitCliConfig before Cobra resolves any command; only the second, PersistentPreRun-scoped InitCliConfig call knew how to tolerate the CI bootstrap clone's expected missing config (applyCIGitCloneBootstrap), so the first call's error aborted the process before that check could run. Add isCIGitCloneBootstrapArgs, an os.Args-based equivalent of the existing cmd-aware bootstrap check, so the pre-Cobra handler recognizes the same no-argument `atmos git clone` shape and defers to the same ATMOS_CI/CI-provider resolution (via the new exported CIGitCloneModeRequestedFromEnv) before Cobra ever parses the command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): record CI git-clone bootstrap profile fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(git): parse CI bootstrap flags with real pflag arity, not a heuristic The pre-Cobra CI git-clone bootstrap check (added in the prior commit) disqualified the bootstrap on any bare, non-"-"-prefixed token, including a space-separated flag value like the "0" in `--depth 0`. That misread a value-taking flag's argument as a positional repo name/URI, so the exact reported reproduction (`atmos git clone --ci --depth 0` in a fresh CI workspace) still failed on "profile not found". Replace the heuristic with CIGitCloneBootstrapRequestedFromRawArgs, which parses the clone-specific args against a throwaway command carrying the real clone flag set (a fresh newCloneParser() instance, never the shared singleton) via actual pflag parsing, then defers to the existing CICloneBootstrapRequested. This also lets an explicit --ci/--ci=false in the raw args be honored before Cobra resolves the command, which the removed env-only CIGitCloneModeRequestedFromEnv could not do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): update CI git-clone bootstrap fix record for pflag rewrite Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(schema): decode with: into Build/Run/Push/Inspect for custom commands Fixes cloudposse#2876. A custom command's `type: container` step with a `with:` block (engine, driver, cache, tags, etc.) silently dropped everything, falling back to a bare `docker build -f Dockerfile .`, when loaded from a commands.yaml merged into atmos.yaml's Viper config tree. Root cause: `with:` is polymorphic -- decoded into Build/Run/Push/Inspect for `type: container` steps, or the generic With map otherwise -- but that promotion lives entirely in Task.UnmarshalYAML/WorkflowStep.UnmarshalYAML (go-yaml's yaml.Unmarshaler interface), invoked only when something calls yaml.Node.Decode directly (e.g. standalone workflows/*.yaml files via pkg/utils.UnmarshalYAMLFromFile). Custom commands merged into atmos.yaml decode via Viper's mapstructure pipeline (TasksDecodeHook -> decodeTaskFromMap), which never invokes yaml.Unmarshaler and had no equivalent promotion, so `with:` only ever reached the raw generic map. decodeTaskFromMap now pulls `with:` out before the mapstructure decode and replays the same polymorphic decode via decodeStepWith, round-tripping the value through YAML so both code paths share one implementation and can't drift apart. Reproduced through the real production paths per the bug report's request: config loaded via InitCliConfig (pkg/config), and the full custom command executed via RootCmd through a fake logging docker executable (cmd/) -- not by manually constructing schema.Task/WorkflowStep/ContainerBuildStep literals, which would have bypassed the actual decode bug. Added a complementary test proving workflow-file and custom-command steps decode with: identically, per the report's public-contract requirement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): sanitize nested component names in BuildPath A component name containing "/" (e.g. a nested layout like ecs/cluster) made workdir.BuildPath produce a real extra subdirectory instead of a single path segment, since the name was interpolated into "<stack>-<name>" without escaping and then filepath.Join'd. That put the nested component's workdir one level deeper than a flat component's at the same stack. Any path computed relative to the workdir -- most visibly a relative `backend.local.path` template like `../../../.context/tfstate/...` -- therefore climbed to a different real ancestor for the nested component than for the flat one, silently writing state under a different root (<repo>/.workdir/.context/... instead of <repo>/.context/...) even though both components used the identical backend config. Sanitize the component name the same way internal/exec/terraform_generate_ backends.go already does for backend template context: replace "/" with "-" before building the workdir directory name. BuildPath is the single formula reused by the source provisioner and by internal/terraform_backend's JIT-workdir state lookup, so both pick up the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): record custom-command with: and workdir path-depth fixes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(terraform/output): retarget containment-guard test at stack traversal; surface cached output lookups BuildPath now sanitizes "/" out of component names, so the containment guard test's traversal-via-component vector no longer escapes BasePath. Retarget it at the stack argument, which isn't sanitized the same way and still needs the guard. Also make cache-hit output lookups emit the same visible "Fetching ..." notification a real fetch would, instead of only a Debug-level log, so a second output lookup on an already-cached component isn't silently invisible. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(terraform/output): strip ANSI before asserting cache-hit visibility; correct fix-log formatter name CI forces color output (CI=true), which makes the markdown-based UI renderer split "Fetching vpc_id ..." into multiple ANSI-styled runs right at the literal underscore, without dropping or reordering any visible characters. Strip ANSI before the assert.Contains checks, matching the ansi.Strip convention already used elsewhere in the test suite. Also correct the fix-log's "gofmt" validation bullet to "gofumpt", the formatter this repo actually mandates and runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(claude): deny gofmt in Claude Code permissions Repo mandates gofumpt, not gofmt (CLAUDE.md, .golangci.yml). Denying the raw command prevents Claude Code from running gofmt directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: close Codecov patch-coverage gaps on PR cloudposse#2879 Adds behavior-focused tests for the 5 lines Codecov flagged as uncovered on this branch's added code: isCIGitCloneBootstrapArgs's len(args) < 1 guard, decodeTaskFromMap/decodeStepWithFromMapValue's three error-wrap branches (invalid container action, yaml.Marshal failure via a yaml.Marshaler that errors, yaml.Unmarshal failure via a dangling YAML alias), and resolveOutputFromCache's cache-miss and getOutputVariable- error branches. No production code changes; no assertions weakened. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): record terraform/output CI fixes; correct gofmt->gofumpt typo Two fixes from this branch (cache-hit output lookups now visible; containment-guard test retargeted at the still-open stack-traversal vector after the workdir fix closed the component-name one) had no docs/fixes/ record. Also corrects a stale "gofmt" mention in the git-clone-ci-bootstrap doc to "gofumpt", matching the correction already applied to the container with-block doc. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(registry): widen timing margin in provider-mirror concurrency test Fixes a flaky Windows Acceptance Tests failure: resolving 10 platforms took 784ms against a 750ms threshold, even though that's nowhere near the 1.5s serial floor the test guards against. Raises the bound to 4/5 of the serial floor (1200ms) for headroom against normal CI timing variance, Windows runners especially. No production code changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): sanitize nested component names in createWorkdirDirectory createWorkdirDirectory duplicated the unsanitized stack-componentName formula that BuildPath was already fixed to sanitize, so a local (non-source) component with provision.workdir.enabled: true and a nested name still got a workdir one level deeper than a flat sibling, silently shifting where relative backend.local.path state resolves. Now delegates to BuildPath so both formulas can't drift apart again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner): guard path traversal in source-vendoring fallback DetermineTargetDirectory's non-workdir fallback (the default vendoring path when provision.workdir.enabled is unset) joined the component base path with the raw component name with no containment check, so a component named with ../ segments could vendor outside components/terraform/. Adds the same absolutize-and-prefix containment guard already used by the two other BuildPath-derived callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cli): decode and execute custom-command step-level container: overrides Sibling gap to the with: block fix (docs/fixes/2026-08-05-custom- command-container-with-block-dropped.md): a custom command step's container: override went through three independent failures. The bare boolean opt-out (container: false) broke InitCliConfig for the whole atmos.yaml because decodeTaskFromMap never round-tripped container: through YAML the way with: now does. The mapping form decoded fine but was never consulted at execution time -- the custom-command step loop always ran type: shell steps on the host. And once both of those were fixed, container: false still ran the step inside a container because cloneCommand's JSON round-trip silently dropped WorkflowContainer.Enabled (json:"-"), inverting the opt-out. Fixes all three: decodeTaskContainerFromMapValue mirrors the with: fix's round-trip for container:, cmd/cmd_utils.go's step loop now reuses the same pkg/workflow/container.go session logic the workflow-file path already uses, and WorkflowContainer gained MarshalJSON/UnmarshalJSON so Enabled survives a JSON round-trip. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(container): pass restart/healthcheck through to ephemeral run steps ContainerRunStep.Restart/.HealthCheck decoded fine but EphemeralConfig (the runtime config for type: container, action: run steps) had no such fields, and buildRunConfig never populated them -- unlike the persistent-component path, which already wires the same settings. Adds the fields to EphemeralConfig and populates them via the existing (previously unused for this path) RestartPolicyFromStep/ HealthCheckFromStep helpers, so --restart/--health-* flags now reach the real docker/podman invocation for step-based container runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(config): surface swallowed import errors and hide-nothing validation Two DX gaps where already-computed diagnostic detail never reached the default log level: a YAML syntax error in an import:-loaded commands file was silently swallowed (Debug-only), leaving only a generic "Unknown command" with no hint a config file failed to parse; and container-step validation (missing required field, invalid pull: value) already computed the field/step/type and the bad value but only exposed them via --verbose or dropped them entirely. LocalAdapter now pairs its existing log.Debug with a ui.Warning naming the file and parse error. ValidateRequired's default message now names the field; invalidContainerField now echoes the actual invalid value typed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 Dependabot alerts in website dependencies Bumps three pnpm.overrides pins to their patched releases, all within the major-version line dependabot.yml's ignore policy allows: - js-yaml 3.15.0 -> 3.15.1 (GHSA-5p4m-2wfm-xmqj, alert cloudposse#269) - js-yaml 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, alert cloudposse#268) - mermaid 11.16.0 -> 11.16.1 (GHSA-rhh3-jpg6-66xh cloudposse#267, GHSA-c4c3-pg64-4m4v cloudposse#266, GHSA-6x64-9x62-f2gx cloudposse#265, GHSA-3rrr-jr9j-h3q3 cloudposse#264, GHSA-2v8p-3f2j-5mp7 cloudposse#263) Verified via `pnpm run build` in website/; NOTICE unchanged (no license changes from these patch bumps). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(schema): reject unknown fields in container step with:/driver: blocks The JSON Schema for a container step's with: block already documented it as "validated by the step handler at run time," but nothing fulfilled that promise for unknown keys -- yaml.Node.Decode (used by both the workflow-file and custom-command loading paths) has no strict/KnownFields mode, so a typo'd field like `platforms:` was silently dropped with no error. This masked a real pre-existing test bug: TestWorkflowStep_DecodeWith's push-action case used `tag: v1` instead of the actual `tags: []string` field and passed anyway. decodeYAMLInto and ContainerDriverConfig.UnmarshalYAML now decode through a stream-level yaml.Decoder with KnownFields(true) (the only place go-yaml exposes strict decoding) instead of plain node.Decode, scoped narrowly to the typed container structs -- the generic with: map[string]any fallback other step types use is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 5 Dependabot alerts, 2 unpatched and deferred - github.com/go-git/go-git/v5 5.19.1 -> 5.19.2 (GHSA-hc8v-wwc9-vgxm cloudposse#270, GHSA-qgq7-7hm3-q39j cloudposse#271), pulling in patch bumps to golang.org/x/{mod,net,text,tools} via go mod tidy - nanoid pnpm override widened from a pinned 3.3.3->^3.3.15 mapping to a ^3->^3.3.17 range so every 3.x requester resolves past both vulnerable versions (GHSA-28wg-ghj8-5hjv cloudposse#274, GHSA-2v37-7h3g-55p8 cloudposse#273); previously two different 3.x versions were resolving simultaneously because the override only matched exact-version requests - dompurify pnpm override 3.4.12 -> 3.4.13 (GHSA-55q2-fjhq-7xh7 cloudposse#272) image-size alerts cloudposse#275/cloudposse#276 (GHSA-5p2g-fcmc-qvqq, GHSA-w3rx-r6r6-pgpr) have no first_patched_version yet in any release line -- left open, nothing to bump to. Verified via `go build ./...`, targeted go-git-consumer package tests, and `pnpm run build`; NOTICE regenerated (version-string changes only, no license changes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd): update stale path assertions in container build argv test TestCustomCommandContainerBuildPassesWithBlockToDocker asserted the buildx argv contained bare "Dockerfile" and "app" strings. Since main's cloudposse#2880 (resolve relative paths against step.WorkingDirectory), context: and dockerfile: values in a with: block are correctly resolved to absolute paths before reaching docker, so the argv now contains the full resolved paths instead of the bare relative strings. Docker still receives the same file -- update the assertions to check for the resolved absolute paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner): pass OutputWriters in nested-workdir Provision test Merging origin/main's fix(terraform): prevent concurrent output corruption (cloudposse#2898) added a fourth provisioner.OutputWriters parameter to (*Service).Provision. Every call site main's own history knew about was updated by that commit, but this branch's own TestServiceProvision_NestedComponentName_SanitizesLikeBuildPath (added by an earlier, unrelated fix on this branch) didn't exist in main's history, so git's auto-merge had nothing to reconcile it against and left the stale three-argument call in place -- a compile failure only visible once this branch actually merges main, which is exactly what GitHub Actions' implicit PR-merge checkout does on every CI run regardless of whether this branch has locally merged yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(cmd): assert cache/driver flag values, not just presence TestCustomCommandContainerBuildPassesWithBlockToDocker only checked that --cache-from/--cache-to/--builder appeared somewhere in the argv, not that they carried the configured registry ref, mode=max, or driver. The driver: block also provisions a real Buildx builder via a separate `docker buildx create` invocation (pkg/container/docker.go's ensureBuilder) that the fake runtime already recorded but the test never inspected. Adds table-driven flag-value assertions covering both the create and build invocations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd): normalize marker path for cross-platform shell command markerPath comes from t.TempDir(), which contains backslashes on Windows. The step command string goes through Atmos's mvdan/sh interpreter, which treats \ as an escape character, so the redirection target would resolve to a mangled path. Convert to slashes and quote the path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner): sanitize backslash in workdir component names BuildPath sanitized "/" in a component name to prevent it from adding a real directory level, but not "\", which is Windows' actual path separator. A crafted or copy-pasted component name containing "\" (e.g. "..\\..\\evil") would let filepath.Join/Clean treat it as real ".."-traversal segments on that platform, escaping the intended workdir root. Sanitize "\" identically and unconditionally on every platform, matching the existing "/" handling, so a given component name's workdir path also stays identical across OSes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(runner): make step-name assertion independent, cover empty type The default-message test used step name "run" and field "run.image", so the step-name assertion could pass even if ValidateRequired dropped the step name entirely, since "run.image" also contains "run". Converted to a table with a distinct step name/field per case and no shared substrings, and added a step.Type == "" case to cover the previously-untested branch that omits the "(type ...)" clause. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(schema): reject unknown fields in container: override blocks WorkflowContainer.UnmarshalYAML's mapping branch used plain value.Decode, so a typo'd field (e.g. `imgae` instead of `image`) in a workflow-level or step-level container: block was silently discarded rather than rejected -- the same class of gap already fixed for with: blocks in decodeYAMLInto. Use the existing decodeYAMLKnownFields helper instead, and add regression coverage for both the workflow-file (yaml.Unmarshal) and custom-command (mapstructure + TasksDecodeHook) decode paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixtures): sync workdir-nested fixture with path-safety fixes The README still documented the pre-fix behavior for the app/local-nested "known bug" scenario and the ../escape-test-nowd path-traversal probe, which are both now fixed on this branch. Re-verified both scenarios for real (atmos terraform apply/source pull against the fixture) and updated the manual-testing steps and expected output to match: the nested local component now sanitizes to a sibling workdir, and the unguarded probe now fails with ErrPathTraversal instead of vendoring outside components/terraform/. Updated the .gitignore comment on the now-defensive-only escape-test-nowd entry accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): correct formatter name, markdown syntax, typos, spelling - gofmt -> gofumpt (the repository-required formatter) - double-backtick delimiter for a code span containing a literal backtick, which single backticks can't escape in Markdown - add `text` language identifiers to unlabeled fenced output blocks - "on a already-parsed" -> "on an already-parsed" - "macos" -> "macOS" in a CI platform list Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): correct macOS spelling in CI platform list Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner): use filepath.Rel for component base path containment The naive absBase+separator prefix check breaks when componentBasePath resolves to a filesystem root ("/" on Unix, "C:\" on Windows): absBase already ends in the separator there, so the literal absBase+sep prefix ("//" or "C:\\") never matches any real descendant, rejecting every valid target with ErrPathTraversal. filepath.Rel doesn't have this edge case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(schema): wrap WorkflowContainer JSON decode error UnmarshalJSON returned the raw json.Unmarshal error directly instead of wrapping it with a static error from errors/errors.go, so callers couldn't classify a WorkflowContainer JSON decode failure the way they already can for its YAML counterpart. Wrap with the existing ErrInvalidWorkflowContainer sentinel, matching UnmarshalYAML. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixtures): sync stack-manifest comments with path-safety fixes The comments on the three path-traversal probes in this fixture's dev.yaml still described pre-fix behavior (determineSourceTargetDirectory having no containment guard, DetermineTargetDirectory's non-workdir fallback having no sanitization, createWorkdirDirectory reimplementing an unsanitized formula) even though all three are now fixed on this branch. Updated to describe the current, correct expected behavior, matching the README sync in the prior commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner,cmd): close symlink containment gap and route script steps through container overrides - pkg/provisioner/source: validateWithinComponentBasePath now resolves symlinks in the existing portion of target/base paths before checking containment, closing a bypass where a symlink under componentBasePath pointing outside it passed the old lexical-only check. - cmd/cmd_utils: type: script custom-command steps now route through StepContainerOverride/RunStepContainerOverride like type: shell steps already do, instead of silently ignoring a step-level container: override. - tests/fixtures: drop the POSIX-only /dev/stderr log destination from the source-provisioner-workdir-nested fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd): propagate Cobra cancellation to custom-command step execution executor.Execute and both RunStepContainerOverride call sites in executeCustomCommand used context.Background() instead of cmd.Context(), so a Ctrl-C on the top-level invocation couldn't cancel an extended step handler or a container runtime operation. Derive one executionCtx from cmd.Context() (falling back to context.Background() for direct-test invocations), mirroring the existing depCtx pattern used for dependency graph execution in the same function. Addresses CodeRabbit review comment on PR cloudposse#2879. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): add missing comma after "e.g." in fix-log doc Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd,schema): address CodeRabbit findings on PR cloudposse#2879 Completes Cobra-cancellation propagation to the remaining context.Background() call sites in executeCustomCommand, wraps with:/container: decode failures with their static sentinel errors so errors.Is works regardless of which decode step fails, and extends cmd.NewTestKit(t) to restore RootCmd.Commands() between tests so custom commands registered by one test no longer leak into the next. Also fixes comment/doc-accuracy nits (godot periods, exported test-double doc comments, and two docs/fixes/*.md wording corrections). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(workdir): document BuildPath's separator sanitization in its doc comment CodeRabbit nitpick on PR cloudposse#2879: the doc comment didn't mention that both "/" and "\" are replaced with "-", only the inline comment did. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(cmd): add regression test for RootCmd.Commands() restoration Addresses CodeRabbit findings on PR cloudposse#2879: a dedicated table-driven test was missing for restoreRootCmdCommands (confirmed failing pre-fix, passing post-fix), plus a comment period and an inconsistent path-traversal probe description in a test fixture README. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir,schema): close BuildPath collision/traversal gaps, stop mutating caller task maps Addresses CodeRabbit findings on PR cloudposse#2879: - workdir.BuildPath encoded "/" and "-" identically, so components named e.g. "app/local" and "app-local" collided on the same workdir, sharing files, metadata, and Terraform state. Encode "/" and "\" as "--" instead. - BuildPath validated the component name but never the stack name, both of which come from user-controlled YAML; a crafted stack value could escape BasePath. Move a shared containment check into BuildPath itself so all six call sites get it, instead of the two ad hoc copies that existed before (and the four call sites that had none). - decodeTaskFromMap deleted "with"/"container" keys in place, which could mutate the caller's own map (e.g. Viper's live config tree) when earlier normalization steps returned it unchanged instead of a copy. Also converts a hard-coded JIT-workdir test case to table-driven per a separate nitpick on the same PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): fix EditorConfig indentation in workdir fix-log doc The numbered list's continuation lines used 3-space indentation (aligned under the "1. " marker), which fails the repo's indent_size=2 EditorConfig rule (want multiple of 2). CI's "Run pre-commit hooks" job caught this. Switched to the 2-space continuation indentation already used elsewhere in docs/fixes/. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir,cmd): make BuildPath's encoding fully injective, restore removed RootCmd commands Addresses a second CodeRabbit review pass on PR cloudposse#2879: - The prior "/" -> "--" fix for workdir.BuildPath's component-name collision was still not injective: a component literally named "app--local" collided with "app/local" (both encode to "app--local"). Replaced it with a fully injective scheme -- escape the literal hyphen ("-" -> "-h") before encoding separators ("/" or "\" -> "-s") -- so "-" never appears unescaped in the output and no two distinct component names can produce the same encoded path segment. This changes the on-disk workdir directory name for existing hyphenated components; updated every hardcoded expected-path assertion this touched across six packages, several switched to compute the expected path via the real BuildPath instead of a hand-rolled formula so they can't go stale the same way again. - restoreRootCmdCommands (added in an earlier pass on this PR) only removed commands added to RootCmd after a NewTestKit snapshot; a command present in the snapshot but removed mid-test (via RootCmd.RemoveCommand) stayed gone for every later test. It now also re-adds any snapshot command whose Parent() is no longer RootCmd. Also fixes a godot comment-period nit and stale sanitized-name comments in a fixture referencing the earlier "--" encoding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: update hardcoded workdir names for BuildPath's injective encoding Fixes CI failures on all three platforms (linux/windows/macos) from the previous commit: several tests hardcoded the pre-encoding workdir directory name (e.g. "dev-vpc-remote-workdir", "dev-null-label-exports", "test-producer-from-source") instead of the new "-h"/"-s" escaped form BuildPath now produces for hyphenated component names. These packages weren't covered by the test sweep before that commit landed: pkg/ci/plugins/terraform and the tests/ acceptance suite's JIT-source and source-provisioner-workdir tests. The pkg/git TestDefaultBranchAndGitHubRepository failure on the Windows run is unrelated -- a transient runner permission error on C:/Users/runneradmin/.gitconfig, not a code issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): give backslash its own escape token, route CleanWorkdir through BuildPath Addresses a third CodeRabbit review pass on PR cloudposse#2879: - escapeComponentNameForPath aliased "/" and "\" to the same "-s" token, so "ecs/cluster" and `ecs\cluster` still collided. The aliasing was meant to keep a component name's encoding OS-independent, but that's already guaranteed by the encoding being pure Go string processing (never delegated to path/filepath) -- so backslash now gets its own token ("-b") at no cost to that property. - CleanWorkdir had its own separate, unsanitized stack+"-"+component formula, never routed through BuildPath. It already couldn't find workdirs for "/"-containing components; the encoding fixes on this PR widened that to ordinary hyphenated components too. Now delegates to BuildPath like every other consumer. - TestCustomCommandStepContainerFalseOptOutRunsOnHost only proved the host command ran, not that docker was never invoked. Now installs the fake container runtime and asserts its argument log was never created. - Two other fix-log docs and this PR's own fix-log doc still described earlier, now-superseded encoding examples; updated for consistency. - Replaced a slash-delimited path literal with filepath.Join in a test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): handle filesystem-root basePath, de-tautologize workdir_path_test oracles containWithinBase's absBase+separator prefix check rejected every legitimate workdir path when basePath resolved to a filesystem root (absBase already ends in the separator there, so absBase+sep doubled up). Switch to filepath.Rel, mirroring pkg/provisioner/source/source.go's isWithinBase. Also replace pkg/component/workdir_path_test.go's BuildPath-derived expected paths with independent hand-computed literals: BuildAndResolveWorkdirPath calls the same BuildPath internally, so a setup+assertion pair that both called BuildPath would silently agree on a wrong path if the encoding ever regressed again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: cover BuildPath's error-propagation branches across workdir consumers Adds behavioral tests for the (string, error) BuildPath signature change: stack-name path-traversal now returns errUtils.ErrPathTraversal instead of silently resolving, and every caller's new `if err != nil` branch needs its own test to prove it actually forwards/wraps that error rather than swallowing it. Also covers resolveExistingSymlinks's non-ENOENT propagation and buildWorkdirPath's empty-BasePath default, both left untested by the original patch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir,cmd,tests): contain stack traversal to component-type root, fix bootstrap flag inheritance, fix mock fixture marker pkg/provisioner/workdir/types.go: BuildPath validated the derived path against basePath only, but stack (unlike component) was never escaped before being folded into workdirName -- a stack like "../../components" resolves inside basePath while still escaping .workdir/<componentType>. Now also validates against the canonical per-component-type workdir root. cmd/git/bootstrap.go: CIGitCloneBootstrapRequestedFromRawArgs's throwaway Cobra tree only registered clone-specific flags, so an inherited global flag like --config made clone.ParseFlags reject the args and silently report no CI-bootstrap request. Now registers the real global persistent flags before parsing. tests/fixtures/.../components/terraform/mock/main.tf: was byte-identical to the vendored source-modules/mock/main.tf, including its "vendored" header -- the local component is never vendored, so its component_type marker couldn't distinguish which module actually produced a given state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir,cmd/git): address PR cloudposse#2879 CodeRabbit round and fix local-backend state loss on re-provision CodeRabbit review round on PR cloudposse#2879 (verified against current code, not just the diff it saw): - pkg/provisioner/source/source.go: attach underlying filesystem errors to symlink-resolution failures instead of discarding them. - pkg/provisioner/workdir/types.go: BuildPath rejects a stack name containing "/" or "\" instead of only checking containment after the fact, closing a workdir-collision gap (e.g. stack "team/../prod" aliasing stack "prod"). - pkg/provisioner/workdir/clean.go, cmd/terraform/workdir/workdir_helpers.go: CleanWorkdir/GetWorkdirInfo/DescribeWorkdir now honor atmos_component overrides via BuildPath. The CLI-wired DefaultWorkdirManager had its own separate, never-updated path formula that couldn't find any hyphenated component's real workdir at all -- fixed too. - pkg/provisioner/workdir/workdir.go: best-effort migration of a workdir found at the pre-escaping path onto the new encoded one, so upgrading doesn't orphan existing local state. - cmd/git/bootstrap.go: a malformed `atmos git clone --depth not-a-number` no longer gets masked by an unrelated config/profile error; Cobra's own flag-parsing error now surfaces as intended. Also fixes a real, separate bug found while testing the above: workdir sync was deleting local-backend Terraform state (terraform.tfstate) on every re-provision, since only provider lock files and the workspace-specific terraform.tfstate.d/ were protected from the sync's delete-orphaned-files pass. A local-backend component's state was silently gone after the second run. shouldSkipSyncFile now also protects terraform.tfstate, terraform.tfstate.backup, and .terraform.tfstate.lock.info. See docs/fixes/2026-08-17-pr2879-coderabbit-round-workdir-and-bootstrap-fixes.md and docs/fixes/2026-08-17-workdir-sync-deletes-local-backend-state.md for full details, and website/blog/2026-08-17-container-config-validation-and-workdir-path-encoding.mdx for the user-facing changelog. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): reject only '.'/'..' segments in stack, not every '/' CI caught a real regression from the previous commit's validateStackForPath: rejecting any stack value containing "/" broke cmd/terraform/migrate's own test fixtures (stack "deploy/test") and, transitively, three tests/cli_workdir_test.go fixtures for hyphenated component names. Only a literal "." or ".." path segment is an actual collision/traversal risk (filepath.Join's implicit Clean() can fold it away, aliasing e.g. stack "team/../prod" onto stack "prod"). A plain "/" without such a segment, like "deploy/test", is a real, already-supported nesting convention with no traversal risk -- it just becomes a real subdirectory, exactly as it always has. Narrowed the check accordingly. Also fixed tests/cli_workdir_test.go's testWorkdirShow/testWorkdirDescribe/ testWorkdirCleanSpecific fixtures, which hand-rolled a pre-escaping workdir path for a hyphenated component name instead of computing it via BuildPath -- the same class of drift the prior commit's CleanWorkdir fix addressed. testWorkdirShow/testWorkdirDescribe had been silently masking their own breakage via a weak assert.Contains check that passed on error output too; tightened to require.NoError. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor(cmd): extract shared container-override step helper The "shell" and "script" custom-command step cases each built an identical workflowPkg.ContainerStepParams and called RunStepContainerOverride, differing only in the workflowStep and the display command. Extracted into a shared runContainerOverrideStep closure. No behavior change: TestCustomCommandStepContainerOverrideRunsInsideContainer (shell), its _ScriptType variant, and TestCustomCommandStepContainerFalseOptOutRunsOnHost all still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd,workdir,terraform): address PR cloudposse#2879 CodeRabbit round 3 findings Propagate executionCtx to non-TTY shell steps and the atmos step type so Ctrl-C/prompt cancellation actually stops an in-flight custom-command step instead of letting it run to completion. Make migrateLegacyWorkdir fail closed on a rename error instead of silently creating a fresh empty workdir over an orphaned legacy directory that may hold real Terraform state. Make ExtractComponentPath propagate a BuildPath rejection instead of falling back to the source component directory, which could point Terraform at the wrong workdir on a rejected (traversal/invalid) stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd): make workdir clean/describe/show honor atmos_component overrides resolveComponentConfig passed the caller's already-loaded (processStacks=false) AtmosConfiguration into ExecuteDescribeComponent, which only does its own full stack-processing init when passed nil. That branch never ran, so component resolution always failed silently and every clean/describe/show call fell back to treating the component as its own instance name -- the exact failure mode atmos_component-override support exists to prevent. It now builds its own fully-processed config from the same CLI flag overrides (base-path, config, config-path, profile) the caller already derived, so overrides actually resolve. Adds three regression tests that execute the real command path against a real stack fixture and assert the manager receives the resolved atmos_component, replacing gomock.Any() assertions CodeRabbit flagged as too permissive to catch this. Also fixes two stale doc/comment references from the same review round (obsolete BuildPath encoding description; a stale fixture path in a test comment). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd,provisioner): address PR cloudposse#2879 CodeRabbit round 4 findings Fixes four independent path-identity gaps in the vendoring/workdir subsystem: DetermineTargetDirectory's default vendoring target permitted resolving to the shared component-type directory itself (component name "." or "child/.."); shouldSkipSyncFile protected local-backend state files by basename, over-broadly excluding nested source files with the same name; migrateLegacyWorkdir could rename the wrong identity's directory since its legacy-name formula isn't injective across stack/component; and validateStackForPath's segment split silently dropped empty segments from a leading or repeated "/", letting a stack name alias another's workdir path. Also fixes a test helper that suppressed all panics instead of only the expected one, and corrects three stale doc references from earlier rounds. Skipped one invalid finding (adding perf.Track to pkg/schema/workflow.go would create an import cycle with pkg/perf). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This branch was previously 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
3 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
Prevent concurrent Terraform runs from interleaving provisioner and lifecycle UI with component-prefixed output.
Why
JIT provisioning, backend provisioning, post-init provider locking, and
clearorspinstep hooks could bypass the scheduler's concurrent-output suppression. Terminal control sequences could corrupt output from other components.Validation
go build ./...atmos lint --changedinternal/execsuite completed in a clean worktree.tenv, a non-linked Git worktree for one sandbox test, and an environment without inherited GitLab tokens.