Address review guidance feedback

Preserve independent event seams, cover loop-carried handled state, strengthen checkpoint and UI-handler fixtures, and align DeleteAll fallback guidance.

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

Copilot-Session: 10646e50-2d8b-4cca-b02b-dfa78629e6a1

microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al

@@ -17,6 +17,20 @@ codeunit 50241 "IsHandled Init Bad Sample"
             DiscountPct += 2;
     end;
 
+    procedure ApplyLineDiscounts(var SalesLine: Record "Sales Line")
+    var
+        LineIsHandled: Boolean;
+    begin
+        if SalesLine.FindSet() then
+            repeat
+                // Bug: the local initializes only once. A subscriber that handles
+                // one line leaves true for every later iteration.
+                OnBeforeApplyLineDiscount(SalesLine, LineIsHandled);
+                if not LineIsHandled then
+                    SalesLine.Validate("Line Discount %", 5);
+            until SalesLine.Next() = 0;
+    end;
+
     [IntegrationEvent(false, false)]
     local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean)
     begin
@@ -26,4 +40,9 @@ codeunit 50241 "IsHandled Init Bad Sample"
     local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean)
     begin
     end;
+
+    [IntegrationEvent(false, false)]
+    local procedure OnBeforeApplyLineDiscount(var SalesLine: Record "Sales Line"; var IsHandled: Boolean)
+    begin
+    end;
 }

microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al

@@ -4,19 +4,18 @@ codeunit 50240 "IsHandled Init Good Sample"
     procedure ApplyDiscounts(var SalesHeader: Record "Sales Header")
     var
         DiscountPct: Decimal;
-        IsHandled: Boolean;
+        HeaderIsHandled: Boolean;
+        PaymentIsHandled: Boolean;
     begin
-        // A freshly declared local Boolean is false.
-        OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled);
-        if IsHandled then
-            exit;
-        DiscountPct := 5;
+        // Each fresh local is false and belongs to one non-looping raise.
+        OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, HeaderIsHandled);
+        if not HeaderIsHandled then
+            DiscountPct := 5;
 
-        // Reaching this point proves that IsHandled is still false.
-        OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled);
-        if IsHandled then
-            exit;
-        DiscountPct += 2;
+        // Handling the header event does not suppress this independent seam.
+        OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, PaymentIsHandled);
+        if not PaymentIsHandled then
+            DiscountPct += 2;
     end;
 
     [IntegrationEvent(false, false)]

microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md

@@ -11,16 +11,16 @@ application-area: [all]
 
 ## Description
 
-A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event is already deterministic. The same is true when control flow proves the variable is `false`; for example, reaching a second raise after `if IsHandled then exit;` proves that the first raise did not leave it `true`.
+A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event exactly once per procedure invocation is already deterministic. Initialization does not repeat for each loop iteration: a local declared outside a loop can carry `true` from one iteration to the next even when the source contains only one textual event raise. Outside a loop, reaching a later raise after `if IsHandled then exit;` also proves the value is `false`, provided that early exit is semantically correct and does not skip required downstream events.
 
 ## Best Practice
 
-Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, the value comes from an input parameter, field, or global, or earlier code seeds it. A reset on a guaranteed-false fresh local can be retained for readability, but its absence is not a correctness finding.
+Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, a raise is re-entered by a loop, the value comes from an input parameter, field, or global, or earlier code seeds it. Prefer separate fresh locals when independent event seams need independent handled state. A reset on a guaranteed-false fresh local used by one non-looping raise, or before a later raise reached only after a semantically valid `if IsHandled then exit;`, can be retained for readability, but its absence is not a correctness finding.
 
 See sample: `initialize-ishandled-to-false-before-publishing.good.al`.
 
 ## Anti Pattern
 
-Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise or another source, so the new publisher call starts with stale state. Do not match a single raise using a fresh local Boolean, or a later raise reached only after `if IsHandled then exit;`.
+Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise, an earlier loop iteration, or another source, so the publisher call starts with stale state. Do not match a single non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false.
 
 See sample: `initialize-ishandled-to-false-before-publishing.bad.al`.

microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al

@@ -3,12 +3,24 @@ codeunit 50129 "Perf Sample CommitInLoop Bad"
     procedure NormalizeCustomerNames()
     var
         Customer: Record Customer;
+        LastCustomerNo: Code[20];
+        ProcessedCount: Integer;
     begin
+        Customer.SetFilter("No.", '>%1', LastCustomerNo);
         if Customer.FindSet(true) then
             repeat
                 Customer.Name := UpperCase(Customer.Name);
                 Customer.Modify();
-                Commit();
+
+                // LastCustomerNo exists only in memory, so a retry cannot exclude
+                // work that was already committed.
+                LastCustomerNo := Customer."No.";
+                ProcessedCount += 1;
+
+                // This still opened a FindSet over the complete remaining tail;
+                // periodic commits do not turn retrieval into bounded TOP X.
+                if ProcessedCount mod 500 = 0 then
+                    Commit();
             until Customer.Next() = 0;
     end;
 }

microsoft/knowledge/performance/avoid-commit-inside-loops.good.al

@@ -19,7 +19,11 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
         NormalizeState: Record "Perf Normalize State";
         LastCustomerNo: Code[20];
     begin
-        NormalizeState.Get('CUSTOMER');
+        if not NormalizeState.Get('CUSTOMER') then begin
+            NormalizeState.Init();
+            NormalizeState.Code := 'CUSTOMER';
+            NormalizeState.Insert();
+        end;
         LastCustomerNo := NormalizeState."Last Customer No.";
 
         while NormalizeNextChunk(LastCustomerNo) do begin

microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md

@@ -1,7 +1,7 @@
 ---
 bc-version: [all]
 domain: performance
-keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass]
+keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass, security-filtering, media, companion-fields]
 technologies: [al]
 countries: [w1]
 application-area: [all]
@@ -13,16 +13,16 @@ application-area: [all]
 
 ## Description
 
-`DeleteAll(false)` is eligible for a set-based SQL delete with the record variable's filters applied. It is not guaranteed to stay one statement. The base table `OnDelete` trigger is skipped, but table-extension `OnBeforeDelete` and `OnAfterDelete` triggers still run. Extension event subscribers, global delete triggers, and media fields can also require row processing. `DeleteAll(true)` runs the base table `OnDelete` trigger as well and has no performance advantage over `Delete(true)` in a loop.
+`DeleteAll(false)` is eligible for a set-based SQL delete with the record variable's filters applied, but it is not guaranteed to stay one statement. Microsoft documents that `DeleteAll` [reverts to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. Setting `RunTrigger` to false skips the base table `OnDelete` trigger, but [table-extension `OnBeforeDelete` and `OnAfterDelete` triggers still run](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-deleteall-method#remarks).
 
 ## Best Practice
 
-Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and installed extensions, subscribers, global triggers, and media fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately.
+Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and that trigger code, related subscribers, security filtering, media fields, and companion fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately.
 
 See sample: `use-deleteall-for-filtered-bulk-deletion.good.al`.
 
 ## Anti Pattern
 
-Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking table extensions and subscribers.
+Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic or fallback condition. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking the documented fallback conditions.
 
 See sample: `use-deleteall-for-filtered-bulk-deletion.bad.al`.

microsoft/knowledge/testing/ui-handlers-in-tests.bad.al

@@ -4,11 +4,11 @@ codeunit 50401 "Test UI Handler Proof Bad"
 
     [Test]
     [HandlerFunctions('CustomerCardHandler')]
-    procedure CustomerCardActionSucceeds()
+    procedure PreSetBooleanDoesNotProveCustomerCardResult()
     var
         Customer: Record Customer;
     begin
-        Customer.Get('10000');
+        LibrarySales.CreateCustomer(Customer);
         ActionSucceeded := true;
 
         Page.RunModal(Page::"Customer Card", Customer);
@@ -17,12 +17,42 @@ codeunit 50401 "Test UI Handler Proof Bad"
         Assert.IsTrue(ActionSucceeded, 'The customer card action failed.');
     end;
 
+    [Test]
+    [HandlerFunctions('CustomerCardHandler')]
+    procedure MissingMessageHandlerFailsAtRuntime()
+    var
+        Customer: Record Customer;
+    begin
+        LibrarySales.CreateCustomer(Customer);
+
+        Page.RunModal(Page::"Customer Card", Customer);
+        Message('Customer card closed.');
+    end;
+
+    [Test]
+    [HandlerFunctions('CustomerCardHandler,UnusedConfirmHandler')]
+    procedure UnreachedListedHandlerFailsAtRuntime()
+    var
+        Customer: Record Customer;
+    begin
+        LibrarySales.CreateCustomer(Customer);
+
+        Page.RunModal(Page::"Customer Card", Customer);
+    end;
+
     [ModalPageHandler]
     procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card")
     begin
     end;
 
+    [ConfirmHandler]
+    procedure UnusedConfirmHandler(Question: Text[1024]; var Reply: Boolean)
+    begin
+        Reply := true;
+    end;
+
     var
         Assert: Codeunit "Library Assert";
+        LibrarySales: Codeunit "Library - Sales";
         ActionSucceeded: Boolean;
 }

microsoft/knowledge/testing/ui-handlers-in-tests.good.al

@@ -8,7 +8,7 @@ codeunit 50400 "Test UI Handler Capture Good"
     var
         Customer: Record Customer;
     begin
-        Customer.Get('10000');
+        LibrarySales.CreateCustomer(Customer);
         CapturedCustomerNo := '';
 
         Page.RunModal(Page::"Customer Card", Customer);
@@ -24,5 +24,6 @@ codeunit 50400 "Test UI Handler Capture Good"
 
     var
         Assert: Codeunit "Library Assert";
+        LibrarySales: Codeunit "Library - Sales";
         CapturedCustomerNo: Code[20];
 }

microsoft/skills/review/al-events-review.md

@@ -51,7 +51,7 @@ When the post-conflict worklist is empty because no applicable events knowledge
 
 The following targeted checks map diff signals to specific `events` articles. Treat each as a candidate-selection cue: when the signal appears in the changed code, add the named article to the worklist and evaluate it in Action.
 
-- An `IsHandled` value that can carry over as `true` (reused after an earlier raise, input/global/field, or otherwise seeded) is passed to another publisher without a reset — `initialize-ishandled-to-false-before-publishing`. Do not match a single raise using a fresh local Boolean, or a later raise reached only after `if IsHandled then exit;`.
+- An `IsHandled` value that can carry over as `true` (reused after an earlier raise, re-entered on a later loop iteration, input/global/field, or otherwise seeded) is passed to a publisher without a reset — `initialize-ishandled-to-false-before-publishing`. Do not match one non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false.
 - `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event later, so the after-event is skipped whenever the call is handled — `preserve-onafter-execution-when-ishandled-skips-the-body`.
 - Any parameter added to a public Business/Integration event procedure, regardless of position; do not flag additions or reordering on `local`/`internal` publishers merely because a new parameter was not appended — `add-new-event-parameters-at-the-end`.
 - A shipped Business/Integration event renamed or removed, or an existing parameter renamed, removed, retyped, or changed to/from `var`, based on the mistaken assumption that `local` or `internal` prevents dependent subscription; parameter order alone is not a subscriber-contract violation — `treat-local-and-internal-events-as-subscriber-contracts`.

microsoft/skills/review/al-testing-review.md

@@ -74,7 +74,7 @@ Set `confidence` to:
 
 After evaluating each worklist entry, also consider whether the diff exhibits a testing 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 testing 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 AL testing; 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: add the matching `ExpectedError` assertion after `asserterror`; add or remove a handler name in `HandlerFunctions`; add `LibraryVariableStorage.Clear` or `AssertEmpty`; or replace hand-rolled fixture creation with an evident library call). 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`.
+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: add the matching `ExpectedError` assertion after `asserterror`; add or remove a handler name in `HandlerFunctions`; add `LibraryVariableStorage.Clear` or `AssertEmpty` when queue/LVS intentionally verifies interaction order, count, text, replies, or a scripted sequence; or replace hand-rolled fixture creation with an evident library call). 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.