fix(ai-projects): route resolveVars through foundry.ExpandEnv - #9367
fix(ai-projects): route resolveVars through foundry.ExpandEnv#9367glharper wants to merge 1 commit 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 2 pipeline(s). 20 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
Aligns Foundry network environment expansion across the projects and agents extensions.
Changes:
- Uses
foundry.ExpandEnvfor network variables. - Adds default-reference and escaping support.
- Updates environment-reference discovery and tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
azure.ai.projects/internal/synthesis/synthesizer.go |
Updates network expansion. |
azure.ai.projects/internal/synthesis/synthesizer_test.go |
Adds expansion and eject-path tests. |
azure.ai.agents/internal/synthesis/synthesizer.go |
Mirrors synthesis changes. |
azure.ai.agents/internal/cmd/init_env.go |
Honors escaping for network fields. |
azure.ai.agents/internal/cmd/init_env_test.go |
Updates reference-scanning coverage. |
Comments suppressed due to low confidence (2)
cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go:1053
- [azd-code-reviewer] For escaped input
$${A}, returning an empty mapping makesExpandEnvproduce the literal${A}. The provision path then mistakes that literal for an unresolved reference and skips VNet/subscription shape validation, passing a malformed ID into ARM instead of failing locally. Distinguish preserved eject references from escaped literals after expansion and validate provision-path results.
if _, ok := required[name]; ok && unresolved == "" {
unresolved = name
}
return ""
cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go:1053
- [azd-code-reviewer] For escaped input
$${A}, returning an empty mapping makesExpandEnvproduce the literal${A}. The provision path then mistakes that literal for an unresolved reference and skips VNet/subscription shape validation, passing a malformed ID into ARM instead of failing locally. Distinguish preserved eject references from escaped literals after expansion and validate provision-path results.
if _, ok := required[name]; ok && unresolved == "" {
unresolved = name
}
return ""
| required := map[string]struct{}{} | ||
| for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) { | ||
| if match[2] == "" { | ||
| required[match[1]] = struct{}{} |
There was a problem hiding this comment.
Confirmed on this branch, this one isn't hypothetical. With an env that has no FOO:
$${FOO} ${FOO:-fallback} -> error: unresolved environment variable ${FOO}
${{connections.${FOO}.key}} ${FOO:-fallback} -> error: unresolved environment variable ${FOO}
Both should resolve to fallback. The escaped occurrence and the one inside the ${{...}} span never reach the callback, but they still seed required, and the defaulted reference then trips it.
That also makes the new doc comment on resolveVars inaccurate where it says names inside a Foundry ${{...}} span "cannot trip this check". They can, any time the same name also appears outside the span.
| required := map[string]struct{}{} | ||
| for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) { | ||
| if match[2] == "" { | ||
| required[match[1]] = struct{}{} |
jongio
left a comment
There was a problem hiding this comment.
Verified the parity test enforces the byte-identical agents/projects copy automatically, so that part of the scope note holds up on its own.
One new issue below, plus a confirmed repro for the escaped-reference case Copilot already flagged.
resolveVars uses varRefPattern to model what ExpandEnv will expand, but ExpandEnv runs the full drone/envsubst grammar. That model is wrong in both directions: it over-matches escaped and ${{...}} occurrences (Copilot's finding, confirmed below), and it under-matches every envsubst operator other than :-, which slips past the unresolved-variable guard silently.
| // those spans, so they cannot trip this check. | ||
| func resolveVars(s string, env map[string]string) (string, error) { | ||
| required := map[string]struct{}{} | ||
| for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) { |
There was a problem hiding this comment.
varRefPattern only models ${VAR} and ${VAR:-default}, but ExpandEnv runs the full drone/envsubst grammar. Any other operator form still gets expanded by envsubst, never lands in required, and never satisfies containsVarRef, so it slips past the unresolved-variable guard without a word.
Verified on this branch with an env that has no MISSING:
${MISSING:=default} -> "default", nil
${MISSING:+alt} -> "alt", nil
${MISSING:?boom} -> "boom", nil
${MISSING#prefix} -> "", nil
${MISSING:0:3} -> "", nil
On main these all stayed literal, so the ARM id and subscription shape checks rejected them with a message naming the offending value. Now peSubnet.vnet: "${MISSING#x}" silently becomes "", and dns.subscription: "${MISSING:=<guid>}" silently resolves without ever consulting the azd environment. Typing := instead of :- is a one character slip that now quietly succeeds while bypassing exactly the guard this PR sets out to preserve.
Related, same cause: ${MISSING-nodefault} now returns the raw envsubst error missing closing brace, which is confusing given the braces are closed.
This is the same root cause as the escaped-reference case Copilot flagged. Chasing envsubst's grammar with a local regex is going to keep drifting. It's probably cheaper to validate up front that these three fields only use ${VAR}, ${VAR:-default}, $${VAR}, or ${{...}}, and reject anything else with a clear error. That keeps required and containsVarRef accurate by construction.
Applies to the agents copy too, but the parity test keeps them in sync so one fix covers both.
Summary
resolveVarsthroughfoundry.ExpandEnv, the shared expander every other Foundry field uses, so${VAR:-default}and the$${VAR}escape now behave the same onnetwork.agentSubnet.vnet,network.peSubnet.vnet, andnetwork.dns.subscriptionvarRefPatternto match${VAR:-default}, socontainsVarRefcorrectly takes the "unresolved reference, validate at provision time" branch on the eject path instead of rejecting the value as a malformed resource idinit_env.gotohonorAzureYamlEnvironmentEscapingand drop the now-unusedignoreAzureYamlEnvironmentEscapingconstant, whose doc comment described the divergence this PR removesPreserving the unresolved-variable error
foundry.ExpandEnvresolves through a callback that only receives the variable name, so a name cannot be failed just because the callback saw it —${MISSING:-fallback}also calls the callback with an empty result before applying the default. The set of names that must resolve is therefore collected up front: a name is required only when it appears at least once without a:-default. Names inside a Foundry${{...}}span never reach the callback, becauseExpandEnvmasks those spans, so they cannot produce a false failure.${A:-ok}/${A}still fails onA, since the bare reference genuinely cannot resolve.Scope note
The issue notes this cleanup lets the escaping constant "delete itself". On
mainthat constant isignoreAzureYamlEnvironmentEscapingininit_env.go, used only by the three project network fields, and it is removed here.env_refs.godoes not exist yet — it arrives with #9079, which is still open — so nothing in that file is touched.internal/synthesis/synthesizer.gois duplicated inazure.ai.agentsduring the staged ownership migration; both copies are updated and verified byte-for-byte identical.Testing
go test ./... -count=1(azure.ai.projects)go test ./internal/cmd ./internal/synthesis -count=1(azure.ai.agents)go build ./...in both extensionsgolangci-lint runon the changed packages in both extensionscspell linton the changed Go filesNew coverage:
${VAR:-default}falls back (including inside a resource id),$${VAR}stays literal, an empty env value takes the default, the first unresolved variable is still named, a default elsewhere does not excuse a bare reference, and both the provision and eject paths accept a defaulted network reference end to end.One existing case in
TestFindAzureYamlEnvironmentReferencesasserted the old behavior — that an escaped$${VNET_ID}in a network field was still a required env reference. It now asserts the fixed semantics and covers both the escaped and unescaped halves.Fixes #9350