Merge pull request #8 from microsoft/preview/expand-review-skills

Expand review skills to match the 6-domain knowledge corpus

README.md

@@ -58,7 +58,7 @@ Skills define how agents consume knowledge. They come in three flavors:
 
   READ and DO are read on demand — typically when the first dispatched action skill runs. They are not prerequisites for invoking Entry. WRITE is only used when scaffolding new content.
 
-- **Action skills** — concrete skills that follow the Action Skill template to do real work (review code, audit telemetry, etc.). Action skills live inside the layers that own them (`/microsoft/skills/`, `/community/skills/`, `/custom/skills/`). An action skill is either a **leaf** that evaluates knowledge files directly, or a **super-skill** that composes other action skills (declared via `sub-skills` in frontmatter). The canonical reference is [`microsoft/skills/al-code-review.md`](microsoft/skills/al-code-review.md) (super-skill), which composes [`microsoft/skills/al-performance-review.md`](microsoft/skills/al-performance-review.md) and [`microsoft/skills/al-security-review.md`](microsoft/skills/al-security-review.md) (leaves).
+- **Action skills** — concrete skills that follow the Action Skill template to do real work (review code, audit telemetry, etc.). Action skills live inside the layers that own them (`/microsoft/skills/`, `/community/skills/`, `/custom/skills/`). An action skill is either a **leaf** that evaluates knowledge files directly, or a **super-skill** that composes other action skills (declared via `sub-skills` in frontmatter). The canonical reference is [`microsoft/skills/review/al-code-review.md`](microsoft/skills/review/al-code-review.md) (super-skill), which composes six leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) — one per knowledge domain (performance, security, privacy, upgrade, style, UI).
 
 ### Agent bootstrapping
 

agent-consumption.md

@@ -35,7 +35,7 @@ The agent reads `/skills/entry.md` and runs it against the task context. Entry a
 The dispatch record names one or more action skills and the subset of inputs each should receive. If the outcome is `no-match` or `failed`, the agent returns the record to the orchestrator unchanged.
 
 ### 4. Agent invokes each dispatched action skill
-Action skills live inside the layers — `/microsoft/skills/`, `/community/skills/`, `/custom/skills/` — so their authority is carried by their location. For a PR review, Entry typically dispatches `microsoft/skills/al-code-review.md`. The agent reads the file and executes it.
+Action skills live inside the layers — `/microsoft/skills/`, `/community/skills/`, `/custom/skills/` — so their authority is carried by their location. For a PR review, Entry typically dispatches `microsoft/skills/review/al-code-review.md`. The agent reads the file and executes it.
 
 ### 5. Action skill executes the four-step pattern
 

microsoft/skills/review/al-code-review.md

@@ -3,7 +3,7 @@ kind: action-skill
 id: al-code-review
 version: 1
 title: AL code review
-description: Reviews AL source changes by composing the AL review leaf skills (performance, security, ...).
+description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI).
 inputs: [pr-diff, file-path]
 outputs: [findings-report]
 bc-version: [all]
@@ -11,8 +11,12 @@ technologies: [al]
 countries: [w1]
 application-area: [all]
 sub-skills:
-  - microsoft/skills/al-performance-review.md
+  - microsoft/skills/review/al-performance-review.md
-  - microsoft/skills/al-security-review.md
+  - microsoft/skills/review/al-security-review.md
+  - microsoft/skills/review/al-privacy-review.md
+  - microsoft/skills/review/al-upgrade-review.md
+  - microsoft/skills/review/al-style-review.md
+  - microsoft/skills/review/al-ui-review.md
 ---
 
 # AL code review
@@ -27,10 +31,14 @@ An orchestrator invokes this skill with either a `pr-diff` (the standard PR-revi
 
 The sub-skills invoked by this skill are those listed in frontmatter `sub-skills`:
 
-- `microsoft/skills/al-performance-review.md`
+- `microsoft/skills/review/al-performance-review.md`
-- `microsoft/skills/al-security-review.md`
+- `microsoft/skills/review/al-security-review.md`
+- `microsoft/skills/review/al-privacy-review.md`
+- `microsoft/skills/review/al-upgrade-review.md`
+- `microsoft/skills/review/al-style-review.md`
+- `microsoft/skills/review/al-ui-review.md`
 
-Additional leaf skills (for example, UX, telemetry, testing) are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly.
+Additional leaf skills (for example, telemetry, testing) are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly.
 
 ## Relevance
 
@@ -74,7 +82,7 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip
   "skill": { "id": "al-code-review", "version": 1 },
   "outcome": "completed",
   "summary": {
-    "counts": { "blocker": 1, "major": 1, "minor": 1, "info": 1 },
+    "counts": { "blocker": 1, "major": 1, "minor": 2, "info": 0 },
     "coverage": { "worklist-size": 4, "items-evaluated": 4 }
   },
   "findings": [
@@ -94,40 +102,44 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip
       "from-sub-skill": "al-performance-review"
     },
     {
-      "id": "community/knowledge/performance/use-setloadfields.md",
+      "id": "community/knowledge/performance/call-setloadfields-before-filters.md",
-      "severity": "info",
+      "severity": "minor",
-      "message": "Posting routine iterates ledger entries; consider whether SetLoadFields applies per the linked guidance.",
+      "message": "SetLoadFields is called after SetRange. Per the referenced guidance the call must come before filters to be folded into the query plan.",
+      "location": {
+        "file": "src/Sales/PostingRoutines.Codeunit.al",
+        "line": 152
+      },
       "references": [
-        { "path": "community/knowledge/performance/use-setloadfields.md" }
+        { "path": "community/knowledge/performance/call-setloadfields-before-filters.md" }
       ],
-      "confidence": "low",
+      "confidence": "high",
       "from-sub-skill": "al-performance-review"
     },
     {
-      "id": "microsoft/knowledge/security/no-plaintext-secrets-in-telemetry.md",
+      "id": "microsoft/knowledge/security/use-secrettext-for-credentials.md",
       "severity": "blocker",
-      "message": "A bearer token is passed to Session.LogMessage as part of the CustomDimensions payload. The referenced guidance documents this as a platform-level data-protection violation.",
+      "message": "A bearer token is declared as a Text parameter and passed through the HTTP request path as plain text. The referenced guidance requires credentials to flow as SecretText end-to-end.",
       "location": {
         "file": "src/Integration/ApiClient.Codeunit.al",
         "line": 85,
         "range": { "start-line": 85, "end-line": 89 }
       },
       "references": [
-        { "path": "microsoft/knowledge/security/no-plaintext-secrets-in-telemetry.md" }
+        { "path": "microsoft/knowledge/security/use-secrettext-for-credentials.md" }
       ],
       "confidence": "high",
       "from-sub-skill": "al-security-review"
     },
     {
-      "id": "microsoft/knowledge/security/avoid-implicit-commit.md",
+      "id": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md",
       "severity": "minor",
-      "message": "An explicit COMMIT inside a posting routine may leave the ledger in an inconsistent state if subsequent steps fail.",
+      "message": "An API key is assigned from a string literal rather than retrieved from IsolatedStorage or Key Vault at runtime.",
       "location": {
-        "file": "src/Sales/PostingRoutines.Codeunit.al",
+        "file": "src/Integration/ApiClient.Codeunit.al",
         "line": 201
       },
       "references": [
-        { "path": "microsoft/knowledge/security/avoid-implicit-commit.md" }
+        { "path": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md" }
       ],
       "confidence": "medium",
       "from-sub-skill": "al-security-review"
@@ -139,7 +151,7 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip
       "skill": { "id": "al-performance-review", "version": 1 },
       "outcome": "completed",
       "summary": {
-        "counts": { "blocker": 0, "major": 1, "minor": 0, "info": 1 },
+        "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 },
         "coverage": { "worklist-size": 2, "items-evaluated": 2 }
       },
       "findings": [
@@ -158,13 +170,17 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip
           "confidence": "high"
         },
         {
-          "id": "community/knowledge/performance/use-setloadfields.md",
+          "id": "community/knowledge/performance/call-setloadfields-before-filters.md",
-          "severity": "info",
+          "severity": "minor",
-          "message": "Posting routine iterates ledger entries; consider whether SetLoadFields applies per the linked guidance.",
+          "message": "SetLoadFields is called after SetRange. Per the referenced guidance the call must come before filters to be folded into the query plan.",
+          "location": {
+            "file": "src/Sales/PostingRoutines.Codeunit.al",
+            "line": 152
+          },
           "references": [
-            { "path": "community/knowledge/performance/use-setloadfields.md" }
+            { "path": "community/knowledge/performance/call-setloadfields-before-filters.md" }
           ],
-          "confidence": "low"
+          "confidence": "high"
         }
       ],
       "suppressed": []
@@ -178,29 +194,29 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip
       },
       "findings": [
         {
-          "id": "microsoft/knowledge/security/no-plaintext-secrets-in-telemetry.md",
+          "id": "microsoft/knowledge/security/use-secrettext-for-credentials.md",
           "severity": "blocker",
-          "message": "A bearer token is passed to Session.LogMessage as part of the CustomDimensions payload. The referenced guidance documents this as a platform-level data-protection violation.",
+          "message": "A bearer token is declared as a Text parameter and passed through the HTTP request path as plain text. The referenced guidance requires credentials to flow as SecretText end-to-end.",
           "location": {
             "file": "src/Integration/ApiClient.Codeunit.al",
             "line": 85,
             "range": { "start-line": 85, "end-line": 89 }
           },
           "references": [
-            { "path": "microsoft/knowledge/security/no-plaintext-secrets-in-telemetry.md" }
+            { "path": "microsoft/knowledge/security/use-secrettext-for-credentials.md" }
           ],
           "confidence": "high"
         },
         {
-          "id": "microsoft/knowledge/security/avoid-implicit-commit.md",
+          "id": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md",
           "severity": "minor",
-          "message": "An explicit COMMIT inside a posting routine may leave the ledger in an inconsistent state if subsequent steps fail.",
+          "message": "An API key is assigned from a string literal rather than retrieved from IsolatedStorage or Key Vault at runtime.",
           "location": {
-            "file": "src/Sales/PostingRoutines.Codeunit.al",
+            "file": "src/Integration/ApiClient.Codeunit.al",
             "line": 201
           },
           "references": [
-            { "path": "microsoft/knowledge/security/avoid-implicit-commit.md" }
+            { "path": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md" }
           ],
           "confidence": "medium"
         }

microsoft/skills/review/al-performance-review.md

@@ -78,7 +78,7 @@ Output conforms to the DO output contract. A populated example:
   "skill": { "id": "al-performance-review", "version": 1 },
   "outcome": "completed",
   "summary": {
-    "counts": { "blocker": 0, "major": 1, "minor": 0, "info": 1 },
+    "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 },
     "coverage": { "worklist-size": 2, "items-evaluated": 2 }
   },
   "findings": [
@@ -97,13 +97,17 @@ Output conforms to the DO output contract. A populated example:
       "confidence": "high"
     },
     {
-      "id": "community/knowledge/performance/use-setloadfields.md",
+      "id": "community/knowledge/performance/call-setloadfields-before-filters.md",
-      "severity": "info",
+      "severity": "minor",
-      "message": "Posting routine iterates ledger entries; consider whether SetLoadFields applies per the linked guidance.",
+      "message": "SetLoadFields is called after SetRange. Per the referenced guidance the call must come before filters to be folded into the query plan.",
+      "location": {
+        "file": "src/Sales/PostingRoutines.Codeunit.al",
+        "line": 152
+      },
       "references": [
-        { "path": "community/knowledge/performance/use-setloadfields.md" }
+        { "path": "community/knowledge/performance/call-setloadfields-before-filters.md" }
       ],
-      "confidence": "low"
+      "confidence": "high"
     }
   ],
   "suppressed": []

microsoft/skills/review/al-privacy-review.md

@@ -0,0 +1,102 @@
+---
+kind: action-skill
+id: al-privacy-review
+version: 1
+title: AL privacy review
+description: Reviews AL source changes against privacy and data-classification guidance from BCQuality.
+inputs: [pr-diff, file-path]
+outputs: [findings-report]
+bc-version: [all]
+technologies: [al]
+countries: [w1]
+application-area: [all]
+---
+
+# AL privacy review
+
+Reviews AL source changes against the `privacy` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
+
+An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract.
+
+## Source
+
+Collect all knowledge files under `*/knowledge/privacy/**/*.md`, across every enabled layer (`/microsoft/`, `/community/`, `/custom/`). Relevance trims the result to the subset that applies.
+
+## Relevance
+
+Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context:
+
+- `bc-version` — the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`.
+- `technologies` — `[al]`.
+- `countries` — the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`.
+- `application-area` — the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`.
+
+Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown.
+
+## Worklist
+
+Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
+
+- The changed AL object names and types — especially tables and tableextensions (for `DataClassification` on fields), codeunits that call `Error` or `Session.LogMessage`, codeunits performing outgoing HTTP requests with customer data, and objects reading or writing `IsolatedStorage`.
+- The changed procedures and triggers, weighted toward those that call `Error`, `Session.LogMessage`, `StrSubstNo`, `GetLastErrorText`, `HttpClient.Post`/`Get`, `IsolatedStorage.Set`/`SetEncrypted`/`Get`, or `PrivacyNotice.GetPrivacyNoticeApprovalState`.
+- Tokens extracted from the diff that relate to privacy (`DataClassification`, `CustomerContent`, `EndUserIdentifiableInformation`, `SystemMetadata`, `ToBeClassified`, `PrivacyNotice`, `GetLastErrorText`, `TelemetryScope`).
+
+A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from filename and Description) matches a changed object type.
+
+Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
+
+When the post-conflict worklist is empty because no applicable privacy knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable privacy knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
+
+## Action
+
+For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows:
+
+- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee (for example, documented telemetry-classification rules or GDPR-adjacent data-handling requirements). When the file does not make such a claim, the ceiling is `major`.
+- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
+- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
+
+Set `confidence` to:
+
+- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type).
+- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`.
+- `low` when the finding is an advisory derived only from applicability.
+
+Outcome selection:
+
+- `completed` — the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty.
+- `no-knowledge` — no applicable privacy knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty.
+- `not-applicable` — the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task).
+- `partial` — a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause.
+- `failed` — an unrecoverable error occurred. `outcome-reason` is required.
+
+## Output
+
+Output conforms to the DO output contract. A populated example:
+
+```json
+{
+  "skill": { "id": "al-privacy-review", "version": 1 },
+  "outcome": "completed",
+  "summary": {
+    "counts": { "blocker": 0, "major": 1, "minor": 0, "info": 0 },
+    "coverage": { "worklist-size": 1, "items-evaluated": 1 }
+  },
+  "findings": [
+    {
+      "id": "microsoft/knowledge/privacy/strsubstno-prebuild-breaks-error-telemetry-classification.md",
+      "severity": "major",
+      "message": "Error receives a pre-built Text produced by StrSubstNo with customer name and email as arguments. Per the referenced guidance the platform cannot classify or strip PII from an opaque Text and will export the full message to telemetry.",
+      "location": {
+        "file": "src/Sales/CustomerValidation.Codeunit.al",
+        "line": 64,
+        "range": { "start-line": 60, "end-line": 64 }
+      },
+      "references": [
+        { "path": "microsoft/knowledge/privacy/strsubstno-prebuild-breaks-error-telemetry-classification.md" }
+      ],
+      "confidence": "high"
+    }
+  ],
+  "suppressed": []
+}
+```

microsoft/skills/review/al-security-review.md

@@ -83,29 +83,29 @@ Output conforms to the DO output contract. A populated example:
   },
   "findings": [
     {
-      "id": "microsoft/knowledge/security/no-plaintext-secrets-in-telemetry.md",
+      "id": "microsoft/knowledge/security/use-secrettext-for-credentials.md",
       "severity": "blocker",
-      "message": "A bearer token is passed to Session.LogMessage as part of the CustomDimensions payload. The referenced guidance documents this as a platform-level data-protection violation.",
+      "message": "A bearer token is declared as a Text parameter and passed through the HTTP request path as plain text. The referenced guidance requires credentials to flow as SecretText end-to-end.",
       "location": {
         "file": "src/Integration/ApiClient.Codeunit.al",
         "line": 85,
         "range": { "start-line": 85, "end-line": 89 }
       },
       "references": [
-        { "path": "microsoft/knowledge/security/no-plaintext-secrets-in-telemetry.md" }
+        { "path": "microsoft/knowledge/security/use-secrettext-for-credentials.md" }
       ],
       "confidence": "high"
     },
     {
-      "id": "microsoft/knowledge/security/avoid-implicit-commit.md",
+      "id": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md",
       "severity": "minor",
-      "message": "An explicit COMMIT inside a posting routine may leave the ledger in an inconsistent state if subsequent steps fail.",
+      "message": "An API key is assigned from a string literal rather than retrieved from IsolatedStorage or Key Vault at runtime.",
       "location": {
-        "file": "src/Sales/PostingRoutines.Codeunit.al",
+        "file": "src/Integration/ApiClient.Codeunit.al",
         "line": 201
       },
       "references": [
-        { "path": "microsoft/knowledge/security/avoid-implicit-commit.md" }
+        { "path": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md" }
       ],
       "confidence": "medium"
     }

microsoft/skills/review/al-style-review.md

@@ -0,0 +1,99 @@
+---
+kind: action-skill
+id: al-style-review
+version: 1
+title: AL style review
+description: Reviews AL source changes against naming, labelling, and code-convention guidance from BCQuality.
+inputs: [pr-diff, file-path]
+outputs: [findings-report]
+bc-version: [all]
+technologies: [al]
+countries: [w1]
+application-area: [all]
+---
+
+# AL style review
+
+Reviews AL source changes against the `style` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
+
+Style findings cover AL conventions that CodeCop and similar analyzers partially enforce — label suffixes, API page naming, temporary-variable prefixes, label properties, named invocations, `FieldCaption`/`TableCaption` in user messages, `OptionCaption` pairing, Error-parameter passing, `this` keyword, required parentheses, file-naming. Use together with a formal analyzer; this skill adds BCQuality's remedial-knowledge explanations of why each rule exists.
+
+An orchestrator invokes this skill with either a `pr-diff` or a `file-path`. The skill produces a single JSON document conforming to the DO output contract.
+
+## Source
+
+Collect all knowledge files under `*/knowledge/style/**/*.md`, across every enabled layer.
+
+## Relevance
+
+Apply the frontmatter matching rules defined in READ against the task context:
+
+- `bc-version` — the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`.
+- `technologies` — `[al]`.
+- `countries` — the countries declared in the consuming app's `app.json`. If absent, `unknown`.
+- `application-area` — pass the actual set declared by the changed objects; do not substitute `[all]`.
+
+Discard files that are not applicable. Retain conditionally applicable files only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium` and MUST name the unknown dimensions in `message`.
+
+## Worklist
+
+Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
+
+- Changed AL objects — especially API pages (`PageType = API`), tables and pages declaring Labels/TextConsts, codeunits issuing `Error`/`Message`/`Confirm`, and any file whose name violates the `<ObjectName>.<ObjectType>.al` convention.
+- Changed declarations, weighted toward `: Label '...'`, `: TextConst '...'`, temporary record variables, option fields, error-handling call sites, and codeunit-internal method calls.
+- Tokens extracted from the diff (`Label`, `TextConst`, `Locked`, `Comment`, `MaxLength`, `temporary`, `OptionMembers`, `OptionCaption`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `DelayedInsert`, `FieldCaption`, `TableCaption`, `FieldName`, `TableName`, `Page.RunModal`, `Report.Run`, `this.`, `StrSubstNo`).
+
+A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic matches a changed object or declaration.
+
+Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.
+
+When the post-conflict worklist is empty because no applicable style knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable style knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
+
+## Action
+
+For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Style findings rarely reach `blocker` — reserve it for cases where the knowledge file documents a platform-level requirement (for example, API page property constraints the OData runtime rejects). Most style findings are `minor` or `info`; egregious misuse (`Error` with pre-built Text losing translation and telemetry classification) may reach `major`.
+
+Set `confidence` to:
+
+- `high` when the detection is based on an unambiguous pattern match.
+- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`.
+- `low` when the finding is an advisory derived only from applicability.
+
+Outcome selection:
+
+- `completed` — the skill evaluated every worklist item.
+- `no-knowledge` — no applicable style knowledge survived filtering.
+- `not-applicable` — no AL changes in the diff.
+- `partial` — a budget was hit before the worklist was exhausted.
+- `failed` — an unrecoverable error occurred.
+
+## Output
+
+Output conforms to the DO output contract. A populated example:
+
+```json
+{
+  "skill": { "id": "al-style-review", "version": 1 },
+  "outcome": "completed",
+  "summary": {
+    "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 },
+    "coverage": { "worklist-size": 1, "items-evaluated": 1 }
+  },
+  "findings": [
+    {
+      "id": "microsoft/knowledge/style/apply-approved-label-suffixes.md",
+      "severity": "minor",
+      "message": "A Label named Text000 has no approved suffix (Msg/Err/Qst/Tok/Lbl/Txt). Per the referenced CodeCop AA0074 guidance, every Label and TextConst carries a suffix indicating its consuming call.",
+      "location": {
+        "file": "src/Sales/PostingRoutines.Codeunit.al",
+        "line": 42
+      },
+      "references": [
+        { "path": "microsoft/knowledge/style/apply-approved-label-suffixes.md" }
+      ],
+      "confidence": "high"
+    }
+  ],
+  "suppressed": []
+}
+```

microsoft/skills/review/al-ui-review.md

@@ -0,0 +1,99 @@
+---
+kind: action-skill
+id: al-ui-review
+version: 1
+title: AL UI text review
+description: Reviews AL page files against UI-text, caption, and tooltip guidance from BCQuality.
+inputs: [pr-diff, file-path]
+outputs: [findings-report]
+bc-version: [all]
+technologies: [al]
+countries: [w1]
+application-area: [all]
+---
+
+# AL UI text review
+
+Reviews AL page source against the `ui` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
+
+UI findings apply to page files — files that declare `PageType = ...`, including `*.Page.al` under the standard file-naming convention. The skill returns `not-applicable` when the diff contains no page changes.
+
+An orchestrator invokes this skill with either a `pr-diff` or a `file-path`. The skill produces a single JSON document conforming to the DO output contract.
+
+## Source
+
+Collect all knowledge files under `*/knowledge/ui/**/*.md`, across every enabled layer.
+
+## Relevance
+
+Apply the frontmatter matching rules defined in READ against the task context:
+
+- `bc-version` — the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`.
+- `technologies` — `[al]`.
+- `countries` — the countries declared in the consuming app's `app.json`. If absent, `unknown`.
+- `application-area` — pass the actual set declared by the changed objects; do not substitute `[all]`.
+
+Discard files that are not applicable. Retain conditionally applicable files only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium` and MUST name the unknown dimensions in `message`.
+
+## Worklist
+
+Narrow the relevant files to the subset that applies to the changes under review.
+
+- **Page-file filter.** UI review applies only to files declaring `page`, `pageextension`, or `pagecustomization`. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files.
+- For each relevant knowledge file, compute overlap against changed page declarations, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, action definitions, and field-level properties.
+- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions).
+
+A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic matches a changed page element.
+
+Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.
+
+When the post-conflict worklist is empty because no applicable UI knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable UI knowledge matched the page changes, emit `outcome: "completed"` with an empty `findings` array.
+
+## Action
+
+For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. UI text findings are generally `minor` — they affect localization and polish rather than correctness. Reach for `major` only when a banned term appears in customer-facing text or a caption truncation is guaranteed at the stated character limit.
+
+Set `confidence` to:
+
+- `high` when the detection is based on an unambiguous pattern match (banned term literal, missing "Specifies" opener on a field tooltip, caption exceeding documented limit).
+- `medium` when detection relies on heuristics (judging whether a caption is a noun phrase or a sentence phrase) or when any frontmatter dimension was `unknown`.
+- `low` when the finding is an advisory derived only from applicability.
+
+Outcome selection:
+
+- `completed` — the skill evaluated every worklist item.
+- `no-knowledge` — no applicable UI knowledge survived filtering.
+- `not-applicable` — the diff contains no page, pageextension, or pagecustomization files.
+- `partial` — a budget was hit before the worklist was exhausted.
+- `failed` — an unrecoverable error occurred.
+
+## Output
+
+Output conforms to the DO output contract. A populated example:
+
+```json
+{
+  "skill": { "id": "al-ui-review", "version": 1 },
+  "outcome": "completed",
+  "summary": {
+    "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 },
+    "coverage": { "worklist-size": 1, "items-evaluated": 1 }
+  },
+  "findings": [
+    {
+      "id": "microsoft/knowledge/ui/field-tooltips-start-with-specifies-and-end-with-period.md",
+      "severity": "minor",
+      "message": "Field ToolTip is a fragment ('Customer name') — missing the 'Specifies' opener and the terminating period the house-style guidance requires.",
+      "location": {
+        "file": "src/Sales/CustomerCard.Page.al",
+        "line": 58
+      },
+      "references": [
+        { "path": "microsoft/knowledge/ui/field-tooltips-start-with-specifies-and-end-with-period.md" }
+      ],
+      "confidence": "high"
+    }
+  ],
+  "suppressed": []
+}
+```

microsoft/skills/review/al-upgrade-review.md

@@ -0,0 +1,101 @@
+---
+kind: action-skill
+id: al-upgrade-review
+version: 1
+title: AL upgrade review
+description: Reviews AL source changes against upgrade-code and migration guidance from BCQuality.
+inputs: [pr-diff, file-path]
+outputs: [findings-report]
+bc-version: [all]
+technologies: [al]
+countries: [w1]
+application-area: [all]
+---
+
+# AL upgrade review
+
+Reviews AL source changes against the `upgrade` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
+
+An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). Upgrade findings are narrow by design — they apply when the diff touches upgrade codeunits, install codeunits, table schema, enums, or objects under migration namespaces. The skill returns `not-applicable` when none of those apply.
+
+## Source
+
+Collect all knowledge files under `*/knowledge/upgrade/**/*.md`, across every enabled layer (`/microsoft/`, `/community/`, `/custom/`). Relevance trims the result to the subset that applies.
+
+## Relevance
+
+Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context:
+
+- `bc-version` — the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`.
+- `technologies` — `[al]`.
+- `countries` — the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`.
+- `application-area` — the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`.
+
+Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown.
+
+## Worklist
+
+Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
+
+- The changed AL object names and types — especially codeunits with `Subtype = Upgrade` or `Subtype = Install`, tables and tableextensions adding or changing fields, enums and enumextensions, and objects under `Hybrid*`/`Migration`/`Upgrade` namespaces.
+- The changed triggers and procedures, weighted toward `OnUpgradePerCompany`, `OnUpgradePerDatabase`, `OnInstallAppPerCompany`, and the `OnGetPerCompanyUpgradeTags`/`OnGetPerDatabaseUpgradeTags` subscribers.
+- Tokens extracted from the diff that relate to upgrade concerns (`Subtype = Upgrade`, `Upgrade Tag`, `HasUpgradeTag`, `SetUpgradeTag`, `DataTransfer`, `InitValue`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `DataVersion`, `ExecutionContext`, `value(`, `enum`, `enumextension`).
+
+A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic matches a changed object type. When the diff contains no upgrade-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
+
+Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files suppressed by configuration are recorded with `reason: "configuration"`.
+
+When the post-conflict worklist is empty because no applicable upgrade knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable upgrade knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
+
+## Action
+
+For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows:
+
+- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` for irreversible data corruption (enum-ordinal shift, unguarded reads that abort the upgrade) and for changes that would ship to customers without a migration path (new InitValue on an existing table without upgrade code).
+- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
+- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file.
+
+Set `confidence` to:
+
+- `high` when the detection is based on an unambiguous pattern match.
+- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`.
+- `low` when the finding is an advisory derived only from applicability.
+
+Outcome selection:
+
+- `completed` — the skill evaluated every worklist item.
+- `no-knowledge` — no applicable upgrade knowledge survived filtering.
+- `not-applicable` — the diff touches no upgrade, install, schema, or enum surface.
+- `partial` — a budget was hit before the worklist was exhausted.
+- `failed` — an unrecoverable error occurred.
+
+## Output
+
+Output conforms to the DO output contract. A populated example:
+
+```json
+{
+  "skill": { "id": "al-upgrade-review", "version": 1 },
+  "outcome": "completed",
+  "summary": {
+    "counts": { "blocker": 1, "major": 0, "minor": 0, "info": 0 },
+    "coverage": { "worklist-size": 1, "items-evaluated": 1 }
+  },
+  "findings": [
+    {
+      "id": "microsoft/knowledge/upgrade/enum-changes-must-be-additive-at-the-end.md",
+      "severity": "blocker",
+      "message": "A new enum value was inserted at ordinal 1, shifting every subsequent value by one. Rows that store the old ordinal 1 will silently resolve to the new value. Per the referenced guidance, enum values must be appended at the end.",
+      "location": {
+        "file": "src/Shared/OrderStatus.Enum.al",
+        "line": 7
+      },
+      "references": [
+        { "path": "microsoft/knowledge/upgrade/enum-changes-must-be-additive-at-the-end.md" }
+      ],
+      "confidence": "high"
+    }
+  ],
+  "suppressed": []
+}
+```

skills/entry.md

@@ -76,7 +76,7 @@ Emit a single JSON document conforming to the output contract below. Entry does
       "skill": {
         "id": "al-code-review",
         "version": 1,
-        "path": "microsoft/skills/al-code-review.md"
+        "path": "microsoft/skills/review/al-code-review.md"
       },
       "rationale": "string",
       "inputs": ["pr-diff"]
@@ -141,14 +141,14 @@ Populated example (PR review on a repo where only `al-performance-review` is ena
   "outcome": "routed",
   "dispatch": [
     {
-      "skill": { "id": "al-performance-review", "version": 1, "path": "microsoft/skills/al-performance-review.md" },
+      "skill": { "id": "al-performance-review", "version": 1, "path": "microsoft/skills/review/al-performance-review.md" },
       "rationale": "Goal 'review pull request' matched; inputs-available contains pr-diff.",
       "inputs": ["pr-diff"]
     }
   ],
   "skipped": [
-    { "skill": { "id": "al-code-review", "path": "microsoft/skills/al-code-review.md" }, "reason": "configuration" },
+    { "skill": { "id": "al-code-review", "path": "microsoft/skills/review/al-code-review.md" }, "reason": "configuration" },
-    { "skill": { "id": "al-security-review", "path": "microsoft/skills/al-security-review.md" }, "reason": "configuration" }
+    { "skill": { "id": "al-security-review", "path": "microsoft/skills/review/al-security-review.md" }, "reason": "configuration" }
   ]
 }
 ```