Skip to content

Extend arm-resource-patch rule with PATCH body schema diagnostics#4381

Draft
Copilot wants to merge 19 commits into
mainfrom
copilot/add-arm-patch-body-schema-rule
Draft

Extend arm-resource-patch rule with PATCH body schema diagnostics#4381
Copilot wants to merge 19 commits into
mainfrom
copilot/add-arm-patch-body-schema-rule

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 5, 2026 •
edited
Loading

  • Add four PATCH body diagnostics (requiredInPatch, defaultInPatch, notUpdateableInPatch, nonMergePatchContentType)
  • Codefixes (make optional, remove default, remove property)
  • Recursive notUpdateableInPatch across nested model + Record<Model> properties with cycle protection
  • hasVisibility(Lifecycle.Update) || isReadOnly(...) predicate
  • Resolve Lifecycle enum members once in create(context)
  • Test for tracked-resource properties field included as optional in PATCH body
  • Switch recursion caches (readOnlyCache, modelHasNonUpdateableCache, inProgressModels) to use fully-qualified names from getTypeName(...) as keys
  • Pull latest from main; all 23 patch-operations tests pass; prettier clean; PR diff limited to in-scope files

@microsoft-github-policy-service microsoft-github-policy-service Bot added lib:azure-resource-manager Issues for @azure-tools/typespec-azure-core library meta:website TypeSpec.io updates labels May 5, 2026
markcowl
markcowl requested changes May 5, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Pull the latest from main, Reformulate ttests for the suggested changes, implement the changes, ensure that all tests pass, all files are formatted correctly and only the changes for this PR are in the PR diff.

Comment thread packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts Outdated
copilot and others added 2 commits May 5, 2026 21:24
Merge remote-tracking branch 'origin/main' into copilot/add-arm-patch…
2a201ba 
…-body-schema-rule

Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
@markcowl
Allow read-only-only properties in PATCH body; merge main; suppress n…
624b73c 
…ew deprecation

Agent-Logs-Url: https://github.com/Azure/typespec-azure/sessions/713242e6-42ef-450f-adbc-fa7ec7dca99c

Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 5, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • iojs.org
    • Triggering command: /usr/bin/curl curl -q --fail --compressed -L -s REDACTED -o - (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from markcowl May 5, 2026 21:34
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 6, 2026 •
edited
Loading

Open in StackBlitz

npm i https://pkg.pr.new/@azure-tools/typespec-azure-resource-manager@4381

commit: c73f36d

markcowl
markcowl requested changes May 6, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Pull the latest from main, make tests for the suggested changes, then imlement the changes. Ensure that all tests pass, that samples rebuild correctly and without error, that all files are correctly formatted and that your branch is up to date with mean and only changes that are part of this PR are in the PR diff.

Comment thread .chronus/changes/arm-patch-body-schema-2026-05-05-02-30-00.md Outdated
Comment thread packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts Outdated
Comment thread packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts Outdated
Comment thread packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts Outdated
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 6, 2026 •
edited
Loading

Pull the latest from main, make tests for the suggested changes, then imlement the changes. Ensure that all tests pass, that samples rebuild correctly and without error, that all files are correctly ...

Branch is up to date with main, all 302 ARM tests pass (including 5 new patch tests covering the recursive check, cyclic graphs, the (Create, Read) combination, and the Lifecycle.Read-only required-property exemption), all 68 sample tests pass after pnpm regen-samples, prettier reports clean, and git diff origin/main only contains the 4 in-scope files (rule, test, docs, changeset).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • iojs.org
    • Triggering command: /usr/bin/curl curl -q --fail --compressed -L -s REDACTED -o - (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@markcowl
Recursive notUpdateableInPatch with per-iteration cycle state; allow …
8c9ed97 
…Read-only required props

Agent-Logs-Url: https://github.com/Azure/typespec-azure/sessions/a4b1fbcd-d639-4dd2-bc82-f1dc449f07e6

Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Copilot AI requested a review from markcowl May 6, 2026 02:56
@markcowl markcowl added the int:azure-specs Run integration tests against azure-rest-api-specs label May 7, 2026
@markcowl
Copy link
Copy Markdown
Member

markcowl commented May 7, 2026

/azp run typespec-azure - PR Tools

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented May 7, 2026

Azure Pipelines successfully started running 1 pipeline(s).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026 •
edited
Loading

⚡ Benchmark Results

⚠️ 16 metric(s) regressed above the +5% threshold:

Metric Baseline Current Change
total 🔴 714.1ms 🔴 787.5ms +10.3% 🔴
loader 🟢 138.4ms 🟢 155.9ms +12.6% 🔴
resolver 🟢 16.1ms 🟢 17.3ms +7.0% 🔴
checker 🟢 167.8ms 🟢 189.8ms +13.1% 🔴
linter 🟢 116.4ms 🟢 134.2ms +15.3% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/no-header-explode 🟡 16.0ms 🟡 19.6ms +22.7% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/no-query-explode 🟡 16.8ms 🔴 20.5ms +22.0% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/no-response-body 🔴 21.4ms 🔴 23.7ms +11.0% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/no-route-parameter-name-mismatch 🟢 4.4ms 🟢 5.9ms +35.6% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/response-schema-problem 🟡 19.0ms 🔴 22.4ms +17.6% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-common-types-version 🟢 3.6ms 🟢 4.7ms +31.9% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-delete-operation-response-codes 🟢 4.6ms 🟢 5.7ms +24.0% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation-response 🟢 4.1ms 🟢 5.3ms +29.1% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/lro-location-header 🟡 12.7ms 🟡 14.7ms +15.9% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/no-response-body 🟡 18.8ms 🔴 22.0ms +16.7% 🔴
 ↳ emit/@azure-tools/typespec-autorest 🟢 144.4ms 🟢 152.0ms +5.3% 🔴
Full details – comparing b140d81 vs baseline a16a6ea
Metric Baseline Current Change
total 🔴 714.1ms 🔴 787.5ms +10.3% 🔴
loader 🟢 138.4ms 🟢 155.9ms +12.6% 🔴
resolver 🟢 16.1ms 🟢 17.3ms +7.0% 🔴
checker 🟢 167.8ms 🟢 189.8ms +13.1% 🔴
validation 🟢 38.9ms 🟢 40.3ms +3.4%
 ↳ validation/@azure-tools/typespec-azure-core 🟢 5.5ms 🟢 6.3ms +14.8%
 ↳ validation/@typespec/http 🟢 5.1ms 🟢 5.7ms +12.7%
 ↳ validation/@typespec/rest 🟢 0.5ms 🟢 0.5ms -11.0%
 ↳ validation/@typespec/versioning 🔴 26.1ms 🔴 25.9ms -0.6%
 ↳ validation/compiler 🟢 1.4ms 🟢 1.4ms -2.2%
linter 🟢 116.4ms 🟢 134.2ms +15.3% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/auth-required 🟢 0.0ms 🟢 0.0ms -3.7%
 ↳ linter/@azure-tools/typespec-azure-core/bad-record-type 🟢 0.2ms 🟢 0.2ms -4.0%
 ↳ linter/@azure-tools/typespec-azure-core/byos 🟢 5.1ms 🟢 5.6ms +8.3%
 ↳ linter/@azure-tools/typespec-azure-core/casing-style 🟢 0.5ms 🟢 0.5ms +2.3%
 ↳ linter/@azure-tools/typespec-azure-core/composition-over-inheritance 🟢 0.1ms 🟢 0.1ms +10.0%
 ↳ linter/@azure-tools/typespec-azure-core/documentation-required 🟢 0.7ms 🟢 0.7ms -6.5%
 ↳ linter/@azure-tools/typespec-azure-core/friendly-name 🟢 0.5ms 🟢 0.5ms +1.6%
 ↳ linter/@azure-tools/typespec-azure-core/key-visibility-required 🟢 0.1ms 🟢 0.1ms +1.5%
 ↳ linter/@azure-tools/typespec-azure-core/known-encoding 🟢 0.2ms 🟢 0.2ms -7.5%
 ↳ linter/@azure-tools/typespec-azure-core/long-running-polling-operation-required 🟢 0.3ms 🟢 0.3ms +4.8%
 ↳ linter/@azure-tools/typespec-azure-core/no-case-mismatch 🟢 0.2ms 🟢 0.2ms +5.6%
 ↳ linter/@azure-tools/typespec-azure-core/no-closed-literal-union 🟢 0.2ms 🟢 0.2ms -0.5%
 ↳ linter/@azure-tools/typespec-azure-core/no-enum 🟢 0.0ms 🟢 0.0ms +7.2%
 ↳ linter/@azure-tools/typespec-azure-core/no-error-status-codes 🟢 0.1ms 🟢 0.1ms +10.3%
 ↳ linter/@azure-tools/typespec-azure-core/no-explicit-routes-resource-ops 🟢 0.1ms 🟢 0.1ms +0.9%
 ↳ linter/@azure-tools/typespec-azure-core/no-format 🟢 0.4ms 🟢 0.4ms -2.4%
 ↳ linter/@azure-tools/typespec-azure-core/no-generic-numeric 🟢 0.4ms 🟢 0.3ms -3.9%
 ↳ linter/@azure-tools/typespec-azure-core/no-header-explode 🟡 16.0ms 🟡 19.6ms +22.7% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/no-legacy-usage 🟢 1.0ms 🟢 1.0ms -3.2%
 ↳ linter/@azure-tools/typespec-azure-core/no-multiple-discriminator 🟢 0.1ms 🟢 0.1ms -9.8%
 ↳ linter/@azure-tools/typespec-azure-core/no-nullable 🟢 0.2ms 🟢 0.2ms -0.5%
 ↳ linter/@azure-tools/typespec-azure-core/no-offsetdatetime 🟢 1.1ms 🟢 1.1ms +0.9%
 ↳ linter/@azure-tools/typespec-azure-core/no-openapi 🟢 1.7ms 🟢 1.7ms +0.3%
 ↳ linter/@azure-tools/typespec-azure-core/no-private-usage 🟢 1.7ms 🟢 1.6ms -4.4%
 ↳ linter/@azure-tools/typespec-azure-core/no-query-explode 🟡 16.8ms 🔴 20.5ms +22.0% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/no-response-body 🔴 21.4ms 🔴 23.7ms +11.0% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/no-rest-library-interfaces 🟢 0.0ms 🟢 0.0ms +4.9%
 ↳ linter/@azure-tools/typespec-azure-core/no-route-parameter-name-mismatch 🟢 4.4ms 🟢 5.9ms +35.6% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/no-rpc-path-params 🟢 0.2ms 🟢 0.2ms +20.1%
 ↳ linter/@azure-tools/typespec-azure-core/no-string-discriminator 🟢 0.0ms 🟢 0.0ms -0.3%
 ↳ linter/@azure-tools/typespec-azure-core/no-unknown 🟢 0.1ms 🟢 0.2ms +8.9%
 ↳ linter/@azure-tools/typespec-azure-core/no-unnamed-union 🟢 0.3ms 🟢 0.3ms -5.1%
 ↳ linter/@azure-tools/typespec-azure-core/operation-missing-api-version 🟢 0.2ms 🟢 0.1ms -10.8%
 ↳ linter/@azure-tools/typespec-azure-core/request-body-problem 🟢 0.2ms 🟢 0.2ms -6.7%
 ↳ linter/@azure-tools/typespec-azure-core/require-versioned 🟢 0.0ms 🟢 0.0ms -12.6%
 ↳ linter/@azure-tools/typespec-azure-core/response-schema-problem 🟡 19.0ms 🔴 22.4ms +17.6% 🔴
 ↳ linter/@azure-tools/typespec-azure-core/rpc-operation-request-body 🟢 0.3ms 🟢 0.3ms +19.0%
 ↳ linter/@azure-tools/typespec-azure-core/spread-discriminated-model 🟢 0.2ms 🟢 0.2ms -6.8%
 ↳ linter/@azure-tools/typespec-azure-core/use-standard-names 🟢 4.2ms 🟢 5.1ms +20.6%
 ↳ linter/@azure-tools/typespec-azure-core/use-standard-operations 🟢 0.1ms 🟢 0.1ms +11.5%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-common-types-version 🟢 3.6ms 🟢 4.7ms +31.9% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-custom-resource-no-key 🟢 0.1ms 🟢 0.1ms +8.1%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-custom-resource-usage-discourage 🟢 0.1ms 🟢 0.1ms +5.7%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-delete-operation-response-codes 🟢 4.6ms 🟢 5.7ms +24.0% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-no-path-casing-conflicts 🟢 4.1ms 🟢 5.0ms +21.2%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-no-record 🟢 0.3ms 🟢 0.3ms -7.4%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-post-operation-response-codes 🟢 0.4ms 🟢 0.5ms +28.3%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-put-operation-response-codes 🟢 0.0ms 🟢 0.0ms -22.7%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-action-no-segment 🟢 0.2ms 🟢 0.2ms +1.8%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property 🟢 0.1ms 🟢 0.1ms +4.5%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-interface-requires-decorator 🟢 0.0ms 🟢 0.0ms +6.2%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-action-verb 🟢 0.1ms 🟢 0.1ms +11.2%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-envelope-property 🟢 0.1ms 🟢 0.1ms +7.5%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-version-format 🟢 0.0ms 🟢 0.0ms -3.8%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-key-invalid-chars 🟢 0.2ms 🟢 0.2ms +18.8%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-name-pattern 🟢 0.0ms 🟢 0.0ms -7.2%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation 🟢 0.1ms 🟢 0.2ms +4.9%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation-response 🟢 4.1ms 🟢 5.3ms +29.1% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-patch 🟢 0.3ms 🟢 0.4ms +46.0%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-path-segment-invalid-chars 🟢 0.2ms 🟢 0.2ms +12.2%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-provisioning-state 🟢 0.1ms 🟢 0.1ms +2.5%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/beyond-nesting-levels 🟢 0.1ms 🟢 0.1ms +7.3%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/empty-updateable-properties 🟢 0.1ms 🟢 0.1ms +17.7%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/improper-subscription-list-operation 🟢 0.0ms 🟢 0.0ms -0.2%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/lro-location-header 🟡 12.7ms 🟡 14.7ms +15.9% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/missing-operations-endpoint 🟢 0.0ms 🟢 0.0ms -18.4%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/missing-x-ms-identifiers 🟢 0.3ms 🟢 0.2ms -7.6%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/no-empty-model 🟢 0.1ms 🟢 0.1ms +9.0%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/no-resource-delete-operation 🟢 0.2ms 🟢 0.2ms +1.5%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/no-response-body 🟡 18.8ms 🔴 22.0ms +16.7% 🔴
 ↳ linter/@azure-tools/typespec-azure-resource-manager/patch-envelope 🟢 0.1ms 🟢 0.1ms +2.2%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/resource-name 🟢 0.1ms 🟢 0.1ms +16.8%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/secret-prop 🟢 2.2ms 🟢 2.0ms -7.8%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/unsupported-type 🟢 0.3ms 🟢 0.4ms +5.7%
 ↳ linter/@azure-tools/typespec-azure-resource-manager/version-progression 🟢 0.0ms 🟢 0.0ms -12.6%
 ↳ linter/@azure-tools/typespec-client-generator-core/property-name-conflict 🟢 0.9ms 🟢 1.0ms +3.7%
 ↳ linter/@azure-tools/typespec-client-generator-core/require-client-suffix 🟢 0.2ms 🟢 0.2ms -2.7%
emit 🟡 232.2ms 🟡 243.2ms +4.7%
 ↳ emit/@azure-tools/typespec-autorest 🟢 144.4ms 🟢 152.0ms +5.3% 🔴
 ↳ emit/@typespec/openapi3 🟢 129.9ms 🟢 134.9ms +3.8%
 ↳ emit/@typespec/openapi3/compute 🟢 115.7ms 🟢 121.0ms +4.6%
 ↳ emit/@typespec/openapi3/write 🟢 13.6ms 🟢 13.9ms +2.6%

Averaged across 3 specs (azure-arm-resource-manager, azure-core-dataplane, azure-full).
Threshold: changes > ±5% are highlighted.
🟢 Fast · 🟡 Moderate (stages >200ms, rules >10ms) · 🔴 Slow (stages >400ms, rules >20ms)

@azure-sdk
Copy link
Copy Markdown
Collaborator

azure-sdk commented May 7, 2026 •
edited
Loading

All changed packages have been documented.

  • ✅ @azure-tools/typespec-azure-resource-manager
Show changes

@azure-tools/typespec-azure-resource-manager - feature ✏️

Extend the arm-resource-patch linter rule to validate PATCH request body schemas. New diagnostics are emitted when:,> ,> - A PATCH request body property is required and not read-only.,> - A PATCH request body property has a default value (defaults are never applied to PATCH requests).,> - A PATCH request body property is not updateable and not read-only.,> - A PATCH operation specifies an explicit content-type other than application/merge-patch+json (or the implicit application/json).,> ,> Codefixes are provided to remove default values, mark properties as optional, and remove non-updateable properties from PATCH bodies.

@azure-sdk
Copy link
Copy Markdown
Collaborator

azure-sdk commented May 7, 2026 •
edited
Loading

You can try these changes here

🛝 Playground 🌐 Website

markcowl
markcowl reviewed May 7, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@markcowl markcowl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@copilot Pull the latest from main, then add some tests for the code fixes. Ensure all tests pass, all files are correctly formatted, samples rebuild cleanly, and only the changes associated withy this pull request are in the PR diff.

@markcowl
Copy link
Copy Markdown
Member

markcowl commented May 8, 2026 •
edited
Loading

@copilot Note that, properties with default visibility have visibility (Lifecycle.Read, Lifecycle.Update, Lifecycle.Create). You seem to be marking these properties as not being updatable.

Please change the way you look for the correct viisibility in a PATCH body - any property that has default visibility or create, update visibility, or create, update, read visibility, or read visibility can be included in a PATCH request.

Update tests to validate this behavior, then implement the behavior, ensure all tests pass, all files are properly formatted, pull the latest from main and ensure that only changes associate with this pull request are in the pr diff.

Copilot AI and others added 2 commits May 13, 2026 00:05
Merge remote-tracking branch 'origin/main' into copilot/add-arm-patch…
e2a8f3b 
…-body-schema-rule

Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
@markcowl
Address review feedback: shorter messages, content-type variable, rem…
8107e3d 
…ove comments, hoist lifecycle setup

Agent-Logs-Url: https://github.com/Azure/typespec-azure/sessions/da5ec04b-586d-41d4-9f2a-a6f06146326d

Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 13, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • iojs.org
    • Triggering command: /usr/bin/curl curl -q --fail --compressed -L -s REDACTED -o - (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from markcowl May 13, 2026 00:15
markcowl
markcowl requested changes May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Pull the latest from main
Make the suggested changes
Ensure all tests pass and all files are correctly formatted
Ensure that the PR diff contains only changes associated with this PR

Comment thread packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts Outdated
for (const property of patchModel) {
if (
!property.optional &&
!isReadOnlyOnly(context.program, property, lifecycleMembers, readOnlyOnlyCache)
Copy link
Copy Markdown
Member

@markcowl markcowl May 13, 2026

Choose a reason for hiding this comment

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

use isReadOnly as the name of this method. isReadOnlyOnly reads as an unnatural repetition of only

Copy link
Copy Markdown
Contributor Author

Copilot AI May 13, 2026

Choose a reason for hiding this comment

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

Renamed to isReadOnly.

Comment thread packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts Outdated
Comment thread packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts Outdated
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 13, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • iojs.org
    • Triggering command: /usr/bin/curl curl -q --fail --compressed -L -s REDACTED -o - (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from markcowl May 13, 2026 02:50
@markcowl
Copy link
Copy Markdown
Member

markcowl commented May 13, 2026

/azp run typespec-azure - PR Tools

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented May 13, 2026

Azure Pipelines successfully started running 1 pipeline(s).

markcowl
markcowl requested changes May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Pull the latest from main, make this change, ensure that all files are properly formatted, all tests pass, and only the changes associated with this PR are in the PR diff.

Comment thread packages/typespec-azure-resource-manager/test/rules/patch-operations.test.ts
});
});

it("emits requiredInPatch diagnostic when a PATCH body property is required", async () => {
Copy link
Copy Markdown
Member

@markcowl markcowl May 13, 2026

Choose a reason for hiding this comment

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

Add a test that verifies that there is no diagnostic when the 'properties' property of a tracked resource is included as an optional property in the patch request body. Make any necessary changes to the implementaion to make this test pass

Copy link
Copy Markdown
Contributor Author

Copilot AI May 13, 2026

Choose a reason for hiding this comment

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

Added a test that verifies no diagnostic is emitted when properties?: FooProperties is included as an optional property in the PATCH body of a tracked resource (with FooProperties containing both a default-visibility property and a @visibility(Lifecycle.Read) property). The existing implementation already handles this correctly — the properties field on TrackedResource has default visibility (so hasVisibility(Update) is true), and the recursive walk into FooProperties finds only updateable or read-only properties — so no implementation change was needed. All 23 patch-operations tests pass.

Copilot AI and others added 2 commits May 13, 2026 17:54
Merge remote-tracking branch 'origin/main' into copilot/add-arm-patch…
e40d942 
…-body-schema-rule

Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
@markcowl
Add test: tracked resource 'properties' as optional in PATCH body is …
938e57c 
…valid

Agent-Logs-Url: https://github.com/Azure/typespec-azure/sessions/f133c0ec-6735-40c7-8b00-4dc5fad8927f

Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 13, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • iojs.org
    • Triggering command: /usr/bin/curl curl -q --fail --compressed -L -s REDACTED -o - (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from markcowl May 13, 2026 17:59
markcowl
markcowl reviewed May 14, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Pull the latest, make these changes. validate that all tests pass and all files are correctly formatted. Ensure that only changes associated with this pr are in the pr diff.

Comment thread packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts Outdated
/** Resolved Lifecycle visibility members; computed once at rule activation. */
lifecycleMembers: LifecycleMembers;
/** Pure per-source-property cache; safe to share across the entire traversal. */
readOnlyCache: Map<ModelProperty, boolean>;
Copy link
Copy Markdown
Member

@markcowl markcowl May 14, 2026

Choose a reason for hiding this comment

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

TypeScript is notoriously bad at hashing types, so I am skeptical this cache will work. If we are going to cahce this, I would expect us to use something like a fully-quqlified name (i.e. <fully-qualified-namespace-name>.<modelName>.<propertyName>) for the cache key (since these names are guaranteed to be unqiue in typespec).

Copy link
Copy Markdown
Contributor Author

Copilot AI May 20, 2026

Choose a reason for hiding this comment

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

Switched the caches and the in-progress set to use fully-qualified names from getTypeName(...) as keys instead of ModelProperty/Model object identity. readOnlyCache is now Map<string, boolean> keyed by the source property's FQN, and modelHasNonUpdateableCache / inProgressModels are keyed by the model's FQN. All 23 patch-operations tests still pass.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 20, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • iojs.org
    • Triggering command: /usr/bin/curl curl -q --fail --compressed -L -s REDACTED -o - (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from markcowl May 20, 2026 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markcowl markcowl Awaiting requested review from markcowl markcowl is a code owner

@bterlson bterlson Awaiting requested review from bterlson bterlson will be requested when the pull request is marked ready for review bterlson is a code owner

@timotheeguerin timotheeguerin Awaiting requested review from timotheeguerin timotheeguerin will be requested when the pull request is marked ready for review timotheeguerin is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@markcowl markcowl

Copilot code review Copilot

Labels

int:azure-specs Run integration tests against azure-rest-api-specs lib:azure-resource-manager Issues for @azure-tools/typespec-azure-core library meta:website TypeSpec.io updates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@markcowl @azure-sdk