Open
Conversation
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
added
the
no-release
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughA new product requirements document is added describing how the CLI extends instance status upload requests to Atmos Pro. The doc specifies a DTO extension including optional terraform plan/apply metrics (change/error flags, resource counts, outputs, warnings, errors, and base64 stdout log) with conditional gating via Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
Comment on lines
+59
to
+63
| // OutputLog contains the raw terraform stdout captured during command execution, | ||
| // base64-encoded. This preserves the full output including ANSI formatting | ||
| // for server-side rendering and debugging. | ||
| OutputLog string `json:"output_log,omitempty"` | ||
| } |
Contributor
There was a problem hiding this comment.
Add explicit secret-redaction and payload-size limits for ci.outputs and ci.output_log.
Right now the PRD requires shipping raw outputs and full stdout, which can leak secrets and can blow request size on large plans/applies. Add requirements for masking sensitive values (or dropping them), stripping tokens/credentials from logs, and enforcing a max payload with truncation metadata.
Suggested PRD patch
## Error Handling
@@
- The upload itself remains fatal (matching existing behavior in `executeMainTerraformCommand`).
+## Security and Payload Constraints
+
+- Treat `ci.outputs` and `ci.output_log` as potentially sensitive.
+- Never upload values marked sensitive by terraform metadata; mask as `"[REDACTED]"` (or omit).
+- Apply log scrubbing before base64 encoding (credentials, tokens, secrets).
+- Enforce a maximum encoded size for `ci.output_log` (for example, 256KB or 1MB), truncating safely when exceeded.
+- Include truncation metadata (e.g., `output_log_truncated: true`) so the server/UI can signal partial logs.Also applies to: 181-193, 241-245, 255-256
🤖 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 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.
Comment on lines
+133
to
+140
| 2. Call `terraform.ParseOutput()` on the captured command output to get the `OutputResult`. | ||
| 3. Build an `InstanceStatusCI` struct and map the parsed result: | ||
| - `HasChanges` ← `result.HasChanges` | ||
| - `HasErrors` ← `result.HasErrors` | ||
| - `ResourceCounts` ← `result.Data.(*TerraformOutputData).ResourceCounts` | ||
| - `Outputs` ← `result.Data.(*TerraformOutputData).Outputs` (apply only, convert `TerraformOutput.Value` to raw values) | ||
| - `Warnings` ← `result.Data.(*TerraformOutputData).Warnings` | ||
| - `Errors` ← `result.Errors` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how ParseOutput and OutputResult.Data are defined and used.
rg -n -C3 --type go '\bfunc\s+ParseOutput\s*\('
rg -n -C3 --type go '\btype\s+OutputResult\s+struct\b|\bData\s+.*\b'
rg -n -C3 --type go '\btype\s+TerraformOutputData\s+struct\b'
rg -n -C3 --type go 'Data\.\(\*TerraformOutputData\)'Repository: cloudposse/atmos
Length of output: 26893
Update PRD to reflect safe type assertion pattern.
The PRD snippet shows a direct type assertion without checking success, which could panic. The actual codebase (e.g., pkg/ci/plugins/terraform/plugin.go:133) uses safe assertions with ok checks when accessing result.Data. Update the PRD to document this proper pattern:
Suggested PRD patch
3. Build an `InstanceStatusCI` struct and map the parsed result using a safe cast:
+ - `tfData, ok := result.Data.(*TerraformOutputData)`; if `!ok`, log warning and skip `ci` enrichment
- `HasChanges` ← `result.HasChanges`
- `HasErrors` ← `result.HasErrors`
- - `ResourceCounts` ← `result.Data.(*TerraformOutputData).ResourceCounts`
- - `Outputs` ← `result.Data.(*TerraformOutputData).Outputs` (apply only, convert `TerraformOutput.Value` to raw values)
- - `Warnings` ← `result.Data.(*TerraformOutputData).Warnings`
+ - `ResourceCounts` ← `tfData.ResourceCounts`
+ - `Outputs` ← `tfData.Outputs` (apply only, convert `TerraformOutput.Value` to raw values)
+ - `Warnings` ← `tfData.Warnings`🤖 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 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2259 +/- ##
==========================================
+ Coverage 77.21% 77.26% +0.04%
==========================================
Files 1018 1018
Lines 96368 96368
==========================================
+ Hits 74415 74457 +42
+ Misses 17755 17712 -43
- Partials 4198 4199 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
InstanceStatusUploadRequestDTO with a nestedCIblock containing resource counts, terraform outputs, warnings, errors, and base64-encoded output log--upload-statusflag +ci.enabled— backward compatible viaomitemptypointerTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit