From eea23cc8425b01008c54bdc793781c74b1f1db2a Mon Sep 17 00:00:00 2001 From: Glenn Harper Date: Thu, 30 Jul 2026 12:49:12 -0400 Subject: [PATCH 1/7] fix(ai-projects): route resolveVars through foundry.ExpandEnv Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fed9e97b-e79b-4889-ac76-0d9a428599cd --- .../azure.ai.agents/internal/cmd/init_env.go | 18 +-- .../internal/cmd/init_env_test.go | 8 +- .../internal/synthesis/synthesizer.go | 36 ++++- .../internal/synthesis/synthesizer.go | 36 ++++- .../internal/synthesis/synthesizer_test.go | 138 ++++++++++++++++++ 5 files changed, 208 insertions(+), 28 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go index 019a8ac0945..ada7528a5c3 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go @@ -23,14 +23,10 @@ import ( // environment value because the runtime expander supplies the fallback. var azureYamlEnvRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)(:-[^}]*)?\}`) -// Escape handling must match the expander that owns each field. -// foundry.ExpandEnv treats an odd leading '$' as an escape, while the project -// synthesizers' resolveVars helper expands every ${VAR} match regardless of -// a preceding '$'. -const ( - honorAzureYamlEnvironmentEscaping = true - ignoreAzureYamlEnvironmentEscaping = false -) +// Escape handling must match the expander that owns each field. Every Foundry +// field, including the project network values, expands through +// foundry.ExpandEnv, which treats an odd leading '$' as an escape. +const honorAzureYamlEnvironmentEscaping = true // These types mirror only the fields each Foundry provider expands from the // azd environment. The owning provider types are unexported or live in sibling @@ -406,7 +402,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.AgentSubnet.VNet, false, - ignoreAzureYamlEnvironmentEscaping, + honorAzureYamlEnvironmentEscaping, references, indexByName, ) @@ -415,7 +411,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.PESubnet.VNet, false, - ignoreAzureYamlEnvironmentEscaping, + honorAzureYamlEnvironmentEscaping, references, indexByName, ) @@ -424,7 +420,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.DNS.Subscription, false, - ignoreAzureYamlEnvironmentEscaping, + honorAzureYamlEnvironmentEscaping, references, indexByName, ) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go index 41018fcb2df..a49755a3a5e 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go @@ -135,7 +135,7 @@ services: }, }, { - name: "project scans only expanded network fields", + name: "project scans only expanded network fields and honors escaping", content: `name: sample services: project: @@ -151,12 +151,14 @@ services: agentSubnet: vnet: $${AGENT_VNET_ID} peSubnet: - vnet: $${PE_VNET_ID} + vnet: ${PE_VNET_ID} dns: subscription: ${DNS_SUBSCRIPTION_ID} `, + // The network fields expand through foundry.ExpandEnv, so an escaped + // $${VAR} stays literal and needs no environment value; only the + // unescaped references are required. want: []azureYamlEnvironmentReference{ - {Name: "AGENT_VNET_ID"}, {Name: "PE_VNET_ID"}, {Name: "DNS_SUBSCRIPTION_ID"}, }, diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go index 8c4e7d7baea..ae41ef6ae72 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go @@ -808,8 +808,10 @@ var guidPattern = regexp.MustCompile( // rgNamePattern matches a valid Azure resource group name. var rgNamePattern = regexp.MustCompile(`^[-\w._()]{1,90}$`) -// varRefPattern matches a ${VAR} reference. -var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}`) +// varRefPattern matches a ${VAR} reference, optionally carrying a +// ${VAR:-default} fallback. Group 2 is non-empty when a default is present, +// which means the reference resolves even with no environment value. +var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)(:-[^}]*)?\}`) // synthesizeNetwork validates the network: block and returns the bicep // parameter set plus the telemetry mode. When net is nil the returned @@ -1010,7 +1012,8 @@ func sameVNet(a, b string) bool { return strings.EqualFold(a, b) } -// containsVarRef reports whether s still contains a ${VAR} reference. +// containsVarRef reports whether s still contains a ${VAR} reference, including +// the ${VAR:-default} form. func containsVarRef(s string) bool { return varRefPattern.MatchString(s) } @@ -1018,21 +1021,40 @@ func containsVarRef(s string) bool { // resolveVars expands ${VAR} references in s using env first, then the // process environment. An unresolved reference is an error naming the // variable. +// +// Expansion routes through foundry.ExpandEnv, the shared expander every other +// Foundry field uses, so ${VAR:-default} and the $${VAR} escape behave here +// exactly as they do elsewhere. +// +// ExpandEnv resolves through a mapping callback that only receives the variable +// name, so the set of names that must resolve is 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 trip this check. func resolveVars(s string, env map[string]string) (string, error) { + required := map[string]struct{}{} + for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) { + if match[2] == "" { + required[match[1]] = struct{}{} + } + } + var unresolved string - out := varRefPattern.ReplaceAllStringFunc(s, func(match string) string { - name := varRefPattern.FindStringSubmatch(match)[1] + out, err := foundry.ExpandEnv(s, func(name string) string { if v, ok := env[name]; ok { return v } if v, ok := os.LookupEnv(name); ok { return v } - if unresolved == "" { + if _, ok := required[name]; ok && unresolved == "" { unresolved = name } - return match + return "" }) + if err != nil { + return "", err + } if unresolved != "" { return "", fmt.Errorf("unresolved environment variable ${%s}", unresolved) } diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go index 8c4e7d7baea..ae41ef6ae72 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go @@ -808,8 +808,10 @@ var guidPattern = regexp.MustCompile( // rgNamePattern matches a valid Azure resource group name. var rgNamePattern = regexp.MustCompile(`^[-\w._()]{1,90}$`) -// varRefPattern matches a ${VAR} reference. -var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}`) +// varRefPattern matches a ${VAR} reference, optionally carrying a +// ${VAR:-default} fallback. Group 2 is non-empty when a default is present, +// which means the reference resolves even with no environment value. +var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)(:-[^}]*)?\}`) // synthesizeNetwork validates the network: block and returns the bicep // parameter set plus the telemetry mode. When net is nil the returned @@ -1010,7 +1012,8 @@ func sameVNet(a, b string) bool { return strings.EqualFold(a, b) } -// containsVarRef reports whether s still contains a ${VAR} reference. +// containsVarRef reports whether s still contains a ${VAR} reference, including +// the ${VAR:-default} form. func containsVarRef(s string) bool { return varRefPattern.MatchString(s) } @@ -1018,21 +1021,40 @@ func containsVarRef(s string) bool { // resolveVars expands ${VAR} references in s using env first, then the // process environment. An unresolved reference is an error naming the // variable. +// +// Expansion routes through foundry.ExpandEnv, the shared expander every other +// Foundry field uses, so ${VAR:-default} and the $${VAR} escape behave here +// exactly as they do elsewhere. +// +// ExpandEnv resolves through a mapping callback that only receives the variable +// name, so the set of names that must resolve is 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 trip this check. func resolveVars(s string, env map[string]string) (string, error) { + required := map[string]struct{}{} + for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) { + if match[2] == "" { + required[match[1]] = struct{}{} + } + } + var unresolved string - out := varRefPattern.ReplaceAllStringFunc(s, func(match string) string { - name := varRefPattern.FindStringSubmatch(match)[1] + out, err := foundry.ExpandEnv(s, func(name string) string { if v, ok := env[name]; ok { return v } if v, ok := os.LookupEnv(name); ok { return v } - if unresolved == "" { + if _, ok := required[name]; ok && unresolved == "" { unresolved = name } - return match + return "" }) + if err != nil { + return "", err + } if unresolved != "" { return "", fmt.Errorf("unresolved environment variable ${%s}", unresolved) } diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go index 04f90f496f7..48828ba5b16 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go @@ -1544,3 +1544,141 @@ services: }) } } + +// TestResolveVars_MatchesFoundryExpandEnv locks in that the three project +// network fields resolved through resolveVars use the same expander semantics +// as every other Foundry field: ${VAR:-default} falls back, $${VAR} stays +// literal, and a reference with neither a value nor a default is still a +// load-bearing error naming the variable. +func TestResolveVars_MatchesFoundryExpandEnv(t *testing.T) { + env := map[string]string{ + "SET_VAR": "set-value", + "EMPTY_VAR": "", + } + + tests := []struct { + name string + in string + want string + wantErr string + }{ + {name: "plain reference", in: "${SET_VAR}", want: "set-value"}, + {name: "default is unused when set", in: "${SET_VAR:-fallback}", want: "set-value"}, + {name: "default fills in when unset", in: "${MISSING_VAR_XYZ:-fallback}", want: "fallback"}, + {name: "empty default is allowed", in: "${MISSING_VAR_XYZ:-}", want: ""}, + {name: "empty env value takes the default", in: "${EMPTY_VAR:-fallback}", want: "fallback"}, + {name: "escaped reference stays literal", in: "$${MISSING_VAR_XYZ}", want: "${MISSING_VAR_XYZ}"}, + {name: "no references", in: "/subscriptions/abc", want: "/subscriptions/abc"}, + { + name: "default inside a resource id", + in: "${MISSING_VAR_XYZ:-/subscriptions/s/resourceGroups/rg}", + want: "/subscriptions/s/resourceGroups/rg", + }, + { + name: "unresolved reference errors", + in: "${MISSING_VAR_XYZ}", + wantErr: "unresolved environment variable ${MISSING_VAR_XYZ}", + }, + { + name: "first unresolved reference is named", + in: "${MISSING_A_XYZ}/${MISSING_B_XYZ}", + wantErr: "unresolved environment variable ${MISSING_A_XYZ}", + }, + { + name: "a default elsewhere does not excuse a bare reference", + in: "${MISSING_VAR_XYZ:-ok}/${MISSING_VAR_XYZ}", + wantErr: "unresolved environment variable ${MISSING_VAR_XYZ}", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolveVars(tt.in, env) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestContainsVarRef_RecognizesDefaults guards the eject path: a reference with +// a default must be recognized as still-unresolved so the value is kept +// verbatim and the ARM-shape checks are deferred to provision time, instead of +// being rejected as a malformed resource id. +func TestContainsVarRef_RecognizesDefaults(t *testing.T) { + tests := []struct { + in string + want bool + }{ + {in: "${VNET}", want: true}, + {in: "${VNET:-/subscriptions/s}", want: true}, + {in: "/subscriptions/s/resourceGroups/rg", want: false}, + {in: "", want: false}, + } + + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + assert.Equal(t, tt.want, containsVarRef(tt.in)) + }) + } +} + +// TestSynthesize_NetworkVarRefDefaults covers the reported bug end to end: +// ${VAR:-default} on the three network fields previously fell through +// resolveVars unchanged and was then rejected by the ARM id / subscription +// shape checks, blaming the resource id instead of the unsupported syntax. +func TestSynthesize_NetworkVarRefDefaults(t *testing.T) { + const ( + fallbackVNet = "/subscriptions/00000000-0000-0000-0000-000000000000" + + "/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/default" + fallbackSub = "11111111-1111-1111-1111-111111111111" + ) + + yaml := ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: {vnet: "${MISSING_VNET_XYZ:-` + fallbackVNet + `}", name: pe-subnet} + dns: + subscription: "${MISSING_SUB_XYZ:-` + fallbackSub + `}" +` + res, err := Synthesize(Input{ + RawAzureYAML: []byte(yaml), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + }) + require.NoError(t, err) + require.NotNil(t, res) + assert.Equal(t, fallbackVNet, res.Parameters["vnetId"]) + assert.Equal(t, fallbackSub, res.Parameters["dnsZonesSubscription"]) +} + +// TestSynthesize_NetworkPreserveVarRefsWithDefault is the eject-path half of the +// same bug: a defaulted reference must survive verbatim rather than being +// rejected as a malformed VNet id. +func TestSynthesize_NetworkPreserveVarRefsWithDefault(t *testing.T) { + const ref = "${AZURE_VNET_ID:-/subscriptions/s/resourceGroups/rg" + + "/providers/Microsoft.Network/virtualNetworks/default}" + + yaml := ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: {vnet: "` + ref + `", name: pe-subnet} +` + res, err := Synthesize(Input{ + RawAzureYAML: []byte(yaml), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + PreserveVarRefs: true, + }) + require.NoError(t, err, "a defaulted ${VAR} must not fail on the eject path") + require.NotNil(t, res) + assert.Equal(t, ref, res.Parameters["vnetId"]) +} From c47449bc893019b69e2124808ac7da9f523e708d Mon Sep 17 00:00:00 2001 From: Glenn Harper Date: Tue, 4 Aug 2026 11:26:15 -0400 Subject: [PATCH 2/7] Share one ${VAR} scanner and reject envsubst forms azd does not model Addresses PR review on #9367. resolveVars built its required-name set from a local regex scan of the raw value, which re-modelled which occurrences foundry.ExpandEnv would actually expand -- and got it wrong two ways: - Escaped and ${{...}}-reserved occurrences seeded `required` even though the expander never resolves them, so a live defaulted reference to the same name reported a spurious unresolved variable: "$${FOO} ${FOO:-fallback}" and "${{connections.${FOO}.key}} ${FOO:-fallback}" both errored instead of resolving. - The pattern modelled only ${VAR} and ${VAR:-default}, but envsubst implements the full shell grammar. ${M:=d}, ${M:+alt}, ${M:?boom}, ${M#p} and ${M:0:3} expanded silently, never landed in `required` and never satisfied containsVarRef, so they bypassed both the unresolved-variable guard and the ARM id / subscription shape checks. Typing ':=' for ':-' quietly succeeded. Instead of a second scanner, the one landed by #9079 moves to internal/synthesis as FindEnvReferences: the only import path the two byte-identical synthesizer copies and internal/cmd can all spell the same way, since pkg/foundry is consumed at a pinned azd release. internal/cmd/env_refs.go becomes an adapter over it. Moving it next to ExpandEnv is tracked by #9427. ValidateEnvReferences then rejects any '$' form outside ${VAR}, ${VAR:-default}, $${VAR} and ${{...}}, which keeps the scan complete by construction. It runs on all three network fields before either the provision or the eject path reads them. It steps into ':-' defaults, because envsubst evaluates the default expression, and it leaves a '$' alone when the next character opens a Foundry span, matching how ExpandEnv masks spans before envsubst sees the pair. Two follow-ons: - containsVarRef now reports only references the expander will resolve, so an escaped or span-reserved value is treated as final. - The ARM id / subscription shape checks are deferred only on the eject path. After resolveVars nothing is 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. honorEscaping and ignoreEnvironmentEscaping are gone: every Foundry field, including the three project network values, now resolves through ExpandEnv, so there is no second policy left to select. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9 --- .../azure.ai.agents/internal/cmd/env_refs.go | 234 ++------------- .../internal/cmd/env_refs_test.go | 128 +++----- .../azure.ai.agents/internal/cmd/init_env.go | 10 +- .../internal/cmd/resource_services.go | 2 +- .../internal/synthesis/envrefs.go | 283 ++++++++++++++++++ .../internal/synthesis/synthesizer.go | 62 ++-- .../internal/synthesis/envrefs.go | 283 ++++++++++++++++++ .../internal/synthesis/synthesizer.go | 62 ++-- .../internal/synthesis/synthesizer_test.go | 177 +++++++++++ 9 files changed, 897 insertions(+), 344 deletions(-) create mode 100644 cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go create mode 100644 cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go index f002d19456e..72981a317e6 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go @@ -4,219 +4,31 @@ package cmd import ( - "fmt" - "regexp" - "strings" - - "github.com/azure/azure-dev/cli/azd/pkg/foundry" -) - -// Escape handling must match the expander that owns each field. -// Every Foundry field, including the three project network values -// (network.agentSubnet.vnet, network.peSubnet.vnet, -// network.dns.subscription), resolves through foundry.ExpandEnv and -// so takes honorEnvironmentEscaping: it collapses '$' pairs, so -// $${VAR} stays literal, and it reserves ${{...}} spans for Foundry. -// ignoreEnvironmentEscaping remains for a field owned by an expander -// without that behavior; no field takes it today. -const ( - honorEnvironmentEscaping = true - ignoreEnvironmentEscaping = false + "azureaiagent/internal/synthesis" ) -// environmentReferencePrefix parses only the reference prefix. -// Balanced defaults remain the scanner's responsibility. -var environmentReferencePrefix = regexp.MustCompile( - `^\$\{([A-Za-z_][A-Za-z0-9_]*)(\}|:-)`, -) - -// environmentReference is one azd ${VAR} occurrence in a string. -// Start and End bound the whole reference, including any :- -// default, so a caller can resume scanning at End. -type environmentReference struct { - Name string - Start int - End int - HasDefault bool -} - -// findEnvironmentReferences returns the azd ${VAR} references in -// value, in order of appearance. It is the single scanner for the -// package: callers layer their own policy on the result rather -// than reimplementing discovery. init prompting skips references -// with a default because the expander supplies the fallback, -// while the generated service env block records them so the -// owning extension can re-apply the default. +// environmentReference and findEnvironmentReferences are this package's view of +// the single azd ${VAR} scanner. See [synthesis.FindEnvReferences] for the +// discovery rules; the policy layered on top lives in the callers +// (collectAzureYamlEnvironmentReferences for init prompting, +// collectStringEnvironmentTemplates for the generated service env block). // -// References the expander would not resolve are dropped: escaped -// ones and any reserved by a Foundry ${{...}} span. honorEscaping -// must match the expander that owns the field. +// The implementation lives in internal/synthesis because that package is the +// other consumer — resolveVars derives its unresolved-variable guard from the +// same scan — and its two byte-identical copies (this extension and +// azure.ai.projects) can only share code through an import path both spell +// identically. pkg/foundry, next to ExpandEnv, is the natural home, but both +// extensions consume azd core at a pinned release, so moving it there needs a +// core release plus a go.mod bump in both modules. Tracked by +// https://github.com/Azure/azure-dev/issues/9427. // -// 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 -// fallback, and to empty where a declared env: drops it. Keep -// defaults literal. -func findEnvironmentReferences(value string, honorEscaping bool) []environmentReference { - candidates := environmentReferenceCandidates(value, honorEscaping) - if !honorEscaping || len(candidates) == 0 { - return candidates - } - - protected := protectedEnvironmentReferences(value, candidates) - references := make([]environmentReference, 0, len(candidates)) - for i, candidate := range candidates { - if protected[i] { - continue - } - references = append(references, candidate) - } - if len(references) == 0 { - return nil - } - return references -} - -// environmentReferenceCandidates scans value left to right for -// ${NAME} and ${NAME:-default} occurrences. drone/envsubst, which -// backs foundry.ExpandEnv, collapses a '$' pair into a literal -// '$' and keeps reading, so an escape only neutralizes the '${' -// it precedes: the text after it, including a default, still -// holds live references. Membership of a ${{...}} span is left to -// findEnvironmentReferences. Scanning resumes at the end of a -// match, so a default span is never scanned again; that is what -// keeps nested references out. -func environmentReferenceCandidates(value string, honorEscaping bool) []environmentReference { - var references []environmentReference - for index := 0; index < len(value); { - if value[index] != '$' { - index++ - continue - } - if honorEscaping && strings.HasPrefix(value[index:], "$$") { - index += 2 - continue - } - - reference, found := environmentReferenceAt(value, index) - if !found { - index++ - continue - } - - references = append(references, reference) - index = reference.End - } - return references -} - -// environmentReferenceAt parses the reference opening at start. -// The anchored prefix keeps a bare '$' from being read as one. -// Balanced defaults still need the stateful end scanner below. -func environmentReferenceAt(value string, start int) (environmentReference, bool) { - if start < 0 || start >= len(value) { - return environmentReference{}, false - } - - match := environmentReferencePrefix.FindStringSubmatch(value[start:]) - if match == nil { - return environmentReference{}, false - } - - name := match[1] - prefixEnd := start + len(match[0]) - if match[2] == "}" { - return environmentReference{ - Name: name, - Start: start, - End: prefixEnd, - }, true - } - - end, found := environmentReferenceEnd(value, prefixEnd) - if !found { - return environmentReference{}, false - } - return environmentReference{ - Name: name, - Start: start, - End: end, - HasDefault: true, - }, true -} - -// environmentReferenceEnd finds the '}' closing a :- default. It -// counts nested ${...} and steps over Foundry ${{...}} spans, -// which are legal default values, so the reported span covers the -// whole reference. -func environmentReferenceEnd(value string, index int) (int, bool) { - depth := 1 - for index < len(value) { - if strings.HasPrefix(value[index:], "${{") { - end := strings.Index(value[index+3:], "}}") - if end < 0 { - return 0, false - } - index += end + 5 - continue - } - if strings.HasPrefix(value[index:], "${") { - depth++ - index += 2 - continue - } - if value[index] == '}' { - depth-- - index++ - if depth == 0 { - return index, true - } - continue - } - index++ - } - return 0, false -} - -// protectedEnvironmentReferences reports which candidates sit -// inside a server-side ${{...}} span. Each candidate is replaced -// with a unique probe before running [foundry.ExpandEnv]; probes -// left verbatim are reserved by the shared expander. This keeps -// discovery linked to the owning implementation without ambiguous -// name-based occurrence counting. -func protectedEnvironmentReferences(value string, references []environmentReference) []bool { - protected := make([]bool, len(references)) - if len(references) == 0 { - return protected - } - - probePrefix := "AZD_ENV_REFERENCE_PROBE_" - for strings.Contains(value, probePrefix) { - probePrefix += "_" - } - - probeRefs := make([]string, len(references)) - var probed strings.Builder - last := 0 - for i, reference := range references { - probed.WriteString(value[last:reference.Start]) - probeRefs[i] = fmt.Sprintf("${%s%d}", probePrefix, i) - probed.WriteString(probeRefs[i]) - last = reference.End - } - probed.WriteString(value[last:]) - - expanded, err := foundry.ExpandEnv(probed.String(), func(name string) string { - return "expanded_" + name - }) - if err != nil { - return protected - } - for i, probeRef := range probeRefs { - protected[i] = strings.Contains(expanded, probeRef) - } - return protected +// Escape handling is no longer a per-field choice. Every Foundry field, +// including the three project network values (network.agentSubnet.vnet, +// network.peSubnet.vnet, network.dns.subscription), resolves through +// foundry.ExpandEnv, which collapses '$' pairs so $${VAR} stays literal and +// reserves ${{...}} spans for Foundry. +type environmentReference = synthesis.EnvReference + +func findEnvironmentReferences(value string) []environmentReference { + return synthesis.FindEnvReferences(value) } diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go index 99f2fded480..81985020070 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go @@ -18,7 +18,7 @@ import ( func TestFindEnvironmentReferencesEscapedOuterKeepsInnerLive(t *testing.T) { t.Parallel() - got := findEnvironmentReferences("$${A:-${B}}", honorEnvironmentEscaping) + got := findEnvironmentReferences("$${A:-${B}}") require.Equal(t, []environmentReference{{Name: "B", Start: 6, End: 10}}, got) } @@ -26,29 +26,25 @@ func TestFindEnvironmentReferences(t *testing.T) { t.Parallel() tests := []struct { - name string - value string - honorEscaping bool - want []environmentReference + name string + value string + want []environmentReference }{ { - name: "bare reference", - value: "${PLAIN}", - honorEscaping: honorEnvironmentEscaping, - want: []environmentReference{{Name: "PLAIN", Start: 0, End: 8}}, + name: "bare reference", + value: "${PLAIN}", + want: []environmentReference{{Name: "PLAIN", Start: 0, End: 8}}, }, { - name: "reference with default", - value: "${NAME:-fallback}", - honorEscaping: honorEnvironmentEscaping, + name: "reference with default", + value: "${NAME:-fallback}", want: []environmentReference{ {Name: "NAME", Start: 0, End: 17, HasDefault: true}, }, }, { - name: "multiple references keep order", - value: "prefix ${ONE} mid ${TWO:-x} suffix", - honorEscaping: honorEnvironmentEscaping, + name: "multiple references keep order", + value: "prefix ${ONE} mid ${TWO:-x} suffix", want: []environmentReference{ {Name: "ONE", Start: 7, End: 13}, {Name: "TWO", Start: 18, End: 27, HasDefault: true}, @@ -57,55 +53,35 @@ func TestFindEnvironmentReferences(t *testing.T) { { // drone/envsubst collapses '$' pairs, so one leading '$' // escapes the reference. - name: "single escape is dropped", - value: "$${VAR}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "single escape is dropped", + value: "$${VAR}", + want: nil, }, { // Two leading '$' collapse to a literal '$' and the // reference still expands. - name: "double escape still expands", - value: "$$${VAR}", - honorEscaping: honorEnvironmentEscaping, - want: []environmentReference{{Name: "VAR", Start: 2, End: 8}}, + name: "double escape still expands", + value: "$$${VAR}", + want: []environmentReference{{Name: "VAR", Start: 2, End: 8}}, }, { - name: "triple escape is dropped", - value: "$$$${VAR}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "triple escape is dropped", + value: "$$$${VAR}", + want: nil, }, { - name: "escapes ignored when the owner does not honor them", - value: "$${VAR}", - honorEscaping: ignoreEnvironmentEscaping, - want: []environmentReference{{Name: "VAR", Start: 1, End: 7}}, + name: "foundry expression yields nothing", + value: "${{connections.store.credentials.key}}", + want: nil, }, { - name: "foundry expression yields nothing", - value: "${{connections.store.credentials.key}}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "reference inside a foundry expression is reserved", + value: "${{ tools.${INNER} }}", + want: nil, }, { - name: "reference inside a foundry expression is reserved", - value: "${{ tools.${INNER} }}", - honorEscaping: honorEnvironmentEscaping, - want: nil, - }, - { - // Protection rides on the same switch as escaping so the - // scan matches whichever expander owns the field. - name: "foundry expression unprotected when escaping ignored", - value: "${{ tools.${INNER} }}", - honorEscaping: ignoreEnvironmentEscaping, - want: []environmentReference{{Name: "INNER", Start: 10, End: 18}}, - }, - { - name: "foundry expression as a default value", - value: "${MISSING:-${{event.body}}}", - honorEscaping: honorEnvironmentEscaping, + name: "foundry expression as a default value", + value: "${MISSING:-${{event.body}}}", want: []environmentReference{ {Name: "MISSING", Start: 0, End: 27, HasDefault: true}, }, @@ -114,9 +90,8 @@ func TestFindEnvironmentReferences(t *testing.T) { // Nested references are unsupported by design. The span // covers the default so scanning resumes after it and // NESTED is never reported. - name: "nested default is spanned, inner name unsupported", - value: "${OUTER:-${NESTED}} ${AFTER}", - honorEscaping: honorEnvironmentEscaping, + name: "nested default is spanned, inner name unsupported", + value: "${OUTER:-${NESTED}} ${AFTER}", want: []environmentReference{ {Name: "OUTER", Start: 0, End: 19, HasDefault: true}, {Name: "AFTER", Start: 20, End: 28}, @@ -126,36 +101,31 @@ func TestFindEnvironmentReferences(t *testing.T) { // A '$' the expander ignores must stay ignored here, // or a literal like "costs $price} today" writes a // phantom rice: ${rice} into the service env block. - name: "bare dollar is not a reference", - value: "$foo}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "bare dollar is not a reference", + value: "$foo}", + want: nil, }, { // The phantom would span the whole string and carry // HasDefault, hiding REAL from init prompting. - name: "bare dollar keeps a later reference live", - value: "$ab:-${REAL}}", - honorEscaping: honorEnvironmentEscaping, - want: []environmentReference{{Name: "REAL", Start: 5, End: 12}}, + name: "bare dollar keeps a later reference live", + value: "$ab:-${REAL}}", + want: []environmentReference{{Name: "REAL", Start: 5, End: 12}}, }, { - name: "invalid name yields nothing", - value: "${1BAD}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "invalid name yields nothing", + value: "${1BAD}", + want: nil, }, { - name: "unterminated reference yields nothing", - value: "${UNCLOSED", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "unterminated reference yields nothing", + value: "${UNCLOSED", + want: nil, }, { - name: "plain text yields nothing", - value: "no references here", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "plain text yields nothing", + value: "no references here", + want: nil, }, } @@ -163,7 +133,7 @@ func TestFindEnvironmentReferences(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := findEnvironmentReferences(tt.value, tt.honorEscaping) + got := findEnvironmentReferences(tt.value) require.Equal(t, tt.want, got) }) } @@ -186,7 +156,6 @@ func TestFindEnvironmentReferencesPolicies(t *testing.T) { collectAzureYamlEnvironmentReferences( value, false, - honorEnvironmentEscaping, &references, map[string]int{}, ) @@ -223,7 +192,6 @@ func TestNestedDefaultIsNotDiscovered(t *testing.T) { collectAzureYamlEnvironmentReferences( value, false, - honorEnvironmentEscaping, &references, map[string]int{}, ) @@ -238,14 +206,12 @@ func TestCollectAzureYamlEnvironmentReferencesUpgradesSecret(t *testing.T) { collectAzureYamlEnvironmentReferences( "${SHARED}", false, - honorEnvironmentEscaping, &references, indexByName, ) collectAzureYamlEnvironmentReferences( "${SHARED}", true, - honorEnvironmentEscaping, &references, indexByName, ) @@ -298,7 +264,7 @@ func TestFindEnvironmentReferencesMatchesExpander(t *testing.T) { }) require.NoError(t, err) - for _, reference := range findEnvironmentReferences(value, honorEnvironmentEscaping) { + for _, reference := range findEnvironmentReferences(value) { require.Truef( t, lookedUp[reference.Name], diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go index 722bbf98a62..59f153f8b6f 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go @@ -348,7 +348,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( variable.Value, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -361,7 +360,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Target, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -391,7 +389,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.AgentSubnet.VNet, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -400,7 +397,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.PESubnet.VNet, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -409,7 +405,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.DNS.Subscription, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -438,7 +433,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Endpoint, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -501,7 +495,6 @@ func collectAzureYamlEnvironmentReferencesFromNode( collectAzureYamlEnvironmentReferences( node.Value, secret, - honorEnvironmentEscaping, references, indexByName, ) @@ -511,11 +504,10 @@ func collectAzureYamlEnvironmentReferencesFromNode( func collectAzureYamlEnvironmentReferences( value string, secret bool, - honorEscaping bool, references *[]azureYamlEnvironmentReference, indexByName map[string]int, ) { - for _, reference := range findEnvironmentReferences(value, honorEscaping) { + for _, reference := range findEnvironmentReferences(value) { // A ${VAR:-default} needs no environment value because the // runtime expander supplies the fallback. if reference.HasDefault { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services.go index 524a10d5966..83b5c8ae13b 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services.go @@ -337,7 +337,7 @@ func collectEnvironmentTemplates(value any, environment map[string]string) { } func collectStringEnvironmentTemplates(value string, environment map[string]string) { - for _, reference := range findEnvironmentReferences(value, honorEnvironmentEscaping) { + for _, reference := range findEnvironmentReferences(value) { // env is keyed by name, so store one canonical ${NAME}. // A ${NAME:-default} default is re-applied by the owning // extension against the raw config at deploy, so the env section diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go new file mode 100644 index 00000000000..5e8ab1a7284 --- /dev/null +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go @@ -0,0 +1,283 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package synthesis + +import ( + "fmt" + "regexp" + "strings" + + "github.com/azure/azure-dev/cli/azd/pkg/foundry" +) + +// EnvReference is one azd ${VAR} occurrence in a string. Start and End bound the +// whole reference, including any :- default, so a caller can resume scanning at +// End. +type EnvReference struct { + Name string + Start int + End int + HasDefault bool +} + +// envReferencePrefix parses only the reference prefix. Balanced defaults remain +// the scanner's responsibility. +var envReferencePrefix = regexp.MustCompile(`^\$\{([A-Za-z_][A-Za-z0-9_]*)(\}|:-)`) + +// FindEnvReferences returns the azd ${VAR} references in value that +// [foundry.ExpandEnv] actually resolves, in order of appearance. +// +// This is the single scanner for azd references in a Foundry value. Every +// consumer layers its own policy on the result rather than reimplementing +// discovery: init prompting skips references with a default because the +// expander supplies the fallback, the generated service env block records them +// so the owning extension can re-apply the default, and resolveVars treats the +// ones without a default as the names that must resolve. Re-deriving the escape +// and ${{...}} rules per consumer is what lets them drift away from the +// expander. +// +// References the expander would not resolve are dropped: escaped ones, and any +// reserved by a Foundry ${{...}} span. +// +// 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 +// fallback, and to empty where a declared env: drops it. Keep defaults literal. +func FindEnvReferences(value string) []EnvReference { + candidates := envReferenceCandidates(value) + if len(candidates) == 0 { + return nil + } + + protected := protectedEnvReferences(value, candidates) + references := make([]EnvReference, 0, len(candidates)) + for i, candidate := range candidates { + if protected[i] { + continue + } + references = append(references, candidate) + } + if len(references) == 0 { + return nil + } + return references +} + +// ValidateEnvReferences reports an error when value carries a '$' form that +// [foundry.ExpandEnv] would act on but azd does not model. +// +// drone/envsubst, which backs the expander, implements the full shell parameter +// grammar: ${VAR:=default}, ${VAR:+alt}, ${VAR:?message}, ${VAR#prefix} and +// ${VAR:0:3} all expand. None of them are shapes the scanner above reports, so +// without this check they slip past the unresolved-variable guard and quietly +// rewrite a value that the caller then validates as if the user had written it. +// Typing ':=' instead of ':-' is a one character slip that would otherwise +// succeed while skipping the very guard it looks like it is using. +// +// Rejecting the whole grammar except the four shapes azd documents keeps +// [FindEnvReferences] complete by construction: after this passes, every +// occurrence the expander acts on is one the scanner saw. +func ValidateEnvReferences(value string) error { + for index := 0; index < len(value); { + if value[index] != '$' { + index++ + continue + } + // A '$' pair collapses to a literal '$', neutralizing only the '${' it + // precedes — unless the second '$' opens a Foundry span, which + // foundry.ExpandEnv masks before envsubst ever sees the pair. Leaving + // that '$' for the span rule below keeps the two in step. + if strings.HasPrefix(value[index:], "$$") && + !strings.HasPrefix(value[index+1:], "${{") { + index += 2 + continue + } + // A Foundry span is reserved verbatim for the service to resolve. + if strings.HasPrefix(value[index:], "${{") { + end := strings.Index(value[index+3:], "}}") + if end < 0 { + return fmt.Errorf("%q is missing the closing }} of a Foundry expression", + unsupportedEnvFragment(value, index)) + } + index += end + 5 + continue + } + // A bare '$' is not a reference: envsubst expands only the braced form, + // so "$VAR" and "costs $5" survive expansion untouched. + if !strings.HasPrefix(value[index:], "${") { + index++ + continue + } + + reference, found := envReferenceAt(value, index) + if !found { + return fmt.Errorf( + "%q is not a supported environment variable reference; use ${VAR} or "+ + "${VAR:-default}, $${VAR} to keep it literal, or ${{...}} for a Foundry expression", + unsupportedEnvFragment(value, index)) + } + if reference.HasDefault { + // Step into the default rather than over it. FindEnvReferences stops + // at the default because nested references are not *discovered*; + // envsubst still *expands* them, so an unsupported form nested in a + // default reaches the expander exactly like a top-level one. + index = reference.Start + len("${") + len(reference.Name) + len(":-") + continue + } + index = reference.End + } + return nil +} + +// unsupportedEnvFragment returns the reference-looking fragment starting at +// index so an error can quote the offending text rather than the whole value. It +// stops at the first '}' because an unsupported form is by definition one the +// span scanner cannot bound. +func unsupportedEnvFragment(value string, index int) string { + rest := value[index:] + if end := strings.IndexByte(rest, '}'); end >= 0 { + return rest[:end+1] + } + return rest +} + +// envReferenceCandidates scans value left to right for ${NAME} and +// ${NAME:-default} occurrences. drone/envsubst, which backs +// [foundry.ExpandEnv], collapses a '$' pair into a literal '$' and keeps +// reading, so an escape only neutralizes the '${' it precedes: the text after +// it, including a default, still holds live references. Membership of a ${{...}} +// span is left to [FindEnvReferences]. Scanning resumes at the end of a match, +// so a default span is never scanned again; that is what keeps nested references +// out. +func envReferenceCandidates(value string) []EnvReference { + var references []EnvReference + for index := 0; index < len(value); { + if value[index] != '$' { + index++ + continue + } + if strings.HasPrefix(value[index:], "$$") { + index += 2 + continue + } + + reference, found := envReferenceAt(value, index) + if !found { + index++ + continue + } + + references = append(references, reference) + index = reference.End + } + return references +} + +// envReferenceAt parses the reference opening at start. The anchored prefix +// keeps a bare '$' from being read as one. Balanced defaults still need the +// stateful end scanner below. +func envReferenceAt(value string, start int) (EnvReference, bool) { + if start < 0 || start >= len(value) { + return EnvReference{}, false + } + + match := envReferencePrefix.FindStringSubmatch(value[start:]) + if match == nil { + return EnvReference{}, false + } + + name := match[1] + prefixEnd := start + len(match[0]) + if match[2] == "}" { + return EnvReference{ + Name: name, + Start: start, + End: prefixEnd, + }, true + } + + end, found := envReferenceEnd(value, prefixEnd) + if !found { + return EnvReference{}, false + } + return EnvReference{ + Name: name, + Start: start, + End: end, + HasDefault: true, + }, true +} + +// envReferenceEnd finds the '}' closing a :- default. It counts nested ${...} +// and steps over Foundry ${{...}} spans, which are legal default values, so the +// reported span covers the whole reference. +func envReferenceEnd(value string, index int) (int, bool) { + depth := 1 + for index < len(value) { + if strings.HasPrefix(value[index:], "${{") { + end := strings.Index(value[index+3:], "}}") + if end < 0 { + return 0, false + } + index += end + 5 + continue + } + if strings.HasPrefix(value[index:], "${") { + depth++ + index += 2 + continue + } + if value[index] == '}' { + depth-- + index++ + if depth == 0 { + return index, true + } + continue + } + index++ + } + return 0, false +} + +// protectedEnvReferences reports which candidates sit inside a server-side +// ${{...}} span. Each candidate is replaced with a unique probe before running +// [foundry.ExpandEnv]; probes left verbatim are reserved by the shared expander. +// This keeps discovery linked to the owning implementation without ambiguous +// name-based occurrence counting. +func protectedEnvReferences(value string, references []EnvReference) []bool { + protected := make([]bool, len(references)) + if len(references) == 0 { + return protected + } + + probePrefix := "AZD_ENV_REFERENCE_PROBE_" + for strings.Contains(value, probePrefix) { + probePrefix += "_" + } + + probeRefs := make([]string, len(references)) + var probed strings.Builder + last := 0 + for i, reference := range references { + probed.WriteString(value[last:reference.Start]) + probeRefs[i] = fmt.Sprintf("${%s%d}", probePrefix, i) + probed.WriteString(probeRefs[i]) + last = reference.End + } + probed.WriteString(value[last:]) + + expanded, err := foundry.ExpandEnv(probed.String(), func(name string) string { + return "expanded_" + name + }) + if err != nil { + return protected + } + for i, probeRef := range probeRefs { + protected[i] = strings.Contains(expanded, probeRef) + } + return protected +} diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go index 24874ef49c6..fa333944f6d 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go @@ -902,11 +902,6 @@ var guidPattern = regexp.MustCompile( // rgNamePattern matches a valid Azure resource group name. var rgNamePattern = regexp.MustCompile(`^[-\w._()]{1,90}$`) -// varRefPattern matches a ${VAR} reference, optionally carrying a -// ${VAR:-default} fallback. Group 2 is non-empty when a default is present, -// which means the reference resolves even with no environment value. -var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)(:-[^}]*)?\}`) - // synthesizeNetwork validates the network: block and returns the bicep // parameter set plus the telemetry mode. When net is nil the returned // params disable network isolation and the output is byte-identical to the @@ -1019,6 +1014,12 @@ func synthesizeNetwork( params["dnsZonesResourceGroup"] = rg } if sub := strings.TrimSpace(net.DNS.Subscription); sub != "" { + // Rejected before either path reads it: an unsupported '$' form is + // silently rewritten by the expander on the provision path, and + // written verbatim into the ejected template on the other. + if err := ValidateEnvReferences(sub); err != nil { + return nil, "", fmt.Errorf("%s.dns.subscription: %w", fp(""), err) + } if resolve { resolved, err := resolveVars(sub, env) if err != nil { @@ -1026,9 +1027,12 @@ func synthesizeNetwork( } sub = resolved } - // Normalize to a bare GUID only when concrete; an unexpanded ${VAR} - // (eject path) is normalized at provision time. - if containsVarRef(sub) { + // Normalize to a bare GUID only when the value is final. On the eject + // path an unexpanded ${VAR} is normalized at provision time; once + // resolveVars has run there is nothing left to expand, so anything + // still shaped like a reference (an escaped $${VAR} resolves to a + // literal ${VAR}) is a subscription id that never will be. + if !resolve && containsVarRef(sub) { params["dnsZonesSubscription"] = sub } else { guid, err := normalizeSubscription(sub) @@ -1072,6 +1076,11 @@ func resolveSubnet( if name == "" { return "", "", "", false, fmt.Errorf("%s.name: required", fieldPath) } + // Rejected on both paths: see the dns.subscription call for why this cannot + // wait for resolveVars. + if err := ValidateEnvReferences(vnetID); err != nil { + return "", "", "", false, fmt.Errorf("%s.vnet: %w", fieldPath, err) + } if resolve { resolved, rerr := resolveVars(vnetID, env) if rerr != nil { @@ -1079,9 +1088,12 @@ func resolveSubnet( } vnetID = resolved } - // Validate the ARM id shape only when fully concrete; an unexpanded ${VAR} - // (eject path) is validated at provision time. - if !containsVarRef(vnetID) && !vnetIDPattern.MatchString(vnetID) { + // Validate the ARM id shape unless the value can still change: on the eject + // path an unexpanded ${VAR} is validated at provision time. After + // resolveVars there is nothing left to expand, so a leftover ${VAR} (what an + // escaped $${VAR} resolves to) is checked now rather than deferred to a + // provision that can only fail. + if (resolve || !containsVarRef(vnetID)) && !vnetIDPattern.MatchString(vnetID) { return "", "", "", false, fmt.Errorf( "%s.vnet: %q is not a well-formed Microsoft.Network/virtualNetworks id", fieldPath, vnetID) } @@ -1106,10 +1118,14 @@ func sameVNet(a, b string) bool { return strings.EqualFold(a, b) } -// containsVarRef reports whether s still contains a ${VAR} reference, including -// the ${VAR:-default} form. +// containsVarRef reports whether s still carries an azd ${VAR} reference the +// expander will resolve, including the ${VAR:-default} form. +// +// Escaped references and names reserved by a Foundry ${{...}} span do not count: +// [foundry.ExpandEnv] leaves those alone, so the value is already as concrete as +// it will ever be and the caller's own shape validation should run on it. func containsVarRef(s string) bool { - return varRefPattern.MatchString(s) + return len(FindEnvReferences(s)) > 0 } // resolveVars expands ${VAR} references in s using env first, then the @@ -1121,15 +1137,19 @@ func containsVarRef(s string) bool { // exactly as they do elsewhere. // // ExpandEnv resolves through a mapping callback that only receives the variable -// name, so the set of names that must resolve is 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 trip this check. +// name, so the names that must resolve are collected up front from +// [FindEnvReferences]: a name is required only where it occurs at least once +// without a :- default, in a position the expander will actually act on. Reusing +// that scanner is what keeps an escaped or ${{...}} reserved occurrence from +// making a live, defaulted occurrence of the same name look unresolvable. +// +// Callers validate the value with [ValidateEnvReferences] first, so every +// occurrence the expander acts on is one the scanner saw. func resolveVars(s string, env map[string]string) (string, error) { required := map[string]struct{}{} - for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) { - if match[2] == "" { - required[match[1]] = struct{}{} + for _, reference := range FindEnvReferences(s) { + if !reference.HasDefault { + required[reference.Name] = struct{}{} } } diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go new file mode 100644 index 00000000000..5e8ab1a7284 --- /dev/null +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go @@ -0,0 +1,283 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package synthesis + +import ( + "fmt" + "regexp" + "strings" + + "github.com/azure/azure-dev/cli/azd/pkg/foundry" +) + +// EnvReference is one azd ${VAR} occurrence in a string. Start and End bound the +// whole reference, including any :- default, so a caller can resume scanning at +// End. +type EnvReference struct { + Name string + Start int + End int + HasDefault bool +} + +// envReferencePrefix parses only the reference prefix. Balanced defaults remain +// the scanner's responsibility. +var envReferencePrefix = regexp.MustCompile(`^\$\{([A-Za-z_][A-Za-z0-9_]*)(\}|:-)`) + +// FindEnvReferences returns the azd ${VAR} references in value that +// [foundry.ExpandEnv] actually resolves, in order of appearance. +// +// This is the single scanner for azd references in a Foundry value. Every +// consumer layers its own policy on the result rather than reimplementing +// discovery: init prompting skips references with a default because the +// expander supplies the fallback, the generated service env block records them +// so the owning extension can re-apply the default, and resolveVars treats the +// ones without a default as the names that must resolve. Re-deriving the escape +// and ${{...}} rules per consumer is what lets them drift away from the +// expander. +// +// References the expander would not resolve are dropped: escaped ones, and any +// reserved by a Foundry ${{...}} span. +// +// 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 +// fallback, and to empty where a declared env: drops it. Keep defaults literal. +func FindEnvReferences(value string) []EnvReference { + candidates := envReferenceCandidates(value) + if len(candidates) == 0 { + return nil + } + + protected := protectedEnvReferences(value, candidates) + references := make([]EnvReference, 0, len(candidates)) + for i, candidate := range candidates { + if protected[i] { + continue + } + references = append(references, candidate) + } + if len(references) == 0 { + return nil + } + return references +} + +// ValidateEnvReferences reports an error when value carries a '$' form that +// [foundry.ExpandEnv] would act on but azd does not model. +// +// drone/envsubst, which backs the expander, implements the full shell parameter +// grammar: ${VAR:=default}, ${VAR:+alt}, ${VAR:?message}, ${VAR#prefix} and +// ${VAR:0:3} all expand. None of them are shapes the scanner above reports, so +// without this check they slip past the unresolved-variable guard and quietly +// rewrite a value that the caller then validates as if the user had written it. +// Typing ':=' instead of ':-' is a one character slip that would otherwise +// succeed while skipping the very guard it looks like it is using. +// +// Rejecting the whole grammar except the four shapes azd documents keeps +// [FindEnvReferences] complete by construction: after this passes, every +// occurrence the expander acts on is one the scanner saw. +func ValidateEnvReferences(value string) error { + for index := 0; index < len(value); { + if value[index] != '$' { + index++ + continue + } + // A '$' pair collapses to a literal '$', neutralizing only the '${' it + // precedes — unless the second '$' opens a Foundry span, which + // foundry.ExpandEnv masks before envsubst ever sees the pair. Leaving + // that '$' for the span rule below keeps the two in step. + if strings.HasPrefix(value[index:], "$$") && + !strings.HasPrefix(value[index+1:], "${{") { + index += 2 + continue + } + // A Foundry span is reserved verbatim for the service to resolve. + if strings.HasPrefix(value[index:], "${{") { + end := strings.Index(value[index+3:], "}}") + if end < 0 { + return fmt.Errorf("%q is missing the closing }} of a Foundry expression", + unsupportedEnvFragment(value, index)) + } + index += end + 5 + continue + } + // A bare '$' is not a reference: envsubst expands only the braced form, + // so "$VAR" and "costs $5" survive expansion untouched. + if !strings.HasPrefix(value[index:], "${") { + index++ + continue + } + + reference, found := envReferenceAt(value, index) + if !found { + return fmt.Errorf( + "%q is not a supported environment variable reference; use ${VAR} or "+ + "${VAR:-default}, $${VAR} to keep it literal, or ${{...}} for a Foundry expression", + unsupportedEnvFragment(value, index)) + } + if reference.HasDefault { + // Step into the default rather than over it. FindEnvReferences stops + // at the default because nested references are not *discovered*; + // envsubst still *expands* them, so an unsupported form nested in a + // default reaches the expander exactly like a top-level one. + index = reference.Start + len("${") + len(reference.Name) + len(":-") + continue + } + index = reference.End + } + return nil +} + +// unsupportedEnvFragment returns the reference-looking fragment starting at +// index so an error can quote the offending text rather than the whole value. It +// stops at the first '}' because an unsupported form is by definition one the +// span scanner cannot bound. +func unsupportedEnvFragment(value string, index int) string { + rest := value[index:] + if end := strings.IndexByte(rest, '}'); end >= 0 { + return rest[:end+1] + } + return rest +} + +// envReferenceCandidates scans value left to right for ${NAME} and +// ${NAME:-default} occurrences. drone/envsubst, which backs +// [foundry.ExpandEnv], collapses a '$' pair into a literal '$' and keeps +// reading, so an escape only neutralizes the '${' it precedes: the text after +// it, including a default, still holds live references. Membership of a ${{...}} +// span is left to [FindEnvReferences]. Scanning resumes at the end of a match, +// so a default span is never scanned again; that is what keeps nested references +// out. +func envReferenceCandidates(value string) []EnvReference { + var references []EnvReference + for index := 0; index < len(value); { + if value[index] != '$' { + index++ + continue + } + if strings.HasPrefix(value[index:], "$$") { + index += 2 + continue + } + + reference, found := envReferenceAt(value, index) + if !found { + index++ + continue + } + + references = append(references, reference) + index = reference.End + } + return references +} + +// envReferenceAt parses the reference opening at start. The anchored prefix +// keeps a bare '$' from being read as one. Balanced defaults still need the +// stateful end scanner below. +func envReferenceAt(value string, start int) (EnvReference, bool) { + if start < 0 || start >= len(value) { + return EnvReference{}, false + } + + match := envReferencePrefix.FindStringSubmatch(value[start:]) + if match == nil { + return EnvReference{}, false + } + + name := match[1] + prefixEnd := start + len(match[0]) + if match[2] == "}" { + return EnvReference{ + Name: name, + Start: start, + End: prefixEnd, + }, true + } + + end, found := envReferenceEnd(value, prefixEnd) + if !found { + return EnvReference{}, false + } + return EnvReference{ + Name: name, + Start: start, + End: end, + HasDefault: true, + }, true +} + +// envReferenceEnd finds the '}' closing a :- default. It counts nested ${...} +// and steps over Foundry ${{...}} spans, which are legal default values, so the +// reported span covers the whole reference. +func envReferenceEnd(value string, index int) (int, bool) { + depth := 1 + for index < len(value) { + if strings.HasPrefix(value[index:], "${{") { + end := strings.Index(value[index+3:], "}}") + if end < 0 { + return 0, false + } + index += end + 5 + continue + } + if strings.HasPrefix(value[index:], "${") { + depth++ + index += 2 + continue + } + if value[index] == '}' { + depth-- + index++ + if depth == 0 { + return index, true + } + continue + } + index++ + } + return 0, false +} + +// protectedEnvReferences reports which candidates sit inside a server-side +// ${{...}} span. Each candidate is replaced with a unique probe before running +// [foundry.ExpandEnv]; probes left verbatim are reserved by the shared expander. +// This keeps discovery linked to the owning implementation without ambiguous +// name-based occurrence counting. +func protectedEnvReferences(value string, references []EnvReference) []bool { + protected := make([]bool, len(references)) + if len(references) == 0 { + return protected + } + + probePrefix := "AZD_ENV_REFERENCE_PROBE_" + for strings.Contains(value, probePrefix) { + probePrefix += "_" + } + + probeRefs := make([]string, len(references)) + var probed strings.Builder + last := 0 + for i, reference := range references { + probed.WriteString(value[last:reference.Start]) + probeRefs[i] = fmt.Sprintf("${%s%d}", probePrefix, i) + probed.WriteString(probeRefs[i]) + last = reference.End + } + probed.WriteString(value[last:]) + + expanded, err := foundry.ExpandEnv(probed.String(), func(name string) string { + return "expanded_" + name + }) + if err != nil { + return protected + } + for i, probeRef := range probeRefs { + protected[i] = strings.Contains(expanded, probeRef) + } + return protected +} diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go index 24874ef49c6..fa333944f6d 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go @@ -902,11 +902,6 @@ var guidPattern = regexp.MustCompile( // rgNamePattern matches a valid Azure resource group name. var rgNamePattern = regexp.MustCompile(`^[-\w._()]{1,90}$`) -// varRefPattern matches a ${VAR} reference, optionally carrying a -// ${VAR:-default} fallback. Group 2 is non-empty when a default is present, -// which means the reference resolves even with no environment value. -var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)(:-[^}]*)?\}`) - // synthesizeNetwork validates the network: block and returns the bicep // parameter set plus the telemetry mode. When net is nil the returned // params disable network isolation and the output is byte-identical to the @@ -1019,6 +1014,12 @@ func synthesizeNetwork( params["dnsZonesResourceGroup"] = rg } if sub := strings.TrimSpace(net.DNS.Subscription); sub != "" { + // Rejected before either path reads it: an unsupported '$' form is + // silently rewritten by the expander on the provision path, and + // written verbatim into the ejected template on the other. + if err := ValidateEnvReferences(sub); err != nil { + return nil, "", fmt.Errorf("%s.dns.subscription: %w", fp(""), err) + } if resolve { resolved, err := resolveVars(sub, env) if err != nil { @@ -1026,9 +1027,12 @@ func synthesizeNetwork( } sub = resolved } - // Normalize to a bare GUID only when concrete; an unexpanded ${VAR} - // (eject path) is normalized at provision time. - if containsVarRef(sub) { + // Normalize to a bare GUID only when the value is final. On the eject + // path an unexpanded ${VAR} is normalized at provision time; once + // resolveVars has run there is nothing left to expand, so anything + // still shaped like a reference (an escaped $${VAR} resolves to a + // literal ${VAR}) is a subscription id that never will be. + if !resolve && containsVarRef(sub) { params["dnsZonesSubscription"] = sub } else { guid, err := normalizeSubscription(sub) @@ -1072,6 +1076,11 @@ func resolveSubnet( if name == "" { return "", "", "", false, fmt.Errorf("%s.name: required", fieldPath) } + // Rejected on both paths: see the dns.subscription call for why this cannot + // wait for resolveVars. + if err := ValidateEnvReferences(vnetID); err != nil { + return "", "", "", false, fmt.Errorf("%s.vnet: %w", fieldPath, err) + } if resolve { resolved, rerr := resolveVars(vnetID, env) if rerr != nil { @@ -1079,9 +1088,12 @@ func resolveSubnet( } vnetID = resolved } - // Validate the ARM id shape only when fully concrete; an unexpanded ${VAR} - // (eject path) is validated at provision time. - if !containsVarRef(vnetID) && !vnetIDPattern.MatchString(vnetID) { + // Validate the ARM id shape unless the value can still change: on the eject + // path an unexpanded ${VAR} is validated at provision time. After + // resolveVars there is nothing left to expand, so a leftover ${VAR} (what an + // escaped $${VAR} resolves to) is checked now rather than deferred to a + // provision that can only fail. + if (resolve || !containsVarRef(vnetID)) && !vnetIDPattern.MatchString(vnetID) { return "", "", "", false, fmt.Errorf( "%s.vnet: %q is not a well-formed Microsoft.Network/virtualNetworks id", fieldPath, vnetID) } @@ -1106,10 +1118,14 @@ func sameVNet(a, b string) bool { return strings.EqualFold(a, b) } -// containsVarRef reports whether s still contains a ${VAR} reference, including -// the ${VAR:-default} form. +// containsVarRef reports whether s still carries an azd ${VAR} reference the +// expander will resolve, including the ${VAR:-default} form. +// +// Escaped references and names reserved by a Foundry ${{...}} span do not count: +// [foundry.ExpandEnv] leaves those alone, so the value is already as concrete as +// it will ever be and the caller's own shape validation should run on it. func containsVarRef(s string) bool { - return varRefPattern.MatchString(s) + return len(FindEnvReferences(s)) > 0 } // resolveVars expands ${VAR} references in s using env first, then the @@ -1121,15 +1137,19 @@ func containsVarRef(s string) bool { // exactly as they do elsewhere. // // ExpandEnv resolves through a mapping callback that only receives the variable -// name, so the set of names that must resolve is 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 trip this check. +// name, so the names that must resolve are collected up front from +// [FindEnvReferences]: a name is required only where it occurs at least once +// without a :- default, in a position the expander will actually act on. Reusing +// that scanner is what keeps an escaped or ${{...}} reserved occurrence from +// making a live, defaulted occurrence of the same name look unresolvable. +// +// Callers validate the value with [ValidateEnvReferences] first, so every +// occurrence the expander acts on is one the scanner saw. func resolveVars(s string, env map[string]string) (string, error) { required := map[string]struct{}{} - for _, match := range varRefPattern.FindAllStringSubmatch(s, -1) { - if match[2] == "" { - required[match[1]] = struct{}{} + for _, reference := range FindEnvReferences(s) { + if !reference.HasDefault { + required[reference.Name] = struct{}{} } } diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go index ea0831ad1b4..47674e243a3 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go @@ -6,6 +6,7 @@ package synthesis import ( "encoding/json" "errors" + "fmt" "os" "path/filepath" "testing" @@ -1716,6 +1717,19 @@ func TestResolveVars_MatchesFoundryExpandEnv(t *testing.T) { {name: "empty default is allowed", in: "${MISSING_VAR_XYZ:-}", want: ""}, {name: "empty env value takes the default", in: "${EMPTY_VAR:-fallback}", want: "fallback"}, {name: "escaped reference stays literal", in: "$${MISSING_VAR_XYZ}", want: "${MISSING_VAR_XYZ}"}, + { + // required is derived from the same scanner the expander drives, so + // an occurrence the expander never resolves cannot make a live, + // defaulted occurrence of the same name look unresolvable. + name: "escaped reference does not make a defaulted one required", + in: "$${MISSING_VAR_XYZ} ${MISSING_VAR_XYZ:-fallback}", + want: "${MISSING_VAR_XYZ} fallback", + }, + { + name: "a name in a Foundry span does not make a defaulted one required", + in: "${{connections.${MISSING_VAR_XYZ}.key}} ${MISSING_VAR_XYZ:-fallback}", + want: "${{connections.${MISSING_VAR_XYZ}.key}} fallback", + }, {name: "no references", in: "/subscriptions/abc", want: "/subscriptions/abc"}, { name: "default inside a resource id", @@ -1757,6 +1771,11 @@ func TestResolveVars_MatchesFoundryExpandEnv(t *testing.T) { // a default must be recognized as still-unresolved so the value is kept // verbatim and the ARM-shape checks are deferred to provision time, instead of // being rejected as a malformed resource id. +// +// The converse matters too. An escaped reference and a name reserved by a +// Foundry ${{...}} span are never expanded, so the value is already as concrete +// as it will ever be and the shape checks have to run on it now rather than +// being deferred to a provision that can only fail. func TestContainsVarRef_RecognizesDefaults(t *testing.T) { tests := []struct { in string @@ -1765,6 +1784,9 @@ func TestContainsVarRef_RecognizesDefaults(t *testing.T) { {in: "${VNET}", want: true}, {in: "${VNET:-/subscriptions/s}", want: true}, {in: "/subscriptions/s/resourceGroups/rg", want: false}, + {in: "$${VNET}", want: false}, + {in: "${{connections.store.key}}", want: false}, + {in: "$${VNET} ${OTHER}", want: true}, {in: "", want: false}, } @@ -1775,6 +1797,161 @@ func TestContainsVarRef_RecognizesDefaults(t *testing.T) { } } +// TestValidateEnvReferences_RejectsUnsupportedForms pins the guard on +// drone/envsubst's wider grammar. Every rejected form below is one envsubst +// expands and the scanner does not report, so without this check it slips past +// the unresolved-variable guard: ${MISSING:=x} silently resolves, ${MISSING#x} +// silently becomes "", and the caller then validates the rewritten value as if +// the user had typed it. Typing ':=' for ':-' is a one character slip. +func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { + t.Parallel() + + supported := []string{ + "${VAR}", + "${VAR:-default}", + "${VAR:-}", + "$${VAR}", + "${{connections.store.credentials.key}}", + "${{ tools.${INNER} }}", + "${MISSING:-${{event.body}}}", + "${OUTER:-${NESTED}}", + // An escaped Foundry span: ExpandEnv masks the span starting at the + // second '$', so nothing inside it reaches envsubst and the '$' pair is + // never an escape. + "$${{ tools.${INNER} }}", + "/subscriptions/s/resourceGroups/rg", + // Bare '$' forms survive expansion untouched: envsubst only expands the + // braced shape, so these need no rejection. + "$VAR", + "costs $5 today", + "a$b", + "", + } + for _, value := range supported { + t.Run("ok/"+value, func(t *testing.T) { + t.Parallel() + assert.NoError(t, ValidateEnvReferences(value)) + }) + } + + unsupported := []string{ + "${MISSING:=default}", + "${MISSING:+alt}", + "${MISSING:?boom}", + "${MISSING#prefix}", + "${MISSING%suffix}", + "${MISSING:0:3}", + "${MISSING-nodefault}", + "${1BAD}", + "prefix ${MISSING:=x} suffix", + // envsubst evaluates the default expression, so an unsupported form + // nested in one expands exactly like a top-level one. The scanner stops + // at the default, which is why this needs its own step-in. + "${OUTER:-${INNER:=x}}", + } + for _, value := range unsupported { + t.Run("rejected/"+value, func(t *testing.T) { + t.Parallel() + err := ValidateEnvReferences(value) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a supported environment variable reference") + assert.Contains(t, err.Error(), "${VAR:-default}", + "the message has to name the shape the user probably meant") + }) + } + + t.Run("rejected/unterminated foundry span", func(t *testing.T) { + t.Parallel() + err := ValidateEnvReferences("${{connections.store.key}") + require.Error(t, err) + assert.Contains(t, err.Error(), "missing the closing }}") + }) +} + +// TestSynthesize_NetworkRejectsUnsupportedVarSyntax covers the guard end to end +// on both paths. envsubst would expand these, so on the provision path the +// value is silently rewritten before the ARM id / subscription checks see it, +// and on the eject path it is written into the template verbatim and rewritten +// at provision. Either way the user never learns their ':=' did not mean ':-'. +func TestSynthesize_NetworkRejectsUnsupportedVarSyntax(t *testing.T) { + tests := []struct { + name string + yaml string + field string + }{ + { + name: "subnet vnet", + field: "peSubnet.vnet", + yaml: ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: {vnet: "${MISSING_VNET_XYZ:=/subscriptions/s}", name: pe-subnet} +`, + }, + { + name: "dns subscription", + field: "dns.subscription", + yaml: ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: + vnet: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/v + name: pe-subnet + dns: + subscription: "${MISSING_SUB_XYZ#prefix}" +`, + }, + } + + for _, tt := range tests { + for _, preserve := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/preserveVarRefs=%v", tt.name, preserve), func(t *testing.T) { + _, err := Synthesize(Input{ + RawAzureYAML: []byte(tt.yaml), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + PreserveVarRefs: preserve, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a supported environment variable reference") + assert.Contains(t, err.Error(), tt.field, + "the error has to name the offending field") + }) + } + } +} + +// TestSynthesize_NetworkEscapedRefIsValidatedOnBothPaths pins that an escaped +// reference is final on both paths. $${VNET} resolves to the literal ${VNET}, +// which is not a vnet id and never becomes one, so deferring the shape check to +// provision only moves the failure somewhere less useful — and made eject and +// provision disagree about the same azure.yaml. +func TestSynthesize_NetworkEscapedRefIsValidatedOnBothPaths(t *testing.T) { + const yaml = ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: {vnet: "$${VNET_XYZ}", name: pe-subnet} +` + for _, preserve := range []bool{false, true} { + t.Run(fmt.Sprintf("preserveVarRefs=%v", preserve), func(t *testing.T) { + _, err := Synthesize(Input{ + RawAzureYAML: []byte(yaml), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + PreserveVarRefs: preserve, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a well-formed Microsoft.Network/virtualNetworks id") + }) + } +} + // TestSynthesize_NetworkVarRefDefaults covers the reported bug end to end: // ${VAR:-default} on the three network fields previously fell through // resolveVars unchanged and was then rejected by the ARM id / subscription From 7d1539830cab619e3ffe51ffa108a00fca8869b9 Mon Sep 17 00:00:00 2001 From: Glenn Harper Date: Tue, 4 Aug 2026 11:49:12 -0400 Subject: [PATCH 3/7] Shorten the vnet fixture in the unsupported-syntax test The subscription segment is irrelevant to what that case asserts (the dns.subscription refusal) and vnetIDPattern accepts any non-slash segment, so the full GUID only made the line long. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9 --- .../azure.ai.projects/internal/synthesis/synthesizer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go index 47674e243a3..40df7392722 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go @@ -1899,7 +1899,7 @@ services: host: azure.ai.project network: peSubnet: - vnet: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/v + vnet: /subscriptions/s/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/v name: pe-subnet dns: subscription: "${MISSING_SUB_XYZ#prefix}" From bc22d6cc5c10e3c904437eae805c178c1e3fa425 Mon Sep 17 00:00:00 2001 From: Glenn Harper Date: Tue, 4 Aug 2026 12:13:50 -0400 Subject: [PATCH 4/7] Refuse nested references, scope the completeness claim, fix a stale comment Addresses the second review round on #9367. - ValidateEnvReferences now refuses a reference nested in a ':-' default. The expander resolves it and the scanner deliberately does not report it, so ${A:-${B}} with neither set expanded to empty and the field's own shape check then blamed the empty value instead of naming B. Refusing it is what actually makes the scan complete for the fields that run the validator, rather than only closing the unsupported-grammar half. - Scope the "complete by construction" claim in the doc comment. It is a property of calling the validator, not of the scanner, and today only the three project network fields call it. Discovery-only consumers still scan unvalidated values -- an agent env: value of ${BAR:=x} yields no references, so init never prompts for BAR and ExpandEnv rewrites it at deploy. Tracked by #9428. - resolveSubnet's header comment still said the vnet id is validated "only when fully concrete", which stopped being true when the shape check started running on the provision path regardless. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9 --- .../internal/synthesis/envrefs.go | 39 +++++++++++++++---- .../internal/synthesis/synthesizer.go | 5 ++- .../internal/synthesis/envrefs.go | 39 +++++++++++++++---- .../internal/synthesis/synthesizer.go | 5 ++- .../internal/synthesis/synthesizer_test.go | 26 ++++++++++--- 5 files changed, 91 insertions(+), 23 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go index 5e8ab1a7284..2cdfe26d60f 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go @@ -67,7 +67,7 @@ func FindEnvReferences(value string) []EnvReference { } // ValidateEnvReferences reports an error when value carries a '$' form that -// [foundry.ExpandEnv] would act on but azd does not model. +// [foundry.ExpandEnv] would act on but [FindEnvReferences] does not report. // // drone/envsubst, which backs the expander, implements the full shell parameter // grammar: ${VAR:=default}, ${VAR:+alt}, ${VAR:?message}, ${VAR#prefix} and @@ -77,11 +77,27 @@ func FindEnvReferences(value string) []EnvReference { // Typing ':=' instead of ':-' is a one character slip that would otherwise // succeed while skipping the very guard it looks like it is using. // -// Rejecting the whole grammar except the four shapes azd documents keeps -// [FindEnvReferences] complete by construction: after this passes, every -// occurrence the expander acts on is one the scanner saw. +// A reference nested in a :- default is refused for the same reason: the +// expander resolves it, the scanner deliberately does not report it, so +// ${A:-${B}} with neither set expands to empty and the caller then blames the +// empty value rather than naming B. +// +// Where this runs, it makes [FindEnvReferences] complete: every occurrence the +// expander acts on is one the scanner saw. That is a property of the *call*, +// not of the scanner — only callers that invoke this get it. Today that is the +// three project network fields (network.agentSubnet.vnet, network.peSubnet.vnet, +// network.dns.subscription). Discovery-only consumers, such as init prompting +// and the generated service env block, still scan values that were never +// validated; extending the check to them is tracked by +// https://github.com/Azure/azure-dev/issues/9428. func ValidateEnvReferences(value string) error { + // Non-zero while scanning the inside of a :- default. Nesting is refused on + // sight, so one boundary is enough — there is never a second level. + defaultEnd := 0 for index := 0; index < len(value); { + if defaultEnd > 0 && index >= defaultEnd { + defaultEnd = 0 + } if value[index] != '$' { index++ continue @@ -95,7 +111,8 @@ func ValidateEnvReferences(value string) error { index += 2 continue } - // A Foundry span is reserved verbatim for the service to resolve. + // A Foundry span is reserved verbatim for the service to resolve. Legal + // as a default value, so this stays allowed inside one. if strings.HasPrefix(value[index:], "${{") { end := strings.Index(value[index+3:], "}}") if end < 0 { @@ -111,6 +128,13 @@ func ValidateEnvReferences(value string) error { index++ continue } + if defaultEnd > 0 { + return fmt.Errorf( + "%q nests an environment variable reference inside a :- default, which azd does "+ + "not resolve as one; give the default a literal value, or use ${{...}} for a "+ + "Foundry expression", + unsupportedEnvFragment(value, index)) + } reference, found := envReferenceAt(value, index) if !found { @@ -122,8 +146,9 @@ func ValidateEnvReferences(value string) error { if reference.HasDefault { // Step into the default rather than over it. FindEnvReferences stops // at the default because nested references are not *discovered*; - // envsubst still *expands* them, so an unsupported form nested in a - // default reaches the expander exactly like a top-level one. + // envsubst still *expands* whatever is in there, so both an + // unsupported form and a nested reference have to be caught here. + defaultEnd = reference.End index = reference.Start + len("${") + len(reference.Name) + len(":-") continue } diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go index fa333944f6d..2179527eb67 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go @@ -1058,8 +1058,9 @@ func synthesizeNetwork( // vnet + name + prefix -> create subnet with that CIDR (create=true) // // vnet and name are required; ${VAR} references in vnet are expanded when -// resolve is true and validated as a Microsoft.Network/virtualNetworks id only -// when fully concrete. +// resolve is true. The Microsoft.Network/virtualNetworks id shape is then +// checked, except on the eject path (resolve false), where an unexpanded +// reference is left for provision time to validate. func resolveSubnet( s *subnetSpec, fieldPath string, env map[string]string, resolve bool, ) (vnetID, name, prefix string, create bool, err error) { diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go index 5e8ab1a7284..2cdfe26d60f 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go @@ -67,7 +67,7 @@ func FindEnvReferences(value string) []EnvReference { } // ValidateEnvReferences reports an error when value carries a '$' form that -// [foundry.ExpandEnv] would act on but azd does not model. +// [foundry.ExpandEnv] would act on but [FindEnvReferences] does not report. // // drone/envsubst, which backs the expander, implements the full shell parameter // grammar: ${VAR:=default}, ${VAR:+alt}, ${VAR:?message}, ${VAR#prefix} and @@ -77,11 +77,27 @@ func FindEnvReferences(value string) []EnvReference { // Typing ':=' instead of ':-' is a one character slip that would otherwise // succeed while skipping the very guard it looks like it is using. // -// Rejecting the whole grammar except the four shapes azd documents keeps -// [FindEnvReferences] complete by construction: after this passes, every -// occurrence the expander acts on is one the scanner saw. +// A reference nested in a :- default is refused for the same reason: the +// expander resolves it, the scanner deliberately does not report it, so +// ${A:-${B}} with neither set expands to empty and the caller then blames the +// empty value rather than naming B. +// +// Where this runs, it makes [FindEnvReferences] complete: every occurrence the +// expander acts on is one the scanner saw. That is a property of the *call*, +// not of the scanner — only callers that invoke this get it. Today that is the +// three project network fields (network.agentSubnet.vnet, network.peSubnet.vnet, +// network.dns.subscription). Discovery-only consumers, such as init prompting +// and the generated service env block, still scan values that were never +// validated; extending the check to them is tracked by +// https://github.com/Azure/azure-dev/issues/9428. func ValidateEnvReferences(value string) error { + // Non-zero while scanning the inside of a :- default. Nesting is refused on + // sight, so one boundary is enough — there is never a second level. + defaultEnd := 0 for index := 0; index < len(value); { + if defaultEnd > 0 && index >= defaultEnd { + defaultEnd = 0 + } if value[index] != '$' { index++ continue @@ -95,7 +111,8 @@ func ValidateEnvReferences(value string) error { index += 2 continue } - // A Foundry span is reserved verbatim for the service to resolve. + // A Foundry span is reserved verbatim for the service to resolve. Legal + // as a default value, so this stays allowed inside one. if strings.HasPrefix(value[index:], "${{") { end := strings.Index(value[index+3:], "}}") if end < 0 { @@ -111,6 +128,13 @@ func ValidateEnvReferences(value string) error { index++ continue } + if defaultEnd > 0 { + return fmt.Errorf( + "%q nests an environment variable reference inside a :- default, which azd does "+ + "not resolve as one; give the default a literal value, or use ${{...}} for a "+ + "Foundry expression", + unsupportedEnvFragment(value, index)) + } reference, found := envReferenceAt(value, index) if !found { @@ -122,8 +146,9 @@ func ValidateEnvReferences(value string) error { if reference.HasDefault { // Step into the default rather than over it. FindEnvReferences stops // at the default because nested references are not *discovered*; - // envsubst still *expands* them, so an unsupported form nested in a - // default reaches the expander exactly like a top-level one. + // envsubst still *expands* whatever is in there, so both an + // unsupported form and a nested reference have to be caught here. + defaultEnd = reference.End index = reference.Start + len("${") + len(reference.Name) + len(":-") continue } diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go index fa333944f6d..2179527eb67 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go @@ -1058,8 +1058,9 @@ func synthesizeNetwork( // vnet + name + prefix -> create subnet with that CIDR (create=true) // // vnet and name are required; ${VAR} references in vnet are expanded when -// resolve is true and validated as a Microsoft.Network/virtualNetworks id only -// when fully concrete. +// resolve is true. The Microsoft.Network/virtualNetworks id shape is then +// checked, except on the eject path (resolve false), where an unexpanded +// reference is left for provision time to validate. func resolveSubnet( s *subnetSpec, fieldPath string, env map[string]string, resolve bool, ) (vnetID, name, prefix string, create bool, err error) { diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go index 40df7392722..271138c1b35 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go @@ -1814,7 +1814,6 @@ func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { "${{connections.store.credentials.key}}", "${{ tools.${INNER} }}", "${MISSING:-${{event.body}}}", - "${OUTER:-${NESTED}}", // An escaped Foundry span: ExpandEnv masks the span starting at the // second '$', so nothing inside it reaches envsubst and the '$' pair is // never an escape. @@ -1844,10 +1843,6 @@ func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { "${MISSING-nodefault}", "${1BAD}", "prefix ${MISSING:=x} suffix", - // envsubst evaluates the default expression, so an unsupported form - // nested in one expands exactly like a top-level one. The scanner stops - // at the default, which is why this needs its own step-in. - "${OUTER:-${INNER:=x}}", } for _, value := range unsupported { t.Run("rejected/"+value, func(t *testing.T) { @@ -1860,6 +1855,27 @@ func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { }) } + // A nested reference is expanded but never discovered, so ${A:-${B}} with + // neither set resolves to empty and the field's own shape check then blames + // the empty value instead of naming B. Refusing it is what makes the scan + // complete for the fields that run this. + nested := []string{ + "${OUTER:-${NESTED}}", + "${OUTER:-prefix-${NESTED}-suffix}", + "${OUTER:-${NESTED:-inner}}", + // Unsupported grammar nested in a default is caught by the same walk; + // without stepping into the default it would expand unseen. + "${OUTER:-${INNER:=x}}", + } + for _, value := range nested { + t.Run("nested/"+value, func(t *testing.T) { + t.Parallel() + err := ValidateEnvReferences(value) + require.Error(t, err) + assert.Contains(t, err.Error(), "nests an environment variable reference inside a :- default") + }) + } + t.Run("rejected/unterminated foundry span", func(t *testing.T) { t.Parallel() err := ValidateEnvReferences("${{connections.store.key}") From db503c79dff0193bd5d4b9e42f0274db3d30e874 Mon Sep 17 00:00:00 2001 From: Glenn Harper Date: Tue, 4 Aug 2026 12:42:58 -0400 Subject: [PATCH 5/7] State the real reason nested references are refused, and quote them whole Addresses the third review round on #9367. - The doc comment justified refusing ${A:-${B}} with the "expands to empty" case, but the refusal is broader than that: ${A:-${B}} with B set resolves correctly today, so a working ${VNET_ID:-${FALLBACK_VNET_ID}} is being withdrawn. The load-bearing reason is that `required` is static -- whether the nested name has to resolve depends on whether the outer one does, which is unknown at scan time, so reporting it fails whenever the outer name IS set and not reporting it expands to empty. Both the comment and the error message now say that, and the message points at setting a single ${VAR} in the azd environment rather than telling the user to hardcode a resource id. - unsupportedEnvFragment truncates at the first '}', which is right for a form the span scanner cannot bound but wrong for a nested reference that can be: "${A:-${B:-${C}}}" quoted an unbalanced "${B:-${C}". The nested branch now uses envReferenceAt's span when it parses and falls back to the fragment only when it does not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9 --- .../internal/synthesis/envrefs.go | 31 ++++++++++++++----- .../internal/synthesis/envrefs.go | 31 ++++++++++++++----- .../internal/synthesis/synthesizer_test.go | 26 ++++++++++------ 3 files changed, 62 insertions(+), 26 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go index 2cdfe26d60f..794e3e9417e 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go @@ -77,10 +77,16 @@ func FindEnvReferences(value string) []EnvReference { // Typing ':=' instead of ':-' is a one character slip that would otherwise // succeed while skipping the very guard it looks like it is using. // -// A reference nested in a :- default is refused for the same reason: the -// expander resolves it, the scanner deliberately does not report it, so -// ${A:-${B}} with neither set expands to empty and the caller then blames the -// empty value rather than naming B. +// A reference nested in a :- default is refused too — and that is a shape which +// resolves correctly today whenever the nested name is set: +// ${VNET_ID:-${FALLBACK_VNET_ID}} works on a project that has FALLBACK_VNET_ID +// in its environment. 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 — which is not known 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 caller blames the empty value instead of naming +// B. Neither half is right, so the shape goes. // // Where this runs, it makes [FindEnvReferences] complete: every occurrence the // expander acts on is one the scanner saw. That is a property of the *call*, @@ -129,11 +135,20 @@ func ValidateEnvReferences(value string) error { continue } if defaultEnd > 0 { + // A nested reference can usually be bounded, so quote its real span + // rather than the truncated fragment, which would come out + // unbalanced once the nested one carries its own default. + fragment := unsupportedEnvFragment(value, index) + if nested, ok := envReferenceAt(value, index); ok { + fragment = value[nested.Start:nested.End] + } return fmt.Errorf( - "%q nests an environment variable reference inside a :- default, which azd does "+ - "not resolve as one; give the default a literal value, or use ${{...}} for a "+ - "Foundry expression", - unsupportedEnvFragment(value, index)) + "%q 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", + fragment) } reference, found := envReferenceAt(value, index) diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go index 2cdfe26d60f..794e3e9417e 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go @@ -77,10 +77,16 @@ func FindEnvReferences(value string) []EnvReference { // Typing ':=' instead of ':-' is a one character slip that would otherwise // succeed while skipping the very guard it looks like it is using. // -// A reference nested in a :- default is refused for the same reason: the -// expander resolves it, the scanner deliberately does not report it, so -// ${A:-${B}} with neither set expands to empty and the caller then blames the -// empty value rather than naming B. +// A reference nested in a :- default is refused too — and that is a shape which +// resolves correctly today whenever the nested name is set: +// ${VNET_ID:-${FALLBACK_VNET_ID}} works on a project that has FALLBACK_VNET_ID +// in its environment. 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 — which is not known 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 caller blames the empty value instead of naming +// B. Neither half is right, so the shape goes. // // Where this runs, it makes [FindEnvReferences] complete: every occurrence the // expander acts on is one the scanner saw. That is a property of the *call*, @@ -129,11 +135,20 @@ func ValidateEnvReferences(value string) error { continue } if defaultEnd > 0 { + // A nested reference can usually be bounded, so quote its real span + // rather than the truncated fragment, which would come out + // unbalanced once the nested one carries its own default. + fragment := unsupportedEnvFragment(value, index) + if nested, ok := envReferenceAt(value, index); ok { + fragment = value[nested.Start:nested.End] + } return fmt.Errorf( - "%q nests an environment variable reference inside a :- default, which azd does "+ - "not resolve as one; give the default a literal value, or use ${{...}} for a "+ - "Foundry expression", - unsupportedEnvFragment(value, index)) + "%q 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", + fragment) } reference, found := envReferenceAt(value, index) diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go index 271138c1b35..0e619c4859a 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go @@ -1855,24 +1855,30 @@ func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { }) } - // A nested reference is expanded but never discovered, so ${A:-${B}} with - // neither set resolves to empty and the field's own shape check then blames - // the empty value instead of naming B. Refusing it is what makes the scan - // complete for the fields that run this. - nested := []string{ - "${OUTER:-${NESTED}}", - "${OUTER:-prefix-${NESTED}-suffix}", - "${OUTER:-${NESTED:-inner}}", + // A nested reference is expanded but never discovered. Refusing it withdraws + // a shape that works today when the nested name is set, because `required` + // is static: reporting the nested name would fail whenever the outer one + // resolves, and not reporting it lets ${A:-${B}} with neither set expand to + // empty, so the field's own shape check blames the empty value. + nested := map[string]string{ + "${OUTER:-${NESTED}}": "${NESTED}", + "${OUTER:-prefix-${NESTED}-suffix}": "${NESTED}", + "${OUTER:-${NESTED:-inner}}": "${NESTED:-inner}", + // The quoted fragment has to be the nested reference's real span; a + // truncate-at-the-first-'}' fragment would come out unbalanced here. + "${A:-${B:-${C}}}": "${B:-${C}}", // Unsupported grammar nested in a default is caught by the same walk; // without stepping into the default it would expand unseen. - "${OUTER:-${INNER:=x}}", + "${OUTER:-${INNER:=x}}": "${INNER:=x}", } - for _, value := range nested { + for value, fragment := range nested { t.Run("nested/"+value, func(t *testing.T) { t.Parallel() err := ValidateEnvReferences(value) require.Error(t, err) assert.Contains(t, err.Error(), "nests an environment variable reference inside a :- default") + assert.Contains(t, err.Error(), fmt.Sprintf("%q", fragment), + "the message has to quote the nested reference's real span") }) } From 472beda61ec84983187c5fba7b6f44b75664b9c2 Mon Sep 17 00:00:00 2001 From: Glenn Harper Date: Tue, 4 Aug 2026 15:45:13 -0400 Subject: [PATCH 6/7] Route unsupported nested env syntax to its accurate error A parseable nested reference still gets the nesting-specific diagnostic, because static required-name analysis cannot express whether it is needed. When envReferenceAt rejects the nested text, let the existing unsupported- form branch report the actual problem instead. This keeps ${OUTER:-${INNER:=x}} and ${A:-${9BAD}} from being described as conditional required-name failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9 --- .../azure.ai.agents/internal/synthesis/envrefs.go | 14 +++----------- .../internal/synthesis/envrefs.go | 14 +++----------- .../internal/synthesis/synthesizer_test.go | 5 ++--- 3 files changed, 8 insertions(+), 25 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go index 794e3e9417e..d23af8b2ea8 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go @@ -134,24 +134,16 @@ func ValidateEnvReferences(value string) error { index++ continue } - if defaultEnd > 0 { - // A nested reference can usually be bounded, so quote its real span - // rather than the truncated fragment, which would come out - // unbalanced once the nested one carries its own default. - fragment := unsupportedEnvFragment(value, index) - if nested, ok := envReferenceAt(value, index); ok { - fragment = value[nested.Start:nested.End] - } + reference, found := envReferenceAt(value, index) + if defaultEnd > 0 && found { return fmt.Errorf( "%q 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", - fragment) + value[reference.Start:reference.End]) } - - reference, found := envReferenceAt(value, index) if !found { return fmt.Errorf( "%q is not a supported environment variable reference; use ${VAR} or "+ diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go index 794e3e9417e..d23af8b2ea8 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go @@ -134,24 +134,16 @@ func ValidateEnvReferences(value string) error { index++ continue } - if defaultEnd > 0 { - // A nested reference can usually be bounded, so quote its real span - // rather than the truncated fragment, which would come out - // unbalanced once the nested one carries its own default. - fragment := unsupportedEnvFragment(value, index) - if nested, ok := envReferenceAt(value, index); ok { - fragment = value[nested.Start:nested.End] - } + reference, found := envReferenceAt(value, index) + if defaultEnd > 0 && found { return fmt.Errorf( "%q 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", - fragment) + value[reference.Start:reference.End]) } - - reference, found := envReferenceAt(value, index) if !found { return fmt.Errorf( "%q is not a supported environment variable reference; use ${VAR} or "+ diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go index 0e619c4859a..ea8db0527d8 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go @@ -1843,6 +1843,8 @@ func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { "${MISSING-nodefault}", "${1BAD}", "prefix ${MISSING:=x} suffix", + "${OUTER:-${INNER:=x}}", + "${A:-${9BAD}}", } for _, value := range unsupported { t.Run("rejected/"+value, func(t *testing.T) { @@ -1867,9 +1869,6 @@ func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { // The quoted fragment has to be the nested reference's real span; a // truncate-at-the-first-'}' fragment would come out unbalanced here. "${A:-${B:-${C}}}": "${B:-${C}}", - // Unsupported grammar nested in a default is caught by the same walk; - // without stepping into the default it would expand unseen. - "${OUTER:-${INNER:=x}}": "${INNER:=x}", } for value, fragment := range nested { t.Run("nested/"+value, func(t *testing.T) { From cc1c1fbfbc640ec14381e966c8f9e3171a555cbb Mon Sep 17 00:00:00 2001 From: Glenn Harper Date: Wed, 5 Aug 2026 11:27:12 -0400 Subject: [PATCH 7/7] fix(foundry): reject escaped nested refs in defaults Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fed9e97b-e79b-4889-ac76-0d9a428599cd --- .../azure.ai.agents/internal/synthesis/envrefs.go | 12 +++++++----- .../azure.ai.projects/internal/synthesis/envrefs.go | 12 +++++++----- .../internal/synthesis/synthesizer_test.go | 8 +++++--- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go index d23af8b2ea8..5afddf74645 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go @@ -108,11 +108,13 @@ func ValidateEnvReferences(value string) error { index++ continue } - // A '$' pair collapses to a literal '$', neutralizing only the '${' it - // precedes — unless the second '$' opens a Foundry span, which - // foundry.ExpandEnv masks before envsubst ever sees the pair. Leaving - // that '$' for the span rule below keeps the two in step. - if strings.HasPrefix(value[index:], "$$") && + // Outside a default, a '$' pair collapses to a literal '$', + // neutralizing only the '${' it precedes. Inside a :- default, + // envsubst re-parses the default text and the second '$' can still open + // a live ${VAR}, so leave it for the nested-reference check below. + // A Foundry span is masked before envsubst sees the pair in either case. + if defaultEnd == 0 && + strings.HasPrefix(value[index:], "$$") && !strings.HasPrefix(value[index+1:], "${{") { index += 2 continue diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go index d23af8b2ea8..5afddf74645 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go @@ -108,11 +108,13 @@ func ValidateEnvReferences(value string) error { index++ continue } - // A '$' pair collapses to a literal '$', neutralizing only the '${' it - // precedes — unless the second '$' opens a Foundry span, which - // foundry.ExpandEnv masks before envsubst ever sees the pair. Leaving - // that '$' for the span rule below keeps the two in step. - if strings.HasPrefix(value[index:], "$$") && + // Outside a default, a '$' pair collapses to a literal '$', + // neutralizing only the '${' it precedes. Inside a :- default, + // envsubst re-parses the default text and the second '$' can still open + // a live ${VAR}, so leave it for the nested-reference check below. + // A Foundry span is masked before envsubst sees the pair in either case. + if defaultEnd == 0 && + strings.HasPrefix(value[index:], "$$") && !strings.HasPrefix(value[index+1:], "${{") { index += 2 continue diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go index ea8db0527d8..59371b092a0 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go @@ -1863,9 +1863,11 @@ func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { // resolves, and not reporting it lets ${A:-${B}} with neither set expand to // empty, so the field's own shape check blames the empty value. nested := map[string]string{ - "${OUTER:-${NESTED}}": "${NESTED}", - "${OUTER:-prefix-${NESTED}-suffix}": "${NESTED}", - "${OUTER:-${NESTED:-inner}}": "${NESTED:-inner}", + "${OUTER:-${NESTED}}": "${NESTED}", + "${OUTER:-prefix-${NESTED}-suffix}": "${NESTED}", + "${OUTER:-${NESTED:-inner}}": "${NESTED:-inner}", + "${OUTER:-$${NESTED}}": "${NESTED}", + "${OUTER:-prefix-$${NESTED}-suffix}": "${NESTED}", // The quoted fragment has to be the nested reference's real span; a // truncate-at-the-first-'}' fragment would come out unbalanced here. "${A:-${B:-${C}}}": "${B:-${C}}",