Repository navigation
fix(ci): use terraform exit code as the source of truth for CI status - #2382
Merged
Erik Osterman (Cloud Posse) (osterman) merged 9 commits intoMay 3, 2026
Merged
Erik Osterman (Cloud Posse) (osterman) merged 9 commits into
Erik Osterman (Cloud Posse) (osterman) merged 9 commits into
Conversation
…ore terraform runs When `atmos terraform deploy` fails at authentication (or any pre-terraform step), the GitHub Actions job summary used to render "No Changes Applied" with a NO CHANGE badge instead of an APPLY-FAILED badge. The handler enriched the OutputResult with the command error via parseOutputWithError, but writeSummary discarded that argument and buildTemplateContext re-parsed the raw output, so Result.HasErrors was false and the template fell through to the no-changes branch. Thread the enriched result through writeSummary into buildTemplateContext (with a nil fallback) so the failure branch is rendered correctly. Same fix applies to plan and apply. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
May 1, 2026 02:05
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExit codes from Terraform commands are now extracted at the command execution boundary and propagated authoritatively through the CI hooks system. Terraform plan's exit code 2 (changes detected) is treated as success despite being non-zero, and error reconciliation happens before template rendering instead of during output parsing. Tests validate the complete flow from subprocess exit codes to template context. ChangesExit Code Authority & Propagation
Sequence DiagramsequenceDiagram
participant CLI as Terraform CLI Executor
participant Boundary as Command Boundary<br/>(utils.go)
participant Hooks as Hooks System<br/>(hooks.go)
participant CI as CI Executor<br/>(executor.go)
participant Plugin as Plugin Handler<br/>(handlers.go)
participant Template as Template Renderer<br/>(plugin.go)
CLI->>Boundary: Execute command, get output/error
Boundary->>Boundary: Extract exit code from error
Boundary->>Hooks: RunCIHooks(options with ExitCode)
Hooks->>CI: ci.Execute(options with ExitCode)
CI->>Plugin: buildHookContext with exit code
Plugin->>Plugin: parseOutputWithError<br/>(uses ctx.ExitCode as authority)
alt Plan with exit code 2
Plugin->>Plugin: Treat as success (changes)
else Exit code 1 or non-nil error
Plugin->>Plugin: Mark errors/failure
else Exit code 0
Plugin->>Plugin: Success, clear errors
end
Plugin->>Template: writeSummary(enriched result)
Template->>Template: buildTemplateContext<br/>(use result, not re-parse)
Template->>Template: Render with reconciled error state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ci/plugins/terraform/handlers_test.go (1)
1208-1265: ⚡ Quick winConsider table-driving the three failure-summary scenarios.
These tests validate the same behavior shape across commands and are a good fit for one table-driven test to reduce duplication and future drift.
♻️ Suggested refactor sketch.
+func TestFailureSummaryRendering_WithCommandError(t *testing.T) { + p := &Plugin{} + tests := []struct { + name string + command string + wantHeader string + wantBadgePart string + notHeader string + }{ + {"plan", "plan", "Plan Failed for", "PLAN-FAILED-ff0000", "No Changes for"}, + {"apply", "apply", "Apply Failed for", "APPLY-FAILED-ff0000", "No Changes Applied for"}, + {"deploy", "deploy", "Apply Failed for", "APPLY-FAILED-ff0000", "No Changes Applied for"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := newFailureSummaryHookContext(tt.command, fmt.Errorf("identity failed: assume role denied")) + mp := ctx.Provider.(*mockProvider) + var err error + switch tt.command { + case "plan": + err = p.onAfterPlan(ctx) + case "apply": + err = p.onAfterApply(ctx) + default: + err = p.onAfterDeploy(ctx) + } + require.NoError(t, err) + require.Len(t, mp.writer.summaries, 1) + rendered := mp.writer.summaries[0] + assert.Contains(t, rendered, tt.wantHeader) + assert.Contains(t, rendered, tt.wantBadgePart) + assert.NotContains(t, rendered, tt.notHeader) + assert.NotContains(t, rendered, "NO_CHANGE-inactive") + }) + } +}As per coding guidelines:
**/*_test.go: “Use table-driven tests for testing multiple scenarios in Go.”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ci/plugins/terraform/handlers_test.go` around lines 1208 - 1265, Replace the three nearly identical tests TestOnAfterPlan_WithCommandError_RendersFailureSummary, TestOnAfterApply_WithCommandError_RendersFailureSummary, and TestOnAfterDeploy_WithCommandError_RendersFailureSummary with a single table-driven test that iterates over cases for command ("plan","apply","deploy"), expected header substring ("Plan Failed for"/"Apply Failed for"), expected badge ("PLAN-FAILED-ff0000"/"APPLY-FAILED-ff0000"), and uses newFailureSummaryHookContext(cmd, err) to build ctx, calls the appropriate handler (call p.onAfterPlan/onAfterApply/onAfterDeploy based on the case), and asserts on mp := ctx.Provider.(*mockProvider); mp.writer.summaries length and that the rendered summary contains the command-specific header, badge, the error string "identity failed: assume role denied", and does not contain the no-change strings; keep references to Plugin, newFailureSummaryHookContext, mockProvider, and mp.writer.summaries so the assertions target the same symbols as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ci/plugins/terraform/handlers_test.go`:
- Around line 1208-1265: Replace the three nearly identical tests
TestOnAfterPlan_WithCommandError_RendersFailureSummary,
TestOnAfterApply_WithCommandError_RendersFailureSummary, and
TestOnAfterDeploy_WithCommandError_RendersFailureSummary with a single
table-driven test that iterates over cases for command
("plan","apply","deploy"), expected header substring ("Plan Failed for"/"Apply
Failed for"), expected badge ("PLAN-FAILED-ff0000"/"APPLY-FAILED-ff0000"), and
uses newFailureSummaryHookContext(cmd, err) to build ctx, calls the appropriate
handler (call p.onAfterPlan/onAfterApply/onAfterDeploy based on the case), and
asserts on mp := ctx.Provider.(*mockProvider); mp.writer.summaries length and
that the rendered summary contains the command-specific header, badge, the error
string "identity failed: assume role denied", and does not contain the no-change
strings; keep references to Plugin, newFailureSummaryHookContext, mockProvider,
and mp.writer.summaries so the assertions target the same symbols as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0494b83-e04f-46be-af6b-ea3af57d3cab
📒 Files selected for processing (4)
pkg/ci/plugins/terraform/handlers.gopkg/ci/plugins/terraform/handlers_test.gopkg/ci/plugins/terraform/plugin.gopkg/ci/plugins/terraform/plugin_test.go
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
…ge detection Plumb the exit code through HookContext (via RunCIHooks → ExecuteOptions → buildHookContext) and use it as the source of truth in parseOutputWithError: - apply/deploy: HasErrors = (exitCode != 0) - plan: HasErrors = (exitCode == 1); exitCode == 2 also implies HasChanges - other commands: HasErrors = (exitCode != 0) Text parsing is downgraded from the primary failure signal to enrichment only — it still extracts resource counts, output values, and error message bodies, but no longer drives the binary HasErrors / HasChanges decisions. Exit code is authoritative both ways: when it says success, any spurious "Error:" lines from text matching are discarded. This is the structural fix behind the previous commit's symptom: the auth failure leaked through because the regex `^Error:` did not match the captured stderr. With exit code authoritative, any pre-terraform failure (auth, OOM, killed by signal, anything) flips HasErrors=true correctly, regardless of output format. cmd/terraform/utils.go computes the exit code via errUtils.GetExitCode, which already unwraps exec.ExitError, ExecError, exitCoder, and the WorkflowStepError types — so the existing error chains carry it through without further plumbing. RunCIHooks now takes a *RunCIHooksOptions struct (per CLAUDE.md options pattern) since the parameter list grew past the linter's max-args limit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/terraform/utils.go`:
- Around line 60-71: Add a unit test that verifies the command error preserves
its exit-code wrapper before it flows into RunCIHooks: call the routine that
produces cmdErr (e.g., ExecuteShellCommand or the function that returns cmdErr),
assert errors.As(err, &errUtils.ExitCodeError{}) on that real error path, and
then pass that err into the same RunCIHooks plumbing (or simulate building a
RunCIHooksOptions with CommandError: cmdErr and ExitCode:
errUtils.GetExitCode(cmdErr)) to ensure the wrapper contract is preserved;
reference symbols to locate code: ExecuteShellCommand (or the function creating
cmdErr), cmdErr, errUtils.GetExitCode, errUtils.ExitCodeError, and
h.RunCIHooks/RunCIHooksOptions.
In `@pkg/ci/plugins/terraform/handlers_test.go`:
- Around line 1321-1339: The test
TestOnAfterApply_WithExitCodeOnly_RendersFailureSummary currently sets
ctx.ExitCode=1 but leaves checks disabled, so updateCheckRun() (which calls
resolveCheckResult(ctx) that only inspects CommandError) is not exercised for
the exit-code-only failure; enable checks on the fixture (set ctx.ChecksEnabled
= true or use the provider/mock fixture flag that turns on checks) before
calling p.onAfterApply(ctx) and then assert that the mockProvider's check update
recorded a "failure" result (inspect mp.updateCheckCalls or similar mock check
record used by updateCheckRun), ensuring updateCheckRun(ctx) treats
ExitCode-only failures as failure. Also keep the existing summary assertions in
TestOnAfterApply_WithExitCodeOnly_RendersFailureSummary.
In `@pkg/ci/plugins/terraform/handlers.go`:
- Around line 287-305: The code currently treats any non-nil ctx.CommandError as
a failure which flips a legitimate Terraform "plan" exit code 2 into an error;
update the reconciliation in handlers.go so CommandError only forces
hasErrors/ExitCode change when the ExitCode is not 2: change the guard from "if
ctx.CommandError != nil { hasErrors = true ... }" to "if ctx.CommandError != nil
&& result.ExitCode != 2 { hasErrors = true; if result.ExitCode == 0 {
result.ExitCode = 1 } }" so a detailed-exitcode plan (ExitCode==2) is preserved,
and ensure the downstream block that populates result.Errors remains driven by
hasErrors (so it won’t inject the CommandError when ExitCode==2); also add a
regression test exercising runHooksOnErrorWithOutput to forward both
CommandError and ExitCode==2 and assert the result still indicates "changes
detected" (ExitCode 2, HasErrors false/no error body).
🪄 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: 300321e9-1959-47d1-808e-e48346970cd4
📒 Files selected for processing (7)
cmd/terraform/utils.gopkg/ci/executor.gopkg/ci/internal/plugin/types.gopkg/ci/plugins/terraform/handlers.gopkg/ci/plugins/terraform/handlers_test.gopkg/hooks/hooks.gopkg/hooks/hooks_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2382 +/- ##
==========================================
+ Coverage 77.95% 78.06% +0.10%
==========================================
Files 1090 1091 +1
Lines 103078 103248 +170
==========================================
+ Hits 80355 80601 +246
+ Misses 18316 18226 -90
- Partials 4407 4421 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… wrapped Address CodeRabbit review on PR #2382: - parseOutputWithError no longer flips HasErrors=true when ctx.CommandError is set for `terraform plan -detailed-exitcode` exit code 2. The shell wrapper turns the non-zero exit into an ExitCodeError, so CommandError is non-nil for the success case — the override regressed the summary from "Changes detected" to "Plan Failed". - resolveCheckResult now treats non-zero ExitCode as failure too (with the same plan/2 carve-out) so the check run cannot disagree with the summary. - New regression tests cover plan/2 + wrapped error, exit-code-only check runs, and the ExitCodeError wrapping contract at the cmd/terraform → internal/exec boundary. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address CodeRabbit follow-up + codecov patch-coverage gap (67% → ≥80%): - cmd/terraform/utils_exit_wrapping_test.go now feeds the wrapped cmdErr produced by ExecuteShellCommand through a real RunCIHooksOptions and invokes h.RunCIHooks (ci.enabled=false short-circuits). This proves the ExitCodeError wrapper survives the entire boundary CodeRabbit asked about, not just the GetExitCode call. - pkg/hooks/hooks_test.go gains tests covering RunCIHooks ForwardsErrorAndExitCode (nil/code 1/code 2), nil AtmosConfig, and experimental=disable error paths. Lifts pkg/hooks/RunCIHooks coverage from 55.6% → 100%. - cmd/terraform/utils_hooks_test.go covers runHooks/runHooksOnError/ runHooksOnErrorWithOutput/runCIHooksForDeploy against the demo-stacks fixture (which has ci.enabled=false → clean RunCIHooks short-circuit). Lifts those wrapper functions from 0% to 65–100%. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/terraform/utils_hooks_test.go (1)
49-79: ⚡ Quick winAdd a plain-error test case to cover the documented default fallback.
The table tests
ExitCodeError{Code:1}andExitCodeError{Code:2}, but theutils.gocomment explicitly documents a third case:GetExitCode"returns 1 by default for non-nil errors with no attached code (e.g., auth failures)." This default is the whole motivation for the PR — pre-exec failures (auth, OOM, signals) produce plain errors with no wrapped exit code, and without this case there's no regression guard for it.✅ Proposed addition
import ( "testing" + "errors" "github.com/spf13/cobra" ... ) tests := []struct { name string cmdErr error wantExt int }{ { name: "wrapped ExitCodeError code 1", cmdErr: errUtils.ExitCodeError{Code: 1}, wantExt: 1, }, { name: "wrapped ExitCodeError code 2 (plan changes detected)", cmdErr: errUtils.ExitCodeError{Code: 2}, wantExt: 2, }, + { + name: "plain error defaults to exit code 1 (e.g. auth failure)", + cmdErr: errors.New("auth failure: credentials not found"), + wantExt: 1, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/terraform/utils_hooks_test.go` around lines 49 - 79, Add a third table test case that covers a plain non-wrapped error so GetExitCode falls back to 1: add an entry with name like "plain error falls back to 1", cmdErr set to a plain error (e.g., errors.New("auth failed")), and wantExt 1; in the same t.Run block keep the assert.Equal(t, tc.wantExt, errUtils.GetExitCode(tc.cmdErr)) and call runHooksOnError(hooks.AfterTerraformPlan, cmd, []string{"--stack","dev","myapp"}, tc.cmdErr) to exercise the short-circuit path. Ensure you import/errors.New or use the existing errors package consistently and mirror the structure of the other table entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/terraform/utils_hooks_test.go`:
- Around line 49-79: Add a third table test case that covers a plain non-wrapped
error so GetExitCode falls back to 1: add an entry with name like "plain error
falls back to 1", cmdErr set to a plain error (e.g., errors.New("auth failed")),
and wantExt 1; in the same t.Run block keep the assert.Equal(t, tc.wantExt,
errUtils.GetExitCode(tc.cmdErr)) and call
runHooksOnError(hooks.AfterTerraformPlan, cmd,
[]string{"--stack","dev","myapp"}, tc.cmdErr) to exercise the short-circuit
path. Ensure you import/errors.New or use the existing errors package
consistently and mirror the structure of the other table entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f67ff115-54c0-44dc-a575-f4002886a9d1
📒 Files selected for processing (3)
cmd/terraform/utils_exit_wrapping_test.gocmd/terraform/utils_hooks_test.gopkg/hooks/hooks_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/terraform/utils_exit_wrapping_test.go
![atmos-pro[bot]](https://avatars.githubusercontent.com/in/927493?s=60&v=4){kind=small}
Igor Rodionov (goruha)
approved these changes
May 2, 2026
Erik Osterman (Cloud Posse) (osterman)
deleted the
osterman/ci-summary-error-status
branch
May 3, 2026 00:27
Contributor
|
These changes were released in v1.217.0-rc.4. |
This was referenced May 19, 2026
This was referenced May 26, 2026
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
2 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
terraform planwith-detailed-exitcode, for change detection) in the CI summary path. Text parsing of stdout/stderr is downgraded to enrichment only — it still extracts resource counts, output values, and error message bodies, but no longer drives the binaryHasErrors/HasChangesdecisions.cmd/terraform/utils.go→pkg/hooks RunCIHooks→pkg/ci ExecuteOptions→plugin.HookContextso the plugin handler has a clean signal independent of output format.parseOutputWithError(pkg/ci/plugins/terraform/handlers.go) so that:apply/deploy:HasErrors = (exitCode != 0)plan:HasErrors = (exitCode == 1);exitCode == 2also impliesHasChangesHasErrors = (exitCode != 0)CommandError.Error()for the body if text parsing didn't find one.*plugin.OutputResultfromparseOutputWithErrorthroughwriteSummaryandbuildTemplateContext(it had been silently dropped —writeSummaryhad_ *plugin.OutputResultas its second arg, andbuildTemplateContextre-parsedctx.Outputfrom scratch).buildTemplateContextkeeps anil-fallback so legacy callers continue to work.RunCIHooksto take a*RunCIHooksOptionsstruct (per the repo's options pattern) since the parameter list grew past the linter's max-args limit.HasChangesfor plan, apply exit 0 with strayError:in output → no error, plus the original failure-summary tests for plan/apply/deploy.why
atmos terraform deploy <component> -s <stack> --upload-statusfailing at the authentication step (before terraform itself ran, exit code 1) still produced a job summary that read## No Changes Applied for eks/karpenter-node-pool in e98d-gov-use1-dsswith aNO CHANGEbadge. The check run was correctly marked failed, but the summary contradicted it.ExtractErrors's^Error:regex (it's emitted as**Error:**in markdown form), andwriteSummarysilently dropped the already-enrichedOutputResult, so the apply template fell through to the no-changes branch. Anything that fails before terraform runs — auth, OOM, signal kill, network — would have hit the same bug.apply: 0 = success / non-zero = error;plan -detailed-exitcode: 0/1/2). Using them as the authoritative signal makes the hook robust against output-format drift between Terraform and OpenTofu, and against any pre-terraform failure that produces no parseable output.errUtils.GetExitCodealready unwrapsexec.ExitError,ExecError,exitCoder, andWorkflowStepError, so the existing error chains carry it through without further plumbing.references
pkg/ci/plugins/terraform/handlers.go(parseOutputWithError,writeSummary).pkg/ci/plugins/terraform/plugin.go(buildTemplateContext).pkg/ci/internal/plugin/types.go,pkg/ci/executor.go,pkg/hooks/hooks.go,cmd/terraform/utils.go.pkg/ci/plugins/terraform/templates/{apply,plan}.mdalready had{{ if .Result.HasErrors }}branches; they just weren't being reached.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
planandapplyoperations, properly handling plan changes and command execution failures.Tests