Add code-review agent skill for Copilot code review

[Timothee Guerin (timotheeguerin)]

Copilot code review runs on every PR here with no knowledge of the rules this repo actually enforces. It doesn't know that no-sdk-clients reported on NoTarget can never be suppressed, that a feat changeKind is invalid, that appending a SyntaxKind member is fine but inserting one is not, or that each package exports a shared Tester from test/test-host.ts. So it reviews TypeSpec PRs like generic TypeScript.

This adds .github/skills/code-review/SKILL.md, which Copilot code review picks up automatically from .github/skills. It gives the reviewer five things to check, each pointing at the file that defines the rule:

• Breaking changes — the Tier 0–3 table from breaking-change-policy.mdx, plus the bug exception. An undeclared breaking change is the top finding.
• Changesets — the six valid changeKind values from .chronus/config.yaml, per-package/per-type splitting, and the changedFiles exclusions so docs-only PRs aren't nagged.
• Compiler API usage — diagnostics targeted at NoTarget are never suppressible and error severity can't be suppressed at all (program.ts); listServices(program)[0] is undefined for model-only programs; a severity downgrade changes control flow via program.hasError().
• Tests — reuse the package's shared tester instead of createTester per file, assert with expectDiagnostics/expectDiagnosticEmpty rather than hand-rolled diagnostics.some(...), and drive the real entrypoint.
• Emitter output stability — unexplained snapshot diffs, inline-vs-hoist, $ref correctness.

It also says what not to comment on: formatting, lint-covered rules, style preferences, generated artifacts.

Does it work?

Dry-run against #11823. The skill surfaces the three points reviewers raised by hand there:

Skill rule	Human comment it reproduces
§3 NoTarget is not suppressible	"if there is no service this will still not be suppressable"
§4 shared tester	"you shouldn't be creating a tester for every file"
§4 expectDiagnostics	"reason can't use expectDiagnostics?"

It additionally flags the new test importing getNoSdkClientsDiagnosticTarget, which isn't exported from src/emitter.ts, and the diagnostic message that still says the generator "expects at least one client" after it was downgraded to a warning.

An earlier draft excluded packages/http-client-*; that exclusion hid every finding above, so the skill now covers those packages too and defers to .github/instructions/*.instructions.md for their specifics.

No changeset: .github/** isn't a published package.

[github-actions]

No changes needing a change description found.

[azure-sdk-automation]

You can try these changes here

🛝 Playground	🌐 Website	🛝 VSCode Extension

[Copilot]

🟡 Changes recommended

The new skill document contains a few objectively inaccurate/overbroad rules (notably around .chronus scope/exclusions and diagnostic suppression) that would lead Copilot to produce incorrect review feedback.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new Copilot Code Review skill (code-review) under .github/skills/ to guide automated reviews toward TypeSpec-specific repository rules (breaking-change tiers, .chronus changesets, compiler API usage, test conventions, and emitter output stability), and away from low-signal feedback (formatting, lint-owned issues, generated artifacts).

Changes:

• Introduces .github/skills/code-review/SKILL.md with TypeSpec-specific review checks and “what not to comment on” scope boundaries.
• Documents repository rule sources to cite (breaking-change policy doc, .chronus/config.yaml, compiler implementation, and package-local test patterns).

File summaries

File	Description
.github/skills/code-review/SKILL.md	Adds a TypeSpec-specific code review skill document with explicit review checks and scope guidance.

Review details

Suppressed comments (2)

.github/skills/code-review/SKILL.md:81

• The changeset exclusions are broader here than what .chronus/config.yaml actually defines under changedFiles (it’s not “test files” in general). Tighten this list to the exact patterns so the skill doesn’t incorrectly tell reviewers a changeset is optional for other test naming schemes.

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

.github/skills/code-review/SKILL.md:96

• This suppression guidance omits a key compiler limitation: diagnostics targeting only a file are also not suppressible (shouldSuppress returns false when "file" in target). Without this, reviewers may think a file-targeted warning is suppressible when it isn’t.

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

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.