fix(ci): plan-0062 package-validation gap fixes - #129
Merged
Conversation
Closes two real gaps RESEARCH-0021 found in package-validation.yaml and its scripts: - inspect-packed-nupkgs.sh's package loop was missing Compono.Logging entirely (10 of 11 packages checked) - added, with its actual package-specific invariants: build/+buildTransitive/ Compono.Logging.props in the file listing (no analyzer, unlike core Compono - generation lives in the shared Compono.Generators), and a new assert_dependency_range_per_tfm function for Microsoft.Extensions.Logging.Abstractions, the only third-party dependency in the package set whose Directory.Packages.props range is conditioned per TargetFramework (and is absent entirely for net11.0, satisfied by that TFM's own shared framework) - the existing assert_dependency_range's single-value lookup can't see this shape. New regression coverage added for the new function (pass, per-TFM mismatch, unexpected-net11.0-presence). - CS1591 was enforced via a redundant second full rebuild of all 11 packages in package-validation.yaml, even though Directory.Build.props already makes it an ordinary build warning for every project (GenerateDocumentationFile=true unconditionally). Directory.Build.targets now promotes it to an error scoped to $(IsPackable) != 'false' - the same 11 packages, verified against every non-packable project in the repo (all set IsPackable=false directly). The redundant workflow step is removed; dotnet pack itself now also fails on a missing doc comment, so no coverage is lost. Both changes independently validated: a full local pack+inspect of all 11 packages, a full solution build/test (3478/3478, 0 regressions), a positive CS1591 enforcement proof (temporarily undocumented member in Compono.Http failed an ordinary build/pack, reverted), and a non-packable-boundary proof (same in Compono.Samples.AspNetApi built clean with CS1591 remaining a warning, reverted). The NUnit compatibility-matrix applicability scoping RESEARCH-0021 also flagged is explicitly deferred, not included here - no demonstrated CI-duration problem justifies adding new change-detection machinery (package-validation.yaml's job is already ~3 minutes total, and removing the redundant CS1591 rebuild may shrink that further). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
package-validation.yaml: 2m58s post-fix vs 3m0s baseline (PR #128's last run) - the redundant CS1591 rebuild's removal and Compono.Logging's added pack+inspect roughly offset; no meaningful net change, no regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d973a8abf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses Codex P1 finding on PR #129: docs/contributing.md still told contributors to run `dotnet build -p:WarningsAsErrors=CS1591` separately and described it as "the same gate that runs in CI" - stale since PLAN-0062 moved enforcement into Directory.Build.targets. An ordinary `dotnet build Compono.slnx` now enforces it directly for publishable packages; no extra flag needed, and the dedicated package-validation rebuild step no longer exists. Verified no other current-facing doc, skill, or contributor instruction (root CONTRIBUTING.md, the engineering-workflow skill's own contributing.md reference) still describes the old invocation - only historical, already-Done plan records and the RESEARCH-0021 record itself mention it, correctly, as past-tense history. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
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
1 participant
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.
Summary
PLAN-0062 (package-validation gap fixes) - a follow-up discovered during RESEARCH-0021's post-PLAN-0061 audit, not part of PLAN-0061's own scope. See
docs/plans/0062-package-validation-gap-fixes.mdfor the full plan anddocs/research/0021-package-validation-execution-policy-audit.mdfor the underlying research.Two real gaps closed:
inspect-packed-nupkgs.shwas missingCompono.Loggingentirely (10 of 11 packages checked). Added, with its actual package-specific invariants (ownbuild/+buildTransitive/props, no analyzer entry; a newassert_dependency_range_per_tfmfunction forMicrosoft.Extensions.Logging.Abstractions, the only third-party dependency whose range is conditioned per-TFM and absent for net11.0) - not merely added to a loop.package-validation.yaml, intoDirectory.Build.targets(scoped$(IsPackable) != 'false') so an ordinarypr-build.yamlbuild (anddotnet packitself) already catches it - earlier feedback, less CI cost, no coverage loss.Also includes
docs/research/0020-repository-wide-comment-quality-audit.md- a companion audit that found the repository's comments already conform to its own stated policy; no cleanup warranted there (Tier 3, no code changes).Explicitly not included: NUnit compatibility-matrix applicability scoping (RESEARCH-0021's other Tier 2 candidate) - deferred, since there's no demonstrated CI-duration problem to justify new change-detection machinery on an already ~3-minute job.
Test plan
inspect-packed-nupkgs.tests.sh- all pass, including newassert_dependency_range_per_tfmregression coverage (pass / per-TFM mismatch / unexpected-net11.0-presence)inspect-packed-nupkgs.shrun against all 11 publishable packages - all pass, includingCompono.Logging's new assertionsdotnet build Compono.slnx -c Release- 0 errors, same 40 pre-existing warnings baseline (no new CS1591 regressions anywhere)dotnet test Compono.slnx -c Release- 3478/3478 passedCompono.Http(a real packable package) - confirmed bothdotnet buildanddotnet packfail with a realCS1591error; revertedCompono.Samples.AspNetApi(non-packable, non-test, no localNoWarn) - confirmed the build succeeds,CS1591remains an ordinary warning, never promoted to error; revertedpackage-validation.yamlCI run on this PR - confirming the removed step is gone with no coverage loss,Compono.Loggingis now inspected, and everything else stays green🤖 Generated with Claude Code
https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA