From 928552a5d121673a989e44166b5d38f2d957f7f5 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Sat, 6 Jun 2026 06:47:32 -0700 Subject: [PATCH 1/5] schema: express conditional-required constraints for grok-style tool callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two tool schemas advertised `required: []` (or just `["mode"]`) but enforced additional requirements in the Python handler. Anthropic-tuned models like Claude read the prose "REQUIRED PARAMETERS: …" hints and emit the fields. xAI grok-4.3 reads the JSON Schema literally and omits both/all of them, then loops on the Python validator's `tool_error`. This blocked autonomous orchestration profiles (e.g. JARVIS in the 1Team-Engineering hermes-jarvis fleet) running on grok from completing or patching anything. Fix: express the real constraints in the schema using JSON Schema 2020-12's `anyOf` / `oneOf`. Both branches use only `required` so the constraint is honored by every conformant validator. - kanban_complete: anyOf summary or result (matches handler line 543) - patch: oneOf mode=replace (path+old_string+new_string) | mode=patch (patch) (matches handler in _handle_patch) Schema validation tested with jsonschema 4.25.1 (Draft 2020-12): - kanban_complete: 5/5 cases match expected validity - patch: 6/6 cases match expected validity Handlers unchanged — they still tolerate both shapes for CLI legacy callers that bypass the schema layer (e.g. `hermes kanban complete --result "..."`). Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/file_tools.py | 16 ++++++++++++++++ tools/kanban_tools.py | 9 +++++++++ 2 files changed, 25 insertions(+) diff --git a/tools/file_tools.py b/tools/file_tools.py index 45186ae6cf276..47807e54ba3a4 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -1452,6 +1452,22 @@ def _check_file_reqs(): }, }, "required": ["mode"], + # The handler enforces mode-conditional requirements (see _handle_patch: + # mode=replace needs path+old_string+new_string; mode=patch needs patch). + # Express that in the schema so tool-using LLMs that honor JSON Schema + # literally (e.g. grok-4.3) emit the required fields up-front instead + # of looping on python validator errors. 1Team-Engineering/hermes-agent + # patch: grok-tool-call-tolerance. + "oneOf": [ + { + "properties": {"mode": {"const": "replace"}}, + "required": ["mode", "path", "old_string", "new_string"], + }, + { + "properties": {"mode": {"const": "patch"}}, + "required": ["mode", "patch"], + }, + ], }, } diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 67157dfc1c622..20a522f90a4ba 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -1066,6 +1066,15 @@ def _board_schema_prop() -> dict[str, str]: "board": _board_schema_prop(), }, "required": [], + # The handler enforces "summary or result" (line ~543). Express that + # constraint in the schema itself so tool-using LLMs that read JSON + # Schema literally (e.g. grok-4.3) emit the field instead of silently + # omitting both and then looping on the python validator error. + # 1Team-Engineering/hermes-agent patch: grok-tool-call-tolerance. + "anyOf": [ + {"required": ["summary"]}, + {"required": ["result"]}, + ], }, } From 2aed6f5cc64038f24956d146d9b2c74b42a13389 Mon Sep 17 00:00:00 2001 From: jarvis-1team Date: Sun, 7 Jun 2026 16:33:17 -0700 Subject: [PATCH 2/5] =?UTF-8?q?feat(gateway):=20dispatcher=20heartbeat=20?= =?UTF-8?q?=E2=80=94=20detect=20silent=20stalls=20from=20outside=20(#8)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #7. Problem The kanban dispatcher loop in `_kanban_dispatcher_watcher` can silently stop cycling while the gateway process stays alive — observed twice in one day (2026-06-07). Cause is unclear (possibly hung `dispatch_once()`, sqlite lock, or event-loop livelock). Without instrumentation we can't tell a dead loop from an idle gateway. This unblocks investigation of #5 (HTTP 429 → delayed retry) and #6 (provider auth crashes → skip-not-stall), both of which were blocked on "is the dispatcher even running?" being answerable. Solution Write a heartbeat JSON to `$HERMES_HOME/state/dispatcher_health.json` at the end of every dispatcher cycle (success AND exception paths AND cancellation). Schema (v1, stable contract): schema_version: 1 last_cycle_ts: float # unix seconds, end of cycle last_cycle_iso: str # UTC ISO 8601 with Z suffix cycle_started_at: float cycle_duration_seconds: float interval_seconds: float # configured cadence (default 60) cycles_since_start: int # monotonic; first cycle = 1, 0 means "never wrote" any_spawned_this_cycle: bool spawned_total_this_cycle: int # count across all boards ready_pending: bool # ready queue non-empty consecutive_bad_ticks: int # mirrors the existing HEALTH_WINDOW counter gateway_pid: int cycle_error: str | null # exception text if cycle errored Detection rule for monitors: stall = time.time() - last_cycle_ts > 2 × interval_seconds dead = last_cycle_ts > 5 minutes ago AND gateway PID still alive Implementation - New method `GatewayRunner._write_dispatcher_heartbeat` (gateway/run.py). Uses existing `atomic_json_write` for crash-safe writes. - Called from `_kanban_dispatcher_watcher` at end of every iteration. - Heartbeat-write failures are wrapped in try/except so they can NEVER kill the dispatcher (heartbeat is a diagnostic, not a dependency). - Cancellation path also writes a final heartbeat with cycle_error="cancelled" before re-raising — so monitors can distinguish clean shutdown from crash. - Locals (`cycles_since_start`, `any_spawned`, etc.) initialized at top of the loop body BEFORE any try block so they're defined for the heartbeat call even if zombie-reap or main tick throws. Tests (5/5 passing) - Schema-v1 contract pinned (all 13 keys, types, ISO Z suffix) - Cycle-error path recorded correctly - Two writes overwrite (not append) - First-cycle-is-1 contract (monitors treat 0 as "never wrote") - Auto-creates `state/` dir if missing Follow-up (separate issues) - Extend `hermes gateway status` to read this file and show stall age - Schema v2: add `gateway_started_at` for uptime computation without ps - Optional Prometheus textfile exposition for node_exporter setups Co-authored-by: Jarvis Co-authored-by: Claude Opus 4.7 (1M context) --- gateway/run.py | 107 +++++++++++- tests/gateway/test_dispatcher_heartbeat.py | 186 +++++++++++++++++++++ 2 files changed, 291 insertions(+), 2 deletions(-) create mode 100644 tests/gateway/test_dispatcher_heartbeat.py diff --git a/gateway/run.py b/gateway/run.py index 18aa5ef175fc8..ca6556538fdfd 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -5744,6 +5744,63 @@ def _add(path: str) -> None: path, exc, ) + def _write_dispatcher_heartbeat( + self, + *, + heartbeat_path: Path, + interval: float, + cycles_since_start: int, + cycle_started_at: float, + any_spawned: bool, + spawned_total: int, + ready_pending: bool, + bad_ticks: int, + cycle_error: str | None, + ) -> None: + """Write a one-shot heartbeat snapshot to disk for external observers. + + Called at the end of every dispatcher cycle (success OR failure). + External tools (`hermes gateway status`, monitoring scripts) read + this to detect silent stalls — the gateway PID may stay alive but + the dispatch loop can stop cycling (hermes-agent#6, #7). + + Write is atomic (temp + rename) and never raises — the caller wraps + in try/except so heartbeat-write failures cannot kill the dispatcher. + + Schema (stable; tools may grow tolerant of new fields): + schema_version: int (currently 1) + last_cycle_ts: float (unix seconds, end of cycle) + last_cycle_iso: str (UTC ISO 8601) + cycle_started_at: float (unix seconds, start of cycle) + cycle_duration_seconds: float + interval_seconds: float (configured cadence) + cycles_since_start: int (monotonic counter) + any_spawned_this_cycle: bool + spawned_total_this_cycle: int (count across all boards) + ready_pending: bool (whether the ready queue had work this cycle) + consecutive_bad_ticks: int + gateway_pid: int + cycle_error: str | null (exception text if the cycle errored) + """ + from datetime import timezone as _tz + now = time.time() + payload = { + "schema_version": 1, + "last_cycle_ts": now, + "last_cycle_iso": datetime.fromtimestamp(now, tz=_tz.utc).isoformat(timespec="seconds").replace("+00:00", "Z"), + "cycle_started_at": cycle_started_at, + "cycle_duration_seconds": max(0.0, now - cycle_started_at), + "interval_seconds": float(interval), + "cycles_since_start": cycles_since_start, + "any_spawned_this_cycle": any_spawned, + "spawned_total_this_cycle": spawned_total, + "ready_pending": ready_pending, + "consecutive_bad_ticks": bad_ticks, + "gateway_pid": os.getpid(), + "cycle_error": cycle_error, + } + atomic_json_write(heartbeat_path, payload) + async def _kanban_dispatcher_watcher(self) -> None: """Embedded kanban dispatcher — one tick every `dispatch_interval_seconds`. @@ -5918,6 +5975,13 @@ async def _kanban_dispatcher_watcher(self) -> None: HEALTH_WINDOW = 6 bad_ticks = 0 last_warn_at = 0 + # Dispatcher heartbeat — written to disk every cycle so external + # observers can detect silent stalls (the gateway PID stays alive + # but the dispatch loop has stopped cycling). See `hermes gateway + # status`. Path is HERMES_HOME/state/dispatcher_health.json. + cycles_since_start = 0 + last_cycle_started_at = 0.0 + _heartbeat_path = _hermes_home / "state" / "dispatcher_health.json" # Avoid hot-looping corrupt-looking board DBs, but do not suppress # same-fingerprint retries forever: transient WAL/open races can # surface as "database disk image is malformed" for one tick. @@ -6187,6 +6251,12 @@ def _auto_decompose_tick() -> int: "kanban dispatcher: embedded in gateway (interval=%.1fs)", interval ) while self._running: + cycles_since_start += 1 + last_cycle_started_at = time.time() + cycle_error: str | None = None + any_spawned = False + ready_pending = False + spawned_total = 0 try: # Reap zombie children before per-board work so a board DB # failure cannot block cleanup of unrelated workers. @@ -6204,10 +6274,10 @@ def _auto_decompose_tick() -> int: if auto_decompose_enabled: await asyncio.to_thread(_auto_decompose_tick) results = await asyncio.to_thread(_tick_once) - any_spawned = False for slug, res in (results or []): if res is not None and getattr(res, "spawned", None): any_spawned = True + spawned_total += len(res.spawned) # Quiet by default — only log when something actually # happened, so an idle gateway stays silent. logger.info( @@ -6240,9 +6310,42 @@ def _auto_decompose_tick() -> int: last_warn_at = now except asyncio.CancelledError: logger.debug("kanban dispatcher: cancelled") + # Write a final heartbeat noting the cancellation, then re-raise. + self._write_dispatcher_heartbeat( + heartbeat_path=_heartbeat_path, + interval=interval, + cycles_since_start=cycles_since_start, + cycle_started_at=last_cycle_started_at, + any_spawned=any_spawned, + spawned_total=spawned_total, + ready_pending=ready_pending, + bad_ticks=bad_ticks, + cycle_error="cancelled", + ) raise - except Exception: + except Exception as e: logger.exception("kanban dispatcher: unexpected watcher error") + cycle_error = f"{type(e).__name__}: {e}" + + # Write heartbeat every cycle — success OR exception. + # External observers (hermes gateway status / monitoring) read this + # to detect silent stalls where the gateway PID is alive but the + # dispatcher has stopped cycling. + try: + self._write_dispatcher_heartbeat( + heartbeat_path=_heartbeat_path, + interval=interval, + cycles_since_start=cycles_since_start, + cycle_started_at=last_cycle_started_at, + any_spawned=any_spawned, + spawned_total=spawned_total, + ready_pending=ready_pending, + bad_ticks=bad_ticks, + cycle_error=cycle_error, + ) + except Exception: + # Heartbeat-write failure must NEVER kill the dispatcher. + logger.exception("kanban dispatcher: heartbeat write failed (non-fatal)") # Sleep in 1s slices so shutdown is snappy — otherwise a stop() # waits up to `interval` seconds for the current sleep to finish. diff --git a/tests/gateway/test_dispatcher_heartbeat.py b/tests/gateway/test_dispatcher_heartbeat.py new file mode 100644 index 0000000000000..14eb087f0726c --- /dev/null +++ b/tests/gateway/test_dispatcher_heartbeat.py @@ -0,0 +1,186 @@ +"""Tests for the kanban dispatcher heartbeat written by gateway.run.GatewayRunner. + +The heartbeat is a stable contract for external observers (monitoring scripts, +future `hermes gateway status` integration). These tests pin the schema so +silent contract breaks fail loudly. +""" +from __future__ import annotations + +import json +import os +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + + +@pytest.fixture +def runner(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + """Construct a minimal GatewayRunner with a tmp HERMES_HOME.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + # Importing run.py is heavy (loads adapters); import here so the fixture + # picks up the env override. + from gateway import run as gateway_run + + # Reload module so the module-level _hermes_home picks up our tmp path. + import importlib + + importlib.reload(gateway_run) + + instance = gateway_run.GatewayRunner.__new__(gateway_run.GatewayRunner) + return instance, gateway_run, tmp_path + + +def test_heartbeat_writes_expected_schema_v1(runner) -> None: + instance, _gateway_run, tmp_path = runner + hb_path = tmp_path / "state" / "dispatcher_health.json" + + instance._write_dispatcher_heartbeat( + heartbeat_path=hb_path, + interval=60.0, + cycles_since_start=5, + cycle_started_at=1_780_000_000.0, + any_spawned=True, + spawned_total=3, + ready_pending=False, + bad_ticks=0, + cycle_error=None, + ) + + assert hb_path.exists(), "heartbeat file should exist after write" + data = json.loads(hb_path.read_text()) + + # Schema-v1 required keys — any missing key = silent contract break. + required = { + "schema_version", + "last_cycle_ts", + "last_cycle_iso", + "cycle_started_at", + "cycle_duration_seconds", + "interval_seconds", + "cycles_since_start", + "any_spawned_this_cycle", + "spawned_total_this_cycle", + "ready_pending", + "consecutive_bad_ticks", + "gateway_pid", + "cycle_error", + } + assert required <= set(data.keys()), f"missing keys: {required - set(data.keys())}" + + # Type checks (catches refactors that change shape). + assert data["schema_version"] == 1 + assert isinstance(data["last_cycle_ts"], (int, float)) + assert data["last_cycle_iso"].endswith("Z"), "iso should be UTC with Z suffix" + assert data["interval_seconds"] == 60.0 + assert data["cycles_since_start"] == 5 + assert data["any_spawned_this_cycle"] is True + assert data["spawned_total_this_cycle"] == 3 + assert data["ready_pending"] is False + assert data["consecutive_bad_ticks"] == 0 + assert data["gateway_pid"] == os.getpid() + assert data["cycle_error"] is None + + +def test_heartbeat_records_cycle_error(runner) -> None: + instance, _gateway_run, tmp_path = runner + hb_path = tmp_path / "state" / "dispatcher_health.json" + + instance._write_dispatcher_heartbeat( + heartbeat_path=hb_path, + interval=60.0, + cycles_since_start=1, + cycle_started_at=1_780_000_000.0, + any_spawned=False, + spawned_total=0, + ready_pending=True, + bad_ticks=2, + cycle_error="RuntimeError: simulated provider auth crash", + ) + + data = json.loads(hb_path.read_text()) + assert data["cycle_error"] == "RuntimeError: simulated provider auth crash" + assert data["consecutive_bad_ticks"] == 2 + assert data["any_spawned_this_cycle"] is False + assert data["ready_pending"] is True + + +def test_heartbeat_overwrites_previous_atomic(runner) -> None: + """Second write must replace the first, not append.""" + instance, _gateway_run, tmp_path = runner + hb_path = tmp_path / "state" / "dispatcher_health.json" + + instance._write_dispatcher_heartbeat( + heartbeat_path=hb_path, + interval=60.0, + cycles_since_start=1, + cycle_started_at=1_780_000_000.0, + any_spawned=False, + spawned_total=0, + ready_pending=False, + bad_ticks=0, + cycle_error=None, + ) + instance._write_dispatcher_heartbeat( + heartbeat_path=hb_path, + interval=60.0, + cycles_since_start=2, + cycle_started_at=1_780_000_060.0, + any_spawned=True, + spawned_total=1, + ready_pending=False, + bad_ticks=0, + cycle_error=None, + ) + + data = json.loads(hb_path.read_text()) + assert data["cycles_since_start"] == 2, "second write should replace first" + assert data["any_spawned_this_cycle"] is True + + +def test_heartbeat_counter_starts_at_one_first_cycle(runner) -> None: + """Contract pinned: the dispatcher's watcher increments cycles_since_start + BEFORE the first cycle body runs, so the first heartbeat shows 1, not 0. + Monitors should expect the smallest valid value to be 1; 0 means "never wrote." + """ + instance, _gateway_run, tmp_path = runner + hb_path = tmp_path / "state" / "dispatcher_health.json" + + # Simulate what the watcher does at the very first iteration. + instance._write_dispatcher_heartbeat( + heartbeat_path=hb_path, + interval=60.0, + cycles_since_start=1, # ← contract: first cycle is 1 + cycle_started_at=1_780_000_000.0, + any_spawned=False, + spawned_total=0, + ready_pending=False, + bad_ticks=0, + cycle_error=None, + ) + + data = json.loads(hb_path.read_text()) + assert data["cycles_since_start"] == 1, ( + "first heartbeat must report cycles_since_start=1 — monitors treat 0 as 'never wrote'" + ) + + +def test_heartbeat_creates_parent_state_dir_if_missing(runner) -> None: + """state/ subdir may not exist on first gateway run — atomic_json_write should mkdir.""" + instance, _gateway_run, tmp_path = runner + hb_path = tmp_path / "state" / "deep" / "nested" / "dispatcher_health.json" + assert not hb_path.parent.exists() + + instance._write_dispatcher_heartbeat( + heartbeat_path=hb_path, + interval=60.0, + cycles_since_start=1, + cycle_started_at=1_780_000_000.0, + any_spawned=False, + spawned_total=0, + ready_pending=False, + bad_ticks=0, + cycle_error=None, + ) + + assert hb_path.exists() From ae8707b1ab70494c2f959e0bd6e9cbc2e4f72a81 Mon Sep 17 00:00:00 2001 From: jarvis-1team Date: Sun, 7 Jun 2026 18:01:01 -0700 Subject: [PATCH 3/5] fix(oneshot): honor fallback_providers chain during worker startup, not just runtime (#9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #6. Problem Worker startup calls `resolve_runtime_provider` to acquire credentials for the primary provider. If that raises AuthError (xAI OAuth token expired, Anthropic logged out, Codex revoked), the worker crashes before AIAgent's runtime fallback loop ever gets a chance — even though the user has explicitly configured a fallback chain for exactly this case. Observed in the v6.6 incident 2026-06-07: xAI OAuth token went missing mid-session and every subsequent worker crashed at startup despite having `fallback_providers: [openai-codex/gpt-5.5, xai-oauth/grok-4.3]` configured. Solution New helper `_resolve_runtime_with_fallback` wraps the primary-resolution call. On AuthError, iterates the configured fallback chain (read once from `get_fallback_chain(cfg)`) until one succeeds. If all fail, re-raises the LAST AuthError so cli.py's exit handling can surface it. Three safety bounds preserved (informed by code-review): 1. **Explicit CLI pin** — `hermes -z --model X --provider Y ...` should NOT silently downgrade. When `model` OR `provider` was a non-empty CLI arg, the helper re-raises primary AuthError verbatim, no fallback attempt. 2. **Rate-limit AuthError on primary** — falling through to other providers would burn their quota in milliseconds (the "quota amplification" footgun). Detected via existing `is_rate_limited_auth_error()` — re-raise immediately; existing rate-limit handling (cli.py exit 75) gets the task requeued. 3. **Remaining-chain handoff to AIAgent** — when fallback lands on chain entry [N], AIAgent's runtime fallback loop should only see entries AFTER N (not the dead primary, not the entry we just used). The helper now returns `(runtime, effective_model, landed_at_index, remaining_chain)` and the caller passes `remaining` to AIAgent's `fallback_model`. Implementation - `hermes_cli/oneshot.py:33-110` — new helper (testable at module level). - `hermes_cli/oneshot.py:439-460` — call site updated; reads chain once, detects explicit_pin from CLI args, passes remaining_chain to AIAgent. - AIAgent receives the correctly-sliced chain via `fallback_model=_fb`, preserving existing runtime-fallback semantics for mid-conversation failures. Tests (9/9 passing) — tests/cli/test_oneshot_runtime_fallback.py - primary succeeds → no fallback attempted, full chain preserved for AIAgent - primary fails, first fallback succeeds → effective_model advances, remaining_chain sliced correctly - two failures → third succeeds, slicing correct - all fail → LAST AuthError propagates (not primary's) - empty chain → primary error verbatim - fallback without model → effective_model preserved - explicit_pin=True → no fallback, primary error verbatim - rate-limit AuthError → no fallback, primary error verbatim - same provider in chain → no infinite loop, advances to next entry Code-review pre-merge: reviewer caught silent-downgrade regression, stale chain handoff, and quota-amplification footgun. All three addressed. Follow-up (separate issues, not blocking) - Consider applying the same pattern to `gateway/run.py:_resolve_runtime_agent_kwargs` and `cli.py:4881-4914` for a consistent worker-startup contract across surfaces. - Optional: emit a metric/heartbeat counter when fallback fires so we can detect "constantly failing primary" silently. Co-authored-by: Jarvis Co-authored-by: Claude Opus 4.7 (1M context) --- hermes_cli/oneshot.py | 152 ++++++++++- tests/cli/test_oneshot_runtime_fallback.py | 281 +++++++++++++++++++++ 2 files changed, 426 insertions(+), 7 deletions(-) create mode 100644 tests/cli/test_oneshot_runtime_fallback.py diff --git a/hermes_cli/oneshot.py b/hermes_cli/oneshot.py index f66d71c62e6d9..f94ef0f41bedf 100644 --- a/hermes_cli/oneshot.py +++ b/hermes_cli/oneshot.py @@ -27,9 +27,131 @@ from contextlib import redirect_stderr, redirect_stdout from typing import Optional +from hermes_cli.auth import AuthError from hermes_cli.fallback_config import get_fallback_chain +def _resolve_runtime_with_fallback( + *, + resolve_runtime_provider, + effective_provider: Optional[str], + effective_model: str, + explicit_base_url: Optional[str], + fallback_chain: list, + logger: logging.Logger, + explicit_pin: bool = False, +) -> tuple[dict, str, int, list]: + """Resolve runtime credentials with fallback-chain tolerance. + + Tries the primary provider first. On AuthError, iterates the configured + fallback chain (typically Claude → Codex → Grok 4.3 per team policy) until + one succeeds. Raises the LAST AuthError if every fallback also fails. + + This closes hermes-agent#6: a single provider's auth failure (e.g. xAI OAuth + token expired) used to crash worker startup before AIAgent's runtime + fallback loop could ever try the next provider. The fallback chain was + configured precisely for this case but was only honored AFTER successful + credential resolution, not during it. + + Special cases: + * ``explicit_pin=True`` — the caller pinned model AND/OR provider on + the CLI (e.g. ``hermes -z --model grok-4.3 --provider xai-oauth``). + Silent downgrade would surprise them, so we re-raise the primary + AuthError verbatim with no fallback attempt. + * Rate-limit AuthError on the primary — falling through wastes the + quota of every other configured provider in ~milliseconds (the + "quota amplification" footgun). Re-raise primary error verbatim; + existing rate-limit handling (cli.py exit code 75) takes over. + + Returns: + (runtime_dict, effective_model, landed_at_index, remaining_chain) + + ``landed_at_index`` is -1 if primary succeeded, else the 0-based + index into the original ``fallback_chain`` that resolved. Callers + should pass ``remaining_chain`` (the entries AFTER the landed-on + one) to AIAgent's ``fallback_model`` to avoid AIAgent re-attempting + the already-dead primary or the entry we just used. + """ + # Lazy-import to avoid a hard dependency at module load time (the rate-limit + # detector lives in hermes_cli.auth alongside AuthError). + try: + from hermes_cli.auth import is_rate_limited_auth_error + except ImportError: # pragma: no cover - defensive only + is_rate_limited_auth_error = lambda _exc: False # noqa: E731 + + try: + runtime = resolve_runtime_provider( + requested=effective_provider, + target_model=effective_model or None, + explicit_base_url=explicit_base_url, + ) + return runtime, effective_model, -1, list(fallback_chain) + except AuthError as primary_exc: + if explicit_pin: + # User explicitly pinned model/provider on the CLI — they would + # rather see the failure than get silently downgraded to another + # provider. Preserve that contract. + logger.warning( + "primary provider %r auth failed and caller pinned model/provider; " + "not attempting fallback (use auto-detection to enable fallback)", + effective_provider, + ) + raise + if is_rate_limited_auth_error(primary_exc): + # Rate limits are recoverable on the same provider after a cooldown. + # Falling through to the chain would burn quotas across every + # provider in milliseconds — the "quota amplification" footgun. + # Existing rate-limit handling (cli.py:16128 → exit code 75) gets + # this task requeued; let it do its job. + logger.warning( + "primary provider %r is rate-limited; not attempting fallback " + "(letting rate-limit retry path handle it)", + effective_provider, + ) + raise + if not fallback_chain: + # Nothing to fall back to — propagate original error verbatim. + raise + logger.warning( + "primary provider %r auth failed during worker startup (%s); " + "trying %d fallback provider(s) before giving up", + effective_provider, primary_exc, len(fallback_chain), + ) + last_exc: Exception = primary_exc + for fb_idx, fb in enumerate(fallback_chain): + fb_provider = (fb.get("provider") or "").strip() or None + fb_model = (fb.get("model") or "").strip() or None + fb_base_url = (fb.get("base_url") or "").strip() or None + try: + runtime = resolve_runtime_provider( + requested=fb_provider, + target_model=fb_model, + explicit_base_url=fb_base_url, + ) + # Successful fallback — update effective_model so AIAgent sees + # the right model for the provider we landed on. + new_effective_model = fb_model if fb_model else effective_model + logger.info( + "worker startup recovered: fallback[%d] %s/%s healthy", + fb_idx, fb_provider or "auto", fb_model or "", + ) + # Slice the chain so AIAgent's own runtime fallback loop only + # sees entries AFTER the one we just landed on. Avoids + # re-attempting the dead primary OR the entry we just used. + remaining = list(fallback_chain[fb_idx + 1:]) + return runtime, new_effective_model, fb_idx, remaining + except AuthError as fb_exc: + logger.debug( + "fallback[%d] %s/%s auth failed: %s", + fb_idx, fb_provider or "auto", fb_model or "", fb_exc, + ) + last_exc = fb_exc + continue + # All fallbacks exhausted. Re-raise the last AuthError; cli.py's + # generic exit handling will surface it as a non-zero exit. + raise last_exc + + def _normalize_toolsets(toolsets: object = None) -> list[str] | None: if not toolsets: return None @@ -314,10 +436,24 @@ def _run_agent( if detected: effective_provider, effective_model = detected - runtime = resolve_runtime_provider( - requested=effective_provider, - target_model=effective_model or None, - explicit_base_url=explicit_base_url_from_alias, + # Hoist the fallback chain once and reuse — avoids two reads of cfg and any + # TOCTOU window if cfg were mutated between calls. + _fb_chain = get_fallback_chain(cfg) or [] + # Caller pinned model/provider explicitly on the CLI (e.g. + # `hermes -z --model grok-4.3 --provider xai-oauth ...`) → silent + # downgrade to a fallback would surprise them. Detect from the original + # args, not the resolved effective_* values. + _explicit_pin = bool((model or "").strip() or (provider or "").strip()) + runtime, effective_model, _landed_idx, _remaining_fb = ( + _resolve_runtime_with_fallback( + resolve_runtime_provider=resolve_runtime_provider, + effective_provider=effective_provider, + effective_model=effective_model, + explicit_base_url=explicit_base_url_from_alias, + fallback_chain=_fb_chain, + logger=logging.getLogger(__name__), + explicit_pin=_explicit_pin, + ) ) # Pull in explicit toolsets when provided; otherwise use whatever the user @@ -328,9 +464,11 @@ def _run_agent( toolsets_list = sorted(_get_platform_tools(cfg, "cli")) session_db = _create_session_db_for_oneshot() - # Read the effective fallback chain from profile config so oneshot workers - # honour the same merge semantics as interactive CLI and gateway sessions. - _fb = get_fallback_chain(cfg) + # If we landed on a fallback during startup resolution, hand AIAgent only + # the entries AFTER the one we used — avoids re-trying the dead primary + # or the entry we just succeeded with when AIAgent's runtime fallback + # loop kicks in mid-conversation. + _fb = _remaining_fb agent = AIAgent( api_key=runtime.get("api_key"), diff --git a/tests/cli/test_oneshot_runtime_fallback.py b/tests/cli/test_oneshot_runtime_fallback.py new file mode 100644 index 0000000000000..8ebee737fb310 --- /dev/null +++ b/tests/cli/test_oneshot_runtime_fallback.py @@ -0,0 +1,281 @@ +"""Tests for hermes_cli.oneshot._resolve_runtime_with_fallback. + +Closes hermes-agent#6: when the worker's primary provider auth fails during +startup (e.g. xAI OAuth token expired, Anthropic logged out, Codex revoked), +the configured fallback_providers chain should be tried before the worker +gives up. The fallback chain was previously only honored AFTER successful +credential resolution, not during initial startup. + +Three safety bounds preserved: +1. Explicit CLI pin (--model/--provider) → no silent downgrade, raise. +2. Rate-limit AuthError on primary → don't burn the chain, let rate-limit retry. +3. Successful fallback → AIAgent only sees the REMAINING chain entries (no + re-attempt of dead primary or already-used entry). +""" +from __future__ import annotations + +import logging + +import pytest + +from hermes_cli.auth import AuthError +from hermes_cli.oneshot import _resolve_runtime_with_fallback + + +def _runtime(provider: str, model: str = "x", api_key: str = "k") -> dict: + return { + "provider": provider, + "model": model, + "api_key": api_key, + "base_url": "https://example.test", + "api_mode": "openai_chat", + } + + +def test_primary_succeeds_no_fallback_attempted() -> None: + calls: list[tuple] = [] + + def fake_resolve(*, requested, target_model, explicit_base_url): + calls.append((requested, target_model, explicit_base_url)) + return _runtime(provider=requested or "auto", model=target_model or "x") + + runtime, model, landed_idx, remaining = _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="anthropic", + effective_model="claude-opus-4-7", + explicit_base_url=None, + fallback_chain=[{"provider": "xai-oauth", "model": "grok-4.3"}], + logger=logging.getLogger("test"), + ) + assert runtime["provider"] == "anthropic" + assert model == "claude-opus-4-7" + assert landed_idx == -1, "primary succeeded → landed_idx must be -1" + assert remaining == [{"provider": "xai-oauth", "model": "grok-4.3"}], ( + "primary succeeded → remaining chain unchanged for AIAgent's runtime loop" + ) + assert len(calls) == 1, "fallback should not have been touched" + + +def test_primary_auth_fails_first_fallback_succeeds() -> None: + calls: list[tuple] = [] + + def fake_resolve(*, requested, target_model, explicit_base_url): + calls.append((requested, target_model, explicit_base_url)) + if requested == "xai-oauth": + raise AuthError( + "xAI OAuth state is missing access_token", + provider="xai-oauth", + code="xai_auth_missing_access_token", + ) + return _runtime(provider=requested or "auto", model=target_model or "x") + + runtime, model, landed_idx, remaining = _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="xai-oauth", + effective_model="grok-4.3", + explicit_base_url=None, + fallback_chain=[ + {"provider": "openai-codex", "model": "gpt-5.5"}, + {"provider": "anthropic", "model": "claude-opus-4-7"}, + ], + logger=logging.getLogger("test"), + ) + assert runtime["provider"] == "openai-codex" + assert model == "gpt-5.5", "effective_model should advance to fallback's model" + assert landed_idx == 0 + assert remaining == [{"provider": "anthropic", "model": "claude-opus-4-7"}], ( + "AIAgent should only see fallbacks AFTER the one we just used" + ) + assert len(calls) == 2, "primary tried, then first fallback succeeded" + + +def test_primary_and_first_fallback_fail_second_fallback_succeeds() -> None: + calls: list[str] = [] + + def fake_resolve(*, requested, target_model, explicit_base_url): + calls.append(requested or "auto") + if requested in ("xai-oauth", "openai-codex"): + raise AuthError(f"{requested} auth dead", provider=requested) + return _runtime(provider=requested or "auto", model=target_model or "x") + + runtime, model, landed_idx, remaining = _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="xai-oauth", + effective_model="grok-4.3", + explicit_base_url=None, + fallback_chain=[ + {"provider": "openai-codex", "model": "gpt-5.5"}, + {"provider": "anthropic", "model": "claude-opus-4-7"}, + ], + logger=logging.getLogger("test"), + ) + assert runtime["provider"] == "anthropic" + assert model == "claude-opus-4-7" + assert calls == ["xai-oauth", "openai-codex", "anthropic"] + assert landed_idx == 1 + assert remaining == [], "landed on last entry → nothing left for AIAgent" + + +def test_all_providers_fail_last_auth_error_propagates() -> None: + last_seen = {"provider": None} + + def fake_resolve(*, requested, target_model, explicit_base_url): + last_seen["provider"] = requested + raise AuthError(f"{requested} auth dead", provider=requested or "auto", code="xx") + + with pytest.raises(AuthError) as excinfo: + _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="xai-oauth", + effective_model="grok-4.3", + explicit_base_url=None, + fallback_chain=[ + {"provider": "openai-codex", "model": "gpt-5.5"}, + {"provider": "anthropic", "model": "claude-opus-4-7"}, + ], + logger=logging.getLogger("test"), + ) + # The raised exception should be from the LAST attempted provider so + # downstream consumers see the final failure mode (not the original primary). + assert last_seen["provider"] == "anthropic" + assert "anthropic" in str(excinfo.value) + + +def test_empty_fallback_chain_propagates_primary_error_verbatim() -> None: + primary_err = AuthError("primary dead", provider="xai-oauth", code="dead") + + def fake_resolve(*, requested, target_model, explicit_base_url): + raise primary_err + + with pytest.raises(AuthError) as excinfo: + _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="xai-oauth", + effective_model="grok-4.3", + explicit_base_url=None, + fallback_chain=[], + logger=logging.getLogger("test"), + ) + assert excinfo.value is primary_err, "no fallback configured → original error must surface unchanged" + + +def test_fallback_without_explicit_model_keeps_primary_model() -> None: + """A fallback entry that omits 'model' should leave effective_model alone.""" + def fake_resolve(*, requested, target_model, explicit_base_url): + if requested == "xai-oauth": + raise AuthError("dead", provider="xai-oauth") + return _runtime(provider=requested or "auto", model=target_model or "x") + + runtime, model, _, _ = _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="xai-oauth", + effective_model="grok-4.3", + explicit_base_url=None, + fallback_chain=[{"provider": "openai-codex"}], # no model key + logger=logging.getLogger("test"), + ) + assert runtime["provider"] == "openai-codex" + assert model == "grok-4.3", "effective_model unchanged when fallback has no model" + + +# === Safety-bound tests (closes P0 review findings) === + + +def test_explicit_pin_does_not_fall_back_even_with_chain() -> None: + """User pinned --model/--provider on CLI → silent downgrade would surprise. + Re-raise primary AuthError verbatim with no fallback attempt. + """ + primary_err = AuthError( + "xAI OAuth missing access_token", provider="xai-oauth", + code="xai_auth_missing_access_token", + ) + calls: list[str] = [] + + def fake_resolve(*, requested, target_model, explicit_base_url): + calls.append(requested or "auto") + raise primary_err + + with pytest.raises(AuthError) as excinfo: + _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="xai-oauth", + effective_model="grok-4.3", + explicit_base_url=None, + fallback_chain=[ + {"provider": "openai-codex", "model": "gpt-5.5"}, + ], + logger=logging.getLogger("test"), + explicit_pin=True, + ) + assert excinfo.value is primary_err + assert calls == ["xai-oauth"], "explicit_pin must skip fallback chain entirely" + + +def test_rate_limit_auth_error_on_primary_does_not_burn_chain() -> None: + """Rate-limit on primary should re-raise immediately — falling through + would burn the quota of every other configured provider in milliseconds. + Existing rate-limit handling (cli.py exit code 75) gets the task requeued. + """ + # Construct a real rate-limit AuthError that is_rate_limited_auth_error + # will recognize — needs `code=CODEX_RATE_LIMITED_CODE` and + # `relogin_required=False` per auth.py:746-750. + from hermes_cli.auth import CODEX_RATE_LIMITED_CODE + primary_err = AuthError( + "rate limit exceeded", + provider="openai-codex", + code=CODEX_RATE_LIMITED_CODE, + relogin_required=False, + ) + calls: list[str] = [] + + def fake_resolve(*, requested, target_model, explicit_base_url): + calls.append(requested or "auto") + raise primary_err + + with pytest.raises(AuthError) as excinfo: + _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="openai-codex", + effective_model="gpt-5.5", + explicit_base_url=None, + fallback_chain=[ + {"provider": "xai-oauth", "model": "grok-4.3"}, + {"provider": "anthropic", "model": "claude-opus-4-7"}, + ], + logger=logging.getLogger("test"), + ) + assert excinfo.value is primary_err + assert calls == ["openai-codex"], ( + "rate-limit on primary must skip fallback chain entirely — let rate-limit retry handle it" + ) + + +def test_same_provider_in_chain_no_infinite_loop() -> None: + """If the fallback chain (perhaps misconfigured) contains the same + provider that failed primary, the loop must not retry it forever. Each + chain entry is attempted at most once; a second AuthError on the same + provider just advances to the next entry. + """ + call_count: dict[str, int] = {} + + def fake_resolve(*, requested, target_model, explicit_base_url): + call_count[requested or "auto"] = call_count.get(requested or "auto", 0) + 1 + if requested == "xai-oauth": + raise AuthError("xai dead", provider="xai-oauth") + return _runtime(provider=requested or "auto", model=target_model or "x") + + runtime, _, landed_idx, _ = _resolve_runtime_with_fallback( + resolve_runtime_provider=fake_resolve, + effective_provider="xai-oauth", + effective_model="grok-4.3", + explicit_base_url=None, + fallback_chain=[ + {"provider": "xai-oauth", "model": "grok-4.3"}, # same as primary + {"provider": "openai-codex", "model": "gpt-5.5"}, + ], + logger=logging.getLogger("test"), + ) + assert runtime["provider"] == "openai-codex" + assert landed_idx == 1, "should have skipped misconfigured same-provider entry" + assert call_count.get("xai-oauth") == 2, "primary + chain[0] both tried, then stopped" + assert call_count.get("openai-codex") == 1 From bef7089f27883c158795965a00e45fc3deff6262 Mon Sep 17 00:00:00 2001 From: jarvis-1team Date: Sun, 7 Jun 2026 18:16:56 -0700 Subject: [PATCH 4/5] fix(cli): non-quiet `chat -q` rate-limit must exit 75 in kanban workers (#10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #5. Problem The non-quiet `hermes -p chat -q "..."` path (the actual invocation pattern Marvel team kanban workers use, e.g. `chat -q "work kanban task X"`) never applied the kanban EX_TEMPFAIL exit-code mapping. A rate-limited worker exited 0 by virtue of `cli.chat()` returning cleanly. The dispatcher's reap classifier then treated rc=0 as a "protocol violation" (worker exited cleanly without calling `kanban_complete` or `kanban_block`) and auto-blocked the task. This was the v6.6 incident root cause. Tony+Tchalla re-reviews (t_77ac35a7, t_a30a88db) hit Codex 429, exited 0, dispatcher auto-blocked, chain stalled 2 hours until quota reset — even though `KANBAN_RATE_LIMIT_EXIT_CODE = 75` and the mapping at cli.py existed (just only on the QUIET single-query path). Solution 1. Extract the exit-code mapping into `_worker_exit_code_from_result(result)` at module level. Returns 0 / 1 / 75 per these rules: - None or non-dict or success → 0 - Failure outside a kanban worker → 1 - Failure inside a worker with failure_reason ∈ {rate_limit, billing} → 75 - Any other failure inside a worker → 1 2. Refactor the quiet path (~line 16172) to call the helper instead of inline. 3. Make `cli.chat()` stash its run_conversation result on `self._last_run_result` so the non-quiet caller can inspect failure metadata after chat() returns only the response string. Reset to None at __init__ AND at start of every turn — invariant doesn't depend on early-return order (caught by code review). 4. Wire the non-quiet path (~line 16197) to call the helper after chat() returns. 5. Exception path inside chat() synthesizes `{failed:True, error:..., completed:False}` so single-query callers still apply mapping (rate-limit branch doesn't fire because no failure_reason — correctly falls through to exit 1). Tests (9/9 passing) — tests/cli/test_worker_exit_code_from_result.py - None result → 0 - Non-dict result → 0 - Success result → 0 - Failure outside kanban (no HERMES_KANBAN_TASK env) → 1, regardless of reason - Rate-limit inside kanban → KANBAN_RATE_LIMIT_EXIT_CODE (75) - Billing inside kanban → 75 (same recovery story) - Other failures inside kanban → 1 - Missing failure_reason field inside kanban → 1 (defensive) - Rate-limit without HERMES_KANBAN_TASK env → 1 (human CLI gets generic exit) Combined with #6 (worker-startup fallback) and #7 (dispatcher heartbeat), both already merged, the worker-startup → dispatcher-detect → next-retry loop is now operationally robust: - #7 — silent stalls detectable via heartbeat JSON - #6 — primary provider auth crash falls through to fallback chain at startup - #5 (this) — rate-limit failures exit 75 so dispatcher requeues without burning the retry counter Code-review pre-merge: reviewer caught a stale-stash bug (previous turn's `_last_run_result` leaking into a downstream consumer if chat() takes an early return path). Fixed by initializing the stash to None in __init__ AND resetting at the top of each chat() turn — invariant pinned. Follow-up (separate issues, not blocking) - `_print_exit_summary()` shows "Resume this session with:" even on rate-limit failure in the non-quiet path. Pre-existing; not introduced by this PR. - Non-quiet branch doesn't check `HERMES_KANBAN_GOAL_MODE` env var (only quiet path runs `_run_kanban_goal_loop_q`). If a goal_mode worker ever spawns via the non-quiet path, the goal loop silently skips. Pre-existing. Co-authored-by: Jarvis Co-authored-by: Claude Opus 4.7 (1M context) --- cli.py | 100 ++++++++++++----- .../cli/test_worker_exit_code_from_result.py | 104 ++++++++++++++++++ 2 files changed, 179 insertions(+), 25 deletions(-) create mode 100644 tests/cli/test_worker_exit_code_from_result.py diff --git a/cli.py b/cli.py index bb11587562ffc..cc0499a056bc9 100644 --- a/cli.py +++ b/cli.py @@ -3109,6 +3109,13 @@ def __init__( # Initialize Rich console self.console = Console() self.config = CLI_CONFIG + # Stash for the most recent chat() turn's run_conversation result. + # Single-query callers in main() inspect this AFTER chat() returns to + # decide the worker exit code (e.g. rate-limit → exit 75 per + # `_worker_exit_code_from_result`). Initialized here so the + # invariant "stash is reset at turn boundary" doesn't depend on + # any early returns inside chat(). + self._last_run_result = None self.compact = compact if compact is not None else CLI_CONFIG["display"].get("compact", False) # tool_progress: "off", "new", "all", "verbose" (from config.yaml display section) # YAML 1.1 parses bare `off` as boolean False — normalise to string. @@ -12350,6 +12357,12 @@ def chat(self, message, images: list = None) -> Optional[str]: ChatConsole().print(f"[{_accent_hex()}]{'─' * 40}[/]") print(flush=True) + # Reset stash at turn boundary so a downstream consumer (e.g. the + # non-quiet `chat -q "..."` exit-code mapper) never reads a stale + # failure dict from a previous turn. Set explicitly here so the + # invariant doesn't depend on the order of early returns below. + self._last_run_result = None + try: # Run the conversation with interrupt monitoring result = None @@ -12827,10 +12840,26 @@ def run_agent(): print(f"\n⏩ Delivering leftover /steer as next turn: '{preview}'") self._pending_input.put(_leftover_steer) + # Stash the run result so single-query callers can inspect + # failure_reason (rate_limit/billing) for kanban-worker exit code + # mapping. See `_worker_exit_code_from_result` and hermes-agent#5. + # chat() itself returns only the response string for back-compat; + # this attribute is the only out-of-band channel for run metadata. + self._last_run_result = result + return response except Exception as e: print(f"Error: {e}") + # Synthesize a failure result so single-query callers downstream + # can still apply the kanban exit-code mapping (a thrown exception + # is by definition a failure — `failure_reason` is unknown so the + # rate-limit special case won't fire, and we fall through to exit 1). + self._last_run_result = { + "failed": True, + "error": str(e)[:300], + "completed": False, + } return None finally: # Ensure streaming TTS resources are cleaned up even on error. @@ -15697,6 +15726,39 @@ def _block(reason: str) -> None: ) +def _worker_exit_code_from_result(result) -> int: + """Map a run_conversation/chat result dict to a process exit code. + + Contract: + - Success or no result info → 0 + - Failure outside a kanban worker → 1 + - Failure inside a kanban worker (HERMES_KANBAN_TASK env set) WITH + failure_reason ∈ {rate_limit, billing} → KANBAN_RATE_LIMIT_EXIT_CODE (75) + - Any other failure inside a kanban worker → 1 + + Closes hermes-agent#5: the non-quiet `chat -q "..."` path (the one + Marvel team workers actually use) previously didn't apply this mapping + at all — a rate-limited worker exited 0 by virtue of `cli.chat()` + returning cleanly. The dispatcher then classified the 0-exit as a + "protocol violation" and auto-blocked the task. This helper is now + called from BOTH the quiet (-Q / --quiet) and non-quiet (--q "...") + single-query paths so the exit-code contract is consistent. + """ + if not isinstance(result, dict) or not result.get("failed"): + return 0 + if os.environ.get("HERMES_KANBAN_TASK") and result.get( + "failure_reason" + ) in ("rate_limit", "billing"): + try: + from hermes_cli.kanban_db import ( + KANBAN_RATE_LIMIT_EXIT_CODE as _RL_CODE, + ) + return _RL_CODE + except Exception: + return 1 + return 1 + + def main( query: str = None, q: str = None, @@ -16118,31 +16180,9 @@ def _signal_handler_q(signum, frame): print(f"\nsession_id: {cli.session_id}", file=sys.stderr) # Ensure proper exit code for automation wrappers. - # - # Kanban workers get a special case: when the run failed - # purely because the provider rate-limited / exhausted - # quota (not because the task itself is broken), exit with - # the EX_TEMPFAIL sentinel instead of the generic 1. The - # dispatcher's reap classifier maps that code to a - # ``rate_limited`` exit and releases the task back to - # ``ready`` WITHOUT incrementing the failure counter, so a - # 5-hour quota window can't trip the circuit breaker and - # permanently block the card. Non-kanban runs keep the - # plain 0/1 contract automation wrappers expect. - _exit_code = 0 - if isinstance(result, dict) and result.get("failed"): - _exit_code = 1 - if os.environ.get("HERMES_KANBAN_TASK") and result.get( - "failure_reason" - ) in ("rate_limit", "billing"): - try: - from hermes_cli.kanban_db import ( - KANBAN_RATE_LIMIT_EXIT_CODE as _RL_CODE, - ) - _exit_code = _RL_CODE - except Exception: - _exit_code = 1 - sys.exit(_exit_code) + # See `_worker_exit_code_from_result` for the kanban + # EX_TEMPFAIL special case (rate-limit → exit 75). + sys.exit(_worker_exit_code_from_result(result)) # Exit with error code if credentials or agent init fails sys.exit(1) @@ -16168,6 +16208,16 @@ def _signal_handler_q(signum, frame): cli._show_security_advisories() cli.chat(query, images=single_query_images or None) cli._print_exit_summary() + # Apply the same kanban-worker exit-code mapping that the quiet + # path uses, so a rate-limited run gets EX_TEMPFAIL (75) instead + # of the implicit 0 that defaults from a clean function return. + # Without this, Codex 429 in a worker exits 0 → dispatcher + # classifies as "protocol violation" → auto-blocks the task. + # (hermes-agent#5 root cause.) + _last_result = getattr(cli, "_last_run_result", None) + _exit_code = _worker_exit_code_from_result(_last_result) + if _exit_code != 0: + sys.exit(_exit_code) return # Run interactive mode diff --git a/tests/cli/test_worker_exit_code_from_result.py b/tests/cli/test_worker_exit_code_from_result.py new file mode 100644 index 0000000000000..c600955f10b45 --- /dev/null +++ b/tests/cli/test_worker_exit_code_from_result.py @@ -0,0 +1,104 @@ +"""Tests for cli._worker_exit_code_from_result. + +Closes hermes-agent#5. The non-quiet `chat -q "..."` path (the one Marvel +team workers use) previously didn't apply the kanban exit-code mapping at +all — a rate-limited worker exited 0 by virtue of `cli.chat()` returning +cleanly. The dispatcher then classified the 0-exit as a "protocol violation" +and auto-blocked the task. + +This helper centralizes the mapping and is now called from BOTH the quiet +and non-quiet single-query paths so the contract is consistent. +""" +from __future__ import annotations + +import importlib +import os +import sys +from typing import Any + +import pytest + + +@pytest.fixture +def helper(monkeypatch: pytest.MonkeyPatch): + """Import cli._worker_exit_code_from_result without triggering CLI side effects.""" + # cli.py is heavyweight; import once via the package. + import cli + importlib.reload(cli) + return cli._worker_exit_code_from_result + + +def _result(failed: bool = True, failure_reason: str | None = None) -> dict[str, Any]: + out: dict[str, Any] = {"failed": failed} + if failure_reason is not None: + out["failure_reason"] = failure_reason + return out + + +def test_none_result_returns_zero(helper, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("HERMES_KANBAN_TASK", raising=False) + assert helper(None) == 0 + + +def test_non_dict_result_returns_zero(helper, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("HERMES_KANBAN_TASK", raising=False) + assert helper("just a string") == 0 + + +def test_success_result_returns_zero(helper, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("HERMES_KANBAN_TASK", raising=False) + assert helper({"completed": True, "failed": False}) == 0 + + +def test_failure_outside_kanban_returns_one(helper, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("HERMES_KANBAN_TASK", raising=False) + assert helper(_result(failure_reason="rate_limit")) == 1 + assert helper(_result(failure_reason="billing")) == 1 + assert helper(_result(failure_reason="other")) == 1 + assert helper(_result()) == 1 + + +def test_rate_limit_inside_kanban_returns_75(helper, monkeypatch: pytest.MonkeyPatch) -> None: + """The bug v6.6 hit: worker rate-limited but exited 0 → dispatcher + auto-blocked as protocol violation. Fix: exit 75 (EX_TEMPFAIL). + """ + monkeypatch.setenv("HERMES_KANBAN_TASK", "t_abc123") + code = helper(_result(failure_reason="rate_limit")) + # Pull the canonical constant to ensure we match the dispatcher's classifier + from hermes_cli.kanban_db import KANBAN_RATE_LIMIT_EXIT_CODE as RL_CODE + assert code == RL_CODE == 75 + + +def test_billing_inside_kanban_returns_75(helper, monkeypatch: pytest.MonkeyPatch) -> None: + """Billing/quota wall is the same recovery story as rate-limit.""" + monkeypatch.setenv("HERMES_KANBAN_TASK", "t_abc123") + from hermes_cli.kanban_db import KANBAN_RATE_LIMIT_EXIT_CODE as RL_CODE + assert helper(_result(failure_reason="billing")) == RL_CODE + + +def test_other_failure_inside_kanban_returns_one(helper, monkeypatch: pytest.MonkeyPatch) -> None: + """Non-rate-limit failures inside a kanban worker still exit 1 — the + dispatcher treats those as real task failures (worth retry-counting). + """ + monkeypatch.setenv("HERMES_KANBAN_TASK", "t_abc123") + assert helper(_result(failure_reason="other")) == 1 + assert helper(_result()) == 1 # no failure_reason field + + +def test_missing_failure_reason_field_inside_kanban_returns_one( + helper, monkeypatch: pytest.MonkeyPatch +) -> None: + """Defensive: a result dict that's `failed=True` but doesn't carry a + failure_reason (e.g. a thrown exception synthesized into a dict) should + NOT be treated as rate-limited. + """ + monkeypatch.setenv("HERMES_KANBAN_TASK", "t_abc123") + assert helper({"failed": True}) == 1 + + +def test_rate_limit_no_kanban_env_returns_one(helper, monkeypatch: pytest.MonkeyPatch) -> None: + """The kanban exit-code mapping only fires for actual workers. + A human-driven CLI run that hits 429 still gets the generic exit 1. + """ + monkeypatch.delenv("HERMES_KANBAN_TASK", raising=False) + assert helper(_result(failure_reason="rate_limit")) == 1 From 307e5677ea3d9658d5a310ce35fe05255686dde8 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Tue, 9 Jun 2026 14:44:51 -0700 Subject: [PATCH 5/5] =?UTF-8?q?feat(kanban):=20v6.7=20Tranche=201=20?= =?UTF-8?q?=E2=80=94=20kanban=5Fcomplete=20verification=20gates?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds three pre-write-txn gates that fire before complete_task transitions a task to done. Mirrors the existing _verify_created_cards / HallucinatedCardsError pattern: any violation is recorded as an audit event and raised, so worker state is unchanged and the worker can retry after fixing the underlying issue. Closes hermes-jarvis#28 (repo hygiene gate) Closes hermes-jarvis#62 (workspace-diff verification) Closes hermes-jarvis#64 (per-role runtime floor) Context: hermes-jarvis#61 (bootstrap-paradox case study) ## The three gates 1. verify_runtime_floor — per-role floor on completed_at - started_at. build roles 5min, review roles 90s, orchestration roles 0. Catches Tony's 20-second "approve" verdicts and Friday's 59-second "implemented 7 dispatcher gates" claims. 2. verify_workspace_diff — when a non-review worker on a dir/worktree workspace claims to have produced code, git diff against the tracking base must show actual changes. Catches Friday's "Wave A gates implemented" with an empty diff on the branch. 3. verify_no_stray_artifacts — rejects untracked or tracked artifacts matching patterns the swarm has historically committed by accident: *evidence*, commit-hash*, triage/*, tmp-*, and tracked files with no extension and no shebang (the "all prior block evidence files" failure mode from agent-dashboard PR #1). ## Opt-outs Workers may bypass individual gates via per-call metadata keys: x_fast_justified → allow_below_floor x_no_code → allow_no_code x_stray_ok → allow_stray Opt-outs are recorded as part of the completed event for audit. ## Tests 28 new tests cover the exact 2026-06-09 failure modes (Tony 20s, Friday 59s + empty diff, PR-1 "all prior block evidence files") plus clean-path passes and opt-outs. 258 passed / 0 failed in the wider kanban+complete+task test suite — zero regressions. Co-Authored-By: Claude Opus 4.7 (1M context) --- hermes_cli/kanban_completion_gates.py | 381 ++++++++++++++++++++++ hermes_cli/kanban_db.py | 89 +++++ tests/cli/test_kanban_completion_gates.py | 335 +++++++++++++++++++ tools/kanban_tools.py | 10 + 4 files changed, 815 insertions(+) create mode 100644 hermes_cli/kanban_completion_gates.py create mode 100644 tests/cli/test_kanban_completion_gates.py diff --git a/hermes_cli/kanban_completion_gates.py b/hermes_cli/kanban_completion_gates.py new file mode 100644 index 0000000000000..00ce28f6c22b5 --- /dev/null +++ b/hermes_cli/kanban_completion_gates.py @@ -0,0 +1,381 @@ +"""Verification gates run before `complete_task` writes `status=done`. + +Each gate is a pure function that takes structured inputs and returns either +``None`` (pass) or a violation dataclass describing why the completion should +be rejected. The caller (``complete_task`` in ``kanban_db.py``) collects any +violation, emits an auditable event, and raises so the worker layer surfaces a +structured retry message. + +Pattern mirrors the existing ``_verify_created_cards`` / +``HallucinatedCardsError`` flow — gates fire BEFORE the write transaction so +state is unchanged on rejection and the worker can simply retry with corrected +output. + +Three gates ship today (Tranche 1 of v6.7, closes #28, #62, #64): + +1. :func:`verify_runtime_floor` — per-role floor on + ``completed_at - started_at``. Catches Tony's 20-second "approve" verdicts + and Friday's 59-second "implemented 7 dispatcher gates" claims. + +2. :func:`verify_workspace_diff` — when a non-review worker on a + ``dir`` / ``worktree`` workspace claims to have produced code, the workspace + must show a real diff against its tracking base. Catches Friday's "Wave A + gates implemented" with zero changes on the branch. + +3. :func:`verify_no_stray_artifacts` — reject untracked artifacts matching + patterns the swarm has historically committed by accident + (``*evidence*``, ``commit-hash*``, ``triage/*``, ``tmp-*``, and tracked + files with no extension and no shebang — the "all prior block evidence + files" failure mode). + +See hermes-jarvis#61 for the bootstrap-paradox case study that motivates +these gates. +""" +from __future__ import annotations + +import os +import re +import shlex +import subprocess +from dataclasses import dataclass +from pathlib import Path +from typing import Optional + + +# ===================================================================== +# Per-role runtime floors (#64) +# ===================================================================== + +# Empirically derived from the 2026-06-09 build-chain failure: any number +# below the floor on a non-orchestration task is more likely fabrication +# than fast work. Workers may opt out per-call via the +# ``x_fast_justified`` metadata field, surfaced through ``allow_below_floor``. +ROLE_RUNTIME_FLOORS_SECONDS: dict[str, int] = { + # Build / implementation roles — real code changes don't ship in <5 min + "friday": 5 * 60, + "shuri": 5 * 60, + "build-engineer": 5 * 60, + # Review roles — even a tiny review needs to read the diff + "tony": 90, + "tchalla": 90, + "vision": 90, + "reviewer": 90, + # Orchestration roles — JARVIS umbrella spawn can be fast and correct + "jarvis": 0, + "pepper": 0, + "banner": 0, +} + + +@dataclass(frozen=True) +class RuntimeFloorViolation: + role: str + started_at: int + completed_at: int + floor_seconds: int + actual_seconds: int + + def message(self) -> str: + return ( + f"runtime-floor: {self.role} completed in {self.actual_seconds}s, " + f"below the {self.floor_seconds}s floor for this role. " + f"Either keep working (add evidence and re-call kanban_complete after " + f"the floor passes) or, if the work was genuinely trivial, set " + f"metadata={{\"x_fast_justified\": \"\"}} on the " + f"completion call." + ) + + +def verify_runtime_floor( + assignee: Optional[str], + started_at: Optional[int], + completed_at: int, + *, + allow_below_floor: bool = False, +) -> Optional[RuntimeFloorViolation]: + """Return a violation if the worker's runtime is below its role floor. + + ``started_at`` is the timestamp the dispatcher recorded when the worker + claimed the task (NOT the run-row creation time). ``completed_at`` is + "now" from the dispatcher's perspective when ``complete_task`` runs. + + A floor of 0 (or an unknown assignee, or a missing ``started_at``) is a + pass — we never invent floors for roles we don't know. + """ + if allow_below_floor: + return None + if not assignee or started_at is None: + return None + floor = ROLE_RUNTIME_FLOORS_SECONDS.get(assignee.lower()) + if not floor: + return None + actual = max(0, completed_at - int(started_at)) + if actual >= floor: + return None + return RuntimeFloorViolation( + role=assignee, started_at=int(started_at), completed_at=completed_at, + floor_seconds=floor, actual_seconds=actual, + ) + + +# ===================================================================== +# Workspace-diff gate (#62) +# ===================================================================== + +REVIEW_ROLES = {"tony", "tchalla", "vision", "reviewer"} +ORCHESTRATION_ROLES = {"jarvis", "pepper", "banner"} + +# Phrases workers used in fabricated completion summaries that should be +# backed by a real diff. Conservative — only triggers the gate when the +# worker has explicitly claimed code changes. +_IMPLEMENTATION_CLAIM_PATTERNS = [ + re.compile(r"\b(implement(?:ed|s)?|build(?:s|t)?|add(?:ed|s)?|" + r"creat(?:ed|es)?|wrote|wr(?:ites|ote)|ship(?:ped|s)?|" + r"land(?:ed|s)?|introduc(?:ed|es)?|refactor(?:ed|s)?|" + r"fix(?:ed|es)?|patch(?:ed|es)?)\b", re.IGNORECASE), +] + + +@dataclass(frozen=True) +class WorkspaceDiffViolation: + assignee: str + workspace_path: str + summary_excerpt: str + diff_stat: str # may be empty string if no changes + + def message(self) -> str: + diff_preview = self.diff_stat.strip() or "(no changes against tracking base)" + return ( + f"workspace-diff: {self.assignee} summary claims implementation " + f"work ({self.summary_excerpt!r}) but `git diff` in " + f"{self.workspace_path} shows: {diff_preview}. " + f"Either produce the changes the summary describes, or block " + f"with an honest reason. To skip this check on a doc-only or " + f"genuinely-no-code task, set metadata={{\"x_no_code\": true}}." + ) + + +def _summary_claims_implementation(summary: str) -> bool: + return any(p.search(summary or "") for p in _IMPLEMENTATION_CLAIM_PATTERNS) + + +def _git_diff_stat_against_base(workspace_path: str) -> str: + """Return `git diff --stat` against the workspace's tracking base. + + Tracking base is, in order: ``@{upstream}`` if it exists, else + ``origin/main`` if it exists, else ``main``. If git rejects all three, + returns the empty string (gate treats as "no diff"). + + Subprocess calls use a hard 10s wallclock so a hung git can't stall the + dispatcher. + """ + if not workspace_path or not os.path.isdir(workspace_path): + return "" + if not os.path.isdir(os.path.join(workspace_path, ".git")): + # Worktree-backed dirs have .git as a file pointer; that's fine. + if not os.path.isfile(os.path.join(workspace_path, ".git")): + return "" + + def _run(args: list[str]) -> Optional[str]: + try: + out = subprocess.run( + args, cwd=workspace_path, capture_output=True, + text=True, timeout=10, check=False, + ) + except (subprocess.TimeoutExpired, FileNotFoundError, OSError): + return None + if out.returncode != 0: + return None + return out.stdout + + for base_spec in ("@{upstream}", "origin/main", "main"): + # First check the base exists (cheap), then diff against it. + if _run(["git", "rev-parse", "--verify", base_spec]) is None: + continue + stat = _run(["git", "diff", "--stat", base_spec, "HEAD"]) + if stat is not None: + return stat + return "" + + +def verify_workspace_diff( + assignee: Optional[str], + workspace_kind: Optional[str], + workspace_path: Optional[str], + summary: Optional[str], + *, + allow_no_code: bool = False, +) -> Optional[WorkspaceDiffViolation]: + """Reject completions that claim code work but show no diff. + + Skipped (returns None) when: + - assignee is a review or orchestration role + - workspace is scratch (no diff target) + - workspace_path is missing or not a directory + - summary doesn't claim implementation + - caller opted out via ``allow_no_code=True`` + """ + if allow_no_code: + return None + if not assignee: + return None + role = assignee.lower() + if role in REVIEW_ROLES or role in ORCHESTRATION_ROLES: + return None + if (workspace_kind or "scratch") not in {"dir", "worktree"}: + return None + if not workspace_path or not os.path.isdir(workspace_path): + # Wrong / typo'd path is the dispatcher's problem to surface + # elsewhere — we don't punish the worker for it. + return None + if not _summary_claims_implementation(summary or ""): + return None + diff_stat = _git_diff_stat_against_base(workspace_path) + # A real implementation produces SOME change line. We only reject when + # the diff is empty / whitespace. + if diff_stat and diff_stat.strip(): + return None + summary_excerpt = (summary or "").strip().splitlines()[0][:200] + return WorkspaceDiffViolation( + assignee=assignee, workspace_path=workspace_path, + summary_excerpt=summary_excerpt, diff_stat=diff_stat, + ) + + +# ===================================================================== +# Repo-hygiene gate (#28) +# ===================================================================== + +# Patterns that mark a path as "stray orchestration artifact" rather than +# real source. Matched against the path relative to the repo root, case +# insensitive. Aligned with the agent-dashboard PR #1 audit findings ( +# `all prior block evidence files`, `commit-hash.txt`, `triage/v6.4-*`). +_STRAY_PATH_PATTERNS = [ + re.compile(r"(^|/)(evidence|.*-evidence|.*_evidence)(/|\b)", re.IGNORECASE), + re.compile(r"(^|/)commit-hash(\.[a-z]+)?$", re.IGNORECASE), + re.compile(r"(^|/)triage/", re.IGNORECASE), + re.compile(r"(^|/)tmp-[^/]+$", re.IGNORECASE), + re.compile(r"(^|/)all prior block evidence files$", re.IGNORECASE), +] + + +@dataclass(frozen=True) +class StrayArtifactViolation: + workspace_path: str + stray_paths: tuple[str, ...] + + def message(self) -> str: + listing = "\n ".join(self.stray_paths) + return ( + f"repo-hygiene: workspace {self.workspace_path} contains files " + f"that look like leftover orchestration artifacts:\n {listing}\n" + f"Delete (or .gitignore) them before calling kanban_complete. " + f"If a stray-looking path is intentional, prefix it with a real " + f"file extension and add a one-line comment explaining why it's " + f"in the repo." + ) + + +def _has_shebang(path: str) -> bool: + try: + with open(path, "rb") as f: + head = f.read(2) + return head == b"#!" + except (OSError, IOError): + return False + + +def _stray_path_score(repo_root: str, rel_path: str) -> bool: + """True if ``rel_path`` looks like a stray artifact.""" + norm = rel_path.replace("\\", "/") + if any(p.search(norm) for p in _STRAY_PATH_PATTERNS): + return True + # Tracked file with no extension and no shebang — the "all prior block + # evidence files" failure mode. + base = os.path.basename(norm) + if "." not in base and not _has_shebang(os.path.join(repo_root, rel_path)): + return True + return False + + +def _list_workspace_files(workspace_path: str) -> list[str]: + """Return the union of `git ls-files` (tracked) and `git ls-files + --others --exclude-standard` (untracked & not gitignored), as relative + paths. Empty list on any git error. + """ + if not workspace_path or not os.path.isdir(workspace_path): + return [] + + def _run(args: list[str]) -> Optional[str]: + try: + out = subprocess.run( + args, cwd=workspace_path, capture_output=True, + text=True, timeout=10, check=False, + ) + except (subprocess.TimeoutExpired, FileNotFoundError, OSError): + return None + if out.returncode != 0: + return None + return out.stdout + + tracked = _run(["git", "ls-files"]) or "" + untracked = _run(["git", "ls-files", "--others", "--exclude-standard"]) or "" + paths = set() + for blob in (tracked, untracked): + for line in blob.splitlines(): + line = line.strip() + if line: + paths.add(line) + return sorted(paths) + + +def verify_no_stray_artifacts( + workspace_kind: Optional[str], + workspace_path: Optional[str], + *, + allow_stray: bool = False, +) -> Optional[StrayArtifactViolation]: + """Reject completions where the workspace tree contains stray files. + + Skipped (returns None) when: + - workspace is scratch + - workspace_path is missing or not a directory + - caller opted out via ``allow_stray=True`` + """ + if allow_stray: + return None + if (workspace_kind or "scratch") not in {"dir", "worktree"}: + return None + if not workspace_path or not os.path.isdir(workspace_path): + return None + stray = [p for p in _list_workspace_files(workspace_path) + if _stray_path_score(workspace_path, p)] + if not stray: + return None + return StrayArtifactViolation( + workspace_path=workspace_path, stray_paths=tuple(stray), + ) + + +# ===================================================================== +# Exception class for the integration in `complete_task` +# ===================================================================== + +class CompletionGateError(ValueError): + """Raised by ``complete_task`` when one or more v6.7 gates reject. + + ``violations`` is a list of dataclasses (one per failed gate). Each has + a ``.message()`` returning a worker-actionable string. Subclass of + ``ValueError`` so existing tool-error handlers treat this as a + recoverable user error (same convention as + :class:`HallucinatedCardsError`). + """ + + def __init__(self, violations: list, completing_task_id: str): + self.violations = list(violations) + self.completing_task_id = completing_task_id + lines = [v.message() for v in self.violations] + super().__init__( + "kanban_complete blocked by v6.7 gates:\n- " + + "\n- ".join(lines) + ) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index c8c53dba7ecb6..b7eef59eeda30 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -90,6 +90,13 @@ from toolsets import get_toolset_names +from hermes_cli.kanban_completion_gates import ( + CompletionGateError, + verify_no_stray_artifacts, + verify_runtime_floor, + verify_workspace_diff, +) + _log = logging.getLogger(__name__) @@ -3559,6 +3566,64 @@ def __init__(self, phantom: list[str], completing_task_id: str): ) +def _v6_7_run_completion_gates( + conn: sqlite3.Connection, + task_id: str, + *, + summary: Optional[str], + metadata: Optional[dict], + now: int, +) -> list: + """Run the v6.7 Tranche 1 completion gates and return any violations. + + Reads task assignee / workspace / started_at from the tasks row and + delegates to the pure gate functions in ``kanban_completion_gates``. + Returns an empty list when all gates pass. + + Workers may opt out of individual gates via per-call metadata keys + (``x_fast_justified``, ``x_no_code``, ``x_stray_ok``) which surface as + the gate functions' ``allow_*`` kwargs. Opt-outs are recorded as part + of the completed event for audit. + """ + row = conn.execute( + "SELECT assignee, workspace_kind, workspace_path, started_at " + " FROM tasks WHERE id = ?", + (task_id,), + ).fetchone() + if row is None: + return [] + md = metadata or {} + fast_ok = bool(md.get("x_fast_justified")) + no_code = bool(md.get("x_no_code")) + stray_ok = bool(md.get("x_stray_ok")) + violations: list = [] + floor = verify_runtime_floor( + assignee=row["assignee"], + started_at=row["started_at"], + completed_at=now, + allow_below_floor=fast_ok, + ) + if floor is not None: + violations.append(floor) + diff = verify_workspace_diff( + assignee=row["assignee"], + workspace_kind=row["workspace_kind"], + workspace_path=row["workspace_path"], + summary=summary, + allow_no_code=no_code, + ) + if diff is not None: + violations.append(diff) + stray = verify_no_stray_artifacts( + workspace_kind=row["workspace_kind"], + workspace_path=row["workspace_path"], + allow_stray=stray_ok, + ) + if stray is not None: + violations.append(stray) + return violations + + def complete_task( conn: sqlite3.Connection, task_id: str, @@ -3626,6 +3691,30 @@ def complete_task( else: verified_cards = [] + # v6.7 Tranche 1: kanban_complete verification gates. + # See hermes-jarvis#61, #62, #28, #64. Same pre-write-txn pattern as + # _verify_created_cards: any violation raises before state changes, so + # the worker can retry after fixing the underlying issue. + _violations = _v6_7_run_completion_gates( + conn, task_id, summary=summary, metadata=metadata, now=now, + ) + if _violations: + with write_txn(conn): + _append_event( + conn, task_id, "completion_blocked_v6_7_gates", + { + "violations": [ + {"kind": type(v).__name__, "message": v.message()} + for v in _violations + ], + "summary_preview": ( + (summary or result or "").strip().splitlines()[0][:200] + if (summary or result) else None + ), + }, + ) + raise CompletionGateError(_violations, task_id) + with write_txn(conn): if expected_run_id is None: cur = conn.execute( diff --git a/tests/cli/test_kanban_completion_gates.py b/tests/cli/test_kanban_completion_gates.py new file mode 100644 index 0000000000000..448c495155256 --- /dev/null +++ b/tests/cli/test_kanban_completion_gates.py @@ -0,0 +1,335 @@ +"""Tests for hermes_cli.kanban_completion_gates — v6.7 Tranche 1. + +Closes hermes-jarvis#62 (workspace-diff verification), #28 (repo hygiene +gate), and #64 (per-role runtime floor). See hermes-jarvis#61 for the +bootstrap-paradox case study where a v6.7 swarm build chain rubber-stamped +9 tasks done in ~10 minutes with zero real deliverables. + +Each gate is a pure function and gets a focused test that pins the exact +failure modes the 2026-06-09 chain demonstrated. +""" +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +from hermes_cli.kanban_completion_gates import ( + RuntimeFloorViolation, + StrayArtifactViolation, + WorkspaceDiffViolation, + verify_no_stray_artifacts, + verify_runtime_floor, + verify_workspace_diff, +) + + +# ===================================================================== +# verify_runtime_floor — #64 +# ===================================================================== + + +class TestRuntimeFloor: + def test_tony_20s_review_is_below_floor(self) -> None: + """The exact case from 2026-06-09: Tony approved Wave A in 20s.""" + v = verify_runtime_floor("tony", started_at=1000, completed_at=1020) + assert isinstance(v, RuntimeFloorViolation) + assert v.actual_seconds == 20 + assert v.floor_seconds == 90 + assert "tony" in v.message().lower() + assert "below" in v.message().lower() + + def test_friday_59s_implementation_is_below_floor(self) -> None: + """Friday claimed 7 dispatcher gates implemented in 59s.""" + v = verify_runtime_floor("friday", started_at=1000, completed_at=1059) + assert isinstance(v, RuntimeFloorViolation) + assert v.floor_seconds == 300 + + def test_tony_91s_review_passes(self) -> None: + """One second above the floor is a pass — the floor is the floor.""" + assert verify_runtime_floor("tony", 1000, 1091) is None + + def test_jarvis_orchestration_has_no_floor(self) -> None: + """Orchestration roles routinely complete in seconds and that's fine.""" + assert verify_runtime_floor("jarvis", 1000, 1001) is None + + def test_unknown_assignee_skips(self) -> None: + """Don't invent floors for roles we haven't categorized.""" + assert verify_runtime_floor("rando-profile", 1000, 1001) is None + + def test_missing_assignee_skips(self) -> None: + assert verify_runtime_floor(None, 1000, 1001) is None + + def test_missing_started_at_skips(self) -> None: + """If the dispatcher never recorded started_at the gate can't fire.""" + assert verify_runtime_floor("tony", None, 1100) is None + + def test_allow_below_floor_opt_out(self) -> None: + """Workers can justify fast completions via metadata.""" + assert ( + verify_runtime_floor("tony", 1000, 1020, allow_below_floor=True) + is None + ) + + def test_completed_before_started_is_zero(self) -> None: + """Clock skew / wrong order doesn't crash — actual=0, still below floor.""" + v = verify_runtime_floor("tony", 1100, 1000) + assert v is not None + assert v.actual_seconds == 0 + + def test_case_insensitive_role_match(self) -> None: + """Profile names sometimes capitalize differently — match insensitively.""" + v = verify_runtime_floor("Tony", 1000, 1020) + assert v is not None + + +# ===================================================================== +# verify_workspace_diff — #62 +# ===================================================================== + + +@pytest.fixture +def git_workspace(tmp_path: Path) -> Path: + """A real git repo with one committed file on main, no other changes.""" + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=tmp_path, check=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], + cwd=tmp_path, check=True, + ) + subprocess.run( + ["git", "config", "user.name", "test"], + cwd=tmp_path, check=True, + ) + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "src.py"], cwd=tmp_path, check=True) + subprocess.run( + ["git", "commit", "-q", "-m", "init"], cwd=tmp_path, check=True, + ) + return tmp_path + + +class TestWorkspaceDiff: + def test_friday_empty_diff_with_implementation_claim_rejects( + self, git_workspace: Path, + ) -> None: + """The exact case from 2026-06-09: Friday's branch had no new commits + but his summary claimed "Wave A dispatcher discipline gates + implemented; tests cover #28-#34". + """ + v = verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Wave A dispatcher discipline gates implemented; tests cover #28-#34", + ) + assert isinstance(v, WorkspaceDiffViolation) + assert "friday" in v.message().lower() + assert "implementation" in v.message().lower() or "implement" in v.message().lower() + + def test_real_diff_with_implementation_claim_passes( + self, git_workspace: Path, + ) -> None: + """A worker who actually did work and committed it gets through.""" + # Make a second commit so HEAD differs from main's first commit + # but we still test against HEAD's diff against base. Setup: detach, + # add a new commit, then diff stat will be non-empty against `main` + # if HEAD has more. + new_file = git_workspace / "feature.py" + new_file.write_text("def real(): pass\n") + subprocess.run(["git", "checkout", "-q", "-b", "feature"], cwd=git_workspace, check=True) + subprocess.run(["git", "add", "feature.py"], cwd=git_workspace, check=True) + subprocess.run( + ["git", "commit", "-q", "-m", "feat"], cwd=git_workspace, check=True, + ) + v = verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Implemented feature module per spec", + ) + assert v is None + + def test_review_role_skipped(self, git_workspace: Path) -> None: + """Tony's deliverable is a verdict, not code — skip the diff gate.""" + assert ( + verify_workspace_diff( + assignee="tony", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="approve - implementation matches spec", + ) + is None + ) + + def test_orchestration_role_skipped(self, git_workspace: Path) -> None: + """JARVIS umbrella spawn doesn't ship code, even when body says + 'implemented chain'.""" + assert ( + verify_workspace_diff( + assignee="jarvis", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Spawned and implemented the build chain", + ) + is None + ) + + def test_scratch_workspace_skipped(self) -> None: + """scratch workspaces have no diff target.""" + assert ( + verify_workspace_diff( + assignee="friday", + workspace_kind="scratch", + workspace_path=None, + summary="implemented thing", + ) + is None + ) + + def test_no_implementation_claim_skipped(self, git_workspace: Path) -> None: + """Summary that doesn't claim code work doesn't trip the gate.""" + assert ( + verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Investigated the issue; recommendations in comment.", + ) + is None + ) + + def test_x_no_code_opt_out(self, git_workspace: Path) -> None: + assert ( + verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path=str(git_workspace), + summary="Implemented the docs reshuffle", + allow_no_code=True, + ) + is None + ) + + def test_nonexistent_workspace_path_skipped(self) -> None: + """We don't crash when workspace_path is wrong; we just skip.""" + assert ( + verify_workspace_diff( + assignee="friday", + workspace_kind="dir", + workspace_path="/tmp/this-does-not-exist-v67", + summary="implemented thing", + ) + is None + ) + + +# ===================================================================== +# verify_no_stray_artifacts — #28 +# ===================================================================== + + +def _git_init(tmp_path: Path) -> None: + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], cwd=tmp_path, check=True, + ) + subprocess.run( + ["git", "config", "user.name", "test"], cwd=tmp_path, check=True, + ) + + +class TestStrayArtifacts: + def test_pr1_all_prior_block_evidence_files(self, tmp_path: Path) -> None: + """The literal failure mode from agent-dashboard PR #1: a file + named 'all prior block evidence files' (no extension) committed + because the evidence-path gate took a descriptive phrase + literally. + """ + _git_init(tmp_path) + stray = tmp_path / "all prior block evidence files" + stray.write_text("nothing\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run( + ["git", "add", "all prior block evidence files", "src.py"], + cwd=tmp_path, check=True, + ) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert isinstance(v, StrayArtifactViolation) + assert "all prior block evidence files" in v.stray_paths + + def test_commit_hash_txt_stray(self, tmp_path: Path) -> None: + _git_init(tmp_path) + (tmp_path / "commit-hash.txt").write_text("abc123\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert "commit-hash.txt" in v.stray_paths + + def test_triage_dir_stray(self, tmp_path: Path) -> None: + _git_init(tmp_path) + td = tmp_path / "triage" + td.mkdir() + (td / "v6.4-report.md").write_text("notes\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert any("triage/" in p for p in v.stray_paths) + + def test_evidence_subdir_stray(self, tmp_path: Path) -> None: + _git_init(tmp_path) + ed = tmp_path / "changes" / "fix-14" / "evidence" + ed.mkdir(parents=True) + (ed / "out.json").write_text("{}\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert any("evidence" in p for p in v.stray_paths) + + def test_clean_repo_passes(self, tmp_path: Path) -> None: + _git_init(tmp_path) + (tmp_path / "src.py").write_text("print('hi')\n") + (tmp_path / "README.md").write_text("# hi\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + assert verify_no_stray_artifacts("dir", str(tmp_path)) is None + + def test_shebang_file_without_extension_is_ok(self, tmp_path: Path) -> None: + """Real scripts have shebangs — those aren't stray.""" + _git_init(tmp_path) + (tmp_path / "bin").mkdir() + script = tmp_path / "bin" / "deploy" + script.write_text("#!/bin/bash\necho hi\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + assert verify_no_stray_artifacts("dir", str(tmp_path)) is None + + def test_scratch_workspace_skipped(self) -> None: + assert verify_no_stray_artifacts("scratch", None) is None + + def test_x_stray_ok_opt_out(self, tmp_path: Path) -> None: + _git_init(tmp_path) + (tmp_path / "all prior block evidence files").write_text("x\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + assert ( + verify_no_stray_artifacts("dir", str(tmp_path), allow_stray=True) + is None + ) + + def test_nonexistent_workspace_path_skipped(self) -> None: + assert ( + verify_no_stray_artifacts("dir", "/tmp/does-not-exist-v67") is None + ) + + def test_tmp_prefixed_files_stray(self, tmp_path: Path) -> None: + _git_init(tmp_path) + (tmp_path / "tmp-scratch").write_text("x\n") + (tmp_path / "src.py").write_text("print('hi')\n") + subprocess.run(["git", "add", "."], cwd=tmp_path, check=True) + v = verify_no_stray_artifacts("dir", str(tmp_path)) + assert v is not None + assert "tmp-scratch" in v.stray_paths diff --git a/tools/kanban_tools.py b/tools/kanban_tools.py index 20a522f90a4ba..562dd1e9aeb33 100644 --- a/tools/kanban_tools.py +++ b/tools/kanban_tools.py @@ -580,6 +580,16 @@ def _handle_complete(args: dict, **kw) -> str: f"and either drop these ids from created_cards, or pass " f"created_cards=[] to skip the card-claim check entirely." ) + except kb.CompletionGateError as gate_err: + # v6.7 verification gates rejected. Task state unchanged — + # the worker retries kanban_complete after fixing the + # underlying issue, OR calls kanban_block honestly. See + # hermes-jarvis#61 for why these gates exist. + return tool_error( + f"{gate_err}\n" + f"Your task is still in-flight (no state change). " + f"Address each violation above and retry kanban_complete." + ) if not ok: return tool_error( f"could not complete {tid} (unknown id or already terminal)"