Microsoft/bcquality
1
0
Fork 0
mirror of https://github.com/microsoft/BCQuality.git synced 2026-10-06 07:06:54 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Merge main into PR #208 and preserve both review routes

Browse source 
Resolve data-modeling applicability, token, and not-applicable conflicts by retaining TableRelation guidance alongside master-record insert/delete routing. Preserve approved canonical knowledge and samples, RecordRef exclusions, and all fixture registrations.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Jesper Schulz-Wedde 2026-10-05 15:09:21 +02:00
commit 1d140cb4fe
35 changed files with 1260 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

12
CODEOWNERS
View file

@ -6,13 +6,13 @@
/.github/ @jesperschulz
# Domain experts — required reviewers for coding rules
/microsoft/knowledge/events/ @AleksandricMarko @pchriste-microsoft-com
/microsoft/knowledge/performance/ @BardurKnudsen @pchriste-microsoft-com
/microsoft/knowledge/privacy/ @haoranpb @pchriste-microsoft-com
/microsoft/knowledge/security/ @darjoo @WaelAbuSeada @Aleyenda @pchriste-microsoft-com
/microsoft/knowledge/events/ @AleksandricMarko @pchriste-microsoft-com @jesperschulz
/microsoft/knowledge/performance/ @BardurKnudsen @pchriste-microsoft-com @jesperschulz
/microsoft/knowledge/privacy/ @haoranpb @pchriste-microsoft-com @jesperschulz
/microsoft/knowledge/security/ @darjoo @WaelAbuSeada @Aleyenda @pchriste-microsoft-com @jesperschulz
/microsoft/knowledge/style/ @nikolakukrika @jesperschulz @pchriste-microsoft-com
/microsoft/knowledge/testing/ @nikolakukrika @ventselartur @pchriste-microsoft-com
/microsoft/knowledge/upgrade/ @nikolakukrika @pchriste-microsoft-com
/microsoft/knowledge/testing/ @nikolakukrika @ventselartur @pchriste-microsoft-com @jesperschulz
/microsoft/knowledge/upgrade/ @nikolakukrika @pchriste-microsoft-com @jesperschulz
# Community content — open to broader review
# /community/ reviewers are added as the contributor base grows

18
evaluation/review-fixtures.json
View file

@ -17,11 +17,13 @@
"check-blocked-in-referencing-code-not-in-master",
"code-must-not-change-workdate",
"custom-document-dispatch-must-not-bypass-report-selections",
"delete-master-data-with-trigger",
"document-line-prices-follow-prices-including-vat",
"document-print-and-email-actions-call-report-selections-directly",
"extend-find-entries-navigate-for-new-document-types",
"extend-price-source-type-must-sync-document-subset-enum",
"extend-report-selection-usage-for-new-document-types",
"master-data-must-be-inserted-with-trigger",
"new-price-source-must-add-candidate-and-trigger-recalculation",
"pictures-must-use-media-not-blob",
"report-barcodes-must-use-barcode-module-and-production-font-name",
@ -33,6 +35,7 @@
"error-handling": {
"articles": [
"collect-validation-errors-with-errorbehavior",
"declined-confirm-must-abort-not-partially-apply",
"defensive-vs-offensive-code-must-match-blast-radius",
"log-writes-must-survive-rollback"
]
@ -40,7 +43,8 @@
"events": {
"articles": [
"reset-ishandled-only-when-the-value-can-carry-over",
"changecompany-runs-triggers-in-the-calling-company"
"changecompany-runs-triggers-in-the-calling-company",
"database-trigger-setup-flags-may-only-be-set-to-true"
]
},
"finance": {
@ -86,11 +90,16 @@
"use-setloadfields-for-partial-records",
"prefer-modifyall-over-per-row-modify",
"al-methods-limited-during-write-transactions",
"avoid-user-prompts-inside-transactions"
"avoid-user-prompts-inside-transactions",
"use-grouped-query-for-distinct-values-and-duplicates"
]
},
"privacy": {
"article": "no-pii-in-telemetry-message-string"
"articles": [
"no-pii-in-telemetry-message-string",
"register-owned-log-tables-for-retention-policies",
"ship-a-default-retention-policy-setup"
]
},
"query": {
"articles": [
@ -139,7 +148,8 @@
"label-comment-explains-placeholders",
"dateformula-evaluate-needs-language-independent-literals",
"al-comments-must-not-restate-what-code-already-shows",
"pages-must-not-contain-business-logic"
"pages-must-not-contain-business-logic",
"mutating-procedure-for-a-page-caller-takes-var-record"
]
},
"telemetry": {

12
microsoft/knowledge/data-modeling/delete-master-data-with-trigger.bad.al Normal file
View file

@ -0,0 +1,12 @@
codeunit 50100 "Sample Currency Cleanup"
{
procedure DeleteRetiredCurrencies(CurrencyFilter: Text)
var
Currency: Record Currency;
begin
Currency.SetFilter(Code, CurrencyFilter);
// RunTrigger defaults to false: Currency.OnDelete never runs, so the
// open-entry guard is skipped and exchange rates are left orphaned.
Currency.DeleteAll();
end;
}

12
microsoft/knowledge/data-modeling/delete-master-data-with-trigger.good.al Normal file
View file

@ -0,0 +1,12 @@
codeunit 50100 "Sample Currency Cleanup"
{
procedure DeleteRetiredCurrencies(CurrencyFilter: Text)
var
Currency: Record Currency;
begin
Currency.SetFilter(Code, CurrencyFilter);
// DeleteAll(true) runs Currency.OnDelete for each record: it errors while
// open ledger entries use the code and removes the exchange rates itself.
Currency.DeleteAll(true);
end;
}

39
microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md Normal file
View file

@ -0,0 +1,39 @@
---
bc-version: [all]
domain: data-modeling
keywords: [delete, deleteall, runtrigger, ondelete, master-data, currency, cleanup, data-migration]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Delete master and reference data with `Delete(true)` so the owning table's `OnDelete` decides
## Description
`Record.Delete()` and `Record.DeleteAll()` do not run `OnDelete` unless `RunTrigger` is `true`; the default is `false`. On a master or reference table, `OnDelete` is where Business Central decides whether the delete is safe and removes what the record owns. `Currency.OnDelete` refuses the delete while any **open** customer, vendor, or employee ledger entry uses the code, then deletes the currency's `Currency Exchange Rate` rows itself. `Customer.OnDelete` refuses when a job bills the customer, calls codeunit 361 `MoveEntries` (which refuses while ledger entries fall in an unclosed fiscal year or are still open, and otherwise detaches the closed history), and removes default dimensions, related data, and the contact link.
A cleanup, migration, or "remove obsolete codes" routine that calls `Delete()`/`DeleteAll()` without `true` on such a table skips all of it (only `OnBeforeDelete`/`OnAfterDelete` triggers in table extensions still run): it can remove a currency that open entries still use, and leaves exchange rates and other dependents orphaned. That a record *looks* obsolete — a superseded currency nobody posts in any more — is no evidence the guard would pass, and keeping a record that closed history still refers to is often the better choice. Where the master has a `Blocked` field, blocking it is an alternative to deleting it; `Currency` has none.
## Best Practice
Outside the owning table's own triggers, delete master and reference records with `Delete(true)`/`DeleteAll(true)` and let `OnDelete` raise its error, as BCApps does when it removes items (`CatalogItemManagement`, `NewItem.Delete(true)`). This is the "trigger does work the caller depends on" case of [`pass-false-to-insert-when-trigger-not-needed`](../performance/pass-false-to-insert-when-trigger-not-needed.md); a hand-written reference check is no substitute for the guard.
Legitimate `false` deletes, not in scope: the owning table's own `OnDelete` cascade removing its dependents (`Currency.OnDelete` itself calls `CurrExchRate.DeleteAll()`; see [`owning-table-must-delete-dependents-in-ondelete`](owning-table-must-delete-dependents-in-ondelete.md)); temporary records and buffers; deleting and immediately re-inserting the same primary key to restore or recreate a record, where dependents stay valid (`JobArchiveManagement` restoring a project from its archive; codeunit 1812 recreating each `"Customer Posting Group"` with the same `Code`); and a data-migration or setup reset in a company with no posted entries that removes master rows and their setup references as one rebuild, such as codeunit 1812 `"Data Migration Del G/L Account"`, whose `DeleteAll()` bypasses `"G/L Account".OnDelete`'s `MoveGLEntries` guard — acceptable only because no postings exist yet. Posted ledger entries are not master data; see [`do-not-modify-or-delete-posted-ledger-entries`](../finance/do-not-modify-or-delete-posted-ledger-entries.md).
See sample: [`delete-master-data-with-trigger.good.al`](delete-master-data-with-trigger.good.al).
## Anti Pattern
Code outside the owning table's `OnDelete` calls `Delete()`/`DeleteAll()` (or `false`) on a non-temporary master or reference table — `Currency`, `Customer`, `Vendor`, `Item`, `G/L Account`, or a custom equivalent with an `OnDelete` guard or cascade — typically from a filter on codes judged obsolete, outside a same-key delete-and-reinsert or a no-postings migration/setup rebuild.
See sample: [`delete-master-data-with-trigger.bad.al`](delete-master-data-with-trigger.bad.al).
## References
- [Record.Delete method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-delete-method) and [Record.DeleteAll method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-deleteall-method): `RunTrigger` "The default value is false."; the DeleteAll note adds that setting it to false "only affects the OnDelete trigger" — table-extension `OnBeforeDelete`/`OnAfterDelete` still run.
- BCApps `src/Layers/W1/BaseApp/Finance/Currency/Currency.Table.al`, `OnDelete`, lines 797-820.
- BCApps `src/Layers/W1/BaseApp/Sales/Customer/Customer.Table.al`, `OnDelete`, lines 2401-2430; `src/Layers/W1/BaseApp/Utilities/MoveEntries.Codeunit.al`, `MoveCustEntries`, lines 126-167.
- BCApps `src/Layers/W1/BaseApp/Inventory/Item/Catalog/CatalogItemManagement.Codeunit.al`, line 419.
- BCApps `src/Layers/W1/BaseApp/System/DataMigration/DataMigrationDelGLAccount.Codeunit.al`, `OnRun` lines 18-32 and `DeleteGLAccounts` lines 34-42 (rebuild); lines 53-56 (same-key delete and re-insert); `src/Layers/W1/BaseApp/Finance/GeneralLedger/Account/GLAccount.Table.al`, `OnDelete`, line 1144 (`MoveGLEntries`).
- BCApps `src/Layers/W1/BaseApp/Projects/Project/Archive/JobArchiveManagement.Codeunit.al`, lines 212-219 (restore: `Job.Delete()`, then re-insert the same `No.`).

15
microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.bad.al Normal file
View file

@ -0,0 +1,15 @@
codeunit 50100 "Sample Customer Import"
{
procedure CreateCustomer(ExternalName: Text[100]; ExternalCountry: Code[10]): Code[20]
var
Customer: Record Customer;
begin
Customer.Init();
Customer.Validate(Name, ExternalName);
Customer.Validate("Country/Region Code", ExternalCountry);
// RunTrigger defaults to false: OnInsert never runs, so "No." stays
// blank and no contact, salesperson, or timestamps are set.
Customer.Insert();
exit(Customer."No.");
end;
}

16
microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al Normal file
View file

@ -0,0 +1,16 @@
codeunit 50100 "Sample Customer Import"
{
procedure CreateCustomer(ExternalName: Text[100]; ExternalCountry: Code[10]): Code[20]
var
Customer: Record Customer;
begin
Customer.Init();
// Insert(true) runs Customer.OnInsert: "No." from the number series,
// contact and salesperson defaults (when set up), global dimensions, timestamps.
Customer.Insert(true);
Customer.Validate(Name, ExternalName);
Customer.Validate("Country/Region Code", ExternalCountry);
Customer.Modify(true);
exit(Customer."No.");
end;
}

39
microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md Normal file
View file

@ -0,0 +1,39 @@
---
bc-version: [all]
domain: data-modeling
keywords: [insert, runtrigger, oninsert, master-data, no-series, customer, item, import]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Create master records with `Insert(true)` unless the caller does the trigger's work itself
## Description
`Record.Insert()` does not run `OnInsert`: `RunTrigger` defaults to `false`. On a standard master table that trigger initializes the record. Unless an `OnBeforeInsert` subscriber sets `IsHandled`, `Customer.OnInsert` assigns `No.` and `No. Series` from Sales & Receivables Setup when `No.` is blank, then defaults `Invoice Disc. Code`, defaults a blank salesperson from the user's User Setup `"Salespers./Purch. Code"` when one is set, creates the contact when Marketing Setup has a `"Bus. Rel. Code for Customers"` (unless the insert comes from a contact or from a template with a contact), overwrites `Global Dimension 1/2 Code` from the customer's Default Dimension rows and clears them when there are none (`DimMgt.UpdateDefaultDim` creates no default dimensions), calls `UpdateReferencedIds`, and sets the last-modified timestamps. `Item.OnInsert` assigns `No.`, `No. Series`, and `Costing Method` when `No.` is blank and, blank or not, runs the same global-dimension update and `UpdateReferencedIds`.
A bare `Insert()` produces a row that looks complete but lacks what downstream code assumes: with a blank `No.` the key stays blank; with a supplied `No.` the contact, defaults, and timestamps are silently missing. This is the caller-side counterpart of [`master-table-no-from-number-series-in-oninsert`](master-table-no-from-number-series-in-oninsert.md): that design only works when callers run the trigger.
## Best Practice
When code creates a record in `Customer`, `Vendor`, `Item`, `G/L Account`, `Contact`, or a custom master with initializing `OnInsert` logic, call `Insert(true)`, then validate fields and `Modify(true)`. Importing from an external source is not an exception: BCApps' data-migration facades (`CustomerDataMigrationFacade`, `ItemDataMigrationFacade`, `GLAccDataMigrationFacade`) use `Insert(true)`. This is the "trigger does work the caller depends on" case of [`pass-false-to-insert-when-trigger-not-needed`](../performance/pass-false-to-insert-when-trigger-not-needed.md); the decision stays per call.
Legitimate `Insert()` calls, not in scope: temporary records and buffer or staging tables; a caller that visibly assigns what the trigger would and then applies a template (`CatalogItemManagement.CreateNewItem` sets `No.` and `Costing Method` before `Item.Insert()`) or copies from a source record (`CopyItem` transfers the source item's fields and assigns the target `No.` before `TargetItem.Insert()`); and an XMLport that round-trips complete rows exported from Business Central (`ExportItemData`). `Modify()` without the trigger is routine on masters for technical fields and is not covered. Upgrade code that bypasses triggers is covered by [`datatransfer-skips-triggers-and-subscribers`](../upgrade/datatransfer-skips-triggers-and-subscribers.md).
See sample: [`master-data-must-be-inserted-with-trigger.good.al`](master-data-must-be-inserted-with-trigger.good.al).
## Anti Pattern
Code creates a non-temporary master record with `Init`, field assignments or `Validate` calls, and `Insert()`/`Insert(false)`, without itself assigning the number and the other fields `OnInsert` would set.
See sample: [`master-data-must-be-inserted-with-trigger.bad.al`](master-data-must-be-inserted-with-trigger.bad.al).
## References
- [Record.Insert(Boolean) method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-insert-boolean-method): "If this parameter is false, the code in the OnInsert trigger is not executed. The default value is false."
- BCApps `src/Layers/W1/BaseApp/Sales/Customer/Customer.Table.al`, `OnInsert`, lines 2432-2472; `src/Layers/W1/BaseApp/Inventory/Item/Item.Table.al`, `OnInsert`, from line 2587.
- BCApps `src/Layers/W1/BaseApp/Sales/Customer/Customer.Table.al`, `SetDefaultSalesperson`, lines 3848-3863; `src/Layers/W1/BaseApp/CRM/BusinessRelation/CustContUpdate.Codeunit.al`, `OnInsert`, lines 26-40.
- BCApps `src/Layers/W1/BaseApp/Finance/Dimension/DimensionManagement.Codeunit.al`, `UpdateDefaultDim`, lines 894-913.
- BCApps `src/Layers/W1/BaseApp/Inventory/Item/Catalog/CatalogItemManagement.Codeunit.al`, `CreateNewItem`, lines 545-565; `src/Layers/W1/BaseApp/Inventory/Item/CopyItem.Codeunit.al`, `InitTargetItem` and `CopyItem`, lines 107-133; `src/Layers/W1/BaseApp/Inventory/Item/ExportItemData.XmlPort.al`, line 405.
- BCApps `src/Layers/W1/BaseApp/System/DataMigration/`: `CustomerDataMigrationFacade.Codeunit.al` line 67, `ItemDataMigrationFacade.Codeunit.al` line 76, `GLAccDataMigrationFacade.Codeunit.al` line 77.

51
microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al Normal file
View file

@ -0,0 +1,51 @@
table 50100 "Sample Mailbox Watch"
{
fields
{
field(1; "Code"; Code[20])
{
}
field(2; "Watched Email"; Text[250])
{
trigger OnValidate()
var
StopWatchingQst: Label 'Email %1 is being watched. Stop watching it?', Comment = '%1 = previous email address';
begin
if "Watched Email" = xRec."Watched Email" then
exit;
if xRec."Watched Email" <> '' then
if Confirm(StopWatchingQst, false, xRec."Watched Email") then begin
Unsubscribe("Subscription ID");
Clear("Subscription ID");
end;
// On "no" the old subscription is never removed, yet the new value is
// kept and the only field that tracked it is overwritten: it is orphaned.
if "Watched Email" <> '' then
"Subscription ID" := Subscribe("Watched Email");
end;
}
field(3; "Subscription ID"; Guid)
{
Editable = false;
}
}
keys
{
key(PK; "Code")
{
Clustered = true;
}
}
local procedure Subscribe(EmailAddress: Text[250]): Guid
begin
// Registers EmailAddress with the external watch service and returns its subscription.
exit(CreateGuid());
end;
local procedure Unsubscribe(SubscriptionId: Guid)
begin
// Removes the subscription from the external watch service.
end;
}

51
microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al Normal file
View file

@ -0,0 +1,51 @@
table 50100 "Sample Mailbox Watch"
{
fields
{
field(1; "Code"; Code[20])
{
}
field(2; "Watched Email"; Text[250])
{
trigger OnValidate()
var
StopWatchingQst: Label 'Email %1 is being watched. Stop watching it?', Comment = '%1 = previous email address';
begin
if "Watched Email" = xRec."Watched Email" then
exit;
if xRec."Watched Email" <> '' then begin
// Declining cancels the whole change: the field keeps its old value.
if not Confirm(StopWatchingQst, false, xRec."Watched Email") then
Error('');
Unsubscribe("Subscription ID");
Clear("Subscription ID");
end;
if "Watched Email" <> '' then
"Subscription ID" := Subscribe("Watched Email");
end;
}
field(3; "Subscription ID"; Guid)
{
Editable = false;
}
}
keys
{
key(PK; "Code")
{
Clustered = true;
}
}
local procedure Subscribe(EmailAddress: Text[250]): Guid
begin
// Registers EmailAddress with the external watch service and returns its subscription.
exit(CreateGuid());
end;
local procedure Unsubscribe(SubscriptionId: Guid)
begin
// Removes the subscription from the external watch service.
end;
}

44
microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md Normal file
View file

@ -0,0 +1,44 @@
---
bc-version: [all]
domain: error-handling
keywords: [confirm, onvalidate, xrec, abort, revert, side-effect, consistency]
technologies: [al]
countries: [w1]
application-area: [all]
---
# A declined `Confirm` in `OnValidate` must abort or revert, not skip a required side effect
## Description
By the time a field's `OnValidate` body runs, the field already holds its new value. When the trigger asks the user to confirm a side effect that the new value makes **required for consistency** — releasing or replacing state that is tied to the old value and that nothing will reference any more once the change commits — the shape `if Confirm(...) then <side effect>;` with nothing on the `false` branch lets the new value commit while silently skipping that effect. No error is raised, the user sees no indication that anything was declined, and the record is left inconsistent with its own dependents.
Not every confirmed follow-up is required. When the confirmed effect is a convenience the user may legitimately decline, skipping it is correct: `"Sales Header"`'s `UpdateSalesLinesByFieldNo` asks whether to update the lines after a header field changes and, on "no", simply `exit`s — the header keeps its new value and the lines stay as they were, by design. A `Confirm` at the top of an action procedure, before anything has been written, may also just `exit` on "no" (for example the delete action in `"Test Input Groups"`). Neither shape is this anti-pattern. Nor is a declined update whose old state stays valid: `Opportunity`'s `"Campaign No."` `OnValidate` asks before moving open tasks filtered on `xRec."Campaign No."` to the new campaign and does nothing on "no" — the tasks keep pointing at a campaign that still exists.
## Best Practice
Make the declined branch match what "no" means:
- **Cancel the whole change** — raise an error before the side effect. The usual BCApps form inside `OnValidate` is the silent abort `if not Confirm(...) then Error('');` (for example `"Bank Account"`, field `"Disable Bank Rec. Optimization"`, and `"Interaction Template"`, field `"Language Code (Default)"`, which ends `if Confirm(...) then begin ... end else Error('');`). The field trigger documentation states that in case of an error "the user entry is not written to the database."
- **Keep the old value but let the rest of the edit continue** — assign the field back in code. In the `"To-do"` table, field `"Team Code"`, declining the reassignment runs `"Team Code" := xRec."Team Code"`; on a page, `"Upload And Deploy Extension"` resets its sync-mode value to `Add` when the user declines `Force Sync`.
Ask before the side effect runs, and before taking locks the prompt would hold open (see [`avoid-user-prompts-inside-transactions`](../performance/avoid-user-prompts-inside-transactions.md)). When the same validation can run without a UI, a required confirmation must not be silently skipped behind `GuiAllowed`; decide the non-interactive outcome explicitly (see [`job-queue-handlers-must-not-require-ui`](../performance/job-queue-handlers-must-not-require-ui.md)).
See sample: [`declined-confirm-must-abort-not-partially-apply.good.al`](declined-confirm-must-abort-not-partially-apply.good.al).
## Anti Pattern
Inside a field `OnValidate` (or a procedure it calls), a `Confirm` gates a side effect that releases, cancels, or replaces state belonging to the old value (`xRec`), the `false` branch neither errors nor restores the field, and code after it proceeds as if the change were accepted — for example overwriting the only field that tracks the old state. Flag it only when, after the change, nothing references the old state any more, so declining leaves it orphaned. Do not flag declined updates whose old state remains valid and referenced, optional follow-ups whose skipping leaves every record consistent, or `exit` on "no" in an action before any write.
See sample: [`declined-confirm-must-abort-not-partially-apply.bad.al`](declined-confirm-must-abort-not-partially-apply.bad.al).
## References
- [OnValidate (Field) trigger](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/triggers-auto/field/devenv-onvalidate-field-trigger).
- BCApps `src/Layers/W1/BaseApp/Bank/BankAccount/BankAccount.Table.al`, lines 980-988 (silent abort in `OnValidate`).
- BCApps `src/Layers/W1/BaseApp/CRM/Interaction/InteractionTemplate.Table.al`, lines 157-165 (`else Error('')` in `OnValidate`).
- BCApps `src/Layers/W1/BaseApp/CRM/Task/Todo.Table.al`, lines 80-90 (table-field revert to `xRec`).
- BCApps `src/System Application/App/Extension Management/src/UploadAndDeployExtension.Page.al`, lines 78-83 (explicit revert on a page).
- BCApps `src/Layers/W1/BaseApp/CRM/Opportunity/Opportunity.Table.al`, lines 142-157 (declined update; old campaign still valid).
- BCApps `src/Layers/W1/BaseApp/Sales/Document/SalesHeader.Table.al`, `UpdateSalesLinesByFieldNo`, lines 4997-5014 (optional follow-up; `end else exit`).
- BCApps `src/Tools/Test Framework/Test Runner/src/DataDrivenTest/DataInputs/TestInputGroups.Page.al`, lines 85-86 (`exit` before any write).

62
microsoft/knowledge/events/database-trigger-setup-flags-may-only-be-set-to-true.bad.al Normal file
View file

@ -0,0 +1,62 @@
codeunit 50110 "Change Tracking Subscr. Bad"
{
// Self-contained demonstration of the anti pattern. Not derived from base-app source.
[EventSubscriber(ObjectType::Codeunit, Codeunit::GlobalTriggerManagement, OnAfterGetDatabaseTableTriggerSetup, '', false, false)]
local procedure OptInTrackedTables(TableId: Integer; var OnDatabaseInsert: Boolean; var OnDatabaseModify: Boolean; var OnDatabaseDelete: Boolean; var OnDatabaseRename: Boolean)
var
TrackedTable: Record "Tracked Table Bad";
IsTracked: Boolean;
begin
IsTracked := TrackedTable.Get(TableId);
// Stores false for every table this feature does not track, clearing flags that
// Dataverse integration, API webhooks, or another app already set for that table.
OnDatabaseModify := IsTracked;
OnDatabaseDelete := IsTracked;
// Opting out of operations this feature does not need clears them for everyone else too.
OnDatabaseInsert := false;
OnDatabaseRename := false;
end;
[EventSubscriber(ObjectType::Codeunit, Codeunit::GlobalTriggerManagement, OnAfterOnDatabaseModify, '', false, false)]
local procedure LogModify(RecRef: RecordRef)
var
TrackedChange: Record "Tracked Change Bad";
begin
TrackedChange.Init();
TrackedChange."Table No." := RecRef.Number();
TrackedChange."Record ID" := RecRef.RecordId();
TrackedChange.Insert();
end;
}
table 50110 "Tracked Table Bad"
{
DataClassification = SystemMetadata;
fields
{
field(1; "Table No."; Integer) { }
}
keys
{
key(PK; "Table No.") { Clustered = true; }
}
}
table 50111 "Tracked Change Bad"
{
DataClassification = SystemMetadata;
fields
{
field(1; "Entry No."; Integer) { AutoIncrement = true; }
field(2; "Table No."; Integer) { }
field(3; "Record ID"; RecordId) { }
}
keys
{
key(PK; "Entry No.") { Clustered = true; }
}
}

65
microsoft/knowledge/events/database-trigger-setup-flags-may-only-be-set-to-true.good.al Normal file
View file

@ -0,0 +1,65 @@
codeunit 50112 "Change Tracking Subscr. Good"
{
// Self-contained demonstration of the best practice. Not derived from base-app source.
[EventSubscriber(ObjectType::Codeunit, Codeunit::GlobalTriggerManagement, OnAfterGetDatabaseTableTriggerSetup, '', false, false)]
local procedure OptInTrackedTables(TableId: Integer; var OnDatabaseInsert: Boolean; var OnDatabaseModify: Boolean; var OnDatabaseDelete: Boolean; var OnDatabaseRename: Boolean)
var
TrackedTable: Record "Tracked Table Good";
begin
// Only ever turn flags on; flags set by other features stay untouched.
if not TrackedTable.Get(TableId) then
exit;
OnDatabaseModify := true;
OnDatabaseDelete := true;
end;
[EventSubscriber(ObjectType::Codeunit, Codeunit::GlobalTriggerManagement, OnAfterOnDatabaseModify, '', false, false)]
local procedure LogModify(RecRef: RecordRef)
var
TrackedTable: Record "Tracked Table Good";
TrackedChange: Record "Tracked Change Good";
begin
// The event also fires for tables other features opted in.
if RecRef.IsTemporary() then
exit;
if not TrackedTable.Get(RecRef.Number()) then
exit;
TrackedChange.Init();
TrackedChange."Table No." := RecRef.Number();
TrackedChange."Record ID" := RecRef.RecordId();
TrackedChange.Insert();
end;
}
table 50112 "Tracked Table Good"
{
DataClassification = SystemMetadata;
fields
{
field(1; "Table No."; Integer) { }
}
keys
{
key(PK; "Table No.") { Clustered = true; }
}
}
table 50113 "Tracked Change Good"
{
DataClassification = SystemMetadata;
fields
{
field(1; "Entry No."; Integer) { AutoIncrement = true; }
field(2; "Table No."; Integer) { }
field(3; "Record ID"; RecordId) { }
}
keys
{
key(PK; "Entry No.") { Clustered = true; }
}
}

46
microsoft/knowledge/events/database-trigger-setup-flags-may-only-be-set-to-true.md Normal file
View file

@ -0,0 +1,46 @@
---
bc-version: [15..]
domain: events
keywords: [getdatabasetabletriggersetup, onaftergetdatabasetabletriggersetup, globaltriggermanagement, global-triggers, ondatabaseinsert, ondatabasemodify, ondatabasedelete, ondatabaserename, change-log, var-parameter]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Database trigger setup flags may only be set to true
## Description
The `OnDatabaseInsert`, `OnDatabaseModify`, `OnDatabaseDelete`, and `OnDatabaseRename` events let one subscriber react to writes on any table, receiving the record as a `RecordRef`. They are opt-in per table through `GetDatabaseTableTriggerSetup(TableId; var OnDatabaseInsert; var OnDatabaseModify; var OnDatabaseDelete; var OnDatabaseRename)`, raised by the system codeunit `Global Triggers` (in the 2000000001..2000000010 range). Codeunit 49 `GlobalTriggerManagement` subscribes to it, collects the Dataverse integration and API webhook flags, raises its own integration event `OnAfterGetDatabaseTableTriggerSetup` with the same four `var` Booleans, and then, in the normal execution context only, adds the change log flags.
That the platform raises the database events for a table only when the matching flag ends up `true` is not stated on Microsoft Learn; it is inferred from the sources below. Learn states that subscribing to global triggers disables bulk SQL insert, modify, and delete for that table, so the answer is per table. A BCApps change log test notes that global trigger management may have cached the setup for a table. The `No Transactions Subscriber` test library sets all four flags for every table so that it sees every write.
The four Booleans are shared by every subscriber in the chain, and subscribers run in no particular order. A subscriber that assigns `false`, or assigns an expression that can be `false` such as `OnDatabaseModify := MySetup.Get(TableId)`, overwrites whatever another feature already set for that table if it runs after that feature. The table may then stop raising the database events, and features that rely on them (the change log, Dataverse synchronization, API webhook notifications, data archiving) stop working for it with no error. `GlobalTriggerManagement` asks the change log last, in the normal execution context only, and its comment says it does not want anyone to disable change log management. That ordering protects only the change log flags, and only against `OnAfterGetDatabaseTableTriggerSetup` subscribers. It does not protect the other features' flags, and it does not protect anything against another direct subscriber to `Global Triggers`.
## Best Practice
Prefer table-specific mechanisms when the set of tables is known. Learn advises avoiding global trigger subscribers in general because every opted-in table loses bulk SQL operations. Use the database events only when the set of tables is open-ended or configurable, and turn on only the operations and tables the feature needs.
Subscribe to `GlobalTriggerManagement`'s integration events, `OnAfterGetDatabaseTableTriggerSetup` to opt in and `OnAfterOnDatabaseInsert`, `OnAfterOnDatabaseModify`, `OnAfterOnDatabaseDelete`, or `OnAfterOnDatabaseRename` to react. Learn does not recommend subscribing directly to the events of system codeunits 2000000001..2000000010. Some Microsoft apps do, for example `Data Archive Db Subscriber`, `GP Collect All Modifications`, and the `No Transactions Subscriber` test library, and those subscriptions still compile and run.
In the setup subscriber, only turn flags on: `if IsTracked(TableId) then OnDatabaseModify := true;`, or `OnDatabaseModify := OnDatabaseModify or IsTracked(TableId);` as `Change Log Management` does. A statement such as `if not OnDatabaseDelete then OnDatabaseDelete := false;`, which appears in BCApps, cannot clear a flag and is not this anti-pattern. In the handler, check `RecRef.Number` against the feature's own setup and skip temporary records, because the events also fire for every table another feature opted in.
See sample: [`database-trigger-setup-flags-may-only-be-set-to-true.good.al`](database-trigger-setup-flags-may-only-be-set-to-true.good.al).
## Anti Pattern
In a subscriber to `GetDatabaseTableTriggerSetup` (`Global Triggers`) or `OnAfterGetDatabaseTableTriggerSetup` (`GlobalTriggerManagement`), any assignment to one of the four `var` flags that can store `false` when the flag was already `true`. This includes a literal `false`, an assignment from a lookup or Boolean expression without `or` on the flag's current value, and `Clear` on the parameter.
BCApps has this shape in two places, which shows the mechanism rather than an exception to it. The demo data generator assigns `OnDatabaseInsert := IsTableIDIncludedIntoFullPack(TableId)` while it collects table IDs. The test library `Backup Management` assigns all four flags from its own lookup. Both clear flags set by other features, and both run only in demo-data generation or test sessions where that is accepted. Production and extension code has no such session boundary.
See sample: [`database-trigger-setup-flags-may-only-be-set-to-true.bad.al`](database-trigger-setup-flags-may-only-be-set-to-true.bad.al).
## References
- [Transitioning from codeunit 1 to system codeunits](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/upgrade/transition-from-codeunit1): v14-era upgrade guidance. `GetDatabaseTableTriggerSetup` and `OnDatabase*` moved to codeunit 49 `GlobalTriggerManagement`, and Learn advises against subscribing directly to system codeunits 2000000001..2000000010.
- [How to refactor use of integration records to system fields](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-integration-record-refactoring): subscribers to codeunit 49 hurt performance, and subscribing to global triggers disables bulk SQL insert, modify, and delete for that table.
- [Event types, global events](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-event-types#global-events): the codeunit 49 integration events. [Subscribing to events](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-subscribing-to-events): subscribers run one at a time in no particular order.
- [GlobalTriggerManagement.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/GlobalTriggerManagement.Codeunit.al): the `Global Triggers` setup subscriber and its signature (lines 51-52), the change log ordering and comment in the normal execution context (62-64), `OnAfterGetDatabaseTableTriggerSetup` (173-174), and `OnAfterOnDatabase*` (178-194).
- Inference sources: [ChangeLog.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/Tests/Misc/ChangeLog.Codeunit.al) line 2087 (setup cached per table) and [NoTransactionsSubscriber.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/LibraryNoTransactions/app/NoTransactionsSubscriber.Codeunit.al) lines 4-11 (all four flags on).
- Only-true assignments in BCApps: `ChangeLogManagement.Codeunit.al` lines 85-88 (`or`), `APIWebhookNotificationMgt.Codeunit.al` 224-227, `CRMIntegrationManagement.Codeunit.al` 3748-3753, `MasterDataManagement.Codeunit.al` 1511-1516, `DataArchiveDbSubscriber.Codeunit.al` 27-32, and `GPCollectAllModifications.codeunit.al` 11-21.
- Flag-clearing assignments in demo and test code: [CreateDemonstrationData.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/DemoTool/CreateDemonstrationData.Codeunit.al) lines 343-344 (also the CZ layer line 353 and the IN layer line 357) and [BackupManagement.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/Tests/TestLibraries/BackupManagement.Codeunit.al) lines 470-476.

21
microsoft/knowledge/performance/use-grouped-query-for-distinct-values-and-duplicates.bad.al Normal file
View file

@ -0,0 +1,21 @@
codeunit 50572 "Customer Name Audit"
{
// The outer loop is unfiltered: one extra filtered Count() per customer.
// The question is about the whole table, not about any single row.
procedure HasDuplicateCustomerNames(): Boolean
var
Customer: Record Customer;
OtherCustomer: Record Customer;
begin
Customer.SetLoadFields(Name);
if Customer.FindSet() then
repeat
if Customer.Name <> '' then begin
OtherCustomer.SetRange(Name, Customer.Name);
if OtherCustomer.Count() > 1 then
exit(true);
end;
until Customer.Next() = 0;
exit(false);
end;
}

47
microsoft/knowledge/performance/use-grouped-query-for-distinct-values-and-duplicates.good.al Normal file
View file

@ -0,0 +1,47 @@
// One row per customer name that occurs more than once.
// Name is a plain column, so it is the grouping key; the ColumnFilter on the
// Count column is applied after grouping (HAVING COUNT(*) > 1).
query 50570 "Duplicate Customer Names"
{
QueryType = Normal;
elements
{
dataitem(Customer; Customer)
{
column(Name; Name)
{
ColumnFilter = Name = filter(<> '');
}
column(NameCount)
{
Method = Count;
ColumnFilter = NameCount = filter(> 1);
}
}
}
}
codeunit 50571 "Customer Name Review"
{
procedure HasDuplicateCustomerNames(): Boolean
var
DuplicateCustomerNames: Query "Duplicate Customer Names";
Found: Boolean;
begin
DuplicateCustomerNames.Open();
Found := DuplicateCustomerNames.Read();
DuplicateCustomerNames.Close();
exit(Found);
end;
procedure GetDuplicateCustomerNames(var DuplicateNames: List of [Text])
var
DuplicateCustomerNames: Query "Duplicate Customer Names";
begin
DuplicateCustomerNames.Open();
while DuplicateCustomerNames.Read() do
DuplicateNames.Add(DuplicateCustomerNames.Name);
DuplicateCustomerNames.Close();
end;
}

39
microsoft/knowledge/performance/use-grouped-query-for-distinct-values-and-duplicates.md Normal file
View file

@ -0,0 +1,39 @@
---
bc-version: [all]
domain: performance
keywords: [duplicate, distinct, select-distinct, count, having, group-by, columnfilter, method-count, query]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Use a grouped query for distinct values and duplicate detection
## Description
The AL `Record` type has no `SELECT DISTINCT` or `GROUP BY ... HAVING`. Code that must find which values of a field occur more than once in a table is therefore often written as a loop over the table that, for every row, filters a second record variable on that row's value and calls `Count()`. That sends one extra SQL statement per looped row, so an unfiltered loop costs as many statements as the table has rows, to answer a question about the whole table. A query object answers it in one statement: when any column has an aggregate `Method`, every other `column` becomes an implicit grouping key, so the dataset has one row per distinct combination. A `ColumnFilter` on a non-aggregated column is applied like a `WHERE` clause; a `ColumnFilter` on an aggregated column is applied like a `HAVING` clause, after grouping. A `Method = Count` column with `ColumnFilter = <column> = filter(> 1)` therefore returns only the duplicate groups. This complements [aggregate-before-persisting-intermediate-results](aggregate-before-persisting-intermediate-results.md), which covers grouped totals.
## Best Practice
Declare one plain `column` per field of the combination to check, plus a `column` with `Method = Count` and no source field. For a distinct list, read the rows and ignore the count. For duplicates, set `ColumnFilter` on the count column to `filter(> 1)`; one successful `Read()` proves a duplicate exists. Restrict rows with `SetRange`/`SetFilter` on plain columns, or with a `filter` element, which restricts rows but is not included in the dataset. Every extra `column` changes the grouping grain. A runtime `SetFilter` or `SetRange` on the count column replaces its `ColumnFilter` ([setfilter-overwrites-query-columnfilter](../query/setfilter-overwrites-query-columnfilter.md)). Base Application uses this shape to reject duplicate descriptions, for example query 762 "Acc. Sched. Line Desc. Count".
See sample: [`use-grouped-query-for-distinct-values-and-duplicates.good.al`](use-grouped-query-for-distinct-values-and-duplicates.good.al).
## Anti Pattern
A loop over a table (`FindSet` ... `Next`) that, for each row, sets a filter on a second record variable of the same table, over the same rows, to the current row's value and calls `Count()`, `IsEmpty()`, or `FindFirst()` only to learn whether the value occurs more than once. Do not flag:
- a single uniqueness check for one record, for example in `OnValidate` or before `Insert`;
- an outer loop bounded to one parent, such as the lines of one document, where the statement count does not grow with the table;
- a lookup against a different subset than the one being looped, for example looping quote lines and looking up contract lines;
- a loop that acts on each row it finds, such as marking or updating it, rather than only answering a table-level question;
- a loop where the record being filtered and counted is temporary, which makes no database calls.
See sample: [`use-grouped-query-for-distinct-values-and-duplicates.bad.al`](use-grouped-query-for-distinct-values-and-duplicates.bad.al).
## References
- [Aggregating data in query objects](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-query-totals-grouping): an aggregate `Method` groups the dataset by the other columns; a `Count` column takes only a name; "Using a query to get distinct values".
- [Filtering in query objects](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-query-filters): `ColumnFilter` can be applied to aggregated columns; filters on columns with a totals method correspond to a `HAVING` clause, others to `WHERE`; a filter row is not included in the dataset.
- Base Application: [AccSchedLineDescCount.Query.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Finance/FinancialReports/AccSchedLineDescCount.Query.al#L15-L37) (query 762), used by `CheckDuplicateAccScheduleLineDescription` in [AccSchedChartManagement.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Finance/FinancialReports/AccSchedChartManagement.Codeunit.al#L390-L398). The same shape appears in [ColmLaytColmHeaderCount.Query.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Finance/FinancialReports/ColmLaytColmHeaderCount.Query.al) and [Inventory/Analysis/AnalysisLineDescCount.Query.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Inventory/Analysis/AnalysisLineDescCount.Query.al).
- A legitimate per-row lookup that this rule must not flag: [ServiceContractHeader.Table.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Service/Contract/ServiceContractHeader.Table.al#L2692-L2705) loops one quote's lines, looks up contract lines for each service item, and marks each hit.

33
microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al Normal file
View file

@ -0,0 +1,33 @@
table 50567 "Contoso Activity Log"
{
DataClassification = SystemMetadata;
fields
{
field(1; "Entry No."; Integer) { AutoIncrement = true; }
field(2; "Activity"; Text[250]) { }
}
keys
{
key(PK; "Entry No.") { Clustered = true; }
}
}
codeunit 50563 "Contoso Activity Log Cleanup"
{
Access = Internal;
// The log table is never added to the allowed tables, so it cannot appear
// on the Retention Policies page. Cleanup is hard-coded here instead:
// the period is not configurable, the deletion is not written to the
// Retention Policy Log, and an administrator cannot switch it off.
trigger OnRun()
var
ContosoActivityLog: Record "Contoso Activity Log";
begin
ContosoActivityLog.SetFilter(
SystemCreatedAt, '<%1', CreateDateTime(CalcDate('<-30D>', Today()), 0T));
ContosoActivityLog.DeleteAll();
end;
}

87
microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al Normal file
View file

@ -0,0 +1,87 @@
table 50566 "Contoso Activity Log"
{
DataClassification = SystemMetadata;
fields
{
field(1; "Entry No."; Integer) { AutoIncrement = true; }
field(2; "Activity"; Text[250]) { }
}
keys
{
key(PK; "Entry No.") { Clustered = true; }
}
}
codeunit 50560 "Contoso Reten. Pol. Setup"
{
Access = Internal;
procedure AddAllowedTables()
begin
AddAllowedTables(false);
end;
// ForceUpdate re-registers even after the upgrade tag is set, so the
// table comes back when an administrator refreshes the allowed tables.
procedure AddAllowedTables(ForceUpdate: Boolean)
var
ContosoActivityLog: Record "Contoso Activity Log";
RetenPolAllowedTables: Codeunit "Reten. Pol. Allowed Tables";
UpgradeTag: Codeunit "Upgrade Tag";
IsInitialSetup: Boolean;
begin
IsInitialSetup := not UpgradeTag.HasUpgradeTag(AllowedTableTag());
if not (IsInitialSetup or ForceUpdate) then
exit;
if not RetenPolAllowedTables.IsAllowedTable(Database::"Contoso Activity Log") then
RetenPolAllowedTables.AddAllowedTable(
Database::"Contoso Activity Log",
ContosoActivityLog.FieldNo(SystemCreatedAt),
28); // support cases need at least four weeks of log history
if IsInitialSetup then
UpgradeTag.SetUpgradeTag(AllowedTableTag());
end;
local procedure AllowedTableTag(): Code[250]
begin
exit('Contoso-ActivityLogAllowedTable-20260910');
end;
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Reten. Pol. Allowed Tables", OnRefreshAllowedTables, '', false, false)]
local procedure AddAllowedTablesOnRefreshAllowedTables()
begin
AddAllowedTables(true);
end;
}
codeunit 50561 "Contoso Reten. Pol. Install"
{
Subtype = Install;
Access = Internal;
trigger OnInstallAppPerCompany()
var
ContosoRetenPolSetup: Codeunit "Contoso Reten. Pol. Setup";
begin
ContosoRetenPolSetup.AddAllowedTables();
end;
}
codeunit 50562 "Contoso Reten. Pol. Upgrade"
{
Subtype = Upgrade;
Access = Internal;
// Install code does not run on upgrade, so tenants that already have the
// app get their registration here.
trigger OnUpgradePerCompany()
var
ContosoRetenPolSetup: Codeunit "Contoso Reten. Pol. Setup";
begin
ContosoRetenPolSetup.AddAllowedTables();
end;
}

33
microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md Normal file
View file

@ -0,0 +1,33 @@
---
bc-version: [22..]
domain: privacy
keywords: [retention-policy, allowed-tables, addallowedtable, reten-pol-allowed-tables, onrefreshallowedtables, append-only-table, log-table-growth, deleteall, mandatory-minimum-retention, install-upgrade-codeunit]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Extension-owned log tables must be registered as retention-policy allowed tables
## Description
The retention policy engine only ever deletes from tables that appear in its allowed-tables list, and an extension may register only tables it owns — it cannot add a base application table or a table from another extension. Registration is a call to `Codeunit "Reten. Pol. Allowed Tables".AddAllowedTable`, passing the table ID and the field number of the Date or DateTime field that ages each record (`SystemCreatedAt` is the usual choice). Until that call has run in a company, the table cannot be selected on the **Retention Policies** page at all, so an activity log, integration log, or archive table the extension writes to grows with no supported way for an administrator to trim it. The registration is stored per company and is not part of the table's metadata — it exists only because install or upgrade code put it there.
## Best Practice
Register every table the extension owns that accumulates rows over time: activity and audit logs, integration and API request logs, archived documents. Call a shared routine from both the install codeunit (`OnInstallAppPerCompany`) and an upgrade codeunit (`OnUpgradePerCompany`), because install code does not run when an existing installation moves to a new version — registration added only to install code never reaches tenants that already have the app. Guard the routine with `IsAllowedTable` and an upgrade tag so repeated runs are idempotent, but keep a force path past the tag: the **Retention Policies** pages raise `Reten. Pol. Allowed Tables.OnRefreshAllowedTables`, and the platform's own installers (System Application, Base Application, Shopify) subscribe to it and re-run registration with `ForceUpdate`. A routine that exits whenever the tag is set cannot take part in that refresh, so the upgrade tag should gate one-time setup only, not re-registration. Pass `MandatoryMinRetenDays` when the data must survive a minimum period for audit or support reasons; the platform then rejects any shorter period an administrator configures. When only a subset of rows should ever expire, build the filter with `AddTableFilterToJsonArray` and pass it to the `AddAllowedTable` overload that takes a `JsonArray` — a filter added as locked cannot be removed later by the administrator.
Registering a table only makes it eligible; the policy itself is a separate concern, covered by [`ship-a-default-retention-policy-setup.md`](ship-a-default-retention-policy-setup.md).
See sample: [`register-owned-log-tables-for-retention-policies.good.al`](register-owned-log-tables-for-retention-policies.good.al).
## Anti Pattern
An extension-owned table that only grows — the app inserts into it but never deletes from it — with no `AddAllowedTable` call anywhere in the app. The name is not a reliable signal: an "Incoming Data" buffer-history table grows the same way an "Activity Log" does, and such tables have reached hundreds of gigabytes on customer tenants. A variant is a table cleaned by hand-rolled code — a job queue codeunit or scheduled task running `DeleteAll` against a hard-coded date window: the deletion happens outside the **Retention Policy Log**, the administrator has no page on which to lengthen, shorten, or disable it, and the table is invisible during a data-retention review. The same defect in slower form is registration performed only in the install codeunit: new tenants are covered, every existing tenant stays unregistered after the upgrade.
See sample: [`register-owned-log-tables-for-retention-policies.bad.al`](register-owned-log-tables-for-retention-policies.bad.al).
## References
- [Clean up data with retention policies](https://learn.microsoft.com/en-us/dynamics365/business-central/admin-data-retention-policies), section *Include your extension in a retention policy*.
- [`RetenPolAllowedTables.Codeunit.al`](https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/Retention%20Policy/src/Retention%20Policy%20Allowed%20Tables/RetenPolAllowedTables.Codeunit.al) in microsoft/BCApps — the `AddAllowedTable` overloads, `IsAllowedTable`, `AddTableFilterToJsonArray`, and `OnRefreshAllowedTables`.

82
microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al Normal file
View file

@ -0,0 +1,82 @@
table 50569 "Contoso Activity Log"
{
DataClassification = SystemMetadata;
fields
{
field(1; "Entry No."; Integer) { AutoIncrement = true; }
field(2; "Activity"; Text[250]) { }
}
keys
{
key(PK; "Entry No.") { Clustered = true; }
}
}
page 50570 "Contoso Activity Log"
{
PageType = List;
ApplicationArea = All;
UsageCategory = Lists;
SourceTable = "Contoso Activity Log";
Editable = false;
// Registration only makes the table selectable on the Retention Policies
// page. No Retention Policy Setup exists and nothing is deleted, yet the
// page tells the administrator that cleanup is running.
AboutTitle = 'About the activity log';
AboutText = 'Entries older than six months are deleted automatically, so the log never needs manual cleanup.';
layout
{
area(Content)
{
repeater(Entries)
{
field("Entry No."; Rec."Entry No.") { ToolTip = 'Specifies the entry number.'; }
field(Activity; Rec.Activity) { ToolTip = 'Specifies the logged activity.'; }
}
}
}
}
codeunit 50565 "Contoso Reten. Pol. Register"
{
Access = Internal;
procedure AddAllowedTables()
var
ContosoActivityLog: Record "Contoso Activity Log";
RetenPolAllowedTables: Codeunit "Reten. Pol. Allowed Tables";
begin
if not RetenPolAllowedTables.IsAllowedTable(Database::"Contoso Activity Log") then
RetenPolAllowedTables.AddAllowedTable(
Database::"Contoso Activity Log", ContosoActivityLog.FieldNo(SystemCreatedAt));
end;
}
codeunit 50571 "Contoso Reten. Pol. Install"
{
Subtype = Install;
Access = Internal;
trigger OnInstallAppPerCompany()
var
ContosoRetenPolRegister: Codeunit "Contoso Reten. Pol. Register";
begin
ContosoRetenPolRegister.AddAllowedTables();
end;
}
codeunit 50572 "Contoso Reten. Pol. Upgrade"
{
Subtype = Upgrade;
Access = Internal;
trigger OnUpgradePerCompany()
var
ContosoRetenPolRegister: Codeunit "Contoso Reten. Pol. Register";
begin
ContosoRetenPolRegister.AddAllowedTables();
end;
}

55
microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al Normal file
View file

@ -0,0 +1,55 @@
table 50568 "Contoso Activity Log"
{
DataClassification = SystemMetadata;
fields
{
field(1; "Entry No."; Integer) { AutoIncrement = true; }
field(2; "Activity"; Text[250]) { }
}
keys
{
key(PK; "Entry No.") { Clustered = true; }
}
}
codeunit 50564 "Contoso Reten. Pol. Default"
{
Access = Internal;
Permissions = tabledata "Retention Policy Setup" = ri;
procedure CreateDefaultPolicy()
var
RetentionPolicySetup: Record "Retention Policy Setup";
RetentionPolicySetupMgt: Codeunit "Retention Policy Setup";
RetenPolAllowedTables: Codeunit "Reten. Pol. Allowed Tables";
UpgradeTag: Codeunit "Upgrade Tag";
begin
// A setup can only be created for a table that is already registered.
if not RetenPolAllowedTables.IsAllowedTable(Database::"Contoso Activity Log") then
exit;
// Created once per company: an administrator who deletes the policy
// does not get it back on the next upgrade.
if UpgradeTag.HasUpgradeTag(DefaultPolicyTag()) then
exit;
if not RetentionPolicySetup.Get(Database::"Contoso Activity Log") then begin
RetentionPolicySetup.Validate("Table Id", Database::"Contoso Activity Log");
RetentionPolicySetup.Validate("Apply to all records", true);
RetentionPolicySetup.Validate(
"Retention Period",
RetentionPolicySetupMgt.FindOrCreateRetentionPeriod("Retention Period Enum"::"6 Months"));
RetentionPolicySetup.Validate(Enabled, false); // the administrator opts in to deletion
RetentionPolicySetup.Insert(true);
end;
UpgradeTag.SetUpgradeTag(DefaultPolicyTag());
end;
local procedure DefaultPolicyTag(): Code[250]
begin
exit('Contoso-ActivityLogDefaultPolicy-20260910');
end;
}

31
microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md Normal file
View file

@ -0,0 +1,31 @@
---
bc-version: [22..]
domain: privacy
keywords: [retention-policy, retention-policy-setup, addallowedtable, findorcreateretentionperiod, retention-period, default-policy, unbounded-table-growth, opt-in-deletion, upgrade-tag]
technologies: [al]
countries: [w1]
application-area: [all]
---
# Registering a table does not delete anything — consider shipping a default setup
## Description
`AddAllowedTable` only makes a table selectable on the **Retention Policies** page. Nothing is deleted until a `Retention Policy Setup` record exists for that table, names a `Retention Period`, and is enabled. Registration alone is a valid pattern — several Microsoft apps register tables and leave the policy entirely to the administrator — but it means the table keeps growing until someone discovers the page, works out which of the extension's tables are safe to trim, and picks a period. Shipping a default setup is optional; where the extension's author knows a sensible period, it removes that discovery step. Microsoft's `Codeunit 3907 "Retention Policy Installer"` shows the shape — it registers `Retention Policy Log Entry`, then creates a setup record with a six-month period on first install, inserted disabled, guarded by an upgrade tag so a policy the administrator later deleted is not recreated on the next upgrade.
## Best Practice
When you ship a default, do it in the same install and upgrade routine that registers the table (see [`register-owned-log-tables-for-retention-policies.md`](register-owned-log-tables-for-retention-policies.md)), and create the `Retention Policy Setup` record: get the period code from `Codeunit "Retention Policy Setup".FindOrCreateRetentionPeriod`, which reuses an existing `Retention Period` with the requested enum value and otherwise creates one without colliding on an existing code (a hand-written lookup-then-insert fails when a period with the chosen code already exists for a different value), then `Validate` `"Table Id"`, `"Apply to all records"` and `"Retention Period"` before inserting. The codeunit that inserts the record needs `tabledata "Retention Policy Setup" = ri`. Gate the creation on an upgrade tag so it happens once per company rather than on every upgrade. Default to inserting with `Enabled` set to false: pre-creating the line puts a reviewed, sensible period in front of the administrator while leaving the decision to delete tenant data with them. Shipping the policy enabled is defensible for rows that are purely diagnostic and documented as transient — state that choice, and the default period, in the app's onboarding material either way.
See sample: [`ship-a-default-retention-policy-setup.good.al`](ship-a-default-retention-policy-setup.good.al).
## Anti Pattern
Registering a table and then claiming, in a message, notification, label, teaching tip, or setup text, that its data is now cleaned up automatically. Registration only makes the table selectable; without an enabled `Retention Policy Setup` nothing is deleted, so the administrator is told a cleanup is running when none is, and the table grows unnoticed. Registration without a default setup is not itself a defect.
See sample: [`ship-a-default-retention-policy-setup.bad.al`](ship-a-default-retention-policy-setup.bad.al).
## References
- [Clean up data with retention policies](https://learn.microsoft.com/en-us/dynamics365/business-central/admin-data-retention-policies) — retention periods, enabling a policy, and the job queue entry that applies it.
- [`RetentionPolicyInstaller.Codeunit.al`](https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/Retention%20Policy/src/Install/RetentionPolicyInstaller.Codeunit.al) in microsoft/BCApps — the platform's own register-then-create-disabled-setup pattern.

68
microsoft/knowledge/style/mutating-procedure-for-a-page-caller-takes-var-record.bad.al Normal file
View file

@ -0,0 +1,68 @@
table 50140 "Sample Document Header"
{
fields
{
field(1; "No."; Code[20]) { }
field(2; Status; Option) { OptionMembers = Open,Archived; }
field(3; "Archived By"; Code[50]) { }
}
keys
{
key(PK; "No.") { Clustered = true; }
}
}
codeunit 50140 "Sample Document Mgt."
{
// Takes only the key and mutates its own local copy of the record.
procedure Archive(DocumentNo: Code[20])
var
DocumentHeader: Record "Sample Document Header";
begin
DocumentHeader.Get(DocumentNo);
DocumentHeader.TestField(Status, DocumentHeader.Status::Open);
DocumentHeader.Status := DocumentHeader.Status::Archived;
DocumentHeader."Archived By" := CopyStr(UserId(), 1, MaxStrLen(DocumentHeader."Archived By"));
DocumentHeader.Modify(true);
end;
}
page 50140 "Sample Document Card"
{
PageType = Card;
SourceTable = "Sample Document Header";
layout
{
area(Content)
{
group(General)
{
field("No."; Rec."No.") { }
field(Status; Rec.Status) { }
field("Archived By"; Rec."Archived By") { }
}
}
}
actions
{
area(Processing)
{
action(Archive)
{
trigger OnAction()
var
DocumentMgt: Codeunit "Sample Document Mgt.";
begin
DocumentMgt.Archive(Rec."No.");
// Rec still holds the pre-call values: "Archived By" is read from the stale buffer.
Message(ArchivedMsg, Rec."No.", Rec."Archived By");
end;
}
}
}
var
ArchivedMsg: Label 'Document %1 was archived by %2.', Comment = '%1 = document number, %2 = user who archived the document';
}

64
microsoft/knowledge/style/mutating-procedure-for-a-page-caller-takes-var-record.good.al Normal file
View file

@ -0,0 +1,64 @@
table 50140 "Sample Document Header"
{
fields
{
field(1; "No."; Code[20]) { }
field(2; Status; Option) { OptionMembers = Open,Archived; }
field(3; "Archived By"; Code[50]) { }
}
keys
{
key(PK; "No.") { Clustered = true; }
}
}
codeunit 50140 "Sample Document Mgt."
{
// Takes the caller's record by reference, so Modify updates the caller's buffer.
procedure Archive(var DocumentHeader: Record "Sample Document Header")
begin
DocumentHeader.TestField(Status, DocumentHeader.Status::Open);
DocumentHeader.Status := DocumentHeader.Status::Archived;
DocumentHeader."Archived By" := CopyStr(UserId(), 1, MaxStrLen(DocumentHeader."Archived By"));
DocumentHeader.Modify(true);
end;
}
page 50140 "Sample Document Card"
{
PageType = Card;
SourceTable = "Sample Document Header";
layout
{
area(Content)
{
group(General)
{
field("No."; Rec."No.") { }
field(Status; Rec.Status) { }
field("Archived By"; Rec."Archived By") { }
}
}
}
actions
{
area(Processing)
{
action(Archive)
{
trigger OnAction()
var
DocumentMgt: Codeunit "Sample Document Mgt.";
begin
DocumentMgt.Archive(Rec);
Message(ArchivedMsg, Rec."No.", Rec."Archived By");
end;
}
}
}
var
ArchivedMsg: Label 'Document %1 was archived by %2.', Comment = '%1 = document number, %2 = user who archived the document';
}

57
microsoft/knowledge/style/mutating-procedure-for-a-page-caller-takes-var-record.md Normal file
View file

@ -0,0 +1,57 @@
---
bc-version: [all]
domain: style
keywords: [var-record, by-reference, pass-by-value, record-parameter, page-action, re-fetch, stale-buffer, codeunit, modify]
technologies: [al]
countries: [w1]
application-area: [all]
---
# A new procedure that changes the page's current record should take it as `var Record`
## Description
AL passes parameters by value unless they are declared `var`, and a by-value `Record` parameter is a copy: changes the procedure makes "affect only the copy, not the variable itself" ([Working with AL methods](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-al-methods#parameters)). Suppose a page/pageextension trigger hands a codeunit only a key (`Rec."No."`) or a by-value `Rec`, and the procedure `Get`s or modifies its own variable. The trigger's `Rec` then keeps the field values it had before the call. Any code later in the same trigger that reads `Rec` or writes from it works on stale data. Examples are a message, a `TestField`, or a follow-up assignment and `Modify`.
The page display is not the problem. After an action runs, the page refreshes its current record by itself, because `OnAfterGetRecord` runs (or `OnFindRecord` when the record left the filter) ([Actions at runtime](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-actions-overview#actions-at-runtime)). The harm is confined to code that uses `Rec` after the call in the same trigger, or to a non-page caller that keeps holding the record.
## Best Practice
When you design a new procedure whose job is to change the record a page/pageextension trigger already holds, declare that record as `var Record` and mutate it in place. The procedure's `Modify` then updates the trigger's `Rec`. The Learn page above shows the same shape: `CurrPage.SetSelectionFilter(Rec); codeunit.Run(50000, Rec);`, through `Codeunit.Run(Number, var Record)`. Base Application's Sales Order Reopen action calls `ReleaseSalesDoc.PerformManualReopen(Rec)`, and `Reopen(var SalesHeader)` sets `Status` and calls `Modify(true)` on that parameter. A procedure that needs a locked read can call `LockTable`/`ReadIsolation` and `Find` on the `var` parameter. That refreshes the caller's buffer too.
A `var Record` parameter has a cost: the callee receives the page's live record, including its filters and current key. It must not change filters or keys on the parameter. If it needs its own filtering, it should copy the record or call `SetRecFilter` on a copy first.
Treat this as design guidance (`minor`). Do not change an already-shipped public procedure from by-value to `var` in place. Adding or removing `var` there is a breaking change (AppSourceCop [AS0078](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/analyzers/appsourcecop-as0078); see `breaking-changes/do-not-change-published-procedure-signatures`). Add a new procedure instead. A `var Record` overload can sit beside a key-typed one, but an overload that differs only by `var` is rejected with [AL0440](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al440) (verified with AL compiler 30.0).
See sample: [`mutating-procedure-for-a-page-caller-takes-var-record.good.al`](mutating-procedure-for-a-page-caller-takes-var-record.good.al).
## Anti Pattern
All four of these must hold:
- A new codeunit procedure takes a key (`Code`, `Integer`, `Guid`) or a non-`var` `Record`.
- It `Get`s or uses its own copy and calls `Modify` on that same table's record.
- A page/pageextension trigger calls it with `Rec` or a key field of `Rec`.
- Later in the same trigger, code reads `Rec` fields the procedure changed, or writes from `Rec`.
A `Rec.Get` right after the call is context only, not evidence. Re-reading is a common, benign Base Application idiom, used even after `var` calls.
Do not flag these legitimate key-based or by-value shapes:
- Background and scheduled entry points. Job queue codeunits resolve `"Record ID to Process"`; `TaskScheduler.CreateTask` takes a `RecordId`; page background tasks pass text parameters.
- API page actions over a buffer or entity table that locate the real record by `SystemId`.
- Generic, table-agnostic APIs that take `RecordId`, `RecordRef`, or `Variant`, such as `Approvals Mgmt.ApproveRecordApprovalRequest(RecordId)`.
- Key-based procedures whose change happens inside a platform or System API.
- Procedures that change a different table or only read, and temporary records.
- A by-value record modified inside a `FindSet` loop, which `performance/avoid-cloning-records-before-modify-delete-in-loops` covers.
- In-place changes to published procedures, which the breaking-changes article above covers.
See also `style/pages-must-not-contain-business-logic` for where the mutation itself belongs.
See sample: [`mutating-procedure-for-a-page-caller-takes-var-record.bad.al`](mutating-procedure-for-a-page-caller-takes-var-record.bad.al).
## References
- [Working with AL methods, Parameters](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-al-methods#parameters): by value is the default, and "a *copy of the variable* is passed to the method".
- [Actions overview, Actions at runtime](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-actions-overview#actions-at-runtime): the page refresh after an action, and the `SetSelectionFilter` + `codeunit.Run(50000, Rec)` example. [Codeunit.Run](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/codeunit/codeunit-run-method): `Codeunit.Run(Number: Integer [, var Record: Record])`.
- BCApps at [`837ef80`](https://github.com/microsoft/BCApps/tree/837ef802485ee457e52310d2ecaa08b93d0122fd), Base Application examples:
- `var` idiom: [`SalesOrder.Page.al#L1610`](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Document/SalesOrder.Page.al#L1610) and [`ReleaseSalesDocument.Codeunit.al#L223-L240`](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Document/ReleaseSalesDocument.Codeunit.al#L223-L240).
- Carve-outs: job queue [`SalesPostviaJobQueue.Codeunit.al#L20-L33`](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Posting/SalesPostviaJobQueue.Codeunit.al#L20-L33); `RecordId` API [`ApprovalsMgmt.Codeunit.al#L251`](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/OtherCapabilities/Approvals/ApprovalsMgmt.Codeunit.al#L251); `SystemId` API action [`APIV2SalesOrders.Page.al#L797-L801`](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Apps/W1/APIV2/app/src/pages/APIV2SalesOrders.Page.al#L797-L801); System API [`SOAReplyRetryMgt.Codeunit.al#L27-L42`](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Apps/W1/SalesOrderAgent/app/src/Integration/SOAReplyRetryMgt.Codeunit.al#L27-L42).

8
microsoft/skills/review/al-data-modeling-review.md
View file

@ -16,7 +16,7 @@ application-area: [all]
Reviews AL source changes against the `data-modeling` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, audit fields, document print/email/Post-and-Send actions, `Navigate` page subscribers, Report Selection registration or dispatch, price-calculation/price-source extensibility, code that reads or writes sales/purchase/service line price and amount fields, `TransferFields`-based posting-cascade field mirroring, `TableRelation` field-length design, barcode/report-layout font-provider usage, dimension wiring, journal-based posting-routine structure, or Item Ledger Entry document-number lookups after a combined sales post. The skill returns `not-applicable` when none of those apply.
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, audit fields, document print/email/Post-and-Send actions, `Navigate` page subscribers, Report Selection registration or dispatch, price-calculation/price-source extensibility, code that reads or writes sales/purchase/service line price and amount fields, `TransferFields`-based posting-cascade field mirroring, `TableRelation` field-length design, barcode/report-layout font-provider usage, dimension wiring, journal-based posting-routine structure, Item Ledger Entry document-number lookups after a combined sales post, or code that inserts or deletes master/reference records. The skill returns `not-applicable` when none of those apply.
## Source
@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review
- The changed AL object names and types — especially `* Setup` singleton tables and Card pages, custom master tables, tableextensions that add master-data fields, document or journal lines that reference a master, document pages/codeunits exposing print/email/Post-and-Send actions, codeunits subscribing to `Navigate`, enumextensions to `"Report Selection Usage"`/`"Price Calculation Handler"`/`"Price Source Type"`, and report objects that render barcodes.
- The changed fields, keys, triggers, and procedures, weighted toward `Primary Key`, `No.`, `No. Series`, `Blocked`, `Last Date Modified`, `OnInsert`, `OnModify`, `OnRename`, reference-field `OnValidate`, posting validation, and posting-cascade `TransferFields` calls.
- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `InitRecord`, `Round`, `Precision`, `Direction`, `TableRelation`, `ValidateTableRelation`, `TestTableRelation`, `Text[`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`, `TransferFields`, `Navigate`, `OnAfterFindRecords`, `OnBeforeShowRecords`, `Report Selections`, `Report Selection Usage`, `InsertRecord`, `Document Sending Profile`, `PrintForCust`, `PrintWithDialogForCust`, `PrintWithDialogForVend`, `SendEmailToCust`, `SendEmailToVendor`, `Report.RunModal`, `Report.Run`, `Price Calculation Handler`, `Price Calculation`, `OnFindSupportedSetup`, `Price Calculation Setup`, `Price Source Type`, `PriceSourceList`, `OnAfterAddSources`, `UpdateUnitPrice`, `PlanPriceCalcByField`, `UpdateUnitPriceByField`, `Prices Including VAT`, `Unit Price`, `Direct Unit Cost`, `Line Amount`, `Prepmt. Line Amount`, `Amount Including VAT`, `CalculateOutstandingAmountExclTax`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`).
- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `InitRecord`, `Round`, `Precision`, `Direction`, `TableRelation`, `ValidateTableRelation`, `TestTableRelation`, `Text[`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`, `TransferFields`, `Navigate`, `OnAfterFindRecords`, `OnBeforeShowRecords`, `Report Selections`, `Report Selection Usage`, `InsertRecord`, `Document Sending Profile`, `PrintForCust`, `PrintWithDialogForCust`, `PrintWithDialogForVend`, `SendEmailToCust`, `SendEmailToVendor`, `Report.RunModal`, `Report.Run`, `Price Calculation Handler`, `Price Calculation`, `OnFindSupportedSetup`, `Price Calculation Setup`, `Price Source Type`, `PriceSourceList`, `OnAfterAddSources`, `UpdateUnitPrice`, `PlanPriceCalcByField`, `UpdateUnitPriceByField`, `Prices Including VAT`, `Unit Price`, `Direct Unit Cost`, `Line Amount`, `Prepmt. Line Amount`, `Amount Including VAT`, `CalculateOutstandingAmountExclTax`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`, `Insert`, `Delete`, `DeleteAll`).
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no data-modeling changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
@ -51,6 +51,8 @@ The following targeted checks cover every current `data-modeling` article. Treat
- A new or extended table's name, fields, or usage positively establish it as one of Business Central's nine business-record types — a name ending `Ledger Entry`/`Register`/`Journal Line`/`Header`/`Line`/`Setup`, an auto-generated `Entry No.`/`No.` key posted from elsewhere, a `Template Name`+`Batch Name`+`Line No.` key, or a singleton `Primary Key` field — `table-design-must-match-bc-table-type-conventions`. Do not worklist it from a bare `keys` block or primary-key declaration alone: a temporary/buffer table, a work queue, a log, a cross-reference/mapping table, or a process-local staging table is not one of the nine types and is out of this rule's scope entirely, not an unresolved case.
- Code reads `Item Ledger Entry."Document No."` (or `"Last Shipping No."`/`"Last Posting No."`) after a combined Ship+Invoice **sales** post — `item-ledger-entry-document-no-follows-last-shipping-no`. This is a sales-specific rule: purchase combined posting is Receive+Invoice and uses receiving fields such as `"Last Receiving No."`, not the shipment/document-number behavior this article describes. Do not worklist it from purchase posting code.
- A custom master table changes its primary key, `No.`/`No. Series` fields, or `OnInsert` without assigning a blank `No.` from setup through a number series — `master-table-no-from-number-series-in-oninsert`.
- Code outside the table creates a non-temporary master record (`Customer`, `Vendor`, `Item`, `G/L Account`, `Contact`, or a custom master with initializing `OnInsert` logic) with `Insert()`/`Insert(false)` and does not itself assign the number and the fields `OnInsert` would set — `master-data-must-be-inserted-with-trigger`. Do not flag temporary records, buffer/staging tables, a caller that visibly assigns the trigger's fields before applying a template, or an XMLport round-tripping rows exported from Business Central; `Modify()` without the trigger is not this rule.
- Code outside the owning table's own `OnDelete` calls `Delete()`/`DeleteAll()` (or passes `false`) on a non-temporary master or reference table with an `OnDelete` guard or cascade (`Currency`, `Customer`, `Vendor`, `Item`, `G/L Account`, or a custom equivalent) — `delete-master-data-with-trigger`. Do not flag an owning table's `OnDelete` deleting its own dependents, temporary/buffer records, deleting and immediately re-inserting the same primary key (restore/recreate) where dependents stay valid, or a data-migration/setup reset in a company with no posted entries that removes master rows and their setup references as one rebuild.
- BC v22 or later code introduces or retains `NoSeriesManagement`, `InitSeries`, `SelectSeries`, or `SetSeries`, or number assignment/manual-entry checks do not use codeunit `"No. Series"` methods such as `GetNextNo`, `IsManual`, or `TestManual` — `use-no-series-codeunit-not-noseriesmanagement`.
- A master gains or changes `Blocked`, or a document line, journal line, reference-field `OnValidate`, or posting routine uses that master without `TestField(Blocked, false)` at the point of use; also cue when the check is placed only in the master's own triggers — `check-blocked-in-referencing-code-not-in-master`.
- A master table adds or changes `Last Date Modified`, `OnModify`, or `OnRename`, but the non-editable field is not assigned `Today()` in both triggers — `set-last-date-modified-in-onmodify-and-onrename`.
@ -102,7 +104,7 @@ Outcome selection:
- `completed` — the skill evaluated every worklist item.
- `no-knowledge` — no applicable data-modeling knowledge survived filtering.
- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, or audit-field surface, and no document print/email/Post-and-Send action, `Navigate` subscriber, Report Selection registration/dispatch, price-calculation/price-source extensibility point, sales/purchase/service line price or amount read/write, posting-cascade `TransferFields` mirroring, `TableRelation` field length, barcode/report-font-provider usage, dimension wiring, posting-routine structure, or Item-Ledger-Entry-document-number surface.
- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, or audit-field surface, and no document print/email/Post-and-Send action, `Navigate` subscriber, Report Selection registration/dispatch, price-calculation/price-source extensibility point, sales/purchase/service line price or amount read/write, posting-cascade `TransferFields` mirroring, `TableRelation` field length, barcode/report-font-provider usage, dimension wiring, posting-routine structure, Item-Ledger-Entry-document-number surface, or master/reference-record insert/delete call.
- `partial` — a budget was hit before the worklist was exhausted.
- `failed` — an unrecoverable error occurred.

3
microsoft/skills/review/al-error-handling-review.md
View file

@ -46,7 +46,7 @@ Narrow the relevant files to the subset that applies to the changes under review
- The changed AL object names and types — especially codeunits that post or validate, tables and table extensions with `OnValidate` triggers, and any procedure that raises errors or orchestrates a batch over records.
- The changed procedures and triggers, weighted toward `OnValidate`/`OnInsert`/`OnModify` triggers, posting and validation routines, and procedures attributed with `[ErrorBehavior(...)]` or `[TryFunction]`.
- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `FieldError`, `TestField`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`, `TryFunction`, `GetLastErrorText`, Boolean assignment).
- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `FieldError`, `TestField`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`, `TryFunction`, `GetLastErrorText`, `Confirm`, `xRec`, Boolean assignment).
- For the outbound HTTP call paths identified in Source, include `HttpClient`, `Get`, `Post`, `HttpResponseMessage`, response use, and caller failure handling (including `[TryFunction]` call sites) in keyword and topic matching.
- Resolve changed standalone call targets; when the target declaration has `[TryFunction]`, worklist the ignored-return rule even if the declaration itself is unchanged. Only assignment and conditional use activate try semantics.
@ -54,6 +54,7 @@ A file enters the candidate worklist when its `keywords` intersect the extracted
The following targeted checks cover every current `error-handling` article:
- A field `OnValidate` (or a procedure it calls) uses `Confirm` to gate a side effect that releases, cancels, or replaces state tied to the old (`xRec`) value, after the change nothing references that old state any more (it is orphaned), and the declined branch neither raises an error nor restores the field while the change proceeds — `declined-confirm-must-abort-not-partially-apply`. Do not flag declined updates whose old state stays valid (such as leaving tasks on a still-existing old campaign), optional follow-ups whose skipping leaves every record consistent (such as declining to update document lines after a header change), or `exit` on a declined `Confirm` in an action before anything is written.
- `[ErrorBehavior(ErrorBehavior::Collect)]`, `ErrorInfo.Collectible`, `HasCollectedErrors`, `GetCollectedErrors`, or `ClearCollectedErrors` is added or changed, especially when errors are collected without later surfacing/clearing them — `collect-validation-errors-with-errorbehavior`.
- New or changed code inserts an error/duration log record around a failed `TryFunction`/`GetLastErrorText`/`GetLastErrorCode` path and then raises, propagates, or rethrows the error — `log-writes-must-survive-rollback`. Do not worklist it when the log insert already happens inside a `Session.StartSession`-targeted codeunit's `OnRun`; that is the compliant shape, not the signal to flag.
- A guarded lookup (`if Record.Get(...) then ... else` or similar) sets a value used later, and the same guard shape (with the same blank/zero fallback style) is applied to a field that feeds a posted amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output — `defensive-vs-offensive-code-must-match-blast-radius`. The signal is a posting-critical or compliance-facing field guarded defensively with a silent fallback, not the mere presence of a guarded lookup.

3
microsoft/skills/review/al-events-review.md
View file

@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review
- The changed AL object names and types — especially codeunits that publish events or host event subscribers, posting/release/validation routines that should expose extension points, and test codeunits that bind subscribers.
- The changed procedures and triggers, weighted toward event publisher methods, methods carrying the `[EventSubscriber(...)]` attribute, routines that raise `OnBefore`/`OnAfter` events, and any procedure that calls `BindSubscription`/`UnbindSubscription`.
- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `InternalEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`, `IncludeSender`, `GlobalVarAccess`, `Isolated`, `local`, `internal`, `Sender`, `this`, `RecordRef`, `xRec`, `temporary`, `Temp`, `repeat`, `ChangeCompany`, `StartSession`, `RunTrigger`).
- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `InternalEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`, `IncludeSender`, `GlobalVarAccess`, `Isolated`, `local`, `internal`, `Sender`, `this`, `RecordRef`, `xRec`, `temporary`, `Temp`, `repeat`, `ChangeCompany`, `StartSession`, `RunTrigger`, `GetDatabaseTableTriggerSetup`, `OnAfterGetDatabaseTableTriggerSetup`, `GlobalTriggerManagement`, `Global Triggers`, `OnDatabaseInsert`, `OnDatabaseModify`, `OnDatabaseDelete`, `OnDatabaseRename`).
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
@ -66,6 +66,7 @@ The following targeted checks map diff signals to specific `events` articles. Tr
- A `var IsHandled` added to a pre-existing event rather than introduced through a new `OnBefore` publisher — `do-not-add-ishandled-to-an-existing-event`.
- An `if IsHandled then exit;` whose skipped body performs posting, ledger-entry creation, number-series consumption, or integrity/permission validation — `do-not-bypass-critical-operations-with-ishandled`.
- A record variable that had `ChangeCompany(<name>)` called on it and is later used with `Insert`, `Modify`, `Delete`, or `Validate`, where the table is not owned by the extension, has triggers that read company data, or has trigger-event subscribers that do not exit on `RunTrigger = false` — `changecompany-runs-triggers-in-the-calling-company`. Do not match a read-only use after `ChangeCompany`, a write with `RunTrigger = false` into an extension-owned table whose triggers do not read company data and whose trigger-event subscribers exit on `RunTrigger = false`, or the parameterless `ChangeCompany()` reset.
- A subscriber to `GetDatabaseTableTriggerSetup` (`Global Triggers`) or `OnAfterGetDatabaseTableTriggerSetup` (`GlobalTriggerManagement`) that assigns one of its `var` flags (`OnDatabaseInsert`, `OnDatabaseModify`, `OnDatabaseDelete`, `OnDatabaseRename`) a literal `false` or a lookup/Boolean expression that does not `or` in the flag's current value, or that calls `Clear` on one — `database-trigger-setup-flags-may-only-be-set-to-true`. Do not match `Flag := true` under a condition, `Flag := Flag or <condition>`, or `if not Flag then Flag := false;`, none of which can clear a flag. Do not match code that runs only in demo-data generation or test-library sessions (for example the base application's demo data tool or a test library's backup/restore subscriber), where clearing other features' flags is accepted.
## Action

1
microsoft/skills/review/al-performance-review.md
View file

@ -47,6 +47,7 @@ Apply these targeted cues even when simple token overlap would rank the article
- Worklist `design-covering-keys-from-read-pattern.md` for a changed secondary key alongside a filtered reader of its table, and `review-overlapping-keys-before-adding-an-index.md` when added or changed keys have overlapping leading fields. A changed key alone is not a finding; read and write workloads determine whether either rule applies.
- Worklist `preserve-buffered-inserts-by-separating-target-reads.md` when a loop calls `Insert` and interleaves operations on the insert target or `Commit`. Worklist `aggregate-before-persisting-intermediate-results.md` when repeated grouping calculations and persistent intermediate summaries appear in the same processing path. Do not infer either pattern from `Insert` or `CalcSums` alone.
- Worklist `use-grouped-query-for-distinct-values-and-duplicates.md` when a record loop sets a filter on a second record variable of the same table to the current row's value and calls `Count`, `IsEmpty`, or `FindFirst` only to decide whether that value is duplicated. Do not worklist it for a single uniqueness check on one record (for example in `OnValidate` or before `Insert`), an outer loop bounded to one parent document, a lookup against a different subset than the looped rows, a loop that acts on each row it finds, or a filtered/counted record that is temporary.
- Worklist `cache-repeated-filtered-results-with-explicit-scope.md` only when the code or workload establishes repeated **complete** lookup keys (for example, querying the same filtered set in multiple passes, or measured key reuse), or shows a cache that omits result-affecting inputs. A single loop over possibly distinct keys does not establish reuse or justify a cache finding. Worklist `avoid-repeating-unchanged-validation.md` for repeated `Validate` of the same field in one path; do not worklist it from a single validation call.
- 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. Also worklist `calcsums-instead-of-calcfields-in-loop.md` when the loop accumulates one set total: use `CalcSums` directly only for stored source fields, never directly on FlowFields; a FlowField total requires deriving equivalent source filters from its `CalcFormula`.
- 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.

8
microsoft/skills/review/al-privacy-review.md
View file

@ -37,11 +37,17 @@ Discard files that are not applicable. Retain conditionally applicable files (an
Narrow the relevant files to the subset that applies to the changes under review. Exclude test codeunits, test libraries, test helper code, files under test/Test/Tests paths, and objects with `Subtype = Test`; test data is synthetic and does not ship to customers. For each relevant file, compute overlap against:
- The changed AL object names and types — especially tables and tableextensions (for `DataClassification` on fields), codeunits that call `Error`, `Session.LogMessage`, or `FeatureTelemetry`, codeunits performing outgoing HTTP requests with customer data, migration codeunits, and objects reading or writing `IsolatedStorage`.
- The changed AL object names and types — especially tables and tableextensions (for `DataClassification` on fields), codeunits that call `Error`, `Session.LogMessage`, or `FeatureTelemetry`, codeunits performing outgoing HTTP requests with customer data, migration codeunits, objects reading or writing `IsolatedStorage`, and install/upgrade codeunits or new tables relevant to retention policies.
- The changed procedures and triggers, weighted toward those that call `Error`, construct `ErrorInfo`, call `Session.LogMessage`, `StrSubstNo`, `GetLastErrorText`/`GetLastErrorCallStack`, `FeatureTelemetry.LogUsage`/`LogUptake`/`LogError`, `HttpClient.Post`/`Get`, `IsolatedStorage.Set`/`SetEncrypted`/`Get`, or `PrivacyNotice.GetPrivacyNoticeApprovalState`.
- Tokens extracted from the diff that relate to privacy (`DataClassification`, `CustomerContent`, `EndUserIdentifiableInformation`, `EndUserPseudonymousIdentifiers`, `SystemMetadata`, `ToBeClassified`, `PrivacyNotice`, `ErrorInfo`, `GetLastErrorText`, `GetLastErrorCallStack`, `TelemetryScope`, `FeatureTelemetry`, `CustomDimensions`, `LogUsage`, `LogUptake`, `LogError`, `ErrorText`, `ErrorCallStack`, `alErrorText`, `alErrorCallStack`, `HybridSL`, `HybridGP`, `HybridBC`).
- Treat `ErrorInfo.Message`, `ErrorInfo.DataClassification`, `ErrorInfo.ErrorType`, and `ErrorInfo.DetailedMessage` as qualified member signals: accept a call or assignment only when symbol resolution proves that its receiver expression or variable has type `ErrorInfo`. Normalize those accesses to `errorinfo-message`, `errorinfo-dataclassification`, `errorinfo-errortype`, and `errorinfo-detailedmessage` retrieval tokens. Bare `Message` or `DataClassification` tokens MUST NOT trigger this article; do not emit the qualified tokens for `Message(...)` dialog calls, table or table-field `DataClassification` properties, or similarly named members on other types. Resolve the receiver's declaration from the containing object when it is outside the changed hunk.
- Worklist ErrorInfo privacy guidance only from those typed `ErrorInfo` member tokens or from construction of an `ErrorInfo` value. For every `FeatureTelemetry.LogError`, inspect the dedicated error text and call-stack arguments in addition to explicit custom dimensions.
- Retention-policy signals. Emit `addallowedtable` for any call to `Codeunit "Reten. Pol. Allowed Tables".AddAllowedTable`, `retention-policy-setup` for any use of `Record "Retention Policy Setup"`, and `retention-policy` for either. Emit `append-only-table` for an extension-owned table (never a tableextension) that the diff adds, or into which the diff adds a new `Insert` path, when the table also passes this bounded check: search the app's `.al` files for the table's object name once each for `AddAllowedTable`, `Delete`, and `DeleteAll`, and emit the token only when none of the three matches. This is a text search per candidate table — do not read or resolve the rest of the app. Emit `deleteall` when the diff adds a `DeleteAll` on an extension-owned table filtered by a date or datetime field inside a job queue codeunit or other scheduled cleanup and the table has no `AddAllowedTable` call.
Route retention-policy guidance deterministically:
- `register-owned-log-tables-for-retention-policies.md` is worklisted by `append-only-table` or `deleteall`, or when an `AddAllowedTable` routine is reached only from an install codeunit, or is guarded by an upgrade tag with no `OnRefreshAllowedTables` subscriber that bypasses the tag. Do not use the table's name as the trigger: an append-only table is a candidate whatever it is called ("Incoming Data" as much as "Activity Log"). A name ending in `Log`, `Entry`, `Archive`, `History`, or `Buffer`, an `AutoIncrement` integer primary key, or inserts from event subscribers, job queue codeunits, or API/web-service handlers raise confidence to `medium`; without any of these the finding stays `low`. Findings from `append-only-table` are advisory (`minor`) because table volume cannot be observed from source.
- `ship-a-default-retention-policy-setup.md` is worklisted by `addallowedtable` or `retention-policy-setup`, but registration without a `Retention Policy Setup` is valid and MUST NOT produce a finding. Report only a false claim: a `Message`, notification, label, page `AboutText`/`InstructionalText`, or setup text in the diff stating that the registered table's data is now cleaned up or deleted automatically when no enabled `Retention Policy Setup` is created for it.
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Apply the topic-specific gates above after this overlap check; in particular, bare `Message` and `DataClassification` tokens cannot admit ErrorInfo guidance. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.

1
microsoft/skills/review/al-style-review.md
View file

@ -61,6 +61,7 @@ Apply these high-signal mappings before fuzzy topic ranking:
- A field or variable is typed `Integer` with the meaning of each value tracked only in a comment, or an `Enum` with more than two members is proposed as `Boolean`-like — `fixed-choice-set-must-use-enum-not-integer.md`. Do not flag a genuinely two-state `Enum`/`Option` for having "too few" members; that overlaps `binary-choice-must-be-boolean.md` instead when the domain is a true/false predicate.
- A new `using` directive is added for an existing AL object without the diff also showing that object's own `namespace` declaration or a symbol-package lookup backing the choice — `namespace-must-be-verified-from-source.md`. Require repository/dependency context; a single new `using` line cannot itself prove whether the namespace was verified or guessed.
- An intrinsic/built-in AL function call (`MESSAGE`, `ERROR`, `CONFIRM`, `STRSUBSTNO`, etc.) is written in ALL-CAPS or another non-PascalCase form — `intrinsic-al-functions-must-use-modern-casing.md`.
- A new codeunit procedure takes a key (`Code`/`Integer`/`Guid`) or a non-`var` `Record`, then `Get`s or uses its own copy and calls `Modify` on that same table's record; a `page`/`pageextension` trigger in the diff calls it with `Rec` or a key field of `Rec`, and later in the same trigger reads the changed `Rec` fields or writes from `Rec` — `mutating-procedure-for-a-page-caller-takes-var-record.md`. Severity at most `minor`. The page refreshes its current record after an action, so stale display alone is not this pattern, and a `Rec.Get` after the call is not evidence by itself. Do not flag job-queue/`TaskScheduler`/page-background-task entry points, API page actions resolving the real record by `SystemId`, generic `RecordId`/`RecordRef`/`Variant` APIs, key-based procedures whose change happens inside a platform/System API, procedures that change a different table or only read, temporary records, by-value records modified inside a `FindSet` loop (owned by `performance/avoid-cloning-records-before-modify-delete-in-loops`), or an in-place signature change to an already-published procedure (owned by `breaking-changes/do-not-change-published-procedure-signatures`).
- A procedure call passes a literal or a computed expression (not a caller-scope variable) to a parameter position the callee declares `var` — `var-parameters-require-an-addressable-variable.md`. This is a compile-time-guaranteed shape; flag it only when the callee's declared signature is visible in the diff or resolvable from context.
Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.

13
schemas/findings-report.schema.json
View file

@ -122,7 +122,7 @@
"additionalProperties": false,
"required": ["id", "severity", "message", "references", "confidence"],
"properties": {
"id": { "type": "string", "minLength": 1 },
"id": { "type": "string", "minLength": 1, "pattern": "^[^#]+$" },
"severity": { "enum": ["blocker", "major", "minor", "info"] },
"message": { "type": "string", "minLength": 1 },
"location": { "$ref": "#/definitions/location" },
@ -135,6 +135,17 @@
"domain": { "type": "string", "minLength": 1, "pattern": "^[^\\r\\n]+$" },
"suggested-code": { "type": "string", "minLength": 1 },
"suggested-code-omission-reason": { "type": "string", "minLength": 1 }
},
"if": {
"properties": {
"references": { "maxItems": 0 }
}
},
"then": {
"properties": {
"confidence": { "enum": ["medium", "low"] },
"severity": { "enum": ["minor", "info"] }
}
}
},
"suppressed": {

24
skills/do.md
View file

@ -128,8 +128,8 @@ source-scope locations, and article-body retrieval.
"message": "string",
"location": {
"file": "string",
"line": 0,
"range": { "start-line": 0, "end-line": 0 }
"line": 1,
"range": { "start-line": 1, "end-line": 1 }
},
"references": [
{ "path": "string", "sha": "string" }
@ -165,6 +165,26 @@ The emitted document MUST be strict, valid JSON per [RFC 8259](https://www.rfc-e
AL source is the common failure case. Quoted identifiers (for example `Rec."No."`) and multi-line snippets routinely appear in `message`, `suggested-code`, and `suggested-code-omission-reason`, and each embedded quote or newline MUST be escaped when placed in a string value. A `suggested-code` payload that spans several lines is a single JSON string with `\n` separators, not a literal multi-line block. Emit the document as one JSON value with no trailing commentary, and do not rely on the consumer to repair unescaped output.
### Producer pre-emission checklist
Before emitting each leaf report or super-skill rollup:
1. Copy every citation-based `findings[].id` verbatim from
`references[0].path`, with no `#` fragment or other suffix. Apply the
reference-integrity gate below.
2. For `references: []`, emit only `confidence: "medium"` or `"low"` and
`severity: "minor"` or `"info"`. Preserve the role-specific agent ID
prefixes defined below.
3. Open the final source snapshot for every `location.file`. Verify `line`
and any inclusive range bounds are 1-based final-file line numbers within
that file's length, never diff/patch-relative line numbers. If the source
snapshot cannot be verified, return `outcome: "failed"` with an
`outcome-reason`, not unverified locations.
Validate the complete document against the schema and semantic rules before
returning it. Consumers MUST NOT strip ID suffixes, downgrade agent findings,
or clamp locations to make an invalid report pass the acceptance gate.
### Consumer acceptance gate
Capture the exact Task return as the immutable raw audit payload and primary

121
tools/Test-ReviewContract.ps1
View file

@ -56,6 +56,14 @@ function Assert-ThrowsLike {
throw "Expected error like '$Pattern', but no error was thrown."
}
function Assert-ReportSchema {
param([object] $Report, [bool] $Expected, [string] $Message)
$valid = $Report | ConvertTo-Json -Depth 30 |
Test-Json -SchemaFile (Join-Path $Root 'schemas/findings-report.schema.json') -ErrorAction SilentlyContinue
Assert-True ($valid -eq $Expected) $Message
}
function Test-PositiveInteger {
param([object] $Value)
@ -116,6 +124,11 @@ foreach ($surface in @(
$normalizedDoContract = $doContract -replace '\s+', ' '
foreach ($expected in @(
'Copy every citation-based `findings[].id` verbatim from `references[0].path`, with no `#` fragment or other suffix.',
'For `references: []`, emit only `confidence: "medium"` or `"low"` and `severity: "minor"` or `"info"`.',
'Open the final source snapshot for every `location.file`.',
'1-based final-file line numbers within that file''s length, never diff/patch-relative line numbers.',
'Consumers MUST NOT strip ID suffixes, downgrade agent findings, or clamp locations',
'positive integers',
'start-line <= line <= end-line',
'does not contain the `suggested-code` field',
@ -286,6 +299,66 @@ try {
$acceptedSuper = & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super
Assert-True (-not $acceptedSuper.normalized) 'valid super-skill report is accepted'
foreach ($isAgent in @($false, $true)) {
foreach ($severity in 'blocker', 'major', 'minor', 'info') {
foreach ($confidence in 'high', 'medium', 'low') {
$schemaLeaf = $validReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json
$schemaLeaf.findings[0].severity = $severity
$schemaLeaf.findings[0].confidence = $confidence
$schemaLeaf.summary.counts.minor = 0
$schemaLeaf.summary.counts.$severity = 1
if ($isAgent) {
$schemaLeaf.findings[0].id = 'agent:uncited-defect'
$schemaLeaf.findings[0].references = @()
}
$expected = -not $isAgent -or ($severity -in @('minor', 'info') -and $confidence -ne 'high')
$caseName = "agent=$isAgent severity=$severity confidence=$confidence"
Assert-ReportSchema $schemaLeaf $expected "leaf schema: $caseName"
$schemaSuper = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json
$schemaSuper.'sub-results'[0] = $schemaLeaf
$schemaSuper.summary.counts = $schemaLeaf.summary.counts
$schemaSuper.findings = @($schemaLeaf.findings[0] | ConvertTo-Json -Depth 20 | ConvertFrom-Json)
$schemaSuper.findings[0] | Add-Member -NotePropertyName 'from-sub-skill' -NotePropertyValue 'al-style-review'
if ($isAgent) {
$schemaSuper.findings[0].id = "al-style-review:$($schemaLeaf.findings[0].id)"
}
Assert-ReportSchema $schemaSuper $expected "rolled-up schema: $caseName"
if ($isAgent) {
$schemaSuper.'sub-results'[0] = $completedLeaf
$schemaSuper.findings[0].id = $schemaLeaf.findings[0].id
$schemaSuper.findings[0].'from-sub-skill' = 'agent'
$schemaSuper.findings[0].domain = 'Agent'
Assert-ReportSchema $schemaSuper $expected "root-owned agent schema: $caseName"
$schemaSuper.findings = @()
$schemaSuper.summary.counts = $completedLeaf.summary.counts
$schemaSuper.'sub-results'[0] = $schemaLeaf
Assert-ReportSchema $schemaSuper $expected "nested leaf schema: $caseName"
}
}
}
}
$citationSuper = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json
$citationSuper.'sub-results'[0] = $validReport
$citationSuper.summary.counts = $validReport.summary.counts
$citationSuper.findings = @($validReport.findings[0] | ConvertTo-Json -Depth 20 | ConvertFrom-Json)
$citationSuper.findings[0] | Add-Member -NotePropertyName 'from-sub-skill' -NotePropertyValue 'al-style-review'
foreach ($position in 'leaf', 'root', 'nested-leaf') {
$fragmentReport = $(if ($position -eq 'leaf') { $validReport } else { $citationSuper }) |
ConvertTo-Json -Depth 20 | ConvertFrom-Json
$fragmentFinding = if ($position -eq 'nested-leaf') {
$fragmentReport.'sub-results'[0].findings[0]
}
else {
$fragmentReport.findings[0]
}
$fragmentFinding.id = "$articlePath#location"
Assert-ReportSchema $fragmentReport $false "$position citation id cannot append a fragment"
}
$duplicateLeafReport = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json
$duplicateLeafReport.'sub-results' = @($completedLeaf, $completedLeaf)
Set-Content -LiteralPath $reportPath -Value ($duplicateLeafReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM
@ -897,8 +970,52 @@ try {
$invalidAgent.findings[0].references = @()
$invalidAgent.findings[0].confidence = 'high'
Set-Content -LiteralPath $reportPath -Value ($invalidAgent | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM
Assert-ThrowsLike -Pattern '*AGENT_CONFIDENCE_INVALID*' -Action {
& $validator -ReportPath $reportPath -BCQualityRoot $Root -SourceRoot $tmp -SourcePaths $sourcePath
$invalidAgentRaw = [IO.File]::ReadAllText($reportPath)
Assert-ThrowsLike -Pattern '*Invalid findings-report JSON or schema*' -Action {
& $validator -ReportPath $reportPath -BCQualityRoot $Root -SourceRoot $tmp -SourcePaths $sourcePath `
-AllowBoundedNormalization
}
Assert-True ([IO.File]::ReadAllText($reportPath) -ceq $invalidAgentRaw) 'invalid agent payload is not silently repaired'
$mismatchedCitation = $validReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json
$mismatchedCitation.findings[0].id = "${articlePath}:location"
Assert-ReportSchema $mismatchedCitation $true 'cross-field citation equality still requires semantic validation'
Set-Content -LiteralPath $reportPath -Value ($mismatchedCitation | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM
Assert-ThrowsLike -Pattern '*PRIMARY_REFERENCE_MISMATCH*' -Action {
& $validator -ReportPath $reportPath -BCQualityRoot $Root -SourceRoot $tmp `
-SourcePaths $sourcePath -RetrievedArticlePaths $articlePath -AllowBoundedNormalization
}
$finalSourcePath = 'src/final-snapshot.al'
Set-Content -LiteralPath (Join-Path $tmp $finalSourcePath) -Value (1..22 | ForEach-Object { "line $_" }) -Encoding utf8NoBOM
foreach ($bounds in @(
@{ Line = 22; End = 22; Error = $null }
@{ Line = 44; End = $null; Error = '*SOURCE_LINE_INVALID*' }
@{ Line = 22; End = 44; Error = '*SOURCE_RANGE_INVALID*' }
)) {
$locationReport = $validReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json
$locationReport.findings[0].location = @{
file = $finalSourcePath
line = $bounds.Line
}
if ($bounds.End) {
$locationReport.findings[0].location.range = @{ 'start-line' = $bounds.Line; 'end-line' = $bounds.End }
}
Assert-ReportSchema $locationReport $true 'schema alone cannot verify final-file line bounds'
Set-Content -LiteralPath $reportPath -Value ($locationReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM
$locationRaw = [IO.File]::ReadAllText($reportPath)
$validateLocation = {
& $validator -ReportPath $reportPath -BCQualityRoot $Root -SourceRoot $tmp `
-SourcePaths $finalSourcePath -RetrievedArticlePaths $articlePath -AllowBoundedNormalization
}
if ($bounds.Error) {
Assert-ThrowsLike -Pattern $bounds.Error -Action $validateLocation
}
else {
$acceptedLocation = & $validateLocation
Assert-True (-not $acceptedLocation.normalized) 'the final source line is accepted without normalization'
}
Assert-True ([IO.File]::ReadAllText($reportPath) -ceq $locationRaw) 'source locations are never clamped in the raw payload'
}
$normalizable = $validReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json