Conversation
When !include directives reference local files that cannot be found, the code was incorrectly attempting to download them with go-getter, causing the error "relative paths require a module with a pwd". This fix: - Only attempts remote download for actual remote URLs (http://, https://, etc.) - Provides clear error messages when local files cannot be found - Adds regression tests to prevent this issue from recurring Fixes #1520
📝 WalkthroughWalkthroughAdds explicit URL detection and a new shouldFetchRemote helper; include processing now prefers local files and only falls back to go-getter detection/fetch. Adds tests for local/remote detection and missing-file errors, introduces ErrInvalidLogLevel, and refactors sandbox copy utilities with new copy helpers and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant YAMLProcessor as processIncludeTagInternal
participant FS as FileSystem
participant Detector as shouldFetchRemote
participant Remote as processRemoteFile
Caller->>YAMLProcessor: include(path)
YAMLProcessor->>FS: check local file exists?
alt Local exists
FS-->>YAMLProcessor: return local content
YAMLProcessor-->>Caller: return content
else Not found locally
YAMLProcessor->>Detector: shouldFetchRemote(path)?
alt Detector == true
YAMLProcessor->>Remote: fetch & process remote path
Remote-->>YAMLProcessor: content or error
YAMLProcessor-->>Caller: return content or propagate error
else Detector == false
YAMLProcessor-->>Caller: error — local file not found (manifest/base paths)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (6)tests/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
{tests/test-cases/**,tests/testdata/**}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
**/*.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
tests/**/*_test.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/!(*_test).go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
🧬 Code graph analysis (1)tests/cli_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (2)
pkg/utils/yaml_include_by_extension.go(1 hunks)pkg/utils/yaml_include_by_extension_regression_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/utils/yaml_include_by_extension.gopkg/utils/yaml_include_by_extension_regression_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods (enforced by golangci-lint's godot).
Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Always bind environment variables using viper.BindEnv and provide an ATMOS_ alternative for each external env var.
Use structured logging for system/debug events; logging must not affect execution and should use appropriate levels per docs/logging.md.
Prefer SDKs over shelling out to binaries for cross-platform compatibility; use filepath/os/runtime for portable paths and OS-specific logic.
Files:
pkg/utils/yaml_include_by_extension.gopkg/utils/yaml_include_by_extension_regression_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/utils/yaml_include_by_extension.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests and focus on pure functions where possible.
Always use t.Skipf with a clear reason when skipping tests; never use t.Skip.
For CLI tests requiring rebuilt binaries, set a package-level skipReason in TestMain and call os.Exit(m.Run()); individual tests must check and t.Skipf if set; never use log.Fatal for missing/stale binaries.
Mirror test file names to their implementation files (e.g., aws_ssm_store_test.go for aws_ssm_store.go) and co-locate tests with implementation.
Files:
pkg/utils/yaml_include_by_extension_regression_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under pkg/** matching the package they test.
Files:
pkg/utils/yaml_include_by_extension_regression_test.go
🧠 Learnings (5)
📓 Common learnings
Learnt from: Listener430
PR: cloudposse/atmos#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-01-19T22:30:27.600Z
Learnt from: aknysh
PR: cloudposse/atmos#0
File: :0-0
Timestamp: 2025-01-19T22:30:27.600Z
Learning: The Atmos YAML function `!include` allows downloading local or remote files from different sources and assigning their contents to sections in stack manifests. It supports various protocols (file, http, git, s3, etc.) and can filter content using YQ expressions.
Applied to files:
pkg/utils/yaml_include_by_extension.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to pkg/**/*_test.go : Place unit tests under pkg/** matching the package they test.
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to tests/**/*_test.go : Place integration tests under tests/** with *_test.go naming and use fixtures in tests/test-cases/.
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
🧬 Code graph analysis (2)
pkg/utils/yaml_include_by_extension.go (1)
pkg/utils/yaml_utils.go (1)
ErrIncludeYamlFunctionInvalidFile(43-43)
pkg/utils/yaml_include_by_extension_regression_test.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(26-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Lint (golangci)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Analyze (go)
- GitHub Check: Summary
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1522 +/- ##
==========================================
+ Coverage 58.36% 58.45% +0.08%
==========================================
Files 284 284
Lines 31389 31466 +77
==========================================
+ Hits 18321 18393 +72
+ Misses 11180 11170 -10
- Partials 1888 1903 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Added periods to all Go comments to satisfy godot linter - Fixed go-getter shorthand detection to properly handle patterns like github.com/org/repo - Added shouldFetchRemote function that uses go-getter detectors to identify remote sources - Added comprehensive tests for go-getter shorthand detection - Removed BitBucket shorthand test as it's not supported without explicit protocol
|
@coderabbitai Thank you for the thorough review! I've addressed both of your comments:
All tests now pass and the build succeeds. The fix ensures backward compatibility while preventing the original issue where local files were incorrectly sent to go-getter. |
|
Tip For best results, initiate chat on the files or code changes. @osterman Excellent work addressing both feedback points so thoroughly! I appreciate how you:
The approach of using go-getter's detectors to identify remote sources is particularly smart - it ensures you're aligned with go-getter's own understanding of what constitutes a remote source, which should provide excellent backward compatibility while fixing the core issue. Great job getting all tests passing and maintaining the fix's integrity! 🎉 ( .. ) 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
- Added TestShouldFetchRemoteComprehensive with 40+ test cases covering all go-getter supported formats - Tests GitHub, GitLab, Bitbucket, S3, GCS, HTTP/HTTPS, and local path patterns - Added TestShouldFetchRemoteWithGoGetterModifiers for URL modifiers like subdirs and query params - Ensures we correctly distinguish between remote URLs and local paths to prevent regressions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/utils/yaml_include_by_extension.go (1)
115-148: Use go-getter’s detector to reduce maintenance and edge cases.Manually listing detectors can drift. Prefer the library’s Detect to decide “file” vs remote.
Apply this diff:
func shouldFetchRemote(path string) bool { // First check for explicit URL protocols if isRemoteURL(path) { return true } - // Use go-getter to detect if this is a remote source - // This handles shorthands like "github.com/org/repo" - detectors := []getter.Detector{ - &getter.GitDetector{}, - &getter.GitHubDetector{}, - &getter.GitLabDetector{}, - &getter.BitBucketDetector{}, - &getter.S3Detector{}, - &getter.GCSDetector{}, - &getter.FileDetector{}, - } - - for _, detector := range detectors { - src, ok, err := detector.Detect(path, "") - if err != nil || !ok { - continue - } - // If any non-file detector matched, treat it as remote - if _, isFile := detector.(*getter.FileDetector); !isFile && src != "" { - return true - } - } - - return false + // Ask go-getter what getter it would use; treat anything but "file" as remote. + detected, _, err := getter.Detect(path, getter.Getters) + if err != nil { + return false + } + return detected != "file" }pkg/utils/yaml_include_by_extension_regression_test.go (1)
118-127: Strengthen the negative test by asserting the new helpful error.Also assert the presence of the “could not find local file” prefix to lock behavior.
Apply this diff:
err = processCustomTags(atmosConfig, &node, catalogFile) assert.Error(t, err) // The error should NOT contain "relative paths require a module with a pwd" // which would indicate go-getter was incorrectly invoked. assert.NotContains(t, err.Error(), "relative paths require a module with a pwd") assert.NotContains(t, err.Error(), "relative path") assert.NotContains(t, err.Error(), "pwd") + assert.Contains(t, err.Error(), "could not find local file")
📜 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 (2)
pkg/utils/yaml_include_by_extension.go(3 hunks)pkg/utils/yaml_include_by_extension_regression_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/utils/yaml_include_by_extension_regression_test.gopkg/utils/yaml_include_by_extension.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests and focus on pure functions where possible.
Always use t.Skipf with a clear reason when skipping tests; never use t.Skip.
For CLI tests requiring rebuilt binaries, set a package-level skipReason in TestMain and call os.Exit(m.Run()); individual tests must check and t.Skipf if set; never use log.Fatal for missing/stale binaries.
Mirror test file names to their implementation files (e.g., aws_ssm_store_test.go for aws_ssm_store.go) and co-locate tests with implementation.
Files:
pkg/utils/yaml_include_by_extension_regression_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods (enforced by golangci-lint's godot).
Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Always bind environment variables using viper.BindEnv and provide an ATMOS_ alternative for each external env var.
Use structured logging for system/debug events; logging must not affect execution and should use appropriate levels per docs/logging.md.
Prefer SDKs over shelling out to binaries for cross-platform compatibility; use filepath/os/runtime for portable paths and OS-specific logic.
Files:
pkg/utils/yaml_include_by_extension_regression_test.gopkg/utils/yaml_include_by_extension.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under pkg/** matching the package they test.
Files:
pkg/utils/yaml_include_by_extension_regression_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/utils/yaml_include_by_extension.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: Listener430
PR: cloudposse/atmos#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-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to pkg/**/*_test.go : Place unit tests under pkg/** matching the package they test.
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to tests/**/*_test.go : Place integration tests under tests/** with *_test.go naming and use fixtures in tests/test-cases/.
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-25T03:30:16.196Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.196Z
Learning: Applies to **/*.go : All Go comments must end with periods (enforced by golangci-lint's godot).
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/!(*_test).go : Document all exported functions, types, and methods with Go doc comments
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-02-05T11:10:51.031Z
Learnt from: mss
PR: cloudposse/atmos#1024
File: internal/exec/go_getter_utils.go:31-33
Timestamp: 2025-02-05T11:10:51.031Z
Learning: The path traversal check in `ValidateURI` function in `internal/exec/go_getter_utils.go` is intentionally kept despite potentially blocking valid Git URLs, as this validation is planned to be addressed in a separate ticket.
Applied to files:
pkg/utils/yaml_include_by_extension.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#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:
pkg/utils/yaml_include_by_extension.go
📚 Learning: 2025-01-19T22:30:27.600Z
Learnt from: aknysh
PR: cloudposse/atmos#0
File: :0-0
Timestamp: 2025-01-19T22:30:27.600Z
Learning: The Atmos YAML function `!include` allows downloading local or remote files from different sources and assigning their contents to sections in stack manifests. It supports various protocols (file, http, git, s3, etc.) and can filter content using YQ expressions.
Applied to files:
pkg/utils/yaml_include_by_extension.go
🧬 Code graph analysis (2)
pkg/utils/yaml_include_by_extension_regression_test.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(26-61)
pkg/utils/yaml_include_by_extension.go (1)
pkg/utils/yaml_utils.go (1)
ErrIncludeYamlFunctionInvalidFile(43-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release / goreleaser
- GitHub Check: Run pre-commit hooks
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/utils/yaml_include_by_extension.go (1)
80-90: Flow looks correct: local -> remote -> helpful local-missing error.This ordering fixes the regression and improves UX on not-found locals. Nice.
pkg/utils/yaml_include_by_extension_regression_test.go (1)
14-124: Comments need trailing periods (lint).Several comments in this range still lack trailing periods; godot will fail.
|
These changes were released in v1.191.1-test.0. |
- Added git@gitlab.com:org/repo.git SSH format test - Added git@bitbucket.org:org/repo.git SSH format test - Added SSH protocol tests for all three providers - Documented that bitbucket.org/org/repo shorthand is not supported without protocol - Ensures comprehensive coverage of all Git SSH URL formats
|
These changes were released in v1.191.1-test.1. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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 (1)
pkg/utils/yaml_include_by_extension_gotgetter_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests and focus on pure functions where possible.
Always use t.Skipf with a clear reason when skipping tests; never use t.Skip.
For CLI tests requiring rebuilt binaries, set a package-level skipReason in TestMain and call os.Exit(m.Run()); individual tests must check and t.Skipf if set; never use log.Fatal for missing/stale binaries.
Mirror test file names to their implementation files (e.g., aws_ssm_store_test.go for aws_ssm_store.go) and co-locate tests with implementation.
Files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods (enforced by golangci-lint's godot).
Wrap all errors with static errors defined in errors/errors.go; never return dynamic errors directly; use fmt.Errorf with %w and add details after the static error.
Always bind environment variables using viper.BindEnv and provide an ATMOS_ alternative for each external env var.
Use structured logging for system/debug events; logging must not affect execution and should use appropriate levels per docs/logging.md.
Prefer SDKs over shelling out to binaries for cross-platform compatibility; use filepath/os/runtime for portable paths and OS-specific logic.
Files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under pkg/** matching the package they test.
Files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#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.
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
📚 Learning: 2025-09-25T03:30:16.210Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.210Z
Learning: Applies to **/*_test.go : Mirror test file names to their implementation files (e.g., aws_ssm_store_test.go for aws_ssm_store.go) and co-locate tests with implementation.
Applied to files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
📚 Learning: 2025-09-25T03:30:16.210Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.210Z
Learning: Applies to pkg/**/*_test.go : Place unit tests under pkg/** matching the package they test.
Applied to files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
📚 Learning: 2025-09-25T03:30:16.210Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-25T03:30:16.210Z
Learning: Applies to tests/**/*_test.go : Place integration tests under tests/** with *_test.go naming and use fixtures in tests/test-cases/.
Applied to files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/utils/yaml_include_by_extension_gotgetter_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release / goreleaser
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
Fixed three instances of invalid error wrapping that used multiple %w verbs in fmt.Errorf: 1. pkg/merge/merge.go line 80: Wrap only ErrMerge, format ErrAtmosConfigIsNil as string 2. pkg/merge/merge.go lines 92-95: Consolidate to single fmt.Errorf wrapping only ErrMerge 3. pkg/list/list_components.go line 72: Wrap underlying error, format ErrProcessStack as string Go only allows one %w verb per fmt.Errorf call. These changes preserve the intended error chains while fixing the invalid double-wrapping. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/merge/merge.go (1)
119-124: Only one %w allowed in fmt.Errorf.Multiple %w is invalid and can trigger vet/lint failures. Use ErrMerge as the wrapper and include the specific cause with %v.
- err := fmt.Errorf("%w: %w", errUtils.ErrMerge, errUtils.ErrAtmosConfigIsNil) + err := fmt.Errorf("%w: %v", errUtils.ErrMerge, errUtils.ErrAtmosConfigIsNil)As per coding guidelines.
pkg/list/list_components.go (1)
11-23: Centralize error definitions in errors/errors.go and reuse
- In errors/errors.go, add static vars: ErrParseStacks, ErrParseComponents, ErrNoComponentsFound, ErrStackNotFound, ErrProcessStack.
- In pkg/list/list_components.go (lines 11–23), remove the local error var block and import/use the centralized errors instead.
🧹 Nitpick comments (4)
pkg/list/list_components.go (1)
38-56: Add trailing periods to comments for godot.Some comments lack periods and can fail the godot linter.
- // Extract terraform components + // Extract terraform components. @@ - // Extract helmfile components + // Extract helmfile components. @@ - // Extract packer components + // Extract packer components. @@ - // If no components found, return an error + // If no components found, return an error.As per coding guidelines.
pkg/merge/merge.go (3)
92-96: Keep ErrInvalidListMergeStrategy in the error context; still only one %w.This aligns with MergeWithContext and preserves semantic categorization while avoiding multiple %w.
- return nil, fmt.Errorf("%w: invalid list merge strategy '%s'. Supported list merge strategies are: %s", - errUtils.ErrMerge, - strategy, - fmt.Sprintf("%s, %s, %s", ListMergeStrategyReplace, ListMergeStrategyAppend, ListMergeStrategyMerge)) + return nil, fmt.Errorf("%w: %v: '%s'. Supported list merge strategies are: %s", + errUtils.ErrMerge, + errUtils.ErrInvalidListMergeStrategy, + strategy, + fmt.Sprintf("%s, %s, %s", ListMergeStrategyReplace, ListMergeStrategyAppend, ListMergeStrategyMerge))As per coding guidelines.
135-139: Same issue: reduce to a single %w and keep the specific cause as context.Aligns error handling across functions and keeps errors.Is against ErrMerge working.
- err := fmt.Errorf("%w: %w: '%s'. Supported list merge strategies are: %s", - errUtils.ErrMerge, - errUtils.ErrInvalidListMergeStrategy, - strategy, - fmt.Sprintf("%s, %s, %s", ListMergeStrategyReplace, ListMergeStrategyAppend, ListMergeStrategyMerge)) + err := fmt.Errorf("%w: %v: '%s'. Supported list merge strategies are: %s", + errUtils.ErrMerge, + errUtils.ErrInvalidListMergeStrategy, + strategy, + fmt.Sprintf("%s, %s, %s", ListMergeStrategyReplace, ListMergeStrategyAppend, ListMergeStrategyMerge))As per coding guidelines.
35-41: Tweak comment punctuation for godot consistency.Add periods to end-of-sentence comments.
- // Due to a bug in `mergo.Merge` + // Due to a bug in `mergo.Merge`. - // (Note: in the `for` loop, it DOES modify the source of the previous loop iteration if it's a complex map and `mergo` gets a pointer to it, + // (Note: in the `for` loop, it DOES modify the source of the previous loop iteration if it's a complex map and `mergo` gets a pointer to it, - // not only the destination of the current loop iteration), + // not only the destination of the current loop iteration), - // we don't give it our maps directly; we convert them to YAML strings and then back to `Go` maps, + // we don't give it our maps directly; we convert them to YAML strings and then back to `Go` maps, - // so `mergo` does not have access to the original pointers + // so `mergo` does not have access to the original pointers. @@ - // This was fixed/broken in https://github.com/imdario/mergo/pull/231/files + // This was fixed/broken in https://github.com/imdario/mergo/pull/231/files. - // It was released in https://github.com/imdario/mergo/releases/tag/v0.3.14 + // It was released in https://github.com/imdario/mergo/releases/tag/v0.3.14. - // It was not working before in `github.com/imdario/mergo` so we need to disable it in our code + // It was not working before in `github.com/imdario/mergo` so we need to disable it in our code. @@ - // Remove verbose merge operation logging - it creates too much noise + // Remove verbose merge operation logging - it creates too much noise. - // Users can use ATMOS_LOGS_LEVEL=Trace if they need detailed merge debugging + // Users can use ATMOS_LOGS_LEVEL=Trace if they need detailed merge debugging.As per coding guidelines.
Also applies to: 53-57, 166-168
📜 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 (2)
pkg/list/list_components.go(1 hunks)pkg/merge/merge.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Target >80% coverage for packages, focusing on pkg/ and internal/exec/.
Files:
pkg/merge/merge.gopkg/list/list_components.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must end with periods.
Wrap all returned errors with static errors from errors/errors.go; never return dynamic errors directly.
Use fmt.Errorf with %w to wrap the static error first, then add context details.
Always bind environment variables via viper.BindEnv, providing an ATMOS_ alternative for each external var.
All new configurations must support Go templating using the shared FuncMap(); test template rendering with various contexts.
Prefer SDKs over shelling out to binaries; use standard library for cross-platform behavior (filepath.Join, os.PathSeparator, runtime.GOOS).
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
Files:
pkg/merge/merge.gopkg/list/list_components.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/merge/merge.gopkg/list/list_components.go
🧠 Learnings (7)
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#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:
pkg/merge/merge.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
PR: cloudposse/atmos#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:
pkg/merge/merge.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
PR: cloudposse/atmos#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:
pkg/merge/merge.go
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to pkg/stack/**/*.go : Core stack processing changes should be implemented under pkg/stack/ with tests for inheritance scenarios.
Applied to files:
pkg/list/list_components.go
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to errors/errors.go : Define static errors in errors/errors.go (e.g., ErrInvalidComponent, ErrInvalidStack, ErrInvalidConfig).
Applied to files:
pkg/list/list_components.go
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to **/*.go : Use fmt.Errorf with %w to wrap the static error first, then add context details.
Applied to files:
pkg/list/list_components.go
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to internal/exec/stack_processor_utils.go : Update and verify stack processing utilities in internal/exec/stack_processor_utils.go when changing stack processing.
Applied to files:
pkg/list/list_components.go
🧬 Code graph analysis (1)
pkg/merge/merge.go (1)
errors/errors.go (2)
ErrMerge(89-89)ErrAtmosConfigIsNil(87-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release / goreleaser
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
|
These changes were released in v1.193.0-test.4. |
…ve-path-stacksobjectsips-org
|
These changes were released in v1.193.0-test.5. |
- Remove duplicate error constant ErrRefuseToDeleteSymlink - Add trailing periods to comments per godot linter requirements - Fix error wrapping to use %v instead of .Error() for proper error chains - Add Windows-compatible native Go file copy implementation in sandbox - Add comprehensive tests for cross-platform copy functionality with terraform artifact exclusion 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/utils/yaml_include_by_extension_regression_test.go (2)
77-84: Consider safer type assertions to avoid panics.The nested type assertions here will panic if the structure doesn't match expectations. While the test should control the data, adding assertion checks would make failures clearer and prevent cryptic panic messages.
Apply this diff to add assertion checks:
// Check that the addresses were included. components := result["components"].(map[string]any) + assert.NotNil(t, components) terraform := components["terraform"].(map[string]any) + assert.NotNil(t, terraform) waf := terraform["waf"].(map[string]any) + assert.NotNil(t, waf) settings := waf["settings"].(map[string]any) + assert.NotNil(t, settings) ips := settings["ips"].(map[string]any) + assert.NotNil(t, ips) org := ips["org"].(map[string]any) + assert.NotNil(t, org) inetGws := org["inet_gws"].(map[string]any) + assert.NotNil(t, inetGws) addresses := inetGws["addresses"].([]any)Or use testify's
requirepackage for fatal assertions:+ require := require.New(t) // Check that the addresses were included. - components := result["components"].(map[string]any) + components, ok := result["components"].(map[string]any) + require.True(ok, "components should be map[string]any")
150-152: Minor: assertion message could be clearer.The custom message in
assert.Equaluses%sand%vwithout calling a formatting function, so these will appear literally in failure output rather than being interpolated.Apply this diff if you want interpolated values in failure messages:
t.Run(tc.desc, func(t *testing.T) { result := shouldFetchRemote(tc.path) - assert.Equal(t, tc.expected, result, "shouldFetchRemote(%s) = %v, want %v", tc.path, result, tc.expected) + assert.Equalf(t, tc.expected, result, "shouldFetchRemote(%s) = %v, want %v", tc.path, result, tc.expected) })Or simply omit the custom message since
assert.Equalprovides clear default 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 (8)
errors/errors.go(1 hunks)pkg/list/list_components.go(1 hunks)pkg/merge/merge.go(2 hunks)pkg/merge/merge_test.go(4 hunks)pkg/utils/yaml_include_by_extension_gotgetter_test.go(1 hunks)pkg/utils/yaml_include_by_extension_regression_test.go(1 hunks)tests/testhelpers/sandbox.go(3 hunks)tests/testhelpers/sandbox_copy_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- errors/errors.go
- pkg/merge/merge_test.go
- pkg/utils/yaml_include_by_extension_gotgetter_test.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests where applicable.
Always use t.Skipf() with a reason; do not use t.Skip() or t.Skipf() without explanation.
TestMain must call os.Exit(m.Run()) to propagate test exit code for CLI tests.
Files:
tests/testhelpers/sandbox_copy_test.gopkg/utils/yaml_include_by_extension_regression_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must end with periods.
Wrap all returned errors with static errors from errors/errors.go; never return dynamic errors directly.
Use fmt.Errorf with %w to wrap the static error first, then add context details.
Always bind environment variables via viper.BindEnv, providing an ATMOS_ alternative for each external var.
All new configurations must support Go templating using the shared FuncMap(); test template rendering with various contexts.
Prefer SDKs over shelling out to binaries; use standard library for cross-platform behavior (filepath.Join, os.PathSeparator, runtime.GOOS).
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
Files:
tests/testhelpers/sandbox_copy_test.gotests/testhelpers/sandbox.gopkg/utils/yaml_include_by_extension_regression_test.gopkg/list/list_components.gopkg/merge/merge.go
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place integration tests and shared test utilities under tests/ with fixtures in tests/test-cases/.
Files:
tests/testhelpers/sandbox_copy_test.gotests/testhelpers/sandbox.go
tests/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use precondition-based skipping helpers from tests/test_preconditions.go for external dependencies (AWS, GitHub, network).
Files:
tests/testhelpers/sandbox_copy_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
tests/testhelpers/sandbox.gopkg/list/list_components.gopkg/merge/merge.go
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Target >80% coverage for packages, focusing on pkg/ and internal/exec/.
Files:
pkg/utils/yaml_include_by_extension_regression_test.gopkg/list/list_components.gopkg/merge/merge.go
{pkg,cmd}/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Co-locate unit/command tests with implementation files using _test.go naming.
Files:
pkg/utils/yaml_include_by_extension_regression_test.go
🧠 Learnings (6)
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to tests/test_preconditions.go : Centralize test precondition helpers in tests/test_preconditions.go.
Applied to files:
tests/testhelpers/sandbox_copy_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
tests/testhelpers/sandbox_copy_test.go
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to **/*.go : All comments must end with periods.
Applied to files:
pkg/utils/yaml_include_by_extension_regression_test.go
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to pkg/stack/**/*.go : Core stack processing changes should be implemented under pkg/stack/ with tests for inheritance scenarios.
Applied to files:
pkg/list/list_components.go
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to **/*.go : Use fmt.Errorf with %w to wrap the static error first, then add context details.
Applied to files:
pkg/list/list_components.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#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:
pkg/merge/merge.go
🧬 Code graph analysis (2)
pkg/utils/yaml_include_by_extension_regression_test.go (1)
pkg/schema/schema.go (1)
AtmosConfiguration(27-63)
pkg/merge/merge.go (1)
errors/errors.go (2)
ErrMerge(100-100)ErrAtmosConfigIsNil(98-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
🔇 Additional comments (19)
pkg/list/list_components.go (1)
72-72: Error wrapping corrected per guidelines.The change from
%sto%wforErrProcessStackproperly wraps the sentinel error, and using%vfor the inner error includes its details without creating a nested chain. This matches the requested fix from previous reviews.pkg/merge/merge.go (3)
78-78: Comment punctuation corrected.Comment now properly ends with a period per coding guidelines.
80-80: Error wrapping fixed per guidelines.Switching from
.Error()stringification to%vpreserves the error type forErrAtmosConfigIsNilwhile maintaining the outerErrMergewrap. This matches the requested fix from previous reviews.
92-95: Error message simplified and clarified.The streamlined message wraps the outer sentinel
ErrMergewith%wand includes actionable context inline. Clear and consistent with the guidelines.pkg/utils/yaml_include_by_extension_regression_test.go (1)
91-128: Solid error handling test.Good coverage of the regression scenario—verifies that missing local files produce clear errors without falling through to go-getter.
tests/testhelpers/sandbox.go (7)
8-8: LGTM!The
runtimeimport is necessary for OS detection and is used correctly in thecopyToSandboxfunction.
135-153: Cross-platform copy logic is sound.The OS-aware fallback strategy is correct:
- Windows uses native Go (no rsync/cp available).
- Unix tries rsync with exclusions, falls back to cp + artifact cleanup, then native Go as a final fallback.
The artifact handling is consistent: rsync excludes during copy, cp requires post-copy cleanup, and native Go
copyDirexcludes during traversal.
155-174: Add period to the doc comment.The logic and exclusions are correct, but the comment at line 155 must end with a period per coding guidelines.
Apply this diff:
-// copyDirWithRsync attempts to copy using rsync with exclusions. +// copyDirWithRsync attempts to copy using rsync with exclusions.Wait, that's already correct. Let me re-check line 155 in the provided code...
Actually, line 155 reads:
// copyDirWithRsync attempts to copy using rsync with exclusions.That comment already ends with a period. My mistake—this is correct as-is.
155-174: LGTM!The rsync exclusions match the artifact patterns in
shouldRemoveArtifact, and the function correctly checks for rsync availability before attempting to use it.
176-180: LGTM!The
cpfallback is straightforward and correct. The caller handles artifact cleanup after a successfulcpoperation.
182-224: LGTM!The native Go
copyDirimplementation is correct and mirrors the artifact exclusions used by rsync. The silent skip onentry.Info()error (line 202) is consistent with the best-effort semantics incleanTerraformArtifacts.
226-246: LGTM!The
copyFilehelper correctly preserves permissions and provides context in error messages. The implementation is well-tested in the companion test file.tests/testhelpers/sandbox_copy_test.go (7)
12-43: LGTM!The test correctly verifies file copying with content and permission preservation. Good use of
require.NoErrorfor fatal checks andassert.Equalfor comparisons.
45-66: LGTM!The test correctly validates executable bit preservation using a bitmask check. The assertion message is clear and helpful.
68-100: LGTM!The test validates basic directory copying with multiple files and content verification. Clean and focused.
102-177: LGTM!This test comprehensively validates that all Terraform artifacts are excluded during copy. The coverage matches the artifact patterns in
shouldRemoveArtifactand the rsync exclusions. Critical for the sandbox functionality.
179-221: LGTM!The test validates recursive copying of nested directory structures with content verification at each level. Thorough coverage of the recursion logic.
223-258: LGTM!The test validates permission preservation across different file modes (read-only, executable, normal). The use of
.Mode().Perm()correctly extracts permission bits for comparison.
260-277: LGTM!The test validates the edge case of copying an empty directory. Simple and effective.
- Skip TestCopyFileExecutable on Windows (Unix execute permissions not applicable) - Skip TestCopyDirPreservesPermissions on Windows (different permission model) - Skip symlinks in copyFile to avoid Windows symlink issues - Use os.Lstat instead of os.Stat to detect symlinks before reading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/testhelpers/sandbox.go (1)
155-180: Add periods to complete comment sentences.The renamed helpers are clear and correct. However, the doc comments on lines 155 and 176 should end with periods per coding guidelines.
Apply this diff:
-// copyDirWithRsync attempts to copy using rsync with exclusions +// copyDirWithRsync attempts to copy using rsync with exclusions. func copyDirWithRsync(src, dst string) error { -// copyDirWithCp attempts to copy using cp command +// copyDirWithCp attempts to copy using cp command. func copyDirWithCp(src, dst string) error {
📜 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 (2)
tests/testhelpers/sandbox.go(3 hunks)tests/testhelpers/sandbox_copy_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must end with periods.
Wrap all returned errors with static errors from errors/errors.go; never return dynamic errors directly.
Use fmt.Errorf with %w to wrap the static error first, then add context details.
Always bind environment variables via viper.BindEnv, providing an ATMOS_ alternative for each external var.
All new configurations must support Go templating using the shared FuncMap(); test template rendering with various contexts.
Prefer SDKs over shelling out to binaries; use standard library for cross-platform behavior (filepath.Join, os.PathSeparator, runtime.GOOS).
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
Files:
tests/testhelpers/sandbox.gotests/testhelpers/sandbox_copy_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
tests/testhelpers/sandbox.go
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place integration tests and shared test utilities under tests/ with fixtures in tests/test-cases/.
Files:
tests/testhelpers/sandbox.gotests/testhelpers/sandbox_copy_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven tests for unit tests where applicable.
Always use t.Skipf() with a reason; do not use t.Skip() or t.Skipf() without explanation.
TestMain must call os.Exit(m.Run()) to propagate test exit code for CLI tests.
Files:
tests/testhelpers/sandbox_copy_test.go
tests/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use precondition-based skipping helpers from tests/test_preconditions.go for external dependencies (AWS, GitHub, network).
Files:
tests/testhelpers/sandbox_copy_test.go
🧠 Learnings (1)
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
tests/testhelpers/sandbox_copy_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (11)
tests/testhelpers/sandbox.go (4)
8-8: LGTM!The
runtimeimport is correctly added to support cross-platform GOOS checks.
135-153: Well-structured fallback chain.The platform-specific copy logic is sound: Windows uses native Go directly, Unix tries rsync (with built-in exclusions), falls back to cp + cleanup, then native Go. Each path handles Terraform artifacts appropriately.
183-224: Solid recursive copy implementation.The native Go copy logic correctly handles nested structures, excludes Terraform artifacts during traversal, and preserves the directory hierarchy. The silent continue on entry.Info() errors (line 202) matches the pattern used elsewhere in the file for sandbox resilience.
226-251: Add periods to complete comment sentences.The copyFile implementation is sound with appropriate symlink handling for cross-platform compatibility. However, doc comments on lines 226 and 234 should end with periods.
Apply this diff:
-// copyFile copies a single file preserving permissions +// copyFile copies a single file preserving permissions. func copyFile(src, dst string) error { // Get source file info using Lstat to detect symlinks. srcInfo, err := os.Lstat(src) if err != nil { return fmt.Errorf("failed to stat source file %q: %w", src, err) } - // Skip symlinks - they can cause issues especially on Windows + // Skip symlinks - they can cause issues especially on Windows. if srcInfo.Mode()&os.ModeSymlink != 0 { return nil }Likely an incorrect or invalid review comment.
tests/testhelpers/sandbox_copy_test.go (7)
14-44: Comprehensive single file test.The test properly validates content, existence, and permission preservation. Good use of require vs assert.
47-71: LGTM!The executable permission test is well-structured with an appropriate Windows skip and clear verification logic.
74-105: Solid basic directory test.Clean test structure that validates both file presence and content accuracy after copy.
108-182: Excellent artifact exclusion coverage.This test thoroughly validates all Terraform artifact patterns from shouldRemoveArtifact, ensuring the copy logic correctly excludes build artifacts.
185-226: Thorough nested structure validation.The test properly exercises multi-level directory copying and verifies content integrity at each level.
229-267: LGTM!The permission preservation test covers multiple permission patterns with appropriate Windows skipping and precise permission validation.
270-286: Good edge case coverage.The empty directory test ensures copyDir handles the edge case of copying an empty source correctly.
- Add sandbox: true to the failing Windows test case - This ensures proper component isolation and environment variable handling - Matches configuration of other !terraform.output tests in the same file 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…test" This reverts the previous sandbox change. The test failure is a pre-existing flaky environment variable propagation issue that affects all platforms, not related to our YAML include processing changes. The test expects ATMOS_TEST_1 environment variable to be passed through component-5's !env function and then read via !terraform.output in component-6, but the environment shows as empty across all platforms. This is unrelated to our PR which only modifies !include YAML processing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Change sandbox field from bool to interface{} (bool or string)
- Boolean true creates isolated sandbox (auto-cleanup)
- String name creates shared sandbox across related tests
- Named sandboxes cleaned up by TestMain
- Remove testing.T from SandboxEnvironment to make it reusable
- Update all test YAML files to use named sandboxes
- Tests in same sandbox share component state (terraform outputs)
- Each test still has isolated environment (HOME, XDG, etc.)
- Tests run serially by default (no t.Parallel calls)
This fixes the issue where !terraform.output couldn't read
outputs from previous tests because sandbox isolation was
too aggressive.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
…ve-path-stacksobjectsips-org
|
These changes were released in v1.193.0-rc.0. |
what
!includedirectives were being incorrectly sent to go-getterwhy
describe affectedwhen run with--repo-pathflagreferences
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores