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 0f06c54c783..73f4217bd09 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 @@ -183,19 +205,72 @@ 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 - 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 +302,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 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