Skip to content

[Epic] C# ARM SDK readiness before spec merge #17269

Open
0 of 3 issues completed
Open

Description

Summary

Add a process that enforces that C# (.NET) names have been reviewed and validated before a TypeSpec spec PR is merged.

Background

The current C# loop is:

  1. The spec PR is merged.
  2. The merge creates an auto-PR for C#.
  3. That PR is fixed for various CI issues, and an APIView is created.
  4. The APIView is reviewed and may produce name feedback.
  5. Addressing that feedback requires a new spec PR and a whole new loop.

Step 5 is highly inefficient: name changes discovered late force a full re-cycle (spec change → merge → auto-PR → CI fixes → APIView review).

Goal

Ensure C# names are reviewed and approved before the spec is merged, so the post-merge loop does not produce name-driven rework.

Important consideration

The names in the TypeSpec and the final names produced by the C# emitter may not be the same. Validating only the names in the TypeSpec may therefore not be sufficient. This needs to be confirmed with an expert (e.g. JoshLove-msft ) to determine what must be checked (TypeSpec names, emitter output, or both).

Initial idea

A dotnet-name-approval-required process (e.g. a label/check) applied to all spec PRs, gating merge until C# names are approved.

Scope of this epic

This issue is an epic to design and agree on a solution. It is not yet a committed implementation.

Open questions

  • What exactly needs to be validated: TypeSpec names, emitter-generated names, or both? (confirm with Josh Love)
  • How can emitter-generated C# names be previewed at spec-PR time (e.g. run the emitter in the PR, generate an APIView/name report)?
  • Who approves C# names, and how is approval recorded (label, required check, review)?
  • Should the process apply to all spec PRs, or only those that add/change public-facing names?
  • How are approvals invalidated when names change after approval?
  • Is dotnet-name-approval-required the right mechanism (label + required check), or is there a better alternative?

Acceptance criteria

  • Design agreed with .NET SDK experts and spec/tooling owners
  • Follow-up implementation issues created and linked to this epic

Confirmed direction

The naming skill supports authors while they draft TypeSpec and client.tsp changes. The TypeSpec assessment uses the same shared naming guidance in read-only mode.

For the spec PR experience, the assessment will report C# naming findings in a managed PR comment. When actionable findings require human review, the workflow will apply an approval-required label and a required check will prevent merge until an authorized approval is recorded. Deterministic TypeSpec linter rules continue to enforce the cases they already cover.

The assessment and naming skill are nondeterministic, so the first rollout must be report-only. The assessment should route findings to review rather than independently decide that a name is invalid.

Implementation tracking

Remaining coordination

  • Establish priority and ETA for the assessment naming integration and remote spec PR experience.
  • Agree on the final label names, authorized approver source, and which finding severities require review.
  • Define report-only exit criteria before enabling the required merge check.

Activity

  1. added
    needs-triageWorkflow: This is a new issue that needs to be triaged to the appropriate team.
    on Oct 14, 2026
  2. samvaity commented on Oct 19, 2026

    @samvaity
    Member

    Based on the direction discussed, here is a possible first pass for this epic:

    1. Inventory the existing C# naming guidance across TypeSpec rules and .NET analyzers.
    2. Identify the small set of deterministic rules we can run during TypeSpec authoring and spec PR validation.
    3. Use the existing SDK Validation generation and build flow to validate the effective generated C# names.
    4. Report only new naming findings introduced by the spec PR. API Review Hub would continue to provide the complete API diff and review experience.
    5. Use the naming skill for cases where we can detect a concern but need context to choose the correct name.

    We can start in report-only mode and use the results to decide which rules should block. For findings that still need C# architect review, we should agree whether that is represented by a label, required check, or another review mechanism.

    JoshLove-msft m-nash, would appreciate your eyes on whether this is the right initial scope and which deterministic rules should be included first.

  3. changed the title [-][Epic] Enforce C# name validation before TypeSpec spec PR is merged[/-] [+][Epic] C# ARM SDK readiness before spec merge[/+] on Oct 20, 2026
  4. samvaity commented on Oct 20, 2026

    @samvaity
    Member

    Following up after discussing this with JoshLove-msft : this is still the same direction, with the TypeSpec assessment as the review component.

    • The naming skill helps authors draft the C# naming and client.tsp changes before merge.
    • The assessment uses the same naming guidance in read-only mode and reports findings on the spec PR.
    • If the assessment reports actionable naming findings, the workflow requires C# naming approval and CI blocks merge until that approval is recorded.
    • Existing deterministic TypeSpec rules continue to enforce the cases they already cover.

    Because the assessment is nondeterministic, the initial rollout should be report-only and route findings to reviewers rather than independently reject a name.

    The implementation gaps are now tracked in #17336, #17337, and #17338, with #17297 covering the assessment naming integration.

  5. haolingdong-msft commented on Oct 21, 2026

    @haolingdong-msft
    Member

    Thanks Sameeksha Vaity (@samvaity) for the summary and JoshLove-msft for the heads up!
    I've brought up enabling assessment skill in CI in our internal meeting with Josephine Qiu (@josefree), Arthur Ma (@ArthurMa1978) and Renhe Li (@lirenhe), my current take away is: our first priority is still inner loop/local experience, so enabling assessment skill in CI is not prioritized till this calendar year. Happy to be corrected if I understand wrongly.

    There’s also some prerequisite work we’d need to complete before enabling assessment in CI: #17329. For example, we’d need to ensure parity with CI. Given these prerequisites, enabling the assessment skill in CI will take some time to get ready.

    Given that, the best approach could depend on the timeline of this epic, if it is urgent, probably calling naming skill directly from CI would be the quickest approach. But if we agree that enabling assessment in CI to support the .NET naming check is the right direction, I think we can definitely prioritize that as well.

  6. lirenhe commented on Oct 21, 2026

    @lirenhe
    Member

    I think the first thing we should clarify is the goal of “.NET readiness” at spec PR time. Is the goal specifically to ensure the generated .NET names are reviewed before merge, or more broadly to ensure the spec is ready for SDK generation with minimal post-merge rework?

    Depending on the goal, I see two possible options:

    1. Enable the assessment skill for .NET readiness. However, a lot of checks, including API and SDK breaking-change detection, are already covered by CI. To avoid duplicating CI, we may want a focused .NET assessment that only covers gaps CI does not provide, especially design/name review.

    2. Integrate the .NET name conversion skill directly into the spec PR pipeline. If naming is the primary problem we want to solve, this could be a more targeted solution by correcting .NET names before the spec is merged.

    My preference is to first define exactly what “.NET ready” means, then choose the simple solution that covers the gap without duplicating existing CI.

  7. JoshLove-msft commented on Oct 21, 2026

    @JoshLove-msft
    Member

    The goal is to ensure the generated .NET library will require minimal (ideally no) post-merge rework.

  8. samvaity commented on Oct 21, 2026

    @samvaity
    Member

    Thanks, this helps. Given the goal is minimal post-merge rework, calling the naming skill directly from CI seems like the practical first milestone while the assessment CI prerequisites continue.

    We should start report-only and pass generated C# evidence from SDK Validation when available, so the skill can distinguish recommendations from verified names. Together with the existing generation, build, and breaking-change checks, this addresses the design and naming-review gap without duplicating CI. We can keep assessment integration as the longer-term reporting path.

  9. JoshLove-msft commented on Oct 21, 2026

    @JoshLove-msft
    Member

    Also, even though this is scoped to C# ARM, we should think about applying the same conventions across languages. I don't think C# is unique in that we have things that could be caught in spec authoring/review that relate to the client library's public API.

  10. JoshLove-msft commented on Oct 21, 2026

    @JoshLove-msft
    Member

    Enable the assessment skill for .NET readiness. However, a lot of checks, including API and SDK breaking-change detection, are already covered by CI. To avoid duplicating CI, we may want a focused .NET assessment that only covers gaps CI does not provide, especially design/name review.

    Renhe Li (@lirenhe) isn't this a problem that we will have to solve regardless? i.e. the duplication between assessment and other CI checks. If so, then the concern you are raising is really about when we have to solve this problem, as opposed to "if" we have to solve it. Also, am I understanding correctly that SDK breaking-change detection is now enabled for C#?

  11. haolingdong-msft commented on Oct 22, 2026

    @haolingdong-msft
    Member

    We should start report-only and pass generated C# evidence from SDK Validation when available, so the skill can distinguish recommendations from verified names. Together with the existing generation, build, and breaking-change checks, this addresses the design and naming-review gap without duplicating CI. We can keep assessment integration as the longer-term reporting path.

    I agree with direction, and will keep Sdk Agent working group updated on the progress of enabling assessment from CI.

    However, a lot of checks, including API and SDK breaking-change detection, are already covered by CI. To avoid duplicating CI, we may want a focused .NET assessment that only covers gaps CI does not provide, especially design/name review.

    One clarification: I don’t think assessment duplicates CI. I see it as a complement to CI. The primary goal of assessment, from my point of view, is to help authors and reviewers understand what a TypeSpec change actually means, what impact it may have, and whether that impact matches the author’s intent.
    Take a real-world case as example. The change resulted in required properties in the PATCH operation, and the impact was significant enough that the service eventually had to redeploy some of existing API versions or deploy a new API version.
    What assessment can provide here is intent explaination: explain the consequence of the TypeSpec change and let the service team double check, “This change results in required properties in your PATCH operation."
    To me, that’s the key value beyond CI. CI validates known rules; assessment helps connect TypeSpec code to its actual API/SDK impact and verifies whether that impact matches what the author intended to build.

  12. live1206 commented on Oct 22, 2026

    @live1206
    Member

    Given the goal of this epic:

    Ensure C# names are reviewed and approved before the spec is merged, so the post-merge loop does not produce name-driven rework.

    Following up on the earlier discussion, I think the spec-PR validation workflow needs to generate the .NET SDK and apply the same C# review rules used in azure-sdk-for-net. TypeSpec names alone do not necessarily reflect the final generated C# names.

    The naming skill could contribute to that workflow, but checking the spec alone would not close the downstream review gap. #17371 also looks important for aligning the acceptance criteria across repositories.

    Is generated-SDK validation included in the planned report-only milestone?

  13. lmazuel commented on Oct 22, 2026

    @lmazuel
    MemberAuthor

    21. Given the goal of this epic:
    > Ensure C# names are reviewed and approved before the spec is merged, so the post-merge loop does not produce name-driven rework.

     Following up on [the earlier discussion](https://github.com/Azure/azure-sdk-tools/pull/17121#discussion_r4310973664), I think the spec-PR validation workflow needs to generate the .NET SDK and apply the same C# review rules used in azure-sdk-for-net. TypeSpec names alone do not necessarily reflect the final generated C# names.
     The naming skill could contribute to that workflow, but checking the spec alone would not close the downstream review gap. [#17371](https://github.com/Azure/azure-sdk-tools/issues/17371) also looks important for aligning the acceptance criteria across repositories.
    

    Yes, I agree we can't trust the TypeSpec alone to make sure names are correct.

     Is generated-SDK validation included in the planned report-only milestone?
    

    I don't understand your question :)

  14. live1206 commented on Oct 22, 2026

    @live1206
    Member

    I agree we can trust the TypeSpec alone to make sure names are correct.

    Laurent Mazuel (@lmazuel) Do you think we can or can not trust the TypeSpec alone?

    Is generated-SDK validation included in the planned report-only milestone?

    The current C# naming skill doesn't have SDK generation involved, I am asking if we should have it for this review step in spec.

  15. JoshLove-msft commented on Oct 23, 2026

    @JoshLove-msft
    Member

    I agree with Wei Hu (@live1206) here. I think the idea to incorporate the .NET PR review skill with the SDK validation step is a great idea and should give us close to 100% coverage (it won't cover cases where custom code is needed to fix build breaks as that needs to happen in the language repo). I don't think this is a replacement for the naming/assessment skills as those operate at the TypeSpec level and are still valuable in getting the spec as close to correct as possible from the start.
    For reference here is the review skill - Azure/azure-sdk-for-net#62615

  16. samvaity commented on Oct 23, 2026

    @samvaity
    Member

    Based on the discussion, we landed on a two-part approach:

    • Keep the naming skill early in the authoring flow so TypeSpec and client.tsp names are improved before generation.
    • During spec PR SDK Validation, use the generated .NET library and run the .NET review skill against the generated change. This covers the gap between TypeSpec names and the effective public C# API.

    We will start the generated-SDK review in report-only mode and try it on representative C# ARM PRs before deciding which findings should require approval or block a PR. Custom-code cases that require changes in the language repo remain outside the initial scope.

    Next steps:

    1. JoshLove-msft will confirm whether generator naming transformations are already documented and identify the right source of truth.
    2. Complete Add repository-wide Azure SDK code review skill azure-sdk-for-net#62615 and align the shared naming skill with the canonical .NET rules through [C# Naming] Consume C# naming rules from azure-sdk-for-net instead of restating them #17371.
    3. Confirm the generated SDK baseline and diff available in spec PR SDK Validation.
    4. Assign an owner and create a tracking issue for running the .NET review skill after SDK generation.
    5. Run the report-only flow on a C# ARM PR set and compare the findings with later .NET/APIView review.
    6. Revisit approval and merge gating after we have the results.

    #17336 continues to track TypeSpec-level naming review. The generated-SDK review will be tracked separately. Assessment remains useful for explaining TypeSpec intent and impact, but it is not a prerequisite for this first implementation.

    cc: Laurent Mazuel (@lmazuel) Wei Hu (@live1206)

  17. JoshLove-msft commented on Oct 23, 2026

    @JoshLove-msft
    Member

    I'm not sure we need the separate CI step for invoking the naming skill in report-only mode. If we plan to add the PR review skill integration, we could end up with duplicate findings. BTW how are we going to propagate the skill findings to PR comments? I suppose we can add a skill that looks at the SDK validation .NET results and invokes the .NET PR review skill?

  18. samvaity commented on Oct 23, 2026

    @samvaity
    Member

    If we plan to add the PR review skill integration, we could end up with duplicate findings.

    Agreed, this could create two reporting paths. The SDK PR CI can: SDK Validation generates the .NET library, then invokes the .NET PR review skill against the generated changes.

    BTW how are we going to propagate the skill findings to PR comments?

    I was thinking SDK Validation could publish the generated .NET diff, then a GitHub workflow would run the .NET review skill and update one managed PR comment. This keeps generation in Azure Pipelines while GitHub handles permissions, reruns, and comment publishing.
    Not sure if a skill alone can trigger review itself, download pipeline artifacts, or publish comments. It still needs a workflow or runner to provide that orchestration.

    Uncertain part: We would need to prototype the artifact handoff first. I have not seen this pattern precedence before used in the Azure SDK repos. But both parts are supported: GitHub workflows can trigger from an external Azure Pipelines check, and Azure Pipeline artifacts can be downloaded through the Azure DevOps API.

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

Metadata

Metadata

Labels

needs-triageWorkflow: This is a new issue that needs to be triaged to the appropriate team.

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions