diff --git a/.github/workflows/review-fixtures.yml b/.github/workflows/review-fixtures.yml index 0f39d9a..788c32c 100644 --- a/.github/workflows/review-fixtures.yml +++ b/.github/workflows/review-fixtures.yml @@ -16,3 +16,7 @@ jobs: - name: Validate review evaluation corpus shell: pwsh run: ./tools/Test-ReviewFixtures.ps1 -Root . -PrepareDirectory "$env:RUNNER_TEMP/bcquality-review-fixtures" + + - name: Validate review contracts + shell: pwsh + run: ./tools/Test-ReviewContract.ps1 -Root . diff --git a/docs/contributing.md b/docs/contributing.md index 27da177..51f6d0f 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -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 diff --git a/docs/standalone-runner.md b/docs/standalone-runner.md index 61ecb55..5829e4e 100644 --- a/docs/standalone-runner.md +++ b/docs/standalone-runner.md @@ -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; diff --git a/microsoft/skills/review/al-code-review.md b/microsoft/skills/review/al-code-review.md index 6a577ba..9239220 100644 --- a/microsoft/skills/review/al-code-review.md +++ b/microsoft/skills/review/al-code-review.md @@ -67,7 +67,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). @@ -80,7 +80,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 `:` to prevent collisions across sub-skills. Other finding fields are preserved. @@ -126,9 +126,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 diff --git a/skills/do.md b/skills/do.md index f86509b..9abdf58 100644 --- a/skills/do.md +++ b/skills/do.md @@ -161,13 +161,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 @@ -183,11 +219,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 diff --git a/tools/Test-ReviewContract.ps1 b/tools/Test-ReviewContract.ps1 new file mode 100644 index 0000000..1d40058 --- /dev/null +++ b/tools/Test-ReviewContract.ps1 @@ -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)."