mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 06:36:55 +01:00
Merge main after Job Queue sample link fix
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 76eb42c4-2acd-4f9c-a898-f4a44f9d46f7
This commit is contained in:
dayland
commit
0a018fd9fb
48 changed files with 1145 additions and 49 deletions
|
|
@ -0,0 +1,12 @@
|
|||
codeunit 50100 "Rental Profile Install"
|
||||
{
|
||||
Subtype = Install;
|
||||
|
||||
trigger OnInstallAppPerDatabase()
|
||||
var
|
||||
RentalProfile: Record Profile;
|
||||
begin
|
||||
RentalProfile.Init();
|
||||
RentalProfile.Insert(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,6 @@
|
|||
profile "RENTAL MANAGER"
|
||||
{
|
||||
Caption = 'Rental Manager';
|
||||
Description = 'Manages rental agreements and equipment availability.';
|
||||
RoleCenter = "Business Manager Role Center";
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: appsource
|
||||
keywords: [profile-object, profile-table, install-codeunit, role-center, page-customization]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Define profiles as AL objects
|
||||
|
||||
## Description
|
||||
|
||||
Profiles delivered by a Marketplace extension must be declared as AL `profile` objects. A profile object is validated with its Role Center and page customizations when the extension is compiled and is registered through extension synchronization. Inserting profile-table records from install or setup code bypasses that object lifecycle.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Declare each app-owned profile with the `profile` object and set its `RoleCenter`, user-facing caption, and optional customizations in AL. Let installation and synchronization register the object.
|
||||
|
||||
See sample: [`define-profiles-as-al-objects.good.al`](define-profiles-as-al-objects.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Install, upgrade, or setup code that creates an app-owned profile by inserting a `Profile` table record. Detection signal: a `Record Profile` variable followed by `Insert` in profile provisioning code.
|
||||
|
||||
See sample: [`define-profiles-as-al-objects.bad.al`](define-profiles-as-al-objects.bad.al).
|
||||
|
|
@ -0,0 +1,7 @@
|
|||
codeunit 50100 "Rental Audit"
|
||||
{
|
||||
procedure SetCreatedAt(var RentalAgreement: Record "Rental Agreement")
|
||||
begin
|
||||
RentalAgreement."Created At" := CurrentDateTime() + 7200000;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,7 @@
|
|||
codeunit 50100 "Rental Audit"
|
||||
{
|
||||
procedure SetCreatedAt(var RentalAgreement: Record "Rental Agreement")
|
||||
begin
|
||||
RentalAgreement."Created At" := CurrentDateTime();
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: appsource
|
||||
keywords: [datetime, time-zone, utc, currentdatetime, locale, regional-settings]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not hard-code time-zone offsets
|
||||
|
||||
## Description
|
||||
|
||||
Marketplace extensions run for users and services in many time zones. Adding a fixed offset to a `DateTime` assumes one locale, ignores daylight-saving transitions, and changes an absolute timestamp into an incorrect value for other regions.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Store and compare `DateTime` values without a manually applied regional offset. Business Central stores `DateTime` values in UTC and presents them according to the client time zone. Keep service contracts time-zone explicit and perform a conversion only when the business requirement identifies a particular zone.
|
||||
|
||||
See sample: [`do-not-hard-code-time-zone-offsets.good.al`](do-not-hard-code-time-zone-offsets.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Adding or subtracting a fixed duration solely to convert `CurrentDateTime` or another timestamp to an assumed local time. Detection signals include fixed hour-sized millisecond values near `DateTime` assignments and comments naming a specific time zone; confirm the duration is an offset rather than a legitimate deadline or schedule interval.
|
||||
|
||||
See sample: [`do-not-hard-code-time-zone-offsets.bad.al`](do-not-hard-code-time-zone-offsets.bad.al).
|
||||
|
|
@ -0,0 +1,21 @@
|
|||
codeunit 50100 "Rental Service"
|
||||
{
|
||||
[ServiceEnabled]
|
||||
procedure CloseAgreement(AgreementNo: Code[20]): Boolean
|
||||
var
|
||||
RentalAgreement: Record "Rental Agreement";
|
||||
begin
|
||||
if not Confirm(CloseAgreementQst, false, AgreementNo) then
|
||||
exit(false);
|
||||
|
||||
RentalAgreement.Get(AgreementNo);
|
||||
RentalAgreement.Closed := true;
|
||||
RentalAgreement.Modify(true);
|
||||
Message(AgreementClosedMsg, AgreementNo);
|
||||
exit(true);
|
||||
end;
|
||||
|
||||
var
|
||||
CloseAgreementQst: Label 'Close rental agreement %1?';
|
||||
AgreementClosedMsg: Label 'Rental agreement %1 was closed.';
|
||||
}
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
codeunit 50100 "Rental Service"
|
||||
{
|
||||
[ServiceEnabled]
|
||||
procedure CloseAgreement(AgreementNo: Code[20]): Boolean
|
||||
var
|
||||
RentalAgreement: Record "Rental Agreement";
|
||||
begin
|
||||
if not RentalAgreement.Get(AgreementNo) then
|
||||
exit(false);
|
||||
|
||||
RentalAgreement.Closed := true;
|
||||
RentalAgreement.Modify(true);
|
||||
exit(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: appsource
|
||||
keywords: [web-service, serviceenabled, guiallowed, message, confirm, strmenu]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Keep web-service paths free of UI calls
|
||||
|
||||
## Description
|
||||
|
||||
Pages and codeunits exposed as web services run without an interactive client. Calls that require a UI callback, including `Confirm`, `StrMenu`, and modal pages, can terminate the service request instead of completing the operation. `Message` does not raise the callback error: the message is suppressed and logged, making it ineffective for communicating a service result.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Keep service entry points and every procedure they call free of interactive UI. Return data through the service contract and report validation failures with service-safe error handling. When a procedure is shared with an interactive client, guard UI-only behavior with `GuiAllowed` while preserving the underlying operation.
|
||||
|
||||
See sample: [`keep-web-service-paths-free-of-ui-calls.good.al`](keep-web-service-paths-free-of-ui-calls.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A web-service-exposed page or codeunit calls an interactive UI method directly or indirectly. Detection signals include `Message`, `Confirm`, `StrMenu`, `Page.RunModal`, and confirmation-dialog pages on a service call path. Treat `Message` as suppressed and ineffective, not as a callback failure. Do not flag a controlled `Error` solely because it returns a service fault.
|
||||
|
||||
See sample: [`keep-web-service-paths-free-of-ui-calls.bad.al`](keep-web-service-paths-free-of-ui-calls.bad.al).
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
pageextension 50100 "Rental Customer List" extends "Customer List"
|
||||
{
|
||||
actions
|
||||
{
|
||||
addafter("Customer Ledger Entries")
|
||||
{
|
||||
action(OpenRentalAgreements)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Rental Agreements';
|
||||
RunObject = page "Rental Agreement List";
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
pageextension 50100 "Rental Customer List" extends "Customer List"
|
||||
{
|
||||
actions
|
||||
{
|
||||
addlast(Processing)
|
||||
{
|
||||
action(OpenRentalAgreements)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Rental Agreements';
|
||||
RunObject = page "Rental Agreement List";
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: appsource
|
||||
keywords: [pageextension, actions, addfirst, addlast, addbefore, addafter]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Place page extension actions with addfirst or addlast
|
||||
|
||||
## Description
|
||||
|
||||
Place new page-extension actions at the beginning or end of an existing action group with `addfirst` or `addlast`. Anchoring a new action relative to a specific base-app action with `addbefore` or `addafter` couples the extension to an implementation detail that can move or disappear between Business Central releases.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Choose the semantic action area or group and append or prepend the extension's actions. This keeps placement deterministic without depending on the continued existence of one neighboring action.
|
||||
|
||||
See sample: [`place-page-extension-actions-with-addfirst-or-addlast.good.al`](place-page-extension-actions-with-addfirst-or-addlast.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Using `addbefore` or `addafter` to place newly added actions next to a specific action from another app. The syntax is valid AL, but the placement anchor is brittle for a Marketplace extension.
|
||||
|
||||
See sample: [`place-page-extension-actions-with-addfirst-or-addlast.bad.al`](place-page-extension-actions-with-addfirst-or-addlast.bad.al).
|
||||
|
|
@ -0,0 +1,20 @@
|
|||
page 50100 "Rental Agreement List"
|
||||
{
|
||||
PageType = List;
|
||||
SourceTable = "Rental Agreement";
|
||||
ApplicationArea = All;
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
repeater(Agreements)
|
||||
{
|
||||
field("No."; Rec."No.")
|
||||
{
|
||||
ApplicationArea = All;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,21 @@
|
|||
page 50100 "Rental Agreement List"
|
||||
{
|
||||
PageType = List;
|
||||
SourceTable = "Rental Agreement";
|
||||
ApplicationArea = All;
|
||||
UsageCategory = Lists;
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
repeater(Agreements)
|
||||
{
|
||||
field("No."; Rec."No.")
|
||||
{
|
||||
ApplicationArea = All;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: appsource
|
||||
keywords: [usagecategory, tell-me, search, page, report, discoverability]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Set UsageCategory on searchable entry points
|
||||
|
||||
## Description
|
||||
|
||||
Pages and reports that users are expected to open directly must set `UsageCategory`. Without it, the object is absent from Tell Me and users cannot bookmark it from the web client. Supporting objects such as list parts, dialogs, API pages, and objects reached only through another page do not need to be searchable entry points.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Set `UsageCategory` to the category that matches the entry point, such as `Lists`, `Tasks`, `ReportsAndAnalysis`, or `Documents`. Also set the appropriate object-level `ApplicationArea` so search results respect feature visibility.
|
||||
|
||||
See sample: [`set-usagecategory-on-searchable-entry-points.good.al`](set-usagecategory-on-searchable-entry-points.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A user-facing page or report intended for direct discovery omits `UsageCategory` or sets it to `None`. Do not infer intent from the object type alone; require evidence that the object is a direct user entry point.
|
||||
|
||||
See sample: [`set-usagecategory-on-searchable-entry-points.bad.al`](set-usagecategory-on-searchable-entry-points.bad.al).
|
||||
|
|
@ -0,0 +1,10 @@
|
|||
codeunit 50100 "Rental Period Defaults"
|
||||
{
|
||||
procedure GetPolicyStartDate(): Date
|
||||
var
|
||||
PolicyStartDate: Date;
|
||||
begin
|
||||
Evaluate(PolicyStartDate, '01/31/2025');
|
||||
exit(PolicyStartDate);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,7 @@
|
|||
codeunit 50100 "Rental Period Defaults"
|
||||
{
|
||||
procedure GetPolicyStartDate(): Date
|
||||
begin
|
||||
exit(20250131D);
|
||||
end;
|
||||
}
|
||||
26
community/knowledge/appsource/use-invariant-date-literals.md
Normal file
26
community/knowledge/appsource/use-invariant-date-literals.md
Normal file
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: appsource
|
||||
keywords: [date-literal, invariant-date, dateformula, localization, appsourcecop]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use invariant date literals
|
||||
|
||||
## Description
|
||||
|
||||
Write fixed dates in AL with the invariant `yyyymmddD` syntax. A locale-dependent text value parsed with `Evaluate` can change meaning or fail under another user's regional settings, which makes the Marketplace extension unreliable across markets.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Represent a fixed date directly as an AL date literal, such as `20250131D`. Use `CalcDate` with a date formula when the value is relative rather than fixed.
|
||||
|
||||
See sample: [`use-invariant-date-literals.good.al`](use-invariant-date-literals.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Building a fixed date by passing localized text such as `01/02/2025` to `Evaluate`. Detection signal: `Evaluate` converting a hard-coded or label-backed formatted string into a `Date`.
|
||||
|
||||
See sample: [`use-invariant-date-literals.bad.al`](use-invariant-date-literals.bad.al).
|
||||
|
|
@ -161,12 +161,15 @@ If PyYAML is not installed in your development environment, install it with
|
|||
```powershell
|
||||
python .github\scripts\validate_frontmatter.py --root .
|
||||
pwsh .\tools\Test-ReviewFixtures.ps1 -Root .
|
||||
pwsh .\tools\Test-ReviewContract.ps1 -Root .
|
||||
```
|
||||
|
||||
The first command checks schema, sections, naming, sample references, and
|
||||
skill registration. The second checks that every review leaf has a valid
|
||||
positive/clean sample pair. Neither proves a model will find every defect.
|
||||
See [evaluation](../evaluation/README.md) for optional model-based scoring.
|
||||
positive/clean sample pair. The third checks the cross-surface findings-report
|
||||
contract and its bounded range-normalization cases. None proves a model will
|
||||
find every defect. See [evaluation](../evaluation/README.md) for optional
|
||||
model-based scoring.
|
||||
|
||||
In the PR description, explain the mistake being prevented, supporting
|
||||
evidence, applicable BC versions, and why the chosen domain owns it. For a
|
||||
|
|
|
|||
|
|
@ -52,10 +52,17 @@ only result.
|
|||
4. When an action skill declares `sub-skills`, execute every relevant leaf as a
|
||||
discrete invocation. Leaves are independent and may be scheduled serially
|
||||
or concurrently.
|
||||
5. Collect each complete findings-report into `sub-results` in the declared
|
||||
5. Capture the exact Task return as the immutable raw audit payload and primary
|
||||
transport. Preserve it unchanged in private artifacts or host logs. Before
|
||||
the full DO acceptance gate, create a normalized candidate only for DO's
|
||||
bounded optional-range case, record that normalization separately in private
|
||||
telemetry, and accept the candidate only if the entire copy passes the
|
||||
unchanged strict gate. The accepted report contains no undeclared telemetry
|
||||
fields.
|
||||
6. Collect each accepted findings-report into `sub-results` in the declared
|
||||
`sub-skills` order, not completion order. Run the super-skill self-review
|
||||
only after all leaves have finished.
|
||||
6. Apply the DO composition, failure, deduplication, reference-integrity, and
|
||||
7. Apply the DO composition, failure, deduplication, reference-integrity, and
|
||||
outcome rules. Return strict JSON before rendering it for people or another
|
||||
system.
|
||||
|
||||
|
|
@ -86,6 +93,15 @@ A compatible runner:
|
|||
- invokes every worklisted leaf exactly once unless a documented retry replaces
|
||||
a failed attempt;
|
||||
- keeps leaf contexts isolated and passes only the inputs they declare;
|
||||
- preserves each raw Task return unchanged for audit and distinguishes it from
|
||||
any normalized accepted copy;
|
||||
- removes only an optional range whose positive integer bounds contain the
|
||||
primary line but start before it, and only when the complete report has no
|
||||
other defect and the finding has no `suggested-code`;
|
||||
- records normalization only in private runner telemetry and never adds fields
|
||||
to the findings-report;
|
||||
- rejects reversed, invalid, or out-of-bounds ranges, range mismatches attached
|
||||
to `suggested-code`, and every repair outside DO's bounded exception;
|
||||
- preserves every leaf report, including failed reports, in `sub-results`;
|
||||
- excludes unreliable findings from failed leaves and returns `partial` when
|
||||
only part of the review is reliable;
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@
|
|||
|
||||
The evaluation is convention-driven. The harness discovers every `<layer>/skills/review/al-<domain>-review.md` leaf across the enabled `microsoft`, `community`, and `custom` layers. Duplicate domains resolve with `custom > community > microsoft` precedence. For each selected leaf, the harness finds paired knowledge across the same layers, applies the same precedence to duplicate article slugs, selects the first article (by filename) with both `.bad.al` and `.good.al` companions, and derives the expected positive and clean control automatically. Adding a conforming leaf requires no scoring-contract edit.
|
||||
|
||||
`review-fixtures.json` contains only global thresholds and optional exceptional overrides. An override may select a different article or add context when the generic convention cannot express a scenario. It should remain empty in the normal case.
|
||||
`review-fixtures.json` contains only global thresholds and optional exceptional overrides. An override may select a different `article`, add context when the generic convention cannot express a scenario, or use an `articles` array when one domain needs explicit regression coverage for several paired articles. Specify either `article` or `articles`, not both. The first selected article retains the stable `<domain>-bad` and `<domain>-good` manifest IDs; additional articles use slug-qualified IDs. Overrides should remain empty in the normal case.
|
||||
|
||||
Model-facing preparation hashes case IDs, neutralizes `Good`/`Bad` object-name tokens, and removes full-line sample comments so neither the article slug, domain, nor expected outcome reveals the answer.
|
||||
|
||||
|
|
@ -26,7 +26,7 @@ This credential-free check proves every selected leaf maps to a same-named knowl
|
|||
|
||||
2. For a fast/small model, use one fresh invocation per `request-case-*.json`. Each request embeds the exact leaf instructions, that domain's candidate index rows with authoritative paths, and one opaque case. The model opens only matching articles and copies finding IDs from `candidateArticles[].path`. Save each response with the matching `result-case-*.json` name in the same directory.
|
||||
|
||||
`request-<domain>.json` files provide optional two-case leaf batches and identify the selected layer-owned skill path; save those as `result-<domain>.json`. Directory scoring prefers `result-case-*.json` when present and otherwise falls back to `result-*.json`. `review-request.json` is an optional all-domains stress test for larger models. Neither batch form is the preferred fast-model profile.
|
||||
`request-<domain>.json` files provide optional leaf batches containing every selected case for that domain and identify the selected layer-owned skill path; save those as `result-<domain>.json`. A normal convention-selected domain has one bad/good pair, while an `articles` override contributes one pair per listed article. Directory scoring prefers `result-case-*.json` when present and otherwise falls back to `result-*.json`. `review-request.json` is an optional all-domains stress test for larger models. Neither batch form is the preferred fast-model profile.
|
||||
|
||||
3. Save only this result shape:
|
||||
|
||||
|
|
|
|||
|
|
@ -17,7 +17,15 @@
|
|||
"article": "set-defaultimplementation-on-enum"
|
||||
},
|
||||
"performance": {
|
||||
"article": "use-isempty-for-existence-check"
|
||||
"articles": [
|
||||
"use-isempty-for-existence-check",
|
||||
"job-queue-category-code-serializes-conflicting-jobs",
|
||||
"job-queue-external-effects-must-be-idempotent",
|
||||
"job-queue-handlers-must-not-require-ui",
|
||||
"job-queue-handlers-must-propagate-failures",
|
||||
"job-queue-on-hold-does-not-stop-running-work",
|
||||
"store-scheduled-task-id-to-avoid-duplicate-tasks"
|
||||
]
|
||||
},
|
||||
"privacy": {
|
||||
"article": "no-pii-in-telemetry-message-string"
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ The first database write opens an AL write transaction that the runtime holds un
|
|||
|
||||
## Best Practice
|
||||
|
||||
Defer the HTTP call to a separate session. When the external operation must correspond to a committed database change, insert an outbox work item in the same transaction as that change and process committed outbox rows with a recurring job queue entry. The change and work item then commit or roll back together, and the worker performs HTTP before deleting the item so it holds no write lock during the call. Make the external operation idempotent because a failure after a successful HTTP response can cause the work item to be retried.
|
||||
Defer the HTTP call to a separate session. When the external operation must correspond to a committed database change, insert an outbox work item in the same transaction as that change and process committed outbox rows with a recurring job queue entry. The change and work item then commit or roll back together, and the worker performs HTTP before deleting the item so it holds no write lock during the call. The separate retry-safety requirement is covered by `job-queue-external-effects-must-be-idempotent.md`.
|
||||
|
||||
A directly created scheduled task is suitable only when its work is independent of the caller's commit. An immediately ready task can run concurrently with the caller, so it must not assume that the caller's writes are already committed. Do **not** use `Commit()` as a general remedy: it irrevocably commits all prior writes in the current transaction, so any subsequent failure cannot roll them back. `Commit()` is appropriate only at top-level entry points where partial persistence is intentional and understood.
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,11 @@
|
|||
codeunit 50113 "Job Queue Category Bad"
|
||||
{
|
||||
procedure ConfigureJobsForSharedExclusiveResource(var SalesPostingJob: Record "Job Queue Entry"; var PurchasePostingJob: Record "Job Queue Entry"; ExclusiveResourceId: Text[250])
|
||||
begin
|
||||
// Both jobs update the same posting resources, but nothing prevents overlap.
|
||||
SalesPostingJob.Validate("Parameter String", ExclusiveResourceId);
|
||||
PurchasePostingJob.Validate("Parameter String", ExclusiveResourceId);
|
||||
SalesPostingJob.Validate("Job Queue Category Code", '');
|
||||
PurchasePostingJob.Validate("Job Queue Category Code", '');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
codeunit 50113 "Job Queue Category Good"
|
||||
{
|
||||
procedure ConfigureJobsForSharedExclusiveResource(var SalesPostingJob: Record "Job Queue Entry"; var PurchasePostingJob: Record "Job Queue Entry"; ExclusiveResourceId: Text[250])
|
||||
var
|
||||
JobQueueCategory: Record "Job Queue Category";
|
||||
begin
|
||||
if not JobQueueCategory.Get('POSTING') then begin
|
||||
JobQueueCategory.Code := 'POSTING';
|
||||
JobQueueCategory.Insert();
|
||||
end;
|
||||
|
||||
// The shared category lets only one conflicting posting job run at a time.
|
||||
SalesPostingJob.Validate("Parameter String", ExclusiveResourceId);
|
||||
PurchasePostingJob.Validate("Parameter String", ExclusiveResourceId);
|
||||
SalesPostingJob.Validate("Job Queue Category Code", 'POSTING');
|
||||
PurchasePostingJob.Validate("Job Queue Category Code", 'POSTING');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [job-queue, category-code, concurrency, waiting, serialization, locking]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use a job queue category to serialize conflicting jobs
|
||||
|
||||
> Contributions welcome — open a PR to refine or extend this article.
|
||||
|
||||
## Description
|
||||
|
||||
Different job queue entries can run at the same time. When two jobs update the same exclusive resource, concurrent execution can cause lock contention, deadlocks, or conflicting results. Within one company, entries with the same Job Queue Category Code are serialized: while one runs, another entry in that category waits.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Assign the same non-empty Job Queue Category Code to job queue entries in the same company that must not overlap, regardless of which codeunit they run. Define categories around the shared resource or exclusivity requirement, not merely around object names. Leave independent jobs in different categories so they can still run concurrently. A category does not serialize work across companies or environments, or coordinate workers outside the job queue dispatcher. Protect shared external or cross-company resources with a separate application-level locking mechanism.
|
||||
|
||||
See sample: [`job-queue-category-code-serializes-conflicting-jobs.good.al`](job-queue-category-code-serializes-conflicting-jobs.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Creating or configuring multiple job queue entries that update the same exclusive resource while leaving their Job Queue Category Code empty or different. Do not flag jobs merely because they touch the same tables; the rule applies when their operation requires mutual exclusion.
|
||||
|
||||
See sample: [`job-queue-category-code-serializes-conflicting-jobs.bad.al`](job-queue-category-code-serializes-conflicting-jobs.bad.al).
|
||||
|
|
@ -0,0 +1,57 @@
|
|||
table 50112 "Queued Export Bad"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "Entry No."; Integer)
|
||||
{
|
||||
AutoIncrement = true;
|
||||
}
|
||||
field(2; Payload; Text[250])
|
||||
{
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Entry No.")
|
||||
{
|
||||
Clustered = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50112 "Queued Export Worker Bad"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
var
|
||||
QueuedExport: Record "Queued Export Bad";
|
||||
Client: HttpClient;
|
||||
Content: HttpContent;
|
||||
Response: HttpResponseMessage;
|
||||
begin
|
||||
if not QueuedExport.FindFirst() then
|
||||
exit;
|
||||
|
||||
Content.WriteFrom(QueuedExport.Payload);
|
||||
Client.Post('https://example.local/exports', Content, Response);
|
||||
if not Response.IsSuccessStatusCode() then
|
||||
Error('Export failed with HTTP status %1.', Response.HttpStatusCode());
|
||||
|
||||
// If this local step fails, the external export exists but this row is retried.
|
||||
UpdateLocalStatus();
|
||||
FinalizeExport(QueuedExport);
|
||||
end;
|
||||
|
||||
local procedure UpdateLocalStatus()
|
||||
begin
|
||||
end;
|
||||
|
||||
local procedure FinalizeExport(var QueuedExport: Record "Queued Export Bad")
|
||||
begin
|
||||
QueuedExport.Delete();
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,65 @@
|
|||
table 50112 "Queued Export Good"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "Entry No."; Integer)
|
||||
{
|
||||
AutoIncrement = true;
|
||||
}
|
||||
field(2; Payload; Text[250])
|
||||
{
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Entry No.")
|
||||
{
|
||||
Clustered = true;
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
codeunit 50112 "Queued Export Worker Good"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
var
|
||||
QueuedExport: Record "Queued Export Good";
|
||||
Client: HttpClient;
|
||||
Content: HttpContent;
|
||||
ContentHeaders: HttpHeaders;
|
||||
JsonPayload: JsonObject;
|
||||
RequestBody: Text;
|
||||
Response: HttpResponseMessage;
|
||||
begin
|
||||
if not QueuedExport.FindFirst() then
|
||||
exit;
|
||||
|
||||
JsonPayload.Add('idempotencyKey', Format(QueuedExport.SystemId));
|
||||
JsonPayload.Add('payload', QueuedExport.Payload);
|
||||
JsonPayload.WriteTo(RequestBody);
|
||||
|
||||
Content.WriteFrom(RequestBody);
|
||||
Content.GetHeaders(ContentHeaders);
|
||||
ContentHeaders.Clear();
|
||||
ContentHeaders.Add('Content-Type', 'application/json');
|
||||
Client.Post('https://example.local/exports', Content, Response);
|
||||
if not Response.IsSuccessStatusCode() then
|
||||
Error('Export failed with HTTP status %1.', Response.HttpStatusCode());
|
||||
|
||||
// The external service must atomically create a record only when idempotencyKey
|
||||
// does not exist. When the key already exists, it must return the existing record
|
||||
// without repeating the side effect.
|
||||
UpdateLocalStatus();
|
||||
QueuedExport.Delete();
|
||||
end;
|
||||
|
||||
local procedure UpdateLocalStatus()
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [job-queue, idempotency, retry, outbox, httpclient, external-effect]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Job queue external effects must be idempotent
|
||||
|
||||
> Contributions welcome — open a PR to refine or extend this article.
|
||||
|
||||
## Description
|
||||
|
||||
A job queue handler can successfully create something in an external system and then fail while updating Business Central. Business Central rolls back its database changes, but it cannot roll back the external request. The same work can later run again through configured retries, recurrence, rescheduling, or manual restart. Without a way for the external system to recognize the repeated request, a later run can create a duplicate shipment, payment, notification, or other side effect.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use a stable request ID that exists before the job queue processes the outbox row. For example, include the outbox record's `SystemId` as an `idempotencyKey` value in the JSON body of every POST attempt. The external service must enforce uniqueness on that value: when it receives the key again, it returns the existing record instead of creating another one. Delete the outbox row only after the external call and all required local updates succeed.
|
||||
|
||||
A `Processed` flag set after the external call does not solve this failure window. If a later AL error rolls back that flag, the outbox row again looks unprocessed even though the external operation already happened.
|
||||
|
||||
See sample: [`job-queue-external-effects-must-be-idempotent.good.al`](job-queue-external-effects-must-be-idempotent.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Sending a state-changing request from a job queue handler with no stable request ID understood by the external API. Specifically, look for this sequence: read an outbox row, call `HttpClient.Post` or another side-effecting API, update or delete local data, and propagate an error after which the same outbox row can be processed again. The key may be part of the request body, URI, headers, or an existing business key; a naturally idempotent remote operation is already safe and should not be flagged.
|
||||
|
||||
See sample: [`job-queue-external-effects-must-be-idempotent.bad.al`](job-queue-external-effects-must-be-idempotent.bad.al).
|
||||
|
|
@ -0,0 +1,17 @@
|
|||
codeunit 50110 "Job Queue UI Bad"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
begin
|
||||
if not Confirm('Process the queued export now?') then
|
||||
exit;
|
||||
|
||||
ProcessExport(Rec."Parameter String");
|
||||
Message('The queued export completed.');
|
||||
end;
|
||||
|
||||
local procedure ProcessExport(ParameterString: Text)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 50110 "Job Queue UI Good"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
begin
|
||||
Rec.TestField("Parameter String");
|
||||
ProcessExport(Rec."Parameter String");
|
||||
end;
|
||||
|
||||
local procedure ProcessExport(ParameterString: Text)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [job-queue, background-session, guiallowed, confirm, runmodal, client-callback]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Job queue handlers must not require user interaction
|
||||
|
||||
> Contributions welcome — open a PR to refine or extend this article.
|
||||
|
||||
## Description
|
||||
|
||||
A job queue handler runs in a background session with no client UI. Calls that require a client callback, such as `Confirm`, `Page.RunModal`, `Report.RunModal`, upload, or download, can stop the job with a non-retriable callback error. `Message` is suppressed and logged by the server, so it cannot communicate a result to the user who scheduled the job.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Make a dedicated job queue entry point non-interactive. Validate parameters and data in AL, persist business-visible status when needed, and let failures propagate to the job queue log. If one procedure genuinely serves both foreground and background callers, isolate optional UI-only behavior behind `GuiAllowed`; do not use the guard to silently skip a decision that the operation requires.
|
||||
|
||||
See sample: [`job-queue-handlers-must-not-require-ui.good.al`](job-queue-handlers-must-not-require-ui.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `Confirm`, `Page.Run`, `Page.RunModal`, `Report.Run`, `Report.RunModal`, `Hyperlink`, `File.Upload`, or `File.Download` from a codeunit run by the job queue. Another signal is using `Message` as the only success or failure notification: no user is attached to receive it.
|
||||
|
||||
See sample: [`job-queue-handlers-must-not-require-ui.bad.al`](job-queue-handlers-must-not-require-ui.bad.al).
|
||||
|
|
@ -0,0 +1,23 @@
|
|||
codeunit 50111 "Job Queue Failure Bad"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
begin
|
||||
if not TryProcessCustomer(Rec."Parameter String") then
|
||||
exit;
|
||||
end;
|
||||
|
||||
[TryFunction]
|
||||
local procedure TryProcessCustomer(CustomerNo: Code[20])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.Get(CustomerNo);
|
||||
ProcessCustomer(Customer);
|
||||
end;
|
||||
|
||||
local procedure ProcessCustomer(Customer: Record Customer)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,16 @@
|
|||
codeunit 50111 "Job Queue Failure Good"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.Get(Rec."Parameter String");
|
||||
ProcessCustomer(Customer);
|
||||
end;
|
||||
|
||||
local procedure ProcessCustomer(Customer: Record Customer)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [job-queue, error-propagation, tryfunction, retry, dispatcher, job-queue-log]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Job queue handlers must propagate execution failures
|
||||
|
||||
> Contributions welcome — open a PR to refine or extend this article.
|
||||
|
||||
## Description
|
||||
|
||||
The job queue dispatcher can mark an entry as failed, record the error, and apply its configured retry behavior only when the handler terminates with an error. A handler that catches a failed `TryFunction` or Boolean-returning operation and then returns normally reports success to the dispatcher, even though its work did not complete.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Let an error that invalidates the whole run propagate out of the job queue entry point. Add context only when it helps an operator diagnose the failure and does not expose sensitive data. Per-item failures may be collected deliberately, but the batch must persist or emit an observable aggregate outcome instead of silently treating incomplete work as success.
|
||||
|
||||
See sample: [`job-queue-handlers-must-propagate-failures.good.al`](job-queue-handlers-must-propagate-failures.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling a `TryFunction`, `Codeunit.Run`, or another Boolean-returning operation from a job queue handler and using `exit` or normal fall-through on failure without recording an intentional partial-success outcome. The dispatcher sees a successful return, so the entry's status and log do not represent the failed work and configured retries are not applied.
|
||||
|
||||
See sample: [`job-queue-handlers-must-propagate-failures.bad.al`](job-queue-handlers-must-propagate-failures.bad.al).
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
codeunit 50114 "Job Queue On Hold Bad"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
begin
|
||||
repeat
|
||||
if not ProcessNextBatch() then
|
||||
exit;
|
||||
Rec.Get(Rec.ID);
|
||||
until Rec.Status = Rec.Status::"On Hold";
|
||||
end;
|
||||
|
||||
local procedure ProcessNextBatch(): Boolean
|
||||
begin
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,49 @@
|
|||
table 50114 "Job Cancellation Control"
|
||||
{
|
||||
DataClassification = SystemMetadata;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "Job Queue Entry ID"; Guid)
|
||||
{
|
||||
}
|
||||
field(2; "Stop Requested"; Boolean)
|
||||
{
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Job Queue Entry ID")
|
||||
{
|
||||
Clustered = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50114 "Job Queue On Hold Good"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
begin
|
||||
while not IsStopRequested(Rec.ID) do
|
||||
if not ProcessNextBatch() then
|
||||
exit;
|
||||
end;
|
||||
|
||||
local procedure IsStopRequested(JobQueueEntryId: Guid): Boolean
|
||||
var
|
||||
JobCancellationControl: Record "Job Cancellation Control";
|
||||
begin
|
||||
if not JobCancellationControl.Get(JobQueueEntryId) then
|
||||
exit(false);
|
||||
|
||||
exit(JobCancellationControl."Stop Requested");
|
||||
end;
|
||||
|
||||
local procedure ProcessNextBatch(): Boolean
|
||||
begin
|
||||
exit(false);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [job-queue, on-hold, cancellation, in-process, long-running, stop-request]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Putting a job queue entry on hold does not stop its current run
|
||||
|
||||
> Contributions welcome — open a PR to refine or extend this article.
|
||||
|
||||
## Description
|
||||
|
||||
The On Hold status prevents a job queue entry from starting again, but it does not cancel a run that is already in process. A long-running handler continues until it completes, fails, reaches a cancellation point implemented by the application, or its session is stopped externally.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use On Hold to pause future scheduling. When a long-running operation must support graceful cancellation, store a separate application-owned stop request and check it before every bounded unit of work, including the first. Exit only at a point where completed work and the checkpoint are consistent. The code that resumes scheduling must clear the stop request before restarting the job. Use administrative session termination only when graceful cancellation is impossible.
|
||||
|
||||
See sample: [`job-queue-on-hold-does-not-stop-running-work.good.al`](job-queue-on-hold-does-not-stop-running-work.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Polling the job queue entry's Status field from inside its handler and expecting a change to On Hold to cancel the active run. The status controls scheduling, not cooperative cancellation, so the handler can continue processing despite the operator's action.
|
||||
|
||||
See sample: [`job-queue-on-hold-does-not-stop-running-work.bad.al`](job-queue-on-hold-does-not-stop-running-work.bad.al).
|
||||
|
|
@ -17,7 +17,7 @@ application-area: [all]
|
|||
|
||||
## Best Practice
|
||||
|
||||
Keep company-open subscribers to cheap in-memory work: set a flag, enqueue a job-queue entry, or `TaskScheduler.CreateTask`. Perform HTTP and large SQL after the session is running, in that background work.
|
||||
Keep company-open subscribers to cheap in-memory work: set a flag, enqueue a job-queue entry, or `TaskScheduler.CreateTask`. Perform HTTP and large SQL after the session is running, in that background work. When the subscriber can run repeatedly, use `store-scheduled-task-id-to-avoid-duplicate-tasks.md` to avoid creating the same logical task more than once.
|
||||
|
||||
See sample: [`oncompanyopen-subscribers-must-not-do-io.good.al`](oncompanyopen-subscribers-must-not-do-io.good.al).
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,15 @@
|
|||
codeunit 50115 "Scheduled Task Duplicate Bad"
|
||||
{
|
||||
procedure EnsureCleanupTask()
|
||||
begin
|
||||
// Every call creates another task for the same cleanup work.
|
||||
TaskScheduler.CreateTask(Codeunit::"Scheduled Cleanup Work Bad", 0, true, CompanyName());
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50116 "Scheduled Cleanup Work Bad"
|
||||
{
|
||||
trigger OnRun()
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,23 @@
|
|||
codeunit 50115 "Scheduled Task Duplicate Good"
|
||||
{
|
||||
internal procedure EnsureCleanupTask()
|
||||
var
|
||||
TaskId: Guid;
|
||||
StoredTaskId: Text;
|
||||
begin
|
||||
if IsolatedStorage.Get('CleanupTaskId', DataScope::Company, StoredTaskId) then
|
||||
if Evaluate(TaskId, StoredTaskId) then
|
||||
if TaskScheduler.TaskExists(TaskId) then
|
||||
exit;
|
||||
|
||||
TaskId := TaskScheduler.CreateTask(Codeunit::"Scheduled Cleanup Work Good", 0, true, CompanyName());
|
||||
IsolatedStorage.Set('CleanupTaskId', Format(TaskId), DataScope::Company);
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50116 "Scheduled Cleanup Work Good"
|
||||
{
|
||||
trigger OnRun()
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [task-scheduler, scheduled-task, taskexists, duplicate-task, createtask, guid]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Store the scheduled task ID to avoid duplicate tasks
|
||||
|
||||
> Contributions welcome — open a PR to refine or extend this article.
|
||||
|
||||
## Description
|
||||
|
||||
Every call to `TaskScheduler.CreateTask` creates a new scheduled task and returns its unique GUID. Repeating setup or lifecycle code without retaining that GUID can create multiple tasks for the same logical work, consuming scheduler capacity and running the work more than once.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Persist the GUID returned by `CreateTask` at the same scope as the logical task. Before creating a replacement, parse the stored GUID and call `TaskScheduler.TaskExists`; create and store a new task only when the previous task no longer exists. `TaskExists` checks one GUID, not whether an equivalent codeunit is already scheduled, so callers that can schedule concurrently still need serialization around this check-and-create sequence.
|
||||
|
||||
See sample: [`store-scheduled-task-id-to-avoid-duplicate-tasks.good.al`](store-scheduled-task-id-to-avoid-duplicate-tasks.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `TaskScheduler.CreateTask` every time initialization, login, setup, or another repeatable path runs while ignoring its return value. Each invocation creates another independent task even when an equivalent task is already pending.
|
||||
|
||||
See sample: [`store-scheduled-task-id-to-avoid-duplicate-tasks.bad.al`](store-scheduled-task-id-to-avoid-duplicate-tasks.bad.al).
|
||||
|
|
@ -16,7 +16,7 @@ application-area: [all]
|
|||
|
||||
Reviews AL source and app metadata changes against the `appsource` 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`. AppSource findings are narrow by design — they apply to AppSource-facing metadata and complete permission coverage that requires repository context. Mechanical AppSourceCop diagnostics are intentionally outside this skill. The skill returns `not-applicable` when none of those surfaces apply.
|
||||
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. AppSource findings are narrow by design — they apply to Marketplace-facing metadata, complete permission coverage that requires repository context, and contextual AL constructs covered by Marketplace submission requirements. Mechanical compiler and analyzer diagnostics are intentionally outside this skill. The skill returns `not-applicable` when none of those surfaces apply.
|
||||
|
||||
## Source
|
||||
|
||||
|
|
@ -37,14 +37,20 @@ Discard files that are not applicable. Retain conditionally applicable files (an
|
|||
|
||||
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed files and AL object types — especially `app.json`, permission-set objects, setup and usage entry points, and AppSource-facing help metadata.
|
||||
- Tokens extracted from the diff that relate to AppSource (`permissionset`, `Assignable`, `Permissions`, `SUPER`, `tabledata`, `execute`, `app.json`, `help`, `ContextSensitiveHelpPage`, `Copilot`, `https`).
|
||||
- The changed files and AL object types — especially `app.json`, permission-set and profile objects, setup and usage entry points, service-enabled procedures, user-facing pages and reports, and AppSource-facing help metadata.
|
||||
- Tokens extracted from the diff that relate to AppSource (`permissionset`, `Assignable`, `Permissions`, `SUPER`, `tabledata`, `execute`, `profile`, `Record Profile`, `Evaluate`, `Date`, `DateTime`, `CurrentDateTime`, `UsageCategory`, `PageType`, `addfirst`, `addlast`, `addbefore`, `addafter`, `ServiceEnabled`, `GuiAllowed`, `Message`, `Confirm`, `StrMenu`, `RunModal`, `app.json`, `help`, `ContextSensitiveHelpPage`, `Copilot`, `https`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no AppSource-related source or metadata changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files.
|
||||
|
||||
The following targeted checks cover every current `appsource` article across the Microsoft and community layers. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action.
|
||||
|
||||
- The app has no assignable permission set covering its setup and usage paths, omits visible object/tabledata grants, or requires `SUPER` for normal operation — `permission-sets-cover-setup-and-usage-without-super`. Require repository-level app context; one isolated permission-set object cannot prove complete coverage.
|
||||
- Install, upgrade, or setup code provisions an app-owned profile through `Record Profile` and `Insert` instead of declaring a `profile` object — `define-profiles-as-al-objects`.
|
||||
- A hard-coded or label-backed formatted string is converted to `Date` with `Evaluate` — `use-invariant-date-literals`. Do not select this article for variable external input whose format must be validated at runtime.
|
||||
- A page extension uses `addbefore` or `addafter` to place a newly added action relative to a specific action owned by another app — `place-page-extension-actions-with-addfirst-or-addlast`. Do not flag those keywords in layouts or placement relative to an action owned by the same extension.
|
||||
- A page or codeunit web-service entry point, including a `[ServiceEnabled]` procedure, contains or reaches `Message`, `Confirm`, `StrMenu`, `Page.RunModal`, or a confirmation-dialog page without an effective non-GUI guard — `keep-web-service-paths-free-of-ui-calls`. Treat `Message` as suppressed and logged, making it ineffective as a service response; treat the other UI calls as callback-failure risks. Do not treat a controlled `Error` as interactive UI solely because it returns a service fault.
|
||||
- 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`.
|
||||
|
||||
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`.
|
||||
|
|
@ -75,7 +81,7 @@ Outcome selection:
|
|||
|
||||
- `completed` — the skill evaluated every worklist item.
|
||||
- `no-knowledge` — no applicable AppSource knowledge survived filtering.
|
||||
- `not-applicable` — the diff touches no AppSource permission or app-metadata surface.
|
||||
- `not-applicable` — the diff touches no Marketplace-related source, permission, or app-metadata surface.
|
||||
- `partial` — a budget was hit before the worklist was exhausted.
|
||||
- `failed` — an unrecoverable error occurred.
|
||||
|
||||
|
|
|
|||
|
|
@ -72,7 +72,7 @@ The Action step consists of **discrete leaf invocations**, not one combined gene
|
|||
|
||||
- **Isolate leaf invocations when the host supports it.** Each sub-skill SHOULD run in a fresh model call or child context containing only its assigned source paths, READ/DO contracts, the leaf instructions, the complete bounded domain catalog per READ, and articles that leaf worklists. Preserve each catalog row's exact `path`; the leaf must copy references from that catalog.
|
||||
- **Keep run artifacts private.** Before dispatch, allocate a new GUID-named directory under the current session's artifact directory and a distinct scratch/report child directory for every leaf. Pass a leaf only its own assigned source paths and child directory, never the run root or sibling paths. A leaf MUST NOT discover, enumerate, read, modify, or delete sibling artifacts. Do not reuse a prior run directory, and do not clean up any run artifact until every leaf has finished and consolidation is complete.
|
||||
- **Use the exact Task return as the report.** Capture each leaf's exact return as the primary transport and apply DO's consumer acceptance gate before rollup. Worker-side persistence of the same report in its private directory is optional and redundant; a missing report file does not invalidate an otherwise valid exact return.
|
||||
- **Keep raw Task transport distinct from the accepted copy.** Capture the exact Task return as the immutable raw audit payload and primary transport. Preserve it unchanged in the leaf's private artifacts or host log. Then apply DO's bounded pre-gate range normalization, when eligible, and its full consumer acceptance gate. The report accepted for rollup is the exact return when no normalization occurred, or the normalized candidate copy when DO permits it; worker-side persistence of another report file is optional and redundant.
|
||||
- **Treat automatic output spills as host-owned.** If the host reports that a Task return was automatically spilled, the coordinator MAY read that file read-only only at the exact path returned by the tool. Never modify, delete, enumerate around, or reuse an automatic spill path. Never bypass a content-exclusion or access denial.
|
||||
- Treat each sub-skill in the worklist as its own pass: read the sub-skill's instructions, apply its Source → Relevance → Worklist → Action steps to the orchestrator-supplied inputs, and produce that sub-skill's complete findings-report independently.
|
||||
- Do not collapse multiple sub-skills into one shared reasoning step. Each sub-skill has a distinct knowledge subset and a distinct evaluation procedure; sharing one rolled-up scan dilutes per-skill attention and causes leaves to silently underreport (this has been observed in production: leaf skills returned empty `findings[]` while their standalone runs against the same diff produced multiple matches).
|
||||
|
|
@ -85,7 +85,7 @@ The Action step consists of **discrete leaf invocations**, not one combined gene
|
|||
For each sub-skill in the worklist:
|
||||
|
||||
1. Invoke the sub-skill with the orchestrator's inputs, passing only the subset each sub-skill declares in its `inputs`.
|
||||
2. Capture the exact Task return and validate it against DO's consumer acceptance gate before accepting it. Preserve an invalid raw return unchanged in the leaf's private artifacts or host log; do not reconstruct or repair it. Record a separate failed validation result with no findings for rollup.
|
||||
2. Capture the exact Task return as the immutable raw audit payload and primary transport. Preserve it unchanged in the leaf's private artifacts or host log before deriving a candidate. Apply only DO's bounded pre-gate normalization: when the complete raw report has no other defect, a finding has positive-integer `line`, `start-line`, and `end-line`, `start-line <= line <= end-line`, `start-line != line`, and no `suggested-code` field, copy the complete report and remove only that finding's optional `location.range`. Record the normalization separately in private run telemetry or artifacts, never in the findings-report. Validate the entire candidate through DO's existing strict acceptance gate. Accept the exact return when unchanged or the normalized candidate when it passes; otherwise record a separate failed validation result with no findings for rollup. Do not reconstruct JSON, infer fields, alter paths or references, clamp lines, normalize reversed or out-of-bounds ranges, remove a range associated with `suggested-code`, or salvage individual findings.
|
||||
3. Append the accepted findings-report, or the separate failed validation result, to `sub-results`. If its `outcome` is `failed`, stop here for this sub-skill: its findings are not reliable per the DO contract and MUST NOT be copied into the super-skill's top-level `findings[]` or counted in `summary.counts`.
|
||||
4. Otherwise, compare each entry from the sub-skill's `findings[]` with findings already rolled up. Two findings are duplicates when they point to the same file and overlapping line/range and prescribe materially the same correction, even when their knowledge-file IDs differ. Merge duplicates instead of appending both: keep the more specific domain owner, preserve that finding's optional `domain` field verbatim (including its absence), use its reference as `references[0]` and therefore as `id`, append the other references as supporting references, keep the highest severity and confidence justified by either report, and preserve one self-contained message. Article and leaf ownership notes decide specificity; do not choose by execution order.
|
||||
5. Append each non-duplicate finding, setting `from-sub-skill` to the sub-skill's `skill.id` and preserving its optional `domain` field verbatim, including its absence. For non-citation findings (those whose `id` is a skill-defined slug rather than a reference path), prefix `id` with `<from-sub-skill>:` to prevent collisions across sub-skills. Other finding fields are preserved.
|
||||
|
|
@ -131,9 +131,12 @@ Calculate `summary.counts` from the final top-level `findings[]`, after failed s
|
|||
Derive `outcome` using the DO rollup rules. `outcome-reason` is populated for `partial` and `failed` and SHOULD summarize per-sub-skill state, for example: *"al-security-review failed (tool timeout); al-performance-review completed."*
|
||||
|
||||
Before emitting the rollup, apply DO's consumer acceptance gate to every nested
|
||||
and top-level finding. Treat an invalid sub-result as failed and exclude all of
|
||||
its findings from the top-level rollup. Preserve its exact raw payload
|
||||
separately; never reconstruct it into a success-shaped report.
|
||||
and top-level finding. A leaf's nested report is its accepted exact return or
|
||||
its accepted normalized candidate copy; its exact Task return remains the
|
||||
separate immutable raw audit payload. Treat an invalid sub-result as failed and
|
||||
exclude all of its findings from the top-level rollup. Never reconstruct it
|
||||
into a success-shaped report or perform normalization beyond DO's bounded
|
||||
exception.
|
||||
|
||||
## Output
|
||||
|
||||
|
|
|
|||
|
|
@ -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 and hot-path costs (`SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `ReadIsolation`, `LockTable`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`, `FlowField`, `Visible`).
|
||||
- Tokens extracted from the diff that relate to data access, hot-path costs, and background scheduling (`SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `ReadIsolation`, `LockTable`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`, `FlowField`, `Visible`, `Job Queue Entry`, `Job Queue Category Code`, `Confirm`, `RunModal`, `GuiAllowed`, `TryFunction`, `Codeunit.Run`, `HttpClient`, `Status`, `On Hold`, `stop request`, `TaskScheduler.CreateTask`, `TaskScheduler.TaskExists`).
|
||||
|
||||
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.
|
||||
|
||||
|
|
@ -52,6 +52,12 @@ Apply these targeted cues even when simple token overlap would rank the article
|
|||
- Worklist `avoid-cloning-records-before-modify-delete-in-loops.md` when an iteration calls `Copy` or `RecordRef.GetTable` before `Modify`/`Delete`, or passes the iterated record without `var` to a helper that writes that record. Do not worklist it from `Modify`, `Delete`, or `RecordRef` alone; exclude a direct write on the iterator, a read-only copy, a temporary record, a different target table, and a `RecordRef` opened and iterated directly.
|
||||
- Worklist `use-tryfunction-for-error-catching-not-rollback.md` only when writes occur inside a try method and the code or surrounding flow expects an error to roll them back. A bare try-method call whose Boolean result is ignored belongs exclusively to `error-handling/ignored-tryfunction-return-disables-try-semantics.md`; do not worklist the performance article from that call shape alone.
|
||||
- For `LockTable` in a pure read helper, select exactly one owner. Use `do-not-locktable-in-read-only-procedure.md` when the helper needs no stronger isolation and should remove the lock. Use `prefer-readisolation-over-locktable-for-reads.md` instead when the code explicitly requires committed-read semantics and `ReadIsolation` is the replacement. Never emit both findings for the same call.
|
||||
- Worklist `job-queue-handlers-must-not-require-ui.md` when a codeunit run by the job queue calls `Confirm`, `Page.Run`, `Page.RunModal`, `Report.Run`, `Report.RunModal`, `Hyperlink`, `File.Upload`, or `File.Download`, or uses `Message` as its only success or failure notification. Exclude optional UI-only behavior guarded by `GuiAllowed`; do not exclude a guard that silently skips a decision required by the operation.
|
||||
- Worklist `job-queue-handlers-must-propagate-failures.md` when a codeunit run by the job queue handles a failed `TryFunction`, `Codeunit.Run`, or another Boolean-returning operation with `exit` or normal fall-through, causing the dispatcher to observe success. Exclude intentional partial-success handling that persists or emits an observable aggregate outcome. A bare try-method call whose Boolean result is ignored remains owned exclusively by `error-handling/ignored-tryfunction-return-disables-try-semantics.md`.
|
||||
- Worklist `job-queue-external-effects-must-be-idempotent.md` when rerunnable job queue work reads an outbox row, performs a state-changing external request, then updates or deletes local data without sending a stable request ID understood by the external system. Exclude naturally idempotent operations and requests whose body, URI, headers, or business key lets the external service return the existing result instead of repeating the side effect.
|
||||
- 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.
|
||||
|
||||
These targeted inclusions and exclusions override generic token overlap. Do not retain an excluded article solely because the diff contains one of its keywords.
|
||||
|
||||
|
|
|
|||
57
skills/do.md
57
skills/do.md
|
|
@ -167,13 +167,49 @@ AL source is the common failure case. Quoted identifiers (for example `Rec."No."
|
|||
|
||||
### Consumer acceptance gate
|
||||
|
||||
The exact action-skill return is the primary report transport. Before accepting
|
||||
it as a findings-report, a coordinator or host MUST validate it
|
||||
deterministically:
|
||||
Capture the exact Task return as the immutable raw audit payload and primary
|
||||
transport. Preserve it unchanged in private run artifacts or host logs before
|
||||
creating any derived value. The accepted findings-report is either that exact
|
||||
return or the bounded normalized candidate described below; the raw audit
|
||||
payload never changes.
|
||||
|
||||
1. Parse the exact return as strict JSON and validate every required field,
|
||||
enum, type, conditional requirement, summary count, coverage value, and
|
||||
leaf/super-skill constraint against this output contract.
|
||||
Before the full acceptance gate, a coordinator MAY create a normalized
|
||||
candidate copy only through this deterministic procedure:
|
||||
|
||||
1. Parse the exact return as strict JSON and provisionally check the complete
|
||||
report without mutating it. Every acceptance rule below MUST already pass
|
||||
except for one or more findings whose optional `location.range` has
|
||||
`start-line != line`.
|
||||
2. Each such finding is eligible only when `location.line`,
|
||||
`location.range.start-line`, and `location.range.end-line` are positive
|
||||
integers, `start-line <= line <= end-line`, and the finding does not contain
|
||||
the `suggested-code` field. Field presence disqualifies normalization even
|
||||
if its value is empty because suggested code may be bound to the reported
|
||||
range.
|
||||
3. Deep-copy the complete parsed report. In the candidate copy, remove only
|
||||
`location.range` from every eligible finding. Retain `location.line` and
|
||||
every other value unchanged. Do not add normalization metadata to the
|
||||
findings-report.
|
||||
4. Record each removed range separately in private run telemetry or artifacts,
|
||||
associated with the immutable raw audit payload. This record is
|
||||
runner-owned and is not part of the declared report schema.
|
||||
5. Validate the entire normalized candidate with the existing full consumer
|
||||
acceptance gate below. Only a candidate that passes every rule becomes the
|
||||
accepted copy used for rollup. If any other validation defect exists, or
|
||||
full validation fails, discard the candidate, preserve the raw payload, and
|
||||
fail the complete leaf as before.
|
||||
|
||||
This exception does not infer missing fields, alter references or paths, clamp
|
||||
line numbers, repair JSON, normalize a reversed or out-of-bounds range, remove
|
||||
a range from a finding containing `suggested-code`, or salvage arbitrary
|
||||
individual findings.
|
||||
|
||||
Before accepting either the exact return or an eligible normalized candidate
|
||||
as a findings-report, a coordinator or host MUST validate it deterministically:
|
||||
|
||||
1. Validate every required field, enum, type, conditional requirement, summary
|
||||
count, coverage value, and leaf/super-skill constraint against this output
|
||||
contract.
|
||||
2. For every knowledge-backed finding, verify each `references[].path` is an
|
||||
exact repo-relative knowledge path that exists in the live BCQuality
|
||||
snapshot, and verify `findings[].id` exactly equals
|
||||
|
|
@ -189,11 +225,10 @@ deterministically:
|
|||
Validation failure invalidates the complete return; consumers MUST NOT salvage
|
||||
individual findings, infer missing fields, reconstruct JSON, clamp ranges,
|
||||
rewrite paths, or otherwise silently repair model output. Preserve the invalid
|
||||
raw payload unchanged in private run artifacts or host logs. Record a separate
|
||||
failed validation result for that leaf with no findings, and derive the
|
||||
super-skill outcome as `partial` or `failed` using the normal rollup rules.
|
||||
Worker-side report-file persistence is optional and never replaces validation
|
||||
of the exact return.
|
||||
raw payload unchanged. Record a separate failed validation result for that leaf
|
||||
with no findings, and derive the super-skill outcome as `partial` or `failed`
|
||||
using the normal rollup rules. Worker-side report-file persistence is optional
|
||||
and never replaces validation of the accepted exact or normalized copy.
|
||||
|
||||
### Field semantics
|
||||
|
||||
|
|
|
|||
171
tools/Test-ReviewContract.ps1
Normal file
171
tools/Test-ReviewContract.ps1
Normal file
|
|
@ -0,0 +1,171 @@
|
|||
<#
|
||||
.SYNOPSIS
|
||||
Validates the bounded leaf-range normalization contract.
|
||||
|
||||
.DESCRIPTION
|
||||
BCQuality has no executable findings-report consumer. These assertions keep
|
||||
the normative DO contract, AL coordinator, and standalone runner aligned
|
||||
while exercising the exact normalization predicate against representative
|
||||
safe and ambiguous inputs.
|
||||
#>
|
||||
[CmdletBinding()]
|
||||
param(
|
||||
[string] $Root = (Resolve-Path (Join-Path $PSScriptRoot '..'))
|
||||
)
|
||||
|
||||
Set-StrictMode -Version Latest
|
||||
$ErrorActionPreference = 'Stop'
|
||||
|
||||
$Root = (Resolve-Path -LiteralPath $Root).Path
|
||||
|
||||
function Assert-True {
|
||||
param(
|
||||
[bool] $Condition,
|
||||
[string] $Message
|
||||
)
|
||||
|
||||
if (-not $Condition) {
|
||||
throw "Assertion failed: $Message"
|
||||
}
|
||||
}
|
||||
|
||||
function Assert-Contains {
|
||||
param(
|
||||
[string] $Text,
|
||||
[string] $Expected,
|
||||
[string] $Message
|
||||
)
|
||||
|
||||
Assert-True $Text.Contains($Expected) $Message
|
||||
}
|
||||
|
||||
function Test-PositiveInteger {
|
||||
param([object] $Value)
|
||||
|
||||
if (($null -eq $Value) -or ($Value -is [bool]) -or ($Value -isnot [ValueType])) {
|
||||
return $false
|
||||
}
|
||||
|
||||
$number = [double]$Value
|
||||
return [double]::IsFinite($number) -and ($number -gt 0) -and ([math]::Truncate($number) -eq $number)
|
||||
}
|
||||
|
||||
function Test-RangeNormalizationEligibility {
|
||||
param([pscustomobject] $Finding)
|
||||
|
||||
if ($Finding.PSObject.Properties.Name -contains 'suggested-code') {
|
||||
return $false
|
||||
}
|
||||
if (-not ($Finding.PSObject.Properties.Name -contains 'location')) {
|
||||
return $false
|
||||
}
|
||||
if (-not ($Finding.location.PSObject.Properties.Name -contains 'line')) {
|
||||
return $false
|
||||
}
|
||||
if (-not ($Finding.location.PSObject.Properties.Name -contains 'range')) {
|
||||
return $false
|
||||
}
|
||||
|
||||
$range = $Finding.location.range
|
||||
if (-not ($range.PSObject.Properties.Name -contains 'start-line') -or
|
||||
-not ($range.PSObject.Properties.Name -contains 'end-line')) {
|
||||
return $false
|
||||
}
|
||||
|
||||
$line = $Finding.location.line
|
||||
$startLine = $range.'start-line'
|
||||
$endLine = $range.'end-line'
|
||||
if (-not (Test-PositiveInteger $line) -or
|
||||
-not (Test-PositiveInteger $startLine) -or
|
||||
-not (Test-PositiveInteger $endLine)) {
|
||||
return $false
|
||||
}
|
||||
|
||||
return ($startLine -le $line) -and ($line -le $endLine) -and ($startLine -ne $line)
|
||||
}
|
||||
|
||||
$transportSentence = 'Capture the exact Task return as the immutable raw audit payload and primary transport.'
|
||||
$doContract = Get-Content -LiteralPath (Join-Path $Root 'skills/do.md') -Raw
|
||||
$coordinatorContract = Get-Content -LiteralPath (Join-Path $Root 'microsoft/skills/review/al-code-review.md') -Raw
|
||||
$runnerContract = Get-Content -LiteralPath (Join-Path $Root 'docs/standalone-runner.md') -Raw
|
||||
|
||||
foreach ($surface in @(
|
||||
[pscustomobject]@{ Name = 'DO'; Text = ($doContract -replace '\s+', ' ') }
|
||||
[pscustomobject]@{ Name = 'AL coordinator'; Text = ($coordinatorContract -replace '\s+', ' ') }
|
||||
[pscustomobject]@{ Name = 'standalone runner'; Text = ($runnerContract -replace '\s+', ' ') }
|
||||
)) {
|
||||
Assert-Contains $surface.Text $transportSentence "$($surface.Name) preserves exact Task transport wording"
|
||||
}
|
||||
|
||||
$normalizedDoContract = $doContract -replace '\s+', ' '
|
||||
foreach ($expected in @(
|
||||
'positive integers',
|
||||
'start-line <= line <= end-line',
|
||||
'does not contain the `suggested-code` field',
|
||||
'remove only',
|
||||
'private run telemetry or artifacts',
|
||||
'Validate the entire normalized candidate',
|
||||
'If any other validation defect exists',
|
||||
'salvage arbitrary individual findings'
|
||||
)) {
|
||||
Assert-Contains $normalizedDoContract $expected "DO documents '$expected'"
|
||||
}
|
||||
|
||||
$cases = @(
|
||||
[pscustomobject]@{
|
||||
Name = 'contained mismatched range without suggested code'
|
||||
Expected = $true
|
||||
Finding = '{"message":"keep me","location":{"file":"src/codeunit.al","line":37,"range":{"start-line":36,"end-line":38}}}' | ConvertFrom-Json
|
||||
}
|
||||
[pscustomobject]@{
|
||||
Name = 'aligned range'
|
||||
Expected = $false
|
||||
Finding = '{"location":{"line":37,"range":{"start-line":37,"end-line":38}}}' | ConvertFrom-Json
|
||||
}
|
||||
[pscustomobject]@{
|
||||
Name = 'suggested code present'
|
||||
Expected = $false
|
||||
Finding = '{"location":{"line":37,"range":{"start-line":36,"end-line":38}},"suggested-code":""}' | ConvertFrom-Json
|
||||
}
|
||||
[pscustomobject]@{
|
||||
Name = 'line outside range'
|
||||
Expected = $false
|
||||
Finding = '{"location":{"line":39,"range":{"start-line":36,"end-line":38}}}' | ConvertFrom-Json
|
||||
}
|
||||
[pscustomobject]@{
|
||||
Name = 'reversed range'
|
||||
Expected = $false
|
||||
Finding = '{"location":{"line":37,"range":{"start-line":38,"end-line":36}}}' | ConvertFrom-Json
|
||||
}
|
||||
[pscustomobject]@{
|
||||
Name = 'zero bound'
|
||||
Expected = $false
|
||||
Finding = '{"location":{"line":1,"range":{"start-line":0,"end-line":2}}}' | ConvertFrom-Json
|
||||
}
|
||||
[pscustomobject]@{
|
||||
Name = 'fractional primary line'
|
||||
Expected = $false
|
||||
Finding = '{"location":{"line":37.5,"range":{"start-line":36,"end-line":38}}}' | ConvertFrom-Json
|
||||
}
|
||||
[pscustomobject]@{
|
||||
Name = 'missing end line'
|
||||
Expected = $false
|
||||
Finding = '{"location":{"line":37,"range":{"start-line":36}}}' | ConvertFrom-Json
|
||||
}
|
||||
)
|
||||
|
||||
foreach ($case in $cases) {
|
||||
$actual = Test-RangeNormalizationEligibility $case.Finding
|
||||
Assert-True ($actual -eq $case.Expected) "$($case.Name) eligibility is $($case.Expected)"
|
||||
}
|
||||
|
||||
$rawFinding = $cases[0].Finding
|
||||
$candidateFinding = $rawFinding | ConvertTo-Json -Depth 10 | ConvertFrom-Json
|
||||
$candidateFinding.location.PSObject.Properties.Remove('range')
|
||||
|
||||
Assert-True ($rawFinding.location.PSObject.Properties.Name -contains 'range') 'raw finding remains unchanged'
|
||||
Assert-True (-not ($candidateFinding.location.PSObject.Properties.Name -contains 'range')) 'candidate removes only the optional range'
|
||||
Assert-True ($candidateFinding.location.line -eq $rawFinding.location.line) 'candidate preserves the primary line'
|
||||
Assert-True ($candidateFinding.message -ceq $rawFinding.message) 'candidate preserves all other finding content'
|
||||
|
||||
Write-Output "Review contract validation passed ($($cases.Count) normalization cases)."
|
||||
|
|
@ -195,12 +195,42 @@ foreach ($domain in $leafDomains) {
|
|||
}
|
||||
|
||||
$override = if ($overrides.ContainsKey($domain)) { $overrides[$domain] } else { $null }
|
||||
$selectedArticle = $null
|
||||
if ($override -and ($override.PSObject.Properties.Name -contains 'article')) {
|
||||
$articleName = [string]$override.article
|
||||
$hasArticleOverride = $override -and ($override.PSObject.Properties.Name -contains 'article')
|
||||
$hasArticlesOverride = $override -and ($override.PSObject.Properties.Name -contains 'articles')
|
||||
if ($hasArticleOverride -and $hasArticlesOverride) {
|
||||
$problems.Add("${domain}: override must specify either 'article' or 'articles', not both.") | Out-Null
|
||||
continue
|
||||
}
|
||||
|
||||
$articleNames = @()
|
||||
if ($hasArticlesOverride) {
|
||||
$articleNames = @($override.articles)
|
||||
if (-not $articleNames.Count) {
|
||||
$problems.Add("${domain}: override 'articles' must contain at least one article.") | Out-Null
|
||||
continue
|
||||
}
|
||||
} elseif ($hasArticleOverride) {
|
||||
$articleNames = @($override.article)
|
||||
} else {
|
||||
$articleNames = @($articles | Select-Object -First 1 | ForEach-Object BaseName)
|
||||
}
|
||||
|
||||
$selectedArticles = [System.Collections.Generic.List[object]]::new()
|
||||
$seenArticleNames = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase)
|
||||
foreach ($articleNameValue in $articleNames) {
|
||||
if ($articleNameValue -isnot [string] -or [string]::IsNullOrWhiteSpace([string]$articleNameValue)) {
|
||||
$problems.Add("${domain}: override article names must be non-empty strings.") | Out-Null
|
||||
continue
|
||||
}
|
||||
$articleName = [string]$articleNameValue
|
||||
if ($articleName.EndsWith('.md')) {
|
||||
$articleName = [System.IO.Path]::GetFileNameWithoutExtension($articleName)
|
||||
}
|
||||
if (-not $seenArticleNames.Add($articleName)) {
|
||||
$problems.Add("${domain}: override contains duplicate article: $articleName.md") | Out-Null
|
||||
continue
|
||||
}
|
||||
|
||||
$selectedArticle = $articles | Where-Object BaseName -eq $articleName | Select-Object -First 1
|
||||
if (-not $selectedArticle) {
|
||||
$articleExists = @(
|
||||
|
|
@ -218,24 +248,32 @@ foreach ($domain in $leafDomains) {
|
|||
}
|
||||
continue
|
||||
}
|
||||
} else {
|
||||
$selectedArticle = $articles | Select-Object -First 1
|
||||
$selectedArticles.Add($selectedArticle) | Out-Null
|
||||
}
|
||||
if (-not $selectedArticle) {
|
||||
if (-not $selectedArticles.Count) {
|
||||
if (-not $articleNames.Count) {
|
||||
$problems.Add("${domain}: no article has both .good.al and .bad.al companion samples.") | Out-Null
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
$articlePath = [string]$selectedArticle.ArticlePath
|
||||
$sampleDirectory = (Split-Path -Parent $articlePath).Replace('\', '/')
|
||||
$context = if ($override -and ($override.PSObject.Properties.Name -contains 'context')) {
|
||||
[string]$override.context
|
||||
} else {
|
||||
$null
|
||||
}
|
||||
for ($articleIndex = 0; $articleIndex -lt $selectedArticles.Count; $articleIndex++) {
|
||||
$selectedArticle = $selectedArticles[$articleIndex]
|
||||
$articlePath = [string]$selectedArticle.ArticlePath
|
||||
$sampleDirectory = (Split-Path -Parent $articlePath).Replace('\', '/')
|
||||
foreach ($kind in 'bad', 'good') {
|
||||
$caseId = if ($articleIndex -eq 0) {
|
||||
"$domain-$kind"
|
||||
} else {
|
||||
"$domain-$($selectedArticle.BaseName)-$kind"
|
||||
}
|
||||
$case = [pscustomobject]@{
|
||||
id = "$domain-$kind"
|
||||
id = $caseId
|
||||
domain = $domain
|
||||
input = "$sampleDirectory/$($selectedArticle.BaseName).$kind.al"
|
||||
expected = if ($kind -eq 'bad') { @($articlePath) } else { @() }
|
||||
|
|
@ -246,6 +284,7 @@ foreach ($domain in $leafDomains) {
|
|||
$caseList.Add($case) | Out-Null
|
||||
}
|
||||
}
|
||||
}
|
||||
$cases = @($caseList)
|
||||
|
||||
$seenIds = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::Ordinal)
|
||||
|
|
@ -434,6 +473,7 @@ if ($PrepareDirectory) {
|
|||
}
|
||||
|
||||
if (-not $ResultsPath -and -not $ResultsDirectory) {
|
||||
& (Join-Path $PSScriptRoot 'Test-ReviewContract.ps1') -Root $Root
|
||||
Write-Host "Review fixture validation PASSED: $($cases.Count) cases cover $($leafDomains.Count) leaf domains." -ForegroundColor Green
|
||||
exit 0
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue