Abiba (pi)
1c8f48fd77
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
106 lines
3.6 KiB
Markdown
106 lines
3.6 KiB
Markdown
# 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.
|