Skip to content

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

Open
glharper wants to merge 1 commit into
mainfrom
glharper/9350-resolvevars-expandenv
Open

fix(ai-projects): route resolveVars through foundry.ExpandEnv#9367
glharper wants to merge 1 commit into
mainfrom
glharper/9350-resolvevars-expandenv

Conversation

@glharper

Copy link
Copy Markdown
Member

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
  • widen varRefPattern to match ${VAR:-default}, so containsVarRef correctly takes the "unresolved reference, validate at provision time" branch on the eject path instead of rejecting the value as a malformed resource id
  • rebuild the load-bearing unresolved-variable check inside the mapping callback
  • switch the three project network fields in init_env.go to honorAzureYamlEnvironmentEscaping and drop the now-unused ignoreAzureYamlEnvironmentEscaping constant, whose doc comment described the divergence this PR removes

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 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, because ExpandEnv masks those spans, so they cannot produce a false failure.

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

Scope note

The issue notes this cleanup lets the escaping constant "delete itself". On main that constant is ignoreAzureYamlEnvironmentEscaping in init_env.go, used only by the three project network fields, and it is removed here. env_refs.go does not exist yet — it arrives with #9079, which is still open — so nothing in that file is touched.

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.

Testing

  • go test ./... -count=1 (azure.ai.projects)
  • go test ./internal/cmd ./internal/synthesis -count=1 (azure.ai.agents)
  • go build ./... in both extensions
  • golangci-lint run on the changed packages in both extensions
  • cspell lint on the changed Go files

New 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 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.

Fixes #9350

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fed9e97b-e79b-4889-ac76-0d9a428599cd
@glharper
glharper requested a review from JeffreyCA as a code owner July 30, 2026 16:49
Copilot AI review requested due to automatic review settings July 30, 2026 16:49
@github-actions

Copy link
Copy Markdown

📋 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

Copy link
Copy Markdown
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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ""

Comment on lines +1035 to +1038
required := map[string]struct{}{}
for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) {
if match[2] == "" {
required[match[1]] = struct{}{}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1035 to +1038
required := map[string]struct{}{}
for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) {
if match[2] == "" {
required[match[1]] = struct{}{}
@github-actions github-actions Bot added ext-agents azure.ai.agents extension ext-projects azure.ai.projects extension labels Jul 30, 2026

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ext-agents azure.ai.agents extension ext-projects azure.ai.projects extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Issue] resolveVars diverges from foundry.ExpandEnv on defaults and escaping

3 participants