docs: add PR review feedback patterns to AGENTS.md#7300
Closed
Conversation
Add lessons learned from recent PR reviews (#7290, #7251, #7250, #7247, #7236, #7235, #7202, #7039) as agent instructions to prevent recurring review findings. New sections: - Error handling: ErrorWithSuggestion completeness, telemetry service attribution, scope-agnostic messages - Architecture boundaries: pkg/project target-agnostic, extension docs - Output formatting: shell-safe paths, consistent JSON contracts - Path safety: traversal validation, quoted paths in messages - Testing best practices: test actual rules, extract shared helpers, correct env vars, TypeScript patterns, efficient dir checks - CI/GitHub Actions: permissions, PATH handling, artifact downloads, prefer ADO for secrets Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
spboyer
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989 and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates cli/azd/AGENTS.md to capture recurring PR review feedback patterns as concrete guidelines for contributors (agents and humans), aiming to reduce repeated review findings across the azd CLI codebase.
Changes:
- Adds guidance for shell-safe output, JSON contract consistency, and path-safety validation.
- Expands conventions for
ErrorWithSuggestion, telemetry attribution, and scope-agnostic error messaging. - Adds testing best practices and GitHub Actions workflow patterns to prevent common CI/review issues.
|
|
||
| - Wrap errors with `fmt.Errorf("context: %w", err)` to preserve the error chain | ||
| - Consider using `internal.ErrorWithSuggestion` for straightforward, deterministic user-fixable issues | ||
| - When using `ErrorWithSuggestion`, always populate **all** relevant fields (`Err`, `Suggestion`, and `Links` if applicable). Don't leave `Message` or `Links` empty if the YAML pipeline rule would have provided them — `ErrorMiddleware` skips the YAML pipeline for errors that are already `ErrorWithSuggestion` |
There was a problem hiding this comment.
The guidance here lists Err, Suggestion, and Links, but ErrorWithSuggestion also has a Message field (and you later reference Message in the same sentence). To avoid confusion/misleading docs, include Message explicitly in the “populate all fields” list and align the wording with the actual struct fields in pkg/errorhandler/errors.go.
Suggested change
| - When using `ErrorWithSuggestion`, always populate **all** relevant fields (`Err`, `Suggestion`, and `Links` if applicable). Don't leave `Message` or `Links` empty if the YAML pipeline rule would have provided them — `ErrorMiddleware` skips the YAML pipeline for errors that are already `ErrorWithSuggestion` | |
| - When using `ErrorWithSuggestion`, always populate **all** relevant fields (`Err`, `Message`, `Suggestion`, and `Links` when applicable). Don't leave `Message` or `Links` empty if the YAML pipeline rule would have provided them — `ErrorMiddleware` skips the YAML pipeline for errors that are already `ErrorWithSuggestion` |
|
|
||
| - **Test the actual rules, not just the framework**: When adding YAML-based error suggestion rules, write tests that exercise the YAML rules end-to-end through the pipeline, not just tests that validate the framework's generic matching behavior | ||
| - **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` or `test/mocks` packages | ||
| - **Use correct env vars**: For forcing non-interactive mode in tests, use `AZD_FORCE_TTY=false` (not `AZD_DEBUG_FORCE_NO_TTY`). For no-prompt, use `AZD_NO_PROMPT=true` |
There was a problem hiding this comment.
AZD_NO_PROMPT is used for extension processes (it’s propagated by pkg/extensions/runner.go) but the core azd CLI does not appear to read AZD_NO_PROMPT for its own --no-prompt behavior (global flags are parsed via ParseGlobalFlags without an env-var fallback). This bullet may mislead test authors—consider recommending the --no-prompt flag for core CLI tests, and mention AZD_NO_PROMPT specifically in the context of extension subprocesses if needed.
Suggested change
| - **Use correct env vars**: For forcing non-interactive mode in tests, use `AZD_FORCE_TTY=false` (not `AZD_DEBUG_FORCE_NO_TTY`). For no-prompt, use `AZD_NO_PROMPT=true` | |
| - **Use correct env vars and flags**: For forcing non-interactive mode in tests, use `AZD_FORCE_TTY=false` (not `AZD_DEBUG_FORCE_NO_TTY`). For no-prompt behavior in core azd CLI tests, pass `--no-prompt` on the command line; reserve `AZD_NO_PROMPT=true` for extension subprocesses that explicitly honor this env var |
| @@ -154,6 +154,13 @@ func (a *myAction) Run(ctx context.Context) (*actions.ActionResult, error) { | |||
|
|
|||
| - Commands can support multiple output formats via `--output` flag like `json` and`table` | |||
There was a problem hiding this comment.
Minor formatting typo: add a space between and and table so the inline code renders correctly ("json and table").
Suggested change
| - Commands can support multiple output formats via `--output` flag like `json` and`table` | |
| - Commands can support multiple output formats via `--output` flag like `json` and `table` |
Member
Author
|
Superseding with a more complete version. |
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 recent PR reviews to
AGENTS.mdso coding agents (and humans) avoid recurring review findings.Feedback Sources
Analyzed review comments from 8 PRs: #7290, #7251, #7250, #7247, #7236, #7235, #7202, #7039 — from team reviewers (
vhvb1989,weikanglim,JeffreyCA,jongio,wbreza,danieljurek) and Copilot code review.Patterns Added (+36 lines)
ErrorWithSuggestionfields; only seterror.service.namefor actual external service errors; keep error messages scope-agnosticpkg/project/is target-agnostic — no Container Apps logic there; extension-specific docs stay in extension dirs..,.); quote paths in user messagesAZD_FORCE_TTY=falsenotAZD_DEBUG_FORCE_NO_TTY; TypeScript:catch (e: unknown)notany; setNO_COLOR=1; reasonable timeoutspermissions:; don't overwritePATHviaenv.PATH; prefer ADO for secrets; no placeholder steps