Skip to content

Add first-class pipeline CLI options - #3577

Merged
thomhurst merged 5 commits into
mainfrom
issue-3537-cli
Jul 30, 2026
Merged

Add first-class pipeline CLI options#3577
thomhurst merged 5 commits into
mainfrom
issue-3537-cli

Conversation

@thomhurst

@thomhurst thomhurst commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • add built-in --list-modules, --module, --skip-module, category, and --validate options
  • execute targeted modules with their transitive dependency closure while preserving skip/category behavior
  • preserve assembly-qualified module names and keep validation free of execution-time condition evaluation
  • forward unknown arguments to host configuration and provide an opt-out
  • expose programmatic module targeting and document the CLI

Validation

  • PipelineCommandLineTests: 15 passed
  • DependencyInjectionTests: 3 passed
  • ModuleRetrieverTests: 1 passed
  • ModularPipelines.sln Release build: 0 errors (227 existing analyzer warnings)
  • Docusaurus production build: passed (existing documentation warnings)

Closes #3537

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c56ed16af5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/CommandLine/PipelineCommandHandler.cs
Comment thread src/ModularPipelines/CommandLine/PipelineCommandHandler.cs Outdated
@thomhurst

Copy link
Copy Markdown
Owner Author

Addressed both Codex findings in 0496ac5: informational commands now invoke registration events and validate the finalized dynamic dependency graph; module listing reads categories from finalized metadata. Added regression coverage for registration-time missing dependencies and fluent categories. Validation: PipelineCommandLineTests 13/13; core Release build 0 errors. @codex review

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0496ac58f3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/CommandLine/PipelineCommandLineParser.cs Outdated
Comment thread src/ModularPipelines/Validation/ModuleSelectionValidator.cs
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Code review

Reviewed the CLI targeting/validation feature (--list-modules, --module, --skip-module, --validate, categories) against CLAUDE.md and for correctness. The prior @codex findings (registration-time events for informational commands, categories read from finalized metadata) look addressed in 0496ac5.

CLAUDE.md compliance: No violations found. No auto-generated options classes were touched, the Module<T>/[DependsOn<T>]/ExecuteAsync pattern is untouched, and new files land under the documented directories (src/ModularPipelines/CommandLine, Engine, Validation, Exceptions, test/ModularPipelines.UnitTests, docs/).

Bug — broken DI graph for PipelineCommandHandler (confirmed, reproduced against an existing test)

{
services
.AddSingleton<PipelineCommandHandler>()
.Configure<PipelineOptions>(_ => { })
.Configure<SchedulerOptions>(_ => { })

RegisterBundledServices now does .AddSingleton<PipelineCommandHandler>(), but PipelineCommandHandler's constructor requires a PipelineCommandLineOptions instance:

internal sealed class PipelineCommandHandler(
PipelineCommandLineOptions commandLineOptions,
IEnumerable<IModule> modules,
IDependencyChainProvider dependencyChainProvider,
IRegistrationEventExecutor registrationEventExecutor,
IModuleDependencyRegistry dependencyRegistry,
IModuleMetadataRegistry metadataRegistry,
IConsoleWriter consoleWriter)
{

That type is only registered later, inside PipelineBuilder.BuildPipelineAsync:

services
.AddSingleton(_commandLineOptions)
.AddSingleton(_options)
.AddTransient<IOptionsFactory<PipelineOptions>, PipelineOptionsFactory>();

So any service provider built directly from DependencyInjectionSetup.Initialize(...) — which is exactly what the existing DependencyInjectionTests.Validate test does with ValidateOnBuild = true — now fails to resolve. I reran that test against this branch and it fails with:

Unable to resolve service for type 'ModularPipelines.PipelineCli.PipelineCommandLineOptions'
while attempting to activate 'ModularPipelines.PipelineCli.PipelineCommandHandler'.

This wasn't caught by the PR's stated validation (PipelineCommandLineTests, ModuleRetrieverTests) because that test lives in DependencyInjectionTests instead.

Why this matters architecturally, not just as a one-line fix: DependencyInjectionSetup.Initialize is treated elsewhere in the codebase (and by this existing test) as a contract that must produce a fully resolvable service graph on its own, independent of PipelineBuilder's instance-specific wiring. PipelineOptions already follows that contract correctly — it's registered with .Configure<PipelineOptions>(_ => { }) in DependencyInjectionSetup so a default always exists, and PipelineBuilder layers the real instance data on top via AddSingleton(_options). PipelineCommandLineOptions should follow the same pattern (e.g. register a default/no-op instance in DependencyInjectionSetup, or make PipelineCommandHandler's dependency resolve via IOptions<T> with a registered default) rather than relying on PipelineBuilder to be the only caller that ever constructs this graph. Otherwise every future singleton added to RegisterBundledServices that depends on builder-supplied instance data reintroduces this same class of break, and it'll keep slipping through unless the affected test happens to be in a reviewer's run list.

Fix suggestion: register a default PipelineCommandLineOptions (e.g. PipelineCommandLineOptions.Empty) in DependencyInjectionSetup.RegisterBundledServices, letting PipelineBuilder's later AddSingleton(_commandLineOptions) win as the final registration for the real pipeline run — mirroring how PipelineOptions is handled today.

No other high-signal bugs or security issues were found in the introduced code (arg parsing, dependency-closure resolution in ModuleSelection, and the new validator all checked out).

@thomhurst

Copy link
Copy Markdown
Owner Author

Fixed the failing core CI test by registering PipelineCommandLineOptions.Empty as the bundled DI fallback; the parsed builder instance still overrides it. DI tests 3/3, CLI tests 13/13, formatter, and core Release build pass.

@codex review

@thomhurst

Copy link
Copy Markdown
Owner Author

Addressed both review findings: assembly-qualified selections remain a single value, and selection validation now resolves registration metadata without evaluating run conditions. CLI tests 15/15 and core Release build pass.

@codex review

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Code review (follow-up)

Re-reviewed at HEAD 62a370d1f7fa28cc36433d7c4e5ef0af5c9d72d3 (commits 26c6e4dcae, 0496ac58f3, c56ed16af5 since the last pass).

Previously flagged DI bug is fixed and now regression-tested. DependencyInjectionSetup.RegisterBundledServices registers services.TryAddSingleton(PipelineCommandLineOptions.Empty) before AddSingleton<PipelineCommandHandler>() (src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs:93-96), so the bundled service graph resolves standalone, and PipelineBuilder's later AddSingleton(_commandLineOptions) (PipelineBuilder.cs:749) still wins for a real run — exactly mirroring how PipelineOptions is handled. DependencyInjectionTests.Validate now explicitly does serviceProvider.GetRequiredService<PipelineCommandHandler>(), so this class of break (a bundled singleton depending on builder-only instance data) won't silently regress again.

Both Codex findings hold up:

  • Assembly-qualified module names stay intact as a single value — PipelineCommandLineParser.IsAssemblyQualifiedModuleName sniffs for Version=/Culture=/PublicKeyToken= segments before splitting on commas (PipelineCommandLineParser.cs:99-114), so --module Foo, Bar, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null isn't shredded by the generic comma-split.
  • ModuleSelectionValidator/ModuleRetriever.ValidateSelectionAsync runs registration events and builds the dependency chain, but never calls IModuleConditionHandler.ShouldIgnore — so --validate and build-time selection checks resolve names against finalized dynamic dependencies/metadata without evaluating ShouldSkip() run conditions, consistent with the stated fix.

No new issues found. The rest of the diff (arg parsing, ModuleSelection's dependency-closure resolution via AllDescendantDependenciesAndSelf(), ModuleSelectionValidator's Order=250 placement, PipelineImpl.RunAsync short-circuiting to the command handler before hitting the orchestrator) is consistent and matches the documented behavior in docs/docs/how-to/command-line.md.

One low-severity observation, not a blocker: PipelineCommandHandler.ListModules() always lists every registered module and ignores any --module/--skip-module/--categories filters that happen to be passed alongside --list-modules (the parser doesn't forbid combining them, since SetCommand only guards against combining two different commands). That's a defensible choice — showing the full graph lets users decide what to target — but if --list-modules --categories Test is meant to preview a filtered run, it currently won't. Worth a one-line doc callout or a follow-up if that combination comes up in practice; not worth blocking on.

CLAUDE.md compliance: no violations — no generated options classes touched, Module<T>/[DependsOn<T>]/ExecuteAsync pattern untouched, changes land under the documented directories.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 62a370d1f7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@thomhurst
thomhurst merged commit a67297a into main Jul 30, 2026
15 checks passed
@thomhurst
thomhurst deleted the issue-3537-cli branch July 30, 2026 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: first-class CLI - list modules, run one module with its dependency closure, category flags, dry-run, graph

1 participant