Repository navigation
Move message-tags, echo-message, away-notify, multi-prefix and batch onto the capability SPI - #106
Merged
Merged
Conversation
…onto the capability SPI Core still carried behaviour for five capabilities through name checks, contributeTags and includeSenderInFanOut, so a capability module was a marker rather than the owner of its behaviour. Each module now registers SPI hooks and core keeps only CAP negotiation plus generic mechanisms: - MessageDelivery is shared by PRIVMSG, NOTICE and TAGMSG. deliver only accepts an opaque authorization from authorize, so a caller cannot bypass send gates by supplying its own recipients. - Forwarded client tags record every capability that selected them and reach only recipients of one of those; unowned tags fail closed. - TAGMSG moves into message-tags (421 when it is absent). - Committed join/part/quit/nick/away facts are published after locks; away-notify builds its fan-out from them. - multi-prefix renders member status through generic member facts and declares its own reply bound. - batch shapes output groups; SessionWriter.enqueueGroup admits a group whole, and a framed group that exceeds SendQ overflows instead of falling back to plain. Existing integration tests are unchanged; irctest shows no case-level difference from main. Fixes #101 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The data model and protocol contracts still described capability behaviour as core's. Point them at the owning modules and the generic reply and output-group rules, without claiming the runtime lifecycle (#97) or fault isolation (#102) that later PRs add. Refs #101 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Reviewer's GuideThis PR completes the migration of message-tags, echo-message, away-notify, multi-prefix, and batch—alongside server-time—onto the typed capability SPI, extracting generic delivery, committed-event, reply-rendering, and atomic group-queue mechanisms from core while deleting legacy capability branches and documenting and testing module-owned behavior. Sequence diagram for authorized message delivery with capability hookssequenceDiagram
participant Sender
participant Handler as MessageCommandHandler
participant Delivery as MessageDelivery
participant SPI as CapabilitySPI
participant Recipient
Sender->>Handler: handle
Handler->>Delivery: authorize
Delivery-->>Handler: AuthorizedTarget
Handler->>Delivery: deliver
Delivery->>SPI: inboundSelection
SPI-->>Delivery: forwarded tags and owners
Delivery->>SPI: recipientRoles
SPI-->>Delivery: delivery roles
Delivery->>Recipient: enqueue envelope
Sequence diagram for committed away-notify eventssequenceDiagram
participant User
participant Away as AwayCommandHandler
participant Events as CommittedEvents
participant SPI as away-notify provider
participant Peers
User->>Away: handle
Away->>Away: setAwayReason
Away->>User: AWAY confirmation
Away->>Events: awayChanged
Events->>SPI: event
SPI-->>Events: EventIntent
Events->>Peers: deliver AWAY notice
Flow diagram for provider-owned output groupingflowchart LR
Producer[Server output group]
Writer[SessionWriter.enqueueGroup]
Renderer[CapabilityTagRenderer.frame]
Batch[batch provider]
Queue[Atomic queue entry]
Recipient[Recipient connection]
Producer --> Writer
Writer --> Renderer
Renderer --> Batch
Batch -->|validated framed plan| Queue
Renderer -->|no applicable provider| Queue
Queue --> Recipient
Queue -->|SendQ overflow| Recipient
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 5 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/CommittedEvents.java" line_range="79" />
<code_context>
+ after,
+ new Audience(() -> List.copyOf(audience.get())));
+ for (EventIntent intent : extensionRegistry.hookDispatcher().event(snapshot, event)) {
+ Recipients.deliver(intent.recipients(), intent.message());
+ }
+ } catch (RuntimeException e) {
</code_context>
<issue_to_address>
**Slow hooks stall requests**
When an enabled provider hook blocks indefinitely or takes an unusually long time, provider callbacks run synchronously on the command or enqueueing thread, and the exception handling only catches callbacks that return or throw. A stalled event, inbound-tag, or framing hook therefore blocks that thread indefinitely, delaying command replies or output-group admission.
Run hooks behind a bounded execution policy or otherwise ensure a stalled provider cannot block command handling and group admission.
Also at `jircd-core/src/main/java/net/jircd/core/session/MessageDelivery.java:196`, `jircd-core/src/main/java/net/jircd/core/session/SessionWriter.java:215`.
</issue_to_address>
### Comment 2
<location path="jircd-capabilities/message-tags/src/main/java/net/jircd/capabilities/messagetags/TagmsgCommandHandler.java" line_range="72" />
<code_context>
+ if (recipients.isEmpty()) {
+ return;
+ }
+ shared.deliver(
+ session,
+ COMMAND_NAME,
</code_context>
<issue_to_address>
**Unnegotiated senders can use TAGMSG**
When a registered client sends `TAGMSG` without negotiating `message-tags`, `TagmsgCommandHandler` authorizes and delivers `TAGMSG` without checking the sender’s negotiated capabilities, and the new test explicitly expects this; clients that have not negotiated `message-tags` can therefore use a command whose extended-command contract requires that negotiation.
Enforce the documented sender precondition or update the extended-command contract to explicitly permit unnegotiated senders.
</issue_to_address>
### Comment 3
<location path="jircd-core/src/main/java/net/jircd/core/session/command/AwayCommandHandler.java" line_range="59-60" />
<code_context>
// An empty trailing argument clears away status the same
// way a fully-absent one already does, rather than setting an empty-string away reason.
if (message.params().isEmpty() || message.params().getFirst().isEmpty()) {
+ String previous = session.awayReason();
session.setAwayReason(null);
Replies.sendFitted(
</code_context>
<issue_to_address>
**Away notices can contradict final state**
When two threads invoke `AwayCommandHandler.handle` for the same session and one pauses after setting its reason while the other sets and publishes a later reason, concurrent `AWAY` handlers can interleave their reason reads, writes, and `awayChanged` publications. One handler can publish after a later state change, so peers receive notices in an order that leaves their observed away status different from the session’s final reason.
Serialize each session’s AWAY read, state update, and event publication so later transitions cannot publish ahead of earlier ones.
Also at `jircd-core/src/main/java/net/jircd/core/session/command/AwayCommandHandler.java:47-48`.
</issue_to_address>
### Comment 4
<location path="jircd-core/src/main/java/net/jircd/core/session/command/JoinCommandHandler.java" line_range="144-147" />
<code_context>
Message joinNotification =
new Message(Map.of(), presentedForm, Command.JOIN, "JOIN", List.of(channel.name()));
Recipients.deliver(result.recipients(), joinNotification);
+ extensionRegistry
+ .committedEvents()
+ .joined(session, presentedForm, channel.name(), result.recipients());
Channel.Topic topic = channel.topicState();
if (topic != null) {
</code_context>
<issue_to_address>
**Committed events arrive out of order**
When two threads commit joins to the same channel and the first pauses before publishing, the join commits before this publication call, but `CommittedEvents.publish` assigns `operationId` only when publication begins. A second thread can commit and publish a later join first, so listeners receive events in the opposite order from the channel transitions and can end with stale derived state.
Assign an ordering token at the transition commit and ensure listeners observe committed events in that order.
Also at `jircd-core/src/main/java/net/jircd/core/session/CommittedEvents.java:73`.
</issue_to_address>
### Comment 5
<location path="jircd-core/src/main/java/net/jircd/core/session/command/PartCommandHandler.java" line_range="93-100" />
<code_context>
Message joinNotification =
new Message(Map.of(), presentedForm, Command.JOIN, "JOIN", List.of(channel.name()));
Recipients.deliver(result.recipients(), joinNotification);
+ extensionRegistry
+ .committedEvents()
+ .joined(session, presentedForm, channel.name(), result.recipients());
</code_context>
<issue_to_address>
**Kicked members emit no event**
When a capability listener relies on committed membership-departure events and a member is removed with KICK, `KickCommandHandler` removes the target and sends the KICK notification, but never publishes a committed event; unlike this PART path, extension listeners therefore receive no fact about a KICK departure.
Publish a committed KICK/departure event from the successful KICK path after the membership transition, with the captured audience and kick details.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and the shared delivery path now decides which channel and direct-message recipients are authorized, while capability hooks add sender copies and control which client tags reach which peers. A faulty gate or recipient selection could expose messages or tags that have already been sent; reverting restores the previous delivery path, but cannot recall that traffic.
Blocking findings: jircd-core/src/main/java/net/jircd/core/session/CommittedEvents.java:79, jircd-capabilities/message-tags/src/main/java/net/jircd/capabilities/messagetags/TagmsgCommandHandler.java:72, jircd-core/src/main/java/net/jircd/core/session/command/AwayCommandHandler.java:60, jircd-core/src/main/java/net/jircd/core/session/command/JoinCommandHandler.java:147, jircd-core/src/main/java/net/jircd/core/session/command/PartCommandHandler.java:100
Sourcery noted that a successful KICK removed a member without publishing anything, so providers listening for membership changes missed kicks. KICK now publishes EventKind.KICKED after the transition, carrying the kicker as the event's actor. The victim's identity is captured before the removal (RFC1459 folding), so a racing NICK can't rename the event; if the capture fails, the KICK still proceeds and only the event is skipped. Sourcery also asked about ordering between committed events. The guarantee is now stated where it holds: events a session causes with its own commands are published in the order it issued them. Events another session causes about it, and events about different subjects, have no relative order. Refs #101 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The KICKED event and ordering tests added 8 jircd-core tests, so the recorded build figures were stale. Refs #101 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
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
#100 added the capability plugin SPI and moved server-time onto it, but core still carried behaviour for the other five capabilities through name checks,
contributeTagsandincludeSenderInFanOut. The modules were markers. This is the second of four PRs for #98 (design:specs/014-capability-plugin-spi/, tasks T017–T041). After it, each capability module owns its behaviour and core keeps only CAP negotiation plus generic mechanisms.Changes
Migrated one capability at a time, with the listed integration tests green and unchanged at each checkpoint (
validation/pr-101/migrations.md):MessageDeliveryis extracted fromMessageCommandHandlerand serves PRIVMSG, NOTICE and TAGMSG.deliveraccepts only the opaqueAuthorizedTargetfromauthorize, so a provider command cannot bypass channel, membership, moderation or ban checks by supplying recipients.CommittedEvents); the module builds the AWAY fan-out from them. Core keeps away state and confirmations.MemberFacts,MemberReplies). The module declares its own 2-byte reply bound, replacing the fixed widths inConfigurationLoader/HostmaskReplyBudget.SessionWriter.enqueueBatchbecomesenqueueGroupwith validated framed/plain alternatives admitted as one queue entry. A framed group whose wrappers exceed SendQ overflows that recipient instead of falling back to plain.contributeTags,includeSenderInFanOut, the temporary bridge and the coreTagmsgCommandHandler. README documents the API migration (unreleased 0.1.0-SNAPSHOT).Deviations from
tasks.mdare listed with reasons invalidation/pr-101/results.md.Testing
./gradlew build: 1,254 tests pass, Spotless/SpotBugs/PMD clean. Aggregate coverage 95.56% line / 88.61% branch (94.95% / 87.30% after [Feature] Capability plugin SPI foundation, with server-time as the first module #100).CapabilityGroupTestandCapabilityProviderAbsenceTest. No existing integration test was modified or deleted../gradlew loadTeston main and this branch, interleaved: all 8 pass, differences within noise (results.md).CapTestCaserequests for multi-prefix, which the frozen controller config does not enable (validation/pr-101/irctest.md).validation/pr-101/review.md).Fixes #101
🤖 Generated with Claude Code
Summary by Sourcery
Migrate the remaining bundled capabilities onto the typed capability SPI while leaving core responsible only for negotiation and generic delivery, event, reply, and queue mechanisms.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: