feat: Add VHS demo video infrastructure with TUI demo#1907
feat: Add VHS demo video infrastructure with TUI demo#1907
Conversation
|
Warning This PR exceeds the recommended limit of 1,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 @osterman? 🙏 |
|
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 |
|
Warning This PR is blocked from merging because a required semver label is missing. You'll need to add one before this PR can be merged. |
Dependency ReviewThe following issues were found:
Vulnerabilitiestools/director/go.mod
Only included vulnerabilities with severity moderate or higher. License Issuestools/director/go.mod
Scanned Files
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (2.23%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1907 +/- ##
==========================================
- Coverage 73.23% 72.92% -0.32%
==========================================
Files 614 616 +2
Lines 57827 58094 +267
==========================================
+ Hits 42352 42364 +12
- Misses 12523 12779 +256
+ Partials 2952 2951 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis PR introduces a comprehensive VHS-based demo video infrastructure for Atmos. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Director CLI
participant Scene as Scene Loader
participant Cache as Cache Manager
participant Renderer as VHS Renderer
participant Backend as Backend (Stream/R2)
participant FS as File System
CLI->>Scene: LoadScenes(scenes.yaml)
Scene->>FS: Read scenes.yaml
FS-->>Scene: Scene list
Scene-->>CLI: ScenesList
CLI->>Cache: LoadCache()
Cache->>FS: Read .cache/metadata.json
FS-->>Cache: Cache metadata
Cache-->>CLI: CacheMetadata
CLI->>Renderer: Render(scene)
Renderer->>Cache: NeedsRender(scene, outputs)
alt Render needed
Renderer->>FS: Run VHS on tape
FS-->>Renderer: Outputs (GIF, MP4, SVG)
Renderer->>Renderer: Optional: merge audio, fix SVG
Renderer->>FS: Move outputs to cache
Renderer->>Cache: UpdateScene(hashes, duration)
else Cached
Renderer-->>CLI: CACHED status
end
Cache->>FS: SaveCache()
Renderer-->>CLI: RenderResult
CLI->>Backend: Upload(output, format)
Backend->>FS: Read local file
FS-->>Backend: File content
Backend-->>Backend: Validate credentials
alt Validation ok
Backend->>Backend: Upload to Stream/R2
Backend-->>CLI: Public URL
CLI->>Cache: UpdatePublish(URL, metadata)
else Validation failed
Backend-->>CLI: Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR introduces significant heterogeneous changes across multiple domains: a full-featured Go CLI tool with command routing and multiple backends, 100+ demo automation scripts (mostly homogeneous tapes), complex media processing logic (FFmpeg), React components with state management, and fixture configurations. While individual tape files are trivial to review, the director tool commands are logic-dense and interdependent, and the backend implementations require careful credential/API handling validation. The component query feature adds YQ evaluation logic that warrants scrutiny. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/list/renderer/renderer.go (2)
48-92: Add performance tracking to public function.Per coding guidelines, all public functions should include
defer perf.Track()at the start. Since this function doesn't have anatmosConfigparameter, usenil:func (r *Renderer) RenderToString(data []map[string]any) (string, error) { defer perf.Track(nil, "renderer.RenderToString")() // Guard against nil column selector. if r.selector == nil {Don't forget to add the blank line after the defer statement.
Based on coding guidelines: "Add
defer perf.Track(atmosConfig, \"pkg.FuncName\")()+ blank line to all public functions for performance tracking; use nil if no atmosConfig param"
94-107: Add performance tracking to public function.This public function also needs performance tracking:
func (r *Renderer) Render(data []map[string]any) error { defer perf.Track(nil, "renderer.Render")() formatted, err := r.RenderToString(data)Based on coding guidelines.
🟠 Major comments (18)
pkg/vhs/vhs.go-3-11 (1)
3-11: Add blank line between import groups.The import organization should separate standard library packages from Atmos packages with a blank line.
As per coding guidelines, imports must be organized in three groups separated by blank lines.
🔎 Proposed fix
import ( "context" "fmt" "os" "os/exec" + errUtils "github.com/cloudposse/atmos/errors" "github.com/cloudposse/atmos/pkg/perf" )cmd/list/utils.go-264-287 (1)
264-287: Add performance tracking and consider export visibility.Two observations:
Missing performance tracking: Per coding guidelines, all public functions should include
defer perf.Track(atmosConfig, "cmd/list.renderWithPager")()at the start.Export visibility: This function is exported (capitalized) but appears to be a package-internal helper used only within cmd/list. Consider making it private (
renderWithPager→renderWithPagerkeeps it exported, but verify if external packages need to call it).Based on coding guidelines requiring perf.Track for all public functions.
🔎 Proposed fix to add performance tracking
+import ( + "github.com/cloudposse/atmos/pkg/perf" +) + func renderWithPager(atmosConfig *schema.AtmosConfiguration, title string, r *renderer.Renderer, data []map[string]any) error { + defer perf.Track(atmosConfig, "cmd/list.renderWithPager")() + // Check if pager is enabled in config. if atmosConfig.Settings.Terminal.IsPagerEnabled() {Committable suggestion skipped: line range outside the PR's diff.
tools/director/cmd/export.go-7-25 (1)
7-25: Add required godoc comment and performance tracking.The exported function
exportCmdis missing required documentation and performance tracking per coding guidelines:
- Missing godoc comment: All exported functions must be documented following Go's documentation conventions.
- Missing performance tracking: Public functions should include
defer perf.Track(atmosConfig, "cmd.exportCmd")()(usenilif no atmosConfig param).- Comment style (line 21): The comment should end with a period per godot linter rules.
🔎 Proposed fix
+// exportCmd creates the export command for generating gallery data and manifests. func exportCmd() *cobra.Command { + defer perf.Track(nil, "cmd.exportCmd")() + cmd := &cobra.Command{ Use: "export", Short: "Export demo data and metadata", Long: `Export commands for generating gallery data, metadata, and manifests. The export command provides subcommands for exporting VHS demo metadata in various formats suitable for integration with website components.`, Example: ` # Export gallery manifest JSON director export manifest > website/src/data/demos.json `, } - // Add subcommands. + // Add subcommands. cmd.AddCommand(exportManifestCmd()) return cmd }Based on coding guidelines requiring documentation and performance tracking for exported functions.
tools/director/go.mod-133-133 (1)
133-133: Upgrade rardecode to patch DoS vulnerability.The dependency
github.com/nwaples/rardecode v1.1.3has a confirmed DoS vulnerability (GHSA-rwvp-r38j-9rgg / CVE-2025-11579). Versions up to v2.1.1 fail to restrict RAR dictionary sizes, risking OOM crashes when processing malformed RAR files. Upgrade to v2.2.0 or later to remediate.tools/director/cmd/show.go-16-89 (1)
16-89: Refactor to use the existing scene.LoadScenes function.The YAML loading logic is duplicated. Use
scene.LoadScenes(scenesFile)instead of manually reading and unmarshaling, which would simplify the code and align with the existing implementation intools/director/internal/scene/scene.go.tools/director/cmd/install.go-54-60 (1)
54-60: The cache clearing logic is incorrect.The comment states "Clear cache to force reinstall" but
LoadCache()actually reads the cache from disk rather than clearing it. The method loads the cache file usingos.ReadFile()and unmarshals it into memory—it does not clear or invalidate the cache. Whenforceis true, you should call a cache-clearing method instead, or if no such method exists in the manager, consider adding one.tools/director/internal/toolmgr/atmos.go-149-152 (1)
149-152: isAbs is not Windows-compatible.The isAbs helper only checks for leading
/, which won't work for Windows absolute paths likeC:\path. Per coding guidelines, code must be Linux/macOS/Windows compatible.🔎 Recommended fix
Use the standard library's filepath.IsAbs instead:
-// isAbs is a simple check for absolute paths. -func isAbs(path string) bool { - return len(path) > 0 && path[0] == '/' -} +import "path/filepath" + +// Use filepath.IsAbs throughout instead of this custom helper.Then update line 109:
- if !isAbs(candidate) { + if !filepath.IsAbs(candidate) {Based on coding guidelines: Code must be Linux/macOS/Windows compatible.
Committable suggestion skipped: line range outside the PR's diff.
tools/director/internal/backend/stream_test.go-216-222 (1)
216-222: Use errors.Is for error checking.The isErrorType helper uses string matching instead of errors.Is. Per coding guidelines, error checking should use errors.Is() for proper error chain inspection.
🔎 Suggested refactor
+import "errors" + // isErrorType checks if an error wraps a specific error type. func isErrorType(err, target error) bool { - if err == nil { - return target == nil - } - // Simple string-based check (can be replaced with errors.Is if errors are wrapped). - return contains(err.Error(), target.Error()) + return errors.Is(err, target) }Based on coding guidelines for **/*_test.go: use errors.Is() for error checking.
tools/director/internal/backend/stream_test.go-3-5 (1)
3-5: Missing errors import for proper error checking.To use errors.Is in the isErrorType helper, add the errors package to imports.
tools/director/internal/toolmgr/config.go-17-35 (1)
17-35: Use filepath.Join for path construction.Line 19 uses string concatenation with a hardcoded
/separator. Per coding guidelines, use filepath.Join() for cross-platform compatibility.🔎 Suggested fix
+import "path/filepath" + func LoadToolsConfig(demosDir string) (*ToolsConfig, error) { - defaultsFile := demosDir + "/defaults.yaml" + defaultsFile := filepath.Join(demosDir, "defaults.yaml")Based on coding guidelines: use filepath.Join() instead of hardcoded path separators.
tools/director/cmd/update_thumbnails.go-130-137 (1)
130-137: Potential division by zero.If
GetVideoDurationreturns 0.0 (e.g., for a corrupt file), this will produceInforNaN. Add a guard.Proposed fix
// Calculate percentage for Stream API. + if duration <= 0 { + fmt.Printf("⚠️ %s: invalid duration (%.2f)\n", name, duration) + continue + } thumbnailPct := thumbnailTime / durationCommittable suggestion skipped: line range outside the PR's diff.
tools/director/internal/vhs/renderer.go-189-200 (1)
189-200: Loop default logic is incorrect.The comment acknowledges the limitation, but the current code ignores any explicit
loop: falsein YAML since!loopis always true whenloopis its zero value. Consider using a pointer inAudioConfigor a different approach.One approach using explicit check
If modifying
AudioConfigisn't feasible, document thatloopalways defaults totrueand cannot be disabled, or use a sentinel value approach:- loop := sc.Audio.Loop - // Note: Zero value for bool is false, but we want default true. - // ... - if !loop { - // ... - loop = true - } + // Loop defaults to true. Only disable if explicitly set in the scene config. + // TODO: Consider using *bool in AudioConfig to distinguish unset from false. + loop := true + // For now, we always loop background audio.Or better - use a pointer in
scene.AudioConfig:Loop *bool `yaml:"loop,omitempty"`Then check
if sc.Audio.Loop != nil { loop = *sc.Audio.Loop }.tools/director/internal/backend/r2.go-16-17 (1)
16-17: Useerrors.Newfor sentinel errors.Per coding guidelines, static errors should be defined with
errors.New, notfmt.Errorf. The current form doesn't provide additional value.Proposed fix
+import "errors" + // ErrR2UploadFailed is returned when uploading to R2 fails. -var ErrR2UploadFailed = fmt.Errorf("failed to upload to R2") +var ErrR2UploadFailed = errors.New("failed to upload to R2")tools/director/internal/backend/r2.go-74-79 (1)
74-79: Inefficient byte-to-string conversion.
strings.NewReader(string(data))allocates a new string from the byte slice. Usebytes.NewReader(data)directly.Proposed fix
+import "bytes" + // Upload to R2. _, err = r.client.PutObject(ctx, &s3.PutObjectInput{ Bucket: aws.String(r.config.BucketName), Key: aws.String(key), - Body: strings.NewReader(string(data)), + Body: bytes.NewReader(data), ContentType: aws.String(contentType), })website/src/components/FeaturedDemoCarousel/index.tsx-24-42 (1)
24-42: Missing dependencies in useEffect - stale closure risk.
goToPreviousandgoToNextare called inside this effect but not listed in the dependency array. These functions referencecurrentIndexvia closure. Should include them or use refs.Proposed fix using useCallback dependencies
useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { if (!containerRef.current?.contains(document.activeElement) && document.activeElement !== document.body) { return; } if (e.key === 'ArrowLeft') { e.preventDefault(); goToPrevious(); } else if (e.key === 'ArrowRight') { e.preventDefault(); goToNext(); } }; window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); - }, [demos.length]); + }, [demos.length, goToPrevious, goToNext]);Also wrap
goToPreviousandgoToNextwithuseCallback:const goToPrevious = useCallback(() => { cancelPendingTransition(); transitionToIndex((currentIndex - 1 + demos.length) % demos.length); }, [currentIndex, demos.length]);tools/director/internal/backend/stream.go-52-58 (1)
52-58: HTTP client lacks timeout configuration.The default
http.Client{}has no timeout, which could cause hanging requests if the API is unresponsive.Proposed fix
func NewStreamBackend(config *StreamConfig) (*StreamBackend, error) { return &StreamBackend{ config: config, - client: &http.Client{}, + client: &http.Client{ + Timeout: 5 * time.Minute, // Generous timeout for video uploads + }, }, nil }Add
"time"to imports.tools/director/internal/backend/stream.go-220-255 (1)
220-255: POST is correct for the Cloudflare Stream API — verified against the official "Edit video details" endpoint documentation.The implementation properly handles context, JSON marshaling, and includes reasonable error messages. However, the code needs to align with the Go guidelines:
- Replace
fmt.Errorf()with static errors fromerrors/errors.go- Add
defer perf.Track(atmosConfig, "StreamBackend.SetThumbnailTime")()at the function start- Follow the error wrapping and handling patterns specified in the guidelines
tools/director/internal/vhs/cache.go-162-185 (1)
162-185: UpdateScene overwrites existing publish metadata.When updating a scene, the entire
SceneHashis replaced (lines 177-182), which losesPublishedAt,PublicURLs,StreamUID, andStreamVersionsif the scene was previously published.🔎 Proposed fix
- c.Scenes[sceneName] = SceneHash{ - TapeHash: hash, - AudioHash: audioHash, - RenderedAt: time.Now(), - Outputs: outputs, + existing, exists := c.Scenes[sceneName] + if !exists { + existing = SceneHash{} } + existing.TapeHash = hash + existing.AudioHash = audioHash + existing.RenderedAt = time.Now() + existing.Outputs = outputs + c.Scenes[sceneName] = existing
🟡 Minor comments (16)
example.tape-42-42 (1)
42-42: Fix duplicate Down key documentation.Line 42 and line 49 both document the Down key, creating redundancy in the Keys section. Remove one of the duplicate entries.
🔎 Proposed fix
- Down[@<time>] [number] Press the Down key Enter[@<time>] [number] Press the Enter key Space[@<time>] [number] Press the Space key Tab[@<time>] [number] Press the Tab key Left[@<time>] [number] Press the Left Arrow key Right[@<time>] [number] Press the Right Arrow key Up[@<time>] [number] Press the Up Arrow key Down[@<time>] [number] Press the Down Arrow keyAlso applies to: 49-49
internal/exec/vendor_component_utils.go-152-155 (1)
152-155: Comments must end with periods.Lines 152-155 contain comments that don't end with periods, violating the godot linter rule specified in the coding guidelines.
🔎 Proposed fix
- // OnDirExists handles existing directories at the destination. - // We skip .git directories from source, but if the destination already has a .git directory - // (from a previous vendor run), we need to leave it untouched to avoid permission errors - // on git packfiles which often have restrictive permissions. + // OnDirExists handles existing directories at the destination. + // We skip .git directories from source, but if the destination already has a .git directory + // (from a previous vendor run), we need to leave it untouched to avoid permission errors + // on git packfiles which often have restrictive permissions.Committable suggestion skipped: line range outside the PR's diff.
internal/exec/vendor_component_utils.go-573-576 (1)
573-576: Comments must end with periods.Lines 573-576 contain comments that don't end with periods, violating the godot linter rule specified in the coding guidelines.
🔎 Proposed fix
- // OnDirExists handles existing directories at the destination. - // If the destination already has a .git directory (from a previous vendor run), - // we need to leave it untouched to avoid permission errors on git packfiles - // which often have restrictive permissions. + // OnDirExists handles existing directories at the destination. + // If the destination already has a .git directory (from a previous vendor run), + // we need to leave it untouched to avoid permission errors on git packfiles + // which often have restrictive permissions.Committable suggestion skipped: line range outside the PR's diff.
internal/exec/vendor_utils.go-570-573 (1)
570-573: Comments must end with periods.Lines 570-573 contain comments that don't end with periods, violating the godot linter rule specified in the coding guidelines.
🔎 Proposed fix
- // OnDirExists handles existing directories at the destination. - // We skip .git directories from source, but if the destination already has a .git directory - // (from a previous vendor run), we need to leave it untouched to avoid permission errors - // on git packfiles which often have restrictive permissions. + // OnDirExists handles existing directories at the destination. + // We skip .git directories from source, but if the destination already has a .git directory + // (from a previous vendor run), we need to leave it untouched to avoid permission errors + // on git packfiles which often have restrictive permissions.internal/exec/copy_glob.go-482-485 (1)
482-485: Comments must end with periods.Lines 482-485 contain comments that don't end with periods, violating the godot linter rule specified in the coding guidelines.
🔎 Proposed fix
- // OnDirExists handles existing directories at the destination. - // If the destination already has a .git directory (from a previous vendor run), - // we need to leave it untouched to avoid permission errors on git packfiles - // which often have restrictive permissions. + // OnDirExists handles existing directories at the destination. + // If the destination already has a .git directory (from a previous vendor run), + // we need to leave it untouched to avoid permission errors on git packfiles + // which often have restrictive permissions.Committable suggestion skipped: line range outside the PR's diff.
test.tape-1-69 (1)
1-69: Move or remove test file from repository root.This test.tape file is in the root directory, but the project structure expects tape files in
demos/scenes/. Either move it to the proper location or remove it if it's just for local testing.website/src/data/manifest.json-506-515 (1)
506-515: Remove featured tag from scenes without videos.The
auth-listscene is tagged as "featured" but has an empty formats object (no video available). This could cause issues in the demo gallery UI or create a poor user experience when featured content is unavailable.Consider either rendering the video before tagging as featured, or removing the featured tag until the video is available.
demos/fixtures/acme/workflows/deploy.yaml-8-20 (1)
8-20: Document or include missing components.The main
deployworkflow describes itself as "Deploy all components to a stack" but only deploys vpc, cluster, database, and api. The specialized workflows reference additional components (cdn, frontend, cache) that aren't included in the full deployment.Consider either:
- Including all components in the main workflow
- Updating the description to clarify scope
- Adding comments explaining component selection
demos/fixtures/acme/atmos.yaml-40-49 (1)
40-49: Align deploy-stack with plan-all components.The
deploy-stackcommand deploys vpc → cluster → api, butplan-all(line 58-61) includes database. This mismatch could confuse users who runplan-allexpecting to see whatdeploy-stackwill deploy.Consider including database in deploy-stack or documenting why it's excluded.
website/src/components/FeaturedDemoCarousel/index.tsx-70-85 (1)
70-85: Nested setTimeout lacks cleanup for unmount during transition.If the component unmounts between the two
setTimeoutcalls,setCurrentIndexandsetIsTransitioningwill be called on an unmounted component. Consider tracking timeout IDs for cleanup.tools/director/internal/vhs/renderer.go-297-297 (1)
297-297: Shell execution not Windows-compatible.Same as in
render.go- usessh -cwhich won't work on Windows.tools/director/cmd/publish.go-411-425 (1)
411-425: Dry-run doesn't report unknown scenes.Unlike the main publish flow (lines 132-134), the dry-run silently ignores scenes that aren't found. Consider aligning behavior.
tools/director/cmd/render.go-291-298 (1)
291-298: Shell execution not Windows-compatible.Using
sh -cwill fail on Windows. Consider using a cross-platform approach or documenting this as a Unix-only tool.Possible cross-platform fix
- cmd := exec.CommandContext(ctx, "sh", "-c", cmdStr) + var cmd *exec.Cmd + if runtime.GOOS == "windows" { + cmd = exec.CommandContext(ctx, "cmd", "/C", cmdStr) + } else { + cmd = exec.CommandContext(ctx, "sh", "-c", cmdStr) + }Committable suggestion skipped: line range outside the PR's diff.
website/src/components/DemoGallery/index.tsx-187-192 (1)
187-192: Handle potential clipboard API failures.
navigator.clipboard.writeTextcan fail (permissions, insecure context). The rejected promise is currently unhandled.Proposed fix
const handleShare = async (demoId: string) => { const url = `${window.location.origin}/demos#${demoId}`; - await navigator.clipboard.writeText(url); - setCopied(true); - setTimeout(() => setCopied(false), 2000); + try { + await navigator.clipboard.writeText(url); + setCopied(true); + setTimeout(() => setCopied(false), 2000); + } catch (err) { + console.warn('Failed to copy to clipboard:', err); + } };tools/director/internal/vhs/cache.go-79-93 (1)
79-93: Missing directory creation.Unlike
toolmgr.SaveCache, this doesn't create the cache directory before writing. Will fail ifcacheDirdoesn't exist.🔎 Proposed fix
func (c *CacheMetadata) SaveCache(cacheDir string) error { metadataFile := filepath.Join(cacheDir, "metadata.json") + if err := os.MkdirAll(cacheDir, 0o755); err != nil { + return fmt.Errorf("failed to create cache directory: %w", err) + } + data, err := json.MarshalIndent(c, "", " ")pkg/ffmpeg/ffmpeg.go-388-391 (1)
388-391: Potential division by zero.When
sampleCount == 1, line 390 computespct = float64(i) / float64(sampleCount-1)which is division by zero.🔎 Proposed fix
for i := 0; i < sampleCount; i++ { // Calculate timestamp (evenly spaced in middle 80%). - pct := float64(i) / float64(sampleCount-1) + var pct float64 + if sampleCount > 1 { + pct = float64(i) / float64(sampleCount-1) + } timestamp := startTime + (sampleRange * pct)
🧹 Nitpick comments (39)
demos/scenes/_theme.tape (1)
1-11: Theme configuration works, consider readability.The Atmos theme and VHS settings are properly configured. Line 5's JSON theme definition is quite long (~400 chars) which makes it tough to scan. If VHS supports it, consider formatting the JSON across multiple lines or extracting to a separate JSON file.
demos/defaults.yaml (1)
48-48: Placeholder URL needs attention.The
base_urlcontainspub-xxxxx.r2.devwhich is clearly a placeholder. Consider adding a more explicit comment or using an environment variable pattern similar to the credentials.🔎 Suggested clarification
- base_url: "https://pub-xxxxx.r2.dev/demos" # Update with actual R2 public URL + base_url: "" # Required: Set to your R2 public URL (e.g., https://pub-xxxxx.r2.dev/demos)website/src/css/custom.css (1)
1002-1021: Nice metallic gradient effect.The gradient text effect for
.demos-titleworks well. Note thatbackground: ... textis a shorthand that may not work in all browsers. The-webkit-background-clip: textprovides good coverage, but you might want a fallback color.🔎 Consider adding a fallback
html[data-theme="dark"] .demos-title { + color: rgb(255, 255, 255); /* Fallback for browsers without background-clip support */ background: linear-gradient( to right bottom, rgb(255, 255, 255) 30%, rgba(255, 255, 255, 0.5) ) text; -webkit-background-clip: text; -webkit-text-fill-color: transparent; }tools/director/Makefile (2)
3-3: Add "all" to PHONY declaration.Static analysis correctly notes that "all" should be declared as PHONY since it doesn't produce a file named "all".
🔎 Fix
-.PHONY: build clean install test lint help +.PHONY: all build clean install test lint help
7-7: BUILD_DIR appears unused.
BUILD_DIRis set but only referenced incleanto remove$(BUILD_DIR)/build, while the binary is built to$(BINARY_NAME)directly. Consider removing or using it consistently.cmd/list/utils.go (1)
269-274: Consider error context when rendering to string fails.When
RenderToStringfails, the error is returned directly without additional context. Consider wrapping it to indicate this occurred during pager preparation.🔎 Suggested improvement for error context
// Get rendered content as string. content, err := r.RenderToString(data) if err != nil { - return err + return fmt.Errorf("failed to render content for pager: %w", err) }demos/fixtures/acme/stacks/catalog/cache.yaml (1)
14-14: Redis 7.1 is supported but consider Valkey for newer features.Redis 7.1 remains a valid, actively supported ElastiCache version. However, AWS now offers Valkey (a Redis-compatible fork) with versions 7.2+ as newer alternatives. If this fixture's purpose is to represent current best practices, Valkey 7.2 might be worth evaluating.
website/src/pages/demos.tsx (1)
5-29: Component works well.The DemosPage component is functional and properly structured. Consider extracting the inline styles to a CSS module or styled component for easier maintenance, but this is optional for now.
Optional: Extract inline styles to CSS module
You could create a
demos.module.cssfile and replace the inline styles for better separation of concerns, but the current implementation is perfectly fine for this use case.tools/director/cmd/root.go (1)
11-55: Minor: godot compliance.Comment at line 11 doesn't end with a period (godot linter requirement).
-// Execute runs the root command with Fang styling +// Execute runs the root command with Fang styling. func Execute() error {demos/fixtures/acme/.mock-states/setup.sh (1)
5-5: Optional: Consider adding set -u.Adding
set -uwould error on undefined variables, catching typos and improving robustness.set -e +set -udemos/fixtures/acme/atmos.yaml (1)
81-92: Consider adding a comment about placeholder values.The GitOps configuration uses placeholder values (account ID
111111111111). While appropriate for demo fixtures, adding a comment clarifying these are example values could help users understand this is template content.Optional clarification
# Integrations integrations: github: gitops: + # Example values - replace with your actual AWS resources opentofu-version: 1.8.0 terraform-version: 1.9.0tools/director/cmd/install.go (1)
67-68: Handle version retrieval error.The error from
mgr.Version("atmos")is silently ignored, which could result in confusing output like "✓ atmos v installed at..." if version retrieval fails.Suggested improvement
- version, _ := mgr.Version("atmos") - c.Printf("✓ atmos v%s installed at %s\n", version, path) + version, err := mgr.Version("atmos") + if err != nil { + version = "unknown" + } + c.Printf("✓ atmos v%s installed at %s\n", version, path)tools/director/cmd/new.go (2)
59-88: Consider aligning template with demos fixture structure.The generated template uses
examples/completeas the ATMOS_BASE_PATH (line 73), but this PR introducesdemos/fixtures/acmeas the demo structure. Consider updating the template to reference the actual demos fixture path or making it configurable.Suggested update
# Setup -Type "export ATMOS_BASE_PATH=examples/complete" +Type "export ATMOS_BASE_PATH=demos/fixtures/acme" Enter Sleep 500ms
106-117: Validate leading/trailing slashes in scene names.The validation allows slashes for subfolder scenes but doesn't check for leading or trailing slashes. Names like
/testortest/could create unexpected directory structures.Enhanced validation
func isValidSceneName(name string) bool { if len(name) == 0 { return false } // Allow slashes for subfolder scenes for _, ch := range name { if !((ch >= 'a' && ch <= 'z') || (ch >= '0' && ch <= '9') || ch == '-' || ch == '/') { return false } } - return !strings.HasPrefix(name, "-") && !strings.HasSuffix(name, "-") + return !strings.HasPrefix(name, "-") && !strings.HasSuffix(name, "-") && + !strings.HasPrefix(name, "/") && !strings.HasSuffix(name, "/") }website/src/data/manifest.json (1)
108-108: Thumbnail URL inconsistency.Two scenes (
terraform-planandlist-vars) have thumbnail URLs without the?time=X.Xsquery parameter, while all other scenes include it. This inconsistency might be intentional (using default thumbnail) but could be worth standardizing for consistency.Also applies to: 231-231
tools/director/internal/backend/r2_test.go (5)
8-37: Use t.Setenv for automatic cleanup.The test manually calls os.Unsetenv. Use t.Setenv instead, which automatically restores environment variables after the test.
🔎 Suggested refactor
func TestLoadR2Config_MissingCredentials(t *testing.T) { // Clear all R2 environment variables. - os.Unsetenv("CLOUDFLARE_ACCOUNT_ID") - os.Unsetenv("CLOUDFLARE_R2_ACCESS_KEY_ID") - os.Unsetenv("CLOUDFLARE_R2_SECRET_ACCESS_KEY") - os.Unsetenv("CLOUDFLARE_BUCKET_NAME") + t.Setenv("CLOUDFLARE_ACCOUNT_ID", "") + t.Setenv("CLOUDFLARE_R2_ACCESS_KEY_ID", "") + t.Setenv("CLOUDFLARE_R2_SECRET_ACCESS_KEY", "") + t.Setenv("CLOUDFLARE_BUCKET_NAME", "")
39-81: Use t.Setenv instead of manual cleanup.Replace the manual Setenv/defer Unsetenv pattern with t.Setenv for cleaner test code.
🔎 Suggested refactor
func TestLoadR2Config_Success(t *testing.T) { // Set required environment variables. - os.Setenv("CLOUDFLARE_ACCOUNT_ID", "test-account-id") - os.Setenv("CLOUDFLARE_R2_ACCESS_KEY_ID", "test-access-key") - os.Setenv("CLOUDFLARE_R2_SECRET_ACCESS_KEY", "test-secret-key") - defer func() { - os.Unsetenv("CLOUDFLARE_ACCOUNT_ID") - os.Unsetenv("CLOUDFLARE_R2_ACCESS_KEY_ID") - os.Unsetenv("CLOUDFLARE_R2_SECRET_ACCESS_KEY") - }() + t.Setenv("CLOUDFLARE_ACCOUNT_ID", "test-account-id") + t.Setenv("CLOUDFLARE_R2_ACCESS_KEY_ID", "test-access-key") + t.Setenv("CLOUDFLARE_R2_SECRET_ACCESS_KEY", "test-secret-key")
83-111: Same t.Setenv recommendation applies here.
113-139: Same t.Setenv recommendation applies here.
204-216: Helper duplication between test files.The contains and containsAt helpers are duplicated from stream_test.go. Consider moving these to a shared test helper file in the backend package (e.g., testing.go or helpers_test.go).
tools/director/cmd/catalog.go (1)
113-124: Consider making category order configurable.The category order is hardcoded (line 114). If the list of categories grows or changes frequently, consider moving this to a config file or constant at the package level for easier maintenance.
🔎 Optional refactor
// At package level: var defaultCategoryOrder = []string{ "flagship", "terraform", "demo-stacks", "auth", "dx", } // In function: categoryOrder := make([]string, len(defaultCategoryOrder)) copy(categoryOrder, defaultCategoryOrder)tools/director/cmd/render.go (1)
199-206: No validation for format filter values.Invalid format strings (e.g.,
--format jpg,webp) are silently accepted but won't match any scene outputs. Consider validating against known formats.tools/director/cmd/update_thumbnails.go (2)
110-111: Inconsistent output method.Uses
fmt.Printfthroughout whilerender.gousesc.Printf. For consistency with other commands in this package, consider using the Cobra command's print methods.Also applies to: 114-114, 119-119, 126-126, 139-139, 142-142, 149-149, 158-158, 169-173
160-161: Hardcoded rate limit delay.The 100ms delay is reasonable but might need tuning. Consider making it configurable or at least documenting the rationale.
website/src/components/Video/CloudflareStream.tsx (1)
41-48: Redundant loop assignment in handleInteraction.The useEffect on line 33-39 already handles updating
loopwhenhasInteractedchanges. Setting it again here is redundant since the state change will trigger the effect.Simplified version
const handleInteraction = () => { if (!hasInteracted) { setHasInteracted(true); - if (streamRef.current) { - streamRef.current.loop = loop; - } } };tools/director/internal/backend/r2.go (1)
60-65: Consider streaming for large files.Reading the entire file into memory before uploading could be problematic for large video files. Consider using
os.Openand passing the file handle directly.Streaming alternative
- // Read the file. - data, err := os.ReadFile(localPath) + // Open the file for streaming. + file, err := os.Open(localPath) if err != nil { - return fmt.Errorf("%w: failed to read file %s: %v", ErrR2UploadFailed, localPath, err) + return fmt.Errorf("%w: failed to open file %s: %v", ErrR2UploadFailed, localPath, err) } + defer file.Close() + + stat, err := file.Stat() + if err != nil { + return fmt.Errorf("%w: failed to stat file %s: %v", ErrR2UploadFailed, localPath, err) + } // Upload to R2. _, err = r.client.PutObject(ctx, &s3.PutObjectInput{ Bucket: aws.String(r.config.BucketName), Key: aws.String(key), - Body: bytes.NewReader(data), + Body: file, + ContentLength: aws.Int64(stat.Size()), ContentType: aws.String(contentType), })tools/director/cmd/publish.go (1)
348-350: Unused--backendflag.The flag is defined but marked as "not implemented". Consider removing it until implemented, or add a TODO comment and validation to reject its use.
website/src/components/DemoGallery/index.tsx (2)
66-72: Consider adding keyboard accessibility for cards.Interactive cards are missing keyboard event handlers. Users navigating with keyboards can't activate non-placeholder cards.
Proposed fix
return ( <div className={`${styles.card} ${demo.isPlaceholder ? styles.placeholder : ''}`} onClick={handleClick} onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave} + onKeyDown={(e) => { if (!demo.isPlaceholder && (e.key === 'Enter' || e.key === ' ')) { e.preventDefault(); onClick(); } }} + tabIndex={demo.isPlaceholder ? -1 : 0} + role="button" + aria-label={`Play ${demo.title}`} >
126-128: Add keyboard dismiss support for the modal.Modal should close on Escape key press for accessibility.
Proposed fix
Add an effect inside
DemoModalto handle Escape:useEffect(() => { const handleEscape = (e: KeyboardEvent) => { if (e.key === 'Escape') onClose(); }; document.addEventListener('keydown', handleEscape); return () => document.removeEventListener('keydown', handleEscape); }, [onClose]);tools/director/internal/scene/scene.go (1)
45-58: Wrap errors with context for better diagnostics.Per coding guidelines, errors should be wrapped with context using
fmt.Errorf("context: %w", err).Proposed fix
func LoadScenes(path string) (*ScenesList, error) { data, err := os.ReadFile(path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to read scenes file %s: %w", path, err) } var scenes ScenesList if err := yaml.Unmarshal(data, &scenes); err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse scenes YAML: %w", err) } return &scenes, nil }You'll need to add
"fmt"to the imports.website/src/data/demos.js (1)
113-122: Getter behavior with spread operator.When you spread
...demo, the getter is invoked and the result is copied as a regular property. The comment "Trigger getter" is accurate but slightly misleading since the spread itself triggers it. The explicitduration: demo.durationon line 117 is redundant after the spread.Simplify by removing redundant assignment
export function getAllDemos() { return demoCategories.flatMap(category => category.demos.map(demo => ({ ...demo, - duration: demo.duration, // Trigger getter categoryId: category.id, categoryTitle: category.title, })) ); }The spread already evaluates and copies the getter's value.
tools/director/internal/toolmgr/toolmgr.go (4)
29-35: SHA256 field declared but never populated.
InstalledTool.SHA256is defined but not set when updating cache inEnsureInstalled(lines 153-157). Either populate it for integrity verification or remove if unused.
78-95: Consider atomic writes for cache persistence.
os.WriteFileis non-atomic. If the process is interrupted mid-write, the cache file could be corrupted. The main atmos codebase uses atomic writes viawriteFileAtomic(seepkg/config/cache.go). For a director tool, this may be acceptable, but worth noting.
134-134: Direct stdout usage.Per coding guidelines, UI output should use
ui.*functions rather thanfmt.Printf. This applies to line 134 and line 163 as well.🔎 Suggested approach
Consider using a logger or UI package for status messages, or if this tool doesn't follow the main atmos UI conventions, document the deviation.
167-181: Duplicated default install directory logic.The
.cache/bindefault appears in bothEnsureInstalled(line 117) andGetPath(line 177). Consider extracting to a constant or helper.pkg/ffmpeg/ffmpeg.go (4)
55-59: Platform-specific install hint."brew install ffmpeg" only applies to macOS. Per guidelines, code should be Linux/macOS/Windows compatible. Consider a more generic message or platform detection.
🔎 Suggested change
if _, err := exec.LookPath("ffmpeg"); err != nil { - return fmt.Errorf("%w (install: brew install ffmpeg)", errUtils.ErrFFmpegNotFound) + return fmt.Errorf("%w (install ffmpeg from https://ffmpeg.org/download.html)", errUtils.ErrFFmpegNotFound) } if _, err := exec.LookPath("ffprobe"); err != nil { - return fmt.Errorf("%w (install: brew install ffmpeg)", errUtils.ErrFFprobeNotFound) + return fmt.Errorf("%w (install ffmpeg from https://ffmpeg.org/download.html)", errUtils.ErrFFprobeNotFound) }
117-119: Direct stdout/stderr usage.Piping ffmpeg output to
os.Stdout/os.Stderrmay cause issues in non-interactive contexts. Consider capturing output or using a configurable writer.
313-317: Heuristic may be noisy.The regex
\b[0-9]{1,3}\bmatches any 1-3 digit number in ffmpeg output, not specifically color values. As a last-resort fallback, this is acceptable but may produce inconsistent scores.
436-467: Missing perf.Track and error wrapping.Per guidelines, public functions need
perf.Track, and errors should be wrapped with context.🔎 Proposed fix
func DetectSceneChanges(ctx context.Context, videoPath string) ([]float64, error) { + defer perf.Track(nil, "ffmpeg.DetectSceneChanges")() + // Use ffmpeg's scene detection filter. cmd := exec.CommandContext(ctx, "ffmpeg", "-i", videoPath, "-vf", "select='gt(scene,0.3)',showinfo", "-f", "null", "-", ) output, err := cmd.CombinedOutput() if err != nil { - return nil, err + return nil, fmt.Errorf("scene detection failed: %w", err) }
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
1-1: Upgrade rardecode to address DoS vulnerability.Line 324 includes rardecode@1.1.3 (indirect), which has a known DoS vulnerability (GHSA-rwvp-r38j-9rgg / CVE-2025-11579). The library fails to restrict RAR dictionary sizes, allowing crafted RAR files to exhaust memory. Upgrade to v2.2.0 or later by updating the transitive dependency that pulls this in.
🧹 Nitpick comments (27)
pkg/list/extract/stacks.go (1)
97-119: Order instability from set-to-slice conversion.The set iteration at line 114 produces arbitrary ordering, losing any benefit from
component.ListTypes()returning a sorted list. ForStacksForComponent, this is fine. ForfindVarsFromComponents, it compounds the non-determinism.🔎 Stable ordering if needed
func getComponentTypes() []string { - typeSet := map[string]struct{}{ - config.TerraformComponentType: {}, - config.HelmfileComponentType: {}, - config.PackerComponentType: {}, - } - - for _, t := range component.ListTypes() { - typeSet[t] = struct{}{} - } - - types := make([]string, 0, len(typeSet)) - for t := range typeSet { - types = append(types, t) - } - - return types + // Built-in types first, in stable order. + types := []string{ + config.TerraformComponentType, + config.HelmfileComponentType, + config.PackerComponentType, + } + + seen := map[string]struct{}{ + config.TerraformComponentType: {}, + config.HelmfileComponentType: {}, + config.PackerComponentType: {}, + } + + // Registry types are already sorted. + for _, t := range component.ListTypes() { + if _, exists := seen[t]; !exists { + types = append(types, t) + seen[t] = struct{}{} + } + } + + return types }pkg/list/extract/stacks_test.go (1)
255-281: Consider adding multi-component test.If the non-deterministic behavior in
findVarsFromComponentsis addressed, a test with multiple components having different vars would strengthen coverage. Currently such a test would be flaky.internal/exec/describe_stacks.go (2)
1076-1103: Consider usingfilepath.Joinfor path construction.Using
fmt.Sprintfwith hardcoded/separators works for config output, butfilepath.Joinwould align with the codebase conventions. If you intentionally want forward slashes in output regardless of OS, you could usepath.Join(frompathpackage) which always uses forward slashes.🔎 Optional refactor using path.Join for consistent forward slashes
+ import "path" ... case cfg.TerraformSectionName: basePath := atmosConfig.Components.Terraform.BasePath if componentFolderPrefix != "" { - relativePath = fmt.Sprintf("%s/%s/%s", basePath, componentFolderPrefix, finalComponent) + relativePath = path.Join(basePath, componentFolderPrefix, finalComponent) } else { - relativePath = fmt.Sprintf("%s/%s", basePath, finalComponent) + relativePath = path.Join(basePath, finalComponent) }Apply similar changes to Helmfile and Packer cases.
1082-1087: Edge case: empty BasePath produces leading slash.If
atmosConfig.Components.Terraform.BasePathis an empty string, the resulting path would be/componentor//prefix/component. If relative paths are expected, you might want to handle this edge case.🔎 Optional: Handle empty BasePath
case cfg.TerraformSectionName: basePath := atmosConfig.Components.Terraform.BasePath + if basePath == "" { + basePath = "components/terraform" + } if componentFolderPrefix != "" {tools/director/internal/backend/r2_config.go (1)
90-93: GetEndpoint may produce invalid URL if AccountID is empty.If
LoadR2Configvalidation is bypassed or the config is constructed directly,GetEndpoint()would returnhttps://.r2.cloudflarestorage.com. SinceLoadR2Configvalidates AccountID presence, this is likely safe in practice, but consider defensive validation here.🔎 Optional defensive check
// GetEndpoint returns the S3-compatible endpoint URL for Cloudflare R2. func (c *R2Config) GetEndpoint() string { + if c.AccountID == "" { + return "" + } return fmt.Sprintf("https://%s.r2.cloudflarestorage.com", c.AccountID) }demos/scenes/workflow/list.tape (1)
7-7: Consider simplifying the theme path.The path
../../scenes/_theme.tapecorrectly resolves todemos/scenes/_theme.tape. However, since this file is already indemos/scenes/workflow/, the simpler path../_theme.tapeworks and is cleaner.tools/director/scripts/fix-svg-lineheight.sh (1)
24-36: gawk-specific syntax may break on some systems.The
match($0, /pattern/, arr)with a third argument for captures is a GNU awk extension. Systems with mawk or BSD awk will fail here.Additionally, the sed→sed→awk pipeline is convoluted. The first sed produces literal
$(...)strings that the second sed partially reverses, only for awk to do the actual work.🔎 Simplified approach using awk alone
-# Process the SVG - adjust all y="N" attributes -sed -E "s/y=\"([0-9]+)\"/y=\"\$(echo \"scale=0; \1 * $SCALE / 1\" | bc)\"/g" "$INPUT" | \ - while IFS= read -r line; do - # Evaluate the bc expressions - echo "$line" | sed -E 's/\$\(echo "scale=0; ([0-9]+) \* ([0-9.]+) \/ 1" \| bc\)/\1/g' | \ - awk -v scale="$SCALE" '{ - while (match($0, /y="([0-9]+)"/, arr)) { - old_y = arr[1] - new_y = int(old_y * scale) - gsub(/y="[0-9]+"/, "y=\"" new_y "\"", $0) - } - print - }' - done > "$OUTPUT" +# Process the SVG - adjust all y="N" attributes using POSIX-compatible awk +awk -v scale="$SCALE" '{ + while (match($0, /y="[0-9]+"/)) { + prefix = substr($0, 1, RSTART + 2) + rest = substr($0, RSTART + 3) + match(rest, /[0-9]+/) + old_y = substr(rest, RSTART, RLENGTH) + new_y = int(old_y * scale) + $0 = prefix new_y "\"" substr(rest, RSTART + RLENGTH + 1) + } + print +}' "$INPUT" > "$OUTPUT"demos/scenes/flagship/onboarding.tape (1)
47-55: Consider clarifying the narrative.The comment "Initialize a new project" suggests an initialization step, but the command just displays help text. Consider updating the comment to match the actual action, e.g., "Explore available options" or "See full command list."
demos/scenes/commands.tape (1)
50-53: Optional: Remove trailing empty line.Line 51 has an extra blank line before the final hold. While harmless, removing it would make the formatting more consistent with other tape scripts.
demos/scenes/terraform/init.tape (1)
38-41: Optional: Extra blank line before final hold.Lines 39-40 create an extra blank line before the final hold. Consider removing one for consistency with other tape scripts.
tools/director/internal/scene/scene_test.go (2)
9-59: Consider refactoring to table-driven test format.Per coding guidelines, prefer table-driven tests for multiple scenarios. These two test functions could be combined into a single table-driven test with cases for synthetic tape data and optional real file testing.
🔎 Example table-driven structure
func TestCalculateTapeDuration(t *testing.T) { tests := []struct { name string setup func(t *testing.T) string expectMin float64 expectExact *float64 skipIf func() bool }{ { name: "synthetic tape with known sleeps", setup: func(t *testing.T) string { tmpDir := t.TempDir() tapePath := filepath.Join(tmpDir, "test.tape") tapeContent := `# Test tape Sleep 2s Sleep 500ms Sleep 3s Sleep 1.5s ` if err := os.WriteFile(tapePath, []byte(tapeContent), 0o644); err != nil { t.Fatalf("failed to write test tape: %v", err) } return tapePath }, expectExact: func() *float64 { v := 7.0; return &v }(), }, { name: "vendor pull tape", setup: func(t *testing.T) string { return "../../../../demos/scenes/vendor/pull.tape" }, expectMin: 20.0, skipIf: func() bool { _, err := os.Stat("../../../../demos/scenes/vendor/pull.tape") return os.IsNotExist(err) }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if tt.skipIf != nil && tt.skipIf() { t.Skip("tape file not found") } tapePath := tt.setup(t) duration, err := CalculateTapeDuration(tapePath) if err != nil { t.Fatalf("CalculateTapeDuration failed: %v", err) } if tt.expectExact != nil && duration != *tt.expectExact { t.Errorf("expected duration %.1f, got %.1f", *tt.expectExact, duration) } if tt.expectMin > 0 && duration < tt.expectMin { t.Errorf("expected duration >= %.1fs, got %.1f", tt.expectMin, duration) } }) } }Based on coding guidelines: "Use table-driven tests for testing multiple scenarios in Go."
55-58: Fragile assertion on vendor tape duration.The test expects vendor pull tape duration >= 20 seconds, but this depends on the actual tape content which could change. Consider whether this test adds value or if it's testing implementation details that could break unnecessarily.
tools/director/internal/validation/validation_test.go (1)
204-223: Consider adding boundary test for maxLen <= 3.The truncate function appends "..." (3 chars). If
maxLenis 3 or less, the output could be unexpected. Worth adding a test case to document behavior.Suggested additional test case
tests := []struct { input string maxLen int expected string }{ {"short", 10, "short"}, {"exactly10c", 10, "exactly10c"}, {"this is a longer string", 10, "this is..."}, {"abc", 5, "abc"}, {"", 5, ""}, + {"longstring", 3, "..."}, // edge case: maxLen equals ellipsis length }tools/director/internal/toolmgr/atmos.go (2)
28-89: Consider adding timeout and checksum verification for downloads.Using
http.DefaultClientwithout a timeout could hang indefinitely on network issues. Also, downloading binaries without checksum verification is a supply-chain risk.Suggested improvements
+import "time" // installAtmosFromGitHub downloads atmos from GitHub releases. func (m *Manager) installAtmosFromGitHub(ctx context.Context, version, destPath string) error { + // Create client with timeout. + client := &http.Client{Timeout: 5 * time.Minute} + // ... existing code ... // Execute request. - resp, err := http.DefaultClient.Do(req) + resp, err := client.Do(req) if err != nil { return fmt.Errorf("failed to download atmos: %w", err) }For checksums, GitHub releases typically include SHA256 sums that could be fetched and verified.
34-42: Windows support not included.The arch mapping and binary naming don't account for Windows (
.exesuffix). If Windows support is needed, consider:Windows extension handling
+ // Add .exe suffix on Windows. + binaryName := fmt.Sprintf("atmos_%s_%s_%s", version, platform, releaseArch) + if platform == "windows" { + binaryName += ".exe" + } + // Build download URL. - url := fmt.Sprintf( - "https://github.com/cloudposse/atmos/releases/download/v%s/atmos_%s_%s_%s", - version, version, platform, releaseArch, - ) + url := fmt.Sprintf( + "https://github.com/cloudposse/atmos/releases/download/v%s/%s", + version, binaryName, + )tools/director/cmd/catalog.go (1)
115-115: Consider extracting shared category order.The category order list here differs from
listCategoriesCmd(line 293), which could cause inconsistent ordering across commands. Extract to a package-level constant for consistency.tools/director/cmd/publish.go (2)
400-402: Remove or implement the unused--backendflag.The flag is declared but annotated as "not implemented." Either implement the functionality or remove the flag to avoid user confusion.
151-157: Map iteration produces non-deterministic output order.Iterating over
scenesToPublishmap gives inconsistent ordering across runs. For reproducible output, sort scene names before processing.🔎 Suggested approach
// Sort scene names for deterministic output. var sortedNames []string for name := range scenesToPublish { sortedNames = append(sortedNames, name) } sort.Strings(sortedNames) for _, sceneName := range sortedNames { sc := scenesToPublish[sceneName] // ... rest of loop }website/src/components/FeaturedDemoCarousel/index.tsx (1)
31-49: Consider stabilizing navigation functions in useEffect deps.The keydown handler calls
goToPreviousandgoToNextbut deps only includedemos.length. Since these functions capturecurrentIndex, this works but is fragile. Wrapping them inuseCallbackwith proper deps or includingcurrentIndexin this effect's deps would be cleaner.tools/director/internal/validation/validation.go (3)
32-45:Result.Sceneis never populated.The
Scenefield remains empty. If this is intentional (caller sets it), consider adding a comment. Otherwise, you may want to accept a scene name parameter.
84-88: Consider pre-compiling the regex.
tspanReis compiled on every call. For repeated validation, a package-levelvarwould be more efficient. Low priority for a CLI tool.🔎 Proposed fix
+var tspanRe = regexp.MustCompile(`<tspan[^>]*>([^<]*)</tspan>`) + func ExtractTextFromSVG(svg string) string { - tspanRe := regexp.MustCompile(`<tspan[^>]*>([^<]*)</tspan>`) matches := tspanRe.FindAllStringSubmatch(svg, -1)
126-132: Byte-based truncation may split UTF-8 characters.Using
len(s)counts bytes. For multi-byte characters, this could produce invalid UTF-8 at the boundary. Acceptable for ASCII-heavy terminal output, but considerutf8.RuneCountInStringif internationalized text is expected.tools/director/internal/scene/scene.go (2)
75-88: Wrap errors with context.Per coding guidelines, errors should be wrapped. Currently, raw errors from
ReadFileandUnmarshalare returned without context about the file path.🔎 Proposed fix
func LoadScenes(path string) (*ScenesList, error) { data, err := os.ReadFile(path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to read scenes file %s: %w", path, err) } var scenes ScenesList if err := yaml.Unmarshal(data, &scenes); err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse scenes file %s: %w", path, err) } return &scenes, nil }Add
"fmt"to imports.
94-133: Consider wrapping errors with context.Lines 97 and 132 return raw errors. Adding context (e.g., tape file path) would help debugging.
🔎 Proposed fix
func CalculateTapeDuration(tapePath string) (float64, error) { file, err := os.Open(tapePath) if err != nil { - return 0, err + return 0, fmt.Errorf("failed to open tape %s: %w", tapePath, err) } defer file.Close() ... if err := scanner.Err(); err != nil { - return 0, err + return 0, fmt.Errorf("failed to scan tape %s: %w", tapePath, err) }tools/director/internal/vhs/renderer.go (2)
476-498:maxYis computed but never used.The variable
maxYis tracked across all replacements (lines 496-498, 514-516, 533-535) but never utilized. The comment mentions calculating new height, but no height adjustment occurs. Either complete the feature or remove the dead code.
482-542: Consider pre-compiling regexes.Four regexes are compiled on each call. For multiple SVG files, pre-compiling as package-level vars would be more efficient. Low priority for a CLI tool.
tools/director/internal/toolmgr/toolmgr.go (1)
110-113: LoadCache is called on every EnsureInstalled invocation.This reads from disk each time. If
EnsureInstalledis called multiple times in a loop, consider caching the load result (check if already loaded).🔎 Proposed fix
// Load cache to check if already installed. -if err := m.LoadCache(); err != nil { - return "", err +if len(m.cache.Tools) == 0 { + if err := m.LoadCache(); err != nil { + return "", err + } }
| loop := sc.Audio.Loop | ||
| // Note: Zero value for bool is false, but we want default true. | ||
| // If Audio struct was just created, all fields are zero. | ||
| // We'll assume if the Audio config exists, loop should default to true. | ||
| // This is a limitation of the zero-value defaults in Go. | ||
| // Better approach: use pointers for optional fields, but keeping it simple for now. | ||
| if !loop { | ||
| // Check if this is intentionally false or just zero-value. | ||
| // For simplicity, we'll always default to true unless explicitly set. | ||
| // This requires the YAML to explicitly say "loop: false" to disable. | ||
| loop = true | ||
| } |
There was a problem hiding this comment.
Bug: loop: false is ignored and always becomes true.
The code at line 326-331 unconditionally sets loop = true when !loop, which overrides an explicit loop: false from YAML. The comment acknowledges this limitation but doesn't fix it.
Consider using a pointer (*bool) in AudioConfig to distinguish between unset and explicitly false.
🔎 Proposed approach
In scene.AudioConfig:
type AudioConfig struct {
Source string `yaml:"source"`
Volume float64 `yaml:"volume,omitempty"`
FadeOut float64 `yaml:"fade_out,omitempty"`
- Loop bool `yaml:"loop,omitempty"`
+ Loop *bool `yaml:"loop,omitempty"`
}Then in the renderer:
- loop := sc.Audio.Loop
- if !loop {
- loop = true
- }
+ loop := true // default
+ if sc.Audio.Loop != nil {
+ loop = *sc.Audio.Loop
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tools/director/internal/vhs/renderer.go around lines 320 to 331, the renderer
currently treats any false zero-value as "unset" and forces loop = true, which
overrides an explicit YAML "loop: false"; change Audio config to use a *bool
(pointer) so you can distinguish unset vs explicitly false, update the struct
definition and YAML unmarshalling accordingly, then in this renderer check if
sc.Audio.Loop == nil -> default to true, else use *sc.Audio.Loop so an explicit
false is preserved; adjust any callers/tests to construct the pointer where
needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
demos/scenes.yaml (1)
23-23: Missing audio file remains unresolved.As previously flagged, every scene references
audio/background.mp3which doesn't exist atdemos/audio/background.mp3. This must be resolved before the scenes can render with audio.
🧹 Nitpick comments (1)
demos/scenes.yaml (1)
22-26: Consider using YAML anchors to reduce audio configuration repetition.The same audio configuration appears in every scene. Using YAML anchors would make maintenance easier if you need to adjust these settings globally.
🔎 Example refactor using YAML anchors
Define the audio configuration once at the top of the file:
--- version: "1.0" + +# Audio configuration shared across all scenes +audio-defaults: &default-audio + source: "audio/background.mp3" + volume: 0.15 + fade_out: 2.0 + loop: true scenes:Then reference it in each scene:
outputs: - gif - png - mp4 - svg - audio: - source: "audio/background.mp3" - volume: 0.15 - fade_out: 2.0 - loop: true + audio: *default-audio tags: - describe
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
demos/fixtures/acme/atmos.yamldemos/scenes.yamldemos/scenes/auth/login.tapedemos/scenes/docs.tapedemos/scenes/profile.tapedemos/scenes/theme.tapetools/director/internal/vhs/renderer.go
✅ Files skipped from review due to trivial changes (1)
- demos/fixtures/acme/atmos.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- demos/scenes/profile.tape
- demos/scenes/theme.tape
- tools/director/internal/vhs/renderer.go
- demos/scenes/docs.tape
- demos/scenes/auth/login.tape
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (2)
demos/scenes.yaml (2)
1-4: LGTM!Schema reference and versioning are in place for proper validation and future evolution.
10-33: Well-structured scene inventory.The scene schema is consistent and comprehensive. Each entry includes all necessary metadata for rendering, validation, and gallery presentation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
demos/scenes.yaml (1)
1-1048: Acknowledge the prior review comment on missing audio file.A previous review already flagged that
audio/background.mp3is missing. Since this is a known issue from the prior review, I won't duplicate that comment here.
🧹 Nitpick comments (4)
demos/fixtures/acme/.devcontainer/devcontainer.json (1)
3-3: Consider pinning the Geodesic image version.Using
:latestcan lead to environment drift as the upstream image changes. Pinning to a specific version or digest improves reproducibility across the team.🔎 Suggested approach
- "image": "cloudposse/geodesic:latest", + "image": "cloudposse/geodesic:2.0.0",Replace
2.0.0with the current stable version you intend to use.cmd/list/aliases.go (1)
36-37: Consider embedding examples from markdown files.Per coding guidelines, examples should be embedded from
cmd/markdown/*_usage.mdusing//go:embedand rendered withutils.PrintfMarkdown()rather than inline strings. This ensures consistency with other commands and allows documentation to be maintained separately.Based on coding guidelines: "Embed examples from cmd/markdown/*_usage.md using //go:embed and render with utils.PrintfMarkdown()".
cmd/list/aliases_test.go (2)
128-135: Use stdlib instead of custom indexOf.The custom
indexOfhelper duplicates functionality already available instrings.Index()from the standard library.🔎 Replace with stdlib function
-// indexOf returns the index of the first occurrence of substr in s. -func indexOf(s, substr string) int { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return i - } - } - return -1 -}Then update the usage in TestFormatAliasesTableSortOrder:
+ "strings" + // Check that alpha appears before mike and mike appears before zulu. - alphaIdx := indexOf(output, "alpha") - mikeIdx := indexOf(output, "mike") - zuluIdx := indexOf(output, "zulu") + alphaIdx := strings.Index(output, "alpha") + mikeIdx := strings.Index(output, "mike") + zuluIdx := strings.Index(output, "zulu")
42-45: TestAliasesOptions could be more comprehensive.The current test only verifies struct instantiation. Consider testing actual flag parsing and option initialization to provide more meaningful coverage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
cmd/list/aliases.gocmd/list/aliases_test.gocmd/list/list.godemos/fixtures/acme/.devcontainer/devcontainer.jsondemos/fixtures/acme/atmos.yamldemos/fixtures/acme/profiles/ci/settings.yamldemos/fixtures/acme/profiles/developer/settings.yamldemos/fixtures/acme/profiles/production/settings.yamldemos/scenes.yamldemos/scenes/aliases.tapedemos/scenes/commands.tapedemos/scenes/completion.tapedemos/scenes/devcontainer.tapedemos/scenes/profile.tape
✅ Files skipped from review due to trivial changes (1)
- demos/fixtures/acme/profiles/developer/settings.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- demos/fixtures/acme/atmos.yaml
- demos/scenes/completion.tape
- demos/scenes/devcontainer.tape
- demos/scenes/commands.tape
- demos/scenes/aliases.tape
🧰 Additional context used
📓 Path-based instructions (4)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: Commands MUST use flags.NewStandardParser() for command-specific flags - NEVER call viper.BindEnv() or viper.BindPFlag() directly
Embed examples from cmd/markdown/*_usage.md using //go:embed and render with utils.PrintfMarkdown()
Files:
cmd/list/list.gocmd/list/aliases_test.gocmd/list/aliases.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
cmd/list/list.gocmd/list/aliases_test.gocmd/list/aliases.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesUse table-driven tests for comprehensive coverage and target >80% coverage
Files:
cmd/list/aliases_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS use cmd.NewTestKit(t) for cmd tests to auto-clean RootCmd state (flags, args)
Files:
cmd/list/aliases_test.go
🧠 Learnings (12)
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Applied to files:
cmd/list/list.gocmd/list/aliases_test.gocmd/list/aliases.go
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
cmd/list/list.gocmd/list/aliases.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Applied to files:
cmd/list/list.gocmd/list/aliases_test.gocmd/list/aliases.go
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
cmd/list/list.gocmd/list/aliases_test.gocmd/list/aliases.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
cmd/list/list.gocmd/list/aliases_test.gocmd/list/aliases.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
cmd/list/aliases_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
cmd/list/aliases_test.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*_test.go : Use table-driven tests for comprehensive coverage and target >80% coverage
Applied to files:
cmd/list/aliases_test.go
📚 Learning: 2025-12-10T18:32:43.260Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:43.260Z
Learning: In cmd subpackages, tests should avoid using NewTestKit(t) unless tests actually interact with RootCmd (e.g., execute commands via RootCmd or modify RootCmd state). For structural tests that only verify command structure/flags without touching RootCmd, TestKit cleanup is unnecessary.
Applied to files:
cmd/list/aliases_test.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
cmd/list/aliases.go
📚 Learning: 2025-02-03T05:57:18.407Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/cmd_utils.go:121-148
Timestamp: 2025-02-03T05:57:18.407Z
Learning: The Atmos CLI should fail fast (exit) when encountering configuration errors, including command alias configuration issues, to prevent undefined behavior. Use LogErrorAndExit instead of returning errors in such cases.
Applied to files:
cmd/list/aliases.go
📚 Learning: 2025-10-14T01:54:48.410Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-completion--help.html:2-7
Timestamp: 2025-10-14T01:54:48.410Z
Learning: Screengrab HTML files in website/src/components/Screengrabs/ are generated from actual Atmos CLI output converted to HTML. The ANSI-art headers and formatting in these files are intentional and reflect the real CLI user experience, so they should not be suggested for removal or modification.
Applied to files:
demos/scenes/profile.tape
🧬 Code graph analysis (1)
cmd/list/aliases_test.go (3)
cmd/cmd_utils.go (1)
Contains(1257-1264)cmd/list/aliases.go (1)
AliasesOptions(27-29)pkg/schema/command.go (1)
CommandAliases(49-49)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Summary
🔇 Additional comments (17)
demos/fixtures/acme/.devcontainer/devcontainer.json (1)
8-9: Good security practice.Mounting sensitive credentials as readonly prevents accidental modification and limits the blast radius if the container is compromised.
demos/fixtures/acme/profiles/ci/settings.yaml (1)
1-16: Solid CI profile configuration.The settings are well-suited for CI/CD environments—disabling visual features keeps logs clean, and Info-level logging strikes a good balance for troubleshooting without overwhelming output.
demos/fixtures/acme/profiles/production/settings.yaml (2)
1-16: Production settings look appropriate.Enabling visual features makes sense for human operators, and Warning-level logging reduces noise while still capturing important events.
18-20: Excellent safety guard for production deployments.Setting
apply_auto_approve: falseprevents accidental auto-approval of Terraform changes in production—this is exactly the kind of guardrail you want.demos/scenes.yaml (2)
1-1048: Well-structured scene manifest.The YAML is cleanly organized with consistent structure across all 42 scenes. The categorization (demo-stacks, terraform, list, workflows, vendor, dx, auth, config, flagship) provides clear organization, and the separation between gallery-enabled and docs-only scenes is well-defined. The uniform audio settings and output formats across scenes will simplify maintenance.
354-355: Prep script confirmed. Thelist-varsscene's prep script at.mock-states/setup.shexists and is executable (rwxr-xr-x).cmd/list/aliases.go (5)
32-40: Help text is clear and follows conventions.The command has appropriate Short, Long, and Example fields. The help text clearly describes what the command does and provides usage examples.
42-53: Flag initialization follows the standard pattern.Properly creates a StandardParser, registers flags, and binds to Viper with appropriate error handling in init(). This aligns with the required flag parsing approach.
88-98: TTY detection and output handling is well implemented.The function correctly detects TTY mode and provides appropriate formatting for both interactive terminals and piped/redirected output.
100-134: Formatting functions are clean and consistent.Both
formatAliasesTableandformatSimpleAliasesOutput(lines 136-165) properly sort aliases alphabetically and handle singular/plural messaging. The themed table output for TTY mode enhances user experience.
75-75: This follows the correct error handling pattern.The code uses
fmt.Errorfwith%wto add string context to the underlying error, which aligns with the coding guidelines: "Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking". A static error for general config loading failures doesn't exist inerrors/errors.go(onlyErrMissingAtmosConfigexists, which is for "not found or invalid" specifically). The current approach is correct.Likely an incorrect or invalid review comment.
cmd/list/list.go (1)
35-35: Clean integration of aliases subcommand.The aliasesCmd is properly registered alongside other list subcommands, following the established pattern.
cmd/list/aliases_test.go (2)
13-25: Command structure tests are appropriate.The test validates command metadata and argument handling without needing NewTestKit since it doesn't interact with RootCmd state.
48-125: Formatting tests provide good coverage.The tests cover both table and simple output formats, verify singular/plural messaging, and check alphabetical sorting. This provides solid coverage of the formatting logic.
demos/scenes/profile.tape (3)
1-9: LGTM! Clean configuration.The output formats and VHS settings are well-structured. The 1400x800 dimensions provide good viewing resolution, and sourcing the shared theme ensures consistency across demos.
11-19: Nice prompt handling.The Hide/Show pattern for prompt setup keeps the demo output clean, and the custom PS1 with RGB color codes maintains visual consistency. Requiring
atmosupfront ensures the demo fails fast if dependencies are missing.
21-75: Profile demo sequence is solid and ready.All referenced profiles (
developer,ci,production) are properly configured indemos/fixtures/acme/profiles/, theatmos profilecommands are fully implemented, the--profileflag works across list operations, andATMOS_PROFILEenvironment variable support is in place. Auto-discovery will find your profiles without additional configuration. The timing (300ms–3s sleeps) is appropriate, and the 10s final hold works well for video looping.
Add comprehensive background audio support for MP4 outputs in the VHS demo video infrastructure, with automatic looping, fade-out effects, and smart caching. ## New Packages - **pkg/vhs/** - VHS command execution wrapper - CheckInstalled() - Verify VHS availability - Render() - Execute VHS with context support - **pkg/ffmpeg/** - FFmpeg command execution for audio processing - CheckInstalled() - Verify FFmpeg/ffprobe availability - GetVideoDuration() - Get video duration using ffprobe - MergeAudioWithVideo() - Merge audio with looping and fade-out ## Schema Extensions Extended Scene struct with AudioConfig: - source: Path to MP3 file (relative to demos/) - volume: Volume level (0.0-1.0, default 0.3) - fade_out: Fade-out duration in seconds (default 2.0) - loop: Loop audio if shorter than video (default true) ## Key Features - Audio only applies to MP4 outputs (GIF remains silent) - Automatic looping for seamless background music - Fade-out prevents abrupt audio cutoff - Cache tracks both tape and audio file hashes - FFmpeg validation in both validate and render commands - Comprehensive error messages with installation instructions ## Implementation - Refactored VHS renderer to use new pkg/vhs package - Added audio post-processing after VHS generation - Updated cache to track audio file changes - Added validation for audio files and MP4 output requirement - Updated Brewfile comment for FFmpeg dual purpose - Documented feature in PRD with examples 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add Atmos TUI (flagship-tui) as featured demo showing interactive command/stack/component exploration - Configure FFmpeg for audio processing and video rendering - Add Cloudflare Stream and R2 storage backends for video/image hosting - Set up demo directory caching and tool version management - Reorder flagship demos with TUI as primary entry point 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Extract magic numbers into named constants - Fix godot comment capitalization - Export DetectSceneChanges to avoid unused function warning 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ponents - Add normalizeComponentQuery() to auto-wrap queries in array brackets - Support simplified select() syntax: `select(.locked == true)` instead of `[.[] | select(.locked == true)]` - Detect and reject scalar extraction queries with helpful error message - Add log.Trace when query is normalized for debugging - Add comprehensive tests for query normalization and filtering - Update VHS demo tape to use simplified query syntax - Update documentation with examples of simplified syntax - Add ErrScalarExtractionNotSupported and ErrQueryUnexpectedResultType sentinels 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… rendering Adds the ability to render all scenes within a specific category at once: - `director render --category terraform --force --publish` - `director render --category list --format svg` Also adds: - `director list categories` command to show available categories with counts - `--category` filter for `director list scenes` - Restructures list command as parent with scenes/categories subcommands 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update the config-describe-affected demo to showcase the --query flag for filtering describe affected output to specific components using yq expressions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @demos/scenes/toolchain.tape:
- Line 7: The import in demos/scenes/toolchain.tape references the wrong
relative path ("Source ../../scenes/_theme.tape"); update that line in
toolchain.tape (the line containing "Source ../../scenes/_theme.tape") to the
correct local path, e.g., "Source ./_theme.tape" or "Source _theme.tape", so it
correctly loads _theme.tape from the same demos/scenes directory.
In @tools/director/cmd/export_manifest.go:
- Around line 166-175: The code currently appends existingScene unchanged when
isPublishedInCache is false, losing updates from scenes.yaml; modify the block
handling !isPublishedInCache so that instead of appending existingScene verbatim
you merge selective metadata fields from manifestScene into existingScene (at
least Description, Tags, Category, Title, Order, FeaturedOrder), preserving
published format data from existingScene, then append the merged scene to
manifest.Scenes; update the logic around hasExistingFormats/isPublishedInCache
to use the merged object in place of existingScene so metadata edits are
reflected without discarding published format info.
🧹 Nitpick comments (1)
tools/director/cmd/export_manifest.go (1)
48-68: Consider unexporting and adding error context.This function is only used within the package. Consider making it unexported (
loadExistingManifest→loadExistingManifestis fine, but if it's internal, lowercase it). Also, the JSON unmarshal error could include the file path for easier debugging.Suggested improvement
-func loadExistingManifest(path string) (*Manifest, error) { +func loadExistingManifest(path string) (*Manifest, error) { if path == "" || path == "-" { return nil, nil } data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { return nil, nil // No existing manifest. } return nil, err } var manifest Manifest if err := json.Unmarshal(data, &manifest); err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse manifest %s: %w", path, err) } return &manifest, nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
demos/scenes/config/describe-affected.tapedemos/scenes/toolchain.tapetools/director/cmd/export_manifest.gowebsite/src/data/manifest.json
🚧 Files skipped from review as they are similar to previous changes (1)
- demos/scenes/config/describe-affected.tape
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter) in Go code
Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (cfg,log,u,errUtils)
All errors MUST be wrapped using static errors defined inerrors/errors.go- useerrors.Joinfor combining errors,fmt.Errorfwith%wfor context, anderrors.Is()for error checking
Never manually create mocks - usego.uber.org/mock/mockgenwith//go:generatedirectives in Go code
Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use//revive:disable:file-length-limit
Use colors frompkg/ui/theme/colors.gofor all UI theming in Go code
Code must be compatible with Linux, macOS, and Windows - use SDKs over binaries, usefilepath.Join()instead of h...
Files:
tools/director/cmd/export_manifest.go
**/{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, usingnilif no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Files:
tools/director/cmd/export_manifest.go
🧠 Learnings (5)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
demos/scenes/toolchain.tape
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
demos/scenes/toolchain.tape
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
tools/director/cmd/export_manifest.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
tools/director/cmd/export_manifest.go
🧬 Code graph analysis (1)
tools/director/cmd/export_manifest.go (2)
pkg/list/sort/sort.go (1)
Order(13-13)tools/director/internal/scene/scene.go (1)
LoadScenes(88-100)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (10)
demos/scenes/toolchain.tape (6)
1-4: Output formats look good.The three output formats (gif, mp4, svg) are properly declared with consistent naming.
8-9: Dimensions are appropriate.The 1400x800 dimensions provide good visibility for the demo content.
11-12: TrueColor setup is correct.Setting
COLORTERMensures proper color rendering in the VHS output.
14-19: Prompt setup follows VHS best practices.Using Hide/Show to conceal the PS1 setup keeps the recording clean.
21-22: Dependency declaration is appropriate.Requiring
atmosis correct for this toolchain demo.
24-83: Scene flow demonstrates toolchain features well.The demo logically progresses through listing, installing, executing tools, and showing
.tool-versionscompatibility. Pacing looks reasonable.The
.tool-versionsfile referenced at line 73 is not a pre-existing fixture but will be created by theatmos toolchain installcommand on line 45, which is the expected behavior for a toolchain management system.tools/director/cmd/export_manifest.go (4)
3-13: Imports look good.Import grouping follows the convention. The
vhsCachealias works, thoughvhsby itself would be clearer since the package handles more than just cache operations.
15-46: Types are well-defined.Clean struct definitions with appropriate JSON tags and documentation.
218-238: Output handling looks solid.Pretty-printing by default for files is user-friendly. Appending the newline is good practice.
243-305: Command definition is solid.Flags are well-defined with sensible defaults and the examples cover the main use cases.
findDemosDir()is properly defined in the package.
| if !isPublishedInCache { | ||
| if hasExistingFormats { | ||
| // Preserve existing manifest entry (don't lose published data). | ||
| manifest.Scenes = append(manifest.Scenes, existingScene) | ||
| } else { | ||
| // Add as placeholder. | ||
| manifest.Scenes = append(manifest.Scenes, manifestScene) | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
Preserved scenes lose updated metadata.
When a scene exists in the manifest but not in cache, the code preserves existingScene directly. This loses any metadata updates from scenes.yaml (Description, Tags, Category, Title, Order, FeaturedOrder). If someone updates scene metadata without re-rendering, the manifest won't reflect those changes.
Suggested fix to merge metadata
if !isPublishedInCache {
if hasExistingFormats {
// Preserve existing manifest entry (don't lose published data).
- manifest.Scenes = append(manifest.Scenes, existingScene)
+ // Merge current metadata with existing formats.
+ mergedScene := manifestScene
+ mergedScene.Formats = existingScene.Formats
+ manifest.Scenes = append(manifest.Scenes, mergedScene)
} else {
// Add as placeholder.
manifest.Scenes = append(manifest.Scenes, manifestScene)
}
continue
}📝 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.
| if !isPublishedInCache { | |
| if hasExistingFormats { | |
| // Preserve existing manifest entry (don't lose published data). | |
| manifest.Scenes = append(manifest.Scenes, existingScene) | |
| } else { | |
| // Add as placeholder. | |
| manifest.Scenes = append(manifest.Scenes, manifestScene) | |
| } | |
| continue | |
| } | |
| if !isPublishedInCache { | |
| if hasExistingFormats { | |
| // Preserve existing manifest entry (don't lose published data). | |
| // Merge current metadata with existing formats. | |
| mergedScene := manifestScene | |
| mergedScene.Formats = existingScene.Formats | |
| manifest.Scenes = append(manifest.Scenes, mergedScene) | |
| } else { | |
| // Add as placeholder. | |
| manifest.Scenes = append(manifest.Scenes, manifestScene) | |
| } | |
| continue | |
| } |
🤖 Prompt for AI Agents
In @tools/director/cmd/export_manifest.go around lines 166 - 175, The code
currently appends existingScene unchanged when isPublishedInCache is false,
losing updates from scenes.yaml; modify the block handling !isPublishedInCache
so that instead of appending existingScene verbatim you merge selective metadata
fields from manifestScene into existingScene (at least Description, Tags,
Category, Title, Order, FeaturedOrder), preserving published format data from
existingScene, then append the merged scene to manifest.Scenes; update the logic
around hasExistingFormats/isPublishedInCache to use the merged object in place
of existingScene so metadata edits are reflected without discarding published
format info.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @demos/scenes/toolchain.tape:
- Around line 73-76: The demo fails because the "atmos toolchain install"
invocation uses tools without pinned versions (opentofu helm k9s tflint) and the
fixtures directory lacks a .tool-versions file, so create a .tool-versions file
in the demos/fixtures/acme fixture containing the required tool=version lines
(or alternatively edit the "atmos toolchain install" command to specify explicit
versions), and update the tape so the "cat .tool-versions" step at line 73 will
display that file; ensure the file lives in demos/fixtures/acme and matches the
tool names used by the install command.
In @tools/director/cmd/export_manifest.go:
- Around line 76-241: Add simple performance tracking to
runExportManifestWithOptions by recording its execution time: at the start of
runExportManifestWithOptions capture start := time.Now() and defer a call that
records or logs the elapsed time (e.g.,
metrics.RecordDuration("runExportManifestWithOptions", time.Since(start)) or a
similar telemetry API used in the repo); ensure you add the necessary import(s)
(time and the project's metrics/telemetry package) and use a descriptive metric
name so callers can monitor runtime for this public function.
🧹 Nitpick comments (1)
tools/director/cmd/export_manifest.go (1)
218-225: Update comment to match conditional logic.The comment states "always pretty-print for readability" but the code conditionally pretty-prints based on
pretty || output != "-".📝 Clarify comment
- // Marshal to JSON (always pretty-print for readability). + // Marshal to JSON (pretty-print for files by default or when --pretty is set). var jsonData []byte if pretty || output != "-" { - // Pretty-print when writing to file (default) or when --pretty is set. jsonData, err = json.MarshalIndent(manifest, "", " ")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
demos/scenes/config/describe-affected.tapedemos/scenes/toolchain.tapetools/director/cmd/export_manifest.gowebsite/src/data/manifest.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter) in Go code
Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (cfg,log,u,errUtils)
All errors MUST be wrapped using static errors defined inerrors/errors.go- useerrors.Joinfor combining errors,fmt.Errorfwith%wfor context, anderrors.Is()for error checking
Never manually create mocks - usego.uber.org/mock/mockgenwith//go:generatedirectives in Go code
Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use//revive:disable:file-length-limit
Use colors frompkg/ui/theme/colors.gofor all UI theming in Go code
Code must be compatible with Linux, macOS, and Windows - use SDKs over binaries, usefilepath.Join()instead of h...
Files:
tools/director/cmd/export_manifest.go
**/{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, usingnilif no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Files:
tools/director/cmd/export_manifest.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
📚 Learning: 2025-10-14T01:54:48.410Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-completion--help.html:2-7
Timestamp: 2025-10-14T01:54:48.410Z
Learning: Screengrab HTML files in website/src/components/Screengrabs/ are generated from actual Atmos CLI output converted to HTML. The ANSI-art headers and formatting in these files are intentional and reflect the real CLI user experience, so they should not be suggested for removal or modification.
Applied to files:
demos/scenes/config/describe-affected.tape
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
tools/director/cmd/export_manifest.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
tools/director/cmd/export_manifest.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
demos/scenes/toolchain.tape
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
demos/scenes/toolchain.tape
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (12)
tools/director/cmd/export_manifest.go (5)
1-13: LGTM!Import organization follows the guidelines correctly: stdlib, third-party, then Atmos packages with proper aliasing.
15-46: LGTM!Type definitions are clean, well-documented, and properly structured for JSON marshaling.
48-68: LGTM!The helper function correctly handles file loading with appropriate nil returns for stdout and missing files.
243-305: LGTM!Command constructor is well-structured with clear documentation, appropriate flags, and correct flag handling (no Viper binding per learnings).
70-74: Add performance tracking to this exported function.Per coding guidelines, all public functions require performance tracking unless they're trivial getters/setters, command constructors, or simple factories.
⚡ Add performance tracking
+import ( + "github.com/cloudposse/atmos/pkg/utils/perf" +) + func runExportManifest(demosDir string) error { + defer perf.Track(nil, "cmd.runExportManifest")() + return runExportManifestWithOptions(demosDir, "", false, false, false) }Based on coding guidelines for public functions.
⛔ Skipped due to learnings
Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-04T00:55:21.720Z Learning: Applies to **/{pkg,internal,cmd}/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` plus blank line to all public functions, using `nil` if no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functionsLearnt from: aknysh Repo: cloudposse/atmos PR: 1821 File: pkg/merge/deferred.go:34-48 Timestamp: 2025-11-30T04:16:24.155Z Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.Learnt from: aknysh Repo: cloudposse/atmos PR: 1821 File: pkg/merge/deferred.go:50-59 Timestamp: 2025-11-30T04:16:01.899Z Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` should NOT be added to trivial O(1) getter methods that only return field references or check map lengths (e.g., `GetDeferredValues()`, `HasDeferredValues()`). The guideline to add perf tracking to "all public functions" applies to functions that do meaningful work (I/O, computation, external calls), not to trivial accessors where the tracking overhead would exceed the operation time and pollute performance reports.Learnt from: osterman Repo: cloudposse/atmos PR: 1752 File: pkg/profile/list/formatter_table.go:27-29 Timestamp: 2025-11-09T19:06:58.470Z Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.Learnt from: osterman Repo: cloudposse/atmos PR: 740 File: cmd/cmd_utils.go:340-359 Timestamp: 2024-10-23T21:36:40.262Z Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.Learnt from: aknysh Repo: cloudposse/atmos PR: 1622 File: pkg/perf/perf.go:140-184 Timestamp: 2025-10-13T18:13:54.020Z Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.Learnt from: osterman Repo: cloudposse/atmos PR: 808 File: pkg/config/config.go:478-483 Timestamp: 2024-12-02T21:26:32.337Z Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.demos/scenes/config/describe-affected.tape (4)
6-20: Scene flow reads clean and is paced well.Theme reuse, prompt normalization, and the “comment → command → hold” rhythm make the recording easy to follow.
Also applies to: 24-55
1-5: Declare the screenshot as an output (or drop it).You create
config-describe-affected.png(Line 39) but don’t declare it inOutput ...(Lines 2-4), so it’s easy to lose in render/publish steps.Proposed diff
# describe-affected scene - Find affected components from changes Output config-describe-affected.gif Output config-describe-affected.mp4 Output config-describe-affected.svg +Output config-describe-affected.pngAlso applies to: 39-40
⛔ Skipped due to learnings
Learnt from: osterman Repo: cloudposse/atmos PR: 1466 File: website/src/components/Screengrabs/demo-stacks/deploy-dev.html:28-37 Timestamp: 2025-09-07T17:38:17.619Z Learning: Screengrab files in the CloudPosse/atmos repository are programmatically generated and should be ignored during code reviews. Do not provide suggestions or comments on files in screengrab directories or screengrab-related HTML files.
25-37: Using--ref mainin demo recordings is standard practice.Demo tape files illustrate command usage; the source files are static and committed to the repo. The maintainer has already approved using
mainreferences in example configurations. For instructional purposes, this is acceptable.Likely an incorrect or invalid review comment.
11-13: Harden runtime dependencies + avoid pager hangs during recording.This scene will be more reliable if it explicitly requires
git(used for worktree operations) and forces non-paged output for VHS runs. Note:--queryexpressions are evaluated internally via the yq library, so an externalyqrequirement is not needed.Proposed diff
# Enable TrueColor for proper color rendering in VHS Env COLORTERM "truecolor" +# Avoid interactive pagers during recording +Env PAGER "cat" +Env NO_PAGER "1" # Fix prompt (VHS runs with --no-rcs) Hide Type "PS1='\033[38;2;90;86;224m> \033[0m'" Enter Ctrl+L Show # Dependencies Require atmos +Require gitAlso applies to: 21-23, 33-49
Likely an incorrect or invalid review comment.
demos/scenes/toolchain.tape (3)
1-22: VHS configuration looks solid.The output formats, theming, dimensions, and environment setup are all appropriate for a toolchain demo. The prompt fix pattern is a good touch for VHS's
--no-rcsbehavior.
24-83: Demo flow effectively showcases toolchain features.The progression from listing tools through installation and execution to showing the version file creates a clear narrative. The timing and commands demonstrate the key value proposition nicely.
45-48: Removek9sfrom the toolchain install command.The demo installs
opentofu helm k9s tflint, but k9s is not defined in the atmos toolchain configuration (demos/fixtures/acme/atmos.yaml). The other three tools are configured correctly. Either add k9s to the toolchain config or remove it from the install command.⛔ Skipped due to learnings
Learnt from: osterman Repo: cloudposse/atmos PR: 808 File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10 Timestamp: 2024-12-12T15:17:45.245Z Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.Learnt from: aknysh Repo: cloudposse/atmos PR: 810 File: internal/exec/terraform_utils.go:145-146 Timestamp: 2024-12-03T05:29:07.718Z Learning: In the Atmos project, a 5-minute timeout in the `execTerraformOutput` function is acceptable for retrieving `terraform output` for a component in a stack.Learnt from: RoseSecurity Repo: cloudposse/atmos PR: 1448 File: cmd/ansible.go:26-28 Timestamp: 2025-09-05T14:57:37.360Z Learning: The Atmos codebase uses a consistent pattern for commands that delegate to external tools: `PersistentFlags().Bool("", false, doubleDashHint)` where doubleDashHint provides help text about using double dashes to separate Atmos options from native command arguments. This pattern is used across terraform, packer, helmfile, atlantis, aws, and ansible commands.Learnt from: osterman Repo: cloudposse/atmos PR: 727 File: internal/exec/terraform.go:114-118 Timestamp: 2024-10-21T17:51:07.087Z Learning: Use `bubbletea` for confirmation prompts instead of `fmt.Scanln` in the `atmos terraform clean` command.Learnt from: osterman Repo: cloudposse/atmos PR: 1466 File: website/docs/cli/commands/toolchain/usage.mdx:117-121 Timestamp: 2025-09-08T01:25:44.958Z Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
| Type "cat .tool-versions" | ||
| Sleep 1s | ||
| Enter | ||
| Sleep 2s |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for .tool-versions in demo fixtures
echo "Looking for .tool-versions in demo directories..."
fd -H -t f '\.tool-versions' demos/ 2>/dev/null || echo "No .tool-versions found"
# Check the tape file and surrounding context
echo -e "\n=== Tape file content (lines 65-80) ==="
cat -n demos/scenes/toolchain.tape | sed -n '65,80p'
# Check for working directory hints in related files
echo -e "\n=== Checking tape/asdf related context ==="
rg -n 'WorkingDir|workdir|working.?dir|cwd' demos/ --type yaml -i -A 1
# Check if there are any fixtures or setup files
echo -e "\n=== Looking for fixture/setup files in demos ==="
find demos -type f -name "*.tape" -o -name "setup*" -o -name "fixture*" -o -name ".tool-versions" 2>/dev/null | head -20Repository: cloudposse/atmos
Length of output: 6105
🏁 Script executed:
#!/bin/bash
# Find the toolchain scene definition in demos/scenes.yaml
echo "=== Finding toolchain scene definition ==="
rg -n -A 5 'toolchain:' demos/scenes.yaml
# Check if .tool-versions exists in fixtures/acme
echo -e "\n=== Checking for .tool-versions in fixtures ==="
ls -la demos/fixtures/acme/.tool-versions 2>/dev/null || echo "File not found in fixtures/acme"
ls -la demos/fixtures/live/.tool-versions 2>/dev/null || echo "File not found in fixtures/live"
# List all files in fixtures/acme
echo -e "\n=== Files in demos/fixtures/acme (first 30) ==="
find demos/fixtures/acme -type f | head -30Repository: cloudposse/atmos
Length of output: 1724
🏁 Script executed:
#!/bin/bash
# Search for toolchain references more broadly
echo "=== Searching for 'toolchain' in demos/scenes.yaml ==="
rg -n 'toolchain' demos/scenes.yaml -i
# Check if toolchain.tape is referenced anywhere
echo -e "\n=== Searching for toolchain.tape references ==="
rg -r 'toolchain.tape' demos/
# See what the toolchain.tape file looks like (first 30 lines)
echo -e "\n=== First 30 lines of toolchain.tape ==="
head -30 demos/scenes/toolchain.tape
# Check if there's any reference to tool-versions in the codebase
echo -e "\n=== Searching for tool-versions references ==="
rg 'tool-versions' . --type yaml --type txt --type md -iRepository: cloudposse/atmos
Length of output: 20786
🏁 Script executed:
#!/bin/bash
# Get the toolchain scene definition from scenes.yaml
echo "=== Toolchain scene definition ==="
sed -n '839,870p' demos/scenes.yaml
# Check full toolchain.tape to see if .tool-versions is created earlier
echo -e "\n=== Full toolchain.tape ==="
cat demos/scenes/toolchain.tape | head -100Repository: cloudposse/atmos
Length of output: 2090
🏁 Script executed:
#!/bin/bash
# Check if atmos toolchain install creates .tool-versions
echo "=== Searching for toolchain install command behavior ==="
rg -n 'toolchain install' demos/scenes/ -A 2 -B 2
# Search toolchain implementation for .tool-versions creation
echo -e "\n=== Checking if toolchain commands create .tool-versions ==="
rg 'WriteFile|WriteToolVersions|tool.versions|tool-versions' --type go -l | head -5
# Look for any initialization or setup in the tape files
echo -e "\n=== Checking other toolchain tapes for .tool-versions setup ==="
grep -n "tool-versions\|atmos toolchain" demos/scenes/toolchain*.tapeRepository: cloudposse/atmos
Length of output: 2245
🏁 Script executed:
#!/bin/bash
# Check the toolchain install command documentation/implementation
echo "=== Looking for toolchain install command implementation ==="
fd -t f 'toolchain' cmd/atmos/ | head -10
# Search for the install subcommand
echo -e "\n=== Searching for toolchain install implementation ==="
rg -n 'func.*install|toolchain.*install' --type go -A 3 | head -40
# Check what atmos.yaml says about tool-versions
echo -e "\n=== Checking atmos.yaml configuration ==="
cat -n demos/fixtures/acme/atmos.yaml | grep -A 5 -B 5 'tool'
# Look at examples/toolchain to understand the expected behavior
echo -e "\n=== Checking examples/toolchain ==="
ls -la examples/toolchain/ 2>/dev/null || echo "Not found"
cat examples/toolchain/atmos.yaml 2>/dev/null | grep -A 10 'tool' | head -20Repository: cloudposse/atmos
Length of output: 4715
🏁 Script executed:
#!/bin/bash
# Check if toolchain install command creates .tool-versions or assumes it exists
echo "=== Searching for tool-versions file creation in toolchain code ==="
rg 'tool.versions|tool-versions' . --type go -B 3 -A 3 | grep -A 5 -B 5 'Write\|Create\|Install' | head -50
# Check the toolchain package for install behavior
echo -e "\n=== Looking for install function in toolchain ==="
fd -t f 'install.go' --path '*/toolchain/*'
# Check if examples/toolchain has a .tool-versions file
echo -e "\n=== Checking examples/toolchain for .tool-versions ==="
find examples/toolchain -name '.tool-versions' -o -name '.tool-*'
# See if there are any pre-existing .tool-versions files anywhere
echo -e "\n=== Searching all .tool-versions files in repo ==="
find . -name '.tool-versions' -type f 2>/dev/null | head -20Repository: cloudposse/atmos
Length of output: 3222
🏁 Script executed:
#!/bin/bash
# Search for the install subcommand implementation in toolchain
echo "=== Looking for toolchain install command ==="
rg -n 'func.*Install|install.*Command' toolchain/ --type go -A 5 | head -60
# Check if install creates/writes to .tool-versions
echo -e "\n=== Checking if install writes to .tool-versions ==="
rg -n 'WriteFile.*tool-versions|os.WriteFile.*tool|versions_file.*Write' . --type go -B 2 -A 2
# Look at the install command documentation
echo -e "\n=== Searching for install documentation ==="
grep -r "install" cmd/markdown/atmos_toolchain_install.md 2>/dev/null | head -20
cat cmd/markdown/atmos_toolchain_install.md 2>/dev/null | head -50Repository: cloudposse/atmos
Length of output: 6891
🏁 Script executed:
#!/bin/bash
# Final verification: check tape execution order and syntax
echo "=== Lines 40-80 of toolchain.tape to see full context ==="
sed -n '40,80p' demos/scenes/toolchain.tape
# Check if atmos add command creates .tool-versions
echo -e "\n=== Checking atmos toolchain add command ==="
grep -A 10 'atmos_toolchain_add.md' cmd/markdown/atmos_toolchain_add.md 2>/dev/null | head -30Repository: cloudposse/atmos
Length of output: 769
🏁 Script executed:
#!/bin/bash
# Check the actual install.go implementation
echo "=== Looking for install.go in toolchain package ==="
find . -name 'install.go' -type f | grep toolchain
# Check what happens when install is called without versions
echo -e "\n=== Looking at install.go implementation ==="
cat toolchain/install.go | head -100Repository: cloudposse/atmos
Length of output: 2945
🏁 Script executed:
#!/bin/bash
# Continue reading install.go to see what happens with tools without version specs
echo "=== Continuing install.go implementation (lines 100-200) ==="
sed -n '100,200p' toolchain/install.go
# Check installSingleToolSpec
echo -e "\n=== Looking for installSingleToolSpec function ==="
rg -n 'func installSingleToolSpec|func installFromToolVersions' toolchain/install.go -A 20 | head -60Repository: cloudposse/atmos
Length of output: 4546
The demo requires .tool-versions file to exist in demos/fixtures/acme.
The atmos toolchain install command at line 45 specifies tools without versions (opentofu helm k9s tflint), which means it needs to load versions from a .tool-versions file. Since this file doesn't exist in the fixture directory, the demo will fail at that step, and again at line 73 when trying to cat .tool-versions.
🤖 Prompt for AI Agents
In @demos/scenes/toolchain.tape around lines 73 - 76, The demo fails because the
"atmos toolchain install" invocation uses tools without pinned versions
(opentofu helm k9s tflint) and the fixtures directory lacks a .tool-versions
file, so create a .tool-versions file in the demos/fixtures/acme fixture
containing the required tool=version lines (or alternatively edit the "atmos
toolchain install" command to specify explicit versions), and update the tape so
the "cat .tool-versions" step at line 73 will display that file; ensure the file
lives in demos/fixtures/acme and matches the tool names used by the install
command.
| // runExportManifestWithOptions exports the manifest with configurable options. | ||
| // When replace is false (default), existing manifest entries are preserved if they | ||
| // have formats and the cache doesn't have updated PublicURLs for that scene. | ||
| func runExportManifestWithOptions(demosDir string, output string, pretty bool, includeDrafts bool, replace bool) error { | ||
| // Load cache metadata. | ||
| cacheDir := filepath.Join(demosDir, ".cache") | ||
| cache, err := vhsCache.LoadCache(cacheDir) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load cache: %w", err) | ||
| } | ||
|
|
||
| // Load scenes. | ||
| scenesFile := filepath.Join(demosDir, "scenes.yaml") | ||
| scenesList, err := scene.LoadScenes(scenesFile) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load scenes: %w", err) | ||
| } | ||
|
|
||
| // Determine output path early (needed for loading existing manifest). | ||
| outputPath := output | ||
| if outputPath == "" { | ||
| repoRoot := filepath.Dir(demosDir) | ||
| outputPath = filepath.Join(repoRoot, "website", "src", "data", "manifest.json") | ||
| } | ||
|
|
||
| // Load existing manifest for in-place updates (unless --replace is set). | ||
| var existingScenes map[string]ManifestScene | ||
| if !replace && outputPath != "-" { | ||
| existing, err := loadExistingManifest(outputPath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load existing manifest: %w", err) | ||
| } | ||
| if existing != nil { | ||
| existingScenes = make(map[string]ManifestScene) | ||
| for _, s := range existing.Scenes { | ||
| existingScenes[s.Name] = s | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Build manifest. | ||
| manifest := Manifest{ | ||
| Scenes: make([]ManifestScene, 0), | ||
| } | ||
|
|
||
| // Determine common subdomain from any scene. | ||
| for _, sceneHash := range cache.Scenes { | ||
| if sceneHash.StreamSubdomain != "" { | ||
| manifest.Subdomain = sceneHash.StreamSubdomain | ||
| break | ||
| } | ||
| } | ||
|
|
||
| for _, sc := range scenesList.Scenes { | ||
| // Skip draft scenes unless --include-drafts is set. | ||
| if sc.IsDraft() && !includeDrafts { | ||
| continue | ||
| } | ||
|
|
||
| sceneHash, exists := cache.Scenes[sc.Name] | ||
| isPublishedInCache := exists && sceneHash.PublicURLs != nil | ||
|
|
||
| // Check existing manifest for this scene. | ||
| existingScene, existsInManifest := existingScenes[sc.Name] | ||
| hasExistingFormats := existsInManifest && len(existingScene.Formats) > 0 | ||
|
|
||
| // Skip scenes without gallery config that aren't published (in cache or existing manifest). | ||
| if !isPublishedInCache && !hasExistingFormats && sc.Gallery == nil { | ||
| continue | ||
| } | ||
|
|
||
| manifestScene := ManifestScene{ | ||
| Name: sc.Name, | ||
| Description: sc.Description, | ||
| Tags: sc.Tags, | ||
| FeaturedOrder: sc.FeaturedOrder, | ||
| Formats: make(map[string]ManifestFormat), | ||
| } | ||
|
|
||
| // Add gallery metadata if present. | ||
| if sc.Gallery != nil { | ||
| manifestScene.Category = sc.Gallery.Category | ||
| manifestScene.Title = sc.Gallery.Title | ||
| manifestScene.Order = sc.Gallery.Order | ||
| } | ||
|
|
||
| // Priority: cache > existing manifest > placeholder | ||
| // 1. If scene is published in cache, use cache data (latest). | ||
| // 2. Else if scene has formats in existing manifest, preserve them. | ||
| // 3. Else add as placeholder. | ||
| if !isPublishedInCache { | ||
| if hasExistingFormats { | ||
| // Preserve existing manifest entry (don't lose published data). | ||
| manifest.Scenes = append(manifest.Scenes, existingScene) | ||
| } else { | ||
| // Add as placeholder. | ||
| manifest.Scenes = append(manifest.Scenes, manifestScene) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // Add each published format from cache. | ||
| for format, url := range sceneHash.PublicURLs { | ||
| manifestFormat := ManifestFormat{ | ||
| URL: url, | ||
| } | ||
|
|
||
| // Determine backend type based on Stream metadata presence. | ||
| if sceneHash.StreamUID != "" && format == "mp4" { | ||
| // This is a Stream video. | ||
| manifestFormat.Type = "stream" | ||
| manifestFormat.UID = sceneHash.GetLatestStreamUID() | ||
| manifestFormat.Subdomain = sceneHash.StreamSubdomain | ||
| manifestFormat.AllUIDs = sceneHash.GetAllStreamUIDs() | ||
|
|
||
| // Include video metadata. | ||
| if sceneHash.Duration > 0 { | ||
| manifestFormat.Duration = sceneHash.Duration | ||
| } | ||
|
|
||
| // Build thumbnail URL from Stream subdomain and UID. | ||
| // Include time parameter if thumbnail time is set. | ||
| if sceneHash.StreamSubdomain != "" && manifestFormat.UID != "" { | ||
| if sceneHash.ThumbnailTime > 0 { | ||
| manifestFormat.Thumbnail = fmt.Sprintf("https://%s/%s/thumbnails/thumbnail.jpg?time=%.1fs", | ||
| sceneHash.StreamSubdomain, manifestFormat.UID, sceneHash.ThumbnailTime) | ||
| } else { | ||
| manifestFormat.Thumbnail = fmt.Sprintf("https://%s/%s/thumbnails/thumbnail.jpg", | ||
| sceneHash.StreamSubdomain, manifestFormat.UID) | ||
| } | ||
| } | ||
| } else { | ||
| // This is an R2 file. | ||
| manifestFormat.Type = "r2" | ||
| } | ||
|
|
||
| manifestScene.Formats[format] = manifestFormat | ||
| } | ||
|
|
||
| manifest.Scenes = append(manifest.Scenes, manifestScene) | ||
| } | ||
|
|
||
| // Marshal to JSON (always pretty-print for readability). | ||
| var jsonData []byte | ||
| if pretty || output != "-" { | ||
| // Pretty-print when writing to file (default) or when --pretty is set. | ||
| jsonData, err = json.MarshalIndent(manifest, "", " ") | ||
| } else { | ||
| jsonData, err = json.Marshal(manifest) | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal manifest: %w", err) | ||
| } | ||
|
|
||
| // Write to stdout or file. | ||
| if outputPath == "-" { | ||
| fmt.Println(string(jsonData)) | ||
| } else { | ||
| if err := os.WriteFile(outputPath, append(jsonData, '\n'), 0o644); err != nil { | ||
| return fmt.Errorf("failed to write manifest to %s: %w", outputPath, err) | ||
| } | ||
| fmt.Printf("Manifest written to %s\n", outputPath) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add performance tracking to this exported function.
This public function needs performance tracking per coding guidelines.
⚡ Add performance tracking
+import (
+ "github.com/cloudposse/atmos/pkg/utils/perf"
+)
+
func runExportManifestWithOptions(demosDir string, output string, pretty bool, includeDrafts bool, replace bool) error {
+ defer perf.Track(nil, "cmd.runExportManifestWithOptions")()
+
// Load cache metadata.
cacheDir := filepath.Join(demosDir, ".cache")Based on coding guidelines for public functions.
🤖 Prompt for AI Agents
In @tools/director/cmd/export_manifest.go around lines 76 - 241, Add simple
performance tracking to runExportManifestWithOptions by recording its execution
time: at the start of runExportManifestWithOptions capture start := time.Now()
and defer a call that records or logs the elapsed time (e.g.,
metrics.RecordDuration("runExportManifestWithOptions", time.Since(start)) or a
similar telemetry API used in the repo); ensure you add the necessary import(s)
(time and the project's metrics/telemetry package) and use a descriptive metric
name so callers can monitor runtime for this public function.
Simplify VHS tape source resolution by always preprocessing tapes to inline Source directives. This eliminates the need for every tape file to have a `cd $VHS_WORKDIR` command in its Hide section. Key changes: - Add preprocess.go with PreprocessTape function for source inlining - Modify vhs.go to run VHS from workdir with absolute tape paths - Update renderer.go to always preprocess tapes (not just with format filter) - Update validate.go and render.go to preprocess before validation - Fix Source directive regex to handle quoted paths - Remove cd commands from all 67 tape files Before: Each tape needed `Type "cd $VHS_WORKDIR"` in Hide section (fragile) After: Director handles path resolution centrally (maintainable) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @demos/scenes/auth/whoami.tape:
- Around line 38-44: The scene contains a dangling comment "Type \"# Quick
identity check across all providers\"" with no following command and an extra
blank line; either remove that comment and the trailing blank line (to avoid
misleading viewers) or replace it by adding the intended command(s) (e.g., the
identity check or provider invocation) immediately after the comment so the
sequence after Sleep 1s performs the described action; update the block
containing the lines that type the comment and the Sleep 1s / Sleep 10s flow
accordingly (the area mentioning the comment and the Sleep commands).
🧹 Nitpick comments (19)
demos/scenes/auth/list.tape (1)
39-49: Consider capturing a screenshot after the table format command.You capture
auth-list.pngafter the default tree view, but not afteratmos auth list --format table. If the table format output is worth showcasing separately, you might want a second screenshot here.📸 Optional: Add screenshot after table format
Type "atmos auth list --format table" Sleep 2s Enter Sleep 3s + +# Capture table format screenshot +Screenshot auth-list-table.pngdemos/scenes/commands.tape (2)
25-53: Scene flow is clear and well-paced.The progression from showing the config, capturing a screenshot, running the custom command, and teasing workflows makes for a logical demo. Sleep durations give viewers time to follow along.
One thing: line 51 mentions "Compose atmos commands into workflows" but the scene ends without demonstrating that. Consider either removing that teaser or adding a brief follow-up command to deliver on the promise.
55-57: Minor: extra blank line.Line 55 has an extra blank line before the final comment. Harmless, but trimming it keeps the file tidy.
🧹 Optional cleanup
- - # Hold final frame Sleep 10sdemos/scenes/list/vars.tape (1)
22-52: Scene sequence demonstrates the feature well.The command flow showcases both basic usage and the
--queryfiltering capability. Sleep timings provide good pacing for viewers. One minor note: the 10-second hold at the end is generous—consider if 5-6 seconds might suffice for the final frame, though this is purely a preference call.Optional: Shorter final hold
# Hold final frame -Sleep 10s +Sleep 5sdemos/scenes/terraform/init.tape (1)
1-45: Solid demo tape setup.Structure is clean: outputs defined, theme sourced, dependencies declared, pacing is reasonable for demo visibility. The
Hide/Showpattern ensures a clean recording start.Two minor style notes:
- Line 43 has an extra blank line
- File should end with a trailing newline
Neither impacts functionality.
🔧 Optional: clean up trailing whitespace
Type "# Downloads providers and modules for the component" Enter Sleep 1s - # Hold final frame Sleep 10s +pkg/utils/highlight_utils.go (1)
86-108: Logic changes look solid for the VHS use case.The dynamic TTY detection and precedence order (NoColor → highlighting disabled → terminal check with ForceColor override) is well-structured. The comment on lines 87-88 explains why the previous package-init caching was problematic for VHS environments.
One small nit: line 91 comment is missing a trailing period.
🔧 Suggested fix
- // Check if color is forced via ForceColor (ATMOS_FORCE_COLOR) + // Check if color is forced via ForceColor (ATMOS_FORCE_COLOR).Based on learnings, the previous
isTermPresentcaching at init time was intentional, but your justification for dynamic detection (VHS sets up TTY after binary loads) is valid and well-documented.demos/scenes/auth/shell.tape (1)
39-50: Consider moving screenshot after the identity verification.Screenshot is captured at line 40, before
aws sts get-caller-identityruns. The caller identity output (lines 46-49) is arguably the more interesting frame to capture for documentation purposes.Suggested reordering
Type "# Credentials are automatically available" Enter Sleep 2s Type "aws sts get-caller-identity" Sleep 2s Enter Sleep 3s +# Capture screenshot +Screenshot auth-shell.png + Type "exit" Enter Sleep 1sAnd remove lines 39-40 from their current position.
demos/scenes/completion.tape (1)
30-31: Consider movingRequireearlier.Placing
Require atmosbefore the hidden setup block (after the VHS config) would be more conventional. Not a blocker, just a readability preference.demos/scenes/flagship/tui.tape (1)
79-88: Consider the 10-second hold duration.The final 10-second hold is quite long. Depending on where this video is displayed (website gallery, social media), you may want to adjust this. If intentional for looping purposes or to let the closing message sink in, it's fine.
demos/scenes/terraform/destroy.tape (1)
43-45: Trailing blank line before final hold.Minor nit — extra empty line at 43. Not a blocker.
Optional cleanup
- - # Hold final frame Sleep 10stools/director/cmd/validate.go (2)
118-167:filterScenesduplicates logic from render.go.This function mirrors the filtering logic in the render command. Worth extracting to a shared utility to keep things DRY.
Consider extracting to a shared helper
Create a shared function in a common location:
// In a shared file like tools/director/internal/scene/filter.go func FilterScenes(scenesList *ScenesList, args []string, tag, category string, all, includeDrafts bool) []*Scene { // ... implementation }Then both
validate.goandrender.gocan import and use it.
169-233:validateTapeSyntaxhandles temp file cleanup correctly, but considerdefer.The current approach removes temp files after validation. A
deferpattern could simplify and ensure cleanup on all exit paths:tempTape, err := vhs.PreprocessTape(tapeFile) if err != nil { cmd.Printf("✗ %s: failed to preprocess tape: %v\n", sc.Name, err) hasErrors = true continue } +defer os.Remove(tempTape) if err := vhs.ValidateTape(ctx, tempTape, workdir); err != nil { - os.Remove(tempTape) cmd.Printf("✗ %s\n", sc.Name) // ... hasErrors = true -} else { - os.Remove(tempTape) +}That said, current implementation is functionally correct.
.claude/agents/director.md (3)
171-195: Directory structure block missing language specifier.Per markdownlint MD040, fenced code blocks should have a language. Add
textorplaintext:-``` +```text demos/ ├── scenes.yaml # Scene definitions
218-247: VHS tape format block missing language specifier.Same fix — add a language identifier:
-``` +```text # Output directives Output scene-name.gif
311-319: Task tool example block missing language specifier.-``` +```text Use the Task tool with subagent_type='Explore' to search the atmos codebasetools/director/cmd/render.go (2)
143-202: Scene filtering logic duplicated from validate.go.This is the same filtering pattern seen in
filterScenesin validate.go. Extracting this to a shared helper would reduce maintenance burden.Extract to shared filter function
// tools/director/internal/scene/filter.go func Filter(scenes []*Scene, args []string, tag, category string, all, includeDrafts bool) []*Scene { // shared implementation }Then both commands can use
scene.Filter(...).
215-239: Tape pre-validation is a good safeguard.Validating all tapes upfront before starting the render loop prevents wasted time on long renders that would fail mid-way. The early-exit on first failure is appropriate here.
Consider using
defer os.Remove(tempTape)right after creation for cleaner cleanup:tempTape, err := vhs.PreprocessTape(tapeFile) if err != nil { c.Printf("✗ %s: failed to preprocess: %v\n", sc.Name, err) return fmt.Errorf("tape preprocessing failed for %s: %w", sc.Name, err) } +defer os.Remove(tempTape) if err := vhs.ValidateTape(ctx, tempTape, workdir); err != nil { - os.Remove(tempTape) c.Printf("✗ %s: %s\n", sc.Name, sc.Tape) return fmt.Errorf("tape validation failed for %s: %w", sc.Name, err) } -os.Remove(tempTape)tools/director/cmd/export_manifest.go (1)
56-68: Wrap errors with context.Per guidelines, errors should be wrapped with meaningful context. The bare returns here lose information about what operation failed.
Proposed fix
data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { return nil, nil // No existing manifest. } - return nil, err + return nil, fmt.Errorf("failed to read manifest file %s: %w", path, err) } var manifest Manifest if err := json.Unmarshal(data, &manifest); err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse manifest file %s: %w", path, err) }demos/scenes/devcontainer.tape (1)
72-75: Minor: extra blank line.Line 73 is an extra blank line before the final hold section. Harmless, but trimming it would keep things tidy.
🧹 Optional cleanup
Type "# Reproducible environments - no more 'works on my machine'" Enter Sleep 1s - # Hold final frame Sleep 10s
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (75)
.claude/agents/director.mddemos/fixtures/acme/components/terraform/api/main.tfdemos/fixtures/acme/components/terraform/cdn/main.tfdemos/fixtures/acme/stacks/catalog/cache.yamldemos/fixtures/acme/stacks/catalog/database.yamldemos/fixtures/acme/stacks/catalog/vpc.yamldemos/fixtures/acme/stacks/mixins/region/eu-west-1.yamldemos/fixtures/acme/stacks/mixins/region/us-east-2.yamldemos/fixtures/acme/stacks/mixins/region/us-west-2.yamldemos/fixtures/live/components/terraform/vpc/.terraform/environmentdemos/scenes/about.tapedemos/scenes/aliases.tapedemos/scenes/auth/console.tapedemos/scenes/auth/env.tapedemos/scenes/auth/exec.tapedemos/scenes/auth/list.tapedemos/scenes/auth/login.tapedemos/scenes/auth/shell.tapedemos/scenes/auth/terraform.tapedemos/scenes/auth/whoami.tapedemos/scenes/commands.tapedemos/scenes/completion.tapedemos/scenes/config/describe-affected.tapedemos/scenes/config/template-functions.tapedemos/scenes/config/validate-stacks.tapedemos/scenes/demo-stacks/component.tapedemos/scenes/demo-stacks/describe.tapedemos/scenes/demo-stacks/list.tapedemos/scenes/demo-stacks/terraform-plan.tapedemos/scenes/demo-stacks/validate.tapedemos/scenes/devcontainer.tapedemos/scenes/docs.tapedemos/scenes/featured/see-everything.tapedemos/scenes/featured/tools-just-work.tapedemos/scenes/featured/unified-config.tapedemos/scenes/flagship/deploy-workflow.tapedemos/scenes/flagship/onboarding.tapedemos/scenes/flagship/tui.tapedemos/scenes/list/components.tapedemos/scenes/list/stacks.tapedemos/scenes/list/vars.tapedemos/scenes/profile.tapedemos/scenes/support.tapedemos/scenes/terraform/affected.tapedemos/scenes/terraform/apply.tapedemos/scenes/terraform/describe.tapedemos/scenes/terraform/destroy.tapedemos/scenes/terraform/init.tapedemos/scenes/terraform/output.tapedemos/scenes/terraform/plan.tapedemos/scenes/theme.tapedemos/scenes/toolchain-deps.tapedemos/scenes/toolchain-env.tapedemos/scenes/toolchain-search.tapedemos/scenes/toolchain.tapedemos/scenes/use-version.tapedemos/scenes/validate.tapedemos/scenes/vendor/pull.tapedemos/scenes/version.tapedemos/scenes/workflow/list.tapedemos/scenes/workflow/run.tapepkg/utils/highlight_utils.gotools/director/cmd/export_manifest.gotools/director/cmd/render.gotools/director/cmd/validate.gotools/director/internal/vhs/cache.gotools/director/internal/vhs/preprocess.gotools/director/internal/vhs/renderer.gotools/director/internal/vhs/vhs.gowebsite/src/components/DemoGallery/index.tsxwebsite/src/components/DemoGallery/styles.module.csswebsite/src/components/SvgThumbnail/index.tsxwebsite/src/components/Video/DemoVideo.tsxwebsite/src/data/demos.jswebsite/src/data/manifest.json
🚧 Files skipped from review as they are similar to previous changes (39)
- demos/fixtures/acme/stacks/catalog/cache.yaml
- demos/scenes/list/components.tape
- demos/scenes/theme.tape
- demos/scenes/workflow/run.tape
- demos/scenes/auth/console.tape
- demos/scenes/support.tape
- demos/scenes/list/stacks.tape
- demos/scenes/terraform/apply.tape
- demos/scenes/auth/env.tape
- demos/scenes/terraform/plan.tape
- demos/fixtures/acme/stacks/catalog/vpc.yaml
- demos/fixtures/live/components/terraform/vpc/.terraform/environment
- demos/scenes/aliases.tape
- demos/scenes/terraform/describe.tape
- demos/scenes/docs.tape
- demos/scenes/toolchain.tape
- demos/scenes/vendor/pull.tape
- demos/scenes/auth/login.tape
- demos/scenes/workflow/list.tape
- demos/scenes/use-version.tape
- demos/scenes/demo-stacks/component.tape
- demos/scenes/config/template-functions.tape
- demos/fixtures/acme/stacks/mixins/region/eu-west-1.yaml
- demos/fixtures/acme/stacks/mixins/region/us-east-2.yaml
- demos/scenes/featured/unified-config.tape
- demos/scenes/terraform/output.tape
- demos/scenes/demo-stacks/terraform-plan.tape
- demos/scenes/validate.tape
- demos/scenes/flagship/onboarding.tape
- demos/scenes/terraform/affected.tape
- demos/scenes/profile.tape
- demos/fixtures/acme/stacks/mixins/region/us-west-2.yaml
- demos/scenes/auth/exec.tape
- demos/scenes/auth/terraform.tape
- demos/scenes/about.tape
- demos/fixtures/acme/components/terraform/api/main.tf
- demos/scenes/toolchain-search.tape
- demos/scenes/featured/see-everything.tape
- demos/scenes/demo-stacks/validate.tape
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter) in Go code
Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (cfg,log,u,errUtils)
All errors MUST be wrapped using static errors defined inerrors/errors.go- useerrors.Joinfor combining errors,fmt.Errorfwith%wfor context, anderrors.Is()for error checking
Never manually create mocks - usego.uber.org/mock/mockgenwith//go:generatedirectives in Go code
Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use//revive:disable:file-length-limit
Use colors frompkg/ui/theme/colors.gofor all UI theming in Go code
Code must be compatible with Linux, macOS, and Windows - use SDKs over binaries, usefilepath.Join()instead of h...
Files:
tools/director/cmd/validate.gopkg/utils/highlight_utils.gotools/director/cmd/render.gotools/director/cmd/export_manifest.go
**/{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, usingnilif no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Files:
tools/director/cmd/validate.gopkg/utils/highlight_utils.gotools/director/cmd/render.gotools/director/cmd/export_manifest.go
🧠 Learnings (52)
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-18T12:26:25.329Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.
Applied to files:
demos/scenes/terraform/init.tapedemos/scenes/terraform/destroy.tapedemos/scenes/featured/tools-just-work.tape
📚 Learning: 2024-12-03T05:29:07.718Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 810
File: internal/exec/terraform_utils.go:145-146
Timestamp: 2024-12-03T05:29:07.718Z
Learning: In the Atmos project, a 5-minute timeout in the `execTerraformOutput` function is acceptable for retrieving `terraform output` for a component in a stack.
Applied to files:
demos/scenes/terraform/init.tapedemos/scenes/terraform/destroy.tapedemos/scenes/toolchain-deps.tape
📚 Learning: 2026-01-09T04:49:35.038Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1907
File: demos/fixtures/acme/stacks/catalog/api.yaml:1-29
Timestamp: 2026-01-09T04:49:35.038Z
Learning: In the cloudposse/atmos demos/fixtures, components can provide Terraform outputs via `remote_state_backend.static` configuration blocks instead of traditional Terraform output blocks. This pattern is used for demo/fixture purposes to simulate cross-component state references without deploying actual infrastructure. The `!terraform.state` YAML function reads from these static backends.
Applied to files:
demos/scenes/terraform/init.tapedemos/fixtures/acme/components/terraform/cdn/main.tf
📚 Learning: 2025-01-09T22:27:25.538Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/validate_stacks.go:20-23
Timestamp: 2025-01-09T22:27:25.538Z
Learning: The validate commands in Atmos can have different help handling implementations. Specifically, validate_component.go and validate_stacks.go are designed to handle help requests differently, with validate_stacks.go including positional argument checks while validate_component.go does not.
Applied to files:
demos/scenes/config/validate-stacks.tapetools/director/cmd/validate.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Applied to files:
tools/director/cmd/validate.gotools/director/cmd/render.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
tools/director/cmd/validate.gotools/director/cmd/render.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-02-03T06:00:11.419Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/describe_config.go:20-20
Timestamp: 2025-02-03T06:00:11.419Z
Learning: Commands should use `PrintErrorMarkdownAndExit` with empty title and suggestion (`"", err, ""`) for general error handling. Specific titles like "Invalid Usage" or "File Not Found" should only be used for validation or specific error scenarios.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in `errors/errors.go` - use `errors.Join` for combining errors, `fmt.Errorf` with `%w` for context, and `errors.Is()` for error checking
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
tools/director/cmd/validate.gotools/director/cmd/render.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-12-13T06:07:37.766Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Applied to files:
tools/director/cmd/validate.godemos/scenes/terraform/destroy.tape
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
tools/director/cmd/validate.gotools/director/cmd/render.go
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
tools/director/cmd/validate.godemos/scenes/commands.tapedemos/scenes/terraform/destroy.tape
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-01-25T04:01:58.095Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2024-10-20T13:06:20.839Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 736
File: pkg/utils/file_utils.go:177-194
Timestamp: 2024-10-20T13:06:20.839Z
Learning: In `pkg/utils/file_utils.go`, the `SearchConfigFile` function should focus on returning the file path and an `exists` boolean without returning an error.
Applied to files:
tools/director/cmd/validate.go
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
tools/director/cmd/validate.gotools/director/cmd/render.gotools/director/cmd/export_manifest.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
tools/director/cmd/validate.gopkg/utils/highlight_utils.gotools/director/cmd/render.gotools/director/cmd/export_manifest.go
📚 Learning: 2025-06-07T19:27:40.807Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1266
File: pkg/utils/highlight_utils.go:0-0
Timestamp: 2025-06-07T19:27:40.807Z
Learning: In pkg/utils/highlight_utils.go, the global variable `isTermPresent` that caches terminal detection at package initialization is an intentional design choice and should not be changed to per-call detection.
Applied to files:
pkg/utils/highlight_utils.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
pkg/utils/highlight_utils.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/*.go : Use colors from `pkg/ui/theme/colors.go` for all UI theming in Go code
Applied to files:
pkg/utils/highlight_utils.go
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
pkg/utils/highlight_utils.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Write code assuming full TTY - system automatically handles color degradation, width adaptation, TTY detection, CI detection, markdown rendering, icon support, and secret masking
Applied to files:
pkg/utils/highlight_utils.go
📚 Learning: 2025-10-14T01:54:48.410Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-completion--help.html:2-7
Timestamp: 2025-10-14T01:54:48.410Z
Learning: Screengrab HTML files in website/src/components/Screengrabs/ are generated from actual Atmos CLI output converted to HTML. The ANSI-art headers and formatting in these files are intentional and reflect the real CLI user experience, so they should not be suggested for removal or modification.
Applied to files:
demos/scenes/commands.tapedemos/scenes/version.tape.claude/agents/director.mddemos/scenes/config/describe-affected.tapedemos/scenes/flagship/tui.tape
📚 Learning: 2026-01-09T04:49:27.370Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1907
File: demos/fixtures/acme/stacks/catalog/api.yaml:1-29
Timestamp: 2026-01-09T04:49:27.370Z
Learning: In Atmos demo fixtures under demos/fixtures, avoid using real Terraform output blocks. Instead, provide simulated outputs via remote_state_backend.static blocks and use the !terraform.state YAML function to read from these static backends. This helps demonstrate cross-component references without provisioning infrastructure. Apply this pattern to all YAML fixture files under demos/fixtures as appropriate.
Applied to files:
demos/fixtures/acme/stacks/catalog/database.yaml
📚 Learning: 2024-11-22T12:38:33.132Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_utils.go:496-513
Timestamp: 2024-11-22T12:38:33.132Z
Learning: In the Atmos project, continue to flag path traversal issues in code reviews but acknowledge when they are expected and acceptable in specific cases.
Applied to files:
tools/director/cmd/render.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
tools/director/cmd/render.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/{pkg,internal,cmd}/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` plus blank line to all public functions, using `nil` if no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Applied to files:
tools/director/cmd/export_manifest.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
tools/director/cmd/export_manifest.go
📚 Learning: 2025-11-30T04:16:24.155Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:34-48
Timestamp: 2025-11-30T04:16:24.155Z
Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.
Applied to files:
tools/director/cmd/export_manifest.go
📚 Learning: 2025-11-30T04:16:01.899Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:50-59
Timestamp: 2025-11-30T04:16:01.899Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` should NOT be added to trivial O(1) getter methods that only return field references or check map lengths (e.g., `GetDeferredValues()`, `HasDeferredValues()`). The guideline to add perf tracking to "all public functions" applies to functions that do meaningful work (I/O, computation, external calls), not to trivial accessors where the tracking overhead would exceed the operation time and pollute performance reports.
Applied to files:
tools/director/cmd/export_manifest.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
Applied to files:
tools/director/cmd/export_manifest.go
📚 Learning: 2024-11-12T13:06:56.194Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: website/docs/cheatsheets/vendoring.mdx:70-70
Timestamp: 2024-11-12T13:06:56.194Z
Learning: In `atmos vendor pull --everything`, the `--everything` flag uses the TTY for TUI but is not interactive.
Applied to files:
demos/scenes/flagship/tui.tape
📚 Learning: 2024-10-21T17:51:07.087Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-10-21T17:51:07.087Z
Learning: Use `bubbletea` for confirmation prompts instead of `fmt.Scanln` in the `atmos terraform clean` command.
Applied to files:
demos/scenes/terraform/destroy.tape
📚 Learning: 2025-09-07T17:38:17.619Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/src/components/Screengrabs/demo-stacks/deploy-dev.html:28-37
Timestamp: 2025-09-07T17:38:17.619Z
Learning: Screengrab files in the CloudPosse/atmos repository are programmatically generated and should be ignored during code reviews. Do not provide suggestions or comments on files in screengrab directories or screengrab-related HTML files.
Applied to files:
demos/scenes/terraform/destroy.tape
📚 Learning: 2024-10-21T17:51:53.976Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-10-21T17:51:53.976Z
Learning: When `atmos terraform clean --everything` is used without specifying a component and without the `--force` flag, prompt the user for confirmation before deleting all components. Use the `--force` flag to skip the confirmation prompt.
Applied to files:
demos/scenes/terraform/destroy.tape
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
demos/scenes/terraform/destroy.tape
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Applied to files:
demos/scenes/terraform/destroy.tape
📚 Learning: 2025-11-07T14:52:55.217Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1761
File: docs/prd/claude-agent-architecture.md:331-439
Timestamp: 2025-11-07T14:52:55.217Z
Learning: In the cloudposse/atmos repository, Claude agents are used as interactive tools, not in automated/headless CI/CD contexts. Agent documentation and patterns should assume synchronous human interaction.
Applied to files:
demos/scenes/featured/tools-just-work.tape
🧬 Code graph analysis (2)
pkg/utils/highlight_utils.go (2)
internal/tui/templates/term/term_writer.go (1)
IsTTYSupportForStdout(121-123)pkg/schema/schema.go (2)
Settings(985-989)Terminal(284-297)
tools/director/cmd/render.go (9)
tools/director/internal/toolmgr/config.go (2)
LoadDefaultsConfig(47-64)DefaultsConfig(14-19)tools/director/internal/scene/scene.go (3)
Scene(20-35)ScenesList(14-17)ValidationConfig(48-51)tools/director/internal/vhs/vhs.go (4)
CheckInstalled(14-19)ValidateTape(41-50)CheckSVGSupport(23-35)Render(57-72)pkg/ffmpeg/ffmpeg.go (1)
CheckInstalled(67-77)tools/director/internal/vhs/preprocess.go (1)
PreprocessTape(14-30)tools/director/internal/vhs/renderer.go (1)
NewRenderer(36-42)pkg/schema/schema.go (2)
Validation(903-903)Validate(241-243)tools/director/internal/tape/parser.go (2)
ParseCommands(30-91)FilterExecutable(94-102)tools/director/internal/tape/executor.go (2)
ExecuteCommandDirect(17-55)ExecuteCommand(86-88)
🪛 LanguageTool
.claude/agents/director.md
[typographical] ~38-~38: Consider using typographic quotation marks here.
Context: ...?** → Fix the tape (last resort) Never "dumb down" a tape to make it pass. If a documented...
(EN_QUOTES)
[typographical] ~116-~116: To join two clauses or introduce examples, consider using an em dash.
Context: ...--test ``` Flags: - --test / `-T` - Run commands, show pass/fail only (buffe...
(DASH_RULE)
[typographical] ~117-~117: To join two clauses or introduce examples, consider using an em dash.
Context: ...ail only (buffered) - --verbose / -v - Run commands with streaming output to te...
(DASH_RULE)
[typographical] ~201-~201: To join two clauses or introduce examples, consider using an em dash.
Context: ...ifferent purposes: ### fixtures/acme/ - Offline Demo Data - Works without cloud ...
(DASH_RULE)
[typographical] ~207-~207: To join two clauses or introduce examples, consider using an em dash.
Context: ... stack exploration ### fixtures/live/ - Terraform Execution - For demos that run...
(DASH_RULE)
[style] ~311-~311: Consider a different adjective to strengthen your wording.
Context: ...nfiguration ``` ## Using Task Tool for Deep Investigation When investigating atmos...
(DEEP_PROFOUND)
🪛 markdownlint-cli2 (0.18.1)
.claude/agents/director.md
173-173: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
222-222: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
315-315: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Summary
| Type "# Quick identity check across all providers" | ||
| Enter | ||
| Sleep 1s | ||
|
|
||
|
|
||
| # Hold final frame | ||
| Sleep 10s |
There was a problem hiding this comment.
Incomplete scene flow - comment without follow-up action.
Lines 38-40 type a comment about "Quick identity check across all providers" but no command follows. This leaves the viewer expecting an action that never happens. Either remove this dangling comment or add the corresponding command.
Also, line 42 has an extra blank line.
Suggested fix
Option A - Remove the dangling comment:
# Capture screenshot
Screenshot auth-whoami.png
-Type "# Quick identity check across all providers"
-Enter
-Sleep 1s
-
-
# Hold final frame
Sleep 10sOption B - Or add the missing command if intended:
Type "# Quick identity check across all providers"
Enter
Sleep 1s
-
+Type "atmos auth whoami --all"
+Sleep 2s
+Enter
+Sleep 3s
# Hold final frame
Sleep 10s📝 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.
| Type "# Quick identity check across all providers" | |
| Enter | |
| Sleep 1s | |
| # Hold final frame | |
| Sleep 10s | |
| # Capture screenshot | |
| Screenshot auth-whoami.png | |
| # Hold final frame | |
| Sleep 10s |
| Type "# Quick identity check across all providers" | |
| Enter | |
| Sleep 1s | |
| # Hold final frame | |
| Sleep 10s | |
| Type "# Quick identity check across all providers" | |
| Enter | |
| Sleep 1s | |
| Type "atmos auth whoami --all" | |
| Sleep 2s | |
| Enter | |
| Sleep 3s | |
| # Hold final frame | |
| Sleep 10s |
🤖 Prompt for AI Agents
In @demos/scenes/auth/whoami.tape around lines 38 - 44, The scene contains a
dangling comment "Type \"# Quick identity check across all providers\"" with no
following command and an extra blank line; either remove that comment and the
trailing blank line (to avoid misleading viewers) or replace it by adding the
intended command(s) (e.g., the identity check or provider invocation)
immediately after the comment so the sequence after Sleep 1s performs the
described action; update the block containing the lines that type the comment
and the Sleep 1s / Sleep 10s flow accordingly (the area mentioning the comment
and the Sleep commands).
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
…y extension The goldmark linkify extension was using ast.NewString() to replace auto-linked package references like foo/bar@1.2.3. However, ast.String nodes are not handled by glamour's ANSI renderer, causing: - "Warning: unhandled element String" errors in output - Package reference text being completely dropped from rendered output Changed to use ast.NewTextSegment() which creates proper ast.Text nodes that glamour renders correctly. Added findLabelOffset helper to locate the label position in source text for segment creation. Added TestCustomRenderer_PackageReferences integration test that verifies package references render correctly through the full glamour pipeline. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix $GITHUB_OUTPUT bug by parsing Env directives from tape files - Unescape \$ to $ in Type commands for proper shell expansion - Pass tape environment variables to command executor - Replace magic strings with constants in toolchain/install.go Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add cross-platform provider hashes to .terraform.lock.hcl - Update manifest.json with new demo video URLs and metadata Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
toolchain/install.go (1)
112-151: Single-tool flow bypasses the reinstall flag entirely—it always installs regardless of binary state.The
installSingleToolSpecfunction intentionally dropsreinstallFlagandInstallSingleTooldoesn't accept or use it. Meanwhile,installMultipleToolsandinstallFromToolVersionsboth honor the flag viainstallOrSkipToolto skip installation when binaries already exist. This creates inconsistent behavior: reinstalling a single tool always performs a full reinstall, but batch operations respect the flag and can skip already-installed versions.Either remove the flag from
RunInstall's single-tool branch to match the actual behavior, or wire it throughInstallSingleToolso all flows are consistent.
🤖 Fix all issues with AI agents
In `@pkg/ui/markdown/extensions/linkify.go`:
- Around line 64-74: nodesToReplace may get the same autoLink appended twice
when both conditions hit (strings.Contains(url, "/") and
!StrictEmailRegexp.MatchString(url)), causing ReplaceChild to operate on an
already-replaced node; modify the logic around nodesToReplace and the checks for
strings.Contains(url, "/") and StrictEmailRegexp so you only append autoLink
once (e.g., check if autoLink is already present in nodesToReplace or use a
boolean flag/temporary set keyed by the autoLink reference before appending) to
ensure ReplaceChild is only called on unique nodes.
In `@toolchain/install.go`:
- Around line 363-376: Comments describing the progress bar strategy are missing
terminal periods and should be punctuated; update the three comment lines in the
block that mentions "Strategy: Status messages..." so each sentence ends with a
period. Also fix the skipped message in the status switch: replace the
double-parentheses text in the statusSkipped branch by a single parenthetical
and use proper wording — update the ui.Successf call in the case handling
state.result == statusSkipped (the statusSkipped branch that currently calls
ui.Successf("Skipped `%s/%s@%s` ((already installed))", tool.owner, tool.repo,
tool.version)) to use a single parenthesis form like "Skipped `%s/%s@%s`
(already installed)". Ensure you adjust only those comment sentences and the
statusSkipped ui.Successf invocation; leave terminal.EscResetLine and other
cases untouched.
In `@tools/director/cmd/render.go`:
- Around line 383-403: The hook/prep execution currently hardcodes
exec.CommandContext(ctx, "sh", "-c", cmdStr) which breaks on Windows; modify the
command construction (where exec.CommandContext is called for hooks and the prep
commands around the other occurrence) to choose the shell based on runtime.GOOS:
use exec.CommandContext(ctx, "cmd.exe", "/C", cmdStr) when runtime.GOOS ==
"windows" and keep "sh", "-c" on Unix-like systems, preserving existing Dir,
Stdout, Stderr and Env handling (including using extraEnv); factor the platform
selection into a small helper (e.g., buildShellCommand or makeShellCmd) and
replace the inline exec.CommandContext calls with that helper to keep behavior
consistent across platforms.
- Around line 108-141: Import the error sentinels package as errUtils
("github.com/cloudposse/atmos/errors") and replace the raw fmt.Errorf returns in
this block: wrap the error from toolmgr.LoadDefaultsConfig with
fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrReadConfig, err); wrap the error
from runPreRenderHooks with fmt.Errorf(errUtils.ErrWrapFormat,
errUtils.ErrRunPreRenderHooks, err) (or ErrRunHooks if that sentinel is used in
the project); wrap the error from mgr.EnsureInstalled with
fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrToolInstall, err); and wrap the
error from scene.LoadScenes with fmt.Errorf(errUtils.ErrWrapFormat,
errUtils.ErrReadConfig, err). Keep the surrounding logic (defaultsConfig,
toolsConfig, runPreRenderHooks, New/EnsureInstalled, PrependToPath, scenesFile)
unchanged.
In `@tools/director/internal/tape/parser.go`:
- Around line 15-17: The struct field comments for Line, Text, and IsComment in
parser.go need terminal periods to satisfy godoc/godot rules; update the comment
strings for the fields Line, Text, and IsComment in the relevant struct (the
Tape parser line struct) so each comment ends with a period (e.g., "Line number
in tape file (1-indexed)." "The command text." "True if this is a comment line
(starts with #).").
- Around line 126-134: FilterExecutable currently performs non-trivial work but
doesn't record performance metrics; add a perf.Track invocation at the start of
the function to measure its execution time. Specifically, in FilterExecutable
insert a call to perf.Track (using the project's perf API pattern) with a
descriptive name like "FilterExecutable" and defer the appropriate stop/record
call so the timing is captured for the whole function; ensure the perf package
is imported if missing and that the existing return value (executable []Command)
is unchanged.
♻️ Duplicate comments (1)
tools/director/internal/tape/parser.go (1)
52-55: Wrap I/O errors with static errors fromerrors/errors.go.
Current wrapping lacks the required static sentinel.As per coding guidelines.
Run this to find the correct static errors:
#!/bin/bash set -euo pipefail fd --type f 'errors.go' -x rg -n 'Err[A-Za-z0-9_]+' {}Also applies to: 119-121
🧹 Nitpick comments (3)
pkg/ui/markdown/extensions/linkify.go (1)
103-119: Consider usingbytes.Indexfrom stdlib.This function reimplements what
bytes.Index(source, label)already provides.Suggested simplification
+import "bytes" + // findLabelOffset finds the byte offset of label in source. // Returns -1 if not found. func findLabelOffset(source, label []byte) int { - for i := 0; i <= len(source)-len(label); i++ { - match := true - for j := 0; j < len(label); j++ { - if source[i+j] != label[j] { - match = false - break - } - } - if match { - return i - } - } - return -1 + return bytes.Index(source, label) }Or inline directly at the call site and remove the helper entirely.
pkg/ui/markdown/custom_renderer_test.go (2)
255-286: Consider replacing magic ANSI indices with shared color constants.
Using the markdown color constants would keep tests aligned if theme values change.Based on learnings, consider the constants in
pkg/ui/markdown/colors.go(e.g., Purple) instead of raw codes.Also applies to: 316-379
421-477: Table-drive the multi-scenario option tests.
This trims repetition while keeping the same assertions.As per coding guidelines, prefer table-driven tests for multiple scenarios.♻️ Suggested refactor (table-driven subtests)
func TestWithStylesFromJSONBytes(t *testing.T) { - // Test with valid JSON. - validJSON := `{ - "document": { - "color": "#ffffff" - } - }` - renderer, err := NewCustomRenderer( - WithStylesFromJSONBytes([]byte(validJSON)), - WithColorProfile(termenv.TrueColor), - ) - require.NoError(t, err) - assert.NotNil(t, renderer) - - // Test with invalid JSON (should not error, just ignore). - invalidJSON := `{invalid}` - renderer, err = NewCustomRenderer( - WithStylesFromJSONBytes([]byte(invalidJSON)), - ) - require.NoError(t, err) - assert.NotNil(t, renderer) + tests := []struct { + name string + opts []CustomRendererOption + }{ + { + name: "valid JSON", + opts: []CustomRendererOption{ + WithStylesFromJSONBytes([]byte(`{ + "document": { + "color": "#ffffff" + } + }`)), + WithColorProfile(termenv.TrueColor), + }, + }, + { + name: "invalid JSON (ignored)", + opts: []CustomRendererOption{ + WithStylesFromJSONBytes([]byte(`{invalid}`)), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + renderer, err := NewCustomRenderer(tt.opts...) + require.NoError(t, err) + assert.NotNil(t, renderer) + }) + } } func TestCustomRenderer_Options(t *testing.T) { // Test that options are applied correctly by verifying renderer creation succeeds // and renders properly with each option. We can't inspect internal state directly // since the CustomRenderer now wraps glamour.TermRenderer. - t.Run("WithWordWrap creates valid renderer", func(t *testing.T) { - renderer, err := NewCustomRenderer(WithWordWrap(40)) - require.NoError(t, err) - assert.NotNil(t, renderer) - // Verify it renders - output, err := renderer.Render("test") - require.NoError(t, err) - assert.NotEmpty(t, output) - }) - - t.Run("WithColorProfile creates valid renderer", func(t *testing.T) { - renderer, err := NewCustomRenderer(WithColorProfile(termenv.ANSI256)) - require.NoError(t, err) - assert.NotNil(t, renderer) - // Verify it renders - output, err := renderer.Render("test") - require.NoError(t, err) - assert.NotEmpty(t, output) - }) - - t.Run("WithPreservedNewLines creates valid renderer", func(t *testing.T) { - renderer, err := NewCustomRenderer(WithPreservedNewLines()) - require.NoError(t, err) - assert.NotNil(t, renderer) - // Verify it renders - output, err := renderer.Render("test") - require.NoError(t, err) - assert.NotEmpty(t, output) - }) + tests := []struct { + name string + opts []CustomRendererOption + }{ + {name: "WithWordWrap creates valid renderer", opts: []CustomRendererOption{WithWordWrap(40)}}, + {name: "WithColorProfile creates valid renderer", opts: []CustomRendererOption{WithColorProfile(termenv.ANSI256)}}, + {name: "WithPreservedNewLines creates valid renderer", opts: []CustomRendererOption{WithPreservedNewLines()}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + renderer, err := NewCustomRenderer(tt.opts...) + require.NoError(t, err) + assert.NotNil(t, renderer) + // Verify it renders. + output, err := renderer.Render("test") + require.NoError(t, err) + assert.NotEmpty(t, output) + }) + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
demos/fixtures/live/components/terraform/vpc/.terraform.lock.hclpkg/ui/markdown/custom_renderer_test.gopkg/ui/markdown/extensions/linkify.gotoolchain/install.gotools/director/cmd/render.gotools/director/internal/tape/parser.gowebsite/src/data/manifest.json
✅ Files skipped from review due to trivial changes (1)
- demos/fixtures/live/components/terraform/vpc/.terraform.lock.hcl
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter) in Go code
Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (cfg,log,u,errUtils)
All errors MUST be wrapped using static errors defined inerrors/errors.go- useerrors.Joinfor combining errors,fmt.Errorfwith%wfor context, anderrors.Is()for error checking
Never manually create mocks - usego.uber.org/mock/mockgenwith//go:generatedirectives in Go code
Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use//revive:disable:file-length-limit
Use colors frompkg/ui/theme/colors.gofor all UI theming in Go code
Code must be compatible with Linux, macOS, and Windows - use SDKs over binaries, usefilepath.Join()instead of h...
Files:
pkg/ui/markdown/custom_renderer_test.gotools/director/cmd/render.gopkg/ui/markdown/extensions/linkify.gotools/director/internal/tape/parser.gotoolchain/install.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesPrefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with
go.uber.org/mock/mockgen, use table-driven tests, target >80% coverage
Files:
pkg/ui/markdown/custom_renderer_test.go
**/{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, usingnilif no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Files:
pkg/ui/markdown/custom_renderer_test.gotools/director/cmd/render.gopkg/ui/markdown/extensions/linkify.gotools/director/internal/tape/parser.go
🧠 Learnings (37)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/ui/markdown/custom_renderer_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/ui/markdown/custom_renderer_test.gotools/director/cmd/render.go
📚 Learning: 2025-02-05T20:16:24.036Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: pkg/ui/markdown/renderer.go:75-75
Timestamp: 2025-02-05T20:16:24.036Z
Learning: The Purple constant for markdown rendering is defined in pkg/ui/markdown/colors.go.
Applied to files:
pkg/ui/markdown/custom_renderer_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/ui/markdown/custom_renderer_test.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with `go.uber.org/mock/mockgen`, use table-driven tests, target >80% coverage
Applied to files:
pkg/ui/markdown/custom_renderer_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/ui/markdown/custom_renderer_test.gotools/director/cmd/render.gopkg/ui/markdown/extensions/linkify.gotools/director/internal/tape/parser.gotoolchain/install.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
tools/director/cmd/render.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
tools/director/cmd/render.gotoolchain/install.go
📚 Learning: 2024-11-22T12:38:33.132Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_utils.go:496-513
Timestamp: 2024-11-22T12:38:33.132Z
Learning: In the Atmos project, continue to flag path traversal issues in code reviews but acknowledge when they are expected and acceptable in specific cases.
Applied to files:
tools/director/cmd/render.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
tools/director/cmd/render.go
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
tools/director/cmd/render.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Document complex logic with inline comments in Go code
Applied to files:
tools/director/internal/tape/parser.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Applied to files:
tools/director/internal/tape/parser.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/{pkg,internal,cmd}/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` plus blank line to all public functions, using `nil` if no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Applied to files:
tools/director/internal/tape/parser.gotoolchain/install.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in `errors/errors.go` - use `errors.Join` for combining errors, `fmt.Errorf` with `%w` for context, and `errors.Is()` for error checking
Applied to files:
tools/director/internal/tape/parser.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
tools/director/internal/tape/parser.gotoolchain/install.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
tools/director/internal/tape/parser.go
📚 Learning: 2025-02-03T06:00:11.419Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/describe_config.go:20-20
Timestamp: 2025-02-03T06:00:11.419Z
Learning: Commands should use `PrintErrorMarkdownAndExit` with empty title and suggestion (`"", err, ""`) for general error handling. Specific titles like "Invalid Usage" or "File Not Found" should only be used for validation or specific error scenarios.
Applied to files:
tools/director/internal/tape/parser.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
tools/director/internal/tape/parser.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
tools/director/internal/tape/parser.gotoolchain/install.go
📚 Learning: 2025-11-30T04:16:24.155Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:34-48
Timestamp: 2025-11-30T04:16:24.155Z
Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.
Applied to files:
tools/director/internal/tape/parser.gotoolchain/install.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
tools/director/internal/tape/parser.gotoolchain/install.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
Applied to files:
tools/director/internal/tape/parser.gotoolchain/install.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
toolchain/install.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
toolchain/install.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
toolchain/install.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
Applied to files:
toolchain/install.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
toolchain/install.go
📚 Learning: 2025-01-25T04:01:58.095Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
Applied to files:
toolchain/install.go
📚 Learning: 2024-12-13T15:28:13.630Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/version.go:34-44
Timestamp: 2024-12-13T15:28:13.630Z
Learning: In `cmd/version.go`, when handling the `--check` flag in the `versionCmd`, avoid using `CheckForAtmosUpdateAndPrintMessage(cliConfig)` as it updates the cache timestamp, which may not be desired in this context.
Applied to files:
toolchain/install.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
toolchain/install.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
toolchain/install.go
📚 Learning: 2025-11-30T04:16:01.899Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:50-59
Timestamp: 2025-11-30T04:16:01.899Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` should NOT be added to trivial O(1) getter methods that only return field references or check map lengths (e.g., `GetDeferredValues()`, `HasDeferredValues()`). The guideline to add perf tracking to "all public functions" applies to functions that do meaningful work (I/O, computation, external calls), not to trivial accessors where the tracking overhead would exceed the operation time and pollute performance reports.
Applied to files:
toolchain/install.go
📚 Learning: 2025-10-13T18:13:54.020Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1622
File: pkg/perf/perf.go:140-184
Timestamp: 2025-10-13T18:13:54.020Z
Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.
Applied to files:
toolchain/install.go
📚 Learning: 2024-10-21T17:51:07.087Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-10-21T17:51:07.087Z
Learning: Use `bubbletea` for confirmation prompts instead of `fmt.Scanln` in the `atmos terraform clean` command.
Applied to files:
toolchain/install.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Use `ui.Write/Writef/Writeln()` and `ui.Success/Error/Warning/Info()` for human messages to stderr in Go commands
Applied to files:
toolchain/install.go
🧬 Code graph analysis (1)
pkg/ui/markdown/custom_renderer_test.go (1)
pkg/ui/markdown/custom_renderer.go (6)
CustomRendererOption(32-32)WithWordWrap(43-47)WithColorProfile(50-54)WithPreservedNewLines(64-68)NewCustomRenderer(90-112)WithStylesFromJSONBytes(71-78)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (21)
pkg/ui/markdown/extensions/linkify.go (4)
1-13: Looks good.Package documentation and import organization are solid.
15-26: Well documented regex with clear intent.The TLD requirement (
[a-zA-Z]{2,}) effectively filters out version-like suffixes. Examples in the doc comment make the behavior crystal clear.
79-101: Replacement logic is sound.Good defensive check for nil parent. The fallback to
ast.NewStringhandles edge cases gracefully.
121-145: Clean extension implementation.Factory pattern is appropriate here. Documentation clearly explains the two-phase approach (linkify runs first, then this transformer cleans up).
toolchain/install.go (9)
13-13: Import update looks fine.
21-24: Status constants improve readability.
89-109: RunInstall branching reads clean and predictable.
154-165: Parsing helper looks good.
168-195: Good error handling and resolution flow here.
198-235: Batch install flow and summary integration look solid.
306-311: Status constant usage is consistent.
337-345: Clear return statuses for install outcomes.
394-408: Summary formatting update looks good.pkg/ui/markdown/custom_renderer_test.go (7)
12-31: Clean ANSI-stripping helper for tests.
Straightforward and scoped to test needs.
33-87: Constructor coverage looks solid.
The option matrix is clear and well-scoped.
89-143: Basic markdown + muted coverage is solid.
Nice breadth of scenarios and assertions.Also applies to: 145-189
191-201: Strikethrough/highlight cases look good.
Concise and readable.Also applies to: 203-245
387-419: Combined syntax + Close coverage is solid.
Good integration-style smoke check.
479-529: Edge-case coverage looks thorough.
Nice set of malformed inputs.
531-599: Package-reference/linkify coverage is strong.
Good guardrails around mailto handling.tools/director/cmd/render.go (1)
410-487: Validation flow is clear and user‑friendly.The rendered‑scene filtering, SVG discovery, and per‑scene reporting are clean and make failures easy to diagnose.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Check if it looks like a package reference (contains "/"). | ||
| // Valid emails cannot contain "/" so this is safe. | ||
| if strings.Contains(url, "/") { | ||
| nodesToReplace = append(nodesToReplace, autoLink) | ||
| } | ||
|
|
||
| // Also check if domain part looks like a version (e.g., @1.0.0). | ||
| // Valid emails have letter TLDs, not numeric ones. | ||
| if !StrictEmailRegexp.MatchString(url) { | ||
| nodesToReplace = append(nodesToReplace, autoLink) | ||
| } |
There was a problem hiding this comment.
Potential duplicate entries in nodesToReplace.
If a URL contains "/" and also fails the StrictEmailRegexp match (e.g., foo/bar@1.0.0), it gets appended twice. This could cause issues when ReplaceChild is called on an already-replaced node.
Proposed fix
// Check if it looks like a package reference (contains "/").
// Valid emails cannot contain "/" so this is safe.
- if strings.Contains(url, "/") {
- nodesToReplace = append(nodesToReplace, autoLink)
- }
-
- // Also check if domain part looks like a version (e.g., `@1.0.0`).
- // Valid emails have letter TLDs, not numeric ones.
- if !StrictEmailRegexp.MatchString(url) {
+ // Also check if domain part looks like a version (e.g., `@1.0.0`).
+ // Valid emails have letter TLDs, not numeric ones.
+ if strings.Contains(url, "/") || !StrictEmailRegexp.MatchString(url) {
nodesToReplace = append(nodesToReplace, autoLink)
}🤖 Prompt for AI Agents
In `@pkg/ui/markdown/extensions/linkify.go` around lines 64 - 74, nodesToReplace
may get the same autoLink appended twice when both conditions hit
(strings.Contains(url, "/") and !StrictEmailRegexp.MatchString(url)), causing
ReplaceChild to operate on an already-replaced node; modify the logic around
nodesToReplace and the checks for strings.Contains(url, "/") and
StrictEmailRegexp so you only append autoLink once (e.g., check if autoLink is
already present in nodesToReplace or use a boolean flag/temporary set keyed by
the autoLink reference before appending) to ensure ReplaceChild is only called
on unique nodes.
toolchain/install.go
Outdated
| // Strategy: Status messages scroll up normally, progress bar updates in place at bottom. | ||
| // 1. Clear current line (where progress bar was) | ||
| // 2. Print status message with newline (scrolls up, cursor on new line) | ||
| // 3. Print progress bar on current line (no newline, overwrites itself each frame) | ||
|
|
||
| // Clear the progress bar line first. | ||
| _ = ui.Write(terminal.EscResetLine) | ||
|
|
||
| // Print status message (with newline - this becomes a permanent line that scrolls up). | ||
| switch state.result { | ||
| case "skipped": | ||
| _ = ui.Successf("Skipped `%s/%s@%s` (already installed)", tool.owner, tool.repo, tool.version) | ||
| case "installed": | ||
| case statusSkipped: | ||
| _ = ui.Successf("Skipped `%s/%s@%s` ((already installed))", tool.owner, tool.repo, tool.version) | ||
| case statusInstalled: | ||
| _ = ui.Successf("Installed `%s/%s@%s`", tool.owner, tool.repo, tool.version) |
There was a problem hiding this comment.
Fix godot punctuation and the skipped message text.
Line 364–366 comments are missing terminal periods (godot), and the skipped message has double parentheses. As per coding guidelines.
🔧 Proposed fix
- // 1. Clear current line (where progress bar was)
- // 2. Print status message with newline (scrolls up, cursor on new line)
- // 3. Print progress bar on current line (no newline, overwrites itself each frame)
+ // 1. Clear current line (where progress bar was).
+ // 2. Print status message with newline (scrolls up, cursor on new line).
+ // 3. Print progress bar on current line (no newline, overwrites itself each frame).
...
- _ = ui.Successf("Skipped `%s/%s@%s` ((already installed))", tool.owner, tool.repo, tool.version)
+ _ = ui.Successf("Skipped `%s/%s@%s` (already installed)", tool.owner, tool.repo, tool.version)🤖 Prompt for AI Agents
In `@toolchain/install.go` around lines 363 - 376, Comments describing the
progress bar strategy are missing terminal periods and should be punctuated;
update the three comment lines in the block that mentions "Strategy: Status
messages..." so each sentence ends with a period. Also fix the skipped message
in the status switch: replace the double-parentheses text in the statusSkipped
branch by a single parenthetical and use proper wording — update the ui.Successf
call in the case handling state.result == statusSkipped (the statusSkipped
branch that currently calls ui.Successf("Skipped `%s/%s@%s` ((already
installed))", tool.owner, tool.repo, tool.version)) to use a single parenthesis
form like "Skipped `%s/%s@%s` (already installed)". Ensure you adjust only those
comment sentences and the statusSkipped ui.Successf invocation; leave
terminal.EscResetLine and other cases untouched.
| defaultsConfig, err := toolmgr.LoadDefaultsConfig(demosDir) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load defaults config: %w", err) | ||
| } | ||
|
|
||
| // Run pre-render hooks before anything else. | ||
| if defaultsConfig != nil && defaultsConfig.Hooks != nil && len(defaultsConfig.Hooks.PreRender) > 0 { | ||
| repoRoot := filepath.Dir(demosDir) | ||
| if err := runPreRenderHooks(ctx, c, defaultsConfig.Hooks.PreRender, repoRoot); err != nil { | ||
| return fmt.Errorf("pre-render hooks failed: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // Load tools configuration and ensure atmos is installed at the correct version. | ||
| var toolsConfig *toolmgr.ToolsConfig | ||
| if defaultsConfig != nil { | ||
| toolsConfig = defaultsConfig.Tools | ||
| } | ||
|
|
||
| if toolsConfig != nil && toolsConfig.Atmos != nil { | ||
| mgr := toolmgr.New(toolsConfig, demosDir) | ||
| atmosPath, err := mgr.EnsureInstalled(ctx, "atmos") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to ensure atmos is installed: %w", err) | ||
| } | ||
| // Prepend install directory to PATH so VHS uses our version. | ||
| toolmgr.PrependToPath(filepath.Dir(atmosPath)) | ||
| } | ||
|
|
||
| scenesFile := filepath.Join(demosDir, "scenes.yaml") | ||
| scenesList, err := scene.LoadScenes(scenesFile) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load scenes: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the actual render.go file
find . -name "render.go" -path "*/tools/director/*" -type fRepository: cloudposse/atmos
Length of output: 90
🏁 Script executed:
# Check the structure of tools/director
ls -la tools/director/ 2>/dev/null || echo "Directory not found"Repository: cloudposse/atmos
Length of output: 575
🏁 Script executed:
# Look at errors/errors.go to understand the sentinel pattern
head -50 errors/errors.goRepository: cloudposse/atmos
Length of output: 3397
🏁 Script executed:
# Search for error definitions in errors/errors.go, especially relevant ones
rg "var \(Err|ErrTools)" errors/errors.go -A 2Repository: cloudposse/atmos
Length of output: 148
🏁 Script executed:
# Check if tools/director has its own errors package
find tools/director -name "errors.go" -o -name "*error*" | head -20Repository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
# Examine the actual render.go content to see error handling
cat -n tools/director/cmd/render.go | head -150Repository: cloudposse/atmos
Length of output: 5550
🏁 Script executed:
# Check tools/director/go.mod to see if it imports the main errors package
cat tools/director/go.modRepository: cloudposse/atmos
Length of output: 9569
🏁 Script executed:
# Search for any error definitions in tools/director
rg "var.*Err" tools/director --type go | head -20Repository: cloudposse/atmos
Length of output: 269
🏁 Script executed:
# Check if render.go imports the errors package from atmos
rg "github.com/cloudposse/atmos" tools/director/cmd/render.goRepository: cloudposse/atmos
Length of output: 430
🏁 Script executed:
# Check if tools/director has its own errors package
find tools/director -name "errors.go" -o -path "*/errors/*"Repository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
# Look at how other files in tools/director handle errors
rg "return fmt.Errorf|return errUtils|errors.Is" tools/director/internal --type go -A 1 | head -30Repository: cloudposse/atmos
Length of output: 1592
🏁 Script executed:
# Check if tools directory imports main atmos errors
rg "errors" tools/director/go.modRepository: cloudposse/atmos
Length of output: 152
🏁 Script executed:
# Find where errUtils is defined/imported in vhs.go
rg "errUtils|ErrVHSNotFound" tools/director/internal/vhs -B 5 | head -40Repository: cloudposse/atmos
Length of output: 1324
🏁 Script executed:
# Check vhs.go imports
head -30 tools/director/internal/vhs/vhs.goRepository: cloudposse/atmos
Length of output: 848
🏁 Script executed:
# Check what error sentinels exist in the main errors package related to loading/rendering
rg "ErrVHS|Err.*Load|Err.*Render|Err.*Config|Err.*Scene|Err.*Install" errors/errors.goRepository: cloudposse/atmos
Length of output: 4468
🏁 Script executed:
# Check if render.go should be importing errUtils like vhs.go does
rg "fmt.Errorf" tools/director/cmd/render.go -B 1 -A 1 | head -20Repository: cloudposse/atmos
Length of output: 606
🏁 Script executed:
# Check entire tools/director for pattern consistency - how many files use errUtils
rg "errUtils" tools/director --type go -lRepository: cloudposse/atmos
Length of output: 135
🏁 Script executed:
# Check if tools/director/internal has its own errors definitions
find tools/director -path "*/errors*" -o -name "*error*" | grep -v testRepository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
# Check the learning about toolchain errors - does it apply to tools/director?
rg "toolchain/registry|toolchain/errors.go" tools/director --type goRepository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
# Let's see if we should add new error sentinels or reuse existing ones
# Check what makes sense for: "failed to load defaults config", "pre-render hooks failed", "failed to load scenes"
rg "LoadDefaultsConfig|LoadScenes|runPreRenderHooks" tools/director --type go -A 2 | head -40Repository: cloudposse/atmos
Length of output: 2576
🏁 Script executed:
# Verify the complete imports section of render.go to see what's missing
head -25 tools/director/cmd/render.goRepository: cloudposse/atmos
Length of output: 653
🏁 Script executed:
# Check validate.go, publish.go to see if they have the same issue
head -25 tools/director/cmd/validate.go
head -25 tools/director/cmd/publish.goRepository: cloudposse/atmos
Length of output: 1507
Import error sentinels and wrap with appropriate error types.
This file returns raw fmt.Errorf values at lines 110, 117, 131, and 140. Other files in tools/director (vhs.go, ffmpeg.go) already import errUtils "github.com/cloudposse/atmos/errors" and wrap with sentinels like ErrReadConfig and ErrToolInstall. Apply the same pattern here: add the import and wrap these returns accordingly (e.g., fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrReadConfig, err) for load failures, or ErrToolInstall for installation issues).
🤖 Prompt for AI Agents
In `@tools/director/cmd/render.go` around lines 108 - 141, Import the error
sentinels package as errUtils ("github.com/cloudposse/atmos/errors") and replace
the raw fmt.Errorf returns in this block: wrap the error from
toolmgr.LoadDefaultsConfig with fmt.Errorf(errUtils.ErrWrapFormat,
errUtils.ErrReadConfig, err); wrap the error from runPreRenderHooks with
fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrRunPreRenderHooks, err) (or
ErrRunHooks if that sentinel is used in the project); wrap the error from
mgr.EnsureInstalled with fmt.Errorf(errUtils.ErrWrapFormat,
errUtils.ErrToolInstall, err); and wrap the error from scene.LoadScenes with
fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrReadConfig, err). Keep the
surrounding logic (defaultsConfig, toolsConfig, runPreRenderHooks,
New/EnsureInstalled, PrependToPath, scenesFile) unchanged.
| // Check if running in Conductor workspace - disable VCS stamping. | ||
| var extraEnv []string | ||
| if os.Getenv("CONDUCTOR_WORKSPACE_PATH") != "" { | ||
| extraEnv = append(extraEnv, "GOFLAGS=-buildvcs=false") | ||
| c.Printf(" (Conductor workspace detected, setting GOFLAGS=-buildvcs=false)\n") | ||
| } | ||
|
|
||
| for i, cmdStr := range hooks { | ||
| c.Printf(" [%d/%d] %s\n", i+1, len(hooks), cmdStr) | ||
|
|
||
| cmd := exec.CommandContext(ctx, "sh", "-c", cmdStr) | ||
| cmd.Dir = workdir | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| if len(extraEnv) > 0 { | ||
| cmd.Env = append(os.Environ(), extraEnv...) | ||
| } | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| return fmt.Errorf("hook %d failed (%s): %w", i+1, cmdStr, err) | ||
| } |
There was a problem hiding this comment.
Use OS‑appropriate shell invocation for hooks and prep commands.
Hardcoding sh/bash will fail on Windows, which violates the repo’s cross‑platform requirement. Consider a small helper to select the shell per runtime.GOOS while preserving current behavior on Unix-like systems.
🛠️ Suggested fix
@@
import (
"context"
"fmt"
"os"
"os/exec"
"path/filepath"
+ "runtime"
"strings"
@@
)
@@
func runPreRenderHooks(ctx context.Context, c *cobra.Command, hooks []string, workdir string) error {
@@
- cmd := exec.CommandContext(ctx, "sh", "-c", cmdStr)
+ cmd := shellCommandContext(ctx, cmdStr, false)
@@
}
+
+func shellCommandContext(ctx context.Context, cmdStr string, preferBash bool) *exec.Cmd {
+ if runtime.GOOS == "windows" {
+ return exec.CommandContext(ctx, "cmd", "/C", cmdStr)
+ }
+ shell := "sh"
+ if preferBash {
+ shell = "bash"
+ }
+ return exec.CommandContext(ctx, shell, "-c", cmdStr)
+}
@@
- prepCmd := exec.CommandContext(ctx, "bash", "-c", prep)
+ prepCmd := shellCommandContext(ctx, prep, true)As per coding guidelines, ...
Also applies to: 508-517
🤖 Prompt for AI Agents
In `@tools/director/cmd/render.go` around lines 383 - 403, The hook/prep execution
currently hardcodes exec.CommandContext(ctx, "sh", "-c", cmdStr) which breaks on
Windows; modify the command construction (where exec.CommandContext is called
for hooks and the prep commands around the other occurrence) to choose the shell
based on runtime.GOOS: use exec.CommandContext(ctx, "cmd.exe", "/C", cmdStr)
when runtime.GOOS == "windows" and keep "sh", "-c" on Unix-like systems,
preserving existing Dir, Stdout, Stderr and Env handling (including using
extraEnv); factor the platform selection into a small helper (e.g.,
buildShellCommand or makeShellCmd) and replace the inline exec.CommandContext
calls with that helper to keep behavior consistent across platforms.
| Line int // Line number in tape file (1-indexed) | ||
| Text string // The command text | ||
| IsComment bool // True if this is a comment line (starts with #) |
There was a problem hiding this comment.
Add periods to field comments.
Required for godot compliance.
Proposed fix
- Line int // Line number in tape file (1-indexed)
- Text string // The command text
- IsComment bool // True if this is a comment line (starts with #)
+ Line int // Line number in tape file (1-indexed).
+ Text string // The command text.
+ IsComment bool // True if this is a comment line (starts with #).As per coding guidelines.
📝 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.
| Line int // Line number in tape file (1-indexed) | |
| Text string // The command text | |
| IsComment bool // True if this is a comment line (starts with #) | |
| Line int // Line number in tape file (1-indexed). | |
| Text string // The command text. | |
| IsComment bool // True if this is a comment line (starts with #). |
🤖 Prompt for AI Agents
In `@tools/director/internal/tape/parser.go` around lines 15 - 17, The struct
field comments for Line, Text, and IsComment in parser.go need terminal periods
to satisfy godoc/godot rules; update the comment strings for the fields Line,
Text, and IsComment in the relevant struct (the Tape parser line struct) so each
comment ends with a period (e.g., "Line number in tape file (1-indexed)." "The
command text." "True if this is a comment line (starts with #).").
| // FilterExecutable returns only commands that should be executed (non-comments). | ||
| func FilterExecutable(commands []Command) []Command { | ||
| var executable []Command | ||
| for _, cmd := range commands { | ||
| if !cmd.IsComment { | ||
| executable = append(executable, cmd) | ||
| } | ||
| } | ||
| return executable |
There was a problem hiding this comment.
Add perf.Track to FilterExecutable.
This is a public function doing non-trivial work.
Proposed fix
func FilterExecutable(commands []Command) []Command {
+ defer perf.Track(nil, "tape.FilterExecutable")()
+
var executable []Command
for _, cmd := range commands {
if !cmd.IsComment {
executable = append(executable, cmd)
}
}
return executable
}As per coding guidelines.
🤖 Prompt for AI Agents
In `@tools/director/internal/tape/parser.go` around lines 126 - 134,
FilterExecutable currently performs non-trivial work but doesn't record
performance metrics; add a perf.Track invocation at the start of the function to
measure its execution time. Specifically, in FilterExecutable insert a call to
perf.Track (using the project's perf API pattern) with a descriptive name like
"FilterExecutable" and defer the appropriate stop/record call so the timing is
captured for the whole function; ensure the perf package is imported if missing
and that the existing return value (executable []Command) is unchanged.
Resolve merge conflicts: - Go API changes: Updated RunInstall signature (single tool + new params), added RunInstallBatch for batch operations - Component extraction: Updated field names (type, component_type) and added vars/settings extraction - Website features: Kept both demoId and experimental properties in roadmap/milestones - Navigation: Added both Demos and Examples links in docusaurus config - Styling: Kept both demo page styles and YAML syntax highlighting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…support - Add tape files for 19 example projects (demo-stacks, demo-vendoring, etc.) - Rename `prep` to `setup` in scene config for clarity - Add `cleanup` field for teardown commands (e.g., localstack down) - Add requirements check to skip scenes with missing dependencies - Create DemosAndExamples component combining Demos and Examples tabs - Extract IndexPageContent for embedding without Layout wrapper - Add demoId support to FileBrowser for video integration - Fix demo-schemas example with proper stacks config - Add workflow file for demo-workflows example Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
# Conflicts: # .gitignore # cmd/list/aliases.go # cmd/list/aliases_test.go # cmd/terraform/output.go # cmd/toolchain/install.go # docs/prd/list-pager-integration.md # errors/errors.go # internal/exec/describe_stacks.go # pkg/auth/factory/factory_test.go # pkg/list/extract/components_test.go # pkg/list/extract/stacks.go # pkg/list/format/table.go # pkg/terraform/output/format.go # pkg/terraform/output/output.go # pkg/terraform/output/output_test.go # pkg/ui/formatter.go # pkg/ui/markdown/custom_renderer.go # pkg/ui/markdown/custom_renderer_test.go # pkg/ui/markdown/extensions/admonition.go # pkg/ui/markdown/extensions/badge.go # pkg/ui/markdown/extensions/extensions_test.go # pkg/ui/markdown/extensions/highlight.go # pkg/ui/markdown/extensions/linkify.go # pkg/utils/highlight_utils.go # website/docs/cli/commands/describe/describe-affected.mdx # website/src/components/Roadmap/FeaturedDrawer.tsx # website/src/components/Roadmap/MilestoneList.tsx # website/src/components/Video/DemoVideo.tsx # website/src/data/roadmap.js # website/src/theme/Navbar/Content/styles.module.css # website/src/theme/Navbar/Content/usePriorityNavbar.ts
osterman
force-pushed
the
osterman/vhs-demo-videos
branch
from
f0b7f0e to
8fe6d39
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/list/extract/components.go (1)
288-308: MissingcomputeComponentStatuscall for unique components.
enrichUniqueComponentMetadatadoesn't callcomputeComponentStatus, but the default columns incmd/list/components.gorender{{ .status }}. This will produce empty status indicators for unique components.Suggested fix
func enrichUniqueComponentMetadata(comp map[string]any, componentData any) { compMap, ok := componentData.(map[string]any) if !ok { setDefaultMetadataFields(comp) + computeComponentStatus(comp) return } metadata, hasMetadata := compMap[fieldMetadata].(map[string]any) if hasMetadata { comp[fieldMetadata] = metadata extractMetadataFields(comp, metadata) } else { setDefaultMetadataFields(comp) } + // Compute status indicator after enabled/locked are set. + computeComponentStatus(comp) + // Extract component_folder for column templates. if folder, ok := compMap[fieldComponentFolder].(string); ok { comp[fieldComponentFolder] = folder } }
🤖 Fix all issues with AI agents
In `@cmd/list/components.go`:
- Around line 393-399: The type filter is currently applying to the wrong column
("type") but metadata real/abstract lives under "component_type"; update the
filter logic that builds filters (where opts.Type/opts.Abstract are processed
and filter.NewColumnFilter is called) to use "component_type" instead of "type",
and ensure the default branch that currently appends
filter.NewColumnFilter("type", "real") is changed to append
filter.NewColumnFilter("component_type", "real") so buildBaseComponent's "type"
(terraform/helmfile/packer) is not used for the real/abstract filter.
In `@examples/demo-workflows/stacks/workflows/deploy.yaml`:
- Around line 5-9: The plan job's description ("Plan all components in a stack")
is inaccurate because the steps only run the step named "Plan myapp" which
executes "terraform plan myapp -s {{ .Stack }}"; either update the
plan.description to something specific like "Plan myapp in a stack" or add
additional steps mirroring "Plan myapp" (new step names and commands e.g., "Plan
<component>" running "terraform plan <component> -s {{ .Stack }}") so the
description matches the actual behavior.
- Around line 11-15: The description for the apply step is inaccurate: the
top-level key "apply" and its "description" claim to "Apply all components in a
stack" but the single step "name: Apply myapp" with command "terraform apply
myapp -s {{ .Stack }}" only applies the myapp component; either update the
description to say "Apply myapp component" or change the step(s) to
iterate/apply all components (replace the single myapp-targeted step with a loop
or multiple steps that run terraform apply for each component). Target the
"apply" block, the "description" string, and the step named "Apply myapp" when
making the change.
In `@Makefile`:
- Around line 63-71: The Makefile's director target uses the non-expanding glob
tools/director/**/*.go and assumes build/ exists, causing missed rebuilds and
failures on fresh checkouts; update the build/director rule (target:
build/director, dependency reference: tools/director/**/*.go) to: 1) ensure
build/ is created before building (mkdir -p build) and 2) replace the glob
dependency with a reliable list of source files generated at make-time (e.g.,
use shell find or make's wildcard to collect tools/director/*.go recursively) so
file changes trigger rebuilds and the output path always exists.
🧹 Nitpick comments (7)
demos/scenes/examples/demo-localstack.tape (2)
31-38: Consider making the LocalStack connection more explicit in the demo.The title promises a "LocalStack AWS emulator" demo, but the shown command
atmos describe component demo -s devdoesn't visually convey anything LocalStack-specific to viewers. If the "dev" stack is configured to use LocalStack under the hood, consider adding a brief narration comment showing that configuration or mentioning why this demonstrates LocalStack integration.💡 Optional: Add context about the LocalStack connection
Type "# Describe component against LocalStack" Enter Sleep 500ms +Type "# The 'dev' stack uses LocalStack as the AWS backend" +Enter +Sleep 500ms + Type "atmos describe component demo -s dev" Sleep 1s Enter Sleep 3s
47-48: Missing trailing newline.Minor style nit: Add a trailing newline at the end of the file for POSIX compliance.
🔧 Add trailing newline
# Hold final frame Sleep 10s +demos/scenes/examples/devcontainer.tape (1)
3-5: Declare the screenshot as an output for asset collection.
If the demo pipeline only collects declared outputs,devcontainer.pngmay be missed. Consider adding it to the Output list for consistent artifact capture.Proposed change
Output devcontainer.gif Output devcontainer.mp4 Output devcontainer.svg +Output devcontainer.pngAlso applies to: 40-42
pkg/list/extract/components.go (1)
112-117: Variable shadows importedpathpackage.The variable
pathon line 114 shadows thepathpackage import. Consider renaming tocomponentPathorpathValfor clarity.Suggested fix
// Extract component_path from component_info if available. if componentInfo, ok := compMap["component_info"].(map[string]any); ok { - if path, ok := componentInfo["component_path"].(string); ok { - comp["component_path"] = path + if pathVal, ok := componentInfo["component_path"].(string); ok { + comp["component_path"] = pathVal } }cmd/list/components.go (3)
284-287: Bracketed queries pass without validation.A query like
[.component]would extract scalars but bypass the scalar extraction check. It would fail later infilterComponentsWithQuerywith a type error, but an earlier validation would give users a better error message.Consider validating bracketed queries too, or documenting that the type check in
filterComponentsWithQueryserves as the fallback.
362-368: Non-map items silently dropped.When iterating
[]any, items that aren'tmap[string]anyare silently skipped. This could mask issues where a query unexpectedly returns mixed types. Consider logging a warning when items are skipped.Suggested enhancement
filtered := make([]map[string]any, 0, len(v)) for _, item := range v { if m, ok := item.(map[string]any); ok { filtered = append(filtered, m) + } else { + log.Debug("Query returned non-map item, skipping", "type", fmt.Sprintf("%T", item)) } }
440-446: Column "Type" shows component kind, but--typeflag filters by abstract/real.The default "Type" column displays
terraform/helmfile/packer, while the--typeflag is documented to filter byreal/abstract. This could confuse users. Consider:
- Renaming the column to "Kind"
- Adding a separate column for abstract/real if that distinction matters in the UI
| // Type filter (authoritative when provided, targets type field: real/abstract). | ||
| if opts.Type != "" && opts.Type != "all" { | ||
| filters = append(filters, filter.NewColumnFilter("component_type", opts.Type)) | ||
| filters = append(filters, filter.NewColumnFilter("type", opts.Type)) | ||
| } else if opts.Type == "" && !opts.Abstract { | ||
| // Only apply default abstract filter when Type is not set. | ||
| filters = append(filters, filter.NewColumnFilter("component_type", "real")) | ||
| filters = append(filters, filter.NewColumnFilter("type", "real")) | ||
| } |
There was a problem hiding this comment.
Type filter targets wrong field.
The filter checks type field expecting values like real/abstract, but buildBaseComponent sets type to the component kind (terraform, helmfile, packer). The metadata type (real/abstract) is stored in component_type.
This means --type=real will filter on type=terraform/helmfile/packer instead of component_type=real/abstract.
Suggested fix
// Type filter (authoritative when provided, targets type field: real/abstract).
if opts.Type != "" && opts.Type != "all" {
- filters = append(filters, filter.NewColumnFilter("type", opts.Type))
+ filters = append(filters, filter.NewColumnFilter("component_type", opts.Type))
} else if opts.Type == "" && !opts.Abstract {
// Only apply default abstract filter when Type is not set.
- filters = append(filters, filter.NewColumnFilter("type", "real"))
+ filters = append(filters, filter.NewColumnFilter("component_type", "real"))
}#!/bin/bash
# Verify the field naming in extract/components.go
echo "=== Checking 'type' field assignment in buildBaseComponent ==="
rg -n '"type"' pkg/list/extract/components.go
echo ""
echo "=== Checking 'component_type' field assignment ==="
rg -n 'component_type' pkg/list/extract/components.go🤖 Prompt for AI Agents
In `@cmd/list/components.go` around lines 393 - 399, The type filter is currently
applying to the wrong column ("type") but metadata real/abstract lives under
"component_type"; update the filter logic that builds filters (where
opts.Type/opts.Abstract are processed and filter.NewColumnFilter is called) to
use "component_type" instead of "type", and ensure the default branch that
currently appends filter.NewColumnFilter("type", "real") is changed to append
filter.NewColumnFilter("component_type", "real") so buildBaseComponent's "type"
(terraform/helmfile/packer) is not used for the real/abstract filter.
| plan: | ||
| description: Plan all components in a stack | ||
| steps: | ||
| - name: Plan myapp | ||
| command: terraform plan myapp -s {{ .Stack }} |
There was a problem hiding this comment.
Description doesn't match implementation.
The description says "Plan all components in a stack" but only plans myapp. Either update the description to be specific, or add steps for additional components if "all" is intended.
🤖 Prompt for AI Agents
In `@examples/demo-workflows/stacks/workflows/deploy.yaml` around lines 5 - 9, The
plan job's description ("Plan all components in a stack") is inaccurate because
the steps only run the step named "Plan myapp" which executes "terraform plan
myapp -s {{ .Stack }}"; either update the plan.description to something specific
like "Plan myapp in a stack" or add additional steps mirroring "Plan myapp" (new
step names and commands e.g., "Plan <component>" running "terraform plan
<component> -s {{ .Stack }}") so the description matches the actual behavior.
| apply: | ||
| description: Apply all components in a stack | ||
| steps: | ||
| - name: Apply myapp | ||
| command: terraform apply myapp -s {{ .Stack }} |
There was a problem hiding this comment.
Same mismatch here.
Description says "Apply all components" but only handles myapp. Keep descriptions accurate.
🤖 Prompt for AI Agents
In `@examples/demo-workflows/stacks/workflows/deploy.yaml` around lines 11 - 15,
The description for the apply step is inaccurate: the top-level key "apply" and
its "description" claim to "Apply all components in a stack" but the single step
"name: Apply myapp" with command "terraform apply myapp -s {{ .Stack }}" only
applies the myapp component; either update the description to say "Apply myapp
component" or change the step(s) to iterate/apply all components (replace the
single myapp-targeted step with a loop or multiple steps that run terraform
apply for each component). Target the "apply" block, the "description" string,
and the step named "Apply myapp" when making the change.
| # Build director tool for VHS demo generation. | ||
| .PHONY: director | ||
| director: build/director | ||
|
|
||
| build/director: tools/director/**/*.go | ||
| @echo "Building director..." | ||
| @cd tools/director && go build -o ../../build/director . | ||
| @chmod +x build/director | ||
| @test -x build/director || (echo "Error: Failed to make director executable" && exit 1) |
There was a problem hiding this comment.
Ensure director rebuilds reliably and build/ exists.
tools/director/**/*.go won’t expand under GNU Make, so changes might not trigger rebuilds. Also, a fresh checkout can fail if build/ doesn’t exist.
🔧 Suggested fix
-# Build director tool for VHS demo generation.
-.PHONY: director
-director: build/director
-
-build/director: tools/director/**/*.go
- `@echo` "Building director..."
- `@cd` tools/director && go build -o ../../build/director .
- `@chmod` +x build/director
+DIRECTOR_SOURCES := $(shell find tools/director -type f -name '*.go')
+
+# Build director tool for VHS demo generation.
+.PHONY: director
+director: build/director
+
+build/director: $(DIRECTOR_SOURCES)
+ `@mkdir` -p build
+ `@echo` "Building director..."
+ `@cd` tools/director && go build -o ../../build/director .
+ `@chmod` +x build/director
`@test` -x build/director || (echo "Error: Failed to make director executable" && exit 1)
`@echo` "Director built successfully: ./build/director"📝 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.
| # Build director tool for VHS demo generation. | |
| .PHONY: director | |
| director: build/director | |
| build/director: tools/director/**/*.go | |
| @echo "Building director..." | |
| @cd tools/director && go build -o ../../build/director . | |
| @chmod +x build/director | |
| @test -x build/director || (echo "Error: Failed to make director executable" && exit 1) | |
| DIRECTOR_SOURCES := $(shell find tools/director -type f -name '*.go') | |
| # Build director tool for VHS demo generation. | |
| .PHONY: director | |
| director: build/director | |
| build/director: $(DIRECTOR_SOURCES) | |
| `@mkdir` -p build | |
| `@echo` "Building director..." | |
| `@cd` tools/director && go build -o ../../build/director . | |
| `@chmod` +x build/director | |
| `@test` -x build/director || (echo "Error: Failed to make director executable" && exit 1) |
🤖 Prompt for AI Agents
In `@Makefile` around lines 63 - 71, The Makefile's director target uses the
non-expanding glob tools/director/**/*.go and assumes build/ exists, causing
missed rebuilds and failures on fresh checkouts; update the build/director rule
(target: build/director, dependency reference: tools/director/**/*.go) to: 1)
ensure build/ is created before building (mkdir -p build) and 2) replace the
glob dependency with a reliable list of source files generated at make-time
(e.g., use shell find or make's wildcard to collect tools/director/*.go
recursively) so file changes trigger rebuilds and the output path always exists.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
2 participants
what
why
atmoscommand) is a key feature that needs proper demo coveragereferences
Summary by CodeRabbit
New Features
--queryflag toatmos list componentsfor filtering components using query expressions/demospage with interactive demo gallery featuring curated video demonstrationsDocumentation
✏️ Tip: You can customize this high-level summary in your review settings.