build: actually enable NuGet package validation (props/targets evaluation-order bug) (#551)

* build: actually enable NuGet package validation

EnablePackageValidation and the CS1591 waiver were conditioned on
$(IsPackable) / $(PackageId) inside Directory.Build.props, which is
imported before project bodies set those properties — both conditions
always saw empty values, so package validation never ran for any project
and CS1591 was waived everywhere. The same evaluation-order rule was
already documented in Directory.Build.targets for PackageReadmeFile;
both conditions now live there. An unset IsPackable counts as packable
because NuGet's own IsPackable=true default is applied by the pack
targets, which import even later.

PackageValidationBaselineVersion stays in Directory.Build.props so the
bump-baseline workflow's sed keeps finding it.

Fallout from turning the gates on:
- CS1591 enforcement surfaced 16 undocumented public constants in
  WopiTelemetry — documented.
- Baseline validation against 9.0.0 flagged the two intentional
  AddActivities interface additions on WopiHost.Abstractions —
  suppressed via the generated CompatibilitySuppressions.xml.

Verified with a release-style pack (dotnet pack -p:Version=9.99.0):
validation downloads the 9.0.0 baselines and passes for all nine
published packages.

https://claude.ai/code/session_019MeqK2SERqN3JAQKAKaRKR

* build: drop tombstone comments pointing from props to targets

The CS1591-waiver note and the EnablePackageValidation note in
Directory.Build.props described code that no longer lives there; the
conditions and their evaluation-order rationale are documented in
Directory.Build.targets where they actually run. The baseline-version
comment stays, since it documents the property directly below it.

* ci(bump-baseline): retire CompatibilitySuppressions.xml when the baseline advances

A suppression records an intentional break against the previous baseline.
Once bump-baseline moves the baseline to the just-released version those
breaks are part of the baseline and the suppressions match nothing, so the
auto-bump PR now git-rm's them in the same change. The glob matches at any
depth, so it works for a single shared file or per-project files.

* ci(api-compat): make the PR check suppression-aware and blocking

Feed each project's CompatibilitySuppressions.xml (the same default path
the SDK's pack-time validator reads at release) to the apicompat global
tool via --suppression-file, and fail the job on any break that isn't
suppressed. Acknowledged breaks are masked, so the report shows only what
the PR itself changes; a deliberate break is acknowledged by committing the
regenerated suppression file. The sticky comment now posts on always() so
the report is visible even when the check fails.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: petrsvihlik

.github/workflows/bump-baseline.yml

@@ -112,6 +112,24 @@ jobs:
 
           sed -i "s|<PackageValidationBaselineVersion>[^<]*</PackageValidationBaselineVersion>|<PackageValidationBaselineVersion>${NEW}</PackageValidationBaselineVersion>|g" "$FILE"
 
+      - name: Retire suppressions absorbed into the new baseline
+        # A CompatibilitySuppressions.xml entry records an intentional API break against the
+        # *previous* baseline. Once the baseline advances to the just-released version those
+        # breaks are part of the baseline, so the suppressions match nothing and are dead
+        # weight. Drop them in the same bump PR so each release cycle starts clean and the
+        # files only ever carry breaks introduced *after* the last release. The glob matches
+        # at any depth, so it works whether suppressions live in one file or one per project.
+        shell: bash
+        run: |
+          set -euo pipefail
+          mapfile -t files < <(git ls-files '*CompatibilitySuppressions.xml')
+          if [[ ${#files[@]} -eq 0 ]]; then
+            echo "No CompatibilitySuppressions.xml files to retire."
+          else
+            printf 'Retiring: %s\n' "${files[@]}"
+            git rm --quiet "${files[@]}"
+          fi
+
       - name: Open PR with the bump
         # peter-evans/create-pull-request is intentionally idempotent: if Directory.Build.props
         # has no diff (because CURRENT was already at NEW, or because the backward-bump guard
@@ -131,6 +149,8 @@ jobs:
             ${{ github.event_name == 'release' && format('Triggered by the [{0}]({1}) release.', github.event.release.tag_name, github.event.release.html_url) || 'Triggered manually via workflow_dispatch.' }}
 
             Keeps the SDK's pack-time package validator comparing the current public surface against the version that's actually published to NuGet.org.
+
+            Any `CompatibilitySuppressions.xml` left over from the previous cycle is retired here too — those entries are now part of the baseline and would otherwise linger as dead weight.
           labels: |
             chore
             automated

.github/workflows/pull_request.yml

@@ -152,7 +152,11 @@ jobs:
           echo "<!-- apicompat-report -->"
           echo "## API Compatibility Report"
           echo ""
-          echo "Compared this PR's packed assemblies against the latest stable release on [NuGet.org](https://www.nuget.org/packages?q=WopiHost) for each library. This check is **informational only** — intentional breaks at major version bumps are expected."
+          echo "Compared this PR's packed assemblies against the latest stable release on [NuGet.org](https://www.nuget.org/packages?q=WopiHost) for each library, honoring each project's \`CompatibilitySuppressions.xml\`. **This check fails on any public-API break not recorded as intentional** — already-suppressed breaks are masked, so what surfaces below is what *this PR* changes. If a break is deliberate, regenerate and commit the suppressions:"
+          echo ""
+          echo '```'
+          echo "dotnet pack -c Release -p:ApiCompatGenerateSuppressionFile=true"
+          echo '```'
           echo ""
         } > "$REPORT_FILE"
 
@@ -187,21 +191,31 @@ jobs:
             "https://api.nuget.org/v3-flatcontainer/${pkg_lower}/${baseline_version}/${pkg_lower}.${baseline_version}.nupkg" \
             -o "$baseline_nupkg"
 
+          # Feed the project's suppression file (default path; the same one the SDK's pack-time
+          # validator reads at release) so acknowledged breaks are masked and only new ones show.
+          # --permit-unnecessary-suppressions keeps a stale entry (one whose break is already in
+          # the baseline, before the post-release cleanup runs) from failing the check.
+          supp_args=()
+          supp_file="src/${pkg}/CompatibilitySuppressions.xml"
+          [[ -f "$supp_file" ]] && supp_args+=(--suppression-file "$supp_file")
+
           # Run apicompat. Note: <package> is positional, --baseline-package is named, --run-api-compat is required.
           # The tool installs as 'apicompat' (not 'dotnet apicompat') and exits 0 on both clean and break
           # cases; detect findings via output text.
           set +e
           output="$(apicompat package "$new_nupkg" \
             --baseline-package "$baseline_nupkg" \
             --run-api-compat \
+            --permit-unnecessary-suppressions \
+            "${supp_args[@]}" \
             --verbosity Normal 2>&1)"
           set -e
 
           if grep -Eq '^(CP|PKV)[0-9]+:' <<< "$output"; then
             ANY_DIFFS=1
             {
-              echo "### :warning: ${pkg} vs \`${baseline_version}\`"
-              echo "<details><summary>API differences found — click to expand</summary>"
+              echo "### :x: ${pkg} vs \`${baseline_version}\`"
+              echo "<details open><summary>Unsuppressed public-API break(s) — revert or suppress</summary>"
               echo ""
               echo '```'
               echo "${output}"
@@ -222,15 +236,23 @@ jobs:
         {
           echo "---"
           if [[ $ANY_DIFFS -eq 0 ]]; then
-            echo "_No API differences detected across any package._"
+            echo "_No unsuppressed public-API breaks across any package._"
           else
-            echo "_Diagnostic IDs starting with \`CP\` are assembly-level diffs; \`PKV\` are package-shape diffs. See the [ApiCompat docs](https://learn.microsoft.com/dotnet/fundamentals/apicompat/diagnostic-ids) for details._"
+            echo "_If these breaks are intentional, run \`dotnet pack -c Release -p:ApiCompatGenerateSuppressionFile=true\` and commit the regenerated \`CompatibilitySuppressions.xml\`. \`CP\` IDs are assembly-level diffs; \`PKV\` are package-shape diffs ([docs](https://learn.microsoft.com/dotnet/fundamentals/apicompat/diagnostic-ids))._"
           fi
         } >> "$REPORT_FILE"
 
         # Surface in the job log too.
         cat "$REPORT_FILE"
+
+        # Fail the check on any unsuppressed break (the report is posted by the next step,
+        # which runs on always()). A deliberate break is acknowledged via its suppression file.
+        if [[ $ANY_DIFFS -ne 0 ]]; then
+          echo "::error::Unsuppressed public-API break(s) detected — see the API Compatibility Report comment."
+          exit 1
+        fi
     - name: Post sticky PR comment
+      if: always()
       uses: marocchino/sticky-pull-request-comment@v3
       with:
         header: apicompat-report

Directory.Build.props

@@ -50,12 +50,6 @@
 		<IncludeCobalt Condition="'$(IncludeCobalt)' == ''">false</IncludeCobalt>
 	</PropertyGroup>
 
-	<!-- Non-packable projects (samples, infra, tests): silence CS1591 since they don't
-	     ship to consumers and don't need XML docs on every public member. -->
-	<PropertyGroup Condition="$(IsPackable) != 'true' OR $(PackageId) == ''">
-		<NoWarn>$(NoWarn);CS1591</NoWarn>
-	</PropertyGroup>
-
 	<!-- Package Validation Configuration for NuGet packages.
 
 	     PackageValidationBaselineVersion is the last NuGet-published version we validate the
@@ -67,8 +61,7 @@
 	     after the release then leaves the baseline at the wrong version with no auto-bump PR
 	     queued. If you genuinely need to override the baseline (e.g. to skip a buggy release),
 	     do it in its own dedicated PR, not bundled with API changes. -->
-	<PropertyGroup Condition="$(IsPackable) == 'true' AND $(PackageId) != ''">
-		<EnablePackageValidation>true</EnablePackageValidation>
+	<PropertyGroup>
 		<PackageValidationBaselineVersion>9.0.0</PackageValidationBaselineVersion>
 	</PropertyGroup>
 

Directory.Build.targets

@@ -5,4 +5,26 @@
   <PropertyGroup Condition="'$(PackageId)' != ''">
     <PackageReadmeFile>README.md</PackageReadmeFile>
   </PropertyGroup>
+
+  <!-- Same evaluation-order rule as PackageReadmeFile above: these conditions read
+       $(IsPackable) / $(PackageId), which projects set in their bodies (or directory-level
+       props imported below the root), so they only see the final values here. An UNSET
+       IsPackable counts as packable — NuGet's own `IsPackable=true` default is applied by the
+       pack targets, which import after this file, so it can't be observed here either. -->
+
+  <!-- Missing-XML-doc warnings are waived for everything that is not a published package
+       (tests, samples, infra, tools); the packaged libraries keep CS1591 enforced via
+       TreatWarningsAsErrors. -->
+  <PropertyGroup Condition="$(IsPackable) == 'false' OR $(PackageId) == ''">
+    <NoWarn>$(NoWarn);CS1591</NoWarn>
+  </PropertyGroup>
+
+  <!-- Pack-time API validation for published packages, comparing against
+       $(PackageValidationBaselineVersion) from Directory.Build.props (auto-bumped after each
+       stable release by .github/workflows/bump-baseline.yml). The empty-value guard respects
+       per-project opt-outs: a package with no prior release on NuGet.org sets
+       EnablePackageValidation=false in its csproj because there is no baseline to download. -->
+  <PropertyGroup Condition="$(EnablePackageValidation) == '' AND $(IsPackable) != 'false' AND $(PackageId) != ''">
+    <EnablePackageValidation>true</EnablePackageValidation>
+  </PropertyGroup>
 </Project>

src/WopiHost.Abstractions/CompatibilitySuppressions.xml

@@ -0,0 +1,18 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- https://learn.microsoft.com/dotnet/fundamentals/package-validation/diagnostic-ids -->
+<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
+  <Suppression>
+    <DiagnosticId>CP0006</DiagnosticId>
+    <Target>M:WopiHost.Abstractions.IWopiHostExtensions.OnAddActivitiesAsync(WopiHost.Abstractions.WopiAddActivitiesContext,System.Threading.CancellationToken)</Target>
+    <Left>lib/net10.0/WopiHost.Abstractions.dll</Left>
+    <Right>lib/net10.0/WopiHost.Abstractions.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0006</DiagnosticId>
+    <Target>P:WopiHost.Abstractions.IWopiHostCapabilities.SupportsAddActivities</Target>
+    <Left>lib/net10.0/WopiHost.Abstractions.dll</Left>
+    <Right>lib/net10.0/WopiHost.Abstractions.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+</Suppressions>

src/WopiHost.Core/Infrastructure/WopiTelemetry.cs

@@ -37,30 +37,59 @@ public static class WopiTelemetry
     /// <summary>Tag keys shared across spans, log scopes, and metrics.</summary>
     public static class Tags
     {
+        /// <summary>WOPI operation name (e.g. <c>CheckFileInfo</c>, <c>Lock</c>).</summary>
         public const string Operation = "wopi.operation";
+
+        /// <summary>File identifier of the targeted resource.</summary>
         public const string FileId = "wopi.file_id";
+
+        /// <summary>Container identifier of the targeted resource.</summary>
         public const string ContainerId = "wopi.container_id";
+
+        /// <summary>Lock identifier involved in the operation.</summary>
         public const string LockId = "wopi.lock_id";
+
+        /// <summary>Operation outcome; values come from <see cref="Outcomes"/>.</summary>
         public const string Outcome = "wopi.outcome";
+
+        /// <summary>The request's <c>X-WOPI-Override</c> header value.</summary>
         public const string Override = "wopi.override";
+
+        /// <summary>Authenticated user id (OpenTelemetry's standard <c>enduser.id</c> key).</summary>
         public const string UserId = "enduser.id";
     }
 
     /// <summary>Canonical strings for the <see cref="Tags.Outcome"/> dimension.</summary>
     public static class Outcomes
     {
+        /// <summary>Operation completed successfully (2xx).</summary>
         public const string Success = "success";
+
+        /// <summary>Target resource not found (404).</summary>
         public const string NotFound = "not_found";
+
+        /// <summary>Generic conflict (409) other than a lock mismatch.</summary>
         public const string Conflict = "conflict";
+
+        /// <summary>Lock conflict (409 with <c>X-WOPI-Lock</c>).</summary>
         public const string LockMismatch = "lock_mismatch";
+
+        /// <summary>Malformed or invalid request (400).</summary>
         public const string BadRequest = "bad_request";
+
+        /// <summary>Precondition failed (412).</summary>
         public const string PreconditionFailed = "precondition_failed";
+
+        /// <summary>Operation not supported by this host (501).</summary>
         public const string NotImplemented = "not_implemented";
+
+        /// <summary>Request rejected by WOPI proof-key validation (500 per spec).</summary>
         public const string ProofValidationFailed = "proof_validation_failed";
 
         /// <summary>Client disconnected / request aborted before completion. Not a server error.</summary>
         public const string Cancelled = "cancelled";
 
+        /// <summary>Unhandled server error (5xx).</summary>
         public const string Error = "error";
     }