Support Atmos Toolchain for 3rd Party Tools #1466
Open
+16,284
−562
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
toolchain/add_test.go (4)
128-147: Invalid version should fail now that findTool validates versions.Flip expectation and drop file assertions.
Apply:
-err := AddToolVersion("terraform", "999.999.999") -require.NoError(t, err, "Should pass since we only validate tool existence, not specific version") - -// Verify the tool was added to the file (even with invalid version) -toolVersions, err := LoadToolVersions(toolVersionsFile) -require.NoError(t, err) -assert.Contains(t, toolVersions.Tools, "terraform") -assert.Contains(t, toolVersions.Tools["terraform"], "999.999.999") +err := AddToolVersion("terraform", "999.999.999") +require.Error(t, err) +assert.ErrorIs(t, err, ErrToolNotFound)
19-20: Make tests deterministic: avoid live registry/network.These call AddToolVersion(), which resolves via real registries. Introduce a test seam and stub it per test.
Apply in toolchain/add.go:
+// test seam: can be overridden in tests. +var newInstaller = NewInstaller +var ensureToolExists = func(installer *Installer, owner, repo, version string) error { + _, err := installer.findTool(owner, repo, version) + return err +} + func AddToolVersion(tool, version string) error { - installer := NewInstaller() + installer := newInstaller() owner, repo, err := installer.parseToolSpec(tool) if err != nil { - return fmt.Errorf("failed to resolve tool '%s': %w", tool, err) + return fmt.Errorf("failed to resolve tool %q: %w", tool, err) } - // Ensure the tool exists in the registry. - if _, err := installer.findTool(owner, repo, version); err != nil { - return fmt.Errorf("tool '%s' not found in registry: %w", tool, err) - } + // Ensure the tool exists in the registry. + if err := ensureToolExists(installer, owner, repo, version); err != nil { + return fmt.Errorf("tool %q not found in registry: %w", tool, err) + }Then in this test file add a small helper and use it:
@@ -import ( +import ( + "os" "path/filepath" "testing" @@ ) +// test helper to stub tool existence checks. +func stubEnsureToolExists(t *testing.T, retErr error) { + t.Helper() + prev := ensureToolExists + t.Cleanup(func() { ensureToolExists = prev }) + ensureToolExists = func(_ *Installer, _, _, _ string) error { return retErr } +} @@ SetAtmosConfig(&schema.AtmosConfiguration{ Toolchain: schema.Toolchain{FilePath: toolVersionsFile}, }) + stubEnsureToolExists(t, nil) err := AddToolVersion("terraform", "1.11.4")Repeat stubEnsureToolExists(t, nil) in the other success-path tests shown in this comment.
Also applies to: 36-37, 54-55, 117-118, 158-159, 179-180
71-74: Stop asserting on error strings; use sentinel errors and errors.Is.Define exported sentinels and wrap with context; assert with require.Error and assert.ErrorIs.
Add toolchain/errors.go:
+package toolchain + +import "errors" + +var ( + ErrToolNotFound = errors.New("tool not found") + ErrInvalidSpec = errors.New("invalid tool specification") + ErrEmptyVersion = errors.New("empty version") + ErrRegistryLookup = errors.New("registry lookup failed") +)Wrap in add.go:
@@ - owner, repo, err := installer.parseToolSpec(tool) - if err != nil { - return fmt.Errorf("failed to resolve tool %q: %w", tool, err) - } + owner, repo, err := installer.parseToolSpec(tool) + if err != nil { + return fmt.Errorf("resolve tool %q: %w", tool, ErrInvalidSpec) + } @@ - if err := ensureToolExists(installer, owner, repo, version); err != nil { - return fmt.Errorf("tool %q not found in registry: %w", tool, err) - } + if version == "" { + return fmt.Errorf("add %q: %w", tool, ErrEmptyVersion) + } + if err := ensureToolExists(installer, owner, repo, version); err != nil { + return fmt.Errorf("lookup %q: %w", tool, ErrToolNotFound) + }Update tests:
- require.Error(t, err, "Should fail when adding invalid tool") - assert.Contains(t, err.Error(), "not found in local aliases or Aqua registry") + require.Error(t, err) + assert.ErrorIs(t, err, ErrToolNotFound) @@ - require.Error(t, err, "Should fail when adding invalid tool with canonical name") - assert.Contains(t, err.Error(), "not found in any registry") + require.Error(t, err) + assert.ErrorIs(t, err, ErrToolNotFound) @@ - require.Error(t, err, "Should fail for edge case") - assert.Contains(t, err.Error(), tt.errorMsg) + require.Error(t, err) + switch tt.name { + case "empty tool name", "malformed tool name": + assert.ErrorIs(t, err, ErrInvalidSpec) + case "empty version": + assert.ErrorIs(t, err, ErrEmptyVersion) + }Also applies to: 90-93, 203-234
173-181: Gate network-dependent Aqua test or stub it; also fix assertion message.Either stub ensureToolExists or skip unless explicitly enabled.
Apply:
-// Note: This test may fail if kubectl is not available in the Aqua registry -// or if there are network issues +// Note: Network/registry dependent. Skip unless explicitly enabled. @@ -err := AddToolVersion("kubectl", "v1.2.7") -require.NoError(t, err, "Should fail when adding tool from Aqua registry") +if testing.Short() || os.Getenv("ATMOS_TOOLCHAIN_E2E") == "" { + t.Skipf("Skipping network-dependent Aqua test; set ATMOS_TOOLCHAIN_E2E=1 to run.") +} +stubEnsureToolExists(t, nil) +err := AddToolVersion("kubectl", "v1.2.7") +require.NoError(t, err, "Should succeed when adding tool from Aqua registry.")Ensure import of os (see prior diff).
🧹 Nitpick comments (5)
toolchain/add_test.go (5)
75-80: Assert the file was not created on failure.Be strict: check os.Stat for non-existence instead of best‑effort LoadToolVersions.
Apply:
- toolVersions, err := LoadToolVersions(toolVersionsFile) - if err == nil { - assert.NotContains(t, toolVersions.Tools, "nonexistent-tool") - } + _, statErr := os.Stat(toolVersionsFile) + require.True(t, os.IsNotExist(statErr), "tool-versions file should not be created on failure.")And same change for the canonical-name case.
Also applies to: 94-99
12-12: Rename tests for clarity.These test AddToolVersion, not the CLI command. Prefer TestAddToolVersion_*.
Apply:
-func TestAddCommand_ValidTool(t *testing.T) { +func TestAddToolVersion_ValidTool(t *testing.T) { @@ -func TestAddCommand_ValidToolWithAlias(t *testing.T) { +func TestAddToolVersion_ValidToolWithAlias(t *testing.T) { @@ -func TestAddCommand_ValidToolWithCanonicalName(t *testing.T) { +func TestAddToolVersion_ValidToolWithCanonicalName(t *testing.T) { @@ -func TestAddCommand_InvalidTool(t *testing.T) { +func TestAddToolVersion_InvalidTool(t *testing.T) { @@ -func TestAddCommand_InvalidToolWithCanonicalName(t *testing.T) { +func TestAddToolVersion_InvalidToolWithCanonicalName(t *testing.T) { @@ -func TestAddCommand_UpdateExistingTool(t *testing.T) { +func TestAddToolVersion_UpdateExistingTool(t *testing.T) { @@ -func TestAddCommand_InvalidVersion(t *testing.T) { +func TestAddToolVersion_InvalidVersion(t *testing.T) { @@ -func TestAddCommand_CustomToolVersionsFile(t *testing.T) { +func TestAddToolVersion_CustomToolVersionsFile(t *testing.T) { @@ -func TestAddCommand_AquaRegistryTool(t *testing.T) { +func TestAddToolVersion_AquaRegistryTool(t *testing.T) { @@ -func TestAddCommand_EdgeCases(t *testing.T) { +func TestAddToolVersion_EdgeCases(t *testing.T) {Also applies to: 29-29, 46-46, 64-64, 82-82, 101-101, 128-128, 149-149, 168-168, 189-189
13-16: Fix godot linter violations (comments must end with periods).Many // comments lack trailing periods.
Apply (sample):
-// Create a temporary .tool-versions file +// Create a temporary .tool-versions file. @@ -// Test adding a valid tool using canonical name +// Test adding a valid tool using canonical name. @@ -// Create a temporary directory with custom .tool-versions file +// Create a temporary directory with custom .tool-versions file. @@ -// Test adding a tool from Aqua registry -// Note: This test may fail if kubectl is not available in the Aqua registry -// or if there are network issues +// Test adding a tool from Aqua registry. +// Note: This test may fail if kubectl is not available in the Aqua registry or if there are network issues.Please apply similarly across this file.
Also applies to: 30-33, 47-50, 65-68, 83-86, 102-106, 149-156, 168-176, 189-199
39-44: Optional: consolidate repeated success-path tests into a table-driven test.Reduces duplication and speeds iteration.
I can provide a table-driven version if helpful.
Also applies to: 57-62, 120-126, 161-166, 182-187
239-240: Add CLI E2E foratmos toolchain add.Complement package tests with cmd/ integration tests gated by -short/env.
I can draft a minimal cmd test harness using golden .tool-versions fixtures and stubs.
📜 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)
toolchain/add_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
**/*.go: All comments in Go code must end with periods (enforced by golangci-lint godot).
Wrap all returned errors using static errors from the errors package; never return dynamic errors directly.
Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Separate structured logging from UI output: use stderr for prompts/errors to user; stdout only for data; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString and never capture user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Files:
toolchain/add_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 testing multiple scenarios
Include integration tests for command flows
Test CLI end-to-end when possible
Use test fixtures for complex inputs
Consider using testify/mock for creating mock implementations
**/*_test.go: Always use t.Skipf() with a reason for skipped tests; never use t.Skip() or reasonless t.Skipf().
Test files must mirror implementation structure and naming (e.g., aws_ssm_store_test.go).
Use table-driven unit tests and mock interfaces for external dependencies; target >80% coverage.
Files:
toolchain/add_test.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
toolchain/add_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Include integration tests for command flows
Applied to files:
toolchain/add_test.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to cmd/**/*_test.go : Command tests reside under cmd/.
Applied to files:
toolchain/add_test.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*_test.go : Use table-driven unit tests and mock interfaces for external dependencies; target >80% coverage.
Applied to files:
toolchain/add_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Consider using testify/mock for creating mock implementations
Applied to files:
toolchain/add_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Use test fixtures for complex inputs
Applied to files:
toolchain/add_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
toolchain/add_test.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*_test.go : Always use t.Skipf() with a reason for skipped tests; never use t.Skip() or reasonless t.Skipf().
Applied to files:
toolchain/add_test.go
🧬 Code graph analysis (1)
toolchain/add_test.go (4)
toolchain/setup.go (1)
SetAtmosConfig(22-24)pkg/schema/schema.go (2)
AtmosConfiguration(25-61)Toolchain(204-209)toolchain/add.go (1)
AddToolVersion(9-28)toolchain/tool_versions.go (3)
LoadToolVersions(22-48)ToolVersions(17-19)SaveToolVersions(51-78)
⏰ 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 (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (2)
toolchain/add_test.go (2)
13-18: Good isolation of filesystem state.Using t.TempDir() and per-test .tool-versions paths is solid.
12-27: Sanity-check alias coverage — covered by tests.toolchain/tool_versions_test.go contains explicit alias/duplicate checks (opentofu cases) and toolchain/set_test.go repeatedly mocks installer.parseToolSpec to inject deterministic alias resolution; no extra config setup needed for TestAddCommand_ValidTool.
osterman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
toolchain/installer.go (5)
3-21: Remove external tar/unzip exec calls; use Go stdlib extraction and re-run verificationVerification failed early — the script found external archive invocations in toolchain/installer.go so remaining checks weren't executed.
Locations:
- toolchain/installer.go:589 — cmd = exec.Command("tar", "-xf", zipPath, "-C", tempDir)
- toolchain/installer.go:591 — cmd = exec.Command("unzip", "-o", zipPath, "-d", tempDir)
- toolchain/installer.go:656 — cmd := exec.Command("tar", "-xzf", tarPath, "-C", tempDir)
Replace these with Go stdlib (archive/zip, archive/tar, compress/gzip) and ensure cross-platform (.exe) handling, then re-run the verification script.
769-775: executeBinary is a no-op; tools never run.This is user-visible breakage. Execute the binary and plumb stdio.
func (i *Installer) executeBinary(binaryPath string, args []string) error { - // This would use os/exec to run the binary - // For now, just print what would be executed - log.Debug("Would execute binary", "path", binaryPath, "args", args) - return nil + cmd := exec.Command(binaryPath, args...) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + if err := cmd.Run(); err != nil { + return fmt.Errorf("failed to execute %s: %w", filepath.Base(binaryPath), err) + } + return nil }
60-80: Align cache dir with XDG helpers (team decision).Docs/learnings say installer.go should use GetXDGCacheDir(); current code hardcodes ~/.cache/tools-cache.
func NewInstallerWithResolver(resolver ToolResolver) *Installer { - homeDir, err := os.UserHomeDir() - if err != nil { - log.Warn("Falling back to temp dir for cache.", "error", err) - homeDir = os.TempDir() - } - cacheDir := filepath.Join(homeDir, ".cache", "tools-cache") + // Use XDG Base Directory for cache. + cacheDir := GetXDGCacheDir()
578-644: Replace external unzip with archive/zip (portability, fewer deps).Shelling out to unzip/tar is brittle and breaks on minimal Windows/macOS containers. Use archive/zip and stream the target entry. Also removes the need for syscall and ioutil.
Apply this diff:
func (i *Installer) extractZip(zipPath, binaryPath string, tool *Tool) error { - log.Debug("Extracting ZIP archive", "filename", filepath.Base(zipPath)) - - tempDir, err := ioutil.TempDir("", "installer-extract-") - if err != nil { - return fmt.Errorf("failed to create temp dir: %w", err) - } - defer os.RemoveAll(tempDir) - var cmd *exec.Cmd - if runtime.GOOS == "windows" { - cmd = exec.Command("tar", "-xf", zipPath, "-C", tempDir) - } else { - cmd = exec.Command("unzip", "-o", zipPath, "-d", tempDir) - } - if output, err := cmd.CombinedOutput(); err != nil { - var exitCode int - // Case 1: Command ran but failed (non-zero exit) - if exitErr, ok := err.(*exec.ExitError); ok { - if status, ok := exitErr.Sys().(syscall.WaitStatus); ok { - exitCode = status.ExitStatus() - } - log.Debug("Command failed.", "exit_code", exitCode) - } else { - // Case 2: Command did NOT run at all (e.g., not found) - log.Debug("Command execution failed.", "error", err) - } - return fmt.Errorf("failed to extract ZIP: %w, output: %s", err, string(output)) - } - - binaryName := tool.Name - if binaryName == "" { - binaryName = tool.RepoName - } - - // Find the binary in the temp dir (recursively) - var found string - err = filepath.Walk(tempDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if info.Mode().IsRegular() && (info.Name() == binaryName || info.Name() == binaryName+".exe") { - found = path - return filepath.SkipDir - } - return nil - }) - if err != nil { - return fmt.Errorf("failed to search extracted files: %w", err) - } - if found == "" { - return fmt.Errorf("binary %s not found in extracted archive", binaryName) - } - - // Ensure the destination directory exists - dir := filepath.Dir(binaryPath) - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("failed to create destination directory: %w", err) - } - - // Move the binary into place - if err := os.Rename(found, binaryPath); err != nil { - return fmt.Errorf("failed to move extracted binary: %w", err) - } - - return nil + log.Debug("Extracting ZIP archive", "filename", filepath.Base(zipPath)) + zr, err := zip.OpenReader(zipPath) + if err != nil { + return fmt.Errorf("failed to open zip: %w", err) + } + defer zr.Close() + + want := tool.Name + if want == "" { + want = tool.RepoName + } + alt := want + ".exe" + + for _, f := range zr.File { + name := filepath.Base(f.Name) + if name != want && name != alt { + continue + } + rc, err := f.Open() + if err != nil { + return fmt.Errorf("failed to open zip entry: %w", err) + } + if err := os.MkdirAll(filepath.Dir(binaryPath), 0o755); err != nil { + _ = rc.Close() + return fmt.Errorf("failed to create destination directory: %w", err) + } + out, err := os.Create(binaryPath) + if err != nil { + _ = rc.Close() + return fmt.Errorf("failed to create destination file: %w", err) + } + if _, err := io.Copy(out, rc); err != nil { + _ = rc.Close() + _ = out.Close() + return fmt.Errorf("failed to write binary: %w", err) + } + _ = rc.Close() + if err := out.Close(); err != nil { + return fmt.Errorf("failed to close destination file: %w", err) + } + return nil + } + return fmt.Errorf("binary %s not found in extracted archive", want) }Also update imports as shown in the imports comment.
646-697: Replace external tar with archive/tar+gzip (portability).Avoid exec tar; parse in-process and stream the matching entry.
func (i *Installer) extractTarGz(tarPath, binaryPath string, tool *Tool) error { - log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath)) - - tempDir, err := ioutil.TempDir("", "installer-extract-") - if err != nil { - return fmt.Errorf("failed to create temp dir: %w", err) - } - defer os.RemoveAll(tempDir) - - cmd := exec.Command("tar", "-xzf", tarPath, "-C", tempDir) - if output, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("failed to extract tar.gz: %w, output: %s", err, string(output)) - } - - binaryName := tool.Name - if binaryName == "" { - binaryName = tool.RepoName - } - - // Find the binary in the temp dir (recursively) - var found string - err = filepath.Walk(tempDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if info.Mode().IsRegular() && (info.Name() == binaryName || info.Name() == binaryName+".exe") { - found = path - return filepath.SkipDir - } - return nil - }) - if err != nil { - return fmt.Errorf("failed to search extracted files: %w", err) - } - if found == "" { - return fmt.Errorf("binary %s not found in extracted archive", binaryName) - } - - // Ensure the destination directory exists - dir := filepath.Dir(binaryPath) - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("failed to create destination directory: %w", err) - } - - // Move the binary into place - if err := os.Rename(found, binaryPath); err != nil { - return fmt.Errorf("failed to move extracted binary: %w", err) - } - - return nil + log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath)) + f, err := os.Open(tarPath) + if err != nil { + return fmt.Errorf("failed to open tar.gz: %w", err) + } + defer f.Close() + gz, err := gzip.NewReader(f) + if err != nil { + return fmt.Errorf("failed to create gzip reader: %w", err) + } + defer gz.Close() + tr := tar.NewReader(gz) + + want := tool.Name + if want == "" { + want = tool.RepoName + } + alt := want + ".exe" + + for { + hdr, err := tr.Next() + if err == io.EOF { + break + } + if err != nil { + return fmt.Errorf("failed to read tar entry: %w", err) + } + if hdr.Typeflag != tar.TypeReg { + continue + } + name := filepath.Base(hdr.Name) + if name != want && name != alt { + continue + } + if err := os.MkdirAll(filepath.Dir(binaryPath), 0o755); err != nil { + return fmt.Errorf("failed to create destination directory: %w", err) + } + out, err := os.Create(binaryPath) + if err != nil { + return fmt.Errorf("failed to create destination file: %w", err) + } + if _, err := io.Copy(out, tr); err != nil { + _ = out.Close() + return fmt.Errorf("failed to write binary: %w", err) + } + if err := out.Close(); err != nil { + return fmt.Errorf("failed to close destination file: %w", err) + } + return nil + } + return fmt.Errorf("binary %s not found in extracted archive", want) }
🧹 Nitpick comments (4)
toolchain/installer.go (4)
3-21: Clean imports: drop deprecated/unused; add stdlib archives, errors, sha256.Required by the extraction and download fixes.
-import ( - "compress/gzip" - "fmt" - "io" - "io/ioutil" - "net/http" - "os" - "os/exec" - "path/filepath" - "runtime" - "strings" - "syscall" - "text/template" - "time" -) +import ( + "archive/tar" + "archive/zip" + "compress/gzip" + "crypto/sha256" + "errors" + "fmt" + "io" + "net/http" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "text/template" + "time" +)
262-269: Wrap errors with context (file path).Aids debugging and matches repo conventions.
- data, err := os.ReadFile(filePath) - if err != nil { - return nil, err - } + data, err := os.ReadFile(filePath) + if err != nil { + return nil, fmt.Errorf("read tool file %s: %w", filePath, err) + } @@ - if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil { - return nil, err - } + if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil { + return nil, fmt.Errorf("unmarshal tool file %s: %w", filePath, err) + }
23-25: Fix godoc/godot nits.End sentences with periods; add missing doc for GetResolver.
-// ToolResolver defines an interface for resolving tool names to owner/repo pairs -// This allows for mocking in tests and flexible resolution in production. +// ToolResolver defines an interface for resolving tool names to owner/repo pairs. +// This allows for mocking in tests and flexible resolution in production.+// GetResolver returns the active ToolResolver. func (i *Installer) GetResolver() ToolResolver { return i.resolver }Also applies to: 931-933
175-177: Avoid hardcoding CLI incantations in library errors.Keep messages generic; CLI may differ.
- return fmt.Errorf("tool %s/%s@%s is not installed. Run 'toolchain install %s/%s@%s' first", - owner, repo, version, owner, repo, version) + return fmt.Errorf("tool %s/%s@%s is not installed", owner, repo, version)
📜 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)
toolchain/exec_test.go(1 hunks)toolchain/installer.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- toolchain/exec_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
**/*.go: All comments in Go code must end with periods (enforced by golangci-lint godot).
Wrap all returned errors using static errors from the errors package; never return dynamic errors directly.
Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Separate structured logging from UI output: use stderr for prompts/errors to user; stdout only for data; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString and never capture user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Files:
toolchain/installer.go
🧠 Learnings (21)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*.go : Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to pkg/utils/file_utils.go : Prefer existing file operation utilities in pkg/utils/file_utils.go before adding new ones.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document all exported functions, types, and methods
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow Go's documentation conventions
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T16:51:37.868Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:69-97
Timestamp: 2024-12-13T16:51:37.868Z
Learning: In `pkg/config/cache.go`, the function `SaveCache2` is currently unused because it does not work properly on Windows and will be addressed later.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to cmd/*.go : Include troubleshooting hints when appropriate
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
PR: cloudposse/atmos#896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
🧬 Code graph analysis (1)
toolchain/installer.go (6)
toolchain/local_config.go (2)
NewLocalConfigManager(43-45)LocalConfigManager(38-40)toolchain/setup.go (3)
GetToolsConfigFilePath(43-48)GetToolsDirPath(35-40)GetToolVersionsFilePath(27-32)toolchain/types.go (2)
Tool(15-30)ToolRegistry(10-12)toolchain/tool_versions.go (1)
LoadToolVersions(22-48)toolchain/aqua_registry.go (1)
NewAquaRegistry(32-40)toolchain/http_client.go (1)
NewDefaultHTTPClient(68-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 (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Lint (golangci)
- GitHub Check: autofix
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Summary
Comment on lines
+522
to
+529
| // Determine the binary name | ||
| binaryName := tool.Name | ||
| if binaryName == "" { | ||
| binaryName = tool.RepoName | ||
| } | ||
|
|
||
| binaryPath := filepath.Join(versionDir, binaryName) | ||
|
|
There was a problem hiding this comment.
Fix Windows binary naming (.exe) for install and lookup.
Currently we install without .exe and then cannot find/execute on Windows.
@@
- // Determine the binary name
- binaryName := tool.Name
+ // Determine the binary name
+ binaryName := tool.Name
if binaryName == "" {
binaryName = tool.RepoName
}
+ if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") {
+ binaryName += ".exe"
+ }
@@
- // Determine the binary name (use repo name as default)
- binaryName := repo
+ // Determine the binary name (use repo name as default)
+ binaryName := repo
@@
}
+ if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") {
+ binaryName += ".exe"
+ }
return filepath.Join(i.binDir, owner, repo, version, binaryName)
@@
- // Determine the binary name (use repo name as default)
- binaryName := repo
+ // Determine the binary name (use repo name as default)
+ binaryName := repo
@@
}
+ if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") {
+ binaryName += ".exe"
+ }
alternativePath := filepath.Join(i.binDir, version, binaryName)Also applies to: 752-767, 836-849
🤖 Prompt for AI Agents
In toolchain/installer.go around lines 522-529 (and similarly adjust 752-767 and
836-849), the code sets binaryName from tool.Name or tool.RepoName but does not
append the Windows executable suffix; update the logic to detect runtime.GOOS ==
"windows" and, when true, ensure binaryName ends with ".exe" (add the suffix if
missing) before computing binaryPath and before any lookup/installation paths in
the other ranges, so installs and lookups use the .exe filename on Windows.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
toolchain/installer.go (6)
3-21: Modernize imports: drop ioutil; add archive/tar, errors (and sha256 for cache keys).Replace deprecated io/ioutil and prepare for pure‑Go tar extraction and 404 sentinel + hashed cache keys.
import ( + "archive/tar" "archive/zip" "compress/gzip" + "crypto/sha256" "fmt" "io" - "io/ioutil" + "errors" "net/http" "os" "os/exec" "path/filepath" "runtime" "strings" "text/template" "time" ) + +var ( + // ErrHTTPNotFound signals remote asset responded with HTTP 404. + ErrHTTPNotFound = errors.New("http 404") +)
23-27: Godoc punctuation.End the first sentence with a period to satisfy godot.
-// ToolResolver defines an interface for resolving tool names to owner/repo pairs +// ToolResolver defines an interface for resolving tool names to owner/repo pairs.
509-512: Use errors.Is with sentinel instead of string contains.-func isHTTP404(err error) bool { - return strings.Contains(err.Error(), "HTTP 404") -} +func isHTTP404(err error) bool { + return errors.Is(err, ErrHTTPNotFound) +}
582-587: Replace ioutil.TempDir with os.MkdirTemp.io/ioutil is deprecated.
- tempDir, err := ioutil.TempDir("", "installer-extract-") + tempDir, err := os.MkdirTemp("", "installer-extract-")
878-894: Alt path lookup needs .exe handling too.alternativePath := filepath.Join(i.binDir, version, binaryName) + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + alternativePath = filepath.Join(i.binDir, version, binaryName+".exe") + }
974-976: Add godoc for exported GetResolver.+// GetResolver returns the ToolResolver used by this Installer. func (i *Installer) GetResolver() ToolResolver { return i.resolver }
📜 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)
toolchain/installer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
**/*.go: All comments in Go code must end with periods (enforced by golangci-lint godot).
Wrap all returned errors using static errors from the errors package; never return dynamic errors directly.
Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Separate structured logging from UI output: use stderr for prompts/errors to user; stdout only for data; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString and never capture user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Files:
toolchain/installer.go
🧠 Learnings (21)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*.go : Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to pkg/utils/file_utils.go : Prefer existing file operation utilities in pkg/utils/file_utils.go before adding new ones.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document all exported functions, types, and methods
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow Go's documentation conventions
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T16:51:37.868Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:69-97
Timestamp: 2024-12-13T16:51:37.868Z
Learning: In `pkg/config/cache.go`, the function `SaveCache2` is currently unused because it does not work properly on Windows and will be addressed later.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to cmd/*.go : Include troubleshooting hints when appropriate
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
PR: cloudposse/atmos#896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
🧬 Code graph analysis (1)
toolchain/installer.go (6)
toolchain/local_config.go (2)
NewLocalConfigManager(43-45)LocalConfigManager(38-40)toolchain/setup.go (3)
GetToolsConfigFilePath(43-48)GetToolsDirPath(35-40)GetToolVersionsFilePath(27-32)toolchain/types.go (3)
Tool(15-30)ToolRegistry(10-12)File(33-36)toolchain/tool_versions.go (1)
LoadToolVersions(22-48)toolchain/aqua_registry.go (1)
NewAquaRegistry(32-40)toolchain/http_client.go (1)
NewDefaultHTTPClient(68-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). (6)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (10)
toolchain/installer.go (10)
311-330: Version templating flexibility was discussed previously.Leaving as‑is per maintainer decision; no action.
373-392: Same as above for GH asset templating.
693-699: Also replace ioutil.TempDir above if you keep a temp dir path.Please ensure no remaining references to ioutil.TempDir exist in this file.
522-529: Windows install path missing .exe suffix.Binary is renamed without extension, breaking execution on Windows.
binaryName := tool.Name if binaryName == "" { binaryName = tool.RepoName } + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + binaryName += ".exe" + } binaryPath := filepath.Join(versionDir, binaryName)
795-810: Ensure .exe in computed binary path (lookup).Finds/executes will fail on Windows without the suffix.
func (i *Installer) getBinaryPath(owner, repo, version string) string { // Determine the binary name (use repo name as default) binaryName := repo @@ if lcm := i.getLocalConfigManager(); lcm != nil { if toolConfig, exists := lcm.GetToolConfig(fmt.Sprintf("%s/%s", owner, repo)); exists { if toolConfig.BinaryName != "" { binaryName = toolConfig.BinaryName } } } + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + binaryName += ".exe" + } return filepath.Join(i.binDir, owner, repo, version, binaryName) }
60-80: Use XDG cache helpers (team decision) instead of ~/.cache/tools-cache.Aligns with finalized XDG implementation (GetXDGCacheDir/GetXDGTempCacheDir).
func NewInstallerWithResolver(resolver ToolResolver) *Installer { - homeDir, err := os.UserHomeDir() - if err != nil { - log.Warn("Falling back to temp dir for cache.", "error", err) - homeDir = os.TempDir() - } - cacheDir := filepath.Join(homeDir, ".cache", "tools-cache") + cacheDir := GetXDGCacheDir() + if cacheDir == "" { + log.Warn("XDG cache dir not set; using temp XDG cache.") + cacheDir = GetXDGTempCacheDir() + } binDir := filepath.Join(GetToolsDirPath(), "bin")
478-507: Tighten fallback flow with sentinel 404 — verification requireddownloadAsset returns ErrHTTPNotFound; fallback logic is fine. The prior rg failed (wrong arg ordering). Re-run to confirm no brittle "HTTP 404" string checks remain:
#!/bin/bash set -euo pipefail rg -n -e 'HTTP 404' -e '404 Not Found' || echo "OK: no brittle 404 checks"
812-818: executeBinary is a stub; tool never runs.Wire up execution (std{in,out,err} pass‑through).
func (i *Installer) executeBinary(binaryPath string, args []string) error { - // This would use os/exec to run the binary - // For now, just print what would be executed - log.Debug("Would execute binary", "path", binaryPath, "args", args) - return nil + cmd := exec.Command(binaryPath, args...) + cmd.Stdout, cmd.Stderr, cmd.Stdin = os.Stdout, os.Stderr, os.Stdin + if err := cmd.Run(); err != nil { + return fmt.Errorf("execute %s: %w", filepath.Base(binaryPath), err) + } + return nil }
431-476: Make downloads cache‑safe and atomic; return sentinel on 404.Avoid collisions, partial files, and brittle 404 checks.
// Extract filename from URL parts := strings.Split(url, "/") filename := parts[len(parts)-1] - cachePath := filepath.Join(i.cacheDir, filename) + sum := sha256.Sum256([]byte(url)) + cachePath := filepath.Join(i.cacheDir, fmt.Sprintf("%x-%s", sum[:8], filename)) @@ client := NewDefaultHTTPClient() resp, err := client.Get(url) @@ - if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode) - } + if resp.StatusCode != http.StatusOK { + if resp.StatusCode == http.StatusNotFound { + return "", fmt.Errorf("%w", ErrHTTPNotFound) + } + return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode) + } @@ - // Create the file - file, err := os.Create(cachePath) + // Write to a temp file then atomically move into place. + tmp, err := os.CreateTemp(i.cacheDir, ".dl-*") if err != nil { - return "", fmt.Errorf("failed to create cache file: %w", err) + return "", fmt.Errorf("failed to create temp cache file: %w", err) } - defer file.Close() + defer func() { _ = tmp.Close(); _ = os.Remove(tmp.Name()) }() @@ - _, err = io.Copy(file, resp.Body) - if err != nil { - return "", fmt.Errorf("failed to write cache file: %w", err) - } + if _, err = io.Copy(tmp, resp.Body); err != nil { + return "", fmt.Errorf("failed to write temp cache file: %w", err) + } + if err := tmp.Close(); err != nil { + return "", fmt.Errorf("failed to close temp cache file: %w", err) + } + if err := os.Rename(tmp.Name(), cachePath); err != nil { + return "", fmt.Errorf("failed to move temp cache file into place: %w", err) + }
3-976: Fix portability: replace ioutil.TempDir and stop relying on externaltar
- Replace ioutil.TempDir usages (toolchain/installer.go:582, 693) with os.MkdirTemp and remove the deprecated "io/ioutil" import.
- Remove/replace the external extraction call exec.Command("tar", ...) (toolchain/installer.go:699–701). Implement pure-Go tar.gz extraction (archive/tar + compress/gzip) or at minimum guard with exec.LookPath("tar") and provide a clear fallback/error on Windows and minimal containers.
- Windows .exe handling is present (toolchain/installer.go:604–605 and 715–716) — ensure the new extraction preserves .exe filenames and sets executable bits appropriately on non-Windows platforms.
Comment on lines
+262
to
+270
| data, err := os.ReadFile(filePath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var toolToolRegistry ToolRegistry | ||
| if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap returned errors with context.
Follow error‑wrapping guideline for better diagnostics.
- data, err := os.ReadFile(filePath)
- if err != nil {
- return nil, err
- }
+ data, err := os.ReadFile(filePath)
+ if err != nil {
+ return nil, fmt.Errorf("read tool file %s: %w", filePath, err)
+ }
@@
- if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil {
- return nil, err
- }
+ if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil {
+ return nil, fmt.Errorf("unmarshal tool file %s: %w", filePath, err)
+ }🤖 Prompt for AI Agents
In toolchain/installer.go around lines 262 to 270 the errors returned from
os.ReadFile and yaml.Unmarshal lack context; wrap both errors with descriptive
messages (e.g., using fmt.Errorf("read tool registry %s: %w", filePath, err) and
fmt.Errorf("unmarshal tool registry %s: %w", filePath, err)) so callers get
file/path and operation context while preserving the original error with %w.
samtholiya
force-pushed
the
tools-experiment
branch
from
1357ecc to
cd75310
Compare
samtholiya
force-pushed
the
tools-experiment
branch
from
cd75310 to
842200c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
toolchain/installer.go (3)
745-766: Make MoveFile fallback more reliable across OSes.Relying only on EXDEV misses Windows’ ERROR_NOT_SAME_DEVICE. Fall back to copy+remove when rename fails and succeeds to copy.
- if err := os.Rename(src, dst); err != nil { - // On Windows, cross-volume move returns syscall.ERROR_NOT_SAME_DEVICE (0x11) - if errno, ok := err.(*os.LinkError).Err.(syscall.Errno); ok && errno == syscall.EXDEV { - // fallback: copy + remove - if err := copyFile(src, dst); err != nil { - return fmt.Errorf("failed to copy during move fallback: %w", err) - } - if err := os.Remove(src); err != nil { - return fmt.Errorf("failed to remove source after copy: %w", err) - } - return nil - } - return fmt.Errorf("failed to rename file: %w", err) - } + if err := os.Rename(src, dst); err != nil { + // Generic, portable fallback: try copy+remove if rename fails. + if err := copyFile(src, dst); err != nil { + return fmt.Errorf("failed to copy during move fallback: %w", err) + } + if err := os.Remove(src); err != nil { + return fmt.Errorf("failed to remove source after copy: %w", err) + } + return nil + }
1021-1023: Add doc comment for exported method.Required by lint rules.
+// GetResolver returns the ToolResolver used by this Installer. func (i *Installer) GetResolver() ToolResolver { return i.resolver }
818-840: Deduplicate copy helpers (optional).There are two copyFile implementations (method and package-level). Consider consolidating to one to reduce maintenance.
Also applies to: 768-787
📜 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)
toolchain/installer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
**/*.go: All comments in Go code must end with periods (enforced by golangci-lint godot).
Wrap all returned errors using static errors from the errors package; never return dynamic errors directly.
Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Separate structured logging from UI output: use stderr for prompts/errors to user; stdout only for data; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString and never capture user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Files:
toolchain/installer.go
🧠 Learnings (22)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*.go : Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to pkg/utils/file_utils.go : Prefer existing file operation utilities in pkg/utils/file_utils.go before adding new ones.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document all exported functions, types, and methods
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow Go's documentation conventions
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T16:51:37.868Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:69-97
Timestamp: 2024-12-13T16:51:37.868Z
Learning: In `pkg/config/cache.go`, the function `SaveCache2` is currently unused because it does not work properly on Windows and will be addressed later.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to cmd/*.go : Include troubleshooting hints when appropriate
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
PR: cloudposse/atmos#896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
toolchain/installer.go
🧬 Code graph analysis (1)
toolchain/installer.go (6)
toolchain/local_config.go (2)
NewLocalConfigManager(43-45)LocalConfigManager(38-40)toolchain/setup.go (3)
GetToolsConfigFilePath(43-48)GetToolsDirPath(35-40)GetToolVersionsFilePath(27-32)toolchain/types.go (3)
Tool(15-30)ToolRegistry(10-12)File(33-36)toolchain/tool_versions.go (1)
LoadToolVersions(22-48)toolchain/aqua_registry.go (1)
NewAquaRegistry(32-40)toolchain/http_client.go (1)
NewDefaultHTTPClient(68-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). (9)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (9)
toolchain/installer.go (9)
263-271: Wrap returned errors with context for diagnostics.Bare returns from ReadFile/Unmarshal lose path/operation context.
- data, err := os.ReadFile(filePath) - if err != nil { - return nil, err - } + data, err := os.ReadFile(filePath) + if err != nil { + return nil, fmt.Errorf("read tool file %s: %w", filePath, err) + } @@ - if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil { - return nil, err - } + if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil { + return nil, fmt.Errorf("unmarshal tool file %s: %w", filePath, err) + }
24-26: Fix godot on exported interface comment.End the first sentence with a period.
-// ToolResolver defines an interface for resolving tool names to owner/repo pairs -// This allows for mocking in tests and flexible resolution in production. +// ToolResolver defines an interface for resolving tool names to owner/repo pairs. +// This allows for mocking in tests and flexible resolution in production.
523-531: Fix Windows binary naming; installs won’t run on Windows without .exe.We build/lookup paths without appending “.exe” on Windows, but extraction can find “name.exe”. Result: tools are installed as “name” and are not runnable on Windows. Add the suffix during install and both path helpers.
@@ - // Determine the binary name - binaryName := tool.Name - if binaryName == "" { - binaryName = tool.RepoName - } + // Determine the binary name. + binaryName := tool.Name + if binaryName == "" { + binaryName = tool.RepoName + } + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + binaryName += ".exe" + } @@ func (i *Installer) getBinaryPath(owner, repo, version string) string { // Determine the binary name (use repo name as default) binaryName := repo @@ if lcm := i.getLocalConfigManager(); lcm != nil { if toolConfig, exists := lcm.GetToolConfig(fmt.Sprintf("%s/%s", owner, repo)); exists { if toolConfig.BinaryName != "" { binaryName = toolConfig.BinaryName } } } + + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + binaryName += ".exe" + } return filepath.Join(i.binDir, owner, repo, version, binaryName) } @@ - alternativePath := filepath.Join(i.binDir, version, binaryName) + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + binaryName += ".exe" + } + alternativePath := filepath.Join(i.binDir, version, binaryName)Also applies to: 842-857, 925-945
690-741: Stop shelling out to tar; implement pure-Go tar.gz extraction.External “tar” breaks on Windows and in minimal containers. Use archive/tar + gzip.
func (i *Installer) extractTarGz(tarPath, binaryPath string, tool *Tool) error { - log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath)) - - tempDir, err := ioutil.TempDir("", "installer-extract-") - if err != nil { - return fmt.Errorf("failed to create temp dir: %w", err) - } - defer os.RemoveAll(tempDir) - - cmd := exec.Command("tar", "-xzf", tarPath, "-C", tempDir) - if output, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("failed to extract tar.gz: %w, output: %s", err, string(output)) - } - - binaryName := tool.Name - if binaryName == "" { - binaryName = tool.RepoName - } - - // Find the binary in the temp dir (recursively) - var found string - err = filepath.Walk(tempDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if info.Mode().IsRegular() && (info.Name() == binaryName || info.Name() == binaryName+".exe") { - found = path - return filepath.SkipDir - } - return nil - }) - if err != nil { - return fmt.Errorf("failed to search extracted files: %w", err) - } - if found == "" { - return fmt.Errorf("binary %s not found in extracted archive", binaryName) - } - - // Ensure the destination directory exists - dir := filepath.Dir(binaryPath) - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("failed to create destination directory: %w", err) - } - - // Move the binary into place - if err := MoveFile(found, binaryPath); err != nil { - return fmt.Errorf("failed to move extracted binary: %w", err) - } - - return nil + log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath)) + f, err := os.Open(tarPath) + if err != nil { + return fmt.Errorf("failed to open tar.gz: %w", err) + } + defer f.Close() + gz, err := gzip.NewReader(f) + if err != nil { + return fmt.Errorf("failed to create gzip reader: %w", err) + } + defer gz.Close() + tr := tar.NewReader(gz) + + want := tool.Name + if want == "" { + want = tool.RepoName + } + if runtime.GOOS == "windows" && !strings.HasSuffix(want, ".exe") { + want += ".exe" + } + if err := os.MkdirAll(filepath.Dir(binaryPath), 0o755); err != nil { + return fmt.Errorf("failed to create destination directory: %w", err) + } + for { + h, err := tr.Next() + if err == io.EOF { + break + } + if err != nil { + return fmt.Errorf("failed to read tar entry: %w", err) + } + if h.Typeflag != tar.TypeReg { + continue + } + if filepath.Base(h.Name) != want { + continue + } + out, err := os.Create(binaryPath) + if err != nil { + return fmt.Errorf("failed to create destination file: %w", err) + } + if _, err := io.Copy(out, tr); err != nil { + out.Close() + return fmt.Errorf("failed to write binary: %w", err) + } + if err := out.Close(); err != nil { + return fmt.Errorf("failed to close destination file: %w", err) + } + return nil + } + return fmt.Errorf("binary %s not found in extracted archive", want) }
3-17: Modernize imports and add a sentinel for 404s.Needed for pure‑Go tar, atomic caching, and robust 404 detection.
import ( + "archive/tar" "archive/zip" "compress/gzip" + "crypto/sha256" + "encoding/hex" + "errors" "fmt" "io" - "io/ioutil" "net/http" "os" - "os/exec" + "os/exec" "path/filepath" "runtime" "strings" "syscall" "text/template" "time" ) + +var ( + // ErrHTTPNotFound is returned when a remote asset responds with HTTP 404. + ErrHTTPNotFound = errors.New("http 404") +)Also applies to: 23-23
432-477: Make downloads cache‑safe and atomic; detect 404s with sentinel.Current caching collides on filename; writes are non‑atomic; 404 detection is brittle.
@@ - // Extract filename from URL - parts := strings.Split(url, "/") - filename := parts[len(parts)-1] - cachePath := filepath.Join(i.cacheDir, filename) + // Derive a collision-resistant cache filename from the full URL. + parts := strings.Split(url, "/") + filename := parts[len(parts)-1] + sum := sha256.Sum256([]byte(url)) + cachePath := filepath.Join(i.cacheDir, fmt.Sprintf("%s-%s", hex.EncodeToString(sum[:8]), filename)) @@ - if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode) - } + if resp.StatusCode != http.StatusOK { + if resp.StatusCode == http.StatusNotFound { + return "", fmt.Errorf("%w", ErrHTTPNotFound) + } + return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode) + } @@ - // Create the file - file, err := os.Create(cachePath) - if err != nil { - return "", fmt.Errorf("failed to create cache file: %w", err) - } - defer file.Close() - - // Copy the response body to the file - _, err = io.Copy(file, resp.Body) - if err != nil { - return "", fmt.Errorf("failed to write cache file: %w", err) - } - - return cachePath, nil + // Write to a temp file, then atomically rename into place. + tmp, err := os.CreateTemp(i.cacheDir, ".dl-*") + if err != nil { + return "", fmt.Errorf("failed to create temp cache file: %w", err) + } + defer func() { _ = tmp.Close(); _ = os.Remove(tmp.Name()) }() + if _, err = io.Copy(tmp, resp.Body); err != nil { + return "", fmt.Errorf("failed to write temp cache file: %w", err) + } + if err := tmp.Close(); err != nil { + return "", fmt.Errorf("failed to close temp cache file: %w", err) + } + if err := os.Rename(tmp.Name(), cachePath); err != nil { + return "", fmt.Errorf("failed to move temp cache file into place: %w", err) + } + return cachePath, nil @@ -func isHTTP404(err error) bool { - return strings.Contains(err.Error(), "HTTP 404") -} +func isHTTP404(err error) bool { + return errors.Is(err, ErrHTTPNotFound) +}Also applies to: 510-513
61-81: Use XDG cache helpers; path currently diverges from finalized spec.Team decision/docs call for GetXDGCacheDir()/GetXDGTempCacheDir(). Please align.
func NewInstallerWithResolver(resolver ToolResolver) *Installer { - homeDir, err := os.UserHomeDir() - if err != nil { - log.Warn("Falling back to temp dir for cache.", "error", err) - homeDir = os.TempDir() - } - cacheDir := filepath.Join(homeDir, ".cache", "tools-cache") + // XDG Base Directory for toolchain cache. + cacheDir := GetXDGCacheDir() binDir := filepath.Join(GetToolsDirPath(), "bin")
624-627: Use robust move + replace deprecated ioutil.TempDir.Avoid cross-device rename failures and the deprecated API.
- tempDir, err := ioutil.TempDir("", "installer-extract-") + tempDir, err := os.MkdirTemp("", "installer-extract-") @@ - // Move the binary into place - if err := os.Rename(found, binaryPath); err != nil { - return fmt.Errorf("failed to move extracted binary: %w", err) - } + // Move the binary into place (handles cross-device). + if err := MoveFile(found, binaryPath); err != nil { + return fmt.Errorf("failed to move extracted binary: %w", err) + }Also applies to: 583-587
859-865: Implement execution; stub never runs the tool.Hook up os/exec and pass through stdio.
func (i *Installer) executeBinary(binaryPath string, args []string) error { - // This would use os/exec to run the binary - // For now, just print what would be executed - log.Debug("Would execute binary", "path", binaryPath, "args", args) - return nil + cmd := exec.Command(binaryPath, args...) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + if err := cmd.Run(); err != nil { + return fmt.Errorf("failed to execute %s: %w", filepath.Base(binaryPath), err) + } + return nil }
samtholiya
force-pushed
the
tools-experiment
branch
from
842200c to
79d8c0a
Compare
samtholiya
force-pushed
the
tools-experiment
branch
from
79d8c0a to
9ae2b4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
toolchain/installer.go (8)
24-28: Fix GoDoc and godot for ToolResolver.Start with the identifier and end with a period.
Apply:
-// ToolResolver defines an interface for resolving tool names to owner/repo pairs -// This allows for mocking in tests and flexible resolution in production. +// ToolResolver resolves tool names to owner/repo pairs. +// This allows for mocking in tests and flexible resolution in production.
579-630: Replace ioutil.TempDir and use MoveFile to avoid cross-device issues.Apply:
- tempDir, err := ioutil.TempDir("", "installer-extract-") + tempDir, err := os.MkdirTemp("", "installer-extract-") @@ - // Move the binary into place - if err := os.Rename(found, binaryPath); err != nil { - return fmt.Errorf("failed to move extracted binary: %w", err) - } + // Move the binary into place (handles cross-device). + if err := MoveFile(found, binaryPath); err != nil { + return fmt.Errorf("failed to move extracted binary: %w", err) + }
261-271: Wrap errors with context when reading tool YAML.Improves diagnostics and aligns with guidelines.
Apply:
data, err := os.ReadFile(filePath) if err != nil { - return nil, err + return nil, fmt.Errorf("read tool file %s: %w", filePath, err) } @@ if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil { - return nil, err + return nil, fmt.Errorf("unmarshal tool file %s: %w", filePath, err) }
632-688: Harden Unzip against ZipSlip and I/O errors (minor nits).Close files with defer ordering and wrap errors for context.
Apply:
- outFile, err := os.OpenFile(fpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) + outFile, err := os.OpenFile(fpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) if err != nil { - rc.Close() - return err + rc.Close() + return fmt.Errorf("create %s: %w", fpath, err) } - // Copy contents - _, err = io.Copy(outFile, rc) - - // Close files - outFile.Close() - rc.Close() - - if err != nil { - return err - } + _, copyErr := io.Copy(outFile, rc) + closeErr1, closeErr2 := outFile.Close(), rc.Close() + if copyErr != nil { + return fmt.Errorf("copy %s: %w", fpath, copyErr) + } + if closeErr1 != nil { + return fmt.Errorf("close %s: %w", fpath, closeErr1) + } + if closeErr2 != nil { + return fmt.Errorf("close entry %s: %w", f.Name, closeErr2) + }
743-749: Dead code and syscall import; consider removing.isCrossDeviceError is unused; MoveFile doesn’t reference it. Drop both to reduce surface.
Apply:
-func isCrossDeviceError(errno syscall.Errno) bool { - if runtime.GOOS == "windows" { - return errno == 0x11 // ERROR_NOT_SAME_DEVICE - } - return errno == syscall.EXDEV -}And remove syscall from imports if not used elsewhere.
Also applies to: 750-768
303-430: Asset URL version mangling may be too opinionated.Hard-trimming leading "v" limits HTTP templates; consider exposing both .Version and .VersionNoV.
If desired, I can patch templates to include both without breaking current behavior.
983-1026: Add GoDoc for GetResolver.Satisfy linters.
Apply:
+// GetResolver returns the ToolResolver used by this Installer. func (i *Installer) GetResolver() ToolResolver { return i.resolver }
820-842: Duplicate copy helpers (method vs package-level).Two copyFile implementations increase maintenance cost. Prefer one (package-level) and reuse.
I can consolidate to a single helper if you want a quick patch.
Also applies to: 770-789
📜 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)
cmd/toolchain.go(1 hunks)cmd/toolchain_get.go(1 hunks)toolchain/installer.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/toolchain_get.go
- cmd/toolchain.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
**/*.go: All comments in Go code must end with periods (enforced by golangci-lint godot).
Wrap all returned errors using static errors from the errors package; never return dynamic errors directly.
Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Separate structured logging from UI output: use stderr for prompts/errors to user; stdout only for data; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString and never capture user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Files:
toolchain/installer.go
🧠 Learnings (22)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*.go : Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to pkg/utils/file_utils.go : Prefer existing file operation utilities in pkg/utils/file_utils.go before adding new ones.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document all exported functions, types, and methods
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow Go's documentation conventions
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T16:51:37.868Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:69-97
Timestamp: 2024-12-13T16:51:37.868Z
Learning: In `pkg/config/cache.go`, the function `SaveCache2` is currently unused because it does not work properly on Windows and will be addressed later.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to cmd/*.go : Include troubleshooting hints when appropriate
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
PR: cloudposse/atmos#896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
toolchain/installer.go
🧬 Code graph analysis (1)
toolchain/installer.go (6)
toolchain/local_config.go (2)
NewLocalConfigManager(43-45)LocalConfigManager(38-40)toolchain/setup.go (3)
GetToolsConfigFilePath(43-48)GetToolsDirPath(35-40)GetToolVersionsFilePath(27-32)toolchain/types.go (3)
Tool(15-30)ToolRegistry(10-12)File(33-36)toolchain/tool_versions.go (1)
LoadToolVersions(22-48)toolchain/aqua_registry.go (1)
NewAquaRegistry(32-40)toolchain/http_client.go (1)
NewDefaultHTTPClient(68-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). (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (5)
toolchain/installer.go (5)
3-17: Drop deprecated ioutil, add tar/errors; prep for pure-Go extraction.Replace ioutil with os.MkdirTemp; add archive/tar and errors for tar.gz extraction and sentinel errors.
Apply:
import ( + "archive/tar" "archive/zip" "compress/gzip" "fmt" "io" - "io/ioutil" "net/http" "os" "os/exec" "path/filepath" "runtime" "strings" - "syscall" "text/template" "time" + "errors" )
432-477: Fix collision-prone cache keys and other critical installer issues
- Collision-prone cache filenames — toolchain/installer.go:442 (cachePath := filepath.Join(i.cacheDir, filename)). Use a deterministic collision-resistant key (e.g., sha256 of the download URL or include explicit version) instead of the basename.
- Fragile 404 detection — toolchain/installer.go:512 (strings.Contains(err.Error(), "HTTP 404")). Detect HTTP 404 via resp.StatusCode or return/compare a sentinel error, not by matching error strings.
- Replace deprecated ioutil.TempDir — toolchain/installer.go:583,694: use os.MkdirTemp.
- Platform-dependent tar exec — toolchain/installer.go:700 (exec.Command("tar", ...)). Validate tar availability or prefer Go archive/compress libraries for portability.
- Missing Windows .exe handling in getBinaryPath — no Windows branch found; append ".exe" on runtime.GOOS == "windows" when resolving binary paths.
861-867: Implement execution; current stub never runs the tool.Core functionality is missing.
Apply:
func (i *Installer) executeBinary(binaryPath string, args []string) error { - // This would use os/exec to run the binary - // For now, just print what would be executed - log.Debug("Would execute binary", "path", binaryPath, "args", args) - return nil + cmd := exec.Command(binaryPath, args...) + cmd.Stdout, cmd.Stderr, cmd.Stdin = os.Stdout, os.Stderr, os.Stdin + if err := cmd.Run(); err != nil { + return fmt.Errorf("execute %s: %w", filepath.Base(binaryPath), err) + } + return nil }
61-81: Use XDG cache helpers (team decision) instead of ~/.cache/tools-cache.Align with the finalized XDG implementation (GetXDGCacheDir/GetXDGTempCacheDir).
Apply:
func NewInstallerWithResolver(resolver ToolResolver) *Installer { - homeDir, err := os.UserHomeDir() - if err != nil { - log.Warn("Falling back to temp dir for cache.", "error", err) - homeDir = os.TempDir() - } - cacheDir := filepath.Join(homeDir, ".cache", "tools-cache") + // XDG-compliant cache base (e.g. ${XDG_CACHE_HOME}/atmos-toolchain). + cacheDir := GetXDGCacheDir() binDir := filepath.Join(GetToolsDirPath(), "bin")
690-741: Stop shelling out to tar; implement pure-Go tar.gz extraction.External tar isn't portable and violates repo guidelines.
Apply:
func (i *Installer) extractTarGz(tarPath, binaryPath string, tool *Tool) error { log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath)) - - tempDir, err := ioutil.TempDir("", "installer-extract-") - if err != nil { - return fmt.Errorf("failed to create temp dir: %w", err) - } - defer os.RemoveAll(tempDir) - - cmd := exec.Command("tar", "-xzf", tarPath, "-C", tempDir) - if output, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("failed to extract tar.gz: %w, output: %s", err, string(output)) - } - - binaryName := tool.Name - if binaryName == "" { - binaryName = tool.RepoName - } - - // Find the binary in the temp dir (recursively) - var found string - err = filepath.Walk(tempDir, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if info.Mode().IsRegular() && (info.Name() == binaryName || info.Name() == binaryName+".exe") { - found = path - return filepath.SkipDir - } - return nil - }) - if err != nil { - return fmt.Errorf("failed to search extracted files: %w", err) - } - if found == "" { - return fmt.Errorf("binary %s not found in extracted archive", binaryName) - } - - // Ensure the destination directory exists - dir := filepath.Dir(binaryPath) - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("failed to create destination directory: %w", err) - } - - // Move the binary into place - if err := MoveFile(found, binaryPath); err != nil { - return fmt.Errorf("failed to move extracted binary: %w", err) - } - - return nil + want := tool.Name + if want == "" { + want = tool.RepoName + } + if runtime.GOOS == "windows" && !strings.HasSuffix(want, ".exe") { + want += ".exe" + } + f, err := os.Open(tarPath) + if err != nil { return fmt.Errorf("open tar.gz: %w", err) } + defer f.Close() + gz, err := gzip.NewReader(f) + if err != nil { return fmt.Errorf("gzip reader: %w", err) } + defer gz.Close() + tr := tar.NewReader(gz) + if err := os.MkdirAll(filepath.Dir(binaryPath), 0o755); err != nil { + return fmt.Errorf("mkdir dest: %w", err) + } + for { + hdr, err := tr.Next() + if err == io.EOF { break } + if err != nil { return fmt.Errorf("read tar: %w", err) } + if hdr.Typeflag != tar.TypeReg { continue } + if filepath.Base(hdr.Name) != want { continue } + out, err := os.Create(binaryPath) + if err != nil { return fmt.Errorf("create dest: %w", err) } + if _, err := io.Copy(out, tr); err != nil { + out.Close() + return fmt.Errorf("write dest: %w", err) + } + if err := out.Close(); err != nil { return fmt.Errorf("close dest: %w", err) } + return nil + } + return fmt.Errorf("binary %s not found in archive", want) }
Comment on lines
+432
to
+477
| // downloadAsset downloads an asset to the cache directory. | ||
| func (i *Installer) downloadAsset(url string) (string, error) { | ||
| // Create cache directory if it doesn't exist | ||
| if err := os.MkdirAll(i.cacheDir, 0o755); err != nil { | ||
| return "", fmt.Errorf("failed to create cache directory: %w", err) | ||
| } | ||
|
|
||
| // Extract filename from URL | ||
| parts := strings.Split(url, "/") | ||
| filename := parts[len(parts)-1] | ||
| cachePath := filepath.Join(i.cacheDir, filename) | ||
|
|
||
| // Check if already cached | ||
| if _, err := os.Stat(cachePath); err == nil { | ||
| log.Debug("Using cached asset", "filename", filename) | ||
| return cachePath, nil | ||
| } | ||
|
|
||
| // Download the file using authenticated HTTP client | ||
| log.Debug("Downloading asset", "filename", filename) | ||
| client := NewDefaultHTTPClient() | ||
| resp, err := client.Get(url) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to download asset: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode) | ||
| } | ||
|
|
||
| // Create the file | ||
| file, err := os.Create(cachePath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create cache file: %w", err) | ||
| } | ||
| defer file.Close() | ||
|
|
||
| // Copy the response body to the file | ||
| _, err = io.Copy(file, resp.Body) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to write cache file: %w", err) | ||
| } | ||
|
|
||
| return cachePath, nil | ||
| } |
There was a problem hiding this comment.
Make downloads robust: 404 sentinel, hashed cache key, atomic writes.
Avoid brittle string checks, filename collisions, and partial files.
Apply:
+var ErrHTTPNotFound = errors.New("http 404")
+
func (i *Installer) downloadAsset(url string) (string, error) {
// Create cache directory if it doesn't exist
@@
- // Extract filename from URL
- parts := strings.Split(url, "/")
- filename := parts[len(parts)-1]
- cachePath := filepath.Join(i.cacheDir, filename)
+ parts := strings.Split(url, "/")
+ filename := parts[len(parts)-1]
+ sum := sha256.Sum256([]byte(url))
+ cachePath := filepath.Join(i.cacheDir, fmt.Sprintf("%x-%s", sum[:8], filename))
@@
client := NewDefaultHTTPClient()
resp, err := client.Get(url)
@@
- if resp.StatusCode != http.StatusOK {
- return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode)
- }
+ if resp.StatusCode != http.StatusOK {
+ if resp.StatusCode == http.StatusNotFound {
+ return "", fmt.Errorf("%w", ErrHTTPNotFound)
+ }
+ return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode)
+ }
@@
- // Create the file
- file, err := os.Create(cachePath)
+ // Write to a temp file, then atomically rename.
+ tmp, err := os.CreateTemp(i.cacheDir, ".dl-*")
if err != nil {
- return "", fmt.Errorf("failed to create cache file: %w", err)
+ return "", fmt.Errorf("failed to create temp cache file: %w", err)
}
- defer file.Close()
+ defer func() { _ = tmp.Close(); _ = os.Remove(tmp.Name()) }()
@@
- _, err = io.Copy(file, resp.Body)
- if err != nil {
- return "", fmt.Errorf("failed to write cache file: %w", err)
- }
+ if _, err = io.Copy(tmp, resp.Body); err != nil {
+ return "", fmt.Errorf("failed to write temp cache file: %w", err)
+ }
+ if err := tmp.Close(); err != nil {
+ return "", fmt.Errorf("failed to close temp cache file: %w", err)
+ }
+ if err := os.Rename(tmp.Name(), cachePath); err != nil {
+ return "", fmt.Errorf("failed to move temp cache file into place: %w", err)
+ }
@@
-func isHTTP404(err error) bool {
- return strings.Contains(err.Error(), "HTTP 404")
-}
+func isHTTP404(err error) bool { return errors.Is(err, ErrHTTPNotFound) }Note: add missing imports for sha256/fmt if not already present (fmt is present).
Also applies to: 479-508, 510-513
🤖 Prompt for AI Agents
In toolchain/installer.go around lines 432 to 477, the downloadAsset
implementation is brittle: it uses raw URL basename (causing filename
collisions), doesn't handle 404 sentinel bodies, and writes files directly
(risking partial/corrupt cache). Fix by hashing the URL (sha256) and using the
hex hash plus original filename for a deterministic cache key; detect HTTP 404
by treating StatusNotFound or a 404-like response body as an error and return a
clear sentinel error; perform atomic writes by downloading to a temp file in the
same directory (e.g., cachePath + ".tmp.<pid>") and fsync/close then rename to
final cachePath; ensure proper cleanup of temp files on errors; keep the cache
directory creation and cached-hit check but check for final file only; add
missing imports for crypto/sha256 and encoding/hex and ensure fmt is imported.
Comment on lines
+523
to
+531
| // Determine the binary name | ||
| binaryName := tool.Name | ||
| if binaryName == "" { | ||
| binaryName = tool.RepoName | ||
| } | ||
|
|
||
| binaryPath := filepath.Join(versionDir, binaryName) | ||
|
|
||
| // For now, just copy the file (simplified extraction) |
There was a problem hiding this comment.
Fix Windows install/lookup: preserve .exe in paths.
Without appending .exe, install/Run fail on Windows.
Apply:
// Determine the binary name
binaryName := tool.Name
if binaryName == "" {
binaryName = tool.RepoName
}
+if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") {
+ binaryName += ".exe"
+}
@@
func (i *Installer) getBinaryPath(owner, repo, version string) string {
// Determine the binary name (use repo name as default)
binaryName := repo
@@
if lcm := i.getLocalConfigManager(); lcm != nil {
if toolConfig, exists := lcm.GetToolConfig(fmt.Sprintf("%s/%s", owner, repo)); exists {
if toolConfig.BinaryName != "" {
binaryName = toolConfig.BinaryName
}
}
}
+ if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") {
+ binaryName += ".exe"
+ }
return filepath.Join(i.binDir, owner, repo, version, binaryName)
}
@@
// Determine the binary name (use repo name as default)
binaryName := repo
// Try to get binary name from configuration
if lcm := i.getLocalConfigManager(); lcm != nil {
if toolConfig, exists := lcm.GetToolConfig(fmt.Sprintf("%s/%s", owner, repo)); exists {
if toolConfig.BinaryName != "" {
binaryName = toolConfig.BinaryName
}
}
}
+ if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") {
+ binaryName += ".exe"
+ }
alternativePath := filepath.Join(i.binDir, version, binaryName)Also applies to: 844-859, 927-943
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
toolchain/installer.go (4)
743-748: Remove unused isCrossDeviceError and syscall dependency.Dead code and unnecessary syscall usage; drop function and import.
-func isCrossDeviceError(errno syscall.Errno) bool { - if runtime.GOOS == "windows" { - return errno == 0x11 // ERROR_NOT_SAME_DEVICE - } - return errno == syscall.EXDEV -}
263-271: Wrap file I/O and YAML errors with context.data, err := os.ReadFile(filePath) if err != nil { - return nil, err + return nil, fmt.Errorf("read tool file %s: %w", filePath, err) } @@ - if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil { - return nil, err + if err := yaml.Unmarshal(data, &toolToolRegistry); err != nil { + return nil, fmt.Errorf("unmarshal tool file %s: %w", filePath, err) }
1023-1026: Add godoc for exported GetResolver (godot).+// GetResolver returns the ToolResolver in use. func (i *Installer) GetResolver() ToolResolver { return i.resolver }
820-842: Avoid duplicate “copyFile” identifiers (func vs method).Rename one to reduce confusion during code search and reviews.
Also applies to: 770-789
📜 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)
toolchain/installer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults
**/*.go: All comments in Go code must end with periods (enforced by golangci-lint godot).
Wrap all returned errors using static errors from the errors package; never return dynamic errors directly.
Always bind environment variables with viper.BindEnv() and provide ATMOS_ alternatives for each external var.
Separate structured logging from UI output: use stderr for prompts/errors to user; stdout only for data; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd or telemetry.CaptureCmdString and never capture user data.
Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Files:
toolchain/installer.go
🧠 Learnings (22)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to **/*.go : Ensure cross-platform compatibility: prefer SDKs to external binaries, use filepath/os/runtime for portability, and add build constraints when needed.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-07T15:13:17.831Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T15:13:17.831Z
Learning: Applies to pkg/utils/file_utils.go : Prefer existing file operation utilities in pkg/utils/file_utils.go before adding new ones.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document all exported functions, types, and methods
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow Go's documentation conventions
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
toolchain/installer.go
📚 Learning: 2024-12-13T16:51:37.868Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: pkg/config/cache.go:69-97
Timestamp: 2024-12-13T16:51:37.868Z
Learning: In `pkg/config/cache.go`, the function `SaveCache2` is currently unused because it does not work properly on Windows and will be addressed later.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to cmd/*.go : Include troubleshooting hints when appropriate
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:21.906Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-07T20:38:09.618Z
Learnt from: samtholiya
PR: cloudposse/atmos#896
File: cmd/editor_config.go:37-40
Timestamp: 2025-01-07T20:38:09.618Z
Learning: Error handling suggestion for `cmd.Help()` in `cmd/editor_config.go` was deferred as the code is planned for future modifications.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
toolchain/installer.go
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
toolchain/installer.go
📚 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:
toolchain/installer.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Wrap errors with context using `fmt.Errorf("context: %w", err)`
Applied to files:
toolchain/installer.go
🧬 Code graph analysis (1)
toolchain/installer.go (6)
toolchain/local_config.go (2)
NewLocalConfigManager(43-45)LocalConfigManager(38-40)toolchain/setup.go (3)
GetToolsConfigFilePath(43-48)GetToolsDirPath(35-40)GetToolVersionsFilePath(27-32)toolchain/types.go (3)
Tool(15-30)ToolRegistry(10-12)File(33-36)toolchain/tool_versions.go (1)
LoadToolVersions(22-48)toolchain/aqua_registry.go (1)
NewAquaRegistry(32-40)toolchain/http_client.go (1)
NewDefaultHTTPClient(68-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). (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: website-deploy-preview
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (11)
toolchain/installer.go (11)
431-508: Optional: version templating ergonomics (raw + no‑v).Expose both Version and VersionNoV in templates to avoid hard trimming.
3-22: Drop deprecated/unused imports; add required ones for fixes.Replace ioutil/syscall, add archive/tar, crypto/sha256, and errors. This unblocks pure‑Go extraction, atomic downloads, and sentinel errors.
import ( - "archive/zip" + "archive/tar" + "archive/zip" "compress/gzip" + "crypto/sha256" + "errors" "fmt" "io" - "io/ioutil" "net/http" "os" - "os/exec" + "os/exec" "path/filepath" "runtime" "strings" - "syscall" "text/template" "time" )Add a sentinel 404 error for robust detection (outside this hunk):
// Sentinel errors. var ErrHTTPNotFound = errors.New("http 404")
861-867: Implement execution; stub currently does nothing.func (i *Installer) executeBinary(binaryPath string, args []string) error { - // This would use os/exec to run the binary - // For now, just print what would be executed - log.Debug("Would execute binary", "path", binaryPath, "args", args) - return nil + cmd := exec.Command(binaryPath, args...) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + if err := cmd.Run(); err != nil { + return fmt.Errorf("failed to execute %s: %w", filepath.Base(binaryPath), err) + } + return nil }
523-531: Windows: preserve .exe in installed filename.Without this, execution on Windows fails.
// Determine the binary name binaryName := tool.Name if binaryName == "" { binaryName = tool.RepoName } + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + binaryName += ".exe" + } binaryPath := filepath.Join(versionDir, binaryName)
844-859: Windows: ensure .exe suffix in getBinaryPath.// Determine the binary name (use repo name as default) binaryName := repo @@ if lcm := i.getLocalConfigManager(); lcm != nil { if toolConfig, exists := lcm.GetToolConfig(fmt.Sprintf("%s/%s", owner, repo)); exists { if toolConfig.BinaryName != "" { binaryName = toolConfig.BinaryName } } } + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + binaryName += ".exe" + } return filepath.Join(i.binDir, owner, repo, version, binaryName)
927-943: Windows: ensure .exe when checking alternative path.// Determine the binary name (use repo name as default) binaryName := repo @@ if lcm := i.getLocalConfigManager(); lcm != nil { if toolConfig, exists := lcm.GetToolConfig(fmt.Sprintf("%s/%s", owner, repo)); exists { if toolConfig.BinaryName != "" { binaryName = toolConfig.BinaryName } } } + if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") { + binaryName += ".exe" + } alternativePath := filepath.Join(i.binDir, version, binaryName)
61-81: Align cache path with finalized XDG helpers.Use GetXDGCacheDir() per team decision/docs; stop hardcoding ~/.cache/tools-cache.
func NewInstallerWithResolver(resolver ToolResolver) *Installer { - homeDir, err := os.UserHomeDir() - if err != nil { - log.Warn("Falling back to temp dir for cache.", "error", err) - homeDir = os.TempDir() - } - cacheDir := filepath.Join(homeDir, ".cache", "tools-cache") + // XDG Base Directory compliance. + cacheDir := GetXDGCacheDir() binDir := filepath.Join(GetToolsDirPath(), "bin") registries := []string{ "https://raw.githubusercontent.com/aquaproj/aqua-registry/main/pkgs", "./tool-registry", }
510-513: Avoid brittle string matching for 404; use errors.Is.-func isHTTP404(err error) bool { - return strings.Contains(err.Error(), "HTTP 404") -} +func isHTTP404(err error) bool { + return errors.Is(err, ErrHTTPNotFound) +}
439-477: Make downloads collision‑proof and atomic; return 404 sentinel.Current cache key collides across URLs and writes are non‑atomic.
// Extract filename from URL parts := strings.Split(url, "/") filename := parts[len(parts)-1] - cachePath := filepath.Join(i.cacheDir, filename) + sum := sha256.Sum256([]byte(url)) + cachePath := filepath.Join(i.cacheDir, fmt.Sprintf("%x-%s", sum[:8], filename)) @@ client := NewDefaultHTTPClient() resp, err := client.Get(url) if err != nil { return "", fmt.Errorf("failed to download asset: %w", err) } defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode) - } + if resp.StatusCode != http.StatusOK { + if resp.StatusCode == http.StatusNotFound { + return "", fmt.Errorf("%w", ErrHTTPNotFound) + } + return "", fmt.Errorf("failed to download asset: HTTP %d", resp.StatusCode) + } @@ - // Create the file - file, err := os.Create(cachePath) + // Write to a temp file, then atomically rename into place. + tmp, err := os.CreateTemp(i.cacheDir, ".dl-*") if err != nil { - return "", fmt.Errorf("failed to create cache file: %w", err) + return "", fmt.Errorf("failed to create temp cache file: %w", err) } - defer file.Close() + defer func() { _ = tmp.Close(); _ = os.Remove(tmp.Name()) }() - // Copy the response body to the file - _, err = io.Copy(file, resp.Body) - if err != nil { - return "", fmt.Errorf("failed to write cache file: %w", err) - } + if _, err = io.Copy(tmp, resp.Body); err != nil { + return "", fmt.Errorf("failed to write temp cache file: %w", err) + } + if err := tmp.Close(); err != nil { + return "", fmt.Errorf("failed to close temp cache file: %w", err) + } + if err := os.Rename(tmp.Name(), cachePath); err != nil { + return "", fmt.Errorf("failed to move temp cache file into place: %w", err) + }
24-26: Fix Go comment punctuation (golangci-lint: godot).Update comments in toolchain/installer.go so every comment line ends with a period to satisfy the godot linter.
- Offending lines: 24, 34, 44, 126, 140, 147, 171, 174, 180, 211, 220, 233, 235, 241, 247, 273, 304, 307, 312, 318, 333, 362, 367, 370, 374, 380, 395, 421, 434, 439, 444, 450, 463, 470, 488, 517, 523, 531, 541, 557, 564, 599, 618, 624, 642, 645, 651, 658, 663, 669, 676, 679, 710, 729, 735, 753, 846, 849, 863, 864, 871, 877, 880, 885, 887, 891, 895, 900, 912, 921, 927, 928, 931, 945, 951, 959, 987, 988, 991, 1006.
3-22: Fix deprecated ioutil, external tar/unzip usage, and brittle 404 detection
- toolchain/installer.go:512 — don’t use strings.Contains(err.Error(), "HTTP 404"); check resp.StatusCode == http.StatusNotFound (or return/compare a typed error) instead.
- toolchain/installer.go:583, 694 — replace ioutil.TempDir with os.MkdirTemp.
- toolchain/installer.go:700 — stop spawning system tar; use Go stdlib extraction (archive/tar + compress/gzip or archive/zip) to avoid external unzip/tar dependency.
Comment on lines
+583
to
+588
| tempDir, err := ioutil.TempDir("", "installer-extract-") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create temp dir: %w", err) | ||
| } | ||
| defer os.RemoveAll(tempDir) | ||
|
|
There was a problem hiding this comment.
Replace deprecated ioutil.TempDir with os.MkdirTemp.
- tempDir, err := ioutil.TempDir("", "installer-extract-")
+ tempDir, err := os.MkdirTemp("", "installer-extract-")Also applies to: 694-699
🤖 Prompt for AI Agents
In toolchain/installer.go around lines 583-588 (and similarly at 694-699),
replace the deprecated ioutil.TempDir call with os.MkdirTemp: call
os.MkdirTemp("", "installer-extract-") and keep the existing error handling and
defer os.RemoveAll(tempDir). Update imports to remove "io/ioutil" if no longer
used and ensure "os" is imported. Make the same replacement for the second
occurrence at lines 694-699.
Comment on lines
+690
to
+741
| // extractTarGz extracts a tar.gz file. | ||
| func (i *Installer) extractTarGz(tarPath, binaryPath string, tool *Tool) error { | ||
| log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath)) | ||
|
|
||
| tempDir, err := ioutil.TempDir("", "installer-extract-") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create temp dir: %w", err) | ||
| } | ||
| defer os.RemoveAll(tempDir) | ||
|
|
||
| cmd := exec.Command("tar", "-xzf", tarPath, "-C", tempDir) | ||
| if output, err := cmd.CombinedOutput(); err != nil { | ||
| return fmt.Errorf("failed to extract tar.gz: %w, output: %s", err, string(output)) | ||
| } | ||
|
|
||
| binaryName := tool.Name | ||
| if binaryName == "" { | ||
| binaryName = tool.RepoName | ||
| } | ||
|
|
||
| // Find the binary in the temp dir (recursively) | ||
| var found string | ||
| err = filepath.Walk(tempDir, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if info.Mode().IsRegular() && (info.Name() == binaryName || info.Name() == binaryName+".exe") { | ||
| found = path | ||
| return filepath.SkipDir | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to search extracted files: %w", err) | ||
| } | ||
| if found == "" { | ||
| return fmt.Errorf("binary %s not found in extracted archive", binaryName) | ||
| } | ||
|
|
||
| // Ensure the destination directory exists | ||
| dir := filepath.Dir(binaryPath) | ||
| if err := os.MkdirAll(dir, 0o755); err != nil { | ||
| return fmt.Errorf("failed to create destination directory: %w", err) | ||
| } | ||
|
|
||
| // Move the binary into place | ||
| if err := MoveFile(found, binaryPath); err != nil { | ||
| return fmt.Errorf("failed to move extracted binary: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Stop shelling out to tar; use archive/tar (+ gzip).
Exec’ing tar breaks portability and violates repo guidance.
func (i *Installer) extractTarGz(tarPath, binaryPath string, tool *Tool) error {
- log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath))
-
- tempDir, err := ioutil.TempDir("", "installer-extract-")
- if err != nil {
- return fmt.Errorf("failed to create temp dir: %w", err)
- }
- defer os.RemoveAll(tempDir)
-
- cmd := exec.Command("tar", "-xzf", tarPath, "-C", tempDir)
- if output, err := cmd.CombinedOutput(); err != nil {
- return fmt.Errorf("failed to extract tar.gz: %w, output: %s", err, string(output))
- }
-
- binaryName := tool.Name
- if binaryName == "" {
- binaryName = tool.RepoName
- }
- // Find the binary in the temp dir (recursively)
- var found string
- err = filepath.Walk(tempDir, func(path string, info os.FileInfo, err error) error {
- if err != nil {
- return err
- }
- if info.Mode().IsRegular() && (info.Name() == binaryName || info.Name() == binaryName+".exe") {
- found = path
- return filepath.SkipDir
- }
- return nil
- })
- if err != nil {
- return fmt.Errorf("failed to search extracted files: %w", err)
- }
- if found == "" {
- return fmt.Errorf("binary %s not found in extracted archive", binaryName)
- }
-
- // Ensure the destination directory exists
- dir := filepath.Dir(binaryPath)
- if err := os.MkdirAll(dir, 0o755); err != nil {
- return fmt.Errorf("failed to create destination directory: %w", err)
- }
-
- // Move the binary into place
- if err := MoveFile(found, binaryPath); err != nil {
- return fmt.Errorf("failed to move extracted binary: %w", err)
- }
-
- return nil
+ log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath))
+ f, err := os.Open(tarPath)
+ if err != nil {
+ return fmt.Errorf("failed to open tar.gz: %w", err)
+ }
+ defer f.Close()
+ gz, err := gzip.NewReader(f)
+ if err != nil {
+ return fmt.Errorf("failed to create gzip reader: %w", err)
+ }
+ defer gz.Close()
+ tr := tar.NewReader(gz)
+
+ want := tool.Name
+ if want == "" {
+ want = tool.RepoName
+ }
+ alt := want + ".exe"
+
+ if err := os.MkdirAll(filepath.Dir(binaryPath), 0o755); err != nil {
+ return fmt.Errorf("failed to create destination directory: %w", err)
+ }
+ for {
+ h, err := tr.Next()
+ if err == io.EOF {
+ break
+ }
+ if err != nil {
+ return fmt.Errorf("failed to read tar entry: %w", err)
+ }
+ if h.Typeflag != tar.TypeReg {
+ continue
+ }
+ name := filepath.Base(h.Name)
+ if name != want && name != alt {
+ continue
+ }
+ out, err := os.Create(binaryPath)
+ if err != nil {
+ return fmt.Errorf("failed to create destination file: %w", err)
+ }
+ if _, err := io.Copy(out, tr); err != nil {
+ out.Close()
+ return fmt.Errorf("failed to write binary: %w", err)
+ }
+ if err := out.Close(); err != nil {
+ return fmt.Errorf("failed to close destination file: %w", err)
+ }
+ return nil
+ }
+ return fmt.Errorf("binary %s not found in extracted archive", want)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // extractTarGz extracts a tar.gz file. | |
| func (i *Installer) extractTarGz(tarPath, binaryPath string, tool *Tool) error { | |
| log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath)) | |
| tempDir, err := ioutil.TempDir("", "installer-extract-") | |
| if err != nil { | |
| return fmt.Errorf("failed to create temp dir: %w", err) | |
| } | |
| defer os.RemoveAll(tempDir) | |
| cmd := exec.Command("tar", "-xzf", tarPath, "-C", tempDir) | |
| if output, err := cmd.CombinedOutput(); err != nil { | |
| return fmt.Errorf("failed to extract tar.gz: %w, output: %s", err, string(output)) | |
| } | |
| binaryName := tool.Name | |
| if binaryName == "" { | |
| binaryName = tool.RepoName | |
| } | |
| // Find the binary in the temp dir (recursively) | |
| var found string | |
| err = filepath.Walk(tempDir, func(path string, info os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if info.Mode().IsRegular() && (info.Name() == binaryName || info.Name() == binaryName+".exe") { | |
| found = path | |
| return filepath.SkipDir | |
| } | |
| return nil | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to search extracted files: %w", err) | |
| } | |
| if found == "" { | |
| return fmt.Errorf("binary %s not found in extracted archive", binaryName) | |
| } | |
| // Ensure the destination directory exists | |
| dir := filepath.Dir(binaryPath) | |
| if err := os.MkdirAll(dir, 0o755); err != nil { | |
| return fmt.Errorf("failed to create destination directory: %w", err) | |
| } | |
| // Move the binary into place | |
| if err := MoveFile(found, binaryPath); err != nil { | |
| return fmt.Errorf("failed to move extracted binary: %w", err) | |
| } | |
| return nil | |
| } | |
| // extractTarGz extracts a tar.gz file. | |
| func (i *Installer) extractTarGz(tarPath, binaryPath string, tool *Tool) error { | |
| log.Debug("Extracting tar.gz archive", "filename", filepath.Base(tarPath)) | |
| f, err := os.Open(tarPath) | |
| if err != nil { | |
| return fmt.Errorf("failed to open tar.gz: %w", err) | |
| } | |
| defer f.Close() | |
| gz, err := gzip.NewReader(f) | |
| if err != nil { | |
| return fmt.Errorf("failed to create gzip reader: %w", err) | |
| } | |
| defer gz.Close() | |
| tr := tar.NewReader(gz) | |
| want := tool.Name | |
| if want == "" { | |
| want = tool.RepoName | |
| } | |
| alt := want + ".exe" | |
| if err := os.MkdirAll(filepath.Dir(binaryPath), 0o755); err != nil { | |
| return fmt.Errorf("failed to create destination directory: %w", err) | |
| } | |
| for { | |
| h, err := tr.Next() | |
| if err == io.EOF { | |
| break | |
| } | |
| if err != nil { | |
| return fmt.Errorf("failed to read tar entry: %w", err) | |
| } | |
| if h.Typeflag != tar.TypeReg { | |
| continue | |
| } | |
| name := filepath.Base(h.Name) | |
| if name != want && name != alt { | |
| continue | |
| } | |
| out, err := os.Create(binaryPath) | |
| if err != nil { | |
| return fmt.Errorf("failed to create destination file: %w", err) | |
| } | |
| if _, err := io.Copy(out, tr); err != nil { | |
| out.Close() | |
| return fmt.Errorf("failed to write binary: %w", err) | |
| } | |
| if err := out.Close(); err != nil { | |
| return fmt.Errorf("failed to close destination file: %w", err) | |
| } | |
| return nil | |
| } | |
| return fmt.Errorf("binary %s not found in extracted archive", want) | |
| } |
🤖 Prompt for AI Agents
In toolchain/installer.go around lines 690 to 741, replace the
exec.Command("tar", ...) call with native Go archive extraction: open tarPath,
create a gzip.Reader via compress/gzip, create an archive/tar.Reader, iterate
entries, and when you encounter a regular file whose base name matches
binaryName or binaryName+".exe" write it into the tempDir (create any parent
dirs with os.MkdirAll), preserve file mode (set executable bit if present) and
stop iteration once found; after extraction use MoveFile(found, binaryPath) as
before. Make sure to handle non-gzip input errors, close readers, and return
clear wrapped errors on failure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe the Feature
Atmos should automatically add the packages base-path as a priority search PATH when executing any commands.
Expected Behavior
Packages are automatically installed and available to atmos components, custom commands, and workflows.
When running
atmos terraform planand it depends on[email protected], if not installed it should automatically install it (if it's configured).When running a custom command, that needs
tflint, it should be able to automatically install it.When running a workflow that needs
tfsec, it should automatically install it.Use Case
usekeywordDescribe Ideal Solution
Atmos Commands
Atmos Configuration
The
usekeyword will installatmosat version1.183.1, then callsyscall.Execto replace the current process with the atmos at the correct version, if the current version is not already 1.183.1.Aliases
Aliases map a short name like
opentofuto a registry configuration ofopentofu/opentofuorterraformto something likehashicorp/terraformTool Versions
We should use the simple
.tool-versionsconvention popularized byasdfThe first semver is the default version, when nothing is specified
This file is consulted when calling install/uninstall/add/remove.
We should use aliases in this file to refer to toolchain tools. This is so we can change the mapping in the future.
Aqua Registry Configuration
We're going to add partial Aqua support for registries and expand the implementation over time. To start, we want to support the essential packages we depend on.
Local Configuration
We also want a local toolchain configuration, when not depending on the Aqua registry.
Stack Configuration
When the
commandis evaluated, it's first checked against the toolchain, and if it's not installed, it will be automatically installed.The
PATHfor exec will be updated to use the directory containingterraformfor version1.11.4Workflows
Custom Commands
Alternatives Considered
Using
aquadirectly. However, the aims of the Aqua Project are different than our aims and Aqua does not expose an SDK or stable interfaces for other CLIs to use Aqua.https://github.com/suzuki-shunsuke
Summary by CodeRabbit
New Features
Documentation
Tests
Chores