d1e8cda5ed
Fix reviewer BLOCK: the stateless poll loop re-planned every terminal card each tick, so SHIP and the two cycle-limit escalations - whose source cards stay in the done state - re-fired their comment/block on every pass (~2880/day/chain). - Add a persistent emission Ledger (/opt/data/supervisor/emitted.json): SHIP and escalate perform their action only when the (action, target) key is absent, then record it, so each fires at most once and survives a pod restart. Spawns remain self-limiting via the existing dedup scan. - Cycle-limit escalations now target the SOURCE review/repair card (not the impl root), so the card also leaves the done state and is skipped next tick even if block_task round-trips imperfectly. - Harden the _spawn TypeError fallback: re-run the dedup scan before retrying so a post-insert TypeError can never double-insert. - Add across-ticks idempotency tests (10x supervise_once -> exactly one comment/flag/block) plus ledger persistence/corruption and spawn-guard tests. Both modules stay 100% line+branch, <500 LOC. Stacks on the merge train (base 5f27e50c). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
499 lines
18 KiB
Python
499 lines
18 KiB
Python
"""Behavioral tests for the supervisor I/O shell: config, board iteration,
|
|
decision application, across-ticks idempotency, the poll loop, the
|
|
auto_supervise gate, deployment wiring, and the NULL->20 goal-turn fix.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from contextlib import nullcontext
|
|
from types import SimpleNamespace
|
|
|
|
import pytest
|
|
import yaml
|
|
|
|
from testing.tests.test_hermes_cli_support import HERMES, _agent_deployment, _load
|
|
|
|
supervisor = _load("kanban_supervisor")
|
|
policy = supervisor.policy
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _isolate_ledger(tmp_path, monkeypatch):
|
|
"""Keep each test's emission ledger on an isolated, writable path."""
|
|
monkeypatch.setattr(supervisor, "LEDGER_PATH", tmp_path / "emitted.json")
|
|
|
|
|
|
class RecordingDb:
|
|
"""Minimal in-memory kanban_db stub capturing every mutating call."""
|
|
|
|
def __init__(self, tasks, boards=None, raise_on=None):
|
|
self._tasks = tasks
|
|
self._boards = boards if boards is not None else [{"slug": "cassandra"}]
|
|
self.created = []
|
|
self.comments = []
|
|
self.blocked = []
|
|
self.metadata_sets = []
|
|
self._raise_on = raise_on or set()
|
|
|
|
def list_boards(self, include_archived=False):
|
|
if "list_boards" in self._raise_on:
|
|
raise RuntimeError("registry down")
|
|
return list(self._boards)
|
|
|
|
def scoped_current_board(self, _board):
|
|
return nullcontext()
|
|
|
|
def connect(self, *, board):
|
|
if "connect" in self._raise_on:
|
|
raise RuntimeError(f"cannot open {board}")
|
|
return SimpleNamespace(close=lambda: None)
|
|
|
|
def list_tasks(self, _conn):
|
|
return list(self._tasks)
|
|
|
|
def create_task(self, _conn, **kwargs):
|
|
if "create_task" in self._raise_on:
|
|
raise RuntimeError("write failed")
|
|
self.created.append(kwargs)
|
|
return "new-task"
|
|
|
|
def add_comment(self, _conn, task_id, author, body):
|
|
self.comments.append((task_id, author, body))
|
|
|
|
def block_task(self, _conn, task_id, reason, kind):
|
|
self.blocked.append((task_id, reason, kind))
|
|
return True
|
|
|
|
def set_task_metadata(self, _conn, task_id, metadata):
|
|
self.metadata_sets.append((task_id, metadata))
|
|
|
|
|
|
def _task(**kw):
|
|
base = {
|
|
"id": "t",
|
|
"status": "done",
|
|
"metadata": {},
|
|
"result": None,
|
|
"parents": [],
|
|
"title": "",
|
|
"body": "",
|
|
}
|
|
base.update(kw)
|
|
return SimpleNamespace(**base)
|
|
|
|
|
|
def _write_config(tmp_path, monkeypatch, kanban):
|
|
path = tmp_path / "config.yaml"
|
|
path.write_text(yaml.safe_dump({"kanban": kanban}), encoding="utf-8")
|
|
monkeypatch.setattr(supervisor, "CONFIG_PATH", path)
|
|
|
|
|
|
# --- configuration --------------------------------------------------------
|
|
|
|
|
|
def test_defaults_are_inert_and_conservative_when_config_missing(tmp_path, monkeypatch):
|
|
monkeypatch.setattr(supervisor, "CONFIG_PATH", tmp_path / "absent.yaml")
|
|
settings = supervisor.load_settings()
|
|
assert settings.enabled is False
|
|
assert settings.interval == supervisor.DEFAULT_INTERVAL_SECONDS
|
|
assert settings.limits.max_cycles == supervisor.DEFAULT_MAX_CYCLES
|
|
assert settings.limits.review_assignee == supervisor.DEFAULT_REVIEW_ASSIGNEE
|
|
|
|
|
|
def test_config_values_are_read_and_sanitized(tmp_path, monkeypatch):
|
|
_write_config(
|
|
tmp_path,
|
|
monkeypatch,
|
|
{
|
|
"auto_supervise": True,
|
|
"supervise_interval_seconds": 45,
|
|
"supervise_max_cycles": 0, # non-positive -> default
|
|
"supervise_max_chains": 8,
|
|
"supervise_review_assignee": " ", # blank -> default
|
|
"supervise_repair_assignee": "cli-codex-high",
|
|
},
|
|
)
|
|
settings = supervisor.load_settings()
|
|
assert settings.enabled is True
|
|
assert settings.interval == 45
|
|
assert settings.limits.max_cycles == supervisor.DEFAULT_MAX_CYCLES
|
|
assert settings.limits.max_chains == 8
|
|
assert settings.limits.review_assignee == supervisor.DEFAULT_REVIEW_ASSIGNEE
|
|
assert settings.limits.repair_assignee == "cli-codex-high"
|
|
|
|
|
|
def test_malformed_config_falls_back_to_defaults(tmp_path, monkeypatch):
|
|
path = tmp_path / "config.yaml"
|
|
path.write_text("kanban: [not, a, mapping]", encoding="utf-8")
|
|
monkeypatch.setattr(supervisor, "CONFIG_PATH", path)
|
|
settings = supervisor.load_settings()
|
|
assert settings.enabled is False and settings.limits.max_chains == supervisor.DEFAULT_MAX_CHAINS
|
|
|
|
|
|
def test_bool_int_text_helpers_reject_wrong_types():
|
|
assert supervisor._bool("yes", False) is False
|
|
assert supervisor._positive_int(True, 5) == 5
|
|
assert supervisor._positive_int(-3, 5) == 5
|
|
assert supervisor._positive_int(2.0, 5) == 2
|
|
assert supervisor._text(7, "d") == "d"
|
|
|
|
|
|
# --- board iteration ------------------------------------------------------
|
|
|
|
|
|
def test_board_slug_handles_dict_object_and_scalar():
|
|
assert supervisor._board_slug({"slug": "s"}) == "s"
|
|
assert supervisor._board_slug(SimpleNamespace(slug=None, id="i")) == "i"
|
|
assert supervisor._board_slug("plain") == "plain"
|
|
|
|
|
|
def test_iter_boards_survives_registry_failure(capsys):
|
|
db = RecordingDb([], raise_on={"list_boards"})
|
|
assert supervisor._iter_boards(db) == []
|
|
assert "could not list boards" in capsys.readouterr().err
|
|
|
|
|
|
def test_iter_boards_drops_empty_slugs():
|
|
db = RecordingDb([], boards=[{"slug": "a"}, {"slug": ""}])
|
|
assert supervisor._iter_boards(db) == ["a"]
|
|
|
|
|
|
# --- decision application -------------------------------------------------
|
|
|
|
|
|
def test_impl_done_spawns_review_and_comments():
|
|
impl = _task(
|
|
id="impl",
|
|
result={"changed_files": ["a.py"], "head_commit": "c1", "pull_request": "pr/1"},
|
|
)
|
|
db = RecordingDb([impl])
|
|
assert supervisor.supervise_once(db, policy.Limits()) == 1
|
|
assert len(db.created) == 1
|
|
assert db.created[0]["idempotency_key"] == "supervisor:review:impl:c1:1"
|
|
assert any("supervisor:" in body for _, _, body in db.comments)
|
|
assert db.blocked == []
|
|
|
|
|
|
def test_ship_marks_ready_for_human_without_merging():
|
|
review = _task(
|
|
id="rev",
|
|
metadata={"supervisor": {"kind": "review", "root": "impl", "parent": "impl", "head_commit": "c1", "cycle": 1}},
|
|
result={"verdict": "SHIP", "summary": "clean"},
|
|
)
|
|
db = RecordingDb([review])
|
|
supervisor.supervise_once(db, policy.Limits())
|
|
assert db.metadata_sets == [("impl", {"supervisor_ready_for_human_merge": True})]
|
|
body = db.comments[-1][2]
|
|
assert "READY FOR HUMAN MERGE" in body and "never" in body
|
|
assert db.created == [] and db.blocked == []
|
|
|
|
|
|
def test_escalation_blocks_card_and_creates_no_followup():
|
|
impl = _task(id="impl", result="unparseable")
|
|
db = RecordingDb([impl])
|
|
supervisor.supervise_once(db, policy.Limits())
|
|
assert db.created == []
|
|
assert db.blocked and db.blocked[0][0] == "impl" and db.blocked[0][2] == "supervisor"
|
|
|
|
|
|
def test_spawn_retries_without_idempotency_key_on_typeerror():
|
|
impl = _task(
|
|
id="impl",
|
|
result={"changed_files": ["a.py"], "head_commit": "c1", "pull_request": "pr/1"},
|
|
)
|
|
|
|
class LegacyDb(RecordingDb):
|
|
def create_task(self, _conn, **kwargs):
|
|
if "idempotency_key" in kwargs:
|
|
raise TypeError("unexpected idempotency_key")
|
|
self.created.append(kwargs)
|
|
return "id"
|
|
|
|
db = LegacyDb([impl])
|
|
supervisor.supervise_once(db, policy.Limits())
|
|
assert len(db.created) == 1 and "idempotency_key" not in db.created[0]
|
|
|
|
|
|
def test_apply_decision_ignores_none_and_rejects_unsafe():
|
|
db = RecordingDb([])
|
|
conn = SimpleNamespace(close=lambda: None)
|
|
ledger = supervisor.Ledger(supervisor.LEDGER_PATH)
|
|
assert supervisor.apply_decision(db, conn, policy.Decision("none"), ledger) is False
|
|
try:
|
|
supervisor.apply_decision(db, conn, policy.Decision("teleport"), ledger)
|
|
raise AssertionError("expected rejection")
|
|
except ValueError:
|
|
pass
|
|
|
|
|
|
def test_ready_flag_tolerates_missing_metadata_setter_and_setter_errors():
|
|
class NoSetterDb(RecordingDb):
|
|
set_task_metadata = None
|
|
|
|
db = NoSetterDb([])
|
|
conn = SimpleNamespace(close=lambda: None)
|
|
supervisor._mark_ready_for_human(db, conn, "impl", "ready")
|
|
assert db.comments[-1][0] == "impl"
|
|
|
|
class BadSetterDb(RecordingDb):
|
|
def set_task_metadata(self, _conn, _task_id, _metadata):
|
|
raise RuntimeError("nope")
|
|
|
|
db2 = BadSetterDb([])
|
|
supervisor._mark_ready_for_human(db2, conn, "impl", "ready")
|
|
assert db2.comments[-1][2] == "ready"
|
|
|
|
|
|
def test_comment_and_block_failures_are_swallowed(capsys):
|
|
class BrokenDb(RecordingDb):
|
|
def add_comment(self, *_a, **_k):
|
|
raise RuntimeError("comment down")
|
|
|
|
def block_task(self, *_a, **_k):
|
|
raise RuntimeError("block down")
|
|
|
|
db = BrokenDb([])
|
|
conn = SimpleNamespace(close=lambda: None)
|
|
supervisor._escalate(db, conn, "impl", "reason")
|
|
err = capsys.readouterr().err
|
|
assert "could not block impl" in err and "could not comment on impl" in err
|
|
|
|
|
|
# --- across-ticks idempotency (the re-emission bug) -----------------------
|
|
|
|
|
|
def _ship_review(commit="c1"):
|
|
return _task(
|
|
id="rev",
|
|
metadata={"supervisor": {"kind": "review", "root": "impl", "parent": "impl", "head_commit": commit, "cycle": 1}},
|
|
result={"verdict": "SHIP", "summary": "clean"},
|
|
)
|
|
|
|
|
|
def _cycle_exhausted_review():
|
|
return _task(
|
|
id="rev",
|
|
metadata={"supervisor": {"kind": "review", "root": "impl", "parent": "impl", "head_commit": "c1", "cycle": 5}},
|
|
result={"verdict": "BLOCK", "findings": ["still broken"]},
|
|
)
|
|
|
|
|
|
def _cycle_exhausted_repair():
|
|
return _task(
|
|
id="rep",
|
|
metadata={"supervisor": {"kind": "repair", "root": "impl", "parent": "rev", "head_commit": "c1", "cycle": 5}},
|
|
result={"head_commit": "c2"},
|
|
)
|
|
|
|
|
|
def test_ship_marks_ready_exactly_once_across_ten_ticks():
|
|
review = _ship_review()
|
|
db = RecordingDb([review])
|
|
for _ in range(10):
|
|
supervisor.supervise_once(db, policy.Limits())
|
|
ready_comments = [c for c in db.comments if "READY FOR HUMAN MERGE" in c[2]]
|
|
assert len(ready_comments) == 1
|
|
assert len(db.metadata_sets) == 1
|
|
assert db.created == [] and db.blocked == []
|
|
|
|
|
|
def test_review_cycle_limit_escalates_exactly_once_across_ten_ticks():
|
|
# block_task does not move the card out of done here, so the ledger - not a
|
|
# status transition - must be what stops re-emission.
|
|
db = RecordingDb([_cycle_exhausted_review()])
|
|
for _ in range(10):
|
|
supervisor.supervise_once(db, policy.Limits())
|
|
assert len(db.blocked) == 1 and db.blocked[0][0] == "rev"
|
|
assert len([c for c in db.comments if c[0] == "rev"]) == 1
|
|
assert db.created == []
|
|
|
|
|
|
def test_repair_cycle_limit_escalates_exactly_once_across_ten_ticks():
|
|
db = RecordingDb([_cycle_exhausted_repair()])
|
|
for _ in range(10):
|
|
supervisor.supervise_once(db, policy.Limits())
|
|
assert len(db.blocked) == 1 and db.blocked[0][0] == "rep"
|
|
assert len([c for c in db.comments if c[0] == "rep"]) == 1
|
|
|
|
|
|
def test_supervise_once_accepts_an_injected_ledger():
|
|
review = _ship_review()
|
|
db = RecordingDb([review])
|
|
ledger = supervisor.Ledger(supervisor.LEDGER_PATH)
|
|
supervisor.supervise_once(db, policy.Limits(), ledger)
|
|
supervisor.supervise_once(db, policy.Limits(), ledger)
|
|
assert len([c for c in db.comments if "READY FOR HUMAN MERGE" in c[2]]) == 1
|
|
|
|
|
|
def test_ledger_survives_reload_from_disk():
|
|
ledger = supervisor.Ledger(supervisor.LEDGER_PATH)
|
|
assert ledger.has("ship:impl") is False
|
|
ledger.record("ship:impl")
|
|
reloaded = supervisor.Ledger(supervisor.LEDGER_PATH)
|
|
assert reloaded.has("ship:impl") is True
|
|
|
|
|
|
def test_ledger_tolerates_corrupt_or_unwritable_state(monkeypatch, tmp_path, capsys):
|
|
corrupt = tmp_path / "corrupt.json"
|
|
corrupt.write_text("{not json", encoding="utf-8")
|
|
assert supervisor.Ledger(corrupt).has("x") is False
|
|
# A record failure is swallowed and logged, never raised into the tick.
|
|
monkeypatch.setattr(supervisor, "LEDGER_PATH", tmp_path / "missing" / "x")
|
|
bad = supervisor.Ledger(supervisor.LEDGER_PATH)
|
|
monkeypatch.setattr(supervisor.os, "replace", lambda *_a: (_ for _ in ()).throw(OSError("ro")))
|
|
bad.record("k")
|
|
assert "could not persist emission ledger" in capsys.readouterr().err
|
|
|
|
|
|
def test_spawn_typeerror_does_not_double_insert_when_row_already_exists():
|
|
review = _task(
|
|
id="rev",
|
|
metadata={"supervisor": {"kind": "review", "root": "impl", "parent": "impl", "head_commit": "c1", "cycle": 1}},
|
|
status="ready",
|
|
)
|
|
|
|
class InsertThenTypeErrorDb(RecordingDb):
|
|
def create_task(self, _conn, **kwargs):
|
|
# Simulate an insert that lands but still raises TypeError.
|
|
self.created.append(kwargs)
|
|
self._tasks.append(review)
|
|
raise TypeError("post-insert boom")
|
|
|
|
impl = _task(
|
|
id="impl",
|
|
result={"changed_files": ["a"], "head_commit": "c1", "pull_request": "pr"},
|
|
)
|
|
db = InsertThenTypeErrorDb([impl])
|
|
supervisor.supervise_once(db, policy.Limits())
|
|
assert len(db.created) == 1 # retry suppressed by the re-scan guard
|
|
|
|
|
|
def test_already_created_returns_false_without_stamp_or_on_scan_error():
|
|
db = RecordingDb([])
|
|
conn = SimpleNamespace(close=lambda: None)
|
|
assert supervisor._already_created(db, conn, {"metadata": {}}) is False
|
|
|
|
class BrokenScanDb(RecordingDb):
|
|
def list_tasks(self, _conn):
|
|
raise RuntimeError("scan down")
|
|
|
|
payload = {"metadata": {"supervisor": {"kind": "review", "root": "impl", "head_commit": "c1"}}}
|
|
assert supervisor._already_created(BrokenScanDb([]), conn, payload) is False
|
|
|
|
|
|
# --- fault isolation ------------------------------------------------------
|
|
|
|
|
|
def test_board_action_failure_does_not_stop_the_pass(capsys):
|
|
good = _task(
|
|
id="impl",
|
|
result={"changed_files": ["a"], "head_commit": "c1", "pull_request": "pr"},
|
|
)
|
|
db = RecordingDb([good], raise_on={"create_task"})
|
|
# create_task raises -> action fails, but the pass completes without crashing.
|
|
assert supervisor.supervise_once(db, policy.Limits()) == 0
|
|
assert "action failed on board" in capsys.readouterr().err
|
|
|
|
|
|
def test_non_actionable_task_is_a_clean_no_op():
|
|
idle = _task(id="idle", status="running")
|
|
db = RecordingDb([idle])
|
|
assert supervisor.supervise_once(db, policy.Limits()) == 0
|
|
assert db.created == [] and db.blocked == [] and db.comments == []
|
|
|
|
|
|
def test_board_connect_failure_is_isolated(capsys):
|
|
db = RecordingDb([], boards=[{"slug": "cassandra"}], raise_on={"connect"})
|
|
assert supervisor.supervise_once(db, policy.Limits()) == 0
|
|
assert "temporarily skipping board" in capsys.readouterr().err
|
|
|
|
|
|
# --- poll loop / auto_supervise gate -------------------------------------
|
|
|
|
|
|
def test_loop_supervises_when_enabled(monkeypatch):
|
|
calls = []
|
|
monkeypatch.setattr(supervisor, "supervise_once", lambda db, limits: calls.append(limits) or 0)
|
|
settings = supervisor.Settings(enabled=True, interval=0, limits=policy.Limits())
|
|
sleeps = []
|
|
supervisor.run_forever(
|
|
object(), sleep=sleeps.append, load=lambda: settings, max_ticks=2
|
|
)
|
|
assert len(calls) == 2 and sleeps == [0, 0]
|
|
|
|
|
|
def test_loop_is_inert_when_flag_off(monkeypatch):
|
|
calls = []
|
|
monkeypatch.setattr(supervisor, "supervise_once", lambda *a: calls.append(a) or 0)
|
|
settings = supervisor.Settings(enabled=False, interval=0, limits=policy.Limits())
|
|
supervisor.run_forever(object(), sleep=lambda _s: None, load=lambda: settings, max_ticks=3)
|
|
assert calls == []
|
|
|
|
|
|
def test_loop_survives_a_failing_tick(monkeypatch, capsys):
|
|
def boom(_db, _limits):
|
|
raise RuntimeError("tick blew up")
|
|
|
|
monkeypatch.setattr(supervisor, "supervise_once", boom)
|
|
settings = supervisor.Settings(enabled=True, interval=0, limits=policy.Limits())
|
|
supervisor.run_forever(object(), sleep=lambda _s: None, load=lambda: settings, max_ticks=1)
|
|
assert "tick failed" in capsys.readouterr().err
|
|
|
|
|
|
def test_main_wires_the_runtime_kanban_db(monkeypatch):
|
|
seen = {}
|
|
monkeypatch.setitem(
|
|
__import__("sys").modules, "hermes_cli", SimpleNamespace(kanban_db="RUNTIME")
|
|
)
|
|
monkeypatch.setattr(supervisor, "run_forever", lambda db: seen.setdefault("db", db))
|
|
assert supervisor.main([]) == 0
|
|
assert seen["db"] == "RUNTIME"
|
|
|
|
|
|
# --- deployment / config wiring ------------------------------------------
|
|
|
|
|
|
def test_sidecar_is_deployed_with_hardened_non_metered_posture():
|
|
spec = _agent_deployment()["spec"]["template"]["spec"]
|
|
sidecar = next(c for c in spec["containers"] if c["name"] == "kanban-supervisor")
|
|
assert sidecar["command"][-1].endswith("kanban_supervisor.py")
|
|
sec = sidecar["securityContext"]
|
|
assert sec["runAsNonRoot"] is True and sec["runAsUser"] == 10000
|
|
assert sec["allowPrivilegeEscalation"] is False
|
|
assert sec["readOnlyRootFilesystem"] is True
|
|
assert sec["capabilities"]["drop"] == ["ALL"]
|
|
mounts = {m["name"] for m in sidecar["volumeMounts"]}
|
|
# No runtime-access mount: it holds no provider credential / metered path.
|
|
assert "runtime-access" not in mounts
|
|
assert {"home", "coordinator"} <= mounts
|
|
|
|
|
|
def test_auto_supervise_flag_defaults_false_in_configmap():
|
|
documents = list(
|
|
yaml.safe_load_all((HERMES / "agent-configmap.yaml").read_text(encoding="utf-8"))
|
|
)
|
|
config_doc = next(
|
|
doc for doc in documents if doc and doc.get("metadata", {}).get("name") == "hermes-agent-config"
|
|
)
|
|
payload = yaml.safe_load(config_doc["data"]["config.yaml"])
|
|
assert payload["kanban"]["auto_supervise"] is False
|
|
|
|
|
|
def test_supervisor_scripts_registered_in_coordinator_configmap():
|
|
kustomization = yaml.safe_load((HERMES / "kustomization.yaml").read_text(encoding="utf-8"))
|
|
coordinator = next(
|
|
gen for gen in kustomization["configMapGenerator"] if gen["name"] == "hermes-coordinator"
|
|
)
|
|
joined = "\n".join(coordinator["files"])
|
|
assert "kanban_supervisor.py=scripts/kanban_supervisor.py" in joined
|
|
assert "supervisor_policy.py=scripts/supervisor_policy.py" in joined
|
|
|
|
|
|
# --- the confirmed NULL -> 20 goal-turn fallback fix ----------------------
|
|
|
|
|
|
def test_goal_max_turns_defaults_to_twenty_not_one():
|
|
source = (HERMES / "scripts/cli_lane_execution.py").read_text(encoding="utf-8")
|
|
assert 'int(_task_value(task, "goal_max_turns", 20) or 20)' in source
|
|
assert 'int(_task_value(task, "goal_max_turns", 1) or 1)' not in source
|