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).
- 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>
- 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>
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.
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>
* knowledge(style): allow event subscribers to omit trailing publisher parameters
The article told reviewers to copy the publisher signature exactly and reproduce every parameter verbatim. That contradicts events/add-new-event-parameters-at-the-end, which already states that existing subscribers bind to the leading parameters, and it produced a false positive on BCApps PR 10277 where a subscriber legitimately declared only the leading two of the publisher's three parameters.
Clarify that the name-match rule applies to every parameter the subscriber declares, state that AL binds on a leading prefix so trailing parameters may be omitted, and keep the real defect - a subscriber list that is not a prefix of the publisher's - as the anti pattern. Add the false-positive keyword for retrieval.
* fix: subscribers bind by parameter name, not by leading prefix
The first revision claimed AL binds a subscriber to a leading prefix of the publisher parameter list and that a parameter may not be skipped in the middle. That is wrong. Verified against shipping BCApps code: Test Runner - Mgt publishes OnBeforeTestMethodRun(var CurrentTestMethodLine; CodeunitID; CodeunitName; FunctionName; FunctionTestPermissions; var Skip), and ALTestRunnerResetEnvironment binds to it declaring (CodeunitID; CodeunitName; FunctionName; FunctionTestPermissions; var CurrentTestMethodLine) - omitting Skip and moving the first parameter to last. Binding is by name, so any subset in any order is valid. Detection now targets a parameter whose name or type matches nothing on the publisher.
---------
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
The v1.4 policy deferred every missing-tooltip case to compiler analyzer AA0218. But AA0218 severity is per-app ruleset config and is routinely downgraded to info/None or disabled, so a genuine gap can ship unflagged. PR review is the last line of defence and should raise it independently.
Keeps the false-positive guard intact: a bound field whose source table field supplies a ToolTip still inherits it and is NOT flagged. Adds the genuinely-missing case (bound field whose source is also tooltip-less, or an unbound control) as a medium-severity finding. Updates both the ui inheritance article and the style AA0218 article.
* 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>
Author 7 remedial BCQuality knowledge articles plus good/bad AL samples
(21 files) covering AL master-table and data-model design:
- data-modeling: master No. from number series in OnInsert; use codeunit
"No. Series" not obsolete NoSeriesManagement; setup table is a singleton;
set Last Date Modified in OnModify and OnRename; enforce Blocked in
referencing code not in the master.
- style: ApplicationArea required on page controls (AS0062).
- appsource: object affixes prevent collisions (AS0011).
Clean-room authored from own BC knowledge; specifics verified against public
sources only (learn.microsoft.com, microsoft/BCApps). Introduces two new
domains (data-modeling, appsource).
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>
Adds the contract field, skill instructions, and two knowledge articles
that BCAppsBCQuality's PR-review agent needs to match (and exceed) the
coverage of the embedded review agent in BCAppsCampAIRHack:
skills/do.md
- New optional findings[].suggested-code field. Documents what it MUST
contain (a literal line-replacement payload) and when to emit it.
microsoft/skills/review/al-code-review.md
- Instructs both the agent self-review pass and rolled-up sub-skill
findings to populate suggested-code when the fix is mechanical.
- Lists examples (dead code removal, Count > 0 -> IsEmpty, object-scope
Label) that map to issues observed in the parity comparison.
microsoft/knowledge/style/telemetry-event-id-stable-unique.{md,bad.al,good.al}
- New knowledge article: telemetry event IDs must be stable, unique,
and non-placeholder. Closes a gap surfaced by the parity comparison.
microsoft/knowledge/style/labels-declared-at-object-scope.{md,bad.al,good.al}
- New knowledge article: Labels must live in the object-level var
block, not in procedure-local var blocks. Closes the second gap.
microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.md
- Adds an explicit note that changing DataClassification alone does not
make embedding PII into the message string acceptable, plus links to
the two adjacent privacy articles. Resolves the privacy advice the
parity comparison flagged as ambiguous.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
Adds 55 articles (plus 76 code samples) spanning four new domains and
two existing domains, extracted from the internal Business Central
review-agent prompt. Content was filtered against BCQuality's
remedial-knowledge premise: each article encodes BC-specific behaviour,
a CodeCop rule, a platform API semantic, or an anti-false-positive
guideline that a capable LLM would otherwise get wrong.
New domains:
- privacy (11 articles): DataClassification inheritance semantics, the
StrSubstNo-defeats-Error-telemetry-classification pitfall, Privacy
Notice consent for outgoing requests, anti-false-positives for pages
and in-memory data.
- upgrade (11 articles): upgrade-codeunit structure, upgrade-tag
lifecycle and registration, protected DB reads, DataTransfer for
large datasets, InitValue semantics, enum-ordinal preservation,
obsolete-workflow, first-install detection.
- ui (9 articles): caption capitalization by phrase type, tooltip voice,
teaching-tip vs tooltip, tour-tip conventions, character limits,
banned terms, ampersand handling, title punctuation.
- style (11 articles): label-suffix convention, API page naming,
temporary-variable prefix, label properties (Comment/Locked), named
invocations, FieldCaption in user messages, OptionCaption pairing,
Error-parameter passing, `this` keyword, required parentheses, file
naming.
Gaps in existing domains:
- performance (11 articles): production-scale table catalog (no row
counts, per internal-data concern), anti-false-positive for bounded
tables, guard-before-Get ordering, redundant-Get-in-OnAfterGetRecord,
LockTable in read-only helpers, combined ModifyAll passes, writes in
OnAfterGetRecord, SetLoadFields heuristics, temporary-table
regressions, FlowField source-table widening, MaintainSQLIndex
disabling SIFT.
- security (2 articles): environment-specific hardcoded GUIDs,
ValidateTableRelation=false on user input.
Intentionally excluded: specific production P95 row-count numbers
(aggregated internal telemetry); rewritten as categorical guidance on
which tables to treat as production-scale without publishing sizes.
All articles use `bc-version: [all]` (applies to every BC version, per
the new schema sentinel). Validator passes with 0 errors / 0 warnings.
