diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md new file mode 100644 index 00000000000..b9f9438e9ad --- /dev/null +++ b/.github/skills/code-review/SKILL.md @@ -0,0 +1,164 @@ +--- +name: code-review +description: > + Review TypeSpec pull requests against the project's own rules: the Tier 0-3 breaking change + policy, `.chronus` changelog requirements, compiler API usage, test framework conventions, and + emitter output stability. Use when reviewing a diff or pull request in this repository. +--- + +# TypeSpec code review + +Review the diff against the rules this project actually enforces. Every comment must point at a +concrete problem in the changed code and cite the rule or code that makes it a problem — if you +cannot cite it, do not post it. + +## Scope + +Applies to the whole repository: the pnpm workspace packages (`packages/compiler`, `packages/http`, +`packages/openapi3`, `packages/json-schema`, `packages/versioning`, …), the language emitter +packages (`packages/http-client-{csharp,java,python}`), `.chronus/changes/**`, and their tests. + +For the language emitter packages, §1 (tier policy) does not apply to their internal APIs and their +own rules in `.github/instructions/*.instructions.md` take precedence — but §2 (changeset), §3 +(compiler API usage) and §4 (tests) apply everywhere. + +Out of scope — do not comment on: + +- Generated artifacts: `pnpm-lock.yaml`, generated Python/C#/Java code under `generator/`, + `tests/generated/`, and generated reference docs under + `website/src/content/docs/docs/libraries/*/reference/**`. +- Formatting and lint-covered issues — prettier (`pnpm format`) and oxlint own those. +- Naming/style preferences, or unchanged code surrounding the diff. + +## Read before judging + +Consult these and cite them in findings: + +- `website/src/content/docs/docs/handbook/breaking-change-policy.mdx` — the authoritative tier table + and the bug exception. +- `.chronus/config.yaml` — the valid `changeKind` values and the `changedFiles` exclusions. +- `.github/copilot-instructions.md` — changelog message guidelines and the TDD expectation. +- Sibling tests in the touched package — the established tester/assertion patterns. +- The public compiler API (`@typespec/compiler`) and `@typespec/compiler/testing` exports — to + confirm a used API exists and behaves as the change assumes. + +## 1. Breaking changes and public surface + +Decide which tier each change touches, then whether it breaks an _existing_ spec or consumer. +Adding new syntax, types, or capabilities is not breaking; changing how something that already +worked behaves is. + +- **Tier 0 — language syntax/semantics, the type graph (`Type`), default TypeKits, exported helper + and accessor functions.** Flag: an existing valid spec that now parses differently, fails, or + produces a different type graph; a removed/renamed export or changed signature; a changed runtime + result for existing input; a removed or repurposed field on a `Type`. Adding a _required_ field to + a `Type` is allowed by policy but breaks downstream construction — call it out. +- **Tier 1 — the AST (`Node`, `SyntaxKind`).** Appended enum members and new union variants are + allowed (TypeScript exhaustiveness errors are explicitly not breaking). Flag renumbered enums + (members inserted rather than appended), removed node kinds, and changed shapes of existing nodes. +- **Tier 2 — checker, symbols, `@internal` / `/internals` / `/experimental` exports, template + declarations, stdout/stderr output.** Not breaking; mention only if a change here silently alters + observable behavior. +- **Tier 3 — formatter and emitter output.** Semantically equivalent output changes are not + breaking; see §5 for the risk that matters. +- **Bug exception.** A technically breaking change that fixes clearly unintended behavior is not a + breaking change — but say so explicitly rather than staying silent. + +A change you judge breaking that has no `breaking` changeset is the highest-value finding in a +review. Raise it first. + +## 2. Changeset (`.chronus`) + +- Valid `changeKind` values are exactly: `internal`, `fix`, `dependencies`, `feature`, + `deprecation`, `breaking`. Anything else (`feat`, `docs`, `patch`, `minor`, `major`) is invalid. +- The kind must match reality: a new capability is `feature`, a fix is `fix`, and anything breaking + per §1 is `breaking`. +- One entry **per package and per change type**. A single entry bundling a feature in one package + with a fix in another must be split. +- `packages:` must list exactly the affected packages. +- Every package with a user-visible change needs an entry. Missing one is a finding — but note the + exclusions in `.chronus/config.yaml`: `**/*.md`, test files, and `packages/*/test/**` don't + require a changeset, so docs-only or test-only changes legitimately have none. +- Message quality: describes the change from the user's perspective; area tag only when targeting a + secondary area of a multi-area package, in `[bracket]` form (e.g. `[converter]`, `[formatter]`) — + no generic `core -` prefixes; a `feature` entry includes a short illustrative code block, a simple + fix does not. + +## 3. Compiler API usage + +Judge whether the change uses the TypeSpec compiler API the way the compiler expects, not merely +whether it type-checks. + +- **Diagnostic targets decide whether a diagnostic can be suppressed.** A diagnostic reported with `NoTarget` can never be + suppressed (`packages/compiler/src/core/program.ts` — `target === NoTarget` returns early before + the `#suppress` lookup), and a diagnostic with `severity: "error"` can never be suppressed either + (suppressing one produces `suppress-error`). So a diagnostic the user is meant to be able to + silence must be a `warning` **and** carry a real node/type target. Flag any target that resolves + to `NoTarget` on a realistic input as "not actually suppressible". +- **Pick the most precise target available**, falling back deliberately: the offending type → the + service namespace → `program.getGlobalNamespaceType()` → `NoTarget` only for genuinely + program-wide failures with no node to point at. +- **Don't assume the program has a service.** `listServices(program)` returns a possibly empty + array; `[0]` is `undefined` for model-only or service-less programs. Same for any lookup that can + return `undefined` — ask what happens in that case, and whether the diagnostic is still reachable + and suppressible there. +- **Severity changes have control-flow consequences.** `error` blocks emission via + `program.hasError()`; downgrading to `warning` means everything downstream now runs in a state it + previously never saw. Verify the continue path is safe and intended. +- **Diagnostic hygiene.** Report through the library's `reportDiagnostic`/`createDiagnostic` with a + catalog entry and stable code; never `throw` for a user error. The message text must still match + the behavior after the change — a message saying the emitter "requires" something it now merely + warns about is a finding. +- **Prefer stable public APIs.** Use exported helpers and TypeKits over the checker, symbols, or AST + (Tier 2, see §1); use `resolvePath`/`joinPaths` rather than `node:path`; exclude template + declarations with `isTemplateDeclaration` when navigating types. + +## 4. Tests + +- Every new code path, branch, diagnostic, and public API needs a test co-located in the package's + `test/**`. New behavior with no test is a finding. +- **Reuse the package's shared tester.** Packages export one from `test/test-host.ts` or + `test/tester.ts` (e.g. `packages/http/test/test-host.ts`, `packages/compiler/test/tester.ts`). + Calling `createTester(...)` inline in a new test file duplicates library/import setup and drifts — + flag it and point at the existing module. +- **Assert diagnostics with the helpers.** Use `expectDiagnostics` / `expectDiagnosticEmpty` from + `@typespec/compiler/testing`, not hand-rolled checks like + `program.diagnostics.some((x) => x.code.endsWith(...))`, which silently pass when the code, + severity, or target is wrong. +- **Exercise the real entrypoint.** A test must drive the shipped path (`$onEmit`, the decorator, + the emitter run) rather than re-implement the logic or call a helper in isolation — otherwise it + passes while the actual behavior is broken. +- **Verify imports resolve.** If a test imports a helper from `src`, confirm that symbol is actually + exported there; an import of a non-existent export makes the whole file fail to load. +- Tests must be meaningful: they should fail if the change were reverted. Flag tests that only smoke + run or assert nothing specific to the new behavior. +- Beyond the happy path: error and diagnostic paths, empty/anonymous forms, templates, and negative + cases — including the fallback branches identified in §3. +- Flag any weakening of existing coverage: `.skip`/`.only`, deleted assertions, or matchers + broadened to make a change pass. + +## 5. Emitter output stability + +For `packages/{openapi3,openapi,json-schema,http,http-server-*}`: + +- **Unexplained snapshot changes are the headline risk.** Every changed snapshot must be an + intentional, explained consequence of the change. A snapshot diff for specs unrelated to the + feature means existing users' output changed — raise it. +- Inline-vs-hoist, component naming, and declaration-creation changes must preserve behavior for + existing named types and only affect genuinely new inputs. Anonymous/empty-named types inline; + named types hoist without key collisions. +- `$ref` and component-key correctness: reordering name validation relative to ref generation must + not silently drop validation that previously errored — if that is an intended fix, it needs to be + called out and changeset'd. +- Emitters must handle new type-graph shapes defensively (new node kinds, new union variants) + rather than crash on them. + +## How to comment + +- One issue per comment, anchored on the offending line. +- State the problem, cite the rule (policy section, `.chronus/config.yaml`, guideline) or the + relevant `file:line`, and suggest the concrete fix. +- One or two sentences. No preamble, no summarizing the diff back to the author. +- Don't repeat a point another comment already makes. +- Lead with correctness, compatibility, and coverage. Minor suggestions are welcome only when they + are actionable and specific.