feat(cleanup): plan-0061 phase 1 - pre-1.0 correctness and quality gate - #128
Merged
Conversation
Fixes the negative-seed guard gap in Compono.XunitV3/MSTest/NUnit's ComposeAttribute<TProfile> (a copy-paste gap predating PR #65's fix, using Compono.TUnit's already-correct behavior as the contract), lands a permanent CI-blocking Native AOT smoke gate for the existing eight AotSmokeTest projects (ADR-0041 Amendment 7), consolidates StableHash (3 copies)/GeneratorVersion (5 copies)/the ComposableAttribute metadata-name literal in the generator, adds the incremental-caching regression test ADR-0005 required, fixes mkdocs navigation and stale docs text, documents Compono.NSubstitute's AOT limitation and Compono.XunitV3.SampleTests' CI filter requirement, consolidates package-validation.yaml's publishable- package list to one job-level declaration, and reverts release-drafter's 0.x-preview breaking-change-to-minor override now that Compono is ready to leave 0.x (ADR-0031 Amendment 5). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
ncipollina
changed the title
global.json pins a preview 11.0.x SDK for the whole repo checkout - dotnet CLI resolution fails entirely (not just for net11.0-specific work) if that SDK isn't installed, exactly as pr-build.yaml/package-validation.yaml already install all four majors for the same reason. 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: f12371f5e3
ℹ️ 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".
… fails Temporary - PLAN-0061 Phase 1 requires proving the required aot-gate check actually fails an applicable leg's failure before merging it as a real gate. Reverted in the next commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
…aot-gate fails" This reverts commit 3a8331c.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
…ails Addresses Codex P1 finding on PR #128: aot-gate only inspected needs.aot-smoke.result, so a failed/cancelled `changes` job (checkout, git diff, or the applicability script erroring) produced the same `aot-smoke == skipped` conclusion as a legitimate zero-applicable-legs run, letting the required check report green without applicability ever being computed. aot-gate now checks needs.changes.result first, before anything else, and additionally rejects a skip whose legs output isn't literally "[]" - closing the gap without adding a job or changing the valid success/no-op paths. Validated all 6 required states locally. Also addresses Codex P2 finding: docs/plans/README.md's PLAN-0061 index row synced to the plan's current status (In Progress, not Codex's originally-correct-at-the-time Not Started suggestion, since Phase 1 completed since that review). Housekeeping: release-drafter.yml's comment corrected to not claim Compono has already left 0.x (it hasn't shipped 1.0 yet - the policy change only takes effect on the next breaking-change-labeled merge); resolver behavior itself unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
ncipollina
added a commit
that referenced
this pull request
Sep 3, 2026
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
ncipollina
added a commit
that referenced
this pull request
Sep 3, 2026
* fix(ci): PLAN-0062 - package-validation gap fixes 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 * docs(plan): mark PLAN-0062 Done - confirmed green on real CI 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 * docs(contributing): correct CS1591 enforcement guidance for PLAN-0062 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 --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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-0061 Phase 1 (pre-1.0 cleanup gate) - product correctness and repository quality gate. See
docs/plans/0061-pre-1-0-cleanup-and-consolidation.mdfor the full plan anddocs/adr/0041-aot-safe-row-binding-dispatch.md's Amendment 7 /docs/adr/0033-...Amendment 2 for the governing decisions.Compono.XunitV3/Compono.MSTest/Compono.NUnit'sComposeAttribute<TProfile>(copy-paste gap predating PR feat(xunitv3): add ComposeAttribute<TProfile, TConfig> for call-site profile configuration #65's fix) + regression tests, usingCompono.TUnit's already-correct behavior as the contract..github/workflows/aot-validation.yaml) per ADR-0041 Amendment 7 - no trigger-levelpaths:filter (avoids leaving the required checkPending), selectivity via an internal changed-files job,aot-gateas the always-resolving required check that also fails closed if applicability computation itself fails (Codex review finding, fixed).StableHash(3 copies),GeneratorVersion(5 copies), and theCompono.ComposableAttributemetadata-name literal inCompono.Generators.mkdocs.ymlnav fix (MSTest/NUnit reachable),docs.ymlstale-count fix,Compono.NSubstituteAOT-limitation doc,Compono.XunitV3.SampleTestsCI-filter documentation + stale-comment cleanup.package-validation.yaml's publishable-package list consolidated to one job-levelenv:(survives across its separaterun:steps, unlike a Bash array).release-drafter.yml: reverts ADR-0031's0.x-preview breaking-change→minor override now that Compono is ready to leave0.x(ADR-0031 Amendment 5) -breaking-changemaps tomajoragain. (Not yet shipped as 1.0 - this only changes what the next breaking-change-labeled merge resolves to.)Test plan
dotnet build Compono.slnx -c Release- 0 errorsdotnet test Compono.slnx -c Release- 3478/3478 passedCompono.Generators.Tests- 313/313 passed after StableHash/GeneratorVersion/ComposableAttribute consolidation (generated output unchanged)IncrementalCachingTestsempirically observes realIncrementalStepRunReasonvalues (verified it fails for the right reason before correcting the expected reason)Compono.XunitV3.SampleTestsfiltered invocation from its new README verified locally: 40/40 passCompono.MSTest.AotSmokeTest): realdotnet publish -p:PublishAot=true+ run, both rows PASSaot-validation.yaml- all 8 legs +aot-gatepass; deliberate one-leg failure proved the gate fails (aot-gate-> fail, other 7 legs unaffected), reverted before finalizingaot-gate's fail-closed logic (Codex P1 review finding) validated against all 6 required states locally before pushing, then confirmed green on real CIpackage-validation.yamlgreen on CIdocs.ymlgreen on CIReview
Both Codex findings addressed:
aot-validation.yaml):aot-gatenow checksneeds.changes.resultbefore anything else, so a failed/cancelled applicability computation can never be mistaken for a legitimate "zero applicable legs" skip.docs/plans/README.md): PLAN-0061 index row synced to the plan's current status (In Progress- Phase 1Done, Phase 2Not Started).🤖 Generated with Claude Code
https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA