diff --git a/evaluation/README.md b/evaluation/README.md index 7be55b4..2125ce3 100644 --- a/evaluation/README.md +++ b/evaluation/README.md @@ -2,7 +2,7 @@ The evaluation is convention-driven. The harness discovers every `/skills/review/al--review.md` leaf across the enabled `microsoft`, `community`, and `custom` layers. Duplicate domains resolve with `custom > community > microsoft` precedence. For each selected leaf, the harness finds paired knowledge across the same layers, applies the same precedence to duplicate article slugs, selects the first article (by filename) with both `.bad.al` and `.good.al` companions, and derives the expected positive and clean control automatically. Adding a conforming leaf requires no scoring-contract edit. -`review-fixtures.json` contains only global thresholds and optional exceptional overrides. An override may select a different article or add context when the generic convention cannot express a scenario. It should remain empty in the normal case. +`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 `-bad` and `-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-.json` files provide optional two-case leaf batches and identify the selected layer-owned skill path; save those as `result-.json`. Directory scoring prefers `result-case-*.json` when present and otherwise falls back to `result-*.json`. `review-request.json` is an optional all-domains stress test for larger models. Neither batch form is the preferred fast-model profile. + `request-.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-.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: diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index e11508a..352fccf 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -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" diff --git a/microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.good.al b/microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.good.al index 3659edf..a60af48 100644 --- a/microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.good.al +++ b/microsoft/knowledge/performance/job-queue-category-code-serializes-conflicting-jobs.good.al @@ -1,7 +1,14 @@ codeunit 50113 "Job Queue Category Good" { procedure ConfigurePostingJobs(var PostSales: Record "Job Queue Entry"; var PostPurchases: Record "Job Queue Entry") + 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. PostSales.Validate("Job Queue Category Code", 'POSTING'); PostPurchases.Validate("Job Queue Category Code", 'POSTING'); diff --git a/microsoft/skills/review/al-performance-review.md b/microsoft/skills/review/al-performance-review.md index 664f20d..7829d70 100644 --- a/microsoft/skills/review/al-performance-review.md +++ b/microsoft/skills/review/al-performance-review.md @@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially 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. diff --git a/tools/Test-ReviewFixtures.ps1 b/tools/Test-ReviewFixtures.ps1 index c4b0bf1..8cf019f 100644 --- a/tools/Test-ReviewFixtures.ps1 +++ b/tools/Test-ReviewFixtures.ps1 @@ -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,32 +248,41 @@ foreach ($domain in $leafDomains) { } continue } - } else { - $selectedArticle = $articles | Select-Object -First 1 + $selectedArticles.Add($selectedArticle) | Out-Null } - if (-not $selectedArticle) { - $problems.Add("${domain}: no article has both .good.al and .bad.al companion samples.") | Out-Null + 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 } - foreach ($kind in 'bad', 'good') { - $case = [pscustomobject]@{ - id = "$domain-$kind" - domain = $domain - input = "$sampleDirectory/$($selectedArticle.BaseName).$kind.al" - expected = if ($kind -eq 'bad') { @($articlePath) } else { @() } + 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 = $caseId + domain = $domain + input = "$sampleDirectory/$($selectedArticle.BaseName).$kind.al" + expected = if ($kind -eq 'bad') { @($articlePath) } else { @() } + } + if ($context) { + $case | Add-Member -NotePropertyName context -NotePropertyValue $context + } + $caseList.Add($case) | Out-Null } - if ($context) { - $case | Add-Member -NotePropertyName context -NotePropertyValue $context - } - $caseList.Add($case) | Out-Null } } $cases = @($caseList)