Align context service resolution - #4272
Conversation
|
@claude review |
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. |
|
Warning Review limit reachedNext included review available in 27 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 (7)
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 (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe service-resolution API now distinguishes required and optional lookups. Duplicate context extensions were removed. Production consumers, generated accessors, tests, public API records, and documentation now use the new names. ChangesService Resolution API
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to This change renames the public service-resolution methods and makes required lookups fail immediately when a service is not registered. The PR is mergeable with explicit owner awareness that downstream callers must migrate and incomplete registrations may cause runtime failures. Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR satisfies the coding objectives in [ Full details: Out of Scope Changes checkExplanation Most changes support [ Full details: Docstring CoverageExplanation Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 25 files. (2 skipped: 2 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 |
GitHub Actions
Failure
Greptile SummaryThe PR aligns pipeline service resolution with standard .NET naming while preserving required-versus-optional behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/ModularPipelines/Context/Domains/IServicesContext.cs | Renames the required and optional service-resolution contracts with corresponding nullability. |
| src/ModularPipelines/Context/Domains/Implementations/ServicesContext.cs | Implements the renamed contracts while preserving existing throw and null-return semantics. |
| src/ModularPipelines/Context/ContextExtensions.cs | Removes duplicate pipeline-context service-resolution extensions in favor of context.Services. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/DependencyRegistrationGenerator.cs | Updates generated integration accessors to use the renamed required-service API. |
| src/ModularPipelines/PublicAPI.Unshipped.txt | Records the removed and newly introduced public API members for compatibility validation. |
Reviews (8): Last reviewed commit: "fix(api): record removed service members" | Re-trigger Greptile
![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 full diff (81e024b) against main. This is a clean, mechanical API rename that aligns IServicesContext with standard .NET DI naming/nullability conventions:
IServicesContext.Get<T>()→GetRequiredService<T>()(throws when missing)IServicesContext.TryGet<T>()→GetService<T>()(nullable, returnsnullwhen missing)- Duplicate
IPipelineContext.GetService<T>()/TryGetService<T>()extensions removed in favor of going throughcontext.Services
Verification performed:
- Checked out the PR branch in an isolated worktree and grepped the full tree for any remaining call sites of the old
Services.Get</Services.TryGet<API and the removedIPipelineContext.GetService</TryGetService<extensions. Every call site was migrated — the handful of remainingGetService<T>()hits (OptionsValidator.cs,PipelineBuilder.cs,CliDocumentationScraperBase.cs,PluginIntegrationTests.cs) resolve to unrelatedIServiceProvider/AngleSharp APIs, not the removed extension, so nothing was missed. - Confirmed
ToolsContext.Get<T>()(a distinct, internalIToolsContextAPI with its own tool-registration exception) was correctly left untouched — it's out of scope for this rename and not part of theIServicesContextsurface. PublicAPI.Shipped.txt/PublicAPI.Unshipped.txtare updated consistently with the interface changes, the source generator (DependencyRegistrationGenerator) and its hardening test were updated to emitGetRequiredService<T>(), andRELEASE_NOTES_V4.mddocuments the breaking change with a before/after example.- Test updates (
ContextExtensionsTests,BranchConditionLoggingTests,TelemetryIntegrationTests) consistently exercise the new method names and preserve the original "throws when missing / returns null when missing" semantics.
Architectural note (non-blocking): the old members are removed outright rather than routed through an [Obsolete] shim for a deprecation window. That's a reasonable call here since this lands alongside other breaking changes documented in RELEASE_NOTES_V4.md for what looks like a V4 major-version bump — bundling breaking renames into one release is preferable to a lingering obsolete surface, so no change requested.
No correctness bugs, CLAUDE.md violations, or missed call sites found.
|
@claude review |
81e024b to
6658410
Compare
GitHub Actions
Failure
|
@claude review |
GitHub Actions
Inactive
6658410 to
e63275b
Compare
There was a problem hiding this comment.
Code review
Re-reviewed at the current head (e63275b), which is a rebase of the previously-reviewed commit (81e024b, cleared 2026-08-28) onto the latest main (picking up #4280's logging-options unification). Diffed the two commits' content directly and confirmed the substance is unchanged:
IServicesContext.Get<T>()→GetRequiredService<T>(),TryGet<T>()→GetService<T>(), implemented consistently inServicesContextand consumed correctly inToolsContext.Get<T>().- Duplicate
IPipelineContext.GetService<T>()/TryGetService<T>()extensions removed fromContextExtensions.csin favor ofcontext.Services. - All ~60 generated/manual tool-extension files updated to the new call sites,
PublicAPI.Shipped.txt/PublicAPI.Unshipped.txtupdated consistently, andRELEASE_NOTES_V4.mddocuments the breaking change with a before/after example. - Tests (
ContextExtensionsTests,BranchConditionLoggingTests,TelemetryIntegrationTests) updated to the new names and preserve the original throw/null semantics.
Verification performed this pass: checked out the PR head commit's tree and grepped the full working tree for any remaining Services.Get<, .TryGet<, or .TryGetService< call sites — none found, confirming the rebase didn't drop or reintroduce any stale references.
Removing the old members outright rather than an [Obsolete] shim remains the right call, since this bundles into the documented V4 breaking-change set rather than adding a lingering deprecated surface.
No correctness bugs, CLAUDE.md violations, or missed call sites found.
GitHub Actions
Inactive
e63275b to
c19cfab
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at the current head (c19cfab), which is a rebase of the previously-reviewed commit (e63275b, cleared 2026-08-29) onto the latest main (picking up #4281's PowerShell casing normalization and other recent merges). The PR branch is now a single squashed commit on top of main.
Verification performed this pass:
- Diffed the rebased tree against
main(76 files changed, +103/-149) — the substance is unchanged from the prior two reviews:IServicesContext.Get<T>()→GetRequiredService<T>(),TryGet<T>()→GetService<T>(), duplicateIPipelineContext.GetService<T>()/TryGetService<T>()extensions removed fromContextExtensions.cs. - Checked out the PR head tree and grepped for any remaining
Services.Get<,Services.TryGet<, or.TryGetService<call sites — none found. - Confirmed
ToolsContext.Get<T>()(a distinct, internalIToolsContextAPI) remains untouched, as it's out of scope for this rename. ServicesContext.GetRequiredService<T>()/GetService<T>()implementations match what was verified in the prior reviews — throw-with-diagnostic-message vs. nullable-return semantics are preserved.PublicAPI.Shipped.txt/PublicAPI.Unshipped.txt, the source generator output,RELEASE_NOTES_V4.md, and the ~60 tool-extension call sites are all consistent with the previously validated change.
No new commits were added beyond the rebase — this confirms the rebase didn't drop or reintroduce any stale references.
No correctness bugs, CLAUDE.md violations, or missed call sites found.
c19cfab to
b7b88db
Compare
|
Rebased onto current main after #4274. The Git test conflict now uses this PR's GetRequiredService() with main's public ILogger context surface. Validation: core build 0 warnings/errors; ContextExtensionsTests 19/19; BranchConditionLoggingTests 3/3; diff check clean. @claude please review current head b7b88db. |
There was a problem hiding this comment.
Review
This PR is a mechanical rename of the service-resolution API: IServicesContext.Get<T>() → GetRequiredService<T>(), TryGet<T>() → GetService<T>(), and removal of the IPipelineContext.GetService<T>()/TryGetService<T>() shortcut extensions. I verified all ~70 call-site edits across the tool packages, the core framework (src/ModularPipelines/Context/ContextExtensions.cs, Context/Domains/IServicesContext.cs, Context/Domains/Implementations/ServicesContext.cs, Context/ToolsContext.cs), the source generator (tools/ModularPipelines.OptionsGenerator/.../DependencyRegistrationGenerator.cs), and tests — all consistent, and a repo-wide grep of the PR branch found no leftover callers of the old APIs.
Blocking: PublicAPI.Shipped.txt / Unshipped.txt updated incorrectly
src/ModularPipelines/PublicAPI.Shipped.txt (around line 433) has the four removed members (IServicesContext.Get<T>(), TryGet<T>(), ContextExtensions.GetService<T>(), TryGetService<T>()) deleted outright, rather than following the Microsoft.CodeAnalysis.PublicApiAnalyzers convention of recording them as *REMOVED* entries in PublicAPI.Unshipped.txt.
Directory.Build.props enables RS0016/RS0017 as WarningsAsErrors whenever $(GITHUB_ACTIONS)=='true', so on CI, RS0017 ("Remove deleted types and members from the declared API") will fire as a build error because these four deleted signatures were never recorded with the *REMOVED* marker — only the new GetRequiredService<T>/GetService<T> entries were added to Unshipped.txt. This is very likely to break the PR's own CI build.
Suggested fix: add the removed signatures back to PublicAPI.Unshipped.txt prefixed with *REMOVED*, e.g.:
*REMOVED*ModularPipelines.Context.Domains.IServicesContext.Get<T>() -> T
*REMOVED*ModularPipelines.Context.Domains.IServicesContext.TryGet<T>() -> T
*REMOVED*ModularPipelines.Context.ContextExtensions.GetService<T>(ModularPipelines.Context.IPipelineContext! context) -> T
*REMOVED*ModularPipelines.Context.ContextExtensions.TryGetService<T>(ModularPipelines.Context.IPipelineContext! context) -> T
(adjust exact signatures to match what was in Shipped.txt) rather than deleting them from Shipped.txt directly.
GitHub Actions
Inactive
|
Rebased onto current main after #4278. Conflict resolution combines ICmdContext with GetRequiredService semantics and preserves both release-note sections. Validation: core test build 0 errors; ContextExtensionsTests 18/18; Git solution build 0 warnings/errors; BranchConditionLoggingTests 3/3. Current head: 960ee9d. @claude review |
b7b88db to
960ee9d
Compare
There was a problem hiding this comment.
Review
This PR is a mechanical rename of the service-resolution API: IServicesContext.Get<T>() → GetRequiredService<T>(), TryGet<T>() → GetService<T>(), and removal of the IPipelineContext.GetService<T>()/TryGetService<T>() shortcut extensions. I re-verified the current head (960ee9d8) against the substance already confirmed correct in prior reviews (throw-vs-nullable semantics preserved in ServicesContext/ToolsContext, all ~60 tool-extension call sites and tests updated consistently, no stray old-API references anywhere in the repo, full core-solution build succeeds).
Blocking: PublicAPI.Shipped.txt / Unshipped.txt still not following the *REMOVED* convention
This was flagged as blocking in the prior review at b7b88dbc and has not been fixed — the current head still has the same problem.
src/ModularPipelines/PublicAPI.Shipped.txt deletes the four removed members outright:
-ModularPipelines.Context.Domains.IServicesContext.Get<T>() -> T!
-ModularPipelines.Context.Domains.IServicesContext.TryGet<T>() -> T?
-static ModularPipelines.Context.ContextExtensions.GetService<T>(this ModularPipelines.Context.IPipelineContext! context) -> T!
-static ModularPipelines.Context.ContextExtensions.TryGetService<T>(this ModularPipelines.Context.IPipelineContext! context) -> T?
PublicAPI.Unshipped.txt only adds the two new members (GetRequiredService<T>, GetService<T>) — it never records the four deleted signatures as *REMOVED* entries.
Directory.Build.props:57 sets WarningsAsErrors to include RS0016;RS0017 whenever $(GITHUB_ACTIONS)=='true' (and this project's PublicApiAnalyzers package reference is gated the same way at line 44). RS0017 ("Remove deleted types and members from the declared API") will fire because these four members are gone from the API surface but were never declared as removed via the *REMOVED* prefix — this is very likely to fail the PR's own CI build.
Suggested fix: put the removed signatures back in PublicAPI.Shipped.txt (or leave them where they were) and instead add to PublicAPI.Unshipped.txt:
*REMOVED*ModularPipelines.Context.Domains.IServicesContext.Get<T>() -> T
*REMOVED*ModularPipelines.Context.Domains.IServicesContext.TryGet<T>() -> T
*REMOVED*ModularPipelines.Context.ContextExtensions.GetService<T>(ModularPipelines.Context.IPipelineContext! context) -> T
*REMOVED*ModularPipelines.Context.ContextExtensions.TryGetService<T>(ModularPipelines.Context.IPipelineContext! context) -> T
(adjust exact signatures/nullability to match what's actually in Shipped.txt today — that's the analyzer's documented convention for marking a shipped API as removed, rather than deleting the Shipped.txt line directly.)
GitHub Actions
Inactive
960ee9d to
8c0159e
Compare
|
Addressed the current blocking review at 8c0159e. All four deleted shipped signatures are now recorded as exact |
There was a problem hiding this comment.
Re-review at 8c0159e
Re-reviewed the current head (8c0159e0), which adds two commits on top of the previously reviewed 960ee9d8 (BLOCKING):
23f1b0d2"fix(api): record removed service members"8c0159e0"fix(tests): use renamed file path type"
Verification performed:
- Confirmed the previously blocking
PublicAPI.Shipped.txt/Unshipped.txtissue is now fixed correctly:PublicAPI.Unshipped.txtnow has*REMOVED*entries for all four removed members (IServicesContext.Get<T>(),IServicesContext.TryGet<T>(),ContextExtensions.GetService<T>(),ContextExtensions.TryGetService<T>()), matching theMicrosoft.CodeAnalysis.PublicApiAnalyzersconvention. - Built
ModularPipelines.slnx(Release) against the PR branch, both normally and withGITHUB_ACTIONS=trueset (which gatesWarningsAsErrorsfor RS0016/RS0017 perDirectory.Build.props) — build succeeds with 0 errors in both cases and no PublicAPI analyzer violations. This confirms the fix actually resolves the CI-breaking issue, not just superficially. - The remaining commit (
8c0159e0) is an unrelated, correct fixup:CmdTests.GetCmdTestFilereturn type updated fromModularPipelines.FileSystem.FiletoFilePath, following theFile→FilePathrename that landed in#4260and was merged intomainsince this PR's earlier reviews. - Re-confirmed (as in prior passes) that the core rename itself —
IServicesContext.Get<T>()→GetRequiredService<T>(),TryGet<T>()→GetService<T>(), removal of the duplicateIPipelineContext.GetService<T>()/TryGetService<T>()extensions — is fully and consistently propagated across all ~60 tool-extension call sites, the source generator template, and tests, with no stale references left anywhere in the repo.
No correctness bugs, CLAUDE.md violations, or missed call sites found. The previously blocking issue has been properly addressed.
GitHub Actions
Inactive
8c0159e to
c91e222
Compare
|
Rebased onto main after #4273 merged; resolved RELEASE_NOTES_V4.md additively, retaining both requirements and service-resolution guidance. Validation: ModularPipelines.Tests.slnf Release build passed with 0 errors; ContextExtensionsTests passed 18/18; no conflict markers or diff-check errors. @claude please review the new head. |
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Re-reviewed at the current head (c91e222), which rebases the previously-cleared 8c0159e0 onto main after #4273 merged. Diffed the rebased tree against main (76 files, +106/-149) and confirmed the substance is unchanged from the five prior review passes on this PR:
IServicesContext.Get<T>()→GetRequiredService<T>()(throws when missing),TryGet<T>()→GetService<T>()(nullable), implemented consistently inServicesContextand consumed correctly byToolsContext.Get<T>().- Duplicate
IPipelineContext.GetService<T>()/TryGetService<T>()extensions removed fromContextExtensions.csin favor ofcontext.Services. - All ~70 tool-extension call sites updated consistently; grepped the rebased tree for any remaining
Services.Get</Services.TryGet</.TryGetService<references — none found.
Previously-blocking issue confirmed still fixed: the PublicAPI.Shipped.txt/Unshipped.txt handling (flagged blocking at b7b88dbc and 960ee9d8, fixed at 8c0159e0) remains correct at this head — the four removed members are recorded as *REMOVED* entries in PublicAPI.Unshipped.txt rather than deleted outright from Shipped.txt, matching the PublicApiAnalyzers convention that satisfies RS0017 under CI's WarningsAsErrors.
Test changes since the last review are a genuine improvement, not just mechanical renames: ContextExtensionsTests now builds a real ServiceCollection/ServicesContext and asserts against it directly, rather than mocking IModuleContext/IServicesContext and mocking the now-removed IPipelineContext.GetService<T>() extension. Since the extension being tested no longer exists, exercising the real DI container end-to-end is a better fit than a mock would have been here — it validates actual IServiceProvider resolution behavior instead of a hand-configured mock returning a canned value.
RELEASE_NOTES_V4.md documents the breaking change with a clear before/after example, consistent with the rest of the V4 migration guide.
No correctness bugs, CLAUDE.md violations, or missed call sites found. No architectural concerns beyond what was already discussed and resolved in prior rounds (removing the old members outright rather than an [Obsolete] shim remains the right call, since this bundles into the documented V4 breaking-change set).
1 participant
Closes #4229.
Summary
IServicesContext.Get<T>()toGetRequiredService<T>()TryGet<T>()to nullableGetService<T>()IPipelineContextservice extensionsValidation
Summary by CodeRabbit
Breaking Changes
Bug Fixes
null, improving error visibility.Documentation