fix(ai-agents): preserve service fields during container defaults - #9211
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>
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 |
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.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Re-checked at d08d5b8. The targeted SetServiceConfigValue writes only the container leaf on the live services map, so hooks, templated image, and other sibling keys survive instead of being dropped. The direct services[name] lookup fixes the old dotted-path split (e.g. my.agent), and configMutationMu now wraps the full load/save/reload so concurrent mutations can't clobber each other. Tests cover the inline, legacy, dotted-name, and concurrent paths.
…leases (#9371) * chore: prepare azure.ai.agents 1.0.0-beta.8 release Bump version and add changelog for 10 PRs since beta.7: - Features: max_stalls early-stopping (#9314), invocations protocol 2.0.0 default (#9327) - Bug fixes: #9365, #9328, #9291, #9290, #9212, #9211, #9280, #9237 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * chore: prepare azure.ai.projects 1.0.0-beta.4 release Bump version and add changelog for 1 PR since beta.3: - Bug fix: cap Foundry ARM deployment names to 64 chars (#9292) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * chore: add imatiach to cspell contributor aliases Fix cspell-ext CI gate failure for the azure.ai.agents CHANGELOG entry referencing @imatiach-msft (PR #9314). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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