code-review

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.