chore(api): check the packaged API against the release on nuget.org, not just against a file in the PR - #705
Conversation
…not just against a file in the PR PR #680 checked the public surface into PublicAPI.Shipped.txt and let RS0016/RS0017 fail the build when the code and that file disagree. Both files are part of the PR, though, so removing a public member and its entry in one change still compiles green - which is the exact case ADR 0002 cares about. Turn on EnablePackageValidation with the baseline pinned to 1.7.0, the last version on nuget.org, and pack the library in CI so ApiCompat compares the packaged assemblies for both target frameworks against that published package. A PR cannot edit the baseline. CP0003 is suppressed: it compares assembly versions, and the version is stamped only by release.yml from the git tag, so every other build carries the SDK's 1.0.0 placeholder and CP0003 fires on changes that touch no API at all. The CI step deliberately omits --no-build, which skips the ApiCompat targets outright and makes the pack validate nothing; a test asserts it stays absent. Closes #636 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoValidate packaged API against the latest NuGet release
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…est release Qodo review, round 1. The claim that an out-of-date baseline "only makes the check stricter" was wrong, and it was repeated in the csproj comment and in CONTRIBUTING.md. ApiCompat can only report a member the baseline package actually contains, so an old baseline is a *narrower* check: everything added since the pinned version sits outside the comparison and could be removed - together with its PublicAPI.Shipped.txt entry - with both guards still green. Correct the claim in both places, and add a CI step that resolves the newest published version from the nuget.org index and fails when the pinned baseline is behind it, so forgetting the bump after a release is loud rather than silent. Verified by pinning 1.6.0 against the published 1.7.0: exit 1 with the ::error:: annotation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 1 finding accepted — it was a genuine logic error, not just a wording problem. I had claimed in two places (the Fixed in 526b554:
Verified by pinning the baseline to 1.6.0 against the published 1.7.0: the step exits 1 with the One deliberate consequence worth flagging: this is a failure, not a warning, so CI goes red on every PR between a release being published and the baseline bump landing. That is the intent — the bump PR itself is green, and the alternative is a check that quietly covers less over time. Full suite green on net9.0 + net10.0 after the change: 4152 + 217 per framework, 0 failed. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 526b554 |
…rd test bite Qodo review, round 2. Two findings, both valid. A published prerelease would have deadlocked the two halves of this change against each other. release.yml accepts prerelease tags, the flat-container index lists them, and the freshness check took the last entry verbatim - so publishing 1.8.0-beta.1 would have demanded a baseline that CoreProject_PinsThePackageValidationBaselineToAPublishedVersion rejects, since Version.TryParse does not accept SemVer prerelease strings. Neither keeping the baseline nor bumping it could pass. The query now filters prereleases out: the baseline is always the newest stable release, which is what a consumer restores by default, and which is parseable. Also guard an empty result. Ci_FailsWhenTheBaselineHasFallenBehindTheLatestRelease asserted only that two strings appeared somewhere in ci.yml, so deleting the comparison or the `exit 1` left it green - the exact silent disablement it exists to catch. It now reads the named step's own `run:` block and asserts each part the enforcement rests on. Verified by replacing both `exit 1` lines with an echo: the test fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 2 — both findings accepted, fixed in 5d31439. 1. Prerelease baseline cannot pass. Correct, and it was worse than a mismatch: the two halves of this change would have deadlocked against each other. The query now filters prereleases out ( Verified against a simulated index 2. CI guard tests only strings. Also correct — asserting that two strings appear somewhere in the file would have stayed green with the comparison or the It now locates the step by name, extracts its own I stopped short of executing the shell from the test. That would mean spawning bash on all three matrix legs for a build-configuration guard, and this file is explicit that its reach is deliberately shallow rather than a second, worse copy of the thing it guards. Scoping the assertions to the step and to the load-bearing tokens closes the gap you identified without that cost. Full suite green on net9.0 + net10.0: 4152 + 217 per framework, 0 failed. CI was green on 526b554 including the new pack and baseline steps (the pack step runs in ~6s). |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 5d31439 |
|
Qodo-clean, CI green — ready for review. Verified against head Re-validated against current |
|
Negative control: the gate actually fails. Verified independently on Mutation — the exact scenario #636 describes: deleted the shipped public parameterless ctor Result — Compared against the real published One discrepancy, non-blocking. The comment on |
1 participant
What was wrong
ADR 0002 promises source compatibility for
Daqifi.Core's public API. PR #680 made that promise visible: the surface is checked in asPublicAPI.Shipped.txt, and RS0016/RS0017 fail the build whenever the code and that file disagree.But both files travel in the PR. Delete a public member and its line in
PublicAPI.Shipped.txtin the same change and the build is green — which is precisely the shape of a break ADR 0002 is meant to catch. A deliberate break and an accidental one look identical to the build, and the accidental one reaches a consumer (daqifi-desktop,Daqifi.Mcp, an external integrator) on their next restore.I confirmed this on main before changing anything: narrowing
DeviceMetadata.IpAddress's setter tointernaland deleting its one line fromPublicAPI.Shipped.txtbuilds clean, 0 warnings, 0 errors.How it's fixed
Compare against something the PR cannot edit: the package already on nuget.org.
Daqifi.Corenow setsEnablePackageValidationwithPackageValidationBaselineVersionpinned to 1.7.0, the last published version, and CI packs the library so ApiCompat compares the packaged assemblies — bothnet9.0andnet10.0— against that download. The same removal above now fails withCP0002on both frameworks.That leaves the source analyzer as the fast, precise, everyday check and package validation as the backstop that answers "does this still work for someone who already depends on 1.7.0?"
A second CI step keeps the baseline honest: it resolves the newest stable version from the nuget.org index and fails when the pinned baseline is behind it. Without that, the check quietly narrows over time — ApiCompat can only report a member the baseline package actually contains, so everything added since the pinned version would fall outside the comparison entirely.
Things a reviewer may want to push back on
CP0003is suppressed. It compares assembly versions, and this repo carries none — the version is stamped only byrelease.ymlfrom the git tag. Every other build ships the SDK's1.0.0placeholder, soCP0003reports "lower than the 1.7.0 baseline" on every build, including ones that change no API at all. Suppressing it keeps the rules that carry signal here (CP0002,CP1002) readable. Version ordering is decided upstream by which tag the release is cut from.The gate is in
ci.yml, notrelease.yml. Package validation only runs insidedotnet pack, and--no-buildskips the ApiCompat targets entirely — verified: with--no-buildthe pack is silent on a break that otherwise producesCP0002.release.ymlpacks--no-buildon purpose, so its assemblies keep the version stamped by the earlier build step; removing that flag would republish the assemblies as1.0.0.0. So the PR/main gate is the right place, and everything reaching a release tag has passed it.PublicApiTrackingTestsasserts the CI pack step never grows a--no-build.The baseline-freshness check is a failure, not a warning. After a release is published, CI goes red on every PR until the baseline bump lands. That is the intent — the bump PR itself is green, and the alternative is a check that silently covers less over time. The bump belongs in the same change that moves
Unshippedentries intoShipped.Prereleases are excluded when deciding what is "newest".
release.ymlaccepts prerelease tags and the index lists them, but a prerelease is not what a consumer restores by default, so it must not become the version every other PR is required to match.It costs one extra compile.
dotnet packdefaults to Release, so the step rebuildsDaqifi.Corerather than reusing the Debug output. One project, on the ubuntu leg only — the packaged shape is identical on all three. Measured at ~6s in CI.Verification
internaland itsPublicAPI.Shipped.txtline deleted →dotnet buildgreen,dotnet packred withCP0002onnet9.0andnet10.0.RS0016; a narrowed setter →RS0017.::error::annotation. Prerelease filter checked against a simulated index and the live one.No bench run — this is a build-configuration change with no device path.
Closes #636
Not merging — for review.
🤖 Generated with Claude Code