diff --git a/tools/Test-ReviewContract.ps1 b/tools/Test-ReviewContract.ps1 index 7144f55..185eb9a 100644 --- a/tools/Test-ReviewContract.ps1 +++ b/tools/Test-ReviewContract.ps1 @@ -195,6 +195,7 @@ try { New-Item -ItemType Directory -Path (Split-Path -Parent $sourceFile) -Force | Out-Null Set-Content -LiteralPath $sourceFile -Value @('line one', 'line two', 'line three') -Encoding utf8NoBOM $articlePath = 'microsoft/knowledge/style/caption-required-on-page-fields.md' + $supportingArticlePath = 'microsoft/knowledge/style/tooltip-required-on-page-fields.md' $reportPath = Join-Path $tmp 'report.json' $validReport = [ordered]@{ @@ -297,6 +298,56 @@ try { -SourceRoot $tmp -SourcePaths $sourcePath -RetrievedArticlePaths $articlePath Assert-True (-not $acceptedDeduplicatedSuper.normalized) 'one top-level finding may deduplicate the same citation from two leaves' + $twoOccurrenceLeaf = $styleFindingLeaf | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $secondOccurrence = $twoOccurrenceLeaf.findings[0] | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $secondOccurrence.location.line = 1 + $secondOccurrence.location.range.'start-line' = 1 + $secondOccurrence.location.range.'end-line' = 1 + $twoOccurrenceLeaf.findings = @($twoOccurrenceLeaf.findings[0], $secondOccurrence) + $twoOccurrenceLeaf.summary.counts.minor = 2 + $emptySecurityLeaf = $completedLeaf | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $emptySecurityLeaf.skill.id = 'al-security-review' + $sameIdOccurrenceOmitted = $deduplicatedSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $sameIdOccurrenceOmitted.'sub-results' = @($twoOccurrenceLeaf, $emptySecurityLeaf) + Set-Content -LiteralPath $reportPath -Value ($sameIdOccurrenceOmitted | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + Assert-ThrowsLike -Pattern '*SUPER_FINDING_MISSING*' -Action { + & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super ` + -SourceRoot $tmp -SourcePaths $sourcePath -RetrievedArticlePaths $articlePath + } + + $mergeOwnerLeaf = $styleFindingLeaf | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $mergeOwnerLeaf.findings[0] | Add-Member -NotePropertyName 'suggested-code' -NotePropertyValue 'Caption = ''Customer name'';' + $supportingFindingLeaf = $securityFindingLeaf | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $supportingFindingLeaf.findings[0].id = $supportingArticlePath + $supportingFindingLeaf.findings[0].references[0].path = $supportingArticlePath + $supportingFindingLeaf.findings[0].confidence = 'medium' + $supportingFindingLeaf.findings[0].message = 'The field needs the same mechanical correction for a supporting rule.' + $supportingFindingLeaf.findings[0] | Add-Member -NotePropertyName 'suggested-code' -NotePropertyValue 'Caption = ''Customer name'';' + $mergedFinding = $rolledFinding | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $mergedFinding | Add-Member -NotePropertyName 'suggested-code' -NotePropertyValue 'Caption = ''Customer name'';' + $mergedFinding.references = @( + [pscustomobject]@{ path = $articlePath } + [pscustomobject]@{ path = $supportingArticlePath } + ) + $mergedSuperReport = $deduplicatedSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $mergedSuperReport.findings = @($mergedFinding) + $mergedSuperReport.'sub-results' = @($mergeOwnerLeaf, $supportingFindingLeaf) + Set-Content -LiteralPath $reportPath -Value ($mergedSuperReport | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + $acceptedMergedSuper = & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super ` + -SourceRoot $tmp -SourcePaths $sourcePath -RetrievedArticlePaths $articlePath, $supportingArticlePath + Assert-True (-not $acceptedMergedSuper.normalized) 'overlapping A and B findings may merge into A with B as a supporting reference' + + $unmergedSupportingFinding = $supportingFindingLeaf.findings[0] | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $unmergedSupportingFinding | Add-Member -NotePropertyName 'from-sub-skill' -NotePropertyValue 'al-security-review' + $unmergedDuplicates = $mergedSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $unmergedDuplicates.summary.counts.minor = 2 + $unmergedDuplicates.findings = @($mergedFinding, $unmergedSupportingFinding) + Set-Content -LiteralPath $reportPath -Value ($unmergedDuplicates | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + Assert-ThrowsLike -Pattern '*SUPER_DUPLICATE_FINDINGS*' -Action { + & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super ` + -SourceRoot $tmp -SourcePaths $sourcePath -RetrievedArticlePaths $articlePath, $supportingArticlePath + } + $omittedLeafFinding = $deduplicatedSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json $omittedLeafFinding.summary.counts.minor = 0 $omittedLeafFinding.findings = @() @@ -347,6 +398,17 @@ try { -SourceRoot $tmp -SourcePaths $sourcePath -RetrievedArticlePaths $articlePath } + $failedLeafRelabeledAsAgent = $partialSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json + $failedLeafRelabeledAsAgent.summary.counts.minor = 1 + $failedLeafRelabeledAsAgent.findings = @($rolledFinding | ConvertTo-Json -Depth 20 | ConvertFrom-Json) + $failedLeafRelabeledAsAgent.findings[0].'from-sub-skill' = 'agent' + $failedLeafRelabeledAsAgent.findings[0].domain = 'Agent' + Set-Content -LiteralPath $reportPath -Value ($failedLeafRelabeledAsAgent | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM + Assert-ThrowsLike -Pattern '*SUPER_AGENT_FINDING_INVALID*' -Action { + & $validator -ReportPath $reportPath -BCQualityRoot $Root -SkillKind super ` + -SourceRoot $tmp -SourcePaths $sourcePath -RetrievedArticlePaths $articlePath + } + $incorrectOutcome = $validSuperReport | ConvertTo-Json -Depth 20 | ConvertFrom-Json $incorrectOutcome.outcome = 'not-applicable' Set-Content -LiteralPath $reportPath -Value ($incorrectOutcome | ConvertTo-Json -Depth 20) -Encoding utf8NoBOM diff --git a/tools/Validate-FindingsReport.ps1 b/tools/Validate-FindingsReport.ps1 index 7be25d9..a7d3aa7 100644 --- a/tools/Validate-FindingsReport.ps1 +++ b/tools/Validate-FindingsReport.ps1 @@ -103,65 +103,98 @@ function Get-SemanticErrors { return 'completed' } - function Get-RolledFindingId { - param([object] $Finding, [string] $ProducerId) - - if (@($Finding.references).Count) { - return $Finding.id - } - return "${ProducerId}:$($Finding.id)" + function Get-SeverityRank { + param([string] $Severity) + return @{'info' = 0; 'minor' = 1; 'major' = 2; 'blocker' = 3}[$Severity] } - function Test-RolledFindingMatches { - param([object] $RolledFinding, [object] $LeafFinding, [string] $ProducerId) + function Get-ConfidenceRank { + param([string] $Confidence) + return @{'low' = 0; 'medium' = 1; 'high' = 2}[$Confidence] + } - if ($RolledFinding.id -cne (Get-RolledFindingId $LeafFinding $ProducerId)) { + function Test-LocationsOverlap { + param([object] $First, [object] $Second) + + $firstHasLocation = Test-HasProperty $First 'location' + $secondHasLocation = Test-HasProperty $Second 'location' + if ($firstHasLocation -ne $secondHasLocation) { return $false } - foreach ($name in 'severity', 'message', 'confidence', 'domain', 'suggested-code', 'suggested-code-omission-reason') { - $rolledHasProperty = Test-HasProperty $RolledFinding $name - $leafHasProperty = Test-HasProperty $LeafFinding $name - if ($rolledHasProperty -ne $leafHasProperty -or - ($rolledHasProperty -and $RolledFinding.$name -cne $LeafFinding.$name)) { - return $false - } + if (-not $firstHasLocation) { + return $true } - - $rolledHasLocation = Test-HasProperty $RolledFinding 'location' - $leafHasLocation = Test-HasProperty $LeafFinding 'location' - if ($rolledHasLocation -ne $leafHasLocation) { + if ($First.location.file -cne $Second.location.file) { return $false } - if ($rolledHasLocation) { - if ($RolledFinding.location.file -cne $LeafFinding.location.file -or - $RolledFinding.location.line -ne $LeafFinding.location.line) { - return $false - } - $rolledHasRange = Test-HasProperty $RolledFinding.location 'range' - $leafHasRange = Test-HasProperty $LeafFinding.location 'range' - if ($rolledHasRange -ne $leafHasRange -or - ($rolledHasRange -and - ($RolledFinding.location.range.'start-line' -ne $LeafFinding.location.range.'start-line' -or - $RolledFinding.location.range.'end-line' -ne $LeafFinding.location.range.'end-line'))) { + $firstEnd = if (Test-HasProperty $First.location 'range') { $First.location.range.'end-line' } else { $First.location.line } + $secondEnd = if (Test-HasProperty $Second.location 'range') { $Second.location.range.'end-line' } else { $Second.location.line } + return $First.location.line -le $secondEnd -and $Second.location.line -le $firstEnd + } + + function Test-SameCorrection { + param([object] $First, [object] $Second) + + $firstHasCode = Test-HasProperty $First 'suggested-code' + $secondHasCode = Test-HasProperty $Second 'suggested-code' + if ($firstHasCode -or $secondHasCode) { + return $firstHasCode -and $secondHasCode -and $First.'suggested-code' -ceq $Second.'suggested-code' + } + return $First.message -ceq $Second.message + } + + function Test-ReferencesInclude { + param([object[]] $RolledReferences, [object[]] $LeafReferences) + + foreach ($leafReference in $LeafReferences) { + $matched = @($RolledReferences | Where-Object { + if ($_.path -cne $leafReference.path) { + return $false + } + $rolledHasSha = Test-HasProperty $_ 'sha' + $leafHasSha = Test-HasProperty $leafReference 'sha' + return $rolledHasSha -eq $leafHasSha -and + (-not $rolledHasSha -or $_.sha -ceq $leafReference.sha) + }).Count + if (-not $matched) { return $false } } + return $true + } + + function Test-RolledFindingRepresents { + param( + [object] $RolledFinding, + [object] $LeafFinding, + [string] $LeafProducerId, + [switch] $RequirePrimaryOwner + ) + + if (-not (Test-LocationsOverlap $RolledFinding $LeafFinding) -or + -not (Test-SameCorrection $RolledFinding $LeafFinding) -or + (Get-SeverityRank $RolledFinding.severity) -lt (Get-SeverityRank $LeafFinding.severity) -or + (Get-ConfidenceRank $RolledFinding.confidence) -lt (Get-ConfidenceRank $LeafFinding.confidence)) { + return $false + } - $rolledReferences = @($RolledFinding.references) $leafReferences = @($LeafFinding.references) - if ($rolledReferences.Count -ne $leafReferences.Count) { + if (-not $leafReferences.Count) { + return $RolledFinding.'from-sub-skill' -ceq $LeafProducerId -and + $RolledFinding.id -ceq "${LeafProducerId}:$($LeafFinding.id)" -and + -not @($RolledFinding.references).Count + } + if (-not (Test-ReferencesInclude @($RolledFinding.references) $leafReferences)) { return $false } - for ($index = 0; $index -lt $rolledReferences.Count; $index++) { - if ($rolledReferences[$index].path -cne $leafReferences[$index].path) { - return $false - } - $rolledHasSha = Test-HasProperty $rolledReferences[$index] 'sha' - $leafHasSha = Test-HasProperty $leafReferences[$index] 'sha' - if ($rolledHasSha -ne $leafHasSha -or - ($rolledHasSha -and $rolledReferences[$index].sha -cne $leafReferences[$index].sha)) { - return $false - } + if ($RequirePrimaryOwner) { + $rolledHasDomain = Test-HasProperty $RolledFinding 'domain' + $leafHasDomain = Test-HasProperty $LeafFinding 'domain' + return $RolledFinding.'from-sub-skill' -ceq $LeafProducerId -and + $RolledFinding.id -ceq $LeafFinding.id -and + @($RolledFinding.references)[0].path -ceq $leafReferences[0].path -and + $rolledHasDomain -eq $leafHasDomain -and + (-not $rolledHasDomain -or $RolledFinding.domain -ceq $LeafFinding.domain) } return $true } @@ -293,22 +326,29 @@ function Get-SemanticErrors { } $failedProducerIds = @($subResults | Where-Object outcome -CEQ 'failed' | ForEach-Object { $_.skill.id }) - $eligibleFindingsById = [Collections.Generic.Dictionary[string, object]]::new([StringComparer]::Ordinal) + $includedProducerIds = [Collections.Generic.HashSet[string]]::new([StringComparer]::Ordinal) + $eligibleFindings = [Collections.Generic.List[object]]::new() foreach ($subResult in $includedSubResults) { + $includedProducerIds.Add([string]$subResult.skill.id) | Out-Null foreach ($finding in @($subResult.findings)) { - $rolledId = Get-RolledFindingId $finding $subResult.skill.id - if (-not $eligibleFindingsById.ContainsKey($rolledId)) { - $eligibleFindingsById[$rolledId] = [Collections.Generic.HashSet[string]]::new([StringComparer]::Ordinal) - } - $eligibleFindingsById[$rolledId].Add([string]$subResult.skill.id) | Out-Null + $eligibleFindings.Add([pscustomobject]@{ + ProducerId = [string]$subResult.skill.id + Finding = $finding + }) | Out-Null } } - $validRolledIds = [Collections.Generic.HashSet[string]]::new([StringComparer]::Ordinal) for ($index = 0; $index -lt $findings.Count; $index++) { $finding = $findings[$index] $producerId = [string]$finding.'from-sub-skill' if ($producerId -ceq 'agent') { + if (@($finding.references).Count -or + $finding.id -cnotmatch '^agent:[a-z0-9]+(?:-[a-z0-9]+)*$' -or + -not (Test-HasProperty $finding 'domain') -or + $finding.domain -cne 'Agent') { + Add-Error 'SUPER_AGENT_FINDING_INVALID' "$ReportPathPrefix.findings[$index]" ` + 'A super-skill agent finding must use an agent: id, Agent domain, and no references.' + } continue } if ($producerId -cin $failedProducerIds) { @@ -316,30 +356,52 @@ function Get-SemanticErrors { "Finding is attributed to failed sub-skill '$producerId'." continue } - if (-not $eligibleFindingsById.ContainsKey($finding.id) -or - -not $eligibleFindingsById[$finding.id].Contains($producerId)) { + if (-not $includedProducerIds.Contains($producerId)) { Add-Error 'SUPER_PRODUCER_INVALID' "$ReportPathPrefix.findings[$index].from-sub-skill" ` - "Sub-skill '$producerId' did not emit finding '$($finding.id)' in a non-failed result." + "Sub-skill '$producerId' has no non-failed result." continue } - $producerLeafFindings = @( - $includedSubResults | - Where-Object { $_.skill.id -ceq $producerId } | - ForEach-Object { $_.findings } | - Where-Object { (Get-RolledFindingId $_ $producerId) -ceq $finding.id } - ) - if (-not @($producerLeafFindings | Where-Object { Test-RolledFindingMatches $finding $_ $producerId }).Count) { + $ownedLeafFindings = @($eligibleFindings | Where-Object { + $_.ProducerId -ceq $producerId -and + (Test-RolledFindingRepresents $finding $_.Finding $_.ProducerId -RequirePrimaryOwner) + }) + if (-not $ownedLeafFindings.Count) { Add-Error 'SUPER_FINDING_MISMATCH' "$ReportPathPrefix.findings[$index]" ` - "Rolled-up finding '$($finding.id)' does not preserve the finding emitted by '$producerId'." + "Rolled-up finding '$($finding.id)' is not owned by a matching finding from '$producerId'." continue } - $validRolledIds.Add([string]$finding.id) | Out-Null + $representedLeafFindings = @($eligibleFindings | Where-Object { + Test-RolledFindingRepresents $finding $_.Finding $_.ProducerId + }) + $expectedSeverityRank = ($representedLeafFindings | ForEach-Object { Get-SeverityRank $_.Finding.severity } | Measure-Object -Maximum).Maximum + $expectedConfidenceRank = ($representedLeafFindings | ForEach-Object { Get-ConfidenceRank $_.Finding.confidence } | Measure-Object -Maximum).Maximum + if ((Get-SeverityRank $finding.severity) -ne $expectedSeverityRank -or + (Get-ConfidenceRank $finding.confidence) -ne $expectedConfidenceRank) { + Add-Error 'SUPER_FINDING_MISMATCH' "$ReportPathPrefix.findings[$index]" ` + "Rolled-up finding '$($finding.id)' must retain the highest severity and confidence justified by its represented leaf findings." + } } - foreach ($rolledId in $eligibleFindingsById.Keys) { - if (-not $validRolledIds.Contains($rolledId)) { + for ($firstIndex = 0; $firstIndex -lt $findings.Count; $firstIndex++) { + for ($secondIndex = $firstIndex + 1; $secondIndex -lt $findings.Count; $secondIndex++) { + if ((Test-HasProperty $findings[$firstIndex] 'location') -and + (Test-HasProperty $findings[$secondIndex] 'location') -and + (Test-LocationsOverlap $findings[$firstIndex] $findings[$secondIndex]) -and + (Test-SameCorrection $findings[$firstIndex] $findings[$secondIndex])) { + Add-Error 'SUPER_DUPLICATE_FINDINGS' "$ReportPathPrefix.findings[$secondIndex]" ` + "Top-level findings $firstIndex and $secondIndex overlap and prescribe the same correction; they must be merged." + } + } + } + + foreach ($eligibleFinding in $eligibleFindings) { + $represented = @($findings | Where-Object { + $_.'from-sub-skill' -cne 'agent' -and + (Test-RolledFindingRepresents $_ $eligibleFinding.Finding $eligibleFinding.ProducerId) + }).Count + if (-not $represented) { Add-Error 'SUPER_FINDING_MISSING' "$ReportPathPrefix.findings" ` - "No valid rolled-up finding represents non-failed leaf finding '$rolledId'." + "No valid rolled-up finding represents a finding from '$($eligibleFinding.ProducerId)' at its source location." } } }