From 11fbd227f4a90e962e3370e60753a71a76b6fc69 Mon Sep 17 00:00:00 2001 From: demiliani Date: Thu, 1 Oct 2026 11:05:00 +0200 Subject: [PATCH] Validate composed reviews against the expected leaf worklist --- docs/standalone-runner.md | 46 ++++- microsoft/skills/review/al-code-review.md | 14 +- skills/do.md | 41 ++++- tools/Test-ReviewContract.ps1 | 200 +++++++++++++++++++++- tools/Validate-FindingsReport.ps1 | 141 ++++++++++++++- 5 files changed, 424 insertions(+), 18 deletions(-) diff --git a/docs/standalone-runner.md b/docs/standalone-runner.md index e5e2a0c..d3186ef 100644 --- a/docs/standalone-runner.md +++ b/docs/standalone-runner.md @@ -53,7 +53,9 @@ only result. with `tools/Resolve-SkillWorklist.ps1`, passing the enabled layers and disabled skill paths from the task context. Execute every resolved leaf as a discrete invocation. Leaves are independent and may be scheduled serially - or concurrently. + or concurrently. Before dispatch, preserve the final ordered selection and + legitimate configuration/input-compatibility exclusions as a private expected + composition artifact; see [composition acceptance](#composition-acceptance). 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 @@ -61,7 +63,7 @@ only result. telemetry, and accept the candidate only if the entire copy passes the unchanged strict gate. Use `tools/Validate-FindingsReport.ps1`, passing the exact source paths and fully retrieved article paths; pass `-SkillKind super` - for the final rolled-up report. The accepted report contains no undeclared + and `-ExpectedCompositionPath` for the final rolled-up report. 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 @@ -74,6 +76,44 @@ The runner must never inspect the diff to skip a review domain. A leaf decides its own task-level applicability and reports `not-applicable` or `no-knowledge`. +## Composition acceptance + +A report can be internally consistent while omitting a selected review. Bind +the final acceptance gate to the host's selection, not just the returned +reports. The private JSON input to `-ExpectedCompositionPath` follows the +[DO consumer acceptance contract](../skills/do.md#consumer-acceptance-gate). +For example, if style is selected and security was disabled: + +```json +{ + "superSkill": { "id": "al-code-review", "version": 1 }, + "subSkills": [{ "id": "al-style-review", "version": 1 }], + "skipped": [{ "id": "al-security-review", "version": 1, "reason": "configuration" }] +} +``` + +Build this artifact from `Resolve-SkillWorklist.ps1` and the input-compatibility +decision before dispatch. Resolver `skipped` entries have no version: enrich +them from the declared skill's indexed version. Move an input-incompatible +selected slot to `skipped` with its selected version and `not-applicable` +reason; never use source content or later model output to make that decision. +Keep paths, layers, and other resolver metadata if useful for private audit. +Do not add the artifact to the findings-report or overwrite it to hide an +unfinished invocation. Preserve it with the run's raw payloads. + +The gate rejects repeated or unexpected leaf IDs, wrong selected versions, +reordered results, and fabricated or missing exclusions. If selected leaves +remain unfinished, the report must be `partial` with a non-failed returned +report, otherwise `failed`; identify unfinished leaf IDs in `outcome-reason`. +Do not fabricate leaf reports or use configuration skips for budget exhaustion. +Wait for started invocations to finish; omit the self-review if the selected +composition remains incomplete. Coverage still sums the non-failed leaf +knowledge worklists, not the number of selected leaf slots. + +Calls without the expected artifact remain supported for structural/semantic +validation, but cannot certify that a composed review covered its selection. +They still reject duplicate leaf IDs and returned-and-skipped conflicts. + ## Runner-owned choices Keep these settings and behaviors outside BCQuality: @@ -113,6 +153,8 @@ A compatible runner: only part of the review is reliable; - orders `sub-results` by the declared worklist and orders rendered findings deterministically; +- validates composition against the host-owned selection, and reports selected + leaves left unfinished as incomplete rather than clean or configured away; - calculates top-level severity counts from deduplicated top-level findings, not by summing leaf counts; - preserves knowledge paths verbatim and verifies references before publishing; diff --git a/microsoft/skills/review/al-code-review.md b/microsoft/skills/review/al-code-review.md index 7a3d0c6..8699d7d 100644 --- a/microsoft/skills/review/al-code-review.md +++ b/microsoft/skills/review/al-code-review.md @@ -71,6 +71,14 @@ Sub-skills that fail either check are not invoked and are recorded in `skipped-s The worklist is the list of sub-skills judged relevant by the previous step. Every sub-skill in the worklist will be invoked in the Action step. +Before dispatch, preserve that selection in a private host-owned expected +composition artifact using DO's input contract. Start from the resolver's +ordered selection; enrich configuration skips with the declared indexed +skill's version, and record any input-incompatible exclusions with +`reason: "not-applicable"`. Do not derive this artifact from leaf reports or +change it merely because execution later runs out of budget. Pass its path as +`-ExpectedCompositionPath` for final super-skill validation. + ## Action ### Execution discipline (mandatory) @@ -85,7 +93,7 @@ The Action step consists of **discrete leaf invocations**, not one combined gene - 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). - The agent self-review pass is its own final iteration. Begin it only after every sub-skill in the worklist has completed and its sub-result is recorded. - Sub-skills are independent: re-walking the diff once per sub-skill is correct and expected. The output schema accommodates this — `sub-results` carries one entry per sub-skill, each a complete findings-report, in the frontmatter `sub-skills` order regardless of completion order. -- When isolated calls are unavailable and the current model cannot finish every leaf within its budget, return `partial` with completed `sub-results` and name the first unevaluated sub-skill in `outcome-reason`. Never silently mark the remaining leaves clean. +- When the execution budget prevents invoking every selected leaf, wait for started invocations to finish and preserve their accepted `sub-results`. Return `partial` if any returned report is non-`failed`, otherwise `failed`, and name the unfinished leaf IDs in `outcome-reason`. Do not run the self-review on incomplete composition, invent sub-results, or record budget exhaustion as a configured/input-incompatible skip. Never silently mark the remaining leaves clean. ### Roll up sub-skill findings @@ -138,7 +146,9 @@ 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. A leaf's nested report is its accepted exact return or +and top-level finding, and validate the final report with +`-SkillKind super -ExpectedCompositionPath ` against the +selection preserved before dispatch. 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 diff --git a/skills/do.md b/skills/do.md index db0d634..16fbdd9 100644 --- a/skills/do.md +++ b/skills/do.md @@ -224,7 +224,21 @@ as a findings-report, a coordinator or host MUST validate it deterministically: Hosts SHOULD execute `tools/Validate-FindingsReport.ps1` with the exact source scope and the leaf's recorded set of fully retrieved article paths. Pass -`-SkillKind super` when validating a super-skill's rolled-up report. Pass +`-SkillKind super -ExpectedCompositionPath ` when validating +a super-skill's rolled-up report. Prepare that private artifact before leaf +dispatch, after layer resolution and input compatibility checks. It contains +`superSkill` (`id`, `version`), ordered selected `subSkills` (each with `id`, +`version`), and `skipped` (each with `id`, `version`, `reason`). Reasons are +`configuration` or `not-applicable`; budget exhaustion is not a skip reason. +Additional resolver metadata may be retained in the artifact, not the report. +The validator binds the super-skill and leaf identities and versions, checks +selected order, and requires exact agreement on exclusions. Every returned +leaf must be unique; a selected leaf cannot be reclassified as skipped by the +report. Without the artifact, validation remains structural and semantic but +cannot prove composition completeness, selected versions, order, or legitimate +exclusions. Duplicate or both returned-and-skipped leaf IDs are invalid even +without the artifact. Never derive the expected composition from model output. +Pass `-AllowBoundedNormalization` only when the host preserves the immutable raw payload and records `removedRanges` in private telemetry as required above. @@ -330,7 +344,7 @@ Omit `suggested-code` only when the appropriate fix depends on context the skill - `reference` — the suppressed file (same object shape as `findings[].references`). - `reason` — `layer-precedence` when another layer won under READ's precedence rules; `configuration` when the consumer disabled the file's layer. -**`sub-results`** — super-skills only. Array of complete findings-reports, one per sub-skill that was invoked (i.e., every sub-skill not listed in `skipped-sub-skills`). Each entry MUST itself conform to this output contract. Entries MUST appear in the worklist's declared order, regardless of invocation or completion order. Leaf skills MUST NOT emit `sub-results`. +**`sub-results`** — super-skills only. Array of complete findings-reports, one per invoked sub-skill, with no duplicate skill IDs. Each entry MUST itself conform to this output contract. Entries MUST appear in the worklist's declared order, regardless of invocation or completion order. A selected leaf left uninvoked by budget exhaustion has no fabricated sub-result and is not a configured or input-incompatible skip; its absence requires the incomplete-composition outcome below. Leaf skills MUST NOT emit `sub-results`. **`skipped-sub-skills`** — super-skills only. Array of sub-skills that were declared in frontmatter but not invoked. `reason` is `configuration` when the orchestrator disabled the sub-skill, or `not-applicable` when the super-skill's Relevance step ruled it out. @@ -356,10 +370,14 @@ orchestrator. Each leaf invocation MUST remain a discrete evaluation with its own complete findings-report. An orchestrator MAY execute independent leaves serially or -concurrently, but MUST invoke every worklisted leaf, preserve `sub-results` in -the declared worklist order, and wait for every invocation to finish before -performing any super-skill self-review or final rollup. Scheduling MUST NOT -change relevance, coverage, failure, reference-integrity, or output semantics. +concurrently, but MUST attempt every worklisted leaf, preserve `sub-results` +in the declared worklist order, and wait for every started invocation to +finish before final rollup. If its execution budget prevents dispatching +remaining leaves, preserve the unfinished selection in the host-owned expected +composition and use the incomplete-composition outcome below. Do not perform +the super-skill self-review until every selected leaf has returned. Scheduling +MUST NOT change relevance, coverage, failure, reference-integrity, or output +semantics. Orchestrators SHOULD generate `skill-index.json` with `tools/Build-SkillIndex.ps1` instead of parsing Markdown. Each declared @@ -390,7 +408,16 @@ The five required sections still apply. Their meaning shifts from knowledge file ### Outcome rollup -A super-skill's `outcome` is derived from its sub-skills' outcomes. Let S be the multiset of sub-skill outcomes for sub-skills in the worklist (skipped sub-skills do not contribute): +A super-skill's `outcome` is derived from its selected worklist and returned +sub-skills' outcomes. If selected leaves have no returned report, the outcome +is `partial` when at least one returned report is non-`failed`, or `failed` +when no non-`failed` report is available. It MUST NOT be `completed`, +`not-applicable`, or `no-knowledge`. Name unfinished leaf IDs in +`outcome-reason`, preserve the valid returned reports, and never invent +successful or failed invocations for leaves that were not invoked. + +When every selected leaf has returned, let S be the multiset of their outcomes +(skipped sub-skills do not contribute): - `failed` — every element of S is `failed`. - `partial` — S contains at least one `partial`, OR S contains at least one `failed` alongside at least one non-`failed` outcome. diff --git a/tools/Test-ReviewContract.ps1 b/tools/Test-ReviewContract.ps1 index adacb36..118fc3a 100644 --- a/tools/Test-ReviewContract.ps1 +++ b/tools/Test-ReviewContract.ps1 @@ -269,6 +269,8 @@ try { & $validator -ReportPath $reportPath -BCQualityRoot $Root } + $completedSecurityLeaf = $completedLeaf | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $completedSecurityLeaf.skill.id = 'al-security-review' $validSuperReport = [ordered]@{ skill = [ordered]@{ id = 'al-code-review'; version = 1 } outcome = 'completed' @@ -278,12 +280,207 @@ try { } findings = @() suppressed = @() - 'sub-results' = @($completedLeaf, $completedLeaf) + 'sub-results' = @($completedLeaf, $completedSecurityLeaf) } Set-Content -LiteralPath $reportPath -Value ($validSuperReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM $acceptedSuper = & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super Assert-True (-not $acceptedSuper.normalized) 'valid super-skill report is accepted' + $duplicateLeafReport = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $duplicateLeafReport.'sub-results' = @($completedLeaf, $completedLeaf) + Set-Content -LiteralPath $reportPath -Value ($duplicateLeafReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + Assert-ThrowsLike -Pattern '*SUPER_DUPLICATE_SUB_RESULT*' -Action { + & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super + } + + $compositionPath = Join-Path $tmp 'composition.json' + $expectedComposition = [ordered]@{ + superSkill = @{ id = 'al-code-review'; version = 1 } + subSkills = @( + @{ id = 'al-style-review'; version = 1 } + @{ id = 'al-security-review'; version = 1 } + ) + skipped = @() + } + Set-Content -LiteralPath $compositionPath -Value ($expectedComposition | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + Set-Content -LiteralPath $reportPath -Value ($validSuperReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + $acceptedBoundSuper = & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super ` + -ExpectedCompositionPath $compositionPath + Assert-True (-not $acceptedBoundSuper.normalized) 'complete composition matches the expected worklist' + + $missingLeafReport = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $missingLeafReport.'sub-results' = @($completedLeaf) + $missingLeafReport.summary.coverage.'worklist-size' = 1 + $missingLeafReport.summary.coverage.'items-evaluated' = 1 + Set-Content -LiteralPath $reportPath -Value ($missingLeafReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + Assert-ThrowsLike -Pattern '*SUPER_OUTCOME_MISMATCH*' -Action { + & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super -ExpectedCompositionPath $compositionPath + } + $missingLeafReport.outcome = 'partial' + $missingLeafReport | Add-Member -NotePropertyName 'outcome-reason' -NotePropertyValue 'al-security-review was not evaluated before the budget expired.' + Set-Content -LiteralPath $reportPath -Value ($missingLeafReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + $acceptedIncompleteSuper = & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super ` + -ExpectedCompositionPath $compositionPath + Assert-True ($acceptedIncompleteSuper.report.outcome -ceq 'partial') 'unfinished selected leaves require a truthful partial outcome' + + function Assert-CompositionReport { + param([object] $Candidate, [string] $ErrorPattern) + + Set-Content -LiteralPath $reportPath -Value ($Candidate | ConvertTo-Json -Depth 30) -Encoding utf8NoBOM + if ($ErrorPattern) { + Assert-ThrowsLike -Pattern $ErrorPattern -Action { + & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super ` + -ExpectedCompositionPath $compositionPath -AllowBoundedNormalization + } + } + else { + $accepted = & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super ` + -ExpectedCompositionPath $compositionPath + Assert-True (-not $accepted.normalized) 'valid expected composition is accepted without repairs' + } + } + + foreach ($case in @( + @{ Pattern = '*SUPER_IDENTITY_MISMATCH*'; Change = { param($candidate) $candidate.skill.id = 'al-other-review' } } + @{ Pattern = '*SUPER_IDENTITY_MISMATCH*'; Change = { param($candidate) $candidate.skill.version = 2 } } + @{ Pattern = '*SUPER_LEAF_VERSION_MISMATCH*'; Change = { param($candidate) $candidate.'sub-results'[0].skill.version = 2 } } + @{ Pattern = '*SUPER_UNEXPECTED_SUB_RESULT*'; Change = { param($candidate) $candidate.'sub-results'[0].skill.id = 'al-other-review' } } + @{ Pattern = '*SUPER_SUB_RESULT_ORDER*'; Change = { param($candidate) $candidate.'sub-results' = @($candidate.'sub-results'[1], $candidate.'sub-results'[0]) } } + @{ Pattern = '*SUPER_DUPLICATE_SUB_RESULT*'; Change = { param($candidate) $candidate.'sub-results' = @($candidate.'sub-results'[0], $candidate.'sub-results'[0]) } } + )) { + $candidate = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + & $case.Change $candidate + Assert-CompositionReport $candidate $case.Pattern + } + + $fabricatedSkip = $missingLeafReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $fabricatedSkip | Add-Member -NotePropertyName 'skipped-sub-skills' -NotePropertyValue @( + @{ skill = @{ id = 'al-security-review'; version = 1 }; reason = 'configuration' } + ) + Assert-CompositionReport $fabricatedSkip '*SUPER_UNEXPECTED_SKIP*' + + $noResults = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $noResults.'sub-results' = @() + $noResults.summary.coverage.'worklist-size' = 0 + $noResults.summary.coverage.'items-evaluated' = 0 + $noResults.outcome = 'not-applicable' + Assert-CompositionReport $noResults '*SUPER_OUTCOME_MISMATCH*' + $noResults.outcome = 'failed' + $noResults | Add-Member -NotePropertyName 'outcome-reason' -NotePropertyValue 'No selected leaf could be evaluated.' + Assert-CompositionReport $noResults + + $allFailed = $noResults | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $allFailed.'sub-results' = @($completedLeaf, $completedSecurityLeaf) | ConvertTo-Json -Depth 20 | ConvertFrom-Json + foreach ($leaf in $allFailed.'sub-results') { + $leaf.outcome = 'failed' + $leaf.summary.coverage.'items-evaluated' = 0 + $leaf | Add-Member -NotePropertyName 'outcome-reason' -NotePropertyValue 'Invocation failed.' + } + Assert-CompositionReport $allFailed + + $skipComposition = $expectedComposition | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $skipComposition.subSkills = @($skipComposition.subSkills[0]) + $skipComposition.skipped = @(@{ id = 'al-security-review'; version = 1; reason = 'not-applicable' }) + Set-Content -LiteralPath $compositionPath -Value ($skipComposition | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + $validSkippedReport = $fabricatedSkip | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $validSkippedReport.outcome = 'completed' + $validSkippedReport.PSObject.Properties.Remove('outcome-reason') + $validSkippedReport.'skipped-sub-skills'[0].reason = 'not-applicable' + Assert-CompositionReport $validSkippedReport + Assert-CompositionReport $missingLeafReport '*SUPER_SKIP_MISSING*' + $wrongSkip = $validSkippedReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $wrongSkip.'skipped-sub-skills'[0].reason = 'configuration' + Assert-CompositionReport $wrongSkip '*SUPER_SKIP_MISMATCH*' + $wrongSkip.'skipped-sub-skills'[0].reason = 'not-applicable' + $wrongSkip.'skipped-sub-skills'[0].skill.version = 2 + Assert-CompositionReport $wrongSkip '*SUPER_SKIP_MISMATCH*' + $duplicateSkip = $validSkippedReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $duplicateSkip.'skipped-sub-skills' = @($duplicateSkip.'skipped-sub-skills'[0], $duplicateSkip.'skipped-sub-skills'[0]) + Assert-CompositionReport $duplicateSkip '*SUPER_SKIP_CONFLICT*' + $returnedAndSkipped = $validSkippedReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $returnedAndSkipped.'skipped-sub-skills'[0].skill.id = 'al-style-review' + Assert-CompositionReport $returnedAndSkipped '*SUPER_SKIP_CONFLICT*' + + $allSkippedComposition = $expectedComposition | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $allSkippedComposition.skipped = @($allSkippedComposition.subSkills | ForEach-Object { + @{ id = $_.id; version = $_.version; reason = 'configuration' } + }) + $allSkippedComposition.subSkills = @() + Set-Content -LiteralPath $compositionPath -Value ($allSkippedComposition | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + $allSkippedReport = $noResults | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $allSkippedReport.outcome = 'not-applicable' + $allSkippedReport.PSObject.Properties.Remove('outcome-reason') + $allSkippedReport | Add-Member -NotePropertyName 'skipped-sub-skills' -NotePropertyValue @( + $allSkippedComposition.skipped | ForEach-Object { @{ skill = @{ id = $_.id; version = $_.version }; reason = $_.reason } } + ) + Assert-CompositionReport $allSkippedReport + + $invalidComposition = $expectedComposition | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $invalidComposition.skipped = @(@{ id = 'al-style-review'; version = 1; reason = 'configuration' }) + Set-Content -LiteralPath $compositionPath -Value ($invalidComposition | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + Assert-CompositionReport $validSuperReport '*Invalid expected composition*' + $invalidComposition.skipped = @() + $invalidComposition.subSkills[0].version = '1' + Set-Content -LiteralPath $compositionPath -Value ($invalidComposition | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + Assert-CompositionReport $validSuperReport '*Invalid expected composition*' + Set-Content -LiteralPath $compositionPath -Value ($expectedComposition | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + + $layerFixtureRoot = Join-Path $tmp 'layered-skills' + foreach ($layer in 'microsoft', 'community', 'custom') { + $layerDirectory = Join-Path $layerFixtureRoot $layer + New-Item -ItemType Directory -Path $layerDirectory -Force | Out-Null + $sourceSkills = Join-Path $Root "$layer/skills" + if (Test-Path -LiteralPath $sourceSkills -PathType Container) { + Copy-Item -LiteralPath $sourceSkills -Destination (Join-Path $layerDirectory 'skills') -Recurse + } + } + $customSkillDirectory = Join-Path $layerFixtureRoot 'custom/skills/review' + New-Item -ItemType Directory -Path $customSkillDirectory -Force | Out-Null + $customSkillPath = 'custom/skills/review/company-style-review.md' + $styleSkillText = Get-Content -LiteralPath (Join-Path $Root 'microsoft/skills/review/al-style-review.md') -Raw + Set-Content -LiteralPath (Join-Path $layerFixtureRoot $customSkillPath) ` + -Value ($styleSkillText -replace '(?m)^version: 1\r?$', 'version: 7') -Encoding utf8NoBOM + $fixtureIndexPath = Join-Path $tmp 'layered-skill-index.json' + & (Join-Path $Root 'tools/Build-SkillIndex.ps1') -BCQualityRoot $layerFixtureRoot -IndexPath $fixtureIndexPath | Out-Null + $fixtureIndex = Get-Content -LiteralPath $fixtureIndexPath -Raw | ConvertFrom-Json + foreach ($selection in @( + @{ Disabled = @(); ExpectedLayer = 'custom'; ExpectedVersion = 7 } + @{ Disabled = @($customSkillPath); ExpectedLayer = 'microsoft'; ExpectedVersion = 1 } + @{ Disabled = @($customSkillPath, 'microsoft/skills/review/al-style-review.md'); ExpectedLayer = $null } + )) { + $resolved = & (Join-Path $Root 'tools/Resolve-SkillWorklist.ps1') -BCQualityRoot $layerFixtureRoot ` + -IndexPath $fixtureIndexPath -SuperSkillPath 'microsoft/skills/review/al-code-review.md' ` + -DisabledSkills $selection.Disabled + $styleSlots = @($resolved.subSkills | Where-Object id -CEQ 'al-style-review') + if ($selection.ExpectedLayer) { + Assert-True ($styleSlots.Count -eq 1 -and $styleSlots[0].layer -ceq $selection.ExpectedLayer -and + $styleSlots[0].version -eq $selection.ExpectedVersion) 'resolver-selected override or fallback is authoritative' + } + else { + Assert-True ($styleSlots.Count -eq 0) 'fully disabled slot is not selected' + } + $resolved.skipped = @($resolved.skipped | ForEach-Object { + $declaredPath = $_.declaredPath + $declaredSkill = @($fixtureIndex.skills | Where-Object path -CEQ $declaredPath)[0] + @{ id = $_.id; version = $declaredSkill.version; reason = $_.reason; declaredPath = $declaredPath } + }) + Set-Content -LiteralPath $compositionPath -Value ($resolved | ConvertTo-Json -Depth 30) -Encoding utf8NoBOM + $resolvedReport = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $resolvedReport.'sub-results' = @($resolved.subSkills | ForEach-Object { + $leafReport = $completedLeaf | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $leafReport.skill.id = $_.id + $leafReport.skill.version = $_.version + $leafReport + }) + $resolvedReport.summary.coverage.'worklist-size' = $resolved.subSkills.Count + $resolvedReport.summary.coverage.'items-evaluated' = $resolved.subSkills.Count + $resolvedReport | Add-Member -NotePropertyName 'skipped-sub-skills' -NotePropertyValue @( + $resolved.skipped | ForEach-Object { @{ skill = @{ id = $_.id; version = $_.version }; reason = $_.reason } } + ) + Assert-CompositionReport $resolvedReport + } + Set-Content -LiteralPath $compositionPath -Value ($expectedComposition | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + $styleFindingLeaf = $validReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json $securityFindingLeaf = $validReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json $securityFindingLeaf.skill.id = 'al-security-review' @@ -446,6 +643,7 @@ try { Set-Content -LiteralPath $reportPath -Value ($partialSuperReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM $acceptedPartialSuper = & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super Assert-True (-not $acceptedPartialSuper.normalized) 'partial super-skill excludes failed coverage from its rollup' + Assert-CompositionReport $partialSuperReport $failedLeafLeakage = $partialSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json $failedLeafLeakage.summary.counts.minor = 1 diff --git a/tools/Validate-FindingsReport.ps1 b/tools/Validate-FindingsReport.ps1 index d3442d3..cf42c5b 100644 --- a/tools/Validate-FindingsReport.ps1 +++ b/tools/Validate-FindingsReport.ps1 @@ -12,6 +12,7 @@ param( [string[]] $RetrievedArticlePaths = @(), [ValidateSet('leaf', 'super')] [string] $SkillKind = 'leaf', + [string] $ExpectedCompositionPath, [switch] $AllowBoundedNormalization ) @@ -34,6 +35,61 @@ catch { throw "Invalid findings-report JSON or schema: $($_.Exception.Message)" } +$expectedComposition = $null +$expectedLeaves = [Collections.Generic.Dictionary[string, object]]::new([StringComparer]::Ordinal) +$expectedSkips = [Collections.Generic.Dictionary[string, object]]::new([StringComparer]::Ordinal) +if ($ExpectedCompositionPath) { + if ($SkillKind -cne 'super') { + throw 'Expected composition is supported only for super-skill reports.' + } + $contractSchema = Get-Content -LiteralPath $schemaPath -Raw | ConvertFrom-Json -AsHashtable + $identitySchema = @{ + type = 'object' + required = @('id', 'version') + properties = $contractSchema.definitions.skillReference.properties + } + $skipProperties = @{ + id = $identitySchema.properties.id + version = $identitySchema.properties.version + reason = @{ enum = @('configuration', 'not-applicable') } + } + $compositionSchema = @{ + type = 'object' + required = @('superSkill', 'subSkills', 'skipped') + properties = @{ + superSkill = $identitySchema + subSkills = @{ type = 'array'; items = $identitySchema } + skipped = @{ + type = 'array' + items = @{ type = 'object'; required = @('id', 'version', 'reason'); properties = $skipProperties } + } + } + } | ConvertTo-Json -Depth 20 + try { + $compositionRaw = Get-Content -LiteralPath $ExpectedCompositionPath -Raw + if (-not ($compositionRaw | Test-Json -Schema $compositionSchema -ErrorAction Stop)) { + throw 'Expected composition does not satisfy its input contract.' + } + $expectedComposition = $compositionRaw | ConvertFrom-Json -Depth 100 + $expectedIds = [Collections.Generic.HashSet[string]]::new([StringComparer]::Ordinal) + foreach ($leaf in @($expectedComposition.subSkills)) { + if (-not $expectedIds.Add([string]$leaf.id)) { + throw "Duplicate expected skill id '$($leaf.id)'." + } + $expectedLeaves.Add([string]$leaf.id, $leaf) + } + foreach ($skip in @($expectedComposition.skipped)) { + if (-not $expectedIds.Add([string]$skip.id)) { + throw "Duplicate or selected-and-skipped expected skill id '$($skip.id)'." + } + $expectedSkips.Add([string]$skip.id, $skip) + } + } + catch { + throw "Invalid expected composition: $($_.Exception.Message)" + } +} + $retrieved = [Collections.Generic.HashSet[string]]::new([StringComparer]::Ordinal) foreach ($path in $RetrievedArticlePaths) { $retrieved.Add($path) | Out-Null @@ -79,8 +135,14 @@ function Get-SemanticErrors { } function Get-DerivedSuperOutcome { - param([object[]] $SubResults) + param([object[]] $SubResults, [int] $MissingResults = 0) + if ($MissingResults -gt 0) { + if (@($SubResults | Where-Object outcome -CNE 'failed').Count) { + return 'partial' + } + return 'failed' + } if (-not $SubResults.Count) { return 'not-applicable' } @@ -342,20 +404,87 @@ function Get-SemanticErrors { if ($CurrentSkillKind -ceq 'super' -and $hasSubResults) { $subResults = @($Current.'sub-results') + $producerIds = [Collections.Generic.HashSet[string]]::new([StringComparer]::Ordinal) for ($index = 0; $index -lt $subResults.Count; $index++) { + if (-not $producerIds.Add([string]$subResults[$index].skill.id)) { + Add-Error 'SUPER_DUPLICATE_SUB_RESULT' "$ReportPathPrefix.sub-results[$index].skill.id" ` + 'A leaf may appear only once in sub-results.' + } Test-Report $subResults[$index] "$ReportPathPrefix.sub-results[$index]" 'leaf' } - $expectedOutcome = Get-DerivedSuperOutcome $subResults + $skippedIds = [Collections.Generic.HashSet[string]]::new([StringComparer]::Ordinal) + $skips = if ($hasSkippedSubSkills) { @($Current.'skipped-sub-skills') } else { @() } + foreach ($skip in $skips) { + if (-not $skippedIds.Add([string]$skip.skill.id) -or $producerIds.Contains([string]$skip.skill.id)) { + Add-Error 'SUPER_SKIP_CONFLICT' "$ReportPathPrefix.skipped-sub-skills" ` + "Skill '$($skip.skill.id)' is duplicated or both returned and skipped." + } + } + + $missingResults = 0 + if ($null -ne $expectedComposition) { + if ($Current.skill.id -cne $expectedComposition.superSkill.id -or + $Current.skill.version -ne $expectedComposition.superSkill.version) { + Add-Error 'SUPER_IDENTITY_MISMATCH' "$ReportPathPrefix.skill" 'Super-skill identity differs from expected composition.' + } + $previousSlot = -1 + $orderedIds = @($expectedComposition.subSkills | ForEach-Object { $_.id }) + for ($index = 0; $index -lt $subResults.Count; $index++) { + $identity = $subResults[$index].skill + if (-not $expectedLeaves.ContainsKey([string]$identity.id)) { + Add-Error 'SUPER_UNEXPECTED_SUB_RESULT' "$ReportPathPrefix.sub-results[$index].skill" ` + "Skill '$($identity.id)' was not selected." + continue + } + if ($identity.version -ne $expectedLeaves[$identity.id].version) { + Add-Error 'SUPER_LEAF_VERSION_MISMATCH' "$ReportPathPrefix.sub-results[$index].skill.version" ` + "Unexpected version for '$($identity.id)'." + } + $slot = [Array]::IndexOf($orderedIds, $identity.id) + if ($slot -le $previousSlot) { + Add-Error 'SUPER_SUB_RESULT_ORDER' "$ReportPathPrefix.sub-results[$index].skill" ` + 'Sub-results must preserve the selected worklist order.' + } + $previousSlot = $slot + } + foreach ($leaf in @($expectedComposition.subSkills)) { + if (-not $producerIds.Contains([string]$leaf.id)) { + $missingResults++ + } + } + foreach ($skip in $skips) { + if (-not $expectedSkips.ContainsKey([string]$skip.skill.id)) { + Add-Error 'SUPER_UNEXPECTED_SKIP' "$ReportPathPrefix.skipped-sub-skills" ` + "Skill '$($skip.skill.id)' was not excluded by the coordinator." + continue + } + $expectedSkip = $expectedSkips[$skip.skill.id] + if ($skip.skill.version -ne $expectedSkip.version -or $skip.reason -cne $expectedSkip.reason) { + Add-Error 'SUPER_SKIP_MISMATCH' "$ReportPathPrefix.skipped-sub-skills" ` + "Skip identity or reason differs for '$($skip.skill.id)'." + } + } + foreach ($skip in @($expectedComposition.skipped)) { + if (-not $skippedIds.Contains([string]$skip.id)) { + Add-Error 'SUPER_SKIP_MISSING' "$ReportPathPrefix.skipped-sub-skills" ` + "Expected exclusion '$($skip.id)' is missing." + } + } + } + + $expectedOutcome = Get-DerivedSuperOutcome $subResults $missingResults if ($Current.outcome -cne $expectedOutcome) { Add-Error 'SUPER_OUTCOME_MISMATCH' "$ReportPathPrefix.outcome" "Expected '$expectedOutcome' from sub-results." } $includedSubResults = @($subResults | Where-Object outcome -CNE 'failed') - $expectedWorklistSize = ($includedSubResults | Measure-Object -Property { $_.summary.coverage.'worklist-size' } -Sum).Sum - $expectedItemsEvaluated = ($includedSubResults | Measure-Object -Property { $_.summary.coverage.'items-evaluated' } -Sum).Sum - if ($null -eq $expectedWorklistSize) { $expectedWorklistSize = 0 } - if ($null -eq $expectedItemsEvaluated) { $expectedItemsEvaluated = 0 } + $expectedWorklistSize = 0 + $expectedItemsEvaluated = 0 + foreach ($subResult in $includedSubResults) { + $expectedWorklistSize += $subResult.summary.coverage.'worklist-size' + $expectedItemsEvaluated += $subResult.summary.coverage.'items-evaluated' + } if ($worklistSize -ne $expectedWorklistSize -or $itemsEvaluated -ne $expectedItemsEvaluated) { Add-Error 'SUPER_COVERAGE_MISMATCH' "$ReportPathPrefix.summary.coverage" ` "Expected worklist-size $expectedWorklistSize and items-evaluated $expectedItemsEvaluated from non-failed sub-results."