diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 1aa47c6..752ae8d 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -8,10 +8,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", + "description": "Business Central AL quality knowledge base and review skills, packaged as an installable plugin. Exposes an AL review adapter while preserving BCQuality's internal Entry and action-skill protocols.", + "version": "0.2.0", "skills": [ - "./skills/bcquality-al-review/" + "./skills/" ] } ] diff --git a/.github/scripts/validate_frontmatter.py b/.github/scripts/validate_frontmatter.py index dd3ef4e..b0076cc 100644 --- a/.github/scripts/validate_frontmatter.py +++ b/.github/scripts/validate_frontmatter.py @@ -42,6 +42,7 @@ ACTION_SKILL_OPTIONAL_KEYS = { } META_SKILL_REQUIRED_KEYS = {"kind", "id", "version", "title"} ENTRY_SKILL_REQUIRED_KEYS = {"kind", "id", "version", "title"} +HOST_SKILL_REQUIRED_KEYS = {"name", "description"} STANDARD_INPUTS = { "pr-diff", "object-list", "file-path", "repository", "telemetry-query", @@ -444,10 +445,36 @@ def validate_entry_skill(path: Path, parsed: Parsed, report: Report) -> None: report.error(path, "R23", f"version must be a positive integer: {v!r}", 1) +def validate_host_skill(path: Path, parsed: Parsed, report: Report) -> None: + if parsed.frontmatter_error: + report.error(path, "R01", parsed.frontmatter_error, 1) + return + fm = parsed.frontmatter + assert fm is not None + missing = HOST_SKILL_REQUIRED_KEYS - fm.keys() + if missing: + report.error(path, "R29", f"missing required host-skill keys: {sorted(missing)}", 1) + + name = fm.get("name") + if not isinstance(name, str) or not name: + report.error(path, "R29", "host-skill name must be a non-empty string", 1) + else: + if len(name) > 64 or not KEBAB_CASE.fullmatch(name): + report.error(path, "R29", f"host-skill name must be lowercase kebab-case and at most 64 characters: '{name}'", 1) + if name != path.parent.name: + report.error(path, "R29", f"host-skill name must match parent directory '{path.parent.name}', got '{name}'", 1) + + description = fm.get("description") + if not isinstance(description, str) or not description: + report.error(path, "R29", "host-skill description must be a non-empty string", 1) + elif len(description) > 1024: + report.error(path, "R29", "host-skill description must be at most 1024 characters", 1) + + # --- Path and sample checks ------------------------------------------------- def classify(path_from_root: Path) -> str | None: - """Return 'knowledge' | 'action-skill' | 'meta' | 'entry' | None.""" + """Return 'knowledge' | 'action-skill' | 'host-skill' | 'meta' | 'entry' | None.""" parts = path_from_root.parts if len(parts) < 2: return None @@ -459,6 +486,8 @@ def classify(path_from_root: Path) -> str | None: return "entry" if name in META_SKILL_FILES: return "meta" + if len(parts) == 3 and parts[2] == "SKILL.md": + return "host-skill" return None if top in LAYERS and path_from_root.suffix == ".md": if len(parts) >= 3 and parts[1] == "skills": @@ -616,6 +645,8 @@ def run(root: Path) -> Report: validate_entry_skill(path, parsed, report) if parsed.frontmatter and isinstance(parsed.frontmatter.get("id"), str): skill_records.append(SkillRecord(path, "entry-point", parsed.frontmatter["id"])) + elif kind == "host-skill": + validate_host_skill(path, parsed, report) # Second pass: sample files per knowledge domain for layer in LAYERS: diff --git a/README.md b/README.md index b939c6f..43032d4 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,7 @@ BCQuality contains **knowledge** and **skills**. It does not contain agents. Age ### Knowledge files -Atomic markdown files with YAML frontmatter. Each file covers one concern — one thing an agent would cite when reviewing or generating code. Knowledge files live in two layers: +Atomic markdown files with YAML frontmatter. Each file covers one concern — one thing an agent would cite when reviewing or generating code. Knowledge files live in three layers: - **`/microsoft/`** — Microsoft-endorsed layer. - `/microsoft/knowledge/` — Platform guardrails, official guidance. @@ -39,7 +39,9 @@ Atomic markdown files with YAML frontmatter. Each file covers one concern — on - `/custom/knowledge/` — Organization-specific knowledge files. - `/custom/skills/` — Organization-specific action skills. -All three layers are enabled by default when an agent consumes BCQuality. Content can be promoted from Community to Microsoft-endorsed once it proves itself — this is a first-class concept, not an afterthought. +All three layers are enabled by default when an agent consumes BCQuality. In the shared upstream layers, an action skill and the canonical knowledge it owns should live together: knowledge used by a Microsoft-endorsed skill belongs in `/microsoft/`, while `/community/` holds community-owned skills and their related knowledge. A split is acceptable briefly while a skill or corpus is being promoted, but it should not be the steady state. The `/custom/` layer remains the intentional exception because it overrides shared content in consumer forks. + +Layer authority follows review and ownership, not the contributor's affiliation. Community contributions to a Microsoft-owned knowledge domain can therefore be accepted directly into `/microsoft/`; content can also be promoted from Community to Microsoft-endorsed once its owning skill is promoted. ### Skills @@ -60,6 +62,52 @@ Skills define how agents consume knowledge. They come in three flavors: An orchestrator (such as AL-Go) points the agent at BCQuality's URL and provides a task context. The agent's first call is `/skills/entry.md`, which returns a dispatch record naming the action skill(s) to invoke. The agent then invokes each dispatched skill in turn, reading READ and DO on demand. No prior knowledge of BCQuality's structure is baked into the orchestrator — only the convention *"invoke `/skills/entry.md` first."* +### Standalone plugin installation + +BCQuality can also be installed directly as a plugin. The plugin registers one +host-native skill, +[`al-code-review`](skills/al-code-review/SKILL.md), which adapts the caller's +request to the same Entry protocol used by orchestrators. + +For GitHub Copilot CLI: + +```shell +copilot plugin install microsoft/BCQuality +``` + +Plugin version `0.2.0` renamed the former `bcquality-al-review` skill to +`al-code-review`; explicit invocations and allowlists using the old skill name +must be updated. The name remains distinct from BC-ALAgents' public +`al-review` skill because current hosts may load plugin skill names into one +shared inventory. + +The adapter is intentionally not a second review implementation: + +```text +standalone host skill: skills/al-code-review/SKILL.md + -> routing contract: skills/entry.md + -> review coordinator: microsoft/skills/review/al-code-review.md + -> domain review leaves +``` + +Only the first file follows the host's `SKILL.md` packaging format. The +remaining files are BCQuality's internal protocol and layered action skills. +Entry remains the single owner of routing and index preparation; +`al-code-review.md` remains the single owner of broad-review composition. This +separation keeps standalone installation available without duplicating those +policies in the plugin adapter. + +Note that a plugin install ships the entire tree, so `BCQUALITY_ENABLED_LAYERS` +narrows discovery without removing any files. Layer selection is a filter here, +not a deny mechanism — see [the adapter](skills/al-code-review/SKILL.md) for the +difference from the pruned-clone model. + +The host adapter and internal action skill intentionally share the +`al-code-review` name: they expose the same operation in two different skill +formats. Their paths make the boundary explicit. The adapter lives under +`skills/al-code-review/SKILL.md`; the internal Microsoft-layer coordinator +lives at `microsoft/skills/review/al-code-review.md`. + ## Knowledge file format Every knowledge file is a markdown file with mandatory YAML frontmatter. Files target under 100 lines (ideal under 50). If two ideas would share a file, split them. @@ -90,7 +138,7 @@ Code examples belong in separate files, not in the knowledge file itself. Knowle ## Scope -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. +The current curated corpus is focused on **technical AL code review**: Agents, 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 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. @@ -148,7 +196,7 @@ Contributions are welcome. Before submitting a PR: 1. Read the knowledge file format above — frontmatter and sections are validated by CI. 2. Keep files atomic: one concern per file, under 100 lines. -3. Target your contribution to the right layer — most community contributions go in `/community/knowledge/`. +3. Target your contribution to the layer that owns the action skill: use `/microsoft/knowledge/` for Microsoft-owned domains and `/community/knowledge/` for knowledge that accompanies a community-owned skill. 4. Adding a BC fact — or stopping the agent from flagging a false positive — is a knowledge file, not a skill edit. If a PR changes *what* a review skill flags, the change almost certainly belongs in a knowledge file. See [`skills/write.md`](skills/write.md). CI runs validation on every PR. If your knowledge file has schema violations, missing sections, code blocks, or exceeds 100 lines, the check will fail with a clear error message. diff --git a/agent-consumption.md b/agent-consumption.md index 8db80d6..37a7a10 100644 --- a/agent-consumption.md +++ b/agent-consumption.md @@ -12,6 +12,10 @@ For the high-level framing and repo structure, start with the [README](README.md - **Global skills** in `/skills/` — the `entry.md` entry-point skill plus the READ · DO · WRITE contracts that govern the rest of the repo. - **Layer content** in `/microsoft/`, `/community/`, and `/custom/` — knowledge files and action skills grouped by authority. +When BCQuality is installed as a standalone plugin, it additionally exposes +`skills/al-code-review/SKILL.md`. This is a host-format adapter, not another +action skill: it creates the task context and enters the same flow at Entry. + ## The flow ```mermaid @@ -31,6 +35,12 @@ The orchestrator has a URL setting that points at BCQuality (default: `github.co ### 2. Agent invokes Entry The agent reads `/skills/entry.md` and runs it against the task context. Entry applies its Source → Relevance → Worklist → Action steps over the action skills under `*/skills/**/*.md` and returns a **dispatch record**: the set of action skills to invoke, plus a list of candidates it skipped (with reasons). Routing is a skill, not orchestrator logic. +For a standalone plugin installation, the host activates the +`skills/al-code-review/SKILL.md` adapter first. That adapter preserves the +caller's actual goal, constructs the task context, and invokes Entry. It does +not select the internal `microsoft/skills/review/al-code-review.md` action skill +itself or duplicate Entry's preparation, routing, and failure semantics. + ### 3. Agent consumes the dispatch record The dispatch record names one or more action skills and the subset of inputs each should receive. If the outcome is `no-match` or `failed`, the agent returns the record to the orchestrator unchanged. @@ -48,7 +58,7 @@ Each action skill is a markdown file that specifies what to do at each step. The | **Worklist** | Narrow from N candidates to the M that apply to this specific task. | | **Action** | Apply the relevant knowledge and produce structured output. | -Example: a performance review skill sources from `/microsoft/knowledge/performance/` and `/community/knowledge/performance/`, filters to `bc-version: 26` and `technologies: [al]`, narrows the 25 candidate files to the 8 that apply to the 15 objects changed in the PR, and then evaluates each file against the diff. +Example: the Microsoft-owned performance review skill selects `performance` entries across every enabled layer, filters to `bc-version: 26` and `technologies: [al]`, narrows the candidate files to those that apply to the changed objects, and then evaluates each file against the diff. Its canonical corpus lives beside it under `/microsoft/knowledge/performance/`; cross-layer entries are limited to custom overrides or short-lived promotion work. At this point the agent reads READ and DO on demand — it needs READ to interpret each knowledge file's frontmatter and sections, and DO to shape its output. Those contracts are fetched when first needed, not as part of bootstrap. @@ -89,8 +99,12 @@ Orchestrators MUST tolerate an absent `domain` in reports from older producers. ## Why this architecture - **Entry is the only hardcoded thing.** Orchestrators ship with one convention — *"invoke `/skills/entry.md` first"* — and nothing else. New action skills and new knowledge files are picked up automatically because Entry discovers them at dispatch time. +- **Standalone installation adds an adapter, not another policy layer.** The + plugin's host-format `al-code-review` skill only translates the invocation + into Entry's task context. Entry and the dispatched action skills remain + authoritative. - **Layers decide authority, not code.** The agent sees `/microsoft/` and `/community/` together; if two files conflict, the precedence rule defined in READ resolves it. A partner fork can disable `/community/` — that's a config choice, not a code change. -- **Knowledge and skills evolve independently.** A new knowledge file requires no skill changes — existing skills pick it up via frontmatter filters. A new skill requires no knowledge changes — it sources from what's already there. +- **Knowledge and skills evolve independently within their owning layer.** A new knowledge file requires no skill changes because existing skills pick it up via frontmatter filters. Layer placement still follows skill ownership, so promoting a skill also promotes its canonical corpus. ## The mental model, in one sentence diff --git a/community/knowledge/agents/agent-permissions-intersect-with-assigner.bad.al b/community/knowledge/agents/agent-permissions-intersect-with-assigner.bad.al new file mode 100644 index 0000000..eab5ef7 --- /dev/null +++ b/community/knowledge/agents/agent-permissions-intersect-with-assigner.bad.al @@ -0,0 +1,11 @@ +permissionset 50100 "SALES REVIEW AGENT" +{ + Assignable = true; + Permissions = + tabledata "Sales Header" = RIM, + tabledata Customer = R, + tabledata User = RIMD, + tabledata "Access Control" = RIMD, + page "Sales Order" = X, + page "User Card" = X; +} diff --git a/community/knowledge/agents/agent-permissions-intersect-with-assigner.good.al b/community/knowledge/agents/agent-permissions-intersect-with-assigner.good.al new file mode 100644 index 0000000..836c6a5 --- /dev/null +++ b/community/knowledge/agents/agent-permissions-intersect-with-assigner.good.al @@ -0,0 +1,8 @@ +permissionset 50100 "SALES REVIEW AGENT" +{ + Assignable = true; + Permissions = + tabledata "Sales Header" = RIM, + tabledata Customer = R, + page "Sales Order" = X; +} diff --git a/community/knowledge/agents/agent-permissions-intersect-with-assigner.md b/community/knowledge/agents/agent-permissions-intersect-with-assigner.md new file mode 100644 index 0000000..2259f66 --- /dev/null +++ b/community/knowledge/agents/agent-permissions-intersect-with-assigner.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [permissions, assigner, intersection, user-card, least-privilege] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Agent permissions intersect the assigner's; agents cannot configure users + +## Description + +An agent is a user, but it cannot configure users or other agents, and it cannot open sensitive pages such as user cards or permission-set assignment. Effective rights are the intersection of the assigning user's permissions and the agent's permission sets. Granting the agent a wide set does not bypass the assigner's limits, and a wide assigner still cannot give the agent user-admin powers the platform forbids. + +## Best Practice + +Document that intersection. Give the agent only the table and page rights its tasks need. Do not add user-setup or permission-assignment pages to the agent profile or permission sets; those operations will fail by design. + +See sample: `agent-permissions-intersect-with-assigner.good.al`. + +## Anti Pattern + +Permission sets or profiles that include User card, Permission Set Assignment, or agent-admin pages, or comments that the agent runs as SUPER regardless of who assigned it. Detection signal: default access controls or profile including user-administration objects. + +See sample: `agent-permissions-intersect-with-assigner.bad.al`. + +## See also + +`get-default-access-controls-least-privilege.md` covers the permission sets assigned when an agent instance is created. diff --git a/community/knowledge/agents/agent-profile-narrows-visible-ui.bad.al b/community/knowledge/agents/agent-profile-narrows-visible-ui.bad.al new file mode 100644 index 0000000..58b1cba --- /dev/null +++ b/community/knowledge/agents/agent-profile-narrows-visible-ui.bad.al @@ -0,0 +1,8 @@ +codeunit 50100 "Sales Review Agent Factory" +{ + procedure GetDefaultProfile(var TempAllProfile: Record "All Profile" temporary) + begin + TempAllProfile."Profile ID" := 'BUSINESS MANAGER'; + TempAllProfile.Insert(); + end; +} diff --git a/community/knowledge/agents/agent-profile-narrows-visible-ui.good.al b/community/knowledge/agents/agent-profile-narrows-visible-ui.good.al new file mode 100644 index 0000000..5ffe61a --- /dev/null +++ b/community/knowledge/agents/agent-profile-narrows-visible-ui.good.al @@ -0,0 +1,26 @@ +profile "SALES REVIEW AGENT" +{ + Caption = 'Sales Review Agent'; + Description = 'Restricted UI for the Sales Review Agent.'; + RoleCenter = "Order Processor Role Center"; + Customizations = "Sales Review Agent Sales Ord."; +} + +pagecustomization "Sales Review Agent Sales Ord." customizes "Sales Order" +{ + layout + { + modify("Payment Terms Code") + { + Visible = false; + } + } + + actions + { + modify(Post) + { + Visible = false; + } + } +} diff --git a/community/knowledge/agents/agent-profile-narrows-visible-ui.md b/community/knowledge/agents/agent-profile-narrows-visible-ui.md new file mode 100644 index 0000000..30d286a --- /dev/null +++ b/community/knowledge/agents/agent-profile-narrows-visible-ui.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [profile, page-customization, hidden-actions, tooltip, role-center] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Give the agent a dedicated profile that hides unrelated UI + +## Description + +The agent only sees what its profile shows. Extra actions, views, and Role Center tiles become extra tools and extra tokens. Accuracy and cost both get worse as the UI widens. A human Order Processor profile is usually far too broad. Tooltips on the remaining actions are part of the tool description. + +## Best Practice + +Ship an agent-specific profile and page customizations: hide unrelated actions, keep descriptive tooltips, add Role Center links to the few pages the agent should open. Prefer fewer navigation hops. + +See sample: `agent-profile-narrows-visible-ui.good.al`. + +## Anti Pattern + +Assigning `BUSINESS MANAGER` or `ORDER PROCESSOR` as `GetDefaultProfile` so the agent can do anything. Detection signal: default profile equal to a full-user role with no agent page customizations. + +See sample: `agent-profile-narrows-visible-ui.bad.al`. + +## See also + +`get-default-profile-lives-in-the-app.md` covers packaging and assigning the profile that this rule narrows. diff --git a/community/knowledge/agents/agent-setup-page-is-configuration-dialog.bad.al b/community/knowledge/agents/agent-setup-page-is-configuration-dialog.bad.al new file mode 100644 index 0000000..c23bec1 --- /dev/null +++ b/community/knowledge/agents/agent-setup-page-is-configuration-dialog.bad.al @@ -0,0 +1,19 @@ +page 50100 "Sales Review Agent Setup" +{ + PageType = Card; + Caption = 'Set up Sales Review Agent'; + SourceTable = "Sales Review Agent Setup"; + + layout + { + area(Content) + { + field(ReviewThreshold; Rec."Review Threshold") + { + ApplicationArea = All; + Caption = 'Review Threshold'; + ToolTip = 'Specifies the threshold used when the agent requests a review.'; + } + } + } +} diff --git a/community/knowledge/agents/agent-setup-page-is-configuration-dialog.good.al b/community/knowledge/agents/agent-setup-page-is-configuration-dialog.good.al new file mode 100644 index 0000000..6c4eaf2 --- /dev/null +++ b/community/knowledge/agents/agent-setup-page-is-configuration-dialog.good.al @@ -0,0 +1,30 @@ +page 50100 "Sales Review Agent Setup" +{ + PageType = ConfigurationDialog; + Caption = 'Set up Sales Review Agent'; + SourceTable = "Sales Review Agent Setup"; + SourceTableTemporary = true; + Extensible = false; + + layout + { + area(Content) + { + part(AgentSetupPart; "Agent Setup Part") + { + ApplicationArea = All; + UpdatePropagation = Both; + } + group(AdditionalConfiguration) + { + Caption = 'Additional Configuration'; + field(ReviewThreshold; Rec."Review Threshold") + { + ApplicationArea = All; + Caption = 'Review Threshold'; + ToolTip = 'Specifies the threshold used when the agent requests a review.'; + } + } + } + } +} diff --git a/community/knowledge/agents/agent-setup-page-is-configuration-dialog.md b/community/knowledge/agents/agent-setup-page-is-configuration-dialog.md new file mode 100644 index 0000000..d844d33 --- /dev/null +++ b/community/knowledge/agents/agent-setup-page-is-configuration-dialog.md @@ -0,0 +1,26 @@ +--- +bc-version: [27..] +domain: agents +keywords: [configurationdialog, agent-setup-part, setup-page, pagetype, system-actions] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Agent setup pages use ConfigurationDialog and the Agent Setup Part + +## Description + +Instance setup is not a Card or StandardDialog. The toolkit expects `PageType = ConfigurationDialog` so OK and Cancel are system actions, plus the built-in `Agent Setup Part` for name, display name, state, and access. A Card with custom fields only drops those shared controls and the AI-use notices the part carries. + +## Best Practice + +Declare `PageType = ConfigurationDialog`, host `part(...; "Agent Setup Part")`, and put agent-specific fields in another group. Keep system OK/Cancel. Use a temporary source record and defer persistence until Update, as described in `agent-setup-source-table-is-temporary.md`. Following Microsoft's agent setup samples, set `Extensible = false`. + +See sample: `agent-setup-page-is-configuration-dialog.good.al`. + +## Anti Pattern + +A Card or StandardDialog setup page with no `Agent Setup Part`. Detection signal: setup page ID from `IAgentFactory` / `IAgentMetadata` whose page is not `ConfigurationDialog` or has no `Agent Setup Part`. + +See sample: `agent-setup-page-is-configuration-dialog.bad.al`. diff --git a/community/knowledge/agents/agent-setup-source-table-is-temporary.bad.al b/community/knowledge/agents/agent-setup-source-table-is-temporary.bad.al new file mode 100644 index 0000000..96ef570 --- /dev/null +++ b/community/knowledge/agents/agent-setup-source-table-is-temporary.bad.al @@ -0,0 +1,29 @@ +page 50100 "Sales Review Agent Setup" +{ + PageType = ConfigurationDialog; + SourceTable = "Sales Review Agent Setup"; + + layout + { + area(Content) + { + field(ReviewThreshold; Rec."Review Threshold") + { + ApplicationArea = All; + Caption = 'Review Threshold'; + ToolTip = 'Specifies the threshold used when the agent requests a review.'; + + trigger OnValidate() + begin + Rec.Modify(true); + end; + } + } + } + + trigger OnOpenPage() + begin + if Rec.IsEmpty() then + Rec.Insert(true); + end; +} diff --git a/community/knowledge/agents/agent-setup-source-table-is-temporary.good.al b/community/knowledge/agents/agent-setup-source-table-is-temporary.good.al new file mode 100644 index 0000000..cbeabca --- /dev/null +++ b/community/knowledge/agents/agent-setup-source-table-is-temporary.good.al @@ -0,0 +1,68 @@ +page 50100 "Sales Review Agent Setup" +{ + PageType = ConfigurationDialog; + SourceTable = "Sales Review Agent Setup"; + SourceTableTemporary = true; + Extensible = false; + + layout + { + area(Content) + { + part(AgentSetupPart; "Agent Setup Part") + { + ApplicationArea = All; + UpdatePropagation = Both; + } + group(AdditionalConfiguration) + { + Caption = 'Additional Configuration'; + field(ReviewThreshold; Rec."Review Threshold") + { + ApplicationArea = All; + Caption = 'Review Threshold'; + ToolTip = 'Specifies the threshold used when the agent requests a review.'; + } + } + } + } + + trigger OnOpenPage() + var + SalesReviewAgentSetup: Record "Sales Review Agent Setup"; + begin + if IsNullGuid(Rec."User Security ID") then + exit; + if SalesReviewAgentSetup.Get(Rec."User Security ID") then + Rec := SalesReviewAgentSetup; + end; + + trigger OnQueryClosePage(CloseAction: Action): Boolean + var + AgentSetup: Codeunit "Agent Setup"; + AgentSetupBuffer: Record "Agent Setup Buffer"; + begin + if CloseAction = CloseAction::Cancel then + exit(true); + CurrPage.AgentSetupPart.Page.GetAgentSetupBuffer(AgentSetupBuffer); + if AgentSetup.GetChangesMade(AgentSetupBuffer) then + Rec."User Security ID" := AgentSetup.SaveChanges(AgentSetupBuffer); + if IsNullGuid(Rec."User Security ID") then + exit(true); + SaveCustomProperties(); + exit(true); + end; + + local procedure SaveCustomProperties() + var + SalesReviewAgentSetup: Record "Sales Review Agent Setup"; + begin + if not SalesReviewAgentSetup.Get(Rec."User Security ID") then begin + SalesReviewAgentSetup.Init(); + SalesReviewAgentSetup."User Security ID" := Rec."User Security ID"; + SalesReviewAgentSetup.Insert(true); + end; + SalesReviewAgentSetup."Review Threshold" := Rec."Review Threshold"; + SalesReviewAgentSetup.Modify(true); + end; +} diff --git a/community/knowledge/agents/agent-setup-source-table-is-temporary.md b/community/knowledge/agents/agent-setup-source-table-is-temporary.md new file mode 100644 index 0000000..8f31aa0 --- /dev/null +++ b/community/knowledge/agents/agent-setup-source-table-is-temporary.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [sourcetabletemporary, configurationdialog, savechanges, cancel, draft] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep the agent setup page source temporary until Update + +## Description + +ConfigurationDialog setup is a draft: the user can Cancel without writing. That only works if `SourceTableTemporary = true` and custom fields stay in memory until Update. Writing the real table in OnValidate or OnOpenPage commits a partial agent when the dialog errors or is cancelled. + +## Best Practice + +Mark the page `SourceTableTemporary = true`. Copy into the temp record on open. Persist the Agent Setup buffer and custom fields only from the close path when the action is not Cancel, using `Agent Setup.GetChangesMade` / `SaveChanges`. + +See sample: `agent-setup-source-table-is-temporary.good.al`. + +## Anti Pattern + +A non-temporary source table, or `Insert`/`Modify` on the persisted setup row from field OnValidate. Detection signal: agent `ConfigurationDialog` without `SourceTableTemporary = true`, or database writes before Update. + +See sample: `agent-setup-source-table-is-temporary.bad.al`. + +## See also + +`agent-setup-page-is-configuration-dialog.md` defines the setup page shape that uses this draft lifecycle. diff --git a/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.bad.al b/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.bad.al new file mode 100644 index 0000000..cff2738 --- /dev/null +++ b/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.bad.al @@ -0,0 +1,24 @@ +table 50100 "Sales Review Agent Setup" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Primary Key"; Code[10]) + { + Caption = 'Primary Key'; + } + field(10; "Review Threshold"; Decimal) + { + Caption = 'Review Threshold'; + } + } + + keys + { + key(PK; "Primary Key") + { + Clustered = true; + } + } +} diff --git a/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.good.al b/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.good.al new file mode 100644 index 0000000..f5b1f9b --- /dev/null +++ b/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.good.al @@ -0,0 +1,25 @@ +table 50100 "Sales Review Agent Setup" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "User Security ID"; Guid) + { + Caption = 'User Security ID'; + DataClassification = EndUserPseudonymousIdentifiers; + } + field(10; "Review Threshold"; Decimal) + { + Caption = 'Review Threshold'; + } + } + + keys + { + key(PK; "User Security ID") + { + Clustered = true; + } + } +} diff --git a/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.md b/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.md new file mode 100644 index 0000000..c59a9f1 --- /dev/null +++ b/community/knowledge/agents/agent-setup-table-keyed-by-user-security-id.md @@ -0,0 +1,26 @@ +--- +bc-version: [27..] +domain: agents +keywords: [user-security-id, setup-table, primary-key, agent-instance, guid] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Agent setup tables are keyed by User Security ID + +## Description + +Each agent instance is a user. Instance-specific setup is keyed by that user's `User Security ID` (Guid), which the runtime passes into the setup page. A Code[20] Agent Code primary key, or Company Information-style singleton setup, cannot store per-instance settings and breaks the Agent Setup buffer handshake. + +## Best Practice + +Give the setup table a Guid field `User Security ID` as the clustered primary key. Other settings are attributes of that key. When the page opens, `Get` or insert by the Guid the Agent Setup part already holds. + +See sample: `agent-setup-table-keyed-by-user-security-id.good.al`. + +## Anti Pattern + +A setup table keyed by Code, Integer, or with no Guid user key, then mapping one row to every instance. Detection signal: source table of the agent setup page whose primary key is not `User Security ID`. + +See sample: `agent-setup-table-keyed-by-user-security-id.bad.al`. diff --git a/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.bad.al b/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.bad.al new file mode 100644 index 0000000..d75db08 --- /dev/null +++ b/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.bad.al @@ -0,0 +1,8 @@ +codeunit 50100 "Sales Review Agent Task" +{ + procedure AnalyzeAgentTaskMessage(AgentTaskMessage: Record "Agent Task Message"; var Annotations: Record "Agent Annotation") + begin + // No validation. Combined with SetRequiresReview(false) this auto-runs + // untrusted input. Warnings are the only way to force a review later. + end; +} diff --git a/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.good.al b/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.good.al new file mode 100644 index 0000000..c3bbc18 --- /dev/null +++ b/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.good.al @@ -0,0 +1,41 @@ +codeunit 50100 "Sales Review Agent Task" +{ + procedure AnalyzeAgentTaskMessage(AgentTaskMessage: Record "Agent Task Message"; var Annotations: Record "Agent Annotation") + var + AgentMessage: Codeunit "Agent Message"; + EmptyMessageMsg: Label 'Message is empty.'; + EmptyMessageDetailsTxt: Label 'Provide a sales order task before running the agent.'; + NotRelevantMsg: Label 'Message is not a sales order task.'; + NotRelevantDetailsTxt: Label 'Provide a message related to sales order review.'; + MessageText: Text; + begin + if AgentTaskMessage.Type = AgentTaskMessage.Type::Output then begin + AgentMessage.UpdateText(AgentTaskMessage, AgentMessage.GetText(AgentTaskMessage) + #13#10 + #13#10 + 'Written with the help of AI'); + exit; + end; + + MessageText := AgentMessage.GetText(AgentTaskMessage); + if MessageText = '' then begin + Clear(Annotations); + Annotations.Code := 'MESSAGE001'; + Annotations.Severity := Annotations.Severity::Error; + Annotations.Message := EmptyMessageMsg; + Annotations.Details := EmptyMessageDetailsTxt; + Annotations.Insert(); + exit; + end; + if not IsRelevant(MessageText) then begin + Clear(Annotations); + Annotations.Code := 'RELEVANCE001'; + Annotations.Severity := Annotations.Severity::Warning; + Annotations.Message := NotRelevantMsg; + Annotations.Details := NotRelevantDetailsTxt; + Annotations.Insert(); + end; + end; + + local procedure IsRelevant(MessageText: Text): Boolean + begin + exit(StrPos(LowerCase(MessageText), 'sales order') > 0); + end; +} diff --git a/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.md b/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.md new file mode 100644 index 0000000..f8c6eb5 --- /dev/null +++ b/community/knowledge/agents/analyze-message-error-stops-warning-forces-review.md @@ -0,0 +1,26 @@ +--- +bc-version: [27..] +domain: agents +keywords: [analyzeagenttaskmessage, agent-annotation, error, warning, setrequiresreview] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AnalyzeAgentTaskMessage: Error stops the task; Warning still requires review + +## Description + +`IAgentTaskExecution.AnalyzeAgentTaskMessage` runs on inbound and outbound messages. An Error annotation stops processing. A Warning annotation requests user intervention. If analysis returns Warning, the platform still requires approval even when the incoming message used `SetRequiresReview(false)`. Output text can be rewritten here (signature, redaction). + +## Best Practice + +Validate inbound payloads in analysis: Error when the task must not run; Warning when a human must confirm. For outbound messages, adjust text in this method rather than in a later subscriber. Do not rely on skip-review to bypass warnings. + +See sample: `analyze-message-error-stops-warning-forces-review.good.al`. + +## Anti Pattern + +Ignoring analysis entirely, or emitting Warning while documenting that `SetRequiresReview(false)` means unattended run. Detection signal: empty `AnalyzeAgentTaskMessage` plus skip-review on external input. + +See sample: `analyze-message-error-stops-warning-forces-review.bad.al`. diff --git a/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.bad.al b/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.bad.al new file mode 100644 index 0000000..998066c --- /dev/null +++ b/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.bad.al @@ -0,0 +1,9 @@ +codeunit 50101 "Sales Review Agent Events" +{ + [EventSubscriber(ObjectType::Table, Database::"Sales Header", OnAfterInsertEvent, '', false, false)] + local procedure OnAfterInsertSalesHeader(var Rec: Record "Sales Header") + begin + // Runs for every user session, not only the agent. + Message('Keep going, agent.'); + end; +} diff --git a/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.good.al b/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.good.al new file mode 100644 index 0000000..e4894fd --- /dev/null +++ b/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.good.al @@ -0,0 +1,34 @@ +codeunit 50101 "Sales Review Agent Subscribers" +{ + Access = Internal; + EventSubscriberInstance = Manual; + SingleInstance = true; + + [EventSubscriber(ObjectType::Table, Database::"Sales Header", OnAfterInsertEvent, '', false, false)] + local procedure OnAfterInsertSalesHeader(var Rec: Record "Sales Header") + begin + Message('Keep going, agent.'); + end; +} + +codeunit 50102 "Agent Session Events" +{ + Access = Internal; + SingleInstance = true; + InherentEntitlements = X; + InherentPermissions = X; + + var + GlobalAgentSubscribers: Codeunit "Sales Review Agent Subscribers"; + + [EventSubscriber(ObjectType::Codeunit, Codeunit::"System Initialization", OnAfterInitialization, '', false, false)] + local procedure RegisterSubscribersOnAfterInitialization() + var + AgentSession: Codeunit "Agent Session"; + AgentMetadataProvider: Enum "Agent Metadata Provider"; + begin + if not AgentSession.IsAgentSession(AgentMetadataProvider) then + exit; + if BindSubscription(GlobalAgentSubscribers) then; + end; +} diff --git a/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.md b/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.md new file mode 100644 index 0000000..3d18818 --- /dev/null +++ b/community/knowledge/agents/bind-agent-subscribers-only-in-agent-session.md @@ -0,0 +1,26 @@ +--- +bc-version: [27..] +domain: agents +keywords: [agent-session, isagentsession, bindsubscription, system-initialization, singleinstance] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Bind extra agent subscribers only inside an agent session + +## Description + +Page-filter tweaks, extra validation, and prompt dialogs for the agent should not run for every user. `Agent Session.IsAgentSession` distinguishes agent UI sessions. Binding those subscribers on `System Initialization` only when the session is an agent session avoids global subscriber cost. Models register `SingleInstance` table subscribers unconditionally. + +## Best Practice + +On `OnAfterInitialization`, exit unless `Agent Session.IsAgentSession`. Then `BindSubscription` a single-instance codeunit that holds the current task id. Keep those subscribers internal. + +See sample: `bind-agent-subscribers-only-in-agent-session.good.al`. + +## Anti Pattern + +Event subscribers on `Sales Header` OnAfterInsert that always `Message` the agent, with no `IsAgentSession` guard. Detection signal: agent-only behaviour in a static subscriber that is not bind-gated. + +See sample: `bind-agent-subscribers-only-in-agent-session.bad.al`. diff --git a/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.bad.al b/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.bad.al new file mode 100644 index 0000000..93a7afe --- /dev/null +++ b/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.bad.al @@ -0,0 +1,10 @@ +codeunit 50110 "Other App Agent Hook" +{ + procedure RenameForeignAgent(AgentUserSecurityId: Guid) + var + Agent: Codeunit Agent; + begin + // Fails at runtime when the instance was defined in another app. + Agent.SetDisplayName(AgentUserSecurityId, 'Updated Name'); + end; +} diff --git a/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.good.al b/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.good.al new file mode 100644 index 0000000..51181e8 --- /dev/null +++ b/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.good.al @@ -0,0 +1,21 @@ +codeunit 50110 "Sales Review Agent API" +{ + Access = Public; + + procedure SetDisplayName(AgentUserSecurityId: Guid; NewDisplayName: Text[80]) + var + Agent: Codeunit Agent; + begin + Agent.SetDisplayName(AgentUserSecurityId, NewDisplayName); + end; + + procedure SetActiveState(AgentUserSecurityId: Guid; ActivateAgent: Boolean) + var + Agent: Codeunit Agent; + begin + if ActivateAgent then + Agent.Activate(AgentUserSecurityId) + else + Agent.Deactivate(AgentUserSecurityId); + end; +} diff --git a/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.md b/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.md new file mode 100644 index 0000000..41c1c2f --- /dev/null +++ b/community/knowledge/agents/cross-app-agent-calls-need-your-public-api.md @@ -0,0 +1,26 @@ +--- +bc-version: [27..] +domain: agents +keywords: [cross-app, public-api, agent-create, isolation, access-public] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Other apps cannot call the toolkit APIs on your agent; publish your own API + +## Description + +For isolation, `Agent`, `Agent Task Builder`, and related toolkit codeunits error when the target instance belongs to another app. There is no supported way to pass another extension's metadata provider into `SetInstructions` or `Create`. Partners who need to enqueue work must call a public API you own. + +## Best Practice + +Expose a public codeunit in the agent app (`Access = Public`) whose procedures take `User Security ID` and forward to `Agent` / `Agent Task Builder`. Document that surface as the integration contract. Keep toolkit calls inside that app. + +See sample: `cross-app-agent-calls-need-your-public-api.good.al`. + +## Anti Pattern + +From app B, calling `Agent.SetDisplayName` or `Agent.Create` with app A's metadata provider. Detection signal: toolkit agent APIs used with an `Agent Metadata Provider` value not declared in the same app. + +See sample: `cross-app-agent-calls-need-your-public-api.bad.al`. diff --git a/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.bad.al b/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.bad.al new file mode 100644 index 0000000..26cbb38 --- /dev/null +++ b/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.bad.al @@ -0,0 +1,19 @@ +codeunit 50100 "Sales Review Agent Install" +{ + Subtype = Install; + + trigger OnInstallAppPerCompany() + var + Agent: Codeunit Agent; + TempAgentAccessControl: Record "Agent Access Control" temporary; + AgentUserSecurityId: Guid; + begin + // Create requires an interactive session. Install is not one. + AgentUserSecurityId := Agent.Create( + Enum::"Agent Metadata Provider"::"Sales Review Agent", + 'SALESREVIEW', + 'Sales Review Agent', + TempAgentAccessControl); + Agent.Activate(AgentUserSecurityId); + end; +} diff --git a/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.good.al b/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.good.al new file mode 100644 index 0000000..a1831f4 --- /dev/null +++ b/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.good.al @@ -0,0 +1,44 @@ +page 50100 "Sales Review Agent Setup" +{ + PageType = ConfigurationDialog; + ApplicationArea = All; + SourceTable = "Sales Review Agent Setup"; + SourceTableTemporary = true; + Extensible = false; + + layout + { + area(Content) + { + part(AgentSetupPart; "Agent Setup Part") + { + ApplicationArea = All; + UpdatePropagation = Both; + } + } + } + + trigger OnQueryClosePage(CloseAction: Action): Boolean + var + Agent: Codeunit Agent; + AgentSetup: Codeunit "Agent Setup"; + TempAgentSetupBuffer: Record "Agent Setup Buffer" temporary; + AgentUserSecurityId: Guid; + begin + if CloseAction = CloseAction::Cancel then + exit(true); + + CurrPage.AgentSetupPart.Page.GetAgentSetupBuffer(TempAgentSetupBuffer); + AgentUserSecurityId := AgentSetup.SaveChanges(TempAgentSetupBuffer); + Agent.SetInstructions(AgentUserSecurityId, GetInstructions()); + Agent.Activate(AgentUserSecurityId); + exit(true); + end; + + local procedure GetInstructions() Instructions: SecretText + var + InstructionsNameTxt: Label 'Instructions.txt', Locked = true; + begin + Instructions := NavApp.GetResourceAsText(InstructionsNameTxt); + end; +} diff --git a/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.md b/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.md new file mode 100644 index 0000000..41d0573 --- /dev/null +++ b/community/knowledge/agents/do-not-create-agents-in-install-upgrade-or-background.md @@ -0,0 +1,26 @@ +--- +bc-version: [27..] +domain: agents +keywords: [agent-create, install, upgrade, job-queue, interactive-session, background] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not create agent instances from install, upgrade, or background sessions + +## Description + +`Agent.Create` requires an interactive user session. The platform blocks creation from install codeunits, upgrade codeunits, and background sessions (job queue, scheduled tasks). Packaging an agent in an app does not mean spinning up instances at install. Models still call `Create` from `OnInstallAppPerCompany` to activate the agent. + +## Best Practice + +Create instances from a setup page, a wizard, or another UI-driven path after the user is in a client session. Apply instructions and `Activate` there. For existing companies after an upgrade, document that an admin must open setup; do not create from the upgrade codeunit. + +See sample: `do-not-create-agents-in-install-upgrade-or-background.good.al`. + +## Anti Pattern + +`Agent.Create` inside `OnInstallAppPerCompany`, `OnUpgradePerCompany`, or a job-queue codeunit. The call fails at runtime even if it compiles. Detection signal: `Agent.Create` in `Subtype = Install`, `Subtype = Upgrade`, or a non-UI session. + +See sample: `do-not-create-agents-in-install-upgrade-or-background.bad.al`. diff --git a/community/knowledge/agents/get-default-access-controls-least-privilege.bad.al b/community/knowledge/agents/get-default-access-controls-least-privilege.bad.al new file mode 100644 index 0000000..618f964 --- /dev/null +++ b/community/knowledge/agents/get-default-access-controls-least-privilege.bad.al @@ -0,0 +1,14 @@ +codeunit 50100 "Sales Review Agent Factory" +{ + procedure GetDefaultAccessControls(var TempAccessControlBuffer: Record "Access Control Buffer" temporary) + var + BaseApplicationAppIdTok: Label '437dbf0e-84ff-417a-965d-ed2bb9650972', Locked = true; + begin + Clear(TempAccessControlBuffer); + TempAccessControlBuffer."Company Name" := CopyStr(CompanyName(), 1, MaxStrLen(TempAccessControlBuffer."Company Name")); + TempAccessControlBuffer.Scope := TempAccessControlBuffer.Scope::System; + TempAccessControlBuffer."App ID" := BaseApplicationAppIdTok; + TempAccessControlBuffer."Role ID" := 'D365 BUS FULL ACCESS'; + TempAccessControlBuffer.Insert(); + end; +} diff --git a/community/knowledge/agents/get-default-access-controls-least-privilege.good.al b/community/knowledge/agents/get-default-access-controls-least-privilege.good.al new file mode 100644 index 0000000..2c855f7 --- /dev/null +++ b/community/knowledge/agents/get-default-access-controls-least-privilege.good.al @@ -0,0 +1,25 @@ +permissionset 50100 "SALES REVIEW AGENT" +{ + Assignable = true; + Caption = 'Sales Review Agent'; + Permissions = + tabledata "Sales Header" = R, + tabledata "Sales Line" = R; +} + +codeunit 50100 "Sales Review Agent Factory" +{ + procedure GetDefaultAccessControls(var TempAccessControlBuffer: Record "Access Control Buffer" temporary) + var + CurrentModuleInfo: ModuleInfo; + RoleIdTok: Label 'SALES REVIEW AGENT', Locked = true; + begin + NavApp.GetCurrentModuleInfo(CurrentModuleInfo); + Clear(TempAccessControlBuffer); + TempAccessControlBuffer."Company Name" := CopyStr(CompanyName(), 1, MaxStrLen(TempAccessControlBuffer."Company Name")); + TempAccessControlBuffer.Scope := TempAccessControlBuffer.Scope::System; + TempAccessControlBuffer."App ID" := CurrentModuleInfo.Id; + TempAccessControlBuffer."Role ID" := RoleIdTok; + TempAccessControlBuffer.Insert(); + end; +} diff --git a/community/knowledge/agents/get-default-access-controls-least-privilege.md b/community/knowledge/agents/get-default-access-controls-least-privilege.md new file mode 100644 index 0000000..87349af --- /dev/null +++ b/community/knowledge/agents/get-default-access-controls-least-privilege.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [getdefaultaccesscontrols, access-control-buffer, permissionset, least-privilege, iagentfactory] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Default agent permission sets must exist in AL and stay least privilege + +## Description + +`IAgentFactory.GetDefaultAccessControls` fills a temporary `Access Control Buffer` used when an instance is created. Permission sets that exist only as user-created sets in a sandbox are missing in the next environment. Granting `D365 BUS FULL ACCESS` or SUPER gives the agent a user-sized blast radius. Effective rights are still the intersection with the assigning user's permissions. + +## Best Practice + +Insert only the permission sets the agent needs. For an AL `permissionset` object, use `Scope::System` and the ID of the app that defines it. Recreate permission sets that exist only as user-defined configuration in Business Central as AL objects first. Prefer a dedicated permission set over a full-user role. + +See sample: `get-default-access-controls-least-privilege.good.al`. + +## Anti Pattern + +Empty `GetDefaultAccessControls`, or inserting `SUPER` / `D365 BUS FULL ACCESS` because it made the demo work. Detection signal: Role ID on the default buffer that is a full-user role, or a set that is not in the app. + +See sample: `get-default-access-controls-least-privilege.bad.al`. + +## See also + +`agent-permissions-intersect-with-assigner.md` explains the platform limits that still apply after default access controls are assigned. diff --git a/community/knowledge/agents/get-default-profile-lives-in-the-app.bad.al b/community/knowledge/agents/get-default-profile-lives-in-the-app.bad.al new file mode 100644 index 0000000..ba713e4 --- /dev/null +++ b/community/knowledge/agents/get-default-profile-lives-in-the-app.bad.al @@ -0,0 +1,9 @@ +codeunit 50100 "Sales Review Agent Factory" +{ + procedure GetDefaultProfile(var TempAllProfile: Record "All Profile" temporary) + begin + // Profile exists only as a user personalization in the design sandbox. + TempAllProfile."Profile ID" := 'SALES REVIEW SANDBOX'; + TempAllProfile.Insert(); + end; +} diff --git a/community/knowledge/agents/get-default-profile-lives-in-the-app.good.al b/community/knowledge/agents/get-default-profile-lives-in-the-app.good.al new file mode 100644 index 0000000..455fba6 --- /dev/null +++ b/community/knowledge/agents/get-default-profile-lives-in-the-app.good.al @@ -0,0 +1,20 @@ +profile "SALES REVIEW AGENT" +{ + Caption = 'Sales Review Agent'; + Description = 'UI surface for the Sales Review Agent.'; + RoleCenter = "Order Processor Role Center"; + Customizations = "Sales Review Agent Sales Ord."; +} + +codeunit 50100 "Sales Review Agent Factory" +{ + procedure GetDefaultProfile(var TempAllProfile: Record "All Profile" temporary) + var + Agent: Codeunit Agent; + CurrentModuleInfo: ModuleInfo; + DefaultProfileTok: Label 'SALES REVIEW AGENT', Locked = true; + begin + NavApp.GetCurrentModuleInfo(CurrentModuleInfo); + Agent.PopulateDefaultProfile(DefaultProfileTok, CurrentModuleInfo.Id, TempAllProfile); + end; +} diff --git a/community/knowledge/agents/get-default-profile-lives-in-the-app.md b/community/knowledge/agents/get-default-profile-lives-in-the-app.md new file mode 100644 index 0000000..25103b5 --- /dev/null +++ b/community/knowledge/agents/get-default-profile-lives-in-the-app.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [getdefaultprofile, profile, page-customization, populatedefaultprofile, role-center] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# The default agent profile must be an AL profile in the app + +## Description + +`IAgentFactory.GetDefaultProfile` assigns the Role Center and page customizations the agent UI-navigates. A profile built only in the client, or page personalization that was never exported, is absent after deploy. `Agent.PopulateDefaultProfile` still needs a profile ID that exists in the current module. + +## Best Practice + +Ship a `profile` object (and page customizations) in the app. In `GetDefaultProfile`, call `Agent.PopulateDefaultProfile` with that profile ID and `NavApp.GetCurrentModuleInfo`. Include UI-exported customizations as AL. + +See sample: `get-default-profile-lives-in-the-app.good.al`. + +## Anti Pattern + +Setting `TempAllProfile."Profile ID"` to a client-only profile, or skipping `GetDefaultProfile`. Detection signal: factory default profile ID with no matching `profile` object in the app. + +See sample: `get-default-profile-lives-in-the-app.bad.al`. + +## See also + +`agent-profile-narrows-visible-ui.md` explains which UI the app-owned profile should expose. diff --git a/community/knowledge/agents/instruction-structure-is-role-rules-steps.bad.al b/community/knowledge/agents/instruction-structure-is-role-rules-steps.bad.al new file mode 100644 index 0000000..27d1a08 --- /dev/null +++ b/community/knowledge/agents/instruction-structure-is-role-rules-steps.bad.al @@ -0,0 +1,9 @@ +codeunit 50100 "Sales Review Agent Instr." +{ + procedure GetInstructions() Instructions: SecretText + var + PromptLbl: Label 'Check customer credit for the given sales order. Document the result.', Locked = true; + begin + Instructions := PromptLbl; + end; +} diff --git a/community/knowledge/agents/instruction-structure-is-role-rules-steps.good.al b/community/knowledge/agents/instruction-structure-is-role-rules-steps.good.al new file mode 100644 index 0000000..5876151 --- /dev/null +++ b/community/knowledge/agents/instruction-structure-is-role-rules-steps.good.al @@ -0,0 +1,17 @@ +codeunit 50100 "Sales Review Agent Instr." +{ + procedure GetInstructions() Instructions: SecretText + var + Builder: TextBuilder; + begin + Builder.AppendLine('# Responsibilities'); + Builder.AppendLine('You validate sales orders against customer credit and hold status.'); + Builder.AppendLine('# Guidelines'); + Builder.AppendLine('Always request a review before posting or sending external mail.'); + Builder.AppendLine('# Instructions'); + Builder.AppendLine('1. Open the sales order named in the task.'); + Builder.AppendLine('2. Check credit limit and overdue balance.'); + Builder.AppendLine('3. Document the result on the order and request a review.'); + Instructions := Builder.ToText(); + end; +} diff --git a/community/knowledge/agents/instruction-structure-is-role-rules-steps.md b/community/knowledge/agents/instruction-structure-is-role-rules-steps.md new file mode 100644 index 0000000..1b56625 --- /dev/null +++ b/community/knowledge/agents/instruction-structure-is-role-rules-steps.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [instructions, responsibilities, guidelines, steps, setinstructions] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Instruction documents use responsibilities, guidelines, then ordered steps + +## Description + +The runtime treats instructions as the agent's standing prompt. A one-line goal produces inconsistent navigation. Microsoft's instruction framework is three layers: responsibilities (what the agent owns), guidelines (rules for every task), and instructions (ordered steps per task, with substeps). That structure is BC-specific, not generic prompt flavour. + +## Best Practice + +Store a document that states responsibilities, then non-negotiable guidelines (when to request a review, when not to post), then numbered steps for each task. Keep that text in the resource you pass to `SetInstructions`. + +See sample: `instruction-structure-is-role-rules-steps.good.al`. + +## Anti Pattern + +A single sentence such as Check customer credit for the sales order. Detection signal: instruction resource or `SetInstructions` payload with no responsibilities / guidelines / steps sections. + +See sample: `instruction-structure-is-role-rules-steps.bad.al`. + +## See also + +`instructions-describe-work-not-tool-ids.md` and `use-documented-instruction-keywords.md` define how to write the steps inside this structure. diff --git a/community/knowledge/agents/instructions-describe-work-not-tool-ids.bad.al b/community/knowledge/agents/instructions-describe-work-not-tool-ids.bad.al new file mode 100644 index 0000000..4662547 --- /dev/null +++ b/community/knowledge/agents/instructions-describe-work-not-tool-ids.bad.al @@ -0,0 +1,7 @@ +codeunit 50100 "Sales Review Agent Instr." +{ + procedure GetInstructions() Instructions: SecretText + begin + Instructions := 'Open page 42. Invoke action Post_Promoted. Use tool SalesOrder.CreditCheck_v3.'; + end; +} diff --git a/community/knowledge/agents/instructions-describe-work-not-tool-ids.good.al b/community/knowledge/agents/instructions-describe-work-not-tool-ids.good.al new file mode 100644 index 0000000..75fd986 --- /dev/null +++ b/community/knowledge/agents/instructions-describe-work-not-tool-ids.good.al @@ -0,0 +1,12 @@ +codeunit 50100 "Sales Review Agent Instr." +{ + procedure GetInstructions() Instructions: SecretText + var + Builder: TextBuilder; + begin + Builder.AppendLine('Memorize the sales order number from the task.'); + Builder.AppendLine('Set the order on hold when credit fails, with a reason.'); + Builder.AppendLine('When credit passes, request a review before posting the order.'); + Instructions := Builder.ToText(); + end; +} diff --git a/community/knowledge/agents/instructions-describe-work-not-tool-ids.md b/community/knowledge/agents/instructions-describe-work-not-tool-ids.md new file mode 100644 index 0000000..a1a536a --- /dev/null +++ b/community/knowledge/agents/instructions-describe-work-not-tool-ids.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [instructions, tools, invoke-action, memorize, page-actions] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Instructions describe outcomes, not page action or tool names + +## Description + +Agent tools are the UI the profile exposes. Action names and tool ids change across pages and versions. Best-practice guidance is to say what to accomplish, not which tool to invoke. Page state is also not fully in history; values needed later must be memorized. Models paste Promoted action names into the prompt. + +## Best Practice + +Write steps as business outcomes (release the order, set the hold reason). Tell the agent to memorize identifiers it must reuse. Do not hard-code action captions or tool ids. + +See sample: `instructions-describe-work-not-tool-ids.good.al`. + +## Anti Pattern + +Instructions that say invoke SalesOrder.Post_Promoted or use tool page-42-action-3. Detection signal: instruction text containing Promoted action names or tool identifiers. + +See sample: `instructions-describe-work-not-tool-ids.bad.al`. + +## See also + +`instruction-structure-is-role-rules-steps.md` defines the containing document structure, and `use-documented-instruction-keywords.md` identifies runtime-recognized phrases. diff --git a/community/knowledge/agents/reapply-resource-instructions-on-upgrade.bad.al b/community/knowledge/agents/reapply-resource-instructions-on-upgrade.bad.al new file mode 100644 index 0000000..56ca228 --- /dev/null +++ b/community/knowledge/agents/reapply-resource-instructions-on-upgrade.bad.al @@ -0,0 +1,18 @@ +codeunit 50100 "Sales Review Agent Create" +{ + procedure CreateWithInstructions() + var + Agent: Codeunit Agent; + TempAgentAccessControl: Record "Agent Access Control" temporary; + AgentUserSecurityId: Guid; + InstructionsNameTxt: Label 'Instructions.txt', Locked = true; + begin + AgentUserSecurityId := Agent.Create( + Enum::"Agent Metadata Provider"::"Sales Review Agent", + 'SALESREVIEW', + 'Sales Review Agent', + TempAgentAccessControl); + // Only new instances get the resource. Upgrades never re-apply it. + Agent.SetInstructions(AgentUserSecurityId, NavApp.GetResourceAsText(InstructionsNameTxt)); + end; +} diff --git a/community/knowledge/agents/reapply-resource-instructions-on-upgrade.good.al b/community/knowledge/agents/reapply-resource-instructions-on-upgrade.good.al new file mode 100644 index 0000000..da255d0 --- /dev/null +++ b/community/knowledge/agents/reapply-resource-instructions-on-upgrade.good.al @@ -0,0 +1,33 @@ +codeunit 50100 "Sales Review Agent Upgrade" +{ + Subtype = Upgrade; + + trigger OnUpgradePerCompany() + var + Agent: Codeunit Agent; + UpgradeTag: Codeunit "Upgrade Tag"; + Instructions: SecretText; + AgentUserSecurityIds: List of [Guid]; + AgentUserSecurityId: Guid; + TagTxt: Label 'SALESREVIEW-INSTR-2.0.0', Locked = true; + InstructionsNameTxt: Label 'Instructions.txt', Locked = true; + begin + if UpgradeTag.HasUpgradeTag(TagTxt) then + exit; + Instructions := NavApp.GetResourceAsText(InstructionsNameTxt); + AgentUserSecurityIds := GetExistingAgentUserIds(); + foreach AgentUserSecurityId in AgentUserSecurityIds do + Agent.SetInstructions(AgentUserSecurityId, Instructions); + UpgradeTag.SetUpgradeTag(TagTxt); + end; + + local procedure GetExistingAgentUserIds() AgentUserSecurityIds: List of [Guid] + var + SalesReviewAgentSetup: Record "Sales Review Agent Setup"; + begin + if SalesReviewAgentSetup.FindSet() then + repeat + AgentUserSecurityIds.Add(SalesReviewAgentSetup."User Security ID"); + until SalesReviewAgentSetup.Next() = 0; + end; +} diff --git a/community/knowledge/agents/reapply-resource-instructions-on-upgrade.md b/community/knowledge/agents/reapply-resource-instructions-on-upgrade.md new file mode 100644 index 0000000..5345d25 --- /dev/null +++ b/community/knowledge/agents/reapply-resource-instructions-on-upgrade.md @@ -0,0 +1,26 @@ +--- +bc-version: [27..] +domain: agents +keywords: [upgrade, setinstructions, navapp-getresourceastext, existing-instances, upgrade-tag] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Reapply resource instructions to existing agent instances on upgrade + +## Description + +Static instructions stored as an app resource are copied onto an instance only when you call `SetInstructions`. Shipping a new `Instructions.txt` in version 2.0 does not update agents created under 1.0. Models change the resource and assume running instances pick it up. + +## Best Practice + +In the upgrade codeunit, find existing instances of your metadata provider and call `SetInstructions` again with `NavApp.GetResourceAsText`. Guard with an upgrade tag so the rewrite runs once per version that changes the file. + +See sample: `reapply-resource-instructions-on-upgrade.good.al`. + +## Anti Pattern + +Editing only the resource file, or calling `SetInstructions` solely from the first-time setup path. Detection signal: instruction resource in `resourceFolders` with no upgrade procedure that re-applies it. + +See sample: `reapply-resource-instructions-on-upgrade.bad.al`. diff --git a/community/knowledge/agents/register-copilot-capability-for-the-agent.bad.al b/community/knowledge/agents/register-copilot-capability-for-the-agent.bad.al new file mode 100644 index 0000000..eaf6440 --- /dev/null +++ b/community/knowledge/agents/register-copilot-capability-for-the-agent.bad.al @@ -0,0 +1,21 @@ +enumextension 50100 "Sales Review Agent Metadata" extends "Agent Metadata Provider" +{ + value(50100; "Sales Review Agent") + { + Caption = 'Sales Review Agent'; + Implementation = IAgentFactory = "Sales Review Agent Factory", + IAgentMetadata = "Sales Review Agent Metadata", + IAgentTaskExecution = "Sales Review Agent Task"; + } +} + +codeunit 50101 "Sales Review Agent Install" +{ + Subtype = Install; + Access = Internal; + + trigger OnInstallAppPerDatabase() + begin + // Agent type exists, but no Copilot Capability value and no RegisterCapability. + end; +} diff --git a/community/knowledge/agents/register-copilot-capability-for-the-agent.good.al b/community/knowledge/agents/register-copilot-capability-for-the-agent.good.al new file mode 100644 index 0000000..ba940d0 --- /dev/null +++ b/community/knowledge/agents/register-copilot-capability-for-the-agent.good.al @@ -0,0 +1,26 @@ +enumextension 50101 "Sales Review Agent Copilot" extends "Copilot Capability" +{ + value(50101; "Sales Review Agent") + { + Caption = 'Sales Review Agent'; + } +} + +codeunit 50101 "Sales Review Agent Install" +{ + Subtype = Install; + Access = Internal; + + trigger OnInstallAppPerDatabase() + var + CopilotCapability: Codeunit "Copilot Capability"; + LearnMoreUrlTxt: Label 'https://example.com/sales-review-agent', Locked = true; + begin + if not CopilotCapability.IsCapabilityRegistered(Enum::"Copilot Capability"::"Sales Review Agent") then + CopilotCapability.RegisterCapability( + Enum::"Copilot Capability"::"Sales Review Agent", + Enum::"Copilot Availability"::Preview, + Enum::"Copilot Billing Type"::"Microsoft Billed", + LearnMoreUrlTxt); + end; +} diff --git a/community/knowledge/agents/register-copilot-capability-for-the-agent.md b/community/knowledge/agents/register-copilot-capability-for-the-agent.md new file mode 100644 index 0000000..79dfe44 --- /dev/null +++ b/community/knowledge/agents/register-copilot-capability-for-the-agent.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [copilot-capability, registercapability, install, feature-switch, enumextension] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Register a Copilot capability for the agent on install + +## Description + +Each agent type needs a `Copilot Capability` enum value that the factory links as the feature switch and billing surface. The capability is invisible on Copilot and agent capabilities until an install codeunit calls `RegisterCapability` when it is not already registered. Unique ordinals matter across installed apps. Models often extend `Agent Metadata Provider` and never register the capability. + +## Best Practice + +Extend `Copilot Capability` with a unique value. In `OnInstallAppPerDatabase`, call `Copilot Capability.IsCapabilityRegistered` and, if false, `RegisterCapability` with availability, billing type, and a learn-more URL. Point `IAgentFactory` at that capability. + +See sample: `register-copilot-capability-for-the-agent.good.al`. + +## Anti Pattern + +Shipping the agent enum without a `Copilot Capability` value, or adding the enum but never calling `RegisterCapability`. Duplicate ordinals across extensions also collide. Detection signal: agent metadata provider with no matching capability registration in an install codeunit. + +See sample: `register-copilot-capability-for-the-agent.bad.al`. + +## See also + +`wire-all-three-agent-interfaces.md` covers registration of the provider implementation that references this capability. diff --git a/community/knowledge/agents/set-instructions-as-secrettext.bad.al b/community/knowledge/agents/set-instructions-as-secrettext.bad.al new file mode 100644 index 0000000..734f0b2 --- /dev/null +++ b/community/knowledge/agents/set-instructions-as-secrettext.bad.al @@ -0,0 +1,19 @@ +codeunit 50100 "Sales Review Agent Create" +{ + procedure CreateWithInstructions() + var + Agent: Codeunit Agent; + TempAgentAccessControl: Record "Agent Access Control" temporary; + AgentUserSecurityId: Guid; + InstructionsLbl: Label 'You are a sales validation agent. Check credit.', Locked = true; + begin + AgentUserSecurityId := Agent.Create( + Enum::"Agent Metadata Provider"::"Sales Review Agent", + 'SALESREVIEW', + 'Sales Review Agent', + TempAgentAccessControl); + // Label/text is not SecretText and is type-wide, not per instance. + Agent.SetInstructions(AgentUserSecurityId, InstructionsLbl); + Agent.Activate(AgentUserSecurityId); + end; +} diff --git a/community/knowledge/agents/set-instructions-as-secrettext.good.al b/community/knowledge/agents/set-instructions-as-secrettext.good.al new file mode 100644 index 0000000..8bedf69 --- /dev/null +++ b/community/knowledge/agents/set-instructions-as-secrettext.good.al @@ -0,0 +1,20 @@ +codeunit 50100 "Sales Review Agent Create" +{ + procedure CreateWithInstructions() + var + Agent: Codeunit Agent; + TempAgentAccessControl: Record "Agent Access Control" temporary; + AgentUserSecurityId: Guid; + Instructions: SecretText; + InstructionsNameTxt: Label 'Instructions.txt', Locked = true; + begin + AgentUserSecurityId := Agent.Create( + Enum::"Agent Metadata Provider"::"Sales Review Agent", + 'SALESREVIEW', + 'Sales Review Agent', + TempAgentAccessControl); + Instructions := NavApp.GetResourceAsText(InstructionsNameTxt); + Agent.SetInstructions(AgentUserSecurityId, Instructions); + Agent.Activate(AgentUserSecurityId); + end; +} diff --git a/community/knowledge/agents/set-instructions-as-secrettext.md b/community/knowledge/agents/set-instructions-as-secrettext.md new file mode 100644 index 0000000..0f246f6 --- /dev/null +++ b/community/knowledge/agents/set-instructions-as-secrettext.md @@ -0,0 +1,26 @@ +--- +bc-version: [27..] +domain: agents +keywords: [setinstructions, secrettext, instructions, per-instance, resource] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Set agent instructions as SecretText on the instance + +## Description + +Instructions are instance data, not an enum caption. `Agent.SetInstructions` takes `SecretText` so the payload is not logged or copied as ordinary text. A Label or plaintext Text on the agent type is the wrong store: it leaks into telemetry-friendly strings and cannot vary per instance or company. + +## Best Practice + +Load instruction text from a resource or builder into a `SecretText` variable and call `Agent.SetInstructions(AgentUserSecurityId, Instructions)` after `Create`. Keep one instruction document per instance. + +See sample: `set-instructions-as-secrettext.good.al`. + +## Anti Pattern + +Passing a `Label` or `Text` to `SetInstructions`, storing instructions in a setup Text field without wrapping as `SecretText`, or putting the prompt only in a code comment. Detection signal: `SetInstructions` with a non-`SecretText` argument, or no `SetInstructions` after `Create`. + +See sample: `set-instructions-as-secrettext.bad.al`. diff --git a/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.bad.al b/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.bad.al new file mode 100644 index 0000000..570ca09 --- /dev/null +++ b/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.bad.al @@ -0,0 +1,36 @@ +codeunit 50100 "Sales Review Agent Factory" +{ + procedure ShowCanCreateAgent(): Boolean + begin + // Author intends this to forbid all creates. It only hides the UI tile. + exit(false); + end; +} + +pageextension 50100 "Sales Order List Agent Create" extends "Sales Order List" +{ + actions + { + addlast(Processing) + { + action(CreateAgent) + { + ApplicationArea = All; + Caption = 'Create review agent'; + + trigger OnAction() + var + Agent: Codeunit Agent; + TempAgentAccessControl: Record "Agent Access Control" temporary; + begin + // Still succeeds for any caller with permission to run this action. + Agent.Create( + Enum::"Agent Metadata Provider"::"Sales Review Agent", + 'SALESREVIEW', + 'Sales Review Agent', + TempAgentAccessControl); + end; + } + } + } +} diff --git a/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.good.al b/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.good.al new file mode 100644 index 0000000..4601f1d --- /dev/null +++ b/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.good.al @@ -0,0 +1,25 @@ +codeunit 50100 "Sales Review Agent Factory" +{ + procedure ShowCanCreateAgent(): Boolean + var + AgentSystemPermissions: Codeunit "Agent System Permissions"; + begin + // Hides the type from non-admins in the UI. Does not block Agent.Create. + exit(AgentSystemPermissions.CurrentUserHasCanManageAllAgentsPermission()); + end; + + procedure CreateIfAllowed() + var + Agent: Codeunit Agent; + AgentSystemPermissions: Codeunit "Agent System Permissions"; + TempAgentAccessControl: Record "Agent Access Control" temporary; + begin + if not AgentSystemPermissions.CurrentUserHasCanManageAllAgentsPermission() then + Error('Only agent administrators can create this agent.'); + Agent.Create( + Enum::"Agent Metadata Provider"::"Sales Review Agent", + 'SALESREVIEW', + 'Sales Review Agent', + TempAgentAccessControl); + end; +} diff --git a/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.md b/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.md new file mode 100644 index 0000000..1eb151d --- /dev/null +++ b/community/knowledge/agents/show-can-create-agent-does-not-block-code-create.md @@ -0,0 +1,26 @@ +--- +bc-version: [28..] +domain: agents +keywords: [showcancreateagent, agent-discovery, agent-create, administrator, agent-configuration-rights] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# ShowCanCreateAgent only hides UI create, not programmatic create + +## Description + +`IAgentFactory.ShowCanCreateAgent` controls whether the type appears in the in-client create UI. Returning false does not stop `Agent.Create` from AL. From 28.1, non-admins can discover extension agents unless this method (and agent configuration rights) restrict them. Models treat a false return as a hard create lock. + +## Best Practice + +Use `ShowCanCreateAgent` to decide discovery. If only agent administrators should see the type, return `Agent System Permissions.CurrentUserHasCanManageAllAgentsPermission`. Enforce extra policy inside your own create API. Never assume UI hiding blocks code. + +See sample: `show-can-create-agent-does-not-block-code-create.good.al`. + +## Anti Pattern + +Returning `exit(false)` from `ShowCanCreateAgent` and then documenting that instances cannot be created, while page actions or other apps still call `Agent.Create`. Detection signal: `ShowCanCreateAgent` always false with no matching guard on programmatic create. + +See sample: `show-can-create-agent-does-not-block-code-create.bad.al`. diff --git a/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.bad.al b/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.bad.al new file mode 100644 index 0000000..8a184c6 --- /dev/null +++ b/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.bad.al @@ -0,0 +1,15 @@ +codeunit 50100 "Sales Review Agent Tasks" +{ + procedure EnqueueFromEmailBody(RawEmailBody: Text; AgentUserSecurityId: Guid) + var + AgentTaskBuilder: Codeunit "Agent Task Builder"; + AgentTaskMessageBuilder: Codeunit "Agent Task Message Builder"; + AgentTask: Record "Agent Task"; + begin + AgentTaskMessageBuilder.Initialize('Internet', RawEmailBody) + .SetRequiresReview(false); + AgentTask := AgentTaskBuilder.Initialize(AgentUserSecurityId, 'Process inbound mail') + .AddTaskMessage(AgentTaskMessageBuilder) + .Create(); + end; +} diff --git a/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.good.al b/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.good.al new file mode 100644 index 0000000..394aeb2 --- /dev/null +++ b/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.good.al @@ -0,0 +1,16 @@ +codeunit 50100 "Sales Review Agent Tasks" +{ + procedure EnqueueFromSalesOrder(SalesHeader: Record "Sales Header"; AgentUserSecurityId: Guid) + var + AgentTaskBuilder: Codeunit "Agent Task Builder"; + AgentTaskMessageBuilder: Codeunit "Agent Task Message Builder"; + AgentTask: Record "Agent Task"; + begin + SalesHeader.TestField("No."); + AgentTaskMessageBuilder.Initialize('Sales Team', 'Review sales order ' + SalesHeader."No.") + .SetRequiresReview(false); + AgentTask := AgentTaskBuilder.Initialize(AgentUserSecurityId, 'Review Sales Order') + .AddTaskMessage(AgentTaskMessageBuilder) + .Create(); + end; +} diff --git a/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.md b/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.md new file mode 100644 index 0000000..449038f --- /dev/null +++ b/community/knowledge/agents/skip-incoming-review-only-for-trusted-input.md @@ -0,0 +1,26 @@ +--- +bc-version: [28..] +domain: agents +keywords: [setrequiresreview, agent-task-message-builder, approval, trusted-input, skip-review] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Skip incoming message review only after the caller validated the payload + +## Description + +Incoming task messages default to requiring user approval before the agent runs. From 28.1, `Agent Task Message Builder.SetRequiresReview(false)` starts the agent immediately. That is safe only for inputs you already validated in AL (your page action, your posting subscriber). External email or partner payloads are not trusted by default. Analysis Warnings still force a review. + +## Best Practice + +Leave the default review-on for anything that originated outside your extension. Call `SetRequiresReview(false)` only on messages you constructed from already-authorized BC data. + +See sample: `skip-incoming-review-only-for-trusted-input.good.al`. + +## Anti Pattern + +`SetRequiresReview(false)` on simulated email, incoming webhooks, or user-free text. Detection signal: `SetRequiresReview(false)` next to external content with no prior validation. + +See sample: `skip-incoming-review-only-for-trusted-input.bad.al`. diff --git a/community/knowledge/agents/use-documented-instruction-keywords.bad.al b/community/knowledge/agents/use-documented-instruction-keywords.bad.al new file mode 100644 index 0000000..0854a3d --- /dev/null +++ b/community/knowledge/agents/use-documented-instruction-keywords.bad.al @@ -0,0 +1,7 @@ +codeunit 50100 "Sales Review Agent Instr." +{ + procedure GetInstructions() Instructions: SecretText + begin + Instructions := 'When done, email the customer and remember the credit limit. Click Post_Promoted.'; + end; +} diff --git a/community/knowledge/agents/use-documented-instruction-keywords.good.al b/community/knowledge/agents/use-documented-instruction-keywords.good.al new file mode 100644 index 0000000..e03adee --- /dev/null +++ b/community/knowledge/agents/use-documented-instruction-keywords.good.al @@ -0,0 +1,13 @@ +codeunit 50100 "Sales Review Agent Instr." +{ + procedure GetInstructions() Instructions: SecretText + var + Builder: TextBuilder; + begin + Builder.AppendLine('When the sales order is ready, request a review before posting.'); + Builder.AppendLine('If a field is missing, ask for assistance.'); + Builder.AppendLine('Memorize the customer credit limit for later steps.'); + Builder.AppendLine('When confirmed, write an email to the salesperson; outbound mail is reviewed.'); + Instructions := Builder.ToText(); + end; +} diff --git a/community/knowledge/agents/use-documented-instruction-keywords.md b/community/knowledge/agents/use-documented-instruction-keywords.md new file mode 100644 index 0000000..2150a5d --- /dev/null +++ b/community/knowledge/agents/use-documented-instruction-keywords.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [instruction-keywords, request-a-review, memorize, write-an-email, invoke-action] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Use the toolkit instruction keywords for review, mail, and memory + +## Description + +The agent runtime looks for specific phrases: ask for assistance, request a review, reply, write an email, memorize, `Set field`, use lookup, `Invoke action`. Ordinary English such as get a human to look or remember this is weaker. Outbound reply and email always require review; that is platform policy, not optional tone. + +## Best Practice + +In the instruction resource, use those keywords at the decision points: request a review before posting; write an email only after stating that outbound mail is reviewed; memorize values the later steps need. Pair `Reply` / `Write an email` with an explicit review sentence. + +See sample: `use-documented-instruction-keywords.good.al`. + +## Anti Pattern + +Inventing tool-like verbs (call Copilot, click Post_Promoted) or omitting request a review before posting. Detection signal: instruction text that says email the customer with no review keyword. + +See sample: `use-documented-instruction-keywords.bad.al`. + +## See also + +`instruction-structure-is-role-rules-steps.md` defines the containing document structure, while `instructions-describe-work-not-tool-ids.md` keeps outcomes independent of UI tool identifiers. diff --git a/community/knowledge/agents/wire-all-three-agent-interfaces.bad.al b/community/knowledge/agents/wire-all-three-agent-interfaces.bad.al new file mode 100644 index 0000000..e4d4bcb --- /dev/null +++ b/community/knowledge/agents/wire-all-three-agent-interfaces.bad.al @@ -0,0 +1,9 @@ +enumextension 50100 "Sales Review Agent Metadata" extends "Agent Metadata Provider" +{ + value(50100; "Sales Review Agent") + { + Caption = 'Sales Review Agent'; + // Only factory is bound. Metadata UI and task execution never resolve. + Implementation = IAgentFactory = "Sales Review Agent Factory"; + } +} diff --git a/community/knowledge/agents/wire-all-three-agent-interfaces.good.al b/community/knowledge/agents/wire-all-three-agent-interfaces.good.al new file mode 100644 index 0000000..bd13f65 --- /dev/null +++ b/community/knowledge/agents/wire-all-three-agent-interfaces.good.al @@ -0,0 +1,10 @@ +enumextension 50100 "Sales Review Agent Metadata" extends "Agent Metadata Provider" +{ + value(50100; "Sales Review Agent") + { + Caption = 'Sales Review Agent'; + Implementation = IAgentFactory = "Sales Review Agent Factory", + IAgentMetadata = "Sales Review Agent Meta. Impl.", + IAgentTaskExecution = "Sales Review Agent Task"; + } +} diff --git a/community/knowledge/agents/wire-all-three-agent-interfaces.md b/community/knowledge/agents/wire-all-three-agent-interfaces.md new file mode 100644 index 0000000..d3ce4b2 --- /dev/null +++ b/community/knowledge/agents/wire-all-three-agent-interfaces.md @@ -0,0 +1,30 @@ +--- +bc-version: [27..] +domain: agents +keywords: [agent-metadata-provider, iagentfactory, iagentmetadata, iagenttaskexecution, enumextension, implementation] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Wire all three agent interfaces on the metadata provider + +## Description + +An AL agent type is registered by extending `Agent Metadata Provider`. The platform locates factory, metadata, and task-execution behaviour only through the `Implementation` property on that enum value. Omitting `IAgentFactory`, `IAgentMetadata`, or `IAgentTaskExecution` leaves create, UI identity, or task runs unbound. Models often ship a single codeunit and skip the enum wiring. + +## Best Practice + +On the enum value, set `Implementation` for all three interfaces, each pointing at a dedicated codeunit. Keep factory (create, defaults, first-time setup), metadata (setup page, summary, annotations), and task execution (message analysis, intervention suggestions) in separate objects. + +See sample: `wire-all-three-agent-interfaces.good.al`. + +## Anti Pattern + +An `Agent Metadata Provider` value with no `Implementation`, only one interface mapped, or all three interfaces pointing at one catch-all codeunit that cannot satisfy the contracts. Detection signal: enumextension of `Agent Metadata Provider` whose value does not list `IAgentFactory`, `IAgentMetadata`, and `IAgentTaskExecution`. + +See sample: `wire-all-three-agent-interfaces.bad.al`. + +## See also + +`register-copilot-capability-for-the-agent.md` covers the feature capability linked by the factory implementation. diff --git a/community/skills/review/al-agents-review.md b/community/skills/review/al-agents-review.md new file mode 100644 index 0000000..4bc2a6b --- /dev/null +++ b/community/skills/review/al-agents-review.md @@ -0,0 +1,70 @@ +--- +kind: action-skill +id: al-agents-review +version: 1 +title: AL agents review +description: Reviews AL source changes against agent guidance from BCQuality. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL agents review + +Reviews AL source changes against the `agents` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is not one of the skills composed by `al-code-review`; Entry discovers and dispatches it as a top-level peer, so it produces an independent findings report. + +Agent findings apply to AL files that implement or invoke Agent SDK surfaces, including agent interfaces, setup, creation, task execution, capability registration, profiles, access controls, instructions, and session-bound subscribers. Return `not-applicable` when the diff contains no AL changes or no Agent SDK implementation or usage. + +An orchestrator invokes this skill with either a `pr-diff` or a `file-path`. The skill produces one JSON document conforming to the DO output contract. + +## Source + +Read the root `knowledge-index.json` generated by Entry and select entries whose `domain` is `agents` across every enabled layer. Use index metadata for candidate selection and open an article body only after it enters the worklist. + +## Relevance + +Apply READ's frontmatter matching semantics to the target BC version, AL technology, countries, and application areas. If a dimension is unknown, retain conditionally applicable guidance only when configuration permits it; cap resulting confidence at `medium` and name the unknown dimension in the finding message. + +## Worklist + +Match changed objects, procedures, interfaces, and tokens against article keywords, titles, descriptions, and paths. Give particular weight to: + +- Implementations of `IAgentFactory`, `IAgentMetadata`, and `IAgentTaskExecution`. +- Agent setup tables and `ConfigurationDialog` pages using `Agent Setup`, `Agent Setup Buffer`, or `Agent Setup Part`. +- Agent creation, upgrade, capability registration, profile configuration, access controls, and subscriber binding. +- Instruction construction, `SecretText`, documented instruction keywords, task messages, trusted input, warnings, errors, and review behavior. +- Public APIs invoked by agent tasks across app boundaries. + +Use these targeted rules to avoid broad token-only matches: + +- Worklist setup-page shape guidance when the page returned by agent metadata is not a `ConfigurationDialog` or omits `Agent Setup Part`. +- Worklist temporary-source guidance when setup writes occur before a non-Cancel close path or a setup page is not temporary. +- Worklist permission guidance when default access controls are broad or when code assumes an agent can exceed the assigning user's permissions. +- Worklist instruction guidance only for text used as agent instructions; do not flag unrelated prompts, labels, or user-facing help. +- Worklist session-binding guidance only when subscribers are bound outside an agent session or left bound after execution. + +After selection, resolve conflicting guidance using READ's layer precedence. Record displaced candidates in `suppressed` with `reason: "layer-precedence"`; record disabled-layer candidates with `reason: "configuration"`. + +An empty worklist caused by absent applicable knowledge produces `no-knowledge`. An empty worklist caused by no match produces `completed` with no findings. + +## Action + +Evaluate each worklisted article's `## Best Practice` and `## Anti Pattern` against the changed code: + +- Emit `major` for a clear anti-pattern and `minor` for a concrete best-practice contradiction. +- Use `blocker` only when the article identifies a violated platform guarantee. +- Do not emit a finding from applicability alone. +- Set confidence to `high` for unambiguous syntax or identifier evidence, `medium` for heuristic or conditionally applicable evidence, and `low` only for an explicit advisory. + +Agent-originated findings without a matching article must follow the DO contract: prefix the ID with `agent:`, use `references: []`, cap severity at `minor` and confidence at `medium`, and emit only concrete defects within the agents domain. + +Provide `suggested-code` when the repair is small, local, and unambiguous. Otherwise, when a mechanical-looking repair depends on missing context or has multiple valid forms, set `suggested-code-omission-reason`. + +Use the standard DO outcomes: `completed`, `no-knowledge`, `not-applicable`, `partial`, or `failed`. + +## Output + +Return only one JSON document conforming to the DO output contract. Every finding emitted by this skill MUST set `findings[].domain` to `"Agents"`. Knowledge-backed finding IDs and references MUST use the exact repository-relative article path from the knowledge index. \ No newline at end of file diff --git a/evaluation/README.md b/evaluation/README.md index cd25d3d..7063baf 100644 --- a/evaluation/README.md +++ b/evaluation/README.md @@ -1,6 +1,6 @@ # 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. +The evaluation is convention-driven. The harness discovers every `/skills/review/al--review.md` leaf across the enabled `microsoft`, `community`, and `custom` layers. Duplicate domains resolve with `custom > community > microsoft` precedence. For each selected leaf, the harness finds paired knowledge across the same layers, applies the same precedence to duplicate article slugs, 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. @@ -12,7 +12,7 @@ Model-facing preparation hashes case IDs, neutralizes `Good`/`Bad` object-name t 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. +This credential-free check proves every selected 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 @@ -26,7 +26,7 @@ This credential-free check proves every registered leaf maps to a same-named kno 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. + `request-.json` files provide optional two-case leaf batches and identify the selected layer-owned skill path; 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: diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index 2f85d5c..f5c7046 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -4,6 +4,9 @@ "minimumExpectedRecall": 1.0, "minimumCleanRate": 1.0, "overrides": { + "agents": { + "article": "wire-all-three-agent-interfaces" + }, "appsource": { "context": "AppSourceCop mandatoryAffixes is configured to ABC." }, @@ -11,7 +14,7 @@ "article": "do-not-expose-sensitive-data-through-public-api" }, "events": { - "article": "initialize-ishandled-to-false-before-publishing" + "article": "reset-ishandled-only-when-the-value-can-carry-over" }, "interfaces": { "article": "set-defaultimplementation-on-enum" @@ -28,6 +31,9 @@ "telemetry": { "article": "telemetry-event-id-stable-unique" }, + "testing": { + "article": "ui-handlers-in-tests" + }, "upgrade": { "article": "initvalue-does-not-update-existing-rows", "context": "The extended table existed in the previous app version and already contains rows." diff --git a/microsoft/knowledge/appsource/object-affixes-prevent-collisions.md b/microsoft/knowledge/appsource/object-affixes-prevent-collisions.md index a46c0d1..a3542a1 100644 --- a/microsoft/knowledge/appsource/object-affixes-prevent-collisions.md +++ b/microsoft/knowledge/appsource/object-affixes-prevent-collisions.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: appsource -keywords: [object-affix, prefix, suffix, as0011, appsourcecop, collision, tableextension] +keywords: [object-affix, prefix, suffix, as0011, appsourcecop, collision, tableextension, first-party, isv] technologies: [al] countries: [w1] application-area: [all] @@ -15,6 +15,8 @@ An AppSource extension must prevent name collisions through its registered affix 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. +This rule scopes to Marketplace ISV extensions, which is what AppSourceCop validates. A first-party Microsoft in-box module (publisher `Microsoft`, an object range reserved for first-party use, and no `AppSourceCop.json`/`mandatoryAffixes` in the app) is not built or shipped as an Marketplace extension and is not subject to AS0011, so an unaffixed action or field it adds to a base-application page is not a collision risk to flag. Renaming an existing shipped first-party member to add an affix is itself a breaking change to that module's own history and is not required by this rule. + ## Best Practice 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). 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 index 509fe6f..8e46049 100644 --- 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 @@ -1,7 +1,7 @@ --- bc-version: [23..] domain: appsource -keywords: [namespace, two-level, affix, prefix, suffix, as0011, tableextension, pageextension] +keywords: [namespace, two-level, affix, prefix, suffix, as0011, tableextension, pageextension, false-positive] technologies: [al] countries: [w1] application-area: [all] @@ -13,14 +13,16 @@ application-area: [all] 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. +The requirement comes from AppSourceCop rule AS0011, which only runs when the app enables AppSourceCop and configures a mandatory affix — normally an `AppSourceCop.json` next to the app manifest. An app that ships no such configuration is not subject to AS0011, and its extension members are not a compliance gap. This is the usual situation for first-party, in-box apps that ship as part of the product rather than through AppSource: their uniqueness comes from allocated object ID ranges and a controlled source tree, not from a registered affix. Confirm the extending app actually configures a mandatory affix before reporting an unaffixed extension member. + ## 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. +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. Do not raise a missing member affix against an app that does not enable AppSourceCop with a mandatory affix; there AS0011 never fires, and the app's namespace is not the reason — the absent configuration is. 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. +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` in an app that does configure a mandatory affix. The namespace distinguishes the extension's own objects; it cannot disambiguate members on Customer. The mirror-image mistake is reporting an unaffixed extension member in an app that enables no mandatory affix at all — AS0011 does not apply there, and the finding is a false positive. See sample: `two-level-namespace-replaces-object-affix-not-extension-member-affix.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md index a557d41..75fa6c1 100644 --- a/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md +++ b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: breaking-changes -keywords: [signature, public-procedure, parameter, return-value, overload, contract] +keywords: [signature, public-procedure, parameter, return-value, overload, contract, integration-event] technologies: [al] countries: [w1] application-area: [all] @@ -13,6 +13,8 @@ application-area: [all] A procedure that is reachable from outside its object — any procedure not marked `local` (and, for on-prem-scoped code, anything a dependent app can still bind to) — is a contract. Once another extension compiles against it, changing its shape breaks that extension at build time. Signature changes include adding, removing, or reordering parameters, changing a parameter or return type, and toggling a parameter between by-value and `var` (by-reference). The platform treats the procedure's identity as its full signature, so even a "compatible-looking" tweak is a new method to dependents. There is exactly one safe edit: naming a previously unnamed return value, which adds no caller obligation. LLMs routinely "improve" a public procedure in place by adding a parameter, not realizing every consumer must be recompiled. +This rule governs procedures that dependents *call*. An event publisher — a procedure carrying `[IntegrationEvent]` or `[BusinessEvent]`, conventionally declared `local` — is bound to, not called, and AL binds each subscriber parameter by name rather than by position. Adding a parameter to a shipped event therefore leaves every existing subscriber binding successfully, at any position in the list, so it is additive rather than breaking and must not be flagged under this rule. See `events/adding-a-parameter-to-an-event-is-not-a-breaking-change` for the full treatment, and `events/add-new-event-parameters-at-the-end` for when publisher access does make the addition breaking. Every other edit to a published event signature — removing or retyping a parameter, renaming one, or flipping one to or from `var` — still breaks subscribers, and `events/treat-local-and-internal-events-as-subscriber-contracts` owns that case together with the analyzer rules that enforce it: AS0025 for parameter names and types, AS0063 for removing `var`, and AS0077 for adding it. Adding `var IsHandled: Boolean` is a separate concern: it binds fine but changes the event's contract, and is covered by `events/do-not-add-ishandled-to-an-existing-event`. + ## Best Practice Treat a published signature as frozen. When new behavior needs more inputs, add a new procedure or overload alongside the original — for example a `CalculateDiscountWithRate(Amount; Rate)` next to the unchanged `CalculateDiscount(Amount)` — and let the old one delegate to the new one. Existing callers keep compiling; new callers opt into the richer entry point. Naming an unnamed return value is the one in-place change that is always safe. @@ -21,6 +23,6 @@ See sample: `do-not-change-published-procedure-signatures.good.al`. ## Anti Pattern -Editing the existing public procedure's parameter list — here, adding a `Rate` parameter to `CalculateDiscount` — so every dependent extension that called the old form fails to compile. Detection: a parameter added, removed, reordered, retyped, or flipped to/from `var`, or a changed return type, on any non-`local` procedure that already shipped. Add a new overload instead. +Editing the existing public procedure's parameter list — here, adding a `Rate` parameter to `CalculateDiscount` — so every dependent extension that called the old form fails to compile. Detection: a parameter added, removed, reordered, retyped, or flipped to/from `var`, or a changed return type, on any non-`local` procedure that already shipped. Add a new overload instead. Exclude event publishers whose only change is an added parameter: subscribers bind by parameter name, not position, so that edit is additive and reporting it here is a false positive. See sample: `do-not-change-published-procedure-signatures.bad.al`. diff --git a/microsoft/knowledge/data-modeling/insert-only-transfer-may-rely-on-caller-cleanup.md b/microsoft/knowledge/data-modeling/insert-only-transfer-may-rely-on-caller-cleanup.md new file mode 100644 index 0000000..be1b784 --- /dev/null +++ b/microsoft/knowledge/data-modeling/insert-only-transfer-may-rely-on-caller-cleanup.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [transfer, cleanup, deleteall, onvalidate, caller, stale-rows, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# An insert-only transfer routine may rely on cleanup its caller already performed + +## Description + +A routine that copies rows from a template table into a target table — a `TransferX` procedure filling comment, dimension, or attribute lines from a standard-task or template record — is frequently written as filter-and-insert with no `DeleteAll` of its own. That is not automatically a stale-row or duplicate-primary-key defect. In the common AL shape, the field's `OnValidate` trigger first calls a sibling cleanup procedure that clears the same filtered range, then calls the transfer. By the time `Insert` runs, the target range is guaranteed empty, so the transfer has nothing to clean up and adding a second `DeleteAll` inside it would be redundant. + +Deciding whether a missing cleanup is real therefore requires reading the caller, not just the routine in the diff. The relevant question is whether every path that reaches the transfer clears the target range first — not whether the transfer clears it itself. + +## Best Practice + +Before reporting a transfer or copy routine for missing cleanup, trace its call sites. If the callers in scope invoke a cleanup procedure that clears the same filtered range immediately beforehand — typically in the same `OnValidate` trigger or the same routine — the insert-only transfer is correct and must not be flagged for stale rows, duplicate keys, or a missing `DeleteAll`. Raise the finding only when a reachable call path inserts into a range that was not cleared, or when the cleanup filters a different range than the insert writes to. + +## Anti Pattern + +Reporting an insert-only transfer as a stale-row or duplicate-key risk on the strength of the routine body alone, when the trigger that calls it already ran the cleanup. The mirror-image mistake is waving through a transfer whose caller clears a *different* filter range than the one the transfer inserts into, or one reachable from a path with no cleanup at all — those are genuine defects. diff --git a/community/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.bad.al b/microsoft/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.bad.al similarity index 100% rename from community/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.bad.al rename to microsoft/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.bad.al diff --git a/community/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.good.al b/microsoft/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.good.al similarity index 100% rename from community/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.good.al rename to microsoft/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.good.al diff --git a/community/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.md b/microsoft/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.md similarity index 100% rename from community/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.md rename to microsoft/knowledge/data-modeling/owning-table-must-delete-dependents-in-ondelete.md diff --git a/community/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.bad.al b/microsoft/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.bad.al similarity index 100% rename from community/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.bad.al rename to microsoft/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.bad.al diff --git a/community/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.good.al b/microsoft/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.good.al similarity index 100% rename from community/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.good.al rename to microsoft/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.good.al diff --git a/community/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.md b/microsoft/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.md similarity index 100% rename from community/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.md rename to microsoft/knowledge/data-modeling/transferfields-skip-type-mismatch-can-drop-data.md diff --git a/community/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.bad.al b/microsoft/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.bad.al similarity index 100% rename from community/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.bad.al rename to microsoft/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.bad.al diff --git a/community/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.good.al b/microsoft/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.good.al similarity index 100% rename from community/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.good.al rename to microsoft/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.good.al diff --git a/community/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.md b/microsoft/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.md similarity index 100% rename from community/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.md rename to microsoft/knowledge/data-modeling/validate-table-relation-false-suppresses-rename-propagation.md diff --git a/community/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.bad.al b/microsoft/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.bad.al similarity index 100% rename from community/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.bad.al rename to microsoft/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.bad.al diff --git a/community/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.good.al b/microsoft/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.good.al similarity index 100% rename from community/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.good.al rename to microsoft/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.good.al diff --git a/community/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.md b/microsoft/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.md similarity index 100% rename from community/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.md rename to microsoft/knowledge/data-modeling/xrec-is-a-before-image-only-in-some-triggers.md diff --git a/microsoft/knowledge/error-handling/fielderror-vs-testfield.md b/microsoft/knowledge/error-handling/fielderror-vs-testfield.md index 1353c03..9b58b32 100644 --- a/microsoft/knowledge/error-handling/fielderror-vs-testfield.md +++ b/microsoft/knowledge/error-handling/fielderror-vs-testfield.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: error-handling -keywords: [fielderror, testfield, field-validation, onvalidate, error-message, mandatory-field, record-context] +keywords: [fielderror, testfield, field-validation, onvalidate, error-message, mandatory-field, record-context, onaction, enabled-property] technologies: [al] countries: [w1] application-area: [all] @@ -14,6 +14,8 @@ 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. +A page action's `OnAction` trigger is a different case: a page action is only invocable through its own UI control, so when the action's `Enabled` property is already bound to the same condition the trigger would otherwise `TestField`, the control cannot be clicked while the field is blank and the field can never reach the trigger empty. Adding a `TestField` there is redundant defensive code, not a missing check — flag it only when the trigger can run through a path `Enabled` does not cover (a shared procedure, an API, or a condition broader than what gates the action). + See sample: `fielderror-vs-testfield.good.al`. ## Anti Pattern diff --git a/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.bad.al b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.bad.al index 4dfce97..5b4c1c0 100644 --- a/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.bad.al +++ b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.bad.al @@ -3,7 +3,7 @@ codeunit 50116 "Payment Processor Bad" { [IntegrationEvent(false, false)] - procedure OnBeforeSubmitPayment(var PaymentAmount: Decimal; var Cancel: Boolean) + local procedure OnBeforeSubmitPayment(var PaymentAmount: Decimal; var Cancel: Boolean) begin end; diff --git a/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.good.al b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.good.al index 7c76303..10f331c 100644 --- a/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.good.al +++ b/microsoft/knowledge/events/avoid-raising-events-inside-try-functions.good.al @@ -3,7 +3,7 @@ codeunit 50114 "Payment Processor" { [IntegrationEvent(false, false)] - procedure OnBeforeSubmitPayment(var PaymentAmount: Decimal; var Cancel: Boolean) + local procedure OnBeforeSubmitPayment(var PaymentAmount: Decimal; var Cancel: Boolean) begin end; diff --git a/microsoft/knowledge/events/declare-event-publishers-local-or-internal.bad.al b/microsoft/knowledge/events/declare-event-publishers-local-or-internal.bad.al new file mode 100644 index 0000000..faf873c --- /dev/null +++ b/microsoft/knowledge/events/declare-event-publishers-local-or-internal.bad.al @@ -0,0 +1,43 @@ +// Demonstration only. Shows the wrong pattern: the publisher carries no access modifier, so it is +// public - which never was what lets extensions subscribe. + +codeunit 50100 "Loyalty Points Mgt Bad" +{ + procedure AwardPoints(CustomerNo: Code[20]; SalesAmount: Decimal) + var + Points: Decimal; + IsHandled: Boolean; + begin + Points := SalesAmount / 10; + + IsHandled := false; + OnBeforeAwardPoints(CustomerNo, Points, IsHandled); + if IsHandled then + exit; + + // ... insert the loyalty entry ... + end; + + // BAD: no access modifier, so this publisher is public. Public access does not enable + // subscription - it enables raising. Narrowing it to internal after release breaks callers, + // so the widening cannot be walked back cheaply. + [IntegrationEvent(false, false)] + procedure OnBeforeAwardPoints(CustomerNo: Code[20]; var Points: Decimal; var IsHandled: Boolean) + begin + end; +} + +codeunit 50101 "Loyalty Points Caller Bad" +{ + procedure FirePublisherDirectly(CustomerNo: Code[20]) + var + LoyaltyPointsMgt: Codeunit "Loyalty Points Mgt Bad"; + Points: Decimal; + IsHandled: Boolean; + begin + // Compiles only because the publisher is public. Every subscriber runs although no points + // were ever awarded, on a Points value nobody computed, and the IsHandled answer the + // subscribers write is read by no one. + LoyaltyPointsMgt.OnBeforeAwardPoints(CustomerNo, Points, IsHandled); + end; +} diff --git a/microsoft/knowledge/events/declare-event-publishers-local-or-internal.good.al b/microsoft/knowledge/events/declare-event-publishers-local-or-internal.good.al new file mode 100644 index 0000000..70064e5 --- /dev/null +++ b/microsoft/knowledge/events/declare-event-publishers-local-or-internal.good.al @@ -0,0 +1,59 @@ +// Demonstration only. Shows the correct pattern: a public facade codeunit whose event publishers +// are internal, so only the implementation codeunit decides when they fire. + +codeunit 50100 "Loyalty Points Mgt Good" +{ + procedure AwardPoints(CustomerNo: Code[20]; SalesAmount: Decimal) + var + LoyaltyPointsImpl: Codeunit "Loyalty Points Impl Good"; + begin + LoyaltyPointsImpl.AwardPoints(CustomerNo, SalesAmount); + end; + + // internal, not public: the implementation codeunit raises this and nobody else. Subscribers + // bind through Codeunit::"Loyalty Points Mgt Good", which is public by default - that object + // access is all a subscriber in another extension needs. + [IntegrationEvent(false, false)] + internal procedure OnBeforeAwardPoints(CustomerNo: Code[20]; var Points: Decimal; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + internal procedure OnAfterAwardPoints(CustomerNo: Code[20]; Points: Decimal) + begin + end; +} + +codeunit 50101 "Loyalty Points Impl Good" +{ + Access = Internal; + + procedure AwardPoints(CustomerNo: Code[20]; SalesAmount: Decimal) + var + LoyaltyPointsMgt: Codeunit "Loyalty Points Mgt Good"; + Points: Decimal; + IsHandled: Boolean; + begin + Points := SalesAmount / 10; + + IsHandled := false; + LoyaltyPointsMgt.OnBeforeAwardPoints(CustomerNo, Points, IsHandled); + if IsHandled then + exit; + + // ... insert the loyalty entry ... + + LoyaltyPointsMgt.OnAfterAwardPoints(CustomerNo, Points); + end; +} + +codeunit 50102 "Loyalty Points Sub Good" +{ + // The shape a subscriber in a dependent extension takes: it names the public object, and is + // indifferent to the publisher being internal. + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Loyalty Points Mgt Good", 'OnAfterAwardPoints', '', false, false)] + local procedure LogAwardedPointsOnAfterAwardPoints(CustomerNo: Code[20]; Points: Decimal) + begin + // ... write telemetry ... + end; +} diff --git a/microsoft/knowledge/events/declare-event-publishers-local-or-internal.md b/microsoft/knowledge/events/declare-event-publishers-local-or-internal.md new file mode 100644 index 0000000..9d097b4 --- /dev/null +++ b/microsoft/knowledge/events/declare-event-publishers-local-or-internal.md @@ -0,0 +1,42 @@ +--- +bc-version: [all] +domain: events +keywords: [event-publisher, access-modifier, local, internal, integration-event, business-event, subscriber, breaking-change] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Declare event publishers local or internal + +> Contributions welcome — open a PR to refine or extend this article. + +## Description + +The access modifier on an `[IntegrationEvent]` or `[BusinessEvent]` publisher controls who may *raise* the procedure, not who may *subscribe* to it. A subscriber in a dependent extension binds through the object named in its `[EventSubscriber(...)]` attribute, so the only accessibility a foreign subscriber needs is on the *object* — a codeunit left at its default public access. The publisher procedure itself can and should stay `local` or `internal`. Publishing an event is an invitation to subscribe, not an invitation to call: an omitted access modifier makes the publisher public, which hands every dependent extension the ability to fire the event on its own. The signature-compatibility consequences of a shipped publisher are covered separately by `treat-local-and-internal-events-as-subscriber-contracts`. + +## Applies to + +Ordinary `[IntegrationEvent]` and `[BusinessEvent]` publishers. `[InternalEvent]` has its own module-only visibility semantics, and external business events are out of scope. + +## Best Practice + +Give an event publisher the narrowest access modifier that still lets the code owning the operation raise it: + +- `local` when only the declaring object raises the event. This is the common case and the default choice. +- `internal` when another object in the same app raises it — typically an internal implementation codeunit raising an event declared on a public facade codeunit. The facade object stays public so dependent extensions can name it in `[EventSubscriber(...)]`; the publisher stays `internal` so only the implementation decides when the event fires. +- `public` only when a *different app* must raise the event — a hub or event-bus codeunit in a foundation app that sibling apps signal through, where `internal` cannot reach across the app boundary. This is a deliberate caller contract, not a concession to subscribers, and it is maintained like any other public API. + +Subscribers are unaffected by any of these choices. A non-public publisher also keeps the freedom to add a parameter later, which a public publisher gives up — see `add-new-event-parameters-at-the-end`. + +See sample: `declare-event-publishers-local-or-internal.good.al`. + +## Anti Pattern + +An event publisher declared with no access modifier — or widened to public — in the belief that dependent extensions need that to subscribe. They do not. Two consequences follow. Any dependent extension can now call the publisher directly, firing every subscriber outside the owning routine's control flow, on state the publisher never prepared and with an `IsHandled` answer nobody reads. And because the publisher is a public procedure, it is a caller contract: narrowing it back to `local` or `internal` after release is itself a breaking change, so the mistake is not cheaply reversible. + +Detection: an `[IntegrationEvent]` or `[BusinessEvent]` publisher that is public although every raiser is in its own app — typically raised only from its declaring object. A publisher deliberately made public so another app can raise it is not this anti-pattern; do not report it. When the surrounding repository or API context does not reveal whether an external raiser is intended, treat the public modifier as intentional rather than reporting it. + +The mirror-image anti-pattern belongs to the reviewer, human or agent: recommending that a publisher be made public so extensions can subscribe, or reporting a `local`/`internal` publisher as unreachable dead code. Both readings mistake raising for subscribing. Neither should be raised as a finding. + +See sample: `declare-event-publishers-local-or-internal.bad.al`. diff --git a/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.bad.al b/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.bad.al new file mode 100644 index 0000000..ff2b277 --- /dev/null +++ b/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.bad.al @@ -0,0 +1,72 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. + +// Anti-pattern 1: the context is kept in single-instance state. +codeunit 50545 "Process State Bad Sample" +{ + SingleInstance = true; + + var + ProcessRunning: Boolean; + + procedure SetProcessRunning(NewProcessRunning: Boolean) + begin + ProcessRunning := NewProcessRunning; + end; + + procedure IsProcessRunning(): Boolean + begin + exit(ProcessRunning); + end; +} + +codeunit 50546 "Process Driver Bad Sample" +{ + procedure Run(DocumentNo: Code[20]) + var + ProcessState: Codeunit "Process State Bad Sample"; + begin + ProcessState.SetProcessRunning(true); + RunSharedCode(DocumentNo); + // An error above never reaches this line. The database writes roll + // back, the single-instance variable does not: ProcessRunning stays + // true until the company is closed, so every later run in this session + // is treated as part of the process. + ProcessState.SetProcessRunning(false); + end; + + local procedure RunSharedCode(DocumentNo: Code[20]) + begin + end; +} + +// Anti-pattern 2: the context stays private. Flag and driver look like the +// good sample, but the query is internal, so only the owning app can ever ask. +codeunit 50547 "Process Ctx Bad Sample" +{ + internal procedure IsProcessRunning(): Boolean + var + IsRunning: Boolean; + begin + OnCheckProcessRunning(IsRunning); + exit(IsRunning); + end; + + [InternalEvent(false)] + local procedure OnCheckProcessRunning(var IsRunning: Boolean) + begin + end; +} + +reportextension 50548 "Shared Report Ext Bad Sample" extends "Standard Sales - Invoice" +{ + trigger OnPreReport() + begin + // No callable query exists, so the extension infers the context from + // something it hopes only that process does - here, running without a + // UI. The guess is wrong for every other background run, and breaks + // silently the first time the owning app changes how it works. + if GuiAllowed() then + exit; + // ... behaviour that was meant to apply only inside that process ... + end; +} diff --git a/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.good.al b/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.good.al new file mode 100644 index 0000000..608a4e1 --- /dev/null +++ b/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.good.al @@ -0,0 +1,70 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. + +// The published context API - the entire public surface of the pattern. +// Any dependent extension may call IsProcessRunning; nothing else is exposed. +codeunit 50540 "Process Context Good Sample" +{ + procedure IsProcessRunning(): Boolean + var + IsRunning: Boolean; + begin + OnCheckProcessRunning(IsRunning); + exit(IsRunning); + end; + + // InternalEvent: only this app can subscribe, which is all the pattern + // needs. local: only this codeunit can raise it. + [InternalEvent(false)] + local procedure OnCheckProcessRunning(var IsRunning: Boolean) + begin + end; +} + +// The flag - implementation, not API, hence Access = Internal. It stores +// nothing between runs: being bound is the state. +codeunit 50541 "Process Flag Good Sample" +{ + Access = Internal; + EventSubscriberInstance = Manual; + + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Process Context Good Sample", 'OnCheckProcessRunning', '', false, false)] + local procedure SetProcessRunning(var IsRunning: Boolean) + begin + IsRunning := true; + end; +} + +// The app that drives the process claims the context for exactly its own run. +codeunit 50542 "Process Driver Good Sample" +{ + procedure Run(DocumentNo: Code[20]) + var + ProcessFlag: Codeunit "Process Flag Good Sample"; + begin + // A fresh instance, bound for exactly this call. If the shared code + // errors, the stack unwinds and takes the binding with it - nothing to reset. + BindSubscription(ProcessFlag); + RunSharedCode(DocumentNo); + end; + + local procedure RunSharedCode(DocumentNo: Code[20]) + begin + // A base application report, a posting routine, or any other object + // that extensions hook into - including a customer's own replacement. + end; +} + +// An extension hooked into that shared code can now ask the question directly +// instead of guessing which process is driving the run. The hook happens to be +// a report extension here; a subscriber on any other shared object is the same. +reportextension 50543 "Shared Report Ext Good Sample" extends "Standard Sales - Invoice" +{ + trigger OnPreReport() + var + ProcessContext: Codeunit "Process Context Good Sample"; + begin + if not ProcessContext.IsProcessRunning() then + exit; + // ... behaviour that applies only inside that process ... + end; +} diff --git a/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.md b/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.md new file mode 100644 index 0000000..a8e72be --- /dev/null +++ b/microsoft/knowledge/events/expose-process-context-via-manually-bound-flag.md @@ -0,0 +1,40 @@ +--- +bc-version: [all] +domain: events +keywords: [bindsubscription, manual-binding, eventsubscriberinstance, internalevent, singleinstance, process-context, running-flag, scoped-state, rollback] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Expose process context through a manually bound flag, not a single-instance boolean + +> Contributions welcome — open a PR to refine or extend this article. + +## Description + +When an extension drives a process over shared code — a base application report, a posting routine — other extensions hooked into that code cannot tell whether a run belongs to that process: AL keeps no ambient "current process", so the driving app has to publish the context itself. The reflex answer, a `SingleInstance` codeunit holding a boolean set at the start of the run and cleared at the end, is unsafe: single-instance variables are not part of the database transaction, so a failed run rolls back the writes but not the flag, which stays `true` until the company is closed and marks every later run in the session as part of the process. A manual event binding carries the same signal safely, because the platform ties its lifetime to a variable's scope instead of to cleanup code that has to run. + +## Best Practice + +Publish the context as a query and let the binding itself be the state. One procedure is public; everything behind it is internal: + +- A public context codeunit exposes `IsProcessRunning(): Boolean`, which raises an `[InternalEvent]` publisher taking a `var Boolean` and returns what comes back — the entire public surface. The publisher is internal because only the owning app subscribes, `local` because only this codeunit raises it. +- A second codeunit, `Access = Internal` with `EventSubscriberInstance = Manual`, subscribes to that event and sets the boolean to `true`. Internal keeps it out of the API and stops other apps binding it to forge the context; it stores nothing between runs — being bound *is* the state. +- The driving process calls `BindSubscription` on a variable whose scope is exactly the span it wants to claim: a local in the procedure that drives the run, or a global on an object that lives exactly as long as the run. While that variable is alive the query answers `true`; when it leaves scope — normally, or because an error unwound the call stack — the platform removes the binding and the query answers `false` again. + +Bind a fresh instance per run rather than reusing one: the platform refuses to bind the same instance twice but accepts several instances of the same codeunit, so nesting and re-entrancy need no counter. The binding is session-scoped, so work the process starts in another session — a background session, a page background task, a job queue entry — cannot see it; pass the context explicitly there. + +See sample: `expose-process-context-via-manually-bound-flag.good.al`. + +## Anti Pattern + +Two shapes. + +First, the single-instance boolean — the failure described above. Detection: a `SingleInstance = true` codeunit with a boolean set before a process and cleared after it, read by other code to decide whether that process is running. + +Second, the context kept private: the driving app arranges its own marker — typically a manually bound subscriber on an event added for its benefit alone — and offers no query, or only an `internal` one. Other extensions are left inferring the context from side effects, request-page values, or record state, which breaks silently the first time the process changes. Detection: a manual binding used purely as an internal run marker, with no public query procedure over it. + +The mirror-image anti-pattern belongs to the reviewer: flagging the `BindSubscription` here as a leaked binding because no `UnbindSubscription` follows it. Scope release is the mechanism, not an omission — see `microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md`, whose leak case is an instance parked on a `SingleInstance` global that never leaves scope. + +See sample: `expose-process-context-via-manually-bound-flag.bad.al`. diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al deleted file mode 100644 index 94b50f3..0000000 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al +++ /dev/null @@ -1,31 +0,0 @@ -// Demonstration-only AL. Not compiled by CI; illustrates the article. -codeunit 50241 "IsHandled Init Bad Sample" -{ - procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") - var - DiscountPct: Decimal; - IsHandled: Boolean; - begin - // IsHandled is never initialized before the first raise, so flow depends - // on the variable's default rather than an explicit, documented intent. - OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); - if not IsHandled then - DiscountPct := 5; - - // Bug: IsHandled is not reset. If the first subscriber set it true, the - // payment-discount default below is silently skipped too. - OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); - if not IsHandled then - DiscountPct += 2; - end; - - [IntegrationEvent(false, false)] - local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) - begin - end; - - [IntegrationEvent(false, false)] - local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) - begin - end; -} diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al deleted file mode 100644 index 190e321..0000000 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al +++ /dev/null @@ -1,31 +0,0 @@ -// Demonstration-only AL. Not compiled by CI; illustrates the article. -codeunit 50240 "IsHandled Init Good Sample" -{ - procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") - var - DiscountPct: Decimal; - IsHandled: Boolean; - begin - IsHandled := false; - OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); - if not IsHandled then - DiscountPct := 5; - - // Reset before reusing the same variable for the next event so a - // subscriber that handled the first raise can't suppress this one. - IsHandled := false; - OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); - if not IsHandled then - DiscountPct += 2; - end; - - [IntegrationEvent(false, false)] - local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) - begin - end; - - [IntegrationEvent(false, false)] - local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) - begin - end; -} diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md deleted file mode 100644 index acfb54a..0000000 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md +++ /dev/null @@ -1,26 +0,0 @@ ---- -bc-version: [all] -domain: events -keywords: [ishandled, initialization, deterministic, onbefore, reset, integration-event, control-flow] -technologies: [al] -countries: [w1] -application-area: [all] ---- - -# Initialize IsHandled to false before publishing - -## Description - -A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable in by reference, so its incoming value decides whether the default logic is skipped. A freshly declared Boolean starts as `false`, but the same variable is frequently reused to raise several events in one routine, and after the first raise it may already be `true`. Assigning `IsHandled := false;` on the line immediately before every raise makes the control flow deterministic and self-documenting, and prevents a stale `true` from silently suppressing logic the author never meant to make skippable. Generated code often reuses one `IsHandled` across several raises without resetting it. - -## Best Practice - -Set `IsHandled := false;` immediately before each `OnBeforeX(…, IsHandled)` raise, then guard the default logic with `if IsHandled then exit;` or `if not IsHandled then …`. Do this even when the variable was just declared: the explicit reset documents intent and stays correct if a second event raise is added to the routine later. This applies only to events that carry a `var IsHandled: Boolean`; an `OnBefore` event with no `IsHandled` parameter needs no reset. - -See sample: `initialize-ishandled-to-false-before-publishing.good.al`. - -## Anti Pattern - -Raising `OnBeforeX(…, IsHandled)` with a variable whose value carries over from an earlier raise, so a subscriber that handled the first event unintentionally suppresses the second routine's default logic. Detection: an `IsHandled` variable passed to more than one event in a routine without an intervening `IsHandled := false;`, or any `OnBefore…` raise that passes an `IsHandled` variable without an intervening `IsHandled := false;`. - -See sample: `initialize-ishandled-to-false-before-publishing.bad.al`. diff --git a/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.bad.al b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.bad.al new file mode 100644 index 0000000..a7d7709 --- /dev/null +++ b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.bad.al @@ -0,0 +1,48 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50241 "IsHandled Carry Over Bad Sample" +{ + procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") + var + DiscountPct: Decimal; + IsHandled: Boolean; + begin + OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); + if not IsHandled then + DiscountPct := 5; + + // Bug: execution continues when the first event set IsHandled to true, + // and that stale value is passed to a different publisher. + OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); + if not IsHandled then + DiscountPct += 2; + end; + + procedure ApplyLineDiscounts(var SalesLine: Record "Sales Line") + var + LineIsHandled: Boolean; + begin + if SalesLine.FindSet() then + repeat + // Bug: the local initializes only once. A subscriber that handles + // one line leaves true for every later iteration. + OnBeforeApplyLineDiscount(SalesLine, LineIsHandled); + if not LineIsHandled then + SalesLine.Validate("Line Discount %", 5); + until SalesLine.Next() = 0; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyLineDiscount(var SalesLine: Record "Sales Line"; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.good.al b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.good.al new file mode 100644 index 0000000..84d547e --- /dev/null +++ b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.good.al @@ -0,0 +1,50 @@ +// Demonstration-only AL. Not compiled by CI; illustrates the article. +codeunit 50240 "IsHandled Carry Over Good Sample" +{ + procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") + var + DiscountPct: Decimal; + HeaderIsHandled: Boolean; + PaymentIsHandled: Boolean; + begin + // Each fresh local is false and belongs to one non-looping raise. + OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, HeaderIsHandled); + if not HeaderIsHandled then + DiscountPct := 5; + + // Handling the header event does not suppress this independent seam. + OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, PaymentIsHandled); + if not PaymentIsHandled then + DiscountPct += 2; + end; + + procedure ApplyLineDiscounts(var SalesLine: Record "Sales Line") + var + LineIsHandled: Boolean; + begin + if SalesLine.FindSet() then + repeat + // The local initializes once, so reset it per iteration; a + // subscriber that handles one line must not skip the rest. + LineIsHandled := false; + OnBeforeApplyLineDiscount(SalesLine, LineIsHandled); + if not LineIsHandled then + SalesLine.Validate("Line Discount %", 5); + until SalesLine.Next() = 0; + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) + begin + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyLineDiscount(var SalesLine: Record "Sales Line"; var IsHandled: Boolean) + begin + end; +} diff --git a/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.md b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.md new file mode 100644 index 0000000..fba378b --- /dev/null +++ b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: events +keywords: [ishandled, carry-over, loop-iteration, onbefore, reset, integration-event, control-flow, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Reset IsHandled before publishing only when its value can carry over + +## Description + +A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event exactly once per procedure invocation is already deterministic. Initialization does not repeat for each loop iteration: a local declared outside a loop can carry `true` from one iteration to the next even when the source contains only one textual event raise. Outside a loop, reaching a later raise after `if IsHandled then exit;` also proves the value is `false`, provided that early exit is semantically correct and does not skip required downstream events. + +## Best Practice + +Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, a raise is re-entered by a loop, the value comes from an input parameter, field, or global, or earlier code seeds it. Prefer separate fresh locals when independent event seams need independent handled state. A reset on a guaranteed-false fresh local used by one non-looping raise, or before a later raise reached only after a semantically valid `if IsHandled then exit;`, can be retained for readability, but its absence is not a correctness finding. + +See sample: `reset-ishandled-only-when-the-value-can-carry-over.good.al`. + +## Anti Pattern + +Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise, an earlier loop iteration, or another source, so the publisher call starts with stale state. Do not match a single non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false. + +See sample: `reset-ishandled-only-when-the-value-can-carry-over.bad.al`. diff --git a/microsoft/knowledge/performance/avoid-cloning-records-before-modify-delete-in-loops.bad.al b/microsoft/knowledge/performance/avoid-cloning-records-before-modify-delete-in-loops.bad.al index 0a70e85..1faf1b7 100644 --- a/microsoft/knowledge/performance/avoid-cloning-records-before-modify-delete-in-loops.bad.al +++ b/microsoft/knowledge/performance/avoid-cloning-records-before-modify-delete-in-loops.bad.al @@ -5,8 +5,8 @@ codeunit 50493 "Perf Record Clone Bad" Customer: Record Customer; CustomerCopy: Record Customer; begin - Customer.SetLoadFields("Credit Limit (LCY)"); Customer.SetFilter("Credit Limit (LCY)", '>0'); + Customer.SetLoadFields("Credit Limit (LCY)"); if Customer.FindSet(true) then repeat CustomerCopy.Copy(Customer); diff --git a/microsoft/knowledge/performance/avoid-cloning-records-before-modify-delete-in-loops.good.al b/microsoft/knowledge/performance/avoid-cloning-records-before-modify-delete-in-loops.good.al index 84bfda4..16e1160 100644 --- a/microsoft/knowledge/performance/avoid-cloning-records-before-modify-delete-in-loops.good.al +++ b/microsoft/knowledge/performance/avoid-cloning-records-before-modify-delete-in-loops.good.al @@ -4,8 +4,8 @@ codeunit 50492 "Perf Record Clone Good" var Customer: Record Customer; begin - Customer.SetLoadFields("Credit Limit (LCY)"); Customer.SetFilter("Credit Limit (LCY)", '>0'); + Customer.SetLoadFields("Credit Limit (LCY)"); if Customer.FindSet(true) then repeat Customer.Validate( diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al b/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al index feacfcc..696b671 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al @@ -3,12 +3,24 @@ codeunit 50129 "Perf Sample CommitInLoop Bad" procedure NormalizeCustomerNames() var Customer: Record Customer; + LastCustomerNo: Code[20]; + ProcessedCount: Integer; begin + Customer.SetFilter("No.", '>%1', LastCustomerNo); if Customer.FindSet(true) then repeat Customer.Name := UpperCase(Customer.Name); Customer.Modify(); - Commit(); + + // LastCustomerNo exists only in memory, so a retry cannot exclude + // work that was already committed. + LastCustomerNo := Customer."No."; + ProcessedCount += 1; + + // This still opened a FindSet over the complete remaining tail; + // periodic commits do not turn retrieval into bounded TOP X. + if ProcessedCount mod 500 = 0 then + Commit(); until Customer.Next() = 0; end; } diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al index 2ffa386..2eb5bd0 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al @@ -16,11 +16,22 @@ codeunit 50128 "Perf Sample CommitInLoop Good" { procedure NormalizeCustomerNames() var + NormalizeState: Record "Perf Normalize State"; LastCustomerNo: Code[20]; begin - // The outer loop owns checkpoints; the per-row loop contains no Commit. - while NormalizeNextChunk(LastCustomerNo) do + if not NormalizeState.Get('CUSTOMER') then begin + NormalizeState.Init(); + NormalizeState.Code := 'CUSTOMER'; + NormalizeState.Insert(); + end; + LastCustomerNo := NormalizeState."Last Customer No."; + + while NormalizeNextChunk(LastCustomerNo) do begin + // Persist progress in the same transaction as the completed chunk. + NormalizeState."Last Customer No." := LastCustomerNo; + NormalizeState.Modify(); Commit(); + end; end; local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean @@ -58,3 +69,17 @@ codeunit 50128 "Perf Sample CommitInLoop Good" exit(true); end; } + +table 50128 "Perf Normalize State" +{ + fields + { + field(1; Code; Code[10]) { } + field(2; "Last Customer No."; Code[20]) { } + } + + keys + { + key(PK; Code) { Clustered = true; } + } +} diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.md b/microsoft/knowledge/performance/avoid-commit-inside-loops.md index 13f483a..25ad958 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.md +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.md @@ -13,16 +13,18 @@ application-area: [all] ## Description -Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that select an exact list of at most N keys and process only those rows. +Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). + +A durability checkpoint inside an outer batch loop can be valid only when the same transaction persists a progress marker or state that makes retries strictly exclude completed work, the checkpoint follows a complete business unit, and errors propagate instead of being swallowed. Restart safety and bounded retrieval are separate requirements: a persisted watermark can make retries safe, but an outer `FindSet` over the full remaining tail with periodic commits still retrieves the complete set because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next). ## Best Practice -If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. `FindSet` is optimized for reading the complete filtered set and isn't implemented as `TOP X`, so calling it over the remaining tail and breaking after N rows does not bound retrieval. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Commit after the bounded inner loop returns and persist its last selected key as the next watermark. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`. +If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Persist the last selected key in the same transaction as the completed chunk, then commit after the bounded helper returns. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. Let errors escape so failed work is not recorded as complete. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`. See sample: `avoid-commit-inside-loops.good.al`. ## Anti Pattern -Placing Commit inside `repeat ... until Next() = 0` is almost always a mistake: it is unusual for the correctness of the operation to depend on per-row commits, and the cost of starting a new transaction on every row dominates the work. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint. +Placing Commit inside `repeat ... until Next() = 0` without persisted progress is almost always a mistake: retries re-enter already committed work, while the cost of starting a transaction on every row dominates the operation. A progress variable held only in memory is not restart-safe. A full-tail `FindSet` with a commit every N rows is not bounded retrieval, even if a persisted watermark makes it restart-safe. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint. See sample: `avoid-commit-inside-loops.bad.al`. diff --git a/community/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.bad.al b/microsoft/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.bad.al similarity index 100% rename from community/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.bad.al rename to microsoft/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.bad.al diff --git a/community/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.good.al b/microsoft/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.good.al similarity index 100% rename from community/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.good.al rename to microsoft/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.good.al diff --git a/community/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.md b/microsoft/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.md similarity index 100% rename from community/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.md rename to microsoft/knowledge/performance/avoid-currpage-update-in-onaftergetrecord.md diff --git a/community/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.bad.al b/microsoft/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.bad.al similarity index 100% rename from community/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.bad.al rename to microsoft/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.bad.al diff --git a/community/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.good.al b/microsoft/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.good.al similarity index 100% rename from community/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.good.al rename to microsoft/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.good.al diff --git a/community/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.md b/microsoft/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.md similarity index 100% rename from community/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.md rename to microsoft/knowledge/performance/batch-number-series-instead-of-getnextno-per-row.md diff --git a/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.bad.al b/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.bad.al new file mode 100644 index 0000000..7fd2fc8 --- /dev/null +++ b/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.bad.al @@ -0,0 +1,31 @@ +codeunit 50541 "Perf Sample NoShortCircuit Bad" +{ + procedure ExceedsThreshold(var Thresholds: array[10] of Decimal; Index: Integer; Amount: Decimal): Boolean + begin + // Thresholds[Index] is evaluated even when Index is 0, so the leading range + // check does not prevent the subscript from being read out of range. + exit((Index >= 1) and (Index <= ArrayLen(Thresholds)) and (Amount > Thresholds[Index])); + end; + + procedure IsBlockedCustomer(CustomerNo: Code[20]): Boolean + var + Customer: Record Customer; + begin + // The Get runs even for an empty CustomerNo, and Blocked is read even when the + // Get failed, so the result is taken from a record that was never loaded. + exit((CustomerNo <> '') and Customer.Get(CustomerNo) and (Customer.Blocked <> Customer.Blocked::" ")); + end; + + procedure IsEligibleForFreeShipping(SalesHeader: Record "Sales Header"): Boolean + begin + // HasActiveLoyaltyBenefit runs even when the amount alone already qualifies, + // paying for the costly check on every evaluation instead of only the path + // where it can still change the outcome. + exit((SalesHeader."Amount Including VAT" >= 1000) or HasActiveLoyaltyBenefit(SalesHeader."Sell-to Customer No.")); + end; + + local procedure HasActiveLoyaltyBenefit(CustomerNo: Code[20]): Boolean + begin + exit(CustomerNo <> ''); + end; +} diff --git a/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.good.al b/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.good.al new file mode 100644 index 0000000..0bfda10 --- /dev/null +++ b/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.good.al @@ -0,0 +1,41 @@ +codeunit 50540 "Perf Sample NoShortCircuit Good" +{ + procedure ExceedsThreshold(var Thresholds: array[10] of Decimal; Index: Integer; Amount: Decimal): Boolean + begin + // 'and' is safe here: both operands are cheap and neither depends on the other. + if (Index >= 1) and (Index <= ArrayLen(Thresholds)) then + // The subscript lives in its own if, so it is never evaluated out of range. + if Amount > Thresholds[Index] then + exit(true); + exit(false); + end; + + procedure IsBlockedCustomer(CustomerNo: Code[20]): Boolean + var + Customer: Record Customer; + begin + // The cheap test runs first, and the field is read only after Get succeeded. + if CustomerNo = '' then + exit(false); + if not Customer.Get(CustomerNo) then + exit(false); + exit(Customer.Blocked <> Customer.Blocked::" "); + end; + + procedure IsEligibleForFreeShipping(SalesHeader: Record "Sales Header"): Boolean + begin + // 'or' is unsafe here: nesting would also be wrong, since it would drop the + // case where the amount alone already qualifies. Exit as soon as the cheap + // condition already decides the result; the costly lookup runs only on the + // path where it can still change the outcome. + if SalesHeader."Amount Including VAT" >= 1000 then + exit(true); + exit(HasActiveLoyaltyBenefit(SalesHeader."Sell-to Customer No.")); + end; + + local procedure HasActiveLoyaltyBenefit(CustomerNo: Code[20]): Boolean + begin + // Stands in for a costly check — a webservice call or a large table scan. + exit(CustomerNo <> ''); + end; +} diff --git a/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.md b/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.md new file mode 100644 index 0000000..7c4073b --- /dev/null +++ b/microsoft/knowledge/performance/boolean-operators-do-not-short-circuit.md @@ -0,0 +1,36 @@ +--- +bc-version: [all] +domain: performance +keywords: [short-circuit, lazy-evaluation, boolean-operators, nested-if, guard, and-operator, or-operator, xor-operator, early-exit] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL boolean operators do not short-circuit + +## Description + +AL gives no short-circuit (lazy) evaluation guarantee for `and`, `or`, and `xor`: every operand of a boolean expression is evaluated, even when the leftmost operand already determines the result. Neither the AL operators documentation nor the boolean operators documentation defines a lazy evaluation order, so code must not depend on one. Developers arriving from C#, JavaScript, or SQL routinely assume the left operand guards the right; in AL it does not. `xor` is not actually a short-circuit candidate in any language — its result depends on both operands regardless of their values, so there is nothing to skip — but AL still evaluates both operands unconditionally, so neither should carry a cost or a risk the developer assumed the other would guard against. For `and` and `or`, the right operand still runs even when the left already decides the result, so its cost is paid on every evaluation, and a check intended to protect an unsafe expression — an array subscript, a division, a field read that is only valid after a successful `Get` — does not protect it. + +## Best Practice + +For an `and`-shaped guard — a condition that must hold before the next operand is safe or worth evaluating — split into nested `if` statements: the guarding or cheapest condition in the outer `if`, the dependent or expensive one in the inner `if`. This preserves the result, since `if A then if B then Action` matches `if A and B then Action` exactly. Where there is no `else` branch, nesting is a pure win; where there is one, extract the conditions into a helper procedure that exits early instead. + +For an `or`-shaped condition, do not nest: nesting `if A then if B then Action` drops the case where `A` is true and `B` is false, silently changing the result of `A or B`. Exit as soon as the cheap or safe operand already decides the outcome, and reach the other operand only on the path where it can still change the result — `if A then exit(true); exit(B);` for a boolean return, or `if A then Action else if B then Action;` when both branches share one action. + +`xor` has no equivalent rewrite, because its result always depends on both operands; the only actionable guidance is to keep both operands of an `xor` cheap and free of side effects, since AL evaluates both unconditionally. + +Where a chain of `and`-guards runs past about three conditions, stop nesting and use a `case` statement instead — see `case-true-of-for-long-condition-chains.md`. Keep `and` and `or` for operands that are independently safe and cheap — in-memory field comparisons, enum tests, bound checks — where combining them reads better and costs nothing. + +See sample: `boolean-operators-do-not-short-circuit.good.al`. + +## Anti Pattern + +A single condition that joins a guard with an operand depending on that guard, or with an expensive operand, using `and` or `or`. The consequence is either wasted work on every evaluation — a database call or validation procedure invoked even when the outcome is already decided — or a runtime error or silently wrong result that the guard was written to prevent. Applying the `and` fix to an `or` condition is a distinct mistake: rewriting `A or B` as nested `if`s drops the `A`-true/`B`-false case instead of preserving it. Detection signals: an operand that indexes an array or list with a variable whose bounds are checked in a sibling operand; `Record.Get(...)` or a `Find`/`IsEmpty` call as one operand of `and` with a field read of the same record as another; an expensive or unsafe operand combined with `or` next to a condition that alone already makes the result true; a boolean-returning procedure call combined with a cheap field test. The pattern is common in code ported from a language that does short-circuit, and in conditions grown by appending a clause to an existing `if`. + +See sample: `boolean-operators-do-not-short-circuit.bad.al`. + +## See also + +`case-true-of-for-long-condition-chains.md` covers what to do when nesting an `and`-guard chain would go more than about three levels deep. `microsoft/knowledge/performance/apply-guards-before-get.md` covers the related ordering rule for statements rather than operands. diff --git a/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.bad.al b/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.bad.al new file mode 100644 index 0000000..da6200b --- /dev/null +++ b/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.bad.al @@ -0,0 +1,29 @@ +codeunit 50543 "Perf Sample CaseChain Bad" +{ + procedure IsShippableLine(SalesLine: Record "Sales Line"): Boolean + var + Item: Record Item; + begin + // Five levels of nesting to sequence five guards. The evaluation order is + // carried by indentation alone and the body drifts steadily right. + if SalesLine.Type = SalesLine.Type::Item then + if SalesLine."No." <> '' then + if SalesLine."Qty. to Ship" > 0 then + if Item.Get(SalesLine."No.") then + if not Item.Blocked then + exit(true); + exit(false); + end; + + procedure IsShippableLineCollapsed(SalesLine: Record "Sales Line"): Boolean + var + Item: Record Item; + begin + // The wrong escape from the ladder: flattening it into 'and' trades the + // nesting for a defect, because every operand is still evaluated. Item + // fields are read even when the Get failed. The parentheses are not + // optional either — 'and' binds tighter than '=' and '<>' in AL. + exit((SalesLine.Type = SalesLine.Type::Item) and (SalesLine."No." <> '') and + (SalesLine."Qty. to Ship" > 0) and Item.Get(SalesLine."No.") and not Item.Blocked); + end; +} diff --git a/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.good.al b/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.good.al new file mode 100644 index 0000000..03497e8 --- /dev/null +++ b/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.good.al @@ -0,0 +1,48 @@ +codeunit 50542 "Perf Sample CaseChain Good" +{ + procedure IsShippableLine(SalesLine: Record "Sales Line"): Boolean + var + Item: Record Item; + begin + // 'case false of' matches value sets in order and stops at the first match. + // The first three checks are pure and order-independent, so they share one + // value set. Get and Blocked are each their own value set, in order, because + // the ordering the documentation guarantees is across value sets, not within + // one — Item.Get must run, and succeed, before Blocked is read. + case false of + SalesLine.Type = SalesLine.Type::Item, + SalesLine."No." <> '', + SalesLine."Qty. to Ship" > 0: + exit(false); + Item.Get(SalesLine."No."): + exit(false); + not Item.Blocked: + exit(false); + end; + exit(true); + end; + + procedure FindOpenDocumentType(CustomerNo: Code[20]): Text + begin + // 'case true of' stops at the first condition that holds, so the later + // lookups never run once an earlier one matched. + case true of + HasOpenDocument(CustomerNo, "Sales Document Type"::Quote): + exit('Quote'); + HasOpenDocument(CustomerNo, "Sales Document Type"::Order): + exit('Order'); + HasOpenDocument(CustomerNo, "Sales Document Type"::Invoice): + exit('Invoice'); + end; + exit('None'); + end; + + local procedure HasOpenDocument(CustomerNo: Code[20]; DocumentType: Enum "Sales Document Type"): Boolean + var + SalesHeader: Record "Sales Header"; + begin + SalesHeader.SetRange("Document Type", DocumentType); + SalesHeader.SetRange("Sell-to Customer No.", CustomerNo); + exit(not SalesHeader.IsEmpty()); + end; +} diff --git a/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.md b/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.md new file mode 100644 index 0000000..1e0457e --- /dev/null +++ b/microsoft/knowledge/performance/case-true-of-for-long-condition-chains.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: performance +keywords: [case-statement, case-true-of, nested-if, condition-chain, guard, lazy-evaluation, nesting-depth] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Use case true of for long chains of dependent conditions + +## Description + +Because AL gives no short-circuit guarantee for `and` and `or`, a chain of conditions that must be evaluated in order has to be sequenced with nested `if` statements — and past three conditions the nesting itself becomes the problem: the body drifts right, the order of evaluation is carried by indentation alone, and any shared failure path is repeated at every level. AL's `case` statement is the flat alternative. Its value sets "must be an expression or a range", so `case true of` and `case false of` accept arbitrary boolean expressions, and the statement "is evaluated, and the first matching value set executes the associated statement" — evaluation stops at the first matching value set, which is exactly the laziness the boolean operators do not provide. That guarantee is stated for value sets, plural: it orders evaluation *across* separate value sets, and says nothing about the order of the individual expressions listed inside one comma-separated value set. + +## Best Practice + +Sequence two or three dependent conditions with nested `if`. Beyond that, switch to `case`: use `case false of` for a chain of guards where every condition must hold, letting control fall past `end` when all of them pass; use `case true of` for first-match dispatch, where each later probe runs only if the earlier ones did not match. Comma-separate conditions into one value set only when every one of them is a pure, order-independent test with no side effect — a field comparison, an enum check, a bound test — so it makes no difference whether AL evaluates all of them or stops early; grouping these costs nothing and removes the repeated action. A condition that guards another, or that carries a side effect or a cost of its own — a `Get`, a `Find`, a procedure call — keeps its own value set, placed immediately after the value set it depends on, so the code relies only on the ordering the documentation actually states. A value set needs no parentheses around a comparison, unlike an operand of `and` or `or`: the AL operator hierarchy places `and` and `or` above the comparison operators, so parentheses are mandatory there and the chain fills up with them. This keeps every condition at one indentation level, makes evaluation order explicit rather than implied by nesting, and preserves the stop-at-first-match behaviour it relies on. It also aligns with the AL programming convention that more than two alternatives belong in a `case` statement rather than an `if-then-else`. + +See sample: `case-true-of-for-long-condition-chains.good.al`. + +## Anti Pattern + +An `if` ladder four or more levels deep whose only purpose is sequencing guards. Detection: a chain of nested `if` statements with no `else`, each condition guarding the one below it, terminating in a single action or `exit`; or the same `exit`/`error` duplicated at every level of such a nested chain, purely to escape it. The second, worse form is collapsing that ladder into one `and` chain to escape the nesting — that trades indentation for a real defect, because the operands are still all evaluated. A third, subtler form is over-applying the comma-grouping itself: putting a guard and the condition it protects — for example `Item.Get(...)` and a read of a field on that same record — into one comma-separated value set. That relies on an evaluation order within a single value set that the documentation does not state; keep them in separate value sets instead. Reach for `case` over nested `if` or a collapsed `and` chain, and keep order-dependent conditions in their own value sets within it. + +See sample: `case-true-of-for-long-condition-chains.bad.al`. + +## See also + +`boolean-operators-do-not-short-circuit.md` covers the underlying evaluation rule that makes the sequencing necessary in the first place. diff --git a/community/knowledge/performance/changecompany-in-loop-drops-caches.bad.al b/microsoft/knowledge/performance/changecompany-in-loop-drops-caches.bad.al similarity index 100% rename from community/knowledge/performance/changecompany-in-loop-drops-caches.bad.al rename to microsoft/knowledge/performance/changecompany-in-loop-drops-caches.bad.al diff --git a/community/knowledge/performance/changecompany-in-loop-drops-caches.good.al b/microsoft/knowledge/performance/changecompany-in-loop-drops-caches.good.al similarity index 100% rename from community/knowledge/performance/changecompany-in-loop-drops-caches.good.al rename to microsoft/knowledge/performance/changecompany-in-loop-drops-caches.good.al diff --git a/community/knowledge/performance/changecompany-in-loop-drops-caches.md b/microsoft/knowledge/performance/changecompany-in-loop-drops-caches.md similarity index 100% rename from community/knowledge/performance/changecompany-in-loop-drops-caches.md rename to microsoft/knowledge/performance/changecompany-in-loop-drops-caches.md diff --git a/community/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.bad.al b/microsoft/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.bad.al similarity index 100% rename from community/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.bad.al rename to microsoft/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.bad.al diff --git a/community/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.good.al b/microsoft/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.good.al similarity index 100% rename from community/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.good.al rename to microsoft/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.good.al diff --git a/community/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.md b/microsoft/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.md similarity index 100% rename from community/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.md rename to microsoft/knowledge/performance/dataaccessintent-readonly-on-analytical-objects.md diff --git a/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.bad.al b/microsoft/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.bad.al similarity index 100% rename from community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.bad.al rename to microsoft/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.bad.al diff --git a/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.good.al b/microsoft/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.good.al similarity index 100% rename from community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.good.al rename to microsoft/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.good.al diff --git a/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.md b/microsoft/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.md similarity index 100% rename from community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.md rename to microsoft/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.md diff --git a/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.bad.al b/microsoft/knowledge/performance/httpclient-inside-write-transaction-holds-locks.bad.al similarity index 100% rename from community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.bad.al rename to microsoft/knowledge/performance/httpclient-inside-write-transaction-holds-locks.bad.al diff --git a/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al b/microsoft/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al similarity index 100% rename from community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al rename to microsoft/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al diff --git a/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md b/microsoft/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md similarity index 100% rename from community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md rename to microsoft/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md diff --git a/community/knowledge/performance/isempty-before-findset-is-extra-round-trip.bad.al b/microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.bad.al similarity index 100% rename from community/knowledge/performance/isempty-before-findset-is-extra-round-trip.bad.al rename to microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.bad.al diff --git a/community/knowledge/performance/isempty-before-findset-is-extra-round-trip.good.al b/microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.good.al similarity index 100% rename from community/knowledge/performance/isempty-before-findset-is-extra-round-trip.good.al rename to microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.good.al diff --git a/community/knowledge/performance/isempty-before-findset-is-extra-round-trip.md b/microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.md similarity index 100% rename from community/knowledge/performance/isempty-before-findset-is-extra-round-trip.md rename to microsoft/knowledge/performance/isempty-before-findset-is-extra-round-trip.md diff --git a/community/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.bad.al b/microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.bad.al similarity index 100% rename from community/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.bad.al rename to microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.bad.al diff --git a/community/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.good.al b/microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.good.al similarity index 100% rename from community/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.good.al rename to microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.good.al diff --git a/community/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.md b/microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.md similarity index 100% rename from community/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.md rename to microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.md diff --git a/community/knowledge/performance/job-queue-external-effects-must-be-idempotent.bad.al b/microsoft/knowledge/performance/job-queue-external-effects-must-be-idempotent.bad.al similarity index 100% rename from community/knowledge/performance/job-queue-external-effects-must-be-idempotent.bad.al rename to microsoft/knowledge/performance/job-queue-external-effects-must-be-idempotent.bad.al diff --git a/community/knowledge/performance/job-queue-external-effects-must-be-idempotent.good.al b/microsoft/knowledge/performance/job-queue-external-effects-must-be-idempotent.good.al similarity index 100% rename from community/knowledge/performance/job-queue-external-effects-must-be-idempotent.good.al rename to microsoft/knowledge/performance/job-queue-external-effects-must-be-idempotent.good.al diff --git a/community/knowledge/performance/job-queue-external-effects-must-be-idempotent.md b/microsoft/knowledge/performance/job-queue-external-effects-must-be-idempotent.md similarity index 100% rename from community/knowledge/performance/job-queue-external-effects-must-be-idempotent.md rename to microsoft/knowledge/performance/job-queue-external-effects-must-be-idempotent.md diff --git a/community/knowledge/performance/job-queue-handlers-must-not-require-ui.bad.al b/microsoft/knowledge/performance/job-queue-handlers-must-not-require-ui.bad.al similarity index 100% rename from community/knowledge/performance/job-queue-handlers-must-not-require-ui.bad.al rename to microsoft/knowledge/performance/job-queue-handlers-must-not-require-ui.bad.al diff --git a/community/knowledge/performance/job-queue-handlers-must-not-require-ui.good.al b/microsoft/knowledge/performance/job-queue-handlers-must-not-require-ui.good.al similarity index 100% rename from community/knowledge/performance/job-queue-handlers-must-not-require-ui.good.al rename to microsoft/knowledge/performance/job-queue-handlers-must-not-require-ui.good.al diff --git a/community/knowledge/performance/job-queue-handlers-must-not-require-ui.md b/microsoft/knowledge/performance/job-queue-handlers-must-not-require-ui.md similarity index 100% rename from community/knowledge/performance/job-queue-handlers-must-not-require-ui.md rename to microsoft/knowledge/performance/job-queue-handlers-must-not-require-ui.md diff --git a/community/knowledge/performance/job-queue-handlers-must-propagate-failures.bad.al b/microsoft/knowledge/performance/job-queue-handlers-must-propagate-failures.bad.al similarity index 100% rename from community/knowledge/performance/job-queue-handlers-must-propagate-failures.bad.al rename to microsoft/knowledge/performance/job-queue-handlers-must-propagate-failures.bad.al diff --git a/community/knowledge/performance/job-queue-handlers-must-propagate-failures.good.al b/microsoft/knowledge/performance/job-queue-handlers-must-propagate-failures.good.al similarity index 100% rename from community/knowledge/performance/job-queue-handlers-must-propagate-failures.good.al rename to microsoft/knowledge/performance/job-queue-handlers-must-propagate-failures.good.al diff --git a/community/knowledge/performance/job-queue-handlers-must-propagate-failures.md b/microsoft/knowledge/performance/job-queue-handlers-must-propagate-failures.md similarity index 100% rename from community/knowledge/performance/job-queue-handlers-must-propagate-failures.md rename to microsoft/knowledge/performance/job-queue-handlers-must-propagate-failures.md diff --git a/community/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.bad.al b/microsoft/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.bad.al similarity index 100% rename from community/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.bad.al rename to microsoft/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.bad.al diff --git a/community/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.good.al b/microsoft/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.good.al similarity index 100% rename from community/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.good.al rename to microsoft/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.good.al diff --git a/community/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.md b/microsoft/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.md similarity index 100% rename from community/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.md rename to microsoft/knowledge/performance/job-queue-on-hold-does-not-stop-running-work.md diff --git a/microsoft/knowledge/performance/load-only-primary-key-fields-for-reference-work.good.al b/microsoft/knowledge/performance/load-only-primary-key-fields-for-reference-work.good.al index 5dcc499..7437a4b 100644 --- a/microsoft/knowledge/performance/load-only-primary-key-fields-for-reference-work.good.al +++ b/microsoft/knowledge/performance/load-only-primary-key-fields-for-reference-work.good.al @@ -6,8 +6,8 @@ codeunit 50100 "Item Reindex Queue" ReindexQueue: Codeunit "Reindex Queue"; begin // Only the primary key is used in the loop body; load nothing else. - Item.SetLoadFields("No."); Item.SetRange("Item Category Code", CategoryCode); + Item.SetLoadFields("No."); if Item.FindSet() then repeat diff --git a/community/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.bad.al b/microsoft/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.bad.al similarity index 100% rename from community/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.bad.al rename to microsoft/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.bad.al diff --git a/community/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.good.al b/microsoft/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.good.al similarity index 100% rename from community/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.good.al rename to microsoft/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.good.al diff --git a/community/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.md b/microsoft/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.md similarity index 100% rename from community/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.md rename to microsoft/knowledge/performance/oncompanyopen-subscribers-must-not-do-io.md diff --git a/community/knowledge/performance/page-background-tasks-for-expensive-cues.bad.al b/microsoft/knowledge/performance/page-background-tasks-for-expensive-cues.bad.al similarity index 100% rename from community/knowledge/performance/page-background-tasks-for-expensive-cues.bad.al rename to microsoft/knowledge/performance/page-background-tasks-for-expensive-cues.bad.al diff --git a/community/knowledge/performance/page-background-tasks-for-expensive-cues.good.al b/microsoft/knowledge/performance/page-background-tasks-for-expensive-cues.good.al similarity index 100% rename from community/knowledge/performance/page-background-tasks-for-expensive-cues.good.al rename to microsoft/knowledge/performance/page-background-tasks-for-expensive-cues.good.al diff --git a/community/knowledge/performance/page-background-tasks-for-expensive-cues.md b/microsoft/knowledge/performance/page-background-tasks-for-expensive-cues.md similarity index 100% rename from community/knowledge/performance/page-background-tasks-for-expensive-cues.md rename to microsoft/knowledge/performance/page-background-tasks-for-expensive-cues.md diff --git a/community/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.bad.al b/microsoft/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.bad.al similarity index 100% rename from community/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.bad.al rename to microsoft/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.bad.al diff --git a/community/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.good.al b/microsoft/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.good.al similarity index 100% rename from community/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.good.al rename to microsoft/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.good.al diff --git a/community/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.md b/microsoft/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.md similarity index 100% rename from community/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.md rename to microsoft/knowledge/performance/pass-var-record-to-preserve-partial-load-enumerator.md diff --git a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md index ae83968..024aae1 100644 --- a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md +++ b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md @@ -15,12 +15,12 @@ application-area: [all] ## Best Practice -Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table-extension triggers, event subscribers, global triggers, or media fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). +Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). A visible loop for progress UX is acceptable only when evidence shows the equivalent bulk call already executes as individual operations and the loop preserves trigger and business semantics. See sample: `prefer-modifyall-over-per-row-modify.good.al`. ## Anti Pattern -A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior. +A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects or bulk fallback condition. A progress dialog alone does not exempt this loop. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior. See sample: `prefer-modifyall-over-per-row-modify.bad.al`. diff --git a/community/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.bad.al b/microsoft/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.bad.al similarity index 100% rename from community/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.bad.al rename to microsoft/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.bad.al diff --git a/community/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.good.al b/microsoft/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.good.al similarity index 100% rename from community/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.good.al rename to microsoft/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.good.al diff --git a/community/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.md b/microsoft/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.md similarity index 100% rename from community/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.md rename to microsoft/knowledge/performance/prefer-related-table-over-extension-on-hot-ledgers.md diff --git a/community/knowledge/performance/query-results-bypass-primary-key-cache.bad.al b/microsoft/knowledge/performance/query-results-bypass-primary-key-cache.bad.al similarity index 100% rename from community/knowledge/performance/query-results-bypass-primary-key-cache.bad.al rename to microsoft/knowledge/performance/query-results-bypass-primary-key-cache.bad.al diff --git a/community/knowledge/performance/query-results-bypass-primary-key-cache.good.al b/microsoft/knowledge/performance/query-results-bypass-primary-key-cache.good.al similarity index 100% rename from community/knowledge/performance/query-results-bypass-primary-key-cache.good.al rename to microsoft/knowledge/performance/query-results-bypass-primary-key-cache.good.al diff --git a/community/knowledge/performance/query-results-bypass-primary-key-cache.md b/microsoft/knowledge/performance/query-results-bypass-primary-key-cache.md similarity index 100% rename from community/knowledge/performance/query-results-bypass-primary-key-cache.md rename to microsoft/knowledge/performance/query-results-bypass-primary-key-cache.md diff --git a/community/knowledge/performance/reset-clears-partial-record-selection.bad.al b/microsoft/knowledge/performance/reset-clears-partial-record-selection.bad.al similarity index 100% rename from community/knowledge/performance/reset-clears-partial-record-selection.bad.al rename to microsoft/knowledge/performance/reset-clears-partial-record-selection.bad.al diff --git a/community/knowledge/performance/reset-clears-partial-record-selection.good.al b/microsoft/knowledge/performance/reset-clears-partial-record-selection.good.al similarity index 100% rename from community/knowledge/performance/reset-clears-partial-record-selection.good.al rename to microsoft/knowledge/performance/reset-clears-partial-record-selection.good.al diff --git a/community/knowledge/performance/reset-clears-partial-record-selection.md b/microsoft/knowledge/performance/reset-clears-partial-record-selection.md similarity index 100% rename from community/knowledge/performance/reset-clears-partial-record-selection.md rename to microsoft/knowledge/performance/reset-clears-partial-record-selection.md diff --git a/community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.bad.al b/microsoft/knowledge/performance/skip-setloadfields-on-write-and-transferfields.bad.al similarity index 100% rename from community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.bad.al rename to microsoft/knowledge/performance/skip-setloadfields-on-write-and-transferfields.bad.al diff --git a/community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.good.al b/microsoft/knowledge/performance/skip-setloadfields-on-write-and-transferfields.good.al similarity index 100% rename from community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.good.al rename to microsoft/knowledge/performance/skip-setloadfields-on-write-and-transferfields.good.al diff --git a/community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.md b/microsoft/knowledge/performance/skip-setloadfields-on-write-and-transferfields.md similarity index 100% rename from community/knowledge/performance/skip-setloadfields-on-write-and-transferfields.md rename to microsoft/knowledge/performance/skip-setloadfields-on-write-and-transferfields.md diff --git a/community/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.bad.al b/microsoft/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.bad.al similarity index 100% rename from community/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.bad.al rename to microsoft/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.bad.al diff --git a/community/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.good.al b/microsoft/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.good.al similarity index 100% rename from community/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.good.al rename to microsoft/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.good.al diff --git a/community/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.md b/microsoft/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.md similarity index 100% rename from community/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.md rename to microsoft/knowledge/performance/store-scheduled-task-id-to-avoid-duplicate-tasks.md diff --git a/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md b/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md index c4890a0..dabeb31 100644 --- a/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md +++ b/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: performance -keywords: [modifyall, deleteall, regression, triggers, media, getglobaltabletriggermask, subscriber] +keywords: [modifyall, deleteall, regression, triggers, media, security-filtering, companion-fields, subscriber, progress] technologies: [al] countries: [w1] application-area: [all] @@ -11,12 +11,12 @@ application-area: [all] ## Description -`ModifyAll` and `DeleteAll` usually execute as single SQL statements, but the platform falls back to a fetch-then-row-by-row loop under specific conditions. Per the upstream guidance, the regression is triggered by any of: global database triggers defined via `GetGlobalTableTriggerMask` or `GetDatabaseTableTriggerSetup` (so that `OnDatabaseDelete`/`OnGlobalDelete` must run); event subscribers on the table's `OnBeforeDelete`/`OnAfterDelete` (for `DeleteAll`) or `OnBeforeModify`/`OnAfterModify` (for `ModifyAll`); or "adding a Media or MediaSet table field to either the table or table extension." Each of these forces the platform to materialize each affected row in AL. +`ModifyAll` and `DeleteAll` can limit SQL calls, but Microsoft documents that they [revert to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related modify/delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. These conditions must be assessed from the target table and runtime context, not only from the visible bulk call. ## Best Practice -Before introducing any of the above on a table — a global trigger registration, a `Modify`/`Delete` subscriber, a media or media-set field — note every `ModifyAll`/`DeleteAll` that targets the table and assess whether the regression cost is acceptable. The upstream guidance is explicit: "There should be a very good reason for doing any of the above since they will significantly regress performance of `ModifyAll` and/or `DeleteAll`." Once a table has regressed, multiple `ModifyAll` calls each iterate the rows themselves, so consolidating to one explicit `FindSet`+`Modify` loop becomes faster than chaining several `ModifyAll` calls. +Before introducing a fallback condition, audit the `ModifyAll`/`DeleteAll` call sites that target the table and assess the regression cost. Once a bulk path already executes row by row, one explicit loop can be reasonable when it preserves the same trigger semantics and adds required per-row progress UX; consolidating several regressed bulk calls into one pass can also avoid repeated iteration. This is a narrow equivalence check, not a generic progress-dialog exemption: when no fallback condition applies, retain the bulk API. ## Anti Pattern -Adding a media field to a hot table — or subscribing to its modify/delete events from a generic logging codeunit — without auditing the bulk-write call sites. The schema change is mechanical; the performance change is invisible at the call site and only surfaces when a previously fast `ModifyAll` starts paying the per-row trigger cost in production. The mirror anti-pattern is chaining several `ModifyAll` calls on a table that has already regressed; each one re-iterates the same rows. +Adding a fallback condition to a hot table without auditing bulk-write call sites, or replacing a working bulk API with a per-row loop solely to show progress. The mirror anti-pattern is chaining several bulk calls on a table that already falls back, causing repeated row-by-row passes. diff --git a/community/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.bad.al b/microsoft/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.bad.al similarity index 100% rename from community/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.bad.al rename to microsoft/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.bad.al diff --git a/community/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.good.al b/microsoft/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.good.al similarity index 100% rename from community/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.good.al rename to microsoft/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.good.al diff --git a/community/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.md b/microsoft/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.md similarity index 100% rename from community/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.md rename to microsoft/knowledge/performance/use-dedicated-lookup-pages-not-full-lists.md diff --git a/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md b/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md index 1c80835..41ad41f 100644 --- a/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md +++ b/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: performance -keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass] +keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass, security-filtering, media, companion-fields] technologies: [al] countries: [w1] application-area: [all] @@ -13,16 +13,16 @@ application-area: [all] ## Description -`DeleteAll(false)` is eligible for a set-based SQL delete with the record variable's filters applied. It is not guaranteed to stay one statement. The base table `OnDelete` trigger is skipped, but table-extension `OnBeforeDelete` and `OnAfterDelete` triggers still run. Extension event subscribers, global delete triggers, and media fields can also require row processing. `DeleteAll(true)` runs the base table `OnDelete` trigger as well and has no performance advantage over `Delete(true)` in a loop. +`DeleteAll(false)` is eligible for a set-based SQL delete with the record variable's filters applied, but it is not guaranteed to stay one statement. Microsoft documents that `DeleteAll` [reverts to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. Setting `RunTrigger` to false skips the base table `OnDelete` trigger, but [table-extension `OnBeforeDelete` and `OnAfterDelete` triggers still run](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-deleteall-method#remarks). ## Best Practice -Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and installed extensions, subscribers, global triggers, and media fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately. +Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and that trigger code, related subscribers, security filtering, media fields, and companion fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately. See sample: `use-deleteall-for-filtered-bulk-deletion.good.al`. ## Anti Pattern -Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking table extensions and subscribers. +Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic or fallback condition. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking the documented fallback conditions. See sample: `use-deleteall-for-filtered-bulk-deletion.bad.al`. diff --git a/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.bad.al b/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.bad.al index 86b1842..42779b4 100644 --- a/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.bad.al +++ b/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.bad.al @@ -4,8 +4,8 @@ codeunit 50491 "Perf AutoCalcFields Bad" var Customer: Record Customer; begin - Customer.SetLoadFields("Credit Limit (LCY)"); Customer.SetFilter("Credit Limit (LCY)", '>0'); + Customer.SetLoadFields("Credit Limit (LCY)"); if Customer.FindSet() then repeat Customer.CalcFields("Balance (LCY)"); diff --git a/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.good.al b/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.good.al index 072321c..3c1208f 100644 --- a/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.good.al +++ b/microsoft/knowledge/performance/use-setautocalcfields-for-per-row-flowfields.good.al @@ -4,8 +4,8 @@ codeunit 50490 "Perf AutoCalcFields Good" var Customer: Record Customer; begin - Customer.SetLoadFields("Credit Limit (LCY)"); Customer.SetFilter("Credit Limit (LCY)", '>0'); + Customer.SetLoadFields("Credit Limit (LCY)"); Customer.SetAutoCalcFields("Balance (LCY)"); if Customer.FindSet() then repeat diff --git a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.good.al b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.good.al index 772023c..a5bee5f 100644 --- a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.good.al +++ b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.good.al @@ -4,8 +4,8 @@ codeunit 50218 "Perf Sample LoadFields Good" var Customer: Record Customer; begin - Customer.SetLoadFields(Name); Customer.SetRange("Country/Region Code", 'US'); + Customer.SetLoadFields(Name); if Customer.FindSet() then repeat Message(Customer.Name); diff --git a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.md b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.md index 85d20b3..a8d134f 100644 --- a/microsoft/knowledge/performance/use-setloadfields-for-partial-records.md +++ b/microsoft/knowledge/performance/use-setloadfields-for-partial-records.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: performance -keywords: [setloadfields, partial-record, normal-field, flowfield, get, findset] +keywords: [setloadfields, partial-record, normal-field, flowfield, get, findset, statement-order] technologies: [al] countries: [w1] application-area: [all] @@ -11,11 +11,11 @@ application-area: [all] ## Description -`SetLoadFields(...)` declares the subset of normal fields the next read should materialize, "reducing data read and transfer thereby improving performance significantly." Per the upstream guidance, "the gains scale with the amount of rows read, so for loops that read many rows `SetLoadFields` is even more important." Primary-key fields, `SystemId`, and system audit fields are loaded automatically, "and fields that are filtered on are also automatically included" — those do not need to appear in the list. `SetLoadFields` only affects `FieldClass = Normal`; it does not narrow FlowFields or FlowFilters. +`SetLoadFields(...)` declares the subset of normal fields the next read should materialize, "reducing data read and transfer thereby improving performance significantly." Per the upstream guidance, "the gains scale with the amount of rows read, so for loops that read many rows `SetLoadFields` is even more important." Primary-key fields, `SystemId`, and system audit fields are loaded automatically, "and fields that are filtered on are also automatically included" — those do not need to appear in the list. `SetLoadFields` only affects `FieldClass = Normal`; it does not narrow FlowFields or FlowFilters. Its position relative to `SetRange`/`SetFilter` does not change the projection: filtered fields are added to the load set at read time either way. Projection-changing operations are separate: `AddLoadFields(...)` expands the selection, a later `SetLoadFields(...)` or `SetBaseLoadFields()` overwrites it, and `Reset()` or a fieldless `SetLoadFields()` restores all readable normal fields. ## Best Practice -Before a `Get`, `FindSet`, or `FindFirst` that the procedure follows by reading only a handful of the table's fields, call `SetLoadFields` listing exactly those fields. The pattern `SetLoadFields(...); if Record.Get(...) then ...` is the upstream-endorsed shape. Skip `SetLoadFields` when the table has few fields (under ten), when the code reads most of them (above 60 %), when the loop runs ten or fewer iterations, or when the table is exempt for other reasons (`singleton-setup-tables-need-no-access-optimization.md`, `temporary-tables-have-no-database-cost.md`). For report dataitems, use `AddLoadFields` in `OnPreDataItem` instead (see `addloadfields-in-report-onpredataitem.md`). +Before a `Get`, `FindSet`, or `FindFirst` that the procedure follows by reading only a handful of the table's fields, call `SetLoadFields` listing exactly those fields. The pattern `SetLoadFields(...); if Record.Get(...) then ...` is the upstream-endorsed shape. Place the call immediately before the read, after any `SetRange`/`SetFilter`, so a reader can see at a glance which read the selection governs and any projection-changing operation is easy to spot. Skip `SetLoadFields` when the table has few fields (under ten), when the code reads most of them (above 60 %), when the loop runs ten or fewer iterations, or when the table is exempt for other reasons (`singleton-setup-tables-need-no-access-optimization.md`, `temporary-tables-have-no-database-cost.md`). For report dataitems, use `AddLoadFields` in `OnPreDataItem` instead (see `addloadfields-in-report-onpredataitem.md`). See sample: `use-setloadfields-for-partial-records.good.al`. @@ -23,4 +23,6 @@ See sample: `use-setloadfields-for-partial-records.good.al`. Loading a wide table and reading one field per row in a loop. The bytes transferred per row are dominated by the columns the procedure does not touch; the SQL query selects them anyway. The same applies to a single `Get` on a wide table — the platform reads the whole row when a single field would have sufficed. +Statement order is not part of this anti pattern. `SetLoadFields` placed ahead of `SetRange`/`SetFilter` materializes exactly the same columns as the reverse order, so a reviewer reports it as a readability observation at most — never as a performance defect. + See sample: `use-setloadfields-for-partial-records.bad.al`. diff --git a/community/knowledge/performance/validate-on-partial-record-forces-jit.bad.al b/microsoft/knowledge/performance/validate-on-partial-record-forces-jit.bad.al similarity index 100% rename from community/knowledge/performance/validate-on-partial-record-forces-jit.bad.al rename to microsoft/knowledge/performance/validate-on-partial-record-forces-jit.bad.al diff --git a/community/knowledge/performance/validate-on-partial-record-forces-jit.good.al b/microsoft/knowledge/performance/validate-on-partial-record-forces-jit.good.al similarity index 100% rename from community/knowledge/performance/validate-on-partial-record-forces-jit.good.al rename to microsoft/knowledge/performance/validate-on-partial-record-forces-jit.good.al diff --git a/community/knowledge/performance/validate-on-partial-record-forces-jit.md b/microsoft/knowledge/performance/validate-on-partial-record-forces-jit.md similarity index 100% rename from community/knowledge/performance/validate-on-partial-record-forces-jit.md rename to microsoft/knowledge/performance/validate-on-partial-record-forces-jit.md diff --git a/microsoft/knowledge/security/commitbehavior-attribute-scopes-explicit-commits.bad.al b/microsoft/knowledge/security/commitbehavior-attribute-scopes-explicit-commits.bad.al index c3b796b..4633488 100644 --- a/microsoft/knowledge/security/commitbehavior-attribute-scopes-explicit-commits.bad.al +++ b/microsoft/knowledge/security/commitbehavior-attribute-scopes-explicit-commits.bad.al @@ -1,7 +1,7 @@ codeunit 50151 "Sec Sample CommitBeh Bad" { [IntegrationEvent(true, false)] - procedure OnBeforeApplyingDiscount(var Customer: Record Customer) + local procedure OnBeforeApplyingDiscount(var Customer: Record Customer) begin end; diff --git a/microsoft/knowledge/security/commitbehavior-attribute-scopes-explicit-commits.good.al b/microsoft/knowledge/security/commitbehavior-attribute-scopes-explicit-commits.good.al index 0204d12..f0f3233 100644 --- a/microsoft/knowledge/security/commitbehavior-attribute-scopes-explicit-commits.good.al +++ b/microsoft/knowledge/security/commitbehavior-attribute-scopes-explicit-commits.good.al @@ -2,7 +2,7 @@ codeunit 50149 "Sec Sample CommitBeh Good" { [CommitBehavior(CommitBehavior::Ignore)] [IntegrationEvent(true, false)] - procedure OnBeforeApplyingDiscount(var Customer: Record Customer) + local procedure OnBeforeApplyingDiscount(var Customer: Record Customer) begin end; diff --git a/community/knowledge/security/guard-bulk-operations-with-istemporary.bad.al b/microsoft/knowledge/security/guard-bulk-operations-with-istemporary.bad.al similarity index 100% rename from community/knowledge/security/guard-bulk-operations-with-istemporary.bad.al rename to microsoft/knowledge/security/guard-bulk-operations-with-istemporary.bad.al diff --git a/community/knowledge/security/guard-bulk-operations-with-istemporary.good.al b/microsoft/knowledge/security/guard-bulk-operations-with-istemporary.good.al similarity index 100% rename from community/knowledge/security/guard-bulk-operations-with-istemporary.good.al rename to microsoft/knowledge/security/guard-bulk-operations-with-istemporary.good.al diff --git a/community/knowledge/security/guard-bulk-operations-with-istemporary.md b/microsoft/knowledge/security/guard-bulk-operations-with-istemporary.md similarity index 100% rename from community/knowledge/security/guard-bulk-operations-with-istemporary.md rename to microsoft/knowledge/security/guard-bulk-operations-with-istemporary.md 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 fd458a4..21dccb3 100644 --- a/microsoft/knowledge/style/caption-required-on-page-fields.bad.al +++ b/microsoft/knowledge/style/caption-required-on-page-fields.bad.al @@ -9,16 +9,22 @@ page 50253 "Sample Caption Bad" { group(General) { - field("Customer No."; Rec."No.") + Caption = 'General'; + field(CustomerNoValue; CustomerNoValue) { ApplicationArea = All; + ToolTip = 'Specifies the customer number to look up.'; } field("Customer Name"; Rec.Name) { ApplicationArea = All; Caption = ''; + ToolTip = 'Specifies the customer name shown on sales documents.'; } } } } + + var + CustomerNoValue: Code[20]; } 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 0493b6e..f91ed73 100644 --- a/microsoft/knowledge/style/caption-required-on-page-fields.good.al +++ b/microsoft/knowledge/style/caption-required-on-page-fields.good.al @@ -1,7 +1,36 @@ +// BC24 / runtime 13.0 or later for table-field tooltips. +table 50252 "Sample Caption Source" +{ + Caption = 'Caption Source'; + DataClassification = CustomerContent; + + fields + { + field(1; "No."; Code[20]) + { + Caption = 'No.'; + ToolTip = 'Specifies the unique number used to distinguish this customer record from other records.'; + } + field(2; Name; Text[100]) + { + Caption = 'Name'; + ToolTip = 'Specifies the name used to identify the customer alongside the unique customer number.'; + } + } + + keys + { + key(PK; "No.") + { + Clustered = true; + } + } +} + page 50252 "Sample Caption Good" { PageType = Card; - SourceTable = Customer; + SourceTable = "Sample Caption Source"; layout { @@ -10,19 +39,25 @@ page 50252 "Sample Caption Good" group(General) { Caption = 'General'; - field("Customer No."; Rec."No.") + field("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.'; + } + field(DisplayValue; DisplayValue) + { + ApplicationArea = All; + Caption = 'Display Value'; + ToolTip = 'Specifies temporary text for this page; the text is not saved in the customer record.'; } } } } + + var + DisplayValue: Text[100]; } diff --git a/microsoft/knowledge/style/caption-required-on-page-fields.md b/microsoft/knowledge/style/caption-required-on-page-fields.md index e3a69c3..b5d2e03 100644 --- a/microsoft/knowledge/style/caption-required-on-page-fields.md +++ b/microsoft/knowledge/style/caption-required-on-page-fields.md @@ -1,28 +1,38 @@ --- bc-version: [all] domain: style -keywords: [caption, page-field, aa0225, aa0226, codecop, captionclass] +keywords: [caption, page-field, source-field, inheritance, aa0225, aa0226, codecop, captionclass, false-positive] technologies: [al] countries: [w1] application-area: [all] --- -# Every page field needs a `Caption` (CodeCop AA0225/AA0226) +# Page fields can inherit their source table field's `Caption` ## Description -CodeCop AA0225 and AA0226 require every field control to expose a `Caption` property, separately from the field's source name. The caption is what the user sees as the column header or label; the source name is what the code uses to reference the field. Without an explicit `Caption`, AL falls back to the source field's caption — which may be wrong for the page's context — or to the field name itself in code casing, which surfaces internal naming to users and to translators. +A page field bound to a table field inherits the source field's `Caption` unless the page overrides it. An inherited caption is valid, user-facing, and translatable; omitting a page-level `Caption` does not mean the control displays an internal identifier or loses translations. CodeCop AA0225/AA0226 concern missing or empty captions, not a requirement to duplicate a caption already supplied by the source table field. -Acceptable exceptions: a field whose caption is inherited via `CaptionClass = '3,5,' + CurrencyCode` (or another CaptionClass formula) does not need a literal `Caption`; the formula provides it. API pages and test pages may omit captions because their consumers are not human users. Boolean fields whose name already reads as a sentence — `Enabled`, `Posted`, `Released` — do not need a redundant Caption that repeats the name. +Redundant page-level captions compile successfully, so compiler-error recovery does not prevent an agent from adding them. This guidance prevents that false positive rather than replacing analyzer diagnostics. + +Controls bound to variables or expressions cannot rely on table-field caption inheritance. For user-facing fields that need a label, supply a `Caption` or a `CaptionClass` that resolves to the intended caption. API pages are not human-facing UI; do not apply this UI-label guidance to their API contract names. ## Best Practice -`Caption = 'Customer No.';` paired with `ToolTip = 'Specifies …';`. Captions are short, noun-phrase, title-case for primary labels; sentence-case is allowed for descriptive labels that read as a sentence fragment. +Define the shared caption on the table field and let bound page fields inherit it. Add a page-level `Caption` only when there is no suitable inherited caption or the page genuinely needs different wording. Keep a valid `CaptionClass` rather than adding a redundant literal caption. -See sample: `caption-required-on-page-fields.good.al`. +Before reporting a missing caption, inspect the binding and source field, including dependency symbols when needed. If the source definition is unavailable, do not treat an omitted page property as proof that the caption is missing. Caption and tooltip requirements are separate: do not add a `ToolTip` just because a caption is being reviewed; see [tooltip inheritance guidance](tooltip-required-on-page-fields.md). + +See sample: `caption-required-on-page-fields.good.al`. Caption inheritance applies across BC versions; the sample uses BC24/runtime 13.0 or later to also define tooltips on its table fields. ## Anti Pattern -A field control with no `Caption` and no `CaptionClass`, or `Caption = '';`. The user sees the internal identifier as the column header and the translation pipeline has nothing to translate. +A user-facing field that needs a label but has no non-empty explicit or inherited caption and no resolving `CaptionClass` has a genuine labeling gap. This includes `Caption = '';` when no `CaptionClass` supplies the label. A variable name alone is not a translatable caption. + +The opposite review defect is flagging a bound field solely because it omits a page-level `Caption`, or inserting a copy of the table field's caption to satisfy AA0225/AA0226. That adds redundant text and prevents subsequent table-caption changes from flowing through to the page. See sample: `caption-required-on-page-fields.bad.al`. + +## References + +[Caption property](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/properties/devenv-caption-property) and [ToolTip property remarks documenting inheritance of both properties](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/properties/devenv-tooltip-property). diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al b/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al deleted file mode 100644 index 33c63fb..0000000 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al +++ /dev/null @@ -1,11 +0,0 @@ -codeunit 50262 "Sample Label Scope Bad" -{ - procedure LookupCustomer(CustomerNo: Code[20]) - var - Customer: Record Customer; - GreetingMsg: Label 'Hello %1', Comment = '%1 = Customer Name'; - begin - if Customer.Get(CustomerNo) then - Message(GreetingMsg, Customer.Name); - end; -} diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.good.al b/microsoft/knowledge/style/labels-declared-at-object-scope.good.al deleted file mode 100644 index 415bda7..0000000 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.good.al +++ /dev/null @@ -1,13 +0,0 @@ -codeunit 50263 "Sample Label Scope Good" -{ - var - GreetingMsg: Label 'Hello %1', Comment = '%1 = Customer Name'; - - procedure LookupCustomer(CustomerNo: Code[20]) - var - Customer: Record Customer; - begin - if Customer.Get(CustomerNo) then - Message(GreetingMsg, Customer.Name); - end; -} diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.md b/microsoft/knowledge/style/labels-declared-at-object-scope.md index 24a868e..91d75d7 100644 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.md +++ b/microsoft/knowledge/style/labels-declared-at-object-scope.md @@ -1,30 +1,18 @@ --- bc-version: [all] domain: style -keywords: [label, scope, procedure, translation, localization, xliff] +keywords: [label, scope, procedure, translation, localization, xliff, false-positive] technologies: [al] countries: [w1] application-area: [all] --- -# Declare Labels at object scope, not inside procedure `var` blocks +# Procedure-local Labels are valid ## Description -`Label` is the AL declaration that participates in the translation pipeline: the build extracts every Label declared in an object into the `.xlf` file shipped to translators, and the runtime substitutes the localized value when the object is loaded. Translation tooling discovers Labels by walking the object's top-level declarations. - -Labels declared inside a procedure-local `var` block are still **compiled** as Label values, but their participation in localization is fragile: depending on the BC version, the build pipeline, and the translation toolchain in use, procedure-local Labels may be missed during XLIFF extraction, may be re-emitted with auto-generated keys that change between builds, or may not be addressable by reviewers triaging translations. The reliable, supported pattern is to declare every Label in the object's top-level `var` block. - -The same rule applies to all object types that own behavior: codeunits, pages, tables, reports, queries, and their extensions. For shared messages used by multiple objects, declare the Label in the most appropriate owning object and reference it — do not duplicate the literal across procedure-scoped declarations in several places. +The AL language supports `Label` variables at both object and procedure scope. Microsoft documents the [Label data type](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-using-labels#label-data-type) without imposing an object-scope requirement, and the translation pipeline generates an XLF file containing [all labels used by the extension](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-work-with-translation-files#generating-the-xliff-file). There is no documented correctness or localization defect caused solely by declaring a Label in a procedure-local `var` block. ## Best Practice -Move every `Label` to the object's top-level `var` block. Use the appropriate suffix (`Msg`, `Err`, `Qst`, `Lbl`, `Tok`, `Txt`) on the variable name so reviewers and the translation team can see at a glance what role the string plays. Pair non-translatable strings (URLs, JSON/XML fragments, integration tokens) with `Locked = true`, as covered by `label-locked-for-non-translatable.md`. - -See sample: `labels-declared-at-object-scope.good.al`. - -## Anti Pattern - -Declaring `Label` inside a procedure-local `var` block — `procedure Lookup() var GreetingMsg: Label 'Hello %1';` — couples the translatable string to one procedure, hides it from object-level review, and depends on a translation pipeline behavior that is not part of the AL language contract. - -See sample: `labels-declared-at-object-scope.bad.al`. +Choose object scope when a Label is reused or when an established repository convention prefers central declarations; choose procedure scope when the Label belongs to one procedure. Do not report a correctness or localization finding solely because a Label is local. An explicit object-scope convention is at most a low-severity maintainability preference. This guidance applies equally to production and test apps: test code still needs localization where its strings are user-facing or translator-facing. diff --git a/microsoft/knowledge/style/tooltip-required-on-page-fields.bad.al b/microsoft/knowledge/style/tooltip-required-on-page-fields.bad.al index e6b356c..6f5f269 100644 --- a/microsoft/knowledge/style/tooltip-required-on-page-fields.bad.al +++ b/microsoft/knowledge/style/tooltip-required-on-page-fields.bad.al @@ -1,23 +1,29 @@ page 50251 "Sample Tooltip Bad" { PageType = Card; - SourceTable = Customer; layout { area(Content) { group(General) { - field("No."; Rec."No.") + Caption = 'General'; + field(CustomerNoValue; CustomerNoValue) { ApplicationArea = All; + Caption = 'Customer No.'; } - field(Amount; Rec."Balance (LCY)") + field(PreviewAmount; PreviewAmount) { ApplicationArea = All; + Caption = 'Preview Amount'; ToolTip = ''; } } } } + + var + CustomerNoValue: Code[20]; + PreviewAmount: Decimal; } diff --git a/microsoft/knowledge/style/tooltip-required-on-page-fields.good.al b/microsoft/knowledge/style/tooltip-required-on-page-fields.good.al index 1816de5..cd1fc8c 100644 --- a/microsoft/knowledge/style/tooltip-required-on-page-fields.good.al +++ b/microsoft/knowledge/style/tooltip-required-on-page-fields.good.al @@ -1,24 +1,62 @@ +// BC24 / runtime 13.0 or later. +table 50250 "Sample Tooltip Source" +{ + Caption = 'Tooltip Source'; + DataClassification = CustomerContent; + + fields + { + field(1; "No."; Code[20]) + { + Caption = 'No.'; + ToolTip = 'Specifies the unique number used to distinguish this entry from other entries.'; + } + field(2; Amount; Decimal) + { + Caption = 'Amount'; + ToolTip = 'Specifies the monetary value recorded for this entry; changing it updates the saved entry.'; + } + } + + keys + { + key(PK; "No.") + { + Clustered = true; + } + } +} + page 50250 "Sample Tooltip Good" { PageType = Card; - SourceTable = Customer; + SourceTable = "Sample Tooltip Source"; layout { area(Content) { group(General) { + Caption = 'General'; field("No."; Rec."No.") { ApplicationArea = All; - ToolTip = 'Specifies the number that identifies the customer.'; } - field(Amount; Rec."Balance (LCY)") + field(Amount; Rec.Amount) { ApplicationArea = All; - ToolTip = 'Shows the total balance in local currency.'; + ToolTip = 'Specifies the recorded amount to compare with the temporary preview amount.'; + } + field(PreviewAmount; PreviewAmount) + { + ApplicationArea = All; + Caption = 'Preview Amount'; + ToolTip = 'Specifies a temporary amount to compare with the recorded entry amount; this value is not saved.'; } } } } + + var + PreviewAmount: Decimal; } diff --git a/microsoft/knowledge/style/tooltip-required-on-page-fields.md b/microsoft/knowledge/style/tooltip-required-on-page-fields.md index a11d4a9..bd1dd2b 100644 --- a/microsoft/knowledge/style/tooltip-required-on-page-fields.md +++ b/microsoft/knowledge/style/tooltip-required-on-page-fields.md @@ -1,30 +1,44 @@ --- bc-version: [all] domain: style -keywords: [tooltip, page-field, aa0218, codecop, accessibility, specifies] +keywords: [tooltip, page-field, source-field, inheritance, aa0218, codecop, accessibility, specifies] technologies: [al] countries: [w1] application-area: [all] --- -# Every page field needs a `ToolTip` (CodeCop AA0218) +# Page fields need an explicit or inherited `ToolTip` (CodeCop AA0218) ## Description -CodeCop AA0218 requires a non-empty `ToolTip` property on every field control on a page. The tooltip is what users see on hover and is what screen readers announce; an empty or missing tooltip removes a piece of UI affordance that is part of BC's accessibility baseline. AppSource technical validation rejects pages with missing tooltips. The companion rules AA0219 and AA0220 push the wording further — tooltips should describe what the field shows, conventionally starting with `'Specifies …'`, though `'Shows …'` and similar variants are acceptable when they clearly describe the field's purpose. +User-facing page fields need tooltip text, but it does not have to be declared on each page control. Starting with BC24 (2024 release wave 1), runtime 13.0 supports `ToolTip` on table fields, and bound page fields inherit it unless they override it. A non-empty inherited tooltip satisfies the requirement; do not interpret CodeCop AA0218 as a requirement to repeat it on the page. -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. +For targets before runtime 13.0, table-field tooltip inheritance is not available, so user-facing page fields need page-level tooltips. Controls bound to variables or expressions also need page-level tooltips because they have no table field to inherit from. This is UI guidance, not a blanket requirement to add tooltips to every table field, including fields never exposed 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`. +AA0218's severity is configured per app and may be downgraded or disabled. Review should still report a genuinely missing tooltip, but absence of a page-level declaration alone is not evidence of a gap. See [bound page-field tooltip inheritance](../ui/bound-page-field-inherits-source-field-tooltip.md). ## 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". 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. +On runtime 13.0 or later, define shared tooltip text on the table field and omit duplicate page-level properties. Add a page-level `ToolTip` when no tooltip can be inherited or when the page needs different, context-specific help. Describe what the value shows, conventionally starting with "Specifies" or another clear phrasing. -See sample: `tooltip-required-on-page-fields.good.al`. +Make the text answer a question the caption does not: what the value is used for, which values or units are expected, or what changing it affects. Do not mechanically generate "Specifies the ." and consider the help complete. Use behavior established by the implementation or requirements; do not invent effects, defaults, or constraints to make a tooltip sound useful. Keep shared table-field help applicable to all pages that inherit it, and improve that shared text rather than duplicating it on each page. + +Before raising a `medium`-severity finding, check the target runtime, the control's binding, and the source field's tooltip, including dependency symbols when needed. Report a field with neither an explicit nor an inherited tooltip independently of whether AA0218 is active. If the source definition or target runtime is unavailable, do not assume a missing page property means missing tooltip text. + +See sample: `tooltip-required-on-page-fields.good.al` (BC24/runtime 13.0 or later). ## Anti Pattern -A field control with no `ToolTip` property at all, or `ToolTip = '';`. AA0218 flags both; the hover state is blank and the screen reader has nothing to announce. +A user-facing control with no page-level `ToolTip` and no non-empty source tooltip it can inherit, or a page-level `ToolTip = '';` that leaves the effective tooltip empty. + +Flagging a bound field that already inherits its tooltip, or adding the same tooltip to every page, is also incorrect: duplicate overrides add maintenance and translation work and prevent source-field tooltip changes from reaching those pages. + +Treating a non-empty tooltip that merely repeats the caption as useful help is a separate quality issue, not a missing-tooltip finding. Point out the concrete information users need rather than demanding longer wording or a page-level override for its own sake. See sample: `tooltip-required-on-page-fields.bad.al`. + +## References + +[ToolTip property](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/properties/devenv-tooltip-property). + +[Guidelines for tooltip text](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/user-assistance#guidelines-for-tooltip-text). 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 index 8e3e126..e53986b 100644 --- a/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.md +++ b/microsoft/knowledge/testing/permission-tests-must-lower-the-execution-context.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: testing -keywords: [testpermissions, restrictive, disabled, permissions-mock, lower-permissions, super, permission-test] +keywords: [testpermissions, restrictive, disabled, permissions-mock, lower-permissions, super, permission-test, false-positive] technologies: [al] countries: [w1] application-area: [all] @@ -13,14 +13,16 @@ application-area: [all] `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. +What matters is the effective permission context at the moment the protected operation runs, not which permission set object the test names. A test may establish that context through a composed role that includes the permission set under test rather than applying that set directly — that mirrors how the permission set actually reaches a user in production, where roles are assigned and permission sets are included. Such a test is adequate when it proves the boundary it claims: for an indirect (lowercase `imd`) grant, asserting `WritePermission()` is false before invoking the mediating codeunit shows that no direct access was granted and that the subsequent write succeeded only through code. + ## 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. +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 a permission context that actually contains the rights the scenario tests — either the permission set itself or a role that includes it — and restore or stop the mock afterward. Use `Disabled` only for suites that do not assert permission behavior, or where the test lowers the context explicitly through the test libraries instead of relying on the runner. Do not require a test to apply the permission set under test directly when it reaches the same rights through a composed role and then asserts the boundary. 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. +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. Do not report the mirror image: a test that lowers the context through a role including the permission set under test, and then asserts the boundary, has exercised that permission set and is not a coverage gap. See sample: `permission-tests-must-lower-the-execution-context.bad.al`. diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al index 1ecf47d..47743e0 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al @@ -1,43 +1,75 @@ -codeunit 50401 "Test UI Handlers Bad" +codeunit 50401 "Test UI Handler Proof Bad" { Subtype = Test; - // Several wiring mistakes, each of which fails at runtime rather than as a - // clean assertion the reviewer can read: - // * A UI call with no listed handler -> "unhandled UI" abort (the Message - // below has no handler). - // * The mirror mistake, listing a handler the path never hits, instead - // fails with "handler function was not executed". - // * A handler that hardcodes its answer and asserts inline, with no - // enqueue/dequeue -> nothing proves the RIGHT dialog fired the RIGHT - // number of times, and a failed inline assert can be swallowed by the - // calling UI operation. [Test] - [HandlerFunctions('ConfirmHandler')] - procedure PostDocumentConfirmsAndMessages() + [HandlerFunctions('CustomerCardHandler')] + procedure PreSetBooleanDoesNotProveCustomerCardResult() + var + Customer: Record Customer; begin - // No Initialize(): a value leaked by an earlier test corrupts this one. - RunPostingThatConfirmsAndMessages(); - // No AssertEmpty(): a missing or extra dialog goes unnoticed. + LibrarySales.CreateCustomer(Customer); + ActionSucceeded := true; + + Page.RunModal(Page::"Customer Card", Customer); + + // This only proves a value assigned before the action stayed true. + Assert.IsTrue(ActionSucceeded, 'The customer card action failed.'); end; - local procedure RunPostingThatConfirmsAndMessages() + [Test] + [HandlerFunctions('CustomerCardHandler')] + procedure MissingMessageHandlerFailsAtRuntime() + var + Customer: Record Customer; + begin + LibrarySales.CreateCustomer(Customer); + + Page.RunModal(Page::"Customer Card", Customer); + Message('Customer card closed.'); + end; + + [Test] + [HandlerFunctions('CustomerCardHandler,UnusedConfirmHandler')] + procedure UnreachedListedHandlerFailsAtRuntime() + var + Customer: Record Customer; + begin + LibrarySales.CreateCustomer(Customer); + + Page.RunModal(Page::"Customer Card", Customer); + end; + + [Test] + [HandlerFunctions('CustomerCardHandler,MandatoryNotificationHandler')] + procedure UnreachedNonoptionalNotificationHandlerFailsAtRuntime() + var + Customer: Record Customer; + begin + LibrarySales.CreateCustomer(Customer); + + Page.RunModal(Page::"Customer Card", Customer); + end; + + [ModalPageHandler] + procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin - // Raises a Confirm AND a Message, but only ConfirmHandler is listed: - // the Message has nothing to intercept it -> unhandled-UI runtime abort. - if Confirm('Post this document?', false) then - Message('Posting completed.'); end; [ConfirmHandler] - procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean) + procedure UnusedConfirmHandler(Question: Text[1024]; var Reply: Boolean) begin - // Hardcoded expectation and hardcoded reply. If the wrong dialog fires, - // this inline assert may never surface as the test's verdict. - Assert.AreEqual('Post this document?', Question, 'Wrong confirm.'); Reply := true; end; + [SendNotificationHandler] + procedure MandatoryNotificationHandler(var TheNotification: Notification): Boolean + begin + exit(true); + end; + var Assert: Codeunit "Library Assert"; + LibrarySales: Codeunit "Library - Sales"; + ActionSucceeded: Boolean; } diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al index f955477..753873f 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al @@ -1,57 +1,49 @@ -codeunit 50400 "Test UI Handlers Good" +codeunit 50400 "Test UI Handler Capture Good" { Subtype = Test; [Test] - [HandlerFunctions('ConfirmHandler,PostMessageHandler')] - procedure PostDocumentConfirmsAndMessages() + [HandlerFunctions('CustomerCardHandler')] + procedure CustomerCardShowsSelectedCustomer() + var + Customer: Record Customer; begin - Initialize(); + LibrarySales.CreateCustomer(Customer); + CapturedCustomerNo := ''; - // [GIVEN] the test enqueues, in interaction order, what each handler - // will see and how it should answer: the Confirm's expected - // question plus the reply to return, then the expected Message. - LibraryVariableStorage.Enqueue('Post this document?'); // expected question (substring) - LibraryVariableStorage.Enqueue(true); // reply ConfirmHandler returns - LibraryVariableStorage.Enqueue('Posting completed.'); // expected message (substring) + Page.RunModal(Page::"Customer Card", Customer); - // [WHEN] the code under test raises the Confirm and then the Message - RunPostingThatConfirmsAndMessages(); - - // [THEN] every enqueued expectation was consumed exactly once - LibraryVariableStorage.AssertEmpty(); + Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.'); end; - local procedure Initialize() + [Test] + [HandlerFunctions('CustomerCardHandler,CreditLimitNotificationHandler')] + procedure CustomerCardOpensForCustomerWithinCreditLimit() + var + Customer: Record Customer; begin - // Clear leftover values so a value leaked by an earlier test cannot - // cascade into this one. - LibraryVariableStorage.Clear(); + LibrarySales.CreateCustomer(Customer); + CapturedCustomerNo := ''; + + Page.RunModal(Page::"Customer Card", Customer); + + Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.'); end; - local procedure RunPostingThatConfirmsAndMessages() + [ModalPageHandler] + procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin - // Stands in for the production routine that confirms, then messages. - if Confirm('Post this document?', false) then - Message('Posting completed.'); + CapturedCustomerNo := CustomerCard."No.".Value(); end; - [ConfirmHandler] - procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean) + [SendNotificationHandler(true)] + procedure CreditLimitNotificationHandler(var CreditLimitNotification: Notification): Boolean begin - // Verify the RIGHT dialog fired (substring match), then return the - // reply the test enqueued for it. - Assert.ExpectedConfirm(LibraryVariableStorage.DequeueText(), Question); - Reply := LibraryVariableStorage.DequeueBoolean(); - end; - - [MessageHandler] - procedure PostMessageHandler(Message: Text[1024]) - begin - Assert.ExpectedMessage(LibraryVariableStorage.DequeueText(), Message); + exit(true); end; var Assert: Codeunit "Library Assert"; - LibraryVariableStorage: Codeunit "Library - Variable Storage"; + LibrarySales: Codeunit "Library - Sales"; + CapturedCustomerNo: Code[20]; } diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.md b/microsoft/knowledge/testing/ui-handlers-in-tests.md index 338e9ec..d451301 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.md +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.md @@ -1,28 +1,30 @@ --- bc-version: [all] domain: testing -keywords: [handler, handlerfunctions, confirm, message, strmenu, variable-storage, enqueue, unhandled-ui] +keywords: [handler, handlerfunctions, confirm, message, notification, optional-handler, enqueue, capture, runmodal, unhandled-ui] technologies: [al] countries: [w1] application-area: [all] --- -# Wire and verify UI handlers with enqueue-driven expectations +# Wire UI handlers and verify meaningful outcomes ## Description -A test runs headless: there is no interactive user to answer a dialog. Every UI call the executed path raises — `Confirm`, `Message`, error dialogs, `Page.Run`/`RunModal`, `Report.Run`/`RunModal`, request pages, `StrMenu`, `Notification.Send` — must be intercepted by a handler carrying the matching attribute (`[ConfirmHandler]`, `[MessageHandler]`, `[StrMenuHandler]`, `[ModalPageHandler]`, …) and named in the method's `[HandlerFunctions(...)]`. The list is a two-sided contract: raise a UI call with no listed handler and the platform aborts with an *unhandled UI* error; list a handler the path never hits and it fails with *"handler function was not executed"*. Both are runtime failures — the test never reaches its verdict, so a reviewer sees an infrastructure error instead of a result on the behavior under test. +A test runs headless, so every UI call on the executed path must be intercepted by a matching handler named in `[HandlerFunctions(...)]`. The list is a two-sided contract: an unhandled UI call aborts the test, while Microsoft documents that [every nonoptional listed handler must execute at least once](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/attributes/devenv-handlerfunctions-attribute#remarks) or the test fails. -Getting the handler *present* is only half the job; the handler must also verify the *right* dialog fired the *right* number of times. Do that by driving handlers from the test, not by hardcoding answers inside them. +Optionality is declared, not inferred. `SendNotificationHandler` and `RecallNotificationHandler` accept a `HandlerIsOptional` argument, so `[SendNotificationHandler(true)]` may stay listed on a run that never raises the notification, while the same attribute written without that argument is nonoptional like every other handler type. Notifications are conditional by nature, so an optional notification handler is listed precisely because the scenario may or may not reach it. + +Beyond that wiring guarantee, the test must verify the behavior it cares about. The appropriate pattern depends on the contract: a handler can capture concrete page state or a result and the test can assert that semantic postcondition after `RunModal`; assertions inside a handler are also supported. Queue/enqueue/dequeue and `LibraryVariableStorage.AssertEmpty` are useful when interaction order, count, text, replies, or a scripted sequence is itself part of the contract, but they are not mandatory for every handler. ## Best Practice -Make the test own the expectations and the handlers consume them. Before acting, the test `Enqueue`s — in interaction order — the expected text (a stable substring) and any reply each handler must return. The handler `Dequeue`s the expected text, verifies it with the purpose-built asserts (`Assert.ExpectedMessage`, `Assert.ExpectedConfirm`, `Assert.ExpectedStrMenu` — which match on a fragment, not the full localized caption), then `Dequeue`s and returns its reply. Finish the test body with `LibraryVariableStorage.AssertEmpty` to prove every enqueued interaction fired exactly once, and start each test with an `Initialize` that calls `LibraryVariableStorage.Clear` so a value leaked by an earlier test cannot cascade. List in `[HandlerFunctions]` precisely the handlers the scenario triggers — no superset "just in case", no subset that happens to work today. +List the handlers the scenario triggers, keep an optional notification handler listed for a notification the scenario may conditionally raise, and make each executed handler contribute meaningful evidence. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. For ordered or repeated interactions, let the test enqueue expectations, let handlers dequeue and verify them, clear storage during initialization, and finish with `AssertEmpty`. See sample: `ui-handlers-in-tests.good.al`. ## Anti Pattern -Omitting a handler for a UI call the path raises (unhandled-UI abort), padding the list with a handler the path never reaches ("handler function was not executed"), or writing handlers that hardcode their answer and assert inline with no enqueue/dequeue. The last is the subtle one: nothing proves the correct dialog fired the expected number of times, and an inline assertion that fails inside a handler can be swallowed by the calling UI operation, leaving the suite green while the behavior is broken. Skipping `Initialize`/`AssertEmpty` hides both a leaked queue and a missing or extra dialog. +Omitting a handler for a UI call, listing a nonoptional handler the path never reaches, or claiming action success from a Boolean set before the action runs. A handler that only closes a page can also leave the test without a semantic assertion. Do not flag the absence of queue storage by itself; require it only when the test needs to prove interaction order, count, text, replies, or a scripted sequence. Do not flag a listed `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` that the run does not reach, and never propose removing one: the entry is what keeps the test passing on the runs where the notification does fire. See sample: `ui-handlers-in-tests.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 index 87b539b..00dc9b8 100644 --- a/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md +++ b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md @@ -1,5 +1,5 @@ --- -bc-version: [all] +bc-version: [24..] domain: ui keywords: [tooltip, page-field, source-field, inheritance, aa0218, false-positive] technologies: [al] @@ -11,14 +11,20 @@ application-area: [all] ## 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. +Starting with BC24 (2024 release wave 1), runtime 13.0 supports `ToolTip` on table fields. A page field bound to a table field inherits the source field's `ToolTip` 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. This inheritance is not available when targeting earlier runtimes. -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. +The genuinely-missing case is different: a control with no inline `ToolTip` also has no text to inherit when it is unbound or its source table field carries no non-empty `ToolTip`. This leaves a real user-assistance gap. The compiler analyzer AA0218 detects this mechanically, but its severity is set by each app's ruleset and may be downgraded or disabled, so review should raise the genuine gap 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. +Check the target runtime and inspect the source field, including dependency symbols when needed. On runtime 13.0 or later, do not raise a missing-`ToolTip` finding for a bound page field whose source table field supplies a non-empty `ToolTip`, and do not add a duplicate page-level property. A page-level override is appropriate only when the page needs different help text or no tooltip can be inherited. + +Do raise a `medium`-severity finding when the field has no inline `ToolTip` **and** no inherited one, rather than assuming AA0218 will catch it downstream. If the source definition is unavailable, do not infer that its tooltip is missing. See [tooltip requirements across target versions](../style/tooltip-required-on-page-fields.md). ## 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. + +## References + +[ToolTip property](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/properties/devenv-tooltip-property). diff --git a/community/knowledge/ui/factbox-design.md b/microsoft/knowledge/ui/factbox-design.md similarity index 100% rename from community/knowledge/ui/factbox-design.md rename to microsoft/knowledge/ui/factbox-design.md diff --git a/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.bad.al b/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.bad.al new file mode 100644 index 0000000..e7e8e26 --- /dev/null +++ b/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.bad.al @@ -0,0 +1,59 @@ +table 50540 "Sample Shipping Agent Bad" +{ + fields + { + field(1; "Code"; Code[20]) + { + DataClassification = CustomerContent; + NotBlank = true; + } + field(2; Description; Text[100]) + { + DataClassification = CustomerContent; + } + } + + keys + { + key(PK; "Code") + { + Clustered = true; + } + } + + trigger OnInsert() + begin + TestField(Description); + end; +} + +page 50541 "Sample Shipping Agents Bad" +{ + PageType = List; + ApplicationArea = All; + UsageCategory = Lists; + SourceTable = "Sample Shipping Agent Bad"; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(Agents) + { + field("Code"; Rec."Code") + { + ApplicationArea = All; + ToolTip = 'Specifies the code of the shipping agent.'; + } + field(Description; Rec.Description) + { + ApplicationArea = All; + ToolTip = 'Specifies a description of the shipping agent.'; + // Required by OnInsert, but nothing marks it. The user types + // the row, leaves it, and only then gets the error. + } + } + } + } +} diff --git a/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.good.al b/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.good.al new file mode 100644 index 0000000..0c63162 --- /dev/null +++ b/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.good.al @@ -0,0 +1,61 @@ +table 50542 "Sample Shipping Agent" +{ + fields + { + field(1; "Code"; Code[20]) + { + DataClassification = CustomerContent; + NotBlank = true; + } + field(2; Description; Text[100]) + { + DataClassification = CustomerContent; + } + } + + keys + { + key(PK; "Code") + { + Clustered = true; + } + } + + trigger OnInsert() + begin + TestField(Description); + end; +} + +page 50543 "Sample Shipping Agents" +{ + PageType = List; + ApplicationArea = All; + UsageCategory = Lists; + SourceTable = "Sample Shipping Agent"; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(Agents) + { + field("Code"; Rec."Code") + { + ApplicationArea = All; + ToolTip = 'Specifies the code of the shipping agent.'; + ShowMandatory = true; + } + field(Description; Rec.Description) + { + ApplicationArea = All; + ToolTip = 'Specifies a description of the shipping agent.'; + // Mirrors the TestField in OnInsert. ShowMandatory is what the + // client reads for the marker, so it has to be set here. + ShowMandatory = true; + } + } + } + } +} diff --git a/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.md b/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.md new file mode 100644 index 0000000..91fc65e --- /dev/null +++ b/microsoft/knowledge/ui/showmandatory-on-code-required-page-fields.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: ui +keywords: [showmandatory, notblank, mandatory-field, red-asterisk, delayedinsert, testfield, page-field] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Mark code-required page fields with ShowMandatory + +> Contributions welcome — open a PR to refine or extend this article. + +## Description + +`ShowMandatory` draws the red asterisk on a page field and, per the platform documentation, enforces no validation. The reverse is not reliable: code that enforces a value — `TestField` in `OnInsert`/`OnModify`, a `NotBlank` table field, a mandatory setup value — does not guarantee that the page field renders as mandatory. Because the two halves are independent, it is easy to ship a field that the code requires but the UI presents as optional. Microsoft documents that `NotBlank` can mark primary-key fields, but current client behavior does not do so consistently; on non-primary-key fields, a value that was never entered is not validated at all. `ShowMandatory` also overrides any marking `NotBlank` would contribute, so set it explicitly when the page must communicate a requirement. The gap is widest on a list page with `DelayedInsert = true`, where the enforcing error surfaces only when the user leaves the row — after the rest of the line is typed, with nothing having indicated which field was missing. + +## Best Practice + +Set `ShowMandatory = true` on every visible, editable page field whose value the user must supply before the record can be committed or an action can complete, and leave the enforcement in place: the property is presentation, `TestField`/`Error` is the guarantee, and the two belong together in the same change. When the requirement is conditional, bind `ShowMandatory` to a Boolean variable or field that mirrors the condition the enforcement checks — the base application drives `Vendor Invoice No.` on the Purchase Invoice page from an `Ext. Doc. No. Mandatory` setup flag this way. Two expression limits are worth knowing: the property cannot call an AL method, so compute the value into a variable first, and a numeric field that has a default value counts as filled, so it never shows the asterisk. See sample: `showmandatory-on-code-required-page-fields.good.al`. + +## Anti Pattern + +A required field with no mandatory marker: the table's `OnInsert` or the page's `OnInsertRecord` calls `TestField` on a field, or `NotBlank` is expected to force entry, while the page field bound to it carries no `ShowMandatory`. On a `DelayedInsert = true` list page the user fills the row, leaves it, and gets an error naming a field that never looked different from the optional ones. Reviewer signal: code on the relevant commit or action path requires the user to supply a field, the corresponding page control is visible and editable, and its `ShowMandatory` property is missing or does not mirror the same condition. A `TestField` or `Error` elsewhere in `OnValidate` or `OnModify` is not sufficient evidence: the field may be populated by code, non-editable, or required only for another path. Setting `ShowMandatory = false` on a field that is unconditionally required on the current path is the same defect stated explicitly, and per the documentation it also overrides any marking `NotBlank` would otherwise contribute. See sample: `showmandatory-on-code-required-page-fields.bad.al`. + +## See also + +`ShowMandatory` property — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-showmandatory-property + +`NotBlank` property — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-notblank-property + +Review finding this article generalizes — https://github.com/microsoft/BCApps/pull/9315#discussion_r3568817946 diff --git a/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.bad.al b/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.bad.al new file mode 100644 index 0000000..a74c3d0 --- /dev/null +++ b/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.bad.al @@ -0,0 +1,96 @@ +report 50545 "Sample Statement Late Check" +{ + ApplicationArea = All; + UsageCategory = ReportsAndAnalysis; + Caption = 'Sample Statement Late Check'; + + dataset + { + dataitem(CustLedgerEntry; "Cust. Ledger Entry") + { + column(CustomerNo; "Customer No.") { } + column(Amount; Amount) { } + } + } + + requestpage + { + layout + { + area(content) + { + group(Options) + { + field(StatementDateField; StatementDate) + { + ApplicationArea = All; + Caption = 'Statement Date'; + ToolTip = 'Specifies the date the statement is printed for.'; + ShowMandatory = true; + } + } + } + } + // No OnQueryClosePage: nothing inspects the input while the page is open. + } + + var + StatementDate: Date; + StatementDateMissingErr: Label 'Enter a statement date.'; + + trigger OnPreReport() + begin + // The request page is already closed. The user cannot correct the date + // here — the run is aborted and every entry on the page is lost. + if StatementDate = 0D then + Error(StatementDateMissingErr); + end; +} + +report 50546 "Sample Statement Close Trap" +{ + ApplicationArea = All; + UsageCategory = ReportsAndAnalysis; + Caption = 'Sample Statement Close Trap'; + + dataset + { + dataitem(CustLedgerEntry; "Cust. Ledger Entry") + { + column(CustomerNo; "Customer No.") { } + column(Amount; Amount) { } + } + } + + requestpage + { + layout + { + area(content) + { + group(Options) + { + field(StatementDateField; StatementDate) + { + ApplicationArea = All; + Caption = 'Statement Date'; + ToolTip = 'Specifies the date the statement is printed for.'; + ShowMandatory = true; + } + } + } + } + + trigger OnQueryClosePage(CloseAction: Action): Boolean + begin + // No close-action guard. Cancel and Esc raise the error too, and an + // error prevents the page from closing — the user cannot get out. + if StatementDate = 0D then + Error(StatementDateMissingErr); + end; + } + + var + StatementDate: Date; + StatementDateMissingErr: Label 'Enter a statement date.'; +} diff --git a/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.good.al b/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.good.al new file mode 100644 index 0000000..4ad160a --- /dev/null +++ b/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.good.al @@ -0,0 +1,62 @@ +report 50547 "Sample Statement Good" +{ + ApplicationArea = All; + UsageCategory = ReportsAndAnalysis; + Caption = 'Sample Statement Good'; + + dataset + { + dataitem(CustLedgerEntry; "Cust. Ledger Entry") + { + column(CustomerNo; "Customer No.") { } + column(Amount; Amount) { } + } + } + + requestpage + { + layout + { + area(content) + { + group(Options) + { + field(StatementDateField; StatementDate) + { + ApplicationArea = All; + Caption = 'Statement Date'; + ToolTip = 'Specifies the date the statement is printed for.'; + ShowMandatory = true; + } + } + } + } + + trigger OnQueryClosePage(CloseAction: Action): Boolean + begin + // Only when the user confirmed the run. Erroring on Cancel or Esc + // would trap the user in a page that refuses to close. The error + // itself keeps the page open, so the date can be fixed in place. + if CloseAction = Action::OK then + CheckStatementDate(); + end; + } + + var + StatementDate: Date; + StatementDateMissingErr: Label 'Enter a statement date.'; + + trigger OnPreReport() + begin + // The same check for runs that have no request page: job queue entries, + // Report.Run with the request window suppressed, scheduled and + // web-service invocations. + CheckStatementDate(); + end; + + local procedure CheckStatementDate() + begin + if StatementDate = 0D then + Error(StatementDateMissingErr); + end; +} diff --git a/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.md b/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.md new file mode 100644 index 0000000..75c8a09 --- /dev/null +++ b/microsoft/knowledge/ui/validate-request-page-input-in-onqueryclosepage.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: ui +keywords: [request-page, onqueryclosepage, onprereport, closeaction, mandatory-input, report-validation, job-queue] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Validate request-page input in OnQueryClosePage, not only in OnPreReport + +> Contributions welcome — open a PR to refine or extend this article. + +## Description + +`OnPreReport` runs after the request page has closed and before the data items are processed. A validation error raised there aborts the run with the request page already gone: everything the user typed is lost, and the only way forward is to open the report again and retype it. The request page's own `OnQueryClosePage` trigger runs while the page is still open, and the platform does not close a page whose `OnQueryClosePage` raises an error or returns `false` — so the same check placed there leaves the user in front of their input, with the offending field still filled in and correctable. Moving the check rather than duplicating it fails the other way: a report can run with no request page at all — `Report.Run`/`Report.RunModal` with the request window suppressed, `UseRequestPage = false`, job queue entries, scheduled and web-service invocations — and `OnQueryClosePage` never fires on those paths. + +## Best Practice + +Put the validation in one local procedure and call it from both places: from the request page's `OnQueryClosePage`, so an interactive user can correct the input where they entered it, and from `OnPreReport` (or the relevant `OnPreDataItem`), so a run without a request page is still refused. Guard the interactive call on the close action — validate only when the user confirmed the run, for example `if CloseAction = Action::OK then`. The base application uses this shape; report 292, `Copy Sales Document`, validates its request-page input in `OnQueryClosePage` behind a close-action check. Mark the control with `ShowMandatory` as well, so the requirement is visible before the user submits — see `showmandatory-on-code-required-page-fields.md`. See sample: `validate-request-page-input-in-onqueryclosepage.good.al`. + +## Anti Pattern + +Validating mandatory request-page input only in `OnPreReport`. The check is correct and the report is never run with bad input, but every interactive mistake costs the user the whole request page: the error arrives after the page is gone, and filters, dates, and options all have to be entered again. Reviewer signal: a `TestField`, `Error`, or blank/zero-value check in `OnPreReport` or `OnPreDataItem` against a variable that is bound to a request-page control, in a report whose request page declares no `OnQueryClosePage`. + +The mirror defect is an `OnQueryClosePage` that validates without inspecting `CloseAction`: because an error prevents the page from closing, a user who presses Cancel or Esc to abandon the report is trapped in a request page that errors on every attempt to leave it. Validating only in `OnQueryClosePage` is the third variant — the interactive path behaves well, and a job queue entry runs the report with unchecked input. See sample: `validate-request-page-input-in-onqueryclosepage.bad.al`. + +## See also + +`OnQueryClosePage` (Request Page) trigger — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/triggers-auto/requestpage/devenv-onqueryclosepage-requestpage-trigger + +`OnPreReport` (Report) trigger — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/triggers-auto/report/devenv-onprereport-report-trigger diff --git a/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.bad.al b/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.bad.al new file mode 100644 index 0000000..9b5ad06 --- /dev/null +++ b/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.bad.al @@ -0,0 +1,52 @@ +// Ordinal 2 was renamed from CreditNote to CreditMemo with ordinal and caption kept. The compiler stays silent +// and AS0082 fires only against a baseline; every schema 2.0 consumer that filters on or posts CreditNote fails. +enum 50120 "Document Kind Bad" +{ + Extensible = true; + + value(0; Invoice) { Caption = 'Invoice'; } + value(1; Order) { Caption = 'Order'; } + value(2; CreditMemo) { Caption = 'Credit Memo'; } +} + +table 50121 "Document Header Bad" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "No."; Code[20]) { DataClassification = CustomerContent; } + field(2; Kind; Enum "Document Kind Bad") { DataClassification = CustomerContent; } + } + + keys + { + key(PK; "No.") { Clustered = true; } + } +} + +page 50122 "Document API Bad" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'documents'; + APIVersion = 'v1.0'; + EntityName = 'document'; + EntitySetName = 'documents'; + ODataKeyFields = SystemId; + SourceTable = "Document Header Bad"; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) { Caption = 'id'; Editable = false; } + field(number; Rec."No.") { Caption = 'number'; } + field(kind; Rec.Kind) { Caption = 'kind'; } + } + } + } +} diff --git a/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.good.al b/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.good.al new file mode 100644 index 0000000..ef3337b --- /dev/null +++ b/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.good.al @@ -0,0 +1,54 @@ +// Neither the name CreditNote nor its caption changes in place: schema 2.0 consumers bind to the name, +// schema 1.0 consumers to the caption. A new kind is appended; a retired kind is obsoleted, never deleted. +enum 50120 "Document Kind Good" +{ + Extensible = true; + + value(0; Invoice) { Caption = 'Invoice'; } + value(1; Order) { Caption = 'Order'; } + value(2; CreditNote) { Caption = 'Credit Note'; } + value(3; ReturnOrder) { Caption = 'Return Order'; } + value(4; Quote) { Caption = 'Quote'; ObsoleteState = Pending; ObsoleteReason = 'Quotes moved to the quotes API.'; ObsoleteTag = '3.0'; } +} + +table 50121 "Document Header Good" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "No."; Code[20]) { DataClassification = CustomerContent; } + field(2; Kind; Enum "Document Kind Good") { DataClassification = CustomerContent; } + } + + keys + { + key(PK; "No.") { Clustered = true; } + } +} + +page 50122 "Document API Good" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'documents'; + APIVersion = 'v1.0'; + EntityName = 'document'; + EntitySetName = 'documents'; + ODataKeyFields = SystemId; + SourceTable = "Document Header Good"; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(records) + { + field(id; Rec.SystemId) { Caption = 'id'; Editable = false; } + field(number; Rec."No.") { Caption = 'number'; } + field(kind; Rec.Kind) { Caption = 'kind'; } + } + } + } +} diff --git a/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.md b/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.md new file mode 100644 index 0000000..7988326 --- /dev/null +++ b/microsoft/knowledge/web-services/api-enum-values-are-a-contract-by-name-not-ordinal.md @@ -0,0 +1,44 @@ +--- +bc-version: [17..] +domain: web-services +keywords: [api-page, enum, enum-value-name, rename, ordinal, caption, schemaversion, breaking-change, dataverse, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Under OData schema version 2.0 an API enum field is a contract by member name; under 1.0 it is the caption + +> Contributions welcome — open a PR to refine or extend this article. + +## Description + +What an API page publishes for an enum field depends on the OData `$schemaversion` the caller receives, and never on the ordinal. Under schema 2.0 the field is a strongly typed enum: `$metadata`, every response and every `$filter` carry the AL member **names**, and captions are published separately through `entityDefinitions`. Under schema 1.0 the same field is `Edm.String` and responses carry the en-US **caption**. Microsoft's API v2.0 is always schema 2.0. Custom APIs defaulted to schema 1.0 through BC 23; BC 24 changed the default to 2.0, and a caller can still pin `?$schemaversion=1.0`. Dataverse virtual tables build on API v2.0 and match choices by the value's External Name, with the integer values documented as not stable. + +LLMs treat one carrier as universal. Some assume the caption is serialised and report every caption change as an API break; others assume the name is serialised and wave a rename through when its ordinal and caption are kept. Each is right for one schema version and wrong for the other, and neither knows that the schema version decides. Page-shape changes are covered by `version-apis-by-adding-not-mutating-published-versions.md`; ordinal stability for persisted rows by `enum-values-additive-at-end.md`. This article is about the values inside one exposed field. + +## Best Practice + +Establish which schema versions the field is served under before changing anything about its enum. Under schema 2.0 (Microsoft's API v2.0, an explicit `$schemaversion=2.0` in the consumer contract, or another reliable context signal) the member name is the contract: keep names stable, put wording changes in `Caption`, add a value by appending a new name with an ordinal above every existing one, and retire a value through `ObsoleteState` rather than by deleting it. For a custom API that clients may still call as schema 1.0, any install of BC 17 to 23 or a caller that pins 1.0, the caption is a contract as well: change neither name nor caption in place, or publish the change as a new `APIVersion` on a new page object. A rename is out in every case: AppSourceCop AS0082 rejects it against a baseline, and dependent extensions bind to the name. + +See sample: `api-enum-values-are-a-contract-by-name-not-ordinal.good.al`. + +## Anti Pattern + +Renaming a value on an enum that an API page field exposes while keeping its ordinal and caption, or re-pointing an API page field at a source field whose enum carries different member names. Under schema 2.0 every consumer that filters on, posts, or maps the old name fails at runtime and Dataverse choices built on the old External Name stop matching; AS0082 reports the rename only when AppSourceCop runs against a baseline package, and nothing reports the re-pointed field. + +Detection signal: a diff hunk that changes the name in a `value(...)` line while keeping its ordinal, on an enum used by a table field that a `PageType = API` page exposes; or an API page `field(...)` whose source expression moves to a field of another enum type. + +The mirror image is a review defect: suppressing a caption-change finding because "the API serialises names". That holds only under schema 2.0. Do not flag a `Caption` change when the reviewer can establish schema 2.0 for every consumer; on a custom API where clients may select schema 1.0, report a caption change on an exposed value as a consumer-visible change and ask for versioning. A value appended at the end changes no contract under either schema and is never a finding. + +See sample: `api-enum-values-are-a-contract-by-name-not-ordinal.bad.al`. + +## See also + +Deprecated features in the platform, Schema version for custom APIs (changed default in BC 24) — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/upgrade/deprecated-features-platform#changes-in-2024-release-wave-1-version-240 + +Transitioning from API v1.0 to API v2.0, Enums and Schema version — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/api-reference/v2.0/transition-to-api-v2.0#enums + +Working with Virtual Tables, Table fields — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/powerplatform/powerplat-entity-modeling#table-fields + +AppSourceCop AS0082 (rename) — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/analyzers/appsourcecop-as0082 and AS0083 (delete) — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/analyzers/appsourcecop-as0083 diff --git a/microsoft/skills/review/al-events-review.md b/microsoft/skills/review/al-events-review.md index de2700c..c854f1c 100644 --- a/microsoft/skills/review/al-events-review.md +++ b/microsoft/skills/review/al-events-review.md @@ -51,7 +51,7 @@ When the post-conflict worklist is empty because no applicable events knowledge The following targeted checks map diff signals to specific `events` articles. Treat each as a candidate-selection cue: when the signal appears in the changed code, add the named article to the worklist and evaluate it in Action. -- `IsHandled` raised without an immediately preceding `IsHandled := false;`, or one `IsHandled` variable reused across several raises with no reset between them — `initialize-ishandled-to-false-before-publishing`. +- An `IsHandled` value that can carry over as `true` (reused after an earlier raise, re-entered on a later loop iteration, input/global/field, or otherwise seeded) is passed to a publisher without a reset — `reset-ishandled-only-when-the-value-can-carry-over`. Do not match one non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false. - `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event later, so the after-event is skipped whenever the call is handled — `preserve-onafter-execution-when-ishandled-skips-the-body`. - Any parameter added to a public Business/Integration event procedure, regardless of position; do not flag additions or reordering on `local`/`internal` publishers merely because a new parameter was not appended — `add-new-event-parameters-at-the-end`. - A shipped Business/Integration event renamed or removed, or an existing parameter renamed, removed, retyped, or changed to/from `var`, based on the mistaken assumption that `local` or `internal` prevents dependent subscription; parameter order alone is not a subscriber-contract violation — `treat-local-and-internal-events-as-subscriber-contracts`. diff --git a/microsoft/skills/review/al-performance-review.md b/microsoft/skills/review/al-performance-review.md index 3262179..bf2f4e8 100644 --- a/microsoft/skills/review/al-performance-review.md +++ b/microsoft/skills/review/al-performance-review.md @@ -47,7 +47,8 @@ Apply these targeted cues even when simple token overlap would rank the article - 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-commit-inside-loops.md` when `Commit()` is inside a record-iteration body or a checkpoint loop lacks persisted progress that excludes completed work on retry. Do not match a commit after a complete business unit when the same transaction persists a restart-safe watermark/state and errors propagate. Still match a full-tail `FindSet` with periodic commits as unbounded retrieval; restart safety does not make it `TOP X`. +- Worklist `prefer-modifyall-over-per-row-modify.md` for a constant-assignment `Modify(false)` loop with no validation or per-row semantics. Worklist `triggers-and-media-field-regress-modifyall.md` when table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields affect a bulk path. A progress dialog does not generically exempt a loop; accept it only when the equivalent bulk call already falls back to individual operations and semantics are preserved. - 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. @@ -74,7 +75,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a performance defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material performance defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly performance; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. -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`. +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()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-privacy-review.md b/microsoft/skills/review/al-privacy-review.md index ea95269..0bbd8ae 100644 --- a/microsoft/skills/review/al-privacy-review.md +++ b/microsoft/skills/review/al-privacy-review.md @@ -70,7 +70,7 @@ Set `confidence` to: 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`. +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()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-security-review.md b/microsoft/skills/review/al-security-review.md index 12956bd..8472afe 100644 --- a/microsoft/skills/review/al-security-review.md +++ b/microsoft/skills/review/al-security-review.md @@ -70,7 +70,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a security 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 security 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 security; 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. -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`. +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()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-style-review.md b/microsoft/skills/review/al-style-review.md index ee4daf5..bffef4d 100644 --- a/microsoft/skills/review/al-style-review.md +++ b/microsoft/skills/review/al-style-review.md @@ -60,7 +60,7 @@ When the post-conflict worklist is empty because no applicable style knowledge e For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Style findings rarely reach `blocker` — reserve it for cases where the knowledge file documents a platform-level requirement (for example, API page property constraints the OData runtime rejects). Most style findings are `minor` or `info`; egregious misuse (`Error` with pre-built Text losing translation and telemetry classification) may reach `major`. -Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, 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. +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 — lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, or a misleading named invocation. A procedure-local `Label` is valid and is not a correctness or localization finding; an explicit repository preference for object scope is at most low-severity maintainability guidance. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. Set `confidence` to: @@ -70,7 +70,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a style 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 clear, widely-accepted AL style violation with a concrete basis a knowledgeable BC reviewer would agree on — steelman it first and drop personal preference, speculation, and any single defensible formatting choice among several; when in doubt, omit. The scope is strictly style — naming, labelling, formatting, and analyzer-adjacent conventions. A correctness, logic, data-integrity, or contract defect is NOT a style finding even when it can be reworded as a convention: a method that mutates a shared `Record`'s filters, an unfiltered `DeleteAll`, a violated interface contract, or a wrong boolean guard are behavioural defects, not conventions — do not emit them here under a style framing. If a specific domain leaf covers the concern (performance, security, error-handling, …) it belongs there; if no knowledge file in any domain covers it, it belongs to the `al-code-review` super-skill's cross-cutting self-review agent channel (`from-sub-skill: "agent"`, `severity` capped at `minor`), not to this leaf. A reliable test: if you cannot cite a style `## Best Practice`/`## Anti Pattern` for the concern, it is very likely not a style finding. 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. -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`. +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()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 2483f12..8bad734 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-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 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(...)]`, `[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`). +- 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`, `SendNotificationHandler`, `RecallNotificationHandler`, `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. @@ -50,7 +50,7 @@ The following targeted checks cover every current `testing` article. Treat each - 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`. +- A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Only nonoptional handlers have to execute: a listed handler declared `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` is optional by design, so do not treat it as unmatched when the run never raises the notification. 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`. @@ -64,6 +64,8 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice` - When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. - 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 `ui-handlers-in-tests`, use `major` when missing or incorrectly listed handlers make the test fail at runtime. Use `minor` when the test executes but lacks a meaningful semantic postcondition, including a pre-set Boolean used as proof. Do not escalate solely because a handler does not use queue storage or asserts inside the handler. + Set `confidence` to: - `high` when the detection is based on an unambiguous pattern match (attribute, handler declaration, assertion sequence, or fixture call). @@ -72,7 +74,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a testing 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 testing 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 AL testing; 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. -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: add the matching `ExpectedError` assertion after `asserterror`; add or remove a handler name in `HandlerFunctions`; add `LibraryVariableStorage.Clear` or `AssertEmpty`; or replace hand-rolled fixture creation with an evident library call). 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`. +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: add the matching `ExpectedError` assertion after `asserterror`; add or remove a handler name in `HandlerFunctions`, except that a listed optional notification handler must never be proposed for removal; add `LibraryVariableStorage.Clear` or `AssertEmpty` when queue/LVS intentionally verifies interaction order, count, text, replies, or a scripted sequence; or replace hand-rolled fixture creation with an evident library call). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index 81ab12e..c0af5af 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -63,7 +63,7 @@ Set `confidence` to: 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`. +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()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index 879e788..667ea32 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -66,7 +66,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a upgrade 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 upgrade or breaking-change 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 upgrade; 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. -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`. +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()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/plugin.json b/plugin.json index 0934bd3..c99785b 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "name": "bcquality", - "description": "Quality skills and knowledge for Business Central development. Exposes a review bridge skill that drives the BCQuality Entry protocol over the installed knowledge base.", - "version": "0.1.0", + "description": "Quality skills and knowledge for Business Central development. Exposes a standalone AL review adapter backed by BCQuality's Entry protocol.", + "version": "0.2.0", "author": { "name": "microsoft/BCQuality", "url": "https://github.com/microsoft/BCQuality" @@ -16,6 +16,6 @@ "quality" ], "skills": [ - "./skills/bcquality-al-review/" + "./skills/" ] } diff --git a/skills/README.md b/skills/README.md index d0500a3..3d8fdfb 100644 --- a/skills/README.md +++ b/skills/README.md @@ -1,6 +1,9 @@ # BCQuality global skills -This folder contains the skills that are not owned by any single layer. There are two kinds: +This folder contains BCQuality's layer-independent protocol files and the +host-native adapter used by standalone plugin installations. + +The protocol files have two kinds: - **The entry-point skill** — the first skill an agent invokes at runtime. - **The three meta-skill contracts** — stable references that define what the rest of BCQuality means. @@ -23,6 +26,35 @@ Routing logic lives in Entry, not in the orchestrator. An agent that knows only READ and DO are read on demand — typically by the first action skill the agent executes after dispatch. They are not prerequisites for invoking Entry. WRITE is only used when scaffolding new content. +## Standalone plugin adapter + +| Path | Role | +|---|---| +| [`al-code-review/SKILL.md`](al-code-review/SKILL.md) | Exposes BCQuality through the standard `SKILL.md` format when this repository is installed as a plugin. | + +The adapter is deliberately thin. It translates the caller's request into an +Entry task context, then follows Entry's dispatch without owning routing, +review, index, or output policy. It is not an action skill, is not considered +by Entry, and should not accumulate behavior already defined by `entry.md`, +`read.md`, `do.md`, or a layered action skill. + +This gives the two skill formats distinct roles: + +- `skills/al-code-review/SKILL.md` is the public host integration surface for a + standalone plugin installation. +- `microsoft/skills/review/al-code-review.md` is BCQuality's internal + Microsoft-layer super-skill for coordinating a broad AL review. + +The host adapter and internal coordinator deliberately share the +`al-code-review` name because they represent the same user-facing operation in +their respective formats. Their locations distinguish their roles. The +adapter remains distinct from BC-ALAgents' separately installed `al-review` +skill, avoiding a collision in hosts that use one shared skill inventory. The +reference from the adapter to Entry, and from a dispatched super-skill to its +leaf skills, is intentional progressive disclosure. It avoids registering +every internal BCQuality protocol file as an ambient host skill while allowing +each review domain to run in an isolated context. + These contracts are stable. Changes require a PR approved by both maintainers. For the end-to-end flow — from orchestrator trigger through to findings integration — see [`../agent-consumption.md`](../agent-consumption.md). For the high-level project framing, see [`../README.md`](../README.md). diff --git a/skills/al-code-review/SKILL.md b/skills/al-code-review/SKILL.md new file mode 100644 index 0000000..991a771 --- /dev/null +++ b/skills/al-code-review/SKILL.md @@ -0,0 +1,69 @@ +--- +name: al-code-review +description: Review Business Central AL code changes using BCQuality's curated rules. Use for an AL pull request, working-tree diff, branch, or individual AL file when BCQuality is installed as a standalone plugin. +--- + +# AL code review + +This is BCQuality's host-native adapter for standalone plugin installations. It +is not a BCQuality action skill and contains no review or routing policy. Its +only responsibility is to translate the caller's request into an Entry task +context and execute the resulting dispatch. + +## Execute + +1. Resolve `PLUGIN_ROOT` to the directory containing this plugin's root + `plugin.json`. This file is + `PLUGIN_ROOT/skills/al-code-review/SKILL.md`; when the host does not expose + the plugin root, resolve it two levels above this file. +2. Build the `task-context` required by + `PLUGIN_ROOT/skills/entry.md`: + - Copy the caller's actual request verbatim into `goal`; do not replace a + focused request such as "review performance" with a generic full-review + goal. + - Set `inputs-available` to the inputs actually available to the review, + normally `pr-diff` for changes or `file-path` for one file. + - Set `technologies: [al]` when the input is known to be AL. + - Pass `bc-version`, `countries`, and `application-area` only when supplied + or reliably determined. + - If `BCQUALITY_ENABLED_LAYERS` is set, split its comma-separated value and + pass the trimmed, non-empty entries as `enabled-layers`; otherwise omit the + field and let Entry apply its default. + - If `BCQUALITY_DISABLED_SKILLS` is set, split its comma-separated value and + pass the trimmed, non-empty entries as `disabled-skills`; otherwise omit + the field. +3. Read and execute `PLUGIN_ROOT/skills/entry.md` exactly as written, including + its Preparation step. Entry is authoritative for index freshness, routing, + defaults, and failure behavior; this adapter must not duplicate or weaken + those rules. Entry is written for a checkout whose root is the current + directory, so resolve every repo-relative path it names against + `PLUGIN_ROOT` rather than the caller's working directory, which is the + user's own project. In particular, run Preparation's index build as + `pwsh PLUGIN_ROOT/tools/Build-KnowledgeIndex.ps1`: the generator resolves + its own root and writes `PLUGIN_ROOT/knowledge-index.json`, which is not + shipped and is therefore absent on a fresh install. If `pwsh` is + unavailable or the build fails, continue — READ falls back to path-based + discovery — but do not treat a failed build as a failed review. +4. Follow Entry's **How the agent uses the dispatch** instructions. Invoke only + the returned action skills, pass each dispatch entry's exact input subset, + and read `PLUGIN_ROOT/skills/read.md` and `PLUGIN_ROOT/skills/do.md` on + demand. When a dispatched super-skill requests isolated leaf execution and + the host supports child contexts, use them. +5. Return each dispatched action skill's findings report unchanged. If Entry + returns `no-match` or `failed`, return its dispatch record unchanged. + +The internal `microsoft/skills/review/al-code-review.md` action skill remains +the canonical coordinator for a broad AL review. Entry decides whether that +super-skill or a narrower domain skill applies; this host adapter never chooses +between them. + +## Layer selection is not a deny mechanism + +A plugin install ships the whole BCQuality tree, so `enabled-layers` here can +only narrow *discovery*: the files of a layer left out of the list still exist +on disk. This differs from the clone model Entry's Preparation step describes, +where a consumer prunes its checkout to policy before the agent runs and the +index is rebuilt over the pruned tree. Treat `BCQUALITY_ENABLED_LAYERS` as a +selection filter, never as a security boundary. A host that needs a genuine +deny mechanism must prune the installed tree itself. + diff --git a/skills/bcquality-al-review/SKILL.md b/skills/bcquality-al-review/SKILL.md deleted file mode 100644 index 32c8207..0000000 --- a/skills/bcquality-al-review/SKILL.md +++ /dev/null @@ -1,100 +0,0 @@ ---- -name: bcquality-al-review -description: Review Business Central AL code changes using the BCQuality knowledge base. Use when reviewing an AL pull request, a working-tree diff, or a single AL file, and you want findings backed by BCQuality's curated, BC-specific quality rules. ---- - -# BCQuality AL review - -This skill drives the BCQuality **Entry protocol** over the knowledge base that ships -inside this plugin. It is the plugin entry point for consumers (orchestrators, CLIs) -that do not already know BCQuality's internal conventions — the only convention they -need is "invoke this skill for an AL review." - -BCQuality itself is orchestrator-agnostic content: knowledge files plus routing and -action skills. This bridge is the thin consumer glue that lets a plugin host run that -content without hardcoding BCQuality's layout. - -## When to use - -- Reviewing an AL pull request or an uncommitted working-tree diff. -- Reviewing a single AL file. -- Any task whose goal is "review Business Central / AL code for quality issues." - -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 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. - -## Steps - -1. **Refresh the knowledge index (best effort).** If `pwsh` is available, run - `pwsh PLUGIN_ROOT/tools/Build-KnowledgeIndex.ps1` from `PLUGIN_ROOT` to (re)generate - `PLUGIN_ROOT/knowledge-index.json` over the installed tree. This is a discovery - accelerator only — if `pwsh` is missing or the build fails, continue; the review - skills fall back to path-based discovery. - -2. **Run Entry.** Read `PLUGIN_ROOT/skills/entry.md` and execute it against a - task context describing the review: - - ```yaml - task-context: - goal: "Review the AL changes for quality issues" - inputs-available: [pr-diff] # or [file-path] for single-file review - technologies: [al] - enabled-layers: [microsoft, community, custom] # see "Layer selection" below - ``` - - **Layer selection.** `enabled-layers` defaults to all three layers. A host can - narrow it by setting the `BCQUALITY_ENABLED_LAYERS` environment variable to a - comma-separated subset (e.g. `microsoft` or `microsoft,community`); when set, pass - exactly those layers instead of the default. This is the plugin path's only knob - for layer policy — see the limitation in Notes. - - Fill `bc-version`, `countries`, and `application-area` only when the caller - supplies them; omit them otherwise (an omitted dimension is unconstrained). - -3. **Follow the dispatch record.** Entry returns a dispatch record naming the action - skill(s) to invoke — for a PR review this is normally - `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, - 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. - -## Notes - -- This skill adds nothing to BCQuality's knowledge or routing logic; it only bootstraps - the existing Entry protocol from a plugin host. Knowledge and skill changes belong in - the layers under `PLUGIN_ROOT/microsoft/`, `PLUGIN_ROOT/community/`, and - `PLUGIN_ROOT/custom/`, not here. -- **Layer pruning is coarser than the URL/clone model.** In the clone model a consumer - prunes its checkout to policy *before* the agent runs, and the knowledge index is - rebuilt over the pruned tree, so a denied layer can never leak into discovery. A - plugin install ships the whole tree, so this bridge can only *narrow discovery* via - `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'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). 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 23bfb4a..a79c5fc 100644 --- a/skills/do.md +++ b/skills/do.md @@ -220,7 +220,7 @@ A review super-skill MUST preserve `domain` verbatim when rolling a leaf finding **`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`. +Emit `suggested-code` whenever the fix is small, local, and mechanical: deleting unreachable code; replacing one expression (`Count() > 0` → `not IsEmpty()`); 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`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but `suggested-code` is omitted, set `findings[].suggested-code-omission-reason` to a short explanation (for example, `requires choosing a real event id` or `fix spans multiple non-contiguous locations`). The `suggested-code` payload supplements `message`; it does not replace the explanation in `message`. diff --git a/skills/entry.md b/skills/entry.md index f094aa6..0196c35 100644 --- a/skills/entry.md +++ b/skills/entry.md @@ -35,7 +35,7 @@ task-context: ## Preparation — knowledge index -Before routing, ensure the knowledge index is current for the **live** clone. The dispatched review skills read `knowledge-index.json` (at the clone root) at their Source step instead of opening every knowledge file — see READ's [Retrieval workflow](read.md). Because a consumer prunes its clone to policy *before* the agent runs, the index MUST be built over the clone as it exists now, so it lists exactly the articles that survived pruning and never an article the consumer denied: +Before routing, ensure the knowledge index is current for the **live** clone. The dispatched review skills read `knowledge-index.json` (at the clone root) at their Source step instead of opening every knowledge file — see READ's [Retrieval workflow](read.md). When a consumer prunes its clone to policy *before* the agent runs, the index MUST be built over the clone as it exists now, so it lists exactly the articles that survived pruning and never an article the consumer denied: - If `knowledge-index.json` is absent — or you cannot confirm it reflects the current knowledge tree — regenerate it by running, from the checkout root: @@ -44,6 +44,8 @@ Before routing, ensure the knowledge index is current for the **live** clone. Th ``` It defaults to indexing this checkout and writes `knowledge-index.json` at the root in well under a second. When in doubt, rebuild: a sub-second rebuild is always cheaper than a stale or over-listing index, which is a correctness risk. +- The paths above assume the checkout root is the current directory. A caller that enters Entry from elsewhere — a plugin host, whose working directory is the user's own project — MUST resolve them against the BCQuality root it already knows instead. The generator resolves its own root, so invoking it by absolute path indexes and writes the right tree. +- Pruning is the consumer's job, not Entry's, and not every consumer does it: an installation that ships the whole tree gets no deny guarantee from this step. There, `enabled-layers` narrows discovery only, and the unlisted layers' files remain on disk. - This is a side step. It MUST NOT change Entry's output — the dispatch record below is the only thing Entry emits, and build logs are never part of the dispatch JSON. Generation is **owned by BCQuality**: the generator ships here next to the skills and knowledge it derives from, and the consuming orchestrator neither builds nor knows about the index. diff --git a/skills/write.md b/skills/write.md index 731f469..8096f1a 100644 --- a/skills/write.md +++ b/skills/write.md @@ -80,8 +80,10 @@ Knowledge files do not contain code. Samples live as **sibling files** next to t ## Choosing a layer -- **`/microsoft/knowledge//`** — platform-endorsed guidance. Authored or approved by the BC platform team. Use this layer only when the guidance reflects a platform guarantee or official recommendation. -- **`/community/knowledge//`** — shared community patterns. The default layer for contributions from outside the platform team. Content here can be promoted to `/microsoft/` once it proves itself. +In the shared upstream layers, keep an action skill and the canonical knowledge it acts on in the same layer. The action skill's ownership determines the destination; the author's affiliation does not. Do not use `/community/knowledge/` as a staging area for articles in a domain already owned by a Microsoft-endorsed skill. A cross-layer split is acceptable only as a short-lived migration state while the skill or corpus is being promoted. Custom overrides are intentionally exempt because they extend shared skills from a consumer fork. + +- **`/microsoft/knowledge//`** — guidance owned by a Microsoft-endorsed action skill. It has been approved as platform-endorsed guidance, whether authored by Microsoft or contributed by the community. +- **`/community/knowledge//`** — knowledge that accompanies a community-owned action skill. Promote the knowledge with the skill when that skill becomes Microsoft-endorsed. - **`/custom/knowledge//`** — partner or customer overrides. Generally does not appear in the BCQuality repository itself; `/custom/` lives in consumer repositories. ### Writing to `/custom/` — fork precondition diff --git a/tools/Test-ReviewFixtures.ps1 b/tools/Test-ReviewFixtures.ps1 index 3f3f144..a9912b7 100644 --- a/tools/Test-ReviewFixtures.ps1 +++ b/tools/Test-ReviewFixtures.ps1 @@ -109,12 +109,44 @@ if (([double]$manifest.minimumCleanRate -lt 0) -or ([double]$manifest.minimumCle $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 +$layers = @( + [pscustomobject]@{ Name = 'microsoft'; Rank = 1 } + [pscustomobject]@{ Name = 'community'; Rank = 2 } + [pscustomobject]@{ Name = 'custom'; Rank = 3 } ) +$layerRanks = @{} +foreach ($layer in $layers) { + $layerRanks[[string]$layer.Name] = [int]$layer.Rank +} +$leafCandidates = @( + foreach ($layer in $layers) { + $reviewDirectory = Join-Path $Root "$($layer.Name)/skills/review" + if (-not (Test-Path -LiteralPath $reviewDirectory -PathType Container)) { + continue + } + Get-ChildItem -LiteralPath $reviewDirectory -File -Filter 'al-*-review.md' | + Where-Object Name -ne 'al-code-review.md' | + ForEach-Object { + [pscustomobject]@{ + Domain = $_.BaseName -replace '^al-', '' -replace '-review$', '' + Layer = $layer.Name + Rank = $layer.Rank + RelativePath = [System.IO.Path]::GetRelativePath($Root, $_.FullName).Replace('\', '/') + } + } + } +) +$leafSkills = @( + $leafCandidates | + Group-Object Domain | + ForEach-Object { $_.Group | Sort-Object Rank -Descending | Select-Object -First 1 } | + Sort-Object Domain +) +$leafDomains = @($leafSkills | ForEach-Object Domain) +$leafByDomain = @{} +foreach ($leafSkill in $leafSkills) { + $leafByDomain[[string]$leafSkill.Domain] = $leafSkill +} $overrides = @{} if ($manifest.PSObject.Properties.Name -contains 'overrides') { @@ -130,9 +162,35 @@ foreach ($overrideDomain in $overrides.Keys) { $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 + $articleCandidates = @( + foreach ($layer in $layers) { + $knowledgeDirectory = Join-Path $Root "$($layer.Name)/knowledge/$domain" + if (-not (Test-Path -LiteralPath $knowledgeDirectory -PathType Container)) { + continue + } + Get-ChildItem -LiteralPath $knowledgeDirectory -File -Filter '*.md' | + 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) + } | + ForEach-Object { + [pscustomobject]@{ + BaseName = $_.BaseName + File = $_ + Rank = $layer.Rank + ArticlePath = [System.IO.Path]::GetRelativePath($Root, $_.FullName).Replace('\', '/') + } + } + } + ) + $articles = @( + $articleCandidates | + Group-Object BaseName | + ForEach-Object { $_.Group | Sort-Object Rank -Descending | Select-Object -First 1 } | + Sort-Object BaseName + ) + if (-not $articles.Count) { + $problems.Add("${domain}: no enabled knowledge layer has an article with both .good.al and .bad.al companion samples.") | Out-Null continue } @@ -143,27 +201,33 @@ foreach ($domain in $leafDomains) { 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 + $selectedArticle = $articles | Where-Object BaseName -eq $articleName | Select-Object -First 1 + if (-not $selectedArticle) { + $articleExists = @( + foreach ($layer in $layers) { + $articleFile = Join-Path $Root "$($layer.Name)/knowledge/$domain/$articleName.md" + if (Test-Path -LiteralPath $articleFile -PathType Leaf) { + $articleFile + } + } + ).Count -gt 0 + if ($articleExists) { + $problems.Add("${domain}: override article does not have both .good.al and .bad.al companion samples: $articleName.md") | Out-Null + } else { + $problems.Add("${domain}: override article does not exist: $articleName.md") | Out-Null + } + continue } } 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 + $selectedArticle = $articles | 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)" + $articlePath = [string]$selectedArticle.ArticlePath + $sampleDirectory = (Split-Path -Parent $articlePath).Replace('\', '/') $context = if ($override -and ($override.PSObject.Properties.Name -contains 'context')) { [string]$override.context } else { @@ -173,7 +237,7 @@ foreach ($domain in $leafDomains) { $case = [pscustomobject]@{ id = "$domain-$kind" domain = $domain - input = "microsoft/knowledge/$domain/$($selectedArticle.BaseName).$kind.al" + input = "$sampleDirectory/$($selectedArticle.BaseName).$kind.al" expected = if ($kind -eq 'bad') { @($articlePath) } else { @() } } if ($context) { @@ -188,7 +252,7 @@ $seenIds = [System.Collections.Generic.HashSet[string]]::new([System.StringCompa foreach ($case in $cases) { $id = [string]$case.id $domain = [string]$case.domain - $input = [string]$case.input + $inputRelativePath = [string]$case.input $expected = @($case.expected) if ([string]::IsNullOrWhiteSpace($id)) { @@ -200,15 +264,15 @@ foreach ($case in $cases) { $problems.Add("${id}: domain '$domain' has no registered al-$domain-review leaf.") | Out-Null } - $inputPath = Join-Path $Root $input + $inputPath = Join-Path $Root $inputRelativePath if (-not (Test-Path -LiteralPath $inputPath -PathType Leaf)) { - $problems.Add("${id}: input does not exist: $input") | Out-Null + $problems.Add("${id}: input does not exist: $inputRelativePath") | Out-Null } - if ($expected.Count -and $input -notmatch '\.bad\.[^.]+$') { - $problems.Add("${id}: positive case must use a .bad sample: $input") | Out-Null + if ($expected.Count -and $inputRelativePath -notmatch '\.bad\.[^.]+$') { + $problems.Add("${id}: positive case must use a .bad sample: $inputRelativePath") | Out-Null } - if (-not $expected.Count -and $input -notmatch '\.good\.[^.]+$') { - $problems.Add("${id}: clean case must use a .good sample: $input") | Out-Null + if (-not $expected.Count -and $inputRelativePath -notmatch '\.good\.[^.]+$') { + $problems.Add("${id}: clean case must use a .good sample: $inputRelativePath") | Out-Null } foreach ($reference in $expected) { @@ -218,7 +282,7 @@ foreach ($case in $cases) { } } if ($expected.Count) { - $sampleSlug = ([System.IO.Path]::GetFileName($input) -replace '\.(?:good|bad)\.[^.]+$', '') + $sampleSlug = ([System.IO.Path]::GetFileName($inputRelativePath) -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 @@ -311,9 +375,16 @@ if ($PrepareDirectory) { } | 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) + $domainArticles = @( + $fullIndex.articles | + Where-Object domain -eq $domain | + Sort-Object @{ Expression = { $layerRanks[[string]$_.layer] }; Descending = $true }, path | + Group-Object { [System.IO.Path]::GetFileName([string]$_.path) } | + ForEach-Object { $_.Group | Select-Object -First 1 } | + Sort-Object path + ) $domainIndexName = "index-$domain.json" - $leafPath = "microsoft/skills/review/al-$domain-review.md" + $leafPath = [string]$leafByDomain[$domain].RelativePath $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."