Simplify module result metadata names - #4268
Conversation
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR renames five ChangesModuleResult metadata contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to This PR renames the result metadata in both the public API and persisted/distributed JSON. Mixed-version workers or existing cache entries can fail to deserialize results during rollout or rollback, so merging should wait for an explicit compatibility, deployment, and cache-invalidation plan. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes implement the linked issue requirements [ Full details: Out of Scope Changes checkExplanation All changes support the linked issue [ Full details: Docstring CoverageExplanation Docstring coverage is 7.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 76 functions across 25 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR renames ModuleResult metadata across the public API, serialization, distributed processing, reports, and tests.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/ModularPipelines/Models/ModuleResult.cs | Renames result metadata and updates generic and non-generic converters to emit new keys while reading both key sets. |
| src/ModularPipelines/Models/IModuleResult.cs | Replaces the five Module-prefixed public metadata members with their concise v4 names. |
| RELEASE_NOTES_V4.md | Adds the public member and serialized-field migration mapping. |
| test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs | Adds exact public API contract and legacy JSON compatibility coverage. |
| src/ModularPipelines/Distributed/Serialization/ModuleResultSerializer.cs | Aligns distributed type metadata normalization with the renamed TypeName member. |
Reviews (6): Last reviewed commit: "fix(api): baseline renamed result member..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Inactive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2f1a8142f
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Reviewed the mechanical rename of IModuleResult/ModuleResult metadata (ModuleName→Name, ModuleTypeName→TypeName, ModuleDuration→Duration, ModuleStart→StartTime, ModuleEnd→EndTime) plus the lowercased Success/Deconstruct parameter names. This matches issue #4221 exactly, and I spot-checked every call site across src/ and test/ (production code, distributed serializer, GitHub summary generator, Spectre printer, dependency graph exporter, and all the affected test projects) — the rename is complete and consistent; I found no leftover references to the old names and no stale Value:-named arguments against the now-lowercase constructor/Deconstruct parameter. The new ModuleResultContractTests.MetadataSurface_UsesConciseNames test is a good addition that locks the public surface down via reflection so this can't silently regress.
One real gap, consistent with what Greptile's automated pass already flagged independently:
Missing RELEASE_NOTES_V4.md entry for this breaking rename. Every other breaking change in this v4 cycle (ExecutionMode→FailureMode, ExecutionType→ExecutionHint, PipelineBuilderOptions→PipelineBuilderSettings, ArgumentPlacement→CommandLinePhase, etc.) has a corresponding section in RELEASE_NOTES_V4.md with before/after names and, where relevant, a migration snippet. This PR renames five public IModuleResult members and changes the wire JSON field names (ModuleResultNonGenericJsonConverter/ModuleResultJsonConverter<T> in src/ModularPipelines/Models/ModuleResult.cs) but adds no corresponding entry. That's inconsistent with the established pattern in this exact file and leaves consumers — including anyone reading persisted/distributed ModuleResult JSON directly — without documented migration guidance, unlike the precedent set by keeping a "ModuleStatus" read-compat case for the earlier Status rename in the same converters.
Suggest adding a short section to RELEASE_NOTES_V4.md alongside the existing entries, listing the five renamed members (and the JSON field rename) so consumers migrating to v4 have one place to look, matching how every other breaking change in this file is documented.
e2f1a81 to
ea9f106
Compare
|
Addressed the blocking review in ea9f106: RELEASE_NOTES_V4.md now lists all five IModuleResult/ModuleResult member renames and the matching JSON field migration. Rebased onto current main. Exact-head validation: core/tests Release build passed (178 existing warnings, 0 errors); ModuleResultContractTests passed 23/23. |
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD ea9f1060. This PR is a mechanical rename of IModuleResult/ModuleResult metadata (ModuleName→Name, ModuleTypeName→TypeName, ModuleDuration→Duration, ModuleStart→StartTime, ModuleEnd→EndTime) plus lowercasing the Success constructor/Deconstruct parameter. I re-checked every call site touched across src/ and test/ — production code, the distributed serializer, GitHub summary generator, Spectre printer, dependency graph exporter, and all affected test projects — and the rename is complete and consistent, matching issue #4221. The ModuleResultContractTests.MetadataSurface_UsesConciseNames reflection test is a good guard against future regressions.
Previously flagged and now resolved: my prior review blocked on the missing RELEASE_NOTES_V4.md entry. Commit ea9f1060 ("docs: describe module result migration") adds a "Module result metadata" section listing all five renames, consistent with how every other v4 breaking change in that file is documented. That gap is closed.
Still open — legacy JSON key compatibility (flagged by the automated codex-connector review, not addressed):
ModuleResultNonGenericJsonConverter.Read and ModuleResultJsonConverter<T>.Read in src/ModularPipelines/Models/ModuleResult.cs only recognize the new property names (Name, TypeName, Duration, StartTime, EndTime) when deserializing. Unlike Status, which explicitly keeps case "ModuleStatus": alongside case "Status": for read-compat with a prior rename, none of these five new names got the same treatment. A JSON payload written by a pre-v4 build (ModuleName, ModuleTypeName, ModuleDuration, ModuleStart, ModuleEnd) will silently fail to populate name, and the converter throws "Name is required but was not found in the JSON." on read.
This converter backs ModuleResultSerializer in src/ModularPipelines/Distributed/Serialization/ModuleResultSerializer.cs, i.e. cross-process distributed communication and any persisted result payloads. Because the existing ModuleStatus case already establishes the project's pattern for this exact scenario (rename a field, keep the old JSON key readable), leaving the other four unhandled is an inconsistency rather than a deliberate decision — it reads as an oversight in an otherwise careful rename, not a "this is fine because it's a major version" call. Concretely, it breaks:
- Rolling/mixed-version distributed deployments (old worker → new master, or vice versa) exchanging
ModuleResultJSON mid-upgrade. - Any consumer that persists
ModuleResultJSON across process restarts/upgrades (e.g. caching, custom result repositories) and now gets a hardJsonExceptioninstead of a value with defaulted/renamed fields.
Suggest adding case "ModuleName":, case "ModuleTypeName":, case "ModuleDuration":, case "ModuleStart":, case "ModuleEnd": fallthroughs next to the new names in both converters' Read methods (writers should continue to emit only the new names, matching the existing Status/ModuleStatus precedent). This keeps the rename purely additive on the read side, matches the project's own established compatibility contract for this type, and avoids a silent breaking change for anyone with in-flight or persisted v3 payloads.
|
Addressed the legacy JSON compatibility review at Both Validation:
|
GitHub Actions
Failure
There was a problem hiding this comment.
Code review
Reviewed the metadata rename (ModuleName→Name, ModuleTypeName→TypeName, ModuleDuration→Duration, ModuleStart→StartTime, ModuleEnd→EndTime, and the lowercase Success/Deconstruct parameter) across IModuleResult, ModuleResult, the JSON converters, and all runtime/distributed/test consumers.
What I checked:
- Grepped
src/andtest/for any remaining.ModuleName/.ModuleDuration/.ModuleStart/.ModuleEndaccesses onIModuleResult/ModuleResult— none found. The other hits (IModuleContext.ModuleName,PlannedModule.ModuleName,ModuleAssignment.ModuleTypeName, Ansible'sModuleName) are distinct, unrelated types and correctly untouched. - Grepped for the old capitalized
Success(Value: ...)/Deconstruct(Value: out ...)named-argument usage — none remain, so the parameter-name rename is complete. - Confirmed
docs/docs/**references toModuleName/ModuleTypeName(hooks, command-line planning, distributed Redis schema, Ansible) refer to unrelated types, notIModuleResult, so no docs are stale. - The JSON converters (
ModuleResultNonGenericJsonConverter,ModuleResultJsonConverter<T>) read both the new and legacy property names on deserialize, but only ever write the new names — this is a deliberate, documented one-way breaking change (RELEASE_NOTES_V4.md), and the newGeneric_Result_Reads_Legacy_Metadata_Keys/NonGeneric_Result_Reads_Legacy_Metadata_Keystests plus the head commit (fix(json): read legacy result metadata) show this was added specifically to address the wire-compat concern CodeRabbit raised earlier in the thread. Worth calling out for anyone doing a live/rolling distributed upgrade (old workers can't parse new-format JSON), but that's already spelled out in the release notes, so I'm not blocking on it. ModuleResultContractTests.MetadataSurface_UsesConciseNameslocks the public surface via reflection, which is a good guardrail against future drift back to the verbose names.
Minor, non-blocking observation:
- The
await usingrefactor intest/ModularPipelines.UnitTests/Caching/ModuleCacheTests.cs(collapsing a block-scopedawait using (...) { }into a using-declaration) is unrelated to the metadata rename. It's a harmless, behavior-preserving cleanup, just flagging it as out-of-scope churn in an otherwise tightly-scoped PR.
No correctness issues or CLAUDE.md violations found. This is a clean, mechanical rename with good regression coverage.
|
Current red jobs are downstream of the stale core public-API baselines introduced by #4251 after #4267. PR #4262 now carries the shared baseline repair and is awaiting CI/review; duplicating it here before #4262 merges would incorrectly remove APIs that still exist on this branch. This PR should be rebased after #4262 lands, then only its ModuleResult rename delta needs baselining. |
2913c7b to
7658b0d
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
GitHub Actions
Failure
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD 7658b0d6. The prior review history on this PR shows the commit SHAs changed (e2f1a814 → ea9f1060 → 2913c7b9 → 7658b0d6), but that's a rebase onto the latest main (past #4270 "Tidy exception hierarchy and contracts" and #4251 "Baseline public APIs"), not new content — I diffed the current PR against the last reviewed state and confirmed the changed file set (26 files) and every hunk are identical in substance to what was already reviewed and cleared.
Verified still in place from the prior CLEAR review:
- The metadata rename (
ModuleName→Name,ModuleTypeName→TypeName,ModuleDuration→Duration,ModuleStart→StartTime,ModuleEnd→EndTime, plus lowercaseSuccess/Deconstructparameter) is complete and consistent acrosssrc/andtest/— no leftover old-name references onIModuleResult/ModuleResult. ModuleResultNonGenericJsonConverter.ReadandModuleResultJsonConverter<T>.Readinsrc/ModularPipelines/Models/ModuleResult.cs(lines 690-712, 864-886) read both new and legacy property names (ModuleName,ModuleTypeName,ModuleDuration,ModuleStart,ModuleEnd), matching the existingStatus/ModuleStatuscompat pattern. Writers emit only the new names — a deliberate, documented one-way wire change.RELEASE_NOTES_V4.mdhas the "Module result metadata" section documenting all five renames.ModuleResultContractTests.MetadataSurface_UsesConciseNameslocks the public surface via reflection, and the newGeneric_Result_Reads_Legacy_Metadata_Keys/NonGeneric_Result_Reads_Legacy_Metadata_Keystests exercise the legacy-key read path end-to-end (verified theRenameMetadataPropertieshelper's string replacements target only the intended JSON keys, with no accidental substring collisions like"Name":matching inside"TypeName":).
No correctness issues or CLAUDE.md violations found. This is a clean, mechanical rename with good regression coverage; nothing new to flag since the last review.
Align ModuleResult metadata with CommandResult and remove redundant Module prefixes before the v4 release.
Accept the five pre-v4 module result metadata keys during deserialization while continuing to emit the concise v4 names.
7658b0d to
ebb47b6
Compare
GitHub Actions
Failure
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD ebb47b64 (another rebase — commit SHAs shifted again, but I diffed this PR's 28 changed files against the last reviewed/cleared state and the substance is identical).
Verified still in place, independently re-checked (not just trusted from history):
- The metadata rename (
ModuleName→Name,ModuleTypeName→TypeName,ModuleDuration→Duration,ModuleStart→StartTime,ModuleEnd→EndTime, plus lowercaseSuccess/Deconstructvalueparameter) is complete and consistent acrosssrc/andtest/— no stale references to the old names onIModuleResult/ModuleResult. ModuleResultNonGenericJsonConverter.ReadandModuleResultJsonConverter<T>.Readinsrc/ModularPipelines/Models/ModuleResult.csaccept both the new and legacy JSON property names (case "Name": case "ModuleName":, etc.) for all five renamed fields, matching the establishedStatus/ModuleStatuscompat pattern. Writers emit only the new names — a deliberate, documented one-way wire change.RELEASE_NOTES_V4.mdhas the "Module result metadata" section documenting all five renames and the JSON migration note.ModuleResultContractTests.MetadataSurface_UsesConciseNameslocks the public surface via reflection, andGeneric_Result_Reads_Legacy_Metadata_Keys/NonGeneric_Result_Reads_Legacy_Metadata_Keysexercise the legacy-key read path end-to-end. I checked theRenameMetadataPropertiestest helper's ordered string replacements by hand —"Name":does not collide with"TypeName":(no quote immediately precedesNameinsideTypeName), so the test fixture actually produces the intended legacy JSON rather than accidentally double-renaming.
No correctness issues or CLAUDE.md violations found. This remains a clean, mechanical, well-tested rename; nothing new to flag on this rebase.
|
@claude review |
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at HEAD d712cc2b. This is another rebase point on top of the previously cleared ebb47b64 state; the only new commit is d712cc2b ("fix(api): baseline renamed result members"), which updates PublicAPI.Shipped.txt/PublicAPI.Unshipped.txt to match the metadata rename.
Verified the new commit specifically:
- It moves the five renamed
IModuleResult/ModuleResultmembers (Name,TypeName,Duration,StartTime,EndTime) plus the lowercase-parameterSuccess(T value)/Deconstruct(out T value)fromShipped.txtintoUnshipped.txt, and removes the old PascalCase-parameter/old-name entries — exactly the delta this rename introduces, and exactly what a prior review comment on this PR asked for (rebase after #4262, then baseline only theModuleResultrename delta). - Cross-checked
src/ModularPipelines/Models/ModuleResult.cs: theSuccessrecord's constructor andDeconstructboth use the lowercasevalueparameter, matching the new baseline entries. - No duplicate or cross-file (Shipped vs Unshipped) entries introduced.
- Built
ModularPipelines.slnx(Release) at this HEAD: succeeded, 0 errors, only the 12 pre-existingRS0026warnings (unrelated optional-parameter-overload analyzer, present before this PR). NoRS0016/RS0017public-API-mismatch diagnostics, confirming the new baseline entries now match the compiled API surface.
Everything previously verified in this rename (JSON legacy-key compat readers, release notes, contract tests) is untouched by this commit. No correctness issues or CLAUDE.md violations found.
1 participant
Summary
ModuleResult/IModuleResultmetadata toName,TypeName,Status,Duration,StartTime, andEndTimeSuccessconstructor andDeconstructparameter namesValidation
ModularPipelines.Tests.slnfRelease build (178 existing warnings, 0 errors)ModuleResultContractTests: 23 passedModuleResultSerializerTests: 3 passedCloses #4221
Summary by CodeRabbit
Name,TypeName,Duration,StartTime, andEndTime.