fix: read unified azure.yaml in agent run and deploy#9149
Conversation
huimiu
requested review from
JeffreyCA,
RickWinter,
glharper,
hemarina,
tg-msft,
therealjohn,
trangevi,
trrwilson and
vhvb1989
as code owners
|
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
Unifies agent configuration resolution across run, deploy, provision, and synthesis while retaining legacy configuration support.
Changes:
- Resolves
$reffiles and typed service environment values. - Applies normalized configuration across lifecycle paths.
- Adds coverage for referenced services and resource definitions.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/synthesis/synthesizer.go |
Resolves referenced synthesis inputs. |
internal/synthesis/synthesizer_test.go |
Tests referenced synthesis resources. |
internal/project/service_update.go |
Serializes project service updates. |
internal/project/service_target_agent.go |
Uses resolved deploy configuration. |
internal/project/service_target_agent_test.go |
Tests deployment and build resolution. |
internal/project/resource_group_location_check.go |
Resolves referenced brownfield endpoints. |
internal/project/resource_group_location_check_validate_test.go |
Tests referenced endpoint validation. |
internal/project/foundry_provisioning_provider.go |
Resolves provisioning references. |
internal/project/foundry_provisioning_provider_test.go |
Tests referenced Foundry endpoints. |
internal/project/config.go |
Supports toolbox service-name values. |
internal/project/agent_definition.go |
Centralizes effective agent resolution. |
internal/project/agent_definition_test.go |
Tests unified, legacy, and referenced definitions. |
internal/pkg/projectconfig/environment.go |
Loads and normalizes raw service environments. |
internal/pkg/projectconfig/environment_test.go |
Tests typed and referenced environments. |
internal/cmd/run.go |
Applies unified environment values locally. |
internal/cmd/run_test.go |
Tests service environment resolution. |
internal/cmd/resource_services.go |
Resolves referenced Foundry resources. |
internal/cmd/resource_services_test.go |
Tests referenced resource collection. |
internal/cmd/optimize_apply.go |
Rejects updates to referenced definitions. |
internal/cmd/listen.go |
Updates provision and deploy hooks. |
internal/cmd/listen_test.go |
Tests referenced container settings. |
internal/cmd/helpers.go |
Builds resolved local-run context. |
internal/cmd/helpers_test.go |
Tests raw unified environment loading. |
.vscode/cspell.yaml |
Adds the new package name. |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
One question on a behavior change that's outside what the description covers. Everything else (the $ref/unified resolution across run, deploy, provision, and the env precedence handling) reads cleanly and is well covered by tests.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
The new commit restores the hosted-agent session carryover (capture in predeploy, resume in postdeploy) that the first commit had dropped, so my earlier question is resolved. The restored wiring matches what's on main. Approving.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
The new commit swaps the persist/restore state mutation for a per-request service_path override on the container build, package, and publish calls. The server treats it as a one-operation override without touching the stored config, rejects path traversal via filepath.IsLocal, and falls back to protowire unknown fields for wire compatibility. That resolves my earlier question about how the referenced-service path was being carried into the container operations. Re-approving against the latest commit.
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Re the automated flag about container RPC version compatibility: this looks already handled. The extension sets requiredAzdVersion: ">=1.28.0", and the matching core ContainerOperationOptions (field 3) handling ships in this same PR (1.28.0-beta.1). Any host that can load the updated extension will understand the new options field, so the resolved service path won't silently no-op on a supported host, and there's no need to split the extension change out or bump to a later SDK for that reason.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Re-verified the outstanding automated flags against the current head.
The container RPC options field (ContainerOperationOptions, field 3) is understood by any host that can load this extension, since it pins requiredAzdVersion >= 1.28.0 and the matching core handling ships in this same PR, so the resolved service path won't silently no-op on a supported host.
The docker $ref hydration and non-agent $ref resolution notes are narrow edge cases: docker overrides supplied only through a referenced file, or a non-Foundry service carrying an incompatible remote $ref. Neither matches the service shapes this extension emits today. Worth a follow-up if referenced docker blocks become a supported pattern, but not blocking here.
Approving.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Following up on my earlier note about container RPC version compatibility, which I need to correct.
I previously said the core ContainerOperationOptions (field 3) handling ships in this same PR, so any host meeting the required version would understand it. That isn't right. The core reader lives in #9170 (container_service.go and container.proto), which is still open against main. azd core 1.28.0 was released on 2026-07-15 without it, and field 3 is absent from both the 1.28.0 tag and main.
With requiredAzdVersion: ">=1.28.0", a user on the released 1.28.0 can install this extension, but core treats field 3 as an unknown field and drops the resolved service path. The container build, package, and publish then run against the unresolved settings, silently. Worth pinning the gate to the first release that actually contains #9170 rather than 1.28.0.
Minor and non-blocking: the def/service env merge in run.go builds an exact-case map, so on Windows a definition PATH plus a service Path both land in the slice. The sort emits PATH first and envSliceHasKey's case-insensitive check then suppresses Path, so the definition value wins over the service value. Narrow case (same key, different casing, Windows only).
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
huimiu
force-pushed
the
hui/unified-config-runtime
branch
from
abe8c3c to
6457366
Compare
| resolved, err := resolveServiceProps( | ||
| props, | ||
| svc.GetName(), | ||
| projectRoot, | ||
| ) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if structHasKind(resolved) { | ||
| return true, nil | ||
| } |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
My earlier block was the container options passed through proto field 3, whose core reader ships in #9170 rather than released 1.28.0, so it would silently no-op on a 1.28.0 host. This revision drops that injection and the AddService round-trip, and relies on the service config declared in azure.yaml instead, so there's no dependency on an unreleased core reader.
The typed env change is applied to both the extension schema and the v1.0 service schema, and the new root `` core-field validation is now consistent between synthesis and run/deploy. Verified the affected extension packages pass locally (the only failures are the bicep/ARM template regeneration checks, which need the bicep toolchain and are unrelated to these changes).
trangevi
left a comment
trangevi
left a comment
There was a problem hiding this comment.
Generally looks good, I very much appreciate the work that's been done to simplify the changes we're making here. Most of my comment are effectively the same, regarding unnecessary wrapper methods, so should hopefully be easy to resolve in bulk.
| @@ -355,10 +357,26 @@ func reserveServiceName(used map[string]string, name, source string) error { | |||
| // service carries any, so an azure.yaml written before the per-resource split | |||
| // still provisions without re-running init. | |||
| func collectProjectDeployments(services map[string]*azdext.ServiceConfig) ([]project.Deployment, error) { | |||
There was a problem hiding this comment.
This wrapper seems unnecessary since there's no conditional handling, we should just either update this method to the new contract, or change all the callers to use the new method
| // agent service when no connection service carries any, so a pre-split | ||
| // azure.yaml still provisions without re-running init. | ||
| func collectConnections(services map[string]*azdext.ServiceConfig) ([]project.Connection, error) { | ||
| return collectConnectionsAtRoot(services, "") |
There was a problem hiding this comment.
Same comment about the unnecessary wrapper method
| svc *azdext.ServiceConfig, | ||
| projectRoot string, | ||
| ) error { | ||
| _, err := prepareContainerSettings(svc, projectRoot) |
There was a problem hiding this comment.
Same comment as the other wrapper methods, seems like unnecessary overhead.
|
|
||
| // NormalizeServiceConfigInPlace converts environment scalars without | ||
| // resolving nested file references. | ||
| func NormalizeServiceConfigInPlace(svc *azdext.ServiceConfig) error { |
There was a problem hiding this comment.
Can you please expand on the comments for both ResolveServiceConfigInPlace and NormalizeServiceConfigInPlace to give a bit more detail on what they are doing and why?
| err, | ||
| ) | ||
| } | ||
| if err := projectconfig.NormalizeEnvironment(resolved); err != nil { |
There was a problem hiding this comment.
422-437 look almost identical to what normalizeServiceProps, is there a way to have some code reuse?
| svc *azdext.ServiceConfig, | ||
| projectRoot string, | ||
| ) (string, error) { | ||
| _ = projectRoot |
There was a problem hiding this comment.
What is the point of this? Why take the parameter if we do nothing with it?
| } | ||
|
|
||
| if inline.Kind != agent_yaml.AgentKindHosted { | ||
| // Validate non-hosted kinds (e.g. workflow) with the same |
There was a problem hiding this comment.
Do we support workflow agents in any way? What's the value of adding this handling now if we don't actually support anything else yet?
| exterrors.CodeInvalidAgentManifest, | ||
| fmt.Sprintf("agent service definition is not valid: %s", err), | ||
| "fix the agent service entry in azure.yaml or "+ | ||
| "re-run `azd ai agent init`", |
There was a problem hiding this comment.
Re-running init, depending on when this method is being called, may not be a clean solution to fixing problems. Especially if this method is called as a part of init, a user is likely to just end up back in this spot again even if a rerun does work well. As long as the returned error provides what is wrong with the service definition, it's probably cleanest to just leave this message with the guidance to fix the definition
| // to learn which model deployments to create on the existing account. Returns | ||
| // nil (not an error) when the service declares no deployments. | ||
| func BrownfieldDeployments(raw []byte, serviceName string) ([]Deployment, error) { | ||
| return BrownfieldDeploymentsAtRoot(raw, "", serviceName) |
There was a problem hiding this comment.
Same comment about the unnecessary wrapper
| // expressions pass through. Returns an empty slice (not an error) when none | ||
| // are declared. | ||
| func BrownfieldConnections(raw []byte, env map[string]string) ([]Connection, error) { | ||
| return BrownfieldConnectionsAtRoot(raw, "", env) |
There was a problem hiding this comment.
Same comment about the unnecessary wrapper
Why this change is needed
Azure AI Agents services can now be defined directly in
azure.yaml, under the deprecatedconfig:block, or in local files referenced through$ref. These forms were not resolved consistently across local run, deploy, and provision. As a result, a valid project could lose environment expressions or value types, skip referenced resources, or use inconsistent agent configuration.What changed
config:, and local$refagent configuration.azure.ymlsupport while retaining compatibility withagent.yamlandagent.ymlprojects.azd ai agent optimize applyreject only root agent-definition references, avoiding writes to the wrong file without blocking unrelated nested references.Configuration contract
A root
$refmay provide agent-owned configuration such as the agent definition, protocols, environment settings, deployments, connections, and Foundry-specific settings. Core-owned execution fields (project,language,image, anddocker) must remain inline on the service entry inazure.yaml.Why this approach
Core azd selects frameworks and executes build, package, and deploy operations from the service entry it reads directly. Keeping execution fields inline makes that lifecycle deterministic and avoids a new cross-boundary API. The extension still resolves its own configuration without rewriting user files.
Testing
From
cli/azd/extensions/azure.ai.agents:go test ./internal/cmd ./internal/pkg/projectconfig ./internal/project ./internal/synthesisgo build ./...Closes #9151.
Part of #8710.