Repository navigation
refactor(broker): one documented encoder per message kind; encodeRequest/encodeResponse visit them (#3043) - #3051
Merged
Merged
Conversation
…est/encodeResponse visit them (#3043) CodeQL flagged encodeRequest (cpp/poorly-documented-function, alert #3900): one 103-line if/else chain over every request kind, past 100 lines since protocol v6/v7. encodeResponse (248 lines) had the same shape. As #2526 did for the decoders, each kind now has a Detail::encodePayload() overload that writes its payload and returns its MessageKind, and both entry points are a std::visit over them, so a message kind without an encoder no longer compiles. The bytes on the wire are the same; no protocol version change. Fixes #3043 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
mgradwohl
marked this pull request as ready for review
October 26, 2026 16:36
mgradwohl
enabled auto-merge (squash)
October 26, 2026 16:36
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.
Description
CodeQL alert #3900 (
cpp/poorly-documented-function) flagsBroker::encodeRequest. It was one 103-lineif/else ifchain over every request kind, and passed 100 lines with protocol v6 and v7 (#2972, #3002).encodeResponsewas 248 lines of the same shape, kept under the alert only by its comments.As #2526 did for the decoders, each message kind now has its own short, documented
Detail::encodePayload(Writer&, const T&)overload. It writes that kind's payload and returns itsMessageKind.encodeRequest/encodeResponseare now astd::visitover those overloads.MessageKind::Hello/HelloReplywith an empty payload.Fixes #3043
PR Size
Medium in lines (+403/−342, one header), but each block is a branch moved into its own function.
Type of Change
Refactor, with no behaviour change.
Checklist
clang-tidyand addressed any warningsclang-formaton my changespre-commit run --all-filesor installed pre-commit hooksNo new tests: the decoders are unchanged, and
test_BrokerProtocol.cpp's round trips cover every message kind, so the encoders are checked against them. The helper tests run each request end to end.Testing
Additional Notes
Main only, no backport: no wire change, and v0.9.x's protocol code stays as it shipped.
🤖 Generated with Claude Code