feat: add service-scoped env to Foundry extensions - #9079
Conversation
Co-authored-by: huimiu <107838226+huimiu@users.noreply.github.com>
Co-authored-by: huimiu <107838226+huimiu@users.noreply.github.com>
jongio
left a comment
There was a problem hiding this comment.
Re-ran the full pass on de05ddf9. Build, vet and tests are clean in all four modules at this commit and CI is green. The two MatchesBicepBuild failures I hit locally also fail on the base commit 7d34c7e, so those are a local bicep version difference and not from this branch.
One new thing, on the legacy definition path. ResolveAgentEnvironmentVariable falls back to the project-wide azd environment even when the service declares an isolated env: scope, which is the opposite of what connectionEnvironmentMapping does in the projects extension in this same PR. Detail inline.
My earlier threads on the agents GetServiceConfigValue error path, the empty generated env: block, the provisioning provider ordering and wording, and ${A:-${B}} collection are unchanged by this commit and still stand. My approve on de05ddf9 stays as is.
| if mapping == nil { | ||
| return "" | ||
| } | ||
| return mapping(variableName) |
There was a problem hiding this comment.
When the name isn't in serviceEnvironment this always falls through to mapping, and both callers hand it the full azd environment: run.go:552 passes azdEnvVars[varName] and service_target_agent.go:2712 passes azdEnv[varName]. Neither caller knows whether the service declared env:.
The projects extension goes the other way in this same PR. Once declared is true, connectionEnvironmentMapping returns serviceEnvironment[name] with no project fallback, so a connection service with env: {} resolves ${FOO} to empty instead of picking up a project-wide FOO.
Net effect: a hosted agent with env: {} plus a legacy environment_variables: [{name: TARGET, value: "${FOO}"}] still resolves FOO from the project environment, at run and at deploy.
Deliberate for the deprecated environment_variables block, or should it follow the connection rule? run.go already carries runCtx.HasServiceEnvironment that could be threaded in, and the deploy path would need the same probe the routines and toolboxes targets do. If it is deliberate, worth saying so on the doc comment, because ResolveAgentEnvironmentVariable reads as scope-aware right now.
jongio
left a comment
There was a problem hiding this comment.
Incremental review of the 5 commits since fea0f498.
internal/cmd/env_refs.go:96 - environmentReferenceAt doesn't verify value[start+1] == '{', so $name} parses as a reference with the first character dropped. That writes phantom entries into the generated env: block, and on $ab:-${REAL}} it swallows the real REAL reference so init never prompts for it. Repro and a fix inline; your existing tests pass with the change.
Still open from my last pass: internal/cmd/helpers.go:1000 swallows the GetServiceConfigValue error, so a transient RPC failure makes a service that declared env: {} fall back to the full azd environment. The routines and toolboxes targets fail closed on the same call.
Replied to @trangevi on the escaping constants.
d3705f4 to
4f00973
Compare
jongio
left a comment
There was a problem hiding this comment.
Two notes on env_refs_test.go.
TestFindEnvironmentReferencesMatchesExpander is the guard for the scanner/expander contract, and every value in its list has $ immediately followed by {. That shape never reaches the environmentReferenceAt path from the open thread on line 107 of env_refs.go, so the guard passes while the contract it states is already broken.
The doc comment on TestNestedDefaultIsNotDiscovered says the outer name reaches init prompting, but the assertion below it says otherwise.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
cli/azd/extensions/azure.ai.agents/internal/cmd/helpers.go:999
- azd-code-reviewer: Do not treat a failed
GetServiceConfigValuecall as a legacy service. If this RPC fails for a service withenv: {},HasServiceEnvironmentremains false andmergeAgentRunEnvironmentinjects the full active azd environment into the child process, defeating service isolation and potentially exposing unrelated secrets. Return the error instead.
if resp, envErr := azdClient.Project().GetServiceConfigValue(
cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go:250
- azd-code-reviewer: The PR description says this scanner has a 588-input differential test against both former implementations, but this file contains only the table cases and this one-way check. This assertion only verifies that reported names are expanded, so an implementation that misses a live reference—or returns no references at all—still passes. Add the claimed generated differential/completeness coverage so the shared scanner cannot silently omit variables from both prompting and generated
envblocks.
values := []string{
jongio
left a comment
There was a problem hiding this comment.
One thing the escaping comment leaves out. resolveVars diverges on :- as well as on $$, and that half breaks the scanner's own policy line for the three ignoreEnvironmentEscaping fields.
trangevi
left a comment
There was a problem hiding this comment.
Approved, pending addressing Jon's remaining comments
jongio
left a comment
There was a problem hiding this comment.
Seven threads from my earlier passes are still open on this HEAD with no replies. Listing them so they don't get lost now that the PR is showing approved.
env_refs.goresolveVarsdivergence: I added detail on that thread. The:-half fails silently, and a value the user was prompted for ends up in the Bicep param as literal text.helpers.gothe service env read swallows the RPC error and leaveshasServiceEnvironmentfalse, so the run path falls back to the full azd environment, which is the leak this PR closes. Routines and toolboxes return the error instead.agent_definition.goResolveAgentEnvironmentVariablefalls through to the callermappingfor any name a declaredenv:doesn't carry, and both callers hand it the full azd environment.resource_services.gosetServiceEnvironmentreturns before writing when collection comes back empty, so a service with no${VAR}references gets noenv:key and reads as legacy at run time.env_refs.goenvironmentReferenceAtnever checks that the byte after$is{."$HOME}"reportsOMEwhilefoundry.ExpandEnvleaves the value untouched, which is the exact invariantTestFindEnvironmentReferencesMatchesExpanderasserts.env_refs_test.goevery value in that parity corpus has$followed by{, so it can't catch the case above.env_refs_test.gotheTestNestedDefaultIsNotDiscoveredcomment says the outer name reaches init prompting, but the assertion four lines down isrequire.Empty.
None of these are new asks, they're the same threads. The two scanner ones and the two test ones are narrow. The three that fall back to the full azd environment are the ones I'd want settled before this merges, since they all undo the isolation the PR is adding.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.agents/internal/cmd/run.go:579
azd-code-reviewer: This no longer resolves service-scoped Foundry connection references for local runs. A forwarded value such as${{connections.search.credentials.key}}is only cloned here (the only replacement below isproject.endpoint). Although the same entry is resolved intodefEnv,mergeAgentRunEnvironmentdiscards it because the key exists inserviceEnvironment, so the child receives the literal expression instead of the credential. Route service-environment entries through the existing connection-reference resolution before giving them precedence.
func resolveLocalServiceEnvironment(
environment map[string]string,
endpoint string,
) map[string]string {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.agents/internal/cmd/run.go:1128
- Service-scoped connection expressions are resolved into
definitionEnvironment, but this branch discards that resolved entry whenever the same key exists inserviceEnvironment. The child therefore receives the forwarded literal${{connections...}}instead of the credential value; the previousresolveServiceEnvironmentVarspath resolved these expressions before merging. Merge the resolved definition entry for service-scoped keys (while retaining process/command precedence).
for _, entry := range definitionEnvironment {
key, _, _ := strings.Cut(entry, "=")
_, serviceScoped := serviceEnvironment[key]
if serviceScoped {
continue
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
cli/azd/extensions/azure.ai.agents/internal/cmd/run.go:1129
azd-code-reviewer: Service-scoped Foundry connection expressions are left literal here. A raw env value such asAPI_KEY: $${{connections.search.credentials.key}}is forwarded as${{...}};resolveAgentDefinitionEnvVarsresolves its merged definition entry to the credential, but this branch then discards that resolved entry because the key is service-scoped, leaving the literal expression in the child process. Preserve service precedence after resolving these expressions (the previous service-env path resolved connection refs before merging), and add an end-to-end merge test for this case.
for _, entry := range definitionEnvironment {
key, _, _ := strings.Cut(entry, "=")
_, serviceScoped := serviceEnvironment[key]
if serviceScoped {
continue
}
cli/azd/extensions/azure.ai.agents/internal/project/agent_definition_test.go:859
azd-code-reviewer: This test referencesos.Stdout, whichcli/azd/.golangci.yaml:18-21explicitly forbids in test files;agent_definition_test.gois not covered by the stdout-test allowlist, so lint will fail. Pass anio.Writerinto the warning helper and assert with abytes.Bufferinstead of swapping global stdout.
original := os.Stdout
os.Stdout = writer
defer func() { os.Stdout = original }()
cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go:680
azd-code-reviewer: This declaration check runs afterserviceForHostexpands$ref, so anenv:placed in a referenced connection file is treated as scoped even though azd core only forwards/expands core service fields from the rootazure.yamlentry.serviceEnvironments[name]is therefore empty and every${VAR}in that connection resolves to""instead of the legacy environment. Rejectenvas a core field in connection root refs, or determine scope from the unexpanded root node before selecting this mapping.
declared := len(serviceEnvironments[name]) > 0 || connectionEnvDeclared(node)
mapping := connectionEnvironmentMapping(
env,
serviceEnvironments[name],
declared,
cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go:680
azd-code-reviewer: This declaration check runs afterserviceForHostexpands$ref, so anenv:placed in a referenced connection file is treated as scoped even though azd core only forwards/expands core service fields from the rootazure.yamlentry.serviceEnvironments[name]is therefore empty and every${VAR}in that connection resolves to""instead of the legacy environment. Rejectenvas a core field in connection root refs (as the agent path now does), or determine scope from the unexpanded root node before selecting this mapping.
trangevi
left a comment
There was a problem hiding this comment.
Approving, pending addressing Jon's comments, and with a possible suggestion to consider now or as a follow up, if it would indeed make things easier.
| // alone. "$ab:-${REAL}}" is the costly shape, because the | ||
| // phantom would span the string and hide the live REAL. | ||
| func environmentReferenceAt(value string, start int) (environmentReference, bool) { | ||
| if start+1 >= len(value) || value[start+1] != '{' { |
There was a problem hiding this comment.
Maybe regex would be easier here? and in some of the other places where we're introducing these kinds of checks. Idk for sure that it would be, just throwing it out there to consider
jongio
left a comment
There was a problem hiding this comment.
Four earlier issues are still open on this HEAD:
foundry_provisioning_provider.go: on-disk and ejected provisioning still substitutes only the project environment, so service aliases can remain unresolved or bind project-wide values.resource_services.go: init still skips writingenv: {}when no references are collected, so generated services fall back to the full azd environment locally.foundry_provisioning_provider.go: subscription and location prompting still happens before project service validation, obscuring configuration errors, especially under--no-prompt.foundry_provisioning_provider.go: the broadened$reffailure still reports an existing-endpoint error for greenfield service configuration.
These paths are unchanged by the latest commits, so I can't approve this revision yet.
🤖 agent jongio
Summary
Foundry extensions (agent, connection, routine, toolbox) now read service-scoped environment values from
ServiceConfig.Environmentinstead of the project-wide azd environment, and write generated variables into the standardservices.<name>.envblock.envkeep the old project-wide behavior.${VAR}scanners with one shared scanner.Follow-up to #8936. Fixes: #9231
Why this change
Service scoping. azd core now expands each service's environment before handing it to the extension. Extensions have to consume those values so services stop leaking variables into each other, while projects with no service-level
envkeep working unchanged.The empty-value bug.
azd upinitializes every service target while building the execution graph — before the user is prompted for a missing subscription or location. A config read at that moment holds empty strings, so the user answers the prompt and the extension still deploys a blank value, with no error anywhere. Two fixes: the agent target now uses the config passed to each deploy-time entrypoint instead of the copy cached atInitialize(re-running$refexpansion on it), and the provisioning provider resolves the azd environment before it reads service environments.One
${VAR}scanner. Writing anenv:block means deciding which variables a service references. #9212 answered a near-identical question — which variables to prompt for — with a second implementation, and the two had already drifted:env:block${A:-${B}}$count$They are now one scanner, pinned by a test to
foundry.ExpandEnv— the code that actually resolves these values — so it can't drift again. Escaping follows the expander too:$${VAR}is literal,$$${VAR}expands. The only remaining policy difference is one explicit line: init skips a reference that has a default, the env block records it.Nested references stay out of scope by design:
${OUTER:-${NESTED}}reportsOUTERonly.foundry.ExpandEnvstill resolvesNESTEDat deploy, but nothing discovers it, so it never reaches the generatedenv:block and init never prompts for it. That limitation is now stated in the scanner's doc comment and pinned by a test instead of left implicit — keep defaults literal.Environment terminology
envalso use them as the legacy fallback.envservices.<name>.env. Commands use the project config API to preserve them on disk.ServiceConfig.Environment. Consumed as-is, never expanded again.environmentVariablesor legacyagent.yaml. Serviceenvwins when both exist.Template syntax has distinct owners:
${VAR}is resolved by azd,${{...}}by Foundry, and$${{...}}preserves a Foundry expression through azd expansion.Manual Testing
azd ai agent init— migrated legacyenvironmentVariablestoenv, preserved${VAR:-default}, escaped Foundry templates as$${{...}}.azd ai agent run— serviceenvresolved for the child process while the template stayed intact on disk; legacy inline definitions resolved through the azd fallback.azd ai agent optimize apply— raw templates and scalars preserved through field-level updates.azd deploy— toolbox endpoint resolved from serviceenv, and from the azd fallback for a legacy toolbox.