Skip to content

feat(kanban): config-gated auto-subscribe on kanban_create with TUI support - #28720

Closed
flooryyyy wants to merge 1 commit into
NousResearch:mainfrom
flooryyyy:feat/kanban-auto-subscribe
Closed

feat(kanban): config-gated auto-subscribe on kanban_create with TUI support#28720
flooryyyy wants to merge 1 commit into
NousResearch:mainfrom
flooryyyy:feat/kanban-auto-subscribe

Conversation

@flooryyyy

@flooryyyy flooryyyy commented May 19, 2026

Copy link
Copy Markdown
Contributor

Summary

kanban_create now auto-subscribes the calling session to completion / block events for the new task, but only when the call has a persistent delivery channel. Gated by a new kanban.auto_subscribe_on_create config flag (default true).

This is a rewrite of the original 1-commit / 34-line proposal in response to the review feedback on the previous version (and the prior history of #19718#19721#19864). Both blocking concerns are addressed.

What changed

  • Config gate kanban.auto_subscribe_on_create in ~/.hermes/config.yaml (default true). Users who want explicit kanban_notify-subscribe calls per task can set it to false. This is what the prior reviewer flagged as the blocker, and the design call that originally got feat(kanban): auto-subscribe gateway chat on tool-driven kanban_create #19718 reverted.
  • Logged exceptions, not except: pass. The notify sub write is still swallowed at the boundary (a bookkeeping failure must never fail a kanban_create mid-conversation) but the failure is no longer invisible. The parent create still succeeds with subscribed=False.
  • TUI / desktop fallback path. TUI clears the platform / chat_id ContextVars (single local channel, not multi-tenant chat) but exports HERMES_SESSION_KEY to subprocesses. We write a row with platform="tui" + chat_id=<session_key> for the TUI notification poller. Real local bug from deployed integration, was missing from the original PR.
  • HERMES_SESSION_ID is not a fallback channel on purpose. It is set by ACP for telemetry regardless of CLI / TUI parent, so using it as a notification target would auto-subscribe every CLI invocation. That re-introduces the over-eager behaviour that got feat(kanban): auto-subscribe gateway chat on tool-driven kanban_create #19718 reverted.
  • subscribed: bool in the kanban_create response so orchestrators can fall back to explicit kanban_notify-subscribe or to polling when the sub fails.
  • Rebased onto current upstream/main. Was 2981 commits behind.

Subscription paths

Path Trigger Row written
Gateway (telegram / discord / slack / ...) HERMES_SESSION_PLATFORM + HERMES_SESSION_CHAT_ID ContextVars set platform=<gateway>, chat_id=<chat_id>
TUI (herm / desktop) HERMES_SESSION_KEY in env, platform / chat_id ContextVars empty platform="tui", chat_id=<session_key>
CLI / cron / test No delivery channel vars none
Config gate disabled Any of the above none

Tests

6 new tests in tests/tools/test_kanban_tools.py:

  • test_create_subscribes_gateway_session
  • test_create_subscribes_tui_session_via_session_key
  • test_create_does_not_subscribe_in_cli_session
  • test_create_respects_auto_subscribe_on_create_false
  • test_create_partial_session_context_no_subscribe
  • test_maybe_auto_subscribe_swallows_add_notify_sub_failure

All 90 tests in the file pass. 509 broader kanban tests pass. Real-import, real-DB tests with HERMES_HOME isolation per the project conftest pattern.

Files changed

  • tools/kanban_tools.py: added _maybe_auto_subscribe(), wired into _handle_create, added subscribed to the response.
  • hermes_cli/config.py: added kanban.auto_subscribe_on_create to DEFAULT_CONFIG.
  • tests/tools/test_kanban_tools.py: 6 new tests plus 2 helpers.
  • website/docs/user-guide/features/kanban.md: config knob table row.
  • website/i18n/zh-Hans/.../kanban.md: same row, translated.

Diff: +306 / -0 across 5 files. New commit is 5ce8f3cda.

Open question for the maintainers

Is feat/kanban-standing the right long-term home for _maybe_auto_subscribe, or should it stay in tools/kanban_tools.py for parity with the slash-command path? Happy to move it.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets labels May 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

⚠️ Duplicate / related cluster — This feature (kanban_create auto-subscribe) was previously implemented in #19718, then reverted in #19721, and replaced by explicit dashboard notification toggles in #19864. At least 7 prior PRs attempted the same (#21523, #22190, #24307, #25195, #25357, #27064, #23223) — all closed.

Open competing PR: #28331 also includes auto-subscribe on create.

Maintainer should decide whether the revert+replacement in #19721/#19864 was a deliberate design choice or if this approach is now acceptable.

@flooryyyy

Copy link
Copy Markdown
Contributor Author

i think it would be significantly beneficial to have agents auto-subscribe to kanban tasks if they're the ones dispatching it. maybe config.yaml option for changing this behaviour? previous attempts shows demand.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused patch. I verified the premise against current main, and the missing behavior is real in the narrow sense: tools/kanban_tools.py:795 creates a task and currently returns only task_id/status, while /kanban create still has its own auto-subscribe path at gateway/slash_commands.py:349.

Problems

  • This exact design was deliberately reverted. Commit 3fb3552 says tool-driven kanban_create should not mirror the slash-command path and that orchestrators should subscribe explicitly instead. PR #19864 then replaced the reverted implicit behavior with explicit dashboard home-channel toggles.
  • The PR adds unconditional implicit subscription but no config gate. The PR discussion mentions a possible config.yaml option; AGENTS.md:102 says behavioral settings should live in config.yaml, not as implicit/uncontrolled behavior.
  • There are no regression tests in the diff. Current tests/tools/test_kanban_tools.py has no coverage for _maybe_auto_subscribe, gateway session context, partial context, or no-op CLI sessions.

Suggested changes

  • If maintainers want to revisit this behavior, make the implicit subscription an explicit config.yaml-controlled option rather than always-on.
  • Add tests for gateway context subscribe, CLI/no-context no-op, partial context no-op, and notifier_profile/board correctness.
  • Consider returning a subscribed boolean, as #19718 did, so the calling agent can tell whether the subscription was created.

This is an automated hermes-sweeper review.

Comment thread tools/kanban_tools.py
@@ -654,6 +654,11 @@ def _handle_create(args: dict, **kw) -> str:
created_by=os.environ.get("HERMES_PROFILE") or "worker",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes tool-driven kanban_create implicitly subscribe again, but commit 3fb3552 explicitly reverted that behavior. If this is revived, it likely needs an explicit config.yaml gate rather than unconditional behavior.

Comment thread tools/kanban_tools.py
@@ -667,6 +672,35 @@ def _handle_create(args: dict, **kw) -> str:
return tool_error(f"kanban_create: {e}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silently swallowing every exception makes subscription failures impossible to diagnose. If this remains best-effort, log at debug with exc_info=True like the earlier #19718 implementation did.

@flooryyyy

Copy link
Copy Markdown
Contributor Author

forgot to sync local changes to this PR. will update soon

When a worker calls kanban_create from inside a session that has a
persistent delivery channel, the originating session is now subscribed
to the new task's completion/block events automatically. The agent
that dispatched the task gets notified instead of having to poll.

- Gateway sessions (telegram/discord/slack): HERMES_SESSION_PLATFORM +
  HERMES_SESSION_CHAT_ID ContextVars, set by the messaging gateway.
- TUI / desktop sessions: HERMES_SESSION_KEY in the subprocess env.
  The TUI notification poller keys on platform='tui' + chat_id=<key>.
- CLI / cron / test: no persistent channel, no subscription.

Gated by kanban.auto_subscribe_on_create in config.yaml (default True).
Disable to mirror pre-feature behaviour — users who want explicit
kanban_notify-subscribe calls per task can set it to false. This
config gate addresses the design concern that got PR NousResearch#19718 reverted
upstream (unconditional implicit auto-subscribe on tool-driven
kanban_create was too aggressive for orchestrator users).

HERMES_SESSION_ID is intentionally not a fallback channel — it is
set by ACP/agent subprocess telemetry for every invocation, not just
TUI, so treating it as a notification target would auto-subscribe
every CLI session and re-introduce the over-eager behaviour.

The kanban_create response now includes a 'subscribed' bool so
orchestrators can react if subscription failed (e.g. by falling
back to explicit kanban_notify-subscribe or to polling).

Includes 6 tests covering the gateway / TUI / CLI / partial-context /
gated / add_notify_sub-failure paths. All 90 tests in
test_kanban_tools.py pass; 509 broader kanban tests pass.
@flooryyyy
flooryyyy force-pushed the feat/kanban-auto-subscribe branch from 9d78ba4 to 5ce8f3c Compare June 15, 2026 17:27
@flooryyyy flooryyyy changed the title feat: auto-subscribe kanban_create callers to task completion notifications feat(kanban): config-gated auto-subscribe on kanban_create with TUI support Jun 15, 2026
@flooryyyy

Copy link
Copy Markdown
Contributor Author

Rewrote this from scratch in response to the review feedback (and the #19718#19721#19864 history). Both blocking concerns addressed:

1. Unconditional behaviour (your Issue 1)

Added kanban.auto_subscribe_on_create to DEFAULT_CONFIG (default true). Users who want explicit kanban_notify-subscribe calls per task can flip it to false in ~/.hermes/config.yaml. This complements the per-platform toggles from #19864 rather than fighting them — the gate is "should the dispatcher auto-fill the sub table on tool-driven creates", the dashboard toggles are "should the watcher deliver to this channel at all".

2. Silent exception swallowing (your Issue 2)

Replaced except: pass with a logger.warning(..., _exc, platform, key_set) call. The exception is still swallowed at the boundary (a notify bookkeeping failure must never fail a kanban_create that's mid-conversation) but it's no longer invisible.

A few things added beyond the original PR while I was in there:

  • TUI / desktop fallback path. TUI clears the platform/chat_id ContextVars (single local channel, not multi-tenant chat) but exports HERMES_SESSION_KEY to subprocesses. The TUI notification poller keys on platform="tui" + chat_id=<session_key>, so the function writes that row shape. Was a real local bug from deployed integration.
  • HERMES_SESSION_ID is intentionally not a fallback channel. It's set by ACP for telemetry regardless of CLI/TUI parent, so using it as a notification target would auto-subscribe every CLI invocation and re-introduce the over-eager behaviour that got feat(kanban): auto-subscribe gateway chat on tool-driven kanban_create #19718 reverted.
  • subscribed: bool added to the kanban_create response so orchestrators can fall back to explicit kanban_notify-subscribe or to polling when the sub fails.
  • Rebased onto current upstream/main (was 2981 commits behind).
  • 6 new tests in tests/tools/test_kanban_tools.py covering gateway / TUI / CLI / partial-ctx / gated / add_notify_sub-failure paths. All 90 tests in the file pass; 509 broader kanban tests pass. Real-import, real-DB tests with HERMES_HOME isolation per the project's conftest pattern.
  • Docs updated in website/docs/user-guide/features/kanban.md (config knob table) and the zh-Hans translation.

Diff: +306 / -0 across 5 files. The new commit is 5ce8f3cda. Happy to split it up or move _maybe_auto_subscribe into feat/kanban-standing if you'd rather it live there for parity.

@flooryyyy

Copy link
Copy Markdown
Contributor Author

i've tried pinging teknium and mentioning this PR in the discord but i was met with radio silence. i've brought this PR up to date with my local changes and i'd additionally add an arg to let the agent control subscriptions on the fly in cases where the agent knows when to subscribe vs not, however with no constructive feedback i can not do anything and therefore i don't think i will contribute in the future seen as this PR has been completely ignored for over a month

i understand everyone is busy, however it is not nice being on the other end either. even a simple "too busy at the moment" would be better than me chasing someone to look at my contribution. i imagine it wouldn't be hard just re-running the sweeper bot either

i'm still ok with finishing this PR and getting it merged but it's my last unless the contribution system improves in the future

cc: @teknium1 @alt-glitch

@alt-glitch

Copy link
Copy Markdown
Collaborator

@daimon-nous review this please!

@daimon-nous

daimon-nous Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Code is clean and the two prior sweeper blockers (config gate + logged exceptions) are genuinely addressed. The remaining question is a design/taste call that belongs to @teknium1, plus two technical items below. I verified everything against current main with real-import E2E.

Premise — confirmed real

_handle_create on main still returns only task_id/status; no auto-subscribe. Not redundant. APIs used (cfg_get, load_config, get_session_env, add_notify_sub) all match current signatures. The 6 new tests pass (6 passed, 84 deselected).

The design decision (for the maintainer, not automatable)

This exact behavior was deliberately reverted in 3fb35520c6 (#19721): "tool-driven kanban_create should not mirror the slash-command path… we're not going to make it implicit", and replaced by explicit dashboard toggles (#19864). This PR re-adds it gated by kanban.auto_subscribe_on_createbut the default is true, so out of the box the reverted implicit behavior is back on. Whether to revisit that call is a taste decision and stays with @teknium1. If the intent is to respect the revert while still offering the feature, the gate should default to false (opt-in), which also matches AGENTS.md "behavioral settings live in config.yaml" without changing the shipped default.

Technical: notifier_profile diverges from the parity path

The slash path (gateway/slash_commands.py:376) writes notifier_profile = _kanban_notifier_profile or self._active_profile_name(). This PR writes os.environ.get("HERMES_PROFILE") (kanban_tools.py:914). For the default profile HERMES_PROFILE is unset, so the row is written with notifier_profile=None.

E2E (default-profile telegram session): ROW notifier_profile = None | platform = telegram | chat_id = chat-999.

Not delivery-breaking — kanban_watchers.py:167-173 only skips a sub when owner_profile and owner_profile != current, so a None row is treated as un-owned and any profile's watcher delivers it. But in a multi-profile setup the auto-subscribed sub is no longer pinned to the originating profile the way the slash path pins it. Recommend resolving the active profile name to match the parity path the PR claims.

Minor

  • kanban_tools.py WARNING logs bool(chat_id) under label key_set= — mislabeled (it's chat-id presence).
  • load_config() does a full file read + YAML parse on every kanban_create; load_config_readonly() is the lighter call here.

On the contribution experience

The rewrite is a real, good-faith response to the sweeper's feedback — config gate, logged exceptions, subscribed flag, TUI fallback, tests, docs (en + zh-Hans), all there. The month of silence wasn't deserved. The only thing genuinely blocking is the human design call above; the two technical items are small and fixable.

Automated review by daimon-nous. The default-true / revert tension and the notifier_profile divergence were verified via real-import E2E; the merge/design decision is deferred to the maintainer.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #48635 — your commit was cherry-picked onto current main with your authorship preserved in git log (commit f8d8f045, author flooryyyy).

#48635

Thanks for the rewrite addressing the config gate and the TUI fallback path — both landed as-is. The 6 new tests and docs (EN + zh-Hans) all came across.

One heads-up on cadence so expectations are calibrated: kanban auto-notification is a low-priority area for us. The tool surface here is narrow and opt-in (only dispatcher-spawned workers and profiles that have explicitly enabled the kanban toolset ever reach kanban_create), so changes in this space don't get prioritized. For future PRs touching kanban notifications, please expect a 1+ month turnaround on review and merge. Not a reflection on contribution quality — just where this corner sits in the queue.

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

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants