From 223417a6d7866de001f1e997c7710928823dd5a3 Mon Sep 17 00:00:00 2001 From: Casey West Date: Wed, 24 Jun 2026 13:15:19 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix(kanban):=20subscribe=20confi?= =?UTF-8?q?gured=20report-back=20target=20for=20sessionless=20cards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Webhook-spawned review cards (stage-pr-review via gateway/platforms/ webhook.py -> kanban_create(initial_status='review')) carry no session context: neither HERMES_SESSION_PLATFORM/CHAT_ID nor HERMES_SESSION_KEY is set. So _maybe_auto_subscribe hit the CLI/cron/test branch and wrote no kanban_notify_subs row. With no subscription, when Lamport later completes the review card the existing terminal-kind notifier (which already watches the 'completed' kind) had no target -- Casey never learned a verdict was ready. Add a config-only fallback: when there is no session context, read kanban.report_back_target {platform, chat_id, thread_id?} from config.yaml and, if platform and chat_id are both present, register a notify-sub for it via the same add_notify_sub path the gateway/TUI branches use. The existing 'completed' event then delivers the verdict. No new event kind, no new transition, no dashboard allow-list change, no new core tool, no HERMES_ env var. With the key absent or incomplete the function returns False exactly as before (behavior preserved). Idempotent (INSERT OR IGNORE) and still gated by kanban.auto_subscribe_on_create. Tests: report_back_target fallback subscribes without session context, thread_id optional, no-key preserves current no-sub behavior, incomplete target (platform without chat_id) does not subscribe. --- docs/plans/notifier-report-back.md | 92 ++++++++++++++++++++++ tests/tools/test_kanban_tools.py | 121 +++++++++++++++++++++++++++++ tools/kanban_tools.py | 40 +++++++++- 3 files changed, 252 insertions(+), 1 deletion(-) create mode 100644 docs/plans/notifier-report-back.md diff --git a/docs/plans/notifier-report-back.md b/docs/plans/notifier-report-back.md new file mode 100644 index 000000000000..12e34a0b3cb8 --- /dev/null +++ b/docs/plans/notifier-report-back.md @@ -0,0 +1,92 @@ +# Report-back gap: webhook-spawned cards subscribe a configured target + +Task: t_d3cd25a9 (fork cwest/hermes-agent). Branch `topic/notifier-report-back` +off `origin/cwest/integration`. + +## Problem (final, corrected scope) + +Webhook-spawned review cards (stage-pr-review via `gateway/platforms/webhook.py` +-> `kanban_create(initial_status='review')`) carry **no session context**: +neither `HERMES_SESSION_PLATFORM`/`HERMES_SESSION_CHAT_ID` nor +`HERMES_SESSION_KEY` is set. So `tools/kanban_tools.py::_maybe_auto_subscribe` +hits the `return False` at the CLI/cron/test branch and writes **no** +`kanban_notify_subs` row. With no subscription, when Lamport later **completes** +the review card, the existing notifier (which already watches the `completed` +terminal kind) has no target to deliver to. Casey never learns there's a verdict. + +### What this is NOT (premise correction, confirmed with Casey via Hollis) + +- There is **no into-review transition** to hook an event onto. Review cards are + *born* in `review` (`create_task(initial_status='review')`); they never + transition into it. Verified: no `kanban_db.py` function writes + `status='review'`; the only writers in-tree are test files via raw SQL. +- Therefore **GAP 1 collapses into GAP 2**: the signal Casey needs ("Lamport has + a verdict") is the review card *completing*, which **already** fires the + `completed` event the notifier **already** watches. No new event kind, no new + transition, no dashboard allow-list change, no `kanban.notify_watched_statuses` + config key. Pure bias-to-edges. + +The whole fix = give webhook-spawned cards a subscription via a configured +fallback target. + +## Design (pre-approved shape) + +### Config key (config.yaml only — AGENTS.md: not a HERMES_ env var) + +```yaml +kanban: + report_back_target: + platform: telegram # required for the fallback to fire + chat_id: "-1001234567" # required + thread_id: "42" # optional +``` + +When `platform` AND `chat_id` are both present, webhook/CLI/cron cards (cards +created with no session context) subscribe this target. When the key is absent +or incomplete, behavior is **identical to today** (returns False, no sub). + +### Code change — single site + +`tools/kanban_tools.py::_maybe_auto_subscribe` (the `:909` "CLI / cron / test — +no persistent channel" branch). Before returning False, attempt a config +fallback: + +1. Read `kanban.report_back_target` from the already-loaded `cfg` + (`load_config()` is already called at the top of the function; + `cfg_get` is already imported in-scope). +2. If `platform` and `chat_id` are both truthy, set `platform`/`chat_id`/ + `thread_id` from the configured target (user_id stays None) and fall through + to the existing `add_notify_sub(...)` call. Otherwise keep `return False`. + +This reuses the identical `add_notify_sub(...)` write the gateway/TUI branches +use; the notifier consumes those rows unchanged. + +### Why this is correct / safe + +- **Idempotent**: `add_notify_sub` is `INSERT OR IGNORE` on + (task, platform, chat, thread). +- **Gate preserved**: still short-circuits on + `kanban.auto_subscribe_on_create=false`. +- **No default behavior change**: with no config key set, the function returns + False exactly as before. Verified by keeping the existing tests green. +- **No new core tool, no env var, no cache impact.** + +## Tests (TDD RED -> GREEN) + +`tests/tools/test_kanban_tools.py`: + +1. RED: with `kanban.report_back_target` configured in `config.yaml` and **no** + session env, `kanban_create` (webhook context) returns `subscribed=True` and + writes a `kanban_notify_subs` row matching the configured target. +2. Regression guard: with **no** config key and no session env, `subscribed` + is False and no sub row is written (current behavior preserved). +3. Edge: incomplete target (platform but no chat_id) -> no sub, returns False. + +Full suite to run before claiming done: +`tests/tools/test_kanban_tools.py`, `tests/gateway/test_kanban_notifier.py`, +`tests/hermes_cli/test_kanban_notify.py`. + +## Out of scope (separate child card) + +Retiring the cron poller `team-status-watch.py` (job e6a472e3a604) once this +lands and is cut over. diff --git a/tests/tools/test_kanban_tools.py b/tests/tools/test_kanban_tools.py index e9b41f812bb6..5307a5b7ff75 100644 --- a/tests/tools/test_kanban_tools.py +++ b/tests/tools/test_kanban_tools.py @@ -1027,6 +1027,127 @@ def test_create_rejects_non_list_skills(worker_env): assert json.loads(out).get("error") +# --------------------------------------------------------------------------- +# report_back_target fallback (webhook/CLI/cron cards with no session context) +# --------------------------------------------------------------------------- + +def _clear_session_env(monkeypatch): + """Simulate a webhook-spawned card: no session context whatsoever. + + Webhook stage-pr-review cards run with neither HERMES_SESSION_PLATFORM/ + CHAT_ID (gateway path) nor HERMES_SESSION_KEY (TUI path) set, so + _maybe_auto_subscribe falls through to the CLI/cron/test branch. + """ + for var in ( + "HERMES_SESSION_PLATFORM", + "HERMES_SESSION_CHAT_ID", + "HERMES_SESSION_THREAD_ID", + "HERMES_SESSION_USER_ID", + "HERMES_SESSION_KEY", + ): + monkeypatch.delenv(var, raising=False) + + +def _write_report_back_target(monkeypatch, **fields): + """Write a kanban.report_back_target block into the isolated config.yaml.""" + import os + from pathlib import Path + lines = ["kanban:", " report_back_target:"] + for k, v in fields.items(): + lines.append(f" {k}: \"{v}\"") + home = Path(os.environ["HERMES_HOME"]) + (home / "config.yaml").write_text("\n".join(lines) + "\n", encoding="utf-8") + + +def test_create_subscribes_report_back_target_without_session( + monkeypatch, worker_env, +): + """A webhook-spawned card (no session env) subscribes the configured + kanban.report_back_target so the existing notifier can deliver the + review-card completion to Casey's thread.""" + _clear_session_env(monkeypatch) + _write_report_back_target( + monkeypatch, platform="telegram", chat_id="-1009999", thread_id="42", + ) + from tools import kanban_tools as kt + from hermes_cli import kanban_db as kb + + out = kt._handle_create({"title": "review card", "assignee": "lamport"}) + d = json.loads(out) + assert d["ok"] is True + assert d["subscribed"] is True, "expected report_back_target fallback to subscribe" + + conn = kb.connect() + try: + subs = kb.list_notify_subs(conn, d["task_id"]) + finally: + conn.close() + assert len(subs) == 1, subs + sub = subs[0] + assert sub["platform"] == "telegram" + assert sub["chat_id"] == "-1009999" + assert sub["thread_id"] == "42" + + +def test_create_report_back_target_thread_optional(monkeypatch, worker_env): + """thread_id is optional in the configured target.""" + _clear_session_env(monkeypatch) + _write_report_back_target(monkeypatch, platform="discord", chat_id="chan-1") + from tools import kanban_tools as kt + from hermes_cli import kanban_db as kb + + d = json.loads(kt._handle_create({"title": "rc", "assignee": "lamport"})) + assert d["subscribed"] is True + conn = kb.connect() + try: + subs = kb.list_notify_subs(conn, d["task_id"]) + finally: + conn.close() + assert len(subs) == 1 + assert subs[0]["platform"] == "discord" + assert subs[0]["chat_id"] == "chan-1" + assert subs[0]["thread_id"] in ("", None) + + +def test_create_no_report_back_target_preserves_current_behavior( + monkeypatch, worker_env, +): + """With no config key and no session env, behavior is unchanged: no + subscription is written and subscribed is False.""" + _clear_session_env(monkeypatch) + # No config.yaml report_back_target written. + from tools import kanban_tools as kt + from hermes_cli import kanban_db as kb + + d = json.loads(kt._handle_create({"title": "rc", "assignee": "lamport"})) + assert d["subscribed"] is False + conn = kb.connect() + try: + subs = kb.list_notify_subs(conn, d["task_id"]) + finally: + conn.close() + assert subs == [] + + +def test_create_incomplete_report_back_target_no_subscription( + monkeypatch, worker_env, +): + """An incomplete target (platform but no chat_id) does not subscribe.""" + _clear_session_env(monkeypatch) + _write_report_back_target(monkeypatch, platform="telegram") # no chat_id + from tools import kanban_tools as kt + from hermes_cli import kanban_db as kb + + d = json.loads(kt._handle_create({"title": "rc", "assignee": "lamport"})) + assert d["subscribed"] is False + conn = kb.connect() + try: + subs = kb.list_notify_subs(conn, d["task_id"]) + finally: + conn.close() + assert subs == [] + + def test_link_happy_path(worker_env): from hermes_cli import kanban_db as kb conn = kb.connect() diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 15988bcba897..1e715d65212c 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -872,6 +872,7 @@ def _maybe_auto_subscribe(conn: Any, task_id: str) -> bool: We never want a notification bookkeeping failure to fail the kanban_create that the agent is mid-conversation about. """ + cfg: Any = None try: cfg = load_config() if not cfg_get(cfg, "kanban", "auto_subscribe_on_create", default=True): @@ -906,7 +907,44 @@ def _maybe_auto_subscribe(conn: Any, task_id: str) -> bool: or os.environ.get("HERMES_SESSION_KEY", "") ) if not session_key: - return False # CLI / cron / test — no persistent channel + # CLI / cron / webhook — no per-session delivery channel. + # Fall back to a statically configured report-back target so + # cards created without session context (notably the webhook + # stage-pr-review review cards spawned via + # gateway/platforms/webhook.py) still subscribe a chat. When + # Lamport later completes the review card, the existing + # terminal-kind notifier delivers the verdict to this target. + # + # Config (config.yaml only — never a HERMES_ env var, per + # AGENTS.md, since this is behavioral config, not a secret): + # + # kanban: + # report_back_target: + # platform: telegram + # chat_id: "-1001234567" + # thread_id: "42" # optional + # + # Absent or incomplete (missing platform or chat_id) -> no + # subscription, identical to the pre-fallback behaviour. + rbt = cfg_get(cfg, "kanban", "report_back_target", default=None) + rbt = rbt if isinstance(rbt, dict) else {} + rbt_platform = str(rbt.get("platform") or "").strip() + rbt_chat_id = str(rbt.get("chat_id") or "").strip() + if not (rbt_platform and rbt_chat_id): + return False # CLI / cron / test — no persistent channel + platform = rbt_platform + chat_id = rbt_chat_id + rbt_thread = rbt.get("thread_id") + thread_id = str(rbt_thread).strip() or None if rbt_thread else None + user_id = None + from hermes_cli import kanban_db as _kb + _kb.add_notify_sub( + conn, task_id=task_id, + platform=platform, chat_id=chat_id, + thread_id=thread_id, user_id=user_id, + notifier_profile=os.environ.get("HERMES_PROFILE"), + ) + return True platform = "tui" chat_id = session_key thread_id = get_session_env("HERMES_SESSION_THREAD_ID", "") or None