Azure AI Agents: add next-step guidance and doctor diagnostics#8198
Merged
trangevi merged 82 commits intoMay 22, 2026
Conversation
antriksh30
requested review from
JeffreyCA,
hemarina,
jongio,
rajeshkamal5050,
tg-msft,
therealjohn,
trangevi,
trrwilson,
vhvb1989,
wbreza and
weikanglim
as code owners
antriksh30
changed the title
antriksh30
force-pushed
the
antrikshjain/next-step-implementation
branch
from
401ca65 to
829063a
Compare
jongio
reviewed
May 17, 2026
Member
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Solid foundation for the doctor + nextstep subsystems - clean module boundaries, interface-driven testability, and well-structured check pipeline. A few things to sort out before this leaves draft:
invoke.go:541-543 is the one I'd prioritize - it silently changes CLI semantics for --name. The rest are lower-stakes but worth a look.
antriksh30
pushed a commit
to antriksh30/azure-dev
that referenced
this pull request
May 18, 2026
The remote.auth doctor check surfaced the raw user principal name in
both the Message string and the structured Details map regardless of
the --unredacted flag, in contrast with the rest of the doctor checks
(checks_rbac.go, checks_agent_identity_roles.go) which already gate
identity values on opts.Unredacted.
This change threads Options into the auth check function and adds two
small helpers that reuse the existing redactedPlaceholder constant:
- redactUPN(upn, unredacted) returns the value to surface in Message
text: raw when --unredacted, the shared <redacted> placeholder when
a UPN was discovered but should be scrubbed, and empty when none
was found so composeAuthMessage cleanly drops the prefix.
- authDetails(upn, minutes, unredacted) builds the Details map and
omits the "upn" key entirely unless --unredacted is set, so
machine consumers do not see the raw value by default.
PASS, WARN, and expired-FAIL branches now compose their messages from
the redacted display value. Existing tests that asserted the raw UPN
were updated to pass Options{Unredacted: true}; new table tests cover
the default-redacted and --unredacted contracts on every branch, the
empty-UPN drop, and both helpers in isolation.
Resolves PR Azure#8198 review comment from @jongio.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
antriksh30
pushed a commit
to antriksh30/azure-dev
that referenced
this pull request
May 18, 2026
TestErrorCodeWireValues pins the lowercase JSON wire values of every exported enum the nextstep package consumes from the Agents API, but the AgentVersionStatus map was missing the "idle" entry. The idle status is actively read at show.go:207 and nextstep/resolver.go:248, so silent drift on that one literal would regress the show command's idle branch and the resolver's deployment-pending hint without any test failure. Add the missing "idle": string(AgentVersionIdle) case. Resolves PR Azure#8198 Copilot review comments (ids 3246075889, 3246075800). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
antriksh30
changed the title
antriksh30
force-pushed
the
antrikshjain/next-step-implementation
branch
from
6312729 to
da2efb5
Compare
antriksh30
pushed a commit
to antriksh30/azure-dev
that referenced
this pull request
May 20, 2026
The remote.auth doctor check surfaced the raw user principal name in
both the Message string and the structured Details map regardless of
the --unredacted flag, in contrast with the rest of the doctor checks
(checks_rbac.go, checks_agent_identity_roles.go) which already gate
identity values on opts.Unredacted.
This change threads Options into the auth check function and adds two
small helpers that reuse the existing redactedPlaceholder constant:
- redactUPN(upn, unredacted) returns the value to surface in Message
text: raw when --unredacted, the shared <redacted> placeholder when
a UPN was discovered but should be scrubbed, and empty when none
was found so composeAuthMessage cleanly drops the prefix.
- authDetails(upn, minutes, unredacted) builds the Details map and
omits the "upn" key entirely unless --unredacted is set, so
machine consumers do not see the raw value by default.
PASS, WARN, and expired-FAIL branches now compose their messages from
the redacted display value. Existing tests that asserted the raw UPN
were updated to pass Options{Unredacted: true}; new table tests cover
the default-redacted and --unredacted contracts on every branch, the
empty-UPN drop, and both helpers in isolation.
Resolves PR Azure#8198 review comment from @jongio.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the third remote doctor check, `remote.rbac`, implementing Check Azure#10 of issue Azure#7975 / Check 10 of the design doc (`azd-ai-agent-doctor-remote-checks.md` lines 159-180): "Developer has required role on Foundry project". The check queries the developer's role assignments on the Foundry project's ARM scope via the Microsoft Graph + ARM stack, then classifies the result: * Pass: "<display> has the required role on project '<acct>/<proj>'" (display name replaced with "the current principal" in the default redacted mode for UPN safety). * Fail: templated `az role assignment create --role "Azure AI User" --assignee <oid> --scope <scope>` + a learn.microsoft.com link to the RBAC concepts page. Principal ID and scope ARN are substituted with shell-safe ALL_CAPS placeholders (OBJECT_ID / PROJECT_SCOPE) in redacted mode — NOT `<redacted>`, because bash/zsh interpret `<word>` as input redirection. * Skip: precondition unmet (no AzdClient, no env, auth Failed, `AZURE_AI_PROJECT_ID` missing/malformed/unset, service-principal token detected, transient Graph/ARM error, cancellation). Architecture ------------ Two new files: internal/project/developer_rbac_query.go (~175 LoC) - QueryDeveloperRBAC: side-effect-free wrapper returning a structured DeveloperRBACResult. Reuses package-private parseAgentIdentityInfo / hasAnyRoleAssignment / sufficientAIUserRoles from developer_rbac_check.go. - ValidateProjectResourceID: shape check for AZURE_AI_PROJECT_ID, returns wrapped ErrInvalidProjectResourceID sentinel. - ErrInvalidProjectResourceID, ErrSPNDelegatedAuthRequired: sentinel errors for diagnostic classification. - Graph /me failure detection: routes app-only/SPN token rejections onto ErrSPNDelegatedAuthRequired (case-insensitive message match) so doctor surfaces a SPN-aware Skip. internal/cmd/doctor/checks_rbac.go (~430 LoC) - newCheckRBAC: skip-cascade + projectID lookup + upfront ValidateProjectResourceID gate + probe dispatch. - classifyRBACProbeError: sentinel-keyed error classification (Canceled / SPN / InvalidProjectID / generic transient). - classifyRBACResult: pure Pass/Fail mapping for diagnostic consumers. - sanitizeScopeARNs: regex-based scope+GUID scrubber for probe error text. - readProjectResourceID: AZURE_AI_PROJECT_ID env lookup via gRPC EnvironmentService. - redactID / redactScope / redactDisplay: centralized placeholder substitution helpers. Two new seams on Dependencies: probeDeveloperRBAC - replaces project.QueryDeveloperRBAC readProjectResourceIDFn - replaces readProjectResourceID Skip-cascade rationale ---------------------- Per design dependency matrix (line 115), `remote.rbac` cascades against `local.environment-selected` + `remote.auth` but NOT `remote.foundry-endpoint`. RBAC reads ARM, not the data plane; a transient DNS / proxy / outage on the data-plane check must not prevent the user from learning their role assignment is missing. `TestCheckRBAC_DoesNotSkipOnFoundryEndpointFail` pins this. Probe errors → Skip (not Fail) to avoid false alarms on transient Graph/ARM hiccups. Cancellation similarly Skips with a clean message rather than rendering an error trace. Review fixes applied -------------------- Following the 3-reviewer pass (Opus xhigh + Sonnet 4.6 + GPT-5.5) of an earlier draft (commit 0c4d5ee), the following findings were addressed: * MEDIUM (Opus + GPT-5.5): probe-error path leaked raw subscription/scope IDs via azcore.ResponseError.Error()'s first line (`GET https://management.azure.com/subscriptions/...`) into Message + Details. Fix: sanitizeScopeARNs regex pass strips ARM scopes + bare GUIDs from Message in redacted mode; Details["probeError"] is OMITTED entirely unless --unredacted. * MEDIUM (Sonnet 4.6): malformed AZURE_AI_PROJECT_ID got "check your network" Suggestion despite no network call. Fix: upfront ValidateProjectResourceID gate runs before the probe; surfaces "is not a valid Foundry project ARM resource ID" with an `azd env set` Suggestion. * MEDIUM (GPT-5.5): PrincipalDisplay rendered verbatim in Message even in default redacted mode; display names can contain UPN fragments (e.g., "Alice (alice@contoso.com)"). Fix: redactedDisplayLabel ("the current principal") substitutes for raw display in default mode; unredacted mode still echoes the real display. * MEDIUM (GPT-5.5): service-principal `azd auth login` cannot use Graph /me — confusing generic transient Skip. Fix: ErrSPNDelegatedAuthRequired sentinel + case-insensitive detection of the canonical "delegated authentication flow" Graph response; doctor surfaces a SPN-aware Skip with user-delegated guidance. * LOW/Nit (Opus): `<redacted>` is a bash input-redirection token; `--assignee <redacted>` would fail with `redacted: No such file or directory` on copy-paste. Fix: shellSafePlaceholderID/Scope constants render `OBJECT_ID` / `PROJECT_SCOPE` in the templated az command. Verified clean (no action): skip-cascade structure, firstLine helper, AZURE_AI_PROJECT_ID provenance (set by init_foundry_resources_helpers.go:327), seam design, account-scope check missing (acceptable per `assignedTo()` inheriting parent- scope assignments). Testing ------- 22 unit tests in checks_rbac_test.go and developer_rbac_query covering: skip cascades, probe-error branches (transient, parse, SPN, defensive sentinel, cancellation), end-to-end probe Pass/ Fail, classifyRBACResult mapping with both redaction modes, display-name fallback, sensitive-identifier leak prevention, shell-safe placeholder substitution, ValidateProjectResourceID shape coverage, sanitizeScopeARNs regex coverage, all three redaction helpers (redactID/Scope/Display) with empty-input + flag permutations. Preflight (from cli/azd/extensions/azure.ai.agents) --------------------------------------------------- gofmt -s -w . - clean go vet ./... - clean go build ./... - clean go test ./... -count=1 - all packages green golangci-lint run ./internal/cmd/... - 0 issues ./internal/project/... npx cspell lint - 0 issues "internal/cmd/doctor/**/*.go" "internal/project/developer_rbac_query.go" Refs: Azure#7975 (PR Azure#8057, Phase 5 / C16) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the fourth (and final per the current phase-5 scope) remote
doctor check: `remote.agent-status`. For each hosted-agent service
declared in azure.yaml, the check resolves the deployed agent
name + version from the active azd environment (the AGENT_<KEY>_NAME
and AGENT_<KEY>_VERSION values written by service_target_agent.go
on a successful `azd deploy`) and probes Foundry's
`GET /agents/{name}/versions/{version}` endpoint, mapping the
lifecycle status to a doctor classification.
Per-service classification
- Active → Pass (one of N agents active)
- Creating → Warn (azd ai agent monitor --follow)
- Failed → Fail (azd ai agent monitor --follow,
NOT azd deploy — read logs first)
- 404 / Deleting / Deleted → Fail (azd deploy)
- AGENT_<KEY>_NAME unset, OR NAME set but AGENT_<KEY>_VERSION
unset → Fail (azd deploy — the service is
declared but has not been
deployed cleanly; the post-deploy
hook writes both env vars
atomically so a half-pair is a
deterministic config issue)
- Unrecognized status → Fail with raw status surfaced so the
user can search; suggests the Foundry
portal
- Probe error / 401-403 → service-scoped transient skip; does
NOT fail the aggregate when other
services are healthy
Aggregate
The per-service entries fold into a single doctor Result via a
ranked classifier (`agentClassRank`). Worst-class drives the
aggregate Status:
- All Active → Pass
- Worst is Failed → Fail (monitor)
- Worst is Missing (404 / Deleted / Deleting) → Fail (deploy)
- Worst is NotDeployed (no env var / half-pair) → Fail (deploy)
- Worst is Unknown → Fail (portal)
- Worst is Deploying / Creating → Warn (monitor)
- Worst is Transient AND ≥1 Active → Pass with
Message note
- All Transient → Skip (retry)
When multiple failing classes coexist (e.g., one Failed agent and
one Missing agent in the same project) the dominant class drives
the headline and Suggestion. Detail lines are filtered to the
dominant class only so the headline count and the rendered body
always agree; a short "other agents have additional issues" hint
is appended to the Suggestion telling the user to read Details for
the secondary fix path.
The Message lists up to 3 failing services (with `(N more)` after
that). Active services are listed in the all-active Pass message.
Details["services"] always carries the full per-service list for
JSON consumers.
Fan-out
Probes execute concurrently across services with a bounded worker
pool (probeConcurrency = 4) so wall-clock cost is bounded by the
slowest probe rather than the sum of probes. Each probe still
enforces its own 6 s timeout via agentStatusProbeTimeout; the
parent ctx propagates cancellation to all in-flight workers.
Skip-cascade (per design dependency matrix lines 110-117 of
.tmp/pr-8057/azd-ai-agent-doctor-remote-checks.md):
- local.environment-selected (env reads would fail)
- local.agent-service-detected (no service list ⇒ Pass = bug)
- remote.auth (no token ⇒ every probe 401s)
- remote.foundry-endpoint (no reachability ⇒ N transports)
We deliberately do NOT cascade from `remote.rbac`: agent-list /
agent-get is Reader-level, and developers with read-only access
on the Foundry project still benefit from knowing whether their
agents are healthy. Pinned by
TestCheckAgentStatus_DoesNotSkipOnRBACFail.
Review findings applied (Opus 4.7 xhigh + Sonnet 4.6 + GPT-5.5)
- MEDIUM (Sonnet Azure#1 + Opus Azure#1): doc/code/test disagreement on
Active + Transient mix. The godoc promised "Pass with note"
but the code returned Skip and the test pinned the wrong
behavior. Implemented the documented Pass-with-note so a
transient probe failure for one service does not mask the
healthy state of the rest. Test renamed to
TestCheckAgentStatus_Aggregate_ActiveAndTransient_PassWithNote
and asserts the Message text.
- MEDIUM (Sonnet Azure#2 + Opus Azure#2 + GPT-5.5 Azure#3): headline count and
rendered detail body diverged when entries from multiple
failing classes coexisted (e.g., "1 of 2 agents are in a
failed state" with 2 detail lines including a Missing entry).
The aggregate now filters detail lines to the dominant class
via `detailsForClass(worst)`, and the Suggestion is enriched
with an "Other agents have additional issues (<classes>); see
the per-service Details" hint when other failing classes are
present. Pinned by
TestCheckAgentStatus_Aggregate_FailedDominatesMissing.
- MEDIUM (GPT-5.5 Azure#1): AGENT_<KEY>_NAME present with
AGENT_<KEY>_VERSION empty was classified as `transient`,
surfacing "retry doctor" — wrong for a deterministic config
issue. Reclassified as `not-deployed` so the user is told to
`azd deploy`. Test
TestCheckAgentStatus_MismatchedNameVersion_NotDeployedForService
+ TestProbeOneService_NameSetVersionEmpty_NotDeployed.
- MEDIUM (GPT-5.5 Azure#2): serial probes — 100 services × 6 s could
drag the doctor run past 10 minutes. The design spec calls it
"fan out", so probes now run in parallel via a bounded
4-worker pool (probeConcurrency). Order preservation guarantees
deterministic Details rendering.
- LOW (Sonnet Azure#3): added TestProbeOneService_DeletingStatus_Missing
covering the `Deleting` lifecycle branch that previously had no
direct test (only `Deleted` was covered).
Files
- internal/cmd/doctor/checks_agent_status.go
* newCheckAgentStatus + the Check shape
* probeOneService — per-service classification body
(now-corrected name-set-version-empty path)
* probeAllServices — bounded-concurrency fan-out helper
* classifyAgentStatusAggregate — folds entries into one
Result with class-filtered detail lines + mixed-class
Suggestion hint + Active+Transient Pass-with-note branch
* makeRealProbeAgentStatus — production probe closure (uses
agent_api.GetAgentVersion + azidentity.NewAzureDeveloperCLI
Credential, the same auth path the runtime invoke flow uses)
* readAgentNameVersion + readAgentServices helpers
* doctorServiceKey (mirrors cmd.toServiceKey; duplicated to
avoid an import cycle, same rationale as agentHost in
checks_project.go)
* Lifecycle constants (Active / Creating / Failed / Deleted /
Deleting) sourced from
vienna:Contracts/V2/Generated/Agents/AgentVersionStatus.cs
* truncateLines / serviceNamesByClass / firstTransient
- internal/cmd/doctor/checks_agent_status_test.go (~640 LoC, 34
tests; +2 new tests over v1)
* Skip-cascade gates × 9
* Per-service classification × 7 (incl. Deleting, NameNoVersion)
* Status case-insensitive matching
* Aggregate ranking × 5 (incl. Active+Transient Pass-with-note,
FailedDominatesMissing with Message-text assertion)
* Aggregate truncation at 3 + "(N more)"
* probeOneService transport branches × 6 (incl. context.Canceled
and context.DeadlineExceeded handling)
* Service-key edge cases, rank fallback, truncateLines
boundary, makeRealProbeAgentStatus closure check, plus a
ServerHandler-based smoke test that the
azcore.ResponseError code is surfaced via statusCode
- internal/cmd/doctor/checks_local.go
* Dependencies struct grows two new seams:
probeAgentStatus + readAgentNameVersionFn (mirrors the
probeDeveloperRBAC + readProjectResourceIDFn pattern from C16)
- internal/cmd/doctor/checks_remote.go
* NewRemoteChecks adds newCheckAgentStatus(deps) as the 4th
entry, after auth / foundry-endpoint / rbac
- internal/cmd/doctor/checks_remote_test.go
* TestNewRemoteChecks_HasAuthFoundryEndpointRBACAndAgentStatus
pins the 4-entry shape
Out of scope (deferred)
- Sharing the credential across services: each probe currently
constructs its own azidentity.NewAzureDeveloperCLICredential.
Since the credential is essentially a thin shell around
`azd auth token`, the cost is negligible (a single in-process
call per probe) and threading it through complicates the
test-seam shape. Will revisit if benchmarks surface it.
- Re-using readAgentNameVersion for the doctor's eventual ENV
pretty-print mode. Out of scope for the check itself; the
helper is unexported and can be promoted when the renderer
needs it.
Preflight (from cli/azd/extensions/azure.ai.agents)
- gofmt -s -w . clean
- go vet ./... clean
- go build ./... clean
- go test ./... -count=1 32 doctor tests + full ext suite PASS
- golangci-lint run ./... 0 issues
- cspell lint ... (cspell.yaml) 0 issues
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes two pre-existing UX bugs in `azd ai agent run`:
B5 (Next: race): pre-C8 the Next: block was rendered BEFORE
`proc.Start()`, so its "in another terminal, try: <example>" line could
land in the user's terminal seconds before the agent's port was bound.
Following the suggestion verbatim while the agent was still starting
yielded a connection-refused.
B6 (stale cached OpenAPI): pre-C8 the OpenAPI probe was strictly
cache-only. The first `run` ever issued — and every `run` against an
agent whose schema had drifted — surfaced a protocol-generic
`<payload>` literal instead of the agent's actual example.
Scope — only `run.go` uses the live probe. `show.go` (`WithOpenAPIProbe
(name, "remote")`), `deploy`/artifact-note path, `init.go`, and
`init_from_code.go` remain cache-only by design.
Changes
-------
nextstep/state.go
- New functional option `WithLiveOpenAPIProbe(fetch func(context.
Context) ([]byte, error))`. Stores the fetcher in
`cfg.openAPILiveFetch`.
- `populateOpenAPIPayload` now takes `(ctx, cfg, projectPath, envName,
state)`. Order of resolution: (1) live, on success → use it;
(2) cache (existing `WithOpenAPIProbe` path), on success → use it;
(3) leave HasOpenAPI=false. Every failure is silent.
- `assembleState` threads `ctx` through to the new signature.
- Doc comment on `WithOpenAPIProbe` updated to note that
`WithLiveOpenAPIProbe` overrides it when both are supplied.
run.go
- Removed the pre-Start `nextstep.AssembleState` + `PrintNext` block
that was emitting Next: before the agent bound its port.
- After `proc.Start()`, spawn `emitNextAfterBind` in a goroutine with
a `nextDone` channel. After `proc.Wait()` returns, the main flow
calls `cancel()` and waits on `<-nextDone` so the goroutine is
fully joined before stdout writes from `runRun`'s caller resume —
closes a stdout race on shutdown.
- `emitNextAfterBind` early-returns when stdout is not a terminal,
honoring the documented nextstep call-site TTY-gating contract
(matches `invoke.go:217`, `show.go:381`). Redirected stdout
(`run > log`) no longer receives the banner or Next: block.
- After waitForPortReady succeeds, builds a closure that wraps
`fetchLiveOpenAPI(ctx, port)` and passes BOTH
`WithOpenAPIProbe(serviceName, "local")` (cache fallback) and
`WithLiveOpenAPIProbe(...)` to `AssembleState`. The state
assembler picks live first, falls back to cache silently if the
live fetch fails.
- Re-checks `ctx.Err()` after `AssembleState` returns so a Ctrl+C
arriving mid-call doesn't surface "Agent ready" after
"Agent stopped." was already printed.
- Four new constants: `portReadyBudget` (5 s),
`portReadyPollInterval` (100 ms), `portReadyDialTimeout` (50 ms),
`liveOpenAPITimeout` (3 s).
- `waitForPortReady(ctx, port, budget) bool`: bounded TCP dial-loop
that honors ctx.
- `fetchLiveOpenAPI(ctx, port) ([]byte, error)`: uses
`http.NewRequestWithContext` to GET
`http://localhost:<port>/invocations/docs/openapi.json`. The route
matches the cache-side fetcher in `helpers.go:368` and the
user-facing curl tip in `nextstep/resolver.go:226`. Non-200
responses are returned as errors so the assembler falls back to
cache rather than ingesting a 404 body via `ExtractInvokeExample`.
Tests
-----
state_test.go (+5 new TestAssembleState_WithLiveOpenAPIProbe_* cases +
expanded `TestOptionsApplyCleanly`):
- PrefersLiveOverCache, FallsBackToCacheOnError,
FallsBackToCacheOnEmptyBody, LiveWorksEvenWithoutCacheProbe,
LiveFailureWithoutCacheLeavesUnset.
run_test.go (+8 new tests + `listenLoopback` helper):
- 3× waitForPortReady (bound port, budget elapse, ctx cancel).
- 3× fetchLiveOpenAPI (200 body asserts
`/invocations/docs/openapi.json` path, non-200 error, ctx
deadline).
- 2× emitNextAfterBind (never-binds, ctx cancelled — both pass nil
azdClient through the safe early-return paths to verify the helper
exits silently without panic or goroutine leak).
Preflight clean: gofmt -s -w, go vet, go build,
go test ./internal/cmd/... ./internal/cmd/nextstep/... -count=1
(cmd 10.5 s, doctor 1.6 s, nextstep 1.9 s), golangci-lint run
./internal/cmd/... (0 issues), cspell on the four modified files (0).
Review fixes (3-reviewer pass)
------------------------------
Three independent reviewers (Opus xhigh, Sonnet 4.6, GPT-5.5) reached
consensus on three correctness findings before merge:
- Live probe URL corrected to /invocations/docs/openapi.json
(matches existing cache fetcher and user-facing curl tip).
- Banner + PrintNext now gated on isTerminal(os.Stdout.Fd()) to
honor the nextstep call-site contract.
- emitNextAfterBind goroutine is now joined via nextDone channel
after proc.Wait, and re-checks ctx.Err() before printing so the
banner cannot land after "Agent stopped."
- Replaced misleading "ReturnsSilentlyWhenPortNeverBinds" test that
only exercised waitForPortReady with two tests that actually call
emitNextAfterBind with nil azdClient on the safe early-return
paths.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…P5.1 C12)
Adds the final doctor check (P5.1 C12) that surfaces deployed-agent
managed-identity role assignments at three ARM scopes — project,
account, and resource group. For each agent classified active by
the upstream `remote.agent-status` check, the new
`remote.agent-identity-roles` check fetches the agent's
`instance_identity.principal_id` from Foundry and lists ARM role
assignments at all three scopes via the existing
`armauthorization` SDK already pulled in for the developer-RBAC
check.
## What lands
`internal/project/agent_identity_query.go` (new, ~340 LoC):
- Public API `QueryAgentIdentityRoles(ctx, azdClient,
projectResourceID, principals) (*AgentIdentityRolesResult,
error)` reuses `parseAgentIdentityInfo` from
`agent_identity_rbac.go` to derive the three scope ARNs, looks
up the user-access tenant via `LookupTenant`, builds an
`AzureDeveloperCLICredential` pinned to that tenant, and fans
out per-principal role-assignment listings with `wg.Go`.
- Public types `AgentPrincipal`, `AgentScopeRoles`,
`AgentIdentityRolesEntry`, `AgentIdentityRolesScopes`,
`AgentIdentityRolesResult` form the structured listing the
doctor renders.
- `queryAgentIdentityRolesWithLister` separates the per-scope
listing strategy from credential acquisition so unit tests
drive the inner classifier without standing up ARM fakes.
- `listRoleNamesAtScope` lists role assignments with ARM's
server-side `assignedTo('<principal>')` filter, then resolves
role-definition IDs to human-readable names via
`RoleDefinitions.Get`. Failures on individual role-name
resolution downgrade gracefully (omitted from the listing).
`internal/cmd/doctor/checks_agent_identity_roles.go` (new, ~640 LoC
including doc comments):
- `newCheckAgentIdentityRoles(deps)` builds the check Closure.
Skip cascade against `local.environment-selected`,
`local.agent-service-detected`, `remote.auth`,
`remote.foundry-endpoint`, and `remote.agent-status`'s Pass
(per the design's "for each active agent found in check 11").
- `readActiveAgents(prior)` enumerates agents reachable to this
check by reading the upstream `remote.agent-status` Details'
`services` slice and filtering to Classification == "active".
- `classifyOneAgent` buckets a single agent into fine /
underscoped / empty / unknown per the design's pass condition
("project + (account|RG) covered"). `describeOneAgent`
renders the one-line per-agent breakdown
(`<agent>: project=N, account=M, resource-group=K`, with `?`
on probe-error scopes).
- `classifyAgentIdentityRolesAggregate` folds per-agent entries
into a single doctor Result: all "fine" → Info; any "empty"
→ Fail (smoking-gun for "every tool call 403s"); worst
"underscoped" → Warn; worst "unknown" → Warn.
- `makeRealProbeAgentPrincipal` builds the production probe
closure (mirrors `makeRealProbeAgentStatus` byte-for-byte
apart from the field consumed — `InstanceIdentity.PrincipalID`
vs `Status`).
## Renderer additions
`StatusInfo` joins the existing Pass/Warn/Fail/Skip status set so the
"all agents are fine" case can surface as an informational role
listing without flagging the run yellow.
- `types.go`: `StatusInfo Status = "info"` + `Summary.Info int`
(JSON tag added).
- `runner.go`: canonical validation switch + summarize switch
extended for Info; `ExitCode` treats Info as a "useful
diagnostic completed" status (matches Pass for exit-code
purposes).
- `doctor_format.go`: glyph "ⓘ" and label "INFO" added to
`statusGlyphAndLabel`; `writeSummaryLine` appends ", N info"
when Info > 0 (preserves existing test assertions otherwise).
## Dependencies wiring
`internal/cmd/doctor/checks_local.go` adds two test seams on
`Dependencies`:
- `probeAgentPrincipal` — replaces the production
`GetAgentVersion` call with a unit-test fake. Same signature
shape as `probeAgentStatus`.
- `queryAgentIdentityRoles` — replaces the production
`project.QueryAgentIdentityRoles` call. Signature mirrors the
public API so wiring is a single substitution.
`internal/cmd/doctor/checks_remote.go` appends
`newCheckAgentIdentityRoles(deps)` after the existing
`remote.agent-status` entry in `NewRemoteChecks`. The append-after
ordering is load-bearing — every skip-cascade guard in C12 reads
`remote.agent-status`'s Result from `prior []Result`, and the
local-then-remote ordering invariant remains intact (verified by
the existing `TestNewLocalAndRemoteChecks_ProductionCompositionLocalsFirst`).
## Tests
`internal/cmd/doctor/checks_agent_identity_roles_test.go` (new, 16 KB):
- Skip-cascade gates: nil AzdClient, `remote.agent-status` not
Passed, project endpoint missing, no active agents,
project-resource-ID unset, project-resource-ID malformed.
- Aggregate classification: Info when all fine; Fail when any
agent has zero roles; Warn when worst is underscoped; Warn on
transient query error.
- Per-agent classifier table (six cases: project+account,
project+RG, project-only, account-only, all-empty,
all-errored).
- Detail formatting: scope counts and `?` for probe-error
scopes.
- Missing-principal degradation: agent with no
`instance_identity` surfaces as a warning rather than a fail.
- `readActiveAgents` filtering invariants (active-only,
missing-name dropped, nil-return on missing upstream).
`internal/cmd/doctor/checks_remote_test.go` updated: the
`NewRemoteChecks` contract test now pins five entries (auth →
foundry-endpoint → rbac → agent-status → agent-identity-roles)
with their ID / Name / Remote / Fn invariants.
## Preflight
- gofmt -s -w . clean
- go vet ./... clean
- go build ./... clean
- go test ./... -count=1 all green
- internal/cmd 14.6s
- internal/cmd/doctor 1.6s
- internal/cmd/nextstep 3.8s
- internal/pkg/agents/agent_api 10.9s
- internal/pkg/agents/agent_yaml 1.0s
- internal/pkg/azure 12.7s
- internal/project 5.5s
- golangci-lint run ./internal/cmd/... ./internal/project/... 0 issues
- cspell on new files (after "underscoped" added to cspell.yaml) 0 issues
- copyright-check.sh on extension clean
## Design notes
- The spec at `.tmp/pr-8057/azd-ai-agent-doctor-remote-checks.md`
lines 193–223 specifies a per-agent fan-out at three scopes
with the "fine" pass condition (project + (account|RG)). Renders
as INFO rather than PASS because the design's intent is a
diagnostic listing — operators inspect it on `--output json`
and confirm no MI is starved; the check should not flip the
doctor green on its own.
- C12 uses the `wg.Go` Go 1.26 idiom for per-principal fan-out;
per-scope probes within one principal run sequentially (3
scopes × 1 ARM listing each is well under budget and avoids
the goroutine-per-scope-explosion).
- `probeAgentPrincipal` deliberately does NOT extend C17's
`probeAgentStatus` surface — extending it would couple two
independent checks. The mirror cost is one ~40-line factory
function shared by both.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…P5.1 C2)
Adds a best-effort manifest walker that surfaces model / toolbox /
connection resources from each service's `agent.manifest.yaml`
into `nextstep.State`. Unblocks the doctor checks C13 (model
deployments), C14 (toolboxes), and C15 (connections), all of which
need to know whether the relevant resource kinds are declared
before they can decide to run or skip.
## State additions
- `State.HasModels`, `State.HasToolboxes`, `State.HasConnections` —
aggregate boolean flags. True when at least one matching resource
is found across all `azure.ai.agent` services.
- `State.ModelRefs`, `State.Toolboxes`, `State.Connections` —
sorted `[]ResourceRef` per kind.
- `ResourceRef{Name, ServiceName, Detail}` — slim doctor-facing
shape. Detail carries the kind-specific identifier (model id,
connection `<Category> | <Target>`, empty for toolboxes).
## Walker semantics
- File names probed (in order): `agent.manifest.yaml`,
`agent.manifest.yml`. `agent.yaml` is deliberately excluded —
it describes the container, not declared resources.
- Uses `agent_yaml.ExtractResourceDefinitions` directly (NOT
`LoadAndValidateAgentManifest`) so a manifest with an absent /
partial `template` block — common during init — still surfaces
its `resources:` declarations.
- Best-effort: missing file, unreadable bytes, malformed YAML,
zero resources, and unknown resource kinds all silently degrade
(Has* flags stay false; lists stay nil). Walker never adds to
the `errs` slice so a manifest-in-flight (which init re-writes
mid-flow) never blocks the rest of state assembly.
- Dedup key is `(ServiceName, Name)`. Same name twice in one
service collapses to one entry (first-occurrence wins, matching
agent_yaml's manifest semantics). Same name under two services
surfaces twice so per-service doctor failures remain
individually addressable.
- Result slices are sorted by `(Name, ServiceName)` so doctor
output snapshots and downstream renderers are deterministic.
## Why this is its own commit
The walker is a pure data-collection step with no resolver-side
consumers in this commit. Doctor checks C13/C14/C15 (following
commits) gate-skip themselves on `state.Has{Models,Toolboxes,
Connections}` and iterate the matching ref slice. Landing the
walker first keeps each downstream commit focused on its single
check.
## Tests
8 new tests in `manifest_test.go`:
- All three kinds present → flags + lists populated, sort order
+ detail formatting locked.
- Missing manifest → silent, no errors logged through the
walker.
- Malformed YAML → silent, no errors.
- Manifest with no `resources:` key → silent, flags false.
- Multi-service aggregation → entries sorted by Name, ties
broken by ServiceName.
- Duplicate `(service, name)` within one manifest → first
occurrence wins.
- `.yaml` wins over `.yml` when both exist.
- `agent.yaml` (not a manifest) is ignored even if its content
happens to parse as one.
- `connectionDetail` table-driven test covers all four
category/target combinations.
## Preflight
- `gofmt -s -w .` — clean
- `go vet ./...` — clean
- `go test ./... -count=1` — full extension suite green
- `golangci-lint run ./internal/cmd/...` — 0 issues
- `cspell` over the touched files — 0 issues
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the seventh doctor remote check (`remote.model-deployments`) which verifies that every model resource declared in any service's `agent.manifest.yaml` (collected by the C2 manifest walker into `State.ModelRefs`) has a corresponding Cognitive Services deployment on the Foundry project's underlying account. # What it does For each project run: 1. Skip-cascade gates (in order): `AzdClient` nil → `local.environment-selected` → `local.azure-yaml` / `local.agent-service-detected` → `remote.auth` → `remote.foundry-endpoint` → `!state.HasModels` → `AZURE_AI_PROJECT_ID` unreadable / cannot be parsed. Every gate produces a single, actionable Skip message that points the user at the upstream check. 2. Parse `AZURE_AI_PROJECT_ID` (Foundry project ARM resource ID) for `(subscription, resourceGroup, accountName)` via the new `parseAccountFromProjectID` helper. Deployments live at the Cognitive Services *account* level, NOT the project level, so the account name is the load-bearing parameter here. 3. Issue exactly one `armcognitiveservices/v2.DeploymentsClient .NewListPager` round trip via the new `realProbeModelDeployments` helper, capped at 10s (matches the design's per-probe budget in `.tmp/pr-8057/azd-ai-agent-doctor-remote-checks.md`). Returns `[]string` of deployment names. Transport errors short-circuit the check to Skip with the error verbatim plus an actionable retry suggestion — we can not distinguish "deployment missing" from "ARM unreachable" without a successful round trip. 4. `classifyModelDeployments` joins `state.ModelRefs` to the deployment set on name. All match → Pass with the matched count. One or more missing → Fail with the missing names listed in the Message and structured under `Details["missingModels"]` (each entry carries both `name` and `service` so the user can locate the offending manifest entry). Suggestion: `azd provision` to create the missing deployments, or update `agent.manifest.yaml` `resources[].name` to match deployments that already exist. # Aggregation The walker may surface `ModelRefs` from multiple services. Every service in an azd project belongs to the same Foundry project (and therefore the same Cognitive Services account), so the check issues exactly one deployments list per run regardless of how many services / model refs exist. The same model referenced by two services collapses to a single match check; a missing model referenced by two services surfaces as two `missingModels` entries (one per service) so the user can pinpoint each affected manifest. # Test seam `Dependencies.probeModelDeployments` (lowercase, package-internal) matches the established pattern from `probeAuth`, `probeFoundryEndpoint`, `probeDeveloperRBAC`, `probeAgentStatus`, `probeAgentPrincipal`. Production wiring leaves it nil; tests inject a closure that returns canned `(names, err)` tuples and (optionally) captures the `(subscription, resourceGroup, accountName)` it was called with. `Dependencies.assembleState` and `Dependencies.readProjectResourceIDFn` are reused from earlier checks; no new top-level seam is added besides the probe. # Files - `internal/cmd/doctor/checks_model_deployments.go` — new check factory `newCheckModelDeployments`, `parseAccountFromProjectID`, `classifyModelDeployments`, `realProbeModelDeployments`, `listDeploymentNames`. 363 lines. - `internal/cmd/doctor/checks_model_deployments_test.go` — 11 tests: skip-cascade (1 + table of 5 upstream-blocked permutations), no manifest models, unset project ID, unparsable project ID, probe transport error, all-match Pass, partial-match Fail, all-missing Fail, parser table (canonical / mixed-case / missing segments / garbage), factory shape pin. - `internal/cmd/doctor/checks_local.go` — adds the `probeModelDeployments` field to `Dependencies` next to its same-shape siblings. - `internal/cmd/doctor/checks_remote.go` — appends `newCheckModelDeployments` after `newCheckAgentIdentityRoles` in `NewRemoteChecks`. - `internal/cmd/doctor/checks_remote_test.go` — extends the composition pin test to assert 6 checks (was 5) including the new `remote.model-deployments` slot. # Preflight - `gofmt -s -w .` clean. - `go vet ./...` clean. - `go build ./...` clean. - Full extension test suite: green (`cmd`, `cmd/doctor`, `cmd/nextstep`, `exterrors`, `agents/agent_api`, `agents/agent_yaml`, `pkg/azure`, `project` — all pass). - `golangci-lint run ./internal/cmd/doctor/...` 0 issues. - `cspell` 0 issues on production file. - No `go.mod` or `go.sum` changes (uses already-imported `armcognitiveservices/v2`). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the eighth local doctor check, `local.toolboxes`, which verifies
that every ToolboxResource declared in any service's
agent.manifest.yaml has its canonical MCP endpoint env var
(TOOLBOX_<NORMALIZED_NAME>_MCP_ENDPOINT) set in the active azd
environment.
Why local (Remote: false). The check only reads the active azd
environment via the existing gRPC env service — no ARM / Foundry
round trips. Tagging it local means `--local-only` still runs it
(which is exactly what we want: a missing TOOLBOX_*_MCP_ENDPOINT is
diagnosable without network access).
Skip cascade. Skips on AzdClient==nil, local.environment-selected,
local.azure-yaml, local.agent-service-detected, and when
state.HasToolboxes is false. Deliberately NOT gated on remote.auth /
remote.foundry-endpoint — a down Foundry must not poison a local env
diagnostic.
Classification.
- All endpoints set → Pass with matchedCount.
- One or more missing → Fail with the missing toolbox names + env
var keys in the Message, Suggestion points at `azd provision`
(the canonical fix) or `azd env set` as the manual override,
Details["missingToolboxes"] carries a structured list (name,
service, envVar) for JSON consumers.
- Env lookup transport error → Fail (NOT Skip). Divergent from
C13's model-deployments which Skips on probe error, because env
lookup is local; a transport failure here means the user's azd
config / extension is broken and a Skip would silently swallow
that signal. Suggestion points at `azd env list` / `azd env
get-values`.
- Empty / whitespace-only value → treated as missing (matches
detectMissingVars semantics in nextstep/state.go).
Convention. TOOLBOX_<NORMALIZED_NAME>_MCP_ENDPOINT, with name
upper-cased and `-` / `.` / ` ` mapped to `_`. Matches the
hosted-toolbox Bicep sample output names. The prefix and suffix are
pinned in code (not derived from the env) so the Fail message can
name the exact env var the user must grep their Bicep template
for.
Dedup. classifyToolboxEndpoints dedupes on the canonical env key
because the C2 manifest walker dedupes on (ServiceName, Name) — the
same toolbox referenced by two services would otherwise produce two
env lookups and two missing-list entries. Exposed
`dedupToolboxKeys` for callers (renderer / future telemetry) that
want the expected-key list up front; the classifier does its own
inline dedup so it does not depend on this helper.
Test seam. `Dependencies.lookupToolboxEnv toolboxEnvLookupFn`
matches the established seam pattern (probeAuth,
probeFoundryEndpoint, probeModelDeployments). Production wiring
leaves it nil; the check binds `makeRealToolboxEnvLookup(deps
.AzdClient)` on first call, which calls
`client.Environment().GetValue` — the canonical one-key env reader
used by service_target_agent.go and checks_rbac.go.
Tests (15). Skip cascade (azdClient nil + 3 priors), not-gated-on-
remote-priors invariant, state emptiness (no toolboxes / nil
state), 3 classifier paths (all-set / partial / all-missing),
whitespace-as-missing, transport-error-is-Fail, cross-service
dedup, normalizeToolboxName table (8 cases), toolboxEndpointKey
roundtrip, dedupToolboxKeys table, factory-shape pin.
Wired into NewLocalChecks (now 8 entries); local-checks pin test
updated. NewRemoteChecks unchanged (still 6 entries).
Preflight clean: gofmt, vet, build, full extension test suite green
(cmd 16.7s, doctor 2.9s, nextstep 6.7s, etc.), golangci-lint 0
issues, cspell 0 issues on production files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Issue: Doctor previously had no way to verify that Foundry connections
referenced by agent manifests (e.g. `bing-grounding`, key-vault-backed
auth connections) actually exist on the project. Failure surfaced
later at invoke time as 401/403 from the upstream tool, with no clear
path back to the missing connection.
Approach: New `remote.connections` doctor check that enumerates manifest
ConnectionResource entries (already discovered by the C2 manifest
walker into `state.Connections`), calls
`FoundryProjectsClient.GetAllConnections(ctx)` on the active project,
and reports any manifest-declared connection that isn't present on the
project as a Fail. Missing-entry rendering format:
`<name> [<detail>] (service <svc>)` when Detail is non-empty, falling
back to `<name> (service <svc>)` to avoid a bare `[]`. Detail
typically renders as `<Category> | <Target>` for connections
(types.go:183-189; manifest.go:152-162).
Classification: REMOTE check (Remote: true). Calls the Foundry API.
Skip cascade mirrors C13 (`remote.model-deployments`):
AzdClient → environment-selected → azure.yaml / agent-service-detected
→ remote.auth → remote.foundry-endpoint → !state.HasConnections
→ unparsable project ID
Probe error → Skip (matching C13's pattern — distinguishing transport
failure from "missing connection" requires a successful round-trip).
10s probe timeout.
Wiring: `newCheckConnections(deps)` added as the 7th and final entry
in `NewRemoteChecks` (after C12 agent-identity-roles, C13 model-
deployments). Pin test `TestNewRemoteChecks_HasAuthFoundryEndpointRBAC
AgentStatusIdentityRolesModelDeployments` renamed to
`...ModelDeploymentsConnections`, Len bumped 6 → 7, 4 new
index-6 assertions for ID / Title / Description / Remote.
Test seam: New `probeFoundryConnections` field appended to
`Dependencies` matching the existing seam pattern from
`probeModelDeployments` (C13). Production wiring uses
`realProbeFoundryConnections` which constructs a credential via
`azidentity.NewAzureDeveloperCLICredential` (matching the rest of
the extension per `agent_context.go:101-109`).
New helper: `parseAccountProjectFromProjectID(projectID) (account,
project, err)` — sibling of C13's `parseAccountFromProjectID`. Returns
two segments instead of the four C13 needs; kept separate to avoid
churning C13's signature for a single new caller. Both case-insensitive
on segment markers. Follow-up: consolidate into a single parser when
a third caller appears.
Tests: `checks_connections_test.go` — 13 tests mirroring C13 patterns:
- Skip cascade table (5 rows: AzdClient, environment, azure.yaml /
agent-service-detected, auth, foundry-endpoint).
- State emptiness (HasConnections false → Skip).
- Project ID unset → Skip.
- Project ID unparsable → Skip.
- Probe error → Skip.
- All-match → Pass.
- Partial mismatch → Fail with missing names + service tags.
- All-missing → Fail.
- Empty-Detail rendering omits `[]`.
- Parser table (5 cases: canonical, mixed case, missing project,
missing account, garbage).
- Factory shape pin (Remote: true, ID, Title, Description).
Preflight:
- gofmt -s -w . (clean)
- go vet ./... (clean)
- go build ./... (clean)
- go test ./... -count=1 (all packages pass; doctor 5.474s)
- golangci-lint run ./internal/cmd/doctor/... (0 issues)
- cspell lint "internal/cmd/doctor/*.go" (14 files, 0 issues)
- copyright header verified on both new files
Files:
- internal/cmd/doctor/checks_connections.go (NEW, +332)
- internal/cmd/doctor/checks_connections_test.go (NEW, +326)
- internal/cmd/doctor/checks_local.go (probeFoundryConnections seam +5)
- internal/cmd/doctor/checks_remote.go (wire +newCheckConnections +1)
- internal/cmd/doctor/checks_remote_test.go (pin test 6 → 7, +14)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two reviewer-consensus findings from the batched code review of commits 385b147 (C13 remote.model-deployments), 87e1dcc (C14 local.toolboxes), and 1b3143f (C15 remote.connections): Fix 1 (MEDIUM, Opus + GPT-5.5): toolbox env-key normalizer divergence. C14's `normalizeToolboxName` only mapped `-`, `.`, and ` ` to `_` rune-by-rune, while the production helpers `init.go:toolboxMCPEndpointEnvKey` (manifest injection) and `listen.go:toolboxMCPEndpointEnvKey` (runtime env write) both use the regex `[^A-Z0-9]+` -> `_` (run-collapsing, all non-alphanumerics). The two algorithms agreed only on the subset of inputs the test table exercised (`web-search-tools`, `my.toolbox.v2`, `my toolbox`, ...) and diverged on inputs like `my--tool`, `my+tool`, `my:tool`, `my(tool)`, `my\ttool`. A user with such a toolbox name would see the doctor flag a missing endpoint under a key (`TOOLBOX_MY__TOOL_MCP_ENDPOINT`, `TOOLBOX_MY+TOOL_MCP_ENDPOINT`, ...) that nothing in the system ever writes. Resolution: hoist the canonical helper into a shared `internal/pkg/envkey` package so the producing and diagnostic sides cannot drift again. - new internal/pkg/envkey/envkey.go -- `ToolboxMCPEndpoint` - new internal/pkg/envkey/envkey_test.go -- 13 cases incl. double-hyphen run, `+`, `:`, `/`, tab, parens, empty - internal/cmd/listen.go -- drop local helper, drop `regexp` import, route through envkey - internal/cmd/init.go -- route through envkey - internal/cmd/init_test.go -- delete duplicated table (covered by envkey package test) - internal/cmd/doctor/checks_toolboxes.go -- drop local normalizeToolboxName / toolboxEndpointKey /toolbox{Prefix, Suffix}, route 2 callsites through envkey - internal/cmd/doctor/checks_toolboxes_test.go -- replace the normalize-table test with a thin pin test verifying the doctor's renderer helper routes through envkey - cspell.yaml -- allowlist `envkey` Fix 2 (MEDIUM, Sonnet): assembler errors silently swallowed. C13/C14/C15 all used `state, _ := assembler(...)` and reported a `Skip` with "no X declared in any service's agent.manifest.yaml" whenever `state == nil || !state.HasX`. The existing pattern at `checks_manual_env.go:95-109` instead captures `errs` and Fails with the actual cause when `state == nil` (defensive against a future contract change where AssembleState may return a nil state with a populated errs slice). Resolution: mirror the established pattern in all three new checks. The Skip for `!state.HasX` is preserved; only the `state == nil` branch becomes a Fail surfacing `errs[0].Error()`. - checks_model_deployments.go -- Fail-on-nil with cause - checks_toolboxes.go -- Fail-on-nil with cause - checks_connections.go -- Fail-on-nil with cause - checks_model_deployments_test.go -- new test: nil state surfaces errs[0] - checks_toolboxes_test.go -- update existing `SkipsWhenAssemblerReturnsNil` to `FailsWhenAssembler ReturnsNilState` plus new test asserting errs[0] surfaces in the Fail message - checks_connections_test.go -- new test: nil state surfaces errs[0] Not addressed (deferred): LOW (GPT-5.5): `parseAccountProjectFromProjectID` (C15) accepts partial paths; `parseAccountFromProjectID` (C13) does not. Opus reviewed and called the dual-parser duplication "defensible for two callers"; commit 1b3143f's message already notes the follow-up to consolidate when a third caller appears. Preflight: - gofmt -s -w . (clean) - go build ./... (clean) - go vet ./... (clean) - go test ./... -count=1 (all packages pass; envkey 1.837s, doctor 6.276s, cmd 14.401s) - golangci-lint run ./... (0 issues) - cspell lint <new+touched> (17 files, 0 issues) Files (10): - internal/pkg/envkey/envkey.go (NEW) - internal/pkg/envkey/envkey_test.go (NEW) - internal/cmd/listen.go (MOD) - internal/cmd/init.go (MOD) - internal/cmd/init_test.go (MOD) - internal/cmd/doctor/checks_toolboxes.go (MOD) - internal/cmd/doctor/checks_toolboxes_test.go (MOD) - internal/cmd/doctor/checks_model_deployments.go (MOD) - internal/cmd/doctor/checks_model_deployments_test.go (MOD) - internal/cmd/doctor/checks_connections.go (MOD) - internal/cmd/doctor/checks_connections_test.go (MOD) - cspell.yaml (MOD) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When Doctor/post-deploy guidance has no cached OpenAPI-derived sample payload but a service README is present, don't suggest a concrete protocol-generic payload that may fail for that sample's schema. Emit the README pointer first, then an invoke command with an explicit '<payload>' placeholder. Cached OpenAPI payloads still produce runnable invoke commands, and services without a README still get the protocol-generic fallback payload with a generic label. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Active/idle show output should stay an inspection view of the hosted agent resource. Remove the active-state invoke suggestion from ResolveAfterShow and avoid attaching next_step in show JSON when the agent is already healthy. Non-active states keep actionable guidance: - creating -> monitor --type system --follow - failed/empty -> monitor --follow - deleting/deleted -> azd deploy - unknown -> azd ai agent show <service> This also avoids state/OpenAPI assembly work for active show output because no active-state guidance is rendered. Validation: - go test ./internal/cmd ./internal/cmd/nextstep -run 'TestResolveAfterShow|TestResolveNextStepFromStatus|TestShowResultJSON|TestPrintAgentVersionJSON|TestPrintAgentVersionTable|TestResolveAfterInvoke_Success|TestResolveAfterInit_UnresolvedPlaceholders|TestResolveAfterRun' -count=1 - go test ./internal/cmd/... -count=1 - go vet ./internal/cmd/... - golangci-lint run ./internal/cmd/... - cspell lint touched show/nextstep files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stream text-mode doctor output by observing each finalized check result, while keeping JSON output buffered and unchanged. Split the text formatter into header/check/footer pieces so the streaming path preserves the existing report shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add project-root validation for service-relative file probes and reads, including symlink-aware containment checks and root-service handling.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the unused nextstep auth probe option and state fields that were added by the PR but never consumed by the resolver or doctor wiring. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Route PR-added nextstep stdout emission through one cmd helper so TTY gating stays consistent across init, invoke, run, show, and doctor text output. This intentionally applies the nextstep call-site TTY contract to the PR-added init next-step blocks as well, keeping redirected output free of human-only guidance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The remote.auth doctor check surfaced the raw user principal name in
both the Message string and the structured Details map regardless of
the --unredacted flag, in contrast with the rest of the doctor checks
(checks_rbac.go, checks_agent_identity_roles.go) which already gate
identity values on opts.Unredacted.
This change threads Options into the auth check function and adds two
small helpers that reuse the existing redactedPlaceholder constant:
- redactUPN(upn, unredacted) returns the value to surface in Message
text: raw when --unredacted, the shared <redacted> placeholder when
a UPN was discovered but should be scrubbed, and empty when none
was found so composeAuthMessage cleanly drops the prefix.
- authDetails(upn, minutes, unredacted) builds the Details map and
omits the "upn" key entirely unless --unredacted is set, so
machine consumers do not see the raw value by default.
PASS, WARN, and expired-FAIL branches now compose their messages from
the redacted display value. Existing tests that asserted the raw UPN
were updated to pass Options{Unredacted: true}; new table tests cover
the default-redacted and --unredacted contracts on every branch, the
empty-UPN drop, and both helpers in isolation.
Resolves PR Azure#8198 review comment from @jongio.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TestErrorCodeWireValues pins the lowercase JSON wire values of every exported enum the nextstep package consumes from the Agents API, but the AgentVersionStatus map was missing the "idle" entry. The idle status is actively read at show.go:207 and nextstep/resolver.go:248, so silent drift on that one literal would regress the show command's idle branch and the resolver's deployment-pending hint without any test failure. Add the missing "idle": string(AgentVersionIdle) case. Resolves PR Azure#8198 Copilot review comments (ids 3246075889, 3246075800). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e output Responds to therealjohn's UX review on PR Azure#8198 by rewriting the doctor text renderer around five contracts: 1. Default = concise. PASS shows just the check name; FAIL shows a one-line Message + one-line `fix:` Suggestion; SKIP inlines the skip reason after `-- skipped`. Use `--debug` to surface the verbose path (full Message + Suggestion + Links). 2. Section grouping. Checks render under Local, Authentication, and Remote headers, derived from the check ID prefix. `remote.auth` gets its own Authentication section per the literal mock. 3. New glyph format: `(✓)`, `(x)`, `(-)`, `(!)`, `(ⓘ)`. ASCII `x` for FAIL matches the mock literally. 4. "To fix" footer on failure. When at least one failure maps to a canonical remediation (`remediationForCheckID`), the footer is a numbered, deduplicated command list in execution order (auth → init → provision → deploy). When all failures are unmapped (or any are unmapped alongside mapped ones), the footer defers to the per-check `fix:` notes rendered in the body. The re-run instruction always closes the block so the user is never left without an actionable next step. 5. First-letter capitalization at render time, with a brand-name blocklist so `azd`, `azure.yaml`, `agent.yaml`, `agent.manifest. yaml`, and `skipped:` leads stay lowercase. Source check strings are untouched. Summary line simplified from `Summary: 1 passed, 1 failed, 1 skipped, 0 warned` to `1 passed, 1 failed, 1 skipped`. Warn/info segments are appended only when non-zero. The streaming render path (`runAndRenderDoctorText` → `renderer.write Check` per result) and the buffered path (`printDoctorReportText`, used by tests) share a single `doctorRenderState` so they produce byte-identical output. The parity test exercises both concise and verbose modes with a fixture covering Message detail, multi-line Suggestion (so `writeIndentedBlock` runs), and Links. The trailing `Next:` block (via `nextstep.PrintAllNext`) is suppressed when the To-fix footer fires, because the failure block is the actionable next step. The `--debug` flag is the existing persistent root flag provided by the azdext SDK; we read it via `isDebug(cmd.Flags())` and thread it through `runAndRenderDoctorText` and `newDoctorRenderer`. No new flag is registered. The JSON output path (`--output json`) does not traverse this renderer and is unchanged. Test additions pin every new contract: concise defaults (including zero-suppression of warn/info), verbose `--debug`, section transitions, streaming/buffered parity in both modes, trailing `Next:`, To-fix footer with mapped failures, To-fix footer with all-unmapped failures (deferred to per-check `fix:` notes), To-fix footer with mixed mapped and unmapped failures (numbered list plus per-check pointer), summary line with non-zero warn/info, empty report, status glyphs, category routing, capitalize edges, and the `firstLine` helper. Three-model code review consensus reached (Opus 4.7 xhigh, Sonnet 4.6, GPT-5.5). All flagged issues addressed. Out of scope (deferred to a follow-up after user input): the trangevi + therealjohn proposal to move doctor into a separate `azure.ai.doctor` extension. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
antriksh30
force-pushed
the
antrikshjain/next-step-implementation
branch
from
804913a to
f29b4e3
Compare
CI surfaced two follow-ups after the rebase that were originally landed
on the branch in the comment-trim cluster (skipped during rebase as
cosmetic):
1. go fix ./... modernizations
- replaced loopvar t := tt captures (Go 1.22+ scoping makes them
unnecessary)
- strings.SplitSeq over comma-separated tag lists in state.go and
pending_provision.go
- max(limit-1, 0) builtin in nextstep/format.go
- strings.Cut in doctor/checks_auth.go::firstLine
2. cspell entries for words introduced by the doctor UX redesign and
next-step package: inlines, Remediations, remediations, uppercases,
parseable, azd's
No behavior change. Build, tests, and lint pass.
therealjohn
approved these changes
May 22, 2026
trangevi
approved these changes
May 22, 2026
Signed-off-by: trangevi <trangevi@microsoft.com>
trangevi
approved these changes
May 22, 2026
therealjohn
approved these changes
May 22, 2026
Member
|
/check-enforcer override |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Next:guidance for the Azure AI Agents extension across init, run, invoke, show, deploy-hook, and doctor flowsazd ai agent doctorwith local and remote checks for project setup, environment variables, authentication, Foundry reachability/RBAC, hosted agent status, identity roles, model deployments, connections, and toolboxesshownext steps, and stream text-mode doctor checks as they complete