Conversation
Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Assisted-By: Claude Fable 5
Assisted-By: Claude Fable 5
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds a public gRPC contract with descriptor-based dispatch, unary and streaming call APIs, immutable context, metadata handling, response lifecycle controls, status values, and gRPC-specific exceptions. The README documents these contracts and related suspension and address rules. ChangesgRPC contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GrpcDispatcher
participant Application
participant Responder
Client->>GrpcDispatcher: Send RPC
GrpcDispatcher->>Application: Provide Call and Responder
Application->>Responder: respond(message) or respond(generator)
Responder-->>Client: Send response metadata and status
``
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 5</summary>
<details>
<summary>✅ Passed checks (5 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :----------------------------------------------------------------------------------------------------------------------------------------------------- |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Title check | ✅ Passed | The title clearly identifies the pull request's main change: introducing the gRPC plugin contract. |
| Description check | ✅ Passed | The description directly explains the gRPC dispatcher contract, streaming behavior, metadata, context, and error handling introduced by the changeset. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches</summary>
<details>
<summary>📝 Generate docstrings</summary>
- [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> Create stacked PR
- [ ] <!-- {"checkboxId": "3e1879ae-f29b-4d0d-8e06-d12b7ba33d98"} --> Commit on current branch
</details>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
- [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `grpc`
</details>
<details>
<summary>✨ Simplify code</summary>
- [ ] <!-- {"checkboxId": "f120d606-b0e2-4b7d-8316-181794555b43", "radioGroupId": "simplify-output-choice-group-unknown_comment_id"} --> Create PR with simplified code
- [ ] <!-- {"checkboxId": "9a4e3077-58f6-4eba-b7ee-62e936ea00ea", "radioGroupId": "simplify-output-choice-group-unknown_comment_id"} --> Commit simplified code in branch `grpc`
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->
---
Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=rapira-rs/contract&utm_content=1)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
<details>
<summary>❤️ Share</summary>
- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai)
- [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)
- [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)
- [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
</details>
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Grpc/Responder/ResponseMetadata.php`:
- Around line 35-88: Implement the documented accumulator behavior in
ResponseMetadata, or replace the final concrete class with an interface that
declares the same methods and provide a concrete implementation used by
Responder::getResponseMetadata(). Ensure all mutators validate names, values,
lifecycle state, and binary suffix rules, accumulate repeated metadata, and make
headers() and trailers() return the stored Metadata instances.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eaf042e5-951d-4929-8631-1edd86ba76dd
📒 Files selected for processing (23)
README.mdsrc/Grpc/Call.phpsrc/Grpc/Call/Context.phpsrc/Grpc/Call/MessageStream.phpsrc/Grpc/Call/Protocol.phpsrc/Grpc/Call/StreamingRequest.phpsrc/Grpc/Call/UnaryRequest.phpsrc/Grpc/ErrorDetail.phpsrc/Grpc/Exception/GrpcException.phpsrc/Grpc/Exception/HeadersAlreadyCommittedError.phpsrc/Grpc/GrpcDispatcher.phpsrc/Grpc/GrpcDispatcherInfo.phpsrc/Grpc/Metadata.phpsrc/Grpc/MethodInfo.phpsrc/Grpc/MethodKind.phpsrc/Grpc/Responder.phpsrc/Grpc/Responder/ResponseMetadata.phpsrc/Grpc/Responder/StreamingResponse.phpsrc/Grpc/Responder/UnaryResponse.phpsrc/Grpc/ServiceInfo.phpsrc/Grpc/Status.phpsrc/Grpc/StatusCode.phpsrc/InetAddress.php
📜 Review details
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Grpc/Responder/ResponseMetadata.php
[warning] 48-48: Avoid unused parameters such as '$name'. (undefined)
(UnusedFormalParameter)
[warning] 48-48: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
[warning] 60-60: Avoid unused parameters such as '$name'. (undefined)
(UnusedFormalParameter)
[warning] 60-60: Avoid unused parameters such as '$bytes'. (undefined)
(UnusedFormalParameter)
[warning] 71-71: Avoid unused parameters such as '$name'. (undefined)
(UnusedFormalParameter)
[warning] 71-71: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
[warning] 81-81: Avoid unused parameters such as '$name'. (undefined)
(UnusedFormalParameter)
[warning] 81-81: Avoid unused parameters such as '$bytes'. (undefined)
(UnusedFormalParameter)
src/Grpc/Metadata.php
[warning] 25-25: Avoid unused parameters such as '$name'. (undefined)
(UnusedFormalParameter)
🪛 PHPStan (2.2.7)
src/Grpc/Call/MessageStream.php
[warning] 34-34: Method Rapira\Grpc\Call\MessageStream::current() should return string but return statement is missing.
(return.missing)
[warning] 37-37: Method Rapira\Grpc\Call\MessageStream::key() should return int<0, max> but return statement is missing.
(return.missing)
[warning] 50-50: Method Rapira\Grpc\Call\MessageStream::valid() has Rapira\Exception\WorkDiscardedException in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 50-50: Method Rapira\Grpc\Call\MessageStream::valid() should return bool but return statement is missing.
(return.missing)
[warning] 57-57: Method Rapira\Grpc\Call\MessageStream::rewind() has Error in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
src/Grpc/Responder/ResponseMetadata.php
[warning] 48-48: Method Rapira\Grpc\Responder\ResponseMetadata::addHeader() has Rapira\Exception\AlreadyFinalizedError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 48-48: Method Rapira\Grpc\Responder\ResponseMetadata::addHeader() has Rapira\Grpc\Exception\HeadersAlreadyCommittedError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 48-48: Method Rapira\Grpc\Responder\ResponseMetadata::addHeader() has ValueError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 60-60: Method Rapira\Grpc\Responder\ResponseMetadata::addBinaryHeader() has Rapira\Exception\AlreadyFinalizedError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 60-60: Method Rapira\Grpc\Responder\ResponseMetadata::addBinaryHeader() has Rapira\Grpc\Exception\HeadersAlreadyCommittedError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 60-60: Method Rapira\Grpc\Responder\ResponseMetadata::addBinaryHeader() has ValueError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 71-71: Method Rapira\Grpc\Responder\ResponseMetadata::addTrailer() has Rapira\Exception\AlreadyFinalizedError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 71-71: Method Rapira\Grpc\Responder\ResponseMetadata::addTrailer() has ValueError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 81-81: Method Rapira\Grpc\Responder\ResponseMetadata::addBinaryTrailer() has Rapira\Exception\AlreadyFinalizedError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 81-81: Method Rapira\Grpc\Responder\ResponseMetadata::addBinaryTrailer() has ValueError in PHPDoc @throws tag but it's not thrown.
(throws.unusedType)
[warning] 84-84: Method Rapira\Grpc\Responder\ResponseMetadata::headers() should return Rapira\Grpc\Metadata but return statement is missing.
(return.missing)
[warning] 87-87: Method Rapira\Grpc\Responder\ResponseMetadata::trailers() should return Rapira\Grpc\Metadata but return statement is missing.
(return.missing)
src/Grpc/Metadata.php
[warning] 25-25: Method Rapira\Grpc\Metadata::values() should return list but return statement is missing.
(return.missing)
[warning] 32-32: Method Rapira\Grpc\Metadata::all() should return array<string, list> but return statement is missing.
(return.missing)
[warning] 35-35: Method Rapira\Grpc\Metadata::count() should return int<0, max> but return statement is missing.
(return.missing)
[warning] 38-38: Method Rapira\Grpc\Metadata::getIterator() should return Iterator<string, list> but return statement is missing.
(return.missing)
🔇 Additional comments (21)
src/Grpc/Responder/StreamingResponse.php (1)
48-51: 🩺 Stability & AvailabilityVerify that cancellation closes retained generators.
PHP exposes no
Generator::close()operation. A generator runsfinallyon completion or after all references are removed. Service code can retain$messages, so discarding only the host reference cannot guarantee the documented cleanup. (php.net)Show that the host injects an uncatchable terminal cancellation path, or weaken this guarantee and document the retained-reference behavior.
src/Grpc/StatusCode.php (1)
7-30: LGTM!src/Grpc/Status.php (1)
16-27: LGTM!src/Grpc/ErrorDetail.php (1)
12-21: LGTM!src/Grpc/Exception/GrpcException.php (1)
28-42: LGTM!src/Grpc/Exception/HeadersAlreadyCommittedError.php (1)
9-17: LGTM!src/InetAddress.php (1)
9-11: LGTM!src/Grpc/Responder.php (1)
28-39: 🎯 Functional CorrectnessNo change needed.
The interface methods are declared with trailing semicolons and do not define method bodies.
src/Grpc/GrpcDispatcher.php (1)
40-72: LGTM!src/Grpc/GrpcDispatcherInfo.php (1)
9-15: LGTM!src/Grpc/MethodKind.php (1)
13-42: LGTM!src/Grpc/MethodInfo.php (1)
11-24: LGTM!src/Grpc/Call/StreamingRequest.php (1)
23-29: LGTM!src/Grpc/Call/MessageStream.php (1)
28-57: 🩺 Stability & AvailabilityVerify
MessageStreamregistration order.
Rapira\Grpc\Call\MessageStreamis declaredfinalwith empty typed methods, but this package providesRapira\Grpc\Call\StreamingRequest::getMessages(): MessageStream. Composer autoloads this source asRapira\; require evidence that the native host registers that class before Composer can load it. Otherwise implementcurrent(),key(), andvalid()here or publish the implemented interface for the host.src/Grpc/ServiceInfo.php (1)
7-21: LGTM!README.md (1)
271-448: LGTM!Also applies to: 477-483, 515-523
src/Grpc/Call.php (1)
10-23: LGTM!src/Grpc/Call/Context.php (1)
14-53: LGTM!src/Grpc/Call/Protocol.php (1)
7-16: LGTM!src/Grpc/Call/UnaryRequest.php (1)
11-26: LGTM!src/Grpc/Metadata.php (1)
7-38: LGTM!
Assisted-By: Claude Fable 5
1 participant
The second dispatcher surface. Host side is ConnectRPC: one registration serves native gRPC, gRPC-Web and Connect (proto + JSON). Whatever the client spoke, PHP sees canonical binary protobuf — framing, compression,
grpc-timeout, per-protocol error encoding never cross the boundary.Worker loop
Dispatch: two
instanceof, not four kindsRequest and response shapes are independent facts of the
.proto, so each is its own interface pair.MethodKindprojects onto the same axes at boot viaisStreamingRequest()/isStreamingResponse().Streaming
respond(\Generator)returns when the stream terminates; backpressure = generator not resumed.finallyruns,respond()returns normally. No cancellation token API.GrpcExceptionescaping mid-stream becomes the terminal status; anything else is a bug → sanitizedINTERNAL, worker recycled.Context and metadata
Call/Respondersplit is a privilege ladder:Contextreads data,CalladdsWorkfacts,Responderanswers without reading;receive()hands out the intersection. Namespaces follow one rule — the directory answers who gives you the object:Call\{Context, Protocol, MessageStream},Responder\ResponseMetadata; what both sides hand out (Metadata) or the user constructs (Status, descriptors) lives at the root.Full rationale, including everything deliberately left out: README § gRPC.