From f76279057e6ec4179de8292053433646f3d09b40 Mon Sep 17 00:00:00 2001 From: Jesper Schulz-Wedde Date: Wed, 15 Jul 2026 09:11:15 +0200 Subject: [PATCH] Generalize review fixture discovery Derive smoke cases from the leaf, domain, and paired-sample conventions so new leaves require no scoring-contract changes. Keep only exceptional selection/context overrides and fail when retrieval metadata cannot rank the selected article. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27 --- evaluation/README.md | 12 +- evaluation/review-fixtures.json | 249 ++++---------------------------- skills/write.md | 2 +- tools/Test-ReviewFixtures.ps1 | 93 ++++++++++-- 4 files changed, 115 insertions(+), 241 deletions(-) diff --git a/evaluation/README.md b/evaluation/README.md index 3c1cf0b..cd25d3d 100644 --- a/evaluation/README.md +++ b/evaluation/README.md @@ -1,6 +1,10 @@ # AL review evaluation -`review-fixtures.json` contains one positive and one clean control for every AL review leaf. Inputs are existing companion samples, but 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. +The evaluation is convention-driven. For every `microsoft/skills/review/al--review.md` leaf, the harness finds `microsoft/knowledge//`, 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. + +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. ## Validate the corpus @@ -8,7 +12,7 @@ pwsh ./tools/Test-ReviewFixtures.ps1 -Root . ``` -This is credential-free and runs in CI. It validates fixture paths, expected article references, unique IDs, and positive/clean coverage for every registered leaf. +This credential-free check proves every registered leaf maps to a same-named knowledge domain with at least one complete AL sample pair and that all configured overrides are valid. ## Run a fast-model evaluation @@ -18,6 +22,8 @@ This is credential-free and runs in CI. It validates fixture paths, expected art pwsh ./tools/Test-ReviewFixtures.ps1 -Root . -PrepareDirectory ./.evaluation-run ``` + This is also the CI path. It derives all cases, builds the current index, requires the convention-selected article to rank naturally into the candidate cutoff, and prepares the neutral requests. + 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; 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. @@ -47,4 +53,4 @@ This is credential-free and runs in CI. It validates fixture paths, expected art For a single combined stress-test result, use `-ResultsPath` instead. -The committed gate requires full expected recall and no findings on clean controls. `allowedAdditional` in the manifest records known, independently valid overlaps without weakening the required primary finding. +The committed gate requires full expected recall, the exact convention-derived article ID, and no findings on clean controls. diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index e7afd7e..36f413a 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -1,237 +1,40 @@ { - "version": 1, + "version": 2, + "selection": "first-paired-al-article", "minimumExpectedRecall": 1.0, "minimumCleanRate": 1.0, - "cases": [ - { - "id": "appsource-bad", - "domain": "appsource", - "input": "microsoft/knowledge/appsource/object-affixes-prevent-collisions.bad.al", - "context": "AppSourceCop mandatoryAffixes is configured to ABC.", - "expected": ["microsoft/knowledge/appsource/object-affixes-prevent-collisions.md"], - "allowedAdditional": [] + "overrides": { + "appsource": { + "context": "AppSourceCop mandatoryAffixes is configured to ABC." }, - { - "id": "appsource-good", - "domain": "appsource", - "input": "microsoft/knowledge/appsource/object-affixes-prevent-collisions.good.al", - "context": "AppSourceCop mandatoryAffixes is configured to ABC.", - "expected": [], - "allowedAdditional": [] + "breaking-changes": { + "article": "obsolete-table-fields-instead-of-deleting-them", + "context": "Baseline version: field ID 2 was named Email. Review the provided changed version." }, - { - "id": "breaking-changes-bad", - "domain": "breaking-changes", - "input": "microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al", - "context": "Baseline version: field ID 2 was named Email. Review the provided changed version.", - "expected": ["microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md"], - "allowedAdditional": [] + "events": { + "article": "initialize-ishandled-to-false-before-publishing" }, - { - "id": "breaking-changes-good", - "domain": "breaking-changes", - "input": "microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al", - "context": "Baseline version: field ID 2 was named Email. Review the provided changed version.", - "expected": [], - "allowedAdditional": [] + "interfaces": { + "article": "set-defaultimplementation-on-enum" }, - { - "id": "data-modeling-bad", - "domain": "data-modeling", - "input": "microsoft/knowledge/data-modeling/setup-table-is-a-singleton.bad.al", - "expected": ["microsoft/knowledge/data-modeling/setup-table-is-a-singleton.md"], - "allowedAdditional": [] + "performance": { + "article": "use-isempty-for-existence-check" }, - { - "id": "data-modeling-good", - "domain": "data-modeling", - "input": "microsoft/knowledge/data-modeling/setup-table-is-a-singleton.good.al", - "expected": [], - "allowedAdditional": [] + "privacy": { + "article": "no-pii-in-telemetry-message-string" }, - { - "id": "error-handling-bad", - "domain": "error-handling", - "input": "microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.bad.al", - "expected": ["microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md"], - "allowedAdditional": [] + "style": { + "article": "label-comment-explains-placeholders" }, - { - "id": "error-handling-good", - "domain": "error-handling", - "input": "microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.good.al", - "expected": [], - "allowedAdditional": [] + "telemetry": { + "article": "telemetry-event-id-stable-unique" }, - { - "id": "events-bad", - "domain": "events", - "input": "microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al", - "expected": ["microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md"], - "allowedAdditional": [] + "upgrade": { + "article": "initvalue-does-not-update-existing-rows", + "context": "The extended table existed in the previous app version and already contains rows." }, - { - "id": "events-good", - "domain": "events", - "input": "microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "interfaces-bad", - "domain": "interfaces", - "input": "microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.bad.al", - "expected": ["microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.md"], - "allowedAdditional": [] - }, - { - "id": "interfaces-good", - "domain": "interfaces", - "input": "microsoft/knowledge/interfaces/set-defaultimplementation-on-enum.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "performance-bad", - "domain": "performance", - "input": "microsoft/knowledge/performance/use-isempty-for-existence-check.bad.al", - "expected": ["microsoft/knowledge/performance/use-isempty-for-existence-check.md"], - "allowedAdditional": [] - }, - { - "id": "performance-good", - "domain": "performance", - "input": "microsoft/knowledge/performance/use-isempty-for-existence-check.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "privacy-bad", - "domain": "privacy", - "input": "microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.bad.al", - "expected": ["microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.md"], - "allowedAdditional": [] - }, - { - "id": "privacy-good", - "domain": "privacy", - "input": "microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "query-bad", - "domain": "query", - "input": "microsoft/knowledge/query/set-query-filters-before-open.bad.al", - "expected": ["microsoft/knowledge/query/set-query-filters-before-open.md"], - "allowedAdditional": [] - }, - { - "id": "query-good", - "domain": "query", - "input": "microsoft/knowledge/query/set-query-filters-before-open.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "security-bad", - "domain": "security", - "input": "microsoft/knowledge/security/secrettext-with-httpclient.bad.al", - "expected": ["microsoft/knowledge/security/secrettext-with-httpclient.md"], - "allowedAdditional": ["microsoft/knowledge/security/secrettext-for-credentials.md"] - }, - { - "id": "security-good", - "domain": "security", - "input": "microsoft/knowledge/security/secrettext-with-httpclient.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "style-bad", - "domain": "style", - "input": "microsoft/knowledge/style/label-comment-explains-placeholders.bad.al", - "expected": ["microsoft/knowledge/style/label-comment-explains-placeholders.md"], - "allowedAdditional": [] - }, - { - "id": "style-good", - "domain": "style", - "input": "microsoft/knowledge/style/label-comment-explains-placeholders.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "telemetry-bad", - "domain": "telemetry", - "input": "microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.bad.al", - "expected": ["microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.md"], - "allowedAdditional": [] - }, - { - "id": "telemetry-good", - "domain": "telemetry", - "input": "microsoft/knowledge/telemetry/telemetry-event-id-stable-unique.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "testing-bad", - "domain": "testing", - "input": "microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.bad.al", - "expected": ["microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md"], - "allowedAdditional": [] - }, - { - "id": "testing-good", - "domain": "testing", - "input": "microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "ui-bad", - "domain": "ui", - "input": "microsoft/knowledge/ui/show-caption-on-editable-fields.bad.al", - "expected": ["microsoft/knowledge/ui/show-caption-on-editable-fields.md"], - "allowedAdditional": [] - }, - { - "id": "ui-good", - "domain": "ui", - "input": "microsoft/knowledge/ui/show-caption-on-editable-fields.good.al", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "upgrade-bad", - "domain": "upgrade", - "input": "microsoft/knowledge/upgrade/initvalue-does-not-update-existing-rows.bad.al", - "context": "The extended table existed in the previous app version and already contains rows.", - "expected": ["microsoft/knowledge/upgrade/initvalue-does-not-update-existing-rows.md"], - "allowedAdditional": [] - }, - { - "id": "upgrade-good", - "domain": "upgrade", - "input": "microsoft/knowledge/upgrade/initvalue-does-not-update-existing-rows.good.al", - "context": "The extended table existed in the previous app version and already contains rows.", - "expected": [], - "allowedAdditional": [] - }, - { - "id": "web-services-bad", - "domain": "web-services", - "input": "microsoft/knowledge/web-services/expose-systemid-as-the-api-key.bad.al", - "expected": ["microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md"], - "allowedAdditional": [] - }, - { - "id": "web-services-good", - "domain": "web-services", - "input": "microsoft/knowledge/web-services/expose-systemid-as-the-api-key.good.al", - "expected": [], - "allowedAdditional": [] + "web-services": { + "article": "expose-systemid-as-the-api-key" } - ] + } } diff --git a/skills/write.md b/skills/write.md index 3814095..a9e3c31 100644 --- a/skills/write.md +++ b/skills/write.md @@ -89,6 +89,6 @@ Before opening a pull request: - File is in the correct layer and domain folder. - Name is kebab-case and descriptive. - Every companion sample is referenced by filename from the article, and every referenced sample exists. -- A changed review domain has a positive and clean control in `evaluation/review-fixtures.json`. +- Every review-leaf domain has at least one article with both `.good.al` and `.bad.al` companions; the evaluation harness derives positive and clean controls from that convention automatically. Agents scaffolding new files SHOULD run this checklist programmatically before emitting the file. diff --git a/tools/Test-ReviewFixtures.ps1 b/tools/Test-ReviewFixtures.ps1 index 7949b16..3f3f144 100644 --- a/tools/Test-ReviewFixtures.ps1 +++ b/tools/Test-ReviewFixtures.ps1 @@ -36,7 +36,6 @@ if (-not (Test-Path -LiteralPath $ManifestPath)) { } $manifest = Get-Content -LiteralPath $ManifestPath -Raw | ConvertFrom-Json -$cases = @($manifest.cases) $problems = [System.Collections.Generic.List[string]]::new() function Get-ModelCaseId { @@ -97,11 +96,11 @@ function Get-RankedArticles { ) } -if ($manifest.version -ne 1) { +if ($manifest.version -ne 2) { $problems.Add("Unsupported manifest version: $($manifest.version)") | Out-Null } -if (-not $cases.Count) { - $problems.Add('Manifest has no cases.') | Out-Null +if ($manifest.selection -ne 'first-paired-al-article') { + $problems.Add("Unsupported selection strategy: $($manifest.selection)") | Out-Null } if (([double]$manifest.minimumExpectedRecall -lt 0) -or ([double]$manifest.minimumExpectedRecall -gt 1)) { $problems.Add('minimumExpectedRecall must be between 0 and 1.') | Out-Null @@ -117,13 +116,80 @@ $leafDomains = @( Sort-Object -Unique ) +$overrides = @{} +if ($manifest.PSObject.Properties.Name -contains 'overrides') { + foreach ($property in $manifest.overrides.PSObject.Properties) { + $overrides[$property.Name] = $property.Value + } +} +foreach ($overrideDomain in $overrides.Keys) { + if ($leafDomains -notcontains $overrideDomain) { + $problems.Add("Override domain '$overrideDomain' has no registered al-$overrideDomain-review leaf.") | Out-Null + } +} + +$caseList = [System.Collections.Generic.List[object]]::new() +foreach ($domain in $leafDomains) { + $knowledgeDirectory = Join-Path $Root "microsoft/knowledge/$domain" + if (-not (Test-Path -LiteralPath $knowledgeDirectory -PathType Container)) { + $problems.Add("${domain}: no Microsoft knowledge directory exists.") | Out-Null + continue + } + + $override = if ($overrides.ContainsKey($domain)) { $overrides[$domain] } else { $null } + $selectedArticle = $null + if ($override -and ($override.PSObject.Properties.Name -contains 'article')) { + $articleName = [string]$override.article + if ($articleName.EndsWith('.md')) { + $articleName = [System.IO.Path]::GetFileNameWithoutExtension($articleName) + } + $candidate = Join-Path $knowledgeDirectory "$articleName.md" + if (Test-Path -LiteralPath $candidate -PathType Leaf) { + $selectedArticle = Get-Item -LiteralPath $candidate + } else { + $problems.Add("${domain}: override article does not exist: $articleName.md") | Out-Null + } + } else { + $selectedArticle = Get-ChildItem -LiteralPath $knowledgeDirectory -File -Filter '*.md' | + Sort-Object Name | + Where-Object { + (Test-Path -LiteralPath (Join-Path $knowledgeDirectory "$($_.BaseName).good.al") -PathType Leaf) -and + (Test-Path -LiteralPath (Join-Path $knowledgeDirectory "$($_.BaseName).bad.al") -PathType Leaf) + } | + Select-Object -First 1 + } + if (-not $selectedArticle) { + $problems.Add("${domain}: no article has both .good.al and .bad.al companion samples.") | Out-Null + continue + } + + $articlePath = "microsoft/knowledge/$domain/$($selectedArticle.Name)" + $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 = "microsoft/knowledge/$domain/$($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 + } +} +$cases = @($caseList) + $seenIds = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::Ordinal) foreach ($case in $cases) { $id = [string]$case.id $domain = [string]$case.domain $input = [string]$case.input $expected = @($case.expected) - $allowed = @($case.allowedAdditional) if ([string]::IsNullOrWhiteSpace($id)) { $problems.Add('Case with empty id.') | Out-Null @@ -145,7 +211,7 @@ foreach ($case in $cases) { $problems.Add("${id}: clean case must use a .good sample: $input") | Out-Null } - foreach ($reference in @($expected + $allowed)) { + foreach ($reference in $expected) { $referencePath = Join-Path $Root ([string]$reference) if (-not (Test-Path -LiteralPath $referencePath -PathType Leaf)) { $problems.Add("${id}: referenced article does not exist: $reference") | Out-Null @@ -275,13 +341,13 @@ if ($PrepareDirectory) { } $rankedArticles = @(Get-RankedArticles -Articles $domainArticles -CaseText $caseText) $manifestCase = $manifestCaseByModelId[[string]$requestCase.id] - $expectedPaths = @($manifestCase.expected | ForEach-Object { [string]$_ }) - if ($expectedPaths.Count) { - $rankedPaths = @($rankedArticles | ForEach-Object { [string]$_.path }) - if ($rankedPaths -notcontains $expectedPaths[0]) { - throw "$($manifestCase.id): deterministic ranking omitted expected primary article '$($expectedPaths[0])'." - } + $selectedArticlePath = ([string]$manifestCase.input) -replace '\.(?:good|bad)\.al$', '.md' + $rankedPaths = @($rankedArticles | ForEach-Object { [string]$_.path }) + if ($rankedPaths -notcontains $selectedArticlePath) { + throw "$($manifestCase.id): deterministic ranking omitted selected article '$selectedArticlePath'. Improve its retrieval metadata or choose an exceptional override article." } + # Candidate order must not reveal which article owns the fixture. + $rankedArticles = @($rankedArticles | Sort-Object path) [pscustomobject]@{ protocol = "Run only $leafPath over this case. Follow leafInstructions exactly, evaluate the ranked candidate article rows, open matching articles in full, and copy every finding id verbatim from candidateArticles[].path." skill = $leafPath @@ -377,12 +443,11 @@ foreach ($case in $cases) { } | Where-Object { -not [string]::IsNullOrWhiteSpace($_) } | Sort-Object -Unique ) $expected = @($case.expected | ForEach-Object { [string]$_ }) - $allowed = @($case.allowedAdditional | ForEach-Object { [string]$_ }) if ($expected.Count) { $positiveTotal++ $missing = @($expected | Where-Object { $findingIds -notcontains $_ }) - $unexpected = @($findingIds | Where-Object { ($expected + $allowed) -notcontains $_ }) + $unexpected = @($findingIds | Where-Object { $expected -notcontains $_ }) if (-not $missing.Count -and -not $unexpected.Count) { $positivePassed++ } else {