Fix Response constructor regression for direct PSR-7 stream bodies

[Copilot]

• Inspect the current Response constructor and response tests to understand the current regression fix
• Review Stream::create() / createNotNull() and existing response parsing behavior to find the smallest viable change
• Revert the current constructor/test changes and rework them toward a smaller fix
• Replace the fully mocked regression test with a more concrete stream-body regression test, or remove it if it is not needed
• Run baseline and targeted validation for the affected response behavior
• Run final validation, keep only the minimal necessary diff, and summarize the new direction

---

This change is 

[voku]

/gemini review

[coveralls]

coverage: 89.902% (+0.5%) from 89.439% — copilot/increase-code-coverage-validation into master

[gemini-code-assist]

Code Review

This pull request updates the Response constructor to use StreamInterface and introduces a mechanism to preserve the body stream when specific conditions are met, such as the absence of a request context. It also adds a comprehensive set of tests in OutputContractCoverageTest.php and ResponseTest.php. The review feedback recommends renaming the new private method to _shouldPreserveBodyStream to follow the repository's naming conventions and suggests adding a check for raw_body to ensure the preservation logic correctly identifies original stream inputs.

[gemini-code-assist]

Update the method call to match the suggested rename to _shouldPreserveBodyStream to maintain consistency with the repository's naming convention for private methods.

        $bodyParsed = $this->_shouldPreserveBodyStream($body) ? $body : $this->_parse($body);

[gemini-code-assist]

In this repository, private and internal methods consistently use a leading underscore (e.g., _parse, _interpretHeaders). To adhere to this implicit style guide, please rename this method to _shouldPreserveBodyStream.

    private function _shouldPreserveBodyStream($body): bool

References

1. Private and internal methods in this repository follow a convention of being prefixed with an underscore. ^(link)

[gemini-code-assist]

The current implementation might inadvertently skip the parsing path for non-stream inputs (like strings or arrays) that are converted to streams earlier in the constructor (lines 95-98). This would cause raw_body to be overwritten with a StreamInterface object at line 134, which changes the expected behavior of getRawBody() for those types.

Adding a check for $this->raw_body === null ensures that this preservation logic only applies when the input was originally a stream (or null), as raw_body is only populated at line 96 when the input is not a stream.

        return $body instanceof StreamInterface
            && $this->raw_body === null
            && $this->request === null

[Copilot]

Pull request overview

This PR fixes a regression in Httpful\Response where constructing a response directly from a PSR-7 StreamInterface could eagerly stringify/parse the stream during construction, replacing the original body instance.

Changes:

• Treat any PSR-7 StreamInterface as a first-class response body input in Response::__construct().
• Add a regression test ensuring new Response($stream) preserves the same stream instance and does not call __toString() during construction.
• Add broader output/contract coverage tests for several behaviors across Factory/Setup/Handlers/Response/ServerRequest.

Reviewed changes

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

File	Description
src/Httpful/Response.php	Updates constructor logic to preserve direct PSR-7 stream bodies when there’s no parsing context.
tests/Httpful/ResponseTest.php	Adds a regression test asserting generic StreamInterface bodies are preserved and not stringified during construction.
tests/Httpful/OutputContractCoverageTest.php	Introduces additional tests to exercise various output/contract behaviors for coverage.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When shouldPreserveBodyStream() returns true, $bodyParsed can be an arbitrary PSR-7 StreamInterface. Passing that into Stream::createNotNull() will hit Stream::createNotNull()’s assert($stream instanceof self) and can raise an assertion warning/exception (PHPUnit converts warnings to exceptions), breaking the new regression test and any runtime with assertions enabled. Consider bypassing createNotNull() when $bodyParsed is already a StreamInterface (assign it directly), or adjust createNotNull() to only assert StreamInterface rather than self.

Suggested change

	$this->body = Stream::createNotNull($bodyParsed);
	$this->body = $bodyParsed instanceof StreamInterface
	? $bodyParsed
	: Stream::createNotNull($bodyParsed);

[Copilot]

This test mocks StreamInterface and constructs new Response($body). With the current constructor implementation, $bodyParsed will be the mock and then Stream::createNotNull($bodyParsed) triggers assert($stream instanceof self) inside Stream::createNotNull(), which can fail (and PHPUnit will treat the resulting warning as an exception). Once the constructor avoids calling createNotNull() for an already-StreamInterface body (or createNotNull() is relaxed), the test will behave as intended.

[Copilot]

shouldPreserveBodyStream($body) is evaluated after non-stream inputs have already been coerced into a StreamInterface via Stream::create(). That means responses like new Response('baz') / new Response(null) will now also skip _parse() whenever there’s no request and no headers, changing getRawBody() (and potentially __toString() fallback behavior) from the previously-parsed scalar to a stream instance. If the intent is only to preserve original stream inputs, capture a $bodyWasStream = $body instanceof StreamInterface before coercion (or similar) and gate the preservation logic on that flag so scalar inputs keep the old parsing/raw_body semantics.

[voku]

@copilot apply changes based on the comments in this thread PS: let's revert the code changes and rework it the TDD way

[Copilot]

@copilot apply changes based on the comments in this thread PS: let's revert the code changes and rework it the TDD way

Addressed in 2278465. I reverted the earlier constructor change, reintroduced the fix TDD-first with a failing regression test, and reworked Response so only original StreamInterface inputs are preserved directly while scalar inputs stay on the old parse path. No UI changes in this library PR, so there isn’t a screenshot to attach.

[voku]

@copilot Explain me this change if the production code, I don't see what regressions / bug was fixed here?? Just for testing? If here comes a yes, then at least use it and add tests until you find a real regression, otherwise your tests are shit

[Copilot]

@copilot Explain me this change if the production code, I don't see what regressions / bug was fixed here?? Just for testing? If here comes a yes, then at least use it and add tests until you find a r...

This was a production bug, not a test-only change. Before c4639ec, Response::__construct() only special-cased Httpful\Stream, so passing any other PSR-7 StreamInterface into new Response($stream) went through Stream::createNotNull() and hit assert($stream instanceof self). That means a valid non-Httpful PSR-7 stream could fail under assertions and the original body instance was not reliably preserved. The regression test uses a non-Httpful StreamInterface on purpose, because that exact case was the broken one; using Httpful\Stream would not cover the bug.