Skip to content

fix(kanban): surface terminal ACK relay failures - #49752

Open
verybigdog wants to merge 2 commits into
NousResearch:mainfrom
verybigdog:contrib/kanban-ack-relay-status
Open

fix(kanban): surface terminal ACK relay failures#49752
verybigdog wants to merge 2 commits into
NousResearch:mainfrom
verybigdog:contrib/kanban-ack-relay-status

Conversation

@verybigdog

Copy link
Copy Markdown
Contributor

Summary

  • records terminal ACK relay status/delivery problems instead of silently losing missing-subscription failures
  • extends diagnostics/tests for relay failure visibility

Tests

  • python3 -m pytest tests/hermes_cli/test_kanban_diagnostics.py tests/hermes_cli/test_kanban_notify.py -q -o addopts=
  • python3 -m py_compile hermes_cli/kanban_db.py hermes_cli/kanban_diagnostics.py

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have labels Jun 20, 2026
@verybigdog
verybigdog marked this pull request as ready for review June 20, 2026 18:24
@verybigdog
verybigdog force-pushed the contrib/kanban-ack-relay-status branch from 59e5022 to 03b9e60 Compare June 23, 2026 07:50
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for making ACK delivery state visible. The failure needs to be recorded at the delivery boundary rather than inferred from worker handoff text.

Problems

  • complete_task writes the completed event in hermes_cli/kanban_db.py:4146-4150, but delivery occurs later at gateway/kanban_watchers.py:415-418; actual send failures are caught at gateway/kanban_watchers.py:448-476. The proposed completion-time classifier cannot observe those failures.
  • list_notify_subs() is an explicit optional subscription lookup (hermes_cli/kanban_db.py:8530-8539), so an empty result is not evidence of an undelivered ACK.
  • Current tracked source has no producer for the three added failure literals; the tests inject them into completion prose rather than exercising the notifier failure path.

Suggested changes

  • Persist delivery outcomes around adapter.send() and cover retries plus the final subscription-drop path with a failing adapter.
  • Only diagnose a requested notification target that has a recorded delivery failure; do not infer failure from no subscriptions.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@verybigdog

Copy link
Copy Markdown
Contributor Author

Recommend keeping this as the clean, reviewable implementation of the 'delivery-boundary failure receipt' item. It's self-contained: classify_ack_relay writes a durable, once-per-terminal-transition ack_relay_status event and _rule_missing_ack_relay surfaces it as an operator-facing diagnostic with recovery actions — something #37865/main only log at WARNING. No forbidden patterns (no chat_type=group, no model-exposed control, no schema alias); persistence is actually consumed, and the receipt is bounded (matched pattern id + short verdict token + status, no raw summary or secrets). Note this same commit is duplicated inside the larger, unreviewed #49593 (byte-identical kanban_diagnostics.py) — resolve that by dropping the commit from #49593, not by blocking this PR. Suggest rebasing on #37865 so the two land together. Nothing is closed by this note. (This PR is not closed.)

Hermes Agent added 2 commits July 15, 2026 09:58
(cherry picked from commit 2181f11f229ac8f3883323c737e8e5310fa47305)
(cherry picked from commit 3fc98272507a64dae29fa00644a6237ea14ed0d9)
@verybigdog
verybigdog force-pushed the contrib/kanban-ack-relay-status branch from 03b9e60 to d0b8453 Compare July 15, 2026 10:11
@verybigdog

Copy link
Copy Markdown
Contributor Author

M28.6 refresh pushed: d0b84539ff (fix(kanban): persist ACK delivery failures at notifier boundary).

Evidence / changes:

  • Removed completion-time ACK classification from complete_task(); no more inference from summary/result/metadata prose or empty kanban_notify_subs.
  • Added sanitized notify_delivery_status events written by the gateway notifier at the actual adapter.send boundary: retry_failure, delivered, terminal_failure, subscription_dropped.
  • Diagnostics now fire only when an explicitly requested notification target has a recorded terminal_failure; retry-then-success and no-target/no-failure cases stay silent.
  • Durable diagnostic payload avoids raw summary/result/exception text and raw chat/thread IDs; target is stored as a short SHA-256 fingerprint plus platform/thread_present/outcome counters.
  • Preserved existing routing/profile adapter selection path (_authorization_adapter) and left wake/session semantics untouched.

Tests:

  • scripts/run_tests.sh tests/hermes_cli/test_kanban_notify.py tests/hermes_cli/test_kanban_diagnostics.py → 67 passed.
  • scripts/run_tests.sh tests/hermes_cli/test_kanban_notify.py tests/hermes_cli/test_kanban_diagnostics.py tests/gateway/test_kanban_notifier.py tests/gateway/test_kanban_notifier_watcher_dispatch_gate.py tests/gateway/test_kanban_watchers_mixin.py tests/hermes_cli/test_kanban_db.py → 311 passed.

Mergeability: PR head is d0b84539ff; gh pr view reports mergeStateStatus: BLOCKED with no status checks currently listed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants