Add error-handling knowledge domain and review leaf skill

Seed the first new AL knowledge domain toward issue #34: a fully wired
error-handling domain that the review pipeline actually consumes.

- 3 knowledge articles (+ good/bad AL samples):
  - prefer-errorinfo-for-actionable-errors (ErrorInfo Fix-it/Show-it actions)
  - collect-validation-errors-with-errorbehavior (ErrorBehavior::Collect)
  - errortype-internal-vs-client-for-diagnostics (ErrorType Internal vs Client)
- New leaf skill al-error-handling-review.md, modeled on al-performance-review
- Wire the leaf into al-code-review (sub-skills, Source, description)
- README: six -> seven leaf skills

Validated: frontmatter validator clean; knowledge-index check passes
(deterministic, full coverage, selection inputs intact).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Copilot 2026-06-23 10:35:24 +02:00
parent 822cae1b27
commit 8901b83e07
12 changed files with 374 additions and 2 deletions

View file

@ -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/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).
- **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 seven leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) — one per knowledge domain (performance, security, privacy, upgrade, style, UI, error handling).
### Agent bootstrapping

View file

@ -0,0 +1,21 @@
codeunit 50187 "Collect Errors Bad Sample"
{
procedure ValidateAllItems()
var
Item: Record Item;
ErrorText: Text;
begin
// Hand-rolled accumulation: reimplements the platform feature, loses each
// error's ErrorInfo structure, and skips telemetry classification.
if Item.FindSet() then
repeat
if Item.Description = '' then
ErrorText += StrSubstNo('Item %1 has no description.\', Item."No.");
if Item."Unit Cost" <= 0 then
ErrorText += StrSubstNo('Item %1 must have a positive unit cost.\', Item."No.");
until Item.Next() = 0;
if ErrorText <> '' then
Error(ErrorText);
end;
}

View file

@ -0,0 +1,37 @@
codeunit 50185 "Collect Errors Good Sample"
{
[ErrorBehavior(ErrorBehavior::Collect)]
procedure ValidateAllItems()
var
Item: Record Item;
CollectedErrors: List of [ErrorInfo];
CollectedError: ErrorInfo;
ErrorText: Text;
begin
if Item.FindSet() then
repeat
// Run each item in its own context so one failure does not abandon the rest.
Codeunit.Run(Codeunit::"Collect Errors Item Check", Item);
until Item.Next() = 0;
if HasCollectedErrors() then begin
CollectedErrors := GetCollectedErrors();
foreach CollectedError in CollectedErrors do
ErrorText += CollectedError.Message() + '\';
Message('The following must be fixed before posting:\%1', ErrorText);
end;
end;
}
codeunit 50186 "Collect Errors Item Check"
{
TableNo = Item;
trigger OnRun()
begin
if Rec.Description = '' then
Error('Item %1 has no description.', Rec."No.");
if Rec."Unit Cost" <= 0 then
Error('Item %1 must have a positive unit cost.', Rec."No.");
end;
}

View file

@ -0,0 +1,26 @@
---
bc-version: [all]
domain: error-handling
keywords: [collectible-errors, errorbehavior, collect, getcollectederrors, hascollectederrors, validation, batch]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Collect validation errors with ErrorBehavior::Collect and handle the collected list
## Description
By default a procedure stops on the first `Error`, so a user fixing ten bad rows must rerun the operation ten times. The collectible-errors feature postpones error handling to the end of the call: a procedure attributed `[ErrorBehavior(ErrorBehavior::Collect)]` keeps running as errors occur and gathers them, so all failures can be presented together. The collected errors are read with `HasCollectedErrors()` and `GetCollectedErrors()` (which returns a `List of [ErrorInfo]`); `ClearCollectedErrors()` empties the buffer. This is a platform mechanism most LLMs are unaware of — they reach for a manually concatenated `Text` buffer or a temporary error table instead.
## Best Practice
Mark the orchestrating procedure `[ErrorBehavior(ErrorBehavior::Collect)]` and run each item's validation so one failure doesn't abandon the rest — typically by calling the per-item routine through `Codeunit.Run`. When the run finishes, inspect `HasCollectedErrors()` and surface `GetCollectedErrors()` to the user as a single list. Always handle the collected errors yourself: the platform's own guidance is that any errors still in the collected list when the procedure ends are concatenated into one dialog, which is hard for users to read.
See sample: `collect-validation-errors-with-errorbehavior.good.al`.
## Anti Pattern
Two shapes signal trouble. The first is hand-rolled accumulation — appending messages to a `Text` variable and showing them at the end — which reimplements the platform feature, loses each error's `ErrorInfo` structure, and skips telemetry classification. The second is applying `[ErrorBehavior(ErrorBehavior::Collect)]` but never calling `HasCollectedErrors`/`GetCollectedErrors`, so every collected error spills into the platform's concatenated end-of-procedure dialog. Detection: a `Collect` attribute with no matching `GetCollectedErrors` call, or a per-row loop that builds an error string by concatenation.
See sample: `collect-validation-errors-with-errorbehavior.bad.al`.

View file

@ -0,0 +1,14 @@
codeunit 50191 "Error Type Bad Sample"
{
procedure ApplyLedgerBucket(BucketId: Integer)
begin
// Developer-facing detail shown straight to the user, and no structured telemetry signal.
if not BucketInitialized(BucketId) then
Error('Unexpected state: ledger bucket %1 not initialized', BucketId);
end;
local procedure BucketInitialized(BucketId: Integer): Boolean
begin
exit(false);
end;
}

View file

@ -0,0 +1,19 @@
codeunit 50190 "Error Type Good Sample"
{
procedure ApplyLedgerBucket(BucketId: Integer)
var
InternalErr: ErrorInfo;
begin
if not BucketInitialized(BucketId) then begin
InternalErr.ErrorType := ErrorType::Internal;
InternalErr.Message := StrSubstNo('Ledger bucket %1 was not initialized before posting.', BucketId);
InternalErr.DetailedMessage := 'Internal invariant violated. Inspect the call stack captured in telemetry.';
Error(InternalErr);
end;
end;
local procedure BucketInitialized(BucketId: Integer): Boolean
begin
exit(false);
end;
}

View file

@ -0,0 +1,26 @@
---
bc-version: [all]
domain: error-handling
keywords: [errorinfo, errortype, internal, client, telemetry, diagnostics, generic-message]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Set ErrorInfo.ErrorType to Internal for defects you want in telemetry but not in the user's face
## Description
`ErrorInfo.ErrorType` controls where an error's message is shown. With `ErrorType::Client` — the behaviour of a normal `Error` — the message is both shown to the user and sent to telemetry. With `ErrorType::Internal` the user sees a generic message while the specific message you set is sent to telemetry only. The distinction matters for *unexpected* failures — a broken invariant, a failed internal assertion, a "this should never happen" branch — where the technical detail helps the partner diagnose the defect but would only confuse the end user. LLMs are unaware `ErrorType` exists, so they expose raw internal-failure text directly to users.
## Best Practice
Reserve `ErrorType::Internal` for errors the user cannot act on: corrupted internal state, an unreachable branch, a contract a caller violated. Set a precise, detail-rich `Message` and `DetailedMessage` for telemetry, raise it via `Error(ErrorInfo)`, and let the platform show the user a generic dialog. Keep `ErrorType::Client` (or a plain `Error`) for failures the user is expected to read and resolve — validation messages, missing setup, business-rule violations. The test is simple: if the message only makes sense to a developer, mark it `Internal`.
See sample: `errortype-internal-vs-client-for-diagnostics.good.al`.
## Anti Pattern
Raising an internal failure with a plain `Error('Unexpected state: ledger bucket %1 not initialized', BucketId)`. The user is shown a technical message they can do nothing about, and the signal is buried in a generic error rather than carried as structured telemetry detail. Detection: an `Error` whose wording targets a developer ("unexpected", "should not happen", raw internal identifiers) raised with default `Client` visibility instead of an `ErrorInfo` marked `ErrorType::Internal`.
See sample: `errortype-internal-vs-client-for-diagnostics.bad.al`.

View file

@ -0,0 +1,21 @@
table 50182 "Actionable Error Bad Sample"
{
fields
{
field(1; "Entry No."; Integer) { }
field(2; "Qty. to Invoice"; Decimal)
{
trigger OnValidate()
begin
// Dead-end error: the code knows the maximum but offers the user no way to apply it.
if "Qty. to Invoice" > MaxQtyToInvoice() then
Error('You cannot invoice more than %1 units.', MaxQtyToInvoice());
end;
}
}
local procedure MaxQtyToInvoice(): Decimal
begin
exit(10);
end;
}

View file

@ -0,0 +1,44 @@
table 50180 "Actionable Error Good Sample"
{
fields
{
field(1; "Entry No."; Integer) { }
field(2; "Qty. to Invoice"; Decimal)
{
trigger OnValidate()
var
CannotInvoiceErr: ErrorInfo;
begin
if "Qty. to Invoice" > MaxQtyToInvoice() then begin
CannotInvoiceErr.Title := 'Qty. to Invoice isn''t valid';
CannotInvoiceErr.Message := StrSubstNo('You cannot invoice more than %1 units.', MaxQtyToInvoice());
CannotInvoiceErr.DetailedMessage := 'Reduce the quantity to invoice, or apply the maximum allowed.';
CannotInvoiceErr.RecordId := Rec.RecordId();
CannotInvoiceErr.AddAction(
StrSubstNo('Set value to %1', MaxQtyToInvoice()),
Codeunit::"Actionable Error Fixit Sample",
'SetQtyToMax');
Error(CannotInvoiceErr);
end;
end;
}
}
local procedure MaxQtyToInvoice(): Decimal
begin
exit(10);
end;
}
codeunit 50181 "Actionable Error Fixit Sample"
{
procedure SetQtyToMax(SourceError: ErrorInfo)
var
Line: Record "Actionable Error Good Sample";
begin
if Line.Get(SourceError.RecordId) then begin
Line.Validate("Qty. to Invoice", 10);
Line.Modify(true);
end;
end;
}

View file

@ -0,0 +1,26 @@
---
bc-version: [23..28]
domain: error-handling
keywords: [errorinfo, actionable-errors, fix-it, show-it, addaction, addnavigationaction, error-dialog]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Prefer ErrorInfo with recommended actions over a plain Error for recoverable failures
## Description
A plain `Error('text')` ends the operation with a dead-end dialog: the user reads the message but the system offers no way forward. The `ErrorInfo` data type, combined with the actionable-errors framework added in 2023 release wave 2, lets an error carry a recommended action the user can take to unblock themselves without leaving their task. Two kinds exist: a **Fix-it** action (`AddAction`), used when the code already knows the correct value and can apply it in one step, and a **Show-it** action (`AddNavigationAction` together with `PageNo`), used when the correction lives on a related record the user should be taken to. An error dialog renders at most two recommended actions. LLMs trained on older AL almost always emit a bare `Error(...)` and rarely reach for `ErrorInfo`, so this guidance is remedial.
## Best Practice
Build an `ErrorInfo`, set `Title`, `Message`, and `DetailedMessage`, then attach the action that matches the situation. For a Fix-it, call `AddAction(Caption, Codeunit::Handler, 'MethodName')` where the handler method (which receives the `ErrorInfo`) applies the known-good value; phrase the caption as "Set value to …". For a Show-it, set `PageNo := Page::"…"`, set `RecordId` so navigation opens the right record, and call `AddNavigationAction('Show …')`. Raise it with `Error(ErrorInfo)`. Reserve recommended actions for cases where the solution is genuinely known and the user has permission to apply it.
See sample: `prefer-errorinfo-for-actionable-errors.good.al`.
## Anti Pattern
Surfacing a recoverable validation failure with `Error('You cannot invoice more than %1 units.', MaxQty)` and nothing else. The user is blocked with no offered remedy even though the code knows the maximum and could set it. The detection signal: an `Error` call in a validation or posting path whose message names a specific correct value or a specific related page, with no surrounding `ErrorInfo`, `AddAction`, or `AddNavigationAction`. Replace it with an `ErrorInfo` that carries the corresponding Fix-it or Show-it action.
See sample: `prefer-errorinfo-for-actionable-errors.bad.al`.

View file

@ -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, privacy, upgrade, style, UI).
description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI, error handling).
inputs: [pr-diff, file-path]
outputs: [findings-report]
bc-version: [all]
@ -17,6 +17,7 @@ sub-skills:
- 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
---
# AL code review
@ -37,6 +38,7 @@ The sub-skills invoked by this skill are those listed in frontmatter `sub-skills
- `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`
Additional leaf skills (for example, telemetry, testing) are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly.

View file

@ -0,0 +1,136 @@
---
kind: action-skill
id: al-error-handling-review
version: 1
title: AL error handling review
description: Reviews AL source changes against error-handling guidance from BCQuality.
inputs: [pr-diff, file-path]
outputs: [findings-report]
bc-version: [all]
technologies: [al]
countries: [w1]
application-area: [all]
---
# AL error handling review
Reviews AL source changes against the `error-handling` 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
Read the BCQuality knowledge index once — the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone — see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint — exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `error-handling` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/error-handling/**`.
## 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 that post or validate, tables and table extensions with `OnValidate` triggers, and any procedure that raises errors or orchestrates a batch over records.
- The changed procedures and triggers, weighted toward `OnValidate`/`OnInsert`/`OnModify` triggers, posting and validation routines, and procedures attributed with `[ErrorBehavior(...)]`.
- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`).
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
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 error-handling knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable error-handling 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. 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.
After evaluating each worklist entry, also consider whether the diff exhibits an error-handling defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material error-handling defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly error handling; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract.
For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: replace a string-concatenated `Error` with a Label-backed call; mark an internal-only failure `ErrorType::Internal`; add a missing `DetailedMessage`). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`.
Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract.
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 error-handling 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-error-handling-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/error-handling/prefer-errorinfo-for-actionable-errors.md",
"severity": "major",
"message": "A validation error names the maximum allowed quantity but raises a plain Error with no recommended action. Use an ErrorInfo with a Fix-it AddAction so the user can apply the known value.",
"location": {
"file": "src/Sales/SalesLine.TableExt.al",
"line": 88,
"range": { "start-line": 86, "end-line": 89 }
},
"references": [
{ "path": "microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md" }
],
"confidence": "high"
},
{
"id": "microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md",
"severity": "minor",
"message": "This 'unexpected state' failure is developer-facing but is raised with default Client visibility. Mark it ErrorType::Internal so the detail goes to telemetry and the user sees a generic message.",
"location": {
"file": "src/Ledger/PostingEngine.Codeunit.al",
"line": 211
},
"references": [
{ "path": "microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md" }
],
"confidence": "medium"
}
],
"suppressed": []
}
```
The empty-corpus case — BCQuality's state until error-handling knowledge files land — produces:
```json
{
"skill": { "id": "al-error-handling-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": []
}
```