Add code-review agent skill for Copilot code review

.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.