From fd45741071db94e635d1d375633567b06651009c Mon Sep 17 00:00:00 2001 From: sswaminathan Date: Thu, 1 Oct 2026 18:06:51 -0700 Subject: [PATCH 1/3] docs: add command authoring guidelines Add reuse, correctness, and user-facing text conventions to AGENTS.md, with a longer checklist in docs/command-authoring.md linked from the maintainers guide. Co-Authored-By: Claude Sonnet 5.5 --- AGENTS.md | 25 ++++++++++++++++++ docs/command-authoring.md | 55 +++++++++++++++++++++++++++++++++++++++ docs/maintainers.md | 1 + 3 files changed, 81 insertions(+) create mode 100644 docs/command-authoring.md diff --git a/AGENTS.md b/AGENTS.md index 0180b5ec..9ebfd524 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -60,6 +60,31 @@ Commands that charge money or request user consent should follow the examples of - Return `Ok(CommandResult::new(json!({...})))` for success. - Prefer `crate::error::GddyError::{not_found,validation,auth,config,security,network,…}` (and `GddyError::from` for module client errors) so agents get stable `error.code` + top-level `fix`. Use `Err(cli_engine::CliCoreError::message("..."))` only for one-off cases that do not yet have a shared mapping. - Streaming commands use `RuntimeCommandSpec::new_streaming` and emit events via `StreamSender`. +- Dry-run paths return `CommandResult::with_dry_run()`. +- Next actions (suggested follow-up commands) use a command template plus structured params, not a `format!`-built command line like `--query '{query}'` (a quote in the value breaks it; metacharacters can inject commands). Param names must match the target command's args and the template's ``s. +- Encode dynamic URL path segments with `api::http::encode_path_segment`. +- Do not call `--debug transport` logging helpers for payloads that may hold customer, payment or order data. +- Polling/retry wrappers map only the exhausted expected status (e.g. 404) to `not_found`; keep 429/5xx/network errors as-is. +- Resolve the API base URL from the selected environment; do not add per-service URL overrides or `--env` flags on follow-up commands. + +## Reuse Before You Build (Required) + +Search the codebase and `cli-engine` before writing a helper; reviewers reject duplication. + +- Typed clients: generate with Progenitor from the OpenAPI spec (as existing generated clients in the workspace do). Do not hand-write `reqwest` clients that traverse `serde_json::Value`. +- Rendering: prefer `cli-engine` rendering (`HumanViewDef`/`TableColumn`, structured next actions and its standard footer) over hand-formatted tables or local display logic. Only write custom rendering when `cli-engine` cannot express it. +- Shared formatting (money, etc.): reuse existing helpers rather than adding per-module copies. + +## User-Facing Text (Required) + +- Write help, guides and output for customers: no internal system or API names, scopes, environments or implementation jargon. +- Where an AI assistant must act differently from a human (e.g. consent before a charge), address it directly in a clearly marked `AI assistants:` note; never mix that into customer-facing prose. +- Command descriptions are short imperatives from the user's point of view; give flags concrete examples and discoverable values; show where prerequisite values (IDs) come from. +- Don't expose internals (retry mechanics, generated keys, etc.) in normal help or output; when something fails, put the suggested next step in the error `fix`. +- Avoid raw JSON inputs in the main flow. Guides should use soft line breaks (hard breaks only in shell examples). +- PR descriptions must match implemented behavior. + +Full checklist: [Command authoring](./docs/command-authoring.md). ## Code File Structure (Required) diff --git a/docs/command-authoring.md b/docs/command-authoring.md new file mode 100644 index 00000000..bae42051 --- /dev/null +++ b/docs/command-authoring.md @@ -0,0 +1,55 @@ +# Command authoring checklist + +Read this before adding a command group. The short, enforceable version lives in [`AGENTS.md`](../AGENTS.md); this doc adds the reasoning. + +## 1. Reuse before you build + +Search the codebase and `cli-engine` before writing a helper. Duplication is the most common review finding. + +| Need | Use | Not | +| --- | --- | --- | +| Typed API client | Progenitor-generated client from the OpenAPI spec (as existing generated clients in the workspace do) | A hand-written `reqwest` client traversing `serde_json::Value` | +| Human output (tables, lists, footers) | `cli-engine` rendering: `HumanViewDef` / `TableColumn` | Hand-formatted strings | +| Follow-up suggestions | `cli-engine` structured next actions and its standard footer | Project-local display or placeholder-substitution code | +| Shared formatting (currency, etc.) | The existing shared helper | A per-module copy | +| URL path segments | `api::http::encode_path_segment` | String interpolation into paths | + +Prefer `cli-engine` rendering whenever it can express the output. Custom rendering should be the exception, with a reason stated in the PR. If duplication is truly unavoidable, say why. + +## 2. Correctness and security + +- **Encode dynamic path segments.** Caller-supplied IDs containing `/`, `?`, `#` or `%` must not change URL structure. Cover those characters in a test. +- **Build next actions from a command template plus structured params, never by formatting a command line.** Don't write `format!("... --query '{query}'")`: +a quote in the value breaks the command, and shell metacharacters can inject another one. Instead, declare the command and pass each value +(e.g. a search query or pagination cursor) as a named param; the consumer fills them in and handles quoting. Param names must match the target command's args, +`` names in the template must match those params, and don't declare params the target command doesn't accept. +- **Mark dry runs.** Every dry-run path returns `CommandResult::with_dry_run()` so envelope and audit consumers can tell a preview from an executed mutation. +- **Don't log sensitive payloads.** Avoid the `--debug transport` logging helpers for requests or responses that may contain customer, payment or order data. +- **Map only the error you mean to map.** When polling or retrying for eventual consistency, only the exhausted expected status (e.g. 404) becomes `not_found`. +Network errors, 429s and 5xxs keep their real error mapping. +- **Use the selected environment.** Don't add private per-service URL overrides or `--env` flags on follow-up commands. + +## 3. Write for the customer + +Users don't know our system names, API names or environments. + +- Help text, guides and output must not leak internal terms (service names, scopes, environment plumbing, implementation jargon). +- Command descriptions are short imperatives from the user's point of view. +- Text may address AI assistants directly when they must behave differently from a human (e.g. obtaining explicit user consent before a charge). +Put it in a clearly marked `AI assistants:` note, and keep it separate from the customer-facing prose. +- Prefer common terms users already know. If an API resource name differs, define it once in the guide. +- Every flag gets concrete examples and discoverable values. Don't ask for things the system can infer, and don't assume users know standards by name. +- If a command needs a value produced by an earlier command (an ID, a selection), that command's output and the guide must show where to get it. +- Don't over-communicate internals (retry mechanics, generated keys, etc.) in normal help or output. When something fails, put the suggested next step in the error's `fix` text. +- Avoid raw JSON inputs (`--body`, `--file`) in the main flow. Prefer simple flags. If JSON is unavoidable, point agents at where to get the schema instead of embedding samples. + +## 4. Guides (`guides/*.md`) + +- Structure: introduction (what you'll learn), concepts (what the CLI exposes), then task-oriented command sequences with any non-obvious step explained. +- Use soft line breaks in prose. The renderer wraps to the terminal width, and hard breaks look wrong in narrow terminals. Hard breaks are fine inside shell examples. +- Describe what customers can do, not which APIs are being called. + +## 5. Pull requests + +- Make sure the PR description matches the implemented behavior. +- Reply on each review thread with the fixing commit. Leave human reviewers' threads open for them to resolve. diff --git a/docs/maintainers.md b/docs/maintainers.md index 5873e728..3c2795db 100644 --- a/docs/maintainers.md +++ b/docs/maintainers.md @@ -8,6 +8,7 @@ If you are a new developer on this project, read the following to get started: - [`cli-engine` concepts](https://github.com/godaddy/cli-engine/blob/main/cli-engine/docs/concepts.md) goes over the components making up GoDaddy CLIs. - [This repo's docs](../docs/) +- [Command authoring checklist](./command-authoring.md) covers conventions for adding commands and command groups. - [`CONTRIBUTING.md`](../CONTRIBUTING.md) gives some general tips for setting up your workspace for development. If you are doing agentic coding and see your agent struggling with following standards correctly, contributions to [`AGENTS.md`](../AGENTS.md) are greatly appreciated. From 0dce73d14eea619f508169953397b2e0523725c8 Mon Sep 17 00:00:00 2001 From: sswaminathan Date: Thu, 1 Oct 2026 18:12:40 -0700 Subject: [PATCH 2/3] docs: scope path-encoding rule and add further command conventions Address review feedback (generated clients already encode path params; checklist applies to standalone commands too) and add conventions for dry-run, next actions, input validation, API result handling, and guides. Co-Authored-By: Claude Sonnet 5.5 --- AGENTS.md | 9 ++++++--- docs/command-authoring.md | 18 +++++++++++++----- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9ebfd524..b5d0ecbc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -60,10 +60,13 @@ Commands that charge money or request user consent should follow the examples of - Return `Ok(CommandResult::new(json!({...})))` for success. - Prefer `crate::error::GddyError::{not_found,validation,auth,config,security,network,…}` (and `GddyError::from` for module client errors) so agents get stable `error.code` + top-level `fix`. Use `Err(cli_engine::CliCoreError::message("..."))` only for one-off cases that do not yet have a shared mapping. - Streaming commands use `RuntimeCommandSpec::new_streaming` and emit events via `StreamSender`. -- Dry-run paths return `CommandResult::with_dry_run()`. -- Next actions (suggested follow-up commands) use a command template plus structured params, not a `format!`-built command line like `--query '{query}'` (a quote in the value breaks it; metacharacters can inject commands). Param names must match the target command's args and the template's ``s. -- Encode dynamic URL path segments with `api::http::encode_path_segment`. +- Commands with external effects are marked mutating so the engine's dry-run safeguard applies. Dry-run paths validate and read every prerequisite the real call needs (so they fail where it would) and return `CommandResult::with_dry_run()`. +- Next actions (suggested follow-up commands) use a command template plus structured params, not a `format!`-built command line like `--query '{query}'` (a quote in the value breaks it; metacharacters can inject commands). Param names must match the target command's args and the template's ``s. Emit next actions only when executable and appropriate to the returned state, and never include consent-bypass flags (e.g. `--agree`) in them. +- Encode dynamic path segments with `api::http::encode_path_segment` when assembling URLs by hand. Generated (Progenitor) clients already percent-encode path parameters; pass them raw to avoid double encoding. - Do not call `--debug transport` logging helpers for payloads that may hold customer, payment or order data. +- Constrain flags to the API's documented values at argument parsing, so bad input fails locally with clear help. +- Correctable input or config failures use a stable validation error with an actionable `fix`; never turn malformed config into an empty payload. +- Surface in-band API errors as errors and preserve empty acknowledgements as-is; do not substitute default "success" objects or cache errors as empty data. - Polling/retry wrappers map only the exhausted expected status (e.g. 404) to `not_found`; keep 429/5xx/network errors as-is. - Resolve the API base URL from the selected environment; do not add per-service URL overrides or `--env` flags on follow-up commands. diff --git a/docs/command-authoring.md b/docs/command-authoring.md index bae42051..b961e992 100644 --- a/docs/command-authoring.md +++ b/docs/command-authoring.md @@ -1,6 +1,6 @@ # Command authoring checklist -Read this before adding a command group. The short, enforceable version lives in [`AGENTS.md`](../AGENTS.md); this doc adds the reasoning. +Read this before adding a command or command group. The short, enforceable version lives in [`AGENTS.md`](../AGENTS.md); this doc adds the reasoning. ## 1. Reuse before you build @@ -12,18 +12,25 @@ Search the codebase and `cli-engine` before writing a helper. Duplication is the | Human output (tables, lists, footers) | `cli-engine` rendering: `HumanViewDef` / `TableColumn` | Hand-formatted strings | | Follow-up suggestions | `cli-engine` structured next actions and its standard footer | Project-local display or placeholder-substitution code | | Shared formatting (currency, etc.) | The existing shared helper | A per-module copy | -| URL path segments | `api::http::encode_path_segment` | String interpolation into paths | +| URL path segments (hand-built URLs) | `api::http::encode_path_segment` | String interpolation into paths | Prefer `cli-engine` rendering whenever it can express the output. Custom rendering should be the exception, with a reason stated in the PR. If duplication is truly unavoidable, say why. ## 2. Correctness and security -- **Encode dynamic path segments.** Caller-supplied IDs containing `/`, `?`, `#` or `%` must not change URL structure. Cover those characters in a test. +- **Encode dynamic path segments.** Caller-supplied IDs containing `/`, `?`, `#` or `%` must not change URL structure. Encode them with `encode_path_segment` when assembling URLs by hand, and cover those characters in a test. Generated (Progenitor) clients already percent-encode path parameters, so pass raw values to their setters; pre-encoding would double-encode `%`. - **Build next actions from a command template plus structured params, never by formatting a command line.** Don't write `format!("... --query '{query}'")`: a quote in the value breaks the command, and shell metacharacters can inject another one. Instead, declare the command and pass each value (e.g. a search query or pagination cursor) as a named param; the consumer fills them in and handles quoting. Param names must match the target command's args, `` names in the template must match those params, and don't declare params the target command doesn't accept. -- **Mark dry runs.** Every dry-run path returns `CommandResult::with_dry_run()` so envelope and audit consumers can tell a preview from an executed mutation. +- **Mark dry runs.** Every dry-run path returns `CommandResult::with_dry_run()` so envelope and audit consumers can tell a preview from an executed mutation. Mark every command with external effects as mutating so the engine's dry-run safeguard prevents unintended calls, and have dry-run validate and read every prerequisite the real call needs, so it fails wherever the real call would rather than reporting an impossible success. +- **Keep next actions honest.** Emit them only when they are executable and appropriate to the returned state. Never include consent-bypass flags (e.g. `--agree`) in suggested commands, and re-present approval guidance after material changes so stale approval isn't reused. +- **Constrain inputs at parse time.** Restrict flags to the API's documented values in argument parsing so invalid input fails locally with clear help, not after a network request. +- **Fail with actionable validation errors.** Every user-correctable input or manifest failure gets a stable validation error with a `fix`. Never silently turn malformed configuration into an empty payload. +- **Don't mask API results.** Surface in-band API errors as errors. Preserve empty acknowledgements explicitly; don't substitute default "success" objects or cache errors as empty data. +- **Honor protocol semantics.** Handle case-insensitive headers, relative pagination links and query-bearing paths, while preserving the original request value. +- **Report ambiguity.** When several catalog entries match, return an explicit ambiguity result rather than silently picking the first. +- **Keep output consistent.** Output schemas, default-field projections and every execution mode must agree; document mode-only fields as optional, since default rendering can otherwise discard the useful result. Test the rendered, default-projected output, and state each test's real scope. - **Don't log sensitive payloads.** Avoid the `--debug transport` logging helpers for requests or responses that may contain customer, payment or order data. - **Map only the error you mean to map.** When polling or retrying for eventual consistency, only the exhausted expected status (e.g. 404) becomes `not_found`. Network errors, 429s and 5xxs keep their real error mapping. @@ -45,7 +52,8 @@ Put it in a clearly marked `AI assistants:` note, and keep it separate from the ## 4. Guides (`guides/*.md`) -- Structure: introduction (what you'll learn), concepts (what the CLI exposes), then task-oriented command sequences with any non-obvious step explained. +- Structure: introduction (what you'll learn), concepts (what the CLI exposes, including entity relationships, opaque identifiers and consistent terminology), then task-oriented command sequences with any non-obvious step explained. +- Update discovery metadata and any related proposal docs when command availability changes, so agent matching and documentation don't contradict shipped behavior. - Use soft line breaks in prose. The renderer wraps to the terminal width, and hard breaks look wrong in narrow terminals. Hard breaks are fine inside shell examples. - Describe what customers can do, not which APIs are being called. From 6df167adfa4d4a854f88a1c7364533ddee3e5532 Mon Sep 17 00:00:00 2001 From: sswaminathan Date: Thu, 1 Oct 2026 18:23:23 -0700 Subject: [PATCH 3/3] docs: clarify and tighten command authoring guidance Expand the unclear conventions with concrete explanations, join hard-wrapped lines, merge overlapping bullets, and drop the ambiguity rule. Co-Authored-By: Claude Sonnet 5.5 --- AGENTS.md | 2 +- docs/command-authoring.md | 32 +++++++++++++------------------- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b5d0ecbc..a178916a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,7 +66,7 @@ Commands that charge money or request user consent should follow the examples of - Do not call `--debug transport` logging helpers for payloads that may hold customer, payment or order data. - Constrain flags to the API's documented values at argument parsing, so bad input fails locally with clear help. - Correctable input or config failures use a stable validation error with an actionable `fix`; never turn malformed config into an empty payload. -- Surface in-band API errors as errors and preserve empty acknowledgements as-is; do not substitute default "success" objects or cache errors as empty data. +- If an API returns an error payload inside a 2xx response, treat it as an error: don't let a typed client turn it into an empty success, and don't cache it. Keep an empty 202/204 response distinct (null/none) rather than substituting a default object that looks like a real, empty resource. - Polling/retry wrappers map only the exhausted expected status (e.g. 404) to `not_found`; keep 429/5xx/network errors as-is. - Resolve the API base URL from the selected environment; do not add per-service URL overrides or `--env` flags on follow-up commands. diff --git a/docs/command-authoring.md b/docs/command-authoring.md index b961e992..579db008 100644 --- a/docs/command-authoring.md +++ b/docs/command-authoring.md @@ -18,23 +18,18 @@ Prefer `cli-engine` rendering whenever it can express the output. Custom renderi ## 2. Correctness and security -- **Encode dynamic path segments.** Caller-supplied IDs containing `/`, `?`, `#` or `%` must not change URL structure. Encode them with `encode_path_segment` when assembling URLs by hand, and cover those characters in a test. Generated (Progenitor) clients already percent-encode path parameters, so pass raw values to their setters; pre-encoding would double-encode `%`. -- **Build next actions from a command template plus structured params, never by formatting a command line.** Don't write `format!("... --query '{query}'")`: -a quote in the value breaks the command, and shell metacharacters can inject another one. Instead, declare the command and pass each value -(e.g. a search query or pagination cursor) as a named param; the consumer fills them in and handles quoting. Param names must match the target command's args, -`` names in the template must match those params, and don't declare params the target command doesn't accept. -- **Mark dry runs.** Every dry-run path returns `CommandResult::with_dry_run()` so envelope and audit consumers can tell a preview from an executed mutation. Mark every command with external effects as mutating so the engine's dry-run safeguard prevents unintended calls, and have dry-run validate and read every prerequisite the real call needs, so it fails wherever the real call would rather than reporting an impossible success. -- **Keep next actions honest.** Emit them only when they are executable and appropriate to the returned state. Never include consent-bypass flags (e.g. `--agree`) in suggested commands, and re-present approval guidance after material changes so stale approval isn't reused. -- **Constrain inputs at parse time.** Restrict flags to the API's documented values in argument parsing so invalid input fails locally with clear help, not after a network request. -- **Fail with actionable validation errors.** Every user-correctable input or manifest failure gets a stable validation error with a `fix`. Never silently turn malformed configuration into an empty payload. -- **Don't mask API results.** Surface in-band API errors as errors. Preserve empty acknowledgements explicitly; don't substitute default "success" objects or cache errors as empty data. -- **Honor protocol semantics.** Handle case-insensitive headers, relative pagination links and query-bearing paths, while preserving the original request value. -- **Report ambiguity.** When several catalog entries match, return an explicit ambiguity result rather than silently picking the first. -- **Keep output consistent.** Output schemas, default-field projections and every execution mode must agree; document mode-only fields as optional, since default rendering can otherwise discard the useful result. Test the rendered, default-projected output, and state each test's real scope. -- **Don't log sensitive payloads.** Avoid the `--debug transport` logging helpers for requests or responses that may contain customer, payment or order data. -- **Map only the error you mean to map.** When polling or retrying for eventual consistency, only the exhausted expected status (e.g. 404) becomes `not_found`. -Network errors, 429s and 5xxs keep their real error mapping. -- **Use the selected environment.** Don't add private per-service URL overrides or `--env` flags on follow-up commands. +- **Encode dynamic path segments.** IDs containing `/`, `?`, `#` or `%` must not change URL structure. Use `encode_path_segment` for hand-built URLs and test those characters. Generated (Progenitor) clients already encode path params, so pass raw values to them; pre-encoding double-encodes `%`. +- **Build next actions from a command template plus structured params, never a formatted command line.** A quote in a value breaks `format!("... --query '{query}'")`, and shell metacharacters can inject commands. Param names must match the target command's args and the template's ``s. +- **Keep next actions honest.** Emit them only when executable and appropriate to the returned state. Never include consent-bypass flags (e.g. `--agree`), and re-present approval guidance after material changes so stale approval isn't reused. +- **Make dry runs faithful.** Mark commands with external effects as mutating so the engine's dry-run safeguard applies. Dry-run validates and reads every prerequisite the real call needs, so it fails where the real call would, and returns `CommandResult::with_dry_run()` so consumers can tell a preview from an executed mutation. +- **Keep output consistent across modes.** Preview and real runs return the same fields in camelCase, and the preview says what the real run would do (e.g. items that would fail are reported separately). Output schemas and default-field projections must include every field users should see, or default rendering drops it. +- **Test what users see.** Assert on rendered output with default fields, not just the helper that builds it. A test comment must state what the test actually exercises; rendering a hand-written JSON literal doesn't prove the handler produces it. +- **Validate early, fail actionably.** Restrict flags to the API's documented values at argument parsing. Every user-correctable input or config failure gets a stable validation error with a `fix`; never turn malformed config into an empty payload. +- **Don't turn failures or "no content" into fake successes.** APIs may report failure inside a 2xx response (an error object, or error-severity `messages`), which generated clients can deserialize as a valid empty result. Return an error and never cache it, or a transient failure is served as truth until the cache expires. A 202/204 with no body stays null/none, not `Default::default()`, which prints as a real, empty resource. +- **Follow HTTP and API conventions.** Header names are case-insensitive (`idempotency-key` matches a spec header `Idempotency-Key`). Pagination links may be relative, so read the token from the query string rather than requiring an absolute URL. Strip a user-supplied query string (`/v1/items?limit=10`) when matching the catalog, but keep it on the request. +- **Don't log sensitive payloads.** Avoid the `--debug transport` helpers for requests or responses that may contain customer, payment or order data. +- **Map only the error you mean to map.** When polling for eventual consistency, only the exhausted expected status (e.g. 404) becomes `not_found`; network errors, 429s and 5xxs keep their real mapping. +- **Use the selected environment.** No private per-service URL overrides or `--env` flags on follow-up commands. ## 3. Write for the customer @@ -42,8 +37,7 @@ Users don't know our system names, API names or environments. - Help text, guides and output must not leak internal terms (service names, scopes, environment plumbing, implementation jargon). - Command descriptions are short imperatives from the user's point of view. -- Text may address AI assistants directly when they must behave differently from a human (e.g. obtaining explicit user consent before a charge). -Put it in a clearly marked `AI assistants:` note, and keep it separate from the customer-facing prose. +- Text may address AI assistants directly when they must behave differently from a human (e.g. obtaining explicit user consent before a charge). Put it in a clearly marked `AI assistants:` note, and keep it separate from the customer-facing prose. - Prefer common terms users already know. If an API resource name differs, define it once in the guide. - Every flag gets concrete examples and discoverable values. Don't ask for things the system can infer, and don't assume users know standards by name. - If a command needs a value produced by an earlier command (an ID, a selection), that command's output and the guide must show where to get it.