fix(agents): prompt for unified config env values#9212
Conversation
Prompt for unset bare ${VAR} references in adopted Foundry service configuration and persist the responses to the active azd environment. Preserve defaults, Foundry expressions, existing values, hook behavior, and --no-prompt execution.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds prompting and persistence for missing environment references in adopted Foundry configurations.
Changes:
- Detects bare
${VAR}references while excluding defaults, hooks, and escaped references. - Masks credential-like prompts and saves responses to the active environment.
- Integrates the flow into unified
azure.yamladoption with tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
init_env.go |
Implements reference discovery, prompting, and persistence. |
init_env_test.go |
Tests detection, masking, persistence, and no-prompt behavior. |
init_adopt.go |
Invokes environment configuration during adoption. |
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
jongio
left a comment
There was a problem hiding this comment.
Traced the `` resolution through foundry.ResolveFileRefs (sibling keys overlay the loaded object) and confirmed the `${{...}}` masking is multiline-aware before the env-ref regex runs. Escape handling, `:-` default skipping, hooks exclusion, and secret-upgrade dedup all line up with the tests. Nothing blocking from me.
Mask prompted values only when the reference appears under explicit credential or secret configuration. Avoid inferring sensitivity from arbitrary environment variable names while continuing to prompt for all unset Foundry references. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9
jongio
left a comment
There was a problem hiding this comment.
init_env.go:78 - the missing-value check reads from GetValues, which returns only persisted dotenv entries. Foundry expansion resolves unset refs from the process environment as well (synthesis/synthesizer.go maybeExpand falls back to os.LookupEnv). A var that is exported in the shell but absent from the azd env is treated as missing here, so we prompt for it and then persist a value that shadows the exported one. Mirroring the same dotenv-first, process-env fallback lookup (while still counting an explicitly empty dotenv entry as missing) avoids the spurious prompt. A process-env-only regression test would lock it in.
Use persisted azd values first and fall back to the process environment before prompting for unified Foundry references. Keep explicitly empty persisted values missing so users can replace them interactively. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9
jongio
left a comment
There was a problem hiding this comment.
The escape-handling point flagged inline on init_env.go:507 is real. Flagging as non-blocking with the trace.
Discovery treats an odd run of leading $ as an escape (isEscapedAzureYamlEnvironmentReference, init_env.go:529), so $${VNET_ID} in a network vnet or dns.subscription field is skipped and never prompted. The expander that owns those fields is resolveVars (azure.ai.projects synthesizer.go:895), and it uses a plain ${VAR} regex with no escape awareness (varRefPattern, synthesizer.go:686). So the same $${VNET_ID} expands there: it resolves to $<value> when the var is set (then fails vnetIDPattern), or errors as unresolved when it isn't. Either way provisioning fails on a reference discovery chose not to prompt for.
The trigger is degenerate: a literal ${VNET_ID} isn't a valid VNet ARM id or subscription GUID, so no real config escapes a ref in these fields. The plausible case is a user mistaking $$ for variable syntax, which then fails at provision instead of prompting. Not silent corruption, just a worse error path.
If escaping isn't meant to apply to the network ARM-id/GUID fields, scoping the escape skip to the Foundry string fields keeps discovery and resolveVars from disagreeing. Low priority.
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go:515
- [azd-code-reviewer] This blanket skip assumes every supported provider expands
:-defaults. The project-network resolver only matches bare${VAR}references (azure.ai.projects/internal/synthesis/synthesizer.go:685-686), so${VNET_ID:-...}or${DNS_SUBSCRIPTION_ID:-...}remains literal and then fails ID/GUID validation. Route project network fields through the shared Foundry expander (including the legacy provider) before treating these references as self-sufficient.
if match[4] != -1 {
continue
}
cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go:381
- [azd-code-reviewer] The behavior under test depends on these Playwright variables being absent from the process environment. A developer who has either value exported will skip that prompt and fail the assertions. Set both names to an empty value before configuring the project so this test is deterministic.
projectDir := t.TempDir()
Clear process environment values in prompt-dependent tests so exported developer or runner variables cannot bypass the prompts under test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9
jongio
left a comment
There was a problem hiding this comment.
Verified the escape alignment in e0639ba against both expanders.
Project and Foundry network fields (agentSubnet.vnet, peSubnet.vnet, dns.subscription) are expanded by resolveVars, which runs a bare ${VAR} regex with no escape handling. Same function in both synthesizers that own those hosts: azure.ai.projects/internal/synthesis/synthesizer.go:895 (reached from resolveSubnet at :854 and the dns.subscription branch at :801) and the azure.ai.agents copy at the same lines for the legacy microsoft.foundry host. So honorEscaping=false there is the right match: discovery now prompts for $${VNET_ID} because resolveVars is going to substitute it either way.
Everything else routes through foundry.ExpandEnv (drone/envsubst), which does honor the escape: agent environmentVariables via project.ExpandEnv (service_target_agent.go:2620), connection target/credentials/metadata via maybeExpand and expandCredentials (synthesizer.go:571, :588), routine action.input (routines service_target.go:214), toolbox endpoint/tools (toolboxes service_target.go:248, :352). I ran the expander directly to pin the parity rule: $${VAR} gives a literal ${VAR}, $$${VAR} gives $ plus the expanded value. honorEscaping=true on those paths lines up.
Also checked the test isolation in 9b75e58. t.Setenv(name, "") clears these correctly because the process fallback at init_env.go:160 requires ok && value != "", so a present-but-empty var still lands in missing. I ran the prompt tests with PLAYWRIGHT_SERVICE_ACCESS_TOKEN, PLAYWRIGHT_SERVICE_RESOURCE_ID, REFERENCED_API_TOKEN and ROOT_RESOURCE_ID exported to non-empty values and they pass. The two tests in that file still on t.Parallel() don't reach the fallback: NoPromptSkipsPrompts returns at init_env.go:120 before discovery runs, and SkipsConfiguredValues has the value in the azd env so it short-circuits at :153.
Build, vet, and the internal/cmd suite pass at 9b75e58. Two non-blocking notes inline.
Name and document the escape modes used by each owning provider expander, and cover a second project-network reference that intentionally ignores leading-dollar escaping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9
jongio
left a comment
There was a problem hiding this comment.
Two things, neither blocking.
Medium: --no-prompt returns before discovery runs, so process-env values never get persisted in automation, and the three hosts that read only the persisted azd environment will expand them to empty at deploy. init_env.go:129
Low: the ${{...}} span regex is duplicated from pkg/foundry instead of shared, which is the drift class this PR has been chasing. init_env.go:26
Separately, on how the field list was decided: I traced it against production and it matches. Agent environmentVariables[*].value, connection target/credentials/metadata, and toolbox/routine values all route through foundry.ExpandEnv, while project network.*.vnet and network.dns.subscription go through resolveVars. That split is exactly why the two escape modes exist.
Continue environment discovery in no-prompt mode so exported values are persisted for providers that read only azd state. Derive protected reference occurrences through foundry.ExpandEnv with unique probes instead of duplicating its server-span matcher. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4617dfd-adfb-4b30-9222-477a041f8af9
Summary
${VAR}references in adopted Foundry service configuration duringazd ai agent init${VAR:-default}, Foundry${{...}}, hook variables, and--no-promptbehaviorTesting
go test ./... -count=1go build ./...golangci-lint run ./internal/cmd/...Fixes #9110