diff --git a/.github/scripts/inspect-packed-nupkgs.sh b/.github/scripts/inspect-packed-nupkgs.sh index 96b2970..8a0b0fd 100755 --- a/.github/scripts/inspect-packed-nupkgs.sh +++ b/.github/scripts/inspect-packed-nupkgs.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -# Asserts the packed .nupkg contents for all seven publishable Compono packages +# Asserts the packed .nupkg contents for every publishable Compono package # match ADR-0031's package-readiness bar (PLAN-0008 Phase 0's package-contents- # inspection CI job): the .nupkg's file listing matches the expected shape # exactly (an allowlist, not a denylist - nothing unexpected snuck in, not just @@ -168,6 +168,65 @@ assert_dependency_range() { fi } +# Compono.Logging's Microsoft.Extensions.Logging.Abstractions dependency is the one exception to +# every other integration package's single, TFM-uniform third-party range: Directory.Packages.props +# conditions it per $(TargetFramework) (net8.0/net9.0/net10.0 each track a different BCL logging- +# abstractions version), and net11.0 carries no such dependency at all in the packed .nuspec - the +# type is satisfied by net11.0's own shared framework, so an explicit PackageReference produces no +# packed dependency entry for that TFM (confirmed against a real local pack, not assumed). +# assert_dependency_range's single authoritative_json blob (evaluated with no $(TargetFramework) set) +# can't see this per-TFM branching, so this sibling function re-evaluates Directory.Packages.props +# once per TFM instead of once per package. +assert_dependency_range_per_tfm() { + local nuspec="$1" + local pkg_name="$2" + local dep_id="$3" + local packages_props="$4" + local tfm + + for tfm in net8.0 net9.0 net10.0; do + local expected_range + expected_range=$(dotnet msbuild "$packages_props" -nologo -getItem:PackageVersion -p:TargetFramework="$tfm" 2>/dev/null \ + | jq -r --arg id "$dep_id" '.Items.PackageVersion[]? | select(.Identity == $id) | .Version' | head -1) + if [ -z "$expected_range" ]; then + echo "FAIL: could not determine authoritative PackageVersion for $dep_id ($tfm) in Directory.Packages.props" >&2 + fail=1 + continue + fi + + local actual_range + actual_range=$(awk -v tfm="$tfm" -v dep="$dep_id" ' + $0 ~ "" { in_group=0 } + in_group && $0 ~ "id=\"" dep "\"" { print; exit } + ' "$nuspec" | sed -E "s/.*id=\"${dep_id}\" version=\"([^\"]*)\".*/\1/") + + if [ "$actual_range" = "$expected_range" ]; then + echo "OK: $pkg_name's .nuspec constrains $dep_id to the intended tested range $actual_range for $tfm (matches Directory.Packages.props)" + else + echo "FAIL: $pkg_name's .nuspec dependency on $dep_id for $tfm is '${actual_range:-}', expected the intended tested range '$expected_range' (from Directory.Packages.props)" >&2 + fail=1 + fi + done + + # net11.0: the BCL's own shared framework satisfies this dependency for that TFM - no packed + # entry should exist at all. Asserted explicitly (not just left unchecked) so a + # regression in either direction - the framework un-bundling it, or a future change accidentally + # reintroducing an explicit dependency - fails loudly instead of silently. + local net11_entry + net11_entry=$(awk -v dep="$dep_id" ' + $0 ~ "" { in_group=0 } + in_group && $0 ~ "id=\"" dep "\"" { print; exit } + ' "$nuspec") + if [ -z "$net11_entry" ]; then + echo "OK: $pkg_name's .nuspec has no $dep_id dependency for net11.0 (satisfied by net11.0's own shared framework)" + else + echo "FAIL: $pkg_name's .nuspec unexpectedly declares a $dep_id dependency for net11.0: $net11_entry" >&2 + fail=1 + fi +} + main() { local pack_output="${1:?usage: inspect-packed-nupkgs.sh }" local script_dir @@ -186,7 +245,7 @@ main() { } 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 Compono.NUnit; do + for pkg in Compono Compono.XunitV3 Compono.NSubstitute Compono.Bogus Compono.TUnit Compono.TestDoubles Compono.DependencyInjection Compono.Http Compono.Logging Compono.MSTest Compono.NUnit; 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 @@ -203,6 +262,12 @@ main() { # ComponoGeneratedTestDoubles (ADR-0043 Amendment 4, Finding F) - without it, # AnalyzerConfigOptionsProvider can never see a consumer's MSBuild setting for the opt-in. extra_paths=$'analyzers/dotnet/cs/Compono.Generators.dll\nbuild/Compono.props\nbuildTransitive/Compono.props' + elif [ "$pkg" = "Compono.Logging" ]; then + # build/ + buildTransitive/ Compono.Logging.props: defaults ComponoGeneratedLogging to true + # (ADR-0055 Amendment 3) - no analyzers/ entry here, unlike Compono above: logging activation + # generation lives inside the existing Compono.Generators (packed only into Compono.nupkg), + # reached transitively through this package's Compono dependency, not a second analyzer DLL. + extra_paths=$'build/Compono.Logging.props\nbuildTransitive/Compono.Logging.props' fi assert_exact_file_listing "$nupkg" "$pkg" "$extra_paths" @@ -259,6 +324,11 @@ main() { # over System.Net.Http (BCL) - nothing else to range-assert here (ADR-0051 "Minimal # dependency graph"). ;; + Compono.Logging) + assert_manifest_field "$nuspec" "$pkg" "title" "Compono — Microsoft.Extensions.Logging Testing Support" + assert_exact_pin_dependency "$nuspec" "$pkg" "Compono" + assert_dependency_range_per_tfm "$nuspec" "$pkg" "Microsoft.Extensions.Logging.Abstractions" "$packages_props" + ;; Compono.MSTest) assert_manifest_field "$nuspec" "$pkg" "title" "Compono — MSTest Integration" assert_exact_pin_dependency "$nuspec" "$pkg" "Compono" diff --git a/.github/scripts/inspect-packed-nupkgs.tests.sh b/.github/scripts/inspect-packed-nupkgs.tests.sh index 7c18c42..de964bf 100755 --- a/.github/scripts/inspect-packed-nupkgs.tests.sh +++ b/.github/scripts/inspect-packed-nupkgs.tests.sh @@ -121,6 +121,71 @@ else tests_failed=1 fi +# 6a. assert_dependency_range_per_tfm: Compono.Logging's real shape - per-TFM ranges for net8/9/10, +# absent for net11.0 (satisfied by net11.0's own shared framework). Uses the real repository +# Directory.Packages.props (via dotnet msbuild -p:TargetFramework=X per TFM, the same mechanism the +# real function uses) rather than a synthetic fixture, since the whole point of this coverage is +# proving the function reads the *actual* per-TFM authoritative values correctly - a synthetic props +# file would just test that the function echoes back whatever synthetic value it was given. +make_multi_tfm_nuspec() { + local path="$1" + local dep_id="$2" + local net8_range="$3" + local net9_range="$4" + local net10_range="$5" + local net11_has_entry="$6" # "yes" or "no" + local net11_line="" + if [ "$net11_has_entry" = "yes" ]; then + net11_line=" " + fi + cat >"$path" < + + + + + + + + + + + + + +$net11_line + + + + +EOF +} + +repo_root="$script_dir/../.." +real_props="$repo_root/Directory.Packages.props" +real_net8=$(dotnet msbuild "$real_props" -nologo -getItem:PackageVersion -p:TargetFramework=net8.0 2>/dev/null | jq -r '.Items.PackageVersion[]? | select(.Identity == "Microsoft.Extensions.Logging.Abstractions") | .Version') +real_net9=$(dotnet msbuild "$real_props" -nologo -getItem:PackageVersion -p:TargetFramework=net9.0 2>/dev/null | jq -r '.Items.PackageVersion[]? | select(.Identity == "Microsoft.Extensions.Logging.Abstractions") | .Version') +real_net10=$(dotnet msbuild "$real_props" -nologo -getItem:PackageVersion -p:TargetFramework=net10.0 2>/dev/null | jq -r '.Items.PackageVersion[]? | select(.Identity == "Microsoft.Extensions.Logging.Abstractions") | .Version') + +nuspec_per_tfm_matching="$work_dir/per-tfm-matching.nuspec" +make_multi_tfm_nuspec "$nuspec_per_tfm_matching" "Microsoft.Extensions.Logging.Abstractions" "$real_net8" "$real_net9" "$real_net10" "no" +expect_pass "per-TFM range matching Directory.Packages.props for net8/9/10, absent for net11.0" \ + assert_dependency_range_per_tfm "$nuspec_per_tfm_matching" "Compono.Logging" "Microsoft.Extensions.Logging.Abstractions" "$real_props" + +# 6b. A stale net9.0 range must fail even though net8.0/net10.0 still match - proves each TFM is +# checked independently, not just "at least one matches". +nuspec_per_tfm_stale_net9="$work_dir/per-tfm-stale-net9.nuspec" +make_multi_tfm_nuspec "$nuspec_per_tfm_stale_net9" "Microsoft.Extensions.Logging.Abstractions" "$real_net8" "[0.0.1, 0.0.2)" "$real_net10" "no" +expect_fail "per-TFM check fails when only net9.0's range disagrees" \ + assert_dependency_range_per_tfm "$nuspec_per_tfm_stale_net9" "Compono.Logging" "Microsoft.Extensions.Logging.Abstractions" "$real_props" + +# 6c. An unexpected net11.0 dependency entry must fail - proves the "must be absent" direction is +# actually checked, not merely unchecked. +nuspec_per_tfm_unexpected_net11="$work_dir/per-tfm-unexpected-net11.nuspec" +make_multi_tfm_nuspec "$nuspec_per_tfm_unexpected_net11" "Microsoft.Extensions.Logging.Abstractions" "$real_net8" "$real_net9" "$real_net10" "yes" +expect_fail "per-TFM check fails when net11.0 unexpectedly declares the dependency" \ + assert_dependency_range_per_tfm "$nuspec_per_tfm_unexpected_net11" "Compono.Logging" "Microsoft.Extensions.Logging.Abstractions" "$real_props" + # 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. diff --git a/.github/workflows/package-validation.yaml b/.github/workflows/package-validation.yaml index eb14981..302ace3 100644 --- a/.github/workflows/package-validation.yaml +++ b/.github/workflows/package-validation.yaml @@ -30,10 +30,10 @@ jobs: # CS1591 enforcement. BREAKING_CHANGE: ${{ contains(github.event.pull_request.labels.*.name, 'breaking-change') }} PACK_OUTPUT: ${{ github.workspace }}/artifacts/package-validation - # Single authoritative publishable-package list (PLAN-0061 Phase 1) - the baseline-lookup, - # pack, and CS1591-enforcement steps below all derive from this one job-level env var instead - # of each repeating the same 11-package literal, so adding/removing a package can't drift - # across the three independently. A job-level `env:` entry is injected into every step's own + # Single authoritative publishable-package list (PLAN-0061 Phase 1) - the baseline-lookup and + # pack steps below both derive from this one job-level env var instead of each repeating the + # same 11-package literal, so adding/removing a package can't drift across the two + # independently. A job-level `env:` entry is injected into every step's own # process environment by GitHub Actions itself, so this survives across the separate `run:` # steps below (each its own shell process) with no extra plumbing - deliberately not a Bash # array, which would only live for the one step that declared it. @@ -108,15 +108,6 @@ jobs: pack_one "src/$pkg/$pkg.csproj" "BASELINE_$(echo "$pkg" | tr '.' '_')" done - - name: Enforce XML doc comments (CS1591) on publishable packages - run: | - set -euo pipefail - for pkg in $PACKAGES; do - csproj="src/$pkg/$pkg.csproj" - echo "Building $csproj with CS1591 as an error" - 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 diff --git a/Directory.Build.targets b/Directory.Build.targets index cfa965e..da0de84 100644 --- a/Directory.Build.targets +++ b/Directory.Build.targets @@ -9,4 +9,21 @@ package surface, so test code doesn't need to satisfy CS1591. --> $(NoWarn);CS1591 + + + + $(WarningsAsErrors);CS1591 + diff --git a/docs/contributing.md b/docs/contributing.md index 17e7163..bde0642 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -55,12 +55,14 @@ need to scope a run to one project or class. handwritten/explicit test data (this repo deliberately doesn't use AutoFixture-style generated test data for its own tests — see [Architecture](architecture/index.md) if you're curious why). -- **XML doc comments are required on every new or changed public member** - across all projects — `Compono` and its integration packages are - published NuGet libraries, and IntelliSense is the primary - discoverability surface for a consumer who's never read the source. - `dotnet build -p:WarningsAsErrors=CS1591` fails a PR that's missing one, - the same gate that runs in CI. +- **XML doc comments are required on every new or changed public member of + a publishable package** (`Compono` and every integration package) — + they're published NuGet libraries, and IntelliSense is the primary + discoverability surface for a consumer who's never read the source. An + ordinary `dotnet build Compono.slnx` already enforces this as a build + error for those packages — no extra flag needed. Non-packable projects + (samples, benchmarks, tests, fixtures) are intentionally outside this + boundary. - **Update the relevant docs page in the same PR**, not as a follow-up — if your change affects behavior a Concept, How-to Guide, or Package Guide already describes, update that page alongside the code. diff --git a/docs/plans/0062-package-validation-gap-fixes.md b/docs/plans/0062-package-validation-gap-fixes.md new file mode 100644 index 0000000..8f1385f --- /dev/null +++ b/docs/plans/0062-package-validation-gap-fixes.md @@ -0,0 +1,168 @@ +# [PLAN-0062] Package-Validation Gap Fixes + +**Status:** Done + +**Implements:** [ADR-0031](../adr/0031-public-preview-release-and-versioning-policy.md) +(implementation-correctness follow-up — no amendment; ADR-0031 requires the +outcomes below, this plan only corrects how CI enforces them) + +## Goal + +Close the two real gaps [RESEARCH-0021](../research/0021-package-validation-execution-policy-audit.md) +found in `package-validation.yaml` and its scripts: `Compono.Logging`'s +packed-nupkg content is never inspected, and CS1591 enforcement runs as a +redundant second full rebuild of all 11 packages instead of as part of the +ordinary build every PR already does. Done when `inspect-packed-nupkgs.sh` +checks all 11 publishable packages with package-specific invariants +correct for `Compono.Logging`'s real shape, and CS1591-as-error is enforced +via `Directory.Build.targets` with the standalone workflow step removed. + +## Scope + +**In scope — two substantive changes only:** +1. Correct `inspect-packed-nupkgs.sh`'s real `Compono.Logging` packed-nupkg + inspection coverage gap — not merely add its name to the loop, but add + the package-specific invariants its actual `.nupkg` shape requires + (see "`Compono.Logging` inspection" below). +2. Move CS1591-as-error enforcement into the ordinary build for publishable + packages (`Directory.Build.targets`, scoped `$(IsPackable) != 'false'`) + and remove the redundant `package-validation.yaml` rebuild step. + +**Explicitly deferred — NUnit compatibility-matrix applicability scoping** +(revised disposition, narrowing RESEARCH-0021's original Tier 2 +recommendation): dropped from this plan's scope entirely, not merely left +optional. RESEARCH-0021 established NUnit as the *safest candidate* for +selective execution, but safety of the mechanism is not itself evidence +that building it now is warranted. There is no demonstrated CI-duration +problem — the whole `package-validation.yaml` job runs in ~3 minutes total, +and removing the redundant CS1591 rebuild (this plan's second change) may +shrink that further without adding any new change-detection machinery. +Building an applicability mechanism speculatively, with no measured +duration problem to justify it, is exactly the kind of premature +optimization this repo's cleanup work has repeatedly declined elsewhere. +**Recorded disposition: deferred/declined, absent future evidence that +`package-validation.yaml`'s duration becomes a meaningful problem in +practice.** Revisit only if that evidence appears. + +**Explicitly deferred** (per RESEARCH-0021's own recommendation, with +evidence, unchanged from the original research record): +- No applicability-aware redesign of the pack/baseline-compare/ + nuspec-inspection/`*SampleTests`-smoke steps — they stay universal. + `publish-preview.yaml`/`publish-release.yaml` perform zero re-validation + of their own, so `package-validation.yaml` is the only safety net these + invariants ever get; under-testing them via a fragile applicability + script is a real risk with no backstop. +- No PR-validation/release-validation two-tier split — no release-time + validation tier exists today to receive an "exhaustive" half, and no + evidence in this repo shows the current single-gate model has caused a + problem. + +## `Compono.Logging` inspection — treated as a correctness bug, not cosmetic + +Pre-1.0 compatibility policy (ADR-0031's `0.x` policy) may intentionally +permit public API evolution without triggering the baseline-compatibility +gate — that tolerance is scoped narrowly to API *shape* changes under a +`breaking-change` label. It says nothing about, and does not excuse, +malformed or incorrect *package contents*: a wrong dependency range, a +missing lockstep pin, or an incomplete file listing are packaging defects +regardless of what version line Compono is on. `Compono.Logging`'s missing +inspection coverage is treated accordingly — a real, silently-uncaught +correctness gap, not a nice-to-have consistency fix. + +Verified (not assumed) against a real local pack of `Compono.Logging`, +which needs invariants distinct from every other integration package +already covered: +- **File listing**: `build/Compono.Logging.props` and + `buildTransitive/Compono.Logging.props` (defaulting + `ComponoGeneratedLogging` to `true`, per ADR-0055 Amendment 3) are extra + files beyond the common template — but, unlike core `Compono`, **no** + `analyzers/dotnet/cs/*.dll` entry: `Compono.Logging` ships no generator + of its own (ADR-0055 Amendment 3 moved its generation into the existing + `Compono.Generators`, embedded only in `Compono.nupkg`). +- **Third-party dependency range**: `Microsoft.Extensions.Logging.Abstractions` + is the *only* third-party dependency in the whole package set whose + `Directory.Packages.props` range is conditioned per `$(TargetFramework)` + (net8.0/net9.0/net10.0 each track a different BCL version) — the + existing `assert_dependency_range` function's single authoritative-value + lookup can't see this. `net11.0` carries **no** such dependency entry at + all in the packed nuspec (satisfied by net11.0's own shared framework) — + a state that must be asserted explicitly, not left unchecked, so a + regression in either direction is caught. +- **Lockstep `Compono` pin**: identical mechanism to every other + integration package — `assert_exact_pin_dependency` applies unchanged. + +## Tasks + +- [x] `.github/scripts/inspect-packed-nupkgs.sh`: add a new + `assert_dependency_range_per_tfm` function for the per-TFM-varying + dependency case (re-evaluates `Directory.Packages.props` once per + TFM via `dotnet msbuild -getItem:PackageVersion -p:TargetFramework=X`, + and explicitly asserts the `net11.0` absence). +- [x] `.github/scripts/inspect-packed-nupkgs.sh`: add `Compono.Logging` to + the `main()` package loop with its correct `extra_paths` (the two + `.props` files, no analyzer) and `case` branch (title, lockstep pin, + per-TFM range assertion). +- [x] `.github/scripts/inspect-packed-nupkgs.sh`: fix the stale header + comment ("all seven publishable Compono packages" → accurate). +- [x] `.github/scripts/inspect-packed-nupkgs.tests.sh`: add regression + coverage for `assert_dependency_range_per_tfm` — a passing case + (real `Directory.Packages.props` values), a failing case (one TFM's + range disagrees), and a failing case (an unexpected `net11.0` entry) + — so this coverage cannot silently disappear or go stale again. +- [x] `Directory.Build.targets`: add + `$(WarningsAsErrors);CS1591`. +- [x] `.github/workflows/package-validation.yaml`: remove the "Enforce XML + doc comments (CS1591)" step; fix the two stale comments that + referenced it (job-level `env:` comment, `BREAKING_CHANGE` comment + context). +- [x] Positive enforcement proof: temporarily added an undocumented public + member to a real packable package (`Compono.Http`), confirmed an + ordinary `dotnet build` (and separately `dotnet pack`) fails with a + real `CS1591` error, removed the temporary member. +- [x] Non-packable boundary proof: temporarily added an undocumented + public member to a non-packable, non-test project with no local + `NoWarn` override (`Compono.Samples.AspNetApi`), confirmed the build + succeeds with `CS1591` remaining an ordinary warning (not promoted to + an error), removed the temporary member. +- [x] Full solution build (`dotnet build Compono.slnx`) to confirm no + sample/benchmark/test/fixture project unexpectedly regresses under + the new `WarningsAsErrors` scoping. +- [x] Confirmed no subsystem doc/ADR describes the CS1591 enforcement + step's specific CI shape (only the XML-doc-comment *requirement* + itself, in `documentation.md`/ADR-0031, which is unchanged) — no + documentation update needed beyond this plan/RESEARCH-0021 themselves. + +## Critical Files + +- `.github/scripts/inspect-packed-nupkgs.sh` — new + `assert_dependency_range_per_tfm` function, `Compono.Logging` coverage. +- `.github/scripts/inspect-packed-nupkgs.tests.sh` — regression coverage + for the new function. +- `Directory.Build.targets` — new scoped `WarningsAsErrors` for CS1591. +- `.github/workflows/package-validation.yaml` — removed the redundant + step; fixed two stale comments. + +## Test Plan + +- `inspect-packed-nupkgs.tests.sh` covers `assert_dependency_range_per_tfm` + directly (pass/fail/fail-on-unexpected-presence), plus the existing + regression suite for every other function, run locally: all pass. +- A real local pack of all 11 publishable packages plus a full + `inspect-packed-nupkgs.sh` run against them: all pass, including + `Compono.Logging`'s new package-specific assertions. +- A full `dotnet build Compono.slnx` after the `Directory.Build.targets` + change, confirming zero new warnings/errors on any project. +- Explicit positive proof (undocumented member in a packable package fails + the build) and explicit negative/boundary proof (same in a non-packable + project does not) — both performed and reverted, not merely asserted. +- A real `package-validation.yaml` CI run confirming: the removed step is + gone with no coverage loss, `Compono.Logging`'s nupkg content is now + correctly inspected, and every other existing check remains green. + +## Notes + +Drafted and implemented from +[RESEARCH-0021](../research/0021-package-validation-execution-policy-audit.md). +This plan does not fold into PLAN-0061 — it is an explicit follow-up +discovered after PLAN-0061 Phase 1 completed, not part of that plan's own +history. diff --git a/docs/plans/README.md b/docs/plans/README.md index bb041fe..4f5a34b 100644 --- a/docs/plans/README.md +++ b/docs/plans/README.md @@ -76,3 +76,4 @@ one. This file is just the mechanics: numbering, status, and the index. | [0059](0059-compono-nunit-package-design-impl-plan.md) | Compono.NUnit Package Design | Done | | [0060](0060-public-generator-facing-runtime-infrastructure.md) | Public Generator-Facing Runtime Infrastructure | Done | | [0061](0061-pre-1-0-cleanup-and-consolidation.md) | Pre-1.0 Cleanup and Consolidation Gate | In Progress | +| [0062](0062-package-validation-gap-fixes.md) | Package-Validation Gap Fixes | Done | diff --git a/docs/research/0020-repository-wide-comment-quality-audit.md b/docs/research/0020-repository-wide-comment-quality-audit.md new file mode 100644 index 0000000..b604941 --- /dev/null +++ b/docs/research/0020-repository-wide-comment-quality-audit.md @@ -0,0 +1,137 @@ +# [RESEARCH-0020] Repository-Wide Comment Quality Audit + +**Status:** Complete — no follow-up work warranted. + +## Why this exists + +The original pre-1.0 repository-wide cleanup audit (which produced +PLAN-0061) didn't evaluate comments as an explicit dimension. This is a +narrow, dedicated follow-up: audit every comment-bearing area of the +repository against `references/coding-standards.md`'s already-stated +policy ("Default to writing no comments. Only add one when the WHY is +non-obvious... Don't explain WHAT the code does") and classify what's +actually there. + +## Method + +Read-only. Comment-line inventory by area (grep-counted), followed by +in-context reading (not grep-snippet reading) of a representative sample +across categories most likely to surface a problem if one existed: +files with the heaviest ADR/PLAN/PR-number reference density, files with +unusually long comment blocks, and a control sample of ordinary +"workhorse" files unlikely to be special-cased. Targeted greps for +classic narration smells (`// This method...`, `// Loop through...`, +`// Set the...`, restated-trivial-property comments) were run across all +of `src/`, `test/`, `samples/`. + +## Inventory (comment lines / total lines) + +| Area | Comment / total lines | Density | +|---|---|---| +| `src/Compono` | 2563 / 5242 | ~49% | +| `src/Compono.Generators` | 2810 / 7410 | ~38% | +| `src/Compono.XunitV3` | 475 / 1021 | — | +| `src/Compono.TUnit` | 403 / 965 | — | +| `src/Compono.MSTest` | 350 / 919 | — | +| `src/Compono.NUnit` | 345 / 893 | — | +| `src/Compono.Bogus` | 234 / 459 | — | +| `src/Compono.Http` | 186 / 433 | — | +| `src/Compono.Logging` | 289 / 720 | — | +| `src/Compono.DependencyInjection` | 139 / 270 | — | +| `src/Compono.NSubstitute` | 43 / 97 | — | +| `src/Compono.TestDoubles` | 19 / 37 | — | +| `test/` | 7041 / 52257 | ~13.5% (largest absolute volume, lowest density) | +| `samples/` | 88 / 358 | — | +| `.github/workflows/` | 138 / 696 | — | +| `.github/scripts/` | 229 / 782 | — | + +Density is highest exactly where invariants are subtlest (core engine, +generator) and lowest in samples and mechanical test bodies — consistent +with the stated policy being actually followed, not a random accumulation. + +## Findings by classification bucket + +- **Durable why** — the large majority of everything read. Representative + examples: `PositionalArgumentBinder.cs:47-50` (a CLR nullable-boxing + rule explaining why an unwrap-first step is required, not optional); + `TransitiveClosureWalker.cs:111-114` (a defensive narrowing explicitly + documented as intentional rather than an oversight a reader might + "fix"); `generate-api-reference.sh:19-25` (a DefaultDocumentation + cross-link fallback that would silently 404 if simplified away). +- **ADR/PLAN breadcrumb that earns its keep** — `TestDoubleAnalyzer.cs`'s + ~135 `Codex review, PR #106/#108`-style references each sit next to a + collision-safety rule that has been *wrong in review before* — the + breadcrumb is exactly what stops a future maintainer (or a coding + agent) from "helpfully" reverting to the buggy shape. Same reasoning + applies to `MatchingTests.cs`'s per-fixture-interface comments — the + fixture's entire purpose is regression-proofing one specific finding, + so the comment *is* the information, not decoration. +- **ADR/PLAN breadcrumb that doesn't earn its keep** — none found in the + sampled files. +- **Restates the code** — none found; the targeted narration-smell greps + returned zero matches across the entire `src/`, `test/`, `samples/` + tree. +- **Historical narration with no current value** — none found. Every + "we tried X, it broke because Y" comment sampled was load-bearing: it + explains why the *current* shape must stay, not merely what changed. +- **Large prose block obscuring simple code** — `TestDoubleAnalyzer.cs` + is the one real candidate, at the file level (2310 lines, extremely + dense). This is not a new finding — the original pre-1.0 cleanup audit + already flagged this exact file as a NEEDS-SPIKE decomposition + candidate specifically because of its hard-won correctness history. + Any fix here is a structural decomposition question with real risk, + not a comment-trimming exercise — see PLAN-0061's Explicitly Deferred + list, unchanged by this audit. +- **Compensating for unclear naming/structure** — none found. + +## Removal / shortening / retain lists + +- **Remove**: none identified. +- **Shorten**: none identified beyond the `TestDoubleAnalyzer.cs` volume + concern, which is structural (see above), not a comment-level edit. +- **Retain (representative, to calibrate what "good" looks like here)**: + `PositionalArgumentBinder.cs:47-50`, `TransitiveClosureWalker.cs:111-114`, + every `MatchingTests.cs` fixture-interface comment, + `generate-api-reference.sh`'s header block. + +## Assessment + +This codebase already conforms tightly to its own stated comment policy. +This is not a repository that drifted and needs a cleanup pass — the +evidence (targeted greps returning zero hits for every classic narration +smell, no removal candidates surfacing anywhere in a deliberately +adversarial sample) reads as actively enforced, consistent with the +heavy, iterated review history (Codex rounds) already visible in the +comments themselves. + +**How much of this is mechanical vs. judgment-requiring**: neither — +there is no backlog to apply either mode to. A "the repo is already +clean" finding is itself a complete, valid result of this audit, not an +absence of effort. + +## Tier classification + +**Tier 3 — leave alone, entirely.** No Tier 1 or Tier 2 findings anywhere +in this audit. + +## Proposed implementation scope + +None warranted. The only actionable item adjacent to this audit is the +already-known `TestDoubleAnalyzer.cs` decomposition spike carried forward +unchanged from the original pre-1.0 cleanup audit (PLAN-0061's +Explicitly Deferred list) — this is not new work this audit discovered. + +## Relationship to 1.0 + +- Nothing here should block PLAN-0061 Phase 2. +- Nothing here should block 1.0. +- Nothing found here becomes harder or riskier to change after 1.0 — + there is nothing to change. + +## Links + +- `references/coding-standards.md` — the comment policy this audit + checked actual comments against (not a new policy authored here). +- [PLAN-0061](../plans/0061-pre-1-0-cleanup-and-consolidation.md) — the + pre-1.0 cleanup plan this audit follows up on; `TestDoubleAnalyzer.cs`'s + decomposition remains tracked there (Explicitly Deferred), not here. diff --git a/docs/research/0021-package-validation-execution-policy-audit.md b/docs/research/0021-package-validation-execution-policy-audit.md new file mode 100644 index 0000000..7e22566 --- /dev/null +++ b/docs/research/0021-package-validation-execution-policy-audit.md @@ -0,0 +1,275 @@ +# [RESEARCH-0021] Package-Validation Execution-Policy Audit + +**Status:** Complete. Two real findings recommended for a follow-up plan +(not implemented here); one findings area explicitly recommends leaving +the current design alone. + +## Why this exists + +PLAN-0061 Phase 1 touched `.github/workflows/package-validation.yaml` (the +publishable-package list consolidation) and, separately, built +`.github/workflows/aot-validation.yaml` as a new applicability-aware, +change-detection-driven CI gate. This raised the natural question: should +`package-validation.yaml`'s current "run everything, on every PR" policy +be redesigned the same way? This is a dedicated, read-only follow-up +audit answering that, plus two real defects the audit surfaced along the +way. + +## Method + +Read `package-validation.yaml` step-by-step, `aot-validation.yaml` (the +comparison precedent), `.github/scripts/inspect-packed-nupkgs.sh` + +its own test script, `.github/scripts/nunit-compatibility-matrix.sh`, +ADR-0031 (the package-readiness ADR this workflow implements, including +all 5 amendments), ADR-0059's NUnit compatibility-matrix rationale, and +`publish-preview.yaml`/`publish-release.yaml` (to determine whether any +release-time re-validation exists). Every load-bearing claim below was +independently re-verified against the actual files (not taken on an +initial pass's word alone) before being recorded here. + +## Step-by-step findings + +### 1. Restore +Trivial, no findings. + +### 2. Resolve nuget.org baseline versions +Feeds check 3's API-compatibility baseline; no failure of its own beyond +network I/O. Structurally independent per package. Could be package-scoped +safely, but it's cheap (read-only HTTP) — low value in isolation. + +### 3. Pack publishable packages (11 packages) + API-compatibility baseline check +Catches an unintentional public-API breaking change and any packing-time +failure. Requires the packed artifact specifically — `dotnet build`/`test` +never invoke the pack target or the baseline-compare MSBuild target. +Each package's own compat baseline is structurally independent (`dotnet +pack` here doesn't require sibling packages' `.nupkg`s to exist on disk). +**Value increases sharply post-1.0**: pre-1.0, ADR-0031's own `0.x` +compatibility policy already permits a labeled breaking-change PR to +bypass this; post-1.0, a false negative here is a real, uncommunicated +consumer-facing break. + +### 4. Enforce XML doc comments (CS1591) — real finding, see dedicated section below. + +### 5/6. `inspect-packed-nupkgs.tests.sh` + `inspect-packed-nupkgs.sh` +Catches packaging misconfiguration invisible to `dotnet build`/`test`: wrong +`PrivateAssets`, a missing `build/`/`analyzers/` asset, a non-lockstep +Compono dependency pin, an untested/unbounded dependency range drifting +from `Directory.Packages.props`. Requires the packed nupkg specifically +(unzips and inspects the real file listing/`.nuspec`). Per-package, +structurally independent — each package has its own `case` branch in the +script. + +**Real finding, independently verified**: `inspect-packed-nupkgs.sh`'s +`main()` loop (line 189) iterates 10 packages — +`Compono Compono.XunitV3 Compono.NSubstitute Compono.Bogus Compono.TUnit +Compono.TestDoubles Compono.DependencyInjection Compono.Http Compono.MSTest +Compono.NUnit` — **`Compono.Logging` is missing entirely.** Confirmed by +direct read of the file: no `Compono.Logging` branch or loop entry exists +anywhere in the script. `Compono.Logging`'s packed nupkg content, manifest +fields, and lockstep dependency pin have zero content-inspection coverage +today. This is exactly the class of silent drift risk this cleanup gate +exists to catch. + +### 7. Local-feed packed-consumer smoke tests (5 `*SampleTests` steps) +Catches a packaging defect only visible when consumed as a real external +package. Each of the 5 (XunitV3/TUnit/MSTest/NUnit/TestDoubles) is +structurally independent — XunitV3's proof says nothing about TUnit's +chain. Requires the packed artifact by design (confirmed unchanged via +`test/Compono.XunitV3.SampleTests/README.md`'s own classification as a +packaged-consumer validation fixture, not a user sample). + +### 8. NUnit compatibility matrix +Catches a resolved-NUnit-assembly-version regression across the supported +`[3.14.0, 5.0.0)` range × VSTest/MTP runners — ADR-0059 §6's explicit, +accepted monitoring requirement for the NUnit-internal-namespace +dependency risk. Only `Compono.NUnit`'s own source, its `NUnit` version +range in `Directory.Packages.props`, or the matrix script itself can +invalidate it. This is the single most narrowly-scoped and most expensive +check in the job (4-5 full build+dual-runner-run legs). + +## CS1591 — dedicated finding + +**`Directory.Build.props` (line 82) already sets `GenerateDocumentationFile=true` +unconditionally**, with its own comment (lines 70-80) stating explicitly +that CS1591 is "deliberately left as a real build warning rather than +suppressed... so a missing doc comment on a new public member is caught +immediately." Confirmed by direct read: **no `WarningsAsErrors`/ +`TreatWarningsAsErrors` exists anywhere in the repo.** `Directory.Build.targets` +adds `CS1591` to `NoWarn` only when `IsTestProject == true` (line 10). + +**This means CS1591 is already an ordinary build warning on every +`pr-build.yaml` run for every non-test project today.** +`package-validation.yaml`'s "Enforce XML doc comments" step only promotes +that pre-existing warning to a hard error — via a **full second `dotnet +build` of all 11 packages**, solely to add `-p:WarningsAsErrors=CS1591`. + +Independently verified every non-packable project (all samples, +benchmarks, and `test/*` projects — 30 csproj files checked) sets +`IsPackable=false` directly in its own csproj, not only via the +`IsTestProject`-derived default. This means a `Directory.Build.targets` +condition on `$(IsPackable) != 'false'` would apply `WarningsAsErrors` +for CS1591 **only** to the same 11 real publishable packages +`package-validation.yaml` already targets, with zero risk of newly +breaking any sample/benchmark/test/fixture project. + +**Recommendation**: move CS1591 enforcement into `Directory.Build.targets` +(scoped `$(IsPackable) != 'false'`), delete the redundant "Enforce XML doc +comments" step from `package-validation.yaml`. This catches a missing doc +comment on the PR that introduces it — via ordinary `pr-build.yaml`, not +only at the separate package-validation gate — strictly earlier feedback, +and removes 11 redundant full rebuilds from the job. No ADR governs the +specific enforcement *mechanism* (ADR-0031 requires the XML-doc-coverage +outcome, not this particular CI shape), so this is a pure implementation +correction, not a policy change. + +## Redundancy findings + +None of `package-validation.yaml`'s checks duplicate `pr-build.yaml` +(build/test only, never packs), `docs.yml` (docs-only), or +`aot-validation.yaml` (Native-AOT runtime survival, a different guarantee +than packaging correctness). The CS1591 finding above is about a +duplicated *mechanism* (a second full rebuild), not duplicated coverage. + +## Applicability-aware redesign — recommendation: partial + +**Do it only for the NUnit compatibility matrix; leave every other check +universal.** + +Evidence against broad selectivity: the whole job (11 packages + 5 +`*SampleTests` + the NUnit matrix) already completes in roughly 3 minutes +(confirmed from PLAN-0061 Phase 1's own PR #128 CI run). An +applicability-aware redesign mirroring `aot-validation.yaml`'s +`changes`/`smoke`/`gate` structure would add per-leg +checkout/restore/setup-dotnet overhead across up to 16 legs (11 packages + +5 SampleTests) — plausibly costing *more* aggregate CI resource than it +saves on a job this size. + +**The decisive evidence, independently confirmed**: `publish-preview.yaml` +and `publish-release.yaml` are both opaque `uses:` calls to the shared +external `LayeredCraft/devops-templates` reusable workflow. Read in full — +neither re-runs the API-compatibility baseline check, `inspect-packed-nupkgs.sh`, +CS1591 enforcement, any `*SampleTests` smoke test, or the NUnit +compatibility matrix. **`package-validation.yaml` is the only safety net +these invariants will ever get, at any point in the pipeline.** +`aot-validation.yaml`'s own applicability script was exactly the kind of +change-detection logic Codex's review of PR #128 caught a real fail-open +bug in — proving this class of script is genuinely bug-prone even when +carefully built. Introducing a new one here, for checks with no +release-time backstop if the script under-detects, is a real risk not +justified by the modest time savings on an already-fast job. This matches +the audit brief's own steer: prefer a simple, comprehensive gate over a +clever applicability system that can silently under-test changes. + +The NUnit compatibility matrix is the one legitimate exception: narrowest +blast radius of any check here (only `Compono.NUnit`-relevant changes can +invalidate it), and it is also the slowest/most expensive leg — the +cost/benefit of scoping it is real, and an incorrect skip has a much +smaller blast radius (one framework's version-compat surveillance, not +core packaging correctness) than an incorrect skip anywhere else in this +job would. + +## PR-validation vs. release-validation split — recommendation: not supported by evidence + +The proposed two-tier framing ("PR validation: this change hasn't broken +package contracts it could plausibly affect" vs. "release validation: +every package about to publish is comprehensively validated") assumes a +release-time validation tier already exists to receive the "exhaustive" +half of that split. It doesn't. `publish-preview.yaml`/`publish-release.yaml` +trust the PR gate's prior result unconditionally and perform no +re-validation of their own. The honest framing isn't "split lightweight +PR checks from exhaustive release checks" — it's "`package-validation.yaml` +**is** the only validation, full stop, and it happens to run at PR time." +Building a genuine release-time exhaustive check would be new +infrastructure, not a refactor of the existing gate, and no evidence in +this repo suggests the current single-gate model has actually caused a +problem. **Recommend Tier 3 (leave alone)** unless/until a concrete +release-time gap is found in practice. + +## Tier classification + +- **Tier 1**: fix the missing `Compono.Logging` entry in + `inspect-packed-nupkgs.sh`'s package loop (real, silent coverage gap, + independently verified). Move CS1591 enforcement into + `Directory.Build.targets` (scoped `$(IsPackable) != 'false'`); delete + the now-redundant "Enforce XML doc comments" step from + `package-validation.yaml`. +- **Tier 2**: make the NUnit compatibility matrix step conditionally + skippable via a small, narrowly-scoped applicability check (only + `Compono.NUnit`-relevant changes need to run it). +- **Tier 3**: leave the pack/baseline-compare/nuspec-inspection/ + `*SampleTests`-smoke steps universal — no applicability-aware redesign. + Leave the PR-validation/release-validation split unbuilt — no + release-time backstop exists to make partial PR-time coverage safe. + +## Relationship to 1.0 + +- **Should this block PLAN-0061 Phase 2?** No — Phase 2 is sample + coverage work, unrelated to package-validation mechanics. +- **Should this block 1.0?** No — the missing `Compono.Logging` inspection + coverage and the CS1591 double-rebuild are both real but low-severity + (neither currently causes an undetected failure; `Compono.Logging` + simply isn't checked as deeply as its 10 siblings, and CS1591 is still + enforced today, just via extra CI cost). +- **What becomes materially harder/riskier to change after 1.0?** The + API-compatibility-baseline check (step 3) and the lockstep-pin/ + dependency-range nuspec inspection (steps 5/6, including the + `Compono.Logging` gap) — a false negative in either is advisory pre-1.0 + (ADR-0031's `0.x` policy tolerates it) but becomes a real, + uncommunicated consumer-facing break once 1.0.0 ships. This argues for + fixing the `Compono.Logging` gap **before** 1.0, not after, even though + it isn't a hard blocker. +- **What can safely wait until after 1.0?** The CS1591-relocation + mechanism change (pure CI efficiency, no coverage change) and the NUnit + matrix applicability scoping (a cost optimization, not a correctness + fix) are both safe to defer past 1.0 if there's ever a reason to + prioritize other work first. + +## Unverified assumptions requiring a spike before implementation + +- Whether any currently-`IsPackable=false` project has an undocumented + public member that would newly trip CS1591-as-error if the proposed + `Directory.Build.targets` condition were scoped even slightly wrong — + the recommendation above is scoped specifically to avoid this + (`IsPackable != 'false'` matches exactly the 11 packages + `package-validation.yaml` already targets, verified against all 30 + non-packable csproj files), but a real CI run of the change should + confirm no unexpected breakage across the whole solution before + merging it. +- The CS1591 step's actual individual wall-clock cost wasn't measured in + isolation (only the job's ~3-minute total is known) — worth confirming + its removal produces a measurable time saving, not just a + redundancy-on-paper argument. + +## No ADR proposed + +Nothing in this audit surfaces a genuine architectural or long-lived +policy decision that needs its own ADR. The `Compono.Logging` inspection +gap is a script bug (an omission from a loop), not a policy question. The +CS1591 relocation is an implementation-mechanism correction — ADR-0031 +already establishes *that* XML docs are required; it never mandated *how* +that gets enforced in CI. The applicability-aware-redesign question is +answered by evidence (partial: NUnit matrix only) without needing a new +decision record — it's an application of the same judgment ADR-0041 +Amendment 7 already exercised, not a new one. + +## Recommended follow-up + +[PLAN-0062](../plans/0062-package-validation-gap-fixes.md) — not yet +implemented, drafted alongside this research record as the smallest +scoped follow-up for the Tier 1/Tier 2 findings above. + +## Links + +- [ADR-0031](../adr/0031-public-preview-release-and-versioning-policy.md) — + the package-readiness ADR this workflow implements; its `0.x` + compatibility policy is why the baseline-compare/nuspec-inspection + findings above are framed as "advisory now, real risk post-1.0." +- [ADR-0059](../adr/0059-compono-nunit-package-design.md) — §6's + monitoring requirement, the reason the NUnit compatibility matrix + exists. +- `.github/workflows/aot-validation.yaml` — the applicability-aware + precedent this audit evaluated `package-validation.yaml` against, and + the source of the "a fail-open bug is realistic even when carefully + built" evidence (the Codex-caught gap fixed on PR #128). +- [PLAN-0061](../plans/0061-pre-1-0-cleanup-and-consolidation.md) — the + cleanup plan this audit follows up on.