cloudposse
diff --git a/‎CLAUDE.md‎
Lines changed: 8 additions & 0 deletions b/‎CLAUDE.md‎
Lines changed: 8 additions & 0 deletions
diff --git a/‎docs/fixes/2026-03-20-executeterraform-refactor.md‎
Lines changed: 225 additions & 0 deletions b/‎docs/fixes/2026-03-20-executeterraform-refactor.md‎
Lines changed: 225 additions & 0 deletions
@@ -362,6 +362,14 @@ Always ask first: "This will discard uncommitted changes. Proceed? [y/N]"
 ### Test Coverage (MANDATORY)
 80% minimum (CodeCov enforced). All features need tests. `make testacc-coverage` for reports.
 
+### Cyclomatic Complexity (MANDATORY)
+golangci-lint enforces `cyclop: max-complexity: 15` and `funlen: lines: 60, statements: 40`.
+When refactoring high-complexity functions:
+1. Extract blocks with clear single responsibilities into named helper functions.
+2. Use the pattern: `buildXSubcommandArgs`, `resolveX`, `checkX`, `assembleX`, `handleX`.
+3. Keep the orchestrator function as a flat linear pipeline of named steps (see `ExecuteTerraform`).
+4. Previously high-complexity functions: `ExecuteTerraform` (160→26, see `internal/exec/terraform.go`), `ExecuteDescribeStacks` (247→10), `processArgsAndFlags`.
+
 ### Environment Variables (MANDATORY)
 Use `viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")` - ATMOS_ prefix required.
 
 
@@ -0,0 +1,225 @@
+# `ExecuteTerraform` Refactor Audit — Findings and Fixes
+
+**Date:** 2026-03-20
+
+**Related PR:** #2226 — `refactor(terraform): reduce ExecuteTerraform cyclomatic complexity 160→26 with 100+ unit tests`
+
+**Severity:** Low–Medium — test quality improvements, no production bugs found
+
+---
+
+## What PR #2226 Did
+
+PR #2226 refactored `ExecuteTerraform` — formerly a ~900-line monolith with cyclomatic
+complexity 160 — into a clean pipeline of 26 focused helper functions across 4 files:
+
+- **`terraform.go`** (185 lines) — orchestrator: `ExecuteTerraform` → `prepareComponentExecution`
+  → `executeCommandPipeline` → `cleanupTerraformFiles`.
+- **`terraform_execute_helpers.go`** (512 lines) — auth, env vars, init, validation helpers.
+- **`terraform_execute_helpers_args.go`** (156 lines) — argument builders for plan/apply/init/workspace.
+- **`terraform_execute_helpers_exec.go`** (322 lines) — execution pipeline, workspace setup, TTY, cleanup.
+
+All files are within the 600-line limit. 100+ unit tests were added across 6 test files.
+
+### Key design decisions
+
+- Injectable test vars (`defaultMergedAuthConfigGetter`, `defaultComponentConfigFetcher`,
+  `defaultAuthManagerCreator`) enable isolated unit testing of auth paths without real
+  infrastructure.
+- Mutual exclusion between `executeTerraformInitPhase` and `buildInitSubcommandArgs` is
+  well-documented (they share `prepareInitExecution` but are guarded by
+  `SubCommand == "init"` branching).
+
+---
+
+## Audit Findings
+
+### Finding 1: Duplicate `resolveExitCode` tests across files
+
+**Severity:** Low (test duplication)
+
+`resolveExitCode` was tested in both `terraform_execute_helpers_test.go` (5 test functions
+including `os/exec.ExitError`) and `terraform_execute_helpers_pipeline_test.go` (4 test
+functions, subset). The `_test.go` version is a superset.
+
+**Fix:** Removed the 4 duplicate tests from `_pipeline_test.go`. The 5 canonical tests in
+`_test.go` remain.
+
+### Finding 2: Tautological cleanup test
+
+**Severity:** Low (misleading test)
+
+`TestCleanupTerraformFiles_ApplyRemovesVarFile` in `_args_test.go` created a file but only
+asserted `NotPanics` — never verifying the file was removed. A proper file-removal test already
+exists in `_coverage_test.go` (`TestCleanupTerraformFiles_ApplyRemovesVarfileForReal`).
+
+**Fix:** Clarified the test's purpose in its comment — it tests graceful handling of
+mismatched path layouts, not actual removal. The real removal test is in `_coverage_test.go`.
+
+### Finding 3: No tests for `resolveAndProvisionComponentPath`
+
+**Severity:** High (coverage gap)
+
+This ~55-line function handles auto-generate files, JIT source provisioning, workdir path
+override, and re-checking directory existence. It has zero dedicated unit tests. Testing
+requires mocking `generateConfigFiles`, `PrepareComponentSourceForJITProvisioning`, and
+filesystem operations.
+
+**Status:** Documented. Requires significant mocking infrastructure to test properly —
+deferred to a follow-up PR.
+
+### Finding 4: No tests for `generateConfigFiles`, `executeTerraformInitPhase`, `handleVersionSubcommand`
+
+**Severity:** Medium (coverage gaps)
+
+These orchestration functions combine sub-calls but have no tests verifying error propagation
+through the combined flow.
+
+**Status:** Documented. These are orchestration functions that delegate to individually-tested
+helpers; the risk is lower than Finding 3.
+
+### Finding 5: `TestRunWorkspaceSetup_OutputSubcommand_DryRun_RedirectsToStderr` is misleading
+
+**Severity:** Low (misleading test name)
+
+The test claims to exercise the `wsOpts` stderr-redirect branch for "output"/"show"
+subcommands, but because `DryRun=true`, `ExecuteShellCommand` returns nil without ever using
+the options. The test only verifies the function does not error.
+
+**Status:** Documented. The test name is misleading about what it covers, but the behavior
+is correct.
+
+### Finding 6: Hardcoded `/dev/stdout` in `runWorkspaceSetup`
+
+**Severity:** Low (pre-existing, cross-platform)
+
+`terraform_execute_helpers_exec.go` line 167 uses `"/dev/stdout"` which doesn't exist on
+Windows. This is a pre-existing pattern (not introduced by this PR), and `ExecuteShellCommand`
+already handles Windows conversion for `/dev/null`.
+
+**Status:** Pre-existing. Not a regression.
+
+---
+
+## Audit Pass 7 — CodeRabbit Review Items
+
+### Item 1: Duplicate resolveExitCode tests (Pass 6 #1)
+
+**Status:** Fixed. Removed 4 duplicate tests from `_pipeline_test.go`.
+
+### Item 2: Duplicate TestBuildWorkspaceSubcommandArgs_NoSubCommand2 (Pass 6 #2)
+
+**Status:** Fixed. Removed from `_pipeline_test.go`; canonical test is
+`TestBuildWorkspaceSubcommandArgs_Bare` in `_args_test.go`.
+
+### Item 3: Tautological cleanup tests (Pass 6 #3)
+
+**Status:** Fixed. Removed `TestCleanupTerraformFiles_ApplyRemovesVarFile` and
+`TestCleanupTerraformFiles_PlanSubcommandSkipsCleanup` from `_args_test.go`. Real
+file-assertion tests exist in `_coverage_test.go`.
+
+### Item 4: logTerraformContext coverage theater
+
+**Status:** Kept with documentation. The function is logging-only — it calls `log.Debug`
+which is not capturable in unit tests without injecting a logger. The NotPanics tests
+are the best we can do without over-engineering. Comment added explaining this.
+
+### Item 5: storeAutoDetectedIdentity no-op branch untested
+
+**Status:** Fixed. Added `TestStoreAutoDetectedIdentity_ExistingIdentity_NotOverwritten`
+to `_auth_test.go` with explicit `GetChain().Times(0)` assertion. The existing table-driven
+test in `utils_auth_test.go` also covers this via gomock strict mode, but the explicit
+`Times(0)` makes the contract self-documenting.
+
+### Item 6: Redundant warnOnConflictingEnvVars test
+
+**Status:** Fixed. Removed `TestWarnOnConflictingEnvVars_NoConflictsNoError` from
+`_args_test.go`. The 5 `t.Setenv` tests in `_coverage_test.go` implicitly cover the
+clean-environment path.
+
+### Item 7: assembleComponentEnvVars with tenv != nil untested
+
+**Status:** Fixed. Added `TestAssembleComponentEnvVars_NonNilTenv` that passes a
+zero-value `*dependencies.ToolchainEnvironment`. The `tenv != nil` branch runs. Full
+PATH ordering test requires a real toolchain install (deferred).
+
+### Item 8: printAndWriteVarFiles debug-print branches
+
+**Status:** Fixed. Added 3 tests exercising LogLevelDebug and LogLevelTrace branches:
+- `TestPrintAndWriteVarFiles_DebugLogLevel_Success` — ComponentVarsSection at debug level.
+- `TestPrintAndWriteVarFiles_DebugLogLevel_CliVarsSection` — CLI vars override at debug level.
+- `TestPrintAndWriteVarFiles_TraceLogLevel` — trace level path.
+
+### Item 9: buildTerraformCommandArgs init branch untested
+
+**Status:** Fixed. Added `TestBuildTerraformCommandArgs_Init` that tests the `case "init"`
+dispatch including `-reconfigure` and `-var-file` flags.
+
+### Item 10: GenerateFilesForComponent double-invocation
+
+**Status:** Documented. Added comment to `generateConfigFiles` explaining that
+`autoGenerateComponentFiles` and `generateConfigFiles` serve different purposes (generate
+sections vs backend/provider overrides). Pre-existing behavior, not a regression.
+
+---
+
+## Changes Made
+
+### `internal/exec/terraform.go`
+
+- Add `subcommandApply`, `subcommandDeploy`, `subcommandInit`, `subcommandWorkspace`,
+  `dirPermissions` constants.
+
+### `internal/exec/terraform_execute_helpers.go`
+
+- Fix `hugeParam`: `handleVersionSubcommand` takes pointer params.
+- Fix `unlambda`: `defaultMergedAuthConfigGetter` uses direct function ref.
+- Fix `add-constant`: replace string literals with named constants.
+- Fix `cyclomatic`/`funlen`: extract `autoGenerateComponentFiles`,
+  `provisionComponentSource`, `logAndWriteComponentVars`, `logCliVarsOverrides`.
+- Add comment to `generateConfigFiles` re: double-invocation (item 10).
+
+### `internal/exec/terraform_execute_helpers_args.go`
+
+- Fix `unparam`: remove unused `planFile` from `buildApplySubcommandArgs`.
+- Replace `"apply"`, `"init"`, `"workspace"` with constants.
+
+### `internal/exec/terraform_execute_helpers_exec.go`
+
+- Fix `forbidigo`: nolint for `TF_WORKSPACE`.
+- Fix `nestif`: extract `handlePlanStatusUpload`.
+- Fix `argument-limit`: nolint for variadic opts.
+- Fix `cyclomatic`: extract `shouldSkipWorkspaceSetup`, `runPreExecutionSteps`.
+
+### `internal/exec/terraform_execute_helpers_pipeline_test.go`
+
+- Remove duplicate `resolveExitCode` tests and `TestBuildWorkspaceSubcommandArgs_NoSubCommand2`.
+- Clean up unused imports.
+
+### `internal/exec/terraform_execute_helpers_args_test.go`
+
+- Remove tautological cleanup tests and redundant `warnOnConflictingEnvVars` test.
+- Add `TestAssembleComponentEnvVars_NonNilTenv` (item 7).
+- Add `TestBuildTerraformCommandArgs_Init` (item 9).
+- Add docs for logTerraformContext tests explaining logging-only functions (item 4).
+
+### `internal/exec/terraform_execute_helpers_coverage_test.go`
+
+- Add `TestPrintAndWriteVarFiles_DebugLogLevel_Success` (item 8).
+- Add `TestPrintAndWriteVarFiles_DebugLogLevel_CliVarsSection` (item 8).
+- Add `TestPrintAndWriteVarFiles_TraceLogLevel` (item 8).
+
+### `internal/exec/terraform_execute_helpers_test.go`
+
+- Fix `filepathJoin`: split `"infra/networking"` into separate path segments.
+
+---
+
+## References
+
+- PR #2226: `refactor(terraform): reduce ExecuteTerraform cyclomatic complexity 160→26`
+- `internal/exec/terraform.go` — refactored orchestrator
+- `internal/exec/terraform_execute_helpers.go` — auth/env/init/validation helpers
+- `internal/exec/terraform_execute_helpers_args.go` — argument builders
+- `internal/exec/terraform_execute_helpers_exec.go` — execution pipeline
+- `internal/exec/terraform_execute_helpers_*_test.go` — 6 test files