The "Category Code" field had no TableRelation at all, so the good
subscriber's RemoveTableRelation call targeted metadata that never
existed - a no-op. Added a real TableRelation to "Sample Setup" on
that field (the one known exception to exclude) and a second,
ordinary self-referencing relation ("Parent No." -> "Sample
Header"."No.") with no exception. The good fixture now removes only
the first; the bad fixture's table-wide removal (field/related
table/field all 0) now demonstrably also strips the second, showing
the actual anti-pattern instead of removing nothing meaningful.
- al-testing-review.md: the generic ExpectedError cue's asserterror
Assert.IsTrue/IsFalse exclusion was unconditional, but the
specialized rule it deferred to only claims the pure-inversion
shape. A test expecting the guarded Boolean-returning call itself to
raise fell through both routes. Narrowed the exclusion to the same
inversion-only condition the specialized cue already uses.
- asserterror-needs-expectederror-and-code.md: the rollback-sentinel
exception (a trailing asserterror Error(...) used purely to force a
fixture rollback, not to verify a specific failure) previously lived
only in skill routing prose. Encoded it directly in the article's
Anti Pattern section so every consumer of the knowledge base sees it,
not just this one skill.
- commit-shared-test-fixture-inside-lazy-initialize.good.al/.bad.al:
replaced hand-rolled Item.Init()/Insert(true) with
LibraryInventory.CreateItem, so the canonical fixture doesn't itself
trigger use-library-codeunits-for-test-fixtures.
- table-relation-test-exclude-known-invalid-relations-via-event.good.al/
.bad.al: declared minimal "Sample Setup"/"Sample Header" tables
inline instead of referencing undefined symbols, matching this
repo's own convention that every fixture is self-contained.
- commit-shared-test-fixture-inside-lazy-initialize: three sub-issues.
Recommended TestIsolation = Codeunit instead of listing Disabled as an
equal option - Disabled never rolls back at all ("tests are not
isolated from each other" per the property's own docs), so a fixture
this pattern commits under Disabled is permanent database
contamination unless something else tears it down; Disabled is now
only mentioned alongside that explicit teardown requirement. Added
precedence in al-testing-review.md so the deliberate end-of-test
asserterror Error(...) rollback sentinel isn't also flagged by the
generic asserterror-needs-expectederror-and-code rule. Rewrote both
fixtures to actually demonstrate the pattern: persisted fixture data
(an Item record) instead of an empty comment, a second [Test] method
that depends on the fixture surviving into it, and an explicit
Subtype = TestRunner / TestIsolation = Codeunit runner codeunit.
- table-relation-test-exclude-known-invalid-relations-via-event:
the length/type rule was stated as one global requirement. Verified
ValidateFieldRelation in codeunit 134926 directly (BCApps reference
clone) and split it into the two branches the source actually has:
a field with any unconditional relation needs exact length and exact
resolved type; a field whose relations are all conditional only fails
on being shorter (longer is fine) than the largest related field, and
when the required type is specifically Code, a Text source passes too
- a tolerance that does not apply on the unconditional side and does
not extend to a required Text.
Rebased onto upstream/main (one conflict in
transactionmodel-attribute-governs-test-transactions.md - upstream had
already linked its sample references via the READ convention, ours
added a Source section; merged both). Also converted the 3 remaining
plain-backtick sample references in this PR to the READ-convention
markdown-link form, same fix as #156/#157/#158.
- commit-shared-test-fixture-inside-lazy-initialize.md: fundamentally
rewritten. AutoCommit is the documented default TransactionModel, not
AutoRollback. Explains the real mechanism (Commit() protects a fixture
from the test method's own later deliberate rollback, per Codeunit.Run/
TransactionModel-property semantics) and the TestIsolation dependency
(Disabled/Codeunit survive across methods, Function does not). Fixtures
rewritten to demonstrate the actual failure/success shape.
- transactionmodel-attribute-governs-test-transactions.md: now states the
AutoCommit default explicitly and agrees with the article above, closing
the contradiction Jesper flagged between the two testing articles.
- Deleted confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text
(.md/.good.al/.bad.al): the underlying platform bug (microsoft/
ALAppExtensions#23935) was closed as completed in Feb 2024; cannot be
reproduced or bc-version-pinned on any currently supported version.
- table-relation-test-exclude-known-invalid-relations-via-event.md: added
the [Scope('OnPrem')] boundary verified against BCApps' Table Relation
Test codeunit.
- use-assert-isfalse-not-asserterror-for-boolean-checks.md: added a Scope
section resolving the overlap with asserterror-needs-expectederror-and-code.
- al-testing-review.md: fixed the shared-fixture cue to catch the actual
anti-pattern instead of the compliant shape, added the missing cue for
use-assert-isfalse-not-asserterror-for-boolean-checks, wired precedence
between it and the generic asserterror rule, and removed the cue for the
deleted article.
- Added in-file Source provenance (specific fluxxus.nl post per article,
with what was independently verified vs. taken from the post) to the
three surviving externally-inspired articles, per Jesper's request that
provenance live in the knowledge file itself, not only the PR description.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- transactionmodel-attribute-governs-test-transactions.md: the "Commit
causes an error" behavior is specific to an explicitly declared
AutoRollback attribute. A test method with no TransactionModel
attribute at all is a distinct, valid shape — BCApps' own
codeunit 134915 "ERM Online Mapping Setup" commits inside a lazy
Initialize() with no attribute declared, cleaning up via a manual
asserterror at the end. Evidence for commit-shared-test-fixture-
inside-lazy-initialize.md (this PR), which is correct as submitted.
- confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md:
reframe as a known, unconfirmed-fix platform defect
(microsoft/ALAppExtensions#23935) rather than designed behavior; add
the Message/MessageHandler asymmetry as supporting evidence.
- table-relation-test-exclude-known-invalid-relations-via-event.md:
note the test-app-only consumer dependency; correct "walks every
TableRelation field property in the app" to the actual tenant-wide
Table Relations Metadata scope across installed apps.
- Wire confirm-needs-strsubstno, commit-shared-test-fixture-inside-
lazy-initialize, and table-relation-test-exclude-known-invalid-
relations-via-event into al-testing-review.md's candidate-selection
cues.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fourth batch from CURABIS ApS, mined from an external BC/NAV testing expert's blog archive (fluxxus.nl). Confirm+StrSubstNo interaction with ConfirmHandler, Table Relation Test's OnAfterRemoveTableRelation exclusion hook (verified against BCApps source, codeunit 134926), committing shared lazy-Initialize fixture data, and Assert.IsFalse vs asserterror for boolean checks.
* Add Finance posting domain-knowledge pilot
Adds three atomic domain-rule knowledge files under community/knowledge/finance/ as a pilot for Type-A (normative) Business Central domain knowledge: post through the posting engine, treat posted ledger entries as immutable, and treat the Dimension Set ID as the source of truth for dimensions. Includes a good/bad AL sample pair for the posting rule.
These encode BC-specific invariants that LLMs reliably get wrong, fitting the existing remedial/atomic knowledge grain with no schema or contract changes. Passes the repo frontmatter validator and is discovered by the knowledge index.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Clarify editable operational fields on posted ledger entries
Addresses review feedback from @JeremyVyska on PR #57: the immutability rule applies to financial content, not the whole entry. Reframes the Description around financial content and gives the operational-field exception (payment/application data, on-hold, applies-to, communication fields edited via CustEntry-Edit/VendEntry-Edit and the ledger entry pages) its own paragraph in Best Practice instead of understating it as a narrow set.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Expand Finance pilot into a source-verified review domain
Move Finance knowledge to the Microsoft-owned layer, add nine scoped rules with eighteen AL samples, and register bounded Finance review with complete paired evaluation coverage.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Fix Finance review applicability and ownership boundaries
Remove application-area gating and later VAT-field dependencies, align dynamic shared conventions, separate SCM ownership, and keep journal examples focused on the intended invariant.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Remove logo branding (#194)
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add title and description to README
* Add foundational AL developer knowledge (#195)
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Clarify locale-safe DateFormula Evaluate inputs (#193)
* Clarify locale-safe DateFormula Evaluate inputs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Normalize DateFormula article sections
Keep the analyzer-gap explanation in Description and its scoped probe evidence in References, without a novel Validation section. Normative guidance and fixtures are unchanged.
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 App <223556219+Copilot@users.noreply.github.com>
* Add SCM functional knowledge domain (#192)
* Add SCM functional knowledge domain
Introduce nine source-backed rules with original AL sample pairs, bounded SCM review routing, and complete positive/clean evaluation coverage.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Normalize SCM knowledge and review ownership
Align article and AL sample conventions, keep BC facts separate from review mechanics, and clarify reciprocal Finance ownership without bespoke shared test assertions.
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 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>
Use [\slug.good.al\](slug.good.al) form so tools/Knowledge-Retrieval.ps1
Assert-SampleLink validation passes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New remedial article for spotting when AL calls an endpoint that does not
authenticate itself to the client (bare HttpClient.Get, blank SOAP SecretText,
post-DisableHttpsCheck HTTP) and requires the response to be size-, schema-, and
request/response-integrity-validated before it is trusted. Includes the
BC-specific false-positive clarifications (platform buffers the full body, so an
in-AL size check after buffering is correct; no DNS-rebinding/bounded-read demand;
HTTPS not always enforceable) plus good/bad AL samples.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* knowledge(events): ChangeCompany leaves triggers and trigger-event subscribers running in the calling company
ChangeCompany redirects only the data access of a record variable; Learn states that triggers still run in the current company. The database trigger events are raised on every database operation and only pass RunTrigger to the subscriber, so Insert(false) after ChangeCompany still runs every subscriber in the calling company. Generated code either assumes the record 'becomes' a target-company record, or switches RunTrigger off and hand-copies the trigger logic, leaving the subscribers writing to the wrong company; none of the tested runs reached StartSession with the company parameter.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* knowledge(events): qualify StartSession async semantics and fix concurrency-unsafe sample key
Addresses PR #152 review: StartSession is a fire-and-forget background
session (Ok reports only whether it started, not whether the codeunit
succeeded, and errors inside it do not propagate), so the Best Practice
now scopes the recommendation and calls out the durable status/error
channel a synchronous-success write needs. The good sample's
FindLast()+1 entry-number pattern raced under concurrent background
sessions; switched to AutoIncrement, which the platform guarantees is
unique across concurrent transactions.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* knowledge(events): serialize the setup-counter increment; promote article and wire the review skill
PR #152 round 3 (JesperSchulz):
- The good sample's OnAfterInsertEvent subscriber still raced on the
shared "Transfer Setup Good" singleton (Get/increment/Modify);
AutoIncrement only protected the request key. Added
TransferSetup.LockTable() before Get() to serialize concurrent
background sessions.
- Promoted changecompany-runs-triggers-in-the-calling-company from
community/knowledge/events/ to microsoft/knowledge/events/, and wired
ChangeCompany/StartSession/RunTrigger tokens plus a targeted
detection cue into microsoft/skills/review/al-events-review.md so a
diff containing the anti-pattern reliably worklists this article,
preserving the documented RunTrigger=false hand-off exception.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: waldo1001 <12088142+waldo1001@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Generate a deterministic action-skill index from frontmatter, publish structural schemas for orchestration and findings, and validate flat review composition in CI.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: dayland <dayland@microsoft.com>
Copilot-Session: 76eb42c4-2acd-4f9c-a898-f4a44f9d46f7
Use the required READ-convention Markdown links so knowledge retrieval can associate all new samples with their articles.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: dayland <dayland@microsoft.com>
Copilot-Session: 76eb42c4-2acd-4f9c-a898-f4a44f9d46f7
* Add bounded knowledge retrieval
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d8b7764-f15a-49ea-8d50-d9147334f8af
* Fix bc-version overflow and pathless-row identity in bounded retrieval
Catalog matching compared an Int32 -BCVersion against a bigint range bound.
PowerShell coerces the right operand to the left operand's type, so a bound
wider than Int32 threw a conversion error and failed the whole domain catalog
rather than the single row. Metadata validation already accepts such bounds,
so compare as bigint on both sides.
The shared pager built its oversized-row message with $row.path, which
throws under Set-StrictMode -Version Latest when a row carries no path,
replacing the explicit bound failure with a property-lookup error. Resolve the
path defensively for dictionary and object rows so the offset-based fallback
is reachable.
Both paths gain regression coverage that fails without these fixes.
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 App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d8b7764-f15a-49ea-8d50-d9147334f8af
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>
Remove historical naming and external orchestrator references, and present the app-folder review as one example of the broader host-native skill pattern.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Document standalone review runner contract
Keep model selection and scheduling outside BCQuality while allowing orchestrators to run isolated review leaves concurrently with deterministic rollup semantics.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Support complete app folder reviews
Define folder-path as a current-state review scope and accept it across the standalone adapter, broad coordinator, and every AL review leaf.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add walk-up app review quick start
Put the complete app-folder installation and prompt flow directly in the README so partners can discover the standalone experience without reading integration details first.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Organize conceptual guides under docs
Move architecture and standalone runner documentation out of the repository root, add a documentation index, and update all inbound links.
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 App <223556219+Copilot@users.noreply.github.com>
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
* Simplify standalone AL review skill
Rename the host-facing skill to al-code-review, reduce it to a thin Entry adapter, document the architecture, and validate host skill metadata.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: af96bb3d-893a-48a2-8298-c8f4271c162c
* Anchor plugin paths at PLUGIN_ROOT and restore the layer-filter caveat
The adapter delegated index preparation to Entry's Preparation step, but that
step is written for the clone model: it runs `pwsh ./tools/Build-KnowledgeIndex.ps1`
from the checkout root. A plugin host's working directory is the user's own
project, so the path does not resolve and the index is never built. Because
knowledge-index.json is gitignored, a fresh install has none, and READ silently
degrades to path-based discovery. The adapter now resolves Entry's repo-relative
paths against PLUGIN_ROOT and names the absolute index build; the generator
resolves its own root, so it indexes and writes the right tree from any cwd.
Entry also asserted that pruning has always happened before it runs, which is
false for an installation that ships the whole tree. Entry now scopes that
guarantee to consumers that actually prune, and the caveat dropped in the
rewrite - that enabled-layers narrows discovery rather than denying access - is
restored in the adapter and summarized in the README.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b67b90d-e4b4-4b92-9684-726c72c43b3f
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: af96bb3d-893a-48a2-8298-c8f4271c162c
Copilot-Session: 0b67b90d-e4b4-4b92-9684-726c72c43b3f
What an API page publishes for an enum field depends on the OData schema
version: member names under 2.0, the caption as Edm.String under 1.0, never
the ordinal. Custom APIs defaulted to 1.0 through BC 23 and to 2.0 from BC 24.
LLMs assert one carrier as universal and get one direction wrong.
Co-authored-by: waldo1001 <12088142+waldo1001@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
* Avoid Public Event publisher
* knowledge(events): scope public-publisher detection to same-app raisers
The detection rule flagged every public event publisher, including ones
deliberately public so a sibling app can raise them - a contract `internal`
cannot express across app boundaries. Scope the finding to publishers that
are public although only their own app raises them, and record the
cross-app case as a valid Best Practice option.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b67b90d-e4b4-4b92-9684-726c72c43b3f
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b67b90d-e4b4-4b92-9684-726c72c43b3f
* feat(community/agents): add AL agent quality guidance
- add 20 agent knowledge rules with good and bad AL samples
- clarify setup dialog shape, temporary persistence, permissions, profiles, instructions, capability registration, and interface wiring
- add the community-owned AL agents review skill
- make review fixture discovery layer-aware with custom, community, and Microsoft precedence
- document layer-aware evaluation behavior
* fix(community/agents): align setup and permission samples
- mark agent setup pages as non-extensible where required
- narrow the agent profile by hiding an unrelated sales-order field
- define a dedicated read-only permission set for the sales review agent
- assign AL-defined permission sets with system scope and the owning app ID
- clarify the permission scope guidance for default access controls
* Address agent review feedback
* 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): 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 narrowed UI-handler guidance still stated the execution rule without the
qualifier the linked Microsoft reference uses. The article said every listed
handler must execute at least once, and the testing leaf skill asked for
`[HandlerFunctions(...)]` to match the invoked handlers exactly. The reference
says every *nonoptional* listed handler must execute, and that send-notification
and recall-notification handlers can be optional. As written, an agent could
flag a deliberately unused optional notification handler.
The discriminator is narrower than the handler type. Both
`[SendNotificationHandler([HandlerIsOptional: Boolean])]` and
`[RecallNotificationHandler([HandlerIsOptional: Boolean])]` take an explicit
optionality argument, so `[SendNotificationHandler(true)]` is exempt while the
same attribute written without the argument stays nonoptional like every other
handler type. Keying the exemption on the argument rather than the type keeps it
checkable from the diff and avoids the opposite false positive, where an agent
stops flagging genuinely nonoptional notification handlers.
Changes:
- The article now states the nonoptional qualifier, explains that optionality is
declared rather than inferred, and adds an explicit do-not-flag clause. That
clause also forbids proposing removal, because the listed entry is what keeps
the test passing on the runs where the notification does fire.
- The testing leaf skill carries the same boundary in its `ui-handlers-in-tests`
cue, and its mechanical-fix list no longer allows removing a listed optional
notification handler as a one-click suggestion.
- `SendNotificationHandler` and `RecallNotificationHandler` were missing from the
skill's testing token list, so notification handlers were not reliably
surfaced to the relevance step at all. Both are now listed.
- The good sample gains a test that lists an unreached
`[SendNotificationHandler(true)]`; the bad sample gains the mirror image, an
unreached `[SendNotificationHandler]` with no optionality argument. The pair
differs only by that argument, which is the point.
- `evaluation/review-fixtures.json` pins the testing domain to
`ui-handlers-in-tests` so the boundary is exercised: the good sample is the
clean control at `minimumCleanRate` 1.0 and the bad sample is the expected
finding. Keywords were retagged with `notification` and `optional-handler`.
validate_frontmatter.py reports 0 errors; Test-ReviewFixtures.ps1 passes with
32 cases across 16 leaf domains and resolves the testing fixture to this article.
data-modeling: add insert-only-transfer-may-rely-on-caller-cleanup. A filter-and-insert transfer routine was reported for stale rows and duplicate keys even though the field OnValidate trigger calls a sibling cleanup procedure that clears the same range immediately before it. Deciding this requires reading the caller, so the article asks reviewers to trace call sites and keeps uncleared or mismatched-filter paths reportable.
appsource: scope two-level-namespace-replaces-object-affix-not-extension-member-affix to apps that actually configure a mandatory affix. AS0011 only runs when AppSourceCop is enabled with a mandatory affix; a first-party in-box app that ships no such configuration is not subject to it. The member-affix requirement itself is unchanged for apps that do configure one.
testing: allow permission-tests-must-lower-the-execution-context to accept a composed role. The article demanded the exact permission set under test be assigned directly, so a test that lowered permissions through a role including that set and then asserted WritePermission was false was reported as a coverage gap. What matters is the effective context plus a boundary assertion, not which object the test names.
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
The article was rewritten to say a reset is required only when the value can
carry over, and its H1 was updated to match, but three artefacts still carried
the old "always initialize to false" premise:
- The slug still read `initialize-ishandled-to-false-before-publishing`, which
contradicts the body. The slug is not cosmetic: Build-KnowledgeIndex.ps1
ranks candidates on keywords, frontmatter dimensions, domain, path and title,
so a stale path pushes selection back toward the behaviour this change
narrows. Renamed to `reset-ishandled-only-when-the-value-can-carry-over`,
following the existing precedent for conditional slugs such as
`unreleased-symbol-change-is-not-a-breaking-change`.
- Keywords still listed `initialization` and `deterministic` and omitted
`false-positive`, the tag this repository uses for suppression articles.
Replaced with `carry-over` and `loop-iteration` and added `false-positive`.
- The good sample demonstrated only the "prefer separate fresh locals" clause
and contained no reset at all, so the article's headline case had no positive
example. It was also asymmetric with the bad sample, which gained a loop
procedure showing a local that carries `true` into the next iteration.
Added the matching loop procedure to the good sample: a local declared outside
the loop is reset at the top of each iteration. That case cannot be solved by
introducing another local, because AL has no block scope, so it is the only
shape that demonstrates the reset the article still requires. It also gives the
engine the correct `suggested-code` shape for the loop finding; without it the
one-click fix adapted from the good sample would propose splitting the variable
rather than adding one line.
Also renamed the sample codeunits from "IsHandled Init ..." to
"IsHandled Carry Over ...", and updated the two references to the old slug:
the events leaf skill cue and the events pin in evaluation/review-fixtures.json.
validate_frontmatter.py reports 0 errors; Test-ReviewFixtures.ps1 passes with
32 cases across 16 leaf domains and resolves the events fixture to the renamed
article.
