diff --git a/.github/skills/mpg-sdk-migration/SKILL.md b/.github/skills/mpg-sdk-migration/SKILL.md index b40ef1c6fd48..d9cd1a6e4602 100644 --- a/.github/skills/mpg-sdk-migration/SKILL.md +++ b/.github/skills/mpg-sdk-migration/SKILL.md @@ -60,7 +60,10 @@ Use **explore** agents in parallel to gather information: - `tsp-location.yaml` → already migrated (may just need version bump) - `src/autorest.md` → legacy Swagger-based, needs migration 3. **Inventory existing csharp customizations in spec**: Search all `.tsp` files in the spec directory for `@clientName("...", "csharp")` and `@@clientName` decorators. Also check for `back-compatible.tsp`. These are already applied and must not be duplicated when adding renames later. -4. **Review naming conventions**: Consult the `azure-sdk-pr-review` skill for naming review rules. +4. **Snapshot old API surface**: Read `api/.net*.cs` and extract all public type names. Store in a lookup table for later rename resolution (Phase 8). +5. **Extract autorest rename mappings**: If `src/autorest.md` exists, extract all `rename-mapping` and `prepend-rp-prefix` entries. Store for comparison after generation. +6. **Identify custom code folder convention**: Check which name the package uses: `Custom/`, `Customization/`, or `Customized/`. Match this convention for all new custom code files. +7. **Review naming conventions**: Consult the `azure-sdk-mgmt-pr-review` skill for naming review rules. Present a summary plan and **ask the user** to confirm before proceeding. @@ -88,14 +91,20 @@ If `src/autorest.md` exists: 1. Extract key config: `namespace`, `title`, `azure-arm: true`, `require` URL, `output-folder`, directives. 2. **Thoroughly analyze rename mappings** before deleting: - Extract ALL `rename-mapping` entries and `prepend-rp-prefix` entries from `autorest.md`. - - Many renames are handled **automatically** by the mgmt emitter: RP prefix prepending (`prepend-rp-prefix`), `Is*` boolean property naming, acronym casing (e.g., `Ip` → `IP`), and standard format rules. Do NOT blindly add all renames to `client.tsp`. - - Check what `@clientName("...", "csharp")` decorators already exist in the individual `.tsp` files (e.g., `Volume.tsp`, `Account.tsp`, `back-compatible.tsp`) — these are already applied and must not be duplicated. - - After initial code generation, **compare old vs new public type names** to find which renames are missing. Only add `@@clientName` decorators for types that actually differ. + - The mgmt emitter auto-handles these naming transforms (anything **not** in this list still needs `@@clientName`): + - **Model/property suffixes**: `Url`→`Uri`, `Etag`→`ETag` + - **DateTimeOffset property suffixes**: `Time`→`On`, `Date`→`On`, `DateTime`→`On`, `At`→`On` (e.g. `CreatedAt`→`CreatedOn`). Also transforms word stems: `Creation`→`Created`, `Deletion`→`Deleted`, `Expiration`→`Expire`, `Modification`→`Modified`. Excludes properties starting with `From`/`To` or ending with `PointInTime`. + - **RP prefix prepending**: Automatically prepends the resource provider name to: `Sku`, `SkuName`, `SkuTier`, `SkuFamily`, `SkuInformation`, `Plan`, `Usage`, `Kind`, `PrivateEndpointConnection`, `PrivateLinkResource`, `PrivateLinkServiceConnectionState`, `PrivateEndpointServiceConnectionStatus`, `PrivateEndpointConnectionProvisioningState`, `PrivateLinkResourceProperties`, `PrivateLinkServiceConnectionStateProperty`, `PrivateEndpointConnectionListResult`, `PrivateLinkResourceListResult`. + - **Resource update models**: Models using the `ResourceUpdateModel` base type are auto-renamed — `{Resource}Patch` if used only in PATCH, or `{Resource}CreateOrUpdateContent` if used in both CREATE and UPDATE. + - Most other renames from `autorest.md` will still need `@@clientName` decorators. + - Do NOT blindly add all renames — check what `@clientName("...", "csharp")` decorators already exist in the spec `.tsp` files (e.g., `back-compatible.tsp`). These are already applied and must not be duplicated. + - After initial code generation, **compare old vs new public type names** to find which renames are missing. Only add `@@clientName` decorators for types that actually cause build errors. 3. Delete `src/autorest.md` — git history preserves the original content. 4. Do NOT create a `client.tsp` in the SDK repo. The TypeSpec source lives in the spec repo. 5. Map remaining AutoRest directives to TypeSpec customization approach: - - Model/property renames → `@@clientName(SpecTypeName, "SdkName", "csharp")` in spec repo `client.tsp` - - Accessibility overrides → `@@access(TypeName, Access.public, "csharp")` in spec repo `client.tsp` (for types generated as `internal` that need to be `public`) + - Model/property renames → `@@clientName(SpecNamespace.SpecTypeName, "SdkName", "csharp")` in spec repo `client.tsp` + - Accessibility overrides → `@@access(SpecNamespace.TypeName, Access.public, "csharp")` in spec repo `client.tsp` (for types generated as `internal` that need to be `public`) + - Type mapping overrides → `@@alternateType(SpecNamespace.Model.property, "Azure.ResourceManager.CommonTypes.ResourceIdentifier", "csharp")` for properties that should use SDK types instead of raw strings (e.g., resource IDs) - Suppressions → `#suppress` decorators in spec `.tsp` files - Format overrides → TypeSpec `@format` / `@encode` decorators @@ -118,11 +127,11 @@ The goal of iteration is to **get `dotnet build` to pass with zero errors** on t ``` - **Generator-only change** — regenerate with local generator: ```powershell - pwsh eng/packages/http-client-csharp-mgmt/eng/scripts/RegenSdkLocal.ps1 + pwsh eng/packages/http-client-csharp-mgmt/eng/scripts/RegenSdkLocal.ps1 -Services ``` - **Both spec and generator changed** — regenerate with both local: ```powershell - pwsh eng/packages/http-client-csharp-mgmt/eng/scripts/RegenSdkLocal.ps1 -LocalSpecRepoPath + pwsh eng/packages/http-client-csharp-mgmt/eng/scripts/RegenSdkLocal.ps1 -Services -LocalSpecRepoPath ``` - **Customization-only change** — no regeneration needed, just rebuild. 4. **Rebuild**: `dotnet build` to check if errors are resolved. @@ -156,10 +165,14 @@ sdk/// ## Phase 5 — Customization (Naming Review) -Apply naming rules from the `azure-sdk-pr-review` skill. For detailed customization techniques, invoke the `mitigate-breaking-changes` skill. +Apply naming rules from the `azure-sdk-mgmt-pr-review` skill. For detailed customization techniques, invoke the `mitigate-breaking-changes` skill. Key approaches: -- **SDK-side**: Partial classes under `Custom/` or `Customization/` with `[CodeGenType]`, `[CodeGenSuppress]`, `[CodeGenMember]` +- **SDK-side**: Partial classes under `Custom/` or `Customization/`: + - Plain partial class — add new members or override behavior (no attributes needed) + - `[CodeGenType("SpecName")]` — only needed when the custom class name differs from the spec/generated type name, to link them + - `[CodeGenSuppress("MemberName", typeof(...))]` — only needed when replacing a specific generated member with a custom implementation + - `[CodeGenMember("MemberName")]` — only needed when a custom property name differs from the generated property name - **Spec-side**: `@@clientName`, `@@access`, `@@markAsPageable`, `@@alternateType` decorators in `client.tsp` - **Extension resources**: Parameterized scopes, `ActionSync<>` for sub-resource ops (see the `mitigate-breaking-changes` skill) @@ -233,91 +246,233 @@ extends: - Migrated from Swagger to TypeSpec-based generation ``` -## Phase 8 — Build Error Triage & Fix Loop +## Phase 8 — Autonomous Build-Fix Loop -After code generation, build errors (including breaking changes) surface via `dotnet build`. Each error must be triaged and fixed iteratively. +After code generation, build errors surface via `dotnet build`. This phase runs **autonomously** — Copilot triages and fixes errors without asking the user, following the decision trees below. The loop continues until `dotnet build` passes with zero errors. -### Step 1 — Collect Build Errors +### Autonomous Execution Protocol -```powershell -cd sdk\\\src -dotnet build 2>&1 | Select-String "error CS" +``` +LOOP: + 1. Run `dotnet build` → collect ALL errors + 2. IF zero errors → EXIT LOOP (proceed to Phase 9) + 3. Parse errors into structured list (error code, file, message) + 4. Group errors by root cause using Classification Algorithm + 5. For each error group, check attempt_count: + - IF attempt_count >= 5 for any error → escalate to user via ask_user + - IF error count increased from previous iteration → escalate to user + 6. Apply fixes in batch using Priority Order and Decision Tree + 7. Increment attempt_count for each error that was targeted by a fix + 8. IF any fix required spec change → regenerate with LocalSpecRepo + 9. IF any fix required generator change → regenerate with RegenSdkLocal.ps1 + 10. GOTO 1 ``` -Parse the output into a structured list of errors (file, code, message). Use a SQL table to track them: +**Priority order** (fix these first — they cascade into many other errors): +1. Missing/renamed types (CS0234, CS0246) — one `@@clientName` fixes 5–20 errors +2. Accessibility issues (CS0051, CS0122) — one `@@access` or `[CodeGenType]` fixes multiple +3. Naming rule violations (AZC0030, AZC0032) — one rename per type +4. Signature mismatches (CS1729, CS0029, CS1503) — often a spec template issue +5. Duplicate definitions (CS0111) — usually generator or spec dedup issue +6. Other errors — investigate individually -```sql -CREATE TABLE build_errors ( - id TEXT PRIMARY KEY, - error_code TEXT, - file TEXT, - message TEXT, - root_cause TEXT, -- 'spec' | 'generator' | 'customization' | 'unknown' - status TEXT DEFAULT 'pending' -- 'pending' | 'investigating' | 'fixed' | 'blocked' -); +### Error Classification Algorithm + +For each build error, classify it **without asking the user**: + +``` +Given: error in file F with message M + +1. IF F is under `src/Generated/`: + a. IF M mentions a type that exists in old API (`api/*.cs`) but with different name: + → ROOT CAUSE: spec (missing @@clientName) + b. IF M says type is "inaccessible due to protection level" (CS0051/CS0122): + → ROOT CAUSE: spec (missing @@access) or customization ([CodeGenType]) + c. IF M is about wrong constructor args, type mismatch in return types: + → ROOT CAUSE: spec (wrong template usage, or missing @@alternateType) or generator bug + Check if old API used a different type (e.g., ResourceIdentifier vs string). + If so, try @@alternateType in client.tsp first. + d. IF M is AZC0030/AZC0032 (forbidden suffix): + → ROOT CAUSE: spec (needs @@clientName rename) + e. IF the generated code looks structurally wrong (missing serialization, + wrong inheritance, wrong type mapping) and the spec is correct: + → ROOT CAUSE: generator bug + +2. IF F is under `src/Custom/` or `src/Customization/` or `src/Customized/`: + a. IF M references a type that was renamed or no longer exists: + → ROOT CAUSE: spec (add @@clientName to preserve old name) OR + customization (update custom code to use new name) + b. IF M references a type that became internal: + → ROOT CAUSE: spec (@@access) or customization ([CodeGenType]) + +3. IF error is from ApiCompat: + a. IF rule is `TypesMustExist` AND the "missing" type matches a renamed type + whose behavior/shape is otherwise unchanged: + → ROOT CAUSE: spec (add @@clientName in client.tsp to restore previous public name) + b. OTHERWISE (e.g., `MembersMustExist`, shape/behavior changes, or truly removed API): + → ROOT CAUSE: customization (need backward-compat shim) ``` -### Step 2 — Triage Each Error +### Autonomous Fix Decision Tree -For each build error, determine the root cause and look up the fix in [error-reference.md](https://github.com/Azure/azure-sdk-for-net/blob/main/.github/skills/mpg-sdk-migration/error-reference.md). The error reference contains: -- **Root cause triage table** — how to classify errors as spec, generator, or customization issues -- **Common build error patterns** — CS0234, CS0051, CS0246, AZC0030, etc. with specific fixes -- **ApiCompat error patterns** — MembersMustExist, TypesMustExist, etc. +For each classified error, apply the fix **without asking the user**. Look up the specific error code in [error-reference.md](https://github.com/Azure/azure-sdk-for-net/blob/main/.github/skills/mpg-sdk-migration/error-reference.md) for the migration-specific root cause and fix. -### Step 3 — Fix Based on Root Cause +#### Decision: Spec Fix vs SDK Custom Code vs Generator Fix -For each error, apply the fix locally and regenerate. **The iteration loop continues until `dotnet build` passes with zero errors.** All three fix types (spec, generator, customization) can be combined in a single iteration — use `LocalSpecRepo` and `RegenSdkLocal.ps1` to avoid pushing to remote during the loop. +``` +PREFER spec-side fix (@@clientName, @@access, @@alternateType in client.tsp) when: + - The fix is a simple rename, accessibility change, or type mapping override + - Multiple errors would be resolved by one decorator + - The old name/accessibility is clearly documented in api/*.cs + +PREFER SDK custom code when: + - @@access doesn't work (nested/wrapper types — try spec first, fall back to [CodeGenType]) + - The fix requires adding backward-compat methods or properties + - The change is specific to this SDK and shouldn't affect the spec + - It's a one-off workaround for a generator limitation + +PREFER generator fix when: + - The same bug would affect ALL management SDKs (not just this one) + - The generated code is structurally wrong despite a correct spec + - CAUTION: Generator fixes require running Generate.ps1 to verify no regressions +``` -#### If TypeSpec issue (including naming/accessibility): -1. Update `client.tsp` (or other `.tsp` files) in the local spec repo. -2. Regenerate using the local spec repo: - ```powershell - dotnet build /t:GenerateCode /p:LocalSpecRepo= - ``` -3. Rebuild with `dotnet build` and verify the error is resolved. +#### Generator Fix Workflow -#### If generator bug/gap: -1. Fix the generator code under `eng/packages/http-client-csharp-mgmt/`. -2. Regenerate with the local generator (and optionally local spec): - ```powershell - # Generator-only fix - pwsh eng/packages/http-client-csharp-mgmt/eng/scripts/RegenSdkLocal.ps1 -Services - # Generator + spec fixes together - pwsh eng/packages/http-client-csharp-mgmt/eng/scripts/RegenSdkLocal.ps1 -Services -LocalSpecRepoPath - ``` -3. Rebuild and confirm the error is resolved. -4. Create a feature branch for the generator fix and open a **draft PR** for it. -5. Continue migration work on top of that branch. +``` +1. CONFIRM it's a generator bug (spec compiles, generated code is structurally wrong) +2. DECIDE: fix generator vs workaround (see error-reference.md Generator Bug Detection) +3. IF fixing generator: + - Edit code under eng/packages/http-client-csharp-mgmt/ + - Regenerate with RegenSdkLocal.ps1: + pwsh eng/packages/http-client-csharp-mgmt/eng/scripts/RegenSdkLocal.ps1 -Services + (add -LocalSpecRepoPath if spec was also changed) + - CLEAN UP stale custom workarounds that are now redundant after the fix + (e.g., [CodeGenSuppress] + manual implementations for the same issue) + - Run Generate.ps1 to verify no regressions on other SDKs + - Rebuild and confirm +4. IF workaround: [CodeGenSuppress] + custom implementation, document the issue +``` + +### Autonomous Rename Resolution Strategy -#### If customization needed: -1. Add a partial class under `src/Customization/` to address the issue. -2. Rebuild and confirm (no regeneration needed). +When migrating from autorest, many types get renamed. Resolve renames autonomously: -### Step 4 — Iterate Until Build Succeeds +``` +1. EXTRACT old names: + a. Read api/.net*.cs for all public type names + b. Read src/autorest.md rename-mapping entries (before deleting it) + c. Store both in a lookup table + +2. AFTER code generation, COMPARE: + a. Get all new public type names from src/Generated/ + b. For each type referenced in custom code or old API surface: + - IF type exists with same name → no action needed + - IF type exists with different name → add @@clientName to preserve old name + - IF type no longer exists → check if flattened/merged/removed + +3. For ALL name mismatches that cause build errors, add @@clientName in client.tsp. + PREFER @@clientName over updating custom code — it preserves backward compat + and minimizes SDK-side changes. + +4. For missing/moved operations (method not found, wrong resource type): + a. Check the operation's HTTP path in the spec + b. Compare with the old autorest-generated REST client + c. IF the operation path changed in the spec → the spec was updated, + add backward-compat wrapper in custom code if needed + d. IF the operation path is the same but mapped to a different resource + or extension scope → likely a generator bug in resource/scope assignment. + Check the resource's resourceType and resourceIdPattern in generated code. + e. IF the operation moved from one interface to another in the spec → + check if it should be an extension resource operation (different scope) +``` -**Exit condition: `dotnet build` passes with zero errors.** +### Batch Fix Strategy -After fixing one error, rebuild and update the error list. Each fix may resolve multiple cascading errors or reveal new ones. Repeat until the build is clean. Update the SQL tracking table as you go: +When there are many errors (20+), fix them in batches to avoid unnecessary regeneration cycles: + +``` +1. Collect ALL errors from dotnet build +2. Group by root cause type +3. For spec fixes (@@clientName, @@access): + a. Identify ALL needed decorators from the full error list + b. Add them ALL to client.tsp in one batch + c. Run `npx tsp format "**/*.tsp"` in the spec directory + d. Regenerate ONCE with LocalSpecRepo + e. Rebuild to see remaining errors +4. For custom code fixes: + a. These don't need regeneration — fix as many as possible in one pass + b. Rebuild after all custom code changes +5. For generator fixes: + a. Fix one at a time — each may have cascading effects + b. Regenerate and rebuild after each fix +``` + +### SQL Error Tracking + +Track errors and fixes for observability: ```sql -UPDATE build_errors SET status = 'fixed', root_cause = 'generator' WHERE id = 'err-1'; +CREATE TABLE build_errors ( + id TEXT PRIMARY KEY, + error_code TEXT, + file TEXT, + message TEXT, + root_cause TEXT, -- 'spec' | 'generator' | 'customization' | 'unknown' + fix_type TEXT, -- 'clientName' | 'access' | 'codeGenType' | 'codeGenSuppress' | 'custom_code' | 'generator_fix' | 'spec_template' + fix_detail TEXT, -- description of what was changed + attempt_count INTEGER DEFAULT 0, -- incremented each time a fix is attempted + last_fix_attempted TEXT, -- description of the last fix tried + status TEXT DEFAULT 'pending' -- 'pending' | 'investigating' | 'fixed' | 'blocked' | 'cascaded' +); ``` -**After the loop completes (build succeeds):** -Proceed to **Phase 9 — Create Pull Requests** to push changes and create separate PRs. +Update as you work: +```sql +-- After fixing a batch of renames +UPDATE build_errors SET status = 'fixed', root_cause = 'spec', fix_type = 'clientName', + fix_detail = 'Added @@clientName(NewName, "OldName", "csharp") in client.tsp' +WHERE error_code = 'CS0234' AND message LIKE '%OldTypeName%'; + +-- Errors resolved by cascade (fixed by another fix) +UPDATE build_errors SET status = 'cascaded' WHERE status = 'pending' AND id IN (...); +``` -### Sub-Agent Strategy for Error Triage +### Escalation Criteria -This loop is best handled **sequentially** with a mix of agent types: +The autonomous loop proceeds **without asking the user** except when: -1. **task agent** — Run `dotnet build`, collect errors, populate the SQL table. -2. **For each error** (sequential — each fix may change subsequent errors): - - **explore agent** — Investigate the error: read the generated file, compare with spec, determine root cause. - - **general-purpose agent** — Apply the fix (spec change, generator fix, or customization). Include full context: error message, file path, root cause, and relevant spec/generated code. - - **task agent** — Regenerate (if spec/generator fix) and rebuild to verify. -3. **task agent** — Final full build + test to confirm all errors resolved. +1. **Ambiguous rename** — Two equally valid old names exist and the correct choice is unclear +2. **Breaking API change with no backward-compat option** — A public type/method must be removed +3. **Generator fix affects other SDKs** — The fix would change behavior for all management SDKs +4. **Spec correctness concern** — The spec itself appears to have a bug (missing model, wrong type) beyond naming +5. **5 consecutive failed attempts** for the same error — something unexpected is happening +6. **Error count increases after a fix** — The fix made things worse + +For all other cases, **proceed autonomously** using the decision trees above. + +### Sub-Agent Strategy -> **Why sequential?** Each fix can resolve multiple errors or introduce new ones. Parallel fixing would cause conflicts and wasted work. +This loop is best handled with batched sequential execution: + +1. **task agent** — Run `dotnet build`, collect errors, populate the SQL table. +2. **Batch spec fixes** (most impactful first): + - **explore agent** — Analyze ALL CS0234/CS0246/CS0051/CS1729/CS1503/AZC0030/AZC0032 errors. Compare old API surface (`api/*.cs`) vs new generated names. Identify ALL needed `@@clientName`, `@@access`, and `@@alternateType` decorators. + - **general-purpose agent** — Apply ALL spec fixes to `client.tsp` in one batch. Include full context: all error messages, old API names, new generated names. + - **task agent** — Regenerate with `LocalSpecRepo` and rebuild. +3. **Batch custom code fixes**: + - **explore agent** — Analyze remaining errors in custom code, determine fixes needed. + - **general-purpose agent** — Apply custom code fixes (partial classes, `[CodeGenSuppress]`, `[CodeGenType]`, etc.). + - **task agent** — Rebuild (no regeneration needed). +4. **Generator fixes** (if any — one at a time): + - **explore agent** — Confirm generator bug, determine fix vs workaround approach. + - **general-purpose agent** — Apply generator fix or custom code workaround. + - **task agent** — Regenerate with `RegenSdkLocal.ps1` and rebuild. + - **general-purpose agent** — After generator fix, check for stale custom code workarounds that were added for the same issue earlier. Remove any `[CodeGenSuppress]` + manual implementations that are now redundant because the generator produces correct output. + - **task agent** — Rebuild again after cleanup to confirm no regressions. +5. **task agent** — Final full build to confirm all errors resolved. + +> **Why batched-sequential?** Spec fixes should be batched (one regeneration for many renames). Custom code fixes can be batched (no regeneration). Generator fixes are one-at-a-time (cascading effects). This minimizes regeneration cycles while avoiding conflicts. ## Phase 9 — Create Pull Requests @@ -377,7 +532,7 @@ When operating in fleet/autopilot mode, use sub-agents for parallelism: Launch these simultaneously at the start: - **Agent 1**: Find spec location and determine spec type (TypeSpec vs Swagger) - **Agent 2**: Analyze existing SDK package structure and current state -- **Agent 3**: Read naming guidelines from the azure-sdk-pr-review skill +- **Agent 3**: Read naming guidelines from the azure-sdk-mgmt-pr-review skill ### Sequential Phase (task/general-purpose agents) Execute these in order after planning: @@ -392,7 +547,7 @@ Execute these in order after planning: ### Rules for Fleet Agents - Always pass complete context in the prompt — agents are stateless. - Include the service name, paths, spec commit, and API version in every agent prompt. -- For customization agents, include the full naming rules from the azure-sdk-pr-review skill. +- For customization agents, include the full naming rules from the azure-sdk-mgmt-pr-review skill. - After each agent completes, verify output before launching dependent agents. - Use `ask_user` for any destructive changes or ambiguous naming decisions. @@ -409,17 +564,44 @@ After completing (or making significant progress on) a migration, review what wa ## Common Pitfalls -See [error-reference.md](https://github.com/Azure/azure-sdk-for-net/blob/main/.github/skills/mpg-sdk-migration/error-reference.md) for the full list of common pitfalls. Key ones to remember during the migration flow: +See [error-reference.md](https://github.com/Azure/azure-sdk-for-net/blob/main/.github/skills/mpg-sdk-migration/error-reference.md) for the full list of common pitfalls and autonomous fix recipes. Key ones to remember during the migration flow: - **Do NOT use `tsp-client update`** — use `dotnet build /t:GenerateCode`. -- **Do NOT blindly copy all renames from `autorest.md`** — the mgmt emitter handles many automatically. -- **Build errors cascade** — fix one at a time, rebuild, and re-assess. +- **Do NOT blindly copy all renames from `autorest.md`** — after generation, only add `@@clientName` for names that actually cause build errors. Check existing spec decorators to avoid duplicates. +- **Batch spec fixes, then rebuild** — collect ALL needed `@@clientName`/`@@access` decorators before regenerating, to minimize regeneration cycles. +- **Build errors cascade** — one spec fix can resolve 5–20 errors. Always rebuild after each batch. +- **Try spec-side fix (`@@access`) before custom code (`[CodeGenType]`)** — spec-side is cleaner but doesn't work for all types. - **Finalize `tsp-location.yaml` before creating the PR** — easy to forget when using `LocalSpecRepo`. +- **Match the existing custom code folder convention** — check if the package uses `Custom/`, `Customization/`, or `Customized/`. ## Safety Rules -1. **Never modify spec files** without explicit user confirmation. -2. **Delete `autorest.md`** after extracting directives — git history preserves it. -3. **Never edit files under `Generated/`** — they are overwritten by codegen. -4. **Always ask** before making destructive changes (deleting files, removing code). -5. **Preserve git history** — prefer renames over delete+create. +### Autonomous Mode (Default) + +During the build-fix loop (Phase 8), Copilot operates autonomously. These actions are **permitted without user confirmation**: + +1. **Spec changes**: Adding `@@clientName`, `@@access`, `@@markAsPageable`, `@@alternateType`, and other decorators to `client.tsp` — these are safe, reversible, and csharp-scoped. +2. **Custom code**: Adding partial classes in the SDK custom code folder. Use `[CodeGenType]`/`[CodeGenSuppress]`/`[CodeGenMember]` only when needed (see Phase 5). +3. **Deleting `autorest.md`** after extracting directives — git history preserves it. +4. **Updating custom code** to reference new generated type names. +5. **Regenerating code** using `dotnet build /t:GenerateCode` or `RegenSdkLocal.ps1`. +6. **Updating CHANGELOG.md** and other metadata files. + +### Actions Requiring User Confirmation + +These actions **require explicit user approval** (use `ask_user`): + +1. **Modifying spec `.tsp` files beyond `client.tsp`** — e.g., changing `main.tsp`, model definitions, or operation signatures. These affect all languages, not just C#. +2. **Generator code changes** that affect other SDKs — run `Generate.ps1` to verify scope first. +3. **Removing public API surface** with no backward-compat option (true breaking change). +4. **Adding `ApiCompatBaseline.txt` entries** — this should almost never be done. +5. **Deleting existing custom code files** — may lose manually-written logic. + +### Hard Rules (Never Violate) + +1. **Never edit files under `Generated/`** — they are overwritten by codegen. +2. **Never hand-edit `metadata.json`** — it is auto-generated. +3. **Never use `tsp-client update`** — use `dotnet build /t:GenerateCode`. +4. **Never add entries to `ApiCompatBaseline.txt`** without explicit user approval. +5. **Never bump the major version** of a management SDK package. +6. **Preserve git history** — prefer renames over delete+create. diff --git a/.github/skills/mpg-sdk-migration/error-reference.md b/.github/skills/mpg-sdk-migration/error-reference.md index 027313c8063d..e988618692b6 100644 --- a/.github/skills/mpg-sdk-migration/error-reference.md +++ b/.github/skills/mpg-sdk-migration/error-reference.md @@ -1,102 +1,63 @@ # MPG Migration: Error Reference -Reference for build errors, ApiCompat errors, and common pitfalls encountered during management-plane SDK migration. +Concise mapping from build errors to **mgmt-migration-specific** root causes and fixes. Copilot already understands C# error codes — this file only covers the Azure SDK-specific diagnosis and fix patterns. -## Build Error Triage +## Build Error → Root Cause Decision Table -For each build error, determine the root cause: +| Error | Migration-Specific Root Cause | Fix | +|-------|------------------------------|-----| +| **CS0234/CS0246** (type not found) | Type was renamed by new generator. Compare old name in `api/*.cs` with new names in `src/Generated/` | `@@clientName(SpecNamespace.NewName, "OldName", "csharp")` in `client.tsp`. OR update custom code to use new name | +| **CS0051/CS0122** (inaccessible type) | Type generated as `internal` but custom code uses it publicly | Try `@@access(SpecNamespace.Type, Access.public, "csharp")` in `client.tsp` first. If ineffective (common for nested types), use `[CodeGenType("SpecName")]` in custom code | +| **CS0111** (duplicate member) | Custom code and Generated/ both define same member; or extension resource with duplicate scope entries | Add `[CodeGenSuppress("Member", typeof(...))]` to custom partial class | +| **CS1729/CS0029/CS1503** (type mismatch) | Property uses wrong type (e.g., `string` instead of `ResourceIdentifier`), or sub-resource using `Read<>` template causes wrong REST client | `@@alternateType(SpecNamespace.Model.property, "ArmType", "csharp")` for type mapping overrides. For wrong REST client: change `Read<>` to `ActionSync<>` with `@get` in spec. OR update custom code constructor calls to match new signature | +| **CS0535** (interface not implemented) | Custom type needs `IJsonModel`/`IPersistableModel` after migration | Add missing interface methods in custom partial class | +| **CS0115** (no method to override) | Custom code overrides a method that was renamed/removed in new generated code | Update override to match new method name, or remove override | +| **AZC0030** (forbidden suffix: Request/Response/Parameters) | Azure SDK naming analyzer rejects generated name | `@@clientName` to old name from `api/*.cs`, or rename per convention: Request→Content, Response→Result, Parameters→Content | +| **AZC0032** (forbidden 'Data' suffix) | Type ends with `Data` but doesn't inherit `ResourceData` | `@@clientName` to old name, or rename to `*Info` | -| Root Cause | Symptoms | Fix Location | -|-----------|----------|--------------| -| **TypeSpec issue** | Missing/wrong model, property, enum value, or operation in generated code that can be corrected by updating the `.tsp` spec | Spec repo | -| **Naming/accessibility mismatch** | CS0234 (type not found — renamed), CS0051 (internal type used in public Custom code) | `client.tsp` in spec repo (`@@clientName` / `@@access`) or `src/Customization/*.cs` in SDK | -| **Generator bug/gap** | Correct spec but wrong codegen output (e.g., missing serialization, wrong type mapping, incorrect inheritance) | `eng/packages/http-client-csharp-mgmt` in SDK repo | -| **Customization needed** | Breaking change from rename, removed property, or type change that needs a compatibility shim | `src/Customization/*.cs` in SDK package | +### Key Diagnosis Rules -## Common Build Error Patterns +- **Error in `src/Generated/`** → almost always a spec fix (`@@clientName` or `@@access` in `client.tsp`) +- **Error in `src/Custom*/`** → either update custom code to use new names, or add `@@clientName` to preserve old names +- **ApiCompat error** → fix via spec-side rename (`@@clientName`) when the type still exists under a new name, or via custom code shim when the API truly changed (never use `ApiCompatBaseline.txt`) +- **Structurally wrong generated code despite correct spec** → generator bug -| Error Code | Typical Cause | Fix | -|-----------|--------------|-----| -| **CS0234** | Type name changed due to missing `@@clientName` rename | Add `@@clientName(SpecType, "OldSdkName", "csharp")` in `client.tsp`, regenerate | -| **CS0051** | Type became `internal` but Custom code references it publicly | First try `@@access(SpecType, Access.public, "csharp")` in `client.tsp`. If that doesn't work (common for nested/wrapper types), use `[CodeGenType("OriginalSpecName")]` in Custom code to override accessibility | -| **CS0246** | Type removed or restructured | Check if type was flattened, merged, or renamed. May need Custom code update | -| **CS0111** | Duplicate method/type definitions | For extension resources with parameterized scopes, check for duplicate resource entries. May need generator dedup fix | -| **CS1729/CS0029/CS1503** | Wrong REST client or type mismatch in collections | Sub-resource ops using `Read<>` template cause lifecycle misclassification. Fix by changing to `ActionSync<>` in spec (see the `mitigate-breaking-changes` skill, Extension Resources section) | -| **AZC0030** | Model name has forbidden suffix (`Request`, `Response`, `Parameters`) | Add `@@clientName` rename. Check old autorest SDK API surface for the **original name** to maintain backward compatibility, rather than inventing a new name | -| **AZC0032** | Model name has forbidden suffix (`Data`) not inheriting `ResourceData` | Add `@@clientName` rename to remove or replace the suffix | +## ApiCompat Error → Fix Table -## ApiCompat Error Patterns +ApiCompat errors surface via `dotnet pack --no-restore` (not `dotnet build`). **Never use `ApiCompatBaseline.txt`** — always mitigate with custom code. -ApiCompat compares the new generated API against the existing API surface files (`api/*.cs`). ApiCompat errors surface via `dotnet pack` (not `dotnet build`). **Do NOT use `ApiCompatBaseline.txt` to bypass breaking changes** — mitigate them with custom code instead. +| ApiCompat Error | Fix | +|----------------|-----| +| **MembersMustExist** (changed return type) | `[CodeGenSuppress("Method", typeof(...))]` + custom implementation with old return type delegating to new method | +| **MembersMustExist** (missing extension method) | Add custom extension method delegating to collection Get | +| **MembersMustExist** (missing setter) | `[CodeGenSuppress("Property")]` + re-declare with `{ get; set; }` in custom code | +| **TypesMustExist** (missing type) | `@@clientName` to restore old name, or create type in custom code with matching public API | -| ApiCompat Error | Cause | Fix | -|----------------|-------|-----| -| **MembersMustExist** (method with different return type) | Generated method has different return type than old API (e.g., `Response` vs `Pageable`) | Use `[CodeGenSuppress("MethodName", typeof(ParamType))]` on the partial class to suppress the generated method, then provide a custom implementation with the old return type | -| **MembersMustExist** (missing extension method) | Old API had `GetXxx(ArmClient, ResourceIdentifier scope, string name)` but new only has `GetXxxResource(ArmClient, ResourceIdentifier id)` | Add custom extension methods that delegate to collection Get | -| **TypesMustExist** | Old API had a type that no longer exists (e.g., base class removed) | Create the type in Custom code with matching properties and `IJsonModel<>`/`IPersistableModel<>` serialization | -| **MembersMustExist** (property setter missing) | Generated property is get-only but old API had setter | Use `[CodeGenSuppress("PropertyName")]` and re-declare with `{ get; set; }` in custom partial class | +After fixing all ApiCompat errors: `pwsh eng/scripts/Export-API.ps1 ` -### Handling ApiCompat Errors in the Migration Flow +## Generator Bug Detection -For each ApiCompat error: +If an error doesn't match the table above, check if it's a generator bug: +1. Spec is correct (`npx tsp compile .` succeeds, models look right) +2. Generated code is structurally wrong (not just naming) +3. Not fixable with `@@clientName`/`@@access`/`@@markAsPageable`/`@@alternateType` -1. **Missing member/type**: The old API had a public member that no longer exists. Determine why: - - **Renamed**: Add `@@clientName` in `client.tsp` to restore the old name, or add a backward-compat wrapper in Custom code. - - **Removed from spec**: If the member was removed in a newer API version, it may be acceptable. Document in CHANGELOG. - - **Changed signature**: Add a Custom code overload with the old signature that delegates to the new one. - - **Changed accessibility**: Use `@@access` or `[CodeGenType]` to restore public visibility. -2. After fixing all breaking changes, re-export the API surface: - ```powershell - pwsh eng/scripts/Export-API.ps1 - ``` - Where `` is the service folder name under `sdk/` (e.g., `guestconfiguration`, NOT the full package name). -3. Verify the full build passes: `dotnet build`. +**Fix generator** when: bug affects multiple SDKs, fix is straightforward. +**Workaround with custom code** when: one-off issue, complex generator fix, time pressure. Use `[CodeGenSuppress]` + custom implementation, document the workaround. -## Common Pitfalls - -1. **Do NOT use `tsp-client update` for code generation.** Use `dotnet build /t:GenerateCode`. The former can produce different output and `@@clientName`/`@@access` decorators may not take effect. - -2. **Do NOT compare only file names after generation.** The generated file names may be identical between old and new, but **file contents** change significantly (thousands of lines). Always use `git diff --stat` to verify content changes. - -3. **Do NOT blindly copy all `rename-mapping` entries from `autorest.md` to `client.tsp`.** The mgmt emitter automatically handles many renames (RP prefix, acronym casing, `Is*` booleans). Compare old vs new generated types to find only the missing renames. - -4. **Do NOT hand-author or manually edit `metadata.json`.** It is auto-generated by the tooling during code generation. Always include the auto-generated file in your PR. - -5. **`@@access` may not work for all types.** Some types nested inside other models (e.g., `VolumePropertiesExportPolicy`) may not respond to `@@access` decorators. In those cases, update the Custom code instead. - -6. **Custom code (`src/Custom/`) often references old type names.** After migration, scan Custom code for references to renamed or removed types. Common fixes: update type references, or add `@@clientName` to preserve the old name. - -7. **Build errors cascade.** A single missing rename can cause dozens of errors. Fix one error at a time, rebuild, and re-assess — many errors may resolve together. +After generator fix: regenerate with `RegenSdkLocal.ps1`, then **clean up stale custom workarounds** that are now redundant. -8. **Do NOT use `ApiCompatBaseline.txt` to bypass breaking changes.** Always mitigate breaking changes with custom code (`[CodeGenSuppress]`, partial classes, wrapper methods). The baseline should only be used as a last resort for changes that are truly impossible to fix. - -9. **Use `dotnet pack` (not just `dotnet build`) to check ApiCompat errors.** API compatibility checks only run during pack. Run `dotnet pack --no-restore` to catch breaking changes early. - -10. **Check the custom code folder name.** Different SDKs use different conventions: `Custom/`, `Customized/`, or `Customization/`. Always match the existing convention in the package. - -11. **Sub-resource operations must NOT use `Read<>` template.** When a TypeSpec spec defines sub-resource Get operations using `Read<>` or `Extension.Read<>`, the ARM library treats them as lifecycle read operations, causing wrong REST client selection. Use `ActionSync<>` with `@get` instead. See the `mitigate-breaking-changes` skill, Extension Resources section. - -12. **Use `@@markAsPageable` instead of custom `SinglePagePageable` wrappers.** When the old SDK returned `Pageable` for a non-pageable list operation, prefer adding `@@markAsPageable(Interface.operation, "csharp")` in `client.tsp` over writing custom `[CodeGenSuppress]` + `SinglePagePageable` wrapper code. This reduces custom code and produces a cleaner generated implementation. - -13. **File name casing mismatches between Windows and Linux CI.** The TypeSpec code generator may produce file names with different casing than what git tracks (e.g., `GuestConfigurationHcrpAssignmentsRestOperations.cs` in git vs `GuestConfigurationHCRPAssignmentsRestOperations.cs` on disk). On Windows (case-insensitive) this is invisible, but on Linux CI these are treated as **different files** — CI sees the old lowercase file as "deleted" and the new uppercase file as "untracked", causing the "Generated code is not up to date" error. **Fix**: After code generation, check for casing mismatches with: - ```powershell - git ls-files "sdk///src/Generated/" | ForEach-Object { - $filename = Split-Path $_ -Leaf - $dir = Split-Path $_ -Parent - $diskFile = Get-ChildItem -LiteralPath (Join-Path (Get-Location) $dir) -Filter $filename -ErrorAction SilentlyContinue - if ($diskFile -and ($diskFile.Name -cne $filename)) { - Write-Host "MISMATCH: git='$filename' disk='$($diskFile.Name)' in $dir" - } - } - ``` - Fix each mismatch with `git rm --cached ` then `git add `. - -14. **`@@markAsPageable` is ineffective on operations already marked with `@list`.** If a TypeSpec operation is already decorated with `@list` (making it pageable), adding `@@markAsPageable` is redundant and will cause a compile error: `@markAsPageable decorator is ineffective since this operation is already marked as pageable with @list decorator`. Before adding `@@markAsPageable`, check whether the target operation already has `@list` in its spec definition. Only add `@@markAsPageable` for operations that are truly non-pageable in the spec. - -15. **Always run `CodeChecks.ps1` locally before pushing.** The CI "Verify Generated Code" step runs `eng\scripts\CodeChecks.ps1 -ServiceDirectory `, which regenerates code, updates snippets, re-exports API, and does `git diff --exit-code`. Run this locally to catch issues before CI: - ```powershell - pwsh eng\scripts\CodeChecks.ps1 -ServiceDirectory - ``` - This is the single most reliable way to verify your changes will pass the "Build Analyze PRBatch" CI check. +## Common Pitfalls -16. **Finalize `tsp-location.yaml` before creating the PR.** When using `LocalSpecRepo` for fast iteration, it's easy to forget to update `tsp-location.yaml` with the final commit SHA. Before creating the PR: (a) commit and push all spec changes, (b) get the final commit SHA, (c) update `tsp-location.yaml` with that SHA, and (d) do one final `dotnet build /t:GenerateCode` **without** `LocalSpecRepo` to verify CI-reproducible generation. +1. **Use `dotnet build /t:GenerateCode`**, not `tsp-client update`. The latter may ignore `@@clientName`/`@@access` decorators. +2. **Don't blindly copy all `rename-mapping` from `autorest.md`** — only add `@@clientName` for names that actually cause build errors after generation. The emitter auto-handles these transforms: `Url`→`Uri`, `Etag`→`ETag`, DateTimeOffset suffixes (`Time`/`Date`/`DateTime`/`At`→`On`, plus `Creation`→`Created`, `Deletion`→`Deleted`, `Expiration`→`Expire`, `Modification`→`Modified`), RP prefix for `Sku`/`SkuName`/`SkuTier`/`SkuFamily`/`SkuInformation`/`Plan`/`Usage`/`Kind`/`PrivateEndpointConnection`/`PrivateLinkResource`/related types, and resource update model naming (`Patch`/`CreateOrUpdateContent`). +3. **Don't hand-edit `metadata.json`** — it's auto-generated. +4. **`@@access` may not work for nested types** — fall back to `[CodeGenType]` in custom code. +5. **`@@markAsPageable` fails on `@list` operations** — check spec before adding; only use for truly non-pageable operations. +6. **Use `@@markAsPageable` instead of custom `SinglePagePageable` wrappers** — cleaner, less custom code. +7. **Sub-resource operations must NOT use `Read<>`** — use `ActionSync<>` with `@get` to avoid lifecycle misclassification. +8. **File name casing mismatches** break Linux CI. After generation, check with `git ls-files` vs disk; fix with `git rm --cached` + `git add`. +9. **Match existing custom code folder** — `Custom/`, `Customization/`, or `Customized/`. +10. **Run `CodeChecks.ps1` before pushing**: `pwsh eng\scripts\CodeChecks.ps1 -ServiceDirectory ` +11. **Finalize `tsp-location.yaml`** with pushed spec commit SHA before creating PR. Do one final `dotnet build /t:GenerateCode` without `LocalSpecRepo`. +12. **Clean up stale custom workarounds after generator fixes** — remove `[CodeGenSuppress]` + manual implementations that are now redundant after regeneration.