Repository navigation
Add process and I/O execution foundation - #2464
Merged
Merged
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
This was referenced May 21, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2464 +/- ##
==========================================
+ Coverage 78.59% 78.63% +0.04%
==========================================
Files 1147 1150 +3
Lines 110578 110981 +403
==========================================
+ Hits 86905 87270 +365
- Misses 18845 18867 +22
- Partials 4828 4844 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
|
CodeRabbit (@coderabbitai) is |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the code first. 🏁 Script executed: Length of output: 3851
What Is the term conventional?
Alternatives worth considering:
Of these, |
Mikhail Shirkov (shirkevich)
force-pushed
the
codex/dag-process-io-foundation
branch
from
May 25, 2026 07:46
7e70c17 to
6a5036e
Compare
Mikhail Shirkov (shirkevich)
marked this pull request as ready for review
May 26, 2026 18:07
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a subprocess Runner abstraction ( ChangesSubprocess & Stream Refactor
Sequence Diagram(s) sequenceDiagram
participant Caller
participant NewOutput
participant DefaultRunner
participant OSProcess
Caller->>NewOutput: NewOutput(OutputOptions) -> Output{Stdout,Stderr}
Caller->>DefaultRunner: Run(ctx, TaskSpec{Command,Args,Env,Streams: Output})
DefaultRunner->>OSProcess: exec.CommandContext with wired Streams
OSProcess-->>DefaultRunner: exit code / error / cancellation
DefaultRunner-->>Caller: Result{ExitCode, Err, Started, Canceled}
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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: 5
🤖 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/shell_utils_test.go`:
- Around line 197-200: Replace the hardcoded "/bin/sh" invocation in the tests
that call ExecuteShellCommand with a Go test helper process to keep tests
cross-platform: implement a TestHelperProcess helper in the test file that
checks an env var like GO_WANT_HELPER_PROCESS and writes the desired
stdout/stderr, and invoke it by calling os.Executable() (the current test
binary) with args "-test.run=TestHelperProcess" and any helper commands, plus
setting GO_WANT_HELPER_PROCESS=1 in the cmd.Env before passing the cmd to
ExecuteShellCommand; apply the same change to the other occurrence around the
ExecuteShellCommand call at the second location.
In `@internal/exec/shell_utils.go`:
- Around line 189-195: The /dev/stdout branch is wiring masked stderr to
stdoutTarget which bypasses the composed stdout capture (WithStdoutCapture);
change the MaskWriter call to wrap the composed stdout writer instead of
stdoutTarget so redirected stderr participates in stdout capture. In other
words, replace ioLayer.MaskWriter(stdoutTarget) with ioLayer.MaskWriter(stdout)
and keep the existing conditional that composes stderr with cfg.stderrCapture
(i.e., stderr = io.MultiWriter(maskedStderr, cfg.stderrCapture) or stderr =
maskedStderr) so stderr goes through the same captured stdout stream.
In `@pkg/process/process_test.go`:
- Around line 17-18: Replace the platform-specific shell fixtures and
runtime.GOOS checks by implementing a Go-native test helper process and using
os.Executable() (or inject a fake command runner) in the tests: remove hardcoded
"/bin/sh" and "sleep" calls in the tests and instead spawn the current test
binary with exec.Command(os.Executable(), "-test.run=TestHelperProcess", "--",
"<args>") and set an env var (e.g., "GO_WANT_HELPER_PROCESS=1") so that a
TestHelperProcess function in the same _test.go file handles the simulated
behavior (sleep, exit codes, stdout/stderr) in a cross-platform way;
alternatively, refactor the code under test to accept a CommandRunner interface
and inject a fake runner in tests, updating the tests that reference the shell
fixtures and the runtime.GOOS skip (the tests around the conditional at the top
and the blocks covering lines 22-24, 39-45, 56-65) to use the TestHelperProcess
or fake runner.
In `@pkg/process/process.go`:
- Around line 65-76: The Result.StartedAt is being set before the process
actually starts (and for dry runs), so update the logic in the function that
builds and runs the command (the code that constructs Result with Command/Args
and then calls cmd.Start()) to leave StartedAt zero until the process
successfully starts: do not set StartedAt when spec.DryRun is true, and only set
result.StartedAt = time.Now() immediately after a successful cmd.Start() return;
keep the defer that sets FinishedAt but ensure it runs regardless, and on
cmd.Start() failure return the Result with StartedAt zero and appropriate
ExitCode/Errors.
- Around line 88-105: Wrap the raw errors returned by cmd.Start() and cmd.Wait()
with the repo sentinel errors from errors/errors.go instead of returning them
raw: when cmd.Start() fails assign result.Err to a wrapped error using
fmt.Errorf or errors.Join with the static start sentinel (wrap the original err
with the sentinel) and set result.ExitCode = -1; when cmd.Wait() returns an
error wrap that error with the static wait sentinel (use errors.Join or
fmt.Errorf("%w", err)) before assigning result.Err, keep result.ExitCode =
exitCode(err), and if ctx.Err() is non-nil combine the wait sentinel, the wait
error and ctxErr using errors.Join so result.Err contains both the sentinel and
underlying causes; update assignments that touch result.Err, result.Started,
result.Canceled, and the handling around exitCode(err) accordingly (references:
cmd.Start(), cmd.Wait(), result.Err, result.ExitCode, result.Started,
result.Canceled, exitCode(err), ctx.Err()).
🪄 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: 68e9c1a2-c710-4aa2-8146-ef55b97400d1
📒 Files selected for processing (8)
internal/exec/shell_utils.gointernal/exec/shell_utils_test.gointernal/exec/terraform_plan_diff.gointernal/exec/terraform_plan_diff_test.gopkg/io/node_streams.gopkg/io/node_streams_test.gopkg/process/process.gopkg/process/process_test.go
Collaborator
Author
|
CodeRabbit (@coderabbitai) review Addressed the prior review threads in f9cb786:
Validation: |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/process/process.go (1)
63-110: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd perf tracking to the public runner method.
DefaultRunner.Runis a public, non-trivial function and currently misses the standard perf instrumentation on Line 63.Suggested patch
import ( "context" "errors" "fmt" "io" "os" "os/exec" "time" errUtils "github.com/cloudposse/atmos/errors" + "github.com/cloudposse/atmos/pkg/perf" ) @@ func (r DefaultRunner) Run(ctx context.Context, spec TaskSpec) (result Result) { + defer perf.Track(nil, "process.DefaultRunner.Run")() + if ctx == nil { ctx = context.Background() }As per coding guidelines "Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, except trivial getters/setters, command constructors, simple factories, delegating functions, and pure validation/lookup functions."🤖 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/process/process.go` around lines 63 - 110, Add perf tracking to DefaultRunner.Run by inserting a defer perf.Track(atmosConfig, "pkg.process.DefaultRunner.Run")() at the top of the function (immediately after the ctx nil-check or as the first statement if you prefer), followed by a blank line; update imports if necessary to include perf and atmosConfig usage. This change targets the public method DefaultRunner.Run and must place the defer before any heavy logic so the tracker wraps the entire function execution.
🤖 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/io/node_output_test.go`:
- Around line 54-55: The test currently ignores the return value of Initialize()
which can hide masking setup failures; change the test to assert initialization
succeeded before calling RegisterSecret by capturing the error from Initialize()
(e.g., err := Initialize()) and failing the test if err != nil (use t.Fatalf or
your test helper) so that Initialize() failures stop the test rather than
allowing RegisterSecret to run against an uninitialized state.
---
Outside diff comments:
In `@pkg/process/process.go`:
- Around line 63-110: Add perf tracking to DefaultRunner.Run by inserting a
defer perf.Track(atmosConfig, "pkg.process.DefaultRunner.Run")() at the top of
the function (immediately after the ctx nil-check or as the first statement if
you prefer), followed by a blank line; update imports if necessary to include
perf and atmosConfig usage. This change targets the public method
DefaultRunner.Run and must place the defer before any heavy logic so the tracker
wraps the entire function execution.
🪄 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: ed797bb5-249a-4b8d-bf21-20a333d9f824
📒 Files selected for processing (8)
docs/prd/dag-concurrent-execution.mderrors/errors.gointernal/exec/shell_utils.gointernal/exec/shell_utils_test.gopkg/io/node_output.gopkg/io/node_output_test.gopkg/process/process.gopkg/process/process_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/prd/dag-concurrent-execution.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/io/node_output_test.go (1)
54-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast if masking initialization fails.
Ignoring the return from
Initialize()can hide setup failures and make this test non-diagnostic.Suggested patch
- _ = Initialize() + require.NoError(t, Initialize()) RegisterSecret("secret-value")🤖 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/io/node_output_test.go` around lines 54 - 55, The test currently ignores the result of Initialize(); change it to capture and assert the error (e.g., call Initialize() and if it returns an error, fail the test) before calling RegisterSecret("secret-value") so initialization failures cause an immediate test failure—update the test to check Initialize()'s return and call t.Fatalf/t.Fatal (or equivalent) on error.
🧹 Nitpick comments (1)
internal/exec/shell_utils_test.go (1)
217-282: ⚡ Quick winAdd a cancellation-path test for
WithProcessContext.This new option changes
ExecuteShellCommand’s contract through theresult.Canceledbranch, but the added coverage only exercises success, exit code, and redirection. A helper mode that blocks until the context is canceled would pin that behavior down before more call sites depend on it.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 `@internal/exec/shell_utils_test.go` around lines 217 - 282, Add a unit test that verifies ExecuteShellCommand honors WithProcessContext cancellation: create a helper mode that blocks (e.g., extend shellHelperCommand with a "block-until-cancel" behavior) then call ExecuteShellCommand with a cancellable context passed via WithProcessContext(ctx) and WithProcessStreams/WithStdoutCapture as needed, cancel the context from a goroutine after a short delay, and assert the returned result indicates cancellation (check result.Canceled or that the error is context.Canceled) and that terminal/capture streams reflect any partial output; reference ExecuteShellCommand, WithProcessContext, shellHelperCommand and result.Canceled when adding the test.
🤖 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/shell_utils_test.go`:
- Around line 278-281: The test currently asserts a fixed merged order
("stdoutstderr") which is unreliable; update the assertions to check that
terminalOut.String() and captureOut.String() each contain the substrings
"stdout" and "stderr" (use assertions like assert.Contains or strings.Contains)
rather than exact equality, and keep the existing require.NoError(t, err) and
assert.Empty(t, terminalErr.String()) checks; locate these in the test around
variables terminalOut, terminalErr, and captureOut to replace the equality
checks.
In `@internal/exec/terraform_plan_diff.go`:
- Line 29: The package-level mutable seam executeTerraformForPlanDiff should be
replaced with a dependency-injected interface: define an Executor interface
(e.g., type TerraformExecutor interface { ExecuteTerraform(args ...type)
(resultType, error) } matching the existing ExecuteTerraform signature), add a
field of that interface to the type that currently relies on
executeTerraformForPlanDiff (refer to executeTerraformForPlanDiff and
ExecuteTerraform), update the constructor to accept the TerraformExecutor and
use that field instead of the package var, and generate a mock for
TerraformExecutor with go.uber.org/mock/mockgen to replace tests that currently
mutate the package-global; update tests to inject the generated mock. Ensure
signatures and return types match the original ExecuteTerraform so call sites
compile.
In `@pkg/io/node_output.go`:
- Around line 70-77: NewPrefixedWriter can return a prefixedWriter wrapping a
nil stdio.Writer which will panic on write; ensure you normalize nil writers to
stdio.Discard. In NewPrefixedWriter, if w is nil set w = stdio.Discard before
any returns so both branches (prefix=="" and the prefixedWriter construction)
never return a nil writer; update references to NewPrefixedWriter and
prefixedWriter accordingly.
---
Duplicate comments:
In `@pkg/io/node_output_test.go`:
- Around line 54-55: The test currently ignores the result of Initialize();
change it to capture and assert the error (e.g., call Initialize() and if it
returns an error, fail the test) before calling RegisterSecret("secret-value")
so initialization failures cause an immediate test failure—update the test to
check Initialize()'s return and call t.Fatalf/t.Fatal (or equivalent) on error.
---
Nitpick comments:
In `@internal/exec/shell_utils_test.go`:
- Around line 217-282: Add a unit test that verifies ExecuteShellCommand honors
WithProcessContext cancellation: create a helper mode that blocks (e.g., extend
shellHelperCommand with a "block-until-cancel" behavior) then call
ExecuteShellCommand with a cancellable context passed via
WithProcessContext(ctx) and WithProcessStreams/WithStdoutCapture as needed,
cancel the context from a goroutine after a short delay, and assert the returned
result indicates cancellation (check result.Canceled or that the error is
context.Canceled) and that terminal/capture streams reflect any partial output;
reference ExecuteShellCommand, WithProcessContext, shellHelperCommand and
result.Canceled when adding the test.
🪄 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: 79593cac-20b6-47a3-9c20-567989b80e8c
📒 Files selected for processing (10)
docs/prd/dag-concurrent-execution.mderrors/errors.gointernal/exec/shell_utils.gointernal/exec/shell_utils_test.gointernal/exec/terraform_plan_diff.gointernal/exec/terraform_plan_diff_test.gopkg/io/node_output.gopkg/io/node_output_test.gopkg/process/process.gopkg/process/process_test.go
Collaborator
Author
|
CodeRabbit (@coderabbitai) review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
CodeRabbit (@coderabbitai) review |
Contributor
✅ Actions performedReview triggered.
|
Erik Osterman (Cloud Posse) (osterman)
added
the
no-release
Erik Osterman (Cloud Posse) (osterman)
approved these changes
May 27, 2026
Collaborator
Author
|
CodeRabbit (@coderabbitai) review |
Contributor
✅ Actions performedReview triggered.
|
Erik Osterman (Cloud Posse) (osterman)
previously approved these changes
May 29, 2026
Andriy Knysh (aknysh)
previously approved these changes
May 29, 2026
* add dag scheduler core * Address CodeRabbit scheduler feedback * Ensure scheduler aggregates non-success statuses
Andriy Knysh (aknysh)
dismissed stale reviews from Erik Osterman (Cloud Posse) (osterman), coderabbitai[bot], and themself
via
May 29, 2026 23:33
3467269
Andriy Knysh (aknysh)
approved these changes
May 30, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.221.0-rc.2. |
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is PR 1 for the DAG concurrent execution rollout. It introduces the reusable process and stream-isolation foundation without enabling scheduler behavior or changing Terraform bulk routing.
Changes:
pkg/processwithRunner,TaskSpec,Streams,Result, defaultos/execrunner, context-aware execution, cancellation reporting, and exit-code preservation.pkg/iowith prefixed per-node stream composition for terminal, file, and capture sinks.internal/exec.ExecuteShellCommand()into a backward-compatible wrapper overpkg/processwhile preserving CI stdout/stderr capture options.runTerraformShow()globalos.Stdoutswap with injected stdout capture.Scope
No scheduler, CLI routing consolidation, concurrency flags, or Terraform adapter behavior is enabled in this PR.
Stacking
This PR is the bottom of the DAG rollout stack and targets
main.Supersedes the earlier fork-headed draft #2459 now that the stack branches exist in
cloudposse/atmos.Validation
rtk env GOCACHE=/private/tmp/atmos-gocache GOMODCACHE=/private/tmp/atmos-gomodcache go test ./pkg/process ./pkg/io ./internal/exec ./cmd/terraformNext PR
PR 2 branches from
codex/dag-process-io-foundationand adds the genericpkg/schedulercore with ready-queue scheduling, bounded workers, deterministic aggregate results, and isolated unit tests only.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation