Repository navigation
[Swagger Linter Migration] RepeatedPathInfo -> no-repeated-path-info - #5526
Yuchao Yan (msyyc) wants to merge 16 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c593588a-de3f-4117-8f10-101d86839706
microsoft-github-policy-service
Bot
added
int:azure-specs
|
All changed packages have been documented.
Show changes
|
📦 Package size report✅ No notable package size changes compared to the base branch. 14 package(s) with no notable change
Packed = gzipped |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address template-operation filtering and body model resolution issues before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Promotes RepeatedPathInfo to the ARM no-repeated-path-info native linter rule.
Changes:
- Adds implementation and regression tests.
- Registers the rule disabled by default.
- Updates documentation, catalogs, and release metadata.
| File | Summary |
|---|---|
website/src/content/docs/docs/libraries/azure-resource-manager/reference/linter.md |
Updates linter reference. |
website/src/content/docs/docs/howtos/ARM/arm-rules.md |
Adds ARM guidance entry. |
packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts |
Registers the disabled rule. |
packages/typespec-azure-resource-manager/test/rules/no-repeated-path-info.test.ts |
Adds rule tests. |
packages/typespec-azure-resource-manager/src/rules/no-repeated-path-info.ts |
Implements duplicate path/query detection. |
packages/typespec-azure-resource-manager/src/rules/no-repeated-path-info.md |
Documents rule behavior. |
packages/typespec-azure-resource-manager/src/linter.ts |
Exposes the rule. |
packages/typespec-azure-resource-manager/README.md |
Updates rule catalog. |
.chronus/changes/register-no-repeated-path-info-disabled.md |
Adds registration metadata. |
.chronus/changes/arm-no-repeated-path-info.md |
Adds feature metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
commit: |
|
You can try these changes here
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
There was a problem hiding this comment.
They overlap, but neither replaces the other. arm-resource-duplicate-property compares the properties bag with envelope property names on recognized ARM resource models, independently of operations. no-repeated-path-info compares authored properties-bag names with a concrete PUT operation's HTTP path/query names.
For example, with envelope name annotated @key("widgetName"), properties.widgetName is caught only by the new rule. Conversely, properties.identity duplicates an envelope identity under the existing rule, but not the new rule when there is no URI parameter named identity.
Added mutual documentation links and regression cases in 4bf3599 that enable both rules on the same ARM source, covering their distinct findings and the overlap when names coincide. Both rule implementations and the new rule's default-disabled ARM ruleset registration are unchanged.
There was a problem hiding this comment.
would like Mark Cowlishaw (@markcowl) feedback on that, some of this feels way too similar to me to split into 2 rules. It seems more of an swagger issue trying to be translated in typespec without a real meaning.
There was a problem hiding this comment.
Yuchao Yan (@msyyc) please do not relesolve this comment until mark as given input
There was a problem hiding this comment.
Agreed, I'll leave this discussion open for Mark's input. The checks currently cover different contracts: for example, a PUT query parameter mode duplicated only in properties.mode is not an envelope-property duplicate. Whether to expose that through a separate rule or broaden the existing rule still needs the public-policy decision; I haven't changed either rule's ID or enablement.
There was a problem hiding this comment.
Mark Cowlishaw (@markcowl), could you confirm whether URI-parameter duplication should remain a separate opt-in rule or be added to arm-resource-duplicate-property? A PUT query parameter mode repeated only in properties.mode distinguishes the current URI check from envelope-property duplication. The implementations and enablement are unchanged; this discussion remains open for your policy input.
Document the envelope-versus-URI distinction and cover separate and overlapping diagnostics with both rules enabled. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Apply the test responsibility guidance from #5572; retain distinct predicate, target, and custom guard regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use default ARM create-or-replace parameters and named overrides only when needed. Compile both authored documentation examples with the rule enabled and assert the duplicate's source target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9118d346-bf18-4e38-bf01-0d17dacc093f
microsoft-github-policy-service
Bot
added
the
stale
|
Hi @Yuchao Yan (@msyyc). Your PR has had no update for 14 days and it is marked as a stale PR. If it is not updated within 14 days, the PR will automatically be closed. If you want to refresh the PR, please remove the |
|
Copilot-generated review feedback. Two helper-reuse changes here do not require adding another utility:
For property enumeration, keep using compiler |
Yuchao Yan (msyyc)
removed
the
stale
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Follow-up to this review feedback. Reused Implemented in cb6432bd4. Scoped formatting, type-aware lint, package build and the focused native tests passed. Review threads are intentionally left open. |
Preserve inherited property targeting, wire-name comparisons and per-operation diagnostic counts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Copilot resolve the merge conflicts in this pull request |
…ated-path-info # Conflicts: # website/src/content/docs/docs/howtos/ARM/arm-rules.md Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com>
Resolved the ARM rule inventory conflict, retaining entries from both branches, in merge commit |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Timothee Guerin (timotheeguerin)
left a comment
Timothee Guerin (timotheeguerin)
left a comment
There was a problem hiding this comment.
Waiting on Mark Cowlishaw (@markcowl) input on if this rule has any purpose over the exising no properties envelope duplicate rules
## Summary - Treat a lint brief/PR description as the proposal under review, not a reason to reject clear requests to simplify, revert, or consolidate it. - Compare existing native mechanisms first; distinguish native requirements and released compatibility from legacy parity and complexity introduced by the proposal. - Delete framework-only suppression/enablement tests without replacement while retaining concrete rule-owned interactions, authored-node deduplication, and meaningful regression preconditions. - Remove bespoke per-rule Markdown parsers instead of moving them into another suite. Validate published snippets through existing shared tooling or recorded one-time execution, without claiming continuous coverage from a one-time receipt. - Keep actual policy/authorization gates, source immutability, required validation, and released compatibility intact. Track replies separately from satisfaction of review concerns. ## Evidence and rationale This skill-only change follows a retrospective of recorded migration feedback, not a new live-head review or a change to the underlying rule PRs. - [#5591 simplification direction](#5591 (comment)), [client-setup question](#5591 (comment)), and [model-tree question](#5591 (comment)): demonstrating differences from the current design does not independently establish that its architecture is required. - [#5551 consolidation direction](#5551 (comment)): assess extending the existing native rule and analyze actual enablement/suppression consequences separately. - Explicit requests for another maintainer's input, such as [#5526](#5526 (comment)) and [#5673](#5673 (comment)), remain distinct decision dependencies. The validation skill already discouraged framework-only unit tests, but evidence-preservation wording could encourage relocating them. Promotion still explicitly accommodated per-rule Markdown extraction. These instructions now agree on removing unnecessary infrastructure without discarding rule-owned behavior or honest published-example evidence. ## Scope Only five existing skill Markdown files change: discovery, implementation, validation, development, and promotion. No production rules, package tests, dependencies, submodules, generated artifacts, release entries, or CI configuration change. Two custom review/fix skills stored outside this repository were updated locally in the same task; they are not included in this PR. Base: `feature/lintdiff-migration-new`, as explicitly requested. ## Validation - Existing Prettier 3.9.5: focused Markdown format/check for the changed files. - `git diff --check`. - Checked 78 concrete local Markdown references and heading anchors across the five changed repository skills; template URL placeholders were excluded. - Reviewed the complete task diff and reconciled development/promotion references with the shared validation guidance. The root Prettier configuration could not load the unbuilt `core` TypeSpec plugin. Formatting therefore used the existing Prettier Markdown parser with the root style options and no custom plugins; no dependency installation, submodule initialization, or build was performed for this documentation-only change. Package/source lint and test suites are not applicable under the skill-only lightweight-validation policy. Normal CI is left enabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Test-responsibility cleanup
Applied the guidance merged in #5572 from the #5507 review. Removed the HTTP-diagnostic-only assertion and redundant nested-namespace case. The path/query collision prerequisite is recorded in the mapping below rather than re-tested as a rule predicate. Retained inherited-property targeting, per-PUT multiplicity, imported project targets, non-recursion, and custom template exclusions. No production code changed.
Current cleanup validation:
mise exec -- pnpm --filter @azure-tools/typespec-azure-resource-manager exec vitest run test\rules\no-repeated-path-info.test.tspassed 19 tests in an isolated run with unchanged default timeouts. An earlier concurrent run hit a 10-second setup-hook timeout; its cause is not established. Repositorypnpm format,pnpm lint, andgit diff --checkpassed. Earlier validation counts and broad-validation limitations below remain historical; the full test suite was not rerun.Original Swagger linter
The original ARM guideline is
RPC-Put-V1-05:propertiesschema.How the Swagger linter works
The Spectral function traverses resolved OpenAPI path items, selects PUT, then compares serialized keys of the resource properties bag against the combined URI parameter names. It reports at the matching PUT parameter entry. That serialized-schema diagnostic location is not copied into a native declaration-based rule. Historical representative snapshots and emitted occurrences do not establish equal diagnostic counts. JSON name overrides can introduce or remove a Swagger match independently of the authored TypeSpec member name; this intentional boundary is covered by native tests rather than emitter reconstruction.
Source TypeSpec lintdiff rule
RepeatedPathInfo; local rule:tsp-lintdiff-local-linter/repeated-path-info.repeated-path-info.feature/lintdiff-repeated-path-info, canonicalAzure/typespec-azure.08ea9770a0209d7a131c467ec0fd93f3918b1714from [WIP][Swagger Linter Development] RepeatedPathInfo #5523.C:\dev\worktrees\lintdiff-repeated-path-info; clean, no uncommitted rule changes.The source rule is assumed done for this promotion and was not modified. Source development review completed cleanly at that exact commit before promotion. No development branch merge or blanket cherry-pick is included.
Destination analysis
The agent selected
@azure-tools/typespec-azure-resource-manager, exposing the concise official nameno-repeated-path-info. Catalog metadata says ARM, the validator source is ARM-specific, fixtures use ARM resource envelopes, and the coverage report selects the resource-manager ruleset. The related officialarm-resource-duplicate-propertyrule compares envelope property names rather than HTTP path/query names and does not provide this check.Azure Core was considered because the implementation needs only compiler and HTTP APIs, but this guideline specifically constrains PUT resource properties bags rather than shared data-plane REST authoring. ARM is the better ownership boundary. No dependency is added, and no TCGC, OpenAPI or emitter logic is introduced.
How the promoted TypeSpec linter works
getHttpOperationsupplies native verb, HTTP parameter names and single request-body metadata. Concrete PUT operations with a model body are checked.walkPropertiesInheritedfinds the model-valued authoredpropertiesmember and then its inherited members. AuthoredModelProperty.nameis compared against supported HTTP path/query names; HTTP name overrides are honored, while JSON@encodedNameoverrides intentionally do not change the result.The warning message and property target are preserved. A local set reports each matching name once per operation, so two PUTs sharing a declaration produce two warnings at that declaration. Nested models are not traversed, including cycles and shared siblings. Template sources/instances are filtered as in the source while concrete aliases are checked. Neither a provider namespace guard nor library-declaration filtering was added. Tests retain a minimal ordinary namespace without provider metadata as well as imported project targets, without duplicating framework namespace traversal.
The production decision body is identical to the reviewed source. Adaptations are the public/export names, documentation binding/URL and formatting. Native linting uses the compiler operation graph, not an emitter-selected API-version projection. The source harness's
http-reachablemetadata handles selected-version comparison populations; it is not copied into production. In particular, native names and JSON aliases remain intentionally different in both directions.The new rule is registered as plain
falsein the Azure resource-manager ruleset. Availability does not enable new diagnostics for existing service specs. Chronus records a new ARMfeatureseparately from the rulesets'internaldisabled registration.Fixture-to-native test mapping
Native tests use the ARM tester and
createLinterRuleTester; no snapshots or lintdiff harness files are copied. The original eight main fixtures and both explicit alias inputs map below, followed by native regression coverage.it("reports a path name repeated in ARM PUT resource properties")it("targets the repeated authored property, including inherited properties")it("reports a query name repeated in ARM PUT resource properties")it("reports each repeated ARM path name")it("reports a repeated tenant resource path name")it("allows ARM PUT properties with no repeated names")it("ignores repeated top-level body properties")it("ignores repeated properties in PATCH bodies")it("allows a JSON alias that alone repeats a path name")it("reports an authored name even when its JSON alias differs")it("uses supported HTTP parameter names rather than parameter source identifiers")@typespec/http/incompatible-uri-param; this is an HTTP diagnostic, not a native rule assertion.it("does not recurse into cycles or shared sibling models")it("reports a shared declaration once per PUT operation at the same source target")it("reports project-imported properties at their imported declaration")it("ignores missing bodies, non-model bodies, and missing or non-model properties bags")it("skips template sources but checks their concrete aliases")it("checks ordinary project namespaces without provider metadata")Migration evidence
See the source RepeatedPathInfo migration evidence for focused fixtures, intentional partial JSON-name parity, historical real-service corpus totals, compile failures, project overlap and Confidential Ledger's selected-version population reconciliation. The historical full-corpus observations are not presented as a newly rerun promotion corpus.
Validation
ARM dependency closure built using the prepared checkout/toolchain.
Focused native suite: 19 passed, no skips.
ARM package build/lint and documentation regeneration passed.
Explicit generated README, rule documentation, website linter/rule page and manual ARM matrix formatted and checked using the empty-ignore override.
Rulesets build/lint and 5 tests passed; Chronus status and diff hygiene passed.
Independent final local prepublication review: no significant issues.
Full ARM package: 493 tests across 67 files passed; no skipped tests. The task-local setup profile is retained for this full-scope run.
Validation blocker
The bounded
TYPESPEC_SKIP_WEBSITE_BUILD=true pnpm validate:pr --verboserun passed all 64 repository build tasks and the engineering TypeScript check. It then exited with Windows status-1073741510(0xC000013A) just after printing$ pnpm test, without a test summary or completed validation summary. Elapsed time was about 5 minutes 23 seconds, before the recorded 20-minute overall deadline; the deadline watchdog did not terminate it. Child output is buffered by the validator wrapper, so the precise internal test progress and termination cause are unestablished. This is not a passing broad test/lint/format/docs result, and the command was not retried.All required targeted validations listed above passed, including full ARM 493/493 and rulesets 5/5 with the recorded local ARM setup profile. The owning queue reconciled this as an incomplete optional broad check: all required targeted validation and independent local prepublication review gates passed. This draft is published with the limitation disclosed, not a claim that repository-wide validation is green; CI remains authoritative for broader integration and the website build. No broad rerun or failed-required-check waiver was authorized. The source rule and both worktree identities remain unchanged; the reviewed promotion commit is retained locally.
Local setup profile
The default 10-second
beforeEachlimit initially timed out during shared library initialization, before a lint assertion ran; failures are preserved rather than treated as passing. After explicit authorization, profiling the unchanged ARM tester measured cold setup at 11.13 seconds and cached setup at 0.04 ms. Native-host instrumentation identifies JS imports as the largest setup cost. Local focused/full ARM validation uses--hookTimeout 30000 --maxWorkers 1; the checked-in 10-second per-test timeout, all assertions, test population and skip set are unchanged. No repository timeout configuration, dependency, test-host or production logic was changed to address this environment-specific setup limitation.Promotion sync policy
The lintdiff source remains immutable during promotion. A confirmed source-semantic defect blocks this PR and returns to the owning queue's bounded development/source-repair cycle. This PR is refreshed only after the repaired exact source head passes a fresh development review. Adaptation-only issues belong here; operational failures are not source-repair permission. The queue owns a separate promotion review after publication. Neither PR is automatically merged.
Review cleanup (October 19, 2026)
Moved the URI/envelope interaction cases into
no-repeated-path-info.test.tsand removed the per-rule interaction/isolation files. ThewidgetName/identitycases retain exact targets, including the two-diagnostics-on-one-property case. A focusedtest/package-isolation.test.tschecks the public ARM entry point with throwing AutoRest/TCGC import guards, without another rule fixture. Removed the redundant internal entry.Published in eeb9daa. Fresh focused validation: 19 tests passed, package production typecheck, root
pnpm format, rootpnpm lint, and diff hygiene passed. The full test TypeScript project retains pre-existing diagnostics (25 before, 25 after); the exact-head baseline comparison found no new diagnostics, accounting explicitly for moved tests and any removed duplicate fixture. This is not a claim that the full test TypeScript project is error-free or that pending CI has passed. These current file-layout/coverage statements supersede earlier descriptions of the removed test files; historical validation remains historical.