refactor: decouple Foundry project selection from model configuration in agent init#8292
Conversation
Split the combined 'Deploy new model(s) / Use existing Foundry project' prompt into two distinct steps: 1. Select a Foundry project (existing or new) 2. Configure model(s) with options that adapt based on Step 1 Also fixes: - init_from_code.go now properly sets USE_EXISTING_AI_PROJECT (was missing) - End message recommends 'azd up' when new model deployments need provisioning
When a manifest template specifies a model (e.g. gpt-4.1-mini), the user is now prompted to confirm or choose a different model from the catalog before proceeding to version/SKU/capacity selection. In no-prompt mode, the manifest model is used without prompting (existing behavior).
There was a problem hiding this comment.
Pull request overview
Refactors the azd ai agent init interactive flow to separate Foundry project selection from model configuration, aiming to make the UX clearer and to allow overriding manifest-specified default models before version/SKU selection.
Changes:
- Split the init UX into sequential steps: (1) Foundry project selection, then (2) model configuration options that adapt to the project choice.
- Add a confirmation step when a manifest-specified model exists in the catalog, allowing the user to keep it or select a different catalog model.
- Update post-init guidance logic (and wire up
USE_EXISTING_AI_PROJECTin the code-init path) to better reflect when users should runazd upvsazd deploy.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cli/azd/extensions/azure.ai.agents/internal/cmd/init.go | Reworks interactive init flow to select Foundry project first, then process models; tweaks next-steps message logic. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/init_models.go | Allows confirming/changing a manifest-specified model before proceeding to deployment version/SKU selection. |
| cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go | Applies the same “project first, models second” UX to init-from-code and ensures USE_EXISTING_AI_PROJECT is written. |
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.agents/internal/cmd/init.go:2045
- The completion message is using
len(a.deploymentDetails) == 0to decide between recommendingazd deployvsazd up, butdeploymentDetailsis populated for both existing and new model deployments (it always records the selected deployment details). This will incorrectly steer users who selected an existing Foundry project + existing deployment(s) to runazd upeven when provisioning isn’t required. Track whether any new resources/deployments need provisioning (or inspect the user’s choice) and base the message on that signal instead ofdeploymentDetailslength.
}); projectID != nil && projectID.Value != "" && len(a.deploymentDetails) == 0 {
fmt.Printf("To deploy your agent, use %s.\n",
color.HiBlueString("azd deploy %s", a.serviceNameOverride))
} else {
fmt.Printf(
wbreza
left a comment
There was a problem hiding this comment.
Code Review Summary
The refactor is correctly implemented and backward-compatible. All five branches of the new two-step flow set USE_EXISTING_AI_PROJECT correctly, the .env / azure.yaml contracts are unchanged from pre-PR behavior, the azd up vs azd deploy end-message condition is logically sound, auth flows correctly use UserTenantId, and the #8287 manifest-model fix works in both interactive and --no-prompt modes. Findings below are about consistency and polish, not blocking issues.
🟡 Medium
Missing IsCancellation() check on Step 1 project-selection prompt
cli/azd/extensions/azure.ai.agents/internal/cmd/init.go(new Step 1 prompt, ~L81–L89)cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go(new Step 1 prompt, ~L236–L249)
Both new Step 1 prompts wrap errors with exterrors.FromPrompt(...) directly. However, Step 2's model-config prompt in init_from_code.go (~L329) does explicitly check exterrors.IsCancellation(err) first and return exterrors.Cancelled(...). This inconsistency means Ctrl+C at Step 1 will be surfaced/telemetered as a generic prompt failure rather than a clean user cancellation, and the two prompts in the same file behave differently.
Suggested:
resp, err := a.azdClient.Prompt().Select(ctx, &azdext.SelectRequest{ /* ... */ })
if err != nil {
if exterrors.IsCancellation(err) {
return nil, exterrors.Cancelled("project selection was cancelled")
}
return nil, exterrors.FromPrompt(err, "failed to prompt for Foundry project configuration choice")
}🔵 Low
1. Duplicated Foundry-project-selection block between the two files
init.go~L849–L925 andinit_from_code.go~L230–L299
Roughly 70 lines of near-identical sequence (ensureSubscription → selectFoundryProject → null-check → setEnvValue → fallback). Future fixes are likely to drift between the two files. Consider extracting a small helper such as promptOrSelectFoundryProject(...) that both callers can use. Non-blocking; would be nice as a follow-up.
2. setEnvValue() errors returned unwrapped
Multiple sites in both files, e.g.:
if err := setEnvValue(ctx, a.azdClient, a.environment.Name, "USE_EXISTING_AI_PROJECT", "true"); err != nil {
return nil, err
}If the underlying gRPC call fails, the caller receives a raw error with no context about which env var was being written. Suggest wrapping:
return nil, fmt.Errorf("failed to set USE_EXISTING_AI_PROJECT: %w", err)3. Error from promptModelFromCatalog in #8287 fix is returned unwrapped
init_models.go(new "Choose a different model" branch, ~L452)
When the user picks "Choose a different model" at the manifest-model confirmation prompt, the follow-up promptModelFromCatalog error is returned without context, so it's indistinguishable from a primary model-selection failure. Suggest:
selectedModel, err := a.promptModelFromCatalog(ctx)
if err != nil {
return nil, fmt.Errorf("failed to select alternative model: %w", err)
}✅ Explicitly verified clean
- Backward compatibility — env-var names/values and
azure.yamlfields are identical to pre-PR behavior across all five flow branches (existing + use-existing-deployment, existing + deploy-from-catalog, existing + skip, new + deploy-from-catalog, new + skip). - End-of-init
azd upvsazd deploymessage — the new conditionprojectID != nil && projectID.Value != "" && len(a.deploymentDetails) == 0is correct;azd deployonly when no new model deployments need provisioning. - Auth / security —
Subscription.UserTenantIdcorrectly used in credential paths; no new credentials or secrets introduced. - #8287 fix — interactive confirm/change works;
--no-promptmode bypasses the prompt and uses the manifest model unchanged, as intended. USE_EXISTING_AI_PROJECT— now set on every branch in bothinit.goandinit_from_code.go, addressing the previously missing set in theinit_from_codeflow.
Review focused on correctness, backward-compatibility, error handling, security/auth, and tests/UX/maintainability. No critical or high-severity issues found.
…ct-id validation - Add IsCancellation() check on Step 1 project-selection prompts (init.go, init_from_code.go) - Wrap setEnvValue errors with context describing which env var failed - Wrap promptModelFromCatalog error with descriptive context - Add extractProjectDetails validation for --project-id in init_from_code.go - Improve error messages when specified foundry project is not found/eligible
thanks for the review and comments, quick improvements are made as
|
…needs provisioning The end-of-init message now checks needsProvision to avoid recommending 'azd deploy' when a new model deployment still needs to be provisioned, aligning with the logic in init.go.
|
/check-enforcer override |
wbreza
left a comment
There was a problem hiding this comment.
Re-review on 56d16a7
Thanks for the quick turnaround on the prior feedback. Status update plus one important correction to my earlier review.
✅ Prior feedback addressed cleanly
- M1 —
IsCancellation()on Step 1 prompts: Fixed in bothinit.go(~L1195) andinit_from_code.go(~L315). Both Step 1 and Step 2 prompts now have consistent cancellation semantics across the two files. 👍 - L2 —
setEnvValueerror wrapping: Wrapped withfmt.Errorf("...: %w", err)at all flagged sites in both files. 👍 - L3 —
promptModelFromCatalogerror wrapping: Wrapped viaexterrors.FromPrompt(...)in the new helper. 👍 - L1 — Duplicated project-selection block: Punted as agreed; non-blocking follow-up.
✨ Bonus improvement
The new extractProjectDetails validator for --project-id in init_from_code.go is a nice add. It validates the ARM resource ID format via regex, returns exterrors.Validation with CodeInvalidProjectResourceId and an actionable suggestion that includes the expected format template. Good telemetry posture.
🟠 Correction to my prior review — Copilot bot reviewer was right
I want to retract the "explicitly verified clean" claim I made about the azd up vs azd deploy end-of-init message. On a deeper trace of a.deploymentDetails, the Copilot bot reviewer's concern is correct, and this is a real (likely Medium) bug.
The condition at cli/azd/extensions/azure.ai.agents/internal/cmd/init.go:~2045:
}); projectID != nil && projectID.Value != "" && len(a.deploymentDetails) == 0 {
// → recommend `azd deploy`
} else {
// → recommend `azd up`
}Why this is wrong: a.deploymentDetails is populated by ProcessModels (init_models.go ~L796: deploymentDetails = append(deploymentDetails, *modelDeployment)) for both code paths:
- Existing-deployment path (init_models.go ~L197–L212): the selected existing
Deploymentis returned and appended →deploymentDetailsnon-empty. - New-deployment path (init_models.go ~L309–L320): the new
Deploymentis returned and appended →deploymentDetailsnon-empty.
So the only way len(a.deploymentDetails) == 0 is true is when the user skipped model configuration entirely. The current logic therefore steers users who selected existing project + existing deployment (no new provisioning needed!) to run azd up — exactly backwards.
Truth table of the four interactive paths:
| Project | Model config | Needs new provisioning? | Current message | Correct message |
|---|---|---|---|---|
| Existing | Use existing deployment | No | azd up ❌ |
azd deploy |
| Existing | Deploy from catalog | Yes (new model) | azd up ✅ |
azd up |
| Existing | Skip | No | azd deploy ✅ |
azd deploy |
| New | Deploy from catalog | Yes (project + model) | azd up ✅ |
azd up |
| New | Skip | Yes (project) | azd up ✅ |
azd up |
The first row is the regression — and it's also the most common "existing-everything" UX path where the user just wants to wire up an agent against pre-existing infra.
Suggested fix: Instead of inferring from len(a.deploymentDetails), track an explicit signal of whether any new resource is being introduced. Two options:
-
Add a boolean
a.needsProvisioning boolset totruewhen:- the user chooses "Create a new Foundry project" (Step 1), OR
- the user chooses "Deploy a new model from the catalog" (Step 2)
Then condition on
if projectID != nil && projectID.Value != "" && !a.needsProvisioning. -
Or mark
Deploymententries with anIsExisting boolfield at creation time ininit_models.go(~L197 vs ~L309), and condition on whetherdeploymentDetailscontains any non-existing entries.
Option 1 is simpler and decouples the UX message from the data structure.
Recap
- M1 / L2 / L3 /
extractProjectDetails: ✅ clean - L1 duplication: deferred (fine)
- 🟠 End-of-init message
azd up/azd deploylogic: this is a real bug, my prior "verified clean" was incorrect — apologies for the noise. Worth fixing before merge.
No other new findings introduced by 56d16a7.
Summary
Splits the combined "Deploy new model(s) / Use existing Foundry project" prompt into two distinct sequential steps, making the
azd ai agent initUX clearer and more flexible.Changes
init_from_code.gonow properly setsUSE_EXISTING_AI_PROJECT(was never set before)azd upinstead ofazd deploywhen new model deployments need provisioningNo impact on downstream
The configuration values written to
.envandazure.yamlare unchanged — this is purely a UX refactor.azd provisionandazd deploybehavior is unaffected.Addresses feedback from #8265.
Fixes #8287.