Repository navigation
Add casts core recording and rendering support - #2692
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
|
Warning This PR exceeds the recommended limit of 10,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. |
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces asciicast-based cast recording and rendering (record, play, render to GIF/MP4/HTML/ASCII/PNG/JPEG), new workflow step handlers (cast, script, workdir, hint), config command merge/YAML-include fixes, terminal color/width and help-rendering refactors, IO masking/recording infrastructure, and supporting CI/build/doc updates. ChangesCast Recording Engine & CLI
Workflow Step Execution & Schema
Config Command Merging & YAML Includes
Terminal, Color & UI Width Handling
IO Masking, Recording & Shell Execution Infrastructure
Misc Supporting Changes
CI Templates and Golden Snapshot Formatting
Documentation Fix Notes and Launch Content
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant Root as RootCmd
participant CastCmd as castcmd (recording.go)
participant Recorder as asciicast.Recorder
participant Render as asciicast.Render
User->>Root: atmos <command> --cast=demo.gif
Root->>CastCmd: StartRecordingIfRequested(cmd, config, args)
CastCmd->>CastCmd: resolveRecordingRequest / planRecordingOutput
CastCmd->>Recorder: Start(Options)
Root->>Root: execute command, write output
Root->>CastCmd: FinalizeRecording()
CastCmd->>Recorder: Close()
CastCmd->>Render: Render(castPath, RenderOptions)
Render-->>CastCmd: rendered gif path
CastCmd-->>User: "Cast rendered" message
sequenceDiagram
participant Runner as WorkflowRunner
participant CastHandler as CastHandler
participant Recorder as asciicast.Recorder
participant Child as Child Step Executor
participant Renderer as asciicast render
Runner->>CastHandler: Execute(step, vars)
CastHandler->>Recorder: startStepRecorder
loop each child step
CastHandler->>Child: runCastChildStep (simulate or executor)
Child-->>CastHandler: StepResult
end
alt child failed
CastHandler->>Recorder: Discard()
else all succeeded
CastHandler->>Recorder: Close()
CastHandler->>Renderer: renderCastOutputs
end
CastHandler-->>Runner: StepResult with cast/gif metadata
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
cmd/root.go (1)
2177-2258: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPager-based help paths overwrite the cast recording tee, dropping captured help output.
recordWriteris declared only inside theif recordWriter := castcmd.StartHelpRecording(...); recordWriter != nil { ... }block (Lines 2180-2183), andcommand.SetOut(...)there correctly tees to it. But later, both theisFlagHelppager-enabled branch and theisInteractiveHelpbranch callcommand.SetOut(&buf), which fully replaces the previous multiwriter and dropsrecordWriterentirely (it's out of scope to re-include). Sinceatmos help(interactive) always renders through thebuf+ pager path, a--castrecording ofatmos helpwould capture no actual help content — undermining the "help output capture" feature called out in the PR objectives.🐛 Suggested fix — hoist `recordWriter` and tee it through the buffer-based render paths too
- if recordWriter := castcmd.StartHelpRecording(command, &atmosConfig); recordWriter != nil { - command.SetOut(io.MultiWriter(command.OutOrStdout(), recordWriter)) - defer castcmd.FinalizeRecording() - } + recordWriter := castcmd.StartHelpRecording(command, &atmosConfig) + if recordWriter != nil { + command.SetOut(io.MultiWriter(command.OutOrStdout(), recordWriter)) + defer castcmd.FinalizeRecording() + }Then in each branch that swaps to a local buffer, tee into
recordWriteras well:if pagerExplicitlySet && pagerEnabled { var buf bytes.Buffer - command.SetOut(&buf) + if recordWriter != nil { + command.SetOut(io.MultiWriter(&buf, recordWriter)) + } else { + command.SetOut(&buf) + }case isInteractiveHelp: var buf bytes.Buffer - command.SetOut(&buf) + if recordWriter != nil { + command.SetOut(io.MultiWriter(&buf, recordWriter)) + } else { + command.SetOut(&buf) + }🤖 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 `@cmd/root.go` around lines 2177 - 2258, The help rendering paths are replacing the cast tee, so `--cast` stops capturing output when `command.SetOut(&buf)` is used in the `isFlagHelp` pager branch and the `isInteractiveHelp` branch. Hoist the `recordWriter` from `castcmd.StartHelpRecording` so it’s available in the later help-rendering branches, and when buffering help output, tee the buffer writes through the same writer before calling `command.Help()`. Keep the existing `command.SetOut(io.MultiWriter(...))` behavior for direct rendering, and make sure the pager path still forwards the captured help text to both the buffer and the cast recording.pkg/schema/task_validate.go (1)
45-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNested script children need the script-step checks too.
ValidateWorkflowStepsrecurses intoparallel/matrixchildren, butvalidateConcurrentChildnever applies theinterpreter/scriptrequired checks or thecommandban. Atype: scriptchild can still slip through here and fail later at execution. Add the script-step validation to the concurrent-child path.🤖 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/schema/task_validate.go` around lines 45 - 105, ValidateExecTasks and ValidateExecWorkflowSteps only enforce exec-step rules, but validateConcurrentChild is missing the script-step checks for nested parallel/matrix children. Update the concurrent-child validation path to also apply the same script validation used by validateScriptSteps, so type: script children are rejected for missing interpreter/script or for setting command. Use the existing execStepView/scriptStepView flow and the ValidateExecWorkflowSteps/validateConcurrentChild symbols to keep the logic consistent.pkg/io/streams.go (1)
126-146: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winpkg/io/streams.go:118-146,158-180 — Return the actual write count on partial writes.
maskedWriter.WriteanddynamicMaskedWriter.Writerecord the bytes that were written, but still return0on error/short-write. If a caller retries aftern=0, that prefix can be written and recorded twice. Returnwrittenon those paths, or skip recording until the write fully succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/io/streams.go` around lines 126 - 146, The Write methods in maskedWriter and dynamicMaskedWriter are returning 0 on error or short-write even after some bytes were successfully written and recorded, which can cause duplicate writes on retries. Update the Write logic so it returns the actual written count whenever progress was made, or defer recordOutput until the full write succeeds. Keep the behavior aligned with the existing maskedBytes/write handling in maskedWriter.Write and dynamicMaskedWriter.Write.cmd/cmd_utils.go (1)
839-1147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a regression test for custom-command failure aggregation. The current
cmdtests cover multi-step success and skipped steps, but not a failing step followed by later steps or the exit code chosen when joined errors include anExitCodeError.🤖 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 `@cmd/cmd_utils.go` around lines 839 - 1147, Add a regression test around the custom command step loop in cmd_utils.go, focusing on the logic that aggregates failures into commandErr and uses errUtils.CheckErrorPrintAndExit at the end. Cover a multi-step custom command where one step fails and later steps still run, then verify the joined error is reported correctly. Also add a case where the failing step returns an ExitCodeError so the final chosen exit code from the aggregated error matches the expected behavior.pkg/datafetcher/schema/atmos/manifest/1.0.json (1)
2021-2257: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
workflow_step.outputtopkg/datafetcher/schema/atmos/manifest/1.0.jsonThe step schema only definesoutputsfor named step exports; it still lacks theoutputobject that Atmos workflow steps support, so validoutput:configs will fail this schema.🤖 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/datafetcher/schema/atmos/manifest/1.0.json` around lines 2021 - 2257, The workflow step schema is missing support for the Atmos workflow step `output` object, so valid `output:` configurations are rejected. Update the step schema in `pkg/datafetcher/schema/atmos/manifest/1.0.json` alongside the existing `outputs` property to add `workflow_step.output` with the correct object shape and validation, using the same step definition block that already defines fields like `type`, `with`, and `outputs`.
🧹 Nitpick comments (17)
pkg/terminal/pacing_writer.go (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
perf.Trackon new public function.
HasRealTTYInputperforms a terminal/fd check (not a pure no-I/O lookup), similar toui.TerminalWidth()which does instrument withperf.Track. As per coding guidelines: "Adddefer perf.Track(atmosConfig, "pkg.FuncName")()... to all public functions ... Exceptions: ... pure validation/lookup functions with no I/O."♻️ Suggested fix
func HasRealTTYInput() bool { + defer perf.Track(nil, "terminal.HasRealTTYInput")() + return term.IsTerminal(int(os.Stdin.Fd())) //nolint:gosec // File descriptors are small OS-provided values accepted by x/term. }🤖 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/terminal/pacing_writer.go` around lines 34 - 36, Add perf instrumentation to the new public HasRealTTYInput function. Since it performs a terminal/fd check and is not a pure lookup, wrap the function body with a deferred perf.Track call using the appropriate atmosConfig and the pkg.HasRealTTYInput symbol, following the same pattern used by ui.TerminalWidth().Source: Coding guidelines
pkg/datafetcher/schema/stacks/stack-config/1.0.json (1)
2083-2086: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider constraining
modewith anenum.Description states simulate steps only support
typedandprompt, but the schema allows any string, so typos won't be caught by validation.♻️ Suggested fix
"mode": { "type": "string", + "enum": ["typed", "prompt"], "description": "Mode for cast and simulate steps. Simulate supports typed and prompt." },🤖 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/datafetcher/schema/stacks/stack-config/1.0.json` around lines 2083 - 2086, Constrain the schema field `mode` in the stack config JSON to the allowed values only. Update the `mode` definition under the cast/simulate step schema to use an `enum` with the supported values described in the `description` (`typed` and `prompt`) so validation catches typos instead of accepting any string.pkg/schema/task.go (1)
810-881: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate structured-prompt normalization logic.
normalizeTaskPromptMapandnormalizeWorkflowStepMapboth re-implement the same "structuredpromptis only valid fortype: simulate" check and error, but take different approaches (one decodes to a struct + deletes the key, the other renames the key for a later generic decode). Consider extracting a shared helper (e.g. one that returns the renamed map, letting the generic decode always build the struct) so the two paths can't drift.🤖 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/schema/task.go` around lines 810 - 881, The structured-prompt normalization logic is duplicated in normalizeTaskPromptMap and normalizeWorkflowStepMap, with both enforcing the same “prompt only valid for type simulate” rule. Extract a shared helper used by both paths to perform the type check and prompt normalization consistently, and make normalizeWorkflowStepMap and normalizeTaskPromptMap delegate to it so the behavior for prompt renaming/deletion and struct decoding stays aligned.pkg/schema/task_test.go (1)
329-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRound-trip test doesn't cover most of the new cast fields.
Only
Script/Interpretergot added to this round-trip test. The other new fields (Mode,Shell,WriteRate,KeyInterval,Jitter,Cursor,CursorSet,Text,Regex,Key,Duration,Interval,Repeat,Defaults,Source,Reset,CastOutput) aren't set on theTaskliteral or asserted afterToWorkflowStep/TaskFromWorkflowStep, so a dropped field in either conversion wouldn't be caught by this test.🤖 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/schema/task_test.go` around lines 329 - 393, The round-trip coverage in TestTaskWorkflowStepControlFieldsRoundTrip is incomplete for the new cast-related fields. Update the Task literal and the post-conversion assertions in this test to include the missing fields handled by Task.ToWorkflowStep and TaskFromWorkflowStep, especially Mode, Shell, WriteRate, KeyInterval, Jitter, Cursor, CursorSet, Text, Regex, Key, Duration, Interval, Repeat, Defaults, Source, Reset, and CastOutput, so a regression in either conversion will be caught.pkg/asciicast/session.go (1)
261-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded teardown timeout duplicates the
defaultTeardownMaxWaitconstant.
time.After(2 * time.Second)re-hardcodes the same value already named asdefaultTeardownMaxWait(line 24). Using the constant avoids the two values silently drifting apart later.♻️ Proposed fix
- case <-time.After(2 * time.Second): + case <-time.After(defaultTeardownMaxWait):🤖 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/asciicast/session.go` around lines 261 - 285, The teardown timeout in finishSession is hardcoded and duplicates the existing defaultTeardownMaxWait constant. Update the timeout branch in finishSession to use defaultTeardownMaxWait instead of an inline duration so the value stays centralized and cannot drift.pkg/runner/step/cast.go (2)
485-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant special-case for
"0"—time.ParseDurationalready handles it.Go's
time.ParseDurationhas a built-in special case that returns0, nilfor the literal string"0"without requiring a unit. The explicitif value == "0" { return 0, nil }branch duplicates stdlib behavior and can be dropped.♻️ Proposed simplification
func parseDurationDefault(value string, fallback time.Duration) (time.Duration, error) { if value == "" { return fallback, nil } - if value == "0" { - return 0, nil - } duration, err := time.ParseDuration(value) if err != nil { return 0, fmt.Errorf("invalid duration %q: %w", value, err) } return duration, nil }Please confirm current Go stdlib behavior for
time.ParseDuration("0")before applying, since this affects runtime library semantics.🤖 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/cast.go` around lines 485 - 497, The parseDurationDefault helper in cast.go has a redundant special-case for the literal "0"; time.ParseDuration already returns zero without an extra branch. Remove the explicit value == "0" check and rely on the existing time.ParseDuration call, keeping the fallback-empty-string behavior and the existing error wrapping intact.
142-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
ui.*/data.*helpers instead of rawfmt.Fprintffor user-facing messages.These write human-readable status messages via
fmt.Fprintfon the UI writer rather than through theui.*formatting layer (e.g.ui.Success/ui.Writef). Per the I/O vs. UI separation used elsewhere in this codebase, human messages should go throughui.*so theming, TTY-degradation, and secret masking apply consistently.As per coding guidelines, "Never use
fmt.Fprintf(os.Stdout/Stderr, ...)orfmt.Println(...). Usedata.*orui.*functions instead" and "pkg/io/**/*.go: ... Usedata.Write/Writef/Writelnfor pipeable output andui.Write/Writef/Writelnorui.Success/Error/Warning/Infofor human messages."Also applies to: 146-146
🤖 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/cast.go` at line 142, The human-facing status output in cast.go is being written with raw fmt.Fprintf on the UI writer, which bypasses the ui formatting layer. Update the message emission in the cast recording flow to use the appropriate ui.* helper (for example in the code around the Cast step and the recording-failed path) so theming, TTY fallback, and masking are handled consistently; also replace the other matching fmt.Fprintf call in this block with the same ui.* pattern.Source: Coding guidelines
pkg/provisioner/source/vendor.go (1)
97-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExisting-target/replace logic duplicated between the go-getter path and the newly added local-copy helpers.
The "does target exist / is replacement allowed / remove it" logic is written out twice for the go-getter path (pre-download fast-fail at lines 97-111, and the real check-and-remove after download at lines 165-186), while a nearly identical
prepareVendorTarget/handleExistingVendorTargetpair was added a few lines down (312-350) for the local-copy path. Consolidating the go-getter path onto the same helpers would remove ~25 lines of duplicated branching and keep both code paths behaviorally identical by construction.Also applies to: 165-186, 312-350
🤖 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/provisioner/source/vendor.go` around lines 97 - 111, The existing-target/replace branching is duplicated in vendor provisioning across the go-getter flow and the new local-copy helpers. Refactor the go-getter path in vendor.go to reuse the same prepareVendorTarget and handleExistingVendorTarget helpers used by the local-copy path, so the pre-download check and post-download removal follow one shared implementation. Keep the behavior for replaceTarget, target existence checks, and cleanup identical by routing both code paths through those shared helpers.errors/errors.go (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlias direction inverted vs. the rest of the PR.
Every other alias touched in this PR flows domain →
errUtils(e.g.asciicast.ErrUnknownSessionAction = errUtils.ErrUnknownSessionAction,step.ErrCastStepRequiresSteps = errUtils.ErrCastStepRequiresSteps). Here it's reversed:errUtils.ErrCommandEnvDecodeFailedaliases frompkg/schema, making the low-levelerrorspackage depend on a domain package. That's an unusual dependency direction for a package meant to be a leaf that everything else imports.Consider defining the sentinel canonically in
errors/errors.goand havingpkg/schemaalias from there instead, matching the pattern used everywhere else in this PR.Also applies to: 73-73
🤖 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 `@errors/errors.go` around lines 6 - 7, The alias direction for ErrCommandEnvDecodeFailed is inverted compared to the rest of the PR, with errors/errors.go currently importing pkg/schema instead of being the canonical source. Move the sentinel definition into the errors package and update pkg/schema to alias from errUtils, matching the existing pattern used by asciicast and step so the low-level errors package remains the leaf dependency.pkg/datafetcher/schema/config/global/1.0.json (1)
1401-1404: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider constraining
modewith an enum.
styleuses an enum for its limited value set, butmodeis a free-form string despite the description enumerating supported values. An enum would catch typos in workflow YAML at validation time.🤖 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/datafetcher/schema/config/global/1.0.json` around lines 1401 - 1404, The mode field is currently a free-form string even though its description implies a fixed set of values, so update the schema for the relevant config object to constrain mode with an enum like style does. Use the existing mode property in the global config schema and add the supported values from the description so workflow YAML typos are caught during validation.pkg/env/env.go (1)
27-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap errors with context, per repo convention.
Chdirreturns rawos.Chdir/os.Setenverrors with no context about which operation or caller failed. Other new code in this PR (e.g.WorkdirHandler.Execute) consistently wraps stdlib errors withfmt.Errorf("...: %w", err)for context. Since this is the exported entry point behind--chdir, an unwrapped*PathErroralone won't tell the user this came from a chdir operation.♻️ Proposed fix
func Chdir(dir string) error { defer perf.Track(nil, "env.Chdir")() if err := os.Chdir(dir); err != nil { - return err + return fmt.Errorf("chdir to %q: %w", dir, err) } - return os.Setenv("PWD", dir) + if err := os.Setenv("PWD", dir); err != nil { + return fmt.Errorf("set PWD to %q: %w", dir, err) + } + return nil }As per coding guidelines, "All errors MUST be wrapped using static errors defined in
errors/errors.go. Useerrors.Joinfor combining multiple errors,fmt.Errorfwith%wfor adding string context...".🤖 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/env/env.go` around lines 27 - 34, Chdir currently returns raw os.Chdir and os.Setenv errors without operation context, so wrap each failure with the repo’s standard error style. Update env.Chdir to keep the existing perf.Track call but return contextual wrapped errors for the os.Chdir and os.Setenv paths, using fmt.Errorf with %w and the static error conventions from errors/errors.go so callers can see which chdir step failed.Source: Coding guidelines
pkg/asciicast/recorder.go (1)
460-480: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
crypto/randover manually opening/dev/urandom.
/dev/urandomdoesn't exist on Windows, soRandomIDalways falls through to the weakertime.Now().UnixNano()-derived fallback there, not just on rare failure. That fallback shifts the same 64-bit value by 4 bits per index rather than sourcing independent entropy per byte, increasing collision risk for closely-timed recordings on Windows (a collision here surfaces asErrCastOutputExistsfromcreateCastTempFile).crypto/rand.Readis cross-platform and removes the need for OS-specific special-casing entirely.♻️ Proposed refactor
+import "crypto/rand" + func RandomID(n int) string { defer perf.Track(nil, "asciicast.RandomID")() const letters = "0123456789abcdef" b := make([]byte, n) - f, err := os.Open("/dev/urandom") - if err == nil { - defer func() { _ = f.Close() }() - if _, err := io.ReadFull(f, b); err == nil { - for i := range b { - b[i] = letters[int(b[i])%len(letters)] - } - return string(b) - } - } - t := time.Now().UnixNano() - for i := range b { - b[i] = letters[int(t>>uint(i*4))%len(letters)] - } + if _, err := rand.Read(b); err == nil { + for i := range b { + b[i] = letters[int(b[i])%len(letters)] + } + return string(b) + } + t := time.Now().UnixNano() + for i := range b { + b[i] = letters[int(t>>uint(i*4))%len(letters)] + } return string(b) }As per coding guidelines, "Ensure Linux/macOS/Windows compatibility." Please confirm
crypto/rand.Readsemantics match expectations across platforms before applying.🤖 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/asciicast/recorder.go` around lines 460 - 480, Replace the manual /dev/urandom handling in RandomID with crypto/rand.Read so the identifier generation works consistently across Linux, macOS, and Windows. Update RandomID to read n bytes directly from crypto/rand, then map those bytes to the existing hex alphabet as before, and keep the time.Now().UnixNano() fallback only as a last-resort error path if needed. Make sure the fix stays localized to RandomID and preserves its current callers such as createCastTempFile.Source: Coding guidelines
cmd/cast/recording.go (1)
37-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
RegisterRecordingFlagis missingperf.Track.Every other public function in this file has
defer perf.Track(...), or delegates entirely to one that does (ActiveRecordingWidth,recorderOutputWriter.Write). This one doesn't and isn't a getter/delegator.🔧 Proposed fix
func RegisterRecordingFlag(flags *pflag.FlagSet) { + defer perf.Track(nil, "cmd.cast.RegisterRecordingFlag")() + flags.String(FlagName, "", "Record command output as an asciinema cast (--cast for generated path, --cast=path with a .cast, .gif, .mp4, .html, .ascii, .png, .jpg, or .jpeg extension for explicit output)")As per coding guidelines, "Add
defer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/cast/recording.go` around lines 37 - 43, RegisterRecordingFlag is missing the required perf tracking call. Add a deferred perf.Track invocation at the start of RegisterRecordingFlag, using the appropriate atmosConfig and the function’s fully qualified name, and keep the blank line after it to match the file’s public-function pattern. Make sure the change is applied in RegisterRecordingFlag alongside the existing flag setup and NoOptDefVal logic.Source: Coding guidelines
pkg/asciicast/exec.go (1)
105-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify with
strings.Cut.Manual byte-scanning for
=can be replaced with the stdlib helper for clarity.♻️ Proposed simplification
func envMap(env []string) map[string]string { result := make(map[string]string, len(env)) for _, pair := range env { - for i := 0; i < len(pair); i++ { - if pair[i] == '=' { - result[pair[:i]] = pair[i+1:] - break - } - } + if key, value, ok := strings.Cut(pair, "="); ok { + result[key] = value + } } return result }🤖 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/asciicast/exec.go` around lines 105 - 116, Simplify envMap by replacing the manual byte scan for the first '=' with strings.Cut, and keep the existing behavior of only adding entries when a separator is present. Update the envMap function in exec.go to use the standard library helper for clearer parsing of each env pair.pkg/runner/step/shell.go (1)
199-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
resolveEnvwith inconsistent error wrapping vsscript.go.This method is near-identical to
ScriptHandler.resolveEnvinpkg/runner/step/script.go(lines 137-157), but uses a barefmt.Errorf("step '%s': %w", ...)here versuserrUtils.Build(errUtils.ErrTemplateEvaluation).WithCause(err).WithContext(...)there. Consider extracting a shared helper (e.g. onVariablesor a common step util) to eliminate the duplication and standardize on the static-error builder pattern used elsewhere in this package.♻️ Proposed shared helper
-func (h *ShellHandler) resolveEnv(step *schema.WorkflowStep, vars *Variables) ([]string, error) { - env := vars.EnvSlice() - if len(env) == 0 { - env = os.Environ() - } - if len(step.Env) == 0 { - return env, nil - } - resolvedEnv, err := vars.ResolveEnvMap(step.Env) - if err != nil { - return nil, fmt.Errorf("step '%s': %w", step.Name, err) - } - for key, value := range resolvedEnv { - env = envpkg.UpdateEnvVar(env, key, value) - } - return env, nil -} +func (h *ShellHandler) resolveEnv(step *schema.WorkflowStep, vars *Variables) ([]string, error) { + return resolveStepEnv(step, vars) +}Then define
resolveStepEnvonce (e.g. invariables.go) using theerrUtils.Build(errUtils.ErrTemplateEvaluation)pattern, and haveScriptHandler.resolveEnvcall it too.🤖 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/shell.go` around lines 199 - 215, The env resolution logic is duplicated in ShellHandler.resolveEnv and ScriptHandler.resolveEnv, and the error wrapping is inconsistent. Extract a shared helper for step env resolution (for example on Variables or a common step util) and have both resolveEnv methods call it. Make sure the shared helper uses the same errUtils.Build(errUtils.ErrTemplateEvaluation).WithCause(...).WithContext(...) pattern already used in ScriptHandler.resolveEnv so error handling is standardized.pkg/schema/command.go (1)
68-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDrop the custom
Is(); it's redundant and string-matching is fragile.
commandEnvDecodeError{}is an empty, comparable struct, soerrors.Isalready matches it by identity once unwrapped from thefmt.Errorf("%w ...")chain — no customIs()needed. Matching bytarget.Error() == commandEnvDecodeFailedMessagealso risks a false positive if any unrelated error happens to carry the same text.♻️ Simplify to a plain sentinel error
-const commandEnvDecodeFailedMessage = "failed to decode command env" - -// ErrCommandEnvDecodeFailed is returned when command env map decoding fails. -var ErrCommandEnvDecodeFailed error = commandEnvDecodeError{} - -type commandEnvDecodeError struct{} - -func (commandEnvDecodeError) Error() string { - return commandEnvDecodeFailedMessage -} - -func (commandEnvDecodeError) Is(target error) bool { - return target != nil && target.Error() == commandEnvDecodeFailedMessage -} +// ErrCommandEnvDecodeFailed is returned when command env map decoding fails. +var ErrCommandEnvDecodeFailed = errors.New("failed to decode command env")🤖 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/schema/command.go` around lines 68 - 81, Simplify ErrCommandEnvDecodeFailed in commandEnvDecodeError by removing the custom Is method and keeping it as a plain comparable sentinel error. Update the error type in command.go so errors.Is still works via identity on commandEnvDecodeError{} without string-based matching, and keep the Error method returning commandEnvDecodeFailedMessage for the shared message.website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json (1)
1739-1742: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
modelacks the enum restriction present in the companion schema.
pkg/datafetcher/schema/atmos/manifest/1.0.jsonrestricts this same field toenum: ["steps", "session", "typed", "prompt"], but here it's an unrestricted string. Since this file is the public schema surfaced to editors/IDEs, users lose validation feedback for typos inmodethat the internal schema would otherwise catch.📝 Align mode enum with companion schema
"mode": { "type": "string", - "description": "Mode for cast and simulate steps. Simulate supports typed and prompt." + "enum": ["steps", "session", "typed", "prompt"], + "description": "Mode for cast and simulate steps. Cast supports steps and session; simulate supports typed and prompt." },🤖 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 `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json` around lines 1739 - 1742, The mode field in the Atmos manifest schema is too loose compared with the companion schema, so align the public schema’s mode definition in the atmost-manifest JSON with the restriction used in the manifest schema under pkg/datafetcher/schema/atmos/manifest/1.0.json. Update the mode property in the schema object to use the same enum values (steps, session, typed, prompt) while keeping the existing description, so editors and IDEs can validate typos consistently.
🤖 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 `@cmd/cast/cast.go`:
- Around line 30-62: The render command is defining its own flags directly and
bypassing the standard command flag parser wiring. Update renderCmd and its
associated GetFlagsBuilder implementation to use flags.NewStandardParser() for
these command-specific options, and move the existing gif/mp4/html/ascii/png/jpg
definitions into that parser so ENV/config precedence is handled consistently
with other commands.
In `@NOTICE`:
- Around line 1089-1092: The NOTICE file is stale because the new
golang.org/x/image entry was hand-edited instead of being generated from the
dependency graph. Regenerate the NOTICE content by running the existing notice
generation script, then commit the resulting NOTICE output so it stays
consistent with the pipeline’s source of truth.
In `@pkg/asciicast/cellgrid.go`:
- Around line 67-98: Guard against non-advancing parses in sanitizeStream:
ansi.DecodeSequence can return n <= 0 on malformed or incomplete input, so the
content loop may never progress. Update sanitizeStream to defensively handle
this case by ensuring the input is always advanced or the loop exits, while
preserving the existing handling for width, newline, carriage return, tab, and
keepSequence.
In `@pkg/asciicast/session_windows.go`:
- Around line 17-30: The `session_windows.go` startup flow is returning raw
errors from `cmd.StdinPipe()` and `cmd.Start()`, which violates the static-error
policy. Update the Windows session setup path to wrap both failure cases with
the appropriate predefined error from `errors/errors.go` (or the existing static
context used in this package) before returning, keeping the cleanup logic intact
around `cmd.Start()` failures.
In `@pkg/datafetcher/schema/config/global/1.0.json`:
- Around line 1383-1412: The schema block in the global config has drifted from
the website schema, specifically around the cast/simulate step properties where
the website version still includes write_rate. Update the corresponding object
in the global schema to match the source of truth used by
website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json, keeping the
same property set and structure around prompt, mode, rate, and text. Use the
nearby schema section for the cast/simulate step to locate and synchronize the
block exactly so both schemas stay identical.
In `@pkg/process/script.go`:
- Around line 63-74: `RunScript` returns `cmd.Run()` directly, so the execution
failure is not wrapped per the error-handling guidelines. Update `RunScript` to
wrap the error from `cmd.Run()` with the appropriate static error from
`errors/errors.go`, preserving the underlying error as context; keep the
existing `NewScriptCommand` flow and only change the return path in `RunScript`.
- Around line 26-42: Update ScriptInvocation so Windows interpreters are handled
explicitly instead of falling back to the stdin form; the current default in
ScriptInvocation only suits stdin-friendly shells. Add cases for pwsh and
powershell to use their command-based invocation, and handle cmd/cmd.exe with a
proper command execution path rather than returning interp with stdin. Keep the
existing behavior for python/node/bash-family cases, and use the interpreter
name normalization already done in name and interp to route the new Windows
branches.
In `@pkg/runner/step/cast_simulate.go`:
- Around line 365-371: The castStepPauseDelay helper is swallowing malformed
Interval values by falling back to defaultCastStepPauseDelay, unlike the
explicit validation done for Rate in validateCastSimulateStep. Update the
validation flow around castStepPauseDelay/parseDurationDefault so an invalid
child.Interval is surfaced as a validation error instead of being silently
replaced with the default, keeping the handling consistent with Rate.
- Around line 231-279: `renderCastStyledText` and `forceCastColorProfile`
currently mutate the process-wide color profile, which can leak into other
goroutines despite `castStyleMu`. Update the cast rendering path to use a
renderer bound to the writer/profile instead of calling `ui.SetColorProfile` and
restoring it, leveraging `ui.NewRenderer` for per-render output. Keep the
existing style selection logic in `renderCastStyledText`, but remove the global
profile toggle so ANSI rendering stays isolated and concurrency-safe.
---
Outside diff comments:
In `@cmd/cmd_utils.go`:
- Around line 839-1147: Add a regression test around the custom command step
loop in cmd_utils.go, focusing on the logic that aggregates failures into
commandErr and uses errUtils.CheckErrorPrintAndExit at the end. Cover a
multi-step custom command where one step fails and later steps still run, then
verify the joined error is reported correctly. Also add a case where the failing
step returns an ExitCodeError so the final chosen exit code from the aggregated
error matches the expected behavior.
In `@cmd/root.go`:
- Around line 2177-2258: The help rendering paths are replacing the cast tee, so
`--cast` stops capturing output when `command.SetOut(&buf)` is used in the
`isFlagHelp` pager branch and the `isInteractiveHelp` branch. Hoist the
`recordWriter` from `castcmd.StartHelpRecording` so it’s available in the later
help-rendering branches, and when buffering help output, tee the buffer writes
through the same writer before calling `command.Help()`. Keep the existing
`command.SetOut(io.MultiWriter(...))` behavior for direct rendering, and make
sure the pager path still forwards the captured help text to both the buffer and
the cast recording.
In `@pkg/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 2021-2257: The workflow step schema is missing support for the
Atmos workflow step `output` object, so valid `output:` configurations are
rejected. Update the step schema in
`pkg/datafetcher/schema/atmos/manifest/1.0.json` alongside the existing
`outputs` property to add `workflow_step.output` with the correct object shape
and validation, using the same step definition block that already defines fields
like `type`, `with`, and `outputs`.
In `@pkg/io/streams.go`:
- Around line 126-146: The Write methods in maskedWriter and dynamicMaskedWriter
are returning 0 on error or short-write even after some bytes were successfully
written and recorded, which can cause duplicate writes on retries. Update the
Write logic so it returns the actual written count whenever progress was made,
or defer recordOutput until the full write succeeds. Keep the behavior aligned
with the existing maskedBytes/write handling in maskedWriter.Write and
dynamicMaskedWriter.Write.
In `@pkg/schema/task_validate.go`:
- Around line 45-105: ValidateExecTasks and ValidateExecWorkflowSteps only
enforce exec-step rules, but validateConcurrentChild is missing the script-step
checks for nested parallel/matrix children. Update the concurrent-child
validation path to also apply the same script validation used by
validateScriptSteps, so type: script children are rejected for missing
interpreter/script or for setting command. Use the existing
execStepView/scriptStepView flow and the
ValidateExecWorkflowSteps/validateConcurrentChild symbols to keep the logic
consistent.
---
Nitpick comments:
In `@cmd/cast/recording.go`:
- Around line 37-43: RegisterRecordingFlag is missing the required perf tracking
call. Add a deferred perf.Track invocation at the start of
RegisterRecordingFlag, using the appropriate atmosConfig and the function’s
fully qualified name, and keep the blank line after it to match the file’s
public-function pattern. Make sure the change is applied in
RegisterRecordingFlag alongside the existing flag setup and NoOptDefVal logic.
In `@errors/errors.go`:
- Around line 6-7: The alias direction for ErrCommandEnvDecodeFailed is inverted
compared to the rest of the PR, with errors/errors.go currently importing
pkg/schema instead of being the canonical source. Move the sentinel definition
into the errors package and update pkg/schema to alias from errUtils, matching
the existing pattern used by asciicast and step so the low-level errors package
remains the leaf dependency.
In `@pkg/asciicast/exec.go`:
- Around line 105-116: Simplify envMap by replacing the manual byte scan for the
first '=' with strings.Cut, and keep the existing behavior of only adding
entries when a separator is present. Update the envMap function in exec.go to
use the standard library helper for clearer parsing of each env pair.
In `@pkg/asciicast/recorder.go`:
- Around line 460-480: Replace the manual /dev/urandom handling in RandomID with
crypto/rand.Read so the identifier generation works consistently across Linux,
macOS, and Windows. Update RandomID to read n bytes directly from crypto/rand,
then map those bytes to the existing hex alphabet as before, and keep the
time.Now().UnixNano() fallback only as a last-resort error path if needed. Make
sure the fix stays localized to RandomID and preserves its current callers such
as createCastTempFile.
In `@pkg/asciicast/session.go`:
- Around line 261-285: The teardown timeout in finishSession is hardcoded and
duplicates the existing defaultTeardownMaxWait constant. Update the timeout
branch in finishSession to use defaultTeardownMaxWait instead of an inline
duration so the value stays centralized and cannot drift.
In `@pkg/datafetcher/schema/config/global/1.0.json`:
- Around line 1401-1404: The mode field is currently a free-form string even
though its description implies a fixed set of values, so update the schema for
the relevant config object to constrain mode with an enum like style does. Use
the existing mode property in the global config schema and add the supported
values from the description so workflow YAML typos are caught during validation.
In `@pkg/datafetcher/schema/stacks/stack-config/1.0.json`:
- Around line 2083-2086: Constrain the schema field `mode` in the stack config
JSON to the allowed values only. Update the `mode` definition under the
cast/simulate step schema to use an `enum` with the supported values described
in the `description` (`typed` and `prompt`) so validation catches typos instead
of accepting any string.
In `@pkg/env/env.go`:
- Around line 27-34: Chdir currently returns raw os.Chdir and os.Setenv errors
without operation context, so wrap each failure with the repo’s standard error
style. Update env.Chdir to keep the existing perf.Track call but return
contextual wrapped errors for the os.Chdir and os.Setenv paths, using fmt.Errorf
with %w and the static error conventions from errors/errors.go so callers can
see which chdir step failed.
In `@pkg/provisioner/source/vendor.go`:
- Around line 97-111: The existing-target/replace branching is duplicated in
vendor provisioning across the go-getter flow and the new local-copy helpers.
Refactor the go-getter path in vendor.go to reuse the same prepareVendorTarget
and handleExistingVendorTarget helpers used by the local-copy path, so the
pre-download check and post-download removal follow one shared implementation.
Keep the behavior for replaceTarget, target existence checks, and cleanup
identical by routing both code paths through those shared helpers.
In `@pkg/runner/step/cast.go`:
- Around line 485-497: The parseDurationDefault helper in cast.go has a
redundant special-case for the literal "0"; time.ParseDuration already returns
zero without an extra branch. Remove the explicit value == "0" check and rely on
the existing time.ParseDuration call, keeping the fallback-empty-string behavior
and the existing error wrapping intact.
- Line 142: The human-facing status output in cast.go is being written with raw
fmt.Fprintf on the UI writer, which bypasses the ui formatting layer. Update the
message emission in the cast recording flow to use the appropriate ui.* helper
(for example in the code around the Cast step and the recording-failed path) so
theming, TTY fallback, and masking are handled consistently; also replace the
other matching fmt.Fprintf call in this block with the same ui.* pattern.
In `@pkg/runner/step/shell.go`:
- Around line 199-215: The env resolution logic is duplicated in
ShellHandler.resolveEnv and ScriptHandler.resolveEnv, and the error wrapping is
inconsistent. Extract a shared helper for step env resolution (for example on
Variables or a common step util) and have both resolveEnv methods call it. Make
sure the shared helper uses the same
errUtils.Build(errUtils.ErrTemplateEvaluation).WithCause(...).WithContext(...)
pattern already used in ScriptHandler.resolveEnv so error handling is
standardized.
In `@pkg/schema/command.go`:
- Around line 68-81: Simplify ErrCommandEnvDecodeFailed in commandEnvDecodeError
by removing the custom Is method and keeping it as a plain comparable sentinel
error. Update the error type in command.go so errors.Is still works via identity
on commandEnvDecodeError{} without string-based matching, and keep the Error
method returning commandEnvDecodeFailedMessage for the shared message.
In `@pkg/schema/task_test.go`:
- Around line 329-393: The round-trip coverage in
TestTaskWorkflowStepControlFieldsRoundTrip is incomplete for the new
cast-related fields. Update the Task literal and the post-conversion assertions
in this test to include the missing fields handled by Task.ToWorkflowStep and
TaskFromWorkflowStep, especially Mode, Shell, WriteRate, KeyInterval, Jitter,
Cursor, CursorSet, Text, Regex, Key, Duration, Interval, Repeat, Defaults,
Source, Reset, and CastOutput, so a regression in either conversion will be
caught.
In `@pkg/schema/task.go`:
- Around line 810-881: The structured-prompt normalization logic is duplicated
in normalizeTaskPromptMap and normalizeWorkflowStepMap, with both enforcing the
same “prompt only valid for type simulate” rule. Extract a shared helper used by
both paths to perform the type check and prompt normalization consistently, and
make normalizeWorkflowStepMap and normalizeTaskPromptMap delegate to it so the
behavior for prompt renaming/deletion and struct decoding stays aligned.
In `@pkg/terminal/pacing_writer.go`:
- Around line 34-36: Add perf instrumentation to the new public HasRealTTYInput
function. Since it performs a terminal/fd check and is not a pure lookup, wrap
the function body with a deferred perf.Track call using the appropriate
atmosConfig and the pkg.HasRealTTYInput symbol, following the same pattern used
by ui.TerminalWidth().
In `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 1739-1742: The mode field in the Atmos manifest schema is too
loose compared with the companion schema, so align the public schema’s mode
definition in the atmost-manifest JSON with the restriction used in the manifest
schema under pkg/datafetcher/schema/atmos/manifest/1.0.json. Update the mode
property in the schema object to use the same enum values (steps, session,
typed, prompt) while keeping the existing description, so editors and IDEs can
validate typos consistently.
🪄 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: e560d381-d128-4f6f-aada-30898ba5fb21
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (101)
.gitattributesNOTICEcmd/cast/cast.gocmd/cast/recording.gocmd/cast/recording_test.gocmd/cast_flag_test.gocmd/cmd_utils.gocmd/env/env.gocmd/help_template.gocmd/help_template_test.gocmd/packer.gocmd/root.gocmd/root_helpers_test.goerrors/errors.gogo.modinternal/exec/shell_utils.gointernal/exec/shell_utils_test.gopkg/asciicast/cellgrid.gopkg/asciicast/cellgrid_test.gopkg/asciicast/exec.gopkg/asciicast/exec_test.gopkg/asciicast/recorder.gopkg/asciicast/recorder_test.gopkg/asciicast/render.gopkg/asciicast/render_ascii.gopkg/asciicast/render_html.gopkg/asciicast/render_image.gopkg/asciicast/render_static_test.gopkg/asciicast/render_test.gopkg/asciicast/session.gopkg/asciicast/session_test.gopkg/asciicast/session_unix.gopkg/asciicast/session_windows.gopkg/asciicast/testmain_test.gopkg/config/atmos_decode_hook_test.gopkg/config/command_include_env_test.gopkg/config/command_merge_core_test.gopkg/config/config_merge_test.gopkg/config/default.gopkg/config/import_commands_test.gopkg/config/load.gopkg/config/load_command_env_test.gopkg/config/load_config_args_test.gopkg/config/load_error_paths_test.gopkg/config/load_test.gopkg/config/process_yaml.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/datafetcher/schema_condition_validation_test.gopkg/env/env.gopkg/env/env_test.gopkg/env/output.gopkg/env/output_test.gopkg/io/context.gopkg/io/global.gopkg/io/interfaces.gopkg/io/masker.gopkg/io/reconcile_masking_test.gopkg/io/recorder.gopkg/io/recorder_test.gopkg/io/streams.gopkg/process/script.gopkg/process/script_test.gopkg/provisioner/source/vendor.gopkg/provisioner/source/vendor_test.gopkg/runner/step/atmos.gopkg/runner/step/cast.gopkg/runner/step/cast_simulate.gopkg/runner/step/cast_test.gopkg/runner/step/command_handlers_test.gopkg/runner/step/output_mode.gopkg/runner/step/output_mode_execution_test.gopkg/runner/step/script.gopkg/runner/step/script_test.gopkg/runner/step/shell.gopkg/runner/step/shell_test.gopkg/runner/step/show_config.gopkg/runner/step/show_config_test.gopkg/runner/step/variables.gopkg/runner/step/variables_test.gopkg/runner/step/workdir.gopkg/runner/step/workdir_test.gopkg/schema/command.gopkg/schema/command_test.gopkg/schema/schema.gopkg/schema/task.gopkg/schema/task_test.gopkg/schema/task_validate.gopkg/schema/task_validate_test.gopkg/schema/workflow.gopkg/schema/workflow_control_test.gopkg/terminal/pacing_writer.gopkg/terminal/pty/pty.gopkg/terminal/pty/pty_test.gopkg/terminal/terminal.gopkg/terminal/terminal_test.gopkg/ui/formatter.gopkg/ui/formatter_test.gopkg/utils/shell_utils.gowebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
💤 Files with no reviewable changes (1)
- cmd/help_template_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.atmos.d/build.yaml (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider anchoring the repeated
working_directoryvalue.Same
!repo-root .value is repeated five times; a YAML anchor (like*build_env) would avoid drift if the tag ever changes.Also applies to: 86-86, 95-95, 114-114, 145-145
🤖 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 @.atmos.d/build.yaml at line 5, The repeated working_directory value in the build configuration should be deduplicated by introducing a YAML anchor for the shared !repo-root . value and reusing it across the repeated entries. Update the relevant working_directory fields in the build YAML so they reference the anchor consistently instead of repeating the literal tag, keeping the existing build settings in sync and reducing drift risk.
🤖 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/runner.go`:
- Around line 265-279: Move the ValidateStepCondition check out of the per-task
execution loop in runner.go so all task.When conditions are validated up front
before any Run calls happen. Use the existing ValidateExecTasks pattern as a
guide, and ensure the runner’s task-processing logic only evaluates and executes
tasks after a full preflight validation pass over the tasks slice.
- Around line 283-306: Resolve templated task env values before building the
condition context in taskConditionContext. The current merge of task.Env into
ConditionContext.Env copies raw template strings, so any when expression sees
unrendered values. Update taskConditionContext (and any helper it uses) to
evaluate task.Env templates against the current context before assigning into
env, while preserving the existing opts.Env and task.Stack/stepName behavior.
---
Nitpick comments:
In @.atmos.d/build.yaml:
- Line 5: The repeated working_directory value in the build configuration should
be deduplicated by introducing a YAML anchor for the shared !repo-root . value
and reusing it across the repeated entries. Update the relevant
working_directory fields in the build YAML so they reference the anchor
consistently instead of repeating the literal tag, keeping the existing build
settings in sync and reducing drift risk.
🪄 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: 49b5202e-c708-44ff-9a75-b44f7b58c017
📒 Files selected for processing (11)
.atmos.d/build.yaml.atmos.d/dev.yamlcmd/cmd_utils.gopkg/runner/runner.gopkg/runner/runner_test.gopkg/runner/step/hint.gopkg/runner/step/hint_test.gopkg/runner/step/registry_test.gopkg/runner/step/ui_handlers_test.gowebsite/docs/workflows/workflows/workflow/steps/type.mdxwebsite/docs/workflows/workflows/workflow/steps/type/hint.mdx
✅ Files skipped from review due to trivial changes (3)
- website/docs/workflows/workflows/workflow/steps/type.mdx
- website/docs/workflows/workflows/workflow/steps/type/hint.mdx
- pkg/runner/step/registry_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cmd_utils.go
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
…s-changes # Conflicts: # cmd/cmd_utils.go
|
Warning Release Documentation RequiredThis PR is labeled
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
pkg/runner/step/script_test.go (1)
119-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate into a table-driven test; add missing
working_directorycase.
TestScriptHandlerResolveInvocationInterpreterTemplateErrorandTestScriptHandlerResolveInvocationScriptTemplateErrorare identical except for which field is templated and the expectedfieldcontext value. Per coding guidelines, these scenarios should be table-driven. While consolidating, also add the untestedworking_directorytemplate-error branch (script.go resolveInvocation, lines ~123-131) for full coverage of all three error-context branches.♻️ Suggested table-driven consolidation
-func TestScriptHandlerResolveInvocationInterpreterTemplateError(t *testing.T) { - handler := &ScriptHandler{} - vars := NewVariables() - - _, err := handler.resolveInvocation(&schema.WorkflowStep{ - Name: "bad-interpreter", - Interpreter: "{{ range .steps }}", - Script: "print('ok')", - }, vars) - require.ErrorIs(t, err, errUtils.ErrTemplateEvaluation) - stepName, ok := errUtils.GetContext(err, "step") - require.True(t, ok) - assert.Equal(t, "bad-interpreter", stepName) - field, ok := errUtils.GetContext(err, "field") - require.True(t, ok) - assert.Equal(t, "interpreter", field) -} - -func TestScriptHandlerResolveInvocationScriptTemplateError(t *testing.T) { - handler := &ScriptHandler{} - vars := NewVariables() - - _, err := handler.resolveInvocation(&schema.WorkflowStep{ - Name: "bad-script", - Interpreter: "python3", - Script: "{{ range .steps }}", - }, vars) - require.ErrorIs(t, err, errUtils.ErrTemplateEvaluation) - stepName, ok := errUtils.GetContext(err, "step") - require.True(t, ok) - assert.Equal(t, "bad-script", stepName) - field, ok := errUtils.GetContext(err, "field") - require.True(t, ok) - assert.Equal(t, "script", field) -} +func TestScriptHandlerResolveInvocationTemplateErrors(t *testing.T) { + tests := []struct { + name string + stepName string + interpreter string + script string + workDir string + wantField string + }{ + {"interpreter", "bad-interpreter", "{{ range .steps }}", "print('ok')", "", "interpreter"}, + {"script", "bad-script", "python3", "{{ range .steps }}", "", "script"}, + {"working_directory", "bad-workdir", "python3", "print('ok')", "{{ range .steps }}", "working_directory"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + handler := &ScriptHandler{} + _, err := handler.resolveInvocation(&schema.WorkflowStep{ + Name: tt.stepName, + Interpreter: tt.interpreter, + Script: tt.script, + WorkingDirectory: tt.workDir, + }, NewVariables()) + require.ErrorIs(t, err, errUtils.ErrTemplateEvaluation) + stepName, ok := errUtils.GetContext(err, "step") + require.True(t, ok) + assert.Equal(t, tt.stepName, stepName) + field, ok := errUtils.GetContext(err, "field") + require.True(t, ok) + assert.Equal(t, tt.wantField, field) + }) + } +}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/script_test.go` around lines 119 - 153, Consolidate the two nearly identical ScriptHandler resolveInvocation template error tests into a single table-driven test covering the templated field and expected field context. Use the existing resolveInvocation, errUtils.ErrTemplateEvaluation, and errUtils.GetContext checks as the common assertions, and add a third case for the working_directory template-error branch to cover all error-context paths. Keep the step name and expected field value in each table row so the behavior remains explicit.Source: Coding guidelines
pkg/runner/step/workdir_test.go (1)
211-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating into a table-driven test.
TestResolveStringSourceMapPropagatesNestedError,TestResolveAnySourceMapPropagatesNestedError, andTestResolveSourceSlicePropagatesNestedErroreach assert the same thing (nested template error propagates) against a different container type. These are natural table-driven candidates.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/workdir_test.go` around lines 211 - 237, Consolidate the three nested-error tests into one table-driven test that covers the map[string]any, map[any]any, and []any cases. Reuse a single test function in workdir_test.go to iterate over the different inputs and call the appropriate resolver function(s) such as resolveStringSourceMap, resolveAnySourceMap, and resolveSourceSlice, asserting require.Error for each scenario.Source: Coding guidelines
pkg/runner/step/testmain_test.go (1)
54-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
errors.Isfor the EOF check.
err == io.EOFworks today sincebufio.ReadBytereturns the unwrapped sentinel, buterrors.Is(err, io.EOF)is the more defensive/idiomatic form and matches the general error-handling guideline.♻️ Optional tweak
- if err == io.EOF { + if errors.Is(err, io.EOF) { return }🤖 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/testmain_test.go` around lines 54 - 60, The EOF handling in the reader loop should use the idiomatic, defensive error check instead of a direct sentinel comparison. Update the `reader.ReadByte` error branch in `testmain_test.go` to use `errors.Is(err, io.EOF)` so the behavior stays correct even if the error is wrapped, while keeping the existing `os.Exit(1)` path for non-EOF errors.Source: Coding guidelines
pkg/asciicast/session_test.go (2)
799-839: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a prerequisite sub-test for env→PTY propagation.
TestRunSessionAppliesDirectoryAndEnvironmentexercises directory + env-map propagation together through a full scripted PTY session. As per path instructions, tests depending on implicit env propagation into a subprocess should have "an explicit sub-test that confirms the behavior before the main test runs" — a minimal case isolating just env-var visibility would make failures easier to diagnose and guard against future regressions in isolation.🤖 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/asciicast/session_test.go` around lines 799 - 839, Add an explicit prerequisite sub-test in TestRunSessionAppliesDirectoryAndEnvironment that isolates env-to-PTY propagation before the full scripted session runs, so failures can be diagnosed independently of directory handling. Use the existing RunSession, SessionOptions, and Env setup to create a minimal check that only verifies the spawned shell sees ATMOS_CAST_SESSION_MARKER, then keep the current combined directory + environment assertions as the main test.Source: Path instructions
725-753: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMinor flake risk: timer assertions right after
Reset(1ms)with no margin.Both tests reset the timer to fire in 1ms then immediately probe the channel with a non-blocking
select. Under CI scheduling jitter, the timer could already have fired by the time theselectruns, flippingTestResetTimerStopsRunningTimer's "fired unexpectedly" assertion (false failure) or invalidating the drain check.🤖 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/asciicast/session_test.go` around lines 725 - 753, The timer checks in TestResetTimerDrainsAlreadyFiredTimer and TestResetTimerStopsRunningTimer are too timing-sensitive because they probe timer.C immediately after resetTimer(timer, time.Millisecond). Update these tests to avoid asserting on a near-immediate 1ms deadline; instead use a longer/reset-safe duration or a deterministic wait strategy so the behavior of resetTimer remains the focus and CI scheduling jitter cannot make the select-based assertions flaky.pkg/asciicast/render_image_test.go (1)
148-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract shared RGBA→8-bit helper.
The
eightBitShift/byteMask+uint8(x >> 8 & 0xff)triplet extraction is repeated across four tests. A small helper (e.g.to8(c color.Color) (r, g, b, a uint8)) would cut duplication.Also applies to: 183-213, 321-333, 355-383
🤖 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/asciicast/render_image_test.go` around lines 148 - 162, The RGBA-to-8-bit conversion logic is duplicated across the color tests, so extract it into a small shared helper in render_image_test.go (for example, a helper used by TestCellColorsFaintDimsForeground and the other RGBA assertions) and replace the repeated eightBitShift/byteMask and shift/mask expressions with calls to that helper. Keep the tests focused on their assertions and reuse the helper wherever the same channel extraction pattern appears.
🤖 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 `@cmd/env/env_test.go`:
- Around line 286-294: `TestStdoutIsCharDevice` is asserting only the static
type of `stdoutIsCharDevice`, which is tautological and does not verify
behavior. Update this test to check an actual runtime outcome from
`stdoutIsCharDevice()` (for example, assert the expected true/false result for
the real `os.Stdout` under test) and keep the coverage focused on the function’s
behavior rather than its return type.
In `@pkg/runner/step/cast_test.go`:
- Around line 1679-1694: The tests in runCastChildStep use Unix-only shell
commands/syntax, which breaks Windows CI. Update
TestRunCastChildStepReturnsPauseDelayError and the related shell-error test in
cast_test.go to avoid native binaries like "true" and shell one-liners such as
rm -f ...; exit 1. Reuse the existing cross-platform pattern already used in the
session-mode tests by invoking os.Executable() with the sessionShellHelperEnv
fake-shell dispatch, or another Go-native helper, so the test behavior stays the
same without depending on platform-specific shells.
---
Nitpick comments:
In `@pkg/asciicast/render_image_test.go`:
- Around line 148-162: The RGBA-to-8-bit conversion logic is duplicated across
the color tests, so extract it into a small shared helper in
render_image_test.go (for example, a helper used by
TestCellColorsFaintDimsForeground and the other RGBA assertions) and replace the
repeated eightBitShift/byteMask and shift/mask expressions with calls to that
helper. Keep the tests focused on their assertions and reuse the helper wherever
the same channel extraction pattern appears.
In `@pkg/asciicast/session_test.go`:
- Around line 799-839: Add an explicit prerequisite sub-test in
TestRunSessionAppliesDirectoryAndEnvironment that isolates env-to-PTY
propagation before the full scripted session runs, so failures can be diagnosed
independently of directory handling. Use the existing RunSession,
SessionOptions, and Env setup to create a minimal check that only verifies the
spawned shell sees ATMOS_CAST_SESSION_MARKER, then keep the current combined
directory + environment assertions as the main test.
- Around line 725-753: The timer checks in TestResetTimerDrainsAlreadyFiredTimer
and TestResetTimerStopsRunningTimer are too timing-sensitive because they probe
timer.C immediately after resetTimer(timer, time.Millisecond). Update these
tests to avoid asserting on a near-immediate 1ms deadline; instead use a
longer/reset-safe duration or a deterministic wait strategy so the behavior of
resetTimer remains the focus and CI scheduling jitter cannot make the
select-based assertions flaky.
In `@pkg/runner/step/script_test.go`:
- Around line 119-153: Consolidate the two nearly identical ScriptHandler
resolveInvocation template error tests into a single table-driven test covering
the templated field and expected field context. Use the existing
resolveInvocation, errUtils.ErrTemplateEvaluation, and errUtils.GetContext
checks as the common assertions, and add a third case for the working_directory
template-error branch to cover all error-context paths. Keep the step name and
expected field value in each table row so the behavior remains explicit.
In `@pkg/runner/step/testmain_test.go`:
- Around line 54-60: The EOF handling in the reader loop should use the
idiomatic, defensive error check instead of a direct sentinel comparison. Update
the `reader.ReadByte` error branch in `testmain_test.go` to use `errors.Is(err,
io.EOF)` so the behavior stays correct even if the error is wrapped, while
keeping the existing `os.Exit(1)` path for non-EOF errors.
In `@pkg/runner/step/workdir_test.go`:
- Around line 211-237: Consolidate the three nested-error tests into one
table-driven test that covers the map[string]any, map[any]any, and []any cases.
Reuse a single test function in workdir_test.go to iterate over the different
inputs and call the appropriate resolver function(s) such as
resolveStringSourceMap, resolveAnySourceMap, and resolveSourceSlice, asserting
require.Error for each scenario.
🪄 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: 2745d760-7f6e-4bfe-a64f-2b4a387ffa78
📒 Files selected for processing (42)
.github/actions/setup-atmos-build/action.yml.github/workflows/native-ci.ymlcmd/cast/cast_test.gocmd/cast/recording_test.gocmd/env/env_test.gopkg/asciicast/cellgrid_test.gopkg/asciicast/exec_test.gopkg/asciicast/recorder.gopkg/asciicast/recorder_test.gopkg/asciicast/render_image_test.gopkg/asciicast/render_static_test.gopkg/asciicast/render_test.gopkg/asciicast/session_test.gopkg/config/command_merge_core_test.gopkg/config/process_yaml_test.gopkg/env/output_test.gopkg/flags/explicit_string_flag_test.gopkg/flags/global_registry.gopkg/flags/global_registry_test.gopkg/flags/types_test.gopkg/io/context_test.gopkg/io/global_test.gopkg/io/streams_test.gopkg/process/script_test.gopkg/provisioner/source/vendor_test.gopkg/runner/runner_test.gopkg/runner/step/cast_test.gopkg/runner/step/hint_test.gopkg/runner/step/script_test.gopkg/runner/step/shell_test.gopkg/runner/step/testmain_test.gopkg/runner/step/workdir_test.gopkg/schema/command_test.gopkg/schema/task_test.gopkg/schema/task_validate_test.gopkg/schema/workflow.gopkg/schema/workflow_container_test.gopkg/schema/workflow_control_test.gopkg/schema/workflow_with_test.gopkg/terminal/pacing_writer_test.gopkg/terminal/pty/pty_test.gopkg/terminal/terminal_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/asciicast/render_static_test.go
- pkg/asciicast/recorder.go
…s-changes # Conflicts: # tests/snapshots/TestCLICommands_atmos_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_--help_config_aliases_section.stdout.golden # tests/snapshots/TestCLICommands_atmos_about_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden # tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden # tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden # tests/snapshots/TestCLICommands_atmos_auth_env_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_auth_exec_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_auth_login_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_auth_user_configure_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_auth_validate_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_auth_whoami_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden # tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden # tests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden # tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden # tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden # tests/snapshots/TestCLICommands_atmos_toolchain_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_toolchain_info_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_toolchain_install_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden # tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden # tests/snapshots/TestCLICommands_config_alias_tp_--help_shows_terraform_plan_help.stdout.golden # tests/snapshots/TestCLICommands_config_alias_tr_--help_shows_terraform_help.stdout.golden # tests/snapshots/TestCLICommands_help_flag_works.stdout.golden
- cmd/env/env_test.go: replace tautological assert.IsType with
assert.NotPanics in TestStdoutIsCharDevice, since the function is
statically typed to return bool.
- pkg/runner/step/cast_test.go: replace Unix-only shell commands
("true", "rm -f ...; exit 1") in two tests with the existing
cross-platform fake-binary dispatch (_ATMOS_STEP_FAKE via
os.Executable()+TestMain), adding a new "rm-glob-and-fail" mode for
the discard-error test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three tests/production paths broke only on Windows (job 85509108939): - pkg/asciicast: TestDiscardReturnsRemoveErrWhenCloseErrIsNil removed the recorder's temp file while its file handle was still open. Unix allows unlinking an open file; Windows doesn't, so os.Remove itself failed before Discard() was ever exercised. Close the recorder's real handle and swap in a dummy file/writer first, matching how Discard() already closes before removing internally. - pkg/provisioner/source: VendorSource's pre-download target-directory check used os.IsNotExist to decide whether to fail fast. Unix reports ENOTDIR (distinct from ErrNotExist) when an ancestor path component is a file, but Windows reports the equivalent stat error as ErrNotExist, so the check silently fell through to a live download attempt instead of failing fast. Add targetPathBlockedByFile to detect a file-blocking-a-directory ancestor cross-platform. This also surfaced a real nil-pointer panic in pkg/downloader/custom_git_detector.go, which dereferenced a possibly-nil atmosConfig when checking token-injection settings - guard it, since VendorSource is called with a nil atmosConfig in several call sites. - pkg/runner/step: TestExecuteWithWorkflowJoinsDiscardError depends on a child subprocess deleting the recorder's still-open temp file. Windows can't delete a file that another handle (even in another process) has open without FILE_SHARE_DELETE, which Go's os.Create doesn't set by default, so the deletion silently no-ops and no discard error is ever joined. Skip on Windows with an explanatory comment; the scenario is only reproducible on POSIX. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RandomID opened /dev/urandom directly, which doesn't exist on Windows, silently falling back to a time.Now().UnixNano() seed. In a tight loop (TestRandomIDProducesDistinctHexIDs), Windows' coarser clock resolution returned the same timestamp across calls, producing identical IDs and failing the test. Use crypto/rand.Read instead, which is cross-platform and removes the need for the OS-specific /dev/urandom path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.223.0-rc.5. |
This branch was successfully deployed
2 participants
what
atmos cast play,atmos cast render, and global--castrecording for CLI output, including help output recording.cast,simulate,script, andworkdir, plus config defaults/loading behavior and focused tests.docs/fixes.scope
ascii-castbranch remains the source for the follow-up content work and can be rebased after this core PR merges.non-cast fix notes
docs/fixes/2026-07-06-custom-command-import-merge.mddocs/fixes/2026-07-06-custom-command-include-env.mddocs/fixes/2026-07-06-custom-command-env-map-form.mddocs/fixes/2026-07-06-custom-command-step-execution.mddocs/fixes/2026-07-06-terminal-color-and-force-tty.mddocs/fixes/2026-07-06-help-rendering-without-config.mddocs/fixes/2026-07-06-mask-shell-and-env-output.mddocs/fixes/2026-07-06-chdir-updates-pwd.mddocs/fixes/2026-07-06-local-vendor-source-paths.mddocs/fixes/2026-07-06-step-output-labels.mdwhy
ascii-castbefore the user-facing documentation and content work lands separately.validation
go test ./cmd/cast ./cmd ./pkg/config ./pkg/schema ./pkg/terminal ./pkg/ui -run 'Test|^$' -count=1COLUMNS=80 GIT_CONFIG_GLOBAL="$PWD/.context/gitconfig-test" go test ./tests -run 'TestCLICommands/(indentation|Invalid_Log_Level_in_Config_File|Invalid_Log_Level_in_Environment_Variable|secrets-masking_describe_config|atmos_toolchain_--help|atmos_toolchain_install_--help|atmos_toolchain_info_--help|atmos_validate_component_failure_with_UI_output|atmos_workflow_shell_command_not_found|atmos_workflow_failure|atmos_workflow_failure_on_shell_command|atmos_workflow_invalid_step_type|atmos_workflow_invalid_from_step|atmos_workflow_invalid_manifest)$' -count=1go test ./cmd/...go test ./pkg/asciicast ./pkg/io ./pkg/terminal/... ./pkg/runner/step ./pkg/process ./pkg/schema ./pkg/configgo test ./cmd/cast -count=1git diff --check -- docs/fixes