* knowledge(performance): align SetLoadFields placement with AL Guidelines (#120)
The AL Guidelines mark `SetLoadFields` placed before `SetRange`/`SetFilter`
as bad code and recommend filters first, while the BCQuality samples used
the opposite order — contradictory guidance across two Microsoft repos.
Per Learn (`Record.SetLoadFields`), "fields that are filtered upon are
always loaded", so the two orders produce an identical projection. The
upstream rule is a readability convention: keep `SetLoadFields` adjacent
to the read it governs.
- Reorder filters ahead of `SetLoadFields` in the six affected AL samples.
- State the placement convention in the Best Practice section.
- Record in Description that order does not change the projection, and
that only a fieldless `SetLoadFields()` or a later overwriting call does.
- Add an Anti Pattern note so review agents treat the reverse order as a
readability observation, never a performance defect.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Clarify partial-record projection changes
Document AddLoadFields, SetBaseLoadFields, and Reset alongside SetLoadFields so the statement-order guidance does not imply those APIs leave the projection unchanged.
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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 89dba8c8-6529-4b60-956f-875a59be499d
The previous LLM-generated knowledge files contained factual
hallucinations. The most visible was the claim that `FindFirst` /
`FindLast` "forces a full-table scan" on an unfiltered record - it does
not; those APIs return a single row via the current key.
Other inaccuracies the audit found and fixed:
* `FindSet(true)` was described as "taking a LockTable". The correct
upstream phrasing is that `FindSet(true)` sets
`ReadIsolation::UpdLock` on the read. UpdLock and LockTable are
related but distinct mechanisms.
* The list of production-scale tables had been invented beyond the
upstream source (e.g. "Detailed Cust. Ledg. Entry") without a
citation. The regenerated list matches the ten tables upstream lists
with their P95 row counts.
* `SetLoadFields` guidance had been augmented with an extra mechanism
claim ("the database resolves the filter using the index without
hydrating the value") not present in upstream.
Approach: full regeneration of `microsoft/knowledge/` from the six
upstream BCApps Code Review instruction files, with Microsoft Learn /
the AL language reference as a secondary source. Every claim in every
regenerated file is anchored to a verbatim upstream quote (or a Learn
URL); the audit trail lives in artifacts/trace-<domain>.json on the
session workspace.
The PR #11 transaction/error-handling cluster is preserved verbatim:
* performance/understand-implicit-transaction-boundary.md
* performance/codeunit-run-as-atomic-sub-operation.{md,good.al,bad.al}
* performance/codeunit-run-requires-prior-commit-inside-transaction.{md,good.al,bad.al}
* performance/use-tryfunction-for-error-catching-not-rollback.{md,good.al,bad.al}
* performance/avoid-commit-inside-loops.{md,good.al,bad.al}
* security/commitbehavior-attribute-scopes-explicit-commits.{md,good.al,bad.al}
* testing/transactionmodel-attribute-governs-test-transactions.{md,good.al,bad.al}
These articles already cite Microsoft Learn and were carefully
cross-referenced; the regeneration skips their topics rather than
duplicating them.
File counts after regeneration:
performance 35 .md (5 preserved + 30 new)
privacy 17 .md
security 18 .md (1 preserved + 17 new)
style 33 .md
testing 1 .md (preserved)
ui 19 .md
upgrade 18 .md
Total 141 atomic knowledge files, each strictly one rule. All pass
.github/scripts/validate_frontmatter.py with 0 errors and 0 warnings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- performance/use-setloadfields-for-partial-records: Clarify that
filter-only fields (SetRange/SetFilter) do not need to be listed in
SetLoadFields — the DB resolves them via the index without hydrating
the value into AL memory.
- performance/avoid-calcfields-in-loops: Add explicit exception for
OnAfterGetRecord and OnValidate triggers, which are platform-managed
and not developer-authored loops.
- performance/split-read-only-and-write-paths-to-avoid-locktable: Add
ReadIsolation as the primary recommendation for read-only paths;
LockTable reserved for confirmed write paths only.
- performance/prefer-direct-record-over-recordref: Scope the finding to
hot unbounded loops (10k+ rows) over ledger-entry-scale tables;
RecordRef in bounded/admin/setup contexts is not a concern.
- upgrade/enum-changes-must-be-additive-at-the-end: Replace direct
ObsoleteState = Removed guidance with the two-stage workflow (Pending
first, Removed later); reference use-obsolete-pending-before-removed.
- upgrade/use-datatransfer-for-large-dataset-initialization: Add the
>300,000 records threshold as the concrete trigger for requiring
DataTransfer.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
The /samples/ top-level tree is replaced with sibling files in each
knowledge-layer folder. An article and its demonstrations now live
side-by-side:
microsoft/knowledge/<domain>/<slug>.md
microsoft/knowledge/<domain>/<slug>.good.al
microsoft/knowledge/<domain>/<slug>.bad.al
Rationale:
- Proximity. An article and its paired samples are one unit; the
filesystem now reflects that.
- Layer ownership. Samples inherit layer precedence for free -- a
/custom/ fork can override an article and its samples atomically,
which the shared /samples/ tree previously made awkward.
- Trivial migration path. Action-skill source globs
(*/knowledge/<domain>/**/*.md) are unchanged; sample discovery is a
sibling-filename lookup.
Changes:
- git mv of all 65 sample files from samples/<domain>/<slug>/{bad,good}.al
to microsoft/knowledge/<domain>/<slug>.{bad,good}.al (history preserved).
- Update See-sample references in all 37 articles that ship samples.
- skills/read.md: replace the no-code-blocks bullet with a pointer to a
new Sample files section that fully specifies the sibling convention,
the kinds (good/bad + forward-compatible), multi-technology rules,
demonstration-only status, and layer-precedence behaviour.
- skills/write.md: update the samples pointer to match.
- README.md: annotate the knowledge tree with the sample sibling shape.
- samples/README.md deleted; content lifted into skills/read.md.
- Both generators (C:\temp\gen_performance_knowledge.py,
C:\temp\gen_security_knowledge.py) updated to emit at the new paths
and to stop writing samples/README.md. Re-running them is idempotent
against the committed layout.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Converts an existing performance-review prompt into 22 atomic
knowledge articles under microsoft/knowledge/performance/, each
paired with AL samples under samples/performance/<slug>/ demonstrating
the anti-pattern and/or the best practice. The full set seeds the
corpus the microsoft/skills/al-performance-review leaf skill matches
against and validates the READ knowledge-file format end-to-end.
Every article conforms to the READ contract: six required frontmatter
fields, Description always present, no fenced code blocks, sample
code referenced by repo-relative path. Each article is marked with a
blockquote 'Seed article' note so domain stewards can extend or
restructure them freely.
Articles (ordered by concern area):
Database query efficiency
- use-findset-with-next (AA0181)
- avoid-findfirst-with-next (AA0233)
- only-fetch-records-you-use (AA0175)
- use-findset-readonly-by-default
- use-setloadfields-for-partial-records
- use-addloadfields-in-report-layouts
- use-calcsums-to-aggregate-filtered-sets (file: use-calcsums-for-flowfield-totals.md)
- avoid-calcfields-in-loops
- add-sift-keys-for-flowfields (AA0232)
- use-isempty-for-existence-checks
Filter and key optimization
- filter-before-find
- set-current-key-to-match-filters
Temporary tables and transactions
- use-temporary-tables-for-intermediate-data
- keep-transaction-scope-short
- avoid-user-interaction-in-transactions
- avoid-commit-inside-loops
Record operations
- prefer-get-for-primary-key-lookups
- use-insert-false-when-skipping-triggers
- prefer-direct-record-over-recordref
Strings, codeunits, events
- use-strsubstno-for-message-formatting
- use-single-instance-codeunits-for-caching
- keep-event-subscribers-lightweight
samples/README.md documents the sample-folder convention and makes
clear the samples are demonstration-only, not derived from BC base
application source, with unique object IDs in the 50100-50199 range.
Rubber-duck pass caught: a misleading good.al in avoid-calcfields-in-loops
(fixed by switching to a hoistable CalcFields scenario), an invalid
event subscriber signature in keep-event-subscribers-lightweight
(fixed by adding var xRec), normative guidance leaked into the
Description of use-findset-readonly-by-default (moved to Anti Pattern),
a missing sample pair for keep-transaction-scope-short (added), and
muddy FlowField/CalcSums framing (retitled and clarified).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
