feat(workflows): VHS-dialect tape interpretation for type: cast steps - #2889
feat(workflows): VHS-dialect tape interpretation for type: cast steps#2889Erik Osterman (Cloud Posse) (osterman) wants to merge 7 commits into
Conversation
A type: cast workflow/custom-command step can now interpret VHS-dialect .tape scripts directly, in memory, via tape: (inline) or tape_file: (path) -- no vhs binary required, nothing written to disk. Directives translate into the step's existing mode: steps (real, exit-code-tracked children) or mode: session (PTY-driven keypress replay) machinery, with Hide/Show, Screenshot, Source, and Require all supported. A new `atmos cast record <input.tape> --output=<path>` CLI subcommand records a tape ad hoc with no workflow YAML at all. Adds the distributed atmos-vhs skill covering tape interpretation and hand-migration to native steps:. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds VHS ChangesVHS cast workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (11)
pkg/runner/step/cast_tape.go (1)
195-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA malformed
Set Width/Set Heightvalue is dropped without a signal.
applyTapeSetIntignores an unparsable value. Every other unsupportedSetkey logs a warning throughlogUnsupportedTapeSet. A warning here keeps the behavior consistent and helps a tape author find the typo.🤖 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_tape.go` around lines 195 - 205, The applyTapeSetInt function currently silently ignores malformed integer values; update it to log a warning through logUnsupportedTapeSet when strconv.Atoi fails, while preserving the existing behavior for already-set values and valid integers.pkg/runner/step/cast_tape_test.go (2)
16-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
captureTapeLogOutputrestoresos.Stderrrather than the previous writer.If another test or the package init points the logger somewhere else, this cleanup overwrites that destination. Capturing and restoring the prior writer is safer, if the logger exposes a getter.
🤖 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_tape_test.go` around lines 16 - 25, Update captureTapeLogOutput to save the logger’s current output writer before redirecting it to buf, then restore that saved writer in t.Cleanup instead of unconditionally using os.Stderr. Preserve the existing capture and cleanup timing around fn.
239-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the matrix in
t.Runsubtests.The nested loop over tapes and modes reports a single test name. A
t.Run(tape+"/"+mode, ...)subtest names the failing combination directly and matches the table-driven style used inTestExpandCastTapeStepsModeErrorsOnSessionOnlyDirectives.🤖 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_tape_test.go` around lines 239 - 250, Update TestExpandCastTapeCopyPasteEnvAlwaysError to wrap each tape/mode combination in a t.Run subtest named with the tape and mode, while keeping the existing step construction and error assertion inside the subtest.cmd/cast/record_test.go (2)
128-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests mutate shared
recordCmdand global Viper state.Each test calls
resetRecordCommandandclearRecordViperOverridesfirst, so the current set passes. The safety depends on every future test in this file doing the same. Moving both helpers into a singlesetupRecordTest(t)helper makes the precondition hard to skip.🤖 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/record_test.go` around lines 128 - 140, Introduce a shared setupRecordTest(t) helper that performs both resetRecordCommand(t) and clearRecordViperOverrides(t), then update TestRecordCmdRunEReturnsErrorForMissingOutput and the other record command tests to call it instead of invoking the helpers separately. Keep the existing test behavior unchanged.
167-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for
--formatandmode: steps.The current tests cover the raw
.castpath and the two error paths. No test drives--formatthroughrunRecordCommand, and none drivesmode: steps. Amode: stepstest with aType "..." Entertape would also coverplanRecordOutput's render branch end to end without a live PTY.🤖 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/record_test.go` around lines 167 - 180, Add tests in record_test.go that exercise runRecordCommand with the --format flag and with a steps-mode tape so the render path is covered end to end. Reuse the existing recordCmd, resetRecordCommand, and clearRecordViperOverrides setup, then add a case that passes a .cast output format through runRecordCommand and a case that uses mode: steps with a Type "..." Enter tape to drive planRecordOutput’s render branch without a live PTY.pkg/asciicast/session_actions.go (2)
53-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPacing loops in
session_actions.goignore context cancellation. Both action handlers pace their output with a baretime.Sleepand neither accepts acontext.Context, so a cancelled or timed-out session keeps writing to the PTY until the loop finishes.runPauseActionalready selects onctx.Done(), so the file is inconsistent with itself. Add onesleepCtx(ctx, d)helper and threadctxfromrunActioninto both handlers.
pkg/asciicast/session_actions.go#L53-L71: add actx context.Contextfirst parameter torunWriteActionand replace the per-runetime.Sleep(rate)withsleepCtx.pkg/asciicast/session_actions.go#L73-L95: add actx context.Contextfirst parameter torunKeyActionand replace the inter-keytime.Sleep(interval)withsleepCtx.As per coding guidelines: "Use
context.Contextonly for cancellation, deadlines/timeouts, ... and place it first in function parameters."🤖 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_actions.go` around lines 53 - 71, Pacing loops ignore context cancellation. In pkg/asciicast/session_actions.go lines 53-71, add ctx context.Context first to runWriteAction and replace time.Sleep(rate) with the shared sleepCtx helper; in lines 73-95, add ctx context.Context first to runKeyAction and replace time.Sleep(interval) with sleepCtx. Thread ctx from runAction into both handlers and implement sleepCtx to return promptly when ctx.Done() is signaled.Source: Coding guidelines
108-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse static sentinel errors instead of dynamic ones.
This file builds errors dynamically in several places:
"invalid pause duration %q"here, plus"invalid write rate %q"(line 58),"invalid key interval %q"(line 103), and the bareregexp.Compileerror (line 137). The coding guidelines require a static error fromerrors/errors.gowrapped with%w. Callers cannot match any of these witherrors.Is.♻️ Example for the pause duration
duration, err := time.ParseDuration(action.Duration) if err != nil { - return fmt.Errorf("invalid pause duration %q: %w", action.Duration, err) + return fmt.Errorf("%w: invalid pause duration %q: %w", errUtils.ErrInvalidSessionAction, action.Duration, err) }Reuse an existing sentinel if one fits, otherwise add one alongside
ErrScreenshotActionRequiresPath.As per coding guidelines: "Wrap all errors with static errors from
errors/errors.go; ... never use dynamic errors directly."🤖 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_actions.go` around lines 108 - 121, The session action validation errors in runPauseAction and the related write-rate, key-interval, and regexp compilation paths must use static sentinels from errors/errors.go so callers can match them with errors.Is. Reuse an existing sentinel where appropriate; otherwise define sentinels alongside ErrScreenshotActionRequiresPath, wrap them with %w, and preserve the existing contextual details in each error message.Source: Coding guidelines
pkg/asciicast/tape_parser.go (1)
143-150: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the Sleep duration while parsing.
consumeTapeSleepstores the raw token. An invalid value only fails later inrunPauseAction, after the PTY session has started, and the resulting error carries no file or line. Parsing it here letstapeErrreport the offending directive before anything is recorded.Two related notes:
- VHS accepts a bare number as seconds, for example
Sleep 1.time.ParseDuration("1")rejects that, so such a tape fails at run time.- A parse-time check also covers the
Setkeys that hold durations, if you want the same treatment there.♻️ Proposed change
func consumeTapeSleep(tokens []tapeToken, pos int, file string) (TapeDirective, int, error) { line := tokens[pos].Line duration, next, err := nextTapeArg(tokens, pos+1, file, "Sleep duration") if err != nil { return TapeDirective{}, 0, err } + if _, err := time.ParseDuration(duration); err != nil { + return TapeDirective{}, 0, tapeErr(file, line, fmt.Errorf("%w: invalid Sleep duration %q", ErrTapeParseFailed, duration)) + } return TapeDirective{Kind: TapeSleep, File: file, Line: line, Duration: duration}, next, 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/asciicast/tape_parser.go` around lines 143 - 150, Update consumeTapeSleep to validate the returned duration token during parsing, accepting bare numeric values as seconds before applying duration parsing. On invalid input, return a tapeErr associated with the directive’s file and line so parsing fails before the PTY session starts; apply the same duration validation to Set keys that store durations if they share the relevant parsing path.pkg/asciicast/tape_test.go (1)
429-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the fixture path with
filepath.Join.The test hardcodes
"testdata/legacy-demo.tape"with a forward slash. The coding guidelines requirefilepath.Joinfor paths in tests.♻️ Proposed change
func TestParseTapeLegacyDemoFixture(t *testing.T) { - tape, err := ParseTapeFile("testdata/legacy-demo.tape", "testdata", osTapeFileReader{}) + tape, err := ParseTapeFile(filepath.Join("testdata", "legacy-demo.tape"), "testdata", osTapeFileReader{}) require.NoError(t, err)Add
"path/filepath"to the imports.As per coding guidelines: "Use
filepath.Joinfor paths, avoid slash concatenation and Unix-specific expected paths".🤖 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/tape_test.go` around lines 429 - 432, Update TestParseTapeLegacyDemoFixture to import path/filepath and construct the fixture argument with filepath.Join("testdata", "legacy-demo.tape"), preserving the existing base directory argument and test assertions.Source: Coding guidelines
pkg/asciicast/tape.go (2)
75-88: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrop
perf.TrackfromError()andUnwrap().Both methods are trivial accessors. The coding guidelines exclude trivial accessors from
perf.Track.Error()also runs on every error formatting, so tracking adds noise to the perf report for no signal.♻️ Proposed cleanup
func (e *TapeError) Error() string { - defer perf.Track(nil, "asciicast.TapeError.Error")() - if e.File != "" { return fmt.Sprintf("%s:%d: %v", e.File, e.Line, e.Err) } return fmt.Sprintf("tape line %d: %v", e.Line, e.Err) } func (e *TapeError) Unwrap() error { - defer perf.Track(nil, "asciicast.TapeError.Unwrap")() - return e.Err }As per coding guidelines: "Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus a blank line to public functions, except trivial accessors, command constructors, simple factories, delegators, and pure validation/lookups."🤖 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/tape.go` around lines 75 - 88, Remove the defer perf.Track calls from TapeError.Error and TapeError.Unwrap, leaving both methods’ existing error formatting and unwrapping behavior unchanged.Source: Coding guidelines
325-332: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAlign quoted-string escape behavior with VHS or document the divergence.
scanTapeQuotedStringcurrently turns"\n"and"\t"into literalnandt, soType "a\nb"typesanb. VHS string tokens do not interpret standard backslash escapes, so either change this parser to match that behavior, or document that Atmos intentionally supports limited escapes such as control characters while raw strings keep backslashes literal.🤖 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/tape.go` around lines 325 - 332, Update scanTapeQuotedString’s backslash handling to match VHS string-token behavior: do not discard the backslash for unsupported escapes such as \n and \t, while preserving any intentionally supported control-character escapes; alternatively, explicitly document the intentional divergence and its limited-escape semantics near the parser.
🤖 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 `@agent-skills/skills/atmos-vhs/SKILL.md`:
- Around line 255-263: Document that cosmetic Set directives emit warnings and
are not applied, rather than being silently ignored. Update the category title
and explanation in agent-skills/skills/atmos-vhs/SKILL.md (lines 255-263) and
agent-skills/skills/atmos-vhs/references/vhs-directive-support.md (lines 43-67),
and revise the Set directive table in
website/docs/workflows/workflows/workflow/steps/type/cast.mdx (lines 121-131) to
describe the warning behavior consistently.
- Around line 85-88: Correct the mode: steps documentation: in
agent-skills/skills/atmos-vhs/SKILL.md lines 85-88, use session mode or a tape
without Sleep and Wait; in lines 96-112, include Sleep and Wait among parse-time
rejection criteria; in lines 222-226, describe removing them as manual migration
work rather than interpreter behavior; and in
website/docs/workflows/workflows/workflow/steps/type/cast.mdx lines 116-119,
state that Sleep and Wait fail in mode: steps.
In `@cmd/cast/record_test.go`:
- Around line 142-165: Update TestRecordCmdRunEExecutesTapeAndRecordsCast to
avoid the default session mode and its PTY-backed shell by configuring recordCmd
to use the steps mode before invoking RunE. Keep the test focused on verifying
the non-empty cast output, without introducing platform-specific binaries, shell
commands, or live shell dependencies.
In `@cmd/cast/record.go`:
- Around line 37-47: Update recordCmd to add comprehensive Long help and an
embedded usage example, following the sibling cast subcommands’ go:embed, Long,
and utils.PrintfMarkdown() pattern. Add the corresponding cast_record_usage.md
markdown resource and wire it into the command’s help output while preserving
the existing argument validation and runRecordCommand flow.
In `@pkg/asciicast/screenshot.go`:
- Around line 19-45: Update writeScreenshotPNG to write the PNG to a temporary
file in the target directory, close it successfully, then atomically rename it
to path; remove the temporary file on any failure. Wrap creation, encoding,
flushing/closing, and renaming failures with the static screenshot-render error
from errors/errors.go, adding operation context with %w.
In `@pkg/asciicast/tape.go`:
- Around line 244-263: Update step() so any '#' character starts skipComment(),
including after non-space tokens on the same line, matching tokenizeTape’s
documented behavior. Remove the lx.atLineStart condition while preserving the
existing handling for line-start whitespace and other token types.
- Around line 123-131: Update ParseTapeFile’s fr.ReadFile error handling to wrap
the failure with the existing ErrTapeSourceNotFound sentinel and the requested
path context using %w, preserving the original error for matching. Keep
successful reads flowing unchanged into ParseTape.
- Around line 257-258: Update tapeLexer regex handling around scanRegexToken so
a leading slash is lexed as a regex only when the preceding tokens permit it:
after Wait, Wait+Screen, Wait+Line, or a Set key; otherwise scan it as a bare
word for absolute paths. Add an Output /tmp/demo.gif case to TestTokenizeTape.
In `@pkg/runner/step/cast_tape_test.go`:
- Around line 188-190: Update the second shell-child assertion in the
tapeResolveCd test to construct the expected WorkingDirectory with
filepath.Join("examples", "quick-start", "nested") instead of a slash-delimited
literal, and add the filepath import if needed.
In `@pkg/runner/step/cast_tape.go`:
- Around line 484-487: Update the TapeHide branch in the tape-processing switch
so a repeated Hide preserves the existing hiddenBuffer instead of clearing it.
Keep setting hidden to true and returning the same result, ensuring actions
buffered before a second Hide remain available until Show.
In `@website/blog/2026-07-11-vhs-tape-interpretation.mdx`:
- Line 36: The mode comparison in
website/blog/2026-07-11-vhs-tape-interpretation.mdx#L36 must state that only
non-liftable Hide/Show blocks require mode: session; setup-only blocks
containing exclusively export, unset, cd, and clear work in mode: steps. Update
website/docs/workflows/workflows/workflow/steps/type/cast.mdx#L118-L130 to
remove the claim that every Hide/Show block fails in mode: steps, while
preserving the restriction for other unsupported commands.
In `@website/docs/cli/commands/cast/record.mdx`:
- Around line 8-12: Add a static CastPlayer component immediately after the
Intro in the record command documentation, using the page’s appropriate
committed terminal cast example. Do not add the legacy Screengrab component, and
preserve the existing Intro content.
---
Nitpick comments:
In `@cmd/cast/record_test.go`:
- Around line 128-140: Introduce a shared setupRecordTest(t) helper that
performs both resetRecordCommand(t) and clearRecordViperOverrides(t), then
update TestRecordCmdRunEReturnsErrorForMissingOutput and the other record
command tests to call it instead of invoking the helpers separately. Keep the
existing test behavior unchanged.
- Around line 167-180: Add tests in record_test.go that exercise
runRecordCommand with the --format flag and with a steps-mode tape so the render
path is covered end to end. Reuse the existing recordCmd, resetRecordCommand,
and clearRecordViperOverrides setup, then add a case that passes a .cast output
format through runRecordCommand and a case that uses mode: steps with a Type
"..." Enter tape to drive planRecordOutput’s render branch without a live PTY.
In `@pkg/asciicast/session_actions.go`:
- Around line 53-71: Pacing loops ignore context cancellation. In
pkg/asciicast/session_actions.go lines 53-71, add ctx context.Context first to
runWriteAction and replace time.Sleep(rate) with the shared sleepCtx helper; in
lines 73-95, add ctx context.Context first to runKeyAction and replace
time.Sleep(interval) with sleepCtx. Thread ctx from runAction into both handlers
and implement sleepCtx to return promptly when ctx.Done() is signaled.
- Around line 108-121: The session action validation errors in runPauseAction
and the related write-rate, key-interval, and regexp compilation paths must use
static sentinels from errors/errors.go so callers can match them with errors.Is.
Reuse an existing sentinel where appropriate; otherwise define sentinels
alongside ErrScreenshotActionRequiresPath, wrap them with %w, and preserve the
existing contextual details in each error message.
In `@pkg/asciicast/tape_parser.go`:
- Around line 143-150: Update consumeTapeSleep to validate the returned duration
token during parsing, accepting bare numeric values as seconds before applying
duration parsing. On invalid input, return a tapeErr associated with the
directive’s file and line so parsing fails before the PTY session starts; apply
the same duration validation to Set keys that store durations if they share the
relevant parsing path.
In `@pkg/asciicast/tape_test.go`:
- Around line 429-432: Update TestParseTapeLegacyDemoFixture to import
path/filepath and construct the fixture argument with filepath.Join("testdata",
"legacy-demo.tape"), preserving the existing base directory argument and test
assertions.
In `@pkg/asciicast/tape.go`:
- Around line 75-88: Remove the defer perf.Track calls from TapeError.Error and
TapeError.Unwrap, leaving both methods’ existing error formatting and unwrapping
behavior unchanged.
- Around line 325-332: Update scanTapeQuotedString’s backslash handling to match
VHS string-token behavior: do not discard the backslash for unsupported escapes
such as \n and \t, while preserving any intentionally supported
control-character escapes; alternatively, explicitly document the intentional
divergence and its limited-escape semantics near the parser.
In `@pkg/runner/step/cast_tape_test.go`:
- Around line 16-25: Update captureTapeLogOutput to save the logger’s current
output writer before redirecting it to buf, then restore that saved writer in
t.Cleanup instead of unconditionally using os.Stderr. Preserve the existing
capture and cleanup timing around fn.
- Around line 239-250: Update TestExpandCastTapeCopyPasteEnvAlwaysError to wrap
each tape/mode combination in a t.Run subtest named with the tape and mode,
while keeping the existing step construction and error assertion inside the
subtest.
In `@pkg/runner/step/cast_tape.go`:
- Around line 195-205: The applyTapeSetInt function currently silently ignores
malformed integer values; update it to log a warning through
logUnsupportedTapeSet when strconv.Atoi fails, while preserving the existing
behavior for already-set values and valid integers.
🪄 Autofix
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 Plus
Run ID: c7661bcc-8ee3-4003-9272-3cb9c9868535
📒 Files selected for processing (33)
.claude/skills/atmos-vhsagent-skills/skills/atmos-vhs/SKILL.mdagent-skills/skills/atmos-vhs/references/vhs-directive-support.mdcmd/cast/record.gocmd/cast/record_test.goerrors/errors.gopkg/asciicast/cellgrid.gopkg/asciicast/cellgrid_test.gopkg/asciicast/recorder.gopkg/asciicast/screenshot.gopkg/asciicast/screenshot_test.gopkg/asciicast/session.gopkg/asciicast/session_actions.gopkg/asciicast/session_test.gopkg/asciicast/tape.gopkg/asciicast/tape_parser.gopkg/asciicast/tape_test.gopkg/asciicast/testdata/legacy-demo.tapepkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/config/global/1.0.jsonpkg/io/recorder.gopkg/runner/step/cast.gopkg/runner/step/cast_tape.gopkg/runner/step/cast_tape_test.gopkg/schema/task.gopkg/schema/workflow.gowebsite/blog/2026-07-11-vhs-tape-interpretation.mdxwebsite/docs/cli/commands/cast/play.mdxwebsite/docs/cli/commands/cast/record.mdxwebsite/docs/cli/commands/cast/render.mdxwebsite/docs/workflows/workflows/workflow/steps/type/cast.mdxwebsite/src/data/roadmap.jswebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
| ```shell | ||
| atmos cast record demo/landing/hero.tape --output=hero.mp4 # mode: session (default) | ||
| atmos cast record demo/landing/hero.tape --output=hero.cast --mode=steps # real exit codes | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the mode: steps contract for Sleep and Wait.
Sleep and Wait are parse-time errors in mode: steps. They are not ignored or dropped.
agent-skills/skills/atmos-vhs/SKILL.md#L85-L88: use a session-mode example or a tape withoutSleepandWait.agent-skills/skills/atmos-vhs/SKILL.md#L96-L112: includeSleepandWaitin the rejection criteria.agent-skills/skills/atmos-vhs/SKILL.md#L222-L226: describe removal as manual migration work, not interpreter behavior.website/docs/workflows/workflows/workflow/steps/type/cast.mdx#L116-L119: state that these directives fail inmode: steps.
📍 Affects 2 files
agent-skills/skills/atmos-vhs/SKILL.md#L85-L88(this comment)agent-skills/skills/atmos-vhs/SKILL.md#L96-L112agent-skills/skills/atmos-vhs/SKILL.md#L222-L226website/docs/workflows/workflows/workflow/steps/type/cast.mdx#L116-L119
🤖 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 `@agent-skills/skills/atmos-vhs/SKILL.md` around lines 85 - 88, Correct the
mode: steps documentation: in agent-skills/skills/atmos-vhs/SKILL.md lines
85-88, use session mode or a tape without Sleep and Wait; in lines 96-112,
include Sleep and Wait among parse-time rejection criteria; in lines 222-226,
describe removing them as manual migration work rather than interpreter
behavior; and in website/docs/workflows/workflows/workflow/steps/type/cast.mdx
lines 116-119, state that Sleep and Wait fail in mode: steps.
| var recordCmd = &cobra.Command{ | ||
| Use: "record <input.tape>", | ||
| Short: "Record a VHS-dialect tape into an asciicast recording", | ||
| Args: cobra.ExactArgs(1), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if err := recordParser.BindFlagsToViper(cmd, viper.GetViper()); err != nil { | ||
| return err | ||
| } | ||
| return runRecordCommand(cmd, args[0]) | ||
| }, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add Long help and an embedded usage example.
recordCmd defines only Short. The coding guidelines require an embedded usage example for command help. Add a cmd/markdown/cast_record_usage.md file, embed it with //go:embed, and render it with utils.PrintfMarkdown(), matching the sibling cast subcommands.
As per coding guidelines: "Embed command usage examples from cmd/markdown/*_usage.md with //go:embed and render them with utils.PrintfMarkdown()" and "Provide comprehensive help text for all commands and flags, include examples in command help".
Run this to copy the pattern from a sibling command:
#!/bin/bash
# Show how other cast subcommands wire Long help and embedded usage markdown.
fd -e go . cmd/cast --exec rg -n 'go:embed|Long:|PrintfMarkdown|Example:' {}
fd -g '*cast*_usage.md' cmd/markdown🤖 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/record.go` around lines 37 - 47, Update recordCmd to add
comprehensive Long help and an embedded usage example, following the sibling
cast subcommands’ go:embed, Long, and utils.PrintfMarkdown() pattern. Add the
corresponding cast_record_usage.md markdown resource and wire it into the
command’s help output while preserving the existing argument validation and
runRecordCommand flow.
Source: Coding guidelines
| atmos cast record demo/hero.tape --output=hero.mp4 | ||
| ``` | ||
|
|
||
| The step's existing `mode` still decides how the tape runs: `mode: session` replays raw keypresses faithfully (the same trade-off session mode already has — no per-command exit code, since it's one continuous PTY session), while `mode: steps` turns each typed command into a real, executed step with a real exit code, and errors up front if the tape uses anything `mode: steps` can't express (a raw keypress, `Hide`/`Show`, or `Copy`/`Paste`/`Env`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the setup-only Hide/Show exception consistently.
A Hide/Show block containing only export, unset, cd, and clear is lifted into cast configuration and works in mode: steps. Only other hidden commands require mode: session.
website/blog/2026-07-11-vhs-tape-interpretation.mdx#L36-L36: limit the session-only warning to non-liftableHide/Showblocks.website/docs/workflows/workflows/workflow/steps/type/cast.mdx#L118-L130: remove the claim that everyHide/Showblock errors inmode: steps.
📍 Affects 2 files
website/blog/2026-07-11-vhs-tape-interpretation.mdx#L36-L36(this comment)website/docs/workflows/workflows/workflow/steps/type/cast.mdx#L118-L130
🤖 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/blog/2026-07-11-vhs-tape-interpretation.mdx` at line 36, The mode
comparison in website/blog/2026-07-11-vhs-tape-interpretation.mdx#L36 must state
that only non-liftable Hide/Show blocks require mode: session; setup-only blocks
containing exclusively export, unset, cd, and clear work in mode: steps. Update
website/docs/workflows/workflows/workflow/steps/type/cast.mdx#L118-L130 to
remove the claim that every Hide/Show block fails in mode: steps, while
preserving the restriction for other unsupported commands.
…istry Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Viper treats any registered flag as shadowing every dotted key nested under its own key, so the global --cast flag (bound to the bare "cast" key) silently zeroed out unrelated keys like cast.record.mode and cast.recording.width whenever --cast itself was left unset. This broke `atmos cast record`'s documented session-mode default and made ATMOS_CAST_RECORDING_WIDTH/HEIGHT a silent no-op. Add WithViperKey to pin a flag's Viper storage key independently of its CLI name, and move the global --cast flag to "cast.target" so "cast" stays free to be the map root the CastConfig schema and other cast.* commands need. Also fix BindFlagsToViper's inherited-flags loop, which was rebinding every persistent flag (including --cast) under its bare name regardless of this override, via a pflag annotation that survives across commands.
…ssion recording Two independent bugs corrupted mode: session cast recordings: 1. answerTerminalQueries (driven by the background output-reader goroutine) and scripted write/key actions (driven by the foreground action loop) both wrote to the same PTY input with no synchronization, so a terminal-capability-query response could interleave with a concurrently in-flight typed command and corrupt it. Fixed with a mutex-guarded syncWriter whose Locked method makes a whole scripted action, or a whole query-response burst, atomic as a unit. 2. answerTerminalQueries only scanned one PTY read chunk at a time via bytes.Contains, so a query sequence split across two reads was silently never answered, and the querying shell would retry indefinitely. Fixed by carrying a small trailing fragment forward across chunks (pendingQueryTail) so a boundary-split query is still recognized. Also bump finishSession's hardcoded 2s teardown safety-valve to 5s (defaultSessionExitMaxWait): real shell startup/profile-loading overhead was legitimately exceeding it, forcibly killing a shell that was about to exit cleanly and discarding an otherwise-successful recording. This is very likely the root cause of the previously flaky TestRecordCmdRunEExecutesTapeAndRecordsCast (now green 10/10, was failing consistently before this fix). Verified against real zsh and sh sessions (100% clean over repeated runs, previously corrupted). Real bash 3.2 (macOS's frozen system bash) still shows intermittent issues under Set Shell bash and is tracked as a known follow-up, not fixed by this change.
…-migration # Conflicts: # pkg/asciicast/session.go # pkg/datafetcher/schema/atmos/manifest/1.0.json # pkg/datafetcher/schema/config/global/1.0.json # pkg/runner/step/cast.go # website/src/data/roadmap.js # website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
Write marker screenshots atomically (temp file + rename) instead of truncating in place, so a failed PNG encode never destroys an existing screenshot. Add Long/Example help and a committed --help screengrab to `atmos cast record`, pinning its test to mode: steps so it no longer spawns a real PTY-backed shell. Make the Windows-cd-tracking test build its expected path with filepath.Join instead of a hardcoded separator. Fix three tape lexer/parser bugs: wrap ParseTapeFile's top-level read error with the same sentinel + path context loadSource already uses, skip '#' comments that follow a directive on the same line (not just at line start), and stop absolute paths (e.g. `Output /tmp/demo.gif`) from being mis-lexed as regex literals -- only Wait*/Set now start a regex. Preserve buffered Hide actions across a second Hide before the matching Show instead of discarding them. Correct the atmos-vhs skill docs: a Hide/Show block of only export/unset/cd/clear lifts into native env:/working_directory: config under mode: session only, never mode: steps (any Hide/Show token is an unconditional parse-time error there); cosmetic Set directives warn before being ignored rather than doing so silently. Two related CodeRabbit threads asked for the opposite change to cast.mdx and the changelog post -- left both untouched since they already state the correct contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
pkg/asciicast/session_actions.go (1)
227-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the key-sequence map to package scope.
sequencesis rebuilt on everykeySequencecall, which happens once perkeyaction and per repeat resolution. A package-levelvaravoids the repeated allocation and makes the table easier to find.Separately,
len(key) == 1at line 250 counts bytes. A single non-ASCII rune such as"é"has length 2, so it falls through toErrUnsupportedCastKey. Useutf8.RuneCountInString(key) == 1if single non-ASCII keypresses should work.♻️ Proposed change
+var keySequences = map[string]string{ + "enter": "\r", + "return": "\r", + "tab": "\t", + "esc": "\x1b", + "escape": "\x1b", + "backspace": "\x7f", + "space": " ", + "up": "\x1b[A", + "down": "\x1b[B", + "right": "\x1b[C", + "left": "\x1b[D", + "pageup": "\x1b[5~", + "pagedown": "\x1b[6~", +} + func keySequence(key string) (string, error) { normalized := strings.ToLower(strings.TrimSpace(key)) - sequences := map[string]string{ - ... - } - if seq, ok := sequences[normalized]; ok { + if seq, ok := keySequences[normalized]; ok { return seq, nil } if seq, ok := ctrlKeySequence(normalized); ok { return seq, nil } - if len(key) == 1 { + if utf8.RuneCountInString(key) == 1 { return key, nil } return "", fmt.Errorf("%w: %q", ErrUnsupportedCastKey, key) }🤖 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_actions.go` around lines 227 - 254, Move the sequences map out of keySequence into a package-level variable and reuse it for lookups. In keySequence, replace the byte-based len(key) == 1 check with utf8.RuneCountInString(key) == 1 so single non-ASCII keypresses are accepted, adding the required utf8 import.pkg/asciicast/session.go (2)
151-157: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider dropping
perf.Trackfrom thesyncWriterhot path.
runWriteActioncallsw.Writeonce per rune insideLocked. Each call adds aperf.Trackdefer plus a map/stat update.WriteandLockedare trivial synchronization wrappers, so the tracking adds cost on the typing path without giving useful profile signal. The repository guideline exempts trivial accessors fromperf.Track.♻️ Proposed change
func (s *syncWriter) Write(p []byte) (int, error) { - defer perf.Track(nil, "asciicast.syncWriter.Write")() - s.mu.Lock() defer s.mu.Unlock() return s.w.Write(p) }As per coding guidelines, "Add
defer perf.Track(atmosConfig, "pkg.FuncName")()... except trivial accessors".Also applies to: 177-183
🤖 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 151 - 157, Remove the perf.Track defer from the trivial synchronization wrappers syncWriter.Write and the additionally referenced method around lines 177–183, while preserving their locking and underlying writer behavior. Do not alter tracking elsewhere unless it belongs to these hot-path wrappers.Source: Coding guidelines
385-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPattern list duplicates the literals in
answerTerminalQueries.
terminalQueryPatternsandanswerTerminalQueries(lines 428-441) each hardcode the same five query sequences. If someone adds a query toanswerTerminalQueriesonly, split-read carry-over silently stops covering it, which is exactly the hang this code fixes. Consider driving both from one table that pairs each query with its response.♻️ Sketch
var terminalQueries = []struct { query []byte response []byte }{ {[]byte("\x1b]11;?\x07"), []byte("\x1b]11;rgb:0000/0000/0000\x1b\\")}, {[]byte("\x1b]11;?\x1b\\"), []byte("\x1b]11;rgb:0000/0000/0000\x1b\\")}, {[]byte("\x1b]10;?\x07"), []byte("\x1b]10;rgb:ffff/ffff/ffff\x1b\\")}, {[]byte("\x1b]10;?\x1b\\"), []byte("\x1b]10;rgb:ffff/ffff/ffff\x1b\\")}, {[]byte("\x1b[6n"), []byte("\x1b[1;1R")}, }
answerTerminalQueriesthen iterates the table, andterminalQueryTailreadsqueryfrom the same source.🤖 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 385 - 393, Replace the separate terminalQueryPatterns literals and response cases in answerTerminalQueries with one shared terminalQueries table pairing each query with its response. Update answerTerminalQueries to iterate that table and update terminalQueryTail to derive matching query bytes from the same table, ensuring newly added queries automatically support both response handling and split-read carry-over.
🤖 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 `@agent-skills/skills/atmos-vhs/SKILL.md`:
- Around line 321-323: Update the portability guidance near the Hide block
documentation to remove any claim that Hide blocks work across both mode: steps
and mode: session. State that Hide/Show requires mode: session, or explicitly
note that mode: steps requires manually lifting those commands outside the
block.
In `@pkg/asciicast/screenshot.go`:
- Around line 52-57: Update writeScreenshotPNG to create the parent directory
returned by filepath.Dir(path) with os.MkdirAll(dir, 0o755) before calling
os.CreateTemp. Handle and return any directory-creation error using the existing
screenshot render error format, while preserving the current temporary-file
flow.
In `@pkg/asciicast/session_actions.go`:
- Around line 78-97: Update runWriteAction and runKeyAction so pacing delays
occur outside syncWriter.Locked, while preserving atomicity for each individual
writer operation and terminal-query response. Avoid holding syncWriter.mu across
time.Sleep, and keep the action’s existing write/error behavior unchanged.
In `@pkg/asciicast/tape_parser.go`:
- Around line 102-107: Update tapeCtrlKey to validate that the one-character
suffix after "Ctrl+" is an ASCII letter before lowercasing and returning it;
reject digits, symbols, and other invalid suffixes while preserving valid letter
handling. Add test cases covering rejected suffixes such as Ctrl+1 and Ctrl++.
In `@pkg/datafetcher/schema/atmos/config/1.0.json`:
- Around line 15001-15022: Add the existing tape and tape_file field definitions
to the $defs.Task schema, matching the types, nullability, and descriptions used
by the workflow step schema. Place them alongside the mode and shell fields so
Command.steps, which resolves through $defs.Tasks to $defs.Task, exposes tape
configuration for custom-command steps.
In `@pkg/flags/options.go`:
- Line 25: Terminate the inline field comments with periods in
pkg/flags/options.go lines 25-25 and pkg/flags/standard.go lines 43-43; update
both comments without changing their content or surrounding code.
In `@pkg/flags/standard_test.go`:
- Around line 239-267: Extend TestStandardFlagParser_WithViperKeyAvoidsShadowing
to use a parent-child Cobra command hierarchy with the root --cast flag
registered as persistent, then bind the child through the inherited-flags path.
Set the inherited flag and assert its value resolves to cast.target while
cast.record.mode remains "session".
In `@pkg/runner/step/cast.go`:
- Around line 91-93: Update ExecuteWithWorkflow to expand cast tape before
execution, ensuring step.Steps and session actions are populated even when
Validate was not called; reuse applyCastRecordingDefaults or call Validate as
appropriate, and make the expansion safe when the same workflowStep is processed
more than once.
In `@website/static/casts/screengrabs/atmos-cast-record--help.cast`:
- Line 10: Mark the --output flag as required in the cast record command
definition near the existing missing-output validation, using required-flag
metadata or explicit help text. Then regenerate the atmos-cast-record--help
recording so both affected help entries show --output as required.
- Around line 35-36: Remove the inline “default: session” text from the cast
mode flag description in the flag definition within record.go, allowing the flag
renderer to provide the sole default suffix. Regenerate the
atmos-cast-record--help.cast asset and verify the help output contains only one
default value.
---
Nitpick comments:
In `@pkg/asciicast/session_actions.go`:
- Around line 227-254: Move the sequences map out of keySequence into a
package-level variable and reuse it for lookups. In keySequence, replace the
byte-based len(key) == 1 check with utf8.RuneCountInString(key) == 1 so single
non-ASCII keypresses are accepted, adding the required utf8 import.
In `@pkg/asciicast/session.go`:
- Around line 151-157: Remove the perf.Track defer from the trivial
synchronization wrappers syncWriter.Write and the additionally referenced method
around lines 177–183, while preserving their locking and underlying writer
behavior. Do not alter tracking elsewhere unless it belongs to these hot-path
wrappers.
- Around line 385-393: Replace the separate terminalQueryPatterns literals and
response cases in answerTerminalQueries with one shared terminalQueries table
pairing each query with its response. Update answerTerminalQueries to iterate
that table and update terminalQueryTail to derive matching query bytes from the
same table, ensuring newly added queries automatically support both response
handling and split-read carry-over.
🪄 Autofix
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 Plus
Run ID: b13ff920-fad4-4b20-b6c0-6608c35aebd9
📒 Files selected for processing (40)
.claude/skills/atmos-vhsagent-skills/skills/atmos-vhs/SKILL.mdagent-skills/skills/atmos-vhs/references/vhs-directive-support.mdcmd/cast/record.gocmd/cast/record_test.gocmd/markdown/atmos_cast_record_usage.mdcmd/markdown/content.godemo/casts/atmos.d/screengrabs/cli.yamlerrors/errors.gopkg/asciicast/cellgrid.gopkg/asciicast/cellgrid_test.gopkg/asciicast/recorder.gopkg/asciicast/screenshot.gopkg/asciicast/screenshot_test.gopkg/asciicast/session.gopkg/asciicast/session_actions.gopkg/asciicast/session_test.gopkg/asciicast/tape.gopkg/asciicast/tape_parser.gopkg/asciicast/tape_test.gopkg/asciicast/testdata/legacy-demo.tapepkg/datafetcher/schema/atmos/config/1.0.jsonpkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/flags/global_builder.gopkg/flags/options.gopkg/flags/standard.gopkg/flags/standard_test.gopkg/io/recorder.gopkg/runner/step/cast.gopkg/runner/step/cast_tape.gopkg/runner/step/cast_tape_test.gopkg/schema/task.gopkg/schema/workflow.gowebsite/blog/2026-07-11-vhs-tape-interpretation.mdxwebsite/docs/cli/commands/cast/play.mdxwebsite/docs/cli/commands/cast/record.mdxwebsite/docs/cli/commands/cast/render.mdxwebsite/docs/workflows/workflows/workflow/steps/type/cast.mdxwebsite/src/data/roadmap.jswebsite/static/casts/screengrabs/atmos-cast-record--help.cast
🚧 Files skipped from review as they are similar to previous changes (22)
- pkg/datafetcher/schema/atmos/manifest/1.0.json
- .claude/skills/atmos-vhs
- website/blog/2026-07-11-vhs-tape-interpretation.mdx
- pkg/asciicast/cellgrid_test.go
- website/docs/cli/commands/cast/play.mdx
- website/docs/cli/commands/cast/record.mdx
- website/src/data/roadmap.js
- pkg/asciicast/recorder.go
- pkg/asciicast/cellgrid.go
- pkg/io/recorder.go
- website/docs/cli/commands/cast/render.mdx
- errors/errors.go
- pkg/schema/workflow.go
- pkg/asciicast/screenshot_test.go
- pkg/asciicast/testdata/legacy-demo.tape
- pkg/asciicast/session_test.go
- cmd/cast/record_test.go
- pkg/schema/task.go
- pkg/asciicast/tape.go
- cmd/cast/record.go
- pkg/runner/step/cast_tape_test.go
- pkg/runner/step/cast_tape.go
| - Keep a tape's `Hide` block to `export`/`unset`/`cd`/`clear` only if you want it to stay portable | ||
| across both `mode: steps` and `mode: session` -- anything else (typing a partial command, | ||
| toggling settings mid-`Hide`) locks the tape into `mode: session`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not describe Hide blocks as portable across both modes.
Hide/Show is a hard parse-time error in mode: steps, even when the block contains only export, unset, cd, and clear. Limit this statement to mode: session, or state that mode: steps requires manual lifting.
🤖 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 `@agent-skills/skills/atmos-vhs/SKILL.md` around lines 321 - 323, Update the
portability guidance near the Hide block documentation to remove any claim that
Hide blocks work across both mode: steps and mode: session. State that Hide/Show
requires mode: session, or explicitly note that mode: steps requires manually
lifting those commands outside the block.
| func writeScreenshotPNG(path string, img image.Image) error { | ||
| dir := filepath.Dir(path) | ||
| tmp, err := os.CreateTemp(dir, ".screenshot-*.png.tmp") | ||
| if err != nil { | ||
| return fmt.Errorf(screenshotRenderErrorFormat, ErrScreenshotRenderFailed, path, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 '(MkdirAll|Screenshot|screenshot)' --type=go -g '!**/*_test.go' | rg -n -C4 'MkdirAll|RecordMarker|Path'Repository: cloudposse/atmos
Length of output: 154
Create the screenshot parent directory before writing.
writeScreenshotPNG creates a temp file with os.CreateTemp(dir, ...) before renaming it to the marker path. If a Screenshot website/static/img/demos/hero.png marker targets a directory that does not exist, CreateTemp fails and the cast step exits. Add os.MkdirAll(dir, 0o755) before os.CreateTemp.
🤖 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/screenshot.go` around lines 52 - 57, Update writeScreenshotPNG
to create the parent directory returned by filepath.Dir(path) with
os.MkdirAll(dir, 0o755) before calling os.CreateTemp. Handle and return any
directory-creation error using the existing screenshot render error format,
while preserving the current temporary-file flow.
| // The whole typed string is one atomic unit: holding the lock only | ||
| // per-rune still lets a concurrent terminal-query response land between | ||
| // two characters of the same command, and some shells (observed with | ||
| // real bash, not zsh/sh) re-issue additional queries when a response is | ||
| // delayed, compounding rather than avoiding corruption. See | ||
| // syncWriter.Locked. | ||
| var writeErr error | ||
| input.Locked(func(w io.Writer) { | ||
| for _, r := range action.Text { | ||
| if _, err := w.Write([]byte(string(r))); err != nil { | ||
| writeErr = err | ||
| return | ||
| } | ||
| if rate > 0 { | ||
| time.Sleep(rate) | ||
| } | ||
| } | ||
| }) | ||
| return writeErr | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C5 'func .*Record\(' --type=go pkg/asciicast pkg/io
rg -nP -C4 'WriteRate|write_rate' --type=go -g '*_test.go'Repository: cloudposse/atmos
Length of output: 2613
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'session_actions.go|session.go|recorder.go|syncwriter|asciicast_.*_test.go' . | sed "s#$(pwd)/##"
echo
echo "== session_actions outline =="
ast-grep outline pkg/asciicast/session_actions.go --view expanded || true
echo
echo "== relevant session_actions lines =="
sed -n '1,180p' pkg/asciicast/session_actions.go
echo
echo "== relevant session.go lines around recordOutputChunk =="
sed -n '330,380p' pkg/asciicast/session.go
echo
echo "== recorder Record and related timing fields =="
sed -n '1,270p' pkg/asciicast/recorder.go
echo
echo "== wait/query and write/key tests around timing/write_rate =="
rg -nC6 'recordOutputChunk|write.*action|key.*action|wait|query|WriteRate|write_rate|time.Sleep|interval' --type=go pkg/asciicast pkg/io -g '*_test.go' | sed -n '1,260p'Repository: cloudposse/atmos
Length of output: 31214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== syncWriter definitions/usages =="
rg -n -C8 'type syncWriter|func \(.*syncWriter.*\) Locked|func \(.*syncWriter.*\) Write|sessionState|recordOutputChunk|outputRecorder|changed|recorder' --type=go pkg/asciicast
echo
echo "== session.go relevant sections =="
sed -n '1,180p' pkg/asciicast/session.go
echo "----"
sed -n '180,310p' pkg/asciicast/session.go
echo "----"
sed -n '380,560p' pkg/asciicast/session.go
echo
echo "== recorder output events =="
sed -n '270,430p' pkg/asciicast/recorder.goRepository: cloudposse/atmos
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== writeRecorderHeader and Recorder.Record usages =="
rg -n -C6 'writeRecorderHeader\(|Recorder|Record\(' pkg/asciicast/session.go pkg/asciicast/exec.go pkg/asciicast/tape.go pkg/asciicast/cast.go 2>/dev/null || true
echo
echo "== session.go action execution lifecycle =="
rg -n -C8 'runAction|NewSession|RunSession|sessionProcess|recOrder|record|Recorder|Data\(' pkg/asciicast/session.go
echo
echo "== io recorder interface/usages =="
rg -n -C6 'type .*Recorder|Record\(stream, content string\)|WriteBytes|Initialize|GetContext|SetRecorder|RecordMarker' --type=go pkg/io pkg/asciicast | sed -n '1,240p'Repository: cloudposse/atmos
Length of output: 22742
Do not keep syncWriter locked while waiting inside write or key actions.
runWriteAction and runKeyAction call input.Locked(...) and then call time.Sleep while syncWriter.mu is held. The output reader’s recordOutputChunk also needs that lock when answering terminal queries and recording output, so output recording can sit behind every paced write. This makes recorded timing bunch at the end of the action and can make wait fail to see output produced during the typed/pressed string.
Keep atomicity for query-response writes, but separate the pacing sleeps from the writer lock, or at least document that recording waits for the whole paced action.
🤖 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_actions.go` around lines 78 - 97, Update runWriteAction
and runKeyAction so pacing delays occur outside syncWriter.Locked, while
preserving atomicity for each individual writer operation and terminal-query
response. Avoid holding syncWriter.mu across time.Sleep, and keep the action’s
existing write/error behavior unchanged.
| func tapeCtrlKey(word string) (string, bool) { | ||
| const prefix = "Ctrl+" | ||
| if !strings.HasPrefix(word, prefix) || len(word) != len(prefix)+1 { | ||
| return "", false | ||
| } | ||
| return "ctrl+" + strings.ToLower(word[len(prefix):]), true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict Ctrl+ directives to letters.
This accepts Ctrl+1, Ctrl++, and other one-byte suffixes. Those values contradict the documented Ctrl+<Letter> grammar and can reach session execution as invalid keys. Validate the suffix as an ASCII letter before normalizing it. Add rejected-suffix test cases.
Proposed fix
if !strings.HasPrefix(word, prefix) || len(word) != len(prefix)+1 {
return "", false
}
- return "ctrl+" + strings.ToLower(word[len(prefix):]), true
+ suffix := word[len(prefix)]
+ if !((suffix >= 'A' && suffix <= 'Z') || (suffix >= 'a' && suffix <= 'z')) {
+ return "", false
+ }
+ return "ctrl+" + strings.ToLower(word[len(prefix):]), 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 tapeCtrlKey(word string) (string, bool) { | |
| const prefix = "Ctrl+" | |
| if !strings.HasPrefix(word, prefix) || len(word) != len(prefix)+1 { | |
| return "", false | |
| } | |
| return "ctrl+" + strings.ToLower(word[len(prefix):]), true | |
| func tapeCtrlKey(word string) (string, bool) { | |
| const prefix = "Ctrl+" | |
| if !strings.HasPrefix(word, prefix) || len(word) != len(prefix)+1 { | |
| return "", false | |
| } | |
| suffix := word[len(prefix)] | |
| if !((suffix >= 'A' && suffix <= 'Z') || (suffix >= 'a' && suffix <= 'z')) { | |
| return "", false | |
| } | |
| return "ctrl+" + strings.ToLower(word[len(prefix):]), true | |
| } |
🤖 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/tape_parser.go` around lines 102 - 107, Update tapeCtrlKey to
validate that the one-character suffix after "Ctrl+" is an ASCII letter before
lowercasing and returning it; reject digits, symbols, and other invalid suffixes
while preserving valid letter handling. Add test cases covering rejected
suffixes such as Ctrl+1 and Ctrl++.
| "tape": { | ||
| "anyOf": [ | ||
| { | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "description": "Inline VHS-dialect tape script interpreted at Execute time. Mutually exclusive with tape_file and with steps." | ||
| }, | ||
| "tape_file": { | ||
| "anyOf": [ | ||
| { | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "description": "Path to an external VHS .tape file, interpreted the same way as tape." | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Expose tape fields for custom-command steps.
Command.steps uses $defs.Tasks, which resolves to $defs.Task. $defs.Task has no tape or tape_file; Line 10113 proceeds from mode to shell. Add the same fields to $defs.Task. Otherwise, custom-command tape configuration is absent from schema completion and generated schema documentation.
Based on the PR objective, both workflow and custom-command steps must support tape fields.
🤖 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/config/1.0.json` around lines 15001 - 15022, Add
the existing tape and tape_file field definitions to the $defs.Task schema,
matching the types, nullability, and descriptions used by the workflow step
schema. Place them alongside the mode and shell fields so Command.steps, which
resolves through $defs.Tasks to $defs.Task, exposes tape configuration for
custom-command steps.
| viperPrefix string // Prefix for Viper keys (optional) | ||
| registry *FlagRegistry | ||
| viperPrefix string // Prefix for Viper keys (optional) | ||
| viperKeyOverrides map[string]string // Flag name -> explicit Viper key, bypassing viperPrefix |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Terminate the new inline comments with periods.
pkg/flags/options.go#L25-L25: End the field comment with a period.pkg/flags/standard.go#L43-L43: End the field comment with a period.
As per coding guidelines, all comments must end with periods. Based on learnings, this applies to these single-line comments.
📍 Affects 2 files
pkg/flags/options.go#L25-L25(this comment)pkg/flags/standard.go#L43-L43
🤖 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/flags/options.go` at line 25, Terminate the inline field comments with
periods in pkg/flags/options.go lines 25-25 and pkg/flags/standard.go lines
43-43; update both comments without changing their content or surrounding code.
Sources: Coding guidelines, Learnings
| func TestStandardFlagParser_WithViperKeyAvoidsShadowing(t *testing.T) { | ||
| v := viper.New() | ||
|
|
||
| rootParser := NewStandardFlagParser( | ||
| WithStringFlag("cast", "", "", "root flag"), | ||
| WithViperKey("cast", "cast.target"), | ||
| ) | ||
| rootCmd := &cobra.Command{Use: "root"} | ||
| rootParser.RegisterFlags(rootCmd) | ||
| require.NoError(t, rootParser.BindToViper(v)) | ||
| require.NoError(t, rootParser.BindFlagsToViper(rootCmd, v)) | ||
|
|
||
| nestedParser := NewStandardFlagParser( | ||
| WithViperPrefix("cast.record"), | ||
| WithStringFlag("mode", "", "session", "nested flag"), | ||
| ) | ||
| nestedCmd := &cobra.Command{Use: "record"} | ||
| nestedParser.RegisterFlags(nestedCmd) | ||
| require.NoError(t, nestedParser.BindToViper(v)) | ||
| require.NoError(t, nestedParser.BindFlagsToViper(nestedCmd, v)) | ||
|
|
||
| assert.Equal(t, "session", v.GetString("cast.record.mode"), | ||
| "WithViperKey must stop the bare-key flag from shadowing the nested flag's default") | ||
|
|
||
| // The root flag's own value must still resolve correctly at its new key. | ||
| require.NoError(t, rootCmd.Flags().Set("cast", "demo.gif")) | ||
| assert.Equal(t, "demo.gif", v.GetString("cast.target"), | ||
| "the overridden flag must still resolve its own explicitly-set value") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover inherited flag binding.
Lines 242-258 bind independent commands. They do not execute the new cmd.InheritedFlags() path in BindFlagsToViper. Add a parent-child command test with an inherited persistent --cast flag. Assert that cast.target receives the flag value and cast.record.mode keeps its default.
🤖 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/flags/standard_test.go` around lines 239 - 267, Extend
TestStandardFlagParser_WithViperKeyAvoidsShadowing to use a parent-child Cobra
command hierarchy with the root --cast flag registered as persistent, then bind
the child through the inherited-flags path. Set the inherited flag and assert
its value resolves to cast.target while cast.record.mode remains "session".
Source: Coding guidelines
| if err := expandCastTape(step); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C6 '\.Validate\(' --type=go pkg/runner internal/exec | rg -n -C6 'Execute|Validate'
rg -nP -C10 'func expandCastTape' --type=goRepository: cloudposse/atmos
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "candidate cast.go outline:"
ast-grep outline pkg/runner/step/cast.go --view compact || true
echo
echo "cast.go relevant sections:"
sed -n '1,220p' pkg/runner/step/cast.go | cat -n
echo "...................................................................."
sed -n '720,810p' pkg/runner/step/cast.go | cat -n
echo
echo "ExecuteWithWorkflow references:"
rg -n "ExecuteWithWorkflow|func .*ExecuteWithWorkflow" --type=go
echo "...................................................................."
cast_exec_ref_line=$(rg -n "ExecuteWithWorkflow" --type=go | head -20 | awk '{print $1}' | sed 's/:.*//' | head -5)
for f in $cast_exec_ref_line; do
echo "--- $f ---"
rg -n -C8 "ExecuteWithWorkflow" --type=go "$f"
done
echo
echo "cast Validate callers:"
rg -n -C4 "\.Validate\(" --type=go pkg/runner/step/cast.go pkg/runner pkg/ | rg -C4 'TaskTypeCast|Validate|cast\.' | head -250Repository: cloudposse/atmos
Length of output: 12346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "runCastBody/runSessionTape/session-related functions:"
rg -n "func runCastBody|func run.*Session|func .*Session|expandCastTape|expandCastSession" pkg/runner/step/cast.go
for pat in "func runCastBody" "func .*SessionTape" "func .*Session.*[" '"session"' "expandCastTape"; do
echo "--- rg $pat ---"
rg -n -C3 "$pat" pkg/runner/step/cast.go
done
echo
echo "Validate and Execute caller scopes:"
rg -n -C4 "Validate\([^)]*step[^\n]+Cast|CastHandler|schema\.TaskTypeCast|\"cast\"" --type=go pkg internal | head -400Repository: cloudposse/atmos
Length of output: 1611
Run tape expansion on the execute path too.
Validate mutates step with expandCastTape, but ExecuteWithWorkflow only calls applyCastRecordingDefaults, so users can run Execute before validation and start a blank, failed recording since step.Steps / session actions remain empty. Add expansion/recording defaults to the execute path, or call Validate first; make expansion idempotent if more than one pass uses the same *workflowStep.
🤖 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 91 - 93, Update ExecuteWithWorkflow to
expand cast tape before execution, ensuring step.Steps and session actions are
populated even when Validate was not called; reuse applyCastRecordingDefaults or
call Validate as appropriate, and make the expansion safe when the same
workflowStep is processed more than once.
| [0.000004708,"o","\n"] | ||
| [0.000004625,"o","\u001b[1;38;2;0;163;224mUSAGE\u001b[0m\n"] | ||
| [9.59e-7,"o","\n"] | ||
| [0.000054708,"o"," \u001b[48;2;47;46;54m \u001b[0m\n \u001b[48;2;47;46;54m \u001b[0m\u001b[38;2;231;229;227;48;2;47;46;54m\u001b[38;2;87;83;78;48;2;47;46;54m$ \u001b[0m\u001b[1;38;2;155;81;224;48;2;47;46;54matmos\u001b[0m\u001b[38;2;231;229;227;48;2;47;46;54m \u001b[0m\u001b[38;2;231;229;227;48;2;47;46;54mcast\u001b[0m\u001b[38;2;231;229;227;48;2;47;46;54m \u001b[0m\u001b[38;2;231;229;227;48;2;47;46;54mrecord\u001b[0m\u001b[38;2;231;229;227;48;2;47;46;54m \u001b[0m\u001b[3;38;2;231;229;227;48;2;47;46;54m\u003cinput.tape\u003e\u001b[0m\u001b[38;2;231;229;227;48;2;47;46;54m \u001b[0m\u001b[3;38;2;231;229;227;48;2;47;46;54m[flags]\u001b[0m\u001b[0m\u001b[48;2;47;46;54m \u001b[0m\n \u001b[48;2;47;46;54m \u001b[0m\n"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mark --output as required in the help.
cmd/cast/record.go:76-81 returns ErrMissingRenderOutput when --output is empty, but this recording presents the flag as optional. Add required-flag metadata or explicit help text, then regenerate the recording.
Also applies to: 39-43
🤖 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/casts/screengrabs/atmos-cast-record--help.cast` at line 10,
Mark the --output flag as required in the cast record command definition near
the existing missing-output validation, using required-flag metadata or explicit
help text. Then regenerate the atmos-cast-record--help recording so both
affected help entries show --output as required.
| [0.000127834,"o","\u001b[38;2;255;255;255m\u001b[38;2;255;255;255m\u001b[0m\u001b[38;2;255;255;255m\u001b[0m \u001b[38;2;255;255;255m \u001b[0m\u001b[38;2;255;255;255mCast mode: session or steps (default: session) \u001b[0m\u001b[38;2;255;255;255m(default\u001b[0m\u001b[0m\n"] | ||
| [0.000003666,"o"," \u001b[38;2;255;255;255m\u001b[0m\u001b[38;2;155;81;224;1m\u001b[0m\u001b[38;2;255;255;255m\u001b[0m \u001b[38;2;255;255;255m \u001b[0m\u001b[38;2;155;81;224;1msession\u001b[0m\u001b[38;2;255;255;255m)\u001b[0m\u001b[0m\n"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the duplicated default value from the help output.
The recording shows both (default: session) and (default session). In cmd/cast/record.go:51-66, the flag description already includes the default, and the flag renderer adds another default suffix. Remove the inline default from the source description, then regenerate this asset.
Suggested source change
- flags.WithStringFlag(recordFlagMode, "", recordModeSession, "Cast mode: session or steps (default: session)"),
+ flags.WithStringFlag(recordFlagMode, "", recordModeSession, "Cast mode: session or steps"),🤖 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/casts/screengrabs/atmos-cast-record--help.cast` around lines
35 - 36, Remove the inline “default: session” text from the cast mode flag
description in the flag definition within record.go, allowing the flag renderer
to provide the sole default suffix. Regenerate the atmos-cast-record--help.cast
asset and verify the help output contains only one default value.
1 participant
what
tape:/tape_file:fields to thetype: castworkflow/custom-command step: a VHS-dialect.tapescript (inline or from a file) is interpreted directly, in memory, at execution time — novhsbinary, no generated YAML, nothing written to disk.mode: steps(real, exit-code-tracked child steps) ormode: session(PTY-driven keypress replay) machinery, includingHide/Show,Screenshot,Source, andRequire; unsupported directives log a warning (cosmeticSetkeys) or error out with the offending line (Sleep/Waitdropped undermode: steps, bare keypresses requiringmode: session).atmos cast record <input.tape> --output=<path>CLI subcommand so a tape can be recorded ad hoc with no workflow YAML at all, completing therecord/play/renderverb set.Screenshotdirective → a rasterized PNG at that point in the recording) topkg/asciicast.atmos-vhsskill covering tape interpretation and hand-migration to nativesteps:.minorroadmap entry + blog post for the new capability.why
.tapefiles previously had to choose between keeping a second recording toolchain installed or hand-rewriting every directive into Atmos's own cast step YAML..tapescripts run as Atmos casts unmodified, whilemode: stepsupgrades typed commands to real, individually-executed steps with real exit codes when that's wanted instead of raw PTY replay.references
/workflows/workflows/workflow/steps/type/cast#tape-interpretation,/cli/commands/cast/recordSummary by CodeRabbit
.tapefiles.atmos cast recordfor recording tapes as.cast, GIF, and other supported formats.