refactor: drop RPC layer; goridge is a pure IPC transport library#203
Merged
refactor: drop RPC layer; goridge is a pure IPC transport library#203
Conversation
The rpc plugin migrated to Connect-RPC, leaving goridge's pkg/rpc
(net/rpc.ServerCodec / ClientCodec on top of the goridge frame protocol)
without an in-tree consumer. Strip it; the package's role going forward
is the binary frame protocol with pipe and socket relays for PHP↔Go IPC.
Removed:
- pkg/rpc/ — codec, client, doc, tests
- benchmarks/main.go — net/rpc benchmark (meaningless without pkg/rpc)
- tests/issues/ — RPC-specific regression test
- tests/message.{pb.go,proto} — proto used only by RPC tests
- tests/php_test_files/ — PHP fixtures for the RPC client
Updated:
- go.mod / go.sum — drop google.golang.org/protobuf and its
transitives (no remaining consumer)
- README.md — reframe as "IPC transport with pipe/socket
relays"; replace the net/rpc sample with a
socket.NewSocketRelay echo server
- Makefile, .github/workflows/{linux,macos,windows}.yml — drop the
./pkg/rpc test invocations
Breaking change for anything importing github.com/roadrunner-server/goridge/v4/pkg/rpc.
A sweep of /home/valery/projects/spiral/plugins shows ~30 test-helper
files in spiral plugins still importing pkg/rpc; those modules pin
v4.0.0-beta.1 in their own go.mod files, so they keep building until
each plugin chooses to bump goridge — that bump is the natural fast-fail
trigger for migrating each plugin's test client to Connect.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes Goridge's net/rpc codec infrastructure entirely, repositioning the project as a standalone binary frame protocol library with pipe and socket transports. The changes eliminate 1,300+ lines of RPC-specific code, tests, and PHP integration while pivoting documentation and build configuration to emphasize frame-based IPC. ChangesRPC Infrastructure Removal & Frame Protocol Pivot
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.claude/settings.local.json:
- Around line 1-21: Remove the local Claude policy file from the PR and stop
tracking it: delete .claude/settings.local.json from the commit, add the
filename (or the entire .claude/ directory) to .gitignore, and run git rm
--cached on the file so it remains local but is not committed; also re-check the
commit history for any other local agent policy files and ensure none are
included before merging.
In `@README.md`:
- Line 23: Update the README wording: change the phrase "no external
dependencies, drop-in (64bit PHP required for the PHP side)" to use "64-bit"
(i.e., "no external dependencies, drop-in (64-bit PHP required for the PHP
side)"), and rename the heading "### Sample of usage" to "### Sample usage" to
improve clarity; edit the exact strings in the README where they appear.
- Line 41: The heading "Sample of usage" currently uses ATX style (###); change
it to the document's setext-style to satisfy MD003 by replacing the ATX line
"### Sample of usage" with a setext header: keep the text "Sample of usage" on
its own line and add a following underline of hyphens (---) to make it a setext
H2 so it matches the rest of the README's header style.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a38fc058-6869-453e-8dbb-4d856ae4d424
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumtests/message.pb.gois excluded by!**/*.pb.gotests/php_test_files/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.claude/settings.local.json.github/workflows/linux.yml.github/workflows/macos.yml.github/workflows/windows.ymlMakefileREADME.mdbenchmarks/main.gogo.modpkg/rpc/client.gopkg/rpc/client_server_test.gopkg/rpc/codec.gopkg/rpc/codec_edge_test.gopkg/rpc/doc.gotests/issues/issue_185_test.gotests/message.prototests/php_test_files/composer.jsontests/php_test_files/issue_185.php
💤 Files with no reviewable changes (14)
- .github/workflows/linux.yml
- .github/workflows/macos.yml
- Makefile
- pkg/rpc/doc.go
- tests/php_test_files/issue_185.php
- tests/php_test_files/composer.json
- pkg/rpc/client_server_test.go
- .github/workflows/windows.yml
- pkg/rpc/client.go
- tests/issues/issue_185_test.go
- benchmarks/main.go
- tests/message.proto
- pkg/rpc/codec.go
- pkg/rpc/codec_edge_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #203 +/- ##
===========================================
+ Coverage 72.05% 90.59% +18.53%
===========================================
Files 8 5 -3
Lines 816 372 -444
===========================================
- Hits 588 337 -251
+ Misses 193 21 -172
+ Partials 35 14 -21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors goridge into a pure IPC transport library by removing the in-tree net/rpc codec layer and associated benchmarks/tests, and updating docs/tooling to reflect the frame protocol + relays as the primary surface.
Changes:
- Remove
pkg/rpc(server/client codecs on top of the frame protocol) and all RPC-specific benchmarks and tests. - Drop protobuf dependencies from
go.mod/go.sum. - Update
README.md,Makefile, and CI workflows to align with a transport-only library.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/php_test_files/issue_185.php | Removed PHP RPC fixture used by the deleted RPC layer tests. |
| tests/php_test_files/composer.lock | Removed PHP dependency lockfile used only by removed fixtures/tests. |
| tests/php_test_files/composer.json | Removed PHP fixture dependency definition. |
| tests/message.proto | Removed proto schema used only by removed RPC tests/benchmarks. |
| tests/message.pb.go | Removed generated Go protobuf code tied to deleted RPC tests/benchmarks. |
| tests/issues/issue_185_test.go | Removed Go regression test that depended on pkg/rpc and PHP fixtures. |
| README.md | Reframed project description around frame protocol + relays; replaced net/rpc sample with socket relay echo server. |
| pkg/rpc/doc.go | Removed RPC package docs due to deletion of RPC layer. |
| pkg/rpc/codec.go | Removed server-side net/rpc codec implementation. |
| pkg/rpc/codec_edge_test.go | Removed RPC codec edge/regression tests. |
| pkg/rpc/client.go | Removed client-side net/rpc codec implementation. |
| pkg/rpc/client_server_test.go | Removed RPC client/server integration tests (proto/json/raw/concurrency). |
| Makefile | Removed go test target invocation for deleted ./pkg/rpc. |
| go.sum | Removed protobuf-related sums; updated roadrunner-server/errors sums. |
| go.mod | Dropped protobuf dependency; bumped roadrunner-server/errors version. |
| benchmarks/main.go | Removed benchmark app that exercised the deleted RPC codec. |
| .github/workflows/windows.yml | Updated CI to stop testing ./pkg/rpc. |
| .github/workflows/macos.yml | Updated CI to stop testing ./pkg/rpc. |
| .github/workflows/linux.yml | Updated CI to stop testing ./pkg/rpc. |
| .claude/settings.local.json | Added Claude local permissions config file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for { | ||
| conn, err := ln.Accept() | ||
| if err != nil { | ||
| continue |
Comment on lines
+80
to
+86
| out := frame.NewFrame() | ||
| out.WriteVersion(out.Header(), frame.Version1) | ||
| out.WriteFlags(out.Header(), frame.CodecRaw) | ||
| out.WritePayload(in.Payload()) | ||
| out.WritePayloadLen(out.Header(), uint32(len(in.Payload()))) | ||
| out.WriteCRC(out.Header()) | ||
|
|
Comment on lines
+3
to
+19
| "allow": [ | ||
| "Bash(tail:*)", | ||
| "Bash(go test:*)", | ||
| "Bash(golangci-lint run:*)", | ||
| "Bash(head:*)", | ||
| "Bash(gh api:*)", | ||
| "Bash(ls:*)", | ||
| "Bash(which yamllint:*)", | ||
| "Bash(go run:*)", | ||
| "WebFetch(domain:github.com)", | ||
| "WebFetch(domain:raw.githubusercontent.com)", | ||
| "Bash(php:*)", | ||
| "WebFetch(domain:packagist.org)", | ||
| "Bash(git:*)", | ||
| "WebFetch(domain:api.github.com)", | ||
| "WebSearch" | ||
| ] |
Removes the accidentally-tracked .claude/settings.local.json (it's a per-user IDE state file, not shared project config) and adds it to .gitignore so it stays out of the repo on future commits.
- "64bit" → "64-bit" hyphenation - "Sample of usage" → "Sample usage", switch from ATX (###) to setext to match the rest of the README and silence markdownlint MD003 - log + return on Accept() failure instead of silently spinning - mirror the incoming frame's version + flags in the response so the echo example is faithful for clients sending JSON / Proto / Gob framed payloads
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pkg/rpc/(thenet/rpc.ServerCodec+ClientCodecimplementations on top of the goridge frame protocol). The rpc plugin migrated to Connect-RPC, so this layer no longer has an in-tree consumer.benchmarks/main.go(benchmarked the deleted RPC codec) and thetests/directory (RPC regression test, generated proto used only by RPC tests, and the PHP fixtures for the goridge RPC client).google.golang.org/protobufand its transitives fromgo.mod/go.sum.README.mdaround the binary frame protocol + pipe/socket relays; replaces thenet/rpcsample with asocket.NewSocketRelayecho server.Makefileand the Linux/macOS/Windows workflows to stop testing./pkg/rpc.Net change: 19 files, +44 / -1874.
Breaking changes
github.com/roadrunner-server/goridge/v4/pkg/rpcno longer compiles after the next beta bump./home/valery/projects/spiral/plugins/*finds ~30 test-helper files still importingpkg/rpc; each pinsv4.0.0-beta.1in its own go.mod and keeps building until that plugin bumps goridge. The bump is the natural fast-fail trigger for migrating each plugin's test client to a Connect client.v4.0.0-beta.1(pre-stable), so the next tag should bev4.0.0-beta.2(or higher) — the existing "never delete beta tags" policy applies.Summary by CodeRabbit
Documentation
Refactor
Tests
Chores