From 0120b874b2859bb999f33015de8786392e6b03ed Mon Sep 17 00:00:00 2001
From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com>
Date: Mon, 7 Sep 2026 21:02:51 +0200
Subject: [PATCH] Address Jesper Schulz-Wedde's review on PR #157

- release-must-update-app-version.md: reframe around AppSource's actual
  strict full-version-ordering requirement; scope branching-policy
  claims as team convention, not platform rule.
- pictures-must-use-media-not-blob.md: MediaSet is a collection of
  independent media objects, not automatic image variants/thumbnails.
- log-writes-must-survive-rollback.{md,good.al}: StartSession's only
  data channel into the new session is its Record parameter to a
  TableNo-scoped codeunit; a setter called on a local instance before
  starting the session populates nothing in the new session.
- exposed-objects-must-be-in-a-permission-set.md: correct the three
  exposure mechanisms (Web Services config, PageType/QueryType=API,
  ServiceEnabled as a method-only attribute).
- pages-must-not-contain-business-logic.md: scope to persisted
  mutations and cross-entry-point rules; presentation-only
  calculations and table-owned invariants are not violations.
- given-blocks-must-cover-full-precondition-chain.good.al: replace
  invented LibrarySales calls with the real API
  (CreateCustomer/CreateSalesOrderForCustomerNo/PostSalesDocument).
- test-feature-scenario-tags.{md,good.al}: move [SCENARIO] inside the
  test procedure body to match the current BCApps corpus; keep
  [FEATURE] at codeunit level per Microsoft's own documented option.
- ui-test-codeunit-naming.md: scope the _UT suffix and adjacent-ID
  pairing as an explicit team convention, not a BCApps-wide standard.
- page-design-must-match-bc-page-type-conventions.md /
  table-design-must-match-bc-table-type-conventions.md: Card's
  single-key primary-key claim is a contextual heuristic, not a
  mandatory constraint (Ship-to Address, Customer/Vendor Bank Account
  are real composite-key Card pages); a Subsidiary table with its own
  identity commonly gets List+Card, not Worksheet/Tabular.
- api-page-least-privilege-write-access.{md,good.al}: only page-placed
  fields are ever exposed; set InsertAllowed/DeleteAllowed=false in the
  good sample so a narrow field set can't still create/delete records.
- source-organized-by-feature-not-object-type.md,
  test-one-when-per-test.md: scope as team/testing-design conventions,
  not Microsoft platform requirements.
- upgrade-tag-logic-must-not-nest-deeply.md: add the Microsoft Learn
  citation that already backs the two-level nesting limit.
- Wire the new articles into the testing/data-modeling/error-handling/
  security/ui review skills' candidate-selection signals.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 .../release-must-update-app-version.md        |  6 +--
 .../pictures-must-use-media-not-blob.md       | 27 +++++-----
 ...gn-must-match-bc-table-type-conventions.md |  7 ++-
 .../log-writes-must-survive-rollback.good.al  | 49 +++++++++++++------
 .../log-writes-must-survive-rollback.md       |  2 +-
 ...sed-objects-must-be-in-a-permission-set.md |  2 +-
 .../pages-must-not-contain-business-logic.md  |  4 +-
 ...ce-organized-by-feature-not-object-type.md |  2 +-
 ...must-cover-full-precondition-chain.good.al | 15 ++++--
 .../test-feature-scenario-tags.good.al        |  2 +-
 .../testing/test-feature-scenario-tags.md     |  2 +-
 .../testing/test-one-when-per-test.md         |  2 +-
 .../testing/ui-test-codeunit-naming.md        | 10 ++--
 ...ign-must-match-bc-page-type-conventions.md | 19 ++++---
 .../upgrade-tag-logic-must-not-nest-deeply.md |  4 ++
 ...-page-least-privilege-write-access.good.al |  2 +
 .../api-page-least-privilege-write-access.md  |  4 +-
 .../skills/review/al-data-modeling-review.md  |  2 +
 .../skills/review/al-error-handling-review.md |  1 +
 microsoft/skills/review/al-security-review.md |  2 +-
 microsoft/skills/review/al-testing-review.md  |  3 ++
 microsoft/skills/review/al-ui-review.md       |  2 +-
 22 files changed, 111 insertions(+), 58 deletions(-)

diff --git a/microsoft/knowledge/appsource/release-must-update-app-version.md b/microsoft/knowledge/appsource/release-must-update-app-version.md
index 91fb093..d7debe8 100644
--- a/microsoft/knowledge/appsource/release-must-update-app-version.md
+++ b/microsoft/knowledge/appsource/release-must-update-app-version.md
@@ -19,16 +19,16 @@ At every release — a branch merged to `main`, a tagged release build, or an Ap
 | Minor | Developer decision | Every release with new functionality |
 | Build / Revision | AL-Go pipeline | Automatic — never hand-edited |
 
-The version number is the only identity a deployed app has. Two customer environments running "the same" version with different code is an undiagnosable support case; an AppSource submission with an unchanged major.minor is a rejected submission. AL-Go increments build numbers on every CI run, which creates the illusion that versioning is handled — but major.minor is a human statement about compatibility, and no pipeline can make it.
+The version number is the only identity a deployed app has. Two customer environments running "the same" version with different code is an undiagnosable support case. AppSource's actual requirement is strict full-version ordering — the complete version must be greater than the previously submitted version — which an AL-Go-generated build/revision increment can satisfy on its own; AppSource does not require major.minor itself to change. Treating major.minor as a deliberate, human-decided compatibility signal is still valuable practice — it is a statement about what changed that no pipeline can make on its own — just not a platform-enforced requirement.
 
 ## Best Practice
 
     Before the release merge:
-      app.json:       "version": "1.3.0.0"    (new functionality -> minor bump)
+      app.json:       "version": "1.3.0.0"    (new functionality -> minor bump, by team convention)
       AL-Go settings: "repoVersion": "1.3"     (where used)
     Then: feature branch -> main via PR, tag, release.
 
-Feature branches never touch the version; only the release does.
+"Feature branches never touch the version" and "every merge to main is a release" are workflow choices your team can adopt for compatibility clarity — not something AppSource itself requires.
 
 ## Anti Pattern
 
diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md
index 3786d47..195c4a8 100644
--- a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md
+++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md
@@ -14,13 +14,15 @@ application-area: [all]
 `BLOB` is still a valid AL field type for arbitrary binary data, but it is
 not the right choice for storing pictures or images. The current
 recommendation is the `Media` field type for a single image, or
-`MediaSet` when a record needs several image variants (e.g. multiple
-sizes). Media/MediaSet integrate with the platform's picture control, the
-media repository, image caching, and thumbnail generation — none of which
-a plain `BLOB` field gets. A `BLOB` field storing a picture works, but it
-is the legacy approach: no caching, no thumbnail support, and no
-integration with the standard picture controls used across Business
-Central pages.
+`MediaSet` when a record needs several independent images (e.g. multiple
+product photos) — `MediaSet` is a collection of separately-imported media
+objects, each with its own identity; it does not generate resized variants
+or thumbnails on its own, and displaying more than one item still requires
+custom page handling. Media/MediaSet integrate with the platform's
+picture control and media repository, which a plain `BLOB` field does not
+— but any derived preview or thumbnail image still has to be generated
+explicitly and stored in its own field, regardless of which type holds the
+source image.
 
 `BLOB` remains the correct choice for genuinely arbitrary binary payloads
 that are not images and don't benefit from the media pipeline (e.g. a raw
@@ -36,9 +38,12 @@ See sample: `pictures-must-use-media-not-blob.good.al`.
 ## Anti Pattern
 
 A `BLOB` field named "Picture" compiles and stores the image bytes, but
-it misses the picture control integration, caching, and thumbnail
-generation that a `Media`/`MediaSet` field provides for free — the anti
-pattern is choosing `BLOB` for image storage out of habit rather than
-recognizing that the field is holding a picture, not generic binary data.
+it misses the picture control integration and media repository that a
+`Media`/`MediaSet` field provides for free — the anti pattern is choosing
+`BLOB` for image storage out of habit rather than recognizing that the
+field is holding a picture, not generic binary data. A related anti
+pattern: assuming `MediaSet` gives automatic image variants or thumbnails
+because it sounds like a collection with derived versions — it is only a
+collection of independently-imported media objects.
 
 See sample: `pictures-must-use-media-not-blob.bad.al`.
diff --git a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md
index 01f2793..e3b4efd 100644
--- a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md
+++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md
@@ -33,7 +33,12 @@ Match the table's design to its type:
    `LookupPageID`.
 3. **Subsidiary** (Item Vendor) — subsidiary to a Master/Supplemental
    table; primary key is the parent key field(s), optionally + `Line No.`;
-   Worksheet or Tabular page, always filtered by the calling page.
+   page shape depends on whether the table carries its own identity: a
+   pure parent-join table (Item Vendor) typically gets a plain List page
+   filtered by the calling page, while a subsidiary table that supplements
+   a master record with its own identity — parent key + own code, e.g.
+   Ship-to Address, Customer/Vendor Bank Account — commonly gets a
+   List+Card pair instead, for direct editing of that record.
 4. **Ledger** (Cust. Ledger Entry) — transactional record of a functional
    area; primary key `Integer` `Entry No.`, always auto-generated by
    posting, never user-editable, no free add/delete; List page as
diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al
index 34b22ec..5c455fd 100644
--- a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al
+++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al
@@ -1,26 +1,45 @@
+table 50100 "Sample Error Log Buffer"
+{
+    TableType = Temporary;
+    fields
+    {
+        field(1; "Call Duration (ms)"; Integer) { }
+        field(2; "Error Message"; Text[250]) { }
+    }
+}
+
 codeunit 50100 "Sample Error Log Writer"
 {
-    // Started via Session.StartSession so its commit is independent of the
-    // caller's transaction. Does one thing: insert the log entry, commit.
-    trigger OnRun()
+    // TableNo makes OnRun receive the Record that Session.StartSession
+    // passes to the new session. This is the only data channel into that
+    // session — there is no shared memory with the caller's instance.
+    TableNo = "Sample Error Log Buffer";
+
+    trigger OnRun(var Rec: Record "Sample Error Log Buffer")
     var
         ErrorLogEntry: Record "Sample Error Log";
     begin
         ErrorLogEntry.Init();
-        ErrorLogEntry."Call Duration (ms)" := CallDurationMs;
+        ErrorLogEntry."Call Duration (ms)" := Rec."Call Duration (ms)";
         ErrorLogEntry."Error Message" :=
-            CopyStr(ErrorMessageText, 1, MaxStrLen(ErrorLogEntry."Error Message"));
+            CopyStr(Rec."Error Message", 1, MaxStrLen(ErrorLogEntry."Error Message"));
         ErrorLogEntry.Insert(true);
         Commit();
     end;
-
-    procedure SetParameters(Duration: Integer; ErrorText: Text)
-    begin
-        CallDurationMs := Duration;
-        ErrorMessageText := ErrorText;
-    end;
-
-    var
-        CallDurationMs: Integer;
-        ErrorMessageText: Text;
+}
+
+// Caller side: populate the buffer record, then hand it to StartSession.
+// The insert-and-commit above happens inside the started session, so it
+// survives even if the caller's own transaction rolls back afterward.
+codeunit 50101 "Sample Error Log Caller Excerpt"
+{
+    procedure LogFailure(Duration: Integer; ErrorText: Text)
+    var
+        LogBuffer: Record "Sample Error Log Buffer" temporary;
+        SessionId: Integer;
+    begin
+        LogBuffer."Call Duration (ms)" := Duration;
+        LogBuffer."Error Message" := CopyStr(ErrorText, 1, MaxStrLen(LogBuffer."Error Message"));
+        Session.StartSession(SessionId, Codeunit::"Sample Error Log Writer", CompanyName, LogBuffer);
+    end;
 }
diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md
index 6939d01..e9f94ad 100644
--- a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md
+++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md
@@ -15,7 +15,7 @@ Inserting a log record inside the same transaction as the operation it logs look
 
 ## Best Practice
 
-Write any log whose purpose includes capturing failures from a transaction that is independent of the operation being logged: start an isolated session (`StartSession` on a codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. Capture duration and other telemetry values in the caller and pass them as parameters — the isolated session must not re-read state that a rollback may have erased. Logs that only record successful, committed work can safely stay in the main transaction; this pattern targets error and diagnostic logs specifically.
+Write any log whose purpose includes capturing failures from a transaction that is independent of the operation being logged: start an isolated session (`Session.StartSession` on a `TableNo`-scoped codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. `StartSession`'s only channel for getting data into that new session is its optional `Record` parameter, delivered to the target codeunit's `OnRun` trigger — a separate session gets a fresh instantiation of the codeunit, so calling a setter procedure on a local object variable before starting the session does not populate anything in the new session's instance. Capture duration and other telemetry values in the caller, place them into the `Record` passed to `StartSession`, and do the insert-and-commit entirely inside that session's own `OnRun`. Logs that only record successful, committed work can safely stay in the main transaction; this pattern targets error and diagnostic logs specifically.
 
 See sample: `log-writes-must-survive-rollback.good.al`.
 
diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md
index 75f1378..f7a05ff 100644
--- a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md
+++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md
@@ -11,7 +11,7 @@ application-area: [all]
 
 ## Description
 
-An object that is reachable from outside the app's own UI — an API page (`PageType = API`), a web-service-enabled page or query (`ServiceEnabled = true`), or a published API query — is only usable if it is also granted execute access through a permission set. When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing.
+An object that is reachable from outside the app's own UI is only usable if it is also granted execute access through a permission set. Three distinct mechanisms make an object reachable this way, and each needs to be checked on its own terms: a page or query published through the **Web Services** configuration page; a custom REST endpoint declared with `PageType = API` / `QueryType = API`; or an individual codeunit method exposed with the `[ServiceEnabled]` attribute (a method-level attribute — it does not apply to pages or queries as a property). When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing.
 
 ## Best Practice
 
diff --git a/microsoft/knowledge/style/pages-must-not-contain-business-logic.md b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md
index d113a79..f067c23 100644
--- a/microsoft/knowledge/style/pages-must-not-contain-business-logic.md
+++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md
@@ -11,7 +11,7 @@ application-area: [all]
 
 ## Description
 
-A page procedure that calculates a value and assigns it to a field, calls `Rec.Modify()` directly, or implements a business rule is an architecture violation even when it compiles. Pages are a presentation layer: they bind data to the UI and invoke actions. Calculations, validations, and record mutations belong in codeunits, where they can be tested, reused, and called consistently regardless of which page (or API, or batch job) triggers them. When logic lives on a page, it only applies when a user opens that specific page — the same business rule silently doesn't run through any other entry point.
+A page procedure that persists a business mutation directly (`Rec.Modify()` outside the standard record-bound save, or a cross-entry-point business rule implemented only in a page trigger) is an architecture violation even when it compiles: the rule only applies when a user opens that specific page, and silently doesn't run through any other entry point (API, batch job, another page). This is narrower than "no calculation may live on a page" — a presentation-specific calculation (formatting, a derived display value) is fine on the page that shows it, and a reusable data invariant commonly belongs on the table itself (a field's own validation/trigger), not forced into a codeunit merely to keep it off the page. The actual line is entry-point independence: a business operation or invariant that must hold regardless of which entry point touches the record belongs in a codeunit or the table, not solely in one page's trigger.
 
 A narrow set of patterns are conventional rather than violations:
 - A setup page reading and writing its own singleton setup record.
@@ -26,6 +26,6 @@ See sample: `pages-must-not-contain-business-logic.good.al`.
 
 ## Anti Pattern
 
-Direct calculations in a page trigger (e.g. `Rec."Total Amount" := Rec.Quantity * Rec."Unit Price"`), calls to `Rec.Modify()` from a page trigger, or business-rule validation embedded in `OnValidate`/`OnAction` instead of routed through a codeunit.
+A cross-entry-point business rule or persisted mutation implemented only in a page trigger — calling `Rec.Modify()` to save a computed business value from `OnValidate`/`OnAction`, or a validation that must hold regardless of caller, instead of routed through a codeunit or the table's own field validation. A presentation-only calculation or a table-owned field invariant is not an instance of this anti-pattern.
 
 See sample: `pages-must-not-contain-business-logic.bad.al`.
diff --git a/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md
index eb1e574..990a93b 100644
--- a/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md
+++ b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md
@@ -11,7 +11,7 @@ application-area: [all]
 
 ## Description
 
-Source folders inside an AL app should group files by the business feature or module they belong to (`src/Sales/Invoice/`, `src/NoSeries/`), not by which kind of AL object they are (`src/Tables/`, `src/Pages/`, `src/Codeunits/`). Object-type folders scatter everything belonging to one feature across half a dozen directories, so a developer picking up a feature has to jump between folders that share nothing but object type to see the whole picture. Feature folders keep a table, its pages, its codeunits, and its test setup physically together.
+Folder structure inside an AL app has no effect on compilation or runtime behavior — this is a repository-organization convention, not a platform requirement, and different projects reasonably choose differently. Grouping files by business feature or module (`src/Sales/Invoice/`, `src/NoSeries/`) rather than by AL object type (`src/Tables/`, `src/Pages/`, `src/Codeunits/`) keeps everything belonging to one feature physically together, which many teams find easier to navigate than jumping between object-type folders that share nothing but their AL object kind. Adopt this consistently on a project rather than mixing both schemes, but treat it as a team convention to apply deliberately, not a Microsoft-mandated structure.
 
 Code genuinely shared across multiple features (utility codeunits, common interfaces, shared enums) belongs in a `Common` or `Shared` folder, not duplicated per feature and not left in a catch-all root.
 
diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al
index 1a509fc..224f0dc 100644
--- a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al
+++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al
@@ -3,13 +3,18 @@ procedure PostSalesOrder_CreatesInvoice()
 var
     Customer: Record Customer;
     SalesHeader: Record "Sales Header";
+    SalesInvoiceHeader: Record "Sales Invoice Header";
+    InvoiceNo: Code[20];
 begin
-    // [GIVEN] a customer with a full posting-group chain and VAT setup
-    LibrarySales.CreateCustomerWithPostingSetup(Customer);
-    // [GIVEN] a sales order for that customer, dated explicitly
-    LibrarySales.CreateSalesOrderForCustomer(SalesHeader, Customer."No.", WorkDate());
+    // [GIVEN] a customer
+    LibrarySales.CreateCustomer(Customer);
+    // [GIVEN] a sales order for that customer
+    LibrarySales.CreateSalesOrderForCustomerNo(SalesHeader, Customer."No.");
+    SalesHeader.Validate("Posting Date", WorkDate());
+    SalesHeader.Modify(true);
     // [WHEN]
-    LibrarySales.PostSalesOrder(SalesHeader, false, true);
+    InvoiceNo := LibrarySales.PostSalesDocument(SalesHeader, false, true);
     // [THEN]
+    SalesInvoiceHeader.Get(InvoiceNo);
     Assert.RecordIsNotEmpty(SalesInvoiceHeader);
 end;
diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al
index 6866403..5079b6d 100644
--- a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al
+++ b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al
@@ -8,7 +8,6 @@ codeunit 50103 "Item Price Testing"
         ItemPriceMgt: Codeunit "Item Price Mgt.";
         Assert: Codeunit "Library Assert";
 
-    // [SCENARIO] Customer with a specific price list line gets that unit price
     [Test]
     procedure GetPrice_CustomerPrice_ReturnsUnitPrice()
     var
@@ -16,6 +15,7 @@ codeunit 50103 "Item Price Testing"
         Item: Record Item;
         UnitPrice, LineDiscPct: Decimal;
     begin
+        // [SCENARIO] Customer with a specific price list line gets that unit price
         // [GIVEN] a customer with a price list line at 100 LCY
         LibrarySales.CreateCustomerWithPrice(Customer, Item, '', 100);
         // [WHEN]
diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.md b/microsoft/knowledge/testing/test-feature-scenario-tags.md
index 5842bee..f9a2754 100644
--- a/microsoft/knowledge/testing/test-feature-scenario-tags.md
+++ b/microsoft/knowledge/testing/test-feature-scenario-tags.md
@@ -15,7 +15,7 @@ Test codeunits are easier to trust and to review when they carry a four-level co
 
 ## Best Practice
 
-Put `[FEATURE]` as a comment before the codeunit's opening brace, naming the domain rather than the object. Put `[SCENARIO]` immediately above each `[Test]` attribute, describing the scenario in business language that complements — not duplicates — the procedure name. Inside the body, mark the precondition setup as `[GIVEN]`, the single action under test as `[WHEN]`, and the assertions as `[THEN]`. The procedure name stays the machine-readable identity shown in test-runner output; the `[SCENARIO]` comment stays the human-readable one. Neither replaces the other.
+Put `[FEATURE]` as a comment before the codeunit's opening brace, naming the domain rather than the object — Microsoft's own guidance allows setting it once for the whole codeunit, inherited by every test in it. Put `[SCENARIO]`, matching the current BCApps corpus, as the first comment inside each test procedure's body (after `begin`), describing the scenario in business language that complements — not duplicates — the procedure name, followed by `[GIVEN]` marking the precondition setup, `[WHEN]` marking the single action under test, and `[THEN]` marking the assertions. The procedure name stays the machine-readable identity shown in test-runner output; the `[SCENARIO]` comment stays the human-readable one. Neither replaces the other.
 
 See sample: `test-feature-scenario-tags.good.al`.
 
diff --git a/microsoft/knowledge/testing/test-one-when-per-test.md b/microsoft/knowledge/testing/test-one-when-per-test.md
index 8aa6ca8..d52a97a 100644
--- a/microsoft/knowledge/testing/test-one-when-per-test.md
+++ b/microsoft/knowledge/testing/test-one-when-per-test.md
@@ -11,7 +11,7 @@ application-area: [all]
 
 ## Description
 
-Each test procedure should contain exactly one `[WHEN]` block: one action that triggers the behaviour under test. A test with multiple WHENs — "do A, then do B, then check C" — is two or more tests in disguise. Splitting them gives failure isolation (a failing test points at one action, not an ambiguous sequence) and keeps each test readable as a single, falsifiable claim. A precondition action, such as posting a document so a ledger entry exists to assert against, belongs in `[GIVEN]`; only the action actually being asserted belongs in `[WHEN]`.
+This is a testing-design practice, not a BC platform requirement — no AL API enforces it, and it should not gate a change the way a platform-contradicted claim would. Each test procedure should contain exactly one `[WHEN]` block: one action that triggers the behaviour under test. A test with multiple WHENs — "do A, then do B, then check C" — is two or more tests in disguise. Splitting them gives failure isolation (a failing test points at one action, not an ambiguous sequence) and keeps each test readable as a single, falsifiable claim. A precondition action, such as posting a document so a ledger entry exists to assert against, belongs in `[GIVEN]`; only the action actually being asserted belongs in `[WHEN]`.
 
 ## Best Practice
 
diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.md b/microsoft/knowledge/testing/ui-test-codeunit-naming.md
index 4166cd7..1cc332a 100644
--- a/microsoft/knowledge/testing/ui-test-codeunit-naming.md
+++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.md
@@ -1,26 +1,26 @@
 ---
 bc-version: [all]
 domain: testing
-keywords: [ui-test, testpage, naming, suffix, codeunit, page-testing]
+keywords: [ui-test, testpage, naming, suffix, codeunit, page-testing, team-convention]
 technologies: [al]
 countries: [w1]
 application-area: [all]
 ---
 
-# Suffix UI-layer test codeunits with _UT and never mix layers in one codeunit
+# Separate UI-layer and logic-layer tests into different codeunits
 
 ## Description
 
-A test codeunit that drives pages through `TestPage` — opening pages, reading FactBox parts, triggering field `OnValidate` through the page — is testing a different layer than a codeunit that calls business-logic procedures directly. Readers need to know which layer a given test exercises without opening it, and a single codeunit that mixes both kinds of test hides that distinction: a failure could mean the logic broke, the page broke, or both.
+A test codeunit that drives pages through `TestPage` — opening pages, reading FactBox parts, triggering field `OnValidate` through the page — is testing a different layer than a codeunit that calls business-logic procedures directly. Readers need to know which layer a given test exercises without opening it, and a single codeunit that mixes both kinds of test hides that distinction: a failure could mean the logic broke, the page broke, or both. The `_UT` suffix and adjacent-object-ID pairing below are one team's naming convention for making that split visible, not a BCApps-wide naming standard — BCApps itself uses `UT` for unit tests generally, not specifically to mean "UI layer," and does not treat adjacent object IDs as a semantic pairing mechanism. Apply the suffix only on a project that has explicitly adopted this convention.
 
 ## Best Practice
 
-Give any test codeunit that uses `TestPage` a `_UT` (Unit Test — UI layer) suffix, and keep it free of tests that call codeunit/table procedures directly. Keep the corresponding logic-only codeunit unsuffixed. Allocate the two codeunits adjacent object IDs so their relationship is visible in the object list.
+Keep UI-layer (`TestPage`-driven) and logic-layer tests in separate codeunits regardless of naming. Projects that adopt a `_UT`-style suffix convention should apply it consistently to every UI-layer test codeunit, keep the corresponding logic-only codeunit unsuffixed, and document the convention where the team's other naming rules live.
 
 See sample: `ui-test-codeunit-naming.good.al`.
 
 ## Anti Pattern
 
-A codeunit named without the `_UT` suffix that nonetheless contains `TestPage` calls, or — worse — one codeunit that mixes a direct logic-call test and a `TestPage`-driven test side by side. Either way, the codeunit's name no longer tells a reader which layer a failing test actually broke.
+One codeunit that mixes a direct logic-call test and a `TestPage`-driven test side by side — a failing test no longer tells a reader which layer actually broke. On a project that has adopted the `_UT` convention, a UI-layer codeunit missing the suffix is also an instance of this anti-pattern; on a project that has not adopted it, the suffix itself is not required.
 
 See sample: `ui-test-codeunit-naming.bad.al`.
diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md
index 5f925d2..b579f69 100644
--- a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md
+++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md
@@ -32,8 +32,14 @@ Match the page's design to its type:
 - **RoleCenter** — tailored home page for a role; named role + `Role
   Center`; links to List pages, shows Cues/Activities.
 - **Card** — view/edit one record; named table + `Card`; FastTabs only,
-  first FastTab named `General`. Requires a single-field primary key — a
-  multi-field key needs a List/Worksheet/Tabular page instead.
+  first FastTab named `General`. A single-field primary key is typical,
+  but not a hard requirement: a subsidiary table that supplements a
+  master record with its own identity (parent key + own code — Ship-to
+  Address, Customer/Vendor Bank Account) commonly gets its own Card page
+  over a composite key too. Treat the key shape as a contextual signal,
+  not a mandatory constraint — a composite-key table with no such
+  supplementing relationship to a master record is the actual signal a
+  List/Worksheet/Tabular page fits better.
 - **List** — view multiple records, also the lookup/drilldown surface;
   named table + `List` if read-only, or the plural table name if
   editable; primary-key fields shown left-most; `CardPageID` must point
@@ -64,10 +70,11 @@ See sample: `page-design-must-match-bc-page-type-conventions.good.al`.
 
 ## Anti Pattern
 
-A page that mixes conventions from two types — for example, a "Card"
-page built on a table with a two-field primary key, or a "List" page
-with no `CardPageID` even though a Card page exists for the same table —
-signals a design step was skipped, not a stylistic choice. Also watch
+A page that mixes conventions from two types — for example, a "List"
+page with no `CardPageID` even though a Card page exists for the same
+table — signals a design step was skipped, not a stylistic choice. A
+Card page over a composite-key table is not automatically this anti
+pattern; check whether the table supplements a master record first. Also watch
 for: a Worksheet or List page showing primary-key fields it shouldn't (or
 hiding them when it should show them), and a page with no
 `UsageCategory` set, which makes it invisible to Tell Me search even
diff --git a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md
index 3bd7e97..f13c4b8 100644
--- a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md
+++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md
@@ -26,3 +26,7 @@ See sample: `upgrade-tag-logic-must-not-nest-deeply.good.al`.
 Nesting the tag check, a record loop, and a multi-branch business condition inside one procedure. Split the buried business condition into its own, separately tagged upgrade step instead.
 
 See sample: `upgrade-tag-logic-must-not-nest-deeply.bad.al`.
+
+## Source
+
+Microsoft's own "Upgrading Extensions" guidance, Design considerations: "Keep tags simple by limiting nesting tags to two levels. Complicated if statements can lead to problems." — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-upgrading-extensions#using-upgrade-tags-to-control-upgrade-code
diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al
index 35af72b..e94f7f2 100644
--- a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al
+++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al
@@ -6,6 +6,8 @@ page 50102 "Vendor Contact Info API"
     APIVersion = 'v1.0';
     SourceTable = Vendor;
     DelayedInsert = true;
+    InsertAllowed = false;
+    DeleteAllowed = false;
 
     layout
     {
diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md
index 5289fad..a06fa35 100644
--- a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md
+++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md
@@ -11,11 +11,11 @@ application-area: [all]
 
 ## Description
 
-A general-purpose API page that exposes many fields should not be widened to allow writes on one additional field. A `PageType = API` page consumed by an external integration, an automation agent, or a partner system carries the same risk regardless of caller: a write-enabled page with no per-field restriction is a wide-open surface. The most common real-world shape of this problem is not a page that started narrow and got widened — it is a page that was never restricted at all: with `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` left at their defaults and no `Editable = false` on any field, every field on the source table — including identity fields and financially significant ones — is fully writable, with nothing marking that as deliberate.
+A general-purpose API page that exposes many fields should not be widened to allow writes on one additional field. A `PageType = API` page consumed by an external integration, an automation agent, or a partner system carries the same risk regardless of caller: a write-enabled page with no per-field restriction is a wide-open surface. Least privilege has to cover both dimensions of exposure: which fields are on the page, and which operations the page allows. Only a field actually placed on the page is reachable at all — but a page that includes many fields, with `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` left at their defaults and no `Editable = false` on most of them, leaves every one of those included fields — identity fields and financially significant ones among them — fully writable, with nothing marking that as deliberate. Restricting fields alone is not enough either: a page with only two fields on it can still let a caller insert brand-new records or delete existing ones if `InsertAllowed`/`DeleteAllowed` are left at their true defaults (both `true`).
 
 ## Best Practice
 
-Create a separate, minimal API page that exposes only the key and the specific field the consumer needs to write, with everything else `Editable = false` or simply absent from the page.
+Create a separate, minimal API page that exposes only the key and the specific field the consumer needs to write, with everything else `Editable = false` or simply absent from the page — and set `InsertAllowed`/`DeleteAllowed` to `false` unless the consumer's use case genuinely needs to create or delete records through that page.
 
 See sample: `api-page-least-privilege-write-access.good.al`.
 
diff --git a/microsoft/skills/review/al-data-modeling-review.md b/microsoft/skills/review/al-data-modeling-review.md
index 3fca8ec..12d555c 100644
--- a/microsoft/skills/review/al-data-modeling-review.md
+++ b/microsoft/skills/review/al-data-modeling-review.md
@@ -46,6 +46,8 @@ A file enters the candidate worklist when its `keywords` intersect the extracted
 The following targeted checks cover every current `data-modeling` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action.
 
 - A `* Setup` table or its page changes singleton structure, uses a nonblank or generated key, permits insert/delete, uses a List page, or does not ensure the blank-keyed row exists — `setup-table-is-a-singleton`.
+- A new field is typed `Media`, `MediaSet`, or `BLOB` and the field's caption/name suggests a picture or image — `pictures-must-use-media-not-blob`.
+- A new or extended table declares its `keys` block, primary-key field list, or naming suffix (`Ledger Entry`, `Journal Line`, `Header`/`Line`, `Setup`) — `table-design-must-match-bc-table-type-conventions`.
 - 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`.
 - 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`.
diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md
index 56bc0fe..98213ba 100644
--- a/microsoft/skills/review/al-error-handling-review.md
+++ b/microsoft/skills/review/al-error-handling-review.md
@@ -47,6 +47,7 @@ A file enters the candidate worklist when its `keywords` intersect the extracted
 The following targeted checks cover every current `error-handling` article:
 
 - `[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 calls `Session.StartSession` from within error/duration logging around a web-service call, background job, or other operation expected to fail — `log-writes-must-survive-rollback`.
 - Developer-only invariant text is raised with default client visibility, or a user-actionable validation is hidden as `ErrorType::Internal` — `errortype-internal-vs-client-for-diagnostics`.
 - `FieldError` receives a complete capitalized sentence, repeats the field caption/value, or ends the predicate with punctuation — `fielderror-default-message-logic`.
 - An unguarded `FieldError` is used as though it performed a comparison, or `TestField` is forced onto a complex rule needing a tailored predicate — `fielderror-vs-testfield`.
diff --git a/microsoft/skills/review/al-security-review.md b/microsoft/skills/review/al-security-review.md
index 00e8d10..e5390fb 100644
--- a/microsoft/skills/review/al-security-review.md
+++ b/microsoft/skills/review/al-security-review.md
@@ -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 permission sets, codeunits handling authentication or authorization, objects touching `Isolated Storage`, `OAuth2` flows, web service endpoints, API pages, event publishers, and RecordRef helpers.
 - The changed procedures and triggers, weighted toward those that call `HttpClient`, validate or compose URLs, write to telemetry, read or write secrets, unwrap SecretText, manipulate record-level security, expose var Boolean guard parameters, or bypass the permission model (for example, `RecordRef.Open`, `Record.WritePermission`, direct table access from a non-owning app).
-- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`).
+- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`, `ServiceEnabled`, `PageType = API`, `QueryType = API`, `permissionset`, `Web Services`).
 
 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.
 
diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md
index c96ac83..057bca0 100644
--- a/microsoft/skills/review/al-testing-review.md
+++ b/microsoft/skills/review/al-testing-review.md
@@ -46,6 +46,9 @@ A file enters the candidate worklist when its `keywords` intersect the extracted
 The following targeted checks cover every current `testing` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action.
 
 - A method in a `Subtype = Test` codeunit adds or changes `[TransactionModel(...)]`, exercises code that calls `Commit` under `AutoRollback`, defaults broadly to `AutoCommit`, or chooses `None` for a writing test — `transactionmodel-attribute-governs-test-transactions`.
+- A test procedure or comment adds `[FEATURE]`/`[SCENARIO]`/`[GIVEN]`/`[WHEN]`/`[THEN]` tags — `test-feature-scenario-tags`.
+- A test codeunit calls `TestPage` methods (`OpenNew`, `OpenView`, `OpenEdit`) — `ui-test-codeunit-naming`.
+- A `[GIVEN]`-tagged setup precedes a posting call or report execution — `given-blocks-must-cover-full-precondition-chain`.
 - An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it.
 - A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`.
 - Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`.
diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md
index 8c35f49..f665543 100644
--- a/microsoft/skills/review/al-ui-review.md
+++ b/microsoft/skills/review/al-ui-review.md
@@ -41,7 +41,7 @@ Narrow the relevant files to the subset that applies to the changes under review
 
 - **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`, and to JavaScript/CSS/HTML that implements a control add-in's rendering or Business Central communication. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files.
 - For each relevant knowledge file, compute overlap against changed page declarations and control add-in files, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, `ShowCaption`, `InstructionalText`, `GridLayout`, `Style`, `StyleExpr`, promoted action definitions, field importance, page background tasks, DOM creation, ARIA attributes, keyboard/focus handlers, packaged-resource AJAX, and calls from JavaScript into AL.
-- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions).
+- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`).
 
 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 page element. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.