fix: Consolidate credential retrieval logic to fix terraform auth #1720
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates credential retrieval into a single keyring→identity-storage fallback, adds interactive no-value --identity selection, preserves identity name casing from atmos.yaml, adds AWS credential file locking and per-identity deletion, threads AuthManager into list renderers to show auth status/expiry, and propagates auth-provided env vars to subprocesses. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI
participant AuthMgr as AuthManager
participant Keyring as Keyring
participant Identity as IdentityStore
participant Proc as Spawned Process
User->>CLI: atmos auth env [--identity]
alt --identity without value
CLI->>AuthMgr: GetDefaultIdentity(forceSelect=true)
AuthMgr->>User: Prompt selector
User-->>AuthMgr: Select identity
else explicit identity
CLI->>AuthMgr: use provided identity
else omitted
CLI->>AuthMgr: GetDefaultIdentity(forceSelect=false)
end
CLI->>AuthMgr: loadCredentialsWithFallback(identity)
AuthMgr->>Keyring: Retrieve credentials
alt keyring hit
Keyring-->>AuthMgr: creds
else keyring miss
AuthMgr->>Identity: LoadCredentials(identity)
Identity-->>AuthMgr: creds / ErrNoStoredCredentials
end
AuthMgr->>AuthMgr: validate expiry (15m buffer)
alt creds valid
AuthMgr->>AuthMgr: GetEnvironmentVariables(identity)
AuthMgr-->>CLI: env map
CLI->>Proc: ConvertComponentEnvSectionToList + inject env
else expired
AuthMgr-->>CLI: prompt re-auth or return error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/auth/manager.go (1)
202-216: Unify expiration logic with isCredentialValid.GetCachedCredentials uses creds.IsExpired() while other paths use isCredentialValid (5‑min buffer). Aligning avoids “whoami says valid” while auth treats as near‑expiry.
Suggested change:
- // Check if credentials are expired. - if creds.IsExpired() { - log.Debug("Cached credentials are expired", logKeyIdentity, identityName) - return nil, fmt.Errorf(errFormatWithString, errUtils.ErrExpiredCredentials, fmt.Sprintf(backtickedFmt, identityName)) - } + // Consistent validation (treat expiring within 5m as invalid). + valid, _ := m.isCredentialValid(identityName, creds) + if !valid { + log.Debug("Cached credentials are expired", logKeyIdentity, identityName) + return nil, fmt.Errorf(errFormatWithString, errUtils.ErrExpiredCredentials, fmt.Sprintf(backtickedFmt, identityName)) + }Also applies to: 218-225
🧹 Nitpick comments (8)
pkg/auth/manager_caching_test.go (1)
349-351: Strengthen failure assertions.When expectSuccess is false, also assert the error type once retrieveCredentialWithFallback standardizes “no creds in identity storage” to a static error. This will harden the regression signal.
Example:
- } else { - require.Error(t, err, "retrieveCachedCredentials should fail when no credentials available") - } + } else { + require.Error(t, err, "retrieveCachedCredentials should fail when no credentials available") + assert.ErrorIs(t, err, errUtils.ErrNoCredentialsFound) + }(Based on learnings)
pkg/auth/manager_test.go (1)
583-631: Minor: add a negative scenario (optional).Consider a second subtest with hasCredentials = false to assert the retrieval error, mirroring the positive path.
docs/prd/credential-retrieval-consolidation.md (3)
89-109: Add language to fenced code blocks.Label unlabeled fences to satisfy MD040 and improve readability.
Example:
-``` +```text cmd/auth_whoami.go → pkg/auth/hooks.go: AuthWhoami() ... -``` +```Repeat for the other unlabeled block(s). (From static analysis)
Also applies to: 97-109, 240-246
5-7: Trim wording.Replace “exact same credentials” with “the same credentials.”
381-399: Phase note may be outdated.This PR already implements the consolidation; update “Phase 2: Next PR” to reflect completion (e.g., “Completed in PR #1720.”).
pkg/auth/manager.go (3)
507-534: Context and target-step retrieval consistency.
- Using context.Background() here loses caller cancellation. Prefer threading ctx through findFirstValidCachedCredentials (signature change) and its callers.
- When this returns len(chain)-1, authenticateHierarchical later re-fetches via keyring only; consider using retrieveCredentialWithFallback there too to avoid a redundant miss then re-fetch later.
632-671: Error semantics and log keys in fallback retriever.Standardize errors and log keys for consistency with the rest of the package.
Proposed tweaks:
- log.Debug("Retrieved credentials from keyring", "identity", identityName) + log.Debug("Retrieved credentials from keyring", logKeyIdentity, identityName) - if !errors.Is(err, credentials.ErrCredentialsNotFound) { - return nil, fmt.Errorf("keyring error for identity %q: %w", identityName, err) - } + if !errors.Is(err, credentials.ErrCredentialsNotFound) { + return nil, fmt.Errorf("%w: keyring read failed for %s: %w", + errUtils.ErrNoCredentialsFound, fmt.Sprintf(backtickedFmt, identityName), err) + } - if !exists { - return nil, fmt.Errorf("identity %q not found in configuration", identityName) - } + if !exists { + return nil, fmt.Errorf(errFormatWithString, errUtils.ErrIdentityNotFound, fmt.Sprintf(backtickedFmt, identityName)) + } - loadedCreds, loadErr := identity.LoadCredentials(ctx) + loadedCreds, loadErr := identity.LoadCredentials(ctx) if loadErr != nil { - return nil, fmt.Errorf("failed to load credentials from identity storage for %q: %w", identityName, loadErr) + return nil, fmt.Errorf("%w: failed to load credentials from identity storage for %s: %w", + errUtils.ErrNoCredentialsFound, fmt.Sprintf(backtickedFmt, identityName), loadErr) } if loadedCreds == nil { - return nil, fmt.Errorf("loaded credentials are nil for identity %q", identityName) + return nil, fmt.Errorf(errFormatWithString, errUtils.ErrNoCredentialsFound, fmt.Sprintf(backtickedFmt, identityName)) } - log.Debug("Successfully loaded credentials from identity storage", "identity", identityName) + log.Debug("Successfully loaded credentials from identity storage", logKeyIdentity, identityName)This keeps callers free to use errors.Is while maintaining detailed context. (As per coding guidelines)
676-683: Avoid Background; thread context into retrieval.retrieveCachedCredentials uses context.Background(). Consider adding ctx to fetchCachedCredentials/retrieveCachedCredentials to preserve cancellation across terraform flows.
Example signature:
-func (m *manager) fetchCachedCredentials(startIndex int) (types.ICredentials, int) +func (m *manager) fetchCachedCredentials(ctx context.Context, startIndex int) (types.ICredentials, int)and pass ctx through to retrieveCredentialWithFallback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
docs/prd/credential-retrieval-consolidation.md(1 hunks)pkg/auth/manager.go(5 hunks)pkg/auth/manager_caching_test.go(1 hunks)pkg/auth/manager_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/manager_caching_test.gopkg/auth/manager.gopkg/auth/manager_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + DI; table-driven tests for coverage; target >80% coverage.
Tests must exercise production code paths; avoid duplicating implementation logic in tests.
Use t.Skipf("reason") with clear context when skipping tests; CLI tests may auto-build temp binaries.
Files:
pkg/auth/manager_caching_test.gopkg/auth/manager_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: Define interfaces for major functionality and use dependency injection to enable testability.
Generate mocks using go.uber.org/mock/mockgen via //go:generate directives (never manual mocks).
Prefer functional Options pattern instead of functions with many parameters for configuration.
Use context.Context only for cancellation, deadlines/timeouts, and request-scoped values; never for configuration or dependencies; context should be the first parameter.
All comments must end with periods (godot linter).
Preserve helpful existing comments; update them to match code when refactoring; only remove when incorrect, redundant, or code removed.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() (or nil if no atmosConfig) plus a blank line at the start of all public functions for performance tracking.
Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.
Never use //revive:disable:file-length-limit to bypass file size limits; keep files small and focused (<600 lines).
Environment variables must be bound with viper.BindEnv using ATMOS_ prefix (e.g., viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")).
Ensure cross-platform compatibility: use filepath.Join instead of hardcoded path separators and prefer SDKs over shelling out to binaries.
Files:
pkg/auth/manager_caching_test.gopkg/auth/manager.gopkg/auth/manager_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/auth/manager.go
docs/prd/[a-z0-9-]*.md
📄 CodeRabbit inference engine (CLAUDE.md)
All PRDs must be placed under docs/prd/ with kebab-case filenames.
Files:
docs/prd/credential-retrieval-consolidation.md
🧬 Code graph analysis (3)
pkg/auth/manager_caching_test.go (1)
pkg/schema/schema_auth.go (1)
AuthConfig(4-9)
pkg/auth/manager.go (3)
pkg/logger/log.go (1)
Debug(24-26)pkg/auth/types/interfaces.go (1)
ICredentials(230-241)pkg/auth/credentials/keyring_file.go (1)
ErrCredentialsNotFound(36-36)
pkg/auth/manager_test.go (3)
pkg/auth/credentials/keyring_file.go (1)
ErrCredentialsNotFound(36-36)pkg/schema/schema_auth.go (2)
AuthConfig(4-9)Identity(45-53)pkg/auth/types/interfaces.go (3)
Identity(70-114)ICredentials(230-241)PostAuthenticateParams(61-67)
🪛 LanguageTool
docs/prd/credential-retrieval-consolidation.md
[style] ~5-~5: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...(e.g., atmos terraform plan) with the exact same credentials. ## Current Architecture A...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[typographical] ~82-~82: Consider using a typographic opening quote here.
Context: ...tion** without a shared abstraction for "retrieve credential from any available s...
(EN_QUOTES)
[typographical] ~82-~82: Consider using a typographic close quote here.
Context: ...ve credential from any available source." ## Why This Manifested in Terraform Co...
(EN_QUOTES)
[typographical] ~114-~114: Consider using typographic quotation marks here.
Context: ...ion** Three separate implementations of "retrieve credential" logic with inconsistent behavior. ### ...
(EN_QUOTES)
[typographical] ~117-~117: Consider using a typographic opening quote here.
Context: ...ngle method/interface that encapsulates "retrieve credential from any available s...
(EN_QUOTES)
[typographical] ~117-~117: Consider using a typographic close quote here.
Context: ...y available source with proper fallback." ### 3. Implicit Assumptions `retri...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic opening quote here.
Context: ...s, headless systems, no D-Bus) - We use "noop" keyring in these environments ###...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic close quote here.
Context: ...adless systems, no D-Bus) - We use "noop" keyring in these environments ### 4. *...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/credential-retrieval-consolidation.md
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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 (linux)
- GitHub Check: Build (macos)
- GitHub Check: Build (windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Review Dependency Licenses
- GitHub Check: autofix
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/auth/manager_test.go (3)
533-581: Nice regression test; this nails the original bug.Covers the terraform execution path precisely and verifies identity-storage fallback. LGTM.
389-397: Time pointer setup looks good.Switch to explicit expired/valid times via variables is clearer and avoids inline address-of on temporaries. LGTM.
Also applies to: 395-397
539-547: Fix comment punctuation for linters.End each comment line with a period to satisfy the godot linter.
Also applies to: 570-581
⛔ Skipped due to learnings
Learnt from: CR PR: cloudposse/atmos#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-24T14:51:19.355Z Learning: Applies to **/*.go : All comments must end with periods (godot linter).
osterman
force-pushed
the
fix-terraform-auth-context
branch
from
4da5aab to
4219b23
Compare
This commit fixes a bug where `atmos terraform plan` and other Terraform commands failed to use file-based credentials, while `atmos auth whoami` and similar commands worked correctly. The root cause was duplicate credential retrieval code across three methods with inconsistent fallback behavior. ## Problem Three separate code paths retrieved credentials: 1. `GetCachedCredentials` - Had keyring → identity storage fallback ✓ 2. `findFirstValidCachedCredentials` - Had fallback logic ✓ 3. `retrieveCachedCredentials` - NO fallback (the bug!) ✗ When users authenticated via AWS SSO (credentials in files, not keyring), Terraform commands would fail because only `retrieveCachedCredentials` was used in that code path, and it didn't have fallback logic. ## Solution Extracted a shared `retrieveCredentialWithFallback` method that implements the single source of truth for credential retrieval. All three code paths now delegate to this method, ensuring consistent behavior: - Tries keyring cache first (fast path) - Falls back to identity storage if not in keyring (slow path) - Provides detailed logging and error handling ## Changes - Added `retrieveCredentialWithFallback()` method (38 lines) - Refactored `GetCachedCredentials()` (40% code reduction) - Refactored `findFirstValidCachedCredentials()` (57% code reduction) - Refactored `retrieveCachedCredentials()` (now uses shared method) - Fixed `TestManager_GetCachedCredentials_Paths` to use proper test data ## Result ✅ All three credential retrieval paths now behave identically ✅ Terraform commands work with file-based credentials ✅ ~110 lines of duplicate code eliminated ✅ All tests pass ✅ Regression test included to prevent this class of bug Fixes the issue where Terraform commands failed with valid session credentials. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
osterman
force-pushed
the
fix-terraform-auth-context
branch
from
4219b23 to
76b0d1d
Compare
- Fix mock Identity.LoadCredentials() to return error when no credentials exist
- Mimics real AWS identity behavior where LoadCredentials fails if credentials
haven't been written to ~/.aws/credentials via authentication
- This ensures auth fallback logic only uses identity storage when credentials
actually exist, not as a bypass for authentication
- Add AWS_REGION and AWS_DEFAULT_REGION to mock Identity.Environment()
- Matches real AWS identity environment variable exports
- Update test snapshots to reflect correct authentication flow
- INFO log 'Chained identity' now appears when authentication occurs
- Mock identity tests now expect LoadCredentials to return error
- Add regex to normalize 'Last Updated' timestamps in auth whoami output - Replaces dynamic timestamps like '2025-10-28 13:10:27 CDT' with fixed '2025-01-01 12:00:00 UTC' - Prevents snapshot test failures due to time-sensitive credential metadata - Update affected snapshot with normalized timestamp
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/auth/manager_caching_test.go (1)
289-293: Fix comment punctuation for linters.End the first comment line with a period to satisfy godot.
-// TestRetrieveCachedCredentials_KeyringMiss_IdentityStorageFallback tests that +// TestRetrieveCachedCredentials_KeyringMiss_IdentityStorageFallback tests that.Based on learnings.
🧹 Nitpick comments (9)
pkg/auth/providers/mock/identity.go (1)
86-88: Region defaults in mock env: good; consider a shared constant.LGTM. To avoid drift with Authenticate()’s "us-east-1", consider a file‑level const and reuse.
Example:
const mockRegion = "us-east-1"Use mockRegion in Environment() and Authenticate().
pkg/auth/manager_caching_test.go (1)
322-328: Run subtests in parallel.Safe here and speeds up the suite.
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel()pkg/auth/providers/mock/identity_test.go (1)
329-335: Prefer ErrorContains over stringing err.Error().Improves readability and failure messages.
- require.Error(t, err) - require.Nil(t, creds) - assert.Contains(t, err.Error(), "no stored credentials") + require.Error(t, err) + require.Nil(t, creds) + assert.ErrorContains(t, err, "no stored credentials")docs/prd/credential-retrieval-consolidation.md (2)
89-109: Add language to fenced code blocks (markdownlint MD040).Mark these as text.
-``` +```text cmd/auth_whoami.go → pkg/auth/hooks.go: AuthWhoami() → pkg/auth/manager.go: Whoami() → pkg/auth/manager.go: GetCachedCredentials() ✅ Has fallback -``` +``` -``` +```text cmd/terraform.go → internal/exec/terraform.go: ExecuteTerraform() → pkg/auth/hooks.go: TerraformPreHook() → pkg/auth/manager.go: Authenticate() → pkg/auth/manager.go: authenticateHierarchical() → pkg/auth/manager.go: authenticateFromIndex() → pkg/auth/manager.go: authenticateProviderChain() → pkg/auth/manager.go: fetchCachedCredentials() → pkg/auth/manager.go: retrieveCachedCredentials() ❌ No fallback -``` +```
240-244: Use a heading instead of bold (markdownlint MD036).Convert emphasized line to a proper heading.
-**Option 1: Extract Common Retrieval Logic** +### Option 1: Extract Common Retrieval Logicpkg/auth/manager.go (4)
507-512: Prefer propagating context over context.Background().Consider threading a ctx into findFirstValidCachedCredentials and using it here, to allow cancellation/timeouts in long-running checks.
Example (sketch):
- func (m *manager) findFirstValidCachedCredentials(ctx context.Context) int
- call m.loadCredentialsWithFallback(ctx, identityName)
Also applies to: 526-531
632-671: Unify “identity not found” error sentinel.Elsewhere you use ErrIdentityNotFound; here it’s ErrIdentityNotInConfig. Use a single sentinel for easier errors.Is checks.
- if !exists { - return nil, fmt.Errorf("%w: %s", errUtils.ErrIdentityNotInConfig, identityName) - } + if !exists { + return nil, fmt.Errorf("%w: %s", errUtils.ErrIdentityNotFound, identityName) + }
673-683: Consider passing ctx into getChainCredentials.Avoid context.Background() inside; add ctx param and forward to loadCredentialsWithFallback for consistency.
Sketch:
- func (m *manager) getChainCredentials(ctx context.Context, chain []string, startIndex int) (...)
- callers pass ctx from authenticateProviderChain/fetchCachedCredentials.
477-497: Optional: use the unified fallback when target has valid cached creds.When validFromIndex == last, you re-read keyring directly; if it’s a keyring miss but files exist, you’ll fetch later anyway. Using loadCredentialsWithFallback here is simpler and avoids the extra miss.
- if validFromIndex == len(m.chain)-1 { - last := m.chain[len(m.chain)-1] - if cachedCreds, err := m.credentialStore.Retrieve(last); err == nil { - log.Debug("Using cached credentials for target identity", logKeyIdentity, targetIdentity) - return cachedCreds, nil - } - } + if validFromIndex == len(m.chain)-1 { + last := m.chain[len(m.chain)-1] + if cachedCreds, err := m.loadCredentialsWithFallback(context.Background(), last); err == nil { + log.Debug("Using cached credentials for target identity", logKeyIdentity, targetIdentity) + return cachedCreds, 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 (13)
docs/prd/credential-retrieval-consolidation.md(1 hunks)pkg/auth/manager.go(8 hunks)pkg/auth/manager_caching_test.go(1 hunks)pkg/auth/manager_test.go(3 hunks)pkg/auth/providers/mock/identity.go(2 hunks)pkg/auth/providers/mock/identity_test.go(2 hunks)tests/snapshots/TestCLICommands_atmos_auth_env_--format_dotenv_--identity_mock-identity.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_env_--format_json_--identity_mock-identity.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_env_--login_--format_bash.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_env_--login_without_cached_credentials.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_env_without_--login_returns_config_env_vars.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_exec_without_authentication.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_whoami_without_authentication.stderr.golden(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- tests/snapshots/TestCLICommands_atmos_auth_exec_without_authentication.stderr.golden
- tests/snapshots/TestCLICommands_atmos_auth_env_--format_json_--identity_mock-identity.stdout.golden
- tests/snapshots/TestCLICommands_atmos_auth_env_--login_--format_bash.stderr.golden
🧰 Additional context used
📓 Path-based instructions (6)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/providers/mock/identity_test.gopkg/auth/providers/mock/identity.gopkg/auth/manager.gopkg/auth/manager_caching_test.gopkg/auth/manager_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + DI; table-driven tests for coverage; target >80% coverage.
Tests must exercise production code paths; avoid duplicating implementation logic in tests.
Use t.Skipf("reason") with clear context when skipping tests; CLI tests may auto-build temp binaries.
Files:
pkg/auth/providers/mock/identity_test.gopkg/auth/manager_caching_test.gopkg/auth/manager_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: Define interfaces for major functionality and use dependency injection to enable testability.
Generate mocks using go.uber.org/mock/mockgen via //go:generate directives (never manual mocks).
Prefer functional Options pattern instead of functions with many parameters for configuration.
Use context.Context only for cancellation, deadlines/timeouts, and request-scoped values; never for configuration or dependencies; context should be the first parameter.
All comments must end with periods (godot linter).
Preserve helpful existing comments; update them to match code when refactoring; only remove when incorrect, redundant, or code removed.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() (or nil if no atmosConfig) plus a blank line at the start of all public functions for performance tracking.
Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.
Never use //revive:disable:file-length-limit to bypass file size limits; keep files small and focused (<600 lines).
Environment variables must be bound with viper.BindEnv using ATMOS_ prefix (e.g., viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")).
Ensure cross-platform compatibility: use filepath.Join instead of hardcoded path separators and prefer SDKs over shelling out to binaries.
Files:
pkg/auth/providers/mock/identity_test.gopkg/auth/providers/mock/identity.gopkg/auth/manager.gopkg/auth/manager_caching_test.gopkg/auth/manager_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/auth/providers/mock/identity.gopkg/auth/manager.go
docs/prd/[a-z0-9-]*.md
📄 CodeRabbit inference engine (CLAUDE.md)
All PRDs must be placed under docs/prd/ with kebab-case filenames.
Files:
docs/prd/credential-retrieval-consolidation.md
tests/snapshots/**
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually edit golden snapshot files; regenerate using -regenerate-snapshots.
Files:
tests/snapshots/TestCLICommands_atmos_auth_env_--format_dotenv_--identity_mock-identity.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_env_without_--login_returns_config_env_vars.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_env_--login_without_cached_credentials.stderr.goldentests/snapshots/TestCLICommands_atmos_auth_whoami_without_authentication.stderr.golden
🧠 Learnings (1)
📚 Learning: 2025-10-24T14:51:19.355Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-24T14:51:19.355Z
Learning: Applies to **/*.go : All comments must end with periods (godot linter).
Applied to files:
pkg/auth/manager_caching_test.go
🧬 Code graph analysis (4)
pkg/auth/providers/mock/identity.go (2)
pkg/auth/types/interfaces.go (2)
Identity(70-114)ICredentials(230-241)pkg/perf/perf.go (1)
Track(121-138)
pkg/auth/manager.go (4)
pkg/logger/log.go (1)
Debug(24-26)pkg/auth/types/interfaces.go (1)
ICredentials(230-241)pkg/auth/credentials/keyring_file.go (1)
ErrCredentialsNotFound(36-36)errors/errors.go (2)
ErrIdentityNotInConfig(428-428)ErrNoCredentialsFound(388-388)
pkg/auth/manager_caching_test.go (1)
pkg/schema/schema_auth.go (1)
AuthConfig(4-9)
pkg/auth/manager_test.go (2)
pkg/auth/credentials/keyring_file.go (1)
ErrCredentialsNotFound(36-36)pkg/auth/types/interfaces.go (3)
Identity(70-114)ICredentials(230-241)PostAuthenticateParams(61-67)
🪛 LanguageTool
docs/prd/credential-retrieval-consolidation.md
[style] ~5-~5: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...(e.g., atmos terraform plan) with the exact same credentials. ## Current Architecture A...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[typographical] ~82-~82: Consider using a typographic opening quote here.
Context: ...tion** without a shared abstraction for "retrieve credential from any available s...
(EN_QUOTES)
[typographical] ~82-~82: Consider using a typographic close quote here.
Context: ...ve credential from any available source." ## Why This Manifested in Terraform Co...
(EN_QUOTES)
[typographical] ~114-~114: Consider using typographic quotation marks here.
Context: ...ion** Three separate implementations of "retrieve credential" logic with inconsistent behavior. ### ...
(EN_QUOTES)
[typographical] ~117-~117: Consider using a typographic opening quote here.
Context: ...ngle method/interface that encapsulates "retrieve credential from any available s...
(EN_QUOTES)
[typographical] ~117-~117: Consider using a typographic close quote here.
Context: ...y available source with proper fallback." ### 3. Implicit Assumptions `retri...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic opening quote here.
Context: ...s, headless systems, no D-Bus) - We use "noop" keyring in these environments ###...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic close quote here.
Context: ...adless systems, no D-Bus) - We use "noop" keyring in these environments ### 4. *...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/credential-retrieval-consolidation.md
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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 (linux)
- GitHub Check: Build (windows)
- GitHub Check: autofix
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Review Dependency Licenses
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
🔇 Additional comments (9)
tests/snapshots/TestCLICommands_atmos_auth_env_--login_without_cached_credentials.stderr.golden (1)
3-3: Snapshot addition looks fine; please confirm it was auto‑generated.Ensure this golden was updated via the snapshot regeneration flow, not manual edits.
Per guidelines, regenerate with -regenerate-snapshots and commit the result to avoid drift.
tests/snapshots/TestCLICommands_atmos_auth_env_--format_dotenv_--identity_mock-identity.stdout.golden (1)
2-2: Added region exports are expected.LGTM. Please confirm the snapshot was regenerated (not hand‑edited).
Also applies to: 4-4
tests/snapshots/TestCLICommands_atmos_auth_whoami_without_authentication.stderr.golden (1)
3-3: Output changes align with the new mock behavior.Looks good. Please ensure these goldens were regenerated via the sanctioned flag.
Also applies to: 11-12
tests/snapshots/TestCLICommands_atmos_auth_env_without_--login_returns_config_env_vars.stdout.golden (1)
2-2: Region exports are expected with the new mock env.Looks good. Please confirm this golden was regenerated via the supported workflow.
Also applies to: 4-4
pkg/auth/manager_test.go (1)
522-530: LGTM: switch to getChainCredentials.Matches the unified fallback path and aligns tests with prod behavior.
pkg/auth/manager.go (4)
202-216: LGTM: centralize retrieval via loadCredentialsWithFallback.Good consolidation and error wrapping with ErrNoCredentialsFound; keeps behavior consistent.
219-221: LGTM: explicit expired check.Clear and matches whoami semantics.
141-146: LGTM: authenticateChain rename and call site.Name and flow read better; no behavior concerns.
599-607: LGTM: fetchCachedCredentials now uses the unified path.This removes the pre-fix inconsistency that broke Terraform flows.
Redesign mock identity to support stateful credential storage simulation, making it provider-agnostic and suitable for testing diverse provider types (AWS, Azure, GitHub, etc.). Changes: - Add hasStoredCredentials flag to track credential persistence state - Update PostAuthenticate() to mark credentials as stored (simulates writing to ~/.aws/credentials, GitHub token file, etc.) - Update LoadCredentials() to check storage state: - Returns error if no credentials stored (before authentication) - Returns mock credentials if stored (after authentication) - Update Logout() to clear stored credentials state - Enhance tests to cover both credential states: - Before authentication: LoadCredentials fails (no stored credentials) - After authentication: LoadCredentials succeeds (credentials loaded) - After logout: LoadCredentials fails again (credentials cleared) Benefits: - Provider-agnostic design supports testing different storage mechanisms: * File-based: AWS ~/.aws/credentials, Azure ~/.azure/ * Token-based: GitHub environment variables or token files * Certificate-based: SSL/TLS certs on disk * Ephemeral: No persistent storage - Tests both credential fallback paths (keyring miss → identity storage) - More realistic simulation of real provider authentication lifecycle - Maintains deterministic fixed timestamps for snapshot stability All tests pass with no snapshot changes required.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1720 +/- ##
========================================
Coverage 68.62% 68.63%
========================================
Files 378 379 +1
Lines 34154 34600 +446
========================================
+ Hits 23439 23748 +309
- Misses 8528 8623 +95
- Partials 2187 2229 +42
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Fix PRD documentation to use correct function name loadCredentialsWithFallback (was incorrectly referencing retrieveCredentialWithFallback) - Update function references to getChainCredentials (formerly retrieveCachedCredentials) - Fix XDG directory references (Atmos uses XDG directories, not ~/.aws/credentials) - Add terminal punctuation to test comment in manager_test.go to satisfy godot linter - Capitalize continuation sentence per godot linter requirements - Add static sentinel error ErrNoStoredCredentials in mock identity - Follows error-handling guidelines for static errors - Removes dynamic error with nolint directive - Use fmt.Errorf with %w to wrap sentinel error with context - Update LoadCredentials comment to mention ErrNoStoredCredentials return type - Clarify XDG directory usage in comments All tests pass with no behavioral changes.
Make auth manager more provider-agnostic by removing AWS-specific assumptions: - Remove '(non-AWS)' from log messages - Replace with provider-agnostic 'expiration: none' for non-expiring credentials - Clarifies that nil expiration means long-lived tokens/API keys, not 'non-AWS' - Supports GitHub tokens, API keys, certificates, and other non-expiring credentials - Fix default credential path from ~/.aws/atmos to ~/.config/atmos - Follows XDG Base Directory Specification - Makes fallback path provider-agnostic - Update test expectations to match Note: AWS-specific code still exists for aws/user identity kind (lines 422-424, 557-558). There's a TODO comment acknowledging this should be handled via identity interface. That's a larger refactor beyond this PR's scope. All tests pass.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/auth/providers/mock/identity.go (1)
126-133: Fix inconsistency between CredentialsExist and LoadCredentials.
CredentialsExist()always returns true, butLoadCredentials()will fail ifhasStoredCredentialsis false. This breaks the contract: callers expect that ifCredentialsExist()returns true,LoadCredentials()should succeed.Apply this diff:
-// CredentialsExist always returns true for mock identities (credentials are in-memory). +// CredentialsExist checks if credentials have been stored via PostAuthenticate. func (i *Identity) CredentialsExist() (bool, error) { defer perf.Track(nil, "mock.Identity.CredentialsExist")() - // Mock identities don't use file-based storage. - // Credentials are always available in-memory. - return true, nil + // Return whether credentials have been stored after authentication. + return i.hasStoredCredentials, nil }
♻️ Duplicate comments (1)
docs/prd/credential-retrieval-consolidation.md (1)
136-168: Unify function name across PRD to match code.Doc uses both “retrieveCredentialWithFallback” and “loadCredentialsWithFallback”. Code implements loadCredentialsWithFallback; keep PRD consistent.
- ## Recommended Approach - **Option 1: Extract Common Retrieval Logic** - 1. Create `retrieveCredentialWithFallback` as the single retrieval method + ## Recommended Approach + **Option 1: Extract Common Retrieval Logic** + 1. Create `loadCredentialsWithFallback` as the single retrieval methodAlso ensure all bullet references (GetCachedCredentials/findFirstValidCachedCredentials/getChainCredentials) point to loadCredentialsWithFallback.
Also applies to: 240-245
🧹 Nitpick comments (12)
tests/cli_test.go (1)
396-401: Harden the “Last Updated” regex to cover numeric offset timezones.Current pattern misses forms like +00:00 or GMT+01. Slight tweak improves resilience; behavior unchanged.
- lastUpdatedRegex := regexp.MustCompile(`Last Updated\s+\d{4}-\d{2}-\d{2}\s+\d{2}:\d{2}:\d{2}\s+[A-Z]{3,4}`) + // Accept UTC/abbr or numeric offsets (e.g. +00:00, +0000). + lastUpdatedRegex := regexp.MustCompile( + `Last Updated\s+\d{4}-\d{2}-\d{2}\s+\d{2}:\d{2}:\d{2}\s+(?:UTC|[A-Z]{3,4}|[+-]\d{2}:?\d{2})`, + )Optional: precompile at package scope to avoid recompiling per call.
docs/prd/credential-retrieval-consolidation.md (2)
89-109: Add code-fence languages for markdown linters.The two flow blocks lack a language spec (MD040). Use text for call flows.
- ``` + ```text cmd/auth_whoami.go → pkg/auth/hooks.go: AuthWhoami() ... - ``` + ```Repeat for the broken path block.
5-5: Tighten phrasing.“exact same credentials” → “the same credentials”.
pkg/auth/providers/mock/identity_test.go (2)
227-245: Prefer errors.Is for sentinel error over string contains.Use the mock’s ErrNoStoredCredentials to avoid brittle string matching.
- creds, err = identity.LoadCredentials(ctx) - require.Error(t, err) - require.Nil(t, creds) - assert.Contains(t, err.Error(), "no stored credentials") + creds, err = identity.LoadCredentials(ctx) + require.Error(t, err) + require.Nil(t, creds) + assert.ErrorIs(t, err, ErrNoStoredCredentials)
353-383: Use errors.Is in table test too.Align error assertions with the sentinel error.
- if tt.expectedError { - require.Error(t, err) - assert.Contains(t, err.Error(), "no stored credentials") - } else { + if tt.expectedError { + require.Error(t, err) + assert.ErrorIs(t, err, ErrNoStoredCredentials) + } else { require.NoError(t, err) }pkg/auth/manager.go (3)
477-498: Keep retrieval via the single path (avoid direct keyring call).This shortcut bypasses the new fallback and duplicates logic. Use getChainCredentials (which delegates to loadCredentialsWithFallback) for the target step too.
- if validFromIndex == len(m.chain)-1 { - last := m.chain[len(m.chain)-1] - if cachedCreds, err := m.credentialStore.Retrieve(last); err == nil { - log.Debug("Using cached credentials for target identity", logKeyIdentity, targetIdentity) - return cachedCreds, nil - } - } + if validFromIndex == len(m.chain)-1 { + // Use unified retrieval with fallback for the target step. + if cachedCreds, err := m.getChainCredentials(m.chain, validFromIndex); err == nil { + log.Debug("Using cached credentials for target identity", logKeyIdentity, targetIdentity) + return cachedCreds, nil + } + }
633-673: Optional: cache fallback-loaded credentials.After loading from identity storage, consider storing to the credential store (best‑effort) to speed subsequent calls.
loadedCreds, loadErr := identity.LoadCredentials(ctx) if loadErr != nil { return nil, fmt.Errorf("failed to load credentials from identity storage for %q: %w", identityName, loadErr) } if loadedCreds == nil { return nil, fmt.Errorf("%w: credentials loaded from storage are nil for identity %q", errUtils.ErrNoCredentialsFound, identityName) } + // Best-effort cache. + if err := m.credentialStore.Store(identityName, loadedCreds); err != nil { + log.Debug("Failed to cache credentials loaded from identity storage", logKeyIdentity, identityName, "error", err) + }
675-684: Thread context instead of context.Background().Propagate the caller’s ctx through getChainCredentials to enable cancellation/timeouts during fallback loads.
- func (m *manager) getChainCredentials(chain []string, startIndex int) (types.ICredentials, error) { + func (m *manager) getChainCredentials(ctx context.Context, chain []string, startIndex int) (types.ICredentials, error) { identityName := chain[startIndex] - creds, err := m.loadCredentialsWithFallback(context.Background(), identityName) + creds, err := m.loadCredentialsWithFallback(ctx, identityName)And update the single caller:
- currentCreds, actualStartIndex = m.fetchCachedCredentials(actualStartIndex) + currentCreds, actualStartIndex = m.fetchCachedCredentials(ctx, actualStartIndex)(And pass ctx through fetchCachedCredentials similarly.)
pkg/auth/manager_test.go (2)
389-397: Minor: stabilize time anchors.Use a single captured now to avoid edge flake if the clock jumps between assignments.
- expiredTime := time.Now().Add(-1 * time.Hour) + now := time.Now() + expiredTime := now.Add(-1 * time.Hour) ... - validTime := time.Now().Add(1 * time.Hour) + validTime := now.Add(1 * time.Hour)
533-582: Great regression coverage; consider asserting Authenticate wasn’t invoked.You already make Authenticate return an error; add a flag to assert it wasn’t called.
type mockIdentityForFallback struct { hasCredentials bool creds types.ICredentials + calledAuth *bool } ... func (m *mockIdentityForFallback) Authenticate(ctx context.Context, previousCredentials types.ICredentials) (types.ICredentials, error) { - return nil, fmt.Errorf("authenticate should not be called in retrieval path") + if m.calledAuth != nil { *m.calledAuth = true } + return nil, fmt.Errorf("authenticate should not be called in retrieval path") } ... - creds, err := m.getChainCredentials(m.chain, 1) + called := false + identity.calledAuth = &called + creds, err := m.getChainCredentials(m.chain, 1) ... require.NoError(t, err, ...) assert.NotNil(t, creds, ...) + assert.False(t, called, "Authenticate should not be called for fallback retrieval")pkg/auth/providers/mock/identity.go (2)
154-156: Extract fixedExpiration to a helper function.The
fixedExpirationcreation is duplicated here and inAuthenticate()(line 63). Consider extracting to a helper function for DRY.Add a helper function in the file:
// getFixedExpiration returns a deterministic expiration timestamp for testing. func getFixedExpiration() time.Time { return time.Date(MockExpirationYear, MockExpirationMonth, MockExpirationDay, MockExpirationHour, MockExpirationMinute, MockExpirationSecond, 0, time.UTC) }Then update both
Authenticate()andLoadCredentials():- fixedExpiration := time.Date(MockExpirationYear, MockExpirationMonth, MockExpirationDay, MockExpirationHour, MockExpirationMinute, MockExpirationSecond, 0, time.UTC) + fixedExpiration := getFixedExpiration()
160-166: Consider using consistent credential values.
LoadCredentials()returns generic credentials ("mock-access-key"), whileAuthenticate()(line 65) returns identity-specific credentials (fmt.Sprintf("MOCK_KEY_%s", i.name)). Using identity-specific values here would improve consistency and make debugging easier.Apply this diff for consistency:
return &Credentials{ - AccessKeyID: "mock-access-key", - SecretAccessKey: "mock-secret-key", - SessionToken: "mock-session-token", + AccessKeyID: fmt.Sprintf("MOCK_KEY_%s", i.name), + SecretAccessKey: fmt.Sprintf("MOCK_SECRET_%s", i.name), + SessionToken: fmt.Sprintf("MOCK_TOKEN_%s", i.name), Region: "us-east-1", Expiration: fixedExpiration, }, 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 (7)
docs/prd/credential-retrieval-consolidation.md(1 hunks)pkg/auth/manager.go(8 hunks)pkg/auth/manager_test.go(4 hunks)pkg/auth/providers/mock/identity.go(5 hunks)pkg/auth/providers/mock/identity_test.go(3 hunks)tests/cli_test.go(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_whoami_without_authentication.stderr.golden(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/snapshots/TestCLICommands_atmos_auth_whoami_without_authentication.stderr.golden
🧰 Additional context used
📓 Path-based instructions (6)
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + DI; table-driven tests for coverage; target >80% coverage.
Tests must exercise production code paths; avoid duplicating implementation logic in tests.
Use t.Skipf("reason") with clear context when skipping tests; CLI tests may auto-build temp binaries.
Files:
tests/cli_test.gopkg/auth/providers/mock/identity_test.gopkg/auth/manager_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: Define interfaces for major functionality and use dependency injection to enable testability.
Generate mocks using go.uber.org/mock/mockgen via //go:generate directives (never manual mocks).
Prefer functional Options pattern instead of functions with many parameters for configuration.
Use context.Context only for cancellation, deadlines/timeouts, and request-scoped values; never for configuration or dependencies; context should be the first parameter.
All comments must end with periods (godot linter).
Preserve helpful existing comments; update them to match code when refactoring; only remove when incorrect, redundant, or code removed.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() (or nil if no atmosConfig) plus a blank line at the start of all public functions for performance tracking.
Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.
Never use //revive:disable:file-length-limit to bypass file size limits; keep files small and focused (<600 lines).
Environment variables must be bound with viper.BindEnv using ATMOS_ prefix (e.g., viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")).
Ensure cross-platform compatibility: use filepath.Join instead of hardcoded path separators and prefer SDKs over shelling out to binaries.
Files:
tests/cli_test.gopkg/auth/providers/mock/identity_test.gopkg/auth/manager_test.gopkg/auth/manager.gopkg/auth/providers/mock/identity.go
tests/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*_test.go: Integration tests, when necessary, must live under tests/.
Use test precondition helpers from tests/test_preconditions.go so tests skip gracefully when prerequisites are missing.
Files:
tests/cli_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/providers/mock/identity_test.gopkg/auth/manager_test.gopkg/auth/manager.gopkg/auth/providers/mock/identity.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/auth/manager.gopkg/auth/providers/mock/identity.go
docs/prd/[a-z0-9-]*.md
📄 CodeRabbit inference engine (CLAUDE.md)
All PRDs must be placed under docs/prd/ with kebab-case filenames.
Files:
docs/prd/credential-retrieval-consolidation.md
🧬 Code graph analysis (4)
pkg/auth/providers/mock/identity_test.go (2)
pkg/auth/types/interfaces.go (1)
PostAuthenticateParams(61-67)pkg/auth/providers/mock/credentials.go (1)
Credentials(11-17)
pkg/auth/manager_test.go (3)
pkg/auth/credentials/keyring_file.go (1)
ErrCredentialsNotFound(36-36)pkg/schema/schema_auth.go (2)
AuthConfig(4-9)Identity(45-53)pkg/auth/types/interfaces.go (3)
Identity(70-114)ICredentials(230-241)PostAuthenticateParams(61-67)
pkg/auth/manager.go (4)
pkg/logger/log.go (1)
Debug(24-26)pkg/auth/types/interfaces.go (1)
ICredentials(230-241)pkg/auth/credentials/keyring_file.go (1)
ErrCredentialsNotFound(36-36)errors/errors.go (2)
ErrIdentityNotInConfig(428-428)ErrNoCredentialsFound(388-388)
pkg/auth/providers/mock/identity.go (4)
pkg/auth/types/interfaces.go (3)
Identity(70-114)PostAuthenticateParams(61-67)ICredentials(230-241)pkg/schema/schema_auth.go (1)
Identity(45-53)pkg/auth/providers/mock/provider.go (6)
MockExpirationYear(15-15)MockExpirationMonth(17-17)MockExpirationDay(19-19)MockExpirationHour(21-21)MockExpirationMinute(23-23)MockExpirationSecond(25-25)pkg/auth/providers/mock/credentials.go (1)
Credentials(11-17)
🪛 LanguageTool
docs/prd/credential-retrieval-consolidation.md
[style] ~5-~5: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...(e.g., atmos terraform plan) with the exact same credentials. ## Current Architecture A...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[typographical] ~82-~82: Consider using a typographic opening quote here.
Context: ...tion** without a shared abstraction for "retrieve credential from any available s...
(EN_QUOTES)
[typographical] ~82-~82: Consider using a typographic close quote here.
Context: ...ve credential from any available source." ## Why This Manifested in Terraform Co...
(EN_QUOTES)
[typographical] ~114-~114: Consider using typographic quotation marks here.
Context: ...ion** Three separate implementations of "retrieve credential" logic with inconsistent behavior. ### ...
(EN_QUOTES)
[typographical] ~117-~117: Consider using a typographic opening quote here.
Context: ...ngle method/interface that encapsulates "retrieve credential from any available s...
(EN_QUOTES)
[typographical] ~117-~117: Consider using a typographic close quote here.
Context: ...y available source with proper fallback." ### 3. Implicit Assumptions `retri...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic opening quote here.
Context: ...s, headless systems, no D-Bus) - We use "noop" keyring in these environments ###...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic close quote here.
Context: ...adless systems, no D-Bus) - We use "noop" keyring in these environments ### 4. *...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/credential-retrieval-consolidation.md
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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: Run pre-commit hooks
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: autofix
- GitHub Check: Review Dependency Licenses
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (11)
pkg/auth/manager.go (3)
202-221: Good consolidation and error context.Keyring→identity-storage fallback centralization here reads well; error includes identity/provider/store for triage. LGTM.
219-221: Expiry check is correct and minimal.Directly defers to credentials’ IsExpired; matches interface intent.
436-438: Default files path fallback is reasonable.Using ~/.config/atmos as XDG-aligned fallback is consistent with tests and docs.
pkg/auth/manager_test.go (3)
14-14: Import addition is appropriate.Brings in credentials.ErrCredentialsNotFound for precise simulation.
523-531: Updated call to getChainCredentials looks good.Matches the new internal API and exercises the fallback.
1247-1248: Updated default path expectation matches code.Test reflects ~/.config/atmos fallback.
pkg/auth/providers/mock/identity.go (5)
14-16: Nice work addressing the past review.The static sentinel error follows Go idioms and the coding guidelines. The documentation is clear.
19-25: Clean state tracking addition.The
hasStoredCredentialsfield and documentation clearly explain the storage simulation behavior.
95-96: Good environment setup for AWS SDK testing.The region variables align with the existing mock configuration.
112-124: Correct simulation of credential persistence.Setting the flag after authentication properly mimics real provider behavior.
169-179: Proper cleanup of stored state.Clearing the flag simulates credential removal and requires re-authentication as expected.
…edential storage
The mock identity now persists credentials to a temporary file instead of using
in-memory state. This fixes the issue where terraform commands couldn't find
credentials because each process invocation created a new mock identity instance
with hasStoredCredentials=false.
Changes:
- PostAuthenticate() now writes credentials to a temp file (simulates XDG directory)
- LoadCredentials() reads from the temp file (simulates loading from ~/.config/atmos/)
- Logout() deletes the temp file
- Removed hasStoredCredentials in-memory flag
- Added getCredentialsFilePath() helper method
- Added constants MockRegion and MockFilePermissions (linter compliance)
- Updated tests to use unique identity names and cleanup with t.Cleanup()
This makes the mock identity behave like real providers (AWS, GitHub, Azure) where
credentials persist to disk after authentication and can be loaded in subsequent
process invocations.
Real-world flow:
1. atmos auth login → PostAuthenticate writes /tmp/atmos-mock-{name}.json
2. atmos terraform plan → LoadCredentials reads /tmp/atmos-mock-{name}.json
3. atmos auth logout → Logout deletes /tmp/atmos-mock-{name}.json
🤖 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/auth/providers/mock/identity.go (2)
120-129: Honor “return a NEW map” contract.PrepareEnvironment must return a new map, not the input reference.
Apply:
func (i *Identity) PrepareEnvironment(_ context.Context, environ map[string]string) (map[string]string, error) { defer perf.Track(nil, "mock.Identity.PrepareEnvironment")() - // Mock identities don't need to modify environment for external processes. - // Just return the environment unchanged. - return environ, nil + // Return a shallow copy to honor the interface contract. + out := make(map[string]string, len(environ)) + for k, v := range environ { + out[k] = v + } + return out, nil }As per coding guidelines.
165-172: Report existence based on storage, not unconditionally true.Aligns with the interface semantics and avoids false positives (e.g., whoami with noop keyring).
Apply:
func (i *Identity) CredentialsExist() (bool, error) { defer perf.Track(nil, "mock.Identity.CredentialsExist")() - // Mock identities don't use file-based storage. - // Credentials are always available in-memory. - return true, nil + _, err := os.Stat(i.getCredentialsFilePath()) + if err == nil { + return true, nil + } + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("stat creds: %w", err) }As per coding guidelines.
🧹 Nitpick comments (4)
pkg/auth/providers/mock/identity_test.go (1)
378-388: Make expiration assertion deterministic.Since mocks use a fixed 2099-12-31T23:59:59Z, assert exact equality to avoid future flakiness.
Apply:
- assert.False(t, mockCreds.Expiration.IsZero()) - assert.True(t, mockCreds.Expiration.After(time.Now())) + expected := time.Date( + MockExpirationYear, MockExpirationMonth, MockExpirationDay, + MockExpirationHour, MockExpirationMinute, MockExpirationSecond, + 0, time.UTC, + ) + assert.Equal(t, expected, mockCreds.Expiration)pkg/auth/providers/mock/identity.go (3)
48-56: Sanitize filename to avoid path components in identity names.Defensive coding: ensure identity names with slashes don’t escape the temp dir.
Apply:
func (i *Identity) getCredentialsFilePath() string { - // Use a temp directory that's cleaned up by the OS. - // In production, real providers would use XDG directories like ~/.config/atmos/aws/{provider}/. - tmpDir := os.TempDir() - return filepath.Join(tmpDir, "atmos-mock-"+i.name+".json") + tmpDir := os.TempDir() + safe := filepath.Base(i.name) + safe = strings.ReplaceAll(safe, string(os.PathSeparator), "_") + return filepath.Join(tmpDir, "atmos-mock-"+safe+".json") }And import:
+ "strings"As per coding guidelines.
131-163: Write atomically and prefer provided credentials when available.Avoid partial writes; use temp+rename. If params.Credentials is set, persist those for better end‑to‑end tests.
Apply:
func (i *Identity) PostAuthenticate(ctx context.Context, params *types.PostAuthenticateParams) error { defer perf.Track(nil, "mock.Identity.PostAuthenticate")() - // Write credentials to disk to simulate persistent storage. - // Use a fixed timestamp for deterministic testing. - fixedExpiration := time.Date(MockExpirationYear, MockExpirationMonth, MockExpirationDay, MockExpirationHour, MockExpirationMinute, MockExpirationSecond, 0, time.UTC) - - creds := &Credentials{ - AccessKeyID: "mock-access-key", - SecretAccessKey: "mock-secret-key", - SessionToken: "mock-session-token", - Region: MockRegion, - Expiration: fixedExpiration, - } + fixedExpiration := time.Date(MockExpirationYear, MockExpirationMonth, MockExpirationDay, MockExpirationHour, MockExpirationMinute, MockExpirationSecond, 0, time.UTC) + var creds *Credentials + if mc, ok := params.Credentials.(*Credentials); ok && mc != nil { + // Persist provided mock credentials, normalizing expiration/region for determinism. + cp := *mc + if cp.Expiration.IsZero() { + cp.Expiration = fixedExpiration + } + if cp.Region == "" { + cp.Region = MockRegion + } + creds = &cp + } else { + // Default deterministic creds. + creds = &Credentials{ + AccessKeyID: "mock-access-key", + SecretAccessKey: "mock-secret-key", + SessionToken: "mock-session-token", + Region: MockRegion, + Expiration: fixedExpiration, + } + } // Serialize credentials to JSON. data, err := json.Marshal(creds) if err != nil { return fmt.Errorf("failed to marshal mock credentials: %w", err) } - // Write to temp file (simulates writing to XDG directory). - credPath := i.getCredentialsFilePath() - if err := os.WriteFile(credPath, data, MockFilePermissions); err != nil { - return fmt.Errorf("failed to write mock credentials to %s: %w", credPath, err) - } + credPath := i.getCredentialsFilePath() + dir := filepath.Dir(credPath) + tmp, err := os.CreateTemp(dir, "creds-*.tmp") + if err != nil { + return fmt.Errorf("create temp file: %w", err) + } + tmpName := tmp.Name() + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return fmt.Errorf("write temp creds: %w", err) + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpName) + return fmt.Errorf("close temp creds: %w", err) + } + if err := os.Chmod(tmpName, MockFilePermissions); err != nil { + _ = os.Remove(tmpName) + return fmt.Errorf("chmod temp creds: %w", err) + } + if err := os.Rename(tmpName, credPath); err != nil { + _ = os.Remove(tmpName) + return fmt.Errorf("rename temp creds to %s: %w", credPath, err) + } return nil }As per coding guidelines.
17-23: Minor: permissions on Windows are advisory.0600 won’t be enforced on Windows; fine for tests, but note if you ever assert perms.
Add a comment:
// MockFilePermissions are the file permissions for credential files (owner read/write only). - MockFilePermissions = 0o600 + MockFilePermissions = 0o600 // On Windows, permissions are best-effort.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/auth/providers/mock/identity.go(6 hunks)pkg/auth/providers/mock/identity_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/providers/mock/identity.gopkg/auth/providers/mock/identity_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: Define interfaces for major functionality and use dependency injection to enable testability.
Generate mocks using go.uber.org/mock/mockgen via //go:generate directives (never manual mocks).
Prefer functional Options pattern instead of functions with many parameters for configuration.
Use context.Context only for cancellation, deadlines/timeouts, and request-scoped values; never for configuration or dependencies; context should be the first parameter.
All comments must end with periods (godot linter).
Preserve helpful existing comments; update them to match code when refactoring; only remove when incorrect, redundant, or code removed.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() (or nil if no atmosConfig) plus a blank line at the start of all public functions for performance tracking.
Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.
Never use //revive:disable:file-length-limit to bypass file size limits; keep files small and focused (<600 lines).
Environment variables must be bound with viper.BindEnv using ATMOS_ prefix (e.g., viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")).
Ensure cross-platform compatibility: use filepath.Join instead of hardcoded path separators and prefer SDKs over shelling out to binaries.
Files:
pkg/auth/providers/mock/identity.gopkg/auth/providers/mock/identity_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/auth/providers/mock/identity.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: Prefer unit tests with mocks over integration tests; use interfaces + DI; table-driven tests for coverage; target >80% coverage.
Tests must exercise production code paths; avoid duplicating implementation logic in tests.
Use t.Skipf("reason") with clear context when skipping tests; CLI tests may auto-build temp binaries.
Files:
pkg/auth/providers/mock/identity_test.go
🧬 Code graph analysis (2)
pkg/auth/providers/mock/identity.go (3)
pkg/auth/types/interfaces.go (3)
Identity(70-114)PostAuthenticateParams(61-67)ICredentials(230-241)pkg/auth/providers/mock/provider.go (6)
MockExpirationYear(15-15)MockExpirationMonth(17-17)MockExpirationDay(19-19)MockExpirationHour(21-21)MockExpirationMinute(23-23)MockExpirationSecond(25-25)pkg/auth/providers/mock/credentials.go (1)
Credentials(11-17)
pkg/auth/providers/mock/identity_test.go (3)
pkg/auth/types/interfaces.go (1)
PostAuthenticateParams(61-67)pkg/auth/providers/mock/identity.go (1)
NewIdentity(39-46)pkg/auth/providers/mock/credentials.go (1)
Credentials(11-17)
⏰ 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: Build (linux)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Review Dependency Licenses
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: autofix
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/auth/providers/mock/identity.go (1)
174-205: Nit: wrap unmarshal/read errors with paths (already done) — LGTM.Read, not-exist handling, and wrapping look good.
The logout command was using PrintfMarkdownToTUI with theme.Styles.Checkmark
which already contains ANSI color codes. When the markdown renderer processed
the string, it didn't strip the existing ANSI codes, causing them to leak
through to the output as raw escape sequences like [38;5;40m.
Changed to use PrintfMessageToTUI instead, which doesn't try to render markdown
and properly handles pre-styled content.
Affected messages:
- "Logged out {identity}"
- "Logged out {identity} with warnings"
- "Failed to log out {identity}"
- "Logged out provider {provider} (N identities)"
Before: [38;5;40m✓[0m Logged out core-identity/managers
After: ✓ Logged out core-identity/managers
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/auth_logout.go (1)
132-145: Inconsistent formatting for partial logout messages.The identity partial logout (line 135) now uses plain text, but provider (line 192) and logout-all (line 312) partial messages still use markdown formatting (
**bold**). Consider standardizing these to either all markdown or all plain text.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cmd/auth_logout.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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: New CLI commands must use the command registry pattern via CommandProvider and register through the command registry (see cmd/internal/registry.go).
Commands should embed *_usage.md via //go:embed and render with utils.PrintfMarkdown().
UI (prompts/status) must write to stderr; data outputs to stdout; logging only for system events (never for UI).
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd(); normal paths are auto-enabled via RootCmd.ExecuteC(). Never capture user data.
Files:
cmd/auth_logout.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: Define interfaces for major functionality and use dependency injection to enable testability.
Generate mocks using go.uber.org/mock/mockgen via //go:generate directives (never manual mocks).
Prefer functional Options pattern instead of functions with many parameters for configuration.
Use context.Context only for cancellation, deadlines/timeouts, and request-scoped values; never for configuration or dependencies; context should be the first parameter.
All comments must end with periods (godot linter).
Preserve helpful existing comments; update them to match code when refactoring; only remove when incorrect, redundant, or code removed.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() (or nil if no atmosConfig) plus a blank line at the start of all public functions for performance tracking.
Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.
Never use //revive:disable:file-length-limit to bypass file size limits; keep files small and focused (<600 lines).
Environment variables must be bound with viper.BindEnv using ATMOS_ prefix (e.g., viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")).
Ensure cross-platform compatibility: use filepath.Join instead of hardcoded path separators and prefer SDKs over shelling out to binaries.
Files:
cmd/auth_logout.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
cmd/auth_logout.go
{cmd,pkg/ui}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use theme colors from pkg/ui/theme/colors.go for UI elements.
Files:
cmd/auth_logout.go
🧬 Code graph analysis (1)
cmd/auth_logout.go (1)
pkg/utils/log_utils.go (1)
PrintfMessageToTUI(33-35)
⏰ 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 (linux)
- GitHub Check: Build (windows)
- GitHub Check: Analyze (go)
- GitHub Check: Run pre-commit hooks
- GitHub Check: autofix
- GitHub Check: Review Dependency Licenses
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (3)
cmd/auth_logout.go (3)
139-142: Looks good.Switching to plain text formatting is appropriate here, and the change is consistent with other failure messages in the file (lines 198, 318).
144-145: Looks good.Plain text formatting is appropriate for this success message, and it's consistent with similar messages at line 324.
203-203: Looks good.The switch to plain text formatting is appropriate and consistent with the identity logout success message.
…ommands
Added fallback to ATMOS_IDENTITY environment variable when the --identity flag
is not provided for terraform commands. This makes the behavior consistent with
auth commands which already support ATMOS_IDENTITY.
Previously, ATMOS_IDENTITY only worked with `atmos auth` commands because the
environment variable binding was only set up in cmd/auth.go. Terraform commands
parsed flags manually in internal/exec/cli_utils.go without checking environment
variables.
Now users can set ATMOS_IDENTITY once and have it work across all commands:
- atmos auth whoami # Uses ATMOS_IDENTITY ✅
- atmos terraform plan # Uses ATMOS_IDENTITY ✅ (NEW)
- atmos terraform apply # Uses ATMOS_IDENTITY ✅ (NEW)
Changes:
- Added viper.BindEnv("atmos_identity_temp", "ATMOS_IDENTITY") in ProcessCommandLineArgs()
- Import viper package in cli_utils.go
- Checks environment variable only if --identity flag not provided
- Uses viper instead of os.Getenv to comply with linter forbidigo rules
Priority: Flag > Environment Variable > Default Identity > Interactive Prompt
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Added comprehensive tests to verify that the ATMOS_IDENTITY environment variable is correctly used as a fallback when the --identity flag is not provided for terraform commands. Test coverage: - Environment variable used when flag not provided - Flag takes precedence over environment variable - Empty identity when neither flag nor env var provided - Works with both --identity and --identity=value syntax - Identity flag parsing in different positions These tests prevent future regressions of the ATMOS_IDENTITY environment variable support that was added in commit 06c9239. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Added regex sanitization to normalize credential_store backend names in error messages that vary by platform: - Mac/Windows: "system-keyring" - Linux CI: "noop" The keyring backend is selected at runtime based on platform availability. Linux CI environments lack system keyring access, so they fall back to the noop backend. This causes snapshot mismatches between platforms. Solution: Sanitize all credential_store values to "<KEYRING_BACKEND>" placeholder, similar to how we sanitize other environment-specific values. Changes: - Added step 13 in sanitizeOutput() to normalize credential_store values - Regex matches "system-keyring", "noop", and "file" backends - Regenerated snapshot with platform-agnostic placeholder - Test now passes consistently across Mac, Windows, and Linux 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cli_test.go (1)
402-407: Hardcoded "(mock)" pattern may miss other auth providers.The regex only normalizes durations for the mock provider. If auth list includes AWS, Azure, or other providers, their expiration durations won't be normalized, potentially causing snapshot mismatches.
Consider a more general pattern:
- // 12. Normalize credential expiration durations in auth list output. - // These appear as "● mock-identity (mock) [DEFAULT] 650202h14m" in tree output. + // 12. Normalize credential expiration durations in auth list output. + // These appear as "● identity-name (provider) [DEFAULT] 650202h14m" in tree output. // The duration changes every minute, so normalize to "1h 0m" like other duration normalizations. // Matches patterns like "650202h14m", "1h30m", "45m", etc. at the end of identity lines. - expirationDurationRegex := regexp.MustCompile(`(\(mock\)(?:\s+\[DEFAULT\])?)\s+\d+h\d+m\b`) + expirationDurationRegex := regexp.MustCompile(`(\([a-z-]+\)(?:\s+\[DEFAULT\])?)\s+\d+h\d+m\b`) result = expirationDurationRegex.ReplaceAllString(result, "$1 1h 0m")Alternatively, if mock-only tests are intentional, update the comment to clarify this limitation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
tests/cli_test.go(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_list.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_whoami_--identity_nonexistent.stderr.golden(1 hunks)tests/test-cases/auth-cli.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/snapshots/TestCLICommands_atmos_auth_whoami_--identity_nonexistent.stderr.golden
- tests/snapshots/TestCLICommands_atmos_auth_list.stdout.golden
🧰 Additional context used
📓 Path-based instructions (3)
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + DI; table-driven tests for coverage; target >80% coverage.
Tests must exercise production code paths; avoid duplicating implementation logic in tests.
Use t.Skipf("reason") with clear context when skipping tests; CLI tests may auto-build temp binaries.
Files:
tests/cli_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: Define interfaces for major functionality and use dependency injection to enable testability.
Generate mocks using go.uber.org/mock/mockgen via //go:generate directives (never manual mocks).
Prefer functional Options pattern instead of functions with many parameters for configuration.
Use context.Context only for cancellation, deadlines/timeouts, and request-scoped values; never for configuration or dependencies; context should be the first parameter.
All comments must end with periods (godot linter).
Preserve helpful existing comments; update them to match code when refactoring; only remove when incorrect, redundant, or code removed.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() (or nil if no atmosConfig) plus a blank line at the start of all public functions for performance tracking.
Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.
Never use //revive:disable:file-length-limit to bypass file size limits; keep files small and focused (<600 lines).
Environment variables must be bound with viper.BindEnv using ATMOS_ prefix (e.g., viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")).
Ensure cross-platform compatibility: use filepath.Join instead of hardcoded path separators and prefer SDKs over shelling out to binaries.
Files:
tests/cli_test.go
tests/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*_test.go: Integration tests, when necessary, must live under tests/.
Use test precondition helpers from tests/test_preconditions.go so tests skip gracefully when prerequisites are missing.
Files:
tests/cli_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (macos)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: website-deploy-preview
- GitHub Check: autofix
- GitHub Check: Review Dependency Licenses
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
🔇 Additional comments (3)
tests/test-cases/auth-cli.yaml (1)
300-304: Clarify fallback logic and verify error message alignment.The comment flags this behavior as needing investigation, which is concerning for a consolidated fix. Two clarifications needed:
- Is the fallback to default identity when a nonexistent identity is requested the intended behavior introduced by the consolidation, or a side effect?
- Does the error message "no credentials found" accurately describe what happened? When identity resolution falls back, should the error be more explicit?
Verify the implementation of
retrieveCredentialWithFallback()andGetDefaultIdentity()to confirm this behavior is intentional and properly tested.tests/cli_test.go (2)
396-400: LGTM! Timestamp normalization stabilizes auth whoami snapshots.The regex correctly matches timestamp patterns and the fixed replacement ensures consistent snapshot comparisons across test runs.
409-413: LGTM! Credential store normalization handles platform differences correctly.The regex matches known keyring backends and the placeholder ensures snapshot consistency across different platforms.
Changed credential_store sanitization from '<KEYRING_BACKEND>' to 'keyring' for better readability and consistency. The angle-bracket syntax looks like HTML/template code and doesn't represent any real value. Using the keyword 'keyring' is simpler and more accurate since all backends (system-keyring, noop, file) are keyring implementations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Changed from 'keyring' to 'keyring-placeholder' to make it clear the value has been sanitized while still resembling actual keyring backend names (system-keyring, noop, file). Benefits: - Clearly indicates sanitization (the "-placeholder" suffix) - Resembles actual values without being confused for a real backend - Avoids template-like syntax (angle brackets) - More explicit than generic "keyring" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fixed linting issue where inline comment at line 100 was missing a terminating period. All comment sentences must end with periods per godot linter rules. Changed: - "Don't fail config loading if this step fails, just log it" + "Don't fail config loading if this step fails, just log it." 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…tity selection
- Add TTY check using term.IsTTYSupportForStdin() to fail fast in CI/non-TTY environments
- Change InitCliConfig parameter from true to false to skip unnecessary stack processing
- Align error wrapping with coding guidelines using fmt.Errorf("%w: %w") pattern
- Improves error handling and prevents hanging in headless environments
- Add t.Parallel() to case sensitivity test for faster execution - Add CLI docs link to blog post for better discoverability - Clarify flag vs env var precedence (--identity overrides ATMOS_IDENTITY) - Add language markers (text) to PRD code blocks for proper markdown linting
The regex was requiring both hours and minutes (\d+h\d+m), but Go's duration formatting omits minutes when they are zero (650194h instead of 650194h0m). Updated regex to make minutes optional with (?:\d+m)? to handle both formats consistently.
Changed from /cli/commands/auth/atmos_auth (non-existent) to /cli/commands/auth/usage which is the correct URL for the main auth documentation page.
Add mandatory guidelines for verifying documentation URLs before adding links: - How to find correct URLs using find and grep - Common mistakes (using command names vs file names) - Importance of checking slug frontmatter - Step-by-step verification approach Prevents broken links like /cli/commands/auth/atmos_auth (non-existent) when the correct URL is /cli/commands/auth/usage.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/prd/credential-retrieval-consolidation.md (2)
282-282: Heading formatting (MD036).Line 282 uses emphasis (
**Recommended Approach**) where a proper markdown heading (## Recommended Approach) would be more idiomatic.-**Recommended Approach** +## Recommended Approach
47-47: Minor wording: "exact same" is redundant.Consider simplifying for conciseness:
-...failed (e.g., `atmos terraform plan`) with the exact same credentials. +...failed (e.g., `atmos terraform plan`) with identical credentials.
📜 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 (5)
cmd/terraform_utils.go(2 hunks)docs/prd/credential-retrieval-consolidation.md(1 hunks)pkg/auth/manager_case_test.go(1 hunks)tests/cli_test.go(1 hunks)website/blog/2025-10-29-auth-ux-improvements.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/auth/manager_case_test.go
- website/blog/2025-10-29-auth-ux-improvements.mdx
🧰 Additional context used
📓 Path-based instructions (7)
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Prefer unit tests with mocks over integration tests; use interfaces + DI; table-driven tests for coverage; target >80% coverage.
Tests must exercise production code paths; avoid duplicating implementation logic in tests.
Use t.Skipf("reason") with clear context when skipping tests; CLI tests may auto-build temp binaries.
Files:
tests/cli_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: Define interfaces for major functionality and use dependency injection to enable testability.
Generate mocks using go.uber.org/mock/mockgen via //go:generate directives (never manual mocks).
Prefer functional Options pattern instead of functions with many parameters for configuration.
Use context.Context only for cancellation, deadlines/timeouts, and request-scoped values; never for configuration or dependencies; context should be the first parameter.
All comments must end with periods (godot linter).
Preserve helpful existing comments; update them to match code when refactoring; only remove when incorrect, redundant, or code removed.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically; maintain aliases cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() (or nil if no atmosConfig) plus a blank line at the start of all public functions for performance tracking.
Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.
Never use //revive:disable:file-length-limit to bypass file size limits; keep files small and focused (<600 lines).
Environment variables must be bound with viper.BindEnv using ATMOS_ prefix (e.g., viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")).
Ensure cross-platform compatibility: use filepath.Join instead of hardcoded path separators and prefer SDKs over shelling out to binaries.
Files:
tests/cli_test.gocmd/terraform_utils.go
tests/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*_test.go: Integration tests, when necessary, must live under tests/.
Use test precondition helpers from tests/test_preconditions.go so tests skip gracefully when prerequisites are missing.
Files:
tests/cli_test.go
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
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: New CLI commands must use the command registry pattern via CommandProvider and register through the command registry (see cmd/internal/registry.go).
Commands should embed *_usage.md via //go:embed and render with utils.PrintfMarkdown().
UI (prompts/status) must write to stderr; data outputs to stdout; logging only for system events (never for UI).
For non-standard execution paths, capture telemetry with telemetry.CaptureCmd(); normal paths are auto-enabled via RootCmd.ExecuteC(). Never capture user data.
Files:
cmd/terraform_utils.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
cmd/terraform_utils.go
{cmd,pkg/ui}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use theme colors from pkg/ui/theme/colors.go for UI elements.
Files:
cmd/terraform_utils.go
docs/prd/[a-z0-9-]*.md
📄 CodeRabbit inference engine (CLAUDE.md)
All PRDs must be placed under docs/prd/ with kebab-case filenames.
Files:
docs/prd/credential-retrieval-consolidation.md
🧠 Learnings (2)
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
cmd/terraform_utils.go
📚 Learning: 2025-09-07T18:07:00.549Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Applied to files:
cmd/terraform_utils.go
🧬 Code graph analysis (1)
cmd/terraform_utils.go (5)
cmd/auth.go (1)
IdentityFlagSelectValue(12-12)internal/tui/templates/term/term_writer.go (1)
IsTTYSupportForStdin(133-135)errors/error_funcs.go (1)
CheckErrorPrintAndExit(75-98)errors/errors.go (3)
ErrDefaultIdentity(382-382)ErrInitializeCLIConfig(341-341)ErrFailedToInitializeAuthManager(391-391)pkg/config/config.go (1)
InitCliConfig(25-62)
🪛 LanguageTool
docs/prd/credential-retrieval-consolidation.md
[typographical] ~21-~21: Consider using typographic quotation marks here.
Context: ...dling:** Properly propagates errors vs. "not found" conditions ### Refactored Call Sites ...
(EN_QUOTES)
[style] ~47-~47: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...(e.g., atmos terraform plan) with the exact same credentials. ## Original Architecture ...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[typographical] ~124-~124: Consider using a typographic opening quote here.
Context: ...tion** without a shared abstraction for "retrieve credential from any available s...
(EN_QUOTES)
[typographical] ~124-~124: Consider using a typographic close quote here.
Context: ...ve credential from any available source." ## Why This Manifested in Terraform Co...
(EN_QUOTES)
[typographical] ~156-~156: Consider using typographic quotation marks here.
Context: ...ion** Three separate implementations of "retrieve credential" logic with inconsistent behavior. ### ...
(EN_QUOTES)
[typographical] ~159-~159: Consider using a typographic opening quote here.
Context: ...ngle method/interface that encapsulates "retrieve credential from any available s...
(EN_QUOTES)
[typographical] ~159-~159: Consider using a typographic close quote here.
Context: ...y available source with proper fallback." ### 3. Implicit Assumptions `retri...
(EN_QUOTES)
[typographical] ~166-~166: Consider using a typographic opening quote here.
Context: ...s, headless systems, no D-Bus) - We use "noop" keyring in these environments ###...
(EN_QUOTES)
[typographical] ~166-~166: Consider using a typographic close quote here.
Context: ...adless systems, no D-Bus) - We use "noop" keyring in these environments ### 4. *...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/credential-retrieval-consolidation.md
282-282: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (10)
- GitHub Check: Review Dependency Licenses
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
- GitHub Check: Run pre-commit hooks
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: autofix
- GitHub Check: Summary
🔇 Additional comments (5)
docs/prd/credential-retrieval-consolidation.md (1)
1-454: Comprehensive and well-structured PRD. Content is accurate and informative.The document effectively explains the problem, analyzes root causes, documents the implemented solution, and outlines testing strategy. The implementation summary correctly describes
loadCredentialsWithFallbackand the three refactored call sites. Excellent historical context and architectural rationale.tests/cli_test.go (1)
396-413: Solid snapshot normalizations.These three additions normalize time-sensitive and platform-specific values to keep snapshot tests stable:
- Last Updated timestamps (lines 396-400)
- Mock credential expiration durations (lines 402-407)
- Platform-specific credential store backends (lines 409-413)
Each follows the existing normalization pattern in this function, with clear comments explaining the purpose. The duration normalization is intentionally scoped to mock identities, which makes sense for test stability without over-normalizing real output.
cmd/terraform_utils.go (3)
11-15: Imports look tidy and on‑spec.Atmos imports are grouped and alphabetized; aliases (errUtils, cfg, log) follow house style. All good.
112-116: Identity precedence preserved — nice.Only override when flag is non‑empty; preserves ATMOS_IDENTITY from env. LGTM.
96-97: Bug: fmt.Errorf has multiple %w (invalid) — will panic at runtime.Replace double‑%w formatting with errors.Join (or a single wrap). This preserves sentinel checks via errors.Is and aligns with earlier usage in this file. As per coding guidelines.
Apply:
- errUtils.CheckErrorPrintAndExit(fmt.Errorf("%w: %w", errUtils.ErrInitializeCLIConfig, err), "", "") + errUtils.CheckErrorPrintAndExit(errors.Join(errUtils.ErrInitializeCLIConfig, err), "", "") - errUtils.CheckErrorPrintAndExit(fmt.Errorf("%w: %w", errUtils.ErrFailedToInitializeAuthManager, err), "", "") + errUtils.CheckErrorPrintAndExit(errors.Join(errUtils.ErrFailedToInitializeAuthManager, err), "", "") - errUtils.CheckErrorPrintAndExit(fmt.Errorf("%w: %w", errUtils.ErrDefaultIdentity, err), "", "") + errUtils.CheckErrorPrintAndExit(errors.Join(errUtils.ErrDefaultIdentity, err), "", "")Also applies to: 102-103, 108-109
⛔ Skipped due to learnings
Learnt from: CR PR: cloudposse/atmos#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-24T14:51:19.355Z Learning: Applies to **/*.go : Use static errors from errors/errors.go; wrap with fmt.Errorf("%w") for chaining; use errors.Join for independent errors; avoid dynamic errors and string comparison; use errors.Is for checks.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.Learnt from: osterman PR: cloudposse/atmos#1695 File: pkg/auth/manager.go:169-171 Timestamp: 2025-10-22T14:55:44.014Z Learning: Go 1.20+ supports multiple %w verbs in fmt.Errorf, which returns an error implementing Unwrap() []error. This is valid and does not panic. Atmos uses Go 1.24.8 and configures errorlint with errorf-multi: true to validate this pattern.Learnt from: Benbentwo PR: cloudposse/atmos#1475 File: pkg/auth/identities/aws/user.go:141-145 Timestamp: 2025-09-10T22:38:42.212Z Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.Learnt from: CR PR: cloudposse/atmos#0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-09-23T02:30:42.362Z Learning: Applies to **/*.go : Wrap errors with context using fmt.Errorf("context: %w", err)
Changed 'retrieveCredentialWithFallback' to 'loadCredentialsWithFallback' in two locations (lines 244, 284) to match the actual implementation and other references throughout the document (lines 5, 15, 178, 181). The implemented function is loadCredentialsWithFallback, which better describes the behavior (load = fetch + process) compared to retrieve (just fetch).
… selection The interactive selector (huh.NewForm) requires: - stdin TTY for user input - stdout TTY for TUI rendering Updated condition to check both term.IsTTYSupportForStdin() and term.IsTTYSupportForStdout() to prevent hanging in environments where only one is available (e.g., piped stdout).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
website/blog/2025-10-29-auth-ux-improvements.mdx (3)
30-48: Document case-insensitive matching as potential breaking change.Case-insensitive identity name matching is a semantic change that could affect existing workflows or scripts that relied on case-sensitive matching. Consider adding a note flagging this as a behavior change and any migration considerations for users with overlapping identity names that differ only in case.
Example addition:
+**Note**: This behavior changed in this version. If you have identity names that differ only in case (e.g., `dev-admin` and `Dev-Admin`), Atmos will now treat them as the same identity with the first match returned. Ensure your `atmos.yaml` uses consistent casing to avoid confusion.
50-76: Add clarity on status indicator scope and refresh.The post introduces status indicators (✓, ⚠, ✗) in
auth listshowing credential expiry, but doesn't explain:
- How "expiring soon" is defined (threshold)
- Whether status is real-time or cached
- When indicators update or if they require command re-run
These details help users understand the reliability of the status display.
Example addition after line 75:
+Status is refreshed each time you run `atmos auth list`. Credentials are considered "expiring soon" if they will expire within 15 minutes.
77-86: Clarify legacy path migration scope.The legacy path warning only applies to
~/.aws/atmos/path, but the blog doesn't explain which commands trigger this warning or whether it appears once per session globally or per identity. Also, the suggested migration command isatmos auth login, but it's not clear if this applies to all identities or requires explicit identity specification.Consider clarifying:
-If you're using the legacy `~/.aws/atmos/` credential path, Atmos will now show the migration warning only once per execution instead of repeatedly: +If you're using the legacy `~/.aws/atmos/` credential path (from older Atmos versions), the next time you run an `auth` or `terraform` command, you'll see a one-time migration warning per session:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CLAUDE.md(1 hunks)website/blog/2025-10-29-auth-ux-improvements.mdx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 cases
Files:
website/blog/2025-10-29-auth-ux-improvements.mdx
🧠 Learnings (4)
📚 Learning: 2025-10-24T14:51:19.355Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-24T14:51:19.355Z
Learning: Applies to website/docs/cli/commands/**/*.md : All commands/flags require Docusaurus docs under website/docs/cli/commands/.
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-24T14:51:19.355Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-24T14:51:19.355Z
Learning: All command additions must include Docusaurus docs and website build steps as part of the workflow (website build should succeed).
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-24T14:51:19.355Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-24T14:51:19.355Z
Learning: Applies to website/blog/[0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]-*.md : PRs labeled minor or major must include a blog post with filename date prefix and feature name; include <!--truncate--> and proper tags.
Applied to files:
CLAUDE.md
📚 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 website/** : Keep CLI and website documentation in sync; document new features with examples and use cases
Applied to files:
CLAUDE.md
⏰ 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). (10)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
- GitHub Check: Build (windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Review Dependency Licenses
- GitHub Check: website-deploy-preview
- GitHub Check: autofix
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (3)
CLAUDE.md (1)
343-350: CLAUDE.md blog post guidance overstates CI enforcement—remove.mdxrequirement or clarify it's aspirational, not enforced.CI only validates that a blog post file exists in
website/blog/matching.(md|mdx)$for PRs labeledminor/majortargeting main. It doesn't enforce.mdxextension specifically, YAML frontmatter structure,<!--truncate-->placement, or tag format. The existing repo has a mix of.mdand.mdxfiles, all passing CI. Either align guidance with actual CI checks or note these are style preferences, not CI-enforced requirements.Likely an incorrect or invalid review comment.
website/blog/2025-10-29-auth-ux-improvements.mdx (2)
13-28: Clarify whether--identitywithout a value is a documented feature or needs implementation.The blog post shows
atmos terraform plan mycomponent -s mystack --identity(without a value) triggering an interactive selector, but Atmos CLI documentation shows--identityas a value-taking flag. Either this is a new feature being added in this PR that needs code verification, or the example should specify a flag value. Confirm that interactive selection without a value is actually implemented in the underlying code changes, and verify the claim that this brings terraform commands "up to parity with other auth commands."
28-28: Documentation reference is accurate and current.The path
/cli/commands/auth/usageexists and the file comprehensively covers all features mentioned in the blog post: interactive identity selection, terraform integration, and authentication precedence rules via the--identityflag. The usage documentation explicitly documents interactive prompts when multiple or no default identities are configured, and includes terraform examples usingatmos auth exec -- terraform planand direct terraform commands.
…espace is set When ignore_trailing_whitespace is enabled for a test, snapshots are now generated without trailing whitespace. This prevents cross-platform snapshot failures caused by terminal width differences (lipgloss padding varies between Mac/Linux). Changes: - Update snapshot generation to strip trailing whitespace if ignore_trailing_whitespace is set - Apply fix to both TTY and non-TTY snapshot generation paths - Enable ignore_trailing_whitespace for 'atmos auth whoami --identity nonexistent' test - Regenerate snapshot with trailing whitespace removed This fixes CI failures where Mac generates snapshots with lipgloss padding but Linux environments produce output without that padding.
Replaced incorrect example using /cli/commands/auth/usage with a concrete, step-by-step example using auth-user-configure.mdx demonstrating: - How to find the doc file with find command - How to check slug in frontmatter with grep - How to verify the URL with existing links Shows expected output for each step and clarifies that MDX filenames (without extension) typically match the URL path unless overridden by slug.
|
These changes were released in v1.196.0. |
Summary
This PR fixes a critical bug where
atmos terraform planand other Terraform commands failed to use file-based credentials, whileatmos auth whoamiand similar commands worked correctly.The root cause was duplicate credential retrieval code across three methods with inconsistent fallback behavior. Two methods had keyring → identity storage fallback logic, but one (
retrieveCachedCredentials) did not, causing Terraform commands to fail when credentials were in files instead of the keyring.Root Cause Analysis
Three separate code paths retrieved credentials:
GetCachedCredentials- Had fallback ✓findFirstValidCachedCredentials- Had fallback ✓retrieveCachedCredentials- NO fallback ✗ (used by Terraform execution)When users authenticated via AWS SSO, credentials were written to files, not cached in the keyring. Terraform commands would fail because the
retrieveCachedCredentialspath didn't check identity storage.Solution
Extracted a shared
retrieveCredentialWithFallbackmethod as the single source of truth for credential retrieval:Changes
retrieveCredentialWithFallback()method (38 lines)GetCachedCredentials()- 40% code reductionfindFirstValidCachedCredentials()- 57% code reductionretrieveCachedCredentials()- Now uses shared methodTestManager_GetCachedCredentials_Pathsto use proper test dataTestManager_retrieveCachedCredentials_TerraformFlow_RegressionTestRetrieveCachedCredentials_KeyringMiss_IdentityStorageFallbackTesting
✅ All auth tests pass (12/12 test suites)
✅ Regression test reproduces original bug, passes with fix
✅ Integration tests verify fallback behavior works
✅ Code compiles successfully
Impact
✅ Terraform commands now work with file-based credentials
✅ ~110 lines of duplicate code eliminated
✅ Single source of truth for credential retrieval
✅ Impossible to have divergent fallback behavior in future
✅ Consistent behavior across all auth operations
References
This PR addresses the issue where valid authenticated sessions would fail during Terraform execution with "credentials not found" error, even though
atmos auth whoamishowed valid credentials.See
docs/prd/credential-retrieval-consolidation.mdfor detailed architectural analysis.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation