* Add 18 more community AL/BC patterns across appsource, data-modeling, error-handling, security, style, testing, ui, upgrade, and web-services
Second contribution from CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Cross-checked against the current microsoft/knowledge corpus before opening; several originally-drafted candidates were dropped as duplicates of existing files.
* 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>
* Address second round of Jesper Schulz-Wedde's review on PR #157
- log-writes-must-survive-rollback.good.al: fixed invalid trigger
OnRun(var Rec: ...) declaration; Rec is implicit when TableNo is set.
- exposed-objects-must-be-in-a-permission-set.md: distinguished the three
exposure mechanisms (page/query web service or API, codeunit published
as a web service, [ServiceEnabled] bound action on a page) and their
actual permission targets (page/query "..." = X vs codeunit "..." = X).
- code-must-not-change-workdate.md: scoped from an absolute "never" to
"not as a side effect of unrelated logic" - verified real WorkDate(x)
setter usage in BCApps demo-data generators and test codeunits.
- bcpt-scenarios-must-be-app-specific.md: SingleInstance and
StartScenario/EndScenario reframed as context-dependent patterns, not
mandatory requirements - BCPT Create Customer uses neither.
- test-feature-scenario-tags.good.al/.bad.al: replaced the invented
LibrarySales.CreateCustomerWithPrice/"Item Price Mgt." calls with a real,
verified price-list-line test using Library - Sales/Library - Inventory/
Library - Price Calculation.
- page-design-must-match-bc-page-type-conventions.md: scoped the missing
UsageCategory anti-pattern to pages intended as searchable entry points.
- defensive-vs-offensive-code-must-match-blast-radius.md/.good.al/.bad.al:
replaced the VAT registration number "low blast radius" example with a
genuinely cosmetic field (customer home page URL).
- source-organized-by-feature-not-object-type.md: anti-pattern reframed as
inconsistency with a repo's own convention, not the object-type scheme
itself.
- pictures-must-use-media-not-blob.md: removed leftover "image variants"
wording contradicting the already-corrected MediaSet description.
Proactively fixed while sweeping all fixtures for invented APIs:
- given-blocks-must-cover-full-precondition-chain.bad.al: PostSalesOrder
called with wrong arity and referenced an undeclared variable.
- ui-test-codeunit-naming.good.al/.bad.al: replaced the same fake
"Item Price Mgt."/TestPage "Item Price" with real Library - Sales calls
and the real Customer Card TestPage.
Worklist completeness: added review-skill 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 x3,
al-testing-review.md x2, al-ui-review.md, al-upgrade-review.md,
al-web-services-review.md), and fixed test-feature-scenario-tags' cue,
which only matched the compliant (tagged) shape instead of the anti-pattern
(untagged/generic-named test).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix ten focused correctness items plus sample links from Jesper's 2026-09-15 re-review
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 a price list in Draft status, which price
calculation ignores. Added Validate(Status, Active) + Modify before the
sales line that depends on it. Verified Status field/enum against
PriceListHeader.Table.al and PriceStatus.Enum.al in the BCApps clone.
- exposed-objects-must-be-in-a-permission-set.md: a published codeunit is
a SOAP endpoint (SOAP is deprecated), not OData - Page/Query are the
OData object types. Corrected and pointed new integrations at API
pages/queries instead.
- al-error-handling-review.md: the log-writes-must-survive-rollback cue
selected on Session.StartSession, which only appears in the compliant
fix, never in the anti-pattern - the bad fixture could never be
worklisted. Recued on the actual risk shape (log insert around a
failed TryFunction/GetLastError* path, then raise/propagate), with
StartSession as an explicit compliant discriminator instead.
- page-design-must-match-bc-page-type-conventions.md: the enum value is
NavigatePage, not Navigate; noted the type list is a selected subset,
not an exhaustive PageType catalogue (PromptDialog, ConfigurationDialog,
UserControlHost, XmlPort also exist, out of this article's scope).
Four new correctness gaps:
- release-must-update-app-version.md: "the version is the only identity"
was backwards - id is the app's stable identity, version identifies a
release/code-state of it.
- defensive-vs-offensive-code-must-match-blast-radius.good.al: the "low
blast radius" example had no else branch, so a failed Customer.Get()
left the field at its prior/default value instead of the explicit
chosen fallback the article claims to demonstrate. Added the else.
- bcpt-scenarios-must-be-app-specific.good.al: InitTest and both measured
StartScenario/EndScenario sections were empty/comment-only, so the
"app-specific" fixture measured no actual work. Filled in a real,
self-contained header+line creation path.
- upgrade-tag-logic-must-not-nest-deeply.good.al: the flattened version
dropped both safety conditions the bad fixture had (Discount % = 0,
nonblank posting group), silently changing behavior 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 "See sample:" sample
references (16 articles) to the READ-convention markdown-link form,
matching the fix already made on #156/#158.
Rebased onto upstream/main (conflicts in al-ui-review.md, al-style-review.md,
al-upgrade-review.md against merged upstream PRs - all additive, both
sides' worklist cues retained).
* Fix four merge-critical issues from Jesper's 2026-09-22 review
- pages-must-not-contain-business-logic.good.al/.bad.al: the "good"
codeunit still directly assigned real Sales Line."Line Amount" and
called Modify(), bypassing the field's normal Validate cascade
(discount, VAT, related-amount maintenance) - persisting inconsistent
document lines regardless of which object the code lived in. Replaced
the real Sales Line example with a self-contained "Sample Order Line"
table and switched the codeunit to Validate()/Modify(true), so the
fixture demonstrates the page-vs-codeunit separation without teaching
unsafe direct field writes to a real BC document table.
- bcpt-scenarios-must-be-app-specific.good.al: Customer.FindFirst()
assumed a pre-existing customer (fails against an empty environment),
and a session-local NextNo counter for the header key collides across
concurrent BCPT sessions and repeated runs. Creates its own customer
when none exists, and generates keys from CreateGuid() instead of an
in-memory counter.
- upgrade-tag-logic-must-not-nest-deeply: the rule conflated two
different things - nesting one tag's existence check inside another
(the real anti-pattern Microsoft's guidance warns against) with
having business-data safety conditions inside a single tagged
migration's own loop body (which Microsoft's own worked example does,
and its own design guidance explicitly requires: "Implement extra
safety checks to avoid data corruption, even though you're using
upgrade tags"). Rewrote the Description/Best Practice/Anti Pattern to
scope the rule to actual tag nesting and migrations blended under one
tag, and rewrote both fixtures: good.al now shows two safety
conditions correctly nested inside one migration's own loop plus a
second, genuinely separate migration as its own flat tagged
procedure; bad.al now shows the real anti-pattern, one tag's check
nested inside another's guarded body.
- table-design-must-match-bc-table-type-conventions: the rule and its
worklist cue fired on any new table with a keys block, forcing
buffers, queues, logs, mapping tables, and staging tables into the
nearest-looking one of nine business-record archetypes. Added an
explicit scope note that these nine types aren't an exhaustive table
catalogue, and narrowed the al-data-modeling-review.md cue to require
positive evidence (a type-specific naming suffix, key shape, or
usage) before worklisting, instead of a bare keys/primary-key
declaration.
* Narrow upgrade-tag nesting cue to match revised article
Cue now flags only nested upgrade-tag checks or functionally unrelated
migrations under one tag, and explicitly excludes record loops and
business-data safety guards belonging to a single migration.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add bounded knowledge retrieval
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d8b7764-f15a-49ea-8d50-d9147334f8af
* Fix bc-version overflow and pathless-row identity in bounded retrieval
Catalog matching compared an Int32 -BCVersion against a bigint range bound.
PowerShell coerces the right operand to the left operand's type, so a bound
wider than Int32 threw a conversion error and failed the whole domain catalog
rather than the single row. Metadata validation already accepts such bounds,
so compare as bigint on both sides.
The shared pager built its oversized-row message with $row.path, which
throws under Set-StrictMode -Version Latest when a row carries no path,
replacing the explicit bound failure with a property-lookup error. Resolve the
path defensively for dictionary and object rows so the offset-based fallback
is reachable.
Both paths gain regression coverage that fails without these fixes.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d8b7764-f15a-49ea-8d50-d9147334f8af
* Document standalone review runner contract
Keep model selection and scheduling outside BCQuality while allowing orchestrators to run isolated review leaves concurrently with deterministic rollup semantics.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Support complete app folder reviews
Define folder-path as a current-state review scope and accept it across the standalone adapter, broad coordinator, and every AL review leaf.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add walk-up app review quick start
Put the complete app-folder installation and prompt flow directly in the README so partners can discover the standalone experience without reading integration details first.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Organize conceptual guides under docs
Move architecture and standalone runner documentation out of the repository root, add a documentation index, and update all inbound links.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Complete AL review knowledge readiness
Fill telemetry and Query coverage, strengthen thin review domains, correct audited content defects, and add deterministic cheap-model evaluation and reference-integrity safeguards.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
* Generalize review fixture discovery
Derive smoke cases from the leaf, domain, and paired-sample conventions so new leaves require no scoring-contract changes. Keep only exceptional selection/context overrides and fail when retrieval metadata cannot rank the selected article.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
* Preserve published field IDs in sample
Keep the existing Email and Contact Email field IDs unchanged, clarify that the sample represents an independent baseline, and use a local breaking-change rule for the generic smoke evaluation.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
* Clarify published field identity rules
State explicitly that a published field keeps its ID, name, and type while a replacement is added as a separate field under an unused ID.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
* Align field obsoletion sample baselines
Use Email field ID 3 as the shared baseline so the bad example demonstrates a same-ID rename while the good example retains the original field and adds a separate replacement.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
* Emit human-readable domain label on review findings
Add an optional findings[].domain field to the DO review output contract so
each finding carries its own human-readable review-domain display label. Leaf
review skills set it on every finding they emit; the al-code-review super-skill
copies it verbatim during rollup and sets it to "Agent" for its own
cross-cutting agent findings. This decouples consumers from BCQuality's domain
taxonomy: they render finding.domain verbatim instead of maintaining a
sub-skill-id -> label map.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Define domain display-label constraints
Clarify that review domains may contain internal whitespace, punctuation, case-sensitive text, and non-ASCII characters. Require consumers to preserve and safely encode the complete label instead of relying on lossy slugs, matching the replacement BC-ALAgents consumer.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 77d0a40e-8bf5-40ac-a450-40eb0255db03
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move eight net-new rules into the Microsoft layer, remove six overlapping articles, and update review skill discovery and references.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b130227-d418-4bc0-9e7d-ec6a37adf039
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
