Skip to content

[WIP] Skip non-public types from ModelReaderWriterContext - #12047

Draft
Jorge Rangel (jorgerangel-msft) with Copilot wants to merge 10 commits into
mainfrom
copilot/skip-non-public-types-model-reader-writer
Draft

[WIP] Skip non-public types from ModelReaderWriterContext#12047
Jorge Rangel (jorgerangel-msft) with Copilot wants to merge 10 commits into
mainfrom
copilot/skip-non-public-types-model-reader-writer

Conversation

Copilot AI commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Inspect context generation, accessibility handling, and C# test workflow.
  • Add focused regression tests and verify they fail before the fix.
  • Filter non-public buildable types and update affected generated output (five contexts; 79 saved inputs regenerated, three blocked by missing input metadata).
  • Run targeted and package validation, scan changes, and address review feedback (4,424 .NET unit/local tests passed; final ClientModel rerun: 1,765 passed; formatting and secret scan passed; final build/review pending. npm/Spector/Cop checks unavailable; automated review unavailable and CodeQL skipped for database size).

@azure-pipelines

azure-pipelines Bot commented Sep 25, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI linked an issue Sep 25, 2026 that may be closed by this pull request
Skip Non-Public Types from ModelReaderWriterContext #12046
Open
@github-actions github-actions Bot mentioned this pull request Sep 25, 2026
Open
@jorgerangel-msft
fix(http-client-csharp): exclude non-public model context attributes
52ffd9c 
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
@microsoft-github-policy-service microsoft-github-policy-service Bot added the emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp label Sep 25, 2026
@jorgerangel-msft
fix(http-client-csharp): handle nested accessibility and refresh cont…
bc8a988 
…exts

Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
jorgerangel-msft
Jorge Rangel (jorgerangel-msft) reviewed Sep 25, 2026
View reviewed changes

@jorgerangel-msft Jorge Rangel (jorgerangel-msft) 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.

Copilot address the feedback I left

@jorgerangel-msft
refactor(http-client-csharp): check public API before building contex…
34d2609 
…t attributes

Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
@pkg-pr-new

pkg-pr-new Bot commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@typespec/http-client-csharp@12047

commit: 9c346f0

Copilot AI reviewed Sep 25, 2026
View reviewed changes

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.

Copilot review overview

🔵 Needs a closer look

The implementation appears coherent, but the PR remains WIP with final build, Cop, and Spector validation incomplete.

Review effort: Balanced
Findings: None

What changed in this PR

Filters non-public C# types from generated ModelReaderWriterContext buildable attributes, including nested and restored contract types.

Changes:

  • Adds accessibility checks across declaring-type chains.
  • Filters framework, generated, customized, and restored types.
  • Adds regression coverage and updates generated contexts.
File Description
_TypePropertyAdditionalPropertiesContext.cs Removes internal discriminator types.
_TypeModelInheritanceSingleDiscriminatorContext.cs Removes internal fallback models.
_TypeModelInheritanceNestedDiscriminatorContext.cs Removes internal nested-discriminator models.
_TypeModelInheritanceEnumDiscriminatorContext.cs Removes internal enum-discriminator models.
SampleTypeSpecContext.cs Removes internal paging and fallback models.
MethodSignatureHelperTests.cs Tests type and declaring-type accessibility.
MethodSignatureHelper.cs Adds recursive public-type detection.
NonPublicTypesAreNotRestoredFromLastContract/​SampleContext.cs Provides previous-contract test data.
NonPublicTypesAreNotRestoredFromLastContract(Custom)/​Models.cs Provides customized accessibility cases.
BuildableAttributesRespectCustomizedAccessibility/​Models.cs Tests customized model visibility.
ModelReaderWriterContextDefinitionTests.cs Adds accessibility regression coverage.
ModelReaderWriterContextDefinition.cs Filters inaccessible buildable attribute targets.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

jorgerangel-msft
Jorge Rangel (jorgerangel-msft) reviewed Sep 25, 2026
View reviewed changes
Comment thread ...Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ModelReaderWriterContextDefinition.cs Outdated
Comment thread ...Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ModelReaderWriterContextDefinition.cs Outdated
Comment thread ...Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ModelReaderWriterContextDefinition.cs Outdated
Comment thread ....Generator.ClientModel/test/Providers/Definitions/ModelReaderWriterContextDefinitionTests.cs Outdated
Comment thread ...ttp-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Shared/MethodSignatureHelper.cs Outdated
Copilot AI and others added 2 commits September 25, 2026 15:57
@jorgerangel-msft
refactor(http-client-csharp): address context visibility review
a04bb9c 
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
@jorgerangel-msft
fix(http-client-csharp): preserve nested visibility filtering
11b7a5b 
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>

Copilot AI commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Copilot address the feedback I left

Addressed in a04bb9c and 11b7a5b. I updated the visibility checks, removed the helper overload, and switched the output validation test to TestData.

Copilot AI reviewed Sep 25, 2026
View reviewed changes

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.

Copilot review overview

🔵 Needs a closer look

The accessibility logic appears sound, but the PR remains WIP with final build and package validation still pending.

Review effort: Balanced
Findings: None

@jorgerangel-msft
refactor(http-client-csharp): simplify provider visibility check and …
086890f 
…revert last-contract filtering

Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
@jorgerangel-msft
test(http-client-csharp): validate accessibility tests against TestDa…
9ef6bef 
…ta baselines

Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
@jorgerangel-msft
fix(http-client-csharp): treat protected types as public API in conte…
50dd958 
…xt attributes

Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
@jorgerangel-msft
refactor(http-client-csharp): extract IsPublicApi private helper
9c346f0 
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Copilot AI reviewed Sep 25, 2026
View reviewed changes

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.

Copilot review overview

🟡 Changes recommended

The back-compatibility restoration path can re-add buildable attributes for internal types.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

Comment thread ...Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ModelReaderWriterContextDefinition.cs

This branch has not been deployed

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

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jorgerangel-msft Jorge Rangel (jorgerangel-msft) Awaiting requested review from jorgerangel-msft jorgerangel-msft is a code owner
@m-nash m-nash Awaiting requested review from m-nash m-nash will be requested when the pull request is marked ready for review m-nash is a code owner
@JoshLove-msft JoshLove-msft Awaiting requested review from JoshLove-msft JoshLove-msft will be requested when the pull request is marked ready for review JoshLove-msft is a code owner
@jsquire Jesse Squire (jsquire) Awaiting requested review from jsquire jsquire will be requested when the pull request is marked ready for review jsquire is a code owner
@joseharriaga Jose Arriaga Maldonado (joseharriaga) Awaiting requested review from joseharriaga joseharriaga will be requested when the pull request is marked ready for review joseharriaga is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@jorgerangel-msft Jorge Rangel (jorgerangel-msft)

Copilot code review Copilot

Labels

emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Skip Non-Public Types from ModelReaderWriterContext

3 participants

@jorgerangel-msft