Repository navigation
Support the multi-prefix capability - #99
Merged
Merged
Conversation
Clients that negotiate multi-prefix need every channel status prefix a member holds, highest first, to show accurate member status; irctest's CAP tests also use it as their sample capability, so CAP REQ for it was NAKed and those tests failed. A new jircd-capabilities/multi-prefix module registers the capability. The prefix rendering stays in core, as for away-notify and batch, since the capability SPI has no rendering hook yet (#98). NAMES 353 (targeted, bare, JOIN, SAJOIN) and WHO 352 flags list all prefixes in PREFIX order when the receiving client negotiated it, and keep the single highest prefix otherwise. The NAMES and WHO reply budgets reserve one more byte for the longer prefix string. Fixes #96 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Add multi-prefix to the supported capabilities and its reply contract, and update the 353 (S + 2N + Cmax + 15) and 352 (+22) budget formulas and examples. Refs #96 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Reviewer's GuideAdds configurable IRCv3 Sequence diagram for negotiated multi-prefix repliessequenceDiagram
participant Client
participant CapabilityExtension
participant JoinCommandHandler
participant WhoCommandHandler
participant Channel
Client->>CapabilityExtension: CAP LS
CapabilityExtension-->>Client: multi-prefix
Client->>CapabilityExtension: CAP REQ multi-prefix
CapabilityExtension-->>Client: CAP ACK multi-prefix
Client->>JoinCommandHandler: JOIN channel
JoinCommandHandler->>Channel: memberListing(true)
Channel-->>JoinCommandHandler: @+nick
JoinCommandHandler-->>Client: 353 NAMES @+nick
Client->>WhoCommandHandler: WHO channel
WhoCommandHandler->>Channel: statusPrefixes(member, true)
Channel-->>WhoCommandHandler: @+
WhoCommandHandler-->>Client: 352 WHO H@+
Flow diagram for client-specific prefix renderingflowchart TD
Request["NAMES, JOIN, or WHO request"] --> Negotiated{"Receiving client negotiated multi-prefix?"}
Negotiated -->|Yes| All["Render all held prefixes in PREFIX order\n@+nick or H@+"]
Negotiated -->|No| Highest["Render highest prefix only\n@nick or H@"]
All --> Budget["Use expanded reply budget"]
Highest --> Reply["Send IRC reply"]
Budget --> Reply
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 3 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/Channel.java" line_range="131-133" />
<code_context>
+ * Without {@code multiPrefix} only the highest is returned. Each symbol comes from a separate
+ * read, so a concurrent privilege change can yield a combination the member never held at once.
+ */
+ public String statusPrefixes(ClientSession member, boolean multiPrefix) {
+ boolean operator = operators.contains(member);
+ boolean voice = voiced.contains(member);
+ if (operator) {
+ return multiPrefix && voice ? "@+" : "@";
</code_context>
<issue_to_address>
**Replies show impossible statuses**
When a privilege change overlaps status-prefix generation for a NAMES, JOIN, or channel-scoped WHO reply, `statusPrefixes` reads `operators` and `voiced` separately while privilege updates are synchronized. A revoke-operator followed by grant-voice between the reads makes a multi-prefix reply show `@+` even though the member never held both statuses.
Read both status sets under the same synchronization used by privilege updates.
</issue_to_address>
### Comment 2
<location path="specs/001-ircv3-server/contracts/server-configuration.md" line_range="270" />
<code_context>
Let `S` be the resolved server name's UTF-8 byte length, `N` be
`nicknameMaxLength`, and `Cmax` be `channelNameMaxLength` used as the channel
- UTF-8 byte cap. Both `S + 2N + Cmax + 14 <= 512` (353 RPL_NAMREPLY) and
+ UTF-8 byte cap. Both `S + 2N + Cmax + 15 <= 512` (353 RPL_NAMREPLY) and
`S + N + Cmax + 29 <= 512` (366 RPL_ENDOFNAMES) MUST hold. The first reserves
- a trailing colon and one member status character; the two nickname lengths
</code_context>
<issue_to_address>
**Data model keeps old NAMES budget**
When a configuration consumer relies on the data-model formula to choose boundary values, a consumer following the data-model validation rule calculates the 353 bound with 14 bytes of overhead, while `ConfigurationLoader.validateNamesBudget` requires 15; configurations that rule says fit can therefore be rejected at startup or reload, and the published limits disagree.
Update the data-model validation formula and its explanation to reserve the extra multi-prefix byte.
</issue_to_address>
### Comment 3
<location path="specs/001-ircv3-server/contracts/irc-protocol-commands.md" line_range="317" />
<code_context>
`@` only, matching standard client expectations (operator implies the
ability to speak; the `+` prefix communicates *additional* information
a plain `@` doesn't).
+- **`multi-prefix`** (issue #96): to a requester that has negotiated it, `353`
+ lists every status the member holds, in ISUPPORT `PREFIX` order (`@` before
+ `+`), so an operator who is also voiced is `@+nick`, and `352`'s `<status>`
</code_context>
<issue_to_address>
**Prefix contract contradicts itself**
When an implementer applies the operator-and-voice rule to a requester that negotiated multi-prefix, an implementer reading the normative rule that an operator who is also voiced is prefixed with `@` only gets a different result from the later `multi-prefix` rule requiring `@+nick`; the contract leaves the expected 353 output contradictory for negotiated clients.
Qualify the `@`-only rule as applying when the requester has not negotiated `multi-prefix`.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: jircd-core/src/main/java/net/jircd/core/session/Channel.java:133, specs/001-ircv3-server/contracts/server-configuration.md:270, specs/001-ircv3-server/contracts/irc-protocol-commands.md:317
Review follow-ups for #96: - statusPrefixes read the operator and voice sets separately, so under multi-prefix a concurrent "-o then +v" could render @+ for a member who never held both. Privilege updates now bracket themselves with a version counter under the channel monitor, and the lock-free read retries when an update overlapped it, so NAMES stays lock-free. - data-model.md still gave the old 353 budget constant (14); it is 15. - The "operator who is also voiced shows @ only" rule now applies only to a requester without multi-prefix. - Rename MultiPrefixExtension.ID, which clashed with id() (S1845), and pass a typed method reference in ChannelRegistryTest (S1612). Refs #96 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
SonarCloud reports only new code on a short-lived branch and judges "new" from git blame, so the documented scan reported nothing for uncommitted, staged or untracked changes. That is how the first local scan for #96 missed S1845 and S1612. With sonar.scm.disabled the scan reports them, and SonarCloud still leaves out issues already on main. 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
Clients that negotiate
multi-prefixexpect every channel status prefix a member holds, highest first (@+nick), so they can show accurate member status. jircd didn't offer it, soCAP REQ multi-prefixgotCAP NAK. irctest usesmulti-prefixas its sample capability for CAP negotiation, socap.CapTestCasetestReqOne, testReqTwo, testReqOneThenOne and testReqPostRegistration failed. They were the only failures left after excluding deprecated tests.Changes
New
jircd-capabilities/multi-prefixmodule registers the capability so it is advertised inCAP LSand ACKed. Like the other capabilities it is opt-in:capabilities: multi-prefix: enabled.The prefix rendering stays in core, the same split as away-notify and batch, because the capability SPI has no rendering hook yet. [Feature] Let capability modules own their behaviour through a real plugin SPI #98 tracks moving capability behaviour into the modules.
Channel.memberListing(boolean)/statusPrefixes(...)render all prefixes in ISUPPORTPREFIXorder. NAMES 353 (targeted, bare, on JOIN and SAJOIN) and WHO 352 flags (H@+) use them when the client receiving the reply negotiated the capability; other clients keep the single highest prefix.Reply budgets reserve one more byte for the longer prefix: NAMES 353 is
S + 2N + Cmax + 15(was 14) and WHO 352+22(was 21). The default server-name ceiling is unchanged (166 bytes), but a configuration tuned to the old limit by one byte is now rejected at load.Docs (separate commit): README and the spec contracts list the capability and the new formulas.
AGENTS.md: the local Sonar step adds-Dsonar.scm.disabled=true. SonarCloud judges new code from git blame, so the documented scan reported nothing for uncommitted changes; that's how the first local scan for this PR missed S1845 and S1612.Disabling a capability at runtime doesn't yet affect clients that already negotiated it. That's true of every capability today, and #97 tracks it.
Testing
ChannelTest: prefix ordering with and without the capability.MultiPrefixReplyTest: exact 353 params for NAMES, bare NAMES and JOIN, and exact 352 flags, including that the receiving client's capability decides the format.MultiPrefixTest(real sockets): CAP LS/ACK,@+alicein 353 andH@+in 352 for the negotiating client,@alice/H@for one that didn't negotiate../gradlew cleanTest buildpasses; a local SonarCloud branch analysis had the gate OK with 0 issues; a cross-vendor (Codex) review against.github/REVIEW_CHECKLIST.mdfound only the runtime-disable gap ([Bug] Disabling a capability at runtime leaves it active for clients that negotiated it #97).Fixes #96
🤖 Generated with Claude Code
Summary by Sourcery
Support opt-in IRCv3 multi-prefix negotiation and render complete channel statuses in client-specific NAMES and WHO replies.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests:
Chores: