Isolate nested IoC scope bindings - #9869
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
c47e419 to
308e61f
Compare
11477fd to
12ba01b
Compare
🔗 Linked Issue RequiredThanks for the contribution! Please link a GitHub issue to this PR by adding |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
All reviewed changes are covered by isolation tests, with no unresolved issues.
Review tier: Balanced
Findings: None
What changed in this PR
Isolates nested IoC registrations while preserving inherited singleton instances.
Changes:
- Clones binding maps when creating child containers.
- Adds regression tests for parent, child, and sibling scope isolation.
| File | Description |
|---|---|
cli/azd/pkg/ioc/container.go |
Prevents child registrations from mutating parent bindings. |
cli/azd/pkg/ioc/container_test.go |
Verifies scope isolation and singleton inheritance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
12ba01b to
64792d1
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Marina He (hemarina)
left a comment
There was a problem hiding this comment.
The underlying golobby/container type is map[reflect.Type]map[string]*binding. Previously, maps.Copy(current, parent.inner) only copied the outer map — each current[type] still pointed at the same inner map[string]*binding as the parent, so any bind() call (e.g. RegisterInstance) mutated that shared map in place, leaking overrides into the parent/siblings.
Cloning the inner map per type (maps.Clone(bindings)) isolates overrides correctly, while un-overridden entries still share the same *binding pointer — preserving singleton caching/sharing across parent/child (as Test_NewNestedContainer_InheritsParent verifies). This is the minimal correct fix, not over- or under-cloning.
Test coverage: Good — Test_Container_LayerEnvironmentManagerOverridesAreIsolated is a solid regression test matching the real infra-provider scenario. Worth calling out explicitly in the PR description: the existing assertion in Test_Container_Singleton_Instance_Register_Resolve (NotSame -> Same) wasn't just "updated" — it was asserting the bug's symptom as correct behavior before this fix. A sentence noting that would help reviewers not read it as an unrelated test change.
Optional nit: A short doc-comment on NewNestedContainer stating the invariant ("bindings are isolated per scope on override; unmodified singleton bindings remain shared with the parent") would help this not silently regress again.
64792d1 to
068a5d0
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is sound, and the test assertion concern is non-blocking.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
cli/azd/pkg/ioc/container_test.go — This assertion only compares the two values created by the test, so it passes even if either scope… |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Shared singleton bindings leave a critical cross-scope leakage path unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
cli/azd/pkg/ioc/container.go — maps.Clone isolates only the name map; every value remains the same *binding. In… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
cli/azd/pkg/ioc/container_test.go — This assertion only compares the two values created by the test, so it passes even if either scope… View resolved comment |
| for registeredType, bindings := range parent.inner { | ||
| // clone bindings as well, otherwise changes in child scopes will | ||
| // actually modify the map for the parent! | ||
| current[registeredType] = maps.Clone(bindings) |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|


This PR fixes a bug in how we created nested scopes. The shallow copy we did, prior to this PR, made it possible for child scopes to affect sibling or root scopes when rebinding a type.