feat: add fluent hidden options#77
Open
autocarl wants to merge 63 commits into
Open
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
dorny/test-reporter creates a check run, which requires `checks: write`. GitHub caps GITHUB_TOKEN to read-only for `pull_request` events raised from a fork, so the step failed with "Resource not accessible by integration" and cascaded into skipping Pack, Validate Repl meta-package, Resolve version and Upload packages — fork PRs never ran packaging validation at all. - gate the step on the PR head repo matching the base repo - keep it running on push builds, where `github.event.pull_request` is null Fork PRs still gate on the Test step and publish the .trx artifact.
The completion emitter separates candidates with '\n' but flushes the final line with Environment.NewLine, so on Windows the last candidate carries a trailing '\r'. Splitting on '\n' alone left that CR attached, so the last candidate never compared equal to its expected value. When_CompletingGlobalPrefixWithHiddenCustomOption_Then_HiddenTokensAreNotSuggested registers `-r` as an alias, which sorts after every `--` token and therefore lands last, so it was the one candidate the assertion could not match. The test passed on the Linux and macOS CI legs, where Environment.NewLine is '\n' and RemoveEmptyEntries absorbs the difference. Split with TrimEntries, matching the existing idiom in Given_OutputFormatting.
The matrix covered only ubuntu-latest and macos-latest, and build-test-pack runs on ubuntu, so the whole pipeline had zero Windows coverage. That gap let a CRLF-only assertion failure ship green on every leg. Windows is a first-class target for a CLI/REPL framework, and the shell scripts are already pinned to LF via .gitattributes, so the new leg needs no per-OS branching.
…he docs docs/commands.md claimed hidden options "remain valid parser inputs and bind normally when supplied explicitly". That is true on the command line and in the REPL, but false over MCP: McpSchemaGenerator and McpToolAdapter's argument allow-list are both built from ReplDocCommand.Options, so removing a hidden option from the documentation model withdraws it from tools/list AND tools/call at once. A hidden option gets the same hard block as a hidden command. - add a characterization pin asserting tools/call rejects a hidden option and the handler does not run - state the MCP behaviour in docs/commands.md instead of contradicting it The pin deliberately does not assert the adapter's own diagnostic: the SDK replaces it with a generic "An error occurred invoking '<tool>'." before it reaches the client, which is the right outcome here — a caller probing for a hidden option learns nothing from the failure. Verified with a mutation check: removing the filter in DocumentationEngine makes both this pin and the pre-existing schema-omission test fail.
Visibility lived on the mutable CommandBuilder, so every discovery surface had to remember to re-test the predicate. Ten sites did; a new one would not have to, and the typo-suggestion path already did not. Move IsHidden onto OptionSchemaParameter and let OptionSchemaBuilder populate it from the ReplOptionAttribute it already holds. That deletes CommandBuilder.CollectAttributeHiddenOptions outright — a second reflection walk over handler parameters and options-group properties, duplicating OptionSchemaBuilder with divergent rules — together with its UnconditionalSuppressMessage(IL2075), whose justification was wrong: a delegate reference does not preserve properties of parameter types under trimming. The surviving options-group walk goes through a DynamicallyAccessedMembers-annotated parameter, which is the established pattern here. Add one shared projection the surfaces consume instead of a local predicate: - OptionSchema.DiscoverableParameters absorbs both the ArgumentOnly and the hidden filter, so help asks one question instead of two - OptionSchema.DiscoverableEntries covers every token of a hidden option — aliases, value aliases, negated flags — not just its canonical form - OptionSchema.IsOptionHidden for the sites that already hold a resolved target OptionTokenCompletionSource loses two overloads and its Func<> parameter: the file whose own doc comment calls it the single source of option-name candidates "so the two surfaces can never drift" was taking a caller-supplied drift vector. It is now one line shorter than on origin/main, having absorbed the feature. Fluent calls after Map publish a new schema through CompareExchange rather than mutating one. Entries are reused verbatim, so parsing and binding observe an identical contract — pinned by a test asserting KnownTokens, resolved arity and the Entries reference are unchanged across a visibility swap. No lock: a blocking wait on another managed thread deadlocks on single-threaded WASM. RouteDefinition reads the schema through the builder instead of snapshotting it, so an already-cached routing graph observes a later change with no cache plumbing. Behaviour-preserving: 1336 tests green, 0 warnings under -warnaserror.
Derived discovery state is rebuilt only when routing is invalidated. Map does that, but the visibility setters did not, so anything captured from Map could retract a command or an option while a live MCP snapshot kept advertising it — help was current, tools/list was stale, and the two disagreed about what exists. - pass the app's InvalidateRouting into CommandBuilder - invoke it from the option-visibility swap and from Hidden, AutomationHidden and WithAnnotations The command-level half of this predates option-level visibility; it is fixed here because it shares the cause and the one-line remedy. Both axes are pinned by tests that list tools, change visibility, and list again.
Validation ran inside ResolveActiveRoutingGraph, which ResolveUniquePrefixes calls before any handler dispatch. ExecuteCoreAsync has try/finally with no catch and ReplApp.RunAsync catches only HostedServiceLifecycleException, so a misconfiguration escaped Run() as an unhandled exception — for every command, not just the MCP one. It also re-walked every command on each resolution, including the per-keystroke completion path. Move the check to the only phase where throwing is legitimate: - Map validates the schema it just built, rejecting the declarative form - the fluent setter validates the candidate schema before publishing it, so the diagnostic carries the caller's own Hidden() frame The diagnostic now lands on the line that caused it. Both entry points are pinned by tests asserting the throw site, not merely the message. Two consequences, both intended: - CoreReplApp.ValidateOptionMetadata disappears, which reattaches the routing-cache comment to the method it documents - McpServerHandler.RunAsync reverts to a plain async Task. The local RunCoreAsync existed only so validation could throw before the returned Task, and the cross-assembly `_app as CoreReplApp` downcast it needed goes with it. The MCP escape-hatch tests still pass: Map throws inside the fixture's configure callback, so CreateAsync still faults. Accepted cost: .Hidden(false) can no longer rescue an attribute-hidden required option, because Map fails before any fluent call runs. An attribute hiding a required option is a bug in the attribute; the docs now say to fix it there.
CreateAsync launched RunAsync fire-and-forget and then awaited the client handshake, so a server that failed while starting was observable only as an initialize timeout carrying the wrong exception. That is the harness deficiency that once justified making RunAsync throw synchronously. - race the handshake against the server task and rethrow the server's own fault - report a clean early server exit explicitly instead of hanging - release the pipes and the token source when construction fails before the fixture takes ownership of them No longer load-bearing for this branch — configuration now fails inside the configure callback, before RunAsync — but any other start failure still hung, as the new test demonstrates: it timed out before this change. Noted while here, not fixed: configureOptions is invoked twice, once through UseMcpServer and again on a fresh ReplMcpServerOptions, so a registration made inside it lands on two different instances. Pre-existing and out of scope.
…ay why it exists The review recommended deleting the six-parameter AddGlobalOptionCore forwarder on the grounds that the method is internal and Repl.Defaults reaches it through a ProjectReference, so no compatibility constraint exists. Packing Repl.Defaults shows otherwise: it emits `<dependency id="Repl.Core" version="0.12.0-dev..." />`, which NuGet reads as a minimum rather than an exact pin. A consumer can therefore run an older compiled Repl.Defaults against a newer Repl.Core, and that binary calls the arity it was built against. Folding isHidden in as a trailing optional parameter would not help — optional parameters are a compile-time convenience and the call site emits the full signature, so folding renames the method as far as the CLR is concerned. Keep the forwarder. Fix what was actually wrong with it: - comment it, naming the exact scenario it protects; an unexplained non-obvious shape was the real defect - replace the reflection existence assertion with one that invokes the arity and checks the option truly lands, visible. The old test passed whether or not the forwarder still forwarded; this one fails when it does not, verified by flipping the forwarded flag.
ParsingOptions.GlobalOption returned the same OptionBuilder that command options use. Command options are about to gain AutomationHidden, and on a global option that could never do anything: global options are consumed before routing and never enter the documentation model, so they cannot reach an MCP tool schema at all. Sharing the type would ship a method that silently does nothing, forever. Split GlobalOptionBuilder off so the invalid state is unrepresentable rather than merely undocumented — the type carries only Hidden. Preferred over a runtime throw, which would turn a compile-time impossibility into a runtime one and still leave the method visible in IntelliSense. Also, while rewiring the setter: - re-read the entry instead of closing over the definition the builder was created from. The closure wrote back a captured record, which was one added mutator away from silently reverting other fields. No test can demonstrate it today because duplicate registration throws, so this is stated rather than covered. - probe the dictionary by key before scanning it, keeping the scan only for a caller passing the rendered token for a bare-registered name. That branch is now pinned by its own test, since a keyed probe alone would miss it. Breaking against this branch only — GlobalOption's return type is unreleased. Nothing on origin/main is affected, and both integration tests that chain .GlobalOption(name).Hidden() compile untouched.
…f erasing them Issue yllibed#76's implementation notes ask for IsHidden to be carried through the documentation metadata so each renderer can filter it. The branch erased hidden options instead, which left an app author with no way to ask which options are hidden — while hidden *commands* have been modelled and exported all along. Mirror the command axis exactly: - ReplDocOption gains IsHidden, declared in the record body rather than as a positional parameter: the record is public with nine positional parameters and no defaults, so a tenth would change the constructor and Deconstruct arity for every already-compiled consumer - the aggregate model still omits hidden options, which is what keeps them out of the MCP tool schema and its argument allow-list, since MCP always builds the aggregate - a model built for an explicitly targeted command includes them, flagged, the same way `doc export contact debug dump-state` already exports a hidden command with "isHidden": true Two existing tests asserted the old exact-path behaviour and now assert both halves of the contract instead. Restructuring the option filter around `parameter.Name is { } name` also removed the two null-forgiving operators that predicate carried.
Commands could already be withheld from agents while staying visible to humans, via CommandAnnotations.AutomationHidden. Options had only the all-surfaces Hidden, so an option meant for people but not for agents had no expression. Add the second axis at the option level, as two independent booleans rather than an enum, matching how the command axis is modelled: - OptionBuilder.AutomationHidden and ReplOptionAttribute.AutomationHidden - IsAutomationHidden on the internal OptionSchemaParameter, populated by OptionSchemaBuilder from the attribute it already holds - OptionSchema.IsOptionAutomationHidden for consumers - ReplDocOption.IsAutomationHidden, again an init-only property in the record body to keep the public positional constructor intact Unlike Hidden, an automation-hidden option is always kept in the documentation model: human-facing exports and help must still show it, so the flag exists rather than an omission. Only the MCP projection drops it, which lands next. Rejected on typed global-options properties: global options are consumed before routing and never enter the documentation model, so the flag could never act on anything. The fluent path prevents this by type — GlobalOptionBuilder has no such method — but an attribute cannot be type-split, so ThrowIfUnsupportedOverrides gains a branch alongside its CaseSensitivity and Arity siblings. Hidden stays supported there, and that asymmetry is the honest one: one flag has a surface to affect, the other does not. The two visibility setters now share one CompareExchange updater, and attribute versus fluent precedence is pinned in both directions on both axes. XML docs on every new member state the surfaces covered, that tools/call rejects the option, and that neither flag is an access-control boundary.
…d WithOption Two related changes, both about a single source of truth. MCP filtering happens once, at the documentation-model boundary in BuildSnapshotCore, rather than in each emitter. The generated tool schema does not declare additionalProperties:false, so McpToolAdapter's argument allow-list is the only thing turning an unadvertised option into a hard error; if schema generation and allow-listing ever read different option lists, an option would vanish from the schema while staying callable. Projecting once makes that unrepresentable, and McpSchemaGenerator, McpToolAdapter, ReplMcpServerPrompt, ReplMcpServerTool and ReplMcpAppLauncherTool need no edits at all. A prompts/list test proves the point: it builds its arguments from its own loop, untouched here, yet still omits the option. The planned unit test against PrepareExecution was dropped rather than written: under this design the projection removes the option before the allow-list sees it, and the allow-list is deliberately flag-blind, so such a test would assert a state production cannot reach. Second, CommandBuilder.WithOption(targetName, configure) — the shape issue yllibed#76 also sketched. Now that CommandBuilder and OptionBuilder both expose Hidden and AutomationHidden, a mid-chain `.Option("x").AutomationHidden()` reads exactly like the command-level call while meaning something quite different, and nothing signals that .Option() changed the subject. The callback keeps the subject explicit and the chain on the command. Every chained call site is converted; .Option() stays for the standalone-statement form, and the docs say which to use where.
…point
CommandBuilder.Option returned the option builder, so a chain could switch
subject silently. Since both CommandBuilder and OptionBuilder now expose Hidden
and AutomationHidden, `.Option("x").AutomationHidden()` mid-chain read exactly
like the command-level call while meaning something quite different — and nothing
in the reading signalled the change.
Remove the public Option(string) and keep only WithOption(targetName, configure).
The ambiguous form is now impossible to write rather than merely discouraged, and
the public surface drops a method. Selection survives as a private helper.
All twelve call sites move to the callback form. Breaking against this branch
only — Option(string) is unreleased and nothing on origin/main uses it.
Diagnostics: - the complete ambient command claimed "no completion provider registered" for a hidden target, where one usually is registered. It now says no completion is available, which is true for a hidden target and an unknown one alike, and a comment records that conflating them is deliberate: distinguishing the two would let a caller probe for hidden options through this command. - the required-hidden rejection now names the rendered token as well as the CLR target, plus the remedy. An operator greps argv for '--internal-token' and would not find the parameter name anywhere. Test fidelity: - the hidden-enum completion test asserted two bare BeEmpty results, which would also hold if this shape offered no values at all. A visible sibling is now asserted in the same pass. It passes, which settles the open question: optional enum options do complete their values, so the original assertions were falsifiable and this was a readability gap, not a hidden bug. - the hidden global-option test asserted the bare substring "-t", which held only while no other rendered token contained it. It now asserts rendered option rows, with a visible sibling whose token deliberately contains "-t". - the two MCP initialization tests are deleted rather than merged. Validation now happens at Map, so neither EnableApps nor UiResource has any bearing — the throw precedes any MCP option being read — and their descriptions claimed these settings "bypass documentation discovery", which was never true. The contract is pinned directly on the core side. Readability: the five-clause shell-completion guard becomes a named predicate rather than a crammed condition; splitting it inline pushed the method past the length analyzer, and naming it was the better answer than a suppression.
… control - a matrix contrasting Hidden and AutomationHidden across help, completion, aggregate and targeted doc export, MCP schema, MCP tools/call, and parsing - an explicit warning that hiding is not access control. The worked examples are credential-shaped (InternalToken, internal-tenant), so the docs now say plainly that a hidden option stays invocable for anyone who knows its name and that privileged behavior needs real authorization - troubleshooting for an option that will not bind: it is almost always a target-name mistake, since WithOption takes the CLR name rather than the rendered token, and `doc export <command> --json` is the way to see hidden options flagged - a note that nothing records *use* of a hidden option, so retiring a deprecated switch needs the handler to log it. Repl.Core cannot do this itself: its build fails on any package reference, so it has no logging abstraction available - option-level rows next to the existing command-level AutomationHidden rows in mcp-reference, mcp-overview and for-coding-agents
The visibility flags are new members on ReplDocOption, a serialized public record, and the yaml and xml emitters serialize it wholesale rather than field by field. Only json and markdown had coverage, so those two formats would have broken silently.
This comment was marked as outdated.
This comment was marked as outdated.
![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.
All four findings reproduced before being fixed. **Required options whose arity cannot express it.** Arity counts the values a token consumes, so a non-nullable bool with no default reports ZeroOrOne while the binder still refuses to bind it when absent. Hiding one therefore produced an uninvocable command — proven: `exit=1, Unable to bind parameter 'force'`. The schema now carries CanBeOmitted and the guard consults it. Narrowed from what was reported: this only strands the command when the token is the sole way in. An OptionAndPositional parameter is still satisfiable positionally with its token hidden, and an existing completion test relies on exactly that, so the check is scoped to OptionOnly. **Automation-hidden options that cannot be omitted.** The projection withdrew the option from both the schema and the allow-list while leaving the command advertised, so every tools/call was guaranteed to fail. The command is now withdrawn from MCP instead, and its derived resources with it. Not rejected at configuration time, unlike the all-surfaces axis: the human command line remains perfectly usable, so forbidding the combination outright would ban a legitimate design. **Global token collisions.** GlobalOptionParser gives a colliding token to the LAST registration, and its own comment warns callers not to scan definitions independently — which the completion source did. A token registered as a visible option's alias and later as a hidden option's canonical form was advertised as visible while accepting it bound the hidden one. Visibility is now decided per token through TryResolveCustomGlobalDefinition, the helper that exists for this. **A documentation promise the code did not keep.** The troubleshooting section said the unknown-target diagnostic lists the registered targets. It did not. The exception now lists them, which is the more useful half of the fix, since a CLR-name mismatch is what that error almost always reports.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The previous fix covered the completion source but not help, which had the same defect: BuildGlobalOptionRows filtered GlobalOptions.Values by each definition's own IsHidden, while GlobalOptionParser gives a colliding token to the LAST registration. A token listed as a visible option's alias could therefore be advertised in help while accepting it bound a hidden option. Each canonical token and each alias is now resolved through TryResolveCustomGlobalDefinition, so a row keeps only the tokens that really belong to it. One funnel covers both consumers — the help model at HelpTextBuilder.cs:331, which markdown and json export read, and the text renderer at Rendering.cs:419. That leaves no site scanning global definitions independently: the completion source and this were the only two.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Six findings, each reproduced in red before being fixed. Two of them corrected earlier fixes of mine; one was a high finding from the original panel that I had simply never wired up. **Typo suggestions leaked hidden tokens.** InvocationOptionParser searched the full KnownTokens set, so mistyping `--secre` answered "Did you mean '--secret'?" for a hidden option — turning a validation error into a way to enumerate hidden options by probing at small edit distance. This was flagged as high in the original review and never addressed: the shared projection existed from the refactor, this one consumer was simply not pointed at it. OptionSchema gains DiscoverableTokens and the suggester reads that; parsing keeps the full set, so a hidden option still binds when supplied. **The required-option rule was wrong in both directions.** Two findings looked contradictory and together showed why: - an inferred ExactlyOne describes how many values a token consumes when present, not whether the option may be absent, so hiding a nullable reference option was rejected even though omitting it binds null happily - CanBeOmitted was scoped to OptionOnly on the theory that a positional fallback rescues the command. It does not for MCP, which reconstructs tool arguments as named tokens and cannot express a positional, so a non-defaulted bool in the default mode advertised a command whose every tool call failed to bind The rule is now: an explicitly declared required arity, or a CLR shape with nothing to fall back on, regardless of Mode. One existing test declared a bool flag with no default — the shape every other test avoids — and now supplies one. **Global help dropped rows it should have kept.** The previous per-token fix required the canonical token to survive, so a definition whose canonical form was claimed by a later hidden option lost its whole row even when an alias nobody took still reached it. Rows are rebuilt from whichever tokens the definition still owns, canonical or not. **Markdown did not flag visibility.** The exact-target export contract promises hidden options are included *and flagged*. Structured formats got that free by serializing the record; markdown formats each field by hand and rendered a hidden option indistinguishably from a public one. Options now carry [hidden] and [automation-hidden] markers, as commands already carry their own Hidden line. The sixth finding — per-token ownership in help — was already fixed in d7656e7; that review targeted the commit before it.
Member
|
@codex please do a deep review of this pr |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60d105e939
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e98f72ef7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08383557d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| var suggestion = TryResolveSuggestion( | ||
| optionToken, | ||
| schema.KnownTokens, | ||
| schema.DiscoverableTokens, |
There was a problem hiding this comment.
When a hidden custom global owns the same token as a visible route option, a near miss still reaches route parsing and this collection suggests the globally owned token because DiscoverableTokens considers only route-option visibility. For example, with hidden global --secret and a visible route option using --secret, typing --secre produces Did you mean '--secret'?, even though help, completion, and documentation suppress that token and an exact spelling is consumed by the hidden global before route binding. Filter suggestion candidates through GlobalOptionParser.BuildCustomTokenOwnership(options) as the other discovery surfaces do.
Useful? React with 👍 / 👎.
Comment on lines
+179
to
+182
| var comparer = ResolveEffectiveOptionComparer(optionAttribute, parsingOptions); | ||
| foreach (var alias in optionAttribute?.Aliases ?? []) | ||
| { | ||
| if (optionAttribute?.HiddenAliases.Contains(alias, comparer) == true) |
There was a problem hiding this comment.
When an option inherits the global case mode and that mode changes after mapping, hidden-alias precedence remains frozen to the mode used while building the schema. For example, map an option under CaseSensitive with visible alias --ACCOUNT and hidden alias --account, then switch OptionCaseSensitivity to CaseInsensitive: parsing now treats the spellings as equivalent, but the uppercase entry remains discoverable in help/completion and can still expose its value provider. Fresh evidence beyond the prior case-equivalent fixes is that runtime parsing reads the current global mode while this visibility decision is permanently materialized here; discovery must reevaluate inherited-case aliases or update mapped schemas when the mode changes.
Useful? React with 👍 / 👎.
Comment on lines
+400
to
+404
| var hasReachableNamedToken = schema.Entries.Any(entry => | ||
| string.Equals(entry.ParameterName, parameterName, StringComparison.OrdinalIgnoreCase) | ||
| && entry.TokenKind is OptionSchemaTokenKind.NamedOption or OptionSchemaTokenKind.BoolFlag | ||
| && !entry.IsHidden | ||
| && !customGlobalOwnership.ContainsKey(entry.Token)); |
There was a problem hiding this comment.
When a custom global shadows every ordinary named token but an unowned reverse or value alias remains, this predicate removes the entire option from both aggregate and exact-target documentation. For example, if global --force shadows a route Boolean whose --no-force reverse alias is still accepted, command help lists --no-force but documentation omits the option and its ReverseAliases metadata. Fresh evidence beyond the ordinary-alias collision fix is that the new reachability check explicitly excludes ReverseFlag, ValueAlias, and EnumAlias; retain these options for human documentation, while handling MCP projection separately if those aliases cannot reconstruct its semantic argument.
Useful? React with 👍 / 👎.
A tool call could supply a value for a visible bool option that itself looked like an option token (e.g. "-t=denim", an alias of a Hidden() option on the same route). ApplyBoolFlagValue declines to consume such a value with no diagnostic — required so legitimate flag-chaining like "--verbose --other" keeps working — leaving it dangling to be re-lexed as a fresh option on the parser's next iteration, silently binding the hidden target. Every other option kind either has no value to smuggle or fails the whole call via a diagnostic when its value looks option-like, so only bool options need protecting: their value is now embedded as a single inline "--name=value" token, which cannot be split apart and re-lexed. Also close the analogous route-segment vector: a positional argument value has no inline-token escape available, so a value that itself looks like an option token is rejected outright when substituted into the CLI stream.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eafe8f4b84
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+234
to
+236
| else | ||
| { | ||
| hiddenAliases.Remove(registeredAlias); |
There was a problem hiding this comment.
When case-distinct global aliases are registered in case-sensitive mode and parsing later switches to case-insensitive mode, hiding one spelling makes every equivalent spelling hidden because IsGlobalOptionAliasHidden uses the active comparer. Calling HiddenAlias(..., isHidden: false) through another equivalent registered spelling removes only that exact spelling here, leaving the original entry in HiddenAliases, so help and completion continue suppressing both aliases even though the caller requested one of the parser-equivalent aliases be shown. Apply the active comparer to the whole equivalent set when unhiding, as route-option alias visibility does.
Useful? React with 👍 / 👎.
OptionBuilder.Hidden()'s remark claimed hiding a required option "fails here rather than later" — true for options-group properties (ValidateOptionVisibility runs at configuration time), but not for direct handler parameters: those may still be satisfiable from DI or a synthesized progress channel, which is only knowable once a real service provider exists, so that case fails the first time discovery runs against one instead. Also document the InvalidOperationException WithOption can throw via that same configuration-time path. Investigated moving this to fail at Run() entry instead of lazily at first discovery, but the aggregate-throws-rather-than-omits behavior is deliberate and already covered by When_RequiredCommandOptionIsHiddenFluentlyWithoutAService_Then_AggregateDocumentationThrows and When_HiddenGlobalOwnsRequiredRouteOptionToken_Then_AggregateDocumentationFailsClosed — silently omitting would advertise an MCP tool that can never succeed. Left that design alone; this is a documentation-only fix.
GetSnapshotAsync rethrew HiddenRequiredOptionException verbatim on every fail-closed visibility retraction, including after a client had already received a working tool schema. That exception's message names the option's target, rendered token and route — the identity Hidden() was meant to withhold — so once any client has a working schema, the failure now surfaces as a generic McpException instead. The very first snapshot build (no client has ever had a schema) still lets the detailed exception through, matching When_HiddenRequiredOptionHasNoServiceFallback_Then_McpStartupFailsFast: that case is a cold-start configuration error for the operator, not a runtime retraction reaching a connected client. Strengthened the existing post-first-success regression test to also assert the exception message excludes the option's identity — it only checked the exception type before.
IsServiceAvailable called GetService directly to probe whether a hidden-required option's CLR type has a DI fallback. A registration that throws on resolution — most commonly a scoped service resolved from the root provider under ValidateScopes, or any constructor that fails — propagated straight through documentation/MCP discovery, crashing it with that registration's own exception instead of the intended hidden-required diagnosis, for every other route in the app. Treat a throw the same as a null result: unavailable. Investigated using IServiceProviderIsService to avoid activation entirely, but Repl.Core is enforced dependency-free at runtime (see the csproj's _DisallowedPackageReference check) and that type lives in Microsoft.Extensions.DependencyInjection.Abstractions — out of reach here. The try/catch closes the crash; it does not avoid activating a transient factory, which is a smaller, accepted residual cost.
ShellCompletionEngine (two sites) and AutocompleteEngine re-derived "entry.IsHidden || schema.IsOptionHidden(entry.ParameterName)" inline instead of reusing OptionSchema.DiscoverableEntries's own rule — the same duplication pattern that produced repeated fix commits across help, documentation and completion earlier in this branch. Added OptionSchema.IsEntryDiscoverable for callers holding one resolved entry, and rewrote DiscoverableEntries on top of it so there is one expression of the rule instead of four. Behavior-preserving: full Repl.Tests and Repl.IntegrationTests suites pass unchanged. Left DocumentationEngine's reachability check alone — it also folds in global-token ownership, a materially different (and correct) condition, not the same duplication.
HelpTextBuilder's IsGlobalTokenOwnedBy and OptionTokenCompletionSource's TryAddGlobalToken independently re-implemented the identical four-part rule (ownership lookup, ReferenceEquals against the expected owner, definition not hidden, this alias spelling not hidden). Moved it to GlobalOptionParser.IsGlobalTokenDiscoverable, next to the BuildCustomTokenOwnership projection it reads, so help and completion share one expression of the rule instead of two. Behavior-preserving: full Repl.Tests and Repl.IntegrationTests suites pass unchanged.
BuildCustomTokenOwnership rebuilt a full Dictionary — one insert per global option plus one per alias — on every call, despite its own comment saying it should run "once for a whole parse/help/completion pass." AutocompleteEngine calls it per keystroke and HelpTextBuilder calls it twice per render, so this was allocation-per-lookup on an interactive path. Moved the cache onto ParsingOptions itself: it owns the mutators that can change token ownership (registration, per-definition and per-alias visibility, and the case-sensitivity setting the ownership dictionary's comparer is built from), so it can null the cache at every one of them without a version counter. GlobalOptionParser's BuildCustomTokenOwnership now delegates to it, so every existing caller benefits without change. Also short-circuit the per-Parse custom-global-values projection to a shared empty instance when nothing was supplied — the overwhelming common case, previously a fresh Dictionary on every invocation regardless. Verified against the full Repl.Tests and Repl.IntegrationTests suites, including the existing runtime-visibility-mutation tests that exercise cache invalidation.
WithAliasVisibility mutated a captured `found` flag from inside a Select projection — correct only because ToArray happens to enumerate eagerly, and would break silently if that materialization were ever removed. Replaced with an explicit loop over Entries. Also make a redundant .HiddenAlias(x, isHidden) call (the alias is already in the requested state) a true no-op: WithAliasVisibility now returns the same schema instance instead of an equivalent copy, and CommandBuilder.UpdateOptionAliasVisibility recognizes that via ReferenceEquals and skips the schema swap and routing invalidation — mirroring the early exit UpdateOptionParameter already had for the same shape of no-op. Verified against the full Repl.Tests and Repl.IntegrationTests suites.
SendNotificationSafeAsync sent with CancellationToken.None. Nothing awaits this fire-and-forget task, so a stuck stdio peer would hang it indefinitely — one more never-completing task per routing invalidation, forever. Bound it to a 5s timeout instead. Left the debounce timer's reset-with-no-ceiling alone: a genuinely continuous stream of invalidations is a narrow, unusual scenario, and McpServerHandler's snapshot/versioning logic is under active concurrent development elsewhere on this branch right now — safer to leave that specific mechanism alone than risk a subtle interaction with it.
Two different Names ("tenant" and "--tenant") can normalize to the
identical canonical token. The existing duplicate check only compares
by the raw Name key, so both registrations succeeded, leaving
ResolveGlobalOptionKey's FirstOrDefault to arbitrarily pick whichever
definition happens to enumerate first whenever GlobalOption(name) is
later called by that token — GlobalOption("tenant").Hidden() could
silently hide the wrong definition, leaving the token it actually owns
still advertised.
Reject the second registration outright, the same way an exact-name
duplicate already is.
No CHANGELOG previously existed. A commit subject marked with a Conventional Commits "!" (e.g. the WithOption consolidation) is not consumer documentation, and this release adds public API (.Hidden, .HiddenAlias, HiddenAliases), changes what doc export --json serializes, and introduces a lockstep-upgrade requirement between Repl.Core and Repl.Defaults that a consumer bumping only one package would otherwise discover as a runtime MissingMethodException. Versions are assigned by Nerdbank.GitVersioning at pack time, so this groups changes under "Unreleased" rather than a specific version number.
Asserted only that HiddenAliases/HiddenAlias exist via reflection — would pass even if HiddenAlias were a no-op. The very next test, When_RouteAliasIsHidden_Then_OnlyParsingRetainsIt, already exercises both APIs behaviorally (parsing, help, and documentation), so this one added no coverage the neighboring test doesn't already provide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47d68267ff
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+242
to
+243
| internal bool IsGlobalOptionAliasHidden(GlobalOptionDefinition definition, string token) => | ||
| definition.HiddenAliases.Contains(token, ResolveOptionTokenComparer()); |
There was a problem hiding this comment.
When a global is registered in case-sensitive mode with canonical --tenant and a case-distinct alias --TENANT, hiding only the alias and then switching OptionCaseSensitivity to CaseInsensitive makes this membership check classify the canonical token as hidden too. Root help and both completion surfaces consequently remove the entire option even though GlobalOptionBuilder.HiddenAlias promises to leave its canonical token visible; preserve canonical-token visibility when reevaluating aliases, or reject aliases that become canonical-equivalent.
Useful? React with 👍 / 👎.
| { | ||
| // Preserve case-distinct aliases in the parse schema so inherited visibility can be | ||
| // reevaluated if the global case mode changes after mapping. | ||
| if (optionAttribute?.HiddenAliases.Contains(alias, StringComparer.Ordinal) == true) |
There was a problem hiding this comment.
When an attribute supplies at least one visible alias but explicitly sets HiddenAliases = null, which is valid attribute metadata despite producing a nullable warning, this call dereferences the null array and command mapping fails with NullReferenceException. The surrounding loops already treat null alias arrays as empty, so apply the same null fallback to this precedence check; the options-group property path repeats the same dereference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c58b062d29
ℹ️ 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".
Comment on lines
+146
to
+147
| changed = entry.IsHidden != isHidden; | ||
| entries[i] = changed ? entry with { IsHidden = isHidden } : entry; |
There was a problem hiding this comment.
When an option has mixed visibility among parser-equivalent aliases, this assignment overwrites an earlier true with the last matching entry's state. For example, map visible --ACCOUNT plus hidden --account under case-sensitive parsing, switch to case-insensitive mode, call HiddenAlias("--ACCOUNT"), and then restore case-sensitive mode: the visible entry should have been hidden, but the later already-hidden entry resets changed to false, causing the method to return the original schema and --ACCOUNT to reappear. Accumulate the flag with OR and use each entry's own comparison result when constructing the replacement.
Useful? React with 👍 / 👎.
WithAliasVisibility overwrote 'changed' on every matching entry instead of accumulating it. When two parser-equivalent aliases match the same request (e.g. --ACCOUNT and --account once parsing turns case-insensitive) and the first genuinely changes while a later one is already in the requested state, the later entry's no-op reset 'changed' to false — the method then returned the original schema unchanged, discarding the first entry's real update even though the constructed entries array held it correctly. Track each entry's own comparison result and OR it into the accumulator instead of assigning it directly. Found via automated PR review (chatgpt-codex-connector) on this same branch, in code I introduced myself in 35f9488.
ReplOptionAttribute.HiddenAliases can be explicitly set to null (valid attribute metadata despite the nullable warning), and optionAttribute?.HiddenAliases.Contains(alias, ...) reads as a null dereference in that case. Investigated with a red test expecting a crash first: it did not reproduce. .Contains here resolves to the MemoryExtensions span overload, and the implicit array-to-ReadOnlySpan<T> conversion turns a null array into Span.Empty rather than throwing, so the call silently (and correctly) treats the null the same as an empty array today. Applied the explicit ?? [] guard anyway, matching the sibling consumption site two lines below and every other HiddenAliases reader in this file: relying on the null-to-empty-span conversion to get the right answer is fragile and non-obvious, even though it isn't presently a bug. Kept both regression tests (direct parameter and options-group property) asserting the alias stays registered and visible, since that is the behavior worth pinning either way.
IsGlobalOptionAliasHidden checked membership in HiddenAliases using the currently active comparer, with no distinction between the canonical token and an alias. A global registered under case-sensitive parsing with canonical --tenant and a case-distinct hidden alias --TENANT (a legal registration at that point) had its canonical token incorrectly classified as hidden the moment parsing switched to case-insensitive, because "--tenant" then compares equal to the already-hidden "--TENANT" under the new comparer. GlobalOptionBuilder.HiddenAlias's own contract promises the canonical token stays visible; only Hidden() on the whole definition may retract it. The canonical-identity check must use Ordinal specifically: using the effective (now case-insensitive) comparer for it would also exempt the "--TENANT" alias string itself from its own HiddenAliases membership, undoing the fix for the entry the caller actually asked to hide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bbd293404
ℹ️ 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".
Comment on lines
+337
to
+339
| var boolOptionNames = command.Options | ||
| .Where(static option => string.Equals(option.Type, "bool", StringComparison.Ordinal)) | ||
| .Select(static option => option.Name) |
There was a problem hiding this comment.
When an advertised bool? option receives a string such as "-x", this exact-type check omits it from boolOptionNames because its documentation type is bool?. MCP therefore reconstructs --visible -x; ApplyBoolFlagValue treats --visible as true and re-lexes -x, which can activate a hidden or automation-hidden short alias on another option even though that field was removed from the MCP schema and allow-list. Classify nullable Boolean flags here as well so their values use the non-relexable inline form.
Useful? React with 👍 / 👎.
Comment on lines
+124
to
+126
| var canonicalEntry = FindNamedEntry(parameterName); | ||
| if (canonicalEntry is not null | ||
| && TokensAreEquivalent(canonicalEntry, alias, currentGlobalCaseSensitivity)) |
There was a problem hiding this comment.
When a command is mapped case-sensitively with canonical --tenant and alias --TENANT, then the global mode changes to case-insensitive, .HiddenAlias("--TENANT", false) throws here because the alias is now parser-equivalent to the canonical entry. The supplied spelling is still an explicitly registered alias whose visibility may need updating before case-sensitive mode is restored, so check for an exact alias entry before treating an equivalent spelling as an attempt to modify the canonical token.
Useful? React with 👍 / 👎.
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
2 participants
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
.Hidden(bool)metadata for direct command options, options-group properties, manually registered globals, and typed global optionsHiddenAliases/.HiddenAlias(...)migration metadata so canonical options remain discoverable while legacy route/global aliases stay accepted only by CLI/REPL parsing; includes direct parameters, options-group properties, manual globals, and typed globals.Hidden(false)to restore visibilityAddGlobalOptionCoredescriptor for binary compatibility with already-compiledRepl.DefaultsassembliesValidation
eafe8f4: 1,413 total; 1,412 passed; 1 external MCP Inspector smoke skipped; 0 failed0838355; current-head CI oneafe8f4is running after final test-matrix coverageRepl.Defaultswith candidateRepl.Corepassedorigin/mainCloses #76