feat(hermes): Zulip adapter Gen 2 — dedup cleanup task, self-test, health stats, dynamic all-bots
Gen 2 improvements from build-zulip-plugin contract run:
1. Background dedup maintenance task
- _dedup_cleanup_forever() runs every 120s, pruning stale entries
- _is_duplicate() is now pure O(1) — no per-call cleanup overhead
- Cleanup is cancelled gracefully on disconnect
2. Self-test diagnostics (selftest())
- 8 checks: connection, queue, HTTP client, bot identity, poll loop,
dedup cleanup, echo prevention, @all-bots configuration
- Returns structured verdict: healthy / degraded / critical_failure
- Verifies every Success Criterion from the contract
3. Health stats tracking (get_health_stats())
- Uptime, poll counts/errors, message routing counts, send stats
- Error rate computation, dedup map size
- Suitable for periodic RA-H OS knowledge graph logging
4. Dynamic @all-bots resolution
- _resolve_all_bots_user_id() queries Zulip /api/v1/users on connect
- Falls back to configured env var or hardcoded default (1)
- Eliminates fragile hardcoded default in multi-bot deployments
5. Connection lifecycle now manages both poll_task and dedup_cleanup_task
- Both cancelled gracefully on disconnect()
- dedup task starts in connect() after queue registration
This commit is contained in:
+105
@@ -0,0 +1,105 @@
|
||||
# PR #21 Review — fix(tanko): adapter fixes, event logging, platform-based deploy.sh
|
||||
|
||||
Reviewer: Abiba
|
||||
Date: 2026-06-20
|
||||
Status: ✅ Approve with changes (3 blocking, 3 advisory)
|
||||
|
||||
---
|
||||
|
||||
## Review Comments
|
||||
|
||||
### Comment 1 (🔴 Blocking): CI stuck at "Waiting to run"
|
||||
|
||||
The Gitea Actions workflow (run #4) hasn't started. No CI results available. Per the GitOps branch protection rules, status checks must pass before merge. Do not merge until CI completes and all jobs are green.
|
||||
|
||||
---
|
||||
|
||||
### Comment 2 (🟡 High): `print()` for journald is an anti-pattern
|
||||
|
||||
In `adapter.py`, the new `_process_event` method uses raw `print()` for journald visibility:
|
||||
|
||||
```python
|
||||
print(f"[ZULIP_EVENT] Processing: {event.get('type', 'unknown')}")
|
||||
```
|
||||
|
||||
The existing `logger.info()` calls already flow to journald via stderr when the systemd unit uses `StandardError=journal`. Using raw `print()` bypasses:
|
||||
- Log level filtering
|
||||
- Format consistency with other log output
|
||||
- Future structured logging needs
|
||||
|
||||
**Fix:** Replace with `logger.info(f"[ZULIP_EVENT] Processing: {event.get('type', 'unknown')}")`.
|
||||
|
||||
---
|
||||
|
||||
### Comment 3 (🟡 High): `asyncio.new_event_loop()` leaks on thread restart
|
||||
|
||||
In `_event_loop`, a new event loop is created but never closed:
|
||||
|
||||
```python
|
||||
def _event_loop(self) -> None:
|
||||
loop = asyncio.new_event_loop()
|
||||
asyncio.set_event_loop(loop)
|
||||
try:
|
||||
self._client.call_on_each_message(...)
|
||||
except Exception as e:
|
||||
logger.error(f"Event loop crashed: {e}")
|
||||
self.connected = False
|
||||
```
|
||||
|
||||
If the thread restarts (e.g., reconnection), the old loop is leaked. This accumulates over time.
|
||||
|
||||
**Fix:** Wrap in `try/finally`:
|
||||
|
||||
```python
|
||||
def _event_loop(self) -> None:
|
||||
loop = asyncio.new_event_loop()
|
||||
asyncio.set_event_loop(loop)
|
||||
try:
|
||||
self._client.call_on_each_message(
|
||||
lambda event: self._process_event(event),
|
||||
)
|
||||
except Exception as e:
|
||||
logger.error(f"Event loop crashed: {e}")
|
||||
self.connected = False
|
||||
finally:
|
||||
loop.close()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Comment 4 (🟡 Medium): Verify `Client(site=...)` parameter name
|
||||
|
||||
The PR changes `Client(server_url=...)` → `Client(site=...)` for "Python 3.13 compatibility." However, the Python Zulip API's `Client` constructor parameter name varies by version:
|
||||
- Some versions use `site`
|
||||
- Others use `server_url`
|
||||
- Parameter names changed across releases
|
||||
|
||||
**Action needed:** Verify the installed `zulip` package version on CT 112 supports the `site` parameter. If it doesn't, the connection will fail silently (no TypeError—kwargs are accepted by the base class).
|
||||
|
||||
---
|
||||
|
||||
### Comment 5 (🟡 Medium): deploy.sh refactor only tested on Hermes platform
|
||||
|
||||
The PR refactors `deploy.sh` across all 3 platforms (Hermes, Agent Zero, Pi) but testing only covers Tanko (Hermes) on CT 112. The Agent Zero (`pip install -r requirements.txt`) and Pi (`/reload` instead of `systemctl`) code paths are untested.
|
||||
|
||||
**Action needed:** Before merging, run at minimum a dry-run deploy against all 3 platform types:
|
||||
```
|
||||
./scripts/deploy.sh --ct=kagentz main --dry-run
|
||||
./scripts/deploy.sh --ct=abiba main --dry-run
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Comment 6 (🟢 Low): Agent Zero pip install assumption
|
||||
|
||||
The deploy.sh case statement lumps hermes and agent-zero together for dependency installation:
|
||||
|
||||
```bash
|
||||
hermes|agent-zero)
|
||||
pip install -r requirements.txt --quiet
|
||||
;;
|
||||
```
|
||||
|
||||
This assumes Agent Zero has the same `requirements.txt` location and content as Hermes. If Agent Zero uses a different dependency file or install method, this will silently install wrong packages.
|
||||
|
||||
**Suggestion:** Add a per-platform dependency install path or document this assumption explicitly.
|
||||
Reference in New Issue
Block a user