refactor(terraform): reduce ExecuteTerraform complexity 160→9, improve test coverage

[Copilot]

what

• Extract 30+ focused helper functions from the ExecuteTerraform monolith (~900 lines, cyclomatic complexity 160), reducing complexity to ~9 across 4 source files all under 600 lines
• Add 100+ unit tests across 6 test files covering argument builders, auth setup, workspace management, cleanup, exit-code resolution, and the execution pipeline
• Fix all 16 golangci-lint issues: cyclomatic complexity, magic constants, unused params, forbidden API calls, huge params, nested blocks, argument limits
• Address all 10 CodeRabbit audit pass 7 items: remove duplicate/tautological tests, add missing coverage for tenv != nil, buildTerraformCommandArgs init branch, and storeAutoDetectedIdentity guard
• Document the GenerateFilesForComponent double-invocation when AutoGenerateFiles=true (pre-existing behavior, not a regression)

Architecture after refactor

File	Lines	Responsibility
terraform.go	189	Orchestrator: ExecuteTerraform → prepareComponentExecution → executeCommandPipeline → cleanupTerraformFiles
terraform_execute_helpers.go	546	Auth, env vars, init, validation, config generation helpers
terraform_execute_helpers_args.go	156	Per-subcommand argument builders (plan/apply/init/workspace/destroy)
terraform_execute_helpers_exec.go	350	Execution pipeline, workspace setup, TTY guard, exit-code resolution, cleanup

Key design decisions

• Injectable test vars (defaultMergedAuthConfigGetter, defaultComponentConfigFetcher, defaultAuthManagerCreator) enable isolated unit testing of auth paths without real infrastructure
• Named subcommand constants (subcommandApply, subcommandDeploy, subcommandInit, subcommandWorkspace) replace 16+ magic string literals
• Mutual exclusion contract between executeTerraformInitPhase and buildInitSubcommandArgs is documented — both call prepareInitExecution but are guarded by SubCommand == "init" branching

Lint fixes (16 issues → 0)

• revive/cyclomatic: extract shouldSkipWorkspaceSetup, runPreExecutionSteps, autoGenerateComponentFiles, provisionComponentSource, logAndWriteComponentVars, logCliVarsOverrides, handlePlanStatusUpload
• revive/add-constant: subcommandApply/Deploy/Init/Workspace, dirPermissions
• gocritic/hugeParam: handleVersionSubcommand takes pointer params
• gocritic/unlambda: defaultMergedAuthConfigGetter uses direct function ref
• gocritic/filepathJoin: split path segments in test
• unparam: remove unused planFile from buildApplySubcommandArgs
• forbidigo: nolint for TF_WORKSPACE (Terraform convention)
• nestif: extract handlePlanStatusUpload
• revive/argument-limit: nolint for variadic opts

why

• ExecuteTerraform was the highest-complexity function in the codebase (cyclomatic 160) — untestable as a unit, any change risked regressions in auth, workspace, plan-file, or cleanup logic
• The function handled 10+ distinct responsibilities in a single method: path resolution, auto-generation, JIT provisioning, toolchain deps, auth hooks, env var assembly, init pre-step, argument construction, workspace setup, TTY guard, command execution, status upload, and cleanup
• Breaking it into focused helpers means each responsibility is independently testable, and adding new subcommands or flags requires changes in one place instead of wading through 900 lines
• The 16 lint issues were blocking pre-commit hooks on any file in the same package

references

• Fix doc: docs/fixes/2026-03-20-executeterraform-refactor.md
• Blog post: website/blog/2026-03-18-refactoring-executeterraform.mdx
• Roadmap: website/src/data/roadmap.js (Code Quality initiative milestone)

Summary by CodeRabbit

Release Notes

• Refactor

  • Improved internal code organization and maintainability by decomposing complex Terraform execution logic into smaller, focused components. Complexity metrics reduced significantly.

• Tests

  • Added comprehensive unit test suite (100+ tests) to improve reliability and catch edge cases.

• Documentation

  • Added documentation detailing refactoring approach and audit findings.
  • Published blog post about internal improvements and lessons learned.

• Chores

  • Updated roadmap to reflect completed internal optimization work.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Files

None

[codecov]

Codecov Report

❌ Patch coverage is 80.20408% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.04%. Comparing base (bb6f71d) to head (fefee3e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/exec/terraform_execute_helpers.go	75.76%	41 Missing and 22 partials ⚠️
internal/exec/terraform_execute_helpers_exec.go	81.13%	17 Missing and 13 partials ⚠️
internal/exec/terraform_execute_helpers_args.go	93.10%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2226      +/-   ##
==========================================
+ Coverage   76.94%   77.04%   +0.10%     
==========================================
  Files        1004     1007       +3     
  Lines       95390    95495     +105     
==========================================
+ Hits        73394    73573     +179     
+ Misses      17740    17680      -60     
+ Partials     4256     4242      -14     

Flag	Coverage Δ
unittests	77.04% <80.20%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/exec/terraform.go	79.22% <100.00%> (+14.18%)	⬆️
internal/exec/utils_auth.go	96.34% <100.00%> (ø)
internal/exec/terraform_execute_helpers_args.go	93.10% <93.10%> (ø)
internal/exec/terraform_execute_helpers_exec.go	81.13% <81.13%> (ø)
internal/exec/terraform_execute_helpers.go	75.76% <75.76%> (ø)

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Erik Osterman (Cloud Posse) (osterman)]

CodeRabbit (@coderabbitai) please audit this refactor thank you for potential regressions or change of behavior

[Andriy Knysh (aknysh)]

CodeRabbit (@coderabbitai) full review please, also check if the "Action Items" have been addressed

[Andriy Knysh (aknysh)]

CodeRabbit (@coderabbitai) full review please, also check if the "Action Items" and "Remaining Action Items" have been addressed

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (1)

internal/exec/terraform_execute_helpers_coverage_test.go (1)

331-363: These tests don't verify the warning behavior.

Every case here only asserts “does not panic”, so a regression where warnOnConflictingEnvVars stops warning entirely would still pass. Please capture/assert the warning output, or drop these if log capture is not worth the maintenance cost.

As per coding guidelines, "Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking."

🤖 Prompt for AI Agents

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

In `@internal/exec/terraform_execute_helpers_coverage_test.go` around lines 331 -
363, These tests only assert that warnOnConflictingEnvVars does not panic and
therefore do not verify that a warning is emitted; update each test
(TestWarnOnConflictingEnvVars_TFCLIArgs, _TFWorkspace, _TFVarPrefix,
_TFCLIArgsPrefix, _MultipleConflicts) to capture and assert the warning output
produced by warnOnConflictingEnvVars (e.g., capture logger/stderr or inject a
test logger via DI) and assert the expected warning substring is present, or if
capturing logs is impractical, remove the redundant tests entirely; ensure you
reference and exercise the warnOnConflictingEnvVars function in the updated
tests and assert on the warning message content.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@internal/exec/terraform_execute_helpers_exec.go`:
- Around line 182-186: The default stderr redirect currently hardcodes
"/dev/stdout" which breaks on Windows; change it to use the platform-aware
os.Stdout.Name() when info.RedirectStdErr is empty (import "os"), i.e., set
workspaceSelectRedirectStdErr := os.Stdout.Name() and keep using
info.RedirectStdErr when provided so workspaceSelectRedirectStdErr is
platform-neutral; reference workspaceSelectRedirectStdErr and
info.RedirectStdErr in your change.

In `@internal/exec/terraform_execute_helpers_test.go`:
- Around line 393-400: TestResolveExitCode_OsExecExitError relies on the Unix
"false" binary (osexec.Command("false")) which fails on Windows; update the test
to avoid platform-specific binaries by either branching on runtime.GOOS (skip or
use Windows-appropriate command) or implement the helper-process pattern: spawn
the current test binary (exec.Command(os.Args[0], "-test.run=TestHelperProcess",
"--", "exit", "1")) with a distinguishing env var and a TestHelperProcess
function that calls os.Exit(1), then pass the resulting error into
resolveExitCode to assert it returns 1; reference
TestResolveExitCode_OsExecExitError, resolveExitCode, and the osexec.Command
call when making the change.

In `@internal/exec/terraform_execute_helpers.go`:
- Around line 197-199: The code branch that reads the workdir override using
provWorkdir.WorkdirPathKey should treat an empty string as “not set”; change the
check in the block that currently does if workdirPath, ok :=
info.ComponentSection[provWorkdir.WorkdirPathKey].(string); ok { ... } to
additionally verify workdirPath != "" and only then return (workdirPath, true,
nil); otherwise behave like prepareInitExecution and return as if the override
was not present (i.e., do not signal ok when the string is empty).
- Around line 145-156: The current logic conflates provisioning errors from
provisionComponentSource with a "component does not exist" error; change the
branch so that if err != nil you return the original provisioning error wrapped
with context (e.g., fmt.Errorf("provision component source: %w", err)) and only
construct/return ErrInvalidTerraformComponent when componentPathExists is false;
update the block around provisionComponentSource, componentPathExists and the
ErrInvalidTerraformComponent return (and keep the GetComponentBasePath call for
the non-existent-component message) so auth/network/JIT provisioning failures
propagate instead of being masked.
- Around line 47-60: The fast-path in handleVersionSubcommand currently
hard-codes only the single `version` token and drops additional caller options;
update handleVersionSubcommand to preserve and forward all caller args and
options by including `info.SubCommand2`, `info.AdditionalArgsAndFlags`, and the
`info.DryRun` flag when invoking the executor (do not truncate to just
`info.SubCommand`), and thread any variadic `opts...` through to the underlying
call; specifically, call the same execution helper used elsewhere
(ExecuteTerraform) or Ensure ExecuteShellCommand is invoked with the full
constructed args slice and `opts` forwarded so shell-command overrides and flags
like `-json` are preserved.

---

Nitpick comments:
In `@internal/exec/terraform_execute_helpers_coverage_test.go`:
- Around line 331-363: These tests only assert that warnOnConflictingEnvVars
does not panic and therefore do not verify that a warning is emitted; update
each test (TestWarnOnConflictingEnvVars_TFCLIArgs, _TFWorkspace, _TFVarPrefix,
_TFCLIArgsPrefix, _MultipleConflicts) to capture and assert the warning output
produced by warnOnConflictingEnvVars (e.g., capture logger/stderr or inject a
test logger via DI) and assert the expected warning substring is present, or if
capturing logs is impractical, remove the redundant tests entirely; ensure you
reference and exercise the warnOnConflictingEnvVars function in the updated
tests and assert on the warning message content.

🪄 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: 898ff5e4-0119-4984-92e1-c9066238fa93

📥 Commits

Reviewing files that changed from the base of the PR and between 990b763 and 472d200.

📒 Files selected for processing (15)

• CLAUDE.md
• docs/fixes/2026-03-20-executeterraform-refactor.md
• internal/exec/terraform.go
• internal/exec/terraform_execute_helpers.go
• internal/exec/terraform_execute_helpers_args.go
• internal/exec/terraform_execute_helpers_args_test.go
• internal/exec/terraform_execute_helpers_auth_test.go
• internal/exec/terraform_execute_helpers_coverage_test.go
• internal/exec/terraform_execute_helpers_exec.go
• internal/exec/terraform_execute_helpers_pipeline_test.go
• internal/exec/terraform_execute_helpers_test.go
• internal/exec/terraform_execute_helpers_workspace_test.go
• internal/exec/utils_auth.go
• website/blog/2026-03-18-refactoring-executeterraform.mdx
• website/src/data/roadmap.js

[github-actions]

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

[github-actions]

These changes were released in v1.211.0-rc.2.