18 more AL/BC patterns: data-modeling, testing, style, security, error-handling, ui, upgrade, web-services, appsource

[Michael Dieringer (MichaelDieringer)]

Summary

Second batch from CURABIS ApS — 18 more knowledge articles generalized from patterns observed across real AppSource/PTE Business Central development, follow-up to #156. Same format (6-key frontmatter, Description/Best Practice/Anti Pattern, sibling .good.al/.bad.al samples where a code contrast helps, no fenced code in the .md), targeted at existing Microsoft-owned domains rather than /community/.

• data-modeling (3): WorkDate must never be assigned in app code, Media/MediaSet over BLOB for pictures, the nine BC table-type design conventions (Master/Supplemental/Subsidiary/Ledger/Register/Journal/Document/Document History/Setup).
• testing (5): app-specific BCPT scenarios instead of generic samples, FEATURE/SCENARIO/GIVEN/WHEN/THEN tagging, _UT suffix for UI-layer test codeunits, GIVEN-block precondition completeness for posting/report scenarios, one WHEN per test (with flow-test and defect-then-fix exceptions).
• style (3): pages must not contain business logic, organize source by feature not object type, comments must not restate what the code already shows.
• security (1): every exposed object (API/web-service) must sit in a permission set.
• error-handling (2): log writes must survive rollback via an isolated session, defensive vs. offensive code should match actual blast radius.
• ui (1): the BC page-type design taxonomy (RoleCenter/Card/List/Worksheet/Document/etc.).
• upgrade (1): upgrade-tag guard logic must not nest deeply.
• web-services (1): give API pages least-privilege write access — a separate minimal page per writable field rather than widening a general-purpose one.
• appsource (1): app version bumps should be a deliberate major/minor decision, not left to automatic build/revision increments.

Checked against the current microsoft/knowledge/ corpus before opening — four originally-drafted articles were dropped as duplicates or substantial overlaps of existing files (xrec-is-a-before-image-only-in-some-triggers.md, use-library-codeunits-for-test-fixtures.md + asserterror-needs-expectederror-and-code.md, choose-telemetry-scope-by-audience.md, compose-permission-sets-with-included-sets.md).

Test plan

• CI frontmatter/format validation passes
• Domain-owning reviewers confirm placement and accuracy per file

🤖 Generated with Claude Code

[Jesper Schulz-Wedde (JesperSchulz)]

There are useful principles in this batch, but it is not ready for the Microsoft layer yet. I verified several concrete platform/source contradictions inline.

There are also two cross-cutting blockers:

1. None of the 18 articles is wired into the corresponding microsoft/skills/review/al-*-review.md worklist. The current contracts say their targeted checks cover every current article, but no changed skill names any new slug and several domains' relevance gates would return not-applicable before considering these topics. Please add explicit signals/targeted checks and coordinate with #155's coverage contract so agents can reliably consume the knowledge.
2. Several entries are CURABIS house conventions presented as universal Microsoft requirements: feature-vs-object folder layout, exactly one WHEN, _UT meaning UI-layer tests, adjacent object IDs, and a hard two-level nesting limit. These can be useful team guidance, but they need authoritative BC-wide grounding and careful scoping before an agent may emit gating findings from the Microsoft layer; otherwise they belong in a custom/community layer.

The structural validators pass (with two keyword-count warnings), but they do not compile the AL fixtures or validate the claims. The CLA is still pending; because this is generalized from CURABIS work, the contributor must use the applicable employer/company declaration and confirm authorization to contribute it.

[Michael Dieringer (MichaelDieringer)]

@microsoft-github-policy-service agree [company="CURABIS ApS"]

[Michael Dieringer (MichaelDieringer)]

@microsoft-github-policy-service agree company="CURABIS ApS"

[Jesper Schulz-Wedde (JesperSchulz)]

Follow-up after 81845a5: the version ordering, API restrictions, FEATURE/SCENARIO placement, _UT/object-ID claims, composite-key Card guidance, and several earlier fixture issues are improved. The remaining correctness issues are inline below.

The deterministic testing worklist is also incomplete: only a subset of the 18 new rules has an explicit cue, and several cues recognize compliant code rather than the anti-pattern. Please ensure every new rule can be selected from the changed-code signals that indicate a possible violation. The invented API comment below applies to all new good fixtures; they should be compiled against the declared dependencies rather than treated as pseudocode.

[Michael Dieringer (MichaelDieringer)]

Jesper Schulz-Wedde (@JesperSchulz) Addressed this round in 30abf07 (pushed 2026-09-08): the 9 file-level issues above (individual replies inline), plus two more invented-API fixtures found in the same sweep that weren't in your review (given-blocks-must-cover-full-precondition-chain.bad.al, ui-test-codeunit-naming.good.al/.bad.al — noted inline on the price-fixture thread). Also added worklist cues for the 12 of 18 new rules that had none (al-appsource-review.md, al-data-modeling-review.md, al-error-handling-review.md, al-security-review.md, al-style-review.md ×3, al-testing-review.md ×2, al-ui-review.md, al-upgrade-review.md, al-web-services-review.md), and fixed test-feature-scenario-tags' own cue, which only matched the compliant tagged shape instead of the anti-pattern. Ready for another look whenever you have time.

[Jesper Schulz-Wedde (JesperSchulz)]

Thank you for another substantial round of corrections. Most of the previous findings are now resolved, and this PR is much closer.

During verification of the updated examples, we found a small number of correctness issues that the structural validators cannot detect—for example, AL that does not compile, test setup that never activates the data it intends to exercise, or guidance that differs from the current platform behavior. We recognize that another round is inconvenient, and we are not trying to prolong the review. These articles will be consumed as authoritative instructions by automated agents, so an incorrect “good” fixture or overly broad rule can be repeated across many future changes. We have therefore limited this round to the material issues below and omitted optional editorial cleanup.

[Jesper Schulz-Wedde (JesperSchulz)]

The six open correctness threads from the previous pass still apply: mandatory API-page entity names, the nonexistent Sales Line."Total Amount" field, activation of price lists used by the good tests, SOAP-vs-OData wording for published codeunits, anti-pattern-first routing for rollback logging, and the current PageType enum/catalogue. I found four additional correctness gaps inline.

The exact-tree validators pass locally (0 errors, two existing keyword-count warnings; 318-article deterministic index; 34 fixtures across 17 domains), and all three reported checks are green. Those checks are structural and do not catch these AL/API/behavioral defects. The PR is also currently conflicting with main in microsoft/skills/review/al-style-review.md. Please address the ten focused items and rebase once; no broader editorial cleanup is requested.

[Jesper Schulz-Wedde (JesperSchulz)]

This good fixture is not behaviorally equivalent to the bad one: it removes the Discount % = 0 and nonblank posting-group guards and updates every customer. Microsoft's guidance also explicitly says to retain extra safety checks to avoid data corruption. Flatten by extracting the guarded update into a helper/early-exit shape while preserving both conditions; do not remove the conditions or turn each safety condition into an independently tagged migration.

[Michael Dieringer (MichaelDieringer)]

Thanks Jesper. Addressed all ten focused items from the 2026-09-15 re-review.

The six carried-over threads:

• api-page-least-privilege-write-access fixtures: added the mandatory EntityName/EntitySetName properties (AL0485).
• pages-must-not-contain-business-logic fixtures: Sales Line has no "Total Amount" field — replaced with the real "Line Amount" (field 103).
• test-feature-scenario-tags.good.al and test-one-when-per-test.good.al: CreatePriceHeader leaves the list in Draft status, which price calculation ignores. Added Validate(Status, Active) + Modify before the sales line that depends on it — verified the Status field/enum against PriceListHeader.Table.al/PriceStatus.Enum.al in the BCApps clone.
• exposed-objects-must-be-in-a-permission-set.md: corrected — a published codeunit is a SOAP endpoint (deprecated), not OData; Page/Query are the OData object types. Now recommends API pages/queries for new integrations.
• al-error-handling-review.md: the cue selected on Session.StartSession, which only appears in the compliant fix — the bad fixture could never be worklisted. Re-cued on the actual risk shape (log insert around a failed TryFunction/GetLastError* path, then raise/propagate), with StartSession as a compliant discriminator instead.
• page-design-must-match-bc-page-type-conventions.md: fixed Navigate → NavigatePage, and noted the type list is a selected subset (not an exhaustive catalogue — PromptDialog/ConfigurationDialog/UserControlHost/XmlPort exist but are out of this article's scope).

The four new gaps:

• release-must-update-app-version.md: corrected — id is the app's stable identity, version identifies a release/code-state of it (was backwards).
• defensive-vs-offensive-code-must-match-blast-radius.good.al: added the missing else branch — a failed Customer.Get() was leaving the field at its prior/default value instead of the explicit chosen fallback the article claims to demonstrate.
• bcpt-scenarios-must-be-app-specific.good.al: InitTest and both measured sections were empty/comment-only. Filled in a real, self-contained header+line creation path so the fixture actually measures something.
• upgrade-tag-logic-must-not-nest-deeply.good.al: the flattened version had silently dropped both safety conditions from the bad fixture (Discount % = 0, nonblank posting group) instead of just removing nesting. Extracted the guarded update into a helper with both conditions preserved as early exits.

Also converted this PR's remaining plain-backtick sample references (16 articles) to the READ-convention markdown-link form, same fix already made on #156/#158.

Rebased onto current main (conflicts in al-ui-review.md, al-style-review.md, al-upgrade-review.md against merged upstream PRs — all additive, both sides' cues retained). All local validators pass: frontmatter (0 errors, the same two pre-existing keyword-count warnings), knowledge-index, knowledge-retrieval (351 articles / 589 samples), review-fixtures (108 cases / 20 leaf domains), skill-index.

[Jesper Schulz-Wedde (JesperSchulz)]

Four merge-critical issues remain:

1. pages-must-not-contain-business-logic.good.al directly assigns Sales Line."Line Amount" and calls Modify(), bypassing validation and related amount/discount/VAT maintenance. The authoritative good sample can persist inconsistent Sales Lines; use a safe custom example or a supported validation/business routine.

2. bcpt-scenarios-must-be-app-specific.good.al depends on Customer.FindFirst() and a session-local counter for persistent keys. It fails in an empty company and collides on later BCPT sessions, so it is neither self-contained nor repeatable.

3. upgrade-tag-logic-must-not-nest-deeply.md treats business-data guards inside an upgrade loop as evidence for separate tagged upgrades. Microsoft's cited guidance requires such safety checks and demonstrates them inside the loop. Restrict this rule to nested tag gates or genuinely distinct migrations, excluding safety guards within one tagged migration.

4. table-design-must-match-bc-table-type-conventions.md routes essentially every new keyed table into nine non-exhaustive archetypes. Valid buffers, queues, logs, mappings, and working tables fall outside that list, creating systematic harmful redesign findings. Apply it only when context establishes one of the listed archetypes.

The prior API metadata, price activation, protocol wording, rollback routing, PageType, manifest, and fallback blockers otherwise appear resolved.

[Jesper Schulz-Wedde (JesperSchulz)]

There has been significant progress here. I closed nine resolved or superseded discussions; the remaining review is now a bounded set of four concrete safety and precision fixes.

[Michael Dieringer (MichaelDieringer)]

Thanks Jesper. All four fixed:

1. `pages-must-not-contain-business-logic`: replaced the real `Sales Line` example with a self-contained `"Sample Order Line"` table and switched the codeunit to `Validate()`/`Modify(true)` instead of a raw field assignment + `Modify()` — the "good" sample no longer bypasses a real document table's validation cascade regardless of which object it lives in.
2. `bcpt-scenarios-must-be-app-specific`: no longer assumes a customer exists (`Customer.FindFirst()` now falls back to creating one), and keys are generated from `CreateGuid()` instead of a session-local counter, so the scenario is self-contained and safe to run concurrently/repeatedly.
3. `upgrade-tag-logic-must-not-nest-deeply`: you're right, and this was a real conflation on our part — confirmed by re-reading Microsoft's own "Upgrading Extensions" page directly. "Keep tags simple by limiting nesting tags to two levels" is about nesting tag checks, and the page's own worked example nests a record loop with two business-data safety conditions inside one tagged procedure, matching the separate design guidance that explicitly requires such safety checks. Rewrote the Description/Best Practice/Anti Pattern to scope the rule to actual tag-check nesting or migrations blended under one tag, and rewrote both fixtures accordingly: `good.al` now nests safety conditions inside one migration's own loop (matching Microsoft's example) plus a second, genuinely separate migration as its own flat procedure; `bad.al` now shows the real anti-pattern — one tag's check nested inside another's guarded body.
4. `table-design-must-match-bc-table-type-conventions`: added an explicit scope note that the nine types aren't exhaustive, and narrowed the `al-data-modeling-review.md` cue to require positive evidence of one of the nine types (a naming suffix, key shape, or usage) rather than firing on any new table with a `keys` block.

Validators pass clean (frontmatter 0 errors, same 2 pre-existing keyword-count warnings; knowledge-index, knowledge-retrieval, review-fixtures all green).

[Jesper Schulz-Wedde (JesperSchulz)]

One merge-critical inconsistency remains. The revised upgrade article now correctly permits business-data safety guards within one tagged migration, but al-upgrade-review.md still worklists nested “multi-branch business-data” or “separately-conditioned business” decisions. The reviewer can therefore still emit the exact false positive the article now excludes.

Please limit the cue to nested upgrade-tag checks or multiple functionally unrelated migrations under one tag, and explicitly exclude record loops and safety guards belonging to a single migration. The other three blockers from the latest review are resolved.

[Jesper Schulz-Wedde (JesperSchulz)]

One merge-critical inconsistency remains. The revised upgrade article now correctly permits business-data safety guards within one tagged migration, but al-upgrade-review.md still worklists nested “multi-branch business-data” or “separately-conditioned business” decisions. The reviewer can therefore still emit the exact false positive the article now excludes.

Please limit the cue to nested upgrade-tag checks or multiple functionally unrelated migrations under one tag, and explicitly exclude record loops and safety guards belonging to a single migration. The other three blockers from the latest review are resolved.

[Jesper Schulz-Wedde (JesperSchulz)]

One merge-critical inconsistency remains. The revised upgrade article now correctly permits business-data safety guards within one tagged migration, but al-upgrade-review.md still worklists nested “multi-branch business-data” or “separately-conditioned business” decisions. The reviewer can therefore still emit the exact false positive the article now excludes.

Please limit the cue to nested upgrade-tag checks or multiple functionally unrelated migrations under one tag, and explicitly exclude record loops and safety guards belonging to a single migration. The other three blockers from the latest review are resolved.

[Michael Dieringer (MichaelDieringer)]

Thanks Jesper — fixed in adc7087 (al-upgrade-review.md only).

The cue is now limited to nested upgrade-tag checks (HasUpgradeTag inside another tag's guarded body), two or more functionally unrelated migrations (different tables, fields, or concerns) under one tag, or one procedure mixing the gated logic of more than one tag. It explicitly excludes record loops and business-data safety guards (corruption checks, redundant-write checks, etc.) that serve the single migration the tag represents, however many if levels they take — matching the revised article.

Test-ReviewContract.ps1 and Test-ReviewFixtures.ps1 pass.

[Jesper Schulz-Wedde (JesperSchulz)]

Reviewed the updates through adc708766af71f235c9fa57e1c443c3d16e1be68. The revised upgrade routing now limits the rule to nested upgrade-tag logic and explicitly excludes record loops and business-data safety guards, resolving the prior false-positive blocker. The article and good/bad companions are consistent with that scope, and I found no merge-critical regressions.

[Jesper Schulz-Wedde (JesperSchulz)]

The merge conflicts have been resolved and fully validated. Because GitHub reports push=false for our token on the Curabis fork despite maintainer edits being enabled, I opened Curabis#4 directly against this PR's source branch.

Please merge that small sync PR. It applies validated merge commit 6e01ca89f6087ccd257d047ec32a4279ac06df52; once it lands, we intend to merge this PR immediately.

[Michael Dieringer (MichaelDieringer)]

Thanks Jesper — Curabis#4 is merged. main moved again right after (#196 landed at 13:03), which put this branch back into conflict in microsoft/skills/review/al-security-review.md, so I merged main in on top of your sync commit (2f6e22e).

Resolution is a plain union of the token list: #196's new SetFilter token is added alongside this PR's already-approved tokens (ServiceEnabled, PageType = API, QueryType = API, permissionset, Web Services). No other files conflicted.

On the merged tree: validate_frontmatter.py (0 errors; the same two pre-existing keyword-count warnings), Test-ReviewContract.ps1, Test-ReviewFixtures.ps1, Test-KnowledgeIndex.ps1 and Test-SkillIndex.ps1 all pass. GitHub reports the PR as mergeable.