From 2c35f442b470942c44290b57af4bac9a08855d58 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Wed, 2 Sep 2026 15:34:04 -0400 Subject: [PATCH] fix(ci): derive package-validation dependency ranges from Directory.Packages.props (#122) inspect-packed-nupkgs.sh's assert_dependency_range hardcoded each expected third-party dependency range as a duplicate literal, separate from the Directory.Packages.props PackageVersion that already states the same policy. Dependabot updates the latter but not the former, so a correct package bump (e.g. PR #121's TUnit.Core update) failed validation until someone noticed and hand-edited the script (5cfe446). assert_dependency_range now looks up the expected range from Directory.Packages.props itself, evaluated via `dotnet msbuild -getItem` (SDK-native, no new CI dependency) and matched by dependency ID via jq. The validator still independently compares that authoritative value against the actually-packed .nuspec - only the duplicated literal is gone, not the check's strength. A missing Directory.Packages.props entry now fails with a distinct diagnostic instead of masquerading as a packed-range mismatch. Adds inspect-packed-nupkgs.tests.sh, a dependency-free regression suite proving both the pass and fail cases, including the exact "bump only the props file" scenario from issue #122, wired into package-validation.yaml. Amends ADR-0031 to record the drift and the fix. Closes #122 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA --- .github/scripts/inspect-packed-nupkgs.sh | 99 +++++++++--- .../scripts/inspect-packed-nupkgs.tests.sh | 142 ++++++++++++++++++ .github/workflows/package-validation.yaml | 6 + ...c-preview-release-and-versioning-policy.md | 43 ++++++ 4 files changed, 270 insertions(+), 20 deletions(-) create mode 100755 .github/scripts/inspect-packed-nupkgs.tests.sh diff --git a/.github/scripts/inspect-packed-nupkgs.sh b/.github/scripts/inspect-packed-nupkgs.sh index 01c4571e..c4d1db08 100755 --- a/.github/scripts/inspect-packed-nupkgs.sh +++ b/.github/scripts/inspect-packed-nupkgs.sh @@ -10,15 +10,37 @@ # .nuspec that matches that package's own version (not merely "some bracketed # string"), and (per ADR-0031 Amendment 1) a deliberate tested range - not a # bare unbounded floor, not a blanket exact pin - on each integration -# package's third-party dependency. +# package's third-party dependency, read from Directory.Packages.props itself +# (issue #122) rather than a second hardcoded literal that Dependabot can't see. set -euo pipefail -pack_output="${1:?usage: inspect-packed-nupkgs.sh }" -work_dir=$(mktemp -d) -trap 'rm -rf "$work_dir"' EXIT - fail=0 +# Evaluates Directory.Packages.props' items via `dotnet msbuild +# -getItem` (SDK-native, already a CI/dev dependency - no new tool) and returns +# the JSON verbatim. This is the one authoritative read of repository policy; +# every assert_dependency_range call below looks up its expected range from +# this same JSON rather than a duplicated string literal, so a Dependabot bump +# to Directory.Packages.props is automatically the new expected value. +load_authoritative_versions() { + local props_file="$1" + dotnet msbuild "$props_file" -nologo -getItem:PackageVersion +} + +# Looks up a single dependency's authoritative version/range by its +# Directory.Packages.props identity, from the +# JSON produced by load_authoritative_versions. Returns empty (not an error) +# when the id isn't found - callers must check for that distinctly from a +# packed-range mismatch, since "policy lookup failed" and "packed output is +# wrong" are different classes of failure (issue #122, point 10). +get_authoritative_range() { + local authoritative_json="$1" + local dep_id="$2" + jq -r --arg id "$dep_id" \ + '.Items.PackageVersion[]? | select(.Identity == $id) | .Version' \ + <<<"$authoritative_json" | head -1 +} + extract() { local nupkg="$1" local dest="$2" @@ -124,18 +146,47 @@ assert_dependency_range() { local nuspec="$1" local pkg_name="$2" local dep_id="$3" - local expected_range="$4" + local authoritative_json="$4" + local expected_range + expected_range=$(get_authoritative_range "$authoritative_json" "$dep_id") + if [ -z "$expected_range" ]; then + # A distinct failure class from a packed-range mismatch below (issue #122, + # point 10): this means Directory.Packages.props itself has no + # , a validator/configuration problem, + # not evidence about what got packed. + echo "FAIL: could not determine authoritative PackageVersion for $dep_id in Directory.Packages.props" >&2 + fail=1 + return + fi local version version=$(grep -o "id=\"${dep_id}\" version=\"[^\"]*\"" "$nuspec" | head -1 | sed -E 's/.*version="([^"]*)".*/\1/') if [ "$version" = "$expected_range" ]; then - echo "OK: $pkg_name's .nuspec constrains $dep_id to the intended tested range $version" + echo "OK: $pkg_name's .nuspec constrains $dep_id to the intended tested range $version (matches Directory.Packages.props)" else - echo "FAIL: $pkg_name's .nuspec dependency on $dep_id is '$version', expected the intended tested range '$expected_range'" >&2 + echo "FAIL: $pkg_name's .nuspec dependency on $dep_id is '$version', expected the intended tested range '$expected_range' (from Directory.Packages.props)" >&2 fail=1 fi } -for pkg in Compono Compono.XunitV3 Compono.NSubstitute Compono.Bogus Compono.TUnit Compono.TestDoubles Compono.DependencyInjection Compono.Http Compono.MSTest; do +main() { + local pack_output="${1:?usage: inspect-packed-nupkgs.sh }" + local script_dir + script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) + local packages_props="$script_dir/../../Directory.Packages.props" + # Not `local`: the EXIT trap below still needs to see this after main() + # itself returns and the script falls off the end (bash pops local scope + # on function return, before the EXIT trap runs). + work_dir=$(mktemp -d) + trap 'rm -rf "$work_dir"' EXIT + + local authoritative_json + authoritative_json=$(load_authoritative_versions "$packages_props") || { + echo "FAIL: could not evaluate $packages_props via dotnet msbuild" >&2 + exit 1 + } + + local pkg nupkg extract_dir extra_paths nuspec + for pkg in Compono Compono.XunitV3 Compono.NSubstitute Compono.Bogus Compono.TUnit Compono.TestDoubles Compono.DependencyInjection Compono.Http Compono.MSTest; do nupkg=$(find "$pack_output" -maxdepth 1 -iname "${pkg}.[0-9]*.nupkg" | head -1) if [ -z "$nupkg" ]; then echo "FAIL: no .nupkg found for $pkg in $pack_output" >&2 @@ -173,22 +224,22 @@ for pkg in Compono Compono.XunitV3 Compono.NSubstitute Compono.Bogus Compono.TUn Compono.XunitV3) assert_manifest_field "$nuspec" "$pkg" "title" "Compono — xUnit v3 Integration" assert_exact_pin_dependency "$nuspec" "$pkg" "Compono" - assert_dependency_range "$nuspec" "$pkg" "xunit.v3.extensibility.core" "[3.2.2, 5.0.0)" + assert_dependency_range "$nuspec" "$pkg" "xunit.v3.extensibility.core" "$authoritative_json" ;; Compono.NSubstitute) assert_manifest_field "$nuspec" "$pkg" "title" "Compono — NSubstitute Integration" assert_exact_pin_dependency "$nuspec" "$pkg" "Compono" - assert_dependency_range "$nuspec" "$pkg" "NSubstitute" "[6.2.0, 7.0.0)" + assert_dependency_range "$nuspec" "$pkg" "NSubstitute" "$authoritative_json" ;; Compono.Bogus) assert_manifest_field "$nuspec" "$pkg" "title" "Compono — Bogus Integration" assert_exact_pin_dependency "$nuspec" "$pkg" "Compono" - assert_dependency_range "$nuspec" "$pkg" "Bogus" "[35.6.5, 36.0.0)" + assert_dependency_range "$nuspec" "$pkg" "Bogus" "$authoritative_json" ;; Compono.TUnit) assert_manifest_field "$nuspec" "$pkg" "title" "Compono — TUnit Integration" assert_exact_pin_dependency "$nuspec" "$pkg" "Compono" - assert_dependency_range "$nuspec" "$pkg" "TUnit.Core" "[1.65.63, 2.0.0)" + assert_dependency_range "$nuspec" "$pkg" "TUnit.Core" "$authoritative_json" ;; Compono.TestDoubles) assert_manifest_field "$nuspec" "$pkg" "title" "Compono — Generated Test Doubles" @@ -211,14 +262,22 @@ for pkg in Compono Compono.XunitV3 Compono.NSubstitute Compono.Bogus Compono.TUn Compono.MSTest) assert_manifest_field "$nuspec" "$pkg" "title" "Compono — MSTest Integration" assert_exact_pin_dependency "$nuspec" "$pkg" "Compono" - assert_dependency_range "$nuspec" "$pkg" "MSTest.TestFramework" "[4.0.0, 5.0.0)" + assert_dependency_range "$nuspec" "$pkg" "MSTest.TestFramework" "$authoritative_json" ;; esac -done + done -if [ "$fail" -ne 0 ]; then - echo "One or more package-contents assertions failed." >&2 - exit 1 -fi + if [ "$fail" -ne 0 ]; then + echo "One or more package-contents assertions failed." >&2 + exit 1 + fi -echo "All package-contents assertions passed." + echo "All package-contents assertions passed." +} + +# Sourced (by the regression-test script) vs. executed directly: only run main +# when this file is the actual entry point, so tests can source it to reach +# the functions above without triggering a real pack-output scan. +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + main "$@" +fi diff --git a/.github/scripts/inspect-packed-nupkgs.tests.sh b/.github/scripts/inspect-packed-nupkgs.tests.sh new file mode 100755 index 00000000..7c18c42d --- /dev/null +++ b/.github/scripts/inspect-packed-nupkgs.tests.sh @@ -0,0 +1,142 @@ +#!/usr/bin/env bash +# Regression coverage for inspect-packed-nupkgs.sh's authoritative-range lookup +# (issue #122): proves the validator derives its expected dependency range from +# Directory.Packages.props itself, not a hardcoded literal that Dependabot +# can't update. No test framework is set up for shell scripts in this repo, so +# this is a plain, dependency-free bash script - source the real script (its +# main() only runs when executed directly, not sourced) and exercise its +# functions directly against small fixture files. +set -euo pipefail + +script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=./inspect-packed-nupkgs.sh +source "$script_dir/inspect-packed-nupkgs.sh" + +work_dir=$(mktemp -d) +trap 'rm -rf "$work_dir"' EXIT + +tests_failed=0 + +make_props() { + local path="$1" + local dep_id="$2" + local range="$3" + cat >"$path" < + + + + +EOF +} + +make_nuspec() { + local path="$1" + local dep_id="$2" + local range="$3" + cat >"$path" < + + + + + + + + + +EOF +} + +expect_pass() { + local description="$1" + fail=0 + "${@:2}" + if [ "$fail" -eq 0 ]; then + echo "PASS: $description" + else + echo "TEST FAILURE: expected $description to pass, but it failed" >&2 + tests_failed=1 + fi +} + +expect_fail() { + local description="$1" + fail=0 + "${@:2}" >/dev/null 2>&1 || true + if [ "$fail" -ne 0 ]; then + echo "PASS: $description" + else + echo "TEST FAILURE: expected $description to fail, but it passed" >&2 + tests_failed=1 + fi +} + +# 1. Passing case: the packed nuspec matches Directory.Packages.props exactly. +props_current="$work_dir/props-current.props" +nuspec_matching="$work_dir/matching.nuspec" +make_props "$props_current" "TUnit.Core" "[1.65.63, 2.0.0)" +make_nuspec "$nuspec_matching" "TUnit.Core" "[1.65.63, 2.0.0)" +json_current=$(load_authoritative_versions "$props_current") +expect_pass "packed range matching current authoritative range" \ + assert_dependency_range "$nuspec_matching" "Compono.TUnit" "TUnit.Core" "$json_current" + +# 2. Failing case: the packed nuspec disagrees with the authoritative range. +nuspec_stale="$work_dir/stale.nuspec" +make_nuspec "$nuspec_stale" "TUnit.Core" "[1.65.38, 2.0.0)" +expect_fail "packed range disagreeing with authoritative range" \ + assert_dependency_range "$nuspec_stale" "Compono.TUnit" "TUnit.Core" "$json_current" + +# 3. The original bug class (issue #122): bump ONLY the authoritative props +# file (as Dependabot would) and re-pack with the SAME new range, with no +# validator literal touched anywhere - validation must still pass. +props_bumped="$work_dir/props-bumped.props" +nuspec_bumped="$work_dir/bumped.nuspec" +make_props "$props_bumped" "TUnit.Core" "[1.65.99, 2.0.0)" +make_nuspec "$nuspec_bumped" "TUnit.Core" "[1.65.99, 2.0.0)" +json_bumped=$(load_authoritative_versions "$props_bumped") +expect_pass "Dependabot-style bump: nuspec and Directory.Packages.props move together" \ + assert_dependency_range "$nuspec_bumped" "Compono.TUnit" "TUnit.Core" "$json_bumped" + +# 4. A bump to the authoritative range that the packed nuspec does NOT reflect +# must still fail - this is not "always pass," the validator still compares +# independently against packed output. +nuspec_not_rebumped="$work_dir/not-rebumped.nuspec" +make_nuspec "$nuspec_not_rebumped" "TUnit.Core" "[1.65.63, 2.0.0)" +expect_fail "authoritative range bumped but packed nuspec left behind" \ + assert_dependency_range "$nuspec_not_rebumped" "Compono.TUnit" "TUnit.Core" "$json_bumped" + +# 5. Distinct diagnostic: the authoritative lookup itself fails (no matching +# PackageVersion in Directory.Packages.props) - a validator/configuration +# error, not a "packed range mismatch". +props_missing="$work_dir/props-missing.props" +make_props "$props_missing" "SomeOtherPackage" "[1.0.0, 2.0.0)" +json_missing=$(load_authoritative_versions "$props_missing") +missing_output=$(fail=0; assert_dependency_range "$nuspec_matching" "Compono.TUnit" "TUnit.Core" "$json_missing" 2>&1 || true) +if echo "$missing_output" | grep -q "could not determine authoritative PackageVersion for TUnit.Core"; then + echo "PASS: missing authoritative entry produces a distinct diagnostic" +else + echo "TEST FAILURE: expected a distinct 'could not determine authoritative PackageVersion' message, got:" >&2 + echo "$missing_output" >&2 + tests_failed=1 +fi + +# 6. Sanity check against the real repository policy file, so this test suite +# breaks if Directory.Packages.props' shape (Identity/Version JSON) ever stops +# being what the validator expects - independent of any specific package. +repo_root="$script_dir/../.." +real_json=$(load_authoritative_versions "$repo_root/Directory.Packages.props") +real_range=$(get_authoritative_range "$real_json" "NSubstitute") +if [ -n "$real_range" ]; then + echo "PASS: authoritative lookup resolves a real range for NSubstitute from the repository's own Directory.Packages.props ($real_range)" +else + echo "TEST FAILURE: could not resolve NSubstitute's range from the real Directory.Packages.props" >&2 + tests_failed=1 +fi + +if [ "$tests_failed" -ne 0 ]; then + echo "One or more inspect-packed-nupkgs.sh regression tests failed." >&2 + exit 1 +fi + +echo "All inspect-packed-nupkgs.sh regression tests passed." diff --git a/.github/workflows/package-validation.yaml b/.github/workflows/package-validation.yaml index e6ad85f7..49faf3bd 100644 --- a/.github/workflows/package-validation.yaml +++ b/.github/workflows/package-validation.yaml @@ -125,6 +125,12 @@ jobs: dotnet build "$csproj" -c Release -p:WarningsAsErrors=CS1591 done + - name: Test inspect-packed-nupkgs.sh itself + # Regression coverage for issue #122 (dependency-range-literal drift) - + # runs before the real inspection below so a broken validator fails + # fast with its own diagnostic, not a confusing downstream mismatch. + run: .github/scripts/inspect-packed-nupkgs.tests.sh + - name: Inspect packed .nupkg contents run: .github/scripts/inspect-packed-nupkgs.sh "$PACK_OUTPUT" diff --git a/docs/adr/0031-public-preview-release-and-versioning-policy.md b/docs/adr/0031-public-preview-release-and-versioning-policy.md index 254921a0..37523aa5 100644 --- a/docs/adr/0031-public-preview-release-and-versioning-policy.md +++ b/docs/adr/0031-public-preview-release-and-versioning-policy.md @@ -567,6 +567,49 @@ which TFM gets added/dropped going forward at the *newest* end starts two releases further back than the original two-TFM window assumed, per ADR-0038's own reasoning for why that trade-off was accepted. +## Amendment 4 (2026-09-02): dependency-range check derives its expected value from `Directory.Packages.props`, not a duplicated literal + +Amendment 1 above established the tested-range policy for each integration +package's third-party dependency (`[6.2.0, 7.0.0)` for `NSubstitute`, etc.). +Its enforcement, `.github/scripts/inspect-packed-nupkgs.sh`'s +`assert_dependency_range`, originally took that expected range as a second +hardcoded string literal in the script itself, duplicating the +`Directory.Packages.props` `PackageVersion` that already states the same +policy for MSBuild's own restore/pack purposes. + +This surfaced as [issue #122](https://github.com/LayeredCraft/compono/issues/122): +Dependabot PR #121 bumped `TUnit.Core` from `[1.65.38, 2.0.0)` to +`[1.65.63, 2.0.0)` in `Directory.Packages.props`; `Compono.TUnit` packed the +new, correct range; the validator's stale duplicate literal still expected +the old range and failed a correctly-packed package. Commit `5cfe446` +corrected that one occurrence, but the structural bug — two independently- +maintained copies of the same range, one of which Dependabot cannot see — +remained for all five dependency-range checks (`xunit.v3.extensibility.core`, +`NSubstitute`, `Bogus`, `TUnit.Core`, `MSTest.TestFramework`). + +**The fix:** `assert_dependency_range` now reads its expected range directly +from `Directory.Packages.props`, evaluated via `dotnet msbuild +Directory.Packages.props -getItem:PackageVersion` (SDK-native, already a CI +dependency — no new tool) and looked up by the dependency's package ID +(`jq`, already used elsewhere in `package-validation.yaml`). The explicit +package → third-party-dependency-ID mapping in the script's `case` +statement is unchanged — only the *range* is no longer duplicated. The +validator still independently compares this authoritative value against the +actually-packed `.nuspec` (extracted from the real `.nupkg`, not trusted +as-is) — a Dependabot bump to `Directory.Packages.props` is now +automatically the new expected value, but an incorrectly-packed `.nuspec` +that doesn't reflect that value still fails, exactly as before. A missing +`Directory.Packages.props` entry for a checked dependency now fails with a +distinct diagnostic ("could not determine authoritative PackageVersion for +...") rather than being silently mistaken for a packed-range mismatch. +`.github/scripts/inspect-packed-nupkgs.tests.sh` (new) regression-tests both +the passing and failing cases directly against the script's functions, +including the exact bump-only-the-props-file scenario above. + +This is an implementation fix for how Amendment 1's policy is *enforced*, +not a change to the policy itself — the tested-range semantics, exclusive +upper bounds, and package/dependency mapping are all unchanged. + ## Links - [ADR-0001](0001-source-generation-first.md) — source-generation-first