mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Add P0 integration and control add-in guidance
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02baffe8-0600-430d-81fa-a9993685e7cb
This commit is contained in:
parent
9214f73819
commit
cf55246ecf
16 changed files with 526 additions and 12 deletions
|
|
@ -16,7 +16,7 @@ application-area: [all]
|
|||
|
||||
Reviews AL page source and control add-in UI files against the `ui` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
|
||||
|
||||
UI findings apply to page files — files that declare `PageType = ...`, including `*.Page.al` under the standard file-naming convention — and to JavaScript/CSS/HTML files that render Business Central control add-ins. The skill returns `not-applicable` when the diff contains no page or control add-in UI changes.
|
||||
UI findings apply to page files — files that declare `PageType = ...`, including `*.Page.al` under the standard file-naming convention — and to JavaScript/CSS/HTML files that implement Business Central control add-ins, including their client-service communication. The skill returns `not-applicable` when the diff contains no page or control add-in changes.
|
||||
|
||||
An orchestrator invokes this skill with either a `pr-diff` or a `file-path`. The skill produces a single JSON document conforming to the DO output contract.
|
||||
|
||||
|
|
@ -39,9 +39,9 @@ Discard files that are not applicable. Retain conditionally applicable files onl
|
|||
|
||||
Narrow the relevant files to the subset that applies to the changes under review.
|
||||
|
||||
- **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`, and to control add-in JavaScript/CSS/HTML that changes rendered UI. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files.
|
||||
- For each relevant knowledge file, compute overlap against changed page declarations and control add-in UI files, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, `ShowCaption`, `InstructionalText`, `GridLayout`, `Style`, `StyleExpr`, action definitions, field-level properties, DOM creation, ARIA attributes, and keyboard/focus handlers.
|
||||
- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions).
|
||||
- **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`, and to JavaScript/CSS/HTML that implements a control add-in's rendering or Business Central communication. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files.
|
||||
- For each relevant knowledge file, compute overlap against changed page declarations and control add-in files, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, `ShowCaption`, `InstructionalText`, `GridLayout`, `Style`, `StyleExpr`, action definitions, field-level properties, DOM creation, ARIA attributes, keyboard/focus handlers, packaged-resource AJAX, and calls from JavaScript into AL.
|
||||
- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed page element. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
@ -53,6 +53,8 @@ When the post-conflict worklist is empty because no applicable UI knowledge exis
|
|||
|
||||
For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. UI text findings are generally `minor` — they affect localization and polish rather than correctness. Accessibility findings for missing labels, broken grid semantics, semantic color without text meaning, or UI-rendering control add-in changes can be `major`; use `minor` for low-risk manual-review reminders and polish issues.
|
||||
|
||||
For packaged-resource requests, flag `$.get` or AJAX/XHR that omits `withCredentials` only when the URL is identifiable as a resource in the control add-in package; do not generalize the rule to external endpoints. For `InvokeExtensibilityMethod`, flag repeated or timer-driven calls that can overlap because they do not wait for the success/error callbacks, and unbounded serialized payloads sent in one call. Prefer bounded chunks serialized through completion callbacks. Do not emit generic browser or JavaScript performance advice.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
- `high` when the detection is based on an unambiguous pattern match (banned term literal, missing "Specifies" opener on a field tooltip, caption exceeding documented limit).
|
||||
|
|
@ -69,7 +71,7 @@ Outcome selection:
|
|||
|
||||
- `completed` — the skill evaluated every worklist item.
|
||||
- `no-knowledge` — no applicable UI knowledge survived filtering.
|
||||
- `not-applicable` — the diff contains no page, pageextension, pagecustomization, or control add-in UI files.
|
||||
- `not-applicable` — the diff contains no page, pageextension, pagecustomization, or control add-in implementation files.
|
||||
- `partial` — a budget was hit before the worklist was exhausted.
|
||||
- `failed` — an unrecoverable error occurred.
|
||||
|
||||
|
|
|
|||
|
|
@ -3,11 +3,11 @@ kind: action-skill
|
|||
id: al-web-services-review
|
||||
version: 1
|
||||
title: AL web services review
|
||||
description: Reviews AL source changes against web-services (API page) guidance from BCQuality.
|
||||
description: Reviews AL API surfaces and webhook integration handlers against web-services guidance from BCQuality.
|
||||
inputs: [pr-diff, file-path]
|
||||
outputs: [findings-report]
|
||||
bc-version: [all]
|
||||
technologies: [al]
|
||||
technologies: [al, javascript]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
|
@ -27,7 +27,7 @@ Read the BCQuality knowledge index once — the `knowledge-index.json` BCQuality
|
|||
Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context:
|
||||
|
||||
- `bc-version` — the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`.
|
||||
- `technologies` — `[al]`.
|
||||
- `technologies` — `[al]` or `[javascript]`.
|
||||
- `countries` — the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`.
|
||||
- `application-area` — the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`.
|
||||
|
||||
|
|
@ -37,9 +37,10 @@ Discard files that are not applicable. Retain conditionally applicable files (an
|
|||
|
||||
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed AL object names and types — especially page objects declared with `PageType = API`, and any procedure on such a page that exposes a bound action.
|
||||
- The changed properties and triggers, weighted toward API page metadata (`APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SourceTable`), CRUD guards (`InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`), the `OnOpenPage` trigger, and `OnValidate` triggers on exposed fields.
|
||||
- Tokens extracted from the diff that relate to API surface and behaviour (`PageType`, `API`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SystemId`, `ServiceEnabled`, `WebServiceActionContext`, `SetActionResponse`, `ReadIsolation`, `IsolationLevel`, `ReadCommitted`, `InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`, `SourceTable`).
|
||||
- The changed AL object names and types — especially pages declared with `PageType = API`, API page `part` controls, queries declared with `QueryType = API`, and procedures that expose bound actions.
|
||||
- The changed properties and triggers, weighted toward API page metadata (`APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SourceTable`, `SourceTableTemporary`), navigation metadata (`SubPageLink`, `Multiplicity`), CRUD guards (`InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`), the `OnOpenPage` trigger, and `OnValidate` triggers on exposed fields.
|
||||
- Webhook subscriber handlers and subscription lifecycle code, especially code that creates or renews subscriptions, handles `validationToken`, schedules from `expirationDateTime`, or targets resources whose eligibility is visible in the diff.
|
||||
- Tokens extracted from the diff that relate to API surface and behaviour (`PageType`, `QueryType`, `API`, `api-page`, `page-part`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SystemId`, `SubPageLink`, `subpagelink`, `Multiplicity`, `multiplicity`, `Many`, `ZeroOrOne`, `SourceTableTemporary`, `Job Queue Entry`, `webhook`, `webhookSupportedResources`, `webhook-supported-resources`, `subscriptions`, `notificationUrl`, `validationToken`, `validationtoken`, `expirationDateTime`, `expirationdatetime`, `ServiceEnabled`, `WebServiceActionContext`, `SetActionResponse`, `ReadIsolation`, `IsolationLevel`, `ReadCommitted`, `InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`, `SourceTable`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
@ -55,6 +56,8 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
|
||||
For API parts whose parent declares `ODataKeyFields = SystemId`, detect a child foreign key linked to a parent business field instead of `Field(SystemId)`, and an omitted `Multiplicity` where the diff defines the navigation contract. Do not apply the SystemId-link rule to APIs intentionally keyed by another field. For webhook eligibility, detect `QueryType = API`, `SourceTableTemporary = true`, composite `ODataKeyFields` (including an omitted property when a visible source primary key is composite), Job Queue Entry, and visible system-table sources; do not infer an unknown table number. For lifecycle code, require both create and renew paths to use a handler that returns the query-string `validationToken` verbatim with `200 OK`, and flag renewal scheduling that assumes subscriptions are permanent instead of using `expirationDateTime`. Do not emit generic HTTP or REST advice.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type).
|
||||
|
|
@ -71,7 +74,7 @@ Outcome selection:
|
|||
|
||||
- `completed` — the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty.
|
||||
- `no-knowledge` — no applicable web-services knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty.
|
||||
- `not-applicable` — the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task).
|
||||
- `not-applicable` — the task context contains no AL API surface, JavaScript webhook subscription lifecycle code, or JavaScript notification handler, or the `technologies` filter rejected the task.
|
||||
- `partial` — a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause.
|
||||
- `failed` — an unrecoverable error occurred. `outcome-reason` is required.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue