From 8901b83e076bc25663efd5a4bf8891b8ca209f75 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Tue, 23 Jun 2026 10:35:24 +0200 Subject: [PATCH 1/2] 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> --- README.md | 2 +- ...alidation-errors-with-errorbehavior.bad.al | 21 +++ ...lidation-errors-with-errorbehavior.good.al | 37 +++++ ...ct-validation-errors-with-errorbehavior.md | 26 ++++ ...-internal-vs-client-for-diagnostics.bad.al | 14 ++ ...internal-vs-client-for-diagnostics.good.al | 19 +++ ...type-internal-vs-client-for-diagnostics.md | 26 ++++ ...fer-errorinfo-for-actionable-errors.bad.al | 21 +++ ...er-errorinfo-for-actionable-errors.good.al | 44 ++++++ .../prefer-errorinfo-for-actionable-errors.md | 26 ++++ microsoft/skills/review/al-code-review.md | 4 +- .../skills/review/al-error-handling-review.md | 136 ++++++++++++++++++ 12 files changed, 374 insertions(+), 2 deletions(-) create mode 100644 microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.bad.al create mode 100644 microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.good.al create mode 100644 microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.md create mode 100644 microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.bad.al create mode 100644 microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al create mode 100644 microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md create mode 100644 microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.bad.al create mode 100644 microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.good.al create mode 100644 microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md create mode 100644 microsoft/skills/review/al-error-handling-review.md diff --git a/README.md b/README.md index e7a180e..01f85c1 100644 --- a/README.md +++ b/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/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 diff --git a/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.bad.al b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.bad.al new file mode 100644 index 0000000..cdb5fa9 --- /dev/null +++ b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.bad.al @@ -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; +} diff --git a/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.good.al b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.good.al new file mode 100644 index 0000000..dcd64b9 --- /dev/null +++ b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.good.al @@ -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; +} diff --git a/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.md b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.md new file mode 100644 index 0000000..6cc891c --- /dev/null +++ b/microsoft/knowledge/error-handling/collect-validation-errors-with-errorbehavior.md @@ -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`. diff --git a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.bad.al b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.bad.al new file mode 100644 index 0000000..4e6be07 --- /dev/null +++ b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.bad.al @@ -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; +} diff --git a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al new file mode 100644 index 0000000..9791909 --- /dev/null +++ b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.good.al @@ -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; +} diff --git a/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md new file mode 100644 index 0000000..7264ad6 --- /dev/null +++ b/microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md @@ -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`. diff --git a/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.bad.al b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.bad.al new file mode 100644 index 0000000..bfd624a --- /dev/null +++ b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.bad.al @@ -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; +} diff --git a/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.good.al b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.good.al new file mode 100644 index 0000000..8880743 --- /dev/null +++ b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.good.al @@ -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; +} diff --git a/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md new file mode 100644 index 0000000..54ef285 --- /dev/null +++ b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md @@ -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`. diff --git a/microsoft/skills/review/al-code-review.md b/microsoft/skills/review/al-code-review.md index f54ff2b..80bd19c 100644 --- a/microsoft/skills/review/al-code-review.md +++ b/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, 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. diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md new file mode 100644 index 0000000..91228aa --- /dev/null +++ b/microsoft/skills/review/al-error-handling-review.md @@ -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": [] +} +``` From 65f6f2215725c4c52a8d488005a3fdb39ecedeba Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Tue, 23 Jun 2026 11:20:11 +0200 Subject: [PATCH 2/2] Add open-ended bc-version range shorthand [N..] A closed range like [23..28] wrongly implies guidance stops applying after BC28, so a reviewer targeting BC29+ would not match the file. Introduce an open-ended shorthand [N..] meaning ''version N and every later version''. - validate_frontmatter.py: RANGE_SHORTHAND allows an optional upper bound; expand_bc_version returns the normalized string ''N..'' for open-ended. - read.md: document the fourth bc-version form and its matching rule (matches target >= N; not enumerable). - write.md: prefer [N..] over a closed range for a feature introduced in N and not expected to be removed. - Apply [23..] to the actionable-errors article (actionable errors shipped in BC23 and are not version-bounded above). - README: mention [N..] in the frontmatter example. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/scripts/validate_frontmatter.py | 14 ++++++++++---- README.md | 2 +- .../prefer-errorinfo-for-actionable-errors.md | 2 +- skills/read.md | 11 ++++++----- skills/write.md | 2 +- 5 files changed, 19 insertions(+), 12 deletions(-) diff --git a/.github/scripts/validate_frontmatter.py b/.github/scripts/validate_frontmatter.py index d969cef..4d9bdc8 100644 --- a/.github/scripts/validate_frontmatter.py +++ b/.github/scripts/validate_frontmatter.py @@ -59,7 +59,7 @@ MAX_KNOWLEDGE_LINES = 100 KEBAB_CASE = re.compile(r"^[a-z0-9]+(-[a-z0-9]+)*$") ISO_ALPHA2 = re.compile(r"^[a-z]{2}$") -RANGE_SHORTHAND = re.compile(r"^(\d+)\.\.(\d+)$") +RANGE_SHORTHAND = re.compile(r"^(\d+)\.\.(\d+)?$") FENCED_CODE_BLOCK = re.compile(r"^```", re.MULTILINE) HEADING_H2 = re.compile(r"^##\s+(.+?)\s*$", re.MULTILINE) @@ -149,6 +149,8 @@ def expand_bc_version(value: Any) -> tuple[list[int] | str | None, str | None]: """Return (expanded, error-message). One of the two is None. For the universal sentinel ["all"], `expanded` is the string "all". + For an open-ended range like ["26.."], `expanded` is the normalized + string "26.." (it cannot be enumerated; consumers match target >= 26). Otherwise it is the expanded list of version integers. """ if not isinstance(value, list) or not value: @@ -163,15 +165,19 @@ def expand_bc_version(value: Any) -> tuple[list[int] | str | None, str | None]: if any(v <= 0 for v in value): return None, "integers must be positive" return sorted(set(value)), None - # Case 2: single-element range-shorthand like "[26..28]" + # Case 2: single-element range shorthand — closed "[26..28]" or open-ended "[26..]" if len(value) == 1 and isinstance(value[0], str): m = RANGE_SHORTHAND.match(value[0].strip()) if m: - start, end = int(m.group(1)), int(m.group(2)) + start = int(m.group(1)) + if m.group(2) is None: + # Open-ended: "start.." applies from start onwards, no upper bound. + return f"{start}..", None + end = int(m.group(2)) if start > end: return None, f"range '{value[0]}' is not ascending" return list(range(start, end + 1)), None - return None, "must be [all], a list of integers, or a single-element range shorthand like [26..28]" + return None, "must be [all], a list of integers, or a range shorthand like [26..28] or [26..]" def headings_in_order(body: str) -> list[tuple[str, int]]: diff --git a/README.md b/README.md index 01f85c1..ecddd1d 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ Every knowledge file is a markdown file with mandatory YAML frontmatter. Files t ```yaml --- -bc-version: [all] # or [26..28] for version-gated guidance +bc-version: [all] # or [26..28], or [26..] for "26 and later" domain: performance # security | performance | ux | telemetry | ... keywords: [query, filtering, partial] # free-text tags for retrieval technologies: [al] # al | javascript | powershell | ... diff --git a/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md index 54ef285..f774cc8 100644 --- a/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md +++ b/microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md @@ -1,5 +1,5 @@ --- -bc-version: [23..28] +bc-version: [23..] domain: error-handling keywords: [errorinfo, actionable-errors, fix-it, show-it, addaction, addnavigationaction, error-dialog] technologies: [al] diff --git a/skills/read.md b/skills/read.md index dbeaa63..8badb97 100644 --- a/skills/read.md +++ b/skills/read.md @@ -26,7 +26,7 @@ A file that violates any of these rules is invalid and MUST be skipped by consum ```yaml --- -bc-version: [all] # or [26, 27, 28] or the range shorthand [26..28] +bc-version: [all] # or [26, 27, 28], the range [26..28], or the open-ended range [26..] domain: performance keywords: [query, filtering, partial] technologies: [al] @@ -39,13 +39,14 @@ All six fields are required. Missing or empty fields invalidate the file. ### Fields -**`bc-version`** — Array. The Business Central major versions this file applies to. Three forms are accepted: +**`bc-version`** — Array. The Business Central major versions this file applies to. Four forms are accepted: - Universal sentinel: `[all]` means the guidance applies to every BC version and matches any target. - Explicit list: `[26, 27, 28]`. -- Range shorthand: `[26..28]` means every integer from 26 through 28 inclusive. +- Closed range shorthand: `[26..28]` means every integer from 26 through 28 inclusive. +- Open-ended range shorthand: `[26..]` means version 26 and every later version, with no upper bound. Use it for guidance tied to a feature introduced in a specific version that is not expected to be removed. -`[all]` is mutually exclusive with explicit versions; do not combine. Consumers MUST expand ranges to the full set before comparison. +`[all]` is mutually exclusive with explicit versions; do not combine. Consumers MUST expand closed ranges to the full set before comparison; an open-ended range `[N..]` is not enumerable and instead matches any target version greater than or equal to `N`. **`domain`** — String. A single domain tag that places the file within a broader area of concern. Standard values include `performance`, `security`, `ux`, `telemetry`, `testing`, `api`, `pipelines`, `finance`, `supply-chain`, `manufacturing`, `jobs`. New domains may be introduced by contributors; no closed enumeration is enforced at the schema level. Consumers MUST treat unknown domains as valid. @@ -94,7 +95,7 @@ Conflict detection is the consumer's responsibility; BCQuality does not enforce When a consumer filters or matches files against a task context, these rules apply: -- **`bc-version`** — the file matches if its set is `[all]`, or if the target BC version is an element of the file's expanded `bc-version` set. Range shorthand (`[26..28]`) MUST be expanded before comparison. +- **`bc-version`** — the file matches if its set is `[all]`, or if the target BC version is an element of the file's expanded `bc-version` set. Closed range shorthand (`[26..28]`) MUST be expanded before comparison; an open-ended range (`[26..]`) matches when the target BC version is greater than or equal to its lower bound. - **`technologies`** — non-empty intersection between the task's technologies and the file's technologies. There is no sentinel for this field. - **`countries`** — the file matches if its set contains `w1`, or if there is a non-empty intersection with the task's countries. - **`application-area`** — the file matches if its set contains `all`, or if there is a non-empty intersection with the task's application areas. diff --git a/skills/write.md b/skills/write.md index 6fe2eee..9a3b208 100644 --- a/skills/write.md +++ b/skills/write.md @@ -43,7 +43,7 @@ Knowledge files do not contain code. Samples live as **sibling files** next to t ## Choosing frontmatter values -**`bc-version`.** Default to `[all]` when the guidance is universal — a BC language pattern, a property on a long-standing platform type, a CodeCop rule, or a platform behaviour that has not changed across versions. Use an explicit list or range (`[26, 27, 28]`, `[26..28]`) only when the guidance is tied to a version-gated API, a deprecation, or platform behaviour that genuinely differs across versions. Most knowledge files should be `[all]`; reach for a range only with a concrete reason. +**`bc-version`.** Default to `[all]` when the guidance is universal — a BC language pattern, a property on a long-standing platform type, a CodeCop rule, or a platform behaviour that has not changed across versions. Use an explicit list or range (`[26, 27, 28]`, `[26..28]`) only when the guidance is tied to a version-gated API, a deprecation, or platform behaviour that genuinely differs across versions. When guidance applies to a feature introduced in version N and not expected to be removed, prefer the open-ended range `[N..]` over a closed range so the file keeps matching future versions — reserve a closed upper bound for guidance that genuinely stops applying (for example, a behaviour removed or replaced in a later version). Most knowledge files should be `[all]`; reach for a range only with a concrete reason. **`domain`.** Pick one. If two fit, the file is probably two concerns. If no existing domain fits, introduce a new one — domains are open. Prefer existing domains when they are a reasonable fit, to keep retrieval predictable.