From cdd3d99cf83f956cded040259bc0e7d3668197b2 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Wed, 15 Jul 2026 14:57:17 +0200 Subject: [PATCH 01/10] review: stop emitting reference-less agent findings in privacy and UI/accessibility leaves Online eval (182 PRs) shows the reference-less agent-finding channel is where 86% of false positives come from, and it is net-negative in the lowest-yield domains: privacy agent findings score 0 TP / 8 FP and UI/accessibility 1 TP / 11 FP. Restrict these two leaves to knowledge-backed findings only; an uncovered concern is omitted (and, if material and recurring, fixed durably by adding a BCQuality article per the self-improvement loop) instead of emitted with references: []. --- microsoft/skills/review/al-privacy-review.md | 2 +- microsoft/skills/review/al-ui-review.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/microsoft/skills/review/al-privacy-review.md b/microsoft/skills/review/al-privacy-review.md index edbf197..ea95269 100644 --- a/microsoft/skills/review/al-privacy-review.md +++ b/microsoft/skills/review/al-privacy-review.md @@ -68,7 +68,7 @@ Set `confidence` to: - `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`. - `low` when the finding is an advisory derived only from applicability. -After evaluating each worklist entry, also consider whether the diff exhibits a privacy defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material privacy defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly privacy; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. +This leaf emits only knowledge-backed privacy findings. Do NOT emit reference-less `agent:` findings in this domain: online evaluation shows the privacy agent-finding channel yields almost no accepted findings and a high volume of dismissed noise, so a privacy concern that no worklist knowledge file covers is omitted here rather than emitted with `references: []`. When you spot a material privacy defect no article covers, the durable fix is to add a knowledge article in BCQuality (per the online-eval self-improvement loop) so this leaf can cite it — not a one-off reference-less finding. Before treating a candidate as uncovered, check the worklist for a knowledge file that matches it; if one exists, emit it as a knowledge-backed finding. See `skills/do.md` for the full contract. For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index 19f03c9..81ab12e 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -61,7 +61,7 @@ Set `confidence` to: - `medium` when detection relies on heuristics (judging whether a caption is a noun phrase or a sentence phrase) or when any frontmatter dimension was `unknown`. - `low` when the finding is an advisory derived only from applicability. -After evaluating each worklist entry, also consider whether the diff exhibits a UI defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material UI defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly UI; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. +This leaf emits only knowledge-backed UI and accessibility findings. Do NOT emit reference-less `agent:` findings in this domain: online evaluation shows the UI/accessibility agent-finding channel yields almost no accepted findings and a high volume of dismissed noise, so a UI or accessibility concern that no worklist knowledge file covers is omitted here rather than emitted with `references: []`. When you spot a material UI or accessibility defect no article covers, the durable fix is to add a knowledge article in BCQuality (per the online-eval self-improvement loop) so this leaf can cite it — not a one-off reference-less finding. Before treating a candidate as uncovered, check the worklist for a knowledge file that matches it; if one exists, emit it as a knowledge-backed finding. See `skills/do.md` for the full contract. For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. From b9c57ba6f993feeac860c24808865045be926167 Mon Sep 17 00:00:00 2001 From: Jesper Schulz-Wedde Date: Fri, 17 Jul 2026 12:15:22 +0200 Subject: [PATCH 02/10] Document the skill-vs-knowledge boundary so BC facts land in knowledge files (#114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewing #112 surfaced that the authoring docs never state where a new BC fact (or a false-positive guard) belongs, so an agent iterated do.md -> leaf skills -> knowledge files across two review rounds before landing knowledge in a knowledge article. The information to decide existed but was split across README/do.md/write.md and framed only as positive best practices. - do.md: add "Skills hold mechanics; knowledge files hold BC facts" — a skill is a finder/applier; every BC behavioural claim it acts on must be a cited knowledge file. Names negative knowledge (false-positive guards) as first class, and forbids both adding a BC fact to a skill and restating an article's fact inline (the drift/duplication smell). - write.md: add "Is this a knowledge file?" decision gate at the top, plus a "Negative knowledge is first-class" section with the Description/Best Practice/Anti Pattern mapping and a worked example. - README: note that false-positive-preventing files are first-class knowledge and add a reviewer heuristic to Contributing. Prose-only additions; validator passes. Meta-skill contract semantics are unchanged, so version stays 1 (maintainers may bump if they consider the explicit boundary rule a contract change). Copilot-Session: 76eba53e-18cd-4618-a205-3607f260f9f4 Co-authored-by: Jesper Schulz-Wedde Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 3 +++ skills/do.md | 13 +++++++++++++ skills/write.md | 19 +++++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/README.md b/README.md index 3b09ff7..b939c6f 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,8 @@ Poor fit: "Use HTTPS instead of HTTP." "Don't hardcode secrets." "Keep transacti The practical consequence: when a code-review agent flags something it shouldn't have, or misses something it should have caught, the remedy is a new knowledge file. When it already behaves correctly on a topic, no file is needed. +A file that *prevents* a false positive — documenting why a pattern is legitimate so the agent stops flagging it — is as valid as one that catches a defect: negative clarifications are first-class knowledge files. What never belongs is a BC fact hard-coded into a skill. Skills are finders and appliers; knowledge files are what the agent knows. See [`skills/do.md`](skills/do.md) and [`skills/write.md`](skills/write.md). + ## What's in this repo BCQuality contains **knowledge** and **skills**. It does not contain agents. Agents that consume BCQuality ship with [AL-Go](https://github.com/microsoft/AL-Go) and other orchestrators. @@ -147,6 +149,7 @@ Contributions are welcome. Before submitting a PR: 1. Read the knowledge file format above — frontmatter and sections are validated by CI. 2. Keep files atomic: one concern per file, under 100 lines. 3. Target your contribution to the right layer — most community contributions go in `/community/knowledge/`. +4. Adding a BC fact — or stopping the agent from flagging a false positive — is a knowledge file, not a skill edit. If a PR changes *what* a review skill flags, the change almost certainly belongs in a knowledge file. See [`skills/write.md`](skills/write.md). CI runs validation on every PR. If your knowledge file has schema violations, missing sections, code blocks, or exceeds 100 lines, the check will fail with a clear error message. diff --git a/skills/do.md b/skills/do.md index 9de11a4..23bfb4a 100644 --- a/skills/do.md +++ b/skills/do.md @@ -21,6 +21,19 @@ An action skill is a single markdown file with YAML frontmatter. It lives inside Action skills do not live at the repo root. The files in `/skills/` — the three meta-skill contracts (READ, DO, WRITE) and the entry-point skill (`entry.md`, `kind: entry-point`) — are the only skills that sit outside a layer. The entry-point skill structurally follows this same four-step pattern but produces a dispatch record rather than a findings-report; see `skills/entry.md` for its contract. +## Skills hold mechanics; knowledge files hold BC facts + +An action skill is a *finder and applier*: its prose says how to discover candidate knowledge (Source), filter it (Relevance), narrow it to the task (Worklist), and shape output (Action). Every Business-Central-specific behavioural claim a skill acts on — what a property defaults to, what a trigger does, why a given shape is or is not a defect — belongs in a knowledge file the skill cites, not in the skill prose. + +This includes **negative knowledge**. A false-positive guard — "pattern X is not a defect, because BC does Y" — is as much a knowledge file as a positive best practice. When an eval shows the agent over-reporting a pattern, the fix is a knowledge file documenting why the pattern is legitimate, so the skill can cite it and any leaf can reuse it — not a hard-coded exclusion buried in one skill. See `skills/write.md` (*Is this a knowledge file?*). + +Two rules follow for skill authors: + +- **Do not add a BC fact to a skill.** If you are editing a skill to change *what it flags* — adding an exclusion, encoding a platform default, teaching it that some pattern is fine — you are holding a knowledge file, not a skill edit. Author the knowledge file and let Worklist route to it. +- **Do not restate an article's fact inline.** A Worklist cue may name the article to load and the diff shape that selects it; it must not re-assert the article's reasoning, which then drifts from the source. Cite, don't copy. + +The meta-skills themselves (`read.md`, `do.md`, `write.md`) are domain-agnostic templates and carry no BC-specific rule. + ## Frontmatter schema ```yaml diff --git a/skills/write.md b/skills/write.md index a9e3c31..731f469 100644 --- a/skills/write.md +++ b/skills/write.md @@ -9,6 +9,25 @@ title: New Knowledge — how to author a knowledge file Anyone — human or agent — adding a knowledge file to BCQuality follows this guide. READ is the format specification; WRITE is the authoring guide. This file does not restate the schema; consult READ for field-by-field semantics. +## Is this a knowledge file? + +Before authoring anything, confirm a knowledge file is the right artifact. BCQuality separates *mechanics* from *facts*: + +- **Skills** (`*/skills/**`) hold only finder/applier mechanics — how to discover, filter, worklist, and emit findings. See `skills/do.md`. +- **Knowledge files** (`*/knowledge/**`) hold every Business-Central-specific fact a skill acts on. + +A new BC fact is therefore a knowledge file, never a skill edit. In particular, if you arrived here because a review agent flagged something it should not have (a false positive) or missed something it should have caught, the remedy is a knowledge file — apply the admission test in the [README](../README.md#what-belongs-here): *would a capable LLM get this wrong without the file?* If you find yourself editing a skill to stop it flagging something, stop and write a knowledge file instead. + +### Negative knowledge is first-class + +A knowledge file does not have to recommend an action. A **negative clarification** — "pattern X is *not* a defect, because BC behaves as Y" — is a first-class knowledge file, authored exactly like a positive rule: + +- **Description** states the BC behaviour that makes the pattern legitimate. +- **Best Practice** tells the reviewer or agent what *not* to flag, and why. +- **Anti Pattern** describes the false-positive report itself — the mistaken finding to suppress. + +For example, `microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md` records that the Boolean page record triggers return `true` by default, so a "missing `exit(true)`" report is not a real defect. It reads as ordinary knowledge; its anti-pattern is the incorrect review comment, not the code. + ## Before you start Read `skills/read.md` first. A file that does not conform to READ will be rejected. WRITE assumes READ is already understood. From 1bf5a3b2764111945051c3881dd0ec338484b3a5 Mon Sep 17 00:00:00 2001 From: Wenjie Fan <31087545+gggdttt@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:17:04 +0200 Subject: [PATCH 03/10] Add precision guards for systematic agent false-positive patterns (#112) * 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 --- ...-symbol-change-is-not-a-breaking-change.md | 24 +++++++++++++++++++ .../page-boolean-triggers-default-to-true.md | 24 +++++++++++++++++++ ...oth-getrecord-triggers-is-not-redundant.md | 22 +++++++++++++++++ ...ective-filter-may-live-outside-the-diff.md | 22 +++++++++++++++++ ...age-field-inherits-source-field-tooltip.md | 24 +++++++++++++++++++ ...sed-schema-change-needs-no-upgrade-path.md | 24 +++++++++++++++++++ 6 files changed, 140 insertions(+) create mode 100644 microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md create mode 100644 microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md create mode 100644 microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md create mode 100644 microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md create mode 100644 microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md create mode 100644 microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md diff --git a/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md b/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md new file mode 100644 index 0000000..fc39c13 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [released-baseline, unreleased, rename, renumber, obsolete, api-stability, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Changing an unreleased symbol is not a breaking change + +## Description + +Breaking-change rules protect contracts that have already shipped to customers or are exposed to external extensions. A symbol — an object, field, key, enum value, or procedure — that is new in this app, was introduced and then changed within the same still-unreleased development cycle, or belongs to an app that has no released version yet, can be renamed, renumbered, or removed freely. There is no shipped contract to break, so the change is not a breaking change. + +Release status is established from the diff, the app's `app.json` version, or a released baseline. An app whose `app.json` version has no corresponding released baseline (for example a `1.0.0.0` app that has never shipped) has no protected surface. + +## Best Practice + +Before treating a rename, renumber, or removal as breaking, establish that the affected symbol was present in a released baseline. Do not flag changes to symbols that are new in the current unreleased cycle or that belong to an app with no released version. When release status cannot be established from the diff, `app.json`, or a released baseline, omit the finding rather than assert a break. + +## Anti Pattern + +Reporting a breaking change for a rename, renumber, or removal without confirming the symbol shipped in a released version — for example flagging a break on an app whose `app.json` version has no released baseline. diff --git a/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md b/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md new file mode 100644 index 0000000..f637fae --- /dev/null +++ b/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [oninsertrecord, onmodifyrecord, ondeleterecord, onquerypage, boolean-trigger, exit, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Page record triggers return true by default; a missing exit(true) does not block the operation + +## Description + +The Boolean page record triggers `OnInsertRecord`, `OnModifyRecord`, `OnDeleteRecord`, and `OnQueryClosePage` return `true` by default. When the trigger body omits an explicit return value, the platform treats the result as `true` and the operation proceeds. Only an explicit `exit(false)` — or a reachable code path that returns `false` — cancels the insert, modify, delete, or page close. + +This is a defined exception to the ordinary Boolean method rule, where the default return is `false`. Reviewers unfamiliar with the exception sometimes read a page record trigger that has no `exit(true)` and conclude the operation is blocked; it is not. + +## Best Practice + +Do not claim that a missing `exit(true)` blocks or prevents an insert, modify, or delete, and do not recommend adding `exit(true)` "to let the operation proceed" — that is already the default. Evaluate these triggers only for an explicit or reachable `exit(false)`/false-returning path that would cancel the operation unintentionally. + +## Anti Pattern + +Flagging `OnInsertRecord`, `OnModifyRecord`, `OnDeleteRecord`, or `OnQueryClosePage` as defective because it "does not return `true`", or asserting that inserts/modifies/deletes will silently fail without an explicit `exit(true)`. The default return already permits the operation. diff --git a/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md b/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md new file mode 100644 index 0000000..79c3b52 --- /dev/null +++ b/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md @@ -0,0 +1,22 @@ +--- +bc-version: [all] +domain: performance +keywords: [calcfields, onaftergetrecord, onaftergetcurrrecord, page-lifecycle, flowfield, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# CalcFields in both OnAfterGetRecord and OnAfterGetCurrRecord is not redundant + +## Description + +`OnAfterGetRecord` fires once per row as the page loads records into the view; `OnAfterGetCurrRecord` fires when a record becomes the active/current record. Calling `CalcFields` in both triggers is not duplicate or redundant work: the two triggers run at different points in the page lifecycle and serve different purposes — populating FlowFields for every displayed row versus refreshing them for the record the user has selected. The same `CalcFields` call appearing in both places is an intentional pattern, not copy-paste waste. + +## Best Practice + +Do not flag `CalcFields` appearing in both `OnAfterGetRecord` and `OnAfterGetCurrRecord` as duplicate, redundant, or removable. Treat each trigger's `CalcFields` on its own lifecycle merits. + +## Anti Pattern + +Recommending that a developer delete one of the two `CalcFields` calls because "the field is already calculated in the other trigger". The genuine per-row FlowField cost is addressed by the separate guidance on FlowField calculation in loops and on hidden FlowFields; it is not addressed by removing a lifecycle-correct `CalcFields`. diff --git a/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md b/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md new file mode 100644 index 0000000..50eb014 --- /dev/null +++ b/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md @@ -0,0 +1,22 @@ +--- +bc-version: [all] +domain: performance +keywords: [filter, drilldown, lookup, sourcetableview, tablerelation, setrange, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A page or lookup's effective filter may be defined outside the changed hunk + +## Description + +The effective filter on a drill-down, lookup, or list result set is frequently defined outside any single changed hunk — on the table via a `SourceTableView` property or a `TableRelation`, or through `SetRange`/`SetFilter` calls in unchanged code that runs before the result is shown. The absence of a filter within the changed lines of a diff is therefore not evidence that the result set is unfiltered or that it will load an entire table. + +## Best Practice + +Do not assert that a drill-down, lookup, or list is "unfiltered" based only on the changed hunk. Confirm the effective filter by checking the page's `SourceTableView`, the field's `TableRelation`, and any `SetRange`/`SetFilter` in the surrounding (possibly unchanged) code before raising a finding about an unbounded result set. + +## Anti Pattern + +Concluding that a lookup or drill-down loads an unfiltered, full-table result set solely because no `SetRange`/`SetFilter` appears in the changed lines, when the filter is defined on the table, in a `TableRelation`, or in unchanged setup code. diff --git a/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md new file mode 100644 index 0000000..a258506 --- /dev/null +++ b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: ui +keywords: [tooltip, page-field, source-field, inheritance, aa0218, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A page field bound to a table field inherits that field's ToolTip + +## Description + +A page field bound to a table field inherits the source field's `ToolTip` at runtime: the control shows the table field's `ToolTip` even when the page control declares none of its own. A page field without an inline `ToolTip` is therefore not, by itself, a missing-tooltip defect — the text may be supplied by the bound source field. + +The genuinely-missing case — a bound field whose source table field also carries no `ToolTip`, or an unbound control that needs one — is already reported by the compiler analyzer AA0218, which BCQuality calibrates to `info`. That analyzer, not an agent finding, owns the missing-tooltip signal. + +## Best Practice + +Do not raise a missing-`ToolTip` finding for a page field that has a source-table binding; assume the source field supplies the tooltip. Reserve tooltip findings for the cases the dedicated tooltip rules define, and let analyzer AA0218 carry the mechanically-detectable missing-tooltip case at its calibrated severity. + +## Anti Pattern + +Flagging every page field that has no inline `ToolTip` property as an accessibility violation, ignoring that a bound field inherits its source field's tooltip and that AA0218 already covers the truly-missing case. diff --git a/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md b/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md new file mode 100644 index 0000000..248943f --- /dev/null +++ b/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [released-baseline, unreleased, schema, migration, obsolete, data-loss, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Unreleased schema changes need no upgrade or migration path + +## Description + +Upgrade and migration findings protect data and schema that have already shipped to customers. A schema element — a table, field, key, or enum — that is new in this app, or was added and then changed within the same still-unreleased development cycle, needs no upgrade code or migration path: no customer has data in it yet, so there is nothing to preserve or migrate. Such a change is not an obsoletion, data-loss, or breaking-migration defect. + +Release status is established from the diff, the app's `app.json` version, or a released baseline. A schema element with no released baseline has no persisted customer data to protect. + +## Best Practice + +Before asserting an obsoletion, data-loss, or breaking-migration defect, establish that the affected table, field, key, or enum existed in a released version. Do not require upgrade or migration code for schema that never shipped. When release status cannot be established from the diff, `app.json`, or a released baseline, omit the finding rather than demand a migration path. + +## Anti Pattern + +Demanding an upgrade codeunit, migration path, or data-preservation step, or flagging data loss, for a table, field, key, or enum that is new in the current unreleased cycle and has no released baseline. From 712dee9ec1f0b098f859753e083e8c6efa903d01 Mon Sep 17 00:00:00 2001 From: Wenjie Fan <31087545+gggdttt@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:17:32 +0200 Subject: [PATCH 04/10] style-review: calibrate variable-declaration order (AA0021) to info (#109) Online eval shows style/variable-declaration-order-by-type firing as false positives (tp/fp 2/1) at minor. The article title itself is 'Order variable declarations by type (CodeCop AA0021)', so it is analyzer-redundant exactly like the this-keyword AA0248, label-suffix AA0074, and ToolTip rules already calibrated to info. Add AA0021 to the analyzer-redundant list so it emits at info and stops competing with substantive style review. Co-authored-by: wenjiefan --- microsoft/skills/review/al-style-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/microsoft/skills/review/al-style-review.md b/microsoft/skills/review/al-style-review.md index c570a08..ee4daf5 100644 --- a/microsoft/skills/review/al-style-review.md +++ b/microsoft/skills/review/al-style-review.md @@ -60,7 +60,7 @@ When the post-conflict worklist is empty because no applicable style knowledge e For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Style findings rarely reach `blocker` — reserve it for cases where the knowledge file documents a platform-level requirement (for example, API page property constraints the OData runtime rejects). Most style findings are `minor` or `info`; egregious misuse (`Error` with pre-built Text losing translation and telemetry classification) may reach `major`. -Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, a missing `ToolTip`, required parentheses). On those, BCQuality's value is the *explanation* of why the rule exists, not a second gate; emit them at `info` so a consumer that gates on severity does not re-flag what CodeCop/AppSourceCop already reports. Reserve `minor` for style issues with concrete downstream impact the analyzer does not catch — a `Label` declared at procedure-local instead of object scope (no analyzer enforces label scope, and mis-scoped Labels are fragile in the translation pipeline), lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, a misleading named invocation. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. +Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, variable-declaration order by type AA0021, a missing `ToolTip`, required parentheses). On those, BCQuality's value is the *explanation* of why the rule exists, not a second gate; emit them at `info` so a consumer that gates on severity does not re-flag what CodeCop/AppSourceCop already reports. Reserve `minor` for style issues with concrete downstream impact the analyzer does not catch — a `Label` declared at procedure-local instead of object scope (no analyzer enforces label scope, and mis-scoped Labels are fragile in the translation pipeline), lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, a misleading named invocation. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. Set `confidence` to: From 89bf8fde311215379faca7f75f8b8c2998e4ceac Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Mon, 20 Jul 2026 14:30:29 +0200 Subject: [PATCH 05/10] tooltip: PR review flags genuinely-missing tooltips instead of deferring to AA0218 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. --- .../knowledge/style/tooltip-required-on-page-fields.md | 4 +++- .../ui/bound-page-field-inherits-source-field-tooltip.md | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/microsoft/knowledge/style/tooltip-required-on-page-fields.md b/microsoft/knowledge/style/tooltip-required-on-page-fields.md index fc5a3cb..a11d4a9 100644 --- a/microsoft/knowledge/style/tooltip-required-on-page-fields.md +++ b/microsoft/knowledge/style/tooltip-required-on-page-fields.md @@ -15,9 +15,11 @@ CodeCop AA0218 requires a non-empty `ToolTip` property on every field control on Acceptable exceptions: table fields inside `Upgrade`, `Migration`, `HybridBC14`, `HybridSL`, and `HybridGP` codeunits and tables are allowed to omit the tooltip — those types are not surfaced to users. +AA0218 is a compiler analyzer, but its severity is configured per app in the ruleset and is frequently downgraded to `info`/`None` or disabled entirely. PR review therefore cannot assume the compiler will surface the gap: it is the last line of defence for a missing tooltip and should flag it independently. The one case review must *not* flag is a bound field that inherits a `ToolTip` from its source table field — see `bound-page-field-inherits-source-field-tooltip`. + ## Best Practice -Every field control on a regular page carries `ToolTip = 'Specifies …';` (or a clear alternative phrasing). Compose the text in the form "what this value shows" rather than "what the user does with it". +Every field control on a regular page carries `ToolTip = 'Specifies …';` (or a clear alternative phrasing). Compose the text in the form "what this value shows" rather than "what the user does with it". In review, raise a `medium`-severity finding for a field that has neither an inline nor an inherited tooltip, independently of whether AA0218 is active in the app's ruleset. See sample: `tooltip-required-on-page-fields.good.al`. diff --git a/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md index a258506..87b539b 100644 --- a/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md +++ b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md @@ -13,12 +13,12 @@ application-area: [all] A page field bound to a table field inherits the source field's `ToolTip` at runtime: the control shows the table field's `ToolTip` even when the page control declares none of its own. A page field without an inline `ToolTip` is therefore not, by itself, a missing-tooltip defect — the text may be supplied by the bound source field. -The genuinely-missing case — a bound field whose source table field also carries no `ToolTip`, or an unbound control that needs one — is already reported by the compiler analyzer AA0218, which BCQuality calibrates to `info`. That analyzer, not an agent finding, owns the missing-tooltip signal. +The genuinely-missing case is different: a bound field whose source table field *also* carries no `ToolTip`, or an unbound control, has no text to inherit and is a real accessibility gap. The compiler analyzer AA0218 detects this mechanically, but its severity is set by each app's ruleset and is routinely downgraded or disabled — so it cannot be relied on as the only net. PR review is the last line of defence and should raise this case independently. ## Best Practice -Do not raise a missing-`ToolTip` finding for a page field that has a source-table binding; assume the source field supplies the tooltip. Reserve tooltip findings for the cases the dedicated tooltip rules define, and let analyzer AA0218 carry the mechanically-detectable missing-tooltip case at its calibrated severity. +Do not raise a missing-`ToolTip` finding for a bound page field whose source table field supplies a `ToolTip`; assume the control inherits it. Do raise a `medium`-severity finding when the field has no inline `ToolTip` **and** no inherited one — that is, a bound field whose source field is also tooltip-less, or an unbound control — rather than assuming AA0218 will catch it downstream. ## Anti Pattern -Flagging every page field that has no inline `ToolTip` property as an accessibility violation, ignoring that a bound field inherits its source field's tooltip and that AA0218 already covers the truly-missing case. +Two opposite failures: (1) flagging every page field that has no inline `ToolTip` as a violation, ignoring that a bound field inherits its source field's tooltip; and (2) staying silent on a field that has neither an inline nor an inherited tooltip on the assumption that the compiler's AA0218 will report it — a ruleset that downgrades or disables AA0218 then lets a genuine gap ship unflagged. From c618071ea66c1c5d661b18ece994e8b6bb53be03 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Tue, 21 Jul 2026 09:45:09 +0200 Subject: [PATCH 06/10] knowledge: add 3 FP-suppression guards from BC apps negative feedback - 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) --- ...hecked-get-throws-when-record-not-found.md | 26 +++++++++++++++++++ .../onaftergetcurrrecord-is-not-per-row.md | 26 +++++++++++++++++++ ...reason-need-not-restate-removal-version.md | 26 +++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 microsoft/knowledge/error-handling/unchecked-get-throws-when-record-not-found.md create mode 100644 microsoft/knowledge/performance/onaftergetcurrrecord-is-not-per-row.md create mode 100644 microsoft/knowledge/upgrade/obsoletereason-need-not-restate-removal-version.md diff --git a/microsoft/knowledge/error-handling/unchecked-get-throws-when-record-not-found.md b/microsoft/knowledge/error-handling/unchecked-get-throws-when-record-not-found.md new file mode 100644 index 0000000..dff1e81 --- /dev/null +++ b/microsoft/knowledge/error-handling/unchecked-get-throws-when-record-not-found.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [get, record-not-found, runtime-error, return-value, boolean-method, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# An unchecked Record.Get raises an error when the record is missing; it is not silently ignored + +## Description + +`Record.Get` returns a Boolean, but its behavior when no record is found depends on whether the return value is consumed. When the return value is used — inside `if Rec.Get(...) then`, or assigned to a variable — a missing record yields `false` and execution continues. When `Rec.Get(...)` is called as a bare statement and the return value is not used, the platform raises a runtime "record not found" error if the record does not exist. A bare `Rec.Get(Key)` therefore acts as an assertion that the record exists: it does not swallow or silently ignore a missing record. This mirrors other AL find methods, where an unconsumed return value lets the platform enforce the not-found error. + +## Best Practice + +Do not claim that a `Record.Get` whose return value is unused silently ignores a missing record or hides an error. Treat a bare `Rec.Get(...)` statement as an intentional existence assertion that already throws when the record is absent. Recommend an explicit existence check only when the surrounding logic must continue gracefully rather than error out. + +## Anti Pattern + +Flagging a bare `Rec.Get(Key)` statement as a defect because "the return value is ignored, so a missing record is swallowed", or recommending it be wrapped in `if Rec.Get(...) then ... else Error(...)` to "handle the not-found case" — the unchecked call already raises an error when the record is missing. + +## See also + +- `ignored-tryfunction-return-disables-try-semantics.md` — a different case where ignoring a Boolean return value changes behavior. diff --git a/microsoft/knowledge/performance/onaftergetcurrrecord-is-not-per-row.md b/microsoft/knowledge/performance/onaftergetcurrrecord-is-not-per-row.md new file mode 100644 index 0000000..928ad02 --- /dev/null +++ b/microsoft/knowledge/performance/onaftergetcurrrecord-is-not-per-row.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: performance +keywords: [onaftergetcurrrecord, onaftergetrecord, calcfields, n-plus-one, page-lifecycle, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Database work in OnAfterGetCurrRecord is not a per-row or N+1 cost + +## Description + +`OnAfterGetCurrRecord` fires only when the current/active record changes — typically once when the page opens and once each time the user selects a different row — not once for every row rendered in a list. Database work placed there, such as `CalcFields`, `Get`, or a lookup, therefore runs a bounded number of times driven by user navigation, not multiplied by the number of visible rows. This is unlike `OnAfterGetRecord`, which fires once per row as the page loads records and can create a genuine N+1 pattern. Reviewers sometimes see `CalcFields` or a database call inside a page trigger and assume it runs for every row; the trigger name determines whether that assumption holds. + +## Best Practice + +Before flagging `CalcFields`, `Get`, or a similar database call in a page trigger as a per-row or N+1 problem, confirm the trigger is `OnAfterGetRecord`, which runs per row. Do not flag the same work in `OnAfterGetCurrRecord`: that trigger runs on current-record change, not for every displayed row. + +## Anti Pattern + +Reporting `CalcFields` or another database call inside `OnAfterGetCurrRecord` as an N+1 or per-row performance defect, or recommending it be moved out "to avoid running once per row". The trigger does not run per row. + +## See also + +- `calcfields-in-both-getrecord-triggers-is-not-redundant.md` — the lifecycle distinction between the two triggers. diff --git a/microsoft/knowledge/upgrade/obsoletereason-need-not-restate-removal-version.md b/microsoft/knowledge/upgrade/obsoletereason-need-not-restate-removal-version.md new file mode 100644 index 0000000..9165921 --- /dev/null +++ b/microsoft/knowledge/upgrade/obsoletereason-need-not-restate-removal-version.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [obsolete-reason, obsolete-tag, deprecation, version, metadata, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# ObsoleteReason need not restate the removal version; ObsoleteTag carries it + +## Description + +An obsoleted object, field, key, enum, or enum value carries both `ObsoleteReason` and `ObsoleteTag`, and the two properties have different jobs. `ObsoleteReason` is free text that explains why the element is obsolete and what replaces it. `ObsoleteTag` identifies when it became obsolete — typically the version, release, or work item that introduced the obsoletion. The version traceability lives in `ObsoleteTag`; there is no requirement that `ObsoleteReason` also name the removal version or repeat what the tag already records. A reason that omits a version number is complete as long as it explains the deprecation and points to a replacement, provided `ObsoleteTag` pins the version. + +## Best Practice + +When `ObsoleteTag` already carries the version or tracking reference, do not flag `ObsoleteReason` for not mentioning a version or removal release. Judge `ObsoleteReason` on whether it explains the deprecation and names a replacement, and judge version traceability on `ObsoleteTag` instead. + +## Anti Pattern + +Flagging an `ObsoleteReason` as vague, incomplete, or missing a version reference solely because it does not restate the removal version, when `ObsoleteTag` already records that version. Requiring the reason to duplicate the tag's version is not a real convention. + +## See also + +- `obsoletion-requires-reason-and-tag.md` — both properties are required; the reason names the replacement and the tag identifies when the element became obsolete. From 2b401aa38ea8be8227542d16e3c54adf3738e6c5 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Tue, 21 Jul 2026 11:08:04 +0200 Subject: [PATCH 07/10] Add batch-2 FP guards: sentence-case action captions + primary-key Get is transaction-cached --- ...y-key-get-in-loop-is-transaction-cached.md | 24 ++++++++++++ ...ion-noun-phrase-vs-sentence-phrase.good.al | 38 +++++++++++++++++++ ...lization-noun-phrase-vs-sentence-phrase.md | 26 +++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md create mode 100644 microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.good.al create mode 100644 microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.md diff --git a/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md b/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md new file mode 100644 index 0000000..ed41f1b --- /dev/null +++ b/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: performance +keywords: [get, primary-key, record-cache, transaction, n-plus-one, dictionary-cache, over-engineering, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A primary-key Get() in a per-row helper is not an N+1 to cache manually + +## Description + +The Business Central server caches primary-key reads within a transaction. Repeated `Record.Get()` calls for the same key are served from that cache rather than re-queried, so a guarded `if not Rec.Get(...) then exit;` inside a per-row helper is not a genuine N+1 pattern. When each row legitimately carries a distinct key — for example one `Bin Content` row per bin, so `Bin.Get` and `BinType.Get` see a different bin each iteration — the `Get` must run per row regardless, and there is nothing to hoist. + +Reviewers sometimes see two `Get` calls inside a routine that runs once per row and recommend wrapping them in a `Dictionary` cache. That is over-engineering: it duplicates the server's built-in record cache, adds state that must be invalidated, and breaks the surrounding extension's established pattern of direct guarded `Get` calls. + +## Best Practice + +Treat a primary-key `Get()` — especially a guarded `if not Rec.Get(...) then exit;` — as a cheap, transaction-cached read. Do not recommend a manual `Dictionary` cache around per-row primary-key `Get` calls. Reserve N+1 concerns for genuinely repeated non-keyed queries (`FindSet`/`FindFirst` with filters, `Count`) that re-hit the database each iteration. + +## Anti Pattern + +Reporting repeated primary-key `Get` calls (such as `Bin.Get` and `BinType.Get`) inside a per-row helper as a performance defect, or recommending they be cached in a `Dictionary`. The reads are already cached by the server within the transaction, and per-row keys often differ so the calls cannot be hoisted. diff --git a/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.good.al b/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.good.al new file mode 100644 index 0000000..077bcdf --- /dev/null +++ b/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.good.al @@ -0,0 +1,38 @@ +page 50210 "UI Sample Caption Case" +{ + PageType = List; + ApplicationArea = All; + SourceTable = "Sales Line"; + + layout + { + area(Content) + { + repeater(Lines) + { + field("Document No."; Rec."Document No.") + { + ToolTip = 'Specifies the document number.'; + } + } + } + } + + actions + { + area(Processing) + { + action(ShowSourceDocument) + { + Caption = 'Show source document'; + Image = ViewSourceDocumentLine; + ToolTip = 'Open the related source document.'; + + trigger OnAction() + begin + Message('%1', Rec."Document No."); + end; + } + } + } +} diff --git a/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.md b/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.md new file mode 100644 index 0000000..f0d3f8c --- /dev/null +++ b/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: ui +keywords: [caption, capitalization, sentence-case, title-case, action, noun-phrase, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Sentence-phrase captions use sentence case, not title case + +## Description + +Business Central caption capitalization depends on whether the caption reads as a **noun phrase** or a **sentence/verb phrase**. Following the Microsoft writing-style guideline, a caption that reads as an imperative sentence — most action captions, such as `'Show source document'`, `'Post and print'`, or `'Copy from last inspection'` — uses **sentence case**: only the first word and any proper nouns are capitalized. Title case (`'Show Source Document'`) is the older convention and is not required for these captions. + +Noun-phrase captions (object names, field labels such as `'Source Document No.'`) follow their own capitalization; that is a separate case and is not what this article covers. Reviewers sometimes see a lower-cased word in an action caption (`'Show source document'`) and flag it as inconsistent title case, but a sentence-phrase action caption is correct as written. + +## Best Practice + +For an action `Caption` that reads as a sentence or verb phrase, capitalize only the first word and proper nouns (sentence case). Do not require every significant word to be capitalized. Before flagging a caption as "should be title case", confirm it is a noun phrase; leave imperative/sentence-phrase action captions in sentence case. + +See sample: `caption-capitalization-noun-phrase-vs-sentence-phrase.good.al`. + +## Anti Pattern + +Reporting a sentence-case action caption such as `'Show source document'` as a style defect and recommending title case (`'Show Source Document'`), or calling it inconsistent with BC conventions. Sentence case is the current guideline for sentence-phrase captions. From 5970984603799dae1bba85b72c1a7b4c2c4e7178 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Wed, 29 Jul 2026 15:58:02 +0200 Subject: [PATCH 08/10] Move plugin.json to plugin root; declare skills for CLI/marketplace compliance The GitHub Copilot CLI plugin reference requires plugin.json at the root of the plugin directory. BCQuality shipped it only under .claude-plugin/, which is tolerated by --plugin-dir but is non-canonical and can be rejected on the marketplace / 'plugin install owner/repo' path. Mirror the proven microsoft/BC-ALAgents al-review plugin layout: move plugin.json to the repo (plugin) root and add an explicit skills array plus repository/license/keywords metadata to both plugin.json and marketplace.json. The skills array pins the one real skill (skills/bcquality-al-review/), avoiding ambiguity with the loose meta .md files in skills/. Verified: 'copilot --plugin-dir ' still loads the bcquality plugin and the bcquality-al-review skill registers at runtime, identical to before. --- .claude-plugin/marketplace.json | 5 ++++- .claude-plugin/plugin.json => plugin.json | 14 +++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) rename .claude-plugin/plugin.json => plugin.json (59%) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 6aa3f8a..1aa47c6 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -9,7 +9,10 @@ "name": "bcquality", "source": "./", "description": "Business Central AL quality knowledge base and review skills, packaged as an installable plugin. Ships the entire BCQuality tree (skills, knowledge, tools) so the Entry routing protocol runs against the installed clone.", - "version": "0.1.0" + "version": "0.1.0", + "skills": [ + "./skills/bcquality-al-review/" + ] } ] } diff --git a/.claude-plugin/plugin.json b/plugin.json similarity index 59% rename from .claude-plugin/plugin.json rename to plugin.json index bacea2a..0934bd3 100644 --- a/.claude-plugin/plugin.json +++ b/plugin.json @@ -5,5 +5,17 @@ "author": { "name": "microsoft/BCQuality", "url": "https://github.com/microsoft/BCQuality" - } + }, + "repository": "https://github.com/microsoft/BCQuality", + "license": "MIT", + "keywords": [ + "bc", + "al", + "business-central", + "code-review", + "quality" + ], + "skills": [ + "./skills/bcquality-al-review/" + ] } From 88dcfd1a7650ded809e2269c9b6975f4991540e2 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 30 Jul 2026 11:13:33 +0200 Subject: [PATCH 09/10] Fix bridge skill manifest references after #122 root move #122 moved the plugin manifest to the root plugin.json (and moved the bridge skill to skills/bcquality-al-review/), but the bridge SKILL.md prose still pointed at the now-deleted .claude-plugin/plugin.json: - ## Plugin root told the host to resolve PLUGIN_ROOT by anchoring on .claude-plugin/plugin.json, a marker that no longer exists, so the location-based fallback could never find it. Anchor on root plugin.json. - ## Notes described .claude-plugin/plugin.json as the manifest the plugin uses and root plugin.json as a future form -- the reverse of reality after #122. Describe root plugin.json as canonical and .claude-plugin/ marketplace.json as the marketplace entry. Doc-only; no behavior change. --- skills/bcquality-al-review/SKILL.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/skills/bcquality-al-review/SKILL.md b/skills/bcquality-al-review/SKILL.md index 20b2417..32c8207 100644 --- a/skills/bcquality-al-review/SKILL.md +++ b/skills/bcquality-al-review/SKILL.md @@ -24,8 +24,8 @@ Do **not** use this skill to *generate* AL code — it only reviews. ## Plugin root -Resolve `PLUGIN_ROOT` to the directory that contains this plugin's -`.claude-plugin/plugin.json`. This skill lives at +Resolve `PLUGIN_ROOT` to the directory that contains this plugin's root +`plugin.json`. This skill lives at `PLUGIN_ROOT/skills/bcquality-al-review/SKILL.md`, so `PLUGIN_ROOT` is two levels up from this file. All paths below are relative to `PLUGIN_ROOT`. If the host exposes a plugin-root environment variable, prefer it. @@ -93,7 +93,8 @@ caller can log the reason. `enabled-layers` (`BCQUALITY_ENABLED_LAYERS`) — the denied layers' files still exist on disk. Treat `enabled-layers` as a selection filter, not a hard security boundary. A future revision could add a genuine deny mechanism (e.g. pruning the installed tree). -- **Manifest location.** This plugin uses `.claude-plugin/plugin.json`, which both +- **Manifest location.** This plugin's manifest is the root `plugin.json`, which both Claude Code and Copilot CLI accept (verified with Copilot CLI: `plugin install` - reports the bridge skill loaded). Copilot CLI also accepts a root `plugin.json`; if a - future host only reads the root form, dual-home the manifest. + reports the bridge skill loaded). A `.claude-plugin/marketplace.json` alongside it + carries the marketplace entry. Claude Code also reads `.claude-plugin/plugin.json`; if + a future host only reads that form, dual-home the manifest there. From 6d1fada5a41f99e00e2d0be627c793b41c4cb193 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Tue, 4 Aug 2026 10:41:03 +0200 Subject: [PATCH 10/10] Add FP guards for field relocation to tableextension and event parameter addition Two knowledge false-positive guards addressing bug 642303 (agent FPs on BCApps PR #9607): - breaking-changes: relocating a field to a tableextension in the same app under the same field ID/name is a relocation, not a deletion/rename; the field still resolves on the table, so it must not be flagged as a deleted shipped field or require ObsoleteState staging. Scoped to the contract axis; silent on data migration. - events: adding a parameter to an event publisher does not break existing subscribers (subscribers bind by name and match a subset), so the addition itself must not be reported as a breaking signature change. --- ...ld-to-a-tableextension-is-not-a-deletion.md | 18 ++++++++++++++++++ ...ter-to-an-event-is-not-a-breaking-change.md | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 microsoft/knowledge/breaking-changes/relocating-a-field-to-a-tableextension-is-not-a-deletion.md create mode 100644 microsoft/knowledge/events/adding-a-parameter-to-an-event-is-not-a-breaking-change.md diff --git a/microsoft/knowledge/breaking-changes/relocating-a-field-to-a-tableextension-is-not-a-deletion.md b/microsoft/knowledge/breaking-changes/relocating-a-field-to-a-tableextension-is-not-a-deletion.md new file mode 100644 index 0000000..d37bfe8 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/relocating-a-field-to-a-tableextension-is-not-a-deletion.md @@ -0,0 +1,18 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [table-field, tableextension, relocation, field-id, obsoletestate, breaking-change, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Relocating a field to a tableextension in the same app is not a deletion + +## Description + +Moving a field out of a base-table definition (or a base-app layer modification of one) into a tableextension that `extends` the same table, within the same app and keeping the same field ID and name, is a relocation — not a deletion or a rename. After the move the field still exists on the table: `Rec."Field Name"` and the field ID resolve exactly as before, so dependent extensions that reference the field continue to compile. Nothing in the field's public contract is removed or renamed, so the deprecation lifecycle that protects a genuinely removed field does not apply. LLM reviewers frequently misread the two-sided diff — the field disappearing from the base object and reappearing in the tableextension — as a shipped field being deleted and illegally re-added under the same ID, and demand `ObsoleteState = Pending` staging that this refactor does not need. + +## Best Practice + +Recognize a field that is removed from a base table (or base-app layer) and re-declared in a tableextension of the same table, with the same field ID and name, as a same-app relocation. Do not flag it as a deleted or renamed shipped field, and do not require `ObsoleteState = Pending`, `ObsoleteReason`, `ObsoleteTag`, or a deprecation window for the move itself. The `obsolete-table-fields-instead-of-deleting-them` and `obsolete-pending-to-removed-staging` rules apply to fields that leave the table's contract entirely, not to fields relocated within the same app under an unchanged ID. diff --git a/microsoft/knowledge/events/adding-a-parameter-to-an-event-is-not-a-breaking-change.md b/microsoft/knowledge/events/adding-a-parameter-to-an-event-is-not-a-breaking-change.md new file mode 100644 index 0000000..330f9bf --- /dev/null +++ b/microsoft/knowledge/events/adding-a-parameter-to-an-event-is-not-a-breaking-change.md @@ -0,0 +1,18 @@ +--- +bc-version: [all] +domain: events +keywords: [event-parameters, signature, subscriber-binding, backward-compatibility, integration-event, breaking-change, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Adding a parameter to an event is not a breaking change + +## Description + +Adding a parameter to an existing event publisher does not break existing subscribers. AL binds a subscriber to a publisher by the event name, and the subscriber's parameter list only has to be a subset of the publisher's, matched by name and type. A subscriber that does not declare the new parameter keeps compiling and keeps binding — it simply ignores the addition. This holds for `IntegrationEvent` and `BusinessEvent` publishers, and even more plainly for `local` events. Appending the new parameter at the end keeps the change a clean, reviewable addition (see `add-new-event-parameters-at-the-end`). LLM reviewers often misreport the mere presence of a new event parameter as a "breaking event signature change" that breaks subscribers, which is incorrect. + +## Best Practice + +Do not flag the addition of a parameter to an event publisher as a breaking or signature-breaking change, and do not claim it breaks existing subscribers. Genuine, separate concerns are covered by their own rules — a parameter inserted in the middle of the list rather than appended (`add-new-event-parameters-at-the-end`), or a parameter that carries no meaningful value — and should be raised on those grounds, not framed as a backward-compatibility break.