From b696e44cb7fdf709a9463c508c7f78eb36e8076a Mon Sep 17 00:00:00 2001 From: Shayne Boyer Date: Tue, 24 Mar 2026 18:07:09 -0700 Subject: [PATCH 1/3] docs: add PR review patterns to AGENTS.md 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> --- cli/azd/AGENTS.md | 49 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/cli/azd/AGENTS.md b/cli/azd/AGENTS.md index 0f06c54c783..b23686d6bb1 100644 --- a/cli/azd/AGENTS.md +++ b/cli/azd/AGENTS.md @@ -1,6 +1,6 @@ # Azure Developer CLI (azd) - Agent Instructions - + Instructions for AI coding agents working with the Azure Developer CLI. @@ -154,24 +154,46 @@ 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 emitting shell commands in user-facing messages (e.g., `cd `), quote paths that may contain spaces. Use `fmt.Sprintf("cd %q", path)` or conditionally wrap in quotes +- **Consistent JSON types**: When adding fields to JSON output (`--output json`), match the types used by similar fields across commands. Don't mix `*time.Time` and custom timestamp wrappers (e.g., `*RFC3339Time`) in the same API surface ### Code Organization - **Import order**: stdlib → external → azure/azd internal → local - **Complex packages**: Consider using `types.go` for shared type definitions (3+ types) - **Context propagation**: Pass `ctx context.Context` as the first parameter to functions that do I/O or may need cancellation +- **Don't duplicate logic across scopes**: When similar logic exists for multiple deployment scopes (e.g., resource group + subscription), extract shared helpers (e.g., `filterActiveDeployments()`) instead of copying code between scope implementations ### Error Handling - Wrap errors with `fmt.Errorf("context: %w", err)` to preserve the error chain - Consider using `internal.ErrorWithSuggestion` for straightforward, deterministic user-fixable issues - Handle context cancellations appropriately +- **`ErrorWithSuggestion` completeness**: When returning `ErrorWithSuggestion`, populate **all** relevant fields (`Err`, `Suggestion`, `Message`, `Links`). `ErrorMiddleware` skips the YAML error-suggestion pipeline for errors already typed as `ErrorWithSuggestion`, so leaving fields empty means the user misses guidance that the YAML rule would have provided +- **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 to an external service — use a local classification instead +- **Scope-agnostic error messages**: Error messages and suggestions in `error_suggestions.yaml` should work across all deployment scopes (subscription, resource group, etc.). Use "target scope" or "deployment scope" instead of hardcoding "resource group" +- **Match links to suggestion text**: If a suggestion mentions multiple tools (e.g., "Docker or Podman"), the `links:` list must include URLs for all of them. Don't mention options you don't link to +- **Stale data in polling loops**: When polling for state changes (e.g., waiting for active deployments), refresh display data (names, counts) from each poll result rather than capturing it once at the start + +### Architecture Boundaries + +- **`ProjectManager` is target-agnostic**: `project_manager.go` should not contain service-target-specific logic (e.g., Container Apps details, Docker checks). Target-specific behavior belongs in the target implementations (e.g., `service_target_containerapp.go`) 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 +- **Extension-specific documentation**: Keep extension-specific environment variables and configuration documented in the extension's own docs, not in core azd reference docs, unless they are consumed by the core CLI itself +- **Verify env vars against source**: When documenting environment variables, verify the actual parsing method in code — `os.LookupEnv` (presence-only) vs `strconv.ParseBool` (true/false) vs `time.ParseDuration` vs integer seconds. Document the expected format and default value accurately + +### Path Safety + +- **Validate derived paths**: 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 outside the working directory +- **Quote paths in user-facing output**: Shell commands in suggestions, follow-up messages, and error hints should quote file paths that may contain spaces ### 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** related help text — `Short`, `Long`, custom description functions used by help generators, and usage snapshot files. Stale help text that contradicts the actual behavior is a common review finding +- **No dead references**: Don't reference files, scripts, directories, or workflows that don't exist in the PR. If a README lists `scripts/generate-report.ts`, it must exist. If a CI table lists `eval-human.yml`, it must be included +- **PR description accuracy**: Keep the PR description in sync with the actual implementation. If the description says "server-side filtering" but the code does client-side filtering, update the description ### Modern Go @@ -196,6 +218,21 @@ 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 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` 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: use the `--no-prompt` flag for core azd commands; `AZD_NO_PROMPT=true` is only used for propagating no-prompt into extension subprocesses + - Suppress color: `NO_COLOR=1` — always set in test environments to prevent ANSI escape codes from breaking assertions +- **TypeScript test patterns**: Use `catch (e: unknown)` with type assertions, not `catch (e: any)` which bypasses strict mode +- **Reasonable timeouts**: Set test timeouts proportional to expected execution time. Don't use 5-minute timeouts for tests that shell out to `azd --help` (which completes 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 +- **Cross-platform paths**: When resolving binary paths in tests, handle `.exe` suffix on Windows (e.g., `azd` vs `azd.exe` via `process.platform === "win32"`) +- **Test new JSON fields**: When adding fields to JSON command output (e.g., `expiresOn` in `azd auth status --output json`), add a test asserting the field's presence and format +- **No unused dependencies**: Don't add npm/pip packages that aren't imported anywhere. Remove dead `devDependencies` before submitting + ## MCP Tools Tools follow `server.ServerTool` interface from `github.com/mark3labs/mcp-go/server`: @@ -227,3 +264,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 + +When creating or modifying GitHub Actions workflows: + +- **Always declare `permissions:`** explicitly with least-privilege (e.g., `contents: read`). All workflows in the repo should have this block for consistency +- **Don't overwrite `PATH`** using `${{ env.PATH }}` — it's not defined in GitHub Actions expressions and will wipe the real PATH. Use `echo "$DIR" >> $GITHUB_PATH` instead +- **Cross-workflow artifacts**: `actions/download-artifact@v4` without `run-id` only downloads artifacts from the *current* workflow run. Cross-workflow artifact sharing requires `run-id` and `repository` parameters +- **Prefer Azure DevOps pipelines** for jobs that need secrets or Azure credentials — the team uses internal ADO pipelines for authenticated workloads in this public repo +- **No placeholder steps**: Don't add workflow steps that echo "TODO" or list directories without producing output. If downstream steps depend on generated files, implement the generation or remove the dependency From c1e215eabb0c63afd50e0d7a2f749798b986be06 Mon Sep 17 00:00:00 2001 From: JeffreyCA Date: Wed, 25 Mar 2026 13:28:53 -0700 Subject: [PATCH 2/3] Fix Copilot instructions for code review and strengthen guidance on Go patterns (#7320) * Fix Copilot instructions for code review and strengthen guidance on Go patterns * Update wording --- .github/copilot-instructions.md | 3 +++ cli/azd/AGENTS.md | 42 +++++++++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000000..8c644c0e49a --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,3 @@ +# Azure Developer CLI (azd) - Instructions for Copilot Chat and Copilot code review + +For any work in this repository, especially for code reviews, you MUST read [cli/azd/AGENTS.md](../cli/azd/AGENTS.md) in its entirety (every line) first. diff --git a/cli/azd/AGENTS.md b/cli/azd/AGENTS.md index b23686d6bb1..73f4217bd09 100644 --- a/cli/azd/AGENTS.md +++ b/cli/azd/AGENTS.md @@ -205,13 +205,51 @@ This project uses Go 1.26. Use modern standard library features: - **Range over integers**: `for i := range 10 { }` ### Modern Go Patterns (Go 1.26+) +- Use `new(val)` not `x := val; &x` - returns pointer to any value. + Go 1.26 extends `new()` to accept expressions, not just types. + Type is inferred: `new(0) → *int`, `new("s") → *string`, `new(T{}) → *T`. + DO NOT use `x := val; &x` pattern — always use `new(val)` directly. + DO NOT use redundant casts like `new(int(0))` — just write `new(0)`. + Before: + ```go + timeout := 30 + debug := true + cfg := Config{ + Timeout: &timeout, + Debug: &debug, + } + ``` + + After: + ```go + cfg := Config{ + Timeout: new(30), // *int + Debug: new(true), // *bool + } + ``` + +- Use `errors.AsType[T](err)` not `errors.As(err, &target)`. + ALWAYS use errors.AsType when checking if error matches a specific type. + + Before: + ```go + var pathErr *os.PathError + if errors.As(err, &pathErr) { + handle(pathErr) + } + ``` + + After: + ```go + if pathErr, ok := errors.AsType[*os.PathError](err); ok { + handle(pathErr) + } + ``` -- Use `errors.AsType[*MyError](err)` instead of `var e *MyError; errors.As(err, &e)` - Use `slices.SortFunc(items, func(a, b T) int { return cmp.Compare(a.Name, b.Name) })` instead of `sort.Slice` - Use `slices.Clone(s)` instead of `append([]T{}, s...)` - Use `slices.Sorted(maps.Keys(m))` instead of collecting keys and sorting them separately - Use `http.NewRequestWithContext(ctx, method, url, body)` instead of `http.NewRequest(...)` -- Use `new(expr)` instead of `to.Ptr(expr)`; `go fix ./...` applies this automatically - Use `wg.Go(func() { ... })` instead of `wg.Add(1); go func() { defer wg.Done(); ... }()` - Use `for i := range n` instead of `for i := 0; i < n; i++` for simple counted loops - Use `t.Context()` instead of `context.Background()` in tests From 0751293558c8a0663fde455646c5679d8227d975 Mon Sep 17 00:00:00 2001 From: Jeffrey Chen Date: Thu, 26 Mar 2026 00:39:40 +0000 Subject: [PATCH 3/3] docs: exclude AGENTS.md from PR validation paths --- eng/pipelines/release-cli.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/eng/pipelines/release-cli.yml b/eng/pipelines/release-cli.yml index 2f5ac96c1a0..fd0ad00ef4c 100644 --- a/eng/pipelines/release-cli.yml +++ b/eng/pipelines/release-cli.yml @@ -39,6 +39,7 @@ pr: - cli/azd/extensions/** - cli/azd/.vscode/cspell.yaml - cli/azd/.vscode/cspell-azd-dictionary.txt + - cli/azd/AGENTS.md extends: template: /eng/pipelines/templates/stages/1es-redirect.yml