Conversation
External MCP servers configured with both `identity:` and an `env:` block containing ATMOS_* variables (e.g. ATMOS_PROFILE, ATMOS_CLI_CONFIG_PATH) failed auth setup with `identity not found` because the parent auth manager was constructed once from the parent's os.Environ and never saw the server's env block. This affected any external MCP server, including configurations that run `atmos mcp start` itself as a server. Defer auth manager construction to per-server: each server's ATMOS_* env vars are temporarily applied to os.Environ for the duration of manager construction (cfg.InitCliConfig + auth.CreateAndAuthenticateManager) and restored immediately after. - pkg/mcp/client/env_overrides.go: ApplyAtmosEnvOverrides helper. - pkg/mcp/client/session.go: PerServerAuthProvider interface; WithAuthManager prefers ForServer when the provider implements it. - cmd/mcp/client/auth_factory.go: PerServerAuthManager for management commands (atmos mcp test/tools/status/restart). - cmd/ai/init.go: parallel perServerAuthProvider for AI commands (atmos ai ask/chat/exec). - docs/fixes/2026-04-06-mcp-server-env-not-applied-to-auth-setup.md. - Tests: env_overrides_test.go, auth_factory_test.go, init_perserver_auth_test.go, plus extended auth_test.go. Coverage at 100% on all new logic. 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
📝 WalkthroughWalkthroughDeferred, per-server scoped MCP authentication was introduced: auth providers are now constructed per server with server Changes
Sequence DiagramsequenceDiagram
participant Cmd as Command Init
participant AuthOpt as buildAuthOption
participant MCP as MCP Session
participant SAP as ScopedAuthProvider
participant EM as EnvMgr
participant Auth as AuthManager
Cmd->>AuthOpt: Check if any MCP servers need auth
AuthOpt->>SAP: Create NewScopedAuthProvider(cfg)
AuthOpt->>MCP: Inject WithAuthManager(scoped provider)
MCP->>SAP: WithAuthManager(ctx, parsedConfig)
alt provider implements PerServerAuthProvider
SAP->>SAP: Call ForServer(ctx, parsedConfig)
SAP->>EM: Apply parsedConfig.Env via SetWithRestore()
EM->>Auth: CreateAndAuthenticateManagerWithEnvOverrides(overrides)
Auth-->>EM: Return manager
EM-->>SAP: Restore env
SAP->>MCP: Return scoped AuthEnvProvider or error
MCP->>SAP: Call PrepareShellEnvironment() on scoped provider
SAP->>Auth: Build manager (no overrides) → return shell env
else provider is global
SAP->>MCP: Use original provider unchanged
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![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: 1
🧹 Nitpick comments (2)
cmd/mcp/client/auth_factory.go (1)
69-84: Same nil-check concern applies tobuildManagerreturn inForServer.While
session.godoes check for nil scoped provider (line 252-254), it's better to be explicit here. IfcreateAuthManagerreturns(nil, nil),ForServerreturns a nilAuthEnvProviderwithout error, which relies on the caller to check.Consider adding an explicit nil check for consistency and clearer error messages:
♻️ Suggested improvement
func (p *PerServerAuthManager) ForServer(_ context.Context, config *mcpclient.ParsedConfig) (mcpclient.AuthEnvProvider, error) { defer perf.Track(nil, "cmd.mcp.client.PerServerAuthManager.ForServer")() mgr, err := p.buildManager(config.Env) if err != nil { return nil, fmt.Errorf("failed to build auth manager for MCP server %q: %w", config.Name, err) } + if mgr == nil { + return nil, fmt.Errorf("auth manager unavailable for MCP server %q (no identity resolved)", config.Name) + } return mgr, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/mcp/client/auth_factory.go` around lines 69 - 84, PerServerAuthManager.buildManager currently returns whatever createAuthManager returns, which can be (nil, nil) and lead ForServer to return a nil AuthEnvProvider silently; after calling createAuthManager (in buildManager), explicitly check if the returned auth.AuthManager is nil and, if so, return a descriptive error (e.g., "createAuthManager returned nil manager") instead of a nil manager with no error so callers (and ForServer) won’t receive a nil AuthEnvProvider unexpectedly; reference createAuthManager, PerServerAuthManager.buildManager, and ForServer when making the change.cmd/mcp/client/auth_factory_test.go (1)
42-45: Consider adding a test for nil manager return.Related to the issue flagged in
auth_factory.go: there's no test coverage for whencreateAuthManagerreturns(nil, nil). Adding such a test would both document the expected behavior and catch regressions if the nil check is added.📝 Suggested test addition
func TestPerServerAuthManager_ForServer_NilManager_ReturnsError(t *testing.T) { withCleanProfile(t) p := NewPerServerAuthManager(&schema.AtmosConfiguration{}) p.initConfig = func(_ schema.ConfigAndStacksInfo, _ bool) (schema.AtmosConfiguration, error) { return schema.AtmosConfiguration{}, nil } p.createAuthManager = func(_ string, _ *schema.AuthConfig, _ string, _ *schema.AtmosConfiguration) (auth.AuthManager, error) { return nil, nil // Simulates auth disabled or no identity resolved } cfg := &mcpclient.ParsedConfig{ Name: "atmos", Identity: "some-identity", } _, err := p.ForServer(context.Background(), cfg) require.Error(t, err) // Should error, not return nil provider }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/mcp/client/auth_factory_test.go` around lines 42 - 45, The test suite lacks coverage for the case where createAuthManager returns (nil, nil); add a unit test that constructs a PerServerAuthManager via NewPerServerAuthManager, stub initConfig to return a valid AtmosConfiguration, and stub createAuthManager to return (nil, nil) to simulate disabled/no-identity auth; then call ForServer (or PerServerAuthManager.ForServer) with a ParsedConfig and assert it returns an error (not a nil provider). Include references to createAuthManager, initConfig, NewPerServerAuthManager, and ForServer in the test so it fails if future changes remove the nil check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/mcp/client/auth_factory.go`:
- Around line 46-54: Both PerServerAuthManager.PrepareShellEnvironment and
PerServerAuthManager.ForServer must guard against buildManager returning (nil,
nil); currently PrepareShellEnvironment will dereference a nil mgr and ForServer
returns a nil AuthEnvProvider. Update PrepareShellEnvironment to check mgr ==
nil after calling p.buildManager and return a safe error (or a no-op empty env,
per project convention) instead of calling mgr.PrepareShellEnvironment; update
ForServer to check the result of p.buildManager and return a well-defined error
or a fallback AuthEnvProvider when mgr is nil rather than returning nil. Ensure
references to the functions buildManager,
PerServerAuthManager.PrepareShellEnvironment, PerServerAuthManager.ForServer and
the AuthEnvProvider interface are used so callers get a non-nil contract or an
explicit error.
---
Nitpick comments:
In `@cmd/mcp/client/auth_factory_test.go`:
- Around line 42-45: The test suite lacks coverage for the case where
createAuthManager returns (nil, nil); add a unit test that constructs a
PerServerAuthManager via NewPerServerAuthManager, stub initConfig to return a
valid AtmosConfiguration, and stub createAuthManager to return (nil, nil) to
simulate disabled/no-identity auth; then call ForServer (or
PerServerAuthManager.ForServer) with a ParsedConfig and assert it returns an
error (not a nil provider). Include references to createAuthManager, initConfig,
NewPerServerAuthManager, and ForServer in the test so it fails if future changes
remove the nil check.
In `@cmd/mcp/client/auth_factory.go`:
- Around line 69-84: PerServerAuthManager.buildManager currently returns
whatever createAuthManager returns, which can be (nil, nil) and lead ForServer
to return a nil AuthEnvProvider silently; after calling createAuthManager (in
buildManager), explicitly check if the returned auth.AuthManager is nil and, if
so, return a descriptive error (e.g., "createAuthManager returned nil manager")
instead of a nil manager with no error so callers (and ForServer) won’t receive
a nil AuthEnvProvider unexpectedly; reference createAuthManager,
PerServerAuthManager.buildManager, and ForServer when making the change.
🪄 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: 4778ecbd-8aea-49dc-9418-867e6518932d
📒 Files selected for processing (10)
cmd/ai/init.gocmd/ai/init_perserver_auth_test.gocmd/mcp/client/auth_factory.gocmd/mcp/client/auth_factory_test.gocmd/mcp/client/start_options.godocs/fixes/2026-04-06-mcp-server-env-not-applied-to-auth-setup.mdpkg/mcp/client/auth_test.gopkg/mcp/client/env_overrides.gopkg/mcp/client/env_overrides_test.gopkg/mcp/client/session.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2291 +/- ##
==========================================
+ Coverage 76.99% 77.08% +0.08%
==========================================
Files 1060 1063 +3
Lines 100618 100683 +65
==========================================
+ Hits 77475 77615 +140
+ Misses 18847 18775 -72
+ Partials 4296 4293 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
osterman
reviewed
Apr 7, 2026
osterman
reviewed
Apr 7, 2026
osterman
reviewed
Apr 7, 2026
…tmos env namespace Addresses CodeRabbit and review feedback on PR #2291: - Move save/set/restore env primitive to pkg/env (env.SetWithRestore) — atmos already has a dedicated env package, and four other local variants existed in internal/exec, pkg/telemetry, pkg/auth/identities/aws, and pkg/auth/cloud/gcp. - Move "create+authenticate manager with env overrides" to pkg/auth as a high-level primitive that delegates env mutation to pkg/env. - Reduce pkg/mcp/client/scoped_auth.go to a thin MCP-specific adapter (~85 lines) over the pkg/auth primitive. Implements PerServerAuthProvider so WithAuthManager dispatches per-server. - Add canonical Atmos env-var namespace constants (AtmosEnvVarNamespace, AtmosEnvVarPrefix) to pkg/config/const.go with build-time invariant tests. Migrate five hardcoded literal call sites in cmd/root.go, cmd/auth_validate.go, pkg/auth, and pkg/ai/agent/codexcli to use them. - Guard nil-manager returns from CreateAndAuthenticateManagerWithAtmosConfig with errUtils.ErrMCPServerAuthUnavailable wrapping (sentinel + server + identity context) so callers can errors.Is-match. - Delete duplicated cmd/mcp/client/auth_factory.go and the parallel perServerAuthProvider in cmd/ai/init.go. - 100% coverage on every new function (env.SetWithRestore including the setenv-error branch via injectable hook; CreateAndAuthenticateManagerWith- EnvOverrides including the env-hook error branch; ScopedAuthProvider all paths; filterAtmosOverrides table-driven; const invariants). Net layering: pkg/env → SetWithRestore (foundational, generic) pkg/auth → CreateAndAuthenticateManagerWithEnvOverrides (composes pkg/env) pkg/mcp/client → ScopedAuthProvider (thin MCP adapter) cmd/ → one-line consumers, zero auth-factory code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aknysh
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cmd/mcp/client/start_options_auth_test.go (1)
26-43: Add a compile-time schema field sentinel in this test file.The tests depend on
schema.MCPServerConfig.Commandand.Identity; add a compile guard to catch field renames at build time.As per coding guidelines, “Add compile-time sentinels for schema field references in tests: when a test uses a specific struct field … add … as a compile guard so a field rename immediately fails the build.”💡 Proposed patch
package client import ( "testing" @@ mcpclient "github.com/cloudposse/atmos/pkg/mcp/client" "github.com/cloudposse/atmos/pkg/schema" ) +var _ = schema.MCPServerConfig{ + Command: "echo", + Identity: "ci", +} + func TestMcpServersNeedAuth(t *testing.T) {Also applies to: 56-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/mcp/client/start_options_auth_test.go` around lines 26 - 43, The test uses schema.MCPServerConfig.Command and .Identity but lacks a compile-time sentinel to catch future renames; add a small unused variable or blank-ident assignment in start_options_auth_test.go that directly references schema.MCPServerConfig{} .Command and .Identity (e.g., _ = schema.MCPServerConfig{}.Command; _ = schema.MCPServerConfig{}.Identity) so the compiler will fail if those fields are renamed, placing it near the top of the test file alongside other test setup.cmd/ai/init_resolve_auth_test.go (1)
15-25: Add a compile-time schema field sentinel.These tests rely on
schema.MCPServerConfig.Commandand.Identity; add a compile guard so field renames fail fast at compile time.As per coding guidelines, “Add compile-time sentinels for schema field references in tests: when a test uses a specific struct field … add … as a compile guard so a field rename immediately fails the build.”💡 Proposed patch
package ai import ( "testing" @@ mcpclient "github.com/cloudposse/atmos/pkg/mcp/client" "github.com/cloudposse/atmos/pkg/schema" ) +var _ = schema.MCPServerConfig{ + Command: "echo", + Identity: "ci", +} + func TestResolveAuthProvider_NoIdentity_ReturnsNil(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ai/init_resolve_auth_test.go` around lines 15 - 25, Add a compile-time sentinel that references the struct fields used by these tests so renaming is caught at build time: add a top-level unused variable like var _ = schema.MCPServerConfig{Command: "", Identity: ""} in the test file (adjacent to TestResolveAuthProvider_*), which will fail to compile if schema.MCPServerConfig.Command or .Identity are renamed. This keeps resolveAuthProvider and the tests unchanged while providing the required compile guard.pkg/auth/manager_env_overrides.go (1)
62-73: Add context when returning initialization errors.Line [66] and Line [72] return raw errors, which makes troubleshooting harder when this path is called from MCP startup flows.
As per coding guidelines: "Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`."🧩 Proposed fix
import ( + "fmt" "strings" @@ restore, err := setEnvWithRestoreFn(atmosOnly) if err != nil { if restore != nil { restore() } - return nil, err + return nil, fmt.Errorf("apply ATMOS_* env overrides: %w", err) } @@ loadedConfig, err := initCliConfigFn(schema.ConfigAndStacksInfo{}, false) if err != nil { - return nil, err + return nil, fmt.Errorf("initialize CLI config for scoped auth manager: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/manager_env_overrides.go` around lines 62 - 73, The returns of raw errors after calling restore() and after calling initCliConfigFn(schema.ConfigAndStacksInfo{}, false) should be replaced with wrapped errors that add context using fmt.Errorf; update the error return where restore() is invoked to something like fmt.Errorf("restoring env overrides: %w", err) and wrap the initCliConfigFn error with context like fmt.Errorf("initializing CLI config: %w", err) so callers get actionable messages; reference the restore variable and the initCliConfigFn call (and the loadedConfig flow) when making these changes.pkg/auth/manager_env_overrides_test.go (1)
54-67: Protect package-hook swaps with a test mutex.These tests mutate package-level hooks. Adding a package-level lock around hook swapping will prevent future flakiness if any same-package tests run in parallel.
Based on learnings: "Tests that mutate these [DI hooks] must not use t.Parallel() and should always restore originals via defer to avoid cross-test interference."🛡️ Suggested hardening
import ( "context" "errors" "os" + "sync" "testing" @@ ) + +var managerEnvOverridesTestMu sync.Mutex @@ func withStubbedManagerEnvOverrides( t *testing.T, observed *string, errOnInit error, mgrToReturn AuthManager, ) { t.Helper() + managerEnvOverridesTestMu.Lock() + t.Cleanup(func() { managerEnvOverridesTestMu.Unlock() }) origInit := initCliConfigFn origCreate := createAuthManagerAlso applies to: 226-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/manager_env_overrides_test.go` around lines 54 - 67, The helper withStubbedManagerEnvOverrides swaps package-level DI hooks without synchronization; add a package-level mutex (e.g., testHookMu sync.Mutex) and acquire it at the start of withStubbedManagerEnvOverrides (testHookMu.Lock()) and release with defer testHookMu.Unlock() to prevent concurrent tests clobbering hooks; also apply the same mutex locking pattern to the other hook-swapping helper(s) referenced around lines 226-255 so all package-level hook swaps are serialized and always restore originals in the t.Cleanup block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/mcp/client/start_options_auth_test.go`:
- Around line 67-78: The test currently only length-checks opts but validates a
separately constructed provider; instead invoke the StartOption produced by
buildAuthOption to capture the actual provider it wires so the assertion is tied
to opts[0]. Create a small test "holder" object that matches the StartOption
target the options expect, call opts[0](holder) (or otherwise apply the
StartOption) to retrieve the provider instance, then assert that the captured
provider satisfies mcpclient.AuthEnvProvider and mcpclient.PerServerAuthProvider
and is the same implementation produced by mcpclient.NewScopedAuthProvider; use
the symbols buildAuthOption, opts, and mcpclient.NewScopedAuthProvider to locate
the code to change.
In `@pkg/auth/manager_env_overrides.go`:
- Around line 56-77: This function mutates process-global env and then reads it,
so wrap the whole env-override lifecycle to prevent concurrent leakage:
introduce a package-level mutex (e.g., envAuthInitMu sync.Mutex) and lock it at
the start of CreateAndAuthenticateManagerWithEnvOverrides before calling
setEnvWithRestoreFn, keep it locked through initCliConfigFn and
createAuthManager, and only unlock after restore() has been executed (ensure
unlock happens even on error by deferring restore and unlocking together).
Reference symbols: CreateAndAuthenticateManagerWithEnvOverrides,
setEnvWithRestoreFn, restore(), initCliConfigFn, createAuthManager.
In `@pkg/mcp/client/scoped_auth.go`:
- Around line 69-75: In ScopedAuthProvider.ForServer, add a nil check for the
config parameter (ParsedConfig) at the start of the method and return a clear,
typed error (e.g., fmt.Errorf("nil config") or a package-specific sentinel
error) instead of proceeding and dereferencing config.Name; this prevents a
panic when config is nil and keeps the error path explicit before calling
p.buildManagerFn or using config fields.
---
Nitpick comments:
In `@cmd/ai/init_resolve_auth_test.go`:
- Around line 15-25: Add a compile-time sentinel that references the struct
fields used by these tests so renaming is caught at build time: add a top-level
unused variable like var _ = schema.MCPServerConfig{Command: "", Identity: ""}
in the test file (adjacent to TestResolveAuthProvider_*), which will fail to
compile if schema.MCPServerConfig.Command or .Identity are renamed. This keeps
resolveAuthProvider and the tests unchanged while providing the required compile
guard.
In `@cmd/mcp/client/start_options_auth_test.go`:
- Around line 26-43: The test uses schema.MCPServerConfig.Command and .Identity
but lacks a compile-time sentinel to catch future renames; add a small unused
variable or blank-ident assignment in start_options_auth_test.go that directly
references schema.MCPServerConfig{} .Command and .Identity (e.g., _ =
schema.MCPServerConfig{}.Command; _ = schema.MCPServerConfig{}.Identity) so the
compiler will fail if those fields are renamed, placing it near the top of the
test file alongside other test setup.
In `@pkg/auth/manager_env_overrides_test.go`:
- Around line 54-67: The helper withStubbedManagerEnvOverrides swaps
package-level DI hooks without synchronization; add a package-level mutex (e.g.,
testHookMu sync.Mutex) and acquire it at the start of
withStubbedManagerEnvOverrides (testHookMu.Lock()) and release with defer
testHookMu.Unlock() to prevent concurrent tests clobbering hooks; also apply the
same mutex locking pattern to the other hook-swapping helper(s) referenced
around lines 226-255 so all package-level hook swaps are serialized and always
restore originals in the t.Cleanup block.
In `@pkg/auth/manager_env_overrides.go`:
- Around line 62-73: The returns of raw errors after calling restore() and after
calling initCliConfigFn(schema.ConfigAndStacksInfo{}, false) should be replaced
with wrapped errors that add context using fmt.Errorf; update the error return
where restore() is invoked to something like fmt.Errorf("restoring env
overrides: %w", err) and wrap the initCliConfigFn error with context like
fmt.Errorf("initializing CLI config: %w", err) so callers get actionable
messages; reference the restore variable and the initCliConfigFn call (and the
loadedConfig flow) when making these changes.
🪄 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: 381e40bd-a82e-4da0-a087-62fbd54f035c
📒 Files selected for processing (17)
cmd/ai/init.gocmd/ai/init_resolve_auth_test.gocmd/auth_validate.gocmd/mcp/client/start_options.gocmd/mcp/client/start_options_auth_test.gocmd/root.godocs/fixes/2026-04-06-mcp-server-env-not-applied-to-auth-setup.mdpkg/ai/agent/codexcli/client.gopkg/ai/agent/codexcli/client_test.gopkg/auth/manager_env_overrides.gopkg/auth/manager_env_overrides_test.gopkg/config/const.gopkg/config/const_test.gopkg/env/restore.gopkg/env/restore_test.gopkg/mcp/client/scoped_auth.gopkg/mcp/client/scoped_auth_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/ai/agent/codexcli/client.go
- pkg/ai/agent/codexcli/client_test.go
- pkg/config/const.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/ai/init.go
- cmd/mcp/client/start_options.go
…ency safety This is a follow-up to the previous refactor commit, addressing six CodeRabbit comments on PR #2291. Compile-time schema sentinels (CLAUDE.md guideline): - cmd/mcp/client/start_options_auth_test.go: var _ = schema.MCPServerConfig{...} - cmd/ai/init_resolve_auth_test.go: same. Field renames now fail the build before tests run. Wrap errors with context using static sentinels (atmos-errors.md): - pkg/auth/manager_env_overrides.go: both error returns now wrap errUtils.ErrAuthManager as the leading %w with descriptive context, so callers can errors.Is-match the sentinel. - All errors.New("...") calls in test files moved to package-level static sentinel vars (errTestInitBoom, errTestSetenvBoom, errTestNilCleanup, errTestScopedBuilderInit). Test assertions now use ErrorIs for stronger contract checking. Critical concurrency fix: - pkg/auth/manager_env_overrides.go: CreateAndAuthenticateManagerWithEnvOverrides now serializes via managerEnvOverridesMu sync.Mutex. Previously documented as "NOT goroutine-safe" but unenforced — concurrent callers would race on os.Environ() between SetWithRestore and InitCliConfig, leading to wrong identity loading. Lock lives at the composition layer (not in pkg/env) because the unsafe pattern is read-after-write across multiple steps. - New test TestCreateAndAuthenticateManagerWithEnvOverrides_ConcurrentSafe runs 32 goroutines under -race, asserting each goroutine observes a stable ATMOS_PROFILE during its own InitCliConfig invocation. Mutation-verified to fail without the lock. Defensive guards at public-API boundaries: - pkg/mcp/client/scoped_auth.go: ForServer now nil-checks config before dereferencing. Returns ErrMCPServerAuthUnavailable wrapped with context. PerServerAuthProvider is an exported interface — external implementations could pass nil; the guard surfaces a typed error instead of panicking. - New test TestScopedAuthProvider_ForServer_NilConfig_ReturnsSentinel. Mutation-verified to catch the panic without the guard. Test mutex for hook-mutating tests: - pkg/auth/manager_env_overrides_test.go: managerEnvOverridesTestMu sync.Mutex serializes withStubbedManagerEnvOverrides. Defensive against future t.Parallel() additions that would silently flake. Adapter error simplification (stops double-wrapping): - pkg/mcp/client/scoped_auth.go: ForServer drops its inner "failed to build auth manager for X" wrap. Session.Start already wraps with ErrMCPServerStartFailed and the server name; the adapter's wrap was redundant and created 6-layer error chains. - pkg/mcp/client/session.go: WithAuthManager dispatcher likewise simplified. - Updated TestWithAuthManager_PerServerProvider_ForServerError and TestScopedAuthProvider_ForServer_BuilderError_PassesThrough to assert the pass-through contract via errors.Is + assert.Equal. Test contract strengthening: - cmd/mcp/client/start_options_auth_test.go: TestBuildAuthOption_ReturnsScopedProvider was a tautology (just checked length and constructed a separate provider). Replaced with TestBuildAuthOption_ReturnsWiredWithAuthManagerOption which behaviorally probes the returned StartOption via the no-identity pass-through contract — proves the wiring without inspecting the closure. All new functions remain at 100% coverage. No production behavior change for existing callers; all changes are safety/correctness/clarity. 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)
pkg/mcp/client/scoped_auth_test.go (1)
189-207: These smoke tests are mostly tautological.
TestScopedAuthProvider_ImplementsBothInterfacesdefers the breakage to runtime, andTestNewScopedAuthProvider_DefaultBuilderWiredToAuthPackagepasses for any non-nilfunction. A pair of compile-time assertions likevar _ AuthEnvProvider = (*ScopedAuthProvider)(nil)andvar _ PerServerAuthProvider = (*ScopedAuthProvider)(nil)plus the existing behavior tests would give stronger signal with less noise.As per coding guidelines, "Test behavior, not implementation. Never test stub functions. Avoid tautological tests. No coverage theater."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mcp/client/scoped_auth_test.go` around lines 189 - 207, Replace the tautological runtime tests TestScopedAuthProvider_ImplementsBothInterfaces and TestNewScopedAuthProvider_DefaultBuilderWiredToAuthPackage with compile-time interface assertions: add var _ AuthEnvProvider = (*ScopedAuthProvider)(nil) and var _ PerServerAuthProvider = (*ScopedAuthProvider)(nil) to assert type conformance, remove the presence-check test of p.buildManagerFn (or keep behavior tests that exercise NewScopedAuthProvider via real behavior), and remove the unused reference to auth.CreateAndAuthenticateManagerWithEnvOverrides; keep other behavior-focused tests intact.
🤖 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/manager_env_overrides_test.go`:
- Around line 330-374: The worker goroutine is calling require.NoError (which
calls t.FailNow) and isn't sending each goroutine's intended want into the
observation stream; change the test so worker goroutines do not call
require.NoError and instead capture the error and their requested want and send
an observation containing want, seen, seenAfter (and optionally err) into the
observations channel; update the observation struct to include a want (and maybe
err) field, have initCliConfigFn produce observed seen/seenAfter as before, have
each goroutine call CreateAndAuthenticateManagerWithEnvOverrides, send
observation{want: want, expected: seen, actual: seenAfter, err: err} to
observations, then in the main test loop receive each observation, assert err is
nil and assert o.expected==o.actual and o.want==o.expected to verify each
goroutine saw its own profile and that no goroutine called t.FailNow from a
worker.
---
Nitpick comments:
In `@pkg/mcp/client/scoped_auth_test.go`:
- Around line 189-207: Replace the tautological runtime tests
TestScopedAuthProvider_ImplementsBothInterfaces and
TestNewScopedAuthProvider_DefaultBuilderWiredToAuthPackage with compile-time
interface assertions: add var _ AuthEnvProvider = (*ScopedAuthProvider)(nil) and
var _ PerServerAuthProvider = (*ScopedAuthProvider)(nil) to assert type
conformance, remove the presence-check test of p.buildManagerFn (or keep
behavior tests that exercise NewScopedAuthProvider via real behavior), and
remove the unused reference to
auth.CreateAndAuthenticateManagerWithEnvOverrides; keep other behavior-focused
tests intact.
🪄 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: 53fb4590-77b8-4908-9601-108443f70860
📒 Files selected for processing (8)
cmd/ai/init_resolve_auth_test.gocmd/mcp/client/start_options_auth_test.gopkg/auth/manager_env_overrides.gopkg/auth/manager_env_overrides_test.gopkg/mcp/client/auth_test.gopkg/mcp/client/scoped_auth.gopkg/mcp/client/scoped_auth_test.gopkg/mcp/client/session.go
✅ Files skipped from review due to trivial changes (2)
- cmd/ai/init_resolve_auth_test.go
- cmd/mcp/client/start_options_auth_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/mcp/client/scoped_auth.go
- pkg/mcp/client/auth_test.go
Addresses two more CodeRabbit comments on PR #2291. 1. ConcurrentSafe test was unsafe and weak (pkg/auth/manager_env_overrides_test.go) The previous version called require.NoError from worker goroutines, which is unsafe per testify docs — require.* invokes t.FailNow() which calls runtime.Goexit() and only exits the current goroutine, leaving t in an inconsistent state. It also only checked that the two reads inside one stub call agreed, not that each goroutine observed its own ATMOS_PROFILE value (missing the cross-goroutine hijack vector entirely). Rewrote to: - Route all worker errors back to the main test goroutine via channels; errors are surfaced with t.Errorf in the main goroutine where it's safe. - Give each goroutine a unique ATMOS_PROFILE want (profile-00 .. profile-31). - Use multiset comparison to verify every want was observed exactly once and no goroutine saw a value it did not request — catches cross-goroutine observation hijacks. - Keep the in-stub double-read divergence check as a second race vector, forwarded via the worker-error channel instead of in-goroutine assertion. Mutation-verified to catch both vectors (divergence + missing wants + unrequested extras) when the production lock is removed. 2. Tautological smoke tests removed (pkg/mcp/client/scoped_auth_test.go) TestScopedAuthProvider_ImplementsBothInterfaces deferred a compile-time fact to runtime via t.(...) type assertion. TestNewScopedAuthProvider_Default- BuilderWiredToAuthPackage claimed to verify wiring but was untestable in Go (function values cannot be compared for equality), so it degraded to a non-nil check that passes for any constructor. Replaced both with package-level compile-time interface assertions in pkg/mcp/client/scoped_auth.go: var ( _ AuthEnvProvider = (*ScopedAuthProvider)(nil) _ PerServerAuthProvider = (*ScopedAuthProvider)(nil) ) Added an explanatory comment in the test file documenting why these checks moved to the production file and why default-builder wiring is not testable at runtime, so a future reader doesn't try to "fix" the missing tests. Per CLAUDE.md: "Test behavior, not implementation. Avoid tautological tests. No coverage theater." Coverage on all production functions remains 100%. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/auth/manager_env_overrides_test.go (1)
42-50: Use t.Setenv to simplify environment setup and guarantee automatic cleanup.The
unsetEnvForTesthelper at lines 42–50 manually restores environment variables viaos.Setenvandos.Unsetenv. Switch tot.Setenvfor each key; it handles cleanup automatically and ensures test-scoped isolation without manual teardown logic.for _, k := range keys { - old, had := os.LookupEnv(k) - require.NoError(t, os.Unsetenv(k)) - t.Cleanup(func() { - if had { - _ = os.Setenv(k, old) - } else { - _ = os.Unsetenv(k) - } - }) + t.Setenv(k, "") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/manager_env_overrides_test.go` around lines 42 - 50, The helper unsetEnvForTest currently unsets env vars and manually restores them via t.Cleanup; replace that logic by calling t.Setenv for each key in keys to set an empty value for the duration of the test and rely on t.Setenv's automatic restoration on test completion (i.e., iterate the same keys and invoke t.Setenv per key instead of using os.Unsetenv and custom t.Cleanup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/auth/manager_env_overrides_test.go`:
- Around line 42-50: The helper unsetEnvForTest currently unsets env vars and
manually restores them via t.Cleanup; replace that logic by calling t.Setenv for
each key in keys to set an empty value for the duration of the test and rely on
t.Setenv's automatic restoration on test completion (i.e., iterate the same keys
and invoke t.Setenv per key instead of using os.Unsetenv and custom t.Cleanup).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58671255-83d6-4dce-ac6a-9796eead6d53
📒 Files selected for processing (3)
pkg/auth/manager_env_overrides_test.gopkg/mcp/client/scoped_auth.gopkg/mcp/client/scoped_auth_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/mcp/client/scoped_auth_test.go
osterman
approved these changes
Apr 7, 2026
|
These changes were released in v1.215.0-rc.4. |
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
identity:and anenv:block containingATMOS_*variables (e.g.ATMOS_PROFILE,ATMOS_CLI_CONFIG_PATH,ATMOS_BASE_PATH) failed auth setup withidentity not found. The parent's auth manager was built once fromos.Environ()and never saw the server'senv:block.env.SetWithRestoreinpkg/env(atmos already has a dedicated env package; four other local save/set/restore variants exist and should consolidate to this in a follow-up).auth.CreateAndAuthenticateManagerWithEnvOverridesinpkg/auththat delegates env mutation topkg/envand composescfg.InitCliConfig+auth.CreateAndAuthenticateManagerWithAtmosConfig.mcpclient.ScopedAuthProvider(~85 lines) that implements a newPerServerAuthProviderinterface soWithAuthManagerdispatches per-server.AtmosEnvVarNamespace,AtmosEnvVarPrefix) added topkg/config/const.gowith build-time invariant tests. Migrated five hardcoded\"ATMOS\"/\"ATMOS_\"literals scattered acrosscmd/root.go,cmd/auth_validate.go,pkg/auth, andpkg/ai/agent/codexclito use them.ATMOS_PROFILE(or any otherATMOS_*variable) in itsenv:block and have it influence identity resolution — including configurations that runatmos mcp startitself as an external MCP server (the original report).why
os.Environ(). Serverenv:blocks were only applied to the spawned subprocess viacmd.Env, so the parent's identity lookup ran against the default profile and never sawATMOS_PROFILE: managers(or any other override). The user's only workaround was exporting the variable in the shell before running atmos.atmos mcp startitself as an external MCP server (the original report), but it affects any external MCP server with bothidentity:andenv: ATMOS_*: ....cmd/mcp/server) was already correct: when spawned as a subprocess it inherits the merged env (os.Environ()+env:block) andcfg.InitCliConfigreadsATMOS_*from there.cmd/is a slippery slope, (b)pkg/authshouldn't re-implement env mutation whenpkg/envalready exists, and (c) defining what counts as an Atmos env variable inside any specific subsystem is the wrong layering. Each round of feedback pushed the primitives down to where they belong, and the result is significantly less code with cleaner layering.layering after this PR
```
pkg/config ← AtmosEnvVarNamespace / AtmosEnvVarPrefix (single source of truth)
pkg/env ← SetWithRestore (foundational env primitive, no policy)
↑
pkg/auth ← CreateAndAuthenticateManagerWithEnvOverrides (composes pkg/env + cfg.InitCliConfig + auth)
↑
pkg/mcp/client ← ScopedAuthProvider (thin MCP adapter, ~85 lines)
↑
cmd/mcp/client, ← one-line consumers, zero auth-factory code
cmd/ai
```
test coverage
100% on every new function:
references
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests