Route Terraform affected through scheduler

[Mikhail Shirkov (shirkevich)]

Summary

• Route terraform --affected execution through the shared graph-backed Terraform scheduler path.
• Preserve affected-set discovery while passing selected affected node IDs into the adapter.
• Preserve auth manager setup, store resolver bridging, YAML function stack resolution, per-component hooks, and signal context behavior.
• Add terraform destroy --affected so plan/apply/destroy can use the affected scheduler path.
• Add --fail-fast/--keep-going for graph-backed Terraform plan/apply/destroy execution. Fail-fast is the default scheduler behavior; --keep-going continues independent nodes while still skipping blocked dependents.
• Add Git extensions.worktreeConfig compatibility for affected/repo-info paths so agent-created Git worktrees do not fail during go-git repository open.
• Add opt-in Floci coverage for affected plan/apply/destroy using isolated BASE/HEAD Git repos.

Upstream context

• go-git issue #1943 tracks the same failure mode: v5.17.x added strict extension validation before filesystem-storage support for worktreeConfig, causing core.repositoryformatversion does not support extension: worktreeconfig.
• Atmos currently uses go-git v5, so this branch keeps a narrow local compatibility shim for extensions.worktreeConfig until the dependency path can rely on upstream support.

Validation

• rtk go test ./pkg/scheduler/adapters ./cmd/terraform
• rtk go test ./pkg/git
• rtk go test ./internal/exec -run 'TestExecuteDescribeAffected|TestExecuteTerraformAffectedRoutesThroughSchedulerAdapter'
• rtk go test ./internal/exec -run 'TestExecuteTerraformAffectedRoutesThroughSchedulerAdapter|TestExecuteTerraformQueryRoutesThroughSchedulerAdapter'
• rtk go test ./tests -run 'TestCLICommands/(atmos terraform apply --help|atmos terraform apply help|tf plan help shows inherited stack flag|config alias tp --help shows terraform plan help)' -count=1
• rtk proxy go build -o build/atmos .
• build/atmos terraform destroy --help
• build/atmos terraform plan --affected --all
• In a downstream repository with extensions.worktreeConfig=true, build/atmos describe affected -s <stack> -i <identity> --format json returned an empty affected set without the worktreeConfig error
• In the same downstream repository, build/atmos terraform plan --affected -s <stack> -i <identity> --max-concurrency 8 --log-order grouped --hide-no-changes returned "No components affected" without the worktreeConfig error
• ATMOS_TEST_FLOCI=true FLOCI_ENDPOINT_URL=http://localhost:4566 AWS_ENDPOINT_URL=http://localhost:4566 rtk go test ./tests -run TestTerraformFlociAffectedApplyDestroyDAG -count=1 -timeout 15m -v
• rtk go test ./tests -run TestTerraformFlociAffectedApplyDestroyDAG -count=1

Floci affected scenarios

• Direct affected component: modifies the seed Terraform component in HEAD, verifies plan summary contains only seed, applies only seed, and destroys only seed.
• Include dependents: verifies --include-dependents expands seed to the downstream DAG nodes and apply/destroy creates then removes all Floci SSM markers.

Summary by CodeRabbit

• New Features
  • Unified --failure-mode (fail-fast | keep-going) for terraform plan/apply/destroy; destroy and affected flows support selective affected execution with include-dependents/include-dependencies and scheduler-backed selection.
• Documentation
  • CLI help snapshots updated to show new flags and defaults.
• Bug Fixes
  • Improved Git worktree handling and clearer error for unexpected git rev-parse output.
• Tests
  • Extensive new/updated unit and integration tests for failure modes, affected execution, scheduler behavior, and help output.

[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. Ask AI.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Files

None

[Mikhail Shirkov (shirkevich)]

CodeRabbit (@coderabbitai) review

[coderabbitai]

✅ 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]

📝 Walkthrough

Walkthrough

Adds --failure-mode to Terraform CLI and threads it into options and runtime info; routes --affected through a context-aware scheduler-backed path with selection support; implements a worktree-config-tolerant git opener; and adds unit/integration tests and updated help snapshots.

Changes

Terraform Scheduling & Affected Execution

Layer / File(s)	Summary
CLI flags and option fields cmd/terraform/apply.go, cmd/terraform/plan.go, cmd/terraform/destroy.go, cmd/terraform/options.go, pkg/schema/schema.go	Register --failure-mode for apply/plan/destroy and --affected for destroy; add failure-mode constants and FailureMode to TerraformRunOptions; add TerraformFailureMode, FailFast, KeepGoing to ConfigAndStacksInfo.
Options parsing, apply->info wiring, and tests cmd/terraform/options_test.go, cmd/terraform/utils.go	Parse failure-mode from Viper, extend tests to assert FailureMode, and map TerraformRunOptions.FailureMode into ConfigAndStacksInfo.TerraformFailureMode, FailFast, and KeepGoing.
Context-aware affected execution refactor internal/exec/terraform_affected.go, cmd/terraform/utils.go	Introduce ExecuteTerraformAffectedWithContext, centralize auth/CLI-config resolution for affected runs, create signal-aware contexts, and route affected execution via the scheduler adapter using a selection of node IDs.
Scheduler: selection filtering & failure-mode pkg/scheduler/adapters/terraform.go	Add TerraformSelection and Selection on TerraformOptions; forward selection into FilterTerraformGraph; add deterministic dedupe/sort and selection-based filtering; validate conflicting failure-mode flags; compute effective fail-fast for scheduler wiring; add related tests.
Scheduler tests pkg/scheduler/adapters/terraform_test.go	Graph-selection and failure-mode tests covering selection edge cases, include-dependents behavior, destroy ordering, fail-fast vs keep-going execution, helper validations, and fixtures.
Git worktree-config tolerant opener pkg/git/git.go, errors/errors.go, go.mod	Add fallback that derives repo/git/common dirs via git rev-parse, builds git-rooted fs/storer, strips extensions.worktreeConfig from loaded config, and reopens repo when go-git fails; add sentinel error and tidy go.mod.
Git native helpers & regression tests pkg/git/git_test.go	Add native git helpers and regression tests validating behavior with extensions.worktreeConfig and linked detached worktrees.
End-to-end terraform affected DAG integration test tests/terraform_floci_dag_test.go	Add integration test exercising terraform plan/apply/destroy --affected across base/head fixture repos; assert Floci SSM marker lifecycle and execution-summary processed node IDs; add supporting helpers.
CLI help snapshots tests/snapshots/*	Update apply and plan help golden snapshots to include the new --failure-mode flag.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• cloudposse/atmos#2465: Related scheduler core and Terraform execution wiring referenced by these changes.
• cloudposse/atmos#2474: Overlaps on scheduler-backed Terraform execution and destroy execution-order changes.
• cloudposse/atmos#2484: Overlaps on refactoring atmos terraform affected execution flow and component filtering.

Suggested labels

minor

Suggested reviewers

• aknysh
• osterman

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.73% 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 accurately summarizes the main change: routing Terraform affected execution through the scheduler adapter.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/dag-terraform-affected-scheduler

---

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: 3

🧹 Nitpick comments (3)

pkg/git/git.go (2)

263-265: 💤 Low value

Wrap error with static error from errors/errors.go.

Per coding guidelines, errors should be wrapped using static errors. Consider defining a static error for this case.

Example fix

+// In errors/errors.go:
+var ErrUnexpectedGitOutput = errors.New("unexpected git rev-parse output")

// In git.go:
-		return "", "", "", fmt.Errorf("unexpected git rev-parse output")
+		return "", "", "", errUtils.ErrUnexpectedGitOutput

🤖 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/git/git.go` around lines 263 - 265, Replace the ad-hoc
fmt.Errorf("unexpected git rev-parse output") returned in the length check (the
if len(lines) != 3 block in pkg/git/git.go) with a wrapped static error from
errors/errors.go: add a descriptive exported sentinel error (e.g. var
ErrUnexpectedRevParseOutput = errors.New("unexpected git rev-parse output")) in
errors/errors.go, then return fmt.Errorf("%w: got %d lines",
errors.ErrUnexpectedRevParseOutput, len(lines)) from the function containing the
len(lines) != 3 check so callers can match the static error while still getting
context.

---

256-260: ⚡ Quick win

Consider adding a timeout to prevent indefinite hangs.

The exec.Command call has no context or timeout. If git becomes unresponsive (e.g., waiting on network for a remote), this could block indefinitely.

Proposed fix using context with timeout

-func gitRepositoryPaths(path string) (repoRoot string, gitDir string, commonDir string, err error) {
-	out, err := exec.Command("git", "-C", path, "rev-parse", "--path-format=absolute", "--show-toplevel", "--git-dir", "--git-common-dir").Output()
+import (
+	"context"
+	"time"
+)
+
+func gitRepositoryPaths(path string) (repoRoot string, gitDir string, commonDir string, err error) {
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	out, err := exec.CommandContext(ctx, "git", "-C", path, "rev-parse", "--path-format=absolute", "--show-toplevel", "--git-dir", "--git-common-dir").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/git/git.go` around lines 256 - 260, The gitRepositoryPaths function uses
exec.Command without a timeout and can hang; change it to use a context with
timeout (e.g., context.WithTimeout) and call exec.CommandContext instead of
exec.Command, ensure you defer cancel(), pass the context into the command
invocation for the "git -C ... rev-parse ..." call, and propagate/return the
error if the context deadline is exceeded so the function fails fast on timeout
while keeping the same return values (repoRoot, gitDir, commonDir, err).

pkg/git/git_test.go (1)

289-308: ⚖️ Poor tradeoff

Consider adding a negative-path test for the fallback logic.

These tests verify the fallback triggers when extensions.worktreeConfig is enabled. Per guidelines, when testing recovery logic, include a test verifying the recovery does not trigger when the condition is absent.

A test confirming that standard go-git opening succeeds (without fallback) when worktreeConfig is disabled would strengthen confidence that the detection logic doesn't over-match.

Also applies to: 310-324

🤖 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/git/git_test.go` around lines 289 - 308, Add a negative-path test
alongside TestGetLocalRepoWithWorktreeConfigExtension that verifies the fallback
is NOT used when extensions.worktreeConfig is absent/false: create a test (e.g.,
TestGetLocalRepoWithoutWorktreeConfigExtension) that sets up the repo with
initNativeGitRepo, does not enable extensions.worktreeConfig (or explicitly set
it to "false" via runNativeGit), calls GetLocalRepo(), asserts no error and
non-nil repo, then calls GetRepoConfig() and GetRepoInfo() and assert expected
values (use require.NoError and requireSamePath/require.Equal as in the existing
test) to confirm standard go-git opening succeeded and recovery/fallback logic
did not trigger; apply the same pattern for the other similar test referenced
around lines 310-324.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/terraform/utils.go`:
- Line 343: The function signature for executeAffectedCommand currently places
ctx last; change it to accept ctx context.Context as the first parameter (func
executeAffectedCommand(ctx context.Context, parentCmd *cobra.Command, args
[]string, info *schema.ConfigAndStacksInfo) error) and update every call site to
pass the context as the first argument; ensure any references/imports still
compile and run go vet/go test to catch missed call sites or mismatched
ordering.

In `@internal/exec/terraform_utils_test.go`:
- Around line 357-358: The test hardcodes "/tmp/base" — change it to use a temp
dir: call repoPath := t.TempDir() (or t.TempDir() per table subtests) and build
the expected path with filepath.Join(repoPath, "base"), then assert
require.Equal(t, filepath.Join(repoPath, "base"), args.RepoPath) and keep
require.Equal(t, "dev", args.Stack); apply the same change to the other
occurrence mentioned (lines 423-424) so no Unix-specific hardcoded paths remain.

In `@pkg/scheduler/adapters/terraform.go`:
- Around line 238-239: The fast-path currently uses len(selection.NodeIDs) ==
graph.Size() which can be spoofed by duplicates or invalid IDs; change the
condition to verify that the set of unique IDs in selection.NodeIDs matches
graph.Size() and that every ID actually exists in the graph before returning
early (while still honoring selection.IncludeDependencies and
selection.IncludeDependents). In practice compute a unique ID set from
selection.NodeIDs, check len(uniqueIDs) == graph.Size() and validate each ID
against the graph (e.g., via graph.Has(id) or equivalent) and only then return
graph when IncludeDependencies and IncludeDependents are false.

---

Nitpick comments:
In `@pkg/git/git_test.go`:
- Around line 289-308: Add a negative-path test alongside
TestGetLocalRepoWithWorktreeConfigExtension that verifies the fallback is NOT
used when extensions.worktreeConfig is absent/false: create a test (e.g.,
TestGetLocalRepoWithoutWorktreeConfigExtension) that sets up the repo with
initNativeGitRepo, does not enable extensions.worktreeConfig (or explicitly set
it to "false" via runNativeGit), calls GetLocalRepo(), asserts no error and
non-nil repo, then calls GetRepoConfig() and GetRepoInfo() and assert expected
values (use require.NoError and requireSamePath/require.Equal as in the existing
test) to confirm standard go-git opening succeeded and recovery/fallback logic
did not trigger; apply the same pattern for the other similar test referenced
around lines 310-324.

In `@pkg/git/git.go`:
- Around line 263-265: Replace the ad-hoc fmt.Errorf("unexpected git rev-parse
output") returned in the length check (the if len(lines) != 3 block in
pkg/git/git.go) with a wrapped static error from errors/errors.go: add a
descriptive exported sentinel error (e.g. var ErrUnexpectedRevParseOutput =
errors.New("unexpected git rev-parse output")) in errors/errors.go, then return
fmt.Errorf("%w: got %d lines", errors.ErrUnexpectedRevParseOutput, len(lines))
from the function containing the len(lines) != 3 check so callers can match the
static error while still getting context.
- Around line 256-260: The gitRepositoryPaths function uses exec.Command without
a timeout and can hang; change it to use a context with timeout (e.g.,
context.WithTimeout) and call exec.CommandContext instead of exec.Command,
ensure you defer cancel(), pass the context into the command invocation for the
"git -C ... rev-parse ..." call, and propagate/return the error if the context
deadline is exceeded so the function fails fast on timeout while keeping the
same return values (repoRoot, gitDir, commonDir, err).

🪄 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: 8ceb4404-1bf3-4e97-a1e6-ccd12ee31d21

📥 Commits

Reviewing files that changed from the base of the PR and between 4799609 and d2f1ec4.

📒 Files selected for processing (19)

• cmd/terraform/apply.go
• cmd/terraform/destroy.go
• cmd/terraform/options.go
• cmd/terraform/options_test.go
• cmd/terraform/plan.go
• cmd/terraform/utils.go
• go.mod
• internal/exec/terraform_affected.go
• internal/exec/terraform_utils_test.go
• pkg/git/git.go
• pkg/git/git_test.go
• pkg/scheduler/adapters/terraform.go
• pkg/scheduler/adapters/terraform_test.go
• pkg/schema/schema.go
• tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden
• tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden
• tests/snapshots/TestCLICommands_config_alias_tp_--help_shows_terraform_plan_help.stdout.golden
• tests/snapshots/TestCLICommands_tf_plan_help_shows_inherited_stack_flag.stdout.golden
• tests/terraform_floci_dag_test.go

[mergify]

💥 This pull request now has conflicts. Could you fix it Mikhail Shirkov (@shirkevich)? 🙏

[Mikhail Shirkov (shirkevich)]

CodeRabbit (@coderabbitai) review

[coderabbitai]

✅ 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

♻️ Duplicate comments (1)

internal/exec/terraform_utils_test.go (1)

357-358: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Replace hardcoded Unix repo path with a temp dir.

"/tmp/base" makes this test platform-specific. Use a per-test temp dir and assert against that variable in both spots.

Suggested patch.

+	repoPath := t.TempDir()
+
 	patches.ApplyFunc(getAffectedComponents, func(args *DescribeAffectedCmdArgs) ([]schema.Affected, error) {
 		describedAffected = true
 		require.NotNil(t, args.CLIConfig)
-		require.Equal(t, "/tmp/base", args.RepoPath)
+		require.Equal(t, repoPath, args.RepoPath)
 		require.Equal(t, "dev", args.Stack)
 		require.True(t, args.ProcessTemplates)
@@
 	args := &DescribeAffectedCmdArgs{
-		RepoPath:             "/tmp/base",
+		RepoPath:             repoPath,
 		Stack:                "dev",
 		IncludeDependents:    true,
 		ProcessTemplates:     true,

As per coding guidelines: “Never hardcode Unix paths in expected values like assert.Equal(t, "/project/components/vpc", path); build expected paths with filepath.Join().”

Also applies to: 423-424

🤖 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_utils_test.go` around lines 357 - 358, Replace the
hardcoded "/tmp/base" in the test with a per-test temp directory: call tmp :=
t.TempDir() (or os.MkdirTemp if not using testing.T helpers), build any expected
subpaths with filepath.Join(tmp, "base") if needed, and replace the literal
"/tmp/base" in the require.Equal assertions that compare args.RepoPath with the
expected path (and the other occurrence of the same assertion later). Keep the
require.Equal for args.Stack as-is, and ensure you import "path/filepath" if you
use filepath.Join.

🤖 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/git/git.go`:
- Around line 256-271: Replace the raw fmt.Errorf return in gitRepositoryPaths
with a wrapped static error defined in errors/errors.go: add a descriptive
exported error (e.g., ErrUnexpectedRevParseOutput) to errors/errors.go and in
gitRepositoryPaths wrap that static error with context (including the actual
output or length) using fmt.Errorf("%w: %s", errors.ErrUnexpectedRevParseOutput,
detail) before returning; ensure the function imports the errors package and
returns the wrapped static error instead of fmt.Errorf directly.

---

Duplicate comments:
In `@internal/exec/terraform_utils_test.go`:
- Around line 357-358: Replace the hardcoded "/tmp/base" in the test with a
per-test temp directory: call tmp := t.TempDir() (or os.MkdirTemp if not using
testing.T helpers), build any expected subpaths with filepath.Join(tmp, "base")
if needed, and replace the literal "/tmp/base" in the require.Equal assertions
that compare args.RepoPath with the expected path (and the other occurrence of
the same assertion later). Keep the require.Equal for args.Stack as-is, and
ensure you import "path/filepath" if you use filepath.Join.

🪄 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: 83663c2f-8f49-4b40-a6de-5d43c9a181c0

📥 Commits

Reviewing files that changed from the base of the PR and between d2f1ec4 and 1b7ad06.

📒 Files selected for processing (19)

• cmd/terraform/apply.go
• cmd/terraform/destroy.go
• cmd/terraform/options.go
• cmd/terraform/options_test.go
• cmd/terraform/plan.go
• cmd/terraform/utils.go
• go.mod
• internal/exec/terraform_affected.go
• internal/exec/terraform_utils_test.go
• pkg/git/git.go
• pkg/git/git_test.go
• pkg/scheduler/adapters/terraform.go
• pkg/scheduler/adapters/terraform_test.go
• pkg/schema/schema.go
• tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden
• tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden
• tests/snapshots/TestCLICommands_config_alias_tp_--help_shows_terraform_plan_help.stdout.golden
• tests/snapshots/TestCLICommands_tf_plan_help_shows_inherited_stack_flag.stdout.golden
• tests/terraform_floci_dag_test.go

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

• tests/snapshots/TestCLICommands_tf_plan_help_shows_inherited_stack_flag.stdout.golden
• go.mod
• tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden

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

• cmd/terraform/apply.go
• cmd/terraform/options.go
• pkg/schema/schema.go
• cmd/terraform/utils.go
• tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden
• cmd/terraform/plan.go
• cmd/terraform/destroy.go
• pkg/scheduler/adapters/terraform.go
• internal/exec/terraform_affected.go
• pkg/git/git_test.go
• tests/terraform_floci_dag_test.go
• pkg/scheduler/adapters/terraform_test.go
• cmd/terraform/options_test.go

[Mikhail Shirkov (shirkevich)]

CodeRabbit (@coderabbitai) review

[coderabbitai]

✅ 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: 0

[coderabbitai]

✅ 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.

[mergify]

💥 This pull request now has conflicts. Could you fix it Mikhail Shirkov (@shirkevich)? 🙏

[Mikhail Shirkov (shirkevich)]

CodeRabbit (@coderabbitai) review

[coderabbitai]

✅ 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]

🧹 Nitpick comments (1)

cmd/terraform/destroy.go (1)

25-30: ⚡ Quick win

Wrap the bind errors with context.

These return the raw error straight from the parser. A little %w context makes flag-binding failures far easier to trace in logs.

♻️ Suggested wrap

-		if err := terraformParser.BindFlagsToViper(cmd, v); err != nil {
-			return err
-		}
-		if err := destroyParser.BindFlagsToViper(cmd, v); err != nil {
-			return err
-		}
+		if err := terraformParser.BindFlagsToViper(cmd, v); err != nil {
+			return fmt.Errorf("binding terraform flags: %w", err)
+		}
+		if err := destroyParser.BindFlagsToViper(cmd, v); err != nil {
+			return fmt.Errorf("binding destroy flags: %w", err)
+		}

As per coding guidelines: "use fmt.Errorf with %w for context".

🤖 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 `@cmd/terraform/destroy.go` around lines 25 - 30, Wrap the raw errors returned
by terraformParser.BindFlagsToViper and destroyParser.BindFlagsToViper with
contextual messages using fmt.Errorf and %w; specifically, when binding flags on
cmd with v fails, return fmt.Errorf("failed to bind terraform flags to viper:
%w", err) for terraformParser.BindFlagsToViper and a similar contextual message
(e.g. "failed to bind destroy flags to viper: %w") for
destroyParser.BindFlagsToViper so logs clearly show which binding failed.

🤖 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.

Nitpick comments:
In `@cmd/terraform/destroy.go`:
- Around line 25-30: Wrap the raw errors returned by
terraformParser.BindFlagsToViper and destroyParser.BindFlagsToViper with
contextual messages using fmt.Errorf and %w; specifically, when binding flags on
cmd with v fails, return fmt.Errorf("failed to bind terraform flags to viper:
%w", err) for terraformParser.BindFlagsToViper and a similar contextual message
(e.g. "failed to bind destroy flags to viper: %w") for
destroyParser.BindFlagsToViper so logs clearly show which binding failed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d53f410d-c815-408a-873d-1fe188be0b4f

📥 Commits

Reviewing files that changed from the base of the PR and between 78f9c66 and 1ed7014.

📒 Files selected for processing (20)

• cmd/terraform/apply.go
• cmd/terraform/destroy.go
• cmd/terraform/options.go
• cmd/terraform/options_test.go
• cmd/terraform/plan.go
• cmd/terraform/utils.go
• errors/errors.go
• go.mod
• internal/exec/terraform_affected.go
• internal/exec/terraform_utils_test.go
• pkg/git/git.go
• pkg/git/git_test.go
• pkg/scheduler/adapters/terraform.go
• pkg/scheduler/adapters/terraform_test.go
• pkg/schema/schema.go
• tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden
• tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden
• tests/snapshots/TestCLICommands_config_alias_tp_--help_shows_terraform_plan_help.stdout.golden
• tests/snapshots/TestCLICommands_tf_plan_help_shows_inherited_stack_flag.stdout.golden
• tests/terraform_floci_dag_test.go

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

• tests/snapshots/TestCLICommands_tf_plan_help_shows_inherited_stack_flag.stdout.golden
• tests/snapshots/TestCLICommands_config_alias_tp_--help_shows_terraform_plan_help.stdout.golden
• tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden
• tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden

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

• cmd/terraform/plan.go
• cmd/terraform/apply.go
• go.mod
• pkg/schema/schema.go
• errors/errors.go
• cmd/terraform/utils.go
• internal/exec/terraform_affected.go
• cmd/terraform/options_test.go
• cmd/terraform/options.go
• pkg/scheduler/adapters/terraform_test.go
• pkg/git/git_test.go
• pkg/scheduler/adapters/terraform.go
• pkg/git/git.go
• internal/exec/terraform_utils_test.go
• tests/terraform_floci_dag_test.go

[Erik Osterman (Cloud Posse) (osterman)]

CodeRabbit (@coderabbitai) can you confirm that with --failure-mode=fail-fast it will still wait for inflight processes to complete (and simply avoid spawning more)? We don't want to leave lock files.

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find and examine the scheduler implementation, focusing on fail-fast behavior
rg -n "WithFailFast\|FailFast\|failFast\|in.flight\|inflight\|running\|drain\|cancel\|wait" pkg/scheduler/ --type=go -A 3 -B 1 | head -200

Length of output: 473

---

🏁 Script executed:

#!/bin/bash
# Find the scheduler run loop to see how it handles failures
fd -e go . pkg/scheduler/ --exclude '*_test.go' | xargs cat

Length of output: 54968

---

---

@osterman — confirmed. With --failure-mode=fail-fast, in-flight processes are always allowed to run to completion. No process is killed mid-execution.

Here's the key path in pkg/scheduler/scheduler.go:

// When a node fails with fail-fast enabled:
if s.failFast {
    stopping = true   // ← stops dispatching NEW nodes
    finished += skipPending(...)  // ← marks queued-but-not-started nodes as skipped
}

// The run loop continues to drain completions from in-flight workers:
for finished < len(s.graph.Nodes) {
    select {
    case out <- nextID:   // out is nil when stopping=true → no new work dispatched
    case event := <-eventCh:  // ← still receives from all running workers
        running--
        finished++
        ...
    case <-done:  // done is nil when stopping=true → context not cancelled mid-drain
    }
}

close(workCh)    // only after every node is accounted for
workers.Wait()   // waits for all goroutines to exit cleanly
// defer cancel() fires here — AFTER all workers have finished

The behavior is exactly what you want:

State	fail-fast behavior
Not-yet-started nodes	Skipped immediately
Currently running nodes	Allowed to complete naturally
Context cancellation	Only after all workers drain (defer cancel())

No Terraform process is interrupted mid-execution, so lock files won't be orphaned.

[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. Ask AI.