diff --git a/README.md b/README.md index 9b71c43..166cbfb 100644 --- a/README.md +++ b/README.md @@ -52,7 +52,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 seven leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) — one per knowledge domain (performance, security, privacy, upgrade, style, UI, error handling). +- **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 eight leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) — one per knowledge domain (performance, security, privacy, upgrade, style, UI, error handling, events). ### Agent bootstrapping diff --git a/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.bad.al b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.bad.al new file mode 100644 index 0000000..4e8ecc6 --- /dev/null +++ b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.bad.al @@ -0,0 +1,53 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50232 "Order Event Pub Bad Sample" +{ + procedure ReleaseOrder(OrderNo: Code[20]) + begin + OnAfterReleaseOrder(OrderNo); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterReleaseOrder(OrderNo: Code[20]) + begin + end; +} + +// Anti-pattern 1: a static subscriber drives an always-on side effect that +// should be scoped. Every release now emails the customer, in every session +// and every automated test, with no way to switch it off. +codeunit 50233 "Always Email Sub Bad Sample" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Order Event Pub Bad Sample", 'OnAfterReleaseOrder', '', false, false)] + local procedure SendEmailOnRelease(OrderNo: Code[20]) + begin + // Send a confirmation email unconditionally on every release. + end; +} + +codeunit 50234 "Scoped Sub Bad Sample" +{ + EventSubscriberInstance = Manual; + + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Order Event Pub Bad Sample", 'OnAfterReleaseOrder', '', false, false)] + local procedure OverrideRelease(OrderNo: Code[20]) + begin + // Scoped behaviour intended only for a specific flow. + end; +} + +// Anti-pattern 2: a manual subscriber is bound and never unbound. Because the +// instance is held on a SingleInstance global, the binding lives for the whole +// session, so later unrelated releases keep hitting the scoped subscriber. +codeunit 50235 "Leaky Binder Bad Sample" +{ + SingleInstance = true; + + var + Scoped: Codeunit "Scoped Sub Bad Sample"; + + procedure ActivateOverride() + begin + BindSubscription(Scoped); + // Missing: a matching UnbindSubscription(Scoped) when the scope ends. + end; +} diff --git a/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.good.al b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.good.al new file mode 100644 index 0000000..66e416b --- /dev/null +++ b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.good.al @@ -0,0 +1,52 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50228 "Item Post Pub Good Sample" +{ + procedure PostItemLine(ItemNo: Code[20]; Qty: Decimal) + begin + // ... post the line ... + OnAfterPostItemLine(ItemNo, Qty); + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterPostItemLine(ItemNo: Code[20]; Qty: Decimal) + begin + end; +} + +codeunit 50229 "Item Post Audit Good Sample" +{ + // Always-on behaviour belongs in a static subscriber (the default). + EventSubscriberInstance = StaticAutomatic; + + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Item Post Pub Good Sample", 'OnAfterPostItemLine', '', false, false)] + local procedure LogPostedLine(ItemNo: Code[20]; Qty: Decimal) + begin + // Audit every posted line, unconditionally. + end; +} + +codeunit 50230 "Item Post Stub Good Sample" +{ + // Scoped/temporary behaviour belongs in a manual subscriber. + EventSubscriberInstance = Manual; + + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Item Post Pub Good Sample", 'OnAfterPostItemLine', '', false, false)] + local procedure CaptureForTest(ItemNo: Code[20]; Qty: Decimal) + begin + // Record the call so a single test can assert on it. + end; +} + +codeunit 50231 "Item Post Test Good Sample" +{ + procedure VerifyPostingRaisesEvent() + var + Publisher: Codeunit "Item Post Pub Good Sample"; + Stub: Codeunit "Item Post Stub Good Sample"; + begin + // Activate the scoped subscriber only for the duration of the test. + BindSubscription(Stub); + Publisher.PostItemLine('1000', 5); + UnbindSubscription(Stub); + end; +} diff --git a/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md new file mode 100644 index 0000000..9fe7312 --- /dev/null +++ b/microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [event-subscriber, static-subscriber, manual-subscriber, bindsubscription, unbindsubscription, eventsubscriberinstance, scoped-binding] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Choose static vs manual subscribers deliberately and bind manual ones with BindSubscription + +## Description + +An `[EventSubscriber]` codeunit is static by default (`EventSubscriberInstance = StaticAutomatic`): it is always bound, so it fires for every raise of the event in every session. That is correct for always-on behaviour such as auditing, but wrong for behaviour that must be scoped — test isolation, a one-off migration, or a conditional override — because a static subscriber cannot be switched off. For scoped behaviour, set `EventSubscriberInstance = Manual` and activate the codeunit only while needed with `BindSubscription`, releasing it with `UnbindSubscription`. LLMs are largely unaware the manual model exists and default everything to static, producing always-on side effects that leak across unrelated operations and tests. + +## Best Practice + +Use a static subscriber for behaviour that genuinely applies all the time. For anything scoped, mark the codeunit `EventSubscriberInstance = Manual`, call `BindSubscription(SubscriberInstance)` at the start of the scope and `UnbindSubscription(SubscriberInstance)` at the end. A manual subscriber held only in a local variable unbinds automatically when that variable leaves scope, which suits test setup/teardown; a binding you intend to outlive a single call must be unbound explicitly. Keep subscriber methods `local` per CodeCop AA0207. + +See sample: `choose-static-vs-manual-subscribers-deliberately.good.al`. + +## Anti Pattern + +Two shapes. First, a static subscriber used for behaviour that should be scoped — an always-on side effect (sending mail, writing extra records) that now fires for every event in every session and test with no way to disable it. Second, a manual subscriber that is bound with `BindSubscription` and never unbound: when the instance is held beyond the intended scope (for example on a `SingleInstance` codeunit), the binding leaks for the whole session and later unrelated operations keep hitting it. Detection: scoped side effects on a static subscriber, or a `BindSubscription` call with no matching `UnbindSubscription` and no scope that releases the instance. + +See sample: `choose-static-vs-manual-subscribers-deliberately.bad.al`. diff --git a/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.bad.al b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.bad.al new file mode 100644 index 0000000..5942ebd --- /dev/null +++ b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.bad.al @@ -0,0 +1,36 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +table 50226 "Reservation Entry Bad Sample" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; Quantity; Decimal) { } + field(4; Reserved; Boolean) { } + } + keys + { + key(PK; "Entry No.") { Clustered = true; } + } +} + +codeunit 50227 "Reservation Post Bad Sample" +{ + procedure Reserve(var ReservationEntry: Record "Reservation Entry Bad Sample") + begin + // Anti-pattern: the operation exposes no OnBefore/OnAfter seam, and the + // logic that should be the routine's own work lives in the event body + // below instead. Partners must overwrite this routine to change it. + OnReserve(ReservationEntry); + end; + + // Anti-pattern: business logic inside an integration-event publisher. A + // publisher must be a thin, empty hook; logic placed here runs on every + // raise and cannot be overridden, which defeats the event entirely. + [IntegrationEvent(false, false)] + local procedure OnReserve(var ReservationEntry: Record "Reservation Entry Bad Sample") + begin + ReservationEntry.Reserved := true; + ReservationEntry.Modify(true); + end; +} diff --git a/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.good.al b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.good.al new file mode 100644 index 0000000..ef67a3e --- /dev/null +++ b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.good.al @@ -0,0 +1,43 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +table 50224 "Reservation Entry Sample" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(2; "Item No."; Code[20]) { } + field(3; Quantity; Decimal) { } + field(4; Reserved; Boolean) { } + } + keys + { + key(PK; "Entry No.") { Clustered = true; } + } +} + +codeunit 50225 "Reservation Post Good Sample" +{ + procedure Reserve(var ReservationEntry: Record "Reservation Entry Sample") + var + IsHandled: Boolean; + begin + OnBeforeReserve(ReservationEntry, IsHandled); + if IsHandled then + exit; + + ReservationEntry.Reserved := true; + ReservationEntry.Modify(true); + + OnAfterReserve(ReservationEntry); + end; + + // Thin publishers: empty bodies, the calling routine owns the logic. + [IntegrationEvent(false, false)] + local procedure OnBeforeReserve(var ReservationEntry: Record "Reservation Entry Sample"; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnAfterReserve(var ReservationEntry: Record "Reservation Entry Sample") + begin + end; +} diff --git a/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md new file mode 100644 index 0000000..30c94df --- /dev/null +++ b/microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [integration-event, onbefore, onafter, extension-point, thin-publisher, publisher-body, extensibility] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Publish thin OnBefore/OnAfter integration events to expose extension points + +## Description + +A key operation — a posting, release, or validation routine — becomes a hard wall for partners when it ships no integration events: the only way to change it is to overwrite or duplicate the base code. The Business Central remedy is to raise thin `OnBeforeX`/`OnAfterX` integration events at the operation's boundaries, passing `var Rec` and the relevant parameters so subscribers have what they need. An equally common defect is the inverse: putting business logic *inside* the publisher method body. An event publisher is a hook, not a procedure — its body must be empty, and the platform even forbids variables, return values, and code other than comments in it. LLMs both omit the extension points and, when they do add an event, wrongly fill its body with logic. + +## Best Practice + +Wrap the operation's core with events: raise `OnBeforeX(var Rec, var IsHandled)` before the default work and `OnAfterX(var Rec)` once it succeeds, at the natural boundaries of the routine. Declare each publisher `[IntegrationEvent(false, false)] local procedure` with an empty body and let the calling routine — never the publisher — own the logic. Pass records by `var` so subscribers can read and adjust them, and include the parameters a subscriber would need to act. This gives partners a stable seam without touching base code. + +See sample: `publish-thin-onbefore-onafter-integration-events.good.al`. + +## Anti Pattern + +Business logic placed inside an `[IntegrationEvent]` publisher method, so the "event" actually mutates state every time it is raised — defeating the hook and surprising every reader — or a core operation that exposes no extension points at all, forcing partners to overwrite or duplicate it. Detection: an `[IntegrationEvent]`/`[BusinessEvent]` method whose body contains statements rather than being empty, or a posting/validation routine with no surrounding `OnBefore`/`OnAfter` publishers. + +See sample: `publish-thin-onbefore-onafter-integration-events.bad.al`. diff --git a/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.bad.al b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.bad.al new file mode 100644 index 0000000..766a30a --- /dev/null +++ b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.bad.al @@ -0,0 +1,38 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. + +// Anti-pattern 1: no OnBefore/IsHandled hook. A partner cannot replace this +// rule without overwriting base code, so the behaviour is not extensible. +codeunit 50222 "Shipping Charge NoHook Bad" +{ + procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal + begin + if OrderAmount >= 1000 then + Charge := 0 + else + Charge := 49; + end; +} + +// Anti-pattern 2: the hook exists but the 'if IsHandled then exit;' guard is +// missing, so the default logic still runs after a subscriber handled the call. +codeunit 50223 "Shipping Charge Guard Bad" +{ + procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal + var + IsHandled: Boolean; + begin + OnBeforeCalculateShippingCharge(OrderAmount, Charge, IsHandled); + + // Bug: no 'if IsHandled then exit;' here. Even when a subscriber set + // Charge and IsHandled := true, the default below overwrites the result. + if OrderAmount >= 1000 then + Charge := 0 + else + Charge := 49; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeCalculateShippingCharge(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.good.al b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.good.al new file mode 100644 index 0000000..6b47535 --- /dev/null +++ b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.good.al @@ -0,0 +1,37 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50220 "Shipping Charge Good Sample" +{ + procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal + var + IsHandled: Boolean; + begin + // Give extensions a sanctioned seam to replace the calculation, then + // skip the default logic when a subscriber has handled it. + OnBeforeCalculateShippingCharge(OrderAmount, Charge, IsHandled); + if IsHandled then + exit(Charge); + + if OrderAmount >= 1000 then + Charge := 0 + else + Charge := 49; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeCalculateShippingCharge(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean) + begin + end; +} + +codeunit 50221 "Shipping Charge Sub Good Sample" +{ + // A partner replaces the flat rate with a contract-specific rule. + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Shipping Charge Good Sample", 'OnBeforeCalculateShippingCharge', '', false, false)] + local procedure ApplyContractRate(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean) + begin + if IsHandled then + exit; + Charge := OrderAmount * 0.02; + IsHandled := true; + end; +} diff --git a/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md new file mode 100644 index 0000000..d273589 --- /dev/null +++ b/microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [ishandled, overridable, onbefore, integration-event, extensibility, event-override, subscriber-hook] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Use the IsHandled pattern to make base behaviour overridable + +## Description + +AL has no method overriding, so a `procedure` that runs its body unconditionally cannot be replaced by an extension without editing base code. The established Business Central seam for substituting default behaviour is the `IsHandled` pattern: the routine raises an `OnBefore…` integration event carrying a `var IsHandled: Boolean`, then exits early when a subscriber has set it. This hands a partner a sanctioned hook to replace the logic instead of overwriting the routine. LLMs trained on languages with inheritance emit routines whose logic always runs and expose no `OnBefore`/`IsHandled` seam, so the behaviour silently cannot be overridden. + +## Best Practice + +Raise `OnBeforeX(…, IsHandled)` as the first step of the routine and guard with `if IsHandled then exit;` before any default logic runs. Declare the publisher `[IntegrationEvent(false, false)] local procedure OnBeforeX(…; var IsHandled: Boolean)` with an empty body, and keep `IsHandled` a `var` parameter so a subscriber can write to it. A subscriber that replaces the behaviour does its work and sets `IsHandled := true`; one that only augments leaves it untouched and guards with `if IsHandled then exit;` itself. Reserve the override hook for cases where a partner genuinely needs to replace logic — when the goal is only to react, a positive `OnAfter` event is the better seam. + +See sample: `use-ishandled-to-make-base-behaviour-overridable.good.al`. + +## Anti Pattern + +Two shapes. First, a routine whose default logic always runs because there is no `OnBefore…`/`IsHandled` hook at all — extensions cannot change it without overwriting base code. Second, a routine that raises `OnBeforeX(IsHandled)` but omits the `if IsHandled then exit;` guard, so the default logic still executes after a subscriber set `IsHandled := true`, duplicating work and side effects. Detection: an `OnBefore` publisher with a `var IsHandled: Boolean` parameter whose caller never tests `IsHandled`, or a public routine doing non-trivial work with no overridable seam. + +See sample: `use-ishandled-to-make-base-behaviour-overridable.bad.al`. diff --git a/microsoft/skills/review/al-code-review.md b/microsoft/skills/review/al-code-review.md index 80bd19c..838a4c6 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, error handling). +description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI, error handling, events). inputs: [pr-diff, file-path] outputs: [findings-report] bc-version: [all] @@ -18,6 +18,7 @@ sub-skills: - 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 --- # AL code review @@ -39,6 +40,7 @@ The sub-skills invoked by this skill are those listed in frontmatter `sub-skills - `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` 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-events-review.md b/microsoft/skills/review/al-events-review.md new file mode 100644 index 0000000..606090a --- /dev/null +++ b/microsoft/skills/review/al-events-review.md @@ -0,0 +1,136 @@ +--- +kind: action-skill +id: al-events-review +version: 1 +title: AL events review +description: Reviews AL source changes against events-and-subscribers guidance from BCQuality. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL events review + +Reviews AL source changes against the `events` 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 `events` 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/events/**`. + +## 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 publish events or host event subscribers, posting/release/validation routines that should expose extension points, and test codeunits that bind subscribers. +- The changed procedures and triggers, weighted toward event publisher methods, methods carrying the `[EventSubscriber(...)]` attribute, routines that raise `OnBefore`/`OnAfter` events, and any procedure that calls `BindSubscription`/`UnbindSubscription`. +- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`). + +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 events knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable events 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 events 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 events 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 events and subscribers; 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: empty out a non-empty `[IntegrationEvent]` publisher body; add the missing `if IsHandled then exit;` guard after an `OnBefore` raise; add a matching `UnbindSubscription` for a leaked `BindSubscription`; set `EventSubscriberInstance = Manual;` on a codeunit that must be scoped). 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 events 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-events-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/events/publish-thin-onbefore-onafter-integration-events.md", + "severity": "major", + "message": "Business logic is placed inside an [IntegrationEvent] publisher body, so the event mutates state on every raise instead of being a thin hook. Move the logic into the calling routine and leave the publisher body empty.", + "location": { + "file": "src/Sales/ReservationMgt.Codeunit.al", + "line": 64, + "range": { "start-line": 61, "end-line": 67 } + }, + "references": [ + { "path": "microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md" } + ], + "confidence": "high" + }, + { + "id": "microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md", + "severity": "minor", + "message": "An OnBefore event is raised with a var IsHandled parameter, but the routine never guards with 'if IsHandled then exit;', so the default logic still runs after a subscriber handled the call.", + "location": { + "file": "src/Sales/ReservationMgt.Codeunit.al", + "line": 41 + }, + "references": [ + { "path": "microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md" } + ], + "confidence": "high" + } + ], + "suppressed": [] +} +``` + +The empty-corpus case — BCQuality's state until events knowledge files land — produces: + +```json +{ + "skill": { "id": "al-events-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": [] +} +```