fix(azure.ai.agents): doctor checks 5-6 — 3-of-3 review fix-ups (skip-cascade + production-equivalent YAML validation)

Two MEDIUM findings, 3-of-3 reviewer consensus across Opus xhigh, Sonnet 4.6, and GPT-5.5.

Finding A (skip-cascade propagation):
priorFailed only matched StatusFail, not StatusSkip. When local.azure-yaml
failed, its dependents (env-selected, agent-service-detected) skipped — but
THEIR dependents (project-endpoint-set, agent-yaml-valid) saw the upstream
as Skip-not-Fail and ran anyway. Concrete user impact in the "wrong
directory" scenario: 1 real failure (check 2: azure.yaml) + 1 misleading
failure (check 5: "Run azd provision") + 1 duplicate failure (check 6:
same error as check 2). Worse: with a stale .env, check 5 could PASS with
a stale endpoint and hide the broken project.

Fix: rename priorFailed -> priorBlocked and treat both Fail and Skip as
blocking. Updated skip messages on checks 5 and 6 to say "failed or
skipped" / "or upstream check blocked" so users know the root cause is
earlier in the chain. Added regression tests for the Skip-state predecessor
path on both checks; updated TestPriorBlocked to cover the new contract
(including the "matching skip" case that previously asserted false).

Finding B (YAML validation gap):
GPT identified that check 6 used bare yaml.Unmarshal into ContainerAgent,
which is decode-permissive — silently accepts manifests with missing kind,
invalid kind (e.g. "nonsense"), missing name, or DNS-invalid name (e.g.
"My_Agent"). Production deploy uses agent_yaml.ValidateAgentDefinition
which rejects all of these.

Opus additionally identified that the doctor's "gopkg.in/yaml.v3" import
was the wrong library entirely — agent_yaml's custom UnmarshalYAML methods
(PropertySchema.UnmarshalYAML at yaml.go:374) bind to *go.yaml.in/yaml/v3.
Node, not gopkg.in/yaml.v3.Node. Go method dispatch is by exact parameter
type, so the doctor was silently skipping every custom unmarshaler in
agent_yaml. Production loads via go.yaml.in (helpers.go:28,772).

Sonnet confirmed both at HIGH (doctor's whole purpose is to surface deploy
blockers pre-flight) and recommended the bundled fix.

3/3 bundling consensus: replace the bare unmarshal with a single call to
agent_yaml.ValidateAgentDefinition(data). Opus verified the library
transitively resolves — parse.go imports go.yaml.in/yaml/v3, so once the
gopkg.in/yaml.v3 call is removed the file's wrong-library import goes with
it (Go compile error otherwise). One function body changed.

This also fixes Opus's wording caveat: REPLACE, do not AUGMENT — a naive
"call Validate THEN keep the existing Unmarshal" would have preserved the
library mismatch.

Test changes:
  - Existing valid-YAML fixtures gain "kind: hosted" (required field).
  - 3 new failure tests: MissingKind, InvalidKind, InvalidName — locking
    in that doctor surfaces the same errors deploy would surface.

Pre-flight: gofmt, vet, build, doctor 6.7s + full extension suite 24s on
cmd / 9.5s on agent_api / etc — all green. golangci-lint 0 issues. cspell
0 issues.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: Antriksh Jain

cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_project.go

@@ -14,7 +14,6 @@ import (
 	"azureaiagent/internal/pkg/agents/agent_yaml"
 
 	"github.com/azure/azure-dev/cli/azd/pkg/azdext"
-	"gopkg.in/yaml.v3"
 )
 
 // agentHost is the value used in azure.yaml for an azure.ai.agent service.
@@ -43,7 +42,7 @@ func newCheckAgentServiceDetected(deps Dependencies) Check {
 			if deps.AzdClient == nil {
 				return Result{Status: StatusSkip, Message: "skipped: azd extension not reachable"}
 			}
-			if priorFailed(prior, "local.azure-yaml") {
+			if priorBlocked(prior, "local.azure-yaml") {
 				return Result{Status: StatusSkip, Message: "skipped: azure.yaml check failed"}
 			}
 
@@ -115,8 +114,8 @@ func newCheckProjectEndpointSet(deps Dependencies) Check {
 			if deps.AzdClient == nil {
 				return Result{Status: StatusSkip, Message: "skipped: azd extension not reachable"}
 			}
-			if priorFailed(prior, "local.environment-selected") {
-				return Result{Status: StatusSkip, Message: "skipped: environment check failed"}
+			if priorBlocked(prior, "local.environment-selected") {
+				return Result{Status: StatusSkip, Message: "skipped: environment check failed or skipped"}
 			}
 
 			resp, err := deps.AzdClient.Environment().GetValue(ctx, &azdext.GetEnvRequest{
@@ -173,8 +172,8 @@ func newCheckAgentYAMLValid(deps Dependencies) Check {
 			if deps.AzdClient == nil {
 				return Result{Status: StatusSkip, Message: "skipped: azd extension not reachable"}
 			}
-			if priorFailed(prior, "local.agent-service-detected") {
-				return Result{Status: StatusSkip, Message: "skipped: no agent services detected"}
+			if priorBlocked(prior, "local.agent-service-detected") {
+				return Result{Status: StatusSkip, Message: "skipped: no agent services detected or upstream check blocked"}
 			}
 
 			resp, err := deps.AzdClient.Project().Get(ctx, &azdext.EmptyRequest{})
@@ -251,27 +250,32 @@ func newCheckAgentYAMLValid(deps Dependencies) Check {
 	}
 }
 
-// validateAgentYAML reads the file at path and ensures it parses as a
-// ContainerAgent. Returns the underlying read/parse error verbatim so
-// the caller can attribute it to the offending service.
+// validateAgentYAML reads the file at path and runs the same validation
+// (`agent_yaml.ValidateAgentDefinition`) that the deploy path uses, so a
+// PASS here implies the manifest will not be rejected by deploy for any
+// of: missing/invalid `kind`, missing/invalid `name`, or kind-specific
+// structural problems. Returns the underlying read/validate error
+// verbatim so the caller can attribute it to the offending service.
 func validateAgentYAML(path string) error {
 	data, err := os.ReadFile(path) //nolint:gosec // G304: path is constructed from azd-resolved project root + service-relative path
 	if err != nil {
 		return fmt.Errorf("read %s: %w", path, err)
 	}
-	var parsed agent_yaml.ContainerAgent
-	if err := yaml.Unmarshal(data, &parsed); err != nil {
-		return fmt.Errorf("parse %s: %w", path, err)
+	if err := agent_yaml.ValidateAgentDefinition(data); err != nil {
+		return fmt.Errorf("validate %s: %w", path, err)
 	}
 	return nil
 }
 
-// priorFailed reports whether the prior results contain a Fail entry
-// for the given check ID. Used for skip-cascade decisions across the
-// local-checks chain.
-func priorFailed(prior []Result, id string) bool {
+// priorBlocked reports whether the prior results contain a Fail or Skip
+// entry for the given check ID. Used for skip-cascade decisions across
+// the local-checks chain: when an upstream check is skipped (e.g.
+// because *its* upstream failed), downstream checks must also skip
+// rather than running on a broken-state assumption — otherwise users
+// see misleading remediation for the wrong root cause.
+func priorBlocked(prior []Result, id string) bool {
 	for _, p := range prior {
-		if p.ID == id && p.Status == StatusFail {
+		if p.ID == id && (p.Status == StatusFail || p.Status == StatusSkip) {
 			return true
 		}
 	}

cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_project_test.go

@@ -179,6 +179,28 @@ func TestCheckProjectEndpointSet_PriorEnvFailed_Skips(t *testing.T) {
 	require.Contains(t, got.Message, "environment check failed")
 }
 
+func TestCheckProjectEndpointSet_PriorEnvSkipped_AlsoSkips(t *testing.T) {
+	// Covers the cascade: azure-yaml fails -> environment-selected skips ->
+	// project-endpoint-set must also skip. Without this propagation, check 5
+	// would run against an unloaded env and surface misleading remediation
+	// for the wrong root cause.
+	t.Parallel()
+
+	client := newTestAzdClient(t,
+		&fakeProjectServer{},
+		// If the check incorrectly proceeds past the guard it would call
+		// GetValue; set valueErr so we'd see the wrong-path Fail in the
+		// assertion instead of a quiet Skip.
+		&fakeEnvironmentServer{valueErr: errors.New("should not be called")})
+	check := newCheckProjectEndpointSet(Dependencies{AzdClient: client})
+
+	prior := []Result{{ID: "local.environment-selected", Status: StatusSkip}}
+	got := check.Fn(t.Context(), Options{}, prior)
+
+	require.Equal(t, StatusSkip, got.Status)
+	require.Contains(t, got.Message, "environment check failed or skipped")
+}
+
 func TestCheckProjectEndpointSet_GRPCError_Fails(t *testing.T) {
 	t.Parallel()
 
@@ -287,6 +309,26 @@ func TestCheckAgentYAMLValid_PriorAgentDetectionFailed_Skips(t *testing.T) {
 	require.Contains(t, got.Message, "no agent services detected")
 }
 
+func TestCheckAgentYAMLValid_PriorAgentDetectionSkipped_AlsoSkips(t *testing.T) {
+	// Covers the cascade: azure-yaml fails -> agent-service-detected skips ->
+	// agent-yaml-valid must also skip. Without this propagation, check 6
+	// would re-fetch the project (failing identically to check 2) and
+	// surface a duplicate failure for the same root cause.
+	t.Parallel()
+
+	client := newTestAzdClient(t,
+		// Server set up to fail if reached, to ensure the guard short-circuits.
+		&fakeProjectServer{err: errors.New("should not be called")},
+		&fakeEnvironmentServer{})
+	check := newCheckAgentYAMLValid(Dependencies{AzdClient: client})
+
+	prior := []Result{{ID: "local.agent-service-detected", Status: StatusSkip}}
+	got := check.Fn(t.Context(), Options{}, prior)
+
+	require.Equal(t, StatusSkip, got.Status)
+	require.Contains(t, got.Message, "no agent services detected or upstream check blocked")
+}
+
 func TestCheckAgentYAMLValid_GRPCError_Fails(t *testing.T) {
 	t.Parallel()
 
@@ -321,6 +363,7 @@ func TestCheckAgentYAMLValid_OneServiceValid_Passes(t *testing.T) {
 	projectPath := t.TempDir()
 	require.NoError(t, os.MkdirAll(filepath.Join(projectPath, "src", "agent"), 0o750))
 	writeYAML(t, projectPath, "src/agent/agent.yaml", `
+kind: hosted
 name: echo-agent
 language: python
 entrypoint: main.py
@@ -352,7 +395,7 @@ func TestCheckAgentYAMLValid_NonAgentServicesIgnored(t *testing.T) {
 
 	projectPath := t.TempDir()
 	require.NoError(t, os.MkdirAll(filepath.Join(projectPath, "src", "agent"), 0o750))
-	writeYAML(t, projectPath, "src/agent/agent.yaml", "name: echo\nlanguage: python\n")
+	writeYAML(t, projectPath, "src/agent/agent.yaml", "kind: hosted\nname: echo\nlanguage: python\n")
 
 	client := newTestAzdClient(t,
 		&fakeProjectServer{resp: &azdext.GetProjectResponse{
@@ -438,7 +481,7 @@ func TestCheckAgentYAMLValid_MixedValidAndInvalid_Fails(t *testing.T) {
 	projectPath := t.TempDir()
 	require.NoError(t, os.MkdirAll(filepath.Join(projectPath, "src", "ok"), 0o750))
 	require.NoError(t, os.MkdirAll(filepath.Join(projectPath, "src", "bad"), 0o750))
-	writeYAML(t, projectPath, "src/ok/agent.yaml", "name: ok-agent\nlanguage: python\n")
+	writeYAML(t, projectPath, "src/ok/agent.yaml", "kind: hosted\nname: ok-agent\nlanguage: python\n")
 	// bad: malformed yaml (mapping key with no value, broken indent).
 	writeYAML(t, projectPath, "src/bad/agent.yaml", "name: bad\n  : nope\n\t- tabs-here\n")
 
@@ -471,9 +514,9 @@ func TestCheckAgentYAMLValid_MixedValidAndInvalid_Fails(t *testing.T) {
 	require.Len(t, validated, 1)
 }
 
-// ---- helper: priorFailed ----
+// ---- helper: priorBlocked ----
 
-func TestPriorFailed(t *testing.T) {
+func TestPriorBlocked(t *testing.T) {
 	t.Parallel()
 
 	cases := []struct {
@@ -485,22 +528,121 @@ func TestPriorFailed(t *testing.T) {
 		{"empty prior", nil, "x", false},
 		{"matching fail", []Result{{ID: "x", Status: StatusFail}}, "x", true},
 		{"matching pass", []Result{{ID: "x", Status: StatusPass}}, "x", false},
-		{"matching skip", []Result{{ID: "x", Status: StatusSkip}}, "x", false},
+		// Skip propagates blocking: if upstream skipped (because *its* upstream failed),
+		// downstream checks must also skip rather than run on broken assumptions.
+		{"matching skip", []Result{{ID: "x", Status: StatusSkip}}, "x", true},
 		{"matching warn", []Result{{ID: "x", Status: StatusWarn}}, "x", false},
 		{"different id fail", []Result{{ID: "y", Status: StatusFail}}, "x", false},
-		{"id matches middle entry", []Result{
+		{"different id skip", []Result{{ID: "y", Status: StatusSkip}}, "x", false},
+		{"id matches middle entry fail", []Result{
 			{ID: "a", Status: StatusPass},
 			{ID: "x", Status: StatusFail},
 			{ID: "c", Status: StatusPass},
 		}, "x", true},
+		{"id matches middle entry skip", []Result{
+			{ID: "a", Status: StatusPass},
+			{ID: "x", Status: StatusSkip},
+			{ID: "c", Status: StatusPass},
+		}, "x", true},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.want, priorFailed(tc.prior, tc.id))
+			require.Equal(t, tc.want, priorBlocked(tc.prior, tc.id))
 		})
 	}
 }
 
+func TestCheckAgentYAMLValid_MissingKind_Fails(t *testing.T) {
+	// Without explicit `kind:`, ValidateAgentDefinition rejects the manifest
+	// because kind is required. Doctor must catch this pre-flight rather
+	// than letting deploy be the first place that surfaces it.
+	t.Parallel()
+
+	projectPath := t.TempDir()
+	writeYAML(t, projectPath, "src/agent/agent.yaml", "name: echo-agent\nlanguage: python\n")
+
+	client := newTestAzdClient(t,
+		&fakeProjectServer{resp: &azdext.GetProjectResponse{
+			Project: &azdext.ProjectConfig{
+				Path: projectPath,
+				Services: map[string]*azdext.ServiceConfig{
+					"echo-agent": {Name: "echo-agent", Host: agentHost, RelativePath: "src/agent"},
+				},
+			},
+		}},
+		&fakeEnvironmentServer{})
+	check := newCheckAgentYAMLValid(Dependencies{AzdClient: client})
+
+	got := check.Fn(t.Context(), Options{}, nil)
+
+	require.Equal(t, StatusFail, got.Status)
+	require.Contains(t, got.Message, "echo-agent")
+	failures, ok := got.Details["failures"].([]string)
+	require.True(t, ok)
+	require.Len(t, failures, 1)
+	require.Contains(t, failures[0], "kind")
+}
+
+func TestCheckAgentYAMLValid_InvalidKind_Fails(t *testing.T) {
+	// A `kind` that isn't in ValidAgentKinds() (hosted/workflow) must be
+	// rejected. Bare yaml.Unmarshal would silently accept this; the
+	// production deploy path rejects it via ValidateAgentDefinition.
+	t.Parallel()
+
+	projectPath := t.TempDir()
+	writeYAML(t, projectPath, "src/agent/agent.yaml", "kind: nonsense\nname: echo-agent\nlanguage: python\n")
+
+	client := newTestAzdClient(t,
+		&fakeProjectServer{resp: &azdext.GetProjectResponse{
+			Project: &azdext.ProjectConfig{
+				Path: projectPath,
+				Services: map[string]*azdext.ServiceConfig{
+					"echo-agent": {Name: "echo-agent", Host: agentHost, RelativePath: "src/agent"},
+				},
+			},
+		}},
+		&fakeEnvironmentServer{})
+	check := newCheckAgentYAMLValid(Dependencies{AzdClient: client})
+
+	got := check.Fn(t.Context(), Options{}, nil)
+
+	require.Equal(t, StatusFail, got.Status)
+	failures, ok := got.Details["failures"].([]string)
+	require.True(t, ok)
+	require.Len(t, failures, 1)
+	require.Contains(t, failures[0], "kind")
+}
+
+func TestCheckAgentYAMLValid_InvalidName_Fails(t *testing.T) {
+	// Agent name must match `^[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?$`
+	// (DNS-style). An underscore is invalid for deployable agent names.
+	// Doctor must surface this before deploy, not after.
+	t.Parallel()
+
+	projectPath := t.TempDir()
+	writeYAML(t, projectPath, "src/agent/agent.yaml", "kind: hosted\nname: My_Agent\nlanguage: python\n")
+
+	client := newTestAzdClient(t,
+		&fakeProjectServer{resp: &azdext.GetProjectResponse{
+			Project: &azdext.ProjectConfig{
+				Path: projectPath,
+				Services: map[string]*azdext.ServiceConfig{
+					"my-agent": {Name: "my-agent", Host: agentHost, RelativePath: "src/agent"},
+				},
+			},
+		}},
+		&fakeEnvironmentServer{})
+	check := newCheckAgentYAMLValid(Dependencies{AzdClient: client})
+
+	got := check.Fn(t.Context(), Options{}, nil)
+
+	require.Equal(t, StatusFail, got.Status)
+	failures, ok := got.Details["failures"].([]string)
+	require.True(t, ok)
+	require.Len(t, failures, 1)
+	require.Contains(t, failures[0], "name")
+}
+
 // writeYAML is a tiny test helper that writes the given content to
 // <root>/<rel> after ensuring the parent directory exists.
 func writeYAML(t *testing.T, root, rel, content string) {