* 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>
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>
* 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>
