Merge pull request #75 from Curabis/rule/edison-audit-4-sharpenings-bundled

[BCQuality] 4 Edison-driven sharpenings (bundled): API least-privilege, naming, test-exceptions, table-type edge case

custom/knowledge/architecture/table-design-must-match-bc-table-type-conventions.md

@@ -89,6 +89,7 @@ decision — key shape, naming suffix, and which pages must exist."
 - **Pages:** one page, same name as the table, primary key field not shown.
 - **Record instantiation:** the page's `OnOpenPage` trigger — not the table — creates the singleton the first time it is opened; it is never assumed to pre-exist. Standard shape: `Rec.Reset(); if not Rec.Get() then begin Rec.Init(); Rec.Insert(); end;` (the `Reset()` clears any stale filter before the `Get()`, since the blank `Code` PK would otherwise be vulnerable to one). Verified against Base App W1: `General Ledger Setup` and `Sales & Receivables Setup` both use this exact pattern. A Setup page that omits this and assumes the record exists fails at runtime on first open in a fresh company.
 - **Caveat:** a table with "Setup" in its name that holds more than one record follows the Subsidiary-table rules instead — the name alone is not proof of type.
+- **When the object definition alone can't resolve the caveat:** a "Setup"-named table with a real business-field primary key (not a blank `Code[10]` "Primary Key") and no associated page looks like a violation of both the Setup and Subsidiary shapes at once. Confirming which one it actually is requires checking real row cardinality (does the table ever hold more than one record in practice?) or tracing the table's other call sites — not something the table/page definitions alone settle. When source can't resolve it, say so explicitly rather than forcing a classification (Edison eval 2026-08-13, Wareco @ a2fc8ff8, `ForsendelsesSetup.Table.al`).
 
 ## Review Checklist
 

custom/knowledge/mcp/api-page-least-privilege-write-access.md

@@ -1,7 +1,7 @@
 ---
 bc-version: [all]
 domain: mcp
-keywords: [api-page, least-privilege, write-access, odata, security]
+keywords: [api-page, least-privilege, write-access, odata, security, external-api, identity-fields]
 technologies: [al]
 countries: [w1]
 application-area: [all]
@@ -12,12 +12,45 @@ application-area: [all]
 
 A general-purpose API page that exposes many fields should not be widened to allow writes on a single additional field. Instead, create a dedicated minimal API page that exposes only the fields the consumer needs to read and write. This limits the blast radius of any agent or integration mistake.
 
+This applies beyond CURABIS's own MCP tooling — it's a general AL API-page design concern. Any `PageType = API` page consumed by an external integration, a Power BI dataset, or a partner system carries the same risk: a write-enabled page with no per-field restriction is a wide-open surface regardless of who or what is calling it.
+
 ## Why This Matters
 
-An MCP agent operates with the permissions of its service identity, not an individual user. A page that allows writing to many fields gives the agent broad power that is hard to audit and easy to misuse. A dedicated page with one writable field makes the intent explicit and the surface area auditable.
+An MCP agent operates with the permissions of its service identity, not an individual user — but the same argument holds for any external consumer of an API page. A page that allows writing to many fields gives the caller broad power that is hard to audit and easy to misuse, whether the caller is CURABIS's own agent, a customer's integration, or a Power Platform flow. A dedicated page with one writable field (or explicit `Editable = false` on everything else) makes the intent explicit and the surface area auditable.
 
 ## Pattern to Avoid
 
+The most common real-world shape of this violation isn't a page that started narrow and got widened — it's a page that was **never restricted at all**. A `PageType = API` page with `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` left at their defaults, and no `Editable = false` on any field, exposes every field on the source table — including identity fields (`No.`, `Document Type`) and financially significant ones (`VAT Bus. Posting Group`, `Amount Including VAT`) — as fully writable, with nothing marking that as deliberate:
+
+    // WRONG: no restriction declared anywhere on a write-capable API page —
+    // ~90 fields, including VAT/posting fields and the record's own key,
+    // are all fully writable by default, with nothing marking that as intentional.
+    page 50100 "Some Document Header API"
+    {
+        PageType = API;
+        APIPublisher = 'contoso';
+        APIGroup = 'docs';
+        APIVersion = 'v1.0';
+        SourceTable = "Some Document Header";
+        // no InsertAllowed/ModifyAllowed/DeleteAllowed override, no Editable = false anywhere
+        layout
+        {
+            area(content)
+            {
+                repeater(GroupName)
+                {
+                    field(no; Rec."No.") { }
+                    field(documentType; Rec."Document Type") { }
+                    field(amountIncludingVAT; Rec."Amount Including VAT") { }
+                    field(vatBusPostingGroup; Rec."VAT Bus. Posting Group") { }
+                    // ... ~85 more fields, none marked Editable = false
+                }
+            }
+        }
+    }
+
+The narrower "widened by one field" shape below is also real, just less common in practice than the above:
+
     // WRONG: General page widened with write access to one field
     // Now the agent can accidentally (or intentionally) write to all other fields too
     field(status; Rec.Status) { }                          // should be read-only

custom/knowledge/style/variable-names-must-be-semantically-descriptive.md

@@ -28,6 +28,28 @@ subtracted), name them for their distinct roles in that calculation
 (`OriginalAmount` / `AdjustedAmount`), not for their shared type
 (`Amt1` / `Amt2`).
 
+The same failure shows up in a second, more common shape that's easy to
+miss because it doesn't look like an abbreviation: a **real record type
+name plus a letter suffix** — `ItemA`/`ItemB`/`ItemC`, `VendorA`/`VendorB`.
+This is most common in test fixtures, where two records of the same type
+play distinct roles the letter suffix erases (e.g. one vendor has a price
+configured, the other doesn't and the test expects a zero-price lookup to
+fall through to it) — a reader has to go read the test body to learn which
+letter means what, exactly the lookup cost this rule exists to avoid. Name
+them for the role: `PricedVendor`/`UnpricedVendor`, `ScrapItem`/`RegularItem`,
+not for their shared type plus an arbitrary letter.
+
+**Read the sibling declarations before flagging a type+letter name.** A
+single-letter suffix on a real type name isn't always the anti-pattern
+above — it can be one member of a deliberate, self-consistent naming
+family that happens to use single letters for a real reason (a country or
+region code, a variant identifier). `ItemN` sitting next to `ItemDk`,
+`ItemSE`, `ItemFI` in the same `var` section isn't an unexplained letter —
+it's Norway's country code, following the exact same pattern as its
+siblings. Flagging `ItemN` in isolation, without reading what else is
+declared alongside it, produces a false positive; the letter/suffix only
+counts as unexplained if nothing nearby explains it.
+
 **Exception:** short-lived variables in a handful of idiomatic, universally
 recognized roles are accepted single-letter, because their entire meaning
 is visible in the few lines that declare and use them:
@@ -78,3 +100,19 @@ A reviewer reading `Amt1 := Amt2 - TempX;` cannot tell what this line is
 computing without opening the variable declarations and searching for every
 other assignment to `Amt2` and `TempX` first. The same line as
 `AmountInclVAT := AmountExclVAT - DiscountAmount;` needs no further lookup.
+
+```al
+// Same failure, real-type-name shape — common in test fixtures.
+var
+    VendorA: Record Vendor;
+    VendorB: Record Vendor;
+...
+LibraryPurchase.CreateVendor(VendorA);
+CreateVendorPrice(VendorA, Item, 10);
+LibraryPurchase.CreateVendor(VendorB); // no price created for VendorB
+Assert.AreEqual(0, PriceMgt.GetVendorPrice(VendorB."No.", Item."No."), '');
+```
+
+`VendorA`/`VendorB` tell the reader nothing about why the test needs two
+vendors. `PricedVendor`/`UnpricedVendor` would make the assertion make
+sense without reading the setup lines above it.

custom/knowledge/testing/test-one-when-per-test.md

@@ -89,6 +89,33 @@ Unit-level tests keep the strict one-WHEN rule without exception. (Edison
 eval 2026-07-02, Jernpladsen @ b7656b1: five deliberate round-labelled flow
 procedures in SVPartialFlowTests — the rule previously gave no verdict.)
 
+## Defect-then-fix regression tests — a second, narrower exception
+
+A test that reproduces a specific stale/broken state and then verifies a
+subsequent action corrects it (`RecalcRestoresStaleDiscountAfterPick`: pick
+creates the stale state, recalc is the fix under test) is not the same
+shape as an unrelated-action test, even though its `[THEN]` assertions
+decompose cleanly per step — decomposing cleanly is expected here, not a
+sign of two unrelated tests. The three flow-test conditions above are the
+wrong fit for this case: the name doesn't need to declare a multi-round
+"flow," and there is no natural "Runde 1/2" framing for "create the broken
+state, then fix it." This shape is permitted when:
+
+1. The second `[WHEN]` cannot be meaningfully tested without the first —
+   the fix being verified only has an effect on the specific stale state
+   the first action produced, so splitting would require re-running the
+   first action inside a second test's `[GIVEN]` anyway, testing nothing
+   new.
+2. The procedure name communicates the before/after relationship (a
+   defect symptom and its correction), even without the word "flow."
+
+Applying flow-test criterion 3 ("assertions decompose cleanly → split") to
+this shape would have been a false positive (Edison eval 2026-08-13,
+Wareco @ a2fc8ff8, `SalesOrderAmountAfterPickTest.RecalcRestoresStaleDiscountAfterPick`)
+— clean decomposition is exactly what a defect-then-fix test's assertions
+are supposed to do at each step, not evidence the steps belong in separate
+tests.
+
 ## Naming implication
 
 The procedure name should make the single WHEN self-evident.