Add parallel and matrix workflow control steps - #2635
Merged
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
marked this pull request as ready for review
June 20, 2026 02:33
Erik Osterman (Cloud Posse) (osterman)
requested review from
a team
as code owners
June 20, 2026 02:33
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
|
Warning Release Documentation RequiredThis PR is labeled
|
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:
📝 WalkthroughWalkthroughAdds parallel, matrix, and background workflow control steps, plus schema, validation, execution wiring, examples, and docs updates. ChangesParallel, Matrix, and Background Workflow Control Steps
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 10
🧹 Nitpick comments (4)
pkg/workflow/control.go (2)
89-89: ⚡ Quick winInstrument the public entrypoint with perf tracking.
ExecuteControlStepis a public, non-trivial function and should adddefer perf.Track(nil, "workflow.ExecuteControlStep")()near the top.As per coding guidelines: "Add
defer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param."🤖 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/workflow/control.go` at line 89, Add performance tracking instrumentation to the ExecuteControlStep function by inserting `defer perf.Track(nil, "workflow.ExecuteControlStep")()` as the first statement in the function body, immediately after the opening brace, followed by a blank line. Use `nil` for the atmosConfig parameter since the function does not have an atmosConfig parameter available.Source: Coding guidelines
35-89: ⚡ Quick winAdd Go doc comments to exported control-step API symbols.
These exported declarations are missing doc comments, which will drift API clarity and can trip lint policy for exported symbols.
As per coding guidelines: "Document all exported functions, types, and methods following Go's documentation conventions."
🤖 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/workflow/control.go` around lines 35 - 89, Add Go doc comments to all exported symbols in the control.go file that are currently missing documentation. The following exported types and functions need doc comments: ControlChild, ControlChildOutput, ControlChildResult, ControlChildExecutor, ControlTemplateDataFunc, ControlStoreResultFunc, ControlExecutionOptions, ControlResult, and ExecuteControlStep. Each doc comment should be placed directly above the symbol declaration and follow Go conventions by starting with the symbol name and providing a clear, concise description of its purpose. For example, "ControlChild represents..." or "ExecuteControlStep executes...".Source: Coding guidelines
pkg/workflow/control_executor.go (2)
47-47: ⚡ Quick winAdd perf tracking to
ControlCommandExecutor.Execute.This is a public method with non-trivial behavior; it should include
defer perf.Track(nil, "workflow.ControlCommandExecutor.Execute")().As per coding guidelines: "Add
defer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions. Usenilif no atmosConfig param."🤖 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/workflow/control_executor.go` at line 47, The public method Execute in the ControlCommandExecutor struct is missing performance tracking instrumentation. Add a defer statement with perf.Track(nil, "workflow.ControlCommandExecutor.Execute")() as the first line inside the Execute method body, followed by a blank line. This follows the coding guideline that all public functions should include perf.Track instrumentation with nil as the first parameter when no atmosConfig is available.Source: Coding guidelines
22-47: ⚡ Quick winDocument exported executor types and function signatures.
The exported API surface in this file should have Go doc comments for maintainability and lint consistency.
As per coding guidelines: "Document all exported functions, types, and methods following Go's documentation conventions."
🤖 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/workflow/control_executor.go` around lines 22 - 47, Add Go doc comments to all exported types and the exported method in this file to follow Go's documentation conventions. For each exported type (ControlEnvironmentFunc, ControlCommandRequest, ControlCommandRunner, ControlCommandExecutor) and the exported method (Execute), add a comment line immediately preceding the declaration that starts with the name of the exported symbol and describes its purpose. Ensure comments follow standard Go documentation format to satisfy lint requirements and improve code maintainability.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 `@examples/parallel-steps/atmos.yaml`:
- Around line 6-8: The logs.file configuration in the atmos.yaml example is
hardcoded to the Unix-specific path `/dev/stderr`, which will not work on native
Windows systems. To fix this, either remove the logs.file entry entirely to rely
on the default stderr behavior, or add clear documentation explaining how users
should configure platform-specific log paths for Windows environments to ensure
cross-platform compatibility.
In `@examples/parallel-steps/README.md`:
- Around line 5-13: The Try It section in the README lacks a platform
prerequisite note that would prevent failed first runs on native Windows. Add a
short one-liner before the shell code block containing the atmos workflow
commands (atmos workflow checks, atmos workflow prefixed, atmos workflow matrix)
to indicate that these commands require a POSIX shell environment or WSL on
Windows. This ensures users on Windows understand the platform compatibility
requirement before attempting to run the provided commands.
In `@internal/exec/workflow_utils.go`:
- Around line 416-423: In the case branch for schema.TaskTypeParallel and
schema.TaskTypeMatrix, the executeWorkflowControlStep function call is passing
the raw commandLineIdentity parameter to the workflowControlContext, but the
stepIdentity variable has already been resolved earlier in the function to
account for step-level identity overrides combined with the CLI fallback.
Replace the commandLineIdentity argument with stepIdentity in the
executeWorkflowControlStep call to ensure nested steps execute under the correct
resolved identity rather than discarding the step override information.
In `@pkg/schema/task_validate.go`:
- Around line 123-160: The `validateControlSteps` function currently calls
`collectWorkflowStepNames` unconditionally to deduplicate step names at the top
level, but only validates the needs DAG via `validateNeedsGraph` when
`inConcurrentGroup` is true. This rejects legacy sequential workflows with
duplicate names while allowing unvalidated `needs` dependencies in non-control
steps. Move the calls to `collectWorkflowStepNames` and `validateNeedsGraph`
inside the `if inConcurrentGroup` block so that name deduplication and DAG
validation only occur within concurrent groups, then call
`validateControlStepList` unconditionally as it will handle validation for both
concurrent and sequential workflows.
In `@pkg/schema/task.go`:
- Around line 456-482: The normalizeTaskOutputMap function is removing the
"output" key from the map even when the mapstructure decoder creation or the
Decode operation fails, which causes malformed structured output to be silently
ignored. Modify the function to check if either creating the decoder or calling
decoder.Decode returns an error, and if so, return that error instead of
proceeding with the map copying logic. Only proceed with removing "output" from
the map when both the decoder is successfully created and the Decode operation
completes without error, ensuring that validation errors are properly surfaced
rather than silently treated as defaults.
In `@pkg/schema/workflow_control_test.go`:
- Around line 75-81: The test case "interactive child disallowed" currently only
covers the unsupported-type code path by using Type: "input" which generates the
"cannot run inside concurrent step" error. Add a separate test case that uses an
allowed task type (such as TaskTypeTask or similar supported type) for the child
step within the parallel steps array, set either Interactive: true or Tty: true
on that child step, and expect the error message "cannot set tty or interactive"
to directly exercise the interactive and TTY validation branch for allowed-type
children.
In `@pkg/workflow/control_executor.go`:
- Around line 91-93: The hardcoded "sh" program and "-c" arguments in the
Command configuration will not work on Windows where sh is unavailable. Replace
the hardcoded Program and Args values with cross-platform logic that selects
"sh" with "-c" on Linux/macOS and "cmd.exe" with "/c" on Windows. Use the
runtime os package to detect the operating system at execution time or use build
tags to conditionally compile the appropriate command configuration. This
ensures the step execution works consistently across all supported platforms.
In `@pkg/workflow/control_matrix.go`:
- Around line 32-43: The matrixRowSuffix function applies lossy sanitization via
sanitizeControlName to individual axis values before joining them, which can
cause different raw values to converge to the same token or sanitize to empty
strings, creating duplicate node IDs in buildMatrixGraph. Instead of sanitizing
each part separately, join the raw unsanitized values with controlNameSep first,
then apply sanitization to the complete joined string once, or implement an
approach that preserves the uniqueness of the original values (such as using a
hash or encoding) while still producing valid control names that won't collide
for distinct matrix inputs.
In `@tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 1289-1416: The workflow_step schema definition has
additionalProperties set to false, which restricts it to only explicitly defined
properties, but the timeout property is missing even though it is supported by
WorkflowStep. Add the timeout property to the properties object within the
workflow_step definition with appropriate type and description, similar to how
working_directory is defined. Additionally, audit the complete list of
WorkflowStep fields to ensure no other properties were inadvertently omitted
during the inline-to-shared refactor that would cause valid manifests to fail
validation.
In `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 1289-1416: The workflow_step schema definition in the
atmos-manifest.json file is missing the timeout property, which causes
validation failures for valid sleep workflow steps since additionalProperties is
set to false. Add a timeout property to the properties object in the
workflow_step definition that matches the implementation added to the fixture
schema, ensuring the published schema validation is consistent with the fixture
schema validation and allows timeout to be used in workflow steps.
---
Nitpick comments:
In `@pkg/workflow/control_executor.go`:
- Line 47: The public method Execute in the ControlCommandExecutor struct is
missing performance tracking instrumentation. Add a defer statement with
perf.Track(nil, "workflow.ControlCommandExecutor.Execute")() as the first line
inside the Execute method body, followed by a blank line. This follows the
coding guideline that all public functions should include perf.Track
instrumentation with nil as the first parameter when no atmosConfig is
available.
- Around line 22-47: Add Go doc comments to all exported types and the exported
method in this file to follow Go's documentation conventions. For each exported
type (ControlEnvironmentFunc, ControlCommandRequest, ControlCommandRunner,
ControlCommandExecutor) and the exported method (Execute), add a comment line
immediately preceding the declaration that starts with the name of the exported
symbol and describes its purpose. Ensure comments follow standard Go
documentation format to satisfy lint requirements and improve code
maintainability.
In `@pkg/workflow/control.go`:
- Line 89: Add performance tracking instrumentation to the ExecuteControlStep
function by inserting `defer perf.Track(nil, "workflow.ExecuteControlStep")()`
as the first statement in the function body, immediately after the opening
brace, followed by a blank line. Use `nil` for the atmosConfig parameter since
the function does not have an atmosConfig parameter available.
- Around line 35-89: Add Go doc comments to all exported symbols in the
control.go file that are currently missing documentation. The following exported
types and functions need doc comments: ControlChild, ControlChildOutput,
ControlChildResult, ControlChildExecutor, ControlTemplateDataFunc,
ControlStoreResultFunc, ControlExecutionOptions, ControlResult, and
ExecuteControlStep. Each doc comment should be placed directly above the symbol
declaration and follow Go conventions by starting with the symbol name and
providing a clear, concise description of its purpose. For example,
"ControlChild represents..." or "ExecuteControlStep executes...".
🪄 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: 429116be-1161-4ecb-9409-598c9ac35013
📒 Files selected for processing (20)
examples/parallel-steps/README.mdexamples/parallel-steps/atmos.yamlexamples/parallel-steps/workflows/parallel.yamlinternal/exec/workflow_control_adapter.gointernal/exec/workflow_utils.gopkg/runner/step/matrix.gopkg/runner/step/parallel.gopkg/runner/step/variables.gopkg/schema/task.gopkg/schema/task_validate.gopkg/schema/workflow.gopkg/schema/workflow_control_test.gopkg/workflow/control.gopkg/workflow/control_executor.gopkg/workflow/control_executor_test.gopkg/workflow/control_matrix.gopkg/workflow/control_test.gopkg/workflow/executor.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2635 +/- ##
==========================================
+ Coverage 80.44% 80.54% +0.09%
==========================================
Files 1444 1454 +10
Lines 134768 135897 +1129
==========================================
+ Hits 108416 109459 +1043
- Misses 20349 20404 +55
- Partials 6003 6034 +31
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/workflow_utils_test.go (1)
871-898: ⚡ Quick winAdd the non-fallback counterpart test for identity resolution.
This test covers the fallback path (
commandLineIdentity). Please add a paired negative-path case where a step/parent identity is explicitly set, and assert fallback is not used.As per coding guidelines, "Include negative-path tests for recovery logic... add a corresponding test that verifies the recovery does NOT trigger when condition X is absent."
🤖 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/workflow_utils_test.go` around lines 871 - 898, Add a paired negative-path test to complement TestExecuteWorkflowControlStepUsesResolvedIdentityFallback that verifies the fallback behavior does NOT trigger when an explicit identity is set. Create a new test function that sets an explicit identity field on the parent WorkflowStep (or child step), passes a different commandLineIdentity to the workflowControlContext, calls executeWorkflowControlStep the same way as the existing test, and then asserts that authManager.identities contains the explicit identity value rather than the commandLineIdentity fallback. This ensures the recovery logic correctly prioritizes explicit identity over the fallback path.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 `@internal/exec/workflow_utils_test.go`:
- Around line 857-870: Replace the manually created
controlStepIdentityAuthManager mock struct with a mockgen-generated mock of the
auth.AuthManager interface. Remove the entire controlStepIdentityAuthManager
type definition and its methods (GetCachedCredentials and
PrepareShellEnvironment), then generate the mock using mockgen for
auth.AuthManager. Update the test to use the generated mock and set explicit
expectations on the mock for GetCachedCredentials and PrepareShellEnvironment
method calls to verify they are invoked with the expected arguments.
---
Nitpick comments:
In `@internal/exec/workflow_utils_test.go`:
- Around line 871-898: Add a paired negative-path test to complement
TestExecuteWorkflowControlStepUsesResolvedIdentityFallback that verifies the
fallback behavior does NOT trigger when an explicit identity is set. Create a
new test function that sets an explicit identity field on the parent
WorkflowStep (or child step), passes a different commandLineIdentity to the
workflowControlContext, calls executeWorkflowControlStep the same way as the
existing test, and then asserts that authManager.identities contains the
explicit identity value rather than the commandLineIdentity fallback. This
ensures the recovery logic correctly prioritizes explicit identity over the
fallback path.
🪄 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: d3619ca2-d5e6-448c-9831-20ebbf0ab94b
📒 Files selected for processing (14)
examples/parallel-steps/README.mdexamples/parallel-steps/atmos.yamlinternal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gopkg/schema/task.gopkg/schema/task_test.gopkg/schema/task_validate.gopkg/schema/workflow_control_test.gopkg/workflow/control_executor.gopkg/workflow/control_executor_test.gopkg/workflow/control_matrix.gopkg/workflow/control_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
💤 Files with no reviewable changes (1)
- examples/parallel-steps/atmos.yaml
✅ Files skipped from review due to trivial changes (1)
- examples/parallel-steps/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/exec/workflow_utils.go
- tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
- pkg/workflow/control_executor_test.go
- pkg/schema/task_validate.go
- pkg/workflow/control_executor.go
- pkg/schema/task.go
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…p-type # Conflicts: # pkg/runner/step/variables.go # pkg/schema/task.go # pkg/schema/workflow.go # pkg/workflow/executor.go
Migrate the parallel and matrix workflow control steps into the new per-step-type documentation layout introduced by the native container steps PR (#2626). Adds parallel.mdx and matrix.mdx under steps/type/, a new "Orchestration Types" section in the step type index and the shared step-types partial, sidebar entries, and a doc reference from the release blog post. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/runner/task_test.go (1)
218-283: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
providerandruntime_auto_starttop-level in this fixture.The new contract says only action-specific fields move under
with:; cross-cutting container modifiers stay on the step itself. Leaving these nested here locks the test suite to an ambiguous shape and can hide schema/validation regressions.Suggested fixture shape.
- name: build type: container action: build + provider: docker + runtime_auto_start: true with: - provider: docker - runtime_auto_start: true engine: buildx context: . dockerfile: Dockerfile tags: - app:local @@ - assert.Equal(t, "docker", task.Build.Provider) - assert.True(t, task.Build.RuntimeAutoStart) + assert.Equal(t, "docker", task.Provider) + assert.True(t, task.RuntimeAutoStart)Based on PR objectives, “Only cross-cutting execution modifiers stay top-level (provider, runtime_auto_start, container).”
🤖 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/runner/task_test.go` around lines 218 - 283, Update the container-action YAML fixture in TestTasks_UnmarshalYAML_WithContainerActionBlocksAndOutputs so only action-specific fields remain under with:, and keep provider and runtime_auto_start at the top level of the task. Adjust the assertions on task.Build accordingly to reflect the new shape while still verifying Build, Bake, and Outputs are unmarshaled correctly. Use the existing Task, Tasks, and task.Build references to keep the test aligned with the intended schema contract.
🧹 Nitpick comments (1)
pkg/background/registry_test.go (1)
12-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a generated mock for the registry handle.
fakeHandleis a new handwritten mock/fake. This repo’s Go test rules ask for mockgen-generated mocks instead, so interface drift is caught in one place. As per coding guidelines, “Generate mocks withgo.uber.org/mock/mockgen… Never manual mocks.”🤖 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/background/registry_test.go` around lines 12 - 23, The test currently adds a handwritten `fakeHandle` for the registry handle interface, but this repo requires generated mocks so interface changes are caught centrally. Replace `fakeHandle` in `registry_test.go` with a `mockgen`-generated mock for the handle interface used by the registry tests, and update the test setup to use that generated mock instead of the manual implementation. Keep the same `Name`, `WaitReady`, and `Stop` expectations in the tests, but move the behavior into the generated mock methods and expectations.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 `@examples/background-steps/workflows/background.yaml`:
- Around line 36-68: The background-steps example is misleading because the two
background services in `fanout` still start serially, so `wait-all` does not
demonstrate concurrent startup. Update the workflow under `fanout` so the
`redis` and `web` steps are launched without waiting on each other and only
synchronize at `wait-all`, using the existing `background` step pattern and the
`wait-all` gate to clearly show parallel startup behavior.
In `@internal/exec/workflow_utils.go`:
- Around line 309-313: The deferred cleanup in workflow_utils.go is dropping
StopAll failures whenever retErr is already set, so teardown problems are lost
on the failure path. Update the defer that calls bgRegistry.StopAll(runCtx) to
always combine any stopErr with retErr using errors.Join instead of only
assigning when retErr is nil. Keep the fix localized to the cleanup logic around
retErr and StopAll so both the step failure and teardown failure are preserved.
- Around line 301-308: Background instance keys are still using shared workflow
identifiers instead of being scoped per run, which can let concurrent executions
collide on the same container. Update the background stack selection in the
workflow utilities flow that builds `bgStack` for `workflowPkg.ContainerRunner`
so it uses a run-specific identifier rather than `workflowDefinition.Stack` or
the fallback `workflow` name, while preserving the existing command-line
override path. Keep the change localized around the `ContainerRunner`
construction and the `bgStack` resolution logic.
In `@pkg/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 1703-1708: The `background` schema description is misleading
because `WorkflowStep` currently treats `background` as a boolean for async
execution, while string values are used for style color elsewhere. Update the
`background` field description in the manifest schema to match the actual
runtime contract, and make sure the wording only documents the supported boolean
behavior for workflow steps rather than implying a string handle reference.
In `@pkg/datafetcher/schema/config/global/1.0.json`:
- Around line 1346-1350: The step-level retry schema is currently a free-form
object, which bypasses validation for unsupported keys. Update the retry
property in the config schema to reference the existing workflow_retry
definition instead of allowing arbitrary properties, and keep the change aligned
with the surrounding schema entries in the same JSON file so editor validation
matches the runtime retry shape.
In `@pkg/datafetcher/schema/stacks/stack-config/1.0.json`:
- Around line 1407-1412: Update the schema entry for background in the stack
config JSON so its description matches the current Go contract: string values
should describe style color usage and boolean values should describe
async/background execution. Adjust the background field documentation in the
relevant schema block only, keeping the oneOf types intact and ensuring the text
no longer refers to background-step references.
- Around line 1358-1444: Restore the default-command guard in the step schema so
objects that omit type are still required to provide command-like input. Update
the workflow step definition in stack-config JSON to add a conditional
requirement for the default command step branch, using the existing step
properties (type, command, and the step object schema) so cases like a bare
name-only step are rejected while explicit non-command step types remain
unchanged.
In `@pkg/runner/step/container_actions_extra_test.go`:
- Around line 213-224: The test in effectiveRunStep only checks run.Mounts
length, so it can miss regressions where the merged mount content is lost.
Update the container_actions_extra_test assertion for effectiveRunStep to verify
the actual Mounts element fields (using the schema.ContainerMount value from the
run.Mounts slice) in addition to, or instead of, require.Len so the new with:
merge behavior is truly covered.
In `@pkg/schema/task.go`:
- Around line 177-181: Update the comment on BackgroundAsync in task.go to match
the current validator behavior: it should describe background:true as applying
only to container steps in v1, not command steps. Adjust the wording near
BackgroundAsync (and keep the For comment consistent if needed) so the struct
tags and comments reflect the actual accepted YAML behavior.
In `@pkg/schema/workflow.go`:
- Around line 419-429: The decodeStringOrSlice helper is accepting every scalar
as a valid step name, so non-string values like booleans or numbers are being
coerced instead of rejected. Update decodeStringOrSlice to only wrap scalar
nodes when the node.Tag indicates a string (for example, !!str), and otherwise
return an error; keep the existing sequence handling via node.Decode unchanged.
- Around line 343-356: Reset WorkflowStep before decoding in UnmarshalYAML so
sanitized.Decode does not merge into a reused receiver and leave stale fields
behind. Update WorkflowStep.UnmarshalYAML to decode into a zero-value plain
WorkflowStep temporary first, then copy/apply it to step before calling
applyStepPolymorphicNodes. Add a regression test around repeated unmarshals of
WorkflowStep to verify omitted fields are cleared on the second decode.
In `@pkg/workflow/background_container.go`:
- Around line 120-126: The Stop method on containerHandle is returning
container.Down errors directly, which drops the static sentinel and step
context. Update containerHandle.Stop to wrap any container.Down failure at the
handle boundary using the existing static error from errors/errors.go, and
include the same step-name/component context that Start adds so Registry.StopAll
preserves consistent error wrapping.
In `@pkg/workflow/background.go`:
- Around line 74-77: The background teardown in the workflow cleanup path
removes the handle even when handle.Stop(ctx) fails, preventing deferred StopAll
from retrying. Update the cleanup logic around handle.Stop(ctx) and
reg.Remove(name) so the handle is only removed from the registry after a
successful stop, while keeping failed stops registered for the later StopAll
retry.
---
Outside diff comments:
In `@pkg/runner/task_test.go`:
- Around line 218-283: Update the container-action YAML fixture in
TestTasks_UnmarshalYAML_WithContainerActionBlocksAndOutputs so only
action-specific fields remain under with:, and keep provider and
runtime_auto_start at the top level of the task. Adjust the assertions on
task.Build accordingly to reflect the new shape while still verifying Build,
Bake, and Outputs are unmarshaled correctly. Use the existing Task, Tasks, and
task.Build references to keep the test aligned with the intended schema
contract.
---
Nitpick comments:
In `@pkg/background/registry_test.go`:
- Around line 12-23: The test currently adds a handwritten `fakeHandle` for the
registry handle interface, but this repo requires generated mocks so interface
changes are caught centrally. Replace `fakeHandle` in `registry_test.go` with a
`mockgen`-generated mock for the handle interface used by the registry tests,
and update the test setup to use that generated mock instead of the manual
implementation. Keep the same `Name`, `WaitReady`, and `Stop` expectations in
the tests, but move the behavior into the generated mock methods and
expectations.
🪄 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: f11b2125-8eb4-4e1e-8bd9-e89a3a04f6da
📒 Files selected for processing (33)
docs/prd/parallel-workflow-steps.mddocs/prd/workflow-step-types.mdexamples/background-steps/README.mdexamples/background-steps/atmos.yamlexamples/background-steps/workflows/background.yamlexamples/container-step/atmos.yamlexamples/container-step/workflows/container-step.yamlinternal/exec/workflow_utils.gopkg/background/registry.gopkg/background/registry_test.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/hooks/step_engine.gopkg/hooks/step_engine_test.gopkg/runner/step/background_steps.gopkg/runner/step/command_handlers_test.gopkg/runner/step/container_actions_extra_test.gopkg/runner/step/container_inspect.gopkg/runner/step/container_inspect_test.gopkg/runner/step/container_push_test.gopkg/runner/step/container_run.gopkg/runner/step/container_test.gopkg/runner/task_test.gopkg/schema/background_validate_test.gopkg/schema/task.gopkg/schema/task_validate.gopkg/schema/workflow.gopkg/schema/workflow_with_test.gopkg/workflow/background.gopkg/workflow/background_container.gopkg/workflow/background_test.gopkg/workflow/executor_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/prd/workflow-step-types.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/schema/task_validate.go
`provider` and `runtime_auto_start` are top-level cross-cutting modifiers that apply to every container action, but effectiveBuildStep/effectivePushStep read only the `with:` block — so the bake-build-run example (which needs `provider: docker` at the step top level) failed validation with "required field missing: build.provider". Make build/push fall the top-level modifiers through, consistent with effectiveRunStep/effectiveInspectStep. Fixes the [container-step] example CI job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve CodeRabbit review comments on PR #2635: - Make `background: true` non-blocking and move the implicit readiness gate to fire before the next foreground step (GatePendingBackground), so consecutive background services start concurrently and `wait-all` is meaningful (the eager per-step gate previously serialized startup). - Join background teardown errors into the workflow result instead of dropping them when a step already failed (errors.Join). - Keep a background handle registered when cancel's Stop fails, so the deferred StopAll can retry teardown. - Wrap container.Down failures at the handle boundary with the static sentinel + step-name context. - Reset WorkflowStep/Task before YAML decode so a reused receiver does not retain fields omitted from the next document. - Reject non-string scalars for `for:` (`for: true`/`for: 1` now error). - Scope background container instance names per run (uuid suffix) while preserving the `--stack` override, so concurrent runs don't collide. - Schema: correct the `background:` descriptions (string = style color), point step `retry` at #/definitions/workflow_retry, and require a step to declare `type` or `command` (anyOf) so a bare name-only step is rejected. - Update BackgroundAsync comments (container, not command) and assert the merged mount element value in the effectiveRunStep test. Add regression tests: non-blocking start, gate-once-then-skip, gate surfaces unhealthy without marking ready, failed-cancel-keeps-registered (negative path), reused-receiver reset, and `for:` scalar rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/workflow/background_test.go (1)
16-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a generated mock for
background.Handlehere.This handwritten double will drift when the interface changes. Swapping it for
mockgenoutput keeps the test aligned with the repo’s mock policy. As per coding guidelines, "Use interfaces + dependency injection for testability. Generate mocks withgo.uber.org/mock/mockgenusing//go:generatedirectives. Never manual mocks."🤖 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/workflow/background_test.go` around lines 16 - 26, The test is using a handwritten fake for background.Handle, which should be replaced with a generated mock to match the repo’s mock policy. Update the background_test.go setup to use a mock generated by go.uber.org/mock/mockgen, and keep the expectations/behavior around Name, WaitReady, and Stop in the test using that mock instead of fakeHandle. Locate the affected test helpers and any references to fakeHandle so the test stays aligned with background.Handle as the interface evolves.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 `@internal/exec/workflow_utils.go`:
- Around line 305-317: The background registry/gating logic in workflow_utils.go
is using step.Name as the key, which lets duplicate top-level names overwrite
earlier background handles and can leave services running. Update the validation
or schema rules to reject duplicate names for any step that can be targeted by
background operations, or change the registry state in the background execution
path to use a stable per-step execution ID instead of the display name. Make
sure the affected workflow execution and stop/cancel flow around bgRegistry,
bgGated, and bgRunner all use the same unique keying strategy.
In `@pkg/workflow/background.go`:
- Around line 67-83: GatePendingBackground currently assumes reg is always
non-nil, but the contract says a nil registry should be a no-op. Update
GatePendingBackground to handle reg == nil before calling reg.Names(), or revise
the function’s documented behavior if nil should not be supported, so callers
are not relying on a panic-free path that does not exist.
---
Nitpick comments:
In `@pkg/workflow/background_test.go`:
- Around line 16-26: The test is using a handwritten fake for background.Handle,
which should be replaced with a generated mock to match the repo’s mock policy.
Update the background_test.go setup to use a mock generated by
go.uber.org/mock/mockgen, and keep the expectations/behavior around Name,
WaitReady, and Stop in the test using that mock instead of fakeHandle. Locate
the affected test helpers and any references to fakeHandle so the test stays
aligned with background.Handle as the interface evolves.
🪄 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: 50b542ee-371d-404c-98f3-f1a97995aa1e
📒 Files selected for processing (11)
internal/exec/workflow_utils.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/runner/step/container_actions_extra_test.gopkg/schema/task.gopkg/schema/workflow.gopkg/schema/workflow_control_test.gopkg/workflow/background.gopkg/workflow/background_container.gopkg/workflow/background_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/datafetcher/schema/config/global/1.0.json
- pkg/datafetcher/schema/atmos/manifest/1.0.json
- pkg/datafetcher/schema/stacks/stack-config/1.0.json
- pkg/schema/workflow.go
- pkg/workflow/background_container.go
- pkg/schema/task.go
- pkg/runner/step/container_actions_extra_test.go
…stry Address CodeRabbit review feedback on PR #2635: - Duplicate top-level step names could leak background services: the registry keys handles by step name, so a second `background: true` step with the same name overwrote (and orphaned) the first handle, hiding it from wait/cancel/StopAll. validateBackgroundSteps now rejects a background name that is still live, while still allowing reuse after the earlier step is cancelled (mirroring the registry's Register/Remove lifecycle). - GatePendingBackground documented a nil registry as a no-op but would panic on reg.Names(); add an explicit nil guard so the contract holds. Add tests: duplicate-live-name rejected, post-cancel name reuse allowed (negative path), and nil-registry gate is a no-op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TestResolveImportPaths_MixedPaths flaked on the Windows CI runner:
open C:\...\cache\atmos\stack-imports\<hash>.yaml:
The system cannot find the path specified.
The remote-import tests drive the package-global importer, which lazily
builds a real-user-cache-backed importer via getGlobalImporter() ->
globalImporterOnce.Do(). The tests tried to isolate it by assigning a
temp-cache importer after `globalImporterOnce = sync.Once{}`, but a freshly
reset Once still runs on the next getGlobalImporter() call and overwrites the
injection with a real-cache importer. So every remote-import test actually
wrote to (and read back from) the shared user cache dir — which races/flakes
on CI runners (Windows most visibly).
Add useTestGlobalImporter(t, cfg) which primes the Once (consumes it via
Do) so the injected temp-cache importer survives, and restores the globals on
cleanup. Route all remote-import tests through it.
Verified: with XDG_CACHE_HOME pointed at an empty dir, the package no longer
writes any stack-imports/*.yaml there — downloads stay in t.TempDir().
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e generated mock Raise patch coverage on the background-steps feature and address review feedback. Coverage (previously 0% on the changed lines): - pkg/workflow/background_container.go: unit-test the pure mapping helpers (toPortBindings/toMounts/toRestartPolicy/toHealthCheck/resolveHealthTest/ firstNonEmpty) and the dry-run paths of ContainerRunner.Start + the handle's Name/WaitReady/Stop. The dry-run Start exercises the full config build and handle creation without a container runtime; error paths cover the missing image and unparseable-command guards. - pkg/runner/step/background_steps.go: 13.79% -> 100% — wait/wait-all/cancel handler Validate (for: required vs not), Execute (refuses to run outside the executor), and registry registration. Review feedback (CodeRabbit nitpick): replace the hand-written background.Handle fake in background_test.go with a mockgen-generated MockHandle/MockRunner (repo policy: no manual mocks). Add a //go:generate directive and route every background test through the generated mocks; call-count expectations are now enforced by gomock .Times(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/stack/imports/remote_test.go`:
- Around line 43-48: The helper comment in useTestGlobalImporter no longer
matches the cleanup behavior: the cleanup path resets globalImporterOnce,
globalImporter, and globalImporterErr instead of restoring prior state. Update
the comment to describe that the singleton is reset on cleanup and that stacked
use is not safe; keep the wording aligned with the useTestGlobalImporter and
getGlobalImporter flow so readers understand the injection is temporary, not
restored.
🪄 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: ff6324e1-00f2-468d-b878-f31408e5247b
📒 Files selected for processing (17)
internal/exec/workflow_utils.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/runner/step/background_steps_test.gopkg/runner/step/container_actions_extra_test.gopkg/schema/background_validate_test.gopkg/schema/task.gopkg/schema/task_validate.gopkg/schema/workflow.gopkg/schema/workflow_control_test.gopkg/stack/imports/remote_test.gopkg/workflow/background.gopkg/workflow/background_container.gopkg/workflow/background_container_test.gopkg/workflow/background_test.gopkg/workflow/mock_background_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/workflow/mock_background_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/datafetcher/schema/config/global/1.0.json
- pkg/datafetcher/schema/atmos/manifest/1.0.json
- pkg/schema/background_validate_test.go
- pkg/datafetcher/schema/stacks/stack-config/1.0.json
- pkg/workflow/background.go
- pkg/runner/step/container_actions_extra_test.go
- pkg/workflow/background_container.go
- pkg/schema/workflow_control_test.go
- pkg/schema/task.go
- internal/exec/workflow_utils.go
- pkg/schema/task_validate.go
- pkg/schema/workflow.go
…e singleton The cleanup path zeroes globalImporterOnce/globalImporter/globalImporterErr, so the helper resets the singleton to its zero state rather than restoring prior state; stacked use is not safe. Update the comment to match (CodeRabbit nit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
Jun 29, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.222.0-rc.13. |
This was referenced Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
parallelandmatrixworkflow control steps with siblingneedsDAG scheduling.pkg/workflow.pkg/runner/step/parallelandpkg/runner/step/matrixhandlers, JSON schema updates, andexamples/parallel-steps.why
internal/exec.pkg/workflowcoverage above 80%.references
pkg/workflowcoverage: 82.9%go test ./pkg/schema ./pkg/runner/step ./pkg/scheduler ./pkg/workflow ./internal/execgo test ./cmd ./tests -run 'Workflow|workflow|Schema|schema'./custom-gcl run --new-from-rev=origin/main --config=.golangci.yml