From 923960fa4ba91afefc45182b5d91b5adcfd59e30 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 18:09:03 -0700 Subject: [PATCH] fix(kanban): dispatch_profiles is config-only and fail-closed; trim tests Follow-up to the salvaged #111004 commit, aligning it with the shape agreed on #110995: - Drop the HERMES_KANBAN_DISPATCH_PROFILES env bridge: non-secret behaviour lives in config.yaml only, like every other kanban.* key. - Read the key via load_config_readonly() with the same fail-open config read as the sibling kanban.* readers (configured_max_in_progress). - Fail closed when the key is set: the "none" sentinel is gone (an empty list already claims nothing), and an assignee that is not a valid profile id is never claimable instead of being lower-cased into the allowlist. - Trim the regression file to two invariants (allowlist without `default` buckets the card as nonspawnable AND turns has_spawnable_ready off; unset key keeps upstream behaviour). Both drive the real dispatch tick against a real config.yaml + kanban.db; the first is red on origin/main. - Docs: move the "Shared boards across homes" section out of the gateway-dispatcher paragraph, state that `default` collides by construction, add the config-reference row. --- hermes_cli/config_defaults.py | 11 +-- hermes_cli/kanban_db_dispatch.py | 54 ++++------- .../test_kanban_dispatch_claim_allowlist.py | 91 ++++++++----------- website/docs/user-guide/features/kanban.md | 32 ++++--- 4 files changed, 77 insertions(+), 111 deletions(-) diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index 637d0eead4..a20e963ad0 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -1771,12 +1771,11 @@ DEFAULT_CONFIG = { # fan-out workflows that would otherwise saturate one profile's local model / API quota / browser # pool while leaving other profiles idle. See #21582. "max_in_progress_per_profile": None, - # Per-home claim allowlist for shared boards (#110995): profile names this - # home's dispatcher may claim, as a list or comma-separated string. Unset - # (None) = any existing profile is claimable (upstream behavior); - # ["none"] = claim nothing. On a shared kanban.db, every home's - # profile_exists("default") is True, so without this each home can claim - # default-assigned cards. Runtime override: HERMES_KANBAN_DISPATCH_PROFILES. + # Per-home claim allowlist for boards shared across Hermes homes (#110995): profile names + # this home's dispatcher may claim (list or comma-separated string). None = any existing + # profile is claimable. Set = fail-closed (an empty list claims nothing). Every home has a + # root profile named "default", so on a shared kanban.db every home can otherwise claim + # default-assigned cards. "dispatch_profiles": None, # Auto-run the decomposer on Triage tasks every tick. False = manual via `hermes kanban # decompose ` or the dashboard's Decompose button. diff --git a/hermes_cli/kanban_db_dispatch.py b/hermes_cli/kanban_db_dispatch.py index c7c91e58e7..d443ce147b 100644 --- a/hermes_cli/kanban_db_dispatch.py +++ b/hermes_cli/kanban_db_dispatch.py @@ -1220,11 +1220,10 @@ def _profile_exists_fn() -> Optional[Callable[[str], bool]]: imported (local import avoids a cycle; callers fall back to trusting the assignee). - When a per-home claim allowlist is configured (``kanban.dispatch_profiles`` - or ``HERMES_KANBAN_DISPATCH_PROFILES``, #110995), the returned predicate - additionally requires the assignee to be listed — so a card assigned to - ``default`` is only claimable by homes that opted into it. Foreign - assignees land in the existing ``skipped_nonspawnable`` bucket. + When ``kanban.dispatch_profiles`` is set (#110995) the returned predicate + additionally requires the assignee to be listed, fail-closed — so a card + assigned to ``default`` is only claimable by homes that opted into it. + Foreign assignees land in the existing ``skipped_nonspawnable`` bucket. """ try: from hermes_cli.profiles import normalize_profile_name, profile_exists @@ -1238,61 +1237,42 @@ def _profile_exists_fn() -> Optional[Callable[[str], bool]]: try: canon = normalize_profile_name(name) except ValueError: - canon = (name or "").strip().lower() + return False return canon in allowlist and bool(profile_exists(name)) return _gated -# Env-var bridge for the per-home kanban dispatch claim allowlist (#110995). -# Non-secret behavioral settings live in config.yaml; this is the fleet-friendly -# runtime override (containers set env per home more easily than per-home -# config.yaml edits), mirroring terminal.cwd -> TERMINAL_CWD. -KANBAN_DISPATCH_PROFILES_ENV = "HERMES_KANBAN_DISPATCH_PROFILES" - - def _dispatch_profile_allowlist(normalize_profile_name) -> Optional[frozenset]: - """Per-home claim allowlist for the kanban dispatcher (#110995). + """Per-home claim allowlist ``kanban.dispatch_profiles`` (#110995). On a shared board (one ``kanban.db`` mounted across several Hermes homes), every home's ``profile_exists`` returns True for ``default`` — the root profile every home has — so a card assigned to ``default`` is claimable by every home's dispatcher. A home opts out of foreign claims by declaring - which assignees it may claim, canonically in config.yaml: + which assignees it may claim:: kanban: dispatch_profiles: ["sage", "researcher"] # or "sage,researcher" - (``HERMES_KANBAN_DISPATCH_PROFILES`` overrides config at runtime.) - - Returns ``None`` when neither is set (upstream behavior: any existing - profile is claimable). The special value ``none`` (or an empty value) - yields an empty allowlist — the home claims nothing. + Returns ``None`` when the key is unset (upstream behavior: any existing + profile is claimable). A set value is fail-closed: an empty list claims + nothing. Config read is fail-open like the sibling ``kanban.*`` readers. """ - raw = os.environ.get(KANBAN_DISPATCH_PROFILES_ENV) - if raw is None: - try: - from hermes_cli.config import load_config - cfg = load_config() - kanban_cfg = cfg.get("kanban", {}) if isinstance(cfg, dict) else {} - raw = kanban_cfg.get("dispatch_profiles") - except Exception: - return None + try: + from hermes_cli.config import load_config_readonly + raw = (load_config_readonly() or {}).get("kanban", {}).get("dispatch_profiles") + except Exception: + return None if raw is None: return None - if isinstance(raw, (list, tuple)): - names = [str(n) for n in raw] - else: - names = str(raw).split(",") - names = [n.strip() for n in names if n.strip()] - if not names or all(n.casefold() == "none" for n in names): - return frozenset() + names = [str(n) for n in raw] if isinstance(raw, (list, tuple)) else str(raw).split(",") allowed = set() for n in names: try: allowed.add(normalize_profile_name(n)) except ValueError: - allowed.add(n.strip().lower()) + continue return frozenset(allowed) diff --git a/tests/hermes_cli/test_kanban_dispatch_claim_allowlist.py b/tests/hermes_cli/test_kanban_dispatch_claim_allowlist.py index 3a6ab905b8..1b869c4185 100644 --- a/tests/hermes_cli/test_kanban_dispatch_claim_allowlist.py +++ b/tests/hermes_cli/test_kanban_dispatch_claim_allowlist.py @@ -1,69 +1,54 @@ -"""Per-home kanban dispatch claim allowlist. +"""kanban.dispatch_profiles: per-home claim allowlist for shared boards (#110995). -Regression tests for #110995: on a shared kanban board (one kanban.db mounted -across several Hermes homes) every home's ``profile_exists("default")`` is -unconditionally True, so any home's dispatcher could claim cards assigned to -``default``. ``kanban.dispatch_profiles`` (or the -``HERMES_KANBAN_DISPATCH_PROFILES`` env bridge) declares which assignees this -home may claim; anything else lands in ``skipped_nonspawnable``. +On a board shared across Hermes homes (one kanban.db mounted in several +containers) every home's ``profile_exists("default")`` is True, so any home's +dispatcher could claim cards assigned to ``default``. The allowlist wraps the +same predicate consumed by the spawn gate and the spawnable telemetry, so a +foreign assignee lands in ``skipped_nonspawnable`` and does not keep the +gateway wake-up loop hot. """ from __future__ import annotations -import os from pathlib import Path import pytest +from hermes_cli import kanban_db as kb +from hermes_cli import kanban_db_connect as kbc from hermes_cli import kanban_db_dispatch as kbd @pytest.fixture -def every_home_has_default(monkeypatch): - """Reproduce the incident premise: ``profile_exists("default")`` is True.""" - from hermes_cli import profiles - monkeypatch.setattr(profiles, "profile_exists", lambda name: True) +def kanban_home(tmp_path, monkeypatch): + """Isolated HERMES_HOME with an empty kanban DB.""" + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + kb.init_db() + return home -# Env bridge for the claim allowlist (mirrors -# kanban_db_dispatch.KANBAN_DISPATCH_PROFILES_ENV; spelled out so the tests -# stay red-on-base against code that lacks the constant). -_DISPATCH_PROFILES_ENV = "HERMES_KANBAN_DISPATCH_PROFILES" +def test_allowlist_without_default_skips_default_card(kanban_home, all_assignees_spawnable): + """Fail-closed: the allowlist is set and ``default`` is not on it, so the + card is neither spawnable (wake-up telemetry) nor claimed (spawn gate), + even though every profile exists locally.""" + (kanban_home / "config.yaml").write_text( + "kanban:\n dispatch_profiles:\n - sage\n", encoding="utf-8", + ) + with kbc.connect() as conn: + tid = kb.create_task(conn, title="foreign card", assignee="default") + assert kbd.has_spawnable_ready(conn) is False + res = kbd.dispatch_once(conn, dry_run=True) + assert res.spawned == [] + assert res.skipped_nonspawnable == [tid] -@pytest.fixture -def no_env_bridge(monkeypatch): - monkeypatch.delenv(_DISPATCH_PROFILES_ENV, raising=False) - - -def test_allowlist_env_restricts_default_claim(monkeypatch, every_home_has_default): - monkeypatch.setenv(_DISPATCH_PROFILES_ENV, "sage,researcher") - claim = kbd._profile_exists_fn() - assert claim is not None - assert claim("sage") is True - assert claim("researcher") is True - # The incident: this home must NOT claim another home's "default". - assert claim("default") is False - - -def test_allowlist_env_none_claims_nothing(monkeypatch, every_home_has_default): - monkeypatch.setenv(_DISPATCH_PROFILES_ENV, "none") - claim = kbd._profile_exists_fn() - assert claim is not None - assert claim("sage") is False - assert claim("default") is False - - -def test_allowlist_config_key_end_to_end(every_home_has_default, no_env_bridge): - """Real config.yaml -> real load_config() -> gated predicate.""" - home = Path(os.environ["HERMES_HOME"]) - (home / "config.yaml").write_text("kanban:\n dispatch_profiles:\n - sage\n") - claim = kbd._profile_exists_fn() - assert claim is not None - assert claim("sage") is True - assert claim("default") is False - - -def test_unset_allowlist_preserves_upstream(every_home_has_default, no_env_bridge): - claim = kbd._profile_exists_fn() - assert claim is not None - assert claim("default") is True +def test_unset_allowlist_keeps_default_claimable(kanban_home, all_assignees_spawnable): + """No key = upstream behaviour: any existing profile, ``default`` included.""" + with kbc.connect() as conn: + tid = kb.create_task(conn, title="local card", assignee="default") + assert kbd.has_spawnable_ready(conn) is True + res = kbd.dispatch_once(conn, dry_run=True) + assert [t for t, _a, _w in res.spawned] == [tid] + assert res.skipped_nonspawnable == [] diff --git a/website/docs/user-guide/features/kanban.md b/website/docs/user-guide/features/kanban.md index ba314bdc26..50c7325640 100644 --- a/website/docs/user-guide/features/kanban.md +++ b/website/docs/user-guide/features/kanban.md @@ -285,23 +285,10 @@ kanban: dispatch_profiles: null # default: this home may claim cards for any # existing profile. Set to a list (or # comma-separated string) of profile names to - # restrict which assignees this home claims. - # ["none"] claims nothing. Override at - # runtime with HERMES_KANBAN_DISPATCH_PROFILES. + # restrict which assignees this home claims; + # fail-closed, an empty list claims nothing. ``` -### Shared boards across homes - -Mounting one `kanban.db` in several Hermes homes (containers, fleet hosts) shares -the board, but profile names are home-local: every home has a root profile named -`default`, and the dispatcher's spawn gate checks `profile_exists(assignee)` -against the *claiming* home. Without further configuration, every home's -dispatcher considers a card assigned to `default` claimable, so the wrong home -can claim and run it. Either give each home unique profile names, or set -`kanban.dispatch_profiles` per home to declare exactly which assignees that home -may claim — anything else lands in the dispatcher's `skipped_nonspawnable` -bucket instead of spawning. - Override the config flag at runtime via `HERMES_KANBAN_DISPATCH_IN_GATEWAY=0` for debugging. Standard gateway supervision applies: run `hermes gateway start` directly, or wire the gateway up as a systemd user unit (see the @@ -316,6 +303,20 @@ the old standalone daemon alive for one release cycle, but running both a gateway-embedded dispatcher AND a standalone daemon against the same `kanban.db` causes claim races and is not supported. +### Shared boards across homes + +Mounting one `kanban.db` in several Hermes homes (containers, fleet hosts) shares +the board, but profile names are home-local and every home has a root profile +named `default` — so `default` collides by construction. The dispatcher's spawn +gate checks `profile_exists(assignee)` against the *claiming* home, and without +further configuration every home's dispatcher considers a card assigned to +`default` claimable, so the wrong home can claim and run it. Either give each +home unique profile names and never assign cards to `default` on a shared board, +or set `kanban.dispatch_profiles` per home to declare exactly which assignees +that home may claim — anything else lands in the dispatcher's +`skipped_nonspawnable` bucket instead of spawning, and no longer counts as +spawnable work for the gateway's wake-up probe. + ### Idempotent create (for automation / webhooks) ```bash @@ -898,6 +899,7 @@ All commands are also available as a slash command in the interactive CLI and in |------------|---------|--------------| | `kanban.max_in_progress` | unset (unlimited) | Caps the number of simultaneously running tasks. When the board already has N running, the dispatcher skips spawning more — useful for slow workers (local LLMs, resource-constrained hosts) so they finish what they have before more pile up and time out. Invalid or below-1 values log a warning and behave as unlimited. | | `kanban.max_in_progress_per_profile` | unset (unlimited) | Per-profile variant of `max_in_progress` — caps how many tasks any single assignee profile may run concurrently. Useful when one profile is slow or rate-limited but others should keep flowing. Applies alongside the board-wide `max_in_progress`; both must allow a spawn for it to proceed. | +| `kanban.dispatch_profiles` | unset (any existing profile) | Per-home claim allowlist for boards shared across Hermes homes. When set, this home's dispatcher only claims cards whose assignee is listed (fail-closed; an empty list claims nothing); other assignees land in `skipped_nonspawnable`. See [Shared boards across homes](#shared-boards-across-homes). | | `kanban.auto_promote_children` | `true` | After `decompose_triage_task()` produces children with no parent-blocker dependencies, they're automatically promoted to `ready` so the dispatcher can pick them up. Set to `false` to require manual review — children stay in `todo` until you promote them. | | `kanban.default_workdir` | unset | Board-level default working directory applied to new tasks when neither `--workspace` nor the task itself overrides it. Per-task `workspace:` still wins. |