fix(ai-projects): route resolveVars through foundry.ExpandEnv

[glharper]

Summary

• route resolveVars through foundry.ExpandEnv, the shared expander every other Foundry field uses, so ${VAR:-default} and the $${VAR} escape now behave the same on network.agentSubnet.vnet, network.peSubnet.vnet, and network.dns.subscription
• share one ${VAR} scanner instead of re-deriving the escape and ${{...}} rules with a local regex, so the unresolved-variable guard is correct by construction
• reject the parts of drone/envsubst's grammar azd does not model, so nothing can expand behind that guard
• drop the now-unused ignoreEnvironmentEscaping constant and the honorEscaping parameter

Fixes #9350

Preserving the unresolved-variable error

foundry.ExpandEnv resolves 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 names that must resolve are therefore collected up front: a name is required only where it occurs at least once without a :- default, in a position the expander will actually act on.

That last clause is the load-bearing part, and a local regex cannot answer it. The scan comes from FindEnvReferences, which drops escaped occurrences and any reserved by a ${{...}} span — the latter by substituting a per-occurrence probe and letting ExpandEnv itself report which probes it left alone.

${A:-ok}/${A} still fails on A, since the bare reference genuinely cannot resolve.

One scanner, and where it lives

FindEnvReferences already existed, in azure.ai.agents/internal/cmd/env_refs.go (landed by #9079). It moves to internal/synthesis/envrefs.go, and internal/cmd/env_refs.go becomes a type alias plus a one-line delegation, so all three consumers share one implementation:

Consumer	Policy layered on the scan
internal/cmd init prompting	skip references with a default — the expander supplies the fallback
internal/cmd service env block	record every name so the owning extension can re-apply the default
internal/synthesis resolveVars	the ones without a default are the names that must resolve

pkg/foundry, next to ExpandEnv, is the natural home. It is not reachable from this PR: both extensions consume azd core at a pinned release (cli/azd v1.28.0, core is at 1.30.0-beta.1) with no replace, so new API there is invisible until core ships and both go.mod files are bumped — the two-PR rule in cli/azd/AGENTS.md. internal/synthesis is the one import path the two byte-identical synthesizer copies and internal/cmd can all spell identically, since parity_test.go compares the non-test .go files byte for byte. Tracked by #9427.

Rejecting the rest of the envsubst grammar

drone/envsubst implements the full shell parameter grammar. ${M:=d}, ${M:+alt}, ${M:?boom}, ${M#p} and ${M:0:3} all expand, none are shapes the scanner reports, so on main they slipped past the guard and the ARM id / subscription shape checks — peSubnet.vnet: "${MISSING#x}" silently became "", and dns.subscription: "${MISSING:=<guid>}" silently resolved without consulting the azd environment. Typing := for :- quietly succeeded.

ValidateEnvReferences refuses any $ form outside ${VAR}, ${VAR:-default}, $${VAR} and ${{...}}:

services.my-project.network.peSubnet.vnet: "${MISSING:=default}" is not a supported
environment variable reference; use ${VAR} or ${VAR:-default}, $${VAR} to keep it
literal, or ${{...}} for a Foundry expression

It runs on all three fields before either the provision or the eject path reads them.

Withdrawn: a reference nested in a :- default

ValidateEnvReferences also refuses ${A:-${B}}. This one is a behavior change, not a bug fix: the shape resolves correctly today whenever the nested name is set, so a working peSubnet.vnet: "${VNET_ID:-${FALLBACK_VNET_ID}}" starts erroring.

It is withdrawn rather than fixed because required is computed statically, and whether the nested name has to resolve depends on whether the outer one does — not knowable when the value is scanned. Reporting the nested name would raise a false unresolved-variable error every time the outer name is set; not reporting it leaves ${A:-${B}} with neither set expanding to empty, so the field's own shape check blames the empty value instead of naming B. Neither half is correct.

The message points at the replacement that keeps the value out of azure.yaml, rather than telling the user to hardcode a resource id:

services.my-project.network.peSubnet.vnet: "${FALLBACK_VNET_ID}" nests an environment variable
reference inside a :- default, which azd cannot check: whether the nested name is
required depends on whether the outer one resolves, and that is not known when the
value is scanned. Use a single ${VAR} and set it in the azd environment, or give the
default a literal value

Two subtleties the walk has to match: it steps into a default, because envsubst evaluates the default expression, and it leaves a $ alone when the next character opens a Foundry span, because ExpandEnv masks spans before envsubst sees the pair. A ${{...}} span stays legal as a default value. Bare $ forms stay accepted — envsubst expands only the braced shape, so $VAR and costs $5 survive untouched.

${MISSING-nodefault} is caught here too, replacing envsubst's confusing raw missing closing brace.

Scope of the completeness guarantee

Where the validator runs, FindEnvReferences is complete: every occurrence the expander acts on is one the scanner saw. That is a property of the call, not of the scanner — only the three network fields invoke it today. Discovery-only consumers, init prompting and the generated service env block, still scan unvalidated values, so an agent env: value of ${BAR:=x} yields no references, is never prompted for, and is rewritten at deploy. Extending the check to those paths changes every Foundry field in every extension and needs its own policy decision, so it is tracked by #9428 rather than folded in here.

Two follow-ons

• containsVarRef is now len(FindEnvReferences(s)) > 0, so it reports only references the expander will resolve. An escaped or span-reserved value counts as final.
• The ARM id / subscription shape checks are deferred only on the eject path. After resolveVars there is nothing left to expand, so a leftover ${VAR} — what $${VAR} resolves to — is rejected now instead of at a provision that can only fail. Previously eject and provision disagreed about the same azure.yaml.

Scope note

internal/synthesis/synthesizer.go is duplicated in azure.ai.agents during the staged ownership migration; both copies are updated and verified byte-for-byte identical by parity_test.go.

Testing

• go test ./... -count=1 in both extensions
• go build ./... in both extensions
• gofmt -s -l . clean, golangci-lint run ./internal/... 0 issues in both
• cspell lint on the changed Go files

New coverage:

• TestResolveVars_MatchesFoundryExpandEnv — ${VAR:-default} falls back (including inside a resource id), an empty env value takes the default, the first unresolved variable is still named, a default elsewhere does not excuse a bare reference, and an escaped or ${{...}}-reserved occurrence no longer makes a live defaulted one look unresolvable.
• TestValidateEnvReferences_RejectsUnsupportedForms — supported, rejected, and nested corpora, including the $${{...}} case.
• TestSynthesize_NetworkRejectsUnsupportedVarSyntax — the refusal end to end on both preserveVarRefs settings for both fields, asserting the field path is named.
• TestSynthesize_NetworkEscapedRefIsValidatedOnBothPaths — eject and provision agree.
• TestContainsVarRef_RecognizesDefaults — defaults still defer; escapes and spans do not.

One existing case in TestFindAzureYamlEnvironmentReferences asserted 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. The two env_refs_test.go cases that only existed to exercise ignoreEnvironmentEscaping are removed with it.

[github-actions]

📋 Prioritization Note

Thanks for the contribution! The linked issue isn't in the current milestone yet.
Thank you for logging this issue; our team is reviewing it. If you need urgent prioritization, tag @RickWinter and @kristenwomack to let us know.

[azure-pipelines]

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.

[Copilot]

Pull request overview

Aligns Foundry network environment expansion across the projects and agents extensions.

Changes:

• Uses foundry.ExpandEnv for 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 makes ExpandEnv produce 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 makes ExpandEnv produce 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 ""

[jongio]

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.

[jongio]

The escaped-reference and ${{...}} false positives already flagged have a ready-made fix inside this same PR.

init_env.go solves exactly this problem for exactly these three network fields, and this PR edits that file. isEscapedAzureYamlEnvironmentReference handles the odd-$ escape. protectedAzureYamlEnvironmentReferenceOccurrences decides which occurrences sit inside a ${{...}} span by swapping in a per-occurrence probe and asking foundry.ExpandEnv itself which probes survive verbatim. That's the same question resolveVars is now answering with a bare regex scan, and the probe answer is correct by construction because it delegates to the expander instead of re-modelling its grammar.

resolveVars applies only the third of those three filters (skip :- defaults) and drops the other two, which is why $${FOO} ${FOO:-fallback} still fails on FOO.

The helpers can't be imported as-is: init_env.go lives in azure.ai.agents/internal/cmd, and internal/synthesis is parity-duplicated across both extensions. pkg/foundry already owns ExpandEnv and both callers already import it, so it's the natural home for one shared "which names must resolve" helper. That also gives the operator gap from the other thread somewhere to live, instead of a second regex drifting from envsubst in a second file.

[glharper]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in cc6ea04.

The conflict was in cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go. main has since landed #9079, which moved the escaping constants and the reference scanner into internal/cmd/env_refs.go, so this branch's locally-defined azureYamlEnvRefPattern and honorAzureYamlEnvironmentEscaping were redundant. I dropped both and kept main's shared honorEnvironmentEscaping / findEnvironmentReferences, pointing the three project network fields (network.agentSubnet.vnet, network.peSubnet.vnet, network.dns.subscription) at honorEnvironmentEscaping instead of ignoreEnvironmentEscaping — which is the behavior change this PR is for.

I also updated the env_refs.go doc comment, since it described the resolveVars-vs-ExpandEnv divergence (including the :- gap and the link to #9350) that this PR removes.

Verified: both extensions build, go test ./internal/cmd/... ./internal/synthesis/... passes in azure.ai.agents, go test ./internal/synthesis/... passes in azure.ai.projects, and the two synthesizer.go copies remain byte-for-byte identical.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (5)

cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go:1112

• [azd-code-reviewer] After ExpandEnv processes $${VNET}, the result is the literal ${VNET} and no unresolved callback fires. This matcher then mistakes that concrete literal for a deferred reference, so the provision path skips the VNet/subscription shape check and sends a malformed value downstream instead of failing closed. Only defer validation on the preserve/eject path; after resolveVars runs, validate its output unconditionally.

// containsVarRef reports whether s still contains a ${VAR} reference, including
// the ${VAR:-default} form.
func containsVarRef(s string) bool {
	return varRefPattern.MatchString(s)

cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go:1112

• [azd-code-reviewer] After ExpandEnv processes $${VNET}, the result is the literal ${VNET} and no unresolved callback fires. This matcher then mistakes that concrete literal for a deferred reference, so the provision path skips the VNet/subscription shape check and sends a malformed value downstream instead of failing closed. Only defer validation on the preserve/eject path; after resolveVars runs, validate its output unconditionally.

// containsVarRef reports whether s still contains a ${VAR} reference, including
// the ${VAR:-default} form.
func containsVarRef(s string) bool {
	return varRefPattern.MatchString(s)

cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go:908

• [azd-code-reviewer] The widened pattern consumes a nested bare reference as part of the outer default. For ${A:-${B}}, this pre-scan records only defaulted A, while foundry.ExpandEnv still invokes the callback for B; if B is missing, it is silently mapped to empty instead of producing the unresolved-variable error that the old matcher produced. Parse all live nested bare references (or explicitly reject nested defaults) so the required-name check cannot miss B.

var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)(:-[^}]*)?\}`)

cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go:908

• [azd-code-reviewer] The widened pattern consumes a nested bare reference as part of the outer default. For ${A:-${B}}, this pre-scan records only defaulted A, while foundry.ExpandEnv still invokes the callback for B; if B is missing, it is silently mapped to empty instead of producing the unresolved-variable error that the old matcher produced. Parse all live nested bare references (or explicitly reject nested defaults) so the required-name check cannot miss B.

var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)(:-[^}]*)?\}`)

cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go:24

• [azd-code-reviewer] This contradicts the PR description and scope note: they say the now-unused ignore constant is removed and env_refs.go is untouched, but this diff retains the constant and changes this file. Either remove the obsolete false-policy branches/tests now that every production caller honors escaping, or update the description to document why this policy remains.

// ignoreEnvironmentEscaping remains for a field owned by an expander
// without that behavior; no field takes it today.
const (
	honorEnvironmentEscaping  = true
	ignoreEnvironmentEscaping = false

[jongio]

Three findings on the post-merge head. The merge pulled in #9079, which changes the picture: the scanner this PR needs now lives in the same extension.

1. resolveVars still builds required from a raw regex scan, so escaped and ${{...}}-protected occurrences still feed it. Re-verified at cc6ea04. Details inline.
2. The new test asserts $${VAR} stays literal, but only in the shape that already passes. Inline.
3. ignoreEnvironmentEscaping no longer has a production caller. Inline.

Still open from my earlier pass and unchanged by the merge: ${VAR:=default}, ${VAR:?msg} and ${VAR:+alt} expand through envsubst but never reach required and never satisfy containsVarRef. That comment is still on line 1130.

The description needs a refresh. The scope note says env_refs.go doesn't exist yet and that #9079 is still open, and the summary says the PR drops the now-unused escaping constant. #9079 merged, env_refs.go is now the main file this PR touches, and the constant is still there.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go:47

• [azd-code-reviewer] Nested ${VAR} references cannot be dropped now that resolveVars consumes this scanner. With ${OUTER:-${NESTED}} and neither variable set, ExpandEnv invokes NESTED, but the required-name set is empty, so resolveVars returns success and the caller reports a malformed resource ID instead of unresolved ${NESTED}. The previous regex caught the inner reference. Either discover nested bare references for unresolved validation or reject this syntax before expansion.

// A reference inside a :- default is not reported: nested azd references are
// unsupported by design, so ${OUTER:-${NESTED}} yields OUTER only.
// [foundry.ExpandEnv] still resolves NESTED at deploy, but nothing discovers it,
// so init never prompts for it and it gets no entry in the generated service env
// block. It then resolves only where the consumer keeps an azd environment

cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go:47

• [azd-code-reviewer] Nested ${VAR} references cannot be dropped now that resolveVars consumes this scanner. With ${OUTER:-${NESTED}} and neither variable set, ExpandEnv invokes NESTED, but the required-name set is empty, so resolveVars returns success and the caller reports a malformed resource ID instead of unresolved ${NESTED}. The previous regex caught the inner reference. Either discover nested bare references for unresolved validation or reject this syntax before expansion.

// A reference inside a :- default is not reported: nested azd references are
// unsupported by design, so ${OUTER:-${NESTED}} yields OUTER only.
// [foundry.ExpandEnv] still resolves NESTED at deploy, but nothing discovers it,
// so init never prompts for it and it gets no entry in the generated service env
// block. It then resolves only where the consumer keeps an azd environment

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (4)

cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go:47

• [azd-code-reviewer] This skips nested references even though resolveVars now treats this scan as the complete unresolved-variable guard. For ${OUTER:-${INNER}} with both names unset, ExpandEnv evaluates INNER, but required contains neither name, so the bare inner reference silently becomes empty and the user gets a later shape error instead of unresolved environment variable ${INNER}. The old regex did catch that inner reference. Either include nested occurrences in the scan or reject nested references in ValidateEnvReferences.

// A reference inside a :- default is not reported: nested azd references are
// unsupported by design, so ${OUTER:-${NESTED}} yields OUTER only.
// [foundry.ExpandEnv] still resolves NESTED at deploy, but nothing discovers it,
// so init never prompts for it and it gets no entry in the generated service env
// block. It then resolves only where the consumer keeps an azd environment

cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go:1096

• [azd-code-reviewer] This still lets an escaped reference reach Azure when it occupies only one ARM-ID segment. For example, /subscriptions/s/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/$${VNET} becomes the final literal .../${VNET}, but vnetIDPattern accepts any non-slash segment, so both provision and eject succeed despite the comment's guarantee that leftovers are rejected. Reject ${ in values whose validation is not deferred, and add this embedded case to the escaped-reference test.

	if (resolve || !containsVarRef(vnetID)) && !vnetIDPattern.MatchString(vnetID) {

cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go:47

• [azd-code-reviewer] This skips nested references even though resolveVars now treats this scan as the complete unresolved-variable guard. For ${OUTER:-${INNER}} with both names unset, ExpandEnv evaluates INNER, but required contains neither name, so the bare inner reference silently becomes empty and the user gets a later shape error instead of unresolved environment variable ${INNER}. The old regex did catch that inner reference. Either include nested occurrences in the scan or reject nested references in ValidateEnvReferences.

// A reference inside a :- default is not reported: nested azd references are
// unsupported by design, so ${OUTER:-${NESTED}} yields OUTER only.
// [foundry.ExpandEnv] still resolves NESTED at deploy, but nothing discovers it,
// so init never prompts for it and it gets no entry in the generated service env
// block. It then resolves only where the consumer keeps an azd environment

cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go:1096

• [azd-code-reviewer] This still lets an escaped reference reach Azure when it occupies only one ARM-ID segment. For example, /subscriptions/s/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/$${VNET} becomes the final literal .../${VNET}, but vnetIDPattern accepts any non-slash segment, so both provision and eject succeed despite the comment's guarantee that leftovers are rejected. Reject ${ in values whose validation is not deferred, and add this embedded case to the escaped-reference test.

	if (resolve || !containsVarRef(vnetID)) && !vnetIDPattern.MatchString(vnetID) {

[jongio]

No blockers. Three small things, all in the new scanner and its callers.

• internal/synthesis/envrefs.go:82 - the "complete by construction" claim only holds for the callers that actually run ValidateEnvReferences, which today is just the three network fields.
• internal/synthesis/envrefs.go:126 - a bare reference nested in a :- default never becomes required, so it resolves to empty with no unresolved-variable error.
• internal/synthesis/synthesizer.go:1096 - the resolveSubnet header comment still describes the old "only when fully concrete" behavior.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

[jongio]

Two low-severity notes on the new nesting refusal, both inline.

Unrelated to this PR: the TestARMTemplate_MatchesBicepBuild and TestBrownfieldARMTemplate_MatchesBicepBuild failures in internal/synthesis reproduce on main at f91b4c4, so they aren't from this change.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go:1166

• [azd-code-reviewer] The name-level required set can change which unresolved reference is reported. For ${A:-ok}/${B}/${A} with both names unset, the callback for the defaulted first occurrence marks A unresolved before reaching the earlier required occurrence B; the previous scanner reported B, and the PR states that the first unresolved variable remains named. Determine the first missing non-defaulted reference in scanner order, then use the callback only for expansion.

		if _, ok := required[name]; ok && unresolved == "" {
			unresolved = name

cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go:1166

• [azd-code-reviewer] The name-level required set can change which unresolved reference is reported. For ${A:-ok}/${B}/${A} with both names unset, the callback for the defaulted first occurrence marks A unresolved before reaching the earlier required occurrence B; the previous scanner reported B, and the PR states that the first unresolved variable remains named. Determine the first missing non-defaulted reference in scanner order, then use the callback only for expansion.

		if _, ok := required[name]; ok && unresolved == "" {
			unresolved = name

[jongio]

One low-severity note on the nested-reference refusal, inline.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

[jongio]

Previous comments addressed. One correctness issue remains in the validator.

_{🤖 agent jongio}

[jongio]

[MEDIUM] azd-code-reviewer: Escapes inside defaults bypass nested-reference validation

Inside a :- default, drone/envsubst parses $${B} as a literal $ plus a live ${B}, but this branch skips both dollars. As a result, ${A:-/subscriptions/$${B}/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/v} passes validation, expands B, and also passes vnetIDPattern; only treat $$ as an escape when defaultEnd == 0, then add this case to the nested-reference tests.

[huimiu]

${OUTER:-$${INNER}} may still expand INNER, but the validator skips it because of $$. If INNER is not set, it could silently become empty. Let's handle or reject this case in both envrefs.go files.