fix: address CodeRabbit review on PR #2883

- Deep-merge pro: for custom component types (container/emulator), matching
  the built-in types' m.Merge pattern instead of a shallow top-level-key
  copy that let a component-local pro.pull_request with one activity wipe
  out other globally-configured activities under the same key.
- Dedupe the 'settings.pro is deprecated' debug notice to once per process
  instead of once per LoadConfig call (which fires repeatedly per CLI
  invocation), regenerating the golden snapshots that captured the noise.
- Fix skill/doc wording: attach "deprecated alias" to settings.pro instead
  of the new pro: form; correct git_sts.git_config_mode/revoke_on_exit
  override location (github/sts integration spec, not an identity spec);
  split the atmos-pro skill's precedence note into its two real behaviors
  (per-field fallback for atmos.yaml, whole-block precedence for
  stack/component pro:).
- Expand pro.go migration test coverage to every ProSettings field and add
  an ATMOS_PRO_* env-var precedence test; add a disabled-Pro test case for
  the CI "test" template scenario.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: osterman

agent-skills/skills/atmos-modernization/SKILL.md

@@ -27,8 +27,8 @@ the umbrella term for replacing legacy patterns with supported, current patterns
 | Large inline workflow/custom-command shell scripts, repeated `echo`, shell loops, ad hoc sleeps | Native step types such as `atmos`, `toast`, `table`, `parallel`, `matrix`, `wait`, `container`, `emulator`, and `http` |
 | Hand-rolled scheduled drift GitHub Actions | Atmos Pro drift detection |
 | `cloudposse/github-action-atmos-terraform-drift-*` | `pro.drift_detection` plus `atmos terraform plan --upload-status` |
-| `settings.pro` in `atmos.yaml` (CLI connection config: `base_url`/`token`/`workspace_id`/etc.) | Top-level `pro:` in `atmos.yaml` (deprecated alias, still works) |
-| `settings.pro.*` per component/stack (`enabled`/`drift_detection`/`pull_request`/`release`/`merge_group`) | Top-level `pro:` component section, a sibling of `vars:`/`metadata:`/`settings:` (deprecated alias, still works) |
+| `settings.pro` in `atmos.yaml` (CLI connection config: `base_url`/`token`/`workspace_id`/etc.) | Top-level `pro:` in `atmos.yaml` (`settings.pro` remains supported as a deprecated alias) |
+| `settings.pro.*` per component/stack (`enabled`/`drift_detection`/`pull_request`/`release`/`merge_group`) | Top-level `pro:` component section, a sibling of `vars:`/`metadata:`/`settings:` (`settings.pro` remains supported as a deprecated alias) |
 | Secret values through raw store calls | Declared `secrets.vars` plus `!secret` |
 | Legacy hook event spelling | Modern dotted lifecycle events such as `after.terraform.plan` |
 | Static GitHub tokens in URLs | Atmos Auth `github/sts` through Atmos Pro |

agent-skills/skills/atmos-pro/SKILL.md

@@ -155,6 +155,11 @@ Pro mint short-lived GitHub App installation tokens in CI without storing long-l
   `atmos describe affected --upload` runs on `merge_group` events.
 - If drift is not dispatched, verify `pro.enabled`, `pro.drift_detection.enabled`,
   and that the instance appears in `atmos list instances --upload`.
-- If a config sets both `pro:` and `settings.pro:`, the top-level `pro:` block wins outright — it
-  is not merged with `settings.pro:`. A stray `settings.pro:` left behind after a partial migration
-  can silently override an intended `pro:` change; check both.
+- If `atmos.yaml` sets both `pro:` and `settings.pro:`, each field falls back independently: a
+  `pro.<field>` set at the top level wins; a field left unset there still falls back to
+  `settings.pro.<field>`. A stray `settings.pro:` field left behind after a partial migration can
+  still take effect for any field the top-level `pro:` block leaves unset; check both.
+- If a stack/component config sets both `pro:` and `settings.pro:`, the top-level `pro:` block wins
+  outright as a whole block — it is not merged field-by-field with `settings.pro:`. A stray
+  `settings.pro:` left behind after a partial migration is ignored entirely once a local `pro:`
+  block exists on the same component; check both.

internal/exec/stack_processor_process_stacks.go

@@ -1372,15 +1372,19 @@ func ProcessStackConfig(
 			if len(componentSettings) > 0 {
 				componentMap[cfg.SettingsSectionName] = componentSettings
 			}
-			// Merge global pro into component pro.
-			componentPro := map[string]any{}
-			for k, v := range globalProSection {
-				componentPro[k] = v
-			}
+			// Merge global pro into component pro. Uses a deep merge (mirroring the
+			// built-in component types above) rather than a shallow top-level-key copy,
+			// because `pro:` nests multi-level maps (e.g. `pull_request.opened` vs
+			// `pull_request.synchronize`) -- a shallow copy would let a component-local
+			// `pro.pull_request` with only one activity silently wipe out other
+			// globally-configured activities under the same key.
+			var componentLocalPro map[string]any
 			if pro, ok := componentMap[cfg.ProSectionName].(map[string]any); ok {
-				for k, v := range pro {
-					componentPro[k] = v
-				}
+				componentLocalPro = pro
+			}
+			componentPro, err := m.Merge(atmosConfig, []map[string]any{globalProSection, componentLocalPro})
+			if err != nil {
+				return nil, err
 			}
 			if len(componentPro) > 0 {
 				componentMap[cfg.ProSectionName] = componentPro

internal/exec/stack_processor_process_stacks_test.go

@@ -1394,6 +1394,92 @@ func TestProcessStackConfig_CustomComponentTypeGlobalMetadata(t *testing.T) {
 	assert.Equal(t, []any{"prod"}, overrideMetadata["tags"], "custom component must still inherit global keys it doesn't override locally")
 }
 
+// TestProcessStackConfig_CustomComponentTypeProDeepMerge verifies that a
+// custom (non-built-in) component type's own `pro:` block is deep-merged
+// with the global stack `pro:` section rather than replacing it wholesale.
+// `pro:` nests multi-level maps (e.g. `pull_request.opened` vs
+// `pull_request.synchronize`), so a shallow top-level-key merge would let a
+// component-local `pro.pull_request` with only one activity silently wipe
+// out other globally-configured activities under the same key.
+func TestProcessStackConfig_CustomComponentTypeProDeepMerge(t *testing.T) {
+	atmosConfig := &schema.AtmosConfiguration{}
+
+	config := map[string]any{
+		cfg.ProSectionName: map[string]any{
+			"enabled": true,
+			"pull_request": map[string]any{
+				"opened": map[string]any{
+					"workflows": map[string]any{"atmos-terraform-plan.yaml": map[string]any{}},
+				},
+			},
+		},
+		cfg.ComponentsSectionName: map[string]any{
+			"container": map[string]any{
+				"no-local-pro": map[string]any{
+					cfg.VarsSectionName: map[string]any{"name": "no-local-pro"},
+				},
+				"local-override": map[string]any{
+					cfg.VarsSectionName: map[string]any{"name": "local-override"},
+					cfg.ProSectionName: map[string]any{
+						"pull_request": map[string]any{
+							"synchronize": map[string]any{
+								"workflows": map[string]any{"atmos-terraform-plan.yaml": map[string]any{}},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	result, err := ProcessStackConfig(
+		atmosConfig,
+		"/test/stacks",
+		"/test/terraform",
+		"/test/helmfile",
+		"/test/packer",
+		"/test/ansible",
+		"test-stack.yaml",
+		config,
+		false,
+		false,
+		"",
+		map[string]map[string][]string{},
+		map[string]map[string]any{},
+		false,
+	)
+	require.NoError(t, err)
+	require.NotNil(t, result)
+
+	components, ok := result[cfg.ComponentsSectionName].(map[string]any)
+	require.True(t, ok, "components section should exist")
+	containerSection, ok := components["container"].(map[string]any)
+	require.True(t, ok, "container components should be present")
+
+	// A component with no local `pro:` must simply inherit the global block.
+	noLocalPro, ok := containerSection["no-local-pro"].(map[string]any)
+	require.True(t, ok, "no-local-pro component should exist")
+	noLocalProSection, ok := noLocalPro[cfg.ProSectionName].(map[string]any)
+	require.True(t, ok, "no-local-pro must have a pro section merged in from global, got: %v", noLocalPro[cfg.ProSectionName])
+	assert.Equal(t, true, noLocalProSection["enabled"])
+	pullRequest, ok := noLocalProSection["pull_request"].(map[string]any)
+	require.True(t, ok, "pull_request must be present")
+	assert.Contains(t, pullRequest, "opened", "global pull_request.opened must be inherited")
+
+	// A component with a local `pro.pull_request.synchronize` must retain the
+	// global `pro.pull_request.opened` -- not replace the whole `pull_request`
+	// map.
+	localOverride, ok := containerSection["local-override"].(map[string]any)
+	require.True(t, ok, "local-override component should exist")
+	overrideProSection, ok := localOverride[cfg.ProSectionName].(map[string]any)
+	require.True(t, ok, "local-override must have a pro section, got: %v", localOverride[cfg.ProSectionName])
+	assert.Equal(t, true, overrideProSection["enabled"], "component must still inherit global pro.enabled")
+	overridePullRequest, ok := overrideProSection["pull_request"].(map[string]any)
+	require.True(t, ok, "pull_request must be present")
+	assert.Contains(t, overridePullRequest, "opened", "global pull_request.opened must survive the deep merge")
+	assert.Contains(t, overridePullRequest, "synchronize", "component-local pull_request.synchronize must be preserved")
+}
+
 // componentHooks extracts the merged hooks section for a terraform component
 // from a ProcessStackConfig result. It fails the test if the component or its
 // hooks section is missing, so inheritance assertions read cleanly.

pkg/ci/plugins/terraform/template_test.go

@@ -500,6 +500,31 @@ func TestTemplateRendering(t *testing.T) {
 				"<sub>__Atmos Pro__ is enabled.</sub>",
 			},
 		},
+		{
+			name:         "test with pro disabled shows silver badge and disabled footer",
+			templateName: "test",
+			context: &TerraformTemplateContext{
+				TemplateContext: &plugin.TemplateContext{
+					Component:     "vpc",
+					ComponentType: "terraform",
+					Stack:         "dev",
+					Command:       "test",
+					Result:        &plugin.OutputResult{ExitCode: 0},
+				},
+				ProEnabled: false,
+				TestResult: &plugin.TerraformTestOutputData{Total: 1, Pass: 1},
+			},
+			wantContains: []string{
+				"PRO-DISABLED-silver",
+				"https://atmos-pro.com",
+				"<sub>__Atmos Pro__ is disabled.</sub>",
+			},
+			wantNotContains: []string{
+				"PRO-ENABLED",
+				"is enabled.",
+				"/dashboard",
+			},
+		},
 	}
 
 	fs := defaultTemplates

pkg/config/load.go

@@ -12,6 +12,7 @@ import (
 	"runtime"
 	"slices"
 	"strings"
+	"sync"
 
 	"github.com/go-viper/mapstructure/v2"
 	"github.com/joho/godotenv"
@@ -546,18 +547,54 @@ func LoadConfig(configAndStacksInfo *schema.ConfigAndStacksInfo) (schema.AtmosCo
 	return atmosConfig, nil
 }
 
+// proDeprecationWarnMu guards proDeprecationWarned.
+var proDeprecationWarnMu sync.Mutex
+
+// proDeprecationWarned tracks whether the `settings.pro` deprecation notice has already been
+// logged in this process. See warnProSettingsDeprecatedOnce for why this is a manually-gated
+// latch rather than a sync.Once.
+var proDeprecationWarned bool
+
+// warnProSettingsDeprecatedOnce logs the `settings.pro` deprecation notice at most once per
+// process. LoadConfig (and therefore resolveProSettings) runs many times within a single CLI
+// invocation -- once per InitCliConfig call, and InitCliConfig is called repeatedly during stack
+// and component processing -- so without a guard the same warning is emitted on every debug-level
+// stderr capture, once per LoadConfig call.
+//
+// This intentionally does NOT use sync.Once: the very first LoadConfig call in a command often
+// runs before the CLI has synced --verbose/--logs-level into the logger, so log.Debug is a no-op
+// at that point. A plain sync.Once would still consume its single call there, silently swallowing
+// the notice for the rest of the process even once debug logging becomes active. Checking
+// log.GetLevel() first, and only latching after the message was actually eligible to print,
+// avoids that trap.
+func warnProSettingsDeprecatedOnce() {
+	if log.GetLevel() > log.DebugLevel {
+		return
+	}
+
+	proDeprecationWarnMu.Lock()
+	defer proDeprecationWarnMu.Unlock()
+	if proDeprecationWarned {
+		return
+	}
+	proDeprecationWarned = true
+	log.Debug("'settings.pro' is deprecated, use 'pro' instead. See: https://atmos.tools/cli/configuration/settings/pro")
+}
+
 // resolveProSettings resolves the deprecated `settings.pro` path into the top-level `pro` field.
 // The top-level field wins field-by-field whenever both are set; the deprecated path is otherwise
 // used as a fallback. Emits a deprecation notice whenever `settings.pro` is present, regardless of
 // which value wins, mirroring the existing 'settings.depends_on' deprecation notice
-// (internal/exec/describe_affected_components.go).
+// (internal/exec/describe_affected_components.go). The notice itself is logged at most once per
+// process via warnProSettingsDeprecatedOnce to avoid flooding debug output across repeated
+// LoadConfig calls.
 func resolveProSettings(atmosConfig *schema.AtmosConfiguration) {
 	legacy := atmosConfig.Settings.Pro //nolint:staticcheck // Deliberate read of the deprecated field to implement the fallback itself.
 	if legacy == (schema.ProSettings{}) {
 		return
 	}
 
-	log.Debug("'settings.pro' is deprecated, use 'pro' instead. See: https://atmos.tools/cli/configuration/settings/pro")
+	warnProSettingsDeprecatedOnce()
 
 	pro := &atmosConfig.Pro
 	if pro.BaseURL == "" {

pkg/config/load_test.go

@@ -2008,31 +2008,106 @@ func TestResolveProSettings(t *testing.T) {
 	})
 
 	t.Run("legacy only, falls back field by field", func(t *testing.T) {
+		revokeOnExit := true
 		cfg := &schema.AtmosConfiguration{
 			Settings: schema.AtmosSettings{Pro: schema.ProSettings{
-				WorkspaceID: "legacy-ws",
-				Token:       "legacy-token",
+				WorkspaceID:     "legacy-ws",
+				Token:           "legacy-token",
+				Endpoint:        "legacy/api",
+				MaxPayloadBytes: 1024,
+				GitHubHeadRef:   "legacy-branch",
+				GithubOIDC: schema.GithubOIDCSettings{
+					RequestURL:   "https://legacy.example.com/oidc",
+					RequestToken: "legacy-oidc-token",
+				},
+				GitSTS: schema.GitSTSSettings{
+					GitConfigMode: "file",
+					RevokeOnExit:  &revokeOnExit,
+				},
 			}},
 		}
 		resolveProSettings(cfg)
 		assert.Equal(t, "legacy-ws", cfg.Pro.WorkspaceID)
 		assert.Equal(t, "legacy-token", cfg.Pro.Token)
+		assert.Equal(t, "legacy/api", cfg.Pro.Endpoint)
+		assert.Equal(t, 1024, cfg.Pro.MaxPayloadBytes)
+		assert.Equal(t, "legacy-branch", cfg.Pro.GitHubHeadRef)
+		assert.Equal(t, "https://legacy.example.com/oidc", cfg.Pro.GithubOIDC.RequestURL)
+		assert.Equal(t, "legacy-oidc-token", cfg.Pro.GithubOIDC.RequestToken)
+		assert.Equal(t, "file", cfg.Pro.GitSTS.GitConfigMode)
+		require.NotNil(t, cfg.Pro.GitSTS.RevokeOnExit)
+		assert.True(t, *cfg.Pro.GitSTS.RevokeOnExit)
 	})
 
 	t.Run("both set, top-level wins per field", func(t *testing.T) {
+		legacyRevokeOnExit := true
+		topRevokeOnExit := false
 		cfg := &schema.AtmosConfiguration{
-			Pro: schema.ProSettings{WorkspaceID: "top-ws"},
+			Pro: schema.ProSettings{
+				WorkspaceID: "top-ws",
+				GitSTS:      schema.GitSTSSettings{GitConfigMode: "env", RevokeOnExit: &topRevokeOnExit},
+			},
 			Settings: schema.AtmosSettings{Pro: schema.ProSettings{
-				WorkspaceID: "legacy-ws",
-				Token:       "legacy-token",
+				WorkspaceID:     "legacy-ws",
+				Token:           "legacy-token",
+				Endpoint:        "legacy/api",
+				MaxPayloadBytes: 1024,
+				GitHubHeadRef:   "legacy-branch",
+				GithubOIDC:      schema.GithubOIDCSettings{RequestURL: "https://legacy.example.com/oidc"},
+				GitSTS:          schema.GitSTSSettings{GitConfigMode: "file", RevokeOnExit: &legacyRevokeOnExit},
 			}},
 		}
 		resolveProSettings(cfg)
 		assert.Equal(t, "top-ws", cfg.Pro.WorkspaceID, "top-level value must not be overwritten by the legacy fallback")
 		assert.Equal(t, "legacy-token", cfg.Pro.Token, "fields unset at top level still fall back to the legacy value")
+		assert.Equal(t, "legacy/api", cfg.Pro.Endpoint, "Endpoint unset at top level falls back to legacy")
+		assert.Equal(t, 1024, cfg.Pro.MaxPayloadBytes, "MaxPayloadBytes unset at top level falls back to legacy")
+		assert.Equal(t, "legacy-branch", cfg.Pro.GitHubHeadRef, "GitHubHeadRef unset at top level falls back to legacy")
+		assert.Equal(t, "https://legacy.example.com/oidc", cfg.Pro.GithubOIDC.RequestURL, "GithubOIDC unset at top level falls back to legacy")
+		assert.Equal(t, "env", cfg.Pro.GitSTS.GitConfigMode, "GitSTS set at top level must not be overwritten by the legacy fallback")
+		require.NotNil(t, cfg.Pro.GitSTS.RevokeOnExit)
+		assert.False(t, *cfg.Pro.GitSTS.RevokeOnExit, "GitSTS set at top level must not be overwritten by the legacy fallback")
 	})
 }
 
+// TestLoadConfig_ProSettingsEnvVarPrecedence verifies that ATMOS_PRO_* environment variables
+// override both the top-level `pro.*` and the deprecated `settings.pro.*` config-file paths,
+// regardless of which one a project has set in atmos.yaml.
+func TestLoadConfig_ProSettingsEnvVarPrecedence(t *testing.T) {
+	tests := []struct {
+		name    string
+		content string
+	}{
+		{
+			name: "env overrides top-level pro",
+			content: "base_path: .\n" +
+				"pro:\n" +
+				"  token: file-pro-token\n",
+		},
+		{
+			name: "env overrides deprecated settings.pro",
+			content: "base_path: .\n" +
+				"settings:\n" +
+				"  pro:\n" +
+				"    token: file-settings-pro-token\n",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(AtmosProTokenEnvVarName, "env-token")
+			tempDir := t.TempDir()
+			configPath := createTestConfig(t, tempDir, tt.content)
+			configInfo := &schema.ConfigAndStacksInfo{
+				AtmosConfigFilesFromArg: []string{configPath},
+			}
+			cfg, err := LoadConfig(configInfo)
+			require.NoError(t, err)
+			assert.Equal(t, "env-token", cfg.Pro.Token, "ATMOS_PRO_TOKEN must override the config-file value")
+		})
+	}
+}
+
 func TestAutoProvisionWorkdirForOutputsEnvVarBinding(t *testing.T) {
 	t.Setenv("ATMOS_COMPONENTS_TERRAFORM_AUTO_PROVISION_WORKDIR_FOR_OUTPUTS", "false")
 	tempDir := t.TempDir()