fix: Buffer HTTP response content before logging to prevent stream consumption

[thomhurst]

Summary

• Fixes Code smell: HTTP logging can consume response body stream, preventing re-reading #1610: HTTP logging can consume response body stream
• Calls LoadIntoBufferAsync() on response content before logging to ensure the stream is buffered and can be read multiple times
• Without this fix, ReadAsStringAsync() in the logger consumes the stream, making it unreadable by subsequent code

Test plan

• Build succeeds
• Manual test: make HTTP request with response body, verify body can still be read after logging

🤖 Generated with Claude Code

[thomhurst]

Summary

Adds XML documentation to ICertificates interface and fixes response body stream consumption issue (#1610) by buffering responses before logging.

Critical Issues

None found ✅

Suggestions

Performance consideration for response buffering

Location: src/ModularPipelines/Http/ResponseLoggingHttpHandler.cs:21-23

The fix correctly addresses issue #1610 by calling LoadIntoBufferAsync() before logging. However, this buffers all responses into memory unconditionally, even when:

• Response body logging is disabled (LogResponseBody = false)
• The response is binary and won't be logged anyway
• Very large responses are being downloaded

Impact: For pipelines that handle large file downloads or API responses with body logging disabled, this adds unnecessary memory pressure.

Possible optimization:
The handler could access the logging options to conditionally buffer only when body logging is enabled. However, this would require passing HttpLoggingOptions to the handler, which may not align with the current architecture.

Recommendation: Consider documenting this behavior (that response logging buffers all responses) or track as a future optimization. For now, the fix is correct and safe - it prioritizes correctness over performance, which is the right trade-off for a bug fix.

---

Overall verdict: The changes are good. The buffering fix correctly solves the stream consumption bug using the approach suggested in issue #1610. The documentation additions are clear and helpful.

Verdict

✅ APPROVE - No critical issues

[Copilot]

Pull request overview

This PR addresses issue #1610 by buffering HTTP response content before logging to prevent stream consumption, and adds comprehensive XML documentation to the ICertificates interface.

• Fixes HTTP response body stream consumption issue in logging handler
• Adds XML documentation to ICertificates interface methods

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/ModularPipelines/Http/ResponseLoggingHttpHandler.cs	Adds LoadIntoBufferAsync() call before logging to ensure response content can be read multiple times
src/ModularPipelines/Context/ICertificates.cs	Adds XML documentation to interface and methods, removes redundant 'public' modifiers from interface members

[Copilot]

The response.Content property could potentially be null, which would cause a NullReferenceException when calling LoadIntoBufferAsync(). Consider adding a null check before buffering the content.

Suggested change

	await response.Content.LoadIntoBufferAsync().ConfigureAwait(false);
	if (response.Content is not null)
	{
	await response.Content.LoadIntoBufferAsync().ConfigureAwait(false);
	}

[Copilot]

The fix for issue #1610 (buffering response content before logging) lacks test coverage. Consider adding a test that verifies the response body can still be read after logging, which would prevent regression of this bug. The test should make an HTTP request, enable response logging, and then verify the response body is still readable.