feat(ci-versioning): derive the subject set from the build, and attribute by declaring assembly - #13
Merged
Conversation
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.
…espace 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes to how the versioning check decides which failures belong to the repository under test.
They are together because landing the first alone replaces one wrong answer with a smaller wrong one.
1. The subject set comes from what the build staged, not from a directory
The check attributes failures only to the repository's own assemblies, so it has to know which those
are. It read them from a
Build\directory at the workspace root, on the assumption that everyproject wrote there. Nothing guaranteed that, and it was false for a large minority of the fleet.
Measured by evaluating
OutputPathper project withdotnet msbuild -getProperty:OutputPathundereach configuration, resolving the result against the project's own directory, across 349 projects in
95 repositories:
Build\Release, which CI buildsDebugBuilding the other configuration does not fix this. It would fix 88 projects and break none, but 56
projects miss the directory under every configuration, so no configuration choice makes it reliable.
Evaluating rather than pattern-matching matters here: a declaration can name
Build\and still landelsewhere, because
OutputPathis relative to the project directory, not the repository root.The convention was never verified. It was maintained by a linter that rewrote
<OutputPath>valuesalready present, never added a missing one, and never inspected which configuration a value applied
to. Nothing read the directory until this check did.
The failure had two shapes:
closure. Measured at 1,056 of 1,056 findings on a real pull request, none belonging to the
repository under test.
warns, and the check silently covers 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 the subject build is both
what the repository produced and what the runner can see. Two snapshots either side of the build; 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 leaving it out is
correct rather than a gap. The set cannot be partially right in the way a directory could, which
closes the second shape by construction rather than by guarding.
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. It has its own tests.
Alt configurations sit inside the bracket deliberately. A Revit repository's year-suffixed
assemblies are its own code, so attributing failures in them to it is correct.
The precondition guard is reshaped, not removed. Same placement, same failure behaviour, now
asserting that the build staged at least one assembly.
--subject-assembliesbecomes--subject-assembly-listand takes a file. It had one caller. Adirectory of copies was rejected: it would duplicate every assembly and reintroduce the filesystem
convention this removes.
2. Attribution uses the declaring assembly, not the namespace prefix
With the subject set correct, the next thing in the way became visible. Attribution matched the
failure description against the repository's namespaces, accepting anything at or below one of them.
A repository declaring a namespace that other repositories extend was therefore attributed all of
their failures. A repository declaring
BH.Adapter, which every adapter in the fleet extends, wasreported 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.BH.Adapter.ETABS.ETABSAdapterandBH.Adapter.BHoMAdapter.Pushhave the same shape: strip one segment and neither parent is a subjectnamespace, strip two and both land on
BH.Adapter. Exact matching cannot separate them either, andpreferring the longest matching namespace cannot help, because it needs the other repository's
namespace to be visible and these assemblies are not loaded at all.
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 theassembly 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, so the decision belongs one layer
up. Concretely: the declaring assembly is now read before the attribution decision rather than four
lines after it, from event messages already in hand.
Where no declaring assembly is recorded, the previous namespace test still runs, because dropping the
finding would lose a real regression. That path keeps the defect, so it is counted and reported in
the run output and the job summary, with a warning when it is non-zero. A number that is only printed
when it is inconvenient cannot be told from one nobody measured.
Verification
Two end-to-end runs on a repository whose
Build\directory did not exist and which declaresBH.Adapter, with all six nested action references flipped so the run tested this code rather thandevelop's, and with every cache deleted first and the misses confirmed in the log.The subject set was the repository's four projects exactly. Coverage is identical across the last two
runs — 72 subject types, 4 assemblies, 75 loaded, one entry point — so the same surface was examined
and only attribution changed. The bracket was confirmed in a real job: exactly the two build steps
ran between the snapshots, and nothing else wrote to the assembly directory inside it.
Unit tests cover both directions, including that an assembly saying "not ours" is honoured rather
than falling through to the namespace, which is what would quietly reinstate the defect.
What is not verified
altConfigs.txt, so that pathrests on the structural test asserting the bracket closes after the alt-config build. A run on a
Revit repository would exercise it.
both runs, and the final run produced no findings to count, so its output path is covered by unit
tests only.
repository has none. Unit tests cover it; the run can only show that the foreign findings are gone.