Align public logging contracts - #4274
Conversation
|
@claude review |
|
Warning Review limit reachedNext included review available in 1 minute. 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 (55)
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 |
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
Greptile SummaryThe PR aligns the v4 public logging surface around
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/ModularPipelines/Logging/ModuleLoggerAccessor.cs | Replaces the public provider pattern with a scoped accessor that resolves the framework-managed logger through the existing module context. |
| src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs | Registers the new logger accessor and maps all summary interfaces to one shared SummaryLogger singleton. |
| src/ModularPipelines/Logging/ISummaryLogger.cs | Narrows the public summary writer contract to mutation operations. |
| src/ModularPipelines/Logging/ISummaryLogReader.cs | Introduces a dedicated public contract for reading summary entries and rendered output. |
| src/ModularPipelines.Analyzers/ModularPipelines.Analyzers.CodeFixes/LoggerInConstructorCodeFixProvider.cs | Updates logger-replacement compatibility checks to target the standard ILogger contract while preserving symbol binding. |
| src/ModularPipelines/Context/IPipelineContext.cs | Changes the public context logger type from the framework-specific disposable interface to standard ILogger. |
| src/ModularPipelines.GitHub/GitHubMarkdownSummaryGenerator.cs | Migrates summary output consumption to the new read-only interface. |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
class IPipelineContext {
+ILogger Logger
+ISummaryLogger Summary
}
class IModuleLoggerAccessor {
+ILogger Logger
}
class ISummaryLogger {
+Information()
+Success()
+Warning()
+Error()
+Log()
}
class ISummaryLogReader {
+GetEntries()
+GetOutput()
}
class IInternalSummaryLogger {
+WriteLogs()
}
class SummaryLogger
IInternalSummaryLogger --|> ISummaryLogger
IInternalSummaryLogger --|> ISummaryLogReader
SummaryLogger ..|> IInternalSummaryLogger
IPipelineContext --> ISummaryLogger
IPipelineContext --> IModuleLoggerAccessor
Reviews (4): Last reviewed commit: "test(http): use public logger accessor" | 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 (55 files) for PR #4274, which aligns the public logging surface with Microsoft.Extensions.Logging: context.Logger now returns ILogger instead of the custom IModuleLogger, IModuleLoggerProvider is replaced by IModuleLoggerAccessor, IModuleLogger itself becomes internal, and ISummaryLogger is split into a write-only ISummaryLogger plus a new ISummaryLogReader.
What's good
- CQS split on summary logging (
ISummaryLoggervsISummaryLogReader) is a solid interface-segregation improvement — consumers that only need to emit summary entries (most modules) no longer seeGetEntries/GetOutput, and the one reader (GitHubMarkdownSummaryGenerator) depends only on the read contract. Both are still backed by the sameSummaryLoggersingleton in DI, so there's no behavioral change, just a narrower public surface. - Making
IModuleLoggerinternal and exposing plainILoggeris the right call: it removes the ability for consumers to accidentally callDispose()on a framework-owned, shared logger, and it lets module code interop naturally with anything written against the standardILoggerabstraction (e.g.NullLogger, existing extension methods) instead of a bespoke type.IModuleLoggerAccessor.Loggerpreserves the same ambient-resolution/caching behavior (ModuleLoggerAccessor.GetLogger()) under the hood, so this is a pure type-surface narrowing, not a behavior change. - I traced every consumer of the renamed/removed types (
IModuleLoggerProvider,ISummaryLogger.GetEntries/GetOutput/Info, publicIModuleLogger) acrosssrc/andtest/, including assemblies outside the core project (ModularPipelines.Testing,ModularPipelines.Git,ModularPipelines.GitHub, analyzers) — all call sites were updated consistently, and thePublicAPI.Shipped.txt/PublicAPI.Unshipped.txtbaselines correctly reflect the removed/added public members.InternalsVisibleToalready covers the assemblies (likeModularPipelines.Testing) that still reference the now-internalIModuleLogger. - The analyzer's
LoggerInConstructorCodeFixProviderwas correctly updated to match againstMicrosoft.Extensions.Logging.ILoggerinstead of the oldIModuleLogger, consistent withcontext.Logger's new type — the speculative-binding safeguard logic is unaffected. - Docs (
logging.md,secrets.md) andRELEASE_NOTES_V4.mdare updated to match the new API surface.
Minor, non-blocking nit
- In
ModularPipelinesAnalyzersILoggerUnitTests.cs,BadModuleSourceILoggerProviderrenames its local variable fromloggerProvidertologgerAccessor, but the constructor parameter type is stillMicrosoft.Extensions.Logging.ILoggerProvider(unrelated to this PR's newIModuleLoggerAccessor). This looks like a global find/replace artifact — the variable name now implies it's testing the new accessor type when it's actually still testing the framework's ownILoggerProviderinjection case. Worth a rename back to something likeframeworkLoggerProviderfor clarity, but it doesn't affect test correctness.
No functional or compile-breaking issues found. Nice, well-scoped refactor with matching test/doc/public-API updates.
|
@claude review |
28e598e to
664c591
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Reviewed the full diff for PR #4274 (55 files) at head 664c5911d08a03f12e072eabc577978dc6515fb1, which aligns the public logging surface with Microsoft.Extensions.Logging: context.Logger now returns ILogger instead of the custom IModuleLogger, IModuleLoggerProvider is replaced by IModuleLoggerAccessor, IModuleLogger itself becomes internal, and ISummaryLogger is split into a write-only ISummaryLogger plus a new ISummaryLogReader (with Info renamed to Information).
Note: the previously-reviewed commit (28e598e1...) is no longer reachable — the branch was rebased per the PR description ("rebase after #4262 merges") — so this review re-checks the diff against main from scratch rather than diffing against the prior review.
What's good
- CQS split on summary logging (
ISummaryLoggervsISummaryLogReader) narrows the surface each consumer sees: modules writing summaries no longer seeGetEntries/GetOutput, and the one reader (GitHubMarkdownSummaryGenerator) depends only onISummaryLogReader. Both are still backed by the sameSummaryLoggersingleton in DI (DependencyInjectionSetup.cs), so this is a pure interface-segregation improvement with no behavioral change. IModuleLoggerAccessor/IInternalModuleLoggerAccessorsplit cleanly separates the public, non-disposableILoggerview from the internalIModuleLogger(disposable, framework-owned lifecycle) used for stack-trace-based module detection and scoped caching.ModuleLoggerAccessor.Logger => GetLogger()preserves the exact same ambient-resolution/locking/caching behavior that existed under the oldModuleLoggerProvider.GetLogger()— this is a rename plus a narrower public accessor, not a behavior change.- I traced every consumer of the renamed/removed types (
IModuleLoggerProvider,ISummaryLogger.Info/GetEntries/GetOutput, publicIModuleLogger) acrosssrc/andtest/, includingModularPipelines.Git,ModularPipelines.GitHub, the analyzers, and the distributed master/worker executors — all call sites were updated consistently, andPublicAPI.Shipped.txt/PublicAPI.Unshipped.txtcorrectly move the removed members out of Shipped and the new/renamed members into Unshipped. - The analyzer's
LoggerInConstructorCodeFixProvidernow matches againstMicrosoft.Extensions.Logging.ILoggerinstead of the oldIModuleLogger, consistent withcontext.Logger's new type. - Docs (
logging.md,secrets.md) andRELEASE_NOTES_V4.mdare updated to match the new API surface, and the breaking renameISummaryLogger.Info→Informationis called out in the release notes.
Minor, non-blocking nit (carried over from the prior review, still present)
- In
src/ModularPipelines.Analyzers/ModularPipelines.Analyzers.Test/ModularPipelinesAnalyzersILoggerUnitTests.cs:112,BadModuleSourceILoggerProviderstill builds its test source with"ILoggerProvider loggerAccessor"— the constructor parameter type is the framework's ownMicrosoft.Extensions.Logging.ILoggerProvider(unrelated to this PR's newIModuleLoggerAccessor), but the variable was renamed fromloggerProvidertologgerAccessoras part of the refactor's find/replace. The name now misleadingly implies it's testing the new accessor type when it's actually still exercising the "inject the framework's ownILoggerProvider" bad-constructor case. Purely cosmetic (doesn't affect test correctness) — a rename to something likeframeworkLoggerProviderwould remove the ambiguity, but this doesn't block the PR.
No functional or compile-breaking issues found in the diff. Well-scoped, mechanical refactor with consistent updates across DI, integrations, analyzers, docs, and the public API baseline.
664c591 to
3861690
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
Code review
Reviewed the full diff for PR #4274 (55 files) at head 3861690782823bae7b7e5fcbac9bf99c9bb2e495. This aligns the public logging surface with Microsoft.Extensions.Logging: context.Logger now returns ILogger instead of the custom IModuleLogger, IModuleLoggerProvider is replaced by IModuleLoggerAccessor (with an internal IInternalModuleLoggerAccessor still exposing GetLogger()/GetLogger(Type) for framework-internal callers), IModuleLogger itself becomes internal, and ISummaryLogger is split into a write-only ISummaryLogger plus a new ISummaryLogReader (with Info renamed to Information).
The branch has been rebased twice since the first review pass on this PR (previously-reviewed heads 28e598e1 and 664c5911 are no longer reachable), so this is effectively a fresh, from-scratch review of the diff against main, plus targeted verification of the one commit that's new since the last review (3861690 – "test(http): use public logger accessor").
What's good
- CQS split on summary logging (
ISummaryLoggervsISummaryLogReader) is a solid interface-segregation improvement: modules that only write summary entries no longer seeGetEntries/GetOutput, and the one reader (GitHubMarkdownSummaryGenerator) depends only on the read contract. Both are still backed by the sameSummaryLoggersingleton inDependencyInjectionSetup.cs, so it's a pure surface narrowing with no behavioral change. IModuleLoggerAccessor/IInternalModuleLoggerAccessorsplit cleanly separates the public, non-disposableILoggerview from the internalIModuleLogger(disposable, framework-owned, used for stack-trace-based module detection and per-module caching).ModuleLoggerAccessor.Logger => GetLogger()preserves the exact ambient-resolution/locking/caching behavior that existed under the oldModuleLoggerProvider.GetLogger().- I re-traced every consumer of the renamed/removed types (
IModuleLoggerProvider,ISummaryLogger.Info/GetEntries/GetOutput, publicIModuleLogger) acrosssrc/andtest/—ModularPipelines.Git,ModularPipelines.GitHub, the analyzers, and the distributed master/worker executors are all updated consistently, andPublicAPI.Shipped.txt/PublicAPI.Unshipped.txtcorrectly move the removed members out of Shipped and the new/renamed members into Unshipped. - New commit check (
test/ModularPipelines.UnitTests/Context/HttpTests.cs): rewires mocks fromIModuleLoggerProvider/IModuleLoggertoIModuleLoggerAccessor/ILogger. I checked whether the tests that now callMock.Of<IModuleLoggerAccessor>()without stubbing.Logger(which yields anulllogger under Moq's loose/DefaultValue.Emptybehavior) could cause aNullReferenceExceptionwhenHttp.cs'sLogStatusCode/LogDurationinvokelogger— they don't, because in every such test_httpLoggeris itself a looseMock.Of<IHttpLogger>()(or the non-loggingLegacyBodyLoggertest double), so the nullloggeris only ever passed as an unused argument to a mocked/no-op method, never dereferenced. Tests that route through a realHttpLoggerinstance correctly stubIModuleLoggerAccessor.Loggerto return a realILoggermock (e.g. lines ~456-483, ~528-545, ~639-656, ~686-708, ~734-751, ~773-791). - The analyzer's
LoggerInConstructorCodeFixProvidernow matches againstMicrosoft.Extensions.Logging.ILoggerinstead of the oldIModuleLogger, consistent withcontext.Logger's new type. - Docs (
logging.md,secrets.md) andRELEASE_NOTES_V4.mdare updated to match the new API surface, and the breaking renameISummaryLogger.Info→Informationis called out in the release notes.
Minor, non-blocking nit (carried over from prior reviews on this PR, still unaddressed)
src/ModularPipelines.Analyzers/ModularPipelines.Analyzers.Test/ModularPipelinesAnalyzersILoggerUnitTests.cs:29still buildsBadModuleSourceILoggerProviderfrom"ILoggerProvider loggerAccessor". The constructor parameter type is the framework's ownMicrosoft.Extensions.Logging.ILoggerProvider(unrelated to this PR's newIModuleLoggerAccessor), but the variable was renamed fromloggerProvidertologgerAccessor— presumably a find/replace artifact — so the name now misleadingly implies it's testing the new accessor type when it's actually still exercising the "inject the framework's ownILoggerProvider" bad-constructor case. Doesn't affect test correctness; a rename to something likeframeworkLoggerProviderwould remove the ambiguity.
No functional or compile-breaking issues found. This remains a well-scoped, mechanical refactor with consistent updates across DI, integrations, analyzers, docs, tests, and the public API baseline.
Expose standard ILogger, replace the logger provider with an accessor, and separate summary reads from writes.
Keep the logging-precedence regression aligned with the ILogger-based accessor after rebasing the two logging API changes.
|
Rebased onto current main after #4268, combining the logging-contract and module-result PublicAPI additions. Normal core and CI-conditioned API builds pass; 62 focused HTTP/logging tests pass; all 61 PublicAPI baseline pairs and assertion harness pass. |
3861690 to
c1c2a5c
Compare
There was a problem hiding this comment.
Code review
Reviewed the full diff for PR #4274 (55 files, +308/-283) at head c1c2a5c234c849c32a5e3a271a5421f54cdf4c4b. This aligns the public logging surface with Microsoft.Extensions.Logging: context.Logger now returns ILogger instead of the custom IModuleLogger, IModuleLoggerProvider is replaced by IModuleLoggerAccessor (with an internal IInternalModuleLoggerAccessor still exposing GetLogger()/GetLogger(Type) for framework-internal callers), IModuleLogger itself becomes internal, and ISummaryLogger is split into a write-only ISummaryLogger plus a new ISummaryLogReader (with Info renamed to Information).
The branch has been rebased again since the last review pass (3861690782823bae7b7e5fcbac9bf99c9bb2e495 is no longer reachable), but the diff content against main is identical to what was previously reviewed — same 55 files, same +308/-283 line counts. I re-verified the core accessor split directly rather than relying on that match alone:
ModuleLoggerAccessor/IInternalModuleLoggerAccessorsplit (src/ModularPipelines/Logging/ModuleLoggerAccessor.cs,IModuleLoggerAccessor.cs,IInternalModuleLoggerAccessor.cs): the public surface (IModuleLoggerAccessor.Logger) is a non-disposableILogger, while the internal interface addsGetLogger()/GetLogger(Type)returning the framework-ownedIModuleLogger.Logger => GetLogger()preserves the same AsyncLocal fast-path / stack-trace-fallback / locking behavior that existed before, so this is a pure type-surface narrowing, not a behavior change.- CQS split on summary logging (
ISummaryLoggervsISummaryLogReader) is a solid interface-segregation improvement — write-only consumers no longer seeGetEntries/GetOutput, the one reader (GitHubMarkdownSummaryGenerator) depends only on the read contract, and both are still backed by the sameSummaryLoggersingleton inDependencyInjectionSetup.cs, so there's no behavioral change. PublicAPI.Shipped.txt/PublicAPI.Unshipped.txtcorrectly move the removed members out of Shipped and the new/renamed members into Unshipped, and the analyzer'sLoggerInConstructorCodeFixProvidernow matchesMicrosoft.Extensions.Logging.ILoggerinstead of the oldIModuleLogger.- Docs (
logging.md,secrets.md) andRELEASE_NOTES_V4.mdare updated to match, and the breaking renameISummaryLogger.Info→Informationis called out in the release notes.
Non-blocking, carried over from prior reviews on this PR
src/ModularPipelines.Analyzers/ModularPipelines.Analyzers.Test/ModularPipelinesAnalyzersILoggerUnitTests.cs:29still buildsBadModuleSourceILoggerProviderfrom"ILoggerProvider loggerAccessor". The constructor parameter type is the framework's ownMicrosoft.Extensions.Logging.ILoggerProvider(unrelated to this PR's newIModuleLoggerAccessor), but the local was renamedloggerProvider→loggerAccessor, presumably a find/replace artifact from the accessor rename. The name now implies it's testing the new accessor type when it's actually still exercising the "inject the framework's ownILoggerProvider" bad-constructor case. Doesn't affect test correctness — a rename to something likeframeworkLoggerProviderwould remove the ambiguity, but this isn't a blocker.
No functional or compile-breaking issues found. Well-scoped, mechanical refactor with consistent updates across DI, integrations, analyzers, docs, tests, and the public API baseline.
GitHub Actions
Inactive
1 participant
Closes #4230
Summary
Microsoft.Extensions.Logging.ILoggerfrom pipeline contextsIModuleLoggerProviderwithIModuleLoggerAccessorISummaryLogger) from reads (ISummaryLogReader)Validation
dotnet format ModularPipelines.slnx --verify-no-changes --severity infoThe shared PublicAPI baseline on current
mainstill depends on #4262; rebase after that merges.