Repository navigation
refactor: Migrate atmos auth to Command Registry with StandardParser - #1919
Merged
Merged
Conversation
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
December 28, 2025 21:37
Contributor
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Scanned Files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughMoves the auth CLI into a dedicated ChangesAuth CLI + Flag Preprocessing (single cohesive DAG)
Sequence DiagramsequenceDiagram
autonumber
actor User as User
participant CLI as Atmos CLI
participant Pre as preprocessArgs()
participant Parser as Flag Parser
participant Viper as Viper
participant AuthCmd as cmd/auth
participant Manager as AuthManager
participant OS as OS / ChildProc
User->>CLI: atmos auth exec --identity admin -- <cmd>
CLI->>Pre: preprocessArgs(os.Args[1:])
Pre->>Parser: normalize NoOptDefVal flags & separate compatibility args
Parser->>CLI: Cobra parse (RootCmd)
CLI->>AuthCmd: auth subcommand RunE
AuthCmd->>Viper: Bind parsed flags into Viper
AuthCmd->>AuthCmd: GetIdentityFromFlags(cmd) -> identity
AuthCmd->>Manager: CreateAuthManager(BuildConfigAndStacksInfo(cmd,v))
AuthCmd->>Manager: Prepare authenticated env (use cache or Authenticate)
Manager-->>AuthCmd: sanitized env list
AuthCmd->>OS: exec child process with sanitized env
OS-->>User: child process runs / returns exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
cmd/auth/markdown/atmos_auth_console_usage.md (1)
1-59: Good coverage of console options, but missing command prompts.Per project convention, CLI docs use
$ atmos commandformat with the dollar sign prompt. Other auth docs likeatmos_auth_logout_usage.mdfollow this pattern. Consider adding$prefix to commands for consistency.🔎 Example fix for first code block
```shell -atmos auth console +$ atmos auth console</details> </blockquote></details> <details> <summary>cmd/auth/helpers.go (1)</summary><blockquote> `111-112`: **Consider using UI layer for stderr output.** Per coding guidelines, `fmt.Fprintf(os.Stderr, ...)` should be replaced with the UI layer utilities. Line 64 correctly uses `u.PrintfMessageToTUI`, but line 111 uses direct stderr write. <details> <summary>🔎 Suggested fix</summary> ```diff - fmt.Fprintf(os.Stderr, "%s\n\n", t) + u.PrintfMessageToTUI("%s\n\n", t)cmd/auth/completion.go (1)
47-72: Works correctly, minor duplication with identityFlagCompletion.The identity gathering logic (lines 57-69) duplicates lines 19-31. Consider extracting a helper if more completion functions need this pattern.
🔎 Optional: Extract helper function
// getIdentityNames returns sorted identity names from config. func getIdentityNames() ([]string, error) { defer perf.Track(nil, "auth.getIdentityNames")() atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) if err != nil { return nil, err } var identities []string if atmosConfig.Auth.Identities != nil { for name := range atmosConfig.Auth.Identities { identities = append(identities, name) } } sort.Strings(identities) return identities, nil }cmd/auth/console.go (1)
278-293: Consider propagating global flags to InitCliConfig.Per learnings, when calling
cfg.InitCliConfig, you should populateschema.ConfigAndStacksInfowith global flag values usingflags.ParseGlobalFlags(cmd, v)rather than passing an empty struct. This ensures config selection flags (--base-path,--config,--config-path,--profile) are respected.🔎 Suggested approach
func initializeAuthManager() (types.AuthManager, error) { defer perf.Track(nil, "auth.initializeAuthManager")() - atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) + // TODO: Consider accepting cmd and viper to populate ConfigAndStacksInfo + // with global flags for config path resolution. + atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) if err != nil { return nil, fmt.Errorf("%w: failed to load atmos config: %w", errUtils.ErrAuthConsole, err) }cmd/identity_helpers.go (1)
74-88: perf.Track on trivial normalizeIdentityValue may add overhead.Per learnings,
perf.Track()should exclude "trivial accessors/mutators where the tracking overhead would exceed the actual method cost." This function is a simple string comparison and switch. Consider removing the tracking here.🔎 Remove perf tracking from trivial function
func normalizeIdentityValue(value string) string { - defer perf.Track(nil, "cmd.normalizeIdentityValue")() - if value == "" { return "" }cmd/auth/whoami.go (1)
171-183: Same config flag propagation note applies here.Like
initializeAuthManagerin console.go,loadAuthManagerpasses an emptyConfigAndStacksInfo{}. Consider whether global flags should be propagated for config path resolution.cmd/auth/user/configure.go (1)
69-73: Consider using UI layer for stderr output.Per coding guidelines, UI messages should use
pkg/ui/orpkg/utilshelpers (u.PrintfMessageToTUI,u.PrintfMarkdownToTUI) rather than directfmt.Fprintln/fmt.Fprintfto stderr. This maintains consistency with other auth commands in the PR.
📜 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 (42)
cmd/auth.gocmd/auth/auth.gocmd/auth/completion.gocmd/auth/console.gocmd/auth/env.gocmd/auth/exec.gocmd/auth/helpers.gocmd/auth/list.gocmd/auth/login.gocmd/auth/logout.gocmd/auth/markdown/atmos_auth_console_usage.mdcmd/auth/markdown/atmos_auth_list_usage.mdcmd/auth/markdown/atmos_auth_logout_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.mdcmd/auth/shell.gocmd/auth/user/configure.gocmd/auth/user/helpers.gocmd/auth/user/user.gocmd/auth/validate.gocmd/auth/whoami.gocmd/auth_caching_test.gocmd/auth_console_test.gocmd/auth_env.gocmd/auth_env_test.gocmd/auth_exec_test.gocmd/auth_integration_test.gocmd/auth_list_test.gocmd/auth_login.gocmd/auth_login_test.gocmd/auth_logout_test.gocmd/auth_shell.gocmd/auth_shell_test.gocmd/auth_user.gocmd/auth_user_helpers_test.gocmd/auth_user_test.gocmd/auth_validate_test.gocmd/auth_whoami_test.gocmd/auth_workflows_test.gocmd/identity_flag_test.gocmd/identity_helpers.gocmd/root.gowebsite/docs/quick-start/install-atmos.mdx
💤 Files with no reviewable changes (21)
- cmd/auth_console_test.go
- cmd/auth_integration_test.go
- cmd/auth_env_test.go
- cmd/auth_logout_test.go
- cmd/auth_user_helpers_test.go
- cmd/auth_validate_test.go
- cmd/auth_user.go
- cmd/identity_flag_test.go
- cmd/auth_shell_test.go
- cmd/auth_whoami_test.go
- cmd/auth_env.go
- cmd/auth_workflows_test.go
- cmd/auth_login_test.go
- cmd/auth_list_test.go
- cmd/auth_login.go
- cmd/auth.go
- cmd/auth_exec_test.go
- cmd/auth_caching_test.go
- website/docs/quick-start/install-atmos.mdx
- cmd/auth_user_test.go
- cmd/auth_shell.go
🧰 Additional context used
📓 Path-based instructions (2)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: Commands MUST use flags.NewStandardParser() for command-specific flags - NEVER call viper.BindEnv() or viper.BindPFlag() directly
Embed examples from cmd/markdown/*_usage.md using //go:embed and render with utils.PrintfMarkdown()
Files:
cmd/auth/user/user.gocmd/identity_helpers.gocmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/root.gocmd/auth/helpers.gocmd/auth/user/helpers.gocmd/auth/login.gocmd/auth/env.gocmd/auth/user/configure.gocmd/auth/completion.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: Context should be first parameter in functions that accept it
Use I/O layer (pkg/io/) for stream access and UI layer (pkg/ui/) for formatting - use data.Write/Writef/Writeln for stdout, ui.Write/Writef/Writeln for stderr - DO NOT use fmt.Fprintf(os.Stdout/Stderr) or fmt.Println
All comments must end with periods (enforced by godot linter)
Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
All errors MUST be wrapped using static errors defined in errors/errors.go - Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, errors.Is() for error checking
Use go.uber.org/...
Files:
cmd/auth/user/user.gocmd/identity_helpers.gocmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/root.gocmd/auth/helpers.gocmd/auth/user/helpers.gocmd/auth/login.gocmd/auth/env.gocmd/auth/user/configure.gocmd/auth/completion.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
🧠 Learnings (65)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1533
File: pkg/config/load.go:585-637
Timestamp: 2025-09-27T20:50:20.564Z
Learning: In the cloudposse/atmos repository, command merging prioritizes precedence over display ordering. Help commands are displayed lexicographically regardless of internal array order, so the mergeCommandArrays function focuses on ensuring the correct precedence chain (top-level file wins) rather than maintaining specific display order.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Use registry pattern for extensibility - all new commands MUST use command registry pattern via CommandProvider interface in cmd/internal/registry.go
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags in CLI commands
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to cmd/**/*.go : Commands MUST use flags.NewStandardParser() for command-specific flags - NEVER call viper.BindEnv() or viper.BindPFlag() directly
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
cmd/auth/markdown/atmos_auth_logout_usage.mdcmd/auth/markdown/atmos_auth_console_usage.mdcmd/auth/markdown/atmos_auth_list_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.md
📚 Learning: 2025-01-19T15:49:15.593Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 955
File: tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden:0-0
Timestamp: 2025-01-19T15:49:15.593Z
Learning: In future commits, the help text for Atmos CLI commands should be limited to only show component and stack parameters for commands that actually use them. This applies to the example usage section in command help text.
Applied to files:
cmd/auth/markdown/atmos_auth_console_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.md
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 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:
cmd/auth/markdown/atmos_auth_console_usage.mdcmd/root.go
📚 Learning: 2025-12-04T02:40:45.489Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1588
File: website/blog/2025-10-18-auth-tutorials-geodesic-leapp.md:21-21
Timestamp: 2025-12-04T02:40:45.489Z
Learning: In Docusaurus documentation for the cloudposse/atmos repository, page routing uses the `id` field from frontmatter, not the filename. For example, `auth-login.mdx` with `id: login` is accessible at `/cli/commands/auth/login`, not `/cli/commands/auth/auth-login`.
Applied to files:
cmd/auth/markdown/atmos_auth_console_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.md
📚 Learning: 2025-11-07T14:52:55.217Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1761
File: docs/prd/claude-agent-architecture.md:331-439
Timestamp: 2025-11-07T14:52:55.217Z
Learning: In the cloudposse/atmos repository, Claude agents are used as interactive tools, not in automated/headless CI/CD contexts. Agent documentation and patterns should assume synchronous human interaction.
Applied to files:
cmd/auth/markdown/atmos_auth_console_usage.md
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Applied to files:
cmd/auth/user/user.gocmd/identity_helpers.gocmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/auth/helpers.gocmd/auth/login.gocmd/auth/env.gocmd/auth/completion.gocmd/auth/auth.gocmd/auth/list.gocmd/auth/console.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Applied to files:
cmd/auth/user/user.gocmd/auth/shell.gocmd/auth/exec.gocmd/auth/user/helpers.gocmd/auth/login.gocmd/auth/auth.gocmd/auth/console.go
📚 Learning: 2025-09-07T18:07:00.549Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Applied to files:
cmd/auth/user/user.gocmd/identity_helpers.gocmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/root.gocmd/auth/login.gocmd/auth/env.gocmd/auth/completion.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
cmd/auth/user/user.gocmd/auth/validate.gocmd/auth/exec.gocmd/auth/helpers.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
cmd/auth/user/user.gocmd/identity_helpers.gocmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/root.gocmd/auth/helpers.gocmd/auth/user/helpers.gocmd/auth/login.gocmd/auth/env.gocmd/auth/user/configure.gocmd/auth/completion.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
cmd/auth/user/user.gocmd/identity_helpers.gocmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/root.gocmd/auth/helpers.gocmd/auth/user/helpers.gocmd/auth/login.gocmd/auth/env.gocmd/auth/user/configure.gocmd/auth/completion.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2025-12-13T06:07:37.766Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Applied to files:
cmd/auth/markdown/atmos_auth_list_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.md
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to README.md : Update README.md with new commands and features
Applied to files:
cmd/auth/markdown/atmos_auth_list_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.md
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to website/docs/cli/commands/**/*.mdx : All new CLI commands MUST have Docusaurus documentation with frontmatter (title, sidebar_label, sidebar_class_name, id, description), Intro component, Screengrab, Usage section, Arguments/Flags in definition lists, and Examples section
Applied to files:
cmd/auth/markdown/atmos_auth_list_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.md
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
cmd/auth/markdown/atmos_auth_list_usage.md
📚 Learning: 2025-10-13T18:13:54.020Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1622
File: pkg/perf/perf.go:140-184
Timestamp: 2025-10-13T18:13:54.020Z
Learning: In pkg/perf/perf.go, the `trackWithSimpleStack` function intentionally skips ownership checks at call stack depth > 1 to avoid expensive `getGoroutineID()` calls on every nested function. This is a performance optimization for the common single-goroutine execution case (most Atmos commands), accepting the rare edge case of potential metric corruption if multi-goroutine execution occurs at depth > 1. The ~19× performance improvement justifies this trade-off.
Applied to files:
cmd/identity_helpers.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*.go : Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions, use nil if no atmosConfig param
Applied to files:
cmd/identity_helpers.gocmd/auth/user/helpers.gocmd/auth/console.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
cmd/identity_helpers.go
📚 Learning: 2025-11-30T04:16:24.155Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:34-48
Timestamp: 2025-11-30T04:16:24.155Z
Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.
Applied to files:
cmd/identity_helpers.gocmd/auth/user/helpers.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
cmd/identity_helpers.gocmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/auth/login.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
cmd/identity_helpers.gocmd/auth/user/helpers.go
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
cmd/identity_helpers.gocmd/auth/shell.gocmd/auth/exec.gocmd/root.gocmd/auth/env.gocmd/auth/completion.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
cmd/identity_helpers.gocmd/auth/validate.gocmd/auth/exec.gocmd/auth/user/helpers.gocmd/auth/env.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
cmd/identity_helpers.gocmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/auth/console.go
📚 Learning: 2025-02-18T13:18:53.146Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-18T13:18:53.146Z
Learning: Error checking is not required for cobra.Command.RegisterFlagCompletionFunc calls as these are static configurations done at init time.
Applied to files:
cmd/identity_helpers.gocmd/auth/completion.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2024-11-16T17:30:52.893Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 786
File: internal/exec/shell_utils.go:159-162
Timestamp: 2024-11-16T17:30:52.893Z
Learning: For the `atmos terraform shell` command in `internal/exec/shell_utils.go`, input validation for the custom shell prompt is not required, as users will use this as a CLI tool and any issues will impact themselves.
Applied to files:
cmd/auth/shell.gocmd/auth/markdown/atmos_auth_shell_usage.md
📚 Learning: 2025-09-05T14:57:37.360Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 1448
File: cmd/ansible.go:26-28
Timestamp: 2025-09-05T14:57:37.360Z
Learning: The Atmos codebase uses a consistent pattern for commands that delegate to external tools: `PersistentFlags().Bool("", false, doubleDashHint)` where doubleDashHint provides help text about using double dashes to separate Atmos options from native command arguments. This pattern is used across terraform, packer, helmfile, atlantis, aws, and ansible commands.
Applied to files:
cmd/auth/shell.gocmd/auth/exec.gocmd/auth/whoami.go
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
cmd/auth/shell.gocmd/auth/validate.gocmd/root.gocmd/auth/markdown/atmos_auth_shell_usage.md
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
cmd/auth/shell.gocmd/auth/exec.gocmd/root.gocmd/auth/env.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
cmd/auth/shell.gocmd/auth/validate.gocmd/auth/exec.gocmd/auth/env.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to cmd/**/*.go : Commands MUST use flags.NewStandardParser() for command-specific flags - NEVER call viper.BindEnv() or viper.BindPFlag() directly
Applied to files:
cmd/auth/validate.gocmd/auth/exec.gocmd/auth/env.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2025-11-10T03:03:31.505Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Applied to files:
cmd/auth/validate.gocmd/auth/exec.gocmd/auth/login.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/logout.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
cmd/auth/validate.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags in CLI commands
Applied to files:
cmd/auth/validate.gocmd/auth/env.gocmd/auth/auth.gocmd/auth/whoami.gocmd/auth/list.gocmd/auth/logout.go
📚 Learning: 2025-01-09T22:27:25.538Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/validate_stacks.go:20-23
Timestamp: 2025-01-09T22:27:25.538Z
Learning: The validate commands in Atmos can have different help handling implementations. Specifically, validate_component.go and validate_stacks.go are designed to handle help requests differently, with validate_stacks.go including positional argument checks while validate_component.go does not.
Applied to files:
cmd/auth/validate.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
cmd/auth/validate.gocmd/root.gocmd/auth/login.gocmd/auth/env.gocmd/auth/whoami.gocmd/auth/console.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
cmd/auth/validate.go
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
cmd/auth/validate.gocmd/auth/exec.gocmd/root.gocmd/auth/whoami.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*.go : Use viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK") for environment variables - ATMOS_ prefix required
Applied to files:
cmd/auth/validate.gocmd/root.gocmd/auth/env.gocmd/auth/list.gocmd/auth/logout.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
cmd/auth/validate.gocmd/auth/env.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to internal/exec/**/*.go : New configs support Go templating with FuncMap() from internal/exec/template_funcs.go
Applied to files:
cmd/auth/exec.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*.go : Context should be first parameter in functions that accept it
Applied to files:
cmd/auth/exec.gocmd/auth/whoami.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
cmd/auth/exec.gocmd/auth/env.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
cmd/root.gocmd/auth/user/helpers.gocmd/auth/list.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to **/*.go : Use three-group import organization separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), Atmos packages - maintain aliases: cfg, log, u, errUtils
Applied to files:
cmd/root.gocmd/auth/user/helpers.gocmd/auth/console.gocmd/auth/logout.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
cmd/root.gocmd/auth/user/helpers.gocmd/auth/login.gocmd/auth/console.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Use registry pattern for extensibility - all new commands MUST use command registry pattern via CommandProvider interface in cmd/internal/registry.go
Applied to files:
cmd/root.gocmd/auth/auth.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
cmd/auth/user/helpers.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
Applied to files:
cmd/auth/user/helpers.go
📚 Learning: 2025-12-26T06:31:02.244Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T06:31:02.244Z
Learning: Applies to cmd/**/*.go : Embed examples from cmd/markdown/*_usage.md using //go:embed and render with utils.PrintfMarkdown()
Applied to files:
cmd/auth/user/helpers.gocmd/auth/list.gocmd/auth/console.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
cmd/auth/user/helpers.gocmd/auth/logout.go
📚 Learning: 2025-11-30T04:16:01.899Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:50-59
Timestamp: 2025-11-30T04:16:01.899Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` should NOT be added to trivial O(1) getter methods that only return field references or check map lengths (e.g., `GetDeferredValues()`, `HasDeferredValues()`). The guideline to add perf tracking to "all public functions" applies to functions that do meaningful work (I/O, computation, external calls), not to trivial accessors where the tracking overhead would exceed the operation time and pollute performance reports.
Applied to files:
cmd/auth/user/helpers.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
cmd/auth/user/helpers.go
📚 Learning: 2025-01-18T15:15:41.645Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform.go:37-46
Timestamp: 2025-01-18T15:15:41.645Z
Learning: In the atmos CLI, error handling is intentionally structured to use LogErrorAndExit for consistent error display, avoiding Cobra's default error handling to prevent duplicate error messages.
Applied to files:
cmd/auth/login.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Applied to files:
cmd/auth/env.go
📚 Learning: 2025-09-29T15:47:10.908Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/terraform_cli_args_utils.go:64-73
Timestamp: 2025-09-29T15:47:10.908Z
Learning: In the Atmos codebase, viper.BindEnv is required for CLI commands in the cmd/ package, but internal utilities can use os.Getenv directly when parsing environment variables for business logic purposes. The requirement to use viper is specific to the CLI interface layer, not all environment variable access throughout the codebase.
Applied to files:
cmd/auth/env.gocmd/auth/logout.go
📚 Learning: 2024-12-05T22:33:40.955Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 820
File: cmd/list_components.go:53-54
Timestamp: 2024-12-05T22:33:40.955Z
Learning: In the Atmos CLI Go codebase, using `u.LogErrorAndExit` within completion functions is acceptable because it logs the error and exits the command execution.
Applied to files:
cmd/auth/completion.go
📚 Learning: 2025-02-09T14:38:53.443Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 992
File: cmd/cmd_utils.go:0-0
Timestamp: 2025-02-09T14:38:53.443Z
Learning: Error handling for RegisterFlagCompletionFunc in AddStackCompletion is not required as the errors are non-critical for tab completion functionality.
Applied to files:
cmd/auth/completion.go
📚 Learning: 2025-02-07T19:21:38.028Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 992
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-07T19:21:38.028Z
Learning: The cobra.Command.RegisterFlagCompletionFunc method (as of cobra v1.8.1) never returns an error. It only initializes an internal map and stores the completion function, always returning nil. Error handling for this method call is unnecessary.
Applied to files:
cmd/auth/completion.gocmd/auth/list.go
📚 Learning: 2025-09-09T02:14:36.708Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: internal/auth/types/whoami.go:14-15
Timestamp: 2025-09-09T02:14:36.708Z
Learning: The WhoamiInfo struct in internal/auth/types/whoami.go requires the Credentials field to be JSON-serializable for keystore unmarshaling operations, despite security concerns about credential exposure.
Applied to files:
cmd/auth/whoami.gocmd/auth/console.go
📚 Learning: 2025-10-11T19:04:55.909Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/workflow.go:4-4
Timestamp: 2025-10-11T19:04:55.909Z
Learning: In Go, blank importing the embed package (`import _ "embed"`) is the correct and idiomatic way to enable `go:embed` directives. The blank import is required for the compiler to recognize and process `//go:embed` directives. The embed package should not be imported with a named import unless its exported functions are explicitly used, which is rare.
Applied to files:
cmd/auth/list.go
📚 Learning: 2025-12-13T06:19:17.739Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: pkg/http/client.go:0-0
Timestamp: 2025-12-13T06:19:17.739Z
Learning: In pkg/http/client.go, GetGitHubTokenFromEnv intentionally reads viper.GetString("github-token") to honor precedence of the persistent --github-token flag over ATMOS_GITHUB_TOKEN and GITHUB_TOKEN. Do not replace this with os.Getenv in future reviews; using Viper here is by design.
Applied to files:
cmd/auth/list.go
📚 Learning: 2025-02-07T19:21:38.028Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 992
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-07T19:21:38.028Z
Learning: The cobra.Command.RegisterFlagCompletionFunc method never returns an error as it simply stores the completion function in an internal map. Error handling for this method call is unnecessary.
Applied to files:
cmd/auth/list.go
📚 Learning: 2025-08-16T23:33:07.477Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:651-652
Timestamp: 2025-08-16T23:33:07.477Z
Learning: In the cloudposse/atmos Go codebase, ExecuteDescribeDependents expects a pointer to AtmosConfiguration (*schema.AtmosConfiguration), so when calling it with a value returned by cfg.InitCliConfig (which returns schema.AtmosConfiguration), the address-of operator (&) is necessary: ExecuteDescribeDependents(&atmosConfig, ...).
Applied to files:
cmd/auth/console.go
🧬 Code graph analysis (9)
cmd/auth/user/user.go (1)
pkg/perf/perf.go (1)
Track(121-138)
cmd/identity_helpers.go (2)
cmd/auth/auth.go (1)
IdentityFlagSelectValue(19-19)pkg/perf/perf.go (1)
Track(121-138)
cmd/auth/shell.go (5)
pkg/config/config.go (1)
InitCliConfig(28-67)pkg/schema/schema.go (1)
ConfigAndStacksInfo(679-775)internal/exec/shell_utils.go (1)
ExecAuthShellCommand(316-374)cmd/auth/auth.go (3)
GetIdentityFromFlags(109-129)IdentityFlagName(17-17)IdentityFlagSelectValue(19-19)cmd/identity_helpers.go (3)
GetIdentityFromFlags(43-69)IdentityFlagName(17-17)IdentityFlagSelectValue(19-19)
cmd/auth/validate.go (5)
pkg/flags/standard_parser.go (2)
StandardParser(18-22)NewStandardParser(34-40)pkg/perf/perf.go (1)
Track(121-138)pkg/flags/options.go (2)
WithBoolFlag(63-74)WithEnvVars(221-244)pkg/flags/parser.go (1)
GetBool(141-150)pkg/utils/markdown_utils.go (1)
PrintfMarkdown(80-84)
cmd/auth/exec.go (1)
cmd/auth/auth.go (3)
GetIdentityFromFlags(109-129)IdentityFlagName(17-17)IdentityFlagSelectValue(19-19)
cmd/auth/user/configure.go (6)
cmd/auth/user/user.go (1)
AuthUserCmd(10-17)pkg/config/config.go (1)
InitCliConfig(28-67)pkg/schema/schema.go (1)
ConfigAndStacksInfo(679-775)errors/errors.go (3)
ErrWrapFormat(12-12)ErrInvalidAuthConfig(534-534)ErrAwsAuth(543-543)pkg/auth/credentials/store.go (1)
NewCredentialStore(42-46)pkg/auth/types/aws_credentials.go (1)
AWSCredentials(16-24)
cmd/auth/completion.go (3)
pkg/config/config.go (1)
InitCliConfig(28-67)pkg/schema/schema.go (1)
ConfigAndStacksInfo(679-775)pkg/logger/log.go (1)
Trace(14-16)
cmd/auth/list.go (6)
pkg/flags/standard_parser.go (2)
StandardParser(18-22)NewStandardParser(34-40)pkg/logger/log.go (1)
Trace(14-16)pkg/config/config.go (1)
InitCliConfig(28-67)pkg/schema/schema.go (1)
ConfigAndStacksInfo(679-775)errors/errors.go (1)
ErrInvalidAuthConfig(534-534)cmd/auth/helpers.go (1)
CreateAuthManager(29-36)
cmd/auth/console.go (4)
pkg/flags/standard_parser.go (2)
StandardParser(18-22)NewStandardParser(34-40)pkg/auth/types/interfaces.go (2)
ConsoleURLOptions(363-380)ConsoleAccessProvider(353-360)cmd/auth/helpers.go (1)
CreateAuthManager(29-36)cmd/auth/auth.go (2)
GetIdentityFromFlags(109-129)IdentityFlagName(17-17)
🪛 GitHub Check: CodeQL
cmd/auth/env.go
[failure] 185-185: Clear-text logging of sensitive information
Sensitive data returned by an access to passwdParts flows to a logging call.
[failure] 204-204: Clear-text logging of sensitive information
Sensitive data returned by an access to passwdParts flows to a logging call.
🪛 LanguageTool
cmd/auth/markdown/atmos_auth_console_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: - Open console with default identity ```...
(DASH_RULE)
cmd/auth/markdown/atmos_auth_list_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: - List all providers and identities ``` ...
(DASH_RULE)
cmd/auth/markdown/atmos_auth_shell_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: - Launch an interactive shell with authen...
(DASH_RULE)
🪛 markdownlint-cli2 (0.18.1)
cmd/auth/markdown/atmos_auth_shell_usage.md
3-3: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
4-4: Dollar signs used before commands without showing output
(MD014, commands-show-output)
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
10-10: Dollar signs used before commands without showing output
(MD014, commands-show-output)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
16-16: Dollar signs used before commands without showing output
(MD014, commands-show-output)
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
22-22: Dollar signs used before commands without showing output
(MD014, commands-show-output)
27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Dollar signs used before commands without showing output
(MD014, commands-show-output)
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
34-34: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
Erik Osterman (Cloud Posse) (osterman)
added
the
no-release
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (71.22%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1919 +/- ##
==========================================
+ Coverage 78.12% 78.16% +0.03%
==========================================
Files 1094 1098 +4
Lines 103377 103630 +253
==========================================
+ Hits 80766 81001 +235
+ Misses 18182 18179 -3
- Partials 4429 4450 +21
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
cmd/auth/env.go (1)
174-218: LGTM! Intentional credential output is properly annotated.The CodeQL warnings for lines 192 and 215 are false positives. This command intentionally outputs credentials for shell sourcing (similar to
aws configure export-credentials). The code includes proper annotations acknowledging this behavior.Single-quote escaping is correct, and sorted output ensures consistency.
cmd/auth/shell.go (1)
122-146: Duplicate identity resolution logic.This mirrors
resolveIdentityNameForExecin exec.go. As noted there, consider consolidating into a shared helper.
🧹 Nitpick comments (5)
cmd/auth/exec.go (1)
154-178: Consider extracting shared identity resolution logic.
resolveIdentityNameForExecis nearly identical toresolveIdentityNameForShellin shell.go. Both follow the same flow: check flags → check Viper → handle forceSelect → call GetDefaultIdentity. Consider a shared helper likeResolveIdentityName(cmd, v, authManager)in helpers.go to reduce duplication.cmd/auth/validate.go (1)
79-84: Potential double error display.When validation fails, you print the error message via
PrintfMarkdown(line 82) and then return the same error (line 83). Depending on how the caller handles this, the user might see the error twice. If the parent command already prints returned errors, consider returning a sentinel or nil after printing.🔎 One approach
if err := validator.ValidateAuthConfig(&atmosConfig.Auth); err != nil { u.PrintfMarkdown("**❌ Authentication configuration validation failed:**\n") u.PrintfMarkdown("%s\n", err.Error()) - return err + return nil // Error already displayed to user }cmd/auth/helpers.go (1)
122-123: Direct stderr write instead of UI layer.Per coding guidelines, output should use
ui.Write/Writeffor stderr messages rather thanfmt.Fprintf(os.Stderr, ...). The table rendering could use the UI layer for consistency.🔎 Suggested approach
- fmt.Fprintf(os.Stderr, "%s\n\n", t) + _ = ui.Writef("%s\n\n", t)cmd/auth/list.go (1)
361-367: Double error wrapping pattern.The error format
%w: failed to marshal JSON: %wwraps the sentinel error and then adds a string with another%w. This creates a somewhat unusual error chain. Consider using the standard pattern from errors.go.🔎 Cleaner approach
- return "", fmt.Errorf("%w: failed to marshal JSON: %w", errUtils.ErrParseFile, err) + return "", fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrParseFile, fmt.Errorf("failed to marshal JSON: %w", err))Or simply:
- return "", fmt.Errorf("%w: failed to marshal JSON: %w", errUtils.ErrParseFile, err) + return "", fmt.Errorf("failed to marshal JSON: %w", err)cmd/auth/whoami.go (1)
193-198: Inconsistent Viper instance usage.Line 197 uses global
viper.GetString(IdentityFlagName)while the rest of the function uses the localvvariable. For consistency, usev.GetString(IdentityFlagName).🔎 Suggested fix
// If flag wasn't provided, check Viper for env var fallback. if identityName == "" { - identityName = viper.GetString(IdentityFlagName) + identityName = v.GetString(IdentityFlagName) }Note: The function signature needs
v *viper.Viperparameter added.
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
mergify
Bot
added
conflict
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/auth/whoami.go (1)
207-210: Use%wfor error wrapping to preserve error chain.Line 209 uses
%vwhich loses the error type information. Use%wfor proper error chain preservation, consistent with the pattern used inloadAuthManager.🔎 Suggested fix
defaultIdentity, err := authManager.GetDefaultIdentity(forceSelect) if err != nil { - return "", fmt.Errorf("%w: no default identity configured and no identity specified: %v", errUtils.ErrInvalidAuthConfig, err) + return "", fmt.Errorf("%w: no default identity configured and no identity specified: %w", errUtils.ErrInvalidAuthConfig, err) }
🧹 Nitpick comments (3)
tests/describe_test.go (1)
32-47: Consider using TestKit to avoid RootCmd state pollution.Both tests modify
RootCmdstate viaSetArgsand execute throughRootCmd.Execute(). According to project learnings, tests that modify RootCmd state should usecmd.NewTestKit(t)to ensure proper cleanup between tests.🔎 Suggested pattern with TestKit
func TestDescribeComponentJSON(t *testing.T) { + tk := cmd.NewTestKit(t) skipIfNoTTY(t) // Set up the environment variables. t.Chdir("./fixtures/scenarios/atmos-providers-section") t.Setenv("ATMOS_PAGER", "more") // Use SetArgs for Cobra command testing. - cmd.RootCmd.SetArgs([]string{"describe", "component", "component-1", "--stack", "nonprod", "--format", "json"}) + tk.RootCmd.SetArgs([]string{"describe", "component", "component-1", "--stack", "nonprod", "--format", "json"}) - err := cmd.Execute() + err := tk.RootCmd.Execute() if err != nil { t.Fatalf("Failed to execute command: %v", err) } }Apply the same pattern to
TestDescribeComponentYAML.Based on learnings, as
cmd.NewTestKit(t)handles RootCmd state cleanup automatically.Also applies to: 49-64
cmd/auth/shell.go (2)
77-80: Consider passing ConfigInfo to ValidateAtmosConfig.
ValidateAtmosConfig()is called beforeBuildConfigAndStacksInfo(), so global flags (--base-path,--config, etc.) won't be respected during validation. The actual config loading at line 96 is correct, but validation may check the wrong path.Consider reordering or passing config info:
Suggested fix
+ // Parse global flags and build ConfigAndStacksInfo to honor --base-path, --config, --config-path, --profile. + v := viper.GetViper() + if err := shellParser.BindFlagsToViper(cmd, v); err != nil { + return err + } + configAndStacksInfo := BuildConfigAndStacksInfo(cmd, v) + handleHelpRequest(cmd, args) - if err := internal.ValidateAtmosConfig(); err != nil { + if err := internal.ValidateAtmosConfig(internal.WithConfigInfo(configAndStacksInfo)); err != nil { return err } - - // Bind parsed flags to Viper for precedence. - v := viper.GetViper() - if err := shellParser.BindFlagsToViper(cmd, v); err != nil { - return err - }
174-177: Use sentinel error for shell environment preparation failure.Per coding guidelines, errors should wrap static sentinels from
errors/errors.go. This raw string should use a defined sentinel.Suggested fix
Add sentinel to
errors/errors.goif not present, then:envList, err := authManager.PrepareShellEnvironment(ctx, identityName, baseEnv) if err != nil { - return nil, "", fmt.Errorf("failed to prepare shell environment: %w", err) + return nil, "", fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrShellEnvironmentPreparationFailed, err) }
📜 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 (11)
cmd/auth/shell.gocmd/auth/whoami.gocmd/root.goerrors/errors.gointernal/exec/helmfile.gopkg/flags/global_registry.gopkg/flags/global_registry_test.gotests/cli_describe_component_test.gotests/describe_test.gotests/snapshots/TestCLICommands_atmos_auth_env_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_exec_--help.stdout.golden
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/TestCLICommands_atmos_auth_exec_--help.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/flags/global_registry.go
- errors/errors.go
- internal/exec/helmfile.go
- tests/cli_describe_component_test.go
- tests/snapshots/TestCLICommands_atmos_auth_env_--help.stdout.golden
- pkg/flags/global_registry_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Never delete existing comments without a very strong reason; preserve helpful comments explaining why/how/what/where, and update comments to match code when refactoring
Organize imports in three groups separated by blank lines, sorted alphabetically: (1) Go stdlib, (2) 3rd-party (NOT cloudposse/atmos), (3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Useflags.NewStandardParser()for command-specific flags; NEVER callviper.BindEnv()orviper.BindPFlag()directly (enforced by Forbidigo)
All errors MUST be wrapped using static errors defined inerrors/errors.go; useerrors.Joinfor combining multiple errors,fmt.Errorfwith%wfor adding string context, error builder for complex errors, anderrors.Is()for error checking. Never use dynamic errors directly
Define interfaces for all major funct...
Files:
tests/describe_test.gocmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Usecmd.NewTestKit(t)for cmd tests to auto-clean RootCmd state (flags, args)
Test behavior, not implementation; never test stub functions; avoid tautological tests; useerrors.Is()for error checking; remove always-skipped tests
Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers fromtests/test_preconditions.go
Never manually edit golden snapshot files undertests/test-cases/ortests/testdata/; always use-regenerate-snapshotsflag. Never use pipe redirection when running tests as it breaks TTY detection
Files:
tests/describe_test.go
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function
Commands MUST use the command registry pattern viaCommandProviderinterface; seedocs/prd/command-registry-pattern.mdandcmd/internal/registry.go
Embed examples fromcmd/markdown/*_usage.mdusing//go:embedand render withutils.PrintfMarkdown()
Files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
🧠 Learnings (67)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: When adding new CLI command: (1) Create `cmd/[command]/` with CommandProvider interface, (2) Add blank import to `cmd/root.go`, (3) Implement in `internal/exec/mycommand.go`, (4) Add tests and Docusaurus docs in `website/docs/cli/commands/`, (5) Build website: `cd website && npm run build`. See `docs/developing-atmos-commands.md` and `docs/prd/command-registry-pattern.md`
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to cmd/**/*.go : Commands MUST use the command registry pattern via `CommandProvider` interface; see `docs/prd/command-registry-pattern.md` and `cmd/internal/registry.go`
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags in CLI commands
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:45.831Z
Learning: In cmd/toolchain/get.go, the get subcommand intentionally uses cobra.MaximumNArgs(1) so it works with zero args (list all tools) or one arg (a specific tool). Flags are defined via flags.NewStandardParser() with --all (bool) and --limit (int); no direct viper.BindEnv/BindPFlag calls are used.
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; use `errors.Is()` for error checking; remove always-skipped tests
Applied to files:
tests/describe_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
tests/describe_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Never manually edit golden snapshot files under `tests/test-cases/` or `tests/testdata/`; always use `-regenerate-snapshots` flag. Never use pipe redirection when running tests as it breaks TTY detection
Applied to files:
tests/describe_test.go
📚 Learning: 2024-11-10T18:37:10.032Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_component_utils.go:354-360
Timestamp: 2024-11-10T18:37:10.032Z
Learning: In the vendoring process, a TTY can exist without being interactive. If the process does not prompt the user, we should not require interactive mode to display the TUI. The `CheckTTYSupport` function should check TTY support on stdout rather than stdin.
Applied to files:
tests/describe_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
tests/describe_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers from `tests/test_preconditions.go`
Applied to files:
tests/describe_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
tests/describe_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Use `cmd.NewTestKit(t)` for cmd tests to auto-clean RootCmd state (flags, args)
Applied to files:
tests/describe_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
tests/describe_test.go
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
tests/describe_test.go
📚 Learning: 2024-11-10T19:28:17.365Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_utils.go:0-0
Timestamp: 2024-11-10T19:28:17.365Z
Learning: When TTY is not supported, log the downgrade message at the Warn level using `u.LogWarning(cliConfig, ...)` instead of `fmt.Println`.
Applied to files:
tests/describe_test.go
📚 Learning: 2025-02-19T05:50:35.853Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden:0-0
Timestamp: 2025-02-19T05:50:35.853Z
Learning: Backtick formatting should only be applied to flag descriptions in Go source files, not in golden test files (test snapshots) as they are meant to capture the raw command output.
Applied to files:
tests/describe_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
tests/describe_test.gocmd/auth/shell.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
tests/describe_test.gocmd/auth/shell.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
tests/describe_test.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
tests/describe_test.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
tests/describe_test.go
📚 Learning: 2024-11-25T17:17:15.703Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 797
File: pkg/list/atmos.yaml:213-214
Timestamp: 2024-11-25T17:17:15.703Z
Learning: The file `pkg/list/atmos.yaml` is primarily intended for testing purposes.
Applied to files:
tests/describe_test.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
tests/describe_test.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Stack pipeline: Load atmos.yaml → process imports/inheritance → apply overrides → render templates → generate config. Templates use Go templates + Gomplate with `atmos.Component()`, `!terraform.state`, `!terraform.output`, store integration
Applied to files:
tests/describe_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
tests/describe_test.gocmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
cmd/root.gocmd/auth/shell.go
📚 Learning: 2025-09-07T18:07:00.549Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Applied to files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : Organize imports in three groups separated by blank lines, sorted alphabetically: (1) Go stdlib, (2) 3rd-party (NOT cloudposse/atmos), (3) Atmos packages. Maintain aliases: `cfg`, `log`, `u`, `errUtils`
Applied to files:
cmd/root.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: When adding new CLI command: (1) Create `cmd/[command]/` with CommandProvider interface, (2) Add blank import to `cmd/root.go`, (3) Implement in `internal/exec/mycommand.go`, (4) Add tests and Docusaurus docs in `website/docs/cli/commands/`, (5) Build website: `cd website && npm run build`. See `docs/developing-atmos-commands.md` and `docs/prd/command-registry-pattern.md`
Applied to files:
cmd/root.gocmd/auth/shell.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to cmd/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function
Applied to files:
cmd/root.gocmd/auth/whoami.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Applied to files:
cmd/root.gocmd/auth/shell.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
cmd/root.gocmd/auth/shell.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
cmd/root.gocmd/auth/shell.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Configuration loading precedence: CLI flags → ENV vars → config files → defaults (use Viper). Environment variables require ATMOS_ prefix via `viper.BindEnv("ATMOS_VAR", ...)`
Applied to files:
cmd/root.gocmd/auth/whoami.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
cmd/root.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
cmd/root.gocmd/auth/whoami.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-10-28T01:51:30.811Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
Applied to files:
cmd/root.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to pkg/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function. Use `nil` if no atmosConfig param
Applied to files:
cmd/root.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to pkg/**/*.go : Avoid adding new functions to `pkg/utils/`; create purpose-built packages for new functionality (e.g., `pkg/store/`, `pkg/git/`, `pkg/pro/`, `pkg/filesystem/`)
Applied to files:
cmd/root.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to cmd/**/*.go : Commands MUST use the command registry pattern via `CommandProvider` interface; see `docs/prd/command-registry-pattern.md` and `cmd/internal/registry.go`
Applied to files:
cmd/root.gocmd/auth/whoami.go
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
cmd/root.gocmd/auth/shell.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Applied to files:
cmd/root.gocmd/auth/shell.go
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2025-02-18T13:18:53.146Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1068
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-18T13:18:53.146Z
Learning: Error checking is not required for cobra.Command.RegisterFlagCompletionFunc calls as these are static configurations done at init time.
Applied to files:
cmd/root.gocmd/auth/whoami.go
📚 Learning: 2025-11-10T03:03:31.505Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Applied to files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2025-09-05T14:57:37.360Z
Learnt from: RoseSecurity
Repo: cloudposse/atmos PR: 1448
File: cmd/ansible.go:26-28
Timestamp: 2025-09-05T14:57:37.360Z
Learning: The Atmos codebase uses a consistent pattern for commands that delegate to external tools: `PersistentFlags().Bool("", false, doubleDashHint)` where doubleDashHint provides help text about using double dashes to separate Atmos options from native command arguments. This pattern is used across terraform, packer, helmfile, atlantis, aws, and ansible commands.
Applied to files:
cmd/root.gocmd/auth/shell.go
📚 Learning: 2025-02-07T19:21:38.028Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 992
File: cmd/vendor_pull.go:31-31
Timestamp: 2025-02-07T19:21:38.028Z
Learning: The cobra.Command.RegisterFlagCompletionFunc method (as of cobra v1.8.1) never returns an error. It only initializes an internal map and stores the completion function, always returning nil. Error handling for this method call is unnecessary.
Applied to files:
cmd/root.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
cmd/root.gocmd/auth/whoami.gocmd/auth/shell.go
📚 Learning: 2025-09-09T02:14:36.708Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: internal/auth/types/whoami.go:14-15
Timestamp: 2025-09-09T02:14:36.708Z
Learning: The WhoamiInfo struct in internal/auth/types/whoami.go requires the Credentials field to be JSON-serializable for keystore unmarshaling operations, despite security concerns about credential exposure.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2025-10-22T14:55:44.014Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1695
File: pkg/auth/manager.go:169-171
Timestamp: 2025-10-22T14:55:44.014Z
Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags in CLI commands
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2024-10-20T13:12:46.499Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 736
File: pkg/config/const.go:6-6
Timestamp: 2024-10-20T13:12:46.499Z
Learning: In `cmd/cmd_utils.go`, it's acceptable to have hardcoded references to `atmos.yaml` in logs, and it's not necessary to update them to use the `CliConfigFileName` constant.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2024-12-11T18:46:02.483Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/terraform.go:39-39
Timestamp: 2024-12-11T18:46:02.483Z
Learning: `cliConfig` is initialized in `cmd/root.go` and can be used across the `cmd` package.
Applied to files:
cmd/auth/whoami.go
📚 Learning: 2024-11-16T17:30:52.893Z
Learnt from: pkbhowmick
Repo: cloudposse/atmos PR: 786
File: internal/exec/shell_utils.go:159-162
Timestamp: 2024-11-16T17:30:52.893Z
Learning: For the `atmos terraform shell` command in `internal/exec/shell_utils.go`, input validation for the custom shell prompt is not required, as users will use this as a CLI tool and any issues will impact themselves.
Applied to files:
cmd/auth/shell.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
cmd/auth/shell.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
cmd/auth/shell.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
cmd/auth/shell.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
cmd/auth/shell.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
cmd/auth/shell.go
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
cmd/auth/shell.go
🧬 Code graph analysis (2)
tests/describe_test.go (1)
internal/tui/templates/term/term_writer.go (1)
IsTTYSupportForStdout(121-123)
cmd/auth/shell.go (11)
pkg/flags/options.go (2)
WithStringFlag(49-60)WithEnvVars(221-244)cmd/internal/validation.go (1)
ValidateAtmosConfig(44-73)pkg/flags/global_registry.go (1)
BuildConfigAndStacksInfo(85-95)cmd/auth/helpers.go (2)
BuildConfigAndStacksInfo(32-36)CreateAuthManager(40-47)pkg/config/config.go (1)
InitCliConfig(28-67)pkg/flags/parser.go (1)
GetString(117-126)internal/exec/shell_utils.go (1)
ExecAuthShellCommand(316-374)cmd/identity_helpers.go (1)
GetIdentityFromFlags(36-62)cmd/auth/auth.go (3)
GetIdentityFromFlags(109-129)IdentityFlagName(17-17)IdentityFlagSelectValue(19-19)pkg/schema/schema.go (1)
AtmosConfiguration(54-100)pkg/env/global.go (1)
MergeGlobalEnv(16-35)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
🔇 Additional comments (15)
tests/describe_test.go (1)
12-30: Nice cross-platform TTY detection!The helper properly addresses the past review comments about resource leaks. The file descriptor is now correctly closed immediately after opening (line 27), and the cross-platform logic using
runtime.GOOSis clean.cmd/root.go (5)
35-35: LGTM on import additions.The preprocess package import and blank import for auth command registration follow the command registry pattern correctly. Based on learnings, this is the expected wiring for auto-registering commands via side-effect imports.
Also applies to: 54-54
1368-1392: Well-structured preprocessing orchestration.The two-step approach is sound: rewriting NoOptDefVal flags before separating compatibility flags ensures proper handling across command types. The early return and conditional SetArgs logic avoids redundant work.
1394-1405: Simple and correct slice comparison.Works as expected. Go 1.21+ has
slices.Equalbut this inline helper is fine for the single call site.
1407-1449: Good refactor to return status.Returning the bool allows
preprocessArgsto know whetherSetArgswas already called, avoiding redundant calls. The separation logic for Atmos vs pass-through flags remains intact.
1451-1477: Solid NoOptDefVal handling.The pipeline approach is clean and extensible. Converting registry flags to FlagInfo interface for the preprocessor is done correctly. This properly addresses Cobra's NoOptDefVal behavior for flags like
--identity.cmd/auth/whoami.go (6)
46-73: Clean init following StandardParser pattern.The flag registration, Viper binding, and completion setup are well-organized. Using
panicfor Viper binding errors ininit()is acceptable since these are static configuration issues. Thelog.Traceon completion registration failure is appropriate for non-critical errors.
75-114: LGTM on command execution flow.The flag binding, auth manager loading, identity resolution, and output handling are properly structured. Using
v.GetString(OutputFlagName)respects the flag/env/config precedence.
171-186: Proper config loading and error wrapping.Using
BuildConfigAndStacksInfo(cmd, v)correctly honors global flags like--base-path,--config,--config-path, and--profilebefore callingInitCliConfig. The%wverb is now used correctly for error chain preservation. Based on learnings, this is the correct pattern.
116-151: Validation logic is sound.The credential validation correctly checks for the validator interface, falls back to expiration checking, and populates whoami info on success. Good defensive nil check on credentials.
214-229: Output functions are well-structured.JSON output properly redacts credentials and home directory. Human output uses consistent theming and provides helpful tips when credentials are invalid.
Also applies to: 231-254
344-360: Sanitization covers expected sensitive patterns.The sensitive keyword list covers common secret patterns. The home directory redaction protects user paths from leaking in output.
cmd/auth/shell.go (3)
124-148: LGTM!Identity resolution logic properly handles the NoOptDefVal quirk, Viper fallback, and interactive selection. Error wrapping uses appropriate sentinel.
182-191: LGTM!Environment variable parsing correctly handles values containing
=by splitting only on the first occurrence.
195-208: LGTM!Correctly uses Cobra's
ArgsLenAtDash()to extract arguments after the--separator with proper bounds checking.
![atmos-pro[bot]](https://avatars.githubusercontent.com/in/927493?s=60&v=4){kind=small}
Adds focused unit tests for the testable helpers exposed by the cmd/auth registry refactor. Targets the lowest-coverage files flagged by Codecov: cmd/auth/auth.go (CommandProvider getters): - TestAuthCommandProvider_OptionalMethods covers GetPositionalArgsBuilder, GetCompatibilityFlags, GetAliases, IsExperimental. - TestGetAuthCmd_ReturnsAuthCmd guards the public accessor used by cmd/ai to attach subcommands. cmd/auth/env.go (env command helpers): - TestResolveEnvOutputFile covers all four branches of the GitHub auto- detect: non-github pass-through, explicit output-file, $GITHUB_ENV detect, and the missing-GITHUB_ENV error sentinel. - TestResolveEnvOutputTarget exercises viper-backed format/output-file resolution including the bash default and the github auto-detect. - TestLoginIfNeeded covers cache-hit (no Authenticate), missing-cache (Authenticate triggered), ErrUserAborted unwrapping, and generic- error wrapping with ErrAuthenticationFailed. - TestResolveIdentityNameForEnv covers the explicit-flag, viper-fallback, default-auto-detect, and __SELECT__ interactive paths. cmd/auth/console.go (browser isolation helpers): - TestConsoleSessionDir asserts the deterministic XDG path is stable across calls, diverges by identity, and diverges by realm. - TestResolveConsoleIsolated covers default false, auth.console.isolated config honoured, flag overrides config in both directions. - TestPrintConsoleHelpers smoke-covers printConsoleURL and printConsoleInfo with full + showURL=true paths. - TestRetrieveCredentials_NoCredentialsAvailable covers the ErrAuthConsole sentinel for the no-credentials-anywhere branch. cmd/auth/login.go (provider-fallback helpers): - TestGetProviderForFallback covers ErrNoProvidersAvailable for empty list, single-provider auto-select (no prompt), and multi-provider non-interactive ErrNoDefaultProvider. - TestIsInteractive guards determinism (same env → same answer). cmd/auth/whoami.go (whoami helpers): - TestAddGCPReauthExplanation covers nil pass-through, unrelated-error pass-through, invalid_grant alone (no enrichment), and invalid_grant + invalid_rapt enrichment with gcloud reauth hint. - TestPrintWhoamiJSON_RedactsCredentials guards the contract that the JSON output never mutates the caller's Environment map. - TestPrintWhoamiHuman covers valid/invalid/no-expiration table render. cmd/auth/list.go (list command helpers): - TestParseFilterFlags covers the default, --providers (with comma list), --identities (with comma list), and the mutually-exclusive ErrMutuallyExclusiveFlags branch. - TestRenderOutput_InvalidFormatErrors guards the default-case ErrInvalidFlag path. - TestListFlagCompletions_NoConfig covers the no-atmos.yaml early-return path of listProvidersFlagCompletion / listIdentitiesFlagCompletion. cmd/auth/logout.go (new logout_test.go): - TestBuildKeychainDeletionMessage covers the pure prompt formatter. - TestConfirmKeychainDeletion_ForceShortCircuit covers --force bypass. - TestConfirmKeychainDeletion_NonTTYWithoutForceErrors covers the ErrKeychainDeletionRequiresConfirmation branch. - TestDetectExternalCredentials covers GCP/Azure/AWS env-var detection. - TestBuildLogoutOptions covers the empty-config, identities+providers, and provider-cascade-label branches. - TestExecuteLogoutOption_InvalidType covers ErrInvalidLogoutOption. - TestDiscoverRealms covers missing dir (no error), no-provider-subdir filter, and aws/azure realm reporting. cmd/auth/completion.go (completion helpers): - TestIdentityFlagCompletion covers the no-atmos.yaml branch. - TestAddIdentityCompletion_NoFlag covers the no-flag-registered no-op. cmd/auth/user/helpers.go (form-builder helper): - TestBuildCredentialFormField covers all six branches: YAML-managed Note, DefaultValue pre-fill, password mode, optional, custom ValidateFunc wired, description message wired. Infra: - New testmain_test.go initialises pkg/data and pkg/ui formatters once for the package so tests that exercise printWhoamiJSON/printWhoamiHuman don't panic with "data.InitWriter() must be called". - Snapshot tests/snapshots/TestCLICommands_atmos_auth_validate_--verbose picks up an env-dependent debug line drop (gh CLI not authenticated). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's pre-commit hook installs `gofumpt@latest` which is now v0.10.0. That release tightened the multiline-call rule: when arguments span multiple lines, the function name + opening paren go on their own line and the closing paren goes on its own line as well. Local toolchain was on v0.9.1 which didn't enforce this, so the changes only surfaced on CI. Files affected (only PR-touched files reformatted): - cmd/auth/env.go: env.Output(...) call. - cmd/describe_dependents.go: getRunnableDescribeDependentsCmd(...) call. - cmd/describe_stacks.go: PersistentFlags().StringP(...) call. No behaviour change. Matches the auto-fix diff produced by the cloudposse/github-action-pre-commit job. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds smoke and branch-coverage tests across the auth command tree to lift patch coverage from the post-merge baseline (47.8%) to ~69%. Orchestrator smoke tests (no-atmos.yaml tempdir, no-panic contract): - TestExecuteAuthLoginCommand_SmokeNoConfig - TestExecuteAuthWhoamiCommand_SmokeNoConfig - TestExecuteAuthListCommand_SmokeNoConfig - TestExecuteAuthValidateCommand_SmokeNoConfig - TestExecuteAuthConsoleCommand_SmokeNoConfig - TestExecuteAuthEnvCommand_SmokeNoConfig - TestExecuteAuthExecCommand_SmokeNoConfig - TestExecuteAuthShellCommand_SmokeNoConfig - TestExecuteAuthLogoutCommand_SmokeNoConfig - TestExecuteAuthUserConfigureCommand_SmokeNoConfig (cmd/auth/user) These cover the config-load error wrap path in each orchestrator and guard against panics for invalid setup. Where a specific error sentinel is documented (e.g. ErrInvalidAuthConfig, ErrFailedToInitializeAtmosConfig, ErrAuthConsole), the test asserts the wrap when an error surfaces. loadAuthManager* helpers (config init + auth manager init): - TestLoadAuthManager_SmokeFromEmptyTempDir (whoami) - TestLoadAuthManagerForEnv_SmokeFromEmptyTempDir (env) - TestLoadAuthManagerForList_SmokeFromEmptyTempDir (list) - TestInitializeAuthManager_SmokeFromEmptyTempDir (console) - TestSuggestProfilesForAuth_NoProfilesReturnsNil prepareShellEnvironment (cmd/auth/shell.go) — mocked-AuthManager test covering cache-hit, fresh-auth-success, ErrUserAborted, generic-error wrap with ErrAuthenticationFailed, PrepareShellEnvironment error, and atmosConfig.Env propagation through MergeGlobalEnv: - TestPrepareShellEnvironment (six subtests) prepareAuthenticatedEnv (cmd/auth/exec.go) — smoke from empty tempdir: - TestPrepareAuthenticatedEnv_SmokeNoConfig Display + handleBrowserOpen helpers (smoke): - TestDisplayExternalCredentialWarnings (with-warnings + clean branch) - TestDisplayBrowserWarning (first-call + cached-skip branches) - TestHandleBrowserOpen (skipOpen=true, nil opener, success, error) Logout perform helpers — mocked AuthManager branch coverage: - TestPerformIdentityLogout_NotFound (ErrIdentityNotInConfig) - TestPerformIdentityLogout_DryRun (no Logout call) - TestPerformProviderLogout_NotFound (missing provider in config) - TestPerformLogoutAll_DryRun (no LogoutAll call) - TestPerformLogoutAll_Success (happy path, two identities) renderOutput dispatcher coverage: - TestRenderOutput_AllValidFormats covers all 9 valid format branches (table/tree/json/yaml/graphviz/dot/mermaid/markdown/md). Coverage summary: - cmd/auth: 47.6% → 69.7% - cmd/auth/user: 51.0% → 58.7% - combined: 47.8% → 68.9% Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
|
CodeRabbit (@coderabbitai) full review please |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
NOTICE (1)
1-1834:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRegenerate NOTICE file using the provided script.
The pipeline is failing because the NOTICE file is out of sync with the dependency state in go.mod. Run
./scripts/generate-notice.shlocally and commit the result to resolve this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NOTICE` around lines 1 - 1834, The NOTICE file is out of sync with go.mod causing pipeline failures; regenerate it by running the provided script and commit the updated file. Run ./scripts/generate-notice.sh locally (ensure your working tree reflects current go.mod/go.sum), verify the regenerated NOTICE content, and commit the updated NOTICE file so the CI/pipeline will pass.cmd/auth/logout.go (1)
42-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject extra positional args.
Use: "logout [identity]"advertises one optional arg, but the handler only readsargs[0]and ignores the rest. AddingArgs: cobra.MaximumNArgs(1)makes typos fail fast.Suggested fix.
var authLogoutCmd = &cobra.Command{ Use: "logout [identity]", + Args: cobra.MaximumNArgs(1), Short: "End session by clearing session data",Based on learnings, "In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/logout.go` around lines 42 - 58, The Cobra command for logout currently allows unlimited positional args despite advertising at most one; add an Args validator to the command definition by inserting Args: cobra.MaximumNArgs(1), alongside Use/Short/RunE so extra positional arguments are rejected (this touches the logout command struct in cmd/auth/logout.go where RunE is executeAuthLogoutCommand and ValidArgsFunction is IdentityArgCompletion); ensure the cobra package symbol MaximumNArgs is used in the same file.cmd/auth/console.go (1)
324-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
ErrNoDefaultIdentityhere.
executeAuthConsoleCommandsends identity-resolution failures throughmaybeOfferProfileFallbackOnAuthConfigError, but this helper wraps the missing-default path withErrAuthConsoleinstead of the shared auth-config sentinel. That meansauth consoleskips the profile-fallback flow that the other auth subcommands use.Suggested fix.
identityName, err := authManager.GetDefaultIdentity(forceSelect) if err != nil { - return "", fmt.Errorf("%w: failed to get default identity: %w", errUtils.ErrAuthConsole, err) + return "", fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrNoDefaultIdentity, err) } if identityName == "" { - return "", fmt.Errorf("%w: no default identity configured", errUtils.ErrAuthConsole) + return "", errUtils.ErrNoDefaultIdentity }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/console.go` around lines 324 - 349, resolveIdentityName currently wraps all GetDefaultIdentity failures with errUtils.ErrAuthConsole which prevents maybeOfferProfileFallbackOnAuthConfigError from detecting the sentinel ErrNoDefaultIdentity; change the error handling so that when authManager.GetDefaultIdentity returns the sentinel errUtils.ErrNoDefaultIdentity you return that sentinel (optionally wrapped for context, e.g. fmt.Errorf("%w: no default identity configured", errUtils.ErrNoDefaultIdentity)) from resolveIdentityName, while preserving the existing wrapping with errUtils.ErrAuthConsole for all other errors from GetDefaultIdentity; reference resolveIdentityName, authManager.GetDefaultIdentity, errUtils.ErrNoDefaultIdentity, errUtils.ErrAuthConsole and maybeOfferProfileFallbackOnAuthConfigError when making the change.
♻️ Duplicate comments (2)
cmd/auth/completion.go (1)
16-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHonor the active profile/config during completion.
These completion helpers still load config with
schema.ConfigAndStacksInfo{}, so--profile,--config,--config-path, and--base-pathare ignored during tab completion. That leavesauthcommands completing identities/providers from the default config even when the command itself would resolve a different profile. Use the current command context when building config here too.Also applies to: 49-71, 86-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/completion.go` around lines 16 - 33, identityFlagCompletion (and the other completion helpers) loads config via cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) which ignores command-level flags like --profile/--config/--config-path/--base-path; update these functions to initialize config using the command context so completion honors the active profile and paths (i.e., replace the plain cfg.InitCliConfig call with the command-aware initializer that accepts the *cobra.Command or context and the same schema/flags), ensure you propagate errors the same way and keep sorting/returning identities unchanged; apply the same change to the other completion helpers referenced (the blocks at lines 49-71 and 86-108).cmd/auth/list.go (1)
98-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake list completions profile-aware too.
Both completion helpers still build config from
schema.ConfigAndStacksInfo{}, so--profileand the config-path flags are ignored while completing--providersand--identities. That meansauth listcan execute against one profile but tab-complete names from another. Load config from the active command context here as well.Also applies to: 117-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/list.go` around lines 98 - 115, The completions are loading config without command context so --profile and config-path flags are ignored; update listProvidersFlagCompletion (and the similar listIdentitiesFlagCompletion at lines 117-134) to initialize the CLI config using the active command context (i.e. call the variant of cfg.InitCliConfig that accepts the cobra command/context used elsewhere in the codebase, e.g. cfg.InitCliConfigFromCommand(cmd, schema.ConfigAndStacksInfo{}, false) or the project’s existing helper that reads flags from cmd), handle the returned error as currently done, and then build the providers/identities slice from atmosConfig so tab-completion respects --profile and config-path.
🧹 Nitpick comments (8)
cmd/identity_helpers.go (1)
182-188: ⚡ Quick winConsider adding perf tracking to CreateAuthManagerFromIdentityWithStackScan.
This exported function is missing
defer perf.Track(nil, "cmd.CreateAuthManagerFromIdentityWithStackScan")()while the other three auth manager creation functions have it. If this was intentional (perhaps because it immediately delegates), please add a comment explaining why.⚡ Suggested addition
func CreateAuthManagerFromIdentityWithStackScan( identityName string, authConfig *schema.AuthConfig, atmosConfig *schema.AtmosConfiguration, ) (auth.AuthManager, error) { + defer perf.Track(nil, "cmd.CreateAuthManagerFromIdentityWithStackScan")() + return auth.CreateAndAuthenticateManagerWithStackScan(identityName, authConfig, cfg.IdentityFlagSelectValue, atmosConfig) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/identity_helpers.go` around lines 182 - 188, The exported function CreateAuthManagerFromIdentityWithStackScan is missing perf timing instrumentation consistent with the other auth manager factory functions; add a defer perf.Track(nil, "cmd.CreateAuthManagerFromIdentityWithStackScan")() at the top of CreateAuthManagerFromIdentityWithStackScan (or, if omission is intentional, add a brief comment above the function explaining why instrumentation is omitted) so its execution is tracked the same way as the other CreateAuthManagerFromIdentity* helpers.cmd/auth/markdown/atmos_auth_list_usage.md (1)
3-91: ⚡ Quick winAdd shell language identifier to fenced code blocks.
The code blocks should specify
shellas the language for better syntax highlighting and consistency with documentation standards. This matches the coding guideline requiring proper documentation formatting.📝 Example fix
-``` +```shell $ atmos auth listApply this pattern to all code blocks in the file (lines 3, 9, 15, 21, 27, 33, 39, 45, 51, 57, 63, 77, 83, 89). </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@cmd/auth/markdown/atmos_auth_list_usage.mdaround lines 3 - 91, Update every
fenced code block in the markdown so each opening triple-backtick specifies the
shell language (e.g., changetoshell) so all examples like the atmos
auth list snippets and the Graphviz/Mermaid/Markdown generation commands usestarting with "$ atmos auth list", "$ atmos auth list --providers", "$ atmos auth list --format graphviz | dot -Tsvg", etc.) and add the language identifier to each opening fence to ensure consistent syntax highlighting.cmd/auth/markdown/atmos_auth_console_usage.md (1)
3-59: ⚡ Quick winAdd shell language identifier to fenced code blocks.
Same as
atmos_auth_list_usage.md, the code blocks should specifyshellfor proper syntax highlighting and documentation consistency.📝 Example fix
-```shell +```shell atmos auth consoleApply to all code blocks in the file. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@cmd/auth/markdown/atmos_auth_console_usage.mdaround lines 3 - 59, The
fenced code blocks showing CLI examples (e.g., the "atmos auth console" usage
and its variants like "--identity prod-admin", "--destination s3",
"--print-only", "--no-open", "--duration 2h", "--issuer my-org") should include
the shell language identifier; update every triple-backtick fence in this file
to start withshell so each block becomesshell ... ``` for consistent
syntax highlighting and parity with atmos_auth_list_usage.md.</details> </blockquote></details> <details> <summary>cmd/auth/user/configure.go (1)</summary><blockquote> `79-80`: _⚡ Quick win_ **Consider using `ui.Writef` for user-facing messages.** These status messages currently use `fmt.Fprintf(cmd.ErrOrStderr(), ...)`. Per coding guidelines, user-facing messages should use the UI layer (`ui.Writef`) for consistent formatting and automatic handling of TTY detection, color degradation, etc. This pattern is used in `cmd/auth/helpers.go` line 79 for similar success messages. <details> <summary>🎨 Suggested refactor</summary> ```diff if yamlInfo.AllInYAML { // All credentials are in YAML - nothing to configure in keyring. - fmt.Fprintln(cmd.ErrOrStderr(), "All credentials are managed by Atmos configuration (atmos.yaml)") - fmt.Fprintln(cmd.ErrOrStderr(), "To update credentials, edit your atmos.yaml configuration file") + ui.Writef("All credentials are managed by Atmos configuration (atmos.yaml)\n") + ui.Writef("To update credentials, edit your atmos.yaml configuration file\n") if yamlInfo.MfaArn != "" { - fmt.Fprintf(cmd.ErrOrStderr(), "MFA ARN is also managed by Atmos configuration: %s\n", yamlInfo.MfaArn) + ui.Writef("MFA ARN is also managed by Atmos configuration: %s\n", yamlInfo.MfaArn) } return nil } ``` ```diff // Save credentials to keyring. store := credentials.NewCredentialStore() if err := store.Store(alias, creds, ""); err != nil { return fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrAwsAuth, err) } - fmt.Fprintf(cmd.ErrOrStderr(), "Saved credentials to keyring: %s\n", alias) + ui.Writef("Saved credentials to keyring: %s\n", alias) if creds.SessionDuration != "" { - fmt.Fprintf(cmd.ErrOrStderr(), "Session duration configured: %s\n", creds.SessionDuration) + ui.Writef("Session duration configured: %s\n", creds.SessionDuration) } return nil ``` </details> As per coding guidelines: Use the two-layer I/O and UI architecture with `ui.Writef` for human messages instead of `fmt.Fprintf(os.Stdout/Stderr)`. Also applies to: 82-82, 106-106, 108-108 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/user/configure.go` around lines 79 - 80, Replace direct fmt.Fprintln calls that write to cmd.ErrOrStderr() with the UI layer: use ui.Writef for all user-facing messages in configure.go (replace occurrences of fmt.Fprintln(cmd.ErrOrStderr(), ...) at the current spots and the other occurrences noted). Specifically, swap each fmt.Fprintln(...) to ui.Writef(...) preserving the same message text and formatting placeholders so TTY detection and formatting are handled by the UI layer; ensure the ui variable used matches the command's UI instance imported/available in the configure command. ``` </details> </blockquote></details> <details> <summary>cmd/auth/validate_test.go (1)</summary><blockquote> `48-50`: _⚡ Quick win_ **Assert the expected failure, not just “no panic.”** This still passes if the handler fails for the wrong reason. Capture the returned error and pin the no-config path explicitly once `executeAuthValidateCommand` exposes a stable sentinel; otherwise the regression coverage stays loose. As per coding guidelines: "Test behavior, not implementation; never test stub functions; avoid tautological tests; no coverage theater." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/validate_test.go` around lines 48 - 50, The test currently only asserts no panic; instead capture the error returned by executeAuthValidateCommand and assert the specific expected failure (e.g., the "no config" sentinel or a stable sentinel error) so the test verifies the correct failure path; if executeAuthValidateCommand does not yet return a stable sentinel, modify it to return one (or export a package-level sentinel error) and then update the test to require that exact error (or match a precise error string) rather than merely asserting NotPanics. ``` </details> </blockquote></details> <details> <summary>cmd/auth/validate.go (1)</summary><blockquote> `71-74`: _⚡ Quick win_ **Use the repo’s static error sentinel here.** This path falls back to an ad-hoc `fmt.Errorf("failed to load atmos config: %w", err)`, which makes `errors.Is` checks and regression tests brittle. Wrap this with the same static atmos-config error used in the other auth commands so callers can match the failure mode reliably. As per coding guidelines: "All errors MUST be wrapped using static errors defined in `errors/errors.go`." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/validate.go` around lines 71 - 74, Replace the ad-hoc error returned from the cfg.InitCliConfig call with the repo's static sentinel by wrapping the original error with the sentinel (e.g., use errors.ErrAtmosConfig); specifically, in the error branch after calling cfg.InitCliConfig, return a wrapped error like fmt.Errorf("%w: %v", errors.ErrAtmosConfig, err) (and add the errors import if missing) so callers can reliably match this failure using errors.Is; the change should be applied at the cfg.InitCliConfig error handling in validate.go. ``` </details> </blockquote></details> <details> <summary>cmd/auth/login_test.go (1)</summary><blockquote> `177-189`: _⚡ Quick win_ **Make the multi-provider fallback test deterministic.** Skipping when the runner looks interactive means the main `ErrNoDefaultProvider` assertion can disappear on local runs. A tiny injectable `isInteractive` hook or helper parameter would let this unit test force the non-interactive branch every time. As per coding guidelines "Use table-driven tests for comprehensive coverage; prefer unit tests with mocks over integration tests; target >80% coverage". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/login_test.go` around lines 177 - 189, The test is flaky because it relies on the real isInteractive detection; modify getProviderForFallback to accept an injectable isInteractive checker (e.g., change signature to getProviderForFallback(lister ProviderLister, isInteractive func() bool) or add an optional functional parameter) and update production call sites to pass the real isInteractive, while updating this test to pass a stub isInteractive that returns false so the non-interactive branch always executes; reference getProviderForFallback and fakeProviderLister when making the change and ensure tests assert ErrNoDefaultProvider deterministically. ``` </details> </blockquote></details> <details> <summary>cmd/auth/identity_resolution_test.go (1)</summary><blockquote> `25-58`: _⚡ Quick win_ **Add the flag-over-env precedence case to the shared table.** The harness never sets both `identityFlag` and `viperIdentity`, so it won't catch a regression where env wins over an explicit `--identity`. One shared case here would protect both shell and exec resolution. As per coding guidelines "Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/identity_resolution_test.go` around lines 25 - 58, Add a shared test case in getIdentityResolutionTestCases to ensure flags take precedence over environment: create a case named like "flag takes precedence over env" that sets identityFlag to a literal (e.g., "flag-identity"), viperIdentity to a different value (e.g., "env-identity"), expectedResult to the flag value ("flag-identity") and expectedError to nil; for setupMock provide no expectation (or a nil/no-op) so the auth manager’s GetDefaultIdentity is not called when an explicit flag is provided. This ensures the harness covers the precedence rule flags > environment variables and protects both shell and exec resolution paths. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@cmd/auth/console.go:
- Around line 180-183: The print-only branch currently ignores the result of
data.Writeln(consoleURL), so stdout/write errors are lost; change the branch to
capture and return the error from data.Writeln (e.g., err :=
data.Writeln(consoleURL); if err != nil { return err }) before returning nil so
write failures (broken pipe) propagate to the caller; update the branch around
the printOnly check in console.go accordingly.In
@cmd/auth/exec_test.go:
- Around line 132-140: Replace the platform-dependent subprocess calls in
TestExecuteCommandWithEnv_WithValidCommand (and its failure-path sibling) with a
Go test helper binary: add a TestMain or helper mode in the test file that
detects a special env var or arg and exits with 0 for the “success” case and
non-zero for the “failure” case, then invoke executeCommandWithEnv using
os.Executable() plus the helper arg/marker and appropriate env to simulate
success and failure deterministically; update the tests to call
executeCommandWithEnv([]string{os.Executable(), "helper-success"} , ...) and
executeCommandWithEnv([]string{os.Executable(), "helper-fail"}, ...) (or use an
env switch) instead of calling external binaries like "go" or "false".- Around line 205-218: The test currently injects the profile by seeding viper
directly which bypasses Cobra flag parsing; change the test to simulate the real
flag path by setting the --profile flag on the command created by
newTestCommandWithGlobalFlags("exec") (e.g., use cmd.SetArgs or cmd.Flags().Set
to provide the profile), run the command's flag parsing or call BindFlagsToViper
against the command's FlagSet, then call BuildConfigAndStacksInfo(cmd, v) and
assert info.ProfilesFromArg; reference newTestCommandWithGlobalFlags,
BindFlagsToViper, and BuildConfigAndStacksInfo to locate where to modify the
test so it exercises Cobra flag binding instead of directly setting viper.In
@cmd/auth/exec.go:
- Around line 139-155: The code converts the []string returned by
authManager.PrepareShellEnvironment into a map (envMap), which loses ordering
and breaks Windows drive-scoped vars; instead remove the conversion loop that
builds envMap and return the envList slice directly from the function (keep the
baseEnv merge + call to PrepareShellEnvironment as-is). Also remove or change
the similar conversion at the other occurrence in this file (the code that
iterates envList and indexes '=' to populate envMap around the later block) so
both places pass the original []string through to the child process rather than
constructing a map.In
@cmd/auth/markdown/atmos_auth_shell_usage.md:
- Around line 3-35: Replace the unlabeled fenced blocks that include
$-prefixed commands with labeled "shell" code fences and remove the leading
$prompt so the examples like "atmos auth shell", "atmos auth shell --identity
prod-admin", "atmos auth shell --shell /bin/zsh", "atmos auth shell -- -c "env
| grep AWS"", "atmos auth shell -- --norc", and "atmos auth shell --identity
staging-readonly -- -c "terraform plan"" are wrapped asshell ...
blocks (no leading $) throughout the snippet to satisfy MD040/MD014 lint rules.In
@cmd/auth/shell.go:
- Around line 36-48: The command currently ignores positional shell args because
getSeparatedArgs only reads args after "--"; to fail fast, update the auth shell
command entrypoint (executeAuthShellCommand) to detect mistyped positional args
by checking if len(args) > 0 && cmd.ArgsLenAtDash() == -1 and return a
user-facing error instructing to use "--" (or show usage) instead of silently
dropping them; place this check before calling getSeparatedArgs so the command
errors when users supply positional args likeatmos auth shell bashwithout
the separator.
Outside diff comments:
In@cmd/auth/console.go:
- Around line 324-349: resolveIdentityName currently wraps all
GetDefaultIdentity failures with errUtils.ErrAuthConsole which prevents
maybeOfferProfileFallbackOnAuthConfigError from detecting the sentinel
ErrNoDefaultIdentity; change the error handling so that when
authManager.GetDefaultIdentity returns the sentinel
errUtils.ErrNoDefaultIdentity you return that sentinel (optionally wrapped for
context, e.g. fmt.Errorf("%w: no default identity configured",
errUtils.ErrNoDefaultIdentity)) from resolveIdentityName, while preserving the
existing wrapping with errUtils.ErrAuthConsole for all other errors from
GetDefaultIdentity; reference resolveIdentityName,
authManager.GetDefaultIdentity, errUtils.ErrNoDefaultIdentity,
errUtils.ErrAuthConsole and maybeOfferProfileFallbackOnAuthConfigError when
making the change.In
@cmd/auth/logout.go:
- Around line 42-58: The Cobra command for logout currently allows unlimited
positional args despite advertising at most one; add an Args validator to the
command definition by inserting Args: cobra.MaximumNArgs(1), alongside
Use/Short/RunE so extra positional arguments are rejected (this touches the
logout command struct in cmd/auth/logout.go where RunE is
executeAuthLogoutCommand and ValidArgsFunction is IdentityArgCompletion); ensure
the cobra package symbol MaximumNArgs is used in the same file.In
@NOTICE:
- Around line 1-1834: The NOTICE file is out of sync with go.mod causing
pipeline failures; regenerate it by running the provided script and commit the
updated file. Run ./scripts/generate-notice.sh locally (ensure your working tree
reflects current go.mod/go.sum), verify the regenerated NOTICE content, and
commit the updated NOTICE file so the CI/pipeline will pass.
Duplicate comments:
In@cmd/auth/completion.go:
- Around line 16-33: identityFlagCompletion (and the other completion helpers)
loads config via cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) which
ignores command-level flags like --profile/--config/--config-path/--base-path;
update these functions to initialize config using the command context so
completion honors the active profile and paths (i.e., replace the plain
cfg.InitCliConfig call with the command-aware initializer that accepts the
*cobra.Command or context and the same schema/flags), ensure you propagate
errors the same way and keep sorting/returning identities unchanged; apply the
same change to the other completion helpers referenced (the blocks at lines
49-71 and 86-108).In
@cmd/auth/list.go:
- Around line 98-115: The completions are loading config without command context
so --profile and config-path flags are ignored; update
listProvidersFlagCompletion (and the similar listIdentitiesFlagCompletion at
lines 117-134) to initialize the CLI config using the active command context
(i.e. call the variant of cfg.InitCliConfig that accepts the cobra
command/context used elsewhere in the codebase, e.g.
cfg.InitCliConfigFromCommand(cmd, schema.ConfigAndStacksInfo{}, false) or the
project’s existing helper that reads flags from cmd), handle the returned error
as currently done, and then build the providers/identities slice from
atmosConfig so tab-completion respects --profile and config-path.
Nitpick comments:
In@cmd/auth/identity_resolution_test.go:
- Around line 25-58: Add a shared test case in getIdentityResolutionTestCases to
ensure flags take precedence over environment: create a case named like "flag
takes precedence over env" that sets identityFlag to a literal (e.g.,
"flag-identity"), viperIdentity to a different value (e.g., "env-identity"),
expectedResult to the flag value ("flag-identity") and expectedError to nil; for
setupMock provide no expectation (or a nil/no-op) so the auth manager’s
GetDefaultIdentity is not called when an explicit flag is provided. This ensures
the harness covers the precedence rule flags > environment variables and
protects both shell and exec resolution paths.In
@cmd/auth/login_test.go:
- Around line 177-189: The test is flaky because it relies on the real
isInteractive detection; modify getProviderForFallback to accept an injectable
isInteractive checker (e.g., change signature to getProviderForFallback(lister
ProviderLister, isInteractive func() bool) or add an optional functional
parameter) and update production call sites to pass the real isInteractive,
while updating this test to pass a stub isInteractive that returns false so the
non-interactive branch always executes; reference getProviderForFallback and
fakeProviderLister when making the change and ensure tests assert
ErrNoDefaultProvider deterministically.In
@cmd/auth/markdown/atmos_auth_console_usage.md:
- Around line 3-59: The fenced code blocks showing CLI examples (e.g., the
"atmos auth console" usage and its variants like "--identity prod-admin",
"--destination s3", "--print-only", "--no-open", "--duration 2h", "--issuer
my-org") should include the shell language identifier; update every
triple-backtick fence in this file to start withshell so each block becomesshell ... ``` for consistent syntax highlighting and parity with
atmos_auth_list_usage.md.In
@cmd/auth/markdown/atmos_auth_list_usage.md:
- Around line 3-91: Update every fenced code block in the markdown so each
opening triple-backtick specifies the shell language (e.g., change ``` toGraphviz/Mermaid/Markdown generation commands use ```shell; locate the blocks by their literal contents (for example the blocks starting with "$ atmos auth list", "$ atmos auth list --providers", "$ atmos auth list --format graphviz | dot -Tsvg", etc.) and add the language identifier to each opening fence to ensure consistent syntax highlighting. In `@cmd/auth/user/configure.go`: - Around line 79-80: Replace direct fmt.Fprintln calls that write to cmd.ErrOrStderr() with the UI layer: use ui.Writef for all user-facing messages in configure.go (replace occurrences of fmt.Fprintln(cmd.ErrOrStderr(), ...) at the current spots and the other occurrences noted). Specifically, swap each fmt.Fprintln(...) to ui.Writef(...) preserving the same message text and formatting placeholders so TTY detection and formatting are handled by the UI layer; ensure the ui variable used matches the command's UI instance imported/available in the configure command. In `@cmd/auth/validate_test.go`: - Around line 48-50: The test currently only asserts no panic; instead capture the error returned by executeAuthValidateCommand and assert the specific expected failure (e.g., the "no config" sentinel or a stable sentinel error) so the test verifies the correct failure path; if executeAuthValidateCommand does not yet return a stable sentinel, modify it to return one (or export a package-level sentinel error) and then update the test to require that exact error (or match a precise error string) rather than merely asserting NotPanics. In `@cmd/auth/validate.go`: - Around line 71-74: Replace the ad-hoc error returned from the cfg.InitCliConfig call with the repo's static sentinel by wrapping the original error with the sentinel (e.g., use errors.ErrAtmosConfig); specifically, in the error branch after calling cfg.InitCliConfig, return a wrapped error like fmt.Errorf("%w: %v", errors.ErrAtmosConfig, err) (and add the errors import if missing) so callers can reliably match this failure using errors.Is; the change should be applied at the cfg.InitCliConfig error handling in validate.go. In `@cmd/identity_helpers.go`: - Around line 182-188: The exported function CreateAuthManagerFromIdentityWithStackScan is missing perf timing instrumentation consistent with the other auth manager factory functions; add a defer perf.Track(nil, "cmd.CreateAuthManagerFromIdentityWithStackScan")() at the top of CreateAuthManagerFromIdentityWithStackScan (or, if omission is intentional, add a brief comment above the function explaining why instrumentation is omitted) so its execution is tracked the same way as the other CreateAuthManagerFromIdentity* helpers.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID:
7d98b9e6-6a78-4699-b150-811207332c27⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum📒 Files selected for processing (135)
NOTICEcmd/auth.gocmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/completion.gocmd/auth/completion_test.gocmd/auth/console.gocmd/auth/console_test.gocmd/auth/env.gocmd/auth/env_test.gocmd/auth/exec.gocmd/auth/exec_test.gocmd/auth/exec_unix_test.gocmd/auth/helpers.gocmd/auth/helpers_test.gocmd/auth/identity_flow_test.gocmd/auth/identity_resolution_test.gocmd/auth/integration_test.gocmd/auth/list.gocmd/auth/list_test.gocmd/auth/login.gocmd/auth/login_test.gocmd/auth/logout.gocmd/auth/logout_test.gocmd/auth/markdown/atmos_auth_console_usage.mdcmd/auth/markdown/atmos_auth_list_usage.mdcmd/auth/markdown/atmos_auth_logout_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.mdcmd/auth/profile_fallback.gocmd/auth/profile_fallback_test.gocmd/auth/shell.gocmd/auth/shell_test.gocmd/auth/testmain_test.gocmd/auth/user/configure.gocmd/auth/user/configure_test.gocmd/auth/user/helpers.gocmd/auth/user/helpers_test.gocmd/auth/user/user.gocmd/auth/user/user_test.gocmd/auth/validate.gocmd/auth/validate_test.gocmd/auth/whoami.gocmd/auth/whoami_test.gocmd/auth_caching_test.gocmd/auth_config.gocmd/auth_config_test.gocmd/auth_console_test.gocmd/auth_env.gocmd/auth_env_test.gocmd/auth_exec.gocmd/auth_exec_test.gocmd/auth_integration_test.gocmd/auth_list_test.gocmd/auth_login_test.gocmd/auth_logout_test.gocmd/auth_shell.gocmd/auth_shell_test.gocmd/auth_user.gocmd/auth_user_helpers_test.gocmd/auth_user_test.gocmd/auth_validate.gocmd/auth_validate_test.gocmd/auth_whoami_test.gocmd/auth_workflows_test.gocmd/describe_affected.gocmd/describe_component.gocmd/describe_dependents.gocmd/describe_skip_auth_test.gocmd/describe_stacks.gocmd/identity_flag_test.gocmd/identity_helpers.gocmd/root.gocmd/terraform/flags.gocmd/terraform/flags_test.goerrors/errors.goexamples/quick-start-advanced/Dockerfilego.modinternal/exec/cli_utils_test.gointernal/exec/terraform_execute_helpers_auth_test.gopkg/ai/analyze/analyze_test.gopkg/devcontainer/lifecycle_rebuild_test.gopkg/flags/flag_parser.gopkg/flags/global_registry.gopkg/flags/global_registry_test.gopkg/flags/preprocess/nooptdefval.gopkg/flags/preprocess/nooptdefval_test.gopkg/flags/preprocess/preprocess.gopkg/flags/preprocess/preprocess_test.gopkg/flags/registry.gopkg/flags/registry_preprocess_test.gopkg/flags/types_test.gotests/cli_describe_component_test.gotests/describe_test.gotests/snapshots/TestCLICommands_atmos_--chdir_config_isolation.stdout.goldentests/snapshots/TestCLICommands_atmos_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_--help_config_aliases_section.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_env_--format_dotenv_--identity_mock-identity.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_env_--format_invalid.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_env_--format_json_--identity_mock-identity.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_env_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_env_--login_--format_bash.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_env_--login_without_cached_credentials.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_env_without_--login_returns_config_env_vars.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_exec_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_exec_without_authentication.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_list.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_list_--format_json.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_login_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_login_--identity_mock-identity#01.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_login_--identity_mock-identity-2.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_login_--identity_mock-identity.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_login_with_default_identity.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_user_configure_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_validate.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_validate_(circular_dependency).stderr.goldentests/snapshots/TestCLICommands_atmos_auth_validate_(invalid_config).stderr.goldentests/snapshots/TestCLICommands_atmos_auth_validate_(missing_provider).stderr.goldentests/snapshots/TestCLICommands_atmos_auth_validate_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_validate_--verbose.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_validate_with_mock_provider.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_whoami_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_whoami_--identity_nonexistent.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_whoami_without_authentication.stderr.goldentests/snapshots/TestCLICommands_atmos_describe_config.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.goldentests/snapshots/TestCLICommands_atmos_terraform_-help_passthrough.stdout.goldentests/snapshots/TestCLICommands_atmos_toolchain_info_tar.gz_format_tool.stderr.goldentests/snapshots/TestCLICommands_help_flag_works.stdout.goldentests/snapshots/TestCLICommands_indentation.stdout.goldentests/snapshots/TestCLICommands_secrets-masking_describe_config.stdout.goldenwebsite/docs/cli/commands/auth/usage.mdxwebsite/docs/cli/commands/terraform/usage.mdxwebsite/docs/cli/global-flags.mdx💤 Files with no reviewable changes (24)
- cmd/terraform/flags_test.go
- cmd/auth_user_helpers_test.go
- cmd/auth_list_test.go
- cmd/auth_shell.go
- cmd/auth_exec_test.go
- cmd/auth_user_test.go
- cmd/auth_whoami_test.go
- cmd/auth_config.go
- cmd/auth.go
- cmd/auth_env_test.go
- cmd/auth_login_test.go
- cmd/identity_flag_test.go
- cmd/auth_workflows_test.go
- cmd/auth_logout_test.go
- cmd/auth_caching_test.go
- cmd/auth_config_test.go
- cmd/auth_env.go
- cmd/auth_console_test.go
- cmd/auth_shell_test.go
- cmd/auth_user.go
- cmd/auth_validate.go
- cmd/auth_exec.go
- cmd/auth_validate_test.go
- cmd/auth_integration_test.go
Addresses CodeRabbit review comments and substantially increases test
coverage of the cmd/auth package using a shared mock-auth fixture.
Review fixes:
- cmd/auth/completion.go, cmd/auth/list.go: all five shell-completion
helpers now honour --base-path, --config, --config-path, and --profile
by routing through BuildConfigAndStacksInfo(cmd, v) instead of using
an empty ConfigAndStacksInfo{}.
- cmd/auth/console.go: --print-only now propagates the data.Writeln
write error so broken-pipe / stdout failures exit non-zero. The
empty-identity branch of resolveIdentityName now uses multi-%w so
ErrNoDefaultIdentity stays in the chain for the profile-fallback
dispatcher.
- cmd/auth/exec.go: prepareAuthenticatedEnv now returns []string
directly (was []string → map → []string round-trip) so environment
ordering is preserved and Windows drive-scoped vars (=C:=...) don't
collide on the empty key.
- cmd/auth/shell.go: fail fast when positional args are not preceded
by `--` (`atmos auth shell bash` previously silently dropped "bash").
- cmd/auth/logout.go: add cobra.MaximumNArgs(1) so extra positional
args are rejected up front.
- cmd/auth/validate.go: wrap config-load failure with the static
ErrFailedToInitializeAtmosConfig sentinel.
- cmd/auth/login.go: introduce isInteractiveFn package var so
getProviderForFallback tests can force the non-interactive branch
deterministically.
- cmd/auth/markdown/*.md: every opening fence now has the `shell`
language identifier (MD040). The `$ ` prefix on commands is kept
for consistency with the rest of cmd/markdown/.
- cmd/auth/user/configure.go: switch user-facing messages from
fmt.Fprintf(cmd.ErrOrStderr()) to ui.Writef / ui.Writeln per the
I/O layer convention.
- cmd/identity_helpers.go: add the missing perf.Track in
CreateAuthManagerFromIdentityWithStackScan to match its sibling
helpers.
Coverage improvements (cmd/auth 47.6% → 79.3%; combined 47.8% → 77.8%):
New shared helpers (helpers_test.go):
- setupMockAuthFixture(t): writes a minimal atmos.yaml wired to the
mock/aws provider and isolates keyring + XDG env to a tempdir, used
by 9 deep-coverage tests to exercise the full orchestrator pipeline
without touching the host's credential store.
- runProfileFlagAppliedRegressionTest(t, commandName): shared
table-driven driver for the issue #1973 regression so the
exec_test.go and shell_test.go wrappers feed the same cases through
one body (and dupl-lint stays clean).
- newTestCommandWithGlobalParser: parser-returning variant of the
global-flags helper so regression tests can drive the real
Cobra → Viper binding path (cmd.ParseFlags → BindFlagsToViper).
End-to-end orchestrator tests against the mock fixture:
- TestExecuteAuthEnvCommand_WithMockAuth
- TestExecuteAuthLoginCommand_WithMockAuth
- TestExecuteAuthListCommand_WithMockAuth / _JSONFormat
- TestExecuteAuthValidateCommand_WithMockAuth
- TestExecuteAuthWhoamiCommand_WithMockAuth
- TestExecuteAuthConsoleCommand_WithMockAuth (covers
ErrProviderNotSupported for mock/aws)
- TestExecuteAuthLogoutCommand_WithMockAuthDryRun
- TestPrepareAuthenticatedEnv_WithMockAuth (asserts AWS_PROFILE +
AWS_REGION injection)
- TestExecuteAuthExecCommand_NoCommand (ErrNoCommandSpecified guard)
Logout perform-helper branch coverage:
- TestPerformIdentityLogout_Success / _PartialLogout / _LogoutError
- TestPerformProviderLogout_Success / _DryRun
- TestExecuteLogoutOption_DispatchAll / _DispatchIdentity / _DispatchProvider
- TestPerformInteractiveLogout_NoIdentities (empty-identities branch)
- TestPerformLogoutAllRealms_NoRealms / _DryRun / _RealRemove
Pure / smoke helpers:
- TestRetrieveCredentials_InlineCredentials (success path)
- TestPromptForProvider_EmptyList (ErrNoProvidersAvailable guard)
- TestExecuteCommandWithEnv_NonZeroExit (errUtils.ExitCodeError
propagation via the test-binary subprocess pattern)
- TestExecuteCommandWithEnv_WithValidCommand rewritten to use
os.Executable() + _ATMOS_AUTH_TEST_EXIT_OK=1 (cross-platform,
no PATH dependency on `go` / `true` / `false`).
- Subprocess env-flag handlers added to TestMain.
Test infrastructure:
- TestAuthExec_ProfileFlagAppliedToConfig and the shell equivalent
now use --profile=devops style CLI args + ParseFlags +
BindFlagsToViper, exercising the full production binding chain
rather than seeding viper directly.
- Identity-resolution shared test table gains a "flag takes
precedence over env" case so the precedence rule (flags > env >
default) is regression-covered for both auth shell and auth exec.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
cmd/auth/console_test.go (3)
387-393: 💤 Low valueConsider removing tautological constant tests.
Testing that
ConsoleLabelWidth == 18andConsoleOutputFormat == "%s %s\n"just verifies code says what code says. Unless these are part of a versioned public API contract, the coverage gain is minimal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/console_test.go` around lines 387 - 393, The two tests TestConsoleLabelWidth and TestConsoleOutputFormat are tautological and should be removed to reduce noise; delete the functions referencing ConsoleLabelWidth and ConsoleOutputFormat from cmd/auth/console_test.go (or replace them with a single documented assertion if you need an explicit API contract), or alternatively convert them into a comment documenting that these constants are part of the public contract if you must preserve their intent. Ensure you remove references to the symbols ConsoleLabelWidth and ConsoleOutputFormat in those test functions (or replace with a single explanatory comment) to satisfy the review.
245-256: ⚡ Quick winFragile test case identification.
Using
tt.name == "invalid provider duration format"couples test logic to the test name string. If someone renames the test case, this special handling silently breaks.♻️ Recommended pattern: add an explicit field to the test struct
tests := []struct { name string setupMock func(*authTypes.MockAuthManager) flagChanged bool flagDuration time.Duration providerName string expectedResult time.Duration expectedError error + expectParseError bool }{ // ... { name: "invalid provider duration format", // ... + expectParseError: true, }, } // In the test body: switch { -case tt.name == "invalid provider duration format": +case tt.expectParseError: assert.Error(t, err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/console_test.go` around lines 245 - 256, The test switches on tt.name to detect the "invalid provider duration format" case, which is fragile; add an explicit field to the test case struct (e.g., expectParseError bool or specialCase string) and set it in the specific test case instead of relying on tt.name, then update the switch/conditional logic in the test to check that new field (referencing tt.name, tt.expectedError, and the switch block) so the special handling remains correct if test names change.
1-688: 💤 Low valueConsider splitting test file to meet 600-line limit.
The file is 688 lines. You could group related tests into separate files:
console_identity_test.go,console_provider_test.go,console_duration_test.go,console_browser_test.go, andconsole_command_test.go. That said, keeping all tests for one command together has navigation benefits, so this is a judgment call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/console_test.go` around lines 1 - 688, The test file exceeds the 600-line guideline; split related test groups into smaller files to improve maintainability: move identity-related tests (e.g., TestResolveIdentityName_Console, TestRetrieveCredentials, TestRetrieveCredentials_NoCredentialsAvailable, TestRetrieveCredentials_InlineCredentials) into console_identity_test.go; move provider and provider-kind tests (e.g., TestGetConsoleProvider, TestDestinationFlagCompletion) into console_provider_test.go; move duration/config tests (e.g., TestResolveConsoleDuration, TestResolveConsoleIsolated, TestConsoleLabelWidth, TestConsoleOutputFormat) into console_duration_test.go; move browser/opener tests (e.g., fakeOpener, TestHandleBrowserOpen, TestConsoleSessionDir, TestPrintConsoleHelpers) into console_browser_test.go; and keep orchestration/command-level tests (e.g., TestAuthConsoleCommand_Structure, TestExecuteAuthConsoleCommand_SmokeNoConfig, TestExecuteAuthConsoleCommand_WithMockAuth, TestInitializeAuthManager_SmokeFromEmptyTempDir) in console_command_test.go — ensure each new file imports the same packages and preserves setup helpers like setupMockAuthFixture and uses the same gomock controller usage and test helper symbols (authConsoleCmd, resolveConsoleDuration, getConsoleProvider, handleBrowserOpen, executeAuthConsoleCommand, initializeAuthManager) so tests compile unchanged.cmd/auth/logout_test.go (1)
465-467: 💤 Low valueConsider adding a brief comment.
While unexported helpers don't require documentation, a one-line comment explaining this returns a stub for mock expectations would improve readability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/logout_test.go` around lines 465 - 467, Add a one-line comment above the unexported helper function realmInfoMatcher() indicating its purpose as a test stub for mock expectations (e.g., "returns a stub RealmInfo used in mock expectations"); this clarifies intent without exposing the helper publicly and makes test code easier to read.cmd/auth/exec_test.go (1)
20-49: 💤 Low valueSingle-case table is over-structured.
A table-driven test with one scenario is just boilerplate. Either fold this into a simple non-table test (like
TestGetSeparatedArgsForExec_EmptyCommandat line 126) or add meaningful cases—e.g., args present after--, multiple args after--, edge cases around dash positioning.♻️ Simplified shape
-func TestGetSeparatedArgsForExec(t *testing.T) { - tests := []struct { - name string - setup func(*cobra.Command) - expected []string - }{ - { - name: "no separator", - setup: func(cmd *cobra.Command) { - // No args set. - }, - expected: nil, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cmd := &cobra.Command{Use: "test"} - tt.setup(cmd) - - result := getSeparatedArgsForExec(cmd) - - if tt.expected == nil { - assert.Nil(t, result) - } else { - assert.Equal(t, tt.expected, result) - } - }) - } +func TestGetSeparatedArgsForExec_NoSeparator(t *testing.T) { + cmd := &cobra.Command{Use: "test"} + result := getSeparatedArgsForExec(cmd) + assert.Nil(t, result) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/exec_test.go` around lines 20 - 49, The table-driven TestGetSeparatedArgsForExec contains only one scenario and should be simplified or expanded: either convert it into a simple unit test (e.g., TestGetSeparatedArgsForExec_EmptyCommand) that directly constructs a cobra.Command, calls getSeparatedArgsForExec and asserts nil, or keep the table but add meaningful cases (no separator, args before/after "--", multiple args after "--", edge cases like "--" at start/end) and update the tests loop to assert expected slices; locate the test function TestGetSeparatedArgsForExec and the helper under test getSeparatedArgsForExec to implement the chosen change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/auth/env_test.go`:
- Around line 84-99: The stdout redirection and pipe handles in the tests around
outputEnvAsExport are not cleaned up if assertions fail; replace the current
manual restore/close logic with unconditional cleanup using t.Cleanup (or a
shared helper): after creating r,w and saving oldStdout, call t.Cleanup to
restore os.Stdout and to close both w and r, and ensure w is closed before
copying from r; apply the same pattern to the other similar blocks (lines
mentioned) so os.Stdout is always restored and both pipe ends are always closed.
- Around line 397-412: Tests in cmd/auth/env_test.go use hardcoded Unix paths
(e.g. "/tmp/out.sh", "/explicit/path") which breaks cross-platform runs; update
the test cases around resolveEnvOutputFile to build expected paths with
t.TempDir() and filepath.Join (importing "path/filepath" if missing), e.g.
create tmp := t.TempDir() then use filepath.Join(tmp, "out.sh") for assertions
and similarly replace other hardcoded assertions noted (lines ~447-456) so the
tests are platform-agnostic; keep the same resolveEnvOutputFile calls and
assertions otherwise.
In `@cmd/auth/logout_test.go`:
- Around line 471-496: Tests are mutating the shared authLogoutCmd (used with
ParseFlags), so wrap each test with a test kit to isolate command state: call
authLogoutCmd.NewTestKit(t) at the start of each test to obtain an isolated cmd
instance and use that returned cmd for ParseFlags and when passing into
executeAuthLogoutCommand (replace direct references to authLogoutCmd with the
kit-provided cmd), ensuring flags/args are cleaned up after the test.
In `@cmd/auth/markdown/atmos_auth_console_usage.md`:
- Line 5: The Markdown code block fences are malformed: closing fences use
"```shell" instead of plain "```", which breaks rendering; open the file and
replace each incorrect closing fence string "```shell" (the ones intended to
close the code blocks) with a plain closing fence "```" for all occurrences so
each code block has a matching opening (```shell) and a plain closing (```).
In `@cmd/auth/shell.go`:
- Around line 187-189: The error returned from
authManager.PrepareShellEnvironment is currently wrapped with a raw fmt.Errorf
which bypasses the repository's static sentinel errors; change the return to
wrap using the repository's defined static error from errors/errors.go (replace
the fmt.Errorf("failed to prepare shell environment: %w", err) with wrapping the
original err using the matching sentinel, e.g. errors.ErrPrepareShellEnvironment
or the exact sentinel name in errors/errors.go) so that callers can use
errors.Is; keep the original err as the cause when wrapping.
- Around line 78-86: The current pre-`--` check only catches when there is no
"--" (cmd.ArgsLenAtDash() == -1) but misses cases where positional args appear
before the separator (e.g., ArgsLenAtDash() > 0) and they get dropped by
getSeparatedArgs; change the condition to reject any invocation with positional
args before the dash (i.e., len(args) > 0 && cmd.ArgsLenAtDash() >= 0) and
update the returned hint to recommend using the --shell flag (not `--`) for
selecting a shell binary; keep the error type errUtils.ErrInvalidArguments and
adjust the fmt.Errorf message accordingly so callers see the correct
remediation.
---
Nitpick comments:
In `@cmd/auth/console_test.go`:
- Around line 387-393: The two tests TestConsoleLabelWidth and
TestConsoleOutputFormat are tautological and should be removed to reduce noise;
delete the functions referencing ConsoleLabelWidth and ConsoleOutputFormat from
cmd/auth/console_test.go (or replace them with a single documented assertion if
you need an explicit API contract), or alternatively convert them into a comment
documenting that these constants are part of the public contract if you must
preserve their intent. Ensure you remove references to the symbols
ConsoleLabelWidth and ConsoleOutputFormat in those test functions (or replace
with a single explanatory comment) to satisfy the review.
- Around line 245-256: The test switches on tt.name to detect the "invalid
provider duration format" case, which is fragile; add an explicit field to the
test case struct (e.g., expectParseError bool or specialCase string) and set it
in the specific test case instead of relying on tt.name, then update the
switch/conditional logic in the test to check that new field (referencing
tt.name, tt.expectedError, and the switch block) so the special handling remains
correct if test names change.
- Around line 1-688: The test file exceeds the 600-line guideline; split related
test groups into smaller files to improve maintainability: move identity-related
tests (e.g., TestResolveIdentityName_Console, TestRetrieveCredentials,
TestRetrieveCredentials_NoCredentialsAvailable,
TestRetrieveCredentials_InlineCredentials) into console_identity_test.go; move
provider and provider-kind tests (e.g., TestGetConsoleProvider,
TestDestinationFlagCompletion) into console_provider_test.go; move
duration/config tests (e.g., TestResolveConsoleDuration,
TestResolveConsoleIsolated, TestConsoleLabelWidth, TestConsoleOutputFormat) into
console_duration_test.go; move browser/opener tests (e.g., fakeOpener,
TestHandleBrowserOpen, TestConsoleSessionDir, TestPrintConsoleHelpers) into
console_browser_test.go; and keep orchestration/command-level tests (e.g.,
TestAuthConsoleCommand_Structure, TestExecuteAuthConsoleCommand_SmokeNoConfig,
TestExecuteAuthConsoleCommand_WithMockAuth,
TestInitializeAuthManager_SmokeFromEmptyTempDir) in console_command_test.go —
ensure each new file imports the same packages and preserves setup helpers like
setupMockAuthFixture and uses the same gomock controller usage and test helper
symbols (authConsoleCmd, resolveConsoleDuration, getConsoleProvider,
handleBrowserOpen, executeAuthConsoleCommand, initializeAuthManager) so tests
compile unchanged.
In `@cmd/auth/exec_test.go`:
- Around line 20-49: The table-driven TestGetSeparatedArgsForExec contains only
one scenario and should be simplified or expanded: either convert it into a
simple unit test (e.g., TestGetSeparatedArgsForExec_EmptyCommand) that directly
constructs a cobra.Command, calls getSeparatedArgsForExec and asserts nil, or
keep the table but add meaningful cases (no separator, args before/after "--",
multiple args after "--", edge cases like "--" at start/end) and update the
tests loop to assert expected slices; locate the test function
TestGetSeparatedArgsForExec and the helper under test getSeparatedArgsForExec to
implement the chosen change.
In `@cmd/auth/logout_test.go`:
- Around line 465-467: Add a one-line comment above the unexported helper
function realmInfoMatcher() indicating its purpose as a test stub for mock
expectations (e.g., "returns a stub RealmInfo used in mock expectations"); this
clarifies intent without exposing the helper publicly and makes test code easier
to read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52ed0a5a-2167-4c2b-8fbe-971b6614bc45
📒 Files selected for processing (27)
cmd/auth/completion.gocmd/auth/completion_test.gocmd/auth/console.gocmd/auth/console_test.gocmd/auth/env_test.gocmd/auth/exec.gocmd/auth/exec_test.gocmd/auth/helpers_test.gocmd/auth/identity_resolution_test.gocmd/auth/list.gocmd/auth/list_test.gocmd/auth/login.gocmd/auth/login_test.gocmd/auth/logout.gocmd/auth/logout_test.gocmd/auth/markdown/atmos_auth_console_usage.mdcmd/auth/markdown/atmos_auth_list_usage.mdcmd/auth/markdown/atmos_auth_logout_usage.mdcmd/auth/markdown/atmos_auth_shell_usage.mdcmd/auth/shell.gocmd/auth/shell_test.gocmd/auth/testmain_test.gocmd/auth/user/configure.gocmd/auth/validate.gocmd/auth/validate_test.gocmd/auth/whoami_test.gocmd/identity_helpers.go
✅ Files skipped from review due to trivial changes (1)
- cmd/auth/markdown/atmos_auth_logout_usage.md
🚧 Files skipped from review as they are similar to previous changes (17)
- cmd/auth/identity_resolution_test.go
- cmd/auth/testmain_test.go
- cmd/identity_helpers.go
- cmd/auth/validate_test.go
- cmd/auth/user/configure.go
- cmd/auth/login_test.go
- cmd/auth/validate.go
- cmd/auth/shell_test.go
- cmd/auth/whoami_test.go
- cmd/auth/list_test.go
- cmd/auth/completion_test.go
- cmd/auth/helpers_test.go
- cmd/auth/list.go
- cmd/auth/login.go
- cmd/auth/console.go
- cmd/auth/completion.go
- cmd/auth/logout.go
Addresses six CodeRabbit review comments from the latest batch:
1. cmd/auth/console_test.go — drop tautological constant tests.
TestConsoleLabelWidth and TestConsoleOutputFormat just asserted that
a literal equals itself. The constants are exported but only used
internally; not part of an external API contract.
2. cmd/auth/console_test.go — fragile test-name dispatch.
TestResolveConsoleDuration's "invalid provider duration format" case
keyed its assertion off tt.name. Renaming the case would silently
skip the error check. Added an explicit `expectParseError bool`
field; switch case now keys off that.
3. cmd/auth/exec_test.go — single-case table replaced.
TestGetSeparatedArgsForExec was a one-case table that duplicated the
sibling TestGetSeparatedArgsForExec_EmptyCommand. Rewrote with five
meaningful cases (no separator, positional-without-sep, separator+
single command, separator+command+args, separator-only). Deleted the
now-redundant sibling test.
4. cmd/auth/env_test.go — safe stdout-capture helper.
Five sites used the same fragile capture pattern: if require.NoError
aborted before restoration, os.Stdout stayed redirected and the read
end of os.Pipe was never closed. Added captureStdout(t) helper in
helpers_test.go that registers t.Cleanup to guarantee restoration
and close both pipe ends even when intervening assertions abort.
All five sites now use `read := captureStdout(t)` / `output := read()`.
5. cmd/auth/env_test.go — replace hardcoded Unix paths with t.TempDir().
Three paths (/tmp/out.sh, /explicit/path, /path/to/.env) replaced
with filepath.Join(t.TempDir(), ...) so the tests don't bake in a
Unix separator.
6. cmd/auth/markdown/atmos_auth_console_usage.md +
cmd/auth/markdown/atmos_auth_logout_usage.md — fix malformed
closers. An earlier awk script that added the shell language
identifier to opening fences had a state-machine flaw on files that
already mixed labeled/unlabeled fences; six closing fences across
two files were rewritten as ```shell instead of plain ```. Repaired
to keep the rest of the doc rendering correctly. Audited all four
auth markdown files; the other two were already correctly paired.
7. cmd/auth/logout_test.go + sibling _WithMockAuth tests — add a
cmd-state isolation helper. Tests that mutate package-level
auth*Cmd via ParseFlags could leak flag.Changed / flag values to
subsequent tests. cmd.NewTestKit isn't reachable from cmd/auth
without a circular import, so added a local equivalent
resetAuthCmdFlags(t, cmd) that snapshots and restores every flag's
(Value, Changed) pair via t.Cleanup. Applied to all 8 tests that
ParseFlags a shared cmd: TestExecuteAuthLogoutCommand_SmokeNoConfig
and _WithMockAuthDryRun (the two reviewer-flagged sites), plus
_WithMockAuth variants for console, env, exec
(TestPrepareAuthenticatedEnv_WithMockAuth + TestExecuteAuthExecCommand_NoCommand),
list (tree + JSON), login, validate, whoami.
8. cmd/auth/shell.go — tighten the pre-dash arg check and fix the hint.
The previous check only caught the no-separator case; `atmos auth
shell bash -- -lc env` had ArgsLenAtDash() == 1 and slipped through
while getSeparatedArgs silently dropped "bash". Condition is now
`dashIndex == -1 || dashIndex > 0` so positional args appearing
before "--" are also rejected. The error message no longer suggests
`-- bash` (which would just feed "bash" as the first arg to the
default shell) — it points at `--shell` for the shell-binary
override and "--" only for the shell args.
Extracted into validateAuthShellArgs to keep executeAuthShellCommand
under the 60-line revive limit. Added TestValidateAuthShellArgs with
5 subtests covering the three acceptable and two rejected arg
shapes plus the ErrInvalidArguments sentinel contract.
9. cmd/auth/shell.go + cmd/auth/exec.go — wrap PrepareShellEnvironment
failures with a static sentinel. Both helpers had a raw
fmt.Errorf("failed to prepare ...: %w", err) that didn't satisfy the
repo-wide "All errors MUST be wrapped using static errors defined in
errors/errors.go" rule. Added new sentinel
errUtils.ErrPrepareShellEnvironment and applied to both call sites.
Updated TestPrepareShellEnvironment to assert the sentinel is in the
chain (errors.Is) plus the original underlying error.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Release Documentation RequiredThis PR is labeled
|
1 similar comment
|
Warning Release Documentation RequiredThis PR is labeled
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/auth/console_test.go`:
- Around line 646-660: The test TestExecuteAuthConsoleCommand_WithMockAuth
currently calls resetAuthCmdFlags(t, cmd) and mutates shared
authConsoleCmd/RootCmd state; replace the manual flag reset by creating a test
kit for isolation: call cmd.NewTestKit(t) at the start of the test (and use its
cleanup) before using authConsoleCmd so RootCmd flags/args are auto-restored,
remove the resetAuthCmdFlags(t, cmd) invocation, and then proceed to SetContext,
ParseFlags and executeAuthConsoleCommand as before.
- Around line 629-639: TestExecuteAuthConsoleCommand_SmokeNoConfig currently
calls authConsoleCmd directly which can leak RootCmd flag/arg state; wrap the
test with cmd.NewTestKit(t) to create and defer-clean a test kit before using
authConsoleCmd and calling executeAuthConsoleCommand so RootCmd state is
isolated; specifically, initialize kt := cmd.NewTestKit(t) (or equivalent per
project helper) at the start of the test and use that test kit's setup/teardown
around the call to executeAuthConsoleCommand(authConsoleCmd, nil).
In `@cmd/auth/exec_test.go`:
- Around line 173-183: The smoke test reuses the package-level authExecCmd
without resetting shared flag/context state; call resetAuthCmdFlags() before
using authExecCmd in TestExecuteAuthExecCommand_SmokeNoConfig (and the other
smoke test that reuses authExecCmd) so the command flags and context are
cleared, then set the context and call executeAuthExecCommand as before;
reference authExecCmd, resetAuthCmdFlags, and executeAuthExecCommand when making
the change.
In `@cmd/auth/login_test.go`:
- Around line 216-225: The test uses the package-global authLoginCmd without
resetting its shared flags/state, making it order-dependent; fix by copying or
resetting that global before use: create a local cmd variable from authLoginCmd
(e.g., cmd := authLoginCmd) and then reset its flag/state (for example iterate
cmd.Flags()/cmd.PersistentFlags() with VisitAll and set each flag back to its
DefValue, or use any existing helper that resets command flags) before calling
cmd.SetContext(...) and executeAuthLoginCommand(cmd, nil). Ensure the unique
symbols authLoginCmd and executeAuthLoginCommand are used so the change targets
this test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06aa772b-a389-4574-acc1-6b07948ec3a8
📒 Files selected for processing (15)
cmd/auth/console_test.gocmd/auth/env_test.gocmd/auth/exec.gocmd/auth/exec_test.gocmd/auth/helpers_test.gocmd/auth/list_test.gocmd/auth/login_test.gocmd/auth/logout_test.gocmd/auth/markdown/atmos_auth_console_usage.mdcmd/auth/markdown/atmos_auth_logout_usage.mdcmd/auth/shell.gocmd/auth/shell_test.gocmd/auth/validate_test.gocmd/auth/whoami_test.goerrors/errors.go
✅ Files skipped from review due to trivial changes (1)
- cmd/auth/markdown/atmos_auth_logout_usage.md
🚧 Files skipped from review as they are similar to previous changes (10)
- errors/errors.go
- cmd/auth/shell.go
- cmd/auth/exec.go
- cmd/auth/helpers_test.go
- cmd/auth/validate_test.go
- cmd/auth/shell_test.go
- cmd/auth/list_test.go
- cmd/auth/env_test.go
- cmd/auth/logout_test.go
- cmd/auth/whoami_test.go
…isolation Add `resetAuthCmdFlags(t, cmd)` to every test that uses a package-level `auth*Cmd` so flag/Changed state cannot leak across tests under shuffled runs. This is the in-package equivalent of `cmd.NewTestKit(t)` — `cmd/auth` cannot import `cmd` (cmd/root.go blank-imports cmd/auth) and `NewTestKit` is `_test.go`-scoped, so the local helper enforces the same auto-cleanup contract via t.Cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
May 12, 2026
Contributor
|
These changes were released in v1.218.0-rc.2. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Refactors the
atmos authcommand family from the legacy flat-file structure to the Command Registry pattern with unified flag handling viapkg/flags/StandardParser. As a side-effect of the refactor, this PR also resolves two long-standing flag-precedence bugs (#1973 and #2392) and adds regression tests so they cannot silently regress again.cmd/auth_*.goto an organisedcmd/auth/package with aCommandProviderinterface.viper.BindPFlag()/viper.BindEnv()calls withflags.NewStandardParser(...)+parser.BindToViper(...)(Forbidigo-compliant).DisableFlagParsingwith theSeparatedArgspattern for pass-through commands (auth shell,auth exec).auth userto a real subcommand package (cmd/auth/user/).Why
viper.BindPFlag()/viper.BindEnv()calls.BuildConfigAndStacksInfo(cmd, v)so global flags (--base-path,--config,--config-path,--profile) are honoured uniformly.CommandProvider.cmd/authrose from 33% → 47% in this PR.Closes
Closes #1973 —
--profilenot applied forauth execandauth shellRoot cause: the legacy
executeAuthExecCommandCore/executeAuthShellCommandCoreloaded the atmos config fromcfg.InitCliConfig(newAuthConfigAndStacksInfo(cmd), false)— a helper that only read--base-path/--config/--config-pathand dropped--profileon the floor.auth list/auth env/auth whoamihappened to call a different helper that did read profile, hence the asymmetric bug.Fix: both
cmd/auth/exec.goandcmd/auth/shell.gonow callBuildConfigAndStacksInfo(cmd, v)(a wrapper overflags.BuildConfigAndStacksInfo) which extracts all global flags including--profileintoConfigAndStacksInfo.ProfilesFromArgbeforecfg.InitCliConfig.Regression tests:
cmd/auth/exec_test.go::TestAuthExec_ProfileFlagAppliedToConfigcmd/auth/shell_test.go::TestAuthShell_ProfileFlagAppliedToConfigBoth cover single-profile, multi-profile, and no-profile cases.
Closes #2392 —
--identitysilently dropped onatmos terraform planRoot cause: the bug as reported at v1.216.0 stemmed from
--identitybeing parsed inconsistently between command families. Terraform commands relied solely on the legacy arg-walkerparseIdentityFlagwhile auth/describe used a Cobra-flag-based path.Fix: terraform commands now register
--identityvia the StandardParser (cmd/terraform/flags.go::registerIdentityFlags) and the legacy arg-walker still populatesinfo.Identityfor backwards compat — so both paths converge on the same value.setupTerraformAuth(ininternal/exec/terraform_execute_helpers.go) passesinfo.Identityverbatim to the auth manager creator;pkg/auth.resolveIdentityNamereturns it as-is when non-empty, so the explicit flag value can never be overridden by a profile-default identity.Regression tests:
internal/exec/cli_utils_test.go::TestProcessCommandLineArgs_TerraformIdentityFlag_Issue2392— replays the exact arg shape from the bug report (terraform plan account-map -s core-gbl-root --identity core-root/admin) and assertsinfo.Identity == "core-root/admin".internal/exec/terraform_execute_helpers_auth_test.go::TestSetupTerraformAuth_IdentityFlagPropagatesToAuthCreator— builds a merged auth config with both adefault: trueidentity and a non-default identity and assertssetupTerraformAuthpasses the explicit--identityvalue verbatim.internal/exec/terraform_execute_helpers_auth_test.go::TestSetupTerraformAuth_EmptyIdentity_AllowsAutoDetection— inverse guard: emptyinfo.Identitymust still allow auto-detect.Testing
go build ./...,go vet ./...,make lint— all clean.go test -short ./cmd/... ./internal/... ./pkg/...— all pass.cmd/auth33.0% → 47.6%,cmd/auth/user35.6% → 51.0%.--profileglobal flag not applied forauth execandauth shellcommands #1973 and--identityflag silently ignored byatmos terraform plan(works onatmos authandatmos describesubcommands) #2392.Summary by CodeRabbit
New Features
Improvements
Documentation