fix: normalize legacy triple-slash vendor URIs for go-getter compatibility #1504
Conversation
The go-getter update from v1.7.8 to v1.7.9 (introduced in Atmos v1.189.0) broke the documented triple-slash pattern (///) for vendoring from Git repository roots. This pattern was widely used and documented in Atmos examples. Changes: - Add normalizeVendorURI() to convert /// patterns to go-getter v1.7.9 format - Convert "repo.git///?ref=v1.0" to "repo.git?ref=v1.0" (empty subdir) - Convert "repo.git///path?ref=v1.0" to "repo.git//path?ref=v1.0" (with subdir) - Add comprehensive test cases for the vendor pull issue Root Cause: go-getter v1.7.9 included security fixes for CVE-2025-8959 that changed subdirectory path handling, making the triple-slash pattern no longer functional for downloading Git repository contents. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds centralized vendor URI normalization (triple‑slash and implicit Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as Root CLI
participant Config as Config Loader
participant Vendor as Vendor Executor
participant Norm as URI Normalizer
participant Downloader as Downloader/Detector
participant FS as Filesystem
User->>CLI: atmos vendor pull [--base-path --config --config-path]
CLI->>Config: InitCliConfig(configAndStacksInfo with CLI overrides)
CLI->>Vendor: ExecuteVendorPullCommand(...)
loop per vendor source
Vendor->>Norm: normalizeVendorURI(uri)
Norm-->>Vendor: normalized URI
Vendor->>Downloader: Detect & fetch (go‑getter / git)
Downloader-->>Vendor: Downloaded package
Vendor->>FS: copyToTargetWithPatterns(...)
FS-->>Vendor: Files written
end
Vendor-->>User: Completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 2
🧹 Nitpick comments (4)
internal/exec/vendor_utils.go (1)
431-463: Harden triple-slash normalization (handle trailing, query, and fragment cases).Current logic misses cases like:
- repo.git/// (no query) → should drop subdir marker.
- repo.git///#frag or repo.git///?ref=... → should drop subdir marker.
- “///” appearing inside the query should not be rewritten.
Refactor as below.
-func normalizeVendorURI(uri string) string { - // Check if the URI contains the triple slash pattern - if strings.Contains(uri, "///") { - // Replace "///" with "//" first to handle any path after the triple slash - normalized := strings.Replace(uri, "///", "//", 1) - - // If the result has "//" followed immediately by "?" (empty subdir), remove the "//" - if strings.Contains(normalized, "//?") { - normalized = strings.Replace(normalized, "//?", "?", 1) - log.Debug("Normalized vendor URI: removed empty subdirectory indicator (///)", - "original", uri, "normalized", normalized) - } else { - log.Debug("Normalized vendor URI: converted triple slash to double slash", - "original", uri, "normalized", normalized) - } - - return normalized - } - - return uri -} +func normalizeVendorURI(uri string) string { + // Normalize only when "///" appears in the path portion (not inside query/fragment). + idx := strings.Index(uri, "///") + if idx < 0 { + return uri + } + // If "///" occurs after a '?' or '#', leave as-is. + qf := strings.IndexAny(uri, "?#") + if qf >= 0 && qf < idx { + return uri + } + + prefix := uri[:idx] + rest := uri[idx+3:] + + // Empty subdir or immediately followed by query/fragment => drop the subdir marker entirely. + if rest == "" || strings.HasPrefix(rest, "?") || strings.HasPrefix(rest, "#") { + normalized := prefix + rest + log.Debug("Normalized vendor URI: removed empty subdirectory indicator (///).", "original", uri, "normalized", normalized) + return normalized + } + + // Non-empty subdir => convert to the go-getter v1.7.9+ compatible double-slash form. + normalized := prefix + "//" + rest + log.Debug("Normalized vendor URI: converted triple-slash to double-slash.", "original", uri, "normalized", normalized) + return normalized +}Would you like me to add a small, table-driven unit test for normalizeVendorURI?
tests/fixtures/scenarios/vendor-pull-issue-dev-3639/vendor.yaml (1)
1-22: Good coverage for the root (///?) case.Consider adding a second source in tests to cover the subdir form (repo.git///path?ref=...), ensuring both normalization branches are exercised.
internal/exec/vendor_issue_dev3639_test.go (2)
26-63: Use t.Setenv and t.Cleanup to reduce boilerplate.Simplifies env save/restore and directory cleanup.
Example:
startDir := mustGetwd(t) t.Cleanup(func() { _ = os.Chdir(startDir) }) t.Setenv("ATMOS_LOGS_LEVEL", "Debug") // (Use t.Setenv for ATMOS_CLI_CONFIG_PATH and ATMOS_BASE_PATH if needed.)Also applies to: 140-177
92-119: Add a test for the subdirectory triple-slash form.Current tests cover repo.git///?ref=… only. Add a case for repo.git///some/path?ref=… to exercise the other normalization branch.
Also applies to: 208-216
📜 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 (6)
internal/exec/vendor_issue_dev3639_test.go(1 hunks)internal/exec/vendor_model.go(1 hunks)internal/exec/vendor_utils.go(2 hunks)tests/fixtures/scenarios/vendor-pull-issue-dev-3639/atmos.yaml(1 hunks)tests/fixtures/scenarios/vendor-pull-issue-dev-3639/vendor-test.yaml(1 hunks)tests/fixtures/scenarios/vendor-pull-issue-dev-3639/vendor.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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 - comments should be complete sentences (enforced by golangci-lint's godot linter)
All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Distinguish between logging and UI output: only use logging for system/debug info, and use stderr/UI for prompts, progress, errors, and data for piping should go to stdout
Most text UI MUST go to stderr; only data/results go to stdout for proper piping; use utils.PrintfMessageToTUI() for UI messages
Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
ALWAYS compile after making changes; verify no compilation errors; run tests after successful compile; fix compilation errors immediately before proceeding
Write a test to reproduce the bug before fixing, run the test and confirm failure, fix the bug iteratively, and verify fix doesn't break existing tests
Files:
internal/exec/vendor_model.gointernal/exec/vendor_issue_dev3639_test.gointernal/exec/vendor_utils.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
internal/exec/vendor_model.gointernal/exec/vendor_utils.go
internal/exec/**.go
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer extending existing utilities in internal/exec/ and pkg/ over creating new ones—search for existing methods before implementation
Files:
internal/exec/vendor_model.gointernal/exec/vendor_issue_dev3639_test.gointernal/exec/vendor_utils.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: Test file naming symmetry: test files mirror implementation structure with _test.go suffix
Unit tests should focus on pure functions and use table-driven tests
ALWAYS use t.Skipf() instead of t.Skip() and provide a clear reason for skipped tests
NEVER use t.Skipf() without a reason
Files:
internal/exec/vendor_issue_dev3639_test.go
🧠 Learnings (6)
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
tests/fixtures/scenarios/vendor-pull-issue-dev-3639/vendor-test.yamltests/fixtures/scenarios/vendor-pull-issue-dev-3639/atmos.yamltests/fixtures/scenarios/vendor-pull-issue-dev-3639/vendor.yaml
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
PR: cloudposse/atmos#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:
tests/fixtures/scenarios/vendor-pull-issue-dev-3639/atmos.yaml
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
tests/fixtures/scenarios/vendor-pull-issue-dev-3639/atmos.yamltests/fixtures/scenarios/vendor-pull-issue-dev-3639/vendor.yaml
📚 Learning: 2024-10-22T23:00:20.627Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:131-141
Timestamp: 2024-10-22T23:00:20.627Z
Learning: In the `ReadAndProcessVendorConfigFile` function in `internal/exec/vendor_utils.go`, the existence of the vendor config file is already checked, so additional file existence checks may be unnecessary.
Applied to files:
internal/exec/vendor_issue_dev3639_test.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to cmd/*.go : Co-locate tests with implementation (_test.go alongside .go files in cmd/)
Applied to files:
internal/exec/vendor_issue_dev3639_test.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to tests/**/*.go : Shared test utilities should be placed in the tests/ directory for integration tests
Applied to files:
internal/exec/vendor_issue_dev3639_test.go
🧬 Code graph analysis (1)
internal/exec/vendor_issue_dev3639_test.go (2)
tests/test_preconditions.go (1)
RequireGitHubAccess(206-230)internal/exec/vendor.go (1)
ExecuteVendorPullCommand(45-73)
⏰ 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). (7)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (4)
tests/fixtures/scenarios/vendor-pull-issue-dev-3639/vendor-test.yaml (1)
1-10: Fixture looks good.Self-contained and benign for exercising the multi‑vendor‑file scenario.
internal/exec/vendor_model.go (1)
345-351: Whitespace-only change. LGTM.No functional impact.
tests/fixtures/scenarios/vendor-pull-issue-dev-3639/atmos.yaml (1)
1-18: Fixture is appropriate.Covers base_path, stacks discovery, and logging for the scenario.
internal/exec/vendor_utils.go (1)
278-281: Apply the godot comment fix; normalization coverage verified.Apply the diff below to satisfy golangci-lint/godot. normalizeVendorURI is already called immediately before determineSourceType in internal/exec/vendor_utils.go (no other call sites found).
- // Normalize the URI to handle triple slash pattern (///) which means "root of the repository" - // go-getter v1.7.9 no longer supports the triple slash pattern, so we need to convert it + // Normalize the URI to handle the triple-slash pattern (///), which means the root of the repository. + // go-getter v1.7.9 no longer supports the triple-slash pattern, so we convert it. uri = normalizeVendorURI(uri)
- Renamed test file from vendor_issue_dev3639_test.go to vendor_triple_slash_test.go - Renamed test scenario directory from vendor-pull-issue-dev-3639 to vendor-triple-slash - Updated test function name and comments to describe what is being tested (triple-slash pattern) - Test files should be named based on what they test, not issue numbers
- Clarified that // is a delimiter and / after it represents the subdirectory path - Explained that /// means empty subdirectory (root of repository) - Added reference to CVE-2025-8959 which caused the breaking change in go-getter v1.7.9 - Updated inline comments to be more precise about the pattern's meaning
…tion - Added comprehensive unit tests for normalizeVendorURI function - Fixed normalizeVendorURI to be more precise about .git/// patterns - Preserved file:/// URIs unchanged (valid file scheme) - Added missing verifyFileExists function in terraform_clean_test.go - Addressed linter issues (nestif complexity, magic numbers)
Updated documentation to describe the triple-slash pattern (///) as indicating the root of the repository rather than an "empty subdirectory", which is more accurate and intuitive for understanding the go-getter URL syntax. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/exec/terraform_clean_test.go (1)
76-84: Mark helper with t.Helper() for cleaner failure reporting.This ensures errors are attributed to the caller, not the helper.
func verifyFileExists(t *testing.T, files []string) (bool, string) { + t.Helper() for _, file := range files { if _, err := os.Stat(file); err != nil { t.Errorf("Reason: Expected file does not exist: %q", file) return false, file } } return true, "" }internal/exec/vendor_utils_test.go (1)
9-63: Solid coverage; add a couple of additional URI forms.Recommend adding cases for HTTPS and SCP‑style Git URLs to prevent regressions with common forms.
{ name: "multiple triple-slash patterns (only first is processed)", input: "github.com/repo.git///path///subpath?ref=v1.0", expected: "github.com/repo.git//path///subpath?ref=v1.0", }, + { + name: "https scheme with triple-slash at root", + input: "https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git///?ref=v5.7.0", + expected: "https://github.com/terraform-aws-modules/terraform-aws-s3-bucket.git?ref=v5.7.0", + }, + { + name: "SCP-style Git URL with triple-slash module path", + input: "[email protected]:cloudposse/terraform-aws-components.git///modules/vpc?ref=1.398.0", + expected: "[email protected]:cloudposse/terraform-aws-components.git//modules/vpc?ref=1.398.0", + },internal/exec/vendor_triple_slash_test.go (2)
15-18: Fix godot: end doc comment sentences with periods.Golangci-lint’s godot rule will flag these.
-// TestVendorPullWithTripleSlashPattern tests the vendor pull command with the triple-slash pattern -// which indicates cloning from the root of a repository (e.g., github.com/repo.git///?ref=v1.0). +// TestVendorPullWithTripleSlashPattern tests the vendor pull command with the triple-slash pattern. +// It indicates cloning from the root of a repository (e.g., github.com/repo.git///?ref=v1.0). // This pattern was broken after go-getter v1.7.9 due to changes in subdirectory path handling.
37-61: Prefer t.Cleanup for env and cwd restoration.t.Cleanup keeps teardown close to setup and runs even if subtests are added later.
Example:
startingDir, err := os.Getwd() require.NoError(t, err) t.Cleanup(func() { require.NoError(t, os.Chdir(startingDir)) }) orig := map[string]string{ "ATMOS_CLI_CONFIG_PATH": os.Getenv("ATMOS_CLI_CONFIG_PATH"), "ATMOS_BASE_PATH": os.Getenv("ATMOS_BASE_PATH"), "ATMOS_LOGS_LEVEL": os.Getenv("ATMOS_LOGS_LEVEL"), } t.Cleanup(func() { for k, v := range orig { if v == "" { os.Unsetenv(k) } else { os.Setenv(k, v) } } })Also applies to: 151-175
📜 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)
internal/exec/terraform_clean_test.go(1 hunks)internal/exec/vendor_triple_slash_test.go(1 hunks)internal/exec/vendor_utils.go(2 hunks)internal/exec/vendor_utils_test.go(1 hunks)tests/fixtures/scenarios/vendor-triple-slash/atmos.yaml(1 hunks)tests/fixtures/scenarios/vendor-triple-slash/vendor-test.yaml(1 hunks)tests/fixtures/scenarios/vendor-triple-slash/vendor.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/fixtures/scenarios/vendor-triple-slash/vendor.yaml
- tests/fixtures/scenarios/vendor-triple-slash/vendor-test.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_utils.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*_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: Test file naming symmetry: test files mirror implementation structure with _test.go suffix
Unit tests should focus on pure functions and use table-driven tests
ALWAYS use t.Skipf() instead of t.Skip() and provide a clear reason for skipped tests
NEVER use t.Skipf() without a reason
Files:
internal/exec/vendor_utils_test.gointernal/exec/vendor_triple_slash_test.gointernal/exec/terraform_clean_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 - comments should be complete sentences (enforced by golangci-lint's godot linter)
All errors MUST be wrapped using static errors defined in errors/errors.go; never use dynamic errors directly
Always wrap errors with the static error first and add context-specific details after the static error; use %w verb to preserve error chain
ALWAYS use viper.BindEnv() for environment variable binding, and EVERY env var MUST have an ATMOS_ alternative
Distinguish between logging and UI output: only use logging for system/debug info, and use stderr/UI for prompts, progress, errors, and data for piping should go to stdout
Most text UI MUST go to stderr; only data/results go to stdout for proper piping; use utils.PrintfMessageToTUI() for UI messages
Atmos MUST work on Linux, macOS, and Windows—write portable implementations and prefer SDKs over calling binaries when available
ALWAYS compile after making changes; verify no compilation errors; run tests after successful compile; fix compilation errors immediately before proceeding
Write a test to reproduce the bug before fixing, run the test and confirm failure, fix the bug iteratively, and verify fix doesn't break existing tests
Files:
internal/exec/vendor_utils_test.gointernal/exec/vendor_triple_slash_test.gointernal/exec/terraform_clean_test.go
internal/exec/**.go
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer extending existing utilities in internal/exec/ and pkg/ over creating new ones—search for existing methods before implementation
Files:
internal/exec/vendor_utils_test.gointernal/exec/vendor_triple_slash_test.gointernal/exec/terraform_clean_test.go
🧠 Learnings (13)
📚 Learning: 2024-10-22T23:00:20.627Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:131-141
Timestamp: 2024-10-22T23:00:20.627Z
Learning: In the `ReadAndProcessVendorConfigFile` function in `internal/exec/vendor_utils.go`, the existence of the vendor config file is already checked, so additional file existence checks may be unnecessary.
Applied to files:
internal/exec/vendor_triple_slash_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:
internal/exec/vendor_triple_slash_test.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to cmd/*.go : Co-locate tests with implementation (_test.go alongside .go files in cmd/)
Applied to files:
internal/exec/vendor_triple_slash_test.go
📚 Learning: 2025-09-23T03:44:59.111Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-23T03:44:59.111Z
Learning: Applies to **/*_test.go : Test file naming symmetry: test files mirror implementation structure with _test.go suffix
Applied to files:
internal/exec/vendor_triple_slash_test.gointernal/exec/terraform_clean_test.go
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
tests/fixtures/scenarios/vendor-triple-slash/atmos.yaml
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
PR: cloudposse/atmos#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:
tests/fixtures/scenarios/vendor-triple-slash/atmos.yaml
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
tests/fixtures/scenarios/vendor-triple-slash/atmos.yaml
📚 Learning: 2025-01-25T03:51:57.689Z
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.
Applied to files:
tests/fixtures/scenarios/vendor-triple-slash/atmos.yaml
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
internal/exec/terraform_clean_test.go
📚 Learning: 2024-10-27T04:28:40.966Z
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:155-175
Timestamp: 2024-10-27T04:28:40.966Z
Learning: In the `CollectDirectoryObjects` function in `internal/exec/terraform_clean.go`, recursive search through all subdirectories is not needed.
Applied to files:
internal/exec/terraform_clean_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
PR: cloudposse/atmos#775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
internal/exec/terraform_clean_test.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
PR: cloudposse/atmos#759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
internal/exec/terraform_clean_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:
internal/exec/terraform_clean_test.go
🧬 Code graph analysis (1)
internal/exec/vendor_triple_slash_test.go (2)
tests/test_preconditions.go (1)
RequireGitHubAccess(206-230)internal/exec/vendor.go (1)
ExecuteVendorPullCommand(45-73)
⏰ 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). (7)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
🔇 Additional comments (2)
tests/fixtures/scenarios/vendor-triple-slash/atmos.yaml (1)
1-18: Fixture LGTM.Config matches test intent and uses local atmos.yaml in scenario dir.
internal/exec/vendor_triple_slash_test.go (1)
128-132: Fix godot on second test doc comment (already flagged earlier).Add a trailing period to the first sentence.
-// TestVendorPullWithMultipleVendorFiles tests that vendor pull works correctly +// TestVendorPullWithMultipleVendorFiles tests that vendor pull works correctly. // even when there are multiple vendor YAML files in the same directory. // This could be a potential cause of the issue where the vendor process // gets confused by multiple configuration files.
- Created unified normalizeVendorURI function to handle all URI normalization in one place - Converts triple-slash (///) to double-slash-dot (//.) for repository root - Adds //. to Git URLs without subdirectory delimiter - Extracted URI pattern detection logic into separate helper functions for better maintainability - Deprecated adjustSubdir in CustomGitDetector as normalization now happens earlier in pipeline - Fixes vendor pull failures with triple-slash patterns after go-getter v1.7.9 update This ensures backward compatibility with existing vendor configurations while properly handling the go-getter v1.7.9+ requirements for subdirectory paths. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Handles all URI normalization in one place - Properly converts /// to //. for repository root - Adds //. to Git URLs without subdirectory - Works with go-getter v1.7.9+
- Remove duplicate test helper function in terraform_clean_test.go - Replace helper calls with inline file existence checks - Add comprehensive test cases for HTTPS and SCP-style Git URLs - Add test coverage for git protocol with triple-slash patterns - Improve test coverage for URI normalization edge cases These changes address the review feedback while maintaining existing functionality and test coverage.
Per godot linter requirements, all comments must end with periods. This is enforced by golangci-lint in the project.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/exec/terraform_clean_test.go (2)
60-65: Tighten assertions and message: use require and say “path”, not “file”.Cleaner and more idiomatic with require, plus accommodates dirs in your list.
Apply this diff:
- // Verify that expected files exist after apply - for _, file := range files { - if _, err := os.Stat(file); err != nil { - t.Fatalf("Expected file %s does not exist: %v", file, err) - } - } + // Verify that expected paths exist after apply. + for _, p := range files { + _, err := os.Stat(p) + require.NoErrorf(t, err, "expected path to exist: %s (os.Stat error: %v)", p, err) + }
72-76: Clarify deletion assertion wording; keep check.Minor wording nit; you’re checking both files and dirs.
Apply this diff:
- // Verify that files were deleted after clean + // Verify that paths were deleted after clean. for _, file := range files { - if _, err := os.Stat(file); !os.IsNotExist(err) { - t.Fatalf("File %s should have been deleted but still exists", file) + if _, err := os.Stat(file); !os.IsNotExist(err) { + t.Fatalf("Path %s should have been deleted but still exists (os.Stat error: %v)", file, err) } }internal/exec/vendor_utils_test.go (1)
9-115: Add a case for triple‑slash without “.git” to guard non‑.git repos.Covers a realistic pattern used in the wild and avoids regressions if the repo path lacks “.git”.
Apply this diff to extend the table:
{ name: "git URL without .git extension and no subdir", input: "github.com/terraform-aws-modules/terraform-aws-s3-bucket?ref=v5.7.0", expected: "github.com/terraform-aws-modules/terraform-aws-s3-bucket//.?ref=v5.7.0", }, + { + name: "triple-slash without .git extension", + input: "github.com/cloudposse/terraform-aws-components///?ref=1.398.0", + expected: "github.com/cloudposse/terraform-aws-components//.?ref=1.398.0", + },internal/exec/vendor_utils.go (2)
432-466: Doc comment polish and consistent stderr logging.
- Godot: end bullet sentences with periods to satisfy lint.
- Use StderrLogger for consistency with the file’s stderr policy.
Apply this diff:
-// This function handles multiple normalization cases: -// 1. Converts triple-slash (///) to double-slash-dot (//.) for root directory -// 2. Adds //. to Git URLs without subdirectory delimiter -// 3. Preserves existing valid patterns unchanged +// This function handles multiple normalization cases: +// 1. Converts triple-slash (///) to double-slash-dot (//.) for root directory. +// 2. Adds //. to Git URLs without subdirectory delimiter. +// 3. Preserves existing valid patterns unchanged. @@ - if needsDoubleSlashDot(uri) { - uri = appendDoubleSlashDot(uri) - log.Debug("Added //. to Git URL without subdirectory", "normalized", uri) - } + if needsDoubleSlashDot(uri) { + uri = appendDoubleSlashDot(uri) + StderrLogger.Debug("Added //. to Git URL without subdirectory.", "normalized", uri) + }As per coding guidelines.
467-496: Make triple‑slash normalization host/TLD agnostic (remove brittle patterns).Current matching against “.git///”, “.com///”, “.org///” is fragile and misses valid forms (e.g., repos without .git, other TLDs). Normalize the first “///” occurrence generically.
Apply this diff:
-func normalizeTripleSlash(uri string) string { - // Try different triple-slash patterns - patterns := []string{".git///", ".com///", ".org///"} - - for _, pattern := range patterns { - base, suffix, found := extractURIParts(uri, pattern) - if !found { - continue - } - - // Check what comes after the triple slash - if suffix == "" || strings.HasPrefix(suffix, "?") { - // Root of repository case: convert /// to //. - normalized := base + "//." + suffix - log.Debug("Normalized triple-slash to double-slash-dot for repository root", - "original", uri, "normalized", normalized) - return normalized - } - - // Path specified after triple slash: convert /// to // - normalized := base + "//" + suffix - log.Debug("Normalized triple-slash to double-slash with path", - "original", uri, "normalized", normalized) - return normalized - } - - return uri -} +func normalizeTripleSlash(uri string) string { + // Normalize only the first occurrence to preserve any intentional subsequent sequences. + idx := strings.Index(uri, "///") + if idx == -1 { + return uri + } + base := uri[:idx] + suffix := uri[idx+3:] + if suffix == "" || strings.HasPrefix(suffix, "?") { + // Root of repository case: convert /// to //. + normalized := base + "//." + suffix + StderrLogger.Debug("Normalized triple-slash to double-slash-dot for repository root.", "original", uri, "normalized", normalized) + return normalized + } + // Path specified after triple slash: convert /// to //. + normalized := base + "//" + suffix + StderrLogger.Debug("Normalized triple-slash to double-slash with path.", "original", uri, "normalized", normalized) + return normalized +}
📜 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 (3)
internal/exec/terraform_clean_test.go(1 hunks)internal/exec/vendor_utils.go(2 hunks)internal/exec/vendor_utils_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*_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:
internal/exec/vendor_utils_test.gointernal/exec/terraform_clean_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:
internal/exec/vendor_utils_test.gointernal/exec/vendor_utils.gointernal/exec/terraform_clean_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
internal/exec/vendor_utils.go
🧠 Learnings (8)
📚 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 : Use table-driven tests for unit tests and focus on pure functions where possible.
Applied to files:
internal/exec/vendor_utils_test.go
📚 Learning: 2024-10-22T23:00:20.627Z
Learnt from: Cerebrovinny
PR: cloudposse/atmos#737
File: internal/exec/vendor_utils.go:131-141
Timestamp: 2024-10-22T23:00:20.627Z
Learning: In the `ReadAndProcessVendorConfigFile` function in `internal/exec/vendor_utils.go`, the existence of the vendor config file is already checked, so additional file existence checks may be unnecessary.
Applied to files:
internal/exec/vendor_utils_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 : Use table-driven tests for multiple scenarios
Applied to files:
internal/exec/vendor_utils_test.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
internal/exec/terraform_clean_test.go
📚 Learning: 2024-10-27T04:28:40.966Z
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:155-175
Timestamp: 2024-10-27T04:28:40.966Z
Learning: In the `CollectDirectoryObjects` function in `internal/exec/terraform_clean.go`, recursive search through all subdirectories is not needed.
Applied to files:
internal/exec/terraform_clean_test.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
PR: cloudposse/atmos#759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
internal/exec/terraform_clean_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
PR: cloudposse/atmos#775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
internal/exec/terraform_clean_test.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
PR: cloudposse/atmos#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:
internal/exec/terraform_clean_test.go
🧬 Code graph analysis (1)
internal/exec/terraform_clean_test.go (2)
pkg/schema/schema.go (1)
ConfigAndStacksInfo(455-532)internal/exec/terraform.go (1)
ExecuteTerraform(64-634)
⏰ 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 (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/vendor_utils_test.go (2)
9-115: Nice, comprehensive table-driven coverage.Good mix of schemes and edge-cases; expectations align with normalization semantics.
71-74: Go-getter supports SCP-style Git URLs with//subdir
go-getter v1.7.9 accepts[email protected]:org/repo.git//path/to/subdir(and//.for root), so the expected[email protected]:cloudposse/atmos.git//.is valid.internal/exec/vendor_utils.go (1)
278-282: Normalization placement is spot-on.Doing this immediately after templating and before type resolution keeps behavior consistent and backward compatible.
Updated golden snapshots to include new debug log messages that show Git URL normalization with //. pattern. These logs help trace the URL transformation process during vendor operations. Tests updated: - TestCLICommands_atmos_vendor_pull_using_SSH - TestCLICommands_atmos_vendor_pull_with_custom_detector_and_handling_credentials_leakage 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1504 +/- ##
==========================================
+ Coverage 59.45% 59.72% +0.27%
==========================================
Files 288 289 +1
Lines 32580 32781 +201
==========================================
+ Hits 19370 19579 +209
+ Misses 11291 11277 -14
- Partials 1919 1925 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
internal/exec/vendor_model.go(1 hunks)internal/exec/vendor_utils.go(2 hunks)pkg/downloader/custom_git_detector.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/downloader/custom_git_detector.go
- internal/exec/vendor_model.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
internal/exec/vendor_utils.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
internal/exec/vendor_utils.go
internal/exec/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain >80% coverage for core orchestration in internal/exec/.
Files:
internal/exec/vendor_utils.go
🧬 Code graph analysis (1)
internal/exec/vendor_utils.go (1)
pkg/logger/log.go (1)
Debug(24-26)
🪛 GitHub Check: golangci-lint
internal/exec/vendor_utils.go
[failure] 436-436:
Sentence should start with a capital letter
🪛 GitHub Actions: Pre-commit
internal/exec/vendor_utils.go
[error] 436-436: golangci-lint: Sentence should start with a capital letter (godot) at line 436.
⏰ 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). (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/vendor_utils.go (3)
282-285: LGTM on the normalization integration!The integration point for URI normalization looks solid. Placing it right after template processing and before source type determination ensures all vendor URIs get normalized consistently. The comment explains the context well too.
471-499: Solid normalization logic with good logging.The triple-slash pattern handling looks comprehensive and well-thought-out. The logging will help debug any vendor pull issues, and the pattern matching covers the main Git hosting scenarios.
432-433: Fix the capitalization in the comment.Based on your golangci-lint configuration, comments should start with a capital letter.
- // If `--tags` list is specified, and it does not contain any tags defined in this component, skip this component. + // If `--tags` list is specified, and it does not contain any tags defined in this component, skip this component.
* updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * [autofix.ci] apply automated fixes * updates * updates * updates * updates * updates * updates --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…1533) * fix: merge commands from .atmos.d instead of replacing them Fixed a regression where commands defined in .atmos.d/ directories were being replaced instead of merged with main configuration commands. This prevented custom commands like 'atmos dev' from working properly. The issue was introduced in PRs #1447 and #1489 which formalized the override behavior for configuration imports. While this behavior is correct for most configuration fields, command arrays should be merged to allow extending the CLI with custom commands. Changes: - Added mergeCommandArrays function to properly merge command arrays with deduplication based on command names - Updated mergeConfigFile to preserve and merge command arrays when processing imports - Modified processConfigImportsAndReapply to ensure commands from .atmos.d and other imports are preserved when re-applying main config - Added comprehensive tests for command merging from .atmos.d - Fixed existing test that was checking for incorrect error message This fix ensures that: - Commands from .atmos.d directories are properly merged with main configuration commands - Multiple command sources can coexist without replacing each other - The 'atmos dev' command and its subcommands work as expected Fixes the regression where 'atmos dev --help' was not working. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * test: add comprehensive test coverage for command import merging - Add command_merging_behavior_test.go with detailed merge scenarios - Add import_commands_test.go for various import configurations - Add final_test.go with real-world validation tests - Update imports.go with debug logging for import processing - Fix load.go command merging to properly handle import precedence These tests validate: - Commands from imports merge with local commands - Local commands override imported commands with duplicate names - Deep nested imports (4+ levels) work correctly - CloudPosse use case: 10 upstream + 1 local = 11 total commands * refactor: rename final_test.go to command_merge_core_test.go - Rename test file to better describe its purpose - Update function name from TestFinalCommandMergingBehavior to TestCommandMergeCore - This test validates core command merging functionality * docs: add comprehensive PRD for command merging functionality - Added Product Requirements Document detailing command merging implementation - Included PR description with technical details and test coverage information - Documents fix for regression where imported commands were replaced instead of merged - Captures CloudPosse's use case and multi-level import requirements * chore: remove PR description file from repository * test: fix failing test expectations after command merge order changes - Update test expectations to match actual command ordering behavior - Commands now appear in order: main, defaults (.atmos.d), imports - This allows main commands to override imported commands by name - Fix test YAML to use string steps instead of complex step objects - Update error message expectation in processConfigImportsAndReapply test * test: update golden snapshots for command merging functionality The test 'atmos_describe_config_imports' was failing because it now correctly merges commands from imported configurations (including remote imports from GitHub) instead of replacing them. Updated the golden snapshots to reflect the expected output with merged commands (tf, terraform, show, and test). This fixes test failures on Linux, macOS, and Windows CI pipelines. * test: fix failing test expectations after command merge order changes - Fixed all inline comments in command_merging_behavior_test.go to end with periods as required by golangci-lint's godot linter - Replaced fragment-only markdown links with full GitHub URLs in command-merging.md to satisfy markdownlint MD051 (fixes #1447 and #1489 links) - Added 'text' language hint to fenced code block in command-merging.md to satisfy markdownlint MD040 These changes address all linting warnings and ensure consistent code style. --------- Co-authored-by: Claude <[email protected]> Co-authored-by: Andriy Knysh <[email protected]>
Bumps [github.com/aws/aws-sdk-go-v2/service/ssm](https://github.com/aws/aws-sdk-go-v2) from 1.62.0 to 1.65.1. - [Release notes](https://github.com/aws/aws-sdk-go-v2/releases) - [Changelog](https://github.com/aws/aws-sdk-go-v2/blob/main/changelog-template.json) - [Commits](aws/aws-sdk-go-v2@service/s3/v1.62.0...service/s3/v1.65.1) --- updated-dependencies: - dependency-name: github.com/aws/aws-sdk-go-v2/service/ssm dependency-version: 1.65.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
- Remove trailing // before appending //. to avoid creating //// - Add test case for URI already ending with // (e.g., github.com/org/repo.git//?ref=v1.0) - Ensures correct normalization to github.com/org/repo.git//.?ref=v1.0 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/load.go (1)
322-356: Drop the double%wwrapper.
fmt.Errorfpermits exactly one%w. WithErrWrappingFormatset to"%w: %w", these three calls will panic the first time the branch executes. Swap to a single%wand include the inner error context via%v(or similar) so we keep the sentinel and the message without tripping the runtime.- return fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrParseMainConfig, err) + return fmt.Errorf("%w: parse main config: %v", errUtils.ErrParseMainConfig, err)Please adjust the merge and re-apply cases the same way.
🧹 Nitpick comments (1)
internal/exec/vendor_uri_helpers_test.go (1)
734-792: Add Azure triple-slash regression test. Would you add a case for an Azure DevOps triple-slash URI (e.g.https://dev.azure.com/org/proj/_git/repo///modules?ref=main) so we lock in the scenario that triggered DEV-3639? That slice of coverage will keep us honest if go-getter tweaks its splitting again.
📜 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)
errors/errors.go(1 hunks)internal/exec/vendor_uri_helpers.go(1 hunks)internal/exec/vendor_uri_helpers_test.go(1 hunks)pkg/config/load.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- errors/errors.go
🧰 Additional context used
📓 Path-based instructions (6)
**/*_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: Always use t.Skipf with a clear reason instead of t.Skip in tests.
Maintain test file naming symmetry: test files mirror implementation names (e.g., aws_ssm_store_test.go for aws_ssm_store.go).
Files:
internal/exec/vendor_uri_helpers_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 linter.
Organize imports into three groups separated by blank lines: (1) Go standard library, (2) 3rd-party (not github.com/cloudposse/atmos), (3) Atmos imports; sort alphabetically within each group and preserve existing aliases.
When binding environment variables, always use viper.BindEnv and ensure every env var has an ATMOS_ alternative.
Files:
internal/exec/vendor_uri_helpers_test.gointernal/exec/vendor_uri_helpers.gopkg/config/load.go
{cmd,internal,pkg}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
{cmd,internal,pkg}/**/*.go: Add defer perf.Track(...) to all public functions and critical private functions, followed by a blank line; use package-qualified names like "packagename.FunctionName"; pass atmosConfig if available, otherwise nil.
Wrap all returned errors using static errors from errors/errors.go first, then add context with fmt.Errorf using %w.
Distinguish UI output from logging: UI prompts/status/errors go to stderr (e.g., utils.PrintfMessageToTUI or fmt.Fprintf(os.Stderr,...)), data/results to stdout; never use logging for UI.
Use structured logging for system/debug info; logging must not affect execution; follow logging hierarchy and avoid string interpolation in log messages.
Ensure cross-platform compatibility: prefer SDKs over external binaries, use filepath/os utilities, and apply GOOS checks only when necessary.
Files:
internal/exec/vendor_uri_helpers_test.gointernal/exec/vendor_uri_helpers.gopkg/config/load.go
internal/exec/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Test template rendering with various contexts when modifying template execution paths.
Files:
internal/exec/vendor_uri_helpers_test.gointernal/exec/vendor_uri_helpers.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
internal/exec/vendor_uri_helpers.gopkg/config/load.go
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/config/load.go
🧠 Learnings (4)
📚 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:
internal/exec/vendor_uri_helpers.go
📚 Learning: 2025-02-03T15:51:48.035Z
Learnt from: osterman
PR: cloudposse/atmos#984
File: internal/exec/go_getter_utils.go:103-109
Timestamp: 2025-02-03T15:51:48.035Z
Learning: When checking for subdirectories in GitHub URLs, use `parsedURL.Path` to check for "//" instead of the entire URL, as the scheme portion (e.g., "https://") will always contain "//".
Applied to files:
internal/exec/vendor_uri_helpers.go
📚 Learning: 2025-10-02T13:26:25.042Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-02T13:26:25.042Z
Learning: Applies to {cmd,internal,pkg}/**/*.go : Wrap all returned errors using static errors from errors/errors.go first, then add context with fmt.Errorf using %w.
Applied to files:
pkg/config/load.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/config/load.go
🧬 Code graph analysis (1)
pkg/config/load.go (1)
errors/errors.go (4)
ErrWrappingFormat(8-8)ErrParseMainConfig(155-155)ErrMergeMainConfig(156-156)ErrReapplyMainConfig(157-157)
⏰ 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). (7)
- GitHub Check: website-deploy-preview
- 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
- Replace invalid double %w format with proper sentinel error wrapping - Use ErrMerge as sentinel with %w and original error as context with %v - Remove unused ErrParseMainConfig, ErrMergeMainConfig, ErrReapplyMainConfig - Add Azure DevOps triple-slash regression test for DEV-3639 - Prevents runtime panic from invalid fmt.Errorf format string Addresses code review feedback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Move ErrBasePath, ErrTempDir, ErrResolveLocal, etc. to errors/errors.go - Update pkg/config/imports.go to use errUtils package - Remove local error declarations per project error policy - Add ErrNoValidAbsolutePaths for line 323 usage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Profiler Errors Cannot Be CentralizedI investigated moving the profiler errors from The Resolution: The profiler errors must remain local to The errors are:
|
- Add sort.Slice to sort resolvedPaths by filePath before iteration - Ensures import logs always appear in consistent alphabetical order - Makes test snapshots deterministic across platforms and runs - Path normalization already handled by tests/cli_test.go sanitizeOutput() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This reverts commit 774c6f9.
- Replace defer with t.Cleanup for better test cleanup management - Use t.Setenv for all environment variables (auto-restores on cleanup) - Set ATMOS_CLI_CONFIG_PATH and ATMOS_BASE_PATH properly - Cleanup runs even if subtests are added later - Addresses CodeRabbit review feedback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Document how to reply to specific review threads using GraphQL API - Include query to find unresolved threads - Mandate replying to threads, not creating new PR comments 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
aknysh
deleted the
feature/dev-3639-failed-to-pull-vendors-on-latest-atmos
branch
|
These changes were released in v1.193.0-rc.0. |
Response to CodeRabbit Review CommentsAll actionable review issues have been addressed in recent commits. However, some CodeRabbit suggestions are not actual issues: ✅ Abstract Filter Column (components.go:217-218) - NOT AN ISSUECodeRabbit suggested: Change filter from Evidence:
The logic is also correct:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 400 🏁 Script executed: Length of output: 1972 🏁 Script executed: Length of output: 1003 🏁 Script executed: Length of output: 3750 🏁 Script executed: Length of output: 1424 🏁 Script executed: Length of output: 2617 @osterman - I notice the files you're referencing ( Looking at the files changed in this PR, I see vendor-related files like:
The only list-related file in this PR is It looks like you may have intended to post this comment on a different PR - possibly #1798 ("Implement configurable output for list commands") or another recent PR dealing with list functionality? Could you clarify which PR these review comments were on? I'd be happy to re-review once we're on the correct PR thread. 🧠 Learnings used |
Summary
///) patterns in vendor.yaml URIsBackground & Root Cause
This issue was introduced in Atmos v1.189.0 when go-getter was updated from v1.7.8 to v1.7.9 to address security vulnerability CVE-2025-8959. The security fix in go-getter v1.7.9 changed how subdirectory paths are handled in Git URLs, breaking the triple-slash pattern that was previously documented in Atmos examples.
Timeline:
The Problem:
Users following Atmos documentation were using vendor configurations like:
The triple-slash pattern (
///) was interpreted by go-getter v1.7.9+ as attempting to access a subdirectory path starting with/, which resulted in:Solution
This PR adds a
normalizeVendorURI()function that detects and converts legacy triple-slash patterns:repo.git///?ref=v1.0→repo.git?ref=v1.0(empty subdirectory)repo.git///path?ref=v1.0→repo.git//path?ref=v1.0(with subdirectory)The normalization is applied transparently to all vendor URIs, ensuring backward compatibility while maintaining the security improvements from go-getter v1.7.9.
Test plan
internal/exec/vendor_issue_dev3639_test.goReferenences
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests