Repository navigation
fix: Atmos Auth stack-level default identity resolution - #2303
Merged
Merged
Conversation
…ssion #122) Fixes two related bugs in Atmos Auth identity resolution without breaking any existing auth functionality: - Issue #2293: auth.identities.<name>.default: true declared in an imported _defaults.yaml is now recognized across ALL command categories (previously only worked when the file was NOT listed in excluded_paths, and never worked for multi-stack commands like `describe stacks` / `list affected`). - Discussion #122: a default identity declared in one stack manifest no longer leaks to unrelated stacks when running terraform/helmfile/describe-component commands. The leak path (the global stack-file pre-scanner) is now structurally isolated from Category A commands that already have a stack-scoped merged auth config. Design: option (d+) — split entry points + import-following scanner. 1. Split pkg/auth entry points into NO-SCAN and SCAN variants. Category A callers (terraform/helmfile/describe component/nested auth) keep using CreateAndAuthenticateManagerWithAtmosConfig, which never consults stack files — it trusts the caller's pre-merged authConfig. Category B callers (describe stacks/affected/dependents, list affected/instances, aws security/compliance, workflows, MCP scoped auth) use the new CreateAndAuthenticateManagerWithStackScan, which runs the Approach 2 pre-scan on a COPY of authConfig before delegating. The scan variant cannot mutate the caller's atmosConfig.Auth, so Category B runs cannot contaminate Category A reuses of the same config. 2. Rewrite pkg/config/stack_auth_loader.go to recursively follow `import:` chains via a new loadAuthWithImports helper. Handles string imports, glob imports (doublestar), map-form imports, and relative (./ ../) paths. Resolves imports through files listed in `excluded_paths` — the exclude filter only prevents standalone processing, not import resolution. Cycle protection via visited-set. Templated imports and unreadable files are skipped gracefully. Issue #2072's allAgree conflict-detection is preserved unchanged. Previous auth fixes remain intact: - stack-level-default-auth-identity.md Approach 1 and Approach 2. - 2026-02-12-auth-realm-isolation-issues.md Issue #2072 (allAgree). - 2026-03-25-describe-affected-auth-identity-not-used.md AuthManager threading through the describe/list-affected pipeline. - 2026-04-06-mcp-server-env-not-applied-to-auth-setup.md — the MCP scoped auth entry point now routes through the scan variant so stack-level defaults are discovered after env overrides trigger a fresh cfg.InitCliConfig. Test coverage: - New tests in pkg/config/stack_auth_loader_test.go covering the import-following scanner (FollowsImports, FollowsImportsFromExcludedPath, ImportCycleProtection, GlobImports, TemplatedImportSkipped, CurrentFileWinsOverImport, ImportedDefaultAgreesAcrossStacks, RelativeImports, EmptyStacksBasePath, FallbackToYml, NonExistentCandidate, GlobNoMatches, MapFormWithPath, MapAnyAny variants, UnknownType, and primitive helpers). New helpers at 100% coverage. - New tests in pkg/auth/manager_helpers_test.go covering the scan/no-scan split, isolation guarantees for copyAuthConfigForScan (both directions), scanStackFilesForDefaults behavior, and Category A non-leak / Category B import-discovery regression paths. New entry points at 100%. - Two new scenario fixtures under tests/fixtures/scenarios/ using mock/aws identities so tests run end-to-end in CI without cloud credentials. - Four CLI regression test cases in tests/test-cases/auth-identity-resolution-bugs.yaml including a new `describe stacks` scenario that exercises the Category B scan variant end-to-end on the auth-imported-defaults fixture. Full regression suite passes: pkg/auth/, pkg/config/, internal/exec/, cmd/, pkg/list/, cmd/list/, pkg/mcp/..., and CLI scenario tests. See docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.md for the full design rationale, caller audit, and rejected alternatives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
Cosmetic only — IDE reformatted the markdown table column widths in docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.md for consistent column alignment. No content changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughSwitched several command flows to create auth managers via a new "stack-scan" entry point that pre-scans stack files (recursively following Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Cmd as cmd/identity_flag.go
participant Manager as pkg/auth/manager_helpers.go
participant Scanner as pkg/config/stack_auth_loader.go
participant FS as File System
participant Auth as Auth Manager
CLI->>Cmd: CreateAuthManagerFromIdentityWithStackScan(identityName, authConfig, atmosConfig)
alt identityName == "" and scan applicable
Cmd->>Manager: CreateAndAuthenticateManagerWithStackScan(...)
Manager->>Scanner: scanStackFilesForDefaults(authConfigCopy, atmosConfig)
Scanner->>FS: Load stack files + follow import: chains
FS-->>Scanner: file contents (import chains)
Scanner->>Manager: mergedAuthConfigCopy (original authConfig unchanged)
Manager->>Auth: CreateAndAuthenticateManagerWithAtmosConfig("", mergedAuthConfigCopy)
else explicit identity provided or scan skipped
Cmd->>Auth: CreateAndAuthenticateManagerWithAtmosConfig(identityName, authConfig)
end
Auth-->>CLI: AuthManager or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2303 +/- ##
==========================================
+ Coverage 77.12% 77.17% +0.05%
==========================================
Files 1069 1069
Lines 101179 101313 +134
==========================================
+ Hits 78034 78193 +159
+ Misses 18834 18808 -26
- Partials 4311 4312 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
Author
|
CodeRabbit (@coderabbitai) full review please |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.md`:
- Around line 577-580: Update the doc to say that the wrapper
CreateAuthManagerFromIdentityWithAtmosConfig remains the NO-SCAN path and that a
new function CreateAuthManagerFromIdentityWithStackScan was added for Category B
callers (e.g., callers that need stack-scan behavior); replace the incorrect
statement that the wrapper switched to the scan variant with a clear explanation
that Category B callers must call CreateAuthManagerFromIdentityWithStackScan and
that describe stacks/describe affected/describe dependents are updated via the
new scan wrapper, not by changing CreateAuthManagerFromIdentityWithAtmosConfig.
In `@pkg/auth/manager_helpers_test.go`:
- Around line 1335-1368: The test
TestCreateAndAuthenticateManagerWithStackScan_ExplicitIdentitySkipsScan
currently passes the sentinel "__DISABLED__" which causes early exit and doesn't
exercise the explicit-identity branch; update the call to
CreateAndAuthenticateManagerWithStackScan to pass the real non-empty identity
"explicit-identity" (matching the authConfig key) so the stack-scan
short-circuit path is exercised while still allowing downstream auth failures to
be ignored.
In `@pkg/config/stack_auth_loader.go`:
- Around line 31-38: The struct field Default in stackAuthFileWithImports (and
the other similar auth structs in this file) currently uses bool which cannot
distinguish “unset” from “explicitly false”; change those Default fields to
*bool (pointer to bool) so nil means unset and true/false are explicit, then
update applyCurrentFileAuthDefaults to treat nil as “no override” and only apply
overrides when Default != nil (using *Default to get the boolean value). Ensure
all places that construct or read these structs handle the pointer (nil checks
and dereferences) so current-file overrides of default:false correctly replace
imported true.
In `@pkg/list/list_affected.go`:
- Around line 91-96: The auth manager is unconditionally created via
auth.CreateAndAuthenticateManagerWithStackScan which triggers stack-scan even
when YAML functions are disabled; update the logic in list_affected.go to guard
creation so it only runs if functions are enabled (e.g., opts.ProcessFunctions
is true) or an identity was explicitly requested (opts.IdentityName not
empty/identity flag selected), otherwise skip calling
CreateAndAuthenticateManagerWithStackScan and leave authManager nil; reference
auth.CreateAndAuthenticateManagerWithStackScan, opts.ProcessFunctions,
opts.IdentityName, cfg.IdentityFlagSelectValue and atmosConfig in the
conditional change.
In `@tests/test-cases/auth-identity-resolution-bugs.yaml`:
- Around line 53-89: The test currently passes because it disables auth with
"--identity off" so CreateAndAuthenticateManagerWithStackScan /
CreateAuthManagerFromIdentityWithStackScan never exercise
pkg/config/stack_auth_loader.go:LoadStackAuthDefaults; change the test case in
tests/test-cases/auth-identity-resolution-bugs.yaml to run with auth enabled
(remove or change the "--identity off" arg so the CLI
auto-detects/authenticates) and update the expect assertions to check for the
resolved default/auth output (e.g., assert the resolved default identity name
and any auth-specific fields) instead of only generic stack metadata so the
imported-default discovery path is actually exercised.
- Around line 97-163: Add an end-to-end test that exercises the auth-manager
path (not just exec-layer) by duplicating one of the existing cases (e.g., the
"plat-staging has NO default in the merged config" scenario) but remove the
"--identity off" flag (or set identity auto/detect) so the global pre-scanner
runs; assert that the plat-staging stack still does NOT receive a default
identity (no identity entry with default: true) and that auth flow exit_code
remains 0 — update tests/test-cases/auth-identity-resolution-bugs.yaml to
include this auth-enabled negative-path case to prevent regressions in the auth
manager global pre-scan leaking defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f175736-a4e7-4ee7-8183-4961381d98f1
📒 Files selected for processing (19)
cmd/describe_affected.gocmd/describe_dependents.gocmd/describe_stacks.gocmd/identity_flag.gocmd/list/utils.godocs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.mdpkg/auth/manager_env_overrides.gopkg/auth/manager_helpers.gopkg/auth/manager_helpers_test.gopkg/config/stack_auth_loader.gopkg/config/stack_auth_loader_test.gopkg/list/list_affected.gotests/fixtures/scenarios/auth-imported-defaults/atmos.yamltests/fixtures/scenarios/auth-imported-defaults/stacks/orgs/acme/dev/_defaults.yamltests/fixtures/scenarios/auth-imported-defaults/stacks/orgs/acme/dev/us-east-1/foundation.yamltests/fixtures/scenarios/auth-stack-scoping/atmos.yamltests/fixtures/scenarios/auth-stack-scoping/stacks/orgs/acme/data/staging/us-east-1/monitoring.yamltests/fixtures/scenarios/auth-stack-scoping/stacks/orgs/acme/plat/staging/us-east-1/eks.yamltests/test-cases/auth-identity-resolution-bugs.yaml
- Use *bool for stackAuthFileWithImports.Default to distinguish "not mentioned" (nil) from "explicitly false" (revoke imported default). Prevents an imported default: true from leaking through when the importing file sets default: false. Two new tests cover the three-state semantics. - Gate auth-manager creation in pkg/list/list_affected.go behind ProcessFunctions || IdentityName (matching describe stacks/affected/ dependents pattern). Avoids unnecessary auth resolution and possible prompts when --process-functions=false. - Remove --identity off from Discussion #122 CLI test cases so they exercise the actual NO-SCAN auth-manager path, not just exec-layer output. plat-staging now asserts data-default.default is null (not true), proving the cross-stack leak does not occur through the auth manager. - Remove coverage-theater describe-stacks CLI test that used --identity off and only checked generic stack metadata. - Use real explicit identity name (not __DISABLED__) in ExplicitIdentitySkipsScan test to exercise the actual scan-guard branch. - Fix 15 stale/incorrect claims in the fix doc: wrong option label, wrong wrapper name in flow diagram, wrong --process-functions value, nonexistent test names, incorrect test counts (20→22 scanner, plus updated auth test list), removed "restored" workflow_utils claim. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/config/stack_auth_loader_test.go (2)
687-731: Rename this case to match the behavior it actually asserts.The name and opening comment say “current file wins,” but the assertion verifies the opposite contract: two different defaults in one merged view are treated as a conflict and discarded. Tightening that wording will make the scanner semantics much easier to follow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/stack_auth_loader_test.go` around lines 687 - 731, Rename the test function TestLoadStackAuthDefaults_CurrentFileWinsOverImport to reflect the actual behavior being asserted (e.g., TestLoadStackAuthDefaults_ConflictingDefaultsInMergedViewAreDiscarded) and update its top comment and any assertion messages to state that when the merged view of a single file contains two different defaults (manifest-identity and imported-identity), the conflict is detected and all defaults are discarded; adjust the function name, the opening comment near the test setup, and the assert.Empty message to match this "conflict/discard" semantics so the test name and description align with the behavior checked by LoadStackAuthDefaults.
884-981: Consider collapsing these helper edge cases into a table-driven block in a small companion file.The coverage is good, but this cluster is very repetitive and pushes
pkg/config/stack_auth_loader_test.goeven farther past the repo’s size cap. A table-driven helper test file would keep the cases easier to scan and extend.As per coding guidelines,
**/*_test.go: "Use table-driven tests for testing multiple scenarios in Go" and**/*.go: "Keep files small and focused (<600 lines)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/stack_auth_loader_test.go` around lines 884 - 981, These tests are repetitive and should be collapsed into one or two table-driven tests in a small companion test file: create a new file (e.g. stack_auth_loader_table_test.go) and replace the cluster of single-case helpers with table-driven cases that call resolveAuthImportPaths, loadAuthWithImports, and extractImportPathString; each table row should include an input value, importer/stacksBasePath values and the expected result (nil, single path, or specific string) and iterate using t.Run, reusing the existing helper assertions (require/ assert) to validate outputs for cases like map-form import with path, unknown types (42, nil, []string), non-YAML extension, nonexistent file, empty stacksBasePath for non-relative imports, .yml fallback, glob no matches, and map[any]any extract variants; keep the original single-case tests only if they exercise unique side effects, otherwise remove them to keep files small and focused and reference the existing functions resolveAuthImportPaths, loadAuthWithImports, and extractImportPathString when implementing the table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.md`:
- Around line 695-699: The documentation paragraph incorrectly claims all three
scenarios run "through the full auth-manager path"; update the paragraph in the
docs to note that the first scenario in tests named
auth-identity-resolution-bugs.yaml still passes "--identity off" and therefore
exercises the exec-layer imported-default check only, while the other two
scenarios run with auth enabled and cover the auth-manager scoping/non-leak
guarantees; reword to explicitly distinguish the exec-layer imported-default
test from the two auth-enabled scoping checks so the coverage summary matches
the checked-in scenarios.
- Around line 729-731: The test summary for
TestLoadStackAuthDefaults_CurrentFileWinsOverImport is misleading because the
test actually asserts that conflicting defaults across merged views are treated
as a conflict and discarded, not that the importing file's default wins; update
the bullet text to state the expected behavior (e.g., "conflicting defaults in
merged views are considered a conflict and discarded") or rename the test to
reflect the discard-on-conflict contract (e.g.,
TestLoadStackAuthDefaults_ConflictsAreDiscarded) and ensure any references to
the old name or wording are updated accordingly.
In `@pkg/config/stack_auth_loader.go`:
- Around line 378-433: normalizeImportPath currently forces extensionless globs
to *.yaml, but matchImportCandidate only falls back to .yml for non-glob paths;
update matchImportCandidate so that when candidate is a glob
(strings.ContainsAny(candidate, "*?[")) and either returns no matches or err, if
the candidate ends with yamlExt also attempt the glob again with the .yml
alternative (replace yamlExt with ymlExt) by calling u.GetGlobMatches on the alt
pattern and return those matches if any; keep existing non-glob .yml stat
fallback as-is. This touches matchImportCandidate and uses u.GetGlobMatches,
yamlExt and ymlExt to locate the alternative glob results.
---
Nitpick comments:
In `@pkg/config/stack_auth_loader_test.go`:
- Around line 687-731: Rename the test function
TestLoadStackAuthDefaults_CurrentFileWinsOverImport to reflect the actual
behavior being asserted (e.g.,
TestLoadStackAuthDefaults_ConflictingDefaultsInMergedViewAreDiscarded) and
update its top comment and any assertion messages to state that when the merged
view of a single file contains two different defaults (manifest-identity and
imported-identity), the conflict is detected and all defaults are discarded;
adjust the function name, the opening comment near the test setup, and the
assert.Empty message to match this "conflict/discard" semantics so the test name
and description align with the behavior checked by LoadStackAuthDefaults.
- Around line 884-981: These tests are repetitive and should be collapsed into
one or two table-driven tests in a small companion test file: create a new file
(e.g. stack_auth_loader_table_test.go) and replace the cluster of single-case
helpers with table-driven cases that call resolveAuthImportPaths,
loadAuthWithImports, and extractImportPathString; each table row should include
an input value, importer/stacksBasePath values and the expected result (nil,
single path, or specific string) and iterate using t.Run, reusing the existing
helper assertions (require/ assert) to validate outputs for cases like map-form
import with path, unknown types (42, nil, []string), non-YAML extension,
nonexistent file, empty stacksBasePath for non-relative imports, .yml fallback,
glob no matches, and map[any]any extract variants; keep the original single-case
tests only if they exercise unique side effects, otherwise remove them to keep
files small and focused and reference the existing functions
resolveAuthImportPaths, loadAuthWithImports, and extractImportPathString when
implementing the table.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db18cb9e-648a-400a-9467-7a6c15f97ead
📒 Files selected for processing (19)
cmd/describe_affected.gocmd/describe_dependents.gocmd/describe_stacks.gocmd/identity_flag.gocmd/list/utils.godocs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.mdpkg/auth/manager_env_overrides.gopkg/auth/manager_helpers.gopkg/auth/manager_helpers_test.gopkg/config/stack_auth_loader.gopkg/config/stack_auth_loader_test.gopkg/list/list_affected.gotests/fixtures/scenarios/auth-imported-defaults/atmos.yamltests/fixtures/scenarios/auth-imported-defaults/stacks/orgs/acme/dev/_defaults.yamltests/fixtures/scenarios/auth-imported-defaults/stacks/orgs/acme/dev/us-east-1/foundation.yamltests/fixtures/scenarios/auth-stack-scoping/atmos.yamltests/fixtures/scenarios/auth-stack-scoping/stacks/orgs/acme/data/staging/us-east-1/monitoring.yamltests/fixtures/scenarios/auth-stack-scoping/stacks/orgs/acme/plat/staging/us-east-1/eks.yamltests/test-cases/auth-identity-resolution-bugs.yaml
…ssue #3) When a component declares auth.identities.<name>.default: true in its stack config and the global atmos.yaml also has a different identity with default: true, both defaults survived the exec-layer deep merge. The user was prompted to choose between multiple defaults (interactive) or got an error (CI/non-interactive). Root cause: MergeComponentAuthConfig in pkg/auth/config_helpers.go does a raw merge.Merge without clearing existing global defaults first. Compare with MergeStackAuthDefaults which already has clearExistingDefaults logic. Fix: added componentAuthHasDefault check + clearExistingIdentityDefaults call before the deep merge. When the component auth section has any identity with default: true, all existing Default flags in the global auth config are cleared first so the component-level default wins cleanly. Tests: 12 new tests in pkg/auth/config_helpers_test.go covering the core override scenario, no-default preservation, same-identity edge case, end-to-end via MergeComponentAuthFromConfig, and helper coverage (componentAuthHasDefault, clearExistingIdentityDefaults). New helpers at 100% coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…x doc - Rename TestLoadStackAuthDefaults_CurrentFileWinsOverImport to ConflictingDefaultsAcrossImportAndFileDiscarded — matches actual behavior (allAgree conflict discard, not "wins"). - Extract 12 repetitive helper edge-case tests into table-driven blocks in new pkg/config/stack_auth_helpers_test.go (194 lines). Reduces stack_auth_loader_test.go from 981 to 880 lines. - Fix doc: update test name reference, correct CLI test descriptions to distinguish --identity off (exec-layer only) from auth-enabled (full NO-SCAN path). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
requested a review
from Erik Osterman (Cloud Posse) (osterman)
April 9, 2026 15:50
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/auth/config_helpers_test.go (1)
509-624: Add input/output isolation assertions for the new default-override tests.These cases validate merged output, but not whether source inputs are preserved. Please add assertions that mutating/processing the result does not mutate the original
globalAuth(and vice versa) so aliasing regressions are caught early.Based on learnings: For aliasing/isolation tests, verify BOTH directions: after a merge, mutate the result and confirm the original inputs are unchanged (result→src isolation); also mutate a source map before the merge and confirm the result is unaffected (src→result isolation).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/config_helpers_test.go` around lines 509 - 624, The tests (TestMergeComponentAuthConfig_ComponentDefaultOverridesGlobalDefault, TestMergeComponentAuthConfig_NoComponentDefault_PreservesGlobalDefault, TestMergeComponentAuthConfig_ComponentDefaultForSameIdentity, TestMergeComponentAuthFromConfig_ComponentDefaultOverridesGlobal) must assert input/output isolation to catch aliasing: after calling MergeComponentAuthConfig or MergeComponentAuthFromConfig, mutate the returned result (e.g., flip result.Identities["..."].Default or add/remove an identity in result.Identities) and assert the original globalAuth and componentAuth/componentConfig remain unchanged (result→src isolation), and conversely mutate the source (e.g., change globalAuth.Identities["..."].Default or add entries to componentAuth/componentConfig) before calling the merge and assert the produced result is unaffected (src→result isolation); add these assertions to each test using the existing variable names (result, globalAuth, componentAuth, componentConfig) so any aliasing regressions are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/auth/config_helpers.go`:
- Around line 91-97: MergeComponentAuthConfig mutates the caller's
globalAuthConfig in-place by calling clearExistingIdentityDefaults on the input
pointer; avoid this side-effect by making a shallow copy of globalAuthConfig
before mutating it and operate on that copy (or otherwise ensure you
return/assign the modified copy back to the caller). Specifically, in
MergeComponentAuthConfig, if componentAuthHasDefault(componentAuthSection) is
true, copy the struct/value of globalAuthConfig into a new local variable and
call clearExistingIdentityDefaults on that copy (or update internal merge logic
to act on a copy) so the original pointer passed in is not altered.
---
Nitpick comments:
In `@pkg/auth/config_helpers_test.go`:
- Around line 509-624: The tests
(TestMergeComponentAuthConfig_ComponentDefaultOverridesGlobalDefault,
TestMergeComponentAuthConfig_NoComponentDefault_PreservesGlobalDefault,
TestMergeComponentAuthConfig_ComponentDefaultForSameIdentity,
TestMergeComponentAuthFromConfig_ComponentDefaultOverridesGlobal) must assert
input/output isolation to catch aliasing: after calling MergeComponentAuthConfig
or MergeComponentAuthFromConfig, mutate the returned result (e.g., flip
result.Identities["..."].Default or add/remove an identity in result.Identities)
and assert the original globalAuth and componentAuth/componentConfig remain
unchanged (result→src isolation), and conversely mutate the source (e.g., change
globalAuth.Identities["..."].Default or add entries to
componentAuth/componentConfig) before calling the merge and assert the produced
result is unaffected (src→result isolation); add these assertions to each test
using the existing variable names (result, globalAuth, componentAuth,
componentConfig) so any aliasing regressions are detected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c61e1f6-8c70-4c6e-8091-4bc5d2c12d98
📒 Files selected for processing (3)
docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.mdpkg/auth/config_helpers.gopkg/auth/config_helpers_test.go
…nfig MergeComponentAuthConfig now copies globalAuthConfig internally via CopyGlobalAuthConfig before clearing defaults. This makes it safe for any direct caller — previously clearExistingIdentityDefaults mutated the input pointer in-place, relying on MergeComponentAuthFromConfig to pass a copy. Updated TestMergeComponentAuthConfig_DoesNotMutateInput to verify non-mutation of the input (was previously documenting the mutation). Added result→src and src→result isolation assertions to MergeComponentAuthFromConfig_ComponentDefaultOverridesGlobal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.md (1)
621-699: Consider adding a subsection for part 3 (component auth merge).The "Fix — Option (d+) implementation" section covers parts 1 (scanner imports) and 2 (entry point split) in detail, but part 3 (clearing global defaults in
MergeComponentAuthConfig) is only described under the Issue 3 section (lines 332-338). Adding a brief subsection here summarizing the component merge fix would improve consistency with the three-part structure and make the document easier to navigate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.md` around lines 621 - 699, Add a short subsection under "Fix — Option (d+) implementation" (after parts 1 and 2) that summarizes the component-auth merge fix: explain that MergeComponentAuthConfig now clears global defaults before merging to avoid inheriting unrelated identities, reference the function name MergeComponentAuthConfig and note the behavior change (clear global defaults then merge component-level auth), and link/point to the existing detailed description in the Issue 3 section (where lines 332–338 discuss it) for full details; keep the subsection concise and consistent with the three-part structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/stack_auth_helpers_test.go`:
- Around line 173-181: In the table-driven tests in stack_auth_helpers_test.go
for the cases named "non-YAML extension" and "nonexistent file", replace the
hardcoded Unix paths in the filePath and basePath fields (e.g. "/tmp/readme.md",
"/tmp", "/nonexistent/path/file.yaml") with OS-agnostic constructions using
filepath.Join (and os.TempDir() for temporary paths); e.g. use
filepath.Join(os.TempDir(), "readme.md") for the temp entry and
filepath.Join(string(os.PathSeparator), "nonexistent", "path", "file.yaml") or a
similarly constructed path via filepath.Join for the nonexistent entry so the
tests work on Windows, macOS, and Linux.
---
Nitpick comments:
In `@docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.md`:
- Around line 621-699: Add a short subsection under "Fix — Option (d+)
implementation" (after parts 1 and 2) that summarizes the component-auth merge
fix: explain that MergeComponentAuthConfig now clears global defaults before
merging to avoid inheriting unrelated identities, reference the function name
MergeComponentAuthConfig and note the behavior change (clear global defaults
then merge component-level auth), and link/point to the existing detailed
description in the Issue 3 section (where lines 332–338 discuss it) for full
details; keep the subsection concise and consistent with the three-part
structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ecf1e719-59fd-407c-a51f-a41bdbf0c66d
📒 Files selected for processing (5)
docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.mdpkg/auth/config_helpers.gopkg/auth/config_helpers_test.gopkg/config/stack_auth_helpers_test.gopkg/config/stack_auth_loader_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/auth/config_helpers.go
- pkg/config/stack_auth_loader_test.go
- pkg/auth/config_helpers_test.go
… isolation - Add subsection 4 to the fix doc covering the Issue #3 component auth merge fix (componentAuthHasDefault, clearExistingIdentityDefaults, internal copy). Adds Issue #3 fixed-flow example. Consistent three-part structure throughout. - Use t.TempDir() + filepath.Join() instead of hardcoded /tmp paths in TestLoadAuthWithImports_EdgeCases for cross-platform compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
approved these changes
Apr 9, 2026
Contributor
|
These changes were released in v1.215.0-rc.5. |
This was referenced Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
auth.identities.<name>.default: truein imported stack files not recognized during identity resolution #2293: Teaches the stack auth scanner to followimport:chains recursively, soauth.identities.<name>.default: truedeclared in an imported_defaults.yamlis visible to every command — including multi-stack commands likedescribe stacks/describe affected/list affected.pkg/authentry points into a NO-SCAN variant (for commands with a stack-scoped merged auth config) and a SCAN variant (for multi-stack commands). The split makes the cross-stack default-identity leak structurally impossible for terraform/helmfile/describe-component flows.auth.identities.<name>.default: trueand the globalatmos.yamlalso has a different default, the component-level default now wins cleanly instead of producing "multiple default identities" prompts.allAgreeconflict-detection, the describe-affected AuthManager threading, and the MCP scoped-auth env-override flow.why
auth.identities.<name>.default: truein imported stack files not recognized during identity resolution #2293 — imported defaults invisible: whenauth.identities.<name>.default: truewas declared in an imported_defaults.yaml(especially one listed understacks.excluded_paths, which is the common reference-architecture layout), the pre-scanner never saw it. Users hit "No default identity configured" on commands that should have auto-authenticated. The exec-layer merge path already handled this correctly for terraform/helmfile/describe-component, but multi-stack commands (describe stacks,describe affected,list affected, workflows,aws security/compliance, MCP scoped auth) all failed.default: true, that identity silently propagated to every other stack across all tenants. Runningatmos terraform plan eks -s plat-stagingwould pick up thedata-stagingdefault declared in an unrelated stack file. Reported to reproduce against Atmos 1.210, 1.211, and 1.213.default: truethan the globalatmos.yamldefault, both defaults survived the exec-layer deep merge inMergeComponentAuthConfig. Users were prompted to choose between multiple defaults (interactive) or got errors (CI). This broke the expected Atmos inheritance semantics where more-specific config overrides more-general.terraform *but regresseddescribe stacks,describe affected,list affected,list instances,aws security,aws compliance, and workflow execution — all of which were documented indocs/fixes/stack-level-default-auth-identity.mdas intentionally using the pre-scanner (Approach 2). This PR preserves that Approach 2 code path while fixing all three bugs, so no existing user-visible functionality is removed.references
auth.identities.<name>.default: truein imported stack files not recognized during identity resolution #2293docs/fixes/2026-04-08-atmos-auth-identity-resolution-fixes.md(full caller audit, rejected alternatives, coverage matrix)docs/fixes/stack-level-default-auth-identity.md(Approach 1 / Approach 2 design this PR preserves and extends)docs/fixes/2026-02-12-auth-realm-isolation-issues.md(Issue atmos loading unrelated stack files #2072allAgreeconflict-detection preserved unchanged)docs/fixes/2026-03-25-describe-affected-auth-identity-not-used.md(AuthManager threading through describe/list-affected preserved unchanged)docs/fixes/2026-04-06-mcp-server-env-not-applied-to-auth-setup.md(MCP scoped-auth flow now routes through the scan variant)Summary by CodeRabbit