Skip to content

[Swagger Linter Migration] LroErrorContent (origin) - #5440

Draft
Yuchao Yan (msyyc) wants to merge 5 commits into
feature/lintdiff-migration-newfrom
feature/repair-lintdiff-lro-error-content
Draft

[Swagger Linter Migration] LroErrorContent (origin)#5440
Yuchao Yan (msyyc) wants to merge 5 commits into
feature/lintdiff-migration-newfrom
feature/repair-lintdiff-lro-error-content

Conversation

@msyyc

Copy link
Copy Markdown
Member

Native-rule repair

Follow-up to #5425. The original implementation used TCGC, @typespec/openapi helpers, and unsafe compiler mutation. Those dependencies are inappropriate for the intended ARM destination, which TCGC itself depends on. This repair removes all three from the production rule. The development-skill restrictions are isolated in #5438; this PR does not promote the rule into the official ARM package.

The selected native contract requires standard error payloads even when Swagger would inline them. This is deliberately partial Swagger coverage, not functional equivalence.

Original Swagger linter

The original rule:

  • Traverses paths and x-ms-paths without resolving references.
  • Selects operations whose x-ms-long-running-operation is exactly true.
  • Inspects existing schema.$ref values on default, 4xx, and 5xx responses.
  • Requires the ARM common-types v2-or-later ErrorResponse reference pattern.
  • Reports at each invalid reference; does not require bodies, inspect success responses, or reject inline schemas.

The reference regex is unanchored and has an unescaped dot in types.json; those quirks are retained for native reference metadata. One authored operation can produce several findings through multiple error statuses or scope expansions.

How the native TypeSpec rule works

The rule traverses native HTTP endpoints, selects non-GET LROs using Azure Core getLroMetadata, and examines every existing error payload with HTTP read-visibility metadata. ARM common-type and native external-reference APIs identify standard errors; model is copies and nullable standard errors are accepted. Inline, primitive, collection, binary, and multipart custom payloads are no longer exempt.

Diagnostics are deduplicated by authored operation node across response statuses, nested services, and shared template instantiations. The mixed lintdiff runner retains an ARM service isolation guard; official ARM promotion must adapt that applicability boundary.

The rule reads the authored program without unsafe historical snapshots. TCGC legacy markers, SDK scope, AutoRest reference overrides, and raw OpenAPI LRO extensions do not control native validation. For example, a false extension cannot disable a native ARM async operation, and a legacy-marked GET is outside the native selector.

Migration evidence

Full migration evidence records native regression tests, six focused comparison fixtures, pinned full-corpus results, every unequal-project cause, selected-version attribution, compile failures, and explicit limitations.

Rule contract and emission matrix documents the stronger native policy and emitter-only divergences. Generated corpus files are intentionally excluded from this PR.

Yuchao Yan (msyyc) and others added 2 commits September 9, 2026 18:19
Remove TCGC, OpenAPI helper, and unsafe snapshot dependencies; require standard native error payloads and document partial Swagger coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new dependency-boundary unit test uses an overly broad source substring match that is brittle and should be tightened to avoid false failures from harmless text/identifier changes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR repairs the LroErrorContent migrated lint rule to be ARM-destination compatible by removing TCGC and @typespec/openapi dependencies and eliminating unsafe compiler mutation, while intentionally enforcing a stronger native contract (standard ARM error payloads) rather than full Swagger equivalence.

Changes:

  • Reworked the production lro-error-content rule to use native HTTP/LRO metadata and ARM common-type/external-reference metadata, without TCGC/OpenAPI helpers or unsafe projection.
  • Expanded/updated unit tests and fixture snapshots to reflect the repaired native behavior (including stricter handling of inline/binary/multipart/custom payloads and operation-level deduplication).
  • Updated rule documentation and migration evidence to clearly state the repaired native contract and explicit partial-coverage boundaries.
File summaries
File Description
packages/typespec-lintdiff/src/rules/lro-error-content.ts Repaired production rule logic to be native-only (no TCGC/OpenAPI/unsafe mutation), using LRO + payload metadata and ARM reference metadata.
packages/typespec-lintdiff/test/rules/lro-error-content.test.ts Updated tests to enforce dependency boundaries and validate the repaired native contract across many payload shapes and traversal/dedup cases.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/template-and-versions/tsp-diagnostics.json Updated expected diagnostic message wording to match “payload” semantics.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/standard-error/validator-diagnostics.json Added validator snapshot for the new compliant control fixture.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/standard-error/tsp-diagnostics.json Added TypeSpec diagnostics snapshot for the new compliant control fixture.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/standard-error/output.json Added emitted Swagger output snapshot for the new compliant control fixture.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/standard-error/main.tsp Added a new compliant control fixture using native ARM templates and standard error patterns.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/standard-error/expect.json Added reviewed ambient-diagnostics expectations for the new control fixture.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/rule.md Updated contract, applicability, partial-coverage boundaries, and emission matrix to reflect repaired native policy.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/reference-shapes/tsp-diagnostics.json Updated expected diagnostics (message wording + additional ambient diagnostics).
packages/typespec-lintdiff/test/fixtures/LroErrorContent/reference-shapes/output.json Updated emitted Swagger snapshot for new polling-operation modeling and related output changes.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/reference-shapes/main.tsp Updated fixture to use polling-operation metadata and updated accepted response model shape.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/non-standard-error/validator-diagnostics.json Updated validator snapshot to include the new native-template case.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/non-standard-error/tsp-diagnostics.json Updated TypeSpec diagnostics snapshot (message wording + additional ambient diagnostics).
packages/typespec-lintdiff/test/fixtures/LroErrorContent/non-standard-error/output.json Updated emitted Swagger snapshot with the new native-template LRO operation.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/non-standard-error/main.tsp Added a new unsuppressed native-template case to demonstrate the actionable payload gap.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/migration.md Rewrote migration evidence summary to reflect repaired results, new boundaries, and updated reconciliation narrative.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/inline-and-standard/tsp-diagnostics.json Updated fixture expectations to reflect the stronger native payload policy (now includes many lro-error-content diagnostics).
packages/typespec-lintdiff/test/fixtures/LroErrorContent/inline-and-standard/output.json Updated emitted Swagger snapshot for polling-operation modeling and related output changes.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/inline-and-standard/main.tsp Updated fixture to use polling-operation metadata and accepted response model shape.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/inline-and-standard/expect.json Updated fixture expectation to mark it as a violation fixture under the repaired native contract.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/external-references/tsp-diagnostics.json Updated expected diagnostics (message wording + additional ambient diagnostics).
packages/typespec-lintdiff/test/fixtures/LroErrorContent/external-references/output.json Updated emitted Swagger snapshot for polling-operation modeling and related output changes.
packages/typespec-lintdiff/test/fixtures/LroErrorContent/external-references/main.tsp Updated fixture to use polling-operation metadata and accepted response model shape.
Review details
  • Files reviewed: 24/24 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/typespec-lintdiff/test/rules/lro-error-content.test.ts Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Yuchao Yan (msyyc) added a commit that referenced this pull request Sep 10, 2026
…5448)

## Summary

- Apply the existing lintdiff targeted formatting/linting procedure to
tests-only fixes as well as production-rule fixes; keep corpus
requirements tied to production-rule changes and preserve promotion
validation.
- Record planned command scope, required/supplemental status, exact
commands, exit codes, and durable evidence. Failed commands remain
blockers rather than being silently replaced by narrower passes.
- Split fixes into preparation and publication: the same persistent fix
agent must return the proposed diff and evidence, then wait for parent
approval bound to the head and content before staging, committing, or
pushing. Apply this to backlog fixes and every counted round.

## Evidence

During #5440, a tests-only Copilot finding led the fix agent to run
package-wide lint, which exited 1, then continue to commit/push after
focused lint passed. Follow-up inspection found 228 package lint
warnings across 11 files, none changed by that PR.
`develop-lintdiff-rule` already prescribes changed-file lint, but the
loop skill explicitly linked that procedure only to production-source
changes. The missing pre-publication handoff also meant the parent
discovered the failure only after the push.

This PR clarifies the validation scope without weakening the failure
stop condition and adds an explicit agent-to-agent publication gate. It
does not require another human approval prompt.

## Scope and validation

Skill-only change based on `feature/lintdiff-migration-new`; head is
`msyyc:feature/review-fix-validation-gate`. No rule code, fixtures,
dependency files, or #5440 commits are included.

- Formatted the changed Markdown with the existing Prettier command.
- Reviewed the complete diff, Markdown reference, backlog/round flow,
and failure safeguards.
- `git diff --check` and staged diff checks passed.
- No builds, source tests, corpus runs, or CI waiting for this
documentation-only change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The remaining issues are minor but actionable documentation portability problems (hard-coded local C:\... paths) that should be corrected before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

packages/typespec-lintdiff/test/fixtures/LroErrorContent/migration.md:469

  • This command example includes a contributor-specific absolute C:\... path. Using a placeholder keeps the guidance usable across environments while still showing the required --specs-repo argument.
mise exec -- pnpm --dir packages\typespec-lintdiff specs:typespec `
  --specs-repo C:\dev\worktrees\azure-rest-api-specs-lintdiff-lro-error-content `
  --concurrency 6
  • Files reviewed: 24/24 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/typespec-lintdiff/test/fixtures/LroErrorContent/migration.md Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The dependency-boundary unit test can miss forbidden dependencies introduced via export ... from "..." statements, weakening the intended enforcement.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

packages/typespec-lintdiff/test/rules/lro-error-content.test.ts:65

  • The dependency-boundary assertion only inspects import ... from "..." declarations; it won’t fail if a forbidden package is introduced via re-exports like export * from "@typespec/openapi" (which still creates a module dependency). Consider collecting module specifiers from any statement that has a moduleSpecifier (imports + export-from) so the test actually enforces “no dependency on these modules”.
  • Files reviewed: 24/24 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants