diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 6150b141537..518e74eb064 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -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 diff --git a/hermes_cli/kanban_diagnostics.py b/hermes_cli/kanban_diagnostics.py index 81da4c19156..8173f1e3338 100644 --- a/hermes_cli/kanban_diagnostics.py +++ b/hermes_cli/kanban_diagnostics.py @@ -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), diff --git a/hermes_cli/main.py b/hermes_cli/main.py index f450a76666f..ddaa3ee3198 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -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 diff --git a/tests/hermes_cli/test_default_interface_resolution.py b/tests/hermes_cli/test_default_interface_resolution.py index c04f8093b29..85208638ec6 100644 --- a/tests/hermes_cli/test_default_interface_resolution.py +++ b/tests/hermes_cli/test_default_interface_resolution.py @@ -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 diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index 05de4a913eb..a9e61f41690 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -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: diff --git a/tests/hermes_cli/test_kanban_diagnostics.py b/tests/hermes_cli/test_kanban_diagnostics.py index 2de4933dc63..474cfd07611 100644 --- a/tests/hermes_cli/test_kanban_diagnostics.py +++ b/tests/hermes_cli/test_kanban_diagnostics.py @@ -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 diff --git a/tests/hermes_cli/test_kanban_worker_spawn_toolsets.py b/tests/hermes_cli/test_kanban_worker_spawn_toolsets.py index 7469a7bf057..da9b6b957b2 100644 --- a/tests/hermes_cli/test_kanban_worker_spawn_toolsets.py +++ b/tests/hermes_cli/test_kanban_worker_spawn_toolsets.py @@ -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"