diff --git a/cli/azd/extensions/azure.ai.agents/README.md b/cli/azd/extensions/azure.ai.agents/README.md index e84a4f60c99..b9c6b46b30a 100644 --- a/cli/azd/extensions/azure.ai.agents/README.md +++ b/cli/azd/extensions/azure.ai.agents/README.md @@ -50,6 +50,26 @@ services: description: My hosted agent ``` +### Environment variables under `config:` + +Older projects could also set environment variables in an `env:` block nested +under the service's `config:`. That position is no longer read: azd takes the +service environment only from the service-level `env:`. A service that still +carries `config: env:` gets a warning naming the affected variables on both +`azd ai agent run` and `azd deploy`. + +Move them up one level to fix it: + +```yaml +services: + my-agent: + host: azure.ai.agent + project: . + env: + API_KEY: ${SECRET} + LOG_LEVEL: debug +``` + ## Content safety policies A hosted agent can be bound to an Azure AI Content Safety (RAI) policy so every 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 new file mode 100644 index 00000000000..c4920873f7a --- /dev/null +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go @@ -0,0 +1,233 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +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. +// Fields resolved by foundry.ExpandEnv take +// honorEnvironmentEscaping: it collapses '$' pairs, so $${VAR} +// stays literal, and it reserves ${{...}} spans for Foundry. +// The three project network fields (network.agentSubnet.vnet, +// network.peSubnet.vnet, network.dns.subscription) take +// ignoreEnvironmentEscaping because resolveVars in the projects +// synthesizer is a plain regex replace with no '$$' handling, +// so $${VAR} does expand there. The split mirrors that existing +// divergence rather than choosing two policies; it collapses +// once resolveVars moves to foundry.ExpandEnv. +// +// resolveVars also diverges on ':-'. Its pattern matches only +// ${NAME}, so a ${NAME:-default} on one of those three fields is +// never substituted and no error is raised; the literal then +// fails the field's own ARM id or subscription validation. No +// escaping flag can mirror that, so the scanner still reports +// the name and the gap is tracked upstream. +// See: https://github.com/Azure/azure-dev/issues/9350 +const ( + honorEnvironmentEscaping = true + ignoreEnvironmentEscaping = false +) + +// 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. +// +// 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. +// +// 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 +} 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 new file mode 100644 index 00000000000..99f2fded480 --- /dev/null +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go @@ -0,0 +1,311 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package cmd + +import ( + "testing" + + "github.com/azure/azure-dev/cli/azd/pkg/foundry" + "github.com/stretchr/testify/require" +) + +// TestFindEnvironmentReferencesEscapedOuterKeepsInnerLive pins a +// case the two former scanners disagreed on: an escape neutralizes +// only the '${' it precedes, so a reference inside the escaped +// default is still live. foundry.ExpandEnv turns "$${A:-${B}}" +// into "${A:-}". +func TestFindEnvironmentReferencesEscapedOuterKeepsInnerLive(t *testing.T) { + t.Parallel() + + got := findEnvironmentReferences("$${A:-${B}}", honorEnvironmentEscaping) + require.Equal(t, []environmentReference{{Name: "B", Start: 6, End: 10}}, got) +} + +func TestFindEnvironmentReferences(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + value string + honorEscaping bool + want []environmentReference + }{ + { + name: "bare reference", + value: "${PLAIN}", + honorEscaping: honorEnvironmentEscaping, + want: []environmentReference{{Name: "PLAIN", Start: 0, End: 8}}, + }, + { + name: "reference with default", + value: "${NAME:-fallback}", + honorEscaping: honorEnvironmentEscaping, + want: []environmentReference{ + {Name: "NAME", Start: 0, End: 17, HasDefault: true}, + }, + }, + { + name: "multiple references keep order", + value: "prefix ${ONE} mid ${TWO:-x} suffix", + honorEscaping: honorEnvironmentEscaping, + want: []environmentReference{ + {Name: "ONE", Start: 7, End: 13}, + {Name: "TWO", Start: 18, End: 27, HasDefault: true}, + }, + }, + { + // drone/envsubst collapses '$' pairs, so one leading '$' + // escapes the reference. + name: "single escape is dropped", + value: "$${VAR}", + honorEscaping: honorEnvironmentEscaping, + 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: "triple escape is dropped", + value: "$$$${VAR}", + honorEscaping: honorEnvironmentEscaping, + 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}}", + honorEscaping: honorEnvironmentEscaping, + 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, + want: []environmentReference{ + {Name: "MISSING", Start: 0, End: 27, HasDefault: true}, + }, + }, + { + // 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, + want: []environmentReference{ + {Name: "OUTER", Start: 0, End: 19, HasDefault: true}, + {Name: "AFTER", Start: 20, End: 28}, + }, + }, + { + // 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, + }, + { + // 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: "invalid name yields nothing", + value: "${1BAD}", + honorEscaping: honorEnvironmentEscaping, + want: nil, + }, + { + name: "unterminated reference yields nothing", + value: "${UNCLOSED", + honorEscaping: honorEnvironmentEscaping, + want: nil, + }, + { + name: "plain text yields nothing", + value: "no references here", + honorEscaping: honorEnvironmentEscaping, + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := findEnvironmentReferences(tt.value, tt.honorEscaping) + require.Equal(t, tt.want, got) + }) + } +} + +// TestFindEnvironmentReferencesPolicies pins the one intended +// difference between the two consumers: init only prompts for +// references the expander cannot resolve on its own, while the +// generated service env block records every name so the owning +// extension can re-apply defaults. +func TestFindEnvironmentReferencesPolicies(t *testing.T) { + t.Parallel() + + const value = "${BARE} ${WITH_DEFAULT:-fallback}" + + t.Run("init prompting skips defaults", func(t *testing.T) { + t.Parallel() + + var references []azureYamlEnvironmentReference + collectAzureYamlEnvironmentReferences( + value, + false, + honorEnvironmentEscaping, + &references, + map[string]int{}, + ) + require.Equal(t, []azureYamlEnvironmentReference{{Name: "BARE"}}, references) + }) + + t.Run("service env block records defaults", func(t *testing.T) { + t.Parallel() + + environment := map[string]string{} + collectStringEnvironmentTemplates(value, environment) + require.Equal(t, map[string]string{ + "BARE": "${BARE}", + "WITH_DEFAULT": "${WITH_DEFAULT}", + }, environment) + }) +} + +// TestNestedDefaultIsNotDiscovered pins the agreed limitation: +// azd nested references are unsupported, so NESTED reaches +// neither consumer. Only the env block keeps the outer name; +// init prompting drops it too because it carries a default. +// Without this, ${OUTER:-${NESTED}} would look half-supported. +func TestNestedDefaultIsNotDiscovered(t *testing.T) { + t.Parallel() + + const value = "${OUTER:-${NESTED}}" + + environment := map[string]string{} + collectStringEnvironmentTemplates(value, environment) + require.Equal(t, map[string]string{"OUTER": "${OUTER}"}, environment) + + var references []azureYamlEnvironmentReference + collectAzureYamlEnvironmentReferences( + value, + false, + honorEnvironmentEscaping, + &references, + map[string]int{}, + ) + require.Empty(t, references) +} + +func TestCollectAzureYamlEnvironmentReferencesUpgradesSecret(t *testing.T) { + t.Parallel() + + var references []azureYamlEnvironmentReference + indexByName := map[string]int{} + collectAzureYamlEnvironmentReferences( + "${SHARED}", + false, + honorEnvironmentEscaping, + &references, + indexByName, + ) + collectAzureYamlEnvironmentReferences( + "${SHARED}", + true, + honorEnvironmentEscaping, + &references, + indexByName, + ) + + require.Equal( + t, + []azureYamlEnvironmentReference{{Name: "SHARED", Secret: true}}, + references, + ) +} + +// TestFindEnvironmentReferencesMatchesExpander guards against the +// scanner drifting from foundry.ExpandEnv, which is what actually +// resolves these values at deploy. Every name the scanner reports +// must be one the expander asks for, so a generated env block +// never declares a variable the expander leaves alone. The corpus +// covers both opening shapes: '$' followed by '{', and a bare '$' +// the expander ignores. +func TestFindEnvironmentReferencesMatchesExpander(t *testing.T) { + t.Parallel() + + values := []string{ + "${PLAIN}", + "${NAME:-fallback}", + "prefix ${ONE} mid ${TWO:-x} suffix", + "$${ESCAPED}", + "$$${DOUBLE_ESCAPED}", + "$$$${TRIPLE_ESCAPED}", + "$${OUTER:-${INNER}}", + "${{connections.store.credentials.key}}", + "${{ tools.${RESERVED} }}", + "${MISSING:-${{event.body}}}", + "${{f.g}}${AFTER}", + "${BEFORE}${{f.g}}", + "https://${HOST}/v1/${PATH:-default}", + "$foo}", + "prefix $bar} suffix", + "costs $price} today", + "$ab:-${REAL}}", + } + + for _, value := range values { + t.Run(value, func(t *testing.T) { + t.Parallel() + + lookedUp := map[string]bool{} + _, err := foundry.ExpandEnv(value, func(name string) string { + lookedUp[name] = true + return "value_" + name + }) + require.NoError(t, err) + + for _, reference := range findEnvironmentReferences(value, honorEnvironmentEscaping) { + require.Truef( + t, + lookedUp[reference.Name], + "scanner reported %q but the expander never resolves it", + reference.Name, + ) + } + }) + } +} diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/helpers.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/helpers.go index 688ecb14c0f..fe50486b22e 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/helpers.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/helpers.go @@ -10,7 +10,6 @@ import ( "fmt" "io" "log" - "maps" "net/http" "net/url" "os" @@ -24,13 +23,13 @@ import ( "azureaiagent/internal/pkg/agents/agent_api" "azureaiagent/internal/pkg/agents/agent_yaml" "azureaiagent/internal/pkg/paths" - "azureaiagent/internal/pkg/projectconfig" projectpkg "azureaiagent/internal/project" "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/google/uuid" "golang.org/x/term" + "google.golang.org/grpc" ) const ( @@ -941,10 +940,11 @@ func resolveAgentServiceFromProject( // ServiceRunContext holds the resolved context needed for local development. type ServiceRunContext struct { - ServiceName string // the resolved service name (from azure.yaml) - ProjectDir string // absolute path to the service source directory - StartupCommand string // startupCommand from AdditionalProperties (may be empty) - Environment map[string]string + ServiceName string // the resolved service name (from azure.yaml) + ProjectDir string // absolute path to the service source directory + StartupCommand string // startupCommand from AdditionalProperties (may be empty) + ServiceEnvironment map[string]string // values already expanded by azd core + HasServiceEnvironment bool // service declares env: even when empty // Definition is the resolved agent definition (from the inline azure.yaml // entry or a legacy agent.yaml). It is nil when no definition can be resolved. Definition *agent_yaml.ContainerAgent @@ -982,29 +982,12 @@ func resolveServiceRunContext(ctx context.Context, azdClient *azdext.AzdClient, } var startupCmd string - serviceEnv := map[string]string{} if agentConfig, cfgErr := projectpkg.LoadServiceTargetAgentConfig( svc, ); cfgErr == nil { startupCmd = agentConfig.StartupCommand - maps.Copy(serviceEnv, agentConfig.Environment) - } - serviceEnv, err = loadServiceRunEnvironment( - project.Path, - svc, - serviceEnv, - ) - if err != nil { - return nil, exterrors.Validation( - exterrors.CodeInvalidServiceConfig, - fmt.Sprintf( - "failed to load environment for %s: %s", - svc.Name, - err, - ), - "fix the service env configuration in azure.yaml", - ) } + projectpkg.WarnOrphanedConfigEnv(svc) var definition *agent_yaml.ContainerAgent if def, _, source, defErr := projectpkg.LoadAgentDefinition(svc, project.Path); defErr == nil { @@ -1014,37 +997,59 @@ func resolveServiceRunContext(ctx context.Context, azdClient *azdext.AzdClient, } } + // A read failure must not be read as "no env: declared": + // that silently reopens the full azd environment fallback + // below. A missing env: is reported as Found=false with no + // error, so only real failures land here. + hasServiceEnvironment, err := serviceEnvDeclared( + ctx, + azdClient.Project(), + svc.Name, + ) + if err != nil { + return nil, err + } + return &ServiceRunContext{ - ServiceName: svc.Name, - ProjectDir: projectDir, - StartupCommand: startupCmd, - Environment: serviceEnv, - Definition: definition, + ServiceName: svc.Name, + ProjectDir: projectDir, + StartupCommand: startupCmd, + ServiceEnvironment: svc.GetEnvironment(), + HasServiceEnvironment: hasServiceEnvironment, + Definition: definition, }, nil } -func loadServiceRunEnvironment( - projectRoot string, - svc *azdext.ServiceConfig, - base map[string]string, -) (map[string]string, error) { - env := maps.Clone(base) - if env == nil { - env = map[string]string{} - } - raw, err := projectconfig.LoadServiceEnvironment( - projectRoot, - svc.GetName(), - ) +// serviceConfigReader reads raw azure.yaml service config values. +// It mirrors the same seam in the routines and toolboxes targets +// so all three read a declared env: the same way. +type serviceConfigReader interface { + GetServiceConfigValue( + ctx context.Context, + in *azdext.GetServiceConfigValueRequest, + opts ...grpc.CallOption, + ) (*azdext.GetServiceConfigValueResponse, error) +} + +// serviceEnvDeclared reports whether the service declares an env: +// block. An explicit empty env: {} declares an isolated scope, +// and core forwards it as an empty map, indistinguishable from an +// omitted env, so the raw config is the only way to tell them +// apart. Errors are returned rather than treated as "not +// declared", which would fall back to the full azd environment. +func serviceEnvDeclared( + ctx context.Context, + projectClient serviceConfigReader, + serviceName string, +) (bool, error) { + resp, err := projectClient.GetServiceConfigValue(ctx, &azdext.GetServiceConfigValueRequest{ + ServiceName: serviceName, + Path: "env", + }) if err != nil { - return nil, err - } - if raw == nil { - maps.Copy(env, svc.GetEnvironment()) - } else { - maps.Copy(env, raw) + return false, fmt.Errorf("reading env for service %q: %w", serviceName, err) } - return env, nil + return resp.GetFound(), nil } // toServiceKey converts a service name into the env var key format (uppercase, underscores). diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/helpers_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/helpers_test.go index 56ce534b7d0..e2d0c69dca1 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/helpers_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/helpers_test.go @@ -957,40 +957,69 @@ func TestResolveAgentProtocol_MultipleServicesPromptsOnce(t *testing.T) { "resolveAgentProtocol should trigger exactly one prompt") } -func TestLoadServiceRunEnvironmentUsesRawValues(t *testing.T) { +// fakeServiceConfigReader stands in for the project client so the +// declared-env read can be exercised without a live RPC. +type fakeServiceConfigReader struct { + found bool + err error +} + +func (f fakeServiceConfigReader) GetServiceConfigValue( + _ context.Context, + _ *azdext.GetServiceConfigValueRequest, + _ ...grpc.CallOption, +) (*azdext.GetServiceConfigValueResponse, error) { + if f.err != nil { + return nil, f.err + } + return &azdext.GetServiceConfigValueResponse{Found: f.found}, nil +} + +// TestServiceEnvDeclaredFailsClosed pins that a read failure is +// surfaced instead of being reported as "no env: declared". The +// false return is what mergeAgentRunEnvironment reads as legacy, +// so swallowing the error would inject the whole azd environment +// into the agent process, which is the leak this scope closes. +func TestServiceEnvDeclaredFailsClosed(t *testing.T) { t.Parallel() - root := t.TempDir() - require.NoError(t, os.WriteFile( - filepath.Join(root, "azure.yaml"), - []byte(`services: - agent: - host: azure.ai.agent - env: - PROJECT: ${{project.endpoint}} - ENABLED: true - SHARED: direct -`), - 0o600, - )) - svc := &azdext.ServiceConfig{ - Name: "agent", - Environment: map[string]string{ - "PROJECT": "", - "ENABLED": "", - "SHARED": "expanded", + declared, err := serviceEnvDeclared( + t.Context(), + fakeServiceConfigReader{ + err: status.Error(codes.Unavailable, "project service unavailable"), }, + "my-agent", + ) + require.Error(t, err) + require.False(t, declared) + require.ErrorContains(t, err, `reading env for service "my-agent"`) +} + +// TestServiceEnvDeclaredReportsFound pins that a missing env: is +// still a successful read, so a legacy service keeps its full +// azd environment fallback. +func TestServiceEnvDeclaredReportsFound(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + found bool + }{ + {name: "env declared", found: true}, + {name: "env omitted", found: false}, } - env, err := loadServiceRunEnvironment( - root, - svc, - map[string]string{"CONFIG_ONLY": "config", "SHARED": "config"}, - ) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() - require.NoError(t, err) - require.Equal(t, "${{project.endpoint}}", env["PROJECT"]) - require.Equal(t, "true", env["ENABLED"]) - require.Equal(t, "direct", env["SHARED"]) - require.Equal(t, "config", env["CONFIG_ONLY"]) + declared, err := serviceEnvDeclared( + t.Context(), + fakeServiceConfigReader{found: tt.found}, + "my-agent", + ) + require.NoError(t, err) + require.Equal(t, tt.found, declared) + }) + } } diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init.go index 884ca71f428..eebd8d81776 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init.go @@ -2923,6 +2923,7 @@ func (a *InitAction) addToProject(ctx context.Context, targetDir string, agentMa if err != nil { return err } + agentEnvironment := project.AgentEnvironment(containerDef) serviceConfig := &azdext.ServiceConfig{ Name: a.serviceNameOverride, @@ -2956,6 +2957,14 @@ func (a *InitAction) addToProject(ctx context.Context, targetDir string, agentMa if _, err := a.azdClient.Project().AddService(ctx, req); err != nil { return fmt.Errorf("adding agent service to project: %w", err) } + if err := setServiceEnvironment( + ctx, + a.azdClient, + a.serviceNameOverride, + agentEnvironment, + ); err != nil { + return err + } // Emit the sibling Foundry resource services (project + deployments, // connections, toolboxes) and wire the agent's uses: to them. A selected 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..54b87aeb4c5 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 @@ -8,7 +8,6 @@ import ( "fmt" "os" "path/filepath" - "regexp" "strings" "azureaiagent/internal/exterrors" @@ -18,20 +17,6 @@ import ( "gopkg.in/yaml.v3" ) -// azureYamlEnvRefPattern matches bare ${VAR} references and references with a -// fallback. Group 2 is non-empty for ${VAR:-default}, which does not require an -// 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 -) - // These types mirror only the fields each Foundry provider expands from the // azd environment. The owning provider types are unexported or live in sibling // extension modules, so init keeps small typed views instead of string paths. @@ -363,7 +348,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( variable.Value, false, - honorAzureYamlEnvironmentEscaping, + honorEnvironmentEscaping, references, indexByName, ) @@ -376,7 +361,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Target, false, - honorAzureYamlEnvironmentEscaping, + honorEnvironmentEscaping, references, indexByName, ) @@ -406,7 +391,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.AgentSubnet.VNet, false, - ignoreAzureYamlEnvironmentEscaping, + ignoreEnvironmentEscaping, references, indexByName, ) @@ -415,7 +400,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.PESubnet.VNet, false, - ignoreAzureYamlEnvironmentEscaping, + ignoreEnvironmentEscaping, references, indexByName, ) @@ -424,7 +409,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.DNS.Subscription, false, - ignoreAzureYamlEnvironmentEscaping, + ignoreEnvironmentEscaping, references, indexByName, ) @@ -453,7 +438,7 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Endpoint, false, - honorAzureYamlEnvironmentEscaping, + honorEnvironmentEscaping, references, indexByName, ) @@ -516,7 +501,7 @@ func collectAzureYamlEnvironmentReferencesFromNode( collectAzureYamlEnvironmentReferences( node.Value, secret, - honorAzureYamlEnvironmentEscaping, + honorEnvironmentEscaping, references, indexByName, ) @@ -530,86 +515,28 @@ func collectAzureYamlEnvironmentReferences( references *[]azureYamlEnvironmentReference, indexByName map[string]int, ) { - matches := azureYamlEnvRefPattern.FindAllStringSubmatchIndex(value, -1) - protected := protectedAzureYamlEnvironmentReferenceOccurrences(value, matches, honorEscaping) - for i, match := range matches { - if honorEscaping && isEscapedAzureYamlEnvironmentReference(value, match[0]) { - continue - } - if protected[i] { - continue - } - if match[4] != -1 { + for _, reference := range findEnvironmentReferences(value, honorEscaping) { + // A ${VAR:-default} needs no environment value because the + // runtime expander supplies the fallback. + if reference.HasDefault { continue } - name := value[match[2]:match[3]] - if index, ok := indexByName[name]; ok { + if index, ok := indexByName[reference.Name]; ok { if secret { (*references)[index].Secret = true } continue } - indexByName[name] = len(*references) + indexByName[reference.Name] = len(*references) *references = append(*references, azureYamlEnvironmentReference{ - Name: name, + Name: reference.Name, Secret: secret, }) } } -// protectedAzureYamlEnvironmentReferenceOccurrences reports which candidate -// references are inside server-side ${{...}} spans. Each candidate is replaced -// with a unique probe before running [foundry.ExpandEnv]; probes left verbatim -// are protected by the shared expander. This keeps discovery linked to the -// owning implementation without ambiguous name-based occurrence counting. -func protectedAzureYamlEnvironmentReferenceOccurrences( - value string, - matches [][]int, - honorEscaping bool, -) []bool { - protected := make([]bool, len(matches)) - if !honorEscaping || len(matches) == 0 { - return protected - } - - probePrefix := "AZD_ENV_REFERENCE_PROBE_" - for strings.Contains(value, probePrefix) { - probePrefix += "_" - } - - probeRefs := make([]string, len(matches)) - var probed strings.Builder - last := 0 - for i, match := range matches { - probed.WriteString(value[last:match[0]]) - probeRefs[i] = fmt.Sprintf("${%s%d}", probePrefix, i) - probed.WriteString(probeRefs[i]) - last = match[1] - } - 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 -} - -func isEscapedAzureYamlEnvironmentReference(value string, start int) bool { - precedingDollars := 0 - for i := start - 1; i >= 0 && value[i] == '$'; i-- { - precedingDollars++ - } - return precedingDollars%2 == 1 -} - func isSecretAzureYamlEnvironmentKey(key string) bool { switch strings.ToLower(key) { case "credential", "credentials", "secret", "secrets": diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go index 466c42a1317..04480d2db45 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go @@ -831,6 +831,7 @@ func (a *InitFromCodeAction) addToProject( if err != nil { return err } + agentEnvironment := project.AgentEnvironment(*definition) language := "python" if !isCodeDeploy { @@ -840,8 +841,9 @@ func (a *InitFromCodeAction) addToProject( language = "csharp" } + agentServiceName := strings.ReplaceAll(agentName, " ", "") serviceConfig := &azdext.ServiceConfig{ - Name: strings.ReplaceAll(agentName, " ", ""), + Name: agentServiceName, RelativePath: targetDir, Host: AiAgentHost, Language: language, @@ -868,11 +870,18 @@ func (a *InitFromCodeAction) addToProject( if _, err := a.azdClient.Project().AddService(ctx, req); err != nil { return fmt.Errorf("adding agent service to project: %w", err) } + if err := setServiceEnvironment( + ctx, + a.azdClient, + agentServiceName, + agentEnvironment, + ); err != nil { + return err + } // Emit the sibling azure.ai.project service carrying the model deployments // and wire the agent's uses: to it. A selected existing project contributes // its endpoint so provision reuses it instead of creating a new project. - agentServiceName := strings.ReplaceAll(agentName, " ", "") if err := emitResourceServices( ctx, a.azdClient, agentServiceName, projectNameHint(ctx, a.azdClient, a.environment.Name, a.selectedFoundryProject), diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_test.go index 4d4741eb750..bfb248c1813 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_test.go @@ -447,6 +447,9 @@ func TestAddToProjectPreBuiltImageWritesServiceImage(t *testing.T) { Protocols: []agent_yaml.ProtocolVersionRecord{ {Protocol: "responses", Version: "2.0.0"}, }, + EnvironmentVariables: &[]agent_yaml.EnvironmentVariable{ + {Name: "LOG_LEVEL", Value: "info"}, + }, }, } @@ -470,9 +473,16 @@ func TestAddToProjectPreBuiltImageWritesServiceImage(t *testing.T) { require.Equal(t, "docker", agentService.GetLanguage()) require.NotNil(t, agentService.GetDocker()) require.NotNil(t, agentService.GetAdditionalProperties()) + require.Empty(t, agentService.GetEnvironment()) + require.Equal(t, map[string]any{ + "LOG_LEVEL": "info", + }, server.env["my-agent"]) _, hasInlineImage := agentService.GetAdditionalProperties().GetFields()["image"] require.False(t, hasInlineImage, "pre-built image must ride on the top-level service image field") + _, hasInlineEnvironment := agentService.GetAdditionalProperties(). + GetFields()["environmentVariables"] + require.False(t, hasInlineEnvironment) } func TestValidateInitAgentName(t *testing.T) { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_apply.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_apply.go index 92b34000836..cdd2ee2ccab 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_apply.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_apply.go @@ -17,6 +17,7 @@ import ( "encoding/json" "fmt" "io" + "maps" "os" "path/filepath" "strings" @@ -24,6 +25,7 @@ import ( "azureaiagent/internal/pkg/agents/opt_eval" "azureaiagent/internal/pkg/agents/optimize_api" "azureaiagent/internal/pkg/paths" + "azureaiagent/internal/pkg/projectconfig" projectpkg "azureaiagent/internal/project" "github.com/azure/azure-dev/cli/azd/pkg/azdext" @@ -196,33 +198,13 @@ func (a *OptimizeApplyAction) apply( return fmt.Errorf("failed to read agent definition: %w", err) } else if found { fmt.Fprintf(out, " Updating agent definition in azure.yaml...\n") - if err := projectpkg.UpsertAgentEnvVars(svc, envUpdates); err != nil { - return fmt.Errorf("failed to update agent definition: %w", err) - } - // Read the current `uses:` value before replacing the whole service entry. - // AddService writes back the proto ServiceConfig shape, which doesn't carry - // the `uses:` field (a core azd-only field). Reading it first and restoring - // it after avoids silently dropping dependency edges that were written by - // `setServiceUses` via SetServiceConfigValue. - prevUses, err := azdClient.Project().GetServiceConfigValue(ctx, &azdext.GetServiceConfigValueRequest{ - ServiceName: svc.Name, - Path: "uses", - }) - if err != nil { - return fmt.Errorf("failed to read uses for service %q: %w", svc.Name, err) - } - if _, err := azdClient.Project().AddService(ctx, &azdext.AddServiceRequest{Service: svc}); err != nil { - return fmt.Errorf("failed to persist agent definition: %w", err) - } - // Restore `uses:` if it was set before the replacement. - if prevUses.GetFound() && prevUses.GetValue() != nil { - if _, err := azdClient.Project().SetServiceConfigValue(ctx, &azdext.SetServiceConfigValueRequest{ - ServiceName: svc.Name, - Path: "uses", - Value: prevUses.GetValue(), - }); err != nil { - return fmt.Errorf("failed to restore uses for service %q: %w", svc.Name, err) - } + if err := persistInlineAgentEnvironment( + ctx, + azdClient, + svc, + envUpdates, + ); err != nil { + return err } } else { agentYamlPath := filepath.Join(serviceDir, "agent.yaml") @@ -272,6 +254,129 @@ func (a *OptimizeApplyAction) apply( return nil } +func persistInlineAgentEnvironment( + ctx context.Context, + azdClient *azdext.AzdClient, + svc *azdext.ServiceConfig, + envUpdates map[string]string, +) error { + _, _, found, source, err := projectpkg.AgentDefinitionFromService(svc) + if err != nil { + return fmt.Errorf("failed to read agent definition: %w", err) + } + if !found { + return fmt.Errorf( + "service %q does not carry an inline agent definition", + svc.GetName(), + ) + } + + // Build the env to persist from raw templates only, never from the + // core-expanded svc.Environment. AddService escapes env values to + // literals, so routing templates through it would freeze a ${VAR} + // template (or snapshot an already-expanded value) into azure.yaml. + legacyEnv, err := projectpkg.InlineAgentEnvironmentVariables(svc) + if err != nil { + return fmt.Errorf("failed to read agent environment: %w", err) + } + existingEnv, err := getRawServiceEnv(ctx, azdClient, svc) + if err != nil { + return err + } + // Merge order mirrors deploy-time precedence in toContainerAgent: + // the top-level env overlays legacy environmentVariables, then the + // new updates win. + mergedEnv := map[string]string{} + maps.Copy(mergedEnv, legacyEnv) + maps.Copy(mergedEnv, existingEnv) + maps.Copy(mergedEnv, envUpdates) + + if err := setServiceEnvironment(ctx, azdClient, svc.Name, mergedEnv); err != nil { + return err + } + + environmentPath := "environmentVariables" + if source == projectpkg.AgentDefinitionSourceLegacyConfig { + environmentPath = "config.environmentVariables" + } + if _, err := azdClient.Project().UnsetServiceConfig( + ctx, + &azdext.UnsetServiceConfigRequest{ + ServiceName: svc.Name, + Path: environmentPath, + }, + ); err != nil { + return fmt.Errorf( + "removing deprecated environmentVariables from service %q: %w", + svc.Name, + err, + ) + } + return nil +} + +// getRawServiceEnv reads the service's existing env section as raw, +// unexpanded templates. GetServiceConfigValue returns the on-disk +// config, so callers can rewrite env without losing ${VAR}/$${{...}} +// templates. +func getRawServiceEnv( + ctx context.Context, + azdClient *azdext.AzdClient, + svc *azdext.ServiceConfig, +) (map[string]string, error) { + serviceName := svc.GetName() + resp, err := azdClient.Project().GetServiceConfigValue( + ctx, + &azdext.GetServiceConfigValueRequest{ + ServiceName: serviceName, + Path: "env", + }, + ) + if err != nil { + return nil, fmt.Errorf( + "failed to read env for service %q: %w", serviceName, err) + } + if !resp.GetFound() || resp.GetValue() == nil { + return nil, nil + } + raw, ok := resp.GetValue().AsInterface().(map[string]any) + if !ok { + return nil, nil + } + originalRaw := maps.Clone(raw) + properties := map[string]any{"env": raw} + if err := projectconfig.NormalizeEnvironment(properties); err != nil { + return nil, fmt.Errorf( + "normalizing env for service %q: %w", + serviceName, + err, + ) + } + env := make(map[string]string, len(raw)) + for key, value := range raw { + if original, wasString := originalRaw[key].(string); wasString { + env[key] = original + continue + } + if forwarded, found := svc.GetEnvironment()[key]; found { + env[key] = forwarded + continue + } + str, ok := value.(string) + if ok { + env[key] = str + continue + } + return nil, fmt.Errorf( + "normalizing env %q for service %q produced %T", + key, + serviceName, + value, + ) + } + return env, nil +} + // agentConfigMetadata is the YAML structure written as metadata.yaml in each // agent config version directory (baseline or candidate). // diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_apply_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_apply_test.go index f84f87e372e..c03ab66de2d 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_apply_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/optimize_apply_test.go @@ -11,13 +11,16 @@ import ( "path/filepath" "testing" + "azureaiagent/internal/pkg/agents/agent_yaml" "azureaiagent/internal/pkg/agents/opt_eval" "azureaiagent/internal/pkg/agents/optimize_api" + projectpkg "azureaiagent/internal/project" "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/fatih/color" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/structpb" ) // ---- newOptimizeApplyCommand — command shape ---- @@ -47,6 +50,270 @@ func TestNewOptimizeApplyCommand_CandidateIsRequired(t *testing.T) { assert.Contains(t, err.Error(), "candidate") } +func TestPersistInlineAgentEnvironmentMigratesLegacyTemplates(t *testing.T) { + props, err := projectpkg.AgentDefinitionToServiceProperties( + agent_yaml.ContainerAgent{ + AgentDefinition: agent_yaml.AgentDefinition{ + Kind: agent_yaml.AgentKindHosted, + Name: "basic-agent", + }, + Protocols: []agent_yaml.ProtocolVersionRecord{ + {Protocol: "responses", Version: "2.0.0"}, + }, + }, + nil, + ) + require.NoError(t, err) + legacyEnvironment, err := structpb.NewValue([]any{ + map[string]any{ + "name": "LEGACY_KEY", + "value": "${LEGACY_KEY}", + }, + }) + require.NoError(t, err) + props.Fields["environmentVariables"] = legacyEnvironment + svc := &azdext.ServiceConfig{ + Name: "basic-agent", + Host: AiAgentHost, + Config: props, + } + + server := &recordingProjectServer{} + client := newProjectRecorderClient(t, server) + require.NoError(t, persistInlineAgentEnvironment( + t.Context(), + client, + svc, + map[string]string{"OPTIMIZATION_CANDIDATE_ID": "candidate-1"}, + )) + + server.mu.Lock() + defer server.mu.Unlock() + require.Empty(t, server.added) + require.Equal( + t, + []string{"config.environmentVariables"}, + server.unsetPaths, + ) + require.Equal(t, map[string]any{ + "LEGACY_KEY": "${LEGACY_KEY}", + "OPTIMIZATION_CANDIDATE_ID": "candidate-1", + }, server.env["basic-agent"]) +} + +// TestPersistInlineAgentEnvironmentPreservesTopLevelEnv verifies a +// modern agent's top-level env templates survive the OPTIMIZATION_* +// update: they are read raw and rewritten via the env section, not +// snapshotted to expanded literals through AddService. +func TestPersistInlineAgentEnvironmentPreservesTopLevelEnv(t *testing.T) { + props, err := projectpkg.AgentDefinitionToServiceProperties( + agent_yaml.ContainerAgent{ + AgentDefinition: agent_yaml.AgentDefinition{ + Kind: agent_yaml.AgentKindHosted, + Name: "basic-agent", + }, + Protocols: []agent_yaml.ProtocolVersionRecord{ + {Protocol: "responses", Version: "2.0.0"}, + }, + }, + nil, + ) + require.NoError(t, err) + svc := &azdext.ServiceConfig{ + Name: "basic-agent", + Host: AiAgentHost, + AdditionalProperties: props, + // Core forwards expanded values; the raw templates live on disk. + Environment: map[string]string{ + "LOG_LEVEL": "debug", + "MODEL_ENDPOINT": "https://resolved.example", + }, + } + + server := &recordingProjectServer{ + rawEnv: map[string]map[string]any{ + "basic-agent": { + "LOG_LEVEL": "${AZURE_LOG_LEVEL}", + "MODEL_ENDPOINT": "$${{project.endpoint}}", + }, + }, + } + client := newProjectRecorderClient(t, server) + require.NoError(t, persistInlineAgentEnvironment( + t.Context(), + client, + svc, + map[string]string{"OPTIMIZATION_CANDIDATE_ID": "candidate-1"}, + )) + + server.mu.Lock() + defer server.mu.Unlock() + require.Empty(t, server.added) + require.Equal( + t, + []string{"environmentVariables"}, + server.unsetPaths, + ) + require.Equal(t, map[string]any{ + "LOG_LEVEL": "${AZURE_LOG_LEVEL}", + "MODEL_ENDPOINT": "$${{project.endpoint}}", + "OPTIMIZATION_CANDIDATE_ID": "candidate-1", + }, server.env["basic-agent"]) +} + +// TestPersistInlineAgentEnvironmentEscapesLegacyFoundrySpan verifies +// a legacy environmentVariables value carrying a raw Foundry ${{...}} +// span is escaped to $${{...}} when migrated into the env section. +func TestPersistInlineAgentEnvironmentEscapesLegacyFoundrySpan(t *testing.T) { + props, err := projectpkg.AgentDefinitionToServiceProperties( + agent_yaml.ContainerAgent{ + AgentDefinition: agent_yaml.AgentDefinition{ + Kind: agent_yaml.AgentKindHosted, + Name: "basic-agent", + }, + Protocols: []agent_yaml.ProtocolVersionRecord{ + {Protocol: "responses", Version: "2.0.0"}, + }, + }, + nil, + ) + require.NoError(t, err) + legacyEnvironment, err := structpb.NewValue([]any{ + map[string]any{ + "name": "SEARCH_KEY", + "value": "${{connections.search.credentials.key}}", + }, + }) + require.NoError(t, err) + props.Fields["environmentVariables"] = legacyEnvironment + svc := &azdext.ServiceConfig{ + Name: "basic-agent", + Host: AiAgentHost, + AdditionalProperties: props, + } + + server := &recordingProjectServer{} + client := newProjectRecorderClient(t, server) + require.NoError(t, persistInlineAgentEnvironment( + t.Context(), + client, + svc, + map[string]string{"OPTIMIZATION_CANDIDATE_ID": "candidate-1"}, + )) + + server.mu.Lock() + defer server.mu.Unlock() + require.Equal( + t, + []string{"environmentVariables"}, + server.unsetPaths, + ) + require.Equal(t, map[string]any{ + "SEARCH_KEY": "$${{connections.search.credentials.key}}", + "OPTIMIZATION_CANDIDATE_ID": "candidate-1", + }, server.env["basic-agent"]) +} + +func TestPersistInlineAgentEnvironmentNormalizesScalars(t *testing.T) { + props, err := projectpkg.AgentDefinitionToServiceProperties( + agent_yaml.ContainerAgent{ + AgentDefinition: agent_yaml.AgentDefinition{ + Kind: agent_yaml.AgentKindHosted, + Name: "basic-agent", + }, + Protocols: []agent_yaml.ProtocolVersionRecord{ + {Protocol: "responses", Version: "2.0.0"}, + }, + }, + nil, + ) + require.NoError(t, err) + svc := &azdext.ServiceConfig{ + Name: "basic-agent", + Host: AiAgentHost, + AdditionalProperties: props, + Environment: map[string]string{ + "LARGE": "9007199254740993", + }, + } + server := &recordingProjectServer{ + rawEnv: map[string]map[string]any{ + "basic-agent": { + "ENABLED": true, + "RETRIES": float64(3), + "EMPTY": nil, + "LARGE": float64(9007199254740992), + }, + }, + } + + client := newProjectRecorderClient(t, server) + require.NoError(t, persistInlineAgentEnvironment( + t.Context(), + client, + svc, + map[string]string{"OPTIMIZATION_CANDIDATE_ID": "candidate-1"}, + )) + + server.mu.Lock() + defer server.mu.Unlock() + require.Equal(t, map[string]any{ + "ENABLED": "true", + "RETRIES": "3", + "EMPTY": "", + "LARGE": "9007199254740993", + "OPTIMIZATION_CANDIDATE_ID": "candidate-1", + }, server.env["basic-agent"]) +} + +func TestPersistInlineAgentEnvironmentKeepsLegacyOnEnvFailure( + t *testing.T, +) { + props, err := projectpkg.AgentDefinitionToServiceProperties( + agent_yaml.ContainerAgent{ + AgentDefinition: agent_yaml.AgentDefinition{ + Kind: agent_yaml.AgentKindHosted, + Name: "basic-agent", + }, + Protocols: []agent_yaml.ProtocolVersionRecord{ + {Protocol: "responses", Version: "2.0.0"}, + }, + }, + nil, + ) + require.NoError(t, err) + legacyEnvironment, err := structpb.NewValue([]any{ + map[string]any{ + "name": "LEGACY_KEY", + "value": "${LEGACY_KEY}", + }, + }) + require.NoError(t, err) + props.Fields["environmentVariables"] = legacyEnvironment + svc := &azdext.ServiceConfig{ + Name: "basic-agent", + Host: AiAgentHost, + AdditionalProperties: props, + } + server := &recordingProjectServer{ + setEnvironmentErr: fmt.Errorf("write failed"), + } + + client := newProjectRecorderClient(t, server) + err = persistInlineAgentEnvironment( + t.Context(), + client, + svc, + map[string]string{"OPTIMIZATION_CANDIDATE_ID": "candidate-1"}, + ) + + require.ErrorContains(t, err, "write failed") + server.mu.Lock() + defer server.mu.Unlock() + require.Empty(t, server.unsetPaths) + require.Empty(t, server.added) +} + // ---- printPreviewLines ---- func TestPrintPreviewLines(t *testing.T) { 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 0931fbd76fb..524a10d5966 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 @@ -277,6 +277,7 @@ func addResourceService( cfg *structpb.Struct, uses []string, ) error { + environment := serviceEnvironmentTemplates(cfg) svc := &azdext.ServiceConfig{ Name: name, Host: host, @@ -287,6 +288,15 @@ func addResourceService( return fmt.Errorf("adding %s service %q: %w", host, name, err) } + if err := setServiceEnvironment( + ctx, + azdClient, + name, + environment, + ); err != nil { + return err + } + if len(uses) > 0 { if err := setServiceUses(ctx, azdClient, name, uses); err != nil { return err @@ -296,6 +306,122 @@ func addResourceService( return nil } +// serviceEnvironmentTemplates discovers client-side templates in the +// generic nested resource config emitted to azure.yaml. +func serviceEnvironmentTemplates(cfg *structpb.Struct) map[string]string { + if cfg == nil { + return nil + } + + environment := map[string]string{} + collectEnvironmentTemplates(cfg.AsMap(), environment) + if len(environment) == 0 { + return nil + } + return environment +} + +func collectEnvironmentTemplates(value any, environment map[string]string) { + switch typed := value.(type) { + case string: + collectStringEnvironmentTemplates(typed, environment) + case map[string]any: + for _, nested := range typed { + collectEnvironmentTemplates(nested, environment) + } + case []any: + for _, nested := range typed { + collectEnvironmentTemplates(nested, environment) + } + } +} + +func collectStringEnvironmentTemplates(value string, environment map[string]string) { + for _, reference := range findEnvironmentReferences(value, honorEnvironmentEscaping) { + // 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 + // only needs NAME's resolved base value. Collapsing every form of + // a var to one value also keeps collection deterministic when the + // same var appears with and without a default. This assumes a + // literal default: a nested ${VAR} default is unsupported and + // gets no entry here. See findEnvironmentReferences. + environment[reference.Name] = "${" + reference.Name + "}" + } +} + +// escapeFoundryTemplates escapes Foundry ${{...}} spans as $${{...}} +// so azd core's envsubst emits a literal ${{...}} for the owning +// extension to resolve. Already-escaped $${{...}} and bare ${VAR} +// are left unchanged, so it is safe on values read back from disk. +func escapeFoundryTemplates(value string) string { + if !strings.Contains(value, "${{") { + return value + } + var b strings.Builder + b.Grow(len(value) + 2) + for i := 0; i < len(value); i++ { + if value[i] == '$' && strings.HasPrefix(value[i:], "${{") && + (i == 0 || value[i-1] != '$') { + b.WriteByte('$') + } + b.WriteByte(value[i]) + } + return b.String() +} + +// setServiceEnvironment writes the env: block of a service, and +// leaves azure.yaml untouched when there is nothing to write. +// +// A generated service with no variables of its own therefore reads +// as legacy at run and deploy. Declaring an explicit env: {} here +// would not change that today: core drops a zero-length env on +// save because ServiceConfig.Environment is tagged omitempty. +// Fixing it needs core to distinguish an absent env: from an +// explicitly empty one. +func setServiceEnvironment( + ctx context.Context, + azdClient *azdext.AzdClient, + serviceName string, + environment map[string]string, +) error { + if len(environment) == 0 { + return nil + } + + sectionValues := make(map[string]any, len(environment)) + for key, value := range environment { + sectionValues[key] = escapeFoundryTemplates(value) + } + section, err := structpb.NewStruct(sectionValues) + if err != nil { + return fmt.Errorf( + "encoding env for service %q: %w", + serviceName, + err, + ) + } + + // ServiceConfig.Environment only carries expanded values. + // The config RPC preserves raw ${VAR} templates. + _, err = azdClient.Project().SetServiceConfigSection( + ctx, + &azdext.SetServiceConfigSectionRequest{ + ServiceName: serviceName, + Path: "env", + Section: section, + }, + ) + if err != nil { + return fmt.Errorf( + "setting env for service %q: %w", + serviceName, + err, + ) + } + return nil +} + // setServiceUses sets the uses: list on an existing service. uses is a real // core ServiceConfig field, so it is written via SetServiceConfigValue (a raw // map path) rather than AddService's inlined config map, which cannot carry it. diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services_test.go index d5851a0d38c..e01c46c4801 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services_test.go @@ -91,6 +91,108 @@ func TestReserveServiceName(t *testing.T) { assert.Contains(t, err.Error(), "agent service") } +func TestServiceEnvironmentTemplates(t *testing.T) { + t.Parallel() + + cfg, err := project.MarshalStruct(&project.Connection{ + Credentials: map[string]any{ + "key": "${SEARCH_KEY}", + }, + Metadata: map[string]string{ + "server": "${SERVER_NAME}", + "default": "${DEFAULT_NAME:-fallback}", + "foundry_default": "${EVENT_BODY:-${{event.body}}}", + "token": "${{connections.search.credentials.key}}", + "literal": "$${LITERAL}", + }, + }) + require.NoError(t, err) + + assert.Equal(t, map[string]string{ + "SEARCH_KEY": "${SEARCH_KEY}", + "SERVER_NAME": "${SERVER_NAME}", + "DEFAULT_NAME": "${DEFAULT_NAME}", + "EVENT_BODY": "${EVENT_BODY}", + }, serviceEnvironmentTemplates(cfg)) +} + +// TestServiceEnvironmentTemplatesDeterministic verifies a var +// referenced both bare and with a default collapses to the same +// canonical ${VAR}, so field order cannot change the result. +func TestServiceEnvironmentTemplatesDeterministic(t *testing.T) { + t.Parallel() + + cfg, err := project.MarshalStruct(&project.Connection{ + Metadata: map[string]string{ + "bare": "${TOPIC}", + "default": "${TOPIC:-general}", + "default2": "${TOPIC:-other}", + }, + }) + require.NoError(t, err) + + assert.Equal(t, map[string]string{ + "TOPIC": "${TOPIC}", + }, serviceEnvironmentTemplates(cfg)) +} + +func TestEscapeFoundryTemplates(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + value string + want string + }{ + {"foundry span", "${{event.body}}", "$${{event.body}}"}, + {"already escaped", "$${{event.body}}", "$${{event.body}}"}, + {"single brace untouched", "${VAR}", "${VAR}"}, + {"literal untouched", "info", "info"}, + { + "embedded span", + "prefix-${{connections.x.key}}-suffix", + "prefix-$${{connections.x.key}}-suffix", + }, + { + "span in default", + "${MISSING:-${{event.body}}}", + "${MISSING:-$${{event.body}}}", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, escapeFoundryTemplates(tt.value)) + }) + } +} + +func TestAddResourceServiceWritesEnvironment(t *testing.T) { + server := &recordingProjectServer{} + client := newProjectRecorderClient(t, server) + cfg, err := project.MarshalStruct(&project.Connection{ + Credentials: map[string]any{"key": "${SEARCH_KEY}"}, + }) + require.NoError(t, err) + + require.NoError(t, addResourceService( + t.Context(), + client, + "search", + AiConnectionHost, + cfg, + nil, + )) + + server.mu.Lock() + defer server.mu.Unlock() + require.Len(t, server.added, 1) + assert.Empty(t, server.added[0].GetEnvironment()) + assert.Equal(t, map[string]any{ + "SEARCH_KEY": "${SEARCH_KEY}", + }, server.env["search"]) +} + func TestCollectLegacyProjectDeploymentsIgnoresSplitProject( t *testing.T, ) { @@ -314,11 +416,19 @@ type recordingProjectServer struct { mu sync.Mutex added []*azdext.ServiceConfig uses map[string][]string + env map[string]map[string]any // configValues records non-"uses" SetServiceConfigValue calls keyed by path. configValues map[string]configValueRecord // existing is returned by Get to simulate services already present in the // project (e.g. a prior init's azure.ai.project service). existing map[string]*azdext.ServiceConfig + // rawEnv is returned by GetServiceConfigValue for path "env" to + // simulate a service that already carries an env section (raw, + // on-disk templates). + rawEnv map[string]map[string]any + unsetPaths []string + setEnvironmentErr error + unsetServiceConfigErr error } // configValueRecord captures a single SetServiceConfigValue call. @@ -346,6 +456,26 @@ func (s *recordingProjectServer) AddService( return &azdext.EmptyResponse{}, nil } +func (s *recordingProjectServer) GetServiceConfigValue( + _ context.Context, req *azdext.GetServiceConfigValueRequest, +) (*azdext.GetServiceConfigValueResponse, error) { + s.mu.Lock() + defer s.mu.Unlock() + if req.Path == "env" { + if raw, ok := s.rawEnv[req.ServiceName]; ok { + value, err := structpb.NewValue(raw) + if err != nil { + return nil, err + } + return &azdext.GetServiceConfigValueResponse{ + Found: true, + Value: value, + }, nil + } + } + return &azdext.GetServiceConfigValueResponse{}, nil +} + func (s *recordingProjectServer) SetServiceConfigValue( _ context.Context, req *azdext.SetServiceConfigValueRequest, ) (*azdext.EmptyResponse, error) { @@ -378,9 +508,43 @@ func (s *recordingProjectServer) SetServiceConfigValue( return &azdext.EmptyResponse{}, nil } +func (s *recordingProjectServer) SetServiceConfigSection( + _ context.Context, + req *azdext.SetServiceConfigSectionRequest, +) (*azdext.EmptyResponse, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.setEnvironmentErr != nil { + return nil, s.setEnvironmentErr + } + if s.env == nil { + s.env = map[string]map[string]any{} + } + if req.Path == "env" && req.Section != nil { + s.env[req.ServiceName] = req.Section.AsMap() + } + return &azdext.EmptyResponse{}, nil +} + +func (s *recordingProjectServer) UnsetServiceConfig( + _ context.Context, + req *azdext.UnsetServiceConfigRequest, +) (*azdext.EmptyResponse, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.unsetPaths = append(s.unsetPaths, req.Path) + if s.unsetServiceConfigErr != nil { + return nil, s.unsetServiceConfigErr + } + return &azdext.EmptyResponse{}, nil +} + // newProjectRecorderClient spins up an in-process gRPC server backed by the // supplied project server stub and returns a client wired to its address. -func newProjectRecorderClient(t *testing.T, server azdext.ProjectServiceServer) *azdext.AzdClient { +func newProjectRecorderClient( + t *testing.T, + server azdext.ProjectServiceServer, +) *azdext.AzdClient { t.Helper() grpcServer := grpc.NewServer() diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/run.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/run.go index 81ca1988fa4..93184537e82 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/run.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/run.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "log" + "maps" "net" "net/http" "os" @@ -47,11 +48,6 @@ type runFlags struct { channel string } -type environmentEntry struct { - key string - value string -} - func newRunCommand(extCtx *azdext.ExtensionContext) *cobra.Command { flags := &runFlags{} extCtx = ensureExtensionContext(extCtx) @@ -191,59 +187,44 @@ func runRun(ctx context.Context, flags *runFlags, noPrompt bool) error { cmdParts = resolveVenvCommand(projectDir, cmdParts) - env := os.Environ() - env = appendPortEnvVars(env, pt, flags.port) + env := appendPortEnvVars(os.Environ(), pt, flags.port) - // Load azd environment variables (e.g., FOUNDRY_PROJECT_ENDPOINT) - // so the agent can reach Azure services during local development. - // Also translate azd env keys to FOUNDRY_* env vars so the agent code - // works identically whether running locally or in a hosted container - // (where the platform automatically injects FOUNDRY_* env vars). + // Load azd values as template inputs and legacy fallback values. var azdEnvVars map[string]string if loaded, err := loadAzdEnvironment(ctx, azdClient); err == nil { azdEnvVars = loaded - for k, v := range azdEnvVars { - env = append(env, fmt.Sprintf("%s=%s", k, v)) - } - env = appendFoundryEnvVars(env, azdEnvVars, runCtx.ServiceName) } else if shouldWarnLoadAzdEnvironmentFailure(err) { fmt.Fprintf(os.Stderr, "Warning: failed to load azd environment values: %s\n", err) } endpoint, _ := resolveAgentEndpoint(ctx, "", "") - defEnv, defErr := resolveAgentDefinitionEnvVars(ctx, runCtx.Definition, azdEnvVars, endpoint) - if defErr != nil { - fmt.Fprintf(os.Stderr, "Warning: %s\n", defErr) - } - serviceEnv, serviceEnvErr := resolveServiceEnvironmentVars( + endpoint = localProjectEndpoint( + env, + runCtx.ServiceEnvironment, + endpoint, + ) + serviceEnvironment := resolveLocalServiceEnvironment( + runCtx.ServiceEnvironment, + endpoint, + ) + defEnv, defErr := resolveAgentDefinitionEnvVars( ctx, - runCtx.Environment, + runCtx.Definition, + serviceEnvironment, azdEnvVars, endpoint, ) - if serviceEnvErr != nil { - fmt.Fprintf(os.Stderr, "Warning: %s\n", serviceEnvErr) + if defErr != nil { + fmt.Fprintf(os.Stderr, "Warning: %s\n", defErr) } - configuredEnv := mergeConfiguredEnvironmentEntries( + env = mergeAgentRunEnvironment( + env, + azdEnvVars, + serviceEnvironment, defEnv, - serviceEnv, - runtime.GOOS == "windows", + runCtx.ServiceName, + runCtx.HasServiceEnvironment, ) - keys := make([]string, 0, len(configuredEnv)) - for key := range configuredEnv { - keys = append(keys, key) - } - slices.Sort(keys) - for _, key := range keys { - entry := configuredEnv[key] - if !envSliceHasKey(env, entry.key) { - env = append( - env, - fmt.Sprintf("%s=%s", entry.key, entry.value), - ) - } - - } // Activity agents bind IPv4 and are reached at 127.0.0.1 everywhere else // (the port-readiness check and the Playground URL), because `localhost` @@ -537,6 +518,7 @@ func shouldWarnLoadAzdEnvironmentFailure(err error) bool { func resolveAgentDefinitionEnvVars( ctx context.Context, agentDef *agent_yaml.ContainerAgent, + serviceEnvironment map[string]string, azdEnvVars map[string]string, endpoint string, ) ([]string, error) { @@ -567,8 +549,15 @@ func resolveAgentDefinitionEnvVars( if _, isConn := connRefEnvNames[ev.Name]; isConn { continue } - // ExpandEnv returns the original value on error, so a failed expansion is a no-op. - resolved, _ := project.ExpandEnv(ev.Value, lookup) + resolved, err := project.ResolveAgentEnvironmentVariable( + ev.Name, + ev.Value, + serviceEnvironment, + lookup, + ) + if err != nil { + resolved = ev.Value + } result = append(result, fmt.Sprintf("%s=%s", ev.Name, resolved)) } @@ -584,43 +573,39 @@ func resolveAgentDefinitionEnvVars( return result, nil } -func resolveServiceEnvironmentVars( - ctx context.Context, - values map[string]string, - azdEnvVars map[string]string, +func resolveLocalServiceEnvironment( + environment map[string]string, endpoint string, -) ([]string, error) { - if len(values) == 0 { - return nil, nil - } - keys := make([]string, 0, len(values)) - for key := range values { - keys = append(keys, key) - } - slices.Sort(keys) - envVars := make([]agent_yaml.EnvironmentVariable, 0, len(keys)) - for _, key := range keys { - value := values[key] - if endpoint != "" { - value = strings.ReplaceAll( - value, - "${{project.endpoint}}", - endpoint, - ) - } - envVars = append(envVars, agent_yaml.EnvironmentVariable{ - Name: key, - Value: value, - }) +) map[string]string { + resolved := maps.Clone(environment) + if endpoint == "" { + return resolved + } + for key, value := range resolved { + resolved[key] = strings.ReplaceAll( + value, + "${{project.endpoint}}", + endpoint, + ) } - return resolveAgentDefinitionEnvVars( - ctx, - &agent_yaml.ContainerAgent{ - EnvironmentVariables: &envVars, - }, - azdEnvVars, - endpoint, - ) + return resolved +} + +func localProjectEndpoint( + baseEnvironment []string, + serviceEnvironment map[string]string, + fallback string, +) string { + if value, found := envSliceValue( + baseEnvironment, + "FOUNDRY_PROJECT_ENDPOINT", + ); found { + return value + } + if value, found := serviceEnvironment["FOUNDRY_PROJECT_ENDPOINT"]; found { + return value + } + return fallback } // findAgentYaml locates the agent definition file in the given directory. @@ -1091,89 +1076,142 @@ func findSystemPython() (pythonInterpreter, error) { return firstCompatiblePython(pythonCandidates(), pythonVersion) } -// appendFoundryEnvVars translates azd environment keys to FOUNDRY_* env vars that hosted -// agent containers receive automatically from the platform. This ensures the agent code -// works identically whether running locally (via azd ai agent run) or in a hosted container. +// mergeAgentRunEnvironment builds the local agent environment. +// +// baseEnvironment contains process and command-owned values. +// azdEnvironment is the full active environment for legacy fallback. +// serviceEnvironment is core-expanded services..env. +// definitionEnvironment comes from legacy agent definitions. +// hasServiceEnvironment reports whether the service declares an +// env: block (even an empty one). +func mergeAgentRunEnvironment( + baseEnvironment []string, + azdEnvironment map[string]string, + serviceEnvironment map[string]string, + definitionEnvironment []string, + serviceName string, + hasServiceEnvironment bool, +) []string { + environment := slices.Clone(baseEnvironment) + + // The full azd environment is a compatibility fallback only. + if !hasServiceEnvironment { + for key, value := range azdEnvironment { + if !envSliceHasKey(baseEnvironment, key) { + environment = append( + environment, + fmt.Sprintf("%s=%s", key, value), + ) + } + } + } else { + for key, value := range serviceEnvironment { + if !envSliceHasKey(baseEnvironment, key) { + environment = append( + environment, + fmt.Sprintf("%s=%s", key, value), + ) + } + } + } + + environment = appendFoundryEnvVars( + environment, + azdEnvironment, + serviceName, + ) + + for _, entry := range definitionEnvironment { + key, _, _ := strings.Cut(entry, "=") + _, serviceScoped := serviceEnvironment[key] + if serviceScoped { + continue + } + if !envSliceHasKey(environment, key) { + environment = append(environment, entry) + } + } + + return environment +} + +// appendFoundryEnvVars adds values injected by hosted agents. // // The mapping is: // -// AZURE_AI_PROJECT_ID → FOUNDRY_PROJECT_ARM_ID -// AGENT_{SVC}_NAME → FOUNDRY_AGENT_NAME -// AGENT_{SVC}_VERSION → FOUNDRY_AGENT_VERSION -// APPLICATIONINSIGHTS_CONNECTION_STRING (unchanged — already matches platform name) +// FOUNDRY_PROJECT_ENDPOINT -> unchanged +// AZURE_AI_PROJECT_ID -> FOUNDRY_PROJECT_ARM_ID +// AGENT_{SVC}_NAME -> FOUNDRY_AGENT_NAME +// AGENT_{SVC}_VERSION -> FOUNDRY_AGENT_VERSION +// APPLICATIONINSIGHTS_CONNECTION_STRING -> unchanged func appendFoundryEnvVars(env []string, azdEnv map[string]string, serviceName string) []string { - // Static mappings from azd env key names to FOUNDRY_* env var names - staticMappings := []struct { - azdKey string - foundryKey string - }{ - {"AZURE_AI_PROJECT_ID", "FOUNDRY_PROJECT_ARM_ID"}, - } - - for _, m := range staticMappings { - if v := azdEnv[m.azdKey]; v != "" { - if _, exists := azdEnv[m.foundryKey]; !exists && !envSliceHasKey(env, m.foundryKey) { - env = append(env, fmt.Sprintf("%s=%s", m.foundryKey, v)) - } - } + env = appendEnvValue( + env, + "FOUNDRY_PROJECT_ENDPOINT", + azdEnv["FOUNDRY_PROJECT_ENDPOINT"], + ) + + projectArmID := azdEnv["FOUNDRY_PROJECT_ARM_ID"] + if projectArmID == "" { + projectArmID = azdEnv["AZURE_AI_PROJECT_ID"] } + env = appendEnvValue(env, "FOUNDRY_PROJECT_ARM_ID", projectArmID) - // Service-specific mappings (AGENT_{SVC}_NAME → FOUNDRY_AGENT_NAME, etc.) + agentName := "" + agentVersion := "" if serviceName != "" { serviceKey := toServiceKey(serviceName) - agentMappings := []struct { - azdKeyFmt string - foundryKey string - }{ - {"AGENT_%s_NAME", "FOUNDRY_AGENT_NAME"}, - {"AGENT_%s_VERSION", "FOUNDRY_AGENT_VERSION"}, - } - - for _, m := range agentMappings { - azdKey := fmt.Sprintf(m.azdKeyFmt, serviceKey) - if v := azdEnv[azdKey]; v != "" { - if _, exists := azdEnv[m.foundryKey]; !exists && !envSliceHasKey(env, m.foundryKey) { - env = append(env, fmt.Sprintf("%s=%s", m.foundryKey, v)) - } - } - } + agentName = azdEnv[fmt.Sprintf("AGENT_%s_NAME", serviceKey)] + agentVersion = azdEnv[fmt.Sprintf("AGENT_%s_VERSION", serviceKey)] } + if agentName == "" { + agentName = azdEnv["FOUNDRY_AGENT_NAME"] + } + if agentVersion == "" { + agentVersion = azdEnv["FOUNDRY_AGENT_VERSION"] + } + env = appendEnvValue(env, "FOUNDRY_AGENT_NAME", agentName) + env = appendEnvValue(env, "FOUNDRY_AGENT_VERSION", agentVersion) + + env = appendEnvValue( + env, + "APPLICATIONINSIGHTS_CONNECTION_STRING", + azdEnv["APPLICATIONINSIGHTS_CONNECTION_STRING"], + ) return env } -func mergeConfiguredEnvironmentEntries( - definitionEnv []string, - serviceEnv []string, - caseInsensitive bool, -) map[string]environmentEntry { - configuredEnv := map[string]environmentEntry{} - for _, entry := range append(definitionEnv, serviceEnv...) { - key, value, _ := strings.Cut(entry, "=") - lookupKey := key - if caseInsensitive { - lookupKey = strings.ToUpper(key) - } - configuredEnv[lookupKey] = environmentEntry{ - key: key, - value: value, - } +func appendEnvValue(env []string, key string, value string) []string { + if value == "" || envSliceHasKey(env, key) { + return env } - return configuredEnv + return append(env, fmt.Sprintf("%s=%s", key, value)) } // envSliceHasKey reports whether env contains an entry for key. func envSliceHasKey(env []string, key string) bool { - return slices.ContainsFunc(env, func(entry string) bool { - entryKey, _, found := strings.Cut(entry, "=") + _, found := envSliceValue(env, key) + return found +} + +func envSliceValue(env []string, key string) (string, bool) { + for _, entry := range env { + entryKey, value, found := strings.Cut(entry, "=") if !found { - return false + continue } if runtime.GOOS == "windows" { - return strings.EqualFold(entryKey, key) + if strings.EqualFold(entryKey, key) { + return value, true + } + continue } - return entryKey == key - }) + if entryKey == key { + return value, true + } + } + return "", false } // loadAzdEnvironment reads all key-value pairs from the current azd environment. diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/run_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/run_test.go index 883e3502a47..014f4855f5f 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/run_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/run_test.go @@ -439,14 +439,15 @@ func createVenv(t *testing.T, projectDir string) string { func TestAppendFoundryEnvVars(t *testing.T) { t.Parallel() - t.Run("does not map FOUNDRY_PROJECT_ENDPOINT to itself", func(t *testing.T) { + t.Run("forwards FOUNDRY_PROJECT_ENDPOINT", func(t *testing.T) { t.Parallel() azdEnv := map[string]string{ "FOUNDRY_PROJECT_ENDPOINT": "https://myaccount.services.ai.azure.com/api/projects/myproject", } env := appendFoundryEnvVars(nil, azdEnv, "") - if len(env) != 0 { - t.Errorf("expected no translated env vars, got %v", env) + expected := "FOUNDRY_PROJECT_ENDPOINT=https://myaccount.services.ai.azure.com/api/projects/myproject" + if !slices.Contains(env, expected) { + t.Errorf("expected %q in env, got %v", expected, env) } }) @@ -495,12 +496,12 @@ func TestAppendFoundryEnvVars(t *testing.T) { "AGENT_AGENT1_VERSION": "v1", } env := appendFoundryEnvVars(nil, azdEnv, "agent1") - if len(env) != 3 { - t.Errorf("expected 3 env vars, got %d: %v", len(env), env) + if len(env) != 4 { + t.Errorf("expected 4 env vars, got %d: %v", len(env), env) } }) - t.Run("skips foundry key when already set in azd env", func(t *testing.T) { + t.Run("prefers service-specific agent metadata", func(t *testing.T) { t.Parallel() azdEnv := map[string]string{ "FOUNDRY_PROJECT_ENDPOINT": "https://explicit.services.ai.azure.com", @@ -509,20 +510,26 @@ func TestAppendFoundryEnvVars(t *testing.T) { } env := appendFoundryEnvVars(nil, azdEnv, "my-svc") - // Neither FOUNDRY_PROJECT_ENDPOINT nor FOUNDRY_AGENT_NAME should be - // appended because they already exist in azdEnv (and were thus already - // added to the env slice by the caller's loop over azdEnv). - for _, entry := range env { - if strings.HasPrefix(entry, "FOUNDRY_PROJECT_ENDPOINT=") || - strings.HasPrefix(entry, "FOUNDRY_AGENT_NAME=") { - t.Errorf("should not translate when foundry key already in azdEnv, got %q", entry) - } + if !slices.Contains( + env, + "FOUNDRY_PROJECT_ENDPOINT=https://explicit.services.ai.azure.com", + ) { + t.Errorf("expected project endpoint in env, got %v", env) } + if !slices.Contains(env, "FOUNDRY_AGENT_NAME=my-agent") { + t.Errorf("expected service agent name in env, got %v", env) + } + }) - // AZURE_AI_PROJECT_ID has no explicit FOUNDRY_PROJECT_ARM_ID, so it should still be skipped - // (it's not in azdEnv either, so appendFoundryEnvVars skips it because the source key is empty) - if len(env) != 0 { - t.Errorf("expected no translated env vars, got %v", env) + t.Run("forwards application insights", func(t *testing.T) { + t.Parallel() + azdEnv := map[string]string{ + "APPLICATIONINSIGHTS_CONNECTION_STRING": "InstrumentationKey=test", + } + env := appendFoundryEnvVars(nil, azdEnv, "") + expected := "APPLICATIONINSIGHTS_CONNECTION_STRING=InstrumentationKey=test" + if !slices.Contains(env, expected) { + t.Errorf("expected %q in env, got %v", expected, env) } }) @@ -572,38 +579,231 @@ func TestAppendFoundryEnvVars(t *testing.T) { }) } -func TestEnvSliceHasKeyUsesPlatformCasing(t *testing.T) { +func TestMergeAgentRunEnvironment(t *testing.T) { t.Parallel() - env := []string{"Path=process-value"} - if !envSliceHasKey(env, "Path") { - t.Fatal("expected exact-case environment key to match") + value := func(environment []string, key string) (string, bool) { + t.Helper() + for i := len(environment) - 1; i >= 0; i-- { + name, entryValue, found := strings.Cut(environment[i], "=") + if found && name == key { + return entryValue, true + } + } + return "", false } - got := envSliceHasKey(env, "PATH") - want := runtime.GOOS == "windows" - if got != want { - t.Errorf("envSliceHasKey() = %t, want %t", got, want) - } + t.Run("service env wins without leaking azd values", func(t *testing.T) { + t.Parallel() + environment := mergeAgentRunEnvironment( + []string{"FOO=process", "PORT=8088"}, + map[string]string{ + "FOO": "global", + "BAR": "global", + "UNDECLARED_SECRET": "hidden", + "FOUNDRY_PROJECT_ENDPOINT": "https://project.example", + }, + map[string]string{ + "FOO": "service", + "BAR": "service", + "EMPTY": "", + "PORT": "9000", + "SERVICE_ONLY": "service-only", + }, + []string{ + "FOO=service", + "BAR=service", + "PORT=9000", + "LEGACY=declared", + }, + "agent", + true, + ) + + if got, _ := value(environment, "FOO"); got != "process" { + t.Errorf("expected process FOO, got %q", got) + } + if got, _ := value(environment, "BAR"); got != "service" { + t.Errorf("expected service BAR, got %q", got) + } + if got, _ := value(environment, "PORT"); got != "8088" { + t.Errorf("expected command PORT, got %q", got) + } + if _, found := value(environment, "UNDECLARED_SECRET"); found { + t.Errorf("did not expect undeclared azd value in %v", environment) + } + if got, _ := value(environment, "FOUNDRY_PROJECT_ENDPOINT"); got != + "https://project.example" { + t.Errorf("expected Foundry endpoint, got %q", got) + } + if got, _ := value(environment, "LEGACY"); got != "declared" { + t.Errorf("expected declared legacy value, got %q", got) + } + if got, _ := value(environment, "SERVICE_ONLY"); got != "service-only" { + t.Errorf("expected service-only value, got %q", got) + } + if got, found := value(environment, "EMPTY"); !found || got != "" { + t.Errorf("expected empty service value, got %q, found %v", got, found) + } + }) + + t.Run("explicit empty env stays isolated", func(t *testing.T) { + t.Parallel() + environment := mergeAgentRunEnvironment( + []string{"FOO=process"}, + map[string]string{"SECRET": "leaked", "OTHER": "leaked"}, + map[string]string{}, + nil, + "agent", + true, + ) + if _, found := value(environment, "SECRET"); found { + t.Errorf("did not expect azd value in isolated env %v", environment) + } + if got, _ := value(environment, "FOO"); got != "process" { + t.Errorf("expected process FOO, got %q", got) + } + }) + + t.Run("legacy service keeps azd fallback", func(t *testing.T) { + t.Parallel() + environment := mergeAgentRunEnvironment( + []string{"FOO=process"}, + map[string]string{ + "FOO": "global", + "BAR": "global", + "PROJECT_ID": "project", + }, + nil, + []string{"BAR=inline", "BAZ=inline"}, + "agent", + false, + ) + + if got, _ := value(environment, "FOO"); got != "process" { + t.Errorf("expected process FOO, got %q", got) + } + if got, _ := value(environment, "BAR"); got != "global" { + t.Errorf("expected global BAR, got %q", got) + } + if got, _ := value(environment, "PROJECT_ID"); got != "project" { + t.Errorf("expected legacy azd value, got %q", got) + } + if got, _ := value(environment, "BAZ"); got != "inline" { + t.Errorf("expected inline BAZ, got %q", got) + } + }) } -func TestMergeConfiguredEnvironmentEntriesUsesServicePrecedence(t *testing.T) { +// TestLegacyEnvVarsKeepProjectFallback pins the deliberate gap in +// the env: scope: an explicit env: {} stops the azd environment +// from reaching the child, but a ${VAR} the author wrote in the +// deprecated environment_variables block still resolves against +// it. Cutting that off would silently empty the value for a +// project mid-migration, so the fallback stays. Without this +// test ResolveAgentEnvironmentVariable reads as fully +// scope-aware, which it is not. +func TestLegacyEnvVarsKeepProjectFallback(t *testing.T) { t.Parallel() - entries := mergeConfiguredEnvironmentEntries( - []string{"PATH=definition-value"}, - []string{"Path=service-value"}, + azdEnvironment := map[string]string{ + "FOO": "project-wide", + "UNDECLARED_SECRET": "hidden", + } + serviceEnvironment := map[string]string{} + + definition := &agent_yaml.ContainerAgent{ + EnvironmentVariables: &[]agent_yaml.EnvironmentVariable{ + {Name: "TARGET", Value: "${FOO}"}, + }, + } + + definitionEnvironment, err := resolveAgentDefinitionEnvVars( + t.Context(), + definition, + serviceEnvironment, + azdEnvironment, + "", + ) + if err != nil { + t.Fatalf("resolveAgentDefinitionEnvVars: %v", err) + } + + environment := mergeAgentRunEnvironment( + []string{"PATH=/usr/bin"}, + azdEnvironment, + serviceEnvironment, + definitionEnvironment, + "agent", true, ) - if len(entries) != 1 { - t.Fatalf("expected one entry, got %v", entries) + if !slices.Contains(environment, "TARGET=project-wide") { + t.Errorf("expected legacy fallback to resolve, got %v", environment) } - if entries["PATH"].key != "Path" { - t.Errorf("key = %q, want %q", entries["PATH"].key, "Path") + for _, entry := range environment { + name, _, _ := strings.Cut(entry, "=") + if name == "FOO" || name == "UNDECLARED_SECRET" { + t.Errorf("did not expect %q in %v", name, environment) + } } - if entries["PATH"].value != "service-value" { - t.Errorf("value = %q, want %q", entries["PATH"].value, "service-value") +} + +func TestResolveLocalServiceEnvironment(t *testing.T) { + t.Parallel() + + original := map[string]string{ + "MODEL_ENDPOINT": "${{project.endpoint}}/models", + "LITERAL": "literal ${NOT_A_TEMPLATE}", + } + endpoint := localProjectEndpoint( + []string{"FOUNDRY_PROJECT_ENDPOINT=https://process.example"}, + map[string]string{ + "FOUNDRY_PROJECT_ENDPOINT": "https://service.example", + }, + "https://azd.example", + ) + resolved := resolveLocalServiceEnvironment( + original, + endpoint, + ) + + if got := resolved["MODEL_ENDPOINT"]; got != + "https://process.example/models" { + t.Errorf("expected resolved endpoint, got %q", got) + } + if got := resolved["LITERAL"]; got != "literal ${NOT_A_TEMPLATE}" { + t.Errorf("expected literal value, got %q", got) + } + if got := original["MODEL_ENDPOINT"]; got != + "${{project.endpoint}}/models" { + t.Errorf("expected original map to stay unchanged, got %q", got) + } + + serviceEndpoint := localProjectEndpoint( + nil, + map[string]string{ + "FOUNDRY_PROJECT_ENDPOINT": "https://service.example", + }, + "https://azd.example", + ) + if serviceEndpoint != "https://service.example" { + t.Errorf("expected service endpoint, got %q", serviceEndpoint) + } +} + +func TestEnvSliceHasKeyUsesPlatformCasing(t *testing.T) { + t.Parallel() + + env := []string{"Path=process-value"} + if !envSliceHasKey(env, "Path") { + t.Fatal("expected exact-case environment key to match") + } + + got := envSliceHasKey(env, "PATH") + want := runtime.GOOS == "windows" + if got != want { + t.Errorf("envSliceHasKey() = %t, want %t", got, want) } } @@ -929,7 +1129,7 @@ environment_variables: value: debug `) - result, err := resolveAgentDefinitionEnvVars(t.Context(), def, nil, "") + result, err := resolveAgentDefinitionEnvVars(t.Context(), def, nil, nil, "") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -953,7 +1153,7 @@ environment_variables: azdEnv := map[string]string{ "FOUNDRY_PROJECT_ENDPOINT": "https://example.azure.com", } - result, err := resolveAgentDefinitionEnvVars(t.Context(), def, azdEnv, "") + result, err := resolveAgentDefinitionEnvVars(t.Context(), def, nil, azdEnv, "") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -974,7 +1174,7 @@ environment_variables: value: hello `) - result, err := resolveAgentDefinitionEnvVars(t.Context(), def, nil, "") + result, err := resolveAgentDefinitionEnvVars(t.Context(), def, nil, nil, "") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -990,7 +1190,7 @@ environment_variables: }) t.Run("returns nil for nil definition", func(t *testing.T) { - result, err := resolveAgentDefinitionEnvVars(t.Context(), nil, nil, "") + result, err := resolveAgentDefinitionEnvVars(t.Context(), nil, nil, nil, "") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1002,7 +1202,7 @@ environment_variables: t.Run("returns nil for empty environment_variables", func(t *testing.T) { def := parse(t, "name: test-agent\n") - result, err := resolveAgentDefinitionEnvVars(t.Context(), def, nil, "") + result, err := resolveAgentDefinitionEnvVars(t.Context(), def, nil, nil, "") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1018,7 +1218,7 @@ environment_variables: value: ${DOES_NOT_EXIST} `) - result, err := resolveAgentDefinitionEnvVars(t.Context(), def, map[string]string{}, "") + result, err := resolveAgentDefinitionEnvVars(t.Context(), def, nil, map[string]string{}, "") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1026,35 +1226,28 @@ environment_variables: t.Errorf("expected MISSING_REF= (empty), got %v", result) } }) -} - -func TestResolveServiceEnvironmentVars(t *testing.T) { - t.Parallel() - result, err := resolveServiceEnvironmentVars( - t.Context(), - map[string]string{ - "ENDPOINT": "${FOUNDRY_PROJECT_ENDPOINT}/agents", - "PROJECT": "${{project.endpoint}}", - "STATIC": "value", - }, - map[string]string{ - "FOUNDRY_PROJECT_ENDPOINT": "https://example", - }, - "https://example/project", - ) + t.Run("keeps forwarded core values literal", func(t *testing.T) { + def := parse(t, `name: test-agent +environment_variables: + - name: FORWARDED_VALUE + value: ${FORWARDED_VALUE} +`) - if err != nil { - t.Fatalf("resolve service environment: %v", err) - } - want := []string{ - "ENDPOINT=https://example/agents", - "PROJECT=https://example/project", - "STATIC=value", - } - if !slices.Equal(want, result) { - t.Fatalf("expected %v, got %v", want, result) - } + result, err := resolveAgentDefinitionEnvVars( + t.Context(), + def, + map[string]string{"FORWARDED_VALUE": "literal ${NOT_A_TEMPLATE}"}, + map[string]string{"NOT_A_TEMPLATE": "expanded"}, + "", + ) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !slices.Contains(result, "FORWARDED_VALUE=literal ${NOT_A_TEMPLATE}") { + t.Errorf("expected literal forwarded value, got %v", result) + } + }) } func TestVenvPip(t *testing.T) { diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/agent_definition.go b/cli/azd/extensions/azure.ai.agents/internal/project/agent_definition.go index 921c7236ff7..f6e91b38229 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/agent_definition.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/agent_definition.go @@ -7,6 +7,8 @@ import ( "fmt" "maps" "os" + "slices" + "strings" "sync" "azureaiagent/internal/exterrors" @@ -16,6 +18,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/azure/azure-dev/cli/azd/pkg/foundry" + "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/braydonk/yaml" "google.golang.org/protobuf/types/known/structpb" ) @@ -70,6 +73,46 @@ func WarnLegacyAgentShape(source AgentDefinitionSource) { }) } +// WarnOrphanedConfigEnv warns when a service still declares +// environment variables under the deprecated config-nested env: +// block. azd reads service environment values only from the +// service-level env:, so anything left under config: is ignored by +// both `azd ai agent run` and deploy. +// +// Unlike the deprecated environmentVariables list, nothing migrates +// config.env and no azd command ever wrote it, so without this the +// values would disappear with no other signal. +func WarnOrphanedConfigEnv(svc *azdext.ServiceConfig) { + names := orphanedConfigEnvNames(svc) + if len(names) == 0 { + return + } + fmt.Printf("%s\n", output.WithWarningFormat( + "WARNING: service %q sets %s under the deprecated `config: "+ + "env:` block, which is no longer read and will be "+ + "ignored. Move them to the service-level `env:` so they "+ + "apply to both run and deploy. See %s", + svc.GetName(), strings.Join(names, ", "), MigrationGuideURL, + )) +} + +// orphanedConfigEnvNames returns the variable names a service still +// declares under the deprecated config-nested env: block, sorted. +// It reads svc.Config directly because core binds the service-level +// env: to ServiceConfig.Environment, so an env key can only reach a +// property bag from the nested shape. +func orphanedConfigEnvNames(svc *azdext.ServiceConfig) []string { + value, found := svc.GetConfig().GetFields()["env"] + if !found { + return nil + } + fields := value.GetStructValue().GetFields() + if len(fields) == 0 { + return nil + } + return slices.Sorted(maps.Keys(fields)) +} + // AgentDefinitionInline is the hosted-agent definition (formerly agent.yaml) // carried as flat service-level properties on the azure.ai.agent service entry. // @@ -84,11 +127,12 @@ func WarnLegacyAgentShape(source AgentDefinitionSource) { type AgentDefinitionInline struct { agent_yaml.AgentDefinition `json:",inline"` Protocols []agent_yaml.ProtocolVersionRecord `json:"protocols,omitempty"` - EnvironmentVariables *[]agent_yaml.EnvironmentVariable `json:"environmentVariables,omitempty"` - AgentEndpoint *agent_yaml.AgentEndpoint `json:"agentEndpoint,omitempty"` - AgentCard *agent_yaml.AgentCard `json:"agentCard,omitempty"` - CodeConfiguration *agent_yaml.CodeConfiguration `json:"codeConfiguration,omitempty"` - Policies []agent_yaml.Policy `json:"policies,omitempty"` + // EnvironmentVariables reads the deprecated inline shape. + EnvironmentVariables *[]agent_yaml.EnvironmentVariable `json:"environmentVariables,omitempty"` + AgentEndpoint *agent_yaml.AgentEndpoint `json:"agentEndpoint,omitempty"` + AgentCard *agent_yaml.AgentCard `json:"agentCard,omitempty"` + CodeConfiguration *agent_yaml.CodeConfiguration `json:"codeConfiguration,omitempty"` + Policies []agent_yaml.Policy `json:"policies,omitempty"` } // agentDefinitionToInline splits a ContainerAgent into the inline definition, @@ -97,13 +141,12 @@ type AgentDefinitionInline struct { // returned separately so the caller can place them on their respective homes. func agentDefinitionToInline(ca agent_yaml.ContainerAgent) (AgentDefinitionInline, *ContainerSettings, string) { inline := AgentDefinitionInline{ - AgentDefinition: ca.AgentDefinition, - Protocols: ca.Protocols, - EnvironmentVariables: ca.EnvironmentVariables, - AgentEndpoint: ca.AgentEndpoint, - AgentCard: ca.AgentCard, - CodeConfiguration: ca.CodeConfiguration, - Policies: ca.Policies, + AgentDefinition: ca.AgentDefinition, + Protocols: ca.Protocols, + AgentEndpoint: ca.AgentEndpoint, + AgentCard: ca.AgentCard, + CodeConfiguration: ca.CodeConfiguration, + Policies: ca.Policies, } var container *ContainerSettings @@ -119,12 +162,28 @@ func agentDefinitionToInline(ca agent_yaml.ContainerAgent) (AgentDefinitionInlin // toContainerAgent rebuilds the agent_yaml.ContainerAgent from the inline // definition, the CPU/memory carried in the `container` config, and the image // carried on the core service field. -func (d AgentDefinitionInline) toContainerAgent(container *ContainerSettings, image string) agent_yaml.ContainerAgent { +func (d AgentDefinitionInline) toContainerAgent( + container *ContainerSettings, + image string, + environment map[string]string, +) agent_yaml.ContainerAgent { + environmentVariables := d.EnvironmentVariables + if len(environment) > 0 { + legacyEnvironment := AgentEnvironment(agent_yaml.ContainerAgent{ + EnvironmentVariables: d.EnvironmentVariables, + }) + if legacyEnvironment == nil { + legacyEnvironment = map[string]string{} + } + maps.Copy(legacyEnvironment, environment) + environmentVariables = environmentVariablesFromMap(legacyEnvironment) + } + ca := agent_yaml.ContainerAgent{ AgentDefinition: d.AgentDefinition, Image: image, Protocols: d.Protocols, - EnvironmentVariables: d.EnvironmentVariables, + EnvironmentVariables: environmentVariables, AgentEndpoint: d.AgentEndpoint, AgentCard: d.AgentCard, CodeConfiguration: d.CodeConfiguration, @@ -141,6 +200,75 @@ func (d AgentDefinitionInline) toContainerAgent(container *ContainerSettings, im return ca } +// AgentEnvironment converts an agent environment list to a map. +func AgentEnvironment(ca agent_yaml.ContainerAgent) map[string]string { + if ca.EnvironmentVariables == nil || len(*ca.EnvironmentVariables) == 0 { + return nil + } + + environment := make(map[string]string, len(*ca.EnvironmentVariables)) + for _, variable := range *ca.EnvironmentVariables { + environment[variable.Name] = variable.Value + } + return environment +} + +// ResolveAgentEnvironmentVariable preserves values forwarded by core. +// A name the service declares in env: wins outright. Every other +// name expands through mapping, which both callers back with the +// full azd environment even when the service declares env:. +// +// That project fallback is deliberate. environment_variables is +// deprecated but additive: mergeAgentRunEnvironment lets env: +// override a same-named entry and keeps the rest, so a ${FOO} an +// author wrote there stays resolvable mid-migration. Dropping the +// fallback would turn it into an empty string with no error. +// +// The connections extension does drop it once env: is declared +// (connectionEnvironmentMapping in azure.ai.projects), because +// those values become ARM parameters at provision time rather +// than runtime values for a container the author owns. +func ResolveAgentEnvironmentVariable( + name string, + value string, + serviceEnvironment map[string]string, + mapping func(string) string, +) (string, error) { + if environmentValue, found := serviceEnvironment[name]; found { + return environmentValue, nil + } + return ExpandEnv(value, func(variableName string) string { + if environmentValue, found := serviceEnvironment[variableName]; found { + return environmentValue + } + if mapping == nil { + return "" + } + return mapping(variableName) + }) +} + +func environmentVariablesFromMap( + environment map[string]string, +) *[]agent_yaml.EnvironmentVariable { + if len(environment) == 0 { + return nil + } + + variables := make( + []agent_yaml.EnvironmentVariable, + 0, + len(environment), + ) + for _, name := range slices.Sorted(maps.Keys(environment)) { + variables = append(variables, agent_yaml.EnvironmentVariable{ + Name: name, + Value: environment[name], + }) + } + return &variables +} + // structHasKind reports whether the struct carries a non-empty string `kind`, // the marker that an agent definition is present in a service entry's inline or // config properties. @@ -227,6 +355,7 @@ func AgentDefinitionFromResolvedService( ca, isHosted, err := agentDefinitionFromStruct( resolved, image, + svc.GetEnvironment(), ) return ca, isHosted, true, candidate.source, err } @@ -287,7 +416,11 @@ func AgentDefinitionFromService( } } - ca, isHosted, err := agentDefinitionFromStruct(inlineStruct, svc.GetImage()) + ca, isHosted, err := agentDefinitionFromStruct( + inlineStruct, + svc.GetImage(), + svc.GetEnvironment(), + ) return ca, isHosted, true, source, err } @@ -464,6 +597,7 @@ func validateRootRefCoreFields( return err } for _, field := range []string{ + "env", "project", "language", "image", @@ -479,11 +613,7 @@ func validateRootRefCoreFields( return nil } -// UpsertAgentEnvVars adds or updates environment variables on the agent -// definition carried inline on the service entry, preserving every other key. -// It is used by commands that mutate the definition (e.g. `optimize apply`). -// Returns an error when the service carries no inline definition; callers fall -// back to mutating a legacy on-disk agent.yaml in that case. +// UpsertAgentEnvVars updates the service-level environment map. func UpsertAgentEnvVars(svc *azdext.ServiceConfig, kv map[string]string) error { ca, _, found, source, err := AgentDefinitionFromService(svc) if err != nil { @@ -493,55 +623,42 @@ func UpsertAgentEnvVars(svc *azdext.ServiceConfig, kv map[string]string) error { return fmt.Errorf("service %q does not carry an inline agent definition", svc.GetName()) } - envVars := []agent_yaml.EnvironmentVariable{} - if ca.EnvironmentVariables != nil { - envVars = *ca.EnvironmentVariables + environment := AgentEnvironment(ca) + if environment == nil { + environment = map[string]string{} } - for key, value := range kv { - idx := -1 - for i := range envVars { - if envVars[i].Name == key { - idx = i - break - } - } - if idx >= 0 { - envVars[idx].Value = value - } else { - envVars = append(envVars, agent_yaml.EnvironmentVariable{Name: key, Value: value}) - } - } - ca.EnvironmentVariables = &envVars + maps.Copy(environment, kv) + svc.Environment = environment - var props *structpb.Struct + props := svc.GetAdditionalProperties() if source == AgentDefinitionSourceLegacyConfig { props = svc.GetConfig() - } else { - props = svc.GetAdditionalProperties() } - if props == nil { - return fmt.Errorf( - "service %q does not carry an inline agent definition", - svc.GetName(), - ) - } - if props.Fields == nil { - props.Fields = map[string]*structpb.Value{} + if props != nil { + delete(props.Fields, "environmentVariables") } + return nil +} - envValues := make([]*structpb.Value, 0, len(envVars)) - for _, envVar := range envVars { - envValues = append(envValues, structpb.NewStructValue( - &structpb.Struct{Fields: map[string]*structpb.Value{ - "name": structpb.NewStringValue(envVar.Name), - "value": structpb.NewStringValue(envVar.Value), - }}, - )) +// InlineAgentEnvironmentVariables returns the deprecated inline +// environmentVariables carried on the agent definition as a raw +// template map, without merging the core-forwarded (already expanded) +// service environment. Values are the templates as authored, suitable +// for migrating into the env section without losing them. +func InlineAgentEnvironmentVariables( + svc *azdext.ServiceConfig, +) (map[string]string, error) { + props := ServiceConfigProps(svc) + if props == nil || len(props.GetFields()) == 0 { + return nil, nil } - props.Fields["environmentVariables"] = structpb.NewListValue( - &structpb.ListValue{Values: envValues}, - ) - return nil + var inline AgentDefinitionInline + if err := UnmarshalStruct(props, &inline); err != nil { + return nil, err + } + return AgentEnvironment(agent_yaml.ContainerAgent{ + EnvironmentVariables: inline.EnvironmentVariables, + }), nil } // SetAgentContainerSettings writes the resolved container settings onto the @@ -586,7 +703,11 @@ func SetAgentContainerSettings( // struct that carries the agent definition as service-level properties. coreImage // is the value of the service's `image` field, which is carried on the core // [azdext.ServiceConfig] rather than in the inline property bag. -func agentDefinitionFromStruct(s *structpb.Struct, coreImage string) (agent_yaml.ContainerAgent, bool, error) { +func agentDefinitionFromStruct( + s *structpb.Struct, + coreImage string, + environment map[string]string, +) (agent_yaml.ContainerAgent, bool, error) { var inline AgentDefinitionInline if err := UnmarshalStruct(s, &inline); err != nil { return agent_yaml.ContainerAgent{}, false, exterrors.Validation( @@ -612,7 +733,7 @@ func agentDefinitionFromStruct(s *structpb.Struct, coreImage string) (agent_yaml ) } - ca := inline.toContainerAgent(cfg.Container, coreImage) + ca := inline.toContainerAgent(cfg.Container, coreImage, environment) if err := validateAgentServiceDefinition(ca); err != nil { return agent_yaml.ContainerAgent{}, false, err diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/agent_definition_test.go b/cli/azd/extensions/azure.ai.agents/internal/project/agent_definition_test.go index 3c56fbf0387..6262f53fec1 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/agent_definition_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/agent_definition_test.go @@ -4,6 +4,7 @@ package project import ( + "io" "os" "path/filepath" "testing" @@ -52,11 +53,14 @@ func TestAgentDefinitionRoundTrip(t *testing.T) { props, err := AgentDefinitionToServiceProperties(ca, extra) require.NoError(t, err) + _, hasInlineEnvironment := props.GetFields()["environmentVariables"] + require.False(t, hasInlineEnvironment) svc := &azdext.ServiceConfig{ Name: "basic-agent", Host: "azure.ai.agent", AdditionalProperties: props, + Environment: AgentEnvironment(ca), } got, isHosted, found, source, err := AgentDefinitionFromService(svc) @@ -111,6 +115,118 @@ func TestAgentDefinitionFromService_LegacyConfigShape(t *testing.T) { require.Equal(t, "basic-agent", got.Name) } +func TestAgentDefinitionFromService_LegacyEnvironment(t *testing.T) { + props, err := AgentDefinitionToServiceProperties( + sampleContainerAgent(), + nil, + ) + require.NoError(t, err) + legacyEnvironment, err := structpb.NewValue([]any{ + map[string]any{ + "name": "LEGACY_KEY", + "value": "${LEGACY_KEY}", + }, + map[string]any{ + "name": "SHARED_KEY", + "value": "legacy", + }, + }) + require.NoError(t, err) + props.Fields["environmentVariables"] = legacyEnvironment + + svc := &azdext.ServiceConfig{ + Name: "basic-agent", + Host: "azure.ai.agent", + Config: props, + Environment: map[string]string{ + "NEW_KEY": "new", + "SHARED_KEY": "service", + }, + } + got, _, found, source, err := AgentDefinitionFromService(svc) + require.NoError(t, err) + require.True(t, found) + require.Equal(t, AgentDefinitionSourceLegacyConfig, source) + require.Equal(t, map[string]string{ + "LEGACY_KEY": "${LEGACY_KEY}", + "NEW_KEY": "new", + "SHARED_KEY": "service", + }, AgentEnvironment(got)) +} + +// TestInlineAgentEnvironmentVariables verifies the raw inline +// environmentVariables are returned as authored, without merging the +// core-forwarded (already expanded) service environment. +func TestInlineAgentEnvironmentVariables(t *testing.T) { + props, err := AgentDefinitionToServiceProperties( + sampleContainerAgent(), + nil, + ) + require.NoError(t, err) + legacyEnvironment, err := structpb.NewValue([]any{ + map[string]any{ + "name": "LEGACY_KEY", + "value": "${LEGACY_KEY}", + }, + map[string]any{ + "name": "SHARED_KEY", + "value": "legacy", + }, + }) + require.NoError(t, err) + props.Fields["environmentVariables"] = legacyEnvironment + + svc := &azdext.ServiceConfig{ + Name: "basic-agent", + Host: "azure.ai.agent", + AdditionalProperties: props, + // Core-forwarded env must NOT leak into the raw result. + Environment: map[string]string{ + "NEW_KEY": "new", + "SHARED_KEY": "service", + }, + } + got, err := InlineAgentEnvironmentVariables(svc) + require.NoError(t, err) + require.Equal(t, map[string]string{ + "LEGACY_KEY": "${LEGACY_KEY}", + "SHARED_KEY": "legacy", + }, got) +} +func TestResolveAgentEnvironmentVariable(t *testing.T) { + t.Parallel() + + t.Run("preserves same-name core value", func(t *testing.T) { + value, err := ResolveAgentEnvironmentVariable( + "FORWARDED_VALUE", + "${FORWARDED_VALUE}", + map[string]string{ + "FORWARDED_VALUE": "literal ${NOT_A_TEMPLATE}", + }, + func(string) string { + return "expanded" + }, + ) + require.NoError(t, err) + require.Equal(t, "literal ${NOT_A_TEMPLATE}", value) + }) + + t.Run("resolves aliases from service env first", func(t *testing.T) { + value, err := ResolveAgentEnvironmentVariable( + "TARGET", + "${SERVICE_ENDPOINT}", + map[string]string{ + "SERVICE_ENDPOINT": "https://service.example", + }, + func(string) string { + return "https://project.example" + }, + ) + require.NoError(t, err) + require.Equal(t, "https://service.example", value) + }) +} + func TestLoadAgentDefinition_UnrelatedInlineFallsBackToConfig( t *testing.T, ) { @@ -483,6 +599,7 @@ func TestResolveServiceConfigInPlaceRejectsCoreFieldsFromRootRef(t *testing.T) { name string value string }{ + {name: "env", value: "env:\n LOG_LEVEL: info\n"}, {name: "project", value: "project: src/agent\n"}, {name: "language", value: "language: docker\n"}, {name: "image", value: "image: registry.example/agent:v1\n"}, @@ -567,9 +684,15 @@ func TestAgentDefinitionUsesFileRefIgnoresInlineDefinition(t *testing.T) { // TestUpsertAgentEnvVars verifies that env vars are added/updated on the inline // definition while preserving the other definition keys. func TestUpsertAgentEnvVars(t *testing.T) { - props, err := AgentDefinitionToServiceProperties(sampleContainerAgent(), nil) + ca := sampleContainerAgent() + props, err := AgentDefinitionToServiceProperties(ca, nil) require.NoError(t, err) - svc := &azdext.ServiceConfig{Name: "basic-agent", Host: "azure.ai.agent", AdditionalProperties: props} + svc := &azdext.ServiceConfig{ + Name: "basic-agent", + Host: "azure.ai.agent", + AdditionalProperties: props, + Environment: AgentEnvironment(ca), + } require.NoError(t, UpsertAgentEnvVars(svc, map[string]string{ "FOUNDRY_MODEL_DEPLOYMENT_NAME": "gpt-4o", // update existing @@ -581,11 +704,10 @@ func TestUpsertAgentEnvVars(t *testing.T) { require.True(t, found) require.Equal(t, "basic-agent", got.Name) // other keys preserved require.NotNil(t, got.EnvironmentVariables) + _, hasInlineEnvironment := props.GetFields()["environmentVariables"] + require.False(t, hasInlineEnvironment) - values := map[string]string{} - for _, ev := range *got.EnvironmentVariables { - values[ev.Name] = ev.Value - } + values := AgentEnvironment(got) require.Equal(t, "gpt-4o", values["FOUNDRY_MODEL_DEPLOYMENT_NAME"]) require.Equal(t, "cand-1", values["OPTIMIZATION_CANDIDATE_ID"]) } @@ -618,3 +740,128 @@ func TestUpsertAgentEnvVarsPreservesNestedReferences(t *testing.T) { GetStructValue().GetFields()["$ref"].GetStringValue(), ) } + +// TestOrphanedConfigEnvNames pins detection of the removed +// config-nested env: block. Nothing reads or migrates it, so run and +// deploy must be able to tell the user which values are being +// dropped. +func TestOrphanedConfigEnvNames(t *testing.T) { + tests := []struct { + name string + svc *azdext.ServiceConfig + want []string + }{ + { + name: "nil service", + svc: nil, + }, + { + name: "no config block", + svc: &azdext.ServiceConfig{ + Name: "agent", + Environment: map[string]string{"API_KEY": "value"}, + }, + }, + { + name: "config without env", + svc: &azdext.ServiceConfig{ + Name: "agent", + Config: mustStruct(t, map[string]any{"kind": "hosted"}), + }, + }, + { + name: "empty config env", + svc: &azdext.ServiceConfig{ + Name: "agent", + Config: mustStruct(t, map[string]any{ + "env": map[string]any{}, + }), + }, + }, + { + name: "populated config env is reported sorted", + svc: &azdext.ServiceConfig{ + Name: "agent", + Config: mustStruct(t, map[string]any{ + "kind": "hosted", + "env": map[string]any{ + "LOG_LEVEL": "debug", + "API_KEY": "${SECRET}", + }, + }), + }, + want: []string{"API_KEY", "LOG_LEVEL"}, + }, + { + // A service-level env: is bound by core to + // ServiceConfig.Environment, so it must never be + // mistaken for the dead nested shape. + name: "service level env is not flagged", + svc: &azdext.ServiceConfig{ + Name: "agent", + Environment: map[string]string{"API_KEY": "value"}, + Config: mustStruct(t, map[string]any{"kind": "hosted"}), + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + require.Equal(t, test.want, orphanedConfigEnvNames(test.svc)) + }) + } +} + +// TestWarnOrphanedConfigEnvOutput verifies the user actually sees the +// dropped variable names, since the whole point is that the values +// no longer disappear silently. +func TestWarnOrphanedConfigEnvOutput(t *testing.T) { + svc := &azdext.ServiceConfig{ + Name: "my-agent", + Config: mustStruct(t, map[string]any{ + "env": map[string]any{ + "API_KEY": "${SECRET}", + "LOG_LEVEL": "debug", + }, + }), + } + + out := captureStdout(t, func() { WarnOrphanedConfigEnv(svc) }) + require.Contains(t, out, "my-agent") + require.Contains(t, out, "API_KEY") + require.Contains(t, out, "LOG_LEVEL") + require.Contains(t, out, "env:") + + quiet := captureStdout(t, func() { + WarnOrphanedConfigEnv(&azdext.ServiceConfig{ + Name: "my-agent", + Environment: map[string]string{"API_KEY": "value"}, + }) + }) + require.Empty(t, quiet) +} + +func mustStruct(t *testing.T, value map[string]any) *structpb.Struct { + t.Helper() + s, err := structpb.NewStruct(value) + require.NoError(t, err) + return s +} + +// captureStdout collects everything fn writes to os.Stdout. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + reader, writer, err := os.Pipe() + require.NoError(t, err) + + original := os.Stdout + os.Stdout = writer + defer func() { os.Stdout = original }() + + fn() + require.NoError(t, writer.Close()) + + data, err := io.ReadAll(reader) + require.NoError(t, err) + return string(data) +} diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/config.go b/cli/azd/extensions/azure.ai.agents/internal/project/config.go index 8a581fc73a8..144d25fb60a 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/config.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/config.go @@ -45,7 +45,6 @@ type ServiceTargetAgentConfig struct { // Foundry project. Its presence is the brownfield signal that makes provision // connect to that project instead of creating a new one. Endpoint string `json:"endpoint,omitempty"` - Environment map[string]string `json:"env,omitempty"` Container *ContainerSettings `json:"container,omitempty"` Deployments []Deployment `json:"deployments,omitempty"` Resources []Resource `json:"resources,omitempty"` diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/config_test.go b/cli/azd/extensions/azure.ai.agents/internal/project/config_test.go index 33c8819ff18..ac4fa16b397 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/config_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/config_test.go @@ -147,7 +147,6 @@ func TestServiceTargetAgentConfig_MultipleToolboxes(t *testing.T) { // alongside other ServiceTargetAgentConfig fields. func TestServiceTargetAgentConfig_WithOtherFields(t *testing.T) { original := ServiceTargetAgentConfig{ - Environment: map[string]string{"KEY": "VALUE"}, Deployments: []Deployment{ { Name: "test-deployment", @@ -187,10 +186,6 @@ func TestServiceTargetAgentConfig_WithOtherFields(t *testing.T) { t.Fatalf("UnmarshalStruct failed: %v", err) } - if roundTripped.Environment["KEY"] != "VALUE" { - t.Errorf("Expected env KEY=VALUE, got '%s'", roundTripped.Environment["KEY"]) - } - if len(roundTripped.Deployments) != 1 { t.Fatalf("Expected 1 deployment, got %d", len(roundTripped.Deployments)) } diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent.go b/cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent.go index c2e410aa174..e6f388e7248 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent.go @@ -15,6 +15,7 @@ import ( "io" "io/fs" "log" + "maps" "net/http" "net/url" "os" @@ -31,7 +32,6 @@ import ( "azureaiagent/internal/pkg/agents/agent_yaml" "azureaiagent/internal/pkg/azure" "azureaiagent/internal/pkg/paths" - "azureaiagent/internal/pkg/projectconfig" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -153,10 +153,14 @@ type AgentServiceTargetProvider struct { // agentDefinitionPath is only set for the file-based and env-override paths // (not the inline unified shape), so both are checked as the idempotency guard. deployContextReady bool - credential *azidentity.AzureDeveloperCLICredential - tenantId string - env *azdext.Environment - foundryProject *arm.ResourceID + // serviceConfigResolved tracks whether serviceConfig has had + // its local $ref includes expanded. Cleared whenever a newer + // config is adopted. + serviceConfigResolved bool + credential *azidentity.AzureDeveloperCLICredential + tenantId string + env *azdext.Environment + foundryProject *arm.ResourceID } const ( @@ -182,16 +186,56 @@ func NewAgentServiceTargetProvider(azdClient *azdext.AzdClient) azdext.ServiceTa // agent.yaml, tenant lookup, credential) lives in ensureDeployContext and runs // only when a deploy-time entrypoint needs it. func (p *AgentServiceTargetProvider) Initialize(ctx context.Context, serviceConfig *azdext.ServiceConfig) error { + p.adoptServiceConfig(serviceConfig) + return nil +} + +// adoptServiceConfig stores the service config azd core supplied +// for the current call. Core re-expands ${VAR} references against +// the environment on every request, so a deploy-time config can +// carry values the Initialize-time snapshot lacked, for example a +// location the user was prompted for during provision. Keeping the +// newest config avoids deploying with the empty strings that unset +// variables expand to. +func (p *AgentServiceTargetProvider) adoptServiceConfig(serviceConfig *azdext.ServiceConfig) { + if serviceConfig == nil || serviceConfig == p.serviceConfig { + return + } p.serviceConfig = serviceConfig + p.serviceConfigResolved = false +} + +// resolveServiceConfig expands local $ref includes on the current +// service config. It is idempotent per config instance, so repeat +// calls stay cheap while a freshly adopted config is always +// re-resolved. +func (p *AgentServiceTargetProvider) resolveServiceConfig() error { + if p.serviceConfigResolved || p.serviceConfig == nil || p.projectPath == "" { + return nil + } + if err := ResolveServiceConfigInPlace(p.serviceConfig, p.projectPath); err != nil { + return exterrors.Validation( + exterrors.CodeInvalidServiceConfig, + fmt.Sprintf( + "failed to resolve service config for %s: %s", + p.serviceConfig.Name, + err, + ), + "fix the agent service configuration in azure.yaml", + ) + } + p.serviceConfigResolved = true return nil } // ensureDeployContext lazily resolves the agent definition file, the azd // environment, the tenant, and the credential. Idempotent via the -// agentDefinitionPath short-circuit. +// agentDefinitionPath short-circuit. The short-circuit still resolves +// the service config so a newer one adopted after the first +// deploy-time call is expanded before consumers read it. func (p *AgentServiceTargetProvider) ensureDeployContext(ctx context.Context) error { if p.deployContextReady || p.agentDefinitionPath != "" { - return nil + return p.resolveServiceConfig() } if p.serviceConfig == nil { return exterrors.Internal( @@ -208,19 +252,9 @@ func (p *AgentServiceTargetProvider) ensureDeployContext(ctx context.Context) er "run 'azd init' to initialize your project", ) } - if err := ResolveServiceConfigInPlace( - p.serviceConfig, - proj.Project.Path, - ); err != nil { - return exterrors.Validation( - exterrors.CodeInvalidServiceConfig, - fmt.Sprintf( - "failed to resolve service config for %s: %s", - p.serviceConfig.Name, - err, - ), - "fix the agent service configuration in azure.yaml", - ) + p.projectPath = proj.Project.Path + if err := p.resolveServiceConfig(); err != nil { + return err } servicePath := p.serviceConfig.GetRelativePath() fullPath, err := paths.JoinAllowRoot(proj.Project.Path, servicePath) @@ -283,7 +317,6 @@ func (p *AgentServiceTargetProvider) ensureDeployContext(ctx context.Context) er } p.credential = cred - p.projectPath = proj.Project.Path p.servicePath = fullPath // Check if user has specified agent definition path via environment variable @@ -464,6 +497,7 @@ func (p *AgentServiceTargetProvider) GetTargetResource( serviceConfig *azdext.ServiceConfig, defaultResolver func() (*azdext.TargetResource, error), ) (*azdext.TargetResource, error) { + p.adoptServiceConfig(serviceConfig) if err := p.ensureDeployContext(ctx); err != nil { return nil, err } @@ -527,9 +561,11 @@ func (p *AgentServiceTargetProvider) Package( serviceContext *azdext.ServiceContext, progress azdext.ProgressReporter, ) (*azdext.ServicePackageResult, error) { + p.adoptServiceConfig(serviceConfig) if err := p.ensureDeployContext(ctx); err != nil { return nil, err } + serviceConfig = p.serviceConfig // Code deploy: ZIP the source directory if p.isCodeDeployAgent() { progress("Packaging code") @@ -643,9 +679,11 @@ func (p *AgentServiceTargetProvider) Publish( }, nil } + p.adoptServiceConfig(serviceConfig) if err := p.ensureDeployContext(ctx); err != nil { return nil, err } + serviceConfig = p.serviceConfig // Code deploy skips Publish (no ACR needed) if p.isCodeDeployAgent() { return &azdext.ServicePublishResult{}, nil @@ -1022,6 +1060,7 @@ func (p *AgentServiceTargetProvider) Deploy( targetResource *azdext.TargetResource, progress azdext.ProgressReporter, ) (*azdext.ServiceDeployResult, error) { + p.adoptServiceConfig(serviceConfig) if err := p.ensureDeployContext(ctx); err != nil { return nil, err } @@ -1442,6 +1481,25 @@ func (p *AgentServiceTargetProvider) prepareDeploy( fmt.Fprintf(os.Stderr, "Using endpoint: %s\n", azdEnv["FOUNDRY_PROJECT_ENDPOINT"]) fmt.Fprintf(os.Stderr, "Agent Name: %s\n", agentDef.Name) + // Seed core-expanded values before resolving legacy variables. + resolvedEnvVars := maps.Clone(serviceConfig.GetEnvironment()) + if resolvedEnvVars == nil { + resolvedEnvVars = make(map[string]string) + } + if agentDef.EnvironmentVariables != nil { + for _, envVar := range *agentDef.EnvironmentVariables { + if _, found := resolvedEnvVars[envVar.Name]; found { + continue + } + resolvedEnvVars[envVar.Name] = p.resolveEnvironmentVariables( + envVar.Name, + envVar.Value, + serviceConfig.GetEnvironment(), + azdEnv, + ) + } + } + // Parse service config for container resource overrides foundryAgentConfig, err := LoadServiceTargetAgentConfig(serviceConfig) if err != nil { @@ -1451,35 +1509,8 @@ func (p *AgentServiceTargetProvider) prepareDeploy( "check the service configuration in azure.yaml", ) } - serviceEnv, err := p.serviceEnvironment(serviceConfig) - if err != nil { - return nil, exterrors.Validation( - exterrors.CodeInvalidServiceConfig, - fmt.Sprintf( - "failed to load service environment: %s", - err, - ), - "fix the service env configuration in azure.yaml", - ) - } - - resolvedEnvVars := make(map[string]string) - if agentDef.EnvironmentVariables != nil { - for _, envVar := range *agentDef.EnvironmentVariables { - resolvedEnvVars[envVar.Name] = - p.resolveEnvironmentVariables(envVar.Value, azdEnv) - } - } - for name, value := range foundryAgentConfig.Environment { - resolvedEnvVars[name] = - p.resolveEnvironmentVariables(value, azdEnv) - } - for name, value := range serviceEnv { - resolvedEnvVars[name] = - p.resolveEnvironmentVariables(value, azdEnv) - } - warnDeprecatedScaleSettings(ServiceConfigProps(serviceConfig)) + WarnOrphanedConfigEnv(serviceConfig) var cpu, memory string if foundryAgentConfig != nil && foundryAgentConfig.Container != nil && foundryAgentConfig.Container.Resources != nil { @@ -1534,22 +1565,6 @@ func (p *AgentServiceTargetProvider) prepareDeploy( }, nil } -func (p *AgentServiceTargetProvider) serviceEnvironment( - serviceConfig *azdext.ServiceConfig, -) (map[string]string, error) { - raw, err := projectconfig.LoadServiceEnvironment( - p.projectPath, - serviceConfig.GetName(), - ) - if err != nil { - return nil, err - } - if raw != nil { - return raw, nil - } - return serviceConfig.GetEnvironment(), nil -} - // deployResult holds the intermediate results from a deploy method (code or container) // before the common post-deploy steps (polling, patching, finalization) are applied. type deployResult struct { @@ -2684,12 +2699,21 @@ func (p *AgentServiceTargetProvider) registerAgentEnvironmentVariables( return nil } -// resolveEnvironmentVariables resolves ${ENV_VAR} style references in value using azd environment variables. -// Supports default values (e.g., "${VAR:-default}") and multiple expressions (e.g., "${VAR1}-${VAR2}"). -func (p *AgentServiceTargetProvider) resolveEnvironmentVariables(value string, azdEnv map[string]string) string { - resolved, err := ExpandEnv(value, func(varName string) string { - return azdEnv[varName] - }) +// resolveEnvironmentVariables expands legacy inline templates. +func (p *AgentServiceTargetProvider) resolveEnvironmentVariables( + name string, + value string, + serviceEnvironment map[string]string, + azdEnv map[string]string, +) string { + resolved, err := ResolveAgentEnvironmentVariable( + name, + value, + serviceEnvironment, + func(varName string) string { + return azdEnv[varName] + }, + ) if err != nil { // If resolution fails, return original value return value diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent_test.go b/cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent_test.go index 307571d6023..8f948bf836d 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/service_target_agent_test.go @@ -406,6 +406,93 @@ func TestInitializeRejectsAgentYamlSymlinkEscapingRoot(t *testing.T) { require.Empty(t, provider.agentDefinitionPath) } +func TestDeployTimeServiceConfigReplacesInitializeSnapshot(t *testing.T) { + // azd core re-expands ${VAR} against the environment on every + // call, so a deploy-time config can carry a value that was still + // unset (and therefore expanded to "") when Initialize ran. + // `azd up` initializes service targets before provisioning + // prompts for a missing subscription or location, so keeping the + // Initialize snapshot would deploy those empty strings. + t.Setenv("AGENT_DEFINITION_PATH", "") + + projectRoot := t.TempDir() + serviceDir := filepath.Join(projectRoot, "svc") + require.NoError(t, os.MkdirAll(serviceDir, 0o750)) + require.NoError(t, os.WriteFile( + filepath.Join(serviceDir, "agent.yaml"), + []byte("kind: hostedAgent\n"), + 0o600, + )) + + provider := &AgentServiceTargetProvider{ + azdClient: newInitializeTestClient(t, projectRoot), + } + + // AGENT_REGION references an unset variable at Initialize time. + stale := &azdext.ServiceConfig{ + Name: "echo", + RelativePath: "svc", + Environment: map[string]string{"AGENT_REGION": ""}, + } + require.NoError(t, provider.Initialize(t.Context(), stale)) + require.NoError(t, provider.ensureDeployContext(t.Context())) + + // The user is prompted during provision, so core hands the + // deploy-time call a config with the persisted value. + fresh := &azdext.ServiceConfig{ + Name: "echo", + RelativePath: "svc", + Environment: map[string]string{"AGENT_REGION": "westus2"}, + } + + // Deploy must adopt the config it was handed rather than reuse the + // snapshot. It still fails further along (this stub has no Foundry + // project), which is after the config has been adopted. + _, err := provider.Deploy( + t.Context(), + fresh, + &azdext.ServiceContext{}, + nil, + func(string) {}, + ) + require.Error(t, err) + require.Same(t, fresh, provider.serviceConfig) + + prep, err := provider.prepareDeploy( + provider.serviceConfig, + sampleContainerAgent(), + map[string]string{"FOUNDRY_PROJECT_ENDPOINT": "https://project.example"}, + []agent_yaml.AgentBuildOption{ + agent_yaml.WithImageURL("registry.example/agent:latest"), + }, + ) + require.NoError(t, err) + require.Equal(t, "westus2", prep.resolvedEnvVars["AGENT_REGION"]) +} + +func TestAdoptServiceConfigIgnoresNilAndKeepsResolvedState(t *testing.T) { + t.Parallel() + + existing := &azdext.ServiceConfig{Name: "echo"} + provider := &AgentServiceTargetProvider{ + serviceConfig: existing, + serviceConfigResolved: true, + } + + // A nil config (or the same instance) must not drop the resolved + // state, otherwise every repeat call would re-expand $ref + // includes. + provider.adoptServiceConfig(nil) + require.Same(t, existing, provider.serviceConfig) + require.True(t, provider.serviceConfigResolved) + + provider.adoptServiceConfig(existing) + require.True(t, provider.serviceConfigResolved) + + provider.adoptServiceConfig(&azdext.ServiceConfig{Name: "echo"}) + require.False(t, provider.serviceConfigResolved) +} + func createSymlinkOrSkip(t *testing.T, oldname, newname string) { t.Helper() @@ -1004,6 +1091,51 @@ func TestLoadContainerAgentDefinition_MalformedYAMLReturnsError(t *testing.T) { require.Contains(t, err.Error(), "agent.yaml is not valid") } +func TestPrepareDeployIncludesServiceEnvironment(t *testing.T) { + t.Parallel() + + agentDef := sampleContainerAgent() + *agentDef.EnvironmentVariables = append( + *agentDef.EnvironmentVariables, + agent_yaml.EnvironmentVariable{ + Name: "LEGACY_ONLY", + Value: "${GLOBAL_VALUE}", + }, + agent_yaml.EnvironmentVariable{ + Name: "SHARED", + Value: "${SHARED}", + }, + ) + serviceConfig := &azdext.ServiceConfig{ + Name: "basic-agent", + Environment: map[string]string{ + "SERVICE_ONLY": "literal ${NOT_A_TEMPLATE}", + "SHARED": "service", + }, + } + + prep, err := (&AgentServiceTargetProvider{}).prepareDeploy( + serviceConfig, + agentDef, + map[string]string{ + "FOUNDRY_PROJECT_ENDPOINT": "https://project.example", + "GLOBAL_VALUE": "legacy", + "SHARED": "global", + }, + []agent_yaml.AgentBuildOption{ + agent_yaml.WithImageURL("registry.example/agent:latest"), + }, + ) + require.NoError(t, err) + require.Equal( + t, + "literal ${NOT_A_TEMPLATE}", + prep.resolvedEnvVars["SERVICE_ONLY"], + ) + require.Equal(t, "service", prep.resolvedEnvVars["SHARED"]) + require.Equal(t, "legacy", prep.resolvedEnvVars["LEGACY_ONLY"]) +} + func TestLoadContainerAgentDefinition_EnvPathOverridesInlineDefinition(t *testing.T) { t.Parallel() @@ -1097,129 +1229,6 @@ func TestPackageBuildsContainerAgent(t *testing.T) { require.Equal(t, int32(1), containerStub.packageCalls.Load()) } -func TestPrepareDeploy_MergesUnifiedEnvironment(t *testing.T) { - t.Parallel() - - agentDef := sampleContainerAgent() - agentDef.EnvironmentVariables = &[]agent_yaml.EnvironmentVariable{ - {Name: "LEGACY_ONLY", Value: "${LEGACY_VALUE}"}, - {Name: "SHARED", Value: "legacy"}, - } - props, err := AgentDefinitionToServiceProperties( - agentDef, - &ServiceTargetAgentConfig{ - Environment: map[string]string{ - "REF_ONLY": "${REF_VALUE}", - "SHARED": "ref", - }, - }, - ) - require.NoError(t, err) - svc := &azdext.ServiceConfig{ - Name: "basic-agent", - AdditionalProperties: props, - Environment: map[string]string{ - "DIRECT_ONLY": "direct", - "SHARED": "direct", - }, - } - provider := &AgentServiceTargetProvider{} - - prep, err := provider.prepareDeploy( - svc, - agentDef, - map[string]string{ - "FOUNDRY_PROJECT_ENDPOINT": "https://example", - "LEGACY_VALUE": "legacy-value", - "REF_VALUE": "ref-value", - }, - []agent_yaml.AgentBuildOption{ - agent_yaml.WithImageURL("registry.example/agent:v1"), - }, - ) - - require.NoError(t, err) - definition, ok := prep.request.Definition.(agent_api.HostedAgentDefinition) - require.True(t, ok) - require.Equal( - t, - "legacy-value", - definition.EnvironmentVariables["LEGACY_ONLY"], - ) - require.Equal( - t, - "ref-value", - definition.EnvironmentVariables["REF_ONLY"], - ) - require.Equal( - t, - "direct", - definition.EnvironmentVariables["DIRECT_ONLY"], - ) - require.Equal( - t, - "direct", - definition.EnvironmentVariables["SHARED"], - ) -} - -func TestPrepareDeployUsesRawUnifiedEnvironment(t *testing.T) { - t.Parallel() - - root := t.TempDir() - require.NoError(t, os.WriteFile( - filepath.Join(root, "azure.yaml"), - []byte(`services: - basic-agent: - host: azure.ai.agent - env: - PROJECT: ${{project.endpoint}} - ENABLED: true -`), - 0o600, - )) - agentDef := sampleContainerAgent() - props, err := AgentDefinitionToServiceProperties( - agentDef, - &ServiceTargetAgentConfig{}, - ) - require.NoError(t, err) - svc := &azdext.ServiceConfig{ - Name: "basic-agent", - AdditionalProperties: props, - Environment: map[string]string{ - "PROJECT": "", - "ENABLED": "", - }, - } - provider := &AgentServiceTargetProvider{projectPath: root} - - prep, err := provider.prepareDeploy( - svc, - agentDef, - map[string]string{ - "FOUNDRY_PROJECT_ENDPOINT": "https://example", - }, - []agent_yaml.AgentBuildOption{ - agent_yaml.WithImageURL("registry.example/agent:v1"), - }, - ) - - require.NoError(t, err) - definition, ok := prep.request.Definition.(agent_api.HostedAgentDefinition) - require.True(t, ok) - require.Equal( - t, - "${{project.endpoint}}", - definition.EnvironmentVariables["PROJECT"], - ) - require.Equal( - t, - "true", - definition.EnvironmentVariables["ENABLED"], - ) -} - func TestPrepareDeployAppliesDefaultResources(t *testing.T) { t.Parallel() 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..4e68a9bdb67 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go @@ -52,12 +52,16 @@ type Input struct { // value is not checked (only existence and endpoint: are). AcceptedHosts []string - // Env maps azd environment variable names to values. Used to resolve - // ${VAR} references in network fields (subnet vnet ids, dns.subscription). - // When a referenced variable is absent here, the synthesizer falls back - // to the process environment before failing. May be nil. + // Env maps project-wide azd values. + // Network fields always use it. + // Legacy connection services use it when service env is absent. + // Missing values may fall back to the process environment. Env map[string]string + // ServiceEnvironments contains core-expanded values by service. + // Connection fields prefer these values over the legacy Env map. + ServiceEnvironments map[string]map[string]string + // PreserveVarRefs keeps ${VAR} references verbatim instead of resolving // them. Used by the eject path, where the synthesized main.parameters.json // must stay environment-portable: the on-disk provision flow resolves @@ -291,6 +295,7 @@ func Synthesize(in Input) (*Result, error) { connections, err := collectConnections( root.Services, in.Env, + in.ServiceEnvironments, !in.PreserveVarRefs, in.ProjectRoot, ) @@ -322,6 +327,39 @@ func Synthesize(in Input) (*Result, error) { }, nil } +// ConnectionEnvironmentScopes returns services that declare env. +// An empty env block still establishes an isolated service scope. +func ConnectionEnvironmentScopes( + raw []byte, + projectRoot string, +) (map[string]bool, error) { + if len(raw) == 0 { + return nil, errors.New("synthesis: raw azure.yaml is empty") + } + + var root projectFile + if err := yaml.Unmarshal(raw, &root); err != nil { + return nil, fmt.Errorf("parse azure.yaml: %w", err) + } + + scopes := map[string]bool{} + for name, node := range root.Services { + node, matches, err := serviceForHost( + node, + projectRoot, + name, + aiConnectionHost, + ) + if err != nil { + return nil, err + } + if matches && connectionEnvDeclared(node) { + scopes[name] = true + } + } + return scopes, nil +} + // BrownfieldDeployments returns the model deployments declared on a brownfield // (endpoint:) Foundry project service. Synthesize short-circuits with // ErrEndpointBrownfield before reading deployments:, so the provider uses this @@ -362,6 +400,7 @@ func BrownfieldDeployments( func BrownfieldConnections( raw []byte, env map[string]string, + serviceEnvironments map[string]map[string]string, projectRoot string, ) ([]Connection, error) { if len(raw) == 0 { @@ -373,7 +412,13 @@ func BrownfieldConnections( return nil, fmt.Errorf("parse azure.yaml: %w", err) } - return collectConnections(root.Services, env, true, projectRoot) + return collectConnections( + root.Services, + env, + serviceEnvironments, + true, + projectRoot, + ) } // ProjectEndpoint returns the endpoint configured on a Foundry project service. @@ -629,12 +674,13 @@ func agentNeedsAcr(a agentBlock) bool { // (the service key is the connection name) and returns them sorted by name so // the synthesized parameter is deterministic regardless of YAML map order. // -// ${VAR} in target/credentials/metadata is expanded from env when resolve is -// true (provision path) and kept verbatim when false (eject path); Foundry -// ${{...}} expressions are always preserved, mirroring synthesizeNetwork. +// Provisioning resolves ${VAR} from service env when present. +// Legacy services use project and process values. +// Eject keeps references, and Foundry ${{...}} remains unchanged. func collectConnections( services map[string]yaml.Node, env map[string]string, + serviceEnvironments map[string]map[string]string, resolve bool, projectRoot string, ) ([]Connection, error) { @@ -660,17 +706,27 @@ func collectConnections( return nil, fmt.Errorf("services.%s: decode connection: %w", name, err) } - target, err := maybeExpand(svc.Target, env, resolve) + declared := len(serviceEnvironments[name]) > 0 || connectionEnvDeclared(node) + mapping := connectionEnvironmentMapping( + env, + serviceEnvironments[name], + declared, + ) + target, err := maybeExpand(svc.Target, mapping, resolve) if err != nil { return nil, fmt.Errorf("services.%s.target: %w", name, err) } - credentials, err := expandCredentials(svc.Credentials, env, resolve) + credentials, err := expandCredentials( + svc.Credentials, + mapping, + resolve, + ) if err != nil { return nil, fmt.Errorf("services.%s.credentials: %w", name, err) } - metadata, err := expandMetadata(svc.Metadata, env, resolve) + metadata, err := expandMetadata(svc.Metadata, mapping, resolve) if err != nil { return nil, fmt.Errorf("services.%s.metadata: %w", name, err) } @@ -691,20 +747,54 @@ func collectConnections( return connections, nil } +// connectionEnvDeclared reports whether the service node +// declares an env: key, including an empty env: {}. Core +// collapses an empty env to an omitted one, so the raw node +// is the only signal that a service opted into an isolated +// (possibly empty) scope. +func connectionEnvDeclared(node yaml.Node) bool { + var fields map[string]yaml.Node + if err := node.Decode(&fields); err != nil { + return false + } + _, ok := fields["env"] + return ok +} + +// Use scoped values when the service declares env. +// Legacy services use the project and process environments. +func connectionEnvironmentMapping( + env map[string]string, + serviceEnvironment map[string]string, + declared bool, +) func(string) string { + if declared { + return func(name string) string { + return serviceEnvironment[name] + } + } + + return func(name string) string { + if value, found := env[name]; found { + return value + } + value, _ := os.LookupEnv(name) + return value + } +} + // maybeExpand expands ${VAR} references in s when resolve is true, preserving // Foundry ${{...}} expressions; when resolve is false it returns s unchanged so // the eject path keeps references verbatim. -func maybeExpand(s string, env map[string]string, resolve bool) (string, error) { +func maybeExpand( + s string, + mapping func(string) string, + resolve bool, +) (string, error) { if !resolve || s == "" { return s, nil } - return foundry.ExpandEnv(s, func(name string) string { - if v, ok := env[name]; ok { - return v - } - v, _ := os.LookupEnv(name) - return v - }) + return foundry.ExpandEnv(s, mapping) } // expandCredentials deep-copies a credentials map, expanding ${VAR} in every @@ -713,7 +803,7 @@ func maybeExpand(s string, env map[string]string, resolve bool) (string, error) // credentials entirely (e.g. None / identity auth). func expandCredentials( creds map[string]any, - env map[string]string, + mapping func(string) string, resolve bool, ) (map[string]any, error) { if creds == nil { @@ -721,7 +811,7 @@ func expandCredentials( } out := make(map[string]any, len(creds)) for k, v := range creds { - expanded, err := expandValue(v, env, resolve) + expanded, err := expandValue(v, mapping, resolve) if err != nil { return nil, err } @@ -732,14 +822,18 @@ func expandCredentials( // expandValue recursively expands ${VAR} in string values, map values, and // slice elements, leaving other types untouched. -func expandValue(v any, env map[string]string, resolve bool) (any, error) { +func expandValue( + v any, + mapping func(string) string, + resolve bool, +) (any, error) { switch val := v.(type) { case string: - return maybeExpand(val, env, resolve) + return maybeExpand(val, mapping, resolve) case map[string]any: out := make(map[string]any, len(val)) for k, inner := range val { - expanded, err := expandValue(inner, env, resolve) + expanded, err := expandValue(inner, mapping, resolve) if err != nil { return nil, err } @@ -749,7 +843,7 @@ func expandValue(v any, env map[string]string, resolve bool) (any, error) { case []any: out := make([]any, len(val)) for i, inner := range val { - expanded, err := expandValue(inner, env, resolve) + expanded, err := expandValue(inner, mapping, resolve) if err != nil { return nil, err } @@ -765,7 +859,7 @@ func expandValue(v any, env map[string]string, resolve bool) (any, error) { // A nil map returns nil so the connection omits metadata entirely. func expandMetadata( metadata map[string]string, - env map[string]string, + mapping func(string) string, resolve bool, ) (map[string]string, error) { if metadata == nil { @@ -773,7 +867,7 @@ func expandMetadata( } out := make(map[string]string, len(metadata)) for k, v := range metadata { - expanded, err := maybeExpand(v, env, resolve) + expanded, err := maybeExpand(v, mapping, resolve) if err != nil { return nil, err } diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer_test.go index d505c04da2e..72ae2c39b1d 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer_test.go @@ -743,6 +743,7 @@ services: conns, err := BrownfieldConnections( []byte(yaml), map[string]string{"SEARCH_API_KEY": "secret"}, + nil, "", ) require.NoError(t, err) @@ -760,13 +761,18 @@ services: host: azure.ai.project endpoint: https://existing.services.ai.azure.com/api/projects/p1 ` - conns, err := BrownfieldConnections([]byte(noConns), nil, "") + conns, err := BrownfieldConnections( + []byte(noConns), + nil, + nil, + "", + ) require.NoError(t, err) assert.Empty(t, conns) }) t.Run("empty raw errors", func(t *testing.T) { - _, err := BrownfieldConnections(nil, nil, "") + _, err := BrownfieldConnections(nil, nil, nil, "") require.Error(t, err) }) @@ -796,6 +802,7 @@ credentials: connections, err := BrownfieldConnections( raw, map[string]string{"SEARCH_KEY": "secret"}, + nil, root, ) diff --git a/cli/azd/extensions/azure.ai.agents/schemas/azure.ai.agent.json b/cli/azd/extensions/azure.ai.agents/schemas/azure.ai.agent.json index 1e362c72ed7..9376b3a90ba 100644 --- a/cli/azd/extensions/azure.ai.agents/schemas/azure.ai.agent.json +++ b/cli/azd/extensions/azure.ai.agents/schemas/azure.ai.agent.json @@ -4,13 +4,6 @@ "description": "Custom configuration for the Azure AI Agent Service target", "type": "object", "properties": { - "env": { - "type": "object", - "description": "Environment variables as key-value pairs", - "additionalProperties": { - "type": "string" - } - }, "container": { "$ref": "#/definitions/ContainerSettings" }, diff --git a/cli/azd/extensions/azure.ai.agents/schemas/examples/complex.azure.yaml b/cli/azd/extensions/azure.ai.agents/schemas/examples/complex.azure.yaml index 1f299dd05bb..b4165873bb3 100644 --- a/cli/azd/extensions/azure.ai.agents/schemas/examples/complex.azure.yaml +++ b/cli/azd/extensions/azure.ai.agents/schemas/examples/complex.azure.yaml @@ -46,6 +46,8 @@ services: host: azure.ai.connection uses: - ai-project + env: + SEARCH_API_KEY: ${SEARCH_API_KEY} category: CognitiveSearch target: https://my-search.search.windows.net authType: ApiKey @@ -108,7 +110,8 @@ services: - research-tools env: LOG_LEVEL: info - MODEL_ENDPOINT: ${{project.endpoint}} + # The extra $ preserves this Foundry expression through azd. + MODEL_ENDPOINT: $${{project.endpoint}} protocols: - protocol: a2a version: "0.2" @@ -143,6 +146,8 @@ services: host: azure.ai.routine uses: - researcher + env: + DIGEST_TOPIC: ${DIGEST_TOPIC} description: Summarize the day's documents every night. triggers: default: diff --git a/cli/azd/extensions/azure.ai.agents/schemas/examples/simple.azure.yaml b/cli/azd/extensions/azure.ai.agents/schemas/examples/simple.azure.yaml index 38197ca2346..6c51d4ab962 100644 --- a/cli/azd/extensions/azure.ai.agents/schemas/examples/simple.azure.yaml +++ b/cli/azd/extensions/azure.ai.agents/schemas/examples/simple.azure.yaml @@ -25,3 +25,5 @@ services: kind: hosted name: assistant description: A simple assistant. + env: + LOG_LEVEL: info diff --git a/cli/azd/extensions/azure.ai.connections/extension.yaml b/cli/azd/extensions/azure.ai.connections/extension.yaml index da3acfbc434..0c0dd40451c 100644 --- a/cli/azd/extensions/azure.ai.connections/extension.yaml +++ b/cli/azd/extensions/azure.ai.connections/extension.yaml @@ -17,4 +17,4 @@ tags: - connection usage: azd ai connection [options] version: 1.0.0-beta.3 -requiredAzdVersion: ">=1.27.0" +requiredAzdVersion: ">=1.27.1" diff --git a/cli/azd/extensions/azure.ai.connections/schemas/azure.ai.connection.json b/cli/azd/extensions/azure.ai.connections/schemas/azure.ai.connection.json index f51ea03067e..831ee68087f 100644 --- a/cli/azd/extensions/azure.ai.connections/schemas/azure.ai.connection.json +++ b/cli/azd/extensions/azure.ai.connections/schemas/azure.ai.connection.json @@ -12,7 +12,7 @@ }, "target": { "type": "string", - "description": "Target endpoint URL or ARM resource ID. May contain ${VAR} (azd env, resolved client-side)." + "description": "Target endpoint URL or ARM resource ID. May contain ${VAR}; declare each referenced variable in the service-level env object." }, "authType": { "type": "string", @@ -38,7 +38,7 @@ }, "credentials": { "type": "object", - "description": "Credentials. Values may contain ${VAR} (azd env, resolved client-side) or ${{...}} (Foundry server-side resolution, passed through untouched).", + "description": "Credentials. Use ${VAR} for azd service env substitution or ${{...}} for Foundry server-side resolution. In a service env value, use $${{...}} so azd forwards the Foundry expression unchanged.", "additionalProperties": true }, "metadata": { diff --git a/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider.go b/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider.go index b34a97ba7fd..ea81f055ccc 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider.go +++ b/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider.go @@ -10,6 +10,7 @@ import ( "fmt" "hash/fnv" "log" + "maps" "net/url" "os" "path/filepath" @@ -68,19 +69,21 @@ type FoundryProvisioningProvider struct { azdClient *azdext.AzdClient // Populated by Initialize. - projectPath string - synthResult *synthesis.Result // nil when onDiskSource != nil - envName string - subID string - location string - rgName string - rgExplicit bool // AZURE_RESOURCE_GROUP came from env, not the rg- default - foundryName string - principalID string - credential azcore.TokenCredential - tenantID string // resolved lazily by ensureCredential; surfaced as AZURE_TENANT_ID - armTemplate map[string]any // embedded ARM JSON; nil when onDiskSource is set - onDiskSource *templateSource // non-nil when ./infra/main.{bicep,bicepparam} exists + projectPath string + synthResult *synthesis.Result // nil when onDiskSource != nil + serviceEnvironments map[string]map[string]string + connectionEnvironmentScopes map[string]bool + envName string + subID string + location string + rgName string + rgExplicit bool // AZURE_RESOURCE_GROUP came from env, not the rg- default + foundryName string + principalID string + credential azcore.TokenCredential + tenantID string // resolved lazily by ensureCredential; surfaced as AZURE_TENANT_ID + armTemplate map[string]any // embedded ARM JSON; nil when onDiskSource is set + onDiskSource *templateSource // non-nil when ./infra/main.{bicep,bicepparam} exists // brownfieldEndpoint is the existing project endpoint when the foundry // service sets endpoint: (bring-your-own). When non-empty the provider skips @@ -127,12 +130,11 @@ func NewFoundryProvisioningProvider(azdClient *azdext.AzdClient) azdext.Provisio // and the on-disk Bicep path, and resolves required env values. It rejects // brownfield (endpoint:) and missing services with structured errors. // -// Initialize is cheap by contract: it does no network I/O and builds no -// credential. Tenant lookup and credential construction happen lazily in -// [FoundryProvisioningProvider.ensureCredential]; the bicep CLI is built -// only when an on-disk template actually needs compiling. azd-core may -// call Initialize on providers it never deploys with, so keeping it cheap -// lets pure metadata calls (Parameters, PlannedOutputs) succeed without auth. +// Initialize is cheap and performs no Azure network I/O. +// Credentials are created lazily by ensureCredential. +// The bicep CLI is built only when an on-disk template needs it. +// azd-core may initialize providers it never deploys with, so this +// keeps metadata calls unauthenticated. func (p *FoundryProvisioningProvider) Initialize( ctx context.Context, projectPath string, @@ -169,27 +171,76 @@ func (p *FoundryProvisioningProvider) Initialize( return err } + // endpoint: (brownfield) reuse connects to an existing project, + // so it needs no subscription or location. Detect it up front so + // the environment can be resolved before any service values are + // read. + endpoint, err := foundryServiceEndpointAtRoot(rawYAML, projectPath, svcName) + if err != nil { + return exterrors.Validation( + exterrors.CodeInvalidAzureYaml, + fmt.Sprintf( + "read Foundry project service configuration: %s", + err, + ), + "fix the project service configuration in azure.yaml", + ) + } + + onDisk := p.onDiskTemplatePresent() + if !onDisk { + // Validate embedded config before any interactive prompts. + _, validationErr := synthesis.Synthesize(synthesis.Input{ + RawAzureYAML: rawYAML, + ServiceName: svcName, + AcceptedHosts: FoundryProvisioningServiceHosts, + PreserveVarRefs: true, + ProjectRoot: projectPath, + }) + if validationErr != nil && + !errors.Is(validationErr, synthesis.ErrEndpointBrownfield) { + return foundrySynthesisError(svcName, validationErr) + } + } + + p.connectionEnvironmentScopes, err = + synthesis.ConnectionEnvironmentScopes(rawYAML, projectPath) + if err != nil { + return exterrors.Validation( + exterrors.CodeInvalidAzureYaml, + fmt.Sprintf( + "read Foundry connection service configuration: %s", + err, + ), + "fix the connection service configuration in azure.yaml", + ) + } + + // Resolve the environment before reading service values. azd core + // expands ${VAR} in service env against the environment, so + // reading them first would capture empty strings for values the + // user is about to be prompted for, and connection synthesis + // would provision those empty strings. + if endpoint != "" { + err = p.resolveEnvName(ctx) + } else { + err = p.resolveEnv(ctx) + } + if err != nil { + return err + } + + p.serviceEnvironments, err = p.projectServiceEnvironments(ctx) + if err != nil { + return err + } + // Detect on-disk Bicep before synthesizing. Stat-only; no compile here. - if p.onDiskTemplatePresent() { + if onDisk { log.Printf("[debug] foundry provider: on-disk Bicep detected under %s; "+ "skipping synthesizer", filepath.Join(projectPath, onDiskInfraDir)) // endpoint: (brownfield) reuse skips provisioning even on the on-disk // path; connect to the existing project instead of compiling Bicep. - endpoint, endpointErr := foundryServiceEndpointAtRoot( - rawYAML, - projectPath, - svcName, - ) - if endpointErr != nil { - return exterrors.Validation( - exterrors.CodeInvalidAzureYaml, - fmt.Sprintf( - "resolve existing Foundry project endpoint: %s", - endpointErr, - ), - "fix the project service configuration in azure.yaml", - ) - } if endpoint != "" { if err := warnNetworkIgnoredInBrownfield( rawYAML, @@ -203,20 +254,18 @@ func (p *FoundryProvisioningProvider) Initialize( ) } p.brownfieldEndpoint = endpoint - if err := p.captureBrownfieldDeployments(ctx, rawYAML, svcName); err != nil { - return err - } - return p.resolveEnvName(ctx) + return p.captureBrownfieldDeployments(ctx, rawYAML, svcName) } - return p.resolveEnv(ctx) + return nil } res, err := synthesis.Synthesize(synthesis.Input{ - RawAzureYAML: rawYAML, - ServiceName: svcName, - AcceptedHosts: FoundryProvisioningServiceHosts, - Env: p.networkEnvMap(ctx), - ProjectRoot: projectPath, + RawAzureYAML: rawYAML, + ServiceName: svcName, + AcceptedHosts: FoundryProvisioningServiceHosts, + Env: p.networkEnvMap(ctx), + ServiceEnvironments: p.serviceEnvironments, + ProjectRoot: projectPath, }) switch { case errors.Is(err, synthesis.ErrEndpointBrownfield): @@ -233,38 +282,10 @@ func (p *FoundryProvisioningProvider) Initialize( "fix the project service configuration in azure.yaml", ) } - endpoint, endpointErr := foundryServiceEndpointAtRoot( - rawYAML, - projectPath, - svcName, - ) - if endpointErr != nil { - return exterrors.Validation( - exterrors.CodeInvalidAzureYaml, - fmt.Sprintf( - "resolve existing Foundry project endpoint: %s", - endpointErr, - ), - "fix the project service configuration in azure.yaml", - ) - } p.brownfieldEndpoint = endpoint - if err := p.captureBrownfieldDeployments(ctx, rawYAML, svcName); err != nil { - return err - } - return p.resolveEnvName(ctx) - case errors.Is(err, synthesis.ErrServiceNotFound): - return exterrors.Dependency( - exterrors.CodeProvisioningServiceNotFound, - fmt.Sprintf("no service in azure.yaml has host in %v", FoundryProjectServiceHosts), - fmt.Sprintf("add a service with `host: %s` to azure.yaml", FoundryProjectHost), - ) + return p.captureBrownfieldDeployments(ctx, rawYAML, svcName) case err != nil: - return exterrors.Validation( - exterrors.CodeInvalidAzureYaml, - fmt.Sprintf("synthesize foundry project service %q: %s", svcName, err), - "check the endpoint, deployments, and network fields under your azure.ai.project service", - ) + return foundrySynthesisError(svcName, err) } p.synthResult = res @@ -284,7 +305,33 @@ func (p *FoundryProvisioningProvider) Initialize( } p.armTemplate = tmpl - return p.resolveEnv(ctx) + return nil +} + +func foundrySynthesisError(serviceName string, err error) error { + if errors.Is(err, synthesis.ErrServiceNotFound) { + return exterrors.Dependency( + exterrors.CodeProvisioningServiceNotFound, + fmt.Sprintf( + "no service in azure.yaml has host in %v", + FoundryProjectServiceHosts, + ), + fmt.Sprintf( + "add a service with `host: %s` to azure.yaml", + FoundryProjectHost, + ), + ) + } + return exterrors.Validation( + exterrors.CodeInvalidAzureYaml, + fmt.Sprintf( + "synthesize foundry project service %q: %s", + serviceName, + err, + ), + "check the endpoint, deployments, and network fields "+ + "under your azure.ai.project service", + ) } // networkEnvMap returns a best-effort name -> value map of the azd environment @@ -296,6 +343,7 @@ func (p *FoundryProvisioningProvider) networkEnvMap(ctx context.Context) map[str log.Printf("[debug] foundry provider: no azd client; network ${VAR} uses process env only") return nil } + envClient := p.azdClient.Environment() if envClient == nil { log.Printf("[debug] foundry provider: no environment client; network ${VAR} uses process env only") @@ -321,6 +369,46 @@ func (p *FoundryProvisioningProvider) networkEnvMap(ctx context.Context) map[str return out } +// projectServiceEnvironments reads core-expanded service values. +// It keeps service scopes separate for connection synthesis. +func (p *FoundryProvisioningProvider) projectServiceEnvironments( + ctx context.Context, +) (map[string]map[string]string, error) { + if p.azdClient == nil { + return nil, exterrors.Dependency( + exterrors.CodeAzdClientFailed, + "read project service environments: azd client is unavailable", + "restart azd and retry", + ) + } + + response, err := p.azdClient.Project().Get( + ctx, + &azdext.EmptyRequest{}, + ) + if err != nil { + return nil, exterrors.Dependency( + exterrors.CodeAzdClientFailed, + fmt.Sprintf("read project service environments: %s", err), + "verify the azd project is accessible, then retry", + ) + } + if response.GetProject() == nil { + return nil, exterrors.Internal( + exterrors.CodeInvalidServiceConfig, + "read project service environments: project is missing", + ) + } + + environments := map[string]map[string]string{} + for name, service := range response.GetProject().GetServices() { + if len(service.GetEnvironment()) > 0 { + environments[name] = maps.Clone(service.GetEnvironment()) + } + } + return environments, nil +} + // warnNetworkIgnoredInBrownfield logs a warning when a service declares both // endpoint: (brownfield) and network:. The account's network posture is fixed // by whoever created it, so the network: block has no effect. @@ -838,6 +926,7 @@ func (p *FoundryProvisioningProvider) captureBrownfieldDeployments( connections, err := synthesis.BrownfieldConnections( rawYAML, p.networkEnvMap(ctx), + p.serviceEnvironments, p.projectPath, ) if err != nil { @@ -1203,7 +1292,16 @@ func (p *FoundryProvisioningProvider) resolveTemplate( ) (*templateSource, error) { if p.onDiskSource == nil && p.onDiskTemplatePresent() { progress("Compiling on-disk Bicep templates...") - src, err := loadOnDiskTemplate(ctx, p.projectPath, p.bicepCli(), p.envValues(ctx)) + src, err := loadOnDiskTemplateWithEnvironment( + ctx, + p.projectPath, + p.bicepCli(), + onDiskEnvironment{ + project: p.envValues(ctx), + services: p.serviceEnvironments, + scopedConnections: p.connectionEnvironmentScopes, + }, + ) if err != nil { return nil, err } diff --git a/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider_resolveenv_test.go b/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider_resolveenv_test.go index de9c5c55dcb..3e8204685d0 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider_resolveenv_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider_resolveenv_test.go @@ -6,9 +6,12 @@ package provisioning import ( "context" "net" + "os" + "path/filepath" "testing" "azure.ai.projects/internal/exterrors" + "azure.ai.projects/internal/synthesis" "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/stretchr/testify/assert" @@ -306,3 +309,209 @@ func TestResolveEnv_EmptyLocationResponseReturnsError(t *testing.T) { assert.Equal(t, exterrors.CodeMissingAzureLocation, local.Code) assert.Empty(t, env.set, "an empty location name must not be persisted") } + +// promptOrderStubProjectServer models azd core expanding ${VAR} in a +// service env at call time: the connection endpoint reads as empty +// until the prompted location has been persisted to the azd +// environment. +type promptOrderStubProjectServer struct { + azdext.UnimplementedProjectServiceServer + projectPath string + env *resolveEnvStubEnvServer +} + +func (s *promptOrderStubProjectServer) Get( + context.Context, *azdext.EmptyRequest, +) (*azdext.GetProjectResponse, error) { + endpoint := "" + if location := s.env.set[envKeyLocation]; location != "" { + endpoint = "https://search." + location + ".example" + } + return &azdext.GetProjectResponse{Project: &azdext.ProjectConfig{ + Path: s.projectPath, + Services: map[string]*azdext.ServiceConfig{ + "connection": { + Environment: map[string]string{"ENDPOINT": endpoint}, + }, + }, + }}, nil +} + +// newPromptOrderTestClient serves the project, environment and prompt +// stubs needed to exercise Initialize end to end. +func newPromptOrderTestClient( + t *testing.T, + projSrv azdext.ProjectServiceServer, + envSrv azdext.EnvironmentServiceServer, + promptSrv azdext.PromptServiceServer, +) *azdext.AzdClient { + t.Helper() + + srv := grpc.NewServer() + azdext.RegisterProjectServiceServer(srv, projSrv) + azdext.RegisterEnvironmentServiceServer(srv, envSrv) + azdext.RegisterPromptServiceServer(srv, promptSrv) + + lis, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + go func() { _ = srv.Serve(lis) }() + t.Cleanup(func() { + srv.Stop() + _ = lis.Close() + }) + + client, err := azdext.NewAzdClient(azdext.WithAddress(lis.Addr().String())) + require.NoError(t, err) + t.Cleanup(func() { client.Close() }) + + return client +} + +func TestInitializeResolvesEnvBeforeReadingServiceEnvironments(t *testing.T) { + // Greenfield: neither AZURE_SUBSCRIPTION_ID nor AZURE_LOCATION is + // set, so Initialize must prompt first. Reading service + // environments before the prompt would synthesize the connection + // with an empty target. + projectPath := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(projectPath, "azure.yaml"), + []byte(` +services: + project: + host: azure.ai.project + connection: + host: azure.ai.connection + uses: [project] + env: + ENDPOINT: ${SEARCH_ENDPOINT} + category: CognitiveSearch + target: ${ENDPOINT} + authType: None +`), + 0o600, + )) + + env := &resolveEnvStubEnvServer{envName: "test", get: map[string]string{}} + prompt := &resolveEnvStubPromptServer{ + subscriptionID: "00000000-0000-0000-0000-000000000001", + location: "westus2", + } + client := newPromptOrderTestClient( + t, + &promptOrderStubProjectServer{projectPath: projectPath, env: env}, + env, + prompt, + ) + provider := &FoundryProvisioningProvider{azdClient: client} + + err := provider.Initialize( + t.Context(), + projectPath, + &azdext.ProvisioningOptions{Provider: FoundryProviderName}, + ) + require.NoError(t, err) + assert.Equal(t, 1, prompt.subscriptionN) + assert.Equal(t, 1, prompt.locationN) + + require.NotNil(t, provider.synthResult) + connections, ok := provider.synthResult.Parameters["connections"].([]synthesis.Connection) + require.True(t, ok) + require.Len(t, connections, 1) + assert.Equal(t, "https://search.westus2.example", connections[0].Target) +} + +func TestInitializeValidatesConfigBeforePrompting(t *testing.T) { + tests := []struct { + name string + config string + wantErr string + }{ + { + name: "invalid deployments", + config: " deployments: invalid\n", + wantErr: "decode service", + }, + { + name: "invalid network", + config: " network:\n" + + " peSubnet: {vnet: not-an-arm-id, name: pe}\n", + wantErr: "not a well-formed", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + projectPath := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(projectPath, "azure.yaml"), + []byte("services:\n"+ + " project:\n"+ + " host: azure.ai.project\n"+ + tt.config), + 0o600, + )) + + env := &resolveEnvStubEnvServer{ + envName: "test", + get: map[string]string{}, + } + prompt := &resolveEnvStubPromptServer{ + subscriptionID: "sub-id", + location: "westus2", + } + client := newResolveEnvTestClient(t, env, prompt) + provider := &FoundryProvisioningProvider{ + azdClient: client, + } + + err := provider.Initialize( + t.Context(), + projectPath, + &azdext.ProvisioningOptions{ + Provider: FoundryProviderName, + }, + ) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + assert.Zero(t, prompt.subscriptionN) + assert.Zero(t, prompt.locationN) + }) + } +} + +func TestInitializeProjectRefErrorUsesGenericMessage(t *testing.T) { + projectPath := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(projectPath, "azure.yaml"), + []byte(`services: + project: + host: azure.ai.project + $ref: missing.yaml +`), + 0o600, + )) + + env := &resolveEnvStubEnvServer{ + envName: "test", + get: map[string]string{}, + } + prompt := &resolveEnvStubPromptServer{} + client := newResolveEnvTestClient(t, env, prompt) + provider := &FoundryProvisioningProvider{azdClient: client} + + err := provider.Initialize( + t.Context(), + projectPath, + &azdext.ProvisioningOptions{Provider: FoundryProviderName}, + ) + require.Error(t, err) + assert.Contains( + t, + err.Error(), + "read Foundry project service configuration", + ) + assert.NotContains(t, err.Error(), "existing Foundry project endpoint") + assert.Zero(t, prompt.subscriptionN) + assert.Zero(t, prompt.locationN) +} diff --git a/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider_test.go b/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider_test.go index 4eb58ccd033..be057ebd9db 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider_test.go @@ -18,6 +18,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cognitiveservices/armcognitiveservices/v2" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/azure/azure-dev/cli/azd/pkg/tools/bicep" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -183,6 +184,196 @@ func TestFoundryProvider_ImplementsContract(t *testing.T) { assert.NotNil(t, p) } +func TestProjectServiceEnvironments(t *testing.T) { + t.Parallel() + + projectServer := &validateStubProjectServer{ + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "connection": { + Environment: map[string]string{ + "ENDPOINT": "https://service.example", + }, + }, + "legacy": {}, + }, + }, + } + client := newValidateTestClient( + t, + projectServer, + &validateStubEnvServer{}, + ) + provider := &FoundryProvisioningProvider{azdClient: client} + + environments, err := provider.projectServiceEnvironments(t.Context()) + require.NoError(t, err) + require.Equal( + t, + map[string]map[string]string{ + "connection": { + "ENDPOINT": "https://service.example", + }, + }, + environments, + ) +} + +func TestInitializeUsesConnectionServiceEnvironment(t *testing.T) { + t.Parallel() + + projectPath := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(projectPath, "azure.yaml"), + []byte(` +services: + project: + host: azure.ai.project + connection: + host: azure.ai.connection + uses: [project] + env: + ENDPOINT: ${SEARCH_ENDPOINT} + category: CognitiveSearch + target: ${ENDPOINT} + authType: None +`), + 0o600, + )) + + projectServer := &validateStubProjectServer{ + project: &azdext.ProjectConfig{ + Path: projectPath, + Services: map[string]*azdext.ServiceConfig{ + "connection": { + Environment: map[string]string{ + "ENDPOINT": "https://service.example", + }, + }, + }, + }, + } + client := newValidateTestClient( + t, + projectServer, + &validateStubEnvServer{ + envName: "test", + get: map[string]string{ + envKeySubscriptionID: "00000000-0000-0000-0000-000000000000", + envKeyLocation: "eastus", + }, + }, + ) + provider := &FoundryProvisioningProvider{azdClient: client} + + err := provider.Initialize( + t.Context(), + projectPath, + &azdext.ProvisioningOptions{Provider: FoundryProviderName}, + ) + require.NoError(t, err) + require.NotNil(t, provider.synthResult) + connections, ok := provider.synthResult.Parameters["connections"].([]synthesis.Connection) + require.True(t, ok) + require.Len(t, connections, 1) + require.Equal(t, "https://service.example", connections[0].Target) +} + +func TestResolveTemplateUsesOnDiskConnectionServiceEnvironment( + t *testing.T, +) { + t.Parallel() + + projectPath := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(projectPath, "azure.yaml"), + []byte(` +services: + project: + host: azure.ai.project + connection: + host: azure.ai.connection + env: + ENDPOINT: ${SEARCH_ENDPOINT} +`), + 0o600, + )) + infraDir := filepath.Join(projectPath, onDiskInfraDir) + require.NoError(t, os.MkdirAll(infraDir, 0o750)) + require.NoError(t, os.WriteFile( + filepath.Join(infraDir, onDiskBicepFile), + []byte("// bicep\n"), + 0o600, + )) + params := minimalARMParametersFile(t, map[string]any{ + "connections": []map[string]any{ + {"name": "connection", "target": "${ENDPOINT}"}, + }, + }) + require.NoError(t, os.WriteFile( + filepath.Join(infraDir, onDiskParamsFile), + []byte(params), + 0o600, + )) + + projectServer := &validateStubProjectServer{ + project: &azdext.ProjectConfig{ + Path: projectPath, + Services: map[string]*azdext.ServiceConfig{ + "connection": { + Environment: map[string]string{ + "ENDPOINT": "https://service.example", + }, + }, + }, + }, + } + client := newValidateTestClient( + t, + projectServer, + &validateStubEnvServer{ + envName: "test", + get: map[string]string{ + envKeySubscriptionID: "sub-id", + envKeyLocation: "eastus", + }, + }, + ) + provider := &FoundryProvisioningProvider{ + azdClient: client, + bicepCliInstance: &stubCompiler{ + buildResult: bicep.BuildResult{ + Compiled: minimalARMTemplate(), + }, + }, + } + + require.NoError(t, provider.Initialize( + t.Context(), + projectPath, + &azdext.ProvisioningOptions{Provider: FoundryProviderName}, + )) + source, err := provider.resolveTemplate( + t.Context(), + func(string) {}, + ) + require.NoError(t, err) + + connectionEntry, ok := + source.parameters["connections"].(map[string]any) + require.True(t, ok) + connections, ok := connectionEntry["value"].([]any) + require.True(t, ok) + require.Len(t, connections, 1) + connection, ok := connections[0].(map[string]any) + require.True(t, ok) + assert.Equal( + t, + "https://service.example", + connection["target"], + ) +} + func TestArmOutputsToProto(t *testing.T) { tests := []struct { name string diff --git a/cli/azd/extensions/azure.ai.projects/internal/provisioning/ondisk_template.go b/cli/azd/extensions/azure.ai.projects/internal/provisioning/ondisk_template.go index d291aec028d..cc8354eb863 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/provisioning/ondisk_template.go +++ b/cli/azd/extensions/azure.ai.projects/internal/provisioning/ondisk_template.go @@ -68,6 +68,21 @@ type bicepCompiler interface { BuildBicepParam(ctx context.Context, file string, env []string) (bicep.BuildResult, error) } +// onDiskEnvironment keeps connection service scopes separate. +// Project values remain the fallback for legacy connections. +type onDiskEnvironment struct { + project map[string]string + services map[string]map[string]string + scopedConnections map[string]bool +} + +func (e onDiskEnvironment) connection(name string) map[string]string { + if e.scopedConnections[name] { + return e.services[name] + } + return e.project +} + // loadOnDiskTemplate compiles the on-disk Bicep source (if any) and returns // a fully-resolved templateSource. Returns (nil, nil) -- not an error -- when // no on-disk template is found, so the caller falls back to the embedded path. @@ -85,6 +100,20 @@ func loadOnDiskTemplate( projectPath string, compiler bicepCompiler, envValues map[string]string, +) (*templateSource, error) { + return loadOnDiskTemplateWithEnvironment( + ctx, + projectPath, + compiler, + onDiskEnvironment{project: envValues}, + ) +} + +func loadOnDiskTemplateWithEnvironment( + ctx context.Context, + projectPath string, + compiler bicepCompiler, + environment onDiskEnvironment, ) (*templateSource, error) { infraDir := filepath.Join(projectPath, onDiskInfraDir) bicepparamPath := filepath.Join(infraDir, onDiskBicepParamFile) @@ -92,10 +121,21 @@ func loadOnDiskTemplate( switch { case fileExistsAt(bicepparamPath): - return loadFromBicepParam(ctx, bicepparamPath, compiler, envValues) + return loadFromBicepParam( + ctx, + bicepparamPath, + compiler, + environment.project, + ) case fileExistsAt(bicepPath): paramsPath := filepath.Join(infraDir, onDiskParamsFile) - return loadFromBicep(ctx, bicepPath, paramsPath, compiler, envValues) + return loadFromBicep( + ctx, + bicepPath, + paramsPath, + compiler, + environment, + ) default: return nil, nil } @@ -108,7 +148,7 @@ func loadFromBicep( ctx context.Context, bicepPath, paramsPath string, compiler bicepCompiler, - envValues map[string]string, + environment onDiskEnvironment, ) (*templateSource, error) { res, err := compiler.Build(ctx, bicepPath) if err != nil { @@ -124,7 +164,10 @@ func loadFromBicep( return nil, err } - params, err := loadParametersFile(paramsPath, envValues) + params, err := loadParametersFileWithEnvironment( + paramsPath, + environment, + ) if err != nil { return nil, err } @@ -199,6 +242,16 @@ func loadFromBicepParam( // Each parameter is substituted in isolation so one unresolved VAR doesn't // affect siblings. func loadParametersFile(paramFilePath string, envValues map[string]string) (map[string]any, error) { + return loadParametersFileWithEnvironment( + paramFilePath, + onDiskEnvironment{project: envValues}, + ) +} + +func loadParametersFileWithEnvironment( + paramFilePath string, + environment onDiskEnvironment, +) (map[string]any, error) { //nolint:gosec // paramFilePath is derived from projectPath supplied by azd-core raw, err := os.ReadFile(paramFilePath) if err != nil { @@ -220,7 +273,12 @@ func loadParametersFile(paramFilePath string, envValues map[string]string) (map[ out := make(map[string]any, len(pre)) for name, raw := range pre { - kept, err := substituteParamValue(raw, paramFilePath, name, envValues) + kept, err := substituteParameterValue( + raw, + paramFilePath, + name, + environment, + ) if err != nil { return nil, err } @@ -232,6 +290,126 @@ func loadParametersFile(paramFilePath string, envValues map[string]string) (map[ return out, nil } +func substituteParameterValue( + rawEntry any, + sourcePath, name string, + environment onDiskEnvironment, +) (any, error) { + switch name { + case "connections": + return substituteConnectionsParameter( + rawEntry, + sourcePath, + name, + environment, + ) + case "connectionCredentials": + return substituteConnectionCredentialsParameter( + rawEntry, + sourcePath, + name, + environment, + ) + default: + return substituteParamValue( + rawEntry, + sourcePath, + name, + environment.project, + ) + } +} + +func substituteConnectionsParameter( + rawEntry any, + sourcePath, name string, + environment onDiskEnvironment, +) (any, error) { + entry, ok := rawEntry.(map[string]any) + if !ok { + return substituteParamValue( + rawEntry, + sourcePath, + name, + environment.project, + ) + } + connections, ok := entry["value"].([]any) + if !ok { + return substituteParamValue( + rawEntry, + sourcePath, + name, + environment.project, + ) + } + + resolvedConnections := make([]any, len(connections)) + for i, connection := range connections { + connectionName := "" + if fields, ok := connection.(map[string]any); ok { + connectionName, _ = fields["name"].(string) + } + resolved, _, err := substituteJSONValue( + connection, + sourcePath, + name, + environment.connection(connectionName), + ) + if err != nil { + return nil, err + } + resolvedConnections[i] = resolved + } + + resolvedEntry := maps.Clone(entry) + resolvedEntry["value"] = resolvedConnections + return resolvedEntry, nil +} + +func substituteConnectionCredentialsParameter( + rawEntry any, + sourcePath, name string, + environment onDiskEnvironment, +) (any, error) { + entry, ok := rawEntry.(map[string]any) + if !ok { + return substituteParamValue( + rawEntry, + sourcePath, + name, + environment.project, + ) + } + credentials, ok := entry["value"].(map[string]any) + if !ok { + return substituteParamValue( + rawEntry, + sourcePath, + name, + environment.project, + ) + } + + resolvedCredentials := make(map[string]any, len(credentials)) + for connectionName, credential := range credentials { + resolved, _, err := substituteJSONValue( + credential, + sourcePath, + name, + environment.connection(connectionName), + ) + if err != nil { + return nil, err + } + resolvedCredentials[connectionName] = resolved + } + + resolvedEntry := maps.Clone(entry) + resolvedEntry["value"] = resolvedCredentials + return resolvedEntry, nil +} + // substituteParamValue runs envsubst over the JSON encoding of one parameter // entry. Returns nil when the entry should be dropped (string value collapsed // to "" AND at least one referenced VAR was unset). @@ -240,9 +418,36 @@ func substituteParamValue( sourcePath, name string, envValues map[string]string, ) (any, error) { + resolved, hasUnsetEnvVar, err := substituteJSONValue( + rawEntry, + sourcePath, + name, + envValues, + ) + if err != nil { + return nil, err + } + + // Drop strings that unresolved variables reduce to empty. + // Non-string values are always kept. + if entry, ok := resolved.(map[string]any); ok { + if val, ok := entry["value"]; ok { + if str, ok := val.(string); ok && str == "" && hasUnsetEnvVar { + return nil, nil + } + } + } + return resolved, nil +} + +func substituteJSONValue( + rawEntry any, + sourcePath, name string, + envValues map[string]string, +) (any, bool, error) { enc, err := json.Marshal(rawEntry) if err != nil { - return nil, exterrors.Internal( + return nil, false, exterrors.Internal( exterrors.CodeOnDiskParametersInvalid, fmt.Sprintf("re-encode parameter %q in %s: %s", name, sourcePath, err), ) @@ -266,7 +471,7 @@ func substituteParamValue( return string(escaped[1 : len(escaped)-1]) }) if err != nil { - return nil, exterrors.Validation( + return nil, false, exterrors.Validation( exterrors.CodeOnDiskParametersInvalid, fmt.Sprintf("substitute env vars in parameter %q of %s: %s", name, sourcePath, err), "check for malformed ${VAR} references in the parameters file", @@ -275,23 +480,13 @@ func substituteParamValue( var resolved any if err := json.Unmarshal([]byte(substituted), &resolved); err != nil { - return nil, exterrors.Validation( + return nil, false, exterrors.Validation( exterrors.CodeOnDiskParametersInvalid, fmt.Sprintf("parse parameter %q in %s after substitution: %s", name, sourcePath, err), "ensure the substituted value is valid JSON", ) } - - // Drop string-valued parameters whose substituted value collapsed to "" - // because of an unresolved ${VAR}. Non-string values are always kept. - if entry, ok := resolved.(map[string]any); ok { - if val, ok := entry["value"]; ok { - if str, ok := val.(string); ok && str == "" && hasUnsetEnvVar { - return nil, nil - } - } - } - return resolved, nil + return resolved, hasUnsetEnvVar, nil } // extractParametersFromARMFile pulls the inner "parameters" map out of diff --git a/cli/azd/extensions/azure.ai.projects/internal/provisioning/ondisk_template_test.go b/cli/azd/extensions/azure.ai.projects/internal/provisioning/ondisk_template_test.go index 5f566b06531..b84094ea17b 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/provisioning/ondisk_template_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/provisioning/ondisk_template_test.go @@ -156,6 +156,103 @@ func TestLoadOnDiskTemplate_BicepWithParams(t *testing.T) { "parameters referencing an unresolved ${VAR} must be dropped, not set to empty string") } +func TestLoadOnDiskTemplate_ConnectionServiceScopes(t *testing.T) { + t.Parallel() + dir := t.TempDir() + infraDir := filepath.Join(dir, onDiskInfraDir) + require.NoError(t, os.MkdirAll(infraDir, 0o750)) + require.NoError(t, os.WriteFile( + filepath.Join(infraDir, onDiskBicepFile), + []byte("// bicep\n"), + 0o600, + )) + + params := minimalARMParametersFile(t, map[string]any{ + "connections": []map[string]any{ + {"name": "first", "target": "${ENDPOINT}"}, + {"name": "second", "target": "${ENDPOINT}"}, + {"name": "isolated", "target": "${ENDPOINT}"}, + {"name": "legacy", "target": "${ENDPOINT}"}, + }, + "connectionCredentials": map[string]any{ + "first": map[string]any{"key": "${KEY}"}, + "second": map[string]any{"key": "${KEY}"}, + "isolated": map[string]any{"key": "${KEY}"}, + "legacy": map[string]any{"key": "${KEY}"}, + }, + }) + require.NoError(t, os.WriteFile( + filepath.Join(infraDir, onDiskParamsFile), + []byte(params), + 0o600, + )) + + stub := &stubCompiler{ + buildResult: bicep.BuildResult{Compiled: minimalARMTemplate()}, + } + got, err := loadOnDiskTemplateWithEnvironment( + t.Context(), + dir, + stub, + onDiskEnvironment{ + project: map[string]string{ + "ENDPOINT": "https://project.example", + "KEY": "project-key", + }, + services: map[string]map[string]string{ + "first": { + "ENDPOINT": "https://first.example", + "KEY": "first-key", + }, + "second": { + "ENDPOINT": "https://second.example", + "KEY": "second-key", + }, + }, + scopedConnections: map[string]bool{ + "first": true, + "second": true, + "isolated": true, + }, + }, + ) + require.NoError(t, err) + + asMap := func(value any) map[string]any { + t.Helper() + mapped, ok := value.(map[string]any) + require.True(t, ok, "expected map, got %T", value) + return mapped + } + connectionEntry := asMap(got.parameters["connections"]) + connections, ok := connectionEntry["value"].([]any) + require.True(t, ok) + require.Len(t, connections, 4) + assert.Equal( + t, + "https://first.example", + asMap(connections[0])["target"], + ) + assert.Equal( + t, + "https://second.example", + asMap(connections[1])["target"], + ) + assert.Equal(t, "", asMap(connections[2])["target"]) + assert.Equal( + t, + "https://project.example", + asMap(connections[3])["target"], + ) + + credentialEntry := asMap(got.parameters["connectionCredentials"]) + credentials := asMap(credentialEntry["value"]) + assert.Equal(t, "first-key", asMap(credentials["first"])["key"]) + assert.Equal(t, "second-key", asMap(credentials["second"])["key"]) + assert.Equal(t, "", asMap(credentials["isolated"])["key"]) + assert.Equal(t, "project-key", asMap(credentials["legacy"])["key"]) +} + func TestLoadOnDiskTemplate_BicepparamPrecedence(t *testing.T) { t.Parallel() dir := t.TempDir() 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..4e68a9bdb67 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go @@ -52,12 +52,16 @@ type Input struct { // value is not checked (only existence and endpoint: are). AcceptedHosts []string - // Env maps azd environment variable names to values. Used to resolve - // ${VAR} references in network fields (subnet vnet ids, dns.subscription). - // When a referenced variable is absent here, the synthesizer falls back - // to the process environment before failing. May be nil. + // Env maps project-wide azd values. + // Network fields always use it. + // Legacy connection services use it when service env is absent. + // Missing values may fall back to the process environment. Env map[string]string + // ServiceEnvironments contains core-expanded values by service. + // Connection fields prefer these values over the legacy Env map. + ServiceEnvironments map[string]map[string]string + // PreserveVarRefs keeps ${VAR} references verbatim instead of resolving // them. Used by the eject path, where the synthesized main.parameters.json // must stay environment-portable: the on-disk provision flow resolves @@ -291,6 +295,7 @@ func Synthesize(in Input) (*Result, error) { connections, err := collectConnections( root.Services, in.Env, + in.ServiceEnvironments, !in.PreserveVarRefs, in.ProjectRoot, ) @@ -322,6 +327,39 @@ func Synthesize(in Input) (*Result, error) { }, nil } +// ConnectionEnvironmentScopes returns services that declare env. +// An empty env block still establishes an isolated service scope. +func ConnectionEnvironmentScopes( + raw []byte, + projectRoot string, +) (map[string]bool, error) { + if len(raw) == 0 { + return nil, errors.New("synthesis: raw azure.yaml is empty") + } + + var root projectFile + if err := yaml.Unmarshal(raw, &root); err != nil { + return nil, fmt.Errorf("parse azure.yaml: %w", err) + } + + scopes := map[string]bool{} + for name, node := range root.Services { + node, matches, err := serviceForHost( + node, + projectRoot, + name, + aiConnectionHost, + ) + if err != nil { + return nil, err + } + if matches && connectionEnvDeclared(node) { + scopes[name] = true + } + } + return scopes, nil +} + // BrownfieldDeployments returns the model deployments declared on a brownfield // (endpoint:) Foundry project service. Synthesize short-circuits with // ErrEndpointBrownfield before reading deployments:, so the provider uses this @@ -362,6 +400,7 @@ func BrownfieldDeployments( func BrownfieldConnections( raw []byte, env map[string]string, + serviceEnvironments map[string]map[string]string, projectRoot string, ) ([]Connection, error) { if len(raw) == 0 { @@ -373,7 +412,13 @@ func BrownfieldConnections( return nil, fmt.Errorf("parse azure.yaml: %w", err) } - return collectConnections(root.Services, env, true, projectRoot) + return collectConnections( + root.Services, + env, + serviceEnvironments, + true, + projectRoot, + ) } // ProjectEndpoint returns the endpoint configured on a Foundry project service. @@ -629,12 +674,13 @@ func agentNeedsAcr(a agentBlock) bool { // (the service key is the connection name) and returns them sorted by name so // the synthesized parameter is deterministic regardless of YAML map order. // -// ${VAR} in target/credentials/metadata is expanded from env when resolve is -// true (provision path) and kept verbatim when false (eject path); Foundry -// ${{...}} expressions are always preserved, mirroring synthesizeNetwork. +// Provisioning resolves ${VAR} from service env when present. +// Legacy services use project and process values. +// Eject keeps references, and Foundry ${{...}} remains unchanged. func collectConnections( services map[string]yaml.Node, env map[string]string, + serviceEnvironments map[string]map[string]string, resolve bool, projectRoot string, ) ([]Connection, error) { @@ -660,17 +706,27 @@ func collectConnections( return nil, fmt.Errorf("services.%s: decode connection: %w", name, err) } - target, err := maybeExpand(svc.Target, env, resolve) + declared := len(serviceEnvironments[name]) > 0 || connectionEnvDeclared(node) + mapping := connectionEnvironmentMapping( + env, + serviceEnvironments[name], + declared, + ) + target, err := maybeExpand(svc.Target, mapping, resolve) if err != nil { return nil, fmt.Errorf("services.%s.target: %w", name, err) } - credentials, err := expandCredentials(svc.Credentials, env, resolve) + credentials, err := expandCredentials( + svc.Credentials, + mapping, + resolve, + ) if err != nil { return nil, fmt.Errorf("services.%s.credentials: %w", name, err) } - metadata, err := expandMetadata(svc.Metadata, env, resolve) + metadata, err := expandMetadata(svc.Metadata, mapping, resolve) if err != nil { return nil, fmt.Errorf("services.%s.metadata: %w", name, err) } @@ -691,20 +747,54 @@ func collectConnections( return connections, nil } +// connectionEnvDeclared reports whether the service node +// declares an env: key, including an empty env: {}. Core +// collapses an empty env to an omitted one, so the raw node +// is the only signal that a service opted into an isolated +// (possibly empty) scope. +func connectionEnvDeclared(node yaml.Node) bool { + var fields map[string]yaml.Node + if err := node.Decode(&fields); err != nil { + return false + } + _, ok := fields["env"] + return ok +} + +// Use scoped values when the service declares env. +// Legacy services use the project and process environments. +func connectionEnvironmentMapping( + env map[string]string, + serviceEnvironment map[string]string, + declared bool, +) func(string) string { + if declared { + return func(name string) string { + return serviceEnvironment[name] + } + } + + return func(name string) string { + if value, found := env[name]; found { + return value + } + value, _ := os.LookupEnv(name) + return value + } +} + // maybeExpand expands ${VAR} references in s when resolve is true, preserving // Foundry ${{...}} expressions; when resolve is false it returns s unchanged so // the eject path keeps references verbatim. -func maybeExpand(s string, env map[string]string, resolve bool) (string, error) { +func maybeExpand( + s string, + mapping func(string) string, + resolve bool, +) (string, error) { if !resolve || s == "" { return s, nil } - return foundry.ExpandEnv(s, func(name string) string { - if v, ok := env[name]; ok { - return v - } - v, _ := os.LookupEnv(name) - return v - }) + return foundry.ExpandEnv(s, mapping) } // expandCredentials deep-copies a credentials map, expanding ${VAR} in every @@ -713,7 +803,7 @@ func maybeExpand(s string, env map[string]string, resolve bool) (string, error) // credentials entirely (e.g. None / identity auth). func expandCredentials( creds map[string]any, - env map[string]string, + mapping func(string) string, resolve bool, ) (map[string]any, error) { if creds == nil { @@ -721,7 +811,7 @@ func expandCredentials( } out := make(map[string]any, len(creds)) for k, v := range creds { - expanded, err := expandValue(v, env, resolve) + expanded, err := expandValue(v, mapping, resolve) if err != nil { return nil, err } @@ -732,14 +822,18 @@ func expandCredentials( // expandValue recursively expands ${VAR} in string values, map values, and // slice elements, leaving other types untouched. -func expandValue(v any, env map[string]string, resolve bool) (any, error) { +func expandValue( + v any, + mapping func(string) string, + resolve bool, +) (any, error) { switch val := v.(type) { case string: - return maybeExpand(val, env, resolve) + return maybeExpand(val, mapping, resolve) case map[string]any: out := make(map[string]any, len(val)) for k, inner := range val { - expanded, err := expandValue(inner, env, resolve) + expanded, err := expandValue(inner, mapping, resolve) if err != nil { return nil, err } @@ -749,7 +843,7 @@ func expandValue(v any, env map[string]string, resolve bool) (any, error) { case []any: out := make([]any, len(val)) for i, inner := range val { - expanded, err := expandValue(inner, env, resolve) + expanded, err := expandValue(inner, mapping, resolve) if err != nil { return nil, err } @@ -765,7 +859,7 @@ func expandValue(v any, env map[string]string, resolve bool) (any, error) { // A nil map returns nil so the connection omits metadata entirely. func expandMetadata( metadata map[string]string, - env map[string]string, + mapping func(string) string, resolve bool, ) (map[string]string, error) { if metadata == nil { @@ -773,7 +867,7 @@ func expandMetadata( } out := make(map[string]string, len(metadata)) for k, v := range metadata { - expanded, err := maybeExpand(v, env, resolve) + expanded, err := maybeExpand(v, mapping, resolve) if err != nil { return nil, err } 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..297e2581004 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 @@ -543,6 +543,14 @@ services: require.Len(t, conns, 1) return conns[0] } + getKeys := func(t *testing.T, c Connection) map[string]any { + t.Helper() + value, found := c.Credentials["keys"] + require.True(t, found, "credentials should contain keys") + keys, ok := value.(map[string]any) + require.True(t, ok, "keys should be a map, got %T", value) + return keys + } t.Run("provision path resolves ${VAR}", func(t *testing.T) { res, err := Synthesize(Input{ @@ -555,15 +563,131 @@ services: c := getConn(t, res) assert.Equal(t, "https://mcp.example.com/mcp", c.Target) - keys, ok := c.Credentials["keys"].(map[string]any) - require.True(t, ok, "keys should be a nested map, got %T", c.Credentials["keys"]) + keys := getKeys(t, c) assert.Equal(t, "secret-value", keys["x-api-key"]) assert.Equal(t, "team-ai", c.Metadata["owner"]) - publicConnections := res.Parameters["connections"].([]Connection) + publicValue, found := res.Parameters["connections"] + require.True(t, found) + publicConnections, ok := publicValue.([]Connection) + require.True(t, ok, "connections should be []Connection") + require.Len(t, publicConnections, 1) assert.Nil(t, publicConnections[0].Credentials) - secureCredentials := res.Parameters["connectionCredentials"].(map[string]map[string]any) - assert.Equal(t, "secret-value", secureCredentials["mcp-conn"]["keys"].(map[string]any)["x-api-key"]) + secureValue, found := res.Parameters["connectionCredentials"] + require.True(t, found) + secureCredentials, ok := secureValue.(map[string]map[string]any) + require.True(t, ok, "connectionCredentials should be a map") + connectionCredentials, found := secureCredentials["mcp-conn"] + require.True(t, found) + keyValue, found := connectionCredentials["keys"] + require.True(t, found) + secureKeys, ok := keyValue.(map[string]any) + require.True(t, ok, "secure keys should be a map") + assert.Equal(t, "secret-value", secureKeys["x-api-key"]) + }) + + t.Run("service env takes precedence and isolates lookup", func(t *testing.T) { + const serviceEnvYAML = ` +services: + my-project: + host: azure.ai.project + mcp-conn: + host: azure.ai.connection + uses: [my-project] + env: + ENDPOINT: ${MCP_URL} + KEY: ${MCP_KEY} + category: RemoteTool + target: ${ENDPOINT} + authType: CustomKeys + credentials: + keys: + x-api-key: ${KEY} + metadata: + owner: ${OWNER:-service-default} +` + res, err := Synthesize(Input{ + RawAzureYAML: []byte(serviceEnvYAML), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + Env: map[string]string{ + "ENDPOINT": "https://wrong.example/mcp", + "KEY": "wrong-secret", + "OWNER": "wrong-owner", + }, + ServiceEnvironments: map[string]map[string]string{ + "mcp-conn": { + "ENDPOINT": "https://service.example/mcp", + "KEY": "service-secret", + }, + }, + }) + require.NoError(t, err) + + c := getConn(t, res) + assert.Equal(t, "https://service.example/mcp", c.Target) + keys := getKeys(t, c) + assert.Equal(t, "service-secret", keys["x-api-key"]) + assert.Equal(t, "service-default", c.Metadata["owner"]) + }) + + t.Run("explicit empty env isolates the connection", func(t *testing.T) { + const emptyEnvYAML = ` +services: + my-project: + host: azure.ai.project + mcp-conn: + host: azure.ai.connection + uses: [my-project] + env: {} + category: RemoteTool + target: ${MCP_URL} + authType: CustomKeys + credentials: + keys: + x-api-key: ${MCP_KEY} +` + res, err := Synthesize(Input{ + RawAzureYAML: []byte(emptyEnvYAML), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + Env: map[string]string{ + "MCP_URL": "https://leak.example/mcp", + "MCP_KEY": "leaked-secret", + }, + }) + require.NoError(t, err) + + c := getConn(t, res) + assert.Equal(t, "", c.Target) + keys := getKeys(t, c) + assert.Equal(t, "", keys["x-api-key"]) + }) + + t.Run("reports declared connection environment scopes", func(t *testing.T) { + const scopesYAML = ` +services: + my-project: + host: azure.ai.project + populated: + host: azure.ai.connection + env: + ENDPOINT: ${MCP_URL} + empty: + host: azure.ai.connection + env: {} + legacy: + host: azure.ai.connection +` + scopes, err := ConnectionEnvironmentScopes( + []byte(scopesYAML), + "", + ) + require.NoError(t, err) + assert.Equal(t, map[string]bool{ + "populated": true, + "empty": true, + }, scopes) }) t.Run("eject path preserves ${VAR} verbatim", func(t *testing.T) { @@ -578,8 +702,7 @@ services: c := getConn(t, res) assert.Equal(t, "${MCP_URL}", c.Target) - keys, ok := c.Credentials["keys"].(map[string]any) - require.True(t, ok) + keys := getKeys(t, c) assert.Equal(t, "${MCP_KEY}", keys["x-api-key"]) assert.Equal(t, "${MCP_OWNER}", c.Metadata["owner"]) }) @@ -608,7 +731,7 @@ services: require.NoError(t, err) c := getConn(t, res) - keys := c.Credentials["keys"].(map[string]any) + keys := getKeys(t, c) assert.Equal(t, "${{connections.other.credentials.key}}", keys["x-api-key"]) }) @@ -627,7 +750,7 @@ services: c := getConn(t, res) assert.Equal(t, "", c.Target) - keys := c.Credentials["keys"].(map[string]any) + keys := getKeys(t, c) assert.Equal(t, "", keys["x-api-key"]) }) } @@ -661,6 +784,7 @@ services: conns, err := BrownfieldConnections( []byte(yaml), map[string]string{"SEARCH_API_KEY": "secret"}, + nil, "", ) require.NoError(t, err) @@ -671,6 +795,20 @@ services: assert.Equal(t, "secret", conns[1].Credentials["key"]) }) + t.Run("service environment takes precedence", func(t *testing.T) { + conns, err := BrownfieldConnections( + []byte(yaml), + map[string]string{"SEARCH_API_KEY": "global"}, + map[string]map[string]string{ + "search-conn": {"SEARCH_API_KEY": "service"}, + }, + "", + ) + require.NoError(t, err) + require.Len(t, conns, 2) + assert.Equal(t, "service", conns[1].Credentials["key"]) + }) + t.Run("no connection services yields empty slice", func(t *testing.T) { const noConns = ` services: @@ -678,13 +816,18 @@ services: host: azure.ai.project endpoint: https://existing.services.ai.azure.com/api/projects/p1 ` - conns, err := BrownfieldConnections([]byte(noConns), nil, "") + conns, err := BrownfieldConnections( + []byte(noConns), + nil, + nil, + "", + ) require.NoError(t, err) assert.Empty(t, conns) }) t.Run("empty raw errors", func(t *testing.T) { - _, err := BrownfieldConnections(nil, nil, "") + _, err := BrownfieldConnections(nil, nil, nil, "") require.Error(t, err) }) } @@ -972,7 +1115,12 @@ services: require.Len(t, deployments, 1) assert.Equal(t, "gpt-4o", deployments[0].Name) - connections, err := BrownfieldConnections([]byte(yaml), nil, root) + connections, err := BrownfieldConnections( + []byte(yaml), + nil, + nil, + root, + ) require.NoError(t, err) require.Len(t, connections, 1) assert.Equal(t, "CognitiveSearch", connections[0].Category) diff --git a/cli/azd/extensions/azure.ai.routines/extension.yaml b/cli/azd/extensions/azure.ai.routines/extension.yaml index b799078e347..81da4c40c59 100644 --- a/cli/azd/extensions/azure.ai.routines/extension.yaml +++ b/cli/azd/extensions/azure.ai.routines/extension.yaml @@ -17,4 +17,4 @@ tags: - routine usage: azd ai routine [options] version: 1.0.0-beta.3 -requiredAzdVersion: ">=1.27.0" +requiredAzdVersion: ">=1.27.1" diff --git a/cli/azd/extensions/azure.ai.routines/internal/cmd/service_target.go b/cli/azd/extensions/azure.ai.routines/internal/cmd/service_target.go index 9203d8a3f34..487fb37bd7c 100644 --- a/cli/azd/extensions/azure.ai.routines/internal/cmd/service_target.go +++ b/cli/azd/extensions/azure.ai.routines/internal/cmd/service_target.go @@ -13,6 +13,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/azure/azure-dev/cli/azd/pkg/foundry" + "google.golang.org/grpc" ) // aiRoutineHost is the azure.yaml service host kind owned by this extension. A @@ -30,17 +31,24 @@ var _ azdext.ServiceTargetProvider = (*routineServiceTarget)(nil) // and Publish are no-ops because a routine has no build artifact. type routineServiceTarget struct { azdClient *azdext.AzdClient - serviceConfig *azdext.ServiceConfig + projectClient serviceConfigReader } // newRoutineServiceTarget creates the azure.ai.routine service-target provider. -func newRoutineServiceTarget(azdClient *azdext.AzdClient) azdext.ServiceTargetProvider { - return &routineServiceTarget{azdClient: azdClient} +func newRoutineServiceTarget( + azdClient *azdext.AzdClient, +) azdext.ServiceTargetProvider { + return &routineServiceTarget{ + azdClient: azdClient, + projectClient: azdClient.Project(), + } } -// Initialize stores the service configuration; no other setup is required. -func (p *routineServiceTarget) Initialize(ctx context.Context, serviceConfig *azdext.ServiceConfig) error { - p.serviceConfig = serviceConfig +// Initialize requires no setup. +func (p *routineServiceTarget) Initialize( + _ context.Context, + _ *azdext.ServiceConfig, +) error { return nil } @@ -111,14 +119,16 @@ func (p *routineServiceTarget) Deploy( // The service key is the routine identity; ignore any name in the body. body.Name = serviceConfig.GetName() - // Resolve ${VAR} references in the routine's action input against the azd - // environment, leaving Foundry server-side ${{...}} expressions untouched. + // Resolve ${VAR} against the service environment forwarded by azd. if body.Action != nil { - env, err := p.currentEnvValues(ctx) + environment, err := p.environmentValues(ctx, serviceConfig) if err != nil { return nil, err } - body.Action.Input = expandRoutineValue(body.Action.Input, env) + body.Action.Input = expandRoutineValue( + body.Action.Input, + environment, + ) } if progress != nil { @@ -185,16 +195,66 @@ func newRoutineServiceClient(ctx context.Context) (*routines.Client, error) { ), nil } -// currentEnvValues loads all key-value pairs from the active azd environment, used to -// resolve ${VAR} references in routine fields at deploy time. -func (p *routineServiceTarget) currentEnvValues(ctx context.Context) (map[string]string, error) { - current, err := p.azdClient.Environment().GetCurrent(ctx, &azdext.EmptyRequest{}) +// serviceConfigReader is the slice of azdext.ProjectServiceClient +// this target uses. Depending on the interface rather than the +// concrete *azdext.AzdClient lets tests supply a fake: the client's +// project field is unexported and no option overrides it. +type serviceConfigReader interface { + GetServiceConfigValue( + ctx context.Context, + in *azdext.GetServiceConfigValueRequest, + opts ...grpc.CallOption, + ) (*azdext.GetServiceConfigValueResponse, error) +} + +func serviceEnvDeclared( + ctx context.Context, + projectClient serviceConfigReader, + serviceName string, +) (bool, error) { + resp, err := projectClient.GetServiceConfigValue(ctx, &azdext.GetServiceConfigValueRequest{ + ServiceName: serviceName, + Path: "env", + }) + if err != nil { + return false, fmt.Errorf("reading env for service %q: %w", serviceName, err) + } + return resp.GetFound(), nil +} + +func (p *routineServiceTarget) environmentValues( + ctx context.Context, + serviceConfig *azdext.ServiceConfig, +) (map[string]string, error) { + environment := serviceConfig.GetEnvironment() + if len(environment) > 0 { + return environment, nil + } + // An explicit empty env: {} declares an isolated scope. + // Core forwards it as an empty map, indistinguishable from + // an omitted env, so consult the raw config before falling + // back to the full azd environment. + declared, err := serviceEnvDeclared(ctx, p.projectClient, serviceConfig.GetName()) + if err != nil { + return nil, err + } + if declared { + return environment, nil + } + + current, err := p.azdClient.Environment().GetCurrent( + ctx, + &azdext.EmptyRequest{}, + ) if err != nil { return nil, fmt.Errorf("resolving current azd environment: %w", err) } - resp, err := p.azdClient.Environment().GetValues(ctx, &azdext.GetEnvironmentRequest{ - Name: current.GetEnvironment().GetName(), - }) + resp, err := p.azdClient.Environment().GetValues( + ctx, + &azdext.GetEnvironmentRequest{ + Name: current.GetEnvironment().GetName(), + }, + ) if err != nil { return nil, fmt.Errorf("loading azd environment values: %w", err) } @@ -205,9 +265,7 @@ func (p *routineServiceTarget) currentEnvValues(ctx context.Context) (map[string return values, nil } -// expandRoutineValue recursively expands ${VAR} references in every string within a -// routine value (maps, slices, scalars) against the azd environment, preserving Foundry -// server-side ${{...}} expressions. +// expandRoutineValue expands ${VAR} in nested routine values. func expandRoutineValue(value any, env map[string]string) any { switch typed := value.(type) { case string: diff --git a/cli/azd/extensions/azure.ai.routines/internal/cmd/service_target_test.go b/cli/azd/extensions/azure.ai.routines/internal/cmd/service_target_test.go index 4df169c2cd9..96d5bd14171 100644 --- a/cli/azd/extensions/azure.ai.routines/internal/cmd/service_target_test.go +++ b/cli/azd/extensions/azure.ai.routines/internal/cmd/service_target_test.go @@ -4,11 +4,13 @@ package cmd import ( + "context" "testing" "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/grpc" "google.golang.org/protobuf/types/known/structpb" ) @@ -57,3 +59,52 @@ func TestParseRoutineServiceConfig_ConfigFallback(t *testing.T) { require.NoError(t, err) assert.Equal(t, "legacy", body.Description) } + +func TestExpandRoutineValue(t *testing.T) { + t.Parallel() + + serviceConfig := &azdext.ServiceConfig{ + Environment: map[string]string{"DIGEST_TOPIC": "weekly changes"}, + } + environment, err := (&routineServiceTarget{}).environmentValues( + t.Context(), + serviceConfig, + ) + require.NoError(t, err) + input := map[string]any{ + "topic": "${DIGEST_TOPIC}", + "secret": "${{connections.search.credentials.key}}", + } + + assert.Equal(t, map[string]any{ + "topic": "weekly changes", + "secret": "${{connections.search.credentials.key}}", + }, expandRoutineValue(input, environment)) +} + +// fakeServiceConfigReader reports a fixed env-declared result. +type fakeServiceConfigReader struct { + found bool +} + +func (f fakeServiceConfigReader) GetServiceConfigValue( + context.Context, + *azdext.GetServiceConfigValueRequest, + ...grpc.CallOption, +) (*azdext.GetServiceConfigValueResponse, error) { + return &azdext.GetServiceConfigValueResponse{Found: f.found}, nil +} + +func TestRoutineEnvironmentValuesEmptyDeclaredIsolates(t *testing.T) { + t.Parallel() + + target := &routineServiceTarget{ + projectClient: fakeServiceConfigReader{found: true}, + } + env, err := target.environmentValues( + t.Context(), + &azdext.ServiceConfig{Name: "nightly-digest"}, + ) + require.NoError(t, err) + require.Empty(t, env) +} diff --git a/cli/azd/extensions/azure.ai.routines/schemas/azure.ai.routine.json b/cli/azd/extensions/azure.ai.routines/schemas/azure.ai.routine.json index 40058c5dfaa..d4f72ec59c7 100644 --- a/cli/azd/extensions/azure.ai.routines/schemas/azure.ai.routine.json +++ b/cli/azd/extensions/azure.ai.routines/schemas/azure.ai.routine.json @@ -36,7 +36,7 @@ "properties": { "type": { "type": "string", "description": "Action variant (e.g. invoke_agent_responses_api, invoke_agent_invocations_api)." }, "agent_name": { "type": "string", "description": "Name of the azure.ai.agent service the routine invokes." }, - "input": { "description": "Static JSON input sent to the agent when the routine fires. Values may use ${VAR} or ${{...}}." } + "input": { "description": "Static JSON input sent to the agent when the routine fires. Values may use ${VAR} declared in the service-level env object or ${{...}} for Foundry server-side resolution." } } } } diff --git a/cli/azd/extensions/azure.ai.toolboxes/README.md b/cli/azd/extensions/azure.ai.toolboxes/README.md index 406294ec0ee..dd3acc39ab0 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/README.md +++ b/cli/azd/extensions/azure.ai.toolboxes/README.md @@ -16,10 +16,13 @@ services: research-tools: host: azure.ai.toolbox endpoint: ${RESEARCH_TOOLBOX_ENDPOINT} + env: + RESEARCH_TOOLBOX_ENDPOINT: ${RESEARCH_TOOLBOX_ENDPOINT} ``` Get the endpoint value from `azd ai toolbox show ` (the `Endpoint:` line). -The value may contain `${VAR}` references, which resolve against the azd -environment. Because a toolbox version is immutable, `endpoint` cannot be +The value may contain `${VAR}` references. Declare each referenced variable +in the service-level `env` object; azd falls back to the active environment +only when the service declares no `env`. Because a toolbox version is immutable, `endpoint` cannot be combined with `tools` or `description`. diff --git a/cli/azd/extensions/azure.ai.toolboxes/extension.yaml b/cli/azd/extensions/azure.ai.toolboxes/extension.yaml index 6086e624d89..a14ef500864 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/extension.yaml +++ b/cli/azd/extensions/azure.ai.toolboxes/extension.yaml @@ -17,4 +17,4 @@ tags: - toolbox usage: azd ai toolbox [options] version: 1.0.0-beta.4 -requiredAzdVersion: ">=1.27.0" +requiredAzdVersion: ">=1.27.1" diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go index d97321acaeb..db8ec17c293 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target.go @@ -15,6 +15,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/azure/azure-dev/cli/azd/pkg/foundry" + "google.golang.org/grpc" ) // aiToolboxHost is the azure.yaml service host kind owned by this extension. A @@ -47,18 +48,26 @@ type toolboxServiceConfig struct { // artifact. type toolboxServiceTarget struct { azdClient *azdext.AzdClient - serviceConfig *azdext.ServiceConfig + projectClient serviceConfigReader resolver connectionResolver } // newToolboxServiceTarget creates the azure.ai.toolbox service-target provider. -func newToolboxServiceTarget(azdClient *azdext.AzdClient) azdext.ServiceTargetProvider { - return &toolboxServiceTarget{azdClient: azdClient, resolver: defaultConnectionResolver{}} +func newToolboxServiceTarget( + azdClient *azdext.AzdClient, +) azdext.ServiceTargetProvider { + return &toolboxServiceTarget{ + azdClient: azdClient, + projectClient: azdClient.Project(), + resolver: defaultConnectionResolver{}, + } } -// Initialize stores the service configuration; no other setup is required. -func (p *toolboxServiceTarget) Initialize(ctx context.Context, serviceConfig *azdext.ServiceConfig) error { - p.serviceConfig = serviceConfig +// Initialize requires no setup. +func (p *toolboxServiceTarget) Initialize( + _ context.Context, + _ *azdext.ServiceConfig, +) error { return nil } @@ -113,8 +122,8 @@ func (p *toolboxServiceTarget) Publish( // Deploy upserts the toolbox by creating a new version from the entry's tools. Tool // entries that name a `connection` are resolved to their project_connection_id (the -// `uses:` edge guarantees the connection is reconciled first). ${VAR} references resolve -// against the azd environment; Foundry ${{...}} expressions pass through untouched. +// `uses:` edge guarantees the connection is reconciled first). ${VAR} +// references resolve from the forwarded service environment. // Removing the service from azure.yaml stops azd managing the toolbox but does not delete // it (use `azd ai toolbox delete`). // When the entry sets `endpoint` instead, azd reuses that existing @@ -135,7 +144,7 @@ func (p *toolboxServiceTarget) Deploy( // Reuse (bring-your-own): endpoint set means azd resolves ${VAR} // and publishes it for agents instead of creating a version. if strings.TrimSpace(cfg.Endpoint) != "" { - return p.deployReuse(ctx, name, cfg, progress) + return p.deployReuse(ctx, name, cfg, serviceConfig, progress) } resolved, err := projectctx.Resolve(ctx, projectctx.ResolveOpts{}) @@ -144,12 +153,16 @@ func (p *toolboxServiceTarget) Deploy( } endpoint := resolved.Endpoint - env, err := p.currentEnvValues(ctx) + environment, err := p.environmentValues(ctx, serviceConfig) if err != nil { return nil, err } - - tools, err := p.buildToolEntries(ctx, endpoint, cfg.Tools, env) + tools, err := p.buildToolEntries( + ctx, + endpoint, + cfg.Tools, + environment, + ) if err != nil { return nil, err } @@ -189,9 +202,10 @@ func (p *toolboxServiceTarget) deployReuse( ctx context.Context, name string, cfg *toolboxServiceConfig, + serviceConfig *azdext.ServiceConfig, progress azdext.ProgressReporter, ) (*azdext.ServiceDeployResult, error) { - env, err := p.currentEnvValues(ctx) + env, err := p.environmentValues(ctx, serviceConfig) if err != nil { return nil, err } @@ -323,16 +337,66 @@ func parseToolboxServiceConfig(svc *azdext.ServiceConfig) (*toolboxServiceConfig return cfg, nil } -// currentEnvValues loads all key-value pairs from the active azd environment, used to -// resolve ${VAR} references in tool fields at deploy time. -func (p *toolboxServiceTarget) currentEnvValues(ctx context.Context) (map[string]string, error) { - current, err := p.azdClient.Environment().GetCurrent(ctx, &azdext.EmptyRequest{}) +// serviceConfigReader is the slice of azdext.ProjectServiceClient +// this target uses. Depending on the interface rather than the +// concrete *azdext.AzdClient lets tests supply a fake: the client's +// project field is unexported and no option overrides it. +type serviceConfigReader interface { + GetServiceConfigValue( + ctx context.Context, + in *azdext.GetServiceConfigValueRequest, + opts ...grpc.CallOption, + ) (*azdext.GetServiceConfigValueResponse, error) +} + +func serviceEnvDeclared( + ctx context.Context, + projectClient serviceConfigReader, + serviceName string, +) (bool, error) { + resp, err := projectClient.GetServiceConfigValue(ctx, &azdext.GetServiceConfigValueRequest{ + ServiceName: serviceName, + Path: "env", + }) + if err != nil { + return false, fmt.Errorf("reading env for service %q: %w", serviceName, err) + } + return resp.GetFound(), nil +} + +func (p *toolboxServiceTarget) environmentValues( + ctx context.Context, + serviceConfig *azdext.ServiceConfig, +) (map[string]string, error) { + environment := serviceConfig.GetEnvironment() + if len(environment) > 0 { + return environment, nil + } + // An explicit empty env: {} declares an isolated scope. + // Core forwards it as an empty map, indistinguishable from + // an omitted env, so consult the raw config before falling + // back to the full azd environment. + declared, err := serviceEnvDeclared(ctx, p.projectClient, serviceConfig.GetName()) + if err != nil { + return nil, err + } + if declared { + return environment, nil + } + + current, err := p.azdClient.Environment().GetCurrent( + ctx, + &azdext.EmptyRequest{}, + ) if err != nil { return nil, fmt.Errorf("resolving current azd environment: %w", err) } - resp, err := p.azdClient.Environment().GetValues(ctx, &azdext.GetEnvironmentRequest{ - Name: current.GetEnvironment().GetName(), - }) + resp, err := p.azdClient.Environment().GetValues( + ctx, + &azdext.GetEnvironmentRequest{ + Name: current.GetEnvironment().GetName(), + }, + ) if err != nil { return nil, fmt.Errorf("loading azd environment values: %w", err) } @@ -343,9 +407,7 @@ func (p *toolboxServiceTarget) currentEnvValues(ctx context.Context) (map[string return values, nil } -// expandToolboxValue recursively expands ${VAR} references in every string within a tool -// value (maps, slices, scalars) against the azd environment, preserving Foundry -// server-side ${{...}} expressions. +// expandToolboxValue expands ${VAR} in nested toolbox values. func expandToolboxValue(value any, env map[string]string) any { switch typed := value.(type) { case string: diff --git a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target_test.go b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target_test.go index 8fece33c91a..37d50fc7ee7 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target_test.go +++ b/cli/azd/extensions/azure.ai.toolboxes/internal/cmd/service_target_test.go @@ -10,6 +10,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/grpc" "google.golang.org/protobuf/types/known/structpb" ) @@ -145,6 +146,30 @@ func TestPublishReuseEndpoint_WritesExpandedEndpoint(t *testing.T) { assert.Equal(t, wantURL, (*calls)[0].value) } +func TestDeployReuseUsesServiceEnvironment(t *testing.T) { + // No t.Parallel: stubToolboxEndpointEnv swaps a package-level seam. + calls := stubToolboxEndpointEnv(t) + const wantURL = "https://mcp.example.com/toolboxes/research" + serviceConfig := &azdext.ServiceConfig{ + Environment: map[string]string{ + "RESEARCH_TOOLBOX_ENDPOINT": wantURL, + }, + } + + result, err := (&toolboxServiceTarget{}).deployReuse( + t.Context(), + "research", + &toolboxServiceConfig{Endpoint: "${RESEARCH_TOOLBOX_ENDPOINT}"}, + serviceConfig, + nil, + ) + + require.NoError(t, err) + require.NotNil(t, result) + require.Len(t, *calls, 1) + assert.Equal(t, wantURL, (*calls)[0].value) +} + func TestBuildToolEntries_ResolvesConnectionRef(t *testing.T) { t.Parallel() @@ -174,16 +199,55 @@ func TestBuildToolEntries_ResolvesConnectionRef(t *testing.T) { func TestExpandToolboxValue(t *testing.T) { t.Parallel() - env := map[string]string{"MCP_URL": "https://resolved.example.com"} + serviceConfig := &azdext.ServiceConfig{ + Environment: map[string]string{ + "MCP_URL": "https://resolved.example.com", + }, + } + environment, err := (&toolboxServiceTarget{}).environmentValues( + t.Context(), + serviceConfig, + ) + require.NoError(t, err) in := map[string]any{ "type": "mcp", "server_url": "${MCP_URL}", "headers": []any{"x-secret: ${{secrets.token}}"}, } - out, ok := expandToolboxValue(in, env).(map[string]any) + out, ok := expandToolboxValue( + in, + environment, + ).(map[string]any) require.True(t, ok) assert.Equal(t, "https://resolved.example.com", out["server_url"]) // Foundry ${{...}} passes through untouched. assert.Equal(t, []any{"x-secret: ${{secrets.token}}"}, out["headers"]) } + +// fakeServiceConfigReader reports a fixed env-declared result. +type fakeServiceConfigReader struct { + found bool +} + +func (f fakeServiceConfigReader) GetServiceConfigValue( + context.Context, + *azdext.GetServiceConfigValueRequest, + ...grpc.CallOption, +) (*azdext.GetServiceConfigValueResponse, error) { + return &azdext.GetServiceConfigValueResponse{Found: f.found}, nil +} + +func TestToolboxEnvironmentValuesEmptyDeclaredIsolates(t *testing.T) { + t.Parallel() + + target := &toolboxServiceTarget{ + projectClient: fakeServiceConfigReader{found: true}, + } + env, err := target.environmentValues( + t.Context(), + &azdext.ServiceConfig{Name: "research-tools"}, + ) + require.NoError(t, err) + require.Empty(t, env) +} diff --git a/cli/azd/extensions/azure.ai.toolboxes/schemas/azure.ai.toolbox.json b/cli/azd/extensions/azure.ai.toolboxes/schemas/azure.ai.toolbox.json index 54ae8ef04e3..08c300a8254 100644 --- a/cli/azd/extensions/azure.ai.toolboxes/schemas/azure.ai.toolbox.json +++ b/cli/azd/extensions/azure.ai.toolboxes/schemas/azure.ai.toolbox.json @@ -8,7 +8,7 @@ "properties": { "endpoint": { "type": "string", - "description": "MCP endpoint URL of an existing Foundry toolbox version, as shown by 'azd ai toolbox show' (e.g. https://my-account.services.ai.azure.com/api/projects/my-project/toolboxes/research/versions/3/mcp?api-version=v1). When set, azd reuses this existing toolbox and publishes the endpoint for agents instead of creating a new version, so 'tools' and 'description' must be omitted. May contain ${VAR} (azd env, resolved client-side)." + "description": "MCP endpoint URL of an existing Foundry toolbox version, as shown by 'azd ai toolbox show' (e.g. https://my-account.services.ai.azure.com/api/projects/my-project/toolboxes/research/versions/3/mcp?api-version=v1). When set, azd reuses this existing toolbox and publishes the endpoint for agents instead of creating a new version, so 'tools' and 'description' must be omitted. May contain ${VAR} declared in the service-level env object." }, "description": { "type": "string", @@ -16,7 +16,7 @@ }, "tools": { "type": "array", - "description": "List of tools in the toolbox.", + "description": "List of tools in the toolbox. Values may use ${VAR} declared in the service-level env object.", "items": { "type": "object", "required": ["type"],