docs: add PR review patterns to AGENTS.md

cli/azd/AGENTS.md

@@ -1,6 +1,6 @@
 # Azure Developer CLI (azd) - Agent Instructions
 
-<!-- cspell:ignore Errorf Chdir azapi gofmt golangci stdlib -->
+<!-- cspell:ignore Errorf Chdir azapi gofmt golangci stdlib strconv Readdirnames -->
 
 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 <path>`), 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
+
+- **`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
+- **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: `AZD_NO_PROMPT=true` or `--no-prompt` flag
+  - 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