Promote knowledge for Microsoft review skills

Move canonical knowledge for Microsoft-owned review domains into the Microsoft layer and document the skill/knowledge co-location policy.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2a6ea875-d38e-4f30-aadb-0d606f9be231

microsoft/knowledge/events/declare-event-publishers-local-or-internal.bad.al

@@ -0,0 +1,43 @@
+// Demonstration only. Shows the wrong pattern: the publisher carries no access modifier, so it is
+// public - which never was what lets extensions subscribe.
+
+codeunit 50100 "Loyalty Points Mgt Bad"
+{
+    procedure AwardPoints(CustomerNo: Code[20]; SalesAmount: Decimal)
+    var
+        Points: Decimal;
+        IsHandled: Boolean;
+    begin
+        Points := SalesAmount / 10;
+
+        IsHandled := false;
+        OnBeforeAwardPoints(CustomerNo, Points, IsHandled);
+        if IsHandled then
+            exit;
+
+        // ... insert the loyalty entry ...
+    end;
+
+    // BAD: no access modifier, so this publisher is public. Public access does not enable
+    // subscription - it enables raising. Narrowing it to internal after release breaks callers,
+    // so the widening cannot be walked back cheaply.
+    [IntegrationEvent(false, false)]
+    procedure OnBeforeAwardPoints(CustomerNo: Code[20]; var Points: Decimal; var IsHandled: Boolean)
+    begin
+    end;
+}
+
+codeunit 50101 "Loyalty Points Caller Bad"
+{
+    procedure FirePublisherDirectly(CustomerNo: Code[20])
+    var
+        LoyaltyPointsMgt: Codeunit "Loyalty Points Mgt Bad";
+        Points: Decimal;
+        IsHandled: Boolean;
+    begin
+        // Compiles only because the publisher is public. Every subscriber runs although no points
+        // were ever awarded, on a Points value nobody computed, and the IsHandled answer the
+        // subscribers write is read by no one.
+        LoyaltyPointsMgt.OnBeforeAwardPoints(CustomerNo, Points, IsHandled);
+    end;
+}

microsoft/knowledge/events/declare-event-publishers-local-or-internal.good.al

@@ -0,0 +1,59 @@
+// Demonstration only. Shows the correct pattern: a public facade codeunit whose event publishers
+// are internal, so only the implementation codeunit decides when they fire.
+
+codeunit 50100 "Loyalty Points Mgt Good"
+{
+    procedure AwardPoints(CustomerNo: Code[20]; SalesAmount: Decimal)
+    var
+        LoyaltyPointsImpl: Codeunit "Loyalty Points Impl Good";
+    begin
+        LoyaltyPointsImpl.AwardPoints(CustomerNo, SalesAmount);
+    end;
+
+    // internal, not public: the implementation codeunit raises this and nobody else. Subscribers
+    // bind through Codeunit::"Loyalty Points Mgt Good", which is public by default - that object
+    // access is all a subscriber in another extension needs.
+    [IntegrationEvent(false, false)]
+    internal procedure OnBeforeAwardPoints(CustomerNo: Code[20]; var Points: Decimal; var IsHandled: Boolean)
+    begin
+    end;
+
+    [IntegrationEvent(false, false)]
+    internal procedure OnAfterAwardPoints(CustomerNo: Code[20]; Points: Decimal)
+    begin
+    end;
+}
+
+codeunit 50101 "Loyalty Points Impl Good"
+{
+    Access = Internal;
+
+    procedure AwardPoints(CustomerNo: Code[20]; SalesAmount: Decimal)
+    var
+        LoyaltyPointsMgt: Codeunit "Loyalty Points Mgt Good";
+        Points: Decimal;
+        IsHandled: Boolean;
+    begin
+        Points := SalesAmount / 10;
+
+        IsHandled := false;
+        LoyaltyPointsMgt.OnBeforeAwardPoints(CustomerNo, Points, IsHandled);
+        if IsHandled then
+            exit;
+
+        // ... insert the loyalty entry ...
+
+        LoyaltyPointsMgt.OnAfterAwardPoints(CustomerNo, Points);
+    end;
+}
+
+codeunit 50102 "Loyalty Points Sub Good"
+{
+    // The shape a subscriber in a dependent extension takes: it names the public object, and is
+    // indifferent to the publisher being internal.
+    [EventSubscriber(ObjectType::Codeunit, Codeunit::"Loyalty Points Mgt Good", 'OnAfterAwardPoints', '', false, false)]
+    local procedure LogAwardedPointsOnAfterAwardPoints(CustomerNo: Code[20]; Points: Decimal)
+    begin
+        // ... write telemetry ...
+    end;
+}

microsoft/knowledge/events/declare-event-publishers-local-or-internal.md

@@ -0,0 +1,42 @@
+---
+bc-version: [all]
+domain: events
+keywords: [event-publisher, access-modifier, local, internal, integration-event, business-event, subscriber, breaking-change]
+technologies: [al]
+countries: [w1]
+application-area: [all]
+---
+
+# Declare event publishers local or internal
+
+> Contributions welcome — open a PR to refine or extend this article.
+
+## Description
+
+The access modifier on an `[IntegrationEvent]` or `[BusinessEvent]` publisher controls who may *raise* the procedure, not who may *subscribe* to it. A subscriber in a dependent extension binds through the object named in its `[EventSubscriber(...)]` attribute, so the only accessibility a foreign subscriber needs is on the *object* — a codeunit left at its default public access. The publisher procedure itself can and should stay `local` or `internal`. Publishing an event is an invitation to subscribe, not an invitation to call: an omitted access modifier makes the publisher public, which hands every dependent extension the ability to fire the event on its own. The signature-compatibility consequences of a shipped publisher are covered separately by `treat-local-and-internal-events-as-subscriber-contracts`.
+
+## Applies to
+
+Ordinary `[IntegrationEvent]` and `[BusinessEvent]` publishers. `[InternalEvent]` has its own module-only visibility semantics, and external business events are out of scope.
+
+## Best Practice
+
+Give an event publisher the narrowest access modifier that still lets the code owning the operation raise it:
+
+- `local` when only the declaring object raises the event. This is the common case and the default choice.
+- `internal` when another object in the same app raises it — typically an internal implementation codeunit raising an event declared on a public facade codeunit. The facade object stays public so dependent extensions can name it in `[EventSubscriber(...)]`; the publisher stays `internal` so only the implementation decides when the event fires.
+- `public` only when a *different app* must raise the event — a hub or event-bus codeunit in a foundation app that sibling apps signal through, where `internal` cannot reach across the app boundary. This is a deliberate caller contract, not a concession to subscribers, and it is maintained like any other public API.
+
+Subscribers are unaffected by any of these choices. A non-public publisher also keeps the freedom to add a parameter later, which a public publisher gives up — see `add-new-event-parameters-at-the-end`.
+
+See sample: `declare-event-publishers-local-or-internal.good.al`.
+
+## Anti Pattern
+
+An event publisher declared with no access modifier — or widened to public — in the belief that dependent extensions need that to subscribe. They do not. Two consequences follow. Any dependent extension can now call the publisher directly, firing every subscriber outside the owning routine's control flow, on state the publisher never prepared and with an `IsHandled` answer nobody reads. And because the publisher is a public procedure, it is a caller contract: narrowing it back to `local` or `internal` after release is itself a breaking change, so the mistake is not cheaply reversible.
+
+Detection: an `[IntegrationEvent]` or `[BusinessEvent]` publisher that is public although every raiser is in its own app — typically raised only from its declaring object. A publisher deliberately made public so another app can raise it is not this anti-pattern; do not report it. When the surrounding repository or API context does not reveal whether an external raiser is intended, treat the public modifier as intentional rather than reporting it.
+
+The mirror-image anti-pattern belongs to the reviewer, human or agent: recommending that a publisher be made public so extensions can subscribe, or reporting a `local`/`internal` publisher as unreachable dead code. Both readings mistake raising for subscribing. Neither should be raised as a finding.
+
+See sample: `declare-event-publishers-local-or-internal.bad.al`.

microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.bad.al

@@ -0,0 +1,72 @@
+// Demonstration-only AL. Not compiled by CI; illustrates the article.
+
+// Anti-pattern 1: the context is kept in single-instance state.
+codeunit 50545 "Process State Bad Sample"
+{
+    SingleInstance = true;
+
+    var
+        ProcessRunning: Boolean;
+
+    procedure SetProcessRunning(NewProcessRunning: Boolean)
+    begin
+        ProcessRunning := NewProcessRunning;
+    end;
+
+    procedure IsProcessRunning(): Boolean
+    begin
+        exit(ProcessRunning);
+    end;
+}
+
+codeunit 50546 "Process Driver Bad Sample"
+{
+    procedure Run(DocumentNo: Code[20])
+    var
+        ProcessState: Codeunit "Process State Bad Sample";
+    begin
+        ProcessState.SetProcessRunning(true);
+        RunSharedCode(DocumentNo);
+        // An error above never reaches this line. The database writes roll
+        // back, the single-instance variable does not: ProcessRunning stays
+        // true until the company is closed, so every later run in this session
+        // is treated as part of the process.
+        ProcessState.SetProcessRunning(false);
+    end;
+
+    local procedure RunSharedCode(DocumentNo: Code[20])
+    begin
+    end;
+}
+
+// Anti-pattern 2: the context stays private. Flag and driver look like the
+// good sample, but the query is internal, so only the owning app can ever ask.
+codeunit 50547 "Process Ctx Bad Sample"
+{
+    internal procedure IsProcessRunning(): Boolean
+    var
+        IsRunning: Boolean;
+    begin
+        OnCheckProcessRunning(IsRunning);
+        exit(IsRunning);
+    end;
+
+    [InternalEvent(false)]
+    local procedure OnCheckProcessRunning(var IsRunning: Boolean)
+    begin
+    end;
+}
+
+reportextension 50548 "Shared Report Ext Bad Sample" extends "Standard Sales - Invoice"
+{
+    trigger OnPreReport()
+    begin
+        // No callable query exists, so the extension infers the context from
+        // something it hopes only that process does - here, running without a
+        // UI. The guess is wrong for every other background run, and breaks
+        // silently the first time the owning app changes how it works.
+        if GuiAllowed() then
+            exit;
+        // ... behaviour that was meant to apply only inside that process ...
+    end;
+}

microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.good.al

@@ -0,0 +1,70 @@
+// Demonstration-only AL. Not compiled by CI; illustrates the article.
+
+// The published context API - the entire public surface of the pattern.
+// Any dependent extension may call IsProcessRunning; nothing else is exposed.
+codeunit 50540 "Process Context Good Sample"
+{
+    procedure IsProcessRunning(): Boolean
+    var
+        IsRunning: Boolean;
+    begin
+        OnCheckProcessRunning(IsRunning);
+        exit(IsRunning);
+    end;
+
+    // InternalEvent: only this app can subscribe, which is all the pattern
+    // needs. local: only this codeunit can raise it.
+    [InternalEvent(false)]
+    local procedure OnCheckProcessRunning(var IsRunning: Boolean)
+    begin
+    end;
+}
+
+// The flag - implementation, not API, hence Access = Internal. It stores
+// nothing between runs: being bound is the state.
+codeunit 50541 "Process Flag Good Sample"
+{
+    Access = Internal;
+    EventSubscriberInstance = Manual;
+
+    [EventSubscriber(ObjectType::Codeunit, Codeunit::"Process Context Good Sample", 'OnCheckProcessRunning', '', false, false)]
+    local procedure SetProcessRunning(var IsRunning: Boolean)
+    begin
+        IsRunning := true;
+    end;
+}
+
+// The app that drives the process claims the context for exactly its own run.
+codeunit 50542 "Process Driver Good Sample"
+{
+    procedure Run(DocumentNo: Code[20])
+    var
+        ProcessFlag: Codeunit "Process Flag Good Sample";
+    begin
+        // A fresh instance, bound for exactly this call. If the shared code
+        // errors, the stack unwinds and takes the binding with it - nothing to reset.
+        BindSubscription(ProcessFlag);
+        RunSharedCode(DocumentNo);
+    end;
+
+    local procedure RunSharedCode(DocumentNo: Code[20])
+    begin
+        // A base application report, a posting routine, or any other object
+        // that extensions hook into - including a customer's own replacement.
+    end;
+}
+
+// An extension hooked into that shared code can now ask the question directly
+// instead of guessing which process is driving the run. The hook happens to be
+// a report extension here; a subscriber on any other shared object is the same.
+reportextension 50543 "Shared Report Ext Good Sample" extends "Standard Sales - Invoice"
+{
+    trigger OnPreReport()
+    var
+        ProcessContext: Codeunit "Process Context Good Sample";
+    begin
+        if not ProcessContext.IsProcessRunning() then
+            exit;
+        // ... behaviour that applies only inside that process ...
+    end;
+}

microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.md

@@ -0,0 +1,40 @@
+---
+bc-version: [all]
+domain: events
+keywords: [bindsubscription, manual-binding, eventsubscriberinstance, internalevent, singleinstance, process-context, running-flag, scoped-state, rollback]
+technologies: [al]
+countries: [w1]
+application-area: [all]
+---
+
+# Expose process context through a manually bound flag, not a single-instance boolean
+
+> Contributions welcome — open a PR to refine or extend this article.
+
+## Description
+
+When an extension drives a process over shared code — a base application report, a posting routine — other extensions hooked into that code cannot tell whether a run belongs to that process: AL keeps no ambient "current process", so the driving app has to publish the context itself. The reflex answer, a `SingleInstance` codeunit holding a boolean set at the start of the run and cleared at the end, is unsafe: single-instance variables are not part of the database transaction, so a failed run rolls back the writes but not the flag, which stays `true` until the company is closed and marks every later run in the session as part of the process. A manual event binding carries the same signal safely, because the platform ties its lifetime to a variable's scope instead of to cleanup code that has to run.
+
+## Best Practice
+
+Publish the context as a query and let the binding itself be the state. One procedure is public; everything behind it is internal:
+
+- A public context codeunit exposes `IsProcessRunning(): Boolean`, which raises an `[InternalEvent]` publisher taking a `var Boolean` and returns what comes back — the entire public surface. The publisher is internal because only the owning app subscribes, `local` because only this codeunit raises it.
+- A second codeunit, `Access = Internal` with `EventSubscriberInstance = Manual`, subscribes to that event and sets the boolean to `true`. Internal keeps it out of the API and stops other apps binding it to forge the context; it stores nothing between runs — being bound *is* the state.
+- The driving process calls `BindSubscription` on a variable whose scope is exactly the span it wants to claim: a local in the procedure that drives the run, or a global on an object that lives exactly as long as the run. While that variable is alive the query answers `true`; when it leaves scope — normally, or because an error unwound the call stack — the platform removes the binding and the query answers `false` again.
+
+Bind a fresh instance per run rather than reusing one: the platform refuses to bind the same instance twice but accepts several instances of the same codeunit, so nesting and re-entrancy need no counter. The binding is session-scoped, so work the process starts in another session — a background session, a page background task, a job queue entry — cannot see it; pass the context explicitly there.
+
+See sample: `expose-process-context-via-manually-bound-flag.good.al`.
+
+## Anti Pattern
+
+Two shapes.
+
+First, the single-instance boolean — the failure described above. Detection: a `SingleInstance = true` codeunit with a boolean set before a process and cleared after it, read by other code to decide whether that process is running.
+
+Second, the context kept private: the driving app arranges its own marker — typically a manually bound subscriber on an event added for its benefit alone — and offers no query, or only an `internal` one. Other extensions are left inferring the context from side effects, request-page values, or record state, which breaks silently the first time the process changes. Detection: a manual binding used purely as an internal run marker, with no public query procedure over it.
+
+The mirror-image anti-pattern belongs to the reviewer: flagging the `BindSubscription` here as a leaked binding because no `UnbindSubscription` follows it. Scope release is the mechanism, not an omission — see `microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md`, whose leak case is an instance parked on a `SingleInstance` global that never leaves scope.
+
+See sample: `expose-process-context-via-manually-bound-flag.bad.al`.