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. diff --git a/microsoft/skills/review/al-breaking-changes-review.md b/microsoft/skills/review/al-breaking-changes-review.md index f76bdd4..82aeda0 100644 --- a/microsoft/skills/review/al-breaking-changes-review.md +++ b/microsoft/skills/review/al-breaking-changes-review.md @@ -61,8 +61,6 @@ When the post-conflict worklist is empty because no applicable breaking-changes ## Action -**Released-baseline precondition.** Breaking-change rules protect contracts that have already shipped to customers or are exposed to external extensions. Before emitting any finding — knowledge-backed or agent — establish that the affected object, field, key, enum value, or procedure was present in a released baseline. A symbol 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 and is not a breaking change. When release status cannot be established from the diff, `app.json`, or a released baseline, do not assert a break; omit the finding rather than flag it. - For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: - When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`. diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index 335e343..39851ac 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -73,8 +73,6 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits an error-handling 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 error-handling 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 error handling; 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. -**Known false-positive — do not emit.** The Boolean page triggers `OnInsertRecord`, `OnModifyRecord`, `OnDeleteRecord`, and `OnQueryClosePage` return `true` by default; omitting an explicit return value lets the operation proceed. Only an explicit `exit(false)` (or a path that returns `false`) cancels it. Do not claim a missing `exit(true)` prevents inserts, modifies, or deletes. - 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: replace a string-concatenated `Error` with a Label-backed call; mark an internal-only failure `ErrorType::Internal`; add a missing `DetailedMessage`). 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`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-performance-review.md b/microsoft/skills/review/al-performance-review.md index c0873ba..3262179 100644 --- a/microsoft/skills/review/al-performance-review.md +++ b/microsoft/skills/review/al-performance-review.md @@ -74,11 +74,6 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a performance 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 performance 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 performance; 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. -**Known false-positive patterns — do not emit.** - -- **"Drill-down / lookup / list is unfiltered".** The effective filter is frequently defined outside the changed hunk — on the table via `SourceTableView`, a `TableRelation`, or `SetRange`/`SetFilter` in unchanged code. Absence of a filter in the diff is not evidence that none applies; do not assert an unfiltered result set. -- **`CalcFields` in both `OnAfterGetRecord` and `OnAfterGetCurrRecord`.** These triggers fire at different points in the page lifecycle (per row as records load vs. when a record becomes current) and serve different purposes; the pair is not duplicate or redundant work. - 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`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. 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 4f8eb36..81ab12e 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -61,9 +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. - -**Known false-positive — do not emit.** A page field bound to a table field inherits that field's `ToolTip`; the genuinely-missing case is already covered by analyzer AA0218 (calibrated to `info`). Do not raise a missing-`ToolTip` agent finding for a field with a source-table binding. +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`. diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index 138c876..879e788 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -52,8 +52,6 @@ When the post-conflict worklist is empty because no applicable upgrade knowledge ## Action -**Released-baseline precondition.** Upgrade and migration findings protect data and schema that have already shipped to customers. Before emitting any finding — knowledge-backed or agent — establish that the affected table, field, key, or enum existed in a released version. A schema element 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 and is not an obsoletion, data-loss, or breaking-migration defect. When release status cannot be established from the diff, `app.json`, or a released baseline, do not assert an upgrade defect; omit the finding rather than flag it. - For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: - When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` for irreversible data corruption (enum-ordinal shift, unguarded reads that abort the upgrade) and for changes that would ship to customers without a migration path (new InitValue on an existing table without upgrade code).