MailboxRPC 4/7: mailboxrpc ServeMux router

[bhandras]

Summary

Adds mailboxrpc.ServeMux: a small in-process router implementation that:

• Implements mailboxrpc.Router.
• Provides ServeRPC to unmarshal binary protobuf requests and dispatch to the
  registered handler.

Motivation

This gives the server side a minimal, dependency-free building block for RPC
method dispatch while higher-level mailbox transport concerns (durability,
ack/cursor handling, auth) are developed.

References

• Base contract/codegen: MailboxRPC 2/7: contract + codegen + make rpc #86
• Durability reference: MailboxRPC 1/7: durability foundation #48

[gemini-code-assist]

Summary of Changes

Hello @bhandras, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request establishes a core routing mechanism within the mailboxrpc package by introducing ServeMux. This new component is an in-process router responsible for efficiently directing incoming binary protobuf requests to their designated handlers. Its design prioritizes simplicity and independence, providing a fundamental layer for RPC dispatch without integrating complex transport-level functionalities, thereby enabling a clear separation of concerns for future development.

Highlights

• New ServeMux Implementation: Introduced mailboxrpc.ServeMux, an in-process router designed to map (service, method) pairs to specific handlers.
• RPC Dispatch Mechanism: The ServeMux provides a ServeRPC method that unmarshals binary protobuf requests and dispatches them to the appropriate registered handler.
• Minimal and Dependency-Free: This implementation serves as a lightweight, foundational building block for RPC method dispatch on the server side, intentionally avoiding higher-level transport concerns like authentication or persistence.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a ServeMux implementation for mailboxrpc, providing a minimal, in-process router for RPC method dispatch. The code is well-structured and includes initial tests for dispatching and handling missing routes. My review focuses on improving the robustness of the API by adding validation for handler registration and enhancing the maintainability and style of the test code. Overall, this is a solid addition that provides a useful building block for the server side.

[bhandras]

@codex review pls

[chatgpt-codex-connector]

To use Codex here, create a Codex account and connect to github.

[litbot-9000]

@bhandras, remember to re-request review from reviewers when ready

[Roasbeef]

Would've maybe thought that we do a message send here after looking up a service key?

[Roasbeef]

Ah I guess that's the role of the handler that's registered?

[Roasbeef]

@claude review this

[Roasbeef]

@codex review this

[claude]

Claude finished @Roasbeef's task in 1m 39s —— View job

---

Code Review Complete

I've reviewed the ServeMux implementation for mailboxrpc. Overall, this is a solid, clean router implementation that properly addresses the previous review feedback. The code is well-structured and follows most repository guidelines. Here are my findings:

---

Strengths

1. Clean API design: The ServeMux provides a minimal, focused router that correctly implements the Router interface
2. Good validation: The Handle method now includes comprehensive fail-fast validation (addressing gemini-code-assist's feedback)
3. Thread-safe: Proper use of sync.RWMutex for concurrent access
4. Forward-compatible unmarshaling: Uses DiscardUnknown: true as specified in the interface contract
5. Good test coverage: Tests cover both happy path (dispatch) and error case (missing handler)

---

Issues Found

1. Missing GoDoc comments for unexported types

Severity: Medium | Location: mailbox/rpc/mux.go:27-34

Per the development guidelines, every function and method (including unexported ones) must have a comment. The unexported types routeKey and handlerEntry lack documentation.

Current:

type routeKey struct {
	service string
	method  string
}

type handlerEntry struct {
	newReq func() proto.Message
	fn     HandlerFunc
}

Should be:

// routeKey uniquely identifies an RPC endpoint by its (service, method) pair.
type routeKey struct {
	service string
	method  string
}

// handlerEntry bundles the request constructor and handler function for a
// registered RPC method.
type handlerEntry struct {
	newReq func() proto.Message
	fn     HandlerFunc
}

2. Unexported lookup method lacks comment

Severity: Medium | Location: mailbox/rpc/mux.go:99

The lookup method should have a GoDoc-style comment starting with the method name.

Current:

// lookup returns the handler entry for (service, method) if present.
func (m *ServeMux) lookup(service string, method string) (handlerEntry, bool) {

This is actually correct! The comment starts with "lookup" and explains what it does. No issue here.

3. Missing struct field comments

Severity: Low | Location: mailbox/rpc/mux.go:23-24

Per guidelines, exported types should have field comments, but even for unexported structs, fields should be documented when their purpose isn't obvious.

Current:

type ServeMux struct {
	mu       sync.RWMutex
	handlers map[routeKey]handlerEntry
}

Should be:

type ServeMux struct {
	// mu guards concurrent access to handlers.
	mu       sync.RWMutex
	
	// handlers maps (service, method) pairs to their handler entries.
	handlers map[routeKey]handlerEntry
}

---

Regarding Roasbeef's Comment

"Would've maybe thought that we do a message send here after looking up a service key?"

Looking at line 89 in ServeRPC, after the lookup succeeds, the code:

1. Creates a new request instance via entry.newReq()
2. Unmarshals the bytes into it
3. Invokes the handler via entry.fn(ctx, req)

I believe Roasbeef may be asking whether there should be some form of message passing/dispatching to a handler goroutine rather than direct synchronous invocation. However, for an in-process router like ServeMux, synchronous dispatch is the correct design:

• The PR description explicitly states this is "in-process" routing for method dispatch
• Higher-level transport concerns (durability, ack/cursor handling) are intentionally deferred
• Synchronous dispatch keeps the mux simple and gives callers full control over concurrency
• If async dispatch is needed, it can be added at the transport layer or in the handler itself

The current implementation correctly provides minimal, synchronous routing as intended. If async message-passing semantics are desired, that would be a higher-level concern for the mailbox transport layer, not this basic mux.

---

Style/Best Practices

Test naming and organization

The tests are well-structured and follow Go testing conventions:

• Use of t.Parallel() for concurrent test execution
• Clear test names that describe what they verify
• Good use of constants (arkrpcServiceName) as addressed in previous feedback

Error handling

Error handling is appropriate:

• Returns wrapped ErrNoHandler with %w for error unwrapping
• Includes service/method context in error messages
• Checks for nil request prototype (defensive programming)

---

Recommendations

1. Add GoDoc comments for unexported types (routeKey, handlerEntry) and struct fields
2. Before merging, run:
   • make tidy-module-check
   • make lint
   • make ast-lint (to verify no formatting issues)
   • make unit pkg=mailbox/rpc (to run the tests)
3. Consider adding a test for concurrent Handle and ServeRPC calls to verify thread-safety, though this is optional for this PR
4. Consider adding a test for the nil request prototype check at mux.go:85-88

---

Verdict

This is a clean, well-designed implementation that provides exactly what it promises: a minimal, in-process router for mailbox RPC dispatch. The code is thread-safe, has good test coverage, and properly addresses previous review feedback. With the addition of missing GoDoc comments, this will be ready to merge.

The previous feedback from gemini-code-assist has been properly addressed:

• ✅ Validation in Handle()
• ✅ Consistent use of arkrpcServiceName constant
• ✅ Proper unused variable naming (_ context.Context)

Great work on keeping this focused and simple! 👍

---

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Bravo.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Roasbeef]

LGTM 🕸️