fix(agent-team): harden listener respawn/close, broaden handle_event guard, channel_ref partial-unique #29

Merged

amoussa1229 merged 2 commits from fix/agent-team-listener-robustness into main

2026-06-22 20:14:23 +00:00

amoussa1229 commented

2026-06-22 20:09:38 +00:00

(Migrated from github.com)

Three robustness hardening fixes to the agent-team listener/coordinator/ledger, surfaced by earlier /sh-security-review passes.

Changes

Respawn close() leak (CWE-772) — coordinator.py: _supervise_slack_listener closes the dead listener (_close_dead_listener) before respawning, and _run_listener closes in a finally (binding a local first so a concurrent respawn can't close the wrong handle). Stops Socket Mode socket/thread leaks on repeated flaps.
Broaden handle_event guard — slack_listener.py: a except Exception (after the existing except ValueError) logs WARNING (exc_info) and returns None, so a KeyError/sqlite3.Error from _question_turn/_question_thread can't escape and kill the listener thread. Authorization still runs first; never fabricates acceptance.
channel_ref partial-unique — db/schema.py/schema.sql: UNIQUE INDEX ... ON pending_questions(channel_ref) WHERE channel_ref IS NOT NULL AND status='open', in init_db + migrate (idempotent). Defense-in-depth so two open rows can never share a channel_ref.

Verification

/sh-security-review: PASS — detector fan-out (authz/injection/logic) zero findings. Auth-first preserved; broad except returns None only; close-before-respawn race-safe + idempotent; index consistent with all notify/reopen/redeliver paths; DDL static; no log leak (literal message, exc_info exposes no answer content/tokens).
ruff check . + ruff format --check . clean; pytest (agent-team) 968 passed (+8 regression tests: respawn-close, crash-finally-close, KeyError/sqlite3.Error swallow, partial-unique reject/allow-nulls/ignore-closed/migrate).

Migration note

migrate() installs the partial index unconditionally (idempotent IF NOT EXISTS) so existing v1 DBs get it. It would raise IntegrityError loudly only on a legacy DB holding a genuine duplicate open channel_ref (none expected; this is fail-loud, not fail-silent).

Non-blocking follow-up (from the review)

If a post-accept helper raises after the CAS flip but before enqueue_resume, the broad except swallows it → the answer is stuck until the next recover() sweep. Worth a small follow-up (enqueue before the helper calls, or recover-on-tick).

Three robustness hardening fixes to the agent-team listener/coordinator/ledger, surfaced by earlier `/sh-security-review` passes. ### Changes 1. **Respawn `close()` leak (CWE-772)** — `coordinator.py`: `_supervise_slack_listener` closes the dead listener (`_close_dead_listener`) before respawning, and `_run_listener` closes in a `finally` (binding a local first so a concurrent respawn can't close the wrong handle). Stops Socket Mode socket/thread leaks on repeated flaps. 2. **Broaden `handle_event` guard** — `slack_listener.py`: a `except Exception` (after the existing `except ValueError`) logs WARNING (`exc_info`) and returns `None`, so a `KeyError`/`sqlite3.Error` from `_question_turn`/`_question_thread` can't escape and kill the listener thread. Authorization still runs first; never fabricates acceptance. 3. **`channel_ref` partial-unique** — `db/schema.py`/`schema.sql`: `UNIQUE INDEX ... ON pending_questions(channel_ref) WHERE channel_ref IS NOT NULL AND status='open'`, in `init_db` + `migrate` (idempotent). Defense-in-depth so two open rows can never share a channel_ref. ### Verification - `/sh-security-review`: **PASS** — detector fan-out (authz/injection/logic) zero findings. Auth-first preserved; broad except returns None only; close-before-respawn race-safe + idempotent; index consistent with all notify/reopen/redeliver paths; DDL static; no log leak (literal message, exc_info exposes no answer content/tokens). - `ruff check .` + `ruff format --check .` clean; `pytest` (agent-team) **968 passed** (+8 regression tests: respawn-close, crash-finally-close, KeyError/sqlite3.Error swallow, partial-unique reject/allow-nulls/ignore-closed/migrate). ### Migration note `migrate()` installs the partial index unconditionally (idempotent `IF NOT EXISTS`) so existing v1 DBs get it. It would raise `IntegrityError` loudly only on a legacy DB holding a genuine duplicate open channel_ref (none expected; this is fail-loud, not fail-silent). ### Non-blocking follow-up (from the review) If a post-accept helper raises after the CAS flip but before `enqueue_resume`, the broad except swallows it → the answer is stuck until the next `recover()` sweep. Worth a small follow-up (enqueue before the helper calls, or recover-on-tick).

This repo is archived. You cannot comment on pull requests.

No reviewers