Repository navigation
feat(cast): auto-install animated renderers - #2763
Merged
Andriy Knysh (aknysh) merged 15 commits intoJul 19, 2026
Merged
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Erik Osterman (Cloud Posse) (osterman)
marked this pull request as ready for review
July 18, 2026 13:00
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
July 18, 2026 13:00
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
|
Warning Release Documentation RequiredThis PR is labeled
|
Erik Osterman (Cloud Posse) (osterman)
added
patch
Resource Changes Found for
|
Contributor
|
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 (2)
📝 WalkthroughWalkthroughAtmos now manages GIF/MP4 renderers, centralizes terminal-width fallback behavior, and updates related tests, documentation, maintenance guidance, portability handling, and CI configuration. ChangesManaged cast rendering
Terminal width resolution
Test maintenance policy
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/asciicast/render_toolchain.go`:
- Around line 39-57: Replace the mutable package-level hooks in
pkg/asciicast/render_toolchain.go:39-57 with an injected interface covering
dependency installation and render-tool binary resolution, and update the
relevant renderer flow to consume that interface. In
pkg/asciicast/render_test.go:452-469, remove global hook mutation and use
generated mockgen mocks for the interface; preserve the existing test behavior
through dependency injection.
- Line 129: Update the renderer execution return in the relevant toolchain
function to detect cmd.Run() failures and wrap them with the appropriate static
sentinel from errors/errors.go using %w, preserving the original error and
identifying the managed renderer that failed.
- Around line 126-128: Update the subprocess stream assignments in the command
execution flow to use the Atmos IO helpers instead of direct os.Stdout and
os.Stderr. Route command data through the data helper’s stdout stream and
renderer/UI messages through the ui helper’s stderr stream, preserving the CLI’s
data/UI separation and test capture.
🪄 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: 7286a0f8-ca3b-41ad-bee0-4a4ab235ea34
📒 Files selected for processing (8)
cmd/cast/recording_test.gopkg/asciicast/render.gopkg/asciicast/render_test.gopkg/asciicast/render_toolchain.gopkg/runner/step/cast_test.gowebsite/docs/cli/commands/cast/render.mdxwebsite/docs/cli/global-flags.mdxwebsite/docs/workflows/workflows/workflow/steps/type/cast.mdx
Contributor
|
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 |
Address CodeRabbit review feedback on render_toolchain.go: wire the managed renderer subprocess (agg/ffmpeg) stdout/stderr through iolib.GetContext() instead of os.Stdout/os.Stderr directly, and wrap cmd.Run() failures with a static ErrRenderToolExecFailed sentinel so callers can identify which renderer failed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2763 +/- ##
==========================================
- Coverage 81.78% 81.77% -0.01%
==========================================
Files 1750 1751 +1
Lines 167186 167259 +73
==========================================
+ Hits 136729 136775 +46
- Misses 22952 22974 +22
- Partials 7505 7510 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Raise the patch-scoped test-coverage go test timeout to 40m (matching .atmos.d/test.yaml) since the untouched-package ./tests binary was tripping Go's 10m default under load, not actually failing. Also drop the "pre-existing means don't touch" carve-out in test-coverage-fix, test-coverage, and fix-all: the loop now attempts a confident fix on any failing test in a scoped package, whether or not this patch broke it, and only reports rather than fixes when it can't do so safely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Re-ran atmos fix coverage against a patch touching tests/ after the timeout fix landed: STATUS: OK, full ./tests package completed cleanly with no false-positive panic. Updates the fix-log's Follow-ups section from deferred to validated, per CodeRabbit's review-thread request not to leave it open-ended. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/asciicast/render_toolchain_test.go (2)
77-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
testify/requirefor cleaner assertions.Nice work on the I/O masking coverage. We can drop the manual
ifandt.Fatalblocks in favor oftestify/requireto make the assertions pop and fail loudly. As per coding guidelines, "Safety checks ... must fail loudly with require.Positive or an equivalent assertion".♻️ Proposed refactor
func TestRunRendererRoutesOutputThroughIOMasking(t *testing.T) { exe, err := os.Executable() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) t.Setenv(asciicastExecHelperEnv, "ok") iolib.RegisterSecret("stdout line") iolib.RegisterSecret("stderr line") stdoutR, stdoutW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) stderrR, stderrW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) origStdout, origStderr := os.Stdout, os.Stderr os.Stdout, os.Stderr = stdoutW, stderrW t.Cleanup(func() { os.Stdout, os.Stderr = origStdout, origStderr }) runErr := runRenderer(exe) _ = stdoutW.Close() _ = stderrW.Close() stdoutBytes, readErr := io.ReadAll(stdoutR) - if readErr != nil { - t.Fatal(readErr) - } + require.NoError(t, readErr) stderrBytes, readErr := io.ReadAll(stderrR) - if readErr != nil { - t.Fatal(readErr) - } + require.NoError(t, readErr) - if runErr != nil { - t.Fatalf("runRenderer: %v", runErr) - } + require.NoError(t, runErr) - if strings.Contains(string(stdoutBytes), "stdout line") { - t.Fatalf("expected masked stdout (raw secret leaked), got %q", stdoutBytes) - } + require.NotContains(t, string(stdoutBytes), "stdout line", "expected masked stdout (raw secret leaked)") - if !strings.Contains(string(stdoutBytes), iolib.MaskReplacement) { - t.Fatalf("expected stdout to contain the mask replacement, got %q", stdoutBytes) - } + require.Contains(t, string(stdoutBytes), iolib.MaskReplacement, "expected stdout to contain the mask replacement") - if strings.Contains(string(stderrBytes), "stderr line") { - t.Fatalf("expected masked stderr (raw secret leaked), got %q", stderrBytes) - } + require.NotContains(t, string(stderrBytes), "stderr line", "expected masked stderr (raw secret leaked)") - if !strings.Contains(string(stderrBytes), iolib.MaskReplacement) { - t.Fatalf("expected stderr to contain the mask replacement, got %q", stderrBytes) - } + require.Contains(t, string(stderrBytes), iolib.MaskReplacement, "expected stderr to contain the mask replacement") }🤖 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_toolchain_test.go` around lines 77 - 129, Update TestRunRendererRoutesOutputThroughIOMasking to use testify/require assertions instead of manual t.Fatal, if checks, and string conditions. Replace error checks with require.NoError and validate masked output with require.NotContains and require.Contains, preserving the existing assertion messages where practical.Source: Coding guidelines
14-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven tests and
testify/require.Good test coverage here. To perfectly align with the project guidelines, let's roll these three related execution scenarios into a single table-driven test and trim the manual error checks using
testify/require. As per coding guidelines, "Use table-driven tests for testing multiple scenarios" and "Safety checks ... must fail loudly with require.Positive or an equivalent assertion".(Remember to run
goimportsor manually pull in"github.com/stretchr/testify/require"if you apply this.)♻️ Proposed refactor to a table-driven test
-// TestRunRendererRejectsEmptyBinary covers the guard clause in runRenderer -// that refuses to exec.Command with an empty path. This protects against a -// resolveRenderTools bug that returns a renderTools with an unset field -// (e.g. tools.ffmpeg left empty) reaching exec.Command, which would otherwise -// surface as a confusing "file not found" error instead of a clear signal -// that the managed renderer path was never resolved. -func TestRunRendererRejectsEmptyBinary(t *testing.T) { - err := runRenderer("") - if !errors.Is(err, errUtils.ErrToolInstall) { - t.Fatalf("expected wrapped ErrToolInstall for empty binary, got %v", err) - } -} - -// TestRunRendererWrapsExecutionFailure asserts that a failing renderer -// process surfaces as the dedicated errUtils.ErrRenderToolExecFailed -// sentinel, with the binary path folded into the message, so callers can use -// errors.Is to distinguish "renderer process failed" from other failure -// modes (e.g. errUtils.ErrToolInstall for a renderer that couldn't be -// resolved/installed at all). -func TestRunRendererWrapsExecutionFailure(t *testing.T) { - exe, err := os.Executable() - if err != nil { - t.Fatal(err) - } - t.Setenv(asciicastExecHelperEnv, "fail") - - runErr := runRenderer(exe) - if runErr == nil { - t.Fatal("expected error from failing renderer process") - } - if !errors.Is(runErr, errUtils.ErrRenderToolExecFailed) { - t.Fatalf("expected ErrRenderToolExecFailed, got %v", runErr) - } - if !strings.Contains(runErr.Error(), exe) { - t.Fatalf("expected error to name the failed binary %q, got %v", exe, runErr) - } -} - -// TestRunRendererSucceedsWithoutError confirms the happy path returns nil -// once the subprocess exits cleanly, exercising the fall-through after -// cmd.Run() succeeds (the counterpart to the failure-wrapping test above). -func TestRunRendererSucceedsWithoutError(t *testing.T) { - exe, err := os.Executable() - if err != nil { - t.Fatal(err) - } - t.Setenv(asciicastExecHelperEnv, "ok") - - if err := runRenderer(exe); err != nil { - t.Fatalf("expected success, got %v", err) - } -} +func TestRunRendererExecution(t *testing.T) { + exe, err := os.Executable() + require.NoError(t, err) + + tests := []struct { + name string + binary string + env string + wantErr error + wantMsg string + }{ + { + // Covers the guard clause in runRenderer that refuses to exec.Command with an empty path. + name: "empty binary", + binary: "", + wantErr: errUtils.ErrToolInstall, + }, + { + // Asserts that a failing renderer process surfaces as the dedicated ErrRenderToolExecFailed sentinel. + name: "execution failure", + binary: exe, + env: "fail", + wantErr: errUtils.ErrRenderToolExecFailed, + wantMsg: exe, + }, + { + // Confirms the happy path returns nil once the subprocess exits cleanly. + name: "success", + binary: exe, + env: "ok", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.env != "" { + t.Setenv(asciicastExecHelperEnv, tt.env) + } + err := runRenderer(tt.binary) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + if tt.wantMsg != "" { + require.ErrorContains(t, err, tt.wantMsg) + } + } else { + require.NoError(t, err) + } + }) + } +}🤖 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_toolchain_test.go` around lines 14 - 65, Consolidate TestRunRendererRejectsEmptyBinary, TestRunRendererWrapsExecutionFailure, and TestRunRendererSucceedsWithoutError into one table-driven test covering the three scenarios. Use testify/require for setup and assertions, including executable lookup, expected errors, and message checks, while preserving each case’s environment, input, and expected outcome. Add the require import and keep the existing helper symbols and sentinel validations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/agents/test-coverage-fix.md:
- Around line 47-54: Update the Step 2 scope in the test-failure workflow so it
applies to every attempted failure, including pre-existing failures, rather than
only “in-scope” failures. Preserve the required reproduce, diagnosis, fix, and
full-package verification steps for all failures, while retaining the existing
stop-and-report path for issues that cannot be safely fixed.
In @.claude/skills/test-coverage/SKILL.md:
- Around line 59-70: Require the test-coverage workflow to enter coverage-gap
analysis only after all scoped tests pass; if any failure remains after one fix
attempt, stop and invoke the human-attention path. Update the failure-handling
guidance in .claude/skills/test-coverage/SKILL.md lines 59-70 and the Section B
transition in .claude/agents/test-coverage-fix.md lines 78-81, preserving the
existing reporting and say-skill requirements.
---
Nitpick comments:
In `@pkg/asciicast/render_toolchain_test.go`:
- Around line 77-129: Update TestRunRendererRoutesOutputThroughIOMasking to use
testify/require assertions instead of manual t.Fatal, if checks, and string
conditions. Replace error checks with require.NoError and validate masked output
with require.NotContains and require.Contains, preserving the existing assertion
messages where practical.
- Around line 14-65: Consolidate TestRunRendererRejectsEmptyBinary,
TestRunRendererWrapsExecutionFailure, and TestRunRendererSucceedsWithoutError
into one table-driven test covering the three scenarios. Use testify/require for
setup and assertions, including executable lookup, expected errors, and message
checks, while preserving each case’s environment, input, and expected outcome.
Add the require import and keep the existing helper symbols and sentinel
validations.
🪄 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: e7070701-aace-4963-a56a-a650e65f0a86
📒 Files selected for processing (12)
.claude/agents/test-coverage-fix.md.claude/skills/fix-all/SKILL.md.claude/skills/test-coverage/SKILL.md.claude/skills/test-coverage/scripts/patch-test-coverage.shdocs/fixes/2026-07-18-pr-maintenance-loop-pre-existing-test-timeout.mddocs/fixes/2026-07-18-windows-cached-test-tool-exe-suffix.mdlychee.tomlpkg/asciicast/render_test.gopkg/asciicast/render_toolchain_test.gotests/cli_remote_imports_test.gotests/precondition_cached_tools_test.gotests/preconditions.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/asciicast/render_test.go
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…-cast-renderers # Conflicts: # .claude/skills/test-coverage/scripts/patch-test-coverage.sh
Andriy Knysh (aknysh)
approved these changes
Jul 19, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.223.1-rc.2. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
why
references
Summary by CodeRabbit
COLUMNS.