diff --git a/packages/typespec-lintdiff/docs/real-service-equivalence-check.md b/packages/typespec-lintdiff/docs/real-service-equivalence-check.md new file mode 100644 index 0000000000..d7730281d2 --- /dev/null +++ b/packages/typespec-lintdiff/docs/real-service-equivalence-check.md @@ -0,0 +1,165 @@ +# Real-service lint equivalence check + +A reusable procedure for answering the question the migration depends on: + +> **Does the TypeSpec lint rule report the same thing as the Swagger LintDiff rule, +> on a real Azure service?** + +The existing evidence in this package does not answer that: + +- **Fixtures** (`test/fixtures/`) are hand-authored and synthetic — every + `main.tsp` uses `Microsoft.TestService` / `TestService`, none is derived from a + real spec. `output.json`, `tsp-diagnostics.json` and + `validator-diagnostics.json` are snapshots regenerated from the rule's own + behaviour (`validate.ts`, `--update-snapshots`), so they record what the rule + already does, not what it should do. +- **The cross-repo run** (`cross-repo-compare.ts` → `coverage-breakdown.py`) + reduces both sides to a per-project boolean — "did code X and code Y both + appear somewhere in this project". Counts, locations and identity are + discarded, so high parity percentages can coexist with the two rules matching + entirely different things. + +This procedure compares the two toolchains on one real service, using the +**production** Swagger command and the **production** TypeSpec ruleset. + +## Procedure + +Prerequisites: a clone of `Azure/azure-rest-api-specs` with `npm ci` completed +(this provides both `autorest` and the official `@azure-tools/typespec-*` +packages at the versions the repo actually gates on). + +### 1. Pick a TypeSpec-authored ARM service + +A project directory containing `tspconfig.yaml` + `main.tsp` whose config +extends `@azure-tools/typespec-azure-rulesets/resource-manager`, and whose +emitted swagger is checked in. At the time of writing there are **467** such +projects. + +```powershell +Get-ChildItem specification -Recurse -Filter tspconfig.yaml -File | + Where-Object { + (Test-Path (Join-Path $_.DirectoryName 'main.tsp')) -and + ((Get-Content $_.FullName -Raw) -match 'typespec-azure-rulesets/resource-manager') + } +``` + +### 2. Run the Swagger linter exactly as CI does + +Mirror `eng/tools/lint-diff/src/runChecks.ts`. Do **not** hand-roll a Spectral +invocation — see [Pitfalls](#pitfalls-do-not-hand-roll-the-swagger-side). + +```powershell +$dep = Resolve-Path node_modules\@microsoft.azure\openapi-validator +npm exec -- autorest --v3 --spectral --azure-validator ` + --semantic-validator=false --model-validator=false --message-format=json ` + --openapi-type=arm --openapi-subtype=arm --use=$dep ` + --tag= > autorest.txt +``` + +Each stdout line is a JSON message; violations are the ones with +`level` of `warning` / `error` / `fatal`, carrying `code` and +`details.jsonpath`. + +```js +const objs = fs.readFileSync("autorest.txt", "utf8").split(/\r?\n/) + .filter((l) => l.trim().startsWith("{")) + .map((l) => { try { return JSON.parse(l); } catch { return null; } }) + .filter(Boolean); +const violations = objs.filter((o) => ["warning", "error", "fatal"].includes(o.level)); +``` + +### 3. Run the TypeSpec linter on the same project + +```powershell +cd +node_modules\.bin\tsp.cmd compile . --no-emit --pretty false +``` + +The linter configuration comes from the project's own `tspconfig.yaml`, so this +is the ruleset the service is actually gated on today. + +### 4. Compare + +Compare **counts per rule** and, where the rule shape allows, **locations**: the +validator's `details.jsonpath` (`["definitions","Widget","properties","flag"]`) +and the TypeSpec diagnostic target both reduce to `(model, property)`. Report the +**symmetric difference**, not a percentage — a diff is actionable, "99.6%" is not. + +## Worked example: Microsoft.Fabric + +`specification/fabric/resource-manager/Microsoft.Fabric/Fabric`, tag +`package-2023-11-01`, emitted swagger `stable/2023-11-01/fabric.json`. +Fully TypeSpec-authored. + +**Swagger side — 53 violations across 3 rules (5 errors, 48 warnings):** + +| Count | Level | Rule | `catalog.json` tier | Mapped TypeSpec lint | +|---:|---|---|---|---| +| 48 | warning | `LatestVersionOfCommonTypesMustBeUsed` | Unconstrained | `tsp-lintdiff-local-linter/latest-version-of-common-types-must-be-used` (`coverageKind: lint`) | +| 3 | error | `PatchBodyParametersSchema` | Unconstrained | `tsp-lintdiff-local-linter/patch-body-parameters-schema` (`coverageKind: partial`) | +| 2 | error | `RequiredPropertiesMissingInResourceModel` | **Template-enforced** | `tspLints: []` | + +**TypeSpec side — 0 diagnostics.** `Compilation completed successfully.` + +### What that shows + +1. **A "Template-enforced" rule fires on real TypeSpec output.** + `RequiredPropertiesMissingInResourceModel` is classified as needing no work, + rationale *"ARM library base models (TrackedResource/ProxyResource) provide + name, id, type as readonly"* — yet it reports 2 errors on + `RpSkuEnumerationForNewResourceResult`, and has no mapped TypeSpec lint. The + tier values in `catalog.json` come from the hand-written + `test/harness/triage-data.ts`; this is a counter-example found on the first + service tried, so the 43-rule Template-enforced tier should be spot-checked + against real services before it is used to remove rules from scope. + +2. **Migrated ≠ enforced.** `LatestVersionOfCommonTypesMustBeUsed` *has* been + migrated, but into `tsp-lintdiff-local-linter`, which no spec repo + references — so it catches 0 of the 48 real findings. + +## Pitfalls: do not hand-roll the Swagger side + +Linting the same file three ways gives three different answers: + +| Configuration | Violations | Distinct rules | Errors | +|---|---:|---:|---:| +| Production `autorest` (step 2 above) | **53** | **3** | 5 | +| Spectral directly, `$ref`s resolved | 82 | 13 | 7 | +| Spectral directly, `$ref`s unresolved | **202** | **23** | 49 | + +The third row is what `cross-repo-compare.ts` currently does. Three causes: + +- **No `$ref` resolution.** `runSpectralRules()` calls `linter.run(swagger)` on a + plain parsed object with no document source, so external `$ref`s into + common-types never resolve. Rules that inspect referenced content then fire on + `{ $ref: … }` placeholders. For this one spec that manufactures ~150 phantom + violations, including every `invalid-ref`, `ApiVersionParameterRequired`, + `NamePropertyDefinitionInParameter`, `ParameterDescription` and `VersionPolicy` + finding — the same rules that show implausible ~445/450 firing rates in the + coverage report. To resolve, construct a `Document` with a source path and pass + `httpAndFileResolver`. +- **Wrong ruleset scope.** All three rulesets are loaded — `azARM` (81 rules), + `azCommon` (38) and `azDataplane` (37) — regardless of service type; the + `serviceType` argument is ignored on the spectral path. Data-plane rules are + therefore applied to ARM specs. +- **Suppressions ignored.** readme-level `suppressions:` are not honoured. + Fabric suppresses `PostResponseCodes`; production reports none, the harness + reports two. + +Consequence: `Fired` counts in the coverage report are measured against a +validator configuration that differs materially from the one gating the specs +repo, so both the parity percentages and the needs-migration classification that +derives from them should be regenerated with the production command. + +## Suggested follow-ups + +- Run this across ~10–20 services to establish whether "validator N, TypeSpec 0" + is systematic or specific to Fabric. +- Switch the harness's validator side to the production `autorest` invocation, or + at minimum fix resolution, ruleset scoping and suppressions. +- Add a per-rule equivalence mode that reports the symmetric difference of + `(model, property)` locations rather than a project-level boolean. +- Seed fixtures from real specs: for each rule, take a project where the + validator fires, extract the minimal TypeSpec that reproduces it, and commit + that as the fixture — so the offline corpus and the real-world corpus test the + same thing. diff --git a/packages/typespec-lintdiff/docs/review.md b/packages/typespec-lintdiff/docs/review.md new file mode 100644 index 0000000000..c6371da6c5 --- /dev/null +++ b/packages/typespec-lintdiff/docs/review.md @@ -0,0 +1,73 @@ +# LintDiff → TypeSpec migration review + +Review of the "Migrate Swagger LintDiff to TypeSpec lints" plan, based on the +`feature/lintdiff-migration` branch of `Azure/typespec-azure` and the ARM coverage +gist https://gist.github.com/catalinaperalta/b2e7d29a33b4b451bcfcc87e8314565a and migration plan doc https://microsoft-my.sharepoint.com/:w:/p/caperal/cQo1-pAoj7UDS5x_eJ4wBOSKEgUCKPmLoi5C5qsjeXpQhQYjdg. + +## Proposals + +### Prove parity by real-service equivalence, not co-occurrence (High) + +I understand aach migrated TypeSpec lint shall be equivalent to its Swagger LintDiff rule as much as possible. +Project-level co-occurrence does not prove that both rules detect the same +violations. Run both tools on generated Swagger and TypeSpec from a set of real +services, then compare rule counts and diagnostic locations. + +## Questions + +### 1 Who reviews a migrated rule, and what does it cost? (High) + +The DoD says only "Reviewer approval obtained" and prices zero days. + +- Which parties are required sign-off — TypeSpec/compiler owners, the ARM + reviewer board, specs-repo tooling owners? Two or three groups per rule makes + review a first-class cost line. +- ARM reviewer capacity is shared with spec reviews: 147 rules × 2–3 rounds may + dominate wall-clock even if dev-days hold. +- How many rounds are assumed, at what latency? Can rules be reviewed in batches? + +### 2 Rollout cost in the spec repos is unpriced (High) + +The report stops at "added to ruleset" and never covers getting rules enforced. + +- **Public specs repo**: 467 ARM projects today (450 at gist time) to triage, + suppress-or-fix and sequence. +- **Private specs repo**: the same again. +- Is adoption in scope for this programme, or a separate budgeted workstream? + +### Other questions about some details in report and migration plan + +3. **Severity mapping and suppression policy** — LintDiff `error` maps to which + TypeSpec severity, and should the rollout preserve existing suppressions? + +4. **One rule denominator** — the report says 180 ARM/Common rules; + `catalog.json` says 209 (172 ARM+common); the coverage gist says 210. The + three numbers cannot be reconciled from any published artifact, and the work + breakdown (33 infallible, 86 migrated, 61 pending) is derived from the first + of them. + + - Which denominator is authoritative, and at which commit? + - How do the 33/86/61 buckets map onto `catalog.json`'s tiers, which also + carry a `Template-enforced` tier (43 ARM/Both rules) that the report never + mentions? + +5. **Where does the "450 compiled projects" denominator come from?** 450 = ARM + TypeSpec projects (`tspconfig.yaml` + `main.tsp` under `specification/**`) + that compiled successfully; `Fired` is a per-project count, max 450. But the + gist records no specs commit, timestamp, project list or compile-failure + count, and the underlying report JSON was never published, so no row is + auditable. The denominator also drifts — 467 ARM projects locally today. + + - Which specs commit produced the 450? + - How many projects were skipped or failed to compile, and does excluding + them bias any rule's parity? + - Can `cross-repo-comparison.json` be published with the gist? + - What is the refresh cadence, and is the denominator re-baselined each run? + +6. **35 rules never fired** across 450 projects. Retire or defer them with a + documented rationale instead of migrating them? The report's per-rule + estimates imply approximately 30–44 dev-days at stake. + +7. **Artifact hygiene** — absolute personal paths in `catalog.json` + (`/Users/wtemple/...`), unnormalized severities (`error`/`warn`/`warning`), + `validate-report.md` dated 2026-05-04 claiming 254 cases when 431 exist.