feat(xunitv3): add ComposeAttribute<TProfile, TConfig> for call-site profile configuration - #65
Conversation
…profile configuration Compono.XunitV3's [Compose<TProfile>] can only select a fixed, default-constructed profile - real migration evidence from a much larger AutoFixture test suite (RESEARCH-0002) showed no clean way for a compile-time-constant value known at a test's call site to reach configuration logic running inside a profile (e.g. a parameterized PersistenceAutoData(repositoryName)-shaped custom AutoDataAttribute). Adds ComposeAttribute<TProfile, TConfig>: profile configuration arguments bind positionally to TConfig's single public constructor (reusing the existing inline-value validation), TProfile is built from that TConfig via its own single qualifying constructor, and the result flows through the already-existing, unchanged AddProfile(ICompositionProfile) core API - zero changes to core Compono. Both constructor-shape checks are deterministic, cached-once-per-attribute-instance runtime diagnostics (no "best match" heuristic), a documented tradeoff against [Compose<TProfile>]'s compile-time new() enforcement. Also documents CompositionProviderRequest.Name-based provider matching (a separate, previously-undocumented capability RESEARCH-0002 flagged as a real migration-guide gap) and updates the published compono skill/evals so an agent recognizes the parameterized-AutoDataAttribute migration pattern instead of recommending a combinatorial subclass explosion, an inline Composer.Create escape hatch, or invented ambient scenario state. See ADR-0036 and PLAN-0036 for the full design/evaluation record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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: 68975407a3
ℹ️ 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".
… discovery Codex review on PR #65 caught that ComposeMethodDiscovery was only registered against the non-generic and one-type-parameter ComposeAttribute metadata names - ComposeAttribute<TProfile,TConfig>'s own arity-suffixed metadata name (ComposeAttribute`2) was never registered with the generator, so a concrete parameter type reached only through [Compose<TProfile, TConfig>] got no generated ICompositionPlan<T> at all and would fail at GetData time. The packaged sample didn't catch this because its only composed parameter type was a registered string, which never needs a generated plan. Adds a third ForAttributeWithMetadataName registration for the arity-2 metadata name, merged into the same discovery pipeline as the other two arities. New isolated Compono.Generators.Tests snapshot proves a concrete type reached only this way now gets a plan; Compono.XunitV3.SampleTests' ConfigProfileTests now composes a real concrete class instead of a bare string, so the packaged sample actually exercises the fixed path. Also corrects docs/roadmap/post-mvp.md, which listed ADR-0036's shipped capability as an outstanding candidate - contradicting the page's own stated "not fully available" scope (docs/roadmap/index.md). Returned to a no-current-candidates state; the historical evidence trail stays intact via ADR-0036/RESEARCH-0002/PLAN-0036. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c1b59d3c3
ℹ️ 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".
… binding Second Codex review round on PR #65 caught two more real gaps in ComposeAttribute<TProfile, TConfig>'s config/profile binding: - ApplyProfile runs inside the base class's Lazy<Composer> initialization, before GetData ever creates a CompositionRow, so a ConfigProfileBinder failure had no row.Seed to read and escaped without the "Seed: ..." suffix every other Compono.XunitV3-owned pre-composition failure carries (ADR-0022). ApplyProfile now catches CompositionException and rethrows via the existing CompositionException.WithSeedInMessage helper. - An abstract TConfig/TProfile with an otherwise-qualifying public constructor passed the "exactly one constructor" check and then threw MemberAccessException from ConstructorInfo.Invoke instead of the documented CompositionException. Both constructor-resolution methods now explicitly reject abstract types first. Regression tests added for both; documented in docs/troubleshooting/common-errors.md. Full solution: 903/903 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c711c16e0
ℹ️ 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".
…exceptions Third Codex review round on PR #65: - A negative configured seed (Seed = -1) combined with an invalid config/profile shape reported the binder failure with "Seed: -1" embedded instead of the documented negative-seed diagnostic. ApplyProfile now checks for a negative SeedAsNullable before attempting any config/profile binding, reusing the base class's exact negative-seed message (AppendSeed promoted to private protected so both share it). - ConstructorInfo.Invoke wraps any exception a TConfig/TProfile constructor itself throws in TargetInvocationException, so ApplyProfile's catch (CompositionException) never observed a constructor's own actionable exception. ConfigProfileBinder's shared Invoke helper now unwraps TargetInvocationException via ExceptionDispatchInfo.Capture(...).Throw(), preserving the original stack trace, for both the TConfig and TProfile construction call sites. Regression tests added for both. Full solution: 909/909 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5c44ebf5e
ℹ️ 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".
Fourth Codex review round on PR #65: - ADR-0036's "Reflection is bounded and cached" section specified the same cached-delegate (MakeGenericMethod/Delegate.CreateDelegate) shape ADR-0022 uses for RowInvokers - the actual implementation uses direct ConstructorInfo.Invoke instead, relying on the base attribute's existing Lazy<Composer> caching for the "once per attribute instance" guarantee. Per this repo's Amendment mechanic, that correction now has its own dated Amendment 1 on the ADR, not just a plan-side note. - ComposeAttribute's XML remarks still described ComposeAttribute<TProfile> as "the one designed extension point," and Compono.XunitV3.csproj's package Description listed only [Compose]/[Compose<TProfile>]. Both updated to describe the new ComposeAttribute<TProfile, TConfig> form; API reference regenerated from the corrected XML docs. A third finding (move the CompositionProviderRequest.Name migration-guide section to a separate PR) was reviewed and deliberately not actioned - including it in this PR was an explicit user instruction when this plan was scoped, not an oversight; replied on the thread explaining this. Full solution: 909/909 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dc47deaa3
ℹ️ 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".
…ge, fix CI gate Fifth Codex review round on PR #65, plus a live CI regression this round's own fix introduced: - ConfigProfileBinder independently reimplemented the exact null/ Nullable<T>-unwrap/assignability check ComposeAttribute.GetData's own inline-value binding already had, contradicting ADR-0036's "retargeted rather than reimplemented" promise. Extracted both into a new shared Binding/PositionalArgumentBinder.cs, used by both call sites. - No regression test covered a non-null value-typed config argument against a Nullable<T> constructor parameter (e.g. 42 for int?). Added fixtures and a dedicated test. - BindingPlan's Compose-family-stacking diagnostic still named only the original two attribute forms. Updated the message and added regression coverage combining the new form with [Compose]. - Live CI regression (caught by the user via the package-validation workflow, not by Codex): that workflow's "Local-feed packed-consumer smoke test" step runs Compono.XunitV3.SampleTests directly, filtered to exclude only FailingCompositionTests by exact name. The new ConfigProfileTests.cs's own deliberately-failing test lived inside an otherwise-green class, so it wasn't excluded and failed the gate. Moved it into its own FailingConfigProfileTests class (mirroring FailingCompositionTests' pattern) and changed the workflow filter to a trailing wildcard covering the whole naming convention, verified directly against the MTP CLI (a mid-string wildcard is rejected). Full solution: 913/913 passed. Verified the corrected package-validation smoke-test command locally: 20/20 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df7c8122c
ℹ️ 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".
Sixth Codex review round on PR #65: docs/packages/index.md's top-level package catalog table and skills/compono/SKILL.md's Detection table and reference table still enumerated only [Compose]/[Compose<TProfile>] - round 4 fixed the base attribute's XML docs and the NuGet package description but missed these. Fixed all three, then grepped the whole repo's markdown for the same stale pattern to confirm nothing else was missed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d31a7fc2b
ℹ️ 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".
Seventh Codex review round on PR #65: as a Done plan, PLAN-0036's Test Plan summary, Critical Files, and Verification-checklist sections are the record maintainers rely on, but still described the pre-review-round state - stale test counts (893/891 vs. the actual final 913), a "left unchanged by design" claim round 6's own fix reversed, an "excluded from CI" claim already corrected in code comments but not here, and a "no BindingPlan.cs changes" claim round 5's own diagnostic-message fix quietly contradicted. Edited those sections directly to the final, true state rather than adding another dated note on top of six already-accumulated ones - this is a living plan document, not an ADR, so a correction here is a direct edit. The round-by-round Notes entries stay as the historical trail; only the summary sections a reader checks first for final state needed refreshing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Real migration evidence from a much larger AutoFixture test suite (
RESEARCH-0002,ncipollina/trivia-platform) surfaced a genuine Compono capability gap:[Compose<TProfile>]can only select a fixed, default-constructed profile, so a custom AutoFixture attribute whose constructor arguments drive customization logic (e.g.PersistenceAutoData(repositoryName), ~45 real call sites) had no clean migration path — only a combinatorial explosion of profile subclasses or a hand-builtComposer.Create(...)per test.This adds
ComposeAttribute<TProfile, TConfig>: profile configuration arguments bind positionally toTConfig's single public constructor (reusing the existing inline-value validation),TProfileis built from thatTConfig, and the result flows through the already-existing, unchangedCompositionBuilder.AddProfile(ICompositionProfile)— zero changes to coreCompono. SeeADR-0036for the full deep-design record (four alternatives evaluated) andPLAN-0036for the closed-out implementation plan.Changes
src/Compono.XunitV3: newComposeAttribute<TProfile, TConfig>+Binding/ConfigProfileBinder(deterministic constructor-shape resolution — exactly one public constructor onTConfig, exactly oneTConfig-accepting public constructor onTProfile; no "best match" heuristic, all reflection bounded/cached to once per attribute instance via the existingLazy<Composer>).ComposeAttribute.cs's inline-value normalization extracted into a shared helper (behavior-preserving) rather than duplicated.Compono.XunitV3.Testscases (valid binding, both constructor-shape failure modes, argument count/type/nullability mismatches, inline-value isolation, caching) + a packaged-consumer proof inCompono.XunitV3.SampleTests(enum-based config, differently-configured profiles, a deliberately-failing constructor-shape case) run against the real packed NuGet, not a project reference.compono-xunitv3.md(new "Profile configuration arguments" section),migrating-from-autofixture.md(parameterized-AutoDataAttributemigration section, drawn from the real evidence — plus a separately-flagged doc gap closed:CompositionProviderRequest.Name-based provider matching, previously undocumented),troubleshooting/common-errors.md,how-to/use-profiles.md, regenerated API reference.skills/compono: updated so an agent recognizes this migration pattern and reaches for[Compose<TProfile, TConfig>]with strongly-typed (enum/typeof) arguments instead of recommending a subclass explosion, an inline escape hatch, ambient scenario state, or AutoFixture retention — new eval (skills/compono-evals/evals.jsonid 19) exercises exactly that.Validation
dotnet build/dotnet test(Compono.slnx): 0 warnings, 0 errors, 893/893 passed.Compono.XunitV3.SampleTests(deliberately excluded fromCompono.slnx/CI — packaged-consumer verification, run manually): both new passing theories confirm differently-configured profiles from different enum arguments; the deliberately-failing case fails with the exact designed diagnostic, stack-traced through the packaged assembly, before the test body ever runs..github/scripts/generate-api-reference.shagainst a Release build — deterministic diff, only the new type's pages plus the two pages that reference it changed.Notes for Reviewers
PLAN-0036's own Notes section records two implementation-time corrections worth a quick look: noBindingPlan.cschanges were actually needed (the existingLazy<Composer>caching already gives the "once per attribute instance" guarantee for free), and the new diagnostics landed introubleshooting/common-errors.mdrather thanreference/diagnostics.mdonce that file's actual scope (compile-timeCMPcodes only) was checked directly.