From 030b0c89c6c3e9942a5c4aba9b8634e878f9bc98 Mon Sep 17 00:00:00 2001 From: huimiu Date: Thu, 13 Aug 2026 14:46:50 +0800 Subject: [PATCH 1/7] fix: unify split toolbox diagnostics --- .../azure.ai.agents/internal/cmd/doctor.go | 24 ++-- .../internal/cmd/doctor/checks_connections.go | 9 +- .../internal/cmd/doctor/checks_local.go | 12 +- .../internal/cmd/doctor/checks_local_test.go | 2 +- .../internal/cmd/doctor/checks_manual_env.go | 12 +- .../internal/cmd/doctor/checks_toolboxes.go | 118 +++++++++++++--- .../cmd/doctor/checks_toolboxes_test.go | 68 ++++++++- .../internal/cmd/doctor/state_cache.go | 48 +++++++ .../internal/cmd/doctor/state_cache_test.go | 39 +++++ .../internal/cmd/nextstep/manifest.go | 66 ++++++++- .../internal/cmd/nextstep/resolver.go | 102 +++++++++++--- .../internal/cmd/nextstep/resolver_test.go | 96 ++++++++++++- .../internal/cmd/nextstep/state.go | 108 +++++++------- .../internal/cmd/nextstep/state_test.go | 133 +++++++++++++++++- .../internal/cmd/nextstep/types.go | 91 ++++++------ 15 files changed, 742 insertions(+), 186 deletions(-) create mode 100644 cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache.go create mode 100644 cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache_test.go diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor.go index cf389285196..d8e18a70cfe 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor.go @@ -72,7 +72,7 @@ Exit codes: Unredacted: flags.unredacted, } - report, err := runAndRenderDoctorText(ctx, deps, opts, azdClient, os.Stdout, debug) + report, err := runAndRenderDoctorText(ctx, deps, opts, os.Stdout, debug) if err != nil { return err } @@ -109,7 +109,6 @@ func runAndRenderDoctorText( ctx context.Context, deps doctor.Dependencies, opts doctor.Options, - azdClient *azdext.AzdClient, w io.Writer, debug bool, ) (doctor.Report, error) { @@ -122,7 +121,6 @@ func runAndRenderDoctorText( ctx, deps, opts, - azdClient, func(result doctor.Result) error { return renderer.writeCheck(result) }, @@ -142,9 +140,11 @@ func runDoctorWithObserver( ctx context.Context, deps doctor.Dependencies, opts doctor.Options, - azdClient *azdext.AzdClient, observer doctor.ResultObserver, ) (doctor.Report, []nextstep.Suggestion, error) { + if deps.StateCache == nil { + deps.StateCache = doctor.NewStateCache() + } // Keep local checks first so remote checks can inspect their prior // results for skip-cascade decisions. checks := append(doctor.NewLocalChecks(deps), doctor.NewRemoteChecks(deps)...) @@ -160,20 +160,20 @@ func runDoctorWithObserver( return report, nil, nil } - trailing := resolveDoctorTrailing(ctx, azdClient) + trailing := resolveDoctorTrailing(ctx, deps) return report, trailing, nil } // resolveDoctorTrailing returns the doctor's trailing Next block, or nil on // error. It chooses deployed-agent suggestions when any service is deployed; // otherwise it reuses the post-init guidance. -func resolveDoctorTrailing(ctx context.Context, azdClient *azdext.AzdClient) []nextstep.Suggestion { - if azdClient == nil { +func resolveDoctorTrailing(ctx context.Context, deps doctor.Dependencies) []nextstep.Suggestion { + if deps.AzdClient == nil { return nil } - state, _ := nextstep.AssembleStateFromSource(ctx, nextstep.NewSource(azdClient)) - if len(state.Services) == 0 { + state, _ := deps.AssembleAgentState(ctx) + if state == nil || len(state.Services) == 0 { // Avoid repeating the missing-service guidance already reported by // `local.agent-service-detected`. return nil @@ -184,12 +184,12 @@ func resolveDoctorTrailing(ctx context.Context, azdClient *azdext.AzdClient) []n // stay copy-paste correct. return nextstep.ResolveAfterDeploy( filterDeployedServices(state), - doctorCachedPayload(ctx, azdClient), - readmeExistsForProject(ctx, azdClient), + doctorCachedPayload(ctx, deps.AzdClient), + readmeExistsForProject(ctx, deps.AzdClient), ) } - return nextstep.ResolveAfterInit(state, readmeExistsForProject(ctx, azdClient)) + return nextstep.ResolveAfterInit(state, readmeExistsForProject(ctx, deps.AzdClient)) } func anyServiceDeployed(services []nextstep.ServiceState) bool { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_connections.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_connections.go index d3536124af1..c8fc40ef91e 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_connections.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_connections.go @@ -15,7 +15,6 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" - "github.com/azure/azure-dev/cli/azd/pkg/azdext" ) // foundryConnectionsProbeTimeout caps the per-project connections @@ -114,13 +113,7 @@ func newCheckConnections(deps Dependencies) Check { } } - assembler := deps.assembleState - if assembler == nil { - assembler = func(c context.Context, client *azdext.AzdClient) (*nextstep.State, []error) { - return nextstep.AssembleState(c, client) - } - } - state, errs := assembler(ctx, deps.AzdClient) + state, errs := deps.AssembleAgentState(ctx) if state == nil { cause := "unknown error" if len(errs) > 0 { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go index 4fb201bef25..c5ed5b51d68 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go @@ -54,13 +54,16 @@ type Dependencies struct { AgentAPIVersion string // assembleState is a test seam: when non-nil it replaces the - // production `nextstep.AssembleState` call inside the - // `local.manual-env-vars` check, letting unit tests inject a - // pre-computed State without standing up a temp project on disk. + // production `nextstep.AssembleState` call, letting unit tests inject + // a pre-computed State without standing up a temp project on disk. // Lowercase so external packages cannot reach it. Production code // (NewLocalChecks via the Cobra wiring) leaves it nil. assembleState func(ctx context.Context, client *azdext.AzdClient) (*nextstep.State, []error) + // StateCache shares one assembled state across Doctor checks and + // trailing guidance during a single invocation. + StateCache *StateCache + // probeAuth is a test seam: when non-nil it replaces the // production `realProbeAuth` call inside the `remote.auth` check, // letting unit tests inject controlled token-acquisition outcomes @@ -174,6 +177,9 @@ type Dependencies struct { // endpoint env vars; it is local because it does not call ARM / // Foundry (only the active azd environment). func NewLocalChecks(deps Dependencies) []Check { + if deps.StateCache == nil { + deps.StateCache = NewStateCache() + } return []Check{ newCheckGRPCAndVersion(deps), newCheckProjectConfig(deps), diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local_test.go index f770df2a2d2..14b538165d7 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local_test.go @@ -457,7 +457,7 @@ func TestNewLocalChecks_OrderAndIDs(t *testing.T) { {"local.project-endpoint-set", "FOUNDRY_PROJECT_ENDPOINT set", false}, {"local.agent-yaml-valid", "agent definition valid (per service)", false}, {"local.manual-env-vars", "manual env vars set", false}, - {"local.toolboxes", "Manifest toolboxes have endpoint env vars set", false}, + {"local.toolboxes", "Configured toolboxes have endpoint env vars set", false}, } for i, w := range want { require.Equal(t, w.id, checks[i].ID, "index %d", i) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_manual_env.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_manual_env.go index 875c4d5edb1..9ab73060a48 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_manual_env.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_manual_env.go @@ -8,10 +8,6 @@ import ( "fmt" "slices" "strings" - - "azureaiagent/internal/cmd/nextstep" - - "github.com/azure/azure-dev/cli/azd/pkg/azdext" ) // newCheckManualEnvVars produces Check `local.manual-env-vars` — the @@ -90,13 +86,7 @@ func newCheckManualEnvVars(deps Dependencies) Check { } } - assembler := deps.assembleState - if assembler == nil { - assembler = func(c context.Context, client *azdext.AzdClient) (*nextstep.State, []error) { - return nextstep.AssembleState(c, client) - } - } - state, errs := assembler(ctx, deps.AzdClient) + state, errs := deps.AssembleAgentState(ctx) if state == nil { // AssembleState always returns a non-nil State even when errs // is non-empty — but defend against a future contract change diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go index aba74aac93b..489fc1265f7 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go @@ -26,8 +26,7 @@ import ( type toolboxEnvLookupFn func(ctx context.Context, key string) (value string, err error) // newCheckToolboxes produces Check `local.toolboxes` (P5.1 C14). -// For each `ToolboxResource` declared in any service's -// `agent.manifest.yaml` (collected by the C2 manifest walker), the +// For each toolbox collected during next-step state assembly, the // check verifies that the canonical // `TOOLBOX__MCP_ENDPOINT` env var is set to a // non-empty value in the active azd environment. @@ -44,7 +43,7 @@ type toolboxEnvLookupFn func(ctx context.Context, key string) (value string, err // skips in this state, so the toolbox check would falsely Pass. // - `local.azure-yaml` / `local.agent-service-detected` failed → // no services to walk; walker output is unreliable. -// - state.HasToolboxes == false → no manifest toolbox declarations; +// - state.HasToolboxes == false → no toolbox declarations; // the check has nothing to verify. // // # Why this check is not gated on `remote.auth` / @@ -69,7 +68,7 @@ type toolboxEnvLookupFn func(ctx context.Context, key string) (value string, err func newCheckToolboxes(deps Dependencies) Check { return Check{ ID: "local.toolboxes", - Name: "Manifest toolboxes have endpoint env vars set", + Name: "Configured toolboxes have endpoint env vars set", Remote: false, Fn: func(ctx context.Context, _ Options, prior []Result) Result { if deps.AzdClient == nil { @@ -94,13 +93,7 @@ func newCheckToolboxes(deps Dependencies) Check { } } - assembler := deps.assembleState - if assembler == nil { - assembler = func(c context.Context, client *azdext.AzdClient) (*nextstep.State, []error) { - return nextstep.AssembleState(c, client) - } - } - state, errs := assembler(ctx, deps.AzdClient) + state, errs := deps.AssembleAgentState(ctx) if state == nil { // AssembleState always returns a non-nil State even when errs // is non-empty (state.go), but defend against a future contract @@ -119,16 +112,109 @@ func newCheckToolboxes(deps Dependencies) Check { if !state.HasToolboxes { return Result{ Status: StatusSkip, - Message: "skipped: no toolbox resources declared in any service's agent.manifest.yaml.", + Message: "skipped: no configured toolbox resources were found.", + } + } + + if state.ToolboxEndpointsChecked { + if len(state.ToolboxEndpointErrors) > 0 { + return Result{ + Status: StatusFail, + Message: fmt.Sprintf( + "could not read toolbox endpoint values: %s", + strings.Join(state.ToolboxEndpointErrors, "; ")), + Suggestion: "Verify the active azd environment is accessible, then re-run " + + "`azd ai agent doctor`.", + Details: map[string]any{ + "toolboxEndpointErrors": state.ToolboxEndpointErrors, + }, + } } + return classifyToolboxState(state.Toolboxes, state.MissingToolboxEndpoints) } - lookup := deps.lookupToolboxEnv - if lookup == nil { - lookup = makeRealToolboxEnvLookup(deps.AzdClient) + // Keep the old seam for callers constructing partial states. Production + // assembly always sets ToolboxEndpointsChecked and never reads again. + if deps.lookupToolboxEnv != nil { + return classifyToolboxEndpoints(ctx, state.Toolboxes, deps.lookupToolboxEnv) } + return classifyToolboxState(state.Toolboxes, state.MissingToolboxEndpoints) + }, + } +} + +func classifyToolboxState( + toolboxes, missing []nextstep.ResourceRef, +) Result { + missingKeys := make(map[string]struct{}, len(missing)) + missingUnique := make([]nextstep.ResourceRef, 0, len(missing)) + for _, toolbox := range missing { + key := envkey.ToolboxMCPEndpoint(toolbox.Name) + if _, duplicate := missingKeys[key]; duplicate { + continue + } + missingKeys[key] = struct{}{} + missingUnique = append(missingUnique, toolbox) + } + seen := make(map[string]struct{}, len(toolboxes)) + matched := 0 + for _, toolbox := range toolboxes { + key := envkey.ToolboxMCPEndpoint(toolbox.Name) + if _, duplicate := seen[key]; duplicate { + continue + } + seen[key] = struct{}{} + if _, ok := missingKeys[key]; !ok { + matched++ + } + } + return classifyToolboxResults(missingUnique, matched) +} + +func classifyToolboxResults( + missing []nextstep.ResourceRef, + matched int, +) Result { + if len(missing) == 0 { + return Result{ + Status: StatusPass, + Message: fmt.Sprintf("all %d declared toolbox(es) have an MCP endpoint set.", matched), + Details: map[string]any{"matchedCount": matched}, + } + } - return classifyToolboxEndpoints(ctx, state.Toolboxes, lookup) + slices.SortFunc(missing, func(a, b nextstep.ResourceRef) int { + if a.Name != b.Name { + return strings.Compare(a.Name, b.Name) + } + return strings.Compare(a.ServiceName, b.ServiceName) + }) + var names []string + hasSplit := false + hasLegacy := false + for _, toolbox := range missing { + names = append(names, fmt.Sprintf("%s (env %s, service %s)", + toolbox.Name, envkey.ToolboxMCPEndpoint(toolbox.Name), toolbox.ServiceName)) + hasSplit = hasSplit || toolbox.ToolboxSource == nextstep.ToolboxSourceSplit + hasLegacy = hasLegacy || toolbox.ToolboxSource != nextstep.ToolboxSourceSplit + } + suggestion := "Run `azd provision` to materialize toolbox infrastructure, or " + + "`azd env set ` to point at an existing toolbox." + switch { + case hasSplit && hasLegacy: + suggestion = "Run `azd deploy` for split toolbox services and `azd provision` " + + "for legacy toolbox resources, or set an existing endpoint." + case hasSplit: + suggestion = "Run `azd deploy` to materialize split toolbox services." + } + return Result{ + Status: StatusFail, + Message: fmt.Sprintf("%d declared toolbox(es) have no MCP endpoint set in the azd environment: %s", + len(missing), strings.Join(names, ", ")), + Suggestion: suggestion, + Details: map[string]any{ + "missingToolboxes": missing, + "matchedCount": matched, }, } } diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go index 14f7fa91b2a..ac166b3f279 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go @@ -117,7 +117,7 @@ func TestCheckToolboxes_SkipsWhenNoToolboxesDeclared(t *testing.T) { } res := runToolboxesCheck(t, deps, nil) require.Equal(t, StatusSkip, res.Status) - require.Contains(t, res.Message, "no toolbox resources") + require.Contains(t, res.Message, "no configured toolbox resources") } func TestCheckToolboxes_FailsWhenAssemblerReturnsNilState(t *testing.T) { @@ -235,12 +235,78 @@ func TestCheckToolboxes_FailsOnEnvLookupTransportError(t *testing.T) { return "", wantErr }, } + res := runToolboxesCheck(t, deps, nil) require.Equal(t, StatusFail, res.Status, "transport errors must Fail (not Skip) so the user has an actionable signal") require.Contains(t, res.Message, "connection refused") require.Contains(t, res.Suggestion, "azd env") } +func TestCheckToolboxes_UsesAssembledStateWithoutLookup(t *testing.T) { + t.Parallel() + + state := stateWithToolboxes(nextstep.ResourceRef{ + Name: "split-tools", + ServiceName: "split-tools", + ToolboxSource: nextstep.ToolboxSourceSplit, + }) + state.ToolboxEndpointsChecked = true + state.MissingToolboxEndpoints = []nextstep.ResourceRef{state.Toolboxes[0]} + deps := Dependencies{ + AzdClient: &azdext.AzdClient{}, + assembleState: fixedAssembler(state), + lookupToolboxEnv: func(context.Context, string) (string, error) { + t.Fatal("assembled endpoint state must not be probed again") + return "", nil + }, + } + + res := runToolboxesCheck(t, deps, nil) + require.Equal(t, StatusFail, res.Status) + require.Contains(t, res.Suggestion, "azd deploy") +} + +func TestCheckToolboxes_AssembledEndpointErrorFails(t *testing.T) { + t.Parallel() + + state := stateWithToolboxes(nextstep.ResourceRef{Name: "split-tools"}) + state.ToolboxEndpointsChecked = true + state.ToolboxEndpointErrors = []string{"read toolbox endpoint: connection refused"} + res := runToolboxesCheck(t, Dependencies{ + AzdClient: &azdext.AzdClient{}, + assembleState: fixedAssembler(state), + }, nil) + require.Equal(t, StatusFail, res.Status) + require.Contains(t, res.Message, "connection refused") +} + +func TestCheckToolboxes_MixedSourcesShowBothRemediations(t *testing.T) { + t.Parallel() + + state := stateWithToolboxes( + nextstep.ResourceRef{ + Name: "split-tools", + ServiceName: "split-tools", + ToolboxSource: nextstep.ToolboxSourceSplit, + }, + nextstep.ResourceRef{ + Name: "legacy-tools", + ServiceName: "agent", + ToolboxSource: nextstep.ToolboxSourceLegacyManifest, + }, + ) + state.ToolboxEndpointsChecked = true + state.MissingToolboxEndpoints = state.Toolboxes + + res := runToolboxesCheck(t, Dependencies{ + AzdClient: &azdext.AzdClient{}, + assembleState: fixedAssembler(state), + }, nil) + require.Equal(t, StatusFail, res.Status) + require.Contains(t, res.Suggestion, "azd deploy") + require.Contains(t, res.Suggestion, "azd provision") +} + // ---- Dedup on canonical env key ---- func TestCheckToolboxes_dedupsSameToolboxAcrossServices(t *testing.T) { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache.go new file mode 100644 index 00000000000..cc856da9371 --- /dev/null +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache.go @@ -0,0 +1,48 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package doctor + +import ( + "context" + "sync" + + "azureaiagent/internal/cmd/nextstep" + + "github.com/azure/azure-dev/cli/azd/pkg/azdext" +) + +// StateCache stores one assembled state snapshot for a Doctor run. +type StateCache struct { + once sync.Once + state *nextstep.State + errs []error +} + +// NewStateCache creates a cache for one Doctor invocation. +func NewStateCache() *StateCache { + return &StateCache{} +} + +// AssembleAgentState returns the cached state or assembles it once. +func (deps Dependencies) AssembleAgentState(ctx context.Context) (*nextstep.State, []error) { + cache := deps.StateCache + if cache == nil { + cache = NewStateCache() + } + + cache.once.Do(func() { + assembler := deps.assembleState + if assembler == nil { + assembler = func( + c context.Context, + client *azdext.AzdClient, + ) (*nextstep.State, []error) { + return nextstep.AssembleState(c, client) + } + } + cache.state, cache.errs = assembler(ctx, deps.AzdClient) + }) + + return cache.state, cache.errs +} diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache_test.go new file mode 100644 index 00000000000..84d0095cd8b --- /dev/null +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache_test.go @@ -0,0 +1,39 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package doctor + +import ( + "context" + "testing" + + "azureaiagent/internal/cmd/nextstep" + + "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/stretchr/testify/require" +) + +func TestAssembleAgentStateCachesSnapshot(t *testing.T) { + t.Parallel() + + var calls int + state := &nextstep.State{HasProjectEndpoint: true} + deps := Dependencies{ + StateCache: NewStateCache(), + assembleState: func(_ context.Context, _ *azdext.AzdClient) ( + *nextstep.State, []error, + ) { + calls++ + return state, nil + }, + } + + first, firstErrs := deps.AssembleAgentState(t.Context()) + second, secondErrs := deps.AssembleAgentState(t.Context()) + + require.Same(t, state, first) + require.Same(t, first, second) + require.Empty(t, firstErrs) + require.Empty(t, secondErrs) + require.Equal(t, 1, calls) +} diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go index 61128166ce5..64a57360847 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go @@ -9,7 +9,10 @@ import ( "slices" "azureaiagent/internal/pkg/agents/agent_yaml" + "azureaiagent/internal/pkg/envkey" "azureaiagent/internal/pkg/paths" + + "github.com/azure/azure-dev/cli/azd/pkg/azdext" ) // manifestFileNames are the candidate manifest filenames the walker @@ -94,8 +97,9 @@ func populateManifestResources(projectPath string, state *State) { continue } toolboxes[k] = ResourceRef{ - Name: r.Name, - ServiceName: svc.Name, + Name: r.Name, + ServiceName: svc.Name, + ToolboxSource: ToolboxSourceLegacyManifest, } case agent_yaml.ConnectionResource: if r.Name == "" { @@ -122,6 +126,64 @@ func populateManifestResources(projectPath string, state *State) { state.HasConnections = len(state.Connections) > 0 } +// populateSplitToolboxes adds standalone azure.ai.toolbox services to state. +// A split service's service name is both its toolbox name and owner name. +// No properties are decoded: the service name is the authoritative toolbox +// identity, so local $ref files cannot affect discovery. +func populateSplitToolboxes( + projectCfg *azdext.ProjectConfig, + state *State, +) { + if projectCfg == nil || state == nil { + return + } + + split := make(map[string]ResourceRef) + for serviceName, svc := range projectCfg.Services { + if svc == nil || svc.GetHost() != "azure.ai.toolbox" { + continue + } + if svc.GetName() != "" { + serviceName = svc.GetName() + } + if serviceName == "" { + continue + } + ref := ResourceRef{ + Name: serviceName, + ServiceName: serviceName, + ToolboxSource: ToolboxSourceSplit, + } + key := envkey.ToolboxMCPEndpoint(ref.Name) + if prior, ok := split[key]; !ok || ref.ServiceName < prior.ServiceName { + split[key] = ref + } + } + if len(split) == 0 { + return + } + + // A split service is authoritative for a canonical endpoint key. Keep + // unrelated legacy manifest keys for compatibility. + merged := make([]ResourceRef, 0, len(state.Toolboxes)+len(split)) + for _, ref := range state.Toolboxes { + if _, replaced := split[envkey.ToolboxMCPEndpoint(ref.Name)]; !replaced { + merged = append(merged, ref) + } + } + for _, ref := range split { + merged = append(merged, ref) + } + slices.SortFunc(merged, func(a, b ResourceRef) int { + if c := cmp.Compare(a.Name, b.Name); c != 0 { + return c + } + return cmp.Compare(a.ServiceName, b.ServiceName) + }) + state.Toolboxes = merged + state.HasToolboxes = len(merged) > 0 +} + // readManifestBytes returns the first manifest file's contents under // `//` (probing the names in // manifestFileNames order) or nil if none exists / is readable. All diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go index 47daeaeb4bd..87f46a6010e 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go @@ -179,6 +179,11 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) hasToolboxEndpoints := len(state.MissingToolboxEndpoints) > 0 hasManualVars := len(state.MissingManualVars) > 0 + hasToolboxEndpointErrors := len(state.ToolboxEndpointErrors) > 0 + hasSplitToolboxEndpoints := hasMissingToolboxSource( + state.MissingToolboxEndpoints, ToolboxSourceSplit) + hasLegacyToolboxEndpoints := hasMissingLegacyToolbox( + state.MissingToolboxEndpoints) needsProvision := len(state.PendingProvisionReasons) > 0 || !state.HasProjectEndpoint || @@ -199,7 +204,7 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) Description: "set up your Foundry project, models, and connections", Priority: priority, }) - case hasToolboxEndpoints || hasManualVars: + case hasToolboxEndpoints || hasToolboxEndpointErrors || hasManualVars: // Combined branch for the two "things the user has to fix before // running locally" categories. They are intentionally additive // (not mutually exclusive) so a manifest that declares a @@ -223,15 +228,33 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) // toolbox); ResolveAfterInit is offline by contract and must // not initiate Foundry API calls. if hasToolboxEndpoints { - out = append(out, Suggestion{ - Command: "azd provision", - Description: "create your toolbox(es) in Foundry", - Priority: priority, - }) - priority++ + if hasSplitToolboxEndpoints { + out = append(out, Suggestion{ + Command: "azd deploy", + Description: "deploy split toolbox services", + Priority: priority, + }) + priority++ + } + if hasLegacyToolboxEndpoints { + out = append(out, Suggestion{ + Command: "azd provision", + Description: "create your toolbox(es) in Foundry", + Priority: priority, + }) + priority++ + out = append(out, Suggestion{ + Command: "azd ai agent doctor", + Description: "(optional) check whether your toolbox(es) already exist in Foundry", + Priority: priority, + }) + priority++ + } + } + if hasToolboxEndpointErrors { out = append(out, Suggestion{ Command: "azd ai agent doctor", - Description: "(optional) check whether your toolbox(es) already exist in Foundry", + Description: "check toolbox endpoint access before running locally", Priority: priority, }) priority++ @@ -260,11 +283,17 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) // literal `{{NAME}}` values in the agent configuration still // break the local agent — the user must finish the placeholder // fix-ups first; the trailing `azd deploy` reminder still applies. - if !hasPlaceholders { + if !hasPlaceholders && !hasToolboxEndpointErrors { out = append(out, Suggestion{ - Command: "azd ai agent run", - Description: runFollowUpDescription(hasToolboxEndpoints, hasManualVars), - Priority: priority, + Command: "azd ai agent run", + Description: runFollowUpDescription( + hasToolboxEndpoints, + hasManualVars, + hasSplitToolboxEndpoints, + hasLegacyToolboxEndpoints, + hasToolboxEndpointErrors, + ), + Priority: priority, }) priority++ out, _ = appendInvokeLocalSecondary(out, state, readmeExists, priority) @@ -288,12 +317,16 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) out, _ = appendInvokeLocalSecondary(out, state, readmeExists, priority) } - out = append(out, Suggestion{ - Command: "azd deploy", - Description: "when ready to deploy to Azure", - Priority: 90, - Trailing: true, - }) + if !slices.ContainsFunc(out, func(s Suggestion) bool { + return strings.TrimSpace(s.Command) == "azd deploy" + }) { + out = append(out, Suggestion{ + Command: "azd deploy", + Description: "when ready to deploy to Azure", + Priority: 90, + Trailing: true, + }) + } if state.EnvironmentName != "" { for i := range out { @@ -304,6 +337,24 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) return out } +func hasMissingToolboxSource(toolboxes []ResourceRef, source ToolboxSource) bool { + for _, toolbox := range toolboxes { + if toolbox.ToolboxSource == source { + return true + } + } + return false +} + +func hasMissingLegacyToolbox(toolboxes []ResourceRef) bool { + for _, toolbox := range toolboxes { + if toolbox.ToolboxSource != ToolboxSourceSplit { + return true + } + } + return false +} + // qualifyCommandEnvironment targets an azd command at an explicitly selected // environment. Non-azd guidance (for example cd/edit/see commands) is returned // unchanged. @@ -318,10 +369,23 @@ func qualifyCommandEnvironment(command, environmentName string) string { // `azd ai agent run` follow-up emitted after the toolbox / manual-vars // branch, so the suffix reflects which categories of work the user // still has to complete first. -func runFollowUpDescription(hasToolboxEndpoints, hasManualVars bool) string { +func runFollowUpDescription( + hasToolboxEndpoints, hasManualVars, hasSplitToolboxEndpoints, + hasLegacyToolboxEndpoints, hasToolboxEndpointErrors bool, +) string { switch { + case hasToolboxEndpointErrors: + return "start the agent locally once toolbox endpoint checks pass" + case hasToolboxEndpoints && + hasSplitToolboxEndpoints && + hasLegacyToolboxEndpoints: + return "start the agent locally once the steps above are complete" + case hasSplitToolboxEndpoints && hasToolboxEndpoints && hasManualVars: + return "start the agent locally once deployment and env values are ready" case hasToolboxEndpoints && hasManualVars: return "start the agent locally once the steps above are complete" + case hasSplitToolboxEndpoints && hasToolboxEndpoints: + return "start the agent locally once deployment completes" case hasToolboxEndpoints: return "start the agent locally once provision completes" case hasManualVars: diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go index c3082c28ad5..b3b8cbebd1c 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go @@ -532,6 +532,63 @@ func TestResolveAfterInit_ToolboxEndpointsEmitsRunAndInvokeLocal(t *testing.T) { assert.Contains(t, rendered, "azd deploy", "trailing deploy reminder missing") } +func TestResolveAfterInit_SplitToolboxUsesDeployOnce(t *testing.T) { + t.Parallel() + + state := &State{ + HasProjectEndpoint: true, + MissingToolboxEndpoints: []ResourceRef{{ + Name: "split-tools", + ServiceName: "split-tools", + ToolboxSource: ToolboxSourceSplit, + }}, + } + + suggestions := ResolveAfterInit(state, nil) + var deployCount int + for _, suggestion := range suggestions { + if suggestion.Command == "azd deploy" { + deployCount++ + } + assert.NotContains(t, suggestion.Command, "azd provision") + assert.NotContains(t, suggestion.Command, "azd env set") + } + assert.Equal(t, 1, deployCount) +} + +func TestResolveAfterInit_SplitToolboxDoesNotSkipProvision(t *testing.T) { + t.Parallel() + + state := &State{ + MissingToolboxEndpoints: []ResourceRef{{ + Name: "split-tools", + ServiceName: "split-tools", + ToolboxSource: ToolboxSourceSplit, + }}, + } + + suggestions := ResolveAfterInit(state, nil) + require.NotEmpty(t, suggestions) + assert.Equal(t, "azd provision", suggestions[0].Command) +} + +func TestResolveAfterInit_ToolboxEndpointErrorBlocksLocalRun(t *testing.T) { + t.Parallel() + + state := &State{ + HasProjectEndpoint: true, + ToolboxEndpointErrors: []string{"read toolbox endpoint: grpc unavailable"}, + } + + var buf strings.Builder + require.NoError(t, PrintAllNext(&buf, ResolveAfterInit(state, nil))) + rendered := buf.String() + assert.Contains(t, rendered, "azd ai agent doctor") + assert.Contains(t, rendered, "check toolbox endpoint access") + assert.NotContains(t, rendered, "azd ai agent run") + assert.NotContains(t, rendered, "azd ai agent invoke --local") +} + // TestResolveAfterInit_ToolboxAndManualVarsCoexist locks the bug both // reviewers caught: when MissingToolboxEndpoints AND MissingManualVars // are populated, the previously-exclusive switch hid the manual @@ -629,10 +686,13 @@ func TestRunFollowUpDescription(t *testing.T) { t.Parallel() tests := []struct { - name string - hasToolboxEndpoint bool - hasManualVars bool - want string + name string + hasToolboxEndpoint bool + hasManualVars bool + hasSplitToolboxEndpoint bool + hasLegacyToolboxEndpoint bool + hasEndpointErrors bool + want string }{ { name: "both", @@ -641,7 +701,20 @@ func TestRunFollowUpDescription(t *testing.T) { want: "start the agent locally once the steps above are complete", }, { - name: "toolbox only", + name: "split toolbox only", + hasToolboxEndpoint: true, + hasSplitToolboxEndpoint: true, + want: "start the agent locally once deployment completes", + }, + { + name: "mixed split and legacy toolboxes", + hasToolboxEndpoint: true, + hasSplitToolboxEndpoint: true, + hasLegacyToolboxEndpoint: true, + want: "start the agent locally once the steps above are complete", + }, + { + name: "legacy toolbox only", hasToolboxEndpoint: true, want: "start the agent locally once provision completes", }, @@ -650,6 +723,11 @@ func TestRunFollowUpDescription(t *testing.T) { hasManualVars: true, want: "start the agent locally once the values above are set", }, + { + name: "endpoint error", + hasEndpointErrors: true, + want: "start the agent locally once toolbox endpoint checks pass", + }, { // Defensive fallthrough — unreachable from ResolveAfterInit's // combined case (guarded by `hasToolboxEndpoints || @@ -661,7 +739,13 @@ func TestRunFollowUpDescription(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got := runFollowUpDescription(tc.hasToolboxEndpoint, tc.hasManualVars) + got := runFollowUpDescription( + tc.hasToolboxEndpoint, + tc.hasManualVars, + tc.hasSplitToolboxEndpoint, + tc.hasLegacyToolboxEndpoint, + tc.hasEndpointErrors, + ) assert.Equal(t, tc.want, got) }) } diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go index 7f9bbc08bbf..f9f3ed6a22b 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go @@ -299,27 +299,26 @@ func assembleState(ctx context.Context, src Source, opts ...Option) (*State, []e &state.EnvironmentLoadErrors, ) + if project != nil { + if len(state.Services) > 0 { + populateManifestResources(project.Path, state) + } + populateSplitToolboxes(project, state) + } + if project != nil && envName != "" { state.MissingInfraVars, state.MissingManualVars, state.UnresolvedPlaceholders = detectMissingVars( - ctx, src, envName, project.Path, state.Services, &errs, + ctx, src, envName, project.Path, state.Services, state.Toolboxes, &errs, ) populateOpenAPIPayload(ctx, cfg, project.Path, envName, state) } - if project != nil && len(state.Services) > 0 { - populateManifestResources(project.Path, state) + if envName != "" && len(state.Toolboxes) > 0 { + state.MissingToolboxEndpoints = probeToolboxEndpoints( + ctx, src, envName, state.Toolboxes, &state.ToolboxEndpointErrors, &errs) + state.ToolboxEndpointsChecked = true } - // Partition toolbox-derived endpoint vars out of MissingManualVars - // into MissingToolboxEndpoints. This must run AFTER - // populateManifestResources because it depends on state.Toolboxes - // being populated — without the manifest's toolbox list we cannot - // tell a toolbox-derived var ("TOOLBOX_WEB_SEARCH_TOOLS_MCP_ENDPOINT" - // for a manifest-declared `web-search-tools` toolbox) apart from a - // generic user-named variable that happens to start with TOOLBOX_. - // See MissingToolboxEndpoints docs (types.go) for the rationale. - partitionToolboxEndpointVars(state) - return state, errs } @@ -340,60 +339,43 @@ func detectMissingAzureContextVars(ctx context.Context, src Source, envName stri return missing } -// partitionToolboxEndpointVars moves any entry in state.MissingManualVars -// whose name is the canonical TOOLBOX__MCP_ENDPOINT key for a -// manifest-declared toolbox into state.MissingToolboxEndpoints. The -// partition is a no-op when state.Toolboxes is empty: any TOOLBOX_* -// entry in MissingManualVars without a corresponding manifest toolbox -// is a generic user variable and stays where it is. -// -// state.MissingManualVars order is preserved (caller-visible sorting -// happens in the resolver). The matched ResourceRefs are then sorted -// by (Name, ServiceName) before being written to MissingToolboxEndpoints -// so callers see a stable ordering that matches state.Toolboxes regardless -// of how MissingManualVars happens to be ordered. -func partitionToolboxEndpointVars(state *State) { - if len(state.MissingManualVars) == 0 || len(state.Toolboxes) == 0 { - return - } - - // keyToToolbox maps each declared toolbox's canonical endpoint key - // to its ResourceRef. envkey.ToolboxMCPEndpoint is the single - // source of truth for the key normalization (sanitize → upper → - // "TOOLBOX__MCP_ENDPOINT") shared with the provisioner and the - // local.toolboxes doctor check; computing the lookup here ensures - // any future normalization change ripples consistently. - keyToToolbox := make(map[string]ResourceRef, len(state.Toolboxes)) - for _, tb := range state.Toolboxes { - keyToToolbox[envkey.ToolboxMCPEndpoint(tb.Name)] = tb - } - - remaining := make([]string, 0, len(state.MissingManualVars)) - var matched []ResourceRef - for _, name := range state.MissingManualVars { - if tb, ok := keyToToolbox[name]; ok { - matched = append(matched, tb) +// probeToolboxEndpoints reads each canonical toolbox endpoint once. Endpoint +// values are produced by azd, so this probe is independent of agent env refs. +func probeToolboxEndpoints( + ctx context.Context, + src Source, + envName string, + toolboxes []ResourceRef, + endpointErrors *[]string, + errs *[]error, +) []ResourceRef { + seen := make(map[string]struct{}, len(toolboxes)) + var missing []ResourceRef + for _, toolbox := range toolboxes { + key := envkey.ToolboxMCPEndpoint(toolbox.Name) + if _, ok := seen[key]; ok { continue } - remaining = append(remaining, name) - } - if len(matched) == 0 { - return + seen[key] = struct{}{} + value, err := src.EnvValue(ctx, envName, key) + if err != nil { + probeErr := fmt.Errorf("read toolbox endpoint %s: %w", key, err) + *endpointErrors = append(*endpointErrors, probeErr.Error()) + *errs = append(*errs, probeErr) + continue + } + if strings.TrimSpace(value) == "" { + missing = append(missing, toolbox) + } } - - state.MissingManualVars = remaining - // Sort matched by (Name, ServiceName) for deterministic rendering; - // state.Toolboxes is already sorted but `matched` was built by - // MissingManualVars iteration order, which is sorted by var name - // rather than toolbox name. Re-sort so callers see the same - // ordering they'd see if they iterated state.Toolboxes directly. - slices.SortFunc(matched, func(a, b ResourceRef) int { + slices.SortFunc(missing, func(a, b ResourceRef) int { if c := strings.Compare(a.Name, b.Name); c != 0 { return c } return strings.Compare(a.ServiceName, b.ServiceName) }) - state.MissingToolboxEndpoints = matched + slices.Sort(*endpointErrors) + return missing } // populateOpenAPIPayload locates a sample invoke payload for the @@ -658,6 +640,7 @@ func detectMissingVars( src Source, envName, projectPath string, services []ServiceState, + toolboxes []ResourceRef, errs *[]error, ) (infra, manual, placeholders []string) { if envName == "" || projectPath == "" || len(services) == 0 { @@ -668,10 +651,17 @@ func detectMissingVars( seenInfra := make(map[string]struct{}) seenManual := make(map[string]struct{}) seenPlaceholder := make(map[string]struct{}) + toolboxKeys := make(map[string]struct{}, len(toolboxes)) + for _, toolbox := range toolboxes { + toolboxKeys[envkey.ToolboxMCPEndpoint(toolbox.Name)] = struct{}{} + } for _, svc := range services { refs, phs := extractEnvironmentRefs(svc.EnvironmentValues) for _, name := range refs { + if _, isToolbox := toolboxKeys[name]; isToolbox { + continue + } if _, ok := seenInfra[name]; ok { continue } diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go index 94c3a2bec84..a1c955eb44b 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go @@ -21,12 +21,14 @@ import ( // fakeSource is a hand-rolled Source for table-driven tests. type fakeSource struct { - envName string - envNameErr error - project *azdext.ProjectConfig - projectErr error - values map[string]string - valueErr error + envName string + envNameErr error + project *azdext.ProjectConfig + projectErr error + values map[string]string + valueErr error + valueErrors map[string]error + calls map[string]int } func (f *fakeSource) CurrentEnvName(_ context.Context) (string, error) { @@ -38,12 +40,131 @@ func (f *fakeSource) Project(_ context.Context) (*azdext.ProjectConfig, error) { } func (f *fakeSource) EnvValue(_ context.Context, envName, key string) (string, error) { + if f.calls != nil { + f.calls[envName+"/"+key]++ + } if f.valueErr != nil { return "", f.valueErr } + if err := f.valueErrors[envName+"/"+key]; err != nil { + return "", err + } + return f.values[envName+"/"+key], nil } +func TestAssembleState_SplitToolboxesProbeCanonicalEndpoints(t *testing.T) { + t.Parallel() + + src := &fakeSource{ + envName: "dev", + values: map[string]string{ + "dev/TOOLBOX_ALPHA_MCP_ENDPOINT": "https://alpha.example/mcp", + "dev/TOOLBOX_ALPHA_COPY_MCP_ENDPOINT": "https://alpha-copy.example/mcp", + }, + calls: make(map[string]int), + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "alpha": { + Name: "alpha", Host: "azure.ai.toolbox", + }, + "alpha-copy": { + Name: "alpha-copy", Host: "azure.ai.toolbox", + }, + }, + }, + } + + state, errs := assembleState(t.Context(), src) + require.Empty(t, errs) + require.True(t, state.ToolboxEndpointsChecked) + require.Len(t, state.Toolboxes, 2) + require.Equal(t, ToolboxSourceSplit, state.Toolboxes[0].ToolboxSource) + require.Equal(t, "alpha", state.Toolboxes[0].Name) + require.Empty(t, state.MissingToolboxEndpoints) + require.Equal(t, 1, src.calls["dev/TOOLBOX_ALPHA_MCP_ENDPOINT"]) + require.Equal(t, 1, src.calls["dev/TOOLBOX_ALPHA_COPY_MCP_ENDPOINT"]) +} + +func TestAssembleState_SplitToolboxMissingEndpointIsNotManual(t *testing.T) { + t.Parallel() + + src := &fakeSource{ + envName: "dev", + values: map[string]string{ + "dev/TOOLBOX_OTHER_MCP_ENDPOINT": "https://other.example/mcp", + }, + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "missing": { + Name: "missing", Host: "azure.ai.toolbox", + }, + "other": { + Name: "other", Host: "azure.ai.toolbox", + }, + }, + }, + } + + state, errs := assembleState(t.Context(), src) + require.Empty(t, errs) + require.Empty(t, state.MissingManualVars) + require.Len(t, state.MissingToolboxEndpoints, 1) + require.Equal(t, "missing", state.MissingToolboxEndpoints[0].Name) +} + +func TestAssembleState_SplitToolboxEndpointErrorIsSurfaced(t *testing.T) { + t.Parallel() + + wantErr := errors.New("grpc: connection refused") + src := &fakeSource{ + envName: "dev", + valueErrors: map[string]error{ + "dev/TOOLBOX_MISSING_MCP_ENDPOINT": wantErr, + }, + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "missing": { + Name: "missing", Host: "azure.ai.toolbox", + }, + }, + }, + } + + state, errs := assembleState(t.Context(), src) + require.True(t, state.ToolboxEndpointsChecked) + require.Empty(t, state.MissingToolboxEndpoints) + require.Equal(t, + []string{"read toolbox endpoint TOOLBOX_MISSING_MCP_ENDPOINT: grpc: connection refused"}, + state.ToolboxEndpointErrors, + ) + require.Len(t, errs, 1) + require.ErrorContains(t, errs[0], "grpc: connection refused") +} + +func TestPopulateSplitToolboxes_PrefersSplitCanonicalKey(t *testing.T) { + t.Parallel() + + state := &State{ + Toolboxes: []ResourceRef{ + {Name: "my+tool", ServiceName: "agent", ToolboxSource: ToolboxSourceLegacyManifest}, + {Name: "legacy", ServiceName: "agent", ToolboxSource: ToolboxSourceLegacyManifest}, + }, + } + project := &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "my-tool": {Name: "my-tool", Host: "azure.ai.toolbox"}, + }, + } + + populateSplitToolboxes(project, state) + require.Len(t, state.Toolboxes, 2) + require.Equal(t, "legacy", state.Toolboxes[0].Name) + require.Equal(t, ToolboxSourceLegacyManifest, state.Toolboxes[0].ToolboxSource) + require.Equal(t, "my-tool", state.Toolboxes[1].Name) + require.Equal(t, ToolboxSourceSplit, state.Toolboxes[1].ToolboxSource) +} + func TestAssembleState(t *testing.T) { t.Parallel() diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go index 53f28987a2a..035fe7ee48f 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go @@ -91,30 +91,29 @@ type State struct { // MissingManualVars names ${...} references that map to user-supplied // variables which are not set in the azd environment. // - // Toolbox-derived endpoint variables (`TOOLBOX__MCP_ENDPOINT` - // keys that correspond to a manifest-declared toolbox) are - // partitioned out into MissingToolboxEndpoints — they are - // azd-managed outputs of `azd provision`, not operator-supplied, - // and routing them to `azd env set` is misleading. + // Toolbox endpoint variables are partitioned out into + // MissingToolboxEndpoints — they are azd-managed outputs, not + // operator-supplied values. MissingManualVars []string - // MissingToolboxEndpoints lists manifest-declared toolboxes whose - // azd-injected TOOLBOX__MCP_ENDPOINT variable is unset in the - // active azd environment. AssembleState partitions these out of - // MissingManualVars because they are produced by - // `azd provision` (listen.go::registerToolboxEnvVars), not by the - // user — the right remediation is `azd provision` (which creates - // the toolbox in the Foundry project on first run and sets the - // derived env var), not `azd env set`. + // MissingToolboxEndpoints lists collected toolboxes whose + // TOOLBOX__MCP_ENDPOINT variable is unset in the active + // azd environment. Split services are produced by `azd deploy`; + // legacy manifest toolboxes retain their existing remediation. // - // Each entry carries the manifest's resource Name and the owning - // ServiceName so the resolver and doctor checks can render - // per-service guidance. The Detail field is unused (toolbox - // endpoints have no kind-specific identifier beyond Name) but the - // shared ResourceRef shape keeps the renderer code uniform with - // state.Toolboxes / state.ModelRefs / state.Connections. + // Each entry carries the resource Name and owning ServiceName so + // resolver and doctor checks can render actionable guidance. The + // shared ResourceRef shape keeps rendering uniform across resources. MissingToolboxEndpoints []ResourceRef + // ToolboxEndpointErrors contains endpoint probe failures from state + // assembly. The doctor check reports these without probing again. + ToolboxEndpointErrors []string + + // ToolboxEndpointsChecked reports that assembly probed every collected + // toolbox endpoint, including endpoints not referenced by agent config. + ToolboxEndpointsChecked bool + // UnresolvedPlaceholders names {{NAME}} Mustache-style placeholders // still present inside an agent configuration environment // value. These are left over from init's manifest processing when @@ -155,13 +154,11 @@ type State struct { // prepend a `cd ` suggestion to the Next: block. CreatedFolderDisplay string - // HasModels, HasToolboxes, HasConnections are aggregate flags - // derived from each azure.ai.agent service's agent.manifest.yaml - // (when present). They are true when at least one resource of the - // matching kind is declared across all services. Doctor checks that - // only make sense in the presence of these resources gate-skip - // themselves on the matching Has* flag; resolvers can use them to - // tailor remediation suggestions. + // HasModels, HasToolboxes, HasConnections are aggregate flags. + // Models and connections currently come from agent manifests. + // Toolboxes include configured split services as well as manifest + // resources. Doctor checks gate-skip when no matching resource is + // present; resolvers can tailor remediation suggestions. // // All three flags are false when the manifest file is missing, // malformed, or declares no resources — the walker is deliberately @@ -171,12 +168,10 @@ type State struct { HasToolboxes bool HasConnections bool - // ModelRefs, Toolboxes, Connections list every resource of the - // matching kind found across all services' agent.manifest.yaml - // files. Entries are sorted by Name (ties broken by ServiceName) - // and deduplicated on (ServiceName, Name) so callers can render - // them deterministically. The slices are nil when the matching - // Has* flag is false. + // ModelRefs, Toolboxes, Connections list collected resources. + // ModelRefs and Connections remain manifest-derived for now. + // Entries are sorted by Name (ties broken by ServiceName) so + // callers can render them deterministically. ModelRefs []ResourceRef Toolboxes []ResourceRef Connections []ResourceRef @@ -191,18 +186,13 @@ type State struct { // boundary. Add fields here only when a doctor check or resolver // branch needs them. type ResourceRef struct { - // Name is the resource's manifest-declared name (the `name:` - // field on the manifest's `resources[]` entry). Doctor checks - // match by this name when looking up Foundry deployments / - // connections / toolboxes. + // Name is the resource's configured name. Doctor checks match by + // this name when looking up deployments, connections, or toolboxes. Name string - // ServiceName is the azd service that declared the resource (the - // service entry under `services:` in azure.yaml whose - // agent.manifest.yaml contains this entry). When the same logical - // resource is declared by multiple services they appear as - // separate entries — doctor checks key on (ServiceName, Name) so - // per-service failures are surfaced individually. + // ServiceName is the azd service that declared the resource. When + // the same logical resource is declared by multiple services they + // appear as separate entries. ServiceName string // Detail carries a kind-specific identifier: @@ -212,8 +202,25 @@ type ResourceRef struct { // Doctor remediation messages render Detail verbatim, so changes // here must match the doctor-message contract. Detail string + + // ToolboxSource identifies how a toolbox was discovered. + ToolboxSource ToolboxSource } +// ToolboxSource identifies the configuration source of a toolbox. +type ToolboxSource int + +const ( + // ToolboxSourceUnknown is the zero-value source. + ToolboxSourceUnknown ToolboxSource = iota + // ToolboxSourceBundled is an agent service toolbox. + ToolboxSourceBundled + // ToolboxSourceSplit is a standalone azure.ai.toolbox service. + ToolboxSourceSplit + // ToolboxSourceLegacyManifest is an agent manifest toolbox. + ToolboxSourceLegacyManifest +) + // ServiceState mirrors one entry from the project's services map, plus a // deployment marker derived from azd environment variables. IsDeployed is // true when AGENT__VERSION is non-empty in the active environment, From 84d7d1c0c9ae2f0cf5e54273361ac449c92d8e38 Mon Sep 17 00:00:00 2001 From: huimiu Date: Thu, 13 Aug 2026 17:04:51 +0800 Subject: [PATCH 2/7] fix: retain toolbox environment fallback --- .../internal/cmd/doctor/checks_toolboxes.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go index 489fc1265f7..94cb31de59d 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go @@ -135,10 +135,11 @@ func newCheckToolboxes(deps Dependencies) Check { // Keep the old seam for callers constructing partial states. Production // assembly always sets ToolboxEndpointsChecked and never reads again. - if deps.lookupToolboxEnv != nil { - return classifyToolboxEndpoints(ctx, state.Toolboxes, deps.lookupToolboxEnv) + lookup := deps.lookupToolboxEnv + if lookup == nil { + lookup = makeRealToolboxEnvLookup(deps.AzdClient) } - return classifyToolboxState(state.Toolboxes, state.MissingToolboxEndpoints) + return classifyToolboxEndpoints(ctx, state.Toolboxes, lookup) }, } } From a3598f195ba1be9039880e1c51711f37ccf208a7 Mon Sep 17 00:00:00 2001 From: huimiu Date: Thu, 13 Aug 2026 22:24:51 +0800 Subject: [PATCH 3/7] fix: preserve toolbox doctor JSON fields --- .../internal/cmd/doctor/checks_toolboxes.go | 26 +++++++++++----- .../cmd/doctor/checks_toolboxes_test.go | 30 +++++++++++++++++++ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go index 94cb31de59d..f235cae0656 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go @@ -214,12 +214,30 @@ func classifyToolboxResults( len(missing), strings.Join(names, ", ")), Suggestion: suggestion, Details: map[string]any{ - "missingToolboxes": missing, + "missingToolboxes": toolboxLookupDetails(missing), "matchedCount": matched, }, } } +type toolboxLookup struct { + Name string `json:"name"` + ServiceName string `json:"service"` + EnvVar string `json:"envVar"` +} + +func toolboxLookupDetails(toolboxes []nextstep.ResourceRef) []toolboxLookup { + details := make([]toolboxLookup, 0, len(toolboxes)) + for _, toolbox := range toolboxes { + details = append(details, toolboxLookup{ + Name: toolbox.Name, + ServiceName: toolbox.ServiceName, + EnvVar: envkey.ToolboxMCPEndpoint(toolbox.Name), + }) + } + return details +} + // normalizeToolboxName / toolboxEndpointKey have been replaced by the // shared `internal/pkg/envkey` package. See envkey.ToolboxMCPEndpoint. @@ -239,12 +257,6 @@ func classifyToolboxEndpoints( toolboxes []nextstep.ResourceRef, lookup toolboxEnvLookupFn, ) Result { - type toolboxLookup struct { - Name string `json:"name"` - ServiceName string `json:"service"` - EnvVar string `json:"envVar"` - } - seen := make(map[string]struct{}, len(toolboxes)) var missing []toolboxLookup matched := 0 diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go index ac166b3f279..a00586708d4 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go @@ -5,6 +5,7 @@ package doctor import ( "context" + "encoding/json" "errors" "testing" @@ -266,6 +267,35 @@ func TestCheckToolboxes_UsesAssembledStateWithoutLookup(t *testing.T) { require.Contains(t, res.Suggestion, "azd deploy") } +func TestCheckToolboxes_AssembledMissingDetailsKeepJSONShape(t *testing.T) { + t.Parallel() + + state := stateWithToolboxes(nextstep.ResourceRef{ + Name: "split-tools", + ServiceName: "split-tools", + ToolboxSource: nextstep.ToolboxSourceSplit, + }) + state.ToolboxEndpointsChecked = true + state.MissingToolboxEndpoints = []nextstep.ResourceRef{state.Toolboxes[0]} + + res := runToolboxesCheck(t, Dependencies{ + AzdClient: &azdext.AzdClient{}, + assembleState: fixedAssembler(state), + }, nil) + require.Equal(t, StatusFail, res.Status) + + payload, err := json.Marshal(res.Details["missingToolboxes"]) + require.NoError(t, err) + + var details []map[string]any + require.NoError(t, json.Unmarshal(payload, &details)) + require.Equal(t, []map[string]any{{ + "name": "split-tools", + "service": "split-tools", + "envVar": "TOOLBOX_SPLIT_TOOLS_MCP_ENDPOINT", + }}, details) +} + func TestCheckToolboxes_AssembledEndpointErrorFails(t *testing.T) { t.Parallel() From 34c09938df01738c5ea08a48773f6be8ef3f0141 Mon Sep 17 00:00:00 2001 From: huimiu Date: Thu, 13 Aug 2026 22:32:58 +0800 Subject: [PATCH 4/7] fix: order manual values before toolbox deployment --- .../internal/cmd/nextstep/resolver.go | 18 ++++++------ .../internal/cmd/nextstep/resolver_test.go | 28 +++++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go index 87f46a6010e..4cc34c2c0e0 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go @@ -228,14 +228,6 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) // toolbox); ResolveAfterInit is offline by contract and must // not initiate Foundry API calls. if hasToolboxEndpoints { - if hasSplitToolboxEndpoints { - out = append(out, Suggestion{ - Command: "azd deploy", - Description: "deploy split toolbox services", - Priority: priority, - }) - priority++ - } if hasLegacyToolboxEndpoints { out = append(out, Suggestion{ Command: "azd provision", @@ -276,6 +268,16 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) priority++ } } + // A split toolbox deploys the whole project, so wait until + // required agent values are set. + if hasSplitToolboxEndpoints { + out = append(out, Suggestion{ + Command: "azd deploy", + Description: "deploy split toolbox services", + Priority: priority, + }) + priority++ + } // Follow-up: once the user finishes the steps above (provision // for toolboxes, env-set for manual vars), the next productive // command is `azd ai agent run` and the invoke-local secondary. diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go index b3b8cbebd1c..b2cecef6cbe 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go @@ -556,6 +556,34 @@ func TestResolveAfterInit_SplitToolboxUsesDeployOnce(t *testing.T) { assert.Equal(t, 1, deployCount) } +func TestResolveAfterInit_SplitToolboxDeployFollowsManualVars(t *testing.T) { + t.Parallel() + + state := &State{ + HasProjectEndpoint: true, + MissingToolboxEndpoints: []ResourceRef{{ + Name: "split-tools", + ServiceName: "split-tools", + ToolboxSource: ToolboxSourceSplit, + }}, + MissingManualVars: []string{"MY_API_KEY"}, + } + + var buf strings.Builder + require.NoError(t, PrintAllNext(&buf, ResolveAfterInit(state, nil))) + rendered := buf.String() + + manualIndex := strings.Index( + rendered, "azd env set MY_API_KEY ") + deployIndex := strings.Index(rendered, "azd deploy") + runIndex := strings.Index(rendered, "azd ai agent run") + require.NotEqual(t, -1, manualIndex) + require.NotEqual(t, -1, deployIndex) + require.NotEqual(t, -1, runIndex) + assert.Less(t, manualIndex, deployIndex) + assert.Less(t, deployIndex, runIndex) +} + func TestResolveAfterInit_SplitToolboxDoesNotSkipProvision(t *testing.T) { t.Parallel() From 59133eb6cdf80f3f21c622dcdd894d172987e6e4 Mon Sep 17 00:00:00 2001 From: huimiu Date: Thu, 13 Aug 2026 22:49:29 +0800 Subject: [PATCH 5/7] fix: humanize agent diagnostic comments --- .../internal/cmd/doctor/checks_local.go | 26 ++++---- .../internal/cmd/doctor/checks_toolboxes.go | 11 ++-- .../internal/cmd/doctor/state_cache.go | 4 +- .../internal/cmd/nextstep/manifest.go | 13 ++-- .../internal/cmd/nextstep/resolver.go | 4 +- .../internal/cmd/nextstep/state.go | 5 +- .../internal/cmd/nextstep/types.go | 62 ++++++++++--------- 7 files changed, 63 insertions(+), 62 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go index c5ed5b51d68..e1b8eff13ee 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go @@ -43,25 +43,21 @@ type Dependencies struct { AzdClientErr error ExtensionVersion string - // AgentAPIVersion is the Foundry Agents api-version the remote - // probes target. The doctor command's Cobra wiring populates this - // with the package-level DefaultAgentAPIVersion constant so the - // design's "single source of truth" requirement is honored — both - // the runtime invoke flow (init, invoke, listen, monitor, - // session, show) and the doctor probe pin against the same - // constant. Tests can override per-call to assert URL composition - // without coupling to the production value. + // AgentAPIVersion is the Foundry Agents API version used by remote + // probes. Cobra wiring sets it from DefaultAgentAPIVersion, so the + // runtime commands (init, invoke, listen, monitor, session, and + // show) and Doctor use the same value. Tests can override it when + // checking URL construction without using the production value. AgentAPIVersion string - // assembleState is a test seam: when non-nil it replaces the - // production `nextstep.AssembleState` call, letting unit tests inject - // a pre-computed State without standing up a temp project on disk. - // Lowercase so external packages cannot reach it. Production code - // (NewLocalChecks via the Cobra wiring) leaves it nil. + // assembleState is a test seam. When set, it replaces the production + // nextstep.AssembleState call so tests can inject a precomputed + // State without creating a temporary project. It is unexported, and + // production wiring leaves it nil. assembleState func(ctx context.Context, client *azdext.AzdClient) (*nextstep.State, []error) - // StateCache shares one assembled state across Doctor checks and - // trailing guidance during a single invocation. + // StateCache reuses one assembled state for all Doctor checks and + // final guidance during an invocation. StateCache *StateCache // probeAuth is a test seam: when non-nil it replaces the diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go index f235cae0656..bb57d694b31 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go @@ -26,8 +26,8 @@ import ( type toolboxEnvLookupFn func(ctx context.Context, key string) (value string, err error) // newCheckToolboxes produces Check `local.toolboxes` (P5.1 C14). -// For each toolbox collected during next-step state assembly, the -// check verifies that the canonical +// The check examines each toolbox collected during next-step state +// assembly and verifies that its canonical // `TOOLBOX__MCP_ENDPOINT` env var is set to a // non-empty value in the active azd environment. // @@ -43,7 +43,7 @@ type toolboxEnvLookupFn func(ctx context.Context, key string) (value string, err // skips in this state, so the toolbox check would falsely Pass. // - `local.azure-yaml` / `local.agent-service-detected` failed → // no services to walk; walker output is unreliable. -// - state.HasToolboxes == false → no toolbox declarations; +// - state.HasToolboxes == false: there are no toolbox declarations; // the check has nothing to verify. // // # Why this check is not gated on `remote.auth` / @@ -133,8 +133,9 @@ func newCheckToolboxes(deps Dependencies) Check { return classifyToolboxState(state.Toolboxes, state.MissingToolboxEndpoints) } - // Keep the old seam for callers constructing partial states. Production - // assembly always sets ToolboxEndpointsChecked and never reads again. + // Keep this fallback for callers that build partial states. + // Normal assembly sets ToolboxEndpointsChecked, so production + // code does not use this lookup. lookup := deps.lookupToolboxEnv if lookup == nil { lookup = makeRealToolboxEnvLookup(deps.AzdClient) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache.go index cc856da9371..5697539276a 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/state_cache.go @@ -12,7 +12,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/azdext" ) -// StateCache stores one assembled state snapshot for a Doctor run. +// StateCache stores the assembled state for one Doctor run. type StateCache struct { once sync.Once state *nextstep.State @@ -24,7 +24,7 @@ func NewStateCache() *StateCache { return &StateCache{} } -// AssembleAgentState returns the cached state or assembles it once. +// AssembleAgentState returns cached state, assembling it only once. func (deps Dependencies) AssembleAgentState(ctx context.Context) (*nextstep.State, []error) { cache := deps.StateCache if cache == nil { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go index 64a57360847..8d69ae307ab 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go @@ -126,10 +126,11 @@ func populateManifestResources(projectPath string, state *State) { state.HasConnections = len(state.Connections) > 0 } -// populateSplitToolboxes adds standalone azure.ai.toolbox services to state. -// A split service's service name is both its toolbox name and owner name. -// No properties are decoded: the service name is the authoritative toolbox -// identity, so local $ref files cannot affect discovery. +// populateSplitToolboxes adds standalone azure.ai.toolbox services to +// state. A split service uses its service name as both the toolbox +// and owner name. It ignores service properties because the service +// name defines the toolbox identity, so local $ref files do not +// affect discovery. func populateSplitToolboxes( projectCfg *azdext.ProjectConfig, state *State, @@ -163,8 +164,8 @@ func populateSplitToolboxes( return } - // A split service is authoritative for a canonical endpoint key. Keep - // unrelated legacy manifest keys for compatibility. + // A split service takes precedence for its canonical endpoint key. + // Keep unrelated legacy manifest keys for compatibility. merged := make([]ResourceRef, 0, len(state.Toolboxes)+len(split)) for _, ref := range state.Toolboxes { if _, replaced := split[envkey.ToolboxMCPEndpoint(ref.Name)]; !replaced { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go index 4cc34c2c0e0..88fd694b64d 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go @@ -268,8 +268,8 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) priority++ } } - // A split toolbox deploys the whole project, so wait until - // required agent values are set. + // A split toolbox deploys the whole project, so add this step + // after the required agent values are set. if hasSplitToolboxEndpoints { out = append(out, Suggestion{ Command: "azd deploy", diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go index f9f3ed6a22b..e7d5998e94c 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go @@ -339,8 +339,9 @@ func detectMissingAzureContextVars(ctx context.Context, src Source, envName stri return missing } -// probeToolboxEndpoints reads each canonical toolbox endpoint once. Endpoint -// values are produced by azd, so this probe is independent of agent env refs. +// probeToolboxEndpoints reads each canonical toolbox endpoint once. +// azd produces these values, so the probe does not depend on agent +// environment references. func probeToolboxEndpoints( ctx context.Context, src Source, diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go index 035fe7ee48f..38e14e28635 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go @@ -91,27 +91,28 @@ type State struct { // MissingManualVars names ${...} references that map to user-supplied // variables which are not set in the azd environment. // - // Toolbox endpoint variables are partitioned out into - // MissingToolboxEndpoints — they are azd-managed outputs, not - // operator-supplied values. + // Toolbox endpoint variables are tracked separately in + // MissingToolboxEndpoints because azd manages them rather than + // the operator. MissingManualVars []string // MissingToolboxEndpoints lists collected toolboxes whose // TOOLBOX__MCP_ENDPOINT variable is unset in the active - // azd environment. Split services are produced by `azd deploy`; - // legacy manifest toolboxes retain their existing remediation. + // azd environment. `azd deploy` produces split services, while + // legacy manifest toolboxes keep their existing remediation. // - // Each entry carries the resource Name and owning ServiceName so - // resolver and doctor checks can render actionable guidance. The - // shared ResourceRef shape keeps rendering uniform across resources. + // Each entry includes the resource Name and owning ServiceName so + // resolvers and Doctor checks can show the right guidance. The + // shared ResourceRef type keeps resource rendering consistent. MissingToolboxEndpoints []ResourceRef // ToolboxEndpointErrors contains endpoint probe failures from state - // assembly. The doctor check reports these without probing again. + // assembly. Doctor reports these errors without probing again. ToolboxEndpointErrors []string - // ToolboxEndpointsChecked reports that assembly probed every collected - // toolbox endpoint, including endpoints not referenced by agent config. + // ToolboxEndpointsChecked is true after assembly probes every + // collected endpoint, including endpoints not referenced by agent + // config. ToolboxEndpointsChecked bool // UnresolvedPlaceholders names {{NAME}} Mustache-style placeholders @@ -154,11 +155,11 @@ type State struct { // prepend a `cd ` suggestion to the Next: block. CreatedFolderDisplay string - // HasModels, HasToolboxes, HasConnections are aggregate flags. - // Models and connections currently come from agent manifests. - // Toolboxes include configured split services as well as manifest - // resources. Doctor checks gate-skip when no matching resource is - // present; resolvers can tailor remediation suggestions. + // HasModels, HasToolboxes, and HasConnections are aggregate flags. + // They describe resources. Models and connections come from + // agent manifests. Toolboxes include split services and manifest + // resources. Doctor checks skip when no matching resource exists, + // while resolvers can tailor remediation. // // All three flags are false when the manifest file is missing, // malformed, or declares no resources — the walker is deliberately @@ -168,10 +169,10 @@ type State struct { HasToolboxes bool HasConnections bool - // ModelRefs, Toolboxes, Connections list collected resources. - // ModelRefs and Connections remain manifest-derived for now. - // Entries are sorted by Name (ties broken by ServiceName) so - // callers can render them deterministically. + // ModelRefs, Toolboxes, and Connections list collected resources. + // ModelRefs and Connections still come from manifests. Entries are + // sorted by Name, then ServiceName, so callers can render them + // deterministically. ModelRefs []ResourceRef Toolboxes []ResourceRef Connections []ResourceRef @@ -186,13 +187,13 @@ type State struct { // boundary. Add fields here only when a doctor check or resolver // branch needs them. type ResourceRef struct { - // Name is the resource's configured name. Doctor checks match by - // this name when looking up deployments, connections, or toolboxes. + // Name is the configured resource name. Doctor checks use it to find + // deployments, connections, and toolboxes. Name string - // ServiceName is the azd service that declared the resource. When - // the same logical resource is declared by multiple services they - // appear as separate entries. + // ServiceName is the azd service that declared the resource. If + // multiple services declare the same logical resource, each entry + // keeps its own service name. ServiceName string // Detail carries a kind-specific identifier: @@ -203,21 +204,22 @@ type ResourceRef struct { // here must match the doctor-message contract. Detail string - // ToolboxSource identifies how a toolbox was discovered. + // ToolboxSource records how this toolbox was discovered. ToolboxSource ToolboxSource } -// ToolboxSource identifies the configuration source of a toolbox. +// ToolboxSource identifies the source used to discover a toolbox. type ToolboxSource int const ( - // ToolboxSourceUnknown is the zero-value source. + // ToolboxSourceUnknown is the zero value. ToolboxSourceUnknown ToolboxSource = iota - // ToolboxSourceBundled is an agent service toolbox. + // ToolboxSourceBundled is a toolbox declared by an agent service. ToolboxSourceBundled // ToolboxSourceSplit is a standalone azure.ai.toolbox service. ToolboxSourceSplit - // ToolboxSourceLegacyManifest is an agent manifest toolbox. + // ToolboxSourceLegacyManifest is a toolbox declared in an agent + // manifest. ToolboxSourceLegacyManifest ) From 607358aea9f4dc015533fadb777e0f5ed481f8da Mon Sep 17 00:00:00 2001 From: huimiu Date: Tue, 18 Aug 2026 12:41:03 +0800 Subject: [PATCH 6/7] fix: filter toolbox diagnostics by agent dependencies --- .../internal/cmd/doctor/checks_toolboxes.go | 42 +++-- .../cmd/doctor/checks_toolboxes_test.go | 21 +++ .../internal/cmd/nextstep/condition.go | 120 ++++++++++++ .../internal/cmd/nextstep/manifest.go | 177 +++++++++++++++--- .../internal/cmd/nextstep/resolver.go | 51 ++++- .../internal/cmd/nextstep/resolver_test.go | 20 ++ .../internal/cmd/nextstep/state.go | 29 ++- .../internal/cmd/nextstep/state_test.go | 159 ++++++++++++++-- .../internal/cmd/nextstep/types.go | 7 +- 9 files changed, 566 insertions(+), 60 deletions(-) create mode 100644 cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/condition.go diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go index bb57d694b31..33ccb976792 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes.go @@ -109,6 +109,35 @@ func newCheckToolboxes(deps Dependencies) Check { Suggestion: "Re-run `azd ai agent doctor`; the state assembly returned nil unexpectedly.", } } + if len(state.ToolboxDependencyErrors) > 0 { + issues := slices.Clone(state.ToolboxDependencyErrors) + slices.Sort(issues) + return Result{ + Status: StatusFail, + Message: fmt.Sprintf( + "configured toolbox dependencies are invalid: %s", + strings.Join(issues, "; ")), + Suggestion: "Update azure.yaml so each agent uses an enabled " + + "toolbox service, then re-run `azd ai agent doctor`.", + Details: map[string]any{ + "toolboxDependencyErrors": issues, + }, + } + } + if state.ToolboxEndpointsChecked && + len(state.ToolboxEndpointErrors) > 0 { + return Result{ + Status: StatusFail, + Message: fmt.Sprintf( + "could not read toolbox endpoint values: %s", + strings.Join(state.ToolboxEndpointErrors, "; ")), + Suggestion: "Verify the active azd environment is accessible, then re-run " + + "`azd ai agent doctor`.", + Details: map[string]any{ + "toolboxEndpointErrors": state.ToolboxEndpointErrors, + }, + } + } if !state.HasToolboxes { return Result{ Status: StatusSkip, @@ -117,19 +146,6 @@ func newCheckToolboxes(deps Dependencies) Check { } if state.ToolboxEndpointsChecked { - if len(state.ToolboxEndpointErrors) > 0 { - return Result{ - Status: StatusFail, - Message: fmt.Sprintf( - "could not read toolbox endpoint values: %s", - strings.Join(state.ToolboxEndpointErrors, "; ")), - Suggestion: "Verify the active azd environment is accessible, then re-run " + - "`azd ai agent doctor`.", - Details: map[string]any{ - "toolboxEndpointErrors": state.ToolboxEndpointErrors, - }, - } - } return classifyToolboxState(state.Toolboxes, state.MissingToolboxEndpoints) } diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go index a00586708d4..0cf17bcf301 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_toolboxes_test.go @@ -121,6 +121,27 @@ func TestCheckToolboxes_SkipsWhenNoToolboxesDeclared(t *testing.T) { require.Contains(t, res.Message, "no configured toolbox resources") } +func TestCheckToolboxes_FailsOnToolboxDependencyError(t *testing.T) { + t.Parallel() + + state := &nextstep.State{ + ToolboxDependencyErrors: []string{ + `toolbox service "disabled" used by agent service(s) "agent" is disabled`, + }, + } + res := runToolboxesCheck(t, Dependencies{ + AzdClient: &azdext.AzdClient{}, + assembleState: fixedAssembler(state), + }, nil) + + require.Equal(t, StatusFail, res.Status) + require.Contains(t, res.Message, "disabled") + require.Contains(t, res.Suggestion, "azure.yaml") + require.NotContains(t, res.Suggestion, "azd deploy") + require.Equal(t, state.ToolboxDependencyErrors, + res.Details["toolboxDependencyErrors"]) +} + func TestCheckToolboxes_FailsWhenAssemblerReturnsNilState(t *testing.T) { t.Parallel() deps := Dependencies{ diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/condition.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/condition.go new file mode 100644 index 00000000000..dac09cf52c9 --- /dev/null +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/condition.go @@ -0,0 +1,120 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package nextstep + +import ( + "context" + "fmt" + "os" + "strconv" + "strings" + + "github.com/azure/azure-dev/cli/azd/pkg/foundry" + "google.golang.org/protobuf/types/known/structpb" +) + +func isServiceEnabled( + ctx context.Context, + src Source, + envName string, + serviceName string, +) (bool, error) { + value, found, err := src.ServiceConfigValue( + ctx, + serviceName, + "condition", + ) + if err != nil { + return false, err + } + if !found || value == nil { + return true, nil + } + + condition, err := conditionValueString(value) + if err != nil { + return false, err + } + if strings.TrimSpace(condition) == "" { + return true, nil + } + + expanded, err := expandServiceCondition( + ctx, + src, + envName, + condition, + ) + if err != nil { + return false, err + } + return isTruthyCondition(expanded), nil +} + +func conditionValueString(value *structpb.Value) (string, error) { + if value == nil { + return "", nil + } + + switch kind := value.Kind.(type) { + case *structpb.Value_StringValue: + return kind.StringValue, nil + case *structpb.Value_BoolValue: + return strconv.FormatBool(kind.BoolValue), nil + case *structpb.Value_NumberValue: + return strconv.FormatFloat(kind.NumberValue, 'g', -1, 64), nil + case *structpb.Value_NullValue: + return "", nil + default: + return "", fmt.Errorf( + "condition must be a string, boolean, or number", + ) + } +} + +func expandServiceCondition( + ctx context.Context, + src Source, + envName string, + condition string, +) (string, error) { + if envName == "" { + return foundry.ExpandEnv(condition, os.Getenv) + } + + values := map[string]string{} + var lookupErr error + expanded, err := foundry.ExpandEnv(condition, func(name string) string { + if value, ok := values[name]; ok { + return value + } + value, err := src.EnvValue(ctx, envName, name) + if err != nil { + lookupErr = fmt.Errorf( + "read condition environment variable %q: %w", + name, + err, + ) + return "" + } + values[name] = value + return value + }) + if err != nil { + return "", err + } + if lookupErr != nil { + return "", lookupErr + } + return expanded, nil +} + +func isTruthyCondition(value string) bool { + switch value { + case "1", "true", "TRUE", "True", "yes", "YES", "Yes": + return true + default: + return false + } +} diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go index 8d69ae307ab..0657b37ae58 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go @@ -5,8 +5,12 @@ package nextstep import ( "cmp" + "context" + "errors" + "fmt" "os" "slices" + "strings" "azureaiagent/internal/pkg/agents/agent_yaml" "azureaiagent/internal/pkg/envkey" @@ -15,6 +19,8 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/azdext" ) +const toolboxHost = "azure.ai.toolbox" + // manifestFileNames are the candidate manifest filenames the walker // probes, in the same precedence order init / deploy paths use: // agent.manifest.yaml wins over agent.manifest.yml. The non-manifest @@ -126,51 +132,88 @@ func populateManifestResources(projectPath string, state *State) { state.HasConnections = len(state.Connections) > 0 } -// populateSplitToolboxes adds standalone azure.ai.toolbox services to -// state. A split service uses its service name as both the toolbox -// and owner name. It ignores service properties because the service -// name defines the toolbox identity, so local $ref files do not -// affect discovery. +// populateSplitToolboxes adds active toolbox dependencies to state. func populateSplitToolboxes( + ctx context.Context, + src Source, + envName string, projectCfg *azdext.ProjectConfig, state *State, + errs *[]error, ) { if projectCfg == nil || state == nil { return } + candidates := splitToolboxDependencies(projectCfg) + if len(candidates) == 0 { + return + } + split := make(map[string]ResourceRef) - for serviceName, svc := range projectCfg.Services { - if svc == nil || svc.GetHost() != "azure.ai.toolbox" { + reserved := make(map[string]struct{}, len(candidates)) + keys := make([]string, 0, len(candidates)) + for key := range candidates { + keys = append(keys, key) + } + slices.Sort(keys) + + for _, key := range keys { + candidate := candidates[key] + reserved[key] = struct{}{} + enabled, err := isServiceEnabled( + ctx, + src, + envName, + candidate.configName, + ) + if err != nil { + issue := fmt.Sprintf( + "toolbox service %q used by agent service(s) %s has an invalid deployment condition: %v", + candidate.ref.ServiceName, + strings.Join(candidate.agents, ", "), + err, + ) + state.ToolboxDependencyErrors = append( + state.ToolboxDependencyErrors, + issue, + ) + *errs = append( + *errs, + fmt.Errorf( + "toolbox service %q deployment condition: %w", + candidate.ref.ServiceName, + err, + ), + ) continue } - if svc.GetName() != "" { - serviceName = svc.GetName() - } - if serviceName == "" { + if !enabled { + issue := fmt.Sprintf( + "toolbox service %q used by agent service(s) %s is disabled by its deployment condition", + candidate.ref.ServiceName, + strings.Join(candidate.agents, ", "), + ) + state.ToolboxDependencyErrors = append( + state.ToolboxDependencyErrors, + issue, + ) + *errs = append( + *errs, + errors.New(issue), + ) continue } - ref := ResourceRef{ - Name: serviceName, - ServiceName: serviceName, - ToolboxSource: ToolboxSourceSplit, - } - key := envkey.ToolboxMCPEndpoint(ref.Name) - if prior, ok := split[key]; !ok || ref.ServiceName < prior.ServiceName { - split[key] = ref - } - } - if len(split) == 0 { - return + + split[key] = candidate.ref } - // A split service takes precedence for its canonical endpoint key. - // Keep unrelated legacy manifest keys for compatibility. merged := make([]ResourceRef, 0, len(state.Toolboxes)+len(split)) for _, ref := range state.Toolboxes { - if _, replaced := split[envkey.ToolboxMCPEndpoint(ref.Name)]; !replaced { - merged = append(merged, ref) + if _, replaced := reserved[envkey.ToolboxMCPEndpoint(ref.Name)]; replaced { + continue } + merged = append(merged, ref) } for _, ref := range split { merged = append(merged, ref) @@ -183,6 +226,86 @@ func populateSplitToolboxes( }) state.Toolboxes = merged state.HasToolboxes = len(merged) > 0 + slices.Sort(state.ToolboxDependencyErrors) +} + +type splitToolboxCandidate struct { + ref ResourceRef + configName string + agents []string +} + +type splitToolboxService struct { + ref ResourceRef + configName string +} + +func splitToolboxDependencies( + projectCfg *azdext.ProjectConfig, +) map[string]splitToolboxCandidate { + services := make(map[string]splitToolboxService) + for serviceName, svc := range projectCfg.Services { + if svc == nil || svc.GetHost() != toolboxHost { + continue + } + name := svc.GetName() + if name == "" { + name = serviceName + } + if name == "" { + continue + } + ref := ResourceRef{ + Name: name, + ServiceName: name, + ToolboxSource: ToolboxSourceSplit, + } + service := splitToolboxService{ + ref: ref, + configName: serviceName, + } + services[serviceName] = service + services[name] = service + } + + candidates := make(map[string]splitToolboxCandidate) + for serviceName, svc := range projectCfg.Services { + if svc == nil || svc.GetHost() != agentHost { + continue + } + agentName := svc.GetName() + if agentName == "" { + agentName = serviceName + } + for _, dependencyName := range svc.GetUses() { + service, ok := services[dependencyName] + if !ok { + continue + } + key := envkey.ToolboxMCPEndpoint(service.ref.Name) + candidate := candidates[key] + if candidate.ref.Name == "" || + service.ref.ServiceName < candidate.ref.ServiceName { + candidate.ref = service.ref + candidate.configName = service.configName + } + candidate.agents = appendUnique(candidate.agents, agentName) + candidates[key] = candidate + } + } + + for key, candidate := range candidates { + slices.Sort(candidate.agents) + candidates[key] = candidate + } + return candidates +} + +func appendUnique(values []string, value string) []string { + if slices.Contains(values, value) { + return values + } + return append(values, value) } // readManifestBytes returns the first manifest file's contents under diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go index 88fd694b64d..da4dab64968 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go @@ -180,6 +180,7 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) hasToolboxEndpoints := len(state.MissingToolboxEndpoints) > 0 hasManualVars := len(state.MissingManualVars) > 0 hasToolboxEndpointErrors := len(state.ToolboxEndpointErrors) > 0 + hasToolboxDependencyErrors := len(state.ToolboxDependencyErrors) > 0 hasSplitToolboxEndpoints := hasMissingToolboxSource( state.MissingToolboxEndpoints, ToolboxSourceSplit) hasLegacyToolboxEndpoints := hasMissingLegacyToolbox( @@ -204,7 +205,14 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) Description: "set up your Foundry project, models, and connections", Priority: priority, }) - case hasToolboxEndpoints || hasToolboxEndpointErrors || hasManualVars: + priority++ + priority = appendToolboxDependencyGuidance( + &out, + state.ToolboxDependencyErrors, + priority, + ) + case hasToolboxEndpoints || hasToolboxEndpointErrors || + hasToolboxDependencyErrors || hasManualVars: // Combined branch for the two "things the user has to fix before // running locally" categories. They are intentionally additive // (not mutually exclusive) so a manifest that declares a @@ -214,7 +222,12 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) // still emit `azd ai agent run`, leaving the user to discover // the unset manual var when the agent crashes. // - // Toolbox sub-branch: manifest declares one or more toolboxes + priority = appendToolboxDependencyGuidance( + &out, + state.ToolboxDependencyErrors, + priority, + ) + // Toolbox sub-branch: configured toolboxes declare one or more // whose azd-injected TOOLBOX__MCP_ENDPOINT variable is // not yet present in the azd environment. The variable is // written by `azd provision` (listen.go::registerToolboxEnvVars) @@ -285,7 +298,9 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) // literal `{{NAME}}` values in the agent configuration still // break the local agent — the user must finish the placeholder // fix-ups first; the trailing `azd deploy` reminder still applies. - if !hasPlaceholders && !hasToolboxEndpointErrors { + if !hasPlaceholders && + !hasToolboxEndpointErrors && + !hasToolboxDependencyErrors { out = append(out, Suggestion{ Command: "azd ai agent run", Description: runFollowUpDescription( @@ -319,9 +334,10 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) out, _ = appendInvokeLocalSecondary(out, state, readmeExists, priority) } - if !slices.ContainsFunc(out, func(s Suggestion) bool { - return strings.TrimSpace(s.Command) == "azd deploy" - }) { + if !hasToolboxDependencyErrors && + !slices.ContainsFunc(out, func(s Suggestion) bool { + return strings.TrimSpace(s.Command) == "azd deploy" + }) { out = append(out, Suggestion{ Command: "azd deploy", Description: "when ready to deploy to Azure", @@ -339,6 +355,29 @@ func ResolveAfterInit(state *State, readmeExists func(relativePath string) bool) return out } +func appendToolboxDependencyGuidance( + out *[]Suggestion, + errors []string, + priority int, +) int { + if len(errors) == 0 { + return priority + } + + issues := slices.Clone(errors) + slices.Sort(issues) + limit := min(len(issues), maxFixupLines) + for _, issue := range issues[:limit] { + *out = append(*out, Suggestion{ + Command: "edit azure.yaml: enable the toolbox or remove it from agent uses", + Description: issue, + Priority: priority, + }) + priority++ + } + return priority +} + func hasMissingToolboxSource(toolboxes []ResourceRef, source ToolboxSource) bool { for _, toolbox := range toolboxes { if toolbox.ToolboxSource == source { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go index b2cecef6cbe..6032781f22a 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver_test.go @@ -617,6 +617,26 @@ func TestResolveAfterInit_ToolboxEndpointErrorBlocksLocalRun(t *testing.T) { assert.NotContains(t, rendered, "azd ai agent invoke --local") } +func TestResolveAfterInit_ToolboxDependencyErrorBlocksRemediation(t *testing.T) { + t.Parallel() + + state := &State{ + HasProjectEndpoint: true, + ToolboxDependencyErrors: []string{ + `toolbox service "disabled" is disabled by its deployment condition`, + }, + } + + var buf strings.Builder + require.NoError(t, PrintAllNext(&buf, ResolveAfterInit(state, nil))) + rendered := buf.String() + assert.Contains(t, rendered, "edit azure.yaml") + assert.Contains(t, rendered, "disabled") + assert.NotContains(t, rendered, "azd deploy") + assert.NotContains(t, rendered, "azd ai agent run") + assert.NotContains(t, rendered, "azd ai agent invoke --local") +} + // TestResolveAfterInit_ToolboxAndManualVarsCoexist locks the bug both // reviewers caught: when MissingToolboxEndpoints AND MissingManualVars // are populated, the previously-exclusive switch hid the manual diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go index e7d5998e94c..d7ba5714932 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go @@ -94,6 +94,12 @@ type Source interface { // string with a nil error means the key is unset; transport errors are // surfaced verbatim. EnvValue(ctx context.Context, envName, key string) (string, error) + // ServiceConfigValue returns a raw service configuration value. + ServiceConfigValue( + ctx context.Context, + serviceName string, + path string, + ) (*structpb.Value, bool, error) } // NewSource adapts an *azdext.AzdClient to the Source interface. The @@ -143,6 +149,27 @@ func (s *clientSource) EnvValue(ctx context.Context, envName, key string) (strin return resp.Value, nil } +func (s *clientSource) ServiceConfigValue( + ctx context.Context, + serviceName string, + path string, +) (*structpb.Value, bool, error) { + resp, err := s.client.Project().GetServiceConfigValue( + ctx, + &azdext.GetServiceConfigValueRequest{ + ServiceName: serviceName, + Path: path, + }, + ) + if err != nil { + return nil, false, err + } + if resp == nil { + return nil, false, nil + } + return resp.Value, resp.Found, nil +} + // Option configures AssembleState. type Option func(*config) @@ -303,7 +330,7 @@ func assembleState(ctx context.Context, src Source, opts ...Option) (*State, []e if len(state.Services) > 0 { populateManifestResources(project.Path, state) } - populateSplitToolboxes(project, state) + populateSplitToolboxes(ctx, src, envName, project, state, &errs) } if project != nil && envName != "" { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go index a1c955eb44b..f8c97367065 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go @@ -21,14 +21,16 @@ import ( // fakeSource is a hand-rolled Source for table-driven tests. type fakeSource struct { - envName string - envNameErr error - project *azdext.ProjectConfig - projectErr error - values map[string]string - valueErr error - valueErrors map[string]error - calls map[string]int + envName string + envNameErr error + project *azdext.ProjectConfig + projectErr error + values map[string]string + valueErr error + valueErrors map[string]error + configValues map[string]*structpb.Value + configErrors map[string]error + calls map[string]int } func (f *fakeSource) CurrentEnvName(_ context.Context) (string, error) { @@ -53,6 +55,19 @@ func (f *fakeSource) EnvValue(_ context.Context, envName, key string) (string, e return f.values[envName+"/"+key], nil } +func (f *fakeSource) ServiceConfigValue( + _ context.Context, + serviceName string, + path string, +) (*structpb.Value, bool, error) { + key := serviceName + "/" + path + if err := f.configErrors[key]; err != nil { + return nil, false, err + } + value, found := f.configValues[key] + return value, found, nil +} + func TestAssembleState_SplitToolboxesProbeCanonicalEndpoints(t *testing.T) { t.Parallel() @@ -71,6 +86,10 @@ func TestAssembleState_SplitToolboxesProbeCanonicalEndpoints(t *testing.T) { "alpha-copy": { Name: "alpha-copy", Host: "azure.ai.toolbox", }, + "agent": { + Name: "agent", Host: agentHost, + Uses: []string{"alpha", "alpha-copy"}, + }, }, }, } @@ -94,6 +113,9 @@ func TestAssembleState_SplitToolboxMissingEndpointIsNotManual(t *testing.T) { values: map[string]string{ "dev/TOOLBOX_OTHER_MCP_ENDPOINT": "https://other.example/mcp", }, + configErrors: map[string]error{ + "missing/condition": errors.New("unreferenced condition must not be read"), + }, project: &azdext.ProjectConfig{ Services: map[string]*azdext.ServiceConfig{ "missing": { @@ -102,6 +124,10 @@ func TestAssembleState_SplitToolboxMissingEndpointIsNotManual(t *testing.T) { "other": { Name: "other", Host: "azure.ai.toolbox", }, + "agent": { + Name: "agent", Host: agentHost, + Uses: []string{"other"}, + }, }, }, } @@ -109,8 +135,10 @@ func TestAssembleState_SplitToolboxMissingEndpointIsNotManual(t *testing.T) { state, errs := assembleState(t.Context(), src) require.Empty(t, errs) require.Empty(t, state.MissingManualVars) - require.Len(t, state.MissingToolboxEndpoints, 1) - require.Equal(t, "missing", state.MissingToolboxEndpoints[0].Name) + require.Empty(t, state.MissingToolboxEndpoints) + require.Len(t, state.Toolboxes, 1) + require.Equal(t, "other", state.Toolboxes[0].Name) + require.Equal(t, 0, src.calls["dev/TOOLBOX_MISSING_MCP_ENDPOINT"]) } func TestAssembleState_SplitToolboxEndpointErrorIsSurfaced(t *testing.T) { @@ -127,6 +155,10 @@ func TestAssembleState_SplitToolboxEndpointErrorIsSurfaced(t *testing.T) { "missing": { Name: "missing", Host: "azure.ai.toolbox", }, + "agent": { + Name: "agent", Host: agentHost, + Uses: []string{"missing"}, + }, }, }, } @@ -154,10 +186,23 @@ func TestPopulateSplitToolboxes_PrefersSplitCanonicalKey(t *testing.T) { project := &azdext.ProjectConfig{ Services: map[string]*azdext.ServiceConfig{ "my-tool": {Name: "my-tool", Host: "azure.ai.toolbox"}, + "agent": { + Name: "agent", Host: agentHost, + Uses: []string{"my-tool"}, + }, }, } - populateSplitToolboxes(project, state) + var errs []error + populateSplitToolboxes( + t.Context(), + &fakeSource{}, + "", + project, + state, + &errs, + ) + require.Empty(t, errs) require.Len(t, state.Toolboxes, 2) require.Equal(t, "legacy", state.Toolboxes[0].Name) require.Equal(t, ToolboxSourceLegacyManifest, state.Toolboxes[0].ToolboxSource) @@ -165,6 +210,98 @@ func TestPopulateSplitToolboxes_PrefersSplitCanonicalKey(t *testing.T) { require.Equal(t, ToolboxSourceSplit, state.Toolboxes[1].ToolboxSource) } +func TestAssembleState_SplitToolboxConditionDisabled(t *testing.T) { + t.Parallel() + + src := &fakeSource{ + envName: "dev", + configValues: map[string]*structpb.Value{ + "disabled/condition": structpb.NewBoolValue(false), + }, + calls: make(map[string]int), + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "disabled": { + Name: "disabled", Host: toolboxHost, + }, + "agent": { + Name: "agent", Host: agentHost, + Uses: []string{"disabled"}, + }, + }, + }, + } + + state, errs := assembleState(t.Context(), src) + require.Len(t, errs, 1) + require.False(t, state.HasToolboxes) + require.Empty(t, state.MissingToolboxEndpoints) + require.Len(t, state.ToolboxDependencyErrors, 1) + require.Contains(t, state.ToolboxDependencyErrors[0], "disabled") + require.Equal(t, 0, src.calls["dev/TOOLBOX_DISABLED_MCP_ENDPOINT"]) +} + +func TestAssembleState_SplitToolboxConditionUsesEnvironment(t *testing.T) { + t.Parallel() + + src := &fakeSource{ + envName: "dev", + values: map[string]string{ + "dev/ENABLE_TOOLBOX": "false", + }, + configValues: map[string]*structpb.Value{ + "conditional/condition": structpb.NewStringValue("${ENABLE_TOOLBOX}"), + }, + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "conditional": { + Name: "conditional", Host: toolboxHost, + }, + "agent": { + Name: "agent", Host: agentHost, + Uses: []string{"conditional"}, + }, + }, + }, + } + + state, errs := assembleState(t.Context(), src) + require.Len(t, errs, 1) + require.Empty(t, state.Toolboxes) + require.Len(t, state.ToolboxDependencyErrors, 1) + require.Contains(t, state.ToolboxDependencyErrors[0], "conditional") +} + +func TestAssembleState_SplitToolboxConditionErrorIsSurfaced(t *testing.T) { + t.Parallel() + + src := &fakeSource{ + envName: "dev", + configValues: map[string]*structpb.Value{ + "malformed/condition": structpb.NewStringValue("${"), + }, + calls: make(map[string]int), + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "malformed": { + Name: "malformed", Host: toolboxHost, + }, + "agent": { + Name: "agent", Host: agentHost, + Uses: []string{"malformed"}, + }, + }, + }, + } + + state, errs := assembleState(t.Context(), src) + require.Len(t, errs, 1) + require.Empty(t, state.Toolboxes) + require.Len(t, state.ToolboxDependencyErrors, 1) + require.Contains(t, state.ToolboxDependencyErrors[0], "invalid deployment condition") + require.Equal(t, 0, src.calls["dev/TOOLBOX_MALFORMED_MCP_ENDPOINT"]) +} + func TestAssembleState(t *testing.T) { t.Parallel() diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go index 38e14e28635..e70cc6637fc 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/types.go @@ -110,9 +110,12 @@ type State struct { // assembly. Doctor reports these errors without probing again. ToolboxEndpointErrors []string + // ToolboxDependencyErrors contains invalid active-agent toolbox + // dependency conditions. + ToolboxDependencyErrors []string + // ToolboxEndpointsChecked is true after assembly probes every - // collected endpoint, including endpoints not referenced by agent - // config. + // active toolbox endpoint. ToolboxEndpointsChecked bool // UnresolvedPlaceholders names {{NAME}} Mustache-style placeholders From b830ceaedf0cb40816abcdde49cd55a688566606 Mon Sep 17 00:00:00 2001 From: huimiu Date: Tue, 18 Aug 2026 15:18:27 +0800 Subject: [PATCH 7/7] fix: filter disabled agent env diagnostics --- .../internal/cmd/nextstep/manifest.go | 85 +++++++++++++- .../internal/cmd/nextstep/state.go | 33 +++++- .../internal/cmd/nextstep/state_test.go | 106 ++++++++++++++++++ 3 files changed, 214 insertions(+), 10 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go index 0657b37ae58..31f0c907f0c 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/manifest.go @@ -140,14 +140,29 @@ func populateSplitToolboxes( projectCfg *azdext.ProjectConfig, state *State, errs *[]error, -) { +) splitToolboxResult { if projectCfg == nil || state == nil { - return + return splitToolboxResult{} } - candidates := splitToolboxDependencies(projectCfg) + candidates, excludedAgents := splitToolboxDependencies( + ctx, + src, + envName, + projectCfg, + state, + errs, + ) + endpointKeys := make(map[string]struct{}, len(candidates)) + for key := range candidates { + endpointKeys[key] = struct{}{} + } if len(candidates) == 0 { - return + slices.Sort(state.ToolboxDependencyErrors) + return splitToolboxResult{ + excludedAgents: excludedAgents, + endpointKeys: endpointKeys, + } } split := make(map[string]ResourceRef) @@ -227,6 +242,15 @@ func populateSplitToolboxes( state.Toolboxes = merged state.HasToolboxes = len(merged) > 0 slices.Sort(state.ToolboxDependencyErrors) + return splitToolboxResult{ + excludedAgents: excludedAgents, + endpointKeys: endpointKeys, + } +} + +type splitToolboxResult struct { + excludedAgents map[string]struct{} + endpointKeys map[string]struct{} } type splitToolboxCandidate struct { @@ -241,8 +265,13 @@ type splitToolboxService struct { } func splitToolboxDependencies( + ctx context.Context, + src Source, + envName string, projectCfg *azdext.ProjectConfig, -) map[string]splitToolboxCandidate { + state *State, + errs *[]error, +) (map[string]splitToolboxCandidate, map[string]struct{}) { services := make(map[string]splitToolboxService) for serviceName, svc := range projectCfg.Services { if svc == nil || svc.GetHost() != toolboxHost { @@ -269,6 +298,7 @@ func splitToolboxDependencies( } candidates := make(map[string]splitToolboxCandidate) + excludedAgents := make(map[string]struct{}) for serviceName, svc := range projectCfg.Services { if svc == nil || svc.GetHost() != agentHost { continue @@ -277,11 +307,54 @@ func splitToolboxDependencies( if agentName == "" { agentName = serviceName } + dependencies := make([]splitToolboxService, 0) for _, dependencyName := range svc.GetUses() { service, ok := services[dependencyName] if !ok { continue } + dependencies = append(dependencies, service) + } + if len(dependencies) == 0 { + continue + } + + enabled, err := isServiceEnabled(ctx, src, envName, serviceName) + if err != nil { + names := make([]string, 0, len(dependencies)) + for _, service := range dependencies { + names = appendUnique(names, service.ref.ServiceName) + } + slices.Sort(names) + issue := fmt.Sprintf( + "agent service %q uses toolbox service(s) %s but has an invalid deployment condition: %v", + agentName, + strings.Join(names, ", "), + err, + ) + state.ToolboxDependencyErrors = append( + state.ToolboxDependencyErrors, + issue, + ) + *errs = append( + *errs, + fmt.Errorf( + "agent service %q deployment condition: %w", + agentName, + err, + ), + ) + excludedAgents[agentName] = struct{}{} + excludedAgents[serviceName] = struct{}{} + continue + } + if !enabled { + excludedAgents[agentName] = struct{}{} + excludedAgents[serviceName] = struct{}{} + continue + } + + for _, service := range dependencies { key := envkey.ToolboxMCPEndpoint(service.ref.Name) candidate := candidates[key] if candidate.ref.Name == "" || @@ -298,7 +371,7 @@ func splitToolboxDependencies( slices.Sort(candidate.agents) candidates[key] = candidate } - return candidates + return candidates, excludedAgents } func appendUnique(values []string, value string) []string { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go index d7ba5714932..5ffe129eeb1 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go @@ -326,17 +326,34 @@ func assembleState(ctx context.Context, src Source, opts ...Option) (*State, []e &state.EnvironmentLoadErrors, ) + var splitToolboxState splitToolboxResult if project != nil { if len(state.Services) > 0 { populateManifestResources(project.Path, state) } - populateSplitToolboxes(ctx, src, envName, project, state, &errs) + splitToolboxState = populateSplitToolboxes( + ctx, + src, + envName, + project, + state, + &errs, + ) } if project != nil && envName != "" { - state.MissingInfraVars, state.MissingManualVars, state.UnresolvedPlaceholders = detectMissingVars( - ctx, src, envName, project.Path, state.Services, state.Toolboxes, &errs, - ) + state.MissingInfraVars, state.MissingManualVars, state.UnresolvedPlaceholders = + detectMissingVars( + ctx, + src, + envName, + project.Path, + state.Services, + state.Toolboxes, + splitToolboxState.endpointKeys, + splitToolboxState.excludedAgents, + &errs, + ) populateOpenAPIPayload(ctx, cfg, project.Path, envName, state) } @@ -669,6 +686,8 @@ func detectMissingVars( envName, projectPath string, services []ServiceState, toolboxes []ResourceRef, + splitToolboxEndpointKeys map[string]struct{}, + excludedAgents map[string]struct{}, errs *[]error, ) (infra, manual, placeholders []string) { if envName == "" || projectPath == "" || len(services) == 0 { @@ -683,8 +702,14 @@ func detectMissingVars( for _, toolbox := range toolboxes { toolboxKeys[envkey.ToolboxMCPEndpoint(toolbox.Name)] = struct{}{} } + for key := range splitToolboxEndpointKeys { + toolboxKeys[key] = struct{}{} + } for _, svc := range services { + if _, excluded := excludedAgents[svc.Name]; excluded { + continue + } refs, phs := extractEnvironmentRefs(svc.EnvironmentValues) for _, name := range refs { if _, isToolbox := toolboxKeys[name]; isToolbox { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go index f8c97367065..03723106272 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go @@ -141,6 +141,48 @@ func TestAssembleState_SplitToolboxMissingEndpointIsNotManual(t *testing.T) { require.Equal(t, 0, src.calls["dev/TOOLBOX_MISSING_MCP_ENDPOINT"]) } +func TestAssembleState_DisabledAgentDoesNotCollectSplitToolbox(t *testing.T) { + t.Parallel() + + disabledAgent := newAgentService(t, map[string]any{ + "kind": "hostedAgent", + "environmentVariables": []any{ + map[string]any{ + "name": "TOOLS_ENDPOINT", + "value": "${TOOLBOX_TOOLS_MCP_ENDPOINT}", + }, + }, + }) + disabledAgent.Name = "disabled-agent" + disabledAgent.Uses = []string{"tools"} + + src := &fakeSource{ + envName: "dev", + configValues: map[string]*structpb.Value{ + "disabled-agent/condition": structpb.NewBoolValue(false), + }, + configErrors: map[string]error{ + "tools/condition": errors.New("disabled toolbox condition must not be read"), + }, + calls: make(map[string]int), + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "tools": { + Name: "tools", Host: toolboxHost, + }, + "disabled-agent": disabledAgent, + }, + }, + } + + state, errs := assembleState(t.Context(), src) + require.Empty(t, errs) + require.Empty(t, state.Toolboxes) + require.Empty(t, state.MissingToolboxEndpoints) + require.Empty(t, state.MissingManualVars) + require.Equal(t, 0, src.calls["dev/TOOLBOX_TOOLS_MCP_ENDPOINT"]) +} + func TestAssembleState_SplitToolboxEndpointErrorIsSurfaced(t *testing.T) { t.Parallel() @@ -241,6 +283,70 @@ func TestAssembleState_SplitToolboxConditionDisabled(t *testing.T) { require.Equal(t, 0, src.calls["dev/TOOLBOX_DISABLED_MCP_ENDPOINT"]) } +func TestAssembleState_InactiveSplitToolboxEndpointIsNotManual(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + condition *structpb.Value + conditionErr error + }{ + "disabled": { + condition: structpb.NewBoolValue(false), + }, + "condition read error": { + conditionErr: errors.New("condition unavailable"), + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + agent := newAgentService(t, map[string]any{ + "kind": "hostedAgent", + "environmentVariables": []any{ + map[string]any{ + "name": "TOOLS_ENDPOINT", + "value": "${TOOLBOX_DISABLED_MCP_ENDPOINT}", + }, + }, + }) + agent.Name = "agent" + agent.Uses = []string{"disabled"} + + src := &fakeSource{ + envName: "dev", + configValues: map[string]*structpb.Value{ + "disabled/condition": tt.condition, + }, + configErrors: map[string]error{ + "disabled/condition": tt.conditionErr, + }, + calls: make(map[string]int), + project: &azdext.ProjectConfig{ + Services: map[string]*azdext.ServiceConfig{ + "disabled": { + Name: "disabled", Host: toolboxHost, + }, + "agent": agent, + }, + }, + } + + state, errs := assembleState(t.Context(), src) + require.Len(t, errs, 1) + require.Empty(t, state.MissingManualVars) + require.Empty(t, state.MissingToolboxEndpoints) + require.Len(t, state.ToolboxDependencyErrors, 1) + require.Equal( + t, + 0, + src.calls["dev/TOOLBOX_DISABLED_MCP_ENDPOINT"], + ) + }) + } +} + func TestAssembleState_SplitToolboxConditionUsesEnvironment(t *testing.T) { t.Parallel()