Repository navigation
feat: Implement workflow step types with registry pattern (DEV-263, DEV-2969) - #1899
Andriy Knysh (aknysh) merged 60 commits into
Conversation
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1899 +/- ##
==========================================
+ Coverage 78.61% 78.69% +0.07%
==========================================
Files 1150 1184 +34
Lines 110981 113079 +2098
==========================================
+ Hits 87253 88984 +1731
- Misses 18884 19209 +325
- Partials 4844 4886 +42
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (7)
📝 WalkthroughWalkthroughAdds a registry-based step execution engine with a reusable executor, enhances workflow UI (header/flags, progress, command rendering), introduces pager viewport constraints, publishes PRDs/docs, adds new sentinel errors, and updates CLI snapshots to use an Explanation section. ChangesExtended Step Engine, Pager, and Workflow UI
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant StepRegistry
participant StepExecutor
participant Shell/Atmos
User->>CLI: run custom command
CLI->>StepExecutor: create/reuse executor
loop for each step
StepExecutor->>StepRegistry: resolve handler by type
alt shell/atmos
StepExecutor->>Shell/Atmos: execute command (mode/viewport)
else extended
StepExecutor->>StepRegistry: handler.Execute(...)
end
StepExecutor->>StepExecutor: store result/env
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (26)
pkg/schema/workflow.go (1)
15-53: WorkflowStep schema extension looks solid.Fields are well-organized by category with clear inline comments. The
Timeoutfield as astringis a reasonable choice for YAML/JSON configs (e.g., "30s", "5m").One note: consider documenting valid
Outputmode values ("viewport", "raw", "log", "none") in the comment on line 46, or referencing a constants file if one exists.pkg/workflow/step/executor_test.go (1)
224-242: ListTypes test verifies registry categories.Confirms all categories are present with sample type checks. Consider adding a count assertion to catch regressions if types are accidentally removed.
pkg/workflow/step/pager.go (1)
27-49: Execute implementation is well-structured.
- Content resolution with error handling.
- Conditional title resolution.
- Error messages include step name for traceability.
The
ctxparameter is unused. Ifpager.Rundoesn't support context cancellation, this is acceptable. Otherwise, passing it through would enable timeout/cancellation support.pkg/workflow/step/error.go (1)
26-38: Consider wrapping the ui.Error failure with step context.Line 33-35: if
ui.Errorfails, the error is returned without step name context. Other handlers (e.g., pager.go line 45) wrap errors with step name for traceability.🔎 Suggested improvement
- if err := ui.Error(content); err != nil { - return nil, err - } + if err := ui.Error(content); err != nil { + return nil, fmt.Errorf("step '%s': failed to display error: %w", step.Name, err) + }This would require adding
"fmt"to imports.pkg/workflow/step/info.go (1)
26-38: Same error wrapping consideration as ErrorHandler.Line 33-35: the
ui.Infoerror lacks step context. For consistency with pager.go and better debugging, consider wrapping.🔎 Suggested improvement
- if err := ui.Info(content); err != nil { - return nil, err - } + if err := ui.Info(content); err != nil { + return nil, fmt.Errorf("step '%s': failed to display info: %w", step.Name, err) + }This would require adding
"fmt"to imports.pkg/workflow/step/file.go (2)
53-53: Consider using static errors from errors/errors.go for error wrapping.Per the coding guidelines, errors should be wrapped using static errors defined in
errors/errors.gorather than dynamicfmt.Errorfstrings. For example, lines 53, 60, 90, 101, and 131 use dynamic error messages. Consider defining static errors likeErrFailedToResolvePath,ErrFailedToScanDirectory,ErrNoFilesFound, andErrFileSelectionFailedin the errors package and wrapping them with context.As per coding guidelines, all errors must be wrapped using static errors defined in errors/errors.go.
Also applies to: 60-60, 90-90, 101-101, 131-131
65-99: filepath.Walk error handling skips inaccessible files silently.Lines 65-99 use
filepath.Walkand returnnilwhen encountering access errors (line 67), which silently skips files the user can't access. While this prevents the walk from failing entirely, users might be confused if expected files don't appear. Consider logging skipped files or accumulating warnings to surface later.pkg/workflow/step/table.go (3)
51-56: Potential index out of bounds if Data is empty.Line 53 accesses
step.Data[0]without verifying thatlen(step.Data) > 0in the condition. While line 51 checkslen(step.Data) > 0, if Data passes validation as empty (when Content is present), this code path shouldn't be reached. Consider adding an explicit guard or comment clarifying the invariant.
83-83: TODO: Integration with existing table formatter.The TODO on line 83 notes plans to integrate with
pkg/list/format/table.gofor lipgloss styling. The current tab-separated approach is functional but basic.Would you like me to create an issue to track this integration work?
27-27: Use static errors for error wrapping.Lines 27 and 90 use dynamic error messages with
fmt.Errorf. Per coding guidelines, consider defining static errors inerrors/errors.go(e.g.,ErrTableDataRequired,ErrFailedToResolveTitle) and wrapping them with context.As per coding guidelines, all errors must be wrapped using static errors defined in errors/errors.go.
Also applies to: 90-90
pkg/workflow/step/confirm.go (1)
75-75: Use static error for confirmation failure.Line 75 uses
fmt.Errorfwith a dynamic message. Per coding guidelines, define a static error likeErrConfirmationFailedinerrors/errors.goand wrap it with the step name context.As per coding guidelines, all errors must be wrapped using static errors defined in errors/errors.go.
pkg/workflow/step/output_mode_test.go (1)
71-89: Concurrent write test assumes deterministic byte accumulation.The test on line 88 asserts an exact length of 40 bytes (10 goroutines × 4 bytes each). While this validates accumulation, concurrent writes may have interleaving or buffering behaviors that could affect the exact output. Consider whether this test should focus on the presence of all data rather than exact byte count, or add a comment documenting the assumption.
pkg/workflow/step/spin.go (1)
68-72: Consider addressing the TODO for shell parsing.
strings.Fieldswill break on quoted arguments (e.g.,echo "hello world"). The TODO is noted, but this could cause subtle bugs. A library likegithub.com/kballard/go-shellquoteorgithub.com/google/shlexwould handle this properly.pkg/workflow/step/join.go (1)
31-56: Consider making the separator configurable.Currently hardcoded to newline. A configurable separator (via a new field or reusing an existing one) would add flexibility. Not blocking—the current default is reasonable.
pkg/workflow/step/style.go (1)
3-9: Organize imports into three groups with blank lines.Per coding guidelines, imports should be in three groups separated by blank lines: 1) Go stdlib, 2) 3rd-party, 3) Atmos packages.
Suggested import organization
import ( "context" + "github.com/cloudposse/atmos/pkg/data" "github.com/cloudposse/atmos/pkg/schema" "github.com/cloudposse/atmos/pkg/ui/theme" )Based on coding guidelines requiring three-group import organization.
pkg/workflow/step/markdown.go (1)
3-8: Organize imports into three groups with blank lines.Imports should be separated into groups per coding guidelines: 1) Go stdlib, 2) 3rd-party, 3) Atmos packages.
Suggested import organization
import ( "context" + "github.com/cloudposse/atmos/pkg/schema" "github.com/cloudposse/atmos/pkg/ui" )Based on coding guidelines.
pkg/workflow/step/atmos.go (1)
3-11: Organize imports with blank lines between groups.Suggested organization
import ( "context" "fmt" "os" "os/exec" "strings" + "github.com/cloudposse/atmos/pkg/schema" )Based on coding guidelines.
pkg/workflow/step/warn.go (1)
3-8: Organize imports with blank lines between groups.Suggested organization
import ( "context" + "github.com/cloudposse/atmos/pkg/schema" "github.com/cloudposse/atmos/pkg/ui" )Based on coding guidelines.
pkg/workflow/step/filter.go (1)
3-14: Organize imports with blank lines between groups.Suggested organization
import ( "context" "errors" "fmt" + "github.com/charmbracelet/bubbles/key" "github.com/charmbracelet/huh" + errUtils "github.com/cloudposse/atmos/errors" uiutils "github.com/cloudposse/atmos/internal/tui/utils" "github.com/cloudposse/atmos/pkg/schema" )Based on coding guidelines.
pkg/workflow/step/input.go (1)
3-14: Organize imports with blank lines between groups.Suggested organization
import ( "context" "errors" "fmt" + "github.com/charmbracelet/bubbles/key" "github.com/charmbracelet/huh" + errUtils "github.com/cloudposse/atmos/errors" uiutils "github.com/cloudposse/atmos/internal/tui/utils" "github.com/cloudposse/atmos/pkg/schema" )Based on coding guidelines.
pkg/workflow/step/output_handlers_test.go (1)
3-10: Organize imports with blank lines between groups.Suggested organization
import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/cloudposse/atmos/pkg/schema" )Based on coding guidelines.
pkg/workflow/step/shell.go (1)
3-12: Organize imports with blank lines between groups.Suggested organization
import ( "context" "errors" "fmt" "os" "os/exec" "strings" + "github.com/cloudposse/atmos/pkg/schema" )Based on coding guidelines.
pkg/workflow/step/interactive_handlers_test.go (2)
90-91: Malformed nolint directive.The
nolintlintcomment on line 90 isn't a valid directive format. It should either be removed or properly formatted.🔎 Proposed fix
-// nolintlint: dupl - Similar test patterns for different handlers. // nolint: dupl func TestConfirmHandlerValidation(t *testing.T) {
203-204: Same malformed nolint directive here.Same issue as line 90 — the
nolintlintcomment isn't doing anything useful.🔎 Proposed fix
-// nolintlint: dupl - Similar test patterns for different handlers. // nolint: dupl func TestWriteHandlerValidation(t *testing.T) {pkg/workflow/step/registry.go (1)
42-46: Consider nil handler guard.If a nil handler is passed,
handler.GetName()will panic. Since this runs ininit(), a panic is acceptable for fail-fast, but an explicit check could provide a clearer error message.🔎 Optional defensive check
func Register(handler StepHandler) { + if handler == nil { + panic("cannot register nil step handler") + } registry.mu.Lock() defer registry.mu.Unlock() registry.handlers[handler.GetName()] = handler }pkg/workflow/step/handler_base.go (1)
73-107: Unused context parameter.The
ctxparameter is passed but unused in all threeResolve*methods. This is fine for forward compatibility (future cancellation support), but you could add a_ = ctxor brief comment to silence linters if they flag it.🔎 Optional: silence unused parameter warnings
func (h BaseHandler) ResolveContent(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (string, error) { + _ = ctx // Reserved for future cancellation support. if step.Content == "" { return "", nil }
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
pkg/workflow/step/atmos.go (3)
30-42: Add performance tracking to public Execute method.Missing the required
defer perf.Track()call per coding guidelines.Add performance tracking
func (h *AtmosHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { + defer perf.Track(nil, "step.AtmosHandler.Execute")() + opts, err := h.prepareExecution(ctx, step, vars)Based on coding guidelines.
165-176: Add performance tracking to public ExecuteWithWorkflow method.Missing the required
defer perf.Track()call per coding guidelines.Add performance tracking
func (h *AtmosHandler) ExecuteWithWorkflow(ctx context.Context, step *schema.WorkflowStep, vars *Variables, workflow *schema.WorkflowDefinition) (*StepResult, error) { + defer perf.Track(nil, "step.AtmosHandler.ExecuteWithWorkflow")() + opts, err := h.prepareExecution(ctx, step, vars)Based on coding guidelines.
179-194: Remove unreachable dead code.Lines 189-191 are unreachable because line 181 already handles
arg == "-s"and returns true. The final check for a following value is never reached.Remove dead code
func containsStackFlag(args []string) bool { for i, arg := range args { if arg == "-s" || arg == "--stack" { return true } // Check for -s=value or --stack=value. if strings.HasPrefix(arg, "-s=") || strings.HasPrefix(arg, "--stack=") { return true } - // Check if next arg would be stack value for -s. - if arg == "-s" && i+1 < len(args) { - return true - } } return false }pkg/workflow/step/filter.go (1)
42-68: Add performance tracking to public Execute method.Missing the required
defer perf.Track()call per coding guidelines.Add performance tracking
func (h *FilterHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { + defer perf.Track(nil, "step.FilterHandler.Execute")() + if err := h.CheckTTY(step); err != nil {Based on coding guidelines.
pkg/workflow/step/choose.go (1)
100-109: Default selection not applied.The call to
opt.Selected(true)returns a new option with the selected state but doesn't mutate the existing option. Since the returned value isn't assigned back, the default selection is lost.Proposed fix
func (h *ChooseHandler) runSelectForm(stepName, prompt string, options []string, defaultVal string) (*StepResult, error) { var choice string - huhOptions := huh.NewOptions(options...) - for _, opt := range huhOptions { + huhOptions := make([]huh.Option[string], len(options)) + for i, optVal := range options { + opt := huh.NewOption(optVal, optVal) - if opt.Value == defaultVal { - opt.Selected(true) - break + if optVal == defaultVal { + opt = opt.Selected(true) } + huhOptions[i] = opt }pkg/workflow/step/executor.go (1)
103-120: RunAll still uses dynamic error for step failure.Line 115 creates
fmt.Errorf("step '%s' failed: %w", ...). Per past review and coding guidelines, this should wrap a static error likeErrWorkflowStepFailed.Recommended fix
- return fmt.Errorf("step '%s' failed: %w", step.Name, err) + return errUtils.Build(errUtils.ErrWorkflowStepFailed). + WithContext("step", step.Name). + WithCause(err). + Err()This assumes
ErrWorkflowStepFailedexists in errors/errors.go (per learnings, it should).
🧹 Nitpick comments (8)
examples/demo-interactive-workflows/README.md (1)
145-145: Add closing punctuation.The final line should end with a period for consistency with documentation standards.
🔎 Proposed fix
-- Use environment variables instead of prompts +- Use environment variables instead of prompts.pkg/workflow/step/log.go (1)
64-71: Consider logging template resolution failures.When
vars.Resolve(value)fails, the error is silently discarded and the original value is used. While this provides a fallback, it could mask configuration issues where template variables are misspelled or undefined.Consider logging the error at debug level so issues are discoverable:
🔎 Proposed enhancement
for key, value := range step.Fields { // Resolve template variables in field values. resolvedValue, err := vars.Resolve(value) if err != nil { - // On error, use the original value. + // On error, use the original value but log for debugging. + log.Debug("Failed to resolve template in log field", "key", key, "value", value, "error", err.Error()) resolvedValue = value } keyvals = append(keyvals, key, resolvedValue) }pkg/workflow/step/linebreak.go (1)
29-41: Add performance tracking to public Execute method.The Execute method is missing the required
defer perf.Track()call per coding guidelines.Add performance tracking
func (h *LinebreakHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { + defer perf.Track(nil, "step.LinebreakHandler.Execute")() + count := step.CountBased on coding guidelines.
pkg/workflow/step/choose.go (1)
42-63: Add performance tracking to public Execute method.Missing the required
defer perf.Track()call per coding guidelines.Add performance tracking
func (h *ChooseHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { + defer perf.Track(nil, "step.ChooseHandler.Execute")() + if err := h.CheckTTY(step); err != nil {Based on coding guidelines.
pkg/workflow/step/pager.go (1)
34-70: Add performance tracking to public Execute method.Missing the required
defer perf.Track()call per coding guidelines.Add performance tracking
func (h *PagerHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { + defer perf.Track(nil, "step.PagerHandler.Execute")() + var content stringBased on coding guidelines.
pkg/workflow/step/file.go (1)
36-73: Add performance tracking to public Execute method.Missing the required
defer perf.Track()call per coding guidelines.Add performance tracking
func (h *FileHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { + defer perf.Track(nil, "step.FileHandler.Execute")() + if err := h.CheckTTY(step); err != nil {Based on coding guidelines.
pkg/workflow/step/handler_base.go (1)
76-110: Unused context parameter in Resolve methods.The
ctxparameter is unused inResolveContent,ResolvePrompt, andResolveCommand. If it's for future cancellation support or interface consistency, consider adding a brief comment. Otherwise, these could be simplified.Option: Document intent or remove unused param
If ctx is for future use:
// ResolveContent resolves Go templates in the content field. // The context is included for future cancellation support. func (h BaseHandler) ResolveContent(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (string, error) {Or if not needed, remove it from the signature (would require updating callers).
pkg/workflow/step/output_mode.go (1)
222-236: StreamingOutputWriter silently ignores write errors.Line 232 discards the error from
fmt.Fprintf. If the target is unavailable, this fails silently. Consider at minimum logging a warning or returning the error.Option: Handle or log write errors
// Write with prefix to target. if w.target != nil { - _, _ = fmt.Fprintf(w.target, "%s %s", w.prefix, string(p)) + if _, writeErr := fmt.Fprintf(w.target, "%s %s", w.prefix, string(p)); writeErr != nil { + // Log at debug level or accumulate for later inspection. + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (34)
errors/errors.go(1 hunks)examples/demo-interactive-workflows/README.md(1 hunks)examples/demo-interactive-workflows/atmos.yaml(1 hunks)examples/demo-interactive-workflows/stacks/deploy/dev.yaml(1 hunks)examples/demo-interactive-workflows/stacks/deploy/prod.yaml(1 hunks)examples/demo-interactive-workflows/stacks/deploy/staging.yaml(1 hunks)examples/demo-interactive-workflows/stacks/workflows/interactive.yaml(1 hunks)internal/exec/workflow_utils.go(5 hunks)pkg/schema/workflow.go(1 hunks)pkg/ui/markdown/styles.go(1 hunks)pkg/ui/theme/converter.go(1 hunks)pkg/ui/theme/styles.go(3 hunks)pkg/workflow/step/atmos.go(1 hunks)pkg/workflow/step/choose.go(1 hunks)pkg/workflow/step/executor.go(1 hunks)pkg/workflow/step/executor_test.go(1 hunks)pkg/workflow/step/file.go(1 hunks)pkg/workflow/step/filter.go(1 hunks)pkg/workflow/step/handler_base.go(1 hunks)pkg/workflow/step/interactive_handlers_test.go(1 hunks)pkg/workflow/step/join.go(1 hunks)pkg/workflow/step/linebreak.go(1 hunks)pkg/workflow/step/log.go(1 hunks)pkg/workflow/step/output_handlers_test.go(1 hunks)pkg/workflow/step/output_mode.go(1 hunks)pkg/workflow/step/pager.go(1 hunks)pkg/workflow/step/shell.go(1 hunks)pkg/workflow/step/spin.go(1 hunks)pkg/workflow/step/style.go(1 hunks)pkg/workflow/step/table.go(1 hunks)pkg/workflow/step/variables.go(1 hunks)pkg/workflow/step/variables_test.go(1 hunks)website/docs/cli/commands/workflow.mdx(1 hunks)website/docs/workflows/workflows.mdx(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- examples/demo-interactive-workflows/atmos.yaml
- examples/demo-interactive-workflows/stacks/deploy/staging.yaml
- website/docs/workflows/workflows.mdx
- examples/demo-interactive-workflows/stacks/deploy/dev.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/workflow/step/variables_test.go
- pkg/workflow/step/join.go
- pkg/workflow/step/interactive_handlers_test.go
- pkg/workflow/step/shell.go
- pkg/workflow/step/variables.go
- pkg/workflow/step/table.go
- pkg/workflow/step/style.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
pkg/ui/theme/converter.gopkg/ui/markdown/styles.gopkg/workflow/step/spin.gopkg/ui/theme/styles.gopkg/workflow/step/executor_test.gopkg/workflow/step/choose.gopkg/workflow/step/linebreak.goerrors/errors.gopkg/workflow/step/pager.gopkg/workflow/step/file.gopkg/workflow/step/filter.gointernal/exec/workflow_utils.gopkg/workflow/step/atmos.gopkg/workflow/step/log.gopkg/workflow/step/output_mode.gopkg/workflow/step/handler_base.gopkg/workflow/step/output_handlers_test.gopkg/workflow/step/executor.gopkg/schema/workflow.go
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in thewebsite/directory when adding new features, ensure consistency between CLI help text and website documentation, and follow the website's documentation structure and style
Keep website code in thewebsite/directory, follow the existing website architecture and style, and test website changes locally before committing
Keep CLI documentation and website documentation in sync and document new features on the website with examples and use cases
Files:
website/docs/cli/commands/workflow.mdx
website/docs/cli/commands/**/*.mdx
📄 CodeRabbit inference engine (CLAUDE.md)
All CLI commands/flags need Docusaurus documentation in website/docs/cli/commands/ with specific structure: frontmatter, Intro component, Screengrab component, Usage section, Arguments/Flags using
- /
- , and Examples section
Files:
website/docs/cli/commands/workflow.mdx
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
pkg/workflow/step/executor_test.gopkg/workflow/step/output_handlers_test.go
🧠 Learnings (56)
📓 Common learnings
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
examples/demo-interactive-workflows/stacks/workflows/interactive.yamlpkg/workflow/step/executor_test.gopkg/workflow/step/choose.gopkg/workflow/step/linebreak.goerrors/errors.goexamples/demo-interactive-workflows/README.mdinternal/exec/workflow_utils.gopkg/workflow/step/atmos.gopkg/workflow/step/log.gopkg/workflow/step/output_mode.gopkg/workflow/step/handler_base.gopkg/workflow/step/output_handlers_test.gopkg/workflow/step/executor.gopkg/schema/workflow.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
examples/demo-interactive-workflows/stacks/workflows/interactive.yamlpkg/workflow/step/spin.gopkg/workflow/step/executor_test.goerrors/errors.goexamples/demo-interactive-workflows/README.mdinternal/exec/workflow_utils.gopkg/workflow/step/atmos.gopkg/workflow/step/output_mode.gopkg/workflow/step/handler_base.gopkg/workflow/step/executor.gopkg/schema/workflow.go
📚 Learning: 2025-10-11T19:12:38.832Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden:0-0
Timestamp: 2025-10-11T19:12:38.832Z
Learning: Usage Examples sections in error output are appropriate for command usage errors (incorrect syntax, missing arguments, invalid flags) but not for configuration validation errors (malformed workflow files, invalid settings in atmos.yaml). Configuration errors should focus on explaining what's wrong with the config, not command usage patterns.
Applied to files:
website/docs/cli/commands/workflow.mdxexamples/demo-interactive-workflows/README.md
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
pkg/workflow/step/spin.go
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
pkg/workflow/step/spin.gopkg/workflow/step/atmos.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
pkg/workflow/step/spin.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
pkg/workflow/step/spin.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
pkg/workflow/step/spin.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
pkg/workflow/step/spin.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Use colors from pkg/ui/theme/colors.go for all UI output
Applied to files:
pkg/ui/theme/styles.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
pkg/ui/theme/styles.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/ui/theme/styles.gopkg/workflow/step/atmos.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/ui/theme/styles.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/workflow/step/executor_test.gopkg/workflow/step/output_handlers_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/workflow/step/executor_test.gopkg/workflow/step/output_handlers_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/workflow/step/executor_test.gopkg/workflow/step/output_handlers_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
pkg/workflow/step/executor_test.gopkg/workflow/step/output_handlers_test.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
errors/errors.gopkg/workflow/step/executor.go
📚 Learning: 2024-12-12T17:13:53.409Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 848
File: pkg/utils/doc_utils.go:19-22
Timestamp: 2024-12-12T17:13:53.409Z
Learning: In `pkg/utils/doc_utils.go`, the `DisplayDocs` function uses the `PAGER` environment variable, which is intentionally user-configurable to allow users to specify custom pager commands that fit their workflow; adding validation to restrict it is not desired.
Applied to files:
pkg/workflow/step/pager.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to README.md : Update README.md with new commands and features
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2025-11-07T14:52:55.217Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1761
File: docs/prd/claude-agent-architecture.md:331-439
Timestamp: 2025-11-07T14:52:55.217Z
Learning: In the cloudposse/atmos repository, Claude agents are used as interactive tools, not in automated/headless CI/CD contexts. Agent documentation and patterns should assume synchronous human interaction.
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2024-11-12T13:06:56.194Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: website/docs/cheatsheets/vendoring.mdx:70-70
Timestamp: 2024-11-12T13:06:56.194Z
Learning: In `atmos vendor pull --everything`, the `--everything` flag uses the TTY for TUI but is not interactive.
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2025-11-30T04:16:24.155Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:34-48
Timestamp: 2025-11-30T04:16:24.155Z
Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.
Applied to files:
pkg/workflow/step/filter.gopkg/workflow/step/atmos.go
📚 Learning: 2025-11-30T04:16:01.899Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:50-59
Timestamp: 2025-11-30T04:16:01.899Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` should NOT be added to trivial O(1) getter methods that only return field references or check map lengths (e.g., `GetDeferredValues()`, `HasDeferredValues()`). The guideline to add perf tracking to "all public functions" applies to functions that do meaningful work (I/O, computation, external calls), not to trivial accessors where the tracking overhead would exceed the operation time and pollute performance reports.
Applied to files:
pkg/workflow/step/filter.gopkg/workflow/step/atmos.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions for performance tracking; use nil if no atmosConfig param
Applied to files:
pkg/workflow/step/filter.gopkg/workflow/step/atmos.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
pkg/workflow/step/filter.gopkg/workflow/step/atmos.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
internal/exec/workflow_utils.gopkg/workflow/step/executor.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
internal/exec/workflow_utils.gopkg/workflow/step/atmos.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-12-05T22:33:40.955Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 820
File: cmd/list_components.go:53-54
Timestamp: 2024-12-05T22:33:40.955Z
Learning: In the Atmos CLI Go codebase, using `u.LogErrorAndExit` within completion functions is acceptable because it logs the error and exits the command execution.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-02-03T06:00:11.419Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/describe_config.go:20-20
Timestamp: 2025-02-03T06:00:11.419Z
Learning: Commands should use `PrintErrorMarkdownAndExit` with empty title and suggestion (`"", err, ""`) for general error handling. Specific titles like "Invalid Usage" or "File Not Found" should only be used for validation or specific error scenarios.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
internal/exec/workflow_utils.gopkg/workflow/step/executor.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/workflow/step/atmos.gopkg/workflow/step/executor.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2025-10-13T18:13:54.020Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1622
File: pkg/perf/perf.go:140-184
Timestamp: 2025-10-13T18:13:54.020Z
Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2025-05-22T15:42:10.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1261
File: internal/exec/utils.go:639-640
Timestamp: 2025-05-22T15:42:10.906Z
Learning: In the Atmos codebase, when appending slices with `args := append(configAndStacksInfo.CliArgs, configAndStacksInfo.AdditionalArgsAndFlags...)`, it's intentional that the result is not stored back in the original slice. This pattern is used when the merged result serves a different purpose than the original slices, such as when creating a filtered version for component section assignments.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2025-02-09T14:38:53.443Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 992
File: cmd/cmd_utils.go:0-0
Timestamp: 2025-02-09T14:38:53.443Z
Learning: Error handling for RegisterFlagCompletionFunc in AddStackCompletion is not required as the errors are non-critical for tab completion functionality.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
pkg/workflow/step/atmos.gopkg/workflow/step/executor.go
📚 Learning: 2025-01-09T22:27:25.538Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/validate_stacks.go:20-23
Timestamp: 2025-01-09T22:27:25.538Z
Learning: The validate commands in Atmos can have different help handling implementations. Specifically, validate_component.go and validate_stacks.go are designed to handle help requests differently, with validate_stacks.go including positional argument checks while validate_component.go does not.
Applied to files:
pkg/workflow/step/atmos.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
pkg/workflow/step/log.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Separate I/O (streams) from UI (formatting): use I/O Layer (pkg/io/) for stream access and UI Layer (pkg/ui/) for formatting; use data.Write/Writeln/WriteJSON/WriteYAML for pipeable output to stdout and ui.Write/Success/Error/Warning/Info for human messages to stderr
Applied to files:
pkg/workflow/step/output_mode.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Applied to files:
pkg/workflow/step/executor.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
pkg/workflow/step/executor.go
📚 Learning: 2024-10-12T18:38:28.458Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 715
File: pkg/logger/logger.go:21-32
Timestamp: 2024-10-12T18:38:28.458Z
Learning: In Go code, avoid prefixing struct names with 'I' (e.g., `IError`) as it can be confusing, suggesting an interface. Use descriptive names for structs to improve code clarity.
Applied to files:
pkg/workflow/step/executor.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
pkg/workflow/step/executor.go
🧬 Code graph analysis (10)
pkg/workflow/step/choose.go (4)
pkg/workflow/step/handler_base.go (2)
BaseHandler(13-17)NewBaseHandler(20-26)pkg/workflow/step/registry.go (1)
Register(42-46)pkg/schema/workflow.go (1)
WorkflowStep(16-75)errors/errors.go (2)
ErrStepOptionsRequired(448-448)ErrUserAborted(555-555)
pkg/workflow/step/linebreak.go (4)
pkg/workflow/step/handler_base.go (2)
BaseHandler(13-17)NewBaseHandler(20-26)pkg/workflow/step/types.go (2)
CategoryUI(12-12)NewStepResult(46-51)pkg/schema/workflow.go (1)
WorkflowStep(16-75)pkg/workflow/step/variables.go (1)
Variables(12-17)
pkg/workflow/step/file.go (5)
pkg/workflow/step/registry.go (1)
Register(42-46)pkg/workflow/step/types.go (3)
CategoryInteractive(8-8)StepResult(32-43)NewStepResult(46-51)pkg/schema/workflow.go (1)
WorkflowStep(16-75)pkg/workflow/step/variables.go (1)
Variables(12-17)errors/errors.go (2)
ErrStepNoFilesFound(452-452)ErrUserAborted(555-555)
internal/exec/workflow_utils.go (4)
pkg/workflow/step/executor.go (3)
IsExtendedStepType(134-149)StepExecutor(13-16)NewStepExecutor(19-23)errors/errors.go (1)
ErrInvalidWorkflowStepType(441-441)errors/exit_code.go (1)
WithExitCode(40-48)pkg/schema/workflow.go (2)
WorkflowStep(16-75)WorkflowDefinition(78-88)
pkg/workflow/step/atmos.go (4)
pkg/workflow/step/handler_base.go (2)
BaseHandler(13-17)NewBaseHandler(20-26)pkg/workflow/step/registry.go (1)
Register(42-46)pkg/workflow/step/variables.go (1)
Variables(12-17)pkg/workflow/step/output_mode.go (2)
GetOutputMode(175-188)GetViewportConfig(191-203)
pkg/workflow/step/log.go (5)
pkg/workflow/step/handler_base.go (2)
BaseHandler(13-17)NewBaseHandler(20-26)pkg/workflow/step/types.go (2)
CategoryOutput(10-10)NewStepResult(46-51)pkg/schema/workflow.go (1)
WorkflowStep(16-75)pkg/workflow/step/variables.go (1)
Variables(12-17)pkg/logger/log.go (9)
Level(203-203)TraceLevel(211-211)Trace(14-16)DebugLevel(213-213)Debug(24-26)WarnLevel(217-217)Warn(44-46)ErrorLevel(219-219)InfoLevel(215-215)
pkg/workflow/step/handler_base.go (5)
pkg/workflow/step/types.go (1)
StepCategory(4-4)pkg/schema/workflow.go (1)
WorkflowStep(16-75)errors/builder.go (1)
Build(24-37)errors/errors.go (2)
ErrStepTTYRequired(454-454)ErrStepFieldRequired(453-453)pkg/workflow/step/variables.go (1)
Variables(12-17)
pkg/workflow/step/output_handlers_test.go (3)
pkg/workflow/step/types.go (2)
StepCategory(4-4)CategoryOutput(10-10)pkg/workflow/step/registry.go (2)
Get(49-54)ListByCategory(68-77)pkg/schema/workflow.go (1)
WorkflowStep(16-75)
pkg/workflow/step/executor.go (5)
pkg/workflow/step/variables.go (2)
Variables(12-17)NewVariables(20-28)pkg/schema/workflow.go (2)
WorkflowDefinition(78-88)WorkflowStep(16-75)pkg/workflow/step/types.go (2)
StepResult(32-43)StepCategory(4-4)pkg/workflow/step/registry.go (3)
Get(49-54)StepHandler(13-28)ListByCategory(68-77)errors/errors.go (1)
ErrUnknownStepType(447-447)
pkg/schema/workflow.go (1)
pkg/schema/schema.go (1)
RetryConfig(784-792)
🪛 LanguageTool
examples/demo-interactive-workflows/README.md
[typographical] ~118-~118: To join two clauses or introduce examples, consider using an em dash.
Context: ...Variables - {{ .steps.<name>.value }} - Primary value from step - `{{ .steps.<na...
(DASH_RULE)
[grammar] ~145-~145: Please add a punctuation mark at the end of paragraph.
Context: ... - Use environment variables instead of prompts
(PUNCTUATION_PARAGRAPH_END)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/workflow/step/toast.go (1)
27-52: Solid implementation with clear level handling.The case-insensitive level matching and sensible Info default provide good UX. The explicit handling of "warn" as an alias for "warning" is thoughtful.
Optional: Make empty string handling explicit
For clarity, you could explicitly handle the empty string case:
// Display based on level (default to info). switch strings.ToLower(step.Level) { case "success": err = ui.Success(content) case "warning", "warn": err = ui.Warning(content) case "error": err = ui.Error(content) - default: - // Default to info for "", "info", or any other value. + case "", "info": err = ui.Info(content) + default: + // Treat unknown levels as info. + err = ui.Info(content) }This makes the intent clearer that both empty and "info" are treated identically.
examples/demo-interactive-workflows/README.md (1)
125-145: Good coverage of output modes and CI considerations.The output mode examples and CI guidance are practical.
Optional: Add missing period for consistency
Line 145 is missing a closing period:
- Use `--dry-run` to preview workflows - Set default values in configuration -- Use environment variables instead of prompts +- Use environment variables instead of prompts.pkg/workflow/step/executor_test.go (1)
33-41: Consider testing SetWorkflow behavior.The test only verifies no panic occurs when calling
SetWorkflow. Consider either testing that the workflow is actually stored/accessible or removing this test case if there's genuinely no observable behavior to verify.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
examples/demo-interactive-workflows/README.md(1 hunks)examples/demo-interactive-workflows/stacks/workflows/interactive.yaml(1 hunks)pkg/workflow/step/executor_test.go(1 hunks)pkg/workflow/step/pager.go(1 hunks)pkg/workflow/step/toast.go(1 hunks)pkg/workflow/step/ui_handlers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/workflow/step/ui_handlers_test.go
- examples/demo-interactive-workflows/stacks/workflows/interactive.yaml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: NEVER use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println(); use data.* or ui.* functions instead
All comments must end with periods (enforced by godot linter)
Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages; maintain aliases: cfg, log, u, errUtils
Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions for performance tracking; use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go; use errors.Join for combining multiple errors; use fmt.Errorf with %w for adding string context; use error builder for complex errors; use errors.Is() for error checking; NEVER use dynamic errors directly
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation; never create manual mocks
Keep files small...
Files:
pkg/workflow/step/executor_test.gopkg/workflow/step/toast.gopkg/workflow/step/pager.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + dependency injection for testability; generate mocks with go.uber.org/mock/mockgen; use table-driven tests; target >80% coverage
Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Files:
pkg/workflow/step/executor_test.go
🧠 Learnings (13)
📓 Common learnings
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
pkg/workflow/step/executor_test.goexamples/demo-interactive-workflows/README.md
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/workflow/step/executor_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/workflow/step/executor_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/workflow/step/executor_test.go
📚 Learning: 2025-12-16T18:20:55.630Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T18:20:55.630Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking
Applied to files:
pkg/workflow/step/executor_test.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
pkg/workflow/step/executor_test.gopkg/workflow/step/pager.goexamples/demo-interactive-workflows/README.md
📚 Learning: 2024-12-12T17:13:53.409Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 848
File: pkg/utils/doc_utils.go:19-22
Timestamp: 2024-12-12T17:13:53.409Z
Learning: In `pkg/utils/doc_utils.go`, the `DisplayDocs` function uses the `PAGER` environment variable, which is intentionally user-configurable to allow users to specify custom pager commands that fit their workflow; adding validation to restrict it is not desired.
Applied to files:
pkg/workflow/step/pager.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to README.md : Update README.md with new commands and features
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2025-11-07T14:52:55.217Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1761
File: docs/prd/claude-agent-architecture.md:331-439
Timestamp: 2025-11-07T14:52:55.217Z
Learning: In the cloudposse/atmos repository, Claude agents are used as interactive tools, not in automated/headless CI/CD contexts. Agent documentation and patterns should assume synchronous human interaction.
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2024-11-12T13:06:56.194Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: website/docs/cheatsheets/vendoring.mdx:70-70
Timestamp: 2024-11-12T13:06:56.194Z
Learning: In `atmos vendor pull --everything`, the `--everything` flag uses the TTY for TUI but is not interactive.
Applied to files:
examples/demo-interactive-workflows/README.md
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
examples/demo-interactive-workflows/README.md
🧬 Code graph analysis (3)
pkg/workflow/step/executor_test.go (5)
pkg/workflow/step/executor.go (6)
NewStepExecutor(19-23)NewStepExecutorWithVars(26-30)IsExtendedStepType(134-149)ValidateStep(152-167)ValidateWorkflow(186-204)ListTypes(170-183)pkg/workflow/step/variables.go (2)
Variables(12-17)NewVariables(20-28)pkg/workflow/step/types.go (1)
NewStepResult(46-51)pkg/schema/workflow.go (2)
WorkflowDefinition(78-88)WorkflowStep(16-75)pkg/workflow/step/registry.go (1)
Get(49-54)
pkg/workflow/step/toast.go (5)
pkg/workflow/step/handler_base.go (2)
BaseHandler(13-17)NewBaseHandler(20-26)pkg/workflow/step/types.go (1)
CategoryUI(12-12)pkg/schema/workflow.go (1)
WorkflowStep(16-75)pkg/workflow/step/variables.go (1)
Variables(12-17)pkg/ui/formatter.go (2)
Success(193-200)Warning(237-244)
pkg/workflow/step/pager.go (6)
pkg/workflow/step/registry.go (1)
Register(42-46)pkg/workflow/step/types.go (2)
StepResult(32-43)NewStepResult(46-51)pkg/schema/workflow.go (1)
WorkflowStep(16-75)pkg/pager/pager.go (1)
NewWithAtmosConfig(27-31)pkg/ui/theme/converter.go (1)
GetGlamourStyleForTheme(255-263)pkg/ui/theme/styles.go (1)
DefaultThemeName(11-11)
🪛 LanguageTool
examples/demo-interactive-workflows/README.md
[typographical] ~118-~118: To join two clauses or introduce examples, consider using an em dash.
Context: ...Variables - {{ .steps.<name>.value }} - Primary value from step - `{{ .steps.<na...
(DASH_RULE)
[grammar] ~145-~145: Please add a punctuation mark at the end of paragraph.
Context: ... - Use environment variables instead of prompts
(PUNCTUATION_PARAGRAPH_END)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (15)
pkg/workflow/step/toast.go (2)
11-20: LGTM—clean handler registration.The ToastHandler correctly registers as
CategoryUIwithrequiresTTY=false, making toast notifications available in both interactive and non-interactive contexts. The pattern aligns well with other UI step handlers.
22-25: Validation is appropriate.Correctly enforces the
contentrequirement using the base validation helper.pkg/workflow/step/pager.go (5)
17-26: Proper registration as output category.Correctly positioned under
CategoryOutputwith appropriate TTY requirements for a pager step.
28-35: Flexible validation for content sources.Correctly allows either inline
contentor filepath, covering both use cases.
37-63: Clean execution flow with graceful degradation.The pipeline (load → render → display) is well-structured. Silently falling back to unrendered content when markdown rendering fails (line 46) provides good UX—the pager still works even if styling fails.
65-83: Well-structured content loading.Cleanly handles both inline and file-based content sources. Returning the resolved path enables accurate markdown detection downstream.
101-140: Helper methods are well-implemented.The markdown detection logic (explicit flag → extension auto-detect) provides good flexibility. Theme-aware rendering integrates nicely with Atmos styling. File reading has appropriate error context.
examples/demo-interactive-workflows/README.md (3)
1-23: Strong introduction with clear prerequisites.The TTY requirement callout is helpful for users. Quick start commands provide immediate value.
25-80: Excellent workflow documentation.The progression from simple (
deploy) to complex (full-deploy) helps users learn incrementally. Each workflow's purpose is immediately clear.
81-124: Comprehensive reference for step types and variables.The step types table provides quick lookup. The variable passing examples with Go templates are clear and practical. The available template variables list serves as a good reference.
pkg/workflow/step/executor_test.go (5)
89-125: Excellent table-driven test coverage.Comprehensive coverage of legacy vs extended step types with clear test cases. Good use of table-driven pattern per coding guidelines.
172-219: Good workflow validation coverage.The test cases cover validation scenarios comprehensively, including valid workflows, invalid steps, empty workflows, and generated step names. The approach of checking error count is appropriate for ValidateWorkflow's []error return type.
221-239: LGTM.Test provides good coverage of the ListTypes functionality, verifying all categories and spot-checking expected types in each.
243-315: Strong integration test coverage for variable passing.These tests effectively verify the variable passing mechanism between steps, covering value access, metadata, environment variables, multi-value scenarios, and state flags. The approach of simulating results is appropriate for testing variable resolution independently of handler execution.
330-346: LGTM.Clean test of the fluent API pattern, verifying all chainable methods work correctly.
083126c to
6cd0539
Compare
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
mergify
Bot
added
conflict
…de claims Add docs/prd/custom-command-builtin-override.md specifying how a step-based custom command can override a user-facing built-in (opt-in `override: true`) and call the native built-in via an `invoke: built-in` step modifier on `type: atmos` steps — cycle-proof by invoking the retained handler in-process (no ATMOS_REEXEC_DEPTH counter). Correct command-registry-pattern.md (Scenario 1 / Why This Works / FAQ) to reflect shipped behavior since #2191 (built-in preserved, custom steps ignored on collision) and document `invoke:` on the atmos step in workflow-step-types.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/prd/command-registry-pattern.md`:
- Around line 716-717: Update the "Nested custom commands → always added as
subcommands to parent" bullet to explicitly document collision behavior: state
that when a nested custom command name collides with an existing command the
registry reuses the existing command (does not create a new subcommand) and will
ignore/skip any custom `steps` from the colliding definition; reference this
collision behavior in the key behaviors section and add a short note advising
contributors to use unique names or explicitly override the existing command
through the registry API if they intend to replace steps.
In `@docs/prd/workflow-step-types.md`:
- Around line 1012-1014: The WorkflowStep schema block is missing the new invoke
field; update the WorkflowStep schema (the JSON/YAML schema object named
WorkflowStep) to include an "invoke" property (type: string, enum:
["exec","built-in"], default: "exec") with a short description matching the docs
text about exec vs built-in and note the default; also update any example
WorkflowStep definitions in the same doc to show the new invoke key so the
schema and examples remain consistent with the introduced invoke behavior.
🪄 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: 36d72d73-afa2-45a2-b04a-6496fcb0500e
📒 Files selected for processing (3)
docs/prd/command-registry-pattern.mddocs/prd/custom-command-builtin-override.mddocs/prd/workflow-step-types.md
…te tests Address CodeRabbit feedback on PR #1899: - command-registry-pattern.md: split the nested custom-command bullet to call out the name-collision case (findSubcommand reuses the existing command and ignores custom steps with a warning at every level, not just top-level). - workflow-step-types.md: add the `invoke` field to the WorkflowStep schema block for consistency with the atmos step reference. Increase pkg/runner/step coverage 78.9% -> 82.3% with hang-safe Execute tests: - spin: full Execute orchestration (non-TTY runs the command directly). - input/confirm/choose/write/filter/file: fail-fast Execute contract. - pager: Execute error paths before the pager launches. - atmos: Execute resolution errors before re-exec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add unit tests for the new workflow step-type subsystem to clear the Codecov patch target (was 73.94%, target 80%). - pkg/workflow/progress_test.go: inject a fake terminal.Terminal to cover formatProgressLine, renderers, Done/IsEnabled, and nil/disabled guards (8.6% -> ~100%). - pkg/workflow/show_renderer_test.go: cover formatHeader, formatFlags (sort + styled/nil branches), and RenderHeaderIfNeeded paths (17% -> 100%). - pkg/runner/step/interactive_execute_test.go: assert each interactive Execute returns ErrStepTTYRequired under go test (no TTY), covering the CheckTTY guard for confirm/write/input/choose/filter/file. - pkg/runner/step/testmain_test.go + atmos_test.go: a sentinel-gated TestMain lets the test binary impersonate atmos so Execute, runAtmosCommand, and ExecuteWithWorkflow are tested cross-platform (0% -> ~85%). Package coverage: pkg/workflow 76.5% -> 96.9%, pkg/runner/step 78.9% -> 82.3%. No production code changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o-atmos-workflows' into feature/dev-263-add-input-type-to-atmos-workflows
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/runner/step/interactive_execute_test.go`:
- Around line 27-39: TestInteractiveHandlers_ExecuteWithoutTTY can hang when
running from an interactive terminal; add a pre-check at the start of that test
(before calling handler.Execute or asserting errUtils.ErrStepTTYRequired) to
detect if both terminal.IsTTY(terminal.Stdin) and
terminal.IsTTY(terminal.Stdout) are true and skip the test in that case (use
t.Skip or equivalent), so the test exercises BaseHandler.CheckTTY and
handler.Execute safely without blocking on the interactive prompt.
In `@pkg/workflow/progress_test.go`:
- Line 16: Several multi-line comment lines in progress_test.go are missing
terminal periods; update each multi-line comment (e.g., the line "// Only Width
returns a configurable value; the remaining methods are no-ops" and the comments
at the other flagged locations) so every line ends with a period. Locate the
comment lines around the test functions in progress_test.go (including the lines
referenced at 32, 114, 172, 186) and append a period to the end of each comment
line so they comply with the godot linter.
🪄 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: 1522650b-65d3-4570-b0a5-d8219327a4e0
📒 Files selected for processing (18)
docs/prd/command-registry-pattern.mddocs/prd/workflow-step-types.mdpkg/runner/step/atmos_test.gopkg/runner/step/interactive_execute_test.gopkg/runner/step/interactive_handlers_test.gopkg/runner/step/pager_test.gopkg/runner/step/spin_test.gopkg/runner/step/testmain_test.gopkg/workflow/progress_test.gopkg/workflow/show_renderer_test.gotests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_failure_with_stack.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_shell_command_not_found.stderr.goldentests/snapshots/TestCLICommands_atmos_workflow_shell_pass.stderr.goldentests/snapshots/TestCLICommands_workflow_retries_example.stderr.golden
✅ Files skipped from review due to trivial changes (8)
- tests/snapshots/TestCLICommands_workflow_retries_example.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_failure_with_stack.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_shell_command_not_found.stderr.golden
- docs/prd/command-registry-pattern.md
- tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden
- docs/prd/workflow-step-types.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/runner/step/pager_test.go
- pkg/runner/step/spin_test.go
Address CodeRabbit feedback on PR #1899: TestInteractiveHandlers_ExecuteWithoutTTY asserts ErrStepTTYRequired, but CheckTTY only triggers when stdin or stdout is not a TTY. Run from an interactive terminal (both TTYs), Execute would fall through and block on the huh prompt, hanging the test. Add a pre-check that skips in that case; the hang-proof sibling TestInteractiveHandlersExecuteFailFast covers the TTY-present path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The workflow executor printed the bare step label (e.g. "step1") to stderr on every run when the progress bar was disabled, breaking backward compatibility — the show.* features are documented as opt-in (default false). Gate the label print on show.count so default output is unchanged while the count feature still works in non-progress/non-TTY mode. Also regenerate the workflow-retries snapshot to match the intended WithExplanationf error formatting (## Explanation before ## Hints), and add the invoke field to the WorkflowStep schema block in the PRD for consistency with the documented exec/built-in behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
|
These changes were released in v1.221.0-rc.2. |
This branch was successfully deployed
3 participants
what
{{ .steps.step1.value }})why
Addresses DEV-263 (add input type to workflows) and DEV-2969 (add viewport support). Enables users to build complex multi-step workflows with user interaction, conditional execution, and flexible result display.
references
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes