Merge 18/dev into feature branch

.claude/skills/umb-review/evals/evals.json

@@ -0,0 +1,97 @@
+{
+  "skill_name": "umb-review",
+  "evals": [
+    {
+      "id": 0,
+      "name": "pr-22214-large-frontend-refactor",
+      "prompt": "Review the changes in PR #22214 (branch origin/pr/22214 targeting main). This is a large frontend refactor migrating create entity actions to use entityCreateOptionAction extensions, with deprecations.",
+      "expected_output": "A structured review that identifies frontend deprecation patterns, flags the large PR complexity, handles 75+ files correctly, checks for breaking changes in exported components, and produces the correct output format.",
+      "pr_number": 22214,
+      "pr_branch": "origin/pr/22214",
+      "base_branch": "origin/main",
+      "files": [],
+      "assertions": [
+        {"id": "deprecation-patterns-noted", "text": "Review identifies deprecation patterns (@deprecated, UmbDeprecation)"},
+        {"id": "frontend-breaking-change-awareness", "text": "Checks frontend-specific breaking changes (exports, custom elements) not just backend"},
+        {"id": "file-references-present", "text": "Findings reference specific files with line numbers"},
+        {"id": "no-false-critical-on-deprecations", "text": "Properly deprecated code is NOT flagged as Critical breaking change"},
+        {"id": "no-stylistic-nitpicks", "text": "Review does not flag purely cosmetic/stylistic issues (formatting, whitespace, naming conventions, comment grammar, code style preferences) unless they affect performance or rendering. Missing JSDoc on new public APIs is NOT a stylistic issue — it is a legitimate finding."},
+        {"id": "manifest-alias-rename-detected", "text": "Alias renames (CreateOptions → Create) flagged as Critical breaking change"},
+        {"id": "non-exported-deletions-dismissed", "text": "Deleted action classes NOT flagged as breaking (verified against package.json exports)"},
+        {"id": "noise-files-filtered", "text": "Does not review noise files (generated files, lock files, etc.)"},
+        {"id": "complexity-advisory-triggers", "text": "Review includes a complexity/split advisory for the large 75+ file scope"}
+      ]
+    },
+    {
+      "id": 1,
+      "name": "pr-21672-small-frontend-bugfix",
+      "prompt": "Review the changes in PR #21672 (branch origin/pr/21672 targeting main). This is a small 4-file frontend bugfix implementing tab validation badges in the block editor.",
+      "expected_output": "A clean review that correctly identifies this as a small focused bugfix, avoids false positives, and either approves or approves with minor suggestions.",
+      "pr_number": 21672,
+      "pr_branch": "origin/pr/21672",
+      "base_branch": "origin/main",
+      "files": [],
+      "assertions": [
+        {"id": "complexity-advisory-absent", "text": "Review does NOT include a complexity/split advisory"},
+        {"id": "no-false-breaking-changes", "text": "Review does not flag breaking changes"},
+        {"id": "proportionate-verdict", "text": "Verdict is 'Request Changes'"},
+        {"id": "concise-review", "text": "Review output is under 200 lines"},
+        {"id": "no-stylistic-nitpicks", "text": "Review does not flag purely cosmetic/stylistic issues (formatting, whitespace, naming conventions, comment grammar, code style preferences) unless they affect performance or rendering. Missing JSDoc on new public APIs is NOT a stylistic issue — it is a legitimate finding."}
+      ]
+    },
+    {
+      "id": 2,
+      "name": "pr-22217-small-backend-webhook",
+      "prompt": "Review the changes in PR #22217 (branch origin/pr/22217 targeting v18/dev). This is a tiny 3-file backend change to the default webhook payload type.",
+      "expected_output": "A concise review that correctly resolves v18/dev as target branch, handles the small change proportionately, and considers the behavioral impact of changing a default value.",
+      "pr_number": 22217,
+      "pr_branch": "origin/pr/22217",
+      "base_branch": "origin/v18/dev",
+      "files": [],
+      "assertions": [
+        {"id": "correct-target-branch", "text": "Review references 'v18/dev' as the target branch (not 'main')"},
+        {"id": "default-value-change-noted", "text": "Review discusses the behavioral impact of changing the default payload type"},
+        {"id": "proportionate-review", "text": "Review output is under 150 lines"},
+        {"id": "no-stylistic-nitpicks", "text": "Review does not flag purely cosmetic/stylistic issues (formatting, whitespace, naming conventions, comment grammar, code style preferences) unless they affect performance or rendering. Missing JSDoc on new public APIs is NOT a stylistic issue — it is a legitimate finding."},
+        {"id": "ignores-preexisting-issues", "text": "Does NOT flag the ~30 builder extension methods with Legacy defaults (pre-existing, not changed in the PR)"},
+        {"id": "side-effect-detection", "text": "Flags stale WebhookSettings.cs docs as a side-effect of the constant value change"},
+        {"id": "consumer-identification", "text": "Identifies affected consumers outside the PR (WebhookSettings, UmbracoBuilder, or WebhookEventCollectionBuilderExtensions)"}
+      ]
+    },
+    {
+      "id": 3,
+      "name": "pr-22268-frontend-feature-workspace-modal",
+      "prompt": "Review the changes in PR #22268 (branch origin/pr/22268 targeting main). This is a 29-file frontend feature adding a current user workspace modal.",
+      "expected_output": "A review of a medium-sized new feature PR. Should assess the new code for architectural compliance, check for breaking changes (new exports, custom elements), and evaluate code quality without flagging pre-existing issues.",
+      "pr_number": 22268,
+      "pr_branch": "origin/pr/22268",
+      "base_branch": "origin/main",
+      "files": [],
+      "assertions": [
+        {"id": "complexity-advisory-triggers", "text": "Review includes a complexity/split advisory (3 layers: Core, API, Frontend across 27+ files)"},
+        {"id": "breaking-changes-on-interface-additions", "text": "Flags new interface methods without default implementations as breaking changes (Pattern 3)"},
+        {"id": "no-stylistic-nitpicks", "text": "Review does not flag purely cosmetic/stylistic issues unless they affect performance or rendering. Missing JSDoc on new public APIs is NOT a stylistic issue — it is a legitimate finding."},
+        {"id": "diff-scoped", "text": "All findings reference code that was changed in the diff, not pre-existing issues"},
+        {"id": "new-feature-assessed", "text": "Review assesses the new feature's architecture, patterns, or integration approach — not just absence of bugs"},
+        {"id": "no-false-notification-finding", "text": "Review does NOT flag UpdateCurrentUserAsync as missing UserSavingNotification/UserSavedNotification — the sibling UpdateAsync also does not publish these notifications, so flagging their absence would be a false positive"}
+      ]
+    },
+    {
+      "id": 4,
+      "name": "pr-22215-frontend-architecture-violation",
+      "prompt": "Review the changes in PR #22215 (branch origin/pr/22215 targeting main). This is a 2-file frontend feature adding user management to the user group workspace.",
+      "expected_output": "A review that catches the architecture violation: the workspace context directly imports and calls UserService and UserGroupService (generated API clients) instead of going through a repository. In the Umbraco backoffice, workspace contexts access data via repositories, not by calling API services directly. The review should flag this as a significant architecture issue and request changes.",
+      "pr_number": 22215,
+      "pr_branch": "origin/pr/22215",
+      "base_branch": "origin/main",
+      "files": [],
+      "assertions": [
+        {"id": "service-bypass-detected", "text": "Review flags that the workspace context directly imports/calls UserService or UserGroupService instead of using a repository"},
+        {"id": "repository-pattern-recommended", "text": "Review recommends using the repository pattern (going through a repository/data-source layer) rather than calling API services directly from the workspace context"},
+        {"id": "verdict-request-changes", "text": "Verdict is 'Request Changes' (the architecture violation warrants requesting changes, not just approving with suggestions)"},
+        {"id": "no-stylistic-nitpicks", "text": "Review does not flag purely cosmetic/stylistic issues (formatting, whitespace, naming conventions, comment grammar, code style preferences) unless they affect performance or rendering. Missing JSDoc on new public APIs is NOT a stylistic issue — it is a legitimate finding."},
+        {"id": "no-false-breaking-changes", "text": "Review does not flag breaking changes (this PR only adds new code, no public API is removed or modified)"}
+      ]
+    }
+  ]
+}

.claude/skills/umb-review/references/breaking-changes.md

@@ -0,0 +1,249 @@
+# Breaking Changes Reference
+
+This document describes how to detect and validate breaking changes during PR review. It covers both backend (.NET) and frontend (TypeScript/Lit) patterns.
+
+---
+
+## Version Detection
+
+**Always read `version.json`** at the repository root to determine the current major version. This drives the obsolete removal target calculation:
+
+- Current major version: read from `version.json` → `version` field (e.g., `"17.4.0-rc"` → major version `17`)
+- Obsolete removal target: `current + 2` (e.g., if current is 17, removal is scheduled for Umbraco 19)
+- Format: `[Obsolete("... Scheduled for removal in Umbraco {current+2}.")]`
+
+---
+
+## Backend (.NET) Breaking Changes
+
+### What Constitutes a Breaking Change
+
+Any of these on a `public` or `protected` member:
+
+- Removing or renaming a class, interface, struct, record, or enum
+- Removing or renaming a method, property, or field
+- Changing a method signature (parameters, return type)
+- Adding required parameters to an existing method
+- Adding methods to a public interface (without default implementation)
+- Changing a constructor signature on a public class
+- Removing or changing enum values
+- Changing type hierarchy (base class, implemented interfaces)
+
+### Pattern 1: Obsolete Constructor + StaticServiceProvider
+
+When a public class needs new dependencies, the existing constructor must be preserved.
+
+**Correct pattern:**
+
+```csharp
+[Obsolete("Please use the constructor with all parameters. Scheduled for removal in Umbraco 19.")]
+public MyService(IDependencyA depA)
+    : this(
+        depA,
+        StaticServiceProvider.Instance.GetRequiredService<IDependencyB>())
+{
+}
+
+public MyService(IDependencyA depA, IDependencyB depB)
+{
+    _depA = depA;
+    _depB = depB;
+}
+```
+
+**Validation checklist:**
+
+- [ ] Old constructor has `[Obsolete]` attribute with correct removal version
+- [ ] Old constructor calls new constructor via `: this(...)`
+- [ ] `StaticServiceProvider.Instance.GetRequiredService<T>()` used for new params only
+- [ ] DI registration uses the NEW constructor (old is for external consumers only)
+- [ ] Removal version is `{current_major + 2}`
+
+**Common mistakes to flag:**
+
+- Removing the old constructor entirely (breaking change!)
+- Old constructor NOT calling new constructor (code duplication)
+- Wrong removal version in `[Obsolete]`
+- Missing `StaticServiceProvider` resolution for new dependencies
+- DI registration still using the old constructor
+
+### Pattern 2: Obsolete Method + New Overload
+
+When a method signature needs to change, add the new overload and obsolete the old.
+
+**Correct pattern:**
+
+```csharp
+[Obsolete("Use the overload taking all parameters. Scheduled for removal in Umbraco 19.")]
+public void DoThing(string name)
+    => DoThing(name, extraParam: null);
+
+public void DoThing(string name, string? extraParam)
+{
+    // Real implementation here
+}
+```
+
+**Validation checklist:**
+
+- [ ] Old method has `[Obsolete]` attribute with correct removal version
+- [ ] Old method calls new method, providing defaults for new parameters
+- [ ] All internal callers updated to use the new method
+- [ ] No internal code references the obsolete method (except the delegation)
+
+### Pattern 3: Default Interface Implementation
+
+When adding methods to a public interface, provide a default implementation.
+
+**Correct pattern:**
+
+```csharp
+public interface IMyService
+{
+    void ExistingMethod();
+
+    // New method with default implementation
+    void NewMethod(string param)
+        => ExistingMethod(); // delegate to existing if possible
+}
+```
+
+**Strategies for defaults (in order of preference):**
+
+1. Use existing interface methods to satisfy the contract
+2. Return a sensible default (empty collection, null, etc.)
+3. Throw `NotImplementedException` if no reasonable default exists
+
+**Validation checklist:**
+
+- [ ] New interface method has a default implementation
+- [ ] TODO comment present: `// TODO (V{next-major}): Remove the default implementation when {obsolete method} is removed.`
+- [ ] Default implementation is functionally correct (even if not optimal)
+- [ ] If `StaticServiceProvider` is used in default impl, noted as temporary
+
+### Obsolete Attribute Validation
+
+For any `[Obsolete]` attribute found in changed code:
+
+1. **Format**: Must contain `"Scheduled for removal in Umbraco {version}."`
+2. **Version**: Must be `current_major + 2` (read from `version.json`)
+3. **Pragma**: Where obsolete members must call each other, `#pragma warning disable CS0618` / `#pragma warning restore CS0618` must be present
+
+### Internal Caller Check
+
+After finding obsolete patterns, verify:
+
+- Search the codebase for usages of the obsolete member
+- **No internal code** (inside `src/`) should reference obsolete members
+- Only the obsolete member's own delegation (calling the new version) is acceptable
+- External consumers (outside the repo) get the deprecation period to migrate
+
+---
+
+## Frontend (TypeScript/Lit) Breaking Changes
+
+The backoffice is published as `@umbraco-cms/backoffice` with 140+ named exports. Plugin developers depend on this public API surface.
+
+**Critical frontend rule (does not apply to backend .NET where `public`/`protected` visibility determines the API surface): only symbols reachable through the `package.json` `exports` field are public API.** Anything not exported — whether classes, functions, constants, types, or entire files — is an internal implementation detail, even if other internal code imports it. Removing or changing unexported frontend symbols is not a breaking change. Before flagging a frontend deletion or rename as breaking, verify the symbol is reachable via `package.json` exports. If it is not, do not flag it.
+
+### Custom Elements (Web Components)
+
+**Breaking changes:**
+
+- Renaming or removing a registered custom element tag (`umb-*`)
+- Removing elements from `HTMLElementTagNameMap`
+- Removing or changing `@property()` decorated fields on exported components
+- Removing event emissions (checked via `this.dispatchEvent`)
+- Removing CSS custom properties (`@cssprop` in JSDoc)
+- Removing CSS parts (`@csspart` in JSDoc)
+
+**How to detect:**
+
+- Check diff for removed `@customElement('umb-...')` decorators
+- Check diff for removed `@property()` fields on exported components
+- Check diff for removed entries in `HTMLElementTagNameMap` declarations
+
+### Exported Types/Interfaces
+
+**Breaking changes:**
+
+- Removing exports from `package.json` `exports` field
+- Changing the shape of exported interfaces (removing properties, changing types)
+- Renaming exported types (consumers import by name)
+- Removing union type members
+- Changing generic type parameter constraints
+
+**How to detect:**
+
+- Check if `package.json` `exports` field is modified
+- Check diff for removed `export` statements
+- Check diff for changed interface/type shapes
+
+### Manifest/Extension System
+
+**Breaking changes:**
+
+- Renaming a manifest `alias` value — plugin developers reference aliases by string in conditions, overwrites, and extension registry lookups. Alias renames are not caught by the compiler since they are string-based. A renamed alias silently breaks any plugin that references the old string.
+- Removing support for a manifest `type` that plugins use
+- Changing manifest `alias` resolution or validation
+- Removing or renaming manifest `kind` types
+- Changing extension bundle structure
+
+**How to detect:**
+
+- **Alias renames**: Compare `alias:` values in manifest files before and after. Changed alias strings are Critical — the old alias should be preserved as a deprecated entry.
+- Search for changes to manifest type definitions
+- Check for removed or renamed manifest kinds
+
+### Context API
+
+**Breaking changes:**
+
+- Removing context tokens from exports
+- Changing the shape of data provided by a context
+- Removing context provider/consumer mechanisms
+
+**How to detect:**
+
+- Check for removed context token exports
+- Check for changes to context provider classes
+
+### Controllers/Lifecycle
+
+**Breaking changes:**
+
+- Changing controller base class inheritance requirements
+- Removing controller lifecycle hooks
+- Breaking cleanup mechanisms in `disconnectedCallback()`
+
+### Observable/State
+
+**Breaking changes:**
+
+- Removing observable properties from the public API
+- Changing observable emission patterns
+
+### npm Publishing
+
+**Breaking changes:**
+
+- Changing version constraints that exclude previously-supported versions
+- Adding incompatible peer dependency constraints
+
+**How to detect:**
+
+- Check if `package.json` `peerDependencies` or `dependencies` changed
+- Verify version ranges are not narrowed
+
+---
+
+## Reporting Breaking Changes
+
+When a breaking change is detected, report:
+
+1. **What**: The specific change and which public symbol is affected
+2. **Pattern**: Which mitigation pattern should be applied (Pattern 1, 2, or 3 for backend)
+3. **Severity**: Critical (no mitigation present) or Important (mitigation present but incorrect)
+4. **Fix**: Concrete code suggestion showing the correct pattern
+
+If no breaking changes are detected, state: "No breaking changes detected."