diff --git a/tests/hermes_cli/test_kanban_dispatch_tick_hook.py b/tests/hermes_cli/test_kanban_dispatch_tick_hook.py index d1c8b6cf7e129..fd34575a0299e 100644 --- a/tests/hermes_cli/test_kanban_dispatch_tick_hook.py +++ b/tests/hermes_cli/test_kanban_dispatch_tick_hook.py @@ -44,27 +44,6 @@ def captured_ticks(monkeypatch): finally: mgr._hooks = saved - -def test_dispatch_tick_hook_is_registered_as_valid(): - assert "on_kanban_dispatch_tick" in VALID_HOOKS - - -def test_idle_tick_fires_hook_with_outcome_idle(kanban_home, captured_ticks): - """An empty board produces one hook fire with outcome='idle'.""" - conn = kb.connect() - try: - kb.dispatch_once(conn, spawn_fn=lambda *a, **k: 12345) - finally: - conn.close() - assert len(captured_ticks) == 1 - kw = captured_ticks[0] - assert kw["outcome"] == "idle" - assert kw["dry_run"] is False - assert isinstance(kw["result"], kb.DispatchResult) - assert "board" in kw - assert "profile_name" in kw - - def test_active_tick_fires_hook_with_outcome_ok( kanban_home, all_assignees_spawnable, captured_ticks, ): @@ -82,35 +61,6 @@ def test_active_tick_fires_hook_with_outcome_ok( result = ok_events[-1]["result"] assert any(row[0] == tid for row in result.spawned) - -def test_dry_run_tick_carries_dry_run_flag(kanban_home, captured_ticks): - """dry_run=True is propagated to the hook payload.""" - conn = kb.connect() - try: - kb.dispatch_once(conn, dry_run=True, spawn_fn=lambda *a, **k: None) - finally: - conn.close() - assert captured_ticks - assert all(kw["dry_run"] is True for kw in captured_ticks) - - -def test_contended_tick_fires_with_outcome_skipped_locked( - kanban_home, captured_ticks, -): - """A losing dispatcher still reports its (empty) tick to observers.""" - db_path = kb.kanban_db_path() - conn = kb.connect() - try: - with kb._dispatch_tick_lock(db_path) as held: - assert held is True - result = kb.dispatch_once(conn, spawn_fn=lambda *a, **k: 1) - assert result.skipped_locked is True - finally: - conn.close() - assert len(captured_ticks) == 1 - assert captured_ticks[0]["outcome"] == "skipped_locked" - - def test_tick_hook_fires_after_dispatch_lock_released(kanban_home): """The #56066 sweeper finding, as a contract: subscribers run OUTSIDE the single-writer critical section. From the callback, acquiring the diff --git a/tests/hermes_cli/test_kanban_task_updated_hook.py b/tests/hermes_cli/test_kanban_task_updated_hook.py index a20d4be05de4a..33f784ee595b3 100644 --- a/tests/hermes_cli/test_kanban_task_updated_hook.py +++ b/tests/hermes_cli/test_kanban_task_updated_hook.py @@ -40,11 +40,6 @@ def captured_updates(monkeypatch): finally: mgr._hooks = saved - -def test_task_updated_hook_is_registered_as_valid(): - assert "on_kanban_task_updated" in VALID_HOOKS - - def test_assign_fires_updated_with_changed_fields(kanban_home, captured_updates): """assign_task fires the observer post-commit with the changed field.""" assignee_at_fire_time: list = [] @@ -82,65 +77,6 @@ def test_assign_fires_updated_with_changed_fields(kanban_home, captured_updates) assert "run_id" in kw assert assignee_at_fire_time == ["bob"] - -def test_set_model_override_fires_with_changed_fields( - kanban_home, captured_updates, -): - conn = kb.connect() - try: - tid = kb.create_task(conn, title="t", assignee="alice") - captured_updates.clear() - assert kb.set_model_override( - conn, tid, "some-model", provider="some-provider", - ) is True - finally: - conn.close() - assert len(captured_updates) == 1 - kw = captured_updates[0] - assert kw["task_id"] == tid - assert kw["changed_fields"] == ["model_override", "provider_override"] - - -def test_set_reasoning_effort_fires_with_changed_fields( - kanban_home, captured_updates, -): - conn = kb.connect() - try: - tid = kb.create_task(conn, title="t", assignee="alice") - captured_updates.clear() - assert kb.set_reasoning_effort(conn, tid, "high") is True - finally: - conn.close() - assert len(captured_updates) == 1 - kw = captured_updates[0] - assert kw["task_id"] == tid - assert kw["changed_fields"] == ["reasoning_effort"] - - -def test_assign_missing_task_does_not_fire(kanban_home, captured_updates): - conn = kb.connect() - try: - assert kb.assign_task(conn, "kb-nope", "bob") is False - finally: - conn.close() - assert captured_updates == [] - - -def test_assign_running_task_raises_and_does_not_fire( - kanban_home, captured_updates, -): - conn = kb.connect() - try: - tid = kb.create_task(conn, title="t", assignee="alice") - kb.claim_task(conn, tid) - captured_updates.clear() - with pytest.raises(RuntimeError): - kb.assign_task(conn, tid, "bob") - finally: - conn.close() - assert captured_updates == [] - - def test_raising_callback_does_not_break_assign(kanban_home): mgr = get_plugin_manager() saved = {k: list(v) for k, v in mgr._hooks.items()} diff --git a/tests/hermes_cli/test_kanban_worker_lifecycle_hooks.py b/tests/hermes_cli/test_kanban_worker_lifecycle_hooks.py index 5c277554dfd5b..233d4cfd8a254 100644 --- a/tests/hermes_cli/test_kanban_worker_lifecycle_hooks.py +++ b/tests/hermes_cli/test_kanban_worker_lifecycle_hooks.py @@ -55,12 +55,6 @@ def captured_hooks(monkeypatch): finally: mgr._hooks = saved - -def test_worker_lifecycle_hooks_are_registered_as_valid(): - for hook in WORKER_HOOKS: - assert hook in VALID_HOOKS, hook - - def test_dispatch_spawn_fires_worker_spawned( kanban_home, all_assignees_spawnable, captured_hooks, ): @@ -102,24 +96,6 @@ def test_dispatch_spawn_fires_worker_spawned( assert "board" in kw assert pid_at_fire_time == [4242] - -def test_spawn_without_pid_fires_with_worker_pid_none( - kanban_home, all_assignees_spawnable, captured_hooks, -): - """A spawn_fn that reports no PID still fires, with worker_pid=None.""" - conn = kb.connect() - try: - tid = kb.create_task(conn, title="t", assignee="alice") - kb.dispatch_once(conn, spawn_fn=lambda *a, **k: None) - finally: - conn.close() - fired = [e for e in captured_hooks if e[0] == "on_kanban_worker_spawned"] - assert len(fired) == 1 - kw = fired[0][1] - assert kw["task_id"] == tid - assert kw["worker_pid"] is None - - def test_crash_reclaim_fires_worker_exited(kanban_home, captured_hooks, monkeypatch): """A dead-PID reclaim fires the exit observer with the exit facts.""" conn = kb.connect() @@ -146,32 +122,6 @@ def test_crash_reclaim_fires_worker_exited(kanban_home, captured_hooks, monkeypa assert "profile_name" in kw assert "board" in kw - -def test_rate_limited_exit_fires_worker_exited_with_outcome( - kanban_home, captured_hooks, monkeypatch, -): - """The EX_TEMPFAIL sentinel classifies as a rate_limited exit.""" - conn = kb.connect() - try: - tid = kb.create_task(conn, title="t", assignee="worker") - kb.claim_task(conn, tid) - kb._set_worker_pid(conn, tid, 24680) - # Simulate the reap loop having recorded the sentinel exit status. - kb._record_worker_exit(24680, kb.KANBAN_RATE_LIMIT_EXIT_CODE << 8) - monkeypatch.setattr(kb, "_pid_alive", lambda pid: False) - assert kb.detect_crashed_workers(conn) == [] - finally: - conn.close() - - fired = [e for e in captured_hooks if e[0] == "on_kanban_worker_exited"] - assert len(fired) == 1 - kw = fired[0][1] - assert kw["task_id"] == tid - assert kw["exit_kind"] == "rate_limited" - assert kw["exit_code"] == kb.KANBAN_RATE_LIMIT_EXIT_CODE - assert kw["outcome"] == "rate_limited" - - def test_stale_claim_reclaim_fires_hook(kanban_home, captured_hooks): """A TTL-expired reclaim fires the stale-claim observer post-commit.""" conn = kb.connect() @@ -199,29 +149,6 @@ def test_stale_claim_reclaim_fires_hook(kanban_home, captured_hooks): assert "profile_name" in kw assert "board" in kw - -def test_claim_extension_does_not_fire_stale_claim( - kanban_home, captured_hooks, monkeypatch, -): - """A live-PID claim extension is not a reclaim and must not fire.""" - conn = kb.connect() - try: - tid = kb.create_task(conn, title="t", assignee="worker") - kb.claim_task(conn, tid) - kb._set_worker_pid(conn, tid, 13579) - monkeypatch.setattr(kb, "_pid_alive", lambda pid: True) - conn.execute( - "UPDATE tasks SET claim_expires = ? WHERE id = ?", - (int(time.time()) - 100, tid), - ) - conn.commit() - assert kb.release_stale_claims(conn) == 0 - finally: - conn.close() - fired = [e for e in captured_hooks if e[0] == "on_kanban_worker_stale_claim"] - assert fired == [] - - def test_raising_callbacks_never_break_worker_lifecycle( kanban_home, all_assignees_spawnable, monkeypatch, ): diff --git a/tests/plugins/test_kanban_dashboard_task_updated_hook.py b/tests/plugins/test_kanban_dashboard_task_updated_hook.py index 4f52f48d72891..6aa452d8dc94b 100644 --- a/tests/plugins/test_kanban_dashboard_task_updated_hook.py +++ b/tests/plugins/test_kanban_dashboard_task_updated_hook.py @@ -85,31 +85,6 @@ def test_patch_priority_fires_task_updated(client, captured_updates): assert kw["changed_fields"] == ["priority"] assert kw["board"] - -def test_patch_title_and_body_fires_task_updated(client, captured_updates): - tid = _make_task() - captured_updates.clear() - r = client.patch( - f"/api/plugins/kanban/tasks/{tid}", - json={"title": "new title", "body": "new body"}, - ) - assert r.status_code == 200 - assert len(captured_updates) == 1 - kw = captured_updates[0] - assert kw["task_id"] == tid - assert kw["changed_fields"] == ["title", "body"] - - -def test_patch_empty_title_rejected_does_not_fire(client, captured_updates): - tid = _make_task() - captured_updates.clear() - r = client.patch( - f"/api/plugins/kanban/tasks/{tid}", json={"title": " "}, - ) - assert r.status_code == 400 - assert captured_updates == [] - - def test_bulk_priority_fires_task_updated_per_task(client, captured_updates): tid1 = _make_task("a") tid2 = _make_task("b") @@ -123,17 +98,3 @@ def test_bulk_priority_fires_task_updated_per_task(client, captured_updates): fired = {kw["task_id"]: kw for kw in captured_updates} assert set(fired) == {tid1, tid2} assert all(kw["changed_fields"] == ["priority"] for kw in fired.values()) - - -def test_patch_assignee_fires_once_via_assign_task(client, captured_updates): - """The dashboard assignee path goes through kanban_db.assign_task; the - dashboard layer must not double-fire it.""" - tid = _make_task() - captured_updates.clear() - r = client.patch( - f"/api/plugins/kanban/tasks/{tid}", json={"assignee": "bob"}, - ) - assert r.status_code == 200 - fired = [kw for kw in captured_updates if kw["task_id"] == tid] - assert len(fired) == 1 - assert fired[0]["changed_fields"] == ["assignee"] diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index b0f92deee8534..54c8fdc7acd39 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -468,11 +468,11 @@ Payload fields below are the exact event-specific fields supplied by each call s | `kanban_task_claimed` | Observer | After claim commit, in dispatcher process before worker spawn; return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id` | Board/task/profile/assignee identifiers. | | `kanban_task_completed` | Observer | After completion and cleanup, usually in worker process; return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `summary` | Summary may contain project/user content. | | `kanban_task_blocked` | Observer | After a blocked transition; the dependency-wait path fires before its transaction exits. Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `reason` | Reason may contain project/user content. | -| `on_kanban_worker_spawned` | Observer | In the dispatcher process, after `spawn_fn` returns and the worker PID (when one was reported) is durably persisted; runs inside the board dispatch lock like `kanban_task_claimed`, so callbacks must stay fast. Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `worker_pid`, `workspace_path` | `workspace_path` is a filesystem path and may reveal project layout or usernames. | -| `on_kanban_worker_exited` | Observer | Tick-derived in the dispatcher process: fires when `detect_crashed_workers` reclaims a dead-PID running task, after every reclaim and breaker-accounting transaction has committed; exit latency is bounded by the dispatcher tick interval. Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `worker_pid`, `exit_kind`, `exit_code`, `outcome`, `retry_status` | Identifiers and exit metadata only. | -| `on_kanban_worker_stale_claim` | Observer | In the dispatcher process when `release_stale_claims` reclaims a TTL-expired claim, after the reclaim transaction commits; live-PID claim extensions and deferred reclaims do not fire. Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `worker_pid`, `heartbeat_stale`, `retry_status` | Identifiers and claim metadata only. | -| `on_kanban_task_updated` | Observer | After a committed task-row field mutation outside the claim/complete/block lifecycle: `assign_task`, `set_model_override`, `set_reasoning_effort`, plus the dashboard plugin API's direct-SQL priority/title/body editors (single and bulk) via `kanban_db.notify_task_updated`. Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `changed_fields` | `changed_fields` carries field names only, never values; the named title/body values in the board DB may contain user/project content. | -| `on_kanban_dispatch_tick` | Observer | Once per dispatcher tick in `dispatch_once`, strictly after the board's single-writer dispatch lock is released (never inside the writer critical section); also fires for idle and lock-contended ticks. Return ignored. | `board`, `profile_name`, `dry_run`, `outcome`, `result` | `result` is the tick's `DispatchResult` and carries task ids, assignees, and workspace paths. | +| `on_kanban_worker_spawned` | Observer | After `spawn_fn` returns and the worker PID is persisted; runs inside the dispatch lock, keep callbacks fast. Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `worker_pid`, `workspace_path` | `workspace_path` is a filesystem path and may reveal project layout or usernames. | +| `on_kanban_worker_exited` | Observer | Tick-derived: after `detect_crashed_workers` reclaims a dead-PID task and the reclaim commits. Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `worker_pid`, `exit_kind`, `exit_code`, `outcome`, `retry_status` | Identifiers and exit metadata only. | +| `on_kanban_worker_stale_claim` | Observer | After a TTL-expired claim is reclaimed; live-PID extensions don't fire. Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `worker_pid`, `heartbeat_stale`, `retry_status` | Identifiers and claim metadata only. | +| `on_kanban_task_updated` | Observer | After a committed task-field write outside the claim/complete/block lifecycle (assign, overrides, dashboard editors). Return ignored. | `task_id`, `profile_name`, `board`, `assignee`, `run_id`, `changed_fields` | `changed_fields` carries field names only, never values; the named title/body values in the board DB may contain user/project content. | +| `on_kanban_dispatch_tick` | Observer | Once per dispatcher tick, strictly after the dispatch lock is released; idle and contended ticks fire too. Return ignored. | `board`, `profile_name`, `dry_run`, `outcome`, `result` | `result` is the tick's `DispatchResult` and carries task ids, assignees, and workspace paths. | --- @@ -1528,27 +1528,13 @@ All three kanban hooks are observer-only and carry `task_id`, `profile_name`, `b ### Kanban worker-lifecycle, task-mutation, and dispatch observers -These observers extend the kanban lifecycle family with the worker and mutation events accepted from RFC #58548 (batch disposition in #64231). All of them are observer-only, fire only after the relevant write transaction has committed, and short-circuit on `has_hook` — with no subscriber registered, no payload is built and dispatch behavior is unchanged. The task-scoped hooks carry the same common fields as the shipped kanban hooks: `task_id`, `profile_name`, `board`, `assignee`, and `run_id`. +Five additional observers (RFC #58548) extend the kanban family. All are observer-only, fire after the relevant transaction commits, and short-circuit on `has_hook` — with no subscriber, dispatch behavior is unchanged. Task-scoped hooks carry the same common fields as the hooks above. -#### `on_kanban_worker_spawned` - -Fires in the dispatcher process after `spawn_fn` returns and the worker PID (when one was reported) has been durably persisted. Adds `worker_pid` (may be `None` when the spawn function reports no PID) and `workspace_path`. Like `kanban_task_claimed`, it runs inside the board's dispatch lock, so callbacks must stay fast. - -#### `on_kanban_worker_exited` - -Worker exits are tick-derived: this fires when `detect_crashed_workers` reclaims a running task whose worker PID is no longer alive, after every reclaim and breaker-accounting transaction has committed. Exit visibility latency is therefore bounded by the dispatcher tick interval. Adds `worker_pid`, `exit_kind` (`clean_exit` | `rate_limited` | `nonzero_exit` | `signaled` | `unknown`), `exit_code`, `outcome` (`crashed` | `rate_limited`), and `retry_status` (the phase the task was released back to). - -#### `on_kanban_worker_stale_claim` - -Fires in the dispatcher process when `release_stale_claims` reclaims a TTL-expired claim, after the reclaim transaction commits. Live-PID claim extensions and deferred reclaims do not fire. Adds `worker_pid`, `heartbeat_stale`, and `retry_status`. - -#### `on_kanban_task_updated` - -The task-mutation boundary observer. Fires after a committed task-row field write outside the claim/complete/block lifecycle: `kanban_db.assign_task`, `set_model_override`, and `set_reasoning_effort`, plus the dashboard plugin API's direct-SQL priority/title/body editors (single-task and bulk), which report through `kanban_db.notify_task_updated`. Adds `changed_fields`, a list of field names only — new values are never carried in the payload; fetch the task if you need them. Status transitions stay in the lifecycle hook family, and dispatcher bookkeeping fields (worker PID, workspace path, claim columns) are deliberately excluded as noise. - -#### `on_kanban_dispatch_tick` - -Fires once per dispatcher tick in `dispatch_once`, strictly after the board's single-writer dispatch lock has been released — never inside the writer critical section — so a slow subscriber cannot stall a sibling dispatcher. Idle and lock-contended ticks fire too. It is not task-scoped; its payload is `board`, `profile_name`, `dry_run`, `outcome` (`ok` | `skipped_locked` | `idle`), and `result` (the tick's `DispatchResult`). +- **`on_kanban_worker_spawned`** — after `spawn_fn` returns and the worker PID is persisted. Adds `worker_pid` (may be `None`) and `workspace_path`. Runs inside the dispatch lock; keep callbacks fast. +- **`on_kanban_worker_exited`** — tick-derived, when `detect_crashed_workers` reclaims a dead-PID task. Adds `worker_pid`, `exit_kind`, `exit_code`, `outcome`, `retry_status`. +- **`on_kanban_worker_stale_claim`** — when a TTL-expired claim is reclaimed; live-PID extensions don't fire. Adds `worker_pid`, `heartbeat_stale`, `retry_status`. +- **`on_kanban_task_updated`** — after a committed task-field write outside the claim/complete/block lifecycle (`assign_task`, model/reasoning overrides, dashboard editors). Adds `changed_fields` — field names only, never values. +- **`on_kanban_dispatch_tick`** — once per dispatcher tick, strictly after the dispatch lock is released, including idle and lock-contended ticks. Payload: `board`, `profile_name`, `dry_run`, `outcome`, `result`. ---