Repository navigation
feat(workflows): interactive steps run in CI via non-TTY defaults - #2709
Brian Ojeda (sgtoj) wants to merge 3 commits into
Conversation
Interactive step types (choose, input, confirm, filter, file, write) now
use their configured `default` value when running without a TTY (e.g. in
CI) instead of failing with ErrStepTTYRequired. Without a default, the
historical TTY-required error is preserved.
Also evaluate value-producing YAML functions (!env, !exec) in workflow
interactive step fields (default/prompt/placeholder/options) so defaults
can be sourced from the environment, and resolve step-variable templates
({{ .steps.* }}, {{ .env.* }}, {{ .flags.* }}) in workflow shell/atmos/exec
step commands (parity with custom command steps).
Covers workflows and custom commands via the shared step handler registry.
- Document non-TTY default fallback in the steps overview and the six interactive step type pages (choose, input, confirm, filter, file, write), adding the `default` field to filter/file/write. - Add a 'Running Interactive Steps in CI (non-TTY)' section to custom command steps docs. - Add changelog blog post and a Workflows Overhaul roadmap milestone. - Add PRD docs/prd/interactive-steps-non-tty-defaults.md.
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
📝 WalkthroughWalkthroughAdds a shared non-TTY interactive fallback ( ChangesNon-TTY interactive defaults and workflow templating
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/runner/step/choose.go (1)
94-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame
resolveDefaultbody duplicated across four handlers — hoist intoBaseHandler.
choose.go,file.go,filter.go, andwrite.goeach define an identical privateresolveDefault(empty-check →vars.Resolve→ wrap error).BaseHandler.ResolvePromptalready does the same thing for thePromptfield — mirror it forDefaultand delete the four copies. This would also makeconfirm.go's missing-templating bug (see comment there) structurally impossible to reintroduce.♻️ Suggested consolidation
+// ResolveDefault resolves template variables in the step's default value. +func (h BaseHandler) ResolveDefault(step *schema.WorkflowStep, vars *Variables) (string, error) { + defer perf.Track(nil, "step.BaseHandler.ResolveDefault")() + + if step.Default == "" { + return "", nil + } + resolved, err := vars.Resolve(step.Default) + if err != nil { + return "", errUtils.Build(errUtils.ErrTemplateEvaluation). + WithCause(err). + WithContext("step", step.Name). + WithContext("field", "default"). + Err() + } + return resolved, nil +}Then replace each handler's local
resolveDefaultwith a call toh.ResolveDefault(step, vars).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/step/choose.go` around lines 94 - 103, The same resolveDefault logic is duplicated in ChooseHandler, FileHandler, FilterHandler, and WriteHandler, so move it into BaseHandler alongside ResolvePrompt and make it handle the Default field with the same empty-check, vars.Resolve call, and wrapped error. Update the handler methods in choose.go, file.go, filter.go, and write.go to call h.ResolveDefault(step, vars) instead of keeping private copies, and remove the duplicated implementations.pkg/runner/step/interactive_default_test.go (1)
168-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemplating test only covers
choose— extend it table-driven across handlers.This is the only test asserting default templating (
{{ .env.* }}), and it exercises just thechoosetype. It's a single-case test today, while the siblingTestInteractiveHandlers_NonTTYUsesDefaultis properly table-driven. Broadening this into a table-driven test acrossinput,confirm,file,filter,writewould have caught the confirm.go templating bug flagged inconfirm.go.As per coding guidelines, "Use table-driven tests for testing multiple scenarios in Go."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/runner/step/interactive_default_test.go` around lines 168 - 194, The non-TTY default-templating coverage is too narrow because TestInteractiveHandlers_NonTTYDefaultTemplating only exercises the choose handler. Refactor this test into a table-driven test like TestInteractiveHandlers_NonTTYUsesDefault and run the same {{ .env.* }} default-templating assertion across the relevant interactive handlers (input, confirm, file, filter, write, and choose). Use Get("...") with each handler name and verify Execute returns the rendered default value for each case so templating regressions in handlers like confirm are covered.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/exec/workflow_step_functions_test.go`:
- Around line 70-73: The test in resolveStepFunctionString should assert the
specific sentinel error instead of just any error. Update the “malformed env
function returns an error” case to use require.ErrorIs against
pkg/utils.ErrInvalidAtmosYAMLFunction, keeping the existing
resolveStepFunctionString and atmosConfig setup intact.
In `@internal/exec/workflow_step_functions.go`:
- Around line 146-155: The current env handling in workflowStepFunction reuses
stepExecutorState.Variables(), so SetEnv mutates shared state and can leak
values into later steps. Update the logic in workflowStepFunction to scope env
overrides only to the current command rendering, either by working on a
step-local copy of the Variables state before calling Resolve or by restoring
the original env map immediately after Resolve completes. Keep the fix centered
around the workflowStepFunction and its Variables/SetEnv/Resolve flow so later
steps do not see prior step env values.
In `@pkg/runner/step/confirm.go`:
- Around line 41-55: The ConfirmHandler default value is using the raw
step.Default string, so templated defaults like {{ .env.* }} or !exec are never
resolved and always fall back to false. In ConfirmHandler.Execute, resolve
step.Default through vars.Resolve first (as done in
choose/input/file/filter/write) and then apply the yes/true check on the
resolved value before the non-TTY branch uses confirmValue(defaultVal).
---
Nitpick comments:
In `@pkg/runner/step/choose.go`:
- Around line 94-103: The same resolveDefault logic is duplicated in
ChooseHandler, FileHandler, FilterHandler, and WriteHandler, so move it into
BaseHandler alongside ResolvePrompt and make it handle the Default field with
the same empty-check, vars.Resolve call, and wrapped error. Update the handler
methods in choose.go, file.go, filter.go, and write.go to call
h.ResolveDefault(step, vars) instead of keeping private copies, and remove the
duplicated implementations.
In `@pkg/runner/step/interactive_default_test.go`:
- Around line 168-194: The non-TTY default-templating coverage is too narrow
because TestInteractiveHandlers_NonTTYDefaultTemplating only exercises the
choose handler. Refactor this test into a table-driven test like
TestInteractiveHandlers_NonTTYUsesDefault and run the same {{ .env.* }}
default-templating assertion across the relevant interactive handlers (input,
confirm, file, filter, write, and choose). Use Get("...") with each handler name
and verify Execute returns the rendered default value for each case so
templating regressions in handlers like confirm are covered.
🪄 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: e0ee244b-3e87-4943-8388-59319ac3aec3
📒 Files selected for processing (23)
docs/prd/interactive-steps-non-tty-defaults.mdinternal/exec/workflow_step_functions.gointernal/exec/workflow_step_functions_test.gointernal/exec/workflow_utils.gopkg/runner/step/choose.gopkg/runner/step/confirm.gopkg/runner/step/file.gopkg/runner/step/filter.gopkg/runner/step/handler_base.gopkg/runner/step/input.gopkg/runner/step/interactive_default_test.gopkg/runner/step/write.gowebsite/blog/2026-07-07-interactive-steps-ci-defaults.mdxwebsite/blog/authors.ymlwebsite/docs/cli/configuration/commands/command/steps.mdxwebsite/docs/workflows/workflows/workflow/steps/index.mdxwebsite/docs/workflows/workflows/workflow/steps/type/choose.mdxwebsite/docs/workflows/workflows/workflow/steps/type/confirm.mdxwebsite/docs/workflows/workflows/workflow/steps/type/file.mdxwebsite/docs/workflows/workflows/workflow/steps/type/filter.mdxwebsite/docs/workflows/workflows/workflow/steps/type/input.mdxwebsite/docs/workflows/workflows/workflow/steps/type/write.mdxwebsite/src/data/roadmap.js
| t.Run("malformed env function returns an error", func(t *testing.T) { | ||
| _, err := resolveStepFunctionString(atmosConfig, "!env") | ||
| require.Error(t, err) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find what error processCustomTags returns for malformed !env
rg -n 'func processCustomTags\b' --type go internal/ pkg/ --glob '!*_test.go' -C 10
# Find error sentinels related to env/yaml function evaluation
rg -n 'Err.*Env\|Err.*Yaml\|Err.*Function\|Err.*Tag\|Err.*Custom' --type go errors/ --glob '!*_test.go'Repository: cloudposse/atmos
Length of output: 3150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and surrounding error definitions.
sed -n '1,260p' internal/exec/yaml_func_utils.go
printf '\n---\n'
sed -n '1,220p' internal/exec/workflow_step_functions_test.go
printf '\n=== sentinels / error types ===\n'
rg -n 'Err[A-Z][A-Za-z0-9_]*|errors\.New|fmt\.Errorf\(' internal/ pkg/ --glob '!*_test.go' -C 2Repository: cloudposse/atmos
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow to the workflow-step function path and the error created for malformed tags.
rg -n 'resolveStepFunctionString|processCustomTagsWithContext|malformed|!env|invalid|error' internal/exec --glob '!*_test.go' -C 3
# Print the lines around processCustomTagsWithContext and any malformed-tag branches.
sed -n '260,520p' internal/exec/yaml_func_utils.goRepository: cloudposse/atmos
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the implementation paths that drive the malformed !env error.
sed -n '170,360p' internal/exec/yaml_func_utils.go
printf '\n---\n'
sed -n '1,140p' internal/exec/workflow_step_functions_test.go
printf '\n---\n'
sed -n '1,220p' pkg/utils/yaml_utils.goRepository: cloudposse/atmos
Length of output: 19962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any sentinel or wrapped error specifically used by the custom-tag resolver.
rg -n 'errors\.New|fmt\.Errorf|ErrorIs|errors\.Is|custom tag|malformed !env|!env' internal/exec pkg/ --glob '!*_test.go' -C 2
# Show the exact implementation of the function under test if it's short enough.
awk 'NR>=170 && NR<=260 {print NR ":" $0}' internal/exec/yaml_func_utils.goRepository: cloudposse/atmos
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact resolver used by the test and its error handling.
rg -n 'func resolveStepFunctionString\b|func resolveWorkflowStepFunctions\b|ProcessTagEnv\(' internal/exec/workflow_step_functions.go internal/exec/yaml_func_utils.go pkg/utils/yaml_func_env.go -C 6
printf '\n--- workflow_step_functions.go ---\n'
awk 'NR>=1 && NR<=220 {print NR ":" $0}' internal/exec/workflow_step_functions.go
printf '\n--- yaml_func_env.go ---\n'
awk 'NR>=1 && NR<=140 {print NR ":" $0}' pkg/utils/yaml_func_env.goRepository: cloudposse/atmos
Length of output: 13257
Use require.ErrorIs for this case. !env returns pkg/utils.ErrInvalidAtmosYAMLFunction, so assert that sentinel instead of only checking that an error occurred.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/workflow_step_functions_test.go` around lines 70 - 73, The test
in resolveStepFunctionString should assert the specific sentinel error instead
of just any error. Update the “malformed env function returns an error” case to
use require.ErrorIs against pkg/utils.ErrInvalidAtmosYAMLFunction, keeping the
existing resolveStepFunctionString and atmosConfig setup intact.
| vars := stepExecutorState.Variables() | ||
| for _, env := range stepEnv { | ||
| key, value, ok := strings.Cut(env, "=") | ||
| if !ok { | ||
| continue | ||
| } | ||
| vars.SetEnv(key, value) | ||
| } | ||
| return vars.Resolve(command) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Variables() returns a shared mutable object and how
# custom command steps handle env var templating for parity comparison.
# Find the Variables() method definition
rg -n 'func.*Variables\(\)' --type go internal/ pkg/ --glob '!*_test.go' -C 5
# Find the SetEnv method definition
rg -n 'func.*SetEnv\(' --type go internal/ pkg/ --glob '!*_test.go' -C 5
# Find stepExecutorState declaration and initialization
rg -n 'stepExecutorState' --type go internal/exec/ --glob '!*_test.go' -C 3
# Check how custom command steps handle env var templating
rg -n 'SetEnv\|\.env\.' --type go internal/exec/ --glob '!*_test.go' -C 3Repository: cloudposse/atmos
Length of output: 10172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the executor/variables implementation and the workflow step flow.
sed -n '1,240p' pkg/runner/step/variables.go
printf '\n---\n'
sed -n '1,220p' pkg/runner/step/executor.go
printf '\n---\n'
sed -n '760,840p' internal/exec/workflow_utils.go
printf '\n---\n'
sed -n '120,190p' internal/exec/workflow_step_functions.goRepository: cloudposse/atmos
Length of output: 16856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where resolveWorkflowStepCommand is called and how stepEnv is assembled.
rg -n 'resolveWorkflowStepCommand\(' internal/ --glob '!*_test.go' -C 4
printf '\n---\n'
rg -n 'stepEnv|envVars' internal/exec/ --glob '!*_test.go' -C 4
printf '\n---\n'
rg -n 'workflowCommandSupportsTemplating|RenderCommand|show\.command|render command' internal/ --glob '!*_test.go' -C 3Repository: cloudposse/atmos
Length of output: 32969
Avoid reusing the shared env store here internal/exec/workflow_step_functions.go:146-151
SetEnv mutates stepExecutorState.Variables(), so a later step can resolve {{ .env.* }} from values left behind by earlier steps and leak them into the rendered command. Scope resolution to the current step only, or restore the env map after Resolve.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/exec/workflow_step_functions.go` around lines 146 - 155, The current
env handling in workflowStepFunction reuses stepExecutorState.Variables(), so
SetEnv mutates shared state and can leak values into later steps. Update the
logic in workflowStepFunction to scope env overrides only to the current command
rendering, either by working on a step-local copy of the Variables state before
calling Resolve or by restoring the original env map immediately after Resolve
completes. Keep the fix centered around the workflowStepFunction and its
Variables/SetEnv/Resolve flow so later steps do not see prior step env values.
| func (h *ConfirmHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { | ||
| defer perf.Track(nil, "step.ConfirmHandler.Execute")() | ||
|
|
||
| if err := h.CheckTTY(step); err != nil { | ||
| useTTY, err := h.resolveInteractive(step) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Parse default value. | ||
| defaultVal := strings.ToLower(step.Default) == "yes" || strings.ToLower(step.Default) == "true" | ||
|
|
||
| // Non-TTY with a configured default: use the default without prompting. | ||
| if !useTTY { | ||
| return NewStepResult(confirmValue(defaultVal)), nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Confirm default is never templated — {{ .env.* }}/!exec defaults silently evaluate to false.
Every other interactive handler (choose, input, file, filter, write) resolves step.Default through vars.Resolve before use. confirm.go compares the raw, un-resolved step.Default string directly against "yes"/"true". A workflow author writing default: "{{ .env.AUTO_APPROVE }}" for a confirm step will always get false in CI, since the literal template string never matches. This is a silent correctness gap — no error, just the wrong (safe-looking but potentially wrong) answer. TestInteractiveHandlers_NonTTYDefaultTemplating only covers choose, so this slipped through.
🐛 Proposed fix
- // Parse default value.
- defaultVal := strings.ToLower(step.Default) == "yes" || strings.ToLower(step.Default) == "true"
+ // Parse default value (resolve templates first, e.g. {{ .env.VAR }}).
+ resolvedDefault := step.Default
+ if resolvedDefault != "" {
+ var resolveErr error
+ resolvedDefault, resolveErr = vars.Resolve(resolvedDefault)
+ if resolveErr != nil {
+ return nil, fmt.Errorf("step '%s': failed to resolve default: %w", step.Name, resolveErr)
+ }
+ }
+ defaultVal := strings.ToLower(resolvedDefault) == "yes" || strings.ToLower(resolvedDefault) == "true"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (h *ConfirmHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { | |
| defer perf.Track(nil, "step.ConfirmHandler.Execute")() | |
| if err := h.CheckTTY(step); err != nil { | |
| useTTY, err := h.resolveInteractive(step) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Parse default value. | |
| defaultVal := strings.ToLower(step.Default) == "yes" || strings.ToLower(step.Default) == "true" | |
| // Non-TTY with a configured default: use the default without prompting. | |
| if !useTTY { | |
| return NewStepResult(confirmValue(defaultVal)), nil | |
| } | |
| func (h *ConfirmHandler) Execute(ctx context.Context, step *schema.WorkflowStep, vars *Variables) (*StepResult, error) { | |
| defer perf.Track(nil, "step.ConfirmHandler.Execute")() | |
| useTTY, err := h.resolveInteractive(step) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Parse default value (resolve templates first, e.g. {{ .env.VAR }}). | |
| resolvedDefault := step.Default | |
| if resolvedDefault != "" { | |
| var resolveErr error | |
| resolvedDefault, resolveErr = vars.Resolve(resolvedDefault) | |
| if resolveErr != nil { | |
| return nil, fmt.Errorf("step '%s': failed to resolve default: %w", step.Name, resolveErr) | |
| } | |
| } | |
| defaultVal := strings.ToLower(resolvedDefault) == "yes" || strings.ToLower(resolvedDefault) == "true" | |
| // Non-TTY with a configured default: use the default without prompting. | |
| if !useTTY { | |
| return NewStepResult(confirmValue(defaultVal)), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/runner/step/confirm.go` around lines 41 - 55, The ConfirmHandler default
value is using the raw step.Default string, so templated defaults like {{ .env.*
}} or !exec are never resolved and always fall back to false. In
ConfirmHandler.Execute, resolve step.Default through vars.Resolve first (as done
in choose/input/file/filter/write) and then apply the yes/true check on the
resolved value before the non-TTY branch uses confirmValue(defaultVal).
|
Closing to iterate on the implementation in my fork (addressing review feedback: dedupe default resolution, remove dead CheckTTY, align workflow template engine with custom commands, non-mutating env resolution). Will reopen when ready. |
What
Interactive workflow and custom-command step types (
choose,input,confirm,filter,file,write) now fall back to their configureddefaultvalue when running without a TTY (e.g. in CI) instead of failing withinteractive terminal required for step. Without adefault, the historical TTY-required error is preserved, so an unattended run never proceeds with an unintended value.This lands as three cohesive parts:
pkg/runner/step) — a sharedBaseHandler.resolveInteractivehelper drives the decision for every interactive handler. Because workflows, custom commands, hooks, and the task runner all execute steps through the same handler registry, one change covers them all.internal/exec) — workflow stepdefault/prompt/options/placeholdernow evaluate the context-free!envand!execfunctions. Workflow manifests previously left these as literal!env ...strings; custom commands defined inatmos.yamlalready resolved them. Stack-dependent functions (!terraform.output,!store, ...) are intentionally not evaluated here (a workflow step has no component/stack context).internal/exec) — workflowshell/atmos/execstep commands resolve{{ .steps.* }},{{ .env.* }}, and{{ .flags.* }}, so values captured by an interactive step (or its CI default) flow into later steps — parity with custom command steps.Example that now runs both locally (prompts) and in CI (uses defaults):
Why
Interactive steps made workflows and custom commands friendly to run by hand but unrunnable in CI, forcing teams to maintain a second, prompt-free copy of the same automation. A configured
defaultis an explicit opt-in to a non-interactive value, so honoring it when there is no TTY is safe and removes the need for duplicate definitions. Leavingdefaultunset keeps the guard for steps where a human must choose.Testing
ErrStepTTYRequiredwhen no default is set.!env/!execresolution (and passthrough of plain, template, and stack-dependent values), nested steps, and{{ .steps.* }}/{{ .env.* }}command resolution.References
docs/prd/interactive-steps-non-tty-defaults.mdwebsite/blog/2026-07-07-interactive-steps-ci-defaults.mdxwebsite/src/data/roadmap.jsSummary by CodeRabbit