feat: add fluent hidden options#77
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.
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.
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.
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.
|
@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: 6d48be511f
ℹ️ 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".
|
Review follow-up complete on
Validation:
Both review threads were answered individually and resolved. |
|
@codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc08f09bc4
ℹ️ 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".
|
Latest review round complete on Two findings were reproducible and are fixed:
The reported Validation:
All three threads were answered with evidence and resolved; no active review thread remains. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b4e6630e4
ℹ️ 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".
| if (InteractionProgressFactory.IsSupportedProgressType(parameterType) | ||
| && IsServiceAvailable(typeof(IReplInteractionChannel), serviceAvailability)) |
There was a problem hiding this comment.
Treat MCP's per-call progress channel as available
When ICoreReplApp.BuildMcpServerOptions() is called without a service provider (or with one that does not register IReplInteractionChannel), an explicitly required IProgress<double>/IProgress<ReplProgressEvent> option marked AutomationHidden is considered required here and its entire tool is removed. The surviving evidence beyond the earlier progress-fallback fix is that this public overload uses EmptyServiceProvider, while McpToolAdapter.ExecuteThroughPipelineAsync always adds an IReplInteractionChannel for the actual call, so omission would bind successfully if the tool were retained. MCP discovery should account for that guaranteed per-call overlay rather than checking only the base provider.
Useful? React with 👍 / 👎.
| && !Attribute.IsDefined(parameter.ParameterType, typeof(ReplOptionsGroupAttribute), inherit: true) | ||
| && (includeHiddenOptions || !schema.IsOptionHidden(name))) | ||
| .Select(parameter => BuildDocumentationOption(schema, parameter, serviceAvailability)); |
There was a problem hiding this comment.
Exclude globally owned tokens from MCP option metadata
When a hidden custom global option owns the same canonical token as a visible route option, this route-only visibility filter still emits the route option into the aggregate documentation model. McpServerHandler.BuildSnapshotCore derives both the tool schema and argument allow-list from that model, but McpToolAdapter reconstructs the advertised argument as --tenant value and GlobalOptionParser consumes it before route parsing, so the handler receives its default or null instead of the value the MCP client supplied. The completion path now filters route entries against custom-global ownership; apply the same ownership projection when building documentation options.
Useful? React with 👍 / 👎.
Summary
.Hidden(bool)metadata for direct command options, options-group properties, manually registered globals, and typed global options.Hidden(false)to restore visibilityAddGlobalOptionCoredescriptor for binary compatibility with already-compiledRepl.DefaultsassembliesValidation
Repl.Defaultswith candidateRepl.Corepassedorigin/mainCloses #76