From 5f1cff2fb6427c14cf9b2ef1f790ee404a144b9e Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Tue, 18 Aug 2026 11:17:14 +0200 Subject: [PATCH 1/2] Refine self-improvement review guidance Narrow IsHandled, label-scope, UI-handler, checkpoint, and bulk-operation guidance to evidence-backed false-positive boundaries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...shandled-to-false-before-publishing.bad.al | 6 +- ...handled-to-false-before-publishing.good.al | 16 +++--- ...ze-ishandled-to-false-before-publishing.md | 8 +-- .../avoid-commit-inside-loops.good.al | 25 ++++++++- .../performance/avoid-commit-inside-loops.md | 8 ++- .../prefer-modifyall-over-per-row-modify.md | 4 +- ...ggers-and-media-field-regress-modifyall.md | 8 +-- .../labels-declared-at-object-scope.bad.al | 11 ---- .../labels-declared-at-object-scope.good.al | 13 ----- .../style/labels-declared-at-object-scope.md | 20 ++----- .../testing/ui-handlers-in-tests.bad.al | 43 +++++---------- .../testing/ui-handlers-in-tests.good.al | 55 +++++-------------- .../knowledge/testing/ui-handlers-in-tests.md | 12 ++-- microsoft/skills/review/al-events-review.md | 2 +- .../skills/review/al-performance-review.md | 5 +- microsoft/skills/review/al-privacy-review.md | 2 +- microsoft/skills/review/al-security-review.md | 2 +- microsoft/skills/review/al-style-review.md | 4 +- microsoft/skills/review/al-testing-review.md | 4 +- microsoft/skills/review/al-ui-review.md | 2 +- microsoft/skills/review/al-upgrade-review.md | 2 +- 21 files changed, 98 insertions(+), 154 deletions(-) delete mode 100644 microsoft/knowledge/style/labels-declared-at-object-scope.bad.al delete mode 100644 microsoft/knowledge/style/labels-declared-at-object-scope.good.al diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al index 94b50f39..2cb465d0 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al @@ -6,14 +6,12 @@ codeunit 50241 "IsHandled Init Bad Sample" DiscountPct: Decimal; IsHandled: Boolean; begin - // IsHandled is never initialized before the first raise, so flow depends - // on the variable's default rather than an explicit, documented intent. OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); if not IsHandled then DiscountPct := 5; - // Bug: IsHandled is not reset. If the first subscriber set it true, the - // payment-discount default below is silently skipped too. + // Bug: execution continues when the first event set IsHandled to true, + // and that stale value is passed to a different publisher. OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); if not IsHandled then DiscountPct += 2; diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al index 190e3218..a595e8ad 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al @@ -6,17 +6,17 @@ codeunit 50240 "IsHandled Init Good Sample" DiscountPct: Decimal; IsHandled: Boolean; begin - IsHandled := false; + // A freshly declared local Boolean is false. OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); - if not IsHandled then - DiscountPct := 5; + if IsHandled then + exit; + DiscountPct := 5; - // Reset before reusing the same variable for the next event so a - // subscriber that handled the first raise can't suppress this one. - IsHandled := false; + // Reaching this point proves that IsHandled is still false. OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); - if not IsHandled then - DiscountPct += 2; + if IsHandled then + exit; + DiscountPct += 2; end; [IntegrationEvent(false, false)] diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md index acfb54a2..9a2aef20 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md @@ -7,20 +7,20 @@ countries: [w1] application-area: [all] --- -# Initialize IsHandled to false before publishing +# Reset IsHandled before publishing only when its value can carry over ## Description -A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable in by reference, so its incoming value decides whether the default logic is skipped. A freshly declared Boolean starts as `false`, but the same variable is frequently reused to raise several events in one routine, and after the first raise it may already be `true`. Assigning `IsHandled := false;` on the line immediately before every raise makes the control flow deterministic and self-documenting, and prevents a stale `true` from silently suppressing logic the author never meant to make skippable. Generated code often reuses one `IsHandled` across several raises without resetting it. +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`. ## Best Practice -Set `IsHandled := false;` immediately before each `OnBeforeX(…, IsHandled)` raise, then guard the default logic with `if IsHandled then exit;` or `if not IsHandled then …`. Do this even when the variable was just declared: the explicit reset documents intent and stays correct if a second event raise is added to the routine later. This applies only to events that carry a `var IsHandled: Boolean`; an `OnBefore` event with no `IsHandled` parameter needs no reset. +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. See sample: `initialize-ishandled-to-false-before-publishing.good.al`. ## Anti Pattern -Raising `OnBeforeX(…, IsHandled)` with a variable whose value carries over from an earlier raise, so a subscriber that handled the first event unintentionally suppresses the second routine's default logic. Detection: an `IsHandled` variable passed to more than one event in a routine without an intervening `IsHandled := false;`, or any `OnBefore…` raise that passes an `IsHandled` variable without an intervening `IsHandled := false;`. +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;`. See sample: `initialize-ishandled-to-false-before-publishing.bad.al`. diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al index 2ffa386c..e82f98b2 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al @@ -16,11 +16,18 @@ codeunit 50128 "Perf Sample CommitInLoop Good" { procedure NormalizeCustomerNames() var + NormalizeState: Record "Perf Normalize State"; LastCustomerNo: Code[20]; begin - // The outer loop owns checkpoints; the per-row loop contains no Commit. - while NormalizeNextChunk(LastCustomerNo) do + NormalizeState.Get('CUSTOMER'); + LastCustomerNo := NormalizeState."Last Customer No."; + + while NormalizeNextChunk(LastCustomerNo) do begin + // Persist progress in the same transaction as the completed chunk. + NormalizeState."Last Customer No." := LastCustomerNo; + NormalizeState.Modify(); Commit(); + end; end; local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean @@ -58,3 +65,17 @@ codeunit 50128 "Perf Sample CommitInLoop Good" exit(true); end; } + +table 50128 "Perf Normalize State" +{ + fields + { + field(1; Code; Code[10]) { } + field(2; "Last Customer No."; Code[20]) { } + } + + keys + { + key(PK; Code) { Clustered = true; } + } +} diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.md b/microsoft/knowledge/performance/avoid-commit-inside-loops.md index 13f483a8..25ad9589 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.md +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.md @@ -13,16 +13,18 @@ application-area: [all] ## Description -Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that select an exact list of at most N keys and process only those rows. +Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). + +A durability checkpoint inside an outer batch loop can be valid only when the same transaction persists a progress marker or state that makes retries strictly exclude completed work, the checkpoint follows a complete business unit, and errors propagate instead of being swallowed. Restart safety and bounded retrieval are separate requirements: a persisted watermark can make retries safe, but an outer `FindSet` over the full remaining tail with periodic commits still retrieves the complete set because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next). ## Best Practice -If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. `FindSet` is optimized for reading the complete filtered set and isn't implemented as `TOP X`, so calling it over the remaining tail and breaking after N rows does not bound retrieval. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Commit after the bounded inner loop returns and persist its last selected key as the next watermark. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`. +If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Persist the last selected key in the same transaction as the completed chunk, then commit after the bounded helper returns. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. Let errors escape so failed work is not recorded as complete. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`. See sample: `avoid-commit-inside-loops.good.al`. ## Anti Pattern -Placing Commit inside `repeat ... until Next() = 0` is almost always a mistake: it is unusual for the correctness of the operation to depend on per-row commits, and the cost of starting a new transaction on every row dominates the work. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint. +Placing Commit inside `repeat ... until Next() = 0` without persisted progress is almost always a mistake: retries re-enter already committed work, while the cost of starting a transaction on every row dominates the operation. A progress variable held only in memory is not restart-safe. A full-tail `FindSet` with a commit every N rows is not bounded retrieval, even if a persisted watermark makes it restart-safe. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint. See sample: `avoid-commit-inside-loops.bad.al`. diff --git a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md index ae839686..024aae1f 100644 --- a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md +++ b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md @@ -15,12 +15,12 @@ application-area: [all] ## Best Practice -Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table-extension triggers, event subscribers, global triggers, or media fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). +Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). A visible loop for progress UX is acceptable only when evidence shows the equivalent bulk call already executes as individual operations and the loop preserves trigger and business semantics. See sample: `prefer-modifyall-over-per-row-modify.good.al`. ## Anti Pattern -A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior. +A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects or bulk fallback condition. A progress dialog alone does not exempt this loop. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior. See sample: `prefer-modifyall-over-per-row-modify.bad.al`. diff --git a/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md b/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md index c4890a0b..dabeb313 100644 --- a/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md +++ b/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: performance -keywords: [modifyall, deleteall, regression, triggers, media, getglobaltabletriggermask, subscriber] +keywords: [modifyall, deleteall, regression, triggers, media, security-filtering, companion-fields, subscriber, progress] technologies: [al] countries: [w1] application-area: [all] @@ -11,12 +11,12 @@ application-area: [all] ## Description -`ModifyAll` and `DeleteAll` usually execute as single SQL statements, but the platform falls back to a fetch-then-row-by-row loop under specific conditions. Per the upstream guidance, the regression is triggered by any of: global database triggers defined via `GetGlobalTableTriggerMask` or `GetDatabaseTableTriggerSetup` (so that `OnDatabaseDelete`/`OnGlobalDelete` must run); event subscribers on the table's `OnBeforeDelete`/`OnAfterDelete` (for `DeleteAll`) or `OnBeforeModify`/`OnAfterModify` (for `ModifyAll`); or "adding a Media or MediaSet table field to either the table or table extension." Each of these forces the platform to materialize each affected row in AL. +`ModifyAll` and `DeleteAll` can limit SQL calls, but Microsoft documents that they [revert 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 modify/delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. These conditions must be assessed from the target table and runtime context, not only from the visible bulk call. ## Best Practice -Before introducing any of the above on a table — a global trigger registration, a `Modify`/`Delete` subscriber, a media or media-set field — note every `ModifyAll`/`DeleteAll` that targets the table and assess whether the regression cost is acceptable. The upstream guidance is explicit: "There should be a very good reason for doing any of the above since they will significantly regress performance of `ModifyAll` and/or `DeleteAll`." Once a table has regressed, multiple `ModifyAll` calls each iterate the rows themselves, so consolidating to one explicit `FindSet`+`Modify` loop becomes faster than chaining several `ModifyAll` calls. +Before introducing a fallback condition, audit the `ModifyAll`/`DeleteAll` call sites that target the table and assess the regression cost. Once a bulk path already executes row by row, one explicit loop can be reasonable when it preserves the same trigger semantics and adds required per-row progress UX; consolidating several regressed bulk calls into one pass can also avoid repeated iteration. This is a narrow equivalence check, not a generic progress-dialog exemption: when no fallback condition applies, retain the bulk API. ## Anti Pattern -Adding a media field to a hot table — or subscribing to its modify/delete events from a generic logging codeunit — without auditing the bulk-write call sites. The schema change is mechanical; the performance change is invisible at the call site and only surfaces when a previously fast `ModifyAll` starts paying the per-row trigger cost in production. The mirror anti-pattern is chaining several `ModifyAll` calls on a table that has already regressed; each one re-iterates the same rows. +Adding a fallback condition to a hot table without auditing bulk-write call sites, or replacing a working bulk API with a per-row loop solely to show progress. The mirror anti-pattern is chaining several bulk calls on a table that already falls back, causing repeated row-by-row passes. diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al b/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al deleted file mode 100644 index 33c63fb3..00000000 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al +++ /dev/null @@ -1,11 +0,0 @@ -codeunit 50262 "Sample Label Scope Bad" -{ - procedure LookupCustomer(CustomerNo: Code[20]) - var - Customer: Record Customer; - GreetingMsg: Label 'Hello %1', Comment = '%1 = Customer Name'; - begin - if Customer.Get(CustomerNo) then - Message(GreetingMsg, Customer.Name); - end; -} diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.good.al b/microsoft/knowledge/style/labels-declared-at-object-scope.good.al deleted file mode 100644 index 415bda75..00000000 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.good.al +++ /dev/null @@ -1,13 +0,0 @@ -codeunit 50263 "Sample Label Scope Good" -{ - var - GreetingMsg: Label 'Hello %1', Comment = '%1 = Customer Name'; - - procedure LookupCustomer(CustomerNo: Code[20]) - var - Customer: Record Customer; - begin - if Customer.Get(CustomerNo) then - Message(GreetingMsg, Customer.Name); - end; -} diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.md b/microsoft/knowledge/style/labels-declared-at-object-scope.md index 24a868e6..91d75d7f 100644 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.md +++ b/microsoft/knowledge/style/labels-declared-at-object-scope.md @@ -1,30 +1,18 @@ --- bc-version: [all] domain: style -keywords: [label, scope, procedure, translation, localization, xliff] +keywords: [label, scope, procedure, translation, localization, xliff, false-positive] technologies: [al] countries: [w1] application-area: [all] --- -# Declare Labels at object scope, not inside procedure `var` blocks +# Procedure-local Labels are valid ## Description -`Label` is the AL declaration that participates in the translation pipeline: the build extracts every Label declared in an object into the `.xlf` file shipped to translators, and the runtime substitutes the localized value when the object is loaded. Translation tooling discovers Labels by walking the object's top-level declarations. - -Labels declared inside a procedure-local `var` block are still **compiled** as Label values, but their participation in localization is fragile: depending on the BC version, the build pipeline, and the translation toolchain in use, procedure-local Labels may be missed during XLIFF extraction, may be re-emitted with auto-generated keys that change between builds, or may not be addressable by reviewers triaging translations. The reliable, supported pattern is to declare every Label in the object's top-level `var` block. - -The same rule applies to all object types that own behavior: codeunits, pages, tables, reports, queries, and their extensions. For shared messages used by multiple objects, declare the Label in the most appropriate owning object and reference it — do not duplicate the literal across procedure-scoped declarations in several places. +The AL language supports `Label` variables at both object and procedure scope. Microsoft documents the [Label data type](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-using-labels#label-data-type) without imposing an object-scope requirement, and the translation pipeline generates an XLF file containing [all labels used by the extension](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-work-with-translation-files#generating-the-xliff-file). There is no documented correctness or localization defect caused solely by declaring a Label in a procedure-local `var` block. ## Best Practice -Move every `Label` to the object's top-level `var` block. Use the appropriate suffix (`Msg`, `Err`, `Qst`, `Lbl`, `Tok`, `Txt`) on the variable name so reviewers and the translation team can see at a glance what role the string plays. Pair non-translatable strings (URLs, JSON/XML fragments, integration tokens) with `Locked = true`, as covered by `label-locked-for-non-translatable.md`. - -See sample: `labels-declared-at-object-scope.good.al`. - -## Anti Pattern - -Declaring `Label` inside a procedure-local `var` block — `procedure Lookup() var GreetingMsg: Label 'Hello %1';` — couples the translatable string to one procedure, hides it from object-level review, and depends on a translation pipeline behavior that is not part of the AL language contract. - -See sample: `labels-declared-at-object-scope.bad.al`. +Choose object scope when a Label is reused or when an established repository convention prefers central declarations; choose procedure scope when the Label belongs to one procedure. Do not report a correctness or localization finding solely because a Label is local. An explicit object-scope convention is at most a low-severity maintainability preference. This guidance applies equally to production and test apps: test code still needs localization where its strings are user-facing or translator-facing. diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al index 1ecf47df..e1b8fc76 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al @@ -1,43 +1,28 @@ -codeunit 50401 "Test UI Handlers Bad" +codeunit 50401 "Test UI Handler Proof Bad" { Subtype = Test; - // Several wiring mistakes, each of which fails at runtime rather than as a - // clean assertion the reviewer can read: - // * A UI call with no listed handler -> "unhandled UI" abort (the Message - // below has no handler). - // * The mirror mistake, listing a handler the path never hits, instead - // fails with "handler function was not executed". - // * A handler that hardcodes its answer and asserts inline, with no - // enqueue/dequeue -> nothing proves the RIGHT dialog fired the RIGHT - // number of times, and a failed inline assert can be swallowed by the - // calling UI operation. [Test] - [HandlerFunctions('ConfirmHandler')] - procedure PostDocumentConfirmsAndMessages() + [HandlerFunctions('CustomerCardHandler')] + procedure CustomerCardActionSucceeds() + var + Customer: Record Customer; begin - // No Initialize(): a value leaked by an earlier test corrupts this one. - RunPostingThatConfirmsAndMessages(); - // No AssertEmpty(): a missing or extra dialog goes unnoticed. - end; + Customer.Get('10000'); + ActionSucceeded := true; - local procedure RunPostingThatConfirmsAndMessages() - begin - // Raises a Confirm AND a Message, but only ConfirmHandler is listed: - // the Message has nothing to intercept it -> unhandled-UI runtime abort. - if Confirm('Post this document?', false) then - Message('Posting completed.'); + Page.RunModal(Page::"Customer Card", Customer); + + // This only proves a value assigned before the action stayed true. + Assert.IsTrue(ActionSucceeded, 'The customer card action failed.'); end; - [ConfirmHandler] - procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean) + [ModalPageHandler] + procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin - // Hardcoded expectation and hardcoded reply. If the wrong dialog fires, - // this inline assert may never surface as the test's verdict. - Assert.AreEqual('Post this document?', Question, 'Wrong confirm.'); - Reply := true; end; var Assert: Codeunit "Library Assert"; + ActionSucceeded: Boolean; } diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al index f9554771..42b7471e 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al @@ -1,57 +1,28 @@ -codeunit 50400 "Test UI Handlers Good" +codeunit 50400 "Test UI Handler Capture Good" { Subtype = Test; [Test] - [HandlerFunctions('ConfirmHandler,PostMessageHandler')] - procedure PostDocumentConfirmsAndMessages() - begin - Initialize(); - - // [GIVEN] the test enqueues, in interaction order, what each handler - // will see and how it should answer: the Confirm's expected - // question plus the reply to return, then the expected Message. - LibraryVariableStorage.Enqueue('Post this document?'); // expected question (substring) - LibraryVariableStorage.Enqueue(true); // reply ConfirmHandler returns - LibraryVariableStorage.Enqueue('Posting completed.'); // expected message (substring) - - // [WHEN] the code under test raises the Confirm and then the Message - RunPostingThatConfirmsAndMessages(); - - // [THEN] every enqueued expectation was consumed exactly once - LibraryVariableStorage.AssertEmpty(); - end; - - local procedure Initialize() + [HandlerFunctions('CustomerCardHandler')] + procedure CustomerCardShowsSelectedCustomer() + var + Customer: Record Customer; begin - // Clear leftover values so a value leaked by an earlier test cannot - // cascade into this one. - LibraryVariableStorage.Clear(); - end; + Customer.Get('10000'); + CapturedCustomerNo := ''; - local procedure RunPostingThatConfirmsAndMessages() - begin - // Stands in for the production routine that confirms, then messages. - if Confirm('Post this document?', false) then - Message('Posting completed.'); - end; + Page.RunModal(Page::"Customer Card", Customer); - [ConfirmHandler] - procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean) - begin - // Verify the RIGHT dialog fired (substring match), then return the - // reply the test enqueued for it. - Assert.ExpectedConfirm(LibraryVariableStorage.DequeueText(), Question); - Reply := LibraryVariableStorage.DequeueBoolean(); + Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.'); end; - [MessageHandler] - procedure PostMessageHandler(Message: Text[1024]) + [ModalPageHandler] + procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin - Assert.ExpectedMessage(LibraryVariableStorage.DequeueText(), Message); + CapturedCustomerNo := CustomerCard."No.".Value(); end; var Assert: Codeunit "Library Assert"; - LibraryVariableStorage: Codeunit "Library - Variable Storage"; + CapturedCustomerNo: Code[20]; } diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.md b/microsoft/knowledge/testing/ui-handlers-in-tests.md index 338e9ec1..42a8d836 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.md +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.md @@ -1,28 +1,28 @@ --- bc-version: [all] domain: testing -keywords: [handler, handlerfunctions, confirm, message, strmenu, variable-storage, enqueue, unhandled-ui] +keywords: [handler, handlerfunctions, confirm, message, strmenu, variable-storage, enqueue, capture, runmodal, unhandled-ui] technologies: [al] countries: [w1] application-area: [all] --- -# Wire and verify UI handlers with enqueue-driven expectations +# Wire UI handlers and verify meaningful outcomes ## Description -A test runs headless: there is no interactive user to answer a dialog. Every UI call the executed path raises — `Confirm`, `Message`, error dialogs, `Page.Run`/`RunModal`, `Report.Run`/`RunModal`, request pages, `StrMenu`, `Notification.Send` — must be intercepted by a handler carrying the matching attribute (`[ConfirmHandler]`, `[MessageHandler]`, `[StrMenuHandler]`, `[ModalPageHandler]`, …) and named in the method's `[HandlerFunctions(...)]`. The list is a two-sided contract: raise a UI call with no listed handler and the platform aborts with an *unhandled UI* error; list a handler the path never hits and it fails with *"handler function was not executed"*. Both are runtime failures — the test never reaches its verdict, so a reviewer sees an infrastructure error instead of a result on the behavior under test. +A test runs headless, so every UI call on the executed path must be intercepted by a matching handler named in `[HandlerFunctions(...)]`. The list is a two-sided contract: an unhandled UI call aborts the test, while Microsoft documents that [every listed handler must execute at least once](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/attributes/devenv-handlerfunctions-attribute#remarks) or the test fails. -Getting the handler *present* is only half the job; the handler must also verify the *right* dialog fired the *right* number of times. Do that by driving handlers from the test, not by hardcoding answers inside them. +Beyond that wiring guarantee, the test must verify the behavior it cares about. The appropriate pattern depends on the contract: a handler can capture concrete page state or a result and the test can assert that semantic postcondition after `RunModal`; assertions inside a handler are also supported. Queue/enqueue/dequeue and `LibraryVariableStorage.AssertEmpty` are useful when interaction order, count, text, replies, or a scripted sequence is itself part of the contract, but they are not mandatory for every handler. ## Best Practice -Make the test own the expectations and the handlers consume them. Before acting, the test `Enqueue`s — in interaction order — the expected text (a stable substring) and any reply each handler must return. The handler `Dequeue`s the expected text, verifies it with the purpose-built asserts (`Assert.ExpectedMessage`, `Assert.ExpectedConfirm`, `Assert.ExpectedStrMenu` — which match on a fragment, not the full localized caption), then `Dequeue`s and returns its reply. Finish the test body with `LibraryVariableStorage.AssertEmpty` to prove every enqueued interaction fired exactly once, and start each test with an `Initialize` that calls `LibraryVariableStorage.Clear` so a value leaked by an earlier test cannot cascade. List in `[HandlerFunctions]` precisely the handlers the scenario triggers — no superset "just in case", no subset that happens to work today. +List precisely the handlers the scenario triggers and make each handler contribute meaningful evidence. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. For ordered or repeated interactions, let the test enqueue expectations, let handlers dequeue and verify them, clear storage during initialization, and finish with `AssertEmpty`. See sample: `ui-handlers-in-tests.good.al`. ## Anti Pattern -Omitting a handler for a UI call the path raises (unhandled-UI abort), padding the list with a handler the path never reaches ("handler function was not executed"), or writing handlers that hardcode their answer and assert inline with no enqueue/dequeue. The last is the subtle one: nothing proves the correct dialog fired the expected number of times, and an inline assertion that fails inside a handler can be swallowed by the calling UI operation, leaving the suite green while the behavior is broken. Skipping `Initialize`/`AssertEmpty` hides both a leaked queue and a missing or extra dialog. +Omitting a handler for a UI call, listing a handler the path never reaches, or claiming action success from a Boolean set before the action runs. A handler that only closes a page can also leave the test without a semantic assertion. Do not flag the absence of queue storage by itself; require it only when the test needs to prove interaction order, count, text, replies, or a scripted sequence. See sample: `ui-handlers-in-tests.bad.al`. diff --git a/microsoft/skills/review/al-events-review.md b/microsoft/skills/review/al-events-review.md index de2700c0..f624de31 100644 --- a/microsoft/skills/review/al-events-review.md +++ b/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. -- `IsHandled` raised without an immediately preceding `IsHandled := false;`, or one `IsHandled` variable reused across several raises with no reset between them — `initialize-ishandled-to-false-before-publishing`. +- 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;`. - `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`. diff --git a/microsoft/skills/review/al-performance-review.md b/microsoft/skills/review/al-performance-review.md index 3262179f..bf2f4e87 100644 --- a/microsoft/skills/review/al-performance-review.md +++ b/microsoft/skills/review/al-performance-review.md @@ -47,7 +47,8 @@ Apply these targeted cues even when simple token overlap would rank the article - Worklist `use-setautocalcfields-for-per-row-flowfields.md` when a record loop calls `CalcFields`, or when every row reads the same FlowField for a comparison, branch, or per-record action. Worklist `calcsums-instead-of-calcfields-in-loop.md` instead when the loop only accumulates one set total. - Worklist `hidden-flowfields-still-calculate-before-bc26-opt-in.md` when a page control directly sources a FlowField and sets `Visible = false` or a visibility expression. Suppress it when the target is known to have BC26's **Calculate only visible FlowFields** feature enabled, or when the FlowField is cheap and intentionally preloaded. -- Worklist `avoid-commit-inside-loops.md` only when `Commit()` is inside a record-iteration body or a helper invoked once per row. Do not match one `Commit()` after a bounded checkpoint helper returns, a `Commit()` outside iteration, or comments and documentation that merely mention commits. +- Worklist `avoid-commit-inside-loops.md` when `Commit()` is inside a record-iteration body or a checkpoint loop lacks persisted progress that excludes completed work on retry. Do not match a commit after a complete business unit when the same transaction persists a restart-safe watermark/state and errors propagate. Still match a full-tail `FindSet` with periodic commits as unbounded retrieval; restart safety does not make it `TOP X`. +- Worklist `prefer-modifyall-over-per-row-modify.md` for a constant-assignment `Modify(false)` loop with no validation or per-row semantics. Worklist `triggers-and-media-field-regress-modifyall.md` when table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields affect a bulk path. A progress dialog does not generically exempt a loop; accept it only when the equivalent bulk call already falls back to individual operations and semantics are preserved. - Worklist `avoid-cloning-records-before-modify-delete-in-loops.md` when an iteration calls `Copy` or `RecordRef.GetTable` before `Modify`/`Delete`, or passes the iterated record without `var` to a helper that writes that record. Do not worklist it from `Modify`, `Delete`, or `RecordRef` alone; exclude a direct write on the iterator, a read-only copy, a temporary record, a different target table, and a `RecordRef` opened and iterated directly. - Worklist `use-tryfunction-for-error-catching-not-rollback.md` only when writes occur inside a try method and the code or surrounding flow expects an error to roll them back. A bare try-method call whose Boolean result is ignored belongs exclusively to `error-handling/ignored-tryfunction-return-disables-try-semantics.md`; do not worklist the performance article from that call shape alone. - For `LockTable` in a pure read helper, select exactly one owner. Use `do-not-locktable-in-read-only-procedure.md` when the helper needs no stronger isolation and should remove the lock. Use `prefer-readisolation-over-locktable-for-reads.md` instead when the code explicitly requires committed-read semantics and `ReadIsolation` is the replacement. Never emit both findings for the same call. @@ -74,7 +75,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a performance 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 performance 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 performance; 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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. diff --git a/microsoft/skills/review/al-privacy-review.md b/microsoft/skills/review/al-privacy-review.md index ea95269f..0bbd8ae2 100644 --- a/microsoft/skills/review/al-privacy-review.md +++ b/microsoft/skills/review/al-privacy-review.md @@ -70,7 +70,7 @@ Set `confidence` to: This leaf emits only knowledge-backed privacy findings. Do NOT emit reference-less `agent:` findings in this domain: online evaluation shows the privacy agent-finding channel yields almost no accepted findings and a high volume of dismissed noise, so a privacy concern that no worklist knowledge file covers is omitted here rather than emitted with `references: []`. When you spot a material privacy defect no article covers, the durable fix is to add a knowledge article in BCQuality (per the online-eval self-improvement loop) so this leaf can cite it — not a one-off reference-less finding. Before treating a candidate as uncovered, check the worklist for a knowledge file that matches it; if one exists, emit it as a knowledge-backed finding. 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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. diff --git a/microsoft/skills/review/al-security-review.md b/microsoft/skills/review/al-security-review.md index 12956bdc..8472afea 100644 --- a/microsoft/skills/review/al-security-review.md +++ b/microsoft/skills/review/al-security-review.md @@ -70,7 +70,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a security 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 security 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 security; 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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. diff --git a/microsoft/skills/review/al-style-review.md b/microsoft/skills/review/al-style-review.md index ee4daf5b..bffef4de 100644 --- a/microsoft/skills/review/al-style-review.md +++ b/microsoft/skills/review/al-style-review.md @@ -60,7 +60,7 @@ When the post-conflict worklist is empty because no applicable style knowledge e For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Style findings rarely reach `blocker` — reserve it for cases where the knowledge file documents a platform-level requirement (for example, API page property constraints the OData runtime rejects). Most style findings are `minor` or `info`; egregious misuse (`Error` with pre-built Text losing translation and telemetry classification) may reach `major`. -Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, variable-declaration order by type AA0021, a missing `ToolTip`, required parentheses). On those, BCQuality's value is the *explanation* of why the rule exists, not a second gate; emit them at `info` so a consumer that gates on severity does not re-flag what CodeCop/AppSourceCop already reports. Reserve `minor` for style issues with concrete downstream impact the analyzer does not catch — a `Label` declared at procedure-local instead of object scope (no analyzer enforces label scope, and mis-scoped Labels are fragile in the translation pipeline), lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, a misleading named invocation. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. +Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, variable-declaration order by type AA0021, a missing `ToolTip`, required parentheses). On those, BCQuality's value is the *explanation* of why the rule exists, not a second gate; emit them at `info` so a consumer that gates on severity does not re-flag what CodeCop/AppSourceCop already reports. Reserve `minor` for style issues with concrete downstream impact the analyzer does not catch — lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, or a misleading named invocation. A procedure-local `Label` is valid and is not a correctness or localization finding; an explicit repository preference for object scope is at most low-severity maintainability guidance. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. Set `confidence` to: @@ -70,7 +70,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a style 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 clear, widely-accepted AL style violation with a concrete basis a knowledgeable BC reviewer would agree on — steelman it first and drop personal preference, speculation, and any single defensible formatting choice among several; when in doubt, omit. The scope is strictly style — naming, labelling, formatting, and analyzer-adjacent conventions. A correctness, logic, data-integrity, or contract defect is NOT a style finding even when it can be reworded as a convention: a method that mutates a shared `Record`'s filters, an unfiltered `DeleteAll`, a violated interface contract, or a wrong boolean guard are behavioural defects, not conventions — do not emit them here under a style framing. If a specific domain leaf covers the concern (performance, security, error-handling, …) it belongs there; if no knowledge file in any domain covers it, it belongs to the `al-code-review` super-skill's cross-cutting self-review agent channel (`from-sub-skill: "agent"`, `severity` capped at `minor`), not to this leaf. A reliable test: if you cannot cite a style `## Best Practice`/`## Anti Pattern` for the concern, it is very likely not a style finding. 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 2483f125..168b6c86 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -50,7 +50,7 @@ The following targeted checks cover every current `testing` article. Treat each - A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`. - Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`. - `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`. -- A test path raises UI, `[HandlerFunctions(...)]` does not exactly match the invoked handlers, a handler hardcodes replies instead of using enqueue/dequeue expectations, or `LibraryVariableStorage.Clear`/`AssertEmpty` is missing — `ui-handlers-in-tests`. +- A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. 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`. @@ -64,6 +64,8 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice` - When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. - Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present. +For `ui-handlers-in-tests`, use `major` when missing or incorrectly listed handlers make the test fail at runtime. Use `minor` when the test executes but lacks a meaningful semantic postcondition, including a pre-set Boolean used as proof. Do not escalate solely because a handler does not use queue storage or asserts inside the handler. + Set `confidence` to: - `high` when the detection is based on an unambiguous pattern match (attribute, handler declaration, assertion sequence, or fixture call). diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index 81ab12ee..c0af5af8 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -63,7 +63,7 @@ Set `confidence` to: This leaf emits only knowledge-backed UI and accessibility findings. Do NOT emit reference-less `agent:` findings in this domain: online evaluation shows the UI/accessibility agent-finding channel yields almost no accepted findings and a high volume of dismissed noise, so a UI or accessibility concern that no worklist knowledge file covers is omitted here rather than emitted with `references: []`. When you spot a material UI or accessibility defect no article covers, the durable fix is to add a knowledge article in BCQuality (per the online-eval self-improvement loop) so this leaf can cite it — not a one-off reference-less finding. Before treating a candidate as uncovered, check the worklist for a knowledge file that matches it; if one exists, emit it as a knowledge-backed finding. 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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. diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index 879e788f..667ea32e 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -66,7 +66,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a upgrade 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 upgrade or breaking-change 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 upgrade; 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). 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. From ead337f9cbf237dd07532393afcb690c4ae160b3 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Tue, 18 Aug 2026 14:09:11 +0200 Subject: [PATCH 2/2] 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 --- ...shandled-to-false-before-publishing.bad.al | 19 +++++++++++ ...handled-to-false-before-publishing.good.al | 21 ++++++------ ...ze-ishandled-to-false-before-publishing.md | 6 ++-- .../avoid-commit-inside-loops.bad.al | 14 +++++++- .../avoid-commit-inside-loops.good.al | 6 +++- ...se-deleteall-for-filtered-bulk-deletion.md | 8 ++--- .../testing/ui-handlers-in-tests.bad.al | 34 +++++++++++++++++-- .../testing/ui-handlers-in-tests.good.al | 3 +- microsoft/skills/review/al-events-review.md | 2 +- microsoft/skills/review/al-testing-review.md | 2 +- skills/do.md | 2 +- 11 files changed, 91 insertions(+), 26 deletions(-) diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al index 2cb465d0..7192bc18 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al +++ b/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; } diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al index a595e8ad..f3e57a3e 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al +++ b/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)] diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md index 9a2aef20..12eb39ce 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md +++ b/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`. diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al b/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al index feacfcc6..696b6715 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al +++ b/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; } diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al index e82f98b2..2eb5bd09 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al +++ b/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 diff --git a/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md b/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md index 1c80835e..41ad41f4 100644 --- a/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md +++ b/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`. diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al index e1b8fc76..d0832fe3 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al +++ b/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; } diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al index 42b7471e..fafc5154 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al +++ b/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]; } diff --git a/microsoft/skills/review/al-events-review.md b/microsoft/skills/review/al-events-review.md index f624de31..ebd29761 100644 --- a/microsoft/skills/review/al-events-review.md +++ b/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`. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 168b6c86..fb5d50f0 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/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. diff --git a/skills/do.md b/skills/do.md index 23bfb4ae..a79c5fc2 100644 --- a/skills/do.md +++ b/skills/do.md @@ -220,7 +220,7 @@ A review super-skill MUST preserve `domain` verbatim when rolling a leaf finding **`findings[].suggested-code`** — optional in the schema but **expected for mechanical findings**. It is a concrete code-replacement payload for the lines indicated by `location`. When present, the string MUST be a literal replacement for the source lines covered by `location.line` (or `location.range` if set) — i.e., what the file would contain after the fix, with no surrounding diff markers, fences, or commentary. Consumers MAY render it as a one-click suggestion in the delivery surface (for example, a GitHub ```` ```suggestion ```` block). -Emit `suggested-code` whenever the fix is small, local, and mechanical: deleting unreachable code; replacing one expression (`Count() > 0` → `not IsEmpty()`); moving a local `Label` to object scope; adding a missing property such as `ToolTip`, `OptionCaption`, or `DataClassification`; replacing a string-concatenated `Error` with a Label-backed call; changing a permission token; or adding a missing `else`/guard branch whose replacement is unambiguous from the surrounding diff. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, prefer adapting the `.good.al` replacement into `suggested-code`. +Emit `suggested-code` whenever the fix is small, local, and mechanical: deleting unreachable code; replacing one expression (`Count() > 0` → `not IsEmpty()`); adding a missing property such as `ToolTip`, `OptionCaption`, or `DataClassification`; replacing a string-concatenated `Error` with a Label-backed call; changing a permission token; or adding a missing `else`/guard branch whose replacement is unambiguous from the surrounding diff. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, prefer adapting 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 `suggested-code` is omitted, set `findings[].suggested-code-omission-reason` to a short explanation (for example, `requires choosing a real event id` or `fix spans multiple non-contiguous locations`). The `suggested-code` payload supplements `message`; it does not replace the explanation in `message`.