Add auth logout command #1656
Add auth logout command #1656
Conversation
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces comprehensive auth logout functionality for Atmos, including a new CLI command ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Command
participant Manager as AuthManager
participant KS as CredentialStore
participant Provider
participant Identity
participant Files as File Manager
User->>CLI: atmos auth logout [identity]
alt Identity-specific logout
CLI->>Manager: Logout(ctx, identityName)
Manager->>Manager: resolveProviderForIdentity(identityName)
loop For each credential in chain
Manager->>KS: Delete(alias)
end
Manager->>Identity: Logout(ctx)
alt Identity cleanup
Identity->>Files: CleanupIdentity(provider, identity)
Files->>Files: Remove INI sections
end
Manager->>Provider: Logout(ctx)
alt Provider cleanup
Provider->>Files: Cleanup(provider)
Files->>Files: Remove credential files
end
Manager-->>CLI: Success or ErrPartialLogout
else Provider-specific logout
CLI->>Manager: LogoutProvider(ctx, providerName)
Manager->>Manager: Get all identities for provider
loop For each identity
Manager->>Manager: Logout(ctx, identityName)
end
Manager->>Provider: Logout(ctx)
Manager-->>CLI: Result
else All identities
CLI->>Manager: LogoutAll(ctx)
Manager->>Manager: Get all providers
loop For each provider
Manager->>Manager: LogoutProvider(ctx, providerName)
end
Manager-->>CLI: Aggregated results
end
alt Dry-run mode
CLI->>CLI: Display cleanup plan
else Browser warning
CLI->>CLI: Show browser session notice
end
CLI-->>User: Display results / errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR involves substantial, multi-faceted changes across the auth system. Key complexity drivers:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (25)
pkg/auth/identities/aws/permission_set.go (1)
267-274: Missing performance tracking call.Per coding guidelines, add
defer perf.Track()to all public functions. This applies to the new Logout method.Apply this diff:
// Logout removes identity-specific credential storage. func (i *permissionSetIdentity) Logout(ctx context.Context) error { + defer perf.Track() + // AWS permission-set identities don't have identity-specific storage. // File cleanup is handled by the provider's Logout method. // Keyring cleanup is handled by AuthManager. log.Debug("Logout called for permission-set identity (no identity-specific cleanup)", "identity", i.name) return nil }Note: You'll need to import the perf package if not already present.
Based on coding guidelines
pkg/auth/identities/aws/assume_role.go (1)
278-285: Missing performance tracking call.Per coding guidelines, add
defer perf.Track()to all public functions.Apply this diff:
// Logout removes identity-specific credential storage. func (i *assumeRoleIdentity) Logout(ctx context.Context) error { + defer perf.Track() + // AWS assume-role identities don't have identity-specific storage. // File cleanup is handled by the provider's Logout method. // Keyring cleanup is handled by AuthManager. log.Debug("Logout called for assume-role identity (no identity-specific cleanup)", "identity", i.name) return nil }Based on coding guidelines
pkg/auth/providers/aws/saml.go (1)
373-387: Missing performance tracking call.Per coding guidelines, add
defer perf.Track()to all public functions. Also consider passing ctx to track if needed.Apply this diff:
// Logout removes provider-specific credential storage. func (p *samlProvider) Logout(ctx context.Context) error { + defer perf.Track() + fileManager, err := awsCloud.NewAWSFileManager() if err != nil { return errors.Join(errUtils.ErrLogoutFailed, err) } if err := fileManager.Cleanup(p.name); err != nil { log.Debug("Failed to cleanup AWS files for SAML provider", "provider", p.name, "error", err) return errors.Join(errUtils.ErrLogoutFailed, err) } log.Debug("Cleaned up AWS files for SAML provider", "provider", p.name) return nil }Based on coding guidelines
pkg/auth/cloud/aws/files.go (2)
192-206: Missing performance tracking call.Per coding guidelines, add
defer perf.Track()to all public functions.Apply this diff:
// Cleanup removes AWS files for the provider. func (m *AWSFileManager) Cleanup(providerName string) error { + defer perf.Track() + providerDir := filepath.Join(m.baseDir, providerName) if err := os.RemoveAll(providerDir); err != nil { // If directory doesn't exist, that's not an error (already cleaned up). if os.IsNotExist(err) { return nil } errUtils.CheckErrorAndPrint(ErrCleanupAWSFiles, providerDir, "failed to cleanup AWS files") return ErrCleanupAWSFiles } return nil }Based on coding guidelines
208-220: Missing performance tracking call.Per coding guidelines, add
defer perf.Track()to all public functions.Apply this diff:
// CleanupAll removes entire ~/.aws/atmos directory. func (m *AWSFileManager) CleanupAll() error { + defer perf.Track() + if err := os.RemoveAll(m.baseDir); err != nil { // If directory doesn't exist, that's not an error (already cleaned up). if os.IsNotExist(err) { return nil } errUtils.CheckErrorAndPrint(ErrCleanupAWSFiles, m.baseDir, "failed to cleanup all AWS files") return ErrCleanupAWSFiles } return nil }Based on coding guidelines
website/blog/2025-10-17-auth-logout-feature.md (7)
40-58: Add language identifier to fenced code block.Code blocks should specify a language for proper syntax highlighting.
Apply this diff:
**Example output:** -``` +```text Logging out from identity: dev-adminAs per coding guidelines
67-74: Add language identifier to fenced code block.Apply this diff:
When run without arguments for an interactive experience: -``` +```text ? Choose what to logout from:As per coding guidelines
88-99: Add language identifier to fenced code block.Apply this diff:
This removes the provider credentials and all identities that authenticate through it: -``` +```text Logging out from provider: aws-ssoAs per coding guidelines
109-120: Add language identifier to fenced code block.Apply this diff:
Preview what would be removed without actually deleting anything: -``` +```text Dry run mode: No credentials will be removedAs per coding guidelines
128-135: Add language identifier to fenced code block.Apply this diff:
Atmos intelligently resolves the complete authentication chain for your identity and removes credentials at each step: -``` +```text aws-sso → dev-org-admin → dev-admin ↓ ↓ ↓ Removed Removed RemovedAs per coding guidelines
149-164: Add language identifier to fenced code block.Apply this diff:
The logout command continues even if individual steps fail, ensuring maximum cleanup: -``` +```text Logging out from identity: dev-adminAs per coding guidelines
209-214: Add language identifier to fenced code block.Apply this diff:
All logout operations are logged for security auditing: -``` +```text 2025-10-17T10:15:30Z DEBUG Starting logout identity=dev-adminAs per coding guidelines
pkg/auth/manager.go (3)
756-763: Call Identity.Logout for every identity in the chain, not just the target.Only the target identity’s Logout is invoked. Intermediate identities may have provider/identity-specific artifacts to clean.
Apply this diff to replace the single call with chain-wide best‑effort cleanup:
- // Step 3: Call identity-specific cleanup. - if identity, exists := m.identities[identityName]; exists { - if err := identity.Logout(ctx); err != nil { - log.Debug("Identity logout failed", "identity", identityName, "error", err) - errs = append(errs, errors.Join(errUtils.ErrIdentityLogout, fmt.Errorf("identity %q: %v", identityName, err))) - } - } + // Step 3: Call identity-specific cleanup for all identities in the chain (skip provider element at index 0). + for i := 1; i < len(chain); i++ { + step := chain[i] + if identity, exists := m.identities[step]; exists { + if err := identity.Logout(ctx); err != nil { + log.Debug("Identity logout failed", "identity", step, "error", err) + errs = append(errs, errors.Join(errUtils.ErrIdentityLogout, fmt.Errorf("identity %q: %v", step, err))) + } else { + log.Debug("Identity logout successful", "identity", step) + } + } + }
800-831: Propagate partial‑success semantics in LogoutProvider.Current logic returns ErrLogoutFailed if any error occurs, even if some deletions succeed. Match PRD: return ErrPartialLogout when at least one operation succeeds.
Apply this diff:
- var errs []error + var errs []error + succeeded := 0 ... - for _, identityName := range identityNames { - if err := m.Logout(ctx, identityName); err != nil { + for _, identityName := range identityNames { + if err := m.Logout(ctx, identityName); err != nil { log.Debug("Failed to logout identity", "identity", identityName, "error", err) errs = append(errs, errors.Join(errUtils.ErrIdentityLogout, fmt.Errorf("identity %q: %v", identityName, err))) - } + if errors.Is(err, errUtils.ErrPartialLogout) { + succeeded++ + } + } else { + succeeded++ + } } ... - if err := m.credentialStore.Delete(providerName); err != nil { + if err := m.credentialStore.Delete(providerName); err != nil { log.Debug("Failed to delete provider keyring entry", "provider", providerName, "error", err) errs = append(errs, errors.Join(errUtils.ErrKeyringDeletion, fmt.Errorf("provider %q: %v", providerName, err))) - } + } else { + succeeded++ + } ... - if provider, exists := m.providers[providerName]; exists { + if provider, exists := m.providers[providerName]; exists { if err := provider.Logout(ctx); err != nil { log.Debug("Provider logout failed", "provider", providerName, "error", err) errs = append(errs, errors.Join(errUtils.ErrProviderLogout, fmt.Errorf("provider %q: %v", providerName, err))) - } + } else { + succeeded++ + } } ... - if len(errs) > 0 { - return errors.Join(append([]error{errUtils.ErrLogoutFailed}, errs...)...) - } + if len(errs) > 0 { + if succeeded > 0 { + return errors.Join(append([]error{errUtils.ErrPartialLogout}, errs...)...) + } + return errors.Join(append([]error{errUtils.ErrLogoutFailed}, errs...)...) + }
841-856: Propagate partial‑success semantics in LogoutAll.Same issue as provider path; always returning ErrLogoutFailed on any error.
Apply this diff:
- var errs []error + var errs []error + succeeded := 0 ... - for identityName := range m.config.Identities { - if err := m.Logout(ctx, identityName); err != nil { + for identityName := range m.config.Identities { + if err := m.Logout(ctx, identityName); err != nil { log.Debug("Failed to logout identity", "identity", identityName, "error", err) errs = append(errs, errors.Join(errUtils.ErrIdentityLogout, fmt.Errorf("identity %q: %v", identityName, err))) - } + if errors.Is(err, errUtils.ErrPartialLogout) { + succeeded++ + } + } else { + succeeded++ + } } ... - if len(errs) > 0 { - return errors.Join(append([]error{errUtils.ErrLogoutFailed}, errs...)...) - } + if len(errs) > 0 { + if succeeded > 0 { + return errors.Join(append([]error{errUtils.ErrPartialLogout}, errs...)...) + } + return errors.Join(append([]error{errUtils.ErrLogoutFailed}, errs...)...) + }After changes, add tests that assert ErrPartialLogout on mixed outcomes.
cmd/auth_logout.go (5)
76-128: Instrument helpers with perf.Track and handle partial‑success for identity.Add perf.Track and keep existing partial‑success printing. Also wrap interactive errors consistently (see another comment).
Apply this diff header for perf:
func performIdentityLogout(ctx context.Context, authManager auth.AuthManager, identityName string, dryRun bool) error { + defer perf.Track(nil, "cmd.AuthLogout.performIdentityLogout")()
129-168: Instrument provider path and honor partial‑success semantics.Print “logged out with warnings” and exit 0 when ErrPartialLogout is returned.
Apply this diff:
func performProviderLogout(ctx context.Context, authManager auth.AuthManager, providerName string, dryRun bool) error { + defer perf.Track(nil, "cmd.AuthLogout.performProviderLogout")() ... - if err := authManager.LogoutProvider(ctx, providerName); err != nil { - u.PrintfMessageToTUI("✗ **Provider logout failed**\n\n") - u.PrintfMessageToTUI("Error: %v\n\n", err) - return err - } + if err := authManager.LogoutProvider(ctx, providerName); err != nil { + if errors.Is(err, errUtils.ErrPartialLogout) { + u.PrintfMessageToTUI("✓ **Provider logout completed with warnings**\n\n") + u.PrintfMessageToTUI("Some credentials could not be removed:\n %v\n\n", err) + } else { + u.PrintfMessageToTUI("✗ **Provider logout failed**\n\n") + u.PrintfMessageToTUI("Error: %v\n\n", err) + return err + } + }
247-274: Instrument logout-all and honor partial‑success semantics.Match PRD exit behavior for partial success.
Apply this diff:
func performLogoutAll(ctx context.Context, authManager auth.AuthManager, dryRun bool) error { + defer perf.Track(nil, "cmd.AuthLogout.performLogoutAll")() ... - if err := authManager.LogoutAll(ctx); err != nil { - u.PrintfMessageToTUI("✗ **Logout all failed**\n\n") - u.PrintfMessageToTUI("Error: %v\n\n", err) - return err - } + if err := authManager.LogoutAll(ctx); err != nil { + if errors.Is(err, errUtils.ErrPartialLogout) { + u.PrintfMessageToTUI("✓ **Logout all completed with warnings**\n\n") + u.PrintfMessageToTUI("Some credentials could not be removed:\n %v\n\n", err) + } else { + u.PrintfMessageToTUI("✗ **Logout all failed**\n\n") + u.PrintfMessageToTUI("Error: %v\n\n", err) + return err + } + }
170-245: Wrap interactive errors and add proper argument/flag completion.
- form.Run() errors should be wrapped with a static sentinel.
- ComponentsArgCompletion is unrelated; provide identity/provider completions.
Apply this diff:
- if err := form.Run(); err != nil { - return err - } + if err := form.Run(); err != nil { + return errors.Join(errUtils.ErrUnsupportedInputType, err) + }And replace the completion:
- ValidArgsFunction: ComponentsArgCompletion, + ValidArgsFunction: authIdentityCompletion,Add the implementations:
+// authIdentityCompletion suggests identity names. +func authIdentityCompletion(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + ac, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) + if err != nil { + return nil, cobra.ShellCompDirectiveNoFileComp + } + names := make([]string, 0, len(ac.Auth.Identities)) + for n := range ac.Auth.Identities { + if strings.HasPrefix(n, toComplete) { + names = append(names, n) + } + } + sort.Strings(names) + return names, cobra.ShellCompDirectiveNoFileComp +}Register provider flag completion in init():
func init() { authLogoutCmd.Flags().String("provider", "", "Logout from specific provider") authLogoutCmd.Flags().Bool("dry-run", false, "Preview what would be removed without deleting") + authLogoutCmd.RegisterFlagCompletionFunc("provider", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + ac, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) + if err != nil { + return nil, cobra.ShellCompDirectiveNoFileComp + } + out := make([]string, 0, len(ac.Auth.Providers)) + for n := range ac.Auth.Providers { + if strings.HasPrefix(n, toComplete) { + out = append(out, n) + } + } + sort.Strings(out) + return out, cobra.ShellCompDirectiveNoFileComp + }) authCmd.AddCommand(authLogoutCmd) }Add imports:
- "github.com/charmbracelet/huh" + "github.com/charmbracelet/huh" + "sort" + "strings"
145-152: Optional: cross‑platform path hints in UI.Hardcoding “~/.aws/atmos/…” can confuse Windows users. Consider deriving display paths with os.UserHomeDir() + filepath.Join and printing them.
Also applies to: 247-257
pkg/auth/manager_logout_test.go (1)
350-389: Add tests for partial‑success signaling in provider/all paths.After updating manager to return ErrPartialLogout when some steps succeed, assert errors.Is(err, ErrPartialLogout) in these cases.
Also applies to: 402-427
docs/prd/auth-logout.md (2)
147-155: Add language identifiers to fenced code blocks.Markdown lint MD040: specify a language for code fences.
Apply this diff:
-``` +```text ? Choose what to logout from: ❯ Identity: dev-admin ... -``` +``` ... -``` +```text ⚠️ Important: This command only removes locally cached credentials. ... -``` +```Also applies to: 594-605
130-141: Optional: add “Examples” section in CLI docs and mirror CLI output styling.Helps align UX, especially around partial-success exit behavior.
Also applies to: 184-198
pkg/auth/types/mock_interfaces.go (2)
7-14: Import groups: stdlib, third‑party, Atmos.Project guideline wants three groups. Consider post‑gen formatting so this file stays compliant without manual edits.
Apply after-generation reordering (example):
import ( context "context" reflect "reflect" time "time" - schema "github.com/cloudposse/atmos/pkg/schema" - gomock "github.com/golang/mock/gomock" + gomock "github.com/golang/mock/gomock" + + schema "github.com/cloudposse/atmos/pkg/schema" )
1-1: Reproducible generation: pin mockgen + go:generate.To avoid drift, pin mockgen and add a go:generate in interfaces.go (not this file).
Example (in pkg/auth/types/interfaces.go):
//go:generate go run github.com/golang/mock/[email protected] -source=interfaces.go -destination=mock_interfaces.go -package=typesAnd pin in tools.go:
//go:build tools package tools import _ "github.com/golang/mock/mockgen"
📜 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 (22)
cmd/auth_logout.go(1 hunks)docs/prd/auth-logout.md(1 hunks)errors/errors.go(1 hunks)pkg/auth/cloud/aws/files.go(1 hunks)pkg/auth/cloud/aws/files_logout_test.go(1 hunks)pkg/auth/hooks_test.go(1 hunks)pkg/auth/identities/aws/assume_role.go(1 hunks)pkg/auth/identities/aws/permission_set.go(1 hunks)pkg/auth/identities/aws/user.go(1 hunks)pkg/auth/identities/aws/user_test.go(1 hunks)pkg/auth/manager.go(1 hunks)pkg/auth/manager_logout_test.go(1 hunks)pkg/auth/manager_test.go(4 hunks)pkg/auth/providers/aws/saml.go(2 hunks)pkg/auth/providers/aws/saml_test.go(1 hunks)pkg/auth/providers/aws/sso.go(1 hunks)pkg/auth/providers/github/oidc.go(1 hunks)pkg/auth/types/interfaces.go(4 hunks)pkg/auth/types/mock_interfaces.go(1 hunks)website/blog/2025-10-17-auth-logout-feature.md(1 hunks)website/docs/cli/commands/auth/logout.mdx(1 hunks)website/docs/cli/commands/auth/user-configure.mdx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/identities/aws/permission_set.gopkg/auth/providers/github/oidc.gopkg/auth/identities/aws/user.gopkg/auth/cloud/aws/files.gopkg/auth/manager_test.gopkg/auth/identities/aws/user_test.gopkg/auth/hooks_test.gopkg/auth/providers/aws/sso.gopkg/auth/providers/aws/saml_test.gopkg/auth/cloud/aws/files_logout_test.gopkg/auth/manager_logout_test.gopkg/auth/types/interfaces.gopkg/auth/identities/aws/assume_role.gopkg/auth/providers/aws/saml.gopkg/auth/manager.gopkg/auth/types/mock_interfaces.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
pkg/auth/identities/aws/permission_set.gopkg/auth/providers/github/oidc.gopkg/auth/identities/aws/user.gopkg/auth/cloud/aws/files.goerrors/errors.gopkg/auth/manager_test.gopkg/auth/identities/aws/user_test.gopkg/auth/hooks_test.gopkg/auth/providers/aws/sso.gopkg/auth/providers/aws/saml_test.gopkg/auth/cloud/aws/files_logout_test.gopkg/auth/manager_logout_test.gopkg/auth/types/interfaces.gopkg/auth/identities/aws/assume_role.gopkg/auth/providers/aws/saml.gocmd/auth_logout.gopkg/auth/manager.gopkg/auth/types/mock_interfaces.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Add
defer perf.Track()to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.
Files:
pkg/auth/identities/aws/permission_set.gopkg/auth/providers/github/oidc.gopkg/auth/identities/aws/user.gopkg/auth/cloud/aws/files.goerrors/errors.gopkg/auth/providers/aws/sso.gopkg/auth/types/interfaces.gopkg/auth/identities/aws/assume_role.gopkg/auth/providers/aws/saml.gocmd/auth_logout.gopkg/auth/manager.gopkg/auth/types/mock_interfaces.go
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in website/ when adding features
Ensure consistency between CLI help text and website documentation
Follow the website's documentation structure and style
Keep website code in website/ and follow its architecture/style; test changes locally
Keep CLI and website documentation in sync; document new features with examples and use casesBefore committing documentation/site changes, run npm run build in website/ and fix errors, broken links, and missing images.
Files:
website/docs/cli/commands/auth/user-configure.mdxwebsite/docs/cli/commands/auth/logout.mdxwebsite/blog/2025-10-17-auth-logout-feature.md
website/docs/cli/commands/**/*.mdx
📄 CodeRabbit inference engine (CLAUDE.md)
Document all new commands/flags/parameters in Docusaurus under website/docs/cli/commands/, using definition lists for args/flags and the prescribed frontmatter and sections.
Files:
website/docs/cli/commands/auth/user-configure.mdxwebsite/docs/cli/commands/auth/logout.mdx
website/docs/**
📄 CodeRabbit inference engine (CLAUDE.md)
website/docs/**: After modifying any docs under website/docs/, build the website (npm run build) and ensure no errors.
Document user-facing template functions in the website if applicable.
Files:
website/docs/cli/commands/auth/user-configure.mdxwebsite/docs/cli/commands/auth/logout.mdx
errors/errors.go
📄 CodeRabbit inference engine (CLAUDE.md)
Define all static errors centrally in errors/errors.go as package-level vars.
Files:
errors/errors.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/auth/manager_test.gopkg/auth/identities/aws/user_test.gopkg/auth/hooks_test.gopkg/auth/providers/aws/saml_test.gopkg/auth/cloud/aws/files_logout_test.gopkg/auth/manager_logout_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/auth/manager_test.gopkg/auth/identities/aws/user_test.gopkg/auth/hooks_test.gopkg/auth/providers/aws/saml_test.gopkg/auth/cloud/aws/files_logout_test.gopkg/auth/manager_logout_test.go
website/blog/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
website/blog/*.md: For PRs labeled minor or major, include a blog post in website/blog/YYYY-MM-DD-*.md following the template and include after the introduction.
Tag blog posts appropriately: user-facing (feature/enhancement/bugfix) or contributor (contributors) plus relevant technical tags.
Files:
website/blog/2025-10-17-auth-logout-feature.md
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown() to render embedded markdown content for CLI command help/examples.
One Cobra command per file in cmd/; keep files focused and small.
Files:
cmd/auth_logout.go
**/{mock_*.go,mock/*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Generate and keep mocks alongside the interfaces they mock; mocks are excluded from coverage.
Files:
pkg/auth/types/mock_interfaces.go
🧠 Learnings (1)
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/{mock_*.go,mock/*.go} : Generate and keep mocks alongside the interfaces they mock; mocks are excluded from coverage.
Applied to files:
pkg/auth/types/mock_interfaces.go
🧬 Code graph analysis (12)
pkg/auth/identities/aws/permission_set.go (1)
pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/providers/github/oidc.go (1)
pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/identities/aws/user.go (3)
pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(43-51)errors/errors.go (1)
ErrLogoutFailed(374-374)pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/cloud/aws/files.go (1)
errors/error_funcs.go (1)
CheckErrorAndPrint(29-52)
pkg/auth/providers/aws/sso.go (3)
pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(43-51)errors/errors.go (1)
ErrLogoutFailed(374-374)pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/cloud/aws/files_logout_test.go (1)
pkg/auth/cloud/aws/files.go (1)
AWSFileManager(38-40)
pkg/auth/manager_logout_test.go (4)
pkg/auth/types/mock_interfaces.go (6)
MockCredentialStore(501-504)MockProvider(17-20)MockIdentity(140-143)NewMockCredentialStore(512-516)NewMockProvider(28-32)NewMockIdentity(151-155)pkg/schema/schema_auth.go (2)
AuthConfig(4-8)IdentityVia(42-45)pkg/auth/types/interfaces.go (2)
Provider(13-37)Identity(40-64)errors/errors.go (4)
ErrIdentityNotInConfig(380-380)ErrPartialLogout(375-375)ErrLogoutFailed(374-374)ErrProviderNotInConfig(381-381)
pkg/auth/identities/aws/assume_role.go (1)
pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/providers/aws/saml.go (3)
pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(43-51)errors/errors.go (1)
ErrLogoutFailed(374-374)pkg/logger/log.go (1)
Debug(24-26)
cmd/auth_logout.go (6)
cmd/cmd_utils.go (1)
ComponentsArgCompletion(768-790)pkg/config/config.go (1)
InitCliConfig(25-62)errors/errors.go (6)
ErrFailedToInitializeAtmosConfig(117-117)ErrAuthManager(334-334)ErrIdentityNotInConfig(380-380)ErrPartialLogout(375-375)ErrProviderNotInConfig(381-381)ErrInvalidLogoutOption(382-382)pkg/auth/types/interfaces.go (1)
AuthManager(67-117)pkg/utils/log_utils.go (1)
PrintfMessageToTUI(33-35)internal/tui/utils/utils.go (1)
NewAtmosHuhTheme(85-93)
pkg/auth/manager.go (4)
pkg/perf/perf.go (1)
Track(121-138)pkg/logger/log.go (3)
Errorf(59-61)Debug(24-26)Info(34-36)errors/errors.go (7)
ErrIdentityNotInConfig(380-380)ErrKeyringDeletion(377-377)ErrProviderLogout(378-378)ErrIdentityLogout(379-379)ErrPartialLogout(375-375)ErrLogoutFailed(374-374)ErrProviderNotInConfig(381-381)pkg/auth/types/interfaces.go (1)
Provider(13-37)
pkg/auth/types/mock_interfaces.go (3)
pkg/schema/schema.go (3)
Context(381-396)Validate(188-190)ConfigAndStacksInfo(460-539)pkg/auth/types/interfaces.go (4)
ICredentials(152-158)AuthManager(67-117)Identity(40-64)Provider(13-37)pkg/auth/types/whoami.go (1)
WhoamiInfo(6-22)
🪛 LanguageTool
website/docs/cli/commands/auth/logout.mdx
[style] ~18-~18: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...ing Local Credentials Only This command only removes **credentials stored on your lo...
(ADVERB_REPETITION_PREMIUM)
[grammar] ~89-~89: Ensure spelling is correct
Context: ...s an interactive menu to choose what to logout from: ``` ? Choose what to logout from...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~217-~217: Ensure spelling is correct
Context: ...o completely end your session and fully logout: 1. Run atmos auth logout to remove local ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~346-~346: Ensure spelling is correct
Context: ...ation chain. ## Best Practices ### 1. Logout When Switching Contexts When switching...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~348-~348: Ensure spelling is correct
Context: ...n different identities or environments, logout first to ensure clean state: ```shell ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
website/blog/2025-10-17-auth-logout-feature.md
[grammar] ~178-~178: Please add a punctuation mark at the end of paragraph.
Context: ...he browser session 4. Close all browser windows The command displays this warning afte...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~307-~307: Please add a punctuation mark at the end of paragraph.
Context: ...ath` package for platform-specific path separators ### Error Handling Following Atmos er...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~316-~316: Please add a punctuation mark at the end of paragraph.
Context: ...on-fatal errors - Reports all errors at completion ### Interface Extensions The logout f...
(PUNCTUATION_PARAGRAPH_END)
docs/prd/auth-logout.md
[typographical] ~57-~57: Consider using typographic quotation marks here.
Context: ...elet Huh for styled selection - Allow "All identities" option - Apply Atmos theme to prompts...
(EN_QUOTES)
[grammar] ~626-~626: Ensure spelling is correct
Context: ...lement styled output with Charmbracelet lipgloss - [ ] Add progress indicators for multi-st...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[typographical] ~693-~693: Consider using typographic quotation marks here.
Context: ...upport --all flag or just interactive "All identities" option? Decision: Use interactive ...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
website/blog/2025-10-17-auth-logout-feature.md
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
67-67: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
149-149: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
209-209: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/prd/auth-logout.md
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
594-594: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (24)
pkg/auth/providers/aws/sso.go (1)
237-251: LGTM! Clean logout implementation.The method follows the established pattern: creates the file manager, performs cleanup, logs the outcome, and properly wraps errors with the sentinel.
pkg/auth/identities/aws/user_test.go (1)
79-79: LGTM! Test stub satisfies interface.The no-op Logout implementation on stubUser is appropriate for test isolation.
website/docs/cli/commands/auth/user-configure.mdx (1)
11-15: LGTM! Consistent doc component usage.Wrapping the introductory text in the Intro component aligns with the documentation pattern used elsewhere in the PR.
pkg/auth/identities/aws/user.go (1)
356-372: LGTM! Identity logout implementation matches pattern.The method correctly uses "aws-user" as the provider name (matching the constant), performs cleanup, and wraps errors properly.
pkg/auth/providers/aws/saml_test.go (1)
111-113: LGTM! Test stub extended for new interface.The no-op logout methods on stubSamlMgr satisfy the expanded AuthManager interface for test purposes.
pkg/auth/manager_test.go (4)
286-286: LGTM! Test stub updated for Provider interface.The no-op Logout on testProvider satisfies the updated interface contract.
478-478: LGTM! Test stub updated for Identity interface.
550-550: LGTM! Test stub updated.
794-794: LGTM! Test stub completed.pkg/auth/types/interfaces.go (4)
3-3: LGTM! Mock generation enabled.The go:generate directive will automate mock creation for testing.
33-36: LGTM! Provider logout interface added.The method signature and best-effort semantics are clearly documented.
61-63: LGTM! Identity logout interface added.
106-116: LGTM! AuthManager logout methods added.The three logout variants (identity-specific, provider-specific, and all) provide comprehensive coverage with clear documentation of best-effort cleanup semantics.
pkg/auth/providers/github/oidc.go (1)
204-210: LGTM! Appropriate no-op logout.GitHub OIDC provider has no local files to clean up—credentials live in the keyring and are handled by the manager. The debug log and comment make this clear.
pkg/auth/hooks_test.go (1)
50-60: LGTM!The stub implementations correctly satisfy the updated AuthManager interface for testing purposes. The nil returns are appropriate for test stubs.
pkg/auth/cloud/aws/files_logout_test.go (3)
9-90: LGTM!Well-structured table-driven tests covering the important scenarios: existing directories, non-existent directories, and nested structures. The use of
t.TempDir()ensures proper cleanup.
92-191: LGTM!Comprehensive tests for CleanupAll covering multiple providers, empty directories, and complex nested structures. Good coverage of edge cases.
193-265: LGTM!Excellent idempotency tests for both Cleanup and CleanupAll. These ensure the methods can be safely called multiple times without errors.
website/docs/cli/commands/auth/logout.mdx (1)
1-392: LGTM!Comprehensive documentation covering usage, examples, security considerations, troubleshooting, and best practices. The warnings about browser sessions are appropriately prominent. The structure and content align well with the feature implementation.
Note: The LanguageTool warnings about "logout" spelling are false positives, as "logout" is the actual command name.
cmd/auth_logout.go (1)
43-52: Fix multiple %w wrapping when returning initialization errors.Use errors.Join or a single %w to avoid fmt.Errorf panics.
Apply this diff:
- if err != nil { - return fmt.Errorf("%w: %w", errUtils.ErrFailedToInitializeAtmosConfig, err) - } + if err != nil { + return errors.Join(errUtils.ErrFailedToInitializeAtmosConfig, err) + } ... - if err != nil { - return fmt.Errorf("%w: %w", errUtils.ErrAuthManager, err) - } + if err != nil { + return errors.Join(errUtils.ErrAuthManager, err) + }⛔ Skipped due to learnings
Learnt from: CR PR: cloudposse/atmos#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-16T15:18:00.319Z Learning: Applies to **/*.go : All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.Learnt from: osterman PR: cloudposse/atmos#740 File: cmd/cmd_utils.go:340-359 Timestamp: 2024-10-23T21:36:40.262Z Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.Learnt from: Benbentwo PR: cloudposse/atmos#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.pkg/auth/manager_logout_test.go (1)
226-264: Review comment is inaccurate; test expectations correctly reflect actual behavior.The test expectation of
provider.Logoutbeing called 3 times is correct. Here's why:
LogoutProvideriterates through 2 identities and callsm.Logoutfor each (line 804)- Each
Logoutcall independently invokesprovider.Logoutonce (line 737)- Then
LogoutProvidercallsprovider.Logoutagain directly (line 818)This results in: 2 (from identity loop) + 1 (direct) = 3 calls. This is intentional, not redundant—the provider cleanup happens both during identity logout and explicitly at the provider level.
Consolidating to 1x would alter the logout semantics and require architectural changes. The current test accurately validates the implemented behavior.
Likely an incorrect or invalid review comment.
pkg/auth/manager.go (1)
724-731: I need to verify the Go version requirement since the go.mod check produced no output:Perfect. Before Go 1.20, fmt.Errorf() is able to support only one occurrence of %w format verb. However, with Go 1.20, fmt.Errorf now accepts multiple %w format specifiers and by having multiple %w, the function fmt.Errorf() will instead create a list of errors, returning error[] instead of error.Now comparing with the learnings: the learning explicitly validates that the pattern in use (
fmt.Errorf(errUtils.ErrWrappingFormat, ...)withErrWrappingFormat = "%w: %w") is correct. The review is based on outdated Go 1.13-1.19 behavior where multiple %w would fail. But this codebase is using the pattern extensively, which indicates it targets Go 1.20+.This review comment is incorrect—no code changes are needed.
The multiple %w pattern is valid in Go 1.20+ and has been validated as correct by a previous reviewer. The code does not panic and uses the proper error wrapping approach for the target Go version.
Likely an incorrect or invalid review comment.
pkg/auth/types/mock_interfaces.go (2)
16-737: LGTM: mocks match updated interfaces (incl. Logout) and are ready for tests.Signatures align with Provider, Identity, AuthManager, CredentialStore, Validator, and ICredentials. Good placement alongside the interfaces.
1-4: Generated mocks correctly follow the standard exclusion pattern.The file has the proper
Code generated by MockGen. DO NOT EDIT.header, and.golangci.ymlis configured withgenerated: laxfor both linters and formatters. This setup correctly exempts generated mocks from lint checks like godot and missing documentation requirements.
Add support for configuring the AWS credentials base directory path through provider spec configuration with proper precedence handling. - Add files.base_path configuration option to provider spec - Update AWSFileManager to accept optional base_path parameter - Implement precedence: spec config > env var > default ~/.aws/atmos - Add GetFilesBasePath helper to extract path from provider spec - Update all providers (SSO, SAML) to use configured base_path - Add GetFilesDisplayPath method to AuthManager interface - Support tilde expansion for user home directory paths - Use viper.GetString for environment variable access - Update golden snapshot for auth invalid-command test This allows users to customize where AWS credential files are stored on a per-provider basis, enabling XDG-compliant paths or custom locations while maintaining backward compatibility with the default ~/.aws/atmos directory. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Updated the `atmos auth logout` command to show the actual configured AWS files path instead of hardcoded `~/.aws/atmos/` in dry-run mode. Changes: - performIdentityLogout: Use authManager.GetFilesDisplayPath() for provider path - performProviderLogout: Use authManager.GetFilesDisplayPath() for provider path - performLogoutAll: Enhanced to show all provider paths using GetFilesDisplayPath() This ensures users see the correct path whether using the default ~/.aws/atmos or a custom path configured via spec.files.base_path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Added validation to ensure spec.files.base_path is properly formatted when configured in AWS auth providers (SSO and SAML). Changes: - Created ValidateFilesBasePath() function in pkg/auth/cloud/aws/spec.go - Updated SSO and SAML provider Validate() methods to call validation - Added comprehensive tests covering valid and invalid path scenarios - Validation checks for: empty/whitespace paths, invalid characters This ensures configuration errors are caught early during auth validation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Updated documentation to explain the configurable AWS files base path feature for auth logout command. Changes: - PRD: Added Configuration section with spec.files.base_path examples - PRD: Updated FR-004 to mention configurable base path - CLI docs: Added "Advanced Configuration" section with full examples - CLI docs: Documented configuration precedence and validation - CLI docs: Added example with environment variable override The documentation explains: - How to configure custom file paths in atmos.yaml - Configuration precedence (config > env var > default) - Path validation rules - Use cases for custom paths (containers, multi-user, etc.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/auth/identities/aws/permission_set.go (1)
145-151: Path mismatch: Environment ignores provider files.base_path.Creating
NewAWSFileManager("")ignores a provider’s configuredfiles.base_path. Downstream cleanup (provider Logout) uses the configured base path, so env points to one path while files may be written/cleaned in another.Pass the provider’s configured base path into the file manager. Options:
- Thread base path from config/manager into Environment.
- Or expose a helper to resolve base path by provider name and use it here.
This prevents stale creds and inconsistent AWS_* env paths.
pkg/auth/providers/aws/saml.go (1)
121-125: Fix error wrapping to preserve sentinels and causes.Several fmt.Errorf calls use %v for the inner error, breaking errors.Is checks per guidelines. Wrap underlying errors with %w or join with the sentinel + contextual error.
- return nil, fmt.Errorf("%w: failed to create SAML client: %v", errUtils.ErrInvalidProviderConfig, err) + return nil, errors.Join(errUtils.ErrInvalidProviderConfig, fmt.Errorf("failed to create SAML client: %w", err)) - return nil, fmt.Errorf("%w: failed to decode assertion: %v", errUtils.ErrAwsSAMLDecodeFailed, err) + return nil, errors.Join(errUtils.ErrAwsSAMLDecodeFailed, fmt.Errorf("failed to decode assertion: %w", err)) - return nil, fmt.Errorf("%w: extract AWS roles: %v", errUtils.ErrAwsSAMLDecodeFailed, err) + return nil, errors.Join(errUtils.ErrAwsSAMLDecodeFailed, fmt.Errorf("extract AWS roles: %w", err)) - return nil, fmt.Errorf("%w: parse AWS roles: %v", errUtils.ErrAwsSAMLDecodeFailed, err) + return nil, errors.Join(errUtils.ErrAwsSAMLDecodeFailed, fmt.Errorf("parse AWS roles: %w", err)) - return nil, fmt.Errorf("%w: failed to assume role with SAML: %v", errUtils.ErrAuthenticationFailed, err) + return nil, errors.Join(errUtils.ErrAuthenticationFailed, fmt.Errorf("failed to assume role with SAML: %w", err)) - return nil, fmt.Errorf("%w: failed to create AWS config: %v", errUtils.ErrAuthenticationFailed, err) + return nil, errors.Join(errUtils.ErrAuthenticationFailed, fmt.Errorf("failed to create AWS config: %w", err)) - return nil, fmt.Errorf("%w: failed to assume role with SAML: %v", errUtils.ErrAuthenticationFailed, err) + return nil, errors.Join(errUtils.ErrAuthenticationFailed, fmt.Errorf("failed to assume role with SAML: %w", err))Also applies to: 132-136, 137-143, 150-154, 253-256, 268-271
pkg/auth/identities/aws/user.go (1)
150-156: Fix error wrapping to follow guidelines.Use %w and/or errors.Join instead of %v to keep sentinel checks working.
- if err := awsFileManager.WriteCredentials(awsUserProviderName, i.name, creds); err != nil { - return fmt.Errorf("%w: failed to write AWS credentials: %v", errUtils.ErrAwsAuth, err) + if err := awsFileManager.WriteCredentials(awsUserProviderName, i.name, creds); err != nil { + return errors.Join(errUtils.ErrAwsAuth, fmt.Errorf("failed to write AWS credentials: %w", err)) } - if err := awsFileManager.WriteConfig(awsUserProviderName, i.name, region, ""); err != nil { - return fmt.Errorf("%w: failed to write AWS config: %v", errUtils.ErrAwsAuth, err) + if err := awsFileManager.WriteConfig(awsUserProviderName, i.name, region, ""); err != nil { + return errors.Join(errUtils.ErrAwsAuth, fmt.Errorf("failed to write AWS config: %w", err)) } - if err != nil { - return nil, fmt.Errorf("%w: failed to load AWS config: %v", errUtils.ErrAwsAuth, err) + if err != nil { + return nil, errors.Join(errUtils.ErrAwsAuth, fmt.Errorf("failed to load AWS config: %w", err)) } - if err := i.writeAWSFiles(sessionCreds, region); err != nil { - return nil, fmt.Errorf("%w: failed to write AWS files: %v", errUtils.ErrAwsAuth, err) + if err := i.writeAWSFiles(sessionCreds, region); err != nil { + return nil, errors.Join(errUtils.ErrAwsAuth, fmt.Errorf("failed to write AWS files: %w", err)) } - if err := form.Run(); err != nil { - return "", fmt.Errorf("%w: failed to get MFA token: %v", errUtils.ErrAuthenticationFailed, err) + if err := form.Run(); err != nil { + return "", errors.Join(errUtils.ErrAuthenticationFailed, fmt.Errorf("failed to get MFA token: %w", err)) } - if err != nil { - return nil, fmt.Errorf("%w: AWS user identity %q authentication failed: %v", errUtils.ErrAuthenticationFailed, identityName, err) + if err != nil { + return nil, errors.Join(errUtils.ErrAuthenticationFailed, fmt.Errorf("AWS user identity %q authentication failed: %w", identityName, err)) }Also applies to: 179-182, 221-223, 236-238, 336-338
♻️ Duplicate comments (1)
cmd/auth_logout.go (1)
39-54: Add perf tracking at command entrypoint.Per guidelines, add defer perf.Track after config init to profile this command.
import ( "context" "errors" "fmt" + "github.com/cloudposse/atmos/pkg/perf" ... func executeAuthLogoutCommand(cmd *cobra.Command, args []string) error { handleHelpRequest(cmd, args) // Load atmos config. atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) if err != nil { return fmt.Errorf("%w: %w", errUtils.ErrFailedToInitializeAtmosConfig, err) } + + defer perf.Track(&atmosConfig, "cmd.AuthLogout.execute")() + // Create auth manager.
🧹 Nitpick comments (13)
pkg/auth/cloud/aws/spec.go (1)
14-24: Unit tests for spec parsing.Please add a tiny table‑driven test covering nil provider/spec, wrong types, and valid base_path. I can draft it.
pkg/auth/cloud/aws/setup.go (1)
13-18: Add perf.Track to exported functions.Per guidelines, add
defer perf.Track()at the top ofSetupFilesandSetEnvironmentVariables(package‑prefixed name, blank line after).Also applies to: 44-57
pkg/auth/identities/aws/assume_role.go (1)
97-103: Clamp AssumeRole duration to STS limits.DurationSeconds should be clamped to [900, 43200] to avoid API errors (you already clamp in SAML). Add min/max guards here too.
- if duration, err := time.ParseDuration(durationStr); err == nil { - input.DurationSeconds = aws.Int32(int32(duration.Seconds())) + if duration, err := time.ParseDuration(durationStr); err == nil { + secs := int32(duration.Seconds()) + if secs < 900 { + secs = 900 + } else if secs > 43200 { + secs = 43200 + } + input.DurationSeconds = aws.Int32(secs)pkg/auth/identities/aws/user.go (1)
259-278: Enable accessible mode for MFA prompt.Support screen readers/non-TTY by enabling huh accessible mode behind an env flag.
+import "os" ... func newMfaForm(longLivedCreds *types.AWSCredentials, mfaToken *string) *huh.Form { - form := huh.NewForm( + form := huh.NewForm( ... ) - return form + return form.WithAccessible(os.Getenv("ACCESSIBLE") != "") }Based on learnings.
cmd/auth_logout.go (3)
20-37: Use identity/provider completions instead of component completion.ValidArgsFunction points to ComponentsArgCompletion. Provide a lightweight completer for identities (and flags for providers) to improve UX.
296-300: Bind flags with Viper (ENV parity).Bind --provider/--dry-run to Viper (and ATMOS_ equivalents) for consistent CLI/ENV handling per guidelines.
39-74: Capture telemetry for non-standard path.Add telemetry.CaptureCmd/CaptureCmdString (no user data) at the start/end of execution to observe usage of logout flows.
pkg/auth/manager.go (2)
862-885: Optional: de-duplicate provider cleanup in LogoutAll.Calling Logout for each identity may trigger provider.Logout many times. Consider tracking visited providers and calling provider.Logout once per provider to reduce redundant I/O.
334-360: Minor: avoid hardcoded Unix-y fallback in display path and rely on file manager when possible.If NewAWSFileManager fails, we return "~/.aws/atmos". On Windows this looks odd. Consider returning an empty string or a best-effort computed path via homedir.Dir + filepath.Join, or surface an error to the caller to decide.
pkg/auth/cloud/aws/files.go (4)
48-75: Prefer internal homedir utilities over external dependency.The repo has pkg/config/homedir with Expand/Dir. Using it avoids duplicate logic and keeps behavior consistent across Atmos.
Apply this diff:
@@ -import ( +import ( "errors" "fmt" "os" "path/filepath" "strings" - homedir "github.com/mitchellh/go-homedir" + homedir "github.com/cloudposse/atmos/pkg/config/homedir" "github.com/spf13/viper" ini "gopkg.in/ini.v1" @@ - expanded, err := homedir.Expand(basePath) + expanded, err := homedir.Expand(basePath) @@ - } else if envPath := viper.GetString("ATMOS_AWS_FILES_BASE_PATH"); envPath != "" { + } else if envPath := viper.GetString("ATMOS_AWS_FILES_BASE_PATH"); envPath != "" { @@ - homeDir, err := homedir.Dir() + homeDir, err := homedir.Dir()
201-208: Windows-friendly display path.strings.HasPrefix on raw paths can fail across separators/case. Normalize before compare.
Apply this diff:
func (m *AWSFileManager) GetDisplayPath() string { - homeDir, err := homedir.Dir() - if err == nil && homeDir != "" && strings.HasPrefix(m.baseDir, homeDir) { - return strings.Replace(m.baseDir, homeDir, "~", 1) + homeDir, err := homedir.Dir() + if err == nil && homeDir != "" { + base := filepath.Clean(m.baseDir) + hd := filepath.Clean(homeDir) + if strings.HasPrefix(strings.ToLower(base), strings.ToLower(hd+string(os.PathSeparator))) || strings.EqualFold(base, hd) { + return strings.Replace(base, hd, "~", 1) + } } - return m.baseDir + return filepath.Clean(m.baseDir) }
235-245: Simplify RemoveAll error handling.os.RemoveAll already returns nil when the path doesn’t exist; the NotExist branch is redundant.
Apply this diff:
func (m *AWSFileManager) Cleanup(providerName string) error { providerDir := filepath.Join(m.baseDir, providerName) - if err := os.RemoveAll(providerDir); err != nil { - // If directory doesn't exist, that's not an error (already cleaned up). - if os.IsNotExist(err) { - return nil - } + if err := os.RemoveAll(providerDir); err != nil { errUtils.CheckErrorAndPrint(ErrCleanupAWSFiles, providerDir, "failed to cleanup AWS files") return ErrCleanupAWSFiles } return nil }
247-259: Same here: RemoveAll already no-ops on missing paths.Drop the NotExist handling to reduce branches.
Apply this diff:
func (m *AWSFileManager) CleanupAll() error { - if err := os.RemoveAll(m.baseDir); err != nil { - // If directory doesn't exist, that's not an error (already cleaned up). - if os.IsNotExist(err) { - return nil - } + if err := os.RemoveAll(m.baseDir); err != nil { errUtils.CheckErrorAndPrint(ErrCleanupAWSFiles, m.baseDir, "failed to cleanup all AWS files") return ErrCleanupAWSFiles } return nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
cmd/auth_logout.go(1 hunks)pkg/auth/cloud/aws/files.go(4 hunks)pkg/auth/cloud/aws/setup.go(2 hunks)pkg/auth/cloud/aws/spec.go(1 hunks)pkg/auth/hooks_test.go(2 hunks)pkg/auth/identities/aws/assume_role.go(2 hunks)pkg/auth/identities/aws/permission_set.go(2 hunks)pkg/auth/identities/aws/user.go(3 hunks)pkg/auth/manager.go(3 hunks)pkg/auth/providers/aws/saml.go(2 hunks)pkg/auth/providers/aws/saml_test.go(1 hunks)pkg/auth/providers/aws/sso.go(1 hunks)pkg/auth/types/interfaces.go(5 hunks)pkg/auth/types/mock_interfaces.go(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_invalid-command.stderr.golden(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/TestCLICommands_atmos_auth_invalid-command.stderr.golden
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/auth/providers/aws/saml_test.go
- pkg/auth/providers/aws/sso.go
🧰 Additional context used
📓 Path-based instructions (7)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/cloud/aws/spec.gopkg/auth/identities/aws/permission_set.gopkg/auth/identities/aws/user.gopkg/auth/providers/aws/saml.gopkg/auth/cloud/aws/files.gopkg/auth/identities/aws/assume_role.gopkg/auth/types/mock_interfaces.gopkg/auth/types/interfaces.gopkg/auth/cloud/aws/setup.gopkg/auth/manager.gopkg/auth/hooks_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
pkg/auth/cloud/aws/spec.gopkg/auth/identities/aws/permission_set.gopkg/auth/identities/aws/user.gopkg/auth/providers/aws/saml.gopkg/auth/cloud/aws/files.gopkg/auth/identities/aws/assume_role.gopkg/auth/types/mock_interfaces.gocmd/auth_logout.gopkg/auth/types/interfaces.gopkg/auth/cloud/aws/setup.gopkg/auth/manager.gopkg/auth/hooks_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Add
defer perf.Track()to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.
Files:
pkg/auth/cloud/aws/spec.gopkg/auth/identities/aws/permission_set.gopkg/auth/identities/aws/user.gopkg/auth/providers/aws/saml.gopkg/auth/cloud/aws/files.gopkg/auth/identities/aws/assume_role.gopkg/auth/types/mock_interfaces.gocmd/auth_logout.gopkg/auth/types/interfaces.gopkg/auth/cloud/aws/setup.gopkg/auth/manager.go
**/{mock_*.go,mock/*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Generate and keep mocks alongside the interfaces they mock; mocks are excluded from coverage.
Files:
pkg/auth/types/mock_interfaces.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
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown() to render embedded markdown content for CLI command help/examples.
One Cobra command per file in cmd/; keep files focused and small.
Files:
cmd/auth_logout.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/auth/hooks_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/auth/hooks_test.go
🧠 Learnings (3)
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to cmd/**/*.go : Use Viper for managing configuration, environment variables, and flags in the CLI
Applied to files:
pkg/auth/cloud/aws/files.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/!(*_test).go : Add `defer perf.Track()` to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.
Applied to files:
cmd/auth_logout.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
PR: cloudposse/atmos#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:
pkg/auth/manager.go
🧬 Code graph analysis (9)
pkg/auth/identities/aws/permission_set.go (2)
pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(47-76)pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/identities/aws/user.go (3)
pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(47-76)errors/errors.go (1)
ErrLogoutFailed(374-374)pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/providers/aws/saml.go (4)
pkg/auth/cloud/aws/spec.go (1)
GetFilesBasePath(9-25)pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(47-76)errors/errors.go (1)
ErrLogoutFailed(374-374)pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/cloud/aws/files.go (2)
pkg/config/homedir/homedir.go (2)
Expand(68-87)Dir(36-63)errors/error_funcs.go (1)
CheckErrorAndPrint(29-52)
pkg/auth/identities/aws/assume_role.go (2)
pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(47-76)pkg/logger/log.go (1)
Debug(24-26)
pkg/auth/types/mock_interfaces.go (3)
pkg/schema/schema.go (3)
Context(381-396)Validate(188-190)ConfigAndStacksInfo(460-539)pkg/auth/types/interfaces.go (4)
ICredentials(156-162)AuthManager(67-121)Identity(40-64)Provider(13-37)pkg/auth/types/whoami.go (1)
WhoamiInfo(6-22)
cmd/auth_logout.go (7)
cmd/cmd_utils.go (1)
ComponentsArgCompletion(768-790)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/schema/schema.go (2)
ConfigAndStacksInfo(460-539)Context(381-396)errors/errors.go (6)
ErrFailedToInitializeAtmosConfig(117-117)ErrAuthManager(334-334)ErrIdentityNotInConfig(380-380)ErrPartialLogout(375-375)ErrProviderNotInConfig(381-381)ErrInvalidLogoutOption(382-382)pkg/auth/types/interfaces.go (1)
AuthManager(67-121)pkg/utils/log_utils.go (1)
PrintfMessageToTUI(33-35)internal/tui/utils/utils.go (1)
NewAtmosHuhTheme(85-93)
pkg/auth/cloud/aws/setup.go (1)
pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(47-76)
pkg/auth/manager.go (5)
pkg/perf/perf.go (1)
Track(121-138)pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(47-76)pkg/auth/cloud/aws/spec.go (1)
GetFilesBasePath(9-25)errors/errors.go (7)
ErrIdentityNotInConfig(380-380)ErrKeyringDeletion(377-377)ErrProviderLogout(378-378)ErrIdentityLogout(379-379)ErrPartialLogout(375-375)ErrLogoutFailed(374-374)ErrProviderNotInConfig(381-381)pkg/auth/types/interfaces.go (1)
Provider(13-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/auth/identities/aws/permission_set.go (1)
266-274: No‑op Logout is fine.Clear and documented; provider/AuthManager handle cleanup.
pkg/auth/types/interfaces.go (1)
33-37: All interface implementations verified—no issues found.Logout is implemented across all concrete providers (sso, oidc, saml) and identities (user, permissionSet, assumeRole). Mocks and test stubs are also in place. No compilation breakage.
pkg/auth/identities/aws/assume_role.go (2)
278-285: Logout noop looks good.Identity has no local artifacts; deferring cleanup to provider/manager is appropriate.
193-197: Environment() cannot access provider.Spec to resolve base_path—the architecture doesn't pass provider config to identity functions.The review comment correctly identifies a consistency gap. While
sso.pyandsaml.pyproperly callGetFilesBasePath(p.config)beforeNewAWSFileManager(), the identity functions (assume_role,permission_set,user) andSetupFiles/SetEnvironmentVariablesinsetup.goonly receiveproviderName(string), not the full provider spec. They cannot accessprovider.Spec["files"]["base_path"]without structural changes.This is a real problem: if a provider configures a custom
files.base_path, the paths used inSetupFiles()(write) won't match whereEnvironment()reads env vars, and won't match the display paths shown to users.The fix requires either:
- Passing the provider object through the identity call chain, or
- Looking up the provider spec by name from an auth manager registry
pkg/auth/providers/aws/saml.go (1)
373-390: Provider logout looks good.Uses provider-scoped base_path and joins ErrLogoutFailed on failure.
pkg/auth/hooks_test.go (2)
51-61: Stub methods look good.Satisfies new AuthManager methods; simple no-op behavior suits these unit tests.
1-166: The review comment asks for cmd-level tests that contradict the stated coding guidelines.The original comment references a non-existent
performLogoutAllfunction (the actual function isperformInteractiveLogout) and requests unit tests for cmd-level wrapper functions. However, the coding guideline explicitly states "Place business logic in pkg rather than in cmd." The logout functions incmd/auth_logout.goare thin orchestration wrappers that delegate topkg/authmethods (Logout, LogoutProvider, LogoutAll). Business logic testing belongs at the package level, wherepkg/auth/manager_logout_test.goalready exists.Cmd-level integration tests (via tests/ fixtures) would validate the CLI flow end-to-end, but unit-level table-driven tests belong at the package level where the actual logout business logic is tested—not in cmd.
Likely an incorrect or invalid review comment.
cmd/auth_logout.go (1)
39-74: This review comment is incorrect. The version command's robustness for broken configs is ensured by root.go's PersistentPreRun logic (lines 94-103), not by auth_logout.go.When
atmos --versionoratmos versionruns, it executes within PersistentPreRun before any subcommand. That logic gracefully handles missing config files viacfg.NotFoundchecks. The auth_logout.go subcommand only runs if explicitly called and cannot affect the version command's execution path. Its error-handling pattern is consistent with other subcommands like auth_login and auth_validate.Likely an incorrect or invalid review comment.
pkg/auth/manager.go (1)
751-760: Idempotent logout issue is real, but suggested fix won't work as written.The core concern is valid: go-keyring's Delete does not guarantee an idempotent/no-error result for missing entries — it forwards the underlying backend's behavior. The current code fails logout when credentials don't exist, breaking idempotency.
However, the suggested diff checks for sentinels that don't exist in the codebase (
ErrCredentialNotFound) or won't be returned by go-keyring (os.ErrNotExist). The package does not normalize all backends to one specific not-found error, so the approach won't work reliably.To fix correctly:
- Verify what specific error go-keyring actually returns on your platform(s) when deleting missing entries
- Either wrap Delete to detect that error, treat all Delete errors as non-fatal (log only), or check existence first
- Ensure logout succeeds when there's nothing to remove
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1656 +/- ##
==========================================
+ Coverage 66.43% 66.57% +0.13%
==========================================
Files 357 359 +2
Lines 41126 41780 +654
==========================================
+ Hits 27322 27814 +492
- Misses 11768 11911 +143
- Partials 2036 2055 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…-to-remove-local-credential-store
Made several code quality improvements to auth logout implementation: 1. Performance Tracking: - Added perf.Track to executeAuthLogoutCommand in cmd/auth_logout.go - Added perf.Track to GetFilesBasePath in pkg/auth/cloud/aws/spec.go - Ensures performance monitoring for auth logout operations 2. Dead Code Removal: - Removed ErrLogoutNotSupported from errors/errors.go (never used) - All providers implement Logout, no concept of unsupported logout exists 3. Environment Variable Removal: - Removed ATMOS_AWS_FILES_BASE_PATH env var support per requirements - Only spec.files.base_path configuration is supported now - Removed viper import from pkg/auth/cloud/aws/files.go - Updated documentation to remove env var references 4. Documentation Updates: - Updated PRD to remove env var from precedence - Updated CLI docs to remove env var example and precedence - Simplified configuration to: spec config > default Changes maintain backward compatibility - default behavior unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add ErrLogoutNotSupported (exit 0) and ErrLogoutNotImplemented (exit non-zero) - GitHub OIDC returns ErrLogoutNotSupported (no local storage to clean) - Manager treats ErrLogoutNotSupported as success - Fix duplicate identity.Logout calls with identityLogoutCalled flag - Add resolveProviderForIdentity for transitive Via chain resolution - Update LogoutProvider to find identities transitively - Add comprehensive tests (79.5% coverage in pkg/auth)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/auth/cloud/aws/files.go (1)
223-236: Preserve error cause when cleanup fails; resolve absolute path before deletion.
- Wrap with the sentinel using errors.Join to keep the root cause.
- Use filepath.Abs before RemoveAll (safety, logs clearer).
func (m *AWSFileManager) Cleanup(providerName string) error { - providerDir := filepath.Join(m.baseDir, providerName) + providerDir := filepath.Join(m.baseDir, providerName) + if abs, err := filepath.Abs(providerDir); err == nil { + providerDir = abs + } - if err := os.RemoveAll(providerDir); err != nil { - // If directory doesn't exist, that's not an error (already cleaned up). - if os.IsNotExist(err) { - return nil - } - errUtils.CheckErrorAndPrint(ErrCleanupAWSFiles, providerDir, "failed to cleanup AWS files") - return ErrCleanupAWSFiles - } + if err := os.RemoveAll(providerDir); err != nil { + if os.IsNotExist(err) { + return nil + } + errUtils.CheckErrorAndPrint(ErrCleanupAWSFiles, providerDir, "failed to cleanup AWS files") + return errors.Join(ErrCleanupAWSFiles, err) + } return nil } func (m *AWSFileManager) CleanupAll() error { - if err := os.RemoveAll(m.baseDir); err != nil { + base := m.baseDir + if abs, err := filepath.Abs(base); err == nil { + base = abs + } + if err := os.RemoveAll(base); err != nil { // If directory doesn't exist, that's not an error (already cleaned up). if os.IsNotExist(err) { return nil } - errUtils.CheckErrorAndPrint(ErrCleanupAWSFiles, m.baseDir, "failed to cleanup all AWS files") - return ErrCleanupAWSFiles + errUtils.CheckErrorAndPrint(ErrCleanupAWSFiles, base, "failed to cleanup all AWS files") + return errors.Join(ErrCleanupAWSFiles, err) } return nil }As per coding guidelines.
Also applies to: 239-251
♻️ Duplicate comments (1)
pkg/auth/manager.go (1)
780-794: Nice: duplicate Identity.Logout on fallback chains is now guarded.The identityLogoutCalled flag prevents double cleanup when chain == [identityName]. Thanks for addressing the earlier review.
Also applies to: 797-810
🧹 Nitpick comments (12)
docs/prd/auth-logout.md (2)
468-482: Fix error-wrapping example: only one %w allowed in fmt.Errorf.The snippet uses two %w. Go permits at most one. Suggest joining the sentinel with the cause.
Replace the example with:
-errs = append(errs, fmt.Errorf("%w: keyring deletion failed: %w", - ErrLogoutFailed, err)) +errs = append(errs, errors.Join( + ErrLogoutFailed, + fmt.Errorf("keyring deletion failed: %v", err), +))As per coding guidelines.
148-156: Add language to fenced code blocks to satisfy markdownlint.Mark these output blocks as text.
-``` +```text ? Choose what to logout from: ❯ Identity: dev-admin ...-``` +```text ⚠️ Important: This command only removes locally cached credentials. ...Also applies to: 629-640
website/docs/cli/commands/auth/logout.mdx (1)
41-58: Specify languages on fenced code blocks (lint).Add “text” for outputs to satisfy MD040 and improve rendering.
Example:
-``` +```text Logging out from identity: dev-admin ...Repeat for the listed blocks.
Also applies to: 91-98, 108-119, 188-205, 248-255, 263-272, 278-281, 296-299
cmd/auth_logout.go (3)
237-239: Graceful cancel in interactive mode (Ctrl+C).Exit cleanly instead of surfacing an error.
- if err := form.Run(); err != nil { - return err - } + if err := form.Run(); err != nil { + if errors.Is(err, context.Canceled) { + u.PrintfMessageToTUI("Canceled.\n") + return nil + } + return err + }As per coding guidelines.
79-132: Add perf tracking to critical helpers.Track time in helper paths too.
func performIdentityLogout(ctx context.Context, authManager auth.AuthManager, identityName string, dryRun bool) error { + defer perf.Track(nil, "cmd.AuthLogout.performIdentityLogout")() + // Validate identity exists.func performProviderLogout(ctx context.Context, authManager auth.AuthManager, providerName string, dryRun bool) error { + defer perf.Track(nil, "cmd.AuthLogout.performProviderLogout")()func performInteractiveLogout(ctx context.Context, authManager auth.AuthManager, dryRun bool) error { + defer perf.Track(nil, "cmd.AuthLogout.performInteractiveLogout")()func performLogoutAll(ctx context.Context, authManager auth.AuthManager, dryRun bool) error { + defer perf.Track(nil, "cmd.AuthLogout.performLogoutAll")()As per coding guidelines.
Also applies to: 134-175, 177-252, 254-290
35-38: Replace unrelated arg completion.ComponentsArgCompletion is not relevant here; it may suggest component names. Either drop it or implement identity completion.
- ValidArgsFunction: ComponentsArgCompletion, + // TODO: implement identity completion (ListIdentities) and provider flag completion. + // ValidArgsFunction: identityArgCompletion,Optionally, register provider flag completion in init(). Want a patch? Based on learnings.
pkg/auth/manager_logout_test.go (2)
226-263: Avoid brittle Times() expectations for provider cleanup.Current test expects provider cleanup called 3x (per identity + once more). Provider cleanup should typically run once per provider. Either assert 1 call or use AnyTimes to avoid binding tests to internal call order.
- provider.EXPECT().Logout(gomock.Any()).Return(nil).Times(3) + provider.EXPECT().Logout(gomock.Any()).Return(nil).Times(1)If manager intentionally calls provider cleanup per identity, document it in a comment.
16-25: Remove unused fields or assert them.checkCalls/expectedCalls are never used. Either drop them or add assertions.
- checkCalls bool - expectedCalls int + // Add explicit call-count assertions or remove these fields.pkg/auth/cloud/aws/files.go (2)
10-10: Use internal homedir utilities for consistency.Prefer pkg/config/homedir over external go-homedir.
- homedir "github.com/mitchellh/go-homedir" + homedir "github.com/cloudposse/atmos/pkg/config/homedir"Based on learnings.
49-66: Consider validating/normalizing baseDir once (abs path).Optional: call filepath.Abs on baseDir post expansion to standardize paths across platforms (logging and deletion).
pkg/auth/manager.go (2)
334-360: Avoid hard-coding ~/.aws/atmos; fall back to default file manager for portability.On error with a configured base_path, return the default manager’s display path (Windows-safe) before hard-coding a POSIX path.
func (m *manager) GetFilesDisplayPath(providerName string) string { defer perf.Track(nil, "auth.Manager.GetFilesDisplayPath")() @@ // Create file manager to get display path. - fileManager, err := awsCloud.NewAWSFileManager(basePath) - if err != nil { - return "~/.aws/atmos" - } + fileManager, err := awsCloud.NewAWSFileManager(basePath) + if err != nil { + log.Debug("Failed to init AWSFileManager with configured base path; falling back to default.", "provider", providerName, "error", err) + if fm2, err2 := awsCloud.NewAWSFileManager(""); err2 == nil { + return fm2.GetDisplayPath() + } + return "~/.aws/atmos" + } return fileManager.GetDisplayPath() }
928-951: LogoutAll: de-dup provider cleanup across identities.Same duplicate provider.Logout concern as above; consider the same context flag approach to skip per‑identity provider cleanup during LogoutAll, then optionally perform one cleanup per provider post‑loop.
📜 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 (9)
cmd/auth_logout.go(1 hunks)docs/prd/auth-logout.md(1 hunks)errors/errors.go(1 hunks)pkg/auth/cloud/aws/files.go(4 hunks)pkg/auth/cloud/aws/spec.go(1 hunks)pkg/auth/manager.go(3 hunks)pkg/auth/manager_logout_test.go(1 hunks)pkg/auth/providers/github/oidc.go(1 hunks)website/docs/cli/commands/auth/logout.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/auth/providers/github/oidc.go
- errors/errors.go
- pkg/auth/cloud/aws/spec.go
🧰 Additional context used
📓 Path-based instructions (9)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/manager_logout_test.gopkg/auth/cloud/aws/files.gopkg/auth/manager.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/auth/manager_logout_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
pkg/auth/manager_logout_test.gopkg/auth/cloud/aws/files.gopkg/auth/manager.gocmd/auth_logout.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/auth/manager_logout_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Add
defer perf.Track()to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.
Files:
pkg/auth/cloud/aws/files.gopkg/auth/manager.gocmd/auth_logout.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
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown() to render embedded markdown content for CLI command help/examples.
One Cobra command per file in cmd/; keep files focused and small.
Files:
cmd/auth_logout.go
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in website/ when adding features
Ensure consistency between CLI help text and website documentation
Follow the website's documentation structure and style
Keep website code in website/ and follow its architecture/style; test changes locally
Keep CLI and website documentation in sync; document new features with examples and use casesBefore committing documentation/site changes, run npm run build in website/ and fix errors, broken links, and missing images.
Files:
website/docs/cli/commands/auth/logout.mdx
website/docs/cli/commands/**/*.mdx
📄 CodeRabbit inference engine (CLAUDE.md)
Document all new commands/flags/parameters in Docusaurus under website/docs/cli/commands/, using definition lists for args/flags and the prescribed frontmatter and sections.
Files:
website/docs/cli/commands/auth/logout.mdx
website/docs/**
📄 CodeRabbit inference engine (CLAUDE.md)
website/docs/**: After modifying any docs under website/docs/, build the website (npm run build) and ensure no errors.
Document user-facing template functions in the website if applicable.
Files:
website/docs/cli/commands/auth/logout.mdx
🧠 Learnings (11)
📚 Learning: 2025-10-18T03:22:41.749Z
Learnt from: osterman
PR: cloudposse/atmos#1656
File: pkg/auth/identities/aws/user.go:365-372
Timestamp: 2025-10-18T03:22:41.749Z
Learning: In pkg/auth/identities/aws/user.go, the Logout method intentionally calls fileManager.Cleanup("aws-user") to remove the entire provider directory. This is correct behavior because all aws-user identities share the same provider authentication state, so logging out one aws-user identity should log out all of them by removing the shared provider credentials.
Applied to files:
pkg/auth/manager_logout_test.gopkg/auth/cloud/aws/files.gopkg/auth/manager.gowebsite/docs/cli/commands/auth/logout.mdx
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#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:
pkg/auth/cloud/aws/files.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#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:
pkg/auth/cloud/aws/files.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*.go : Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Applied to files:
pkg/auth/cloud/aws/files.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*.go : Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
Applied to files:
pkg/auth/cloud/aws/files.go
📚 Learning: 2025-09-29T15:47:10.908Z
Learnt from: aknysh
PR: cloudposse/atmos#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:
pkg/auth/cloud/aws/files.go
📚 Learning: 2024-10-28T01:51:30.811Z
Learnt from: osterman
PR: cloudposse/atmos#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:
pkg/auth/cloud/aws/files.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
pkg/auth/cloud/aws/files.go
📚 Learning: 2025-09-23T04:43:31.857Z
Learnt from: osterman
PR: cloudposse/atmos#1430
File: pkg/config/config.go:98-107
Timestamp: 2025-09-23T04:43:31.857Z
Learning: In the Atmos codebase, NO_PAGER environment variable handling is intentionally kept as direct os.Getenv() access in pkg/config/config.go rather than using Viper binding, because adding no_pager to the config file would be confusing for users. This is an acknowledged exception to the normal Viper binding pattern for environment variables.
Applied to files:
pkg/auth/cloud/aws/files.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
PR: cloudposse/atmos#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:
pkg/auth/manager.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/!(*_test).go : Add `defer perf.Track()` to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.
Applied to files:
cmd/auth_logout.go
🧬 Code graph analysis (4)
pkg/auth/manager_logout_test.go (4)
pkg/auth/types/mock_interfaces.go (6)
MockCredentialStore(515-518)MockProvider(17-20)MockIdentity(140-143)NewMockCredentialStore(526-530)NewMockProvider(28-32)NewMockIdentity(151-155)pkg/schema/schema_auth.go (2)
AuthConfig(4-8)IdentityVia(42-45)pkg/auth/types/interfaces.go (2)
Provider(13-37)Identity(40-64)errors/errors.go (5)
ErrIdentityNotInConfig(381-381)ErrPartialLogout(375-375)ErrLogoutFailed(374-374)ErrProviderNotInConfig(382-382)ErrLogoutNotSupported(376-376)
pkg/auth/cloud/aws/files.go (2)
pkg/config/homedir/homedir.go (2)
Expand(68-87)Dir(36-63)errors/error_funcs.go (1)
CheckErrorAndPrint(29-52)
pkg/auth/manager.go (5)
pkg/perf/perf.go (1)
Track(121-138)pkg/auth/cloud/aws/files.go (1)
NewAWSFileManager(46-68)pkg/auth/cloud/aws/spec.go (1)
GetFilesBasePath(14-32)errors/errors.go (8)
ErrIdentityNotInConfig(381-381)ErrKeyringDeletion(378-378)ErrLogoutNotSupported(376-376)ErrProviderLogout(379-379)ErrIdentityLogout(380-380)ErrPartialLogout(375-375)ErrLogoutFailed(374-374)ErrProviderNotInConfig(382-382)pkg/auth/types/interfaces.go (2)
Provider(13-37)Identity(40-64)
cmd/auth_logout.go (7)
cmd/cmd_utils.go (1)
ComponentsArgCompletion(768-790)pkg/config/config.go (1)
InitCliConfig(25-62)errors/errors.go (6)
ErrFailedToInitializeAtmosConfig(117-117)ErrAuthManager(334-334)ErrIdentityNotInConfig(381-381)ErrPartialLogout(375-375)ErrProviderNotInConfig(382-382)ErrInvalidLogoutOption(383-383)pkg/perf/perf.go (1)
Track(121-138)pkg/auth/types/interfaces.go (1)
AuthManager(67-121)pkg/utils/log_utils.go (1)
PrintfMessageToTUI(33-35)internal/tui/utils/utils.go (1)
NewAtmosHuhTheme(85-93)
🪛 LanguageTool
docs/prd/auth-logout.md
[typographical] ~57-~57: Consider using typographic quotation marks here.
Context: ...elet Huh for styled selection - Allow "All identities" option - Apply Atmos theme to prompts...
(EN_QUOTES)
[grammar] ~661-~661: Ensure spelling is correct
Context: ...lement styled output with Charmbracelet lipgloss - [ ] Add progress indicators for multi-st...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[typographical] ~728-~728: Consider using typographic quotation marks here.
Context: ...upport --all flag or just interactive "All identities" option? Decision: Use interactive ...
(EN_QUOTES)
website/docs/cli/commands/auth/logout.mdx
[style] ~18-~18: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...ing Local Credentials Only This command only removes **credentials stored on your lo...
(ADVERB_REPETITION_PREMIUM)
[grammar] ~89-~89: Ensure spelling is correct
Context: ...s an interactive menu to choose what to logout from: ``` ? Choose what to logout from...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~219-~219: Ensure spelling is correct
Context: ...o completely end your session and fully logout: 1. Run atmos auth logout to remove local ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~403-~403: Ensure spelling is correct
Context: ...validate ``` ## Best Practices ### 1. Logout When Switching Contexts When switching...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~405-~405: Ensure spelling is correct
Context: ...n different identities or environments, logout first to ensure clean state: ```shell ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/auth-logout.md
148-148: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
629-629: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (4)
docs/prd/auth-logout.md (1)
746-751: Docs/code mismatch on partial success exit status.Spec says exit 0 on partial success. CLI currently treats provider/all partial errors as failures. See cmd/auth_logout.go error handling in performProviderLogout/performLogoutAll.
Want me to align the CLI to return 0 on ErrPartialLogout? Based on learnings.
pkg/auth/manager.go (3)
12-12: Import alias usage looks good.aws file-manager helpers are used below; alias keeps call sites clear.
825-866: Both concerns already addressed in the codebase; review comment needs correction.The AWS pseudo-provider handling is fully implemented—
aws.IsStandaloneAWSUserChain()exists,GetProviderForIdentity()returns "aws-user" for standalone users (line 328–329), andLogoutProviderhas fallback logic to handle identities as pseudo-providers (line 779–780).Regarding perf tracking: all public methods in the file follow the pattern of
defer perf.Track()at entry, but private helpers are not systematically tracked—only the public methods. SinceresolveProviderForIdentityis a private helper called exclusively from the already-trackedLogoutProvider, adding separate tracking here would deviate from the established pattern and likely add noise without meaningful visibility gains.The original concern about aws-user appears to misread the codebase; the requirement is already satisfied.
Likely an incorrect or invalid review comment.
729-823: The review comment's error handling suggestion is incorrect and won't work as written.zalando/go-keyring's Delete method returns
nilfor missing keys (most backends treat delete-nonexistent as success). The current code wraps keyring errors withErrCredentialStore, neverErrNoCredentialsFound. The suggestion to checkerrors.Is(err, errUtils.ErrNoCredentialsFound)will never match—Delete doesn't return that error, and semantically it's used for Retrieve failures, not deletions.Additionally, the current Delete implementation already handles missing keys gracefully (keyring library returns nil), so the special case check is unnecessary.
The chain cleanup logic (Step 4) is a separate concern that may have merit based on the learnings about aws-user identities, but it shouldn't be bundled with a flawed error-handling refactor.
Likely an incorrect or invalid review comment.
- Add ErrPartialLogout handling to cmd/auth_logout.go (treat as success/exit 0) - Fix N+1 provider.Logout calls in LogoutProvider using context flag - Treat keyring.ErrNotFound as success in credentialStore.Delete - Fix test map types to use types.Provider and types.Identity - Update test expectations to expect Times(1) for provider operations - Add comprehensive test for LogoutProvider with failures - Test coverage increased from 79.6% to 80.8%
…tials - Add CleanupIdentity() method to AWSFileManager - Remove only specific identity's sections from shared INI files - Prevent deletion of other identities' credentials when logging out one identity - Add removeIniSection() helper to safely remove INI sections - Update aws-user identity to use CleanupIdentity instead of Cleanup This fixes the critical issue where logging out one aws-user identity would delete credentials for all identities using the same provider.
…ntials - Delete now treats keyring.ErrNotFound as success (idempotent) - Updated test to assert.NoError instead of assert.Error - Added test for deleting already-deleted credential (idempotent behavior)
- Already have 2025-10-17-auth-logout-feature.md - Shouldn't have created a duplicate
- Reframe problem: cloud tooling doesn't make logout easy - Focus on credential sprawl across AWS, Azure, GCP - Emphasize manual cleanup burden and security risks - Position Atmos logout as solving general cloud tooling gap - Remove implication that Atmos previously lacked logout
- Add context about cloud tooling not making logout easy - Mention credential sprawl across AWS, Azure, GCP - Frame logout as making cleanup explicit and comprehensive - Maintain existing detailed documentation structure
- Keep intro concise and focused on what the command does - Add 'The Problem' section below intro - Explains credential sprawl and lack of easy logout in cloud tooling - Positions Atmos logout as the solution
…-to-remove-local-credential-store
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
NOTICE(2 hunks)website/blog/2025-10-17-auth-logout-feature.md(1 hunks)website/docs/cli/commands/auth/logout.mdx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in website/ when adding features
Ensure consistency between CLI help text and website documentation
Follow the website's documentation structure and style
Keep website code in website/ and follow its architecture/style; test changes locally
Keep CLI and website documentation in sync; document new features with examples and use casesAlways build the website (npm run build) after documentation changes to ensure no errors, broken links, or missing assets.
Files:
website/docs/cli/commands/auth/logout.mdxwebsite/blog/2025-10-17-auth-logout-feature.md
website/docs/cli/commands/**/*.mdx
📄 CodeRabbit inference engine (CLAUDE.md)
All new commands/flags/parameters must have Docusaurus documentation under website/docs/cli/commands//.mdx using the prescribed frontmatter and sections.
Files:
website/docs/cli/commands/auth/logout.mdx
website/**/{*.md,*.mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use definition lists
for arguments and flags in docs instead of tables.
Files:
website/docs/cli/commands/auth/logout.mdxwebsite/blog/2025-10-17-auth-logout-feature.md
website/blog/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
website/blog/*.md: PRs labeled minor or major must include a blog post in website/blog/YYYY-MM-DD-feature-name.md with required frontmatter, tags, and truncate marker.
Use appropriate audience tags (feature/enhancement/bugfix or contributors) and relevant secondary tags in blog posts.
Files:
website/blog/2025-10-17-auth-logout-feature.md
🪛 LanguageTool
website/docs/cli/commands/auth/logout.mdx
[style] ~18-~18: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...ing Local Credentials Only This command only removes **credentials stored on your lo...
(ADVERB_REPETITION_PREMIUM)
[grammar] ~109-~109: Ensure spelling is correct
Context: ...s an interactive menu to choose what to logout from: ``` ? Choose what to logout from...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~239-~239: Ensure spelling is correct
Context: ...o completely end your session and fully logout: 1. Run atmos auth logout to remove local ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~423-~423: Ensure spelling is correct
Context: ...validate ``` ## Best Practices ### 1. Logout When Switching Contexts When switching...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~425-~425: Ensure spelling is correct
Context: ...n different identities or environments, logout first to ensure clean state: ```shell ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
website/blog/2025-10-17-auth-logout-feature.md
[grammar] ~182-~182: Please add a punctuation mark at the end of paragraph.
Context: ...he browser session 4. Close all browser windows The command displays this warning afte...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~186-~186: Ensure spelling is correct
Context: ...o ensure you don't forget. ### When to Logout **Logout at the end of your work session:*...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~311-~311: Please add a punctuation mark at the end of paragraph.
Context: ...ath` package for platform-specific path separators ### Error Handling Following Atmos er...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~320-~320: Please add a punctuation mark at the end of paragraph.
Context: ...on-fatal errors - Reports all errors at completion ### Interface Extensions The logout f...
(PUNCTUATION_PARAGRAPH_END)
🪛 markdownlint-cli2 (0.18.1)
website/blog/2025-10-17-auth-logout-feature.md
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
153-153: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build (windows)
- GitHub Check: Analyze (go)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Review Dependency Licenses
- GitHub Check: website-deploy-preview
- GitHub Check: autofix
- GitHub Check: Summary
🔇 Additional comments (3)
website/docs/cli/commands/auth/logout.mdx (1)
1-469: Documentation structure and content look solid.The mdx file follows all guidelines: proper frontmatter, definition lists for args/flags, comprehensive sections covering usage, examples, security, troubleshooting, and advanced config. Content is consistent with the blog post but appropriately tailored for reference documentation.
Per coding guidelines, please confirm that you ran
npm run buildin thewebsite/directory to validate the Docusaurus build, check for broken links, and ensure all components render correctly.NOTICE (2)
677-679: Looks good!The
go-difflibdependency entry is correctly formatted and placed in the BSD-3-Clause section. This is a common testing utility, typically used by testify for assertion diffs.
1479-1481: Verified—NOTICE entry is correct.Both license URLs are accessible and the testify entry is properly formatted with the correct version tag. No issues found.
- Tests were calling Reset() on mitchellh/go-homedir - Production code uses our forked pkg/config/homedir - This caused cache to not be cleared properly between test runs - Update both setup_test.go and files_test.go to use our fork - Tests now pass reliably when run multiple times - Add comprehensive Reset() tests to homedir package
- Add forbidigo rule to prevent importing github.com/mitchellh/go-homedir - Direct developers to use our forked pkg/config/homedir instead - Prevents future test flakiness from using wrong homedir package - Config verified with golangci-lint config verify
Blog post fixes (2025-10-17-auth-logout-feature.md): - Add shell language specifiers to 7 code blocks (lines 44, 71, 92, 113, 132, 153, 213) - Add missing periods to list items (lines 182, 311, 320) Homedir test fixes (pkg/config/homedir/homedir_test.go): - Add Reset() calls to TestDir and TestExpand to prevent cross-test interference - Fix TestExpand expected path construction (remove trailing separator) - Tests now pass reliably when run multiple times
aknysh
deleted the
feature/dev-3705-implement-atmos-auth-logout-to-remove-local-credential-store
branch
|
These changes were released in v1.196.0-rc.0. |
what
This pull request introduces the
atmos auth logoutcommand, enabling users to securely remove locally cached credentials. The command supports:why
This feature addresses several key pain points:
The implementation uses native Go operations for file system cleanup and integrates with
go-keyringfor cross-platform credential store access. It leverages Charmbracelet libraries for a polished interactive user experience and styled output.references
closes #735
Summary by CodeRabbit
Release Notes
New Features
atmos auth logoutCLI command to remove stored credentialsDocumentation