* ShowMandatory + OnQueryClosePage Check
* Tighten mandatory-field review guidance
Require explicit ShowMandatory in the good sample, acknowledge that NotBlank marking is unreliable, and limit findings to visible editable controls on paths where users must supply a value.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 89dba8c8-6529-4b60-956f-875a59be499d
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Copilot-Session: 89dba8c8-6529-4b60-956f-875a59be499d
* Add community knowledge on AL boolean operators not short-circuiting
AL gives no short-circuit (lazy) evaluation guarantee for and/or/xor —
neither the AL operators nor the boolean operators documentation defines a
lazy evaluation order. LLMs trained on C#, JavaScript, or SQL assume the
left operand guards the right, which produces conditions where a guard
does not protect an unsafe subscript or a field read after a failed Get,
and where an expensive operand is paid on every path.
Adds community/knowledge/performance/boolean-operators-do-not-short-circuit.md
with good/bad AL companions. The guidance prefers nested if when one operand
depends on another, while keeping and/or legitimate for operands that are
independently safe and cheap, so a reviewer does not flag harmless bound
checks. This is the first article in a community performance domain; the
Microsoft performance review leaf skill already sources candidates by domain
across every enabled layer, so no skill change is needed to reach it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Add case true of pattern for long condition chains
Follow-up from PR review: nested if is the right answer for two or three
dependent conditions, but past that the nesting becomes the problem. AL's
case statement is the flat alternative — the control statements
documentation states a value set "must be an expression or a range" and
that the first matching value set executes, so case true of / case false of
accept boolean expressions and stop at the first match. That is the
laziness the boolean operators do not provide.
Adds case-true-of-for-long-condition-chains.md with good/bad AL companions:
case false of for guard chains where every condition must hold, case true of
for first-match dispatch. The bad sample shows both failure shapes — a
five-level if ladder, and the worse escape of collapsing it into an and
chain, which trades nesting for a real defect.
Cross-links both articles, and adds the threshold to the short-circuit
article's Best Practice so following it does not lead to a deep ladder.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Comma-group the pure value sets in the case false of sample
Review feedback: the five value sets sharing exit(false) should be comma
separated. Applied to the three that are pure field reads with no order
dependency.
The Get and the Blocked read keep their own value sets. The documentation
guarantees that the first matching value set executes, which orders matching
across separate value sets; it says nothing about evaluation within one
comma-separated set, and the natural lowering of that is an equality-or
chain — where AL's or does not short-circuit. Grouping the Get with the
checks that must precede it would rest the sample's correctness on
undocumented behaviour, which is the defect these two articles exist to
prevent.
Encodes the boundary in the Best Practice section so the grouping is applied
where it is safe and not where it is not, and scopes the repeated-exit
detection signal explicitly to nested chains so the case sample does not read
as its own anti-pattern.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Use a single exit(false) for the whole case false of chain
Review decision: all five conditions share one action, so they share one
comma-separated value set with a single exit(false).
Aligns the article's Best Practice with the sample — it previously told
authors to keep side-effecting conditions in their own value set, which the
sample no longer does — and drops the now-contradictory wording about listing
the failure action per condition.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Drop the parentheses from the case false of value sets
Applies NKarolak's review suggestion: a case value set needs no parentheses
around a comparison.
Encodes the rationale in the Best Practice section, since it is a real
advantage of the pattern and is not documented elsewhere in the repo. The AL
operator hierarchy places and/xor above the comparison operators and or just
above them too, so parentheses are mandatory in an and chain — A = B and
C = D misparses without them — while a case value set has no and to bind
tighter and needs none. That inverts the precedence most developers arrive
with from C#.
Also notes in the bad sample that its parentheses are not optional, so the
two samples contrast on parentheses as well as on evaluation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Address correctness review: or/xor pattern, case value-set ordering
Fixes two points from JesperSchulz's review on PR #136.
1. boolean-operators-do-not-short-circuit.md gave one fix — nested if — for
and, or, and xor alike. That's only correct for and: nesting if A then if
B then Action drops the A-true/B-false case of A or B, silently changing
the result. Gives or its own early-exit pattern (if A then exit(true);
exit(B)), warns explicitly against applying the and-rewrite to or, and
clarifies that xor is not a short-circuit candidate in any language since
its result always depends on both operands. Adds an or fixture
(IsEligibleForFreeShipping) to both samples so an agent has a concrete
pattern to match instead of extrapolating from the and-only examples.
2. case-true-of-for-long-condition-chains.md derived stop-at-first-match for
one comma-separated value set from the documentation's guarantee about
the first matching value set — plural, i.e. ordering across value sets,
which is not the same claim. The good sample's Item.Get / Blocked pair
depended on the one the docs don't make. Restructures the sample to only
comma-group the three pure, order-independent checks; Get and Blocked
keep their own value sets, in order, relying solely on the guarantee that
is actually documented. Description and Anti Pattern now state that
boundary so it isn't re-collapsed later.
Also carries forward a parenthesis fix (not Item.Blocked in the collapsed
bad sample) that was made two commits ago but never landed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* knowledge(performance): add community rules for JIT, locks, and false positives
These articles capture BC-specific mechanics agents still invert: partial-record JIT on writes, Reset clearing SetLoadFields, HttpClient inside write transactions, and batched number series, plus negative guidance that stops over-eager Query and IsEmpty "fixes".
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(community/performance): address PR #134 review comments
Fixes technical inaccuracies and behavioral issues raised during review
of the community performance knowledge articles.
avoid-currpage-update-in-onaftergetrecord.md
- Removed OnAfterGetCurrRecord from the Best Practice section. That
trigger is itself implicitly re-entered on every refresh, so placing
CurrPage.Update(false) inside it can re-trigger the very loop the rule
warns about. Only OnAction remains as the recommended location.
batch-number-series-instead-of-getnextno-per-row.md
- Scoped the lock/contention claim to gapless (Normal) series only.
Added explicit statement that Allow Gaps series use NumberSequence
sequences and do not hold the series-line lock, so the anti-pattern
detection signal now excludes Allow Gaps series.
countapprox-for-progress-not-count.bad.al / .good.al
- Changed FindSet(true) to FindSet() in both samples. The loop is
read-only; UpdLock is not needed and was misleading.
countapprox-for-progress-not-count.md
- Qualified the Count() cost claim: it is expensive only when no SIFT
key covers all filtered fields (forcing a SELECT COUNT(*)); a filtered
count with matching SIFT coverage is cheap. Added a parenthetical
noting that SIFT coverage cannot be assumed for arbitrary filters.
dataaccessintent-readonly-on-analytical-objects.md
- Changed bc-version from [all] to ["16.."]. DataAccessIntent was
introduced at runtime 5.0 / BC 16 and has no effect in earlier
versions.
- Added precision to the supported objects: pages must be PageType=API
with Editable=false; for queries, replica routing only applies when
the query is exposed via OData/API, not for AL-to-AL calls.
httpclient-inside-write-transaction-holds-locks.good.al
- Replaced the Commit()-before-HttpClient pattern with a two-codeunit
task-deferral pattern. The write completes inside the caller's
transaction (locks released naturally when it ends); a TaskScheduler
task runs the HTTP call in a separate session where no write-
transaction lock is held.
httpclient-inside-write-transaction-holds-locks.md
- Changed Best Practice to recommend TaskScheduler/job queue deferral
as the primary remedy.
- Added an explicit warning against Commit() as a generic remedy: it
irrevocably commits all prior writes in the current transaction, so a
subsequent failure cannot roll them back. Commit() is appropriate only
at top-level entry points where partial persistence is intentional.
oncompanyopen-subscribers-must-not-do-io.good.al
- Added a ClientType guard so the subscriber exits immediately in
background task sessions (OnAfterLogin fires there too, which would
create an unbounded task chain without the guard).
- Added a TaskScheduler.TaskExists idempotency check to avoid queuing
duplicate tasks on repeated logins.
- Fixed the error-fallback codeunit in CreateTask from a self-reference
to 0 (no error codeunit).
- Added a 60-second delay (CurrentDateTime() + 60000) so the task does
not compete with the login session itself.
prefer-related-table-over-extension-on-hot-ledgers.md
- Changed bc-version from [all] to ["23.."]. The companion-table join
optimisation (single join per base table, automatic exclusion on List/
OData pages with partial records) was introduced in v23.
- Scoped the "join is always paid" claim: since v23 the join is
excluded on List/ListPart/OData pages when no extension field is
loaded under partial-record semantics, but it is still paid on every
posting path and any AL code that accesses an extension field.
skip-setloadfields-on-write-and-transferfields.bad.al / .good.al / .md
- Changed the example scenario from Modify(false) (which is actually
valid with a partial record) to TransferFields+Insert into a temporary
record, which is a documented full-load operation.
- Removed Modify from the list of operations that force a full load.
- Added an explicit note in the .md that Modify itself is not in the
full-load list; SetLoadFields is safe to use before Modify(false).
use-dedicated-lookup-pages-not-full-lists.bad.al
- Added a separate CardPart page definition (50101) to replace the
self-referencing FactBox part that referenced the same list page it
was embedded in. A part cannot refer to its own container page.
validate-on-partial-record-forces-jit.good.al
- Restored SetLoadFields to the good sample so it exercises a partial
record and the contrast with .bad.al is field selection, not the
absence of the feature. The good sample loads both Name and Search
Name (the field Name.OnValidate writes), while the bad sample loads
only Name, causing a JIT reload of Search Name on every Validate call.
* fix(community/performance): correct httpclient and oncompanyopen good samples
httpclient-inside-write-transaction-holds-locks.good.al
- Pass Customer.RecordId as the last argument to CreateTask so the task
is bound to the single customer that was written, not to all customers.
- Declare TableNo = Customer on the task codeunit so the platform loads
the bound record into Rec automatically when OnRun executes.
- Replace the FindSet loop over all customers with Rec."No.", preserving
the one-customer scope of the original SyncCustomerLastName procedure.
oncompanyopen-subscribers-must-not-do-io.good.al
- Replace Session.GetCurrentClientType() with Session.CurrentClientType(),
the correct platform method name.
- Fix the idempotency check: TaskScheduler.TaskExists() requires a Guid,
not a codeunit integer ID. Store the Guid returned by CreateTask in
IsolatedStorage (DataScope::Company) under a fixed key; on the next
login read it back as Text, Evaluate it to Guid, and pass that Guid to
TaskExists so the type matches the method signature.
* fix(community/performance): address review feedback
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
* Add TransferFields SkipFieldsNotMatchingType guidance
* Update transferfields-skip-type-mismatch-can-drop-data.md
* Update transferfields-skip-type-mismatch-can-drop-data.good.al
* Move good sample reference under Best Practice
Aligns the article with the repo convention used by the sibling data-modeling files: the .good.al reference belongs under Best Practice and the .bad.al reference under Anti Pattern. Previously both pointers sat under Anti Pattern, leaving the good-sample reference orphaned in the wrong section.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c998c71-f30b-40f3-b714-87fafed505d8
---------
Co-authored-by: Jesper Schulz <jeschulz@microsoft.com>
Copilot-Session: 3c998c71-f30b-40f3-b714-87fafed505d8
Three related concerns, each proven by executable tests rather than recall.
They were extracted from a real defect that shipped through a six-reviewer
panel undetected, which is the admission test passing on behavior rather than
on theory.
owning-table-must-delete-dependents-in-ondelete
AL has no cascading delete. What makes it missable is an asymmetry: the
platform DOES keep references correct on rename via TableRelation, so a
developer who learns that and generalizes it to delete ships orphans. Also
notes the permission trap (delete rights needed on the dependent table, not
just the parent).
validate-table-relation-false-suppresses-rename-propagation
The non-obvious half. The property name implies input validation only, but
disabling it also switches off rename propagation. Verified against a parent
renamed once while a child held three fields: a normal relation (follows),
the same relation with validation disabled (does NOT follow), and a field
with no relation at all (does not follow) — the third being the control that
proves the test can detect a non-propagating field.
xrec-is-a-before-image-only-in-some-triggers
Corrects both the naive belief that xRec is always the previous record and
the folk rule that it 'only works from a page'. The behavior is per-trigger:
a genuine before-image in OnRename and OnDelete regardless of driver, a
mirror of Rec in OnInsert/OnModify when driven from code, and a real
before-image in those two only when a page drove the write. That last
asymmetry is why an OnModify comparison against xRec passes manual page
testing and silently no-ops in a job queue.
Targets /community per CONTRIBUTING — general BC knowledge, not fork-specific.
Frontmatter validator clean; Test-ReviewFixtures passes (32 cases, 16 leaves).
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>
* Promote security knowledge from community to Microsoft layer
Pure git-mv relocation of the SECURITY domain from the community layer to the Microsoft layer. No content changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address review feedback on security knowledge promotion
- do-not-grant-rights-beyond-a-users-entitlement.md: drop the See sample
reference to a .good.al file that does not exist
- Remove the 'Contributions welcome' boilerplate line from
compose-permission-sets, prefer-oauth2, and protect-sensitive-data
- protect-sensitive-data-in-temporary-tables: remove the pointless
DeleteAll on the locally scoped temp buffer in the good sample and
reword Best Practice to note local buffers are cleaned up automatically
- Drop guard-bulk-operations-with-istemporary from the promotion; it
stays in the community layer pending a decision on whether it is security
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Remove Contributions welcome boilerplate from do-not-grant article for consistency
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 <223556219+Copilot@users.noreply.github.com>
Repair broken knowledge references and align review examples with canonical articles. Correct explicit version gates, restore a missing title, and recognize the plugin directory in the root guard.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add 15 community knowledge articles from BC Code Intel ingest
Ingests net-new /community knowledge from BC Code Intelligence, surviving
the admission test, gray-zone salvage, and dedup against the full corpus.
Domains: ui (6), error-handling (3), performance (2), upgrade (1),
appsource (1), security (1), telemetry (1). The two BC24 No. Series
migration drafts are merged into one article.
Adds good/bad AL samples for the clean-fit articles (error-handling,
performance, security, telemetry). UI and appsource remain knowledge-only.
Validator and knowledge-index checks pass (207 articles).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Correct SetLoadFields JIT-load article to match MS docs
The draft claimed accessing an unlisted field "reloads the entire row"
per record. Microsoft's partial-records docs say otherwise: the platform
does an implicit Get that loads the missing field(s), and in a direct var
loop the first JIT updates the enumerator so later iterations do not
re-load. The genuine per-row penalty is the pass-by-value case, where the
copy's enumerator is not updated.
Rewrite the article around JIT loading and the by-value footgun, rename
the slug from ...full-reload to ...jit-load, and fix the good/bad samples
to demonstrate the by-value repetition accurately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Jeremy Vyska <jeremy@sparebrained.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pure git-mv relocation of all 7 performance articles from community/knowledge/performance/ to microsoft/knowledge/performance/. No content changes.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The article claimed SetLoadFields must be called before filters, but
call order has no impact on the resulting query plan. Removing the
article along with its good/bad AL samples rather than rewriting,
since the premise itself is incorrect.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 35 articles still in their seed form previously carried a banner
reading "Seed article. ... Domain stewards should expand, restructure,
and refine as needed." For a community preview, that phrasing reads as
"TODO left in production" to first-time visitors.
Replace all three banner variants (performance-seeded, security-seeded,
community-ported) with a single positive invitation:
> Contributions welcome — open a PR to refine or extend this article.
Content and structure of the articles are unchanged; only the leading
quote block differs. Articles that had their banner fully stripped in
the earlier triage pass (the showcase-grade ten) are unaffected.
Most of the corpus — FindSet/SetLoadFields/CalcFields patterns, permission
sets, SingleInstance codeunits, DataClassification, IsolatedStorage,
transaction scope, SecretText — describes BC platform behaviour that is
identical across supported versions. The seed [26..28] range on every
file implied a version-specificity the content does not actually have,
and there was no way to express "applies to every version" in the
schema the way [w1] and [all] already do for countries and
application-area.
Extend the v1 schema with a universal sentinel for bc-version, parallel
to the sentinels already defined for the other dimensions:
bc-version: [all] # applies to every BC version
[all] is mutually exclusive with explicit versions. Range shorthand
([26..28]) and explicit lists ([26, 27, 28]) continue to work for files
genuinely tied to a version-gated API or deprecation.
Update read.md (field definition, matching semantics, partial-context
rule), write.md (default to [all], use ranges only with a concrete
reason), README.md (frontmatter example), and the CI validator. All
forty existing knowledge files and the three action skills convert to
[all]; none of the current content is version-gated. Validator passes.
Remove seven knowledge files whose content is generic software-engineering
guidance that a capable LLM already applies without BCQuality present
(HTTPS-only, secret-leakage-in-errors, no-credentials-in-URLs, silent
security-error swallowing, short transaction scope, HTTP timeouts,
StrSubstNo-vs-concatenation). These fail the remedial-knowledge premise
and dilute the signal of the preview corpus.
Strip the "Seed article — domain stewards should expand" banner from ten
files that are ready to showcase (AA0232/AA0233 rules, FindSet read-only
semantics, SetLoadFields ordering and usage, CalcFields-in-loops,
SecretText end-to-end, DataClassification). The banner remains on files
that still need domain-steward refinement.
Add a "What belongs here" section to the README stating the admission
test: a file exists only if a modern LLM would get something wrong or
miss something without it. Gives contributors a concrete yes/no filter
before they open a PR.
Ports 14 concern-sized articles (8 performance, 6 security) and 25
AL samples from BC Code Intelligence, restructured to BCQuality's v1
schema and layered under /community/knowledge/. Each article is
atomic, under 100 lines, and ships <slug>.good.al and (where the
pattern has a clear anti-example) <slug>.bad.al siblings.
Jesper's microsoft-layer leaves (al-performance-review and
al-security-review) source across every enabled layer via
*/knowledge/<domain>/**, so these additions are picked up by the
existing action skills without any new skill definitions.
Performance (8):
- use-deleteall-for-filtered-bulk-deletion
- call-setloadfields-before-filters
- load-common-fields-before-branching-on-case
- load-only-primary-key-fields-for-reference-work
- omit-filter-only-fields-from-setloadfields
- choose-maintainsiftindex-by-read-write-ratio
- avoid-growing-globals-in-singleinstance-subscribers
- order-case-branches-by-frequency
Security (6):
- classify-every-field-with-dataclassification
- protect-sensitive-data-in-temporary-tables
- guard-bulk-operations-with-istemporary
- compose-permission-sets-with-included-sets
- do-not-grant-rights-beyond-a-users-entitlement
- prefer-oauth2-over-api-keys-for-external-http-calls
Graveyard-bound items (not ported; to be captured in a later
/docs/triage-graveyard.md):
- testfield-performance (soft guidance, low actionability)
- table-event-batch-operation-impact (keep-event-subscribers-lightweight
already carries the core insight)
- Most of /roger-reviewer (AL formatting - frontier-model territory)
- sift-technology-fundamentals (descriptive, not a citable concern)
- bc-telemetry-buddy-* (tooling promotion, not guidance)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
