mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-06 07:06:54 +01:00
Merge latest main into performance guidance
Preserve the reviewed performance package while incorporating the strengthened deterministic review-fixture coverage contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Jesper Schulz-Wedde
commit
b67e048070
177 changed files with 5475 additions and 54 deletions
|
|
@ -52,6 +52,8 @@ The following targeted checks cover every current `appsource` article across the
|
|||
- A page or report that repository context identifies as a direct user entry point omits `UsageCategory` or sets it to `None` — `set-usagecategory-on-searchable-entry-points`. Do not select this article based only on object type; exclude supporting parts, dialogs, API pages, and objects intentionally reached through another page.
|
||||
- A `DateTime` assignment adds or subtracts a fixed duration to represent an assumed regional offset — `do-not-hard-code-time-zone-offsets`. Require contextual evidence such as an hour-sized constant, offset-oriented name, or time-zone comment; do not flag deadlines, schedules, or elapsed-time calculations.
|
||||
- For BC v27 or later, `app.json` adds or changes the `help` URL to a path deeper than two levels, or a changed Copilot/context-sensitive help arrangement would ground the app under an overly broad truncated parent — `keep-copilot-help-url-to-two-path-levels`.
|
||||
- A release/submission pipeline change (`AL-Go-Settings.json`, a publish/release workflow) or an `app.json` version bump is present without the new complete version being strictly greater than the previously submitted one, or the change asserts a hand-edited build/revision or every-merge-is-a-release policy as a universal AppSource rule rather than a project-specific workflow choice — `release-must-update-app-version`. Require repository/pipeline context to know the previously submitted version; a single `app.json` diff cannot prove ordering on its own.
|
||||
- Changed code declares or calls `File.Open`/`File.Create`/`File.Read`/`File.Write` in an app targeting Business Central Online — `file-datatype-saas`.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
|
|
|
|||
|
|
@ -50,7 +50,9 @@ The following targeted checks cover every current `breaking-changes` article:
|
|||
- A published procedure changes parameter count/order/type/name, `var`, return type, or array shape instead of preserving the old signature and adding an overload — `do-not-change-published-procedure-signatures`.
|
||||
- A public procedure/event/interface exposes a credential or other sensitive value through `Text` or an externally callable contract — `do-not-expose-sensitive-data-through-public-api`.
|
||||
- Code already marked obsolete is expanded with new behavior instead of routing new callers to its replacement — `do-not-modify-code-already-marked-obsolete`.
|
||||
- A shipped table field is deleted, renamed, renumbered, or replaced without retaining the original field as `ObsoleteState = Pending` and migrating its data — `obsolete-table-fields-instead-of-deleting-them`.
|
||||
- A shipped table field is deleted, renamed, renumbered, or replaced without retaining the original field as `ObsoleteState = Pending` and migrating its data — `obsolete-table-fields-instead-of-deleting-them`. This owns AS0005 field-name changes; do not substitute the namespace article.
|
||||
- A published object's namespace changes between the base and changed source while its identity otherwise remains — `namespace-is-part-of-published-object-identity`. Do not apply it to a new, unshipped object or to an ordinary object-name change with no namespace change.
|
||||
- New or changed code calls `Codeunit Mail`'s `CreateMessage`/`Send`/`GetErrorDesc` instead of `Codeunit Email`/`Codeunit "Email Message"` — `prefer-email-module`.
|
||||
|
||||
For `obsolete-table-fields-instead-of-deleting-them`, compare the baseline ID and name before emitting. When the original field remains under the same ID and name with `ObsoleteState = Pending`, and the replacement uses a new ID, the change follows the rule and must not be flagged.
|
||||
|
||||
|
|
|
|||
|
|
@ -46,8 +46,12 @@ The sub-skills invoked by this skill are those listed in frontmatter `sub-skills
|
|||
|
||||
Hosts that orchestrate leaves mechanically SHOULD run
|
||||
`tools/Build-SkillIndex.ps1` and resolve this skill by `id: al-code-review`.
|
||||
The generated `subSkills` array preserves the frontmatter order and avoids
|
||||
host-specific Markdown parsing.
|
||||
The generated `subSkills` array preserves the frontmatter slot order and
|
||||
avoids host-specific Markdown parsing. Before invoking leaves, run
|
||||
`tools/Resolve-SkillWorklist.ps1` with this skill's path and the task's enabled
|
||||
layers and disabled skill paths. Each declared path supplies a leaf `id`; the
|
||||
resolver selects the highest-precedence enabled implementation with that `id`
|
||||
without changing slot order.
|
||||
|
||||
## Relevance
|
||||
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ application-area: [all]
|
|||
|
||||
Reviews AL source changes against the `data-modeling` 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`.
|
||||
|
||||
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, or audit fields. The skill returns `not-applicable` when none of those apply.
|
||||
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, audit fields, dimension wiring, journal-based posting-routine structure, or Item Ledger Entry document-number lookups after a combined sales post. The skill returns `not-applicable` when none of those apply.
|
||||
|
||||
## Source
|
||||
|
||||
|
|
@ -46,6 +46,10 @@ A file enters the candidate worklist when its `keywords` intersect the extracted
|
|||
The following targeted checks cover every current `data-modeling` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action.
|
||||
|
||||
- A `* Setup` table or its page changes singleton structure, uses a nonblank or generated key, permits insert/delete, uses a List page, or does not ensure the blank-keyed row exists — `setup-table-is-a-singleton`.
|
||||
- A new field is typed `Media`, `MediaSet`, or `BLOB` and the field's caption/name suggests a picture or image — `pictures-must-use-media-not-blob`.
|
||||
- Code outside a test codeunit or a demo-data generator calls `WorkDate(NewDate)` (the assignment form, not a bare `WorkDate()` read) as part of logic whose purpose is unrelated to the work date itself — `code-must-not-change-workdate`. A test deliberately setting a date context, or a demo-data routine that saves, sets, and restores the work date to backdate the data it creates, is not this anti-pattern.
|
||||
- A new or extended table's name, fields, or usage positively establish it as one of Business Central's nine business-record types — a name ending `Ledger Entry`/`Register`/`Journal Line`/`Header`/`Line`/`Setup`, an auto-generated `Entry No.`/`No.` key posted from elsewhere, a `Template Name`+`Batch Name`+`Line No.` key, or a singleton `Primary Key` field — `table-design-must-match-bc-table-type-conventions`. Do not worklist it from a bare `keys` block or primary-key declaration alone: a temporary/buffer table, a work queue, a log, a cross-reference/mapping table, or a process-local staging table is not one of the nine types and is out of this rule's scope entirely, not an unresolved case.
|
||||
- Code reads `Item Ledger Entry."Document No."` (or `"Last Shipping No."`/`"Last Posting No."`) after a combined Ship+Invoice **sales** post — `item-ledger-entry-document-no-follows-last-shipping-no`. This is a sales-specific rule: purchase combined posting is Receive+Invoice and uses receiving fields such as `"Last Receiving No."`, not the shipment/document-number behavior this article describes. Do not worklist it from purchase posting code.
|
||||
- A custom master table changes its primary key, `No.`/`No. Series` fields, or `OnInsert` without assigning a blank `No.` from setup through a number series — `master-table-no-from-number-series-in-oninsert`.
|
||||
- BC v22 or later code introduces or retains `NoSeriesManagement`, `InitSeries`, `SelectSeries`, or `SetSeries`, or number assignment/manual-entry checks do not use codeunit `"No. Series"` methods such as `GetNextNo`, `IsManual`, or `TestManual` — `use-no-series-codeunit-not-noseriesmanagement`.
|
||||
- A master gains or changes `Blocked`, or a document line, journal line, reference-field `OnValidate`, or posting routine uses that master without `TestField(Blocked, false)` at the point of use; also cue when the check is placed only in the master's own triggers — `check-blocked-in-referencing-code-not-in-master`.
|
||||
|
|
@ -54,6 +58,10 @@ The following targeted checks cover every current `data-modeling` article. Treat
|
|||
- A `Media` or `MediaSet` field is assigned directly between different table types or different field IDs instead of registering each shared item with `MediaSet.Insert` — `share-mediaset-items-with-insert-not-field-assignment`.
|
||||
- A custom document header assigns defaults outside an `InitRecord` boundary, calls `InitRecord` before assigning its number, or places UI-independent defaults only in a page trigger — `initialize-document-defaults-in-initrecord`.
|
||||
- Directed `Round` calls use `'<'` as mathematical floor or `'>'` as mathematical ceiling, especially where negative amounts are possible — `round-direction-symbols-use-magnitude`.
|
||||
- A new field is typed `Code`/`Text` and its `OnValidate` calls `DimensionManagement`/`DimMgt`, or a table adds Shortcut Dimension fields, a `Dimension Set ID` field, or `AddDimSource`/`GetDefaultDimID` — `dimension-management-wiring`. A master table calling `SaveDefaultDim` and a document/journal table computing its own `Dimension Set ID` are two different valid shapes; do not flag a master table for lacking a `Dimension Set ID` field or a document for lacking `SaveDefaultDim`.
|
||||
- A journal-based posting codeunit is added or changed and validation, Journal-table access, ledger writes, and user-interaction (`Confirm`/dialogs) all occur in one procedure or one codeunit, rather than split across `Check Line`/`Post Line`/`Post Batch`-shaped companions — `check-post-line-batch-pattern`. A document posting routine calling `Post Line` directly without a `Post Batch` companion is not this anti-pattern.
|
||||
- An existing, already-published table's `keys` block adds, removes, or reorders a field in its primary key or any `Clustered = true` key — `do-not-change-primary-key`. A new table defining its own key for the first time is not this anti-pattern; requires repository/publication context to know the table has already shipped.
|
||||
- Code reads a setup/configuration-table field inside a branch that has already decided the value is required, and blank/zero is handled with a fallback to a default rather than `TestField`/an equivalent guard — `testfield-required-setup-field`. A read that is genuinely optional in that branch, or one already guarded by `TestField`, is not this anti-pattern.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
|
|
@ -83,7 +91,7 @@ Outcome selection:
|
|||
|
||||
- `completed` — the skill evaluated every worklist item.
|
||||
- `no-knowledge` — no applicable data-modeling knowledge survived filtering.
|
||||
- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, or audit-field surface.
|
||||
- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, audit-field, dimension-wiring, posting-routine-structure, or Item-Ledger-Entry-document-number surface.
|
||||
- `partial` — a budget was hit before the worklist was exhausted.
|
||||
- `failed` — an unrecoverable error occurred.
|
||||
|
||||
|
|
|
|||
|
|
@ -22,6 +22,13 @@ An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-pat
|
|||
|
||||
Use READ's **Bounded retrieval for review skills** workflow with `-Domain error-handling`. Consume every catalog page across enabled layers before applying this leaf's Relevance and Worklist; preserve each exact catalog path and open complete bodies only for exact paths selected by the Worklist. If the helper or prepared index is unavailable or invalid, use READ's explicit path-discovery and bounded native-read fallback.
|
||||
|
||||
When the review scope contains outbound `HttpClient.Get` or `HttpClient.Post` calls, including resolved call paths, also retrieve every catalog page with `-Domain web-services`, using the same known task dimensions and enabled layers. Restrict supplementary candidates to these article slugs across enabled layers; the links identify their canonical owners:
|
||||
|
||||
- [`handle-httpclient-platform-failure-before-response-access`](../../knowledge/web-services/handle-httpclient-platform-failure-before-response-access.md)
|
||||
- [`check-http-status-before-consuming-response-body`](../../knowledge/web-services/check-http-status-before-consuming-response-body.md)
|
||||
|
||||
Retain each selected catalog row's exact `path`; do not invent paths for missing, pruned, or disabled entries. Apply this leaf's Relevance, Worklist, and READ layer precedence to the supplementary candidates before retrieving complete bodies with `Get-KnowledgeArticles.ps1`. Apply each selected article's own scope and exceptions when evaluating code and agent-finding candidates. Use READ's bounded path-discovery fallback over these same sources when needed. This supplements error-handling knowledge, not the scope of the review with unrelated web-services concerns.
|
||||
|
||||
## Relevance
|
||||
|
||||
Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context:
|
||||
|
|
@ -40,6 +47,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
- The changed AL object names and types — especially codeunits that post or validate, tables and table extensions with `OnValidate` triggers, and any procedure that raises errors or orchestrates a batch over records.
|
||||
- The changed procedures and triggers, weighted toward `OnValidate`/`OnInsert`/`OnModify` triggers, posting and validation routines, and procedures attributed with `[ErrorBehavior(...)]` or `[TryFunction]`.
|
||||
- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `FieldError`, `TestField`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`, `TryFunction`, `GetLastErrorText`, Boolean assignment).
|
||||
- For the outbound HTTP call paths identified in Source, include `HttpClient`, `Get`, `Post`, `HttpResponseMessage`, response use, and caller failure handling (including `[TryFunction]` call sites) in keyword and topic matching.
|
||||
- Resolve changed standalone call targets; when the target declaration has `[TryFunction]`, worklist the ignored-return rule even if the declaration itself is unchanged. Only assignment and conditional use activate try semantics.
|
||||
|
||||
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.
|
||||
|
|
@ -47,6 +55,8 @@ A file enters the candidate worklist when its `keywords` intersect the extracted
|
|||
The following targeted checks cover every current `error-handling` article:
|
||||
|
||||
- `[ErrorBehavior(ErrorBehavior::Collect)]`, `ErrorInfo.Collectible`, `HasCollectedErrors`, `GetCollectedErrors`, or `ClearCollectedErrors` is added or changed, especially when errors are collected without later surfacing/clearing them — `collect-validation-errors-with-errorbehavior`.
|
||||
- New or changed code inserts an error/duration log record around a failed `TryFunction`/`GetLastErrorText`/`GetLastErrorCode` path and then raises, propagates, or rethrows the error — `log-writes-must-survive-rollback`. Do not worklist it when the log insert already happens inside a `Session.StartSession`-targeted codeunit's `OnRun`; that is the compliant shape, not the signal to flag.
|
||||
- A guarded lookup (`if Record.Get(...) then ... else` or similar) sets a value used later, and the same guard shape (with the same blank/zero fallback style) is applied to a field that feeds a posted amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output — `defensive-vs-offensive-code-must-match-blast-radius`. The signal is a posting-critical or compliance-facing field guarded defensively with a silent fallback, not the mere presence of a guarded lookup.
|
||||
- Developer-only invariant text is raised with default client visibility, or a user-actionable validation is hidden as `ErrorType::Internal` — `errortype-internal-vs-client-for-diagnostics`.
|
||||
- `FieldError` receives a complete capitalized sentence, repeats the field caption/value, or ends the predicate with punctuation — `fielderror-default-message-logic`.
|
||||
- An unguarded `FieldError` is used as though it performed a comparison, or `TestField` is forced onto a complex rule needing a tailored predicate — `fielderror-vs-testfield`.
|
||||
|
|
|
|||
|
|
@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially tables, pages with SourceTable bindings, reports, queries, and codeunits performing record iteration.
|
||||
- The changed procedures and triggers, weighted toward those that perform loops, Find/FindSet/FindFirst calls, CalcFields, SetAutoCalcFields, CalcSums, FlowField access, Commit calls, checkpoint helpers, record copying, RecordRef conversion, Modify/Delete calls, or cross-table navigation.
|
||||
- Tokens extracted from the diff that relate to data access, hot-path costs, and background scheduling (`key`, `IncludedFields`, `SumIndexFields`, `SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `FindLast`, `IsEmpty`, `ReadIsolation`, `LockTable`, `Insert`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Validate`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`, `CalcFormula`, `FlowField`, `Query.Open`, `Query.Read`, `Visible`, `Job Queue Entry`, `Job Queue Category Code`, `Confirm`, `RunModal`, `GuiAllowed`, `TryFunction`, `Codeunit.Run`, `HttpClient`, `Status`, `On Hold`, `stop request`, `TaskScheduler.CreateTask`, `TaskScheduler.TaskExists`).
|
||||
- Tokens extracted from the diff that relate to data access, hot-path costs, and background scheduling (`key`, `IncludedFields`, `SumIndexFields`, `SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `FindLast`, `IsEmpty`, `ReadIsolation`, `LockTable`, `Insert`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Validate`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`, `CalcFormula`, `FlowField`, `Query.Open`, `Query.Read`, `Visible`, `Job Queue Entry`, `Job Queue Category Code`, `Confirm`, `RunModal`, `GuiAllowed`, `TryFunction`, `Codeunit.Run`, `HttpClient`, `Status`, `On Hold`, `stop request`, `TaskScheduler.CreateTask`, `TaskScheduler.TaskExists`, `Page.RunModal`, `Report.RunModal`, `Report.Run`, `Xmlport.Run`, `UseRequestPage`).
|
||||
|
||||
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.
|
||||
|
||||
|
|
@ -61,6 +61,8 @@ Apply these targeted cues even when simple token overlap would rank the article
|
|||
- Worklist `job-queue-on-hold-does-not-stop-running-work.md` when a running job queue handler polls the entry's `Status` or `On Hold` value as a cancellation signal. Exclude application-owned stop requests that are checked before every bounded unit of work, including the first, when completed work and its checkpoint remain consistent and resume logic clears the request.
|
||||
- Worklist `job-queue-category-code-serializes-conflicting-jobs.md` when two or more job queue entries in the same company are shown by the changed context to require mutual exclusion but have empty or different Job Queue Category Codes. Do not infer a conflict merely because jobs touch the same tables, and do not recommend a category to coordinate across companies, environments, or workers outside the job queue dispatcher.
|
||||
- Worklist `store-scheduled-task-id-to-avoid-duplicate-tasks.md` when `TaskScheduler.CreateTask` runs from initialization, login, setup, or another repeatable path without persisting its returned GUID and checking it with `TaskScheduler.TaskExists` before creating a replacement. Exclude one-shot creation and correctly persisted check-before-create flows; concurrent callers still require serialization around that sequence.
|
||||
- Worklist `al-methods-limited-during-write-transactions.md` when `Page.RunModal` follows an `Insert`, `Modify`, or `Delete` in the same trigger or procedure with no intervening `Commit`; or when `Report.RunModal`/`Report.Run` without `false` as its `RequestWindow` argument and without `UseRequestPage(false)` follows one; or when `Xmlport.Run` without `false` as its `RequestWindow` argument and without the `UseRequestPage = false` object property follows one. Do not worklist it from a call that precedes every write, from a report run with its request page suppressed via `UseRequestPage(false)`/`Run(...,false)`/`RunModal(...,false)`, or from an XMLport run with its request page suppressed via the `RequestWindow` argument or the `UseRequestPage` property. A `Codeunit.Run` whose return value is used in that position belongs to `codeunit-run-requires-prior-commit-inside-transaction.md`.
|
||||
- A new or changed report object whose usage/name/caption identifies it as a single-record document (invoice, statement, order confirmation) sets or retains `DefaultRenderingLayout = RDLC` — `document-report-word-layout.md`. Do not worklist this from a tabular/list report with heavy aggregation or calculated columns; RDLC/Excel remains the better fit there.
|
||||
|
||||
These targeted inclusions and exclusions override generic token overlap. Do not retain an excluded article solely because the diff contains one of its keywords.
|
||||
|
||||
|
|
|
|||
|
|
@ -77,6 +77,7 @@ paths; they select articles, not findings. Facts and exceptions stay in articles
|
|||
| Registered warehouse quantity/physical-adjustment synchronization, `"Directed Put-away and Pick"`, `"Adjustment Bin Code"`, `"Warehouse Adjustment"`, or `"Calculate Whse. Adjustment"` and the resulting item-journal posting | `reconcile-warehouse-adjustments-with-the-item-ledger` |
|
||||
| `"Transfer Header"`/`"Transfer Line"` shipment/receipt completion, transfer posting publishers, in-transit/document-link changes, or item-journal posting presented as transfer-order completion | `post-transfers-through-shipment-and-receipt-codeunits` |
|
||||
| `Inventory`, `CalcQtyAvailableToPromise`, or stock sums used in a dated supply/demand promise, including changed location/variant/date filters and source-demand context | `use-date-aware-availability-for-promising` |
|
||||
| Direct assignment to `Quantity`, `"Unit of Measure Code"`, `"Qty. per Unit of Measure"`, or a `(Base)` quantity field on a persisted or posted item journal, sales, purchase, or transfer line, or a line quantity compared with a base-unit inventory value | `derive-base-quantities-through-the-line-unit-of-measure` |
|
||||
| `"Requisition Line"` action-message execution, accepted planning suggestions, `"Req. Wksh.-Make Order"`, `CarryOutBatchAction`, or linked supply creation/change plus requisition-line deletion | `carry-out-requisition-actions-through-the-standard-workflow` |
|
||||
|
||||
Route clean supported calls through the same cues, not just suspicious writes.
|
||||
|
|
|
|||
|
|
@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially permission sets, codeunits handling authentication or authorization, objects touching `Isolated Storage`, `OAuth2` flows, web service endpoints, API pages, event publishers, and RecordRef helpers.
|
||||
- The changed procedures and triggers, weighted toward those that call `HttpClient`, validate or compose URLs, write to telemetry, read or write secrets, unwrap SecretText, manipulate record-level security, expose var Boolean guard parameters, or bypass the permission model (for example, `RecordRef.Open`, `Record.WritePermission`, direct table access from a non-owning app).
|
||||
- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`).
|
||||
- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`, `SetFilter`, `ServiceEnabled`, `PageType = API`, `QueryType = API`, `permissionset`, `Web Services`).
|
||||
|
||||
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.
|
||||
|
||||
|
|
@ -49,6 +49,7 @@ For secret values, select the most specific sink owner:
|
|||
|
||||
- When a `Text`/`Code` credential is declared, passed, returned, or unwrapped without a visible HTTP URI/header/body sink, use `secrettext-for-credentials.md`.
|
||||
- When that value is interpolated into a URI, authorization header, or HTTP body and sent through `HttpClient`, use `secrettext-with-httpclient.md` as the primary finding. It supersedes the generic credential-type article at that location; keep the latter only as a supporting reference when useful.
|
||||
- When a page/query is registered in Web Services, declares `PageType = API`/`QueryType = API`, a codeunit is registered in Web Services, or `[ServiceEnabled]` is added to a page procedure, and no permission set in the app grants a matching `page "..." = X` / `query "..." = X` / `codeunit "..." = X` entry for that specific object — use `exposed-objects-must-be-in-a-permission-set.md`. The anti-pattern is the exposed object missing its own execute entry, even when the underlying table's `tabledata` permissions look complete; require repository-level permission-set context, since one file cannot prove an entry is absent elsewhere in the app.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
|
|
|
|||
|
|
@ -40,8 +40,8 @@ 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. For each relevant file, compute overlap against:
|
||||
|
||||
- Changed AL objects — especially API pages (`PageType = API`), tables and pages declaring Labels/TextConsts, codeunits issuing `Error`/`Message`/`Confirm`, and any file whose name violates the `<ObjectName>.<ObjectType>.al` convention.
|
||||
- Changed declarations, weighted toward `: Label '...'`, `: TextConst '...'`, temporary record variables, `DateFormula` declarations and their `Evaluate` call sites, error-handling call sites, and API declarations.
|
||||
- Tokens extracted from the diff (`Label`, `TextConst`, `Locked`, `Comment`, `MaxLength`, `temporary`, `DateFormula`, `Evaluate`, `CalcDate`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `DelayedInsert`, `FieldCaption`, `TableCaption`, `FieldName`, `TableName`, `Page.RunModal`, `Report.Run`, `StrSubstNo`).
|
||||
- Changed declarations, weighted toward `: Label '...'`, `: TextConst '...'`, temporary record variables, option fields, `DateFormula` declarations and their `Evaluate` call sites, error-handling call sites, API declarations, and codeunit-internal method calls.
|
||||
- Tokens extracted from the diff (`Label`, `TextConst`, `Locked`, `Comment`, `MaxLength`, `temporary`, `DateFormula`, `Evaluate`, `CalcDate`, `OptionMembers`, `OptionCaption`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `DelayedInsert`, `FieldCaption`, `TableCaption`, `FieldName`, `TableName`, `Page.RunModal`, `Report.Run`, `this.`, `StrSubstNo`, `namespace`, `using`, `var `).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object or declaration. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
@ -51,6 +51,17 @@ Apply these high-signal mappings before fuzzy topic ranking:
|
|||
|
||||
- A `Label` or `TextConst` contains multiple or ambiguous placeholders but has no `Comment`, or its Comment does not explain every placeholder — `label-comment-explains-placeholders.md`. A single placeholder whose meaning is explicit in the text, such as `Customer %1`, is allowed without a Comment and must not be flagged.
|
||||
- A normal two-argument `Evaluate` has a resolved `DateFormula` destination and a hard-coded non-angle-bracket date-formula literal, directly or through a visible constant — `dateformula-evaluate-needs-language-independent-literals.md`. Do not use this cue for dynamic/localized external input, already invariant `<...>` input, or direct `CalcDate(Text, ...)` calls.
|
||||
- `function-call-parentheses-required.md` applies only to a zero-argument invocation written without `()`. Never worklist it from an invocation that already has parentheses or supplies arguments, including `Error(Label, Arg1, Arg2)`.
|
||||
- A new or changed comment restates what the adjacent code already makes obvious from its own names and structure (a comment that just repeats a variable/field/method name in prose) rather than explaining a non-obvious constraint, invariant, or workaround — `al-comments-must-not-restate-what-code-already-shows.md`. A comment absent entirely is not this anti-pattern; only a present-but-redundant comment is.
|
||||
- A `page`/`pageextension` adds or changes a procedure body that performs a calculation, validation, or record mutation belonging to a business operation reused across entry points, rather than presentation-specific state or a call into a codeunit — `pages-must-not-contain-business-logic.md`. A page calling a codeunit procedure, or a page's own presentation-only state and formatting, is not this anti-pattern; nor is a data invariant that belongs on the table itself.
|
||||
- Changed source files are added under an object-type folder (`Tables/`, `Pages/`, `Codeunits/`, etc.) in a repository whose existing structure is predominantly feature-based, or vice versa — `source-organized-by-feature-not-object-type.md`. The anti-pattern is inconsistency with the repository's own established convention, not the choice of either scheme; a repository consistently organized by object type throughout is not a violation. Require repository-level folder context; a single new file's path cannot prove the project's convention alone.
|
||||
- A new `.app` build artifact appears at the project root or another unversioned/arbitrary location, or is added to source control alongside the AL source that produced it — `al-build-output-must-not-pollute-project-root.md`. A deliberate `--outfolder`/`outputPath` destination added to `.gitignore` is the compliant shape, not the signal to flag.
|
||||
- A new or renamed AL identifier (variable, procedure, parameter, field, object, enum value, or label identifier) contains non-English words — `al-identifiers-english.md`. A caption, tooltip, or other user-facing text value in a non-English language is not this anti-pattern; only the identifier itself is in scope.
|
||||
- A field or variable is typed `Boolean` and its two possible values are genuinely named domain alternatives (a status pair like Inbound/Outbound, Debit/Credit, Buy/Sell) rather than a true/false predicate, or an `Option`/`Enum`/`Integer` models a domain concept that is intrinsically a yes/no flag — `binary-choice-must-be-boolean.md`. The signal is a semantic mismatch between the type and the domain concept, not the current number of states.
|
||||
- A field or variable is typed `Integer` with the meaning of each value tracked only in a comment, or an `Enum` with more than two members is proposed as `Boolean`-like — `fixed-choice-set-must-use-enum-not-integer.md`. Do not flag a genuinely two-state `Enum`/`Option` for having "too few" members; that overlaps `binary-choice-must-be-boolean.md` instead when the domain is a true/false predicate.
|
||||
- A new `using` directive is added for an existing AL object without the diff also showing that object's own `namespace` declaration or a symbol-package lookup backing the choice — `namespace-must-be-verified-from-source.md`. Require repository/dependency context; a single new `using` line cannot itself prove whether the namespace was verified or guessed.
|
||||
- An intrinsic/built-in AL function call (`MESSAGE`, `ERROR`, `CONFIRM`, `STRSUBSTNO`, etc.) is written in ALL-CAPS or another non-PascalCase form — `intrinsic-al-functions-must-use-modern-casing.md`.
|
||||
- A procedure call passes a literal or a computed expression (not a caller-scope variable) to a parameter position the callee declares `var` — `var-parameters-require-an-addressable-variable.md`. This is a compile-time-guaranteed shape; flag it only when the callee's declared signature is visible in the diff or resolvable from context.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.
|
||||
|
||||
|
|
|
|||
|
|
@ -39,18 +39,30 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially codeunits with `Subtype = Test`, test runner codeunits with `TestIsolation`, test libraries, and codeunits that define UI handlers.
|
||||
- The changed methods and attributes, weighted toward `[Test]`, `[TransactionModel(...)]`, `[TestPermissions(...)]`, `[HandlerFunctions(...)]`, handler attributes, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, fixture initialization, and test-library calls.
|
||||
- Tokens extracted from the diff that relate to testing (`Subtype = Test`, `Subtype = TestRunner`, `TestIsolation`, `TestPermissions`, `Restrictive`, `NonRestrictive`, `Disabled`, `Permissions Mock`, `Library - Lower Permissions`, `TransactionModel`, `AutoRollback`, `AutoCommit`, `Commit`, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, `HandlerFunctions`, `ConfirmHandler`, `MessageHandler`, `StrMenuHandler`, `ModalPageHandler`, `SendNotificationHandler`, `RecallNotificationHandler`, `Enqueue`, `Dequeue`, `AssertEmpty`, `Library Assert`, `LibraryVariableStorage`, `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, `Init`, `Insert`).
|
||||
- Tokens extracted from the diff that relate to testing (`Subtype = Test`, `Subtype = TestRunner`, `TestIsolation`, `TestPermissions`, `Restrictive`, `NonRestrictive`, `Disabled`, `Permissions Mock`, `Library - Lower Permissions`, `TransactionModel`, `AutoRollback`, `AutoCommit`, `Commit`, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, `HandlerFunctions`, `ConfirmHandler`, `MessageHandler`, `StrMenuHandler`, `ModalPageHandler`, `SendNotificationHandler`, `RecallNotificationHandler`, `Enqueue`, `Dequeue`, `AssertEmpty`, `Initialize`, `IsInitialized`, `OnTestInitialize`, `LibrarySetupStorage`, `Library Assert`, `LibraryVariableStorage`, `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, `Library - Utility`, `LibraryUtility`, `GenerateGUID`, `GenerateRandomCode`, `TestPage`, `.Visible(`, `.Enabled(`, `.Editable(`, `OpenNew`, `OpenView`, `OpenEdit`, `Init`, `Insert`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no testing-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
|
||||
|
||||
The following targeted checks cover every current `testing` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action.
|
||||
|
||||
- A method in a `Subtype = Test` codeunit adds or changes `[TransactionModel(...)]`, exercises code that calls `Commit` under `AutoRollback`, defaults broadly to `AutoCommit`, or chooses `None` for a writing test — `transactionmodel-attribute-governs-test-transactions`.
|
||||
- A new or changed `[Test]` procedure is added, whether or not it already carries `[FEATURE]`/`[SCENARIO]`/`[GIVEN]`/`[WHEN]`/`[THEN]` tags — `test-feature-scenario-tags`. A procedure with no tags at all, or a generic name like `Test1`, is the anti-pattern signal; presence of the tags is the compliant shape, not the thing to search for.
|
||||
- A test codeunit calls `TestPage` methods (`OpenNew`, `OpenView`, `OpenEdit`) alongside `[Test]` procedures in the same codeunit that call business-logic procedures directly with no `TestPage` involved — `ui-test-codeunit-naming`. The anti-pattern signal is both kinds of test mixed into one codeunit (or, on a project using the `_UT` convention, a UI-layer codeunit missing the suffix); a codeunit containing only `TestPage`-driven tests is not itself a violation.
|
||||
- A `[GIVEN]`-tagged setup precedes a posting call or report execution and does not visibly set up posting-group/VAT setup records, an explicit date, or (for a report test) both an included and an excluded record — `given-blocks-must-cover-full-precondition-chain`.
|
||||
- A test procedure contains more than one `[WHEN]` block, or more than one distinct action not labelled `[GIVEN]`, without the procedure name declaring a flow/defect-then-fix shape — `test-one-when-per-test`.
|
||||
- A `BCPT*` scenario codeunit is added and the PerformanceTest app's only other scenario codeunits are copies of Microsoft's shipped BCPT samples (`BCPT Create Customer`, `BCPT Create Item Journal`, `BCPT Post GL Entries`, etc.) with no scenario exercising the extension's own codeunits, FlowFields, or pages — `bcpt-scenarios-must-be-app-specific`.
|
||||
- An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it.
|
||||
- A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`.
|
||||
- Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`.
|
||||
- `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`.
|
||||
- A test codeunit's `Initialize` procedure exits on `IsInitialized` before per-test reset such as `LibraryVariableStorage.Clear`, `LibrarySetupStorage.Restore`, or `LibraryTestInitialize.OnTestInitialize`, or a `[Test]` method in a codeunit using that pattern does not call `Initialize()` first — `reset-per-test-state-before-the-isinitialized-guard`.
|
||||
- `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`. Exclude `asserterror Assert.IsTrue(...)` / `asserterror Assert.IsFalse(...)` only when it is used solely to invert the guarded call's Boolean result (the same condition `use-assert-isfalse-not-asserterror-for-boolean-checks` cues on below, which wins for that shape) — not when the test expects the guarded Boolean-returning call itself to raise an error, which this rule still owns even though it happens to wrap an `Assert.IsTrue`/`IsFalse` call. Also exclude a trailing `asserterror Error(...)` used purely as an end-of-test rollback sentinel after a lazy `Initialize()` fixture already committed — that shape belongs to `commit-shared-test-fixture-inside-lazy-initialize`, which wins for it; the sentinel's own error text is not meant to be asserted against.
|
||||
- `asserterror` wraps `Assert.IsTrue(BooleanExpression, ...)` (or the `IsFalse` mirror) solely to invert the boolean result of the guarded call, rather than to assert that call itself raises an error — `use-assert-isfalse-not-asserterror-for-boolean-checks`.
|
||||
- A shared/lazy `Initialize()`-style fixture helper creates fixture data without a following `Commit()`, in a test method whose body later forces its own rollback (for example `asserterror Error(...)` used for end-of-test cleanup) — `commit-shared-test-fixture-inside-lazy-initialize`. The presence of `Commit()` after the fixture is the compliant shape, not the signal to look for; the missing-`Commit()` shape combined with a later deliberate rollback is the anti-pattern. Require runner/repository context for the `TestIsolation` value: a standalone test file cannot prove which runner executes it, and under `Function`-level isolation this whole pattern is moot regardless of `Commit()` — do not raise the finding when the executing runner's `TestIsolation` is known to be `Function`.
|
||||
- Changed code subscribes to `OnAfterRemoveTableRelation`, calls `RemoveTableRelation`, or references `Codeunit "Table Relation Test"`/134926 — `table-relation-test-exclude-known-invalid-relations-via-event`.
|
||||
- Test fixture code assigns a hardcoded literal to a primary-key field or a field the test relies on as a unique lookup identifier, hand-builds a "unique" value for such a field (string concatenation, a counter, `Format(CurrentDateTime)`), or truncates `LibraryUtility.GenerateGUID()`'s result with `CopyStr` for such a field shorter than 10 characters — `use-generateguid-for-unique-test-fixture-values`. Calling `GenerateGUID()` untruncated into a full-length field, `GenerateRandomCodeWithLength` for a shorter field needing real verified uniqueness, or `GenerateRandomCode20` specifically for a `Code[20]` field, is the compliant shape, not the signal to flag. `GenerateRandomCode20` is not a substitute for `GenerateRandomCodeWithLength` on a shorter field — it truncates `GenerateGUID()`'s sequential value down to the field's length by keeping the *leftmost* characters, which change the slowest, so retries against a short field can churn through the same truncated prefix far longer than `GenerateRandomCodeWithLength`'s equivalent. A hardcoded or deterministic value in an ordinary descriptive field is not this anti-pattern — that field carries no uniqueness constraint. Do not claim `GenerateRandomCode` (without `WithLength`/`20`) or `GenerateRandomXMLText` verify uniqueness against the real table, or that `GenerateRandomCode` is collision-free even within one test run for a short field — none of that is true.
|
||||
- A test asserts against a `TestPage` field's `.Visible()` or `.Enabled()` — `use-testpage-visible-enabled-to-verify-field-ui-state`. When the assertion is against `.Editable()`, or the page is opened with `OpenEdit()` specifically to check editability — `use-testpage-editable-to-verify-field-editability`.
|
||||
- A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Only nonoptional handlers have to execute: a listed handler declared `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` is optional by design, so do not treat it as unmatched when the run never raises the notification.
|
||||
- A test's `[GIVEN]`/setup looks up a hardcoded code/number/name assumed to already exist instead of creating it, leaves a mandatory field on a created record empty, uses a value that doesn't satisfy the scenario's own explicit length/format requirement (for example a truncation test whose value never exceeds the field), or a scenario-defining value (amount, quantity, percentage, date, threshold, rounding precision) is generated/randomized instead of an explicit chosen value — `test-data-must-be-random-and-complete`. Generating incidental fixture values (identifiers, names, descriptions) via the standard library codeunits is the compliant shape, not the signal to flag, and neither is a short-but-valid value in an otherwise-unremarkable field.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
|
|
|
|||
|
|
@ -40,8 +40,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 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.
|
||||
- A new or changed page's name/suffix, primary-key handling, `CardPageID`, `SubPageLink`, `AutoSplitKey`, or `UsageCategory` doesn't match the conventions of its own declared `PageType` — `page-design-must-match-bc-page-type-conventions.md`. A Card page over a composite-key table that supplements a master record, or a supporting/subpage/dialog page intended only to be reached through another workflow and correctly omitting `UsageCategory`, is not this anti-pattern on its own; check whether the page is actually mixing conventions or is meant as a searchable entry point before flagging.
|
||||
- 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`, promoted action definitions, field importance, page background tasks, 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`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `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).
|
||||
- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `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, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`).
|
||||
|
||||
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.
|
||||
|
||||
|
|
|
|||
|
|
@ -45,6 +45,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
- Worklist the install-versus-upgrade rule when migration helpers are reachable only from an install codeunit.
|
||||
- Worklist `install-and-upgrade-codeunits-have-no-order.md` when a change adds multiple install or upgrade codeunits whose same-phase triggers share state or depend on one another.
|
||||
- Worklist `appversion-meaning-depends-on-execution-context.md` when install or upgrade code branches on `ModuleInfo.AppVersion()` or confuses it with `DataVersion()`.
|
||||
- An upgrade tag's existence check (`HasUpgradeTag`) is nested inside another tag's guarded body, or one tagged procedure performs two or more functionally unrelated migrations (different tables, fields, or concerns) under a single tag, or one procedure mixes the gated logic for more than one distinct upgrade tag — `upgrade-tag-logic-must-not-nest-deeply.md`. Do not flag record loops or business-data safety guards (corruption checks, redundant-write checks, or other conditions) that serve the single migration the tag represents, however many `if` levels they take — that is the compliant shape the article explicitly permits.
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no upgrade-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
|
||||
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ kind: action-skill
|
|||
id: al-web-services-review
|
||||
version: 1
|
||||
title: AL web services review
|
||||
description: Reviews AL API surfaces and webhook integration handlers against web-services guidance from BCQuality.
|
||||
description: Reviews AL API surfaces, outbound HTTP integrations, and webhook handlers against web-services guidance from BCQuality.
|
||||
inputs: [pr-diff, file-path, folder-path]
|
||||
outputs: [findings-report]
|
||||
bc-version: [all]
|
||||
|
|
@ -14,7 +14,7 @@ application-area: [all]
|
|||
|
||||
# AL web services review
|
||||
|
||||
Reviews AL source changes against the `web-services` 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`.
|
||||
Reviews AL source changes against the `web-services` knowledge domain in BCQuality and emits a findings report. This includes inbound API surfaces, outbound HTTP integrations, and webhook lifecycle code. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
|
||||
|
||||
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. The skill produces a single JSON document conforming to the DO output contract.
|
||||
|
||||
|
|
@ -39,8 +39,13 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- 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`, and visible singleton or collection semantics), CRUD guards (`InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`), the `OnOpenPage` trigger, and `OnValidate` triggers on exposed fields.
|
||||
- Outbound HTTP integration code that constructs or sends requests, captures a client method's optional Boolean result, checks an `HttpResponseMessage`, or reads and parses response content.
|
||||
- Integration code that converts values to or from exchanged text: `Format` or `Evaluate` on a request URL, body, or file line, and `JsonToken`/`JsonValue` conversions of payload properties.
|
||||
- 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`).
|
||||
- An API page (`PageType = API`) is added or changed and its `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` properties are left at their default `true` (or explicitly set `true`) for an operation the endpoint's stated purpose does not need — `api-page-least-privilege-write-access.md`. The signal is an operation left enabled beyond what the endpoint's own described purpose requires, not the mere presence of these properties; a page that genuinely needs full CRUD and grants it deliberately is not a violation.
|
||||
- An API page (`PageType = API`) with `InsertAllowed = true` lists a field in `ODataKeyFields` and that same field control sets `Editable = false` — `api-page-key-fields-must-be-editable-on-insert.md`. A system-generated key such as `SystemId` marked read-only is not this anti-pattern.
|
||||
- An API page exposes a field via `Rec` directly, and that field is a stored value computed from other fields inside an `OnValidate` trigger rather than a FlowField recalculated in `OnAfterGetRecord` — `stored-derived-fields-must-not-be-exposed-directly.md`. Exposing a genuine FlowField, or a value already recalculated in `OnAfterGetRecord`, is not this anti-pattern.
|
||||
- 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`, `HttpClient`, `HttpRequestMessage`, `HttpResponseMessage`, `Get`, `Post`, `Put`, `Delete`, `Send`, `IsSuccessStatusCode`, `HttpStatusCode`, `Content`, `ReadAs`, `JsonObject`, `JsonToken`, `JsonValue`, `AsValue`, `IsNull`, `SelectToken`, `Format`, `Evaluate`, `XmlDocument`).
|
||||
|
||||
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.
|
||||
|
||||
|
|
@ -56,7 +61,14 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice`
|
|||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present.
|
||||
|
||||
For API parts whose parent declares `ODataKeyFields = SystemId`, detect a child foreign key linked to a parent business field instead of `Field(SystemId)`. Do not apply the SystemId-link rule to APIs intentionally keyed by another field. Omitted `Multiplicity` is valid and means the documented default 1:N collection; never report omission alone. Report an explicit `ZeroOrOne` only when the visible contract clearly intends a collection or deep insert, and report an explicit `Many` only when it clearly intends a singleton. Singleton metadata requires an explicit `ZeroOrOne`; do not infer singleton intent from naming alone. For webhook eligibility, detect `QueryType = API`, `SourceTableTemporary = true`, composite `ODataKeyFields` (including an omitted property when a visible source primary key is composite), Job Queue Entry, and visible system-table sources; do not infer an unknown table number. For lifecycle code, require both create and renew paths to use a handler that returns the query-string `validationToken` verbatim with `200 OK`, and flag renewal scheduling that assumes subscriptions are permanent instead of using `expirationDateTime`. Do not emit generic HTTP or REST advice.
|
||||
For API parts whose parent declares `ODataKeyFields = SystemId`, detect a child foreign key linked to a parent business field instead of `Field(SystemId)`. Do not apply the SystemId-link rule to APIs intentionally keyed by another field. Omitted `Multiplicity` is valid and means the documented default 1:N collection; never report omission alone. Report an explicit `ZeroOrOne` only when the visible contract clearly intends a collection or deep insert, and report an explicit `Many` only when it clearly intends a singleton. Singleton metadata requires an explicit `ZeroOrOne`; do not infer singleton intent from naming alone. For webhook eligibility, detect `QueryType = API`, `SourceTableTemporary = true`, composite `ODataKeyFields` (including an omitted property when a visible source primary key is composite), Job Queue Entry, and visible system-table sources; do not infer an unknown table number. For lifecycle code, require both create and renew paths to use a handler that returns the query-string `validationToken` verbatim with `200 OK`, and flag renewal scheduling that assumes subscriptions are permanent instead of using `expirationDateTime`.
|
||||
|
||||
For outbound HTTP calls, apply these targeted checks:
|
||||
|
||||
- When code captures the optional Boolean result of `HttpClient.Get`, `Post`, `Put`, `Delete`, or `Send`, require the failed branch to stop before status, headers, or content are accessed. Do not report a call that omits the Boolean result: that form intentionally lets the runtime raise an error when the request cannot execute.
|
||||
- After platform success, require `IsSuccessStatusCode()` or an explicit acceptable `HttpStatusCode()` check before response content is interpreted as a success payload. Do not report code that reads a bounded non-success body solely for diagnostics and keeps it out of the success parsing path.
|
||||
|
||||
Do not emit generic HTTP or REST advice beyond applicable knowledge articles.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
|
|
@ -64,7 +76,7 @@ Set `confidence` to:
|
|||
- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`.
|
||||
- `low` when the finding is an advisory derived only from applicability.
|
||||
|
||||
After evaluating each worklist entry, also consider whether the diff exhibits a web-services defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material web-services defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly API pages and web-service surfaces; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract.
|
||||
After evaluating each worklist entry, also consider whether the diff exhibits a web-services defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material web-services defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly API pages, outbound HTTP integrations, and other web-service surfaces; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract.
|
||||
|
||||
For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: set `ODataKeyFields = SystemId`; add the three `*Allowed = false` guards to a read-only page; add the missing `OnOpenPage` isolation assignment). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`.
|
||||
|
||||
|
|
@ -74,7 +86,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 contains no AL API surface, JavaScript webhook subscription lifecycle code, or JavaScript notification handler, or the `technologies` filter rejected the task.
|
||||
- `not-applicable` — the task context contains no AL API surface, outbound AL HTTP integration, 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