---
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, one per knowledge domain.
inputs: [pr-diff, file-path, folder-path]
outputs: [findings-report]
bc-version: [all]
technologies: [al]
countries: [w1]
application-area: [all]
sub-skills:
  - microsoft/skills/review/al-performance-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
  - microsoft/skills/review/al-error-handling-review.md
  - microsoft/skills/review/al-events-review.md
  - microsoft/skills/review/al-interfaces-review.md
  - microsoft/skills/review/al-breaking-changes-review.md
  - microsoft/skills/review/al-web-services-review.md
  - microsoft/skills/review/al-testing-review.md
  - microsoft/skills/review/al-data-modeling-review.md
  - microsoft/skills/review/al-query-review.md
  - microsoft/skills/review/al-reporting-review.md
  - microsoft/skills/review/al-appsource-review.md
  - microsoft/skills/review/al-telemetry-review.md
  - microsoft/skills/review/al-scm-review.md
  - microsoft/skills/review/al-finance-review.md
---

# AL code review

Reviews AL source changes by composing the leaf AL review skills. This is the canonical reference implementation of a **super-skill** — skill authors writing composed reviews should copy its structure.

`al-code-review` does not evaluate knowledge files directly. It invokes each of its sub-skills against the same task input, collects their findings-reports, and then performs its own **self-review pass** over the diff using the agent's built-in BC and AL knowledge. BCQuality knowledge is an additive layer: anything the sub-skills found is cited from BCQuality, and anything the agent finds on its own is validated against BCQuality (cited if matched, suppressed if contradicted, surfaced as an **agent finding** otherwise). The result is a single rolled-up findings-report that mixes knowledge-backed and agent findings, each clearly tagged via `from-sub-skill`.

An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. The skill produces a single JSON document conforming to the DO output contract, extended with `sub-results` and — when applicable — `skipped-sub-skills`.

## Source

The sub-skills invoked by this skill are those listed in frontmatter `sub-skills`. Additional leaf skills are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly.

Hosts that orchestrate leaves mechanically SHOULD run
`tools/Build-SkillIndex.ps1` and resolve this skill by `id: al-code-review`.
The generated `subSkills` array preserves the frontmatter slot order and
avoids host-specific Markdown parsing. Before invoking leaves, run
`tools/Resolve-SkillWorklist.ps1` with this skill's path and the task's enabled
layers and disabled skill paths. Each declared path supplies a leaf `id`; the
resolver selects the highest-precedence enabled implementation with that `id`
without changing slot order.

## Relevance

A sub-skill is relevant when both of the following hold:

- The orchestrator has supplied inputs that satisfy the sub-skill's declared `inputs`.
- The orchestrator has not disabled the sub-skill via configuration.

Per the DO contract, the super-skill MUST NOT filter sub-skills by task content. `al-code-review` does not inspect the PR diff to predict whether, for example, there is anything for `al-security-review` to find. Each leaf is responsible for its own task-level applicability decision; leaves signal non-applicability by returning `outcome: "not-applicable"` or `outcome: "no-knowledge"`.

Sub-skills that fail either check are not invoked and are recorded in `skipped-sub-skills`:

- `reason: "configuration"` when the orchestrator disabled the sub-skill.
- `reason: "not-applicable"` when the orchestrator's inputs do not satisfy the sub-skill's declared `inputs`.

## Worklist

The worklist is the list of sub-skills judged relevant by the previous step. Every sub-skill in the worklist will be invoked in the Action step.

## Action

### Execution discipline (mandatory)

The Action step consists of **discrete leaf invocations**, not one combined generation. Invocation scheduling belongs to the orchestrator: independent leaves may run serially or concurrently, but their evaluation contexts and findings-reports remain isolated. Concretely this means:

- **Isolate leaf invocations when the host supports it.** Each sub-skill SHOULD run in a fresh model call or child context containing only its assigned source paths, READ/DO contracts, the leaf instructions, the complete bounded domain catalog per READ, and articles that leaf worklists. Preserve each catalog row's exact `path`; the leaf must copy references from that catalog.
- **Keep run artifacts private.** Before dispatch, allocate a new GUID-named directory under the current session's artifact directory and a distinct scratch/report child directory for every leaf. Pass a leaf only its own assigned source paths and child directory, never the run root or sibling paths. A leaf MUST NOT discover, enumerate, read, modify, or delete sibling artifacts. Do not reuse a prior run directory, and do not clean up any run artifact until every leaf has finished and consolidation is complete.
- **Keep raw Task transport distinct from the accepted copy.** Capture the exact Task return as the immutable raw audit payload and primary transport. Preserve it unchanged in the leaf's private artifacts or host log. Then apply DO's bounded pre-gate range normalization, when eligible, and its full consumer acceptance gate. The report accepted for rollup is the exact return when no normalization occurred, or the normalized candidate copy when DO permits it; worker-side persistence of another report file is optional and redundant.
- **Treat automatic output spills as host-owned.** If the host reports that a Task return was automatically spilled, the coordinator MAY read that file read-only only at the exact path returned by the tool. Never modify, delete, enumerate around, or reuse an automatic spill path. Never bypass a content-exclusion or access denial.
- Treat each sub-skill in the worklist as its own pass: read the sub-skill's instructions, apply its Source → Relevance → Worklist → Action steps to the orchestrator-supplied inputs, and produce that sub-skill's complete findings-report independently.
- Do not collapse multiple sub-skills into one shared reasoning step. Each sub-skill has a distinct knowledge subset and a distinct evaluation procedure; sharing one rolled-up scan dilutes per-skill attention and causes leaves to silently underreport (this has been observed in production: leaf skills returned empty `findings[]` while their standalone runs against the same diff produced multiple matches).
- The agent self-review pass is its own final iteration. Begin it only after every sub-skill in the worklist has completed and its sub-result is recorded.
- Sub-skills are independent: re-walking the diff once per sub-skill is correct and expected. The output schema accommodates this — `sub-results` carries one entry per sub-skill, each a complete findings-report, in the frontmatter `sub-skills` order regardless of completion order.
- When isolated calls are unavailable and the current model cannot finish every leaf within its budget, return `partial` with completed `sub-results` and name the first unevaluated sub-skill in `outcome-reason`. Never silently mark the remaining leaves clean.

### Roll up sub-skill findings

For each sub-skill in the worklist:

1. Invoke the sub-skill with the orchestrator's inputs, passing only the subset each sub-skill declares in its `inputs`.
2. Capture the exact Task return as the immutable raw audit payload and primary transport. Preserve it unchanged in the leaf's private artifacts or host log before deriving a candidate. Apply only DO's bounded pre-gate normalization: when the complete raw report has no other defect, a finding has positive-integer `line`, `start-line`, and `end-line`, `start-line <= line <= end-line`, `start-line != line`, and no `suggested-code` field, copy the complete report and remove only that finding's optional `location.range`. Record the normalization separately in private run telemetry or artifacts, never in the findings-report. Validate the entire candidate through DO's existing strict acceptance gate. Accept the exact return when unchanged or the normalized candidate when it passes; otherwise record a separate failed validation result with no findings for rollup. Do not reconstruct JSON, infer fields, alter paths or references, clamp lines, normalize reversed or out-of-bounds ranges, remove a range associated with `suggested-code`, or salvage individual findings.
3. Append the accepted findings-report, or the separate failed validation result, to `sub-results`. If its `outcome` is `failed`, stop here for this sub-skill: its findings are not reliable per the DO contract and MUST NOT be copied into the super-skill's top-level `findings[]` or counted in `summary.counts`.
4. Otherwise, compare each entry from the sub-skill's `findings[]` with findings already rolled up. Two findings are duplicates when they point to the same file and overlapping line/range and prescribe materially the same correction, even when their knowledge-file IDs differ. Merge duplicates instead of appending both: keep the more specific domain owner, preserve that finding's optional `domain` field verbatim (including its absence), use its reference as `references[0]` and therefore as `id`, append the other references as supporting references, keep the highest severity and confidence justified by either report, and preserve one self-contained message. Article and leaf ownership notes decide specificity; do not choose by execution order.
5. Append each non-duplicate finding, setting `from-sub-skill` to the sub-skill's `skill.id` and preserving its optional `domain` field verbatim, including its absence. For non-citation findings (those whose `id` is a skill-defined slug rather than a reference path), prefix `id` with `<from-sub-skill>:` to prevent collisions across sub-skills. Other finding fields are preserved.

### Agent self-review pass

Each leaf sub-skill emits both knowledge-backed findings and, per its own contract, agent findings within its own domain. After every sub-skill has produced its sub-result, perform a super-skill self-review pass against the same task input. The goal of this pass is to surface defects the agent recognises on its own that **no single leaf could have surfaced** because they cross domain boundaries — architecture-level issues that touch performance and reliability at once, error-handling gaps that span security and UX, resource-lifecycle patterns that affect both correctness and performance, and similar cross-cutting concerns. BCQuality is an **additive** knowledge layer: it augments the agent's review judgement, it does not replace it.

The self-review *reasoning* is mandatory — you MUST actually perform the cross-cutting analysis on every real-size PR, not skip it. But the pass need not produce any output: emitting **zero** agent findings is a valid, expected outcome whenever no candidate clears the precision bar in `skills/do.md` (*Agent findings*). Do not invent or pad findings to prove the pass ran. Always reason; emit only what survives the bar.

Frame the pass by cross-cutting concerns — architecture, error handling, resource lifecycle — and by the seams between leaf domains. Do not duplicate domain-specific reasoning that belongs in a leaf: a security-only concern is the security leaf's responsibility, not the super-skill's. The super-skill pass adds value where no individual leaf has the right scope.

For every candidate the agent identifies in this pass:

1. **Validate against BCQuality knowledge.** Check the candidate against the knowledge files the sub-skills have already loaded for this task (visible via their `references` and `suppressed` lists in `sub-results`).
   - If a BCQuality knowledge file matches the candidate, upgrade it to a knowledge-backed finding: cite the file in `references`, set `id` to the file's path, set `from-sub-skill` to the sub-skill that owns that knowledge domain, set `domain` to the human-readable label required by that sub-skill's Output contract, and merge with or deduplicate against any sub-skill finding that already covers the same concern at the same location.
   - If a BCQuality knowledge file **explicitly contradicts** the candidate (its `## Best Practice` or `## Anti Pattern` says the opposite of what the agent flagged), suppress the candidate and do not surface it.
   - Otherwise the candidate has no BCQuality coverage; emit it as a super-skill agent finding.
2. **Emit agent finding.** Per DO's *Agent findings* rules:
   - `from-sub-skill: "agent"` (the super-skill itself produced it)
   - `domain: "Agent"` (the display label for super-skill cross-cutting findings)
   - `references: []`
   - `id` is a skill-defined slug prefixed with `agent:` (for example, `agent:missing-error-handling-on-http-call`).
   - `confidence` capped at `medium`.
   - `severity` capped at `minor` — agent findings are advisory and non-gating per `skills/do.md`; never assign `major` or `blocker` to a finding with no knowledge file behind it.
   - `message` is non-empty and self-contained, describing both the issue and a concrete recommendation. A consumer rendering the finding has no knowledge-file footer to fall back on.
   - `suggested-code` MUST be set when the fix is small, local, and mechanical. If a mechanical-looking finding omits it, set `suggested-code-omission-reason` with the reason (for example, the fix spans non-contiguous code or requires choosing a real production value).

Leaf-level agent findings (those with `references: []` inside a sub-skill's report) are rolled up into the super-skill's top-level `findings[]` like any other sub-skill finding — they keep their `from-sub-skill: <leaf-id>` attribution and are not rewritten. They are not subject to the "MUST validate against knowledge" step above, because each leaf has already validated within its own domain.

### Suggested-code guidance

For both knowledge-backed findings rolled up from sub-skills and agent findings emitted in the self-review pass, populate `findings[].suggested-code` whenever a concrete code replacement is unambiguous from the diff context. This is a MUST for small, local, mechanical fixes. The payload MUST be a literal replacement for the source lines covered by `location` (typically a single line, or the line range in `location.range`) — no diff markers, fences, or commentary. Examples of good candidates: deleting dead code after `exit`, replacing `Count() > 0` with `not IsEmpty()`, moving an inline `Label` declaration to a codeunit-level `var` block, adding a missing property, replacing string-concatenated `Error`, changing an over-broad permission token, fixing whitespace or keyword casing. Skip `suggested-code` only when the fix requires choosing between multiple defensible alternatives, when the fix spans non-contiguous code, or when the surrounding context the agent cannot see could change the answer. If a mechanical-looking finding omits `suggested-code`, set `suggested-code-omission-reason`.

Sub-skills MAY also emit `suggested-code` when their knowledge file unambiguously implies the replacement (the `.good.al` and `.bad.al` companion examples are useful here). The super-skill copies the field through unchanged.

### Summary and rollup

Calculate `summary.counts` from the final top-level `findings[]`, after failed sub-results have been excluded and duplicates have been merged. Aggregate `summary.coverage` as the sums across invoked sub-skills whose `outcome` is not `failed`. Agent findings emitted by the super-skill itself contribute to `summary.counts` but not to `summary.coverage` (coverage is a sub-skill worklist metric and is undefined for self-review).

`suppressed[]` at the super-skill level remains empty. Knowledge-file-level suppression is reported by each sub-skill within its own entry in `sub-results`.

Derive `outcome` using the DO rollup rules. `outcome-reason` is populated for `partial` and `failed` and SHOULD summarize per-sub-skill state, for example: *"al-security-review failed (tool timeout); al-performance-review completed."*

Before emitting the rollup, apply DO's consumer acceptance gate to every nested
and top-level finding. A leaf's nested report is its accepted exact return or
its accepted normalized candidate copy; its exact Task return remains the
separate immutable raw audit payload. Treat an invalid sub-result as failed and
exclude all of its findings from the top-level rollup. Never reconstruct it
into a success-shaped report or perform normalization beyond DO's bounded
exception.

## Output

Output conforms to the DO output contract, extended with `sub-results` and `skipped-sub-skills`. A populated example — both leaves ran, each produced findings:

```json
{
  "skill": { "id": "al-code-review", "version": 1 },
  "outcome": "completed",
  "summary": {
    "counts": { "blocker": 1, "major": 1, "minor": 3, "info": 0 },
    "coverage": { "worklist-size": 4, "items-evaluated": 4 }
  },
  "findings": [
    {
      "id": "microsoft/knowledge/performance/apply-filters-before-iterating.md",
      "severity": "major",
      "message": "The Country/Region Code predicate is evaluated inside the loop instead of with SetRange before FindSet, so every row crosses the database boundary.",
      "location": {
        "file": "src/Sales/PostingRoutines.Codeunit.al",
        "line": 140,
        "range": { "start-line": 140, "end-line": 144 }
      },
      "references": [
        { "path": "microsoft/knowledge/performance/apply-filters-before-iterating.md" }
      ],
      "confidence": "high",
      "from-sub-skill": "al-performance-review",
      "domain": "Performance"
    },
    {
      "id": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md",
      "severity": "minor",
      "message": "The loop reads only a small subset of fields from a wide table without SetLoadFields, transferring every column for each row.",
      "location": {
        "file": "src/Sales/PostingRoutines.Codeunit.al",
        "line": 152
      },
      "references": [
        { "path": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md" }
      ],
      "confidence": "high",
      "from-sub-skill": "al-performance-review",
      "domain": "Performance"
    },
    {
      "id": "microsoft/knowledge/security/secrettext-for-credentials.md",
      "severity": "blocker",
      "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/secrettext-for-credentials.md" }
      ],
      "confidence": "high",
      "from-sub-skill": "al-security-review",
      "domain": "Security"
    },
    {
      "id": "microsoft/knowledge/security/secrets-isolated-storage.md",
      "severity": "minor",
      "message": "A setup table stores an API key in an ordinary Text field, exposing it through table reads and exports. Persist it in IsolatedStorage instead.",
      "location": {
        "file": "src/Integration/ExternalServiceSetup.Table.al",
        "line": 12
      },
      "references": [
        { "path": "microsoft/knowledge/security/secrets-isolated-storage.md" }
      ],
      "confidence": "medium",
      "from-sub-skill": "al-security-review",
      "domain": "Security"
    },
    {
      "id": "agent:missing-error-handling-on-http-client",
      "severity": "minor",
      "message": "HttpClient.Send is called without inspecting the response status or wrapping the call in a TryFunction. Network or remote-server failures will surface as runtime errors to the user. Recommendation: branch on the HttpResponseMessage.IsSuccessStatusCode and either retry, surface a controlled error, or fall back, depending on the integration's contract.",
      "location": {
        "file": "src/Integration/ApiClient.Codeunit.al",
        "line": 60,
        "range": { "start-line": 60, "end-line": 64 }
      },
      "references": [],
      "confidence": "medium",
      "from-sub-skill": "agent",
      "domain": "Agent"
    }
  ],
  "suppressed": [],
  "sub-results": [
    {
      "skill": { "id": "al-performance-review", "version": 1 },
      "outcome": "completed",
      "summary": {
        "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 },
        "coverage": { "worklist-size": 2, "items-evaluated": 2 }
      },
      "findings": [
        {
          "id": "microsoft/knowledge/performance/apply-filters-before-iterating.md",
          "severity": "major",
          "message": "The Country/Region Code predicate is evaluated inside the loop instead of with SetRange before FindSet, so every row crosses the database boundary.",
          "location": {
            "file": "src/Sales/PostingRoutines.Codeunit.al",
            "line": 140,
            "range": { "start-line": 140, "end-line": 144 }
          },
          "references": [
            { "path": "microsoft/knowledge/performance/apply-filters-before-iterating.md" }
          ],
          "confidence": "high",
          "domain": "Performance"
        },
        {
          "id": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md",
          "severity": "minor",
          "message": "The loop reads only a small subset of fields from a wide table without SetLoadFields, transferring every column for each row.",
          "location": {
            "file": "src/Sales/PostingRoutines.Codeunit.al",
            "line": 152
          },
          "references": [
            { "path": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md" }
          ],
          "confidence": "high",
          "domain": "Performance"
        }
      ],
      "suppressed": []
    },
    {
      "skill": { "id": "al-security-review", "version": 1 },
      "outcome": "completed",
      "summary": {
        "counts": { "blocker": 1, "major": 0, "minor": 1, "info": 0 },
        "coverage": { "worklist-size": 2, "items-evaluated": 2 }
      },
      "findings": [
        {
          "id": "microsoft/knowledge/security/secrettext-for-credentials.md",
          "severity": "blocker",
          "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/secrettext-for-credentials.md" }
          ],
          "confidence": "high",
          "domain": "Security"
        },
        {
          "id": "microsoft/knowledge/security/secrets-isolated-storage.md",
          "severity": "minor",
          "message": "A setup table stores an API key in an ordinary Text field, exposing it through table reads and exports. Persist it in IsolatedStorage instead.",
          "location": {
            "file": "src/Integration/ExternalServiceSetup.Table.al",
            "line": 12
          },
          "references": [
            { "path": "microsoft/knowledge/security/secrets-isolated-storage.md" }
          ],
          "confidence": "medium",
          "domain": "Security"
        }
      ],
      "suppressed": []
    }
  ]
}
```

When the selected leaves find no applicable knowledge, the result rolls up to
`no-knowledge`. This example shows two leaf results; a full run includes every
invoked leaf:

```json
{
  "skill": { "id": "al-code-review", "version": 1 },
  "outcome": "no-knowledge",
  "summary": {
    "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 },
    "coverage": { "worklist-size": 0, "items-evaluated": 0 }
  },
  "findings": [],
  "suppressed": [],
  "sub-results": [
    {
      "skill": { "id": "al-performance-review", "version": 1 },
      "outcome": "no-knowledge",
      "summary": { "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, "coverage": { "worklist-size": 0, "items-evaluated": 0 } },
      "findings": [],
      "suppressed": []
    },
    {
      "skill": { "id": "al-security-review", "version": 1 },
      "outcome": "no-knowledge",
      "summary": { "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, "coverage": { "worklist-size": 0, "items-evaluated": 0 } },
      "findings": [],
      "suppressed": []
    }
  ]
}
```