From 29f42b29b83382db7bec4e3827fc5881d4877b8b Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Tue, 25 Aug 2026 11:13:13 +0100 Subject: [PATCH 1/2] feat(ci-versioning): derive the subject set from what the build staged The check attributes failures only to namespaces the repository under test declares, so it needs to know which assemblies are that repository's own. It read them from a Build directory at the workspace root, on the assumption that every project wrote there. Nothing guaranteed that assumption. The convention was enforced by a linter that rewrote OutputPath values already present, never added a missing one, and never inspected which configuration a value applied to. No check read the directory until this one. Measured across 138 projects in 34 repositories: 30% did not write there under the configuration CI builds, and a different 35% would not under the other, so no configuration made it reliable. The failure had two shapes. With no project writing there the directory was absent and attribution widened to the whole closure, reporting other repositories' failures against this one -- 1,056 of 1,056 on a real pull request. With only some projects writing there the directory existed, the subject set was real but partial, nothing warned, and the check silently covered a fraction of the repository. Every BHoM project stages its output to the shared assembly directory through a PostBuild step, and that directory is what the runner reflects over. So the set staged during this repository's build is both what it produced and what the runner can actually see. Two snapshots either side of the build, and the difference is the subject set. That has a property the directory did not. A project whose output never reaches the assembly directory is also never loaded by the runner, so it can produce no finding, so omitting it from the subject set is correct rather than a gap. The set cannot be partially right in the way the directory could. The comparison keys on name and write time, not name alone. A repository can produce an assembly whose file name already exists from a dependency, and staging overwrites it in place; comparing names would treat that as unchanged and drop the repository's own assembly, which is the partial-subject failure reintroduced one level down. It has its own tests. Alt configurations are inside the bracket deliberately. A Revit repository's year-suffixed assemblies are its own code, so attributing failures in them to it is correct, and excluding them would understate what the check covers. The bracket is an ordering assumption in a file where steps get inserted, so a test fails if anything is added between the snapshots, or if a nested action appears there that could change the assembly directory. It asserts the shape rather than today's list. The precondition guard is reshaped, not removed. It now asserts that the build staged at least one assembly, in the same place and with the same failure behaviour. Its message describes a build that produced nothing rather than a configuration mistake, because the configuration is no longer what determines the answer. --subject-assemblies becomes --subject-assembly-list and takes a file. It had one caller. A directory of copies was considered and rejected: it would duplicate every assembly and reintroduce the filesystem convention this removes. --- .github/actions/ci-versioning/action.yml | 95 ++++++++---- .github/scripts/Get-StagedAssemblies.ps1 | 80 ++++++++++ .../tests/Get-StagedAssemblies.Tests.ps1 | 142 ++++++++++++++++++ .../tests/ci-versioning-action.Tests.ps1 | 61 ++++++-- .../VersioningRunner.Tests/RunCommandTests.cs | 96 ++++++------ .../VersioningRunner/Commands/RunCommand.cs | 75 ++++++--- .../src/VersioningRunner/Program.cs | 9 +- 7 files changed, 449 insertions(+), 109 deletions(-) create mode 100644 .github/scripts/Get-StagedAssemblies.ps1 create mode 100644 .github/scripts/tests/Get-StagedAssemblies.Tests.ps1 diff --git a/.github/actions/ci-versioning/action.yml b/.github/actions/ci-versioning/action.yml index a7ecddd..11f7113 100644 --- a/.github/actions/ci-versioning/action.yml +++ b/.github/actions/ci-versioning/action.yml @@ -149,6 +149,27 @@ runs: uses: BHoM/CI_Toolkit/.github/actions/discover-solution@develop with: check_title: Versioning + # --- Subject assembly bracket: opens here, closes after the alt-config builds --- + # + # The two steps between these snapshots are the only ones that build this repository, and + # everything a BHoM project builds is staged into the assembly directory by its PostBuild + # step. So the difference between the two snapshots is exactly this repository's own output. + # + # NOTHING MAY BE INSERTED BETWEEN THEM. Any step that builds, restores or copies into the + # assembly directory would be attributed to this repository and widen the subject set. + # Later steps deliberately fall outside: the Revit mocks, the upgrades recapture and the + # verification-solution build all stage assemblies that are not this repository's. + # ci-versioning-action.Tests.ps1 fails if a step is added inside the bracket. + - name: Snapshot staged assemblies (before subject build) + id: stage_before + if: steps.changed.outputs.count != '0' + shell: pwsh + run: | + $script = Join-Path $env:GITHUB_ACTION_PATH "../../scripts/Get-StagedAssemblies.ps1" + . $script + Get-AssemblyStamp -Path 'C:\ProgramData\BHoM\Assemblies' | + Set-Content -Path 'staged-before.txt' -Encoding utf8 + Write-Host "Staged before subject build: $(@(Get-Content 'staged-before.txt' -ErrorAction SilentlyContinue).Count) assembl(ies)." - name: Build primary repo id: build_subject @@ -186,6 +207,30 @@ runs: $script = Join-Path $env:GITHUB_ACTION_PATH "../../scripts/Build-AltConfigs.ps1" & $script -SlnPath "${{ steps.solution.outputs.path }}" -Configuration "Release" + # --- Subject assembly bracket: closes here --- + - name: Collect subject assemblies + id: subject_set + if: steps.changed.outputs.count != '0' + shell: pwsh + run: | + $script = Join-Path $env:GITHUB_ACTION_PATH "../../scripts/Get-StagedAssemblies.ps1" + . $script + + $before = @(Get-Content 'staged-before.txt' -ErrorAction SilentlyContinue) + $after = Get-AssemblyStamp -Path 'C:\ProgramData\BHoM\Assemblies' + $subject = @(Get-NewlyStagedAssemblies -Before $before -After $after) + + # Alt configurations are inside the bracket on purpose. A Revit repository's + # year-suffixed assemblies are its own code, so attributing failures in them to it is + # correct, and excluding them would understate what the check covers. + $subject | Set-Content -Path 'subject-assemblies.txt' -Encoding utf8 + Write-Host "Subject assemblies staged by this repository's build: $($subject.Count)." + if ($subject.Count -gt 0) { + Write-Host "::group::Subject assemblies" + $subject | ForEach-Object { Write-Host " $_" } + Write-Host "::endgroup::" + } + # There is deliberately no build-completeness fast-fail here any more. # # A "Fast-fail on missing versioning-critical DLLs" step used to download @@ -557,46 +602,40 @@ runs: } Write-Host "::notice title=Versioning::Found versioning datasets for: $($versions.Name -join ', ')" - # Precondition, not a policy choice. The runner is handed - # --subject-assemblies "\Build" and restricts attribution to the - # namespaces the assemblies there declare. If that directory is absent it falls - # back to attributing every failure across the whole dependency closure, which - # reports other repositories' defects against this one: measured at 1,056 of 1,056 - # on a real pull request. The runner does warn, but on stderr, and a warning does - # not stop the check producing a verdict it has no basis for. - # - # Asserted here rather than left to the runner because this is a property of the - # build, and the build is this action's job. Same shape as the dataset guard above. + # Precondition, not a policy choice. Attribution narrows to the namespaces this + # repository's own assemblies declare; with no assemblies there is nothing to narrow to, + # and the runner would widen to the whole dependency closure and report other + # repositories' failures against this one. Measured at 1,056 of 1,056 on a real pull + # request. # - # Not the vacuous-green question. "Nothing to check" is a legitimate green; - # "the thing to check was never built" is a broken precondition. This is the second. + # Asserted here rather than in the runner because it is a property of the build, and the + # build is this action's job. Same shape as the datasets guard above. # - # Two causes when it fires, and they need different fixes. Either the projects declare - # ..\Build\ only inside PropertyGroups conditioned on Debug or Test, so a - # Release build does not apply it; or they declare no at all and take the - # SDK default. Both send output to bin\\\ and leave Build\ absent. + # Not the vacuous-green question. "Nothing to check" is a legitimate green; "the thing to + # check was never built" is a broken precondition. This is the second. # - # Neither is a fault in those repositories. Nothing ever verified this directory: the - # convention was enforced by a linter that only rewrote lines already - # present, and no check read Build\ until this one. + # When this fires it is a build problem, not a configuration one: the solution built, but + # staged no assemblies into the shared assembly directory. Every BHoM project stages its + # output there through a PostBuild step, so a repository producing none either built + # nothing or has lost that step. - name: Validate subject build output if: steps.changed.outputs.count != '0' shell: pwsh run: | - $subjectDir = "${{ github.workspace }}\Build" + $listPath = 'subject-assemblies.txt' - if (-not (Test-Path $subjectDir)) { - Write-Host "::error title=Versioning::Subject build output missing at $subjectDir. This repository's own assemblies were not built to the directory the check attributes against, so no failure could be attributed to it. Two usual causes: the projects declare ..\Build\ only under Debug or Test conditions, which a Release build does not apply; or they declare no at all and take the SDK default. Either way the output went to bin\Release\\ instead." + if (-not (Test-Path $listPath)) { + Write-Host "::error title=Versioning::Subject assembly list missing. The step that collects this repository's build output did not run, so attribution has no subject set." exit 1 } - $dlls = @(Get-ChildItem $subjectDir -Filter *.dll -Recurse -ErrorAction SilentlyContinue) - if ($dlls.Count -eq 0) { - Write-Host "::error title=Versioning::Subject build output at $subjectDir contains no assemblies. The directory exists but nothing was built into it, so no failure could be attributed to this repository." + $subject = @(Get-Content $listPath | Where-Object { $_.Trim() }) + if ($subject.Count -eq 0) { + Write-Host "::error title=Versioning::This repository's build staged no assemblies. The solution built, but nothing reached C:\ProgramData\BHoM\Assemblies, so no failure could be attributed to this repository. Every BHoM project stages its output there through a PostBuild step; a repository producing none either built no assemblies or is missing that step." exit 1 } - Write-Host "::notice title=Versioning::Subject build output: $($dlls.Count) assembl(ies) in $subjectDir." + Write-Host "::notice title=Versioning::Subject set: $($subject.Count) assembl(ies) staged by this repository's build." - name: Prepare VersioningRunner id: runner @@ -670,7 +709,7 @@ runs: if: steps.changed.outputs.count != '0' shell: pwsh run: | - # --subject-assemblies restricts attribution to the namespaces this repo's own + # --subject-assembly-list restricts attribution to the namespaces this repo's own # assemblies declare. Without it, failures are attributed on a 3-segment # namespace prefix over the whole dependency closure, which cannot distinguish # BH.oM.Adapters.File from BH.oM.Adapters.ETABS: the v9.2 datasets name thousands @@ -690,7 +729,7 @@ runs: # annotations were produced, against two on a comparable production run. & "${{ steps.runner.outputs.runner_exe }}" ` --assemblies 'C:\ProgramData\BHoM\Assemblies' ` - --subject-assemblies "${{ github.workspace }}\Build" ` + --subject-assembly-list 'subject-assemblies.txt' ` --configuration 'Release' ` $(if ('${{ steps.vercond.outputs.file }}') { '--version-conditional', '${{ steps.vercond.outputs.file }}' }) ` --output 'versioning-result.json' | diff --git a/.github/scripts/Get-StagedAssemblies.ps1 b/.github/scripts/Get-StagedAssemblies.ps1 new file mode 100644 index 0000000..5c92d0c --- /dev/null +++ b/.github/scripts/Get-StagedAssemblies.ps1 @@ -0,0 +1,80 @@ +# Get-StagedAssemblies.ps1 — works out which assemblies a build staged, by comparing the +# assembly directory before and after it. +# +# Dot-sourced by .github/actions/ci-versioning/action.yml and by +# .github/scripts/tests/Get-StagedAssemblies.Tests.ps1. Defines functions and does nothing +# else, so dot-sourcing has no side effects. +# +# Why this exists. The versioning check attributes failures only to namespaces the repository +# under test declares, so it needs to know which assemblies are the repository's own. It used +# to read them from a `Build\` directory at the workspace root, on the assumption that every +# project wrote there. Nothing guaranteed that assumption and it was false for roughly a third +# of the fleet, differently under each build configuration, so the check either widened to the +# whole dependency closure and reported other repositories' failures, or attributed against a +# fraction of the repository with nothing to say so. +# +# Every BHoM project stages its output to the shared assembly directory through a PostBuild +# step, and that is the directory the runner reflects over. So the set staged during the +# subject build is both what the repository produced and what the runner can actually see. + +function Get-AssemblyStamp { + <# + .SYNOPSIS + A stable identity per assembly file: name and last-write time. + + .DESCRIPTION + The write time is part of the identity on purpose. A repository can produce an assembly + with the same file name as one already staged by a dependency, and the staging step + overwrites it in place. Comparing names alone would treat that as unchanged and drop the + repository's own assembly from its subject set, which is the failure this whole mechanism + exists to remove — silently attributing against an incomplete set. + + .PARAMETER Path + Assembly directory. A missing directory yields an empty stamp set rather than throwing, + so the caller decides what an empty result means. + #> + [CmdletBinding()] + param([Parameter(Mandatory)][string]$Path) + + if (-not (Test-Path $Path)) { return @() } + + return @( + Get-ChildItem -LiteralPath $Path -Filter *.dll -File -ErrorAction SilentlyContinue | + ForEach-Object { "$($_.Name)|$($_.LastWriteTimeUtc.Ticks)" } + ) +} + +function Get-NewlyStagedAssemblies { + <# + .SYNOPSIS + The assembly names present after a build that were not present, identically, before it. + + .DESCRIPTION + Pure over its two inputs so the comparison can be tested without a build. Returns names + rather than stamps, because the runner identifies an assembly by file name. + + An entry counts as newly staged when its name-and-time pair is absent from the before + set. That covers both shapes: an assembly that did not exist before, and one that existed + and was overwritten. + + .PARAMETER Before, After + Stamp collections from Get-AssemblyStamp. + #> + [CmdletBinding()] + param( + [string[]]$Before = @(), + [string[]]$After = @() + ) + + $seen = [System.Collections.Generic.HashSet[string]]::new( + [string[]]@($Before), [System.StringComparer]::OrdinalIgnoreCase) + + $names = foreach ($entry in @($After)) { + if (-not $seen.Contains($entry)) { ($entry -split '\|', 2)[0] } + } + + # Emitted as a sequence, not wrapped. A comma-wrap here would return an array containing + # the array, which counts as one element and silently breaks any caller that measures it. + # Callers that need a definite collection wrap with @(), which is the repository's idiom. + return @($names | Sort-Object -Unique) +} diff --git a/.github/scripts/tests/Get-StagedAssemblies.Tests.ps1 b/.github/scripts/tests/Get-StagedAssemblies.Tests.ps1 new file mode 100644 index 0000000..641cf5b --- /dev/null +++ b/.github/scripts/tests/Get-StagedAssemblies.Tests.ps1 @@ -0,0 +1,142 @@ +# Get-StagedAssemblies.Tests.ps1 — Pester tests for the staged-assembly comparison. +# +# The comparison decides which assemblies the versioning check treats as the repository's own, +# so getting it wrong reproduces the defect it replaces: a subject set that looks plausible and +# silently covers part of the repository. +# +# Get-NewlyStagedAssemblies is pure over two stamp collections, so every case below is checked +# without a build. Get-AssemblyStamp touches the filesystem and is tested against real temp +# directories, because its contract is about what it does with files that are missing, nested +# or not assemblies. +# +# Run locally: pwsh -Command "Invoke-Pester .github/scripts/tests -Output Detailed" +# Run in CI: lint-workflows.yml, the powershell-tests job. + +BeforeAll { + $repoRoot = (Resolve-Path (Join-Path $PSScriptRoot '../../..')).Path + . (Join-Path $repoRoot '.github/scripts/Get-StagedAssemblies.ps1') +} + +Describe 'Get-NewlyStagedAssemblies' { + + Context 'the ordinary case' { + + It 'returns assemblies that were not there before' { + $before = @('Dep_oM.dll|100', 'Dep_Engine.dll|100') + $after = @('Dep_oM.dll|100', 'Dep_Engine.dll|100', 'Subject_oM.dll|200') + + Get-NewlyStagedAssemblies -Before $before -After $after | Should -Be @('Subject_oM.dll') + } + + It 'returns nothing when the build staged nothing' { + $stamps = @('Dep_oM.dll|100', 'Dep_Engine.dll|100') + + @(Get-NewlyStagedAssemblies -Before $stamps -After $stamps).Count | Should -Be 0 + } + + It 'returns every new assembly, not just the first' { + $after = @('A_oM.dll|200', 'B_Engine.dll|200', 'C_Adapter.dll|200') + + Get-NewlyStagedAssemblies -Before @() -After $after | + Should -Be @('A_oM.dll', 'B_Engine.dll', 'C_Adapter.dll') + } + } + + Context 'the name-collision case, which a name-only comparison gets wrong' { + + # A repository can produce an assembly whose file name already exists in the staging + # directory from a dependency. The staging step overwrites it in place, so the name is + # present both before and after. Comparing names alone concludes nothing changed and + # drops the repository's own assembly from its subject set — which is exactly the + # partial-subject failure this mechanism replaces, reintroduced one layer down. + It 'detects an assembly overwritten in place' { + $before = @('Shared_oM.dll|100') + $after = @('Shared_oM.dll|200') + + Get-NewlyStagedAssemblies -Before $before -After $after | Should -Be @('Shared_oM.dll') + } + + It 'distinguishes an overwrite from an untouched file with the same name' { + $before = @('Untouched_oM.dll|100', 'Overwritten_oM.dll|100') + $after = @('Untouched_oM.dll|100', 'Overwritten_oM.dll|555') + + Get-NewlyStagedAssemblies -Before $before -After $after | Should -Be @('Overwritten_oM.dll') + } + + It 'reports an overwritten assembly once, not twice' { + # It appears in both inputs; the result is a set of names, not a concatenation. + $r = Get-NewlyStagedAssemblies -Before @('X_oM.dll|1') -After @('X_oM.dll|2') + @($r).Count | Should -Be 1 + } + } + + Context 'shape' { + + # The contract is a sequence of names that counts correctly when the caller wraps it + # with @(), which is what the action does before writing one line per entry. An earlier + # version comma-wrapped the return, which produced an array containing the array: it + # counted as one element regardless of content, so an empty result and a three-element + # result were indistinguishable to the caller. + It 'counts correctly for one result' { + @(Get-NewlyStagedAssemblies -Before @() -After @('Only_oM.dll|1')).Count | Should -Be 1 + } + + It 'counts correctly for no result' { + @(Get-NewlyStagedAssemblies -Before @('A.dll|1') -After @('A.dll|1')).Count | Should -Be 0 + } + + It 'counts correctly for many results' { + @(Get-NewlyStagedAssemblies -Before @() -After @('A.dll|1','B.dll|1','C.dll|1')).Count | Should -Be 3 + } + + It 'tolerates empty inputs on both sides' { + @(Get-NewlyStagedAssemblies -Before @() -After @()).Count | Should -Be 0 + } + } +} + +Describe 'Get-AssemblyStamp' { + + BeforeAll { + $script:dir = Join-Path ([IO.Path]::GetTempPath()) ("stamp-" + [Guid]::NewGuid().ToString('N')) + New-Item -ItemType Directory -Force -Path $dir | Out-Null + New-Item -ItemType Directory -Force -Path (Join-Path $dir 'nested') | Out-Null + Set-Content -Path (Join-Path $dir 'One_oM.dll') -Value 'x' + Set-Content -Path (Join-Path $dir 'Two_Engine.dll') -Value 'x' + Set-Content -Path (Join-Path $dir 'notes.txt') -Value 'x' + Set-Content -Path (Join-Path $dir 'nested\Deep_oM.dll') -Value 'x' + } + + AfterAll { Remove-Item $dir -Recurse -Force -ErrorAction SilentlyContinue } + + It 'stamps each assembly with its name and write time' { + $s = Get-AssemblyStamp -Path $dir + ($s | Where-Object { $_ -like 'One_oM.dll|*' }).Count | Should -Be 1 + } + + It 'ignores files that are not assemblies' { + Get-AssemblyStamp -Path $dir | Should -Not -Contain 'notes.txt' + (Get-AssemblyStamp -Path $dir | Where-Object { $_ -like 'notes*' }).Count | Should -Be 0 + } + + # The staging directory is flat: every project copies its own output into it. Recursing + # would pick up anything a dependency happened to nest there and attribute it to the + # repository under test. + It 'does not recurse' { + (Get-AssemblyStamp -Path $dir | Where-Object { $_ -like 'Deep_oM*' }).Count | Should -Be 0 + } + + It 'returns empty for a directory that does not exist, rather than throwing' { + { Get-AssemblyStamp -Path (Join-Path $dir 'no-such-place') } | Should -Not -Throw + @(Get-AssemblyStamp -Path (Join-Path $dir 'no-such-place')).Count | Should -Be 0 + } + + It 'round-trips through the comparison to name a genuinely new assembly' { + $before = Get-AssemblyStamp -Path $dir + Start-Sleep -Milliseconds 20 + Set-Content -Path (Join-Path $dir 'Fresh_oM.dll') -Value 'x' + $after = Get-AssemblyStamp -Path $dir + + Get-NewlyStagedAssemblies -Before $before -After $after | Should -Be @('Fresh_oM.dll') + } +} diff --git a/.github/scripts/tests/ci-versioning-action.Tests.ps1 b/.github/scripts/tests/ci-versioning-action.Tests.ps1 index 460f5ca..f04f36c 100644 --- a/.github/scripts/tests/ci-versioning-action.Tests.ps1 +++ b/.github/scripts/tests/ci-versioning-action.Tests.ps1 @@ -30,11 +30,11 @@ Describe 'ci-versioning action.yml' { } It 'guards the subject build output the same way' { - $text | Should -Match 'Subject build output missing at' -Because 'an absent subject directory makes the check attribute every failure across the whole closure' + $text | Should -Match 'Subject assembly list missing' -Because 'without a subject set the check attributes every failure across the whole closure' } - It 'also rejects a subject directory that exists but is empty' { - $text | Should -Match 'contains no assemblies' -Because 'a present but empty directory yields an empty subject set, which reports nothing and passes' + It 'also rejects a subject set that was collected but is empty' { + $text | Should -Match 'staged no assemblies' -Because 'an empty subject set attributes nothing and passes green having measured nothing' } It 'fails rather than warns, because the check cannot do its job without it' { @@ -52,13 +52,54 @@ Describe 'ci-versioning action.yml' { $guardIdx | Should -BeLessThan $runnerIdx } - # Substring rather than regex: the path contains backslashes and braces, and a guard - # that checked a different directory from the one the runner reads would pass a - # loosely-written pattern while protecting nothing. - It 'checks the same directory the runner is pointed at' { - $subjectPath = '"${{ github.workspace }}\Build"' - $text.Contains('$subjectDir = ' + $subjectPath) | Should -BeTrue -Because 'the guard must read the path the runner is given' - $text.Contains('--subject-assemblies ' + $subjectPath) | Should -BeTrue -Because 'if the runner argument changes, this guard stops protecting it' + # The guard and the runner must read the same artefact. If they diverge the guard + # passes on one thing while the runner attributes against another, which is the + # failure it exists to prevent, one level removed. + It 'checks the same artefact the runner is given' { + $artefact = 'subject-assemblies.txt' + $text.Contains("Set-Content -Path '$artefact'") | Should -BeTrue -Because 'the collection step writes it' + $text.Contains("`$listPath = '$artefact'") | Should -BeTrue -Because 'the guard reads it' + $text.Contains("--subject-assembly-list '$artefact'") | Should -BeTrue -Because 'the runner is given it' + } + } + + Context 'the subject-assembly bracket' { + + # The subject set is the difference between two snapshots of the shared assembly + # directory, so it is exactly whatever was staged between them. That makes the bracket + # an ordering assumption in a file where steps get inserted, and an inserted step that + # builds or copies would be silently attributed to the repository under test. + # + # This asserts the shape rather than today's list: it fails when anything is added + # inside the bracket, which is the point. If a step genuinely belongs there, this test + # is the place to say so deliberately. + It 'contains exactly the two steps that build this repository' { + $open = ($lines | Select-String -Pattern '- name: Snapshot staged assemblies' | Select-Object -First 1).LineNumber + $close = ($lines | Select-String -Pattern '- name: Collect subject assemblies' | Select-Object -First 1).LineNumber + + $open | Should -Not -BeNullOrEmpty + $close | Should -BeGreaterThan $open + + $inner = $lines[$open..($close - 2)] | Where-Object { $_ -match '^\s+- name: ' } + @($inner).Count | Should -Be 2 -Because "only the primary and alt-config builds may sit inside the bracket; found:`n$($inner -join "`n")" + ($inner -join ' ') | Should -Match 'Build primary repo' + ($inner -join ' ') | Should -Match 'Build alt configurations' + } + + # A nested action inside the bracket could wipe or repopulate the assembly directory, + # which would corrupt the difference without adding a step name anyone would question. + It 'contains no nested action call' { + $open = ($lines | Select-String -Pattern '- name: Snapshot staged assemblies' | Select-Object -First 1).LineNumber + $close = ($lines | Select-String -Pattern '- name: Collect subject assemblies' | Select-Object -First 1).LineNumber + + $uses = $lines[$open..($close - 2)] | Where-Object { $_ -match '^\s+uses:' } + @($uses).Count | Should -Be 0 -Because "a nested action inside the bracket can change the assembly directory: $($uses -join '; ')" + } + + It 'closes after the alt-config build, so alt configurations are included' { + $alt = ($lines | Select-String -Pattern '- name: Build alt configurations' | Select-Object -First 1).LineNumber + $close = ($lines | Select-String -Pattern '- name: Collect subject assemblies' | Select-Object -First 1).LineNumber + $close | Should -BeGreaterThan $alt -Because 'a Revit repository year-suffixed assemblies are its own code and belong in the subject set' } } } diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs index 455cf03..9b0be3d 100644 --- a/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs @@ -428,76 +428,86 @@ public void EmptyDescriptionOrEmptySubjectSet_IsNotAttributed() } [Fact] - public void NoSubjectDirSupplied_FallsBackToWholeClosure() + public void NoSubjectListSupplied_FallsBackToWholeClosure() { + Assert.Null(RunCommand.ReadSubjectAssemblyList(null)); + Assert.Null(RunCommand.ReadSubjectAssemblyList(" ")); Assert.Null(RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], null)); - Assert.Null(RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], " ")); } [Fact] - public void MissingSubjectDir_FallsBackToWholeClosure() + public void MissingSubjectList_FallsBackToWholeClosure() { - string missing = Path.Combine(Path.GetTempPath(), "versioning-runner-no-such-dir-" + Guid.NewGuid()); - Assert.Null(RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], missing)); + string missing = Path.Combine(Path.GetTempPath(), "versioning-runner-no-such-list-" + Guid.NewGuid()); + Assert.Null(RunCommand.ReadSubjectAssemblyList(missing)); } [Fact] - public void SubjectDirNamingALoadedAssembly_YieldsThatAssemblysNamespaces() + public void EmptySubjectList_IsNotTheSameAsNoList() { - var asm = typeof(object).Assembly; - string dir = Path.Combine(Path.GetTempPath(), "versioning-runner-subject-" + Guid.NewGuid()); - Directory.CreateDirectory(dir); + // Null means nobody said which assemblies are the subject's, so attribution widens + // to the whole closure. Empty means the caller looked and the build staged nothing, + // which the caller fails on before this point. Collapsing the two would turn a + // build failure into a silent whole-closure report. + string list = Path.Combine(Path.GetTempPath(), "versioning-runner-list-" + Guid.NewGuid() + ".txt"); + File.WriteAllText(list, ""); try { - // Only the file name is used; the content is never loaded, because the - // namespaces come off the already-loaded assembly of the same name. - File.WriteAllText(Path.Combine(dir, Path.GetFileName(asm.Location)), ""); - - var ns = RunCommand.BuildSubjectNamespaces([asm], dir); + var names = RunCommand.ReadSubjectAssemblyList(list); - Assert.NotNull(ns); - Assert.Contains("System.Collections.Generic", ns); + Assert.NotNull(names); + Assert.Empty(names!); } - finally { Directory.Delete(dir, recursive: true); } + finally { File.Delete(list); } } [Fact] - public void SubjectAssemblyInATfmSubdirectory_IsFound() + public void SubjectListNamingALoadedAssembly_YieldsThatAssemblysNamespaces() { - // SDK-style repos append the TFM to OutputPath, so the subject's assemblies sit - // in Build\\ rather than Build\. A top-level-only scan silently produced an - // empty subject set, and an empty set means the check cannot fail. var asm = typeof(object).Assembly; - string dir = Path.Combine(Path.GetTempPath(), "versioning-runner-subject-" + Guid.NewGuid()); - string nested = Path.Combine(dir, "netstandard2.0"); - Directory.CreateDirectory(nested); - try - { - File.WriteAllText(Path.Combine(nested, Path.GetFileName(asm.Location)), ""); + var names = RunCommand.ReadSubjectAssemblyListFrom([Path.GetFileName(asm.Location)]); - var ns = RunCommand.BuildSubjectNamespaces([asm], dir); + var ns = RunCommand.BuildSubjectNamespaces([asm], names); - Assert.NotNull(ns); - Assert.Contains("System.Collections.Generic", ns); - } - finally { Directory.Delete(dir, recursive: true); } + Assert.NotNull(ns); + Assert.Contains("System.Collections.Generic", ns); } [Fact] - public void SubjectDirNamingNothingLoaded_YieldsNoNamespacesRatherThanTheClosure() + public void SubjectListEntriesMayBeFullPaths() { - string dir = Path.Combine(Path.GetTempPath(), "versioning-runner-subject-" + Guid.NewGuid()); - Directory.CreateDirectory(dir); - try - { - File.WriteAllText(Path.Combine(dir, "Nothing_Built_Here.dll"), ""); + // The caller writes names, but a full path is the obvious thing for someone to + // write later, and silently matching nothing would empty the subject set. + var asm = typeof(object).Assembly; + var names = RunCommand.ReadSubjectAssemblyListFrom([asm.Location]); - var ns = RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], dir); + var ns = RunCommand.BuildSubjectNamespaces([asm], names); - Assert.NotNull(ns); - Assert.Empty(ns); - } - finally { Directory.Delete(dir, recursive: true); } + Assert.NotNull(ns); + Assert.Contains("System.Collections.Generic", ns); + } + + [Fact] + public void SubjectListIsCaseInsensitive() + { + var asm = typeof(object).Assembly; + var names = RunCommand.ReadSubjectAssemblyListFrom([Path.GetFileName(asm.Location).ToUpperInvariant()]); + + var ns = RunCommand.BuildSubjectNamespaces([asm], names); + + Assert.NotNull(ns); + Assert.NotEmpty(ns!); + } + + [Fact] + public void SubjectListNamingNothingLoaded_YieldsNoNamespacesRatherThanTheClosure() + { + var names = RunCommand.ReadSubjectAssemblyListFrom(["Nothing_Built_Here.dll"]); + + var ns = RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], names); + + Assert.NotNull(ns); + Assert.Empty(ns); } // Strings below are copied verbatim from a real run's EventMessages. diff --git a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs index eb78b20..0e97d38 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs @@ -14,7 +14,7 @@ public static int Execute( string assembliesPath, string? outputPath, bool testAll, - string? subjectBuildDir = null, + string? subjectAssemblyList = null, string? configuration = null, IReadOnlyCollection? versionConditionalMethods = null) { @@ -63,25 +63,25 @@ public static int Execute( // assembly correctly. Null when no subject build dir was supplied: with whole-closure // attribution there is no basis for calling any assembly foreign, so the // reclassification is disabled and the fail-safe default stands. + HashSet? subjectFileNames = ReadSubjectAssemblyList(subjectAssemblyList); + ClosureContext? closure = null; - if (subjectBuildDir is not null && Directory.Exists(subjectBuildDir)) + if (subjectFileNames is { Count: > 0 }) { var loadedNames = new HashSet( loaded.Select(a => { try { return a.GetName().Name; } catch { return null; } }) .Where(n => !string.IsNullOrEmpty(n))!, StringComparer.Ordinal); var subjectBases = new HashSet( - Directory.GetFiles(subjectBuildDir, "*.dll", SearchOption.AllDirectories) - .Select(f => StripConfigSuffix(Path.GetFileNameWithoutExtension(f))), + subjectFileNames.Select(f => StripConfigSuffix(Path.GetFileNameWithoutExtension(f))), StringComparer.Ordinal); - if (subjectBases.Count > 0) - closure = new ClosureContext( - loadedNames, - new HashSet(loadedNames.Select(StripConfigSuffix), StringComparer.Ordinal), - subjectBases); + closure = new ClosureContext( + loadedNames, + new HashSet(loadedNames.Select(StripConfigSuffix), StringComparer.Ordinal), + subjectBases); } - var subjectNamespaces = BuildSubjectNamespaces(loaded, subjectBuildDir); + var subjectNamespaces = BuildSubjectNamespaces(loaded, subjectFileNames); Func isAttributable; if (subjectNamespaces is null) { @@ -586,29 +586,56 @@ public static HashSet BuildNamespacePrefixes(IEnumerable assem // whole-closure behaviour). Names come off the build output but are resolved against // the loaded set, which is authoritative, so a stale Build\ entry cannot introduce a // namespace for an assembly that is not actually present. - internal static HashSet? BuildSubjectNamespaces(List loaded, string? subjectBuildDir) + + // The subject set is the assemblies this repository's own build staged, handed over as a + // list the caller computed rather than a directory this reads. + // + // It used to be a directory, `\Build`, which the caller assumed every project + // wrote to. Nothing guaranteed that: the convention was enforced by a linter that only + // rewrote OutputPath values already present and never inspected which configuration they + // applied to, and no check read the directory until this one. Measured across 138 projects, + // 30% did not write there under the configuration CI builds, and a different 35% would not + // under the other one. So there was no configuration that made it reliable. + // + // A list has a property the directory did not: it describes what happened rather than what + // was declared. It also cannot be partially right. A directory holding some of the + // repository's assemblies produced a subject set that looked plausible and silently covered + // a fraction of the repo, with nothing to indicate it. + // + // Null means no list was supplied and attribution falls back to the whole closure, which + // over-reports. Empty means the list was supplied and the build staged nothing, which is a + // different state and is warned about by the caller before this runs. + // Split from the file read so the name handling can be tested without a temp file. + internal static HashSet ReadSubjectAssemblyListFrom(IEnumerable entries) => + new(entries.Select(l => l.Trim()) + .Where(l => l.Length > 0) + .Select(Path.GetFileName) + .Where(n => !string.IsNullOrEmpty(n))!, + StringComparer.OrdinalIgnoreCase); + + internal static HashSet? ReadSubjectAssemblyList(string? listPath) { - if (string.IsNullOrWhiteSpace(subjectBuildDir)) + if (string.IsNullOrWhiteSpace(listPath)) return null; - if (!Directory.Exists(subjectBuildDir)) + if (!File.Exists(listPath)) { Console.Error.WriteLine( - $"::warning title=Versioning::Subject build directory '{subjectBuildDir}' not found. " + + $"::warning title=Versioning::Subject assembly list '{listPath}' not found. " + "Falling back to attributing failures across the whole dependency closure, " + "which over-reports: failures owned by dependencies will be attributed to this repo."); return null; } - // Recursive because the layout under Build\ varies by project style, measured: - // SDK-style repos append the TFM (File_Toolkit -> Build\netstandard2.0\File_oM.dll) - // while the old-style Revit projects are flat (Build\Revit_X_oM_2022.dll). A - // top-level scan finds nothing for the former, which silently empties the subject - // set. Safe to recurse: BHoM references carry False, so a - // dependency's DLL is never copied into the subject's output. - var subjectFiles = new HashSet( - Directory.GetFiles(subjectBuildDir, "*.dll", SearchOption.AllDirectories).Select(Path.GetFileName)!, - StringComparer.OrdinalIgnoreCase); + // Names, not paths. Callers may write either; the comparison downstream is by file + // name, because that is what identifies an assembly in the staged set. + return ReadSubjectAssemblyListFrom(File.ReadAllLines(listPath)); + } + + internal static HashSet? BuildSubjectNamespaces(List loaded, HashSet? subjectFiles) + { + if (subjectFiles is null) + return null; var namespaces = new HashSet(StringComparer.Ordinal); int unreadable = 0; @@ -672,7 +699,7 @@ public static HashSet BuildNamespacePrefixes(IEnumerable assem s_subjectTypeCount = subjectTypeCount; Console.WriteLine( - $"Subject assemblies: {subjectFiles.Count} name(s) in {subjectBuildDir}, " + + $"Subject assemblies: {subjectFiles.Count} name(s) staged by the subject build, " + $"{subjectFiles.Count(f => loaded.Any(a => PathName(a) == f))} present in the loaded set"); return namespaces; diff --git a/tools/VersioningRunner/src/VersioningRunner/Program.cs b/tools/VersioningRunner/src/VersioningRunner/Program.cs index f9380df..244f28f 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Program.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Program.cs @@ -29,7 +29,7 @@ if (args.Length == 0 || args.Contains("--help") || args.Contains("-h")) { - Console.WriteLine("Usage: VersioningRunner [--assemblies ] [--output ] [--test-all] [--subject-assemblies ] [--configuration ] [--version-conditional ]"); + Console.WriteLine("Usage: VersioningRunner [--assemblies ] [--output ] [--test-all] [--subject-assembly-list ] [--configuration ] [--version-conditional ]"); Console.WriteLine(); Console.WriteLine("Options:"); Console.WriteLine(" --assemblies BHoM assemblies folder (default: C:\\ProgramData\\BHoM\\Assemblies)"); @@ -40,15 +40,16 @@ Console.WriteLine(" conditional in the subject's source, one per line. A present file"); Console.WriteLine(" means the scan ran, so unlisted methods record No. Omit the flag"); Console.WriteLine(" only when no scan was performed: findings then record Unknown."); - Console.WriteLine(" --subject-assemblies Subject repo's build output. Failures are attributed only to"); - Console.WriteLine(" namespaces these assemblies declare (default: the whole closure)"); + Console.WriteLine(" --subject-assembly-list File listing the assemblies this repo's own build staged,"); + Console.WriteLine(" one name per line. Failures are attributed only to namespaces"); + Console.WriteLine(" those assemblies declare (default: the whole closure)."); return 1; } string assemblies = GetArg(args, "--assemblies") ?? @"C:\ProgramData\BHoM\Assemblies"; string? output = GetArg(args, "--output"); bool testAll = args.Contains("--test-all"); -string? subject = GetArg(args, "--subject-assemblies"); +string? subject = GetArg(args, "--subject-assembly-list"); string? configuration = GetArg(args, "--configuration"); string? vcFile = GetArg(args, "--version-conditional"); From b5c052cd1d1ffd33892869715bcdae9404119d0f Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Tue, 25 Aug 2026 15:25:49 +0100 Subject: [PATCH 2/2] fix(ci-versioning): attribute failures by declaring assembly, not namespace prefix Subject attribution decided whether a failure belonged to the repository under test by matching the description string against the repository's namespaces, accepting anything at or below one of them. A repository that declares a namespace other repositories extend was therefore attributed all of their failures. BHoM_Adapter declares BH.Adapter, which every adapter in the fleet extends, so it was reported 47 findings spread across 28 other repositories' adapter namespaces and none of its own. The description cannot answer the question. It arrives either as a type full name or as "DeclaringType.MethodName" and nothing in the string says which, so BH.Adapter.ETABS.ETABSAdapter and BH.Adapter.BHoMAdapter.Push have the same shape: strip one segment and neither parent is a subject namespace, strip two and both land on BH.Adapter. Exact matching cannot separate them either. The dataset record names the declaring assembly, and the subject set is a list of assemblies, so that comparison is unambiguous. It is also available exactly when the namespace is not: these findings are classified DeclaringTypeNotLoaded, meaning the declaring assembly is outside the closure, so its namespaces appear in no loaded set. The assembly name comes from the record rather than from loading. This is a layering fix. Attribution answers "is this ours", classification answers "is it real". The existing reclassification already performs this comparison but is gated behind a non-empty candidate list, because reclassifying a finding nothing answered for would turn a genuine removal into a silent pass. That gate is correct and is unchanged; a failure whose declaring assembly is another repository's was never ours to classify in the first place. - read the declaring assembly before the attribution decision rather than four lines after it, from event messages already in hand - attribute on the config-stripped assembly base name, reusing the closure's own normalisation so Revit year variants of one assembly still match - fall back to the previous namespace test only when no assembly was recorded, rather than dropping the finding, since a silent drop would lose a real regression - count and report that fallback, in the run output and the job summary, with a warning when it is non-zero: it retains the over-attribution defect, so its size has to be measured rather than assumed Twelve tests, including that an assembly saying "not ours" is honoured rather than falling through to the namespace, which is what would reinstate the defect. --- .github/actions/ci-versioning/action.yml | 14 +- .../ClosureReclassificationTests.cs | 2 +- .../VersioningRunner.Tests/RunCommandTests.cs | 31 ++- .../SubjectAttributionDecisionTests.cs | 222 ++++++++++++++++++ .../VersioningRunner/Commands/RunCommand.cs | 141 +++++++++-- .../Models/VersioningResult.cs | 26 +- 6 files changed, 396 insertions(+), 40 deletions(-) create mode 100644 tools/VersioningRunner/src/VersioningRunner.Tests/SubjectAttributionDecisionTests.cs diff --git a/.github/actions/ci-versioning/action.yml b/.github/actions/ci-versioning/action.yml index 11f7113..d632d7c 100644 --- a/.github/actions/ci-versioning/action.yml +++ b/.github/actions/ci-versioning/action.yml @@ -770,10 +770,19 @@ runs: $attribution = '(not reported)' $classification = '(not reported)' + # Which evidence attributed each finding. Surfaced here and not only in the log because + # the number that matters is the namespace-fallback count: that path cannot tell this + # repository's types from those of repositories extending its namespace, so a non-zero + # value means some findings may not be this repository's. The runner also emits a + # ::warning, but stderr is deliberately not teed into the file this summary reads + # (see the tee step), so without this line the summary would not carry it at all. + $attributionBasis = '(not reported)' if (Test-Path 'versioning-stdout.txt') { $out = Get-Content 'versioning-stdout.txt' -Raw - if ($out -match 'Attribution:\s*(.+)') { $attribution = $Matches[1].Trim() } - if ($out -match 'Classification:\s*(.+)') { $classification = $Matches[1].Trim() } + # 'Attribution basis:' does not contain 'Attribution:', so these cannot cross-match. + if ($out -match 'Attribution:\s*(.+)') { $attribution = $Matches[1].Trim() } + if ($out -match 'Classification:\s*(.+)') { $classification = $Matches[1].Trim() } + if ($out -match 'Attribution basis:\s*(.+)') { $attributionBasis = $Matches[1].Trim() } } $rows = @() @@ -812,6 +821,7 @@ runs: $md += "| | |" $md += "|---|---|" $md += "| Classification | ``$classification`` |" + $md += "| Attribution basis | ``$attributionBasis`` |" $md += "| Reported unverified | $unverified |" if ($coverage) { $md += "| Surface examined | $($coverage.SubjectTypes) subject types across $($coverage.SubjectAssemblies) subject assemblies |" diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs index cf7919b..791a1e0 100644 --- a/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs @@ -62,7 +62,7 @@ private static (VersioningResult Result, FailureDiagnostic Diag) Run( { var diagnostics = new List(); var result = RunCommand.ExtractFilteredResult( - tree, _ => true, new List(), + tree, (_, _) => (true, AttributionBasis.NotRecorded), new List(), (t, m, a) => probe(t, m, a), diagnostics, closure); return (result, Assert.Single(diagnostics)); } diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs index 9b0be3d..4621412 100644 --- a/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs @@ -365,6 +365,11 @@ public void NonBHoMFiles_ReturnFalse(string path) } } + // IsFromSubjectNamespace is no longer the attribution mechanism. It is the fallback used + // only when a failure records no declaring assembly, because the description alone cannot + // say whether its last segment is a type or a method and so cannot separate a repository's + // own types from those of repositories extending its namespace. These cases still hold for + // what the function does; SubjectAttributionDecisionTests covers which one gets consulted. public class SubjectAttributionTests { private static HashSet Namespaces(params string[] ns) => @@ -564,7 +569,7 @@ public void UnresolvableLeaf_IsCollectedSeparatelyAndNotFailed() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; var skips = new List(); - var result = RunCommand.ExtractFilteredResult(outer, _ => true, skips); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), skips); Assert.Equal(VersioningStatus.Pass, result.Status); Assert.Equal(0, result.FailureCount); @@ -589,7 +594,7 @@ public void LeafNamingABHoMTypeFailure_StillFails() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; var skips = new List(); - var result = RunCommand.ExtractFilteredResult(outer, _ => true, skips); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), skips); Assert.Equal(VersioningStatus.Error, result.Status); Assert.Equal(1, result.FailureCount); @@ -647,7 +652,7 @@ public void NoTypeEventsButAnUnresolvableSignature_IsUnverifiedNotFailed() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; var skips = new List(); - var result = RunCommand.ExtractFilteredResult(outer, _ => true, skips, + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), skips, probeSignature: (_, _, _) => ("Autodesk.Revit.DB.LogicalOrFilter", ClassificationPath.SignatureBlockerOutsideBHoM, Array.Empty())); Assert.Equal(0, result.FailureCount); @@ -675,7 +680,7 @@ public void MangledDescriptionOnARealFailure_IsRelabelledFromTheMethodEvent() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; // Probe finds no blocker, so this stays a real failure — but actionable. - var result = RunCommand.ExtractFilteredResult(outer, _ => true, null, probeSignature: (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty())); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), null, probeSignature: (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty())); Assert.Equal(1, result.FailureCount); Assert.Equal("BH.Revit.Engine.MechanicalPlumbing.Create.SomethingGenuine", @@ -697,7 +702,7 @@ public void UnmangledDescriptionAlongsideAMethodEvent_IsStillLeftAlone() var versionSummary = new FakeTestResult { Status = "Error", Information = [leaf] }; var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; - var result = RunCommand.ExtractFilteredResult(outer, _ => true, null, probeSignature: (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty())); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), null, probeSignature: (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty())); Assert.Equal(1, result.FailureCount); Assert.Equal("BH.oM.Adapters.File.FileSettings", result.Failures[0].Description); @@ -716,7 +721,7 @@ public void UnmangledDescription_IsLeftAlone() var versionSummary = new FakeTestResult { Status = "Error", Information = [leaf] }; var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; - var result = RunCommand.ExtractFilteredResult(outer, _ => true); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded)); Assert.Equal(1, result.FailureCount); Assert.Equal("BH.oM.Adapters.File.FileSettings", result.Failures[0].Description); @@ -735,11 +740,11 @@ public void ExtractFilteredResult_HonoursTheSuppliedPredicate() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; var subject = Namespaces("BH.oM.Adapters.File"); - var filtered = RunCommand.ExtractFilteredResult(outer, d => RunCommand.IsFromSubjectNamespace(d, subject)); + var filtered = RunCommand.ExtractFilteredResult(outer, (d, _) => (RunCommand.IsFromSubjectNamespace(d, subject), AttributionBasis.NamespaceFallback)); Assert.Equal(VersioningStatus.Pass, filtered.Status); Assert.Equal(0, filtered.FailureCount); - var kept = RunCommand.ExtractFilteredResult(outer, _ => true); + var kept = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded)); Assert.Equal(VersioningStatus.Error, kept.Status); Assert.Equal(1, kept.FailureCount); } @@ -773,7 +778,7 @@ public void CauseFromEvents_IsRecordedAsUnresolvableFromEvents() { var diagnostics = new List(); RunCommand.ExtractFilteredResult(Tree("BH.oM.Adapters.File.FileSettings", RevitCause), - _ => true, null, null, diagnostics); + (_, _) => (true, AttributionBasis.NotRecorded), null, null, diagnostics); var only = Assert.Single(diagnostics); Assert.False(only.CountedAsReal); @@ -788,7 +793,7 @@ public void NoMethodEvent_IsRecordedAsSuch_AndCountedReal() // real by default rather than by evidence. var diagnostics = new List(); var result = RunCommand.ExtractFilteredResult(Tree("BH.oM.Adapters.File.FileSettings"), - _ => true, null, (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty()), diagnostics); + (_, _) => (true, AttributionBasis.NotRecorded), null, (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty()), diagnostics); Assert.Equal(1, result.FailureCount); var only = Assert.Single(diagnostics); @@ -802,7 +807,7 @@ public void ProbeVerdict_AndDeclaringAssembly_AreCarriedIntoTheDiagnostic() { var diagnostics = new List(); RunCommand.ExtractFilteredResult(Tree("BH.Revit.Engine.MechanicalPlumbing.Compute. }", MethodCause), - _ => true, null, (_, _, _) => (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()), diagnostics); + (_, _) => (true, AttributionBasis.NotRecorded), null, (_, _, _) => (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()), diagnostics); var only = Assert.Single(diagnostics); Assert.True(only.CountedAsReal); @@ -819,7 +824,7 @@ public void DeclaringAssemblyFromTheEvent_IsHandedToTheProbe() string? seen = "not called"; RunCommand.ExtractFilteredResult( Tree("BH.Revit.Engine.MechanicalPlumbing.Compute. }", MethodCause), - _ => true, null, + (_, _) => (true, AttributionBasis.NotRecorded), null, (_, _, asm) => { seen = asm; return (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()); }, null); @@ -844,7 +849,7 @@ public void EveryAttributedFailure_GetsExactlyOneDiagnostic() var versionSummary = new FakeTestResult { Status = "Error", Information = [leafReal, leafUnverified] }; var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; - var result = RunCommand.ExtractFilteredResult(outer, _ => true, skips, null, diagnostics); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), skips, null, diagnostics); Assert.Equal(1, result.FailureCount); Assert.Single(skips); diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/SubjectAttributionDecisionTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/SubjectAttributionDecisionTests.cs new file mode 100644 index 0000000..8e6ecb4 --- /dev/null +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/SubjectAttributionDecisionTests.cs @@ -0,0 +1,222 @@ +using VersioningRunner.Commands; +using VersioningRunner.Models; +using VersioningRunner.Tests.Fixtures; +using Xunit; + +namespace VersioningRunner.Tests +{ + // Attribution decides whether a failure is the subject repository's at all. It used to be + // decided on the description string, which cannot answer the question: a repository that + // declares a namespace other repositories extend was attributed all of their failures. + // Measured on a real run of a repository declaring BH.Adapter: 47 findings reported, spread + // across 28 other repositories' adapter namespaces, none of them its own. + // + // The cases below fix the shape of the fix, not just its outcome. In particular they pin + // that a declaring assembly which says "not yours" is honoured rather than falling through + // to the namespace, because falling through is what would quietly reinstate the defect. + public class SubjectAttributionDecisionTests + { + // The event shape the dataset actually produces. The "Name" field is assembly-qualified + // and is the only place the declaring assembly appears. + private static string MethodEvent(string type, string assembly) => + "Method Push from { \"_t\" : \"System.Type\", \"Name\" : \"" + type + ", " + assembly + + ", Version=9.0.0.0, Culture=neutral, PublicKeyToken=null\", \"_bhomVersion\" : \"9.2\" } failed to deserialise."; + + private static FakeTestResult Tree(string description, params string[] events) + { + var leaf = new FakeTestInfo + { + Status = "Error", + Description = description, + Message = "Error: Returned null from json.", + Information = events.Select(m => (object)new FakeEventMessage { Message = m }).ToList() + }; + return new FakeTestResult + { + Status = "Error", + Information = [new FakeTestResult { Status = "Error", Information = [leaf] }] + }; + } + + private static ClosureContext Closure(params string[] subject) => + new(new HashSet(StringComparer.Ordinal), + new HashSet(StringComparer.Ordinal), + new HashSet(subject.Select(RunCommand.StripConfigSuffix), StringComparer.Ordinal)); + + // BHoM_Adapter's real subject set and namespaces, from validation run 32837506868. + private static readonly HashSet AdapterNamespaces = + new(["BH.Adapter", "BH.Engine.Adapter", "BH.oM.Adapter"], StringComparer.Ordinal); + + private static ClosureContext AdapterClosure() => + Closure("Adapter_Engine", "Adapter_oM", "BHoM_Adapter", "Structure_AdapterModules"); + + public class TheDecision + { + // The defect, stated as a test. Namespace matching accepts this because the subject + // declares BH.Adapter and the type sits below it; the declaring assembly says it + // belongs to another repository, and that is the answer that must win. + [Fact] + public void ForeignAssemblyUnderTheSubjectsOwnNamespaceRoot_IsNotAttributed() + { + var (attributable, basis) = RunCommand.AttributeToSubject( + "BH.Adapter.ETABS.ETABSAdapter..ctor", "ETABS_Adapter", + AdapterNamespaces, AdapterClosure()); + + Assert.False(attributable); + Assert.Equal(AttributionBasis.DeclaringAssembly, basis); + } + + // The same string, with no assembly recorded, still matches by namespace. This is + // the residue the runner counts and warns about: it is wrong, and it is retained + // because dropping it would lose genuine regressions that carry no Method event. + [Fact] + public void TheSameStringWithNoAssembly_FallsBackToNamespaceAndIsStillWrong() + { + var (attributable, basis) = RunCommand.AttributeToSubject( + "BH.Adapter.ETABS.ETABSAdapter..ctor", null, + AdapterNamespaces, AdapterClosure()); + + Assert.True(attributable); + Assert.Equal(AttributionBasis.NamespaceFallback, basis); + } + + [Fact] + public void SubjectsOwnAssembly_IsAttributed() + { + var (attributable, basis) = RunCommand.AttributeToSubject( + "BH.Adapter.BHoMAdapter.Push", "BHoM_Adapter", + AdapterNamespaces, AdapterClosure()); + + Assert.True(attributable); + Assert.Equal(AttributionBasis.DeclaringAssembly, basis); + } + + // Attribution must not depend on the namespace agreeing. A subject assembly may + // declare types anywhere, and the assembly is the authority. + [Fact] + public void SubjectsOwnAssemblyWithAnUnrelatedNamespace_IsStillAttributed() + { + var (attributable, _) = RunCommand.AttributeToSubject( + "BH.Something.Entirely.Else.Thing", "Structure_AdapterModules", + AdapterNamespaces, AdapterClosure()); + + Assert.True(attributable); + } + + // Revit repositories build one assembly per year. The subject staged _2022; a + // record naming _2024 is the same assembly to this repository, which is why the + // comparison strips the configuration suffix. + [Fact] + public void ConfigurationSuffixedVariantOfASubjectAssembly_IsAttributed() + { + var (attributable, _) = RunCommand.AttributeToSubject( + "BH.Revit.Engine.Core.Create.ProjectParameter", "Revit_Core_Engine_2024", + new HashSet(["BH.Revit.Engine.Core"], StringComparer.Ordinal), + Closure("Revit_Core_Engine_2022")); + + Assert.True(attributable); + } + + // No subject set was established, so nothing can be called this repository's. + // Returning true here would attribute the whole closure on no basis. + [Fact] + public void NoClosure_AttributesNothingByAssembly() + { + var (attributable, basis) = RunCommand.AttributeToSubject( + "BH.Adapter.BHoMAdapter.Push", "BHoM_Adapter", AdapterNamespaces, closure: null); + + Assert.False(attributable); + Assert.Equal(AttributionBasis.DeclaringAssembly, basis); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void BlankAssemblyName_IsNotTreatedAsASubjectMatch(string assembly) + => Assert.False(RunCommand.IsFromSubjectAssembly(assembly, AdapterClosure())); + } + + public class ThroughTheCollector + { + private static (VersioningResult Result, List Diags) Run( + FakeTestResult tree, HashSet namespaces, ClosureContext closure) + { + var diagnostics = new List(); + var result = RunCommand.ExtractFilteredResult( + tree, + (d, a) => RunCommand.AttributeToSubject(d, a, namespaces, closure), + new List(), + // Nothing answered for the type: this is the DeclaringTypeNotLoaded shape + // all 47 observed findings had, and the shape whose existing reclassification + // gate deliberately refuses to act. Attribution must therefore stop it, and + // this asserts that it does rather than relying on the classifier. + (_, _, _) => (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()), + diagnostics, closure); + return (result, diagnostics); + } + + [Fact] + public void ForeignFinding_IsDroppedAtAttributionRatherThanClassified() + { + var (result, diags) = Run( + Tree("BH.Adapter.ETABS.ETABSAdapter..ctor", + MethodEvent("BH.Adapter.ETABS.ETABSAdapter", "ETABS_Adapter")), + AdapterNamespaces, AdapterClosure()); + + Assert.Equal(0, result.FailureCount); + // No diagnostic at all: it was never the subject's failure, so there is nothing + // to classify. A diagnostic here would mean attribution had let it through. + Assert.Empty(diags); + } + + [Fact] + public void SubjectFinding_SurvivesAndRecordsTheAssemblyAsItsBasis() + { + var (result, diags) = Run( + Tree("BH.Adapter.BHoMAdapter.Push", + MethodEvent("BH.Adapter.BHoMAdapter", "BHoM_Adapter")), + AdapterNamespaces, AdapterClosure()); + + Assert.Equal(1, result.FailureCount); + Assert.Equal(AttributionBasis.DeclaringAssembly, Assert.Single(diags).AttributedBy); + } + + // The counter the run output reports has to be able to see this path, so the basis + // is recorded on the row rather than inferred from the absence of an assembly. + [Fact] + public void FindingWithNoMethodEvent_IsRecordedAsTheFallbackBasis() + { + var (result, diags) = Run( + Tree("BH.Adapter.BHoMAdapter.Push"), + AdapterNamespaces, AdapterClosure()); + + Assert.Equal(1, result.FailureCount); + Assert.Equal(AttributionBasis.NamespaceFallback, Assert.Single(diags).AttributedBy); + } + + // The observed run in miniature: one of the subject's own failures among several + // other repositories' entries under the same namespace root. + [Fact] + public void MixedTree_KeepsOnlyTheSubjectsOwn() + { + var outer = new FakeTestResult + { + Status = "Error", + Information = + [ + Tree("BH.Adapter.ETABS.ETABSAdapter..ctor", MethodEvent("BH.Adapter.ETABS.ETABSAdapter", "ETABS_Adapter")), + Tree("BH.Adapter.Revit.RevitAdapter..ctor", MethodEvent("BH.Adapter.Revit.RevitAdapter", "Revit_Adapter")), + Tree("BH.Adapter.Mongo.MongoAdapter..ctor", MethodEvent("BH.Adapter.Mongo.MongoAdapter", "Mongo_Adapter")), + Tree("BH.Adapter.BHoMAdapter.Push", MethodEvent("BH.Adapter.BHoMAdapter", "BHoM_Adapter")) + ] + }; + + var (result, diags) = Run(outer, AdapterNamespaces, AdapterClosure()); + + Assert.Equal(1, result.FailureCount); + Assert.Equal("BH.Adapter.BHoMAdapter.Push", Assert.Single(result.Failures).Description); + Assert.Equal(AttributionBasis.DeclaringAssembly, Assert.Single(diags).AttributedBy); + } + } + } +} diff --git a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs index 0e97d38..4c5f733 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs @@ -82,11 +82,20 @@ public static int Execute( } var subjectNamespaces = BuildSubjectNamespaces(loaded, subjectFileNames); - Func isAttributable; + + // Attribution answers "is this failure ours"; classification answers "is it real". + // Those are separate questions and this is the place the first one is answered, which + // is why the declaring assembly is taken into account here rather than being used to + // repair a foreign finding further down. The reclassification at the bottom of + // CollectLeafFailures deliberately refuses to touch a finding nothing answered for, + // because doing so would turn a genuine removal into a silent pass; that guard is + // correct and stays. A failure whose declaring assembly is another repository's was + // never ours to classify in the first place. + Func isAttributable; if (subjectNamespaces is null) { Console.WriteLine($"Attribution: whole closure, {nsPrefixes.Count} namespace prefix(es)"); - isAttributable = d => IsFromLoadedNamespace(d, nsPrefixes); + isAttributable = (d, _) => (IsFromLoadedNamespace(d, nsPrefixes), AttributionBasis.NotRecorded); } else { @@ -108,7 +117,8 @@ public static int Execute( ? " — nothing to attribute to this repo, so no failure can be reported" : $": {string.Join(", ", subjectNamespaces.OrderBy(x => x, StringComparer.Ordinal).Take(20))}")); var subject = subjectNamespaces; - isAttributable = d => IsFromSubjectNamespace(d, subject); + var ctx = closure; + isAttributable = (d, asm) => AttributeToSubject(d, asm, subject, ctx); } var allFailures = new List(); @@ -173,6 +183,31 @@ public static int Execute( int noAssembly = diagnostics.Count(d => d.CountedAsReal && d.DeclaringAssembly is null); if (noAssembly > 0) Console.WriteLine($"Real failures with no declaring assembly recorded: {noAssembly}"); + + // How each attributed failure was decided to be ours. Reported unconditionally + // because the interesting number is the fallback count, and a number that is only + // printed when it is non-zero cannot be distinguished from one nobody measured. + int byAssembly = diagnostics.Count(d => d.AttributedBy == AttributionBasis.DeclaringAssembly); + int byNamespace = diagnostics.Count(d => d.AttributedBy == AttributionBasis.NamespaceFallback); + if (byAssembly + byNamespace > 0) + { + Console.WriteLine( + $"Attribution basis: {byAssembly} by declaring assembly, " + + $"{byNamespace} by namespace fallback (no declaring assembly recorded)"); + + // The fallback cannot tell this repository's namespace from a namespace it is + // merely the root of, so any finding on that path may be another repository's. + // Warned rather than logged: it is the residue of a known defect, and the + // point of counting it is that someone notices when it stops being zero. + if (byNamespace > 0) + { + Console.Error.WriteLine( + $"::warning title=Versioning::{byNamespace} failure(s) were attributed to this repository by " + + "namespace because the dataset record named no declaring assembly. That test cannot separate " + + "this repository's types from those of repositories extending its namespace, so these " + + "specific findings may not be its own."); + } + } } if (infrastructureSkips.Count > 0) @@ -390,11 +425,14 @@ private static IReadOnlyList FindAllFromJsonDatasetsMethods(IEnumera public static VersioningResult ExtractFilteredResult(object? rawResult, List loaded) { var nsPrefixes = BuildNamespacePrefixes(loaded); - return ExtractFilteredResult(rawResult, d => IsFromLoadedNamespace(d, nsPrefixes)); + return ExtractFilteredResult( + rawResult, (d, _) => (IsFromLoadedNamespace(d, nsPrefixes), AttributionBasis.NotRecorded)); } public static VersioningResult ExtractFilteredResult( - object? rawResult, Func isAttributable, List? unresolvableSkips = null, + object? rawResult, + Func isAttributable, + List? unresolvableSkips = null, Func Candidates)>? probeSignature = null, List? diagnostics = null, ClosureContext? closure = null) @@ -421,7 +459,8 @@ public static VersioningResult ExtractFilteredResult( } private static void CollectLeafFailures( - object node, Func isAttributable, List failures, + object node, Func isAttributable, + List failures, List? unresolvableSkips, Func Candidates)>? probeSignature, List? diagnostics, ClosureContext? closure, int depth) @@ -457,14 +496,29 @@ private static void CollectLeafFailures( string desc = nodeType.GetProperty("Description")?.GetValue(node)?.ToString() ?? ""; string msg = nodeType.GetProperty("Message")?.GetValue(node)?.ToString() ?? ""; - if (!isAttributable(desc)) - return; - // allChildren are all EventMessages here, since resultChildren is empty. var eventMessages = allChildren .Select(c => c.GetType().GetProperty("Message")?.GetValue(c)?.ToString() ?? "") .ToList(); + // The Method event's "Name" is assembly-qualified. It is the only handle on which + // assembly should declare the type, so it decides who gets asked when the type + // cannot be resolved, and it is also the only unambiguous answer to whether this + // failure is ours at all. + // + // Read before the attribution decision, not after. It used to be read after, so + // attribution was settled on the description alone while this sat four lines below + // unused: a repository declaring a namespace others extend was attributed all of + // their failures. The events are already in hand at this point, so reading them + // first costs nothing. + string? declaringAssembly = eventMessages + .Select(ParseMethodEventAssembly) + .FirstOrDefault(a => a is not null); + + var (attributable, attributedBy) = isAttributable(desc, declaringAssembly); + if (!attributable) + return; + // DescriptionFromJson mangles many method entries to ". }", // losing the method name. The Method event still carries both, so prefer it. var (eventType, eventMethod) = eventMessages @@ -474,13 +528,6 @@ private static void CollectLeafFailures( ? $"{eventType}.{eventMethod}" : desc; - // The Method event's "Name" is assembly-qualified. It is the only handle on which - // assembly should declare the type, so it decides who gets asked when the type - // cannot be resolved. - string? declaringAssembly = eventMessages - .Select(ParseMethodEventAssembly) - .FirstOrDefault(a => a is not null); - string? cause = ClassifyUnresolvableCause(eventMessages); var path = ClassificationPath.UnresolvableFromEvents; IReadOnlyList candidates = Array.Empty(); @@ -541,7 +588,8 @@ private static void CollectLeafFailures( // the ordinary case and carrying it would add noise to every row. DeclaringTypeCandidates: candidates.Count > 1 ? candidates : null, Configuration: s_configuration, - VersionConditional: ClassifyVersionConditional(eventType, eventMethod))); + VersionConditional: ClassifyVersionConditional(eventType, eventMethod), + AttributedBy: attributedBy)); } else { @@ -724,12 +772,59 @@ public static bool IsFromLoadedNamespace(string description, HashSet nsP return nsPrefixes.Contains(parts[0] + "." + parts[1] + "." + parts[2]); } - // Attribution against exact namespaces. A failure belongs to the subject when its - // declaring type sits in one of the subject's namespaces or below it. Matching on a - // segment boundary keeps BH.oM.Adapters.ETABS.Pier out when the subject only declares - // BH.oM.Adapters.File. Descriptions arrive either as a type full name or as - // "DeclaringType.MethodName" (Versioning_Toolkit's DescriptionFromJson), and both are - // covered: each is a strict prefix extension of the declaring namespace. + // The whole attribution decision for subject mode, in one place so that what the runner + // does and what the tests check cannot drift apart. + // + // The declaring assembly wins whenever there is one, including when it says no: an + // assembly that is not the subject's is a definite answer, and falling through to the + // namespace after it would reinstate the defect this exists to remove. The namespace is + // consulted only in the absence of an assembly, where it is the sole evidence available. + public static (bool Attributable, AttributionBasis Basis) AttributeToSubject( + string description, string? declaringAssembly, + HashSet subjectNamespaces, ClosureContext? closure) + { + if (declaringAssembly is not null) + return (IsFromSubjectAssembly(declaringAssembly, closure), AttributionBasis.DeclaringAssembly); + + // No assembly recorded, so the ambiguous string is all there is. Kept rather than + // dropped: a silent drop would lose a real regression. Counted by the caller so the + // size of this path is measured rather than assumed. + return (IsFromSubjectNamespace(description, subjectNamespaces), AttributionBasis.NamespaceFallback); + } + + // Attribution against the assembly the dataset record names. This is the primary test and + // the only unambiguous one: the subject either staged an assembly of that name or it did + // not, and no string parsing is involved. + // + // It works precisely where the namespace cannot. The failures this was written for are + // classified DeclaringTypeNotLoaded, meaning no loaded assembly yielded the type and the + // one the event named is outside the closure. Their namespaces are therefore absent from + // every loaded set, so no scheme that reasons about which namespace is the longest match + // can see them. The assembly name comes from the record, not from loading, so it is + // available exactly when the type is not. + // + // Compared on the config-stripped base name because Revit repositories build the same + // assembly per year (Revit_Core_Engine_2022 and _2024 are one assembly to the subject), + // reusing the closure's own normalisation so the two cannot disagree. + public static bool IsFromSubjectAssembly(string? declaringAssembly, ClosureContext? closure) + { + // No closure means no subject set was established, so nothing can be called ours. + if (string.IsNullOrWhiteSpace(declaringAssembly) || closure is null) + return false; + + return closure.SubjectBaseNames.Contains(StripConfigSuffix(declaringAssembly)); + } + + // Attribution against exact namespaces. Used only when no declaring assembly was + // recorded, because it cannot be made correct: a failure belongs to the subject when its + // declaring type sits in one of the subject's namespaces or below it, and "or below it" + // is exactly what over-attributes. Matching on a segment boundary keeps + // BH.oM.Adapters.ETABS.Pier out when the subject only declares BH.oM.Adapters.File, but + // nothing here can keep BH.Adapter.ETABS.ETABSAdapter out when the subject declares + // BH.Adapter, because the string does not say whether the last segment is a type or a + // method. Descriptions arrive either as a type full name or as "DeclaringType.MethodName" + // (Versioning_Toolkit's DescriptionFromJson), and both are strict prefix extensions of + // the declaring namespace, which is why this is a prefix test and why it is a fallback. public static bool IsFromSubjectNamespace(string description, HashSet subjectNamespaces) { if (string.IsNullOrWhiteSpace(description) || subjectNamespaces.Count == 0) diff --git a/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs b/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs index 107a7a9..df93f5b 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs @@ -48,6 +48,27 @@ public enum ClassificationPath ConfigurationNotBuilt, } +// Which evidence decided that a failure belongs to the subject repository. +// +// The two are not equally trustworthy and the difference is the whole point of recording it. +// A declaring assembly is unambiguous: the dataset record names it, and the subject either +// staged an assembly of that name or it did not. A description is ambiguous: it arrives +// either as a type full name or as "DeclaringType.MethodName" and nothing in the string says +// which, so BH.Adapter.ETABS.ETABSAdapter (another repository's type) and +// BH.Adapter.BHoMAdapter.Push (this repository's method) have the same shape. No matching +// rule over the string can separate them, which is why the assembly decides where one exists. +public enum AttributionBasis +{ + // Whole-closure attribution, where there is no subject set to compare against. + NotRecorded, + // The dataset record named a declaring assembly and the subject build staged it. + DeclaringAssembly, + // No declaring assembly was recorded, so the namespace decided it. This path still + // over-attributes a repository that owns a namespace others extend, and is counted in + // the run output for exactly that reason: its size must be measured, not assumed. + NamespaceFallback, +} + // What this run actually built, needed to tell "the recorded declaring // assembly is missing because it is someone else's" from "because we did not compile that // configuration" from "because it was genuinely removed". Passed explicitly rather than @@ -89,7 +110,10 @@ public record FailureDiagnostic( // Build configuration this run compiled, carried per row so a finding read on its // own is interpretable. Human-legible; it does not by itself explain a divergence. string? Configuration = null, - VersionConditionalState VersionConditional = VersionConditionalState.Unknown); + VersionConditionalState VersionConditional = VersionConditionalState.Unknown, + // Which evidence attributed this failure to the subject. Per row rather than only as a + // total, so a reader can tell whether any individual finding rests on the ambiguous path. + AttributionBasis AttributedBy = AttributionBasis.NotRecorded); // Coverage denominator. A verdict without one cannot be interpreted: a pass over zero // methods reads identically to a pass over seven thousand. BHoMBot reported object