Skip to content

feat: add fluent hidden options#77

Open
autocarl wants to merge 30 commits into
yllibed:mainfrom
autocarl:agent/issue-76-hidden-options
Open

feat: add fluent hidden options#77
autocarl wants to merge 30 commits into
yllibed:mainfrom
autocarl:agent/issue-76-hidden-options

Conversation

@autocarl

@autocarl autocarl commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add option-level .Hidden(bool) metadata for direct command options, options-group properties, manually registered globals, and typed global options
  • keep hidden options parsable while filtering canonical tokens, aliases, descriptions, defaults, and value candidates from help, documentation, interactive/shell completion, and MCP schemas
  • require hidden command options to be omittable: options-group properties fail during mapping, while direct parameters are validated against the active provider during aggregate documentation/MCP discovery and may be satisfied by DI or synthesized progress; allow .Hidden(false) to restore visibility
  • share one last-registration-wins global token-ownership projection across parsing, help, and completion so hidden/visible alias collisions remain consistent
  • preserve the historical six-parameter AddGlobalOptionCore descriptor for binary compatibility with already-compiled Repl.Defaults assemblies

Validation

  • strict Release build: 0 warnings, 0 errors
  • final Release solution: 1,372 total; 1,371 passed; 1 external MCP Inspector smoke skipped; 0 failed
  • final CI passed build/test/pack on Ubuntu, Windows, and macOS, plus real-shell completion and documentation lint
  • NuGet pack: 10 packages plus 9 symbol packages
  • binary assembly-swap probe: baseline Repl.Defaults with candidate Repl.Core passed
  • real Bash completion smoke passed
  • changed documentation passed markdownlint
  • trimmed MCP sample publish produced a self-contained artifact; strict trim diagnostics are unchanged from origin/main

Closes #76

@autocarl

This comment was marked as resolved.

@carldebilly

This comment was marked as outdated.

@chatgpt-codex-connector

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.
@carldebilly

This comment was marked as outdated.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

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.
chatgpt-codex-connector[bot]

This comment was marked as resolved.

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.
chatgpt-codex-connector[bot]

This comment was marked as resolved.

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.
@carldebilly

Copy link
Copy Markdown
Member

@codex please do a deep review of this pr

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/Repl.Core/Help/HelpTextBuilder.Rendering.cs Outdated
Comment thread docs/commands.md Outdated
Comment thread src/Repl.Core/Internal/Options/OptionTokenCompletionSource.cs Outdated
Comment thread src/Repl.Core/CommandBuilder.cs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread docs/parameter-system.md Outdated
Comment thread src/Repl.Core/Documentation/DocumentationEngine.cs
@autocarl

Copy link
Copy Markdown
Contributor Author

Review follow-up complete on bc08f09.

  • Updated the parameter-system summary to describe CLR omittability, options-group validation, and provider-aware direct-parameter discovery.
  • Aligned discovery requiredness with the binder's synthetic IProgress<double> / IProgress<ReplProgressEvent> fallback through IReplInteractionChannel, while preserving direct service resolution.
  • Added CLI/documentation and MCP schema/invocation regressions for both progress forms, and made the MCP fixture use the production default-service topology.

Validation:

  • dotnet test src/Repl.slnx -c Release: 1,369 passed, 0 failed, 1 expected external-toolchain skip.
  • git diff --check: clean.
  • CI: all build/test/pack, three OS jobs, real-shell completion, and documentation lint checks passed: https://github.com/yllibed/repl/actions/runs/30168500998

Both review threads were answered individually and resolved.

@carldebilly

Copy link
Copy Markdown
Member

@codex review please

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/Repl.Defaults/ReplApp.cs Outdated
Comment thread src/Repl.Defaults/ReplApp.cs
Comment thread src/Repl.Core/Internal/Options/OptionTokenCompletionSource.cs
@autocarl

Copy link
Copy Markdown
Contributor Author

Latest review round complete on 2b4e663.

Two findings were reproducible and are fixed:

  • 138f61e: documentation export no longer materializes/finalizes the shared provider before later Use* service registrations.
  • 2b4e663: route-option completion now respects higher-precedence custom-global token ownership on both interactive and shell surfaces.

The reported CreateDocumentationModel(null) ambiguity was not reproducible: an external net10.0 consumer compiled both the ReplApp and CoreReplApp calls with 0 warnings / 0 errors and ran successfully, so no API change was made.

Validation:

  • dotnet test src/Repl.slnx -c Release: 1,371 passed, 0 failed, 1 expected external Inspector skip.
  • Source-compatibility consumer: build and execution passed.
  • Final CI passed build/test/pack, Ubuntu, Windows, macOS, real-shell completion, and documentation lint: https://github.com/yllibed/repl/actions/runs/30172338497

All three threads were answered with evidence and resolved; no active review thread remains.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +358 to +359
if (InteractionProgressFactory.IsSupportedProgressType(parameterType)
&& IsServiceAvailable(typeof(IReplInteractionChannel), serviceAvailability))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment on lines +253 to +255
&& !Attribute.IsDefined(parameter.ParameterType, typeof(ReplOptionsGroupAttribute), inherit: true)
&& (includeHiddenOptions || !schema.IsOptionHidden(name)))
.Select(parameter => BuildDocumentationOption(schema, parameter, serviceAvailability));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add fluent .Hidden() support for command and global options

2 participants