Conversation
mergify
bot
added
triage
nitrocode
added
patch
…deep merge (3.5× faster) The previous MergeWithOptions implementation called DeepCopyMap on every input before passing it to mergo.Merge, paying two costs per step: a full pre-copy of the source map (including keys that would never conflict) plus reflection overhead inside mergo. The new implementation: - Deep-copies only the first input to seed the accumulator. - Merges each subsequent input via deepMergeNative, which copies values only when they are stored as leaves in the accumulator. Shared intermediate map[string]any containers are recursed into without any allocation. - Contains no reflection; every type switch operates directly on map[string]any and []any (the only types present after YAML parsing). Benchmark result (3-input merge, AMD EPYC 7763): Before: 682 k iter / 5062 ns/op After: 2514 k iter / 1427 ns/op ← 3.5× faster All three list merge strategies (replace, append, merge) and the WithSliceDeepCopy scalar-preservation behaviour are preserved and verified by the full existing test suite plus 23 new dedicated tests. Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Improve performance of deep merge for YAML files
perf(merge): replace mergo pre-copy loop with reflection-free native deep merge (3.5× faster)
Mar 15, 2026
mergify
bot
removed
the
wip
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…tHub Advanced Security alerts 5236-5239) Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![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: 9
🧹 Nitpick comments (2)
internal/exec/yaml_func_template_test.go (1)
439-441: Add terraform fallback for consistency.These subtests only check for
tofu, but the "atmos.Component integration" subtest at lines 397-401 checks bothtofuandterraform. Consider using the same fallback pattern here for consistency and broader compatibility.🔧 Proposed fix for lines 439-441
- if _, err := exec.LookPath("tofu"); err != nil { - t.Skip("skipping: 'tofu' binary not found in PATH (required for atmos.Component integration test)") - } + _, tofuErr := exec.LookPath("tofu") + _, tfErr := exec.LookPath("terraform") + if tofuErr != nil && tfErr != nil { + t.Skip("skipping: neither 'tofu' nor 'terraform' binary found in PATH (required for atmos.Component integration test)") + }🔧 Proposed fix for lines 466-468
- if _, err := exec.LookPath("tofu"); err != nil { - t.Skip("skipping: 'tofu' binary not found in PATH (required for atmos.Component integration test)") - } + _, tofuErr := exec.LookPath("tofu") + _, tfErr := exec.LookPath("terraform") + if tofuErr != nil && tfErr != nil { + t.Skip("skipping: neither 'tofu' nor 'terraform' binary found in PATH (required for atmos.Component integration test)") + }Also applies to: 466-468
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/yaml_func_template_test.go` around lines 439 - 441, The test currently skips when only the "tofu" binary is missing; change the check to try exec.LookPath for both "tofu" and "terraform" and only call t.Skip if neither is found—i.e., attempt exec.LookPath("tofu") then exec.LookPath("terraform") and proceed if either succeeds, otherwise t.Skip with an updated message mentioning both binaries; apply the same change to the other occurrence that checks "tofu" (the block at the second instance around lines 466-468).pkg/merge/merge_native_test.go (1)
176-182: Prefer sentinel checks over error-string matching.These cases pin message text even though the production code already returns or wraps stable merge sentinels.
assert.ErrorIs/require.ErrorIswould keep the tests resilient to added context and align with the repo rule.As per coding guidelines, "
**/*_test.go: ... useerrors.Is()for error checking."Also applies to: 441-448, 687-691, 703-705, 727-741, 1082-1085
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/merge/merge_native_test.go` around lines 176 - 182, The test for deepMergeNative should use errors.Is-style checking against the sentinel error the production code returns for a nil destination instead of matching the error string; replace the assert.Contains(err.Error(), "must not be nil") with require.ErrorIs(err, ErrNilDst) (or the actual sentinel name used by the merge package, e.g., mergeSentinelNilDst) so the test calls require.ErrorIs(err, <sentinel>) after calling deepMergeNative(nil, ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 406-418: The guidance mistakenly suggests adding a standalone
TestMain which will cause duplicate TestMain compile errors in packages that
already define one; update the text to explicitly instruct contributors to
modify/extend the existing TestMain in their package (not add a second function)
by inserting the env-gate check for "_ATMOS_TEST_EXIT_ONE" and the early
os.Exit(1) branch into the package's current TestMain implementation or test
bootstrap (refer to the TestMain symbol and the "_ATMOS_TEST_EXIT_ONE" env var),
and give a brief example wording telling contributors to merge the snippet into
their existing TestMain rather than copying it verbatim.
In `@internal/exec/terraform_execute_helpers_pipeline_test.go`:
- Around line 169-200: Update TestRunWorkspaceSetup_RecoveryPath to assert the
specific warning message is emitted (e.g., check the captured log contains
"Workspace is already active") instead of only asserting nil; capture the logger
output by swapping the global logger/writer before calling runWorkspaceSetup and
after the call restore the original logger writer (store the previous writer in
a variable and restore it, do not hardcode os.Stderr) so logger state is not
leaked to other tests; apply the same fixes to the other test at the same
pattern (lines ~229-232) that restores the logger incorrectly.
In `@internal/exec/terraform_utils_test.go`:
- Around line 1827-1835: The test only covers an absolute TF_DATA_DIR path; add
a second subcase that sets a relative TF_DATA_DIR to "custom-tf-dir" and creates
the environment file under filepath.Join(dir, "custom-tf-dir") so
isTerraformCurrentWorkspace(dir, "nonprod") resolves correctly; modify the t.Run
"respects TF_DATA_DIR env var" to include (or add a new t.Run) where you call
t.Setenv("TF_DATA_DIR", "custom-tf-dir") and write the environment file at
filepath.Join(dir, "custom-tf-dir", "environment") before asserting
assert.True(t, isTerraformCurrentWorkspace(dir, "nonprod")).
In `@internal/exec/validate_stacks_test.go`:
- Around line 94-103: The WalkDir callback in validate_stacks_test.go currently
ignores the error parameter and calls d.IsDir() unguarded which can panic;
modify the filepath.WalkDir callback used to count fixtureFileCount so it checks
the incoming error first (if err != nil return err) and also ensure d is non-nil
before calling d.IsDir(); keep the existing logic that increments
fixtureFileCount for non-dir .yaml files and let WalkDir propagate any directory
read errors by returning the error from the callback so the require.Positive
guard remains meaningful.
In `@pkg/merge/merge_native.go`:
- Around line 53-77: Move the slice/type-mismatch guard earlier in
deepMergeNative so that you check for a conflicting dst slice before handling
map branches: when processing keys in deepMergeNative (symbols: srcVal, dstVal,
dst[k]), detect if dstVal is []any and srcVal is a map and return
ErrMergeTypeMismatch instead of letting the map-handling branches (the fast-path
map branch and the normalize-via-deepCopyValue branch) overwrite dst; ensure
this guard runs before the map-specific branches (the code paths that call
deepMergeNative recursively) so shape changes from slice→map are rejected
consistently (also apply the same reordering to the later similar section around
the other map/type handling).
In `@pkg/merge/merge_no_duplicate_errors_test.go`:
- Around line 93-97: Replace the loose assert.Error check with an assertion that
the returned error wraps the merge sentinel (e.g., use errors.Is against the
package sentinel such as ErrTypeMismatch or the appropriate merge sentinel) so
the test verifies the exact error chain from Merge; add the standard "errors"
import (and errUtils if your project uses it for wrapping helpers) and change
the assertion to something like assert.True(t, errors.Is(err,
merge.ErrTypeMismatch), ...) while keeping the existing assert.Empty check for
stderrOutput.
In `@website/blog/2026-03-15-faster-deep-merge.mdx`:
- Around line 90-94: The blog post contains a placeholder tracking note claiming
a follow-up issue exists for the mergo migration but no issue number is
provided; before publishing, either update that paragraph to link to the actual
follow-up issue number (replacing the sentence that references "[PR `#2201`
description]" and the line "Once the follow-up issue is opened, this sentence
will be updated with its `#number`.") or remove/neutralize the claim about a
tracked follow-up so the post isn’t shipping stale information; if helpful,
create the follow-up issue now and insert its number into that paragraph
referencing the mergo migration and CVE removal.
- Around line 73-80: Update the website paragraph to state that the behavior of
truncating extra source elements when sliceDeepCopy is active is an intentional
divergence from mergo, matching the implementation in merge_native and the
expectations asserted in merge_compare_mergo_test; replace the sentence that
claims this was "also the observed mergo behavior" with a clear statement that
the native implementation intentionally adopts this contract (and link to the
test/implementation by name: merge_native and merge_compare_mergo_test) so
website docs and code/tests present the same, consistent contract for
sliceDeepCopy.
In `@website/src/data/roadmap.js`:
- Line 449: The roadmap entry for the shipped milestone "3.5× faster deep merge
(hot-path)" is added but the initiative progress wasn't updated; update the
initiative's quality.progress value from 80 to 86 to reflect 12 of 14 milestones
shipped, and ensure the new milestone object (the entry with label "3.5× faster
deep merge (hot-path)", status: 'shipped', pr: 2201, changelog:
'faster-deep-merge') is included under the same initiative so progress and
changelog/pr data are consistent with the shipping policy.
---
Nitpick comments:
In `@internal/exec/yaml_func_template_test.go`:
- Around line 439-441: The test currently skips when only the "tofu" binary is
missing; change the check to try exec.LookPath for both "tofu" and "terraform"
and only call t.Skip if neither is found—i.e., attempt exec.LookPath("tofu")
then exec.LookPath("terraform") and proceed if either succeeds, otherwise t.Skip
with an updated message mentioning both binaries; apply the same change to the
other occurrence that checks "tofu" (the block at the second instance around
lines 466-468).
In `@pkg/merge/merge_native_test.go`:
- Around line 176-182: The test for deepMergeNative should use errors.Is-style
checking against the sentinel error the production code returns for a nil
destination instead of matching the error string; replace the
assert.Contains(err.Error(), "must not be nil") with require.ErrorIs(err,
ErrNilDst) (or the actual sentinel name used by the merge package, e.g.,
mergeSentinelNilDst) so the test calls require.ErrorIs(err, <sentinel>) after
calling deepMergeNative(nil, ...).
🪄 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: f0de36a0-a628-439f-a205-f501e0a08e60
📒 Files selected for processing (29)
CLAUDE.mddocs/fixes/2026-03-19-deep-merge-native-fixes.mderrors/errors.gointernal/exec/terraform_clean_test.gointernal/exec/terraform_execute_helpers_exec.gointernal/exec/terraform_execute_helpers_pipeline_test.gointernal/exec/terraform_test.gointernal/exec/terraform_utils.gointernal/exec/terraform_utils_test.gointernal/exec/testmain_test.gointernal/exec/validate_stacks_test.gointernal/exec/workflow_identity_test.gointernal/exec/workflow_utils_test.gointernal/exec/yaml_func_template_test.gointernal/exec/yaml_func_terraform_output_test.gointernal/exec/yaml_func_terraform_state_test.gointernal/exec/yaml_func_terraform_state_workspaces_disabled_test.gointernal/exec/yaml_func_utils_test.gopkg/merge/merge.gopkg/merge/merge_compare_mergo_test.gopkg/merge/merge_context_demo_test.gopkg/merge/merge_context_test.gopkg/merge/merge_native.gopkg/merge/merge_native_test.gopkg/merge/merge_no_duplicate_errors_test.gotests/preconditions.gowebsite/blog/2026-03-15-faster-deep-merge.mdxwebsite/blog/authors.ymlwebsite/src/data/roadmap.js
…docs cleanup Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/4afff323-136a-450a-b021-1ef1bcca14fe
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…s, mergo issue link, CLAUDE.md compare_mergo command Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/2a4fda27-af54-42b0-a99c-111464ba91d9
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…; update benchmark, compare_mergo header, and TF_DATA_DIR comment Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/93ee2135-1e5b-487d-94f5-e3af9cc1f548
…de review Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/93ee2135-1e5b-487d-94f5-e3af9cc1f548
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Audit round number: 12
✏️ Learnings added
🧠 Learnings used |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Audit round number: 13
🧠 Learnings used |
Thorough analysis of the core merge pipeline replacement: - 0 critical bugs found - 2 high-risk items (narrow cross-validation, mergo/native split) - 2 medium items (misleading test name, no concurrent test) - Detailed merge semantics table (maps, slices, nil, types) - Known divergence documented (sliceDeepCopy truncation) - Terraform workspace recovery analysis (separate concern in PR) - Prioritized recommendations (must/should/after merge) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ocess invocation Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/5bb3b12c-a169-44ec-99ea-215b6fed3401
Critical fix: - mergeSlicesNative now extends result when src is longer than dst, matching mergo's WithSliceDeepCopy behavior. Lint fixes (all pre-existing in PR #2201): - nolint directives for deepMergeNative complexity - nolint for TF_DATA_DIR, ReadFile, client.Do - Fix godot, nolintlint, gocritic issues Tests: - Fix misleading test name, update 3 truncation tests - Add 5 cross-validation tests for slice modes Docs: - Add mergo TODO comments, update fix doc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rmance' into copilot/improve-deep-merge-performance
Member
|
@coderabbitai full review please, and check if all your comments and concerns have been correctly addressed |
Contributor
🧩 Analysis chain
Executive summary
What’s addressed
Still open (ranked)
If you’d like, I can open the mergo‑migration follow‑up and draft the two guardrail tests. 🏁 Script executed: Length of output: 17348
Addressed (all previous critical/high)
Open (non‑blocking)
Conclusion
✏️ Learnings added
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/terraform_execute_helpers_exec.go (1)
219-227:⚠️ Potential issue | 🟠 MajorPreserve the select-path routing on the
workspace newfallback.Line 219 drops the
wsOptsandworkspaceSelectRedirectStdErrbehavior from the earlierworkspace selectcall. Ifterraform outputorshowhas to create the workspace, theCreated and switched to workspace ...banner can leak onto stdout and corrupt the data-producing command output.🛠️ Proposed fix
newErr := ExecuteShellCommand( *atmosConfig, info.Command, []string{"workspace", "new", info.TerraformWorkspace}, componentPath, info.ComponentEnvList, info.DryRun, - info.RedirectStdErr, + workspaceSelectRedirectStdErr, + wsOpts..., )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_execute_helpers_exec.go` around lines 219 - 227, The fallback that runs ExecuteShellCommand to create a workspace (the "workspace new" call) must preserve the same select-path routing and stderr-redirection used by the earlier "workspace select" attempt; update the ExecuteShellCommand invocation for workspace creation so it reuses wsOpts (or the same option slice/values passed into the "workspace select" call) and passes workspaceSelectRedirectStdErr (or the same redirect flag) instead of dropping them, ensuring the created-banner remains on the routed/redirected stream and does not pollute stdout for functions like terraform output/show.
♻️ Duplicate comments (4)
internal/exec/validate_stacks_test.go (1)
105-117:⚠️ Potential issue | 🟡 MinorDon't drop the
WalkDirfailure.The callback now returns
err, but_ = filepath.WalkDir(...)still discards it. A bad fixture path or read failure turns into a confusing count assertion instead of surfacing the real scan error.Suggested fix
- _ = filepath.WalkDir(filepath.Join(absPath, "stacks"), func(_ string, d os.DirEntry, err error) error { + walkErr := filepath.WalkDir(filepath.Join(absPath, "stacks"), func(_ string, d os.DirEntry, err error) error { if err != nil { return err } if d == nil { return nil } if !d.IsDir() && strings.HasSuffix(d.Name(), ".yaml") { fixtureFileCount++ } return nil }) + require.NoError(t, walkErr, "failed to scan stacks fixture")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/validate_stacks_test.go` around lines 105 - 117, The call to filepath.WalkDir currently discards its returned error (using `_ = filepath.WalkDir(...)`), so failures scanning the fixtures under filepath.Join(absPath, "stacks") are hidden and tests only show a later count assertion; change the test to capture and check the WalkDir error (e.g., err := filepath.WalkDir(...); require.NoError(t, err) or if err != nil { t.Fatalf(...) }) so any path/read failure is surfaced, keeping the same callback that increments fixtureFileCount and referencing the same absPath and fixtureFileCount variables.internal/exec/terraform_execute_helpers_pipeline_test.go (1)
201-203:⚠️ Potential issue | 🟡 MinorRestore the previous logger output, not a hardcoded one.
Both tests mutate the global logger and always put it back to
os.Stderr. If another test configured a different writer first, this leaks state across the package and makes failures order-dependent. Snapshot the current output before redirecting and restore that value in the defer.Also applies to: 241-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_execute_helpers_pipeline_test.go` around lines 201 - 203, The test unconditionally restores the global logger output to os.Stderr, which leaks state; capture the current output writer before changing it (e.g. oldOut := log.Default().Writer() or obtain via log.Default().Writer()/Output) then call log.Default().SetOutput(&logBuf) and defer restoring the previous writer with log.Default().SetOutput(oldOut); apply the same change to both places (around the logBuf setup at the locations using logBuf) so the original logger output is preserved and restored instead of hardcoding os.Stderr.pkg/merge/merge_no_duplicate_errors_test.go (1)
95-97:⚠️ Potential issue | 🟡 MinorAssert the outer merge sentinel too.
This only pins
ErrMergeTypeMismatch. IfMergestops wrapping withErrMergebut still returns the inner sentinel, this test still passes while the public error contract regresses.Suggested tighten-up
- // The error must be returned, not swallowed, and must wrap ErrMergeTypeMismatch. - assert.True(t, errors.Is(err, errUtils.ErrMergeTypeMismatch), - "type-mismatch must return an error wrapping ErrMergeTypeMismatch, got: %v", err) + // The error must be returned, not swallowed, and must keep the full merge error chain. + if assert.Error(t, err, "type-mismatch must return an error") { + assert.True(t, errors.Is(err, errUtils.ErrMerge)) + assert.True(t, errors.Is(err, errUtils.ErrMergeTypeMismatch)) + }Based on learnings: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions (implement or remove); avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use
errors.Is()for error checking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/merge/merge_no_duplicate_errors_test.go` around lines 95 - 97, The test only asserts that the returned error wraps errUtils.ErrMergeTypeMismatch but does not assert the outer sentinel errUtils.ErrMerge; update the Test (around the assertion of errors.Is(err, errUtils.ErrMergeTypeMismatch) for the Merge call) to also assert errors.Is(err, errUtils.ErrMerge) so the public contract that Merge returns an error wrapped by ErrMerge is enforced; locate the Merge call and existing assertion in merge_no_duplicate_errors_test.go and add the additional errors.Is check against errUtils.ErrMerge (keeping the existing inner-sentinel check).pkg/merge/merge_native.go (1)
83-87:⚠️ Potential issue | 🟠 MajorWrap recursive failures with key/index context.
These branches bubble the raw merge error back up unchanged. On a large stack, callers learn that a merge failed but not where, which forces substring hunting instead of pointing at the offending path. Wrapping here with
merge key %q/merge slice index %dkeepserrors.Isintact while making the failure actionable.Suggested shape
+import ( + "fmt" + "reflect" + + errUtils "github.com/cloudposse/atmos/errors" +)if srcMap, srcIsMap := srcVal.(map[string]any); srcIsMap { if dstMap, dstIsMap := dstVal.(map[string]any); dstIsMap { if err := deepMergeNative(dstMap, srcMap, appendSlice, sliceDeepCopy); err != nil { - return err + return fmt.Errorf("merge key %q: %w", k, err) } continue } @@ if normalizedSrcMap, ok := deepCopyValue(srcVal).(map[string]any); ok { if dstMap, dstIsMap := dstVal.(map[string]any); dstIsMap { if err := deepMergeNative(dstMap, normalizedSrcMap, appendSlice, sliceDeepCopy); err != nil { - return err + return fmt.Errorf("merge key %q: %w", k, err) } continue } @@ if err := deepMergeNative(merged, srcMap, appendSlice, sliceDeepCopy); err != nil { - return nil, err + return nil, fmt.Errorf("merge slice index %d: %w", i, err) }As per coding guidelines, "Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using
fmt.Errorf("context: %w", err)."Also applies to: 98-101, 224-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/merge/merge_native.go` around lines 83 - 87, The recursive calls to deepMergeNative in the map and slice fast-paths should wrap returned errors with contextual path info so callers know which key or index failed; update the map branch where deepMergeNative(dstMap, srcMap, appendSlice, sliceDeepCopy) is called to wrap errors as fmt.Errorf("merge key %q: %w", key, err), and similarly wrap errors from recursive merges in slice branches as fmt.Errorf("merge slice index %d: %w", idx, err). Apply the same wrapping pattern to the other recursive call sites referenced (the other map/slice recursion points) so errors.Is remains usable but messages include the offending key/index.
🧹 Nitpick comments (4)
internal/exec/workflow_identity_test.go (1)
141-143: Precondition check prevents confusing failures.Good addition — this test spawns an actual atmos subprocess, so skipping when the binary is missing is appropriate.
Consider using the existing helper for consistency:
♻️ Optional refactor
- if _, err := exec.LookPath("atmos"); err != nil { - t.Skip("skipping: atmos binary not found in PATH (required to execute atmos workflow steps)") - } + tests.RequireExecutable(t, "atmos", "executing atmos workflow steps")This would require adding the
"github.com/cloudposse/atmos/tests"import. Either approach works — the current code is functionally correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/workflow_identity_test.go` around lines 141 - 143, Replace the manual exec.LookPath check with the project test helper: remove the exec.LookPath block and instead call tests.RequireAtmos(t) (or the equivalent helper in the "github.com/cloudposse/atmos/tests" package) at the top of TestWorkflowIdentity to skip when the atmos binary is missing; add the "github.com/cloudposse/atmos/tests" import and use the helper (tests.RequireAtmos) in place of the current exec.LookPath check to keep consistency with other tests.internal/exec/yaml_func_template_test.go (1)
397-401: Extract the Terraform/OpenTofu precheck into one local helper.The same
LookPathfallback and skip message are copied into three subtests here. A small closure insideTestYamlFuncTemplate_Integrationwould keep the gating consistent and make later fixture changes cheaper.Also applies to: 439-443, 468-472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/yaml_func_template_test.go` around lines 397 - 401, Extract the repeated exec.LookPath precheck into a single local helper closure inside TestYamlFuncTemplate_Integration (e.g., precheck := func(t *testing.T) { ... }) that runs exec.LookPath("tofu") and exec.LookPath("terraform") and calls t.Skip with the existing message when both are missing; replace the three duplicated blocks that currently call exec.LookPath and t.Skip with calls to this precheck(t) to keep gating consistent across subtests and simplify future fixture changes.pkg/merge/merge_test.go (1)
764-808: Collapse these into one table-driven merge-flags test and add one typed/nested row.The three cases are the same harness with different booleans, and they only pin flat
[]anyscalar behavior. A single table-driven test with one typed or nested slice case would cover the risky native-merge shapes with less duplication.As per coding guidelines, "Use table-driven tests for testing multiple scenarios in Go."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/merge/merge_test.go` around lines 764 - 808, Combine the three tests TestMergeWithOptions_AppendSliceFlag, TestMergeWithOptions_SliceDeepCopyFlag, and TestMergeWithOptions_NoFlagsReplaceList into a single table-driven test (e.g., TestMergeWithOptions_Flags) that iterates cases with fields for appendSlice and sliceDeepCopy, inputs, and expected result; for each case call MergeWithOptions(nil, inputs, appendSlice, sliceDeepCopy) and assert no error and that the resulting "tags" value equals the expected slice. Include one additional case that uses a typed or nested slice (for example []int or a slice of maps) to exercise deepMergeNative's typed/nested merge behavior. Keep each table entry descriptive and run the same assertions per-case so the different flag combinations and the typed/nested shape are covered without duplicating the test harness.docs/fixes/2026-03-19-deep-merge-native-fixes.md (1)
22-35: Optional: Add language tags to code blocks for better rendering.The fenced code blocks at lines 22 and 29 are missing language specifiers. Consider adding language tags for syntax highlighting:
♻️ Suggested markdown improvements
**Before:** -``` +```text for each input:**After:** -``` +```text result = DeepCopyMap(inputs[0])This addresses the markdownlint warnings and improves documentation readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/fixes/2026-03-19-deep-merge-native-fixes.md` around lines 22 - 35, Add explicit language tags to the two fenced code blocks that show the merge pseudocode (the blocks containing "for each input:" / "copy = DeepCopyMap(input) ..." and "result = DeepCopyMap(inputs[0])" / "deepMergeNative(result, input)"), e.g., replace the opening ``` with ```text or ```pseudo so markdownlint and syntax highlighters render them correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exec/terraform_execute_single_invocation_test.go`:
- Around line 77-90: The test currently ignores the return from
executeCommandPipeline; capture its error (e.g., err :=
executeCommandPipeline(&atmosConfig, &info, execCtx)) and add an assertion that
the error wraps the exit-code type (use errors.As to check err is or contains
errUtils.ExitCodeError) before or alongside the counter-file assertions so the
test ensures both a single subprocess invocation and that the pipeline preserved
the ExecuteShellCommand exit-1 semantics (refer to executeCommandPipeline and
errUtils.ExitCodeError to locate the code).
In `@internal/exec/terraform_utils.go`:
- Around line 110-136: The helper isTerraformCurrentWorkspace reads TF_DATA_DIR
from the current process environment (os.Getenv) which can differ from the child
env used by the Terraform subprocess; change the function signature
(isTerraformCurrentWorkspace) to accept either the resolved env map (e.g., env
map[string]string or []string) or a precomputed tfDataDir string and use that
value instead of calling os.Getenv inside the function, then update callers
(where execEnvFromConfig / info builds the subprocess env) to pass the resolved
TF_DATA_DIR (or the env) through so envFile is constructed from the same env
Terraform used.
In `@internal/exec/yaml_func_terraform_state_workspaces_disabled_test.go`:
- Around line 173-179: The test currently downgrades failures to delete
terraform.tfstate and terraform.tfstate.backup to t.Logf, which can leave stale
state in the shared fixture; update the cleanup in
yaml_func_terraform_state_workspaces_disabled_test.go to attempt a retry loop
(e.g., try os.Remove on filepath.Join(mockComponentPath, "terraform.tfstate")
and filepath.Join(mockComponentPath, "terraform.tfstate.backup") several times
with small sleeps if removal fails due to Windows locking) and, after retries,
assert/fail the test (use t.Fatalf or t.Errorf) if the file still exists (check
with os.Stat or os.IsNotExist) instead of only logging via t.Logf; keep
references to os.Remove, os.IsNotExist, and the mockComponentPath join calls
when implementing the fix.
In `@pkg/merge/merge_compare_mergo_test.go`:
- Around line 119-146: The test "sliceDeepCopy merges overlapping map elements"
currently only asserts names but the comments describe expectations for merged
scalar/added fields; update the assertions to verify those fields: after calling
MergeWithOptions(cfg, inputs, false, true) and extracting groups, assert for g0
(variable g0) that g0["size"] == "large" (dst override applied), and for g1
(variable g1) assert g1["size"] == "medium" and g1["replicas"] == 3 so the test
actually validates the described merge behavior of MergeWithOptions.
- Around line 8-10: The test run example uses an incorrect -run pattern; update
the documented command to match existing tests (e.g., use -run TestCompareMergo
to run both TestCompareMergo_NestedMapsMerge and TestCompareMergo_SliceModes, or
a regex like -run TestCompareMergo_ to match all TestCompareMergo_* tests). Edit
the comment block containing the local run command so the -run argument
references TestCompareMergo (or TestCompareMergo_) instead of CrossValidate to
ensure the tests actually execute.
In `@website/blog/2026-03-15-faster-deep-merge.mdx`:
- Around line 75-84: The paragraph about sliceDeepCopy is incorrect: update the
website copy to state that when sliceDeepCopy is active the resulting slice
grows to max(len(dst), len(src)) so extra source elements (the tail, e.g.,
"gpu") are preserved rather than dropped; reference the actual behavior
implemented in mergeSlicesNative (in merge_native.go) and validated by
merge_compare_mergo_test.go (run with go test -tags compare_mergo
./pkg/merge/...) and adjust the wording to describe that contract and its
cross-validation against mergo.
---
Outside diff comments:
In `@internal/exec/terraform_execute_helpers_exec.go`:
- Around line 219-227: The fallback that runs ExecuteShellCommand to create a
workspace (the "workspace new" call) must preserve the same select-path routing
and stderr-redirection used by the earlier "workspace select" attempt; update
the ExecuteShellCommand invocation for workspace creation so it reuses wsOpts
(or the same option slice/values passed into the "workspace select" call) and
passes workspaceSelectRedirectStdErr (or the same redirect flag) instead of
dropping them, ensuring the created-banner remains on the routed/redirected
stream and does not pollute stdout for functions like terraform output/show.
---
Duplicate comments:
In `@internal/exec/terraform_execute_helpers_pipeline_test.go`:
- Around line 201-203: The test unconditionally restores the global logger
output to os.Stderr, which leaks state; capture the current output writer before
changing it (e.g. oldOut := log.Default().Writer() or obtain via
log.Default().Writer()/Output) then call log.Default().SetOutput(&logBuf) and
defer restoring the previous writer with log.Default().SetOutput(oldOut); apply
the same change to both places (around the logBuf setup at the locations using
logBuf) so the original logger output is preserved and restored instead of
hardcoding os.Stderr.
In `@internal/exec/validate_stacks_test.go`:
- Around line 105-117: The call to filepath.WalkDir currently discards its
returned error (using `_ = filepath.WalkDir(...)`), so failures scanning the
fixtures under filepath.Join(absPath, "stacks") are hidden and tests only show a
later count assertion; change the test to capture and check the WalkDir error
(e.g., err := filepath.WalkDir(...); require.NoError(t, err) or if err != nil {
t.Fatalf(...) }) so any path/read failure is surfaced, keeping the same callback
that increments fixtureFileCount and referencing the same absPath and
fixtureFileCount variables.
In `@pkg/merge/merge_native.go`:
- Around line 83-87: The recursive calls to deepMergeNative in the map and slice
fast-paths should wrap returned errors with contextual path info so callers know
which key or index failed; update the map branch where deepMergeNative(dstMap,
srcMap, appendSlice, sliceDeepCopy) is called to wrap errors as
fmt.Errorf("merge key %q: %w", key, err), and similarly wrap errors from
recursive merges in slice branches as fmt.Errorf("merge slice index %d: %w",
idx, err). Apply the same wrapping pattern to the other recursive call sites
referenced (the other map/slice recursion points) so errors.Is remains usable
but messages include the offending key/index.
In `@pkg/merge/merge_no_duplicate_errors_test.go`:
- Around line 95-97: The test only asserts that the returned error wraps
errUtils.ErrMergeTypeMismatch but does not assert the outer sentinel
errUtils.ErrMerge; update the Test (around the assertion of errors.Is(err,
errUtils.ErrMergeTypeMismatch) for the Merge call) to also assert errors.Is(err,
errUtils.ErrMerge) so the public contract that Merge returns an error wrapped by
ErrMerge is enforced; locate the Merge call and existing assertion in
merge_no_duplicate_errors_test.go and add the additional errors.Is check against
errUtils.ErrMerge (keeping the existing inner-sentinel check).
---
Nitpick comments:
In `@docs/fixes/2026-03-19-deep-merge-native-fixes.md`:
- Around line 22-35: Add explicit language tags to the two fenced code blocks
that show the merge pseudocode (the blocks containing "for each input:" / "copy
= DeepCopyMap(input) ..." and "result = DeepCopyMap(inputs[0])" /
"deepMergeNative(result, input)"), e.g., replace the opening ``` with ```text or
```pseudo so markdownlint and syntax highlighters render them correctly.
In `@internal/exec/workflow_identity_test.go`:
- Around line 141-143: Replace the manual exec.LookPath check with the project
test helper: remove the exec.LookPath block and instead call
tests.RequireAtmos(t) (or the equivalent helper in the
"github.com/cloudposse/atmos/tests" package) at the top of TestWorkflowIdentity
to skip when the atmos binary is missing; add the
"github.com/cloudposse/atmos/tests" import and use the helper
(tests.RequireAtmos) in place of the current exec.LookPath check to keep
consistency with other tests.
In `@internal/exec/yaml_func_template_test.go`:
- Around line 397-401: Extract the repeated exec.LookPath precheck into a single
local helper closure inside TestYamlFuncTemplate_Integration (e.g., precheck :=
func(t *testing.T) { ... }) that runs exec.LookPath("tofu") and
exec.LookPath("terraform") and calls t.Skip with the existing message when both
are missing; replace the three duplicated blocks that currently call
exec.LookPath and t.Skip with calls to this precheck(t) to keep gating
consistent across subtests and simplify future fixture changes.
In `@pkg/merge/merge_test.go`:
- Around line 764-808: Combine the three tests
TestMergeWithOptions_AppendSliceFlag, TestMergeWithOptions_SliceDeepCopyFlag,
and TestMergeWithOptions_NoFlagsReplaceList into a single table-driven test
(e.g., TestMergeWithOptions_Flags) that iterates cases with fields for
appendSlice and sliceDeepCopy, inputs, and expected result; for each case call
MergeWithOptions(nil, inputs, appendSlice, sliceDeepCopy) and assert no error
and that the resulting "tags" value equals the expected slice. Include one
additional case that uses a typed or nested slice (for example []int or a slice
of maps) to exercise deepMergeNative's typed/nested merge behavior. Keep each
table entry descriptive and run the same assertions per-case so the different
flag combinations and the typed/nested shape are covered without duplicating the
test harness.
🪄 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: 28b959e0-cf50-485b-8d0d-486606c4d151
📒 Files selected for processing (35)
CLAUDE.mddocs/fixes/2026-03-19-deep-merge-native-fixes.mderrors/errors.gointernal/exec/terraform_clean_test.gointernal/exec/terraform_execute_exit_wrapping_test.gointernal/exec/terraform_execute_helpers_exec.gointernal/exec/terraform_execute_helpers_pipeline_test.gointernal/exec/terraform_execute_helpers_workspace_test.gointernal/exec/terraform_execute_single_invocation_test.gointernal/exec/terraform_test.gointernal/exec/terraform_utils.gointernal/exec/terraform_utils_test.gointernal/exec/testmain_test.gointernal/exec/validate_stacks_test.gointernal/exec/workflow_identity_test.gointernal/exec/workflow_utils_test.gointernal/exec/yaml_func_template_test.gointernal/exec/yaml_func_terraform_output_test.gointernal/exec/yaml_func_terraform_state_test.gointernal/exec/yaml_func_terraform_state_workspaces_disabled_test.gointernal/exec/yaml_func_utils_test.gopkg/devcontainer/config_loader.gopkg/merge/merge.gopkg/merge/merge_compare_mergo_test.gopkg/merge/merge_context_demo_test.gopkg/merge/merge_context_test.gopkg/merge/merge_native.gopkg/merge/merge_native_test.gopkg/merge/merge_no_duplicate_errors_test.gopkg/merge/merge_test.gopkg/merge/merge_yaml_functions.gotests/preconditions.gowebsite/blog/2026-03-15-faster-deep-merge.mdxwebsite/blog/authors.ymlwebsite/src/data/roadmap.js
internal/exec/yaml_func_terraform_state_workspaces_disabled_test.go
Outdated
Show resolved
Hide resolved
- isTerraformCurrentWorkspace: accept envList param so TF_DATA_DIR from subprocess env (ComponentEnvList) takes precedence over process env - Add envVarFromList helper (companion to envKeyIsSet) - Assert ExitCodeError in single-invocation regression test - Fix logger restoration in pipeline tests (Writer() doesn't exist) - Add require import to merge_test.go (table-driven tests use it) - Strengthen sliceDeepCopy cross-validation assertions (size, replicas) - Fix misleading comments in compare_mergo_test.go - Fix blog post: sliceDeepCopy extends result, not truncates - Fix -run pattern in docs: CrossValidate -> CompareMergo - Retry-based cleanup for shared fixture state files (Windows compat) - Add code block language tags to fix doc - Assert both ErrMerge and ErrMergeTypeMismatch sentinels in error tests - Wrap recursive merge errors with key/index context Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aknysh
approved these changes
Mar 24, 2026
Member
aknysh
left a comment
aknysh
left a comment
There was a problem hiding this comment.
thanks @nitrocode
I'm merging the PR, it will be available in the next Atmos minor release.
If any issues, we'll fix and create a patch release
|
These changes were released in v1.211.1-rc.0. |
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
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.
TestExecuteMainTerraformCommand_Error_Propagates(item 2 exec test)TestMergeWithOptions_EmptyInputs_ReturnsEmptyMap,StrategyFlags_WireThrough(item 2 merge tests)terraform_execute_exit_wrapping_test.go— contract test for ExitCodeError wrappingterraform_execute_single_invocation_test.go— spy counter via _ATMOS_TEST_COUNTER_FILE in testmaintestmain_test.goupdated to support _ATMOS_TEST_COUNTER_FILEMigrate remaining mergo call-sitesmilestone with issue: 2242 (item 7)🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation