ARM API Reviewer agent: align generated OpenAPI extension cleanup with TypeSpec guidance

## Summary

The ARM API Reviewer can classify removal of legacy raw OpenAPI extensions from TypeSpec-generated Swagger as a published-version immutability violation, then recommend restoring `@OpenAPI.extension(...)` and suppressing the TypeSpec linter. That guidance conflicts with current TypeSpec best practices.

The fix is to classify generated OpenAPI differences by semantic effect before applying published-version immutability. Metadata-only cleanup is not automatically a REST or ARM contract change. Semantics-bearing metadata must come from native TypeSpec constructs. Actual REST wire or ARM semantic changes remain subject to normal versioning rules.

## Problem

The published-version rule is intentionally strict, but it lacks a TypeSpec-generated metadata guard. A textual Swagger diff can result from removing a raw emitter decorator even when the API's wire contract is unchanged.

This creates three failure modes:

1. Legacy client metadata such as `x-ms-parameter-grouping` can be treated as a REST breaking change even when paths, parameters, bodies, status codes, and ARM behavior are unchanged.
2. Semantics-bearing metadata such as `x-ms-pageable` can be restored with a raw OpenAPI decorator rather than modeled with `@list` or an Azure paging template.
3. The suggested fix can require suppressing `@azure-tools/typespec-azure-core/no-openapi-client-extensions`, even though the upstream rule says not to suppress it.

## Evidence

- [PR #46130](https://github.com/Azure/azure-rest-api-specs/pull/46130) removes unnecessary OpenAPI extension decorators as part of a TypeSpec cleanup.
- [Discussion r3959450255](https://github.com/Azure/azure-rest-api-specs/pull/46130#discussion_r3959450255) recommends restoring `x-ms-parameter-grouping` and `x-ms-client-request-id` with `@OpenAPI.extension(...)` plus a `no-openapi-client-extensions` suppression.
- [Discussion r3959451157](https://github.com/Azure/azure-rest-api-specs/pull/46130#discussion_r3959451157) recommends restoring raw `x-ms-pageable` metadata rather than validating the native paging model.
- The current [`no-openapi-client-extensions`](https://azure.github.io/typespec-azure/docs/libraries/azure-core/rules/no-openapi-client-extensions/) guidance says not to suppress the rule. It maps `x-ms-parameter-grouping` to a parameter model and spread, and maps `x-ms-pageable` to `@list` or Azure paging templates.
- The [upstream rule source](https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-core/src/rules/no-openapi-client-extensions.ts) lists the client-altering extensions that must be modeled semantically.
- #43359 covers direct `@extension(...)` enforcement in TypeSpec source. It does not cover generated OpenAPI diff classification or its interaction with Section 26.0.

`x-ms-client-request-id` is not currently listed by the upstream linter rule. In PR #46130, however, removing `x-ms-client-request-id: true` does not remove or rename the actual `x-ms-client-request-id` HTTP header. Classify the observed effect without claiming that the linter covers this extension.

## Required decision procedure

Apply this procedure when changed generated OpenAPI is owned by TypeSpec and an `x-ms-*` value changes.

### 1. Establish ownership

Confirm TypeSpec owns the generated file. Evidence can include `x-typespec-generated`, the sibling TypeSpec version declaration, and emitter output configuration.

If the Swagger is handwritten, apply normal OpenAPI and ARM rules. Do not apply this TypeSpec-specific guard.

### 2. Inspect source and output together

Read the changed TypeSpec and generated OpenAPI diff together. Compare:

- Paths and HTTP methods
- Wire parameter names, locations, requiredness, and types
- Request and response bodies
- Status codes and required headers
- Paging and LRO behavior
- Secret handling
- ARM resource identity and lifecycle
- Discriminator and polymorphism behavior

### 3. Classify the change

#### A. Legacy emitter-only client metadata

Examples:

- Remove `x-ms-parameter-grouping` while preserving the same wire parameters.
- Remove `x-ms-client-request-id: true` while preserving the HTTP header.

This is not a Section 26.0 violation by itself. Do not restore the raw decorator or suppression.

If an SDK compatibility check proves a client surface break, address it through semantic TypeSpec modeling or client customization. Do not infer a break from metadata text alone.

#### B. Semantics-bearing metadata

Paging, LROs, secrets, ARM resources, ARM IDs, client names, flattening, enums, and discriminators affect clients or platform behavior. Preserve those semantics through native TypeSpec constructs.

If generated OpenAPI loses required behavior because the TypeSpec model is incomplete, file the finding against that semantic defect and recommend the native construct.

A native replacement can legitimately change generated metadata. For example, when an operation uses `@list` and its response contains `nextLink`, changing `x-ms-pageable.nextLinkName` from `null` to `nextLink` is not automatically a REST breaking change. Validate the semantic paging model.

#### C. REST wire or ARM semantic change

If the cleanup changes a path, method, wire parameter, requiredness, type, body, status code, required header, paging behavior, LRO behavior, secret behavior, or ARM resource semantics, apply normal published-version and breaking-change rules.

### 4. Check downstream evidence

Before retaining an SDK compatibility finding, inspect available SDK breaking-change and required CI results. Require concrete evidence naming the affected client surface. Never use a raw OpenAPI extension to bypass the TypeSpec model.

## Native TypeSpec mappings

| Generated OpenAPI metadata | Native TypeSpec source of truth |
| --- | --- |
| `x-ms-parameter-grouping` | Group parameters in a model and spread it into the operation. |
| `x-ms-client-name` | `@clientName`. |
| `x-ms-client-flatten` | `@flattenProperty` only when compatibility requires it. Do not add flattening to a new API. |
| `x-ms-pageable` | `@list` or an Azure.Core or Azure.ResourceManager paging template. |
| `x-ms-long-running-operation` and options | Azure.Core or Azure.ResourceManager LRO templates with `@pollingOperation` or `@finalOperation` as applicable. |
| `x-ms-secret` | `@secret`. |
| `x-ms-azure-resource` | Azure.ResourceManager resource templates. |
| `x-ms-arm-id-details` | `armResourceIdentifier`. |
| `x-ms-identifiers` | `@key` or `@identifiers`. |

A genuinely emitter-only, non-client-altering custom extension with no native representation may still be necessary. Require a concrete explanation and ask TypeSpec library owners when unclear.

## File-by-file implementation plan

### 1. Canonical shared reference

Create:

`.github/skills/azure-api-review/references/typespec-openapi-extensions.md`

The file must:

- Record current upstream links and alignment date.
- Define `TSP-NO-RAW-CLIENT-EXTENSIONS`.
- Prohibit adding or restoring raw client-altering extensions.
- Prohibit recommending a `no-openapi-client-extensions` suppression.
- Include the native mapping table.
- Define the decision procedure above.
- Preserve Section 26.0 for actual REST and ARM changes.

Register it in `.github/skills/azure-api-review/SKILL.md`. Other consumers should link to it rather than copy the full rule.

### 2. TypeSpec instructions

Update `.github/instructions/typespec-review.instructions.md`:

- Add `TSP-NO-RAW-CLIENT-EXTENSIONS` guidance.
- Prohibit raw extensions and suppressions as compatibility fixes.
- Link array identifier guidance to the shared reference.
- Update conversion guidance to review missing semantics through native TypeSpec constructs, not raw `x-ms-*` text parity.

### 3. ARM instructions

Update Section 26.0 and its checklist entry in `.github/instructions/arm-api-review.instructions.md`.

Before raising an immutability finding for TypeSpec-generated metadata, require ownership proof, semantic source comparison, and evidence of a REST wire, ARM platform, or semantics-bearing change.

Explicitly name removal of `x-ms-parameter-grouping` and `x-ms-client-request-id: true` with an unchanged header as non-violations by themselves.

### 4. Interactive reviewer

Update `.github/agents/arm-api-reviewer.agent.md`:

- Step 2 loads the shared reference for changed TypeSpec or TypeSpec-owned generated OpenAPI.
- Step 3 applies it before retaining an `x-ms-*` compatibility finding.
- Step 4.5 drops metadata-only findings without a wire or ARM semantic change and replaces raw-extension fixes with native TypeSpec constructs.

### 5. Independent critic

Update `.github/agents/arm-api-review-critic.agent.md`:

- Load the shared reference for TypeSpec-generated `x-ms-*` findings.
- Return `FAIL: rule-misapplied` when metadata-only cleanup is treated as Section 26.0 without contract evidence.
- Return `FAIL: downstream-ci-conflict` when a fix restores a raw extension or suppression.
- Add PR #46130 to known false-positive guidance.

### 6. Automated workflow and lock

Update `.github/workflows/arm-api-review.md`:

- Import the shared reference.
- Apply it in Steps 2, 3, and 4.5.

Regenerate `.github/workflows/arm-api-review.lock.yml` with the compiler version recorded in the existing lock header:

```powershell
gh aw --version
gh aw compile arm-api-review --no-check-update
```

Do not hand-edit the lock. Inspect the diff and exclude unrelated compiler, action, container, or telemetry upgrades.

### 7. Deterministic tests

Update `.github/workflows/test/arm-api-review-workflow.test.js` to assert:

- The shared rule ID and upstream suppression prohibition exist.
- The reference names `x-ms-parameter-grouping`, `x-ms-client-request-id: true`, and the `x-ms-pageable` to `@list` mapping.
- TypeSpec and ARM instructions link to the reference.
- Reviewer and workflow drop metadata-only cleanup findings.
- The workflow imports the reference.
- The critic contains both required failure classifications.
- Eval catalog counts are updated.

### 8. Behavioral eval

Update `.github/skills/evals/arm-api-reviewer/vally/eval-typespec.yaml` with stimulus `070006`, `do-not-restore-legacy-openapi-client-extensions`.

Cover:

1. Removal of `x-ms-parameter-grouping` and `x-ms-client-request-id: true` while the header remains unchanged.
2. Replacement of a legacy pageable override with `@list` when the response already has `nextLink`.

The rubric must require no false Section 26.0 finding, no raw extension, no suppression, correct native paging guidance, concrete SDK evidence, and preserved enforcement for real REST or ARM changes.

Update `.github/skills/evals/arm-api-reviewer/README.md` counts and coverage description.

### 9. Repository parity

Mirror the workflow, reference dependencies, and lock file to `Azure/azure-rest-api-specs-pr` when the assignee has access. If private access is unavailable, finish the public implementation and report the mirror as a follow-up rather than blocking or guessing.

## Existing PR comment reconciliation

The comments on PR #46130 were authored under a human account. A telemetry marker does not make a human-owned thread trusted workflow-owned.

On a later review:

- Use `CLARIFY-CONFLICT` for incompatible old and current guidance.
- State the prior recommendation, current upstream evidence, current guidance, and reason for the change.
- Do not post a duplicate standalone finding.
- Do not auto-resolve the human-owned thread.

## Acceptance criteria

- Metadata-only removal of `x-ms-parameter-grouping` or `x-ms-client-request-id: true` is not flagged as Section 26.0 when the wire contract and ARM semantics are unchanged.
- No suggested fix restores raw client-altering `@OpenAPI.extension(...)` or `@extension(...)`.
- No suggested fix suppresses `no-openapi-client-extensions`.
- Semantics-bearing behavior is validated through native TypeSpec constructs.
- Real REST wire and ARM semantic changes in published versions remain findings.
- SDK compatibility concerns require concrete downstream evidence.
- Interactive and automated reviewers apply the same rule.
- The critic rejects both the false classification and prohibited fix.
- Deterministic tests and stimulus `070006` pass.
- Source and lock remain synchronized without unrelated gh-aw upgrade churn.
- Private repository parity is completed or recorded as an explicit follow-up when access is unavailable.

## Validation commands

```powershell
# Compile workflow
gh aw --version
gh aw compile arm-api-review --no-check-update

# Focused deterministic tests
npx vitest run .github/workflows/test/arm-api-review-workflow.test.js

# Full deterministic validation
npm --prefix .github run check

# Install and invoke the repository-pinned Vally CLI
npm --prefix eng/common/scripts/eval ci
Push-Location eng/common/scripts/eval
npm exec -- vally eval -e ../../../../.github/skills/evals/arm-api-reviewer/vally/eval-typespec.yaml --junit
Pop-Location

# Patch hygiene
git diff --check
```

The behavioral eval requires a Copilot CLI identity that can use the configured agent and judge models. If the environment lacks model access, report the infrastructure limitation and retain the deterministic test results. Do not rewrite the eval to a different model solely to make the run pass.

## Limitations

- This is not a waiver for changes to published API versions. REST wire and ARM semantic changes remain subject to normal immutability and versioning rules.
- Metadata-only cleanup can affect an SDK surface. Require concrete evidence rather than assuming either safety or breakage.
- A genuinely emitter-only, non-client-altering extension may still be necessary when no native construct exists. Require a specific explanation and TypeSpec owner input when unclear.
- Native TypeSpec constructs can intentionally change generated metadata. Review the semantic model and generated behavior together.
- Behavioral evals are non-deterministic. Deterministic source-parity tests remain required.