From 4df1a52d91360edc93aa2b4186beac6ab60b29cb Mon Sep 17 00:00:00 2001 From: Shayne Boyer Date: Tue, 24 Mar 2026 17:56:01 -0700 Subject: [PATCH] docs: add PR feedback patterns to AGENTS.md 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> --- cli/azd/AGENTS.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/cli/azd/AGENTS.md b/cli/azd/AGENTS.md index 0f06c54c783..4b03ec79538 100644 --- a/cli/azd/AGENTS.md +++ b/cli/azd/AGENTS.md @@ -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` - Use structured output for machine consumption +- **Shell-safe output**: When printing shell commands (e.g., `cd `), quote or escape paths that may contain spaces. Use `fmt.Sprintf("cd %q", path)` or conditionally quote +- **Consistent JSON contracts**: When adding new fields to JSON output (e.g., `--output json`), ensure the field type and format are consistent with similar fields across commands. For example, don't mix `*time.Time` and `*RFC3339Time` for timestamp fields in the same API surface + +### Path Safety + +- **Never use user-derived path components without validation**: When deriving directory names from user input or template paths, always validate the result is not `.`, `..`, empty, or contains path separators. These can cause path traversal +- **Quote paths in user-facing messages**: File paths in error messages, suggestions, and follow-up hints should be quoted when they may contain spaces ### Code Organization @@ -165,13 +172,23 @@ func (a *myAction) Run(ctx context.Context) (*actions.ActionResult, error) { - 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` - Handle context cancellations appropriately +- **Telemetry service attribution**: Only set `error.service.name` (e.g., `"aad"`) when an external service actually returned the error. For locally-generated errors (e.g., "not logged in" state checks), don't attribute them to an external service +- **Scope-agnostic messages**: Error messages and suggestions in `error_suggestions.yaml` should be scope-agnostic when the error can occur at multiple scopes (subscription, resource group, etc.). Use "target scope" instead of hardcoding "resource group" ### Documentation Standards - Public functions and types must have Go doc comments - Comments should start with the function/type name - Document non-obvious dependencies or assumptions +- **Help text consistency**: When changing command behavior, update **all** help text that references the old behavior — including `Short`, `Long`, command descriptions used by help generators, and snapshot files. Stale help text is a common review finding +- **Environment variable docs**: When documenting env vars, verify against the actual implementation — check the parsing method (`os.LookupEnv` presence-check vs `strconv.ParseBool`, `time.ParseDuration` vs integer seconds), default values, and which component reads them. Don't assume behavior from the name alone + +### Architecture Boundaries + +- **`pkg/project/` is target-agnostic**: The project manager and service manager should not contain service-target-specific logic (e.g., Container Apps, App Service). Service-target-specific behavior belongs in the target implementations under `pkg/project/` service target files or in the YAML error suggestions pipeline +- **Extension-specific concerns**: Keep extension-specific environment variables and configuration documented in the extension's own docs, not in core azd documentation, unless they are consumed by the core CLI ### Modern Go @@ -196,6 +213,15 @@ This project uses Go 1.26. Use modern standard library features: - Use `t.Chdir(dir)` instead of `os.Chdir` plus a deferred restore in tests - Run `go fix ./...` before committing; CI enforces these modernizations +### Testing Best Practices + +- **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` +- **TypeScript tests**: Use `catch (e: unknown)` with type assertions, not `catch (e: any)`. Set `NO_COLOR=1` in test env to prevent ANSI escape codes from breaking regex assertions +- **Reasonable timeouts**: Set Jest/test timeouts proportional to expected execution time. Don't use 5-minute timeouts for tests that should complete in seconds +- **Efficient directory checks**: To check if a directory is empty, use `os.Open` + `f.Readdirnames(1)` instead of `os.ReadDir` which reads the entire listing into memory + ## MCP Tools Tools follow `server.ServerTool` interface from `github.com/mark3labs/mcp-go/server`: @@ -227,3 +253,13 @@ Feature-specific docs are in `docs/` — refer to them as needed. Some key docs - `docs/extensions/extension-framework.md` - Extension development using gRPC extension framework - `docs/style-guidelines/guiding-principles.md` - Design principles - `docs/tracing-in-azd.md` - Tracing/telemetry guidelines + +## CI / GitHub Actions Patterns + +When creating or modifying GitHub Actions workflows: + +- **Always declare `permissions:`** explicitly with least-privilege (e.g., `contents: read`) +- **Don't overwrite `PATH`** using `${{ env.PATH }}` — it's not defined in GitHub Actions expressions. Use `echo "$DIR" >> $GITHUB_PATH` instead +- **`actions/download-artifact@v4`** without `run-id` only downloads artifacts from the current run, not from other workflows. Cross-workflow artifact sharing requires `run-id` and `repository` parameters +- **Prefer Azure DevOps internal pipelines** for jobs that need secrets or Azure credentials — the team prefers `azdo` over GitHub Actions for authenticated workloads in this public repo +- **No placeholder steps**: Don't add workflow steps that echo "TODO" or are no-ops. If downstream steps depend on generated files, implement the generation or remove the dependency