The review agent repeatedly flagged bare calls to [TryFunction] procedures
(e.g. the System Application "Xml Validation" Try* APIs) as defects,
including claims that the failure is "silently swallowed". A bare call is
an ordinary call: the error propagates as usual.
- Add negative knowledge bare-tryfunction-call-propagates-errors.md.
- Narrow ignored-tryfunction-return-disables-try-semantics to code that
visibly expects the failure to be caught; the bad sample now shows that
in code, and the good sample includes an intentional bare call as the
clean control.
- Qualify "TryFunction catches all errors" in the events article and the
"must be consumed" wording in the performance article.
- Error-handling leaf worklists both articles for bare [TryFunction] calls
and requires the same evidence before flagging.
- Add the three paired articles to the evaluation overrides.
* Add 18 more community AL/BC patterns across appsource, data-modeling, error-handling, security, style, testing, ui, upgrade, and web-services
Second contribution from CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Cross-checked against the current microsoft/knowledge corpus before opening; several originally-drafted candidates were dropped as duplicates of existing files.
* Address Jesper Schulz-Wedde's review on PR #157
- release-must-update-app-version.md: reframe around AppSource's actual
strict full-version-ordering requirement; scope branching-policy
claims as team convention, not platform rule.
- pictures-must-use-media-not-blob.md: MediaSet is a collection of
independent media objects, not automatic image variants/thumbnails.
- log-writes-must-survive-rollback.{md,good.al}: StartSession's only
data channel into the new session is its Record parameter to a
TableNo-scoped codeunit; a setter called on a local instance before
starting the session populates nothing in the new session.
- exposed-objects-must-be-in-a-permission-set.md: correct the three
exposure mechanisms (Web Services config, PageType/QueryType=API,
ServiceEnabled as a method-only attribute).
- pages-must-not-contain-business-logic.md: scope to persisted
mutations and cross-entry-point rules; presentation-only
calculations and table-owned invariants are not violations.
- given-blocks-must-cover-full-precondition-chain.good.al: replace
invented LibrarySales calls with the real API
(CreateCustomer/CreateSalesOrderForCustomerNo/PostSalesDocument).
- test-feature-scenario-tags.{md,good.al}: move [SCENARIO] inside the
test procedure body to match the current BCApps corpus; keep
[FEATURE] at codeunit level per Microsoft's own documented option.
- ui-test-codeunit-naming.md: scope the _UT suffix and adjacent-ID
pairing as an explicit team convention, not a BCApps-wide standard.
- page-design-must-match-bc-page-type-conventions.md /
table-design-must-match-bc-table-type-conventions.md: Card's
single-key primary-key claim is a contextual heuristic, not a
mandatory constraint (Ship-to Address, Customer/Vendor Bank Account
are real composite-key Card pages); a Subsidiary table with its own
identity commonly gets List+Card, not Worksheet/Tabular.
- api-page-least-privilege-write-access.{md,good.al}: only page-placed
fields are ever exposed; set InsertAllowed/DeleteAllowed=false in the
good sample so a narrow field set can't still create/delete records.
- source-organized-by-feature-not-object-type.md,
test-one-when-per-test.md: scope as team/testing-design conventions,
not Microsoft platform requirements.
- upgrade-tag-logic-must-not-nest-deeply.md: add the Microsoft Learn
citation that already backs the two-level nesting limit.
- Wire the new articles into the testing/data-modeling/error-handling/
security/ui review skills' candidate-selection signals.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Address second round of Jesper Schulz-Wedde's review on PR #157
- log-writes-must-survive-rollback.good.al: fixed invalid trigger
OnRun(var Rec: ...) declaration; Rec is implicit when TableNo is set.
- exposed-objects-must-be-in-a-permission-set.md: distinguished the three
exposure mechanisms (page/query web service or API, codeunit published
as a web service, [ServiceEnabled] bound action on a page) and their
actual permission targets (page/query "..." = X vs codeunit "..." = X).
- code-must-not-change-workdate.md: scoped from an absolute "never" to
"not as a side effect of unrelated logic" - verified real WorkDate(x)
setter usage in BCApps demo-data generators and test codeunits.
- bcpt-scenarios-must-be-app-specific.md: SingleInstance and
StartScenario/EndScenario reframed as context-dependent patterns, not
mandatory requirements - BCPT Create Customer uses neither.
- test-feature-scenario-tags.good.al/.bad.al: replaced the invented
LibrarySales.CreateCustomerWithPrice/"Item Price Mgt." calls with a real,
verified price-list-line test using Library - Sales/Library - Inventory/
Library - Price Calculation.
- page-design-must-match-bc-page-type-conventions.md: scoped the missing
UsageCategory anti-pattern to pages intended as searchable entry points.
- defensive-vs-offensive-code-must-match-blast-radius.md/.good.al/.bad.al:
replaced the VAT registration number "low blast radius" example with a
genuinely cosmetic field (customer home page URL).
- source-organized-by-feature-not-object-type.md: anti-pattern reframed as
inconsistency with a repo's own convention, not the object-type scheme
itself.
- pictures-must-use-media-not-blob.md: removed leftover "image variants"
wording contradicting the already-corrected MediaSet description.
Proactively fixed while sweeping all fixtures for invented APIs:
- given-blocks-must-cover-full-precondition-chain.bad.al: PostSalesOrder
called with wrong arity and referenced an undeclared variable.
- ui-test-codeunit-naming.good.al/.bad.al: replaced the same fake
"Item Price Mgt."/TestPage "Item Price" with real Library - Sales calls
and the real Customer Card TestPage.
Worklist completeness: added review-skill cues for the 12 of 18 new rules
that had none (al-appsource-review.md, al-data-modeling-review.md,
al-error-handling-review.md, al-security-review.md, al-style-review.md x3,
al-testing-review.md x2, al-ui-review.md, al-upgrade-review.md,
al-web-services-review.md), and fixed test-feature-scenario-tags' cue,
which only matched the compliant (tagged) shape instead of the anti-pattern
(untagged/generic-named test).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix ten focused correctness items plus sample links from Jesper's 2026-09-15 re-review
Six carried-over threads:
- api-page-least-privilege-write-access fixtures: added the mandatory
EntityName/EntitySetName properties (AL0485).
- pages-must-not-contain-business-logic fixtures: Sales Line has no
"Total Amount" field; replaced with the real "Line Amount" (field 103).
- test-feature-scenario-tags.good.al and test-one-when-per-test.good.al:
CreatePriceHeader leaves a price list in Draft status, which price
calculation ignores. Added Validate(Status, Active) + Modify before the
sales line that depends on it. Verified Status field/enum against
PriceListHeader.Table.al and PriceStatus.Enum.al in the BCApps clone.
- exposed-objects-must-be-in-a-permission-set.md: a published codeunit is
a SOAP endpoint (SOAP is deprecated), not OData - Page/Query are the
OData object types. Corrected and pointed new integrations at API
pages/queries instead.
- al-error-handling-review.md: the log-writes-must-survive-rollback cue
selected on Session.StartSession, which only appears in the compliant
fix, never in the anti-pattern - the bad fixture could never be
worklisted. Recued on the actual risk shape (log insert around a
failed TryFunction/GetLastError* path, then raise/propagate), with
StartSession as an explicit compliant discriminator instead.
- page-design-must-match-bc-page-type-conventions.md: the enum value is
NavigatePage, not Navigate; noted the type list is a selected subset,
not an exhaustive PageType catalogue (PromptDialog, ConfigurationDialog,
UserControlHost, XmlPort also exist, out of this article's scope).
Four new correctness gaps:
- release-must-update-app-version.md: "the version is the only identity"
was backwards - id is the app's stable identity, version identifies a
release/code-state of it.
- defensive-vs-offensive-code-must-match-blast-radius.good.al: the "low
blast radius" example had no else branch, so a failed Customer.Get()
left the field at its prior/default value instead of the explicit
chosen fallback the article claims to demonstrate. Added the else.
- bcpt-scenarios-must-be-app-specific.good.al: InitTest and both measured
StartScenario/EndScenario sections were empty/comment-only, so the
"app-specific" fixture measured no actual work. Filled in a real,
self-contained header+line creation path.
- upgrade-tag-logic-must-not-nest-deeply.good.al: the flattened version
dropped both safety conditions the bad fixture had (Discount % = 0,
nonblank posting group), silently changing behavior instead of just
removing nesting. Extracted the guarded update into a helper with both
conditions preserved as early exits.
Also converted this PR's remaining plain-backtick "See sample:" sample
references (16 articles) to the READ-convention markdown-link form,
matching the fix already made on #156/#158.
Rebased onto upstream/main (conflicts in al-ui-review.md, al-style-review.md,
al-upgrade-review.md against merged upstream PRs - all additive, both
sides' worklist cues retained).
* Fix four merge-critical issues from Jesper's 2026-09-22 review
- pages-must-not-contain-business-logic.good.al/.bad.al: the "good"
codeunit still directly assigned real Sales Line."Line Amount" and
called Modify(), bypassing the field's normal Validate cascade
(discount, VAT, related-amount maintenance) - persisting inconsistent
document lines regardless of which object the code lived in. Replaced
the real Sales Line example with a self-contained "Sample Order Line"
table and switched the codeunit to Validate()/Modify(true), so the
fixture demonstrates the page-vs-codeunit separation without teaching
unsafe direct field writes to a real BC document table.
- bcpt-scenarios-must-be-app-specific.good.al: Customer.FindFirst()
assumed a pre-existing customer (fails against an empty environment),
and a session-local NextNo counter for the header key collides across
concurrent BCPT sessions and repeated runs. Creates its own customer
when none exists, and generates keys from CreateGuid() instead of an
in-memory counter.
- upgrade-tag-logic-must-not-nest-deeply: the rule conflated two
different things - nesting one tag's existence check inside another
(the real anti-pattern Microsoft's guidance warns against) with
having business-data safety conditions inside a single tagged
migration's own loop body (which Microsoft's own worked example does,
and its own design guidance explicitly requires: "Implement extra
safety checks to avoid data corruption, even though you're using
upgrade tags"). Rewrote the Description/Best Practice/Anti Pattern to
scope the rule to actual tag nesting and migrations blended under one
tag, and rewrote both fixtures: good.al now shows two safety
conditions correctly nested inside one migration's own loop plus a
second, genuinely separate migration as its own flat tagged
procedure; bad.al now shows the real anti-pattern, one tag's check
nested inside another's guarded body.
- table-design-must-match-bc-table-type-conventions: the rule and its
worklist cue fired on any new table with a keys block, forcing
buffers, queues, logs, mapping tables, and staging tables into the
nearest-looking one of nine business-record archetypes. Added an
explicit scope note that these nine types aren't an exhaustive table
catalogue, and narrowed the al-data-modeling-review.md cue to require
positive evidence (a type-specific naming suffix, key shape, or
usage) before worklisting, instead of a bare keys/primary-key
declaration.
* Narrow upgrade-tag nesting cue to match revised article
Cue now flags only nested upgrade-tag checks or functionally unrelated
migrations under one tag, and explicitly excludes record loops and
business-data safety guards belonging to a single migration.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- upgrade/obsoletereason-need-not-restate-removal-version: ObsoleteTag carries the version; do not flag ObsoleteReason for omitting it (PR 8290)
- error-handling/unchecked-get-throws-when-record-not-found: a bare Rec.Get() errors on missing record; it is not silently ignored (PR 8584)
- performance/onaftergetcurrrecord-is-not-per-row: OnAfterGetCurrRecord fires on selection change, not per row; CalcFields there is not N+1 (PR 8617)
* 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>
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>
A closed range like [23..28] wrongly implies guidance stops applying after
BC28, so a reviewer targeting BC29+ would not match the file. Introduce an
open-ended shorthand [N..] meaning ''version N and every later version''.
- validate_frontmatter.py: RANGE_SHORTHAND allows an optional upper bound;
expand_bc_version returns the normalized string ''N..'' for open-ended.
- read.md: document the fourth bc-version form and its matching rule
(matches target >= N; not enumerable).
- write.md: prefer [N..] over a closed range for a feature introduced in N
and not expected to be removed.
- Apply [23..] to the actionable-errors article (actionable errors shipped
in BC23 and are not version-bounded above).
- README: mention [N..] in the frontmatter example.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
