From 07140e2223485a9c2e4b02105e02aa1d95cabf22 Mon Sep 17 00:00:00 2001 From: Tharanga Chandrasekara Date: Sun, 7 Jun 2026 12:26:47 +1200 Subject: [PATCH] Add new action skills for AL testing and documentation - Introduced `al-test-writer` to generate AL test codeunits for production objects based on TDD principles. - Added `al-userguide-test-writer` to create test codeunits from user guide steps, mapping actions and assertions. - Implemented `bc-extension-test-guide` to generate a comprehensive TEST_GUIDE.md for Business Central extensions, covering various categories. - Created `bc-webclient-runner` to automate UI testing of the Business Central web client, capturing screenshots and asserting UI states. - Developed `page-scripting-e2e` to produce a recording plan for Page Scripting, ensuring a structured approach to browser-level testing. --- .altestrunner/config.json | 16 +++ custom/README.md | 41 ++++++- ...es-as-api-pages-for-external-agents.bad.al | 60 ++++++++++ ...s-as-api-pages-for-external-agents.good.al | 44 ++++++++ ...tities-as-api-pages-for-external-agents.md | 28 +++++ ...ope-mcp-server-tools-to-least-privilege.md | 28 +++++ ...k-instead-of-synchronous-wait-loops.bad.al | 37 +++++++ ...-instead-of-synchronous-wait-loops.good.al | 87 +++++++++++++++ ...-work-instead-of-synchronous-wait-loops.md | 32 ++++++ ...-messages-with-an-idempotency-check.bad.al | 33 ++++++ ...messages-with-an-idempotency-check.good.al | 67 +++++++++++ ...ound-messages-with-an-idempotency-check.md | 32 ++++++ ...ration-messages-manually-resolvable.bad.al | 36 ++++++ ...ation-messages-manually-resolvable.good.al | 75 +++++++++++++ ...ntegration-messages-manually-resolvable.md | 32 ++++++ ...external-event-subscription-health.good.al | 66 +++++++++++ ...itor-external-event-subscription-health.md | 28 +++++ ...call-external-services-from-posting.bad.al | 33 ++++++ ...all-external-services-from-posting.good.al | 73 ++++++++++++ ...ver-call-external-services-from-posting.md | 32 ++++++ ...k-long-running-work-on-a-status-url.bad.al | 32 ++++++ ...-long-running-work-on-a-status-url.good.al | 59 ++++++++++ .../park-long-running-work-on-a-status-url.md | 32 ++++++ ...events-over-handwritten-retry-loops.bad.al | 35 ++++++ ...vents-over-handwritten-retry-loops.good.al | 36 ++++++ ...ess-events-over-handwritten-retry-loops.md | 32 ++++++ ...e-a-correlation-id-across-every-hop.bad.al | 32 ++++++ ...-a-correlation-id-across-every-hop.good.al | 54 +++++++++ ...agate-a-correlation-id-across-every-hop.md | 32 ++++++ ...mpotency-key-on-every-outbound-call.bad.al | 37 +++++++ ...potency-key-on-every-outbound-call.good.al | 47 ++++++++ ...-idempotency-key-on-every-outbound-call.md | 32 ++++++ ...flows-into-staged-job-queue-entries.bad.al | 38 +++++++ ...lows-into-staged-job-queue-entries.good.al | 59 ++++++++++ ...tep-flows-into-staged-job-queue-entries.md | 32 ++++++ .../stage-every-integration-message.bad.al | 47 ++++++++ .../stage-every-integration-message.good.al | 99 +++++++++++++++++ .../stage-every-integration-message.md | 34 ++++++ ...-framing-record-for-inbound-polling.bad.al | 27 +++++ ...framing-record-for-inbound-polling.good.al | 78 +++++++++++++ ...se-a-framing-record-for-inbound-polling.md | 32 ++++++ ...ess-events-and-keep-payloads-stable.bad.al | 30 +++++ ...ss-events-and-keep-payloads-stable.good.al | 42 +++++++ ...usiness-events-and-keep-payloads-stable.md | 32 ++++++ ...t-the-al-runtime-during-a-debug-session.md | 28 +++++ ...he-limits-of-saas-point-in-time-restore.md | 28 +++++ ...ptimising-with-the-performance-profiler.md | 24 ++++ .../ground-work-in-a-solution-constitution.md | 29 +++++ .../map-each-feature-to-an-object-id-range.md | 29 +++++ .../process/specify-before-you-build.md | 29 +++++ .../integration/azure-integration-review.md | 73 ++++++++++++ .../azure-integration-validator.md | 97 ++++++++++++++++ .../skills/review/al-appsource-validator.md | 90 +++++++++++++++ .../skills/review/al-code-quality-reviewer.md | 102 +++++++++++++++++ .../review/al-event-subscriber-auditor.md | 87 +++++++++++++++ custom/skills/review/al-extended-review.md | 102 +++++++++++++++++ .../review/al-integration-pattern-reviewer.md | 104 ++++++++++++++++++ .../review/al-major-release-readiness.md | 88 +++++++++++++++ .../skills/review/al-multitenancy-reviewer.md | 58 ++++++++++ custom/skills/review/al-obsolete-tracker.md | 89 +++++++++++++++ .../skills/review/al-performance-reviewer.md | 100 +++++++++++++++++ .../review/al-permission-set-auditor.md | 86 +++++++++++++++ .../skills/review/al-readability-checker.md | 86 +++++++++++++++ custom/skills/review/al-table-refactorer.md | 89 +++++++++++++++ .../skills/review/al-translation-auditor.md | 81 ++++++++++++++ custom/skills/review/al-upgrade-checker.md | 92 ++++++++++++++++ .../testing/ai-test-driven-development.md | 90 +++++++++++++++ .../testing/al-test-coverage-enforcer.md | 91 +++++++++++++++ .../testing/al-test-coverage-validator.md | 91 +++++++++++++++ custom/skills/testing/al-test-runner.md | 87 +++++++++++++++ custom/skills/testing/al-test-validator.md | 87 +++++++++++++++ custom/skills/testing/al-test-writer.md | 79 +++++++++++++ .../testing/al-userguide-test-writer.md | 88 +++++++++++++++ .../skills/testing/bc-extension-test-guide.md | 93 ++++++++++++++++ custom/skills/testing/bc-webclient-runner.md | 93 ++++++++++++++++ custom/skills/testing/page-scripting-e2e.md | 79 +++++++++++++ 76 files changed, 4353 insertions(+), 6 deletions(-) create mode 100644 .altestrunner/config.json create mode 100644 custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.bad.al create mode 100644 custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.good.al create mode 100644 custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.md create mode 100644 custom/knowledge/api/scope-mcp-server-tools-to-least-privilege.md create mode 100644 custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.bad.al create mode 100644 custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.good.al create mode 100644 custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.md create mode 100644 custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.bad.al create mode 100644 custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.good.al create mode 100644 custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.md create mode 100644 custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.bad.al create mode 100644 custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.good.al create mode 100644 custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.md create mode 100644 custom/knowledge/integration/monitor-external-event-subscription-health.good.al create mode 100644 custom/knowledge/integration/monitor-external-event-subscription-health.md create mode 100644 custom/knowledge/integration/never-call-external-services-from-posting.bad.al create mode 100644 custom/knowledge/integration/never-call-external-services-from-posting.good.al create mode 100644 custom/knowledge/integration/never-call-external-services-from-posting.md create mode 100644 custom/knowledge/integration/park-long-running-work-on-a-status-url.bad.al create mode 100644 custom/knowledge/integration/park-long-running-work-on-a-status-url.good.al create mode 100644 custom/knowledge/integration/park-long-running-work-on-a-status-url.md create mode 100644 custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.bad.al create mode 100644 custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.good.al create mode 100644 custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.md create mode 100644 custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.bad.al create mode 100644 custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.good.al create mode 100644 custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.md create mode 100644 custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.bad.al create mode 100644 custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.good.al create mode 100644 custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.md create mode 100644 custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.bad.al create mode 100644 custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.good.al create mode 100644 custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.md create mode 100644 custom/knowledge/integration/stage-every-integration-message.bad.al create mode 100644 custom/knowledge/integration/stage-every-integration-message.good.al create mode 100644 custom/knowledge/integration/stage-every-integration-message.md create mode 100644 custom/knowledge/integration/use-a-framing-record-for-inbound-polling.bad.al create mode 100644 custom/knowledge/integration/use-a-framing-record-for-inbound-polling.good.al create mode 100644 custom/knowledge/integration/use-a-framing-record-for-inbound-polling.md create mode 100644 custom/knowledge/integration/version-business-events-and-keep-payloads-stable.bad.al create mode 100644 custom/knowledge/integration/version-business-events-and-keep-payloads-stable.good.al create mode 100644 custom/knowledge/integration/version-business-events-and-keep-payloads-stable.md create mode 100644 custom/knowledge/operations/inspect-the-al-runtime-during-a-debug-session.md create mode 100644 custom/knowledge/operations/know-the-limits-of-saas-point-in-time-restore.md create mode 100644 custom/knowledge/performance/profile-before-optimising-with-the-performance-profiler.md create mode 100644 custom/knowledge/process/ground-work-in-a-solution-constitution.md create mode 100644 custom/knowledge/process/map-each-feature-to-an-object-id-range.md create mode 100644 custom/knowledge/process/specify-before-you-build.md create mode 100644 custom/skills/integration/azure-integration-review.md create mode 100644 custom/skills/integration/azure-integration-validator.md create mode 100644 custom/skills/review/al-appsource-validator.md create mode 100644 custom/skills/review/al-code-quality-reviewer.md create mode 100644 custom/skills/review/al-event-subscriber-auditor.md create mode 100644 custom/skills/review/al-extended-review.md create mode 100644 custom/skills/review/al-integration-pattern-reviewer.md create mode 100644 custom/skills/review/al-major-release-readiness.md create mode 100644 custom/skills/review/al-multitenancy-reviewer.md create mode 100644 custom/skills/review/al-obsolete-tracker.md create mode 100644 custom/skills/review/al-performance-reviewer.md create mode 100644 custom/skills/review/al-permission-set-auditor.md create mode 100644 custom/skills/review/al-readability-checker.md create mode 100644 custom/skills/review/al-table-refactorer.md create mode 100644 custom/skills/review/al-translation-auditor.md create mode 100644 custom/skills/review/al-upgrade-checker.md create mode 100644 custom/skills/testing/ai-test-driven-development.md create mode 100644 custom/skills/testing/al-test-coverage-enforcer.md create mode 100644 custom/skills/testing/al-test-coverage-validator.md create mode 100644 custom/skills/testing/al-test-runner.md create mode 100644 custom/skills/testing/al-test-validator.md create mode 100644 custom/skills/testing/al-test-writer.md create mode 100644 custom/skills/testing/al-userguide-test-writer.md create mode 100644 custom/skills/testing/bc-extension-test-guide.md create mode 100644 custom/skills/testing/bc-webclient-runner.md create mode 100644 custom/skills/testing/page-scripting-e2e.md diff --git a/.altestrunner/config.json b/.altestrunner/config.json new file mode 100644 index 0000000..034f2f5 --- /dev/null +++ b/.altestrunner/config.json @@ -0,0 +1,16 @@ +{ + "containerResultPath": "", + "launchConfigName": "", + "securePassword": "", + "userName": "", + "companyName": "", + "testSuiteName": "", + "vmUserName": "", + "vmSecurePassword": "", + "remoteContainerName": "", + "dockerHost": "", + "newPSSessionOptions": "", + "testRunnerServiceUrl": "", + "codeCoveragePath": ".altestrunner\\codecoverage.json", + "culture": "en-US" +} \ No newline at end of file diff --git a/custom/README.md b/custom/README.md index 28d7aa9..d3e8c0a 100644 --- a/custom/README.md +++ b/custom/README.md @@ -1,17 +1,46 @@ # Custom layer -This folder is the template for partner- and customer-specific overrides. Use it to add knowledge and skills that apply to your organization but are not appropriate for the shared Microsoft or Community layers. +This folder holds partner- and customer-specific knowledge and action skills: content that applies to your organization but is not appropriate for the shared Microsoft or Community layers. It follows exactly the same formats as the other layers, so the consuming agent loads it automatically alongside `/microsoft/` and `/community/`. ## Structure ``` custom/ -├── knowledge/ # Your organization's knowledge files (same format as /microsoft/knowledge/) -└── skills/ # Your organization's action skills +├── knowledge/ # Knowledge files (same format as /microsoft/knowledge/) +└── skills/ # Action skills (Source -> Relevance -> Worklist -> Action -> Output) ``` +Knowledge files in `/custom/knowledge/` follow the frontmatter schema and section rules in [`/skills/read.md`](../skills/read.md) and [`/skills/write.md`](../skills/write.md). Action skills in `/custom/skills/` follow the contract in [`/skills/do.md`](../skills/do.md). + +## What is here + +This layer was seeded by migrating the Business Central AL assets from the `community-integration` project into BCQuality formats. + +### Knowledge (`custom/knowledge/`) + +| Domain | Articles | Covers | +|---|---|---| +| `integration` | 13 | The modern integration pattern catalog: staging through the Integration Message, inbound and outbound idempotency, polling framing records, Business Event versioning and payload safety, correlation propagation, long-running and staged flows, manual resolution, and the hard anti-patterns. | +| `copilot` | 4 | Registering a Copilot capability, calling Azure OpenAI through System.AI, billing type, and authoring a custom agent with the IAgent interfaces. | +| `ux` | 2 | PromptDialog for Copilot Generate UX, and keeping prompt text free of trailing whitespace. | +| `pipelines` | 4 | AL-Go for GitHub CI/CD, settings as the source of truth, environment registration, and headless builds with the AL MCP Server. | +| `security` | 3 | Least-privilege Azure RBAC, Managed Identity over connection strings, and locking environments to an Entra security group. | +| `process` | 3 | Spec-Driven Development: specify before you build, the solution constitution, and mapping features to object ID ranges. | +| `api` | 2 | Exposing BC entities as API pages for external agents, and least-privilege MCP tool surfaces. | +| `operations` | 2 | SaaS point-in-time restore limits, and inspecting the AL runtime during a debug session. | +| `performance` | 1 | Profiling before optimising with the built-in Performance Profiler. | +| `upgrade` | 1 | Gating major version bumps on compatibility testing. | + +Many integration, Copilot, and UX articles ship `.good.al` / `.bad.al` companion samples. + +### Skills (`custom/skills/`) + +| Folder | Skills | Notes | +|---|---|---| +| `review/` | 14 | Net-new AL reviewers and auditors: multi-tenancy, permission-set, event-subscriber, obsolescence, integration-pattern, upgrade, code-quality, readability, table-refactor, performance, translation, AppSource, and major-release-readiness. Plus `al-extended-review`, a super-skill that composes the six net-new domain reviewers so they dispatch as a group alongside the platform `al-code-review`. | +| `testing/` | 10 | The test agent suite (write, validate, run, coverage validate and enforce, user-guide tests, web-client run) plus the release-audit test-guide generator, Page Scripting e2e planning, and Copilot test-driven development. | +| `integration/` | 2 | Validating and reviewing the Azure integration plane (Functions, Service Bus, APIM, Bicep) that BC integrations depend on. | + ## How to use -Fork or clone BCQuality into your own repository and add your content here. Knowledge files in `/custom/knowledge/` follow the same frontmatter schema and section requirements as every other layer. Action skills in `/custom/skills/` follow the Action Skill template defined in `/skills/`. - -When agents consume BCQuality, the custom layer is loaded alongside Microsoft and Community — your overrides apply automatically. +Fork or clone BCQuality into your own repository and add your content here, or adapt the migrated content above. The reviewers cite the knowledge files by path, so a finding always points the author at the rule that backs it. When agents consume BCQuality, the custom layer is loaded alongside Microsoft and Community, and its higher layer precedence means your overrides win on conflict. diff --git a/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.bad.al b/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.bad.al new file mode 100644 index 0000000..9164607 --- /dev/null +++ b/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.bad.al @@ -0,0 +1,60 @@ +// Anti-pattern: an unstable, over-broad agent surface. The version is not a +// fixed contract, the entity is exposed through a part page the agent cannot +// see, and every write is left open. Demonstration-only. + +// Smell 1: a ListPart can never surface as an agent tool. Only top-level API +// pages are picked up, so this entity is silently unreachable. +page 50121 "Agent Customer Part" +{ + PageType = ListPart; + SourceTable = Customer; + + layout + { + area(Content) + { + repeater(Group) + { + field(no; Rec."No.") { } + field(name; Rec.Name) { } + } + } + } +} + +// Smell 2: a top-level API page that is over-broad and unstable. +page 50122 "Agent Vendor API" +{ + PageType = API; + SourceTable = Vendor; + APIPublisher = 'contoso'; + APIGroup = 'purchasing'; + // The version is bumped in place on each change instead of adding a new one, + // so every tool the agent discovered against v1.0 stops resolving. + APIVersion = 'v2.0'; + EntityName = 'vendor'; + EntitySetName = 'vendors'; + + // Nothing is locked down. A read-only agent workflow still gets create, + // modify, and delete tools, so a mistaken or prompt-injected agent can + // mutate or delete vendors it had no business touching. + // InsertAllowed / ModifyAllowed / DeleteAllowed left at permissive defaults. + + layout + { + area(Content) + { + repeater(Group) + { + // The entire table sprayed out, far beyond what the agent needs, + // widening the schema and the write blast radius for no reason. + field(no; Rec."No.") { } + field(name; Rec.Name) { } + field(blocked; Rec.Blocked) { } + field(balance; Rec."Balance (LCY)") { } + field(iban; Rec.IBAN) { } + field(paymentTerms; Rec."Payment Terms Code") { } + } + } + } +} diff --git a/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.good.al b/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.good.al new file mode 100644 index 0000000..e2696d4 --- /dev/null +++ b/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.good.al @@ -0,0 +1,44 @@ +// Best practice: a top-level API page whose entity names and version form a +// stable contract for an external agent, shaped read-only because the agent's +// workflow only needs to read. Demonstration-only; not a deployed object. + +page 50120 "Agent Customer API" +{ + // Only top-level PageType = API pages surface as agent tools. A ListPart or + // CardPart here would be silently unreachable by the agent. + PageType = API; + SourceTable = Customer; + + // These five properties are the contract. They are fixed literals, so the + // route and the tool names the agent binds to never shift under it. + APIPublisher = 'contoso'; + APIGroup = 'sales'; + APIVersion = 'v1.0'; + // EntityName and EntitySetName become the singular/plural tool names. They + // use the agent's domain vocabulary, not the table's internal captions. + EntityName = 'customer'; + EntitySetName = 'customers'; + + // The agent only reads, so the whole surface is locked to read. A read tool + // can never mutate, no matter what the prompt asks for. + Editable = false; + InsertAllowed = false; + ModifyAllowed = false; + DeleteAllowed = false; + + layout + { + area(Content) + { + repeater(Group) + { + // A narrow, deliberately chosen field set: exactly what the + // agent reasons over, named for the agent, nothing more. + field(number; Rec."No.") { Caption = 'number'; } + field(displayName; Rec.Name) { Caption = 'displayName'; } + field(city; Rec.City) { Caption = 'city'; } + field(balanceDue; Rec."Balance Due (LCY)") { Caption = 'balanceDue'; } + } + } + } +} diff --git a/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.md b/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.md new file mode 100644 index 0000000..14baa2c --- /dev/null +++ b/custom/knowledge/api/expose-bc-entities-as-api-pages-for-external-agents.md @@ -0,0 +1,28 @@ +--- +bc-version: [all] +domain: api +keywords: [api-page, external-agent, mcp, copilot-studio, entity, api-version, top-level-page] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Expose BC entities as API pages for external agents + +## Description + +An external agent (Copilot Studio, Claude, ChatGPT, or a custom agent) can only reach Business Central data and operations that are modelled as API pages. The agent never sees the table directly: it sees the API pages the tenant chooses to expose, each becoming a tool with a name derived from its entity and operations. Deciding which entities and which operations are reachable is therefore a design step, not an afterthought, because the published surface is simultaneously the agent's capability list and its blast radius. Stable entity names and explicit API versions matter because they are the contract the agent's tools are built on, and a rename or version bump silently breaks every tool the agent already discovered against the old names. + +The mechanism is the page's metadata. A top-level page with `PageType = API` and a fixed `APIPublisher`, `APIGroup`, and `APIVersion` is addressable at a stable route, and its `EntityName` and `EntitySetName` become the singular and plural tool names the agent binds to. Those five properties are the contract. The fields in the repeater are the schema the agent reasons over, so they should be named for the agent's domain vocabulary, not for the underlying table's field captions. + +## Best Practice + +For each entity an external agent must reach, define a top-level API page with a deliberate `EntityName`, `EntitySetName`, `APIPublisher`, `APIGroup`, and `APIVersion`, and treat those names and the version as a frozen contract: add a new `APIVersion` for breaking changes instead of mutating the existing one, so old tools keep resolving. Model only the operations the agent's workflow needs, setting `Editable = false` and `InsertAllowed`, `ModifyAllowed`, and `DeleteAllowed` to false when the agent only reads, so a read tool can never mutate. Keep the field set narrow and named in the agent's vocabulary so tool discovery is predictable. Choose entities and operations to match one specific agent audience rather than publishing a single broad surface for every possible client. See `expose-bc-entities-as-api-pages-for-external-agents.good.al` for a stable read-shaped API page and `expose-bc-entities-as-api-pages-for-external-agents.bad.al` for the unstable, over-broad form. + +## Anti Pattern + +Pointing an external agent at a `ListPart` or `CardPart` page, or at a non-API page, and expecting it to surface as a tool: only top-level API pages are picked up, so the entity is silently unreachable. The fix is a top-level API page wrapping the same source table. Other smells: renaming an exposed entity or bumping its `APIVersion` in place, which breaks the agent's existing tools; or exposing a sprawling set of entities and write operations the agent does not use. The detection signal: an external-agent integration that depends on a part-subtype API page, an API page whose `EntityName` or `APIVersion` is parameterised or computed rather than a fixed literal, or a read-only agent pointed at a page that leaves `ModifyAllowed` and `DeleteAllowed` at their permissive defaults. + +## See also + +- `scope-mcp-server-tools-to-least-privilege.md` diff --git a/custom/knowledge/api/scope-mcp-server-tools-to-least-privilege.md b/custom/knowledge/api/scope-mcp-server-tools-to-least-privilege.md new file mode 100644 index 0000000..f489b0f --- /dev/null +++ b/custom/knowledge/api/scope-mcp-server-tools-to-least-privilege.md @@ -0,0 +1,28 @@ +--- +bc-version: [all] +domain: api +keywords: [mcp, mcp-server, least-privilege, allow-create, unblock-edit-tools, read-only, configuration] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Scope MCP server tools to least privilege + +## Description + +Business Central's product MCP server exposes selected API pages as tools to outside AI clients. Each configuration controls which API pages an agent sees and what it may do with them: read, create, modify, delete, and bound actions. Because every operation runs as the signed-in user's identity and lands in the audit trail under that name, the tool surface is a privilege surface, and the agent inherits exactly the permission set of whoever signed it in. An agent can never do less than its configuration allows but never more than the user can do; the configuration is the ceiling and the user's permissions are the floor. + +The mechanism has two gates. A newly added page is read-only by default, and turning on any write requires both the configuration-level `Unblock Edit Tools` master switch and the specific per-page create, modify, or delete permission. Least privilege means leaving both gates shut except on the exact pages and operations the agent's workflow actually exercises, so the published tool surface is the smallest set that still lets the workflow succeed. + +## Best Practice + +Create one configuration per intended audience (for example a sales configuration and a warehouse configuration) rather than a single broad configuration shared by every client, so each audience's surface can be reasoned about and revoked on its own. Leave every API page read-only by default and enable create, modify, or delete one entity at a time, only when the agent's workflow requires it, setting both `Unblock Edit Tools` and the per-page permission deliberately rather than as a blanket flip. Document each configuration's audience and intended use, and review quarterly who has access and what is enabled, pruning any write that the workflow no longer exercises. Turn on Dynamic Tool Mode for any configuration that grows large so the surface stays within client tool caps without widening permissions. + +## Anti Pattern + +Building one mega-configuration that exposes many entities with write enabled "just in case", or flipping `Unblock Edit Tools` on at the configuration level with broad per-page create, modify, and delete permissions the agent never uses. Because the agent acts as the signed-in user, an over-broad surface lets a prompt-injected or mistaken agent modify or delete data it had no business touching, all under that user's identity in the audit log. The signal to look for: an MCP configuration with write operations enabled on pages the documented agent workflow does not require, or a single configuration serving multiple unrelated audiences. + +## See also + +- `expose-bc-entities-as-api-pages-for-external-agents.md` diff --git a/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.bad.al b/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.bad.al new file mode 100644 index 0000000..bee9210 --- /dev/null +++ b/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.bad.al @@ -0,0 +1,37 @@ +// Anti-pattern: the inbound handler BLOCKS the request thread, Sleep-polling an external +// service until it completes. The caller's connection is held open for the whole wait, +// and concurrent requests pile up on pinned threads. + +codeunit 50123 "Inbound Intake Bad" +{ + // Called on the request path. It does not return until the remote work is done, + // so its runtime is entirely dictated by a system BC does not control. + procedure Accept(Payload: Text): Text + var + Client: HttpClient; + Response: HttpResponseMessage; + JobId: Text; + Done: Boolean; + begin + JobId := StartRemoteJob(Client, Payload); + + // BAD: Sleep inside a loop on the request thread. This single request now holds + // its thread and session slot for the full duration of the remote job. + repeat + // When the downstream is SLOW: this blocks for seconds or minutes. The caller's + // HTTP connection times out long before the loop ends, and the work it kicked + // off is orphaned with no staged row recording it. + Sleep(2000); + + // When the downstream is DOWN: this Get blocks until its own timeout, every + // iteration, making a slow failure even slower. + Client.Get(StrSubstNo('https://svc.contoso.com/jobs/%1', JobId), Response); + Done := IsComplete(Response); + until Done; + + // Under load: each in-flight request pins a thread here. A handful of slow calls + // exhaust the request slots and BC starts rejecting healthy callers too. One slow + // dependency becomes a site-wide outage. + exit('completed'); // by now the original caller has almost certainly timed out + end; +} diff --git a/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.good.al b/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.good.al new file mode 100644 index 0000000..37f1cff --- /dev/null +++ b/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.good.al @@ -0,0 +1,87 @@ +// Best practice: accept the work, STAGE it, return 202 Accepted with a status URL, and +// free the request thread immediately. A background processor finishes the slow work; +// the caller polls the status URL and watches Status advance. No thread is ever pinned +// to a downstream system's latency. + +codeunit 50120 "Inbound Intake" +{ + // Called from the API insert trigger / webhook receiver. It returns the URL the HTTP + // layer puts in the Location header alongside a 202 Accepted. + procedure Accept(Payload: Text; ExternalRef: Text[100]) StatusUrl: Text + var + IntegrationMessage: Record "Integration Message"; + begin + IntegrationMessage.Init(); + IntegrationMessage."Message ID" := CreateGuid(); + IntegrationMessage.Direction := IntegrationMessage.Direction::Inbound; + IntegrationMessage."External Reference" := ExternalRef; + // New means "accepted, not yet processed". The background processor picks it up; + // the caller sees it move to In Progress, then Resolved or Failed. + IntegrationMessage.Status := IntegrationMessage.Status::New; + IntegrationMessage."Correlation ID" := CopyStr(DelChr(LowerCase(Format(CreateGuid())), '=', '{}'), 1, 40); + IntegrationMessage.SetRequest(Payload); + // The ONLY expensive thing on the request path is this Insert. The moment it + // returns, the request thread is free to serve the next caller. + IntegrationMessage.Insert(true); + + // Point the caller at the staged row. The HTTP layer maps this to + // 202 Accepted + a Location header; the caller polls it for completion. + exit(StrSubstNo('/api/contoso/integration/v1.0/integrationMessages(%1)', IntegrationMessage."Message ID")); + end; +} + +// Read-only status endpoint the caller polls. No blocking, no Sleep, no remote call: +// it just projects the current state of the staged row. +page 50121 "Integration Message Status" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'integration'; + APIVersion = 'v1.0'; + EntityName = 'integrationMessage'; + EntitySetName = 'integrationMessages'; + SourceTable = "Integration Message"; + Editable = false; // a status endpoint never mutates; it only reports + + layout + { + area(Content) + { + repeater(Group) + { + field(id; Rec."Message ID") { } + // The field the caller polls. New -> In Progress -> Resolved / Failed. + field(status; Rec.Status) { } + field(errorMessage; Rec."Error Message") { } // populated only on Failed + } + } + } +} + +codeunit 50122 "Inbound Processor" +{ + TableNo = "Job Queue Entry"; + + // Runs in the background, NOT on the request thread. This is where the slow work lives, + // so the caller's connection is never held open for it. + trigger OnRun() + var + IntegrationMessage: Record "Integration Message"; + begin + IntegrationMessage.SetRange(Direction, IntegrationMessage.Direction::Inbound); + IntegrationMessage.SetRange(Status, IntegrationMessage.Status::New); + if IntegrationMessage.FindSet() then + repeat + IntegrationMessage.Status := IntegrationMessage.Status::"In Progress"; + IntegrationMessage.Modify(true); + Commit(); // make In Progress visible to a polling caller at once + Process(IntegrationMessage); // the slow part: runs here, off the request path + until IntegrationMessage.Next() = 0; + end; + + local procedure Process(var IntegrationMessage: Record "Integration Message") + begin + // ... do the real work; on success set Status::Resolved and store the response, + // on failure set Status::Failed and stamp Error Message. The caller's next poll sees it. + end; +} diff --git a/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.md b/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.md new file mode 100644 index 0000000..2e64a98 --- /dev/null +++ b/custom/knowledge/integration/accept-async-work-instead-of-synchronous-wait-loops.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [async, sleep, polling, http-202, status-url, api-handler, request-thread] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Accept async work instead of synchronous wait loops + +## Description + +An inbound API handler or webhook receiver that kicks off external work and then `Sleep`-and-polls until it finishes holds the request thread for the entire wait, and with it the session slot and any locks the handler has taken. The handler is now blocked on work it does not own and cannot speed up. Web requests into Business Central have a finite server-side budget, so a wait measured in seconds or minutes does not produce a slow-but-correct answer: the connection times out, the caller gets an error, and the work it triggered is orphaned with no record that it ever started. + +The damage compounds under load and under exactly the external conditions you cannot control. When the downstream system is slow, each in-flight request pins a thread, so a handful of slow calls exhaust the available request slots and healthy callers start getting rejected too: one slow dependency becomes a site-wide outage. When the downstream system is down, every request blocks for the full timeout before failing, turning a fast failure into a slow one and multiplying the thread pressure. The handler must never block on work it does not own. Accept the request, persist it, answer immediately, and let the caller check back. + +## Best Practice + +Split acceptance from completion. Stage the request as an Integration Message, return `202 Accepted` with a status URL that points at that staged row, and let a background processor do the slow work. The request thread is freed the instant the row is written, so throughput is bounded by how fast you can insert rows, not by how slow the downstream system is. The mechanism that makes the caller whole is the status URL plus the Status field: the caller polls a read-only API page over the Integration Message keyed by its Message ID and watches Status move from New to In Progress to Resolved or Failed, reading the final response from the same row. This applies to any inbound path where completion is not guaranteed to be immediate. See `accept-async-work-instead-of-synchronous-wait-loops.good.al`. + +The trade-off is that the caller must be willing to poll (or accept a callback), which is a contract you state up front with the 202 and the Location header. For work that genuinely answers in the same call, a synchronous response is fine; reach for staging the moment completion depends on a system you do not control. + +## Anti Pattern + +An inbound handler that contains a `Sleep` inside a `repeat ... until` or `while` loop that re-queries an external service for completion before returning. The detection signal: `Sleep(` together with a loop and an `HttpClient` call inside an API page trigger, a webhook codeunit, or any procedure on the request path; equivalently, a handler whose return value depends on a remote status it polls in-line. The consequence is that the request blocks for the full duration of external work, the caller's connection times out, and concurrent requests pile up on pinned threads until the service stops accepting new ones. The fix is to stage the request and return 202 with a status URL. See `accept-async-work-instead-of-synchronous-wait-loops.bad.al`. + +## See also + +- `park-long-running-work-on-a-status-url.md` +- `stage-every-integration-message.md` +- `propagate-a-correlation-id-across-every-hop.md` diff --git a/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.bad.al b/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.bad.al new file mode 100644 index 0000000..2bdd5d5 --- /dev/null +++ b/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.bad.al @@ -0,0 +1,33 @@ +// Anti-pattern: insert on every call with no idempotency check, keyed on a brand-new GUID. +// A retried or re-fetched delivery is processed again as a fresh message, so a duplicating +// source produces duplicate documents and double-applied side effects. + +codeunit 50142 "Inbound Dedup Bad" +{ + procedure Stage(ExternalRef: Text[100]; MsgType: Code[40]; Payload: Text): Guid + var + IntegrationMessage: Record "Integration Message"; + begin + // BAD: no SetRange on External Reference + Type, no Get, no lookup of any kind. + // The source system's stable id is captured on the row but never used to detect a repeat. + IntegrationMessage.Init(); + + // BAD: the only "identity" is a fresh GUID. If anyone later "dedups" on Message ID, + // it can never match, because every insert mints a new one. This is the illusion of a + // dedup key that can never actually fire. + IntegrationMessage."Message ID" := CreateGuid(); + + IntegrationMessage.Direction := IntegrationMessage.Direction::Inbound; + IntegrationMessage."External Reference" := ExternalRef; + IntegrationMessage.Type := MsgType; + IntegrationMessage.Status := IntegrationMessage.Status::New; + IntegrationMessage.SetRequest(Payload); + + // When the source RETRIES (a webhook that did not see our ack, a restart re-send, an + // overlapping poll window): this runs again with the same ExternalRef and stages a + // second message. Downstream it becomes a second sales order and a second posting. + // The duplicate volume scales with how aggressively the source retries. + IntegrationMessage.Insert(true); + exit(IntegrationMessage."Message ID"); + end; +} diff --git a/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.good.al b/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.good.al new file mode 100644 index 0000000..0960681 --- /dev/null +++ b/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.good.al @@ -0,0 +1,67 @@ +// Best practice: deduplicate on the source system's stable id (External Reference + Type) +// BEFORE staging, backed by a unique key. A replay returns the prior result instead of +// being processed again. The internal Message ID is never the dedup key, because it is +// freshly generated per insert and so could never match a repeat. + +table 50140 "Integration Message" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Message ID"; Guid) { Caption = 'Message ID'; } + field(2; Direction; Enum "Integration Direction") { Caption = 'Direction'; } + field(3; "Type"; Code[40]) { Caption = 'Type'; } + field(4; Status; Enum "Integration Status") { Caption = 'Status'; } + // The source-controlled stable id. This, not Message ID, is what dedup keys on. + field(5; "External Reference"; Text[100]) { Caption = 'External Reference'; } + } + + keys + { + key(PK; "Message ID") { Clustered = true; } + // UNIQUE idempotency key: a second concurrent insert of the same delivery fails at + // the database, so dedup holds even under a race, not only on the explicit lookup. + key(Idempotency; "External Reference", "Type") { Unique = true; } + } +} + +codeunit 50141 "Inbound Dedup" +{ + procedure Stage(ExternalRef: Text[100]; MsgType: Code[40]; Payload: Text): Guid + var + Existing: Record "Integration Message"; + IntegrationMessage: Record "Integration Message"; + begin + // The idempotency lookup: a single indexed read on the unique key. + Existing.SetRange("External Reference", ExternalRef); + Existing.SetRange(Type, MsgType); + if Existing.FindFirst() then begin + case Existing.Status of + Existing.Status::Resolved: + // Already processed. Return the prior result; do NOT do the work again. + exit(Existing."Message ID"); + Existing.Status::"In Progress": + // A run is already handling this exact external reference. Reject the + // second one rather than process the same message concurrently. + Error('Message %1 of type %2 is already in progress', ExternalRef, MsgType); + end; + // Any other prior state (for example Failed): return the existing row so the + // resolution flow handles it, instead of minting a duplicate. + exit(Existing."Message ID"); + end; + + // No prior message exists: stage a genuinely new one. + IntegrationMessage.Init(); + IntegrationMessage."Message ID" := CreateGuid(); // internal id, never the dedup key + IntegrationMessage.Direction := IntegrationMessage.Direction::Inbound; + IntegrationMessage."External Reference" := ExternalRef; + IntegrationMessage.Type := MsgType; + IntegrationMessage.Status := IntegrationMessage.Status::New; + IntegrationMessage.SetRequest(Payload); + // If a concurrent request slipped past the lookup, the unique key makes THIS Insert + // fail rather than create a duplicate. Either way, the side effect runs at most once. + IntegrationMessage.Insert(true); + exit(IntegrationMessage."Message ID"); + end; +} diff --git a/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.md b/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.md new file mode 100644 index 0000000..3ffc3c6 --- /dev/null +++ b/custom/knowledge/integration/deduplicate-inbound-messages-with-an-idempotency-check.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [idempotency, deduplication, external-reference, inbound, replay, in-progress, unique-key] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Deduplicate inbound messages with an idempotency check + +## Description + +Duplicate inbound messages are a guarantee, not an edge case. A source system re-fetches and resends after a restart, a webhook platform fires a retry because it did not see your acknowledgement in time, a poll window overlaps a previous one, a load balancer replays a request. Every one of these delivers a message you have already seen, and the source genuinely believes it is doing the right thing by retrying. The receiver, not the sender, is responsible for recognising the repeat, because only the receiver knows what it has already processed. + +The mechanism that makes recognition possible is the source system's own stable identifier. Before staging an inbound message, look it up by that identifier plus the message type. If a matching message is already Resolved, return its stored response and do nothing else, because the work is already done. If a matching message is In Progress, wait or reject rather than start a second concurrent run against the same external reference. Only when there is no match do you stage a new message and process it. Skip this check and a slow or duplicating external system turns every replay into real work: duplicate sales orders, double postings, duplicate outbound side effects that ripple to yet more systems. + +## Best Practice + +Deduplicate on `External Reference + Type`, the stable id the source system controls, and back it with a unique key on `(External Reference, Type)` so the lookup is a single indexed read and a concurrent duplicate insert fails at the database rather than racing through. Never deduplicate on the internal Message ID: that GUID is generated fresh on every insert, so it never matches a replay and gives you the illusion of a dedup check that can never fire. On a Resolved hit return the stored response so the caller sees the same answer it would have seen the first time; on an In Progress hit reject or back off so two runs do not process the same external reference at once; only on no hit do you insert and process. See `deduplicate-inbound-messages-with-an-idempotency-check.good.al`. + +The trade-off is one indexed read on the ingest path, which is cheap, and a unique key that will reject a genuine duplicate insert, which is the point. Pair this with outbound idempotency keys (see `send-an-idempotency-key-on-every-outbound-call.md`) so the same flow is protected against duplicates on the way out as well as on the way in. + +## Anti Pattern + +An inbound handler that inserts a new Integration Message on every call without first checking for an existing one, or that deduplicates on the internal Message ID instead of the source's External Reference. The detection signal: an `Insert` of an inbound message with no prior `SetRange`/`Get` on `External Reference` and `Type`, a `CreateGuid()` used as the dedup key, or a dedup lookup keyed on `Message ID`. The consequence is that a retried or re-fetched delivery is processed as a brand-new message, so a duplicating source produces duplicate documents and double-applied side effects, and the volume scales with how aggressively the source retries. The fix is a lookup on `External Reference + Type` before any insert, backed by a unique key. See `deduplicate-inbound-messages-with-an-idempotency-check.bad.al`. + +## See also + +- `send-an-idempotency-key-on-every-outbound-call.md` +- `use-a-framing-record-for-inbound-polling.md` +- `stage-every-integration-message.md` diff --git a/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.bad.al b/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.bad.al new file mode 100644 index 0000000..931d52d --- /dev/null +++ b/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.bad.al @@ -0,0 +1,36 @@ +// Anti-pattern: failed messages are PURGED, and the only "retry" mints a NEW message. The +// payload and error context needed to diagnose the failure are destroyed, and the new id breaks +// idempotency so the receiver double-applies the side effect. + +codeunit 50221 "Failed Cleanup Bad" +{ + procedure PurgeFailed() + var + IntegrationMessage: Record "Integration Message"; + begin + // BAD: deleting failed rows means a fix needs a code change and a redeploy, because there + // is no editable row for ops to correct and re-run. Every data-level failure becomes an + // engineering incident. Worse, the payload and the error that explain WHAT failed are gone, + // so diagnosis after the fact is impossible. + IntegrationMessage.SetRange(Status, IntegrationMessage.Status::Failed); + IntegrationMessage.DeleteAll(true); + end; + + procedure RetryFailed(SourceRef: Text[100]; MsgType: Code[40]; Payload: Text) + var + NewMessage: Record "Integration Message"; + begin + // BAD: a manual retry that creates a BRAND-NEW message with a fresh Message ID. The + // idempotency key is derived from the Message ID, so a new id means a new key, and the + // receiver sees this as a new request rather than a repeat of the failed one. If the + // original attempt had partly landed (a charge captured, a shipment booked), this retry + // applies the side effect a SECOND time. The correct fix is to re-run the EXISTING message. + NewMessage.Init(); + NewMessage."Message ID" := CreateGuid(); + NewMessage."External Reference" := SourceRef; + NewMessage.Type := MsgType; + NewMessage.Status := NewMessage.Status::New; + NewMessage.SetRequest(Payload); + NewMessage.Insert(true); + end; +} diff --git a/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.good.al b/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.good.al new file mode 100644 index 0000000..b0d2b3f --- /dev/null +++ b/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.good.al @@ -0,0 +1,75 @@ +// Best practice: a resolution page over failed messages. Ops corrects the payload and flips +// Status to New; the processor re-runs the SAME Message ID under the SAME idempotency key, so +// the fix reprocesses without creating a duplicate. The shape mirrors what Microsoft ships for +// E-Document: editable staging, a resolution page, a status enum, retry actions, and an audit trail. + +page 50220 "Integration Resolution" +{ + PageType = List; + SourceTable = "Integration Message"; + // Failed messages stay EDITABLE so ops can correct the payload without a developer or a deploy. + Editable = true; + SourceTableView = where(Status = const(Failed)); + Caption = 'Integration Resolution'; + + layout + { + area(Content) + { + repeater(Group) + { + // The id is shown but NOT editable: re-running must reuse it so the idempotency + // key (derived from this id) stays the same and the retry cannot double-apply. + field("Message ID"; Rec."Message ID") { Editable = false; } + field("External Reference"; Rec."External Reference") { Editable = false; } + // The error context ops needs to diagnose the failure. Read-only: it is history. + field("Error Message"; Rec."Error Message") { Editable = false; } + field("Retry Count"; Rec."Retry Count") { Editable = false; } + // The payload ops actually edits to fix a malformed or mis-mapped message. + field(Request; Rec.GetRequest()) { } + field("Resolution Note"; Rec."Resolution Note") { } // audit of what was decided + } + } + } + + actions + { + area(Processing) + { + action(Resolve) + { + Caption = 'Resolve'; + trigger OnAction() + begin + // Re-run the SAME message: same Message ID, therefore same idempotency key. + // The processor reprocesses the corrected payload, and the receiver recognises + // the repeat, so a side effect that partly landed is not applied a second time. + Rec.Status := Rec.Status::New; + Rec.Modify(true); + end; + } + action(ConfirmByException) + { + Caption = 'Confirm by Exception'; + trigger OnAction() + begin + // Accept as handled with NO retry (for example the work was completed manually + // out of band). The audit record is kept so the decision is traceable. + Rec.Status := Rec.Status::Resolved; + Rec."Resolution Note" := 'Confirmed by exception'; + Rec.Modify(true); + end; + } + action(Reassign) + { + Caption = 'Reassign'; + trigger OnAction() + begin + // Route to another handler/queue without losing the message or its history. + Rec."Assigned To" := PickHandler(); + Rec.Modify(true); + end; + } + } + } +} diff --git a/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.md b/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.md new file mode 100644 index 0000000..57ff206 --- /dev/null +++ b/custom/knowledge/integration/make-failed-integration-messages-manually-resolvable.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [manual-resolution, failed-message, resolution-page, edocument, confirm-by-exception, ops, audit] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Make failed integration messages manually resolvable + +## Description + +Automation cannot fix every failure. A malformed payload from a source that changed its format, a mapping gap for a product that was set up wrong, a one-off data problem on a single document: these are not transient and no amount of retrying resolves them, because the data itself is the problem. When automation cannot recover, a human has to be able to step in, and the design must let them do it without a developer and a deployment. If the only way to fix a stuck message is to change code and ship a release, then every data-level failure becomes an engineering incident, and the backlog of stuck messages grows while it waits for the next deployment window. + +Manual resolution is therefore a first-class part of the integration design, not an afterthought bolted on once something breaks. Failed Integration Messages must stay editable so operations can correct the payload and re-run the same message, and the re-run has to preserve identity so it does not undo the very guarantees the happy path relied on. The shape worth copying is the one Microsoft already ships for E-Document: inbound staging, a resolution page, a status enum, retry actions, and an audit trail, so the experience is familiar to anyone who has resolved an electronic document and the audit story is already understood. + +## Best Practice + +Keep Failed messages editable. Operations corrects the payload on the row and flips Status back to New, and the processor re-runs the same Message ID under the same idempotency key, so the correction reprocesses without creating a duplicate and without double-applying a side effect that may already have partly landed. Provide a resolution page exposing the payload and the error, with three actions: Resolve (re-run the same message after a fix), Confirm-by-Exception (accept the message as handled with no retry, keeping the audit record so the decision is traceable), and Reassign (route the message to another handler or queue). The mechanism that makes re-run safe is reusing the existing Message ID rather than minting a new one, because the idempotency key is derived from that id (see `send-an-idempotency-key-on-every-outbound-call.md`), so a human-driven retry is as safe against duplicates as an automated one. Mirror the E-Document shape so the experience and the audit trail are familiar. See `make-failed-integration-messages-manually-resolvable.good.al`. + +The trade-off is keeping failed rows around and editable rather than purging them, which costs storage and demands a resolution UI, in exchange for a system where a data problem is an operations task rather than an engineering deployment. + +## Anti Pattern + +Failed messages that are read-only or auto-deleted, so a fix means a code change and redeploy, or a manual retry that mints a new Message ID and so loses the idempotency guarantee. The detection signal: a Failed status with no editable payload and no resolution page, a purge or cleanup job that `DeleteAll`s failed rows, or a manual retry path that calls `CreateGuid()` to create a fresh message instead of re-running the existing one. The consequences are that every data failure becomes a deployment (read-only or deleted rows), the payload and error context needed to diagnose it are gone (deletion), or the retry double-applies the side effect because the receiver sees a new request rather than a repeat (new Message ID). The fix is editable failed rows, a resolution page with retry actions, and a re-run that reuses the same Message ID. See `make-failed-integration-messages-manually-resolvable.bad.al`. + +## See also + +- `deduplicate-inbound-messages-with-an-idempotency-check.md` +- `send-an-idempotency-key-on-every-outbound-call.md` +- `stage-every-integration-message.md` diff --git a/custom/knowledge/integration/monitor-external-event-subscription-health.good.al b/custom/knowledge/integration/monitor-external-event-subscription-health.good.al new file mode 100644 index 0000000..3ac0327 --- /dev/null +++ b/custom/knowledge/integration/monitor-external-event-subscription-health.good.al @@ -0,0 +1,66 @@ +// Best practice: a scheduled monitor lists the LIVE external event subscriptions and diffs +// them against the EXPECTED set. Any expected subscription that BC has silently dropped +// (because the subscriber returned a non-408/429/5xx response) raises an alert and telemetry. + +table 50180 "Expected Event Subscription" +{ + DataClassification = SystemMetadata; + + fields + { + // The expected set lives in configuration, so registering an integration also registers + // its monitoring expectation. The two cannot drift apart. + field(1; "Event Name"; Text[100]) { Caption = 'Event Name'; } + field(2; "Notification URL"; Text[250]) { Caption = 'Notification URL'; } + } + + keys { key(PK; "Event Name", "Notification URL") { Clustered = true; } } +} + +codeunit 50181 "Subscription Health Monitor" +{ + TableNo = "Job Queue Entry"; + + // Runs as a Job Queue entry on a schedule (for example hourly). It must run actively: + // a dropped subscription is indistinguishable from a quiet feed, so silence cannot be trusted. + trigger OnRun() + begin + CheckHealth(); + end; + + procedure CheckHealth() + var + Expected: Record "Expected Event Subscription"; + Live: List of [Text]; + begin + // GET api/microsoft/runtime/v1.0/externaleventsubscriptions and project each live + // subscription to a comparable key. + Live := FetchLiveSubscriptions(); + + if Expected.FindSet() then + repeat + // The diff: an expected subscription missing from the live list was dropped by + // the platform with no notification. That is an incident, not a warning. + if not Live.Contains(SubscriptionKey(Expected."Event Name", Expected."Notification URL")) then + RaiseMissingSubscriptionAlert(Expected); + until Expected.Next() = 0; + end; + + local procedure RaiseMissingSubscriptionAlert(Expected: Record "Expected Event Subscription") + var + Dimensions: Dictionary of [Text, Text]; + begin + // Telemetry carries the event name and URL so operations can re-register it AND can read, + // from the telemetry timeline, roughly when delivery stopped. + Dimensions.Add('eventName', Expected."Event Name"); + Dimensions.Add('notificationUrl', Expected."Notification URL"); + Session.LogMessage('INT0001', 'External event subscription missing', Verbosity::Warning, + DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher, Dimensions); + // ... and raise an operational alert (email, Teams, ticket) so a human acts before the gap grows. + end; + + local procedure SubscriptionKey(EventName: Text; NotificationUrl: Text): Text + begin + exit(StrSubstNo('%1|%2', EventName, NotificationUrl)); + end; +} diff --git a/custom/knowledge/integration/monitor-external-event-subscription-health.md b/custom/knowledge/integration/monitor-external-event-subscription-health.md new file mode 100644 index 0000000..7288da1 --- /dev/null +++ b/custom/knowledge/integration/monitor-external-event-subscription-health.md @@ -0,0 +1,28 @@ +--- +bc-version: [all] +domain: integration +keywords: [subscription-health, monitor, external-business-event, silent-drop, alert, job-queue, telemetry] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Monitor external event subscription health + +## Description + +Business Central removes an external business event subscription when the subscriber's notification endpoint returns anything other than 408, 429, or a 5xx response. A 404 because the consumer redeployed to a new URL, a 401 because a token expired, a 400 because a proxy mangled the request: any of these tells the platform the endpoint is permanently unable to accept the notification, so it stops trying and drops the subscription. This is reasonable platform behaviour, but there is no built-in alert when it happens. The subscription simply disappears and notifications stop flowing. + +The reason this is dangerous is that a dropped subscription is indistinguishable from a quiet feed. If nothing has happened to raise the event lately, no notifications would arrive anyway, so the absence of traffic looks normal. The gap is typically discovered only when someone downstream asks why they stopped receiving events, by which point the integration has been silently broken for hours or days and there may be a backlog of business activity that was never communicated. Because the platform will not tell you, the only way to catch a drop is to check for it actively and on a schedule. + +## Best Practice + +Run a monitor job on a schedule (a Job Queue entry, for example hourly) that lists the current external event subscriptions from the `externaleventsubscriptions` endpoint and compares them against the set the integration expects to exist. Keep the expected set in a small configuration table so that registering an integration also registers its monitoring expectation, and the two never drift apart. The mechanism is the diff: for every expected subscription that is absent from the live list, raise an operational alert and emit telemetry carrying the event name and notification URL, so operations can re-register it before the gap grows and can see, from the telemetry timeline, roughly when delivery stopped. Treat a missing subscription as an incident, not a warning to be filtered out. See `monitor-external-event-subscription-health.good.al`. + +The trade-off is one scheduled read of the subscription list per interval plus a small table of expectations, which is a negligible cost against the alternative of a multi-day silent outage discovered by a downstream complaint. + +## See also + +- `prefer-business-events-over-handwritten-retry-loops.md` +- `version-business-events-and-keep-payloads-stable.md` +- `propagate-a-correlation-id-across-every-hop.md` diff --git a/custom/knowledge/integration/never-call-external-services-from-posting.bad.al b/custom/knowledge/integration/never-call-external-services-from-posting.bad.al new file mode 100644 index 0000000..14946ea --- /dev/null +++ b/custom/knowledge/integration/never-call-external-services-from-posting.bad.al @@ -0,0 +1,33 @@ +// Anti-pattern: the posting subscriber calls an external service INLINE, inside the +// posting transaction. The post now holds document and ledger locks until the remote +// endpoint answers, and a remote failure rolls the whole post back. + +codeunit 50112 "Post Shipment Notifier Bad" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Sales-Post", 'OnAfterPostSalesDoc', '', false, false)] + local procedure NotifyShipmentInline(var SalesHeader: Record "Sales Header") + var + Client: HttpClient; + Content: HttpContent; + Response: HttpResponseMessage; + begin + Content.WriteFrom(BuildShipmentJson(SalesHeader)); + + // BAD: HttpClient.Post runs INSIDE the posting transaction. The locks taken by + // Sales-Post on the header, the lines, and the related ledger entries stay held + // for the entire round trip to the WMS. + // + // When the WMS is SLOW: every other user posting a sales document queues behind + // these locks. One slow endpoint serialises the whole team's posting. + // + // When the WMS is DOWN: this call blocks until the HTTP timeout fires, then throws. + // The throw propagates out of the posting transaction and the entire post ROLLS BACK. + // The shipment physically left the warehouse, but there is now no posted document + // and nothing was staged, so there is nothing to retry and nothing to inspect. + Client.Post('https://wms.contoso.com/api/shipments', Content, Response); + + // Even on a success that is not actually success: a network blip after the WMS + // committed but before BC saw the response leaves the two systems disagreeing, + // with no staged row recording that the notification was attempted. + end; +} diff --git a/custom/knowledge/integration/never-call-external-services-from-posting.good.al b/custom/knowledge/integration/never-call-external-services-from-posting.good.al new file mode 100644 index 0000000..c398cfd --- /dev/null +++ b/custom/knowledge/integration/never-call-external-services-from-posting.good.al @@ -0,0 +1,73 @@ +// Best practice: the posting subscriber only STAGES an outbound message and returns. +// The HttpClient.Send happens later, in a Job Queue codeunit, outside the posting lock. +// The row is inserted inside the posting transaction, so it exists only if the post +// committed; the callout runs in a separate transaction where a remote outage can only +// delay delivery, never roll back a posted shipment. + +codeunit 50110 "Post Shipment Notifier" +{ + // Subscriber on the real posting publisher. It runs while posting locks are held, + // so it must do nothing that can block: no HTTP, no second remote call, just an Insert. + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Sales-Post", 'OnAfterPostSalesDoc', '', false, false)] + local procedure StageShipmentNotification(var SalesHeader: Record "Sales Header") + var + IntegrationMessage: Record "Integration Message"; + begin + // Only notify on an actual posted shipment, not on every posted document kind. + if not SalesHeader.Ship then + exit; + + IntegrationMessage.Init(); + IntegrationMessage."Message ID" := CreateGuid(); + // Direction Outbound + Status New is exactly what the Job Queue processor queries for. + IntegrationMessage.Direction := IntegrationMessage.Direction::Outbound; + IntegrationMessage.Status := IntegrationMessage.Status::New; + IntegrationMessage.Type := 'SHIPMENT-NOTIFY'; + // Carry the document ANCHOR, not a live handle. The processor re-reads detail later. + IntegrationMessage."Document No." := SalesHeader."No."; + IntegrationMessage."External Reference" := SalesHeader."External Document No."; + // Correlation id threads this notification to the rest of the flow's log lines. + IntegrationMessage."Correlation ID" := CopyStr(DelChr(LowerCase(Format(CreateGuid())), '=', '{}'), 1, 40); + // Insert participates in the posting transaction: the row lives only if the post commits, + // and rolls back cleanly with the post if posting fails. No remote system is touched here. + IntegrationMessage.Insert(true); + end; +} + +codeunit 50111 "Outbound Sender" +{ + // Runs as a Job Queue entry, well after posting has committed and released its locks. + // Nothing it does can lengthen a posting lock window, because there is no longer a post in flight. + procedure SendNew() + var + IntegrationMessage: Record "Integration Message"; + begin + IntegrationMessage.SetRange(Direction, IntegrationMessage.Direction::Outbound); + IntegrationMessage.SetRange(Status, IntegrationMessage.Status::New); + if IntegrationMessage.FindSet() then + repeat + // Each row is its own short unit of work. A slow endpoint stalls delivery + // of THIS message only; it cannot stall anyone's posting. + SendOne(IntegrationMessage); + until IntegrationMessage.Next() = 0; + end; + + local procedure SendOne(var IntegrationMessage: Record "Integration Message") + var + Client: HttpClient; + Content: HttpContent; + Response: HttpResponseMessage; + begin + Content.WriteFrom(BuildShipmentJson(IntegrationMessage)); + if Client.Post(GetEndpoint(IntegrationMessage), Content, Response) and Response.IsSuccessStatusCode() then begin + IntegrationMessage.Status := IntegrationMessage.Status::Resolved; + IntegrationMessage.Modify(true); + end else begin + // A failure here is recorded on the row and retried later. The posted shipment + // is already durable, so a WMS outage never costs us the posting. + IntegrationMessage."Retry Count" += 1; + IntegrationMessage."Error Message" := CopyStr(GetLastErrorText(), 1, 2048); + IntegrationMessage.Modify(true); + end; + end; +} diff --git a/custom/knowledge/integration/never-call-external-services-from-posting.md b/custom/knowledge/integration/never-call-external-services-from-posting.md new file mode 100644 index 0000000..83df999 --- /dev/null +++ b/custom/knowledge/integration/never-call-external-services-from-posting.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [posting, httpclient, callout, job-queue, locks, subscriber, rollback] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Never call external services from posting + +## Description + +A posting routine runs as one database transaction and holds write locks on the document header, the document lines, and every ledger and entry table it touches until that transaction commits. Calling an external service from inside that routine, or from a posting subscriber such as `OnAfterPostSalesDoc`, `OnAfterPostPurchaseDoc`, or `OnAfterFinalizePosting`, binds the lifetime of those locks to the response time of a system Business Central does not control. The remote endpoint, not BC, now decides how long the locks are held. + +The failure mode is concrete and it gets worse under exactly the conditions you cannot prevent. When the external system is slow, the posting transaction stays open and every other user who needs those records waits behind it, so one sluggish endpoint serialises an entire team's posting. When the external system is down, the call blocks until the HTTP timeout fires and then throws, and because the throw happens inside the posting transaction the whole post rolls back: the shipment that physically left the warehouse now has no posted document, and nothing was staged to retry. When the external system is healthy but the network blips, you get an uncertain failure on a transaction that may already have committed downstream. The remote call has to leave the posting transaction entirely. + +## Best Practice + +Stage the outbound work instead of sending it inline. From the posting subscriber, write one Integration Message row (Direction Outbound, Status New) that carries the document anchor (for example the posted document number) and whatever payload the receiver needs, then return immediately so posting commits on local state alone. A background Job Queue codeunit reads the staged rows by Status and performs the actual `HttpClient.Send` outside any posting lock. The mechanism that makes this safe is the commit boundary: the row is inserted in the posting transaction, so it exists only if the post succeeded, and the callout runs in a separate later transaction where a remote outage delays delivery without ever touching the posting locks or the posted document. See `never-call-external-services-from-posting.good.al`. + +This applies to every posting and posting-adjacent path, inbound or outbound. The one nuance worth knowing: firing an `[ExternalBusinessEvent]` from a posting subscriber is not a violation, because that is not an HTTP call and the platform delivers it post-commit (see `prefer-business-events-over-handwritten-retry-loops.md`). The trade-off of staging is added latency and one more table, which is the point: you are trading immediacy for a posting path that cannot be held hostage. + +## Anti Pattern + +A posting routine or a posting-event subscriber that calls `HttpClient.Send` directly, or that invokes a client codeunit which does. The detection signal a reviewer or agent can match: an `HttpClient`, `HttpRequestMessage`, `HttpContent`, or REST/JSON client reference inside a `Codeunit "*-Post"`, or inside a subscriber bound to `OnAfterPostSalesDoc`, `OnAfterPostPurchaseDoc`, `OnAfterFinalizePosting`, `OnBeforePost*`, or any publisher on a posting codeunit. The consequence is that posting locks are now held for the full remote round trip, and a remote failure rolls back a post that should have been durable. The fix is structural: move the callout into a Job Queue processor that reads staged rows. See `never-call-external-services-from-posting.bad.al`. + +## See also + +- `stage-every-integration-message.md` +- `prefer-business-events-over-handwritten-retry-loops.md` +- `propagate-a-correlation-id-across-every-hop.md` diff --git a/custom/knowledge/integration/park-long-running-work-on-a-status-url.bad.al b/custom/knowledge/integration/park-long-running-work-on-a-status-url.bad.al new file mode 100644 index 0000000..d25d165 --- /dev/null +++ b/custom/knowledge/integration/park-long-running-work-on-a-status-url.bad.al @@ -0,0 +1,32 @@ +// Anti-pattern: a Job Queue tight loop that Sleep-polls the status URL, with the retry count in +// a local variable that resets on restart. The loop pins a Job Queue slot for the entire +// external wait, and the counter never survives long enough to drive real backoff or give-up. + +codeunit 50202 "Long Running Bad" +{ + procedure Start(var IntegrationMessage: Record "Integration Message") + var + Client: HttpClient; + Response: HttpResponseMessage; + Location: array[1] of Text; + RetryCount: Integer; // BAD: lost on restart; this state belongs on the message row + begin + Client.Post('https://svc.contoso.com/api/jobs', BuildContent(IntegrationMessage), Response); + Response.Headers().GetValues('Location', Location); + + // BAD: a tight Sleep-poll loop INSIDE the Job Queue handler. This single flow now holds a + // Job Queue worker slot for the whole external wait. A flow that can take hours starves + // every other job behind it, because the slot is occupied doing nothing but sleeping. + repeat + Sleep(5000); + // BAD: RetryCount is a local. Every BC restart resets it to zero, so backoff and the + // give-up threshold below never behave correctly across a restart: the loop effectively + // starts over, having forgotten how long it has already been waiting. + RetryCount += 1; + Client.Get(Location[1], Response); + until IsComplete(Response) or (RetryCount > 1000); + + // The flow lives only in this session. If BC recycles the session mid-wait, the work is + // orphaned: nothing parked it, so nothing will ever resume it. + end; +} diff --git a/custom/knowledge/integration/park-long-running-work-on-a-status-url.good.al b/custom/knowledge/integration/park-long-running-work-on-a-status-url.good.al new file mode 100644 index 0000000..e3db130 --- /dev/null +++ b/custom/knowledge/integration/park-long-running-work-on-a-status-url.good.al @@ -0,0 +1,59 @@ +// Best practice: on 202 Accepted, PARK the message as Awaiting Reply with the status URL on +// the row, then let a scheduled poll resume it. Retry count and last error live on the MESSAGE, +// so a resume after a restart still knows how often it has tried and why it last failed. + +codeunit 50200 "Long Running Start" +{ + procedure Start(var IntegrationMessage: Record "Integration Message") + var + Client: HttpClient; + Response: HttpResponseMessage; + Headers: HttpHeaders; + Location: array[1] of Text; + begin + Client.Post('https://svc.contoso.com/api/jobs', BuildContent(IntegrationMessage), Response); + + // 202 means "accepted, answer later". Treat it as a DEFERRAL, not a failure to retry and + // not a completion. Re-sending the request here would duplicate work the service already took. + if Response.HttpStatusCode() = 202 then begin + Response.Headers().GetValues('Location', Location); + // Store the status URL on the row and park it. The flow now lives in the database, + // not in this session, so it survives the session ending. + IntegrationMessage."Status URL" := CopyStr(Location[1], 1, 250); + IntegrationMessage.Status := IntegrationMessage.Status::"Awaiting Reply"; + IntegrationMessage.Modify(true); + end; + + // Start returns immediately. Work that waits more than ~30s belongs to external + // orchestration (a Logic App / Durable Function) or a brief scheduled poll, NEVER a + // Job Queue tight loop. The Job Queue owns short, BC-bounded units of work. + end; +} + +codeunit 50201 "Long Running Resume" +{ + TableNo = "Job Queue Entry"; + + // Runs on a schedule. Each invocation does a quick pass over parked rows and returns; it does + // not sit and wait. A flow that is still pending simply gets picked up again next run. + procedure ResumeAwaiting() + var + IntegrationMessage: Record "Integration Message"; + Client: HttpClient; + Response: HttpResponseMessage; + begin + IntegrationMessage.SetRange(Status, IntegrationMessage.Status::"Awaiting Reply"); + if IntegrationMessage.FindSet() then + repeat + if Client.Get(IntegrationMessage."Status URL", Response) and IsComplete(Response) then + Complete(IntegrationMessage) + else begin + // Retry/last-error state lives ON THE MESSAGE, not in a variable. A resume in + // a different session after a restart still sees the true attempt count. + IntegrationMessage."Retry Count" += 1; + IntegrationMessage."Error Message" := CopyStr(LastError(Response), 1, 2048); + IntegrationMessage.Modify(true); + end; + until IntegrationMessage.Next() = 0; + end; +} diff --git a/custom/knowledge/integration/park-long-running-work-on-a-status-url.md b/custom/knowledge/integration/park-long-running-work-on-a-status-url.md new file mode 100644 index 0000000..deb75a6 --- /dev/null +++ b/custom/knowledge/integration/park-long-running-work-on-a-status-url.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [long-running, http-202, status-url, awaiting-reply, orchestration, retry-state, durable-function] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Park long-running work on a status URL + +## Description + +Some external work does not answer in the call that starts it. The service accepts the request, returns `202 Accepted` with a status URL in the Location header, and finishes minutes or hours later. This is a correct and common pattern on the remote side, and Business Central has to handle it correctly on its side, which means avoiding two opposite mistakes. Blocking on the work until it finishes is wrong, because it pins a session for the whole wait (the no-synchronous-wait-loop rule). Firing the request and forgetting it is also wrong, because the answer arrives later with nothing in BC tracking that it is owed. + +The right shape treats the 202 as a deferral, not a failure or a completion. On receiving it, park the Integration Message as Awaiting Reply with the status URL stored on the row, and let a separate scheduled poll resume the flow when the answer is ready. The request and the eventual confirmation are two states of one message sharing a correlation id, not two unrelated events. The detail that makes a parked flow survivable is where its retry state lives: retry count and last error belong on the message row, not in a codeunit variable, because a variable resets on restart and a flow that can wait hours will almost certainly outlive the session that started it. + +## Best Practice + +On a 202, read the Location header, store it on the message as the status URL, and set Status to Awaiting Reply. A scheduled poll reads Awaiting Reply rows, queries each status URL, and advances the message to Resolved when the work is done or records the failure when it is not. Keep retry count and last error on the message, so a resumed poll, possibly running in a different session after a restart, knows how many times it has tried and why it last failed. The mechanism that keeps the Job Queue healthy is the separation between parking and polling: the Job Queue owns short, BC-bounded units of work, so when the wait exceeds roughly 30 seconds the waiting belongs to external orchestration (a Logic App or a Durable Function) that calls back or that BC polls briefly, never a Job Queue tight loop holding a slot for hours. See `park-long-running-work-on-a-status-url.good.al`. + +The trade-off is an extra status field and a poll job, which buys you a flow that resumes correctly across restarts and never monopolises a worker slot. + +## Anti Pattern + +Treating a 202 as a failure and retrying the original request, holding the work in a Job Queue tight loop that `Sleep`-polls the status URL, or keeping retry count in a codeunit variable that resets on restart. The detection signal: a 202 branch that re-sends the original request, a `Sleep` poll loop over a status URL inside a Job Queue handler, or retry/last-error state held in a local or global variable rather than on the Integration Message. The consequences are duplicate work (re-sending a request the service already accepted), a Job Queue slot pinned for the entire external wait (so a flow that waits hours starves other jobs), and a retry counter that resets to zero every restart so backoff and give-up logic never work. The fix is to park as Awaiting Reply with the status URL and resume via a scheduled poll, with all retry state on the row. See `park-long-running-work-on-a-status-url.bad.al`. + +## See also + +- `accept-async-work-instead-of-synchronous-wait-loops.md` +- `propagate-a-correlation-id-across-every-hop.md` +- `split-multi-step-flows-into-staged-job-queue-entries.md` diff --git a/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.bad.al b/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.bad.al new file mode 100644 index 0000000..16b222a --- /dev/null +++ b/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.bad.al @@ -0,0 +1,35 @@ +// Anti-pattern: a hand-written HTTP retry loop for a fire-and-forget notification. It +// reimplements platform retry, backoff, and durability (usually less correctly), couples +// delivery to BC staying up for the life of the loop, and runs inline on the caller's thread. + +codeunit 50162 "Shipment Notifier Bad" +{ + procedure NotifyShipmentReleased(DocumentNo: Code[20]) + var + Client: HttpClient; + Content: HttpContent; + Response: HttpResponseMessage; + Attempt: Integer; + begin + Content.WriteFrom(BuildJson(DocumentNo)); + + // BAD: a hand-rolled retry loop for a one-way notification an external business event + // would carry. Everything in this loop is something the platform already does for free. + for Attempt := 1 to 5 do begin + if Client.Post('https://wms.contoso.com/api/events', Content, Response) then + // BAD: status classification by hand. A real implementation must distinguish + // 408/429/5xx (retry) from 4xx (give up), and this one does not even try. + if Response.IsSuccessStatusCode() then + exit; + + // BAD: hand-rolled backoff. The platform's external-event delivery already retries + // with backoff for up to ~36 hours and persists the state across restarts. + Sleep(Attempt * 2000); + end; + + // BAD: the loop lives entirely in this session. If the WMS is DOWN for the whole window + // the notification is lost; and if BC RESTARTS mid-loop, the retry state is gone and the + // notification is silently lost with no record that it was ever attempted. Nobody is + // alerted, so the gap is found only when downstream is discovered to be out of sync. + end; +} diff --git a/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.good.al b/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.good.al new file mode 100644 index 0000000..d48f4ce --- /dev/null +++ b/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.good.al @@ -0,0 +1,36 @@ +// Best practice: declare an external business event and fire it from a thin subscriber. +// The platform owns retry (408/429/5xx, up to ~36h) and backoff; delivery is asynchronous +// and post-commit, so the notification is sent only if the firing transaction commits. + +codeunit 50160 "Shipment Events v1" +{ + // [ExternalBusinessEvent], not [BusinessEvent]: the externally deliverable flavour that + // external subscribers can register against. Parameters are a minimal DTO of identifiers, + // never the BC record (see version-business-events-and-keep-payloads-stable). + [ExternalBusinessEvent('ShipmentReleased', 'Shipment released', 'Raised when a warehouse shipment is posted', EventCategory::Sales)] + procedure OnShipmentReleased_v1(DocumentNo: Code[20]; ExternalRef: Text[100]) + begin + // Body is intentionally empty: the platform raises and delivers this; we only declare it. + end; +} + +codeunit 50161 "Shipment Event Firer" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Whse.-Post Shipment", 'OnAfterPostWhseShipment', '', false, false)] + local procedure FireShipmentReleased(var WhseShptHeader: Record "Warehouse Shipment Header") + var + Events: Codeunit "Shipment Events v1"; + begin + // Safe to fire from the posting path, even though an HttpClient.Send here would NOT be: + // - this is not an HTTP call, so it holds no lock open on a remote round trip; + // - the platform queues delivery and sends it only AFTER this transaction commits; + // - if posting rolls back, the event is never sent, so nothing leaks on failure. + Events.OnShipmentReleased_v1(WhseShptHeader."No.", WhseShptHeader."External Document No."); + end; +} + +// External subscribers register themselves with no AL change, by POSTing to +// api/microsoft/runtime/v1.0/externaleventsubscriptions +// with eventName, appId, notificationUrl and clientState. Adding a consumer is configuration, +// not code. (External business events are available from runtime 11 and still preview; confirm +// the surface against current docs before relying on it.) diff --git a/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.md b/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.md new file mode 100644 index 0000000..911817d --- /dev/null +++ b/custom/knowledge/integration/prefer-business-events-over-handwritten-retry-loops.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [business-events, external-business-event, retry, outbound, post-commit, subscription, notification] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Prefer business events over handwritten retry loops + +## Description + +For outbound notification, the kind of "something happened, tell whoever is interested" message that does not need an inline answer, a hand-written AL HTTP retry loop reimplements machinery the platform already ships. To be correct, that loop must own exponential backoff, classify which status codes are worth retrying (408, 429, and 5xx) versus which are permanent, persist its retry state so it survives a restart, and stay alive long enough to exhaust its attempts. Most hand-rolled loops get at least one of these wrong, and the failure is silent: a notification is simply lost and nobody notices until a downstream system is found to be out of sync. + +An `[ExternalBusinessEvent]` hands all of that to the platform. Business Central retries the delivery on 408, 429, and 5xx responses for up to roughly 36 hours, persists the delivery state itself, and lets external subscribers register without any AL change. Just as important, delivery is asynchronous and post-commit: the platform sends the notification only after the firing transaction commits, and never sends it if that transaction rolls back. That is why firing an event from a posting or release path is safe even though calling `HttpClient` from the same path is not. Prefer the event over the loop wherever a subscriber can register for it. + +## Best Practice + +Declare an `[ExternalBusinessEvent('name', 'Display', 'Desc', Category)]` whose parameters are a minimal DTO of identifiers (a document number, an external reference) rather than a record, and fire it from a thin subscriber on the real event such as release or post. The mechanism that makes this both reliable and safe is the platform's delivery model: the event is queued in the committing transaction, so it exists only if the business action succeeded, and the platform then owns retry and backoff against the registered notification URLs. External subscribers self-serve by POSTing to `api/microsoft/runtime/v1.0/externaleventsubscriptions` with the event name, app id, notification URL, and client state, so adding a consumer is a configuration step, not a code change. External business events are available from runtime 11 and are still labelled preview, so confirm the surface against current docs before relying on it. See `prefer-business-events-over-handwritten-retry-loops.good.al`. + +The trade-off and its boundary: business events are for fire-and-forget notification, not for request/response where you need an answer in the same call. If the caller must act on a returned value, this is the wrong tool; stage an outbound message and call with an idempotency key instead. + +## Anti Pattern + +A custom AL codeunit that loops over `HttpClient.Send` with `Sleep` backoff to deliver a notification that an external business event could carry. The detection signal: a retry loop counting attempts around an outbound POST, classifying 429/5xx by hand, with `Sleep`-based backoff, where the payload is a one-way notification rather than a request needing an inline reply. The consequence is twofold: delivery is coupled to Business Central staying up for the life of the loop (a restart mid-loop loses the notification with no record), and you have reimplemented, usually less correctly, the retry, backoff, and durability the platform already provides. The fix is to declare an external business event and fire it from a thin subscriber. See `prefer-business-events-over-handwritten-retry-loops.bad.al`. + +## See also + +- `version-business-events-and-keep-payloads-stable.md` +- `monitor-external-event-subscription-health.md` +- `never-call-external-services-from-posting.md` diff --git a/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.bad.al b/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.bad.al new file mode 100644 index 0000000..b9ee53d --- /dev/null +++ b/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.bad.al @@ -0,0 +1,32 @@ +// Anti-pattern: a new id minted at a downstream hop, and an outbound call with no correlation +// header. Nothing ties the inbound message, this processing step, and the external system's +// logs together. Tracing a failure becomes correlation-by-timestamp guesswork. + +codeunit 50192 "Outbound Step Bad" +{ + procedure Send(var IntegrationMessage: Record "Integration Message") + var + Client: HttpClient; + Request: HttpRequestMessage; + Headers: HttpHeaders; + Response: HttpResponseMessage; + LocalTrace: Guid; + begin + // BAD: a brand-new id, unrelated to IntegrationMessage."Correlation ID". The entry point + // already minted the flow's trace id; minting another one here breaks the chain just as + // thoroughly as having none, because the two halves now log different identifiers. + LocalTrace := CreateGuid(); + + // This trace value appears in no other component's logs, so it joins to nothing. + LogStep('sending', Format(LocalTrace)); + + Request.SetRequestUri('https://svc.contoso.com/api/orders'); + Request.Method := 'POST'; + Request.GetHeaders(Headers); + + // BAD: no Correlation-Id header at all. The receiver logs the call under its own ids, and + // there is no shared value to join the external system's logs back to this flow. When this + // POST fails three hops into a busy system, reconstructing what happened is archaeology. + Client.Send(Request, Response); + end; +} diff --git a/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.good.al b/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.good.al new file mode 100644 index 0000000..a14c41f --- /dev/null +++ b/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.good.al @@ -0,0 +1,54 @@ +// Best practice: mint the correlation id ONCE at the entry point and carry it unchanged on +// every staged message, event payload, queue header, and outbound call. Log it at every step, +// so one filter pulls the entire flow across BC, the queue, and the external system. + +codeunit 50190 "Inbound Entry" +{ + // The boundary: this is the ONLY place a correlation value is created. Everything downstream + // reads it, never regenerates it. + procedure Receive(Payload: Text) + var + IntegrationMessage: Record "Integration Message"; + begin + IntegrationMessage.Init(); + IntegrationMessage."Message ID" := CreateGuid(); + // Generated once, here, at the entry point. This is the trace id for the whole flow. + IntegrationMessage."Correlation ID" := NewCorrelationId(); + IntegrationMessage.SetRequest(Payload); + IntegrationMessage.Insert(true); + + // Log it on the very first step, so even the inbound receipt is part of the trace. + LogStep('received', IntegrationMessage."Correlation ID"); + end; + + local procedure NewCorrelationId(): Code[40] + begin + exit(CopyStr(DelChr(LowerCase(Format(CreateGuid())), '=', '{}'), 1, 40)); + end; +} + +codeunit 50191 "Outbound Step" +{ + // A downstream hop. It READS the correlation id off the message it was handed; it does not + // mint a new one, because that would split the flow into two untraceable halves. + procedure Send(var IntegrationMessage: Record "Integration Message") + var + Client: HttpClient; + Request: HttpRequestMessage; + Headers: HttpHeaders; + Response: HttpResponseMessage; + begin + Request.SetRequestUri('https://svc.contoso.com/api/orders'); + Request.Method := 'POST'; + Request.GetHeaders(Headers); + + // Carry the SAME id onto the outbound call as a header. The receiver logs it too, so the + // external system's logs can be joined back to the BC side by this one value. + Headers.Add('Correlation-Id', IntegrationMessage."Correlation ID"); + + Client.Send(Request, Response); + // Same id logged on this hop. Request and confirmation rows share it, so a status query + // or failure investigation pulls the whole chain with a single filter. + LogStep('sent', IntegrationMessage."Correlation ID"); + end; +} diff --git a/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.md b/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.md new file mode 100644 index 0000000..8f7fff9 --- /dev/null +++ b/custom/knowledge/integration/propagate-a-correlation-id-across-every-hop.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [correlation-id, tracing, propagation, queue-header, telemetry, end-to-end, trace] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Propagate a correlation id across every hop + +## Description + +A single integration flow touches many components in sequence: an inbound message arrives, a row is staged, a business event fires, a queue entry is picked up, an outbound call goes out, a confirmation comes back. Each component logs its own activity, but unless one identifier is threaded through all of them, those log lines are isolated islands. When something fails three hops in, reconstructing what happened means correlating by timestamp and hope, guessing which inbound message produced which outbound call, which is slow at the best of times and nearly impossible when the system is busy and many flows are interleaved. + +A correlation id solves this by giving every component in one flow the same trace identifier to log and to pass along. It is the second most load-bearing field on the Integration Message after its own key, because it is what turns a pile of disconnected log entries into a single traceable story. The discipline that matters is that it is set exactly once, at the point where the flow enters Business Central, and then carried unchanged everywhere downstream. The most common failure is not the absence of a correlation id but its regeneration: a downstream step that mints a fresh id breaks the chain just as thoroughly as having none, because now two halves of the same flow log different identifiers. + +## Best Practice + +Generate the correlation id once at the entry point, the webhook receiver, the poll handler, or the first staged message, and never regenerate it downstream. Carry it on every subsequent Integration Message, every event payload, every queue message header, and every outbound HTTP request as a header such as `Correlation-Id`, and log it at every step. The mechanism that pays off is that the request row and its eventual confirmation row, and every step in between, all carry the one identifier, so a status query or a failure investigation pulls the entire chain, across Business Central, the queue, and the external system, with a single filter. Read the id from the incoming message rather than creating a new one; the only `CreateGuid` for a correlation value lives at the entry point. See `propagate-a-correlation-id-across-every-hop.good.al`. + +The cost is one field carried and one header set per hop, which is trivial; the payoff is that incident response goes from archaeology to a single filtered query. + +## Anti Pattern + +Generating a new id at each hop, or not carrying the id onto outbound calls and queue headers at all, so each component logs an unrelated identifier. The detection signal: a `CreateGuid()` producing a correlation value inside a downstream processor or outbound step rather than reading the id from the incoming message, an outbound `HttpClient` request or event payload that omits the correlation header, or log statements that emit a locally minted trace value. The consequence is that no two components share a trace id, so tracing a failure across the flow requires correlating by timestamp and guesswork, and the external system's logs can never be joined back to the BC side at all. The fix is one id minted at the entry point and read, never regenerated, by every hop after it. See `propagate-a-correlation-id-across-every-hop.bad.al`. + +## See also + +- `stage-every-integration-message.md` +- `park-long-running-work-on-a-status-url.md` +- `monitor-external-event-subscription-health.md` diff --git a/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.bad.al b/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.bad.al new file mode 100644 index 0000000..befd3c2 --- /dev/null +++ b/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.bad.al @@ -0,0 +1,37 @@ +// Anti-pattern: a fresh key per attempt (and the loop would be just as broken with no key). +// Every retry looks like a brand-new request, so after an uncertain failure the receiver +// applies the side effect AGAIN. A flaky payment service produces duplicate charges exactly +// when it is least healthy. + +codeunit 50151 "Outbound Caller Bad" +{ + procedure Send(var IntegrationMessage: Record "Integration Message") + var + Client: HttpClient; + Request: HttpRequestMessage; + Content: HttpContent; + Headers: HttpHeaders; + Response: HttpResponseMessage; + Attempt: Integer; + begin + for Attempt := 1 to 3 do begin + Content.WriteFrom(IntegrationMessage.GetRequest()); + Request.Content := Content; + Request.Method := 'POST'; + Request.SetRequestUri('https://pay.contoso.com/api/charges'); + Request.GetHeaders(Headers); + + // BAD: a new GUID on every attempt. The key is supposed to let the receiver + // recognise a retry, but a value that changes each time is functionally NO key: + // attempt 2 and attempt 3 each look like a completely new charge request. + Headers.Add('Idempotency-Key', Format(CreateGuid())); + + // The dangerous case is the UNCERTAIN failure. If attempt 1 actually reached the + // service and captured the payment, but the response was lost to a timeout, then + // Send returns false here and the loop retries. Attempt 2 carries a different key, + // so the service captures the payment a SECOND time. The customer is charged twice. + if Client.Send(Request, Response) and Response.IsSuccessStatusCode() then + exit; + end; + end; +} diff --git a/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.good.al b/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.good.al new file mode 100644 index 0000000..768cb15 --- /dev/null +++ b/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.good.al @@ -0,0 +1,47 @@ +// Best practice: every outbound call carries Idempotency-Key = the Integration Message GUID. +// The key is created once when the message is staged and never changes, so the first call +// and every retry (Job Queue or operator-driven) send the SAME key. A well-behaved receiver +// collapses them into a single side effect and returns the original response. + +codeunit 50150 "Outbound Caller" +{ + procedure Send(var IntegrationMessage: Record "Integration Message") + var + Client: HttpClient; + Request: HttpRequestMessage; + Content: HttpContent; + ContentHeaders: HttpHeaders; + RequestHeaders: HttpHeaders; + Response: HttpResponseMessage; + begin + Content.WriteFrom(IntegrationMessage.GetRequest()); + + // Content-Type belongs on the content headers, not the request headers. + Content.GetHeaders(ContentHeaders); + if ContentHeaders.Contains('Content-Type') then + ContentHeaders.Remove('Content-Type'); + ContentHeaders.Add('Content-Type', 'application/json'); + + Request.Content := Content; + Request.Method := 'POST'; + Request.SetRequestUri('https://pay.contoso.com/api/charges'); + Request.GetHeaders(RequestHeaders); + + // THE key line. The value is the staged Message ID, which is stable for the life of + // the message. Calling Send again for the same row sends this exact same value, so the + // payment service sees the retry as a repeat of one charge and captures money once. + RequestHeaders.Add('Idempotency-Key', StableKey(IntegrationMessage)); + + // After an UNCERTAIN failure (timeout, dropped connection, 502) the Job Queue will + // re-run this message. Because the key is unchanged, the retry is safe: no double charge. + Client.Send(Request, Response); + IntegrationMessage.RecordResult(Response); + end; + + // The key is derived purely from the durable message id. Nothing here changes between + // attempts: no CreateGuid, no timestamp, no attempt counter. + local procedure StableKey(IntegrationMessage: Record "Integration Message"): Text + begin + exit(DelChr(LowerCase(Format(IntegrationMessage."Message ID")), '=', '{}')); + end; +} diff --git a/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.md b/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.md new file mode 100644 index 0000000..df3854b --- /dev/null +++ b/custom/knowledge/integration/send-an-idempotency-key-on-every-outbound-call.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [idempotency-key, outbound, http-header, retry, message-guid, side-effect, uncertain-failure] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Send an idempotency key on every outbound call + +## Description + +When Business Central calls an external system, some failures are certain (a 400 that clearly rejected the request) but the dangerous ones are uncertain: a socket timeout, a dropped connection, a 502 from a gateway in front of a service that may have processed the request anyway. After an uncertain failure you genuinely do not know whether the work landed. You must retry to make progress, but a blind retry risks doing the side effect twice: a second payment captured, a second shipment booked, a second order placed. The uncertainty is inherent to networks and cannot be engineered away; what you can do is make the retry safe. + +An idempotency key makes it safe. It is a value the caller sends so the receiver can recognise a repeat of a request it has already handled and return the original response instead of acting again. The contract is the caller's responsibility: the receiver can only deduplicate if every retry of the same logical request carries the same key. That is the crux of the rule, because the most common bug is a key that changes between attempts, which looks like a fix but is functionally no key at all. Every outbound call that has a side effect must carry a stable key. + +## Best Practice + +Set an `Idempotency-Key` header on every outbound request and derive its value from the Integration Message GUID, which is created once when the message is staged and never changes. Because the key lives on the staged row, every retry of that row, whether by the Job Queue minutes later or by an operator resolving a failed message days later, sends the identical key, so a well-behaved receiver collapses all of them into one side effect and returns the same response. The mechanism is the binding of the key to the durable message rather than to the attempt: a new key is minted only when a genuinely new message is created. See `send-an-idempotency-key-on-every-outbound-call.good.al`. + +This pairs with re-running the same message on manual resolution (see `make-failed-integration-messages-manually-resolvable.md`): because resolution re-runs the same Message ID, it reuses the same idempotency key, so even a human-driven retry cannot double-apply. The only cost is one header per request and the discipline of never regenerating the key. + +## Anti Pattern + +An outbound `HttpClient` call with no idempotency header, or one that generates a fresh key per attempt (for example `CreateGuid()` or a counter inside the retry loop) so each retry looks like a brand-new request to the receiver. The detection signal: an `HttpClient.Post`/`Send` building an outbound request that has a side effect but no `Idempotency-Key` header, or a key whose source is anything that changes between attempts (a `CreateGuid()` inside the loop, a timestamp, an attempt counter). The consequence is that after an uncertain failure the retry double-applies the side effect, so a slow or flaky receiver produces duplicate payments and duplicate shipments precisely when it is least healthy. The fix is one stable key derived from the message GUID, set on every attempt. See `send-an-idempotency-key-on-every-outbound-call.bad.al`. + +## See also + +- `deduplicate-inbound-messages-with-an-idempotency-check.md` +- `make-failed-integration-messages-manually-resolvable.md` +- `park-long-running-work-on-a-status-url.md` diff --git a/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.bad.al b/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.bad.al new file mode 100644 index 0000000..407e15a --- /dev/null +++ b/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.bad.al @@ -0,0 +1,38 @@ +// Anti-pattern: one handler runs every step in a single transaction, and a SingleInstance +// codeunit holds lookups that leak across runs. One big lock, one big rollback, and stages +// that should be independent are coupled through shared state. + +codeunit 50213 "Monolithic Flow Bad" +{ + procedure RunAll(var IntegrationMessage: Record "Integration Message") + begin + // BAD: fetch, transform, post, and notify all run in ONE transaction. Every lock any step + // takes is held until the final step commits, so the slowest/most contended step sets the + // lock duration for all of them. + Fetch(IntegrationMessage); + Transform(IntegrationMessage); + + // If Post fails, Fetch and Transform ROLL BACK with it: their successful work is discarded + // and the whole flow must re-run from the start, redoing work that had already succeeded. + Post(IntegrationMessage); + + // A transient hiccup HERE, after a perfectly good post, throws away that post too, because + // it is all one transaction. The unit of failure is the entire flow, not the failing step. + Notify(IntegrationMessage); + end; +} + +codeunit 50214 "Cross Stage Cache Bad" +{ + SingleInstance = true; // BAD: a global cache that survives between stage runs couples the stages + var + ItemCache: Dictionary of [Code[20], Code[20]]; + + procedure Lookup(ItemNo: Code[20]): Code[20] + begin + // BAD: stages that read this cache now depend on whichever earlier run populated it. They + // can no longer be retried or reordered in isolation, which is exactly the independence a + // staged split is supposed to give. Wanting a cache this global is the tell the split is wrong. + exit(ItemCache.Get(ItemNo)); + end; +} diff --git a/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.good.al b/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.good.al new file mode 100644 index 0000000..38b37f8 --- /dev/null +++ b/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.good.al @@ -0,0 +1,59 @@ +// Best practice: each stage is its own Job Queue entry implementing IIntegrationStage, +// dispatched from an extensible enum. Status is the cursor that records the flow's position. +// Stages share no state across runs, so each has its own short lock window and its own retry. + +interface IIntegrationStage +{ + procedure Run(var IntegrationMessage: Record "Integration Message"); + procedure NextStatus(): Enum "Integration Status"; +} + +// Extensible: adding a stage is ONE new codeunit plus ONE enum value, with no orchestrator change. +enum 50210 "Integration Stage" implements IIntegrationStage +{ + Extensible = true; + value(0; Fetch) { Implementation = IIntegrationStage = "Stage Fetch"; } + value(1; Transform) { Implementation = IIntegrationStage = "Stage Transform"; } + value(2; Post) { Implementation = IIntegrationStage = "Stage Post"; } +} + +codeunit 50211 "Stage Transform" implements IIntegrationStage +{ + procedure Run(var IntegrationMessage: Record "Integration Message") + var + ItemCache: Dictionary of [Code[20], Code[20]]; + begin + // The cache is local to THIS run. It is created here and gone when Run returns, so it + // cannot couple this stage to any other. If a lookup were hot enough to want a GLOBAL + // cache, that would be the signal the split is in the wrong place. + TransformPayload(IntegrationMessage, ItemCache); + // Only this stage's work is in scope, so its lock window is short and it commits on its own. + end; + + procedure NextStatus(): Enum "Integration Status" + begin + // Advances the cursor to the next stage. A failure here rolls back ONLY this stage; + // Fetch stays committed and the flow resumes from Transform, not from the start. + exit("Integration Status"::Post); + end; +} + +codeunit 50212 "Stage Dispatcher" +{ + TableNo = "Job Queue Entry"; + + // Each invocation runs ONE stage as its own Job Queue entry, advances Status, then the next + // stage runs as a separate entry. No step holds a lock across another step's work. + procedure RunStage(var IntegrationMessage: Record "Integration Message"; Stage: Enum "Integration Stage") + var + StageImpl: Interface IIntegrationStage; + begin + StageImpl := Stage; + StageImpl.Run(IntegrationMessage); + + // Status is the cursor: it records where the flow is up to, so resuming is just reading + // the next stage. There is no separate per-stage row to reconcile. + IntegrationMessage.Status := StageImpl.NextStatus(); + IntegrationMessage.Modify(true); + end; +} diff --git a/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.md b/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.md new file mode 100644 index 0000000..c3c1d7d --- /dev/null +++ b/custom/knowledge/integration/split-multi-step-flows-into-staged-job-queue-entries.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [staged-pipeline, job-queue, iintegrationstage, interface, lock-window, no-shared-state, rollback] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Split multi-step flows into staged job queue entries + +## Description + +A multi-step integration flow (fetch, transform, post, notify) handled by one big codeunit in one transaction is one big lock and one big rollback. Every lock the flow takes anywhere along the chain is held until the final step commits, so the slowest or most contended step sets the lock duration for all of them, and a failure in the last step discards the successful work of every earlier step. A transient hiccup while notifying then throws away a posting that was perfectly good, and the whole flow re-runs from the start, redoing work that had already succeeded. + +Splitting the flow into stages, each its own Job Queue entry, changes the unit of failure and the unit of locking. Every stage gets its own short lock window and its own retry policy, and a failed stage rolls back only its own work, leaving earlier stages committed and the flow free to resume from where it stopped. The flow advances stage by stage with the message Status as the cursor: Status records the position, so resuming is just reading the next stage to run, and there is no separate per-stage row to reconcile. The discipline that keeps the stages genuinely independent is that they share no state across runs. + +## Best Practice + +Make each stage its own Job Queue entry with its own short lock window and retry. Have stages implement a common `IIntegrationStage` interface dispatched from an extensible enum, so adding a stage is one new codeunit plus one enum value with no change to the orchestrator. Use Status as the cursor that records the flow's position; do not create a new row per stage, because the message is the flow and its Status is where it is up to. The rule that makes the split real is shared state: a stage may cache item, customer, or location lookups within a single run (a Dictionary that is created and discarded inside one invocation), but never across stages and never globally, because cross-run cache is exactly the coupling the split exists to remove. A useful tell is that if a lookup is hot enough to tempt you toward a global cache, the split is in the wrong place. See `split-multi-step-flows-into-staged-job-queue-entries.good.al`. + +The trade-off is more moving parts (an interface, an enum, several codeunits) in exchange for short lock windows, per-stage retry, and a flow that resumes instead of restarting. That trade is worth making once a flow has more than one step that can fail independently. + +## Anti Pattern + +One handler that runs every step in a single transaction, or stages that pass data through a global or cross-invocation cache. The detection signal: a single codeunit whose `Run`/`OnRun` does fetch, transform, post, and notify in sequence in one transaction, or a `SingleInstance` codeunit or other long-lived holder caching lookups that survive between stage runs. The consequence of the monolith is one lock window covering the whole chain and one rollback that discards all prior work when any step fails; the consequence of the shared cache is that stages which should be independent are coupled, so they can no longer be retried or reordered in isolation. The fix is one Job Queue entry per stage behind an interface, Status as the cursor, and per-run-only caching. See `split-multi-step-flows-into-staged-job-queue-entries.bad.al`. + +## See also + +- `park-long-running-work-on-a-status-url.md` +- `stage-every-integration-message.md` +- `use-a-framing-record-for-inbound-polling.md` diff --git a/custom/knowledge/integration/stage-every-integration-message.bad.al b/custom/knowledge/integration/stage-every-integration-message.bad.al new file mode 100644 index 0000000..c2ef91f --- /dev/null +++ b/custom/knowledge/integration/stage-every-integration-message.bad.al @@ -0,0 +1,47 @@ +// Anti-pattern: the inbound API insert trigger enriches and posts inline. +// The HTTP request now holds posting locks and depends on two remote systems +// (the pricing service and the caller) staying responsive. A slow pricing call +// or a posting error surfaces to the caller as a request timeout, and the whole +// transaction rolls back: the message is lost, with no staged row to retry. + +page 50100 "Sales Order Intake API" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'integration'; + APIVersion = 'v1.0'; + EntityName = 'salesOrderIntake'; + EntitySetName = 'salesOrderIntakes'; + SourceTable = "Sales Header"; + DelayedInsert = true; + + layout + { + area(Content) + { + repeater(Group) + { + field(externalNo; Rec."External Document No.") { } + field(sellToCustomerNo; Rec."Sell-to Customer No.") { } + } + } + } + + trigger OnInsertRecord(BelowxRec: Boolean): Boolean + var + SalesPost: Codeunit "Sales-Post"; + PricingClient: Codeunit "External Pricing Client"; + begin + // BAD: a second remote call, inside the request that is creating the row. + // If the pricing service is slow, the caller's HTTP request blocks on it. + Rec.Validate("Unit Price", PricingClient.GetPrice(Rec."No.")); + + // BAD: posting inline, inside the request transaction. Posting locks are + // held for the whole HTTP round trip. A posting error rolls back the + // insert too, so there is nothing left to inspect or retry. + SalesPost.Run(Rec); + + // There is no staging row. A duplicate delivery (the source retried after + // a timeout) is processed again from scratch, creating a second order. + end; +} diff --git a/custom/knowledge/integration/stage-every-integration-message.good.al b/custom/knowledge/integration/stage-every-integration-message.good.al new file mode 100644 index 0000000..020f06c --- /dev/null +++ b/custom/knowledge/integration/stage-every-integration-message.good.al @@ -0,0 +1,99 @@ +// Best practice: a single staging table, a thin acceptance endpoint, and a +// background processor. The endpoint only validates and stages; it never posts +// and never calls the source system back. The Job Queue codeunit does the real +// work later, decoupled from the caller and from the remote system's uptime. + +// --- The spine: one staging table for every inbound and outbound message --- +table 50100 "Integration Message" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "Message ID"; Guid) { Caption = 'Message ID'; } + field(2; Direction; Enum "Integration Direction") { Caption = 'Direction'; } + // Type drives the dispatcher below. A new message kind is a new branch, + // not a new published API page. + field(3; "Type"; Code[40]) { Caption = 'Type'; } + field(4; Status; Enum "Integration Status") { Caption = 'Status'; } + // The source system's stable id. Drives inbound de-duplication, so it + // carries a unique key, never the internal Message ID. + field(5; "External Reference"; Text[100]) { Caption = 'External Reference'; } + field(6; "Correlation ID"; Code[40]) { Caption = 'Correlation ID'; } + field(10; Request; Blob) { Caption = 'Request'; } + field(11; Response; Blob) { Caption = 'Response'; } + field(20; "Error Message"; Text[2048]) { Caption = 'Error Message'; } + field(21; "Retry Count"; Integer) { Caption = 'Retry Count'; } + } + + keys + { + key(PK; "Message ID") { Clustered = true; } + // The work key the Job Queue queries: which rows still need processing. + key(Work; Status, Direction) { } + // The idempotency key: detect a replayed inbound message at insert time. + key(Idempotency; "External Reference", "Type") { } + } +} + +// --- Phase one: acceptance. Validate, stage, return. No posting here. --- +page 50100 "Integration Message API" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'integration'; + APIVersion = 'v1.0'; + EntityName = 'integrationMessage'; + EntitySetName = 'integrationMessages'; + SourceTable = "Integration Message"; + DelayedInsert = true; + + layout + { + area(Content) + { + repeater(Group) + { + field(externalReference; Rec."External Reference") { } + field(type; Rec.Type) { } + field(request; Rec.Request) { } + } + } + } + + trigger OnInsertRecord(BelowxRec: Boolean): Boolean + begin + // The only work the endpoint does: stamp identity and mark the row New. + // Everything expensive happens later, in the Job Queue processor. + Rec."Message ID" := CreateGuid(); + Rec.Direction := Rec.Direction::Inbound; + Rec.Status := Rec.Status::New; + end; +} + +// --- Phase two: processing. Runs as a Job Queue entry, reads staged rows. --- +codeunit 50101 "Inbound Message Processor" +{ + TableNo = "Job Queue Entry"; + + trigger OnRun() + var + IntegrationMessage: Record "Integration Message"; + begin + // Read by Status, never from an HTTP call. The caller is long gone. + IntegrationMessage.SetRange(Direction, IntegrationMessage.Direction::Inbound); + IntegrationMessage.SetRange(Status, IntegrationMessage.Status::New); + if IntegrationMessage.FindSet() then + repeat + Dispatch(IntegrationMessage); + until IntegrationMessage.Next() = 0; + end; + + // The Type field routes to the right handler. No giant CASE in the endpoint. + local procedure Dispatch(var IntegrationMessage: Record "Integration Message") + begin + // ... resolve a handler by IntegrationMessage.Type and run it; on + // success set Status::Resolved, on failure stamp Error Message and + // bump Retry Count so the row stays auditable and re-runnable. + end; +} diff --git a/custom/knowledge/integration/stage-every-integration-message.md b/custom/knowledge/integration/stage-every-integration-message.md new file mode 100644 index 0000000..82ef3d2 --- /dev/null +++ b/custom/knowledge/integration/stage-every-integration-message.md @@ -0,0 +1,34 @@ +--- +bc-version: [all] +domain: integration +keywords: [integration, staging, integration-message, webhook, posting, decoupling] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Stage every integration message + +## Description + +Every message that crosses the Business Central boundary, inbound or outbound, should be written to one staging table (the Integration Message) before any business work runs against it. Posting, document creation, and notification then operate on staged data, never on a live external call. This is the single most load-bearing rule in BC integration design because it decouples the local transaction from the availability and latency of a system you do not control: an external outage delays processing, it never breaks posting or forces a rollback. A flow that skips staging couples a database transaction to a remote endpoint, so a slow or failed remote call surfaces inside BC as a request timeout, a lock held too long, or a half-finished document. + +The Integration Message is a normal table whose rows carry everything a processor needs to act without calling back to the source: the external reference, the message type, a status, the request and response payloads, the correlation id, and the retry state. Inbound and outbound rows share the table and are told apart by a Direction field. + +## Best Practice + +Treat staging as a two-phase split. Phase one is acceptance: a webhook receiver or an API page validates the payload, writes one Integration Message row, and returns. It does no posting and makes no second remote call. Phase two is processing: a background Job Queue codeunit reads rows by Status and does the real work, fully decoupled from the original caller. + +Expose the Integration Message as a single API page and let a Type field route each row to the correct dispatcher codeunit, rather than versioning a separate endpoint per source system. One endpoint plus a Type-driven dispatcher means a new message kind is a new dispatcher branch, not a new published API surface. Keep an idempotency key on the external reference so a replayed delivery is detected at insert time rather than processed twice. See `stage-every-integration-message.good.al` for the intake page, the staging table shape, and the Job Queue processor. + +## Anti Pattern + +A webhook handler, an API page insert trigger, or a Job Queue poll handler that posts a document inline, or that calls the external service again to enrich the message before returning. Both couple the request to live database locks and to the remote system staying up. + +Detection signal for a reviewer or agent: an HTTP-triggered handler or an `OnInsertRecord`/`OnModifyRecord` trigger on an API page that calls a posting codeunit (`Codeunit "Sales-Post"`, `OnAfterPostSalesDoc`, and similar) or `HttpClient.Send` directly, instead of `Insert`-ing an Integration Message row and returning. The fix is structural: move the post and the callout into the Job Queue processor that reads staged rows. See `stage-every-integration-message.bad.al`. + +## See also + +- `accept-async-work-instead-of-synchronous-wait-loops.md` +- `never-call-external-services-from-posting.md` +- `deduplicate-inbound-messages-with-an-idempotency-check.md` diff --git a/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.bad.al b/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.bad.al new file mode 100644 index 0000000..94d7201 --- /dev/null +++ b/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.bad.al @@ -0,0 +1,27 @@ +// Anti-pattern: no framing record, no watermark, no lock. Every run fetches the whole +// collection, and two overlapping Job Queue runs stage the same records twice. + +codeunit 50132 "Inbound Poller Bad" +{ + TableNo = "Job Queue Entry"; + + procedure Poll() + var + Client: HttpClient; + Response: HttpResponseMessage; + begin + // BAD: "fetch all". No last-fetch datetime feeds the request and no window cap bounds it, + // so the cost of every poll grows with the TOTAL data set, not with what is new. Records + // that were already staged and resolved are pulled again and reprocessed every run. + Client.Get('https://svc.contoso.com/api/orders', Response); + + // BAD: no lock. The Job Queue can start the next run before this one finishes (a run that + // overruns its recurrence interval overlaps the following one). Both runs fetch the full + // collection concurrently and BOTH stage every order, so each order lands twice and becomes + // a duplicate document downstream. + StageAll(Response); + + // There is also no watermark to advance, so even back-to-back runs cannot narrow their + // windows: there is no notion of "where we left off" anywhere in this design. + end; +} diff --git a/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.good.al b/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.good.al new file mode 100644 index 0000000..a13a399 --- /dev/null +++ b/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.good.al @@ -0,0 +1,78 @@ +// Best practice: one framing record per feed. It remembers where the last run stopped, +// caps how much one run pulls, and carries a lock with a stale timeout so two overlapping +// Job Queue runs can never fetch the same window. + +table 50130 "Inbound Feed Frame" +{ + DataClassification = SystemMetadata; + + fields + { + field(1; "Feed Code"; Code[20]) { Caption = 'Feed Code'; } + // The watermark: the next run starts here, so no run ever re-pulls old data. + field(2; "Last Fetch At"; DateTime) { Caption = 'Last Fetch At'; } + // The cap: a long-quiet feed catches up over several bounded runs instead of one huge pull. + field(3; "Max Window (Hours)"; Integer) { Caption = 'Max Window (Hours)'; } + // Opaque continuation token from the source's paged API, when it offers one. + field(4; "Cursor"; Text[250]) { Caption = 'Cursor'; } + // The lock: a second concurrent run sees this set and backs off. + field(5; "Locked"; Boolean) { Caption = 'Locked'; } + // Stamped when the lock is taken, so a crashed run's lock can be reclaimed after a timeout. + field(6; "Locked At"; DateTime) { Caption = 'Locked At'; } + } + + keys { key(PK; "Feed Code") { Clustered = true; } } +} + +codeunit 50131 "Inbound Poller" +{ + TableNo = "Job Queue Entry"; + + procedure Poll(FeedCode: Code[20]) + var + Frame: Record "Inbound Feed Frame"; + WindowEnd: DateTime; + begin + Frame.Get(FeedCode); + + // Acquire the lock first. If another run already owns this feed, exit quietly: + // overlap is the whole problem we are preventing. + if not TryAcquireLock(Frame) then + exit; + + // Bounded incremental window: from the watermark up to a capped end. Never "fetch all". + WindowEnd := CapWindow(Frame."Last Fetch At", Frame."Max Window (Hours)"); + FetchAndStage(Frame, Frame."Last Fetch At", WindowEnd); + + // Advance the watermark and cursor so the NEXT run starts exactly where this one ended. + // Sequential runs therefore never overlap their windows. + Frame."Last Fetch At" := WindowEnd; + Frame."Cursor" := NextCursor(); + ReleaseLock(Frame); + end; + + local procedure TryAcquireLock(var Frame: Record "Inbound Feed Frame"): Boolean + begin + // If the lock is held AND fresh, someone is actively polling: do not steal it. + if Frame."Locked" and (CurrentDateTime() - Frame."Locked At" < GetStaleTimeoutMs()) then + exit(false); + + // Either free, or stale (a previous run crashed without releasing). Reclaim it. + Frame."Locked" := true; + Frame."Locked At" := CurrentDateTime(); + Frame.Modify(true); + Commit(); // make the lock durable before the long fetch starts + exit(true); + end; + + local procedure ReleaseLock(var Frame: Record "Inbound Feed Frame") + begin + Frame."Locked" := false; + Frame.Modify(true); + end; + + local procedure GetStaleTimeoutMs(): Integer + begin + exit(300000); // 5 minutes: longer than a healthy run, short enough to recover from a crash + end; +} diff --git a/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.md b/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.md new file mode 100644 index 0000000..1913439 --- /dev/null +++ b/custom/knowledge/integration/use-a-framing-record-for-inbound-polling.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [polling, framing-record, cursor, lock, stale-lock, incremental-window, overlap] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Use a framing record for inbound polling + +## Description + +When Business Central polls an external paged API on a schedule, two facts about the schedule create problems that a naive poll handler ignores. First, the handler needs durable memory of where it left off, because the Job Queue run that fetched the last window is gone by the time the next one starts. Second, the Job Queue can overlap: a run that takes longer than its recurrence interval is still working when the next run begins, so two runs are live at once. A framing record is the small per-feed table that solves both: it holds the last fetch datetime, a maximum window size, an optional cursor token, and a lock flag with a stale-lock timeout. + +Without a framing record a poll fails in one of two ways, both of which get worse the busier the feed is. A handler with no last-fetch memory re-fetches the entire collection every run, so the cost of a poll grows with the total data set rather than with what is new, and resolved messages are restaged and reprocessed. A handler with no lock lets overlapping runs fetch the same window concurrently, so the same records are staged twice and downstream they become duplicate documents. The fix is the same record in both cases: bounded windows fix re-fetching, the lock fixes overlap. + +## Best Practice + +Keep one framing record per inbound feed and drive every poll through it. At the start of a run, acquire the lock, honouring a stale-lock timeout so a run that crashed without releasing the lock does not wedge the feed forever; if the lock is held and fresh, the run exits and lets the holder finish. Then compute a bounded window from the last fetch datetime up to a capped end (never an open-ended "everything since"), fetch exactly that window, advance the cursor and last fetch datetime, and release the lock. The mechanism that prevents double-staging is the lock plus the advancing watermark: the second overlapping run sees the lock held and backs off, and even sequential runs never overlap their windows because each one starts where the previous one's watermark ended. See `use-a-framing-record-for-inbound-polling.good.al`. + +The window cap is a deliberate trade-off: capping the end datetime means a feed that has been quiet for a long time catches up over several runs rather than pulling a huge window in one go, which keeps each run bounded and each lock window short. Pair this with idempotent staging (see `deduplicate-inbound-messages-with-an-idempotency-check.md`) so that even a window boundary that overlaps slightly cannot create duplicates. + +## Anti Pattern + +A poll handler that fetches the full collection every run, or that has no lock so concurrent Job Queue runs fetch and stage the same records. The detection signal: a polling codeunit whose `HttpClient` call has no last-fetch datetime or cursor feeding the request (a bare "get all"), no cap on the requested window, or no lock acquisition guarding the fetch. The consequence of fetch-all is wasted work that scales with the whole data set and reprocessing of already-resolved messages; the consequence of a missing lock is duplicate staging whenever two runs overlap, which surfaces downstream as duplicate documents. The fix is a per-feed framing record with a watermark, a window cap, and a stale-aware lock. See `use-a-framing-record-for-inbound-polling.bad.al`. + +## See also + +- `deduplicate-inbound-messages-with-an-idempotency-check.md` +- `stage-every-integration-message.md` +- `split-multi-step-flows-into-staged-job-queue-entries.md` diff --git a/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.bad.al b/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.bad.al new file mode 100644 index 0000000..00d3bba --- /dev/null +++ b/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.bad.al @@ -0,0 +1,30 @@ +// Anti-pattern: mutating a published signature, passing the whole record plus a secret, and +// firing without validation or failure classification. Every external subscriber breaks, and +// credentials and every table field leak into the payload. + +codeunit 50173 "Order Events Bad" +{ + // This event already shipped as OnOrderConfirmed(OrderNo: Code[20]). Subscribers bound to + // that signature. Editing it IN PLACE to add parameters silently breaks all of them: there + // is no compiler across the boundary, so the notification just starts arriving in the wrong + // shape and processing fails on the far side, far from this change. + [ExternalBusinessEvent('OrderConfirmed', 'Order confirmed', 'Raised on confirm', EventCategory::Sales)] + procedure OnOrderConfirmed(var SalesHeader: Record "Sales Header"; ApiKey: Text) + // BAD: the full record exposes every field of Sales Header to every subscriber and couples + // the contract to the table layout. ApiKey leaks a secret credential into the payload. + begin + end; +} + +codeunit 50174 "Order Publisher Bad" +{ + procedure Publish(SalesHeader: Record "Sales Header") + var + Events: Codeunit "Order Events Bad"; + begin + // BAD: no validation. A header with no document number is published and then, because + // the data is permanently invalid, retried by the platform indefinitely. There is no + // transient/permanent classification, so an unfixable payload is treated like a blip. + Events.OnOrderConfirmed(SalesHeader, GetSecretApiKey()); + end; +} diff --git a/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.good.al b/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.good.al new file mode 100644 index 0000000..a979221 --- /dev/null +++ b/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.good.al @@ -0,0 +1,42 @@ +// Best practice: one events codeunit per version, versioned procedure names, a minimal +// stable DTO of identifiers, validation before firing, and transient/permanent classification. + +codeunit 50170 "Order Events v1" +{ + // Shipped contract. Once a subscriber binds to OnOrderConfirmed_v1, this signature is FROZEN. + // Parameters are identifiers only: a subscriber calls back for detail, so no field leaks and + // no secret travels in the payload. + [ExternalBusinessEvent('OrderConfirmed', 'Order confirmed', 'Raised when a sales order is confirmed', EventCategory::Sales)] + procedure OnOrderConfirmed_v1(OrderNo: Code[20]; ExternalRef: Text[100]) + begin + end; +} + +codeunit 50171 "Order Event Publisher" +{ + procedure Publish(SalesHeader: Record "Sales Header") + var + Events: Codeunit "Order Events v1"; + begin + // Validate BEFORE firing. A header with no number cannot produce a meaningful + // notification, so this is a PERMANENT failure: fail and alert, do not publish it and + // let the platform retry an unfixable payload for 36 hours. + if SalesHeader."No." = '' then + Error('Cannot publish OrderConfirmed without a document number'); + + // Transient conditions (subscriber temporarily down, network blip) are NOT handled here: + // the platform's external-event delivery retries those for us. We only guard permanent ones. + Events.OnOrderConfirmed_v1(SalesHeader."No.", SalesHeader."External Document No."); + end; +} + +// A breaking change does NOT edit OnOrderConfirmed_v1. It ships a NEW codeunit with a NEW +// procedure, so v1 subscribers keep receiving exactly what they bound to and new subscribers +// opt into the richer v2 shape. +codeunit 50172 "Order Events v2" +{ + [ExternalBusinessEvent('OrderConfirmedV2', 'Order confirmed (v2)', 'Adds the warehouse location code', EventCategory::Sales)] + procedure OnOrderConfirmed_v2(OrderNo: Code[20]; ExternalRef: Text[100]; LocationCode: Code[10]) + begin + end; +} diff --git a/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.md b/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.md new file mode 100644 index 0000000..9425ec3 --- /dev/null +++ b/custom/knowledge/integration/version-business-events-and-keep-payloads-stable.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: integration +keywords: [business-events, versioning, dto, payload-contract, validate, transient-permanent, breaking-change] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Version business events and keep payloads stable + +## Description + +A business event payload is a published contract. Once an external subscriber has bound to an event's name and signature, that signature is no longer yours to change quietly: adding a parameter, reordering parameters, or retyping one changes the shape the subscriber receives, and because external subscribers live outside your app there is no compiler to catch the break. The notification simply starts arriving in a shape the consumer does not expect, and the failure surfaces as malformed data or dropped processing on the far side, often long after the change shipped and far from the code that caused it. + +Two disciplines keep the contract honest. The first is versioning: treat a shipped event signature as frozen, and express any change as a new procedure or a new codeunit rather than an edit to the old one, so existing subscribers keep receiving exactly what they bound to while new subscribers opt into the new shape. The second is payload hygiene: the payload must be a minimal, stable DTO of identifiers and just enough context, never the raw BC record (which exposes every field and couples the contract to the table layout) and never a secret such as an API key or token (which leaks credentials to every subscriber). A payload that must also be validated before firing, so invalid data is never published and then retried forever. + +## Best Practice + +Put all events for one integration version in a single events codeunit, and give a second version its own codeunit. Name each procedure with its version suffix (`OnOrderConfirmed_v1`) so the version is visible at the call site and a new version sits beside the old one rather than replacing it. Pass a small DTO of identifiers so a subscriber can call back for detail without the payload leaking fields or secrets. Validate the payload before firing, so a record that cannot produce a meaningful notification fails fast rather than being published and retried indefinitely. Classify failures as transient (network, subscriber temporarily down: let the platform retry) versus permanent (invalid data, a malformed payload: fail, alert, and consider a dead-letter path) so a permanent error is not retried for 36 hours as if it were a blip. See `version-business-events-and-keep-payloads-stable.good.al`. + +The trade-off is more codeunits over time as versions accumulate, which is the correct cost: a stable contract for existing consumers is worth more than a tidy single signature that silently breaks them. + +## Anti Pattern + +Adding or reordering parameters on an already-published event, passing the whole BC record or secret-bearing fields as the payload, or firing without validating first. The detection signal: an edit to the signature of an existing `[ExternalBusinessEvent]` or `[BusinessEvent]` that has already shipped, a parameter typed as a full table record (`var Rec: Record ...`) on an event, a payload field that holds a key or token, or a fire with no prior validation and no transient-versus-permanent classification of the failure. The consequence is silently broken subscribers (signature change), leaked data or credentials (record or secret payload), and infinite retry of unfixable data (no validation). The fix is a new versioned procedure or codeunit, a minimal identifier DTO, validation before firing, and explicit failure classification. See `version-business-events-and-keep-payloads-stable.bad.al`. + +## See also + +- `prefer-business-events-over-handwritten-retry-loops.md` +- `monitor-external-event-subscription-health.md` +- `send-an-idempotency-key-on-every-outbound-call.md` diff --git a/custom/knowledge/operations/inspect-the-al-runtime-during-a-debug-session.md b/custom/knowledge/operations/inspect-the-al-runtime-during-a-debug-session.md new file mode 100644 index 0000000..0131d56 --- /dev/null +++ b/custom/knowledge/operations/inspect-the-al-runtime-during-a-debug-session.md @@ -0,0 +1,28 @@ +--- +bc-version: [28] +domain: operations +keywords: [troubleshooting-mcp-server, debug, call-stack, runtime, copilot, breakpoint] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Inspect the AL runtime during a debug session + +## Description + +The Troubleshooting MCP Server lets GitHub Copilot Chat read the live AL runtime state during an active debug session: the call stack, the variables at any frame, the source for a frame, and a breakpoint set by object and line number while paused. It is available only while a debug session is paused at a breakpoint or a runtime error, and only on BC 2026 release wave 1 (BC 28) or later. It is the right surface when you want a natural-language explanation that follows a deep call stack or an answer to why a particular code path executed, rather than stepping through manually. + +What it reads is the runtime as it stands at the pause point, so it answers questions about the present state of an execution rather than its history. The value over manual stepping is that Copilot can fan out across many frames and variables at once and summarise them, which is exactly the work that is tedious to do by hand on a deep stack. It reads; it does not write, step, or apply fixes. + +## Best Practice + +Reach for the Troubleshooting MCP Server when a runtime error has fired or a paused stack is deep across many objects and you want it summarised, or when you want to know why a branch took a particular path without manual stepping. Pause at the breakpoint or error first, then ask Copilot explicitly to use the server, since it does not always reach for it on its own. Use the variable inspection to surface database statistics (SQL latency, executes, row reads) that traditional stepping hides, which is good for spotting hidden DB calls in subscribers. Pair it with the performance profiler: profile first for slowness, then set a breakpoint at the slow frame and ask the Troubleshooting MCP for the runtime detail. + +## Anti Pattern + +Using it for the wrong job or expecting capabilities it does not have. It is not a replacement for interactive step-through debugging, for a quick look at one variable, or for learning unfamiliar code by reading it. It offers no time travel, so it only shows what is in scope right now, and it returns no source for frames whose code lives only in compiled .app packages, where you fall back to inspecting variables. The signal of misuse: trying to invoke it with no active paused debug session, on a version before BC 28, or expecting it to replay history or auto-apply fixes. + +## See also + +- `run-headless-al-builds-with-the-al-mcp-server.md` diff --git a/custom/knowledge/operations/know-the-limits-of-saas-point-in-time-restore.md b/custom/knowledge/operations/know-the-limits-of-saas-point-in-time-restore.md new file mode 100644 index 0000000..3e373d5 --- /dev/null +++ b/custom/knowledge/operations/know-the-limits-of-saas-point-in-time-restore.md @@ -0,0 +1,28 @@ +--- +bc-version: [all] +domain: operations +keywords: [saas-restore, point-in-time, backup, retention, sandbox, production] +technologies: [powershell] +countries: [w1] +application-area: [all] +--- + +# Know the limits of SaaS point-in-time restore + +## Description + +A Business Central SaaS point-in-time restore is bounded by hard platform limits that decide whether a restore is even possible, and by what the platform does and does not bring back. Before promising a customer a restore, you need to know these limits: the backup retention window is the last 28 days, restores are capped per calendar month, the restore must stay in the same Azure region, the localisation cannot change, and a sandbox cannot be restored to production (the allowed paths are production to production, production to sandbox, and sandbox to sandbox). Promising a restore that the limits forbid, or assuming integrations come back live, sets a false expectation during an incident, which is the worst moment to discover a constraint. + +The restore is not a snapshot that comes up identical to the source. It is a managed operation that rebuilds business data and then deliberately neutralises anything that could fire against stale data or reach the wrong system, so the difference between what is restored and what is reset is the part a hand-off most often gets wrong. + +## Best Practice + +Check feasibility against the limits before committing: confirm the desired restore point is within the last 28 days, the target is in the same Azure region, the localisation is unchanged, and the path is allowed. Know what comes back and what does not. Business data, posted documents, master and setup data are restored; AppSource apps return at their latest hotfix even if newer than the restore point; dev-only extensions installed from VS Code are not in the backup and must be reinstalled. The most surprising part is that integrations come up disabled or cleared on purpose, so they cannot fire on stale data, which means a post-restore checklist of re-enabling and re-credentialing each one and running smoke tests. Tell the customer up front about downtime, lost work after the restore point, and integrations needing manual reconnection. + +## Anti Pattern + +Promising or attempting a restore without checking the limits, or assuming the restored environment comes up exactly as it was. The consequences: a restore that is simply not allowed (cross-region, localisation change, sandbox to production, outside the 28-day window, or over the monthly cap), or a customer surprised that integrations are off and post-restore work is gone. The signal: a restore committed to before the restore point, region, localisation, path, and retention window have been confirmed, or a hand-off that does not warn about disabled integrations and lost post-restore-point work. + +## See also + +- `restrict-bc-environments-with-an-entra-security-group.md` diff --git a/custom/knowledge/performance/profile-before-optimising-with-the-performance-profiler.md b/custom/knowledge/performance/profile-before-optimising-with-the-performance-profiler.md new file mode 100644 index 0000000..e00a8f9 --- /dev/null +++ b/custom/knowledge/performance/profile-before-optimising-with-the-performance-profiler.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: performance +keywords: [performance-profiler, alcpuprofile, analyze-performance, slow-page, slow-posting, evidence, triage] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Profile before optimising with the performance profiler + +## Description + +When a Business Central page, report, or posting routine is slow, the first step is to capture a profile with the built-in Analyze Performance profiler, not to start changing code. The profiler produces an `.alcpuprofile` snapshot that shows which app, which objects, and which functions consumed the time during the exact slow action, across Microsoft first-party apps and all installed third-party extensions. Optimising before profiling means guessing: the obvious suspect is often not the real cost, and a change made without evidence cannot be shown to have helped. The profile is also the evidence a technical consultant or partner needs to triage the problem. + +Profiling first reframes the work from a hunch into a measurement. The snapshot attributes time to a concrete call path, so the conversation moves from "the customer card feels slow" to "this subscriber runs a FindSet inside a loop and accounts for most of the time." That attribution is what makes a fix targeted and what lets a before-and-after comparison prove the fix worked rather than merely shifting the cost somewhere less visible. + +## Best Practice + +Capture the profile against the real slow action: open Help and Support, choose Troubleshooting then Analyze Performance, click Start, reproduce the slow action exactly (open the slow page, post the slow document, run the slow report), then Stop and Download Profile. Aim for a 5 to 30 second capture; longer runs get noisy. Read it starting from Active Apps to find the dominant app, cross-check Time Spent to tell continuous AL cost from spiky SQL or external-service cost, then use Aggregate Results and the Call Tree to find the specific functions to fix. When handing a profile to a partner, attach the `.alcpuprofile` file (it is plain JSON, safe to send) with the plain-language action description, the exact reproduction steps, expected versus actual timing, and the BC version and installed-extensions list. + +## Anti Pattern + +Changing AL code to "make it faster" on a hunch, without first capturing a profile of the slow action, or sending a partner a vague slowness report ("the customer card is slow") with no `.alcpuprofile` and no reproduction steps. The consequence is wasted effort optimising code that was never the bottleneck and no way to prove the change helped. The signal to look for: a performance-motivated code change or a partner escalation that cites no captured profile and no before-and-after timing as evidence. Remember the profiler shows AL-side time only: for network latency, SQL execution plans, or Job Queue contention, reach for telemetry instead. diff --git a/custom/knowledge/process/ground-work-in-a-solution-constitution.md b/custom/knowledge/process/ground-work-in-a-solution-constitution.md new file mode 100644 index 0000000..1237b83 --- /dev/null +++ b/custom/knowledge/process/ground-work-in-a-solution-constitution.md @@ -0,0 +1,29 @@ +--- +bc-version: [all] +domain: process +keywords: [constitution, brief, tech-design, roadmap, project-context, sdd] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Ground work in a solution constitution + +## Description + +A Business Central solution should maintain a small set of durable, high-level documents (the constitution) that every feature spec is grounded in: a project brief (customer and localisation, business processes, goals, non-goals, constraints, success measures), a technical design (architecture, which standard BC modules to reuse, the honest custom-code gaps, the assigned object ID range, the high-level data model, integrations, cross-cutting concerns), and a roadmap (an ordered, numbered feature list with status). These are the documents every agent and engineer reads before doing anything, so individual feature work stays consistent with the agreed direction instead of each feature re-deciding the architecture. + +The constitution exists so that decisions are made once and reused, not re-litigated per feature. The brief fixes the business intent, the technical design fixes the architecture and the object ID range, and the roadmap fixes the order and the numbering that feature folders follow. Because every spec is checked against all three, the documents are where solution-wide consistency actually lives; without them each feature quietly invents its own answer to questions the solution already decided. + +## Best Practice + +Establish the constitution once at the start of a solution, and refresh rather than rewrite it when the business need changes materially, preserving decisions still valid. Interview for missing facts rather than inventing them. Write the brief in plain language with no AL, the technical design favouring reuse of standard BC and justifying every custom-code gap, and the roadmap as a numbered feature list so feature folders match the numbering. Treat the constitution as a human decision: stop for review before proceeding to feature specs. Every feature spec must then be consistent with all three documents. + +## Anti Pattern + +Specifying or building features with no shared brief, technical design, or roadmap to ground them. The consequence is features that contradict each other on architecture, object ID ranges, or which standard modules to reuse, because each one re-decides in isolation. The signal: feature specs that exist with no constitution behind them, or custom AL introduced with no recorded justification for not reusing standard BC. + +## See also + +- `specify-before-you-build.md` +- `map-each-feature-to-an-object-id-range.md` diff --git a/custom/knowledge/process/map-each-feature-to-an-object-id-range.md b/custom/knowledge/process/map-each-feature-to-an-object-id-range.md new file mode 100644 index 0000000..67eb677 --- /dev/null +++ b/custom/knowledge/process/map-each-feature-to-an-object-id-range.md @@ -0,0 +1,29 @@ +--- +bc-version: [all] +domain: process +keywords: [plan, object-id-range, al-objects, task-list, data-model, sdd] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Map each feature to an object ID range + +## Description + +Between an approved spec and implementation comes a technical plan that maps the what onto the how: which standard BC modules to reuse, which custom AL objects are genuinely needed, and an object table giving each new object a name, type, and an ID inside the feature's reserved object ID range. The plan also captures the data model, integration points, cross-cutting concerns (permissions, telemetry, upgrade and migration, performance), and an ordered, checkable task list. Doing this before writing production AL keeps object IDs inside the assigned range, surfaces the standard-versus-custom decision explicitly, and gives implementation a sequenced list rather than an open-ended coding task. + +The object table is the part that turns the constitution's reserved range into concrete allocations. Choosing each object's ID up front, against the range the technical design assigned, is what prevents two features from colliding and what lets a verifier reject an out-of-range object as a plan defect rather than a late rework. The task list does the same for sequencing: by naming explicit tasks for permissions, telemetry, tests, and upgrade steps, it stops those cross-cutting concerns from being remembered only after the feature code is written. + +## Best Practice + +After the spec is approved and before implementing, write a plan that decides what standard BC to reuse and what custom AL is needed, with every new object assigned an ID inside the feature's reserved range. Produce an ordered task list that includes explicit tasks for permission-set entries, telemetry, a test per acceptance criterion, the build-and-verify pass, and docs. Pre-flight the plan against the rules the verifiers will enforce later, such as object IDs in range and an upgrade step for any schema change, so implementation starts clean. Stop for review of the plan and object list before writing production AL. + +## Anti Pattern + +Implementing a feature directly from the spec with no object plan, so object IDs are picked ad hoc outside the reserved range, the reuse-versus-custom decision is made implicitly while coding, and there is no ordered task list to work through. The consequence is ID collisions, missed permission-set or upgrade tasks, and rework when a verifier rejects an out-of-range object late. The signal: new AL objects with IDs outside the feature's assigned range, or a feature being built with no plan mapping the spec to a concrete object list and task sequence. + +## See also + +- `specify-before-you-build.md` +- `ground-work-in-a-solution-constitution.md` diff --git a/custom/knowledge/process/specify-before-you-build.md b/custom/knowledge/process/specify-before-you-build.md new file mode 100644 index 0000000..1adb4eb --- /dev/null +++ b/custom/knowledge/process/specify-before-you-build.md @@ -0,0 +1,29 @@ +--- +bc-version: [all] +domain: process +keywords: [spec, specification, acceptance-criteria, requirements, sdd, before-implementation] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Specify before you build + +## Description + +Before planning or writing any AL for a Business Central feature, write a feature specification that captures the problem, the users and roles, the scope and out-of-scope edges, the user flow, and testable acceptance criteria. The spec is the what and why; it deliberately names no AL objects. Writing it first grounds the work in agreed requirements and makes the result verifiable: each acceptance criterion is concrete enough to become a test, so "done" is something you can check rather than something you argue about after the code exists. + +Keeping AL out of the spec is deliberate, not an omission. Naming objects too early collapses the what into the how and quietly commits the design before anyone has agreed what the feature must do. The acceptance criteria are the load-bearing part: they are written so each one maps to a single test, which means the spec doubles as the test plan and the definition of done is fixed before any code can drift away from it. + +## Best Practice + +For each feature, produce a spec before the plan and before any code. State the problem and the affected users and roles, draw the scope and out-of-scope boundaries, describe the user flow, and write acceptance criteria concrete enough to turn directly into tests. Record genuinely open items under open questions rather than guessing, and stop for human review of the spec before planning or implementing. Keep AL object names out of the spec; those belong to the planning step. At implementation, confirm every acceptance criterion is covered by a passing test. + +## Anti Pattern + +Jumping into AL with only an informal idea of the feature and no written, reviewable acceptance criteria. The consequence is scope that drifts during coding, no shared definition of done, and a result that cannot be verified against agreed requirements. The signal: a feature being implemented with no spec, or a spec that lists vague goals instead of testable acceptance criteria, or one that has already committed to AL object names before the what and why are agreed. + +## See also + +- `ground-work-in-a-solution-constitution.md` +- `map-each-feature-to-an-object-id-range.md` diff --git a/custom/skills/integration/azure-integration-review.md b/custom/skills/integration/azure-integration-review.md new file mode 100644 index 0000000..5138e60 --- /dev/null +++ b/custom/skills/integration/azure-integration-review.md @@ -0,0 +1,73 @@ +--- +kind: action-skill +id: azure-integration-review +version: 1 +title: Azure integration review +description: The integration-plane review playbook for the Azure side of a BC integration, pairing with the BC-side validator. +inputs: [repository, pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [bicep, csharp] +countries: [w1] +application-area: [all] +--- + +# Azure integration review + +Reviews the Azure side of a Business Central integration as a playbook: the webhook receiver that catches a storefront event, the Logic App that routes a shipment to a WMS, the Service Bus topic that carries Business Events, the Durable Function that schedules a retry, and the Bicep, ARM, or Terraform that provisions them. The integration plane earns its keep by keeping retry, dead-letter, observability, and credential handling outside BC, so BC stays free of external credentials and third-party schema changes do not break it. This skill pairs with the BC-side `azure-integration-validator` so the inbound, outbound, long-running, and manual arrows line up end to end. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository`, a `pr-diff` (a change to the integration plane), or a `file-path` (a specific artifact to review). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `integration` or `security` as the citable candidate set across every enabled layer; the playbook's rules about receivers, idempotency, retry, dead-letter, correlation, observability, long-running poll, subscription health, and secret handling can match a curated file. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The Azure-side house rules are largely not covered by a BC-focused curated file, so most findings are agent findings within this skill's domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the BC version the plane integrates with, or `unknown` if unavailable. +- `technologies`: `[bicep, csharp]` (the infrastructure-as-code and Function handler code the playbook reviews). +- `countries`: the consuming solution's declared countries, or `unknown`. +- `application-area`: the application areas of the integration, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the artifacts present and the playbook rules each draws. If the repository has no Azure artifacts, report that plainly rather than inventing findings. Read Bicep, ARM, and Terraform that provision Functions, Logic Apps, APIM, Service Bus, and Storage; Logic App and workflow definitions; Function app config (`host.json`, `function.json`); APIM policy XML; and pipeline files. Place each artifact on the four arrows (inbound, outbound, long-running, manual) and build the worklist against the playbook rules: receiver stages to BC, receiver acknowledges fast, idempotency key forwarded, idempotent consumer, retry in the plane, dead-letter configured, transient versus permanent classification, durable retry not a tight loop, correlation header on every hop, observability wired, 202 status poll for long-running, subscription health check, secrets in Key Vault, Managed Identity, HTTPS only. Also worklist the cross-checks with the BC side: an outbound BC call's idempotency key the plane must forward, a BC Correlation ID the plane must carry, a parked long-running message the plane must drive the poll for, and Business Event subscriptions the plane must monitor. + +A curated `integration` or `security` file enters the worklist when its `keywords` intersect these tokens. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklisted artifact and rule, evaluate the plane against the playbook. Where a curated `integration` or `security` knowledge file states the rule, emit a knowledge-backed finding citing it: `id` equal to the file path, `severity` up to `blocker` only when the file states a platform-level guarantee, otherwise `major`, `confidence` `high` for an unambiguous match. Where no curated file covers the rule (the common case for the Azure-side checks), emit an agent finding within this skill's domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:az-receiver-stages-to-bc`, `agent:az-correlation-header`, `agent:az-subscription-health-check`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the artifact, what the playbook expects, what the artifact does, and the concrete fix. When the underlying impact would otherwise be a blocker (a receiver running BC business logic inline, a missing dead-letter path swallowing poison messages, a stripped correlation id breaking end-to-end tracing), keep the emitted `severity` at `minor` but say so plainly in the `message` and flag that the rule should be promoted to a curated knowledge file before it can gate. Set `suggested-code` when the fix is a mechanical edit to a contiguous artifact span; otherwise set `suggested-code-omission-reason`. Hold every agent candidate to the precision bar in `skills/do.md`: steelman that the plane's choice is deliberate before emitting, and omit when in doubt. + +Outcome selection: `completed` when every worklisted artifact was reviewed (including a clean plane with empty `findings`); `not-applicable` when the repository contains no Azure integration artifacts (report this in `outcome-reason`); `no-knowledge` when artifacts exist but no curated knowledge survived and no agent finding was raised; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. Playbook rules with no curated backing are agent findings (`references: []`, `agent:` id mirroring the `az-*` rule, severity capped at `minor`, gating impact stated in the message); findings citing an `integration` or `security` file carry that file path as `id` and primary reference. + +```json +{ + "skill": { "id": "azure-integration-review", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 3, "items-evaluated": 3 } + }, + "findings": [ + { + "id": "agent:az-correlation-header", + "severity": "minor", + "message": "functions/ShipmentRouter reads the inbound message but does not set the Correlation ID on the outbound Service Bus header, so a trace cannot be joined across BC, the plane, and the WMS. Read the correlation id from the inbound message and set it on the Service Bus message header and every outbound HTTP header, and log it at each step. Promote to a curated rule before it can gate.", + "location": { "file": "functions/ShipmentRouter/run.csx" }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "fix spans message construction and logging, not a single contiguous span" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/integration/azure-integration-validator.md b/custom/skills/integration/azure-integration-validator.md new file mode 100644 index 0000000..4851d5f --- /dev/null +++ b/custom/skills/integration/azure-integration-validator.md @@ -0,0 +1,97 @@ +--- +kind: action-skill +id: azure-integration-validator +version: 1 +title: Azure integration validator +description: Validates the Azure plane for a BC integration, checking receivers, Service Bus, Durable Functions, retry/dead-letter, idempotency, correlation, secrets, and subscription health. +inputs: [repository, file-path, pr-diff] +outputs: [findings-report] +bc-version: [all] +technologies: [bicep, csharp] +countries: [w1] +application-area: [all] +--- + +# Azure integration validator + +Validates the Azure component build that sits between Business Central and external systems: webhook receivers (Functions, Logic Apps, APIM), Service Bus topics and queues, Durable Functions, and the Bicep, ARM, or Terraform that provisions them. The single question is whether, when BC stages a message or fires an event, the plane delivers it reliably, traceably, and exactly once. It reads the artifacts and reports where the plane fails its half of the contract; the developer chooses which fixes to apply. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository`, a `file-path` (a narrow scope such as the storefront webhook Function), or a `pr-diff` (a change touching Azure integration artifacts). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `integration` or `security` as the citable candidate set across every enabled layer: receiver-staging, idempotency, retry-and-dead-letter, correlation, observability, secret-handling, and managed-identity rules can back a finding. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The integration-plane house rules (the `az-*` checks below) are largely Azure-side and rarely map onto a BC-focused curated file, so most findings here are agent findings within this skill's domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version the plane integrates with, or `unknown` if unavailable. +- `technologies`: `[bicep, csharp]` (the infrastructure-as-code and Function handler code the checks actually touch). +- `countries`: the consuming solution's declared countries, or `unknown`. +- `application-area`: the application areas of the integration, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the Azure artifacts present and the integration-plane checks each draws. If the repository contains no Azure integration artifacts, do not invent findings: report that plainly. Read Bicep (`*.bicep`), ARM (`azuredeploy.json`, `*.template.json`), Terraform (`*.tf`), Logic App and workflow definitions (`workflow.json`, `*.logicapp.json`), Function app config (`host.json`, `function.json`, retry and binding config) and handler source, and APIM policy XML (inbound, backend, outbound, on-error). Build the worklist against these checks: + +- Receiver stages to BC and does not run BC business logic or block on BC completion inline. +- Receiver acknowledges fast (a 2xx, or 202 for async); no long synchronous work inside it. +- Idempotency key forwarded: outbound calls carry the BC Message ID as `Idempotency-Key`; inbound receivers forward the source system id; the plane does not strip it. +- Idempotent consumer: receivers and queue consumers dedup on the event id or business key before a second side effect. +- Retry in the plane: explicit on the Logic App action, the Function `host.json`, or the Service Bus delivery count, not a silent default or a hand-written loop. +- Dead-letter configured: Service Bus queues and subscriptions enable dead-lettering with a defined max delivery count. +- Transient versus permanent: retries 408, 429, 5xx, and timeouts; routes 4xx and invalid data to DLQ or alert. Retrying a 4xx forever is the most severe failure. +- Durable retry, not a tight loop: long retries scheduled by a Durable Function or a Logic App timer carrying the same idempotency key. +- Correlation header: the Correlation ID is read from the inbound message, set on the Service Bus header and every outbound HTTP header, and logged at each step. +- Observability wired: Functions and Logic Apps have Application Insights or equivalent. +- 202 status poll: a long-running external process is parked and polled or callback-driven, then written back to the same Integration Message; no synchronous connection held open for hours. +- Subscription health check: where the plane relies on BC Business Event subscriptions, a scheduled job lists them and alerts on drift, since they expire silently. +- Secrets in Key Vault: credentials and connection strings come from Key Vault via Managed Identity, not inline. A literal secret is the most severe failure. +- Managed Identity: plane-to-BC and plane-to-resource auth uses Managed Identity where supported. +- HTTPS only: receivers and Function apps enforce HTTPS with a current TLS minimum; the Function is not public where APIM is the intended front door. + +A curated `integration` or `security` file enters the worklist when its `keywords` intersect these tokens. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklisted artifact, evaluate it against the checks. Where a curated `integration` or `security` knowledge file states the rule (for example a secret-handling or idempotency rule), emit a knowledge-backed finding citing it: `id` equal to the file path, `severity` up to `blocker` only when the file states a platform-level guarantee, otherwise `major`, `confidence` `high` for an unambiguous match. Where no curated file covers the integration-plane check (the common case), emit an agent finding within this skill's domain: `references: []`, `id` slug prefixed `agent:` mirroring the house rule (for example `agent:az-secrets-in-keyvault`, `agent:az-dead-letter-configured`, `agent:az-classify-transient-vs-permanent`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the artifact and line, what is wrong, and the concrete fix. When the underlying impact would otherwise be a blocker (a literal secret checked into source, a 4xx retried forever, a receiver running BC logic inline, a stripped idempotency key), keep the emitted `severity` at `minor` but say so plainly in the `message` and flag that the check should be promoted to a curated rule before it can gate. Set `suggested-code` when the fix is a mechanical edit to a contiguous artifact span (a Key Vault reference replacing a literal, a `maxDeliveryCount` plus dead-letter setting on a subscription); otherwise set `suggested-code-omission-reason`. Hold every agent candidate to the precision bar in `skills/do.md`: steelman that the configuration is deliberate (the secret may be a non-sensitive placeholder, the retry default may be intended) before emitting, and omit when in doubt. + +Outcome selection: `completed` when every worklisted artifact was evaluated (including a clean plane with empty `findings`); `not-applicable` when the repository contains no Azure integration artifacts (report this in `outcome-reason`); `no-knowledge` when artifacts exist but no curated knowledge survived and no agent finding was raised; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. Integration-plane checks with no curated backing are agent findings (`references: []`, `agent:` id mirroring the `az-*` rule, severity capped at `minor`, gating impact stated in the message); findings citing an `integration` or `security` file carry that file path as `id` and primary reference. + +```json +{ + "skill": { "id": "azure-integration-validator", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 2, "info": 0 }, + "coverage": { "worklist-size": 4, "items-evaluated": 4 } + }, + "findings": [ + { + "id": "agent:az-secrets-in-keyvault", + "severity": "minor", + "message": "infra/main.bicep line 142: the WMS API key is a literal string in the Function app settings, checked into source and visible in deployment history. Impact is a blocker: move the key to Key Vault and reference it via @Microsoft.KeyVault(...), granting the Function access through its Managed Identity. Promote to a curated rule before it can gate.", + "location": { "file": "infra/main.bicep", "line": 142 }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "fix requires creating a Key Vault secret and a reference whose name is not derivable from the diff" + }, + { + "id": "agent:az-dead-letter-configured", + "severity": "minor", + "message": "infra/servicebus.bicep line 60: the shipments subscription sets neither deadLetteringOnMessageExpiration nor maxDeliveryCount, so poison messages loop or vanish. Enable dead-lettering with a defined max delivery count and add a consumer or alert on the DLQ.", + "location": { "file": "infra/servicebus.bicep", "line": 60 }, + "references": [], + "confidence": "medium" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-appsource-validator.md b/custom/skills/review/al-appsource-validator.md new file mode 100644 index 0000000..cd85651 --- /dev/null +++ b/custom/skills/review/al-appsource-validator.md @@ -0,0 +1,90 @@ +--- +kind: action-skill +id: al-appsource-validator +version: 1 +title: AL AppSource submission validation +description: Audits an AL extension against AppSourceCop rules, app.json metadata, artefacts, links, and the dependency chain, and emits a findings report. +inputs: [repository, object-list] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL AppSource submission validation + +Audits a Business Central extension against the rules Microsoft's AppSource validation applies, plus the soft conventions that surface in the manual review pass, so the developer fixes gates locally rather than after a multi-day Microsoft review cycle. It reports each finding against the `AS0xxx` rule that will flag it. Coverage spans `app.json` metadata completeness, object suffix discipline, object id ranges, permission-set coverage, prohibited objects, translations, logo and screenshots, EULA and privacy and help and url links, the dependency chain, runtime-versus-target-versus-application alignment, demo and dev artefacts, telemetry consent, id collisions, and the marketplace listing checklist folded in from the AppSource validation playbook. It sources from the `security` and `style` knowledge domains and cites curated rules where present; the AppSource-specific gates the corpus does not encode are agent findings within its AppSource compliance domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository` or an `object-list`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `security` or `style` as the citable candidate set across every enabled layer: permission-set minimal-grant and wildcard guidance, captions and tooltips required on page fields, and label discipline each map onto a curated rule and MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The AppSource gates themselves (a `AS0xxx` rule violation, a dead link, a missing logo, a runtime mismatch, the listing-metadata checklist) are not encoded in the corpus; for those concrete defects, emit an agent finding within this skill's AppSource compliance domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries (the `supportedCountries`), or `unknown`. +- `application-area`: the application areas of the extension's objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the submission gates for the extension under review: + +- `app.json` metadata: `id` a stable GUID, `name`/`publisher`/`version` matching the listing, `brief` (empty is AS0036) and `description`, `privacyStatement`, `EULA`, `help`, `url`, `logo`, `runtime`, `target` (`Cloud` for AppSource), `application`, `platform`, all set and not the AL scaffold default; `showMyCode` set only when intentional. +- Object suffix discipline against the `AppSourceCop.json` `mandatorySuffix` (AS0040/AS0041), object ids inside `idRanges` (AS0072), no objects in the system range, no use of Microsoft `Access = Internal` platform objects. +- Permission-set coverage (the AS0029-class tabledata gap), and `supportedCountries` each having an xliff (AS0091). +- Logo PNG at least 350 by 350 and square; at least one screenshot present per the manifest; EULA, privacy, help, and url links resolving with a 2xx HEAD response. +- Dependencies each with `id`/`name`/`publisher`/`version`, version either `0.0.0.0` or a real published version, `propagateDependencies` set when downstream consumers need access (AS0078/AS0079); `runtime` aligned with `target` and `application`; object ids not colliding with the platform or other dependency-chain extensions. +- No demo or dev artefacts in src (`RunModal` in startup paths, hardcoded passwords, demo `Confirm` boxes, `Sleep` in production codeunits); telemetry consent stated in the privacy statement when `applicationInsightsConnectionString` is set. +- Marketplace listing checklist folded in from the AppSource validation playbook: search summary 100 characters or under, description leading with the value proposition, signing via the Key Vault pipeline, README/SETUP/SUPPORT files, support email pointing at the team inbox rather than a personal address, privacy and terms URLs live. + +A curated `security` or `style` file enters the worklist when its `keywords` intersect these tokens (for example `permission-set`, `caption`, `tooltip`, `label`). Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each gate, emit a finding. + +When the gate maps onto a curated `security` or `style` rule (an over-broad permission grant, a page field missing a `Caption` or `ToolTip`), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous match. + +When the gate is an AppSource-specific defect with no curated rule, emit an agent finding within this skill's AppSource compliance domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:as0036-empty-brief` or `agent:runtime-target-mismatch`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the `AS0xxx` rule or listing requirement and the concrete fix. Where the impact would normally gate (any hard AppSource rejection), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the field is intentionally set as-is before emitting, and omit when in doubt. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is a single contiguous metadata edit (setting a `brief` value, correcting a `runtime` number); otherwise set `suggested-code-omission-reason` (for example `requires creating a logo asset` or `requires a live privacy-policy URL`). + +Outcome selection: `completed` when every gate was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the task has no extension manifest to validate; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-appsource-validator", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 14, "items-evaluated": 14 } + }, + "findings": [ + { + "id": "agent:as0036-empty-brief", + "severity": "minor", + "message": "app.json brief is empty. AppSource validation rejects an empty brief under AS0036. Set brief to a one-sentence summary of 100 characters or fewer. Impact would normally be a blocker because it is a hard AppSource rejection; emitted as minor because no curated rule backs it. This concern should be promoted to a knowledge-backed rule before it can gate.", + "location": { + "file": "app.json", + "line": 9 + }, + "references": [], + "confidence": "medium", + "suggested-code": " \"brief\": \"Stage-and-forward integration for warehouse shipments.\"," + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-code-quality-reviewer.md b/custom/skills/review/al-code-quality-reviewer.md new file mode 100644 index 0000000..5b7f0e5 --- /dev/null +++ b/custom/skills/review/al-code-quality-reviewer.md @@ -0,0 +1,102 @@ +--- +kind: action-skill +id: al-code-quality-reviewer +version: 1 +title: AL code quality review +description: Reviews AL source changes for design quality, testability, and structural anti-patterns, and emits a findings report. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL code quality review + +Reviews Business Central production AL for design and structural problems that hurt the codebase later: direct database access from logic codeunits (the IDataAccess rule), business logic on table or page triggers, untestable seams, swallowed errors, excessive coupling and nesting, and thick event subscribers. This skill is about whether the code is well-designed and testable, not about clarity to a fresh reader (that is `al-readability-checker`) or test quality (that is the test validators). Most of what it surfaces is design judgement the curated corpus does not encode, so the bulk of its output is agent findings within a design quality remit, with curated `performance` and `security` rules cited where a structural defect maps onto one. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `performance` or `security` as the citable candidate set across every enabled layer: a structural defect such as a commit inside a loop, a redundant Get, or an integration event that leaks a secret maps onto a curated rule and MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The design quality concerns this skill owns (IDataAccess routing, testability seams, coupling, error-handling robustness, subscriber discipline) are mostly not covered by the corpus; for a concrete, demonstrable defect there, emit an agent finding within this skill's design quality domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the changed production AL (exclude test objects) and the structural shapes this skill audits: + +- Logic codeunits that call `Get`, `Find*`, `SetRange`, `Insert`, `Modify`, `Delete` directly on a record rather than routing through the project's `IDataAccess` interface or its implementation. +- Table and page objects whose triggers carry non-trivial validation, calculation, or posting logic instead of delegating to a management codeunit. +- Public procedures whose `Record` parameter cannot be exercised with a temporary record, and procedures that read ambient state (`UserId`, `WorkDate`, `CompanyName`, `Session`) with no override seam. +- Procedures with high cyclomatic complexity, length over roughly 80 lines, fan-out over ten codeunits, or nesting at five levels or deeper. +- Swallowed errors (`if not Codeunit.Run() then exit` with no handling), empty `Error('')`, and `Commit` inside a loop or without a documented reason. +- Event subscriber codeunits that mix unrelated subscriptions, hold inline business logic, omit early exit on temporary or wrong record type, or leave `EventSubscriberInstance` unset on a non-trivial subscriber. + +A curated `performance` or `security` file enters the worklist when its `keywords` intersect these tokens (for example `commit`, `loop`, `get`, `integrationevent`, `secret`). Read its full `## Best Practice` / `## Anti Pattern` body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklisted shape, evaluate the diff and emit findings. + +When a defect maps onto a curated `performance` or `security` knowledge file (for example a `Commit` inside an iteration, a redundant `Get` on an already-loaded record, or an integration event exposing a secret), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous pattern match. + +When a concrete, demonstrable design quality defect has no curated rule (a logic codeunit reaching the database directly instead of through IDataAccess, a public procedure with no test seam, a swallowed error, a thick subscriber doing inline business logic), emit an agent finding within this skill's design quality domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:direct-db-access-from-logic`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` describing both the defect and a concrete fix. Where the underlying impact would normally gate (a direct-DB-access violation the team treats as a block), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every agent candidate to the precision bar in `skills/do.md`: steelman that the shape is a deliberate, valid choice before emitting, never emit stylistic or speculative concerns, and omit when in doubt. Defects outside design quality (pure readability, pure performance the corpus already covers) belong to other skills and MUST NOT be emitted here. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is mechanical (deleting a swallowed-error guard, moving a `Commit` out of a loop, replacing `Error('')` with a Label-backed call); otherwise set `suggested-code-omission-reason` (for example `requires introducing an IDataAccess implementation`). Group repeated instances of one concern into a single finding with a line range rather than many near-identical ones. + +Outcome selection: `completed` when every worklist item was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the diff has no production AL to review; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-code-quality-reviewer", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 4, "items-evaluated": 4 } + }, + "findings": [ + { + "id": "microsoft/knowledge/performance/avoid-commit-inside-loops.md", + "severity": "major", + "message": "Commit() is called inside a repeat..until loop in EventPostingMgt. Move the commit outside the loop or split the work so the transaction boundary is not broken per row.", + "location": { + "file": "src/EventPostingMgt.Codeunit.al", + "line": 88 + }, + "references": [ + { "path": "microsoft/knowledge/performance/avoid-commit-inside-loops.md" } + ], + "confidence": "high" + }, + { + "id": "agent:direct-db-access-from-logic", + "severity": "minor", + "message": "EventRegistrationMgt.ReleaseRegistration calls Record.Get on Event Registration directly from a logic codeunit, bypassing the project's IDataAccess seam. This couples business logic to the data layer and blocks unit testing with a temporary record. Route the read through the IDataAccess implementation. Impact would normally be major in this codebase; emitted as minor because no curated rule backs it. This concern should be promoted to a knowledge-backed rule before it can gate.", + "location": { + "file": "src/EventRegistrationMgt.Codeunit.al", + "line": 42 + }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "requires routing through the project's IDataAccess implementation" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-event-subscriber-auditor.md b/custom/skills/review/al-event-subscriber-auditor.md new file mode 100644 index 0000000..c3de887 --- /dev/null +++ b/custom/skills/review/al-event-subscriber-auditor.md @@ -0,0 +1,87 @@ +--- +kind: action-skill +id: al-event-subscriber-auditor +version: 1 +title: AL event subscriber audit +description: Audits AL event subscribers for publisher existence, signature match, IsHandled contract, and thin-handler discipline, and emits a findings report. +inputs: [pr-diff, file-path, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL event subscriber audit + +Verifies that every `[EventSubscriber]` in a Business Central extension is wired correctly: the targeted publisher still exists, the signature matches the publisher parameter for parameter, the `IsHandled` contract is honoured, and the handler is thin enough to belong in a subscriber. The failure mode is silent: a typo'd publisher name or a `var` mismatch never fires and never errors at compile time on older event shapes. This skill sources from the `style`, `performance`, and `security` domains and cites curated rules where a subscriber concern maps onto one (subscriber parameter naming, guarding subscribers before a database call, integration events that leak secrets); the binding-correctness checks are mostly agent findings within its event-subscriber domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff`, a `file-path`, or a `repository`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `style`, `performance`, or `security` as the citable candidate set across every enabled layer: subscriber parameter names that must match the publisher, guarding an event subscriber before a database call, and an integration event that must not expose secrets each map onto a curated rule and MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The binding-correctness checks (publisher existence, exact signature match, the `IsHandled` flow, `BindSubscription` for `Manual` instances) are mostly not encoded in the corpus; for those concrete defects, emit an agent finding within this skill's event-subscriber domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the procedures decorated with `[EventSubscriber]` and their immediate radius: + +- The targeted publisher for each subscriber, resolved from `.alpackages` symbols when the publisher is in a dependency or from the extension's own source when internal. Flag a publisher that does not resolve (typo or removed event), a signature that does not match the publisher parameter for parameter (name, type, var-ness, order), and a missing `var` on a parameter the publisher passes by var. +- The `IsHandled` flow: a subscriber that ignores `var IsHandled: Boolean` on a first-handler-wins event, or sets it true without honouring the contract; a subscriber to `OnBeforeValidateEvent` that sets `IsHandled := true` without replicating the base-app validation. +- `OnRun` subscriptions (almost always a mistake), missing `Element` on a control-event binding, and `EventSubscriberInstance = Manual` subscribers with no `BindSubscription` in the call path. +- Handler thickness: subscriber bodies with more than roughly 25 lines of inline business logic that belong in a delegated codeunit; subscribers that call `Commit`, `Confirm`, or `HttpClient` inside a posting hot path. + +A curated `style`, `performance`, or `security` file enters the worklist when its `keywords` intersect these tokens (for example `event-subscriber`, `publisher`, `guard`, `integrationevent`, `secret`). Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklisted subscriber, emit findings. + +When a defect maps onto a curated file (subscriber parameter names that diverge from the publisher, a subscriber that hits the database with no early guard, an integration event payload that leaks a secret), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous match. + +When a concrete binding-correctness defect has no curated rule (a publisher that does not resolve, a `var` mismatch that silently drops mutations, a broken `IsHandled` flow, a `Manual` subscriber nobody binds, a thick handler with inline business logic), emit an agent finding within this skill's event-subscriber domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:subscriber-signature-drift`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` describing why the subscriber will not fire or will misbehave at runtime and the concrete fix. Where the impact would normally gate (a signature drift that silently breaks `IsHandled`), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the loose signature or unbound manual instance is intentional and resolved by code outside the diff before emitting, and omit when in doubt. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is mechanical (adding a missing `var` to a parameter, adding an early-exit guard); otherwise set `suggested-code-omission-reason` (for example `requires the publisher's exact signature from the dependency symbols`). + +Outcome selection: `completed` when every subscriber was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the task has no event subscribers to audit; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-event-subscriber-auditor", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 12, "items-evaluated": 12 } + }, + "findings": [ + { + "id": "agent:subscriber-signature-drift", + "severity": "minor", + "message": "Posting Subscribers.HandleAfterFinalize binds to Sales-Post.OnAfterFinalizePosting but omits the var prefix on RecRef. The publisher passes RecRef by var, so without var the subscriber receives a copy and any mutation is dropped, breaking the IsHandled flow silently. Change RecRef: RecordRef to var RecRef: RecordRef to match the publisher. Impact would normally be major; emitted as minor because no curated rule backs it. This concern should be promoted to a knowledge-backed rule before it can gate.", + "location": { + "file": "src/PostingSubscribers.Codeunit.al", + "line": 18 + }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "requires the publisher's exact parameter list from the base app symbols" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-extended-review.md b/custom/skills/review/al-extended-review.md new file mode 100644 index 0000000..188d205 --- /dev/null +++ b/custom/skills/review/al-extended-review.md @@ -0,0 +1,102 @@ +--- +kind: action-skill +id: al-extended-review +version: 1 +title: AL extended review +description: Composes the custom-layer AL review leaves (multi-tenancy, permissions, events, obsolescence, integration, upgrade) that complement the platform al-code-review. +inputs: [pr-diff, file-path, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +sub-skills: + - custom/skills/review/al-multitenancy-reviewer.md + - custom/skills/review/al-permission-set-auditor.md + - custom/skills/review/al-event-subscriber-auditor.md + - custom/skills/review/al-obsolete-tracker.md + - custom/skills/review/al-integration-pattern-reviewer.md + - custom/skills/review/al-upgrade-checker.md +--- + +# AL extended review + +Composes the custom-layer AL review leaves that cover concerns the platform `microsoft/skills/review/al-code-review` does not: multi-tenant and cross-company safety, permission-set coverage, event-subscriber discipline, obsolescence hygiene, modern integration patterns, and upgrade-codeunit coverage. This is a super-skill: it does not evaluate knowledge files directly, it invokes its sub-skills and rolls up their findings-reports following the DO composition contract. + +Run this alongside the platform `al-code-review` (which covers performance, security, privacy, upgrade, style, and UI) for a full review surface. An orchestrator invokes this skill with a `pr-diff`, a `file-path`, or a `repository`, and receives one JSON document conforming to the DO output contract, extended with `sub-results` and, where applicable, `skipped-sub-skills`. + +## Source + +The sub-skills invoked are exactly those listed in frontmatter `sub-skills`: + +- `custom/skills/review/al-multitenancy-reviewer.md` +- `custom/skills/review/al-permission-set-auditor.md` +- `custom/skills/review/al-event-subscriber-auditor.md` +- `custom/skills/review/al-obsolete-tracker.md` +- `custom/skills/review/al-integration-pattern-reviewer.md` +- `custom/skills/review/al-upgrade-checker.md` + +Additional leaves are added by editing this list; the skill does not discover sub-skills implicitly. Composition is flat: every entry is a leaf skill, never another super-skill. + +## Relevance + +A sub-skill is relevant when the orchestrator has supplied inputs that satisfy the sub-skill's declared `inputs` and has not disabled it via configuration. Per the DO contract, this super-skill MUST NOT filter sub-skills by task content (it does not inspect the diff to guess whether a leaf will find anything). Each leaf decides its own task-level applicability and signals it by returning `outcome: "not-applicable"` or `outcome: "no-knowledge"`. Sub-skills failing the input or configuration check are not invoked and are recorded in `skipped-sub-skills` with `reason: "not-applicable"` or `reason: "configuration"`. + +## Worklist + +The worklist is the set of sub-skills judged relevant by the previous step. Every sub-skill in the worklist is invoked in the Action step; the rest go to `skipped-sub-skills`. + +## Action + +Invoke each worklisted sub-skill as its own discrete pass, one at a time, passing only the subset of inputs the sub-skill declares. Capture each sub-skill's complete findings-report verbatim into `sub-results`. For any sub-skill whose `outcome` is `failed`, do not copy its findings into the top-level `findings[]` or counts. Otherwise append each of its findings to the top-level `findings[]` with `from-sub-skill` set to the sub-skill's `skill.id`, prefixing slug `id` values (non-citation findings) with `:` to avoid collisions; citation-based findings keyed by repo-relative path are left unchanged. + +After every sub-skill has produced its sub-result, perform a super-skill self-review pass for cross-cutting concerns that no single leaf could surface (for example an obsolescence change that is also a cross-tenant data path, or an integration change that is also an upgrade-schema change). Validate each candidate against the knowledge the leaves already loaded: a matching knowledge file upgrades it to a knowledge-backed finding, a contradicting file suppresses it, otherwise emit it as a super-skill agent finding (`from-sub-skill: "agent"`, `references: []`, `id` slug prefixed `agent:`, `confidence` capped at `medium`, `severity` capped at `minor`, self-contained `message`). Hold every candidate to the precision bar in `skills/do.md`. Set `suggested-code` for mechanical fixes, otherwise `suggested-code-omission-reason`. + +Derive `outcome` using the DO rollup rules, aggregate `summary.counts` and `summary.coverage` across invoked non-failed sub-skills, and populate `outcome-reason` for `partial` and `failed`. The top-level `suppressed[]` stays empty; knowledge-file suppression is reported by each leaf inside its own `sub-results` entry. + +## Output + +Output conforms to the DO output contract, extended with `sub-results` (one complete findings-report per invoked sub-skill) and `skipped-sub-skills`. A representative shape: + +```json +{ + "skill": { "id": "al-extended-review", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 2, "info": 0 }, + "coverage": { "worklist-size": 6, "items-evaluated": 6 } + }, + "findings": [ + { + "id": "custom/knowledge/integration/never-call-external-services-from-posting.md", + "severity": "major", + "message": "HttpClient.Send is called from an OnAfterPostSalesDoc subscriber. Posting locks are held; stage the call on the Integration Message and let the Job Queue send it.", + "location": { "file": "src/Integration/PostHooks.Codeunit.al", "line": 42 }, + "references": [ + { "path": "custom/knowledge/integration/never-call-external-services-from-posting.md" } + ], + "confidence": "high", + "from-sub-skill": "al-integration-pattern-reviewer" + }, + { + "id": "al-permission-set-auditor:missing-object-in-permission-set", + "severity": "minor", + "message": "Table 50123 \"Shipment Buffer\" is defined by the extension but does not appear in any permission set. Add it before AppSource submission.", + "location": { "file": "src/Shipment/ShipmentBuffer.Table.al", "line": 1 }, + "references": [], + "confidence": "medium", + "from-sub-skill": "al-permission-set-auditor" + } + ], + "suppressed": [], + "sub-results": [ + { + "skill": { "id": "al-integration-pattern-reviewer", "version": 1 }, + "outcome": "completed", + "summary": { "counts": { "blocker": 0, "major": 1, "minor": 0, "info": 0 }, "coverage": { "worklist-size": 3, "items-evaluated": 3 } }, + "findings": [], + "suppressed": [] + } + ] +} +``` diff --git a/custom/skills/review/al-integration-pattern-reviewer.md b/custom/skills/review/al-integration-pattern-reviewer.md new file mode 100644 index 0000000..7fd68de --- /dev/null +++ b/custom/skills/review/al-integration-pattern-reviewer.md @@ -0,0 +1,104 @@ +--- +kind: action-skill +id: al-integration-pattern-reviewer +version: 1 +title: AL integration pattern review +description: Validates AL inbound, outbound, long-running, and manual integration code against the modern integration patterns, and emits a findings report. +inputs: [pr-diff, file-path, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL integration pattern review + +Validates Business Central integration code against the modern integration patterns. The single question is: when the external system is slow, down, or sends the same message twice, does this code stay correct? It audits staging via an Integration Message, inbound and outbound idempotency, polling framing records, Business Event versioning and payload safety, correlation propagation, staged pipelines, and the hard anti-patterns (HTTP calls from posting, synchronous wait loops in API handlers, inline posting from a webhook or poll handler). This skill is the executor for the `integration` knowledge domain: it cites the curated integration files by path for the concerns they cover, and supplements them with `performance` and `security` citations and agent findings. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff`, a `file-path`, or a `repository`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `integration`, `performance`, or `security` as the citable candidate set across every enabled layer. The `integration` domain is the primary source: cite `custom/knowledge/integration/stage-every-integration-message.md` for the staging requirement and decoupling, and `custom/knowledge/integration/never-call-external-services-from-posting.md` for the callout-from-posting anti-pattern, and any further integration files the index lists for idempotency, framing, business-events versioning, and correlation. The `performance` and `security` domains supply supporting citations: a commit inside a fetch or send loop, a user prompt inside a posting transaction, and an integration event that leaks a secret each map onto a curated rule. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where a concrete integration defect has no curated rule, emit an agent finding within this skill's integration domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the integration code paths under review: + +- Codeunits that call `HttpClient`, especially any reachable from a posting routine or a posting event subscriber (`OnAfterPostSalesDoc`, `OnAfterFinalizePosting`, and similar). +- `PageType = API` pages used as inbound staging endpoints, and any handler that posts or runs business logic inline rather than writing to staging. +- Job Queue codeunits that fetch from or push to an external system, weighted toward a missing framing record (last fetch, max window, lock), an unbounded "fetch all", a missing inbound idempotency lookup on External Reference plus Type, and a missing or non-deterministic outbound `Idempotency-Key`. +- `[BusinessEvent]` declarations: a per-version events codeunit, a versioned name, a stable minimal DTO payload (not the BC record, no secrets), and validation before firing. +- Correlation ID set once at the entry point and carried onto every staged message, event payload, and outbound header. +- Long-running flows: a 202 response parked Awaiting Reply with a status URL, retry count and last error stored on the message, and staged pipelines split behind an integration-stage interface dispatched from an extensible enum with no cross-stage global state. +- Pages and actions that let a human re-run a failed message. + +A curated `integration`, `performance`, or `security` file enters the worklist when its `keywords` intersect these tokens (for example `staging`, `posting`, `httpclient`, `idempotency`, `business-event`, `correlation`, `commit`, `secret`). Read its full `## Best Practice` / `## Anti Pattern` body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklisted code path, check it against the patterns and emit findings. + +When a defect maps onto a curated knowledge file, emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `confidence` `high` for an unambiguous match. Severity is `blocker` only when the file states a platform-level guarantee, otherwise `major`. The hard anti-patterns cite the integration corpus directly: an `HttpClient.Send` reachable from a posting routine or posting subscriber cites `custom/knowledge/integration/never-call-external-services-from-posting.md`; a webhook or poll handler that posts or runs business logic inline rather than staging cites `custom/knowledge/integration/stage-every-integration-message.md`. A commit inside a fetch or send loop, a user prompt inside a posting transaction, or an integration event that exposes a secret cite the matching `performance` or `security` file. + +When a concrete, demonstrable integration defect has no curated rule (a synchronous sleep-and-poll wait loop in an inbound API handler, a missing polling framing record or lock, a missing inbound idempotency lookup keyed on the source id, a missing or non-deterministic outbound idempotency key, a mutated published Business Event signature, a dropped correlation id, cross-stage global state), emit an agent finding within this skill's integration domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:missing-inbound-idempotency-check`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` describing the failure mode under a slow, down, or duplicating external system and a concrete fix. Where the impact would normally gate (a synchronous wait loop that ties up a handler), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the path is correct as written before emitting, and omit when in doubt. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is mechanical (adding a deterministic `Idempotency-Key` header from the Integration Message GUID, moving a `Commit` out of a loop); otherwise set `suggested-code-omission-reason` (for example `requires introducing a staging table and Job Queue sender`). + +Outcome selection: `completed` when every worklist item was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the diff has no integration code to review; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-integration-pattern-reviewer", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 3, "items-evaluated": 3 } + }, + "findings": [ + { + "id": "custom/knowledge/integration/never-call-external-services-from-posting.md", + "severity": "major", + "message": "HttpClient.Send is called from WMS Notifier.NotifyWMS, which runs in OnAfterPostSalesDoc. The posting transaction holds locks on the shipment while waiting on the WMS. Stage an Integration Message inside the posting hook and let the Job Queue send it.", + "location": { + "file": "src/WMSNotifier.Codeunit.al", + "line": 31 + }, + "references": [ + { "path": "custom/knowledge/integration/never-call-external-services-from-posting.md" } + ], + "confidence": "high" + }, + { + "id": "agent:missing-outbound-idempotency-key", + "severity": "minor", + "message": "The outbound POST to the WMS sets no Idempotency-Key header, so a retry after a timeout can create a duplicate shipment on the remote system. Set the header to the Integration Message GUID so it is identical on every retry. Impact would normally be major; emitted as minor because no curated rule backs it. This concern should be promoted to a knowledge-backed rule before it can gate.", + "location": { + "file": "src/WMSSender.Codeunit.al", + "line": 64, + "range": { "start-line": 64, "end-line": 68 } + }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "requires the Integration Message GUID variable in scope at the call site" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-major-release-readiness.md b/custom/skills/review/al-major-release-readiness.md new file mode 100644 index 0000000..c927330 --- /dev/null +++ b/custom/skills/review/al-major-release-readiness.md @@ -0,0 +1,88 @@ +--- +kind: action-skill +id: al-major-release-readiness +version: 1 +title: AL major release readiness review +description: Reviews a PR that bumps app.json application or platform versions against major-upgrade governance and the compatibility-testing gate, and emits a findings report. +inputs: [pr-diff, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL major release readiness review + +Governs Business Central major version bumps. A NextMajor branch is expensive (parallel maintenance), so the house rule delays it as long as the current Production version still compiles and runs cleanly. This skill reviews any PR that bumps `app.json` `application` or `platform` minimum versions, checks the compatibility-testing gate (compatibility testing is a check, not a commitment, and must not change the manifest versions), and surfaces the NextMajor branch decision. It sources from the `upgrade` knowledge domain and cites curated rules where a version-bump concern maps onto one (breaking changes only on tables without data, enum values additive at the end, no external calls in an upgrade codeunit); the governance gates the corpus does not encode are agent findings within its release-governance domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff` (the standard entry point for a version-bump PR) or a `repository`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `upgrade` as the citable candidate set across every enabled layer: breaking changes only on tables without data, enum values additive at the end, no external calls in an upgrade codeunit, and upgrade tags instead of version checks each map onto a curated rule and MUST cite it rather than be paraphrased, because a major bump is the moment those rules bite. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The governance gates (whether all customer environments are on the latest major before a NextMajor branch is cut, whether compatibility testing changed the manifest, whether the current Production version still compiles) are not encoded in the corpus; for those concrete defects, emit an agent finding within this skill's release-governance domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the PR branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the governance signals in the PR: + +- Changes to `app.json` `application` or `platform` minimum versions: whether the bump is a genuine commitment or compatibility testing that should not have touched the manifest. +- Whether the change could instead be made on the current Production version (the rule is to refactor on the current version first and branch NextMajor only when the change cannot work on the older version). +- The driving reason for a NextMajor branch when one is implied: a new API surface only on NextMajor, a Microsoft-required schema change, or a performance feature needed for a customer SLA; documented in the PR description. +- Schema and enum changes riding along with the bump that the `upgrade` corpus governs: a breaking change on a table that already holds data, an enum value inserted in the middle, an external call added to an upgrade codeunit. +- Deprecation-warning handling: warnings resolvable on the current version should be fixed forward, not deferred into a manifest bump. + +A curated `upgrade` file enters the worklist when its `keywords` intersect these tokens (for example `breaking-change`, `enum`, `upgrade`, `version`). Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each signal, emit a finding. + +When a defect maps onto a curated `upgrade` rule (a breaking change on a table with data, a non-additive enum change, an external call in an upgrade codeunit), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous match. + +When a governance defect has no curated rule (a manifest version bump made for compatibility testing rather than commitment, a change that could have stayed on the current version, a NextMajor branch implied with no documented driving feature, a deprecation warning deferred into a bump that was resolvable forward), emit an agent finding within this skill's release-governance domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:compatibility-test-changed-manifest`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` describing the governance gap and the concrete remedy (revert the manifest bump and keep compatibility testing as a check, document the driving feature, fix the warning forward). Where the impact would normally gate (premature NextMajor commitment that strands customers on older majors), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the bump is a deliberate, communicated decision before emitting, and omit when in doubt. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is a single contiguous manifest revert (restoring the prior `application` or `platform` value); otherwise set `suggested-code-omission-reason` (for example `requires a PR-description note documenting the driving feature`). + +Outcome selection: `completed` when every signal was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the PR does not bump the application or platform version; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-major-release-readiness", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "agent:compatibility-test-changed-manifest", + "severity": "minor", + "message": "This PR bumps app.json platform from 26.0 to 27.0 but the description frames it as compatibility testing for the next major. Compatibility testing is a check, not a commitment, and must not change the manifest versions, otherwise the build commits every tenant to the new platform. Revert the platform value and run compatibility testing against a Sandbox-NextMajor environment instead. This concern should be promoted to a knowledge-backed rule before it can gate.", + "location": { + "file": "app.json", + "line": 22 + }, + "references": [], + "confidence": "medium", + "suggested-code": " \"platform\": \"26.0.0.0\"," + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-multitenancy-reviewer.md b/custom/skills/review/al-multitenancy-reviewer.md new file mode 100644 index 0000000..e72e227 --- /dev/null +++ b/custom/skills/review/al-multitenancy-reviewer.md @@ -0,0 +1,58 @@ +--- +kind: action-skill +id: al-multitenancy-reviewer +version: 1 +title: AL multi-tenancy review +description: Audits AL data paths for cross-tenant and cross-company leak risk in SaaS, and emits a findings report. +inputs: [pr-diff, file-path, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL multi-tenancy review + +Audits Business Central AL (and any companion API in the same repository) for code paths that resolve data without correctly scoping by tenant and company. The failure mode is silent: a query that returns another tenant's data, an endpoint that resolves a record by primary key without scoping the caller's company, or a Job Queue codeunit that processes rows from the wrong company. These defects do not surface in single-tenant dev sandboxes; they appear in SaaS as a cross-tenant data leak. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff`, a `file-path`, or a `repository`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `security` or `integration` as the citable candidate set across every enabled layer; multi-tenant scoping is a security-and-integration concern, so a finding that matches a curated rule (for example, permission scoping or data-classification guidance) cites that file. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where no curated rule covers a concrete tenant-scoping defect, this skill emits an agent finding within its own domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the code paths where tenant or company scope is established or relied upon: + +- Codeunits runnable as a Job Queue entry (`TableNo = "Job Queue Entry"`) and any `[ServiceEnabled]` web-service codeunit. +- Procedures that call `CompanyName()`, `Company.Get`, set a `Company` filter, or resolve a record by `SystemId` across companies. +- Reads or writes to tables classified `CustomerContent` or `OrganizationIdentifiableInformation`, especially without a company filter. +- Outbound `HttpClient` calls to a companion API and any sibling API endpoints (`Endpoints/*.cs`, `routes/*.ts`) in the same repo. +- `Session.LogMessage` / Application Insights calls that omit a tenant custom dimension. + +A curated knowledge file enters the worklist when its `keywords` intersect these tokens. Read its full `## Best Practice` / `## Anti Pattern` body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklisted code path, check that tenant and company scope is established before data is resolved, that production paths never fall back to a default tenant, that cross-company writeback verifies the caller's company on both sides, that a company filter is present on every `CustomerContent` query, that `SystemId` resolution is company-scoped for per-company tables, that outbound calls carry tenant context the receiver validates, that any `Session.Companies` traversal is intentional and commented, and that logs carry the tenant id. + +When a defect matches a curated `security` or `integration` knowledge file, emit a knowledge-backed finding citing that file: `severity` up to `blocker` only when the file states a platform-level guarantee, otherwise `major`; `id` equal to the file path; `confidence` `high` for an unambiguous match. When no curated file covers a concrete, demonstrable tenant-scoping defect, emit an agent finding within this skill's domain: `references: []`, `id` slug prefixed `agent:`, `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` describing the leak path and a concrete fix (for example, "filter the pull query by the caller's company id"). Hold every agent candidate to the precision bar in `skills/do.md`: steelman that the cross-company traversal is intentional before emitting, and omit when in doubt. Set `suggested-code` when the fix is mechanical (adding a missing `SetRange(Company, ...)` or a company-id query parameter); otherwise set `suggested-code-omission-reason`. + +Outcome selection: `completed` when every worklist item was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the task has no AL data path to audit; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. Findings without a knowledge file are agent findings (`references: []`, `agent:` id, severity capped at `minor`); findings citing a `security` or `integration` file carry that file path as `id` and primary reference. diff --git a/custom/skills/review/al-obsolete-tracker.md b/custom/skills/review/al-obsolete-tracker.md new file mode 100644 index 0000000..090465e --- /dev/null +++ b/custom/skills/review/al-obsolete-tracker.md @@ -0,0 +1,89 @@ +--- +kind: action-skill +id: al-obsolete-tracker +version: 1 +title: AL obsolete marking audit +description: Audits Obsolete markings on AL objects for reason, tag, removal plan, and progress, and emits a findings report. +inputs: [object-list, repository, pr-diff] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL obsolete marking audit + +Walks the AL source for every `ObsoleteState` marking and reports whether each is well-formed and progressing toward removal. Every object, field, procedure, or enum value marked `ObsoleteState = Pending` should carry a clear `ObsoleteReason`, an `ObsoleteTag` encoding a target removal version, and a planned removal path. The skill flags orphans (Pending with no plan), broken removals (Removed with no prior Pending cycle), and abandoned obsolescence (Pending for several majors with no progress). It sources from the `upgrade` knowledge domain and cites curated obsoletion guidance where present; the hygiene checks the corpus does not encode are agent findings within its obsolescence domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with an `object-list`, a `repository`, or a `pr-diff`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `upgrade` as the citable candidate set across every enabled layer: obsoletion requires a reason and a tag, an enum value made obsolete must keep its ordinal with the new value appended, and the Pending-to-Removed staging cycle each map onto a curated rule and MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The progress and orphan checks (Pending extending beyond two majors, a Pending symbol with live internal callers, removal versions that do not converge) are mostly not encoded; for those concrete defects, emit an agent finding within this skill's obsolescence domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the obsolescence markings under review: + +- Every `ObsoleteState` property on tables, table extensions, fields, codeunits, procedures, pages, page extensions, enums, enum values, reports, queries, and xmlports in the supplied `object-list` or `repository`. When a `pr-diff` is the input, narrow to markings the diff adds or changes. +- `Pending` markings missing an `ObsoleteReason`, missing an `ObsoleteTag` with a removal version, or carrying a generic reason such as "Deprecated". +- `Removed` markings with no prior `Pending` cycle (read git history where available). +- `Pending` markings that have extended beyond roughly two majors, `Pending` symbols still called from inside the extension, `Pending` procedures on public codeunits whose reason names no replacement, obsoleted enum values whose ordinal moved, and removal versions across the extension that do not converge on a single harvest target. + +A curated `upgrade` file enters the worklist when its `keywords` intersect these tokens (for example `obsolete`, `obsoletion`, `enum`, `staging`, `removal`). Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each marking, emit a finding. + +When a defect maps onto a curated `upgrade` rule (a Pending with no reason or tag, an obsoleted enum value whose ordinal changed, a Removed that skipped Pending), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous match. + +When a concrete hygiene defect has no curated rule (Pending abandoned for several majors, a Pending symbol with live internal callers, a Pending procedure whose reason names no migration path, removal versions that do not converge), emit an agent finding within this skill's obsolescence domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:obsolete-pending-with-live-callers`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` describing the hygiene gap and a concrete fix (migrate the internal callers in this PR, commit to a removal version). Where the impact would normally gate, keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the long-lived Pending is a deliberate, documented deferral before emitting, and omit when in doubt. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is mechanical (adding an `ObsoleteTag = '2.0.0';` line next to an existing `ObsoleteReason`); otherwise set `suggested-code-omission-reason` (for example `requires migrating internal callers across multiple files`). + +Outcome selection: `completed` when every marking was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the task has no obsolescence markings to audit; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-obsolete-tracker", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 0, "info": 0 }, + "coverage": { "worklist-size": 8, "items-evaluated": 8 } + }, + "findings": [ + { + "id": "microsoft/knowledge/upgrade/obsoletion-requires-reason-and-tag.md", + "severity": "major", + "message": "Field 20 Old Reference No. on table Event Log has ObsoleteState = Pending and an ObsoleteReason but no ObsoleteTag, so no removal version is planned. Add an ObsoleteTag naming the next major in which the field is removed.", + "location": { + "file": "src/EventLog.Table.al", + "line": 54 + }, + "references": [ + { "path": "microsoft/knowledge/upgrade/obsoletion-requires-reason-and-tag.md" } + ], + "confidence": "high", + "suggested-code": " ObsoleteTag = '2.0.0';" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-performance-reviewer.md b/custom/skills/review/al-performance-reviewer.md new file mode 100644 index 0000000..d63e087 --- /dev/null +++ b/custom/skills/review/al-performance-reviewer.md @@ -0,0 +1,100 @@ +--- +kind: action-skill +id: al-performance-reviewer +version: 1 +title: AL performance anti-pattern review +description: Reviews AL for N+1 queries, missing keys, FlowField overuse on lists, missing SetLoadFields, and FindFirst on growing tables, and emits a findings report. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL performance anti-pattern review + +Reads AL with a single question: at ten times tenant scale, where does this fall over? It targets the patterns the AL compiler will not warn about but that compound badly under data scale: N+1 per-row sibling lookups and per-row `CalcFields`, a `SetCurrentKey` with no matching key, `FindFirst` on tables that grow without bound (ledger entries, document headers), FlowField columns on list-page repeaters, missing `SetLoadFields`, destructive `DeleteAll`/`ModifyAll` with no filter, heavy `OnAfterGetRecord`, HTTP calls inside a loop, and `Commit` inside iterations. This skill sources from the `performance` knowledge domain and cites those files where they match. It overlaps the platform `al-performance-review`, so its primary added value is agent findings for the scale patterns the corpus does not yet cover. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `performance` as the citable candidate set across every enabled layer: a `Get` inside a loop on a large table, `CalcSums` instead of `CalcFields` in a loop, applying filters before iterating, `SetLoadFields` for partial records, `IsEmpty` for existence checks, `Get` instead of `FindFirst` on a full primary key, and commit boundaries each map onto a curated rule and MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where a concrete scale defect has no curated rule (a FlowField column added to a heavily rendered list repeater, a `FindFirst` on a known unbounded ledger table with no tight filter, a missing `HasFilter` guard before a destructive iteration), emit an agent finding within this skill's performance domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow the relevant files to the subset that applies to the changes, computing overlap against: + +- Record-iteration code: `FindSet`/`FindFirst`/`FindLast` and `repeat..until` loops, weighted toward a per-row `Get`/`Find` on a sibling table (N+1) and a per-row `CalcFields`. +- List pages and their `SourceTable`, weighted toward FlowField columns on the repeater and heavy `OnAfterGetRecord` bodies. +- Keys defined on the table compared against the `SetCurrentKey` plus `SetRange`/`SetFilter` combinations the code actually uses; a `SetCurrentKey` with no matching key. +- `FindFirst` against tables that grow without bound (Item Ledger Entry, Value Entry, G/L Entry, Vendor Ledger Entry, Sales Header, Purchase Header) with no tight `SetRange`. +- Reads that use only a few fields and could `SetLoadFields`; `DeleteAll`/`ModifyAll` with no prior filter or `HasFilter` guard; `HttpClient.Send` or `Commit` inside an iteration. + +A curated `performance` file enters the worklist when its `keywords` intersect these tokens or its topic matches a changed object kind. Read its full `## Best Practice` / `## Anti Pattern` body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. When the diff clearly matches an anti-pattern (a `Get` inside a loop on a large table, a `CalcFields` per row, a `FindFirst` on a full primary key), emit a knowledge-backed finding citing the file: `id` equal to the file path, the file as primary reference, `severity` `blocker` only when the file states a platform-level guarantee otherwise `major`, `location` on the offending line or range, `confidence` `high` for an unambiguous match. When the diff contradicts a best practice without being a full anti-pattern, emit `minor` with the same reference shape. + +When a concrete scale defect has no curated rule, emit an agent finding within this skill's performance domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:flowfield-on-list-repeater` or `agent:findfirst-on-unbounded-table`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` that states the cost at scale (one extra query per visible row per render, a full scan on a table that only grows) and a concrete fix (move the FlowField to a factbox, add a `SetRange` that resolves via a key). Where the impact would normally gate (a destructive `DeleteAll` with no filter), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the table is small, the filter is set elsewhere, or the cost is documented and accepted before emitting, and omit when in doubt. The scope is strictly performance; defects outside this domain belong to other skills. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is mechanical (replacing `Count() > 0` with `not IsEmpty()`, adding a `SetLoadFields` before a `FindSet`, adding a `HasFilter` guard before a `DeleteAll`); otherwise set `suggested-code-omission-reason` (for example `requires choosing the right secondary key to add`). + +Outcome selection: `completed` when every worklist item was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the diff has no AL to review; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-performance-reviewer", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 3, "items-evaluated": 3 } + }, + "findings": [ + { + "id": "microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md", + "severity": "major", + "message": "Inside a FindSet over Vendor, Data Sync Mgt.SyncVendors calls Cache.Get for each row, firing one query per vendor. Pre-load the cache table into a Dictionary outside the loop and look up per row instead.", + "location": { + "file": "src/DataSyncMgt.Codeunit.al", + "line": 142, + "range": { "start-line": 142, "end-line": 146 } + }, + "references": [ + { "path": "microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md" } + ], + "confidence": "high" + }, + { + "id": "agent:flowfield-on-list-repeater", + "severity": "minor", + "message": "A FlowField column Total Amount was added to the Vendors list repeater, so every visible row triggers a CalcFields on each render. On a 50-row list that is 50 extra aggregate queries per render. Move the field to a factbox, which renders once per selected row, or document and accept the cost. This concern should be promoted to a knowledge-backed rule before it can gate.", + "location": { + "file": "src/Vendors.Page.al", + "line": 28 + }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "fix is a layout decision between factbox placement and accepted cost" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-permission-set-auditor.md b/custom/skills/review/al-permission-set-auditor.md new file mode 100644 index 0000000..0bd4597 --- /dev/null +++ b/custom/skills/review/al-permission-set-auditor.md @@ -0,0 +1,86 @@ +--- +kind: action-skill +id: al-permission-set-auditor +version: 1 +title: AL permission set audit +description: Audits that every object an AL extension defines appears in a permission set with appropriate scope, and emits a findings report. +inputs: [object-list, repository, pr-diff] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL permission set audit + +Compares the objects a Business Central AL extension defines against the entries in its permission set files and reports every gap. This catches the silent-but-fatal class of bug where a new table ships without permission, the install succeeds in the developer SUPER sandbox, and tenants hit "Permission denied" on first use. It is also the single most common cause of AppSource rejection (the AS0029 family). This skill sources from the `security` domain and cites curated permission-set guidance where present, otherwise it emits agent findings within its security and permissioning domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with an `object-list`, a `repository`, or a `pr-diff`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `security` as the citable candidate set across every enabled layer, weighted toward permission-set guidance: minimal-grant, avoiding wildcard grants, indirect and inherent permissions. A scope that grants more than the usage warrants maps onto a curated rule and MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The object-to-permission coverage gap itself (a defined object with no permission entry, a `table` line with no `tabledata` line) is not encoded in the corpus; for those concrete defects, emit an agent finding within this skill's security and permissioning domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Build the object-to-permission map and narrow to the gaps: + +- Walk the supplied `object-list` (or the source under `repository`) to build the set of `(ObjectType, ObjectName)` declarations. When a `pr-diff` is the input, narrow to the objects the diff adds or renames. +- Walk every `*.PermissionSet.al` the extension defines to build the set of granted entries. +- For each defined object, the gap set is: no matching permission entry anywhere; a `table` entry with no corresponding `tabledata` entry; a `tabledata` scope wider than the object's actual usage; a public codeunit with callable procedures and no entry, or an `Access = Internal` codeunit with a redundant entry; an orphan entry that matches no defined object; a permission set with an empty or non-meaningful `Caption`; a permission set whose own ID falls outside the `app.json` id range. + +A curated `security` permission-set file enters the worklist when its `keywords` intersect these tokens (for example `permission-set`, `tabledata`, `wildcard`, `minimal-grant`). Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each gap, emit a finding. + +When the gap is an over-broad or wildcard scope that maps onto a curated `security` rule (a `tabledata` grant wider than the usage, a wildcard grant), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous match. + +When the gap is a coverage defect with no curated rule (a defined object missing from every permission set, a `table` entry with no `tabledata` line, an orphan entry, a missing or noise `Caption`, a permission set out of the id range), emit an agent finding within this skill's security and permissioning domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:object-missing-from-permission-set` or `agent:table-without-tabledata`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the object, the permission set, and the exact entry to add. Where the impact would normally gate (a new table absent from every permission set, which AppSource rejects), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the object is intentionally not granted (a pure framework object behind `Access = Internal`) before emitting, and omit when in doubt. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is the exact permission line to add (for example `tabledata "Event Registration" = RIMD;`); otherwise set `suggested-code-omission-reason`. Group repeated instances of one concern into a single finding rather than many near-identical ones. + +Outcome selection: `completed` when every object was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the task has no objects or permission sets to compare; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-permission-set-auditor", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 47, "items-evaluated": 47 } + }, + "findings": [ + { + "id": "agent:table-without-tabledata", + "severity": "minor", + "message": "Table 50100 Event Registration is granted as a table entry in permission set MyExt All but has no tabledata entry, so the metadata is exposed but read and write are denied. Add the tabledata line beneath the existing table line. Impact would normally be major because AppSourceCop AS0029 rejects this; emitted as minor because no curated rule backs it. This concern should be promoted to a knowledge-backed rule before it can gate.", + "location": { + "file": "src/MyExt.PermissionSet.al", + "line": 12 + }, + "references": [], + "confidence": "medium", + "suggested-code": " tabledata \"Event Registration\" = RIMD;" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-readability-checker.md b/custom/skills/review/al-readability-checker.md new file mode 100644 index 0000000..94bb12d --- /dev/null +++ b/custom/skills/review/al-readability-checker.md @@ -0,0 +1,86 @@ +--- +kind: action-skill +id: al-readability-checker +version: 1 +title: AL readability review +description: Reviews AL source changes for readability to a fresh reader, covering naming, structure, labels, and comments, and emits a findings report. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL readability review + +Reviews Business Central AL for whether a human reviewer who has never seen the file can understand it without help: identifier clarity (PascalCase, verb-first procedures, no Hungarian notation, no opaque abbreviations), structure (one object per file, short procedures, shallow nesting, no magic numbers), labels and captions (every user-facing string through a `Label` with a translator `Comment`), and comments (no commented-out code, no untracked TODOs). This skill sources primarily from the `style` knowledge domain and cites a curated rule wherever a readability concern maps onto one; where no curated rule exists, it emits an agent finding within its style and readability domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `style` or `ui` as the citable candidate set across every enabled layer: the corpus encodes most naming, label, caption, layout, and keyword-casing rules, so a readability concern that maps onto one MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where a concrete readability defect has no curated rule (an opaque project abbreviation, commented-out code, a TODO with no work-item reference), emit an agent finding within this skill's style and readability domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow the relevant files to the subset that applies to the changes, computing overlap against: + +- Changed identifiers: object, variable, and procedure names, weighted toward Hungarian-prefixed names, single-letter variables outside trivial loops, non-verb-first procedure names, and abbreviations with no obvious BC meaning. +- Changed structure: procedures over roughly 80 lines, nesting at five levels or deeper, more than one object per file, and magic numbers or magic strings. +- Changed user-facing strings: `Label`, `Caption`, `ToolTip` declarations, weighted toward a missing translator `Comment`, an inline rather than object-scope `Label`, and a `Locked` flag missing on a non-translatable label. +- Changed comments: commented-out code, TODO or FIXME without a work-item reference, and public API or library procedures missing XML doc comments. + +A curated `style` or `ui` file enters the worklist when its `keywords` intersect these tokens or its topic matches a changed object kind. Read an article's full `## Best Practice` / `## Anti Pattern` body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. When the diff clearly matches an anti-pattern (a `Label` with no `Comment`, a lowercase-keyword violation, a missing `Caption` on a page field), emit a knowledge-backed finding citing the file: `id` equal to the file path, `severity` `major` when the file states a hard rule otherwise `minor`, `location` on the offending line or range, `confidence` `high` for an unambiguous match. When the diff contradicts a best practice without being a full anti-pattern, emit `minor` with the same reference shape. + +When a concrete readability defect has no curated rule (commented-out code, an opaque abbreviation that a fresh reader cannot decode, a TODO with no tracking reference), emit an agent finding within this skill's style and readability domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:opaque-abbreviation`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the issue and a concrete rename or removal. Hold every candidate to the precision bar in `skills/do.md`: as a dedicated style skill, readability preferences are inside this skill's domain, but still steelman that the choice is a deliberate, established convention before emitting, and omit when in doubt. Defects outside style and readability belong to other skills. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is mechanical (adding a `Comment` to a label, lowercasing a reserved keyword, moving an inline `Label` to object scope, deleting commented-out lines); otherwise set `suggested-code-omission-reason`. Group repeated instances of one concern into a single finding with a line range rather than many near-identical ones. + +Outcome selection: `completed` when every worklist item was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the diff has no AL to review; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-readability-checker", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 0, "info": 0 }, + "coverage": { "worklist-size": 3, "items-evaluated": 3 } + }, + "findings": [ + { + "id": "microsoft/knowledge/style/label-comment-explains-placeholders.md", + "severity": "major", + "message": "The Label declared on line 42 has no Comment attribute. Translators need a Comment that explains the placeholders so word order is preserved in other locales.", + "location": { + "file": "src/EventRegistrationMgt.Codeunit.al", + "line": 42 + }, + "references": [ + { "path": "microsoft/knowledge/style/label-comment-explains-placeholders.md" } + ], + "confidence": "high" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-table-refactorer.md b/custom/skills/review/al-table-refactorer.md new file mode 100644 index 0000000..1bcba72 --- /dev/null +++ b/custom/skills/review/al-table-refactorer.md @@ -0,0 +1,89 @@ +--- +kind: action-skill +id: al-table-refactorer +version: 1 +title: AL table refactor review +description: Reviews an AL table for clarity, performance, and house rules, extracting trigger logic and reorganising keys, and emits a findings report with refactored AL. +inputs: [file-path, object-list] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL table refactor review + +Reviews a Business Central AL table for clarity, performance, and project house rules, and proposes concrete refactorings: extracting non-trivial validation, calculation, and posting logic out of table triggers into a management codeunit, adding an `IDataAccess` seam where one is missing, reorganising fields and keys for a known access pattern, and tidying FlowFields and CalcFormulas. Refactoring must never change observable behaviour. Because the proposed changes are mechanical AL edits, this skill emits findings that carry the refactored AL in `suggested-code`. It sources from the `performance` and `style` knowledge domains and cites curated rules where a refactoring maps onto one; structural moves the corpus does not encode are agent findings within its design and structure domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `file-path` (the table object) or an `object-list`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `performance` or `style` as the citable candidate set across every enabled layer: a key that should align with the filters a caller uses, a FlowField source key that needs `SumIndexFields`, a `CalcFields` that belongs outside a hot loop, declaration order, captions, and object-scope labels each map onto a curated rule and MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The structural moves this skill owns (extracting trigger logic into a codeunit, adding an IDataAccess seam) are mostly not encoded; for those concrete defects, emit an agent finding within this skill's design and structure domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the table, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the refactoring opportunities in the table object: + +- Table triggers (`OnInsert`, `OnModify`, `OnValidate` of a field) holding non-trivial validation, calculation, or posting logic that belongs in a management codeunit, leaving the triggers as thin dispatchers. +- A missing `IDataAccess` implementation where the project's no-naive-data-access rule expects one. +- Field organisation: primary key first, then foreign keys, then descriptive fields, then computed and FlowFields, then audit fields; a missing secondary key for an obvious non-primary access pattern; a `FindFirst`-on-full-key pattern that should be a `Get`; an auto-increment field used as a primary key. +- FlowFields and CalcFormulas: aggregations callers re-derive that a `CalcFormula` could hold; `CalcFields` inside a hot loop; non-obvious formulas with no XML doc comment. +- House rules: object id inside the assigned range, user-facing labels via object-scope `Label` with a `Comment`, telemetry on protected operations, captions populated, one object per file. + +A curated `performance` or `style` file enters the worklist when its `keywords` intersect these tokens (for example `key`, `setcurrentkey`, `flowfield`, `calcfields`, `caption`, `label`). Read its full `## Best Practice` / `## Anti Pattern` body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each refactoring opportunity, emit a finding that carries the refactored AL. + +When the refactoring maps onto a curated `performance` or `style` rule (aligning a key with a filter pattern, adding `SumIndexFields` to a FlowField source key, moving a label to object scope, populating a caption), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous match. + +When the refactoring is a structural move with no curated rule (extracting validation from a trigger into a management codeunit, adding an IDataAccess seam, reordering fields, adding a secondary key), emit an agent finding within this skill's design and structure domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:extract-trigger-logic`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the move and confirming it preserves observable behaviour. Where the impact would normally gate, keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the current shape is deliberate before emitting, and omit when in doubt. + +Because this skill's job is to produce the refactored table, set `suggested-code` on every finding where the change is mechanical and contiguous: it carries the literal refactored AL for the lines indicated by `location` (the reorganised `keys` block, the rewritten thin trigger, the relocated `Label`). When a `.good.al` companion exists and the table matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the move spans non-contiguous code or a behaviour-preserving rewrite cannot be determined from the table alone (for example extracting logic that needs a new sibling codeunit file), and then set `suggested-code-omission-reason`. If a change might alter observable behaviour, lower confidence and say so in the `message` rather than emitting a confident replacement. + +Outcome selection: `completed` when every opportunity was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the input is not a table object; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-table-refactorer", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "agent:extract-trigger-logic", + "severity": "minor", + "message": "The OnValidate trigger of field Attendee Count holds the full attendee-cap validation inline. Extract it to Event Registration Mgt.ValidateAttendeeCount and call that from the trigger, leaving the trigger as a thin dispatcher. The extracted call preserves observable behaviour. This concern should be promoted to a knowledge-backed rule before it can gate.", + "location": { + "file": "src/EventRegistration.Table.al", + "line": 40, + "range": { "start-line": 40, "end-line": 41 } + }, + "references": [], + "confidence": "medium", + "suggested-code": " trigger OnValidate()\n begin\n EventRegistrationMgt.ValidateAttendeeCount(Rec);\n end;" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-translation-auditor.md b/custom/skills/review/al-translation-auditor.md new file mode 100644 index 0000000..92c6415 --- /dev/null +++ b/custom/skills/review/al-translation-auditor.md @@ -0,0 +1,81 @@ +--- +kind: action-skill +id: al-translation-auditor +version: 1 +title: AL translation coverage audit +description: Audits AL source strings against xliff files and the supported countries declared in AppSourceCop.json, and emits a findings report. +inputs: [repository, object-list] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL translation coverage audit + +Compares every `Label`, `Caption`, `ToolTip`, `Comment`, and `Description` string in the AL source against the xliff files under the translations folder and against the `supportedCountries` declared in `AppSourceCop.json`. It catches the silent failure where an extension claims to support a market but ships only en-US text, one of the top AppSource rejection reasons. The skill checks one xliff per supported locale, every source string present and translated in every xliff, empty and verbatim targets, orphan trans-units, translator comments on substituted labels, and truncation risk. It sources from the `style` knowledge domain and cites curated label and caption rules where present; coverage gaps the corpus does not encode are agent findings within its translation domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository` or an `object-list`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `style` as the citable candidate set across every enabled layer: a `Label` or `Caption` missing its translator `Comment`, and the `Comment` that must explain placeholders, each map onto a curated rule and MUST cite it rather than be paraphrased, because a missing comment is the root cause of the mistranslations this skill audits. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The coverage checks themselves (a missing xliff for a supported locale, an untranslated or empty target, an orphan trans-unit) are not encoded in the corpus; for those concrete defects, emit an agent finding within this skill's translation domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries (the `supportedCountries` from `AppSourceCop.json`), or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Build the string-to-translation map and narrow to the gaps: + +- Walk the AL source (or supplied `object-list`) for every `Label`, `Caption`, `ToolTip`, `Comment`, and `Description`. Walk every `*.xlf` or `*.xliff` under the translations folder. Read the `supportedCountries` array from `AppSourceCop.json`. +- Each `supportedCountries` locale with no matching xliff file; each source string with no `` in a non-default xliff or no `` element; empty `` elements; non-en-US targets identical to the source with no `state="needs-review"` or justifying note; orphan trans-units that match no source string; labels with `%1`/`%2` substitutions whose `Comment` is missing or did not round-trip into the xliff `note`; targets whose length exceeds the source caption's apparent `MaxLength`. + +A curated `style` file enters the worklist when its `keywords` intersect these tokens (for example `label`, `caption`, `comment`, `translation`, `placeholder`). Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each gap, emit a finding. + +When the gap maps onto a curated `style` rule (a substituted label with no explanatory `Comment`, a `Caption` missing its `Comment`), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous match. + +When the gap is a coverage defect with no curated rule (a missing xliff for a supported locale, an untranslated or empty target, a verbatim target, an orphan trans-unit, a truncation risk), emit an agent finding within this skill's translation domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:missing-xliff-for-supported-country`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the locale or string and the concrete remedy (generate the xliff, translate the target). Where the impact would normally gate (a missing locale that AppSource rejects), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the locale legitimately falls back to a sibling or that an identical target is correct for that string before emitting, and omit when in doubt. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` only when the fix is an exact, contiguous edit to an AL source string (adding a `Comment` attribute to a label); a missing or untranslated xliff is not a single-line source replacement, so set `suggested-code-omission-reason` (for example `requires generating and translating an xliff file`). + +Outcome selection: `completed` when every supported locale and source string was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the task has no source strings or translations to compare; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-translation-auditor", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "agent:missing-xliff-for-supported-country", + "severity": "minor", + "message": "AppSourceCop.json declares AU as a supported country but no Translations xliff for en-AU exists, so AU tenants see whatever the default xliff carries. Generate the en-AU xliff and translate it, or document an explicit fallback to en-NZ. Impact would normally be major because AppSource validation rejects this; emitted as minor because no curated rule backs it. This concern should be promoted to a knowledge-backed rule before it can gate.", + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "requires generating and translating a new xliff file" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/review/al-upgrade-checker.md b/custom/skills/review/al-upgrade-checker.md new file mode 100644 index 0000000..4d8d0a0 --- /dev/null +++ b/custom/skills/review/al-upgrade-checker.md @@ -0,0 +1,92 @@ +--- +kind: action-skill +id: al-upgrade-checker +version: 1 +title: AL upgrade coverage review +description: Verifies that schema changes in an AL extension are handled by an upgrade codeunit for existing-tenant data, and emits a findings report. +inputs: [pr-diff, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL upgrade coverage review + +Compares a schema diff against the extension's upgrade codeunit and reports every migration that is missing or broken. It catches the silent install failure: a clean install on a dev sandbox, then `Install-NAVApp` errors on production tenants because existing data is not migrated. The skill covers new required fields, fields with `InitValue`, field renames, type or length narrowing, obsoleted fields, enum value changes, primary key changes, per-company versus per-database scope, idempotency, and upgrade-tag registration. It sources from the `upgrade` knowledge domain and cites curated rules where a schema change maps onto one; gaps the corpus does not encode are agent findings within its upgrade domain. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff` (the standard PR-review entry point) or a `repository`. It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `upgrade` as the citable candidate set across every enabled layer: `InitValue` not updating existing rows, upgrade tags instead of version checks, `DataTransfer` for bulk init, enum values added at the end, guarding database reads, and no external calls in an upgrade codeunit each map onto a curated rule and MUST cite it rather than be paraphrased. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where a concrete migration gap has no curated rule (a per-company migration that touches a per-database surface, a non-idempotent migration, a wire-contract break that an upgrade codeunit alone cannot fix), emit an agent finding within this skill's upgrade domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Compute the schema diff and narrow to the changes that need a migration: + +- New required or non-nullable fields with no default, and new fields with an `InitValue` that downstream code assumes is populated on existing rows. +- Field renames (AL `Rename` moves only metadata, not data, and does not rewrite JSON keys that store the old field name). +- Field type or length narrowing (Code[20] to Code[10], Text[100] to Text[50], Decimal to Integer) where existing rows hold out-of-bound values. +- Obsoleted fields whose data the upgrade codeunit must migrate before the Removed cycle, and any `Obsolete Removed` with no prior `Pending`. +- Enum value reorders or renames that break serialised ordinals or AL identifier comparisons, and additions to a non-Extensible versus Extensible enum. +- Primary key changes that require existing rows to disambiguate. +- The upgrade codeunit itself: `OnUpgradePerCompany` versus `OnUpgradePerDatabase` scope, idempotency of each step, and a unique `UpgradeTag` registered per step so re-publishes skip applied work. +- Wire-contract impact when the schema change affects an exposed API shape. + +A curated `upgrade` file enters the worklist when its `keywords` intersect these tokens (for example `initvalue`, `upgrade-tag`, `datatransfer`, `enum`, `rename`, `obsolete`). Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each schema change, check the upgrade codeunit covers it and emit findings. + +When a gap maps onto a curated `upgrade` rule (a new field relying on `InitValue` for existing rows, a missing upgrade tag, an enum value added in the middle), emit a knowledge-backed finding citing that file: `id` equal to the file path, the file as primary reference, `severity` up to `blocker` only when the file states a platform-level guarantee otherwise `major`, `confidence` `high` for an unambiguous match. State the tenants affected in the `message`. + +When a concrete migration gap has no curated rule (a per-company step that writes a per-database surface and risks duplicate writes, a non-idempotent migration that appends rows on re-run, a primary-key change with no disambiguation, a wire-contract break needing an API version bump), emit an agent finding within this skill's upgrade domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:non-idempotent-upgrade-step`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` describing the failure on tenants with existing data and a concrete fix. Where the impact would normally gate (a missing migration that fails the install), keep `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a knowledge-backed rule before it can gate. Hold every candidate to the precision bar in `skills/do.md`: steelman that the migration is covered by a step outside the diff before emitting, and omit when in doubt. Before emitting any agent candidate, check the worklisted knowledge for a match and upgrade it to a knowledge-backed finding if one exists. + +Set `suggested-code` when the fix is mechanical (wrapping a step in `UpgradeTagMgt.HasUpgradeTag`/`SetUpgradeTag`); otherwise set `suggested-code-omission-reason` (for example `requires authoring a new upgrade step body`). + +Outcome selection: `completed` when every schema change was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the diff has no schema change to review; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-upgrade-checker", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 0, "info": 0 }, + "coverage": { "worklist-size": 3, "items-evaluated": 3 } + }, + "findings": [ + { + "id": "microsoft/knowledge/upgrade/initvalue-does-not-update-existing-rows.md", + "severity": "major", + "message": "A required enum field Status Code was added to table Event Registration with no upgrade step. InitValue applies to new rows only, so every existing row on every tenant is left empty. Add a step to OnUpgradePerCompany that sets Status Code to Draft on existing rows, behind a fresh UpgradeTag.", + "location": { + "file": "src/EventRegistration.Table.al", + "line": 30 + }, + "references": [ + { "path": "microsoft/knowledge/upgrade/initvalue-does-not-update-existing-rows.md" } + ], + "confidence": "high" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/ai-test-driven-development.md b/custom/skills/testing/ai-test-driven-development.md new file mode 100644 index 0000000..fd2bd69 --- /dev/null +++ b/custom/skills/testing/ai-test-driven-development.md @@ -0,0 +1,90 @@ +--- +kind: action-skill +id: ai-test-driven-development +version: 1 +title: AI test-driven development +description: TDD for Copilot features and custom agents covering Evaluation suites, JSONL/YAML datasets, AITest codeunits, agent turn loops, intervention validation, and credit tracking. +inputs: [repository, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AI test-driven development + +Drives test-driven development for Business Central Copilot features and custom agents using the Evaluation suite (the data-driven tool where datasets describe inputs and expected outputs and the test codeunit drives the loop). It covers two flows: prompt-based AI tests for PromptDialog features (JSONL or YAML datasets) and multi-turn agent accuracy tests with intervention validation (YAML only). This is a generator-style skill: it generates AITest codeunits, dataset scaffolds, and suite XML as findings whose `suggested-code` carries the artifact, and it also reviews an existing AI test setup for misconfiguration. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository` (the Copilot or agent extension and its test app) and a `file-path` (the capability, PromptDialog, or agent under test, or an existing AI test codeunit or dataset to review). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing` as the citable candidate set across every enabled layer: AITest-codeunit attribute rules, isolation rules (`TestType = AITest`, `RequiredTestIsolation = Disabled` for agent tests), suite-setup discipline, intervention-contract rules, and credit-tracking guidance back the findings here. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Generated artifacts and configuration observations with no curated backing are agent findings within this skill's domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the repository `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown` (the suite XML may enable multilingual evaluation, so countries can matter). +- `application-area`: the application areas of the Copilot or agent feature, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the AI test work the task requires: + +- The flow: prompt-based AI test (PromptDialog plus an Azure OpenAI call, JSONL or YAML dataset) or agent accuracy test (multi-turn, YAML only). +- The AITest codeunit: `Subtype = Test`, `TestType = AITest`, `TestPermissions = Disabled`, and for agent tests `RequiredTestIsolation = Disabled` (essential, because agent tasks run in a different session and span transactions). +- The turn loop for agent tests: the `repeat ... until` delegating to `Library - Agent` (`RunTurnAndWait`, `FinalizeTurn`), with validators returning `false` and a populated `ErrorReason` rather than calling `Error()`. +- The dataset: `test_setup` and `expected_data` keys for AI tests; `turns:` with `query`/`expected_data` for agent tests; the `intervention_request` sub-key the framework reads automatically (both directions: a declared intervention must pause with matching type and suggestions, an undeclared one must not pause); `$DateFormula-<...>$` placeholders, always quoted. +- The suite XML: `TestRunnerId="130451"` (Isolation-Disabled runner, required for agent tests), `TestType="Agent"` versus `"AITest"`, `` children, and the install-time dataset load. +- Suite-setup discipline: `AITTestContext.IsSuiteSetupDone()` is sticky; re-running setup needs the Reset Suite Setup action. +- Credit and permission constraints: Evaluation runs consume Copilot credits (tracked per suite, per line, per entry; limited at environment and company level); users need the `AI TEST TOOLKIT` permission set. + +A curated `testing` file enters the worklist when its `keywords` intersect these tokens. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +When generating, emit one finding per generated artifact (the AITest codeunit, the dataset, the suite XML, the install codeunit), each carrying the artifact in `suggested-code` with a `message` stating what was generated and how to wire it. When reviewing an existing setup, emit a finding per defect: a missing `RequiredTestIsolation = Disabled` on an agent test, a wrong `TestRunnerId`, a validator calling `Error()` instead of returning `ErrorReason`, an unquoted date placeholder, an intervention contract the dataset does not exercise in both directions, or a sticky suite-setup that silently ignores edited setup YAML. Where a curated `testing` file states the rule, emit a knowledge-backed finding citing it: `id` equal to the file path, `severity` up to `major` (`blocker` only when the file states a platform-level guarantee, for example an isolation rule whose violation makes the agent runner unusable), `confidence` `high` for an unambiguous match. Where no curated file applies, emit an agent finding within this skill's domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:generated-aitest-codeunit`, `agent:missing-disabled-isolation`, `agent:unquoted-date-placeholder`), `confidence` capped at `medium`, `severity` capped at `minor`, self-contained `message`. Put generated AL, YAML, or XML in `suggested-code`; for a mechanical fix to an existing file (adding the isolation attribute, quoting a placeholder) also set `suggested-code`. Where the fix is not local (restructuring a turn loop), set `suggested-code-omission-reason`. Hold every agent finding to the precision bar in `skills/do.md`. + +Outcome selection: `completed` when the requested generation or review finished (including a clean review with empty `findings`); `not-applicable` when the repository has no Copilot capability, PromptDialog, or custom agent to test; `no-knowledge` when no curated knowledge survived and no agent finding was raised; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. Generated artifacts and configuration findings with no curated backing are agent findings (`references: []`, `agent:` id, severity capped at `minor`); findings citing a `testing` file carry that file path as `id` and primary reference. + +```json +{ + "skill": { "id": "ai-test-driven-development", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 3, "items-evaluated": 3 } + }, + "findings": [ + { + "id": "agent:missing-disabled-isolation", + "severity": "minor", + "message": "Agent accuracy codeunit 50202 sets TestType = AITest but not RequiredTestIsolation = Disabled. Impact is major: agent tasks run in a different session and span transactions, so the runner cannot enforce isolation and the suite fails to start. Add the attribute. Promote to a curated rule so it can gate.", + "location": { "file": "test/MyAgentAccuracyTest.Codeunit.al", "line": 4 }, + "references": [], + "confidence": "medium", + "suggested-code": " RequiredTestIsolation = Disabled;" + }, + { + "id": "agent:generated-agent-dataset", + "severity": "info", + "message": "Generated a YAML agent dataset with a turns chain and an intervention_request the FinalizeTurn contract enforces in both directions. Date values use quoted $DateFormula placeholders so the dataset does not drift against WorkDate. Ship it under the test app .resources/ folder and load it in an Install codeunit.", + "location": { "file": "test/.resources/datasets/MY-DATASET.yaml" }, + "references": [], + "confidence": "medium", + "suggested-code": "name: MY-DATASET\nsuite_setup: MY-AGENT\ntests:\n - turns:\n - query:\n message: \"Release all open sales orders for next week\"\n expected_data:\n orders_released: 2" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/al-test-coverage-enforcer.md b/custom/skills/testing/al-test-coverage-enforcer.md new file mode 100644 index 0000000..d1835a3 --- /dev/null +++ b/custom/skills/testing/al-test-coverage-enforcer.md @@ -0,0 +1,91 @@ +--- +kind: action-skill +id: al-test-coverage-enforcer +version: 1 +title: AL test coverage enforcer +description: Hard coverage gate, passing only when AL coverage meets the threshold and otherwise naming every uncovered path. +inputs: [pr-diff, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL test coverage enforcer + +Decides whether a Business Central change has enough AL test coverage to ship. Unlike `al-test-coverage-validator`, which reports, this skill gates: it passes (an empty `findings` array) only when every new or behaviour-changed production surface has at least one identifiable covering test, and otherwise emits a finding per uncovered path at gating severity. When in doubt it fails, because the cost of one extra test is low and the cost of an uncovered regression is high. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff` (the production change to gate) and a `repository` (so the test index and any coverage report can be read). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing` as the citable candidate set across every enabled layer: a curated rule about a coverage threshold or a mandatory regression test is the authoritative basis that lets this skill gate at `major` or `blocker`. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where the project threshold is a house default with no curated backing, see Action for how severity is handled. + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the repository `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the production surfaces the threshold applies to: + +- New public procedures (default threshold: every one must have at least one direct or indirect covering test, no exceptions). +- New event subscribers (must have a test that fires the publisher in a realistic context). +- New table triggers (`OnInsert`, `OnModify`, `OnDelete`, field `OnValidate`): each must have a covering test. +- Modified procedures with a behaviour change: an existing or new test must assert the new behaviour. A behaviour change whose existing tests still pass unchanged is itself a gap (the tests do not exercise the new behaviour). +- Bug fixes: must add a regression test that names the bug and fails without the fix. +- Pure refactors with no behaviour change: existing covering tests must still apply; no new test required. + +Compute the covering set from the test index (procedure to referencing tests) and any supplied coverage report. A curated `testing` file enters the worklist when its `keywords` intersect a coverage-threshold rule. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklisted surface, decide PASS or FAIL against the threshold. Emit a finding for every FAIL reason, each naming the specific procedure, subscriber, trigger, or bug fix. Where a curated `testing` knowledge file states the coverage requirement, emit a knowledge-backed finding citing it: `id` equal to the file path, `severity` `blocker` when the file states a platform-level guarantee, otherwise `major`; `confidence` `high` for an unambiguous gap. Where the requirement is the house default with no curated backing, the finding is an agent finding within this skill's domain (`references: []`, `id` slug prefixed `agent:` such as `agent:uncovered-new-public-procedure` or `agent:bug-fix-missing-regression-test`, `confidence` capped at `medium`). Per `skills/do.md`, an agent finding's `severity` is capped at `minor` even though this skill gates: keep the emitted severity at `minor`, state plainly in the `message` that the impact is gating (it blocks completion under the project threshold), and flag that the threshold should be promoted to a curated `testing` rule so the gate carries authoritative weight. The consuming orchestrator combines the threshold configuration with these findings to set the actual PASS/FAIL on the merge. The fix is a new test, so omit `suggested-code` and set `suggested-code-omission-reason` to `the gap is closed by adding a covering test`. + +When no coverage report is supplied, fall back to static analysis of the test codeunits (which procedures each test references directly) and state the limitation in an `info` finding. When in doubt about whether a surface is covered, prefer to emit the finding. + +Outcome selection: `completed` when every worklisted surface was decided (an empty `findings` array means PASS, the gate is satisfied); `not-applicable` when the diff has no new or behaviour-changed production surface (a pure refactor or doc-only change); `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. An empty `findings` array with `outcome: completed` is the PASS signal. Threshold findings with no curated backing are agent findings (`references: []`, `agent:` id, severity capped at `minor`, gating impact stated in the message); findings citing a `testing` file carry that file path as `id` and may gate at `major` or `blocker`. + +```json +{ + "skill": { "id": "al-test-coverage-enforcer", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 2, "info": 0 }, + "coverage": { "worklist-size": 3, "items-evaluated": 3 } + }, + "findings": [ + { + "id": "agent:uncovered-new-public-procedure", + "severity": "minor", + "message": "codeunit 50101 'Event Registration Mgt'.ReleaseRegistration is new in this diff and no test references it directly or indirectly. Impact is gating: under the project threshold this blocks completion. Promote the threshold to a curated testing rule so the gate carries authoritative weight.", + "location": { "file": "src/Sales/EventRegistrationMgt.Codeunit.al" }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "the gap is closed by adding a covering test" + }, + { + "id": "agent:bug-fix-missing-regression-test", + "severity": "minor", + "message": "The commit references work item #1234 (a fix) but no new test names the item or asserts the prior failure mode. Impact is gating: a bug fix must ship with a regression test that fails without the fix.", + "location": { "file": "src/Sales/EventRegistrationMgt.Codeunit.al" }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "the gap is closed by adding a covering test" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/al-test-coverage-validator.md b/custom/skills/testing/al-test-coverage-validator.md new file mode 100644 index 0000000..41737ad --- /dev/null +++ b/custom/skills/testing/al-test-coverage-validator.md @@ -0,0 +1,91 @@ +--- +kind: action-skill +id: al-test-coverage-validator +version: 1 +title: AL test coverage validator +description: Reports AL test coverage shape and identifies untested branches, error handlers, and edge cases without gating. +inputs: [pr-diff, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL test coverage validator + +Reports what a production AL change is and is not covered by, and how meaningfully. For each production procedure or trigger the diff touches, it classifies coverage as covered, shallow, uncovered, or not-applicable, and surfaces uncovered branches, error handlers, and edge cases. It reports; it does not gate. The hard PASS/FAIL decision lives in `al-test-coverage-enforcer`. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff` (the production change to assess) and a `repository` (so the test codeunit index and any coverage report can be read). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing` as the citable candidate set across every enabled layer; coverage is structural and rarely maps onto a curated rule, but a published `testing` rule about minimum coverage for a specific area would back a finding. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Most coverage observations are agent findings within this skill's domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the repository `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the changed objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the production surface the diff touches and the coverage signal for each: + +- Every production procedure and trigger (`OnInsert`, `OnValidate`, and so on) added or changed in the diff, excluding test files. +- For each, the test index: which test codeunits reference it directly or via a clear chain, and whether any asserts an outcome that depends on its body. +- Branch and error-handler coverage: uncovered `if`/`case` arms, uncovered `Error()` paths, uncovered `else` guards. +- Edge-case coverage where the procedure can encounter them: nulls, empty sets, max values, permission failures, date boundaries. +- Bug-fix regression coverage: when the diff message references a work item or fix, whether a test names it. +- Mutation survivors, when supplied: mutants that survived because no test caught the logic change. + +A curated `testing` file enters the worklist only when its `keywords` intersect a real coverage rule. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +Classify each worklisted procedure or trigger: `covered` (a test calls it and asserts an outcome exercising its body), `shallow` (a test calls it but asserts nothing depending on its return or side effects), `uncovered` (no test references it), or `n/a` (deleted or a pure pass-through). Emit a finding for every gap. These are agent findings within this skill's domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:uncovered-procedure`, `agent:shallow-coverage`, `agent:uncovered-branch`, `agent:missing-edge-case`), `confidence` capped at `medium`, `severity` capped at `minor` (this skill reports, it does not gate, so even a wholly uncovered new public procedure is `minor` here and the enforcer raises it). The `message` is self-contained: name the object and procedure, the coverage class, and the concrete gap (which branch, which edge case, which assertion is missing). Where a published `testing` rule genuinely backs a coverage requirement, upgrade that finding to knowledge-backed and cite the file. Hold every agent candidate to the precision bar in `skills/do.md`: a pass-through that genuinely needs no test is not a gap. The fix is a new or extended test rather than a local edit, so omit `suggested-code` and set `suggested-code-omission-reason` to `coverage gap is closed by adding a test, not a local code edit`. + +When the test index is missing, report only what static analysis of the diff yields (new procedure count, new trigger count) and state the limitation in an `info` finding. + +Outcome selection: `completed` when every touched procedure was classified (including an empty `findings` when coverage is complete); `not-applicable` when the diff has no production AL surface; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. Coverage gaps are agent findings (`references: []`, `agent:` id, severity capped at `minor`); the gating decision lives in `al-test-coverage-enforcer`. + +```json +{ + "skill": { "id": "al-test-coverage-validator", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 2, "info": 0 }, + "coverage": { "worklist-size": 8, "items-evaluated": 8 } + }, + "findings": [ + { + "id": "agent:uncovered-procedure", + "severity": "minor", + "message": "codeunit 50101 'Event Registration Mgt'.ReleaseRegistration is touched by the diff but no test codeunit references it directly or indirectly. Add a covering test. The enforcer will gate on this; here it is reported only.", + "location": { "file": "src/Sales/EventRegistrationMgt.Codeunit.al" }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "coverage gap is closed by adding a test, not a local code edit" + }, + { + "id": "agent:shallow-coverage", + "severity": "minor", + "message": "ValidateAttendeeCount is called by a test but the test asserts nothing depending on its outcome, so the resulting error path is not actually verified. Add an asserterror on the over-capacity case.", + "location": { "file": "src/Sales/EventRegistrationMgt.Codeunit.al" }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "coverage gap is closed by adding a test, not a local code edit" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/al-test-runner.md b/custom/skills/testing/al-test-runner.md new file mode 100644 index 0000000..c94d8a8 --- /dev/null +++ b/custom/skills/testing/al-test-runner.md @@ -0,0 +1,87 @@ +--- +kind: action-skill +id: al-test-runner +version: 1 +title: AL test runner +description: Executes AL test codeunits via the AL-Go runner or a local container and returns the run result as a findings report. +inputs: [repository, object-list] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL test runner + +Executes Business Central AL test codeunits and reports the run result in a shape the rest of the verifier chain can consume. It detects the project's runner (AL-Go pipeline, a Docker BC sandbox via BcContainerHelper, or a project-local build script), invokes it, parses the XUnit-style results, and maps each failure to a finding. It does not judge coverage (that is `al-test-coverage-validator` and `al-test-coverage-enforcer`), test quality (that is `al-test-validator`), or write tests (that is `al-test-writer`). This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository` (the project root to run in) and optionally an `object-list` (a filter narrowing the run to specific test codeunits). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing` or `pipelines` as the citable candidate set across every enabled layer: runner-selection, isolation, and AL-Go pipeline rules can back a finding about how the run was configured. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. A reported test failure or a runner-startup failure rarely maps onto a curated rule, so it is emitted as an agent finding within this skill's domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the repository `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the test objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the run to perform and the artifacts it produces: + +- Runner detection, in order: an AL-Go pipeline (`.AL-Go/settings.json` plus a `BuildALGoProject` script); a Docker BC sandbox via BcContainerHelper (a `BcContainerHelperVersion` setting or a `Run-TestsInBcContainer` call); a project-local `scripts/Build.ps1` or equivalent. +- The test codeunits to run: every test codeunit in the repository, narrowed by the `object-list` filter when supplied. +- The results file the run emits (`TestResults.xml` or equivalent) and the runner console output. + +A curated `testing` or `pipelines` file enters the worklist when its `keywords` intersect these tokens. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +Invoke the detected runner with the project's standard arguments, capture its output and the XUnit-style results file, and parse total, passed, failed, and skipped counts. Emit one finding per failed test: an agent finding (`references: []`, `id` slug prefixed `agent:` such as `agent:test-failed`, `confidence` capped at `medium`, `severity` capped at `minor`), with a self-contained `message` carrying the test codeunit, the test procedure, the assertion message, and the source location, and a `location` pointing at the failing line. Keep `severity` at `minor` even though a red test commonly blocks the chain, and say in the `message` that the run failed; the gating decision belongs to `al-test-coverage-enforcer` and the consuming orchestrator, not to this advisory channel. A run with many skipped tests emits an `info` finding naming the skip count. Where a curated `testing` or `pipelines` rule explains a misconfiguration the run surfaced (for example an isolation attribute that produced a spurious failure), upgrade that finding to knowledge-backed and cite the file. Mechanical fixes are rare here (the fix lives in the test or production code, not in the run), so omit `suggested-code` and set `suggested-code-omission-reason` to `fix lives in the test or production source under change`. + +If the runner cannot start (Docker daemon down, BC image missing, AL-Go misconfigured) or exceeds the configured timeout, do not silently succeed: emit `outcome: "failed"` with `outcome-reason` carrying the exact command attempted and the error output, and an agent finding describing the startup failure. + +Outcome selection: `completed` when the run finished and every failure was mapped to a finding (including a green run with empty `findings`); `not-applicable` when the repository has no AL test codeunit or no runner could be detected; `partial` when the run was cancelled on timeout after some tests ran (`summary.coverage` reflects the executed subset); `failed` when the runner could not start, with `outcome-reason` required. + +## Output + +Output conforms to the DO output contract. Test failures are agent findings (`references: []`, `agent:` id, severity capped at `minor`). + +```json +{ + "skill": { "id": "al-test-runner", "version": 1 }, + "outcome": "completed", + "outcome-reason": "al-go-pipeline runner, 27 tests, 1 failed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 1 }, + "coverage": { "worklist-size": 27, "items-evaluated": 27 } + }, + "findings": [ + { + "id": "agent:test-failed", + "severity": "minor", + "message": "Test ReleaseRegistrationShouldFailWhenOverCapacity in codeunit 50202 'Event Registration Tests' failed: expected error 'Capacity exceeded' but got 'Permission denied'. The run is red; fix the production code or the test before the chain can gate. Runner: al-go-pipeline.", + "location": { "file": "test/EventRegistrationTests.al", "line": 88 }, + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "fix lives in the test or production source under change" + }, + { + "id": "agent:tests-skipped", + "severity": "info", + "message": "1 test was skipped via an explicit Skip() call (codeunit 50202, SmokeTest). Skipped tests do not fail the run; confirm the skip is intentional.", + "references": [], + "confidence": "medium" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/al-test-validator.md b/custom/skills/testing/al-test-validator.md new file mode 100644 index 0000000..f907e3f --- /dev/null +++ b/custom/skills/testing/al-test-validator.md @@ -0,0 +1,87 @@ +--- +kind: action-skill +id: al-test-validator +version: 1 +title: AL test validator +description: Flags AL tests with no assertions, missing edge cases, weak names, poor isolation, and tests that exercise implementation not behaviour. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL test validator + +Reads AL test codeunits and reports whether each test is meaningful, well-named, and correctly configured. The failure modes are familiar: a `[Test]` with no `Assert.*`, a name that describes the call site instead of the expected behaviour, an isolation attribute set wrong for an AI test, a test that exercises implementation detail rather than behaviour, or a missing edge case the domain demands. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `pr-diff` (the changed test files) or a `file-path` (a single test codeunit). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing` as the citable candidate set across every enabled layer: test-attribute, isolation, transaction-model, and assertion-pattern rules are the authoritative basis for most findings here. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Project-specific conventions (a naming pattern, a project helper codeunit) rarely map onto a curated file, so a finding about them is an agent finding within this skill's domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the tested objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the test surface under review. Before raising findings, build a mental model of what the tests should assert: identify the business domain, the invariants and state transitions the system under test enforces, and the edge cases (empty sets, max values, null or blank fields, permission failures, date boundaries) the domain demands. Then compute overlap against: + +- Every `[Test]` procedure in the changed or supplied test codeunits, and whether each has at least one `Assert.*` or `asserterror` expectation. +- Codeunit configuration: `Subtype = Test`; for AI tests `TestType = AITest`, `TestPermissions = Disabled`, and for agent tests `RequiredTestIsolation = Disabled`; suite-setup guards via `AITTestContext.IsSuiteSetupDone()`. +- Test names: behaviour-describing (`ReleaseRegistrationShouldFailWhenOverCapacity`) versus implementation-named (`TestReleaseRegistration`) versus opaque (`TestProc01`). +- Anti-patterns: assertions inside an unguarded loop, a test calling `Commit()`, dependence on global state with no seed, deep mocking, a single test asserting multiple behaviours, undocumented `Sleep()`. +- Mutation survivors, when supplied: which test should have caught each survivor and which assertion it lacks. + +A curated `testing` file enters the worklist when its `keywords` intersect these tokens. Read its full `## Best Practice` / `## Anti Pattern` body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each worklisted test, evaluate it against the model and the worklisted knowledge. A missing assertion, a `Commit()` in a test, an assertion inside an unguarded loop, global-state dependence, or an isolation/transaction-model attribute wrong for the test type is a defect: when a curated `testing` file states the rule, emit a knowledge-backed finding citing it (`id` equal to the file path, `severity` up to `major`, `blocker` only when the file states a platform-level guarantee, `confidence` `high` for an unambiguous match). Implementation-named tests, single-test-multiple-behaviour, missing edge cases, and project-convention drift rarely map onto a curated file: emit them as agent findings within this skill's domain (`references: []`, `id` slug prefixed `agent:`, `confidence` capped at `medium`, `severity` capped at `minor`, self-contained `message`). When the underlying impact would otherwise be major (a missing assertion that lets a mutation survive), keep the emitted `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a curated rule before it can gate. Hold every agent candidate to the precision bar in `skills/do.md`: steelman that the test is deliberate (a no-throw contract is sometimes the real contract) before emitting, and omit when in doubt. Set `suggested-code` when the fix is mechanical (rename a test, add a `[HandlerFunctions(...)]` attribute, add the missing isolation attribute); otherwise set `suggested-code-omission-reason`. + +Outcome selection: `completed` when every worklisted test was evaluated (including an empty `findings`); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the task carries no AL test to validate; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. Findings without a knowledge file are agent findings (`references: []`, `agent:` id, severity capped at `minor`); findings citing a `testing` file carry that file path as `id` and primary reference. + +```json +{ + "skill": { "id": "al-test-validator", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 4, "items-evaluated": 4 } + }, + "findings": [ + { + "id": "microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md", + "severity": "major", + "message": "PostAndAssertNoChange posts a document but TransactionModel is unset, so posting rolls back at test end and the state-after-post assertion never observes a committed change.", + "location": { "file": "test/EventRegistrationTests.al", "line": 88 }, + "references": [ { "path": "microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md" } ], + "confidence": "high" + }, + { + "id": "agent:implementation-named-test", + "severity": "minor", + "message": "Test procedure TestReleaseRegistration names the call site, not the expected behaviour. Rename to describe the outcome, for example ReleaseRegistrationShouldEmitTelemetry, so a failure reads as a broken contract.", + "location": { "file": "test/EventRegistrationTests.al", "line": 40 }, + "references": [], + "confidence": "medium", + "suggested-code": " procedure ReleaseRegistrationShouldEmitTelemetry()" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/al-test-writer.md b/custom/skills/testing/al-test-writer.md new file mode 100644 index 0000000..951865c --- /dev/null +++ b/custom/skills/testing/al-test-writer.md @@ -0,0 +1,79 @@ +--- +kind: action-skill +id: al-test-writer +version: 1 +title: AL test writer +description: Generates AL test codeunits for a target production object as findings carrying the test source, the TDD red step. +inputs: [object-list, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL test writer + +Generates Business Central AL test codeunits that exercise a target production object and assert specific behaviours. This is a generator-style skill: it expresses each generated test codeunit as a finding whose `suggested-code` carries ready-to-drop AL, with the `## Action` step explaining what is generated. The contract is the TDD red step: the generated test must fail against the current production code and pass once the intended behaviour is implemented. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with an `object-list` (the target objects to cover) and a `file-path` (the production AL under test, plus the behaviour spec the caller supplies). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing` as the citable candidate set across every enabled layer: test-attribute, isolation, transaction-model, and assertion-pattern rules govern how the generated codeunit must be shaped, so a finding that matches a curated rule cites that file. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where no curated rule covers a concrete generation choice, this skill emits an agent finding within its own domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the branch `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the target objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the generation work the task actually requires: + +- One target per entry in `object-list`: a codeunit, table, page, or report whose behaviour the caller's spec describes. +- The behaviour spec: which inputs should produce which outcome, side effect, or error. If the spec is missing for a target, that target produces no test and is reported as `info` rather than a guessed behaviour. +- The project's existing test conventions (helper codeunits, fakes, naming pattern, assigned test object ID range) read from the supplied source. + +A curated `testing` knowledge file enters the worklist when its `keywords` intersect the tokens of the target (`Subtype = Test`, `TestType = AITest`, `RequiredTestIsolation`, `TestPermissions`, `HandlerFunctions`, `Library Assert`, `Commit`, isolation, transaction-model). Read its full `## Best Practice` / `## Anti Pattern` body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each target in the worklist, generate an AL test codeunit. Set the codeunit attributes for the test type: `Subtype = Test` with `TestPermissions = Disabled` for a regular AL test; add `TestType = AITest` for a Copilot prompt test; add `RequiredTestIsolation = Disabled` as well for an agent accuracy test. Every generated `[Test]` procedure carries a behaviour-describing name (the `Given_When_Then` or `Behaviour_Should_Outcome` pattern), self-seeding setup with no production-data dependence, an invocation of the target procedure or trigger, at least one `Assert.*` call validating the outcome, and `[HandlerFunctions(...)]` where a modal or confirmation is expected. The generated assertions must fail against the current production code (the TDD red contract); when that cannot be guaranteed for a target, say so plainly in the finding `message`. + +Emit one finding per generated test codeunit. Where a curated `testing` knowledge file backs the generation choice (for example an isolation or transaction-model rule the generated attributes satisfy), cite it: `id` equal to the file path, `references` carrying it, `confidence` `high` for an unambiguous match, `severity` `info` (a generator produces artifacts, not gating defects). Where no curated file applies, emit an agent finding: `references: []`, `id` slug prefixed `agent:` (for example `agent:generated-test-codeunit`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the target, the asserted behaviours, and the developer follow-up (implement the behaviour so the red test goes green). Put the generated AL in `suggested-code` since the artifact is mechanical; the `message` states what was generated and the TDD-red status. When a target lacks a behaviour spec, emit an `info` finding asking for the spec and omit `suggested-code` with `suggested-code-omission-reason` set to `behaviour spec missing for target`. Hold any agent finding to the precision bar in `skills/do.md`. + +Outcome selection: `completed` when every worklist target was processed (including when no AL could be generated for lack of a spec); `no-knowledge` when no curated knowledge survived and no agent finding was raised; `not-applicable` when the task supplies no AL target to cover; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-test-writer", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 1, "items-evaluated": 1 } + }, + "findings": [ + { + "id": "agent:generated-test-codeunit", + "severity": "minor", + "message": "Generated test codeunit 50202 'Event Registration Tests' covering ReleaseRegistration. Asserts 'Capacity exceeded' is raised when attendee count exceeds capacity. Fails on current code (red); implement the capacity validation in Event Registration Mgt.ReleaseRegistration to make it pass.", + "location": { "file": "test/EventRegistrationTests.al" }, + "references": [], + "confidence": "medium", + "suggested-code": "codeunit 50202 \"Event Registration Tests\"\n{\n Subtype = Test;\n TestPermissions = Disabled;\n\n [Test]\n procedure ReleaseRegistrationShouldFailWhenOverCapacity()\n begin\n // ... arrange, act, asserterror\n end;\n}" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/al-userguide-test-writer.md b/custom/skills/testing/al-userguide-test-writer.md new file mode 100644 index 0000000..5f027fe --- /dev/null +++ b/custom/skills/testing/al-userguide-test-writer.md @@ -0,0 +1,88 @@ +--- +kind: action-skill +id: al-userguide-test-writer +version: 1 +title: AL user-guide test writer +description: Maps each USER_GUIDE.md step to a BC page, action, and assertion and emits Subtype=Test TestPage codeunits as findings. +inputs: [file-path, repository] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL user-guide test writer + +Reads a Business Central extension's end-user walkthrough (typically `USER_GUIDE.md`), maps each documented step to a page plus action plus assertion, and generates `Subtype = Test` codeunits that script the flow with AL's TestPage library so the suite can run in a container. This is a generator-style skill: each generated test codeunit is a finding whose `suggested-code` carries the AL, and the `## Action` step explains what is generated. It writes tests; it does not run them (that is `al-test-runner`). This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `file-path` (the user-guide markdown) and a `repository` (so page object names, action names, field names, and the test app `idRanges` can be resolved from the source). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing` or `ux` as the citable candidate set across every enabled layer: TestPage patterns, isolation rules, handler-function conventions, and page-interaction guidance shape the generated codeunits, so a finding matching a curated rule cites it. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where no curated rule covers a generation choice or a step that cannot be mapped, this skill emits an agent finding within its own domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the repository `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the pages the guide drives, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the generation work the guide implies: + +- One test codeunit per top-level guide section (`"UserGuide §N __TST"`, N the section number, suffix matching the extension's mandatory suffix), living in the test app's `idRanges`. +- One `[Test]` procedure per documented step or substep, so a failure points at the specific step. +- Per procedure: the page to open (`OpenNew()` / `OpenEdit()`), the field writes (display captions resolved to AL field names from the source), the action invocations (`Invoke()` on the AL action name, not the caption), and the assertions on documented outcomes (`Assert.AreEqual` / `Assert.IsTrue` via `Codeunit "Library Assert"`). +- `[HandlerFunctions(...)]` where the guide implies a dialog or confirmation, with `asserterror` on negative paths. +- Seed data via `LibrarySales` / `LibraryPurchase` / `LibraryInventory` / `LibraryWarehouse` or the extension's own seed library; an `IsInitialized` guard and an `Initialize()` procedure per codeunit. +- A coverage map: which steps mapped to which procedures, and which sections could not be mapped (page does not exist, action unreachable from a TestPage, behaviour is server-side with no UI hook). + +A curated `testing` or `ux` file enters the worklist when its `keywords` intersect these tokens. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +For each mappable section, generate a TestPage codeunit as described in the worklist and emit one finding carrying it. Where a curated `testing` or `ux` file backs a generation choice (an isolation rule, a handler-function convention, a TestPage interaction pattern), cite it: `id` equal to the file path, `references` carrying it, `severity` `info`, `confidence` `high` for an unambiguous match. Where no curated file applies, emit an agent finding: `references: []`, `id` slug prefixed `agent:` (for example `agent:generated-userguide-test`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` naming the section, the steps covered, and the file path the codeunit should be written to. Put the generated AL in `suggested-code`. For a section that cannot be cleanly mapped, emit a separate agent finding (`id` slug `agent:unmappable-userguide-step`) naming the section, the step, why it could not map, and the recommended workaround (for example a parent-subpage navigation pattern, or a note that a server-side behaviour needs a non-UI test); omit `suggested-code` and set `suggested-code-omission-reason` to `step cannot be expressed through a TestPage primitive`. When the extension has no seed library, emit an `info` finding recommending one. Hold every agent finding to the precision bar in `skills/do.md`. + +Outcome selection: `completed` when every section was processed (mapped to a codeunit or reported as unmappable); `not-applicable` when the supplied path is not a user guide or no driven page exists in the repository; `no-knowledge` when no curated knowledge survived and no agent finding was raised; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. Generated codeunits with no curated backing are agent findings (`references: []`, `agent:` id, severity capped at `minor`, AL in `suggested-code`); findings citing a `testing` or `ux` file carry that file path as `id` and primary reference. + +```json +{ + "skill": { "id": "al-userguide-test-writer", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 2, "items-evaluated": 2 } + }, + "findings": [ + { + "id": "agent:generated-userguide-test", + "severity": "minor", + "message": "Generated TestPage codeunit for guide section '2. First-time setup', covering steps 2.1 Open Shipping Setup, 2.2 Review seeded reference data, 2.3 Set up Other Places. Write to src/UserGuide/SetupSection_TST.Codeunit.al in the test app id range.", + "location": { "file": "src/UserGuide/SetupSection_TST.Codeunit.al" }, + "references": [], + "confidence": "medium", + "suggested-code": "codeunit 60001 \"UserGuide §2 Setup_SHP_EQL_TST\"\n{\n Subtype = Test;\n TestPermissions = Disabled;\n // [Test] procedures per documented step\n}" + }, + { + "id": "agent:unmappable-userguide-step", + "severity": "minor", + "message": "Section 5.1 'Click Add to Container on the line ribbon' is a subpage line-level action. A TestPage cannot invoke it directly; use the parent page's TestPage and a SubPage child reference (Page_PurchaseOrder.PurchLines.\"Add to Container_SHP_EQL\".Invoke()). Confirm the parent-subpage navigation matches AL conventions.", + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "step cannot be expressed through a TestPage primitive" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/bc-extension-test-guide.md b/custom/skills/testing/bc-extension-test-guide.md new file mode 100644 index 0000000..2c283c7 --- /dev/null +++ b/custom/skills/testing/bc-extension-test-guide.md @@ -0,0 +1,93 @@ +--- +kind: action-skill +id: bc-extension-test-guide +version: 1 +title: BC extension test guide generator +description: Inventories every page, field, relation, enum, action, state machine, permission set, and telemetry event and emits a category-driven release-audit TEST_GUIDE.md. +inputs: [repository, object-list] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# BC extension test guide generator + +Produces an exhaustive `DOCS/TEST_GUIDE.md` for a Business Central AL extension. The guide is exhaustive by construction: every field, relation, action, and reachable data state in the AL source appears in at least one of twelve category inventories, so the state pivots happy-path testing misses are caught. This is a generator-style skill: the generated guide markdown is carried as a finding's `suggested-code`, and the `## Action` step explains what is generated. It produces the artifact; it does not run tests. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository` (the extension root with `app.json` and AL `src/`) and optionally an `object-list` (a scope narrowing the inventory). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing`, `ux`, `security`, or `telemetry` as the citable candidate set across every enabled layer: the twelve categories pull on lookup, type-conditional relation, visibility-refresh, state-machine, permission-boundary, cross-company isolation, and telemetry-event rules, so a category whose contract matches a curated rule cites it. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The generated guide itself, where no curated rule covers a category, is an agent finding within this skill's domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the repository `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the inventoried objects, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the full AL inventory the twelve categories require. Discover the repo (`app.json`, `.AL-Go/settings.json`, the primary app folder, the `src/**/*.al` file list), then build the inventory across these categories: + +1. Lookup audit: every page field whose underlying table field has a `TableRelation`, both directions. +2. Type-conditional `TableRelation`: every relation conditional on a sibling field, and each sibling enum or option value mapped to its target table. +3. Eligibility filters: every lookup whose downstream `OnValidate` or OK handler rejects a subset of the target table, and whether the lookup pre-filters it. +4. Visibility and Editable conditionals: every dynamic `Visible`/`Editable`, its driver, and whether the driver's `OnValidate` forces a `CurrPage.Update(false)` refresh. +5. StandardDialog Mode pivots: every `PageType = StandardDialog` with a Mode selector, the visible-field set, OK side effect, and error per Mode. +6. Subpage FK persistence: every `part(...)` with a `SubPageLink`, the FK propagated, and whether the explicit-push pattern or default-value behaviour alone is in use. +7. State machine transitions: every status enum, its full from-by-to matrix marking allowed and disallowed transitions, and every code path mutating the status. +8. Permission boundaries: every permission set with full RIMD per table and page or codeunit execute claims. +9. Telemetry events: every custom-event log call site with event id, trigger, and payload keys. +10. Mobile and tablet smoke: every top-level user-facing page. +11. Cross-company isolation: every table with its `DataPerCompany` value, singletons called out. +12. Upgrade paths: every upgrade codeunit and per-release schema delta with seed instructions. + +Scope the inventory by `object-list` when supplied. A curated `testing`, `ux`, `security`, or `telemetry` file enters the worklist when its `keywords` intersect a category's tokens. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +Generate `DOCS/TEST_GUIDE.md` with exactly the twelve categories in order, each carrying a definition, a procedure, and a populated inventory drawn from the actual AL (no placeholders). Run the self-audit pass: every `TableRelation` lands in category 1 or 2, every dynamic `Visible`/`Editable` in category 4, every `SubPageLink` in category 6, every status enum in category 7, every permission set in category 8, every telemetry call site in category 9; if an inventory has fewer rows than the AL warrants, the guide is incomplete and the missing rows are added. Emit one finding carrying the generated guide. Where a curated file backs a category contract (a state-machine rule, a cross-company isolation rule, a telemetry payload rule), cite it: `id` equal to the file path, `references` carrying it, `severity` `info`, `confidence` `high` for an unambiguous match. Where no curated file applies, emit an agent finding: `references: []`, `id` slug prefixed `agent:` (for example `agent:generated-test-guide`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` stating the guide was generated, the categories that genuinely do not apply (stated explicitly, not stubbed), and the path to write it to. Put the generated markdown in `suggested-code`. Emit an `info` finding recommending the cross-link in `USER_GUIDE.md`. Hold every agent finding to the precision bar in `skills/do.md`. + +Outcome selection: `completed` when the full inventory was built and the guide generated (including categories that do not apply, stated as such); `not-applicable` when the repository has no `app.json` and AL `src/`; `partial` when a token budget truncated the inventory (`summary.coverage` reflects the categories completed); `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. The generated guide with no curated backing is an agent finding (`references: []`, `agent:` id, severity capped at `minor`, markdown in `suggested-code`); findings citing a `testing`, `ux`, `security`, or `telemetry` file carry that file path as `id` and primary reference. + +```json +{ + "skill": { "id": "bc-extension-test-guide", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 1 }, + "coverage": { "worklist-size": 12, "items-evaluated": 12 } + }, + "findings": [ + { + "id": "agent:generated-test-guide", + "severity": "minor", + "message": "Generated DOCS/TEST_GUIDE.md with all 12 categories populated from src/ (47 TableRelation rows in cat 1/2, 3 status enums in cat 7, 2 permission sets in cat 8). Category 12 (Upgrade paths) is empty: the extension ships no upgrade codeunit, stated explicitly in the guide rather than stubbed.", + "location": { "file": "DOCS/TEST_GUIDE.md" }, + "references": [], + "confidence": "medium", + "suggested-code": "# , Test Guide\n\n## 0. Category index\n... (12 categories, each with definition, procedure, and inventory table) ..." + }, + { + "id": "agent:userguide-cross-link", + "severity": "info", + "message": "Add a cross-link in the USER_GUIDE.md header pointing to the new TEST_GUIDE.md so QA can find the audit companion.", + "references": [], + "confidence": "medium" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/bc-webclient-runner.md b/custom/skills/testing/bc-webclient-runner.md new file mode 100644 index 0000000..fdf1088 --- /dev/null +++ b/custom/skills/testing/bc-webclient-runner.md @@ -0,0 +1,93 @@ +--- +kind: action-skill +id: bc-webclient-runner +version: 1 +title: BC web client runner +description: Drives the rendered BC web client through a documented flow to catch UI residue AL TestPage cannot observe. +inputs: [repository, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# BC web client runner + +Drives a real Business Central web client through a documented user flow (typically `USER_GUIDE.md`), capturing screenshots and asserting on rendered UI state at every step. It catches the class of bug AL TestPage is structurally blind to: page layout, action enable and disable state, FactBox refresh timing, notification toasts, modal stacking, lookup usability, delayed-insert behaviour on subpages, and state-label drift between the guide and the enum. It executes scripted flows; it does not author AL. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository` (the extension source, so page and action names and the documented sandbox URL and company can be resolved) and a `file-path` (the user-guide markdown to walk). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `ux` or `testing` as the citable candidate set across every enabled layer: rendered-UI rules (delayed-insert, lookup usability, refresh-after-validate, state-label consistency) can back a finding the run surfaces. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. Where no curated rule covers an observed rendered-UI defect, this skill emits an agent finding within its own domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the repository `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the pages walked, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the flow to drive and the rendered checks per step. The environment must be non-production: refuse an on-prem host lacking `sandbox`, `dev`, `test`, or `staging`, and for a SaaS host on `businesscentral.dynamics.com` inspect the environment-name path segment and refuse if it matches `Production` or starts with `Prod`. Then build the worklist: + +- Each top-level guide section (or the supplied subset), and within it each documented step. +- Step-level state checks: read the documented outcome (status pill text, field value, subpage row count) after each action. +- Action availability: confirm a button is enabled or disabled exactly as the guide states, reading `aria-disabled` from the accessibility tree. +- Notification toasts: screenshot the toast region before auto-dismiss and read its content. +- FactBox totals: read the numbers and compare to the documented arithmetic. +- Lookup usability on every lookup-bearing field: open the lookup, confirm it lists records and a selection writes back. +- Delayed-insert behaviour on every editable subpage: type into the first non-PK field, tab off, and watch for an out-of-filter banner, a blank PK column, or the row falling out of the parent filter. +- Missing affordances the guide implies (a lookup drop-down a documented path needs). +- State-label drift: compare the displayed status value and enum dropdown values against the names the guide uses. +- Page-level errors: any `Error` notification, inline validation message, or console `ServerError`, captured even if the guide does not mention it. + +A curated `ux` or `testing` file enters the worklist when its `keywords` intersect these tokens. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +Drive the web client through each worklisted step, screenshot the result, and assert on the documented outcome. The skill requires a Chrome automation surface in the calling session; if it is unavailable, do not fall back to anything else: emit `outcome: "failed"` with `outcome-reason` stating the surface is missing. + +Emit a finding for every rendered-UI defect. Where a curated `ux` or `testing` file states the rule (for example a delayed-insert rule, a refresh-after-validate rule, or a state-label-consistency rule), emit a knowledge-backed finding citing it: `id` equal to the file path, `severity` up to `major`, `blocker` only when the file states a platform-level guarantee, `confidence` `high` for an unambiguous match. Where no curated file covers the observed defect, emit an agent finding within this skill's domain: `references: []`, `id` slug prefixed `agent:` (for example `agent:userguide-action-disabled`, `agent:subpage-missing-delayed-insert`, `agent:factbox-stale`, `agent:state-label-drift`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` carrying the section, the step, what was observed against what the guide promised, and the screenshot path. When the underlying impact would otherwise be major (a subpage missing `DelayedInsert = true` corrupting the parent FK, or state-label drift that breaks every downstream filter), keep the emitted `severity` at `minor` but say so plainly in the `message` and note the concern should be promoted to a curated rule before it can gate. Record the role the run used and the URL in the summary. Hold every agent finding to the precision bar in `skills/do.md`. The fix lives in AL, not in a renderable replacement, so omit `suggested-code` and set `suggested-code-omission-reason` to `fix is an AL change the developer applies after reading the report`. + +Outcome selection: `completed` when every attempted step was driven and asserted (including a clean run with empty `findings`); `not-applicable` when the supplied path is not a user guide or the repository drives no rendered page; `partial` when a block stopped the run mid-flow and not every section was attempted (`summary.coverage` reflects the attempted subset); `failed` when the Chrome surface was unavailable or the run could not start, with `outcome-reason` required. + +## Output + +Output conforms to the DO output contract. Rendered-UI defects with no curated backing are agent findings (`references: []`, `agent:` id, severity capped at `minor`); findings citing a `ux` or `testing` file carry that file path as `id` and primary reference. + +```json +{ + "skill": { "id": "bc-webclient-runner", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 2, "info": 0 }, + "coverage": { "worklist-size": 9, "items-evaluated": 9 } + }, + "findings": [ + { + "id": "agent:userguide-action-disabled", + "severity": "minor", + "message": "Section 3.2: Release on the Freight Movement card is disabled although the guide says it should be enabled once the header is filled (aria-disabled=true on the command-bar item). Screenshot: screenshots/section-3-step-2-release-disabled.png. Verify the action's Enabled expression against the header-filled state.", + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "fix is an AL change the developer applies after reading the report" + }, + { + "id": "agent:subpage-missing-delayed-insert", + "severity": "minor", + "message": "Section 5.1: typing into the line subpage then tabbing off shows an out-of-filter banner and a blank No. column, indicating the subpage is missing DelayedInsert = true. Impact is major: OnInsert fires before the number series assigns the PK, so the row persists with a blank or wrong parent FK. Promote to a curated rule before it can gate.", + "references": [], + "confidence": "medium", + "suggested-code-omission-reason": "fix is an AL change the developer applies after reading the report" + } + ], + "suppressed": [] +} +``` diff --git a/custom/skills/testing/page-scripting-e2e.md b/custom/skills/testing/page-scripting-e2e.md new file mode 100644 index 0000000..36ea180 --- /dev/null +++ b/custom/skills/testing/page-scripting-e2e.md @@ -0,0 +1,79 @@ +--- +kind: action-skill +id: page-scripting-e2e +version: 1 +title: Page Scripting e2e planner +description: Decides what belongs in Page Scripting versus an AL test and produces a deterministic recording plan wired into the bc-replay harness. +inputs: [repository, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Page Scripting e2e planner + +Builds a durable, repeatable browser-level test layer for a Business Central extension using BC's native Page Scripting (record and replay `.yml`) plus the `e2e-replay` (bc-replay) harness. It decides what belongs in Page Scripting versus an AL TestPage test, produces a deterministic recording plan a human follows, and wires the recordings so the whole set re-runs from one command. This is a generator-style skill: the recording plan markdown and the seed-factory scaffold are carried as findings' `suggested-code`, and the `## Action` step explains what is generated. This is a leaf action skill: it invokes no sub-skills. + +An orchestrator invokes this skill with a `repository` (the extension source and any green AL suite) and a `file-path` (the `DOCS/TEST_GUIDE.md` or `USER_GUIDE.md` whose residue the plan covers). It produces a single JSON document conforming to the DO output contract. + +## Source + +Read the BCQuality knowledge index once (the `knowledge-index.json` Entry's preparation step regenerates over the live, already-filtered clone). Take the index entries whose `domain` is `testing` or `ux` as the citable candidate set across every enabled layer: rules about what a rendered client must verify (notification toasts, cue rendering, visibility refresh, factbox refresh) and about deterministic seeding can back a plan decision. Do not open individual article files at this step; open an article's full body only once it enters the Worklist below. The generated plan and the layer-allocation decisions, where no curated rule applies, are agent findings within this skill's domain (see Action). + +## Relevance + +Apply the frontmatter matching rules defined in READ against the task context: + +- `bc-version`: the target BC version from the repository `app.json`, or `unknown` if unavailable. +- `technologies`: `[al]`. +- `countries`: the consuming app's declared countries, or `unknown`. +- `application-area`: the application areas of the flows recorded, or `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when configuration permits; findings derived from them have `confidence` no higher than `medium`, and the finding `message` names the unknown dimensions. + +## Worklist + +Narrow to the residue that needs a rendered client and the recordings that cover it, using the three-layer model: AL TestPage owns logic, state transitions, validation errors, action gates, proportional math, FlowField values, and permission RIMD; Page Scripting owns only what needs a rendered client; the manual checklist owns the irreducible (mobile, subjective look). Build the worklist: + +- For each TEST_GUIDE category or USER_GUIDE flow, decide the layer. Anything verifiable by reading a record or asserting a field after invoking a codeunit stays layer 1 and is excluded here. +- The residue items that need a rendered client: notification toasts firing, cue and tile rendering and Style, visibility and editability refresh after a field change, FactBox refresh on row change, dropdown and lookup population and filter-as-you-type narrowing, modal and dialog flow a user clicks through, real posting through standard codeunits driven from the UI. +- One small single-purpose recording per residue item (`E2E-NN .yml`, the `NN` prefix sorting play order). +- The deterministic precondition per recording: a Test Seed Factory codeunit gated behind an `Allow Test Data Seed` toggle that clears then seeds a fixed-prefix set, a reset No. Series so a recorded New yields a stable number, an `E2E-00 Clear and Seed.yml` head, and filter-as-you-type lookups that narrow to exactly one row. +- The harness wiring: replay via `e2e-replay/run.ps1` or the VS Code task, accounting for the known constraints (bc-replay cannot run from a path with a space, credentials come from env vars not interactive login, the recordings globber rejects `..`). + +A curated `testing` or `ux` file enters the worklist when its `keywords` intersect these tokens. Read its full body only after it makes the worklist. Resolve layer-precedence conflicts per READ and record dropped files in `suppressed`. + +## Action + +Generate `Page Scripting/E2E-PLAN.md`: a numbered chain of small recordings, each with the file name, the exact click-path the human follows while recording (Tell Me, page, field, value, action), the deterministic precondition, and the residue item it verifies. Generate the Test Seed Factory scaffold and the `E2E-00 Clear and Seed.yml` head as supporting artifacts. Emit one finding carrying the plan. Where a curated `testing` or `ux` file backs a residue allocation (a rule that a given behaviour needs a rendered client, or a determinism rule), cite it: `id` equal to the file path, `references` carrying it, `severity` `info`, `confidence` `high` for an unambiguous match. Where no curated file applies, emit an agent finding: `references: []`, `id` slug prefixed `agent:` (for example `agent:generated-e2e-plan`), `confidence` capped at `medium`, `severity` capped at `minor`, and a self-contained `message` listing the residue items, the recordings planned, and the path to write the plan to. Put the generated plan markdown in `suggested-code`. For any flow wrongly placed in layer 2 that an AL test could verify, emit a separate agent finding (`id` slug `agent:belongs-in-al-test`) recommending it stay in the TestPage suite; omit `suggested-code` and set `suggested-code-omission-reason` to `the recommendation is to keep the flow in the AL suite, not to generate a recording`. Hold every agent finding to the precision bar in `skills/do.md`. + +Outcome selection: `completed` when the residue was identified and the plan generated (including when all flows are already covered by layer 1 and no recording is needed); `not-applicable` when the supplied path is not a test or user guide, or the repository has no rendered page to record; `no-knowledge` when no curated knowledge survived and no agent finding was raised; `partial` or `failed` per the DO contract with `outcome-reason`. + +## Output + +Output conforms to the DO output contract. The generated plan with no curated backing is an agent finding (`references: []`, `agent:` id, severity capped at `minor`, markdown in `suggested-code`); findings citing a `testing` or `ux` file carry that file path as `id` and primary reference. + +```json +{ + "skill": { "id": "page-scripting-e2e", "version": 1 }, + "outcome": "completed", + "summary": { + "counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 }, + "coverage": { "worklist-size": 4, "items-evaluated": 4 } + }, + "findings": [ + { + "id": "agent:generated-e2e-plan", + "severity": "minor", + "message": "Generated Page Scripting/E2E-PLAN.md with 4 recordings for the rendered-UI residue the green AL suite cannot reach: notification toast on release, Attention cue turning red at zero, factbox refresh on row change, and the real posting flow driven from the UI. Each recording is anchored to the FE2E- seed prefix with a reset No. Series so replays are deterministic. Write the plan and the Test Seed Factory scaffold into the repo.", + "location": { "file": "Page Scripting/E2E-PLAN.md" }, + "references": [], + "confidence": "medium", + "suggested-code": "# E2E Page Scripting plan\n\n## E2E-00 Clear and Seed\n... numbered recording chain, each with click-path, precondition, and verified residue ..." + } + ], + "suppressed": [] +} +```