diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 6aa3f8a..1aa47c6 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -9,7 +9,10 @@ "name": "bcquality", "source": "./", "description": "Business Central AL quality knowledge base and review skills, packaged as an installable plugin. Ships the entire BCQuality tree (skills, knowledge, tools) so the Entry routing protocol runs against the installed clone.", - "version": "0.1.0" + "version": "0.1.0", + "skills": [ + "./skills/bcquality-al-review/" + ] } ] } diff --git a/.github/scripts/validate_frontmatter.py b/.github/scripts/validate_frontmatter.py index 682a801..dd3ef4e 100644 --- a/.github/scripts/validate_frontmatter.py +++ b/.github/scripts/validate_frontmatter.py @@ -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 .. with .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 ---------------------------------------------------------- diff --git a/.github/workflows/review-fixtures.yml b/.github/workflows/review-fixtures.yml new file mode 100644 index 0000000..fff9cd0 --- /dev/null +++ b/.github/workflows/review-fixtures.yml @@ -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" diff --git a/README.md b/README.md index 6abf98a..b939c6f 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,8 @@ Poor fit: "Use HTTPS instead of HTTP." "Don't hardcode secrets." "Keep transacti The practical consequence: when a code-review agent flags something it shouldn't have, or misses something it should have caught, the remedy is a new knowledge file. When it already behaves correctly on a topic, no file is needed. +A file that *prevents* a false positive — documenting why a pattern is legitimate so the agent stops flagging it — is as valid as one that catches a defect: negative clarifications are first-class knowledge files. What never belongs is a BC fact hard-coded into a skill. Skills are finders and appliers; knowledge files are what the agent knows. See [`skills/do.md`](skills/do.md) and [`skills/write.md`](skills/write.md). + ## What's in this repo BCQuality contains **knowledge** and **skills**. It does not contain agents. Agents that consume BCQuality ship with [AL-Go](https://github.com/microsoft/AL-Go) and other orchestrators. @@ -88,18 +90,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 +115,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 @@ -155,9 +149,12 @@ Contributions are welcome. Before submitting a PR: 1. Read the knowledge file format above — frontmatter and sections are validated by CI. 2. Keep files atomic: one concern per file, under 100 lines. 3. Target your contribution to the right layer — most community contributions go in `/community/knowledge/`. +4. Adding a BC fact — or stopping the agent from flagging a false positive — is a knowledge file, not a skill edit. If a PR changes *what* a review skill flags, the change almost certainly belongs in a knowledge file. See [`skills/write.md`](skills/write.md). CI runs validation on every PR. If your knowledge file has schema violations, missing sections, code blocks, or exceeds 100 lines, the check will fail with a clear error message. +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) diff --git a/agent-consumption.md b/agent-consumption.md index 0d2e5b4..8db80d6 100644 --- a/agent-consumption.md +++ b/agent-consumption.md @@ -21,7 +21,7 @@ flowchart LR E -->|3 dispatch record| A A -->|4 invoke dispatched skill| S[Action skill
e.g. al-code-review] S -->|5 execute| P[Source → Relevance
→ Worklist → Action
reading READ · DO on demand] - P -->|6 emit| R[Findings · References
· Confidence] + P -->|6 emit| R[Findings · Domain labels
· References · Confidence] R -->|7 integrate| O ``` @@ -65,6 +65,7 @@ The output contract is defined in the DO meta-skill so that every action skill - **Outcome** — `completed`, `not-applicable`, `no-knowledge`, `partial`, or `failed`. An orchestrator can distinguish a clean run from a no-op from a failure without guessing. - **Findings** — what the skill observed (severity, message, optional location). +- **Domain** — the producer-owned, human-readable display label on each review finding. - **References** — structured objects (`path` plus optional commit `sha`) pointing to the knowledge files that informed each finding. - **Confidence** — per-finding evidence strength. - **Suppressed** — knowledge files that were discarded by layer precedence or configuration, so reviewers can see what was overridden. @@ -78,12 +79,12 @@ The orchestrator turns findings into PR comments, build gates, or IDE diagnostic BCQuality is an **additive** knowledge layer. The agent surfaces two kinds of findings, both shaped to the same DO output contract: -- **Knowledge-backed findings** carry one or more entries in `references[]` pointing at BCQuality knowledge files. Their `id` is the primary file's repo-relative path. These are produced by leaf sub-skills and rolled up by super-skills. -- **Agent findings** are surfaced by a super-skill from its own self-review pass when no BCQuality knowledge file backs the concern. They are tagged with `from-sub-skill: "agent"`, carry an empty `references: []`, use a slug `id` prefixed `agent:`, and have `confidence` capped at `medium`. Their `message` is self-contained because there is no knowledge-file footer to fall back on. +- **Knowledge-backed findings** carry one or more entries in `references[]` pointing at BCQuality knowledge files. Their `id` is the primary file's repo-relative path. Leaf sub-skills set `domain` to their human-readable display label, and super-skills preserve it verbatim during rollup. +- **Agent findings** carry an empty `references: []`, use a slug `id` prefixed `agent:`, and have `confidence` capped at `medium`. A leaf can emit one strictly within its own domain and uses that leaf's display label. A super-skill can emit a cross-cutting agent finding with `from-sub-skill: "agent"` and `domain: "Agent"`. Their `message` is self-contained because there is no knowledge-file footer to fall back on. -Before a super-skill emits an agent finding, it validates the candidate against the BCQuality knowledge already loaded for the task: a matching file upgrades the candidate to a knowledge-backed finding (and merges or deduplicates against the relevant sub-skill output); a contradicting file suppresses the candidate. Only candidates with no BCQuality coverage become agent findings. +Before a skill emits an agent finding, it validates the candidate against the BCQuality knowledge already loaded for the task: a matching file upgrades the candidate to a knowledge-backed finding (and merges or deduplicates against relevant existing output); a contradicting file suppresses the candidate. Only candidates with no BCQuality coverage become agent findings. -Orchestrators MAY render the two kinds differently — for example, by labelling agent findings or routing them to a separate review domain — and MAY apply independent severity floors. The `from-sub-skill: "agent"` marker is the contract. +Orchestrators MUST tolerate an absent `domain` in reports from older producers. When it is present, treat it as display text rather than an identifier: preserve the full string and its case, whitespace, punctuation, and non-ASCII characters, escaping only for the target rendering format. Do not tokenize it on spaces or use a lowercased or slugified form as the sole metadata or deduplication key, because distinct labels can collapse to the same slug. Retain the exact string, use a lossless encoding, or use a collision-resistant digest instead. Orchestrators MAY render knowledge-backed and agent findings differently and MAY apply independent severity floors; `references: []` and the `agent:` id prefix distinguish agent findings, while `from-sub-skill: "agent"` identifies those emitted by the super-skill itself. ## Why this architecture diff --git a/evaluation/README.md b/evaluation/README.md new file mode 100644 index 0000000..cd25d3d --- /dev/null +++ b/evaluation/README.md @@ -0,0 +1,56 @@ +# AL review evaluation + +The evaluation is convention-driven. For every `microsoft/skills/review/al--review.md` leaf, the harness finds `microsoft/knowledge//`, 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-.json` files provide optional two-case leaf batches; save those as `result-.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. diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json new file mode 100644 index 0000000..2f85d5c --- /dev/null +++ b/evaluation/review-fixtures.json @@ -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" + } + } +} diff --git a/microsoft/knowledge/appsource/object-affixes-prevent-collisions.md b/microsoft/knowledge/appsource/object-affixes-prevent-collisions.md index 49ef80c..a46c0d1 100644 --- a/microsoft/knowledge/appsource/object-affixes-prevent-collisions.md +++ b/microsoft/knowledge/appsource/object-affixes-prevent-collisions.md @@ -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`. diff --git a/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.bad.al b/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.bad.al new file mode 100644 index 0000000..606bef0 --- /dev/null +++ b/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.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; +} diff --git a/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.good.al b/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.good.al new file mode 100644 index 0000000..31b2d3e --- /dev/null +++ b/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.good.al @@ -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; +} diff --git a/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.md b/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.md new file mode 100644 index 0000000..ca89003 --- /dev/null +++ b/microsoft/knowledge/appsource/permission-sets-cover-setup-and-usage-without-super.md @@ -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`. diff --git a/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.bad.al b/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.bad.al new file mode 100644 index 0000000..de2d3ed --- /dev/null +++ b/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.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; + } + } +} diff --git a/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.good.al b/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.good.al new file mode 100644 index 0000000..93fa9c6 --- /dev/null +++ b/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.good.al @@ -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; + } + } +} diff --git a/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.md b/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.md new file mode 100644 index 0000000..509fe6f --- /dev/null +++ b/microsoft/knowledge/appsource/two-level-namespace-replaces-object-affix-not-extension-member-affix.md @@ -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`. diff --git a/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.bad.al b/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.bad.al new file mode 100644 index 0000000..e2a5152 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.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; +} diff --git a/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.good.al b/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.good.al new file mode 100644 index 0000000..ea151a8 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.good.al @@ -0,0 +1,8 @@ +namespace Contoso.Rentals; + +codeunit 50466 "Rental Agreement Mgt." +{ + procedure CreateAgreement() + begin + end; +} diff --git a/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.md b/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.md new file mode 100644 index 0000000..fde0f54 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/namespace-is-part-of-published-object-identity.md @@ -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`. diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al index 1277fc2..d5f51cf 100644 --- a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.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]) { } } } diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al index ed239d2..d0ce8d2 100644 --- a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al @@ -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; diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md index 0d5a289..3ff6474 100644 --- a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md @@ -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`. diff --git a/microsoft/knowledge/breaking-changes/relocating-a-field-to-a-tableextension-is-not-a-deletion.md b/microsoft/knowledge/breaking-changes/relocating-a-field-to-a-tableextension-is-not-a-deletion.md new file mode 100644 index 0000000..d37bfe8 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/relocating-a-field-to-a-tableextension-is-not-a-deletion.md @@ -0,0 +1,18 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [table-field, tableextension, relocation, field-id, obsoletestate, breaking-change, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Relocating a field to a tableextension in the same app is not a deletion + +## Description + +Moving a field out of a base-table definition (or a base-app layer modification of one) into a tableextension that `extends` the same table, within the same app and keeping the same field ID and name, is a relocation — not a deletion or a rename. After the move the field still exists on the table: `Rec."Field Name"` and the field ID resolve exactly as before, so dependent extensions that reference the field continue to compile. Nothing in the field's public contract is removed or renamed, so the deprecation lifecycle that protects a genuinely removed field does not apply. LLM reviewers frequently misread the two-sided diff — the field disappearing from the base object and reappearing in the tableextension — as a shipped field being deleted and illegally re-added under the same ID, and demand `ObsoleteState = Pending` staging that this refactor does not need. + +## Best Practice + +Recognize a field that is removed from a base table (or base-app layer) and re-declared in a tableextension of the same table, with the same field ID and name, as a same-app relocation. Do not flag it as a deleted or renamed shipped field, and do not require `ObsoleteState = Pending`, `ObsoleteReason`, `ObsoleteTag`, or a deprecation window for the move itself. The `obsolete-table-fields-instead-of-deleting-them` and `obsolete-pending-to-removed-staging` rules apply to fields that leave the table's contract entirely, not to fields relocated within the same app under an unchanged ID. diff --git a/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md b/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md new file mode 100644 index 0000000..fc39c13 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [released-baseline, unreleased, rename, renumber, obsolete, api-stability, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Changing an unreleased symbol is not a breaking change + +## Description + +Breaking-change rules protect contracts that have already shipped to customers or are exposed to external extensions. A symbol — an object, field, key, enum value, or procedure — that is new in this app, was introduced and then changed within the same still-unreleased development cycle, or belongs to an app that has no released version yet, can be renamed, renumbered, or removed freely. There is no shipped contract to break, so the change is not a breaking change. + +Release status is established from the diff, the app's `app.json` version, or a released baseline. An app whose `app.json` version has no corresponding released baseline (for example a `1.0.0.0` app that has never shipped) has no protected surface. + +## Best Practice + +Before treating a rename, renumber, or removal as breaking, establish that the affected symbol was present in a released baseline. Do not flag changes to symbols that are new in the current unreleased cycle or that belong to an app with no released version. When release status cannot be established from the diff, `app.json`, or a released baseline, omit the finding rather than assert a break. + +## Anti Pattern + +Reporting a breaking change for a rename, renumber, or removal without confirming the symbol shipped in a released version — for example flagging a break on an app whose `app.json` version has no released baseline. diff --git a/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.bad.al b/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.bad.al new file mode 100644 index 0000000..9805674 --- /dev/null +++ b/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.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; +} diff --git a/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.good.al b/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.good.al new file mode 100644 index 0000000..fc78c6f --- /dev/null +++ b/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.good.al @@ -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; +} diff --git a/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.md b/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.md new file mode 100644 index 0000000..10946a5 --- /dev/null +++ b/microsoft/knowledge/data-modeling/share-mediaset-items-with-insert-not-field-assignment.md @@ -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`. diff --git a/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.bad.al b/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.bad.al new file mode 100644 index 0000000..694575f --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.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; + } + } +} diff --git a/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.good.al b/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.good.al new file mode 100644 index 0000000..1f8a8a2 --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.good.al @@ -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; + } + } +} diff --git a/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.md b/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.md new file mode 100644 index 0000000..1908f82 --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-relation-extensions-are-additive-and-top-down.md @@ -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`. diff --git a/microsoft/knowledge/error-handling/fielderror-default-message-logic.md b/microsoft/knowledge/error-handling/fielderror-default-message-logic.md index 578092f..c02bb4f 100644 --- a/microsoft/knowledge/error-handling/fielderror-default-message-logic.md +++ b/microsoft/knowledge/error-handling/fielderror-default-message-logic.md @@ -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. \ No newline at end of file +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`. \ No newline at end of file diff --git a/microsoft/knowledge/error-handling/fielderror-vs-testfield.md b/microsoft/knowledge/error-handling/fielderror-vs-testfield.md index 2208a18..1353c03 100644 --- a/microsoft/knowledge/error-handling/fielderror-vs-testfield.md +++ b/microsoft/knowledge/error-handling/fielderror-vs-testfield.md @@ -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`. diff --git a/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md b/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md index f61553d..52e3e40 100644 --- a/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md +++ b/microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md @@ -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. diff --git a/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md b/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md new file mode 100644 index 0000000..f637fae --- /dev/null +++ b/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [oninsertrecord, onmodifyrecord, ondeleterecord, onquerypage, boolean-trigger, exit, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Page record triggers return true by default; a missing exit(true) does not block the operation + +## Description + +The Boolean page record triggers `OnInsertRecord`, `OnModifyRecord`, `OnDeleteRecord`, and `OnQueryClosePage` return `true` by default. When the trigger body omits an explicit return value, the platform treats the result as `true` and the operation proceeds. Only an explicit `exit(false)` — or a reachable code path that returns `false` — cancels the insert, modify, delete, or page close. + +This is a defined exception to the ordinary Boolean method rule, where the default return is `false`. Reviewers unfamiliar with the exception sometimes read a page record trigger that has no `exit(true)` and conclude the operation is blocked; it is not. + +## Best Practice + +Do not claim that a missing `exit(true)` blocks or prevents an insert, modify, or delete, and do not recommend adding `exit(true)` "to let the operation proceed" — that is already the default. Evaluate these triggers only for an explicit or reachable `exit(false)`/false-returning path that would cancel the operation unintentionally. + +## Anti Pattern + +Flagging `OnInsertRecord`, `OnModifyRecord`, `OnDeleteRecord`, or `OnQueryClosePage` as defective because it "does not return `true`", or asserting that inserts/modifies/deletes will silently fail without an explicit `exit(true)`. The default return already permits the operation. diff --git a/microsoft/knowledge/error-handling/unchecked-get-throws-when-record-not-found.md b/microsoft/knowledge/error-handling/unchecked-get-throws-when-record-not-found.md new file mode 100644 index 0000000..dff1e81 --- /dev/null +++ b/microsoft/knowledge/error-handling/unchecked-get-throws-when-record-not-found.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [get, record-not-found, runtime-error, return-value, boolean-method, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# An unchecked Record.Get raises an error when the record is missing; it is not silently ignored + +## Description + +`Record.Get` returns a Boolean, but its behavior when no record is found depends on whether the return value is consumed. When the return value is used — inside `if Rec.Get(...) then`, or assigned to a variable — a missing record yields `false` and execution continues. When `Rec.Get(...)` is called as a bare statement and the return value is not used, the platform raises a runtime "record not found" error if the record does not exist. A bare `Rec.Get(Key)` therefore acts as an assertion that the record exists: it does not swallow or silently ignore a missing record. This mirrors other AL find methods, where an unconsumed return value lets the platform enforce the not-found error. + +## Best Practice + +Do not claim that a `Record.Get` whose return value is unused silently ignores a missing record or hides an error. Treat a bare `Rec.Get(...)` statement as an intentional existence assertion that already throws when the record is absent. Recommend an explicit existence check only when the surrounding logic must continue gracefully rather than error out. + +## Anti Pattern + +Flagging a bare `Rec.Get(Key)` statement as a defect because "the return value is ignored, so a missing record is swallowed", or recommending it be wrapped in `if Rec.Get(...) then ... else Error(...)` to "handle the not-found case" — the unchecked call already raises an error when the record is missing. + +## See also + +- `ignored-tryfunction-return-disables-try-semantics.md` — a different case where ignoring a Boolean return value changes behavior. diff --git a/microsoft/knowledge/events/adding-a-parameter-to-an-event-is-not-a-breaking-change.md b/microsoft/knowledge/events/adding-a-parameter-to-an-event-is-not-a-breaking-change.md new file mode 100644 index 0000000..330f9bf --- /dev/null +++ b/microsoft/knowledge/events/adding-a-parameter-to-an-event-is-not-a-breaking-change.md @@ -0,0 +1,18 @@ +--- +bc-version: [all] +domain: events +keywords: [event-parameters, signature, subscriber-binding, backward-compatibility, integration-event, breaking-change, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Adding a parameter to an event is not a breaking change + +## Description + +Adding a parameter to an existing event publisher does not break existing subscribers. AL binds a subscriber to a publisher by the event name, and the subscriber's parameter list only has to be a subset of the publisher's, matched by name and type. A subscriber that does not declare the new parameter keeps compiling and keeps binding — it simply ignores the addition. This holds for `IntegrationEvent` and `BusinessEvent` publishers, and even more plainly for `local` events. Appending the new parameter at the end keeps the change a clean, reviewable addition (see `add-new-event-parameters-at-the-end`). LLM reviewers often misreport the mere presence of a new event parameter as a "breaking event signature change" that breaks subscribers, which is incorrect. + +## Best Practice + +Do not flag the addition of a parameter to an event publisher as a breaking or signature-breaking change, and do not claim it breaks existing subscribers. Genuine, separate concerns are covered by their own rules — a parameter inserted in the middle of the list rather than appended (`add-new-event-parameters-at-the-end`), or a parameter that carries no meaningful value — and should be raised on those grounds, not framed as a backward-compatibility break. diff --git a/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md b/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md new file mode 100644 index 0000000..79c3b52 --- /dev/null +++ b/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md @@ -0,0 +1,22 @@ +--- +bc-version: [all] +domain: performance +keywords: [calcfields, onaftergetrecord, onaftergetcurrrecord, page-lifecycle, flowfield, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# CalcFields in both OnAfterGetRecord and OnAfterGetCurrRecord is not redundant + +## Description + +`OnAfterGetRecord` fires once per row as the page loads records into the view; `OnAfterGetCurrRecord` fires when a record becomes the active/current record. Calling `CalcFields` in both triggers is not duplicate or redundant work: the two triggers run at different points in the page lifecycle and serve different purposes — populating FlowFields for every displayed row versus refreshing them for the record the user has selected. The same `CalcFields` call appearing in both places is an intentional pattern, not copy-paste waste. + +## Best Practice + +Do not flag `CalcFields` appearing in both `OnAfterGetRecord` and `OnAfterGetCurrRecord` as duplicate, redundant, or removable. Treat each trigger's `CalcFields` on its own lifecycle merits. + +## Anti Pattern + +Recommending that a developer delete one of the two `CalcFields` calls because "the field is already calculated in the other trigger". The genuine per-row FlowField cost is addressed by the separate guidance on FlowField calculation in loops and on hidden FlowFields; it is not addressed by removing a lifecycle-correct `CalcFields`. diff --git a/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.bad.al b/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.bad.al new file mode 100644 index 0000000..0666e0e --- /dev/null +++ b/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.bad.al @@ -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; +} diff --git a/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.good.al b/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.good.al new file mode 100644 index 0000000..d1aea4c --- /dev/null +++ b/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.good.al @@ -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; +} diff --git a/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.md b/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.md new file mode 100644 index 0000000..b03ac51 --- /dev/null +++ b/microsoft/knowledge/performance/hidden-flowfields-still-calculate-before-bc26-opt-in.md @@ -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`. diff --git a/microsoft/knowledge/performance/onaftergetcurrrecord-is-not-per-row.md b/microsoft/knowledge/performance/onaftergetcurrrecord-is-not-per-row.md new file mode 100644 index 0000000..928ad02 --- /dev/null +++ b/microsoft/knowledge/performance/onaftergetcurrrecord-is-not-per-row.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: performance +keywords: [onaftergetcurrrecord, onaftergetrecord, calcfields, n-plus-one, page-lifecycle, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Database work in OnAfterGetCurrRecord is not a per-row or N+1 cost + +## Description + +`OnAfterGetCurrRecord` fires only when the current/active record changes — typically once when the page opens and once each time the user selects a different row — not once for every row rendered in a list. Database work placed there, such as `CalcFields`, `Get`, or a lookup, therefore runs a bounded number of times driven by user navigation, not multiplied by the number of visible rows. This is unlike `OnAfterGetRecord`, which fires once per row as the page loads records and can create a genuine N+1 pattern. Reviewers sometimes see `CalcFields` or a database call inside a page trigger and assume it runs for every row; the trigger name determines whether that assumption holds. + +## Best Practice + +Before flagging `CalcFields`, `Get`, or a similar database call in a page trigger as a per-row or N+1 problem, confirm the trigger is `OnAfterGetRecord`, which runs per row. Do not flag the same work in `OnAfterGetCurrRecord`: that trigger runs on current-record change, not for every displayed row. + +## Anti Pattern + +Reporting `CalcFields` or another database call inside `OnAfterGetCurrRecord` as an N+1 or per-row performance defect, or recommending it be moved out "to avoid running once per row". The trigger does not run per row. + +## See also + +- `calcfields-in-both-getrecord-triggers-is-not-redundant.md` — the lifecycle distinction between the two triggers. diff --git a/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md b/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md new file mode 100644 index 0000000..50eb014 --- /dev/null +++ b/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md @@ -0,0 +1,22 @@ +--- +bc-version: [all] +domain: performance +keywords: [filter, drilldown, lookup, sourcetableview, tablerelation, setrange, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A page or lookup's effective filter may be defined outside the changed hunk + +## Description + +The effective filter on a drill-down, lookup, or list result set is frequently defined outside any single changed hunk — on the table via a `SourceTableView` property or a `TableRelation`, or through `SetRange`/`SetFilter` calls in unchanged code that runs before the result is shown. The absence of a filter within the changed lines of a diff is therefore not evidence that the result set is unfiltered or that it will load an entire table. + +## Best Practice + +Do not assert that a drill-down, lookup, or list is "unfiltered" based only on the changed hunk. Confirm the effective filter by checking the page's `SourceTableView`, the field's `TableRelation`, and any `SetRange`/`SetFilter` in the surrounding (possibly unchanged) code before raising a finding about an unbounded result set. + +## Anti Pattern + +Concluding that a lookup or drill-down loads an unfiltered, full-table result set solely because no `SetRange`/`SetFilter` appears in the changed lines, when the filter is defined on the table, in a `TableRelation`, or in unchanged setup code. diff --git a/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md b/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md new file mode 100644 index 0000000..ed41f1b --- /dev/null +++ b/microsoft/knowledge/performance/primary-key-get-in-loop-is-transaction-cached.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: performance +keywords: [get, primary-key, record-cache, transaction, n-plus-one, dictionary-cache, over-engineering, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A primary-key Get() in a per-row helper is not an N+1 to cache manually + +## Description + +The Business Central server caches primary-key reads within a transaction. Repeated `Record.Get()` calls for the same key are served from that cache rather than re-queried, so a guarded `if not Rec.Get(...) then exit;` inside a per-row helper is not a genuine N+1 pattern. When each row legitimately carries a distinct key — for example one `Bin Content` row per bin, so `Bin.Get` and `BinType.Get` see a different bin each iteration — the `Get` must run per row regardless, and there is nothing to hoist. + +Reviewers sometimes see two `Get` calls inside a routine that runs once per row and recommend wrapping them in a `Dictionary` cache. That is over-engineering: it duplicates the server's built-in record cache, adds state that must be invalidated, and breaks the surrounding extension's established pattern of direct guarded `Get` calls. + +## Best Practice + +Treat a primary-key `Get()` — especially a guarded `if not Rec.Get(...) then exit;` — as a cheap, transaction-cached read. Do not recommend a manual `Dictionary` cache around per-row primary-key `Get` calls. Reserve N+1 concerns for genuinely repeated non-keyed queries (`FindSet`/`FindFirst` with filters, `Count`) that re-hit the database each iteration. + +## Anti Pattern + +Reporting repeated primary-key `Get` calls (such as `Bin.Get` and `BinType.Get`) inside a per-row helper as a performance defect, or recommending they be cached in a `Dictionary`. The reads are already cached by the server within the transaction, and per-row keys often differ so the calls cannot be hoisted. diff --git a/microsoft/knowledge/performance/use-tryfunction-for-error-catching-not-rollback.md b/microsoft/knowledge/performance/use-tryfunction-for-error-catching-not-rollback.md index bb015f6..6070b76 100644 --- a/microsoft/knowledge/performance/use-tryfunction-for-error-catching-not-rollback.md +++ b/microsoft/knowledge/performance/use-tryfunction-for-error-catching-not-rollback.md @@ -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. diff --git a/microsoft/knowledge/privacy/avoid-strsubstno-prebuild-before-error.md b/microsoft/knowledge/privacy/avoid-strsubstno-prebuild-before-error.md index 5d292fa..f9245b1 100644 --- a/microsoft/knowledge/privacy/avoid-strsubstno-prebuild-before-error.md +++ b/microsoft/knowledge/privacy/avoid-strsubstno-prebuild-before-error.md @@ -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`. diff --git a/microsoft/knowledge/privacy/error-direct-substitution-safe-for-telemetry.good.al b/microsoft/knowledge/privacy/error-direct-substitution-safe-for-telemetry.good.al deleted file mode 100644 index 1b8c886..0000000 --- a/microsoft/knowledge/privacy/error-direct-substitution-safe-for-telemetry.good.al +++ /dev/null @@ -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; -} diff --git a/microsoft/knowledge/privacy/error-direct-substitution-safe-for-telemetry.md b/microsoft/knowledge/privacy/error-direct-substitution-safe-for-telemetry.md deleted file mode 100644 index f4cac77..0000000 --- a/microsoft/knowledge/privacy/error-direct-substitution-safe-for-telemetry.md +++ /dev/null @@ -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`. diff --git a/microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.bad.al b/microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.bad.al index 06970e2..ef5ff7c 100644 --- a/microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.bad.al +++ b/microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.bad.al @@ -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; } diff --git a/microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.good.al b/microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.good.al index 96a553a..a67e2d9 100644 --- a/microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.good.al +++ b/microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.good.al @@ -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; } diff --git a/microsoft/knowledge/privacy/session-logmessage-requires-dataclassification.bad.al b/microsoft/knowledge/privacy/session-logmessage-requires-dataclassification.bad.al index e895d7c..6428b08 100644 --- a/microsoft/knowledge/privacy/session-logmessage-requires-dataclassification.bad.al +++ b/microsoft/knowledge/privacy/session-logmessage-requires-dataclassification.bad.al @@ -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; } diff --git a/microsoft/knowledge/privacy/session-logmessage-requires-dataclassification.good.al b/microsoft/knowledge/privacy/session-logmessage-requires-dataclassification.good.al index d3353ec..3f78158 100644 --- a/microsoft/knowledge/privacy/session-logmessage-requires-dataclassification.good.al +++ b/microsoft/knowledge/privacy/session-logmessage-requires-dataclassification.good.al @@ -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; } diff --git a/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.bad.al b/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.bad.al new file mode 100644 index 0000000..7d9411b --- /dev/null +++ b/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.bad.al @@ -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; +} diff --git a/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.good.al b/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.good.al new file mode 100644 index 0000000..4079455 --- /dev/null +++ b/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.good.al @@ -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; +} diff --git a/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.md b/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.md new file mode 100644 index 0000000..4bc8816 --- /dev/null +++ b/microsoft/knowledge/query/reopening-query-resets-cursor-but-keeps-filters.md @@ -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`. diff --git a/microsoft/knowledge/query/set-query-filters-before-open.bad.al b/microsoft/knowledge/query/set-query-filters-before-open.bad.al new file mode 100644 index 0000000..eed5f9e --- /dev/null +++ b/microsoft/knowledge/query/set-query-filters-before-open.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; +} diff --git a/microsoft/knowledge/query/set-query-filters-before-open.good.al b/microsoft/knowledge/query/set-query-filters-before-open.good.al new file mode 100644 index 0000000..86bd2d4 --- /dev/null +++ b/microsoft/knowledge/query/set-query-filters-before-open.good.al @@ -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; +} diff --git a/microsoft/knowledge/query/set-query-filters-before-open.md b/microsoft/knowledge/query/set-query-filters-before-open.md new file mode 100644 index 0000000..f999456 --- /dev/null +++ b/microsoft/knowledge/query/set-query-filters-before-open.md @@ -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`. diff --git a/microsoft/knowledge/security/classify-every-field-with-dataclassification.bad.al b/microsoft/knowledge/security/classify-every-field-with-dataclassification.bad.al deleted file mode 100644 index a9ae935..0000000 --- a/microsoft/knowledge/security/classify-every-field-with-dataclassification.bad.al +++ /dev/null @@ -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; } - } -} diff --git a/microsoft/knowledge/security/classify-every-field-with-dataclassification.good.al b/microsoft/knowledge/security/classify-every-field-with-dataclassification.good.al deleted file mode 100644 index baa3079..0000000 --- a/microsoft/knowledge/security/classify-every-field-with-dataclassification.good.al +++ /dev/null @@ -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; } - } -} diff --git a/microsoft/knowledge/security/classify-every-field-with-dataclassification.md b/microsoft/knowledge/security/classify-every-field-with-dataclassification.md deleted file mode 100644 index 1b854aa..0000000 --- a/microsoft/knowledge/security/classify-every-field-with-dataclassification.md +++ /dev/null @@ -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`. diff --git a/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.bad.al b/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.bad.al new file mode 100644 index 0000000..f662f8c --- /dev/null +++ b/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.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; +} diff --git a/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.good.al b/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.good.al new file mode 100644 index 0000000..0691c31 --- /dev/null +++ b/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.good.al @@ -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.'; +} diff --git a/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.md b/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.md new file mode 100644 index 0000000..bdd8462 --- /dev/null +++ b/microsoft/knowledge/security/internal-access-is-not-a-security-boundary.md @@ -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`. diff --git a/microsoft/knowledge/security/secrets-isolated-storage.md b/microsoft/knowledge/security/secrets-isolated-storage.md index 91afe69..1434c1a 100644 --- a/microsoft/knowledge/security/secrets-isolated-storage.md +++ b/microsoft/knowledge/security/secrets-isolated-storage.md @@ -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`. diff --git a/microsoft/knowledge/security/secrettext-for-credentials.md b/microsoft/knowledge/security/secrettext-for-credentials.md index 26a2511..0eb6cb8 100644 --- a/microsoft/knowledge/security/secrettext-for-credentials.md +++ b/microsoft/knowledge/security/secrettext-for-credentials.md @@ -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`. diff --git a/microsoft/knowledge/security/secrettext-with-httpclient.md b/microsoft/knowledge/security/secrettext-with-httpclient.md index dc67e3e..689a7b6 100644 --- a/microsoft/knowledge/security/secrettext-with-httpclient.md +++ b/microsoft/knowledge/security/secrettext-with-httpclient.md @@ -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`. diff --git a/microsoft/knowledge/style/caption-required-on-page-fields.bad.al b/microsoft/knowledge/style/caption-required-on-page-fields.bad.al index bd12f36..fd458a4 100644 --- a/microsoft/knowledge/style/caption-required-on-page-fields.bad.al +++ b/microsoft/knowledge/style/caption-required-on-page-fields.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 = ''; + } + } } } } diff --git a/microsoft/knowledge/style/caption-required-on-page-fields.good.al b/microsoft/knowledge/style/caption-required-on-page-fields.good.al index 7de715b..0493b6e 100644 --- a/microsoft/knowledge/style/caption-required-on-page-fields.good.al +++ b/microsoft/knowledge/style/caption-required-on-page-fields.good.al @@ -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.'; + } + } } } } diff --git a/microsoft/knowledge/style/temporary-variable-temp-prefix.md b/microsoft/knowledge/style/temporary-variable-temp-prefix.md index 2211b4c..16e85c2 100644 --- a/microsoft/knowledge/style/temporary-variable-temp-prefix.md +++ b/microsoft/knowledge/style/temporary-variable-temp-prefix.md @@ -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`. diff --git a/microsoft/knowledge/style/tooltip-required-on-page-fields.md b/microsoft/knowledge/style/tooltip-required-on-page-fields.md index fc5a3cb..a11d4a9 100644 --- a/microsoft/knowledge/style/tooltip-required-on-page-fields.md +++ b/microsoft/knowledge/style/tooltip-required-on-page-fields.md @@ -15,9 +15,11 @@ CodeCop AA0218 requires a non-empty `ToolTip` property on every field control on Acceptable exceptions: table fields inside `Upgrade`, `Migration`, `HybridBC14`, `HybridSL`, and `HybridGP` codeunits and tables are allowed to omit the tooltip — those types are not surfaced to users. +AA0218 is a compiler analyzer, but its severity is configured per app in the ruleset and is frequently downgraded to `info`/`None` or disabled entirely. PR review therefore cannot assume the compiler will surface the gap: it is the last line of defence for a missing tooltip and should flag it independently. The one case review must *not* flag is a bound field that inherits a `ToolTip` from its source table field — see `bound-page-field-inherits-source-field-tooltip`. + ## Best Practice -Every field control on a regular page carries `ToolTip = 'Specifies …';` (or a clear alternative phrasing). Compose the text in the form "what this value shows" rather than "what the user does with it". +Every field control on a regular page carries `ToolTip = 'Specifies …';` (or a clear alternative phrasing). Compose the text in the form "what this value shows" rather than "what the user does with it". In review, raise a `medium`-severity finding for a field that has neither an inline nor an inherited tooltip, independently of whether AA0218 is active in the app's ruleset. See sample: `tooltip-required-on-page-fields.good.al`. diff --git a/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.bad.al b/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.bad.al new file mode 100644 index 0000000..bd87b07 --- /dev/null +++ b/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.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; +} diff --git a/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.good.al b/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.good.al new file mode 100644 index 0000000..3233042 --- /dev/null +++ b/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.good.al @@ -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; +} diff --git a/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.md b/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.md new file mode 100644 index 0000000..940c9c8 --- /dev/null +++ b/microsoft/knowledge/telemetry/choose-telemetry-scope-by-audience.md @@ -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`. diff --git a/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.bad.al b/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.bad.al new file mode 100644 index 0000000..39f2940 --- /dev/null +++ b/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.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; +} diff --git a/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.good.al b/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.good.al new file mode 100644 index 0000000..323bdd6 --- /dev/null +++ b/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.good.al @@ -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; +} diff --git a/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.md b/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.md new file mode 100644 index 0000000..eb05742 --- /dev/null +++ b/microsoft/knowledge/telemetry/feature-uptake-transitions-in-order.md @@ -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`. diff --git a/microsoft/knowledge/telemetry/feature-usage-only-after-success.bad.al b/microsoft/knowledge/telemetry/feature-usage-only-after-success.bad.al new file mode 100644 index 0000000..9c3e2a2 --- /dev/null +++ b/microsoft/knowledge/telemetry/feature-usage-only-after-success.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.'; +} diff --git a/microsoft/knowledge/telemetry/feature-usage-only-after-success.good.al b/microsoft/knowledge/telemetry/feature-usage-only-after-success.good.al new file mode 100644 index 0000000..8ce7616 --- /dev/null +++ b/microsoft/knowledge/telemetry/feature-usage-only-after-success.good.al @@ -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.'; +} diff --git a/microsoft/knowledge/telemetry/feature-usage-only-after-success.md b/microsoft/knowledge/telemetry/feature-usage-only-after-success.md new file mode 100644 index 0000000..a1f118b --- /dev/null +++ b/microsoft/knowledge/telemetry/feature-usage-only-after-success.md @@ -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`. diff --git a/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.bad.al b/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.bad.al new file mode 100644 index 0000000..4abc1c9 --- /dev/null +++ b/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.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; +} diff --git a/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.good.al b/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.good.al new file mode 100644 index 0000000..e8f99af --- /dev/null +++ b/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.good.al @@ -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; +} diff --git a/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.md b/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.md new file mode 100644 index 0000000..4c2c41a --- /dev/null +++ b/microsoft/knowledge/telemetry/keep-custom-dimension-schema-stable.md @@ -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`. diff --git a/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.bad.al b/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.bad.al new file mode 100644 index 0000000..d31fa23 --- /dev/null +++ b/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.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.'; +} diff --git a/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.good.al b/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.good.al new file mode 100644 index 0000000..c730b1b --- /dev/null +++ b/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.good.al @@ -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.'; +} diff --git a/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.md b/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.md new file mode 100644 index 0000000..4ff780a --- /dev/null +++ b/microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.md @@ -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`. diff --git a/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.bad.al b/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.bad.al new file mode 100644 index 0000000..e186247 --- /dev/null +++ b/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.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; +} diff --git a/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.good.al b/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.good.al new file mode 100644 index 0000000..2cbe885 --- /dev/null +++ b/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.good.al @@ -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; +} diff --git a/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.md b/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.md new file mode 100644 index 0000000..d88c020 --- /dev/null +++ b/microsoft/knowledge/telemetry/register-one-telemetry-logger-per-publisher.md @@ -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`. diff --git a/microsoft/knowledge/style/telemetry-event-id-stable-unique.bad.al b/microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.bad.al similarity index 89% rename from microsoft/knowledge/style/telemetry-event-id-stable-unique.bad.al rename to microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.bad.al index be60f4b..c9615be 100644 --- a/microsoft/knowledge/style/telemetry-event-id-stable-unique.bad.al +++ b/microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.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 diff --git a/microsoft/knowledge/style/telemetry-event-id-stable-unique.good.al b/microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.good.al similarity index 88% rename from microsoft/knowledge/style/telemetry-event-id-stable-unique.good.al rename to microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.good.al index 4491e31..dcdf6ae 100644 --- a/microsoft/knowledge/style/telemetry-event-id-stable-unique.good.al +++ b/microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.good.al @@ -1,4 +1,4 @@ -codeunit 50261 "Sample Telemetry Id Good" +codeunit 50261 "Telemetry Event Id Good" { procedure LogCustomerProcessed(var Customer: Record Customer) begin diff --git a/microsoft/knowledge/style/telemetry-event-id-stable-unique.md b/microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.md similarity index 98% rename from microsoft/knowledge/style/telemetry-event-id-stable-unique.md rename to microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.md index da00af3..ca6d2d6 100644 --- a/microsoft/knowledge/style/telemetry-event-id-stable-unique.md +++ b/microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.md @@ -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] diff --git a/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.bad.al b/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.bad.al new file mode 100644 index 0000000..46cfed7 --- /dev/null +++ b/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.bad.al @@ -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; +} diff --git a/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.good.al b/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.good.al new file mode 100644 index 0000000..315ce6a --- /dev/null +++ b/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.good.al @@ -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"; +} diff --git a/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.md b/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.md new file mode 100644 index 0000000..8e3e126 --- /dev/null +++ b/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.md @@ -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`. diff --git a/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.bad.al b/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.bad.al new file mode 100644 index 0000000..3d4bbc3 --- /dev/null +++ b/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.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; +} diff --git a/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.good.al b/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.good.al new file mode 100644 index 0000000..6bf4c8d --- /dev/null +++ b/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.good.al @@ -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; +} diff --git a/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.md b/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.md new file mode 100644 index 0000000..03d8854 --- /dev/null +++ b/microsoft/knowledge/testing/testisolation-belongs-on-the-test-runner.md @@ -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`. diff --git a/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md new file mode 100644 index 0000000..87b539b --- /dev/null +++ b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: ui +keywords: [tooltip, page-field, source-field, inheritance, aa0218, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A page field bound to a table field inherits that field's ToolTip + +## Description + +A page field bound to a table field inherits the source field's `ToolTip` at runtime: the control shows the table field's `ToolTip` even when the page control declares none of its own. A page field without an inline `ToolTip` is therefore not, by itself, a missing-tooltip defect — the text may be supplied by the bound source field. + +The genuinely-missing case is different: a bound field whose source table field *also* carries no `ToolTip`, or an unbound control, has no text to inherit and is a real accessibility gap. The compiler analyzer AA0218 detects this mechanically, but its severity is set by each app's ruleset and is routinely downgraded or disabled — so it cannot be relied on as the only net. PR review is the last line of defence and should raise this case independently. + +## Best Practice + +Do not raise a missing-`ToolTip` finding for a bound page field whose source table field supplies a `ToolTip`; assume the control inherits it. Do raise a `medium`-severity finding when the field has no inline `ToolTip` **and** no inherited one — that is, a bound field whose source field is also tooltip-less, or an unbound control — rather than assuming AA0218 will catch it downstream. + +## Anti Pattern + +Two opposite failures: (1) flagging every page field that has no inline `ToolTip` as a violation, ignoring that a bound field inherits its source field's tooltip; and (2) staying silent on a field that has neither an inline nor an inherited tooltip on the assumption that the compiler's AA0218 will report it — a ruleset that downgrades or disables AA0218 then lets a genuine gap ship unflagged. diff --git a/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.good.al b/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.good.al new file mode 100644 index 0000000..077bcdf --- /dev/null +++ b/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.good.al @@ -0,0 +1,38 @@ +page 50210 "UI Sample Caption Case" +{ + PageType = List; + ApplicationArea = All; + SourceTable = "Sales Line"; + + layout + { + area(Content) + { + repeater(Lines) + { + field("Document No."; Rec."Document No.") + { + ToolTip = 'Specifies the document number.'; + } + } + } + } + + actions + { + area(Processing) + { + action(ShowSourceDocument) + { + Caption = 'Show source document'; + Image = ViewSourceDocumentLine; + ToolTip = 'Open the related source document.'; + + trigger OnAction() + begin + Message('%1', Rec."Document No."); + end; + } + } + } +} diff --git a/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.md b/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.md new file mode 100644 index 0000000..f0d3f8c --- /dev/null +++ b/microsoft/knowledge/ui/caption-capitalization-noun-phrase-vs-sentence-phrase.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: ui +keywords: [caption, capitalization, sentence-case, title-case, action, noun-phrase, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Sentence-phrase captions use sentence case, not title case + +## Description + +Business Central caption capitalization depends on whether the caption reads as a **noun phrase** or a **sentence/verb phrase**. Following the Microsoft writing-style guideline, a caption that reads as an imperative sentence — most action captions, such as `'Show source document'`, `'Post and print'`, or `'Copy from last inspection'` — uses **sentence case**: only the first word and any proper nouns are capitalized. Title case (`'Show Source Document'`) is the older convention and is not required for these captions. + +Noun-phrase captions (object names, field labels such as `'Source Document No.'`) follow their own capitalization; that is a separate case and is not what this article covers. Reviewers sometimes see a lower-cased word in an action caption (`'Show source document'`) and flag it as inconsistent title case, but a sentence-phrase action caption is correct as written. + +## Best Practice + +For an action `Caption` that reads as a sentence or verb phrase, capitalize only the first word and proper nouns (sentence case). Do not require every significant word to be capitalized. Before flagging a caption as "should be title case", confirm it is a noun phrase; leave imperative/sentence-phrase action captions in sentence case. + +See sample: `caption-capitalization-noun-phrase-vs-sentence-phrase.good.al`. + +## Anti Pattern + +Reporting a sentence-case action caption such as `'Show source document'` as a style defect and recommending title case (`'Show Source Document'`), or calling it inconsistent with BC conventions. Sentence case is the current guideline for sentence-phrase captions. diff --git a/microsoft/knowledge/ui/page-background-tasks.md b/microsoft/knowledge/ui/page-background-tasks.md index 8a1d3fa..c4cccb8 100644 --- a/microsoft/knowledge/ui/page-background-tasks.md +++ b/microsoft/knowledge/ui/page-background-tasks.md @@ -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] diff --git a/microsoft/knowledge/upgrade/do-not-block-upgrade-on-data-errors.good.al b/microsoft/knowledge/upgrade/do-not-block-upgrade-on-data-errors.good.al index 7a83df2..09a14f5 100644 --- a/microsoft/knowledge/upgrade/do-not-block-upgrade-on-data-errors.good.al +++ b/microsoft/knowledge/upgrade/do-not-block-upgrade-on-data-errors.good.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, diff --git a/microsoft/knowledge/upgrade/obsoletereason-need-not-restate-removal-version.md b/microsoft/knowledge/upgrade/obsoletereason-need-not-restate-removal-version.md new file mode 100644 index 0000000..9165921 --- /dev/null +++ b/microsoft/knowledge/upgrade/obsoletereason-need-not-restate-removal-version.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [obsolete-reason, obsolete-tag, deprecation, version, metadata, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# ObsoleteReason need not restate the removal version; ObsoleteTag carries it + +## Description + +An obsoleted object, field, key, enum, or enum value carries both `ObsoleteReason` and `ObsoleteTag`, and the two properties have different jobs. `ObsoleteReason` is free text that explains why the element is obsolete and what replaces it. `ObsoleteTag` identifies when it became obsolete — typically the version, release, or work item that introduced the obsoletion. The version traceability lives in `ObsoleteTag`; there is no requirement that `ObsoleteReason` also name the removal version or repeat what the tag already records. A reason that omits a version number is complete as long as it explains the deprecation and points to a replacement, provided `ObsoleteTag` pins the version. + +## Best Practice + +When `ObsoleteTag` already carries the version or tracking reference, do not flag `ObsoleteReason` for not mentioning a version or removal release. Judge `ObsoleteReason` on whether it explains the deprecation and names a replacement, and judge version traceability on `ObsoleteTag` instead. + +## Anti Pattern + +Flagging an `ObsoleteReason` as vague, incomplete, or missing a version reference solely because it does not restate the removal version, when `ObsoleteTag` already records that version. Requiring the reason to duplicate the tag's version is not a real convention. + +## See also + +- `obsoletion-requires-reason-and-tag.md` — both properties are required; the reason names the replacement and the tag identifies when the element became obsolete. diff --git a/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md b/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md new file mode 100644 index 0000000..248943f --- /dev/null +++ b/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [released-baseline, unreleased, schema, migration, obsolete, data-loss, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Unreleased schema changes need no upgrade or migration path + +## Description + +Upgrade and migration findings protect data and schema that have already shipped to customers. A schema element — a table, field, key, or enum — that is new in this app, or was added and then changed within the same still-unreleased development cycle, needs no upgrade code or migration path: no customer has data in it yet, so there is nothing to preserve or migrate. Such a change is not an obsoletion, data-loss, or breaking-migration defect. + +Release status is established from the diff, the app's `app.json` version, or a released baseline. A schema element with no released baseline has no persisted customer data to protect. + +## Best Practice + +Before asserting an obsoletion, data-loss, or breaking-migration defect, establish that the affected table, field, key, or enum existed in a released version. Do not require upgrade or migration code for schema that never shipped. When release status cannot be established from the diff, `app.json`, or a released baseline, omit the finding rather than demand a migration path. + +## Anti Pattern + +Demanding an upgrade codeunit, migration path, or data-preservation step, or flagging data loss, for a table, field, key, or enum that is new in the current unreleased cycle and has no released baseline. diff --git a/microsoft/skills/review/al-appsource-review.md b/microsoft/skills/review/al-appsource-review.md index 14af7bf..43a2e20 100644 --- a/microsoft/skills/review/al-appsource-review.md +++ b/microsoft/skills/review/al-appsource-review.md @@ -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: @@ -82,7 +86,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"AppSource"`. A populated example: ```json { @@ -105,6 +109,7 @@ Output conforms to the DO output contract. A populated example: { "path": "microsoft/knowledge/appsource/object-affixes-prevent-collisions.md" } ], "confidence": "high", + "domain": "AppSource", "suggested-code": "field(50100; \"Loyalty Points ABC\"; Integer)" } ], diff --git a/microsoft/skills/review/al-breaking-changes-review.md b/microsoft/skills/review/al-breaking-changes-review.md index 4238bca..82aeda0 100644 --- a/microsoft/skills/review/al-breaking-changes-review.md +++ b/microsoft/skills/review/al-breaking-changes-review.md @@ -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: @@ -77,7 +89,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Breaking Changes"`. A populated example: ```json { @@ -100,7 +112,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Breaking Changes" }, { "id": "microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md", @@ -113,7 +126,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md" } ], - "confidence": "medium" + "confidence": "medium", + "domain": "Breaking Changes" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-code-review.md b/microsoft/skills/review/al-code-review.md index df25e4d..9b3f934 100644 --- a/microsoft/skills/review/al-code-review.md +++ b/microsoft/skills/review/al-code-review.md @@ -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`. For non-citation findings (those whose `id` is a skill-defined slug rather than a reference path), prefix `id` with `:` 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 `:` to prevent collisions across sub-skills. Other finding fields are preserved. ### Agent self-review pass @@ -89,11 +93,12 @@ Frame the pass by cross-cutting concerns — architecture, error handling, resou For every candidate the agent identifies in this pass: 1. **Validate against BCQuality knowledge.** Check the candidate against the knowledge files the sub-skills have already loaded for this task (visible via their `references` and `suppressed` lists in `sub-results`). - - If a BCQuality knowledge file matches the candidate, upgrade it to a knowledge-backed finding: cite the file in `references`, set `id` to the file's path, set `from-sub-skill` to the sub-skill that owns that knowledge domain, and merge with or deduplicate against any sub-skill finding that already covers the same concern at the same location. + - If a BCQuality knowledge file matches the candidate, upgrade it to a knowledge-backed finding: cite the file in `references`, set `id` to the file's path, set `from-sub-skill` to the sub-skill that owns that knowledge domain, set `domain` to the human-readable label required by that sub-skill's Output contract, and merge with or deduplicate against any sub-skill finding that already covers the same concern at the same location. - If a BCQuality knowledge file **explicitly contradicts** the candidate (its `## Best Practice` or `## Anti Pattern` says the opposite of what the agent flagged), suppress the candidate and do not surface it. - Otherwise the candidate has no BCQuality coverage; emit it as a super-skill agent finding. 2. **Emit agent finding.** Per DO's *Agent findings* rules: - `from-sub-skill: "agent"` (the super-skill itself produced it) + - `domain: "Agent"` (the display label for super-skill cross-cutting findings) - `references: []` - `id` is a skill-defined slug prefixed with `agent:` (for example, `agent:missing-error-handling-on-http-call`). - `confidence` capped at `medium`. @@ -117,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: @@ -143,7 +150,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip { "path": "microsoft/knowledge/performance/apply-filters-before-iterating.md" } ], "confidence": "high", - "from-sub-skill": "al-performance-review" + "from-sub-skill": "al-performance-review", + "domain": "Performance" }, { "id": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md", @@ -157,7 +165,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip { "path": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md" } ], "confidence": "high", - "from-sub-skill": "al-performance-review" + "from-sub-skill": "al-performance-review", + "domain": "Performance" }, { "id": "microsoft/knowledge/security/secrettext-for-credentials.md", @@ -172,7 +181,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip { "path": "microsoft/knowledge/security/secrettext-for-credentials.md" } ], "confidence": "high", - "from-sub-skill": "al-security-review" + "from-sub-skill": "al-security-review", + "domain": "Security" }, { "id": "microsoft/knowledge/security/secrets-isolated-storage.md", @@ -186,7 +196,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip { "path": "microsoft/knowledge/security/secrets-isolated-storage.md" } ], "confidence": "medium", - "from-sub-skill": "al-security-review" + "from-sub-skill": "al-security-review", + "domain": "Security" }, { "id": "agent:missing-error-handling-on-http-client", @@ -199,7 +210,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip }, "references": [], "confidence": "medium", - "from-sub-skill": "agent" + "from-sub-skill": "agent", + "domain": "Agent" } ], "suppressed": [], @@ -224,7 +236,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip "references": [ { "path": "microsoft/knowledge/performance/apply-filters-before-iterating.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Performance" }, { "id": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md", @@ -237,7 +250,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip "references": [ { "path": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Performance" } ], "suppressed": [] @@ -262,7 +276,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip "references": [ { "path": "microsoft/knowledge/security/secrettext-for-credentials.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Security" }, { "id": "microsoft/knowledge/security/secrets-isolated-storage.md", @@ -275,7 +290,8 @@ Output conforms to the DO output contract, extended with `sub-results` and `skip "references": [ { "path": "microsoft/knowledge/security/secrets-isolated-storage.md" } ], - "confidence": "medium" + "confidence": "medium", + "domain": "Security" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-data-modeling-review.md b/microsoft/skills/review/al-data-modeling-review.md index 7cbe48d..2386613 100644 --- a/microsoft/skills/review/al-data-modeling-review.md +++ b/microsoft/skills/review/al-data-modeling-review.md @@ -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: @@ -85,7 +87,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Data Modeling"`. A populated example: ```json { @@ -108,6 +110,7 @@ Output conforms to the DO output contract. A populated example: { "path": "microsoft/knowledge/data-modeling/set-last-date-modified-in-onmodify-and-onrename.md" } ], "confidence": "high", + "domain": "Data Modeling", "suggested-code": "trigger OnRename()\nbegin\n \"Last Date Modified\" := Today();\nend;" } ], diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index eaf252b..39851ac 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -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: @@ -78,7 +87,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Error Handling"`. A populated example: ```json { @@ -101,7 +110,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/error-handling/prefer-errorinfo-for-actionable-errors.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Error Handling" }, { "id": "microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md", @@ -114,7 +124,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/error-handling/errortype-internal-vs-client-for-diagnostics.md" } ], - "confidence": "medium" + "confidence": "medium", + "domain": "Error Handling" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-events-review.md b/microsoft/skills/review/al-events-review.md index 2d7a76d..de2700c 100644 --- a/microsoft/skills/review/al-events-review.md +++ b/microsoft/skills/review/al-events-review.md @@ -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: @@ -96,7 +96,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Events"`. A populated example: ```json { @@ -119,7 +119,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Events" }, { "id": "microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md", @@ -132,7 +133,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Events" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-interfaces-review.md b/microsoft/skills/review/al-interfaces-review.md index 65db35e..0031e8f 100644 --- a/microsoft/skills/review/al-interfaces-review.md +++ b/microsoft/skills/review/al-interfaces-review.md @@ -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 = = ;` 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: @@ -85,7 +87,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Interfaces"`. A populated example: ```json { @@ -108,7 +110,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Interfaces" }, { "id": "microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.md", @@ -121,7 +124,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Interfaces" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-performance-review.md b/microsoft/skills/review/al-performance-review.md index 9bd23dc..3262179 100644 --- a/microsoft/skills/review/al-performance-review.md +++ b/microsoft/skills/review/al-performance-review.md @@ -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: @@ -85,7 +88,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Performance"`. A populated example: ```json { @@ -108,7 +111,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/performance/apply-filters-before-iterating.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Performance" }, { "id": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md", @@ -121,7 +125,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/performance/use-setloadfields-for-partial-records.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Performance" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-privacy-review.md b/microsoft/skills/review/al-privacy-review.md index 2cd4005..ea95269 100644 --- a/microsoft/skills/review/al-privacy-review.md +++ b/microsoft/skills/review/al-privacy-review.md @@ -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: @@ -63,7 +68,7 @@ Set `confidence` to: - `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`. - `low` when the finding is an advisory derived only from applicability. -After evaluating each worklist entry, also consider whether the diff exhibits a privacy defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material privacy defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly privacy; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. +This leaf emits only knowledge-backed privacy findings. Do NOT emit reference-less `agent:` findings in this domain: online evaluation shows the privacy agent-finding channel yields almost no accepted findings and a high volume of dismissed noise, so a privacy concern that no worklist knowledge file covers is omitted here rather than emitted with `references: []`. When you spot a material privacy defect no article covers, the durable fix is to add a knowledge article in BCQuality (per the online-eval self-improvement loop) so this leaf can cite it — not a one-off reference-less finding. Before treating a candidate as uncovered, check the worklist for a knowledge file that matches it; if one exists, emit it as a knowledge-backed finding. See `skills/do.md` for the full contract. For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. @@ -79,7 +84,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Privacy"`. A populated example: ```json { @@ -102,7 +107,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/privacy/data-classification-required-on-pii-fields.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Privacy" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-query-review.md b/microsoft/skills/review/al-query-review.md new file mode 100644 index 0000000..c4ea895 --- /dev/null +++ b/microsoft/skills/review/al-query-review.md @@ -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"`. diff --git a/microsoft/skills/review/al-security-review.md b/microsoft/skills/review/al-security-review.md index e6ee82b..12956bd 100644 --- a/microsoft/skills/review/al-security-review.md +++ b/microsoft/skills/review/al-security-review.md @@ -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: @@ -77,7 +84,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Security"`. A populated example: ```json { @@ -100,7 +107,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/security/secrettext-for-credentials.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Security" }, { "id": "microsoft/knowledge/security/secrets-isolated-storage.md", @@ -113,7 +121,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/security/secrets-isolated-storage.md" } ], - "confidence": "medium" + "confidence": "medium", + "domain": "Security" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-style-review.md b/microsoft/skills/review/al-style-review.md index 68f9834..ee4daf5 100644 --- a/microsoft/skills/review/al-style-review.md +++ b/microsoft/skills/review/al-style-review.md @@ -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. @@ -53,7 +60,7 @@ When the post-conflict worklist is empty because no applicable style knowledge e For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Style findings rarely reach `blocker` — reserve it for cases where the knowledge file documents a platform-level requirement (for example, API page property constraints the OData runtime rejects). Most style findings are `minor` or `info`; egregious misuse (`Error` with pre-built Text losing translation and telemetry classification) may reach `major`. -Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, a missing `ToolTip`, required parentheses). On those, BCQuality's value is the *explanation* of why the rule exists, not a second gate; emit them at `info` so a consumer that gates on severity does not re-flag what CodeCop/AppSourceCop already reports. Reserve `minor` for style issues with concrete downstream impact the analyzer does not catch — a `Label` declared at procedure-local instead of object scope (no analyzer enforces label scope, and mis-scoped Labels are fragile in the translation pipeline), lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, a misleading named invocation. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. +Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, variable-declaration order by type AA0021, a missing `ToolTip`, required parentheses). On those, BCQuality's value is the *explanation* of why the rule exists, not a second gate; emit them at `info` so a consumer that gates on severity does not re-flag what CodeCop/AppSourceCop already reports. Reserve `minor` for style issues with concrete downstream impact the analyzer does not catch — a `Label` declared at procedure-local instead of object scope (no analyzer enforces label scope, and mis-scoped Labels are fragile in the translation pipeline), lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, a misleading named invocation. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. Set `confidence` to: @@ -77,7 +84,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Style"`. A populated example: ```json { @@ -99,7 +106,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/style/label-suffix-approved-list.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Style" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-telemetry-review.md b/microsoft/skills/review/al-telemetry-review.md index 3cad1c2..4a758f0 100644 --- a/microsoft/skills/review/al-telemetry-review.md +++ b/microsoft/skills/review/al-telemetry-review.md @@ -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: @@ -77,7 +88,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. The empty-corpus case produces: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Telemetry"`. The empty-corpus case produces: ```json { diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 54aeded..2483f12 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -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: @@ -84,7 +86,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Testing"`. A populated example: ```json { @@ -107,6 +109,7 @@ Output conforms to the DO output contract. A populated example: { "path": "microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md" } ], "confidence": "high", + "domain": "Testing", "suggested-code": "asserterror PostInvalidOrder();\nAssert.ExpectedError(ExpectedPostingErr);" } ], diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index ef22fa3..81ab12e 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -61,7 +61,7 @@ Set `confidence` to: - `medium` when detection relies on heuristics (judging whether a caption is a noun phrase or a sentence phrase) or when any frontmatter dimension was `unknown`. - `low` when the finding is an advisory derived only from applicability. -After evaluating each worklist entry, also consider whether the diff exhibits a UI defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material UI defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly UI; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. +This leaf emits only knowledge-backed UI and accessibility findings. Do NOT emit reference-less `agent:` findings in this domain: online evaluation shows the UI/accessibility agent-finding channel yields almost no accepted findings and a high volume of dismissed noise, so a UI or accessibility concern that no worklist knowledge file covers is omitted here rather than emitted with `references: []`. When you spot a material UI or accessibility defect no article covers, the durable fix is to add a knowledge article in BCQuality (per the online-eval self-improvement loop) so this leaf can cite it — not a one-off reference-less finding. Before treating a candidate as uncovered, check the worklist for a knowledge file that matches it; if one exists, emit it as a knowledge-backed finding. See `skills/do.md` for the full contract. For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. @@ -77,7 +77,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Accessibility"`. A populated example: ```json { @@ -99,7 +99,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/ui/show-caption-on-editable-fields.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Accessibility" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index c087585..879e788 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -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: @@ -80,7 +80,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Upgrade"`. A populated example: ```json { @@ -102,7 +102,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/upgrade/enum-values-additive-at-end.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Upgrade" } ], "suppressed": [] diff --git a/microsoft/skills/review/al-web-services-review.md b/microsoft/skills/review/al-web-services-review.md index 1136a4d..cfa6fdd 100644 --- a/microsoft/skills/review/al-web-services-review.md +++ b/microsoft/skills/review/al-web-services-review.md @@ -54,7 +54,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. For API parts whose parent declares `ODataKeyFields = SystemId`, detect a child foreign key linked to a parent business field instead of `Field(SystemId)`. Do not apply the SystemId-link rule to APIs intentionally keyed by another field. Omitted `Multiplicity` is valid and means the documented default 1:N collection; never report omission alone. Report an explicit `ZeroOrOne` only when the visible contract clearly intends a collection or deep insert, and report an explicit `Many` only when it clearly intends a singleton. Singleton metadata requires an explicit `ZeroOrOne`; do not infer singleton intent from naming alone. For webhook eligibility, detect `QueryType = API`, `SourceTableTemporary = true`, composite `ODataKeyFields` (including an omitted property when a visible source primary key is composite), Job Queue Entry, and visible system-table sources; do not infer an unknown table number. For lifecycle code, require both create and renew paths to use a handler that returns the query-string `validationToken` verbatim with `200 OK`, and flag renewal scheduling that assumes subscriptions are permanent instead of using `expirationDateTime`. Do not emit generic HTTP or REST advice. @@ -80,7 +80,7 @@ Outcome selection: ## Output -Output conforms to the DO output contract. A populated example: +Output conforms to the DO output contract. Every finding this skill emits MUST set `findings[].domain` to `"Web Services"`. A populated example: ```json { @@ -103,7 +103,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/web-services/set-required-api-page-properties.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Web Services" }, { "id": "microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md", @@ -116,7 +117,8 @@ Output conforms to the DO output contract. A populated example: "references": [ { "path": "microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md" } ], - "confidence": "high" + "confidence": "high", + "domain": "Web Services" } ], "suppressed": [] diff --git a/.claude-plugin/plugin.json b/plugin.json similarity index 59% rename from .claude-plugin/plugin.json rename to plugin.json index bacea2a..0934bd3 100644 --- a/.claude-plugin/plugin.json +++ b/plugin.json @@ -5,5 +5,17 @@ "author": { "name": "microsoft/BCQuality", "url": "https://github.com/microsoft/BCQuality" - } + }, + "repository": "https://github.com/microsoft/BCQuality", + "license": "MIT", + "keywords": [ + "bc", + "al", + "business-central", + "code-review", + "quality" + ], + "skills": [ + "./skills/bcquality-al-review/" + ] } diff --git a/skills/bcquality-al-review/SKILL.md b/skills/bcquality-al-review/SKILL.md index 937e06a..32c8207 100644 --- a/skills/bcquality-al-review/SKILL.md +++ b/skills/bcquality-al-review/SKILL.md @@ -24,8 +24,8 @@ Do **not** use this skill to *generate* AL code — it only reviews. ## Plugin root -Resolve `PLUGIN_ROOT` to the directory that contains this plugin's -`.claude-plugin/plugin.json`. This skill lives at +Resolve `PLUGIN_ROOT` to the directory that contains this plugin's root +`plugin.json`. This skill lives at `PLUGIN_ROOT/skills/bcquality-al-review/SKILL.md`, so `PLUGIN_ROOT` is two levels up from this file. All paths below are relative to `PLUGIN_ROOT`. If the host exposes a plugin-root environment variable, prefer it. @@ -63,11 +63,19 @@ plugin-root environment variable, prefer it. `microsoft/skills/review/al-code-review.md`. For each dispatched skill, read the file and execute its Source → Relevance → Worklist → Action steps, reading `PLUGIN_ROOT/skills/read.md` and `PLUGIN_ROOT/skills/do.md` on demand. + When `al-code-review` composes its leaves and the host supports child contexts or + separate model calls, run each leaf in an isolated context and roll up the returned + JSON. Pass each call the exact index rows for that leaf's domain so references can + be copied verbatim. This is the preferred execution profile for fast/small models; + do not force one generation to retain all domain knowledge at once. -4. **Emit findings.** Produce the rolled-up findings report in the DO output contract - (`outcome`, `findings`, `references`, `confidence`, `suppressed`). Do not invent a - different shape; downstream consumers parse the DO contract without skill-specific - logic. +4. **Emit findings.** Produce the rolled-up findings report in the DO output contract, + including each review finding's producer-supplied `domain` label (`outcome`, + `findings`, `references`, `confidence`, `suppressed`). Do not invent a different + shape; downstream consumers parse the DO contract without skill-specific logic. + Apply DO's reference-integrity gate before returning: every knowledge-backed path + must exist in the installed tree, must have been opened in full, and must be copied + verbatim. Never synthesize a plausible article slug. If Entry returns `no-match` or `failed`, return the dispatch record unchanged so the caller can log the reason. @@ -85,7 +93,8 @@ caller can log the reason. `enabled-layers` (`BCQUALITY_ENABLED_LAYERS`) — the denied layers' files still exist on disk. Treat `enabled-layers` as a selection filter, not a hard security boundary. A future revision could add a genuine deny mechanism (e.g. pruning the installed tree). -- **Manifest location.** This plugin uses `.claude-plugin/plugin.json`, which both +- **Manifest location.** This plugin's manifest is the root `plugin.json`, which both Claude Code and Copilot CLI accept (verified with Copilot CLI: `plugin install` - reports the bridge skill loaded). Copilot CLI also accepts a root `plugin.json`; if a - future host only reads the root form, dual-home the manifest. + reports the bridge skill loaded). A `.claude-plugin/marketplace.json` alongside it + carries the marketplace entry. Claude Code also reads `.claude-plugin/plugin.json`; if + a future host only reads that form, dual-home the manifest there. diff --git a/skills/do.md b/skills/do.md index 777f89f..23bfb4a 100644 --- a/skills/do.md +++ b/skills/do.md @@ -21,6 +21,19 @@ An action skill is a single markdown file with YAML frontmatter. It lives inside Action skills do not live at the repo root. The files in `/skills/` — the three meta-skill contracts (READ, DO, WRITE) and the entry-point skill (`entry.md`, `kind: entry-point`) — are the only skills that sit outside a layer. The entry-point skill structurally follows this same four-step pattern but produces a dispatch record rather than a findings-report; see `skills/entry.md` for its contract. +## Skills hold mechanics; knowledge files hold BC facts + +An action skill is a *finder and applier*: its prose says how to discover candidate knowledge (Source), filter it (Relevance), narrow it to the task (Worklist), and shape output (Action). Every Business-Central-specific behavioural claim a skill acts on — what a property defaults to, what a trigger does, why a given shape is or is not a defect — belongs in a knowledge file the skill cites, not in the skill prose. + +This includes **negative knowledge**. A false-positive guard — "pattern X is not a defect, because BC does Y" — is as much a knowledge file as a positive best practice. When an eval shows the agent over-reporting a pattern, the fix is a knowledge file documenting why the pattern is legitimate, so the skill can cite it and any leaf can reuse it — not a hard-coded exclusion buried in one skill. See `skills/write.md` (*Is this a knowledge file?*). + +Two rules follow for skill authors: + +- **Do not add a BC fact to a skill.** If you are editing a skill to change *what it flags* — adding an exclusion, encoding a platform default, teaching it that some pattern is fine — you are holding a knowledge file, not a skill edit. Author the knowledge file and let Worklist route to it. +- **Do not restate an article's fact inline.** A Worklist cue may name the article to load and the diff shape that selects it; it must not re-assert the article's reasoning, which then drifts from the source. Cite, don't copy. + +The meta-skills themselves (`read.md`, `do.md`, `write.md`) are domain-agnostic templates and carry no BC-specific rule. + ## Frontmatter schema ```yaml @@ -95,6 +108,7 @@ Every action skill emits a single JSON document that conforms to this schema: ], "confidence": "high | medium | low", "from-sub-skill": "string", + "domain": "string", "suggested-code": "string", "suggested-code-omission-reason": "string" } @@ -170,6 +184,8 @@ Consumers that render output MAY treat agent findings differently from knowledge **`findings[].message`** — human-readable explanation of the finding. Single short paragraph. No markdown formatting assumptions. +**Applicability is not a finding.** Loading an article into the worklist only means its rule must be evaluated. If the changed code does not violate the article's normative guidance, emit nothing for that article. An `info` finding still requires a concrete observation defined by the article; skills MUST NOT use `info` to list guidance that merely happened to be relevant. + **`findings[].location`** — optional. When present: - `file` MUST be a repo-relative path using forward slashes. @@ -185,10 +201,23 @@ Findings without a `location` are permitted (for example, repository-wide observ The first reference is the **primary** reference: the knowledge file the finding most directly cites. Additional references provide supporting context and are not ranked. `references` MAY be empty only for **agent findings** (see the `findings[].id` section above for the full encoding); any other finding MUST have at least one reference. +**Reference-integrity gate (mandatory).** A knowledge-backed finding may cite only a path copied verbatim from the current knowledge index or from a file discovered by the index fallback, and the skill must have opened that exact file in full before citing it. Never construct a plausible slug or infer a path from a topic name. Immediately before emitting the JSON document: + +1. Verify every non-empty `references[].path` exists in the live checkout and was opened during this skill run. +2. Verify every citation-based `findings[].id` exactly equals `references[0].path`. +3. Remove any candidate that cannot satisfy both checks; it is not a knowledge-backed finding. Do not convert it into an agent finding merely to preserve it. +4. If reference integrity cannot be checked reliably, return `outcome: "failed"` rather than emitting fabricated or unverified citations. + +This gate applies independently to every leaf result and again to a super-skill's rolled-up result. + **`findings[].confidence`** — the skill's confidence that the finding is a true positive, given the evidence it evaluated. Not applicability confidence, not severity confidence. Values: `high`, `medium`, `low`. **`findings[].from-sub-skill`** — optional. Set only by super-skills. The `skill.id` of the sub-skill that produced the finding, or the literal string `"agent"` for an agent finding the super-skill produced from its own cross-cutting reasoning. Absent on findings emitted directly by a leaf skill — including agent findings the leaf emits within its own domain, which appear in the leaf's own report without this field. +**`findings[].domain`** — optional in the shared schema for backward compatibility and for non-review findings. It is a short, human-readable display label for the review domain that produced the finding (for example, `Security`, `Breaking Changes`, `API & Web Services`). A review leaf skill MUST set it on every finding it emits. The value MUST be a non-empty, single-line string with no leading or trailing whitespace or control characters. Internal whitespace, punctuation, case, and non-ASCII characters are valid and significant. + +A review super-skill MUST preserve `domain` verbatim when rolling a leaf finding into its top-level `findings[]`, including preserving its absence from older producers, and MUST set it to `"Agent"` for agent findings it emits about cross-cutting concerns. Consumers MUST tolerate its absence. When rendering a present value, consumers MUST preserve the complete display text, escaping only as required by the output format; they MUST NOT split it on whitespace or restrict it to identifier characters. `domain` is display text, not a stable machine identifier. If a consumer embeds it in metadata or uses it in a deduplication key, it MUST retain the exact string, use a lossless encoding, or use a collision-resistant digest; it MUST NOT rely on lowercasing or lossy slugification as the sole identity. + **`findings[].suggested-code`** — optional in the schema but **expected for mechanical findings**. It is a concrete code-replacement payload for the lines indicated by `location`. When present, the string MUST be a literal replacement for the source lines covered by `location.line` (or `location.range` if set) — i.e., what the file would contain after the fix, with no surrounding diff markers, fences, or commentary. Consumers MAY render it as a one-click suggestion in the delivery surface (for example, a GitHub ```` ```suggestion ```` block). Emit `suggested-code` whenever the fix is small, local, and mechanical: deleting unreachable code; replacing one expression (`Count() > 0` → `not IsEmpty()`); moving a local `Label` to object scope; adding a missing property such as `ToolTip`, `OptionCaption`, or `DataClassification`; replacing a string-concatenated `Error` with a Label-backed call; changing a permission token; or adding a missing `else`/guard branch whose replacement is unambiguous from the surrounding diff. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, prefer adapting the `.good.al` replacement into `suggested-code`. @@ -226,7 +255,7 @@ The five required sections still apply. Their meaning shifts from knowledge file - `## Source` — names the sub-skills invoked (mirrors `sub-skills` in frontmatter). - `## Relevance` — rules for deciding which sub-skills apply to the current task. A sub-skill is relevant when its declared `inputs` are satisfied by the orchestrator's provided inputs and the orchestrator has not disabled it via configuration. The super-skill MUST NOT filter sub-skills by task content (for example, by inspecting the diff or the file). Task-level applicability is the sub-skill's own responsibility; sub-skills signal non-applicability by returning `outcome: "not-applicable"` or `outcome: "no-knowledge"`. - `## Worklist` — the final list of sub-skills to invoke; the rest go to `skipped-sub-skills`. -- `## Action` — invoke each worklisted sub-skill with the appropriate subset of inputs, collect its findings-report verbatim into `sub-results`, and copy its `findings[]` into the super-skill's top-level `findings[]` with `from-sub-skill` set. Findings from a sub-skill with `outcome: "failed"` MUST NOT be copied into the super-skill's top-level `findings[]` and MUST NOT contribute to the super-skill's `summary.counts` (their report is still preserved in `sub-results` for traceability, consistent with DO's rule that consumers ignore a failed skill's findings). +- `## Action` — invoke each worklisted sub-skill with the appropriate subset of inputs, collect its findings-report verbatim into `sub-results`, and copy its `findings[]` into the super-skill's top-level `findings[]` with `from-sub-skill` set. All finding fields, including the optional `domain`, are preserved verbatim unless this contract explicitly requires a transformation. Findings from a sub-skill with `outcome: "failed"` MUST NOT be copied into the super-skill's top-level `findings[]` and MUST NOT contribute to the super-skill's `summary.counts` (their report is still preserved in `sub-results` for traceability, consistent with DO's rule that consumers ignore a failed skill's findings). - `## Output` — the super-skill's output contract, including `sub-results` and, if any, `skipped-sub-skills`. ### Outcome rollup @@ -288,5 +317,3 @@ Conforms to the DO output contract. ## How orchestrators consume output An orchestrator invokes an action skill with an input appropriate to the skill's declared `inputs`, receives the JSON output, and maps findings to its delivery surface (PR comments, build gates, IDE diagnostics). The orchestrator MUST NOT interpret skill-specific fields beyond the schema above. Skills that need richer semantics MUST encode them within the schema (for example, by adding structured `message` text) rather than extending the output shape. - - diff --git a/skills/write.md b/skills/write.md index 754fe1e..731f469 100644 --- a/skills/write.md +++ b/skills/write.md @@ -9,6 +9,25 @@ title: New Knowledge — how to author a knowledge file Anyone — human or agent — adding a knowledge file to BCQuality follows this guide. READ is the format specification; WRITE is the authoring guide. This file does not restate the schema; consult READ for field-by-field semantics. +## Is this a knowledge file? + +Before authoring anything, confirm a knowledge file is the right artifact. BCQuality separates *mechanics* from *facts*: + +- **Skills** (`*/skills/**`) hold only finder/applier mechanics — how to discover, filter, worklist, and emit findings. See `skills/do.md`. +- **Knowledge files** (`*/knowledge/**`) hold every Business-Central-specific fact a skill acts on. + +A new BC fact is therefore a knowledge file, never a skill edit. In particular, if you arrived here because a review agent flagged something it should not have (a false positive) or missed something it should have caught, the remedy is a knowledge file — apply the admission test in the [README](../README.md#what-belongs-here): *would a capable LLM get this wrong without the file?* If you find yourself editing a skill to stop it flagging something, stop and write a knowledge file instead. + +### Negative knowledge is first-class + +A knowledge file does not have to recommend an action. A **negative clarification** — "pattern X is *not* a defect, because BC behaves as Y" — is a first-class knowledge file, authored exactly like a positive rule: + +- **Description** states the BC behaviour that makes the pattern legitimate. +- **Best Practice** tells the reviewer or agent what *not* to flag, and why. +- **Anti Pattern** describes the false-positive report itself — the mistaken finding to suppress. + +For example, `microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md` records that the Boolean page record triggers return `true` by default, so a "missing `exit(true)`" report is not a real defect. It reads as ordinary knowledge; its anti-pattern is the incorrect review comment, not the code. + ## Before you start Read `skills/read.md` first. A file that does not conform to READ will be rejected. WRITE assumes READ is already understood. @@ -85,7 +104,10 @@ Before opening a pull request: - No fenced code blocks. - File is under 100 lines. - File covers one concern. +- Frontmatter `domain` exactly matches the containing domain folder. - File is in the correct layer and domain folder. - Name is kebab-case and descriptive. +- Every companion sample is referenced by filename from the article, and every referenced sample exists. +- Every review-leaf domain has at least one article with both `.good.al` and `.bad.al` companions; the evaluation harness derives positive and clean controls from that convention automatically. Agents scaffolding new files SHOULD run this checklist programmatically before emitting the file. diff --git a/tools/Test-ReviewFixtures.ps1 b/tools/Test-ReviewFixtures.ps1 new file mode 100644 index 0000000..3f3f144 --- /dev/null +++ b/tools/Test-ReviewFixtures.ps1 @@ -0,0 +1,483 @@ +<# +.SYNOPSIS + Validates and prepares the BCQuality AL review evaluation corpus. + +.DESCRIPTION + CI uses the static validation path to prove every registered AL review leaf + has one positive and one clean control, every fixture/reference exists, and + the manifest remains internally consistent. + + For an actual model run, -PrepareDirectory copies inputs to neutral names and + emits review-request.json without expected answers. After the model writes a + result matching evaluation/README.md, -ResultsPath scores exact knowledge-ID + recall, clean-control rate, and unexpected findings. +#> +[CmdletBinding()] +param( + [string] $Root = (Resolve-Path (Join-Path $PSScriptRoot '..')), + [string] $ManifestPath, + [string] $PrepareDirectory, + [string] $ResultsPath, + [string] $ResultsDirectory +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$Root = (Resolve-Path -LiteralPath $Root).Path +if ($ResultsPath -and $ResultsDirectory) { + throw 'Specify either ResultsPath or ResultsDirectory, not both.' +} +if (-not $ManifestPath) { + $ManifestPath = Join-Path $Root 'evaluation/review-fixtures.json' +} +if (-not (Test-Path -LiteralPath $ManifestPath)) { + throw "Review fixture manifest not found: $ManifestPath" +} + +$manifest = Get-Content -LiteralPath $ManifestPath -Raw | ConvertFrom-Json +$problems = [System.Collections.Generic.List[string]]::new() + +function Get-ModelCaseId { + param([string] $ManifestId) + + $sha = [System.Security.Cryptography.SHA256]::Create() + try { + $bytes = [System.Text.Encoding]::UTF8.GetBytes($ManifestId) + $hash = $sha.ComputeHash($bytes) + $token = ([System.BitConverter]::ToString($hash) -replace '-', '').Substring(0, 8).ToLowerInvariant() + return "case-$token" + } finally { + $sha.Dispose() + } +} + +function Get-RankedArticles { + param( + [object[]] $Articles, + [string] $CaseText, + [int] $Limit = 10 + ) + + if ($Articles.Count -le $Limit) { + return @($Articles) + } + + $normalized = (($CaseText.ToLowerInvariant() -replace '[^a-z0-9]+', ' ') -replace '\s+', ' ').Trim() + $compact = $normalized -replace ' ', '' + $ranked = foreach ($article in $Articles) { + $score = 0 + foreach ($keyword in @($article.keywords)) { + $keywordText = ([string]$keyword).ToLowerInvariant() + $keywordCompact = $keywordText -replace '[^a-z0-9]+', '' + if ($keywordCompact -and $compact.Contains($keywordCompact)) { + $score += 8 + } + foreach ($part in @($keywordText -split '[^a-z0-9]+')) { + if (($part.Length -ge 4) -and ($normalized -match "(^| )$([regex]::Escape($part))( |$)")) { + $score += 1 + } + } + } + $topicText = "$($article.title) $($article.description) $($article.path)".ToLowerInvariant() + foreach ($term in @($normalized -split ' ' | Where-Object Length -ge 5 | Sort-Object -Unique)) { + if ($topicText.Contains($term)) { + $score += 0.25 + } + } + [pscustomobject]@{ score = $score; path = [string]$article.path; article = $article } + } + + return @( + $ranked | + Sort-Object @{ Expression = 'score'; Descending = $true }, @{ Expression = 'path'; Descending = $false } | + Select-Object -First $Limit | + ForEach-Object article + ) +} + +if ($manifest.version -ne 2) { + $problems.Add("Unsupported manifest version: $($manifest.version)") | Out-Null +} +if ($manifest.selection -ne 'first-paired-al-article') { + $problems.Add("Unsupported selection strategy: $($manifest.selection)") | Out-Null +} +if (([double]$manifest.minimumExpectedRecall -lt 0) -or ([double]$manifest.minimumExpectedRecall -gt 1)) { + $problems.Add('minimumExpectedRecall must be between 0 and 1.') | Out-Null +} +if (([double]$manifest.minimumCleanRate -lt 0) -or ([double]$manifest.minimumCleanRate -gt 1)) { + $problems.Add('minimumCleanRate must be between 0 and 1.') | Out-Null +} + +$leafDomains = @( + Get-ChildItem -LiteralPath (Join-Path $Root 'microsoft/skills/review') -File -Filter 'al-*-review.md' | + Where-Object Name -ne 'al-code-review.md' | + ForEach-Object { $_.BaseName -replace '^al-', '' -replace '-review$', '' } | + Sort-Object -Unique +) + +$overrides = @{} +if ($manifest.PSObject.Properties.Name -contains 'overrides') { + foreach ($property in $manifest.overrides.PSObject.Properties) { + $overrides[$property.Name] = $property.Value + } +} +foreach ($overrideDomain in $overrides.Keys) { + if ($leafDomains -notcontains $overrideDomain) { + $problems.Add("Override domain '$overrideDomain' has no registered al-$overrideDomain-review leaf.") | Out-Null + } +} + +$caseList = [System.Collections.Generic.List[object]]::new() +foreach ($domain in $leafDomains) { + $knowledgeDirectory = Join-Path $Root "microsoft/knowledge/$domain" + if (-not (Test-Path -LiteralPath $knowledgeDirectory -PathType Container)) { + $problems.Add("${domain}: no Microsoft knowledge directory exists.") | Out-Null + continue + } + + $override = if ($overrides.ContainsKey($domain)) { $overrides[$domain] } else { $null } + $selectedArticle = $null + if ($override -and ($override.PSObject.Properties.Name -contains 'article')) { + $articleName = [string]$override.article + if ($articleName.EndsWith('.md')) { + $articleName = [System.IO.Path]::GetFileNameWithoutExtension($articleName) + } + $candidate = Join-Path $knowledgeDirectory "$articleName.md" + if (Test-Path -LiteralPath $candidate -PathType Leaf) { + $selectedArticle = Get-Item -LiteralPath $candidate + } else { + $problems.Add("${domain}: override article does not exist: $articleName.md") | Out-Null + } + } else { + $selectedArticle = Get-ChildItem -LiteralPath $knowledgeDirectory -File -Filter '*.md' | + Sort-Object Name | + Where-Object { + (Test-Path -LiteralPath (Join-Path $knowledgeDirectory "$($_.BaseName).good.al") -PathType Leaf) -and + (Test-Path -LiteralPath (Join-Path $knowledgeDirectory "$($_.BaseName).bad.al") -PathType Leaf) + } | + Select-Object -First 1 + } + if (-not $selectedArticle) { + $problems.Add("${domain}: no article has both .good.al and .bad.al companion samples.") | Out-Null + continue + } + + $articlePath = "microsoft/knowledge/$domain/$($selectedArticle.Name)" + $context = if ($override -and ($override.PSObject.Properties.Name -contains 'context')) { + [string]$override.context + } else { + $null + } + foreach ($kind in 'bad', 'good') { + $case = [pscustomobject]@{ + id = "$domain-$kind" + domain = $domain + input = "microsoft/knowledge/$domain/$($selectedArticle.BaseName).$kind.al" + expected = if ($kind -eq 'bad') { @($articlePath) } else { @() } + } + if ($context) { + $case | Add-Member -NotePropertyName context -NotePropertyValue $context + } + $caseList.Add($case) | Out-Null + } +} +$cases = @($caseList) + +$seenIds = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::Ordinal) +foreach ($case in $cases) { + $id = [string]$case.id + $domain = [string]$case.domain + $input = [string]$case.input + $expected = @($case.expected) + + if ([string]::IsNullOrWhiteSpace($id)) { + $problems.Add('Case with empty id.') | Out-Null + } elseif (-not $seenIds.Add($id)) { + $problems.Add("Duplicate case id: $id") | Out-Null + } + if ($leafDomains -notcontains $domain) { + $problems.Add("${id}: domain '$domain' has no registered al-$domain-review leaf.") | Out-Null + } + + $inputPath = Join-Path $Root $input + if (-not (Test-Path -LiteralPath $inputPath -PathType Leaf)) { + $problems.Add("${id}: input does not exist: $input") | Out-Null + } + if ($expected.Count -and $input -notmatch '\.bad\.[^.]+$') { + $problems.Add("${id}: positive case must use a .bad sample: $input") | Out-Null + } + if (-not $expected.Count -and $input -notmatch '\.good\.[^.]+$') { + $problems.Add("${id}: clean case must use a .good sample: $input") | Out-Null + } + + foreach ($reference in $expected) { + $referencePath = Join-Path $Root ([string]$reference) + if (-not (Test-Path -LiteralPath $referencePath -PathType Leaf)) { + $problems.Add("${id}: referenced article does not exist: $reference") | Out-Null + } + } + if ($expected.Count) { + $sampleSlug = ([System.IO.Path]::GetFileName($input) -replace '\.(?:good|bad)\.[^.]+$', '') + $primarySlug = [System.IO.Path]::GetFileNameWithoutExtension([string]$expected[0]) + if ($sampleSlug -ne $primarySlug) { + $problems.Add("${id}: primary expected article '$primarySlug' must match sample slug '$sampleSlug'.") | Out-Null + } + } +} + +foreach ($domain in $leafDomains) { + $domainCases = @($cases | Where-Object domain -eq $domain) + if (-not @($domainCases | Where-Object { @($_.expected).Count -gt 0 }).Count) { + $problems.Add("${domain}: no positive review fixture.") | Out-Null + } + if (-not @($domainCases | Where-Object { @($_.expected).Count -eq 0 }).Count) { + $problems.Add("${domain}: no clean control fixture.") | Out-Null + } +} + +if ($problems.Count) { + Write-Host "Review fixture validation FAILED ($($problems.Count) problem(s)):" -ForegroundColor Red + $problems | ForEach-Object { Write-Host " - $_" -ForegroundColor Red } + exit 1 +} + +if ($PrepareDirectory) { + $markerPath = Join-Path $PrepareDirectory '.bcquality-evaluation' + if (Test-Path -LiteralPath $PrepareDirectory) { + $existing = @(Get-ChildItem -LiteralPath $PrepareDirectory -Force) + if ($existing.Count -and -not (Test-Path -LiteralPath $markerPath -PathType Leaf)) { + throw "PrepareDirectory is not empty and is not a BCQuality evaluation directory: $PrepareDirectory" + } + if (Test-Path -LiteralPath $markerPath -PathType Leaf) { + Get-ChildItem -LiteralPath $PrepareDirectory -File | + Where-Object { + ($_.Name -like 'case*.al') -or + ($_.Name -eq 'review-request.json') -or + ($_.Name -like 'request-*.json') -or + ($_.Name -eq 'knowledge-index.json') -or + ($_.Name -like 'index-*.json') -or + ($_.Name -like 'result-*.json') + } | + Remove-Item -Force + } + } else { + New-Item -ItemType Directory -Force -Path $PrepareDirectory | Out-Null + } + Set-Content -LiteralPath $markerPath -Value 'BCQuality generated evaluation directory' -Encoding UTF8 + + $fullIndexPath = Join-Path $PrepareDirectory 'knowledge-index.json' + & (Join-Path $Root 'tools/Build-KnowledgeIndex.ps1') -BCQualityRoot $Root -IndexPath $fullIndexPath | Out-Null + $fullIndex = Get-Content -LiteralPath $fullIndexPath -Raw | ConvertFrom-Json + + $requestCases = [System.Collections.Generic.List[object]]::new() + $requestCasesByDomain = @{} + $manifestCaseByModelId = @{} + foreach ($case in $cases) { + $extension = [System.IO.Path]::GetExtension([string]$case.input) + $modelId = Get-ModelCaseId -ManifestId ([string]$case.id) + $neutralName = "$modelId$extension" + $sourceText = Get-Content -LiteralPath (Join-Path $Root ([string]$case.input)) -Raw + # Companion samples are human-facing and often label objects/comments as + # Good, Bad, or Anti-pattern. Strip full-line comments and neutralize those + # object-name tokens so model-facing fixtures do not reveal the expected + # outcome while preserving executable AL structure and references. + $neutralText = [regex]::Replace($sourceText, '(?m)^\s*//.*(?:\r?\n|$)', '') + $neutralText = [regex]::Replace($neutralText, '\b(?:Good|Bad)\b', 'Eval') + Set-Content -LiteralPath (Join-Path $PrepareDirectory $neutralName) -Value $neutralText -Encoding UTF8 + $requestCase = [pscustomobject]@{ id = $modelId; file = $neutralName } + if ($case.PSObject.Properties.Name -contains 'context') { + $requestCase | Add-Member -NotePropertyName context -NotePropertyValue ([string]$case.context) + } + $requestCases.Add($requestCase) | Out-Null + $manifestCaseByModelId[$modelId] = $case + $domain = [string]$case.domain + if (-not $requestCasesByDomain.ContainsKey($domain)) { + $requestCasesByDomain[$domain] = [System.Collections.Generic.List[object]]::new() + } + $requestCasesByDomain[$domain].Add($requestCase) | Out-Null + } + $resultSchema = [pscustomobject]@{ + cases = @([pscustomobject]@{ + id = 'case-id' + findings = @([pscustomobject]@{ id = 'repo-relative knowledge article path' }) + }) + } + [pscustomobject]@{ + protocol = 'Run BCQuality al-code-review over all files as one PR; return findings per case. Copy every knowledge-backed id from knowledge-index.json.' + knowledgeIndex = 'knowledge-index.json' + resultSchema = $resultSchema + cases = @($requestCases) + } | ConvertTo-Json -Depth 8 | Set-Content -LiteralPath (Join-Path $PrepareDirectory 'review-request.json') -Encoding UTF8 + + foreach ($domain in $leafDomains) { + $domainArticles = @($fullIndex.articles | Where-Object domain -eq $domain) + $domainIndexName = "index-$domain.json" + $leafPath = "microsoft/skills/review/al-$domain-review.md" + $leafFullText = Get-Content -LiteralPath (Join-Path $Root $leafPath) -Raw + $leafInstructions = @($leafFullText -split '(?m)^## Output\s*\r?\n', 2)[0] + $leafInstructions += "`n## Output`nReturn only the request's resultSchema." + [pscustomobject]@{ + version = $fullIndex.version + domain = $domain + articleCount = $domainArticles.Count + articles = $domainArticles + } | ConvertTo-Json -Depth 8 | Set-Content -LiteralPath (Join-Path $PrepareDirectory $domainIndexName) -Encoding UTF8 + + [pscustomobject]@{ + protocol = "Run only $leafPath over these files. Follow leafInstructions exactly, use only the supplied candidate article rows, open matching articles in full, and copy every finding id verbatim from candidateArticles[].path." + skill = $leafPath + leafInstructions = $leafInstructions + knowledgeIndex = $domainIndexName + candidateArticles = $domainArticles + resultSchema = $resultSchema + cases = @($requestCasesByDomain[$domain]) + } | ConvertTo-Json -Depth 8 | Set-Content -LiteralPath (Join-Path $PrepareDirectory "request-$domain.json") -Encoding UTF8 + + foreach ($requestCase in @($requestCasesByDomain[$domain])) { + $caseText = Get-Content -LiteralPath (Join-Path $PrepareDirectory ([string]$requestCase.file)) -Raw + if ($requestCase.PSObject.Properties.Name -contains 'context') { + $caseText += " $([string]$requestCase.context)" + } + $rankedArticles = @(Get-RankedArticles -Articles $domainArticles -CaseText $caseText) + $manifestCase = $manifestCaseByModelId[[string]$requestCase.id] + $selectedArticlePath = ([string]$manifestCase.input) -replace '\.(?:good|bad)\.al$', '.md' + $rankedPaths = @($rankedArticles | ForEach-Object { [string]$_.path }) + if ($rankedPaths -notcontains $selectedArticlePath) { + throw "$($manifestCase.id): deterministic ranking omitted selected article '$selectedArticlePath'. Improve its retrieval metadata or choose an exceptional override article." + } + # Candidate order must not reveal which article owns the fixture. + $rankedArticles = @($rankedArticles | Sort-Object path) + [pscustomobject]@{ + protocol = "Run only $leafPath over this case. Follow leafInstructions exactly, evaluate the ranked candidate article rows, open matching articles in full, and copy every finding id verbatim from candidateArticles[].path." + skill = $leafPath + leafInstructions = $leafInstructions + knowledgeIndex = $domainIndexName + candidateArticles = $rankedArticles + resultSchema = $resultSchema + cases = @($requestCase) + } | ConvertTo-Json -Depth 8 | Set-Content -LiteralPath (Join-Path $PrepareDirectory "request-$($requestCase.id).json") -Encoding UTF8 + } + } + Write-Host "Prepared $($cases.Count) neutral fixture(s) in $PrepareDirectory." -ForegroundColor Green +} + +if (-not $ResultsPath -and -not $ResultsDirectory) { + Write-Host "Review fixture validation PASSED: $($cases.Count) cases cover $($leafDomains.Count) leaf domains." -ForegroundColor Green + exit 0 +} + +$resultCases = [System.Collections.Generic.List[object]]::new() +if ($ResultsDirectory) { + if (-not (Test-Path -LiteralPath $ResultsDirectory -PathType Container)) { + throw "Results directory not found: $ResultsDirectory" + } + $resultFiles = @(Get-ChildItem -LiteralPath $ResultsDirectory -File -Filter 'result-case-*.json') + if (-not $resultFiles.Count) { + $resultFiles = @(Get-ChildItem -LiteralPath $ResultsDirectory -File -Filter 'result-*.json') + } + if (-not $resultFiles.Count) { + throw "No result-case-*.json or result-*.json files found in: $ResultsDirectory" + } + foreach ($resultFile in $resultFiles) { + try { + $resultDocument = Get-Content -LiteralPath $resultFile.FullName -Raw | ConvertFrom-Json + } catch { + $problems.Add("$($resultFile.Name): invalid JSON: $($_.Exception.Message)") | Out-Null + continue + } + if ($resultDocument.PSObject.Properties.Name -notcontains 'cases') { + $problems.Add("$($resultFile.Name): result must contain a 'cases' array.") | Out-Null + continue + } + foreach ($resultCase in @($resultDocument.cases)) { + $resultCases.Add($resultCase) | Out-Null + } + } +} else { + if (-not (Test-Path -LiteralPath $ResultsPath -PathType Leaf)) { + throw "Results file not found: $ResultsPath" + } + $resultDocument = Get-Content -LiteralPath $ResultsPath -Raw | ConvertFrom-Json + foreach ($resultCase in @($resultDocument.cases)) { + $resultCases.Add($resultCase) | Out-Null + } +} + +$resultById = @{} +$modelToManifestId = @{} +foreach ($case in $cases) { + $manifestId = [string]$case.id + $modelToManifestId[(Get-ModelCaseId -ManifestId $manifestId)] = $manifestId + # Also accept manifest IDs for maintainers generating local oracle results. + $modelToManifestId[$manifestId] = $manifestId +} +foreach ($resultCase in @($resultCases)) { + $rawResultId = [string]$resultCase.id + if (-not $modelToManifestId.ContainsKey($rawResultId)) { + $problems.Add("Results contain unknown case id: $rawResultId") | Out-Null + continue + } + $resultId = $modelToManifestId[$rawResultId] + if ($resultById.ContainsKey($resultId)) { + $problems.Add("Results contain duplicate case id: $rawResultId") | Out-Null + } else { + $resultById[$resultId] = $resultCase + } +} + +$positiveTotal = 0 +$positivePassed = 0 +$cleanTotal = 0 +$cleanPassed = 0 +foreach ($case in $cases) { + $id = [string]$case.id + if (-not $resultById.ContainsKey($id)) { + $problems.Add("Results missing case: $id") | Out-Null + continue + } + + $findingIds = @( + @($resultById[$id].findings) | ForEach-Object { + if ($_ -is [string]) { [string]$_ } else { [string]$_.id } + } | Where-Object { -not [string]::IsNullOrWhiteSpace($_) } | Sort-Object -Unique + ) + $expected = @($case.expected | ForEach-Object { [string]$_ }) + + if ($expected.Count) { + $positiveTotal++ + $missing = @($expected | Where-Object { $findingIds -notcontains $_ }) + $unexpected = @($findingIds | Where-Object { $expected -notcontains $_ }) + if (-not $missing.Count -and -not $unexpected.Count) { + $positivePassed++ + } else { + if ($missing.Count) { $problems.Add("${id}: missing expected finding(s): $($missing -join ', ')") | Out-Null } + if ($unexpected.Count) { $problems.Add("${id}: unexpected finding(s): $($unexpected -join ', ')") | Out-Null } + } + } else { + $cleanTotal++ + if (-not $findingIds.Count) { + $cleanPassed++ + } else { + $problems.Add("${id}: clean control produced finding(s): $($findingIds -join ', ')") | Out-Null + } + } +} + +$recall = if ($positiveTotal) { $positivePassed / $positiveTotal } else { 0 } +$cleanRate = if ($cleanTotal) { $cleanPassed / $cleanTotal } else { 0 } +if ($recall -lt [double]$manifest.minimumExpectedRecall) { + $problems.Add("Expected-finding recall $recall is below $($manifest.minimumExpectedRecall).") | Out-Null +} +if ($cleanRate -lt [double]$manifest.minimumCleanRate) { + $problems.Add("Clean-control rate $cleanRate is below $($manifest.minimumCleanRate).") | Out-Null +} + +if ($problems.Count) { + Write-Host "Review evaluation FAILED ($($problems.Count) problem(s)):" -ForegroundColor Red + $problems | ForEach-Object { Write-Host " - $_" -ForegroundColor Red } + exit 1 +} + +Write-Host "Review evaluation PASSED: recall=$recall ($positivePassed/$positiveTotal), clean-rate=$cleanRate ($cleanPassed/$cleanTotal)." -ForegroundColor Green +exit 0