mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 09:26:52 +01:00
Complete AL review knowledge readiness (#108)
* Complete AL review knowledge readiness Fill telemetry and Query coverage, strengthen thin review domains, correct audited content defects, and add deterministic cheap-model evaluation and reference-integrity safeguards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27 * Generalize review fixture discovery Derive smoke cases from the leaf, domain, and paired-sample conventions so new leaves require no scoring-contract changes. Keep only exceptional selection/context overrides and fail when retrieval metadata cannot rank the selected article. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27 * Preserve published field IDs in sample Keep the existing Email and Contact Email field IDs unchanged, clarify that the sample represents an independent baseline, and use a local breaking-change rule for the generic smoke evaluation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27 * Clarify published field identity rules State explicitly that a published field keeps its ID, name, and type while a replacement is added as a separate field under an unused ID. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27 * Align field obsoletion sample baselines Use Email field ID 3 as the shared baseline so the bad example demonstrates a same-ID rename while the good example retains the original field and adds a separate replacement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27 --------- Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
This commit is contained in:
Jesper Schulz-Wedde
GitHub
parent
ae04938c03
commit
186d8a1314
105 changed files with 2229 additions and 212 deletions
34
.github/scripts/validate_frontmatter.py
vendored
34
.github/scripts/validate_frontmatter.py
vendored
|
|
@ -62,6 +62,7 @@ ISO_ALPHA2 = re.compile(r"^[a-z]{2}$")
|
|||
RANGE_SHORTHAND = re.compile(r"^(\d+)\.\.(\d+)?$")
|
||||
FENCED_CODE_BLOCK = re.compile(r"^```", re.MULTILINE)
|
||||
HEADING_H2 = re.compile(r"^##\s+(.+?)\s*$", re.MULTILINE)
|
||||
SAMPLE_REFERENCE = re.compile(r"`([a-z0-9]+(?:-[a-z0-9]+)*\.(?:good|bad)\.[a-z0-9]+)`")
|
||||
|
||||
|
||||
# --- Diagnostics ------------------------------------------------------------
|
||||
|
|
@ -222,6 +223,13 @@ def validate_knowledge(path: Path, parsed: Parsed, report: Report) -> None:
|
|||
if "domain" in fm:
|
||||
if not isinstance(fm["domain"], str) or not fm["domain"].strip():
|
||||
report.error(path, "R04", "domain must be a non-empty string", 1)
|
||||
elif fm["domain"] != path.parent.name:
|
||||
report.error(
|
||||
path,
|
||||
"R27",
|
||||
f"frontmatter domain '{fm['domain']}' must match directory '{path.parent.name}'",
|
||||
1,
|
||||
)
|
||||
|
||||
# R05 keywords
|
||||
if "keywords" in fm:
|
||||
|
|
@ -477,7 +485,16 @@ def validate_samples_in_domain(domain_dir: Path, root: Path, report: Report) ->
|
|||
"""R14: every non-.md file must match <slug>.<kind>.<ext> with <slug>.md present."""
|
||||
if not domain_dir.is_dir():
|
||||
return
|
||||
article_slugs = {p.stem for p in domain_dir.glob("*.md")}
|
||||
articles = {p.stem: p for p in domain_dir.glob("*.md")}
|
||||
article_slugs = set(articles)
|
||||
article_texts: dict[str, str] = {}
|
||||
for slug, article in articles.items():
|
||||
try:
|
||||
article_texts[slug] = article.read_text(encoding="utf-8")
|
||||
except UnicodeDecodeError:
|
||||
# R01 reports this during the article pass.
|
||||
continue
|
||||
|
||||
for entry in domain_dir.iterdir():
|
||||
if not entry.is_file() or entry.suffix == ".md":
|
||||
continue
|
||||
|
|
@ -491,9 +508,24 @@ def validate_samples_in_domain(domain_dir: Path, root: Path, report: Report) ->
|
|||
kind = m.group("kind")
|
||||
if slug not in article_slugs:
|
||||
report.error(entry, "R14", f"orphan sample: no matching article '{slug}.md' in {domain_dir.relative_to(root).as_posix()}")
|
||||
elif entry.name not in article_texts.get(slug, ""):
|
||||
report.error(
|
||||
entry,
|
||||
"R28",
|
||||
f"sample is not referenced by its article '{slug}.md'",
|
||||
)
|
||||
if kind not in VALID_SAMPLE_KINDS:
|
||||
report.warn(entry, "R14", f"non-standard sample kind '{kind}'; standard kinds are {sorted(VALID_SAMPLE_KINDS)}")
|
||||
|
||||
for slug, article in articles.items():
|
||||
for sample_name in SAMPLE_REFERENCE.findall(article_texts.get(slug, "")):
|
||||
if not (domain_dir / sample_name).is_file():
|
||||
report.error(
|
||||
article,
|
||||
"R28",
|
||||
f"referenced sample does not exist: '{sample_name}'",
|
||||
)
|
||||
|
||||
|
||||
# --- Orchestration ----------------------------------------------------------
|
||||
|
||||
|
|
|
|||
18
.github/workflows/review-fixtures.yml
vendored
Normal file
18
.github/workflows/review-fixtures.yml
vendored
Normal file
|
|
@ -0,0 +1,18 @@
|
|||
name: Validate AL review fixtures
|
||||
|
||||
on:
|
||||
pull_request:
|
||||
branches: [main]
|
||||
push:
|
||||
branches: [main]
|
||||
|
||||
jobs:
|
||||
validate-review-fixtures:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- name: Check out repository
|
||||
uses: actions/checkout@v4
|
||||
|
||||
- name: Validate review evaluation corpus
|
||||
shell: pwsh
|
||||
run: ./tools/Test-ReviewFixtures.ps1 -Root . -PrepareDirectory "$env:RUNNER_TEMP/bcquality-review-fixtures"
|
||||
16
README.md
16
README.md
|
|
@ -88,18 +88,9 @@ Code examples belong in separate files, not in the knowledge file itself. Knowle
|
|||
|
||||
## Scope
|
||||
|
||||
BCQuality covers Business Central broadly — the application domains it supports, the technologies used to extend it, and the practices that keep implementations healthy. The scope includes:
|
||||
The current curated corpus is focused on **technical AL code review**: AppSource and compatibility, data modeling, error handling, events, interfaces, performance, privacy, Query objects, security, style, telemetry, testing, UI, upgrade, and web services. These are the domains backed by knowledge files and registered review leaves today.
|
||||
|
||||
- **Business Central domains** — Finance, Supply Chain Management, Manufacturing, Jobs, Warehousing, Service, and the many other functional areas BC covers. Domain knowledge helps agents understand the business context they are working in.
|
||||
- AL language patterns and anti-patterns
|
||||
- PowerShell scripting for BC
|
||||
- Pipelines (AL-Go, GitHub Actions)
|
||||
- Business Central APIs
|
||||
- Power Platform integration
|
||||
- Telemetry and KQL
|
||||
- AppSource lifecycle
|
||||
|
||||
A BC developer's actual job spans all of this, and BCQuality reflects that.
|
||||
Business Central functional domains (Finance, Supply Chain Management, Manufacturing, Jobs, Warehousing, Service), PowerShell, pipelines, and Power Platform remain valid future repository scope, but they are **not current coverage claims** until corresponding knowledge and action skills exist. Consumers should derive supported review scope from the live knowledge index and dispatched skills, not from roadmap breadth.
|
||||
|
||||
## How agents consume BCQuality
|
||||
|
||||
|
|
@ -122,6 +113,7 @@ For the end-to-end flow — from orchestrator trigger through to how output reac
|
|||
|
||||
```
|
||||
├── /skills/ # Global: entry-point skill + meta-skill contracts (READ, DO, WRITE)
|
||||
├── /evaluation/ # Neutral good/bad review fixtures and scoring contract
|
||||
├── /.github/ # Actions and workflows
|
||||
├── /microsoft/ # Microsoft-endorsed layer
|
||||
│ ├── /knowledge/ # Knowledge files by domain
|
||||
|
|
@ -158,6 +150,8 @@ Contributions are welcome. Before submitting a PR:
|
|||
|
||||
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.
|
||||
|
||||
Companion samples must be referenced by filename from their article, and every referenced sample must exist. The review evaluation corpus under [`evaluation/`](evaluation/) adds one positive and one clean control for every registered AL review leaf; see [`evaluation/README.md`](evaluation/README.md) for credential-free validation and optional fast-model scoring.
|
||||
|
||||
## License
|
||||
|
||||
[MIT](LICENSE)
|
||||
|
|
|
|||
56
evaluation/README.md
Normal file
56
evaluation/README.md
Normal file
|
|
@ -0,0 +1,56 @@
|
|||
# AL review evaluation
|
||||
|
||||
The evaluation is convention-driven. For every `microsoft/skills/review/al-<domain>-review.md` leaf, the harness finds `microsoft/knowledge/<domain>/`, selects the first article (by filename) with both `.bad.al` and `.good.al` companions, and derives the expected positive and clean control automatically. Adding a conforming leaf requires no scoring-contract edit.
|
||||
|
||||
`review-fixtures.json` contains only global thresholds and optional exceptional overrides. An override may select a different article or add context when the generic convention cannot express a scenario. It should remain empty in the normal case.
|
||||
|
||||
Model-facing preparation hashes case IDs, neutralizes `Good`/`Bad` object-name tokens, and removes full-line sample comments so neither the article slug, domain, nor expected outcome reveals the answer.
|
||||
|
||||
## Validate the corpus
|
||||
|
||||
```powershell
|
||||
pwsh ./tools/Test-ReviewFixtures.ps1 -Root .
|
||||
```
|
||||
|
||||
This credential-free check proves every registered leaf maps to a same-named knowledge domain with at least one complete AL sample pair and that all configured overrides are valid.
|
||||
|
||||
## Run a fast-model evaluation
|
||||
|
||||
1. Prepare neutral inputs:
|
||||
|
||||
```powershell
|
||||
pwsh ./tools/Test-ReviewFixtures.ps1 -Root . -PrepareDirectory ./.evaluation-run
|
||||
```
|
||||
|
||||
This is also the CI path. It derives all cases, builds the current index, requires the convention-selected article to rank naturally into the candidate cutoff, and prepares the neutral requests.
|
||||
|
||||
2. For a fast/small model, use one fresh invocation per `request-case-*.json`. Each request embeds the exact leaf instructions, that domain's candidate index rows with authoritative paths, and one opaque case. The model opens only matching articles and copies finding IDs from `candidateArticles[].path`. Save each response with the matching `result-case-*.json` name in the same directory.
|
||||
|
||||
`request-<domain>.json` files provide optional two-case leaf batches; save those as `result-<domain>.json`. Directory scoring prefers `result-case-*.json` when present and otherwise falls back to `result-*.json`. `review-request.json` is an optional all-domains stress test for larger models. Neither batch form is the preferred fast-model profile.
|
||||
|
||||
3. Save only this result shape:
|
||||
|
||||
```json
|
||||
{
|
||||
"cases": [
|
||||
{
|
||||
"id": "case-a1b2c3d4",
|
||||
"findings": [
|
||||
{ "id": "microsoft/knowledge/appsource/object-affixes-prevent-collisions.md" }
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
|
||||
Include every case. A clean control has an empty `findings` array.
|
||||
|
||||
4. Score all per-leaf results together:
|
||||
|
||||
```powershell
|
||||
pwsh ./tools/Test-ReviewFixtures.ps1 -Root . -ResultsDirectory ./.evaluation-run
|
||||
```
|
||||
|
||||
For a single combined stress-test result, use `-ResultsPath` instead.
|
||||
|
||||
The committed gate requires full expected recall, the exact convention-derived article ID, and no findings on clean controls.
|
||||
39
evaluation/review-fixtures.json
Normal file
39
evaluation/review-fixtures.json
Normal file
|
|
@ -0,0 +1,39 @@
|
|||
{
|
||||
"version": 2,
|
||||
"selection": "first-paired-al-article",
|
||||
"minimumExpectedRecall": 1.0,
|
||||
"minimumCleanRate": 1.0,
|
||||
"overrides": {
|
||||
"appsource": {
|
||||
"context": "AppSourceCop mandatoryAffixes is configured to ABC."
|
||||
},
|
||||
"breaking-changes": {
|
||||
"article": "do-not-expose-sensitive-data-through-public-api"
|
||||
},
|
||||
"events": {
|
||||
"article": "initialize-ishandled-to-false-before-publishing"
|
||||
},
|
||||
"interfaces": {
|
||||
"article": "set-defaultimplementation-on-enum"
|
||||
},
|
||||
"performance": {
|
||||
"article": "use-isempty-for-existence-check"
|
||||
},
|
||||
"privacy": {
|
||||
"article": "no-pii-in-telemetry-message-string"
|
||||
},
|
||||
"style": {
|
||||
"article": "label-comment-explains-placeholders"
|
||||
},
|
||||
"telemetry": {
|
||||
"article": "telemetry-event-id-stable-unique"
|
||||
},
|
||||
"upgrade": {
|
||||
"article": "initvalue-does-not-update-existing-rows",
|
||||
"context": "The extended table existed in the previous app version and already contains rows."
|
||||
},
|
||||
"web-services": {
|
||||
"article": "expose-systemid-as-the-api-key"
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -11,18 +11,18 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
An AppSource extension must carry a reserved affix — a prefix or a suffix of at least three characters — on the names of the objects it owns **and** on any field, key, control, or action it adds to a base-application object. The affix is registered with Microsoft; when two coexisting extensions would otherwise collide, the registrant of the affix wins. Without it, two apps that both add a `Loyalty Points` field to `Customer`, or both define a `Loyalty Tier` table, cannot be installed side by side.
|
||||
An AppSource extension must prevent name collisions through its registered affix or, on BC23 and later for objects it owns, a namespace with at least two levels. The affix still applies to every field, key, control, or action added to a base-application object; see `two-level-namespace-replaces-object-affix-not-extension-member-affix.md`. Without either mechanism, two apps that both define a `Loyalty Tier` table cannot coexist, and two apps that add an unaffixed `Loyalty Points` field to `Customer` still collide regardless of their namespaces.
|
||||
|
||||
AppSourceCop enforces this. The primary rule is AS0011 ("An affix is required"); the affixes are configured through `mandatoryAffixes` (and `mandatoryPrefix`) in `AppSourceCop.json`. Two placements matter and are easy to get half-right: an object you define carries the affix at **object-name** level, while a member you add to a **standard** object carries the affix on that **member's** name. Adding an affixed object is not enough — an unaffixed field bolted onto `Customer` still collides and still fails validation.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Own objects are named with the affix (e.g. a table `ABC Loyalty Tier`), and every field or action added to a standard object is individually affixed (e.g. `Loyalty Points ABC` on a `Customer` tableextension).
|
||||
Own objects use the registered affix (for example `ABC Loyalty Tier`) or, when targeting BC23 or later, a qualifying namespace. Every field or action added to a standard object remains individually affixed (for example `Loyalty Points ABC` on a `Customer` tableextension).
|
||||
|
||||
See sample: `object-affixes-prevent-collisions.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Unaffixed object or member names, or the common half-measure: the extension object carries the affix but a field it adds to a standard table does not. AS0011 flags the missing affix and the field can still collide with another app.
|
||||
An owned object with neither a qualifying namespace nor an affix, an unaffixed extension member, or the common half-measure where the extension object carries the affix but a field it adds to a standard table does not. AS0011 flags the missing collision protection and the field can still collide with another app.
|
||||
|
||||
See sample: `object-affixes-prevent-collisions.bad.al`.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,43 @@
|
|||
table 50476 "Rental Setup Bad"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "Primary Key"; Code[10]) { }
|
||||
}
|
||||
}
|
||||
|
||||
page 50477 "Rental Setup Bad"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = "Rental Setup Bad";
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
field("Primary Key"; Rec."Primary Key")
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Primary Key';
|
||||
ToolTip = 'Specifies the setup record.';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50478 "Rental Setup Mgt. Bad"
|
||||
{
|
||||
procedure Initialize()
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
||||
permissionset 50479 "Rental User"
|
||||
{
|
||||
Assignable = true;
|
||||
// The setup page opens, but saving or running setup logic requires SUPER.
|
||||
Permissions =
|
||||
page "Rental Setup Bad" = X;
|
||||
}
|
||||
|
|
@ -0,0 +1,45 @@
|
|||
table 50472 "Rental Setup"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "Primary Key"; Code[10]) { }
|
||||
}
|
||||
}
|
||||
|
||||
page 50473 "Rental Setup"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = "Rental Setup";
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
field("Primary Key"; Rec."Primary Key")
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Primary Key';
|
||||
ToolTip = 'Specifies the setup record.';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50474 "Rental Setup Mgt."
|
||||
{
|
||||
procedure Initialize()
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
||||
permissionset 50475 "Rental Manager"
|
||||
{
|
||||
Assignable = true;
|
||||
Permissions =
|
||||
tabledata "Rental Setup" = RIMD,
|
||||
table "Rental Setup" = X,
|
||||
page "Rental Setup" = X,
|
||||
codeunit "Rental Setup Mgt." = X;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: appsource
|
||||
keywords: [permission-set, super, appsource, setup, usage, tabledata, execute, submission]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# AppSource permission sets must cover setup and usage without SUPER
|
||||
|
||||
## Description
|
||||
|
||||
An AppSource app must provide permission sets that let assigned users complete the app's setup and normal usage without `SUPER`. The requirement is about complete effective grants, not about naming the permission set after the app. A package can compile and install with missing tabledata or execute permissions, then fail only when Marketplace validation or a real non-SUPER user reaches the omitted path.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Trace every setup page, normal page, report, codeunit, and tabledata operation exposed by the app and cover it through assignable role permission sets composed from focused non-assignable sets. Validate setup and representative workflows as a user assigned only those app roles. Grant the minimum required operations; completeness is not a reason to use wildcards.
|
||||
|
||||
See sample: `permission-sets-cover-setup-and-usage-without-super.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Shipping no permission set, omitting a tabledata or execute grant used by the app's own UI, or instructing users and validators to assign `SUPER` when setup fails. Do not flag a permission-set name that differs from the app name; no such naming requirement exists.
|
||||
|
||||
See sample: `permission-sets-cover-setup-and-usage-without-super.bad.al`.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
namespace Contoso;
|
||||
|
||||
table 50462 "Rental Agreement"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
}
|
||||
}
|
||||
|
||||
tableextension 50463 "Rental Customer Ext" extends Customer
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(50463; "Loyalty Points"; Integer)
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
namespace Contoso.Rentals;
|
||||
|
||||
table 50460 "Rental Agreement"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
}
|
||||
}
|
||||
|
||||
tableextension 50461 "Rental Customer Ext" extends Customer
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(50461; "Loyalty Points RNT"; Integer)
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [23..]
|
||||
domain: appsource
|
||||
keywords: [namespace, two-level, affix, prefix, suffix, as0011, tableextension, pageextension]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# A two-level namespace replaces an object affix, not an extension-member affix
|
||||
|
||||
## Description
|
||||
|
||||
Current AppSource naming guidance accepts a namespace with at least two levels, such as `Contoso.Rentals`, instead of a registered prefix or suffix on the names of objects the app owns. The namespace does not qualify members added to another publisher's object: fields, keys, controls, and actions introduced through table or page extensions still share the target object's flat member namespace and still need the registered affix.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Choose one collision strategy for owned objects: a registered affix or a globally meaningful namespace with at least two levels. Regardless of that choice, apply the registered affix to every member added to a base or third-party object. Keep the affix configured for AppSourceCop so member validation remains deterministic.
|
||||
|
||||
See sample: `two-level-namespace-replaces-object-affix-not-extension-member-affix.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Using `namespace Contoso;` as though one level satisfied the AppSource alternative, or declaring `namespace Contoso.Rentals;` and then adding an unaffixed `Loyalty Points` field to `Customer`. The namespace distinguishes the extension's own objects; it cannot disambiguate members on Customer.
|
||||
|
||||
See sample: `two-level-namespace-replaces-object-affix-not-extension-member-affix.bad.al`.
|
||||
|
|
@ -0,0 +1,9 @@
|
|||
// This published object previously used namespace Contoso.Rentals.
|
||||
namespace Contoso.RentalManagement;
|
||||
|
||||
codeunit 50467 "Rental Agreement Mgt."
|
||||
{
|
||||
procedure CreateAgreement()
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,8 @@
|
|||
namespace Contoso.Rentals;
|
||||
|
||||
codeunit 50466 "Rental Agreement Mgt."
|
||||
{
|
||||
procedure CreateAgreement()
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [23..]
|
||||
domain: breaking-changes
|
||||
keywords: [namespace, published-object, dependency, breaking-change, as0007, compile-time-identity]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Treat a published namespace as part of object identity
|
||||
|
||||
## Description
|
||||
|
||||
AL resolves an object by namespace and name. Once an app ships and dependent extensions compile against that identity, changing the namespace breaks their references even when the object name and ID stay unchanged. AppSourceCop AS0007 rejects changing the namespace of published objects; namespaces are therefore not a cosmetic folder-like label that can be reorganized after release.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Choose a globally meaningful namespace before first publication and keep it stable. Add new functional areas beneath that structure without moving existing published objects. If an identity must move, use the platform's supported move/obsoletion lifecycle rather than a source-only namespace rename.
|
||||
|
||||
See sample: `namespace-is-part-of-published-object-identity.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Changing `namespace Contoso.Rentals;` to `namespace Contoso.RentalManagement;` as a cleanup while leaving the object name and ID untouched. Every dependent `using` directive and qualified reference targets the old identity and stops compiling.
|
||||
|
||||
See sample: `namespace-is-part-of-published-object-identity.bad.al`.
|
||||
|
|
@ -3,9 +3,10 @@ table 50311 "Customer Profile Bad"
|
|||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
// Breaking: the published field was renamed while retaining ID 2.
|
||||
// Breaking: the published Email field at ID 3 was renamed while retaining
|
||||
// the ID. The good example keeps Email at ID 3 and adds a separate field.
|
||||
// AppSourceCop AS0005 rejects the compatibility change; retaining the ID
|
||||
// does not by itself mean the stored column was dropped and re-created.
|
||||
field(2; "Contact Email"; Text[80]) { }
|
||||
field(3; "Contact Email"; Text[80]) { }
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -3,10 +3,10 @@ table 50310 "Customer Profile Good"
|
|||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
// Replacement field shipped alongside the old one.
|
||||
// Replacement is a separate field under an otherwise unused ID.
|
||||
field(2; "Contact Email"; Text[80]) { }
|
||||
// Old field kept and marked Pending so dependent code keeps compiling and
|
||||
// an upgrade codeunit can copy its data before it is finally removed.
|
||||
// Old field keeps its original ID, name, and type and is marked Pending so
|
||||
// dependent code keeps compiling while an upgrade codeunit migrates its data.
|
||||
field(3; "Email"; Text[80])
|
||||
{
|
||||
ObsoleteState = Pending;
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ countries: [w1]
|
|||
application-area: [all]
|
||||
---
|
||||
|
||||
# Obsolete published table fields instead of deleting or renumbering them
|
||||
# Obsolete published table fields instead of deleting, renaming, or renumbering them
|
||||
|
||||
## Description
|
||||
|
||||
|
|
@ -15,12 +15,12 @@ A shipped table field carries both a source-level contract and persisted data. R
|
|||
|
||||
## Best Practice
|
||||
|
||||
Add the replacement field under a new ID, then mark the old field `ObsoleteState = Pending` with an `ObsoleteReason` that names the replacement and an `ObsoleteTag` recording the obsoletion version. Keep the old field readable so an upgrade codeunit can copy its data during the deprecation window. Move it to `ObsoleteState = Removed` only in a later release, after the window has passed and data has migrated.
|
||||
Keep the old field's ID, name, and type unchanged. Add the replacement as a separate field under an unused ID, then mark the old field `ObsoleteState = Pending` with an `ObsoleteReason` that names the replacement and an `ObsoleteTag` recording the obsoletion version. Keep the old field readable so an upgrade codeunit can copy its data during the deprecation window. Move it to `ObsoleteState = Removed` only in a later release, after the window has passed and data has migrated.
|
||||
|
||||
See sample: `obsolete-table-fields-instead-of-deleting-them.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Renaming published `Email` to `Contact Email` with the same ID violates the compatibility contract and AS0005, even though the retained ID does not itself imply a fresh empty column. Deleting `Email` or moving the replacement to another ID without migration additionally risks losing its stored values. Detection: a previously shipped field removed, renumbered, or renamed with no retained `Pending` field and migration path.
|
||||
Renaming published `Email` to `Contact Email` with the same ID violates the compatibility contract and AS0005, even though the retained ID does not itself imply a fresh empty column. Deleting `Email` or changing its ID additionally risks losing its stored values. Detection: any previously shipped field whose name changes at the same ID, or whose original ID disappears without the unchanged field being retained as `Pending` and its data migrated to a separate replacement field.
|
||||
|
||||
See sample: `obsolete-table-fields-instead-of-deleting-them.bad.al`.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,26 @@
|
|||
table 50441 "Source Media Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Code; Code[20]) { }
|
||||
field(10; Pictures; MediaSet) { }
|
||||
}
|
||||
}
|
||||
|
||||
table 50442 "Target Media Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Code; Code[20]) { }
|
||||
field(20; Pictures; MediaSet) { }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50443 "Share Media Bad"
|
||||
{
|
||||
procedure CopyPictures(Source: Record "Source Media Bad"; var Target: Record "Target Media Bad")
|
||||
begin
|
||||
Target.Pictures := Source.Pictures;
|
||||
Target.Modify(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,29 @@
|
|||
table 50438 "Source Media Good"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Code; Code[20]) { }
|
||||
field(10; Pictures; MediaSet) { }
|
||||
}
|
||||
}
|
||||
|
||||
table 50439 "Target Media Good"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Code; Code[20]) { }
|
||||
field(20; Pictures; MediaSet) { }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50440 "Share Media Good"
|
||||
{
|
||||
procedure CopyPictures(Source: Record "Source Media Good"; var Target: Record "Target Media Good")
|
||||
var
|
||||
Index: Integer;
|
||||
begin
|
||||
for Index := 1 to Source.Pictures.Count() do
|
||||
Target.Pictures.Insert(Source.Pictures.Item(Index));
|
||||
Target.Modify(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: data-modeling
|
||||
keywords: [mediaset, media, insert, field-assignment, tenant-media, delete-integrity, sharing]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Share MediaSet items with Insert instead of field assignment
|
||||
|
||||
## Description
|
||||
|
||||
`Media` and `MediaSet` fields store IDs that reference tenant media system tables. When a record is deleted, the runtime looks for other references only in the same table and field index; it does not scan every table. Directly assigning a media-set field between different table types copies the ID without registering a separate media-set reference, so deleting one record can remove media that the other record still appears to reference.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When sharing media between different tables, iterate the source `MediaSet` and call `Target.MediaSetField.Insert(Source.MediaSetField.Item(Index))`, then modify the target record. Direct field assignment is safe only when source and target are the same record subtype and use the same field ID. This concern is about reference/delete integrity, not the separate performance cost of `ModifyAll` on tables with media fields.
|
||||
|
||||
See sample: `share-mediaset-items-with-insert-not-field-assignment.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`Target.Picture := Source.Picture;` where the two variables refer to different table types or different media-field IDs. The code copies an opaque ID, but the platform does not know that two independent fields now share the media object.
|
||||
|
||||
See sample: `share-mediaset-items-with-insert-not-field-assignment.bad.al`.
|
||||
|
|
@ -0,0 +1,35 @@
|
|||
enum 50434 "Relation Type Bad"
|
||||
{
|
||||
Extensible = true;
|
||||
|
||||
value(0; Customer) { }
|
||||
}
|
||||
|
||||
table 50435 "Related Entity Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Type; Enum "Relation Type Bad") { }
|
||||
field(2; "Related No."; Code[20])
|
||||
{
|
||||
// This unconditional relation wins before extension branches run.
|
||||
TableRelation = Customer;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
enumextension 50436 "Relation Type Bad Ext" extends "Relation Type Bad"
|
||||
{
|
||||
value(10; Resource) { }
|
||||
}
|
||||
|
||||
tableextension 50437 "Related Entity Bad Ext" extends "Related Entity Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
modify("Related No.")
|
||||
{
|
||||
TableRelation = if (Type = const(Resource)) Resource;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,37 @@
|
|||
enum 50430 "Relation Type Good"
|
||||
{
|
||||
Extensible = true;
|
||||
|
||||
value(0; Customer) { }
|
||||
value(1; Item) { }
|
||||
}
|
||||
|
||||
table 50431 "Related Entity Good"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Type; Enum "Relation Type Good") { }
|
||||
field(2; "Related No."; Code[20])
|
||||
{
|
||||
TableRelation =
|
||||
if (Type = const(Customer)) Customer
|
||||
else if (Type = const(Item)) Item;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
enumextension 50432 "Relation Type Resource" extends "Relation Type Good"
|
||||
{
|
||||
value(10; Resource) { }
|
||||
}
|
||||
|
||||
tableextension 50433 "Related Entity Resource" extends "Related Entity Good"
|
||||
{
|
||||
fields
|
||||
{
|
||||
modify("Related No.")
|
||||
{
|
||||
TableRelation = if (Type = const(Resource)) Resource;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: data-modeling
|
||||
keywords: [tablerelation, tableextension, enumextension, additive, top-down, unconditional-relation]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Design TableRelation branches for additive top-down extension
|
||||
|
||||
## Description
|
||||
|
||||
A `tableextension` can add to an existing `TableRelation`, but the combined relation is evaluated top-down after the original value. The first unconditional relation wins. An extension branch appended after an unconditional base relation is therefore unreachable, even though the extension compiles and appears to describe the new enum value correctly.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When a relation is designed to follow an extensible enum, express the base cases as conditional branches and leave no unconditional catch-all ahead of future extension branches. An enum extension can then append a condition for its new value. When extending a field you do not own, inspect the original `TableRelation`; do not claim that an appended condition overrides an unconditional relation.
|
||||
|
||||
See sample: `table-relation-extensions-are-additive-and-top-down.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A base field has an unconditional `TableRelation = Customer;` and a `tableextension` adds `if (Type = const(Resource)) Resource`. The original unconditional branch always wins, so the new enum value still validates and looks up against Customer. The concern is evaluation order, not `ValidateTableRelation`; free-form input is covered separately by security guidance.
|
||||
|
||||
See sample: `table-relation-extensions-are-additive-and-top-down.bad.al`.
|
||||
|
|
@ -14,5 +14,9 @@ application-area: [all]
|
|||
## Best Practice
|
||||
For a plain required-field check, prefer `TestField`, which tests the condition and raises the error in one call. When the condition is non-trivial and has already been evaluated, call `FieldError(FieldNo)` with no message to get the localized default (`must have a value`, `is not valid`, etc.), or pass a short lowercase predicate such as `FieldError(FieldNo, 'must be a positive number')`. Start the custom text with a lowercase letter so it reads as one sentence with the auto-inserted caption, and use a field-number reference (or the field token) rather than a hard-coded field name so captions and translations stay correct. Let the framework supply the caption, value, table, and key context for you.
|
||||
|
||||
See sample: `fielderror-default-message-logic.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
Re-testing a condition you already evaluated, or passing a fully formed sentence like `'The Amount field must be positive.'` to `FieldError`. The result reads as `Amount The Amount field must be positive. in Gen. Journal Line ...` — capital letter mid-sentence, caption and value repeated, and a stray trailing clause. Reviewer signals: a `FieldError` argument that names the field, restates the current value, starts with a capital letter, or ends with a period. Each is a sign the author treated `FieldError` like `Error` instead of as a predicate slotted into framework-generated context.
|
||||
|
||||
See sample: `fielderror-default-message-logic.bad.al`.
|
||||
|
|
@ -14,5 +14,9 @@ application-area: [all]
|
|||
## Best Practice
|
||||
Use `TestField` when the condition is a simple presence-or-equality check on a single field — mandatory-field gates and prerequisite checks at the top of a procedure read clearly and self-document intent. Use `FieldError` inside an `OnValidate` trigger or a validation procedure where surrounding business logic has already determined the value is invalid and you want a specific, custom message. Rely on the built-in field-and-record context both methods add rather than re-stating the field name in the text.
|
||||
|
||||
See sample: `fielderror-vs-testfield.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
Calling `FieldError` to "test" a field — placing it on a path that is reached unconditionally and expecting it to validate — terminates execution every time because `FieldError` never evaluates a condition. The inverse smell is reaching for `TestField` when the rule needs a tailored message, then bolting a vague generic string onto a check that cannot express the real business reason. A reviewer can spot the first by a `FieldError` that is not guarded by a preceding `if`, and the second by a `TestField` whose intent comment describes a condition more complex than presence or equality.
|
||||
|
||||
See sample: `fielderror-vs-testfield.bad.al`.
|
||||
|
|
|
|||
|
|
@ -24,3 +24,7 @@ See sample: `ignored-tryfunction-return-disables-try-semantics.good.al`.
|
|||
Calling a `[TryFunction]` procedure as a standalone statement and assuming the attribute suppresses its errors. The call has ordinary error semantics because its Boolean result is ignored.
|
||||
|
||||
See sample: `ignored-tryfunction-return-disables-try-semantics.bad.al`.
|
||||
|
||||
## See also
|
||||
|
||||
`microsoft/knowledge/performance/use-tryfunction-for-error-catching-not-rollback.md` owns transaction rollback expectations after a try method has actually caught an error.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,18 @@
|
|||
pageextension 50445 "Customer Balance Hidden" extends "Customer Card"
|
||||
{
|
||||
layout
|
||||
{
|
||||
addlast(General)
|
||||
{
|
||||
field(Balance; Rec.Balance)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
ToolTip = 'Specifies the customer balance.';
|
||||
Visible = ShowBalance;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
var
|
||||
ShowBalance: Boolean;
|
||||
}
|
||||
|
|
@ -0,0 +1,29 @@
|
|||
pageextension 50444 "Customer Balance Lazy" extends "Customer Card"
|
||||
{
|
||||
layout
|
||||
{
|
||||
addlast(General)
|
||||
{
|
||||
field("Balance Preview"; BalancePreview)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Balance Preview';
|
||||
ToolTip = 'Specifies the balance when balance details are enabled.';
|
||||
Visible = ShowBalance;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
trigger OnAfterGetCurrRecord()
|
||||
begin
|
||||
Clear(BalancePreview);
|
||||
if not ShowBalance then
|
||||
exit;
|
||||
Rec.CalcFields(Balance);
|
||||
BalancePreview := Rec.Balance;
|
||||
end;
|
||||
|
||||
var
|
||||
BalancePreview: Decimal;
|
||||
ShowBalance: Boolean;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [flowfield, visible, page-control, calculate-only-visible-flowfields, feature-management, hidden-field]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Hidden page FlowFields still calculate unless visible-only calculation is enabled
|
||||
|
||||
## Description
|
||||
|
||||
By default, a FlowField used directly as a page control's source is calculated when the page loads even when `Visible = false` or its visibility expression evaluates to false. The hidden control can therefore issue an aggregate query that no user sees. Business Central 26 introduced the **Calculate only visible FlowFields** feature-management option; only environments with that option enabled skip calculation for controls that are not visible.
|
||||
|
||||
## Best Practice
|
||||
|
||||
On BC 26 and later, enable and verify the visible-only FlowField feature before relying on `Visible` to suppress calculation. When the target environment does not guarantee that option, avoid binding an expensive FlowField directly to a usually-hidden control: calculate it only in the branch that displays it and bind the page control to a variable. Do not flag a hidden FlowField when the v26 feature is known to be enabled or the FlowField is cheap and intentionally preloaded.
|
||||
|
||||
See sample: `hidden-flowfields-still-calculate-before-bc26-opt-in.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Adding a costly Sum or Lookup FlowField to a page with `Visible = SomeRareMode` and assuming the hidden state prevents its query on all supported versions. The review signal is the direct FlowField source plus conditional or false visibility, not visibility alone.
|
||||
|
||||
See sample: `hidden-flowfields-still-calculate-before-bc26-opt-in.bad.al`.
|
||||
|
|
@ -11,11 +11,11 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
`[TryFunction]` annotates a method so that errors raised inside it can be caught by the caller instead of propagating. Per the platform reference, "changes to the database that are made with a try method aren't rolled back" — the attribute catches the error; it does not unwind database state. This is the critical distinction from `Codeunit.Run`, which does roll back on error (see `codeunit-run-as-atomic-sub-operation.md`). A try function also only catches when its return value is used: "If the return variable for a call to a function, which is attributed with [TryFunction] isn't used, then the call isn't considered a try function call." `DoTry();` propagates errors normally; only `ok := DoTry();` or `if DoTry() then ...` catches. The return type is forced to Boolean; user-defined return types are not allowed, and the value isn't accessible inside the try method itself. On Business Central on-premises, writes inside a try method are blocked by default and raise a runtime error unless `DisableWriteInsideTryFunctions` is set to `false` on the server — SaaS has no such restriction.
|
||||
`[TryFunction]` lets a caller catch an error, but database changes made before that error are not rolled back. The attribute catches; it does not unwind transaction state. This is the critical distinction from `Codeunit.Run`, which can provide an atomic rollback boundary (see `codeunit-run-as-atomic-sub-operation.md`). On Business Central on-premises, writes inside a try method are blocked by default unless `DisableWriteInsideTryFunctions` is set to `false`; SaaS does not provide that server setting.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Reach for `[TryFunction]` when you want to catch a failure without unwinding the transaction — HTTP calls whose non-2xx responses should surface a user-friendly message, .NET interop whose exceptions you want to translate, validation or parsing routines whose errors you intend to log and continue past. Always capture the return: `if MyTry() then ... else HandleFailure(GetLastErrorText());`. When the work is transactional — writes that must either fully apply or fully revert — use `Codeunit.Run` instead. The two primitives solve different problems: one catches errors, the other bounds a rollback scope.
|
||||
Reach for `[TryFunction]` when you want to catch a failure without unwinding the transaction — for example, third-party interop or parsing whose error you intend to translate. When writes must either fully apply or fully revert, use `Codeunit.Run` instead. The two primitives solve different problems: one catches errors, the other bounds a rollback scope.
|
||||
|
||||
Use `[TryFunction]` sparingly. Each caught error writes to the session-wide `GetLastErrorText` and `GetLastErrorCallStack` buffers, and every subsequent catch overwrites the earlier state — a helper that reads `GetLastErrorText` later may see a different error than the one it intended to inspect. Prefer explicit checks (non-throwing predicates, guard conditions, upfront validation) for operations with predictable failure modes; reserve `[TryFunction]` for genuinely unpredictable failures such as network calls, third-party interop, or evaluation of user-supplied expressions. When you do catch, read `GetLastErrorText` immediately after the failed call, and call `ClearLastError` before the call if an earlier catch in the same scope could have left state behind — per the platform reference, "If you call the GetLastErrorText method immediately after you call the ClearLastError method, then an empty string is returned."
|
||||
|
||||
|
|
@ -23,6 +23,10 @@ See sample: `use-tryfunction-for-error-catching-not-rollback.good.al`.
|
|||
|
||||
## Anti Pattern
|
||||
|
||||
Wrapping database writes in `[TryFunction]` expecting the writes to roll back when the method errors. They do not: the writes that succeeded before the error remain, the caller receives `false`, and the corrupted-state bug surfaces in production. A related anti-pattern is calling a try function without capturing the return (`DoTry();`), which silently strips the error-catching behavior and lets the error propagate — the code looks defensive but behaves identically to an unwrapped call. A third is defensive sprinkling: wrapping every operation that *could* theoretically error in `[TryFunction]` on the theory that catching is always safer than propagating. Each extra catch pollutes the shared error buffer and makes the diagnostic signal harder to find when something real does fail.
|
||||
Wrapping database writes in `[TryFunction]` and expecting successful writes before the error to roll back. They remain, the caller receives `false`, and partially applied state can escape. Defensive sprinkling is also unsafe: every catch overwrites the session error buffer and can hide the failure a later helper intended to inspect.
|
||||
|
||||
See sample: `use-tryfunction-for-error-catching-not-rollback.bad.al`.
|
||||
|
||||
## See also
|
||||
|
||||
`microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md` owns the separate call-site rule that a try method's Boolean result must be consumed.
|
||||
|
|
|
|||
|
|
@ -15,7 +15,7 @@ Error method trace telemetry includes the AL error string only when the first `E
|
|||
|
||||
## Best Practice
|
||||
|
||||
Declare the complete message as a `Label` or `TextConst` and pass it directly to `Error`, followed by substitution values. The client receives the formatted message while telemetry can retain the static message template without using the dynamic values as its message. See `error-direct-substitution-safe-for-telemetry.md`.
|
||||
Declare the complete message as a `Label` or `TextConst` and pass it directly to `Error`, followed by substitution values. The client receives the formatted message while telemetry retains the static message template without using the dynamic values as its message. Independently review whether each substitution value is appropriate to show to the current user.
|
||||
|
||||
See sample: `avoid-strsubstno-prebuild-before-error.good.al`.
|
||||
|
||||
|
|
|
|||
|
|
@ -1,10 +0,0 @@
|
|||
codeunit 50205 "Privacy Sample Direct Error"
|
||||
{
|
||||
procedure ValidateCustomer(var Customer: Record Customer)
|
||||
var
|
||||
InvalidEmailErr: Label 'Customer %1 has an invalid e-mail address: %2.', Comment = '%1 = Customer No., %2 = E-Mail';
|
||||
begin
|
||||
if not Customer."E-Mail".Contains('@') then
|
||||
Error(InvalidEmailErr, Customer."No.", Customer."E-Mail");
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,24 +0,0 @@
|
|||
---
|
||||
bc-version: [20..]
|
||||
domain: privacy
|
||||
keywords: [error, strsubstno, direct-substitution, telemetry, classification, label]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use a Label or TextConst for the Error telemetry message
|
||||
|
||||
## Description
|
||||
|
||||
For Error method trace telemetry, the platform includes the AL error string only when `Error` receives a `Label` or `TextConst` as its first argument. Substitution values format the client message, but the static label supplies the telemetry message and preserves its classification context. A string literal, local `Text`, `StrSubstNo` result, or concatenation is not equivalent: telemetry substitutes generic guidance instead of that dynamic string.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Define the complete error template as a `Label` with placeholder comments, pass the label directly as the first argument, and pass values separately. Independently review whether those values are appropriate to show to the current user.
|
||||
|
||||
See sample: `error-direct-substitution-safe-for-telemetry.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Assuming that any direct format string is telemetry-safe, or that a `StrSubstNo`/concatenated first argument is logged verbatim. The required telemetry shape is specifically a directly supplied `Label` or `TextConst`; see `avoid-strsubstno-prebuild-before-error.md`.
|
||||
|
|
@ -2,20 +2,20 @@ codeunit 50213 "Privacy Sample Telemetry Bad"
|
|||
{
|
||||
procedure LogCustomerProcessed(var Customer: Record Customer)
|
||||
begin
|
||||
Session.LogMessage('0000', StrSubstNo('Processed %1', Customer.Name), Verbosity::Normal,
|
||||
Session.LogMessage('PRIV0001', StrSubstNo('Processed %1', Customer.Name), Verbosity::Normal,
|
||||
DataClassification::SystemMetadata, TelemetryScope::All,
|
||||
'Category', 'Privacy');
|
||||
end;
|
||||
|
||||
procedure LogFileError(FileName: Text)
|
||||
begin
|
||||
Session.LogMessage('0001', StrSubstNo('Error processing file %1', FileName), Verbosity::Error,
|
||||
Session.LogMessage('PRIV0002', StrSubstNo('Error processing file %1', FileName), Verbosity::Error,
|
||||
DataClassification::SystemMetadata, TelemetryScope::All);
|
||||
end;
|
||||
|
||||
procedure LogEmployeeUpdate(EmployeeCode: Code[20])
|
||||
begin
|
||||
Session.LogMessage('0002', StrSubstNo('Employee %1 updated record', EmployeeCode), Verbosity::Normal,
|
||||
Session.LogMessage('PRIV0003', StrSubstNo('Employee %1 updated record', EmployeeCode), Verbosity::Normal,
|
||||
DataClassification::SystemMetadata, TelemetryScope::All);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,14 +2,14 @@ codeunit 50212 "Privacy Sample Telemetry Good"
|
|||
{
|
||||
procedure LogCustomerProcessed(var Customer: Record Customer)
|
||||
begin
|
||||
Session.LogMessage('0000', 'Customer record processed', Verbosity::Normal,
|
||||
Session.LogMessage('PRIV0001', 'Customer record processed', Verbosity::Normal,
|
||||
DataClassification::SystemMetadata, TelemetryScope::All,
|
||||
'Category', 'Privacy');
|
||||
end;
|
||||
|
||||
procedure LogFileError()
|
||||
begin
|
||||
Session.LogMessage('0001', 'Error processing uploaded file', Verbosity::Error,
|
||||
Session.LogMessage('PRIV0002', 'Error processing uploaded file', Verbosity::Error,
|
||||
DataClassification::SystemMetadata, TelemetryScope::All);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,6 +2,6 @@ codeunit 50211 "Privacy Sample LogMessage Bad"
|
|||
{
|
||||
procedure LogCompleted()
|
||||
begin
|
||||
Session.LogMessage('0003', 'Operation completed', Verbosity::Normal);
|
||||
Session.LogMessage('PRIV0004', 'Operation completed', Verbosity::Normal);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ codeunit 50210 "Privacy Sample LogMessage Good"
|
|||
{
|
||||
procedure LogCompleted()
|
||||
begin
|
||||
Session.LogMessage('0003', 'Operation completed', Verbosity::Normal,
|
||||
Session.LogMessage('PRIV0004', 'Operation completed', Verbosity::Normal,
|
||||
DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,28 @@
|
|||
query 50426 "Query Reuse Bad"
|
||||
{
|
||||
QueryType = Normal;
|
||||
|
||||
elements
|
||||
{
|
||||
dataitem(Customer; Customer)
|
||||
{
|
||||
column(CustomerNo; "No.") { }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50427 "Query Reuse Bad"
|
||||
{
|
||||
procedure ReadAgain(CustomerNoFilter: Code[20])
|
||||
var
|
||||
CustomerQuery: Query "Query Reuse Bad";
|
||||
begin
|
||||
CustomerQuery.SetRange(CustomerNo, CustomerNoFilter);
|
||||
CustomerQuery.Open();
|
||||
if CustomerQuery.Read() then;
|
||||
|
||||
// Reopening resets to the first row and retains CustomerNo.
|
||||
CustomerQuery.Open();
|
||||
if CustomerQuery.Read() then;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,39 @@
|
|||
query 50424 "Query Reuse Good"
|
||||
{
|
||||
QueryType = Normal;
|
||||
|
||||
elements
|
||||
{
|
||||
dataitem(Customer; Customer)
|
||||
{
|
||||
column(CustomerNo; "No.") { }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50425 "Query Reuse Good"
|
||||
{
|
||||
procedure ReadTwoIndependentSets(FirstNo: Code[20]; SecondNo: Code[20])
|
||||
var
|
||||
CustomerQuery: Query "Query Reuse Good";
|
||||
begin
|
||||
CustomerQuery.SetRange(CustomerNo, FirstNo);
|
||||
ReadAll(CustomerQuery);
|
||||
|
||||
Clear(CustomerQuery);
|
||||
CustomerQuery.SetRange(CustomerNo, SecondNo);
|
||||
ReadAll(CustomerQuery);
|
||||
end;
|
||||
|
||||
local procedure ReadAll(var CustomerQuery: Query "Query Reuse Good")
|
||||
begin
|
||||
CustomerQuery.Open();
|
||||
while CustomerQuery.Read() do
|
||||
ProcessCustomer(CustomerQuery.CustomerNo);
|
||||
CustomerQuery.Close();
|
||||
end;
|
||||
|
||||
local procedure ProcessCustomer(CustomerNo: Code[20])
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: query
|
||||
keywords: [query, open, close, clear, cursor, filters, reuse]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Reopening a Query resets its cursor but keeps its filters
|
||||
|
||||
## Description
|
||||
|
||||
Calling `Open()` on an already open query first closes the current dataset and opens it again. The next `Read()` starts at the first row; it does not continue from the previous cursor. Reopening also retains filters previously applied to the query variable. Only `Clear(QueryVariable)` resets those filters, so reuse can unexpectedly reread the first row or carry an old filter into a logically separate operation.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Open once for one read pass. Close after the pass, and call `Clear(QueryVariable)` before reusing the variable for a logically independent query whose filters must start empty. Set the next pass's filters explicitly before reopening.
|
||||
|
||||
See sample: `reopening-query-resets-cursor-but-keeps-filters.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `Open()` inside or between reads to "advance" or "start fresh", or reusing the same query variable for a new operation while assuming `Open()` cleared old filters. The code compiles but can repeatedly process the first row or silently omit rows behind a retained filter.
|
||||
|
||||
See sample: `reopening-query-resets-cursor-but-keeps-filters.bad.al`.
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
query 50422 "Query Customer Sales Bad"
|
||||
{
|
||||
QueryType = Normal;
|
||||
|
||||
elements
|
||||
{
|
||||
dataitem(Customer; Customer)
|
||||
{
|
||||
column(CustomerNo; "No.") { }
|
||||
column(CustomerName; Name) { }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50423 "Query Filter Order Bad"
|
||||
{
|
||||
procedure ReadCustomer(CustomerNoFilter: Code[20])
|
||||
var
|
||||
CustomerSales: Query "Query Customer Sales Bad";
|
||||
begin
|
||||
CustomerSales.Open();
|
||||
CustomerSales.SetRange(CustomerNo, CustomerNoFilter);
|
||||
while CustomerSales.Read() do
|
||||
ProcessCustomer(CustomerSales.CustomerNo);
|
||||
end;
|
||||
|
||||
local procedure ProcessCustomer(CustomerNo: Code[20])
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,31 @@
|
|||
query 50420 "Query Customer Sales Good"
|
||||
{
|
||||
QueryType = Normal;
|
||||
|
||||
elements
|
||||
{
|
||||
dataitem(Customer; Customer)
|
||||
{
|
||||
column(CustomerNo; "No.") { }
|
||||
column(CustomerName; Name) { }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50421 "Query Filter Order Good"
|
||||
{
|
||||
procedure ReadCustomer(CustomerNoFilter: Code[20])
|
||||
var
|
||||
CustomerSales: Query "Query Customer Sales Good";
|
||||
begin
|
||||
CustomerSales.SetRange(CustomerNo, CustomerNoFilter);
|
||||
CustomerSales.Open();
|
||||
while CustomerSales.Read() do
|
||||
ProcessCustomer(CustomerSales.CustomerNo);
|
||||
CustomerSales.Close();
|
||||
end;
|
||||
|
||||
local procedure ProcessCustomer(CustomerNo: Code[20])
|
||||
begin
|
||||
end;
|
||||
}
|
||||
26
microsoft/knowledge/query/set-query-filters-before-open.md
Normal file
26
microsoft/knowledge/query/set-query-filters-before-open.md
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: query
|
||||
keywords: [query, setfilter, setrange, open, read, dataset, filter-order]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Set Query filters before Open
|
||||
|
||||
## Description
|
||||
|
||||
`Query.SetFilter` and `Query.SetRange` automatically close an open query dataset. A call placed after `Open()` therefore does not refine the rows already being read; it ends that dataset. The next `Read()` has no open dataset unless the code explicitly calls `Open()` again, so a plausible filter change can turn a working loop into an empty or failing read sequence without a compiler diagnostic.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Apply every filter before `Open()`, then read the dataset to completion and call `Close()`. When a later branch needs different filters, close or clear the query, set the new filters, and open a new dataset deliberately.
|
||||
|
||||
See sample: `set-query-filters-before-open.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`Query.Open()` followed by `SetFilter` or `SetRange` and then `Read()` under the assumption that the filter updates the open cursor. Refiltering after `Open()` is valid only when the code intentionally opens a fresh dataset afterward.
|
||||
|
||||
See sample: `set-query-filters-before-open.bad.al`.
|
||||
|
|
@ -1,28 +0,0 @@
|
|||
table 50100 "Customer Feedback"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Feedback No."; Code[20])
|
||||
{
|
||||
// No DataClassification declared. Defaults to ToBeClassified.
|
||||
}
|
||||
field(2; "Contact Name"; Text[100])
|
||||
{
|
||||
DataClassification = ToBeClassified;
|
||||
}
|
||||
field(3; "Email"; Text[80])
|
||||
{
|
||||
// Personal data classified as CustomerContent understates privacy impact.
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
field(4; "Feedback Text"; Text[2048])
|
||||
{
|
||||
DataClassification = ToBeClassified;
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Feedback No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
|
@ -1,36 +0,0 @@
|
|||
table 50100 "Customer Feedback"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Feedback No."; Code[20])
|
||||
{
|
||||
DataClassification = SystemMetadata;
|
||||
}
|
||||
field(2; "Contact Name"; Text[100])
|
||||
{
|
||||
DataClassification = EndUserIdentifiableInformation;
|
||||
}
|
||||
field(3; "Email"; Text[80])
|
||||
{
|
||||
DataClassification = EndUserIdentifiableInformation;
|
||||
}
|
||||
field(4; "Product Code"; Code[20])
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
field(5; "Feedback Text"; Text[2048])
|
||||
{
|
||||
// When uncertain between CustomerContent and EUII, prefer the stronger protection.
|
||||
DataClassification = EndUserIdentifiableInformation;
|
||||
}
|
||||
field(6; "Submitted DateTime"; DateTime)
|
||||
{
|
||||
DataClassification = SystemMetadata;
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Feedback No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
|
@ -1,26 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: security
|
||||
keywords: [dataclassification, gdpr, privacy, euii, compliance]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Classify every field with DataClassification
|
||||
|
||||
## Description
|
||||
|
||||
Every field on every AL table and table extension must have a resolved `DataClassification` value, either declared directly on the field or inherited from a table-level default. The value drives GDPR tooling, data-subject requests, retention policies, and audit reporting — all of which rely on the field metadata to know what data to include, anonymize, or delete. A field with no field-level property and no table-level default resolves to `ToBeClassified`, which is a compliance gap, not a neutral state.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Choose the narrowest value that accurately describes the field's content: `EndUserIdentifiableInformation` for data that directly identifies a person, `EndUserPseudonymousIdentifiers` for indirect identifiers, `CustomerContent` for business operational data, `SystemMetadata` for system-generated housekeeping, `AccountData` for tenant/billing, `OrganizationIdentifiableInformation` for organization-level identifiers. Use a table-level default for homogeneous tables, and override individual fields whose content differs from that default. When uncertain between two values, pick the stronger protection.
|
||||
|
||||
See sample: `classify-every-field-with-dataclassification.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Leaving `DataClassification = ToBeClassified` on a field, omitting classification when the table has no default, or relying on a table-level default that understates a field's actual content. Code in this state fails compliance audits and breaks the subject-access-request and retention tooling that depends on the property being set correctly.
|
||||
|
||||
See sample: `classify-every-field-with-dataclassification.bad.al`.
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
codeunit 50471 "Unprotected Setup Action"
|
||||
{
|
||||
Access = Internal;
|
||||
|
||||
trigger OnRun()
|
||||
begin
|
||||
// Internal does not prevent another extension from invoking this OnRun
|
||||
// through Codeunit.Run.
|
||||
UpdateSensitiveSetup();
|
||||
end;
|
||||
|
||||
local procedure UpdateSensitiveSetup()
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,39 @@
|
|||
table 50468 "Sensitive Setup"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "Primary Key"; Code[10]) { }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50469 "Setup Authorization"
|
||||
{
|
||||
procedure CanManageSetup(): Boolean
|
||||
var
|
||||
SensitiveSetup: Record "Sensitive Setup";
|
||||
begin
|
||||
exit(SensitiveSetup.WritePermission());
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50470 "Protected Setup Action"
|
||||
{
|
||||
Access = Internal;
|
||||
|
||||
trigger OnRun()
|
||||
begin
|
||||
if not SetupAuthorization.CanManageSetup() then
|
||||
Error(NotAuthorizedErr);
|
||||
UpdateSensitiveSetup();
|
||||
end;
|
||||
|
||||
local procedure UpdateSensitiveSetup()
|
||||
begin
|
||||
end;
|
||||
|
||||
var
|
||||
SetupAuthorization: Codeunit "Setup Authorization";
|
||||
NotAuthorizedErr: Label 'You are not authorized to manage this setup.';
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: security
|
||||
keywords: [access, internal, internalsvisibleto, recordref, codeunit-run, security-boundary, authorization]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Access Internal is API hygiene, not an authorization boundary
|
||||
|
||||
## Description
|
||||
|
||||
`Access = Internal` controls compile-time symbol visibility. It does not prevent runtime access through mechanisms such as `RecordRef`, `TransferFields`, or `Codeunit.Run`, and `internalsVisibleTo` deliberately grants compile-time access to named companion apps. Microsoft explicitly documents that access modifiers cannot be used as a security boundary.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `internal` to keep implementation details out of the supported API, but enforce sensitive operations with permissions, entitlements, and explicit authorization checks appropriate to the operation. Treat `internalsVisibleTo` as a same-publisher development/testability relationship, not as a trust grant for secrets or elevated data access.
|
||||
|
||||
See sample: `internal-access-is-not-a-security-boundary.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Placing privileged work in an internal codeunit and claiming that other extensions cannot invoke it, or exposing an app to a different publisher through `internalsVisibleTo` because `internal` is assumed to protect the underlying operation. The access modifier narrows supported callers; it does not authenticate runtime callers.
|
||||
|
||||
See sample: `internal-access-is-not-a-security-boundary.bad.al`.
|
||||
|
|
@ -17,6 +17,10 @@ API keys, OAuth tokens, client secrets, and connection strings must not be store
|
|||
|
||||
Persist every credential in `IsolatedStorage`, write it at the point of capture, and read it only when needed. Prefer `SetEncrypted` when the value fits its documented length limit. On BC24 and later, carry the value through the `SecretText` overloads; on earlier releases, keep any required `Text` handling inside a `[NonDebuggable]` boundary. Choose the `DataScope` that matches the credential's lifetime. See `isolatedstorage-datascope-module-vs-company`, `isolatedstorage-setencrypted-for-sensitive-values`, and `secrettext-for-credentials` for those separate concerns.
|
||||
|
||||
See sample: `secrets-isolated-storage.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A "Setup" or "Connection" table carrying a `Text` field named `API Key`, `Password`, or `Client Secret`. The value is now readable by any object with table permission, ships in RapidStart packages and Excel exports, and appears in record snapshots — a credential disclosure that no amount of encryption-in-transit elsewhere makes up for. Reviewer signal: a secret-shaped field declared on a table instead of an `IsolatedStorage` call.
|
||||
|
||||
See sample: `secrets-isolated-storage.bad.al`.
|
||||
|
|
|
|||
|
|
@ -19,4 +19,4 @@ Declare credential-carrying parameters and variables as `SecretText` from the ca
|
|||
|
||||
## Anti Pattern
|
||||
|
||||
Holding a credential in a `Text` variable (`BearerToken: Text`), concatenating it into a header, then passing it to `HttpClient`. The token is visible in the debugger and in any error that prints the variable, and the compiler offers no help because the type was wrong from the start. Reviewers should flag any local or parameter named like a secret (`ApiKey`, `Token`, `Password`, `ClientSecret`) whose type is `Text` or `Code`. See sample: `secrettext-for-credentials.bad.al`.
|
||||
Holding a credential in a `Text` variable (`BearerToken: Text`) makes it visible in the debugger and in any error that prints the variable, and the compiler offers no help because the type was wrong from the start. Reviewers should flag any local or parameter named like a secret (`ApiKey`, `Token`, `Password`, `ClientSecret`) whose type is `Text` or `Code`. When the same value is visibly sent through an HTTP URI, header, or body, `secrettext-with-httpclient.md` is the more specific primary rule. See sample: `secrettext-for-credentials.bad.al`.
|
||||
|
|
|
|||
|
|
@ -19,4 +19,4 @@ Compose a secret URI with `SecretStrSubstNo`, call `Request.SetSecretRequestUri(
|
|||
|
||||
## Anti Pattern
|
||||
|
||||
Holding a credential in `Text`, interpolating it with `StrSubstNo` or concatenation, and passing that plain text to `HttpClient.Get` or `HttpHeaders.Add`. The secret-aware request and header APIs remove the need to materialize the value as `Text`. See sample: `secrettext-with-httpclient.bad.al`.
|
||||
Holding a credential in `Text`, interpolating it with `StrSubstNo` or concatenation, and passing that plain text to `HttpClient.Get` or `HttpHeaders.Add`. The secret-aware request and header APIs remove the need to materialize the value as `Text`. This HTTP-sink rule supersedes the generic `secrettext-for-credentials.md` rule at the same location. See sample: `secrettext-with-httpclient.bad.al`.
|
||||
|
|
|
|||
|
|
@ -1,13 +1,24 @@
|
|||
table 50253 "Sample Caption Bad"
|
||||
page 50253 "Sample Caption Bad"
|
||||
{
|
||||
fields
|
||||
PageType = Card;
|
||||
SourceTable = Customer;
|
||||
|
||||
layout
|
||||
{
|
||||
field(1; "Customer No."; Code[20])
|
||||
area(Content)
|
||||
{
|
||||
}
|
||||
field(2; "Is Active"; Boolean)
|
||||
{
|
||||
Caption = '';
|
||||
group(General)
|
||||
{
|
||||
field("Customer No."; Rec."No.")
|
||||
{
|
||||
ApplicationArea = All;
|
||||
}
|
||||
field("Customer Name"; Rec.Name)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = '';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,17 +1,28 @@
|
|||
table 50252 "Sample Caption Good"
|
||||
page 50252 "Sample Caption Good"
|
||||
{
|
||||
fields
|
||||
PageType = Card;
|
||||
SourceTable = Customer;
|
||||
|
||||
layout
|
||||
{
|
||||
field(1; "Customer No."; Code[20])
|
||||
area(Content)
|
||||
{
|
||||
Caption = 'Customer No.';
|
||||
}
|
||||
field(2; "Enabled"; Boolean)
|
||||
{
|
||||
}
|
||||
field(3; Amount; Decimal)
|
||||
{
|
||||
CaptionClass = '3,5,' + 'USD';
|
||||
group(General)
|
||||
{
|
||||
Caption = 'General';
|
||||
field("Customer No."; Rec."No.")
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Customer No.';
|
||||
ToolTip = 'Specifies the customer number.';
|
||||
}
|
||||
field("Customer Name"; Rec.Name)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Customer Name';
|
||||
ToolTip = 'Specifies the customer name.';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -15,12 +15,12 @@ A `Record` variable declared with the `temporary` modifier behaves nothing like
|
|||
|
||||
## Best Practice
|
||||
|
||||
Every variable of type `Record X temporary` must start with `Temp`. The same applies to parameters: a procedure that receives a temporary record as a buffer names the parameter `TempBuffer`, `TempSalesLine`, and so on. The convention extends naturally to derived names — `TempJobWIPBufferCopy`, `TempSourceSalesLine` — anything that starts with `Temp` is in-memory.
|
||||
Every local or global variable of type `Record X temporary` must start with `Temp`. Ordinary procedure parameters follow the same convention. Event publisher parameters are owned by the events-domain rule `prefix-temporary-record-event-parameters-with-temp.md`; the style leaf must not emit a second finding for the same event parameter.
|
||||
|
||||
See sample: `temporary-variable-temp-prefix.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`WIPBuffer: Record "Job WIP Buffer" temporary;` reads at the call site as if it were a database operation: `WIPBuffer.Insert()` looks identical to a write to the underlying table. The reader has to scroll back to the declaration to discover that this is in-memory, every time.
|
||||
`WIPBuffer: Record "Job WIP Buffer" temporary;` as a local, global, or ordinary procedure parameter reads at the call site as if it were a database operation. Exclude event publisher parameters here so the events leaf remains their single owner.
|
||||
|
||||
See sample: `temporary-variable-temp-prefix.bad.al`.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,26 @@
|
|||
codeunit 50401 "Telemetry Scope Bad"
|
||||
{
|
||||
procedure LogIntegrationFailure()
|
||||
begin
|
||||
// Tenant operators cannot see an actionable integration failure.
|
||||
Session.LogMessage(
|
||||
'TLM0004',
|
||||
'Document exchange failed',
|
||||
Verbosity::Error,
|
||||
DataClassification::SystemMetadata,
|
||||
TelemetryScope::ExtensionPublisher,
|
||||
'Operation', 'DocumentExchange');
|
||||
end;
|
||||
|
||||
procedure LogCacheMiss()
|
||||
begin
|
||||
// Environment telemetry receives publisher-only implementation noise.
|
||||
Session.LogMessage(
|
||||
'TLM0005',
|
||||
'Internal cache entry missed',
|
||||
Verbosity::Verbose,
|
||||
DataClassification::SystemMetadata,
|
||||
TelemetryScope::All,
|
||||
'Cache', 'ExchangeMetadata');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,24 @@
|
|||
codeunit 50400 "Telemetry Scope Good"
|
||||
{
|
||||
procedure LogIntegrationFailure()
|
||||
begin
|
||||
Session.LogMessage(
|
||||
'TLM0002',
|
||||
'Document exchange failed',
|
||||
Verbosity::Error,
|
||||
DataClassification::SystemMetadata,
|
||||
TelemetryScope::All,
|
||||
'Operation', 'DocumentExchange');
|
||||
end;
|
||||
|
||||
procedure LogCacheMiss()
|
||||
begin
|
||||
Session.LogMessage(
|
||||
'TLM0003',
|
||||
'Internal cache entry missed',
|
||||
Verbosity::Verbose,
|
||||
DataClassification::SystemMetadata,
|
||||
TelemetryScope::ExtensionPublisher,
|
||||
'Cache', 'ExchangeMetadata');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [17..]
|
||||
domain: telemetry
|
||||
keywords: [telemetryscope, extensionpublisher, all, audience, logmessage, application-insights]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Choose TelemetryScope by who must receive the signal
|
||||
|
||||
## Description
|
||||
|
||||
`TelemetryScope::ExtensionPublisher` sends a custom trace only to the Application Insights resource configured by the extension publisher. `TelemetryScope::All` also sends it to the environment's telemetry, where the customer or partner operating the tenant can query it. The compiler accepts either value, so a plausible-looking scope can silently hide an actionable signal from tenant operators or expose publisher-only implementation noise to them.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `ExtensionPublisher` for internal diagnostics that only the app publisher can interpret, such as cache behavior or private algorithm state. Use `All` for signals the tenant operator can act on, such as an integration failure, quota warning, or setup problem. Decide the audience independently from `DataClassification`; privacy guidance still governs whether the payload may be emitted at all.
|
||||
|
||||
See sample: `choose-telemetry-scope-by-audience.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Defaulting every call to `All`, including low-level implementation diagnostics, or defaulting every call to `ExtensionPublisher` and thereby hiding customer-actionable failures from environment telemetry. Review only when the message and surrounding branch make the intended audience clear; an ambiguous diagnostic is not enough to infer the wrong scope.
|
||||
|
||||
See sample: `choose-telemetry-scope-by-audience.bad.al`.
|
||||
|
|
@ -0,0 +1,11 @@
|
|||
codeunit 50405 "Feature Uptake Bad"
|
||||
{
|
||||
procedure FeatureOpened()
|
||||
var
|
||||
FeatureTelemetry: Codeunit "Feature Telemetry";
|
||||
begin
|
||||
// The first uptake state skips Discovered and is not emitted.
|
||||
FeatureTelemetry.LogUptake(
|
||||
'TLM0011', 'Document exchange', Enum::"Feature Uptake Status"::Used);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
codeunit 50404 "Feature Uptake Good"
|
||||
{
|
||||
procedure FeatureDiscovered()
|
||||
var
|
||||
FeatureTelemetry: Codeunit "Feature Telemetry";
|
||||
begin
|
||||
FeatureTelemetry.LogUptake(
|
||||
'TLM0008', 'Document exchange', Enum::"Feature Uptake Status"::Discovered);
|
||||
end;
|
||||
|
||||
procedure FeatureSetUp()
|
||||
var
|
||||
FeatureTelemetry: Codeunit "Feature Telemetry";
|
||||
begin
|
||||
FeatureTelemetry.LogUptake(
|
||||
'TLM0009', 'Document exchange', Enum::"Feature Uptake Status"::"Set up");
|
||||
end;
|
||||
|
||||
procedure FeatureUsed()
|
||||
var
|
||||
FeatureTelemetry: Codeunit "Feature Telemetry";
|
||||
begin
|
||||
FeatureTelemetry.LogUptake(
|
||||
'TLM0010', 'Document exchange', Enum::"Feature Uptake Status"::Used);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [18..]
|
||||
domain: telemetry
|
||||
keywords: [featuretelemetry, loguptake, discovered, set-up, used, uptake-status, lifecycle]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Emit FeatureTelemetry uptake states in lifecycle order
|
||||
|
||||
## Description
|
||||
|
||||
`FeatureTelemetry.LogUptake` accepts `Discovered`, `Set up`, `Used`, and `Undiscovered`, but the platform records the forward transition only as `Discovered` to `Set up` to `Used`. If the first call for a feature is `Set up` or `Used`, no uptake telemetry is emitted. `Undiscovered` is the explicit reset from any state.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Log `Discovered` when the user encounters the feature, `Set up` after its setup is completed, and `Used` when the user attempts it. Keep the same feature name throughout the funnel. Review ordering only when the changed repository context shows the feature's lifecycle; a single isolated `Used` call cannot prove that earlier states are absent elsewhere.
|
||||
|
||||
See sample: `feature-uptake-transitions-in-order.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Introducing a feature whose only uptake call jumps directly to `Set up` or `Used`, or using different feature-name literals for successive states. The calls compile and run, but the funnel silently omits the invalid transition.
|
||||
|
||||
See sample: `feature-uptake-transitions-in-order.bad.al`.
|
||||
|
|
@ -0,0 +1,23 @@
|
|||
codeunit 50407 "Feature Usage Bad"
|
||||
{
|
||||
procedure ExchangeDocument(ShouldFail: Boolean)
|
||||
var
|
||||
FeatureTelemetry: Codeunit "Feature Telemetry";
|
||||
begin
|
||||
FeatureTelemetry.LogUsage(
|
||||
'TLM0014', 'Document exchange', 'Document exchanged');
|
||||
|
||||
if not TryExchangeDocument(ShouldFail) then
|
||||
exit;
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryExchangeDocument(ShouldFail: Boolean)
|
||||
begin
|
||||
if ShouldFail then
|
||||
Error(ExchangeFailedErr);
|
||||
end;
|
||||
|
||||
var
|
||||
ExchangeFailedErr: Label 'Exchange failed.';
|
||||
}
|
||||
|
|
@ -0,0 +1,27 @@
|
|||
codeunit 50406 "Feature Usage Good"
|
||||
{
|
||||
procedure ExchangeDocument(ShouldFail: Boolean)
|
||||
var
|
||||
FeatureTelemetry: Codeunit "Feature Telemetry";
|
||||
begin
|
||||
if not TryExchangeDocument(ShouldFail) then begin
|
||||
FeatureTelemetry.LogError(
|
||||
'TLM0012', 'Document exchange', 'Exchanging document',
|
||||
GetLastErrorText(true), GetLastErrorCallStack());
|
||||
exit;
|
||||
end;
|
||||
|
||||
FeatureTelemetry.LogUsage(
|
||||
'TLM0013', 'Document exchange', 'Document exchanged');
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryExchangeDocument(ShouldFail: Boolean)
|
||||
begin
|
||||
if ShouldFail then
|
||||
Error(ExchangeFailedErr);
|
||||
end;
|
||||
|
||||
var
|
||||
ExchangeFailedErr: Label 'Exchange failed.';
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [18..]
|
||||
domain: telemetry
|
||||
keywords: [featuretelemetry, logusage, logerror, success, tryfunction, feature-usage]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Call FeatureTelemetry.LogUsage only after successful use
|
||||
|
||||
## Description
|
||||
|
||||
`FeatureTelemetry.LogUsage` means that a user successfully used the feature. An attempt belongs in the uptake funnel, while a failed operation belongs in `LogError`. Logging usage before checking the result inflates adoption metrics with failed attempts and makes usage telemetry disagree with the actual business outcome.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Call `LogUsage` only after the operation has completed successfully. On a failure path, call `LogError` with the captured error text and call stack when the failure must be emitted explicitly. Use a past-tense event name for usage and a present-tense scenario name for errors.
|
||||
|
||||
See sample: `feature-usage-only-after-success.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `LogUsage` before a Boolean result, `TryFunction`, `Codeunit.Run`, or HTTP status has been checked, or calling it in both success and failure branches. Do not flag an attempt recorded with `LogUptake(...Used)`; unlike `LogUsage`, that state intentionally records an attempt.
|
||||
|
||||
See sample: `feature-usage-only-after-success.bad.al`.
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 50412 "Telemetry Dimension Bad"
|
||||
{
|
||||
procedure LogBatchResult(RecordCount: Integer)
|
||||
var
|
||||
CustomDimensions: Dictionary of [Text, Text];
|
||||
begin
|
||||
CustomDimensions.Add('record count', Format(RecordCount));
|
||||
CustomDimensions.Add('result_code', 'Success');
|
||||
Session.LogMessage(
|
||||
'TLM0015', 'Order processing completed', Verbosity::Normal,
|
||||
DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher,
|
||||
CustomDimensions);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 50411 "Telemetry Dimension Good"
|
||||
{
|
||||
procedure LogBatchResult(RecordCount: Integer)
|
||||
var
|
||||
CustomDimensions: Dictionary of [Text, Text];
|
||||
begin
|
||||
CustomDimensions.Add('RecordCount', Format(RecordCount));
|
||||
CustomDimensions.Add('Result', 'Success');
|
||||
Session.LogMessage(
|
||||
'TLM0015', 'Order processing completed', Verbosity::Normal,
|
||||
DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher,
|
||||
CustomDimensions);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [17..]
|
||||
domain: telemetry
|
||||
keywords: [customdimensions, dimension-key, schema, pascalcase, kql, breaking-change]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Treat custom dimension keys as a stable telemetry schema
|
||||
|
||||
## Description
|
||||
|
||||
Business Central prefixes AL custom-dimension keys with `al` in Application Insights, so an AL key named `Result` becomes `alResult`. Microsoft guidance treats telemetry definitions as an API: changing or removing a custom dimension can break dashboards and alerts. PascalCase keys without spaces also compose cleanly in KQL; spaces force awkward bracket access and make queries harder to maintain.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Choose stable PascalCase keys such as `Operation`, `Result`, and `RecordCount`. Keep the key set and meaning stable for a shipped event ID; add a new event ID or coordinate a schema migration when the meaning must change. Privacy guidance separately governs whether a dimension value may contain customer data.
|
||||
|
||||
See sample: `keep-custom-dimension-schema-stable.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Keys such as `'order no'` or `'result_code'`, or renaming/removing a key while retaining the same shipped event ID. A naming-only issue is advisory; changing an existing event's schema is the material compatibility defect. New keys on a new event ID are not a breaking change.
|
||||
|
||||
See sample: `keep-custom-dimension-schema-stable.bad.al`.
|
||||
|
|
@ -0,0 +1,24 @@
|
|||
codeunit 50403 "Telemetry Verbosity Bad"
|
||||
{
|
||||
procedure RunExchange()
|
||||
begin
|
||||
if TryExchange() then
|
||||
exit;
|
||||
|
||||
Session.LogMessage(
|
||||
'TLM0007',
|
||||
'Document exchange failed',
|
||||
Verbosity::Normal,
|
||||
DataClassification::SystemMetadata,
|
||||
TelemetryScope::All);
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryExchange()
|
||||
begin
|
||||
Error(ExchangeFailedErr);
|
||||
end;
|
||||
|
||||
var
|
||||
ExchangeFailedErr: Label 'Exchange failed.';
|
||||
}
|
||||
|
|
@ -0,0 +1,24 @@
|
|||
codeunit 50402 "Telemetry Verbosity Good"
|
||||
{
|
||||
procedure RunExchange()
|
||||
begin
|
||||
if TryExchange() then
|
||||
exit;
|
||||
|
||||
Session.LogMessage(
|
||||
'TLM0006',
|
||||
'Document exchange failed',
|
||||
Verbosity::Error,
|
||||
DataClassification::SystemMetadata,
|
||||
TelemetryScope::All);
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryExchange()
|
||||
begin
|
||||
Error(ExchangeFailedErr);
|
||||
end;
|
||||
|
||||
var
|
||||
ExchangeFailedErr: Label 'Exchange failed.';
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [17..]
|
||||
domain: telemetry
|
||||
keywords: [verbosity, severitylevel, critical, error, warning, normal, verbose, logmessage]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Match telemetry Verbosity to the signal's actual severity
|
||||
|
||||
## Description
|
||||
|
||||
`Verbosity` becomes the Application Insights `severityLevel` and participates in on-premises diagnostic trace filtering. `Critical` represents abnormal termination, `Error` a severe error, `Warning` a warning, `Normal` a non-error event, and `Verbose` detailed tracing. Logging a caught failure as `Normal` is not cosmetic: severity-based alerts miss it, and an on-premises service configured to emit only warnings and above can drop it completely.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `Error` for failed operations that need investigation and `Critical` only for abnormal termination or equivalent loss of service. Use `Warning` for degraded but completed behavior, `Normal` for successful business events, and `Verbose` for detailed diagnostics. Judge the outcome, not the procedure name: an expected optional lookup miss can legitimately remain `Normal` or `Verbose`.
|
||||
|
||||
See sample: `match-verbosity-to-signal-severity.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A `Session.LogMessage` in a failed `TryFunction`, failed `Codeunit.Run`, unsuccessful HTTP response, or other explicit failure branch that uses `Verbosity::Normal` or `Verbose` without evidence that the failure is expected and benign.
|
||||
|
||||
See sample: `match-verbosity-to-signal-severity.bad.al`.
|
||||
|
|
@ -0,0 +1,37 @@
|
|||
codeunit 50409 "First Telemetry Logger" implements "Telemetry Logger"
|
||||
{
|
||||
Access = Internal;
|
||||
|
||||
procedure LogMessage(EventId: Text; Message: Text; Verbosity: Verbosity; DataClassification: DataClassification; TelemetryScope: TelemetryScope; CustomDimensions: Dictionary of [Text, Text])
|
||||
begin
|
||||
Session.LogMessage(
|
||||
EventId, Message, Verbosity, DataClassification, TelemetryScope, CustomDimensions);
|
||||
end;
|
||||
|
||||
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Telemetry Loggers", 'OnRegisterTelemetryLogger', '', true, true)]
|
||||
local procedure RegisterFirst(var Sender: Codeunit "Telemetry Loggers")
|
||||
var
|
||||
Logger: Codeunit "First Telemetry Logger";
|
||||
begin
|
||||
Sender.Register(Logger);
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50410 "Second Telemetry Logger" implements "Telemetry Logger"
|
||||
{
|
||||
Access = Internal;
|
||||
|
||||
procedure LogMessage(EventId: Text; Message: Text; Verbosity: Verbosity; DataClassification: DataClassification; TelemetryScope: TelemetryScope; CustomDimensions: Dictionary of [Text, Text])
|
||||
begin
|
||||
Session.LogMessage(
|
||||
EventId, Message, Verbosity, DataClassification, TelemetryScope, CustomDimensions);
|
||||
end;
|
||||
|
||||
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Telemetry Loggers", 'OnRegisterTelemetryLogger', '', true, true)]
|
||||
local procedure RegisterSecond(var Sender: Codeunit "Telemetry Loggers")
|
||||
var
|
||||
Logger: Codeunit "Second Telemetry Logger";
|
||||
begin
|
||||
Sender.Register(Logger);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
codeunit 50408 "Sample Telemetry Logger" implements "Telemetry Logger"
|
||||
{
|
||||
Access = Internal;
|
||||
|
||||
procedure LogMessage(EventId: Text; Message: Text; Verbosity: Verbosity; DataClassification: DataClassification; TelemetryScope: TelemetryScope; CustomDimensions: Dictionary of [Text, Text])
|
||||
begin
|
||||
Session.LogMessage(
|
||||
EventId, Message, Verbosity, DataClassification, TelemetryScope, CustomDimensions);
|
||||
end;
|
||||
|
||||
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Telemetry Loggers", 'OnRegisterTelemetryLogger', '', true, true)]
|
||||
local procedure OnRegisterTelemetryLogger(var Sender: Codeunit "Telemetry Loggers")
|
||||
var
|
||||
SampleTelemetryLogger: Codeunit "Sample Telemetry Logger";
|
||||
begin
|
||||
Sender.Register(SampleTelemetryLogger);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [18..]
|
||||
domain: telemetry
|
||||
keywords: [telemetry-logger, interface, register, publisher, featuretelemetry, onregistertelemetrylogger]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Register exactly one Telemetry Logger implementation per publisher
|
||||
|
||||
## Description
|
||||
|
||||
The `Telemetry` and `Feature Telemetry` codeunits reach an extension publisher's telemetry through an implementation of the `"Telemetry Logger"` interface registered with `"Telemetry Loggers".OnRegisterTelemetryLogger`. The platform requires exactly one registration per app publisher. No registration prevents the module from working as expected; multiple registrations make the destination ambiguous and produce platform error telemetry.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Place one internal logger implementation in one app for the publisher, forward its `LogMessage` method to `Session.LogMessage`, and register it from one event subscriber. Companion apps with the same publisher reuse that registration instead of each adding another. Evaluate absence only with repository or app-family context; a single-file diff cannot prove that no logger exists elsewhere.
|
||||
|
||||
See sample: `register-one-telemetry-logger-per-publisher.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Adding `FeatureTelemetry` calls to a complete app with no logger registration, or registering two logger implementations for apps that share the same publisher. The calls compile, but the telemetry module reports the missing or duplicate registration instead of behaving as intended.
|
||||
|
||||
See sample: `register-one-telemetry-logger-per-publisher.bad.al`.
|
||||
|
|
@ -1,4 +1,4 @@
|
|||
codeunit 50260 "Sample Telemetry Id Bad"
|
||||
codeunit 50260 "Telemetry Event Id Bad"
|
||||
{
|
||||
procedure LogCustomerProcessed(var Customer: Record Customer)
|
||||
begin
|
||||
|
|
@ -1,4 +1,4 @@
|
|||
codeunit 50261 "Sample Telemetry Id Good"
|
||||
codeunit 50261 "Telemetry Event Id Good"
|
||||
{
|
||||
procedure LogCustomerProcessed(var Customer: Record Customer)
|
||||
begin
|
||||
|
|
@ -1,6 +1,6 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
bc-version: [17..]
|
||||
domain: telemetry
|
||||
keywords: [telemetry, logmessage, event-id, sessionlogmessage, observability]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
codeunit 50483 "Protected Setup Action Bad"
|
||||
{
|
||||
trigger OnRun()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.Init();
|
||||
Customer."No." := 'SUPER-INSERT';
|
||||
Customer.Insert();
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50484 "Permission Test Bad"
|
||||
{
|
||||
Subtype = Test;
|
||||
TestPermissions = Disabled;
|
||||
|
||||
[Test]
|
||||
procedure LimitedUserCannotRunSetup()
|
||||
var
|
||||
SetupAction: Codeunit "Protected Setup Action Bad";
|
||||
begin
|
||||
// Disabled runs as SUPER; no limited-user boundary is exercised.
|
||||
asserterror SetupAction.Run();
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,43 @@
|
|||
permissionset 50480 "LIMITED USER"
|
||||
{
|
||||
Assignable = false;
|
||||
Permissions =
|
||||
tabledata Customer = R,
|
||||
codeunit "Protected Setup Action Test" = X;
|
||||
}
|
||||
|
||||
codeunit 50481 "Protected Setup Action Test"
|
||||
{
|
||||
trigger OnRun()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.Init();
|
||||
Customer."No." := 'NO-INSERT';
|
||||
Customer.Insert();
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50482 "Permission Test Good"
|
||||
{
|
||||
Subtype = Test;
|
||||
TestPermissions = Restrictive;
|
||||
|
||||
[Test]
|
||||
procedure LimitedUserCannotRunSetup()
|
||||
var
|
||||
PermissionsMock: Codeunit "Permissions Mock";
|
||||
SetupAction: Codeunit "Protected Setup Action Test";
|
||||
begin
|
||||
PermissionsMock.Start();
|
||||
PermissionsMock.SetExactPermissionSet('LIMITED USER');
|
||||
|
||||
asserterror SetupAction.Run();
|
||||
Assert.ExpectedError('permission');
|
||||
|
||||
PermissionsMock.Stop();
|
||||
end;
|
||||
|
||||
var
|
||||
Assert: Codeunit "Library Assert";
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: testing
|
||||
keywords: [testpermissions, restrictive, disabled, permissions-mock, lower-permissions, super, permission-test]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Permission tests must actually lower the execution context
|
||||
|
||||
## Description
|
||||
|
||||
`TestPermissions` describes how a test runner should establish the permission context; the enum value does not itself assign the business permission set being tested. `Restrictive` is the default and starts from D365 Full Access, requiring the test to lower permissions. `Disabled` leaves the test running as `SUPER`. A test that expects access to be denied while still running with either broad context can pass or fail for the wrong reason and never exercise the intended boundary.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `TestPermissions::Restrictive` for a permission-sensitive test and lower the current test user with the test framework's `"Permissions Mock"` or `"Library - Lower Permissions"` before invoking the protected operation. Assign the exact permission set the scenario claims to test and restore or stop the mock afterward. Use `Disabled` only for suites that do not assert permission behavior.
|
||||
|
||||
See sample: `permission-tests-must-lower-the-execution-context.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Setting `TestPermissions = Disabled` or leaving the effective D365 Full Access context in place while asserting that a limited user is denied, or adding a `[TestPermissions(...)]` attribute without any runner/test-library code that applies the intended permission set.
|
||||
|
||||
See sample: `permission-tests-must-lower-the-execution-context.bad.al`.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
codeunit 50452 "Isolated Test Runner Bad"
|
||||
{
|
||||
Subtype = TestRunner;
|
||||
TestIsolation = Disabled;
|
||||
}
|
||||
|
||||
codeunit 50453 "Committed Write Test Bad"
|
||||
{
|
||||
Subtype = Test;
|
||||
|
||||
[Test]
|
||||
[TransactionModel(TransactionModel::AutoCommit)]
|
||||
procedure TestCommittedWrite()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.Init();
|
||||
Customer."No." := 'PERSISTS';
|
||||
Customer.Insert();
|
||||
Commit();
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
codeunit 50450 "Isolated Test Runner Good"
|
||||
{
|
||||
Subtype = TestRunner;
|
||||
TestIsolation = Codeunit;
|
||||
}
|
||||
|
||||
codeunit 50451 "Committed Write Test Good"
|
||||
{
|
||||
Subtype = Test;
|
||||
|
||||
[Test]
|
||||
[TransactionModel(TransactionModel::AutoCommit)]
|
||||
procedure TestCommittedWrite()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.Init();
|
||||
Customer."No." := 'ISOLATED';
|
||||
Customer.Insert();
|
||||
Commit();
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: testing
|
||||
keywords: [testisolation, testrunner, autocommit, commit, rollback, test-order, database-state]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Configure TestIsolation on the test runner
|
||||
|
||||
## Description
|
||||
|
||||
`TestIsolation` is a property of a `Subtype = TestRunner` codeunit, not of the test codeunit being executed. Its default is `Disabled`. `Codeunit` rolls back database changes after each test codeunit and `Function` after each test method, including changes that the code under test explicitly committed. Without runner isolation, an `AutoCommit` test can leave data behind and make later tests order-dependent.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Run independent suites with `TestIsolation = Codeunit` or `Function`, choosing the narrowest boundary the runner supports. Pair this with the appropriate method-level `TransactionModel`: `AutoCommit` permits code under test to commit, while runner isolation still restores the database afterward. Keep isolation disabled only for an intentionally shared-state suite whose ordering and cleanup are explicit.
|
||||
|
||||
See sample: `testisolation-belongs-on-the-test-runner.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An `AutoCommit` test exercises committed writes under a test runner that omits `TestIsolation` or sets it to `Disabled`, then assumes the database is restored automatically. This article owns runner-level rollback; `transactionmodel-attribute-governs-test-transactions.md` separately owns the method attribute.
|
||||
|
||||
See sample: `testisolation-belongs-on-the-test-runner.bad.al`.
|
||||
|
|
@ -1,5 +1,5 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
bc-version: [15..]
|
||||
domain: ui
|
||||
keywords: [enqueuebackgroundtask, async-calculation, child-session, factbox, cue-tile, onaftergetcurrrecord, responsive-page, read-only]
|
||||
technologies: [al]
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ codeunit 50206 "Upgrade Graceful"
|
|||
begin
|
||||
if not Customer.Get(CustomerNo) then begin
|
||||
Session.LogMessage(
|
||||
'0000ABC',
|
||||
'UPG0001',
|
||||
'Customer not found during upgrade',
|
||||
Verbosity::Warning,
|
||||
DataClassification::SystemMetadata,
|
||||
|
|
|
|||
|
|
@ -37,15 +37,19 @@ Discard files that are not applicable. Retain conditionally applicable files (an
|
|||
|
||||
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed files and AL object types — especially `app.json`, `AppSourceCop.json`, new objects, and table/page/report extensions that add fields, keys, controls, or actions to base objects.
|
||||
- The changed files and AL object types — especially `app.json`, `AppSourceCop.json`, namespace declarations, permission-set objects, new objects, and table/page/report extensions that add fields, keys, controls, or actions to base objects.
|
||||
- The changed object and member names, weighted toward prefix/suffix consistency with `mandatoryAffixes` or `mandatoryPrefix`, plus AppSource-facing help metadata.
|
||||
- Tokens extracted from the diff that relate to AppSource (`AppSourceCop`, `mandatoryAffixes`, `mandatoryPrefix`, `AS0011`, `prefix`, `suffix`, `tableextension`, `pageextension`, `reportextension`, `field`, `key`, `control`, `action`, `app.json`, `help`, `ContextSensitiveHelpPage`, `Copilot`, `https`).
|
||||
- Tokens extracted from the diff that relate to AppSource (`AppSourceCop`, `mandatoryAffixes`, `mandatoryPrefix`, `AS0011`, `prefix`, `suffix`, `namespace`, `using`, `permissionset`, `Assignable`, `Permissions`, `SUPER`, `tableextension`, `pageextension`, `reportextension`, `field`, `key`, `control`, `action`, `app.json`, `help`, `ContextSensitiveHelpPage`, `Copilot`, `https`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no AppSource-related source or metadata changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
|
||||
|
||||
The following targeted checks cover every current `appsource` article across the Microsoft and community layers. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action.
|
||||
|
||||
- A new or renamed object lacks the reserved prefix/suffix, or a tableextension/pageextension/reportextension adds an unaffixed field, key, control, or action to a base object despite `mandatoryAffixes`/`mandatoryPrefix` and AS0011 — `object-affixes-prevent-collisions`.
|
||||
- Select exactly one naming-collision owner. When no namespace declaration is present, a new/renamed object lacks the reserved prefix/suffix, or an extension object adds an unaffixed member to a base object — `object-affixes-prevent-collisions`.
|
||||
- For BC23 or later, use `two-level-namespace-replaces-object-affix-not-extension-member-affix` instead when the changed source actually declares or changes a namespace and relies on it as the owned-object affix alternative, but has fewer than two levels or incorrectly applies that exception to members on another publisher's object. Never worklist this article for an unaffixed source file with no namespace declaration.
|
||||
- The app has no assignable permission set covering its setup and usage paths, omits visible object/tabledata grants, or requires `SUPER` for normal operation — `permission-sets-cover-setup-and-usage-without-super`. Require repository-level app context; one isolated permission-set object cannot prove complete coverage.
|
||||
|
||||
Before emitting an affix finding, compare every owned object name and every member added to another publisher's object against the configured `mandatoryAffixes`/`mandatoryPrefix`. A matching prefix or suffix is compliant. Do not flag an `ABC`-prefixed object or an `ABC`-suffixed extension member when `ABC` is the configured affix.
|
||||
- For BC v27 or later, `app.json` adds or changes the `help` URL to a path deeper than two levels, or a changed Copilot/context-sensitive help arrangement would ground the app under an overly broad truncated parent — `keep-copilot-help-url-to-two-path-levels`.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
|
@ -58,7 +62,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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 change violates an AppSource submission requirement; otherwise the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -39,10 +39,22 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially codeunits, tables, and table extensions that expose procedures, fields, or events to other apps, and any member whose access is being widened.
|
||||
- The changed procedures, fields, and triggers, weighted toward non-`local` procedures, published table fields, event publishers, and any member whose signature, access modifier, or obsolete state is being altered.
|
||||
- Tokens extracted from the diff that relate to API stability and deprecation (`signature`, `parameter`, `return`, `var`, `Obsolete`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `Pending`, `Removed`, `CLEAN`, `SecretText`, `token`, `internal`, `local`, `public`, `protected`, `Scope`).
|
||||
- Tokens extracted from the diff that relate to API stability and deprecation (`signature`, `parameter`, `return`, `var`, `Obsolete`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `Pending`, `Removed`, `CLEAN`, `SecretText`, `token`, `internal`, `local`, `public`, `protected`, `Scope`, `namespace`, `using`, `AS0007`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
The following targeted checks cover every current `breaking-changes` article:
|
||||
|
||||
- A helper or object changes between `local`, `internal`, `protected`, or public access, or a new implementation detail is exposed without a supported-API reason — `choose-access-modifiers-deliberately`.
|
||||
- A public member is removed or replaced without first going through the `[Obsolete]` lifecycle — `deprecate-public-members-with-the-obsolete-lifecycle`.
|
||||
- A published procedure changes parameter count/order/type/name, `var`, return type, or array shape instead of preserving the old signature and adding an overload — `do-not-change-published-procedure-signatures`.
|
||||
- A public procedure/event/interface exposes a credential or other sensitive value through `Text` or an externally callable contract — `do-not-expose-sensitive-data-through-public-api`.
|
||||
- Code already marked obsolete is expanded with new behavior instead of routing new callers to its replacement — `do-not-modify-code-already-marked-obsolete`.
|
||||
- A shipped table field is deleted, renamed, renumbered, or replaced without retaining the original field as `ObsoleteState = Pending` and migrating its data — `obsolete-table-fields-instead-of-deleting-them`. This owns AS0005 field-name changes; do not substitute the namespace article.
|
||||
- A published object's namespace changes between the base and changed source while its identity otherwise remains — `namespace-is-part-of-published-object-identity`. Do not apply it to a new, unshipped object or to an ordinary object-name change with no namespace change.
|
||||
|
||||
For `obsolete-table-fields-instead-of-deleting-them`, compare the baseline ID and name before emitting. When the original field remains under the same ID and name with `ObsoleteState = Pending`, and the replacement uses a new ID, the change follows the rule and must not be flagged.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable breaking-changes knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable breaking-changes knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
|
@ -53,7 +65,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ sub-skills:
|
|||
- microsoft/skills/review/al-web-services-review.md
|
||||
- microsoft/skills/review/al-testing-review.md
|
||||
- microsoft/skills/review/al-data-modeling-review.md
|
||||
- microsoft/skills/review/al-query-review.md
|
||||
- microsoft/skills/review/al-appsource-review.md
|
||||
- microsoft/skills/review/al-telemetry-review.md
|
||||
---
|
||||
|
|
@ -64,10 +65,12 @@ The worklist is the list of sub-skills judged relevant by the previous step. Eve
|
|||
|
||||
The Action step is a sequence of **discrete iterations**, not one combined generation. The contract requires the super-skill to invoke each sub-skill in turn and then perform a self-review pass. Concretely this means:
|
||||
|
||||
- **Isolate leaf invocations when the host supports it.** For fast/small models, each sub-skill SHOULD run in a fresh model call or child context containing only the task input, READ/DO contracts, the leaf instructions, a domain-filtered slice of the current knowledge index, and articles that leaf worklists. Preserve each index row's exact `path`; the leaf must copy references from that slice. The coordinator then collects the resulting JSON. This is the preferred fast-model profile: it bounds context, prevents later leaves from being skipped as attention is exhausted, and removes any reason to synthesize article paths.
|
||||
- Treat each sub-skill in the worklist as its own pass: read the sub-skill's instructions, apply its Source → Relevance → Worklist → Action steps to the orchestrator-supplied inputs, and produce that sub-skill's complete findings-report before moving on.
|
||||
- Do not collapse multiple sub-skills into one shared reasoning step. Each sub-skill has a distinct knowledge subset and a distinct evaluation procedure; sharing one rolled-up scan dilutes per-skill attention and causes leaves to silently underreport (this has been observed in production: leaf skills returned empty `findings[]` while their standalone runs against the same diff produced multiple matches).
|
||||
- The agent self-review pass is its own final iteration. Begin it only after every sub-skill in the worklist has completed and its sub-result is recorded.
|
||||
- Sub-skills are independent: re-walking the diff once per sub-skill is correct and expected. The output schema accommodates this — `sub-results` carries one entry per sub-skill, each a complete findings-report.
|
||||
- When isolated calls are unavailable and the current model cannot finish every leaf within its budget, return `partial` with completed `sub-results` and name the first unevaluated sub-skill in `outcome-reason`. Never silently mark the remaining leaves clean.
|
||||
|
||||
### Roll up sub-skill findings
|
||||
|
||||
|
|
@ -76,7 +79,8 @@ For each sub-skill in the worklist, executed one at a time per the discipline ab
|
|||
1. Invoke the sub-skill with the orchestrator's inputs, passing only the subset each sub-skill declares in its `inputs`.
|
||||
2. Capture the sub-skill's complete findings-report verbatim and append it to `sub-results`.
|
||||
3. If the sub-skill's `outcome` is `failed`, stop here for this sub-skill: its findings are not reliable per the DO contract and MUST NOT be copied into the super-skill's top-level `findings[]` or counted in `summary.counts`.
|
||||
4. Otherwise, append each entry from the sub-skill's `findings[]` to the super-skill's top-level `findings[]`, setting `from-sub-skill` to the sub-skill's `skill.id` and preserving each finding's optional `domain` field verbatim, including its absence. For non-citation findings (those whose `id` is a skill-defined slug rather than a reference path), prefix `id` with `<from-sub-skill>:` to prevent collisions across sub-skills. Other finding fields are preserved.
|
||||
4. Otherwise, compare each entry from the sub-skill's `findings[]` with findings already rolled up. Two findings are duplicates when they point to the same file and overlapping line/range and prescribe materially the same correction, even when their knowledge-file IDs differ. Merge duplicates instead of appending both: keep the more specific domain owner, preserve that finding's optional `domain` field verbatim (including its absence), use its reference as `references[0]` and therefore as `id`, append the other references as supporting references, keep the highest severity and confidence justified by either report, and preserve one self-contained message. Article and leaf ownership notes decide specificity; do not choose by execution order.
|
||||
5. Append each non-duplicate finding, setting `from-sub-skill` to the sub-skill's `skill.id` and preserving its optional `domain` field verbatim, including its absence. For non-citation findings (those whose `id` is a skill-defined slug rather than a reference path), prefix `id` with `<from-sub-skill>:` to prevent collisions across sub-skills. Other finding fields are preserved.
|
||||
|
||||
### Agent self-review pass
|
||||
|
||||
|
|
@ -118,6 +122,8 @@ Aggregate `summary.counts` and `summary.coverage` as the sums across invoked sub
|
|||
|
||||
Derive `outcome` using the DO rollup rules. `outcome-reason` is populated for `partial` and `failed` and SHOULD summarize per-sub-skill state, for example: *"al-security-review failed (tool timeout); al-performance-review completed."*
|
||||
|
||||
Before emitting the rollup, apply DO's reference-integrity gate to every nested and top-level finding. Every knowledge-backed ID/reference path must exist in the live checkout, must have been opened by the producing leaf, and must be copied verbatim rather than synthesized. Treat a sub-result containing an unverifiable citation as failed and exclude its findings from the top-level rollup.
|
||||
|
||||
## Output
|
||||
|
||||
Output conforms to the DO output contract, extended with `sub-results` and `skipped-sub-skills`. A populated example — both leaves ran, each produced findings:
|
||||
|
|
|
|||
|
|
@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially `* Setup` singleton tables and Card pages, custom master tables, tableextensions that add master-data fields, and document or journal lines that reference a master.
|
||||
- The changed fields, keys, triggers, and procedures, weighted toward `Primary Key`, `No.`, `No. Series`, `Blocked`, `Last Date Modified`, `OnInsert`, `OnModify`, `OnRename`, reference-field `OnValidate`, and posting validation.
|
||||
- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`).
|
||||
- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `TableRelation`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no data-modeling changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
|
||||
|
||||
|
|
@ -50,6 +50,8 @@ The following targeted checks cover every current `data-modeling` article. Treat
|
|||
- BC v22 or later code introduces or retains `NoSeriesManagement`, `InitSeries`, `SelectSeries`, or `SetSeries`, or number assignment/manual-entry checks do not use codeunit `"No. Series"` methods such as `GetNextNo`, `IsManual`, or `TestManual` — `use-no-series-codeunit-not-noseriesmanagement`.
|
||||
- A master gains or changes `Blocked`, or a document line, journal line, reference-field `OnValidate`, or posting routine uses that master without `TestField(Blocked, false)` at the point of use; also cue when the check is placed only in the master's own triggers — `check-blocked-in-referencing-code-not-in-master`.
|
||||
- A master table adds or changes `Last Date Modified`, `OnModify`, or `OnRename`, but the non-editable field is not assigned `Today()` in both triggers — `set-last-date-modified-in-onmodify-and-onrename`.
|
||||
- A `tableextension` appends a conditional `TableRelation` as if it overrides an earlier unconditional relation, or relation branches are otherwise designed without accounting for additive top-down evaluation — `table-relation-extensions-are-additive-and-top-down`.
|
||||
- A `Media` or `MediaSet` field is assigned directly between different table types or different field IDs instead of registering each shared item with `MediaSet.Insert` — `share-mediaset-items-with-insert-not-field-assignment`.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
|
|
@ -61,7 +63,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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 model can create ambiguous setup state, incompatible business identifiers, or silently stale synchronization data; otherwise the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -44,6 +44,15 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
The following targeted checks cover every current `error-handling` article:
|
||||
|
||||
- `[ErrorBehavior(ErrorBehavior::Collect)]`, `ErrorInfo.Collectible`, `HasCollectedErrors`, `GetCollectedErrors`, or `ClearCollectedErrors` is added or changed, especially when errors are collected without later surfacing/clearing them — `collect-validation-errors-with-errorbehavior`.
|
||||
- Developer-only invariant text is raised with default client visibility, or a user-actionable validation is hidden as `ErrorType::Internal` — `errortype-internal-vs-client-for-diagnostics`.
|
||||
- `FieldError` receives a complete capitalized sentence, repeats the field caption/value, or ends the predicate with punctuation — `fielderror-default-message-logic`.
|
||||
- An unguarded `FieldError` is used as though it performed a comparison, or `TestField` is forced onto a complex rule needing a tailored predicate — `fielderror-vs-testfield`.
|
||||
- A resolved call target is marked `[TryFunction]` but the call is a standalone statement whose Boolean result is ignored — `ignored-tryfunction-return-disables-try-semantics`. This call-site rule supersedes the performance TryFunction article unless writes and rollback expectations are also visible.
|
||||
- A plain `Error` represents a known actionable correction that can be expressed through `ErrorInfo` actions/navigation, or an `ErrorInfo` omits the context needed for that action — `prefer-errorinfo-for-actionable-errors`.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable error-handling knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable error-handling knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
|
@ -54,7 +63,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -72,7 +72,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -55,13 +55,15 @@ The following targeted checks map diff signals to specific `interfaces` articles
|
|||
- A method added directly to an interface that exists in the baseline, instead of adding a BC25+ interface that `extends` it or a versioned sibling for older targets — `extend-published-interfaces-dont-edit-them`.
|
||||
- A declared enum value with no `Implementation` and no enum-level `DefaultImplementation` — `set-defaultimplementation-on-enum`.
|
||||
|
||||
For `set-defaultimplementation-on-enum`, inspect the complete containing enum before emitting. An enum-level `DefaultImplementation = <Interface> = <Codeunit>;` conclusively covers every declared value that omits its own `Implementation`; do not flag such a value and do not replace the intentional fallback with a per-value mapping.
|
||||
|
||||
## Action
|
||||
|
||||
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`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -39,15 +39,18 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially tables, pages with SourceTable bindings, reports, queries, and codeunits performing record iteration.
|
||||
- The changed procedures and triggers, weighted toward those that perform loops, Find/FindSet/FindFirst calls, CalcFields, SetAutoCalcFields, CalcSums, FlowField access, Commit calls, checkpoint helpers, record copying, RecordRef conversion, Modify/Delete calls, or cross-table navigation.
|
||||
- Tokens extracted from the diff that relate to data access and hot-path costs (`SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `ReadIsolation`, `LockTable`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`).
|
||||
- Tokens extracted from the diff that relate to data access and hot-path costs (`SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `ReadIsolation`, `LockTable`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`, `FlowField`, `Visible`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
Apply these targeted cues even when simple token overlap would rank the article below the worklist cutoff:
|
||||
|
||||
- Worklist `use-setautocalcfields-for-per-row-flowfields.md` when a record loop calls `CalcFields`, or when every row reads the same FlowField for a comparison, branch, or per-record action. Worklist `calcsums-instead-of-calcfields-in-loop.md` instead when the loop only accumulates one set total.
|
||||
- Worklist `hidden-flowfields-still-calculate-before-bc26-opt-in.md` when a page control directly sources a FlowField and sets `Visible = false` or a visibility expression. Suppress it when the target is known to have BC26's **Calculate only visible FlowFields** feature enabled, or when the FlowField is cheap and intentionally preloaded.
|
||||
- Worklist `avoid-commit-inside-loops.md` only when `Commit()` is inside a record-iteration body or a helper invoked once per row. Do not match one `Commit()` after a bounded checkpoint helper returns, a `Commit()` outside iteration, or comments and documentation that merely mention commits.
|
||||
- Worklist `avoid-cloning-records-before-modify-delete-in-loops.md` when an iteration calls `Copy` or `RecordRef.GetTable` before `Modify`/`Delete`, or passes the iterated record without `var` to a helper that writes that record. Do not worklist it from `Modify`, `Delete`, or `RecordRef` alone; exclude a direct write on the iterator, a read-only copy, a temporary record, a different target table, and a `RecordRef` opened and iterated directly.
|
||||
- Worklist `use-tryfunction-for-error-catching-not-rollback.md` only when writes occur inside a try method and the code or surrounding flow expects an error to roll them back. A bare try-method call whose Boolean result is ignored belongs exclusively to `error-handling/ignored-tryfunction-return-disables-try-semantics.md`; do not worklist the performance article from that call shape alone.
|
||||
- For `LockTable` in a pure read helper, select exactly one owner. Use `do-not-locktable-in-read-only-procedure.md` when the helper needs no stronger isolation and should remove the lock. Use `prefer-readisolation-over-locktable-for-reads.md` instead when the code explicitly requires committed-read semantics and `ReadIsolation` is the replacement. Never emit both findings for the same call.
|
||||
|
||||
These targeted inclusions and exclusions override generic token overlap. Do not retain an excluded article solely because the diff contains one of its keywords.
|
||||
|
||||
|
|
@ -61,7 +64,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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 (for example, documented query timeouts or transaction size limits). When the file does not make such a claim, the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -45,6 +45,11 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Apply the topic-specific gates above after this overlap check; in particular, bare `Message` and `DataClassification` tokens cannot admit ErrorInfo guidance. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
Apply API ownership before fuzzy ranking:
|
||||
|
||||
- A `Session.LogMessage` message built with `StrSubstNo` or concatenation from customer, employee, filename, document, or other identifying values belongs to `no-pii-in-telemetry-message-string.md`.
|
||||
- `avoid-strsubstno-prebuild-before-error.md` applies only when `StrSubstNo` or concatenation supplies the first argument to `Error(...)`. Never apply it to `Session.LogMessage`, `FeatureTelemetry`, or another telemetry API.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable privacy knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable privacy knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
|
@ -55,7 +60,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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 (for example, documented telemetry-classification rules or GDPR-adjacent data-handling requirements). When the file does not make such a claim, the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
56
microsoft/skills/review/al-query-review.md
Normal file
56
microsoft/skills/review/al-query-review.md
Normal file
|
|
@ -0,0 +1,56 @@
|
|||
---
|
||||
kind: action-skill
|
||||
id: al-query-review
|
||||
version: 1
|
||||
title: AL Query review
|
||||
description: Reviews AL Query objects and Query instance usage against BCQuality guidance.
|
||||
inputs: [pr-diff, file-path]
|
||||
outputs: [findings-report]
|
||||
bc-version: [all]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# AL Query review
|
||||
|
||||
Reviews AL source changes against the `query` knowledge domain in BCQuality. This is a leaf action skill composed by `al-code-review`.
|
||||
|
||||
## Source
|
||||
|
||||
Read `knowledge-index.json` once and take entries whose `domain` is `query` across enabled layers. Open an article body only after it enters the Worklist. If the index is unavailable, discover `*/knowledge/query/*.md` by path.
|
||||
|
||||
## Relevance
|
||||
|
||||
Apply READ's frontmatter matching rules against the task context. Use the target version from `app.json` when available and `[al]` for technologies. Retain conditionally applicable files only when configured; cap resulting confidence at `medium` and name every unknown dimension in the finding message.
|
||||
|
||||
Return `not-applicable` when the input contains no Query object declaration and no Query variable method call.
|
||||
|
||||
## Worklist
|
||||
|
||||
Match relevant entries against changed `query` objects, variables typed as `Query`, and the tokens `QueryType`, `dataitem`, `column`, `DataItemLink`, `SqlJoinType`, `SetFilter`, `SetRange`, `Open`, `Read`, `Close`, and `Clear`.
|
||||
|
||||
The following targeted checks cover every current `query` article:
|
||||
|
||||
- `SetFilter` or `SetRange` occurs after `Open()` without a new `Open()` before the next `Read()` — `set-query-filters-before-open`.
|
||||
- An already-open query is opened again as if that advanced the cursor, or a query variable is reused for an independent operation without `Clear` even though old filters must not carry over — `reopening-query-resets-cursor-but-keeps-filters`.
|
||||
|
||||
Resolve layer conflicts per READ. When no query knowledge exists, emit `no-knowledge`; when knowledge exists but no article matches the changed Query usage, emit `completed` with no findings.
|
||||
|
||||
## Action
|
||||
|
||||
Evaluate every worklist article against the diff's Query call order and surrounding control flow.
|
||||
|
||||
- Emit `major` for an unambiguous Anti Pattern that can close the dataset, restart processing, or retain an unintended filter.
|
||||
- Emit `minor` when code contradicts a Best Practice but the resulting behavior depends on unseen control flow.
|
||||
- Do not emit applicability-only information. A Query article produces a finding only when the changed code violates its normative guidance.
|
||||
|
||||
Set confidence to `high` for a locally visible call sequence and `medium` when aliases, helper calls, or missing context obscure the sequence. Domain-scoped agent findings follow DO's precision bar and remain capped at `minor`/`medium`.
|
||||
|
||||
Provide `suggested-code` only when moving a filter before `Open()` or adding `Clear` is a complete, local, unambiguous replacement. Otherwise set `suggested-code-omission-reason`.
|
||||
|
||||
Outcome selection follows DO: `completed`, `no-knowledge`, `not-applicable`, `partial`, or `failed`.
|
||||
|
||||
## Output
|
||||
|
||||
Output conforms to the DO findings-report contract. Every finding this skill emits MUST set `findings[].domain` to `"Query"`.
|
||||
|
|
@ -39,10 +39,17 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially permission sets, codeunits handling authentication or authorization, objects touching `Isolated Storage`, `OAuth2` flows, web service endpoints, API pages, event publishers, and RecordRef helpers.
|
||||
- The changed procedures and triggers, weighted toward those that call `HttpClient`, validate or compose URLs, write to telemetry, read or write secrets, unwrap SecretText, manipulate record-level security, expose var Boolean guard parameters, or bypass the permission model (for example, `RecordRef.Open`, `Record.WritePermission`, direct table access from a non-owning app).
|
||||
- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`).
|
||||
- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
Always worklist `internal-access-is-not-a-security-boundary.md` when changed comments or code rely on `Access = Internal` or `internalsVisibleTo` to protect a sensitive operation, or an internal `OnRun` codeunit performs privileged work without an independent authorization boundary. Do not flag `internal` used only to keep implementation details out of the supported API.
|
||||
|
||||
For secret values, select the most specific sink owner:
|
||||
|
||||
- When a `Text`/`Code` credential is declared, passed, returned, or unwrapped without a visible HTTP URI/header/body sink, use `secrettext-for-credentials.md`.
|
||||
- When that value is interpolated into a URI, authorization header, or HTTP body and sent through `HttpClient`, use `secrettext-with-httpclient.md` as the primary finding. It supersedes the generic credential-type article at that location; keep the latter only as a supporting reference when useful.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable security knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable security knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
|
@ -53,7 +60,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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 (for example, documented secret-handling rules, permission-model invariants, or data-protection requirements). When the file does not make such a claim, the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -45,6 +45,13 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object or declaration. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
Do not worklist `temporary-variable-temp-prefix.md` for an event publisher parameter. `events/prefix-temporary-record-event-parameters-with-temp.md` is the exclusive owner of that shape.
|
||||
|
||||
Apply these high-signal mappings before fuzzy topic ranking:
|
||||
|
||||
- A `Label` or `TextConst` contains multiple or ambiguous placeholders but has no `Comment`, or its Comment does not explain every placeholder — `label-comment-explains-placeholders.md`. A single placeholder whose meaning is explicit in the text, such as `Customer %1`, is allowed without a Comment and must not be flagged.
|
||||
- `function-call-parentheses-required.md` applies only to a zero-argument invocation written without `()`. Never worklist it from an invocation that already has parentheses or supplies arguments, including `Error(Label, Arg1, Arg2)`.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable style knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable style knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
|
|
|||
|
|
@ -39,10 +39,21 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL objects and procedures — especially telemetry wrapper codeunits, feature lifecycle instrumentation, error logging, integration diagnostics, and background/session processing.
|
||||
- Calls to `Session.LogMessage`, `Session.LogError`, or `FeatureTelemetry` methods, weighted toward the event ID, verbosity, data classification, custom dimensions, and `TelemetryScope` arguments.
|
||||
- Tokens extracted from the diff that relate to telemetry (`Session.LogMessage`, `Session.LogError`, `FeatureTelemetry`, `TelemetryScope`, `ExtensionPublisher`, `All`, `Verbosity`, `DataClassification`, `CustomDimensions`, `Application Insights`, `LogUsage`, `LogError`, `LogUptake`, `Feature Uptake Status`).
|
||||
- Telemetry infrastructure codeunits that implement `"Telemetry Logger"` or subscribe to `"Telemetry Loggers".OnRegisterTelemetryLogger`.
|
||||
- Tokens extracted from the diff that relate to telemetry (`Session.LogMessage`, `Session.LogError`, `FeatureTelemetry`, `TelemetryScope`, `ExtensionPublisher`, `All`, `Verbosity`, `Critical`, `Error`, `Warning`, `Normal`, `Verbose`, `DataClassification`, `CustomDimensions`, `Application Insights`, `Telemetry Logger`, `Telemetry Loggers`, `OnRegisterTelemetryLogger`, `LogUsage`, `LogError`, `LogUptake`, `Feature Uptake Status`, `Discovered`, `Set up`, `Used`, `Undiscovered`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no telemetry-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
|
||||
|
||||
The following targeted checks cover every current `telemetry` article. Treat each as a candidate-selection cue:
|
||||
|
||||
- A `Session.LogMessage` event ID is empty, generated dynamically, reused for different events, changed on an existing event, or uses a placeholder such as `0000`, `1234`, `TODO`, or `XX0000` — `telemetry-event-id-stable-unique`.
|
||||
- `TelemetryScope::All` is used for a clearly publisher-only implementation diagnostic, or `ExtensionPublisher` hides a clearly customer-actionable failure from environment telemetry — `choose-telemetry-scope-by-audience`. Do not infer the audience when the message and surrounding branch are ambiguous.
|
||||
- An explicit failure branch logs through `Session.LogMessage` with `Verbosity::Normal` or `Verbose`, or a non-error event is inflated to `Error`/`Critical` — `match-verbosity-to-signal-severity`.
|
||||
- A new feature's visible uptake calls skip `Discovered` or `Set up`, jump directly to `Used`, or use inconsistent feature-name literals across states — `feature-uptake-transitions-in-order`. Require repository-level lifecycle evidence; one isolated call is not proof.
|
||||
- `FeatureTelemetry.LogUsage` runs before success is known or on a failure path — `feature-usage-only-after-success`. `LogUptake(...Used)` records an attempt and is not this anti-pattern.
|
||||
- A complete app or app family uses `FeatureTelemetry` without any registered `"Telemetry Logger"`, or registers more than one implementation for the same publisher — `register-one-telemetry-logger-per-publisher`. Absence requires repository/app-family context.
|
||||
- A custom-dimension key contains spaces or non-PascalCase naming, or an existing event ID changes/removes a shipped key — `keep-custom-dimension-schema-stable`. Treat naming alone as advisory; the schema change is the compatibility defect.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable telemetry knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable telemetry knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
|
@ -53,7 +64,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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; otherwise the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -38,14 +38,16 @@ Discard files that are not applicable. Retain conditionally applicable files (an
|
|||
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed AL object names and types — especially codeunits with `Subtype = Test`, test runner codeunits with `TestIsolation`, test libraries, and codeunits that define UI handlers.
|
||||
- The changed methods and attributes, weighted toward `[Test]`, `[TransactionModel(...)]`, `[HandlerFunctions(...)]`, handler attributes, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, fixture initialization, and test-library calls.
|
||||
- Tokens extracted from the diff that relate to testing (`Subtype = Test`, `TestIsolation`, `TransactionModel`, `AutoRollback`, `AutoCommit`, `Commit`, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, `HandlerFunctions`, `ConfirmHandler`, `MessageHandler`, `StrMenuHandler`, `ModalPageHandler`, `Enqueue`, `Dequeue`, `AssertEmpty`, `Library Assert`, `LibraryVariableStorage`, `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, `Init`, `Insert`).
|
||||
- The changed methods and attributes, weighted toward `[Test]`, `[TransactionModel(...)]`, `[TestPermissions(...)]`, `[HandlerFunctions(...)]`, handler attributes, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, fixture initialization, and test-library calls.
|
||||
- Tokens extracted from the diff that relate to testing (`Subtype = Test`, `Subtype = TestRunner`, `TestIsolation`, `TestPermissions`, `Restrictive`, `NonRestrictive`, `Disabled`, `Permissions Mock`, `Library - Lower Permissions`, `TransactionModel`, `AutoRollback`, `AutoCommit`, `Commit`, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, `HandlerFunctions`, `ConfirmHandler`, `MessageHandler`, `StrMenuHandler`, `ModalPageHandler`, `Enqueue`, `Dequeue`, `AssertEmpty`, `Library Assert`, `LibraryVariableStorage`, `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, `Init`, `Insert`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no testing-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
|
||||
|
||||
The following targeted checks cover every current `testing` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action.
|
||||
|
||||
- A method in a `Subtype = Test` codeunit adds or changes `[TransactionModel(...)]`, exercises code that calls `Commit`, defaults broadly to `AutoCommit`, or uses `AutoCommit` (or exercises a path that calls `Commit`) without a `TestIsolation`-enabled runner — `transactionmodel-attribute-governs-test-transactions`. Do not worklist this article solely because an ordinary `AutoRollback` or read-only test has no `TestIsolation` runner.
|
||||
- A method in a `Subtype = Test` codeunit adds or changes `[TransactionModel(...)]`, exercises code that calls `Commit` under `AutoRollback`, defaults broadly to `AutoCommit`, or chooses `None` for a writing test — `transactionmodel-attribute-governs-test-transactions`.
|
||||
- An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it.
|
||||
- A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`.
|
||||
- Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`.
|
||||
- `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`.
|
||||
- A test path raises UI, `[HandlerFunctions(...)]` does not exactly match the invoked handlers, a handler hardcodes replies instead of using enqueue/dequeue expectations, or `LibraryVariableStorage.Clear`/`AssertEmpty` is missing — `ui-handlers-in-tests`.
|
||||
|
|
@ -60,7 +62,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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 test can pass while verifying the wrong behavior or can leave committed data that contaminates later tests; otherwise the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
|
|
@ -56,7 +56,7 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
|
||||
- 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).
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
|
|||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Add table
Add a link
Reference in a new issue