Azure / Azure/azure-rest-api-specs
ARM API Reviewer agent: expand @extension(...) enforcement and eval coverage in TypeSpec source
- Dominant language
- TypeSpec
- Stars
- 3.1k
- Forks
- 5.9k
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 424
Description
### Background
[PR #43273](https://github.com/Azure/azure-rest-api-specs/pull/43273) added Section 6.5 (`TSP-ARRAY-IDENTIFIERS`) to the TypeSpec review instructions and a Section 5 anti-pattern entry stating that `@extension(...)` must not appear in TypeSpec source for any reason. The reasoning is consistent: whenever an author reaches for `@extension`, a dedicated TypeSpec decorator (or an Azure TypeSpec template) exists that does the right thing without bypassing the type system or emitter conventions.
That PR shipped the rule wording and one eval fixture for the `x-ms-identifiers` case (stimulus `070004`). The remaining `@extension(...)` misuses, which are at least as common in practice, are not covered by the eval harness and have no per-case guidance in the instruction files. Without fixtures and rubric coverage, the agent''s behavior on those cases cannot be measured or regression-tested, and reviewers receive uneven feedback depending on which extension the author happened to choose.
The most common cases that need explicit coverage:
1. `@extension("x-ms-mutability", ...)`. Replace with `@visibility(...)` (`Lifecycle.Read`, `Lifecycle.Create`, `Lifecycle.Update`).
2. `@extension("x-ms-secret", true)`. Replace with `@secret`.
3. `@extension("x-ms-client-flatten", true)`. For new APIs, do not flatten at all (this aligns with the existing Section 5 guidance that bans new `@flattenProperty`). For pre-existing flattening preserved for backward compatibility, use `@flattenProperty` on the property, not `@extension`.
4. `@extension("x-ms-enum", { ... })`. Replace with a TypeSpec `union` of string literals; the Azure emitter derives `x-ms-enum` (including `name` and `modelAsString`) automatically.
5. `@extension("x-ms-discriminator-value", "...")`. Replace with `@discriminator(...)` on the base model and a literal-typed discriminator property on each variant.
6. `@extension("x-ms-long-running-operation", true)` and `@extension("x-ms-long-running-operation-options", ...)`. Replace with the ARM async operation templates (`ArmResourceCreateOrReplaceAsync`, `ArmResourcePatchAsync`, `ArmResourceActionAsync`, `ArmResourceDeleteAsync`, etc.).
7. `@extension("x-ms-azure-resource", true)`. Replace with the ARM resource base templates (`TrackedResource`, `ProxyResource`, `ExtensionResource`).
8. `@extension("x-ms-pageable", ...)`. Replace with the ARM list operation templates (`ArmResourceListByParent`, `ArmListBySubscription`) or Azure.Core paged operation templates, which set `x-ms-pageable` automatically.
### References
- [PR #43273](https://github.com/Azure/azure-rest-api-specs/pull/43273) (original `TSP-ARRAY-IDENTIFIERS` rule and the Section 5 anti-pattern entry).
- Agent definition: [`.github/agents/arm-api-reviewer.agent.md`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/agents/arm-api-reviewer.agent.md).
- Rule files to extend:
- [`.github/instructions/typespec-review.instructions.md`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/instructions/typespec-review.instructions.md) (Section 5 anti-patterns, Section 6 type rules, "TypeSpec Review Checklist Summary").
- [`.github/instructions/armapi-review.instructions.md`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/instructions/armapi-review.instructions.md) ("ARM Review Checklist Summary").
- Eval harness and precedent from PR #43273:
- [`.github/skills/evals/arm-api-reviewer/fixtures/typespec/x-ms-identifiers-violations.tsp`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/skills/evals/arm-api-reviewer/fixtures/typespec/x-ms-identifiers-violations.tsp).
- [`.github/skills/evals/arm-api-reviewer/evaluate/eval-typespec.yaml`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/skills/evals/arm-api-reviewer/evaluate/eval-typespec.yaml) (stimulus `070004`).
- [`.github/skills/evals/arm-api-reviewer/README.md`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/skills/evals/arm-api-reviewer/README.md), [`fixtures/README.md`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/skills/evals/arm-api-reviewer/fixtures/README.md), [`.vally.yaml`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/skills/evals/arm-api-reviewer/.vally.yaml), [`run-evals.ps1`](https://github.com/Azure/azure-rest-api-specs/blob/main/.github/skills/evals/arm-api-reviewer/run-evals.ps1).
- [TypeSpec built-in decorators](https://typespec.io/docs/standard-library/built-in-decorators).
- [Azure TypeSpec libraries](https://azure.github.io/typespec-azure/docs/libraries/).
### Requested work
1. For each `@extension(...)` case listed in the Background (and any further case discovered during the work), add per-case guidance to `typespec-review.instructions.md`:
- State the forbidden extension and the dedicated decorator or template that replaces it.
- Include a short correct-pattern code block, following the structure used in Section 6.5.
- State whether a `#suppress` is ever acceptable for the underlying linter rule, and if so under what narrow conditions. Apply the existing Section 4.1 Warning Suppression Policy in every such note.
2. Add a single cross-cutting row to the "TypeSpec Review Checklist Summary" stating that `@extension(...)` must not appear in TypeSpec source for any reason, and add per-case rows for the most common extensions (mutability, secret, enum, discriminator-value, LRO, azure-resource, pageable, client-flatten).
3. Add the matching cross-cutting row to the "ARM Review Checklist Summary" in `armapi-review.instructions.md`, mirroring the row added by PR #43273 for the array identifiers case.
4. Add eval fixtures under `.github/skills/evals/arm-api-reviewer/fixtures/typespec/`:
- One fixture per `@extension(...)` case, each containing both a seeded violation and a positive control that shows the correct decorator or template in use.
- Keep fixtures small and focused so a failure points at one rule.
- Consolidate multiple cases into a single multi-case fixture only when the rubric grading remains unambiguous.
5. Add corresponding stimuli to `.github/skills/evals/arm-api-reviewer/evaluate/eval-typespec.yaml`:
- Each stimulus must include `output-matches` graders for the extension name and the correct replacement decorator or template, plus a `prompt` grader rubric that requires the agent to:
- Flag every seeded violation with the correct rule reference (Section 5 anti-pattern, plus the per-case section).
- Recommend the correct dedicated decorator or template as the fix.
- Never recommend `@extension(...)` as a fix.
- Not flag any of the positive controls.
6. Update the stimulus and fixture counts in `.vally.yaml`, `README.md`, `fixtures/README.md`, and `run-evals.ps1`, following the bookkeeping pattern from PR #43273.
7. Run the full eval suite locally and confirm that all new stimuli pass before opening the PR.
### Acceptance criteria
- Every `@extension(...)` case listed in the Background has dedicated guidance in `typespec-review.instructions.md` with the correct replacement decorator or template and a worked example.
- The "TypeSpec Review Checklist Summary" and the "ARM Review Checklist Summary" each contain a cross-cutting row stating that `@extension(...)` is forbidden in TypeSpec source for any reason.
- The eval suite contains at least one stimulus per `@extension(...)` case, each with a fixture that includes both a seeded violation and a positive control. All new stimuli pass when the suite is run locally.
- Stimulus and fixture counters in `.vally.yaml`, `README.md`, `fixtures/README.md`, and `run-evals.ps1` are consistent with the new count after the change.
- The PR introducing these changes does not modify any service `specification/**` files, language emitters, or `eng/` content. Scope stays within agent instructions, eval fixtures, and eval configuration.
Contributor guide
Assessment
This issue has not been assessed yet.