feat: unify agent environment diagnostics - #9531
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Unifies agent environment diagnostics across unified, deprecated, referenced, and legacy configuration shapes.
Changes:
- Loads effective environment values using the intended precedence.
- Reuses them for missing-variable and placeholder diagnostics.
- Reports environment-loading failures through Doctor.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
nextstep/types.go |
Extends diagnostic state with environment data and errors. |
nextstep/state.go |
Collects effective values and detects missing references. |
nextstep/state_test.go |
Tests unified and referenced configurations. |
nextstep/resolver.go |
Generalizes remediation wording. |
nextstep/resolver_test.go |
Updates resolver expectations. |
nextstep/format_test.go |
Updates formatted-output expectations. |
nextstep/environment.go |
Implements effective environment loading. |
nextstep/environment_test.go |
Tests precedence, fallbacks, and errors. |
doctor/checks_manual_env.go |
Reports environment-loading failures. |
doctor/checks_manual_env_test.go |
Tests Doctor failure reporting. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go:1106
- [azd-code-reviewer]
environmentValuesFromManifestsilently converts malformed YAML into an empty environment. Cases that expect no refs/placeholders would then pass for the wrong reason; fail the test immediately on fixture parse errors.
var hosted agent_yaml.ContainerAgent
if err := yaml.Unmarshal([]byte(data), &hosted); err != nil {
return nil
}
cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_manual_env.go:124
- [azd-code-reviewer] This loader also supports
azure.yml, so directing every affected user to fixazure.yamlcan point them at a file that does not exist. Refer to the project configuration file generically (or preserve its actual path) in the remediation.
Suggestion: "Fix azure.yaml, its referenced file, or the legacy agent file, then re-run `azd ai agent doctor`.",
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go:1285
- Use
t.Context()here so cancellation follows the test lifecycle, as required by the repository's Go test pattern.
state, errs := assembleState(context.Background(), src)
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go:1318
- Use
t.Context()here so cancellation follows the test lifecycle, as required by the repository's Go test pattern.
state, errs := assembleState(context.Background(), src)
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go:1072
- The leading indentation makes this YAML fixture malformed, and
environmentValuesFromManifestsilently converts that parse failure to an empty input. The test therefore passes without exercising either Foundry expression. Remove the extra indentation so this case validates the placeholder filtering behavior.
environment_variables:
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state_test.go:1245
- Use
t.Context()here so cancellation follows the test lifecycle, as required by the repository's Go test pattern.
This issue also appears in the following locations of the same file:
- line 1285
- line 1318
state, errs := assembleState(context.Background(), src)
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/environment.go:82
LoadServiceEnvironmentresolves the raw service entry without the runtime loader'svalidateRootRefCoreFieldscheck (internal/project/agent_definition.go:584-613). Consequently, anenvblock supplied by a root$refis treated as effective here and can generate missing-variable guidance, whilerunanddeployreject that same configuration. Route diagnostics through shared resolution/validation semantics (or extract them into a lower-level package) before merging the environment maps.
serviceEnvironment, err := projectconfig.LoadServiceEnvironment(
projectRoot,
svc.GetName(),
)
if err != nil {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/resolver.go:164
- azd-code-reviewer: Environment load failures are not treated as blockers in this resolver.
collectServicesnow recordsEnvironmentLoadErrorswhile leaving that service's values empty, so a malformedenvor$refcan make this function reach the default branch and recommendazd ai agent run. Add source-neutral repair guidance and prevent runnable follow-ups until these errors are fixed.
// Placeholder fix-ups come first when present: they are
// broken state in the agent configuration and block both `run` and
// `deploy`. The user has to edit the agent configuration (or define a
// matching parameter in agent.manifest.yaml) — `azd env set` cannot
// reach them.
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go:735
- azd-code-reviewer: This special case breaks the documented lockstep with
agent_yaml.ExtractUnresolvedPlaceholders. The init warning still treats${{project.endpoint}}and$${{project.name}}as unresolved and tells users to replace them, while Next now suppresses them. Move Foundry-expression filtering into the shared placeholder extractor so both diagnostics agree.
for _, m := range placeholderPattern.FindAllStringSubmatchIndex(value, -1) {
if len(m) < 4 || (m[0] > 0 && value[m[0]-1] == '$') {
continue
|
Copilot resolve the merge conflicts in this pull request |
…ostics # Conflicts: # cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go Co-authored-by: trangevi <26490000+trangevi@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go:673
- [azd-code-reviewer] This now treats every
${NAME}found in the merged values as an azd-environment dependency, but service-levelenvkeys can satisfy aliases directly. For example, inlineTARGET: ${SERVICE_ENDPOINT}plus serviceenv.SERVICE_ENDPOINTworks throughResolveAgentEnvironmentVariable(internal/project/agent_definition.go:274-277), while this path askssrc.EnvValueforSERVICE_ENDPOINTand incorrectly reports it missing. Preserve the service-level environment keys per service and exclude references satisfied by those keys before classifying missing variables.
refs, phs := extractEnvironmentRefs(svc.EnvironmentValues)
cli/azd/extensions/azure.ai.agents/internal/cmd/nextstep/state.go:753
- [azd-code-reviewer] Filtering Foundry expressions only here breaks the package's explicit lockstep invariant:
agent_yaml.ExtractUnresolvedPlaceholdersstill appliesPlaceholderPatternwithout this exclusion (internal/pkg/agents/agent_yaml/placeholders.go:45), andparameters.go:81-84requires its warning and Next guidance to agree. A valid${{project.endpoint}}can therefore still trigger the init warning while disappearing from Next guidance. Move the Foundry-aware filtering into the shared placeholder extractor and use it at both call sites.
for _, m := range placeholderPattern.FindAllStringSubmatchIndex(value, -1) {
if len(m) < 4 || (m[0] > 0 && value[m[0]-1] == '$') {
continue
cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_manual_env.go:125
- [azd-code-reviewer] This remediation names
azure.yaml, but the loader also supportsazure.yml; users of that supported filename are directed to a file that may not exist. Keep the suggestion source-neutral, consistent with the new diagnostic.
Suggestion: "Fix azure.yaml, its referenced file, or the legacy agent file, " +
"then re-run `azd ai agent doctor`.",
|
/check-enforcer override |
Why this is needed
Issue #8710 still had inconsistent environment diagnostics: next-step guidance and
azd ai agent doctorread legacyagent.yamldirectly, while current projects can define the same values inline inazure.yamlor under deprecatedconfig:. Users could therefore receive different or incomplete remediation guidance depending on the configuration shape.What this PR does
env:, inline agent properties, deprecatedconfig:, and legacyagent.yaml/agent.ymlfallback.E2E validation
azd ai agent doctor --local-onlyazure.yamlenvironment values.azd ai agent doctor --local-only$refenvironment source.azd ai agent doctor --local-onlyconfig:environment values.azd ai agent doctor --local-onlyagent.yamlenvironment values.azd ai agent init --no-prompt -m agent.manifest.yamlThe negative diagnostic scenarios also produced actionable
azd env setguidance for missing variables and source-neutral configuration repair guidance for a malformed environment reference.azd provision --no-promptazd deploy --no-promptazd ai agent run