feat(tooling): migrate docs-builder CLI from ConsoleAppFramework to Nullean.Argh#3202
feat(tooling): migrate docs-builder CLI from ConsoleAppFramework to Nullean.Argh#3202
Conversation
…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>
…s in help, AGH0033)
# Conflicts: # src/tooling/docs-builder/Commands/ChangelogCommand.cs Co-authored-by: Mpdreamz <245275+Mpdreamz@users.noreply.github.com>
Resolved in 9706e56. The only conflict was in |
There was a problem hiding this comment.
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 winReturn the
ShouldBuildresult instead of hard-coding success.
ShouldBuild(...)is awaited and discarded here, somatchwill 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
SetEqualswill fail at runtime ifexporterscomes fromoptions.ExporterswithoutDocumentationState.
AssemblerBuildOptions.ExportersisIReadOnlySet<Exporter>?. At line 48, the??=only assigns if null; otherwiseexportersretains theIReadOnlySettype. If the condition at line 50 is false (noDocumentationStateto strip), line 53 callsSetEqualson anIReadOnlySet, 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 winCancel 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
📒 Files selected for processing (51)
Directory.Packages.propsaspire/AppHost.csaspire/aspire.csprojsrc/Elastic.Documentation.Configuration/Elastic.Documentation.Configuration.csprojsrc/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cssrc/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cssrc/Elastic.Documentation/GlobalCommandLine.cssrc/services/Elastic.Documentation.Assembler/Building/AssemblerBuildOptions.cssrc/services/Elastic.Documentation.Assembler/Building/AssemblerBuildService.cssrc/services/Elastic.Documentation.Assembler/Building/AssemblerSitemapService.cssrc/services/Elastic.Documentation.Assembler/Elastic.Documentation.Assembler.csprojsrc/services/Elastic.Documentation.Assembler/Indexing/AssemblerIndexService.cssrc/services/Elastic.Documentation.Assembler/Sourcing/AssemblerCloneOptions.cssrc/services/Elastic.Documentation.Assembler/Sourcing/AssemblerCloneService.cssrc/services/Elastic.Documentation.Isolated/Elastic.Documentation.Isolated.csprojsrc/services/Elastic.Documentation.Isolated/IsolatedBuildOptions.cssrc/services/Elastic.Documentation.Isolated/IsolatedBuildService.cssrc/services/Elastic.Documentation.Isolated/IsolatedIndexService.cssrc/tooling/docs-builder/Arguments/ExportOption.cssrc/tooling/docs-builder/Arguments/ProductInfoParser.cssrc/tooling/docs-builder/Commands/Assembler/AssemblerCommands.cssrc/tooling/docs-builder/Commands/Assembler/AssemblerIndexCommand.cssrc/tooling/docs-builder/Commands/Assembler/AssemblerSitemapCommand.cssrc/tooling/docs-builder/Commands/Assembler/BloomFilterCommands.cssrc/tooling/docs-builder/Commands/Assembler/ConfigurationCommands.cssrc/tooling/docs-builder/Commands/Assembler/ContentSourceCommands.cssrc/tooling/docs-builder/Commands/Assembler/DeployCommands.cssrc/tooling/docs-builder/Commands/Assembler/NavigationCommands.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cssrc/tooling/docs-builder/Commands/Codex/CodexCommands.cssrc/tooling/docs-builder/Commands/Codex/CodexIndexCommand.cssrc/tooling/docs-builder/Commands/Codex/CodexUpdateRedirectsCommand.cssrc/tooling/docs-builder/Commands/DiffCommands.cssrc/tooling/docs-builder/Commands/FormatCommand.cssrc/tooling/docs-builder/Commands/InboundLinkCommands.cssrc/tooling/docs-builder/Commands/IndexCommand.cssrc/tooling/docs-builder/Commands/IsolatedBuildCommand.cssrc/tooling/docs-builder/Commands/MoveCommand.cssrc/tooling/docs-builder/Commands/ServeCommand.cssrc/tooling/docs-builder/DocumentationTooling.cssrc/tooling/docs-builder/Filters/InfoLoggerFilter.cssrc/tooling/docs-builder/Filters/ReplaceLogFilter.cssrc/tooling/docs-builder/Filters/StopwatchFilter.cssrc/tooling/docs-builder/GlobalCliOptions.cssrc/tooling/docs-builder/Http/InMemoryBuildState.cssrc/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cssrc/tooling/docs-builder/Middleware/CheckForUpdatesMiddleware.cssrc/tooling/docs-builder/Middleware/InfoLoggerMiddleware.cssrc/tooling/docs-builder/Middleware/StopwatchMiddleware.cssrc/tooling/docs-builder/Program.cssrc/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
…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>
There was a problem hiding this comment.
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 winMake log-level parsing case-insensitive and reject unknown values.
ParseLogLevelonly matches lowercase strings today, so--log-level Warningor--log-level Errorsilently becomesInformation. 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 winReject invalid
--config-sourcevalues.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
📒 Files selected for processing (2)
src/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cssrc/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>
There was a problem hiding this comment.
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
📒 Files selected for processing (15)
Directory.Packages.propssrc/Elastic.Documentation.Configuration/DocumentationEndpoints.cssrc/Elastic.Documentation.Configuration/ElasticsearchEndpointConfigurator.cssrc/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cssrc/Elastic.Documentation/GlobalCliOptions.cssrc/Elastic.Documentation/GlobalCommandLine.cssrc/api/Elastic.Documentation.Api.App/Program.cssrc/api/Elastic.Documentation.Mcp.Remote/Program.cssrc/tooling/docs-builder/Commands/Assembler/ContentSourceCommands.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cssrc/tooling/docs-builder/Middleware/CatchExceptionMiddleware.cssrc/tooling/docs-builder/Middleware/CheckForUpdatesMiddleware.cssrc/tooling/docs-builder/Program.cstests-integration/Elastic.Assembler.IntegrationTests/NavigationBuildingTests.cstests-integration/Elastic.Assembler.IntegrationTests/NavigationRootTests.cs
💤 Files with no reviewable changes (1)
- src/Elastic.Documentation/GlobalCommandLine.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>
cotti
left a comment
There was a problem hiding this comment.
Looks good (the bliss of having DTOs on entry...), small caveat to be solved outside. Argh bug:
dotnet run --project src/tooling/docs-builder -- build --canonical-base-url not-an-url
Crashes with an exception instead of a clean error exit.
Otherwise things worked well here.
Why
docs-builderused 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
--endpointrequires an http/https URI, that--plan-filemust exist on disk, or that--exporters Html,Elasticsearchis 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 realassembler --helppage that showed only assembler sub-commands, no per-namespace help, and no way to document what theassemblernamespace 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,changelogetc. are now first-class namespaces.docs-builder assembler --helpshows 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.--helpno 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 —
--endpointand--proxy-addressareUri?with[Url](DataAnnotations); argh rejects non-http/https values at parse time.Timeouts —
--bootstrap-timeoutisTimeSpan?with[TimeSpanRange("1s","60m")]. Users pass4mor90s, 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
FileInfoorDirectoryInfo(notstring). 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~/pathworks 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
ExporterParserwith its own vocabulary (es,config,links…). Now nativeIReadOnlySet<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 introducesIsolatedBuildOptions,AssemblerBuildOptions,AssemblerCloneOptions, and the already-existingElasticsearchIndexOptionsas proper records that:[AsParameters](one line in the command method)docs-builder buildas a named commanddocs-builder(no sub-command) still builds the current documentation set.docs-builder buildnow also works as an explicit named command (MapAndRootAlias).assembleconsolidationThe old
assemble(one-shot) andassembler(individual steps) namespace split is clarified:docs-builder assembleruns config-init + clone + build in one shot;docs-builder assembler clone/build/serve/index/sitemapare the individual steps.Breaking changes
--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 wasstringFileInfo— path must resolve to a real fileExporter names (
html,es,config,links) are not breaking —Enum.TryParseis case-insensitive sohtmlandHtmlboth work.argh bugs found and fixed upstream
During this migration five bugs were found and fixed in Nullean.Argh (0.7→0.11):
--help{identifier}in XML doc text emitted as C# interpolation holes in generated codebool?+bool no<Name>generating duplicate--no-*switch arms (CS8510)__evreused across multiple enum properties in the same options type (CS0136)[AsParameters]DTO property<summary>XML docs not rendered in--helpfor types in referenced assembliesOne known bug remains open:
[Existing]on a nullableFileInfo?/DirectoryInfo?fires its existence check even when the value is absent (null), causingArgumentNullExceptioninside the generated runner.Test plan
dotnet build— 0 errors, 0 warningsdocs-builder --help— root help, no startup logs,(default: build)alias showndocs-builder build --help— all options have descriptions,[existing]on--pathdocs-builder assembler --help— namespace summaries for all sub-namespacesdocs-builder assembler index --help— ES options show[schemes: http|https],[time-span-range],[range]hintsdocs-builder assembler deploy apply --help—--plan-fileshows[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 errordocs-builder build --bootstrap-timeout bad— argh rejects with time-span errordocs-builder codex --help— codex concept explained in namespace summarydocs-builder changelog --help— all sub-command summaries are one terse line