Repository navigation
Add the capability plugin SPI and move server-time onto it - #104
Conversation
Capability modules are mostly markers today: their behaviour lives in core, keyed by CapabilityName constants, and disabling one at runtime leaves it active for sessions that negotiated it. This spec designs a registration SPI so each module owns its behaviour, and one lifecycle for enable, disable and plugin failure (prune negotiated sets, CAP DEL/NEW, CAP REQ :-cap, cap-notify advertised in CAP LS). Delivery is split into four PRs: #100 (SPI foundation, server-time), #101 (remaining migrations), #97 (runtime lifecycle) and #102 (fault isolation and load measurement). Refs #98 Co-Authored-By: GPT-6.1-Sol (Codex) <noreply@openai.com>
Capability modules were mostly markers: core held their behaviour, keyed by CapabilityName constants, and the only SPI hooks were contributeTags and includeSenderInFanOut. This adds the foundation from spec 014 so modules can own their behaviour, and proves it end to end with server-time. - CapabilityRegistrar with typed hooks (events, inbound and outbound tags, recipients, member status, NAMES identity, reply fields, output groups, commands) and CapabilityDescriptor dependencies. - Hooks are published in an immutable RuntimeSnapshot and run through HookDispatcher, which contains a throwing or invalid hook, reports it by provider, hook and category, and falls back to the default. - Enable publishes hooks and commands transactionally and rolls back on any failure; disable removes exactly the handlers the provider installed and runs every cleanup step even if one fails. Providers can't replace core commands, and duplicate provider ids are rejected. - FrameSequence/PreparedDelivery admit validated frames all-or-nothing. Frames are checked for IRC syntax and the per-origin tag budgets, and tag contributions that would overflow are dropped instead of the message. - NAMES/WHO reply budgets go through ReplyLayout/ReplyBudgetCatalog, evaluated against the configured limits. - server-time uses the outbound-tag hook; its wire output is unchanged. The other modules move in #101. API changes (unreleased 0.1.0-SNAPSHOT): ServerContext gains capability services, hook types moved to the Hooks interface, and CommandRegistrar gains abstract isRegistered/unregister. Fixes #100 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Reviewer's GuideThis PR establishes a typed, validated capability-provider SPI with immutable runtime publication, defensive hook dispatch, transactional extension and command lifecycle handling, generic reply/tag/frame budgeting, and an end-to-end migration of server-time to the new outbound-tag hook while leaving the remaining capability migrations and runtime fault withdrawal for follow-up PRs. Sequence diagram for transactional capability enablementsequenceDiagram
participant Registry as ExtensionRegistry
participant Provider as CapabilityExtension
participant Registrar as CapabilityRegistrar
participant Runtime as CapabilityRuntime
participant Commands as CommandRegistrar
Registry->>Provider: start(ServerContext)
Registry->>Provider: register(CapabilityRegistrar)
Provider->>Registrar: onOutboundTags(...)
Registrar-->>Registry: build()
Registry->>Runtime: publish(CapabilityDescriptor)
Registry->>Commands: register(command, handler)
alt publication or command installation fails
Registry->>Commands: unregister(command, installed)
Registry->>Runtime: withdraw(providerId)
Registry->>Provider: stop()
else enable succeeds
Registry-->>Registry: state = ENABLED
end
Sequence diagram for server-time outbound tag deliverysequenceDiagram
participant Message as OutboundMessage
participant Renderer as CapabilityTagRenderer
participant Snapshot as RuntimeSnapshot
participant Dispatcher as HookDispatcher
participant ServerTime as server-time
participant Writer as SessionWriter
Renderer->>Snapshot: hooksFor(OUTBOUND_TAGS, negotiated)
Renderer->>Message: contribution(hook, dispatcher)
Message->>Dispatcher: outboundTags(registration, message)
Dispatcher->>ServerTime: timeTag(message)
ServerTime-->>Dispatcher: time tag map
Dispatcher-->>Message: validated tags
Message-->>Renderer: memoized contribution
Renderer->>Writer: enqueue outbound message
Flow diagram for defensive capability hook dispatchflowchart TD
Invoke[Invoke registered hook] --> Result{Result valid?}
Result -->|Yes| Apply[Apply hook result]
Result -->|No| Report["FaultReporter.report(providerId, hook, category)"]
Report --> Default[Use documented default]
Exception[Hook throws] --> Report
Invoke --> Exception
Flow diagram for reply budget validationflowchart LR
Providers[Discovered providers] --> Catalog[ReplyBudgetCatalog]
Config[Startup or reload configuration] --> Limits[ReplyLimits]
Catalog --> Layout[ReplyLayout]
Limits --> Layout
Layout --> Check[HostmaskReplyBudget.requireFits]
Check -->|Fits| Accept[Accept configuration or enable]
Check -->|Exceeds 512 bytes| Reject[Reject configuration or enable]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 15 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/extension/ExtensionRegistry.java" line_range="384" />
<code_context>
+ capabilityRuntime.withdraw(id);
+ Throwable failure = null;
+ try {
+ removeCommands(installedCommands.getOrDefault(id, List.of()));
+ } catch (RuntimeException | LinkageError e) {
+ failure = e;
</code_context>
<issue_to_address>
**Failed commands remain callable**
When the command registrar throws while unregistering a provider handler, `disable` and rollback discard the installed-command records even when `removeCommands` fails, while the guarded handler has no withdrawn-state check. The handler remains callable after the provider stops, and the registry cannot retry removing it; clients can still invoke the provider and re-enable can fail.
Retain failed-removal records for retry and make guarded handlers reject calls after withdrawal.
Also at `jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java:280`, `jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java:282`, `jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java:388-390`, `jircd-core/src/main/java/net/jircd/core/extension/capability/HookDispatcher.java:376-377`.
</issue_to_address>
### Comment 2
<location path="jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java" line_range="267" />
<code_context>
+ validateReplyBudget(next);
+ List<HookRegistration> commands = commandsOf(descriptor);
+ requireUnownedCommands(commands);
+ capabilityRuntime.publish(descriptor);
+ boolean installed = false;
+ List<InstalledCommand> done = new ArrayList<>();
</code_context>
<issue_to_address>
**Failed enable still affects clients**
When command installation fails after hooks are published, while another thread captures or uses the published hooks, `capabilityRuntime.publish` exposes hooks before `installCommands` completes. Captured snapshots remain usable after rollback withdraws the descriptor, so hooks from an enable that fails still affect output or run provider code.
Make hook publication visible only after command installation succeeds, so a failed enable cannot expose hooks.
Also at `jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java:311`.
</issue_to_address>
### Comment 3
<location path="jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java" line_range="244" />
<code_context>
}
try {
entry.extension.start(serverContext);
+ if (entry.extension instanceof CapabilityExtension capability) publish(capability);
entry.state = Extension.State.ENABLED;
if (point != null) {
</code_context>
<issue_to_address>
**Provider callbacks block lifecycle**
When a provider callback blocks or waits for another thread that needs the registry monitor, `enable` and `disable` hold the registry monitor while running lifecycle callbacks, including `start`, `register`, and `stop`. The callback blocks other synchronized lifecycle operations, and waiting on one deadlocks lifecycle changes.
Run provider callbacks without holding the registry monitor while preserving lifecycle-operation coordination.
Also at `jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java:390`.
</issue_to_address>
### Comment 4
<location path="jircd-core/src/main/java/net/jircd/core/session/CapabilityTagRenderer.java" line_range="91" />
<code_context>
+ List<CapabilityExtension> selected = new ArrayList<>();
+ for (var extension : extensionRegistry.enabled()) {
+ if (extension instanceof CapabilityExtension capability
+ && !snapshot.hasHooks(capability.id())
+ && negotiated.contains(capability.providedCapability().name())) {
+ selected.add(capability);
</code_context>
<issue_to_address>
**Legacy tags use stale lifecycle state**
When a provider is enabled or disabled between snapshot capture and the live enabled-provider scan, `CapabilityTagRenderer.prepare` captures a `RuntimeSnapshot`, but `legacySelection` separately reads `ExtensionRegistry.enabled()`. The two views can disagree, so a withdrawn provider can contribute tags after withdrawal or legacy tags can be included or omitted inconsistently with the captured view.
Select legacy providers from lifecycle state consistent with the captured runtime snapshot.
Also at `jircd-core/src/main/java/net/jircd/core/session/CapabilityTagRenderer.java:52-89`.
</issue_to_address>
### Comment 5
<location path="jircd-core/src/main/java/net/jircd/core/extension/capability/HookDispatcher.java" line_range="163-164" />
<code_context>
+ registration,
+ () -> registration.hook(Hooks.OutboundTagHook.class).apply(message),
+ tags -> validTags(tags, registration))
+ .map(Map::copyOf)
+ .orElse(Map.of());
+ }
+
</code_context>
<issue_to_address>
**Mutable hook results bypass validation**
When a hook returns a mutable result container that another thread changes after validation, `HookDispatcher` validates provider-owned containers and then copies or consumes the originals. A concurrent mutation can introduce invalid tags, recipients, or roles after validation, or make copying throw outside hook containment, so invalid results reach callers or escape preparation.
Copy each provider result before validating it, then consume only the validated copy.
Also at `jircd-core/src/main/java/net/jircd/core/extension/capability/HookDispatcher.java:72`, `jircd-core/src/main/java/net/jircd/core/extension/capability/HookDispatcher.java:149`.
</issue_to_address>
### Comment 6
<location path="jircd-core/src/main/java/net/jircd/core/config/ConfigurationLoader.java" line_range="82" />
<code_context>
+ serverName,
+ nicknameMaxLength,
+ channelNameMaxLength,
+ ReplyBudgetCatalog.legacyCompatible().plus(discoveredReplyCatalog.get()));
}
</code_context>
<issue_to_address>
**Invalid bounds escape configuration validation**
When a provider’s reply bound is valid at reference limits but fails or exceeds 512 at the configured limits, `checkReplyBudgets` evaluates each installed provider’s bound against the actual configuration. If a bound is valid at staging’s reference limits but throws or exceeds 512 for these limits, `ReplyContribution.evaluate` throws `IllegalArgumentException`, which escapes `ConfigurationLoader.load` and reload instead of the declared `ConfigurationException` validation path.
Translate bound-evaluation failures into an actionable `ConfigurationException` during startup/reload validation, while retaining provider fault reporting for invalid declarations.
</issue_to_address>
### Comment 7
<location path="jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java" line_range="249" />
<code_context>
extensionPointOwners.put(point, id);
}
- } catch (RuntimeException e) {
+ } catch (RuntimeException | LinkageError e) {
+ // A start that threw may have acquired resources first; stop must tolerate that.
+ stopQuietly(entry.extension);
</code_context>
<issue_to_address>
**REHASH aborts on linkage failures**
When a provider throws a `LinkageError` while being enabled during REHASH, `ExtensionRegistry.enable` marks the provider failed and rethrows a `LinkageError`, but `ConfigurationReloader.reconcileSection` catches only `RuntimeException`. The error aborts the reload before `current` is updated and can leave earlier reconciliation changes applied, so a valid REHASH partially takes effect.
Handle `LinkageError` as an isolated provider failure during reconciliation so the reload can finish applying unrelated valid changes and publish the configuration.
</issue_to_address>
### Comment 8
<location path="jircd-core/src/main/java/net/jircd/core/session/OutboundMessage.java" line_range="131" />
<code_context>
- capability, key -> WireBytes.tags(key.contributeTags(this)));
+ capability,
+ key -> {
+ Map<String, String> tags = Map.copyOf(key.contributeTags(this));
+ return new Contribution(tags, WireBytes.tags(tags));
+ });
</code_context>
<issue_to_address>
**Legacy tag failure aborts delivery**
When an unmigrated provider's `contributeTags` throws or returns null, `legacyContribution` calls the unmigrated provider directly and `Map.copyOf` propagates its exception or null-result failure; `CapabilityTagRenderer.prepare` does not contain that failure. A faulty legacy provider therefore aborts message preparation instead of dropping its tags and allowing the message through.
Apply equivalent exception/null containment to the temporary legacy path, report the provider fault, and continue with an empty contribution.
</issue_to_address>
### Comment 9
<location path="jircd-core/src/main/java/net/jircd/core/extension/capability/HookDispatcher.java" line_range="176" />
<code_context>
+ return false;
+ }
+ }
+ return MessageSerializer.tagDataBytes(tags) <= registration.maxBytes();
+ }
+
</code_context>
<issue_to_address>
**Oversized tags allocate before rejection**
When a faulty provider returns an exceptionally large tag value or group tag map, `MessageSerializer.tagDataBytes` constructs the escaped tag section and UTF-8 byte array before comparing the result with the declared or per-origin limit. A faulty hook or group containing a very large tag value therefore causes memory use proportional to the invalid result, and concurrent validations can exhaust server memory before the bound rejects it.
Count escaped UTF-8 tag bytes incrementally with an early limit check instead of materializing the serialized section to validate it.
Also at `jircd-core/src/main/java/net/jircd/core/session/FrameSyntax.java:63`.
</issue_to_address>
### Comment 10
<location path="jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java" line_range="249" />
<code_context>
extensionPointOwners.put(point, id);
}
- } catch (RuntimeException e) {
+ } catch (RuntimeException | LinkageError e) {
+ // A start that threw may have acquired resources first; stop must tolerate that.
+ stopQuietly(entry.extension);
</code_context>
<issue_to_address>
**Provider errors skip cleanup**
When `start` or staging throws a non-linkage, non-VM-fatal `Error` after acquiring resources, `ExtensionRegistry.enable` catches `RuntimeException` and `LinkageError`, so another `Error` bypasses `stopQuietly` and the FAILED-state update. The provider remains recorded as DISABLED with its resources unreleased.
Handle non-VM-fatal `Error` values in the enable cleanup path, then rethrow after stopping the provider and recording FAILED.
</issue_to_address>
### Comment 11
<location path="jircd-core/src/main/java/net/jircd/core/session/CapabilityTagRenderer.java" line_range="56-57" />
<code_context>
+ List<CapabilityExtension> legacy = legacySelection(snapshot, negotiated);
+ Map<String, String> composed = new LinkedHashMap<>();
+ for (CapabilityExtension capability : legacy) {
+ composed.putAll(message.legacyTags(capability));
+ bytes = bytes.plus(message.contributionBytes(capability));
+ }
+ List<Map<String, String>> typed = new ArrayList<>();
</code_context>
<issue_to_address>
**Oversized legacy tags drop messages**
When a still-supported legacy provider returns server tags whose aggregate exceeds the origin budget, a legacy provider's tag map is counted for queue reservation but is not checked against the per-origin limit until `renderSelected`. That check throws `MessageTooLongException`, and `SessionWriter.writeFanOutMessage` returns without writing the message, so an oversized legacy contribution drops the core message instead of only the contribution.
Validate and omit an oversized legacy contribution during preparation, reporting it without preventing the message from being written.
</issue_to_address>
### Comment 12
<location path="jircd-core/src/main/java/net/jircd/core/extension/capability/CapabilityRegistrar.java" line_range="79-91" />
<code_context>
+ /** Declares hard availability dependencies on other providers; self, duplicates rejected. */
+ public synchronized void requiresProviders(String... providerIds) {
+ requireOpen();
+ for (String id : providerIds) {
+ Tokens.require(id, "required provider id");
+ if (id.equals(providerId)) {
+ throw new IllegalArgumentException("provider '" + id + "' cannot require itself");
+ }
+ if (requiredProviders.contains(id)) {
+ throw new IllegalArgumentException("duplicate required provider '" + id + "'");
+ }
+ if (requiredProviders.size() >= MAX_REQUIRED_PROVIDERS) {
+ throw new IllegalStateException(
+ "more than " + MAX_REQUIRED_PROVIDERS + " required providers");
+ }
+ requiredProviders.add(id);
+ }
+ }
</code_context>
<issue_to_address>
**Rejected dependencies remain staged**
When a multi-id `requiresProviders` call encounters an invalid, duplicate, self, or over-limit id and the provider catches the exception, `CapabilityRegistrar.requiresProviders` adds ids to `requiredProviders` as it validates them. The failed call leaves its valid prefix in the descriptor, so the provider can be enabled or later withdrawn with dependencies the caller thought were rejected.
Validate the entire id list before adding any ids to `requiredProviders`.
Also at `jircd-core/src/main/java/net/jircd/core/extension/capability/CapabilityRegistrar.java:92`.
</issue_to_address>
### Comment 13
<location path="jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java" line_range="395" />
<code_context>
+ // Already withdrawn: it stays withdrawn, recorded as failed rather than half-enabled.
+ failure = accumulate(failure, e);
+ }
+ entry.state = failure == null ? Extension.State.DISABLED : Extension.State.FAILED;
String point = extensionPoint(entry.extension);
if (point != null) {
</code_context>
<issue_to_address>
**Failed stop frees exclusive extension point**
When `stop()` fails before releasing a resource associated with the extension point, `disable` marks the provider FAILED but still removes its extension-point claim. Another provider can then enable with the same claim while the failed provider's `stop()` left its resource or behavior active, so both providers can own the supposedly exclusive point.
Keep the claim reserved until cleanup succeeds, or otherwise prevent another provider from acquiring it while the previous provider's cleanup is unresolved.
</issue_to_address>
### Comment 14
<location path="jircd-core/src/main/java/net/jircd/core/session/ConnectionHandler.java" line_range="309" />
<code_context>
+
+ @Override
+ public void unregister(Command command, CommandHandler installed) {
+ handlers.remove(command, installed);
+ }
+ };
</code_context>
<issue_to_address>
**Replacement command handler removed**
When a later command handler for the same command is a distinct instance whose `equals` returns true for the provider’s installed handler, `handlers.remove(command, installed)` compares values using `equals`, not object identity, so disabling the provider removes a distinct replacement handler when its implementation considers itself equal to the provider’s handler; the command then becomes unhandled.
Remove the mapping atomically only when its current value is the exact installed instance, using an identity check in a conditional map update.
</issue_to_address>
### Comment 15
<location path="jircd-core/src/main/java/net/jircd/core/session/FrameSyntax.java" line_range="55" />
<code_context>
+ Map<String, String> client = new HashMap<>();
+ for (var entry : tags.entrySet()) {
+ String key = entry.getKey();
+ if (key.isEmpty() || hasAny(key, " ;=\r\n\0"))
+ throw new IllegalArgumentException("Invalid tag key");
+ // CR and LF are escaped by the serializer, but NUL would be emitted unchanged.
</code_context>
<issue_to_address>
**Malformed tag keys reach clients**
When a provider returns a framed message with a tag key containing a tab or another disallowed control character, `FrameSyntax.validateTags` rejects only spaces, separators, and a few line-breaking characters, so a provider-supplied key containing a tab or another control character passes validation and `MessageSerializer` writes it verbatim into the tag section. Clients receive a malformed IRC line and may discard the message.
Validate tag keys against the protocol’s allowed key grammar before serializing them.
</issue_to_address>Sourcery assessment
Approval pending. 15 findings to address first.
Blocking findings: jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java:384, jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java:267, jircd-core/src/main/java/net/jircd/core/extension/ExtensionRegistry.java:244, jircd-core/src/main/java/net/jircd/core/session/CapabilityTagRenderer.java:91, jircd-core/src/main/java/net/jircd/core/extension/capability/HookDispatcher.java:164, and 10 more
Sourcery's review of #104 found gaps in the lifecycle and failure paths that the earlier rounds missed; a follow-up Codex check of the fixes found four more. - Provider start/register/stop run outside the registry monitor, under a private lifecycle lock that no delivery path takes. Callbacks that re-enter enable, disable or reload fail fast instead of deadlocking. - Commands are installed before hooks are published, and a provider command runs only while the current snapshot contains its provider, so hooks and commands switch together. - Failed command removal and failed stop keep their records and the extension-point claim; disable (including from REHASH, and before a re-enable) retries the cleanup. Removal compares handlers by identity. - Enable cleans up after any Error; REHASH handles LinkageError. - Hook results are copied before validation; requiresProviders stages nothing unless every id is valid; tag keys must match the IRCv3 key grammar; oversized tags are rejected from their raw length first. - Legacy contributeTags providers come from the captured snapshot, are contained like hooks, and are budgeted at enqueue, so an overflow drops their tags instead of the message. - A reply bound that fails at the configured limits is a configuration error, not an IllegalArgumentException. Refs #100 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|



Why
Capability modules are mostly markers: their behaviour lives in
jircd-core, keyed byCapabilityNameconstants, because the capability SPI only offerscontributeTagsandincludeSenderInFanOut. Feature 014 (#98) gives modules a real plugin SPI. This is the first of four PRs: it adds the foundation and moves server-time onto it, so the SPI is proven end to end before the other modules migrate in #101.Changes
specs/014-capability-plugin-spi/, separate commit): spec, plan, tasks and contracts for the whole feature, delivered as [Feature] Capability plugin SPI foundation, with server-time as the first module #100 → [Feature] Move message-tags, echo-message, away-notify, multi-prefix and batch onto the capability SPI #101 → [Bug] Disabling a capability at runtime leaves it active for clients that negotiated it #97 → [Feature] Isolate capability plugin faults and measure the runtime lifecycle #102.net.jircd.core.extension.capability):CapabilityRegistrarwith typed hooks for events, inbound and outbound tags, recipients, member status, NAMES identity, reply fields, output groups and commands, plusCapabilityDescriptordependencies.RuntimeSnapshotand run throughHookDispatcher. A hook that throws or returns invalid data is contained, reported by provider id, hook and category only, and replaced by the default result. Withdrawing a faulty provider is [Feature] Isolate capability plugin faults and measure the runtime lifecycle #102.ExtensionRegistry:LinkageError.stop()runs after a failedstart().FrameSequence/PreparedDeliveryadmit validated frames all-or-nothing.FrameSyntaxrejects CR/LF/NUL in parameters, NUL in tag values and invalid tag keys, and enforces the 4094-byte budget per tag origin. Tag contributions that would overflow are dropped and reported; the message still goes out.ReplyLayout/ReplyBudgetCatalog, evaluated against the configured limits at startup, reload and enable.Migration (unreleased
0.1.0-SNAPSHOT)ServerContextgains a capability-services component.Hooksinterface (Hooks.OutboundTagHook, …).CommandRegistrargains abstractisRegisteredandunregister, so it can no longer be written as a lambda.contributeTagsandincludeSenderInFanOutstill work; [Feature] Move message-tags, echo-message, away-notify, multi-prefix and batch onto the capability SPI #101 removes them.Testing
./gradlew build: green, 1125 tests. The existingjircd-integration-testssources are unchanged and pass../gradlew loadTeston this branch and on main (5912b68), with repeated interleaved runs: all 8 load tests pass, and the differences are within run-to-run noise. Details inspecs/014-capability-plugin-spi/validation/pr-100/.validation/pr-100/review.md).Fixes #100
🤖 Generated with Claude Code
Summary by Sourcery
Establish the capability plugin SPI and migrate server-time to validated outbound-tag hooks while preserving legacy providers for subsequent migrations.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: