fix(ai-agents): preserve service fields during container defaults#9211
fix(ai-agents): preserve service fields during container defaults#9211glharper wants to merge 6 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fed9e97b-e79b-4889-ac76-0d9a428599cd
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
|
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. |
There was a problem hiding this comment.
Pull request overview
Preserves user-authored agent service fields while persisting resolved container defaults.
Changes:
- Replaces full service updates with targeted container updates.
- Supports inline and legacy configuration layouts.
- Adds regression coverage for preserved fields and image templates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/cmd/listen.go |
Persists only container settings. |
internal/cmd/listen_test.go |
Tests both configuration layouts and preservation behavior. |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Targeted SetServiceConfigValue on just the container path is the right fix for #9152 - it keeps hooks and ${ENV} image templating that the old full-block AddService was dropping. One non-blocking note inline about the duplicated inline-vs-legacy detection.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fed9e97b-e79b-4889-ac76-0d9a428599cd
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
The refactor addresses my earlier note: the inline-vs-legacy selection now lives entirely in SetAgentContainerSettings, which returns the dotted path and value, so the listener no longer re-derives the shape or reads the value back. New test covers both the inline and legacy layouts. Wiring and error paths look correct.
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
Resolved and pushed in merge commit |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Re-checked after the merge with main and the container-settings conflict resolution. The net PR change is unchanged from my earlier review: the targeted SetServiceConfigValue writes into the resolved service sub-config (so dotted service names resolve correctly), SetAgentContainerSettings returns the path and value the listener persists without re-deriving the shape, and configMutationMu serializes concurrent config mutations. Tests for the inline and legacy layouts plus the dotted-name and concurrency cases are intact. CI is green.
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up commit. The container write now routes through ServiceConfigProps, so a legacy azure.ai.agent service that carries unrelated inline keys (no kind) still persists under config.container instead of landing in AdditionalProperties. The persist is also skipped when the service uses a root $ref, which avoids writing the resolved cpu/memory back as an inline container sibling next to the reference and defeating the file include. New tests cover both paths: the unrelated-inline legacy case and the no-persist-on-file-ref case. Targeted internal/cmd and internal/project tests pass locally.
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up commit. It adds a host-level regression test that drives a container.resources.cpu update through SetServiceConfigValue on an azure.ai.agent service, then reloads the saved config and asserts both the templated image and the predeploy/postdeploy hooks survive. That locks in the exact behavior this PR fixes for #9152. Test-only delta, no production changes, and it follows the parallel/t.Context patterns used by the neighboring cases.
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
|
/check-enforcer override |
|
|
||
| // Construct path to service config value: "services.<serviceName>.<path>" | ||
| servicePath := fmt.Sprintf("services.%s.%s", req.ServiceName, req.Path) | ||
| services, ok := cfg.Raw()["services"].(map[string]any) |
There was a problem hiding this comment.
What are the changes in this file actually fixing? Do we need to make modifications to core?
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
@trangevi this file changes the gRPC config API the extension now writes through. The #9152 fix moves the extension off the full-block AddService onto a targeted SetServiceConfigValue (that's what keeps the hooks and templated image), so the two changes here back that path:
-
Write target for dotted service names. The old code built
services.<name>.<path>and calledconfig.Set, which splits on.. A service whose name has a dot landed in nested keys (services->my->agent) instead of the realmy.agententry. The new code looks upservices[name]by exact key, then sets the relative path on that sub-config.TestProjectService_SetServiceConfigValue_DottedServiceNameasserts no straymykey gets created. -
Concurrent mutation safety.
configMutationMuserializes the load/modify/save cycle across the mutating methods on the shared project service, so overlapping writes don't clobber each other. Those methods don't call into each other, so the mutex won't deadlock.TestProjectService_SetServiceConfigValue_Concurrentcovers it.
Both are correctness gaps in core, and the extension can only reach config through this gRPC surface, so they can't move into the extension. Re-verified against HEAD d08d5b8 with CI green.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Re-verified against HEAD d08d5b8 with CI green. The targeted config write persists only the service's container key, so hooks, the templated image, and the agent definition survive; dotted service names resolve by exact key instead of dot-splitting; and the mutation mutex serializes the load/modify/save cycle without reentrancy since the handlers don't call into each other. Tests cover the inline, legacy, dotted-name, concurrent, and file-ref paths.
Summary
azure.yamlTesting
go test ./internal/cmd -count=1go build ./...golangci-lint run ./internal/cmd/...cspell lint internal/cmd/listen.go internal/cmd/listen_test.goFixes #9152