Add dependency graph export

[thomhurst]

Closes #3539

Adds canonical dependency-graph export in Mermaid, DOT, and JSON; --graph CLI support; builder/pipeline APIs; annotated nodes; GitHub step-summary flowchart; documentation and regression coverage.

Validation:

• ModularPipelines.sln Release build: 0 warnings/errors
• ModularPipelines.GitHub.sln Release build: 0 warnings/errors
• DependencyGraphExporterTests: 2/2
• PipelineCommandLineTests: 16/16
• ModularPipelines.GitHub.UnitTests: 9/9

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a8fd05b8d4

ℹ️ 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".

[claude]

Code review

Reviewed the diff, docs, and new tests. Overall this is a clean, well-integrated addition — the CLI parsing follows the existing TryReadValues/SetCommand conventions exactly, the DI registration follows the existing singleton pattern in DependencyInjectionSetup, and the new IDependencyGraphExporter sits naturally alongside IDependencyPrinter/IDependencyChainProvider. Test coverage for the CLI path, the exporter itself, and the GitHub summary integration all look solid (mermaid/dot/json renderers cross-checked against the same underlying graph in DependencyGraphExporterTests).

Two independent bug/logic passes over the diff turned up no confirmed high-signal bugs. One candidate finding was investigated and ruled out as a false positive, noted below for transparency, plus one design suggestion worth considering.

Investigated and ruled out

GitHubMarkdownSummaryGenerator renames OnStartAsync/OnEndAsync → OnPipelineStartAsync/OnPipelineEndAsync. At first glance this looks like it could break the implicit interface implementation since IPipelineGlobalHooks isn't touched in this diff. I checked IPipelineGlobalHooks directly — it already declares OnPipelineStartAsync/OnPipelineEndAsync as default interface methods. That means before this PR, GitHubMarkdownSummaryGenerator's OnStartAsync/OnEndAsync methods didn't match the interface at all and were effectively dead code — the interface's no-op defaults were being invoked instead, so the step-summary file was likely never actually written via this hook. This PR's rename fixes that latent bug as a side effect. Worth calling out explicitly in the PR description since it's a behavior fix bundled with the new feature, not just plumbing for it.

Suggestion: avoid the escape/un-escape-separator trick for graph labels

In DependencyGraphExporter.EscapeMermaid/EscapeDot, labels are built by joining parts with a raw separator (<br/> for Mermaid, \n for DOT) and then escaping the whole string, followed by a final .Replace(...) that un-escapes just that separator back to its raw form so the diagram renderer still treats it as a line break.

This works for today's inputs, but it's fragile because the escaping and the "protect this one substring" step are coupled by string-matching rather than by construction. If a module's [ModuleCategory] or a skip-reason string ever happens to contain the literal sequence <br/> (Mermaid) or \n (DOT), it will be incorrectly restored to raw markup in the final output — silently altering the rendered diagram. Both categories and skip reasons are developer-authored strings today so the practical risk is low, but it's an easy-to-miss invariant for future maintainers extending this code (e.g. adding a new annotated field).

A more robust approach: escape each label part independently, then join with the raw separator afterward, so there's no need to special-case unescape the joiner at all:

private static string BuildLabel(DependencyGraphNode node, string separator, Func<string, string> escape)
{
    var parts = new List<string> { node.Name };
    // ...
    return string.Join(separator, parts.Select(escape));
}

This removes the "escape everything, then carefully un-break the one string I need intact" ordering dependency entirely — each part is escaped in isolation before the structural separator is introduced, so the separator can never collide with escaped user content regardless of what a category or skip-reason string contains.

No other issues found. Nice addition — the programmatic (ExportDependencyGraphAsync), CLI (--graph), and CI (step summary) entry points all funnel through the same IDependencyGraphExporter, which keeps the three output formats consistent by construction.

[thomhurst]

Addressed all three review findings in d5d6a24 and synced current main in b88bdef.

Validation after sync:

• core Release build: 0 warnings, 0 errors
• DependencyGraphExporterTests: 2 passed
• PipelineCommandLineTests: 19 passed

Threads remain open for the next bot review cycle per repository policy.

[claude]

Code review

Re-reviewed at the current head (b88bdef26a, PR synced with main).

Both findings from the previous review have been correctly addressed:

• Label escaping: BuildLabel now escapes each label part independently (parts.Select(escape)) before joining with the raw separator, so there's no longer an "escape everything, then un-escape the separator" step that could collide with user content. This is exactly the fix suggested — good change.
• OnPipelineStartAsync/OnPipelineEndAsync rename: still in place; this was confirmed as a genuine (and welcome) latent-bug fix bundled with the feature, not something requiring further changes.

I re-read the rest of the current diff (CLI --graph parsing, DependencyGraphExporter, DI registration, PipelineImpl/PipelineBuilderExtensions, and the new tests) and didn't find any new high-signal bugs:

• PipelineCommandLineParser.TryReadGraph correctly rejects numeric enum values (e.g. --graph 0) via the int.TryParse guard before falling through to Enum.TryParse/Enum.IsDefined, and this is covered by GraphCommandRejectsNumericFormats.
• The DependencyGraphExporter singleton calling dependencyChainProvider.Initialize(...) on each render follows the same pre-existing pattern used by PipelineCommandHandler/ModuleSelection, so it's consistent with how IDependencyChainProvider is already used elsewhere.
• Test coverage (CLI export, exporter renderer cross-checks, GitHub step-summary integration) all still looks solid and exercises the annotated (category/estimated-duration/skip-reason) label paths.

No further issues found. Nice work addressing the feedback.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b88bdef26a

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d310dcf70

ℹ️ 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".

[claude]

Code review

Re-reviewed at the current head (2d310dcf70, "fix: resolve graph skips through history"). This is the delta since the last review at b88bdef26a — no new comments were needed on the previously-addressed items (label escaping, OnPipelineStartAsync/OnPipelineEndAsync rename), which remain correctly in place.

What changed

DependencyGraphExporter.CreateGraphAsync now delegates to the shared IIgnoredModuleResultRegistrar (the same component ExecutionOrchestrator uses) instead of calling DependencySkipCascade.ApplyAsync directly, and a node's Skipped flag is now derived from the actual registered result status (resultRegistry.GetResult(moduleType)?.ModuleStatus == Status.Skipped) rather than simply "is this module in the ignored list." This correctly fixes the bug where a module resolved via a history repository (Status.UsedHistory) was shown as "skipped" in the graph even though it wasn't actually skipped. The new Historical_Ignored_Dependency_Does_Not_Skip_Dependent test exercises exactly this case and confirms both the dependency and its dependent are no longer marked skipped when history resolves them. Good, targeted fix.

Design note worth considering: the registrar now does real work a second time on the GitHub summary path

IIgnoredModuleResultRegistrar.RegisterIgnoredModuleResultsAsync is not memoized — every call re-runs the full DependencySkipCascade.ApplyAsync loop and, for each originally ignored module, re-invokes RegisterIgnoredModuleResultAsync, which (when a history repository is configured) does another IModuleResultRepository.GetResultAsync round-trip and unconditionally overwrites the module's entry in IModuleResultRegistry with a freshly created result object.

In the normal run path, this is now called twice within the same pipeline execution:

1. Once up front by ExecutionOrchestrator.ExecuteInternal before real execution.
2. Again at pipeline end, when GITHUB_STEP_SUMMARY is set and GitHubMarkdownSummaryGenerator.OnPipelineEndAsync calls DependencyGraphExporter.RenderAsync, which now routes through the same registrar.

ModuleRetriever.GetOrganizedModules is memoized (_cached ??=), but the registrar sitting downstream of it isn't, so the cascade recomputation and (when history is configured) the second repository query aren't cached. Today this doesn't visibly corrupt output — RegisterResult/TrySetResult are idempotent overwrites and the already-built PipelineSummary passed into the hook isn't affected — but it's doing a full second pass of cascade + I/O purely to compute an annotation for the step-summary graph, and it's an easy thing for a future change (e.g., a history repository with side effects on read, or a cascade that isn't perfectly deterministic across calls) to turn into a real bug. Memoizing the registrar's result per pipeline run (similar to how ModuleRetriever caches OrganizedModules) would remove the redundant work and the implicit "safe to call twice" assumption entirely.

No other issues found in the delta. Nice, well-tested fix for the history-skip case.

[thomhurst]

Addressed both fresh P2 threads in cf0879f and synced current main in 1f1b091. Validation after sync: DependencyGraphExporterTests 5/5; PipelineCommandLineTests 19/19 before sync; core Release build 0 warnings/errors; scoped formatting clean. Fresh threads remain open for the next review cycle.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1f1b091db0

ℹ️ 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".

[claude]

Code review

Re-reviewed at the current head (1f1b091d, "fix: validate dependency graph exports" merged with main). This is the delta since the last review at 2d310dcf70.

What changed

DependencyGraphExporter now validates registration-time dependencies via ValidateRunnableModules/ModuleDependencyValidator, so exporting a graph for a pipeline with an invalid dynamic dependency now fails fast with ModuleNotRegisteredException instead of silently producing a bad graph — covered by the new Render_Rejects_Invalid_Registration_Dependency test. EscapeDot also now normalizes \r\n/\r/\n to \n, fixing a real bug where a multi-line [ModuleCategory(...)] value or skip reason would previously split a DOT node statement across lines — covered by Dot_Escapes_Line_Breaks_Inside_Label_Values. Both are good, targeted fixes with matching tests.

Investigated, not flagged

I looked hard at three candidate issues in this delta and ruled each out after digging into the surrounding engine code (not just the diff):

• EscapeMermaid has no newline handling, unlike the just-fixed EscapeDot. This looked like the same class of bug left half-fixed. It isn't: Mermaid's flowchart grammar consumes [^"]* inside a quoted label (i.e. the string token itself absorbs embedded newlines), so a raw \n inside n0["..."] doesn't split the statement — I confirmed this empirically against mermaid.parse() on the exact output shape RenderMermaid produces. The only effect is cosmetic (DOT gets an explicit line break, Mermaid collapses it), not a parse failure.
• The new ValidateRunnableModules/RenderAsync call happens in GitHubMarkdownSummaryGenerator.OnPipelineEndAsync, which runs inside a bare finally in PipelineExecutor.ExecuteAsync with no try/catch — a thrown exception there would normally mask an in-flight module failure. In practice this can't trigger: every input CreateGraphAsync touches (ModuleRetriever.GetOrganizedModules, the dependency-validation call, dependencyChainProvider.Initialize) is a memoized/deterministic replay of validation that already ran and passed during pipeline startup, so there's no new way for it to throw at pipeline end.
• The type-keyed ToDictionary calls in CreateGraphAsync (over organizedModules.RunnableModules/IgnoredModules) would throw on a duplicate module Type, unlike DependencyChainProvider.Detect's TryAdd. Also moot: ModuleConfigurationValidator.ValidateModuleUniqueness already rejects duplicate module-type registrations during BuildAsync(), before any IPipeline/exporter instance exists.

Still open from the previous review (non-blocking)

The memoization note from the last round about IIgnoredModuleResultRegistrar.RegisterIgnoredModuleResultsAsync doing a redundant cascade+I/O pass when the GitHub step-summary hook calls the exporter a second time per run wasn't addressed in this delta. That's unchanged from before: not visibly causing incorrect output today, just doing avoidable extra work — still worth a follow-up but not blocking.

No high-signal issues found in this delta. Nice, well-tested fixes.

[thomhurst]

Addressed both fresh P2 findings in 3a8b2bc and synced current main in �3f5a5f020.

Post-sync validation:

• DependencyGraphExporterTests: 6/6
• PipelineCommandLineTests: 20/20
• core Release build: 0 warnings, 0 errors
• scoped whitespace formatting clean

Fresh threads remain open for the next review cycle.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a3f5a5f020

ℹ️ 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".

[claude]

Code review

Re-reviewed at the current head (a3f5a5f020, synced with main). This is the delta since the last review at 1f1b091d: the fix: align graph annotations with runtime commit (3a8b2bcffa) plus the main-sync merge (a3f5a5f020).

What changed

DependencyGraphExporter.ApplyRunConditionsAsync now runs every runnable module through the same IModuleConditionHandler.ShouldIgnore path ModuleExecutionPipeline uses at runtime, before the existing history-aware skip cascade. A [RunIfAll<...>]/[SkipIf<...>]/category-excluded module — and everything transitively depending on it — is now correctly annotated skipped: true in the exported graph instead of appearing runnable, with the same fallback skip reason text used at runtime. ShouldIgnore is memoized per module instance, so this can't double-evaluate conditions. Covered by Run_Conditions_And_Their_Cascade_Are_Annotated_As_Skipped, which also asserts _executions == 0 to confirm graph export still never runs a module body.

Separately, PipelineCommandLineParser.GetDefaultGraphPath now defaults the Mermaid export to dependency-graph.mmd instead of .md, since the raw flowchart TD output isn't valid fenced Markdown — covered by MermaidGraphCommandDefaultsToRawMermaidExtension.

Both correctly close the two Codex threads that were still open at the last review. Good, targeted fixes with matching regression tests.

Issue found: docs are now out of sync with the .mmd default

docs/docs/how-to/execution-and-dependencies.md wasn't updated for this delta's default-extension change:

• The example command still shows --graph mermaid dependency-graph.md, and the programmatic example still calls ExportDependencyGraphAsync(DependencyGraphFormat.Mermaid, "dependency-graph.md").
• "The path is optional. The defaults are dependency-graph.md, dependency-graph.dot, and dependency-graph.json" is now factually wrong — the actual default is dependency-graph.mmd.

A reader following these examples literally ends up with a .md file containing raw, non-fenced Mermaid syntax — the exact rendering problem this PR just fixed for the default path. Worth a quick follow-up to swap the three .md references to .mmd.

Investigated, not flagged

• EscapeMermaid's lack of newline handling, RegisterIgnoredModuleResultsAsync's memoization cost, and the type-keyed ToDictionary calls: reasoning from the previous review still holds, and nothing in this delta touches them.
• ApplyRunConditionsAsync mutates module completion sources (via RegisterIgnoredModuleResultsAsync) for condition-skipped modules, same as it already did for category-skipped ones. Confirmed PipelineCommandHandler.ExportGraphAsync returns immediately after export — PipelineCommand.ExportGraph is a terminal case that never proceeds to real execution with the same module instances — so this can't cause a double-run or a stale completion source.

One doc-sync fix needed before merge; no code-level bugs found in this delta.

[thomhurst]

Addressed both fresh P2 findings in c4991a197 and synced current main in d9d5757.

Post-sync validation:

• DependencyGraphExporterTests: 8/8
• core Release build: 0 warnings, 0 errors
• scoped whitespace formatting clean

Fresh threads remain open for the next review cycle.

[claude]

Code review

Re-reviewed at the current head (d9d575765, synced with main). This is the delta since the last review at a3f5a5f020: the fix: resolve configured graph skips commit (fc4991a197) plus the main-sync merge.

Docs fix confirmed

The .mmd default-extension doc-sync issue flagged in the previous review is fixed — docs/docs/how-to/execution-and-dependencies.md now consistently shows dependency-graph.mmd in both the CLI and programmatic examples, and the "defaults are" line matches GetDefaultGraphPath.

What changed

DependencyGraphExporter.ApplyRunConditionsAsync now also evaluates a module's fluent .WithSkipWhen(...)/Configuration.SkipCondition delegate during graph export, not just attribute-based [RunIf]/[SkipIf] conditions. When the condition can't be resolved without runtime data (it throws via the new GetModule<T> guard, or its ValueTask doesn't complete synchronously), the node's skipped is now a tri-state (true/false/null-"unresolved") that cascades to dependents through PropagateUnresolvedSkipDecisions. Good, targeted fix with solid matching tests (Configured_Skip_Conditions_And_Their_Cascade_Are_Annotated, Result_Dependent_Configured_Skips_Are_Annotated_As_Unresolved).

Issue found: evaluating SkipCondition during graph export can execute real side effects, not just "unresolvable" runtime lookups

EvaluateConfiguredSkipConditionAsync (src/ModularPipelines/Engine/DependencyGraphExporter.cs:164-195) builds a real ModuleContext backed by the actual IPipelineContext (pipelineContextProvider.GetModuleContext() — the same instance used during real pipeline execution, per ModuleContextProvider/RequirementChecker), and invokes module.Configuration.SkipCondition!(moduleContext, cancellationToken) for every runnable module that has one configured.

The only guard added is EnsureModuleResultAccessAllowed (ModuleContext.cs:97-103), which throws PlanningModuleResultUnavailableException specifically when the delegate calls context.GetModule<T>()/GetModuleIfRegistered<T>(). But .WithSkipWhen(Func<IModuleContext, CancellationToken, ValueTask<SkipDecision>>) is a fully open API — the delegate gets the entire IModuleContext (file system helpers, command/process execution, HTTP, git, environment mutation, etc.), the same surface a module's ExecuteAsync body gets. Nothing stops a user's skip condition from doing real work, e.g.:

.WithSkipWhen(async (context, ct) =>
{
    await context.Cmd().ExecuteCommandLineTool(new("some-external-tool"), ct); // runs for real
    return SkipDecision.DoNotSkip;
})

Previously, --graph / ExportDependencyGraphAsync was a documented dry-run — the existing test suite explicitly asserts module bodies never execute (_executions == 0 in Run_Conditions_And_Their_Cascade_Are_Annotated_As_Skipped). This change quietly narrows that guarantee: it's no longer "graph export never runs anything," it's "graph export never runs anything except whatever a configured SkipCondition happens to do." A synchronous side effect in the condition body runs unconditionally and before the decision.IsCompleted check even matters — e.g. SkipIf(async (ctx, ct) => { File.Delete("build-artifact.txt"); await Task.Delay(1); return SkipDecision.DoNotSkip; }) deletes the file immediately, then still gets classified as "unresolved" because the ValueTask didn't complete synchronously. So the exact case this PR treats as "safe to skip evaluating" (non-synchronously-completing conditions) is the one where a side effect is most likely to have already happened by the time it's discarded.

This is worth calling out explicitly (in docs, and ideally enforced in code) rather than left as an implicit assumption, since a user running dotnet run -- --graph mermaid to visualize their pipeline — expecting a side-effect-free dry run — could unknowingly trigger real work (deleting files, hitting external services, running processes) if any module uses .WithSkipWhen(...) for anything beyond a pure/declarative check. A more robust design: only evaluate conditions during export that are provably side-effect-free — e.g. keep the current attribute-based [RunIf]/[SkipIf] handling (which is inherently declarative), but for the open Func<IModuleContext, ...> form, either always classify it as "unresolved" in graph mode (never invoke it), or provide a restricted IModuleContext implementation for planning that throws PlanningModuleResultUnavailableException-style guards from every capability that isn't a pure read (not just GetModule<T>), so any accidental side-effecting call fails fast instead of silently executing.

Investigated, not flagged

• PropagateUnresolvedSkipDecisions's fixed-point cascade loop is bounded (finite module list, monotonically growing unresolvedTypes set) and correctly excludes Optional dependencies from propagation, consistent with how the existing skip cascade treats optional dependencies elsewhere.
• The reasoning from prior reviews on EscapeMermaid's lack of newline handling and IIgnoredModuleResultRegistrar.RegisterIgnoredModuleResultsAsync's memoization cost still holds; nothing in this delta touches either.

One design concern worth addressing before/soon after merge (the side-effect exposure above); no other high-signal issues found in this delta. The tri-state skip/unresolved modeling itself, and its cascade to dependents, is well done and well tested.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9d5757653

ℹ️ 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]

Addressed the fresh P1/P2 findings in b874e7b.

Validation:

• core Release build: 0 warnings, 0 errors
• DependencyGraphExporterTests: 9/9
• GitHubMarkdownSummaryGeneratorTests: 2/2
• changed-file whitespace formatting: clean

Fresh threads remain open for the next review cycle.

[claude]

Code review

Re-reviewed at the current head (b874e7b96b). This PR has already been through many review rounds (both automated chatgpt-codex-connector passes and prior Claude reviews) — most flagged issues (skip cascade through history, [RunIf]/[SkipIf] evaluation before annotation, DOT/Mermaid escaping, CLI numeric-enum rejection, .mmd doc/default drift) have been fixed with matching regression tests along the way.

Latest commit (b874e7b96b, "fix: preserve graph execution semantics")

This correctly resolves the two P1/P2 findings from the previous round:

• Re-evaluating skip conditions in the GitHub step-summary hook — GitHubMarkdownSummaryGenerator now renders via the new IPipelineSummaryDependencyGraphRenderer.RenderAsync(format, pipelineSummary, ...) overload, which derives node state from the already-completed PipelineSummary.Results instead of re-invoking Configuration.SkipCondition after execution. The new StepSummaryDoesNotReevaluateSkipConditions test (asserting exactly one evaluation via Interlocked.Increment) verifies this directly.
• History resolution ordering for execution-time condition skips — CascadeRunConditionSkipsAsync now separates "resolved via history" from "skipped by a configured condition," so a module with a usable history result is no longer misclassified as skipped: true. Covered by Execution_Time_Skips_Do_Not_Use_History.

Remaining concern (pre-existing, not touched by the latest commit): --graph CLI/programmatic export can still run real side effects

This is the same underlying issue as the fixed GitHub-hook case, but it applies to the other call path and wasn't addressed by this commit.

ExportDependencyGraphAsync / --graph <format> documents itself as a dry run — docs/docs/how-to/execution-and-dependencies.md:47 says "Export the resolved graph without executing modules." That path goes through CreateGraphAsync(cancellationToken) → ApplyRunConditionsAsync → EvaluateConfiguredSkipConditionAsync (DependencyGraphExporter.cs:226-302), which is unchanged by this commit and still invokes module.Configuration.SkipCondition!(moduleContext, cancellationToken) against a real ModuleContext backed by the live IPipelineContext.

The only guard is EnsureModuleResultAccessAllowed (ModuleContext.cs:98-103), which throws PlanningModuleResultUnavailableException solely when the delegate calls GetModule<T>/GetModuleIfRegistered<T>(). WithSkipWhen(Func<IModuleContext, ...>) is a fully open API (ModuleConfigurationBuilder.cs:69-126) — the delegate gets the entire IModuleContext (command execution, file system, HTTP, git, environment mutation), same as a module's real ExecuteAsync. A synchronous side effect placed before the first await in that delegate — e.g. File.Delete(...), a context.Cmd().ExecuteCommandLineTool(...) call — executes unconditionally during dotnet run -- --graph mermaid, before the code even checks decision.IsCompleted. So the "resolved without executing modules" guarantee only holds if every configured skip condition happens to be side-effect-free, which isn't enforced anywhere.

Suggested fix, consistent with the approach already used for the GitHub-hook path: for the pre-execution export path, don't invoke the open delegate at all — always classify .WithSkipWhen(...)-configured modules as "unresolved" (the same tri-state already used when the ValueTask doesn't complete synchronously), or introduce a restricted planning-mode IModuleContext that throws from every non-pure capability (not just GetModule<T>) so an accidental side-effecting call fails fast instead of silently running.

Nothing else stood out as high-signal in this delta — DI wiring for the new IPipelineSummaryDependencyGraphRenderer registration correctly shares the same DependencyGraphExporter singleton instance, and the per-part label escaping fix (join-after-escape instead of escape-then-unescape-separator) resolves the earlier fragile-escaping concern.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b874e7b96b

ℹ️ 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]

Addressed the two fresh graph-planning P2 findings in 55bfb8f.

Validation:

• core Release build: 0 warnings, 0 errors
• DependencyGraphExporterTests: 11/11
• ModuleConfigurationTests: 33/33
• changed-file whitespace formatting: clean

The four prior threads were resolved after this later review cycle confirmed their fixes. Today's fresh threads remain open for the next cycle.

[claude]

Code review

Re-reviewed at the current head (1a7ab8aa, merge of 55bfb8f2ca into main). This PR has been through many rounds already (both chatgpt-codex-connector and prior Claude passes) — I read the full diff plus the surrounding engine code (ModuleRetriever, DependencyChainProvider, ModuleScheduler, DependencySkipCascade, IPipelineGlobalHooks) rather than just the changed lines, and cross-checked against the prior review threads on this PR.

Prior findings: confirmed fixed

• OnStartAsync/OnEndAsync → OnPipelineStartAsync/OnPipelineEndAsync rename: still correct. Confirmed IPipelineGlobalHooks on main already declares the OnPipeline* names as default-interface methods, so pre-PR the GitHub step-summary hook methods didn't override anything and were dead code — the step summary was never actually written. This PR's rename is a genuine (and welcome) latent-bug fix bundled with the feature.
• Label escaping (BuildLabel), DOT newline handling, numeric-enum rejection in TryReadGraph, .mmd default-extension docs, history-vs-skip misclassification, step-summary hook re-evaluating skip conditions (now correctly routed through IPipelineSummaryDependencyGraphRenderer.RenderAsync(format, pipelineSummary, ...), which reads PipelineSummary.Results instead of re-invoking SkipCondition — verified by StepSummaryDoesNotReevaluateSkipConditions) — all remain fixed and are exercised by matching regression tests.
• Latest delta (HasAsyncSkipCondition): ModuleConfigurationBuilder.WithSkipWhen(Func<IModuleContext, SkipDecision>) (sync overload, ModuleConfigurationBuilder.cs:70-75) leaves _hasAsyncSkipCondition at its false default, while both the ValueTask-returning overload and WithSkipWhenAll set it true. DependencyGraphExporter.EvaluateConfiguredSkipConditionAsync now short-circuits to "unresolved" without invoking the delegate at all whenever HasAsyncSkipCondition is true. Correctly narrows the invocation surface, and SingleUseSkipConditionModule/AsyncConfiguredSkipModule in the new tests cover both branches.

Residual (narrowed, not eliminated): sync WithSkipWhen delegates still run for real during a documented "dry run"

This is a continuation of the side-effect concern raised in the last two review rounds, not a new issue — but it's only partially closed by the HasAsyncSkipCondition change, and I don't think that was fully called out.

EvaluateConfiguredSkipConditionAsync (DependencyGraphExporter.cs) still builds a real ModuleContext backed by the live IPipelineContext and invokes module.Configuration.SkipCondition!(...) whenever the condition was registered via the synchronous WithSkipWhen(Func<IModuleContext, SkipDecision>) overload. That overload hands the delegate the full IModuleContext — file system, command execution, HTTP, git, environment mutation — the same surface ExecuteAsync gets. The only guard, EnsureModuleResultAccessAllowed, blocks GetModule<T>()/GetModuleIfRegistered<T>() specifically; it doesn't restrict anything else. So:

.WithSkipWhen(ctx => { File.Delete("build-artifact.txt"); return SkipDecision.DoNotSkip; })

still executes the delete for real the moment someone runs dotnet run -- --graph mermaid or calls ExportDependencyGraphAsync. The docs (docs/docs/how-to/execution-and-dependencies.md:47) say "Export the resolved graph without executing modules" with no caveat that synchronously-declared skip conditions are the exception.

Given how many rounds have already gone into narrowing this, I'd treat it as a documentation gap rather than a blocker: either (a) add a line to the docs/XML-doc on the sync WithSkipWhen overload stating it's invoked during graph export and must be side-effect-free, or (b) go the last step and always treat all SkipConditions as unresolved during export regardless of the sync/async split, closing the gap in code instead of by convention. Worth a decision either way so this doesn't need re-litigating in a future round.

Architectural observation: planning logic re-implemented alongside the real engine, evidenced by this PR's own history

DependencyGraphExporter.CreateGraphAsync(CancellationToken) re-derives "what would run and why it'd be skipped" independently from the real path (ExecutionOrchestrator/ModuleExecutionPipeline) — it does reuse shared primitives (IIgnoredModuleResultRegistrar, DependencySkipCascade.ApplyAsync, IModuleConditionHandler.ShouldIgnore), but the orchestration that stitches them together (ApplyRunConditionsAsync, CascadeRunConditionSkipsAsync, PropagateUnresolvedSkipDecisions, ValidateRunnableModules) is its own parallel implementation.

The review history on this exact PR is good evidence this is a real maintenance cost, not a hypothetical one: at least three separate follow-up commits (fix: resolve graph skips through history, fix: align graph annotations with runtime, fix: resolve configured graph skips) existed specifically to bring the exporter's speculative resolution back in sync with actual runtime skip/condition/history semantics after they'd drifted. That pattern — the two implementations disagreeing, caught only by manual review — is likely to recur the next time run-condition or skip-cascade behavior changes in the real engine, since there's nothing structural tying the two together (no shared "plan" abstraction, no test that fails automatically if they diverge).

Suggestion for a follow-up (not blocking this PR, which is already well past the point of re-scoping): extract a single IPipelinePlanResolver-style component that both ExecutionOrchestrator and DependencyGraphExporter consume for "resolve run/skip/ignore state for this module set," parameterized by whether runtime results are available yet. That would make behavioral drift a compile-time/single-code-path concern instead of something that has to be caught by re-review each time the engine's skip semantics change.

Test coverage

Thorough — DependencyGraphExporterTests (11 cases) cross-checks all three renderers against the same annotated graph, covers history/condition/category/configured-skip cascades, unresolved tri-state propagation, and duplicate-dependency validation; PipelineCommandLineTests and GitHubMarkdownSummaryGeneratorTests cover the CLI and step-summary entry points including the non-reevaluation guarantee. No gaps found.

Verdict

No new high-signal bugs in this delta. One documentation/consistency decision worth closing out (side-effect scope of sync WithSkipWhen during export) and one non-blocking architectural suggestion for follow-up work given the visible drift-and-refix pattern across this PR's own history. Good, well-tested feature otherwise.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55bfb8f2ca

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 69585a3458

ℹ️ 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".

[claude]

Code review

Re-reviewed at the current head (69585a345, "fix(graph): isolate planning caches"). This is the delta since the last review at 1a7ab8aa.

What changed, and why it matters

DependencyGraphExporter.ApplyRunConditionsAsync now calls the new IModuleConditionHandler.ShouldIgnoreWithoutCaching instead of the caching ShouldIgnore.

This fixes a real correctness bug, not just a cache-hygiene nit. ModuleConditionHandler.ShouldIgnore memoizes its (ShouldIgnore, SkipDecision) result per module instance in a ConditionalWeakTable, and ModuleExecutionPipeline is the only real-execution caller of that cached path, evaluated once per module for the whole run. Before this fix, graph export (ExportDependencyGraphAsync/--graph) called the same cached ShouldIgnore, which meant calling export before pipeline.RunAsync() would permanently seed the cache with a decision made prior to IPipelineGlobalHooks.OnPipelineStartAsync running — so if a [RunIfAll<T>]/[RunIfAny<T>] condition depends on state a startup hook sets up, the real run would silently reuse the stale pre-startup "skip" decision and never actually execute the module. ShouldIgnoreWithoutCaching shares the same per-module semaphore gate (so it still serializes with the real cached path) but never reads or writes evaluation.HasResult/Result, so export no longer poisons the cache the real engine depends on.

The new test Render_Does_Not_Cache_Conditions_Before_Startup_Hooks demonstrates exactly this scenario (export shows the module skipped pre-startup, then the real run still executes it once the hook flips the condition) and would have failed against the pre-fix behavior.

The ModuleRetriever.GetOrganizedModules change is a related but separate fix: previously _cached ??= GetInternal(...) meant a single canceled/faulted discovery permanently poisoned the memoized task for the lifetime of the retriever, so any transient cancellation (e.g. a canceled export call) would make all subsequent calls fail forever. It now only reuses _cached when it isn't canceled/faulted, letting a later call retry with a fresh token — covered by the new Render_Can_Retry_After_Canceled_Module_Discovery test. The read-modify-write on _cached stays race-free under the existing [MethodImpl(MethodImplOptions.Synchronized)] lock.

Validation

• dotnet build ModularPipelines.sln -c Release: 0 warnings, 0 errors.
• DependencyGraphExporterTests (13/13, including both new regression tests): passed.

Prior open items

The architectural note from the previous review (extracting a shared IPipelinePlanResolver-style component so ExecutionOrchestrator and DependencyGraphExporter can't drift on skip/condition semantics) and the sync-WithSkipWhen side-effect documentation gap are both unchanged by this commit — still worth a follow-up, not blocking.

No new high-signal bugs in this delta. Good, well-tested fix for a genuine latent-cache-poisoning bug.

[thomhurst]

Addressed both graph findings in 1988853: planning history resolution is mutation-free, and registered dynamic dependencies are exported. Validation: DependencyGraphExporterTests 15/15; related registrar/provider tests 2/2; core Release build clean; changed-file format clean.

[claude]

Code review

Re-reviewed at the current head (19888537d7, "fix(graph): isolate planning state"). This is the delta since the last review at 69585a3458, addressing the two freshest Codex findings.

What changed, verified correct

"Keep graph planning from completing runtime module results" (P1). IIgnoredModuleResultRegistrar now exposes a separate ResolveIgnoredModuleResultsAsync (read-only) alongside the existing RegisterIgnoredModuleResultsAsync (mutating). Both share ResolveIgnoredModuleResultsCoreAsync(organizedModules, registerResults); when registerResults is false — the path DependencyGraphExporter.CreateGraphAsync now uses — the _resultRegistry.RegisterResult(...) and SetModuleCompletionSource(...) calls are skipped entirely, so exporting a graph before RunAsync() no longer permanently completes a module's CompletionSource or seeds the shared IModuleResultRegistry. The new Render_Does_Not_Complete_Runtime_Module_Results test is a good regression: it uses a ChangingHistoryRepository and asserts ReadCount == 2 (export reads history once, the real run reads it again independently) and that the real run's result reflects the second read ("history-2"), which would fail if planning had cached/completed the first read's result.

"Include registered dynamic dependencies in exported edges" (P2). DependencyChainProvider.GetModuleDependencies now resolves via ModuleDependencyResolver.GetAllDependencies(...) (the same resolver used for runtime validation, which consults IModuleDependencyRegistry for registration-time/dynamic dependencies) instead of only the module's static IsDependentOn metadata, with .Distinct() to avoid duplicate edges. Render_Includes_Registered_Dynamic_Dependency confirms a registration-event-added dependency now produces the expected edge in the exported JSON.

Both fixes are targeted, match the reported failure modes, and have matching regression tests. I independently re-read DependencyGraphExporter.cs, ModuleConditionHandler.cs (ShouldIgnoreWithoutCaching), ModuleRetriever.cs (the IsCanceled || IsFaulted cache-retry guard from the prior round), GitHubMarkdownSummaryGenerator.cs, and PipelineCommandLineParser.cs rather than just the diff, to make sure earlier fixes in this long review chain are still holding — they are:

• Label escaping, DOT newline handling, numeric-enum rejection, .mmd default/docs sync, history-vs-skip cascade ordering, step-summary hook not re-evaluating skip conditions, run-condition/configured-skip annotation before cascade, async-skip non-invocation, pre-condition dependency validation ordering, and canceled-discovery cache poisoning — all remain correctly in place.

No new issues found in this delta. This PR has been through an unusually thorough review cycle (many rounds of chatgpt-codex-connector plus prior Claude passes), and each round's findings have been fixed with matching tests rather than papered over — the DependencyGraphExporter design has converged on a coherent split between "planning" (read-only, used for --graph/pre-run export) and "post-run" (derives strictly from PipelineSummary.Results) rendering paths, which is the right shape for this feature. Good to merge from my perspective, pending final CI.

[coderabbitai]

Warning

Review limit reached

@thomhurst, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c7cd710-13fe-46d2-9523-9a3571b430a2

📥 Commits

Reviewing files that changed from the base of the PR and between cb63ea2 and 1e7107f.

📒 Files selected for processing (62)

• docs/docs/how-to/execution-and-dependencies.md
• src/ModularPipelines.GitHub/GitHubMarkdownSummaryGenerator.cs
• src/ModularPipelines/Attributes/DependsOnModulesInCategoryAttribute.cs
• src/ModularPipelines/Attributes/DependsOnModulesWithAttributeAttribute.cs
• src/ModularPipelines/Attributes/DependsOnModulesWithTagAttribute.cs
• src/ModularPipelines/Attributes/EnvironmentVariableConditionAttributes.cs
• src/ModularPipelines/Attributes/Events/IPlanningSafeModuleRegistrationEventReceiver.cs
• src/ModularPipelines/Attributes/IConditionAttribute.cs
• src/ModularPipelines/Attributes/IPlanningSafeDependencySelector.cs
• src/ModularPipelines/Attributes/OperatingSystemConditionAttributes.cs
• src/ModularPipelines/Attributes/OperatingSystemConditions.cs
• src/ModularPipelines/Attributes/PlanningSafeDependsOnBaseAttribute.cs
• src/ModularPipelines/CommandLine/PipelineCommand.cs
• src/ModularPipelines/CommandLine/PipelineCommandHandler.cs
• src/ModularPipelines/CommandLine/PipelineCommandLineHelp.cs
• src/ModularPipelines/CommandLine/PipelineCommandLineOptions.cs
• src/ModularPipelines/CommandLine/PipelineCommandLineParser.cs
• src/ModularPipelines/Conditions/IPlanningRunCondition.cs
• src/ModularPipelines/Conditions/IsCI.cs
• src/ModularPipelines/Conditions/IsLocal.cs
• src/ModularPipelines/Conditions/OnLinux.cs
• src/ModularPipelines/Conditions/OnMacOS.cs
• src/ModularPipelines/Conditions/OnUnix.cs
• src/ModularPipelines/Conditions/OnWindows.cs
• src/ModularPipelines/Configuration/ModuleConfiguration.cs
• src/ModularPipelines/Configuration/ModuleConfigurationAttributeAdapter.cs
• src/ModularPipelines/Configuration/ModuleConfigurationBuilder.cs
• src/ModularPipelines/Context/ModuleContext.cs
• src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs
• src/ModularPipelines/Engine/Attributes/CustomAttributeMetadata.cs
• src/ModularPipelines/Engine/Attributes/IModuleAttributeEventService.cs
• src/ModularPipelines/Engine/Attributes/ModuleAttributeEventService.cs
• src/ModularPipelines/Engine/Attributes/RegistrationEventExecutor.cs
• src/ModularPipelines/Engine/Dependencies/ModuleMetadataRegistry.cs
• src/ModularPipelines/Engine/DependencyChainProvider.cs
• src/ModularPipelines/Engine/DependencyGraphExporter.cs
• src/ModularPipelines/Engine/Executors/IIgnoredModuleResultRegistrar.cs
• src/ModularPipelines/Engine/Executors/IgnoredModuleResultRegistrar.cs
• src/ModularPipelines/Engine/IDependencyChainProvider.cs
• src/ModularPipelines/Engine/IDependencyGraphExporter.cs
• src/ModularPipelines/Engine/IModuleActivator.cs
• src/ModularPipelines/Engine/IModuleConditionHandler.cs
• src/ModularPipelines/Engine/ModuleActivator.cs
• src/ModularPipelines/Engine/ModuleConditionHandler.cs
• src/ModularPipelines/Engine/ModuleDependencyResolver.cs
• src/ModularPipelines/Engine/ModuleDiscoveryPlanner.cs
• src/ModularPipelines/Engine/ModulePlanningSkipEvaluator.cs
• src/ModularPipelines/Engine/ModuleRetriever.cs
• src/ModularPipelines/Engine/PipelineExecutionState.cs
• src/ModularPipelines/Enums/DependencyGraphFormat.cs
• src/ModularPipelines/Extensions/PipelineBuilderExtensions.cs
• src/ModularPipelines/Extensions/ServiceCollectionExtensions.cs
• src/ModularPipelines/IPipeline.cs
• src/ModularPipelines/Modules/Module.cs
• src/ModularPipelines/PipelineBuilder.cs
• src/ModularPipelines/PipelineImpl.cs
• test/ModularPipelines.GitHub.UnitTests/GitHubMarkdownSummaryGeneratorTests.cs
• test/ModularPipelines.TestHelpers/ExternalConfigurationMutationProbe.cs
• test/ModularPipelines.TestHelpers/ExternalConfigurationState.cs
• test/ModularPipelines.UnitTests/CommandLine/PipelineCommandLineTests.cs
• test/ModularPipelines.UnitTests/Engine/DependencyGraphExporterTests.cs
• test/ModularPipelines.UnitTests/Engine/ModuleConditionHandlerTests.cs

---

_{Comment @coderabbitai help to get the list of available commands.}

[thomhurst]

Triggered a fresh workflow at a4c52b2 with an empty CI commit. Prior run 31334755476 was externally cancelled; analyzer and Windows only failed because the cancelled fast-fail lane never passed. No code changed.

[claude]

Code review

Re-reviewed at the current head (a4c52b257e, an empty CI-retrigger commit on top of 11fe0de0f "feat(graph): export pipeline dependencies"). The branch's long prior commit history was squashed since the last review round, but the resulting code matches what was already reviewed at 8b60cdf997 — this pass re-verified that content fresh rather than assuming it, and specifically followed up on the one open item from the last review.

No new bugs found

• EscapeMermaid/EscapeDot (DependencyGraphExporter.cs:658-673) correctly handle &, ", <, >, backtick, and all newline variants, with backslash escaped before newline substitution in DOT to avoid double-escaping. JSON output goes through Utf8JsonWriter, so escaping is safely delegated there too.
• The OnStartAsync/OnEndAsync → OnPipelineStartAsync/OnPipelineEndAsync rename in GitHubMarkdownSummaryGenerator.cs (fixing the pre-existing dead-code bug flagged in an earlier review, where the hooks silently never fired) is complete — no leftover references to the old names anywhere except an unrelated archived version-3.x doc.

Follow-up on the architectural concern from the 2026-08-05 review

That review flagged ModuleDiscoveryPlanner.cs's "activate-then-verify-via-bytecode-inspection" approach (CIL opcode decoding, private MEDI-internals reflection, deep reflective field comparison, 25 [UnconditionalSuppressMessage] suppressions) as fragile — a future DI package internals rename could silently break it. I traced the reachability further this round: ModuleDiscoveryPlanner.DiscoverAsync only runs via the opt-in --graph CLI flag / IPipeline.ExportDependencyGraphAsync, gated to run before RunAsync (enforced by PipelineExecutionState, with tests covering the mutual exclusion). Normal pipeline execution — including --dry-run — goes through the separate, non-reflective PipelinePlanner, and the always-on GitHub step-summary integration builds its graph from already-resolved runtime modules via ModuleRetriever, bypassing ModuleDiscoveryPlanner entirely.

So this fragility is confined to an opt-in diagnostic feature, not a correctness risk for real pipeline runs — I'm downgrading it from "architectural concern worth blocking on" to a non-blocking follow-up. The suggestion still stands as worth tracking separately: given the repo already has src/ModularPipelines.Analyzers with analyzers like AsyncModuleAnalyzer, a compile-time analyzer that enforces "discovery-time-consulted code is side-effect-free" would replace runtime bytecode/DI-internals inspection with a compile-time guarantee — but that's a larger redesign appropriate for a follow-up issue, not this PR.

Minor, non-blocking

• PipelineExecutionState.MarkExecutionStarted() uses CompareExchange(ref _state, -1, 0) and only guards state > 0; a second call while already -1 fails the exchange silently rather than throwing. Low real-world impact since pipeline instances aren't normally re-run — not blocking.
• DependencyGraphExporterTests.cs has dedicated tests for line-break and markdown-fence escaping, but no dedicated test for double-quote or ampersand characters in labels, even though the escape functions handle them correctly by inspection. Worth a quick addition if there's another revision, not worth blocking on.

Verdict

Ready to merge as-is. Test coverage is extensive (126 tests in DependencyGraphExporterTests.cs), the rendering code is clean and independently testable, and the one outstanding design question has a narrow, opt-in blast radius rather than affecting core pipeline correctness.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4c52b257e

ℹ️ 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".

[claude]

Code review

Re-reviewed at the current head (b0bc22e97d, "fix(graph): isolate planning metadata"). This is the delta since the last review at a4c52b257e, which had verdict "ready to merge as-is."

What changed, verified correct

This commit extends the pipeline's existing "planning-safe" pattern (already used by ModuleMetadataRegistry/ModuleDependencyResolver/PipelinePlanner to avoid side effects during dependency-graph planning) to two more surfaces:

• OperatingSystemConditions.HasImpossibleCombination(Type) — a new overload that inspects CustomAttributeData (via CustomAttributeMetadata.GetApplicable) instead of constructing condition-attribute instances, so ModuleConditionHandler no longer triggers arbitrary attribute constructors just to check for mutually-exclusive OS conditions during planning.
• ModuleContext.GetModuleIfRegistered now calls EnsureModuleResultAccessAllowed, closing a gap where a WithSkipWhen planning-safe skip condition could probe for a not-yet-resolved runtime module's presence via the "if registered" lookup. The new PlanningMetadataValueUnavailableException (ModuleMetadataRegistry.cs) guards GetAttribute/GetAttributes the same way, except for the two attributes already known to be side-effect-free (ModuleTagAttribute, ModuleCategoryAttribute).

Both new exception types are caught exactly where needed: PlanningModuleResultUnavailableException was already handled in ModulePlanningSkipEvaluator, PipelinePlanner, DependencyGraphExporter, and ModuleConfigurationBuilder; PlanningMetadataValueUnavailableException is caught in ModuleDependencyResolver.cs:155, scoped to when (planningSafeOnly) so normal runtime dependency evaluation is unaffected.

New tests (DependencyGraphExporterTests.Render_Does_Not_Expose_Runtime_Modules_To_Planning_Skip_Conditions, Render_Does_Not_Construct_Target_Attribute_Values_During_Planning, ModuleConditionHandlerTests.Distributed_Master_Discovery_Does_Not_Construct_Deferred_Conditions) cover exactly the regressions this closes, using construction/evaluation counters to assert no side effects fire during planning.

No new issues found

No bugs or CLAUDE.md violations in this delta. PipelineBuilderExtensions.ExportDependencyGraphAsync switching to the new internal BuildForDependencyGraphExportAsync() (skips initializePipeline) is consistent with the rest of this change's goal of not running runtime-only setup during export.

Observation (non-blocking)

This PR has now been through 80+ review rounds over the past ~8 days, several ending in a "ready to merge" verdict before another fix commit followed. If an automated fix/review loop is driving this, it may be worth having a human confirm this is still converging as intended rather than continuing indefinitely.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b0bc22e97d

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ae5d2caac

ℹ️ 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]

Rebased conflict-free onto current main after #3888 merged. git range-diff confirms all four PR patches are unchanged; only the base moved. Core Release build: 0 warnings, 0 errors.

Head verified: d0df7f6070bcbed2ffebd89c9ecd42e2a1577650.

@codex review
@claude review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: d0df7f6070

ℹ️ 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".

[claude]

Code review

Reviewed at the current head (d0df7f607), which per @thomhurst's note is a conflict-free rebase of the four existing patches onto main after #3888 merged — no functional code change versus the content already reviewed and marked "ready to merge" at a4c52b257e/b0bc22e97d. I re-verified that claim isn't just taken on faith: IsGraphPath, PipelineExecutionState, the MemberwiseClone-based planning-copy path, and the attribute-metadata caching below were all read fresh from the current tree rather than assumed unchanged.

Given this PR has already been through 80+ review rounds focused heavily on correctness of the new planning-safe machinery (escaping, side-effect isolation, exception routing), I focused this pass on architecture/design rather than re-litigating already-settled correctness ground. Two things stood out that hadn't come up in the prior threads I could see:

1. Three independent copies of "build a planning-scoped ModuleContext and invoke a skip delegate"

• PipelinePlanner.EvaluateFluentSkipConditionAsync (pre-existing)
• ModulePlanningSkipEvaluator.EvaluateConditionAsync (new, src/ModularPipelines/Engine/ModulePlanningSkipEvaluator.cs:67-101)
• DependencyGraphExporter.EvaluateConfiguredSkipConditionAsync (new, src/ModularPipelines/Engine/DependencyGraphExporter.cs:430-465)

All three do the same sequence: serviceProvider.CreateAsyncScope() → build an ExecutionContextFactory → construct a ModuleContext(..., moduleResultAccessAllowed: false) → wrap the call in PlanningModuleResultAccess.Enter() → catch PlanningModuleResultUnavailableException → null → dispose ModuleCancellationTokenSource in finally. ModulePlanningSkipEvaluator reads like it was meant to be the shared planning-condition invoker — it's a new, purpose-built class — but DependencyGraphExporter bypasses it and reimplements the same steps itself (the only real difference is it reads module.Configuration.SynchronousPlanningSkipCondition inline instead of taking the delegate as a parameter). Having DependencyGraphExporter call ModulePlanningSkipEvaluator instead would mean any future fix to this scope/dispose/exception-translation sequence (e.g. a new safety guard around PlanningModuleResultAccess) only needs to land in one place instead of three.

2. Full parallel "real" vs "planning" condition-evaluator pipeline in ModuleConditionHandler

EvaluateConditions/EvaluateSkipConditions/EvaluateAllConditions/EvaluateAnyConditions/AnyConditionMatches (lines ~550-681, pre-existing) and the new EvaluatePlanningConditions/EvaluateSkipPlanningConditions/EvaluateAllPlanningConditions/EvaluateAnyPlanningConditions/EvaluateGroupedPlanningConditions (lines ~321-541) walk the same Skip → All → Any → grouped precedence and build the same SkipDecision.Skip(...) messages — the planning path just layers IsResolved/deferred-condition bookkeeping on top. ~220 lines of near-duplicate branching. If the evaluation precedence changes, or a new ConditionLogic case is added, both copies have to be kept in lockstep or the --graph output silently diverges from real skip behavior. A single evaluator parameterized by a small "resolution" strategy (returning (SkipDecision?, IsResolved)) would remove that lockstep risk. Same shape of duplication shows up once more at smaller scale in the CLI parser — TryReadGraph/TryReadGraphPath (PipelineCommandLineParser.cs:183-279) reimplement the --option value / --option=value guard logic that TryReadValues already provides for --module/--categories/etc., rather than extending that helper.

None of this is a correctness bug in what's shipped — it's an increase in the number of places future maintainers have to touch in lockstep for this specific feature area. Worth a follow-up cleanup pass, not a blocker.

Minor, non-blocking

• ModuleDiscoveryPlanner.InstructionTouchesStaticState (ModuleDiscoveryPlanner.cs:952-964) has no remaining callers — every call site (MethodTouchesStaticStateCore) calls InstructionTouchesStaticStateCore directly with an explicit runtimeType. Dead code, safe to delete.
• IsGraphPath (PipelineCommandLineParser.cs:239-243) treats any bare token right after --graph <format> as the output path (as long as it doesn't start with -- or look like key=value). Every other multi-token option in this parser requires an explicit flag before its value; this one silently swallows the next positional argument, which — before this PR — would have been forwarded to HostArguments. E.g. -- --graph mermaid deploy treats deploy as the graph path instead of passing it through. Narrow edge case, but worth tightening (e.g. require --graph-path explicitly, or --graph=mermaid, rather than positional lookahead) since the failure mode (an argument silently vanishes into the wrong option) is easy to miss.
• IPlanningModuleCopyProvider.CreatePlanningCopyFromRegisteredInstance (Module.cs:132-141) does a shallow MemberwiseClone(). The framework does guard against the one place this is known to bite (RejectRuntimeBoundPlanningCondition throws if the module's SynchronousPlanningSkipCondition delegate references the runtime instance or mutates captured state), but that guard only inspects that one delegate — not the module's own instance fields more broadly, nor the new IPlanningSafeModuleRegistrationEventReceiver receivers (whose doc comment already acknowledges "the receiver is invoked again during execution"). Since modules are DI singletons and ExportDependencyGraphAsync → RunAsync on the same IPipeline instance is an explicitly supported sequence (PipelineExecutionState only blocks concurrent overlap, not sequential), a module with a mutable reference-type field that a planning-safe skip condition or registration receiver touches could theoretically leak mutated state into the singleton used for the real run. I couldn't find a test exercising "planning copy shares a mutable field, planning-time evaluation mutates it, real run observes the mutation" specifically — might be worth one, purely for regression insurance, even if today's known call sites don't trigger it.
• A few reflection/attribute lookups are uncached in the planning-only path (ModuleMetadataRegistry.HasAttribute<T> recomputes via CustomAttributeMetadata.GetApplicable instead of routing through the same _attributesByType cache GetAttribute/GetAttributes use; DependencyGraphExporter.PropagateUnresolvedSkipDecisions is O(N²) per export; per-module run-condition checks in DependencyGraphExporter.ApplyRunConditionsAsync run sequentially instead of via Task.WhenAll). All confined to the opt-in --graph/--list path, not real pipeline execution, so purely a "nice to have" for anyone exporting large graphs repeatedly.

Verdict

No blocking issues. The rebase is a no-op as described, and CI is still running on the new base at the time of this review. The duplication/dead-code items above are good candidates for a fast follow-up PR rather than reasons to hold this one — the feature itself (exporter, CLI, GitHub summary integration, 126 tests in DependencyGraphExporterTests.cs) is solid and well-tested.

[thomhurst]

CI audit: exact-head Codex and Claude reviews are clean with zero unresolved threads. Ubuntu, Windows, and macOS all fail on the same unrelated default-branch compile defect: ValidationTests.ThrowingPipelineExceptionValidator does not implement IPipelineValidator.ValidateAsync(IServiceProvider). No rerun yet because this deterministic base failure must be fixed first.

[thomhurst]

Rebased the existing four commits cleanly onto current main and refreshed the branch at bef25d0. This is a no-content-change rebase; core Release build succeeded with 0 warnings and 0 errors. Prior exact-head reviews were clean and the old CI failures were the now-fixed default-branch validator defect. @claude review @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bef25d0398

ℹ️ 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]

CI diagnosis: Ubuntu failed only in SpectreResultsPrinterTests shared-console races (LocalOutput_DoesNotAddModuleResultsGroup and GitHubOutput_GroupsOnlyModuleResultsTable); no dependency-graph export test failed. Reran failed jobs for run 31374508874.

[thomhurst]

Updated exact head bfec8fb for the two current review findings. Focused PipelineCommandLineTests: 77/77. Please re-review current head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfec8fb379

ℹ️ 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]

Exact head bfec8fb is green with zero unresolved threads; Codex exact-head review has no inline findings. @claude please review this head and submit an approval if clean.

[thomhurst]

Rebased onto current main and pushed exact head 06c072e. Rebase was conflict-free with no content edits; CI is rerunning.

[thomhurst]

Ubuntu failed while the macOS matrix job remains active; annotations show the shared RunCoreUnitTestsModule failure chain. Final failed logs and the permitted single-job rerun are unavailable until completion. No code change.

[thomhurst]

Ubuntu's sole failure is unrelated RunReportTests.RunHistoryOperationsTimeoutWhenStoreIgnoresCancellation timing out. This PR changes dependency-graph export, not run-history timeout behavior; Windows, macOS, analyzers, AOT, and static checks pass. Triggering the exact head's single allowed failed-job rerun for run 31386374714; no code change.

[thomhurst]

Current head 06c072e has all required checks green, including the permitted Ubuntu failed-job rerun; zero unresolved review threads. The visible Codex review is stale at bfec8fb and Claude's architectural review predates the current implementation history.\n\n@codex review exact head 06c072e\n\n@claude review exact head 06c072e, reassess the current implementation against the earlier planning-architecture concern, and submit approval if clean.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 06c072edd9

ℹ️ 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]

Addressed both current exact-head findings in 1e7107f66af93d1a3ad2bbbfe14125c72bbed0be:

• graph-only planning initializes registered/factory configuration once and preserves it in the planning copy, with factory and pre-created-instance regressions;
• complete grouped RunIfAny failures remain definitive despite unrelated deferred conditions.

Validation: DependencyGraphExporterTests 123/123, ModuleConditionHandlerTests 13/13, core Release build 0 warnings/errors, changed-file formatting clean. All 139 review threads audited with pagination; zero unresolved.

@codex review exact head 1e7107f

@claude review exact head 1e7107f and submit approval if clean.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e7107f66a

ℹ️ 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".

[chatgpt-codex-connector]

Defer groups that may contain deferred alternatives

When a deferred RunIfAny attribute implements IGroupedConditionAttribute and shares its group type with a planning-safe alternative that evaluates false, attributes contains only the safe alternative, so continuing past this line evaluates the incomplete group and returns a definitive skip. Runtime evaluation includes the deferred alternative, which may evaluate true and make the module runnable, causing graph export to report the wrong skip state; deferred condition metadata must distinguish group membership, or affected groups must remain unresolved.

Useful? React with 👍 / 👎.