fix(ci-versioning): assert the subject build output before attributing against it - #12
Merged
Merged
Conversation
…g against it The runner is handed --subject-assemblies "<workspace>\Build" and narrows attribution to the namespaces the assemblies there declare. When that directory is absent it falls back to attributing every failure across the whole dependency closure, so the check reports other repositories' defects against the repository under test. Measured on a real pull request: 1,056 of 1,056 findings belonged to other repos, and none matched the repo being checked. The runner already warned. It warns on stderr, and the calling step captures stdout only, so the warning reached the raw log and nothing else. A warning also does not stop the check producing a verdict it has no basis for. Asserted in the action rather than the runner because this is a property of the build, and the build is the action's job. Same shape and placement as the datasets guard immediately above it, which has always done exactly this. Two conditions, because they fail differently. An absent directory widens attribution to the whole closure and over-reports. A present but empty one yields an empty subject set, attributes nothing, and passes green having measured nothing. Usual cause when it fires: projects declaring <OutputPath>..\Build\ only inside PropertyGroups conditioned on Debug or Test, while the check builds Release, so output lands in the SDK default bin\Release\<tfm>\ instead. The error message says so, since that is not obvious from the failure. This is a broken precondition, not a vacuous pass. "Nothing to check" is a legitimate green; "the thing to check was never built" is not, and the distinction is why this does not depend on the wider question of what a vacuous state should do. Six structural tests, verified to fail without the guard.
The error text named projects declaring <OutputPath>..\Build\ only under Debug or Test conditions. That is two of the four repositories the guard newly fails; the other two declare no <OutputPath> at all and take the SDK default. Both shapes end up in bin\Release\<tfm>\ with Build\ absent, and they need different fixes, so the message now names both. Also records that neither shape is a fault in those repositories. The convention that output belongs in Build\ was enforced by a linter that only rewrote <OutputPath> lines already present, never added a missing one, and never inspected which configuration a line applied to. Nothing read the directory until this check did. The message someone acts on should not imply they broke a rule they were never held to.
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.
Problem
ci-versioninghands the runner--subject-assemblies "<workspace>\Build", and the runner narrowsattribution to the namespaces the assemblies in that directory declare. When the directory is absent,
it falls back to attributing every failure across the whole dependency closure — so the check reports
other repositories' defects against the repository under test.
That is not hypothetical. On run 31844962538
(
BHoM/BHoM_AdapterPR 416) it produced 1,056 findings, none of which belonged to the repository beingchecked, against a diff that changed one comment.
The runner already warned:
Two reasons that was not enough. The warning goes to stderr, and the calling step captures stdout
only, so it reaches the raw log and nothing downstream. And a warning does not stop the check going on to
produce a verdict it has no basis for.
Root cause
The affected repositories declare
<OutputPath>..\Build\only insidePropertyGroupelementsconditioned on
Debug|AnyCPUandTest|AnyCPU. There is noRelease|AnyCPUgroup, and the checkbuilds
Release, so output goes to the SDK defaultbin\Release\<tfm>\andBuild\is never created.The contrast confirms it: Revit projects declare
<OutputPath>unconditionally, so it applies toevery configuration, and those repositories attribute correctly.
What changed
One step, placed immediately after the datasets guard it copies, asserting two conditions that fail
differently:
passes green having measured nothing.
Both are
::errorand exit 1. The error text names the likely cause, because it is not deducible fromthe failure.
This is a precondition, not a policy change. "Nothing to check" is a legitimate green. "The thing to
check was never built" is a broken build assumption, and asserting it belongs in the action, which owns
the build, rather than in the runner. The datasets guard directly above has always worked exactly this
way.
Who this newly fails
Measured by resolving every
<OutputPath>condition in every non-.ciproject across the 19 publicrepositories carrying this check:
Build\BHoM_Adapter(0 of 4 projects),Mongo_Toolkit(0 of 3),Socket_Toolkit(0 of 2),SQLite_Toolkit(0 of 4)Revit_Toolkit,BHoM_UI,File_Toolkit,Excel_ToolkitBHoM_Adapterwas already failing, so three repositories move from green-on-a-wrong-basis to red with acause. No
ci-context is required on any repository, so none of this blocks a merge. The fix in eachcase is one
PropertyGroupin the affected projects.Verification
Six structural tests, verified to fail without the guard — removing the step drops them from 6 passing
to 2 passing, 4 failing. They assert the guard exists, covers both conditions, errors rather than warns,
runs before the runner rather than after, and reads the same directory the runner is given.
The logic was exercised directly against three fixtures: absent directory → exit 1 with the
missing-output error; present but empty → exit 1 with the empty error; populated with assemblies nested
under a TFM directory → exit 0 and a notice. The nested case matters, because SDK-style projects append
the target framework and a non-recursive scan would find nothing.
What is not verified
No end-to-end
ci-versioningrun exercises this. Reaching the guard in a real job means running thewhole chain, and validating a change to a composite action requires flipping every nested action
reference — this action names six helpers, two of which resolve their source relative to their own
checkout, so pinning the outer reference alone would test the wrong code. That is a full sandbox-copy
exercise, and for eight lines of self-contained PowerShell with no dependency on any helper it is not
proportionate.
What an end-to-end run would add over the above: confirmation that the step is reached at all in a real
job. Placement and ordering are covered by the structural tests; the logic is covered by the fixtures.
Not in scope
Two related findings are deliberately left alone. Partial output — where some projects reach
Build\and some do not — produces a real but incomplete subject set with no warning at all, and thisguard does not catch it. And the stderr capture gap that hid the original warning is a small change
with real hazards around stream interleaving and annotation loss, and deserves its own scoping.