pkg/schema/workflow.go (1)

15-53: WorkflowStep schema extension looks solid.

Fields are well-organized by category with clear inline comments. The Timeout field as a string is a reasonable choice for YAML/JSON configs (e.g., "30s", "5m").

One note: consider documenting valid Output mode 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 ctx parameter is unused. If pager.Run doesn'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.Error fails, 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.Info error 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.go rather than dynamic fmt.Errorf strings. For example, lines 53, 60, 90, 101, and 131 use dynamic error messages. Consider defining static errors like ErrFailedToResolvePath, ErrFailedToScanDirectory, ErrNoFilesFound, and ErrFileSelectionFailed in 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.Walk and return nil when 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 that len(step.Data) > 0 in the condition. While line 51 checks len(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.go for 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 in errors/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.Errorf with a dynamic message. Per coding guidelines, define a static error like ErrConfirmationFailed in errors/errors.go and 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.Fields will break on quoted arguments (e.g., echo "hello world"). The TODO is noted, but this could cause subtle bugs. A library like github.com/kballard/go-shellquote or github.com/google/shlex would 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 nolintlint comment 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 nolintlint comment 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 in init(), 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 ctx parameter is passed but unused in all three Resolve* methods. This is fine for forward compatibility (future cancellation support), but you could add a _ = ctx or 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
 	}

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 like ErrWorkflowStepFailed.

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 ErrWorkflowStepFailed exists in errors/errors.go (per learnings, it should).

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.Count

Based 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 string

Based 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 ctx parameter is unused in ResolveContent, ResolvePrompt, and ResolveCommand. 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.
+		}
 	}

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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

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.

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.

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.

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

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

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

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

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.

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.

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

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.

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.

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.

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.

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.

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.

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

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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

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

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

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.

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.

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.

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.

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.

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.

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

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

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

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

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: 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.

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

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.

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.

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.

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.

examples/custom-commands/.atmos.d/interactive.yaml (1)

40-43: Consider: Warning displays regardless of selected environment.

The prod_warning toast fires for every environment, not just prod. If the workflow system supports conditional steps, you might want to gate this on {{ .steps.env.value }} equaling prod. If this is intentional for demo purposes, no action needed.

pkg/runner/step/shell_test.go (1)

210-245: Consider removing or adjusting the nil error test case.

The test case for nil error (lines 216-220) is skipped at runtime (lines 237-239). Either remove this case since it's not being tested, or actually test the behavior when getExitCode receives nil (if that's a valid scenario to handle).

Option 1: Remove the skipped case

 func TestGetExitCode(t *testing.T) {
 	tests := []struct {
 		name     string
 		err      error
 		expected int
 	}{
-		{
-			name:     "nil error",
-			err:      nil,
-			expected: 1, // Default for non-exit errors.
-		},
 		{
 			name: "exec.ExitError with code 2",

Option 2: Actually test it

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if tt.err == nil {
-				// Test that nil is handled - though in practice getExitCode is only called on error.
-				return
-			}
 			result := getExitCode(tt.err)
 			assert.Equal(t, tt.expected, result)
 		})
 	}

pkg/runner/step/env_test.go (1)

16-124: Prefer table-driven subtests for EnvHandler scenarios.

Multiple subtests repeat the same setup/assertion patterns; a table-driven structure would reduce duplication and align with the repo’s test conventions.
As per coding guidelines, consider converting the subtests to a table-driven format.

pkg/schema/task.go (1)

250-256: Silent timeout parse failure could mask configuration errors.

When time.ParseDuration fails, the code silently continues with a zero timeout. This hides malformed duration strings like "30x" or "invalid" from users, potentially causing unexpected behavior where no timeout is applied.

Consider logging a warning or returning an error to surface invalid configurations.

🔧 Proposed approach

 func TaskFromWorkflowStep(step *WorkflowStep) Task {
 	// Parse timeout string to time.Duration.
 	var timeout time.Duration
 	if step.Timeout != "" {
-		if parsed, err := time.ParseDuration(step.Timeout); err == nil {
+		parsed, err := time.ParseDuration(step.Timeout)
+		if err != nil {
+			// Log warning for invalid timeout format.
+			// Consider: log.Warn("invalid timeout format", "step", step.Name, "timeout", step.Timeout, "error", err)
+		} else {
 			timeout = parsed
 		}
 	}

pkg/runner/step/executor.go (1)

150-168: Consider making legacyTypes a package-level variable.

The legacyTypes map is recreated on every call to IsExtendedStepType. Moving it to package level would avoid repeated allocations, though the current approach is fine for clarity and the function isn't on a hot path.

🔧 Optional optimization

+// legacyTypes identifies step types handled by the legacy executor.
+var legacyTypes = map[string]bool{
+	"atmos": true,
+	"shell": true,
+	"":      true,
+}
+
 func IsExtendedStepType(stepType string) bool {
 	defer perf.Track(nil, "step.IsExtendedStepType")()
 
-	// Legacy types that should be handled by existing executor.
-	legacyTypes := map[string]bool{
-		"atmos": true,
-		"shell": true,
-		"":      true, // Empty defaults to shell in legacy.
-	}
-
 	if legacyTypes[stepType] {
 		return false
 	}

pkg/workflow/progress.go (1)

36-64: Good defensive initialization with nil return.

Returning nil when progress is disabled or TTY unavailable is clean. All methods handle nil receivers gracefully. Consider adding perf.Track to NewProgressRenderer per coding guidelines.

♻️ Optional: Add performance tracking

 func NewProgressRenderer(workflow *schema.WorkflowDefinition, totalSteps int) *ProgressRenderer {
+	defer perf.Track(nil, "workflow.NewProgressRenderer")()
+
 	// Check if progress is enabled at workflow level.
 	showCfg := step.GetShowConfig(nil, workflow)

internal/exec/workflow_utils.go (1)

522-538: Duplicated flags map builder.

This is similar to buildFlagsMap in pkg/workflow/executor.go. Consider extracting to a shared location if this pattern grows, but acceptable for now given the different call sites and parameter styles.

pkg/runner/step/clear_test.go (1)

37-62: Consider consolidating redundant subtests.

Both "executes successfully" (line 37) and "returns empty result value" (line 51) verify the same behavior: result.Value is empty. They could be a single test, or the second could test something distinct like metadata or a different scenario.

💡 Suggested consolidation

 	t.Run("executes successfully", func(t *testing.T) {
 		step := &schema.WorkflowStep{
 			Name: "test",
 			Type: "clear",
 		}
 		vars := NewVariables()
 		ctx := context.Background()

 		result, err := handler.Execute(ctx, step, vars)
 		require.NoError(t, err)
 		assert.NotNil(t, result)
-		assert.Empty(t, result.Value)
-	})
-
-	t.Run("returns empty result value", func(t *testing.T) {
-		step := &schema.WorkflowStep{
-			Name: "test",
-			Type: "clear",
-		}
-		vars := NewVariables()
-		ctx := context.Background()
-
-		result, err := handler.Execute(ctx, step, vars)
-		require.NoError(t, err)
 		assert.Equal(t, "", result.Value)
 	})

pkg/runner/step/alert.go (1)

35-38: Consider terminal instance reuse.

Creating terminal.New() on each Execute call may be fine, but if this step runs frequently in a workflow, you might consider caching or reusing the terminal instance. Low priority since Alert steps are typically infrequent.

pkg/runner/step/pager.go (1)

125-140: Consider adding word wrap option for terminal width.

The glamour renderer could benefit from glamour.WithWordWrap(width) for better terminal display, but this is optional.

Optional: Add word wrap support

 func (h *PagerHandler) renderMarkdown(content string) (string, error) {
 	var opts []glamour.TermRendererOption
 
 	// Use theme-aware styles.
 	if glamourStyle, err := theme.GetGlamourStyleForTheme(theme.DefaultThemeName); err == nil {
 		opts = append(opts, glamour.WithStylesFromJSONBytes(glamourStyle))
 	}
+	opts = append(opts, glamour.WithWordWrap(80))
 
 	renderer, err := glamour.NewTermRenderer(opts...)

pkg/runner/step/atmos.go (1)

154-167: Consider extracting exit code metadata key.

"exit_code" is used here and in shell.go. The shell.go file defines exitCodeMetadata constant—consider reusing it here for consistency.

Optional: Use shared constant

 func (h *AtmosHandler) buildAtmosResult(stdout, stderr string, err error) *StepResult {
 	if err != nil {
 		return NewStepResult(stdout).
 			WithError(stderr).
 			WithMetadata("stdout", stdout).
 			WithMetadata("stderr", stderr).
-			WithMetadata("exit_code", getExitCode(err))
+			WithMetadata(exitCodeMetadata, getExitCode(err))
 	}
 	return NewStepResult(strings.TrimSpace(stdout)).
 		WithMetadata("stdout", stdout).
 		WithMetadata("stderr", stderr).
-		WithMetadata("exit_code", 0)
+		WithMetadata(exitCodeMetadata, 0)
 }

pkg/runner/step/variables_test.go (1)

324-340: Portability issue resolved.

Using t.Setenv ensures deterministic behavior across platforms. Good fix.

pkg/runner/step/style.go (2)

73-77: Consider handling ui.Writeln error.

The error from ui.Writeln(output) is discarded. If writing fails (e.g., broken pipe), the step silently succeeds. A minor edge case, but worth noting.

Optional: propagate write error

-	ui.Writeln(output)
-
-	return NewStepResult(content), nil
+	if err := ui.Writeln(output); err != nil {
+		return nil, errUtils.Build(errUtils.ErrWriteToStream).
+			WithCause(err).
+			WithContext("step", step.Name).
+			Err()
+	}
+	return NewStepResult(content), nil

---

88-91: Hardcoded theme may ignore user configuration.

GetGlamourStyleForTheme(theme.DefaultThemeName) always uses "atmos" theme for markdown rendering, even if the user configured a different theme. Consider using the active theme.

Use active theme instead

-	if glamourStyle, err := theme.GetGlamourStyleForTheme(theme.DefaultThemeName); err == nil {
+	// Use the currently configured theme for markdown rendering.
+	themeName := theme.DefaultThemeName
+	if activeStyles := theme.GetCurrentStyles(); activeStyles != nil {
+		// GetCurrentStyles loads the active theme, so it's now cached.
+		// We could expose the theme name, but for now fall through to default.
+	}
+	if glamourStyle, err := theme.GetGlamourStyleForTheme(themeName); err == nil {

Alternatively, expose getActiveThemeName() as public or pass theme context.

pkg/workflow/show_renderer.go (1)

27-58: Add performance tracking to RenderHeaderIfNeeded.

Per coding guidelines, public methods doing meaningful work should include defer perf.Track(). This method performs UI rendering and configuration lookups.

Add perf tracking

+import (
+	"github.com/cloudposse/atmos/pkg/perf"
+	// ... existing imports
+)

 func (r *ShowRenderer) RenderHeaderIfNeeded(
 	workflow *schema.WorkflowDefinition,
 	workflowName string,
 	flags map[string]string,
 ) {
+	defer perf.Track(nil, "workflow.ShowRenderer.RenderHeaderIfNeeded")()
+
 	if r.headerDone {
 		return
 	}

Based on coding guidelines requiring perf tracking for all public functions.

pkg/runner/step/sleep.go (1)

58-68: Avoid timer leak on context cancellation.
time.After allocates a timer that can linger if ctx.Done() fires first. Prefer time.NewTimer with Stop() and drain.

♻️ Proposed fix

-	// Create a timer that respects context cancellation.
-	select {
-	case <-time.After(duration):
-		return NewStepResult(duration.String()), nil
-	case <-ctx.Done():
-		return nil, errUtils.Build(errUtils.ErrUserAborted).
-			WithCause(ctx.Err()).
-			WithContext("step", step.Name).
-			WithExplanation("Sleep interrupted by context cancellation").
-			Err()
-	}
+	// Create a timer that respects context cancellation.
+	timer := time.NewTimer(duration)
+	defer func() {
+		if !timer.Stop() {
+			select {
+			case <-timer.C:
+			default:
+			}
+		}
+	}()
+	select {
+	case <-timer.C:
+		return NewStepResult(duration.String()), nil
+	case <-ctx.Done():
+		return nil, errUtils.Build(errUtils.ErrUserAborted).
+			WithCause(ctx.Err()).
+			WithContext("step", step.Name).
+			WithExplanation("Sleep interrupted by context cancellation").
+			Err()
+	}

pkg/runner/step/ui_handlers_test.go (2)

16-40: Test covers core UI handlers but missing linebreak and sleep.

The registration test validates 7 handlers but linebreak and sleep are tested separately. Consider adding them to this table for completeness.

 		{"clear", CategoryUI},
 		{"env", CategoryUI},
 		{"exit", CategoryUI},
+		{"linebreak", CategoryUI},
+		{"sleep", CategoryUI},
 	}

---

277-286: Silent skip on execution failure may mask test issues.

When Execute fails, the test passes without verifying anything. Consider using t.Skip() to make it explicit that verification was skipped.

 			if err == nil {
 				assert.Equal(t, tt.expectedContent, result.Value)
+			} else {
+				t.Skipf("Skipping verification - execution failed in test environment: %v", err)
 			}

pkg/runner/step/alert.go (1)

35-37: Consider caching terminal instance if called frequently in this handler.

terminal.New() performs system calls to detect TTY and determine color support. While this pattern is consistent throughout the codebase and already performance tracked, you could optimize by caching the instance if Execute is called repeatedly (similar to the pattern in pkg/list/extract/metadata.go using sync.OnceValue). However, this is optional since the current approach ensures fresh terminal state detection on each call.

pkg/runner/step/spin_test.go (1)

356-457: Consider table-driven cases for RunCommand.
This block has many similar scenarios; a table-driven structure would reduce repetition and make additions easier. As per coding guidelines.

internal/exec/workflow_utils.go (1)

505-509: Missing perf.Track on public function.

Per coding guidelines, public functions in pkg, internal, and cmd should include performance tracking. Since this is a simple state reset, it qualifies as a trivial setter exception, but worth confirming intent.

Optional: Add perf.Track if desired

 // ResetStepExecutorState resets the step executor state.
 // This should be called at the start of a new workflow execution.
 func ResetStepExecutorState() {
+	defer perf.Track(nil, "exec.ResetStepExecutorState")()
+
 	stepExecutorState = nil
 }