Files

T

Abiba (pi) 1c8f48fd77 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

2026-06-26 22:23:38 +00:00

3.6 KiB

Raw Permalink Blame History

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:

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:

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:

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:

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.

3.6 KiB Raw Permalink Blame History