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