* 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>
Retire deterministic compiler and analyzer duplicates, remove their review routing, and clarify the admission test for contextual analyzer knowledge.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* knowledge(breaking-changes): exclude appended event parameters from the signature-change rule
The article's detection guidance flags 'a parameter added' on any shipped procedure. Applied to an event publisher that is bound to rather than called, this produced a false positive on BCApps PR 10278, where a trailing var parameter was appended to the existing IntegrationEvent OnAfterOpenForRecRef and the developer twice replied that adding a parameter to an existing integration event is not a breaking change.
It also contradicted events/add-new-event-parameters-at-the-end, which already states that existing subscribers still bind to the leading parameters. Scope the rule to called procedures, carve out appended event parameters as additive, and keep every other event signature edit - removal, reorder, retype, var flip - in scope. Point the IsHandled case at events/do-not-add-ishandled-to-an-existing-event, which owns that semantic concern.
* fix: event parameter additions are additive at any position, not only when appended
The first revision justified the carve-out with leading-prefix binding and limited it to parameters appended at the end. Verified against shipping BCApps code that AL binds subscriber parameters by name, not position, so an added parameter is additive wherever it is placed. Also corrects the cross-reference to events/adding-a-parameter-to-an-event-is-not-a-breaking-change, which already states this rule, and drops reordering from the list of edits that break binding.
* Route event signature edits to the analyzer-backed events article
Address review feedback: name AS0025, AS0063 and AS0077 for the var and rename cases instead of claiming them in the breaking-changes article, and point at events/treat-local-and-internal-events-as-subscriber-contracts which already owns them.
---------
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
Two knowledge false-positive guards addressing bug 642303 (agent FPs on BCApps PR #9607):
- breaking-changes: relocating a field to a tableextension in the same app under the same field ID/name is a relocation, not a deletion/rename; the field still resolves on the table, so it must not be flagged as a deleted shipped field or require ObsoleteState staging. Scoped to the contract axis; silent on data migration.
- events: adding a parameter to an event publisher does not break existing subscribers (subscribers bind by name and match a subset), so the addition itself must not be reported as a breaking signature change.
* Add precision guards for systematic agent false-positive patterns
Encodes reviewer-confirmed FP guards from the online eval: tooltip-inherited, page-trigger default return, drill-down filter not visible in diff, dual-trigger CalcFields (do.md); and a released-baseline precondition for breaking-change/upgrade findings on never-shipped symbols.
* Scope suggested-code and location to exactly the changed lines
Addresses reviewer-reported misplaced suggestions from the eval: insert-only-property emitting the whole field, single-statement rewrites anchored on the procedure name, and reductive multi-line collapses. The skill now emits a location range that matches precisely the rewritten lines.
* Correct suggested-code scoping guidance to match one-click anchor mechanics
A lone inserted line matches no existing file line and cannot be anchored; bracket the new line with one adjacent unchanged line instead. Reductive collapses omit suggested-code and fall back to a manual snippet.
* Move BC-specific FP guards out of do.md into leaf skills
do.md is the stable action-skill template and must stay domain-agnostic (JesperSchulz review). Relocate the four known false-positive patterns to their domain leaves: ToolTip-inheritance to al-ui-review; drill-down/lookup filtering and CalcFields lifecycle to al-performance-review; page-trigger exit(true) semantics to al-error-handling-review.
* Move suggested-code line-scoping guidance out of do.md into al-code-review
do.md must not carry instructions for how the review skill behaves (JesperSchulz review). Relocate the location/suggested-code precise-span rules to al-code-review's existing Suggested-code guidance section. do.md is now unchanged vs main.
* Move false-positive guards from skills into knowledge files
Keep review skills slim (finders/appliers). The FP guards and released-baseline preconditions previously embedded in leaf skills become negative-clarification knowledge articles in their domains, and the agent-findings policy edits to al-ui/al-privacy are reverted to main. Adds 6 knowledge files: error-handling (page-boolean-triggers-default-to-true), ui (bound-page-field-inherits-source-field-tooltip), performance (calcfields-in-both-getrecord-triggers-is-not-redundant, page-effective-filter-may-live-outside-the-diff), breaking-changes (unreleased-symbol-change-is-not-a-breaking-change), upgrade (unreleased-schema-change-needs-no-upgrade-path).
* Revert branch's suggested-code scoping addition in al-code-review
The three location-span shapes added to al-code-review are output-format mechanics, not domain knowledge: one-click span correctness is the engine's job (Resolve-SuggestionPlacement) and do.md already owns the suggested-code/location contract. The AL concerns the examples illustrate are already covered by existing knowledge (use-isempty-for-existence-check, data-classification-required-on-pii-fields, no-space-before-method-parenthesis). Restores al-code-review to main; the branch now adds only the 6 knowledge files.
* Restore al-ui/al-privacy review skills to base (zero diff in PR)
These two leaf skills carried an accidental net change against the PR merge-base because an earlier revert used the current origin/main (post-#110) instead of the branch base (pre-#110). Restoring them to the merge-base version removes them from the PR diff entirely. Three-way merge still preserves main's #110 suppression.
---------
Co-authored-by: wenjiefan <wenjiefan@microsoft.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>
Seed a new breaking-changes (AL API stability) knowledge domain with six
articles plus good/bad AL samples, a new al-breaking-changes-review leaf
skill, and minimal wiring into al-code-review and the README.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
