Skip to content

feat(tooling): migrate docs-builder CLI from ConsoleAppFramework to Nullean.Argh#3202

Open
Mpdreamz wants to merge 29 commits intomainfrom
feature/argh
Open

feat(tooling): migrate docs-builder CLI from ConsoleAppFramework to Nullean.Argh#3202
Mpdreamz wants to merge 29 commits intomainfrom
feature/argh

Conversation

@Mpdreamz
Copy link
Copy Markdown
Member

Why

docs-builder used ConsoleAppFramework as its CLI layer. Two fundamental problems made it worth replacing:

1. Help output was nearly useless.
CAF rendered a flat list of flags with no descriptions, no namespace grouping, and no validation hints. There was no way to express that --endpoint requires an http/https URI, that --plan-file must exist on disk, or that --exporters Html,Elasticsearch is a comma-separated collection. Contributors had to read source code to understand what a command accepted.

2. Namespace support was a string hack.
Commands were registered with prefixes like "assembler content-source" — a flat simulation of hierarchy. There was no real assembler --help page that showed only assembler sub-commands, no per-namespace help, and no way to document what the assembler namespace is.


What

This PR replaces ConsoleAppFramework with Nullean.Argh (0.11.0), a Roslyn source-generator CLI framework that is AOT/trim-safe by design.

Real namespace hierarchy

assembler, assembler deploy, assembler navigation, codex, changelog etc. are now first-class namespaces. docs-builder assembler --help shows only assembler commands. Each namespace has a one-liner summary visible in the parent listing.

Help text driven by XML docs

<summary> becomes the terse one-liner. <remarks> becomes the explanatory block. Param docs flow directly into flag descriptions. The migration invested in complete XML documentation for every command, with concepts like assembler, codex, link registry, and bloom filter explained inline for contributors unfamiliar with the infrastructure.

--help no longer emits host startup logs — argh 0.9.0+ detects intrinsic commands (--help, --version, __schema, __completion) at registration time and exits before the host is built.

Typed, validated parameters

URIs--endpoint and --proxy-address are Uri? with [Url] (DataAnnotations); argh rejects non-http/https values at parse time.

Timeouts--bootstrap-timeout is TimeSpan? with [TimeSpanRange("1s","60m")]. Users pass 4m or 90s, not a raw integer.

Thread / buffer counts[Range(1,128)] on thread counts, [Range(1,10_000)] on buffer size, [Range(0,20)] on retries.

File and directory paths — path parameters are FileInfo or DirectoryInfo (not string). Help shows <file> or <dir>. Where a file must exist before the command runs (config files, cert paths, plan files, links.json), [Existing] is applied and argh rejects at parse time rather than deep inside service code. [RejectSymbolicLinks] is on every path parameter. [ExpandUserProfile] means ~/path works everywhere.

File extensions[FileExtensions(Extensions="yml,yaml")] on config file arguments, [FileExtensions(Extensions="json")] on JSON inputs, [FileExtensions(Extensions="pem,der,crt,cer")] on certificate paths.

Exporters — previously a custom ExporterParser with its own vocabulary (es, config, links…). Now native IReadOnlySet<Exporter> binding — argh parses and validates the values, and lists the allowed set in help. Enum parsing is case-insensitive.

DTOs replace flat parameter lists

Service methods previously took 12–22 individual string?/bool?/int? parameters. This PR introduces IsolatedBuildOptions, AssemblerBuildOptions, AssemblerCloneOptions, and the already-existing ElasticsearchIndexOptions as proper records that:

  • Are bound from CLI flags via argh [AsParameters] (one line in the command method)
  • Are passed directly to service methods — no tuple unpacking, no 20-item lambda state
  • Can be constructed in unit tests as plain record initialisers, enabling real service-layer testing without mocking the CLI layer

docs-builder build as a named command

docs-builder (no sub-command) still builds the current documentation set. docs-builder build now also works as an explicit named command (MapAndRootAlias).

assemble consolidation

The old assemble (one-shot) and assembler (individual steps) namespace split is clarified: docs-builder assemble runs config-init + clone + build in one shot; docs-builder assembler clone/build/serve/index/sitemap are the individual steps.


Breaking changes

Before After
--no-ai-enrichment --no-ai-enrichment (via --ai-enrichment / --no-ai-enrichment)
--no-eis --eis / --no-eis
--bootstrap-timeout 4 (minutes as int) --bootstrap-timeout 4m (TimeSpan string)
docs-builder assemble (old one-shot) docs-builder assemble (unchanged behaviour, new name in help)
docs-builder codex <config> positional was string now FileInfo — path must resolve to a real file

Exporter names (html, es, config, links) are not breaking — Enum.TryParse is case-insensitive so html and Html both work.


argh bugs found and fixed upstream

During this migration five bugs were found and fixed in Nullean.Argh (0.7→0.11):

  1. Non-nullable enum global option emitting a required-flag error on --help
  2. {identifier} in XML doc text emitted as C# interpolation holes in generated code
  3. bool? + bool no<Name> generating duplicate --no-* switch arms (CS8510)
  4. __ev reused across multiple enum properties in the same options type (CS0136)
  5. [AsParameters] DTO property <summary> XML docs not rendered in --help for types in referenced assemblies

One known bug remains open: [Existing] on a nullable FileInfo?/DirectoryInfo? fires its existence check even when the value is absent (null), causing ArgumentNullException inside the generated runner.


Test plan

  • dotnet build — 0 errors, 0 warnings
  • docs-builder --help — root help, no startup logs, (default: build) alias shown
  • docs-builder build --help — all options have descriptions, [existing] on --path
  • docs-builder assembler --help — namespace summaries for all sub-namespaces
  • docs-builder assembler index --help — ES options show [schemes: http|https], [time-span-range], [range] hints
  • docs-builder assembler deploy apply --help--plan-file shows [existing]
  • docs-builder assembler deploy apply staging bucket missing.json — argh rejects with "path does not exist"
  • docs-builder index --endpoint not-a-url — argh rejects with scheme error
  • docs-builder build --bootstrap-timeout bad — argh rejects with time-span error
  • docs-builder codex --help — codex concept explained in namespace summary
  • docs-builder changelog --help — all sub-command summaries are one terse line

Mpdreamz and others added 12 commits April 29, 2026 08:58
…rk to Nullean.Argh 0.7.0

Replaces ConsoleAppFramework with Nullean.Argh.Hosting in docs-builder and
Nullean.Argh (standalone) in the aspire AppHost. Key improvements: proper
namespace hierarchy (assembler, codex, etc.), record binding via [AsParameters]
for the shared ElasticsearchIndexOptions, ICommandMiddleware replacing filter
chain, GlobalCliOptions with --log-level/--config-source/--skip-private-repositories
as typed global flags, and docs-builder assemble as a hoisted root command.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Create IsolatedBuildOptions, AssemblerBuildOptions, and AssemblerCloneOptions
records in the service layer. Update all six affected service methods to accept
these DTOs and the existing ElasticsearchIndexOptions directly, eliminating the
tuple-based state unpacking in ServiceInvoker lambdas. Commands are now thin
wrappers that construct a DTO and pass it straight through.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…m> in DTOs

- Bump Nullean.Argh + Nullean.Argh.Hosting to 0.8.0
- Delete ExporterParser: IReadOnlySet<Exporter>? now bound natively by argh
- IsolatedBuildOptions and AssemblerBuildOptions include Exporters via repeated
  flags (--exporters Html --exporters Elasticsearch); [CollectionSyntax(Separator=",")]
  deferred until argh fixes [AsParameters] + IReadOnlySet<Enum> interaction
- IsolatedBuildCommand.Build and AssemblerCommands.Build use [AsParameters] DTO
- [CommandName("build")] + [DefaultCommand] ready for MapAndRootAlias once the
  generator bug (missing { on subsequent methods) is fixed in argh
- Map<IsolatedBuildCommand>() used in place of MapAndRootAlias<> for now

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…s-builder build

- Bump to 0.8.1 (fixes MapAndRootAlias generator bug — missing { on subsequent methods)
- Switch Map<IsolatedBuildCommand>() → MapAndRootAlias<IsolatedBuildCommand>():
  docs-builder build is now both a named command and the root default alias
- IsolatedBuildOptions and AssemblerBuildOptions include IReadOnlySet<Exporter>?
  (repeated flags: --exporters Html --exporters Elasticsearch)
- [CollectionSyntax(Separator=",")] deferred: [AsParameters] + [CollectionSyntax]
  still emits a stray closing brace in 0.8.1 generated code

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Bump to 0.9.0 (fixes [AsParameters] + [CollectionSyntax] stray-brace generator bug)
- Re-enable [CollectionSyntax(Separator=",")] on IsolatedBuildOptions.Exporters
  and AssemblerBuildOptions.Exporters: --exporters Html,Elasticsearch now works
- Add ArghApp.TryArghIntrinsicCommand(args) pre-host fast path: --help, --version,
  __schema and __completion no longer trigger host construction or startup logs
- Nullean.Argh.Interfaces added to Isolated and Assembler service projects

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
ElasticsearchIndexOptions:
- Endpoint/ProxyAddress: string? -> Uri? with [UriScheme("http","https")]
- BootstrapTimeout: int? (minutes) -> TimeSpan? with [TimeSpanRange("1s","60m")]
- [Range] on SearchNumThreads, IndexNumThreads, BufferSize, MaxRetries
- Rename NoAiEnrichment -> AiEnrichment, NoEis -> Eis (positive flags,
  clean --no-ai-enrichment / --no-eis negation; removes double-negative)
- Full XML docs on all properties

XML documentation:
- IsolatedBuildOptions, AssemblerBuildOptions: XML docs on all properties
- Namespace class summaries and remarks: assembler (navigation.yml unified site),
  codex (independent per-set navigation), inbound-links (link registry concept)
- All assembler, codex, and inbound-links commands: complete <summary> and
  <remarks> explaining concepts (assembler, codex, link registry, bloom filter),
  ordering requirements, and usage examples
- Root commands (build, index, diff): <summary> tightened, <remarks> added

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…pace summaries

- ChangelogCommands: add class-level summary; rewrite all method summaries to
  one terse sentence; move detail to <remarks>; remove <code> invocation blocks
- All commands: remove <code> CLI invocation examples from <remarks> throughout
- GlobalCliOptions: remove manual enum listing from --log-level summary (argh
  renders allowed values automatically)
- AssemblerBuildOptions, IsolatedBuildOptions: add XML docs to properties
  (visible in IDE; argh cross-assembly doc limitation tracked separately)
- Nested assembler namespaces: add class-level <summary> to BloomFilterCommands,
  ConfigurationCommand, ContentSourceCommands, DeployCommands, NavigationCommands
  so they appear in assembler --help listings
- ServeCommand, MoveCommand, FormatCommand, IndexCommand: tighten summaries and
  move detail to <remarks> without code blocks

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
… docs from referenced projects

PR #25 in 0.9.1 adds XML doc fallback for [AsParameters] DTO members in
referenced assemblies. Enable GenerateDocumentationFile on Isolated,
Assembler, and Configuration service projects so argh's generator can
read property summaries from the produced .xml files.

Result: docs-builder build --help, assembler build --help, assembler
index --help etc. now show full descriptions for every [AsParameters]
DTO property (IsolatedBuildOptions, AssemblerBuildOptions,
ElasticsearchIndexOptions).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…alBaseUrl as Uri?

- IsolatedBuildOptions.CanonicalBaseUrl: string? -> Uri? with [Url]
  (argh maps [Url] on Uri? to [UriScheme("http","https")] constraint)
- IsolatedBuildService.Build: remove manual Uri.TryCreate; use the
  already-validated Uri? from argh binding with ?? default fallback
- ElasticsearchIndexOptions.Endpoint/ProxyAddress: [UriScheme("http","https")]
  -> [Url] for brevity (equivalent per argh generator)

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ectoryInfo

- ElasticsearchIndexOptions.CertificatePath: string? -> FileInfo? [FileExtensions("pem,der,crt,cer")]
- IsolatedBuildOptions.Path, Output: string? -> DirectoryInfo?
- Codex config params (all four commands): string -> FileInfo [FileExtensions("yml,yaml")]
- Codex output params: string? -> DirectoryInfo?
- DeployCommands: planFile -> FileInfo [json], @out -> FileInfo?, redirectsFile -> FileInfo? [json]
- CodexUpdateRedirectsCommand.redirectsFile: string? -> FileInfo? [json]
- NavigationCommands.ValidateLinkReference file: string? -> FileInfo? [json]
- InboundLinkCommands.ValidateLinkReference file: string? -> FileInfo? [json]
- BloomFilterCommands.Create builtDocsDir: string -> DirectoryInfo
- AssemblerCommands.Serve path: string? -> DirectoryInfo?
- ServeCommand path: string? -> DirectoryInfo?
- ChangelogCommand.Init path/changelogDir/bundlesDir: string? -> DirectoryInfo?
- ChangelogCommand config params (Add, Bundle, Remove, Render, GhRelease, Upload, EvaluatePr): string? -> FileInfo? [yml,yaml]
- ChangelogCommand directory params: string? -> DirectoryInfo?
- ChangelogCommand.BundleAmend bundlePath: string -> FileInfo [yml,yaml]
- Help text: path/file/dir params now show as <dir>, <file>, <path> with extension hints

Tilde expansion report (params needing future argh [ExpandTilde] attribute):
changelog init path/changelogDir/bundlesDir, all config params, all directory
params, assembler deploy planFile/@out, CertificatePath, build Path/Output.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…tSymbolicLinks], [ExpandUserProfile]

- Bump Nullean.Argh, Nullean.Argh.Hosting, Nullean.Argh.Interfaces to 0.11.0
- [Existing] on all input FileInfo/DirectoryInfo params that must be present:
  CertificatePath, all codex config files, changelog config/bundlePath, deploy
  planFile/redirectsFile, navigation/inbound-links file, bloom-filter builtDocsDir,
  serve path params, IsolatedBuildOptions.Path (source dir must exist)
- [RejectSymbolicLinks] on every FileInfo/DirectoryInfo param and property
- [ExpandUserProfile] on every FileInfo/DirectoryInfo param and property
  (fixes ~/path expansion for all path arguments)
- IsolatedBuildOptions.Output intentionally omitted from [Existing] (created by build)
- Codex and changelog output directories omitted from [Existing] (created by commands)

Help output now shows: [existing] [no symlinks] [expand ~ profile] on appropriate args.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Fixes:
- PR #28: DTO XML summaries now render in --help; global options show short
  flag first (e.g. -l, --log-level)
- PR #29: enum choices listed lowercase in help (html, elasticsearch, ...)
- PR #30: [Existing] on optional nullable FileInfo?/DirectoryInfo? no longer
  throws ArgumentNullException when the flag is omitted
- PR #31: MapAndRootAlias root help and leading flag handling fixed

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…Parameters]

Argh 0.12.0 correctly handles IReadOnlySet<T> in [AsParameters] DTOs but
initialises the collection to an empty HashSet (not null) when no --exporters
flags are supplied. The previous `??=` null-coalesce never fired on an empty
set, so IsolatedBuildService and AssemblerBuildService ran with zero exporters
(producing no output, no links.json).

Replace `??=` with an explicit count-based guard in both services.

Also enable [CollectionSyntax(Separator=",")] on IsolatedBuildOptions.Exporters
now that the [AsParameters] + [CollectionSyntax] generator bug is fixed in 0.12.0.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ed code

0.12.1 (PR #33) fixed IReadOnlySet<T>? defaulting to null when omitted in
[AsParameters] DTOs, but the generator emits the nullable null-return into a
non-nullable typed local variable (CS8600 / missing ? on the declaration),
making the project unbuildable. 0.12.2 does not fix this.

Pin to 0.12.0 and keep the is not { Count: > 0 } guard as a workaround until
the generator bug is resolved upstream. Remove [CollectionSyntax] comments
from DTOs (they were only relevant to the broken [AsParameters]+[CollectionSyntax]
combination which is now fixed in 0.12.0 anyway).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Fixes the CS8600 generator bug introduced in 0.12.1 where IReadOnlySet<T>? in
[AsParameters] DTOs was emitted with a non-nullable local type. Remove the
is not { Count: > 0 } workaround — ??= is correct again. Re-enable
[CollectionSyntax(Separator=",")] on IsolatedBuildOptions and AssemblerBuildOptions.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@Mpdreamz
Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

# Conflicts:
#	src/tooling/docs-builder/Commands/ChangelogCommand.cs

Co-authored-by: Mpdreamz <245275+Mpdreamz@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 30, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in 9706e56. The only conflict was in ChangelogCommand.cs — the main branch had added two new CI commands (prepare-artifact, evaluate-artifact). I kept the argh-style [NoOptionsInjection] attribute and ChangelogCommands class name, and adapted the new methods to use CancellationToken ct (argh pattern) instead of the CAF Cancel ctx.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/tooling/docs-builder/Commands/Assembler/ContentSourceCommands.cs (1)

51-56: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return the ShouldBuild result instead of hard-coding success.

ShouldBuild(...) is awaited and discarded here, so match will always exit 0 even when the repository/tag should be rejected.

♻️ Suggested fix
 		serviceInvoker.AddCommand(service, (repository, branchOrTag),
 			static async (s, collector, state, ctx) =>
 			{
-				_ = await s.ShouldBuild(collector, state.repository, state.branchOrTag, ctx);
-				return true;
+				return await s.ShouldBuild(collector, state.repository, state.branchOrTag, ctx);
 			});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tooling/docs-builder/Commands/Assembler/ContentSourceCommands.cs` around
lines 51 - 56, The lambda passed to serviceInvoker.AddCommand currently awaits
and discards the result of s.ShouldBuild, causing the command to always return
true; change the lambda in the AddCommand call so it returns the awaited boolean
result from s.ShouldBuild(...) instead of returning true — i.e., await and
return the value from ShouldBuild inside the static async (s, collector, state,
ctx) => { ... } block so the command's exit status reflects the ShouldBuild
decision.
src/services/Elastic.Documentation.Assembler/Building/AssemblerBuildService.cs (1)

55-55: ⚠️ Potential issue | 🔴 Critical

SetEquals will fail at runtime if exporters comes from options.Exporters without DocumentationState.

AssemblerBuildOptions.Exporters is IReadOnlySet<Exporter>?. At line 48, the ??= only assigns if null; otherwise exporters retains the IReadOnlySet type. If the condition at line 50 is false (no DocumentationState to strip), line 53 calls SetEquals on an IReadOnlySet, which lacks this method in the BCL.

♻️ Suggested fix
-		var elasticsearchExportOnly = exporters.SetEquals([Exporter.Elasticsearch]);
+		var elasticsearchExportOnly =
+			exporters.Count == 1 && exporters.Contains(Exporter.Elasticsearch);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/services/Elastic.Documentation.Assembler/Building/AssemblerBuildService.cs`
at line 55, The code calls exporters.SetEquals(...) but exporters is an
IReadOnlySet<Exporter>? (AssemblerBuildOptions.Exporters) so SetEquals will not
exist at runtime; fix by ensuring exporters is an ISet<Exporter> before calling
SetEquals—e.g., if you already create or mutate exporters when handling
DocumentationState, construct a concrete HashSet<Exporter> from
options.Exporters (or wrap null as empty) and then call
thatHashSet.SetEquals(new[]{Exporter.Elasticsearch}); update references to the
exporters variable used in the equality check (the exporters local and the
Exporter.Elasticsearch check) so the concrete SetEquals method is available.
src/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cs (1)

18-22: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Cancel the console event, not just the token.

This handler discards ConsoleCancelEventArgs, so Ctrl+C can still take down the process before the cancellation path runs.

🔧 Proposed fix
-		Console.CancelKeyPress += (_, _) =>
+		Console.CancelKeyPress += (_, e) =>
 		{
 			logger.LogInformation("Received CTRL+C cancelling");
+			e.Cancel = true;
 			_cancelKeyPressed = true;
 		};
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cs` around lines
18 - 22, The Console.CancelKeyPress handler currently ignores the
ConsoleCancelEventArgs which allows the process to be terminated by Ctrl+C;
update the handler attached to Console.CancelKeyPress so it accepts the
ConsoleCancelEventArgs parameter (e.g., (sender, args) => { ... }), set
args.Cancel = true to prevent the OS from terminating the process, and keep the
existing behavior of logging via logger.LogInformation and setting
_cancelKeyPressed = true so the graceful cancellation path runs (refer to the
Console.CancelKeyPress subscription, the lambda handler, logger.LogInformation,
and the _cancelKeyPressed flag).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cs`:
- Around line 142-147: The code calls ReadAllBytesAsync and
X509CertificateLoader.LoadCertificate even when the certificate path is missing
because EmitGlobalError in the options.CertificatePath check doesn't stop
execution; update the block around options.CertificatePath to short-circuit
after collector.EmitGlobalError (e.g., return from the method or skip the
certificate loading path) so that fileSystem.File.ReadAllBytesAsync and
X509CertificateLoader.LoadCertificate are not invoked when
fileSystem.File.Exists(options.CertificatePath.FullName) is false; keep the same
error emission using collector.EmitGlobalError and only proceed to call
ReadAllBytesAsync and assign cfg.Certificate when the file exists.
- Around line 150-152: The code truncates sub-minute TimeSpan values when
assigning options.BootstrapTimeout to cfg.BootstrapTimeout via
(int)options.BootstrapTimeout.Value.TotalMinutes; preserve precision by storing
seconds instead: compute (int)options.BootstrapTimeout.Value.TotalSeconds (or
change cfg.BootstrapTimeout to a TimeSpan) so inputs like "30s" or "90s" are
represented accurately; update any downstream consumers or property
name/comments for cfg.BootstrapTimeout to reflect that the value is in seconds
if you choose the seconds approach.

In `@src/Elastic.Documentation/GlobalCommandLine.cs`:
- Around line 23-27: The parsing branch for the
--config-source/--configuration-source/-c flag currently ignores invalid values
because ConfigurationSourceExtensions.TryParse silently fails; update the branch
in GlobalCommandLine.cs so that if TryParse(args[i + 1], out var cs, true, true)
returns false you reject the input (e.g., log a clear error and exit or throw a
UsageException) instead of leaving the default, otherwise continue setting
options = options with { ConfigurationSource = cs } and incrementing i; this
change ensures invalid config-source values are surfaced immediately.

In `@src/services/Elastic.Documentation.Isolated/IsolatedBuildService.cs`:
- Around line 77-78: Restore the absolute-URI validation when assigning
canonicalBaseUri in IsolatedBuildService (so BuildContext never gets relative
Uris): perform a Uri.TryCreate(inputValue, UriKind.Absolute, out var
canonicalBaseUri) (or validate the incoming canonicalBaseUri parameter with
UriKind.Absolute) and only set canonicalBaseUri to the default new
Uri("https://docs-v3-preview.elastic.dev") or the parsed Uri when the TryCreate
succeeds; ensure callers that pass invalid/relative values are rejected or fall
back to the absolute default so BuildContext only ever receives absolute Uris.

In `@src/tooling/docs-builder/Commands/ChangelogCommand.cs`:
- Around line 87-89: The default changelog and bundles directories are being
resolved from the current working directory instead of the repository docs
folder; update the logic in ChangelogCommand where configPath, changelogPath and
bundlesPath are computed so that when changelogDir or bundlesDir are null you
join them to docsFolder.FullName (e.g. use
_fileSystem.Path.Join(docsFolder.FullName, "changelog") and
_fileSystem.Path.Join(docsFolder.FullName, "releases")) so the default paths
match where changelog.yml is written; apply the same fix to the other occurrence
mentioned (the block around lines 185-205) so all defaults consistently resolve
relative to docsFolder rather than cwd.

In `@src/tooling/docs-builder/Commands/InboundLinkCommands.cs`:
- Around line 59-65: The [Existing] validation on the ValidateLinkReference
method's file parameter blocks null so the fallback in CheckWithLocalLinksJson
never runs; edit ValidateLinkReference to allow null by removing the [Existing]
attribute (or replace it with an attribute variant that permits null if
available) so that ValidateLinkReference(FileInfo? file = null, ...) can pass a
null through to _linkIndexService.CheckWithLocalLinksJson which will apply file
??= ".artifacts/docs/html/links.json"; ensure the parameter signature and any
related attribute usages on ValidateLinkReference are updated accordingly.

In `@src/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cs`:
- Around line 34-37: Await the collector startup before emitting or stopping it:
change the fire-and-forget call to await
collector.StartAsync(context.CancellationToken) so that EmitGlobalError and
StopAsync run only after StartAsync completes; ensure you still await StopAsync
afterwards and preserve setting context.ExitCode = 1, referencing
collector.StartAsync, collector.EmitGlobalError, collector.StopAsync and
context.ExitCode.

In `@src/tooling/docs-builder/Middleware/CheckForUpdatesMiddleware.cs`:
- Around line 27-31: GetLatestVersion can throw and HttpClient/response are not
disposed, so make the update check best-effort: wrap the call to
GetLatestVersion (and the subsequent CompareWithAssemblyVersion) in a try/catch
that logs the exception (use _logger.LogDebug or LogWarning) but does not
rethrow, ensuring the middleware does not fail the command; inside
GetLatestVersion ensure HTTP resources are disposed (use using/using var for
HttpClient and HttpResponseMessage and dispose any streams/readers used) and
catch file/IO/HTTP exceptions there as well or let them bubble to the outer try
so they are logged and ignored rather than breaking execution.

---

Outside diff comments:
In
`@src/services/Elastic.Documentation.Assembler/Building/AssemblerBuildService.cs`:
- Line 55: The code calls exporters.SetEquals(...) but exporters is an
IReadOnlySet<Exporter>? (AssemblerBuildOptions.Exporters) so SetEquals will not
exist at runtime; fix by ensuring exporters is an ISet<Exporter> before calling
SetEquals—e.g., if you already create or mutate exporters when handling
DocumentationState, construct a concrete HashSet<Exporter> from
options.Exporters (or wrap null as empty) and then call
thatHashSet.SetEquals(new[]{Exporter.Elasticsearch}); update references to the
exporters variable used in the equality check (the exporters local and the
Exporter.Elasticsearch check) so the concrete SetEquals method is available.

In `@src/tooling/docs-builder/Commands/Assembler/ContentSourceCommands.cs`:
- Around line 51-56: The lambda passed to serviceInvoker.AddCommand currently
awaits and discards the result of s.ShouldBuild, causing the command to always
return true; change the lambda in the AddCommand call so it returns the awaited
boolean result from s.ShouldBuild(...) instead of returning true — i.e., await
and return the value from ShouldBuild inside the static async (s, collector,
state, ctx) => { ... } block so the command's exit status reflects the
ShouldBuild decision.

In `@src/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cs`:
- Around line 18-22: The Console.CancelKeyPress handler currently ignores the
ConsoleCancelEventArgs which allows the process to be terminated by Ctrl+C;
update the handler attached to Console.CancelKeyPress so it accepts the
ConsoleCancelEventArgs parameter (e.g., (sender, args) => { ... }), set
args.Cancel = true to prevent the OS from terminating the process, and keep the
existing behavior of logging via logger.LogInformation and setting
_cancelKeyPressed = true so the graceful cancellation path runs (refer to the
Console.CancelKeyPress subscription, the lambda handler, logger.LogInformation,
and the _cancelKeyPressed flag).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b408d686-9799-4f94-9392-3c097980ab2f

📥 Commits

Reviewing files that changed from the base of the PR and between 546c9e6 and 9706e56.

📒 Files selected for processing (51)
  • Directory.Packages.props
  • aspire/AppHost.cs
  • aspire/aspire.csproj
  • src/Elastic.Documentation.Configuration/Elastic.Documentation.Configuration.csproj
  • src/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cs
  • src/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cs
  • src/Elastic.Documentation/GlobalCommandLine.cs
  • src/services/Elastic.Documentation.Assembler/Building/AssemblerBuildOptions.cs
  • src/services/Elastic.Documentation.Assembler/Building/AssemblerBuildService.cs
  • src/services/Elastic.Documentation.Assembler/Building/AssemblerSitemapService.cs
  • src/services/Elastic.Documentation.Assembler/Elastic.Documentation.Assembler.csproj
  • src/services/Elastic.Documentation.Assembler/Indexing/AssemblerIndexService.cs
  • src/services/Elastic.Documentation.Assembler/Sourcing/AssemblerCloneOptions.cs
  • src/services/Elastic.Documentation.Assembler/Sourcing/AssemblerCloneService.cs
  • src/services/Elastic.Documentation.Isolated/Elastic.Documentation.Isolated.csproj
  • src/services/Elastic.Documentation.Isolated/IsolatedBuildOptions.cs
  • src/services/Elastic.Documentation.Isolated/IsolatedBuildService.cs
  • src/services/Elastic.Documentation.Isolated/IsolatedIndexService.cs
  • src/tooling/docs-builder/Arguments/ExportOption.cs
  • src/tooling/docs-builder/Arguments/ProductInfoParser.cs
  • src/tooling/docs-builder/Commands/Assembler/AssemblerCommands.cs
  • src/tooling/docs-builder/Commands/Assembler/AssemblerIndexCommand.cs
  • src/tooling/docs-builder/Commands/Assembler/AssemblerSitemapCommand.cs
  • src/tooling/docs-builder/Commands/Assembler/BloomFilterCommands.cs
  • src/tooling/docs-builder/Commands/Assembler/ConfigurationCommands.cs
  • src/tooling/docs-builder/Commands/Assembler/ContentSourceCommands.cs
  • src/tooling/docs-builder/Commands/Assembler/DeployCommands.cs
  • src/tooling/docs-builder/Commands/Assembler/NavigationCommands.cs
  • src/tooling/docs-builder/Commands/ChangelogCommand.cs
  • src/tooling/docs-builder/Commands/Codex/CodexCommands.cs
  • src/tooling/docs-builder/Commands/Codex/CodexIndexCommand.cs
  • src/tooling/docs-builder/Commands/Codex/CodexUpdateRedirectsCommand.cs
  • src/tooling/docs-builder/Commands/DiffCommands.cs
  • src/tooling/docs-builder/Commands/FormatCommand.cs
  • src/tooling/docs-builder/Commands/InboundLinkCommands.cs
  • src/tooling/docs-builder/Commands/IndexCommand.cs
  • src/tooling/docs-builder/Commands/IsolatedBuildCommand.cs
  • src/tooling/docs-builder/Commands/MoveCommand.cs
  • src/tooling/docs-builder/Commands/ServeCommand.cs
  • src/tooling/docs-builder/DocumentationTooling.cs
  • src/tooling/docs-builder/Filters/InfoLoggerFilter.cs
  • src/tooling/docs-builder/Filters/ReplaceLogFilter.cs
  • src/tooling/docs-builder/Filters/StopwatchFilter.cs
  • src/tooling/docs-builder/GlobalCliOptions.cs
  • src/tooling/docs-builder/Http/InMemoryBuildState.cs
  • src/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cs
  • src/tooling/docs-builder/Middleware/CheckForUpdatesMiddleware.cs
  • src/tooling/docs-builder/Middleware/InfoLoggerMiddleware.cs
  • src/tooling/docs-builder/Middleware/StopwatchMiddleware.cs
  • src/tooling/docs-builder/Program.cs
  • src/tooling/docs-builder/docs-builder.csproj
💤 Files with no reviewable changes (6)
  • src/tooling/docs-builder/DocumentationTooling.cs
  • src/tooling/docs-builder/Filters/StopwatchFilter.cs
  • src/tooling/docs-builder/Filters/InfoLoggerFilter.cs
  • src/tooling/docs-builder/Filters/ReplaceLogFilter.cs
  • src/tooling/docs-builder/Arguments/ExportOption.cs
  • src/tooling/docs-builder/Commands/FormatCommand.cs

Comment thread src/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cs Outdated
Comment thread src/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cs Outdated
Comment thread src/Elastic.Documentation/GlobalCommandLine.cs Outdated
Comment thread src/services/Elastic.Documentation.Isolated/IsolatedBuildService.cs
Comment thread src/tooling/docs-builder/Commands/ChangelogCommand.cs Outdated
Comment thread src/tooling/docs-builder/Commands/InboundLinkCommands.cs
Comment thread src/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cs
Comment thread src/tooling/docs-builder/Middleware/CheckForUpdatesMiddleware.cs Outdated
…tionLogging

IsMcpMode always returned false (no mcp command registered in argh); remove it
and the noConsole parameter it drove. GlobalCommandLine.cs is now minimal startup
plumbing only — ScanArgs + GlobalCliArgs record.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Elastic.Documentation/GlobalCommandLine.cs (1)

38-48: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make log-level parsing case-insensitive and reject unknown values.

ParseLogLevel only matches lowercase strings today, so --log-level Warning or --log-level Error silently becomes Information. That gives you the wrong startup log level and hides input mistakes.

Suggested fix
-private static LogLevel ParseLogLevel(string? logLevel) => logLevel switch
-{
-	"trace" => LogLevel.Trace,
-	"debug" => LogLevel.Debug,
-	"information" => LogLevel.Information,
-	"info" => LogLevel.Information,
-	"warning" => LogLevel.Warning,
-	"error" => LogLevel.Error,
-	"critical" => LogLevel.Critical,
-	_ => LogLevel.Information
-};
+private static LogLevel ParseLogLevel(string? logLevel)
+{
+	if (string.IsNullOrWhiteSpace(logLevel))
+		return LogLevel.Information;
+
+	return logLevel.Trim().ToLowerInvariant() switch
+	{
+		"trace" => LogLevel.Trace,
+		"debug" => LogLevel.Debug,
+		"information" or "info" => LogLevel.Information,
+		"warning" => LogLevel.Warning,
+		"error" => LogLevel.Error,
+		"critical" => LogLevel.Critical,
+		_ => throw new ArgumentException($"Invalid log level: {logLevel}")
+	};
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Elastic.Documentation/GlobalCommandLine.cs` around lines 38 - 48,
ParseLogLevel currently only matches lowercase strings and silently defaults
unknown values to Information; change it to perform a case-insensitive parse and
reject invalid inputs. Replace the manual switch in ParseLogLevel with a
case-insensitive Enum.TryParse<LogLevel>(...) call (or Enum.Parse with
ignoreCase) against LogLevel and, if parsing succeeds, return the parsed
LogLevel; if it fails, throw an ArgumentException (or similar) describing the
invalid log-level input so unknown values are not silently accepted.
♻️ Duplicate comments (1)
src/Elastic.Documentation/GlobalCommandLine.cs (1)

26-30: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reject invalid --config-source values.

An explicit bad value still leaves the default source in place, so startup can read from the wrong configuration before the main CLI parser reports anything.

Suggested fix
 			else if (args[i] is "--config-source" or "--configuration-source" or "-c" && i + 1 < args.Length)
 			{
-				if (ConfigurationSourceExtensions.TryParse(args[i + 1], out var cs, true, true))
-					options = options with { ConfigurationSource = cs };
+				if (!ConfigurationSourceExtensions.TryParse(args[i + 1], out var cs, true, true))
+					throw new ArgumentException($"Invalid configuration source: {args[i + 1]}");
+				options = options with { ConfigurationSource = cs };
 				i++;
 			}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Elastic.Documentation/GlobalCommandLine.cs` around lines 26 - 30, The
args parsing currently leaves the default ConfigurationSource when
ConfigurationSourceExtensions.TryParse(...) fails; change the logic in
GlobalCommandLine.cs (the block handling
"--config-source"/"--configuration-source"/"-c") so that if TryParse returns
false you treat it as an invalid argument: emit a clear error message and abort
startup (return non-zero / throw / exit) instead of silently keeping the
default; keep the successful branch that sets options = options with {
ConfigurationSource = cs } and still advance i when handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/Elastic.Documentation/GlobalCommandLine.cs`:
- Around line 38-48: ParseLogLevel currently only matches lowercase strings and
silently defaults unknown values to Information; change it to perform a
case-insensitive parse and reject invalid inputs. Replace the manual switch in
ParseLogLevel with a case-insensitive Enum.TryParse<LogLevel>(...) call (or
Enum.Parse with ignoreCase) against LogLevel and, if parsing succeeds, return
the parsed LogLevel; if it fails, throw an ArgumentException (or similar)
describing the invalid log-level input so unknown values are not silently
accepted.

---

Duplicate comments:
In `@src/Elastic.Documentation/GlobalCommandLine.cs`:
- Around line 26-30: The args parsing currently leaves the default
ConfigurationSource when ConfigurationSourceExtensions.TryParse(...) fails;
change the logic in GlobalCommandLine.cs (the block handling
"--config-source"/"--configuration-source"/"-c") so that if TryParse returns
false you treat it as an invalid argument: emit a clear error message and abort
startup (return non-zero / throw / exit) instead of silently keeping the
default; keep the successful branch that sets options = options with {
ConfigurationSource = cs } and still advance i when handled.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 77960681-8f54-4dc0-9641-7dc7276b4dee

📥 Commits

Reviewing files that changed from the base of the PR and between 9706e56 and 67f4535.

📒 Files selected for processing (2)
  • src/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cs
  • src/Elastic.Documentation/GlobalCommandLine.cs

…lobalCliArgs

GlobalCliArgs and GlobalCommandLine.cs were a workaround so AppDefaultsExtensions
(ServiceDefaults) could scan args before the argh host builds without referencing
docs-builder. Now GlobalCliOptions lives in Elastic.Documentation as a plain class,
docs-builder calls TryParseArgh before host construction and passes the result to
AddDocumentationServiceDefaults, and all manual arg scanning + ParseLogLevel are gone.

Also fixes from inline review: certificate loading short-circuits on missing file,
BootstrapTimeout stored as TimeSpan, changelog default dirs resolve relative to
docsFolder, Ctrl+C sets args.Cancel, update check wrapped in try/catch, HttpClient
disposed with using.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Adds a convenience overload that accepts only a configure action, so
callers that don't need to supply CLI options no longer need the named
argument workaround. Updates integration tests and Api.App accordingly.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Allows argh to distinguish between "not provided" and "explicitly set to
Information", so the default can be applied at the consumption site.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
0.13.1 fixes non-nullable [AsParameters] / global-options properties with
C# initializers being incorrectly required. LogLevel can now be non-nullable
with = LogLevel.Information and argh will honour it when the flag is absent,
removing the .GetValueOrDefault workaround in AppDefaultsExtensions.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cs`:
- Around line 124-125: The code only sets cfg.NoElasticInferenceService when
options.Eis == false, which ignores explicit true values; update the logic to
honor both explicit true and false by setting cfg.NoElasticInferenceService =
true when options.Eis == false and setting cfg.NoElasticInferenceService = false
when options.Eis == true (do the same for options.AiEnrichment /
cfg.NoAiEnrichment) so CLI-provided true/false overrides config-file values;
locate usages of options.Eis and options.AiEnrichment and adjust to check for ==
true as well as == false and assign the corresponding cfg properties.

In `@src/tooling/docs-builder/Commands/Assembler/ContentSourceCommands.cs`:
- Line 41: The Match command currently always returns success despite the XML
doc; update the Match implementation in ContentSourceCommands (the Match method)
to use the computed ShouldBuild value instead of unconditionally returning
true—i.e., return/exit with success when ShouldBuild is true and failure when
ShouldBuild is false (map the boolean to exit code 0/1 as the doc states) so the
command's behavior matches its documented contract.

In `@src/tooling/docs-builder/Middleware/CheckForUpdatesMiddleware.cs`:
- Around line 23-33: After awaiting next(context) in CheckForUpdatesMiddleware,
skip the version lookup when the command was canceled or failed by returning
early if context.CancellationToken.IsCancellationRequested or context.ExitCode
!= 0; update the middleware's InvokeAsync (or the method containing await
next(context)) to check those two conditions before calling GetLatestVersion or
CompareWithAssemblyVersion so update checks only run for successful,
non-canceled invocations.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7c84d7ea-03e5-4962-a09a-fb5956529a9a

📥 Commits

Reviewing files that changed from the base of the PR and between 67f4535 and 3b47b50.

📒 Files selected for processing (15)
  • Directory.Packages.props
  • src/Elastic.Documentation.Configuration/DocumentationEndpoints.cs
  • src/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cs
  • src/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cs
  • src/Elastic.Documentation/GlobalCliOptions.cs
  • src/Elastic.Documentation/GlobalCommandLine.cs
  • src/api/Elastic.Documentation.Api.App/Program.cs
  • src/api/Elastic.Documentation.Mcp.Remote/Program.cs
  • src/tooling/docs-builder/Commands/Assembler/ContentSourceCommands.cs
  • src/tooling/docs-builder/Commands/ChangelogCommand.cs
  • src/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cs
  • src/tooling/docs-builder/Middleware/CheckForUpdatesMiddleware.cs
  • src/tooling/docs-builder/Program.cs
  • tests-integration/Elastic.Assembler.IntegrationTests/NavigationBuildingTests.cs
  • tests-integration/Elastic.Assembler.IntegrationTests/NavigationRootTests.cs
💤 Files with no reviewable changes (1)
  • src/Elastic.Documentation/GlobalCommandLine.cs

Comment thread src/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cs Outdated
Comment thread src/tooling/docs-builder/Middleware/CheckForUpdatesMiddleware.cs
…tation

0.14.0 fixes two CI failures:
- unknown short option '-p': argh now correctly reads short aliases from
  referenced-assembly XML doc files
- missing required flag --log-level: UseGlobalOptions<T> properties with
  C# initializers are no longer treated as required (same fix as 0.13.1's
  [AsParameters] fix, now applied to global options too)

GenerateDocumentationFile is added to Elastic.Documentation so argh can
read the -l and -c short aliases from GlobalCliOptions at generate time.
The same NoWarn pattern (CS1591 etc.) already used in three other argh
DTO projects is applied here.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Fixes the CI failures: 'missing required flag --log-level' and 'unknown
short option -p' no longer appear when global options are omitted.
Skipped 0.14.1 which introduced 102 CS8600 compile errors.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
… integration tests

0.15.1 relaxes TryParseArgh so unknown flags no longer cause the DTO
pre-parser to fail. Previously, passing command-specific flags like
--assume-cloned alongside global flags caused TryParseArgh to return
false, falling back to new GlobalCliOptions() with SkipPrivateRepositories=false,
which caused the assembler to attempt cloning private repositories.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
ElasticsearchEndpointConfigurator now applies both directions of the Eis
and AiEnrichment flags so a CLI --eis overrides any config-file value
that set NoElasticInferenceService=true, and vice versa.

CheckForUpdatesMiddleware skips the version lookup when the command was
canceled or exited non-zero — no point advertising an update to a user
whose command failed.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants