Resolve al-security-review.md by union: keep both main's additions from #196 and this
PR's already-approved changes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Add 18 community AL/BC patterns across style, data-modeling, web-services, appsource, breaking-changes, performance, and testing
Contributed by CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Each article follows the knowledge file format (frontmatter, Description/Best Practice/Anti Pattern, sibling .good.al/.bad.al samples).
* Address Jesper Schulz-Wedde's review on PR #156
- Rename 3 articles so their .good.al/.bad.al companion stems match
(do-not-change-primary-key, testfield-required-setup-field,
al-identifiers-english), fixing the R14 orphan-sample errors.
- do-not-change-primary-key.good.al: include Flow in the new table's
own primary key so it actually models the discriminating dimension.
- al-build-output-must-not-pollute-project-root.md: drop the
unsubstantiated AL0197 causal claim and the non-existent
al.outputPath setting; reframe as build-artifact hygiene sourced
from ALTool --outfolder / al_build outputPath.
- prefer-email-module.md: Email Message is Codeunit 8904, not a table;
distinguish it from the underlying Sent/Outbox/Draft storage.
- file-datatype-saas.md: File.Open/Create/Read/Write fails to compile
against a Cloud-scoped project, it does not compile and silently
fail at runtime.
- namespace-must-be-verified-from-source.md: narrow to "resolve from
the referenced object's source or symbols," since source-file line
one is not the only authoritative source (symbol packages, comments
before the namespace line).
- test-data-must-be-random-and-complete.md: drop "assume an empty
database" and "collision-free" absolutes; reframe around
independence from unrelated business records and reserving explicit
values for scenario-defining inputs.
- binary-choice-must-be-boolean.md: scope to genuine true/false
semantics, not mechanical two-member-enum-to-boolean conversion.
- document-report-word-layout.md: scope down to a sourced Microsoft
Learn recommendation instead of an unconditional performance
guarantee; cite the three Learn pages.
- Wire the new articles into their review skills' candidate-selection
signals (file-datatype-saas, prefer-email-module,
namespace-must-be-verified-from-source, var-parameters-require-an-
addressable-variable) so they can actually enter a worklist.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix dimension-management-wiring.md: ValidateShortcutDimCode and CreateDim
do not exist on the current DimensionManagement codeunit
Verified against microsoft/BCApps: the real master-table validation
procedure is ValidateDimValueCode (or ValidateShortcutDimValues when a
DimSetID is also needed), and the real document-side inheritance
procedure is GetDefaultDimID, not CreateDim. Caught from Jesper
Schulz-Wedde's review thread, which had been partially hidden by
GitHub's comment folding.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address second round of Jesper Schulz-Wedde's review on PR #156
- dimension-management-wiring.md/.good.al: split into the two distinct
models the article was conflating - master data (Default Dimension
records via ValidateDimValueCode/SaveDefaultDim) vs. transactional/
document data (a single Dimension Set ID assembled via AddDimSource +
GetDefaultDimID, verified against BCApps' ExchRateAdjmtProcess.Codeunit.al).
Added a compiling document-table example alongside the existing master
table one.
- Deleted api-page-flowfields-must-be-calcfields (.md/.good.al/.bad.al):
Microsoft's own FlowFields documentation states a FlowField used as a
control's direct source expression is automatically calculated on any
page - no API-page exception is documented, and none could be
reproduced.
- prefer-email-module.bad.al/.md: Codeunit Mail has no Send/GetErrorDesc
members; fixed to the real current 7-argument CreateMessage signature,
and corrected the claim that the legacy path "still runs" - its base
implementation no longer sends anything, only raises integration events.
- check-post-line-batch-pattern.md/.good.al: reframed from a universal
invariant to the standard shape, naming the real Gen./Item/CA/Res./Job/
Insurance/Mfg. Item/FA Jnl.-Check Line/-Post Line/-Post Batch codeunits
it's based on. Added the missing Check Line companion codeunit so the
good fixture is internally complete.
- test-data-must-be-random-and-complete.good.al: removed leftover
"collision-free" wording contradicting the already-corrected article text.
- fixed-choice-set-must-use-enum-not-integer.md: removed the reintroduced
state-count heuristic ("the line is the state count"), aligned with
binary-choice-must-be-boolean.md's semantics-based distinction.
- namespace-must-be-verified-from-source.md: removed the false claim that
the compiler and AL Language Server use different namespace-resolution
rules.
- intrinsic-al-functions-must-use-modern-casing.md: removed the unverified
claim that PascalCase is the VS Code formatter's default output.
Worklist completeness: added cues for the 8 rules in data-modeling,
testing, performance, and web-services that had none (Jesper's explicit
ask), plus the same gap in all 7 style rules from this PR (not explicitly
named this round, but the identical systemic issue) - 15 cues total across
al-data-modeling-review.md, al-testing-review.md, al-performance-review.md,
al-web-services-review.md, and al-style-review.md.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix remaining correctness issues from Jesper's 2026-09-15 re-review
- dimension-management-wiring: SaveDefaultDim's third argument is the
shortcut dimension number (1-8), not the field's AL field ID; the
fixture passed FieldNo(...) = 10. GetDefaultDimID's InheritFromDimSetID
must be 0 when recomputing after the linking record changes, not the
document's existing Dimension Set ID (which would retain the previous
customer's leftover dimensions). Verified against
DimensionManagement.Codeunit.al and BankDepositHeader.Table.al in the
BCApps reference clone.
- check-post-line-batch-pattern: "Post Line writes exactly one line to
the ledger" overclaimed - Gen. Jnl.-Post Line alone calls InsertGLEntry
from a dozen call sites (balancing entry, VAT, currency rounding,
deferrals) and can write several G/L Entries per journal line.
Reworded to "posts exactly one journal line" and softened the
"distinct, non-overlapping responsibilities" absolute.
- namespace-must-be-verified-from-source.bad.al: dropped the "resolves
in a local build, fails in VS Code" comment (taught an inherent
compiler/language-server disagreement that isn't real); reframed as
stale/cached symbols, matching the prose fix already made.
- file-datatype-saas.good.al: replaced the deprecated 5-argument
UploadIntoStream overload with the current 2-argument one, and
actually staged through TempBlob as the article's own Best Practice
instructs (the declared TempBlob variable was previously unused).
- test-data-must-be-random-and-complete: no longer treats a
short-but-valid value as defective merely for being "underfilled" -
AL field lengths are maxima, not minimums. Scoped to missing values
or a scenario with an explicit length/format requirement (e.g. a
truncation test). Updated the al-testing-review.md routing cue to
match.
- stored-derived-fields-must-not-be-exposed-directly: stopped mandating
source-field exposure as part of the core pattern: the good fixture
exposed only one of the derived value's two inputs (Hours Used, not
Budgeted Hours), making the claimed "so the consumer can verify it"
impossible. Reframed as an optional, all-or-nothing addition and
fixed the fixture to expose both inputs.
Rebased onto upstream/main to resolve conflicts in
al-breaking-changes-review.md, al-data-modeling-review.md,
al-performance-review.md, and al-style-review.md against merged PRs
#148 and #153; all sides' worklist tokens/cues retained.
* Fix remaining READ-convention sample links across this PR's 18 articles
The same plain-backtick "See sample: \`x.good.al\`." form fixed on
al-methods-limited-during-write-transactions (PR #161) turned up
repo-wide on 15 more of this PR's articles - Knowledge-Retrieval.ps1
requires the markdown-link form to associate a sample with its
article. All 16 fixed; the four local validators (frontmatter,
knowledge-index, knowledge-retrieval, review-fixtures, skill-index)
pass.
* Fix two merge-critical correctness issues from Jesper's 2026-09-22 review
- api-page-key-fields-must-be-editable-on-insert.good.al and
stored-derived-fields-must-not-be-exposed-directly.good.al: both were
writable API pages missing DelayedInsert = true, contradicting this
repo's own api-page-delayedinsert-true rule - the canonical "good"
samples were teaching code BCQuality itself flags.
- dimension-management-wiring.good.al: UpdateDimensionSetID exited
early when Customer.Get failed, leaving the previous customer's
shortcut dimension and Dimension Set ID in place - the same staleness
bug the InheritFromDimSetID = 0 fix (from the prior review round) was
meant to prevent, just triggered by a failed lookup instead of a
successful one. Now clears the shortcut field and recomputes with an
empty source list on a failed lookup too, so GetDefaultDimID
correctly returns an empty Dimension Set ID instead of never running.
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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).
- 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>
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.
Move canonical knowledge for Microsoft-owned review domains into the Microsoft layer and document the skill/knowledge co-location policy.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Copilot-Session: 2a6ea875-d38e-4f30-aadb-0d606f9be231
* Seed web-services (API v2) knowledge domain
Add eight web-services API page knowledge articles (each with .good.al/.bad.al samples), a new al-web-services-review leaf skill, and wire it into al-code-review and the README.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Trim web-services domain to 6 non-duplicative articles
Drop API entity-naming/camelCase and DelayedInsert articles (owned by the style domain). Reframe the committed-data and API-versioning articles to stay strictly within the endpoint design/behavior lane, and update the leaf skill's worklist tokens and Output example accordingly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
