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.

[thomhurst]

Refreshed from current main in 3aa5cde after the second shared-suite CI failure. Resolved CLI overlaps by preserving dependency-graph export alongside current help bypass, typo guidance, and dry-run success status. Validation: PipelineCommandLineTests 73/73; core Release build 0 warnings/errors.

[claude]

Code review

Re-reviewed at the current head (3aa5cde4f, "Merge remote-tracking branch 'origin/main' into issue-3539-dependency-graph"). This is the delta since the last review at d5d0f40f4d.

This delta: a clean main-sync, no PR-specific changes

The only new commits are the merge itself and c6169c2ec ("Detect common module authoring mistakes (#3433)") from main. That main commit and this PR both touch PipelineCommandLineParser.cs/PipelineImpl.cs/PipelineBuilder.cs, so the merge diff for those files is entirely --help + option-typo-detection code from main, not new graph-export logic.

I checked the merge conflict resolution specifically for regressions in this PR's own --graph/--graph-path handling:

• PipelineBuilder.cs: validatePipeline = _commandLineOptions.Command != PipelineCommand.Help; — Help is the only command that skips initializePipeline/module validation; ExportGraph (and DryRun, Validate, etc.) still validate and initialize as before.
• PipelineCommandLineParser.cs: TryReadGraph/TryReadGraphPath are checked before the new ThrowForLikelyPipelineOptionTypo typo-suggestion path, and GraphOption/GraphPathOption are already present in KnownLongOptions, so --graph/--graph-path continue to parse correctly and won't be misflagged as typos.
• PipelineImpl.cs: the new Help branch in RunAsync is mutually exclusive with the existing PipelineCommandHandler/dry-run/execute branches, so --graph still flows through PipelineCommandHandler.TryExecuteAsync unchanged.

No new bugs and no CLAUDE.md violations in this delta.

---

Separate from this specific delta: this PR is now at ~70 automated review rounds over roughly 24 hours, almost all titled "fix(graph): isolate/preserve/guard planning ...". That pattern is worth a human look — either the remaining issues are genuinely this subtle, or the loop is chasing diminishing returns and would benefit from a manual pass instead of another automated round.

[claude]

Code review

Re-reviewed at the current head (0e64f1555, "Merge remote-tracking branch 'origin/main' into issue-3539-dependency-graph"). This is the delta since the last review at 3aa5cde4f.

This delta: another clean main-sync, no PR-specific changes

The only new commit is the merge itself, bringing in unrelated main work (a new --no-cache CLI flag, ModuleCacheResultRepository changes, ArtifactDemandPlanCache/AtomicFileWriter additions, EngineCancellationToken/ModuleRunner changes, run-report updates, and their tests). None of this PR's own files (DependencyGraphExporter.cs, ModuleDiscoveryPlanner.cs, the CIL analyzer, etc.) were touched by the merge.

I checked the conflict-adjacent files the PR also owns for regressions:

• PipelineCommandLineOptions.cs / PipelineCommandLineParser.cs: main's new --no-cache option was added as its own independent branch (NoCacheOption check at line 150), separate from and unaffected by the --graph/--graph-path handling (TryReadGraph/TryReadGraphPath, lines 119-140). The "graph specified more than once" guard and default-path resolution are untouched.
• PipelineBuilder.cs: only gained DisableModuleCache = _commandLineOptions.DisableModuleCache wiring; the validatePipeline/graph-export flow from the prior review is unchanged.

No new bugs and no CLAUDE.md violations in this delta.

---

Not new, but repeating briefly for visibility since it's unaddressed and this is now ~75 commits / 24+ hours in: the standing architectural concern about ModuleDiscoveryPlanner's hand-rolled CIL "is this side-effect-free" analysis (raised in detail across the last several reviews) is still open. That's a design question for a maintainer, not a defect in this delta — nothing here blocks on it.

[thomhurst]

Refreshed from current main after the required shared-suite failure. Review threads are all resolved; the standing IL-analysis architecture note remains explicitly non-blocking. Validation: DependencyGraphExporterTests 117/117, PipelineCommandLineTests 75/75, and core Release build with 0 warnings/errors.

[thomhurst]

Addressed both current CodeFactor complexity findings at e23fd08c4e: command selection and graph parsing are split from PipelineCommandLineParser.ParseArgument; IL field/call analysis is split from ModuleDiscoveryPlanner.InstructionTouchesStaticStateCore. Behavior remains conservative on unresolved metadata and unsafe virtual dispatch. Validation: PipelineCommandLineTests 75/75; DependencyGraphExporterTests 117/117; changed-file whitespace clean; core Release build 0 warnings/errors. @claude review

[claude]

Code review

Re-reviewed at the current head (e23fd08c4e, "refactor(graph): split planning decisions"). This is the delta since the last review at 0e64f1555.

This delta: a CodeFactor complexity fix, behavior-preserving

Two files changed, both extract-method refactors with no logic changes:

• PipelineCommandLineParser.cs: TrySetCommand/TryParseGraph are split out of ParseArgument, backed by a new CommandOptions dictionary (--help/-h/--list-modules/--validate/--dry-run → PipelineCommand). TryParseGraph still delegates to TryReadGraph for the actual --graph/--graph-path parsing and preserves the "specified more than once" guard. Argument-index handling is unchanged: TryReadGraph only mutates index on a match, so the false path leaves index untouched exactly as before.
• ModuleDiscoveryPlanner.cs: InstructionTouchesStaticStateCore is split into FieldInstructionTouchesStaticState, CalledMethodTouchesStaticState, and AssemblyMethodTouchesStaticState. The important invariant here is that the outer try/catch (ArgumentException) => return true in InstructionTouchesStaticStateCore still wraps the entire chain, including the recursive MethodTouchesStaticStateCore call reached via AssemblyMethodTouchesStaticState — so the fail-closed (treat-as-stateful) behavior on unresolvable metadata is preserved. The [UnconditionalSuppressMessage("Trimming", "IL2026", ...)] annotation moved onto FieldInstructionTouchesStaticState, which is the method now directly calling ResolveField.

CodeFactor now reports the complexity issue fixed, and the referenced validation (PipelineCommandLineTests / DependencyGraphExporterTests) covers both call paths. No new bugs and no CLAUDE.md violations in this delta.

---

Not new, but noting again for visibility: this PR is now at 57 commits / 63 automated review rounds over roughly 52 hours. The individual deltas (like this one) keep coming back clean, which suggests the remaining churn is CI/CodeFactor/main-sync noise rather than substantive issues — this is probably close to mergeable and would benefit from a human decision to stop the loop rather than another automated round.

[thomhurst]

Merged current main. Resolved Module by preserving dependency-graph planning-copy support while adopting main's internal execution contract. Core Release build passes (0 warnings/errors); the core test project is currently blocked by unrelated main RunReportTests references to removed PipelineOptions properties.

[claude]

Code review

Re-reviewed at the current head (f9c9eb129, "Merge remote-tracking branch 'origin/main' into issue-3539-dependency-graph"). This is the delta since the last review at e23fd08c4e: two main-sync merges (96fdf6472, f9c9eb129), no new PR-specific commits.

Merge conflict resolution in Module.cs is correct

main's 0ced7227d ("Hide module engine plumbing from public API #3835") renamed IModule's internal plumbing (ResultTask, TrySetDistributedResult) onto a new IInternalModule interface. The conflict resolution changed Module<T>'s declaration from : IModule, IPlanningModuleCopyProvider to : IInternalModule, IPlanningModuleCopyProvider. IInternalModule : IModule, so Module<T> is still fully assignable to IModule everywhere. This PR's own subsystem (ModuleDiscoveryPlanner.cs, DependencyGraphExporter.cs, DependencyGraphValidator.cs, IPlanningModuleCopyProvider) never references ResultTask/TrySetDistributedResult directly, so the rename doesn't affect the planning-copy/CIL-analysis code this PR added.

Confirmed: test project currently fails to compile

The most recent PR comment says "the core test project is currently blocked by unrelated main RunReportTests references to removed PipelineOptions properties." I verified this — it's real and still present at the current head:

test/ModularPipelines.UnitTests/Engine/RunReportTests.cs#L1481-L1486 (BuilderRegistrationInvokesRunReportEnricher) still sets PrintLogo/PrintResults directly on PipelineOptions:

builder.ConfigurePipelineOptions(options => options with
{
    PrintLogo = false,
    PrintResults = false,
    RunReport = CreateReportingOptions(Path.Combine(directory, "report.json")).RunReport,
});

PipelineOptions no longer has top-level PrintLogo/PrintResults — they moved under Console (PipelineConsoleOptions) in src/ModularPipelines/Options/PipelineOptions.cs. Every other PrintLogo/PrintResults usage in this same file (10 other call sites) and in TestPipelineBuilder.cs/PipelineProgressTests.cs already uses the correct nested form, e.g. line 1979: Console = options.Console with { PrintLogo = false, PrintResults = false }. This one call site was missed, and it won't compile (CS0117), which blocks the entire ModularPipelines.UnitTests assembly — not just this one test.

It's genuinely unrelated to this PR's own graph-export feature (it's fallout from a main rename), but since it's now sitting on this PR's branch and blocks the whole test project from building, it's worth fixing before merge rather than leaving for the "next review cycle." Suggested fix:

        builder.ConfigurePipelineOptions(options => options with
        {
            Console = options.Console with { PrintLogo = false, PrintResults = false },
            RunReport = CreateReportingOptions(Path.Combine(directory, "report.json")).RunReport,
        });

No other issues found in this delta. The unrelated main additions pulled in by the merge (GitHubRunReportEnricher, the new Status.DependencyFailed case in GitHubMarkdownSummaryGenerator) don't touch this PR's own files and don't need review here.

[thomhurst]

Queue dependency: the exact RunReportTests nested Console correction requested by the current-head review is isolated in #3864, whose fresh full workflow is running. This PR will need a current-main refresh after #3864 merges, then a clean re-review; no duplicate patch or rerun here.

[thomhurst]

Rebase replay conflicted across this long-lived 58-commit branch, so current main was merged into the branch instead (GitHub reported the trees mergeable). This preserves branch history and brings in the RunReportTests fix from #3844 for a fresh CI/review cycle. Current head: 79ea2f9.

[thomhurst]

@codex review

[thomhurst]

@claude review

[claude]

Code review — re-review at current head (79ea2f9a)

Scope of this pass: the only commit since the last reviewed head (f9c9eb129) is a main-sync merge (2b0fa0ff6, "Respect non-throwing pipeline failure mode #3844"), which brought in the RunReportTests.cs nested-Console fix that a previous review flagged as a compile blocker. Verified directly at the current head — RunReportTests.cs now uses Console = options.Console with { PrintLogo = false, PrintResults = false } — so that blocker is resolved. No new PR-authored code landed in this delta, and the merge doesn't touch any of this PR's own files (ModuleDiscoveryPlanner.cs, DependencyGraphExporter.cs, ModuleConditionHandler.cs, etc.), so nothing new to bug-hunt there.

Given the enormous review history on this PR already (45+ "fix(graph): ..." commits addressing narrow edge cases one at a time), I focused this pass on stepping back and asking a design question none of the incremental rounds seem to have asked: is the overall approach the right one?

Architectural concern: the discovery mechanism is fighting the problem, not solving it

ModuleDiscoveryPlanner.cs (1541 lines, new) discovers the graph by constructing near-real module instances (CreatePlanningModule, requiring every module to implement the new internal IPlanningModuleCopyProvider) and then proving after the fact that doing so was safe:

• It hand-decodes raw CIL opcodes at runtime to detect whether a module's Configure() method (or a captured delegate) touches static state or mutates a delegate's target field — see MethodTouchesStaticStateCore/ReadOpCode/GetOperandSize (ModuleDiscoveryPlanner.cs:898-1180) and MutatesDelegateTargetField (:844).
• It reaches into private Microsoft.Extensions.DependencyInjection implementation details via reflection — _disposables, ResolvedServices, CallSiteFactory, _callSiteCache (:1428-1476) — to determine whether an object is container-owned.
• It does deep reflective field-graph comparison to verify a clone didn't retain live state (HasEquivalentFields/HasEquivalentState, :361-570).
• 25 separate [UnconditionalSuppressMessage] trim/AOT suppressions are scattered through the file — itself a signal the approach is fighting the toolchain's own safety analysis rather than working with it.

This is exactly the kind of thing that produces a 45-commit tail of "harden planning isolation," "reject opaque replay state," "close planning safety gaps," "prevent planning constructor replay," etc.: verifying safety by inspecting bytecode/DI internals after construction has an unbounded edge-case surface, because any new module shape, closure, or a future .NET/DI internals change can reopen a "gap." Each round so far has closed one instance of the problem, not the problem class.

Why this matters beyond code cleanliness: the private-DI-internals reflection (_callSiteCache, ResolvedServices, etc.) is coupled to the exact shape of Microsoft.Extensions.DependencyInjection's internal implementation. That's not a hypothetical maintenance cost — it's a real one: any DI package upgrade that changes those private field/property names silently breaks graph export (likely via a caught/suppressed exception rather than a build error, given the pattern in this file), and nothing in the type system will catch it in CI.

Suggested alternative — verify safety by construction, not by inspection: the repo already has its own Roslyn analyzer project (src/ModularPipelines.Analyzers, with existing analyzers like AsyncModuleAnalyzer, ConflictingDependsOnAttributeAnalyzer). Dynamic dependency declarations (DependsOnModulesInCategoryAttribute, DependsOnModulesWithAttributeAttribute, DependsOnModulesWithTagAttribute — all now PlanningSafeDependsOnBaseAttribute) and conditions (OnLinux, IsCI, etc. — now IPlanningRunCondition) are already required to be side-effect-free/deterministic by doc-comment convention. Instead of instantiating real modules and forensically verifying the result at runtime, a compile-time analyzer could enforce (or a narrower static-only API could structurally guarantee) that anything consulted during graph discovery is provably side-effect-free — no instance construction, no CIL disassembly, no DI-internals reflection needed at all. That moves the safety guarantee from "checked per-run, per-module, via best-effort heuristics with a growing exception list" to "checked once, at compile time, for every module shape." It's a bigger upfront redesign, but it would eliminate the entire class of bug this PR has been chasing for two days of iteration, rather than the next instance of it.

Everything else looks solid

• DependencyGraphExporter.cs's actual Mermaid/DOT/JSON renderers (:529-673) are clean, small, and independently testable — the complexity is isolated to the discovery/skip-resolution glue above them, not the rendering itself.
• The [On*]/IsCI condition attribute changes are minimal, consistent one-liners (interface swap to IPlanningRunCondition).
• CLI (--graph) wiring, DI registration, and the GitHubMarkdownSummaryGenerator flowchart integration follow existing repo conventions.
• Test coverage is extensive (DependencyGraphExporterTests.cs at 4749 lines) — though its sheer size is itself a symptom of the above: that many edge-case tests are needed because the discovery mechanism has that many edge cases to guard.

Given how far along this PR already is and how much review cycling has gone into hardening the current approach, I'm flagging this as a design discussion for the maintainer rather than a blocking request — the current code may well be "safe enough" after 45 rounds of hardening. But if further "planning" edge cases keep surfacing after merge, this is where I'd look first: not the next missing null-check, but whether "activate-then-verify-via-bytecode-inspection" is the right foundation at all.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79ea2f9aba

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

Bypass runtime validation for graph-only builds

When this graph-only helper is used on a pipeline with a custom DependsOnBaseAttribute, the normal BuildAsync path still runs full dependency validation before the graph-safe exporter is reached; that validation calls ModuleDependencyValidator/ModuleDependencyResolver with planningSafeOnly: false, constructing the attribute and invoking ShouldDependOn. The exporter itself now defers those predicates, but this new entry point can still mutate shared/static state or throw during graph export before any module execution, so graph builds need a planning-safe build/validation path rather than the regular runtime validation path.

Useful? React with 👍 / 👎.

[thomhurst]

Refreshed onto current main after the prior Ubuntu full-suite host ended without a failing-test summary (core test exit 2, workflow exit 134). The refresh exposed four obsolete direct-IModule fixture expectations; current main rejects those registrations before planning, so the tests now assert that contract without activating/configuring the invalid modules. Validation on 8b60cdf: DependencyGraphExporterTests 117/117 and ModularPipelines.slnx Release build succeeded with 0 warnings/errors.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8b60cdf997

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

Use planning modules for graph skip-condition contexts

When a synchronous WithSkipWhen evaluated for graph export calls context.GetModuleIfRegistered<T>() (for example, an optional-dependency presence check), this context is resolved from the real service provider, whose ModuleLookup contains the runtime modules rather than the planning copies. GetModuleIfRegistered is not blocked by moduleResultAccessAllowed, so otherwise planning-safe callbacks have to be rejected by the scanner; if a custom planning copy reaches evaluation, the callback can receive and mutate the runtime module before RunAsync. Use a planning IPipelineContext/ModuleLookup for these evaluations or block optional module access consistently.

Useful? React with 👍 / 👎.