Skip to content

docs: add PRD for pro summary upload - #2259

Closed
Igor Rodionov (goruha) wants to merge 15 commits into
mainfrom
prd/pro-summary-upload
Closed

Igor Rodionov (goruha) wants to merge 15 commits into
mainfrom
prd/pro-summary-upload

Conversation

@goruha

@goruha Igor Rodionov (goruha) commented Mar 26, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Adds PRD for uploading structured CI summary data to Atmos Pro via the existing instance status PATCH endpoint
  • Extends InstanceStatusUploadRequest DTO with a nested CI block containing resource counts, terraform outputs, warnings, errors, and base64-encoded output log
  • Gated on --upload-status flag + ci.enabled — backward compatible via omitempty pointer

Test plan

  • Review PRD content for completeness and accuracy
  • Validate API contract with Atmos Pro backend team

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation
    • Added a PRD describing a new "Pro Summary Upload" flow: optional, structured CI metadata attached to instance status uploads, upload gating, size/truncation behavior, and backward-compatible handling when metadata build fails.
  • New Features
    • Status uploads can include component-typed metadata with parsed Terraform summary fields (changes, errors, warnings, resource counts, masked outputs) and a base64-encoded, truncation-aware output log.

Add product requirements document for uploading structured CI summary
data (resource counts, terraform outputs, warnings, errors, output log)
to Atmos Pro via the existing instance status PATCH endpoint.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@goruha
Igor Rodionov (goruha) requested a review from a team as a code owner March 26, 2026 22:11
@goruha Igor Rodionov (goruha) added the no-release Do not create a new release (wait for additional code changes) label Mar 26, 2026
@github-actions github-actions Bot added the size/m Medium size PR label Mar 26, 2026
@github-actions

github-actions Bot commented Mar 26, 2026 •

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Files

None

@coderabbitai

coderabbitai Bot commented Mar 26, 2026 •

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a PRD and implements CLI/server-side support for uploading component-specific CI status metadata: new plugin interface StatusDataProvider, plugin registry helpers, Terraform plugin status parsing, capture/truncate/base64 of output_log, extended DTO fields (component_type, metadata), and tests covering the end-to-end upload flow.

Changes

Cohort / File(s) Summary
Documentation
docs/prd/pro-summary-upload.md
New PRD describing the Pro Summary Upload spec: StatusDataProvider contract, CI metadata schema, output_log masking/base64/truncation, and upload gating semantics.
CLI exec flow
internal/exec/pro.go, internal/exec/pro_test.go, internal/exec/terraform_execute_helpers_exec.go
uploadStatus signature extended to accept componentType and metadata; command execution now optionally captures masked stdout when upload-status and CI enabled; tests updated/added for metadata inclusion, output capture, encoding and truncation behavior.
Plugin framework
pkg/ci/internal/plugin/types.go, pkg/ci/plugin_registry.go, pkg/ci/plugin_registry_test.go
Added StatusDataProvider interface and registry helper BuildStatusData; tests and stub plugin added to validate provider lookup and behavior.
Terraform CI plugin
pkg/ci/plugins/terraform/plugin.go, pkg/ci/plugins/terraform/plugin_test.go
Terraform plugin now implements BuildStatusData to parse outputs into has_changes, has_errors, warnings, resource_counts, and outputs with sensitive values masked; tests added for various plan/apply/no-change cases and output masking.
Pro API client / DTOs
pkg/pro/api_client_instance_status.go, pkg/pro/api_client_instance_status_test.go, pkg/pro/dtos/instances.go
Upload payload extended to conditionally include top-level component_type and metadata when present; tests assert JSON shape includes/omits these fields as expected.

Sequence Diagram(s)

sequenceDiagram
    actor CLI
    participant Plugin as Component CI Plugin
    participant Registry as Plugin Registry
    participant Pro as Atmos Pro Server
    participant Git as GitRepo

    CLI->>Git: collect git info
    CLI->>Registry: GetPlugin(component_type)
    alt plugin found & implements StatusDataProvider
        CLI->>Plugin: BuildStatusData(output, command)
        Plugin-->>CLI: StatusData (map[string]any)
        CLI->>CLI: mask sensitive values, base64-encode output_log, truncate from start if needed
        CLI->>Pro: PATCH /api/v1/instances/{id}/status with component_type + metadata + command + exit_code
    else plugin missing or build fails
        CLI-->>CLI: emit warn-only
        CLI->>Pro: PATCH /api/v1/instances/{id}/status with command + exit_code (no metadata)
    end
    Pro->>Pro: validate/accept payload (metadata optional)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

minor

Suggested reviewers

  • milldr
  • osterman
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.89% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'docs: add PRD for pro summary upload' accurately summarizes the primary change—adding documentation (a PRD) for the Pro Summary Upload feature.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch prd/pro-summary-upload

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 59-63: Update the PRD to require explicit secret-redaction and
payload-size limits for the CI output fields: enforce that ci.outputs and the
OutputLog (json:"output_log", referenced as OutputLog) must have sensitive
values masked or dropped (e.g., remove tokens, credentials, secrets), include a
description of the redaction strategy (masking pattern or drop-on-detect) and
add a max payload size limit with behavior on overflow (truncate output, include
truncation metadata and byte counts). Also apply the same requirements to the
other CI output sections noted (the other ci.outputs/ci.output_log occurrences)
so all mentions include the redaction rule, truncation policy, and metadata
requirements.
- Around line 133-140: The PRD shows an unsafe direct type assertion on
result.Data (e.g., result.Data.(*TerraformOutputData).Outputs) which can panic;
update the steps that map ParseOutput()'s OutputResult into InstanceStatusCI to
use a safe assertion pattern: call terraform.ParseOutput(), then check the
assertion like data, ok := result.Data.(*TerraformOutputData) and only read
data.ResourceCounts, data.Outputs (converting TerraformOutput.Value to raw
values), and data.Warnings when ok is true, otherwise set Defaults/empty values
and propagate result.Errors into InstanceStatusCI.Errors; mention the use of the
ok boolean and fallback behavior when mapping HasChanges, HasErrors,
ResourceCounts, Outputs, Warnings, and Errors.
🪄 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: ec11f5ac-c3ff-484f-a00c-90d9c759185f

📥 Commits

Reviewing files that changed from the base of the PR and between 2be2909 and ecbc4cc.

📒 Files selected for processing (1)
  • docs/prd/pro-summary-upload.md

Comment thread docs/prd/pro-summary-upload.md Outdated
Comment thread docs/prd/pro-summary-upload.md Outdated
@codecov

codecov Bot commented Mar 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.53731% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.91%. Comparing base (2867836) to head (7536969).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
internal/exec/terraform_execute_helpers_exec.go 81.48% 4 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2259      +/-   ##
==========================================
+ Coverage   78.90%   78.91%   +0.01%     
==========================================
  Files        1202     1207       +5     
  Lines      116155   116505     +350     
==========================================
+ Hits        91648    91943     +295     
- Misses      19441    19480      +39     
- Partials     5066     5082      +16     
Flag Coverage Δ
unittests 78.91% <92.53%> (+0.01%) ⬆️

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

Files with missing lines Coverage Δ
internal/exec/pro.go 48.14% <100.00%> (+0.77%) ⬆️
pkg/ci/internal/plugin/types.go 100.00% <ø> (ø)
pkg/ci/plugin_registry.go 100.00% <100.00%> (ø)
pkg/ci/plugins/terraform/plugin.go 94.95% <100.00%> (+1.27%) ⬆️
pkg/pro/api_client_instance_status.go 71.42% <100.00%> (+3.68%) ⬆️
internal/exec/terraform_execute_helpers_exec.go 85.30% <81.48%> (-0.15%) ⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…abstraction

- Add business problem section explaining the user pain and value proposition
- Output log must pass through IO masking layer (Gitleaks patterns) before base64 encoding
- Introduce StatusDataProvider interface for component-type abstraction
- Terraform is first implementation; helmfile/packer plug into same contract
- Polymorphic ci.data field discriminated by component_type

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
docs/prd/pro-summary-upload.md (2)

206-207: ⚠️ Potential issue | 🟠 Major

Define redaction rules for ci.data.outputs (not just output_log).

The PRD secures output_log, but ci.data.outputs is still documented as raw values. Terraform outputs can carry secrets, so this needs an explicit mask/omit rule tied to Terraform sensitive metadata before upload.

Suggested PRD patch
- | `ci.data.outputs` | `TerraformOutputData.Outputs` | Terraform output values (raw) |
+ | `ci.data.outputs` | `TerraformOutputData.Outputs` | Terraform output values with sensitive values masked or omitted |
 - **Secret masking before upload**: The output log passes through `io.Mask()` (Gitleaks-based, 120+ patterns) before base64 encoding. This ensures secrets never leave the CLI unredacted, matching the security guarantees of all other Atmos output streams.
+ - **Sensitive Terraform outputs**: Values marked sensitive in Terraform output metadata MUST NOT be uploaded in cleartext. Replace with `"[REDACTED]"` or omit the key.

Also applies to: 301-302

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/prd/pro-summary-upload.md` around lines 206 - 207, Add explicit
redaction rules for ci.data.outputs similar to output_log: detect
TerraformOutputData.Outputs entries flagged as sensitive (Terraform "sensitive"
metadata) and either mask or omit those values before any upload; update the PRD
text to state that ci.data.outputs will be filtered according to Terraform
sensitive metadata and show the same masking/omission policy as output_log (also
apply the same change to the duplicate spots referenced around lines 301-302).

71-73: ⚠️ Potential issue | 🟡 Minor

Align “apply-only outputs” requirement with the implementation snippet.

The prose says outputs are apply-only, but the snippet sets Outputs unconditionally. Add an explicit command/result guard in the PRD snippet to avoid divergent implementations.

Suggested PRD patch
-        data.Data = &TerraformStatusData{
-            ResourceCounts: &tfData.ResourceCounts,
-            Outputs:        extractOutputValues(tfData.Outputs),
-        }
+        tfStatus := &TerraformStatusData{
+            ResourceCounts: &tfData.ResourceCounts,
+        }
+        if isApplyCommand(result) {
+            tfStatus.Outputs = extractOutputValues(tfData.Outputs)
+        }
+        data.Data = tfStatus

Also applies to: 86-89, 202-207

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/prd/pro-summary-upload.md` around lines 71 - 73, The PRD snippet
unconditionally includes the Outputs field (Outputs map[string]any) even though
the prose requires “apply-only outputs”; update the example so Outputs is only
populated when the operation/result indicates an apply (e.g., add an explicit
guard checking command == "apply" or result.IsApplySuccess before setting
Outputs). Locate the struct/variable with the Outputs symbol and change the
example flow to conditionally assign or include Outputs only in the
apply-success branch (and show an empty/omitted Outputs for non-apply commands).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 206-207: Add explicit redaction rules for ci.data.outputs similar
to output_log: detect TerraformOutputData.Outputs entries flagged as sensitive
(Terraform "sensitive" metadata) and either mask or omit those values before any
upload; update the PRD text to state that ci.data.outputs will be filtered
according to Terraform sensitive metadata and show the same masking/omission
policy as output_log (also apply the same change to the duplicate spots
referenced around lines 301-302).
- Around line 71-73: The PRD snippet unconditionally includes the Outputs field
(Outputs map[string]any) even though the prose requires “apply-only outputs”;
update the example so Outputs is only populated when the operation/result
indicates an apply (e.g., add an explicit guard checking command == "apply" or
result.IsApplySuccess before setting Outputs). Locate the struct/variable with
the Outputs symbol and change the example flow to conditionally assign or
include Outputs only in the apply-success branch (and show an empty/omitted
Outputs for non-apply commands).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 67057692-024d-48dd-b052-eb18788bf3f3

📥 Commits

Reviewing files that changed from the base of the PR and between ecbc4cc and 930082e.

📒 Files selected for processing (1)
  • docs/prd/pro-summary-upload.md

coderabbitai[bot]
coderabbitai Bot previously approved these changes Mar 27, 2026
- Fix io.Mask() to use WithStdoutCapture (already-masked capture)
- Clarify two capture paths: raw for parsing, masked for output_log
- Replace sensitive terraform outputs with <MASKED>
- Add server-defined output log size limits with truncation
- Note deploy→apply conversion for Atmos Pro
- Change CI field from typed struct to map[string]any for flexibility
- Add cross-reference to instance-status-raw-upload.md

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
docs/prd/pro-summary-upload.md (1)

177-203: Fix duplicate section numbering for readability.

There are two ### 7. sections (“Integration Point” and “Data to Upload”). Renumber the second one to keep the doc easy to navigate.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/prd/pro-summary-upload.md` around lines 177 - 203, The document has
duplicate heading numbers: both "Integration Point" and "Data to Upload" use
"### 7."; rename the second heading (the one titled "Data to Upload") to "###
8." (or the next sequential number) so headings read "### 7. Integration Point"
and "### 8. Data to Upload" to restore correct numbering and improve navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 216-217: Update the docs to remove the phrase "Full command
stdout" for `ci.output_log` and clarify that `ci.output_log` contains the
captured stdout which may be truncated (see `ci.truncated` and Section 6), e.g.,
describe it as "Captured command stdout (base64-encoded); may be partial if
truncated" and mention that truncation is controlled/masked by
`WithStdoutCapture`; ensure both `ci.output_log` and `ci.truncated` entries
consistently state the partial/truncation behavior.
- Around line 131-132: The doc incorrectly shows resolving the CI plugin with
ci.GetPlugin(info.Command); change this to use the component type string (e.g.,
info.ComponentType) when calling ci.GetPlugin so the plugin lookup succeeds for
components like "terraform" or "ansible"; then, if the resolved plugin
implements StatusDataProvider, call its BuildStatusData(result, output) to
obtain the map[string]any as described (reference symbols: ci.GetPlugin,
info.ComponentType, StatusDataProvider, BuildStatusData, result, output).

---

Nitpick comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 177-203: The document has duplicate heading numbers: both
"Integration Point" and "Data to Upload" use "### 7."; rename the second heading
(the one titled "Data to Upload") to "### 8." (or the next sequential number) so
headings read "### 7. Integration Point" and "### 8. Data to Upload" to restore
correct numbering and improve navigation.
🪄 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: 1bb9c395-580f-4a14-a711-9e01d69a0e87

📥 Commits

Reviewing files that changed from the base of the PR and between 930082e and 29d434d.

📒 Files selected for processing (1)
  • docs/prd/pro-summary-upload.md

Comment thread docs/prd/pro-summary-upload.md Outdated
Comment thread docs/prd/pro-summary-upload.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 153-203: The document has duplicate section numbers for the
headings "Integration Point" and "Data to Upload"; update the heading numbers so
they are sequential by renaming "7. Integration Point" to "8. Integration Point"
and renaming the subsequent "7. Data to Upload" to "9. Data to Upload" (ensure
any in-text references or the table of contents that mention these section
numbers are updated accordingly).
🪄 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: 5c13d652-ef19-4ce7-a8e7-5474a09835a0

📥 Commits

Reviewing files that changed from the base of the PR and between 29d434d and 5da200e.

📒 Files selected for processing (1)
  • docs/prd/pro-summary-upload.md

Comment thread docs/prd/pro-summary-upload.md Outdated
@github-actions

Copy link
Copy Markdown

These changes were released in v1.214.0-test.0.

* feat: upload structured CI data to Atmos Pro with instance status

Add StatusDataProvider interface to CI plugin system, allowing each
component type to contribute structured data to the Pro status upload.
Terraform plugin implements it with resource counts, outputs (sensitive
values masked), warnings, and errors. Output log is captured via
WithStdoutCapture (already masked), base64-encoded, and truncated
from the beginning if exceeding 3MB.

The CI data is included as a flexible map[string]any on the existing
InstanceStatusUploadRequest DTO, gated on ci.enabled. When disabled,
payloads are identical to before (backward compatible).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: improve coverage for CI data payload and output log truncation

- API client: verify CI block present/absent in serialized PATCH payload
- Exec: verify output_log contains masked content (base64 decoded)
- Exec: verify truncation at defaultMaxOutputLogBytes keeps tail, drops head
- Exec: verify truncation flag set, boundary and under-limit cases

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: rename ci to metadata, promote component_type to first-class field

- Rename DTO field from CI to Metadata (json:"metadata,omitempty")
- Add ComponentType as top-level DTO field (json:"component_type,omitempty")
- Remove component_type from plugin BuildStatusData return map
- Update API client to include component_type and metadata in payload
- Update uploadStatus signature to accept componentType and metadata
- Add tests for captureOutput path with realistic terraform output:
  plan with resource counts, apply output, error output, masked tokens,
  empty output, and large output truncation
- Update PRD to reflect all renames and new structure

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: cover buildMetadataForUpload and captureOutput gate

Extract buildMetadataForUpload() from executeMainTerraformCommand to
make the captureOutput→buildCIStatusData path unit-testable. Add 4 test
cases: captureOutput=false returns nil, captureOutput=true with
terraform produces metadata+output_log, unknown plugin returns nil,
empty output skips output_log key.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/l Large size PR and removed size/m Medium size PR labels Mar 31, 2026
@mergify

mergify Bot commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

💥 This pull request now has conflicts. Could you fix it Igor Rodionov (@goruha)? 🙏

@mergify mergify Bot added the conflict This PR has conflicts label Mar 31, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
internal/exec/pro.go (1)

217-218: Consider updating doc comment.

The comment says "terraform results" but the function now handles multiple component types. A small wording tweak would improve clarity.

📝 Suggested doc update
-// uploadStatus uploads the terraform results to the pro API.
+// uploadStatus uploads the component execution results to the pro API.
 func uploadStatus(info *schema.ConfigAndStacksInfo, exitCode int, componentType string, metadata map[string]any, client pro.AtmosProAPIClientInterface, gitRepo git.GitRepoInterface) error {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/exec/pro.go` around lines 217 - 218, The doc comment for
uploadStatus is outdated (mentions "terraform results" while the function
handles multiple component types); update the comment above the
uploadStatus(func uploadStatus(...)) to describe that it uploads operation
results for various component types (use parameter names like componentType,
info, exitCode, metadata) to the pro API so it's clear the function is not
Terraform-specific. Keep the comment short and neutral (e.g., "uploadStatus
uploads operation results for the given component type to the pro API").
internal/exec/pro_test.go (1)

467-482: Conditional assertions may mask test failures.

The if result != nil pattern means assertions are skipped if the terraform plugin isn't registered. Tests pass vacuously in that case.

Consider either:

  1. Adding a blank import _ "github.com/cloudposse/atmos/pkg/ci/plugins/terraform" to ensure registration.
  2. Using require.NotNil(t, result) to fail fast if the precondition isn't met.
Proposed fix to enforce plugin registration
 import (
 	"encoding/base64"
 	"testing"
 	"time"
 
 	gogit "github.com/go-git/go-git/v5"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 
 	atmosgit "github.com/cloudposse/atmos/pkg/git"
 	"github.com/cloudposse/atmos/pkg/pro/dtos"
 	"github.com/cloudposse/atmos/pkg/schema"
+
+	// Import terraform plugin to trigger init() registration.
+	_ "github.com/cloudposse/atmos/pkg/ci/plugins/terraform"
 )

Then update the test:

 	t.Run("returns CI data with output log for terraform", func(t *testing.T) {
-		// The terraform plugin is auto-registered via init().
-		// We need to import the package to trigger registration.
 		info := &schema.ConfigAndStacksInfo{
 			Command:    "terraform",
 			SubCommand: "plan",
 		}
 		output := []byte("Plan: 2 to add, 1 to change, 0 to destroy.")
 		result := buildCIStatusData(info, output)
 
-		// If terraform plugin is registered, we should get data.
-		if result != nil {
-			assert.Contains(t, result, "output_log")
-			assert.Contains(t, result, "has_changes")
-		}
+		require.NotNil(t, result, "terraform plugin should be registered")
+		assert.Contains(t, result, "output_log")
+		assert.Contains(t, result, "has_changes")
 	})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/exec/pro_test.go` around lines 467 - 482, The test uses a
conditional "if result != nil" which masks failures when the terraform plugin
isn't registered; update the test "returns CI data with output log for
terraform" to enforce the precondition by replacing the conditional with
require.NotNil(t, result) (from testify/require) immediately after calling
buildCIStatusData(info, output) and then run the existing assert.Contains
checks; alternatively ensure plugin registration by adding a blank import of the
terraform plugin package, but the simpler change is to call require.NotNil(t,
result) so the test fails fast if buildCIStatusData returned nil.
docs/prd/pro-summary-upload.md (1)

165-165: Missing language specifier on code fence.

Minor markdown issue—can be addressed in a follow-up cleanup per repository conventions.

Based on learnings: "For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/prd/pro-summary-upload.md` at line 165, The code block in
docs/prd/pro-summary-upload.md uses a triple-backtick fence (```) without a
language specifier; update the opening fence for that code block to include the
correct language (e.g., ```yaml, ```json, or ```bash) so markdownlint rules like
MD040/MD010/MD034 are satisfied — locate the code fence with the ``` marker in
pro-summary-upload.md and add the appropriate language token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@docs/prd/pro-summary-upload.md`:
- Line 165: The code block in docs/prd/pro-summary-upload.md uses a
triple-backtick fence (```) without a language specifier; update the opening
fence for that code block to include the correct language (e.g., ```yaml,
```json, or ```bash) so markdownlint rules like MD040/MD010/MD034 are satisfied
— locate the code fence with the ``` marker in pro-summary-upload.md and add the
appropriate language token.

In `@internal/exec/pro_test.go`:
- Around line 467-482: The test uses a conditional "if result != nil" which
masks failures when the terraform plugin isn't registered; update the test
"returns CI data with output log for terraform" to enforce the precondition by
replacing the conditional with require.NotNil(t, result) (from testify/require)
immediately after calling buildCIStatusData(info, output) and then run the
existing assert.Contains checks; alternatively ensure plugin registration by
adding a blank import of the terraform plugin package, but the simpler change is
to call require.NotNil(t, result) so the test fails fast if buildCIStatusData
returned nil.

In `@internal/exec/pro.go`:
- Around line 217-218: The doc comment for uploadStatus is outdated (mentions
"terraform results" while the function handles multiple component types); update
the comment above the uploadStatus(func uploadStatus(...)) to describe that it
uploads operation results for various component types (use parameter names like
componentType, info, exitCode, metadata) to the pro API so it's clear the
function is not Terraform-specific. Keep the comment short and neutral (e.g.,
"uploadStatus uploads operation results for the given component type to the pro
API").

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9045a5a1-23bd-464e-afc8-117bbc51525f

📥 Commits

Reviewing files that changed from the base of the PR and between 5da200e and 1d94398.

📒 Files selected for processing (12)
  • docs/prd/pro-summary-upload.md
  • internal/exec/pro.go
  • internal/exec/pro_test.go
  • internal/exec/terraform_execute_helpers_exec.go
  • pkg/ci/internal/plugin/types.go
  • pkg/ci/plugin_registry.go
  • pkg/ci/plugin_registry_test.go
  • pkg/ci/plugins/terraform/plugin.go
  • pkg/ci/plugins/terraform/plugin_test.go
  • pkg/pro/api_client_instance_status.go
  • pkg/pro/api_client_instance_status_test.go
  • pkg/pro/dtos/instances.go
✅ Files skipped from review due to trivial changes (1)
  • pkg/ci/internal/plugin/types.go

coderabbitai[bot]
coderabbitai Bot previously approved these changes Mar 31, 2026
autofix-ci Bot and others added 3 commits March 31, 2026 17:45
…into prd/pro-summary-upload

* 'prd/pro-summary-upload' of github.com:cloudposse/atmos:
  [autofix.ci] apply automated fixes
@github-actions

Copy link
Copy Markdown

These changes were released in v1.216.0-test.0.

Resolves conflicts by combining CI output capture from pro-summary-upload
with retry logic from main in executeMainCommand, and preserving both test
suites in plugin_test.go.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

These changes were released in v1.221.0-test.0.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

These changes were released in v1.221.0-test.1.

@goruha
Igor Rodionov (goruha) requested a review from a team as a code owner June 9, 2026 12:49
@github-actions github-actions Bot added size/xl Extra large size PR and removed size/l Large size PR labels Jun 9, 2026
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

These changes were released in v1.221.0-test.2.

@mergify

mergify Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

💥 This pull request now has conflicts. Could you fix it Igor Rodionov (@goruha)? 🙏

@mergify mergify Bot added the conflict This PR has conflicts label Jun 11, 2026
@mergify mergify Bot removed the conflict This PR has conflicts label Sep 6, 2026
@mergify

mergify Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

💥 This pull request now has conflicts. Could you fix it Igor Rodionov (@goruha)? 🙏

@mergify mergify Bot added the conflict This PR has conflicts label Sep 6, 2026
@atmos-pro

atmos-pro Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Note

Atmos Pro  

Waiting for your GitHub Actions workflow to upload affected stacks.
Learn More.

@mergify mergify Bot removed the conflict This PR has conflicts label Sep 10, 2026

This branch was previously deployed

1 inactive deployment
feature-releases — 8fd804b5 Deployed Jun 9, 2026 by goruha via release / publish / release #16980
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-release Do not create a new release (wait for additional code changes) release/feature Create release from this PR size/xl Extra large size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant