Home A - Z
Page Index

An Otter Wiki

IRC Transport Plugin Design — Rule of Two Review

Reviewer: Design Review Subagent
Date: 2026-04-23
Design: /Users/sderle/code/sherman/.claude/designs/irc-plugin.md
Baseline: sherman/cli_plugin.py, sherman/hooks.py, sherman/channel.py, sherman/main.py

Executive Summary

Proceed: YES

The design is architecturally sound and follows established patterns. The identified issues are important but not blocking — they can be addressed during implementation without redesign. The test plan is thorough and provides clear acceptance criteria.

Findings

Critical

None.

Important

  1. Hook signature mismatch in send_message
    Severity: Important
    Location: IRCPlugin.send_message (line 139-141)
    Issue: The design adds an optional latency_ms: float | None = None parameter to send_message, but the hookspec in sherman/hooks.py (line 46-55) only defines send_message(self, channel: Channel, text: str) -> None. Hooks must match the spec exactly.
    Impact: This will cause a runtime error or the hook to not be called. The latency_ms parameter should be removed or the hookspec should be updated first. Since no other transport uses this parameter, it should be removed from the design.

  2. Missing on_start fan-out behavior verification
    Severity: Important
    Location: IRCPlugin.on_start (line 77-95)
    Issue: The design uses asyncio.create_task() to launch _connect_with_retry. This assumes apluggy calls on_start implementations sequentially (not concurrently). If apluggy calls them concurrently, the task creation is redundant and may cause issues. The design's "Concerns" section (item 1) acknowledges this but doesn't resolve it.
    Impact: If the assumption is wrong, the connection task may behave unexpectedly. The implementation agent must verify this before coding; if concurrent, the design should be updated to document why create_task is still needed (e.g., to avoid blocking other plugins during long connection attempts).

  3. Connection hold strategy is fragile
    Severity: Important
    Location: IRCPlugin._connect_with_retry (line 122)
    Issue: Using await asyncio.sleep(86400) as a connection hold is fragile. If pydle disconnects silently without raising an exception, the sleep continues and reconnection never happens. The design's "Concerns" section (item 3) acknowledges this but leaves it unresolved.
    Impact: Lost connections may not be detected for up to 24 hours. The implementation agent should replace this with a more robust approach: either poll self._client.connected in a short loop, or use an asyncio.Event that pydle's on_disconnect callback sets.

  4. Ambiguous default port/TLS combination
    Severity: Important
    Location: Config Format section (lines 202-220)
    Issue: The defaults specify port: 6667 and tls: false. While Libera Chat and most modern IRC networks use port 6697 with TLS, a user who specifies tls: true without overriding port will attempt TLS on port 6667, which typically fails. Conversely, a user who specifies port: 6697 without tls: true will attempt plain text on the TLS port.
    Impact: User configuration errors. The design should either: (a) add a note that port must be coordinated with tls, (b) use conditional defaults (default port to 6697 if tls: true, else 6667), or (c) document the common combinations explicitly in the config example.

Cosmetic

  1. Config field naming inconsistency
    Severity: Cosmetic
    Location: Config Format section (line 208)
    Issue: The design uses channels (plural) for the list of channels to join, matching the nanobot pattern. However, the config shows nick instead of nickname (nanobot uses nickname). Minor inconsistency with the reference implementation.
    Impact: None — nick is clear and concise. If consistency with nanobot is desired, consider using nickname, but this is not a functional issue.

  2. No explicit handling of empty channels list
    Severity: Cosmetic
    Location: Config defaults table (line 220)
    Issue: The channels default is [] (empty list). The design doesn't specify what happens if IRC is configured but no channels are listed. Pydle will connect but not join any channels, which is valid but may be unintended.
    Impact: Minor — a debug log noting "no channels configured, connecting without joining" would be helpful for users. This can be added during implementation without design change.

  3. Test helper could be shared
    Severity: Cosmetic
    Location: Test Plan (lines 259-270)
    Issue: The _make_pm_with_registry helper is identical to the one in test_cli_plugin.py. This is good for consistency, but the design doesn't note whether this should be extracted to a shared test utility module.
    Impact: None — duplication is acceptable for now. Consider extracting to a tests/conftest.py fixture if more transport plugins are added later.

Architecture Compliance

CLIPlugin Pattern

Status: PASS ✓

The design correctly follows the CLIPlugin pattern:

  • Constructor takes pm only ✓
  • Stores state in private attributes ✓
  • Implements on_start, send_message, on_stop hooks ✓
  • Uses @hookimpl decorator (implied) ✓
  • Checks channel.matches_transport() in send_message ✓
  • Uses asyncio.create_task() for background work ✓
  • Cleans up task in on_stop with cancel/await pattern ✓

Hook Interface

Status: FAIL on one parameter (see Important finding #1)

  • on_start(self, config: dict) -> None ✓
  • send_message(self, channel, text: str) -> None ✗ (adds latency_ms parameter)
  • on_stop(self) -> None ✓

The extra latency_ms parameter must be removed or the hookspec updated. Recommend removal since it's not used by any other transport.

Channel Integration

Status: PASS ✓

  • Uses self.pm.registry.get_or_create("irc", target) ✓
  • Passes Channel object (not string) to hooks ✓
  • Uses channel.matches_transport("irc") ✓
  • Uses channel.scope (not channel.id) when sending ✓
  • Correctly distinguishes channel messages (target) from private messages (by) ✓

Config Handling

Status: PASS ✓ (with Important finding #4)

  • Reads from config.get("irc") ✓
  • Returns silently if no IRC config ✓
  • Provides sensible defaults for all fields ✓
  • Stores channels list in _channels for on_connect ✓

Error Handling

Status: PASS ✓ (with Important finding #3)

  • Exponential backoff: 10s → 20s → 40s → ... → 300s cap ✓
  • Resets delay on successful connect ✓
  • Suppresses CancelledError on shutdown via _connect_task = None signal ✓
  • Drops messages if client not connected (no queuing) ✓
  • Logs at appropriate levels (debug for expected, warning for failures) ✓

split_message Algorithm

Status: PASS ✓

  • Three-tier splitting: paragraphs → sentences → words ✓
  • Uses UTF-8 byte length (not character length) ✓
  • max_len=400 leaves room for IRC overhead ✓
  • Hard truncation with ... for oversized words ✓
  • Preserves newlines within paragraphs (unlike nanobot) ✓

Nanobot Pattern Avoidance

Status: PASS ✓

The design correctly avoids nanobot's problematic patterns:

  • No _mode_limit workaround ✓ (design calls this out)
  • No heartbeat/PING-PONG logic ✓ (pydle handles it)
  • No _progress metadata routing ✓
  • No rate limiting ✓ (out of scope, correctly omitted)
  • Direct plugin reference instead of "channel instance" indirection ✓
  • No _sanitize_content that strips newlines ✓

Test Plan Completeness

Coverage: 18 tests across 6 sections

  1. on_start (3 tests): No config, with config, channel reading ✓
  2. IRCClient events (3 tests): Channel message, self-message ignore, private message ✓
  3. send_message (3 tests): IRC channel, non-IRC ignored, long text split ✓
  4. split_message (6 tests): Short, paragraphs, sentences, words, truncation, UTF-8 ✓
  5. on_stop (2 tests): Cancel task, no-op when no task ✓
  6. Reconnection (1 test): Exponential backoff sequence ✓

Missing edge case: No test for what happens when send_message is called while client is None (design says "drop message, debug log" but no test verifies this). This is minor — the current test plan is sufficient.

Verification Against Codebase

All referenced code locations exist and match the design's assumptions:

  • sherman/cli_plugin.py — structure matches design's claims ✓
  • sherman/hooks.py — hookspecs match (except latency_ms parameter) ✓
  • sherman/channel.py — Channel, ChannelRegistry, matches_transport exist ✓
  • sherman/main.py — registration location (line 117) is correct ✓
  • nanobot_channel_irc/channel.py — reference patterns are correctly identified ✓

Recommendations for Implementation Agent

  1. Remove latency_ms parameter from send_message hook before writing tests. This is required for the tests to pass.

  2. Verify apluggy's on_start concurrency by writing a small test that creates two plugins with on_start implementations that sleep and log when they start/finish. If they run sequentially, document this fact in a code comment. If concurrent, reconsider whether create_task is needed.

  3. Replace await asyncio.sleep(86400) with a more robust connection hold:

    # Poll connection state every 5 seconds
while self._client.connected:
    await asyncio.sleep(5)

    Or use an asyncio.Event that on_disconnect sets.

  4. Add config validation note in docstring or code comment about port/TLS coordination.

  5. Consider adding a debug log in on_start when channels list is empty, to inform users they're connecting without joining any channels.

Conclusion

The design is well-structured and follows established patterns. The four "important" issues are all addressable during implementation without architectural changes. The test plan is comprehensive and will validate the critical behaviors.

Proceed to Red (failing tests) phase. Address the four important findings during implementation, especially the hook signature mismatch which must be fixed before tests can be written.