fix(parameters): make nullable-enum attribute overrides settable (issue #57 expanded) - #63
Conversation
…655 dead code) Nullable enums are not legal attribute named arguments, so four override properties were impossible to set from user code and always resolved to null: - ReplOptionAttribute.CaseSensitivity - ReplOptionAttribute.Arity - ReplEnumFlagAttribute.CaseSensitivity - ReplValueAliasAttribute.CaseSensitivity Each now exposes a non-nullable public property backed by a nullable field, with the unset state surfaced through internal *Override properties consumed by OptionSchemaBuilder. Behavior tests exercise each override end-to-end (RED observed as CS0655 before the fix). Fixes #57
…override A token matching both a case-insensitive alias and another option's case-sensitive canonical token — reachable now that the per-option case-sensitivity override is settable — is rejected as 'Ambiguous option' at parse time. Documents the behavior flagged during the #55 review as previously unreachable.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Focused review of exact head 851ddf2194bf21af09b6ee328c94085c5f959e53: fix before merge. The nullable-enum backing-field design makes the four attribute named arguments legal and the added end-to-end tests pass, but two newly reachable contract paths remain incorrect:
- collision validation ignores per-entry effective casing and rejects distinguishable case-sensitive tokens under a globally case-insensitive configuration;
ReplArity.OneOrMoredoes not enforce its lower bound when the option is absent, so binding/handler execution is reached without any value.
Both are independently reproduced in the existing threads. Validation otherwise passes: strict Release build with 0 warnings/errors, focused override tests 6/6, full solution 1,284 passed with one expected external MCP Inspector skip, all GitHub checks green, git diff --check clean, detached worktree clean. No additional material architecture, security, operability, style, or public-contract finding beyond those two.
…parity, global-options fail-fast - Expose *Override read-only properties publicly so external tooling can distinguish an unset override from an explicit enum-default assignment; XML docs on the non-nullable getters state they do not reflect effective behavior when unset. - ValidateEnumConflicts now falls back to the global case sensitivity for unset per-option overrides (was hardcoded case-insensitive), aligning with every other effective-case resolution site. Reachable via the now-settable ZeroOrMore arity override (RED observed). - UseGlobalOptions<T> fails fast (NotSupportedException) when a property declares a CaseSensitivity/Arity override, instead of silently discarding it (RED observed). - Coverage: unset-override states (collection arity, global-insensitive inheritance), explicit enum-value-zero override, options-group property path (both overrides), tightened unknown-option assertion.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…r bound (review) - ValidateTokenCollisions decides collisions per pair under each entry's effective case sensitivity: ordinal-equal tokens always collide; tokens differing only by casing collide only when both entries are effectively case-insensitive. Two explicitly case-sensitive options differing only by case now register under a global case-insensitive default (RED observed). - HandlerArgumentBinder enforces the OneOrMore lower bound after both the named and positional binding attempts come up empty — option parsing alone cannot see positional consumption. Handler is no longer invoked with a missing required-repeatable value (RED observed); positional satisfaction is guarded by a boundary test. - ResolveParameterArity moved onto OptionSchema for reuse by the binder.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Focused convergence review of exact head 07edb22b53f14371558e86a7493ae4be98d9460a: fix before merge.
Verified fixed from the previous head:
- two explicitly case-sensitive tokens differing only by casing now register under a globally case-insensitive default;
- an absent handler parameter with
OneOrMorenow fails before handler invocation, while positional values still satisfy it.
Four directly related contract paths remain reproducible:
ExactlyOneis not treated as a required arity after named and positional binding fail;- required arities are not enforced for options-group properties;
- typed global options still silently accept unsupported
ReplValueAlias/ReplEnumFlagcase overrides; AllowUnknownOptionscan rebind a casing-mismatched token to an explicitly case-sensitive known option.
Exact safe reproductions are posted in the existing threads. Validation otherwise passes: strict Release build with 0 warnings/errors, focused override tests 16/16, full solution 1,294 passed with one expected external MCP Inspector skip, all GitHub checks green, git diff --check clean, detached worktree clean.
…e, honest diagnostics, dead Position removed - OptionSchemaParameter carries ExplicitArity so explicit lower bounds (OneOrMore, ExactlyOne) reject absence on ALL binding paths: handler parameters, options-group properties, and ArgumentOnly parameters (previously silently dropped — no schema entry). Inferred arities keep their historical behavior. - Lower-bound diagnostics name the canonical option token, falling back to the parameter name for ArgumentOnly; ResolveDisplayToken added beside ResolveParameterArity on OptionSchema (explicit loop, no closure). - Token-collision message names the command being registered; duplicate parameter/property names get a duplicate-name diagnostic instead of the misleading token-collision wording. - KnownTokens dedupes ordinally so case-distinct tokens of case-sensitive twins both survive for 'Did you mean' suggestions. - UseGlobalOptions<T> fail-fast extended to value-alias CaseSensitivity overrides, with remediation hints; enum-flag overrides deliberately excluded (declared on possibly-shared enum types) with rationale. - ReplArgumentAttribute.Position removed: unsettable (CS0655, nullable int) AND never consumed — doubly dead public API; removal sanctioned pre-stable. Follow-up issue tracks real explicit-position support. All behavior changes RED-observed before the fix (8 failing tests).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…-override bypass (review) - Explicit ZeroOrOne/ExactlyOne upper bounds now apply to positional consumption on both the handler-parameter and options-group paths: a collection consuming several positionals under an explicit cap fails with the same diagnostics as the repeated named option (RED observed). - The permissive unknown-option fallback (AllowUnknownOptions) no longer rebinds a casing-mismatched token to a parameter whose effective case sensitivity is CaseSensitive — the token stays an inert unknown, while unconstrained parameters keep the documented bind-by-name behavior (RED observed; boundary test guards the preserved behavior).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c490a47754
ℹ️ 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".
…requiredness (review) - The permissive-fallback guard now also drops tokens rejected by an EXPLICITLY CaseSensitive option even when the rejected token ordinally equals the CLR parameter name (renamed-token edge: Name differing from the parameter name only by casing). Unconstrained parameters keep the documented bind-by-name behavior (RED observed). - Documentation export reports options with an explicit OneOrMore/ExactlyOne arity as required — they now fail binding when omitted — on both the handler-parameter and options-group paths (group options were hard-coded as not required) (RED observed x2).
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Approved exact head adeee5d592bb463f5228afbf34ebf62f63e55330. Final convergence pass independently replayed the original casing-collision and required-arity reproducers plus ExactlyOne, options-group lower bounds, named/positional upper-bound parity, typed-global value-alias fail-fast, permissive unknown-token rebinding (including renamed tokens), and documentation requiredness. All material paths converge. Strict Release build passed with 0 warnings/errors; focused override suite 34/34; full solution 1,312 passed with one expected external MCP Inspector skip; all GitHub checks green; diff/worktree clean. The remaining typed-global enum limitation and case-distinct discovery rendering are explicitly tracked as non-blocking follow-ups in #64 and #67.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adeee5d592
ℹ️ 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".
| // Explicit lower bounds can only be enforced here, after BOTH the named and the | ||
| // positional binding attempts came up empty — option parsing alone cannot see values | ||
| // the parameter would have consumed positionally. | ||
| ThrowIfExplicitLowerBoundUnsatisfied(context.OptionSchema, parameterName); |
There was a problem hiding this comment.
When an explicit required option has a type registered in DI, invocation with no named or positional value returns from the earlier implicit service lookup before this lower-bound check is reached, so [ReplOption(Arity = ReplArity.ExactlyOne)] string value can run with a registered string service instead of failing as absent. This bypasses the new ExactlyOne/OneOrMore guarantee specifically for user-option parameters whose type is also registered as a service; the required-arity check needs to run before implicit service fallback, or service fallback needs to be skipped for explicit required options.
Useful? React with 👍 / 👎.
| var explicitlySensitive = knownParameter.CaseSensitivity == ReplCaseSensitivity.CaseSensitive; | ||
| var effectivelySensitive = | ||
| (knownParameter.CaseSensitivity ?? options.OptionCaseSensitivity) == ReplCaseSensitivity.CaseSensitive; |
There was a problem hiding this comment.
When AllowUnknownOptions is true with a global case-insensitive parser, this guard only looks at the parameter-level case override, so a casing-mismatched token rejected by a case-sensitive [ReplValueAlias] or [ReplEnumFlag] can still be stored by CLR parameter name. For example, [ReplOption(Name = "env")] [ReplValueAlias("--mode", "prod", CaseSensitivity = ReplCaseSensitivity.CaseSensitive)] string mode lets --Mode value bind mode instead of staying inert, bypassing the alias-level override that was just made settable.
Useful? React with 👍 / 👎.
2 participants
Summary
Nullable enums are not legal attribute named arguments (CS0655), so four override properties were impossible to set from user code and always resolved to
null— dead public API. Issue #57 identifiedReplOptionAttribute.CaseSensitivity; triage for the 0.11 stable release confirmed the same defect on three more properties:ReplOptionAttribute.CaseSensitivityReplOptionAttribute.ArityReplEnumFlagAttribute.CaseSensitivityReplValueAliasAttribute.CaseSensitivityFixing this after a stable release would change the public property types (source-breaking), hence landing it before cutting
release/0.11.0.Fix
Each property now exposes a non-nullable public property backed by a nullable field, preserving the optional-override semantics: only an explicit assignment overrides the global/inferred default. The unset state is surfaced through public read-only
*Overrideproperties, consumed byOptionSchemaBuilder.Follow-up commits harden the newly reachable paths (from adversarial review): pairwise token-collision validation under effective case sensitivity, explicit lower-bound (
OneOrMore/ExactlyOne) enforcement across all binding paths, enum-conflict case parity, typed-global-options fail-fast, honest diagnostics (canonical token, command context, duplicate-name wording), and ordinalKnownTokensdeduplication.Breaking changes (0.11.0 release notes)
ReplOptionAttribute.CaseSensitivity/.Arity,ReplEnumFlagAttribute.CaseSensitivity,ReplValueAliasAttribute.CaseSensitivityare now non-nullable; new read-only*Overrideproperties expose the unset state.ReplArgumentAttribute.Positionremoved: it was both unsettable (CS0655, nullableint) and never consumed — doubly dead API. Explicit positional ordering is tracked as a follow-up feature.OneOrMore/ExactlyOnearity now enforced: an option declaring an explicit lower bound and receiving no value (named or positional) fails binding with a diagnostic instead of silently binding a default. Inferred arities are unaffected.UseGlobalOptions<T>fails fast (NotSupportedException) when a property declares aCaseSensitivity/Arityoverride or a value-aliasCaseSensitivityoverride — previously a silent no-op (unreachable before this fix).Test Plan
Given_OptionAttributeOverridescovering each override, unset states, explicit enum-value-zero, options-group and ArgumentOnly paths, lower AND upper bounds on both named and positional paths, permissive-mode override protection (including the renamed-token edge), doc-export requiredness, collision semantics, diagnostics wording, and suggestion casing fidelity.dotnet test --solution src/Repl.slnx -c Release— 1312 passed, 1 skipped (known external-toolchain smoke), 0 failed.Fixes #57
🤖 Generated with Claude Code