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.