Conversation
…ffected --upload (~250x speedup) Two critical fixes: 1. Native deep merge type-mismatch regression (PR #2201): Remove slice→map and slice→non-slice type-check guards that broke real configs where users override lists with {} (empty map), scalars, or null. The WithOverride contract means src always wins regardless of type differences. Atmos never used mergo's WithTypeCheck — the guards were an incorrect assumption. 2. describe affected --upload timeout on large infrastructures: Cache ExecuteDescribeStacks result, component lookups, and build a reverse dependency index — all computed once instead of per-affected-component. For 2,422 affected items: 40+ min → ~10s (~250x speedup). Three optimizations applied incrementally: - Cache ExecuteDescribeStacks: 40+ min → ~3.5 min - Cache component lookup from cached stacks: ~3.5 min → ~1:54 - Pre-built reverse dependency index: ~1:54 → ~10s Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughReverts native deep-merge type-mismatch guards so stack imports can override values across types, and optimizes describe-affected by caching stack resolution and adding a reverse-dependency index to avoid repeated full-stack scans. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as DescribeAffected
participant Utils as describe_affected_utils_2
participant Deps as describe_dependents
participant Index as describe_dependents_index
participant Stacks as StacksResolver
Client->>Utils: addDependentsToAffected(affected)
Utils->>Stacks: ExecuteDescribeStacks() (single upfront call)
Stacks-->>Utils: stacks map
Utils->>Index: buildDependencyIndex(stacks)
Index-->>Utils: depIndex
loop per affected component
Utils->>Deps: ExecuteDescribeDependents(target, Stacks=stacks, DepIndex=depIndex)
alt DepIndex present
Deps->>Index: findDependentsFromIndex(target)
Index-->>Deps: dependents (index lookup)
else Fallback
Deps->>Stacks: scan stacks for dependents
Stacks-->>Deps: dependents (linear scan)
end
Deps-->>Utils: dependents
end
Utils-->>Client: aggregated dependents
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/merge/merge_no_duplicate_errors_test.go (1)
73-91:⚠️ Potential issue | 🟡 MinorUse
t.Cleanupforos.Stderrrestoration to avoid global-state leakage on early failure.If this test fails/panics before the restore block runs, later tests can inherit a redirected
stderr. A cleanup hook makes teardown deterministic.Suggested fix
oldStderr := os.Stderr - r, w, _ := os.Pipe() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe failed: %v", err) + } os.Stderr = w + t.Cleanup(func() { + _ = w.Close() + _ = r.Close() + os.Stderr = oldStderr + }) @@ - w.Close() - os.Stderr = oldStderr + _ = w.Close() stderrOutput := <-stderrChan🤖 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 73 - 91, The test currently replaces os.Stderr and restores it manually which can leak global state if the test fails; replace the manual restore with t.Cleanup to always restore os.Stderr and close the pipe: after creating oldStderr, r, w and assigning os.Stderr = w, register a t.Cleanup that closes w and sets os.Stderr = oldStderr (and optionally closes r if you create one that needs closing), ensuring the stderr-reading goroutine still receives EOF and the stderrChan is drained; keep the Merge(...) call and subsequent reading of stderrOutput unchanged but remove the explicit restore/close block so cleanup runs even on early failure.
🧹 Nitpick comments (5)
internal/exec/describe_dependents_index.go (1)
79-82: Consider debug logging on decode failures.Silent
returnon line 82 is safe but may complicate debugging malformed stack configs. Alog.Debugcall here would help troubleshoot edge cases.💡 Optional: Add debug logging
var stackComponentVars schema.Context if err := mapstructure.Decode(stackComponentVarsSection, &stackComponentVars); err != nil { + log.Debug("Failed to decode component vars during index build", "component", stackComponentName, "stack", stackName, "error", err) return }This would require adding the
logimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/describe_dependents_index.go` around lines 79 - 82, The mapstructure.Decode call that populates stackComponentVars currently returns silently on error; update the error path in the describe dependents logic to log the decode failure (include the error and the offending stackComponentVarsSection) via the package logger (add the log import) before returning so malformed stack configs are visible; locate the mapstructure.Decode(...) surrounding stackComponentVars and insert a log.Debug/log.Debugf call that prints context and err prior to the return.internal/exec/describe_dependents_index_test.go (1)
86-106: Consider adding test forenabled: falsecomponents.This test covers
metadata.type: "abstract", butisAbstractOrDisabledalso skips components withenabled: falseviaisComponentEnabled. A separate test case would strengthen coverage.💡 Optional: Test disabled component filtering
func TestBuildDependencyIndex_SkipsDisabledComponents(t *testing.T) { stacks := map[string]any{ "dev-use1": map[string]any{ "components": map[string]any{ "terraform": map[string]any{ "disabled-component": map[string]any{ "metadata": map[string]any{"enabled": false}, "vars": map[string]any{"region": "us-east-1"}, "dependencies": map[string]any{ "components": []any{ map[string]any{"component": "vpc"}, }, }, }, }, }, }, } idx := buildDependencyIndex(stacks) assert.Empty(t, idx["vpc"], "disabled components should be skipped") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/describe_dependents_index_test.go` around lines 86 - 106, Add a new unit test named TestBuildDependencyIndex_SkipsDisabledComponents that mirrors TestBuildDependencyIndex_SkipsAbstractComponents but uses metadata with "enabled": false to exercise the isComponentEnabled/isAbstractOrDisabled path; construct a stacks map with a "disabled-component" under terraform that lists a dependency on "vpc", call buildDependencyIndex(stacks), and assert that idx["vpc"] is empty to ensure disabled components are skipped.docs/fixes/2026-03-24-describe-affected-upload-timeout.md (1)
89-96: Line number references may become stale.The table references specific line ranges (e.g.,
describe_affected_utils_2.go:532-594). Consider using function names or permanent links instead, as line numbers shift with future changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/fixes/2026-03-24-describe-affected-upload-timeout.md` around lines 89 - 96, The table uses fragile file line ranges which will break as code moves; replace those line-range references with stable identifiers and links: use function names like addDependentsToAffected, addDependentsToDependents, ExecuteDescribeDependents, UploadAffectedStacks and the Upload flag handler (or its concrete function name) instead of line numbers, and where possible convert each cell to a permanent permalink (commit/blob URL) or package+symbol reference (package.Function) so reviewers can resolve the target reliably; update the table rows to omit all numeric line ranges and use these names/permalinks consistently.pkg/merge/merge_type_override_test.go (1)
71-77: Consider settingwantValfor the nested case for consistency.The special-case handling at lines 85-89 works, but setting
wantValto the expected nested structure would make the table more self-documenting.Optional: fill in wantVal for completeness
{ name: "nested: list inside map overridden by empty map", dst: map[string]any{"vars": map[string]any{"accounts": []any{"a"}}}, src: map[string]any{"vars": map[string]any{"accounts": map[string]any{}}}, wantKey: "vars", - // After deep merge, vars.accounts should be the empty map from src. + wantVal: map[string]any{"accounts": map[string]any{}}, },Then update the assertion:
- if tt.wantKey == "vars" { - // Nested case: check vars.accounts. - vars, ok := tt.dst["vars"].(map[string]any) - require.True(t, ok) - assert.Equal(t, map[string]any{}, vars["accounts"]) - } else { - assert.Equal(t, tt.wantVal, tt.dst[tt.wantKey]) - } + assert.Equal(t, tt.wantVal, tt.dst[tt.wantKey])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/merge/merge_type_override_test.go` around lines 71 - 77, Add a wantVal entry to the "nested: list inside map overridden by empty map" test case and set it to the expected nested result (e.g. map[string]any{"vars": map[string]any{"accounts": map[string]any{}}}), then update the assertion that currently checks only wantKey to also assert equality against wantVal so the test is self-documenting and verifies the nested value as well.pkg/merge/merge_native.go (1)
16-25: Remove the unusedisMapValuehelper function.It's not called anywhere in the codebase—the rg search confirms only its definition and documentation exist. Since the type-mismatch guards that relied on it have been removed, this function is dead code and should be cleaned up.
🤖 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 16 - 25, The helper function isMapValue is dead code and should be removed: delete the entire isMapValue function declaration from pkg/merge/merge_native.go (the function named isMapValue and its body) and ensure no other code references it (rg already shows none); run the build/tests to confirm removal didn't affect anything.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fixtures/scenarios/merge-type-override/atmos.yaml`:
- Around line 1-13: Add an integration test that exercises the
"merge-type-override" fixture end-to-end by changing the working directory to
"./fixtures/scenarios/merge-type-override" (use t.Chdir) and invoking the
stack-composition pipeline via the command layer (call the same entry used by
integration tests that cover "source-provisioner" and "yaml-functions-in-lists"
— e.g., the CLI entry or ComposeStacks/stack composition function). The test
should run the composition, capture the composed stack output, and assert that
the type-override behavior from deepMergeNative is preserved in the final
composed result; mirror the structure and assertions from the existing
integration tests that exercise composition through the command layer.
---
Outside diff comments:
In `@pkg/merge/merge_no_duplicate_errors_test.go`:
- Around line 73-91: The test currently replaces os.Stderr and restores it
manually which can leak global state if the test fails; replace the manual
restore with t.Cleanup to always restore os.Stderr and close the pipe: after
creating oldStderr, r, w and assigning os.Stderr = w, register a t.Cleanup that
closes w and sets os.Stderr = oldStderr (and optionally closes r if you create
one that needs closing), ensuring the stderr-reading goroutine still receives
EOF and the stderrChan is drained; keep the Merge(...) call and subsequent
reading of stderrOutput unchanged but remove the explicit restore/close block so
cleanup runs even on early failure.
---
Nitpick comments:
In `@docs/fixes/2026-03-24-describe-affected-upload-timeout.md`:
- Around line 89-96: The table uses fragile file line ranges which will break as
code moves; replace those line-range references with stable identifiers and
links: use function names like addDependentsToAffected,
addDependentsToDependents, ExecuteDescribeDependents, UploadAffectedStacks and
the Upload flag handler (or its concrete function name) instead of line numbers,
and where possible convert each cell to a permanent permalink (commit/blob URL)
or package+symbol reference (package.Function) so reviewers can resolve the
target reliably; update the table rows to omit all numeric line ranges and use
these names/permalinks consistently.
In `@internal/exec/describe_dependents_index_test.go`:
- Around line 86-106: Add a new unit test named
TestBuildDependencyIndex_SkipsDisabledComponents that mirrors
TestBuildDependencyIndex_SkipsAbstractComponents but uses metadata with
"enabled": false to exercise the isComponentEnabled/isAbstractOrDisabled path;
construct a stacks map with a "disabled-component" under terraform that lists a
dependency on "vpc", call buildDependencyIndex(stacks), and assert that
idx["vpc"] is empty to ensure disabled components are skipped.
In `@internal/exec/describe_dependents_index.go`:
- Around line 79-82: The mapstructure.Decode call that populates
stackComponentVars currently returns silently on error; update the error path in
the describe dependents logic to log the decode failure (include the error and
the offending stackComponentVarsSection) via the package logger (add the log
import) before returning so malformed stack configs are visible; locate the
mapstructure.Decode(...) surrounding stackComponentVars and insert a
log.Debug/log.Debugf call that prints context and err prior to the return.
In `@pkg/merge/merge_native.go`:
- Around line 16-25: The helper function isMapValue is dead code and should be
removed: delete the entire isMapValue function declaration from
pkg/merge/merge_native.go (the function named isMapValue and its body) and
ensure no other code references it (rg already shows none); run the build/tests
to confirm removal didn't affect anything.
In `@pkg/merge/merge_type_override_test.go`:
- Around line 71-77: Add a wantVal entry to the "nested: list inside map
overridden by empty map" test case and set it to the expected nested result
(e.g. map[string]any{"vars": map[string]any{"accounts": map[string]any{}}}),
then update the assertion that currently checks only wantKey to also assert
equality against wantVal so the test is self-documenting and verifies the nested
value as well.
🪄 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: 56cd8206-f687-4f90-8616-b4278316e2dc
📒 Files selected for processing (27)
docs/fixes/2026-03-24-deep-merge-type-mismatch-regression.mddocs/fixes/2026-03-24-describe-affected-upload-timeout.mdinternal/exec/describe_affected_utils_2.gointernal/exec/describe_dependents.gointernal/exec/describe_dependents_index.gointernal/exec/describe_dependents_index_test.gopkg/merge/merge_native.gopkg/merge/merge_native_test.gopkg/merge/merge_no_duplicate_errors_test.gopkg/merge/merge_type_override_test.gotests/fixtures/scenarios/merge-type-override/atmos.yamltests/fixtures/scenarios/merge-type-override/components/terraform/eks-cluster/main.tftests/fixtures/scenarios/merge-type-override/components/terraform/s3-bucket/main.tftests/fixtures/scenarios/merge-type-override/components/terraform/vpc/main.tftests/fixtures/scenarios/merge-type-override/stacks/catalog/eks-defaults.yamltests/fixtures/scenarios/merge-type-override/stacks/catalog/eks-sandbox.yamltests/fixtures/scenarios/merge-type-override/stacks/catalog/s3-defaults.yamltests/fixtures/scenarios/merge-type-override/stacks/catalog/s3-sandbox.yamltests/fixtures/scenarios/merge-type-override/stacks/catalog/vpc-defaults.yamltests/fixtures/scenarios/merge-type-override/stacks/catalog/vpc-sandbox.yamltests/fixtures/scenarios/merge-type-override/stacks/orgs/acme/_defaults.yamltests/fixtures/scenarios/merge-type-override/stacks/orgs/acme/dev/_defaults.yamltests/fixtures/scenarios/merge-type-override/stacks/orgs/acme/dev/us-east-1/eks.yamltests/fixtures/scenarios/merge-type-override/stacks/orgs/acme/sandbox/_defaults.yamltests/fixtures/scenarios/merge-type-override/stacks/orgs/acme/sandbox/us-east-1/eks.yamltests/fixtures/scenarios/merge-type-override/stacks/orgs/acme/sandbox/us-east-1/s3.yamltests/fixtures/scenarios/merge-type-override/stacks/orgs/acme/sandbox/us-east-1/vpc.yaml
… logging - Remove unused isMapValue helper and reflect import (dead code after type-mismatch guard removal) - Add debug logging for mapstructure decode failures in dependency index build - Add test for enabled:false component filtering in dependency index - Simplify merge_type_override_test nested case (set wantVal, remove special-case) - Remove stale line-number references from fix doc - Update validate_stacks_test: type overrides now succeed (not error) - Add end-to-end integration test for merge-type-override fixture Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/fixes/2026-03-24-describe-affected-upload-timeout.md (1)
20-21: Tighten symptom wording to match observed behavior.“Hang indefinitely” is stronger than the measured behavior (“40–50 min, then timeout/killed”). Consider aligning this sentence with the observed timeout window for accuracy.
Proposed wording
-The `--upload` flag causes the command to hang indefinitely on large infrastructures. +The `--upload` flag causes the command to stall for 40–50 minutes on large infrastructures, often ending in timeout or manual termination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/fixes/2026-03-24-describe-affected-upload-timeout.md` around lines 20 - 21, Update the symptom sentence to match observed behavior by replacing “hang indefinitely” with wording that reflects the measured timeout window (e.g., “hangs for ~40–50 minutes before timing out or being killed”); ensure the sentence about the --upload flag states that the command hangs for that observed duration on large infrastructures rather than claiming an indefinite hang.internal/exec/validate_stacks_test.go (1)
153-180: Consider assertingNoErrorwhenexpectedPartsis empty.When
expectedPartsisnil, the test logs any error but doesn't fail. This could silently pass if an unexpected error occurs. For the "type override succeeds" case, you likely want to assert success explicitly.Proposed fix
// If no expected parts, just log the error if it exists - if err != nil { - t.Logf("Error occurred: %v", err) - } + assert.NoError(t, err, "Expected no error when expectedParts is nil") }) }🤖 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 153 - 180, The test currently only logs errors when tt.expectedParts is empty, which can hide unexpected failures; change the branch after the comment "// If no expected parts, just log the error if it exists" to assert no error instead—call assert.NoError(t, err) (or require.NoError if you prefer test-fatal) for the case where tt.expectedParts is nil/empty (this affects the table-driven test that calls tt.setupFunc()), so unexpected errors (e.g., in the "type override succeeds" case) will fail the test instead of being silently logged.tests/merge_type_override_test.go (1)
106-109: Assert element contents, not just length.Per coding guidelines,
assert.Lenalone can mask regressions that corrupt slice contents. Assert at least first/last element values.Proposed fix
// allowed_accounts should be the original list from defaults. accounts, ok := vars["allowed_accounts"].([]any) require.True(t, ok, "allowed_accounts should be a list in dev (no override)") - assert.Len(t, accounts, 2, "allowed_accounts should have 2 entries from defaults") + require.Len(t, accounts, 2, "allowed_accounts should have 2 entries from defaults") + assert.Equal(t, "111111111111", accounts[0], "first allowed_account should match defaults") + assert.Equal(t, "222222222222", accounts[1], "second allowed_account should match defaults") })As per coding guidelines: "For slice-result tests, assert element contents, not just length."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/merge_type_override_test.go` around lines 106 - 109, The test currently only checks the length of vars["allowed_accounts"] (accounts) using require.True and assert.Len; update it to also assert the actual contents of the slice to prevent silent regressions by checking element values (e.g., assert.Equal on accounts[0] and accounts[1] or the first/last entries). Locate the block that reads vars["allowed_accounts"] into accounts and replace or extend the assert.Len with additional assertions that verify the expected string values for the elements in accounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/fixes/2026-03-24-describe-affected-upload-timeout.md`:
- Around line 177-181: Update the table/adjacent text for the "describe
affected" benchmark to clarify scope: add a short note stating that the
`--upload` flag implicitly forces inclusion of dependents (i.e., behaves like
`--include-dependents`) so it follows the same payload/bottleneck and thus the
same timing improvements; reference the `describe affected` command and the
`--include-dependents` and `--upload` flags when adding this one-line
clarification.
---
Nitpick comments:
In `@docs/fixes/2026-03-24-describe-affected-upload-timeout.md`:
- Around line 20-21: Update the symptom sentence to match observed behavior by
replacing “hang indefinitely” with wording that reflects the measured timeout
window (e.g., “hangs for ~40–50 minutes before timing out or being killed”);
ensure the sentence about the --upload flag states that the command hangs for
that observed duration on large infrastructures rather than claiming an
indefinite hang.
In `@internal/exec/validate_stacks_test.go`:
- Around line 153-180: The test currently only logs errors when tt.expectedParts
is empty, which can hide unexpected failures; change the branch after the
comment "// If no expected parts, just log the error if it exists" to assert no
error instead—call assert.NoError(t, err) (or require.NoError if you prefer
test-fatal) for the case where tt.expectedParts is nil/empty (this affects the
table-driven test that calls tt.setupFunc()), so unexpected errors (e.g., in the
"type override succeeds" case) will fail the test instead of being silently
logged.
In `@tests/merge_type_override_test.go`:
- Around line 106-109: The test currently only checks the length of
vars["allowed_accounts"] (accounts) using require.True and assert.Len; update it
to also assert the actual contents of the slice to prevent silent regressions by
checking element values (e.g., assert.Equal on accounts[0] and accounts[1] or
the first/last entries). Locate the block that reads vars["allowed_accounts"]
into accounts and replace or extend the assert.Len with additional assertions
that verify the expected string values for the elements in accounts.
🪄 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: 1824ebff-ca67-496f-856a-51e4f19b76f8
📒 Files selected for processing (7)
docs/fixes/2026-03-24-describe-affected-upload-timeout.mdinternal/exec/describe_dependents_index.gointernal/exec/describe_dependents_index_test.gointernal/exec/validate_stacks_test.gopkg/merge/merge_native.gopkg/merge/merge_type_override_test.gotests/merge_type_override_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/exec/describe_dependents_index.go
- internal/exec/describe_dependents_index_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/merge/merge_type_override_test.go
- pkg/merge/merge_native.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2248 +/- ##
==========================================
+ Coverage 77.18% 77.23% +0.05%
==========================================
Files 1014 1015 +1
Lines 95886 96019 +133
==========================================
+ Hits 74005 74159 +154
+ Misses 17700 17671 -29
- Partials 4181 4189 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Fix doc: "hang indefinitely" → "stall for 40-50 minutes" - Fix doc: add note that --upload forces IncludeDependents - validate_stacks_test: assert NoError when expectedParts is nil - Integration test: assert element contents, not just length Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/fixes/2026-03-24-describe-affected-upload-timeout.md (2)
168-173: Tiny docs style polish: usevs.in the table header.For US English consistency, change
Speedup vs previoustoSpeedup vs. previous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/fixes/2026-03-24-describe-affected-upload-timeout.md` around lines 168 - 173, Update the Markdown table header cell "Speedup vs previous" to "Speedup vs. previous" for US English consistency; locate the table header row containing the exact string "Speedup vs previous" (the column header in the table that also appears alongside "Step", "Time", and "Cumulative speedup") and replace that cell text only, leaving the rest of the table and formatting intact.
29-35: Prefer symbol references over hard-coded source line numbers.These docs cite exact line numbers in Go files (e.g., “lines 220-222”). Those drift quickly and become stale. Consider referencing function names/flow only so the fix note stays accurate longer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/fixes/2026-03-24-describe-affected-upload-timeout.md` around lines 29 - 35, Replace hard-coded source line numbers in the note with symbol-based references: instead of "lines 220-222" and "line 285-289" and "line 138", reference the functions and call flow (e.g., the code path that sets IncludeDependents=true and IncludeSettings=true, the addDependentsToAffected function which iterates affected components, the ExecuteDescribeDependents function, and the fact that ExecuteDescribeDependents calls ExecuteDescribeStacks). Update the text to describe the flow and functions by name and remove any explicit line numbers so the doc remains correct as code moves.internal/exec/validate_stacks_test.go (1)
133-149: Rename/simplify this case to match its new purpose.This block now validates a success-path contract, but the surrounding shape (
TestMergeContextErrorFormatting,expectedParts) still reads like error-formatting verification. Consider renaming and flattening it to a direct “defined contract” success test for clarity.Based on learnings: Applies to **/*_test.go : Contract vs. legacy behavior: if a test says 'matches mergo' (or any other library), add an opt-in cross-validation test behind a build tag (e.g.,
//go:build compare_mergo); otherwise state 'defined contract' explicitly so it's clear the native implementation owns the behavior.Also applies to: 176-177
🤖 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 133 - 149, Rename and simplify the subcase currently named "type override succeeds without error" (inside TestMergeContextErrorFormatting) into a standalone success test that asserts ValidateStacks(atmosConfig) returns nil (e.g., TestValidateStacks_TypeOverrideSucceeds or TestValidateStacks_DefinedContract); remove the error-formatting scaffolding like expectedParts and the table-driven placement and instead call ValidateStacks directly and assert no error, and if you want an opt-in cross-validation against mergo add a separate test guarded by a build tag (//go:build compare_mergo) as suggested for the mergo comparison lines referenced around 176-177.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/fixes/2026-03-24-describe-affected-upload-timeout.md`:
- Around line 168-173: Update the Markdown table header cell "Speedup vs
previous" to "Speedup vs. previous" for US English consistency; locate the table
header row containing the exact string "Speedup vs previous" (the column header
in the table that also appears alongside "Step", "Time", and "Cumulative
speedup") and replace that cell text only, leaving the rest of the table and
formatting intact.
- Around line 29-35: Replace hard-coded source line numbers in the note with
symbol-based references: instead of "lines 220-222" and "line 285-289" and "line
138", reference the functions and call flow (e.g., the code path that sets
IncludeDependents=true and IncludeSettings=true, the addDependentsToAffected
function which iterates affected components, the ExecuteDescribeDependents
function, and the fact that ExecuteDescribeDependents calls
ExecuteDescribeStacks). Update the text to describe the flow and functions by
name and remove any explicit line numbers so the doc remains correct as code
moves.
In `@internal/exec/validate_stacks_test.go`:
- Around line 133-149: Rename and simplify the subcase currently named "type
override succeeds without error" (inside TestMergeContextErrorFormatting) into a
standalone success test that asserts ValidateStacks(atmosConfig) returns nil
(e.g., TestValidateStacks_TypeOverrideSucceeds or
TestValidateStacks_DefinedContract); remove the error-formatting scaffolding
like expectedParts and the table-driven placement and instead call
ValidateStacks directly and assert no error, and if you want an opt-in
cross-validation against mergo add a separate test guarded by a build tag
(//go:build compare_mergo) as suggested for the mergo comparison lines
referenced around 176-177.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06417084-c8a9-487e-a87f-1f7be1bc8c97
📒 Files selected for processing (3)
docs/fixes/2026-03-24-describe-affected-upload-timeout.mdinternal/exec/validate_stacks_test.gotests/merge_type_override_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/merge_type_override_test.go
milldr
approved these changes
Mar 25, 2026
osterman
approved these changes
Mar 25, 2026
|
These changes were released in v1.212.0. |
Member
|
@aknysh thanks for catching this |
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
Two critical fixes addressing regressions and performance issues:
1. Deep merge type-mismatch regression
deepMergeNativethat rejected stack configs where lists are overridden with{}(empty map), scalars, or null — while overriding a list with{}is technically a misconfiguration (the correct way is[]), this pattern exists in production configs and worked with the previous mergo-based mergeallow_ingress_from_vpc_accounts: {}overriding a list of maps — the correct way would beallow_ingress_from_vpc_accounts: [], but the{}pattern worked before and must not break in a patch release2.
describe affected --uploadtimeout on large infrastructures (~250x speedup)--uploadforces--include-dependents, which calledExecuteDescribeDependentsfor every affected component — each call did a fullExecuteDescribeStacksresolution from scratch with no cachingExecuteDescribeStacksresult: called once instead of N times (40+ min → ~3.5 min)ExecuteDescribeComponentper item (~3.5 min → ~1:54)why
Deep merge regression
{}(empty map) instead of[](empty list) — while this is a misconfiguration, it worked with the previous mergo-based merge and must not break in a patch releaselist stacks,describe stacks, etc.) fail on affected configsdescribe affected --upload timeout
--uploadflag is used by Atmos Pro integration to upload affected stacksTest results
Deep merge type-override
merge-type-override) with 3 override patternsatmos list stackssucceedsdescribe affected --upload optimization
describe affected(no dependents)describe affected --include-dependentsfindDependentsByScan95.3%,findDependentsFromIndex100%,buildDependencyIndex81.1%)references
docs/fixes/2026-03-24-deep-merge-type-mismatch-regression.md: detailed analysis of the merge regressiondocs/fixes/2026-03-24-describe-affected-upload-timeout.md: detailed analysis with incremental timing breakdownSummary by CodeRabbit
Bug Fixes
Documentation
Tests