mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
Merge pull request #61649 from NousResearch/bb/kanban-worker-headless
fix(kanban): headless workers, live-retry diagnostics, and re-queue respawn
This commit is contained in:
commit
8e3f9537db
7 changed files with 241 additions and 16 deletions
|
|
@ -6789,7 +6789,9 @@ def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str]
|
|||
``"recent_success"``
|
||||
A completed run exists within ``_RESPAWN_GUARD_SUCCESS_WINDOW``
|
||||
seconds. Useful work already succeeded for this task; wait for
|
||||
human review rather than immediately re-spawning.
|
||||
human review rather than immediately re-spawning. Bypassed when an
|
||||
explicit re-queue event (status change, promote, unblock, reclaim)
|
||||
arrives AFTER that completion — that's a deliberate re-run request.
|
||||
|
||||
``"active_pr"``
|
||||
A GitHub PR URL appears in a recent task comment (within
|
||||
|
|
@ -6852,13 +6854,29 @@ def check_respawn_guard(conn: sqlite3.Connection, task_id: str) -> Optional[str]
|
|||
return "blocker_auth"
|
||||
|
||||
# 3. Completed run within guard window — proof of recent success.
|
||||
# Exception: an explicit re-queue AFTER that success (an operator
|
||||
# dragging done→ready, a dependency re-promotion, an unblock, a
|
||||
# reclaim) is a deliberate "run it again" — honor it instead of
|
||||
# deferring. Without this, a manual done→ready just sits there,
|
||||
# silently held by the guard, until the window elapses.
|
||||
cutoff = now - _RESPAWN_GUARD_SUCCESS_WINDOW
|
||||
if conn.execute(
|
||||
"SELECT id FROM task_runs "
|
||||
"WHERE task_id = ? AND outcome = 'completed' AND ended_at >= ?",
|
||||
recent_completed = conn.execute(
|
||||
"SELECT ended_at FROM task_runs "
|
||||
"WHERE task_id = ? AND outcome = 'completed' AND ended_at >= ? "
|
||||
"ORDER BY ended_at DESC LIMIT 1",
|
||||
(task_id, cutoff),
|
||||
).fetchone():
|
||||
return "recent_success"
|
||||
).fetchone()
|
||||
if recent_completed:
|
||||
completed_at = int(recent_completed["ended_at"] or 0)
|
||||
requeued_after = conn.execute(
|
||||
"SELECT 1 FROM task_events "
|
||||
"WHERE task_id = ? AND created_at >= ? "
|
||||
"AND kind IN ('status', 'promoted', 'unblocked', 'reclaimed') "
|
||||
"LIMIT 1",
|
||||
(task_id, completed_at),
|
||||
).fetchone()
|
||||
if not requeued_after:
|
||||
return "recent_success"
|
||||
|
||||
# 4. GitHub PR URL in a recent comment — prior worker already opened a PR.
|
||||
pr_cutoff = now - _RESPAWN_GUARD_PR_WINDOW
|
||||
|
|
@ -7768,9 +7786,18 @@ def _default_spawn(
|
|||
# attributed correctly regardless of how the child loads config.
|
||||
env["HERMES_PROFILE"] = profile_arg
|
||||
|
||||
# A worker must NEVER boot the interactive TUI: an inherited HERMES_TUI=1
|
||||
# or a `display.interface: tui` in the profile's config would send the
|
||||
# quiet chat run into the Ink TUI, whose no-TTY bail-out exits 0 without
|
||||
# doing the task → "protocol violation" on every attempt. `--cli` is the
|
||||
# highest-precedence interface override; dropping the env var covers
|
||||
# older hermes builds on PATH that predate the flag's precedence.
|
||||
env.pop("HERMES_TUI", None)
|
||||
|
||||
cmd = [
|
||||
*_resolve_hermes_argv(),
|
||||
"-p", profile_arg,
|
||||
"--cli",
|
||||
# Worker subprocesses switch to a profile-scoped HERMES_HOME above,
|
||||
# so they see that profile's shell-hook allowlist instead of the
|
||||
# dispatcher's root allowlist. Pass --accept-hooks explicitly so
|
||||
|
|
|
|||
|
|
@ -530,7 +530,20 @@ def _rule_repeated_failures(task, events, runs, now, cfg) -> list[Diagnostic]:
|
|||
|
||||
Accepts the legacy ``spawn_failure_threshold`` config key for
|
||||
back-compat.
|
||||
|
||||
Terminal statuses are exempt: a done/archived card has nothing left
|
||||
to retry, so a lingering failure streak is history, not a signal.
|
||||
(``complete_task`` resets the counter, but a manual done — e.g. a
|
||||
dashboard drag — ends no run and used to leave the flag stuck.)
|
||||
|
||||
A fresh attempt in flight (``running``) is also exempt: retrying a
|
||||
task should clear the stale failure banner until this attempt also
|
||||
resolves. Otherwise a card that's actively trying again still shows
|
||||
"failed Nx", which reads as a current failure. It re-fires if the new
|
||||
run fails too (status leaves ``running`` with a recorded outcome).
|
||||
"""
|
||||
if _task_field(task, "status") in ("done", "archived", "running"):
|
||||
return []
|
||||
threshold = _positive_int(cfg.get(
|
||||
"failure_threshold",
|
||||
cfg.get("spawn_failure_threshold", 3),
|
||||
|
|
@ -649,7 +662,20 @@ def _rule_repeated_crashes(task, events, runs, now, cfg) -> list[Diagnostic]:
|
|||
total failures) so the operator gets a crash-specific heads-up
|
||||
before the unified rule kicks in. Suppresses itself when the
|
||||
unified rule is also about to fire, to avoid double-flagging.
|
||||
|
||||
Terminal statuses are exempt for the same reason as
|
||||
``repeated_failures`` — with one extra wrinkle: this rule reads run
|
||||
history, and a manual done (dashboard drag) appends no ``completed``
|
||||
run to break the crash streak, so the flag was permanent (#kanban
|
||||
desktop dogfood). Done means done.
|
||||
|
||||
``running`` is exempt too: a fresh attempt is in flight, and its
|
||||
in-flight run (no outcome yet) doesn't break the trailing crash scan,
|
||||
so a retried card kept showing "crashed Nx" over an active run. The
|
||||
banner re-fires if the new attempt also crashes.
|
||||
"""
|
||||
if _task_field(task, "status") in ("done", "archived", "running"):
|
||||
return []
|
||||
failure_threshold = int(cfg.get(
|
||||
"failure_threshold",
|
||||
cfg.get("spawn_failure_threshold", 3),
|
||||
|
|
|
|||
|
|
@ -148,7 +148,17 @@ def _wants_tui_early(argv: "list[str] | None" = None) -> bool:
|
|||
"""Earliest TUI decision, usable before argparse/config imports.
|
||||
|
||||
Precedence: explicit ``--cli`` wins (forces classic REPL), then
|
||||
``--tui``/``HERMES_TUI=1``, then ``display.interface`` in config.
|
||||
explicit ``--tui``/``HERMES_TUI=1``, then a real-TTY gate (a
|
||||
non-interactive stdio can't host the Ink UI, so ambient config never
|
||||
boots it there), then ``display.interface`` in config.
|
||||
|
||||
The TTY gate is load-bearing for headless spawners — kanban workers,
|
||||
cron jobs, pipes run ``hermes … chat -q`` with stdio on a pipe. This
|
||||
is the earliest launch decision (it runs before ``cmd_chat`` /
|
||||
``_resolve_use_tui``), so a ``display.interface: tui`` default used to
|
||||
boot the TUI here — whose no-TTY bail-out exits 0 without doing the
|
||||
task → "protocol violation" on every attempt. An explicit ``--tui``
|
||||
still reaches the informative bail-out.
|
||||
"""
|
||||
if argv is None:
|
||||
argv = sys.argv[1:]
|
||||
|
|
@ -156,6 +166,11 @@ def _wants_tui_early(argv: "list[str] | None" = None) -> bool:
|
|||
return False
|
||||
if os.environ.get("HERMES_TUI") == "1" or "--tui" in argv:
|
||||
return True
|
||||
try:
|
||||
if not (sys.stdin.isatty() and sys.stdout.isatty()):
|
||||
return False
|
||||
except Exception:
|
||||
return False
|
||||
return _config_default_interface_early() == "tui"
|
||||
|
||||
|
||||
|
|
@ -2193,16 +2208,34 @@ def _resolve_use_tui(args) -> bool:
|
|||
|
||||
Precedence (highest first):
|
||||
1. ``--cli`` flag → always classic REPL
|
||||
2. ``--tui`` flag / ``HERMES_TUI=1`` → always TUI
|
||||
3. ``display.interface`` config value ("cli" | "tui")
|
||||
4. default → classic REPL
|
||||
2. ``--tui`` flag → always TUI (explicit ask)
|
||||
3. no TTY → always classic (ambient prefs don't apply)
|
||||
4. ``HERMES_TUI=1`` env → TUI
|
||||
5. ``display.interface`` config value ("cli" | "tui")
|
||||
6. default → classic REPL
|
||||
|
||||
Explicit flags always win over config so muscle memory and scripts keep
|
||||
working regardless of the configured default.
|
||||
|
||||
The TTY gate (3) is load-bearing: ambient TUI preferences (env var or
|
||||
config default) must never hijack a NON-interactive invocation. Kanban
|
||||
workers, cron jobs, and pipelines run ``hermes … chat -q`` with stdout
|
||||
on a pipe; booting the Ink TUI there hits its no-TTY bail-out, which
|
||||
prints a resume hint and exits 0 — a kanban worker then dies with
|
||||
"exited cleanly without calling kanban_complete — protocol violation"
|
||||
on every attempt (found dogfooding the desktop kanban board). A user
|
||||
who *explicitly* passes ``--tui`` still gets the informative bail-out.
|
||||
"""
|
||||
if getattr(args, "cli", False):
|
||||
return False
|
||||
if getattr(args, "tui", False) or os.environ.get("HERMES_TUI") == "1":
|
||||
if getattr(args, "tui", False):
|
||||
return True
|
||||
try:
|
||||
if not (sys.stdin.isatty() and sys.stdout.isatty()):
|
||||
return False
|
||||
except Exception:
|
||||
return False
|
||||
if os.environ.get("HERMES_TUI") == "1":
|
||||
return True
|
||||
try:
|
||||
from hermes_cli.config import load_config
|
||||
|
|
|
|||
|
|
@ -5,10 +5,18 @@ flip ``display.interface: tui`` in config.yaml to make the modern Ink TUI the
|
|||
default for bare ``hermes`` / ``hermes chat``. Explicit flags always win:
|
||||
|
||||
--cli forces the classic REPL (highest precedence)
|
||||
--tui / HERMES_TUI=1 forces the TUI
|
||||
--tui forces the TUI
|
||||
(no TTY) forces the classic REPL — ambient prefs don't apply
|
||||
HERMES_TUI=1 the env default
|
||||
display.interface the configured default
|
||||
(unset) classic REPL
|
||||
|
||||
The no-TTY gate exists because ambient TUI preferences must never hijack
|
||||
non-interactive invocations: kanban workers / cron / pipelines run
|
||||
``hermes … chat -q`` on a pipe, and the TUI's no-TTY bail-out exits 0
|
||||
without doing the work (a kanban worker then dies with "protocol
|
||||
violation" on every attempt).
|
||||
|
||||
These tests pin that precedence at every layer that makes the decision:
|
||||
|
||||
* ``_resolve_use_tui(args)`` — the canonical args-aware resolver used by
|
||||
|
|
@ -46,6 +54,14 @@ def _args(**kw):
|
|||
return SimpleNamespace(**kw)
|
||||
|
||||
|
||||
def _fake_tty(monkeypatch, interactive: bool):
|
||||
"""Pin stdin/stdout TTY-ness — pytest's capture is never a real TTY."""
|
||||
import sys as _sys
|
||||
|
||||
monkeypatch.setattr(_sys.stdin, "isatty", lambda: interactive, raising=False)
|
||||
monkeypatch.setattr(_sys.stdout, "isatty", lambda: interactive, raising=False)
|
||||
|
||||
|
||||
def _patch_config(monkeypatch, interface):
|
||||
import hermes_cli.config as cfg
|
||||
|
||||
|
|
@ -73,19 +89,23 @@ class TestResolveUseTui:
|
|||
|
||||
def test_env_beats_config_cli(self, monkeypatch):
|
||||
_patch_config(monkeypatch, "cli")
|
||||
_fake_tty(monkeypatch, True)
|
||||
monkeypatch.setenv("HERMES_TUI", "1")
|
||||
assert m._resolve_use_tui(_args()) is True
|
||||
|
||||
def test_config_tui_with_no_flags(self, monkeypatch):
|
||||
_patch_config(monkeypatch, "tui")
|
||||
_fake_tty(monkeypatch, True)
|
||||
assert m._resolve_use_tui(_args()) is True
|
||||
|
||||
def test_config_cli_is_default(self, monkeypatch):
|
||||
_patch_config(monkeypatch, "cli")
|
||||
_fake_tty(monkeypatch, True)
|
||||
assert m._resolve_use_tui(_args()) is False
|
||||
|
||||
def test_interface_value_is_case_insensitive(self, monkeypatch):
|
||||
_patch_config(monkeypatch, "TUI")
|
||||
_fake_tty(monkeypatch, True)
|
||||
assert m._resolve_use_tui(_args()) is True
|
||||
|
||||
def test_load_config_failure_falls_back_to_cli(self, monkeypatch):
|
||||
|
|
@ -95,8 +115,28 @@ class TestResolveUseTui:
|
|||
raise RuntimeError("config unreadable")
|
||||
|
||||
monkeypatch.setattr(cfg, "load_config", boom)
|
||||
_fake_tty(monkeypatch, True)
|
||||
assert m._resolve_use_tui(_args()) is False
|
||||
|
||||
# ── the no-TTY gate: ambient prefs never hijack non-interactive runs ────
|
||||
def test_no_tty_blocks_env_tui(self, monkeypatch):
|
||||
_patch_config(monkeypatch, "cli")
|
||||
_fake_tty(monkeypatch, False)
|
||||
monkeypatch.setenv("HERMES_TUI", "1")
|
||||
assert m._resolve_use_tui(_args()) is False
|
||||
|
||||
def test_no_tty_blocks_config_tui(self, monkeypatch):
|
||||
_patch_config(monkeypatch, "tui")
|
||||
_fake_tty(monkeypatch, False)
|
||||
assert m._resolve_use_tui(_args()) is False
|
||||
|
||||
def test_explicit_tui_flag_survives_no_tty(self, monkeypatch):
|
||||
# An explicit --tui is the user's own ask — keep the informative
|
||||
# no-TTY bail-out instead of silently swapping interfaces.
|
||||
_patch_config(monkeypatch, "cli")
|
||||
_fake_tty(monkeypatch, False)
|
||||
assert m._resolve_use_tui(_args(tui=True)) is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _wants_tui_early — dependency-free early resolver
|
||||
|
|
@ -113,10 +153,24 @@ class TestWantsTuiEarly:
|
|||
|
||||
return _make
|
||||
|
||||
def test_config_tui_bare_argv(self, home_with_interface):
|
||||
def test_config_tui_bare_argv(self, home_with_interface, monkeypatch):
|
||||
home_with_interface("tui")
|
||||
_fake_tty(monkeypatch, True) # config-tui only applies on a real TTY
|
||||
assert m._wants_tui_early([]) is True
|
||||
|
||||
def test_no_tty_blocks_config_tui(self, home_with_interface, monkeypatch):
|
||||
# Headless (worker/cron/pipe): ambient config-tui must not boot the
|
||||
# Ink UI in the earliest launch decision — that's the crash the
|
||||
# kanban worker hit before the gate existed.
|
||||
home_with_interface("tui")
|
||||
_fake_tty(monkeypatch, False)
|
||||
assert m._wants_tui_early([]) is False
|
||||
|
||||
def test_explicit_tui_flag_survives_no_tty(self, home_with_interface, monkeypatch):
|
||||
home_with_interface("cli")
|
||||
_fake_tty(monkeypatch, False)
|
||||
assert m._wants_tui_early(["--tui"]) is True
|
||||
|
||||
def test_cli_flag_overrides_config_tui(self, home_with_interface):
|
||||
home_with_interface("tui")
|
||||
assert m._wants_tui_early(["--cli"]) is False
|
||||
|
|
|
|||
|
|
@ -1879,6 +1879,30 @@ def test_respawn_guard_recent_success(kanban_home):
|
|||
assert reason == "recent_success"
|
||||
|
||||
|
||||
def test_respawn_guard_recent_success_bypassed_by_requeue(kanban_home):
|
||||
"""An explicit re-queue after a recent success (operator done->ready,
|
||||
promote, unblock, reclaim) is a deliberate re-run and must bypass the
|
||||
recent_success guard — otherwise a manual done->ready just sits there
|
||||
until the window elapses."""
|
||||
with kb.connect() as conn:
|
||||
t = kb.create_task(conn, title="rerun-me", assignee="alice")
|
||||
now = int(time.time())
|
||||
conn.execute(
|
||||
"INSERT INTO task_runs (task_id, status, outcome, started_at, ended_at) "
|
||||
"VALUES (?, 'done', 'completed', ?, ?)",
|
||||
(t, now - 120, now - 60),
|
||||
)
|
||||
# Baseline: a recent completion defers the respawn.
|
||||
assert kb.check_respawn_guard(conn, t) == "recent_success"
|
||||
# Operator drags done -> ready: a 'status' event after completion.
|
||||
conn.execute(
|
||||
"INSERT INTO task_events (task_id, kind, created_at) "
|
||||
"VALUES (?, 'status', ?)",
|
||||
(t, now - 10),
|
||||
)
|
||||
assert kb.check_respawn_guard(conn, t) is None
|
||||
|
||||
|
||||
def test_respawn_guard_stale_success_not_guarded(kanban_home):
|
||||
"""A completed run outside the guard window does not block re-spawn."""
|
||||
with kb.connect() as conn:
|
||||
|
|
|
|||
|
|
@ -272,6 +272,27 @@ def test_repeated_crashes_escalates_on_many_crashes():
|
|||
assert diags[0].severity == "critical"
|
||||
|
||||
|
||||
def test_failure_rules_exempt_terminal_statuses():
|
||||
# A manual done (dashboard drag) ends no run, so the trailing crash
|
||||
# streak survives in run history — but done means done: neither
|
||||
# failure rule may keep flagging a terminal card.
|
||||
runs = [_run(outcome="crashed", run_id=1), _run(outcome="crashed", run_id=2)]
|
||||
for status in ("done", "archived"):
|
||||
task = _task(status=status, assignee="crashy", consecutive_failures=3)
|
||||
assert kd.compute_task_diagnostics(task, [], runs) == []
|
||||
|
||||
|
||||
def test_failure_rules_exempt_running_retry():
|
||||
# Retrying a task (→ running) puts a fresh attempt in flight; its
|
||||
# in-flight run (no outcome) doesn't break the trailing crash scan,
|
||||
# so the past streak used to keep flagging over an active retry.
|
||||
# A running card must clear the failure/crash banner until this
|
||||
# attempt itself resolves.
|
||||
runs = [_run(outcome="crashed", run_id=1), _run(outcome="crashed", run_id=2)]
|
||||
task = _task(status="running", assignee="crashy", consecutive_failures=3)
|
||||
assert kd.compute_task_diagnostics(task, [], runs) == []
|
||||
|
||||
|
||||
def test_stuck_in_blocked_fires_past_threshold():
|
||||
now = int(time.time())
|
||||
task = _task(status="blocked")
|
||||
|
|
@ -368,15 +389,19 @@ def test_repeated_crashes_truncates_huge_tracebacks():
|
|||
|
||||
def test_diagnostics_sorted_critical_first():
|
||||
"""A task with both a critical (many spawn failures) and a warning
|
||||
(prose phantoms) diagnostic should list the critical one first."""
|
||||
task = _task(status="done", consecutive_failures=10,
|
||||
(prose phantoms) diagnostic should list the critical one first.
|
||||
|
||||
Status must be non-terminal: done/archived are exempt from the
|
||||
failure rules (done means done). ``now=300`` keeps the synthetic
|
||||
timestamps from tripping stranded_in_ready — same dodge as above."""
|
||||
task = _task(status="ready", consecutive_failures=10,
|
||||
last_failure_error="nope")
|
||||
events = [
|
||||
_event("completed", ts=100, summary="referenced t_missing"),
|
||||
_event("suspected_hallucinated_references", ts=101,
|
||||
phantom_refs=["t_missing11"]),
|
||||
]
|
||||
diags = kd.compute_task_diagnostics(task, events, [])
|
||||
diags = kd.compute_task_diagnostics(task, events, [], now=300)
|
||||
kinds = [d.kind for d in diags]
|
||||
assert kinds[0] == "repeated_failures" # critical
|
||||
assert "prose_phantom_refs" in kinds
|
||||
|
|
|
|||
|
|
@ -89,6 +89,42 @@ agent:
|
|||
assert required in pinned
|
||||
|
||||
|
||||
def test_default_spawn_never_boots_the_tui(monkeypatch, tmp_path):
|
||||
"""Workers are headless: an inherited HERMES_TUI=1 (or a TUI-default
|
||||
config) must not send the quiet chat run into the Ink TUI, whose no-TTY
|
||||
bail-out exits 0 without doing the task — every attempt then ends in
|
||||
"protocol violation". The spawn pins --cli (highest-precedence interface
|
||||
flag) and strips HERMES_TUI from the child env."""
|
||||
root = tmp_path / ".hermes"
|
||||
(root / "profiles" / "elias").mkdir(parents=True)
|
||||
root.joinpath("config.yaml").write_text("display:\n interface: tui\n", encoding="utf-8")
|
||||
monkeypatch.setenv("HERMES_HOME", str(root))
|
||||
monkeypatch.setenv("HERMES_TUI", "1")
|
||||
|
||||
from hermes_cli import kanban_db as kb
|
||||
|
||||
monkeypatch.setattr(kb, "_resolve_hermes_argv", lambda: ["hermes"])
|
||||
|
||||
captured = {}
|
||||
|
||||
class FakeProc:
|
||||
pid = 4243
|
||||
|
||||
def fake_popen(cmd, *args, **kwargs):
|
||||
captured["cmd"] = list(cmd)
|
||||
captured["env"] = dict(kwargs.get("env") or {})
|
||||
return FakeProc()
|
||||
|
||||
monkeypatch.setattr(subprocess, "Popen", fake_popen)
|
||||
|
||||
workspace = tmp_path / "workspace"
|
||||
workspace.mkdir()
|
||||
kb._default_spawn(_make_task(kb, assignee="elias"), str(workspace))
|
||||
|
||||
assert "--cli" in captured["cmd"]
|
||||
assert "HERMES_TUI" not in captured["env"]
|
||||
|
||||
|
||||
def test_resolve_worker_cli_toolsets_uses_profile_home_not_parent_config(monkeypatch, tmp_path):
|
||||
root = tmp_path / ".hermes"
|
||||
profile = root / "profiles" / "elias"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue