**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`

---

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.

---

None.

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.

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.

---

**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 ✓

**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.

**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`) ✓

**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` ✓

**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) ✓

**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) ✓

**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 ✓

---

**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.

---

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 ✓

---

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.

---

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.