pkg/store/artifactory_store.go (1)

297-297: Use compound assignment for string concatenation.

For consistency with Go best practices, consider using compound assignment.

-		filePath = filePath + ".json"
+		filePath += ".json"

internal/exec/yaml_func_store_getkey.go (1)

64-107: Address function length concern from static analysis.

This function exceeds the 60-line limit flagged in past reviews. Consider extracting helper functions to improve maintainability.

Extract the store retrieval and value processing into separate functions:

func getStoreFromConfig(atmosConfig schema.AtmosConfiguration, storeName string) (schema.Store, error) {
	store := atmosConfig.Stores[storeName]
	if store == nil {
		return nil, fmt.Errorf("store not found: %s", storeName)
	}
	return store, nil
}

func processStoreValue(value interface{}, query string, atmosConfig *schema.AtmosConfiguration) (interface{}, error) {
	if query == "" {
		return value, nil
	}
	return u.EvaluateYqExpression(atmosConfig, value, query)
}

internal/exec/yaml_func_store_getkey.go (1)

32-46: Improve quote handling in parameter parsing.

The current quote removal approach could strip legitimate quotes from values. Consider using a more robust parsing method.

-			v1 := strings.Trim(pipeParts[0], `"'`) // Remove surrounding quotes if present
-			v2 := strings.Trim(pipeParts[1], `"'`)
+			v1 := strings.TrimSpace(pipeParts[0])
+			v2 := strings.TrimSpace(pipeParts[1])
+			// Only remove quotes if they wrap the entire value
+			if (strings.HasPrefix(v2, `"`) && strings.HasSuffix(v2, `"`)) || 
+			   (strings.HasPrefix(v2, `'`) && strings.HasSuffix(v2, `'`)) {
+				v2 = v2[1 : len(v2)-1]
+			}

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

pkg/store/aws_ssm_param_store.go (2)

280-290: Implement role assumption for security consistency.

The GetKey method bypasses role assumption unlike the existing Get method, creating a security inconsistency. Both methods should follow the same authentication pattern.

Apply the same role assumption pattern used in the Get method:

+	// Assume read role if specified
+	cfg, err := s.assumeRole(ctx, s.readRoleArn)
+	if err != nil {
+		return nil, fmt.Errorf("failed to assume read role: %w", err)
+	}
+
+	// Use the same client if no role was assumed
+	client := s.client
+	if s.readRoleArn != nil {
+		// Create SSM client with assumed role if applicable
+		if s.newSSMClient != nil {
+			client = s.newSSMClient(*cfg)
+		} else {
+			client = ssm.NewFromConfig(*cfg)
+		}
+	}
+
-	resp, err := s.client.GetParameter(context.TODO(), &ssm.GetParameterInput{
+	resp, err := client.GetParameter(context.TODO(), &ssm.GetParameterInput{

---

298-300: Fix the nilerr static analysis issue.

The linter correctly identifies that when JSON unmarshaling fails, the error is not nil but you're returning nil instead of the error.

Apply this fix to properly handle the unmarshaling error:

-	if unmarshalErr := json.Unmarshal([]byte(*resp.Parameter.Value), &result); unmarshalErr != nil {
-		// If JSON unmarshaling fails, return as string.
-		return *resp.Parameter.Value, nil
-	}
+	if err := json.Unmarshal([]byte(*resp.Parameter.Value), &result); err != nil {
+		// If JSON unmarshaling fails, return as string.
+		return *resp.Parameter.Value, nil
+	}

internal/exec/yaml_func_store_getkey.go (1)

64-107: Fix undefined variable causing compilation errors.

The function variable is referenced multiple times but never defined, causing compilation failures.

Define the function variable at the start of the method:

 func processTagStoreGetKey(atmosConfig schema.AtmosConfiguration, input string, currentStack string) any {
+	function := u.AtmosYamlFuncStoreGetKey
 	log.Debug("Executing Atmos YAML function", function, input)

internal/exec/yaml_func_store_getkey.go (2)

20-20: Fix comment formatting.

Comment should end with a period per style guidelines.

-// parseStoreGetKeyInput parses the input string and returns storeGetKeyParams
+// parseStoreGetKeyInput parses the input string and returns storeGetKeyParams.

---

64-64: Pass heavy parameter by pointer.

The atmosConfig parameter is 6KB+ and should be passed by pointer for performance.

-func processTagStoreGetKey(atmosConfig schema.AtmosConfiguration, input string, currentStack string) any {
+func processTagStoreGetKey(atmosConfig *schema.AtmosConfiguration, input string, currentStack string) any {

Update the method body to use pointer dereferencing:

-	store := atmosConfig.Stores[retParams.storeName]
+	store := atmosConfig.Stores[retParams.storeName]

-	res, err = u.EvaluateYqExpression(&atmosConfig, value, retParams.query)
+	res, err = u.EvaluateYqExpression(atmosConfig, value, retParams.query)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/yaml_func_store_getkey.go (2)

72-73: Fix undefined variable causing compilation error

The variable function is undefined, causing compilation failures as noted in past reviews.

Define the missing variable at the function start:

func processTagStoreGetKey(atmosConfig *schema.AtmosConfiguration, input string, currentStack string) any {
+	function := u.AtmosYamlFuncStoreGetKey
	log.Debug("Executing Atmos YAML function", function, input)

---

76-85: Fix additional undefined variables

The variable invalidYamlFuncMsg is also undefined and needs to be defined or replaced with a string literal.

	str, err := getStringAfterTag(input, u.AtmosYamlFuncStoreGetKey)
	if err != nil {
-		return fmt.Sprintf("%s: %s", invalidYamlFuncMsg, input)
+		return fmt.Sprintf("invalid YAML function: %s", input)
	}

	// Parse the input parameters.
	retParams, err := parseStoreGetKeyInput(str)
	if err != nil {
-		log.Error(invalidYamlFuncMsg, function, input, "error", err)
-		return fmt.Sprintf("%s: %s", invalidYamlFuncMsg, input)
+		log.Error("invalid YAML function", "function", function, "input", input, "error", err)
+		return fmt.Sprintf("invalid YAML function: %s", input)
	}

Apply similar fixes to lines 90, 100, and 109 where function is used without the undefined variable fix.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/yaml_func_store_getkey.go (1)

73-115: Fix undefined variables causing compilation errors.

The variables function and invalidYamlFuncMsg are used but not defined, which will cause compilation failures.

Define these variables at the function level:

 func processTagStoreGetKey(atmosConfig *schema.AtmosConfiguration, input string, currentStack string) any {
+	function := u.AtmosYamlFuncStoreGetKey
+	invalidYamlFuncMsg := "invalid YAML function"
+	
 	log.Debug("Executing Atmos YAML function", function, input)

internal/exec/yaml_func_store_getkey.go (1)

72-72: Remove unused parameter.

The currentStack parameter is not used in this function and should be removed for cleaner code.

-func processTagStoreGetKey(atmosConfig *schema.AtmosConfiguration, input string, currentStack string) any {
+func processTagStoreGetKey(atmosConfig *schema.AtmosConfiguration, input string) any {

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

pkg/store/artifactory_store_test.go (1)

34-69: Critical: Mock content writer can override test-provided file content.

MockArtifactoryClient.DownloadFiles writes a default JSON payload on success, regardless of any file created in test-run hooks. In TestArtifactoryStore_GetKeyDirect you write custom content via .Run(), but it will be overwritten by this default write, causing false negatives.

Guard against overwriting if the file already exists.

Apply this diff:

 func (m *MockArtifactoryClient) DownloadFiles(params ...services.DownloadParams) (int, int, error) {
   args := m.Called(params[0])
   totalDownloaded := args.Int(0)
   totalFailed := args.Int(1)
   err := args.Error(2)
   // First check: if there's an error, return immediately.
   if err != nil {
     return totalDownloaded, totalFailed, err
   }

   // Second check: if there are failures, return without creating files.
   if totalFailed > 0 {
     return totalDownloaded, totalFailed, nil
   }

   // Third check: if no downloads, return without creating files.
   if totalDownloaded == 0 {
     return totalDownloaded, totalFailed, nil
   }

   // Only proceed with file creation for successful cases
   targetDir := params[0].Target
   filename := filepath.Base(params[0].Pattern)

   if err := os.MkdirAll(targetDir, 0o755); err != nil {
     return 0, 0, err
   }

-  data := []byte(`{"test":"value"}`)
   fullPath := filepath.Join(targetDir, filename)
-  if err := os.WriteFile(fullPath, data, 0o600); err != nil {
-    return 0, 0, err
-  }
+  // If test code already created the file (e.g., via .Run()), don't overwrite it.
+  if _, statErr := os.Stat(fullPath); os.IsNotExist(statErr) {
+    data := []byte(`{"test":"value"}`)
+    if err := os.WriteFile(fullPath, data, 0o600); err != nil {
+      return 0, 0, err
+    }
+  }

   return totalDownloaded, totalFailed, nil
 }

pkg/store/google_secret_manager_store_test.go (2)

695-706: Use a matcher for proto requests to avoid brittle equality on pointer/struct internals.

Proto messages often carry hidden fields; matching the request by Name is more robust and mirrors earlier tests in this file.

Apply this diff to both branches:

-            if tt.mockError != nil {
-                mockClient.On("AccessSecretVersion", mock.Anything, &secretmanagerpb.AccessSecretVersionRequest{
-                    Name: expectedFullPath,
-                }).Return(nil, tt.mockError)
-            } else {
-                mockClient.On("AccessSecretVersion", mock.Anything, &secretmanagerpb.AccessSecretVersionRequest{
-                    Name: expectedFullPath,
-                }).Return(&secretmanagerpb.AccessSecretVersionResponse{
-                    Payload: &secretmanagerpb.SecretPayload{
-                        Data: tt.mockPayload,
-                    },
-                }, nil)
-            }
+            if tt.mockError != nil {
+                mockClient.On("AccessSecretVersion", mock.Anything,
+                    mock.MatchedBy(func(req *secretmanagerpb.AccessSecretVersionRequest) bool {
+                        return req.GetName() == expectedFullPath
+                    }),
+                ).Return(nil, tt.mockError)
+            } else {
+                mockClient.On("AccessSecretVersion", mock.Anything,
+                    mock.MatchedBy(func(req *secretmanagerpb.AccessSecretVersionRequest) bool {
+                        return req.GetName() == expectedFullPath
+                    }),
+                ).Return(&secretmanagerpb.AccessSecretVersionResponse{
+                    Payload: &secretmanagerpb.SecretPayload{
+                        Data: tt.mockPayload,
+                    },
+                }, nil)
+            }

---

639-647: Minor: Align error message expectation with the mapping behavior.

You’re setting the gRPC error message to "secret not found" while asserting "resource not found". If the implementation normalizes NotFound to "resource not found", this is fine. If not, consider asserting with errors.Is against a sentinel (e.g., ErrResourceNotFound) for stability, similar to other tests in this package.

pkg/store/azure_keyvault_store_test.go (1)

380-389: Prefer sentinel error checks over substring matches.

To keep tests resilient to message changes, assert with errors.Is against domain errors (e.g., ErrResourceNotFound, ErrPermissionDenied) if GetKey maps SDK errors to sentinels (as done in Get tests).

pkg/store/artifactory_store_test.go (1)

417-562: Optional: Add a case for "downloads=1, fails>0" to assert failure handling.

You already cover "error != nil" and "downloads=0". Adding "downloads>0, fails>0, err=nil" helps validate the fails path.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions

Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios

pkg/store/artifactory_store_test.go (6)

417-417: Fix lint: comment should end with a period.

golangci-lint flagged this. Add a trailing period.

-// Custom mock client for GetKey tests that can return different file contents
+// Custom mock client for GetKey tests that can return different file contents.

---

423-451: Solid mock; add a small guard to avoid potential panic on empty variadic args.

Defensive check to prevent params[0] panics if the signature is ever invoked without args. Purely a test helper, so low risk.

 func (m *MockArtifactoryClientForGetKey) DownloadFiles(params ...services.DownloadParams) (int, int, error) {
+	if len(params) == 0 {
+		return 0, 0, fmt.Errorf("no download params provided")
+	}
 	args := m.Called(params[0])
 	totalDownloaded := args.Int(0)
 	totalFailed := args.Int(1)
 	err := args.Error(2)
 	if err != nil {
 		return totalDownloaded, totalFailed, err
 	}

---

457-457: Rename test to reflect the method under test.

The test calls store.GetKey(...), not GetKeyDirect. Rename for clarity.

-func TestArtifactoryStore_GetKeyDirect(t *testing.T) {
+func TestArtifactoryStore_GetKey(t *testing.T) {

---

457-459: Optionally run the parent test in parallel.

This can speed up CI if file targets are isolated by the implementation.

If you adopt subtest parallelization (see next comment), also parallelize the parent:

-func TestArtifactoryStore_GetKey(t *testing.T) {
+func TestArtifactoryStore_GetKey(t *testing.T) {
+	t.Parallel()

Please verify that ArtifactoryStore.GetKey uses unique temp directories (e.g., os.MkdirTemp) to avoid cross-test interference before enabling this.

---

458-469: Prefer any over interface{} in tests.

Go 1.18+ alias improves readability.

-		expectedValue interface{}
+		expectedValue any

---

551-554: Parallelize subtests safely.

To avoid subtest data races when using t.Parallel(), rebind the loop variable and parallelize inside each t.Run.

-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

pkg/store/artifactory_store_test.go (1)

417-417: Doc comment should start with the type name and end with a period.

Keeps golangci-lint happy and matches Go doc conventions.

-// Custom mock client for GetKey tests that can return different file contents
+// MockArtifactoryClientForGetKey is a test mock that can return different file contents.

pkg/store/artifactory_store_test.go (2)

423-451: Defensive guard against empty params to avoid potential panic in mock.

Future changes could call DownloadFiles with no params; this avoids an index out-of-range.

 func (m *MockArtifactoryClientForGetKey) DownloadFiles(params ...services.DownloadParams) (int, int, error) {
+	if len(params) == 0 {
+		return 0, 0, fmt.Errorf("no download params provided")
+	}
 	args := m.Called(params[0])
 	totalDownloaded := args.Int(0)
 	totalFailed := args.Int(1)
 	err := args.Error(2)
 	if err != nil {
 		return totalDownloaded, totalFailed, err
 	}

---

457-590: Strengthen test coverage for edge cases and enable parallel subtests.

• Add a case for “fails > 0 with nil error” to ensure we still surface a “failed to download file” error.
• Run subtests in parallel for speed (this table test doesn’t mutate globals).

Add the missing test case:

@@
 		{
 			name:          "download error",
 			key:           "error-config",
 			prefix:        "configs",
 			mockDownloads: 0,
 			mockFails:     1,
 			mockError:     fmt.Errorf("network error"),
 			fileContent:   nil,
 			expectedValue: nil,
 			expectError:   true,
 			errorContains: "failed to download file",
 		},
+		{
+			name:          "failed download without error",
+			key:           "partial-failure",
+			prefix:        "configs",
+			mockDownloads: 0,
+			mockFails:     1,
+			mockError:     nil,
+			fileContent:   nil,
+			expectedValue: nil,
+			expectError:   true,
+			errorContains: "failed to download file",
+		},

Run each subtest in parallel:

@@
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()

Optional: you can also add a “no downloads, no error” case (mockDownloads=0, mockFails=0, mockError=nil) to assert a consistent error message.

pkg/store/google_secret_manager_store.go (5)

65-72: Drop redundant Close() after NewClient error

NewClient returns a nil client on error. The guard makes it safe, but the Close() adds noise and no value here.

  client, err := secretmanager.NewClient(ctx, clientOpts...)
  if err != nil {
-    // Close the client to prevent resource leaks
-    if client != nil {
-      _ = client.Close()
-    }
     return nil, fmt.Errorf(errWrapFormat, ErrCreateClient, err)
  }

---

282-285: Add doc comment for exported method GetKey

Exported methods must be documented (golangci-lint). Add a short docstring.

+// GetKey retrieves a value by secret ID or full resource name from Google Secret Manager.
+// If a prefix is configured and the key is a bare secret ID, the prefix is prepended.
 func (s *GSMStore) GetKey(key string) (interface{}, error) {

---

291-294: Use the existing separator constant instead of hardcoding "_"

Avoid magic strings. Use gsmKeySeparator for consistency.

-  if s.prefix != "" {
-    secretName = s.prefix + "_" + key
-  }
+  if s.prefix != "" {
+    secretName = s.prefix + gsmKeySeparator + key
+  }

---

288-299: Support full resource names and sanitize secret IDs

Allow passing a full GSM resource name (projects/.../secrets/.../versions/...), and normalize secret IDs by replacing "/" with the GSM separator when a bare ID is provided to prevent malformed paths.

-  // Use the key directly as the secret name
-  secretName := key
-
-  // If prefix is set, prepend it to the key
-  if s.prefix != "" {
-    secretName = s.prefix + "_" + key
-  }
-
-  // Construct the full secret name
-  fullSecretName := fmt.Sprintf("projects/%s/secrets/%s/versions/latest", s.projectID, secretName)
+  var fullSecretName string
+  // If key is a full resource name, use it verbatim
+  if strings.HasPrefix(key, "projects/") {
+    fullSecretName = key
+  } else {
+    // Treat key as a secret ID, optionally prefixed
+    secretName := key
+    if s.prefix != "" {
+      secretName = s.prefix + gsmKeySeparator + key
+    }
+    // Normalize illegal path separators in secret IDs
+    secretName = strings.ReplaceAll(secretName, "/", gsmKeySeparator)
+    // Construct the full secret resource name
+    fullSecretName = fmt.Sprintf("projects/%s/secrets/%s/versions/latest", s.projectID, secretName)
+  }

---

283-324: Add tests for full resource names and PermissionDenied mapping

Edge paths: full resource names (“projects/.../secrets/.../versions/...”) and permission-denied errors. Add table entries to assert both behaviors for GetKey.

I can draft test cases for these, including mocking GSMClient.AccessSecretVersion to return codes.PermissionDenied, and a success path with a full resource name. Want me to open a PR to augment pkg/store/google_secret_manager_store_test.go?

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

internal/exec/yaml_func_store.go (1)

41-54: Fix undefined identifiers in logging; use literal keys and message.

References to invalidYamlFuncMsg and variable function will not compile. Use a literal message and "function" as the key consistently.

Apply this diff:

-      if len(pipeParts) != 2 {
-        log.Error(invalidYamlFuncMsg, function, input, "invalid number of parameters after the pipe", len(pipeParts))
-        return fmt.Sprintf("%s: %s", invalidYamlFuncMsg, input)
-      }
+      if len(pipeParts) != 2 {
+        log.Error("invalid YAML function", "function", input, "reason", "invalid number of parameters after the pipe", "count", len(pipeParts))
+        return fmt.Sprintf("invalid YAML function: %s", input)
+      }
@@
-      default:
-        log.Error(invalidYamlFuncMsg, function, input, "invalid identifier after the pipe", v1)
-        return fmt.Sprintf("%s: %s", invalidYamlFuncMsg, input)
+      default:
+        log.Error("invalid YAML function", "function", input, "reason", "invalid identifier after the pipe", "identifier", v1)
+        return fmt.Sprintf("invalid YAML function: %s", input)
@@
-  if partsLength != 3 && partsLength != 4 {
-    log.Error(invalidYamlFuncMsg, function, input, "invalid number of parameters", partsLength)
-    return fmt.Sprintf("%s: %s", invalidYamlFuncMsg, input)
-  }
+  if partsLength != 3 && partsLength != 4 {
+    log.Error("invalid YAML function", "function", input, "reason", "invalid number of parameters", "count", partsLength)
+    return fmt.Sprintf("invalid YAML function: %s", input)
+  }

Also applies to: 62-64

internal/exec/yaml_func_store.go (1)

23-109: Function length exceeds linter threshold; consider extracting helpers.

processTagStore is lengthy and handles parsing, validation, store lookup, defaulting, and querying. Extracting parsing into helpers will drop complexity and satisfy function-length checks.

Happy to propose a small refactor if you want it in this PR.

internal/exec/yaml_func_store.go (1)

33-56: Optional: make pipe parsing robust to values with spaces (default/query).

strings.Fields forces exactly two tokens; quoted values with spaces (e.g., default "hello world") will be rejected. Consider parsing by prefix and capturing the remainder.

Apply this diff to the pipe parsing block:

-  // Default value and query
-  var defaultValue *string
-  var query string
-  if len(parts) > 1 {
-    // Expecting the format: default <value> or query <yq-expression>
-    for _, p := range parts[1:] {
-      pipeParts := strings.Fields(strings.TrimSpace(p))
-      if len(pipeParts) != 2 {
-        log.Error("invalid YAML function", "function", input, "reason", "invalid number of parameters after the pipe", "count", len(pipeParts))
-        return fmt.Sprintf("invalid YAML function: %s", input)
-      }
-      v1 := strings.Trim(pipeParts[0], `"'`)
-      v2 := strings.Trim(pipeParts[1], `"'`)
-      switch v1 {
-      case "default":
-        defaultValue = &v2
-      case "query":
-        query = v2
-      default:
-        log.Error("invalid YAML function", "function", input, "reason", "invalid identifier after the pipe", "identifier", v1)
-        return fmt.Sprintf("invalid YAML function: %s", input)
-      }
-    }
-  }
+  // Default value and query
+  var defaultValue *string
+  var query string
+  if len(parts) > 1 {
+    for _, raw := range parts[1:] {
+      p := strings.TrimSpace(raw)
+      switch {
+      case strings.HasPrefix(p, "default "):
+        v := strings.TrimSpace(strings.TrimPrefix(p, "default "))
+        v = strings.Trim(v, `"'`)
+        dv := v
+        defaultValue = &dv
+      case strings.HasPrefix(p, "query "):
+        v := strings.TrimSpace(strings.TrimPrefix(p, "query "))
+        v = strings.Trim(v, `"'`)
+        query = v
+      default:
+        log.Error("invalid YAML function", "function", input, "reason", "invalid pipe identifier", "payload", p)
+        return fmt.Sprintf("invalid YAML function: %s", input)
+      }
+    }
+  }

internal/exec/yaml_func_store_get.go (4)

63-67: Align error behavior on invalid pipe params with other YAML funcs.

Returning err.Error() emits internal sentinel text to the user and diverges from processTagStore which returns a stable "invalid YAML function: " string. Prefer consistent UX.

Apply this diff:

-    if err != nil {
-      return err.Error()
-    }
+    if err != nil {
+      // Keep UX consistent with other YAML functions.
+      return fmt.Sprintf("invalid YAML function: %s", input)
+    }

---

21-42: Optional: support spaces in default and query values.

extractPipeParams uses strings.Fields and enforces two tokens, which breaks for values with spaces (e.g., default "hello world" or complex YQ). Parse by prefix and capture the remainder.

Apply this diff:

-func extractPipeParams(parts []string, input string) (defaultValue *string, query string, err error) {
-  for _, p := range parts[1:] {
-    pipeParts := strings.Fields(strings.TrimSpace(p))
-    if len(pipeParts) != 2 {
-      log.Error("invalid YAML function", "function", input, "reason", "invalid number of parameters after the pipe", "count", len(pipeParts))
-      return nil, "", fmt.Errorf("%w: %s", ErrInvalidPipeParams, input)
-    }
-    v1 := strings.Trim(pipeParts[0], `"'`) // Remove surrounding quotes if present.
-    v2 := strings.Trim(pipeParts[1], `"'`)
-    switch v1 {
-    case "default":
-      defaultValue = &v2
-    case "query":
-      query = v2
-    default:
-      log.Error("invalid YAML function", "function", input, "reason", "invalid identifier after the pipe", "identifier", v1)
-      return nil, "", fmt.Errorf("%w: %s", ErrInvalidPipeIdentifier, input)
-    }
-  }
-  return defaultValue, query, nil
-}
+func extractPipeParams(parts []string, input string) (defaultValue *string, query string, err error) {
+  for _, raw := range parts[1:] {
+    p := strings.TrimSpace(raw)
+    switch {
+    case strings.HasPrefix(p, "default "):
+      v := strings.TrimSpace(strings.TrimPrefix(p, "default "))
+      v = strings.Trim(v, `"'`)
+      dv := v
+      defaultValue = &dv
+    case strings.HasPrefix(p, "query "):
+      v := strings.TrimSpace(strings.TrimPrefix(p, "query "))
+      v = strings.Trim(v, `"'`)
+      query = v
+    default:
+      log.Error("invalid YAML function", "function", input, "reason", "invalid pipe identifier", "payload", p)
+      return nil, "", fmt.Errorf("%w: %s", ErrInvalidPipeIdentifier, input)
+    }
+  }
+  return defaultValue, query, nil
+}

---

45-47: Nit: include the tag in the first debug line for symmetry.

Small consistency polish with other handlers.

Apply this diff:

-  log.Debug("Executing Atmos YAML function", "function", input)
+  log.Debug("Executing Atmos YAML function", "function", u.AtmosYamlFuncStoreGet, "input", input)

---

44-114: Coverage: add tests for invalid pipe cases and values with spaces.

Good happy-path flow. Add table tests covering:

• Unknown pipe identifier (e.g., | foo bar) -> returns "invalid YAML function: ..."
• default "value with spaces" and query with spaces or special chars
• partsLength != 2 (missing key) and missing store -> ErrStoreNotFound path

I can draft tests mirroring internal/exec/yaml_func_store_test.go’s table style targeting processTagStoreGet with miniredis.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to go.mod : Manage dependencies with Go modules

Learnt from: aknysh
PR: cloudposse/atmos#810
File: internal/exec/yaml_func_terraform_output.go:35-40
Timestamp: 2024-11-30T22:07:08.610Z
Learning: In the Go function `processTagTerraformOutput` in `internal/exec/yaml_func_terraform_output.go`, parameters cannot contain spaces. The code splits the input by spaces, and if the parameters contain spaces, `len(parts) != 3` will fail and show an error to the user.

Learnt from: aknysh
PR: cloudposse/atmos#863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.

Learnt from: aknysh
PR: cloudposse/atmos#820
File: cmd/list_components.go:53-54
Timestamp: 2024-12-05T22:33:40.955Z
Learning: In the Atmos CLI Go codebase, using `u.LogErrorAndExit` within completion functions is acceptable because it logs the error and exits the command execution.

Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.

Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:71-72
Timestamp: 2025-04-04T02:03:21.906Z
Learning: The codebase currently uses `log.Fatal` for error handling in library functions, which terminates the program. There is a plan to refactor this approach in a separate PR to improve API design by returning error messages instead of terminating execution.

Learnt from: aknysh
PR: cloudposse/atmos#820
File: cmd/list_stacks.go:55-56
Timestamp: 2024-12-05T22:33:54.807Z
Learning: In the atmos project, the `u.LogErrorAndExit` function logs the error and exits the command execution appropriately within flag completion functions.

Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.

Learnt from: samtholiya
PR: cloudposse/atmos#1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.

Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.

Learnt from: Listener430
PR: cloudposse/atmos#934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.

Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use meaningful error messages

Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Provide clear error messages to users

pkg/store/aws_ssm_param_store.go (1)

292-299: Mirror decryption semantics in GetKey()

Same issue here; ensure decryption for SecureString parameters retrieved by arbitrary key.

Apply:

- output, err := client.GetParameter(ctx, &ssm.GetParameterInput{
-  Name: aws.String(paramName),
- })
+ output, err := client.GetParameter(ctx, &ssm.GetParameterInput{
+  Name:           aws.String(paramName),
+  WithDecryption: aws.Bool(true),
+ })

pkg/store/aws_ssm_param_store.go (1)

235-242: Always decrypt SecureString in Get()

Without WithDecryption=true, SecureString values won’t be returned in plaintext. This breaks parity with other stores and surprises users of !store.get.

Apply:

- output, err := client.GetParameter(ctx, &ssm.GetParameterInput{
-  Name: aws.String(paramName),
- })
+ output, err := client.GetParameter(ctx, &ssm.GetParameterInput{
+  Name:           aws.String(paramName),
+  WithDecryption: aws.Bool(true),
+ })

pkg/store/aws_ssm_param_store_test.go (1)

681-911: Add test cases for leading-slash keys and prefix joining

Cover edge cases to prevent accidental "//" in paths and to ensure we always start with "/".

Suggested additions:

• key starts with "/" and prefix set (expect "/prefix/your/key" without double slash)
• key starts with "/" and no prefix (expect "/your/key")
• key without "/" and no prefix (expect "/your/key")

pkg/store/aws_ssm_param_store.go (1)

265-273: Normalize prefix/key joining to avoid double slashes

If prefix ends with "/" and/or key starts with "/", current logic can yield "//". Normalize both ends before constructing the path.

Apply:

- // If the prefix is set, prepend it to the key
- if s.prefix != "" {
-  paramName = s.prefix + "/" + key
- }
-
- // Ensure the parameter name starts with "/" for SSM
- if !strings.HasPrefix(paramName, "/") {
-  paramName = "/" + paramName
- }
+ // Normalize and join prefix/key, ensure it starts with "/"
+ k := strings.TrimPrefix(key, "/")
+ if s.prefix != "" {
+   p := strings.TrimSuffix(s.prefix, "/")
+   paramName = "/" + p + "/" + k
+ } else {
+   paramName = "/" + k
+ }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.

Learnt from: samtholiya
PR: cloudposse/atmos#959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.

website/docs/core-concepts/stacks/yaml-functions/store.mdx (1)

119-121: Fix incorrect reference to the function name

This section is for the !store YAML function but references !terraform.output.

Apply this diff:

-There are multiple ways you can specify the Atmos stack parameter in the `!terraform.output` function.
+There are multiple ways you can specify the Atmos stack parameter in the `!store` function.

website/docs/core-concepts/stacks/yaml-functions/store.get.mdx (1)

19-24: Nice: usage phrasing matches prior suggestion

The usage statement clearly communicates two parameters plus optional default and query, aligning with earlier feedback.

website/docs/core-concepts/stacks/yaml-functions/store.mdx (1)

13-13: Tighten grammar and flow in the intro sentence

The phrasing is awkward. Suggest reordering and adding a comma after e.g.

Apply this diff:

- (e.g. SSM Parameter Store, Artifactory, Redis, etc.) for an Atmos component in a stack into Atmos stack manifests.
+ (e.g., SSM Parameter Store, Artifactory, Redis, etc.) and inject them into Atmos stack manifests for an Atmos component in a stack.

website/docs/core-concepts/stacks/yaml-functions/yaml-functions.mdx (2)

41-41: End the bullet sentence with a period

Minor formatting/punctuation polish for consistency with other bullets.

- - The [__`!terraform.state`__](/core-concepts/stacks/yaml-functions/terraform.state) YAML function reads outputs **directly from the configured Terraform or OpenTofu backend**, without initializing providers or running Terraform — it's **very fast** and currently supports [S3 and local backends](/core-concepts/components/terraform/backends) for accessing [remote state](/core-concepts/share-data/remote-state)
+ - The [__`!terraform.state`__](/core-concepts/stacks/yaml-functions/terraform.state) YAML function reads outputs **directly from the configured Terraform or OpenTofu backend**, without initializing providers or running Terraform — it's **very fast** and currently supports [S3 and local backends](/core-concepts/components/terraform/backends) for accessing [remote state](/core-concepts/share-data/remote-state).

---

47-52: Add terminal punctuation to the new !store.get bullet

Keeps list style consistent and improves readability.

- - The [__`!store.get`__](/core-concepts/stacks/yaml-functions/store.get) YAML function allows retrieving arbitrary keys
-   directly from a [store](/core-concepts/projects/configuration/stores)
-   without following the Atmos stack/component/key naming convention.
-   This is useful for accessing values stored by external systems
-   or for retrieving global configuration that doesn't belong to a specific component
+ - The [__`!store.get`__](/core-concepts/stacks/yaml-functions/store.get) YAML function allows retrieving arbitrary keys
+   directly from a [store](/core-concepts/projects/configuration/stores)
+   without following the Atmos stack/component/key naming convention.
+   This is useful for accessing values stored by external systems
+   or for retrieving global configuration that doesn't belong to a specific component.

website/docs/core-concepts/stacks/yaml-functions/store.get.mdx (2)

51-51: Add a period to complete the sentence

Minor punctuation fix.

-    <dd>The exact key name or path to retrieve from the store. This is the literal key as it exists in the store, not constructed from stack/component/key patterns</dd>
+    <dd>The exact key name or path to retrieve from the store. This is the literal key as it exists in the store, not constructed from stack/component/key patterns.</dd>

---

100-114: Consider adding an Artifactory example for parity with other stores

You cover Redis, SSM, Azure KV, GSM. An Artifactory example would round out the set and help users unfamiliar with its key conventions.

I can draft an Artifactory example snippet if you share the common key/path patterns you expect users to retrieve.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: aknysh
PR: cloudposse/atmos#810
File: website/docs/core-concepts/stacks/yaml-functions/terraform.output.mdx:0-0
Timestamp: 2024-12-03T04:01:16.446Z
Learning: In the `terraform.output.mdx` documentation file (`website/docs/core-concepts/stacks/yaml-functions/terraform.output.mdx`), the cache invalidation and cache scope behavior for the `!terraform.output` function are already described.

Learnt from: aknysh
PR: cloudposse/atmos#0
File: :0-0
Timestamp: 2025-01-19T22:30:27.600Z
Learning: The Atmos YAML function `!include` allows downloading local or remote files from different sources and assigning their contents to sections in stack manifests. It supports various protocols (file, http, git, s3, etc.) and can filter content using YQ expressions.

Learnt from: aknysh
PR: cloudposse/atmos#810
File: examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml:28-32
Timestamp: 2024-12-01T00:33:20.298Z
Learning: In `examples/tests/stacks/catalog/terraform/template-functions-test2/defaults.yaml`, `!exec atmos terraform output` is used in examples to demonstrate its usage, even though `!terraform.output` is the recommended approach according to the documentation.

Learnt from: aknysh
PR: cloudposse/atmos#0
File: :0-0
Timestamp: 2025-01-19T22:30:27.600Z
Learning: The Atmos YAML function `!env` is used to retrieve environment variables and assign them to sections in stack manifests. It supports both simple types (string, number, boolean) and complex types (JSON-encoded lists, maps, objects).

Learnt from: shirkevich
PR: cloudposse/atmos#1034
File: website/docs/core-concepts/projects/configuration/stores.mdx:173-177
Timestamp: 2025-02-11T08:21:33.143Z
Learning: The parameter for configuring stack path delimiter in store configurations is consistently named `stack_delimiter` (not `stacks_delimiter`) across all store types in Atmos.