Open
Conversation
Add lessons learned from team and Copilot reviews across PRs #7290, #7251, #7250, #7247, #7236, #7235, #7202, #7039 as agent instructions to prevent recurring review findings. New/expanded sections: - Error handling: ErrorWithSuggestion field completeness, telemetry service attribution, scope-agnostic messages, link/suggestion parity, stale data in polling loops - Architecture boundaries: pkg/project target-agnostic, extension docs separation, env var verification against source code - Output formatting: shell-safe quoted paths, consistent JSON types - Path safety: traversal validation, quoted paths in messages - Code organization: extract shared logic across scopes - Documentation standards: help text consistency, no dead references, PR description accuracy - Testing best practices: test YAML rules e2e, extract shared helpers, correct env vars (AZD_FORCE_TTY, NO_COLOR), TypeScript patterns, reasonable timeouts, cross-platform paths, test new JSON fields - CI / GitHub Actions: permissions blocks, PATH handling, cross-workflow artifacts, prefer ADO for secrets, no placeholder steps Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
spboyer
requested review from
hemarina,
vhvb1989,
wbreza and
weikanglim
as code owners
spboyer
requested review from
JeffreyCA,
rajeshkamal5050 and
tg-msft
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the azd agent instruction guide (AGENTS.md) to capture recurring review findings and preferred patterns, aiming to reduce repeated issues in future PRs.
Changes:
- Adds guidance on output formatting (shell-safe path quoting; JSON type consistency).
- Expands standards for error handling, architecture boundaries, path safety, documentation, testing, and CI/GitHub Actions.
- Extends the testing section with concrete patterns for env vars, timeouts, cross-platform behavior, and helper reuse.
|
|
||
| ### Architecture Boundaries | ||
|
|
||
| - **`pkg/project/` is target-agnostic**: The project manager and service manager should not contain service-target-specific logic (e.g., Container Apps details, Docker checks). Target-specific behavior belongs in target implementations or in the `error_suggestions.yaml` pipeline. The project manager is an interface for service management and should not make assumptions about which target is running |
There was a problem hiding this comment.
The guidance here is overly broad/inaccurate: pkg/project/ contains target-specific implementations (e.g., service_target_containerapp.go, service_target_appservice.go) and service_manager.go includes Container Apps-specific logic. If the intent is to keep only project_manager.go target-agnostic, consider rewording to reference ProjectManager (or project_manager.go) specifically rather than the entire package and service manager.
Suggested change
| - **`pkg/project/` is target-agnostic**: The project manager and service manager should not contain service-target-specific logic (e.g., Container Apps details, Docker checks). Target-specific behavior belongs in target implementations or in the `error_suggestions.yaml` pipeline. The project manager is an interface for service management and should not make assumptions about which target is running | |
| - **`ProjectManager` (`project_manager.go`) is target-agnostic**: The `ProjectManager` interface and its core orchestration logic should not contain service-target-specific details (e.g., Container Apps–specific checks, Docker wiring). Target-specific behavior belongs in target implementations (such as the service target types under `pkg/project/`) or in the `error_suggestions.yaml` pipeline. `ProjectManager` should not make assumptions about which target is running |
| - **Extract shared test helpers**: Don't duplicate test utilities across files. Extract common helpers (e.g., shell wrappers, auth token fetchers, CLI runners) into shared `test_utils` packages. Duplication across 3+ files should always be refactored | ||
| - **Use correct env vars for testing**: | ||
| - Non-interactive mode: `AZD_FORCE_TTY=false` (not `AZD_DEBUG_FORCE_NO_TTY`, which doesn't exist) | ||
| - No-prompt mode: `AZD_NO_PROMPT=true` or `--no-prompt` flag |
There was a problem hiding this comment.
AZD_NO_PROMPT is not used as a no-prompt fallback by the core azd CLI (global flags only support --no-prompt). It is used when propagating no-prompt into extension subprocesses. To avoid misleading guidance for core tests, please adjust this bullet to prefer --no-prompt for azd itself and mention AZD_NO_PROMPT only in the context of extensions/subprocess propagation.
Suggested change
| - No-prompt mode: `AZD_NO_PROMPT=true` or `--no-prompt` flag | |
| - No-prompt mode (core azd): use the `--no-prompt` flag; `AZD_NO_PROMPT=true` is only used to propagate no-prompt into extensions/subprocesses |
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
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
Adds lessons learned from team and Copilot code reviews to
AGENTS.md(agent instructions) to prevent recurring review findings in future PRs.Feedback Sources
Analyzed code-level review comments across 8 recent PRs from team reviewers (
vhvb1989,weikanglim,JeffreyCA,jongio,wbreza,danieljurek) and GitHub Copilot code review:pkg/projectis target-agnostic), link parityNew/Expanded Sections (+48 lines)
ErrorWithSuggestionmust populate all fields; only seterror.service.namefor external service errors; scope-agnostic messages; match links to suggestion text; refresh stale data in polling loopspkg/project/is target-agnostic (no Container Apps logic); extension docs stay in extension dirs; verify env var parsing against source code../.traversal; quote paths in user messagesAZD_FORCE_TTY=falsenotAZD_DEBUG_FORCE_NO_TTY;NO_COLOR=1in tests;catch (e: unknown)notany; reasonable timeouts; cross-platform paths; test new JSON fields; no unused depspermissions:; don't overwrite PATH viaenv.PATH; cross-workflow artifact needsrun-id; prefer ADO for secrets; no placeholder steps