Add required ModuleResult value access

[thomhurst]

Summary

• add ModuleResult<T>.Value for required, non-null dependency results
• throw outcome-specific InvalidOperationException messages for failure, skip, and null success
• preserve Success construction, nullable variant access, deconstruction, and JSON round-tripping
• teach the required-value path in README and core documentation

Test plan

• ModuleResultContractTests (8/8)
• targeted dotnet format --verify-no-changes --severity info
• npm run build --prefix docs
• core Release build (ModularPipelines.sln, 0 errors)

Closes #3490

[thomhurst]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b82cb77c49

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

Verified: built ModularPipelines.sln (core) and ran ModuleResultContractTests on the PR branch — 0 errors/warnings, 8/8 tests pass, matching the PR description.

No blocking bugs found. ModuleResult<T>.Value correctly resolves to the pattern-matched Success type's hiding property inside the switch (no accidental recursion), and the T?→T narrowing compiles cleanly for both reference and value T (confirmed by the successful build, e.g. Module<int> in the test suite).

Two non-blocking suggestions:

1. Member hiding on Success.Value is a readability trap. Success declares public new T? Value { get; init; }, hiding the base ModuleResult<T>.Value (the new throwing accessor). That means .Value behaves differently depending only on the static type of the reference — non-throwing/nullable when the variable is typed as Success, throwing when typed as ModuleResult<T>, even for the exact same object. This is a classic C# footgun (new hiding + static dispatch), and it's easy for a future contributor to hit a silent behavior change just by narrowing/widening a variable's declared type. Since the nullable-accessor is really only needed for the Success { Value: var x } pattern-matching idiom already used in the docs, consider renaming it (e.g. RawValue) to avoid the same identifier meaning two different things, rather than relying on hiding + new.

2. Follow-up opportunity, not required here: the new Value accessor is exactly the safe replacement for the .ValueOrDefault! null-forgiving pattern used throughout src/ModularPipelines.Build/Modules/* and src/ModularPipelines.Examples/Modules/** (e.g. FindProjectDependenciesModule.cs, PackProjectsModule.cs, the Examples/Modules/Approvals|Launch|Propulsion|Systems modules). Those call sites are exactly the "required dependency" case this PR is designed for, and migrating them would let the codebase drop the null-forgiveness operator in favor of a real, module-aware exception message. Worth a follow-up PR rather than blocking this one, since it's unrelated to the current diff.

Nice addition overall — the Value/ValueOrDefault split is clean, the exception messages are genuinely useful for debugging, and the docs/README updates match the new API well.

[thomhurst]

Addressed the P2 record-formatting finding in 9fd80c5192. Validation: focused ToString regressions 4/4, full ModuleResultContractTests 12/12, scoped formatting/analyzers clean, core Release build clean (228 existing warnings). @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9fd80c5192

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

Verified on HEAD (9fd80c5192): rebuilt ModularPipelines.sln (core, 0 errors/warnings), ran the full ModuleResultContractTests suite (12/12 pass), and dotnet format --verify-no-changes --severity info is clean on the changed file.

Follow-up on prior review rounds

• ✅ Codex's "unsafe record formatting" P2 is fixed and verified. The new ModuleResult<T>.PrintMembers override stops the throwing Value accessor from being evaluated by ToString()/logging on Failure/Skipped/null-Success, while Success still prints its own safe value exactly once. The four regression tests added for this (Failure_ToString_..., Skipped_ToString_..., NullSuccess_ToString_..., Success_ToString_PrintsValueOnce) all pass.
• ❌ Codex's second P2 — constructor parameter rename — is still unaddressed. ModuleResult<T>.Success used to be a positional record (Success(T? Value)), which generates a public constructor parameter literally named Value. The rewritten explicit constructor (ModuleResult.cs:353) is public Success(T? value) (lowercase). C# named arguments are case-sensitive, so any external caller using new ModuleResult<int>.Success(Value: 42) will fail to compile (CS1739) after upgrading. The linked issue (#3490) explicitly scopes this change as "Breaking: no (additive)" — this regresses that guarantee for a real (if narrow) call pattern. Simple fix: keep the constructor parameter named Value to match the pre-existing public surface, e.g. public Success(T? Value) { this.Value = Value; }.

Design note (non-blocking, previously raised)

Success hides the base's new throwing Value with public new T? Value { get; init; } (nullable, non-throwing). This means .Value has two different meanings depending purely on the static type of the reference — throwing/non-null when typed as ModuleResult<T>, nullable/non-throwing when typed as Success. It's a deliberate trade-off to preserve the existing case Success { Value: var x } pattern-matching idiom, but it's a classic C# footgun (member hiding + static dispatch) — a future reader narrowing/widening a variable's declared type gets silently different behavior from the same object. Not blocking, but worth a doc comment on Success.Value calling out that it intentionally shadows the base accessor, so nobody "cleans it up" later and reintroduces the exception-during-logging bug this PR just fixed.

Overall the core design (throwing Value mirroring Nullable<T>.Value semantics, safe PrintMembers override, docs updates) is solid and well tested — just want the constructor-naming regression closed before merge.

[thomhurst]

Addressed named-argument source compatibility in c5668608fe after rebasing onto current main. The focused regression test passes, scoped formatting/analyzers pass, and the core Release build passes with 0 errors (228 existing warnings).

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5668608fe

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

Verified on latest commit c5668608fe: rebuilt ModularPipelines.sln (core, 0 errors/warnings) and ran ModuleResultContractTests — 13/13 pass, matching the PR's updated test plan.

Previous feedback — status check

• The two @codex-flagged follow-ups (record-formatting regression fixed in 9fd80c519, named-argument source compatibility fixed in c5668608f) are both correctly resolved. I independently confirmed the PrintMembers override in ModuleResult<T> (src/ModularPipelines/Models/ModuleResult.cs:474-475) is load-bearing, not cosmetic: without it, the compiler-synthesized PrintMembers for ModuleResult<T> would include the new throwing Value property, so calling .ToString() on any Failure/Skipped result would itself throw. I verified this by reproducing the pattern in isolation — a throwing property left in the auto-generated member list does blow up ToString(). The override plus the four new regression tests (Failure_ToString_DoesNotEvaluateRequiredValue, etc.) close that gap correctly.
• The named-argument fix is also verified: Success(T? Value)'s parameter keeps the capitalized name, so all existing positional call sites (new ModuleResult<T>.Success(value) across the distributed test suite) and the new named-argument test (Success_Constructor_PreservesValueNamedArgument) both compile.
• I also independently confirmed the previously-raised equality concern about converting Success from a positional record to an explicit constructor + manually-declared new T? Value property does not regress Equals/GetHashCode — non-positional auto-properties still participate in record-synthesized equality, confirmed with a small isolated repro.

Still-open, non-blocking suggestion (carried over from the earlier review)

The member-hiding footgun raised previously — Success declaring public new T? Value { get; init; } to shadow the base's new throwing Value — hasn't been addressed, and I'd still recommend fixing it rather than leaving it, for a concrete architectural reason beyond style: new-hiding makes behavior depend on the static type of the reference, not the runtime type, which is unusual for a discriminated-union-style API where callers are trained to trust pattern matching and rely on consistent member semantics. Concretely, ((ModuleResult<T>)success).Value throws-on-null while success.Value (statically typed as Success) does not, for the exact same object — a footgun a future contributor could easily hit by widening/narrowing a variable's declared type during a refactor, with no compiler warning. Since the nullable accessor is only actually needed for the Success { Value: var x } pattern-matching idiom already used in the docs, renaming it (e.g. RawValue) removes the ambiguity entirely and is a strictly safer design than hiding + new — same ergonomics for the pattern-matching case, zero risk of static-dispatch surprises for direct member access. Not a blocker, but worth doing before this shape of API sees wider adoption/follow-up migrations (e.g. the ValueOrDefault! → .Value migration suggested in the earlier review).

No new bugs found. Nice, well-tested addition — the Value/ValueOrDefault split mirrors Nullable<T>'s own .Value/.GetValueOrDefault() convention, which is a good precedent to follow here.

[thomhurst]

Restored the positional record's named Deconstruct(Value: ...) source API with regression coverage and a documented SA1313 compatibility suppression. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4265d6aa29

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

Verified on latest commit 4265d6a ("fix(results): preserve deconstruct API"): rebuilt ModularPipelines.sln (core, 0 errors/0 warnings) and ran ModuleResultContractTests — 13/13 pass, including the new Success_Value_ReturnsValue test that exercises success.Deconstruct(Value: out var deconstructedValue).

This round's change

This commit restores the manual Deconstruct(out T? Value) on ModuleResult<T>.Success (src/ModularPipelines/Models/ModuleResult.cs:365-371) that was implicitly generated back when Success was a positional record. I confirmed no other call site in the repo relied on deconstructing Success (grep for Deconstruct/is ... Success( turned up nothing outside this file and the new test), so this is a clean, low-risk restoration of source compatibility rather than a behavior change — good fix.

Status of all prior review rounds

Every previously-raised issue across this PR's iterations is now resolved and independently re-verified:

• Record-formatting-throws-during-ToString() (P2) — fixed via the PrintMembers override, still load-bearing, still passing its 4 regression tests.
• Named-argument source compatibility (new Success(Value: 42)) — fixed, still passing.
• Positional Deconstruct source compatibility — fixed in this commit, now covered by a regression assertion.

Still-open, non-blocking design note (carried over, not new)

Success still hides the base's throwing Value with public new T? Value { get; init; }. As raised in earlier rounds, this means .Value throws-on-null when statically typed as ModuleResult<T> but doesn't when statically typed as Success — for the same object. This is a deliberate, working trade-off to keep the Success { Value: var x } pattern-matching idiom, and not a blocker, but worth a RawValue-style rename or at least a doc-comment callout before this shape sees wider adoption (e.g. the suggested ValueOrDefault! → .Value migration in src/ModularPipelines.Build/Examples).

No new bugs found. This is a solid, well-tested addition — nice work tracking down each source-compatibility edge case across the review rounds.

[thomhurst]

Synced the required-value example into README_Template.md in 0d652e19d7, so GenerateReadMeModule preserves the checked README change. Exact snippets compared equal; no stale buildResult.ValueOrDefault! example remains.

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: 0d652e19d7

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

Verified on latest commit 0d652e19d7 ("docs(readme): sync required value example"): rebuilt ModularPipelines.sln (core, 0 errors/0 warnings) and ran ModuleResultContractTests — 13/13 pass. src/ModularPipelines/Models/ModuleResult.cs is unchanged since the previously-verified 4265d6a, so this round is docs-only.

This round's change

This commit syncs the required-value example from README.md into README_Template.md (buildResult.ValueOrDefault!.OutputPath → buildResult.Value.OutputPath). I checked GenerateReadMeModule (src/ModularPipelines.Build/Modules/GenerateReadMeModule.cs:26-56): it reads README_Template.md, substitutes only the %%% AVAILABLE MODULES PLACEHOLDER %%% token, and diffs the result against the checked-in README.md, failing the build step if they don't match. Since the example text isn't part of the substituted placeholder, the two files had to be updated in lockstep — this fix is correct and closes the drift that would otherwise have broken that check.

Status of all prior review rounds

Every previously-raised issue across this PR's iterations remains resolved:

• Record-formatting-throws-during-ToString() — fixed via the PrintMembers override, still passing its regression tests.
• Named-argument source compatibility (new Success(Value: 42)) — fixed, still passing.
• Positional Deconstruct source compatibility — fixed, still covered by a regression assertion.

Still-open, non-blocking design note (carried over, not new)

Success still hides the base's throwing Value with public new T? Value { get; init; } (ModuleResult.cs:362). As raised in earlier rounds: .Value throws-on-null when statically typed as ModuleResult<T> but doesn't when statically typed as Success, for the same object — a static-dispatch footgun that's easy to trip over when narrowing/widening a variable's declared type during a future refactor. This is a deliberate trade-off to keep the Success { Value: var x } pattern-matching idiom working, and I'm not blocking on it again, but it's the one item from this review series that hasn't been addressed — worth a RawValue-style rename or a doc-comment callout before this API sees the suggested ValueOrDefault! → .Value migration across src/ModularPipelines.Build/Examples.

No new bugs found. This is a well-tested, source-compatible addition — nice work tracking down every edge case across the review rounds.

[thomhurst]

Rebased onto current main; new head 5eef295306. Whole-PR patch-id remained exact: 476c14c23013fe15585957e42559e2577d378b3c.

Validation after rebase:

• ModuleResultContractTests: 13/13 passed
• ModularPipelines.sln Release build: 0 errors (227 pre-existing warnings)
• docs production build: passed
• README and README_Template.md required-value snippets: exact match
• stale buildResult.ValueOrDefault!: absent
• git diff --check: passed

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 5eef295306

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

Verified on latest commit 5eef2953 ("docs(readme): sync required value example"): this commit is a rebase onto current main with an unchanged whole-PR patch-id (476c14c2...), so I independently re-verified rather than assuming it. Checked out the branch in an isolated worktree, built ModularPipelines.sln (core) with plain dotnet build -c Release — 0 errors, 0 warnings — and ran the full ModuleResultContractTests suite via dotnet run --framework net10.0 -- --treenode-filter "/*/*/ModuleResultContractTests/*" — 13/13 pass. Also confirmed README.md and README_Template.md now carry the identical buildResult.Value.OutputPath example (no stale ValueOrDefault! left in either).

(Aside, unrelated to this PR: the scripts/Invoke-AgentDotNet.ps1 wrapper's -SingleNode flag fails with MSB1008: Only one project can be specified in this environment when it appends -m:1 — plain dotnet build/dotnet run work fine. Flagging in case it's an environment quirk worth a look separately; not something to fix in this PR.)

Status of all prior review rounds

Every issue raised across this PR's review history is still resolved on this commit:

• Record ToString()/logging throwing via the Value accessor (P2) — fixed by the ModuleResult<T>.PrintMembers override (ModuleResult.cs:477), which suppresses the base's throwing Value from auto-printing while Success still prints its own safe value once.
• Named-argument source compatibility (new Success(Value: 42)) — fixed; constructor parameter is still Value (capitalized) via the documented SA1313 suppression.
• Positional Deconstruct source compatibility (success.Deconstruct(Value: out var x)) — fixed and covered by a regression test.
• README/README_Template drift — fixed, both files match exactly.

Still-open, non-blocking design note (carried over, not new)

ModuleResult<T>.Success still hides the base's throwing Value with public new T? Value { get; init; } (ModuleResult.cs:362). .Value throws-on-null when statically typed as ModuleResult<T> but doesn't when statically typed as Success, for the same object — a static-dispatch footgun if a future refactor narrows/widens a variable's declared type. This is a deliberate, working trade-off to keep the case Success { Value: var x } pattern-matching idiom intact, and I'm not blocking on it — just noting it's the one item from this review series that hasn't been addressed, and would be worth a RawValue-style rename or a doc-comment callout before the suggested ValueOrDefault! → .Value migration lands elsewhere in the codebase (src/ModularPipelines.Build/Examples).

No new bugs found. This is a well-tested, source-compatible addition with good exception messages for debugging — nice work tracking every edge case down across the review rounds.

[thomhurst]

Rebased onto current main after #3564 merged. Patch equivalence preserved (476c14c23013fe15585957e42559e2577d378b3c). New head: 9e24fdd8e4.

@codex review

[claude]

Code review

Verified on latest head 9e24fdd8e4: this commit is a rebase-only update onto current main (after #3564 merged). Confirmed independently via git patch-id --stable on main...pr-head — the resulting patch-id (476c14c23013fe1...) is byte-identical to the previously-reviewed 5eef2953 commit. No source lines changed.

Since the diff content is unchanged, the prior review round stands in full:

• Record ToString()/logging throwing via the Value accessor — fixed via ModuleResult<T>.PrintMembers.
• Named-argument source compatibility (new Success(Value: 42)) — fixed.
• Positional Deconstruct source compatibility — fixed, regression-tested.
• README/README_Template.md required-value example drift — fixed, both files match.

Still-open, non-blocking design note (carried over, not new): ModuleResult<T>.Success still hides the base's throwing Value with public new T? Value { get; init; }. .Value throws-on-null when statically typed as ModuleResult<T> but not when statically typed as Success, for the same object — a static-dispatch footgun if a future refactor narrows/widens a variable's declared type. This remains a deliberate trade-off to keep the case Success { Value: var x } pattern-matching idiom working, and isn't a blocker — but a RawValue-style rename (or at least an XML-doc callout on Success.Value) would remove the ambiguity before the suggested ValueOrDefault! → .Value migration lands elsewhere in the codebase (src/ModularPipelines.Build/Examples), since call sites there will otherwise silently get different throw behavior depending on which static type they hold the result as.

No new bugs found. Nothing to re-verify beyond confirming the rebase is patch-equivalent — approving from a correctness standpoint.

[chatgpt-codex-connector]

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

Reviewed commit: 9e24fdd8e4

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

All four prior P2 findings were already fixed and are now resolved. Also documented the intentional static-type difference for Success.Value without breaking its existing source-compatible name. ModuleResult contract tests pass (13/13); core Release build has 0 errors. Branch updated from main. @codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: a07f310a00

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

Verified on latest head a07f310a00 (merge of main into the branch): rebuilt ModularPipelines.sln (core, 0 errors/0 warnings) and ran ModuleResultContractTests — 13/13 pass.

This round's change

The only substantive change since the last review round is a 5-line XML <remarks> doc comment on Success.Value (

ModularPipelines/src/ModularPipelines/Models/ModuleResult.cs

Lines 358 to 368 in a07f310

	/// <summary>
	/// Gets the value produced by the module, which may be <c>null</c>.
	/// </summary>
	/// <remarks>
	/// This property intentionally hides the required <see cref="ModuleResult{T}.Value"/>
	/// accessor to preserve the nullable value carried by a known successful result.
	/// Access through <see cref="ModuleResult{T}"/> uses the required accessor instead.
	/// </remarks>
	public new T? Value { get; init; }

), explaining that it intentionally hides the base's throwing ModuleResult<T>.Value accessor to preserve the nullable value for the Success { Value: var x } pattern-matching idiom. This closes out the one still-open item carried across every prior round of this review — the member-hiding footgun is now explicitly documented at the point a future reader would encounter it, rather than left implicit. The rest of the diff vs. the previously-reviewed 9e24fdd8e4 is unrelated test-infrastructure churn (HttpTests.cs, DownloaderTests.cs, LocalHttpServer.cs) pulled in by merging main, not authored by this PR.

Status of all prior review rounds

Every issue raised across this PR's review history remains resolved:

• Record ToString()/logging throwing via the Value accessor — fixed via the PrintMembers override.
• Named-argument source compatibility (new Success(Value: 42)) — fixed.
• Positional Deconstruct source compatibility — fixed, regression-tested.
• README/README_Template.md required-value example drift — fixed, both files match.
• Success.Value static-dispatch footgun — now documented (this round), a reasonable resolution given the constraint of preserving the existing pattern-matching idiom.

No new bugs found. Nothing outstanding from this review series — the design is sound, well-tested, and now well-documented.