feat(pro): automatic command-execution metadata upload to Atmos Pro - #2926
Open
Igor Rodionov (goruha) wants to merge 8 commits into
Open
feat(pro): automatic command-execution metadata upload to Atmos Pro#2926Igor Rodionov (goruha) wants to merge 8 commits into
Igor Rodionov (goruha) wants to merge 8 commits into
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Igor Rodionov (goruha)
temporarily deployed
to
screengrabs
August 11, 2026 20:22 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Igor Rodionov (goruha)
temporarily deployed
to
screengrabs
August 11, 2026 20:22 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Resource Changes Found for
|
Igor Rodionov (goruha)
temporarily deployed
to
screengrabs
August 11, 2026 20:30 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Member
Author
|
@rabbitcodeia full review |
![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: 20
🧹 Nitpick comments (1)
pkg/proexec/truncate_test.go (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnd the inline comment with a period.
Change the comment to
// Small enough to force trimming.As per coding guidelines, all comments must end with periods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/proexec/truncate_test.go` at line 41, Update the inline comment on atmosConfig.Settings.Pro.MaxPayloadBytes to end with a period and use the wording “Small enough to force trimming.”Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/root.go`:
- Around line 1909-1912: Update the command-execution metadata capture around
proexec.CaptureAsync so synchronous commands use exactly one upload path. Make
the synchronous capture exclusive of the asynchronous call for terraform plan,
terraform apply, and describe affected, or provide both paths with the same
stable execution identifier and server-side deduplication.
In `@internal/exec/pro_test.go`:
- Around line 52-55: Replace the hand-maintained MockProAPIClient, including
UploadExecMetadata, with a complete mockgen-generated mock based on
AtmosProAPIClientInterface using go.uber.org/mock/mockgen. Update the associated
tests to construct and use the generated mock and its expected-call API.
In `@internal/exec/terraform.go`:
- Line 192: Update the root command’s CaptureAsync invocation to exclude
commands already uploaded by CaptureSync: terraform plan, terraform apply, and
describe affected. Apply this exclusion consistently at
internal/exec/terraform.go lines 192-192 and internal/exec/describe_affected.go
lines 372-372, using the existing command-exclusion mechanism rather than
changing CaptureSync.
In `@pkg/metrics/process/metrics_test.go`:
- Around line 10-36: Extend the metrics test suite with build-tagged,
table-driven platform-specific tests covering Unix counter deltas, RSS
normalization, and Windows FILETIME conversion. Place each test with the
relevant platform implementation and exercise multiple input scenarios, while
preserving the existing Baseline, Snapshot.Since, and elapsed-time tests.
In `@pkg/pro/api_client_exec.go`:
- Around line 13-17: Add the required deferred perf.Track call at the beginning
of the public AtmosProAPIClient.UploadExecMetadata method, preserving its
existing upload behavior and error handling.
- Around line 27-40: Update UploadExecMetadata to generate one stable
per-execution idempotency key before doWithRetry and attach the same key/header
to every POST retry. Extend the Pact contract and provider handling to accept
that key and deduplicate repeated metadata submissions, while preserving normal
behavior for distinct executions.
In `@pkg/pro/consumer_pact_test.go`:
- Around line 370-471: Remove TestPact_UploadExecMetadata and regenerate
pacts/atmos-AtmosPro.json without its structured Terraform interaction. Update
specs/002-pro-exec-metadata/contracts/interactions.md (43-58) to defer
structured Terraform data; update data-model.md (54-60, 74-75) to state
Terraform Data is nil and plan/apply structured data is nil; remove the
unsupported TerraformOutputData and data-passing claims from plan.md (17-20,
128).
In `@pkg/proexec/async_test.go`:
- Around line 17-47: The test currently uses a hand-written fake instead of a
generated mock for AtmosProAPIClientInterface. Add a mockgen directive targeting
AtmosProAPIClientInterface, generate its mock, and replace fakeUploadClient in
the async upload tests with the generated mock while preserving the existing
call-count, delay, request-capture, and error behaviors through mock
expectations or callbacks.
In `@pkg/proexec/async.go`:
- Line 52: Update the public CaptureAsync function to defer perf.Track
immediately after resolving atmosConfig, using the metric name
"pkg.CaptureAsync", followed by a blank line before the remaining logic.
- Around line 64-67: Update the exitCode assignment in the error-handling flow
of the async command execution function to use errUtils.GetExitCode(err) instead
of always setting failures to 1. Preserve the existing zero value when err is
nil, while retaining typed command exit codes and the default fallback for other
errors.
In `@pkg/proexec/envelope_test.go`:
- Around line 135-145: Update the masking assertion in the buildRecord test to
verify decoded["aws_key"] equals "<MASKED>" and that the original AWS key is
absent from req.Data. Preserve the existing JSON decoding and require the
redacted value after buildRecord executes.
In `@pkg/proexec/envelope.go`:
- Around line 69-74: Update buildRecord when assigning GitSHA, RepoURL, and
related repository fields so the upload request uses a sanitized RepoURL with
credentials removed. Preserve repoInfo.RepoUrl unchanged for authenticated
cloning and sanitize only the value copied into the upload record.
In `@pkg/proexec/sync_test.go`:
- Around line 47-61: Extend TestCaptureSync_WarnAndContinueOnFailure to use a
controlled slow HTTP endpoint that delays its response beyond the configured
sync timeout, ensuring CaptureSync executes its timeout branch rather than an
immediate connection failure. Configure the test server URL and a short timeout,
then assert CaptureSync returns without error and completes within an
appropriate bounded duration.
In `@pkg/proexec/sync.go`:
- Around line 24-45: Update CaptureSync to accept the invocation arguments, then
pass them through its goroutine to buildRecord instead of allowing buildRecord
to use nil. Preserve the existing synchronous upload flow while ensuring command
arguments are retained in the generated execution record.
- Around line 24-45: Update CaptureAsync to skip commands accepted by the
existing synchronous allowlist predicate, reusing that shared predicate rather
than duplicating command names. Ensure allowlisted commands such as terraform
plan, terraform apply, and describe affected are uploaded only through
CaptureSync, while non-allowlisted commands retain the existing asynchronous
upload behavior.
- Line 24: Add deferred performance tracking as the first statement in the
public CaptureSync function, using atmosConfig and the identifier
"pkg.CaptureSync", followed by a blank line before the existing logic.
In `@pkg/proexec/truncate.go`:
- Around line 24-60: The truncateIfNeeded flow must enforce MaxPayloadBytes for
every final request, including nil or empty Data and requests replaced with the
truncation marker; recheck marshaledSize after marker replacement and return a
static wrapped execution-metadata error when the envelope still exceeds the
limit. Update the table cases in pkg/proexec/truncate_test.go lines 61-80 to
cover an oversized marker and an oversized envelope with nil Data, asserting
successful results never exceed the configured limit.
In `@specs/002-pro-exec-metadata/contracts/interactions.md`:
- Around line 1-10: Correct the Pact documentation to reflect ten total
interactions and both execution-metadata interactions: update
specs/002-pro-exec-metadata/contracts/interactions.md lines 1-10, and revise the
“9th interaction,” scope count, contract-tree comment, Pact test comment, and
regenerated-Pact comment in specs/002-pro-exec-metadata/plan.md at lines 19-21,
64-66, 95-96, 120-121, and 131-131.
In `@specs/002-pro-exec-metadata/spec.md`:
- Around line 89-90: Defer or remove FR-006 from the current feature scope in
specs/002-pro-exec-metadata/spec.md, since Terraform plan/apply structured
resource data is assigned to issue `#2924`. In
specs/002-pro-exec-metadata/tasks.md, update the User Story 3 status so it is
not described as functional until T026 and T028 are complete; no direct change
is required to FR-005.
- Around line 39-50: The critical-command delivery guarantee must describe
warn-and-continue behavior rather than strict success gating. Update
specs/002-pro-exec-metadata/spec.md lines 39-50 to state that upload failures or
timeouts emit a warning without changing the command result and revise the
acceptance criteria accordingly; update
website/blog/2026-08-11-pro-exec-metadata-upload.mdx lines 31-35 to remove the
claim that pipelines cannot report success when recording fails; update
website/src/data/roadmap.js line 487 to describe warning-only behavior instead
of guaranteed delivery.
---
Nitpick comments:
In `@pkg/proexec/truncate_test.go`:
- Line 41: Update the inline comment on atmosConfig.Settings.Pro.MaxPayloadBytes
to end with a period and use the wording “Small enough to force trimming.”
🪄 Autofix
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 Plus
Run ID: b50276b1-ea6a-447d-a2c7-310adffee69e
📒 Files selected for processing (44)
.specify/feature.jsonCLAUDE.mdcmd/root.goerrors/errors.gointernal/exec/describe_affected.gointernal/exec/pro_test.gointernal/exec/terraform.gointernal/exec/terraform_exec_metadata_test.gopacts/atmos-AtmosPro.jsonpkg/config/const.gopkg/config/load.gopkg/metrics/process/doc.gopkg/metrics/process/metrics.gopkg/metrics/process/metrics_test.gopkg/metrics/process/metrics_unix.gopkg/metrics/process/metrics_windows.gopkg/pro/api_client.gopkg/pro/api_client_exec.gopkg/pro/api_client_exec_test.gopkg/pro/consumer_pact_test.gopkg/pro/dtos/exec.gopkg/proexec/async.gopkg/proexec/async_test.gopkg/proexec/doc.gopkg/proexec/envelope.gopkg/proexec/envelope_test.gopkg/proexec/gate.gopkg/proexec/gate_test.gopkg/proexec/sync.gopkg/proexec/sync_test.gopkg/proexec/truncate.gopkg/proexec/truncate_test.gopkg/schema/pro.gospecs/002-pro-exec-metadata/checklists/requirements.mdspecs/002-pro-exec-metadata/contracts/interactions.mdspecs/002-pro-exec-metadata/data-model.mdspecs/002-pro-exec-metadata/plan.mdspecs/002-pro-exec-metadata/quickstart.mdspecs/002-pro-exec-metadata/research.mdspecs/002-pro-exec-metadata/spec.mdspecs/002-pro-exec-metadata/tasks.mdwebsite/blog/2026-08-11-pro-exec-metadata-upload.mdxwebsite/docs/cli/configuration/settings/pro.mdxwebsite/src/data/roadmap.js
Comment on lines
+1909
to
+1912
| // Best-effort, asynchronous Atmos Pro command-execution metadata upload | ||
| // (no-ops unless CI is detected AND Atmos Pro is configured). Placed | ||
| // immediately after the telemetry hook it mirrors — see pkg/proexec. | ||
| proexec.CaptureAsync(cmd, err) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/proexec --items all
rg -n -C 8 '\bCapture(Async|Sync)\s*\(' --glob '*.go' cmd internal pkg
rg -n -C 5 '\buploadExecMetadata\b|\bUploadExecMetadata\b' --glob '*.go' pkg internalRepository: cloudposse/atmos
Length of output: 27985
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- pkg/proexec/async.go ---'
cat -n pkg/proexec/async.go
printf '%s\n' '--- pkg/proexec/sync.go ---'
cat -n pkg/proexec/sync.go
printf '%s\n' '--- pkg/proexec/envelope.go ---'
cat -n pkg/proexec/envelope.go
printf '%s\n' '--- internal/exec/terraform.go ---'
sed -n '180,245p' internal/exec/terraform.go
printf '%s\n' '--- internal/exec/describe_affected.go ---'
sed -n '330,385p' internal/exec/describe_affected.go
printf '%s\n' '--- pkg/proexec documentation and identifiers ---'
cat -n pkg/proexec/doc.go 2>/dev/null || true
rg -n -C 4 'AtmosProRunID|atmosProRunID|RunID|CaptureAsync|CaptureSync' pkg/proexec internal/exec cmd/root.go --glob '*.go'Repository: cloudposse/atmos
Length of output: 36775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("cmd/root.go").read_text()
terraform = Path("internal/exec/terraform.go").read_text()
describe = Path("internal/exec/describe_affected.go").read_text()
sync = Path("pkg/proexec/sync.go").read_text()
async_ = Path("pkg/proexec/async.go").read_text()
envelope = Path("pkg/proexec/envelope.go").read_text()
api = Path("pkg/pro/api_client_exec.go").read_text()
assert "proexec.CaptureAsync(cmd, err)" in root
assert 'proexec.CaptureSync(atmosConfig, "terraform "+info.SubCommand' in terraform
assert 'proexec.CaptureSync(a.CLIConfig, "describe affected"' in describe
assert "client.UploadExecMetadata(req)" in sync
assert "client.UploadExecMetadata(req)" in async_
assert re.search(r"AtmosProRunID:\s*atmosProRunID", envelope)
assert "Idempot" not in api and "idempot" not in api
assert "Idempot" not in sync and "idempot" not in async_
print("root async capture: present")
print("terraform synchronous capture: present")
print("describe affected synchronous capture: present")
print("sync upload calls: 1")
print("async upload calls: 1")
print("request correlation field: ATMOS_PRO_RUN_ID")
print("per-execution idempotency/deduplication logic in client paths: absent")
PY
printf '%s\n' '--- ExecUploadRequest definition ---'
rg -n -B 5 -A 35 'type ExecUploadRequest struct' --glob '*.go' .
printf '%s\n' '--- upload request construction and idempotency headers ---'
rg -n -C 4 'ExecUploadRequest|Idempot|idempot|X-.*Request|UploadExecMetadata' pkg --glob '*.go'Repository: cloudposse/atmos
Length of output: 50373
Prevent duplicate uploads for synchronous commands.
Both capture paths upload one record for terraform plan, terraform apply, and describe affected. ATMOS_PRO_RUN_ID identifies the CI run, not the command execution. No deduplication or idempotency key exists.
Make the synchronous path exclusive, or add a stable execution identifier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/root.go` around lines 1909 - 1912, Update the command-execution metadata
capture around proexec.CaptureAsync so synchronous commands use exactly one
upload path. Make the synchronous capture exclusive of the asynchronous call for
terraform plan, terraform apply, and describe affected, or provide both paths
with the same stable execution identifier and server-side deduplication.
Comment on lines
+52
to
+55
| func (m *MockProAPIClient) UploadExecMetadata(dto *dtos.ExecUploadRequest) error { | ||
| args := m.Called(dto) | ||
| return args.Error(0) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Generate MockProAPIClient with mockgen.
Do not hand-maintain UploadExecMetadata in this mock. Regenerate the complete mock from AtmosProAPIClientInterface and update its tests to use the generated mock.
As per coding guidelines, mocks must be generated with go.uber.org/mock/mockgen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/pro_test.go` around lines 52 - 55, Replace the hand-maintained
MockProAPIClient, including UploadExecMetadata, with a complete
mockgen-generated mock based on AtmosProAPIClientInterface using
go.uber.org/mock/mockgen. Update the associated tests to construct and use the
generated mock and its expected-call API.
Source: Coding guidelines
| invalidateTerraformStateCache(info.Stack, info.ComponentFromArg) | ||
| } | ||
|
|
||
| captureExecMetadataSync(&atmosConfig, &info, err) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline cmd/root.go --items all
rg -n -C 10 'proexec\.Capture(Async|Sync)|telemetry\.CaptureCmd' \
cmd/root.go internal/exec/terraform.go internal/exec/describe_affected.goRepository: cloudposse/atmos
Length of output: 17391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -i 'proexec' .
rg -n -C 12 'func (CaptureAsync|CaptureSync)|CaptureAsync|CaptureSync|Synchronous|allowlist|dedup|duplicate' \
pkg internal cmd --glob '*.go'Repository: cloudposse/atmos
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f . pkg internal cmd | rg '(^|/)(proexec|.*exec.*)\.go$'
rg -l 'func Capture(Async|Sync)|CaptureAsync|CaptureSync' pkg/proexec pkg/pro 2>/dev/nullRepository: cloudposse/atmos
Length of output: 18939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l pkg/proexec/async.go pkg/proexec/sync.go pkg/proexec/gate.go pkg/proexec/async_test.go pkg/proexec/sync_test.go
cat -n pkg/proexec/async.go
cat -n pkg/proexec/sync.go
cat -n pkg/proexec/gate.go
rg -n -C 8 'CaptureAsync|CaptureSync|terraform plan|terraform apply|describe affected|skip|exclude' \
pkg/proexec/*_test.goRepository: cloudposse/atmos
Length of output: 20483
Prevent duplicate Atmos Pro records.
When the CI and Atmos Pro gates are open, CaptureSync uploads records for terraform plan, terraform apply, and describe affected. The root command then calls CaptureAsync without excluding these commands, so each execution uploads twice. Add exclusions to CaptureAsync.
📍 Affects 2 files
internal/exec/terraform.go#L192-L192(this comment)internal/exec/describe_affected.go#L372-L372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/terraform.go` at line 192, Update the root command’s
CaptureAsync invocation to exclude commands already uploaded by CaptureSync:
terraform plan, terraform apply, and describe affected. Apply this exclusion
consistently at internal/exec/terraform.go lines 192-192 and
internal/exec/describe_affected.go lines 372-372, using the existing
command-exclusion mechanism rather than changing CaptureSync.
Comment on lines
+10
to
+36
| func TestBaseline_SinceReportsElapsedWallTime(t *testing.T) { | ||
| snap := Baseline() | ||
| time.Sleep(20 * time.Millisecond) | ||
|
|
||
| m := snap.Since() | ||
|
|
||
| assert.GreaterOrEqual(t, m.WallTime, 15*time.Millisecond) | ||
| } | ||
|
|
||
| func TestSnapshot_SinceIsNonNegative(t *testing.T) { | ||
| snap := Baseline() | ||
| m := snap.Since() | ||
|
|
||
| // CPU time deltas must never be negative even on a near-instant diff. | ||
| assert.GreaterOrEqual(t, m.UserCPUTime, time.Duration(0)) | ||
| assert.GreaterOrEqual(t, m.SystemCPUTime, time.Duration(0)) | ||
| assert.GreaterOrEqual(t, m.WallTime, time.Duration(0)) | ||
| } | ||
|
|
||
| func TestSnapshot_MultipleCallsIndependent(t *testing.T) { | ||
| snap := Baseline() | ||
| first := snap.Since() | ||
| time.Sleep(5 * time.Millisecond) | ||
| second := snap.Since() | ||
|
|
||
| assert.Greater(t, second.WallTime, first.WallTime) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add deterministic platform-specific metric tests.
The current tests do not verify Unix counter deltas, RSS normalization, or Windows FILETIME conversion. Add build-tagged, table-driven tests for these helper and delta paths. This protects the uploaded resource-metric contract on each supported platform.
As per coding guidelines, “Every new feature must include comprehensive unit tests” and tests must use table-driven cases for multiple scenarios.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/metrics/process/metrics_test.go` around lines 10 - 36, Extend the metrics
test suite with build-tagged, table-driven platform-specific tests covering Unix
counter deltas, RSS normalization, and Windows FILETIME conversion. Place each
test with the relevant platform implementation and exercise multiple input
scenarios, while preserving the existing Baseline, Snapshot.Since, and
elapsed-time tests.
Source: Coding guidelines
Comment on lines
+13
to
+17
| // UploadExecMetadata uploads a single command-execution record to Atmos Pro. | ||
| // Each execution record is one indivisible logical unit (unlike affected-stacks | ||
| // uploads), so it is never chunked — oversized payloads are truncated client-side | ||
| // before this method is called (see pkg/proexec/truncate.go). | ||
| func (c *AtmosProAPIClient) UploadExecMetadata(dto *dtos.ExecUploadRequest) error { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add performance tracking to this public method.
UploadExecMetadata is not a trivial accessor or delegator. Add the required defer perf.Track(...)() call at the start of the method.
As per coding guidelines, public non-trivial functions must call perf.Track.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/pro/api_client_exec.go` around lines 13 - 17, Add the required deferred
perf.Track call at the beginning of the public
AtmosProAPIClient.UploadExecMetadata method, preserving its existing upload
behavior and error handling.
Source: Coding guidelines
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds CI-gated Atmos Pro execution metadata uploads. The change adds process metrics, masked and chunked request handling, asynchronous and synchronous capture paths, Terraform and ChangesExecution metadata reporting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AtmosCommand
participant proexec
participant AtmosProAPIClient
participant AtmosPro
AtmosCommand->>proexec: Capture command metadata
proexec->>proexec: Check CI and Pro configuration
proexec->>proexec: Build masked execution record
proexec->>AtmosProAPIClient: Upload record or correlated chunks
AtmosProAPIClient->>AtmosPro: POST /api/v1/atmos/exec
AtmosPro-->>AtmosProAPIClient: Return upload response
sequenceDiagram
participant TerraformOrDescribeAffected
participant proexec
participant AtmosProAPIClient
TerraformOrDescribeAffected->>proexec: CaptureSync with exit code
proexec->>AtmosProAPIClient: UploadExecMetadata
proexec-->>TerraformOrDescribeAffected: Return on completion or timeout
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/exec/describe_affected.go`:
- Around line 362-363: Move the existing performance tracker from executeInner
to the public Execute method, placing defer perf.Track(atmosConfig,
"pkg.FuncName")() first and followed by a blank line. Remove the inner tracker
so timing includes the CaptureSync wait performed by Execute without double
tracking.
In `@internal/exec/terraform.go`:
- Around line 218-223: Update the exitCode assignment in the Terraform execution
flow before proexec.CaptureSync to use resolveExitCode(cmdErr) directly,
preserving errUtils.ExitCodeError.Code such as plan status 2 while retaining 1
for generic errors.
In `@pkg/proexec/async_test.go`:
- Line 76: Update the inline comment on the atmosConfig.Settings.Pro.BaseURL
assignment to read “Unreachable.” with an initial capital letter and a
terminating period.
- Around line 72-107: The current tests only verify return behavior and timing,
not successful upload dispatch or timeout handling. Extend the CaptureAsync
tests with a local test server that observes and validates an upload, and add a
delayed-response server case confirming CaptureAsync returns at
asyncFlushCeiling while the request remains in flight. Exercise behavior through
the existing CaptureAsync API and preserve the current CI/configuration
isolation setup.
In `@pkg/proexec/sync_test.go`:
- Around line 35-61: Add a successful delivery test alongside
TestCaptureSync_NoOpOnGateClosed and TestCaptureSync_WarnAndContinueOnFailure
using a controlled local HTTP server. Configure CaptureSync with the server URL
and a deliberately longer sync timeout, assert the server receives the expected
request method and payload, and verify CaptureSync returns no error before that
timeout.
In `@pkg/proexec/sync.go`:
- Around line 29-54: Update the sync flow around client creation, syncTimeout,
and client.UploadExecMetadata to create a cancellable context with the
configured timeout before calling pro.NewAtmosProAPIClientFromEnv, then pass
that context through the Atmos Pro client’s OIDC setup, token exchange, and
upload APIs. Ensure the context is canceled on timeout and preserve the existing
warn-and-continue behavior.
In `@specs/002-pro-exec-metadata/contracts/interactions.md`:
- Line 15: Update the UploadExecMetadata heading in the interactions document
from level 3 to level 2, preserving the heading text and surrounding content.
- Line 1: Update the contract title for the 9th Pact interaction to use the
exact endpoint path /api/v1/atmos/exec, matching the runtime client and contract
table.
In `@specs/002-pro-exec-metadata/data-model.md`:
- Around line 54-60: Defer Terraform resource data consistently across the
feature artifacts: mark TerraformExecData and structured plan/apply claims as
deferred in specs/002-pro-exec-metadata/data-model.md (54-60, 72-75); remove the
launch-time TerraformOutputData promise and revise Terraform call-site
integration in plan.md (9-19, 128-129); mark the structured-data parameter as
future behavior in research.md (146-153); defer User Story 3, FR-006, and SC-007
in spec.md (55-68, 89-90, 118); keep the User Story 3 checkpoint incomplete and
remove the populated-data Pact requirement in tasks.md (208-245, 254-258).
In `@specs/002-pro-exec-metadata/quickstart.md`:
- Around line 15-29: Fix the indentation in the quickstart steps around the
nested Bash code fences and continuation text: replace the current three-space
indentation with four spaces for the entire nested blocks, including their
opening and closing fences and explanatory lines, so it conforms to
editorconfig’s two-space indentation multiples.
- Around line 13-24: Add Windows-compatible environment setup to the
quickstart’s environment-variable instructions by providing equivalent
PowerShell and cmd.exe commands, or replace the Bash-only setup with a
platform-neutral method. Cover ATMOS_PRO_TOKEN, ATMOS_PRO_BASE_URL, and CI while
preserving the existing local testing values.
- Around line 20-24: Update the quickstart’s CI detection instructions to clear
every environment variable consumed by telemetry.IsCI() before running the
negative check, including provider-specific signals such as GITHUB_ACTIONS,
JENKINS_URL, and BUILD_ID; retain CI=true for the positive check.
In `@specs/002-pro-exec-metadata/research.md`:
- Around line 35-49: Update specs/002-pro-exec-metadata/research.md:35-49,
specs/002-pro-exec-metadata/plan.md:9-15, and
specs/002-pro-exec-metadata/tasks.md:127-132 to require CaptureAsync to skip the
synchronous allowlist commands, including terraform plan, terraform apply, and
describe affected, so each command uses exactly one upload path; update
specs/002-pro-exec-metadata/tasks.md:178-183 and the related tests to verify
async capture is excluded for those commands while synchronous capture remains
active. Preserve CaptureAsync for all other commands and retain the existing
CaptureSync call sites.
In `@specs/002-pro-exec-metadata/spec.md`:
- Line 97: Update FR-011 in the specification to require truncation of oversized
execution-record payloads, matching the data model, research decision, and T009;
remove the alternative permitting chunking and preserve the existing size-limit
requirement.
- Line 51: Update the acceptance scenario for non-critical commands in FR-009 to
describe bounded best-effort flushing rather than immediate exit: state that the
command does not wait for upload confirmation and waits no longer than the fixed
flush ceiling, while preserving the CI and Atmos Pro context.
- Around line 112-116: Update SC-001 so it does not require 100% guaranteed
delivery for asynchronous, best-effort uploads; measure qualifying upload
attempts instead, or explicitly condition the success target on confirmed
delivery. Keep the requirement aligned with FR-009 and SC-004 while preserving
the existing CI and Atmos Pro eligibility conditions.
In `@specs/002-pro-exec-metadata/tasks.md`:
- Around line 12-14: Raise the documented coverage requirement from 80% to 85%
in specs/002-pro-exec-metadata/tasks.md lines 12-14,
specs/002-pro-exec-metadata/plan.md lines 72-78, and
specs/002-pro-exec-metadata/tasks.md lines 262-265; update each feature
prerequisite, constitution check, and final coverage gate to require at least
85% repository coverage and new tests targeting over 85%.
In `@website/blog/2026-08-11-pro-exec-metadata-upload.mdx`:
- Around line 31-35: The critical-command documentation must not imply that
successful command completion guarantees delivery. In
website/blog/2026-08-11-pro-exec-metadata-upload.mdx lines 31-35, state that
critical commands wait up to the configured timeout and warn when delivery
fails; in website/src/data/roadmap.js line 493, remove the claim that they never
report success after a missed record.
🪄 Autofix
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 Plus
Run ID: aa746ae0-9fc9-4675-8b97-13e9ca19ef2f
📒 Files selected for processing (44)
.specify/feature.jsonCLAUDE.mdcmd/root.goerrors/errors.gointernal/exec/describe_affected.gointernal/exec/pro_test.gointernal/exec/terraform.gointernal/exec/terraform_exec_metadata_test.gopacts/atmos-AtmosPro.jsonpkg/config/const.gopkg/config/load.gopkg/metrics/process/doc.gopkg/metrics/process/metrics.gopkg/metrics/process/metrics_test.gopkg/metrics/process/metrics_unix.gopkg/metrics/process/metrics_windows.gopkg/pro/api_client.gopkg/pro/api_client_exec.gopkg/pro/api_client_exec_test.gopkg/pro/consumer_pact_test.gopkg/pro/dtos/exec.gopkg/proexec/async.gopkg/proexec/async_test.gopkg/proexec/doc.gopkg/proexec/envelope.gopkg/proexec/envelope_test.gopkg/proexec/gate.gopkg/proexec/gate_test.gopkg/proexec/sync.gopkg/proexec/sync_test.gopkg/proexec/truncate.gopkg/proexec/truncate_test.gopkg/schema/pro.gospecs/002-pro-exec-metadata/checklists/requirements.mdspecs/002-pro-exec-metadata/contracts/interactions.mdspecs/002-pro-exec-metadata/data-model.mdspecs/002-pro-exec-metadata/plan.mdspecs/002-pro-exec-metadata/quickstart.mdspecs/002-pro-exec-metadata/research.mdspecs/002-pro-exec-metadata/spec.mdspecs/002-pro-exec-metadata/tasks.mdwebsite/blog/2026-08-11-pro-exec-metadata-upload.mdxwebsite/docs/cli/configuration/settings/pro.mdxwebsite/src/data/roadmap.js
Comment on lines
362
to
+363
| func (d *describeAffectedExec) Execute(a *DescribeAffectedCmdArgs) error { | ||
| err := d.executeInner(a) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move performance tracking to Execute.
Execute is the public entry point and now waits for CaptureSync. The tracker in executeInner ends before that wait, so it omits synchronous upload time.
Move the existing tracker to Execute. Remove it from executeInner.
Proposed change
func (d *describeAffectedExec) Execute(a *DescribeAffectedCmdArgs) error {
+ defer perf.Track(a.CLIConfig, "exec.Execute")()
+
err := d.executeInner(a)
@@
func (d *describeAffectedExec) executeInner(a *DescribeAffectedCmdArgs) error {
- defer perf.Track(nil, "exec.Execute")()
-As per coding guidelines: “Add defer perf.Track(atmosConfig, "pkg.FuncName")() followed by a blank line to public functions.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (d *describeAffectedExec) Execute(a *DescribeAffectedCmdArgs) error { | |
| err := d.executeInner(a) | |
| func (d *describeAffectedExec) Execute(a *DescribeAffectedCmdArgs) error { | |
| defer perf.Track(a.CLIConfig, "exec.Execute")() | |
| err := d.executeInner(a) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/describe_affected.go` around lines 362 - 363, Move the existing
performance tracker from executeInner to the public Execute method, placing
defer perf.Track(atmosConfig, "pkg.FuncName")() first and followed by a blank
line. Remove the inner tracker so timing includes the CaptureSync wait performed
by Execute without double tracking.
Source: Coding guidelines
Comment on lines
+72
to
+107
| func TestCaptureAsync_DoesNotAlterCallerError(t *testing.T) { | ||
| withCIEnv(t, true) | ||
| atmosConfig := &schema.AtmosConfiguration{} | ||
| atmosConfig.Settings.Pro.Token = "test-token" | ||
| atmosConfig.Settings.Pro.BaseURL = "http://127.0.0.1:0" // unreachable | ||
| SetAtmosConfig(atmosConfig) | ||
| t.Cleanup(func() { SetAtmosConfig(nil) }) | ||
|
|
||
| cmd := &cobra.Command{Use: "version"} | ||
| callerErr := assertError("caller failed") | ||
|
|
||
| // CaptureAsync must not panic, must not return anything, and (by | ||
| // construction, it has no return value) cannot mutate callerErr. | ||
| CaptureAsync(cmd, callerErr) | ||
| assert.EqualError(t, callerErr, "caller failed") | ||
| } | ||
|
|
||
| // Ensures the process telemetry CI helpers are exercised for completeness; | ||
| // mirrors the isolation approach used by gate_test.go's withCIEnv. | ||
| func TestCaptureAsync_RespectsFlushCeiling(t *testing.T) { | ||
| withCIEnv(t, true) | ||
| _ = telemetry.IsCI // sanity: package imported and usable directly if needed. | ||
|
|
||
| atmosConfig := &schema.AtmosConfiguration{} | ||
| atmosConfig.Settings.Pro.Token = "test-token" | ||
| SetAtmosConfig(atmosConfig) | ||
| t.Cleanup(func() { SetAtmosConfig(nil) }) | ||
|
|
||
| cmd := &cobra.Command{Use: "version"} | ||
|
|
||
| start := time.Now() | ||
| CaptureAsync(cmd, nil) | ||
| elapsed := time.Since(start) | ||
| // Even in the worst case (client construction fails fast, or a slow | ||
| // upload), CaptureAsync must return within its documented ceiling. | ||
| assert.LessOrEqual(t, elapsed, asyncFlushCeiling+time.Second) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test successful asynchronous dispatch and the active timeout path.
These tests do not observe a request sent by CaptureAsync. Add a test that receives the upload through a local test server. Add a delayed-response case that confirms CaptureAsync returns at the flush ceiling while the upload remains in flight.
As per coding guidelines, “Every new feature must include comprehensive unit tests” and tests must “test behavior rather than implementation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/proexec/async_test.go` around lines 72 - 107, The current tests only
verify return behavior and timing, not successful upload dispatch or timeout
handling. Extend the CaptureAsync tests with a local test server that observes
and validates an upload, and add a delayed-response server case confirming
CaptureAsync returns at asyncFlushCeiling while the request remains in flight.
Exercise behavior through the existing CaptureAsync API and preserve the current
CI/configuration isolation setup.
Source: Coding guidelines
| withCIEnv(t, true) | ||
| atmosConfig := &schema.AtmosConfiguration{} | ||
| atmosConfig.Settings.Pro.Token = "test-token" | ||
| atmosConfig.Settings.Pro.BaseURL = "http://127.0.0.1:0" // unreachable |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Terminate the inline comment with a period.
Change // unreachable to // Unreachable.
As per coding guidelines, “All comments must end with periods.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/proexec/async_test.go` at line 76, Update the inline comment on the
atmosConfig.Settings.Pro.BaseURL assignment to read “Unreachable.” with an
initial capital letter and a terminating period.
Source: Coding guidelines
Comment on lines
+35
to
+61
| func TestCaptureSync_NoOpOnGateClosed(t *testing.T) { | ||
| withCIEnv(t, false) | ||
| atmosConfig := &schema.AtmosConfiguration{} | ||
| atmosConfig.Settings.Pro.Token = "test-token" | ||
|
|
||
| err := CaptureSync(atmosConfig, "describe affected", 0, nil) | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| // TestCaptureSync_WarnAndContinueOnFailure verifies a delivery failure (here, | ||
| // an unreachable Pro endpoint) returns nil (warn-and-continue) rather than | ||
| // propagating the upload error to the caller. | ||
| func TestCaptureSync_WarnAndContinueOnFailure(t *testing.T) { | ||
| withCIEnv(t, true) | ||
| atmosConfig := &schema.AtmosConfiguration{} | ||
| atmosConfig.Settings.Pro.Token = "test-token" | ||
| atmosConfig.Settings.Pro.BaseURL = "http://127.0.0.1:0" | ||
| atmosConfig.Settings.Pro.Exec.SyncTimeoutSeconds = defaultSyncTimeoutSeconds | ||
|
|
||
| start := time.Now() | ||
| err := CaptureSync(atmosConfig, "terraform apply", 0, nil) | ||
| elapsed := time.Since(start) | ||
|
|
||
| assert.NoError(t, err, "CaptureSync must warn-and-continue, never return the upload error") | ||
| // Must not hang indefinitely — bounded by the (clamped) sync timeout. | ||
| assert.Less(t, elapsed, 60*time.Second) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test the successful synchronous delivery path.
The tests cover a closed gate and a failed connection. They do not verify that CaptureSync sends a record and returns after a successful response.
Use a controlled local HTTP server. Assert the request method and payload, then assert that CaptureSync returns before the configured timeout.
As per coding guidelines: “Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/proexec/sync_test.go` around lines 35 - 61, Add a successful delivery
test alongside TestCaptureSync_NoOpOnGateClosed and
TestCaptureSync_WarnAndContinueOnFailure using a controlled local HTTP server.
Configure CaptureSync with the server URL and a deliberately longer sync
timeout, assert the server receives the expected request method and payload, and
verify CaptureSync returns no error before that timeout.
Source: Coding guidelines
Comment on lines
+31
to
+35
| Most commands report this in the background and never slow anything down. `terraform | ||
| plan`, `terraform apply`, and `describe affected` wait briefly to confirm the record | ||
| made it to Atmos Pro before the command finishes, so a pipeline never reports success for | ||
| a run Atmos Pro never saw — and if delivery is slow or fails, the command still | ||
| completes; it only logs a warning. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not promise delivery when upload failure preserves command success.
A timeout or upload failure logs a warning and does not change the command exit code. Critical commands can therefore complete successfully when Atmos Pro does not receive the record.
website/blog/2026-08-11-pro-exec-metadata-upload.mdx#L31-L35: say that critical commands wait up to the configured timeout and warn if delivery fails.website/src/data/roadmap.js#L493-L493: remove the claim that critical commands never report success after a missed record.
📍 Affects 2 files
website/blog/2026-08-11-pro-exec-metadata-upload.mdx#L31-L35(this comment)website/src/data/roadmap.js#L493-L493
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@website/blog/2026-08-11-pro-exec-metadata-upload.mdx` around lines 31 - 35,
The critical-command documentation must not imply that successful command
completion guarantees delivery. In
website/blog/2026-08-11-pro-exec-metadata-upload.mdx lines 31-35, state that
critical commands wait up to the configured timeout and warn when delivery
fails; in website/src/data/roadmap.js line 493, remove the claim that they never
report success after a missed record.
…into 1199-pro-exec-metadata * '1199-pro-exec-metadata' of github.com:cloudposse/atmos: Add task-runner dependencies, freshness checks, and preconditions to custom commands and workflows (#2882) feat(provisioner): Azure (azurerm) backend auto-provisioning (#2911) fix: preserve trailing newlines in text-based 3-way merges (#2891) fix(scaffold): preserve source in scaffold config (#2869) fix(deps): update github.com/epiclabs-io/diff3 digest to 3b16698 (#2917) fix(deps): update kubernetes monorepo to v0.36.3 (#2918)
Igor Rodionov (goruha)
temporarily deployed
to
screengrabs
August 11, 2026 20:53 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Igor Rodionov (goruha)
temporarily deployed
to
screengrabs
August 12, 2026 12:28 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/pro/api_client_exec_test.go (1)
190-234: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify each chunked item, not only the total count.
Every fixture item is identical. A faulty implementation can drop one item and duplicate another while still passing
totalItems == numItems.Create distinct item addresses. Assert that the received items contain each source item exactly once and in the expected sequence. Based on learnings: “for slice results assert element values rather than only length.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/pro/api_client_exec_test.go` around lines 190 - 234, The chunking test around UploadExecMetadata currently verifies only aggregate item counts, so it must validate item identity and ordering. Generate distinct addresses for each DataItems fixture, then iterate through the received bodies and assert each item matches the corresponding source item exactly once in sequence while retaining the existing chunk metadata and total-count assertions.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/pro/api_client_exec.go`:
- Around line 24-67: Update the chunk sizing flow around metadataOverhead and
sendChunked to account for BatchID, BatchIndex, and BatchTotal fields that are
added in the callback by sendExecMetadataRequest. Reserve their serialized
overhead before calculating chunk capacity, or size each chunk from the
completed chunkDTO, and add a boundary test verifying every emitted request body
remains within MaxPayloadBytes.
---
Nitpick comments:
In `@pkg/pro/api_client_exec_test.go`:
- Around line 190-234: The chunking test around UploadExecMetadata currently
verifies only aggregate item counts, so it must validate item identity and
ordering. Generate distinct addresses for each DataItems fixture, then iterate
through the received bodies and assert each item matches the corresponding
source item exactly once in sequence while retaining the existing chunk metadata
and total-count assertions.
🪄 Autofix
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 Plus
Run ID: 900f997a-381d-447a-ae0d-feb77d5f876e
📒 Files selected for processing (19)
internal/exec/describe_affected.gointernal/exec/terraform.gopacts/atmos-AtmosPro.jsonpkg/pro/api_client_exec.gopkg/pro/api_client_exec_test.gopkg/pro/consumer_pact_test.gopkg/pro/dtos/exec.gopkg/proexec/async.gopkg/proexec/async_test.gopkg/proexec/envelope.gopkg/proexec/envelope_test.gopkg/proexec/sync.gopkg/proexec/sync_test.gospecs/002-pro-exec-metadata/contracts/interactions.mdspecs/002-pro-exec-metadata/data-model.mdspecs/002-pro-exec-metadata/plan.mdspecs/002-pro-exec-metadata/research.mdspecs/002-pro-exec-metadata/spec.mdspecs/002-pro-exec-metadata/tasks.md
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/exec/describe_affected.go
- pkg/proexec/async.go
- pkg/proexec/sync_test.go
- pkg/pro/dtos/exec.go
- pkg/proexec/async_test.go
- internal/exec/terraform.go
Comment on lines
+24
to
+67
| // Estimate metadata overhead (everything except the DataItems array). | ||
| overheadDTO := dtos.ExecUploadRequest{ | ||
| AtmosProRunID: dto.AtmosProRunID, | ||
| AtmosVersion: dto.AtmosVersion, | ||
| AtmosOS: dto.AtmosOS, | ||
| AtmosArch: dto.AtmosArch, | ||
| Command: dto.Command, | ||
| Args: dto.Args, | ||
| ExitCode: dto.ExitCode, | ||
| GitSHA: dto.GitSHA, | ||
| RepoURL: dto.RepoURL, | ||
| RepoName: dto.RepoName, | ||
| RepoOwner: dto.RepoOwner, | ||
| RepoHost: dto.RepoHost, | ||
| Metrics: dto.Metrics, | ||
| Data: dto.Data, | ||
| DataItems: []json.RawMessage{}, | ||
| } | ||
| overhead := metadataOverhead(overheadDTO) | ||
|
|
||
| return sendChunked(dto.DataItems, c.MaxPayloadBytes, overhead, func(chunk []json.RawMessage, batch *BatchInfo) error { | ||
| chunkDTO := &dtos.ExecUploadRequest{ | ||
| AtmosProRunID: dto.AtmosProRunID, | ||
| AtmosVersion: dto.AtmosVersion, | ||
| AtmosOS: dto.AtmosOS, | ||
| AtmosArch: dto.AtmosArch, | ||
| Command: dto.Command, | ||
| Args: dto.Args, | ||
| ExitCode: dto.ExitCode, | ||
| GitSHA: dto.GitSHA, | ||
| RepoURL: dto.RepoURL, | ||
| RepoName: dto.RepoName, | ||
| RepoOwner: dto.RepoOwner, | ||
| RepoHost: dto.RepoHost, | ||
| Metrics: dto.Metrics, | ||
| Data: dto.Data, | ||
| DataItems: chunk, | ||
| } | ||
| if batch != nil { | ||
| chunkDTO.BatchID = batch.BatchID | ||
| chunkDTO.BatchIndex = &batch.BatchIndex | ||
| chunkDTO.BatchTotal = &batch.BatchTotal | ||
| } | ||
| return c.sendExecMetadataRequest(url, chunkDTO) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include batch fields in the payload budget.
Line 42 measures overhead without batch fields. Lines 62-65 add those fields to every chunk. A chunk sized at the configured limit can therefore exceed that limit and receive a server rejection.
Reserve batch-field overhead before sendChunked, or size chunks from the final serialized DTO. Add a boundary test that verifies every emitted body is within MaxPayloadBytes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/pro/api_client_exec.go` around lines 24 - 67, Update the chunk sizing
flow around metadataOverhead and sendChunked to account for BatchID, BatchIndex,
and BatchTotal fields that are added in the callback by sendExecMetadataRequest.
Reserve their serialized overhead before calculating chunk capacity, or size
each chunk from the completed chunkDTO, and add a boundary test verifying every
emitted request body remains within MaxPayloadBytes.
Igor Rodionov (goruha)
temporarily deployed
to
screengrabs
August 12, 2026 12:41 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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.
what
POST /v1/atmos/exec) whenever Atmos runs in a recognized CI environment with Atmos Pro configured — no new opt-in required.terraform plan,terraform apply, anddescribe affectedadditionally wait (bounded by a newsettings.pro.exec.sync_timeout_seconds, default 10s) to confirm delivery before completing, warning rather than failing on a delivery outage.pkg/metrics/processpackage captures the Atmos process's own wall-clock time, CPU time, and (on Unix) peak memory/page faults/context switches/block I/O.POST /v1/atmos/execAtmosProAPIClientmethod, following the existing retry/auth pattern used by every other Atmos Pro upload.POST /v1/atmos/exec;pacts/atmos-AtmosPro.jsonis regenerated so the Atmos Pro team has a verifiable contract to implement the provider side against.why
plan/apply/describe affected) reliably reported before the pipeline moves on.Scope note
One originally-planned capability — attaching itemized created/updated/deleted resource data from
terraform plan/applyto the execution record — is not included in this PR. Implementing it safely requires tee-ing terraform's raw stdout insideExecuteTerraform's shared pipeline without breaking streaming/TTY/masking behavior across every terraform subcommand — a separately-scoped change. Tracked in #2924.references
specs/002-pro-exec-metadata/(spec, plan, research, data-model, contracts, tasks)Summary by CodeRabbit
New Features
describe affected.Documentation
Bug Fixes