Repository navigation
Make the maximum IRC line length configurable (max-line-len) - #2456
Closed
kevinpostal wants to merge 1 commit into
Closed
kevinpostal wants to merge 1 commit into
kevinpostal wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Webpush strict serialization is now hard-capped at 4096 via utils.MaxLineLen, which can break configurations (or linked-server messages) where server.max-line-len exceeds 4096, undermining configurability.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
This PR removes the hard-coded 512-byte assumption from SplitMessage call sites and the webpush serialization/parsing path, as part of making IRC max line length configurable.
Changes:
- Renames
SplitMessage.Is512()toSplitMessage.IsSingleLine()and updates all callers. - Introduces
utils.MaxLineLen(4096) and uses it for webpush message line serialization/parsing instead of a literal512. - Updates the webpush unit test to parse with the new max line length constant.
| File | Description |
|---|---|
| irc/utils/text.go | Adds utils.MaxLineLen constant and renames Is512 → IsSingleLine. |
| irc/webpush/webpush.go | Uses utils.MaxLineLen for strict line serialization and updates IsSingleLine call site. |
| irc/webpush/webpush_test.go | Parses push lines using utils.MaxLineLen. |
| irc/client.go | Updates split-message fast-path check to IsSingleLine. |
| irc/handlers.go | Updates split-message length validation to use IsSingleLine. |
| irc/message_cache.go | Updates split-message fast-path check to IsSingleLine. |
| irc/responsebuffer.go | Updates split-message fast-path check to IsSingleLine. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
111
to
115
| // MakePushLine serializes an arbitrary IRC message as a web push message; | ||
| // we assume tags were already filtered. | ||
| func MakePushLine(pushMessage ircmsg.Message) ([]byte, error) { | ||
| if line, err := pushMessage.LineBytesStrict(false, 512); err == nil { | ||
| if line, err := pushMessage.LineBytesStrict(false, utils.MaxLineLen); err == nil { | ||
| // strip final \r\n |
Comment on lines
+11
to
+14
| const ( | ||
| // MaxLineLen is the maximum IRC line length, not including tags | ||
| MaxLineLen = 4096 | ||
| ) |
Comment on lines
43
to
47
| line := string(lineBytes) | ||
| parsed, err := ircmsg.ParseLineStrict(line, false, 512) | ||
| parsed, err := ircmsg.ParseLineStrict(line, false, utils.MaxLineLen) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
slingamn
reviewed
Sep 22, 2026
|
|
||
| const ( | ||
| // MaxLineLen is the maximum IRC line length, not including tags | ||
| MaxLineLen = 4096 |
Member
There was a problem hiding this comment.
I don't see how hardcoding this value here makes sense? See #1222 for context, for example.
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.
Follow-up cleanup for the configurable maximum IRC line length, extracted from waveplate/ergo branch
ts6-linking(commit 011144f, author @waveplate).What this commit does (verified against the diff):
SplitMessage.Is512()toSplitMessage.IsSingleLine()inirc/utils/text.goand updates all callers (irc/client.go,irc/handlers.go,irc/message_cache.go,irc/responsebuffer.go,irc/webpush/webpush.go). The old name baked the 512-byte assumption into the API; the new name reflects what it actually tests (sm.Split == nil).irc/utils/text.go:irc/webpush/webpush.go,irc/webpush/webpush_test.go) instead of the hardcoded 512.Context: the
server.max-line-lenconfig key itself (default 512, floored atDefaultMaxLineLen = 512inirc/client.go, clamped inirc/config.go) is already on master; this commit is the remaining piece from the linking work that decouples the split-message fast path and the webpush path from the 512-byte assumption. Motivation is supporting longer lines for modern clients and the server-linking work, where linked servers may exchange lines longer than the classic 512 bytes.Testing:
make test(gencapdefs check, gofmt check, vet, unit tests) passes locally on this branch.make irctestwas NOT run (needs the irctest harness).Author credit: @waveplate — cherry-picked verbatim, authorship preserved.