From 2f3e5afac1fcb4cccf23f09fac1d9e4a79b95e04 Mon Sep 17 00:00:00 2001 From: Jeremy Vyska Date: Thu, 10 Sep 2026 12:58:26 +0200 Subject: [PATCH 1/5] Add retention policy knowledge to the privacy domain Two articles covering retention policies for extension-owned tables, the gap that lets high-volume log tables grow unbounded: - register-owned-log-tables-for-retention-policies: an extension's own log tables must be added to the allowed-tables list from install AND upgrade code, guarded by IsAllowedTable plus an upgrade tag, with a mandatory minimum retention where audit needs one. - ship-a-default-retention-policy-setup: registration only makes a table selectable; nothing is deleted until a Retention Policy Setup record exists, so ship one (disabled by default) as the platform's own Retention Policy Installer does. Each ships good/bad AL samples. Claims verified against the BC admin docs and the Retention Policy module in microsoft/BCApps. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011gvTjm746MtJEVWeRTbG46 --- ...d-log-tables-for-retention-policies.bad.al | 17 ++++++ ...-log-tables-for-retention-policies.good.al | 55 +++++++++++++++++++ ...owned-log-tables-for-retention-policies.md | 33 +++++++++++ ...ip-a-default-retention-policy-setup.bad.al | 18 ++++++ ...p-a-default-retention-policy-setup.good.al | 48 ++++++++++++++++ .../ship-a-default-retention-policy-setup.md | 31 +++++++++++ 6 files changed, 202 insertions(+) create mode 100644 microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al create mode 100644 microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al create mode 100644 microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md create mode 100644 microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al create mode 100644 microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al create mode 100644 microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md diff --git a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al new file mode 100644 index 0000000..c3a35f2 --- /dev/null +++ b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al @@ -0,0 +1,17 @@ +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; +} diff --git a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al new file mode 100644 index 0000000..8941e50 --- /dev/null +++ b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al @@ -0,0 +1,55 @@ +codeunit 50560 "Contoso Reten. Pol. Setup" +{ + Access = Internal; + + procedure AddAllowedTables() + var + ContosoActivityLog: Record "Contoso Activity Log"; + RetenPolAllowedTables: Codeunit "Reten. Pol. Allowed Tables"; + UpgradeTag: Codeunit "Upgrade Tag"; + begin + if UpgradeTag.HasUpgradeTag(AllowedTableTag()) 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 + + UpgradeTag.SetUpgradeTag(AllowedTableTag()); + end; + + local procedure AllowedTableTag(): Code[250] + begin + exit('Contoso-ActivityLogAllowedTable-20260910'); + 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; +} diff --git a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md new file mode 100644 index 0000000..4861049 --- /dev/null +++ b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md @@ -0,0 +1,33 @@ +--- +bc-version: [all] +domain: privacy +keywords: [retention-policy, allowed-tables, addallowedtable, reten-pol-allowed-tables, log-table-growth, 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. 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 log table with no `AddAllowedTable` call anywhere in the app, cleaned instead 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`, and `AddTableFilterToJsonArray`. diff --git a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al new file mode 100644 index 0000000..d008e8e --- /dev/null +++ b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al @@ -0,0 +1,18 @@ +codeunit 50565 "Contoso Reten. Pol. Register" +{ + Subtype = Install; + Access = Internal; + + // The table becomes selectable on the Retention Policies page and nothing + // else happens: no Retention Policy Setup record is created, so no period + // is proposed and no data is ever deleted. The log table keeps growing + // until someone notices the tenant's storage consumption. + trigger OnInstallAppPerCompany() + var + ContosoActivityLog: Record "Contoso Activity Log"; + RetenPolAllowedTables: Codeunit "Reten. Pol. Allowed Tables"; + begin + RetenPolAllowedTables.AddAllowedTable( + Database::"Contoso Activity Log", ContosoActivityLog.FieldNo(SystemCreatedAt)); + end; +} diff --git a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al new file mode 100644 index 0000000..0e32fb2 --- /dev/null +++ b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al @@ -0,0 +1,48 @@ +codeunit 50564 "Contoso Reten. Pol. Default" +{ + Access = Internal; + + var + SixMonthsTok: Label 'Six Months', MaxLength = 20; + + procedure CreateDefaultPolicy() + var + RetentionPolicySetup: Record "Retention Policy Setup"; + UpgradeTag: Codeunit "Upgrade Tag"; + begin + // 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", SixMonthRetentionPeriod()); + RetentionPolicySetup.Validate(Enabled, false); // the administrator opts in to deletion + RetentionPolicySetup.Insert(true); + end; + + UpgradeTag.SetUpgradeTag(DefaultPolicyTag()); + end; + + local procedure SixMonthRetentionPeriod(): Code[20] + var + RetentionPeriod: Record "Retention Period"; + begin + RetentionPeriod.SetRange("Retention Period", RetentionPeriod."Retention Period"::"6 Months"); + if RetentionPeriod.FindFirst() then + exit(RetentionPeriod.Code); + + RetentionPeriod.Code := CopyStr(UpperCase(SixMonthsTok), 1, MaxStrLen(RetentionPeriod.Code)); + RetentionPeriod.Description := SixMonthsTok; + RetentionPeriod.Validate("Retention Period", RetentionPeriod."Retention Period"::"6 Months"); + RetentionPeriod.Insert(true); + exit(RetentionPeriod.Code); + end; + + local procedure DefaultPolicyTag(): Code[250] + begin + exit('Contoso-ActivityLogDefaultPolicy-20260910'); + end; +} diff --git a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md new file mode 100644 index 0000000..a8e4f0f --- /dev/null +++ b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md @@ -0,0 +1,31 @@ +--- +bc-version: [all] +domain: privacy +keywords: [retention-policy-setup, retention-period, default-policy, unbounded-table-growth, opt-in-deletion, upgrade-tag] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Registering a table is not a retention policy — ship 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. An extension that registers its log tables and stops there ships unbounded growth as its default behaviour: the administrator has to discover the page, know which of the extension's tables are safe to trim, and pick a period the extension's own author never documented. Microsoft's `Codeunit 3907 "Retention Policy Installer"` shows the intended 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 + +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)), create the `Retention Policy Setup` record: reuse an existing `Retention Period` whose `"Retention Period"` enum value matches the period you want and create one only when none exists, then `Validate` `"Table Id"`, `"Apply to all records"` and `"Retention Period"` before inserting. 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 + +Install code that calls `AddAllowedTable` and treats the table as covered by retention policies. The signal is an install or upgrade routine that touches `Codeunit "Reten. Pol. Allowed Tables"` but never `Record "Retention Policy Setup"`; the symptom is a support case where the extension's log table holds millions of rows on a tenant whose **Retention Policies** page has no line for it. The mirror-image defect is inserting the setup with `Enabled` set to true and no documentation, so the app begins deleting tenant data on a schedule nobody approved. + +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. From 4bef582ffe817bea39d63b8efc99064131b6cee3 Mon Sep 17 00:00:00 2001 From: Jeremy Vyska Date: Mon, 28 Sep 2026 09:41:25 +0200 Subject: [PATCH 2/5] Address review feedback on retention policy knowledge - Scope both articles to bc-version [17..] (retention policies shipped in v17). - Allowed-tables sample: add OnRefreshAllowedTables subscriber with a ForceUpdate path; the upgrade tag now gates one-time setup only. - Default-policy sample: use Retention Policy Setup.FindOrCreateRetentionPeriod instead of a hand-rolled lookup-then-insert that can collide on code. - Anti-pattern now keys on append-only tables rather than table names. - al-privacy-review: add retention-policy tokens and deterministic routing for both articles, with a bounded per-table text search for delete and registration paths. Co-Authored-By: Claude Opus 5.5 --- ...-log-tables-for-retention-policies.good.al | 20 ++++++++++++++-- ...owned-log-tables-for-retention-policies.md | 10 ++++---- ...p-a-default-retention-policy-setup.good.al | 23 ++++--------------- .../ship-a-default-retention-policy-setup.md | 6 ++--- microsoft/skills/review/al-privacy-review.md | 8 ++++++- 5 files changed, 37 insertions(+), 30 deletions(-) diff --git a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al index 8941e50..b6119e1 100644 --- a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al +++ b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al @@ -3,12 +3,21 @@ 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 - if UpgradeTag.HasUpgradeTag(AllowedTableTag()) then + IsInitialSetup := not UpgradeTag.HasUpgradeTag(AllowedTableTag()); + if not (IsInitialSetup or ForceUpdate) then exit; if not RetenPolAllowedTables.IsAllowedTable(Database::"Contoso Activity Log") then @@ -17,13 +26,20 @@ codeunit 50560 "Contoso Reten. Pol. Setup" ContosoActivityLog.FieldNo(SystemCreatedAt), 28); // support cases need at least four weeks of log history - UpgradeTag.SetUpgradeTag(AllowedTableTag()); + 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" diff --git a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md index 4861049..8c6bc8d 100644 --- a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md +++ b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md @@ -1,7 +1,7 @@ --- -bc-version: [all] +bc-version: [17..] domain: privacy -keywords: [retention-policy, allowed-tables, addallowedtable, reten-pol-allowed-tables, log-table-growth, mandatory-minimum-retention, install-upgrade-codeunit] +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] @@ -15,7 +15,7 @@ The retention policy engine only ever deletes from tables that appear in its all ## 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. 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. +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). @@ -23,11 +23,11 @@ See sample: [`register-owned-log-tables-for-retention-policies.good.al`](registe ## Anti Pattern -An extension-owned log table with no `AddAllowedTable` call anywhere in the app, cleaned instead 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. +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`, and `AddTableFilterToJsonArray`. +- [`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`. diff --git a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al index 0e32fb2..3724ffc 100644 --- a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al +++ b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al @@ -2,12 +2,10 @@ codeunit 50564 "Contoso Reten. Pol. Default" { Access = Internal; - var - SixMonthsTok: Label 'Six Months', MaxLength = 20; - procedure CreateDefaultPolicy() var RetentionPolicySetup: Record "Retention Policy Setup"; + RetentionPolicySetupMgt: Codeunit "Retention Policy Setup"; UpgradeTag: Codeunit "Upgrade Tag"; begin // Created once per company: an administrator who deletes the policy @@ -18,7 +16,9 @@ codeunit 50564 "Contoso Reten. Pol. Default" 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", SixMonthRetentionPeriod()); + 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; @@ -26,21 +26,6 @@ codeunit 50564 "Contoso Reten. Pol. Default" UpgradeTag.SetUpgradeTag(DefaultPolicyTag()); end; - local procedure SixMonthRetentionPeriod(): Code[20] - var - RetentionPeriod: Record "Retention Period"; - begin - RetentionPeriod.SetRange("Retention Period", RetentionPeriod."Retention Period"::"6 Months"); - if RetentionPeriod.FindFirst() then - exit(RetentionPeriod.Code); - - RetentionPeriod.Code := CopyStr(UpperCase(SixMonthsTok), 1, MaxStrLen(RetentionPeriod.Code)); - RetentionPeriod.Description := SixMonthsTok; - RetentionPeriod.Validate("Retention Period", RetentionPeriod."Retention Period"::"6 Months"); - RetentionPeriod.Insert(true); - exit(RetentionPeriod.Code); - end; - local procedure DefaultPolicyTag(): Code[250] begin exit('Contoso-ActivityLogDefaultPolicy-20260910'); diff --git a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md index a8e4f0f..9426b6d 100644 --- a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md +++ b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md @@ -1,7 +1,7 @@ --- -bc-version: [all] +bc-version: [17..] domain: privacy -keywords: [retention-policy-setup, retention-period, default-policy, unbounded-table-growth, opt-in-deletion, upgrade-tag] +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] @@ -15,7 +15,7 @@ application-area: [all] ## Best Practice -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)), create the `Retention Policy Setup` record: reuse an existing `Retention Period` whose `"Retention Period"` enum value matches the period you want and create one only when none exists, then `Validate` `"Table Id"`, `"Apply to all records"` and `"Retention Period"` before inserting. 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. +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)), 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. 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). diff --git a/microsoft/skills/review/al-privacy-review.md b/microsoft/skills/review/al-privacy-review.md index 469000c..1e9df81 100644 --- a/microsoft/skills/review/al-privacy-review.md +++ b/microsoft/skills/review/al-privacy-review.md @@ -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` when the same install or upgrade routine, or the codeunit that contains it, never references `Record "Retention Policy Setup"`. It is also worklisted by `retention-policy-setup` when the diff inserts a setup with `Enabled` validated to true, or builds a `Retention Period` record by hand instead of calling `FindOrCreateRetentionPeriod`. 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. From 07a171386d812bbeff303995321408860e3d3abb Mon Sep 17 00:00:00 2001 From: Jeremy Vyska Date: Wed, 30 Sep 2026 15:46:32 +0200 Subject: [PATCH 3/5] Address second review round on retention policy knowledge - Scope both articles to bc-version [22..]: FindOrCreateRetentionPeriod first appears in the 21.1 System Application and OnRefreshAllowedTables in 22. - Reframe the default-setup article as optional guidance; registration without a setup is valid. The anti-pattern and review routing now cover only false claims that registration alone cleans up data. - Make all four samples self-contained: declare Contoso Activity Log in each, add the Retention Policy Setup permission, and guard the default setup on IsAllowedTable. - Let evaluation overrides list additionalArticles and register both retention pairs as extra privacy cases (38 cases, existing IDs unchanged). Co-Authored-By: Claude Opus 5.5 --- evaluation/README.md | 2 +- evaluation/review-fixtures.json | 6 +- ...d-log-tables-for-retention-policies.bad.al | 16 ++++ ...-log-tables-for-retention-policies.good.al | 16 ++++ ...owned-log-tables-for-retention-policies.md | 2 +- ...ip-a-default-retention-policy-setup.bad.al | 80 +++++++++++++++++-- ...p-a-default-retention-policy-setup.good.al | 22 +++++ .../ship-a-default-retention-policy-setup.md | 10 +-- microsoft/skills/review/al-privacy-review.md | 2 +- tools/Test-ReviewFixtures.ps1 | 51 +++++++++--- 10 files changed, 177 insertions(+), 30 deletions(-) diff --git a/evaluation/README.md b/evaluation/README.md index 7063baf..a3a661b 100644 --- a/evaluation/README.md +++ b/evaluation/README.md @@ -2,7 +2,7 @@ The evaluation is convention-driven. The harness discovers every `/skills/review/al--review.md` leaf across the enabled `microsoft`, `community`, and `custom` layers. Duplicate domains resolve with `custom > community > microsoft` precedence. For each selected leaf, the harness finds paired knowledge across the same layers, applies the same precedence to duplicate article slugs, selects the first article (by filename) with both `.bad.al` and `.good.al` companions, and derives the expected positive and clean control automatically. Adding a conforming leaf requires no scoring-contract edit. -`review-fixtures.json` contains only global thresholds and optional exceptional overrides. An override may select a different article or add context when the generic convention cannot express a scenario. It should remain empty in the normal case. +`review-fixtures.json` contains only global thresholds and optional exceptional overrides. An override may select a different article, add context, or list `additionalArticles` whose sample pairs become extra positive and clean cases for that domain, when the generic convention cannot express a scenario. It should remain empty in the normal case. Model-facing preparation hashes case IDs, neutralizes `Good`/`Bad` object-name tokens, and removes full-line sample comments so neither the article slug, domain, nor expected outcome reveals the answer. diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index f5c7046..d671ba9 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -23,7 +23,11 @@ "article": "use-isempty-for-existence-check" }, "privacy": { - "article": "no-pii-in-telemetry-message-string" + "article": "no-pii-in-telemetry-message-string", + "additionalArticles": [ + "register-owned-log-tables-for-retention-policies", + "ship-a-default-retention-policy-setup" + ] }, "style": { "article": "label-comment-explains-placeholders" diff --git a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al index c3a35f2..19ccdef 100644 --- a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al +++ b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.bad.al @@ -1,3 +1,19 @@ +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; diff --git a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al index b6119e1..b9246ec 100644 --- a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al +++ b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.good.al @@ -1,3 +1,19 @@ +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; diff --git a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md index 8c6bc8d..1e3bc92 100644 --- a/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md +++ b/microsoft/knowledge/privacy/register-owned-log-tables-for-retention-policies.md @@ -1,5 +1,5 @@ --- -bc-version: [17..] +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] diff --git a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al index d008e8e..fbd6d5a 100644 --- a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al +++ b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.bad.al @@ -1,18 +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" { - Subtype = Install; Access = Internal; - // The table becomes selectable on the Retention Policies page and nothing - // else happens: no Retention Policy Setup record is created, so no period - // is proposed and no data is ever deleted. The log table keeps growing - // until someone notices the tenant's storage consumption. - trigger OnInstallAppPerCompany() + procedure AddAllowedTables() var ContosoActivityLog: Record "Contoso Activity Log"; RetenPolAllowedTables: Codeunit "Reten. Pol. Allowed Tables"; begin - RetenPolAllowedTables.AddAllowedTable( - Database::"Contoso Activity Log", ContosoActivityLog.FieldNo(SystemCreatedAt)); + 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; } diff --git a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al index 3724ffc..d85da2a 100644 --- a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al +++ b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.good.al @@ -1,13 +1,35 @@ +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 diff --git a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md index 9426b6d..b450b6f 100644 --- a/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md +++ b/microsoft/knowledge/privacy/ship-a-default-retention-policy-setup.md @@ -1,5 +1,5 @@ --- -bc-version: [17..] +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] @@ -7,21 +7,21 @@ countries: [w1] application-area: [all] --- -# Registering a table is not a retention policy — ship a default setup +# 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. An extension that registers its log tables and stops there ships unbounded growth as its default behaviour: the administrator has to discover the page, know which of the extension's tables are safe to trim, and pick a period the extension's own author never documented. Microsoft's `Codeunit 3907 "Retention Policy Installer"` shows the intended 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. +`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 -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)), 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. 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. +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 -Install code that calls `AddAllowedTable` and treats the table as covered by retention policies. The signal is an install or upgrade routine that touches `Codeunit "Reten. Pol. Allowed Tables"` but never `Record "Retention Policy Setup"`; the symptom is a support case where the extension's log table holds millions of rows on a tenant whose **Retention Policies** page has no line for it. The mirror-image defect is inserting the setup with `Enabled` set to true and no documentation, so the app begins deleting tenant data on a schedule nobody approved. +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). diff --git a/microsoft/skills/review/al-privacy-review.md b/microsoft/skills/review/al-privacy-review.md index 1e9df81..f5aa66a 100644 --- a/microsoft/skills/review/al-privacy-review.md +++ b/microsoft/skills/review/al-privacy-review.md @@ -47,7 +47,7 @@ Narrow the relevant files to the subset that applies to the changes under review 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` when the same install or upgrade routine, or the codeunit that contains it, never references `Record "Retention Policy Setup"`. It is also worklisted by `retention-policy-setup` when the diff inserts a setup with `Enabled` validated to true, or builds a `Retention Period` record by hand instead of calling `FindOrCreateRetentionPeriod`. +- `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. diff --git a/tools/Test-ReviewFixtures.ps1 b/tools/Test-ReviewFixtures.ps1 index a9912b7..1a376b9 100644 --- a/tools/Test-ReviewFixtures.ps1 +++ b/tools/Test-ReviewFixtures.ps1 @@ -4,7 +4,7 @@ .DESCRIPTION CI uses the static validation path to prove every registered AL review leaf - has one positive and one clean control, every fixture/reference exists, and + has at least one positive and one clean control, every fixture/reference exists, and the manifest remains internally consistent. For an actual model run, -PrepareDirectory copies inputs to neutral names and @@ -226,24 +226,49 @@ foreach ($domain in $leafDomains) { continue } - $articlePath = [string]$selectedArticle.ArticlePath - $sampleDirectory = (Split-Path -Parent $articlePath).Replace('\', '/') + # The primary article keeps the domain-level case IDs; additionalArticles add + # further paired cases keyed by slug so existing case hashes stay stable. + $selections = [System.Collections.Generic.List[object]]::new() + $selections.Add([pscustomobject]@{ Article = $selectedArticle; IdPrefix = $domain }) | Out-Null + if ($override -and ($override.PSObject.Properties.Name -contains 'additionalArticles')) { + foreach ($additionalName in @($override.additionalArticles)) { + $additionalName = [string]$additionalName + if ($additionalName.EndsWith('.md')) { + $additionalName = [System.IO.Path]::GetFileNameWithoutExtension($additionalName) + } + if (@($selections | Where-Object { $_.Article.BaseName -eq $additionalName }).Count) { + $problems.Add("${domain}: additional article is already selected: $additionalName.md") | Out-Null + continue + } + $additionalArticle = $articles | Where-Object BaseName -eq $additionalName | Select-Object -First 1 + if (-not $additionalArticle) { + $problems.Add("${domain}: additional article does not exist or lacks .good.al and .bad.al companion samples: $additionalName.md") | Out-Null + continue + } + $selections.Add([pscustomobject]@{ Article = $additionalArticle; IdPrefix = "$domain-$additionalName" }) | Out-Null + } + } + $context = if ($override -and ($override.PSObject.Properties.Name -contains 'context')) { [string]$override.context } else { $null } - foreach ($kind in 'bad', 'good') { - $case = [pscustomobject]@{ - id = "$domain-$kind" - domain = $domain - input = "$sampleDirectory/$($selectedArticle.BaseName).$kind.al" - expected = if ($kind -eq 'bad') { @($articlePath) } else { @() } + foreach ($selection in $selections) { + $articlePath = [string]$selection.Article.ArticlePath + $sampleDirectory = (Split-Path -Parent $articlePath).Replace('\', '/') + foreach ($kind in 'bad', 'good') { + $case = [pscustomobject]@{ + id = "$($selection.IdPrefix)-$kind" + domain = $domain + input = "$sampleDirectory/$($selection.Article.BaseName).$kind.al" + expected = if ($kind -eq 'bad') { @($articlePath) } else { @() } + } + if ($context) { + $case | Add-Member -NotePropertyName context -NotePropertyValue $context + } + $caseList.Add($case) | Out-Null } - if ($context) { - $case | Add-Member -NotePropertyName context -NotePropertyValue $context - } - $caseList.Add($case) | Out-Null } } $cases = @($caseList) From f9333addcdf7afae3f385d07346d52b448af1f99 Mon Sep 17 00:00:00 2001 From: Jeremy Vyska Date: Wed, 30 Sep 2026 15:49:13 +0200 Subject: [PATCH 4/5] Revert evaluation harness change for multiple articles per domain The harness intentionally evaluates one paired article per domain. Keep it as designed; how the retention pairs join privacy evaluation is left to the maintainers. Co-Authored-By: Claude Opus 5.5 --- evaluation/README.md | 2 +- evaluation/review-fixtures.json | 6 +--- tools/Test-ReviewFixtures.ps1 | 51 +++++++++------------------------ 3 files changed, 15 insertions(+), 44 deletions(-) diff --git a/evaluation/README.md b/evaluation/README.md index a3a661b..7063baf 100644 --- a/evaluation/README.md +++ b/evaluation/README.md @@ -2,7 +2,7 @@ The evaluation is convention-driven. The harness discovers every `/skills/review/al--review.md` leaf across the enabled `microsoft`, `community`, and `custom` layers. Duplicate domains resolve with `custom > community > microsoft` precedence. For each selected leaf, the harness finds paired knowledge across the same layers, applies the same precedence to duplicate article slugs, selects the first article (by filename) with both `.bad.al` and `.good.al` companions, and derives the expected positive and clean control automatically. Adding a conforming leaf requires no scoring-contract edit. -`review-fixtures.json` contains only global thresholds and optional exceptional overrides. An override may select a different article, add context, or list `additionalArticles` whose sample pairs become extra positive and clean cases for that domain, when the generic convention cannot express a scenario. It should remain empty in the normal case. +`review-fixtures.json` contains only global thresholds and optional exceptional overrides. An override may select a different article or add context when the generic convention cannot express a scenario. It should remain empty in the normal case. Model-facing preparation hashes case IDs, neutralizes `Good`/`Bad` object-name tokens, and removes full-line sample comments so neither the article slug, domain, nor expected outcome reveals the answer. diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index d671ba9..f5c7046 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -23,11 +23,7 @@ "article": "use-isempty-for-existence-check" }, "privacy": { - "article": "no-pii-in-telemetry-message-string", - "additionalArticles": [ - "register-owned-log-tables-for-retention-policies", - "ship-a-default-retention-policy-setup" - ] + "article": "no-pii-in-telemetry-message-string" }, "style": { "article": "label-comment-explains-placeholders" diff --git a/tools/Test-ReviewFixtures.ps1 b/tools/Test-ReviewFixtures.ps1 index 1a376b9..a9912b7 100644 --- a/tools/Test-ReviewFixtures.ps1 +++ b/tools/Test-ReviewFixtures.ps1 @@ -4,7 +4,7 @@ .DESCRIPTION CI uses the static validation path to prove every registered AL review leaf - has at least one positive and one clean control, every fixture/reference exists, and + has one positive and one clean control, every fixture/reference exists, and the manifest remains internally consistent. For an actual model run, -PrepareDirectory copies inputs to neutral names and @@ -226,49 +226,24 @@ foreach ($domain in $leafDomains) { continue } - # The primary article keeps the domain-level case IDs; additionalArticles add - # further paired cases keyed by slug so existing case hashes stay stable. - $selections = [System.Collections.Generic.List[object]]::new() - $selections.Add([pscustomobject]@{ Article = $selectedArticle; IdPrefix = $domain }) | Out-Null - if ($override -and ($override.PSObject.Properties.Name -contains 'additionalArticles')) { - foreach ($additionalName in @($override.additionalArticles)) { - $additionalName = [string]$additionalName - if ($additionalName.EndsWith('.md')) { - $additionalName = [System.IO.Path]::GetFileNameWithoutExtension($additionalName) - } - if (@($selections | Where-Object { $_.Article.BaseName -eq $additionalName }).Count) { - $problems.Add("${domain}: additional article is already selected: $additionalName.md") | Out-Null - continue - } - $additionalArticle = $articles | Where-Object BaseName -eq $additionalName | Select-Object -First 1 - if (-not $additionalArticle) { - $problems.Add("${domain}: additional article does not exist or lacks .good.al and .bad.al companion samples: $additionalName.md") | Out-Null - continue - } - $selections.Add([pscustomobject]@{ Article = $additionalArticle; IdPrefix = "$domain-$additionalName" }) | Out-Null - } - } - + $articlePath = [string]$selectedArticle.ArticlePath + $sampleDirectory = (Split-Path -Parent $articlePath).Replace('\', '/') $context = if ($override -and ($override.PSObject.Properties.Name -contains 'context')) { [string]$override.context } else { $null } - foreach ($selection in $selections) { - $articlePath = [string]$selection.Article.ArticlePath - $sampleDirectory = (Split-Path -Parent $articlePath).Replace('\', '/') - foreach ($kind in 'bad', 'good') { - $case = [pscustomobject]@{ - id = "$($selection.IdPrefix)-$kind" - domain = $domain - input = "$sampleDirectory/$($selection.Article.BaseName).$kind.al" - expected = if ($kind -eq 'bad') { @($articlePath) } else { @() } - } - if ($context) { - $case | Add-Member -NotePropertyName context -NotePropertyValue $context - } - $caseList.Add($case) | Out-Null + foreach ($kind in 'bad', 'good') { + $case = [pscustomobject]@{ + id = "$domain-$kind" + domain = $domain + input = "$sampleDirectory/$($selectedArticle.BaseName).$kind.al" + expected = if ($kind -eq 'bad') { @($articlePath) } else { @() } } + if ($context) { + $case | Add-Member -NotePropertyName context -NotePropertyValue $context + } + $caseList.Add($case) | Out-Null } } $cases = @($caseList) From f199ba95662f0cb24eb76a483836d19ce7226041 Mon Sep 17 00:00:00 2001 From: Jeremy Vyska Date: Wed, 30 Sep 2026 15:53:49 +0200 Subject: [PATCH 5/5] Register retention policy pairs in the privacy evaluation override Use main's articles override so both retention article pairs get positive and clean cases alongside no-pii-in-telemetry-message-string. Co-Authored-By: Claude Opus 5.5 --- evaluation/review-fixtures.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index 5f0e50a..19819e7 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -84,7 +84,11 @@ ] }, "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": [