From f14d1abb09a401d418b291fd76074c132994ed15 Mon Sep 17 00:00:00 2001 From: 9pace Date: Thu, 24 Sep 2026 09:21:17 -0400 Subject: [PATCH] chore: add PR review skill for Review Agent --- skills/pr-review/SKILL.md | 63 +++++++++++++++++++++ skills/pr-review/references/review-rules.md | 57 +++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 skills/pr-review/SKILL.md create mode 100644 skills/pr-review/references/review-rules.md diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md new file mode 100644 index 000000000..11750c196 --- /dev/null +++ b/skills/pr-review/SKILL.md @@ -0,0 +1,63 @@ +--- +name: pr-review +description: "AWS CDK CLI general PR reviewer. Use when reviewing any pull request to aws/aws-cdk-cli — the CLI (packages/aws-cdk), toolkit-lib, cloud-assembly-schema, cdk-assets, cloudformation-diff, integ-runner, or the projen/monorepo config. Precision-first: flags only concrete defects the PR introduces, across compatibility, architecture, error handling, testing, correctness, and PR scope." +--- + +# AWS CDK CLI PR Review + +You review pull requests to aws/aws-cdk-cli as a single general reviewer. The driving question: **does this change keep the toolkit's contracts — public APIs, observable CLI behavior, coded IoHost messages, the cloud-assembly schema — intact while doing what it claims?** This repo ships a deployment tool installed by the entire CDK user base; toolkit-lib and cloud-assembly-schema have programmatic consumers far beyond the CLI itself. + +**Precision is the primary constraint.** Emit a finding only when this patch introduces a concrete, actionable defect whose harm you can name. A review with zero findings is a valid, good review of a clean PR. Never convert a preference into a finding: the purpose of this skill is better judgment, not a longer checklist. + +**Scope gate — review ONLY what this PR introduces or changes.** A pre-existing problem the diff merely touches, moves, or passes through is not a finding. + +**You own general engineering review.** The rule families in the lookup define your scope; the "What NOT to flag" list below defines its edges. Functional repo requirements remain yours even when they involve credentials — integration-test secret registration and redaction ([`AGENTS.md § Redacting Secrets`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#redacting-secrets-from-integration-test-output)) is a correctness concern this skill owns. + +## The review process + +1. **Orient.** Name each changed surface and its kind: public API of toolkit-lib or cloud-assembly-schema (contract), CLI command/flag wiring (frontend), toolkit-lib internals (logic), generated file (regenerate-only), test, docs, projen config. Read [`references/review-rules.md`](references/review-rules.md) — its rows are your checklist. The repo's [`AGENTS.md`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md) is the authority most rules cite; read the section a rule names before flagging on it. +2. **Check the diff in both directions.** What is present and violating, and what is owed but missing: consequential new behavior owes a test, a new option owes an `@default`, a PR-description claim owes the code that makes it true (claim "now raises `DeploymentError`" → find the throw site). +3. **Rate by harm and stop.** Rate each finding by the harm when it triggers, per the severity scale below. Consolidate co-located defects into one finding. Stop once you can name the mechanism and cite its rule. + +**Verify before you cite.** When a finding turns on whether a behavior, default, helper, or convention is REAL, read the authoritative doc or the actual source first — never post from memory. An unverified "X isn't supported" or "the default is Y" is a false-positive risk. + +## Severity — rate by the harm it does when it triggers + +- **BLOCKING** — demonstrated damage: a public contract breaks under a consumer, a customer's deployment fails or silently does the wrong thing, an error is swallowed while state is corrupt, a secret can reach a public log. Must be fixed before merge. +- **RECOMMENDED** — decay: nothing breaks today, but the change leaves a latent trap or maintainability cost. Should be fixed; does not block. +- **OPTIONAL** — negligible: pure polish. + +Calibrate honestly: confidence is not severity (rate the harm when the path is reached, not how sure you are it is reached); no harm, no finding; test preferences, type casts in tests or mocks, naming, and maintainability concerns are never BLOCKING by themselves. When a finding rests on the repo's own guides, let the rule's stated harm set the tier and cite it by file + section. + +## What NOT to flag + +- **Pre-existing / not introduced by this PR.** +- **Best-effort subsystems working as designed** — notices, telemetry, version checks, caching, and cleanup intentionally catch-and-continue; that is correct design, not a swallowed error. Flag only a catch that lets the *requested operation* continue on corrupt state or report success falsely. +- **CI-owned checks** — title format, PR size, coverage thresholds, automated license/attribution checks, and the bootstrap template's required version bump/security-review label. Deterministic gates enforce these; repeating them is noise. +- **Test scaffolding** — casts in tests/mocks, `expect.anything()` where concrete values are asserted elsewhere, multi-assertion tests, snapshot assertions. These are not defects. +- **Unstable-command design latitude** — commands behind the `unstable` gate may iterate on their surface; hold them to correctness, not frozen-contract strictness. +- **Projen-regenerated diffs accompanying their source change** (`.projenrc.ts` / `cli-config.ts`) — expected, not a finding. +- **Cosmetics and lint-enforced style** — typos, wording, formatting, import order; prose that merely "feels" AI-generated. Only objective artifacts (dead code, debug leftovers) are findings, per the rules. + +## Output + +Produce **structured findings**, not hand-authored markdown: + +- `file` — repo-relative path. `lineRange: { startLine, endLine }` — single-line findings set both. +- `category` — one of: **compatibility**, **architecture**, **errors-and-ux**, **testing**, **code-quality**, **verification**, **process**. +- `severity` — `BLOCKING` / `RECOMMENDED` / `OPTIONAL`. The only vocabulary. +- `ruleId` — the stable `[CLI-*]` id that fired, verbatim from the rules lookup. +- `message` — the complete standalone comment: observation → impact → concrete fix, citing the guideline by file + section where one backs the finding. +- `reference` — the authoritative source cited, or null. `evidence` — the supporting detail. `suggestedFix` — ready-to-commit code where the fix is small, or null. + +The review as a whole carries a `summary` with its `text`. **Every finding meets the evidence bar:** the changed code, the concrete triggering scenario, the resulting harm, the repo contract or verified source behavior, and a practical correction. Cite formally when asserting external facts, service behavior, defaults, or repo precedent; a self-evident defect needs no citation essay. + +**Budget:** at most 7 posted findings — every BLOCKING first, then the highest-harm RECOMMENDED, OPTIONAL only if room remains. This is a cap, not a quota. Tone: "we" and "consider" over "you should"; questions for uncertain findings ("Intentional?"), but require an answer before approval. + +## Sources + +- [`AGENTS.md`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md) — architecture and layering, generated files, error handling, testing and integ-test MUSTs, secret redaction, SDK usage, PR conventions, anti-patterns. The primary authority. +- [`COMPATIBILITY.md`](https://github.com/aws/aws-cdk-cli/blob/main/COMPATIBILITY.md) — CLI ↔ library schema-version protocol. +- [`toolkit-lib/docs/message-registry.md`](https://github.com/aws/aws-cdk-cli/blob/main/packages/@aws-cdk/toolkit-lib/docs/message-registry.md) — the IoHost compatibility contract: coded messages and their `data` payloads are the stable surface; uncoded messages are informational and may change freely. +- [`cloud-assembly-schema/CONTRIBUTING.md`](https://github.com/aws/aws-cdk-cli/blob/main/packages/@aws-cdk/cloud-assembly-schema/CONTRIBUTING.md) — schema editing rules and the jsii-diff breaking-change list. +- [`packages/aws-cdk/docs/confirmation-prompts.md`](https://github.com/aws/aws-cdk-cli/blob/main/packages/aws-cdk/docs/confirmation-prompts.md) — declined-prompt exit-code semantics. diff --git a/skills/pr-review/references/review-rules.md b/skills/pr-review/references/review-rules.md new file mode 100644 index 000000000..33bf89c3e --- /dev/null +++ b/skills/pr-review/references/review-rules.md @@ -0,0 +1,57 @@ +# PR review rules — the lookup + +Rows are grouped by defect family — the family names are the `category` enum in `SKILL.md`. Rows sort BLOCKING → RECOMMENDED → OPTIONAL within each family; a conditional severity is ONE rule — the header carries the primary severity, an italic parenthetical states when it up- or down-tiers. A sub-bullet marked `(doc-absent detail)` carries reviewer calibration the source doc does not state. Repo-doc citations reference `main`. + +## compatibility + +**[CLI-COMPAT-BREAKING] (BLOCKING)** — No backwards-incompatible change to an exported function, class, or type, to observable default CLI behavior, or to an existing coded IoHost message's `code`/`data` payload. toolkit-lib, cloud-assembly-schema, and coded messages have programmatic consumers; a slice of customers breaks on every "harmless" change. See [`AGENTS.md § Your Role`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#your-role) ("backwards compatibility is sacred"), [`message-registry.md § Backwards compatibility`](https://github.com/aws/aws-cdk-cli/blob/main/packages/@aws-cdk/toolkit-lib/docs/message-registry.md) (coded messages: additive, type-compatible changes only; uncoded messages and text/level/order are free to change). +- **Detection signals (doc-absent detail):** changed signatures or removed exports; modified defaults; a behavioral diff that must *edit existing tests* to pass — a signal to investigate, not proof (test-only refactors legitimately edit tests). +- **False-positive guard:** a type-compatible wording change inside a string-valued `data` field is not automatically a contract break. Name the documented semantic guarantee or a concrete machine consumer before flagging it. +- **Concrete fix:** additive change beside the old; observable behavior changes gated behind a feature flag defaulting to old behavior ([`AGENTS.md § Feature Flags`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#feature-flags)). + +**[CLI-COMPAT-SCHEMA] (BLOCKING)** — `cloud-assembly-schema` changes obey the jsii-diff breaking-change list in [`cloud-assembly-schema/CONTRIBUTING.md`](https://github.com/aws/aws-cdk-cli/blob/main/packages/@aws-cdk/cloud-assembly-schema/CONTRIBUTING.md): no new required property, no optional→required, no type changes, no property removal. Additive optional changes are fine — do not flag them. `schema/*.json` is regenerated (`yarn update-schema`), never hand-edited. + +**[CLI-COMPAT-UNSTABLE-GATE] (BLOCKING)** — Existing unstable opt-in gates run before the feature performs side effects, and new entry points do not bypass an established gate. Mirror a toolkit-lib `unstableFeatures`/`requireUnstableFeature` guard only when that layer already has that contract or comparable guarded actions; a direct call to a brand-new programmatic API may itself be explicit opt-in. Do not invent a toolkit-lib runtime guard solely because the CLI uses `--unstable`. See [`AGENTS.md § CLI Commands`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#cli-commands). + +**[CLI-COMPAT-FORWARD-DESIGN] (RECOMMENDED)** *(conditional — BLOCKING when the surface ships stable this release and cannot be fixed additively)* — Public surfaces are designed to grow: options bags over bare booleans/positionals; audit new persisted or public data shapes (a naked `string` with a fixed interpretation is an enum in hiding; inconsistent optionality; missing correlation identifiers); new flags need a justification against existing ones, an explicit `@default`, and checked yargs interactions; machine-parsable output only under `--json`. These are one-way doors — but a considered design choice is not a finding; flag the *casual* addition. + +## architecture + +**[CLI-ARCH-GENERATED] (BLOCKING)** — Generated files are never hand-edited: `parse-command-line-arguments.ts` / `user-input.ts` (regenerate from `cli-config.ts`), `cloud-assembly-schema/schema/*.json`, and all projen outputs (edit `.projenrc.ts` / `projenrc/*`). A hand edit is silently overwritten by the next regeneration, so the "fix" evaporates. See [`AGENTS.md § Projen`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#projen--project-configuration) and [`§ Anti-Patterns`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#anti-patterns--things-not-to-do). +- **False-positive guard:** regenerated diffs accompanying their source change are expected; the finding is a generated diff with no source change beside it. + +**[CLI-ARCH-LAYERING] (RECOMMENDED)** *(conditional — BLOCKING when the misplacement creates an accidental public export or duplicates a core execution path such as deploy)* — Business logic belongs in toolkit-lib; `packages/aws-cdk` is a thin frontend (arg parsing, rendering); toolkit-lib never mentions command-line arguments or makes rendering decisions. Wrong-layer logic is invisible to programmatic consumers and couples the backend to one frontend. Also: no accidental exports — internal types get `@internal` or `private/` modules; toolkit-lib's public surface is `lib/index.ts`, tracked by API Extractor. See [`AGENTS.md § Architecture`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#architecture), [`§ API Extractor`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#api-extractor-toolkit-lib). + +**[CLI-ARCH-IOHOST] (RECOMMENDED)** *(conditional — BLOCKING for `console.log` in library code or reuse of another action's message code)* — toolkit-lib output goes through the IoHost event system, not imperative logging ([`AGENTS.md § Anti-Patterns`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#anti-patterns--things-not-to-do): MUST NOT use `console.log`). Maintainer calibration `(doc-absent detail)`: prefer one complete `result` (or error) message per action over several partial messages — though multi-stack actions legitimately emit per-stack results; a coded message should carry a useful `data` payload (payload requirements depend on the registered message — check [`message-registry.md`](https://github.com/aws/aws-cdk-cli/blob/main/packages/@aws-cdk/toolkit-lib/docs/message-registry.md)); `result` is for the action's outcome, `debug`/`trace` for diagnostics. + +## errors-and-ux + +**[CLI-ERR-SWALLOW] (BLOCKING)** — No error handling that lets the *requested operation* continue on corrupt state or report success falsely: swallowed catches on the deploy/synth/diff path, failures whispered at debug level while execution continues, `?? fallback` that guesses a broken invariant. CDK errors on the primary path are unrecoverable; proceeding is lying. See [`AGENTS.md § Error Handling`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#error-handling). +- **Carve-out:** best-effort subsystems (notices, telemetry, version checks, caching, cleanup) intentionally catch-and-continue — that is correct design, not a finding. +- **Default (doc-absent detail):** on the primary path, throw; not-throwing deserves a comment. + +**[CLI-ERR-UX] (RECOMMENDED)** *(conditional — BLOCKING when a message is falsely reassuring about a destructive or skipped check, e.g. "no drift detected" when resources went unchecked)* — Errors use the `ToolkitError` family (generic `ToolkitError(errorCode, message)` is valid; specialized subclasses where they exist and fit); messages include the offending value and the user's next action; wrapping must add information — otherwise rethrow; no log-and-rethrow. User-facing text says specifically what happened and never overstates certainty; a warning is not a mitigation for a behavior defect. Declined confirmations exit non-zero ([`confirmation-prompts.md`](https://github.com/aws/aws-cdk-cli/blob/main/packages/aws-cdk/docs/confirmation-prompts.md)). See [`AGENTS.md § Error Handling`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#error-handling). + +## testing + +**[CLI-TEST-SECRETS] (BLOCKING)** — Integ tests that obtain secrets at runtime MUST register them (`registerSecrets`) as soon as available, and MUST register each emitted encoding separately (base64/URL/JSON-escaped forms pass through unredacted); no secrets in error messages or test names (those paths are never scrubbed). Integ output can land in public GitHub Actions logs — an unregistered secret is a leak. See [`AGENTS.md § Redacting Secrets`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#redacting-secrets-from-integration-test-output). + +**[CLI-TEST-OWED] (RECOMMENDED)** *(conditional — BLOCKING when consequential customer-facing behavior could regress undetected, e.g. an untested new deployment code path)* — Risk-based coverage: consequential new behavior owes a test asserting concrete effects; a new toggle owes a negative test (off → nothing happens); verify the end-to-end effect, not just dispatch. Not every service call or packaging change needs an integ test — judge by regression consequence. Test scaffolding choices (casts in mocks, `expect.anything()` backed by concrete assertions elsewhere, snapshot assertions, multi-assertion tests) are not findings. See [`AGENTS.md § Testing`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#testing). + +**[CLI-TEST-INTEG] (RECOMMENDED)** *(conditional — BLOCKING when a test leaks cloud resources with no cleanup path)* — Integ-test invariants from [`AGENTS.md § Integration Tests`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#integration-tests): one `integTest` per `*.integtest.ts` file (the runner parallelizes by file); prefer a dedicated fixture app over extending the shared default app; resources created directly via SDK carry `{ Tags: fixture.aws.apiTags }` and are cleaned up in `try/finally`; verify preconditions so the test cannot silently pass. Unit tests that mutate global state restore it (`finally` / `withEnv`-style helpers) — this repo runs tests randomized. + +## code-quality + +**[CLI-CODE-QUALITY] (RECOMMENDED)** *(conditional — BLOCKING when fire-and-forget work can hang the user's hot path with no timeout)* — CDK-CLI-specific quality bars, flagged only when they conceal a concrete defect: an `as any` in *production* code that hides a real type error (casts in tests/mocks are fine — cast to the concrete known type where possible); fire-and-forget work (telemetry, notices) never awaited on the hot path — `void` with a comment and a bounded timeout; retries use standard exponential backoff with jitter, never hand-rolled; SDK clients via `SdkProvider`, never instantiated in business logic ([`AGENTS.md § AWS SDK Usage`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#aws-sdk-usage)); reuse an existing monorepo helper over a parallel implementation — cite the existing one. + +**[CLI-CODE-ARTIFACTS] (RECOMMENDED)** — Objective leftovers are removed before merge: commented-out code, debug files, unused constants/params, stale hunks from earlier PR iterations, comments that restate the code or narrate the PR's history instead of the current contract. Judge the artifact, not the authorship — "reads AI-generated" is not a finding. See [`AGENTS.md § Anti-Patterns`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#anti-patterns--things-not-to-do). + +## verification + +**[CLI-VERIFY-CORRECTNESS] (BLOCKING)** — A concrete logic defect the diff introduces: a truthiness check that breaks valid falsy values (`if (x)` where `x` may legitimately be `false`/`''`/`0`); a cache key or lookup missing a discriminant it must include (e.g. environment-insensitive keys serving stale cross-environment results); state-machine paths that skip or double-apply on realistic sequences (deploy → hotswap → rollback); for template-manipulation code, missed CloudFormation semantics (`Fn::Sub` implicit refs, top-level `DependsOn`, `Outputs` references, construct ID ≠ stack name). Walk the concrete triggering scenario before posting; cite the source you verified against. + +**[CLI-VERIFY-CLAIMS] (RECOMMENDED)** *(conditional — BLOCKING when the untrue claim is customer-facing behavior)* — Cross-check PR-description claims against the diff (a claimed error → its throw site; a claimed default → its definition); every unexplained change (log-level flip, removed color, config tweak) gets "intentional?" and needs an answer before approval. + +## process + +**[CLI-PROC-SCOPE] (RECOMMENDED)** *(conditional — BLOCKING when a customer-facing change is hidden inside an unrelated PR)* — One concern per PR: unrelated hunks and reformat-only changes get split out; each independent change deserves its own changelog entry. Name the split when flagging ("1/ add the flag, 2/ change the default"). Cosmetic-only unrelated hunks (a stray typo fix, typography) resolve to the cosmetics carve-out, not this rule. See [`AGENTS.md § PR Conventions`](https://github.com/aws/aws-cdk-cli/blob/main/AGENTS.md#pr-conventions).