Repository navigation
Skip or fit hostmask replies that carry retained over-limit names - #94
Merged
Merged
Conversation
A reload that lowers nicknameMaxLength or channelNameMaxLength keeps existing sessions and channels, so retained names can push WHO, WHOIS, WHOWAS, 333, INVITE and channel notifications past 512 bytes. Those replies were dropped silently by the writer. Route numerics through Replies.sendChecked, which skips a reply whose fixed fields cannot fit and logs a throttled WARN naming the reply, and fit free trailing text (topic, away reason, closers) instead of dropping the line. The writer applies the same check to broadcasts. Identities are never truncated, and commands still send their closers. Fixes #93 Co-Authored-By: GPT-6.1-Sol (Codex) <noreply@openai.com>
Describe, next to the NAMES policy, how hostmask replies and nick/channel pairs that cannot fit after a limit-lowering reload are fitted or skipped with a throttled WARN. Refs #93 Co-Authored-By: GPT-6.1-Sol (Codex) <noreply@openai.com>
Reviewer's GuideThe PR extends the 512-byte reply budget policy from configured-name validation to retained over-limit identities, using a shared representability guard that skips indivisible hostmask replies with throttled warnings, fits only free trailing text, applies to broadcast notifications, and preserves command termination; documentation and unit/integration coverage validate the behavior. Sequence diagram for the shared IRC reply budget guardsequenceDiagram
participant Handler as CommandHandler
participant Replies
participant Serializer as MessageSerializer
participant Writer as SessionWriter
participant Log
Handler->>Replies: sendChecked(session, message)
Replies->>Serializer: commandBytes(message)
alt fixed fields fit within 512 bytes
Replies->>Writer: enqueueRaw(message)
else fixed fields exceed command budget
Replies->>Log: warn oversized reply
Replies-->>Handler: skip reply
end
Handler->>Replies: sendFitted(session, serverName, numeric, params, text)
Replies->>Replies: truncate(text, budget, false)
Replies->>Replies: sendChecked(session, fittedMessage)
Writer->>Serializer: serializeText(message)
alt hostmask notification is oversized
Writer->>Replies: isRepresentable(message)
Replies->>Log: warn oversized reply
Replies-->>Writer: false
else PRIVMSG or NOTICE text already fits
Writer-->>Writer: drop oversized message
end
Flow diagram for fitting or skipping retained over-limit repliesflowchart TD
Start[Build reply] --> Fixed{Fixed fields fit in 512 bytes?}
Fixed -->|No| Warn[Emit throttled WARN and skip reply]
Fixed -->|Yes| Free{Reply has free trailing text?}
Free -->|Yes| Fit[Fit text on a code-point boundary]
Free -->|No| Send[Send without truncating identities]
Fit --> Check[sendChecked]
Check --> Send
Send --> Continue[Continue command and attempt terminating numeric]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="jircd-core/src/main/java/net/jircd/core/session/command/Replies.java" line_range="55" />
<code_context>
params.addAll(List.of(trailingParams));
Message message = new Message(Map.of(), serverName, null, numeric.wireCode(), params);
- if (session.writer() != null) {
+ sendChecked(session, message);
+ }
+
</code_context>
<issue_to_address>
**Free reply text is dropped**
When an unknown WHOIS target or a direct message with a long away reason makes the free trailing text exceed the remaining 512-byte budget while the fixed fields still fit, `Replies.send` routes these replies through `sendChecked` without fitting the trailing text, so over-budget RPL_ENDOFWHOIS and RPL_AWAY replies are dropped; WHOIS lacks its 318 close and the sender gets no away notification.
Fit the free trailing text in `Replies.send` before checking and sending these replies.
Also at `jircd-core/src/main/java/net/jircd/core/session/command/WhoisCommandHandler.java:125`.
</issue_to_address>
### Comment 2
<location path="jircd-core/src/main/java/net/jircd/core/session/SessionWriter.java" line_range="254" />
<code_context>
} catch (MessageSerializer.MessageTooLongException e) {
+ // Report unrepresentable identities and nick/channel pairs through the same throttled
+ // policy as numeric replies. PRIVMSG/NOTICE have already attempted text fitting above.
+ if (!Replies.isRepresentable(message)) {
+ return;
+ }
</code_context>
<issue_to_address>
**Text-fit failures get misreported**
When an oversized PRIVMSG or NOTICE has fixed fields that leave less than one body code point’s bytes, `serializeText` can throw when an oversized PRIVMSG or NOTICE has no room for a complete body code point. `Replies.isRepresentable` then measures the original body and logs that a reply’s fixed fields exceed the budget, so operators receive a misleading diagnosis for a dropped message.
Separate fixed-field representability from text-fit failure, and log the latter with a text-specific reason.
</issue_to_address>
### Comment 3
<location path="jircd-core/src/main/java/net/jircd/core/session/SessionWriter.java" line_range="34" />
<code_context>
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
import net.jircd.core.config.ServerConfiguration;
+import net.jircd.core.session.command.Replies;
import net.jircd.protocol.CapabilityName;
import net.jircd.protocol.Command;
</code_context>
<issue_to_address>
**Writer depends on command policy**
`SessionWriter` now calls the command-layer `Replies` utility for wire-size decisions, while reply construction already depends on session types. This creates a session-to-command package dependency cycle, so reusing or extracting the writer requires pulling in command policy as well.
Move the shared representability and warning policy to a wire/session-level component that both the writer and reply builders can use.
Also at `jircd-core/src/main/java/net/jircd/core/session/SessionWriter.java:254`.
</issue_to_address>
### Comment 4
<location path="jircd-core/src/test/java/net/jircd/core/session/command/GrandfatheredReplyTest.java" line_range="148" />
<code_context>
+ : new JoinCommandHandler(channels, extensions, () -> SERVER, () -> 351);
+ try (Warnings warnings = new Warnings()) {
+ var replies = HandlerHarness.run(handler, session("requester"), command + " " + name);
+ assertThat(HandlerHarness.codes(replies)).contains("332").doesNotContain("333");
+ warnings.assertReply("333");
+ }
</code_context>
<issue_to_address>
**Fitted reply text can disappear**
When `Replies.sendFitted` regresses to dropping all free trailing text, if `sendFitted` emits an empty trailing field instead of fitting the supplied text, `topicMetadataIsSkippedWhileTopicTextIsFitted` and `whoisSkipsImpossibleIdentitiesButFitsOtherNickLinesAndCloses` still pass: they assert reply codes, while `HandlerHarness.run` checks only the line-length limit. The tests therefore accept missing topic text and away reason.
Assert that the 332 topic text and 301 away reason contain the expected text, including a non-empty fitted prefix.
Also at `jircd-core/src/test/java/net/jircd/core/session/command/GrandfatheredReplyTest.java:90`.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: jircd-core/src/main/java/net/jircd/core/session/command/Replies.java:55, jircd-core/src/test/java/net/jircd/core/session/command/GrandfatheredReplyTest.java:148
… package Review follow-ups for #93: - Replies whose last parameter is free text now cut it to fit instead of being dropped, so e.g. WHOIS on a long unknown target still sends its 318 and an away user's 301 reaches the sender. - OutboundSizeGuard (with WarningThrottle) moves the representability check into the session package, so SessionWriter no longer depends on the command layer. - A PRIVMSG/NOTICE with no room for one body code point is logged with a text-specific reason instead of a fixed-field diagnosis. - Tests assert the fitted topic and away text, and check list emptiness before element predicates. Refs #93 Co-Authored-By: GPT-6.1-Sol (Codex) <noreply@openai.com>
The limit applies to the prefix, command, parameters and CRLF; IRCv3 message tags have their own 8191-byte section and never consume it. Define the term once and use it throughout the reply-budget docs. Refs #93 Co-Authored-By: GPT-6.1-Sol (Codex) <noreply@openai.com>
Use a COMMAND_NAME constant for "TAGMSG" (S1192), pass the system zone to ZonedDateTime.now explicitly (S8688), and assert on the received closer text rather than a literal (S3415). Refs #93 Co-Authored-By: GPT-6.1-Sol (Codex) <noreply@openai.com>
|
codegeek
added a commit
that referenced
this pull request
Oct 22, 2026
* Add pre-PR checks to cut SonarCloud and Sourcery review rounds Most PRs have needed several push-and-review rounds for findings that could have been caught before the PR existed. Add a "Before opening a PR" section to AGENTS.md and a review checklist built from the Sonar issues and Sourcery threads on every merged PR (#28-#94): the Sonar rules this repo keeps hitting and the kinds of defects Sourcery keeps finding. The SonarQube CLI's code analysis returns 403 on this plan, so the local Sonar step uses the Gradle scanner's branch analysis instead. Ignore the .sonar/ cache it leaves behind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Harden the local Sonar step and correct two checklist items Review follow-ups: - Run the token-bearing scan only on trusted worktree branches, since the build executes the branch's Gradle logic with the token present. - Take the branch name from `git branch --show-current`, quoted, instead of a hand-substituted placeholder. - Wait for SonarCloud to process the analysis (sonar.qualitygate.wait) so the issue query doesn't read the previous one, and page the query. - Say when SonarCloud doesn't run (no SONAR_TOKEN, fork PRs). - A missing bind address binds every interface where the contract says so, as ServerConfiguration.Listener documents; the checklist had it backwards. - Public API breaks follow the constitution's migration-path rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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.
Why
A reload that lowers
nicknameMaxLengthorchannelNameMaxLengthkeeps existing sessions and channels ("grandfathered state"). The reply budget from #92 validates only the configured limits, so WHO, WHOIS, WHOWAS, 333, INVITE and channel notifications that embed a retained over-limit name could exceed the 512-byte command-section limit (prefix, command, parameters and CRLF; IRCv3 message tags have their own 8191-byte budget and don't count) and be dropped silently bySessionWriter. NAMES already skips an unrepresentable item with a specific WARN; this extends that policy to the hostmask-carrying replies.Changes
Replies.sendChecked/Replies.isRepresentable: every numeric goes through a size check. A reply whose fixed fields (nick, channel, ident, host, timestamps) cannot fit in the 512-byte command section is skipped with a WARN naming the reply and its byte count, throttled to one per command per minute across recipients with suppressed counts reported. Identities are never truncated.SessionWriterapplies the check to broadcasts (JOIN, PART, KICK, MODE, TOPIC); PRIVMSG/NOTICE keep their existing text fitting.sendListed; unchanged.server-configuration.mddocuments the policy next to the NAMES policy (separate commit).Testing
GrandfatheredReplyTest(13 unit cases).ReplyBudgetReloadTestlowers the limits by reload with long names retained and checks each hostmask reply either fits in the 512-byte command section or is skipped with a WARN.NamesByteBudgetTestexpectations updated: an oversized 366 is still skipped, but its WARN now comes fromRepliesinstead ofSessionWriter../gradlew cleanTest buildpasses (1,003 tests, 95.0% line / 87.4% branch coverage, Spotless, SpotBugs, PMD).Fixes #93
🤖 Generated with Claude Code
Summary by Sourcery
Ensure all outbound IRC replies and notifications respect the 512-byte command-section budget while preserving retained identities and continuing command response sequences.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: